On Wed, Apr 07 2021, Derrick Stolee wrote:

On 4/7/2021 4:46 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

On Mon, Apr 05 2021, Derrick Stolee via GitGitGadget wrote:

quoted

+	return buf.buf;

There's a downthread discussion about the strbuf usage here so that's
covered.

And it's fixed in v2.

quoted

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".

quoted

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.