Re: [PATCH 3/5] refspec: output a refspec item
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Apr 7, 2021, 22:05 UTC
- Message-ID
- <87czv5kaxw.fsf@evledraar.gmail.com>
- In-Reply-To
- <b3e00d3e-c782-9f2a-14e0-f576e50a7e55@gmail.com>
On Wed, Apr 07 2021, Derrick Stolee wrote:
Show 50 quoted lines
> 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".
Show 7 quoted lines
>> 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.