git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: tom.saeger@oracle.com, gitster@pobox.com,
	sunshine@sunshineco.com, Derrick Stolee <stolee@gmail.com>,
	Derrick Stolee <derrickstolee@github.com>
Subject: [PATCH v2 0/5] Maintenance: adapt custom refspecs
Date: Tue, 06 Apr 2021 18:47:45 +0000	[thread overview]
Message-ID: <pull.924.v2.git.1617734870.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.924.git.1617627856.gitgitgadget@gmail.com>

Tom Saeger rightly pointed out [1] that the prefetch task ignores custom
refspecs. This can lead to downloading more data than requested, and it
doesn't even help the future foreground fetches that use that custom
refspec.

[1]
https://lore.kernel.org/git/20210401184914.qmr7jhjbhp2mt3h6@dhcp-10-154-148-175.vpn.oracle.com/

This series fixes this problem by carefully replacing the start of each
refspec's destination with "refs/prefetch/". If the destination already
starts with "refs/", then that is replaced. Otherwise "refs/prefetch/" is
just prepended.

In order to accomplish this safely, a new refspec_item_format() method is
created and tested.

Patch 1 is just a preparation patch that makes the code simpler (and in
hindsight it should have been written this way from the start).

Patch 2 is a simplification of test_subcommand that removes the need for
escaping glob characters. Thanks, Eric Sunshine, for the tip of why my tests
were failing on FreeBSD.

Patches 3-4 add refspec_item_format().

Patch 5 finally modifies the logic in the prefetch task to translate these
refspecs.


Updates in V2
=============

Thanks for the close eye on this series. I appreciate the recommendations,
which I believe I have responded to them all:

 * Fixed typos.
 * Made refspec_item_format() re-entrant. Consumers must free the buffer.
 * Cleaned up style (quoting and tabbing).

Thanks, -Stolee

Derrick Stolee (5):
  maintenance: simplify prefetch logic
  test-lib: use exact match for test_subcommand
  refspec: output a refspec item
  test-tool: test refspec input/output
  maintenance: allow custom refspecs during prefetch

 Documentation/git-maintenance.txt |  3 +-
 Makefile                          |  1 +
 builtin/gc.c                      | 66 ++++++++++++++++++++-----------
 refspec.c                         | 23 +++++++++++
 refspec.h                         |  2 +
 t/helper/test-refspec.c           | 44 +++++++++++++++++++++
 t/helper/test-tool.c              |  1 +
 t/helper/test-tool.h              |  1 +
 t/t5511-refspec.sh                | 41 +++++++++++++++++++
 t/t7900-maintenance.sh            | 43 +++++++++++++++++---
 t/test-lib-functions.sh           |  4 +-
 11 files changed, 195 insertions(+), 34 deletions(-)
 create mode 100644 t/helper/test-refspec.c


base-commit: 2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-924%2Fderrickstolee%2Fmaintenance%2Frefspec-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-924/derrickstolee/maintenance/refspec-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/924

