Re: [PATCH v3 1/2] submodule: use the submodule repository to abbreviate commits

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

"Srujith A Poojary via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/submodule.c b/submodule.c
> index 6fcb606f7e..1302c11ddf 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -543,6 +543,7 @@ static void show_submodule_header(struct diff_options *o,
>  		struct commit **left, struct commit **right,
>  		struct commit_list **merge_bases)
>  {
> +	struct repository *repo = sub ? sub : o->repo;

Let's step back a bit.  Use of any repository other than 'sub' is
wrong as a way to correctly find suitable abbreviation of object
names in the 'sub' repository.  Now, the variable sub is NULL when
the submodule is not initialized, in which case we won't know how
many hexdigits we need to abbreviate "one" and "two" to, because
there is no repository that has any object from that submodule to
begin with.

> @@ -599,9 +600,9 @@ static void show_submodule_header(struct diff_options *o,
>  
>  output_header:
>  	strbuf_addf(&sb, "Submodule %s ", path);
> -	strbuf_add_unique_abbrev(&sb, one, DEFAULT_ABBREV);
> +	strbuf_repo_add_unique_abbrev(&sb, repo, one, DEFAULT_ABBREV);
>  	strbuf_addstr(&sb, (fast_backward || fast_forward) ? ".." : "...");
> -	strbuf_add_unique_abbrev(&sb, two, DEFAULT_ABBREV);
> +	strbuf_repo_add_unique_abbrev(&sb, repo, two, DEFAULT_ABBREV);

And that means the above patch is an utterly wrong rewrite, isn't
it?

Because using o->repo is just as wrong as using the_repository,
replacing strbuf_add_unique_abbrev(), which uses the_repository,
with a call to strbuf_repo_add_unique_abbrev() and using o->repo
does not fix anything.

So when (sub == NULL), the code probably should just grab the first
DEFAULT_ABBREV hexdigits of one and two and be done with it.
Guaranteeing uniqueness of the abbreviations within o->repo does not
help anybody anyway, right?

On the other hand, when (sub != NULL), the rewrite in this patch is
absolutely the right thing to do.  Thanks for working on it.