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

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

[Namhyung Kim]

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

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

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

[Namhyung Kim]

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

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

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

[Junio C Hamano]

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.

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

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

[Namhyung Kim]

Hello, Junio-san.

2011-04-01 (금), 15:44 -0700, Junio C Hamano:
> 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.


> 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