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

[Namhyung Kim <namhyung@gmail.com>]

Hi,

2011-03-07 (월), 16:59 -0800, Junio C Hamano:
> 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 + '^' ?


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


> 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