Maintenance: adapt custom refspecs

From: Derrick Stolee <[email protected]>

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

data is accessible from the 'struct remote' data that is now accessible in fetch_remote().

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/gc.c | 33 ++++++++-------------------------
 1 file changed, 8 insertions(+), 25 deletions(-)

builtin/gc.c +8 -25

index ef7226d7bca4..fa8128de9ae1 100644
@@ -873,55 +873,38 @@ static int maintenance_task_commit_graph(struct maintenance_run_opts *opts)
 	return 0;
 }
 
-static int fetch_remote(const char *remote, struct maintenance_run_opts *opts)
+static int fetch_remote(struct remote *remote, void *cbdata)
 {
+	struct maintenance_run_opts *opts = cbdata;
 	struct child_process child = CHILD_PROCESS_INIT;
 
 	child.git_cmd = 1;
-	strvec_pushl(&child.args, "fetch", remote, "--prune", "--no-tags",
+	strvec_pushl(&child.args, "fetch", remote->name, "--prune", "--no-tags",
 		     "--no-write-fetch-head", "--recurse-submodules=no",
 		     "--refmap=", NULL);
 
 	if (opts->quiet)
 		strvec_push(&child.args, "--quiet");
 
-	strvec_pushf(&child.args, "+refs/heads/*:refs/prefetch/%s/*", remote);
+	strvec_pushf(&child.args, "+refs/heads/*:refs/prefetch/%s/*", remote->name);
 
 	return !!run_command(&child);
 }
 
-static int append_remote(struct remote *remote, void *cbdata)
-{
-	struct string_list *remotes = (struct string_list *)cbdata;
-
-	string_list_append(remotes, remote->name);
-	return 0;
-}
-
 static int maintenance_task_prefetch(struct maintenance_run_opts *opts)
 {
-	int result = 0;
-	struct string_list_item *item;
-	struct string_list remotes = STRING_LIST_INIT_DUP;
-
 	git_config_set_multivar_gently("log.excludedecoration",
 					"refs/prefetch/",
 					"refs/prefetch/",
 					CONFIG_FLAGS_FIXED_VALUE |
 					CONFIG_FLAGS_MULTI_REPLACE);
 
-	if (for_each_remote(append_remote, &remotes)) {
-		error(_("failed to fill remotes"));
-		result = 1;
-		goto cleanup;
+	if (for_each_remote(fetch_remote, opts)) {
+		error(_("failed to prefetch remotes"));
+		return 1;
 	}
 
-	for_each_string_list_item(item, &remotes)
-		result |= fetch_remote(item->string, opts);
-
-cleanup:
-	string_list_clear(&remotes, 0);
-	return result;
+	return 0;
 }
 
 static int maintenance_task_gc(struct maintenance_run_opts *opts)

Signature · 1 line

gitgitgadget

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.

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.

Reported-by Eric Sunshine <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

Quoted 3 lines · Derrick Stolee via GitGitGadget

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.

These are regular expression metacharacters, not glob characters. A more general way to say this might be:

Furthermore, it can be difficult to know which characters need escaping since the actual regular expression language implemented by various greps differs between platforms; for instance, some may employ pure BRE, whereas others a mix of BRE & ERE.

Sidestep this difficulty by using grep -F...

Quoted 6 lines

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.

Reported-by Eric Sunshine <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

The Reported-by: feels a bit unusual in this context. Perhaps
Helped-by: would be more appropriate.

t/t7900-maintenance.sh test_expect_success 'prefetch multiple remotes' ' · line 143

Quoted 6 lines

t/t7900-maintenance.sh +2 -2

@@ -142,8 +142,8 @@ test_expect_success 'prefetch multiple remotes' '
-       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 &&

To be really robust and avoid accidental glob expansion (as unlikely as it is), you should quote any arguments which contain glob metacharacters such as "*" rather than supplying them bare like this.

From: Derrick Stolee <[email protected]>

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 <[email protected]>

---
 refspec.c | 25 +++++++++++++++++++++++++
 refspec.h |  5 +++++
 2 files changed, 30 insertions(+)

refspec.c +0 -0

index e3d852c0bfec..ca65ba01bfe6 100644

refspec.c +8 -0

+
+	if (rsi->matching)
+		return ":";
+
+	if (rsi->negative)
+		strbuf_addch(&buf, '^');
+	else if (rsi->force)
+		strbuf_addch(&buf, '+');

refspec.c +2 -0

+}
+
 void refspec_init(struct refspec *rs, int fetch)
 {
 	memset(rs, 0, sizeof(*rs));

refspec.h +5 -0

index 8b79891d3218..92a312f5b4e6 100644
@@ -56,6 +56,11 @@ 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);
+
 void refspec_init(struct refspec *rs, int fetch);
 void refspec_append(struct refspec *rs, const char *refspec);
 __attribute__((format (printf,2,3)))

Signature · 1 line

gitgitgadget

refspec.c void refspec_item_clear(struct refspec_item *item) · line 187

Quoted 5 lines · Derrick Stolee via GitGitGadget

refspec.c +5 -0

+const char *refspec_item_format(const struct refspec_item *rsi)
+{
+     static struct strbuf buf = STRBUF_INIT;
+
+     strbuf_reset(&buf);

is this even needed?

Quoted 11 lines

refspec.c +11 -0

+     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);
+

refspec.c void refspec_item_clear(struct refspec_item *item) · line 205

Quoted 6 lines

refspec.c +6 -0

+     if (rsi->dst) {
+             strbuf_addch(&buf, ':');
+             strbuf_addstr(&buf, rsi->dst);
+     }
+
+     return buf.buf;

should this be strbuf_detach?

Quoted 8 lines · Tom Saeger

refspec.c void refspec_item_clear(struct refspec_item *item) · line 187

Quoted 5 lines · Derrick Stolee via GitGitGadget

refspec.c +5 -0

+const char *refspec_item_format(const struct refspec_item *rsi)
+{
+     static struct strbuf buf = STRBUF_INIT;
+
+     strbuf_reset(&buf);

is this even needed?

This is needed due to the static strbuf declaration (which is easy to overlook).

Quoted 11 lines

Quoted 11 lines

refspec.c +11 -0

+     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);
+

Quoted 8 lines

refspec.c void refspec_item_clear(struct refspec_item *item) · line 205

Quoted 6 lines

refspec.c +6 -0

+     if (rsi->dst) {
+             strbuf_addch(&buf, ':');
+             strbuf_addstr(&buf, rsi->dst);
+     }
+
+     return buf.buf;

should this be strbuf_detach?

In normal circumstances, yes, however, with the static strbuf, this is correct.

However, a more significant question, perhaps, is why this is using a static strbuf in the first place? Does this need to be optimized because it is on a hot path? If not, then the only obvious reason why static was chosen was that sometimes the function returns a string literal and sometimes a constructed string. However, that's minor, and it would feel cleaner to avoid the static strbuf altogether by using strbuf_detach() for the constructed case and xstrdup() for the string literal case, and making it the caller's responsibility to free the result. (The comment in the header file would need to be updated to say as much.)

Quoted 36 lines · Eric Sunshine

Quoted 8 lines · Tom Saeger

refspec.c void refspec_item_clear(struct refspec_item *item) · line 187

Quoted 5 lines · Derrick Stolee via GitGitGadget

refspec.c +5 -0

+const char *refspec_item_format(const struct refspec_item *rsi)
+{
+     static struct strbuf buf = STRBUF_INIT;
+
+     strbuf_reset(&buf);

is this even needed?

This is needed due to the static strbuf declaration (which is easy to overlook).

Quoted 11 lines

Quoted 11 lines

refspec.c +11 -0

+     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);
+

Quoted 8 lines

refspec.c void refspec_item_clear(struct refspec_item *item) · line 205

Quoted 6 lines

refspec.c +6 -0

+     if (rsi->dst) {
+             strbuf_addch(&buf, ':');
+             strbuf_addstr(&buf, rsi->dst);
+     }
+
+     return buf.buf;

should this be strbuf_detach?

In normal circumstances, yes, however, with the static strbuf, this is correct.

Quoted 10 lines

However, a more significant question, perhaps, is why this is using a static strbuf in the first place? Does this need to be optimized because it is on a hot path? If not, then the only obvious reason why static was chosen was that sometimes the function returns a string literal and sometimes a constructed string. However, that's minor, and it would feel cleaner to avoid the static strbuf altogether by using strbuf_detach() for the constructed case and xstrdup() for the string literal case, and making it the caller's responsibility to free the result. (The comment in the header file would need to be updated to say as much.)

Very good suggestion. That would also make this codepath thread-safe (I do not offhand know how important that is, though).

I was not intending to make this re-entrant/thread safe. The intention was to make it easy to consume the formatted string into output such as a printf without needing to store a temporary 'char *' and free() it afterwards. This ensures that the only lost memory over the life of the process is at most one buffer. At minimum, these are things that could be part of the message to justify this design.

So, I'm torn. This seems like a case where there is value in having the return buffer be "owned" by this method, and the expected consumers will use the buffer before calling it again. I'm not sure how important it is to do this the other way.

Would it be sufficient to justify this choice in the commit message and comment about it in the method declaration? Or is it worth adding this templating around every caller:

        char *buf = refspec_item_format(rsi);
        ...
        <use 'buf'>
        ...
        free(buf);

I don't need much convincing to do this, but I hadn't properly described my opinion before. Just a small nudge would convince me to do it this way.

Quoted 6 lines · Derrick Stolee

I was not intending to make this re-entrant/thread safe. The intention was to make it easy to consume the formatted string into output such as a printf without needing to store a temporary 'char *' and free() it afterwards. This ensures that the only lost memory over the life of the process is at most one buffer. At minimum, these are things that could be part of the message to justify this design.

This has the failing that it won't work if someone calls it twice in the same printf() or calls it again before even consuming the first returned value, so this fails:

    printf("foo: %s\nbar: %s\n",
        refspec_item_format(...),
        refspec_item_format(...));

as does this:

    const char *a = refspec_item_format(...);
    const char *b = refspec_item_format(...);

Historically this project would "work around" that problem by using rotating static buffers in the function, but we've mostly been moving away from that for several reasons (can't predict how many buffers will be needed, re-entrancy, etc.).

