* [PATCH 0/3] describe --contains sanity @ 2017-03-15 13:15 Michael J Gruber 2017-03-15 13:15 ` [PATCH 1/3] describe: debug is incompatible with contains Michael J Gruber ` (2 more replies) 0 siblings, 3 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-15 13:15 UTC (permalink / raw) To: git 2 patches and 1 RFD around describe (--contains). They are technically independent, but happened along the same stroll in that area when I tried to match documentation, my expectations, and reality. 1 and 2 should be no-brainers. 3 is something to ponder for a while. Michael J Gruber (3): describe: debug is incompatible with contains git-prompt: add a describe style for any tags name-rev: Allow lightweight tags and branch refs Documentation/git-describe.txt | 2 +- builtin/describe.c | 2 ++ builtin/name-rev.c | 2 ++ contrib/completion/git-prompt.sh | 3 +++ t/t9903-bash-prompt.sh | 2 +- 5 files changed, 9 insertions(+), 2 deletions(-) -- 2.12.0.384.g157040b11f.dirty ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 1/3] describe: debug is incompatible with contains 2017-03-15 13:15 [PATCH 0/3] describe --contains sanity Michael J Gruber @ 2017-03-15 13:15 ` Michael J Gruber 2017-03-15 19:21 ` Junio C Hamano 2017-03-15 13:15 ` [PATCH 2/3] git-prompt: add a describe style for any tags Michael J Gruber 2017-03-15 13:15 ` [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs Michael J Gruber 2 siblings, 1 reply; 46+ messages in thread From: Michael J Gruber @ 2017-03-15 13:15 UTC (permalink / raw) To: git `git describe --contains` calls into `git name-rev` which does not have any searching to do and thus does not display any debug information. Say so in the documentation and catch the incompatible arguments. Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> --- Documentation/git-describe.txt | 2 +- builtin/describe.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt index 8755f3af7b..0f9adb6e9a 100644 --- a/Documentation/git-describe.txt +++ b/Documentation/git-describe.txt @@ -69,7 +69,7 @@ OPTIONS --debug:: Verbosely display information about the searching strategy being employed to standard error. The tag name will still - be printed to standard out. + be printed to standard out. This is incompatible with --contains. --long:: Always output the long format (the tag, the number of commits diff --git a/builtin/describe.c b/builtin/describe.c index 76c18059bf..01a6d159a0 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -462,6 +462,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix) if (longformat && abbrev == 0) die(_("--long is incompatible with --abbrev=0")); + if (contains && debug) + die(_("--debug is incompatible with --contains")); if (contains) { struct string_list_item *item; -- 2.12.0.384.g157040b11f.dirty ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 1/3] describe: debug is incompatible with contains 2017-03-15 13:15 ` [PATCH 1/3] describe: debug is incompatible with contains Michael J Gruber @ 2017-03-15 19:21 ` Junio C Hamano 2017-03-17 10:54 ` Michael J Gruber 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-15 19:21 UTC (permalink / raw) To: Michael J Gruber; +Cc: git Michael J Gruber <git@drmicha.warpmail.net> writes: > `git describe --contains` calls into `git name-rev` which does not have > any searching to do and thus does not display any debug information. > > Say so in the documentation and catch the incompatible arguments. I am not sure if this is worth it. Those who are really doing the debugging would be staring at the code while running it anyway, so it is not like this new error condition would help anybody from wasting time scratching her head before viewing the source and realize that the underlying name-rev does not honor the option. If "--debug" is truly valuable, "name-rev" can gain "--debug" later and then we can pass it down if we want. Also, it is not like "--debug" is incompatible. It is just the "--contains" codepath is overly silent and does not give any useful information when run in the debug mode. "incompatible" is more like "would not work correctly when both are given", which is not the case here. > > Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> > --- > Documentation/git-describe.txt | 2 +- > builtin/describe.c | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt > index 8755f3af7b..0f9adb6e9a 100644 > --- a/Documentation/git-describe.txt > +++ b/Documentation/git-describe.txt > @@ -69,7 +69,7 @@ OPTIONS > --debug:: > Verbosely display information about the searching strategy > being employed to standard error. The tag name will still > - be printed to standard out. > + be printed to standard out. This is incompatible with --contains. > > --long:: > Always output the long format (the tag, the number of commits > diff --git a/builtin/describe.c b/builtin/describe.c > index 76c18059bf..01a6d159a0 100644 > --- a/builtin/describe.c > +++ b/builtin/describe.c > @@ -462,6 +462,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix) > > if (longformat && abbrev == 0) > die(_("--long is incompatible with --abbrev=0")); > + if (contains && debug) > + die(_("--debug is incompatible with --contains")); > > if (contains) { > struct string_list_item *item; ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 1/3] describe: debug is incompatible with contains 2017-03-15 19:21 ` Junio C Hamano @ 2017-03-17 10:54 ` Michael J Gruber 0 siblings, 0 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-17 10:54 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano venit, vidit, dixit 15.03.2017 20:21: > Michael J Gruber <git@drmicha.warpmail.net> writes: > >> `git describe --contains` calls into `git name-rev` which does not have >> any searching to do and thus does not display any debug information. >> >> Say so in the documentation and catch the incompatible arguments. > > I am not sure if this is worth it. Those who are really doing the > debugging would be staring at the code while running it anyway, so > it is not like this new error condition would help anybody from > wasting time scratching her head before viewing the source and > realize that the underlying name-rev does not honor the option. The story was: I tried to understand why git describe --contains did not do what I expected. The documentation said that --debug would output candidates, but it did not do anything. So, instead of learning how git describe --contains works, I got side-tracked into understanding why git describe --contains does not do what the documentation says. That was a waste of time that we can avoid. "viewing the source" should not be necessary to understand what is going on, should it? > If "--debug" is truly valuable, "name-rev" can gain "--debug" later > and then we can pass it down if we want. > > Also, it is not like "--debug" is incompatible. It is just the > "--contains" codepath is overly silent and does not give any useful > information when run in the debug mode. "incompatible" is more like > "would not work correctly when both are given", which is not the > case here. Well, thee notion of giving debug output is certainly not incompatible. But if the "--debug" does not output anything with "--contains" then it is not working, which I would call incompatible (implementation, not concept). >> >> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> >> --- >> Documentation/git-describe.txt | 2 +- >> builtin/describe.c | 2 ++ >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt >> index 8755f3af7b..0f9adb6e9a 100644 >> --- a/Documentation/git-describe.txt >> +++ b/Documentation/git-describe.txt >> @@ -69,7 +69,7 @@ OPTIONS >> --debug:: >> Verbosely display information about the searching strategy >> being employed to standard error. The tag name will still >> - be printed to standard out. >> + be printed to standard out. This is incompatible with --contains. >> >> --long:: >> Always output the long format (the tag, the number of commits >> diff --git a/builtin/describe.c b/builtin/describe.c >> index 76c18059bf..01a6d159a0 100644 >> --- a/builtin/describe.c >> +++ b/builtin/describe.c >> @@ -462,6 +462,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix) >> >> if (longformat && abbrev == 0) >> die(_("--long is incompatible with --abbrev=0")); >> + if (contains && debug) >> + die(_("--debug is incompatible with --contains")); >> >> if (contains) { >> struct string_list_item *item; ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 2/3] git-prompt: add a describe style for any tags 2017-03-15 13:15 [PATCH 0/3] describe --contains sanity Michael J Gruber 2017-03-15 13:15 ` [PATCH 1/3] describe: debug is incompatible with contains Michael J Gruber @ 2017-03-15 13:15 ` Michael J Gruber 2017-03-15 19:25 ` Junio C Hamano 2017-03-15 13:15 ` [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs Michael J Gruber 2 siblings, 1 reply; 46+ messages in thread From: Michael J Gruber @ 2017-03-15 13:15 UTC (permalink / raw) To: git git-prompt has various describe styles, among them "describe" (by annotated tags) and "default" (by exact match with any tag). Add a mode "tag" that describes by any tag, annotated or not. Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> --- contrib/completion/git-prompt.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index 97eacd7832..c6cbef38c2 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -82,6 +82,7 @@ # contains relative to newer annotated tag (v1.6.3.2~35) # branch relative to newer tag or branch (master~4) # describe relative to older annotated tag (v1.6.3.1-13-gdd42c2f) +# tag relative to any older tag (v1.6.3.1-13-gdd42c2f) # default exactly matching tag # # If you would like a colored hint about the current dirty state, set @@ -443,6 +444,8 @@ __git_ps1 () git describe --contains HEAD ;; (branch) git describe --contains --all HEAD ;; + (tag) + git describe --tags HEAD ;; (describe) git describe HEAD ;; (* | default) -- 2.12.0.384.g157040b11f.dirty ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 2/3] git-prompt: add a describe style for any tags 2017-03-15 13:15 ` [PATCH 2/3] git-prompt: add a describe style for any tags Michael J Gruber @ 2017-03-15 19:25 ` Junio C Hamano 2017-03-17 10:56 ` Michael J Gruber 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-15 19:25 UTC (permalink / raw) To: Michael J Gruber; +Cc: git Michael J Gruber <git@drmicha.warpmail.net> writes: > git-prompt has various describe styles, among them "describe" (by > annotated tags) and "default" (by exact match with any tag). > > Add a mode "tag" that describes by any tag, annotated or not. > > Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> > --- > contrib/completion/git-prompt.sh | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh > index 97eacd7832..c6cbef38c2 100644 > --- a/contrib/completion/git-prompt.sh > +++ b/contrib/completion/git-prompt.sh > @@ -82,6 +82,7 @@ > # contains relative to newer annotated tag (v1.6.3.2~35) > # branch relative to newer tag or branch (master~4) > # describe relative to older annotated tag (v1.6.3.1-13-gdd42c2f) > +# tag relative to any older tag (v1.6.3.1-13-gdd42c2f) I guess this feature makes sense. I just wish we had a well-known unannotated tag we can use for such an example; using v1.6.3.1 that is annotated does not help to make the distinctin between describe and tag stand out. We want to convey "both annotated one and unannotated one can be used". I am wondering if it makes sense to do something like this instead: # tag similar to 'describe' but also allow unannotated tags > # default exactly matching tag > # > # If you would like a colored hint about the current dirty state, set > @@ -443,6 +444,8 @@ __git_ps1 () > git describe --contains HEAD ;; > (branch) > git describe --contains --all HEAD ;; > + (tag) > + git describe --tags HEAD ;; > (describe) > git describe HEAD ;; > (* | default) ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/3] git-prompt: add a describe style for any tags 2017-03-15 19:25 ` Junio C Hamano @ 2017-03-17 10:56 ` Michael J Gruber 0 siblings, 0 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-17 10:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano venit, vidit, dixit 15.03.2017 20:25: > Michael J Gruber <git@drmicha.warpmail.net> writes: > >> git-prompt has various describe styles, among them "describe" (by >> annotated tags) and "default" (by exact match with any tag). >> >> Add a mode "tag" that describes by any tag, annotated or not. >> >> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> >> --- >> contrib/completion/git-prompt.sh | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh >> index 97eacd7832..c6cbef38c2 100644 >> --- a/contrib/completion/git-prompt.sh >> +++ b/contrib/completion/git-prompt.sh >> @@ -82,6 +82,7 @@ >> # contains relative to newer annotated tag (v1.6.3.2~35) >> # branch relative to newer tag or branch (master~4) >> # describe relative to older annotated tag (v1.6.3.1-13-gdd42c2f) >> +# tag relative to any older tag (v1.6.3.1-13-gdd42c2f) > > I guess this feature makes sense. > > I just wish we had a well-known unannotated tag we can use for such > an example; using v1.6.3.1 that is annotated does not help to make > the distinctin between describe and tag stand out. We want to > convey "both annotated one and unannotated one can be used". > > I am wondering if it makes sense to do something like this instead: > > # tag similar to 'describe' but also allow unannotated tags or inventing a local lightweight tag such as "lastgood-5-gbadbad"). Either way would be fine with me. (I guess it's in next as is now.) >> # default exactly matching tag >> # >> # If you would like a colored hint about the current dirty state, set >> @@ -443,6 +444,8 @@ __git_ps1 () >> git describe --contains HEAD ;; >> (branch) >> git describe --contains --all HEAD ;; >> + (tag) >> + git describe --tags HEAD ;; >> (describe) >> git describe HEAD ;; >> (* | default) ^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs 2017-03-15 13:15 [PATCH 0/3] describe --contains sanity Michael J Gruber 2017-03-15 13:15 ` [PATCH 1/3] describe: debug is incompatible with contains Michael J Gruber 2017-03-15 13:15 ` [PATCH 2/3] git-prompt: add a describe style for any tags Michael J Gruber @ 2017-03-15 13:15 ` Michael J Gruber 2017-03-15 16:14 ` Junio C Hamano 2017-03-15 19:50 ` Junio C Hamano 2 siblings, 2 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-15 13:15 UTC (permalink / raw) To: git When name-rev constructs possible names for a rev, it assigns the taggerdate to an annotated tag and ULONG_MAX to other names such as lightweight tags and branch names. Practically, this rules out even naming a tagged commit by the tag if that is lightweight and there is another possible indirect name (e.g. foo~5) coming from an annotated tag. Instead, assign the commit date to lightweight tags or branch refs so that they get their fair chance of being picked up. Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> --- Originally, I didn't even think of submitting this as is until I noticed that all tests succeed with it (bar one for git-prompt.h which looked strange anyways); rather, I thought about another switch "--lightweight" or so. I still submit it as RFD. This origin of my foray into name-rev came from git describe, where a fair expectation should be that "--contains" mode is the same as the odinary mode, albeit with a different direction in time/traversal. (Technically, describe --contains is name-rev.) Consider the following: git init echo past >tense git add tense git commit -m past git tag -m past -a past echo present >tense git commit -am present git tag present echo future >tense git commit -am future "git describe past present future" gives past past-1-g5ad942f future because (as documented) it does not consider lightweight tags, and thus has to describe present from the past. "git describe --tags past present future" gives past present future because (as documented) it does consider lightweight tags. "git describe --contains past present future" gives past^0 future~1 future^0 and I have a hard time matching that with the documentation (describe doc claims that --tags is automatic; name-rev doc does not distinguish between tag types). "--tags" does not make any difference here, nor does "--all" (besides fully qualifying the tags). "git describe --contains past present future" gives past^0 present future^0 with this patch (I'm tempted to say: with the present patch), which is what I would expect. I'm wondering whether I'm overlooking any side-effects that our test suite doesn't cover, though. In any case, we may want to have lightweight tags allowed based on an extra flag (like the existing --tags for describe, which means something else for name-rev). builtin/name-rev.c | 2 ++ t/t9903-bash-prompt.sh | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 8bdc3eaa6f..75ba7bbad5 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -207,6 +207,8 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo if (o && o->type == OBJ_COMMIT) { struct commit *commit = (struct commit *)o; + if (taggerdate == ULONG_MAX) /* lightweight tag */ + taggerdate = commit->date; path = name_ref_abbrev(path, can_abbreviate_output); name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref); } diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh index 97c9b32c2e..d467d5957d 100755 --- a/t/t9903-bash-prompt.sh +++ b/t/t9903-bash-prompt.sh @@ -107,7 +107,7 @@ test_expect_success 'prompt - describe detached head - contains' ' ' test_expect_success 'prompt - describe detached head - branch' ' - printf " ((tags/t2~1))" >expected && + printf " ((b1~1))" >expected && git checkout b1^ && test_when_finished "git checkout master" && ( -- 2.12.0.384.g157040b11f.dirty ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs 2017-03-15 13:15 ` [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs Michael J Gruber @ 2017-03-15 16:14 ` Junio C Hamano 2017-03-15 19:50 ` Junio C Hamano 1 sibling, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-15 16:14 UTC (permalink / raw) To: Michael J Gruber; +Cc: git Michael J Gruber <git@drmicha.warpmail.net> writes: > Consider the following: > ... > "git describe past present future" gives > > past > past-1-g5ad942f > future > > because (as documented) it does not consider lightweight tags, and thus > has to describe present from the past. > > "git describe --tags past present future" gives > > past > present > future > > because (as documented) it does consider lightweight tags. > > "git describe --contains past present future" gives > > past^0 > future~1 > future^0 Nice illustration to convince anybody that "git describe --tags --contains" should give "past present future" (or "past^0 present future^0"). I am not sure if it is a good idea in general to change the output to include "present" when "--tags" is not given, though. For _this_ example, it does give us a more useful result, but the example was to constructed to illustrate the case where the new behaviour is more useful, and does not serve to convince us that it won't have negative impact X-<. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs 2017-03-15 13:15 ` [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs Michael J Gruber 2017-03-15 16:14 ` Junio C Hamano @ 2017-03-15 19:50 ` Junio C Hamano 2017-03-15 20:51 ` Junio C Hamano 1 sibling, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-15 19:50 UTC (permalink / raw) To: Michael J Gruber; +Cc: git Michael J Gruber <git@drmicha.warpmail.net> writes: > I'm wondering whether I'm overlooking any side-effects that our test > suite doesn't cover, though. In any case, we may want to have > lightweight tags allowed based on an extra flag (like the > existing --tags for describe, which means something else for name-rev). > > builtin/name-rev.c | 2 ++ > t/t9903-bash-prompt.sh | 2 +- > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/name-rev.c b/builtin/name-rev.c > index 8bdc3eaa6f..75ba7bbad5 100644 > --- a/builtin/name-rev.c > +++ b/builtin/name-rev.c > @@ -207,6 +207,8 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo > if (o && o->type == OBJ_COMMIT) { > struct commit *commit = (struct commit *)o; > > + if (taggerdate == ULONG_MAX) /* lightweight tag */ > + taggerdate = commit->date; > path = name_ref_abbrev(path, can_abbreviate_output); > name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref); > } I think this comment indicates where this change will bite us ;-) This could have been an invocation of "name-rev" without "--tags", where _any_ tip of ref can be used to name a revision, and in such a case, retaining commit->date may still be valuable, but it is probably wrong to use it for nothing more than tie-breaking. In an extreme case, imagine that your repository does not have any tag and you have a commit that can be reached from both 'maint' and 'master' branches. Because the current code assigns the same useless taggerdate for these tips of refs, the commit is described solely based on the distance from the tip of 'maint' or 'master' with today's code. If you assign commit date to taggerdate variable here, that date is used to trump the distance when deciding whether to name the commit based on 'maint' and 'master', and I doubt it is what you want. Perhaps I regularly advance 'master', but still add a few selected fixes to 'maint' every few days---when these two branches both are active like that, do you want the same commit that is topologically much closer to 'maint' than 'master' to be described in terms of 'maint' on some days (i.e. those days I added fixes to it) and 'master' on some other days? I think the expectation by end-users who do not use "--tags" is that they still prefer a commit to be named in terms of the oldest tag that is fewer number of hops away, and the naming to fall back to non-tag refs only when there is no tags that reach the commit. And between two or more non-tag tips that are _known_ to be still active (iow, branch tips), they want "fewer number of hops away" to be the primary deciding factor to choose among them. The ULONG_MAX setting the code currently uses is very suitable for that purpose. It penalizes the non-tag refs, and it disables the use of timestamp for deciding among these tips and uses only the topological distance. I think you can do this change _ONLY_ when we are operating under the "--tags" option. That would place unannotated tags at a better location in the timestamp order than the current code does, without introducing issues with refs that are actively moving. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs 2017-03-15 19:50 ` Junio C Hamano @ 2017-03-15 20:51 ` Junio C Hamano 2017-03-15 22:50 ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-15 20:51 UTC (permalink / raw) To: Michael J Gruber; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > This could have been an invocation of "name-rev" without "--tags", > where _any_ tip of ref can be used to name a revision, and in such a > case, retaining commit->date may still be valuable, but it is > probably wrong to use it for nothing more than tie-breaking. > ... > I think you can do this change _ONLY_ when we are operating under > the "--tags" option. That would place unannotated tags at a better > location in the timestamp order than the current code does, without > introducing issues with refs that are actively moving. I'll leave "only do so under --tags" as an exercise to the reader, but if we want to use the timestamp for tie-breaking between names based on two branches, a patch to do so may look like this one. I am presenting it as a single ball of wax, but in the real history I have (which I am not publishing, as I haven't decided if this is a good direction to go in the first place), this is a two-patch series, the first one that factors out is_better_name() without doing anything else, followed by a commit that adds "from_tag" and passes it throughout the callpath to allow is_better_name() to use it to: - always favor names based on a tag; - between two names based on tags, favor name based on an older tag and tiebreak with distance; - between two names based on non-tags, favor name with shorter hops and tiebreak with age. builtin/name-rev.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index cd89d48b65..ba30c9ca23 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -13,6 +13,7 @@ typedef struct rev_name { unsigned long taggerdate; int generation; int distance; + int from_tag; } rev_name; static long cutoff = LONG_MAX; @@ -20,9 +21,31 @@ static long cutoff = LONG_MAX; /* How many generations are maximally preferred over _one_ merge traversal? */ #define MERGE_TRAVERSAL_WEIGHT 65535 +static int is_better_name(struct rev_name *name, + const char *tip_name, + unsigned long taggerdate, + int generation, + int distance, + int from_tag) +{ + if (name->from_tag > from_tag) + return 0; + else if (name->from_tag < from_tag) + return 1; + + if (from_tag) + return (name->taggerdate > taggerdate || + (name->taggerdate == taggerdate && + name->distance > distance)); + else + return (name->distance > distance || + (name->distance == distance && + name->taggerdate > taggerdate)); +} + static void name_rev(struct commit *commit, const char *tip_name, unsigned long taggerdate, - int generation, int distance, + int generation, int distance, int from_tag, int deref) { struct rev_name *name = (struct rev_name *)commit->util; @@ -45,14 +68,14 @@ static void name_rev(struct commit *commit, name = xmalloc(sizeof(rev_name)); commit->util = name; goto copy_data; - } else if (name->taggerdate > taggerdate || - (name->taggerdate == taggerdate && - name->distance > distance)) { + } else if (is_better_name(name, tip_name, taggerdate, + generation, distance, from_tag)) { copy_data: name->tip_name = tip_name; name->taggerdate = taggerdate; name->generation = generation; name->distance = distance; + name->from_tag = from_tag; } else return; @@ -72,10 +95,12 @@ static void name_rev(struct commit *commit, parent_number); name_rev(parents->item, new_name, taggerdate, 0, - distance + MERGE_TRAVERSAL_WEIGHT, 0); + distance + MERGE_TRAVERSAL_WEIGHT, + from_tag, 0); } else { name_rev(parents->item, tip_name, taggerdate, - generation + 1, distance + 1, 0); + generation + 1, distance + 1, + from_tag, 0); } } } @@ -174,9 +199,13 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo } if (o && o->type == OBJ_COMMIT) { struct commit *commit = (struct commit *)o; + int from_tag = starts_with(path, "refs/tags/"); + if (taggerdate == ULONG_MAX) + taggerdate = ((struct commit *)o)->date; path = name_ref_abbrev(path, can_abbreviate_output); - name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref); + name_rev(commit, xstrdup(path), taggerdate, 0, 0, + from_tag, deref); } return 0; } ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags 2017-03-15 20:51 ` Junio C Hamano @ 2017-03-15 22:50 ` Junio C Hamano 2017-03-15 22:50 ` [PATCH 1/2] name-rev: refactor logic to see if a new candidate is a better name Junio C Hamano ` (3 more replies) 0 siblings, 4 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-15 22:50 UTC (permalink / raw) To: git; +Cc: Michael J Gruber Here is a reroll of what I did in http://public-inbox.org/git/xmqqd1die00j.fsf@gitster.mtv.corp.google.com/ Junio C Hamano (2): name-rev: refactor logic to see if a new candidate is a better name name-rev: favor describing with tags and use committer date to tiebreak builtin/name-rev.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++------ t/t4202-log.sh | 2 +- 2 files changed, 57 insertions(+), 8 deletions(-) -- 2.12.0-306-g4a9b9b32d4 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 1/2] name-rev: refactor logic to see if a new candidate is a better name 2017-03-15 22:50 ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano @ 2017-03-15 22:50 ` Junio C Hamano 2017-03-15 22:50 ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Junio C Hamano ` (2 subsequent siblings) 3 siblings, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-15 22:50 UTC (permalink / raw) To: git; +Cc: Michael J Gruber When we encounter a new ref that could describe the commit we are looking at, we compare the name that is formed using that ref and the name we found so far and pick a better one. Factor the comparison logic out to a separate helper function, while keeping the current logic the same (i.e. a name that is based on an older tag is better, and if two tags of the same age can reach the commit, the one with fewer number of hops to reach the commit is better). Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/name-rev.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index cd89d48b65..f64c71d9bc 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -20,6 +20,17 @@ static long cutoff = LONG_MAX; /* How many generations are maximally preferred over _one_ merge traversal? */ #define MERGE_TRAVERSAL_WEIGHT 65535 +static int is_better_name(struct rev_name *name, + const char *tip_name, + unsigned long taggerdate, + int generation, + int distance) +{ + return (name->taggerdate > taggerdate || + (name->taggerdate == taggerdate && + name->distance > distance)); +} + static void name_rev(struct commit *commit, const char *tip_name, unsigned long taggerdate, int generation, int distance, @@ -45,9 +56,8 @@ static void name_rev(struct commit *commit, name = xmalloc(sizeof(rev_name)); commit->util = name; goto copy_data; - } else if (name->taggerdate > taggerdate || - (name->taggerdate == taggerdate && - name->distance > distance)) { + } else if (is_better_name(name, tip_name, taggerdate, + generation, distance)) { copy_data: name->tip_name = tip_name; name->taggerdate = taggerdate; -- 2.12.0-306-g4a9b9b32d4 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak 2017-03-15 22:50 ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano 2017-03-15 22:50 ` [PATCH 1/2] name-rev: refactor logic to see if a new candidate is a better name Junio C Hamano @ 2017-03-15 22:50 ` Junio C Hamano 2017-03-17 4:07 ` Lars Schneider 2017-03-17 11:25 ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber 2017-03-16 0:14 ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Stefan Beller 2017-03-16 10:28 ` Jacob Keller 3 siblings, 2 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-15 22:50 UTC (permalink / raw) To: git; +Cc: Michael J Gruber "git name-rev" assigned a phony "far in the future" date to tips of refs that are not pointing at tag objects, and favored names based on a ref with the oldest date. This made it almost impossible for an unannotated tags and branches to be counted as a viable base, which was especially problematic when the command is run with the "--tags" option. If an unannotated tag that points at an ancient commit and an annotated tag that points at a much newer commit reaches the commit that is being named, the old unannotated tag was ignored. Update the "taggerdate" field of the rev-name structure, which is initialized from the tip of ref, to have the committer date if the object at the tip of ref is a commit, not a tag, so that we can optionally take it into account when doing "is this name better?" comparison logic. When "name-rev" is run without the "--tags" option, the general expectation is still to name the commit based on a tag if possible, but use non-tag refs as fallback, and tiebreak among these non-tag refs by favoring names with shorter hops from the tip. The use of a phony "far in the future" date in the original code was an effective way to ensure this expectation is held: a non-tag tip gets the same "far in the future" timestamp, giving precedence to tags, and among non-tag tips, names with shorter hops are preferred over longer hops, without taking the "taggerdate" into account. As we are taking over the "taggerdate" field to store the committer date for tips with commits: (1) keep the original logic when comparing names based on two refs both of which are from refs/tags/; (2) favoring a name based on a ref in refs/tags/ hierarchy over a ref outside the hierarchy; (3) between two names based on a ref both outside refs/tags/, give precedence to a name with shorter hops and use "taggerdate" only to tie-break. A change to t4202 is a natural consequence. The test creates a commit on a branch "side" and points at it with an unannotated tag "refs/tags/side-2". The original code couldn't decide which one to favor at all, and gave a name based on a branch (simply because refs/heads/side sorts earlier than refs/tags/side-2). Because the updated logic is taught to favor refs in refs/tags/ hierarchy, the the test is updated to expect to see tags/side-2 instead. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * I am sure others can add better heurisitics in is_better_name() for comparing names based on non-tag refs, and I do not mind people disagreeing with the logic that this patch happens to implement. This is done primarily to illustrate the value of using a separate helper function is_better_name() instead of open-coding the selection logic in name_rev() function. --- builtin/name-rev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++--------- t/t4202-log.sh | 2 +- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index f64c71d9bc..eac0180c62 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -13,6 +13,7 @@ typedef struct rev_name { unsigned long taggerdate; int generation; int distance; + int from_tag; } rev_name; static long cutoff = LONG_MAX; @@ -24,16 +25,47 @@ static int is_better_name(struct rev_name *name, const char *tip_name, unsigned long taggerdate, int generation, - int distance) + int distance, + int from_tag) { - return (name->taggerdate > taggerdate || - (name->taggerdate == taggerdate && - name->distance > distance)); + /* + * When comparing names based on tags, prefer names + * based on the older tag, even if it is farther away. + */ + if (from_tag && name->from_tag) + return (name->taggerdate > taggerdate || + (name->taggerdate == taggerdate && + name->distance > distance)); + +#define COMPARE(attribute, smaller_is_better) \ + if (name->attribute > attribute) \ + return smaller_is_better; \ + if (name->attribute < attribute) \ + return !smaller_is_better + + /* + * We know that at least one of them is a non-tag at this point. + * favor a tag over a non-tag. + */ + COMPARE(from_tag, 0); + + /* + * We are now looking at two non-tags. Tiebreak to favor + * shorter hops. + */ + COMPARE(distance, 1); + + /* ... or tiebreak to favor older date */ + COMPARE(taggerdate, 1); + + /* keep the current one if we cannot decide */ + return 0; +#undef COMPARE } static void name_rev(struct commit *commit, const char *tip_name, unsigned long taggerdate, - int generation, int distance, + int generation, int distance, int from_tag, int deref) { struct rev_name *name = (struct rev_name *)commit->util; @@ -57,12 +89,13 @@ static void name_rev(struct commit *commit, commit->util = name; goto copy_data; } else if (is_better_name(name, tip_name, taggerdate, - generation, distance)) { + generation, distance, from_tag)) { copy_data: name->tip_name = tip_name; name->taggerdate = taggerdate; name->generation = generation; name->distance = distance; + name->from_tag = from_tag; } else return; @@ -82,10 +115,12 @@ static void name_rev(struct commit *commit, parent_number); name_rev(parents->item, new_name, taggerdate, 0, - distance + MERGE_TRAVERSAL_WEIGHT, 0); + distance + MERGE_TRAVERSAL_WEIGHT, + from_tag, 0); } else { name_rev(parents->item, tip_name, taggerdate, - generation + 1, distance + 1, 0); + generation + 1, distance + 1, + from_tag, 0); } } } @@ -184,9 +219,13 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo } if (o && o->type == OBJ_COMMIT) { struct commit *commit = (struct commit *)o; + int from_tag = starts_with(path, "refs/tags/"); + if (taggerdate == ULONG_MAX) + taggerdate = ((struct commit *)o)->date; path = name_ref_abbrev(path, can_abbreviate_output); - name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref); + name_rev(commit, xstrdup(path), taggerdate, 0, 0, + from_tag, deref); } return 0; } diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 48b55bfd27..038911f230 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -398,7 +398,7 @@ cat > expect <<\EOF | | | | Merge branch 'side' | | -| * commit side +| * commit tags/side-2 | | Author: A U Thor <author@example.com> | | | | side-2 -- 2.12.0-306-g4a9b9b32d4 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak 2017-03-15 22:50 ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Junio C Hamano @ 2017-03-17 4:07 ` Lars Schneider 2017-03-17 5:45 ` Junio C Hamano 2017-03-17 11:25 ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber 1 sibling, 1 reply; 46+ messages in thread From: Lars Schneider @ 2017-03-17 4:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Michael J Gruber > On 16 Mar 2017, at 06:50, Junio C Hamano <gitster@pobox.com> wrote: > > "git name-rev" assigned a phony "far in the future" date to tips of > refs that are not pointing at tag objects, and favored names based > on a ref with the oldest date. This made it almost impossible for > an unannotated tags and branches to be counted as a viable base, > which was especially problematic when the command is run with the > "--tags" option. If an unannotated tag that points at an ancient > commit and an annotated tag that points at a much newer commit > reaches the commit that is being named, the old unannotated tag was > ignored. > > Update the "taggerdate" field of the rev-name structure, which is > initialized from the tip of ref, to have the committer date if the > object at the tip of ref is a commit, not a tag, so that we can > optionally take it into account when doing "is this name better?" > comparison logic. > > When "name-rev" is run without the "--tags" option, the general > expectation is still to name the commit based on a tag if possible, > but use non-tag refs as fallback, and tiebreak among these non-tag > refs by favoring names with shorter hops from the tip. The use of a > phony "far in the future" date in the original code was an effective > way to ensure this expectation is held: a non-tag tip gets the same > "far in the future" timestamp, giving precedence to tags, and among > non-tag tips, names with shorter hops are preferred over longer > hops, without taking the "taggerdate" into account. As we are > taking over the "taggerdate" field to store the committer date for > tips with commits: > > (1) keep the original logic when comparing names based on two refs > both of which are from refs/tags/; > > (2) favoring a name based on a ref in refs/tags/ hierarchy over > a ref outside the hierarchy; > > (3) between two names based on a ref both outside refs/tags/, give > precedence to a name with shorter hops and use "taggerdate" > only to tie-break. > > A change to t4202 is a natural consequence. The test creates a > commit on a branch "side" and points at it with an unannotated tag > "refs/tags/side-2". The original code couldn't decide which one to > favor at all, and gave a name based on a branch (simply because > refs/heads/side sorts earlier than refs/tags/side-2). Because the > updated logic is taught to favor refs in refs/tags/ hierarchy, the > the test is updated to expect to see tags/side-2 instead. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- A quick bisect indicates that this patch might break t9807-git-p4-submit.sh 8 and 13. I haven't looked into it further, yet. https://travis-ci.org/git/git/jobs/211948406#L2152 https://travis-ci.org/git/git/jobs/211948406#L2460 Non-JS: https://s3.amazonaws.com/archive.travis-ci.org/jobs/211948406/log.txt - Lars ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak 2017-03-17 4:07 ` Lars Schneider @ 2017-03-17 5:45 ` Junio C Hamano 2017-03-17 5:56 ` Junio C Hamano 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-17 5:45 UTC (permalink / raw) To: Lars Schneider; +Cc: git, Michael J Gruber Lars Schneider <larsxschneider@gmail.com> writes: > A quick bisect indicates that this patch might break > t9807-git-p4-submit.sh 8 and 13. I haven't looked into > it further, yet. As I do not do P4, help in diagnosing why it breaks is appreciated. If the test script expects a commit, that can be described in two more more ways, is named one way (e.g. named after a branch) and the new "favor tags even if they are unannotated" behaviour breaks its expectation, and the only thing the test _should_ care about is what commit the result is, then clearly the test script needs to be fixed. On the other hand, if git-p4 command internally uses name-rev and it is not prepared to properly handle commits that can be named in more than one way, the problem would be deeper, as it would mean it can misbehave even without the change to name-rev when multiple branches point at the same commit. Thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak 2017-03-17 5:45 ` Junio C Hamano @ 2017-03-17 5:56 ` Junio C Hamano 2017-03-17 17:09 ` Lars Schneider 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-17 5:56 UTC (permalink / raw) To: Lars Schneider; +Cc: git, Michael J Gruber Junio C Hamano <gitster@pobox.com> writes: > Lars Schneider <larsxschneider@gmail.com> writes: > >> A quick bisect indicates that this patch might break >> t9807-git-p4-submit.sh 8 and 13. I haven't looked into >> it further, yet. > > As I do not do P4, help in diagnosing why it breaks is appreciated. > If the test script expects... > On the other hand, if git-p4 command internally uses name-rev and it > is not prepared to properly handle commits that can be named in more > than one way, the problem would be deeper, as it would mean it can > misbehave even without the change to name-rev when multiple branches > point at the same commit. Yikes. Perhaps something along this line? This function seems to want to learn which branch we are on, and running "name-rev HEAD" is *NEVER* the right way to do so. If you are on branch B which happens to point at the same commit as branch A, "name-rev HEAD" can say either A or B (and it is likely it would say A simply because it sorts earlier, and the logic seems to favor the one that was discovered earlier when all else being equal). git-p4.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-p4.py b/git-p4.py index eab319d76e..351d1ab58e 100755 --- a/git-p4.py +++ b/git-p4.py @@ -582,7 +582,7 @@ def currentGitBranch(): # on a detached head return None else: - return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip() + return read_pipe(["git", "symbolic-ref", "HEAD"]).strip()[11:] def isValidGitDir(path): return git_dir(path) != None ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak 2017-03-17 5:56 ` Junio C Hamano @ 2017-03-17 17:09 ` Lars Schneider 2017-03-17 17:17 ` Junio C Hamano 0 siblings, 1 reply; 46+ messages in thread From: Lars Schneider @ 2017-03-17 17:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Michael J Gruber, Luke Diamand > On 17 Mar 2017, at 13:56, Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > >> Lars Schneider <larsxschneider@gmail.com> writes: >> >>> A quick bisect indicates that this patch might break >>> t9807-git-p4-submit.sh 8 and 13. I haven't looked into >>> it further, yet. >> >> As I do not do P4, help in diagnosing why it breaks is appreciated. >> If the test script expects... >> On the other hand, if git-p4 command internally uses name-rev and it >> is not prepared to properly handle commits that can be named in more >> than one way, the problem would be deeper, as it would mean it can >> misbehave even without the change to name-rev when multiple branches >> point at the same commit. > > Yikes. Perhaps something along this line? > > This function seems to want to learn which branch we are on, and > running "name-rev HEAD" is *NEVER* the right way to do so. If you > are on branch B which happens to point at the same commit as branch > A, "name-rev HEAD" can say either A or B (and it is likely it would > say A simply because it sorts earlier, and the logic seems to favor > the one that was discovered earlier when all else being equal). > > git-p4.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/git-p4.py b/git-p4.py > index eab319d76e..351d1ab58e 100755 > --- a/git-p4.py > +++ b/git-p4.py > @@ -582,7 +582,7 @@ def currentGitBranch(): > # on a detached head > return None > else: > - return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip() > + return read_pipe(["git", "symbolic-ref", "HEAD"]).strip()[11:] > > def isValidGitDir(path): > return git_dir(path) != None Following your explanation this patch looks good to me and this fixes the test failure. TBH I never thought about the difference of these commands before. "rev" and "ref" sound so similar although they denote completely different things. Thanks, Lars ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak 2017-03-17 17:09 ` Lars Schneider @ 2017-03-17 17:17 ` Junio C Hamano 2017-03-17 22:43 ` Junio C Hamano 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-17 17:17 UTC (permalink / raw) To: Lars Schneider; +Cc: Git Mailing List, Michael J Gruber, Luke Diamand Lars Schneider <larsxschneider@gmail.com> writes: >> git-p4.py | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/git-p4.py b/git-p4.py >> index eab319d76e..351d1ab58e 100755 >> --- a/git-p4.py >> +++ b/git-p4.py >> @@ -582,7 +582,7 @@ def currentGitBranch(): >> # on a detached head >> return None >> else: >> - return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip() >> + return read_pipe(["git", "symbolic-ref", "HEAD"]).strip()[11:] >> >> def isValidGitDir(path): >> return git_dir(path) != None > > Following your explanation this patch looks good to me and this fixes the > test failure. TBH I never thought about the difference of these commands > before. "rev" and "ref" sound so similar although they denote completely > different things. Thanks for testing. The above was done merely to point out the problematic place and a possible solution. As I am not familiar with the code in git-p4.py, I didn't even try to check if the code already has a helper function that strips "refs/heads/" from the beginning of the string (iow, I am not happy with the [11:]). I didn't and don't like the fact that this function now runs "symbolic-ref HEAD" twice but I didn't try to see if there are more suitable and idiomatic ways to do this with a single invocation. Hence, I would really prefer not to commit mine myself. I'd rather see somebody from git-p4 circle to come up with a version that is more in line with the way things are done in the existing code and send a tested version for me to apply. Thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak 2017-03-17 17:17 ` Junio C Hamano @ 2017-03-17 22:43 ` Junio C Hamano 2017-03-29 14:39 ` [PATCH v2 0/3] name-rev sanity Michael J Gruber 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-17 22:43 UTC (permalink / raw) To: Lars Schneider; +Cc: Git Mailing List, Michael J Gruber, Luke Diamand Junio C Hamano <gitster@pobox.com> writes: > Lars Schneider <larsxschneider@gmail.com> writes: > ... >> Following your explanation this patch looks good to me and this fixes the >> test failure. TBH I never thought about the difference of these commands >> before. "rev" and "ref" sound so similar although they denote completely >> different things. > > Thanks for testing. > ... > Hence, I would really prefer not to commit mine myself. I'd rather > see somebody from git-p4 circle to come up with a version that is > more in line with the way things are done in the existing code and > send a tested version for me to apply. Tentatively I queued my hack together with the name-rev thing on 'pu', and Travis seems to be OK with the result. It would be nice if we can "fix" the use of "name-rev HEAD" in "git p4" sooner rather than later. If the code truly uses it to figure out what branch we are currently on and acts on it, as the name of that function suggests, it may be easy to construct a testcase where the code without the fix does a wrong thing (e.g. have two branches pointing at the same commit, and tell "git p4" to act on the one that sorts later in the "git branch --list" output; the command would act as if it were working on the other branch), and shows that the patch fixes that problem. That would be a bug worth fixing quickly regardless of the "name-rev" change michael wanted to make. Thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v2 0/3] name-rev sanity 2017-03-17 22:43 ` Junio C Hamano @ 2017-03-29 14:39 ` Michael J Gruber 2017-03-29 14:39 ` [PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber ` (3 more replies) 0 siblings, 4 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-29 14:39 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand So here is v2 of the name-rev series, the result of our discussions being: Junio C Hamano (2): name-rev: refactor logic to see if a new candidate is a better name name-rev: favor describing with tags and use committer date to tiebreak That second patch is slighty changed as discussed, but still mostly Junio's Michael J Gruber (1): name-rev: provide debug output This replaces the patch which documented that --debug does not work with --contains :) builtin/describe.c | 2 + builtin/name-rev.c | 117 +++++++++++++++++++++++++++++++++++++++++++++-------- t/t4202-log.sh | 2 +- 3 files changed, 103 insertions(+), 18 deletions(-) -- 2.12.2.712.gba4c48c431 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name 2017-03-29 14:39 ` [PATCH v2 0/3] name-rev sanity Michael J Gruber @ 2017-03-29 14:39 ` Michael J Gruber 2017-03-29 14:39 ` [PATCH v2 2/3] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber ` (2 subsequent siblings) 3 siblings, 0 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-29 14:39 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand From: Junio C Hamano <gitster@pobox.com> When we encounter a new ref that could describe the commit we are looking at, we compare the name that is formed using that ref and the name we found so far and pick a better one. Factor the comparison logic out to a separate helper function, while keeping the current logic the same (i.e. a name that is based on an older tag is better, and if two tags of the same age can reach the commit, the one with fewer number of hops to reach the commit is better). Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Michael J Gruber <git@grubix.eu> --- builtin/name-rev.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 8bdc3eaa6f..7890f826ce 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -20,6 +20,17 @@ static long cutoff = LONG_MAX; /* How many generations are maximally preferred over _one_ merge traversal? */ #define MERGE_TRAVERSAL_WEIGHT 65535 +static int is_better_name(struct rev_name *name, + const char *tip_name, + unsigned long taggerdate, + int generation, + int distance) +{ + return (name->taggerdate > taggerdate || + (name->taggerdate == taggerdate && + name->distance > distance)); +} + static void name_rev(struct commit *commit, const char *tip_name, unsigned long taggerdate, int generation, int distance, @@ -45,9 +56,8 @@ static void name_rev(struct commit *commit, name = xmalloc(sizeof(rev_name)); commit->util = name; goto copy_data; - } else if (name->taggerdate > taggerdate || - (name->taggerdate == taggerdate && - name->distance > distance)) { + } else if (is_better_name(name, tip_name, taggerdate, + generation, distance)) { copy_data: name->tip_name = tip_name; name->taggerdate = taggerdate; -- 2.12.2.712.gba4c48c431 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH v2 2/3] name-rev: favor describing with tags and use committer date to tiebreak 2017-03-29 14:39 ` [PATCH v2 0/3] name-rev sanity Michael J Gruber 2017-03-29 14:39 ` [PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber @ 2017-03-29 14:39 ` Michael J Gruber 2017-03-29 14:39 ` [PATCH v2 3/3] name-rev: provide debug output Michael J Gruber 2017-03-29 17:15 ` [PATCH v2 0/3] name-rev sanity Junio C Hamano 3 siblings, 0 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-29 14:39 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand From: Junio C Hamano <gitster@pobox.com> "git name-rev" assigned a phony "far in the future" date to tips of refs that are not pointing at tag objects, and favored names based on a ref with the oldest date. This made it almost impossible for an unannotated tags and branches to be counted as a viable base, which was especially problematic when the command is run with the "--tags" option. If an unannotated tag that points at an ancient commit and an annotated tag that points at a much newer commit reaches the commit that is being named, the old unannotated tag was ignored. Update the "taggerdate" field of the rev-name structure, which is initialized from the tip of ref, to have the committer date if the object at the tip of ref is a commit, not a tag, so that we can optionally take it into account when doing "is this name better?" comparison logic. When "name-rev" is run without the "--tags" option, the general expectation is still to name the commit based on a tag if possible, but use non-tag refs as fallback, and tiebreak among these non-tag refs by favoring names with shorter hops from the tip. The use of a phony "far in the future" date in the original code was an effective way to ensure this expectation is held: a non-tag tip gets the same "far in the future" timestamp, giving precedence to tags, and among non-tag tips, names with shorter hops are preferred over longer hops, without taking the "taggerdate" into account. As we are taking over the "taggerdate" field to store the committer date for tips with commits: (1) keep the original logic when comparing names based on two refs both of which are from refs/tags/; (2) favoring a name based on a ref in refs/tags/ hierarchy over a ref outside the hierarchy; (3) between two names based on a ref both outside refs/tags/, give precedence to a name with shorter hops and use "taggerdate" only to tie-break. A change to t4202 is a natural consequence. The test creates a commit on a branch "side" and points at it with an unannotated tag "refs/tags/side-2". The original code couldn't decide which one to favor at all, and gave a name based on a branch (simply because refs/heads/side sorts earlier than refs/tags/side-2). Because the updated logic is taught to favor refs in refs/tags/ hierarchy, the the test is updated to expect to see tags/side-2 instead. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * I am sure others can add better heurisitics in is_better_name() for comparing names based on non-tag refs, and I do not mind people disagreeing with the logic that this patch happens to implement. This is done primarily to illustrate the value of using a separate helper function is_better_name() instead of open-coding the selection logic in name_rev() function. Signed-off-by: Michael J Gruber <git@grubix.eu> --- builtin/name-rev.c | 53 ++++++++++++++++++++++++++++++++++++++++++++--------- t/t4202-log.sh | 2 +- 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 7890f826ce..bf7ed015ae 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -13,6 +13,7 @@ typedef struct rev_name { unsigned long taggerdate; int generation; int distance; + int from_tag; } rev_name; static long cutoff = LONG_MAX; @@ -24,16 +25,43 @@ static int is_better_name(struct rev_name *name, const char *tip_name, unsigned long taggerdate, int generation, - int distance) + int distance, + int from_tag) { - return (name->taggerdate > taggerdate || - (name->taggerdate == taggerdate && - name->distance > distance)); + /* + * When comparing names based on tags, prefer names + * based on the older tag, even if it is farther away. + */ + if (from_tag && name->from_tag) + return (name->taggerdate > taggerdate || + (name->taggerdate == taggerdate && + name->distance > distance)); + + /* + * We know that at least one of them is a non-tag at this point. + * favor a tag over a non-tag. + */ + if (name->from_tag != from_tag) + return from_tag; + + /* + * We are now looking at two non-tags. Tiebreak to favor + * shorter hops. + */ + if (name->distance != distance) + return name->distance > distance; + + /* ... or tiebreak to favor older date */ + if (name->taggerdate != taggerdate) + return name->taggerdate > taggerdate; + + /* keep the current one if we cannot decide */ + return 0; } static void name_rev(struct commit *commit, const char *tip_name, unsigned long taggerdate, - int generation, int distance, + int generation, int distance, int from_tag, int deref) { struct rev_name *name = (struct rev_name *)commit->util; @@ -57,12 +85,13 @@ static void name_rev(struct commit *commit, commit->util = name; goto copy_data; } else if (is_better_name(name, tip_name, taggerdate, - generation, distance)) { + generation, distance, from_tag)) { copy_data: name->tip_name = tip_name; name->taggerdate = taggerdate; name->generation = generation; name->distance = distance; + name->from_tag = from_tag; } else return; @@ -82,10 +111,12 @@ static void name_rev(struct commit *commit, parent_number); name_rev(parents->item, new_name, taggerdate, 0, - distance + MERGE_TRAVERSAL_WEIGHT, 0); + distance + MERGE_TRAVERSAL_WEIGHT, + from_tag, 0); } else { name_rev(parents->item, tip_name, taggerdate, - generation + 1, distance + 1, 0); + generation + 1, distance + 1, + from_tag, 0); } } } @@ -216,9 +247,13 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo } if (o && o->type == OBJ_COMMIT) { struct commit *commit = (struct commit *)o; + int from_tag = starts_with(path, "refs/tags/"); + if (taggerdate == ULONG_MAX) + taggerdate = ((struct commit *)o)->date; path = name_ref_abbrev(path, can_abbreviate_output); - name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref); + name_rev(commit, xstrdup(path), taggerdate, 0, 0, + from_tag, deref); } return 0; } diff --git a/t/t4202-log.sh b/t/t4202-log.sh index f577990716..4afd8135cd 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -399,7 +399,7 @@ cat > expect <<\EOF | | | | Merge branch 'side' | | -| * commit side +| * commit tags/side-2 | | Author: A U Thor <author@example.com> | | | | side-2 -- 2.12.2.712.gba4c48c431 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH v2 3/3] name-rev: provide debug output 2017-03-29 14:39 ` [PATCH v2 0/3] name-rev sanity Michael J Gruber 2017-03-29 14:39 ` [PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber 2017-03-29 14:39 ` [PATCH v2 2/3] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber @ 2017-03-29 14:39 ` Michael J Gruber 2017-03-29 17:15 ` [PATCH v2 0/3] name-rev sanity Junio C Hamano 3 siblings, 0 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-29 14:39 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand Currently, `git describe --contains --debug` does not create any debug output because it does not pass the flag down to `git name-rev`, which does not know that flag. Teach the latter that flag and the former to pass it down. The output is patterned after that of `git describe --debug`, with the following differences: describe loops over all args to describe, then over all possible descriptions; name-rev does it the other way round. Therefore, we need to amend each possible description by the arg that it is for (and we leave out the "searching to describe" header). The date cut-off for name-rev kicks in way more often than the candidate number cut-off of describe, so we do not clutter the output with the cut-off. Signed-off-by: Michael J Gruber <git@grubix.eu> --- builtin/describe.c | 2 ++ builtin/name-rev.c | 64 +++++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 53 insertions(+), 13 deletions(-) diff --git a/builtin/describe.c b/builtin/describe.c index a5cd8c513f..30196793f0 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -486,6 +486,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix) NULL); if (always) argv_array_push(&args, "--always"); + if (debug) + argv_array_push(&args, "--debug"); if (!all) { argv_array_push(&args, "--tags"); for_each_string_list_item(item, &patterns) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index bf7ed015ae..51302a79ba 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -18,6 +18,10 @@ typedef struct rev_name { static long cutoff = LONG_MAX; +static const char *prio_names[] = { + N_("head"), N_("lightweight"), N_("annotated"), +}; + /* How many generations are maximally preferred over _one_ merge traversal? */ #define MERGE_TRAVERSAL_WEIGHT 65535 @@ -59,10 +63,19 @@ static int is_better_name(struct rev_name *name, return 0; } +struct name_ref_data { + int tags_only; + int name_only; + int debug; + struct string_list ref_filters; + struct string_list exclude_filters; + struct object_array *revs; +}; + static void name_rev(struct commit *commit, const char *tip_name, unsigned long taggerdate, int generation, int distance, int from_tag, - int deref) + int deref, struct name_ref_data *data) { struct rev_name *name = (struct rev_name *)commit->util; struct commit_list *parents; @@ -75,6 +88,7 @@ static void name_rev(struct commit *commit, if (deref) { tip_name = xstrfmt("%s^0", tip_name); + from_tag += 1; if (generation) die("generation: %d, but deref?", generation); @@ -87,6 +101,36 @@ static void name_rev(struct commit *commit, } else if (is_better_name(name, tip_name, taggerdate, generation, distance, from_tag)) { copy_data: + if (data->debug) { + int i; + static int label_width = -1; + static int name_width = -1; + if (label_width < 0) { + int w; + for (i = 0; i < ARRAY_SIZE(prio_names); i++) { + w = strlen(_(prio_names[i])); + if (label_width < w) + label_width = w; + } + } + if (name_width < 0) { + int w; + for (i = 0; i < data->revs->nr; i++) { + w = strlen(data->revs->objects[i].name); + if (name_width < w) + name_width = w; + } + } + for (i = 0; i < data->revs->nr; i++) + if (!oidcmp(&commit->object.oid, + &data->revs->objects[i].item->oid)) + fprintf(stderr, " %-*s %8d %-*s %s~%d\n", + label_width, + _(prio_names[from_tag]), + distance, name_width, + data->revs->objects[i].name, + tip_name, generation); + } name->tip_name = tip_name; name->taggerdate = taggerdate; name->generation = generation; @@ -112,11 +156,11 @@ static void name_rev(struct commit *commit, name_rev(parents->item, new_name, taggerdate, 0, distance + MERGE_TRAVERSAL_WEIGHT, - from_tag, 0); + from_tag, 0, data); } else { name_rev(parents->item, tip_name, taggerdate, generation + 1, distance + 1, - from_tag, 0); + from_tag, 0, data); } } } @@ -146,13 +190,6 @@ static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous) return refname; } -struct name_ref_data { - int tags_only; - int name_only; - struct string_list ref_filters; - struct string_list exclude_filters; -}; - static struct tip_table { struct tip_table_entry { unsigned char sha1[20]; @@ -236,7 +273,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo } add_to_tip_table(oid->hash, path, can_abbreviate_output); - while (o && o->type == OBJ_TAG) { struct tag *t = (struct tag *) o; if (!t->tagged) @@ -253,7 +289,7 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo taggerdate = ((struct commit *)o)->date; path = name_ref_abbrev(path, can_abbreviate_output); name_rev(commit, xstrdup(path), taggerdate, 0, 0, - from_tag, deref); + from_tag, deref, data); } return 0; } @@ -383,7 +419,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) { struct object_array revs = OBJECT_ARRAY_INIT; int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0; - struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP }; + struct name_ref_data data = { 0, 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP }; struct option opts[] = { OPT_BOOL(0, "name-only", &data.name_only, N_("print only names (no SHA-1)")), OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")), @@ -393,6 +429,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) N_("ignore refs matching <pattern>")), OPT_GROUP(""), OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")), + OPT_BOOL(0, "debug", &data.debug, N_("debug search strategy on stderr")), OPT_BOOL(0, "stdin", &transform_stdin, N_("read from stdin")), OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")), OPT_BOOL(0, "always", &always, @@ -458,6 +495,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) if (cutoff) cutoff = cutoff - CUTOFF_DATE_SLOP; + data.revs = &revs; for_each_ref(name_ref, &data); if (transform_stdin) { -- 2.12.2.712.gba4c48c431 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v2 0/3] name-rev sanity 2017-03-29 14:39 ` [PATCH v2 0/3] name-rev sanity Michael J Gruber ` (2 preceding siblings ...) 2017-03-29 14:39 ` [PATCH v2 3/3] name-rev: provide debug output Michael J Gruber @ 2017-03-29 17:15 ` Junio C Hamano 2017-03-29 17:43 ` Junio C Hamano 3 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-29 17:15 UTC (permalink / raw) To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand Michael J Gruber <git@grubix.eu> writes: > So here is v2 of the name-rev series, the result of our discussions being: > > Junio C Hamano (2): > name-rev: refactor logic to see if a new candidate is a better name > name-rev: favor describing with tags and use committer date to > tiebreak > > That second patch is slighty changed as discussed, but still mostly Junio's > > Michael J Gruber (1): > name-rev: provide debug output > > This replaces the patch which documented that --debug does not work with --contains :) > > builtin/describe.c | 2 + > builtin/name-rev.c | 117 +++++++++++++++++++++++++++++++++++++++++++++-------- > t/t4202-log.sh | 2 +- > 3 files changed, 103 insertions(+), 18 deletions(-) The first two applies cleanly to the same base as jc/name-rev that the first two of these patches are meant to replace, but the third one doesn't apply on top. Are you depending on something newer? ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2 0/3] name-rev sanity 2017-03-29 17:15 ` [PATCH v2 0/3] name-rev sanity Junio C Hamano @ 2017-03-29 17:43 ` Junio C Hamano 2017-03-30 13:48 ` Michael J Gruber 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-29 17:43 UTC (permalink / raw) To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand Junio C Hamano <gitster@pobox.com> writes: > The first two applies cleanly to the same base as jc/name-rev that > the first two of these patches are meant to replace, but the third > one doesn't apply on top. Are you depending on something newer? Ah, of course, you are depending on your other topic ;-) I'll wiggle these in. Thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2 0/3] name-rev sanity 2017-03-29 17:43 ` Junio C Hamano @ 2017-03-30 13:48 ` Michael J Gruber 2017-03-30 17:30 ` Junio C Hamano 0 siblings, 1 reply; 46+ messages in thread From: Michael J Gruber @ 2017-03-30 13:48 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Lars Schneider, Luke Diamand Junio C Hamano venit, vidit, dixit 29.03.2017 19:43: > Junio C Hamano <gitster@pobox.com> writes: > >> The first two applies cleanly to the same base as jc/name-rev that >> the first two of these patches are meant to replace, but the third >> one doesn't apply on top. Are you depending on something newer? > > Ah, of course, you are depending on your other topic ;-) > I'll wiggle these in. > > Thanks. > Yes, sorry, isn't that in next already? I should have meantioned it anyways. Also, I should split patch 3 into name-rev and describe related parts and update the name-rev documentation. We don't have tests for describe --debug, that should be ok. Mihael ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v2 0/3] name-rev sanity 2017-03-30 13:48 ` Michael J Gruber @ 2017-03-30 17:30 ` Junio C Hamano 2017-03-31 13:51 ` [PATCH v3 0/4] " Michael J Gruber 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-30 17:30 UTC (permalink / raw) To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand Michael J Gruber <git@grubix.eu> writes: > Junio C Hamano venit, vidit, dixit 29.03.2017 19:43: > ... >> Ah, of course, you are depending on your other topic ;-) >> I'll wiggle these in. >> >> Thanks. > > Yes, sorry, isn't that in next already? I should have meantioned it anyways. No worries. jc/name-rev was meant to be replaced. > Also, I should split patch 3 into name-rev and describe related parts > and update the name-rev documentation. We don't have tests for describe > --debug, that should be ok. OK. I do not think these three patches are in 'next' yet, so feel free to further replace the last one (or for that matter the first two as necessary). I do not think we want to cast the debugging information in stone, but it would not hurt if you made sure that "describe --debug [--contains]" does not dump core ;-) Thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v3 0/4] name-rev sanity 2017-03-30 17:30 ` Junio C Hamano @ 2017-03-31 13:51 ` Michael J Gruber 2017-03-31 13:51 ` [PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber ` (3 more replies) 0 siblings, 4 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-31 13:51 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand v3 splits the old 3/3 into name-rev and describe related parts and adds documentation. No core dumps encountered so far ;) Did I mention this is on top of mg/describe-debug-l10n (next)? Junio C Hamano (2): name-rev: refactor logic to see if a new candidate is a better name name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber (2): name-rev: provide debug output describe: pass --debug down to name-rev Documentation/git-name-rev.txt | 5 ++ builtin/describe.c | 2 + builtin/name-rev.c | 117 +++++++++++++++++++++++++++++++++++------ t/t4202-log.sh | 2 +- 4 files changed, 108 insertions(+), 18 deletions(-) -- 2.12.2.739.gfc3eb97820 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name 2017-03-31 13:51 ` [PATCH v3 0/4] " Michael J Gruber @ 2017-03-31 13:51 ` Michael J Gruber 2017-03-31 13:51 ` [PATCH v3 2/4] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber ` (2 subsequent siblings) 3 siblings, 0 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-31 13:51 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand From: Junio C Hamano <gitster@pobox.com> When we encounter a new ref that could describe the commit we are looking at, we compare the name that is formed using that ref and the name we found so far and pick a better one. Factor the comparison logic out to a separate helper function, while keeping the current logic the same (i.e. a name that is based on an older tag is better, and if two tags of the same age can reach the commit, the one with fewer number of hops to reach the commit is better). Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Michael J Gruber <git@grubix.eu> --- builtin/name-rev.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 8bdc3eaa6f..7890f826ce 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -20,6 +20,17 @@ static long cutoff = LONG_MAX; /* How many generations are maximally preferred over _one_ merge traversal? */ #define MERGE_TRAVERSAL_WEIGHT 65535 +static int is_better_name(struct rev_name *name, + const char *tip_name, + unsigned long taggerdate, + int generation, + int distance) +{ + return (name->taggerdate > taggerdate || + (name->taggerdate == taggerdate && + name->distance > distance)); +} + static void name_rev(struct commit *commit, const char *tip_name, unsigned long taggerdate, int generation, int distance, @@ -45,9 +56,8 @@ static void name_rev(struct commit *commit, name = xmalloc(sizeof(rev_name)); commit->util = name; goto copy_data; - } else if (name->taggerdate > taggerdate || - (name->taggerdate == taggerdate && - name->distance > distance)) { + } else if (is_better_name(name, tip_name, taggerdate, + generation, distance)) { copy_data: name->tip_name = tip_name; name->taggerdate = taggerdate; -- 2.12.2.739.gfc3eb97820 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH v3 2/4] name-rev: favor describing with tags and use committer date to tiebreak 2017-03-31 13:51 ` [PATCH v3 0/4] " Michael J Gruber 2017-03-31 13:51 ` [PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber @ 2017-03-31 13:51 ` Michael J Gruber 2017-03-31 13:51 ` [PATCH v3 3/4] name-rev: provide debug output Michael J Gruber 2017-03-31 13:51 ` [PATCH v3 4/4] describe: pass --debug down to name-rev Michael J Gruber 3 siblings, 0 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-31 13:51 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand From: Junio C Hamano <gitster@pobox.com> "git name-rev" assigned a phony "far in the future" date to tips of refs that are not pointing at tag objects, and favored names based on a ref with the oldest date. This made it almost impossible for an unannotated tags and branches to be counted as a viable base, which was especially problematic when the command is run with the "--tags" option. If an unannotated tag that points at an ancient commit and an annotated tag that points at a much newer commit reaches the commit that is being named, the old unannotated tag was ignored. Update the "taggerdate" field of the rev-name structure, which is initialized from the tip of ref, to have the committer date if the object at the tip of ref is a commit, not a tag, so that we can optionally take it into account when doing "is this name better?" comparison logic. When "name-rev" is run without the "--tags" option, the general expectation is still to name the commit based on a tag if possible, but use non-tag refs as fallback, and tiebreak among these non-tag refs by favoring names with shorter hops from the tip. The use of a phony "far in the future" date in the original code was an effective way to ensure this expectation is held: a non-tag tip gets the same "far in the future" timestamp, giving precedence to tags, and among non-tag tips, names with shorter hops are preferred over longer hops, without taking the "taggerdate" into account. As we are taking over the "taggerdate" field to store the committer date for tips with commits: (1) keep the original logic when comparing names based on two refs both of which are from refs/tags/; (2) favoring a name based on a ref in refs/tags/ hierarchy over a ref outside the hierarchy; (3) between two names based on a ref both outside refs/tags/, give precedence to a name with shorter hops and use "taggerdate" only to tie-break. A change to t4202 is a natural consequence. The test creates a commit on a branch "side" and points at it with an unannotated tag "refs/tags/side-2". The original code couldn't decide which one to favor at all, and gave a name based on a branch (simply because refs/heads/side sorts earlier than refs/tags/side-2). Because the updated logic is taught to favor refs in refs/tags/ hierarchy, the the test is updated to expect to see tags/side-2 instead. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * I am sure others can add better heurisitics in is_better_name() for comparing names based on non-tag refs, and I do not mind people disagreeing with the logic that this patch happens to implement. This is done primarily to illustrate the value of using a separate helper function is_better_name() instead of open-coding the selection logic in name_rev() function. Signed-off-by: Michael J Gruber <git@grubix.eu> --- builtin/name-rev.c | 53 ++++++++++++++++++++++++++++++++++++++++++++--------- t/t4202-log.sh | 2 +- 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 7890f826ce..bf7ed015ae 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -13,6 +13,7 @@ typedef struct rev_name { unsigned long taggerdate; int generation; int distance; + int from_tag; } rev_name; static long cutoff = LONG_MAX; @@ -24,16 +25,43 @@ static int is_better_name(struct rev_name *name, const char *tip_name, unsigned long taggerdate, int generation, - int distance) + int distance, + int from_tag) { - return (name->taggerdate > taggerdate || - (name->taggerdate == taggerdate && - name->distance > distance)); + /* + * When comparing names based on tags, prefer names + * based on the older tag, even if it is farther away. + */ + if (from_tag && name->from_tag) + return (name->taggerdate > taggerdate || + (name->taggerdate == taggerdate && + name->distance > distance)); + + /* + * We know that at least one of them is a non-tag at this point. + * favor a tag over a non-tag. + */ + if (name->from_tag != from_tag) + return from_tag; + + /* + * We are now looking at two non-tags. Tiebreak to favor + * shorter hops. + */ + if (name->distance != distance) + return name->distance > distance; + + /* ... or tiebreak to favor older date */ + if (name->taggerdate != taggerdate) + return name->taggerdate > taggerdate; + + /* keep the current one if we cannot decide */ + return 0; } static void name_rev(struct commit *commit, const char *tip_name, unsigned long taggerdate, - int generation, int distance, + int generation, int distance, int from_tag, int deref) { struct rev_name *name = (struct rev_name *)commit->util; @@ -57,12 +85,13 @@ static void name_rev(struct commit *commit, commit->util = name; goto copy_data; } else if (is_better_name(name, tip_name, taggerdate, - generation, distance)) { + generation, distance, from_tag)) { copy_data: name->tip_name = tip_name; name->taggerdate = taggerdate; name->generation = generation; name->distance = distance; + name->from_tag = from_tag; } else return; @@ -82,10 +111,12 @@ static void name_rev(struct commit *commit, parent_number); name_rev(parents->item, new_name, taggerdate, 0, - distance + MERGE_TRAVERSAL_WEIGHT, 0); + distance + MERGE_TRAVERSAL_WEIGHT, + from_tag, 0); } else { name_rev(parents->item, tip_name, taggerdate, - generation + 1, distance + 1, 0); + generation + 1, distance + 1, + from_tag, 0); } } } @@ -216,9 +247,13 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo } if (o && o->type == OBJ_COMMIT) { struct commit *commit = (struct commit *)o; + int from_tag = starts_with(path, "refs/tags/"); + if (taggerdate == ULONG_MAX) + taggerdate = ((struct commit *)o)->date; path = name_ref_abbrev(path, can_abbreviate_output); - name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref); + name_rev(commit, xstrdup(path), taggerdate, 0, 0, + from_tag, deref); } return 0; } diff --git a/t/t4202-log.sh b/t/t4202-log.sh index f577990716..4afd8135cd 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -399,7 +399,7 @@ cat > expect <<\EOF | | | | Merge branch 'side' | | -| * commit side +| * commit tags/side-2 | | Author: A U Thor <author@example.com> | | | | side-2 -- 2.12.2.739.gfc3eb97820 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH v3 3/4] name-rev: provide debug output 2017-03-31 13:51 ` [PATCH v3 0/4] " Michael J Gruber 2017-03-31 13:51 ` [PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber 2017-03-31 13:51 ` [PATCH v3 2/4] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber @ 2017-03-31 13:51 ` Michael J Gruber 2017-03-31 16:52 ` Junio C Hamano 2017-03-31 13:51 ` [PATCH v3 4/4] describe: pass --debug down to name-rev Michael J Gruber 3 siblings, 1 reply; 46+ messages in thread From: Michael J Gruber @ 2017-03-31 13:51 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand Currently, `git describe --contains --debug` does not create any debug output because it does not pass the flag down to `git name-rev`, which does not know that flag. Teach the latter that flag, so that the former can pass it down (in the following commit). The output is patterned after that of `git describe --debug`, with the following differences: describe loops over all args to describe, then over all possible descriptions; name-rev does it the other way round. Therefore, we need to amend each possible description by the arg that it is for (and we leave out the "searching to describe" header). The date cut-off for name-rev kicks in way more often than the candidate number cut-off of describe, so we do not clutter the output with the cut-off. Signed-off-by: Michael J Gruber <git@grubix.eu> --- Documentation/git-name-rev.txt | 5 ++++ builtin/name-rev.c | 64 +++++++++++++++++++++++++++++++++--------- 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt index e8e68f528c..fd78ee86e8 100644 --- a/Documentation/git-name-rev.txt +++ b/Documentation/git-name-rev.txt @@ -42,6 +42,11 @@ OPTIONS --all:: List all commits reachable from all refs +--debug:: + Verbosely display information about the searching strategy + being employed to standard error. The symbolic name will still + be printed to standard out. + --stdin:: Transform stdin by substituting all the 40-character SHA-1 hexes (say $hex) with "$hex ($rev_name)". When used with diff --git a/builtin/name-rev.c b/builtin/name-rev.c index bf7ed015ae..51302a79ba 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -18,6 +18,10 @@ typedef struct rev_name { static long cutoff = LONG_MAX; +static const char *prio_names[] = { + N_("head"), N_("lightweight"), N_("annotated"), +}; + /* How many generations are maximally preferred over _one_ merge traversal? */ #define MERGE_TRAVERSAL_WEIGHT 65535 @@ -59,10 +63,19 @@ static int is_better_name(struct rev_name *name, return 0; } +struct name_ref_data { + int tags_only; + int name_only; + int debug; + struct string_list ref_filters; + struct string_list exclude_filters; + struct object_array *revs; +}; + static void name_rev(struct commit *commit, const char *tip_name, unsigned long taggerdate, int generation, int distance, int from_tag, - int deref) + int deref, struct name_ref_data *data) { struct rev_name *name = (struct rev_name *)commit->util; struct commit_list *parents; @@ -75,6 +88,7 @@ static void name_rev(struct commit *commit, if (deref) { tip_name = xstrfmt("%s^0", tip_name); + from_tag += 1; if (generation) die("generation: %d, but deref?", generation); @@ -87,6 +101,36 @@ static void name_rev(struct commit *commit, } else if (is_better_name(name, tip_name, taggerdate, generation, distance, from_tag)) { copy_data: + if (data->debug) { + int i; + static int label_width = -1; + static int name_width = -1; + if (label_width < 0) { + int w; + for (i = 0; i < ARRAY_SIZE(prio_names); i++) { + w = strlen(_(prio_names[i])); + if (label_width < w) + label_width = w; + } + } + if (name_width < 0) { + int w; + for (i = 0; i < data->revs->nr; i++) { + w = strlen(data->revs->objects[i].name); + if (name_width < w) + name_width = w; + } + } + for (i = 0; i < data->revs->nr; i++) + if (!oidcmp(&commit->object.oid, + &data->revs->objects[i].item->oid)) + fprintf(stderr, " %-*s %8d %-*s %s~%d\n", + label_width, + _(prio_names[from_tag]), + distance, name_width, + data->revs->objects[i].name, + tip_name, generation); + } name->tip_name = tip_name; name->taggerdate = taggerdate; name->generation = generation; @@ -112,11 +156,11 @@ static void name_rev(struct commit *commit, name_rev(parents->item, new_name, taggerdate, 0, distance + MERGE_TRAVERSAL_WEIGHT, - from_tag, 0); + from_tag, 0, data); } else { name_rev(parents->item, tip_name, taggerdate, generation + 1, distance + 1, - from_tag, 0); + from_tag, 0, data); } } } @@ -146,13 +190,6 @@ static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous) return refname; } -struct name_ref_data { - int tags_only; - int name_only; - struct string_list ref_filters; - struct string_list exclude_filters; -}; - static struct tip_table { struct tip_table_entry { unsigned char sha1[20]; @@ -236,7 +273,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo } add_to_tip_table(oid->hash, path, can_abbreviate_output); - while (o && o->type == OBJ_TAG) { struct tag *t = (struct tag *) o; if (!t->tagged) @@ -253,7 +289,7 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo taggerdate = ((struct commit *)o)->date; path = name_ref_abbrev(path, can_abbreviate_output); name_rev(commit, xstrdup(path), taggerdate, 0, 0, - from_tag, deref); + from_tag, deref, data); } return 0; } @@ -383,7 +419,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) { struct object_array revs = OBJECT_ARRAY_INIT; int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0; - struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP }; + struct name_ref_data data = { 0, 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP }; struct option opts[] = { OPT_BOOL(0, "name-only", &data.name_only, N_("print only names (no SHA-1)")), OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")), @@ -393,6 +429,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) N_("ignore refs matching <pattern>")), OPT_GROUP(""), OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")), + OPT_BOOL(0, "debug", &data.debug, N_("debug search strategy on stderr")), OPT_BOOL(0, "stdin", &transform_stdin, N_("read from stdin")), OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")), OPT_BOOL(0, "always", &always, @@ -458,6 +495,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) if (cutoff) cutoff = cutoff - CUTOFF_DATE_SLOP; + data.revs = &revs; for_each_ref(name_ref, &data); if (transform_stdin) { -- 2.12.2.739.gfc3eb97820 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v3 3/4] name-rev: provide debug output 2017-03-31 13:51 ` [PATCH v3 3/4] name-rev: provide debug output Michael J Gruber @ 2017-03-31 16:52 ` Junio C Hamano 2017-03-31 16:55 ` Junio C Hamano 2017-03-31 18:02 ` Michael J Gruber 0 siblings, 2 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-31 16:52 UTC (permalink / raw) To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand Michael J Gruber <git@grubix.eu> writes: > Currently, `git describe --contains --debug` does not create any debug > output because it does not pass the flag down to `git name-rev`, which > does not know that flag. > > Teach the latter that flag, so that the former can pass it down (in > the following commit). > > The output is patterned after that of `git describe --debug`, with the > following differences: > > describe loops over all args to describe, then over all possible > descriptions; name-rev does it the other way round. Therefore, we need > to amend each possible description by the arg that it is for (and we > leave out the "searching to describe" header). > > The date cut-off for name-rev kicks in way more often than the candidate > number cut-off of describe, so we do not clutter the output with the > cut-off. > > Signed-off-by: Michael J Gruber <git@grubix.eu> > --- > static void name_rev(struct commit *commit, > const char *tip_name, unsigned long taggerdate, > int generation, int distance, int from_tag, > - int deref) > + int deref, struct name_ref_data *data) > { > struct rev_name *name = (struct rev_name *)commit->util; > struct commit_list *parents; > @@ -75,6 +88,7 @@ static void name_rev(struct commit *commit, > > if (deref) { > tip_name = xstrfmt("%s^0", tip_name); > + from_tag += 1; Why this change? I didn't see it explained in the proposed log message. "deref" is true only when our immediate caller is the one that inspected the object at the tip and found it to be a tag object (i.e. not a lightweight tag or a branch). from_tag is about "is the tip within refs/tags/ hierarchy? Yes/No?" and such a caller will set it appropriately when calling us. This function just passes it down when it recursively calls itself. We shouldn't be mucking with that value ourselves here, should we? The only case that this change may make a difference I can think of is when you have a tag object pointed at from outside refs/tags (e.g. refs/heads/foo is a tag object); if you are trying to change the definition of "from_tag" from the current "Is the tip inside refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag object anywhere?", that may be a good change (I didn't think things through, though), but that shouldn't be hidden inside a commit that claims to only add support for debugging. What problem are you solving? > @@ -236,7 +273,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo > } > > add_to_tip_table(oid->hash, path, can_abbreviate_output); > - > while (o && o->type == OBJ_TAG) { > struct tag *t = (struct tag *) o; > if (!t->tagged) This is a patch noise we can do without. Thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3 3/4] name-rev: provide debug output 2017-03-31 16:52 ` Junio C Hamano @ 2017-03-31 16:55 ` Junio C Hamano 2017-03-31 18:02 ` Michael J Gruber 1 sibling, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-31 16:55 UTC (permalink / raw) To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand Junio C Hamano <gitster@pobox.com> writes: > The only case that this change may make a difference I can think of > is when you have a tag object pointed at from outside refs/tags > (e.g. refs/heads/foo is a tag object); if you are trying to change > the definition of "from_tag" from the current "Is the tip inside > refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag > object anywhere?", that may be a good change (I didn't think things > through, though), but that shouldn't be hidden inside a commit that > claims to only add support for debugging. And if that "a tag object outside refs/tags/" is what you are solving, I think a better place to do it is in name_ref(). Instead of saying "from_tag is true iff it starts with refs/tags/", you'd say "... or deref is set to true, because we know that the original was a tag object in that case". > What problem are you solving? > >> @@ -236,7 +273,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo >> } >> >> add_to_tip_table(oid->hash, path, can_abbreviate_output); >> - >> while (o && o->type == OBJ_TAG) { >> struct tag *t = (struct tag *) o; >> if (!t->tagged) > > This is a patch noise we can do without. > > Thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3 3/4] name-rev: provide debug output 2017-03-31 16:52 ` Junio C Hamano 2017-03-31 16:55 ` Junio C Hamano @ 2017-03-31 18:02 ` Michael J Gruber 2017-03-31 18:06 ` Junio C Hamano 2017-03-31 19:10 ` Junio C Hamano 1 sibling, 2 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-31 18:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Lars Schneider, Luke Diamand Am 31. März 2017 18:52:16 MESZ schrieb Junio C Hamano <gitster@pobox.com>: >Michael J Gruber <git@grubix.eu> writes: > >> Currently, `git describe --contains --debug` does not create any >debug >> output because it does not pass the flag down to `git name-rev`, >which >> does not know that flag. >> >> Teach the latter that flag, so that the former can pass it down (in >> the following commit). >> >> The output is patterned after that of `git describe --debug`, with >the >> following differences: >> >> describe loops over all args to describe, then over all possible >> descriptions; name-rev does it the other way round. Therefore, we >need >> to amend each possible description by the arg that it is for (and we >> leave out the "searching to describe" header). >> >> The date cut-off for name-rev kicks in way more often than the >candidate >> number cut-off of describe, so we do not clutter the output with the >> cut-off. >> >> Signed-off-by: Michael J Gruber <git@grubix.eu> >> --- > >> static void name_rev(struct commit *commit, >> const char *tip_name, unsigned long taggerdate, >> int generation, int distance, int from_tag, >> - int deref) >> + int deref, struct name_ref_data *data) >> { >> struct rev_name *name = (struct rev_name *)commit->util; >> struct commit_list *parents; >> @@ -75,6 +88,7 @@ static void name_rev(struct commit *commit, >> >> if (deref) { >> tip_name = xstrfmt("%s^0", tip_name); >> + from_tag += 1; > >Why this change? I didn't see it explained in the proposed log >message. "deref" is true only when our immediate caller is the one >that inspected the object at the tip and found it to be a tag object >(i.e. not a lightweight tag or a branch). from_tag is about "is the >tip within refs/tags/ hierarchy? Yes/No?" and such a caller will >set it appropriately when calling us. This function just passes it >down when it recursively calls itself. > >We shouldn't be mucking with that value ourselves here, should we? > >The only case that this change may make a difference I can think of >is when you have a tag object pointed at from outside refs/tags >(e.g. refs/heads/foo is a tag object); if you are trying to change >the definition of "from_tag" from the current "Is the tip inside >refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag >object anywhere?", that may be a good change (I didn't think things >through, though), but that shouldn't be hidden inside a commit that >claims to only add support for debugging. > >What problem are you solving? Sorry, I forgot about that change and failed to mention it. It makes no difference in the non-debug case which cares about the Boolean only. In the debug case, I want to distinguish between annotated and lightweight tags, just like describe --debug does. By adding 1 via deref and passing this down, I know that an annotated tag gets the value 2, a lightweight tag 1 and everything else 0, just like describe --tags. >> @@ -236,7 +273,6 @@ static int name_ref(const char *path, const >struct object_id *oid, int flags, vo >> } >> >> add_to_tip_table(oid->hash, path, can_abbreviate_output); >> - >> while (o && o->type == OBJ_TAG) { >> struct tag *t = (struct tag *) o; >> if (!t->tagged) > >This is a patch noise we can do without. > >Thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3 3/4] name-rev: provide debug output 2017-03-31 18:02 ` Michael J Gruber @ 2017-03-31 18:06 ` Junio C Hamano 2017-03-31 18:33 ` Junio C Hamano 2017-03-31 19:10 ` Junio C Hamano 1 sibling, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-31 18:06 UTC (permalink / raw) To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand Michael J Gruber <git@grubix.eu> writes: >>The only case that this change may make a difference I can think of >>is when you have a tag object pointed at from outside refs/tags >>(e.g. refs/heads/foo is a tag object); if you are trying to change >>the definition of "from_tag" from the current "Is the tip inside >>refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag >>object anywhere?", that may be a good change (I didn't think things >>through, though), but that shouldn't be hidden inside a commit that >>claims to only add support for debugging. >> >>What problem are you solving? > > Sorry, I forgot about that change and failed to mention it. > > It makes no difference in the non-debug case which cares about the > Boolean only. In the debug case, I want to distinguish between > annotated and lightweight tags, just like describe --debug does. By > adding 1 via deref and passing this down, I know that an annotated tag > gets the value 2, a lightweight tag 1 and everything else 0, just like > describe --tags. So it sounds like you meant to do something else, and the implementation is wrong for that something else (i.e. it wouldn't do the right thing for a tag object outside refs/tags/, with or without the "--debug" option passed). >>> @@ -236,7 +273,6 @@ static int name_ref(const char *path, const >>struct object_id *oid, int flags, vo >>> } >>> >>> add_to_tip_table(oid->hash, path, can_abbreviate_output); >>> - >>> while (o && o->type == OBJ_TAG) { >>> struct tag *t = (struct tag *) o; >>> if (!t->tagged) >> >>This is a patch noise we can do without. >> >>Thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3 3/4] name-rev: provide debug output 2017-03-31 18:06 ` Junio C Hamano @ 2017-03-31 18:33 ` Junio C Hamano 2017-04-03 14:46 ` Michael J Gruber 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2017-03-31 18:33 UTC (permalink / raw) To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand Junio C Hamano <gitster@pobox.com> writes: > Michael J Gruber <git@grubix.eu> writes: > >>>The only case that this change may make a difference I can think of >>>is when you have a tag object pointed at from outside refs/tags >>>(e.g. refs/heads/foo is a tag object); if you are trying to change >>>the definition of "from_tag" from the current "Is the tip inside >>>refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag >>>object anywhere?", that may be a good change (I didn't think things >>>through, though), but that shouldn't be hidden inside a commit that >>>claims to only add support for debugging. >>> >>>What problem are you solving? >> >> Sorry, I forgot about that change and failed to mention it. >> >> It makes no difference in the non-debug case which cares about the >> Boolean only. In the debug case, I want to distinguish between >> annotated and lightweight tags, just like describe --debug does. By >> adding 1 via deref and passing this down, I know that an annotated tag >> gets the value 2, a lightweight tag 1 and everything else 0, just like >> describe --tags. > > So it sounds like you meant to do something else, and the > implementation is wrong for that something else (i.e. it wouldn't do > the right thing for a tag object outside refs/tags/, with or without > the "--debug" option passed). The damage seems worse, but I may be misreading the code. is_better_name() compares name->from_tag and from_tag numerically, because it was designed to take a boolean view of that variable. Now, an artificially bumped 2 gets compared with name->from_tag that may be 1 and gets different priority. That artificially inflated value may be propagated to name->from_tag when the current tip is judged as a better basis for naming the object. If this change is only for debugging, perhaps inside if(data->debug) you added, instead of looking at from_tag, you can look at both from_tag and deref to choose which prio-nmes to show, without butchering the value in from_tag variable to affect the existing code that is exercised with or without --debug? ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3 3/4] name-rev: provide debug output 2017-03-31 18:33 ` Junio C Hamano @ 2017-04-03 14:46 ` Michael J Gruber 2017-04-03 17:07 ` Junio C Hamano 2017-05-20 5:19 ` Junio C Hamano 0 siblings, 2 replies; 46+ messages in thread From: Michael J Gruber @ 2017-04-03 14:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Lars Schneider, Luke Diamand Junio C Hamano venit, vidit, dixit 31.03.2017 20:33: > Junio C Hamano <gitster@pobox.com> writes: > >> Michael J Gruber <git@grubix.eu> writes: >> >>>> The only case that this change may make a difference I can think of >>>> is when you have a tag object pointed at from outside refs/tags >>>> (e.g. refs/heads/foo is a tag object); if you are trying to change >>>> the definition of "from_tag" from the current "Is the tip inside >>>> refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag >>>> object anywhere?", that may be a good change (I didn't think things >>>> through, though), but that shouldn't be hidden inside a commit that >>>> claims to only add support for debugging. >>>> >>>> What problem are you solving? >>> >>> Sorry, I forgot about that change and failed to mention it. >>> >>> It makes no difference in the non-debug case which cares about the >>> Boolean only. In the debug case, I want to distinguish between >>> annotated and lightweight tags, just like describe --debug does. By >>> adding 1 via deref and passing this down, I know that an annotated tag >>> gets the value 2, a lightweight tag 1 and everything else 0, just like >>> describe --tags. >> >> So it sounds like you meant to do something else, and the >> implementation is wrong for that something else (i.e. it wouldn't do >> the right thing for a tag object outside refs/tags/, with or without >> the "--debug" option passed). > > The damage seems worse, but I may be misreading the code. > > is_better_name() compares name->from_tag and from_tag numerically, > because it was designed to take a boolean view of that variable. > Now, an artificially bumped 2 gets compared with name->from_tag that > may be 1 and gets different priority. That artificially inflated > value may be propagated to name->from_tag when the current tip is > judged as a better basis for naming the object. No, I checked not to change the existing behaviour. If you look at the comment above that then you see that one of the sides of the comparison is a non-tag, so we compare 0 to 1 or 2, the boolean outcome being the same. > If this change is only for debugging, perhaps inside if(data->debug) > you added, instead of looking at from_tag, you can look at both > from_tag and deref to choose which prio-nmes to show, without > butchering the value in from_tag variable to affect the existing > code that is exercised with or without --debug? What I did overlook, though, was that name-rev uses the notion "under refs/tags" for "being a tag". In fact, it's puzzling how different describe and name-rev proceed and weigh the alternatives. I didn't mean to change that. In retrospect, displaying the "same" debug information for the two commands doesn't make too much sense as long as they use different information. name-rev does-not distinguish between tag types, so why even display it? I think I should change 3/4 to display exactly those bits that name-rev actually uses for weighing different possible descriptions; they are differents from the "describe-bits". So please withhold 3/4 and 4/4. Michael ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3 3/4] name-rev: provide debug output 2017-04-03 14:46 ` Michael J Gruber @ 2017-04-03 17:07 ` Junio C Hamano 2017-05-20 5:19 ` Junio C Hamano 1 sibling, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-04-03 17:07 UTC (permalink / raw) To: Michael J Gruber; +Cc: git Michael J Gruber <git@grubix.eu> writes: > No, I checked not to change the existing behaviour. > > If you look at the comment above that then you see that one of the sides > of the comparison is a non-tag, so we compare 0 to 1 or 2, the boolean > outcome being the same. That one I understand, but if you compare 1 and 2 (one side being a lightweight, the other being an annotated tag), they no longer compare equal, no? ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3 3/4] name-rev: provide debug output 2017-04-03 14:46 ` Michael J Gruber 2017-04-03 17:07 ` Junio C Hamano @ 2017-05-20 5:19 ` Junio C Hamano 1 sibling, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-05-20 5:19 UTC (permalink / raw) To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand Michael J Gruber <git@grubix.eu> writes: > I think I should change 3/4 to display exactly those bits that name-rev > actually uses for weighing different possible descriptions; they are > differents from the "describe-bits". So please withhold 3/4 and 4/4. OK, I think 1&2/4 from this series can progress before that as it is an end-user visible improvement. While looking at it, I also found a variable that recent "timestamp_t" series didn't notice and update, so perhaps 1&2/4 needs to be rebased on a fix for that anyway ;-) Thanks. Will hold 3&4/4 for now. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3 3/4] name-rev: provide debug output 2017-03-31 18:02 ` Michael J Gruber 2017-03-31 18:06 ` Junio C Hamano @ 2017-03-31 19:10 ` Junio C Hamano 1 sibling, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-31 19:10 UTC (permalink / raw) To: Michael J Gruber; +Cc: git, Lars Schneider, Luke Diamand Michael J Gruber <git@grubix.eu> writes: >>What problem are you solving? > > Sorry, I forgot about that change and failed to mention it. > > It makes no difference in the non-debug case which cares about the > Boolean only. In the debug case, I want to distinguish between > annotated and lightweight tags, just like describe --debug > does. By adding 1 via deref and passing this down, I know that an > annotated tag gets the value 2, a lightweight tag 1 and everything > else 0, just like describe --tags. If you want to only affect debug display, perhaps you should start with a patch like the attached, before adding any debug code as a preparatory step. Then add your debugging thing, _WITHOUT_ the increment in name_rev(), that uses from_tag to choose between lightweight and annotated as a separate step. When we decide that it would make sense to give precedence to annotated ones over lightweight ones in is_better_name(), the comparison can be further tweaked to actually compare values of the from_tag thing in *name and the current candidate. That would have to be a separate step, as it changes the semantics (I suspect it would be a better change but it may not be). How does that sound? -- >8 -- Subject: name-rev: allow to tell annotated and lightweight tags apart We do not use this feature yet, but from_tag that is passed around and kept in the rev_name structure now takes three values, instead of a boolean "did this come from refs/tags/ hierarchy?". A new value '2' is "this is an annotated tag that came from refs/tags/ hierarchy". --- builtin/name-rev.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index bf7ed015ae..fe2d306e7c 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -41,7 +41,7 @@ static int is_better_name(struct rev_name *name, * We know that at least one of them is a non-tag at this point. * favor a tag over a non-tag. */ - if (name->from_tag != from_tag) + if (!!name->from_tag != !!from_tag) return from_tag; /* @@ -247,8 +247,11 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo } if (o && o->type == OBJ_COMMIT) { struct commit *commit = (struct commit *)o; - int from_tag = starts_with(path, "refs/tags/"); - + int from_tag; + if (starts_with(path, "refs/tags/")) + from_tag = 1 + deref; + else + from_tag = 0; if (taggerdate == ULONG_MAX) taggerdate = ((struct commit *)o)->date; path = name_ref_abbrev(path, can_abbreviate_output); ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH v3 4/4] describe: pass --debug down to name-rev 2017-03-31 13:51 ` [PATCH v3 0/4] " Michael J Gruber ` (2 preceding siblings ...) 2017-03-31 13:51 ` [PATCH v3 3/4] name-rev: provide debug output Michael J Gruber @ 2017-03-31 13:51 ` Michael J Gruber 3 siblings, 0 replies; 46+ messages in thread From: Michael J Gruber @ 2017-03-31 13:51 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Lars Schneider, Luke Diamand Now that name-rev knows --debug, pass that flag down to name-rev when we call it for doing describe --contains. Signed-off-by: Michael J Gruber <git@grubix.eu> --- builtin/describe.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/describe.c b/builtin/describe.c index a5cd8c513f..30196793f0 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -486,6 +486,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix) NULL); if (always) argv_array_push(&args, "--always"); + if (debug) + argv_array_push(&args, "--debug"); if (!all) { argv_array_push(&args, "--tags"); for_each_string_list_item(item, &patterns) -- 2.12.2.739.gfc3eb97820 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak 2017-03-15 22:50 ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Junio C Hamano 2017-03-17 4:07 ` Lars Schneider @ 2017-03-17 11:25 ` Michael J Gruber 2017-03-17 16:03 ` Junio C Hamano 1 sibling, 1 reply; 46+ messages in thread From: Michael J Gruber @ 2017-03-17 11:25 UTC (permalink / raw) To: Junio C Hamano, git > hops, without taking the "taggerdate" into account. As we are > taking over the "taggerdate" field to store the committer date for > tips with commits: > > (1) keep the original logic when comparing names based on two refs > both of which are from refs/tags/; > > (2) favoring a name based on a ref in refs/tags/ hierarchy over > a ref outside the hierarchy; > > (3) between two names based on a ref both outside refs/tags/, give > precedence to a name with shorter hops and use "taggerdate" > only to tie-break. > > A change to t4202 is a natural consequence. The test creates a > commit on a branch "side" and points at it with an unannotated tag > "refs/tags/side-2". The original code couldn't decide which one to > favor at all, and gave a name based on a branch (simply because > refs/heads/side sorts earlier than refs/tags/side-2). Because the > updated logic is taught to favor refs in refs/tags/ hierarchy, the > the test is updated to expect to see tags/side-2 instead. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> I think that strategy is fine and works as I expected in all tested cases. Thanks! > --- > > * I am sure others can add better heurisitics in is_better_name() > for comparing names based on non-tag refs, and I do not mind > people disagreeing with the logic that this patch happens to > implement. This is done primarily to illustrate the value of > using a separate helper function is_better_name() instead of > open-coding the selection logic in name_rev() function. > --- > builtin/name-rev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++--------- > t/t4202-log.sh | 2 +- > 2 files changed, 49 insertions(+), 10 deletions(-) > > diff --git a/builtin/name-rev.c b/builtin/name-rev.c > index f64c71d9bc..eac0180c62 100644 > --- a/builtin/name-rev.c > +++ b/builtin/name-rev.c > @@ -13,6 +13,7 @@ typedef struct rev_name { > unsigned long taggerdate; > int generation; > int distance; > + int from_tag; > } rev_name; > > static long cutoff = LONG_MAX; > @@ -24,16 +25,47 @@ static int is_better_name(struct rev_name *name, > const char *tip_name, > unsigned long taggerdate, > int generation, > - int distance) > + int distance, > + int from_tag) > { > - return (name->taggerdate > taggerdate || > - (name->taggerdate == taggerdate && > - name->distance > distance)); > + /* > + * When comparing names based on tags, prefer names > + * based on the older tag, even if it is farther away. > + */ > + if (from_tag && name->from_tag) > + return (name->taggerdate > taggerdate || > + (name->taggerdate == taggerdate && > + name->distance > distance)); > + > +#define COMPARE(attribute, smaller_is_better) \ > + if (name->attribute > attribute) \ > + return smaller_is_better; \ > + if (name->attribute < attribute) \ > + return !smaller_is_better I find that define pretty confusing. On first reading, the "==" case seems to be missing, but that is basically "pass" as one can see in the later code. Also, the comparison ">" and "<" is used to switch between the cases "tag vs. non-tag" and "non-tag vs. tag" which happen to be encoded by the from_tag attribute beeing "1 vs. 0" resp "0 vs. 1" in the following: > + > + /* > + * We know that at least one of them is a non-tag at this point. > + * favor a tag over a non-tag. > + */ > + COMPARE(from_tag, 0); > + But in the next two instances you use it to do "actual" comparisons between distances and dates: > + /* > + * We are now looking at two non-tags. Tiebreak to favor > + * shorter hops. > + */ > + COMPARE(distance, 1); > + > + /* ... or tiebreak to favor older date */ > + COMPARE(taggerdate, 1); > + > + /* keep the current one if we cannot decide */ > + return 0; > +#undef COMPARE > } So, while I do understand that now, I'm wondering whether I will next time ;) How about something like /* favor tag over non-tag */ if (name->from_tag != from_tag) return from_tag; at least for the first instance and possibly /* favor shorter hops for non-tags */ if (name->distance != distance) return name->distance > distance; /* tie-break by date */ if (name->taggerdate != taggerdate) return name->taggerdate > taggerdate; > > static void name_rev(struct commit *commit, > const char *tip_name, unsigned long taggerdate, > - int generation, int distance, > + int generation, int distance, int from_tag, > int deref) > { > struct rev_name *name = (struct rev_name *)commit->util; > @@ -57,12 +89,13 @@ static void name_rev(struct commit *commit, > commit->util = name; > goto copy_data; > } else if (is_better_name(name, tip_name, taggerdate, > - generation, distance)) { > + generation, distance, from_tag)) { > copy_data: > name->tip_name = tip_name; > name->taggerdate = taggerdate; > name->generation = generation; > name->distance = distance; > + name->from_tag = from_tag; > } else > return; > > @@ -82,10 +115,12 @@ static void name_rev(struct commit *commit, > parent_number); > > name_rev(parents->item, new_name, taggerdate, 0, > - distance + MERGE_TRAVERSAL_WEIGHT, 0); > + distance + MERGE_TRAVERSAL_WEIGHT, > + from_tag, 0); > } else { > name_rev(parents->item, tip_name, taggerdate, > - generation + 1, distance + 1, 0); > + generation + 1, distance + 1, > + from_tag, 0); > } > } > } > @@ -184,9 +219,13 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo > } > if (o && o->type == OBJ_COMMIT) { > struct commit *commit = (struct commit *)o; > + int from_tag = starts_with(path, "refs/tags/"); > > + if (taggerdate == ULONG_MAX) > + taggerdate = ((struct commit *)o)->date; > path = name_ref_abbrev(path, can_abbreviate_output); > - name_rev(commit, xstrdup(path), taggerdate, 0, 0, deref); > + name_rev(commit, xstrdup(path), taggerdate, 0, 0, > + from_tag, deref); > } > return 0; > } > diff --git a/t/t4202-log.sh b/t/t4202-log.sh > index 48b55bfd27..038911f230 100755 > --- a/t/t4202-log.sh > +++ b/t/t4202-log.sh > @@ -398,7 +398,7 @@ cat > expect <<\EOF > | | > | | Merge branch 'side' > | | > -| * commit side > +| * commit tags/side-2 > | | Author: A U Thor <author@example.com> > | | > | | side-2 > ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak 2017-03-17 11:25 ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber @ 2017-03-17 16:03 ` Junio C Hamano 0 siblings, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2017-03-17 16:03 UTC (permalink / raw) To: Michael J Gruber; +Cc: git Michael J Gruber <git@drmicha.warpmail.net> writes: >> +#define COMPARE(attribute, smaller_is_better) \ >> + if (name->attribute > attribute) \ >> + return smaller_is_better; \ >> + if (name->attribute < attribute) \ >> + return !smaller_is_better > > I find that define pretty confusing. On first reading, the "==" case > seems to be missing, but that is basically "pass" as one can see in the > later code. > > Also, the comparison ">" and "<" is used to switch between the cases > "tag vs. non-tag" and "non-tag vs. tag" which happen to be encoded by > the from_tag attribute beeing "1 vs. 0" resp "0 vs. 1" in the following: > ... > But in the next two instances you use it to do "actual" comparisons > between distances and dates: That is pretty much deliberate. The macro is designed to compare the attribute and return if one side is strictly better than the other side, and fall through to tie-breaker that follow (i.e. lack of "==" is very deliberate). Also it is not "return if one side is strictly smaller"---the second parameter decides if smaller means better or bigger means better (i.e. "from_tag" being 1 for a name based on tag and 0 for a name based on non-tag can use the macro by telling that the side that is strictly bigger is better). > How about something like > ... I did a sequence of COMPARE() macros that cascade to implement tie-breaking logic because I thought it was the easiest to read and understand, and I did "tag vs non-tag first decides, and then tiebreak to favor shorter hops and then tiebreak on date, while paying attention to committer dates even when we are comparing two commits", because I thought that would be one of the easier logic to explain to the users. But this topic is not my itch, and quite honestly I'd be happier if you took it over, improving both the implementation and the semantics it implements, following your best judgment. You are equipped with better motivation than I am to make these decisions. This patch has turned up a bug in git-p4 in that it seems to have misunderstood that "name-rev HEAD" is a good way to learn which branch we are currently on (it never is); as far as I am concerned, it has served its purpose ;-) Thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags 2017-03-15 22:50 ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano 2017-03-15 22:50 ` [PATCH 1/2] name-rev: refactor logic to see if a new candidate is a better name Junio C Hamano 2017-03-15 22:50 ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Junio C Hamano @ 2017-03-16 0:14 ` Stefan Beller 2017-03-16 10:28 ` Jacob Keller 3 siblings, 0 replies; 46+ messages in thread From: Stefan Beller @ 2017-03-16 0:14 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Michael J Gruber On Wed, Mar 15, 2017 at 3:50 PM, Junio C Hamano <gitster@pobox.com> wrote: > Here is a reroll of what I did in > > http://public-inbox.org/git/xmqqd1die00j.fsf@gitster.mtv.corp.google.com/ > Both patches look good to me. Thanks, Stefan ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags 2017-03-15 22:50 ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano ` (2 preceding siblings ...) 2017-03-16 0:14 ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Stefan Beller @ 2017-03-16 10:28 ` Jacob Keller 3 siblings, 0 replies; 46+ messages in thread From: Jacob Keller @ 2017-03-16 10:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git mailing list, Michael J Gruber On Wed, Mar 15, 2017 at 3:50 PM, Junio C Hamano <gitster@pobox.com> wrote: > Here is a reroll of what I did in > > http://public-inbox.org/git/xmqqd1die00j.fsf@gitster.mtv.corp.google.com/ > > Junio C Hamano (2): > name-rev: refactor logic to see if a new candidate is a better name > name-rev: favor describing with tags and use committer date to tiebreak > > builtin/name-rev.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++------ > t/t4202-log.sh | 2 +- > 2 files changed, 57 insertions(+), 8 deletions(-) > > -- > 2.12.0-306-g4a9b9b32d4 > This looks like an improvement to me. It may be that someone comes up with a better definition for is_better_name() but it's definitely much easier to do so now as it's easier to understand what the logic is when it's separated out. Thanks, Jake ^ permalink raw reply [flat|nested] 46+ messages in thread
end of thread, other threads:[~2017-05-20 5:19 UTC | newest] Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-15 13:15 [PATCH 0/3] describe --contains sanity Michael J Gruber 2017-03-15 13:15 ` [PATCH 1/3] describe: debug is incompatible with contains Michael J Gruber 2017-03-15 19:21 ` Junio C Hamano 2017-03-17 10:54 ` Michael J Gruber 2017-03-15 13:15 ` [PATCH 2/3] git-prompt: add a describe style for any tags Michael J Gruber 2017-03-15 19:25 ` Junio C Hamano 2017-03-17 10:56 ` Michael J Gruber 2017-03-15 13:15 ` [RFD PATCH 3/3] name-rev: Allow lightweight tags and branch refs Michael J Gruber 2017-03-15 16:14 ` Junio C Hamano 2017-03-15 19:50 ` Junio C Hamano 2017-03-15 20:51 ` Junio C Hamano 2017-03-15 22:50 ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Junio C Hamano 2017-03-15 22:50 ` [PATCH 1/2] name-rev: refactor logic to see if a new candidate is a better name Junio C Hamano 2017-03-15 22:50 ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Junio C Hamano 2017-03-17 4:07 ` Lars Schneider 2017-03-17 5:45 ` Junio C Hamano 2017-03-17 5:56 ` Junio C Hamano 2017-03-17 17:09 ` Lars Schneider 2017-03-17 17:17 ` Junio C Hamano 2017-03-17 22:43 ` Junio C Hamano 2017-03-29 14:39 ` [PATCH v2 0/3] name-rev sanity Michael J Gruber 2017-03-29 14:39 ` [PATCH v2 1/3] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber 2017-03-29 14:39 ` [PATCH v2 2/3] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber 2017-03-29 14:39 ` [PATCH v2 3/3] name-rev: provide debug output Michael J Gruber 2017-03-29 17:15 ` [PATCH v2 0/3] name-rev sanity Junio C Hamano 2017-03-29 17:43 ` Junio C Hamano 2017-03-30 13:48 ` Michael J Gruber 2017-03-30 17:30 ` Junio C Hamano 2017-03-31 13:51 ` [PATCH v3 0/4] " Michael J Gruber 2017-03-31 13:51 ` [PATCH v3 1/4] name-rev: refactor logic to see if a new candidate is a better name Michael J Gruber 2017-03-31 13:51 ` [PATCH v3 2/4] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber 2017-03-31 13:51 ` [PATCH v3 3/4] name-rev: provide debug output Michael J Gruber 2017-03-31 16:52 ` Junio C Hamano 2017-03-31 16:55 ` Junio C Hamano 2017-03-31 18:02 ` Michael J Gruber 2017-03-31 18:06 ` Junio C Hamano 2017-03-31 18:33 ` Junio C Hamano 2017-04-03 14:46 ` Michael J Gruber 2017-04-03 17:07 ` Junio C Hamano 2017-05-20 5:19 ` Junio C Hamano 2017-03-31 19:10 ` Junio C Hamano 2017-03-31 13:51 ` [PATCH v3 4/4] describe: pass --debug down to name-rev Michael J Gruber 2017-03-17 11:25 ` [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak Michael J Gruber 2017-03-17 16:03 ` Junio C Hamano 2017-03-16 0:14 ` [PATCH 0/2] Teach name-rev to pay more attention to lightweight tags Stefan Beller 2017-03-16 10:28 ` Jacob Keller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.