| Message ID | 20200902210539.1981453-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] fetch: no FETCH_HEAD display if --no-write-fetch-head | expand |
Thanks; this round looks good.
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,
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.
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' '
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(-)