| Message ID | 20200902190232.317477-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: no FETCH_HEAD display if --no-write-fetch-head | expand |
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.
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
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(-)