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

[Junio C Hamano <gitster@pobox.com>]

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;