* [PATCH] name-rev: Fix tag lookup on repository with mixed types of tags
@ 2017-06-22 19:52 orgads
2017-06-22 20:38 ` Stefan Beller
0 siblings, 1 reply; 6+ messages in thread
From: orgads @ 2017-06-22 19:52 UTC (permalink / raw)
To: git; +Cc: Orgad Shaneh
From: Orgad Shaneh <orgads@gmail.com>
Commit 7550424804 (name-rev: include taggerdate in considering the best
name) introduced a bug in name-rev.
If a repository has both annotated and non-annotated tags, annotated
tag will always win, even if it was created decades after the commit.
Consider a repository that always used non-annotated tags, and at some
point started using annotated tags - name-rev --tags will return the
first annotated tags for all the old commits (in our repository it is
followed by ~5067 for one commit, or by ~120^2~21^2~88^2~87 for
another...). This is obviously not what the user expects.
The taggerdate should only be matched if *both tags* have it.
---
builtin/name-rev.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 92a5d8a5d2..8f77023482 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -46,11 +46,13 @@ static void name_rev(struct commit *commit,
commit->util = name;
goto copy_data;
} else if (name->taggerdate > taggerdate ||
- (name->taggerdate == taggerdate &&
+ ((taggerdate == ULONG_MAX || name->taggerdate == taggerdate) &&
name->distance > distance)) {
copy_data:
name->tip_name = tip_name;
- name->taggerdate = taggerdate;
+ if (taggerdate != ULONG_MAX) {
+ name->taggerdate = taggerdate;
+ }
name->generation = generation;
name->distance = distance;
} else
--
2.13.1.windows.1.1.ga36e14b3aa
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] name-rev: Fix tag lookup on repository with mixed types of tags
2017-06-22 19:52 [PATCH] name-rev: Fix tag lookup on repository with mixed types of tags orgads
@ 2017-06-22 20:38 ` Stefan Beller
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Beller @ 2017-06-22 20:38 UTC (permalink / raw)
To: Orgad Shaneh; +Cc: git@vger.kernel.org
On Thu, Jun 22, 2017 at 12:52 PM, <orgads@gmail.com> wrote:
> From: Orgad Shaneh <orgads@gmail.com>
>
> Commit 7550424804 (name-rev: include taggerdate in considering the best
> name) introduced a bug in name-rev.
>
> If a repository has both annotated and non-annotated tags, annotated
> tag will always win, even if it was created decades after the commit.
>
> Consider a repository that always used non-annotated tags, and at some
> point started using annotated tags - name-rev --tags will return the
> first annotated tags for all the old commits (in our repository it is
> followed by ~5067 for one commit, or by ~120^2~21^2~88^2~87 for
> another...). This is obviously not what the user expects.
>
> The taggerdate should only be matched if *both tags* have it.
Please sign off the patch. See https://developercertificate.org/
or Documentation/SubmittingPatches
> ---
> builtin/name-rev.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 92a5d8a5d2..8f77023482 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -46,11 +46,13 @@ static void name_rev(struct commit *commit,
> commit->util = name;
> goto copy_data;
> } else if (name->taggerdate > taggerdate ||
> - (name->taggerdate == taggerdate &&
> + ((taggerdate == ULONG_MAX || name->taggerdate == taggerdate) &&
> name->distance > distance)) {
> copy_data:
> name->tip_name = tip_name;
> - name->taggerdate = taggerdate;
> + if (taggerdate != ULONG_MAX) {
> + name->taggerdate = taggerdate;
> + }
style: you may omit the braces for a single statement
after if.
> name->generation = generation;
> name->distance = distance;
> } else
> --
> 2.13.1.windows.1.1.ga36e14b3aa
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] name-rev: Fix tag lookup on repository with mixed types of tags
@ 2017-10-18 13:24 orgads
2017-10-19 1:54 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: orgads @ 2017-10-18 13:24 UTC (permalink / raw)
To: git; +Cc: Orgad Shaneh
From: Orgad Shaneh <orgads@gmail.com>
Commit 7550424804 (name-rev: include taggerdate in considering the best
name) introduced a bug in name-rev.
If a repository has both annotated and non-annotated tags, annotated
tag will always win, even if it was created decades after the commit.
Consider a repository that always used non-annotated tags, and at some
point started using annotated tags - name-rev --tags will return the
first annotated tags for all the old commits (in our repository it is
followed by ~5067 for one commit, or by ~120^2~21^2~88^2~87 for
another...). This is obviously not what the user expects.
There was an attempt to fix this in ef1e74065, but it is not enough.
The taggerdate should only be matched if *both tags* have it.
Signed-off-by: Orgad Shaneh <orgads@gmail.com>
---
builtin/name-rev.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 9e088ebd11..dc9eaf21fa 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -35,7 +35,7 @@ static int is_better_name(struct rev_name *name,
*/
if (from_tag && name->from_tag)
return (name->taggerdate > taggerdate ||
- (name->taggerdate == taggerdate &&
+ ((taggerdate == TIME_MAX || name->taggerdate == taggerdate) &&
name->distance > distance));
/*
@@ -53,7 +53,7 @@ static int is_better_name(struct rev_name *name,
return name->distance > distance;
/* ... or tiebreak to favor older date */
- if (name->taggerdate != taggerdate)
+ if (taggerdate != TIME_MAX && name->taggerdate != taggerdate)
return name->taggerdate > taggerdate;
/* keep the current one if we cannot decide */
@@ -90,7 +90,8 @@ static void name_rev(struct commit *commit,
generation, distance, from_tag)) {
copy_data:
name->tip_name = tip_name;
- name->taggerdate = taggerdate;
+ if (taggerdate != TIME_MAX)
+ name->taggerdate = taggerdate;
name->generation = generation;
name->distance = distance;
name->from_tag = from_tag;
--
2.14.2.windows.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] name-rev: Fix tag lookup on repository with mixed types of tags
2017-10-18 13:24 orgads
@ 2017-10-19 1:54 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2017-10-19 1:54 UTC (permalink / raw)
To: orgads; +Cc: git
orgads@gmail.com writes:
> From: Orgad Shaneh <orgads@gmail.com>
>
> Commit 7550424804 (name-rev: include taggerdate in considering the best
> name) introduced a bug in name-rev.
> ...
> builtin/name-rev.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
Can you update a test to protect the change this patch makes from
future breakages? That would also demonstrate how the current code
is broken and this patch improves the situation, as I am having a
bit of trouble seeing why this change is needed.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] name-rev: Fix tag lookup on repository with mixed types of tags
@ 2017-06-22 19:42 Orgad Shaneh
2017-06-22 22:03 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Orgad Shaneh @ 2017-06-22 19:42 UTC (permalink / raw)
To: git
Commit 7550424804 (name-rev: include taggerdate in considering the best
name) introduced a bug in name-rev.
If a repository has both annotated and non-annotated tags, annotated
tag will always win, even if it was created decades after the commit.
Consider a repository that always used non-annotated tags, and at some
point started using annotated tags - name-rev --tags will return the
first annotated tags for all the old commits (in our repository it is
followed by ~5067 for one commit, or by ~120^2~21^2~88^2~87 for
another...). This is obviously not what the user expects.
The taggerdate should only be matched if *both tags* have it.
---
builtin/name-rev.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 92a5d8a5d2..8f77023482 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -46,11 +46,13 @@ static void name_rev(struct commit *commit,
commit->util = name;
goto copy_data;
} else if (name->taggerdate > taggerdate ||
- (name->taggerdate == taggerdate &&
+ ((taggerdate == ULONG_MAX || name->taggerdate == taggerdate) &&
name->distance > distance)) {
copy_data:
name->tip_name = tip_name;
- name->taggerdate = taggerdate;
+ if (taggerdate != ULONG_MAX) {
+ name->taggerdate = taggerdate;
+ }
name->generation = generation;
name->distance = distance;
} else
--
2.13.1.windows.1.1.ga36e14b3aa
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] name-rev: Fix tag lookup on repository with mixed types of tags
2017-06-22 19:42 Orgad Shaneh
@ 2017-06-22 22:03 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2017-06-22 22:03 UTC (permalink / raw)
To: Orgad Shaneh; +Cc: git
Orgad Shaneh <orgads@gmail.com> writes:
> Commit 7550424804 (name-rev: include taggerdate in considering the best
> name) introduced a bug in name-rev.
>
> If a repository has both annotated and non-annotated tags, annotated
> tag will always win, even if it was created decades after the commit.
Thanks. It is a problem that light-weight tags are unduly
penalized, and we attempted to address it a few months ago in a
slightly different way. The necessary changes are already in
'master' but not yet in any released branch.
Here is an entry in the Release Notes for the next release that will
come out of the current 'master'.
* "git describe --contains" penalized light-weight tags so much that
they were almost never considered. Instead, give them about the
same chance to be considered as an annotated tag that is the same
age as the underlying commit would.
(merge ef1e74065c jc/name-rev-lw-tag later to maint).
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-19 1:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-22 19:52 [PATCH] name-rev: Fix tag lookup on repository with mixed types of tags orgads
2017-06-22 20:38 ` Stefan Beller
-- strict thread matches above, loose matches on Subject: below --
2017-10-18 13:24 orgads
2017-10-19 1:54 ` Junio C Hamano
2017-06-22 19:42 Orgad Shaneh
2017-06-22 22:03 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).