Quoted 4 lines

So, I'm torn. This seems like a case where there is value in having the return buffer be "owned" by this method, and the expected consumers will use the buffer before calling it again. I'm not sure how important it is to do this the other way.

If history is any indication, we'd probably end up moving away from such an API eventually anyhow.

Quoted 9 lines

Would it be sufficient to justify this choice in the commit message and comment about it in the method declaration? Or is it worth adding this templating around every caller:

        char *buf = refspec_item_format(rsi);
        ...
        <use 'buf'>
        ...
        free(buf);

An alternative would be to have the caller pass in a strbuf to be populated by the function. It doesn't reduce the boilerplate needed by the caller (still need to create and release the strbuf), but may avoid some memory allocations. But if this isn't a critical path and won't likely ever be, then passing in strbuf may be overkill.

Quoted 3 lines

I don't need much convincing to do this, but I hadn't properly described my opinion before. Just a small nudge would convince me to do it this way.

For the reasons described above and earlier in the thread, avoiding the static buffer seems the best course of action.

From: Derrick Stolee <[email protected]>

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 <[email protected]>

---
 refspec.c | 25 +++++++++++++++++++++++++
 refspec.h |  5 +++++
 2 files changed, 30 insertions(+)

refspec.c +13 -0

index e3d852c0bfec..ca65ba01bfe6 100644
@@ -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, '+');

refspec.c void refspec_item_clear(struct refspec_item *item) · line 205

Quoted 1 line · Derrick Stolee via GitGitGadget

refspec.c +1 -0

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

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?

Quoted 6 lines · Ævar Arnfjörð Bjarmason

refspec.c void refspec_item_clear(struct refspec_item *item) · line 205

Quoted 1 line · Derrick Stolee via GitGitGadget

refspec.c +1 -0

+	return buf.buf;

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

And it's fixed in v2.

Quoted 18 lines

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:

Quoted 10 lines

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

Quoted 4 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.

Quoted 3 lines · Derrick Stolee

The purpose is to allow us to modify a 'struct refspec_item' andproduce a refspec string instead of munging a refspec string directly.

Quoted 5 lines · Ævar Arnfjörð Bjarmason

Quoted 3 lines · Derrick Stolee

The purpose is to allow us to modify a 'struct refspec_item' andproduce a refspec string instead of munging a refspec string directly.

Ouch. I thought the goal was to take

[remote "origin"]
fetch = $src:$dst

let the code that is used in the actual fetching to parse it into the in-core "refspec_item", and then transform the refspec_item by

- discarding it if the item does not result in storing in the real fetch

- tweaking $dst side so that it won't touch anywhere outside refs/prefetch/ to avoid disturbing end-user's notion of what the latest state of the remote ref is.

so that the "parsed" refspec_item is passed to the fetch machinery without ever having to be converted back to textual form.

Why do we even need to "andproduce a refspec string"?

But yes, it seems even more straightforward to do away with passing the refspec at all to "git fetch", and instead pass some (maybe internal only, and documented as such) "--refspec-dst-prefix=refs/prefetch/" option to "git fetch".

I.e. get_ref_map() over there is already doing a version of this loop over "remote->fetch.nr".

So instead of "git gc" doing the loop, then passing all the refspecs on the command-line, it could tell "git fetch" to do the same munging when it does the same iteration.

Doing the munging in builtin/gc.c's fetch_remote() just seems like a relic from when we didn't care what decision builtin/fetch.c made about refspecs, we always wanted our custom one.

Add a new test-helper, 'test-tool refspec', that currently reads stdin line-by-line and translates the refspecs using the parsing logic of refspec_item_init() and writes them to output.

[...]

Signed-off-by Derrick Stolee <[email protected]>

---

t/t5511-refspec.sh +8 -0

@@ -93,4 +93,45 @@ 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

t/t5511-refspec.sh +7 -0

+       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

Quoted 8 lines · Derrick Stolee via GitGitGadget

Add a new test-helper, 'test-tool refspec', that currently reads stdin line-by-line and translates the refspecs using the parsing logic of refspec_item_init() and writes them to output.

[...]

Signed-off-by Derrick Stolee <[email protected]>

---

t/helper/test-refspec.c line 6

Quoted 6 lines

t/helper/test-refspec.c +6 -0

@@ -0,0 +1,39 @@
+int cmd__refspec(int argc, const char **argv)
+{
+       struct strbuf line = STRBUF_INIT;
+       [...]
+       return 0;
+}

Leaking strbuf line here. Yes, I realize that the function is returning and test-tool exiting immediately after this, so not a big deal, but it's easy to do this correctly by releasing the strbuf, thus setting good precedence for people who might use this as a template for new test-tool functions they add in the future.

Quoted 16 lines

t/t5511-refspec.sh +14 -0

@@ -93,4 +93,45 @@ 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

t/t5511-refspec.sh test_refspec fetch "refs/heads/${good}" · line 113

Quoted 4 lines

t/t5511-refspec.sh +4 -0

+               HEAD
+               @
+               :
+       EOF

Over-indented heredoc body. It is customary[1] in this codebase for the body and EOF to have the same indentation as the command which starts the heredoc.

t/helper/test-refspec.c new file +5 -0

+static const char * const refspec_usage[] = {
+	N_("test-tool refspec [--fetch]"),
+	NULL
+};
+

From: Derrick Stolee <[email protected]>

