git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, tom.saeger@oracle.com, gitster@pobox.com,
	sunshine@sunshineco.com,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 3/5] refspec: output a refspec item
Date: Wed, 07 Apr 2021 10:46:03 +0200	[thread overview]
Message-ID: <87r1jmjxdg.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <e10007e1cf8ff0005295f648b9489c11a9427122.1617627856.git.gitgitgadget@gmail.com>


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

> 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?

  parent reply	other threads:[~2021-04-07  8:47 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-05 13:04 [PATCH 0/5] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
2021-04-05 13:04 ` [PATCH 1/5] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
2021-04-05 17:01   ` Tom Saeger
2021-04-05 13:04 ` [PATCH 2/5] test-lib: use exact match for test_subcommand Derrick Stolee via GitGitGadget
2021-04-05 17:31   ` Eric Sunshine
2021-04-05 17:43     ` Junio C Hamano
2021-04-05 13:04 ` [PATCH 3/5] refspec: output a refspec item Derrick Stolee via GitGitGadget
2021-04-05 16:57   ` Tom Saeger
2021-04-05 17:40     ` Eric Sunshine
2021-04-05 17:44       ` Junio C Hamano
2021-04-06 11:21         ` Derrick Stolee
2021-04-06 15:23           ` Eric Sunshine
2021-04-06 16:51             ` Derrick Stolee
2021-04-07  8:46   ` Ævar Arnfjörð Bjarmason [this message]
2021-04-07 20:53     ` Derrick Stolee
2021-04-07 22:05       ` Ævar Arnfjörð Bjarmason
2021-04-07 22:49         ` Junio C Hamano
2021-04-07 23:01           ` Ævar Arnfjörð Bjarmason
2021-04-08  7:33             ` Junio C Hamano
2021-04-05 13:04 ` [PATCH 4/5] test-tool: test refspec input/output Derrick Stolee via GitGitGadget
2021-04-05 17:52   ` Eric Sunshine
2021-04-06 11:13     ` Derrick Stolee
2021-04-07  8:54   ` Ævar Arnfjörð Bjarmason
2021-04-05 13:04 ` [PATCH 5/5] maintenance: allow custom refspecs during prefetch Derrick Stolee via GitGitGadget
2021-04-05 17:16   ` Tom Saeger
2021-04-06 11:15     ` Derrick Stolee
2021-04-07  8:53   ` Ævar Arnfjörð Bjarmason
2021-04-07 10:26     ` Ævar Arnfjörð Bjarmason
2021-04-09 11:48       ` Derrick Stolee
2021-04-09 19:28         ` Ævar Arnfjörð Bjarmason
2021-04-10  0:56           ` Derrick Stolee
2021-04-10 11:37             ` Ævar Arnfjörð Bjarmason
2021-04-07 13:47   ` Ævar Arnfjörð Bjarmason
2021-04-06 18:47 ` [PATCH v2 0/5] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
2021-04-06 18:47   ` [PATCH v2 1/5] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
2021-04-07 23:23     ` Emily Shaffer
2021-04-09 19:00       ` Derrick Stolee
2021-04-06 18:47   ` [PATCH v2 2/5] test-lib: use exact match for test_subcommand Derrick Stolee via GitGitGadget
2021-04-06 18:47   ` [PATCH v2 3/5] refspec: output a refspec item Derrick Stolee via GitGitGadget
2021-04-06 18:47   ` [PATCH v2 4/5] test-tool: test refspec input/output Derrick Stolee via GitGitGadget
2021-04-07 23:08     ` Josh Steadmon
2021-04-07 23:26     ` Emily Shaffer
2021-04-06 18:47   ` [PATCH v2 5/5] maintenance: allow custom refspecs during prefetch Derrick Stolee via GitGitGadget
2021-04-06 19:36     ` Tom Saeger
2021-04-06 19:45       ` Derrick Stolee
2021-04-07 23:09     ` Josh Steadmon
2021-04-07 23:37     ` Emily Shaffer
2021-04-08  0:23     ` Jonathan Tan
2021-04-10  2:03   ` [PATCH v3 0/3] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
2021-04-10  2:03     ` [PATCH v3 1/3] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
2021-04-12 20:13       ` Tom Saeger
2021-04-12 20:27         ` Derrick Stolee
2021-04-10  2:03     ` [PATCH v3 2/3] fetch: add --prefetch option Derrick Stolee via GitGitGadget
2021-04-11 21:09       ` Ramsay Jones
2021-04-12 20:23         ` Derrick Stolee
2021-04-10  2:03     ` [PATCH v3 3/3] maintenance: use 'git fetch --prefetch' Derrick Stolee via GitGitGadget
2021-04-11  1:35     ` [PATCH v3 0/3] Maintenance: adapt custom refspecs Junio C Hamano
2021-04-12 16:48       ` Tom Saeger
2021-04-12 17:24         ` Tom Saeger
2021-04-12 17:41           ` Tom Saeger
2021-04-12 20:25             ` Derrick Stolee
2021-04-16 12:49     ` [PATCH v4 0/4] " Derrick Stolee via GitGitGadget
2021-04-16 12:49       ` [PATCH v4 1/4] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
2021-04-16 18:02         ` Tom Saeger
2021-04-16 12:49       ` [PATCH v4 2/4] fetch: add --prefetch option Derrick Stolee via GitGitGadget
2021-04-16 17:52         ` Tom Saeger
2021-04-16 18:26           ` Tom Saeger
2021-04-16 12:49       ` [PATCH v4 3/4] maintenance: use 'git fetch --prefetch' Derrick Stolee via GitGitGadget
2021-04-16 12:49       ` [PATCH v4 4/4] maintenance: respect remote.*.skipFetchAll Derrick Stolee via GitGitGadget
2021-04-16 13:54         ` Ævar Arnfjörð Bjarmason
2021-04-16 14:33           ` Tom Saeger
2021-04-16 18:31         ` Tom Saeger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r1jmjxdg.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=sunshine@sunshineco.com \
    --cc=tom.saeger@oracle.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).