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

From: Namhyung Kim <namhyung@gmail.com>
Date: Apr 2, 2011, 01:37 UTC
Message-ID: <1301708228.1507.34.camel@leonhard>
In-Reply-To: <7vbp0pegmb.fsf@alter.siamese.dyndns.org>

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