| Message ID | 20200903194148.2738968-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] fetch: no FETCH_HEAD display if --no-write-fetch-head | expand |
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 . &&
> 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.
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' '
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(-)