Re: [PATCH v4 5/6] get_short_oid: sort ambiguous objects by type, then SHA-1

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2018, 05:36 UTC
Message-ID: <xmqq36yyhhyp.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180510124303.6020-6-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 18 quoted lines

> diff --git a/sha1-name.c b/sha1-name.c
> index 9d7bbd3e96..46d8b1afa6 100644
> --- a/sha1-name.c
> +++ b/sha1-name.c
> @@ -409,6 +437,8 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
>  	status = finish_object_disambiguation(&ds, oid);
>  
>  	if (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {
> +		struct oid_array collect = OID_ARRAY_INIT;
> +
>  		error(_("short SHA1 %s is ambiguous"), ds.hex_pfx);
>  
>  		/*
> @@ -421,7 +451,12 @@ static int get_short_oid(const char *name, int len, struct object_id *oid,
>  			ds.fn = NULL;
>  
>  		advise(_("The candidates are:"));
> -		for_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);

So we used to let for_each_abbrev() to enumerate these object names that share the prefix in the object name order and fed show_ambiguous_object() to show them, which was the cause of the output that is not grouped by type. Now you collect them into another oid_array and sort by type, relying on the fact to that the for_each_abbrev() in the "collect" phase already does the de-duping.

Sounds good.

Show 7 quoted lines

> +		for_each_abbrev(ds.hex_pfx, collect_ambiguous, &collect);
> +		QSORT(collect.oid, collect.nr, sort_ambiguous);
> +
> +		if (oid_array_for_each(&collect, show_ambiguous_object, &ds))
> +			BUG("show_ambiguous_object shouldn't return non-zero");
> +		oid_array_clear(&collect);
>  	}

Show 20 quoted lines

> diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
> index 711704ba5a..2701462041 100755
> --- a/t/t1512-rev-parse-disambiguation.sh
> +++ b/t/t1512-rev-parse-disambiguation.sh
> @@ -361,4 +361,25 @@ test_expect_success 'core.disambiguate does not override context' '
>  		git -c core.disambiguate=committish rev-parse $sha1^{tree}
>  '
>  
> +test_expect_success C_LOCALE_OUTPUT 'ambiguous commits are printed by type first, then hash order' '
> +	test_must_fail git rev-parse 0000 2>stderr &&
> +	grep ^hint: stderr >hints &&
> +	grep 0000 hints >objects &&
> +	cat >expected <<-\EOF &&
> +	tag
> +	commit
> +	tree
> +	blob
> +	EOF
> +	awk "{print \$3}" <objects >objects.types &&
> +	uniq <objects.types >objects.types.uniq &&

Eww, that is somewhat tricky (but correct) use of "uniq", which POSIX not just mandates adjacent duplicates to be removed, but also forbids from removing duplicates that are not adjacent from each other. So the objects in the "hints" file are not grouped by type, we will fail to see these four lines.

Show 6 quoted lines

> +	test_cmp expected objects.types.uniq &&
> +	for type in tag commit tree blob
> +	do
> +		grep $type objects >$type.objects &&
> +		sort $type.objects >$type.objects.sorted &&
> +		test_cmp $type.objects.sorted $type.objects

We not only want to see objects grouped by type (and types shown in a desired order), but within the same type we want them ordered by object name.

OK.

> +	done
> +'
> +
>  test_done