[PATCH] blame: honor core.abbrevguard

[PATCH] blame: honor core.abbrevguard

[Namhyung Kim]

Commit 72a5b561 ("core.abbrevguard: Ensure short object names stay
unique a bit longer") introduced this config variable to make object
name unambiguously. Use it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 builtin/blame.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index aa30ec52692..d1cf22beba1 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1664,14 +1664,22 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
 	struct commit_info ci;
 	char hex[41];
 	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
+	int length;
 
 	get_commit_info(suspect->commit, &ci, 1);
 	strcpy(hex, sha1_to_hex(suspect->commit->object.sha1));
 
+	if (opt & OUTPUT_LONG_OBJECT_NAME)
+		length = 40;
+	else {
+		length = 8 + unique_abbrev_extra_length;
+		if (length > 40)
+			length = 40;
+	}
+
 	cp = nth_line(sb, ent->lno);
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
-		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? 40 : 8;
 
 		if (suspect->commit->object.flags & UNINTERESTING) {
 			if (blank_boundary)
-- 
1.7.4

[PATCH v2] blame: honor core.abbrevguard

[Namhyung Kim]

Commit 72a5b561 ("core.abbrevguard: Ensure short object names stay
unique a bit longer") introduced this config variable to make object
name unambiguously. Use it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
v2:
 - re-init 'length' in every loop

 builtin/blame.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index aa30ec52692..cecfceea45f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1664,14 +1664,23 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
 	struct commit_info ci;
 	char hex[41];
 	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
+	int uniq_length;
 
 	get_commit_info(suspect->commit, &ci, 1);
 	strcpy(hex, sha1_to_hex(suspect->commit->object.sha1));
 
+	if (opt & OUTPUT_LONG_OBJECT_NAME)
+		uniq_length = 40;
+	else {
+		uniq_length = 8 + unique_abbrev_extra_length;
+		if (uniq_length > 40)
+			uniq_length = 40;
+	}
+
 	cp = nth_line(sb, ent->lno);
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
-		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? 40 : 8;
+		int length = uniq_length;
 
 		if (suspect->commit->object.flags & UNINTERESTING) {
 			if (blank_boundary)
-- 
1.7.4

Re: [PATCH v2] blame: honor core.abbrevguard

[Junio C Hamano]

Namhyung Kim <namhyung@gmail.com> writes:

> Commit 72a5b561 ("core.abbrevguard: Ensure short object names stay
> unique a bit longer") introduced this config variable to make object
> name unambiguously. Use it.
>
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
> v2:
>  - re-init 'length' in every loop

In this codepath, we only aim to make sure that the shortened object name
is of the same length.  By choosing hardcoded 8, the original clearly
declares that we do not _care_ about the uniqueness.

The abbrev-guard that adds to the length that ensures the uniqueness in
the particular repository has a very different semantics.  It only makes
sense if you add to a length that you know would make the object names
minimally unique.  Adding it to hardcoded 8 does not make it less
ambiguous the same way as the configuration variable intends to do.

So I am not entirely happy with this patch.

Besides, when OUTPUT_LONG_OBJECT_NAME is specified, the value this
variable holds is _not_ "uniq_length", so the name of the variable is not
quite correct.  We colloquially call the object name "sha1" in the code,
so "sha1_length" would be a better name for it.

If we _were_ to do a change that uses the configuration variable, I would
imagine that it would be a part of a change that makes sure that the
shortened object names in the output actually uniquely identify the
commits.  It would involve find_unique_abbrev() for as many commits as the
number of lines in the blamed range at most (and at that point the abbrev
guard would automatically taken into account because find_unique_abbrev()
already does so) before calling "output()"; I suspect that find_alignment()
is the right function to do so, as that is where the column alignment
logic all happens.

It would probably need to be introduced as a new feature, with its own
command line option, similar to -l option that forces the full 40-hex
output.

So thanks for a patch, but I don't think we would want to take it in the
current shape.

Re: [PATCH v2] blame: honor core.abbrevguard

[Namhyung Kim]

Hi,

2011-03-07 (월), 16:59 -0800, Junio C Hamano:
> Namhyung Kim <namhyung@gmail.com> writes:
> 
> > Commit 72a5b561 ("core.abbrevguard: Ensure short object names stay
> > unique a bit longer") introduced this config variable to make object
> > name unambiguously. Use it.
> >
> > Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> > ---
> > v2:
> >  - re-init 'length' in every loop
> 
> In this codepath, we only aim to make sure that the shortened object name
> is of the same length.  By choosing hardcoded 8, the original clearly
> declares that we do not _care_ about the uniqueness.
> 

Just out of curiousity, where did the number come from? I guess
DEFAULT_ABBREV + '^' ?


> The abbrev-guard that adds to the length that ensures the uniqueness in
> the particular repository has a very different semantics.  It only makes
> sense if you add to a length that you know would make the object names
> minimally unique.  Adding it to hardcoded 8 does not make it less
> ambiguous the same way as the configuration variable intends to do.
> 
> So I am not entirely happy with this patch.
> 
> Besides, when OUTPUT_LONG_OBJECT_NAME is specified, the value this
> variable holds is _not_ "uniq_length", so the name of the variable is not
> quite correct.  We colloquially call the object name "sha1" in the code,
> so "sha1_length" would be a better name for it.
> 

OK. I was not happy with the name, too. :)


> If we _were_ to do a change that uses the configuration variable, I would
> imagine that it would be a part of a change that makes sure that the
> shortened object names in the output actually uniquely identify the
> commits.  It would involve find_unique_abbrev() for as many commits as the
> number of lines in the blamed range at most (and at that point the abbrev
> guard would automatically taken into account because find_unique_abbrev()
> already does so) before calling "output()"; I suspect that find_alignment()
> is the right function to do so, as that is where the column alignment
> logic all happens.
> 
> It would probably need to be introduced as a new feature, with its own
> command line option, similar to -l option that forces the full 40-hex
> output.
> 

Looks Great. How about -u/--unique? I'm also thinking about --abbrev
too, isn't it useful at all?


> So thanks for a patch, but I don't think we would want to take it in the
> current shape.

Thanks for your kindly comment.


-- 
Regards,
Namhyung Kim