* [PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head @ 2020-09-02 19:02 Jonathan Tan 2020-09-02 20:07 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Tan @ 2020-09-02 19:02 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, derrickstolee 887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update", 2020-08-18) introduced the ability to disable writing to FETCH_HEAD during fetch, but did not suppress the "<source> -> FETCH_HEAD" message when this ability is used. This message is misleading in this case, because FETCH_HEAD is not written. Also, because "fetch" is used to lazy-fetch missing objects in a partial clone, this significantly clutters up the output in that case since the objects to be fetched are potentially numerous. Therefore, suppress this message when --no-write-fetch-head is passed. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- This is on origin/jt/lazy-fetch. This might be important for Stolee's maintenance prefetch patch [1] too - presumably we don't want to show FETCH_HEAD there, as it would be misleading and would clutter in the same way (albeit to a lesser extent). [1] https://lore.kernel.org/git/da64c51a8182ec13aeed8f0157079fb29a09ee85.1598380599.git.gitgitgadget@gmail.com/ --- builtin/fetch.c | 3 ++- t/t0410-partial-clone.sh | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 320ba9471d..442df05f5a 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1023,11 +1023,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, rc |= update_local_ref(ref, what, rm, ¬e, summary_width); free(ref); - } else + } else if (write_fetch_head) { format_display(¬e, '*', *kind ? kind : "branch", NULL, *what ? what : "HEAD", "FETCH_HEAD", summary_width); + } if (note.len) { if (verbosity >= 0 && !shown_url) { fprintf(stderr, _("From %.*s\n"), diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index d681e90640..584a039b85 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' ' ' test_expect_success 'fetching of missing objects' ' - rm -rf repo && + rm -rf repo err && test_create_repo server && test_commit -C server foo && git -C server repack -a -d --write-bitmap-index && @@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' ' git -C repo config core.repositoryformatversion 1 && git -C repo config extensions.partialclone "origin" && - git -C repo cat-file -p "$HASH" && + git -C repo cat-file -p "$HASH" 2>err && + + # Ensure that no spurious FETCH_HEAD messages are written + ! grep FETCH_HEAD err && # Ensure that the .promisor file is written, and check that its # associated packfile contains the object -- 2.28.0.402.g5ffc5be6b7-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head 2020-09-02 19:02 [PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head Jonathan Tan @ 2020-09-02 20:07 ` Junio C Hamano 2020-09-02 21:05 ` [PATCH v2] " Jonathan Tan 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2020-09-02 20:07 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, derrickstolee Jonathan Tan <jonathantanmy@google.com> writes: > 887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update", > 2020-08-18) introduced the ability to disable writing to FETCH_HEAD > during fetch, but did not suppress the "<source> -> FETCH_HEAD" message > when this ability is used. This message is misleading in this case, > because FETCH_HEAD is not written. Thanks for noticing, but I wonder if we should keep this for users of dry-run, which tends to give more output to what _would_ have been done if it were not dry-run? > This might be important for Stolee's maintenance prefetch patch [1] too > - presumably we don't want to show FETCH_HEAD there, as it would be > misleading and would clutter in the same way (albeit to a lesser > extent). Yes, that makes sense. > Also, because "fetch" is used to > lazy-fetch missing objects in a partial clone, this significantly > clutters up the output in that case since the objects to be fetched are > potentially numerous. Therefore, suppress this message when > --no-write-fetch-head is passed. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > This is on origin/jt/lazy-fetch. I think this patch, with possible correction for "we are not writing FETCH_HEAD but we do want to show what would have been done when the user asked --dry-run", should be done as a brown-paper-bag bugfix directly on jc/no-update-fetch-head topic and merged quicly down to 'master', instead of taken hostage of some other topic in flight, but it would soon already be a week since jt/lazy-fetch was merged to 'next', so it probably is OK to apply on jt/lazy-fetch, and it indeed is easier to manage that way. > [1] https://lore.kernel.org/git/da64c51a8182ec13aeed8f0157079fb29a09ee85.1598380599.git.gitgitgadget@gmail.com/ > --- > builtin/fetch.c | 3 ++- > t/t0410-partial-clone.sh | 7 +++++-- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 320ba9471d..442df05f5a 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1023,11 +1023,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, > rc |= update_local_ref(ref, what, rm, ¬e, > summary_width); > free(ref); > - } else > + } else if (write_fetch_head) { > format_display(¬e, '*', > *kind ? kind : "branch", NULL, > *what ? what : "HEAD", > "FETCH_HEAD", summary_width); > + } > if (note.len) { > if (verbosity >= 0 && !shown_url) { > fprintf(stderr, _("From %.*s\n"), > diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh > index d681e90640..584a039b85 100755 > --- a/t/t0410-partial-clone.sh > +++ b/t/t0410-partial-clone.sh > @@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' ' > ' > > test_expect_success 'fetching of missing objects' ' > - rm -rf repo && > + rm -rf repo err && > test_create_repo server && > test_commit -C server foo && > git -C server repack -a -d --write-bitmap-index && > @@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' ' > > git -C repo config core.repositoryformatversion 1 && > git -C repo config extensions.partialclone "origin" && > - git -C repo cat-file -p "$HASH" && > + git -C repo cat-file -p "$HASH" 2>err && > + > + # Ensure that no spurious FETCH_HEAD messages are written > + ! grep FETCH_HEAD err && Test also --dry-run, but that perhaps needs to be done outside the context of partial-clone. The above "lazy fetching should be silent and should not bother users with mention of FETCH_HEAD" is good test in the context of partial-clone, though. jc/no-update-fetch-head added its own test to t/t5510, and both the "output lacks FETCH_HEAD when --no-write-fetch-head is given" test and the "output still mentions FETCH_HEAD with --dry-run" test belong there. Thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head 2020-09-02 20:07 ` Junio C Hamano @ 2020-09-02 21:05 ` Jonathan Tan 2020-09-02 21:27 ` Junio C Hamano 2020-09-02 23:56 ` Jonathan Nieder 0 siblings, 2 replies; 9+ messages in thread From: Jonathan Tan @ 2020-09-02 21:05 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, gitster 887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update", 2020-08-18) introduced the ability to disable writing to FETCH_HEAD during fetch, but did not suppress the "<source> -> FETCH_HEAD" message when this ability is used. This message is misleading in this case, because FETCH_HEAD is not written. Also, because "fetch" is used to lazy-fetch missing objects in a partial clone, this significantly clutters up the output in that case since the objects to be fetched are potentially numerous. Therefore, suppress this message when --no-write-fetch-head is passed (but not when --dry-run is set). Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- This is on origin/jt/lazy-fetch. Junio writes [1]: > Test also --dry-run, but that perhaps needs to be done outside the > context of partial-clone. The above "lazy fetching should be silent > and should not bother users with mention of FETCH_HEAD" is good test > in the context of partial-clone, though. > > jc/no-update-fetch-head added its own test to t/t5510, and both the > "output lacks FETCH_HEAD when --no-write-fetch-head is given" test > and the "output still mentions FETCH_HEAD with --dry-run" test > belong there. Ah, thanks for catching that. Here's an updated version. [1] https://lore.kernel.org/git/xmqq7dtcaqob.fsf@gitster.c.googlers.com/ --- builtin/fetch.c | 8 +++++++- t/t0410-partial-clone.sh | 7 +++++-- t/t5510-fetch.sh | 18 ++++++++++-------- 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 320ba9471d..c6c4689250 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, rc |= update_local_ref(ref, what, rm, ¬e, summary_width); free(ref); - } else + } else if (write_fetch_head || dry_run) { + /* + * Display fetches written to FETCH_HEAD (or + * would be written to FETCH_HEAD, if --dry-run + * is set). + */ format_display(¬e, '*', *kind ? kind : "branch", NULL, *what ? what : "HEAD", "FETCH_HEAD", summary_width); + } if (note.len) { if (verbosity >= 0 && !shown_url) { fprintf(stderr, _("From %.*s\n"), diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index d681e90640..584a039b85 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' ' ' test_expect_success 'fetching of missing objects' ' - rm -rf repo && + rm -rf repo err && test_create_repo server && test_commit -C server foo && git -C server repack -a -d --write-bitmap-index && @@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' ' git -C repo config core.repositoryformatversion 1 && git -C repo config extensions.partialclone "origin" && - git -C repo cat-file -p "$HASH" && + git -C repo cat-file -p "$HASH" 2>err && + + # Ensure that no spurious FETCH_HEAD messages are written + ! grep FETCH_HEAD err && # Ensure that the .promisor file is written, and check that its # associated packfile contains the object diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 2a1abe91f0..759aec9305 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -543,16 +543,18 @@ test_expect_success 'fetch into the current branch with --update-head-ok' ' ' -test_expect_success 'fetch --dry-run does not touch FETCH_HEAD' ' - rm -f .git/FETCH_HEAD && - git fetch --dry-run . && - ! test -f .git/FETCH_HEAD +test_expect_success 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' ' + rm -f .git/FETCH_HEAD err && + git fetch --dry-run . 2>err && + ! test -f .git/FETCH_HEAD && + grep FETCH_HEAD err ' -test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD' ' - rm -f .git/FETCH_HEAD && - git fetch --no-write-fetch-head . && - ! test -f .git/FETCH_HEAD +test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' ' + rm -f .git/FETCH_HEAD err && + git fetch --no-write-fetch-head . 2>err && + ! test -f .git/FETCH_HEAD && + ! grep FETCH_HEAD err ' test_expect_success '--write-fetch-head gets defeated by --dry-run' ' -- 2.28.0.402.g5ffc5be6b7-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head 2020-09-02 21:05 ` [PATCH v2] " Jonathan Tan @ 2020-09-02 21:27 ` Junio C Hamano 2020-09-02 23:56 ` Jonathan Nieder 1 sibling, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2020-09-02 21:27 UTC (permalink / raw) To: Jonathan Tan; +Cc: git Thanks; this round looks good. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head 2020-09-02 21:05 ` [PATCH v2] " Jonathan Tan 2020-09-02 21:27 ` Junio C Hamano @ 2020-09-02 23:56 ` Jonathan Nieder 2020-09-03 2:03 ` Junio C Hamano 1 sibling, 1 reply; 9+ messages in thread From: Jonathan Nieder @ 2020-09-02 23:56 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, gitster Jonathan Tan wrote: > 887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update", > 2020-08-18) introduced the ability to disable writing to FETCH_HEAD > during fetch, but did not suppress the "<source> -> FETCH_HEAD" message > when this ability is used. This message is misleading in this case, > because FETCH_HEAD is not written. Also, because "fetch" is used to > lazy-fetch missing objects in a partial clone, this significantly > clutters up the output in that case since the objects to be fetched are > potentially numerous. > > Therefore, suppress this message when --no-write-fetch-head is passed > (but not when --dry-run is set). > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- [...] > builtin/fetch.c | 8 +++++++- > t/t0410-partial-clone.sh | 7 +++++-- > t/t5510-fetch.sh | 18 ++++++++++-------- > 3 files changed, 22 insertions(+), 11 deletions(-) Thanks for fixing it, and sorry I didn't catch it during initial review. > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 320ba9471d..c6c4689250 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, > rc |= update_local_ref(ref, what, rm, ¬e, > summary_width); > free(ref); > - } else > + } else if (write_fetch_head || dry_run) { > + /* > + * Display fetches written to FETCH_HEAD (or > + * would be written to FETCH_HEAD, if --dry-run nit: to fix the parallel construction, s/would/that would/ or s/written/that were written/ > + * is set). > + */ > format_display(¬e, '*', > *kind ? kind : "branch", NULL, > *what ? what : "HEAD", > "FETCH_HEAD", summary_width); > + } > if (note.len) { > if (verbosity >= 0 && !shown_url) { > fprintf(stderr, _("From %.*s\n"), > diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh > index d681e90640..584a039b85 100755 > --- a/t/t0410-partial-clone.sh > +++ b/t/t0410-partial-clone.sh > @@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' ' > ' > > test_expect_success 'fetching of missing objects' ' > - rm -rf repo && > + rm -rf repo err && > test_create_repo server && > test_commit -C server foo && > git -C server repack -a -d --write-bitmap-index && > @@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' ' > > git -C repo config core.repositoryformatversion 1 && > git -C repo config extensions.partialclone "origin" && > - git -C repo cat-file -p "$HASH" && > + git -C repo cat-file -p "$HASH" 2>err && > + > + # Ensure that no spurious FETCH_HEAD messages are written > + ! grep FETCH_HEAD err && This test feels very specific to the bug being addressed. That said, since we *also* have a test below for the generic behavior I don't mind. > > # Ensure that the .promisor file is written, and check that its > # associated packfile contains the object > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index 2a1abe91f0..759aec9305 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -543,16 +543,18 @@ test_expect_success 'fetch into the current branch with --update-head-ok' ' > > ' > > -test_expect_success 'fetch --dry-run does not touch FETCH_HEAD' ' > - rm -f .git/FETCH_HEAD && > - git fetch --dry-run . && > - ! test -f .git/FETCH_HEAD > +test_expect_success 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' ' > + rm -f .git/FETCH_HEAD err && > + git fetch --dry-run . 2>err && > + ! test -f .git/FETCH_HEAD && > + grep FETCH_HEAD err > ' > > -test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD' ' > - rm -f .git/FETCH_HEAD && > - git fetch --no-write-fetch-head . && > - ! test -f .git/FETCH_HEAD > +test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' ' > + rm -f .git/FETCH_HEAD err && > + git fetch --no-write-fetch-head . 2>err && > + ! test -f .git/FETCH_HEAD && > + ! grep FETCH_HEAD err Nice. With or without the following squashed in, Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> diff --git i/builtin/fetch.c w/builtin/fetch.c index c6c46892503..387eeb1ec08 100644 --- i/builtin/fetch.c +++ w/builtin/fetch.c @@ -1025,9 +1025,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, free(ref); } else if (write_fetch_head || dry_run) { /* - * Display fetches written to FETCH_HEAD (or - * would be written to FETCH_HEAD, if --dry-run - * is set). + * Display fetches that wrote to FETCH_HEAD (or, + * if --dry-run is set, that would write to + * FETCH_HEAD). */ format_display(¬e, '*', *kind ? kind : "branch", NULL, ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fetch: no FETCH_HEAD display if --no-write-fetch-head 2020-09-02 23:56 ` Jonathan Nieder @ 2020-09-03 2:03 ` Junio C Hamano 2020-09-03 19:41 ` [PATCH v3] " Jonathan Tan 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2020-09-03 2:03 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jonathan Tan, git Jonathan Nieder <jrnieder@gmail.com> writes: >> builtin/fetch.c | 8 +++++++- >> t/t0410-partial-clone.sh | 7 +++++-- >> t/t5510-fetch.sh | 18 ++++++++++-------- >> 3 files changed, 22 insertions(+), 11 deletions(-) > > Thanks for fixing it, and sorry I didn't catch it during initial > review. > ... >> diff --git a/builtin/fetch.c b/builtin/fetch.c >> index 320ba9471d..c6c4689250 100644 >> --- a/builtin/fetch.c >> +++ b/builtin/fetch.c >> @@ -1023,11 +1023,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, >> rc |= update_local_ref(ref, what, rm, ¬e, >> summary_width); >> free(ref); >> - } else >> + } else if (write_fetch_head || dry_run) { >> + /* >> + * Display fetches written to FETCH_HEAD (or >> + * would be written to FETCH_HEAD, if --dry-run > > nit: to fix the parallel construction, s/would/that would/ or > s/written/that were written/ True. >> + * is set). >> + */ >> format_display(¬e, '*', >> *kind ? kind : "branch", NULL, >> *what ? what : "HEAD", >> "FETCH_HEAD", summary_width); >> + } Strictly speaking, I suspect that this is still broken when the user says "fetch --no-write-fetch-head --dry-run" in which case we should skip this block. And to fix it properly, we would probably need to keep track of three things semi-independently. - were we told this is a "dry-run"? (current 'dry_run' variable) - were we told not to store fetch-head? (missing) - after all, are we going to write or not write fetch-head (current 'write_fetch_head' variable) And the conditional to protect this block would be fixed to use only the second and new "have we seen --no-fetch-head on the command line?" variable, and ignore the settings of the dry_run variable, I think. Thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head 2020-09-03 2:03 ` Junio C Hamano @ 2020-09-03 19:41 ` Jonathan Tan 2020-09-03 21:00 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Tan @ 2020-09-03 19:41 UTC (permalink / raw) To: git; +Cc: Jonathan Tan, gitster, jrnieder 887952b8c6 ("fetch: optionally allow disabling FETCH_HEAD update", 2020-08-18) introduced the ability to disable writing to FETCH_HEAD during fetch, but did not suppress the "<source> -> FETCH_HEAD" message when this ability is used. This message is misleading in this case, because FETCH_HEAD is not written. Also, because "fetch" is used to lazy-fetch missing objects in a partial clone, this significantly clutters up the output in that case since the objects to be fetched are potentially numerous. Therefore, suppress this message when --no-write-fetch-head is passed. In order to do this, a new variable that keeps track of what the user said explicitly about FETCH_HEAD (which may or may not have the same value as the existing one tracking whether FETCH_HEAD is written) is introduced. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- This is on origin/jt/lazy-fetch. OK - updated the code, added a test for the "--dry-run --no-write-fetch-head" case, and updated commit message and code comment. --- builtin/fetch.c | 21 +++++++++++++++++---- t/t0410-partial-clone.sh | 7 +++++-- t/t5510-fetch.sh | 25 +++++++++++++++++-------- 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 320ba9471d..9addd1f2d4 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -56,6 +56,7 @@ static int prune_tags = -1; /* unspecified */ #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */ static int all, append, dry_run, force, keep, multiple, update_head_ok; +static int user_specified_write_fetch_head = 1; static int write_fetch_head = 1; static int verbosity, deepen_relative, set_upstream; static int progress = -1; @@ -164,7 +165,7 @@ static struct option builtin_fetch_options[] = { PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules), OPT_BOOL(0, "dry-run", &dry_run, N_("dry run")), - OPT_BOOL(0, "write-fetch-head", &write_fetch_head, + OPT_BOOL(0, "write-fetch-head", &user_specified_write_fetch_head, N_("write fetched references to the FETCH_HEAD file")), OPT_BOOL('k', "keep", &keep, N_("keep downloaded pack")), OPT_BOOL('u', "update-head-ok", &update_head_ok, @@ -1023,11 +1024,20 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, rc |= update_local_ref(ref, what, rm, ¬e, summary_width); free(ref); - } else + } else if (user_specified_write_fetch_head) { + /* + * If the user specified --write-fetch-head + * (or, equivalently, did not specify + * --no-write-fetch-head), inform the user that + * this ref was written to FETCH_HEAD (or, if + * --dry-run was specified, would have been + * written). + */ format_display(¬e, '*', *kind ? kind : "branch", NULL, *what ? what : "HEAD", "FETCH_HEAD", summary_width); + } if (note.len) { if (verbosity >= 0 && !shown_url) { fprintf(stderr, _("From %.*s\n"), @@ -1822,8 +1832,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) if (depth || deepen_since || deepen_not.nr) deepen = 1; - /* FETCH_HEAD never gets updated in --dry-run mode */ - if (dry_run) + /* + * FETCH_HEAD never gets updated in --dry-run mode, nor if user passed + * --no-write-fetch-head + */ + if (dry_run || !user_specified_write_fetch_head) write_fetch_head = 0; if (all) { diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index d681e90640..584a039b85 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -183,7 +183,7 @@ test_expect_success 'missing CLI object, but promised, passes fsck' ' ' test_expect_success 'fetching of missing objects' ' - rm -rf repo && + rm -rf repo err && test_create_repo server && test_commit -C server foo && git -C server repack -a -d --write-bitmap-index && @@ -194,7 +194,10 @@ test_expect_success 'fetching of missing objects' ' git -C repo config core.repositoryformatversion 1 && git -C repo config extensions.partialclone "origin" && - git -C repo cat-file -p "$HASH" && + git -C repo cat-file -p "$HASH" 2>err && + + # Ensure that no spurious FETCH_HEAD messages are written + ! grep FETCH_HEAD err && # Ensure that the .promisor file is written, and check that its # associated packfile contains the object diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 2a1abe91f0..83b2504519 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -543,16 +543,25 @@ test_expect_success 'fetch into the current branch with --update-head-ok' ' ' -test_expect_success 'fetch --dry-run does not touch FETCH_HEAD' ' - rm -f .git/FETCH_HEAD && - git fetch --dry-run . && - ! test -f .git/FETCH_HEAD +test_expect_success 'fetch --dry-run does not touch FETCH_HEAD, but still prints what would be written' ' + rm -f .git/FETCH_HEAD err && + git fetch --dry-run . 2>err && + ! test -f .git/FETCH_HEAD && + grep FETCH_HEAD err ' -test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD' ' - rm -f .git/FETCH_HEAD && - git fetch --no-write-fetch-head . && - ! test -f .git/FETCH_HEAD +test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does not print what would be written' ' + rm -f .git/FETCH_HEAD err && + git fetch --no-write-fetch-head . 2>err && + ! test -f .git/FETCH_HEAD && + ! grep FETCH_HEAD err +' + +test_expect_success '--no-write-fetch-head and --dry-run does not touch FETCH_HEAD, and does not print what would be written' ' + rm -f .git/FETCH_HEAD err && + git fetch --dry-run --no-write-fetch-head . 2>err && + ! test -f .git/FETCH_HEAD && + ! grep FETCH_HEAD err ' test_expect_success '--write-fetch-head gets defeated by --dry-run' ' -- 2.28.0.526.ge36021eeef-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head 2020-09-03 19:41 ` [PATCH v3] " Jonathan Tan @ 2020-09-03 21:00 ` Junio C Hamano 2020-09-03 21:06 ` Jonathan Tan 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2020-09-03 21:00 UTC (permalink / raw) To: Jonathan Tan; +Cc: git, jrnieder Jonathan Tan <jonathantanmy@google.com> writes: > OK - updated the code, added a test for the "--dry-run > --no-write-fetch-head" case, and updated commit message and code > comment. Unfortunately our actions crossed X-< and the previous one that was good enough is already in 'master', together with the lazy fetch topic. Let's turn this into an incremental fix only for "Ouch, we still say FETCH_HEAD when both --dry-run and --no-write-fetch-head are given" bug. Thanks. -- >8 -- Subject: [PATCH] fetch: fix --dry-run --no-write-fetch-head interaction From: Jonathan Tan <jonathantanmy@google.com> Recently we introduced "--[no-]-write-fetch-head" option to tell "git fetch" not to write FETCH_HEAD file. The command reported that FETCH_HEAD file is written, even with the "--no-write-fetch-head" option. db3c293e (fetch: no FETCH_HEAD display if --no-write-fetch-head, 2020-09-02) tried to squelch this, but the fix was not sufficient. Because we never write the FETCH_HEAD file when "--dry-run" is given, the addition of "--[no-]write-fetch-head" option was made by directly fliping the internal variable 'write_fetch_head' (which defaults to 'on') to 'off' upon seeing "--dry-run", which allowed the condition to decide if we write FETCH_HEAD to be a simple reference to the variable. But now, we need to tell if the user explicitly asked "--no-write-fetch-head" with "--dry-run" to decide when to show the report about FETCH_HEAD correctly. Introduce an extra 'user_specified_write_fetch_head' variable, which is 'on' by default and is turned 'off' with '--no-write-fetch-head'. The 'write_fetch_head' variable that decides if we actually write FETCH_HEAD remains there, retaining its meaning, but use this new variable and 'dry_run' to decide if we report about FETCH_HEAD. Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- builtin/fetch.c | 21 ++++++++++++++------- t/t5510-fetch.sh | 7 +++++++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git c/builtin/fetch.c w/builtin/fetch.c index c6c4689250..9addd1f2d4 100644 --- c/builtin/fetch.c +++ w/builtin/fetch.c @@ -56,6 +56,7 @@ static int prune_tags = -1; /* unspecified */ #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */ static int all, append, dry_run, force, keep, multiple, update_head_ok; +static int user_specified_write_fetch_head = 1; static int write_fetch_head = 1; static int verbosity, deepen_relative, set_upstream; static int progress = -1; @@ -164,7 +165,7 @@ static struct option builtin_fetch_options[] = { PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules), OPT_BOOL(0, "dry-run", &dry_run, N_("dry run")), - OPT_BOOL(0, "write-fetch-head", &write_fetch_head, + OPT_BOOL(0, "write-fetch-head", &user_specified_write_fetch_head, N_("write fetched references to the FETCH_HEAD file")), OPT_BOOL('k', "keep", &keep, N_("keep downloaded pack")), OPT_BOOL('u', "update-head-ok", &update_head_ok, @@ -1023,11 +1024,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, rc |= update_local_ref(ref, what, rm, ¬e, summary_width); free(ref); - } else if (write_fetch_head || dry_run) { + } else if (user_specified_write_fetch_head) { /* - * Display fetches written to FETCH_HEAD (or - * would be written to FETCH_HEAD, if --dry-run - * is set). + * If the user specified --write-fetch-head + * (or, equivalently, did not specify + * --no-write-fetch-head), inform the user that + * this ref was written to FETCH_HEAD (or, if + * --dry-run was specified, would have been + * written). */ format_display(¬e, '*', *kind ? kind : "branch", NULL, @@ -1828,8 +1832,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) if (depth || deepen_since || deepen_not.nr) deepen = 1; - /* FETCH_HEAD never gets updated in --dry-run mode */ - if (dry_run) + /* + * FETCH_HEAD never gets updated in --dry-run mode, nor if user passed + * --no-write-fetch-head + */ + if (dry_run || !user_specified_write_fetch_head) write_fetch_head = 0; if (all) { diff --git c/t/t5510-fetch.sh w/t/t5510-fetch.sh index 759aec9305..83b2504519 100755 --- c/t/t5510-fetch.sh +++ w/t/t5510-fetch.sh @@ -557,6 +557,13 @@ test_expect_success '--no-write-fetch-head does not touch FETCH_HEAD, and does n ! grep FETCH_HEAD err ' +test_expect_success '--no-write-fetch-head and --dry-run does not touch FETCH_HEAD, and does not print what would be written' ' + rm -f .git/FETCH_HEAD err && + git fetch --dry-run --no-write-fetch-head . 2>err && + ! test -f .git/FETCH_HEAD && + ! grep FETCH_HEAD err +' + test_expect_success '--write-fetch-head gets defeated by --dry-run' ' rm -f .git/FETCH_HEAD && git fetch --dry-run --write-fetch-head . && ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] fetch: no FETCH_HEAD display if --no-write-fetch-head 2020-09-03 21:00 ` Junio C Hamano @ 2020-09-03 21:06 ` Jonathan Tan 0 siblings, 0 replies; 9+ messages in thread From: Jonathan Tan @ 2020-09-03 21:06 UTC (permalink / raw) To: gitster; +Cc: jonathantanmy, git, jrnieder > Jonathan Tan <jonathantanmy@google.com> writes: > > > OK - updated the code, added a test for the "--dry-run > > --no-write-fetch-head" case, and updated commit message and code > > comment. > > Unfortunately our actions crossed X-< and the previous one that was > good enough is already in 'master', together with the lazy fetch > topic. > > Let's turn this into an incremental fix only for "Ouch, we still say > FETCH_HEAD when both --dry-run and --no-write-fetch-head are given" > bug. > > Thanks. > > -- >8 -- > Subject: [PATCH] fetch: fix --dry-run --no-write-fetch-head interaction > From: Jonathan Tan <jonathantanmy@google.com> > > Recently we introduced "--[no-]-write-fetch-head" option to tell > "git fetch" not to write FETCH_HEAD file. The command reported that > FETCH_HEAD file is written, even with the "--no-write-fetch-head" > option. > > db3c293e (fetch: no FETCH_HEAD display if --no-write-fetch-head, > 2020-09-02) tried to squelch this, but the fix was not sufficient. > Because we never write the FETCH_HEAD file when "--dry-run" is > given, the addition of "--[no-]write-fetch-head" option was made by > directly fliping the internal variable 'write_fetch_head' (which > defaults to 'on') to 'off' upon seeing "--dry-run", which allowed > the condition to decide if we write FETCH_HEAD to be a simple > reference to the variable. But now, we need to tell if the user > explicitly asked "--no-write-fetch-head" with "--dry-run" to > decide when to show the report about FETCH_HEAD correctly. > > Introduce an extra 'user_specified_write_fetch_head' variable, which > is 'on' by default and is turned 'off' with '--no-write-fetch-head'. > The 'write_fetch_head' variable that decides if we actually write > FETCH_HEAD remains there, retaining its meaning, but use this new > variable and 'dry_run' to decide if we report about FETCH_HEAD. > > Suggested-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- Thanks - this looks good. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-09-03 21:06 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-02 19:02 [PATCH] fetch: no FETCH_HEAD display if --no-write-fetch-head Jonathan Tan 2020-09-02 20:07 ` Junio C Hamano 2020-09-02 21:05 ` [PATCH v2] " Jonathan Tan 2020-09-02 21:27 ` Junio C Hamano 2020-09-02 23:56 ` Jonathan Nieder 2020-09-03 2:03 ` Junio C Hamano 2020-09-03 19:41 ` [PATCH v3] " Jonathan Tan 2020-09-03 21:00 ` Junio C Hamano 2020-09-03 21:06 ` Jonathan Tan
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).