The prefetch task previously used the default refspec source plus a custom refspec destination to avoid colliding with remote refs:

	+refs/heads/*:refs/prefetch/<remote>/*

However, some users customize their refspec to reduce how much data they download from specific remotes. This can involve restrictive patterns for fetching or negative patterns to avoid downloading some refs.

Modify fetch_remote() to iterate over the remote's refspec list and translate that into the appropriate prefetch scenario. Specifically, re-parse the raw form of the refspec into a new 'struct refspec' and modify the 'dst' member to replace a leading "refs/" substring with "refs/prefetch/", or prepend "refs/prefetch/" to 'dst' otherwise. Negative refspecs do not have a 'dst' so they can be transferred to the 'git fetch' command unmodified.

This prefix change provides the benefit of keeping whatever collisions may exist in the custom refspecs, if that is a desirable outcome.

This changes the names of the refs that would be fetched by the default refspec. Instead of "refs/prefetch/<remote>/<branch>" they will now go to "refs/prefetch/remotes/<remote>/<branch>". While this is a change, it is not a seriously breaking one: these refs are intended to be hidden and not used.

Update the documentation to be more generic about the destination refs. Do not mention custom refpecs explicitly, as that does not need to be highlighted in this documentation. The important part of placing refs in refs/prefetch remains.

Reported-by Tom Saeger <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---
 Documentation/git-maintenance.txt |  3 +--
 builtin/gc.c                      | 34 +++++++++++++++++++++++-
 t/t7900-maintenance.sh            | 43 ++++++++++++++++++++++++++-----
 3 files changed, 71 insertions(+), 9 deletions(-)

Documentation/git-maintenance.txt +1 -2

index 80ddd33ceba0..95a24264eb10 100644
@@ -94,8 +94,7 @@ prefetch::
 	objects from all registered remotes. For each remote, a `git fetch`
 	command is run. The refmap is custom to avoid updating local or remote
 	branches (those in `refs/heads` or `refs/remotes`). Instead, the
-	remote refs are stored in `refs/prefetch/<remote>/`. Also, tags are
-	not updated.
+	refs are stored in `refs/prefetch/`. Also, tags are not updated.
 +
 This is done to avoid disrupting the remote-tracking branches. The end users
 expect these refs to stay unmoved unless they initiate a fetch.  With prefetch

builtin/gc.c +20 -1

index fa8128de9ae1..92cb8b4e0bfa 100644
@@ -32,6 +32,7 @@
 #include "remote.h"
 #include "object-store.h"
 #include "exec-cmd.h"
+#include "refspec.h"
 
 #define FAILED_RUN "failed to run %s"
 

@@ -877,6 +878,7 @@ static int fetch_remote(struct remote *remote, void *cbdata)
 {
 	struct maintenance_run_opts *opts = cbdata;
 	struct child_process child = CHILD_PROCESS_INIT;
+	int i;
 
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "fetch", remote->name, "--prune", "--no-tags",

@@ -886,7 +888,37 @@ static int fetch_remote(struct remote *remote, void *cbdata)
 	if (opts->quiet)
 		strvec_push(&child.args, "--quiet");
 
-	strvec_pushf(&child.args, "+refs/heads/*:refs/prefetch/%s/*", remote->name);
+	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;
+
+		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/".

builtin/gc.c +3 -0

+
+		refspec_item_clear(&replace);
+	}
 
 	return !!run_command(&child);
 }

t/t7900-maintenance.sh +37 -6

index fc2315edec11..3366ea188782 100755
@@ -142,20 +142,51 @@ 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_path_is_missing .git/refs/remotes &&
-	git log prefetch/remote1/one &&
-	git log prefetch/remote2/two &&
+	git log prefetch/remotes/remote1/one &&
+	git log prefetch/remotes/remote2/two &&
 	git fetch --all &&
-	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remote1/one &&
-	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remote2/two &&
+	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remotes/remote1/one &&
+	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remotes/remote2/two &&
 
 	test_cmp_config refs/prefetch/ log.excludedecoration &&
 	git log --oneline --decorate --all >log &&
 	! grep "prefetch" log
 '
 
+test_expect_success 'prefetch custom refspecs' '
+	git -C clone1 branch -f special/fetched HEAD &&
+	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_TRACE2_EVENT="$(pwd)/prefetch-refspec.txt" git maintenance run --task=prefetch 2>/dev/null &&
+
+	fetchargs="--prune --no-tags --no-write-fetch-head --recurse-submodules=no --refmap= --quiet" &&
+
+	# skips second refspec because it is not a pattern type
+	rs1="+refs/heads/*:refs/prefetch/remotes/remote1/*" &&
+	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 &&
+
+	# first refspec is overridden by second
+	test_must_fail git rev-parse refs/prefetch/special/fetched &&
+	git rev-parse refs/prefetch/heads/fetched &&
+
+	# possible incorrect places for the non-fetched ref
+	test_must_fail git rev-parse refs/prefetch/remotes/remote1/secret/not-fetched &&
+	test_must_fail git rev-parse refs/prefetch/remotes/remote1/not-fetched &&
+	test_must_fail git rev-parse refs/heads/secret/not-fetched &&
+	test_must_fail git rev-parse refs/heads/not-fetched
+'
+
 test_expect_success 'prefetch and existing log.excludeDecoration values' '
 	git config --unset-all log.excludeDecoration &&
 	git config log.excludeDecoration refs/remotes/remote1/ &&

Signature · 1 line

gitgitgadget

builtin/gc.c static int fetch_remote(struct remote *remote, void *cbdata) · line 918

Quoted 2 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

builtin/gc.c +1 -0

+		strvec_push(&child.args, refspec_item_format(&replace));

see comment on 3/5, think refspec_item_format is leaking here. this code looks fine though.

[...]

t/t7900-maintenance.sh +6 -6

index fc2315edec11..3366ea188782 100755
@@ -142,20 +142,51 @@ 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_path_is_missing .git/refs/remotes &&
-	git log prefetch/remote1/one &&
-	git log prefetch/remote2/two &&
+	git log prefetch/remotes/remote1/one &&
+	git log prefetch/remotes/remote2/two &&
 	git fetch --all &&
-	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remote1/one &&
-	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remote2/two &&
+	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remotes/remote1/one &&
+	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remotes/remote2/two &&
 
 	test_cmp_config refs/prefetch/ log.excludedecoration &&
 	git log --oneline --decorate --all >log &&
 	! grep "prefetch" log
 '

Quoted 29 lines · Derrick Stolee via GitGitGadget

[...]

t/t7900-maintenance.sh +6 -6

index fc2315edec11..3366ea188782 100755
@@ -142,20 +142,51 @@ 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_path_is_missing .git/refs/remotes &&
-	git log prefetch/remote1/one &&
-	git log prefetch/remote2/two &&
+	git log prefetch/remotes/remote1/one &&
+	git log prefetch/remotes/remote2/two &&
 	git fetch --all &&
-	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remote1/one &&
-	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remote2/two &&
+	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remotes/remote1/one &&
+	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remotes/remote2/two &&
 
 	test_cmp_config refs/prefetch/ log.excludedecoration &&
 	git log --oneline --decorate --all >log &&
 	! grep "prefetch" log
 '
 

t/t7900-maintenance.sh test_expect_success 'prefetch multiple remotes' ' · line 167

Quoted 9 lines

t/t7900-maintenance.sh +9 -0

+test_expect_success 'prefetch custom refspecs' '
+	git -C clone1 branch -f special/fetched HEAD &&
+	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_TRACE2_EVENT="$(pwd)/prefetch-refspec.txt" git maintenance run --task=prefetch 2>/dev/null &&

I see this is following some established convention in the file, but is there really not a way to make this pass without directing stderr to /dev/null? It makes ad-hoc debugging when reviewing harder.

I tried just removing it, but then (in an earlier test case) the "test_subcommand" fails because it can't find the line we're looking for, so us piping stderr to /dev/null impacts our trace2 output?

Quoted 6 lines · Ævar Arnfjörð Bjarmason

Review

Quoted 1 line · Derrick Stolee via GitGitGadget

(quoted diff) +1 -0

+	GIT_TRACE2_EVENT="$(pwd)/prefetch-refspec.txt" git maintenance run --task=prefetch 2>/dev/null &&

I see this is following some established convention in the file, but is there really not a way to make this pass without directing stderr to /dev/null? It makes ad-hoc debugging when reviewing harder.

As I later found out this is copy/pasted to get around the fact that --quiet is dependent on isatty(), so without this the result would be different under --verbose and non-verbose testing.

So that dates back to 3ddaad0e060 (maintenance: add --quiet option, 2020-09-17), but I see other quiet=isatty(2) in related code. I wish we could isolate that particular behavior so removing the 2>/dev/null when debugging the tests doesn't cause you to run into this, maybe an explicit --quiet or --no-quiet option for all but one test that's checking that isatty() behavior?

Quoted 3 lines

I tried just removing it, but then (in an earlier test case) the "test_subcommand" fails because it can't find the line we're looking for, so us piping stderr to /dev/null impacts our trace2 output?

I hadn't seen seen test_subcommand before, sorry to be blunt, but "ew!".

So we're ad-hoc grepping trace2 JSON output just to find out whether we invoked some subcommand. But unlike test_expect_code etc. this one doesn't run git for you, but instead we have temp *.txt files and the command disconnected from the run.

And because you're using "grep" and "! grep" to test, you're hiding the difference between "did not find this line" v.s. "did not find anything at all".

Because of that the second test using test_subcommand is either buggy or painfully non-obvious. We check that "run --auto" doesn't contain a "auto --quiet", but in reality it doesn't contain any subcommands at all. We didn't run any because it exited with "nothing to pack".

I think converting the whole thing to something like the WIP/RFC patch below is much better and more readable.

t/test-lib-functions.sh +8 -0

+test_expect_process_tree () {
+	depth= &&
+	>actual &&
+	cat >expect &&
+	cat <&3 >expect.err
+	while test $# != 0
+	do
+		case "$1" in

Quoted 10 lines · Ævar Arnfjörð Bjarmason

Quoted 6 lines · Ævar Arnfjörð Bjarmason

Review

Quoted 1 line · Derrick Stolee via GitGitGadget

(quoted diff) +1 -0

+	GIT_TRACE2_EVENT="$(pwd)/prefetch-refspec.txt" git maintenance run --task=prefetch 2>/dev/null &&

I see this is following some established convention in the file, but is there really not a way to make this pass without directing stderr to /dev/null? It makes ad-hoc debugging when reviewing harder.

Quoted 3 lines

As I later found out this is copy/pasted to get around the fact that --quiet is dependent on isatty(), so without this the result would be different under --verbose and non-verbose testing.

Yes, adding --quiet directly is a better pattern.

Quoted 7 lines

So that dates back to 3ddaad0e060 (maintenance: add --quiet option, 2020-09-17), but I see other quiet=isatty(2) in related code. I wish we could isolate that particular behavior so removing the 2>/dev/null when debugging the tests doesn't cause you to run into this, maybe an explicit --quiet or --no-quiet option for all but one test that's checking that isatty() behavior?

Quoted 5 lines

Quoted 3 lines

I tried just removing it, but then (in an earlier test case) the "test_subcommand" fails because it can't find the line we're looking for, so us piping stderr to /dev/null impacts our trace2 output?

I hadn't seen seen test_subcommand before, sorry to be blunt, but "ew!".

Saying "I'm about to be rude" before being rude doesn't excuse it. I had to step away and get over this comment before I could examine the actually constructive feedback below.

A better way to communicate the same information could be "This test helper seems more complicated than necessary, and has some gaps that could be filled." I completely agree with this statement.

Quoted 8 lines

So we're ad-hoc grepping trace2 JSON output just to find out whether we invoked some subcommand. But unlike test_expect_code etc. this one doesn't run git for you, but instead we have temp *.txt files and the command disconnected from the run.

And because you're using "grep" and "! grep" to test, you're hiding the difference between "did not find this line" v.s. "did not find anything at all".

You're right that there is value in comparing the ordered list of subcommands run by a given Git command. That will catch buggy tests that are checking that a subcommand doesn't run.

Quoted 4 lines

Because of that the second test using test_subcommand is either buggy or painfully non-obvious. We check that "run --auto" doesn't contain a "auto --quiet", but in reality it doesn't contain any subcommands at all. We didn't run any because it exited with "nothing to pack".

That exit is from the third command, which does not pass the --auto command. The "nothing to pack" output is from the 'git gc --no-quiet' subcommand that is being checked in this third test.

Quoted 2 lines

I think converting the whole thing to something like the WIP/RFC patch below is much better and more readable.

This is an interesting approach. I don't see you using the ERR that you are inputting anywhere, so that seems like an unnecessary bloat to the consumers. But maybe I haven't discovered all of the places where this would be useful, but it seems better to pipe stderr to a file for later comparison when needed.

Quoted 12 lines

t/test-lib-functions.sh +12 -0

+test_expect_process_tree () {
+	depth= &&
+	>actual &&
+	cat >expect &&
+	cat <&3 >expect.err
+	while test $# != 0
+	do
+		case "$1" in
+		--depth)
+			depth="$2"
+			shift
+			;;

t/test-lib-functions.sh test_path_is_hidden () { · line 1645

Quoted 6 lines

t/test-lib-functions.sh +6 -0

+		*)
+			break
+			;;
+		esac
+		shift
+	done &&

Do you have an example where this is being checked? Or can depth be left as 1 for now?

Quoted 8 lines

t/test-lib-functions.sh +8 -0

+	log="$(pwd)/proc-tree.txt" &&
+	>"$log" &&
+	GIT_TRACE2_PERF="$log" "$@" 2>actual.err &&
+	grep "child_start" proc-tree.txt >proc-tree-start.txt || : &&
+	if test -n "$depth"
+	then
+		grep " d$depth " proc-tree-start.txt >tmp.txt || : &&
+		mv tmp.txt proc-tree-start.txt

t/test-lib-functions.sh test_path_is_hidden () { · line 1658

Quoted 5 lines

t/test-lib-functions.sh +5 -0

+	fi &&
+	sed -e 's/^.*argv:\[//' -e 's/\]$//' <proc-tree-start.txt >actual &&
+	test_cmp expect actual &&
+	test_cmp expect.err actual.err
+} 7>&2 2>&4

I think similar ideas could apply to test_region. Giving it a try now.

Quoted 9 lines · Derrick Stolee

Quoted 2 lines · Ævar Arnfjörð Bjarmason

I think converting the whole thing to something like the WIP/RFC patch below is much better and more readable.

This is an interesting approach. I don't see you using the ERR that you are inputting anywhere, so that seems like an unnecessary bloat to the consumers. But maybe I haven't discovered all of the places where this would be useful, but it seems better to pipe stderr to a file for later comparison when needed.

Yes, it's probably not a good default here. For the test-lib.sh tests there's check_sub_test_lib_test and check_sub_test_lib_test_err, most of the tests only test stdout.

Quoted 12 lines

Quoted 12 lines

(quoted diff) +12 -0

+test_expect_process_tree () {
+	depth= &&
+	>actual &&
+	cat >expect &&
+	cat <&3 >expect.err
+	while test $# != 0
+	do
+		case "$1" in
+		--depth)
+			depth="$2"
+			shift
+			;;

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+		*)
+			break
+			;;
+		esac
+		shift
+	done &&

Do you have an example where this is being checked? Or can depth be left as 1 for now?

It can probably be hardcoded, but I was hoping someone more familiar with trace2 would chime in, but I'm fairly sure there's not a way to do it without parsing the existing output with either some clever grep/awk-ing of the PERF output, or stateful parsing of the JSON.

I thought that for git maintenance tests perhaps something wanted to assert that we didn't have maintenance invoking maintenance, or that something expected to prune refs really invoked the relevant prune command via "gc".

Quoted 8 lines

Quoted 8 lines

(quoted diff) +8 -0

+	log="$(pwd)/proc-tree.txt" &&
+	>"$log" &&
+	GIT_TRACE2_PERF="$log" "$@" 2>actual.err &&
+	grep "child_start" proc-tree.txt >proc-tree-start.txt || : &&
+	if test -n "$depth"
+	then
+		grep " d$depth " proc-tree-start.txt >tmp.txt || : &&
+		mv tmp.txt proc-tree-start.txt

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	fi &&
+	sed -e 's/^.*argv:\[//' -e 's/\]$//' <proc-tree-start.txt >actual &&
+	test_cmp expect actual &&
+	test_cmp expect.err actual.err
+} 7>&2 2>&4

I think similar ideas could apply to test_region. Giving it a try now.

Probably, I didn't even notice that one...

Quoted 65 lines · Ævar Arnfjörð Bjarmason

Quoted 9 lines · Derrick Stolee

Quoted 2 lines · Ævar Arnfjörð Bjarmason

I think converting the whole thing to something like the WIP/RFC patch below is much better and more readable.

This is an interesting approach. I don't see you using the ERR that you are inputting anywhere, so that seems like an unnecessary bloat to the consumers. But maybe I haven't discovered all of the places where this would be useful, but it seems better to pipe stderr to a file for later comparison when needed.

Yes, it's probably not a good default here. For the test-lib.sh tests there's check_sub_test_lib_test and check_sub_test_lib_test_err, most of the tests only test stdout.

Quoted 12 lines

Quoted 12 lines

(quoted diff) +12 -0

+test_expect_process_tree () {
+	depth= &&
+	>actual &&
+	cat >expect &&
+	cat <&3 >expect.err
+	while test $# != 0
+	do
+		case "$1" in
+		--depth)
+			depth="$2"
+			shift
+			;;

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+		*)
+			break
+			;;
+		esac
+		shift
+	done &&

Do you have an example where this is being checked? Or can depth be left as 1 for now?

It can probably be hardcoded, but I was hoping someone more familiar with trace2 would chime in, but I'm fairly sure there's not a way to do it without parsing the existing output with either some clever grep/awk-ing of the PERF output, or stateful parsing of the JSON.

I thought that for git maintenance tests perhaps something wanted to assert that we didn't have maintenance invoking maintenance, or that something expected to prune refs really invoked the relevant prune command via "gc".

Quoted 8 lines

Quoted 8 lines

(quoted diff) +8 -0

+	log="$(pwd)/proc-tree.txt" &&
+	>"$log" &&
+	GIT_TRACE2_PERF="$log" "$@" 2>actual.err &&
+	grep "child_start" proc-tree.txt >proc-tree-start.txt || : &&
+	if test -n "$depth"
+	then
+		grep " d$depth " proc-tree-start.txt >tmp.txt || : &&
+		mv tmp.txt proc-tree-start.txt

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	fi &&
+	sed -e 's/^.*argv:\[//' -e 's/\]$//' <proc-tree-start.txt >actual &&
+	test_cmp expect actual &&
+	test_cmp expect.err actual.err
+} 7>&2 2>&4

I think similar ideas could apply to test_region. Giving it a try now.

Quoted 1 line

Probably, I didn't even notice that one...

I gave this a few hours today, and I'm giving up. I'm the first to admit that I don't have the correct scripting skills to do some of these things.

I've got what I tried below. It certainly looks like it would work. It solves the problem of "what if the test is flaky?" by ensuring that all subcommands (at depth 0) match the inputs exactly.

However, the problem comes when trying to make that work for all of the maintenance tests, specifically the 'incremental-repack' task. That task dynamically computes a --batch-size=X parameter, and that is not stable across runs of the script.

This was avoided in the past by only checking for the first of three subcommands when verifying that the 'incremental-repack' task worked. That is, except for the EXPENSIVE test that checks that the --batch-size maxes out at 2g.

The thing that might make these changing parameters work is to allow the specified lines be a _prefix_ of the actual parameters. Or, let each line be a pattern that is checked against that line. Issues come up with how to handle this line-by-line check that I was unable to overcome.

The good news is that the idea of adding a '--prefetch' option to 'git fetch' makes the change to t7900-maintenance.sh much easier, making this change to test_subcommand less of a priority.

I include my attempt here as a patch. Feel free to take whatever you want of it, or none of it and start over. I do think that it makes the test script look much nicer.

This changes the names of the refs that would be fetched by the default refspec. Instead of "refs/prefetch/<remote>/<branch>" they will now go to "refs/prefetch/remotes/<remote>/<branch>". While this is a change, it is not a seriously breaking one: these refs are intended to be hidden and not used.

builtin/gc.c +21 -1

-	strvec_pushf(&child.args, "+refs/heads/*:refs/prefetch/%s/*", remote->name);
+	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;
+
+		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;

builtin/gc.c +2 -1

-static int fetch_remote(const char *remote, struct maintenance_run_opts *opts)
+static int fetch_remote(struct remote *remote, void *cbdata)
 {
+	struct maintenance_run_opts *opts = cbdata;

if (opts->quiet)

Overall, the series looks pretty solid to me - which is why I've got a handful of small nits to relay. :)

Quoted 4 lines · Derrick Stolee via GitGitGadget

builtin/gc.c +2 -1

-static int fetch_remote(const char *remote, struct maintenance_run_opts *opts)
+static int fetch_remote(struct remote *remote, void *cbdata)
 {
+	struct maintenance_run_opts *opts = cbdata;

[snip]

Quoted 1 line

if (opts->quiet)

I worry about the lack of null-checking here.

From: Derrick Stolee <[email protected]>

Add a new test-helper, 'test-tool refspec', that currently reads stdin line-by-line and translates the refspecs using the parsing logic of refspec_item_init() and writes them to output.

Create a test in t5511-refspec.sh that uses this helper to test several known special cases. This includes all of the special properties of the 'struct refspec_item', including:

* force: The refspec starts with '+'.
* pattern: Each side of the refspec has a glob character ('*')
* matching: The refspec is simply the string ":".
* exact_sha1: The 'src' string is a 40-character hex string.
* negative: The refspec starts with '^' and 'dst' is NULL.

While the exact_sha1 property doesn't require special logic in refspec_item_format, it is still tested here for completeness.

There is also the special-case refspec "@" which translates to "HEAD".

Note that if a refspec does not start with "refs/", then that is not incorporated as part of the 'struct refspec_item'. This behavior is confirmed by these tests. These refspecs still work in the wild because the refs layer interprets them appropriately as branches, prepending "refs/" or "refs/heads/" as necessary. I spent some time attempting to insert these prefixes explicitly in parse_refspec(), but these are several subtleties I was unable to overcome. If such a change were to be made, then this new test in t5511-refspec.sh will need to be updated with new output. For example, the input lines ending with "translated" are designed to demonstrate these subtleties.

Signed-off-by Derrick Stolee <[email protected]>

---
 Makefile                |  1 +
 t/helper/test-refspec.c | 44 +++++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c    |  1 +
 t/helper/test-tool.h    |  1 +
 t/t5511-refspec.sh      | 41 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 88 insertions(+)
 create mode 100644 t/helper/test-refspec.c

Makefile +1 -0

index a6a73c574191..f858c9f25976 100644
@@ -734,6 +734,7 @@ TEST_BUILTINS_OBJS += test-reach.o
 TEST_BUILTINS_OBJS += test-read-cache.o
 TEST_BUILTINS_OBJS += test-read-graph.o
 TEST_BUILTINS_OBJS += test-read-midx.o
+TEST_BUILTINS_OBJS += test-refspec.o
 TEST_BUILTINS_OBJS += test-ref-store.o
 TEST_BUILTINS_OBJS += test-regex.o
 TEST_BUILTINS_OBJS += test-repository.o

t/helper/test-refspec.c new file +9 -0

new file mode 100644
index 000000000000..b06735ded208
@@ -0,0 +1,44 @@
+#include "cache.h"
+#include "parse-options.h"
+#include "refspec.h"
+#include "strbuf.h"
+#include "test-tool.h"
+
+static const char * const refspec_usage[] = {
+	N_("test-tool refspec [--fetch]"),
+	NULL

t/helper/test-refspec.c new file +25 -0

+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, NULL, refspec_options,
+			     refspec_usage, 0);
+
+	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;
+		}
+
+		buf = refspec_item_format(&rsi);
+		printf("%s\n", buf);
+		free(buf);
+
+		refspec_item_clear(&rsi);
+	}
+
+	strbuf_release(&line);
+	return 0;
+}

t/helper/test-tool.c +1 -0

index 287aa6002307..f534ad1731a9 100644
@@ -55,6 +55,7 @@ static struct test_cmd cmds[] = {
 	{ "read-cache", cmd__read_cache },
 	{ "read-graph", cmd__read_graph },
 	{ "read-midx", cmd__read_midx },
+	{ "refspec", cmd__refspec },
 	{ "ref-store", cmd__ref_store },
 	{ "regex", cmd__regex },
 	{ "repository", cmd__repository },

t/helper/test-tool.h +1 -0

index 9ea4b31011dd..46a0b8850f17 100644
@@ -44,6 +44,7 @@ int cmd__reach(int argc, const char **argv);
 int cmd__read_cache(int argc, const char **argv);
 int cmd__read_graph(int argc, const char **argv);
 int cmd__read_midx(int argc, const char **argv);
+int cmd__refspec(int argc, const char **argv);
 int cmd__ref_store(int argc, const char **argv);
 int cmd__regex(int argc, const char **argv);
 int cmd__repository(int argc, const char **argv);

t/t5511-refspec.sh +41 -0

index be025b90f989..489bec08d570 100755
@@ -93,4 +93,45 @@ test_refspec fetch "refs/heads/${good}"
 bad=$(printf '\011tab')
 test_refspec fetch "refs/heads/${bad}"				invalid
 
+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
+	@
+	:
+	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
+	:
+	EOF
+	test-tool refspec <input >output &&
+	test_cmp expect output &&
+	test-tool refspec --fetch <input >output &&
+	test_cmp expect output
+'
+
 test_done

Signature · 1 line

gitgitgadget

t/t5511-refspec.sh +25 -0

index be025b90f989..489bec08d570 100755
@@ -93,4 +93,45 @@ test_refspec fetch "refs/heads/${good}"
 bad=$(printf '\011tab')
 test_refspec fetch "refs/heads/${bad}"				invalid
 
+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
+	@
+	:
+	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

From: Derrick Stolee <[email protected]>

The prefetch task previously used the default refspec source plus a custom refspec destination to avoid colliding with remote refs:

	+refs/heads/*:refs/prefetch/<remote>/*

However, some users customize their refspec to reduce how much data they download from specific remotes. This can involve restrictive patterns for fetching or negative patterns to avoid downloading some refs.

Modify fetch_remote() to iterate over the remote's refspec list and translate that into the appropriate prefetch scenario. Specifically, re-parse the raw form of the refspec into a new 'struct refspec' and modify the 'dst' member to replace a leading "refs/" substring with "refs/prefetch/", or prepend "refs/prefetch/" to 'dst' otherwise. Negative refspecs do not have a 'dst' so they can be transferred to the 'git fetch' command unmodified.

This prefix change provides the benefit of keeping whatever collisions may exist in the custom refspecs, if that is a desirable outcome.

This changes the names of the refs that would be fetched by the default refspec. Instead of "refs/prefetch/<remote>/<branch>" they will now go to "refs/prefetch/remotes/<remote>/<branch>". While this is a change, it is not a seriously breaking one: these refs are intended to be hidden and not used.

Update the documentation to be more generic about the destination refs. Do not mention custom refpecs explicitly, as that does not need to be highlighted in this documentation. The important part of placing refs in refs/prefetch remains.

Reported-by Tom Saeger <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---
 Documentation/git-maintenance.txt |  3 +--
 builtin/gc.c                      | 37 +++++++++++++++++++++++++-
 t/t7900-maintenance.sh            | 43 ++++++++++++++++++++++++++-----
 3 files changed, 74 insertions(+), 9 deletions(-)

Documentation/git-maintenance.txt +1 -2

index 80ddd33ceba0..95a24264eb10 100644
@@ -94,8 +94,7 @@ prefetch::
 	objects from all registered remotes. For each remote, a `git fetch`
 	command is run. The refmap is custom to avoid updating local or remote
 	branches (those in `refs/heads` or `refs/remotes`). Instead, the
-	remote refs are stored in `refs/prefetch/<remote>/`. Also, tags are
-	not updated.
+	refs are stored in `refs/prefetch/`. Also, tags are not updated.
 +
 This is done to avoid disrupting the remote-tracking branches. The end users
 expect these refs to stay unmoved unless they initiate a fetch.  With prefetch

builtin/gc.c +29 -1

index fa8128de9ae1..76f347dd6b11 100644
@@ -32,6 +32,7 @@
 #include "remote.h"
 #include "object-store.h"
 #include "exec-cmd.h"
+#include "refspec.h"
 
 #define FAILED_RUN "failed to run %s"
 

@@ -877,6 +878,7 @@ static int fetch_remote(struct remote *remote, void *cbdata)
 {
 	struct maintenance_run_opts *opts = cbdata;
 	struct child_process child = CHILD_PROCESS_INIT;
+	int i;
 
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "fetch", remote->name, "--prune", "--no-tags",

@@ -886,7 +888,40 @@ static int fetch_remote(struct remote *remote, void *cbdata)
 	if (opts->quiet)
 		strvec_push(&child.args, "--quiet");
 
-	strvec_pushf(&child.args, "+refs/heads/*:refs/prefetch/%s/*", remote->name);
+	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;
+		char *replace_string;
+
+		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);

Junio brought up the point about configs which 'fetch' have no dst
https://lore.kernel.org/git/[email protected]/

[remote "submaintainer1"]
url = ... repository of submaintainer #1 ... fetch = master
tagopt = --no-tags

This patch fixes segfault for config like above.
You might have ideas on a cleaner way to do this.
I did add child_process_clear.

From: Derrick Stolee <[email protected]>

The prefetch task previously used the default refspec source plus a custom refspec destination to avoid colliding with remote refs:

	+refs/heads/*:refs/prefetch/<remote>/*

However, some users customize their refspec to reduce how much data they download from specific remotes. This can involve restrictive patterns for fetching or negative patterns to avoid downloading some refs.

Modify fetch_remote() to iterate over the remote's refspec list and translate that into the appropriate prefetch scenario. Specifically, re-parse the raw form of the refspec into a new 'struct refspec' and modify the 'dst' member to replace a leading "refs/" substring with "refs/prefetch/", or prepend "refs/prefetch/" to 'dst' otherwise. Negative refspecs do not have a 'dst' so they can be transferred to the 'git fetch' command unmodified.

This prefix change provides the benefit of keeping whatever collisions may exist in the custom refspecs, if that is a desirable outcome.

This changes the names of the refs that would be fetched by the default refspec. Instead of "refs/prefetch/<remote>/<branch>" they will now go to "refs/prefetch/remotes/<remote>/<branch>". While this is a change, it is not a seriously breaking one: these refs are intended to be hidden and not used.

Update the documentation to be more generic about the destination refs. Do not mention custom refpecs explicitly, as that does not need to be

highlighted in this documentation. The important part of placing refs in refs/prefetch remains.

Reported-by Tom Saeger <[email protected]>

Signed-off-by Derrick Stolee <[email protected]>

---
 Documentation/git-maintenance.txt |  3 +--
 builtin/gc.c                      | 37 +++++++++++++++++++++++++-
 t/t7900-maintenance.sh            | 43 ++++++++++++++++++++++++++-----
 3 files changed, 74 insertions(+), 9 deletions(-)

Documentation/git-maintenance.txt +1 -2

index 80ddd33ceba0..95a24264eb10 100644
@@ -94,8 +94,7 @@ prefetch::
 	objects from all registered remotes. For each remote, a `git fetch`
 	command is run. The refmap is custom to avoid updating local or remote
 	branches (those in `refs/heads` or `refs/remotes`). Instead, the
-	remote refs are stored in `refs/prefetch/<remote>/`. Also, tags are
-	not updated.
+	refs are stored in `refs/prefetch/`. Also, tags are not updated.
 +
 This is done to avoid disrupting the remote-tracking branches. The end users
 expect these refs to stay unmoved unless they initiate a fetch.  With prefetch

builtin/gc.c +36 -1

index fa8128de9ae1..76f347dd6b11 100644
@@ -32,6 +32,7 @@
 #include "remote.h"
 #include "object-store.h"
 #include "exec-cmd.h"
+#include "refspec.h"
 
 #define FAILED_RUN "failed to run %s"
 

@@ -877,6 +878,7 @@ static int fetch_remote(struct remote *remote, void *cbdata)
 {
 	struct maintenance_run_opts *opts = cbdata;
 	struct child_process child = CHILD_PROCESS_INIT;
+	int i;
 
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "fetch", remote->name, "--prune", "--no-tags",

@@ -886,7 +888,40 @@ static int fetch_remote(struct remote *remote, void *cbdata)
 	if (opts->quiet)
 		strvec_push(&child.args, "--quiet");
 
-	strvec_pushf(&child.args, "+refs/heads/*:refs/prefetch/%s/*", remote->name);
+	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;
+		char *replace_string;
+
+		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);
+
+		replace_string = refspec_item_format(&replace);
+		strvec_push(&child.args, replace_string);
+		free(replace_string);
+
+		refspec_item_clear(&replace);
+	}
 
 	return !!run_command(&child);
 }

t/t7900-maintenance.sh +37 -6

index 37eed6ed3aa3..03487be3af38 100755
@@ -142,20 +142,51 @@ 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_path_is_missing .git/refs/remotes &&
-	git log prefetch/remote1/one &&
-	git log prefetch/remote2/two &&
+	git log prefetch/remotes/remote1/one &&
+	git log prefetch/remotes/remote2/two &&
 	git fetch --all &&
-	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remote1/one &&
-	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remote2/two &&
+	test_cmp_rev refs/remotes/remote1/one refs/prefetch/remotes/remote1/one &&
+	test_cmp_rev refs/remotes/remote2/two refs/prefetch/remotes/remote2/two &&
 
 	test_cmp_config refs/prefetch/ log.excludedecoration &&
 	git log --oneline --decorate --all >log &&
 	! grep "prefetch" log
 '
 
+test_expect_success 'prefetch custom refspecs' '
+	git -C clone1 branch -f special/fetched HEAD &&
+	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_TRACE2_EVENT="$(pwd)/prefetch-refspec.txt" git maintenance run --task=prefetch 2>/dev/null &&
+
+	fetchargs="--prune --no-tags --no-write-fetch-head --recurse-submodules=no --refmap= --quiet" &&
+
+	# skips second refspec because it is not a pattern type
+	rs1="+refs/heads/*:refs/prefetch/remotes/remote1/*" &&
+	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 &&
+
+	# first refspec is overridden by second
+	test_must_fail git rev-parse refs/prefetch/special/fetched &&
+	git rev-parse refs/prefetch/heads/fetched &&
+
+	# possible incorrect places for the non-fetched ref
+	test_must_fail git rev-parse refs/prefetch/remotes/remote1/secret/not-fetched &&
+	test_must_fail git rev-parse refs/prefetch/remotes/remote1/not-fetched &&
+	test_must_fail git rev-parse refs/heads/secret/not-fetched &&
+	test_must_fail git rev-parse refs/heads/not-fetched
+'
+
 test_expect_success 'prefetch and existing log.excludeDecoration values' '
 	git config --unset-all log.excludeDecoration &&
 	git config log.excludeDecoration refs/remotes/remote1/ &&

Signature · 1 line

gitgitgadget

(quoted diff) +0 -0

@@ -877,6 +878,7 @@ static int fetch_remote(struct remote *remote, void *cbdata)

From: Derrick Stolee <[email protected]>

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 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().

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/gc.c | 33 ++++++++-------------------------
 1 file changed, 8 insertions(+), 25 deletions(-)

builtin/gc.c +0 -0

index ef7226d7bca4..fa8128de9ae1 100644

builtin/gc.c +6 -25

 	child.git_cmd = 1;
-	strvec_pushl(&child.args, "fetch", remote, "--prune", "--no-tags",
+	strvec_pushl(&child.args, "fetch", remote->name, "--prune", "--no-tags",
 		     "--no-write-fetch-head", "--recurse-submodules=no",
 		     "--refmap=", NULL);
 
 	if (opts->quiet)
 		strvec_push(&child.args, "--quiet");
 
-	strvec_pushf(&child.args, "+refs/heads/*:refs/prefetch/%s/*", remote);
+	strvec_pushf(&child.args, "+refs/heads/*:refs/prefetch/%s/*", remote->name);
 
 	return !!run_command(&child);
 }
 
-static int append_remote(struct remote *remote, void *cbdata)
-{
-	struct string_list *remotes = (struct string_list *)cbdata;
-
-	string_list_append(remotes, remote->name);
-	return 0;
-}
-
 static int maintenance_task_prefetch(struct maintenance_run_opts *opts)
 {
-	int result = 0;
-	struct string_list_item *item;
-	struct string_list remotes = STRING_LIST_INIT_DUP;
-
 	git_config_set_multivar_gently("log.excludedecoration",
 					"refs/prefetch/",
 					"refs/prefetch/",
 					CONFIG_FLAGS_FIXED_VALUE |
 					CONFIG_FLAGS_MULTI_REPLACE);
 
-	if (for_each_remote(append_remote, &remotes)) {
-		error(_("failed to fill remotes"));
-		result = 1;
-		goto cleanup;
+	if (for_each_remote(fetch_remote, opts)) {
+		error(_("failed to prefetch remotes"));
+		return 1;
 	}
 
-	for_each_string_list_item(item, &remotes)
-		result |= fetch_remote(item->string, opts);
-
-cleanup:
-	string_list_clear(&remotes, 0);
-	return result;
+	return 0;
 }
 
 static int maintenance_task_gc(struct maintenance_run_opts *opts)
-- 

gitgitgadget

builtin/gc.c static int maintenance_task_commit_graph(struct maintenance_run_opts *opts) · line 879

Quoted 5 lines · Derrick Stolee via GitGitGadget

builtin/gc.c +2 -1

-static int fetch_remote(const char *remote, struct maintenance_run_opts *opts)
+static int fetch_remote(struct remote *remote, void *cbdata)
 {
+	struct maintenance_run_opts *opts = cbdata;
 	struct child_process child = CHILD_PROCESS_INIT;

I think this might be appropriate to add:

       if (remote->skip_default_update)
               return 0;

maintenance prefetch is acting like git fetch --all So it should also skip remotes with configs skipfetchall = true Agree?

From: Derrick Stolee <[email protected]>

The --prefetch option will be used by the 'prefetch' maintenance task instead of sending refspecs explicitly across the command-line. The intention is to modify the refspec to place all results in refs/prefetch/ instead of anywhere else.

Create helper method filter_prefetch_refspec() to modify a given refspec to fit the rules expected of the prefetch task:

* Negative refspecs are preserved.
* Refspecs without a destination are removed.
* Refspecs whose source starts with "refs/tags/" are removed.
* Other refspecs are placed within "refs/prefetch/".

Finally, we add the 'force' option to ensure that prefetch refs are replaced as necessary.

Signed-off-by Derrick Stolee <[email protected]>

---
 Documentation/fetch-options.txt   |  5 +++
 builtin/fetch.c                   | 56 +++++++++++++++++++++++++++++++
 t/t5582-fetch-negative-refspec.sh | 30 +++++++++++++++++
 3 files changed, 91 insertions(+)

Documentation/fetch-options.txt +5 -0

index 07783deee309..9e7b4e189ce0 100644
@@ -110,6 +110,11 @@ ifndef::git-pull[]
 	setting `fetch.writeCommitGraph`.
 endif::git-pull[]
 
+--prefetch::
+	Modify the configured refspec to place all refs into the
+	`refs/prefetch/` namespace. See the `prefetch` task in
+	linkgit:git-maintenance[1].
+
 -p::
 --prune::
 	Before fetching, remove any remote-tracking references that no

builtin/fetch.c +20 -0

index 0b90de87c7a2..30856b442b79 100644
@@ -48,6 +48,7 @@ enum {
 static int fetch_prune_config = -1; /* unspecified */
 static int fetch_show_forced_updates = 1;
 static uint64_t forced_updates_ms = 0;