Range-diff vs v1:

 1:  3a94ff80657c ! 1:  5aa0cb06c3f2 maintenance: simplify prefetch logic
     @@ Commit message
          The previous logic filled a string list with the names of each remote,
          but instead we could simply run the appropriate 'git fetch' data
          directly in the remote iterator. Do this for reduced code size, but also
     -    becuase it sets up an upcoming change to use the remote's refspec. This
     +    because it sets up an upcoming change to use the remote's refspec. This
          data is accessible from the 'struct remote' data that is now accessible
          in fetch_remote().
      
 2:  2b74889c2a32 ! 2:  d58a3e042ee8 test-lib: use exact match for test_subcommand
     @@ Commit message
      
          The use of 'grep' inside test_subcommand uses general patterns, leading
          to sometimes needing escape characters to avoid incorrect matches.
     -    Further, some platforms interpret different glob characters differently.
     +    Further, some platforms interpret regular expression metacharacters
     +    differently. Furthermore, it can be difficult to know which characters
     +    need escaping since the actual regular expression language implemented
     +    by various `grep`s differs between platforms; for instance, some may
     +    employ pure BRE, whereas others a mix of BRE & ERE.
      
     -    Use 'grep -F' to use an exact match. This requires removing escape
     -    characters from existing callers. Luckily, this is only one test that
     -    expects refspecs as part of the subcommand.
     +    Sidestep this difficulty by using `grep -F` to use an exact match. This
     +    requires removing escape characters from existing callers. Luckily,
     +    this is only one test that expects refspecs as part of the subcommand.
      
     -    Reported-by: Eric Sunshine <sunshine@sunshineco.com>
     +    Helped-by: Eric Sunshine <sunshine@sunshineco.com>
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
       ## t/t7900-maintenance.sh ##
     @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '
       	fetchargs="--prune --no-tags --no-write-fetch-head --recurse-submodules=no --refmap= --quiet" &&
      -	test_subcommand git fetch remote1 $fetchargs +refs/heads/\\*:refs/prefetch/remote1/\\* <run-prefetch.txt &&
      -	test_subcommand git fetch remote2 $fetchargs +refs/heads/\\*:refs/prefetch/remote2/\\* <run-prefetch.txt &&
     -+	test_subcommand git fetch remote1 $fetchargs +refs/heads/*:refs/prefetch/remote1/* <run-prefetch.txt &&
     -+	test_subcommand git fetch remote2 $fetchargs +refs/heads/*:refs/prefetch/remote2/* <run-prefetch.txt &&
     ++	test_subcommand git fetch remote1 $fetchargs "+refs/heads/*:refs/prefetch/remote1/*" <run-prefetch.txt &&
     ++	test_subcommand git fetch remote2 $fetchargs "+refs/heads/*:refs/prefetch/remote2/*" <run-prefetch.txt &&
       	test_path_is_missing .git/refs/remotes &&
       	git log prefetch/remote1/one &&
       	git log prefetch/remote2/two &&
 3:  e10007e1cf8f ! 3:  96388d949b98 refspec: output a refspec item
     @@ refspec.c: void refspec_item_clear(struct refspec_item *item)
       	item->exact_sha1 = 0;
       }
       
     -+const char *refspec_item_format(const struct refspec_item *rsi)
     ++char *refspec_item_format(const struct refspec_item *rsi)
      +{
     -+	static struct strbuf buf = STRBUF_INIT;
     -+
     -+	strbuf_reset(&buf);
     ++	struct strbuf buf = STRBUF_INIT;
      +
      +	if (rsi->matching)
     -+		return ":";
     ++		return xstrdup(":");
      +
      +	if (rsi->negative)
      +		strbuf_addch(&buf, '^');
     @@ refspec.c: void refspec_item_clear(struct refspec_item *item)
      +		strbuf_addstr(&buf, rsi->dst);
      +	}
      +
     -+	return buf.buf;
     ++	return strbuf_detach(&buf, NULL);
      +}
      +
       void refspec_init(struct refspec *rs, int fetch)
     @@ refspec.h: int refspec_item_init(struct refspec_item *item, const char *refspec,
       void refspec_item_init_or_die(struct refspec_item *item, const char *refspec,
       			      int fetch);
       void refspec_item_clear(struct refspec_item *item);
     -+/*
     -+ * Output a given refspec item to a string.
     -+ */
     -+const char *refspec_item_format(const struct refspec_item *rsi);
     ++char *refspec_item_format(const struct refspec_item *rsi);
      +
       void refspec_init(struct refspec *rs, int fetch);
       void refspec_append(struct refspec *rs, const char *refspec);
 4:  c8d1de06f844 ! 4:  bf296282323a test-tool: test refspec input/output
     @@ t/helper/test-refspec.c (new)
      +
      +	while (strbuf_getline(&line, stdin) != EOF) {
      +		struct refspec_item rsi;
     ++		char *buf;
      +
      +		if (!refspec_item_init(&rsi, line.buf, fetch)) {
      +			printf("failed to parse %s\n", line.buf);
      +			continue;
      +		}
      +
     -+		printf("%s\n", refspec_item_format(&rsi));
     ++		buf = refspec_item_format(&rsi);
     ++		printf("%s\n", buf);
     ++		free(buf);
     ++
      +		refspec_item_clear(&rsi);
      +	}
      +
     ++	strbuf_release(&line);
      +	return 0;
      +}
      
     @@ t/t5511-refspec.sh: test_refspec fetch "refs/heads/${good}"
       
      +test_expect_success 'test input/output round trip' '
      +	cat >input <<-\EOF &&
     -+		+refs/heads/*:refs/remotes/origin/*
     -+		refs/heads/*:refs/remotes/origin/*
     -+		refs/heads/main:refs/remotes/frotz/xyzzy
     -+		:refs/remotes/frotz/deleteme
     -+		^refs/heads/secrets
     -+		refs/heads/secret:refs/heads/translated
     -+		refs/heads/secret:heads/translated
     -+		refs/heads/secret:remotes/translated
     -+		secret:translated
     -+		refs/heads/*:remotes/xxy/*
     -+		refs/heads*/for-linus:refs/remotes/mine/*
     -+		2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48:refs/heads/fixed
     -+		HEAD
     -+		@
     -+		:
     ++	+refs/heads/*:refs/remotes/origin/*
     ++	refs/heads/*:refs/remotes/origin/*
     ++	refs/heads/main:refs/remotes/frotz/xyzzy
     ++	:refs/remotes/frotz/deleteme
     ++	^refs/heads/secrets
     ++	refs/heads/secret:refs/heads/translated
     ++	refs/heads/secret:heads/translated
     ++	refs/heads/secret:remotes/translated
     ++	secret:translated
     ++	refs/heads/*:remotes/xxy/*
     ++	refs/heads*/for-linus:refs/remotes/mine/*
     ++	2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48:refs/heads/fixed
     ++	HEAD
     ++	@
     ++	:
      +	EOF
      +	cat >expect <<-\EOF &&
     -+		+refs/heads/*:refs/remotes/origin/*
     -+		refs/heads/*:refs/remotes/origin/*
     -+		refs/heads/main:refs/remotes/frotz/xyzzy
     -+		:refs/remotes/frotz/deleteme
     -+		^refs/heads/secrets
     -+		refs/heads/secret:refs/heads/translated
     -+		refs/heads/secret:heads/translated
     -+		refs/heads/secret:remotes/translated
     -+		secret:translated
     -+		refs/heads/*:remotes/xxy/*
     -+		refs/heads*/for-linus:refs/remotes/mine/*
     -+		2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48:refs/heads/fixed
     -+		HEAD
     -+		HEAD
     -+		:
     ++	+refs/heads/*:refs/remotes/origin/*
     ++	refs/heads/*:refs/remotes/origin/*
     ++	refs/heads/main:refs/remotes/frotz/xyzzy
     ++	:refs/remotes/frotz/deleteme
     ++	^refs/heads/secrets
     ++	refs/heads/secret:refs/heads/translated
     ++	refs/heads/secret:heads/translated
     ++	refs/heads/secret:remotes/translated
     ++	secret:translated
     ++	refs/heads/*:remotes/xxy/*
     ++	refs/heads*/for-linus:refs/remotes/mine/*
     ++	2e36527f23b7f6ae15e6f21ac3b08bf3fed6ee48:refs/heads/fixed
     ++	HEAD
     ++	HEAD
     ++	:
      +	EOF
      +	test-tool refspec <input >output &&
      +	test_cmp expect output &&
 5:  7f6c127dac48 ! 5:  9592224e3d42 maintenance: allow custom refspecs during prefetch
     @@ builtin/gc.c: static int fetch_remote(struct remote *remote, void *cbdata)
      +		struct refspec_item *rsi = &remote->fetch.items[i];
      +		struct strbuf new_dst = STRBUF_INIT;
      +		size_t ignore_len = 0;
     ++		char *replace_string;
      +
      +		if (rsi->negative) {
      +			strvec_push(&child.args, remote->fetch.raw[i]);
     @@ builtin/gc.c: static int fetch_remote(struct remote *remote, void *cbdata)
      +		free(replace.dst);
      +		replace.dst = strbuf_detach(&new_dst, NULL);
      +
     -+		strvec_push(&child.args, refspec_item_format(&replace));
     ++		replace_string = refspec_item_format(&replace);
     ++		strvec_push(&child.args, replace_string);
     ++		free(replace_string);
      +
      +		refspec_item_clear(&replace);
      +	}
     @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '
       	test_commit -C clone2 two &&
       	GIT_TRACE2_EVENT="$(pwd)/run-prefetch.txt" git maintenance run --task=prefetch 2>/dev/null &&
       	fetchargs="--prune --no-tags --no-write-fetch-head --recurse-submodules=no --refmap= --quiet" &&
     --	test_subcommand git fetch remote1 $fetchargs +refs/heads/*:refs/prefetch/remote1/* <run-prefetch.txt &&
     --	test_subcommand git fetch remote2 $fetchargs +refs/heads/*:refs/prefetch/remote2/* <run-prefetch.txt &&
     -+	test_subcommand git fetch remote1 $fetchargs +refs/heads/*:refs/prefetch/remotes/remote1/* <run-prefetch.txt &&
     -+	test_subcommand git fetch remote2 $fetchargs +refs/heads/*:refs/prefetch/remotes/remote2/* <run-prefetch.txt &&
     +-	test_subcommand git fetch remote1 $fetchargs "+refs/heads/*:refs/prefetch/remote1/*" <run-prefetch.txt &&
     +-	test_subcommand git fetch remote2 $fetchargs "+refs/heads/*:refs/prefetch/remote2/*" <run-prefetch.txt &&
     ++	test_subcommand git fetch remote1 $fetchargs "+refs/heads/*:refs/prefetch/remotes/remote1/*" <run-prefetch.txt &&
     ++	test_subcommand git fetch remote2 $fetchargs "+refs/heads/*:refs/prefetch/remotes/remote2/*" <run-prefetch.txt &&
       	test_path_is_missing .git/refs/remotes &&
      -	git log prefetch/remote1/one &&
      -	git log prefetch/remote2/two &&
     @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '
      +	git -C clone1 branch -f special/secret/not-fetched HEAD &&
      +
      +	# create multiple refspecs for remote1
     -+	git config --add remote.remote1.fetch +refs/heads/special/fetched:refs/heads/fetched &&
     -+	git config --add remote.remote1.fetch ^refs/heads/special/secret/not-fetched &&
     ++	git config --add remote.remote1.fetch "+refs/heads/special/fetched:refs/heads/fetched" &&
     ++	git config --add remote.remote1.fetch "^refs/heads/special/secret/not-fetched" &&
      +
      +	GIT_TRACE2_EVENT="$(pwd)/prefetch-refspec.txt" git maintenance run --task=prefetch 2>/dev/null &&
      +
     @@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '
      +	rs2="+refs/heads/special/fetched:refs/prefetch/heads/fetched" &&
      +	rs3="^refs/heads/special/secret/not-fetched" &&
      +
     -+	test_subcommand git fetch remote1 $fetchargs $rs1 $rs2 $rs3 <prefetch-refspec.txt &&
     -+	test_subcommand git fetch remote2 $fetchargs +refs/heads/*:refs/prefetch/remotes/remote2/* <prefetch-refspec.txt &&
     ++	test_subcommand git fetch remote1 $fetchargs "$rs1" "$rs2" "$rs3" <prefetch-refspec.txt &&
     ++	test_subcommand git fetch remote2 $fetchargs "+refs/heads/*:refs/prefetch/remotes/remote2/*" <prefetch-refspec.txt &&
      +
      +	# first refspec is overridden by second
      +	test_must_fail git rev-parse refs/prefetch/special/fetched &&

-- 
gitgitgadget

  parent reply	other threads:[~2021-04-06 18: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
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 ` Derrick Stolee via GitGitGadget [this message]
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=pull.924.v2.git.1617734870.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=stolee@gmail.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).