Re: [PATCH v4] for-each-ref: `:short` format for `refname`

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

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> ...
> To integrate this new format into the bash completion to get
> only non-ambiguous refs is beyond the scope of this patch.
>
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>
> ---
> Cc: git@vger.kernel.org
> Cc: szeder@ira.uka.de
> Cc: Shawn O. Pearce <spearce@spearce.org>

Nice writeup of the history of this patch, if on tad-too-verbose side.

> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
> index 21e92bb..9b44092 100644
> --- a/builtin-for-each-ref.c
> +++ b/builtin-for-each-ref.c
> @@ -546,6 +546,107 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
> +/*
> + * Shorten the refname to an non-ambiguous form
> + */
> +static char *get_short_ref(struct refinfo *ref)
> +{
> ...
> +	/* skip first rule, it will always match */
> +	for (i = nr_rules - 1; i > 0 ; --i) {
> +		int j;
> +		int short_name_len;
> +
> +		if (1 != sscanf(ref->refname, scanf_fmts[i], short_name))
> +			continue;
> +
> +		short_name_len = strlen(short_name);
> +
> +		/*
> +		 * check if the short name resolves to a valid ref,
> +		 * but use only rules prior to the matched one
> +		 */
> +		for (j = 0; j < i; j++) {
> ...
> +		}
> +		/*
> +		 * short name is non-ambiguous if all previous rules
> +		 * haven't resolved to a valid ref
> +		 */
> +		if (j == i)
> +			return short_name;

Is this inner loop essentially the same as calling dwim_ref(), while
temporarily turning warn_ambiguous_refs on, and checking for return value
of one?

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 8ced593..4f247dd 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -262,6 +262,50 @@ for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do
>  	"
>  done
>  
> +cat >expected <<\EOF
> +master
> +testtag
> +EOF
> +
> +test_expect_success 'Check short refname format' '
> +	(git for-each-ref --format="%(refname:short)" refs/heads &&
> +	git for-each-ref --format="%(refname:short)" refs/tags) >actual &&
> +	test_cmp expected actual
> +'

Not a complaint nor objection but mere curiosity.  Why does this run two
for-each-ref, not just one with two patterns?

> +test_expect_success 'Check for invalid refname format' '
> +	test_must_fail git for-each-ref --format="%(refname:INVALID)"
> +'

Good.

> +cat >expected <<\EOF
> +heads/master
> +master
> +EOF
> +
> +test_expect_success 'Check ambiguous head and tag refs' '
> +	git checkout -b newtag &&
> +	echo "Using $datestamp" > one &&
> +	git add one &&
> +	git commit -m "Branch" &&
> +	setdate_and_increment &&
> +	git tag -m "Tagging at $datestamp" master &&
> +	git for-each-ref --format "%(refname:short)" refs/heads/master refs/tags/master >actual &&
> +	test_cmp expected actual
> +'
> +
> +cat >expected <<\EOF
> +heads/ambiguous
> +ambiguous
> +EOF
> +
> +test_expect_success 'Check ambiguous head and tag refs II' '
> +	git checkout master &&
> +	git tag ambiguous testtag^0 &&
> +	git branch ambiguous testtag^0 &&
> +	git for-each-ref --format "%(refname:short)" refs/heads/ambiguous refs/tags/ambiguous >actual &&
> +	test_cmp expected actual
> +'
> +

Can we also try first creating a clone of some repo and run:

	for-each-ref --format="%(refname:short)" refs/remotes

I am unsure how "remotes/origin" when "refs/remotes/origin/HEAD" points at
their 'master' branch behaves with your code, and/or how it should behave.

Other than that, nicely done.