+static int prefetch = 0;
 static int prune = -1; /* unspecified */
 #define PRUNE_BY_DEFAULT 0 /* do we prune by default? */
 

@@ -158,6 +159,8 @@ static struct option builtin_fetch_options[] = {
 		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
 	OPT_INTEGER('j', "jobs", &max_jobs,
 		    N_("number of submodules fetched in parallel")),
+	OPT_BOOL(0, "prefetch", &prefetch,
+		 N_("modify the refspec to place all refs within refs/prefetch/")),
 	OPT_BOOL('p', "prune", &prune,
 		 N_("prune remote-tracking branches no longer on remote")),
 	OPT_BOOL('P', "prune-tags", &prune_tags,

@@ -436,6 +439,55 @@ static void find_non_local_tags(const struct ref *refs,
 	oidset_clear(&fetch_oids);
 }
 
+static void filter_prefetch_refspec(struct refspec *rs)
+{
+	int i;
+
+	if (!prefetch)
+		return;
+
+	for (i = 0; i < rs->nr; i++) {
+		struct strbuf new_dst = STRBUF_INIT;
+		char *old_dst;
+		const char *sub = NULL;
+
+		if (rs->items[i].negative)
+			continue;
+		if (!rs->items[i].dst ||
+		    (rs->items[i].src &&
+		     !strncmp(rs->items[i].src, "refs/tags/", 10))) {

builtin/fetch.c +26 -0

+			continue;
+		}
+
+		old_dst = rs->items[i].dst;
+		strbuf_addstr(&new_dst, "refs/prefetch/");
+
+		/*
+		 * If old_dst starts with "refs/", then place
+		 * sub after that prefix. Otherwise, start at
+		 * the beginning of the string.
+		 */
+		if (!skip_prefix(old_dst, "refs/", &sub))
+			sub = old_dst;
+		strbuf_addstr(&new_dst, sub);
+
+		rs->items[i].dst = strbuf_detach(&new_dst, NULL);
+		rs->items[i].force = 1;
+
+		free(old_dst);
+	}
+}
+
 static struct ref *get_ref_map(struct remote *remote,
 			       const struct ref *remote_refs,
 			       struct refspec *rs,

@@ -452,6 +504,10 @@ static struct ref *get_ref_map(struct remote *remote,
 	struct hashmap existing_refs;
 	int existing_refs_populated = 0;
 
+	filter_prefetch_refspec(rs);
+	if (remote)
+		filter_prefetch_refspec(&remote->fetch);
+
 	if (rs->nr) {
 		struct refspec *fetch_refspec;
 

t/t5582-fetch-negative-refspec.sh +30 -0

index f34509727702..030e6f978c4e 100755
@@ -240,4 +240,34 @@ test_expect_success "push with matching +: and negative refspec" '
 	git -C two push -v one
 '
 
+test_expect_success '--prefetch correctly modifies refspecs' '
+	git -C one config --unset-all remote.origin.fetch &&
+	git -C one config --add remote.origin.fetch "refs/tags/*:refs/tags/*" &&
+	git -C one config --add remote.origin.fetch ^refs/heads/bogus/ignore &&
+	git -C one config --add remote.origin.fetch "refs/heads/bogus/*:bogus/*" &&
+
+	git tag -a -m never never-fetch-tag HEAD &&
+
+	git branch bogus/fetched HEAD~1 &&
+	git branch bogus/ignore HEAD &&
+
+	git -C one fetch --prefetch --no-tags &&
+	test_must_fail git -C one rev-parse never-fetch-tag &&
+	git -C one rev-parse refs/prefetch/bogus/fetched &&
+	test_must_fail git -C one rev-parse refs/prefetch/bogus/ignore &&
+
+	# correctly handle when refspec set becomes empty
+	# after removing the refs/tags/* refspec.
+	git -C one config --unset-all remote.origin.fetch &&
+	git -C one config --add remote.origin.fetch "refs/tags/*:refs/tags/*" &&
+
+	git -C one fetch --prefetch --no-tags &&
+	test_must_fail git -C one rev-parse never-fetch-tag &&
+
+	# The refspec for refs that are not fully qualified
+	# are filtered multiple times.
+	git -C one rev-parse refs/prefetch/bogus/fetched &&
+	test_must_fail git -C one rev-parse refs/prefetch/bogus/ignore
+'
+
 test_done

Quoted 11 lines

From: Derrick Stolee <[email protected]>

builtin/fetch.c +10 -0

+	for (i = 0; i < rs->nr; i++) {
+		struct strbuf new_dst = STRBUF_INIT;
+		char *old_dst;
+		const char *sub = NULL;
+
+		if (rs->items[i].negative)
+			continue;
+		if (!rs->items[i].dst ||
+		    (rs->items[i].src &&
+		     !strncmp(rs->items[i].src, "refs/tags/", 10))) {

builtin/fetch.c static void find_non_local_tags(const struct ref *refs, · line 468

Quoted 10 lines

builtin/fetch.c +10 -0

+			int j;
+
+			free(rs->items[i].src);
+			free(rs->items[i].dst);
+
+			for (j = i + 1; j < rs->nr; j++) {
+				rs->items[j - 1] = rs->items[j];
+				rs->raw[j - 1] = rs->raw[j];
+			}
+			rs->nr--;

Hmm, don't you need to do 'i--;' here?

(Sorry in advance if this is nonsense, I am just skimming the patches without reading the whole series carefully).

Maybe try a test which has an entry, which requires the 'prefetch' modification, that immediately follows a 'tag' or 'empty dst' entry. (I can't quite tell, just reading the email, whether that is covered by the tests below - so please just ignore me if it already works ;)

* The fix is almost completely rewritten as an update to 'git fetch'. See the new PATCH 2 for this update.

Quoted 2 lines · Derrick Stolee via GitGitGadget

* The fix is almost completely rewritten as an update to 'git fetch'. See the new PATCH 2 for this update.

I do agree that it gives us the most flexibility there with nice encapsulation. Nobody other than "git fetch" needs to know how it computes which remote refs are fetched given the real pathspec, and the only thing the caller with "--prefetch" is interested in is that the prefetch operation would not contaminate the remote-tracking refs.

Great idea. I wish I were the one who thought of it first ;-)

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · Derrick Stolee via GitGitGadget

* The fix is almost completely rewritten as an update to 'git fetch'. See the new PATCH 2 for this update.

Quoted 8 lines

I do agree that it gives us the most flexibility there with nice encapsulation. Nobody other than "git fetch" needs to know how it computes which remote refs are fetched given the real pathspec, and the only thing the caller with "--prefetch" is interested in is that the prefetch operation would not contaminate the remote-tracking refs.

Great idea. I wish I were the one who thought of it first ;-)

Yes - this simplifies things greatly!

I do have one case that fails prefetch though.
It's a case where all the remote's fetch configs are filtered out.

Example:

[remote "pr-924"]
url = https://github.com/gitgitgadget/git
fetch = +refs/tags/pr-924/derrickstolee/maintenance/refspec-v3 skipfetchall = true
tagopt = --no-tags