threads / patch / 26953

patch, 2 partsblame: add --abbrev command line option

Subject: [PATCH 1/2] blame: add --abbrev command line option

## tl;dr

4 messages between Apr 1, 2011 and Apr 2, 2011. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Namhyung Kim· Apr 1, 2011, 01:54 UTC · lore
Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 builtin/blame.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Show changes to builtin/blame.c +3 −1
diff --git a/builtin/blame.c b/builtin/blame.c
index f6b03f7..253b480 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -41,6 +41,7 @@ static int reverse;
 static int blank_boundary;
 static int incremental;
 static int xdl_opts;
+static int abbrev = 8;
 
 static enum date_mode blame_date_mode = DATE_ISO8601;
 static size_t blame_date_width;
@@ -1670,7 +1671,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
 	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 = (opt & OUTPUT_LONG_OBJECT_NAME) ? 40 : abbrev;
 
 		if (suspect->commit->object.flags & UNINTERESTING) {
 			if (blank_boundary)
@@ -2310,6 +2311,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 'C', NULL, &opt, "score", "Find line copies within and across files", PARSE_OPT_OPTARG, blame_copy_callback },
 		{ OPTION_CALLBACK, 'M', NULL, &opt, "score", "Find line movements within and across files", PARSE_OPT_OPTARG, blame_move_callback },
 		OPT_CALLBACK('L', NULL, &bottomtop, "n,m", "Process only line range n,m, counting from 1", blame_bottomtop_callback),
+		OPT__ABBREV(&abbrev),
 		OPT_END()
 	};
 

-- 
1.7.4
Namhyung Kim· Apr 1, 2011, 01:54 UTC · re: Namhyung Kim · lore

[PATCH 2/2] blame: honor core.abbrev

If user sets config.abbrev option, use it as if --abbrev was given. We can't set abbrev to default_abbrev unconditionally IMHO, because historically default abbrev length of the blame command is 8 and DEFAULT_ABBREV is 7.

As you may see, this is the default value and user can use different abbrev length by specifing --abbrev=N command line option.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 builtin/blame.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Show changes to builtin/blame.c +4 −0
diff --git a/builtin/blame.c b/builtin/blame.c
index 253b480..93693d2 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2317,12 +2317,16 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 
 	struct parse_opt_ctx_t ctx;
 	int cmd_is_annotate = !strcmp(argv[0], "annotate");
+	int orig_abbrev = DEFAULT_ABBREV;
 
 	git_config(git_blame_config, NULL);
 	init_revisions(&revs, NULL);
 	revs.date_mode = blame_date_mode;
 	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
 
+	/* if user sets config.abbrev, honor it */
+	if (orig_abbrev != default_abbrev)
+		abbrev = default_abbrev;
 	save_commit_buffer = 0;
 	dashdash_pos = 0;
 

-- 
1.7.4
Junio C Hamano· Apr 1, 2011, 22:44 UTC · re: Namhyung Kim · lore

Re: [PATCH 2/2] blame: honor core.abbrev

Namhyung Kim <namhyung@gmail.com> writes:
> If user sets config.abbrev option, use it as if --abbrev was given.
> We can't set abbrev to default_abbrev unconditionally IMHO, because
> historically default abbrev length of the blame command is 8 and
> DEFAULT_ABBREV is 7.

Isn't the one-letter difference because we sometimes need to show the boundary commit with a caret at the beginning?

I think the way this patch initializes orig_abbrev using DEFAULT_ABBREV is wrong (at that point, I don't think you have called git_config() to get the user config for DEFAULT_ABBREV).

See the patch to describe.c in dce9648 (Make the default abbrev length configurable, 2010-10-28) for the right way to do this.

 - initialize the variable to -1;
 - call git_config() to get correct value in DEFAULT_ABBREV;
 - call parse_options() to potentially update the variable; then
 - if variable is still -1, assign DEFAULT_ABBREV to it.
After all that, add 1 to it to account for the possible boundary caret.
Show 20 quoted lines
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 253b480..93693d2 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2317,12 +2317,16 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  
>  	struct parse_opt_ctx_t ctx;
>  	int cmd_is_annotate = !strcmp(argv[0], "annotate");
> +	int orig_abbrev = DEFAULT_ABBREV;
>  
>  	git_config(git_blame_config, NULL);
>  	init_revisions(&revs, NULL);
>  	revs.date_mode = blame_date_mode;
>  	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
>  
> +	/* if user sets config.abbrev, honor it */
> +	if (orig_abbrev != default_abbrev)
> +		abbrev = default_abbrev;
>  	save_commit_buffer = 0;
>  	dashdash_pos = 0;
Namhyung Kim· Apr 2, 2011, 01:37 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] blame: honor core.abbrev

Hello, Junio-san.
2011-04-01 (금), 15:44 -0700, Junio C Hamano:
Show 10 quoted lines
> Namhyung Kim <namhyung@gmail.com> writes:
> 
> > If user sets config.abbrev option, use it as if --abbrev was given.
> > We can't set abbrev to default_abbrev unconditionally IMHO, because
> > historically default abbrev length of the blame command is 8 and
> > DEFAULT_ABBREV is 7.
> 
> Isn't the one-letter difference because we sometimes need to show the
> boundary commit with a caret at the beginning?
> 
Yeah, I guessed so.
> I think the way this patch initializes orig_abbrev using DEFAULT_ABBREV is
> wrong (at that point, I don't think you have called git_config() to get
> the user config for DEFAULT_ABBREV).
>

My intention was that I want to check whether the user sets the option or not and apply the value only if it is set (ig. different from "7"). Please see below.

Show 10 quoted lines
> See the patch to describe.c in dce9648 (Make the default abbrev length
> configurable, 2010-10-28) for the right way to do this.
> 
>  - initialize the variable to -1;
>  - call git_config() to get correct value in DEFAULT_ABBREV;
>  - call parse_options() to potentially update the variable; then
>  - if variable is still -1, assign DEFAULT_ABBREV to it.
> 
> After all that, add 1 to it to account for the possible boundary caret.
>

I saw the patch already, and, yes, it would be a simple and probably the right way to do. But it could give a bit of confusion to the users IMHO. Say, A user specify --abbbrev=10 then he/she might expect 10 hexdigits in the output but actually [s]he will get 11.

So I decided to use the number as is, not adding 1. And this would be a constant behavior from the patch 1/2 and all other git command which use --abbrev option, I guess. But I kept to use 8 in case the user doesn't give the config or command-line option, just for backward compatibility.

I'm not sure this is really a concern. Maybe we can give them 11 as you said and document the behavior in the manual page.

Thanks.
-- 
Regards,
Namhyung Kim

← back to recent threads