threads / patch / 26679

patchblame: honor core.abbrevguard

Subject: [PATCH] blame: honor core.abbrevguard

## tl;dr

4 messages between Mar 7, 2011 and Mar 8, 2011. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Namhyung Kim· Mar 7, 2011, 04:35 UTC · lore

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(-)

Show changes to builtin/blame.c +9 −1

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

Namhyung Kim· Mar 7, 2011, 05:22 UTC · re: Namhyung Kim · lore

[PATCH v2] blame: honor core.abbrevguard

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(-)

Show changes to builtin/blame.c +10 −1

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

Junio C Hamano· Mar 8, 2011, 00:59 UTC · re: Namhyung Kim · lore

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

Namhyung Kim <namhyung@gmail.com> writes:

Show 8 quoted lines

> 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.

Namhyung Kim· Mar 8, 2011, 01:38 UTC · re: Junio C Hamano · lore

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

Hi,

2011-03-07 (월), 16:59 -0800, Junio C Hamano:

Show 15 quoted lines

> 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 + '^' ?

Show 13 quoted lines

> 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. :)

Show 14 quoted lines

> 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