* [PATCH v2 1/5] maintenance: simplify prefetch logic
2021-04-06 18:47 ` [PATCH v2 0/5] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
@ 2021-04-06 18:47 ` Derrick Stolee via GitGitGadget
2021-04-07 23:23 ` Emily Shaffer
2021-04-06 18:47 ` [PATCH v2 2/5] test-lib: use exact match for test_subcommand Derrick Stolee via GitGitGadget
` (4 subsequent siblings)
5 siblings, 1 reply; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-06 18:47 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
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 <dstolee@microsoft.com>
---
builtin/gc.c | 33 ++++++++-------------------------
1 file changed, 8 insertions(+), 25 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index ef7226d7bca4..fa8128de9ae1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -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)
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v2 1/5] maintenance: simplify prefetch logic
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
0 siblings, 1 reply; 72+ messages in thread
From: Emily Shaffer @ 2021-04-07 23:23 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, tom.saeger, gitster, sunshine, Derrick Stolee,
Derrick Stolee, Derrick Stolee
Overall, the series looks pretty solid to me - which is why I've got a
handful of small nits to relay. :)
On Tue, Apr 06, 2021 at 06:47:46PM +0000, Derrick Stolee via GitGitGadget wrote:
> -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]
> if (opts->quiet)
I worry about the lack of null-checking here.
- Emily
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v2 1/5] maintenance: simplify prefetch logic
2021-04-07 23:23 ` Emily Shaffer
@ 2021-04-09 19:00 ` Derrick Stolee
0 siblings, 0 replies; 72+ messages in thread
From: Derrick Stolee @ 2021-04-09 19:00 UTC (permalink / raw)
To: Emily Shaffer, Derrick Stolee via GitGitGadget
Cc: git, tom.saeger, gitster, sunshine, Derrick Stolee, Derrick Stolee
On 4/7/2021 7:23 PM, Emily Shaffer wrote:
> Overall, the series looks pretty solid to me - which is why I've got a
> handful of small nits to relay. :)
>
> On Tue, Apr 06, 2021 at 06:47:46PM +0000, Derrick Stolee via GitGitGadget wrote:
>> -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]
>> if (opts->quiet)
> I worry about the lack of null-checking here.
If this was a general-purpose method, I'd agree with you.
But since this is a static method being called exactly once
(and always passing a non-NULL value) then I don't believe
that NULL check is valuable here.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v2 2/5] test-lib: use exact match for test_subcommand
2021-04-06 18:47 ` [PATCH v2 0/5] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
2021-04-06 18:47 ` [PATCH v2 1/5] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
@ 2021-04-06 18:47 ` Derrick Stolee via GitGitGadget
2021-04-06 18:47 ` [PATCH v2 3/5] refspec: output a refspec item Derrick Stolee via GitGitGadget
` (3 subsequent siblings)
5 siblings, 0 replies; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-06 18:47 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
The use of 'grep' inside test_subcommand uses general patterns, leading
to sometimes needing escape characters to avoid incorrect matches.
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.
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.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
t/t7900-maintenance.sh | 4 ++--
t/test-lib-functions.sh | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 2412d8c5c006..37eed6ed3aa3 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -142,8 +142,8 @@ 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/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 &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6348e8d7339c..a5915dec22df 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1652,9 +1652,9 @@ test_subcommand () {
if test -n "$negate"
then
- ! grep "\[$expr\]"
+ ! grep -F "[$expr]"
else
- grep "\[$expr\]"
+ grep -F "[$expr]"
fi
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH v2 3/5] refspec: output a refspec item
2021-04-06 18:47 ` [PATCH v2 0/5] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
2021-04-06 18:47 ` [PATCH v2 1/5] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
2021-04-06 18:47 ` [PATCH v2 2/5] test-lib: use exact match for test_subcommand Derrick Stolee via GitGitGadget
@ 2021-04-06 18:47 ` Derrick Stolee via GitGitGadget
2021-04-06 18:47 ` [PATCH v2 4/5] test-tool: test refspec input/output Derrick Stolee via GitGitGadget
` (2 subsequent siblings)
5 siblings, 0 replies; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-06 18:47 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
Add a new method, refspec_item_format(), that takes a 'struct
refspec_item' pointer as input and returns a string for how that refspec
item should be written to Git's config or a subcommand, such as 'git
fetch'.
There are several subtleties regarding special-case refspecs that can
occur and are represented in t5511-refspec.sh. These cases will be
explored in new tests in the following change. It requires adding a new
test helper in order to test this format directly, so that is saved for
a separate change to keep this one focused on the logic of the format
method.
A future change will consume this method when translating refspecs in
the 'prefetch' task of the 'git maintenance' builtin.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
refspec.c | 23 +++++++++++++++++++++++
refspec.h | 2 ++
2 files changed, 25 insertions(+)
diff --git a/refspec.c b/refspec.c
index e3d852c0bfec..e79cde3c58be 100644
--- a/refspec.c
+++ b/refspec.c
@@ -180,6 +180,29 @@ void refspec_item_clear(struct refspec_item *item)
item->exact_sha1 = 0;
}
+char *refspec_item_format(const struct refspec_item *rsi)
+{
+ struct strbuf buf = STRBUF_INIT;
+
+ if (rsi->matching)
+ return xstrdup(":");
+
+ if (rsi->negative)
+ strbuf_addch(&buf, '^');
+ else if (rsi->force)
+ strbuf_addch(&buf, '+');
+
+ if (rsi->src)
+ strbuf_addstr(&buf, rsi->src);
+
+ if (rsi->dst) {
+ strbuf_addch(&buf, ':');
+ strbuf_addstr(&buf, rsi->dst);
+ }
+
+ return strbuf_detach(&buf, NULL);
+}
+
void refspec_init(struct refspec *rs, int fetch)
{
memset(rs, 0, sizeof(*rs));
diff --git a/refspec.h b/refspec.h
index 8b79891d3218..9f2ddc7949a1 100644
--- a/refspec.h
+++ b/refspec.h
@@ -56,6 +56,8 @@ 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);
+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)))
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH v2 4/5] test-tool: test refspec input/output
2021-04-06 18:47 ` [PATCH v2 0/5] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
` (2 preceding siblings ...)
2021-04-06 18:47 ` [PATCH v2 3/5] refspec: output a refspec item Derrick Stolee via GitGitGadget
@ 2021-04-06 18:47 ` 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-10 2:03 ` [PATCH v3 0/3] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
5 siblings, 2 replies; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-06 18:47 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
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 <dstolee@microsoft.com>
---
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
diff --git a/Makefile b/Makefile
index a6a73c574191..f858c9f25976 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/t/helper/test-refspec.c b/t/helper/test-refspec.c
new file mode 100644
index 000000000000..b06735ded208
--- /dev/null
+++ b/t/helper/test-refspec.c
@@ -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
+};
+
+int cmd__refspec(int argc, const char **argv)
+{
+ struct strbuf line = STRBUF_INIT;
+ int fetch = 0;
+
+ struct option refspec_options [] = {
+ OPT_BOOL(0, "fetch", &fetch,
+ N_("enable the 'fetch' option for parsing refpecs")),
+ 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;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 287aa6002307..f534ad1731a9 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -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 },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 9ea4b31011dd..46a0b8850f17 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -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);
diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
index be025b90f989..489bec08d570 100755
--- a/t/t5511-refspec.sh
+++ b/t/t5511-refspec.sh
@@ -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
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v2 4/5] test-tool: test refspec input/output
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
1 sibling, 0 replies; 72+ messages in thread
From: Josh Steadmon @ 2021-04-07 23:08 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, tom.saeger, gitster, sunshine, Derrick Stolee,
Derrick Stolee, Derrick Stolee
On 2021.04.06 18:47, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
> 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
>
> diff --git a/Makefile b/Makefile
> index a6a73c574191..f858c9f25976 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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
> diff --git a/t/helper/test-refspec.c b/t/helper/test-refspec.c
> new file mode 100644
> index 000000000000..b06735ded208
> --- /dev/null
> +++ b/t/helper/test-refspec.c
> @@ -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
> +};
> +
> +int cmd__refspec(int argc, const char **argv)
> +{
> + struct strbuf line = STRBUF_INIT;
> + int fetch = 0;
> +
> + struct option refspec_options [] = {
> + OPT_BOOL(0, "fetch", &fetch,
> + N_("enable the 'fetch' option for parsing refpecs")),
Typo here: s/refpecs/refspecs/
> + 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;
> +}
> diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> index 287aa6002307..f534ad1731a9 100644
> --- a/t/helper/test-tool.c
> +++ b/t/helper/test-tool.c
> @@ -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 },
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 9ea4b31011dd..46a0b8850f17 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -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);
> diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
> index be025b90f989..489bec08d570 100755
> --- a/t/t5511-refspec.sh
> +++ b/t/t5511-refspec.sh
> @@ -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
> --
> gitgitgadget
>
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v2 4/5] test-tool: test refspec input/output
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
1 sibling, 0 replies; 72+ messages in thread
From: Emily Shaffer @ 2021-04-07 23:26 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, tom.saeger, gitster, sunshine, Derrick Stolee,
Derrick Stolee, Derrick Stolee
On Tue, Apr 06, 2021 at 06:47:49PM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
> index be025b90f989..489bec08d570 100755
> --- a/t/t5511-refspec.sh
> +++ b/t/t5511-refspec.sh
> @@ -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
I don't like these expect/actual here. They are almost entirely
identical, which means that the reader either A) spends a toilsome few
minutes checking every single line to be sure they are not identical, or
B) reads the first three lines, decides they're the same, and misses the
@->HEAD special case.
Why not instead run the test once for all the lines which should be the
same before and after the parse, and again for all the lines which
should differ, to reduce burden on the reader?
- Emily
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v2 5/5] maintenance: allow custom refspecs during prefetch
2021-04-06 18:47 ` [PATCH v2 0/5] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
` (3 preceding siblings ...)
2021-04-06 18:47 ` [PATCH v2 4/5] test-tool: test refspec input/output Derrick Stolee via GitGitGadget
@ 2021-04-06 18:47 ` Derrick Stolee via GitGitGadget
2021-04-06 19:36 ` Tom Saeger
` (3 more replies)
2021-04-10 2:03 ` [PATCH v3 0/3] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
5 siblings, 4 replies; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-06 18:47 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
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 <tom.saeger@oracle.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
Documentation/git-maintenance.txt | 3 +--
builtin/gc.c | 37 +++++++++++++++++++++++++-
t/t7900-maintenance.sh | 43 ++++++++++++++++++++++++++-----
3 files changed, 74 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 80ddd33ceba0..95a24264eb10 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -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
diff --git a/builtin/gc.c b/builtin/gc.c
index fa8128de9ae1..76f347dd6b11 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -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);
}
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 37eed6ed3aa3..03487be3af38 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -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/ &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v2 5/5] maintenance: allow custom refspecs during prefetch
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
` (2 subsequent siblings)
3 siblings, 1 reply; 72+ messages in thread
From: Tom Saeger @ 2021-04-06 19:36 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, gitster, sunshine, Derrick Stolee, Derrick Stolee, Derrick Stolee
On Tue, Apr 06, 2021 at 06:47:50PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <tom.saeger@oracle.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> Documentation/git-maintenance.txt | 3 +--
> builtin/gc.c | 37 +++++++++++++++++++++++++-
> t/t7900-maintenance.sh | 43 ++++++++++++++++++++++++++-----
> 3 files changed, 74 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> index 80ddd33ceba0..95a24264eb10 100644
> --- a/Documentation/git-maintenance.txt
> +++ b/Documentation/git-maintenance.txt
> @@ -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
> diff --git a/builtin/gc.c b/builtin/gc.c
> index fa8128de9ae1..76f347dd6b11 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -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);
> }
Junio brought up the point about configs which 'fetch' have no dst
https://lore.kernel.org/git/c06a198a-2043-27a2-cab3-3471190754cc@gmail.com/
[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`.
--Tom
diff --git a/builtin/gc.c b/builtin/gc.c
index 76f347dd6b11..921266ee30a5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -879,6 +879,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;
+ int nargs;
child.git_cmd = 1;
strvec_pushl(&child.args, "fetch", remote->name, "--prune", "--no-tags",
@@ -888,6 +889,8 @@ static int fetch_remote(struct remote *remote, void *cbdata)
if (opts->quiet)
strvec_push(&child.args, "--quiet");
+ nargs = child.args.nr;
+
for (i = 0; i < remote->fetch.nr; i++) {
struct refspec_item replace;
struct refspec_item *rsi = &remote->fetch.items[i];
@@ -900,6 +903,10 @@ static int fetch_remote(struct remote *remote, void *cbdata)
continue;
}
+ if (!rsi->dst) {
+ continue;
+ }
+
refspec_item_init(&replace, remote->fetch.raw[i], 1);
/*
@@ -923,6 +930,12 @@ static int fetch_remote(struct remote *remote, void *cbdata)
refspec_item_clear(&replace);
}
+ /* skip remote if no refspecs to fetch */
+ if (child.args.nr - nargs <= 0) {
+ child_process_clear(&child);
+ return 0;
+ }
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v2 5/5] maintenance: allow custom refspecs during prefetch
2021-04-06 19:36 ` Tom Saeger
@ 2021-04-06 19:45 ` Derrick Stolee
0 siblings, 0 replies; 72+ messages in thread
From: Derrick Stolee @ 2021-04-06 19:45 UTC (permalink / raw)
To: Tom Saeger, Derrick Stolee via GitGitGadget
Cc: git, gitster, sunshine, Derrick Stolee, Derrick Stolee
On 4/6/2021 3:36 PM, Tom Saeger wrote:
>
> Junio brought up the point about configs which 'fetch' have no dst
> https://lore.kernel.org/git/c06a198a-2043-27a2-cab3-3471190754cc@gmail.com/
Thank you for reminding me about this. It was on the other thread,
and I forgot to go back to it as I was preparing this version.
> [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`.
I will also add a test to ensure this scenario does not regress.
-Stolee
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v2 5/5] maintenance: allow custom refspecs during prefetch
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-07 23:09 ` Josh Steadmon
2021-04-07 23:37 ` Emily Shaffer
2021-04-08 0:23 ` Jonathan Tan
3 siblings, 0 replies; 72+ messages in thread
From: Josh Steadmon @ 2021-04-07 23:09 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, tom.saeger, gitster, sunshine, Derrick Stolee,
Derrick Stolee, Derrick Stolee
On 2021.04.06 18:47, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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
Typo here: s/refpecs/refspecs/
> highlighted in this documentation. The important part of placing refs in
> refs/prefetch remains.
>
> Reported-by: Tom Saeger <tom.saeger@oracle.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> Documentation/git-maintenance.txt | 3 +--
> builtin/gc.c | 37 +++++++++++++++++++++++++-
> t/t7900-maintenance.sh | 43 ++++++++++++++++++++++++++-----
> 3 files changed, 74 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
> index 80ddd33ceba0..95a24264eb10 100644
> --- a/Documentation/git-maintenance.txt
> +++ b/Documentation/git-maintenance.txt
> @@ -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
> diff --git a/builtin/gc.c b/builtin/gc.c
> index fa8128de9ae1..76f347dd6b11 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -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);
> }
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 37eed6ed3aa3..03487be3af38 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -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/ &&
> --
> gitgitgadget
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v2 5/5] maintenance: allow custom refspecs during prefetch
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-07 23:09 ` Josh Steadmon
@ 2021-04-07 23:37 ` Emily Shaffer
2021-04-08 0:23 ` Jonathan Tan
3 siblings, 0 replies; 72+ messages in thread
From: Emily Shaffer @ 2021-04-07 23:37 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, tom.saeger, gitster, sunshine, Derrick Stolee,
Derrick Stolee, Derrick Stolee
On Tue, Apr 06, 2021 at 06:47:50PM +0000, Derrick Stolee via GitGitGadget wrote:
> @@ -877,6 +878,7 @@ static int fetch_remote(struct remote *remote, void *cbdata)
[snip]
> + /*
> + * 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;
Using a literal string plus the literal value of the string length,
twice, doesn't sit great with me...
> +
> + strbuf_addstr(&new_dst, "refs/prefetch/");
> + strbuf_addstr(&new_dst, replace.dst + ignore_len);
...plus with some ugly array pointer math. :) Why not use
git-compat-util.h:skip_prefix() instead of doing your own math? (In
fact, the doc comment on skip_prefix() talks about using it exactly for
stripping "refs/" off the beginning of a string :) )
- Emily
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v2 5/5] maintenance: allow custom refspecs during prefetch
2021-04-06 18:47 ` [PATCH v2 5/5] maintenance: allow custom refspecs during prefetch Derrick Stolee via GitGitGadget
` (2 preceding siblings ...)
2021-04-07 23:37 ` Emily Shaffer
@ 2021-04-08 0:23 ` Jonathan Tan
3 siblings, 0 replies; 72+ messages in thread
From: Jonathan Tan @ 2021-04-08 0:23 UTC (permalink / raw)
To: gitgitgadget
Cc: git, tom.saeger, gitster, sunshine, stolee, derrickstolee,
dstolee, Jonathan Tan
> +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
What second refspec is being skipped?
> + 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 &&
How is this command generated? I don't see any mention of remote2 in
this test. (If it's because this repo was configured in a previous test
and some of the configuration carried over, I think it's best to start
in a new repo or at least have the previous config be cleared.)
> +
> + # 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
> +'
Other than this and the other comments that others have brought up, this
series looks good.
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v3 0/3] Maintenance: adapt custom refspecs
2021-04-06 18:47 ` [PATCH v2 0/5] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
` (4 preceding siblings ...)
2021-04-06 18:47 ` [PATCH v2 5/5] maintenance: allow custom refspecs during prefetch Derrick Stolee via GitGitGadget
@ 2021-04-10 2:03 ` Derrick Stolee via GitGitGadget
2021-04-10 2:03 ` [PATCH v3 1/3] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
` (4 more replies)
5 siblings, 5 replies; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-10 2:03 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Derrick Stolee
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.
This happens inside of git fetch when a --prefetch option is given. This
allows us to maniuplate a struct refspec_item instead of a full refspec
string. It also simplifies our logic in testing the prefetch task.
Update in V3
============
* The fix is almost completely rewritten as an update to 'git fetch'. See
the new PATCH 2 for this update.
* There was some discussion of rewriting test_subcommand, but that can be
delayed until a proper solution is found to complications around softer
matches.
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 (3):
maintenance: simplify prefetch logic
fetch: add --prefetch option
maintenance: use 'git fetch --prefetch'
Documentation/fetch-options.txt | 5 +++
Documentation/git-maintenance.txt | 6 ++--
builtin/fetch.c | 56 +++++++++++++++++++++++++++++++
builtin/gc.c | 36 +++++---------------
t/t5582-fetch-negative-refspec.sh | 30 +++++++++++++++++
t/t7900-maintenance.sh | 14 ++++----
6 files changed, 109 insertions(+), 38 deletions(-)
base-commit: 89b43f80a514aee58b662ad606e6352e03eaeee4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-924%2Fderrickstolee%2Fmaintenance%2Frefspec-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-924/derrickstolee/maintenance/refspec-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/924
Range-diff vs v2:
1: 5aa0cb06c3f2 = 1: 4c0e983ba56f maintenance: simplify prefetch logic
2: d58a3e042ee8 < -: ------------ test-lib: use exact match for test_subcommand
3: 96388d949b98 < -: ------------ refspec: output a refspec item
4: bf296282323a < -: ------------ test-tool: test refspec input/output
-: ------------ > 2: 7f488eea6dbd fetch: add --prefetch option
5: 9592224e3d42 ! 3: ed055d772452 maintenance: allow custom refspecs during prefetch
@@ Metadata
Author: Derrick Stolee <dstolee@microsoft.com>
## Commit message ##
- maintenance: allow custom refspecs during prefetch
+ maintenance: use 'git fetch --prefetch'
- The prefetch task previously used the default refspec source plus a
- custom refspec destination to avoid colliding with remote refs:
+ The 'prefetch' maintenance task previously forced the following refspec
+ for each remote:
+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.
+ If a user has specified a more strict refspec for the remote, then this
+ prefetch task downloads more objects than necessary.
- 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.
+ The previous change introduced the '--prefetch' option to 'git fetch'
+ which manipulates the remote's refspec to place all resulting refs into
+ refs/prefetch/, with further partitioning based on the destinations of
+ those refspecs.
Update the documentation to be more generic about the destination refs.
- Do not mention custom refpecs explicitly, as that does not need to be
+ Do not mention custom refspecs explicitly, as that does not need to be
highlighted in this documentation. The important part of placing refs in
- refs/prefetch remains.
+ refs/prefetch/ remains.
Reported-by: Tom Saeger <tom.saeger@oracle.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
## Documentation/git-maintenance.txt ##
-@@ Documentation/git-maintenance.txt: prefetch::
+@@ Documentation/git-maintenance.txt: commit-graph::
+ prefetch::
+ The `prefetch` task updates the object directory with the latest
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
+- 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.
++ command is run. The configured refspec is modified to place all
++ requested refs within `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 ##
-@@
- #include "remote.h"
- #include "object-store.h"
- #include "exec-cmd.h"
-+#include "refspec.h"
-
- #define FAILED_RUN "failed to run %s"
-
@@ builtin/gc.c: 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",
-@@ builtin/gc.c: static int fetch_remote(struct remote *remote, void *cbdata)
+- strvec_pushl(&child.args, "fetch", remote->name, "--prune", "--no-tags",
++ strvec_pushl(&child.args, "fetch", remote->name,
++ "--prefetch", "--prune", "--no-tags",
+ "--no-write-fetch-head", "--recurse-submodules=no",
+- "--refmap=", NULL);
++ NULL);
+
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 ##
@@ t/t7900-maintenance.sh: test_expect_success 'prefetch multiple remotes' '
+ test_commit -C clone1 one &&
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 &&
+- 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 &&
++ fetchargs="--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet" &&
++ test_subcommand git fetch remote1 $fetchargs <run-prefetch.txt &&
++ test_subcommand git fetch remote2 $fetchargs <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' '
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/ &&
--
gitgitgadget
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v3 1/3] maintenance: simplify prefetch logic
2021-04-10 2:03 ` [PATCH v3 0/3] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
@ 2021-04-10 2:03 ` Derrick Stolee via GitGitGadget
2021-04-12 20:13 ` Tom Saeger
2021-04-10 2:03 ` [PATCH v3 2/3] fetch: add --prefetch option Derrick Stolee via GitGitGadget
` (3 subsequent siblings)
4 siblings, 1 reply; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-10 2:03 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Derrick Stolee, Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
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 <dstolee@microsoft.com>
---
builtin/gc.c | 33 ++++++++-------------------------
1 file changed, 8 insertions(+), 25 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index ef7226d7bca4..fa8128de9ae1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -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)
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/3] maintenance: simplify prefetch logic
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
0 siblings, 1 reply; 72+ messages in thread
From: Tom Saeger @ 2021-04-12 20:13 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Derrick Stolee, Derrick Stolee
On Sat, Apr 10, 2021 at 02:03:43AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
> builtin/gc.c | 33 ++++++++-------------------------
> 1 file changed, 8 insertions(+), 25 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index ef7226d7bca4..fa8128de9ae1 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -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;
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?
>
> 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
>
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/3] maintenance: simplify prefetch logic
2021-04-12 20:13 ` Tom Saeger
@ 2021-04-12 20:27 ` Derrick Stolee
0 siblings, 0 replies; 72+ messages in thread
From: Derrick Stolee @ 2021-04-12 20:27 UTC (permalink / raw)
To: Tom Saeger, Derrick Stolee via GitGitGadget
Cc: git, gitster, sunshine, Josh Steadmon, Emily Shaffer,
Derrick Stolee, Derrick Stolee
On 4/12/21 4:13 PM, Tom Saeger wrote:
> On Sat, Apr 10, 2021 at 02:03:43AM +0000, Derrick Stolee via GitGitGadget wrote:
>> -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?
TIL about skipfetchall. I think that's a good idea to introduce.
It'll be a new patch, not added in this one.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v3 2/3] fetch: add --prefetch option
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-10 2:03 ` Derrick Stolee via GitGitGadget
2021-04-11 21:09 ` Ramsay Jones
2021-04-10 2:03 ` [PATCH v3 3/3] maintenance: use 'git fetch --prefetch' Derrick Stolee via GitGitGadget
` (2 subsequent siblings)
4 siblings, 1 reply; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-10 2:03 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Derrick Stolee, Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
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 <dstolee@microsoft.com>
---
Documentation/fetch-options.txt | 5 +++
builtin/fetch.c | 56 +++++++++++++++++++++++++++++++
t/t5582-fetch-negative-refspec.sh | 30 +++++++++++++++++
3 files changed, 91 insertions(+)
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 07783deee309..9e7b4e189ce0 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -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
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 0b90de87c7a2..30856b442b79 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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))) {
+ 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--;
+ 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;
diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
index f34509727702..030e6f978c4e 100755
--- a/t/t5582-fetch-negative-refspec.sh
+++ b/t/t5582-fetch-negative-refspec.sh
@@ -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
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v3 2/3] fetch: add --prefetch option
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
0 siblings, 1 reply; 72+ messages in thread
From: Ramsay Jones @ 2021-04-11 21:09 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Derrick Stolee, Derrick Stolee
On 10/04/2021 03:03, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
> Documentation/fetch-options.txt | 5 +++
> builtin/fetch.c | 56 +++++++++++++++++++++++++++++++
> t/t5582-fetch-negative-refspec.sh | 30 +++++++++++++++++
> 3 files changed, 91 insertions(+)
>
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 07783deee309..9e7b4e189ce0 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -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
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 0b90de87c7a2..30856b442b79 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -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))) {
> + 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 ;)
ATB,
Ramsay Jones
> + 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;
>
> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> index f34509727702..030e6f978c4e 100755
> --- a/t/t5582-fetch-negative-refspec.sh
> +++ b/t/t5582-fetch-negative-refspec.sh
> @@ -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
>
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 2/3] fetch: add --prefetch option
2021-04-11 21:09 ` Ramsay Jones
@ 2021-04-12 20:23 ` Derrick Stolee
0 siblings, 0 replies; 72+ messages in thread
From: Derrick Stolee @ 2021-04-12 20:23 UTC (permalink / raw)
To: Ramsay Jones, Derrick Stolee via GitGitGadget, git
Cc: tom.saeger, gitster, sunshine, Josh Steadmon, Emily Shaffer,
Derrick Stolee, Derrick Stolee
On 4/11/21 5:09 PM, Ramsay Jones wrote:> On 10/04/2021 03:03, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>> + 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))) {
>> + 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 ;)
You are absolutely right! Thanks.
-Stolee
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v3 3/3] maintenance: use 'git fetch --prefetch'
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-10 2:03 ` [PATCH v3 2/3] fetch: add --prefetch option Derrick Stolee via GitGitGadget
@ 2021-04-10 2:03 ` Derrick Stolee via GitGitGadget
2021-04-11 1:35 ` [PATCH v3 0/3] Maintenance: adapt custom refspecs Junio C Hamano
2021-04-16 12:49 ` [PATCH v4 0/4] " Derrick Stolee via GitGitGadget
4 siblings, 0 replies; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-10 2:03 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Derrick Stolee, Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
The 'prefetch' maintenance task previously forced the following refspec
for each remote:
+refs/heads/*:refs/prefetch/<remote>/*
If a user has specified a more strict refspec for the remote, then this
prefetch task downloads more objects than necessary.
The previous change introduced the '--prefetch' option to 'git fetch'
which manipulates the remote's refspec to place all resulting refs into
refs/prefetch/, with further partitioning based on the destinations of
those refspecs.
Update the documentation to be more generic about the destination refs.
Do not mention custom refspecs 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 <tom.saeger@oracle.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
Documentation/git-maintenance.txt | 6 ++----
builtin/gc.c | 7 +++----
t/t7900-maintenance.sh | 14 +++++++-------
3 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 80ddd33ceba0..1e738ad39832 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -92,10 +92,8 @@ commit-graph::
prefetch::
The `prefetch` task updates the object directory with the latest
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.
+ command is run. The configured refspec is modified to place all
+ requested refs within `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
diff --git a/builtin/gc.c b/builtin/gc.c
index fa8128de9ae1..9d35f7da50d8 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -879,15 +879,14 @@ static int fetch_remote(struct remote *remote, void *cbdata)
struct child_process child = CHILD_PROCESS_INIT;
child.git_cmd = 1;
- strvec_pushl(&child.args, "fetch", remote->name, "--prune", "--no-tags",
+ strvec_pushl(&child.args, "fetch", remote->name,
+ "--prefetch", "--prune", "--no-tags",
"--no-write-fetch-head", "--recurse-submodules=no",
- "--refmap=", NULL);
+ NULL);
if (opts->quiet)
strvec_push(&child.args, "--quiet");
- strvec_pushf(&child.args, "+refs/heads/*:refs/prefetch/%s/*", remote->name);
-
return !!run_command(&child);
}
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 2412d8c5c006..eadb800c08cc 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -141,15 +141,15 @@ test_expect_success 'prefetch multiple remotes' '
test_commit -C clone1 one &&
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 &&
+ fetchargs="--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet" &&
+ test_subcommand git fetch remote1 $fetchargs <run-prefetch.txt &&
+ test_subcommand git fetch remote2 $fetchargs <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 &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v3 0/3] Maintenance: adapt custom refspecs
2021-04-10 2:03 ` [PATCH v3 0/3] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
` (2 preceding siblings ...)
2021-04-10 2:03 ` [PATCH v3 3/3] maintenance: use 'git fetch --prefetch' Derrick Stolee via GitGitGadget
@ 2021-04-11 1:35 ` Junio C Hamano
2021-04-12 16:48 ` Tom Saeger
2021-04-16 12:49 ` [PATCH v4 0/4] " Derrick Stolee via GitGitGadget
4 siblings, 1 reply; 72+ messages in thread
From: Junio C Hamano @ 2021-04-11 1:35 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, tom.saeger, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Derrick Stolee
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> * 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 ;-)
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 0/3] Maintenance: adapt custom refspecs
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
0 siblings, 1 reply; 72+ messages in thread
From: Tom Saeger @ 2021-04-12 16:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee via GitGitGadget, git, sunshine, Derrick Stolee,
Josh Steadmon, Emily Shaffer, Derrick Stolee
On Sat, Apr 10, 2021 at 06:35:40PM -0700, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > * 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 ;-)
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
In this case, running `git fetch pr-924` will fetch and update
FETCH_HEAD, but running with maintenance prefetch task results in:
fatal: Couldn't find remote ref HEAD
error: failed to prefetch remotes
error: task 'prefetch' failed
I tracked this down a bit, but don't have a suggestion how to fix it.
builtin/fetch.c `get_ref_map` makes two calls to `filter_prefetch_refspec`,
one for 'rs' and another for 'remote->fetch'.
`filter_prefetch_refspec` works and filters out the above fetch config.
This correctly yields condition
`rs->nr == 0` and `remote->fetch.nr == 0`
Later a call is made to `get_remote_ref(remote_refs, "HEAD")` which
fails, leading to `fatal: Couldn't find remote ref HEAD`
Should this be expected, or should this now be special-cased for 'prefetch'
somehow?
Regards,
--Tom
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 0/3] Maintenance: adapt custom refspecs
2021-04-12 16:48 ` Tom Saeger
@ 2021-04-12 17:24 ` Tom Saeger
2021-04-12 17:41 ` Tom Saeger
0 siblings, 1 reply; 72+ messages in thread
From: Tom Saeger @ 2021-04-12 17:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee via GitGitGadget, git, sunshine, Derrick Stolee,
Josh Steadmon, Emily Shaffer, Derrick Stolee
On Mon, Apr 12, 2021 at 11:48:09AM -0500, Tom Saeger wrote:
> On Sat, Apr 10, 2021 at 06:35:40PM -0700, Junio C Hamano wrote:
> > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > * 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 ;-)
>
> 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
>
>
> In this case, running `git fetch pr-924` will fetch and update
> FETCH_HEAD, but running with maintenance prefetch task results in:
>
> fatal: Couldn't find remote ref HEAD
> error: failed to prefetch remotes
> error: task 'prefetch' failed
>
> I tracked this down a bit, but don't have a suggestion how to fix it.
This ugly hack fixes this failure. I'll keep staring at it.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 30856b442b79..6489ce7d8d3b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -508,6 +508,9 @@ static struct ref *get_ref_map(struct remote *remote,
if (remote)
filter_prefetch_refspec(&remote->fetch);
+ if (prefetch && !rs->nr && remote && !remote->fetch.nr)
+ return NULL;
+
if (rs->nr) {
struct refspec *fetch_refspec;
--
>
> builtin/fetch.c `get_ref_map` makes two calls to `filter_prefetch_refspec`,
> one for 'rs' and another for 'remote->fetch'.
>
> `filter_prefetch_refspec` works and filters out the above fetch config.
> This correctly yields condition
> `rs->nr == 0` and `remote->fetch.nr == 0`
>
> Later a call is made to `get_remote_ref(remote_refs, "HEAD")` which
> fails, leading to `fatal: Couldn't find remote ref HEAD`
>
> Should this be expected, or should this now be special-cased for 'prefetch'
> somehow?
>
> Regards,
>
> --Tom
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v3 0/3] Maintenance: adapt custom refspecs
2021-04-12 17:24 ` Tom Saeger
@ 2021-04-12 17:41 ` Tom Saeger
2021-04-12 20:25 ` Derrick Stolee
0 siblings, 1 reply; 72+ messages in thread
From: Tom Saeger @ 2021-04-12 17:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: Derrick Stolee via GitGitGadget, git, sunshine, Derrick Stolee,
Josh Steadmon, Emily Shaffer, Derrick Stolee
On Mon, Apr 12, 2021 at 12:24:27PM -0500, Tom Saeger wrote:
> On Mon, Apr 12, 2021 at 11:48:09AM -0500, Tom Saeger wrote:
> > On Sat, Apr 10, 2021 at 06:35:40PM -0700, Junio C Hamano wrote:
> > > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > >
> > > > * 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 ;-)
> >
> > 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
> >
> >
> > In this case, running `git fetch pr-924` will fetch and update
> > FETCH_HEAD, but running with maintenance prefetch task results in:
> >
> > fatal: Couldn't find remote ref HEAD
> > error: failed to prefetch remotes
> > error: task 'prefetch' failed
> >
> > I tracked this down a bit, but don't have a suggestion how to fix it.
>
> This ugly hack fixes this failure. I'll keep staring at it.
>
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 30856b442b79..6489ce7d8d3b 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -508,6 +508,9 @@ static struct ref *get_ref_map(struct remote *remote,
> if (remote)
> filter_prefetch_refspec(&remote->fetch);
>
> + if (prefetch && !rs->nr && remote && !remote->fetch.nr)
> + return NULL;
> +
> if (rs->nr) {
> struct refspec *fetch_refspec;
>
> --
>
Less ugly fix...
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 30856b442b79..5fbffbd17d7d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -576,6 +576,8 @@ static struct ref *get_ref_map(struct remote *remote,
if (has_merge &&
!strcmp(branch->remote_name, remote->name))
add_merge_config(&ref_map, remote_refs, branch, &tail);
+ } else if (prefetch) {
+ ;
} else {
ref_map = get_remote_ref(remote_refs, "HEAD");
if (!ref_map)
--
Other ideas?
>
>
> >
> > builtin/fetch.c `get_ref_map` makes two calls to `filter_prefetch_refspec`,
> > one for 'rs' and another for 'remote->fetch'.
> >
> > `filter_prefetch_refspec` works and filters out the above fetch config.
> > This correctly yields condition
> > `rs->nr == 0` and `remote->fetch.nr == 0`
> >
> > Later a call is made to `get_remote_ref(remote_refs, "HEAD")` which
> > fails, leading to `fatal: Couldn't find remote ref HEAD`
> >
> > Should this be expected, or should this now be special-cased for 'prefetch'
> > somehow?
> >
> > Regards,
> >
> > --Tom
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v3 0/3] Maintenance: adapt custom refspecs
2021-04-12 17:41 ` Tom Saeger
@ 2021-04-12 20:25 ` Derrick Stolee
0 siblings, 0 replies; 72+ messages in thread
From: Derrick Stolee @ 2021-04-12 20:25 UTC (permalink / raw)
To: Tom Saeger, Junio C Hamano
Cc: Derrick Stolee via GitGitGadget, git, sunshine, Josh Steadmon,
Emily Shaffer, Derrick Stolee
On 4/12/21 1:41 PM, Tom Saeger wrote:>
> Less ugly fix...
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 30856b442b79..5fbffbd17d7d 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -576,6 +576,8 @@ static struct ref *get_ref_map(struct remote *remote,
> if (has_merge &&
> !strcmp(branch->remote_name, remote->name))
> add_merge_config(&ref_map, remote_refs, branch, &tail);
> + } else if (prefetch) {
> + ;
> } else {
I'll give this a try, but with "else if (!prefetch)" for the
last block,instead.
Thanks for your diligent testing! It's helping a lot.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v4 0/4] Maintenance: adapt custom refspecs
2021-04-10 2:03 ` [PATCH v3 0/3] Maintenance: adapt custom refspecs Derrick Stolee via GitGitGadget
` (3 preceding siblings ...)
2021-04-11 1:35 ` [PATCH v3 0/3] Maintenance: adapt custom refspecs Junio C Hamano
@ 2021-04-16 12:49 ` Derrick Stolee via GitGitGadget
2021-04-16 12:49 ` [PATCH v4 1/4] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
` (3 more replies)
4 siblings, 4 replies; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-16 12:49 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Ramsay Jones, Derrick Stolee
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.
This happens inside of git fetch when a --prefetch option is given. This
allows us to maniuplate a struct refspec_item instead of a full refspec
string. It also simplifies our logic in testing the prefetch task.
Updates in V4
=============
* Two bugs were fixed. Thanks, Ramsay and Tom, for pointing out the issues.
Tests are added that prevent regressions.
* A new patch is added to respect remote.<name>.skipFetchAll. This is added
at the end to take advantage of the simpler test design after --prefetch
is added.
Update in V3
============
* The fix is almost completely rewritten as an update to 'git fetch'. See
the new PATCH 2 for this update.
* There was some discussion of rewriting test_subcommand, but that can be
delayed until a proper solution is found to complications around softer
matches.
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 (4):
maintenance: simplify prefetch logic
fetch: add --prefetch option
maintenance: use 'git fetch --prefetch'
maintenance: respect remote.*.skipFetchAll
Documentation/fetch-options.txt | 5 +++
Documentation/git-maintenance.txt | 6 ++--
builtin/fetch.c | 59 ++++++++++++++++++++++++++++++-
builtin/gc.c | 39 +++++++-------------
t/t5582-fetch-negative-refspec.sh | 43 ++++++++++++++++++++++
t/t7900-maintenance.sh | 22 +++++++-----
6 files changed, 134 insertions(+), 40 deletions(-)
base-commit: 89b43f80a514aee58b662ad606e6352e03eaeee4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-924%2Fderrickstolee%2Fmaintenance%2Frefspec-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-924/derrickstolee/maintenance/refspec-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/924
Range-diff vs v3:
1: 4c0e983ba56f = 1: 4c0e983ba56f maintenance: simplify prefetch logic
2: 7f488eea6dbd ! 2: 73b4e8496746 fetch: add --prefetch option
@@ Commit message
Finally, we add the 'force' option to ensure that prefetch refs are
replaced as necessary.
+ There are some interesting cases that are worth testing.
+
+ An earlier version of this change dropped the "i--" from the loop that
+ deletes a refspec item and shifts the remaining entries down. This
+ allowed some refspecs to not be modified. The subtle part about the
+ first --prefetch test is that the "refs/tags/*" refspec appears directly
+ before the "refs/heads/bogus/*" refspec. Without that "i--", this
+ ordering would remove the "refs/tags/*" refspec and leave the last one
+ unmodified, placing the result in "refs/heads/*".
+
+ It is possible to have an empty refspec. This is typically the case for
+ remotes other than the origin, where users want to fetch a specific tag
+ or branch. To correctly test this case, we need to further remove the
+ upstream remote for the local branch. Thus, we are testing a refspec
+ that will be deleted, leaving nothing to fetch.
+
+ Helped-by: Tom Saeger <tom.saeger@oracle.com>
+ Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
## Documentation/fetch-options.txt ##
@@ builtin/fetch.c: static void find_non_local_tags(const struct ref *refs,
+ rs->raw[j - 1] = rs->raw[j];
+ }
+ rs->nr--;
++ i--;
+ continue;
+ }
+
@@ builtin/fetch.c: static struct ref *get_ref_map(struct remote *remote,
if (rs->nr) {
struct refspec *fetch_refspec;
+@@ builtin/fetch.c: static struct ref *get_ref_map(struct remote *remote,
+ if (has_merge &&
+ !strcmp(branch->remote_name, remote->name))
+ add_merge_config(&ref_map, remote_refs, branch, &tail);
+- } else {
++ } else if (!prefetch) {
+ ref_map = get_remote_ref(remote_refs, "HEAD");
+ if (!ref_map)
+ die(_("Couldn't find remote ref HEAD"));
## t/t5582-fetch-negative-refspec.sh ##
@@ t/t5582-fetch-negative-refspec.sh: test_expect_success "push with matching +: and negative refspec" '
@@ t/t5582-fetch-negative-refspec.sh: test_expect_success "push with matching +: an
+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/tags/*:refs/tags/*" &&
+ git -C one config --add remote.origin.fetch "refs/heads/bogus/*:bogus/*" &&
+
+ git tag -a -m never never-fetch-tag HEAD &&
@@ t/t5582-fetch-negative-refspec.sh: test_expect_success "push with matching +: an
+ git -C one rev-parse refs/prefetch/bogus/fetched &&
+ test_must_fail git -C one rev-parse refs/prefetch/bogus/ignore
+'
++
++test_expect_success '--prefetch succeeds when refspec becomes empty' '
++ git checkout bogus/fetched &&
++ test_commit extra &&
++
++ git -C one config --unset-all remote.origin.fetch &&
++ git -C one config --unset branch.main.remote &&
++ git -C one config remote.origin.fetch "+refs/tags/extra" &&
++ git -C one config remote.origin.skipfetchall true &&
++ git -C one config remote.origin.tagopt "--no-tags" &&
++
++ git -C one fetch --prefetch
++'
+
test_done
3: ed055d772452 = 3: 565ed8a18929 maintenance: use 'git fetch --prefetch'
-: ------------ > 4: 92652fd9e6e1 maintenance: respect remote.*.skipFetchAll
--
gitgitgadget
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v4 1/4] maintenance: simplify prefetch logic
2021-04-16 12:49 ` [PATCH v4 0/4] " Derrick Stolee via GitGitGadget
@ 2021-04-16 12:49 ` 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
` (2 subsequent siblings)
3 siblings, 1 reply; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-16 12:49 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Ramsay Jones, Derrick Stolee, Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
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 <dstolee@microsoft.com>
---
builtin/gc.c | 33 ++++++++-------------------------
1 file changed, 8 insertions(+), 25 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index ef7226d7bca4..fa8128de9ae1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -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)
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v4 1/4] maintenance: simplify prefetch logic
2021-04-16 12:49 ` [PATCH v4 1/4] maintenance: simplify prefetch logic Derrick Stolee via GitGitGadget
@ 2021-04-16 18:02 ` Tom Saeger
0 siblings, 0 replies; 72+ messages in thread
From: Tom Saeger @ 2021-04-16 18:02 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Ramsay Jones, Derrick Stolee, Derrick Stolee
On Fri, Apr 16, 2021 at 12:49:56PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
Reviewed-by: Tom Saeger <tom.saeger@oracle.com>
> ---
> builtin/gc.c | 33 ++++++++-------------------------
> 1 file changed, 8 insertions(+), 25 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index ef7226d7bca4..fa8128de9ae1 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -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)
> --
> gitgitgadget
>
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v4 2/4] fetch: add --prefetch option
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 12:49 ` Derrick Stolee via GitGitGadget
2021-04-16 17:52 ` 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
3 siblings, 1 reply; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-16 12:49 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Ramsay Jones, Derrick Stolee, Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
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.
There are some interesting cases that are worth testing.
An earlier version of this change dropped the "i--" from the loop that
deletes a refspec item and shifts the remaining entries down. This
allowed some refspecs to not be modified. The subtle part about the
first --prefetch test is that the "refs/tags/*" refspec appears directly
before the "refs/heads/bogus/*" refspec. Without that "i--", this
ordering would remove the "refs/tags/*" refspec and leave the last one
unmodified, placing the result in "refs/heads/*".
It is possible to have an empty refspec. This is typically the case for
remotes other than the origin, where users want to fetch a specific tag
or branch. To correctly test this case, we need to further remove the
upstream remote for the local branch. Thus, we are testing a refspec
that will be deleted, leaving nothing to fetch.
Helped-by: Tom Saeger <tom.saeger@oracle.com>
Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
Documentation/fetch-options.txt | 5 +++
builtin/fetch.c | 59 ++++++++++++++++++++++++++++++-
t/t5582-fetch-negative-refspec.sh | 43 ++++++++++++++++++++++
3 files changed, 106 insertions(+), 1 deletion(-)
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 07783deee309..9e7b4e189ce0 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -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
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 0b90de87c7a2..97c4fe6e6d66 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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,56 @@ 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))) {
+ 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--;
+ i--;
+ 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 +505,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;
@@ -520,7 +577,7 @@ static struct ref *get_ref_map(struct remote *remote,
if (has_merge &&
!strcmp(branch->remote_name, remote->name))
add_merge_config(&ref_map, remote_refs, branch, &tail);
- } else {
+ } else if (!prefetch) {
ref_map = get_remote_ref(remote_refs, "HEAD");
if (!ref_map)
die(_("Couldn't find remote ref HEAD"));
diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
index f34509727702..e5d2e79ad382 100755
--- a/t/t5582-fetch-negative-refspec.sh
+++ b/t/t5582-fetch-negative-refspec.sh
@@ -240,4 +240,47 @@ 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/heads/bogus/ignore &&
+ git -C one config --add remote.origin.fetch "refs/tags/*:refs/tags/*" &&
+ 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_expect_success '--prefetch succeeds when refspec becomes empty' '
+ git checkout bogus/fetched &&
+ test_commit extra &&
+
+ git -C one config --unset-all remote.origin.fetch &&
+ git -C one config --unset branch.main.remote &&
+ git -C one config remote.origin.fetch "+refs/tags/extra" &&
+ git -C one config remote.origin.skipfetchall true &&
+ git -C one config remote.origin.tagopt "--no-tags" &&
+
+ git -C one fetch --prefetch
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v4 2/4] fetch: add --prefetch option
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
0 siblings, 1 reply; 72+ messages in thread
From: Tom Saeger @ 2021-04-16 17:52 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Ramsay Jones, Derrick Stolee, Derrick Stolee
On Fri, Apr 16, 2021 at 12:49:57PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.
>
> There are some interesting cases that are worth testing.
>
> An earlier version of this change dropped the "i--" from the loop that
> deletes a refspec item and shifts the remaining entries down. This
> allowed some refspecs to not be modified. The subtle part about the
> first --prefetch test is that the "refs/tags/*" refspec appears directly
> before the "refs/heads/bogus/*" refspec. Without that "i--", this
> ordering would remove the "refs/tags/*" refspec and leave the last one
> unmodified, placing the result in "refs/heads/*".
>
> It is possible to have an empty refspec. This is typically the case for
> remotes other than the origin, where users want to fetch a specific tag
> or branch. To correctly test this case, we need to further remove the
> upstream remote for the local branch. Thus, we are testing a refspec
> that will be deleted, leaving nothing to fetch.
>
> Helped-by: Tom Saeger <tom.saeger@oracle.com>
> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> Documentation/fetch-options.txt | 5 +++
> builtin/fetch.c | 59 ++++++++++++++++++++++++++++++-
> t/t5582-fetch-negative-refspec.sh | 43 ++++++++++++++++++++++
> 3 files changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 07783deee309..9e7b4e189ce0 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -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
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 0b90de87c7a2..97c4fe6e6d66 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -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,56 @@ 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))) {
> + 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--;
> + i--;
> + 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 +505,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;
>
> @@ -520,7 +577,7 @@ static struct ref *get_ref_map(struct remote *remote,
> if (has_merge &&
> !strcmp(branch->remote_name, remote->name))
> add_merge_config(&ref_map, remote_refs, branch, &tail);
> - } else {
> + } else if (!prefetch) {
That works for me.
> ref_map = get_remote_ref(remote_refs, "HEAD");
> if (!ref_map)
> die(_("Couldn't find remote ref HEAD"));
> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> index f34509727702..e5d2e79ad382 100755
> --- a/t/t5582-fetch-negative-refspec.sh
> +++ b/t/t5582-fetch-negative-refspec.sh
> @@ -240,4 +240,47 @@ 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/heads/bogus/ignore &&
> + git -C one config --add remote.origin.fetch "refs/tags/*:refs/tags/*" &&
> + 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_expect_success '--prefetch succeeds when refspec becomes empty' '
technically this will get skipped based only on "skipfetchall" right?
The remote could have an empty-set of refspecs or multiple
valid refspecs post filter_prefetch_refspec, but the remote gets skipped altogether.
perhaps '--prefetch succeeds when remote.skipfetchall is true' '
anyway this is looking pretty solid
Reviewed-by: Tom Saeger <tom.saeger@oracle.com>
> + git checkout bogus/fetched &&
> + test_commit extra &&
> +
> + git -C one config --unset-all remote.origin.fetch &&
> + git -C one config --unset branch.main.remote &&
> + git -C one config remote.origin.fetch "+refs/tags/extra" &&
> + git -C one config remote.origin.skipfetchall true &&
> + git -C one config remote.origin.tagopt "--no-tags" &&
> +
> + git -C one fetch --prefetch
> +'
> +
> test_done
> --
> gitgitgadget
>
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v4 2/4] fetch: add --prefetch option
2021-04-16 17:52 ` Tom Saeger
@ 2021-04-16 18:26 ` Tom Saeger
0 siblings, 0 replies; 72+ messages in thread
From: Tom Saeger @ 2021-04-16 18:26 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Ramsay Jones, Derrick Stolee, Derrick Stolee
On Fri, Apr 16, 2021 at 12:52:21PM -0500, Tom Saeger wrote:
> On Fri, Apr 16, 2021 at 12:49:57PM +0000, Derrick Stolee via GitGitGadget wrote:
> > From: Derrick Stolee <dstolee@microsoft.com>
> >
> > 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.
> >
> > There are some interesting cases that are worth testing.
> >
> > An earlier version of this change dropped the "i--" from the loop that
> > deletes a refspec item and shifts the remaining entries down. This
> > allowed some refspecs to not be modified. The subtle part about the
> > first --prefetch test is that the "refs/tags/*" refspec appears directly
> > before the "refs/heads/bogus/*" refspec. Without that "i--", this
> > ordering would remove the "refs/tags/*" refspec and leave the last one
> > unmodified, placing the result in "refs/heads/*".
> >
> > It is possible to have an empty refspec. This is typically the case for
> > remotes other than the origin, where users want to fetch a specific tag
> > or branch. To correctly test this case, we need to further remove the
> > upstream remote for the local branch. Thus, we are testing a refspec
> > that will be deleted, leaving nothing to fetch.
> >
> > Helped-by: Tom Saeger <tom.saeger@oracle.com>
> > Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> > ---
> > Documentation/fetch-options.txt | 5 +++
> > builtin/fetch.c | 59 ++++++++++++++++++++++++++++++-
> > t/t5582-fetch-negative-refspec.sh | 43 ++++++++++++++++++++++
> > 3 files changed, 106 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> > index 07783deee309..9e7b4e189ce0 100644
> > --- a/Documentation/fetch-options.txt
> > +++ b/Documentation/fetch-options.txt
> > @@ -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
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index 0b90de87c7a2..97c4fe6e6d66 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -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,56 @@ 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))) {
> > + 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--;
> > + i--;
> > + 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 +505,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;
> >
> > @@ -520,7 +577,7 @@ static struct ref *get_ref_map(struct remote *remote,
> > if (has_merge &&
> > !strcmp(branch->remote_name, remote->name))
> > add_merge_config(&ref_map, remote_refs, branch, &tail);
> > - } else {
> > + } else if (!prefetch) {
>
> That works for me.
>
> > ref_map = get_remote_ref(remote_refs, "HEAD");
> > if (!ref_map)
> > die(_("Couldn't find remote ref HEAD"));
> > diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> > index f34509727702..e5d2e79ad382 100755
> > --- a/t/t5582-fetch-negative-refspec.sh
> > +++ b/t/t5582-fetch-negative-refspec.sh
> > @@ -240,4 +240,47 @@ 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/heads/bogus/ignore &&
> > + git -C one config --add remote.origin.fetch "refs/tags/*:refs/tags/*" &&
> > + 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_expect_success '--prefetch succeeds when refspec becomes empty' '
>
> technically this will get skipped based only on "skipfetchall" right?
>
> The remote could have an empty-set of refspecs or multiple
> valid refspecs post filter_prefetch_refspec, but the remote gets skipped altogether.
>
> perhaps '--prefetch succeeds when remote.skipfetchall is true' '
Forget what I said here. Now seeing this is using 'git fetch --prefetch'
directly and not using `mainenance --task=prefetch`
>
> anyway this is looking pretty solid
>
> Reviewed-by: Tom Saeger <tom.saeger@oracle.com>
>
> > + git checkout bogus/fetched &&
> > + test_commit extra &&
> > +
> > + git -C one config --unset-all remote.origin.fetch &&
> > + git -C one config --unset branch.main.remote &&
> > + git -C one config remote.origin.fetch "+refs/tags/extra" &&
> > + git -C one config remote.origin.skipfetchall true &&
> > + git -C one config remote.origin.tagopt "--no-tags" &&
> > +
> > + git -C one fetch --prefetch
> > +'
> > +
> > test_done
> > --
> > gitgitgadget
> >
^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v4 3/4] maintenance: use 'git fetch --prefetch'
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 12:49 ` [PATCH v4 2/4] fetch: add --prefetch option Derrick Stolee via GitGitGadget
@ 2021-04-16 12:49 ` Derrick Stolee via GitGitGadget
2021-04-16 12:49 ` [PATCH v4 4/4] maintenance: respect remote.*.skipFetchAll Derrick Stolee via GitGitGadget
3 siblings, 0 replies; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-16 12:49 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Ramsay Jones, Derrick Stolee, Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
The 'prefetch' maintenance task previously forced the following refspec
for each remote:
+refs/heads/*:refs/prefetch/<remote>/*
If a user has specified a more strict refspec for the remote, then this
prefetch task downloads more objects than necessary.
The previous change introduced the '--prefetch' option to 'git fetch'
which manipulates the remote's refspec to place all resulting refs into
refs/prefetch/, with further partitioning based on the destinations of
those refspecs.
Update the documentation to be more generic about the destination refs.
Do not mention custom refspecs 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 <tom.saeger@oracle.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
Documentation/git-maintenance.txt | 6 ++----
builtin/gc.c | 7 +++----
t/t7900-maintenance.sh | 14 +++++++-------
3 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
index 80ddd33ceba0..1e738ad39832 100644
--- a/Documentation/git-maintenance.txt
+++ b/Documentation/git-maintenance.txt
@@ -92,10 +92,8 @@ commit-graph::
prefetch::
The `prefetch` task updates the object directory with the latest
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.
+ command is run. The configured refspec is modified to place all
+ requested refs within `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
diff --git a/builtin/gc.c b/builtin/gc.c
index fa8128de9ae1..9d35f7da50d8 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -879,15 +879,14 @@ static int fetch_remote(struct remote *remote, void *cbdata)
struct child_process child = CHILD_PROCESS_INIT;
child.git_cmd = 1;
- strvec_pushl(&child.args, "fetch", remote->name, "--prune", "--no-tags",
+ strvec_pushl(&child.args, "fetch", remote->name,
+ "--prefetch", "--prune", "--no-tags",
"--no-write-fetch-head", "--recurse-submodules=no",
- "--refmap=", NULL);
+ NULL);
if (opts->quiet)
strvec_push(&child.args, "--quiet");
- strvec_pushf(&child.args, "+refs/heads/*:refs/prefetch/%s/*", remote->name);
-
return !!run_command(&child);
}
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 2412d8c5c006..eadb800c08cc 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -141,15 +141,15 @@ test_expect_success 'prefetch multiple remotes' '
test_commit -C clone1 one &&
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 &&
+ fetchargs="--prefetch --prune --no-tags --no-write-fetch-head --recurse-submodules=no --quiet" &&
+ test_subcommand git fetch remote1 $fetchargs <run-prefetch.txt &&
+ test_subcommand git fetch remote2 $fetchargs <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 &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [PATCH v4 4/4] maintenance: respect remote.*.skipFetchAll
2021-04-16 12:49 ` [PATCH v4 0/4] " Derrick Stolee via GitGitGadget
` (2 preceding siblings ...)
2021-04-16 12:49 ` [PATCH v4 3/4] maintenance: use 'git fetch --prefetch' Derrick Stolee via GitGitGadget
@ 2021-04-16 12:49 ` Derrick Stolee via GitGitGadget
2021-04-16 13:54 ` Ævar Arnfjörð Bjarmason
2021-04-16 18:31 ` Tom Saeger
3 siblings, 2 replies; 72+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2021-04-16 12:49 UTC (permalink / raw)
To: git
Cc: tom.saeger, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Ramsay Jones, Derrick Stolee, Derrick Stolee
From: Derrick Stolee <dstolee@microsoft.com>
If a remote has the skipFetchAll setting enabled, then that remote is
not intended for frequent fetching. It makes sense to not fetch that
data during the 'prefetch' maintenance task. Skip that remote in the
iteration without error. The skip_default_update member is initialized
in remote.c:handle_config() as part of initializing the 'struct remote'.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
builtin/gc.c | 3 +++
t/t7900-maintenance.sh | 8 +++++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index 9d35f7da50d8..98a803196b88 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -878,6 +878,9 @@ static int fetch_remote(struct remote *remote, void *cbdata)
struct maintenance_run_opts *opts = cbdata;
struct child_process child = CHILD_PROCESS_INIT;
+ if (remote->skip_default_update)
+ return 0;
+
child.git_cmd = 1;
strvec_pushl(&child.args, "fetch", remote->name,
"--prefetch", "--prune", "--no-tags",
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index eadb800c08cc..b93ae014ee58 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -153,7 +153,13 @@ test_expect_success 'prefetch multiple remotes' '
test_cmp_config refs/prefetch/ log.excludedecoration &&
git log --oneline --decorate --all >log &&
- ! grep "prefetch" log
+ ! grep "prefetch" log &&
+
+ test_when_finished git config --unset remote.remote1.skipFetchAll &&
+ git config remote.remote1.skipFetchAll true &&
+ GIT_TRACE2_EVENT="$(pwd)/skip-remote1.txt" git maintenance run --task=prefetch 2>/dev/null &&
+ test_subcommand ! git fetch remote1 $fetchargs <skip-remote1.txt &&
+ test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt
'
test_expect_success 'prefetch and existing log.excludeDecoration values' '
--
gitgitgadget
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v4 4/4] maintenance: respect remote.*.skipFetchAll
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
1 sibling, 1 reply; 72+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-04-16 13:54 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, tom.saeger, gitster, sunshine, Derrick Stolee,
Josh Steadmon, Emily Shaffer, Ramsay Jones, Derrick Stolee,
Derrick Stolee
On Fri, Apr 16 2021, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> If a remote has the skipFetchAll setting enabled, then that remote is
> not intended for frequent fetching. It makes sense to not fetch that
> data during the 'prefetch' maintenance task. Skip that remote in the
> iteration without error. The skip_default_update member is initialized
> in remote.c:handle_config() as part of initializing the 'struct remote'.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> builtin/gc.c | 3 +++
> t/t7900-maintenance.sh | 8 +++++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 9d35f7da50d8..98a803196b88 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -878,6 +878,9 @@ static int fetch_remote(struct remote *remote, void *cbdata)
> struct maintenance_run_opts *opts = cbdata;
> struct child_process child = CHILD_PROCESS_INIT;
>
> + if (remote->skip_default_update)
> + return 0;
> +
> child.git_cmd = 1;
> strvec_pushl(&child.args, "fetch", remote->name,
> "--prefetch", "--prune", "--no-tags",
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index eadb800c08cc..b93ae014ee58 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -153,7 +153,13 @@ test_expect_success 'prefetch multiple remotes' '
>
> test_cmp_config refs/prefetch/ log.excludedecoration &&
> git log --oneline --decorate --all >log &&
> - ! grep "prefetch" log
> + ! grep "prefetch" log &&
> +
> + test_when_finished git config --unset remote.remote1.skipFetchAll &&
> + git config remote.remote1.skipFetchAll true &&
> + GIT_TRACE2_EVENT="$(pwd)/skip-remote1.txt" git maintenance run --task=prefetch 2>/dev/null &&
> + test_subcommand ! git fetch remote1 $fetchargs <skip-remote1.txt &&
> + test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt
> '
>
> test_expect_success 'prefetch and existing log.excludeDecoration values' '
Without having read the code I'd have very much expected a
"remote.*.skipFetchAll" to impact:
git fetch --all
Or:
git remote update --all # --all does not exist yet
As e.g. remote.<name>.skipDefaultUpdate would do (i.e. impact "git
remote update" ...).
I suspect naming it like this started as a hack around the lack of
4-level .ini config keys, i.e. so we could do:
maintenance.remote.<name>.skipFetchAll = true
But I wonder if we couldn't give this a less confusing name still, even
the pseudo-command form of:
maintenanceRemote.<name>.skipFetchAll = true
Or something...
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v4 4/4] maintenance: respect remote.*.skipFetchAll
2021-04-16 13:54 ` Ævar Arnfjörð Bjarmason
@ 2021-04-16 14:33 ` Tom Saeger
0 siblings, 0 replies; 72+ messages in thread
From: Tom Saeger @ 2021-04-16 14:33 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Derrick Stolee via GitGitGadget, git, gitster, sunshine,
Derrick Stolee, Josh Steadmon, Emily Shaffer, Ramsay Jones,
Derrick Stolee, Derrick Stolee
On Fri, Apr 16, 2021 at 03:54:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, Apr 16 2021, Derrick Stolee via GitGitGadget wrote:
>
> > From: Derrick Stolee <dstolee@microsoft.com>
> >
> > If a remote has the skipFetchAll setting enabled, then that remote is
> > not intended for frequent fetching. It makes sense to not fetch that
> > data during the 'prefetch' maintenance task. Skip that remote in the
> > iteration without error. The skip_default_update member is initialized
> > in remote.c:handle_config() as part of initializing the 'struct remote'.
> >
> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> > ---
> > builtin/gc.c | 3 +++
> > t/t7900-maintenance.sh | 8 +++++++-
> > 2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 9d35f7da50d8..98a803196b88 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -878,6 +878,9 @@ static int fetch_remote(struct remote *remote, void *cbdata)
> > struct maintenance_run_opts *opts = cbdata;
> > struct child_process child = CHILD_PROCESS_INIT;
> >
> > + if (remote->skip_default_update)
> > + return 0;
> > +
> > child.git_cmd = 1;
> > strvec_pushl(&child.args, "fetch", remote->name,
> > "--prefetch", "--prune", "--no-tags",
> > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> > index eadb800c08cc..b93ae014ee58 100755
> > --- a/t/t7900-maintenance.sh
> > +++ b/t/t7900-maintenance.sh
> > @@ -153,7 +153,13 @@ test_expect_success 'prefetch multiple remotes' '
> >
> > test_cmp_config refs/prefetch/ log.excludedecoration &&
> > git log --oneline --decorate --all >log &&
> > - ! grep "prefetch" log
> > + ! grep "prefetch" log &&
> > +
> > + test_when_finished git config --unset remote.remote1.skipFetchAll &&
> > + git config remote.remote1.skipFetchAll true &&
> > + GIT_TRACE2_EVENT="$(pwd)/skip-remote1.txt" git maintenance run --task=prefetch 2>/dev/null &&
> > + test_subcommand ! git fetch remote1 $fetchargs <skip-remote1.txt &&
> > + test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt
> > '
> >
> > test_expect_success 'prefetch and existing log.excludeDecoration values' '
>
> Without having read the code I'd have very much expected a
> "remote.*.skipFetchAll" to impact:
>
> git fetch --all
'skipFetchAll' indeed impacts 'git fetch --all'
But this patch doesn't add "skipFetchAll", instead it just honors that
config if set during 'git maintenance --task=prefetch'
See v3 discussion: https://lore.kernel.org/git/2f4fa2b5-0d8b-b368-ab4d-411740595a4f@gmail.com/
>
> Or:
>
> git remote update --all # --all does not exist yet
>
> As e.g. remote.<name>.skipDefaultUpdate would do (i.e. impact "git
> remote update" ...).
>
> I suspect naming it like this started as a hack around the lack of
> 4-level .ini config keys, i.e. so we could do:
>
> maintenance.remote.<name>.skipFetchAll = true
>
> But I wonder if we couldn't give this a less confusing name still, even
> the pseudo-command form of:
>
> maintenanceRemote.<name>.skipFetchAll = true
>
> Or something...
>
Whether or not additional maintenance specific configs are desired, that might be
something to consider. I've thought about this for a few of my repos
which have remotes which require a VPN connection. Perhaps
I want to skip those during 'prefetch'? Or maybe instead define a
'remotes.prefetch' group and only prefetch remotes listed?
That all said - I wouldn't hold-up this patch series for it.
--Tom
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v4 4/4] maintenance: respect remote.*.skipFetchAll
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 18:31 ` Tom Saeger
1 sibling, 0 replies; 72+ messages in thread
From: Tom Saeger @ 2021-04-16 18:31 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, gitster, sunshine, Derrick Stolee, Josh Steadmon,
Emily Shaffer, Ramsay Jones, Derrick Stolee, Derrick Stolee
On Fri, Apr 16, 2021 at 12:49:59PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> If a remote has the skipFetchAll setting enabled, then that remote is
> not intended for frequent fetching. It makes sense to not fetch that
> data during the 'prefetch' maintenance task. Skip that remote in the
> iteration without error. The skip_default_update member is initialized
> in remote.c:handle_config() as part of initializing the 'struct remote'.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Reviewed-by: Tom Saeger <tom.saeger@oracle.com>
> ---
> builtin/gc.c | 3 +++
> t/t7900-maintenance.sh | 8 +++++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 9d35f7da50d8..98a803196b88 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -878,6 +878,9 @@ static int fetch_remote(struct remote *remote, void *cbdata)
> struct maintenance_run_opts *opts = cbdata;
> struct child_process child = CHILD_PROCESS_INIT;
>
> + if (remote->skip_default_update)
> + return 0;
> +
Well that is way easier than doing it in 'fetch'.
> child.git_cmd = 1;
> strvec_pushl(&child.args, "fetch", remote->name,
> "--prefetch", "--prune", "--no-tags",
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index eadb800c08cc..b93ae014ee58 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -153,7 +153,13 @@ test_expect_success 'prefetch multiple remotes' '
>
> test_cmp_config refs/prefetch/ log.excludedecoration &&
> git log --oneline --decorate --all >log &&
> - ! grep "prefetch" log
> + ! grep "prefetch" log &&
> +
> + test_when_finished git config --unset remote.remote1.skipFetchAll &&
> + git config remote.remote1.skipFetchAll true &&
> + GIT_TRACE2_EVENT="$(pwd)/skip-remote1.txt" git maintenance run --task=prefetch 2>/dev/null &&
> + test_subcommand ! git fetch remote1 $fetchargs <skip-remote1.txt &&
> + test_subcommand git fetch remote2 $fetchargs <skip-remote1.txt
> '
>
> test_expect_success 'prefetch and existing log.excludeDecoration values' '
> --
> gitgitgadget
^ permalink raw reply [flat|nested] 72+ messages in thread