Check that parent_tag is in an ancestor of the checked out branch #211
Conversation
jengelh
force-pushed
the
master
branch
3 times, most recently
from
219fd5c
to
c25018e
Compare
There was a problem hiding this comment.
Thanks, this sounds like a great improvement, and I'm sorry I didn't review this before - I continue to be mystified why I don't always receive email notifications from GitHub when new issues/PRs are submitted 😠
Please could you expand the commit message to explain the scenarios in which parent_tag would not be an ancestor? Is it only if parent_tag is specified manually as a parameter? Presumably if it's not specified and is calculated via _detect_parent_tag then git describe --tags will always choose an ancestor, at least according to my reading of the git-describe(1) man page.
Two other suggestions: I think the code for checking ancestry would benefit from being extracted into a separate function call to avoid _detect_version_tag_offset getting too long and unwieldly, and secondly please can we have a unit test for this?
Thanks a lot!
combined with is such an insatisfiable combination. |
|
@jengelh Sure, but that doesn't answer my previous question:
Emphasis on "only", i.e. could it also happen if |
|
When parent-tag is implicit, the tag git-describe chooses is already known to be an ancestor. Only when this is explicitly overridden with a different |
|
OK good, so we are agreed :-) |
|
message updated |
|
Thanks for the message update, that looks good now. I still think my previous comment is relevant though:
|
TarSCM/scm/git.py
Outdated
| ) | ||
| if rcode: | ||
| sys.exit("\033[31m`git rev-parse " + parent_tag + "` failed: " + | ||
| output + "\033[0m") |
There was a problem hiding this comment.
Travis correctly spotted that output is not defined here.
TarSCM/scm/git.py
Outdated
| ) | ||
| if rcode: | ||
| sys.exit("\033[31m`git rev-parse HEAD` failed: " + | ||
| output + "\033[0m") |
TarSCM/scm/git.py
Outdated
| sys.exit(color_red("@TAG_OFFSET@ cannot be expanded, as no parent tag was discovered.")) | ||
| self.check_ancestry(parent_tag) | ||
| rcode, output = self.helpers.run_cmd( | ||
| self._get_scm_cmd() + ['rev-list', '--count', parent_tag + '..HEAD'], |
There was a problem hiding this comment.
line too long (81 > 79 characters)
TarSCM/scm/git.py
Outdated
| cmd = self._get_scm_cmd() | ||
| cmd.extend(['rev-list', '--count', parent_tag + '..HEAD']) | ||
| rcode, out = self.helpers.run_cmd(cmd, self.clone_dir) | ||
| sys.exit(color_red("@TAG_OFFSET@ cannot be expanded, as no parent tag was discovered.")) |
There was a problem hiding this comment.
line too long (100 > 79 characters)
| @@ -6,6 +6,10 @@ | |||
|
|
|||
| from TarSCM.scm.base import Scm | |||
|
|
|||
| def color_red(s): | |||
There was a problem hiding this comment.
this one makes no sense, there's just one line above every def
|
Just saw this pop in my notification. The easiest way to check if a commit is an ancestor is git merge-base --is-ancestor Returns 0 if it is an ancestor, 1 if it isn't. |
`git rev-list A..B` returns the set of commits present in B but not A. Only if A is an ancestor of B will the cardinality of the set be the same as the forward-running offset from A to B. When parent-tag is implicit, the tag `git-describe` chooses is already known to be an ancestor. Only when this is explicitly overridden with a different parent-tag in the _service file, it will be possible for the chosen parent-tag value not to refer to a commit that is a valid ancestor.
git rev-list A..Breturns the set of commits present in B but notA. Only if A is an ancestor of B will the cardinality of the set be
the same as the forward-running offset from A to B.