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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 7, 2021, 08:46 UTC
Message-ID: <87r1jmjxdg.fsf@evledraar.gmail.com>
In-Reply-To: <e10007e1cf8ff0005295f648b9489c11a9427122.1617627856.git.gitgitgadget@gmail.com>

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

Show 54 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Add a new method, refspec_item_format(), that takes a 'struct
> refspec_item' pointer as input and returns a string for how that refspec
> item should be written to Git's config or a subcommand, such as 'git
> fetch'.
>
> There are several subtleties regarding special-case refspecs that can
> occur and are represented in t5511-refspec.sh. These cases will be
> explored in new tests in the following change. It requires adding a new
> test helper in order to test this format directly, so that is saved for
> a separate change to keep this one focused on the logic of the format
> method.
>
> A future change will consume this method when translating refspecs in
> the 'prefetch' task of the 'git maintenance' builtin.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  refspec.c | 25 +++++++++++++++++++++++++
>  refspec.h |  5 +++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/refspec.c b/refspec.c
> index e3d852c0bfec..ca65ba01bfe6 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -180,6 +180,31 @@ void refspec_item_clear(struct refspec_item *item)
>  	item->exact_sha1 = 0;
>  }
>  
> +const char *refspec_item_format(const struct refspec_item *rsi)
> +{
> +	static struct strbuf buf = STRBUF_INIT;
> +
> +	strbuf_reset(&buf);
> +
> +	if (rsi->matching)
> +		return ":";
> +
> +	if (rsi->negative)
> +		strbuf_addch(&buf, '^');
> +	else if (rsi->force)
> +		strbuf_addch(&buf, '+');
> +
> +	if (rsi->src)
> +		strbuf_addstr(&buf, rsi->src);
> +
> +	if (rsi->dst) {
> +		strbuf_addch(&buf, ':');
> +		strbuf_addstr(&buf, rsi->dst);
> +	}
> +
> +	return buf.buf;

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

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
    +@
     :

Other than that applying this on top makes everything pass:
    
    diff --git a/builtin/gc.c b/builtin/gc.c
    index 92cb8b4e0bf..ea5572d15c5 100644
    --- a/builtin/gc.c
    +++ b/builtin/gc.c
    @@ -889,35 +889,21 @@ static int fetch_remote(struct remote *remote, void *cbdata)
     		strvec_push(&child.args, "--quiet");
     
     	for (i = 0; i < remote->fetch.nr; i++) {
    -		struct refspec_item replace;
     		struct refspec_item *rsi = &remote->fetch.items[i];
    -		struct strbuf new_dst = STRBUF_INIT;
    -		size_t ignore_len = 0;
    +		struct strbuf new_spec = STRBUF_INIT;
    +		char *pos;
     
     		if (rsi->negative) {
     			strvec_push(&child.args, remote->fetch.raw[i]);
     			continue;
     		}
     
    -		refspec_item_init(&replace, remote->fetch.raw[i], 1);
    -
    -		/*
    -		 * If a refspec dst starts with "refs/" at the start,
    -		 * then we will replace "refs/" with "refs/prefetch/".
    -		 * Otherwise, we will prepend the dst string with
    -		 * "refs/prefetch/".
    -		 */
    -		if (!strncmp(replace.dst, "refs/", 5))
    -			ignore_len = 5;
    -
    -		strbuf_addstr(&new_dst, "refs/prefetch/");
    -		strbuf_addstr(&new_dst, replace.dst + ignore_len);
    -		free(replace.dst);
    -		replace.dst = strbuf_detach(&new_dst, NULL);
    -
    -		strvec_push(&child.args, refspec_item_format(&replace));
    -
    -		refspec_item_clear(&replace);
    +		strbuf_addstr(&new_spec, remote->fetch.raw[i]);
    +		if ((pos = strrchr(new_spec.buf, ':')) != NULL)
    +			strbuf_splice(&new_spec, pos - new_spec.buf + 1, sizeof("refs/") - 1,
    +				      "refs/prefetch/", sizeof("refs/prefetch/") - 1);
    +		strvec_push(&child.args, new_spec.buf);
    +		strbuf_release(&new_spec);
     	}
     
     	return !!run_command(&child);

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?