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

["Bert Wesarg" <bert.wesarg@googlemail.com>]

On Sat, Sep 6, 2008 at 00:20, Junio C Hamano <gitster@pobox.com> wrote:
> 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?
Not exactly.

Short version:

To follow my above example, with dwim_ref() we would get this:

heads/xyzzy
tags/xyzzy

Long version:

Currently we consider only rules prior to the matched rule (the rule
which gives us the short name). That is, if any of these rules will
also resolve to a valid ref, the short name is  ambiguous, else its
unambiguous. If we would consider subsequent rules past the matched
one, we may find more valid refs for this short name. Because the
current rule would match first if we try to resolve the short name, we
don't have to check these rules. We get only a "ambiguous refs"
warning.

I have no opinion if we want this 'strict unambiguousness'.

>
>> 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?
Its more a leftover from the :strip version, where the pattern was the point of
interest.

>
>> +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.
I will look at this.


>
> Other than that, nicely done.
>
>

Bert