Re: [PATCH 3/5] refspec: output a refspec item

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Apr 07 2021, Derrick Stolee wrote:

> On 4/7/2021 4:46 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Mon, Apr 05 2021, Derrick Stolee via GitGitGadget wrote:
>>> +	return buf.buf;
>> 
>> There's a downthread discussion about the strbuf usage here so that's
>> covered.
>
> And it's fixed in v2.
>
>> But I'm still confused about the need for this function and the
>> following two patches. If we apply this on top of your series:
>>     
>>     diff --git a/t/helper/test-refspec.c b/t/helper/test-refspec.c
>>     index 08cf441a0a0..9e099e43ebf 100644
>>     --- a/t/helper/test-refspec.c
>>     +++ b/t/helper/test-refspec.c
>>     @@ -31,7 +31,7 @@ int cmd__refspec(int argc, const char **argv)
>>                             continue;
>>                     }
>>     
>>     -               printf("%s\n", refspec_item_format(&rsi));
>>     +               puts(line.buf);
>>                     refspec_item_clear(&rsi);
>>             }
>> 
>> The only failing test is:
>>     
>>     + diff -u expect output
>>     --- expect      2021-04-07 08:12:05.577598038 +0000
>>     +++ output      2021-04-07 08:12:05.577598038 +0000
>>     @@ -11,5 +11,5 @@
>>      refs/heads*/for-linus:refs/remotes/mine/*
>>      2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48:refs/heads/fixed
>>      HEAD
>>     -HEAD
>>     +@
>>      :
>
> It should be obvious that taking refspecs as input, parsing them,
> then reformatting them for output should be almost equivalent to
> printing the input line.
>
> The point is to exercise the logic that actually formats the
> refspec for output. The test-tool clearly does this.
>
> The logic for converting a 'struct refspec_item' to a string is
> non-trivial and worth testing. I don't understand why you are
> concerned that the black-box of the test-tool could be done
> more easily to "trick" the test script.

Yes, but why do we need to convert it to a struct refspec_item in the
first place?

Maybe I'm just overly comfortable with string munging but I think the
smaller patch-on-top to use strbuf_splice() is simpler than adding a new
API just for this use-case.

But I'm still wondering if that @ v.s. HEAD case is something this
series actually needs in its end goal (but then has a missing test?), or
if it was just a "let's test the guts of the refspec.c while we're at
it".

>> So the purpose of this new API is that we don't want to make the
>> assumption that strrchr(buf, ':') is a safe way to find the delimiter in
>> the refspec, or is there some case where we grok "HEAD" but not "@"
>> that's buggy, but not tested for in this series?
>
> The purpose is to allow us to modify a 'struct refspec_item' andproduce a refspec string instead of munging a refspec string
> directly.

But aren't we doing that all over the place, e.g. the grep results for
"refspec_appendf". Even for things purely constructed on the C API level
we pass a const char* now.

I'm not saying it wouldn't be nice to have the refspec.c API changed to
have a clear delimitation between its const char* handling, and C-level
uses which could construct and pass a "struct refspec_item" instead.

But is it *needed* here in a way that I've missed, or is this just a
partial testing/refactoring of that API while we're at it?

[Guessing ahead here because of our TZ difference]:

It seems to me that if this is such a partial refactoring it's a strange
way to go about it.

We're left with freeing/munging the "struct refspec" src/dst in-place
and re-constructing a string that has "+" etc., but we already had that
data in parse_refspec() just before we'd call
refspec_item_format(). That function could then just spew out a
pre-formatted string we'd squirreled away in "struct refspec_item".

If the lengthy paragraph you have at the end of 4/5 holds true, then
such an internal representation doesn't need to have the "refs/" prefix
stores as a const char* (in cases where it's not just "*" or whatever),
no?. We'd then be able to more easily init/copy/munge the refspec for
formatting.