| Message ID | 20241203215713.135068-2-bence@ferdinandy.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | set_head finishing touches | expand |
Bence Ferdinandy <bence@ferdinandy.com> writes: > +static void set_head_advice_msg(const char *remote, const char *head_name) { > + > + printf("Run 'git remote set-head %s %s' to follow the change, or\n" > + "'git config set remote.%s.warn-if-not-%s' to disable this warning\n" > + "until the remote changes HEAD again.\n", > + remote, head_name, remote, head_name); > + > +} Style. "{" that encloses the function body sits on a line of its own. Perhaps use the advise_if_enabled(), so that those who already learned how to deal with the situation can squelch the "how to fix" message. > -static int set_head(const struct ref *remote_refs, int follow_remote_head) > +static int set_head(const struct ref *remote_refs, int follow_remote_head, > + const char *no_warn_branch) > { > int result = 0, create_only, is_bare, was_detached; > struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT, > @@ -1660,7 +1668,10 @@ static int set_head(const struct ref *remote_refs, int follow_remote_head) > result = 1; > goto cleanup; > } > - if (follow_remote_head == FOLLOW_REMOTE_WARN && verbosity >= 0) > + if ((follow_remote_head == FOLLOW_REMOTE_WARN || > + (follow_remote_head == FOLLOW_REMOTE_WARN_IF_NOT_BRANCH && > + strcmp(no_warn_branch, head_name)) > + ) && verbosity >= 0) Reorder conditions combined with &&- to have more expensive ones later. if (verbosity >= 0 && (follow_remote_head == FOLLOW_REMOTE_WARN || (follow_remote_head == FOLLOW_REMOTE_WARN_IF_NOT_BRANCH && strcmp(no_warn_branch, head_name))) As human readers, we may know that no_warn_branch is always valid when (follow_remote_head == FOLLOW_REMOTE_WARN_IF_NOT_BRANCH), but semi clever compilers may not realize it and give a false warning about using no_warn_branch potentially uninitialized. We could do without adding FOLLOW_REMOTE_WARN_IF_NOT_BRANCH and reuse FOLLOW_REMOTE_WARN, like so: if (verbosity >= 0 && follow_remote_head == FOLLOW_REMOTE_WARN && (!no_warn_branch || strcmp(no_warn_branch, head_name))) That is, "if set to remote-warn, always warn, or no_warn_branch is not NULL, only warn if the current head is different from it". > diff --git a/remote.c b/remote.c > index 0b18840d43..f0e1b1b76a 100644 > --- a/remote.c > +++ b/remote.c > @@ -515,6 +515,7 @@ static int handle_config(const char *key, const char *value, > return parse_transport_option(key, value, > &remote->server_options); > } else if (!strcmp(subkey, "followremotehead")) { > + const char *no_warn_branch; > if (!strcmp(value, "never")) > remote->follow_remote_head = FOLLOW_REMOTE_NEVER; > else if (!strcmp(value, "create")) > @@ -523,6 +524,10 @@ static int handle_config(const char *key, const char *value, > remote->follow_remote_head = FOLLOW_REMOTE_WARN; > else if (!strcmp(value, "always")) > remote->follow_remote_head = FOLLOW_REMOTE_ALWAYS; > + else if (skip_prefix(value, "warn-if-not-", &no_warn_branch)) { > + remote->follow_remote_head = FOLLOW_REMOTE_WARN_IF_NOT_BRANCH; > + remote->no_warn_branch = no_warn_branch; > + } If we were to do without FOLLOW_REMOTE_WARN_IF_NOT_BRANCH, then the above becomes remote->follow_remote_head = FOLLOW_REMOTE_WARN; remote->no_warn_branch = NULL; } else if (skip_prefix(value, "warn-if-not-", &no_warn_branch)) { remote->follow_remote_head = FOLLOW_REMOTE_WARN; remote->no_warn_branch = no_warn_branch; } else if (!strcmp(value, "always")) { remote->follow_remote_head = FOLLOW_REMOTE_ALWAYS; } else { warn(_("unrecognized followRemoteHEAD value '%s' ignored"), value); } We'd want the new choice documented before we graduate this topic out of the RFC status. Thanks.
On Wed Dec 04, 2024 at 03:20, Junio C Hamano <gitster@pobox.com> wrote: > Bence Ferdinandy <bence@ferdinandy.com> writes: > >> +static void set_head_advice_msg(const char *remote, const char *head_name) { >> + >> + printf("Run 'git remote set-head %s %s' to follow the change, or\n" >> + "'git config set remote.%s.warn-if-not-%s' to disable this warning\n" >> + "until the remote changes HEAD again.\n", >> + remote, head_name, remote, head_name); >> + >> +} > > Style. "{" that encloses the function body sits on a line of its > own. > > Perhaps use the advise_if_enabled(), so that those who already > learned how to deal with the situation can squelch the "how to fix" > message. > >> -static int set_head(const struct ref *remote_refs, int follow_remote_head) >> +static int set_head(const struct ref *remote_refs, int follow_remote_head, >> + const char *no_warn_branch) >> { >> int result = 0, create_only, is_bare, was_detached; >> struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT, >> @@ -1660,7 +1668,10 @@ static int set_head(const struct ref *remote_refs, int follow_remote_head) >> result = 1; >> goto cleanup; >> } >> - if (follow_remote_head == FOLLOW_REMOTE_WARN && verbosity >= 0) >> + if ((follow_remote_head == FOLLOW_REMOTE_WARN || >> + (follow_remote_head == FOLLOW_REMOTE_WARN_IF_NOT_BRANCH && >> + strcmp(no_warn_branch, head_name)) >> + ) && verbosity >= 0) > > Reorder conditions combined with &&- to have more expensive ones > later. > > if (verbosity >= 0 && > (follow_remote_head == FOLLOW_REMOTE_WARN || > (follow_remote_head == FOLLOW_REMOTE_WARN_IF_NOT_BRANCH && > strcmp(no_warn_branch, head_name))) > > As human readers, we may know that no_warn_branch is always valid > when (follow_remote_head == FOLLOW_REMOTE_WARN_IF_NOT_BRANCH), but > semi clever compilers may not realize it and give a false warning > about using no_warn_branch potentially uninitialized. > > We could do without adding FOLLOW_REMOTE_WARN_IF_NOT_BRANCH and reuse > FOLLOW_REMOTE_WARN, like so: > > if (verbosity >= 0 && > follow_remote_head == FOLLOW_REMOTE_WARN && > (!no_warn_branch || strcmp(no_warn_branch, head_name))) > > That is, "if set to remote-warn, always warn, or no_warn_branch is > not NULL, only warn if the current head is different from it". Ah, nice, this has the added benefit of a bad configuration where "warn-if-not-" doesn't actually specify a branch to fall back to just "warn". > >> diff --git a/remote.c b/remote.c >> index 0b18840d43..f0e1b1b76a 100644 >> --- a/remote.c >> +++ b/remote.c >> @@ -515,6 +515,7 @@ static int handle_config(const char *key, const char *value, >> return parse_transport_option(key, value, >> &remote->server_options); >> } else if (!strcmp(subkey, "followremotehead")) { >> + const char *no_warn_branch; >> if (!strcmp(value, "never")) >> remote->follow_remote_head = FOLLOW_REMOTE_NEVER; >> else if (!strcmp(value, "create")) >> @@ -523,6 +524,10 @@ static int handle_config(const char *key, const char *value, >> remote->follow_remote_head = FOLLOW_REMOTE_WARN; >> else if (!strcmp(value, "always")) >> remote->follow_remote_head = FOLLOW_REMOTE_ALWAYS; >> + else if (skip_prefix(value, "warn-if-not-", &no_warn_branch)) { >> + remote->follow_remote_head = FOLLOW_REMOTE_WARN_IF_NOT_BRANCH; >> + remote->no_warn_branch = no_warn_branch; >> + } > > If we were to do without FOLLOW_REMOTE_WARN_IF_NOT_BRANCH, then the > above becomes > > remote->follow_remote_head = FOLLOW_REMOTE_WARN; > remote->no_warn_branch = NULL; > } else if (skip_prefix(value, "warn-if-not-", &no_warn_branch)) { > remote->follow_remote_head = FOLLOW_REMOTE_WARN; > remote->no_warn_branch = no_warn_branch; > } else if (!strcmp(value, "always")) { > remote->follow_remote_head = FOLLOW_REMOTE_ALWAYS; > } else { > warn(_("unrecognized followRemoteHEAD value '%s' ignored"), > value); > } > > We'd want the new choice documented before we graduate this topic > out of the RFC status. > > Thanks.
diff --git a/builtin/fetch.c b/builtin/fetch.c index 88c5c5d781..fd7f3694cc 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1579,6 +1579,15 @@ static const char *strip_refshead(const char *name){ return name; } +static void set_head_advice_msg(const char *remote, const char *head_name) { + + printf("Run 'git remote set-head %s %s' to follow the change, or\n" + "'git config set remote.%s.warn-if-not-%s' to disable this warning\n" + "until the remote changes HEAD again.\n", + remote, head_name, remote, head_name); + +} + static void report_set_head(const char *remote, const char *head_name, struct strbuf *buf_prev, int updateres) { struct strbuf buf_prefix = STRBUF_INIT; @@ -1590,20 +1599,19 @@ static void report_set_head(const char *remote, const char *head_name, if (prev_head && strcmp(prev_head, head_name)) { printf("'HEAD' at '%s' is '%s', but we have '%s' locally.\n", remote, head_name, prev_head); - printf("Run 'git remote set-head %s %s' to follow the change.\n", - remote, head_name); + set_head_advice_msg(remote, head_name); } else if (updateres && buf_prev->len) { printf("'HEAD' at '%s' is '%s', " "but we have a detached HEAD pointing to '%s' locally.\n", remote, head_name, buf_prev->buf); - printf("Run 'git remote set-head %s %s' to follow the change.\n", - remote, head_name); + set_head_advice_msg(remote, head_name); } strbuf_release(&buf_prefix); } -static int set_head(const struct ref *remote_refs, int follow_remote_head) +static int set_head(const struct ref *remote_refs, int follow_remote_head, + const char *no_warn_branch) { int result = 0, create_only, is_bare, was_detached; struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT, @@ -1660,7 +1668,10 @@ static int set_head(const struct ref *remote_refs, int follow_remote_head) result = 1; goto cleanup; } - if (follow_remote_head == FOLLOW_REMOTE_WARN && verbosity >= 0) + if ((follow_remote_head == FOLLOW_REMOTE_WARN || + (follow_remote_head == FOLLOW_REMOTE_WARN_IF_NOT_BRANCH && + strcmp(no_warn_branch, head_name)) + ) && verbosity >= 0) report_set_head(remote, head_name, &b_local_head, was_detached); cleanup: @@ -1889,7 +1900,8 @@ static int do_fetch(struct transport *transport, "you need to specify exactly one branch with the --set-upstream option")); } } - if (set_head(remote_refs, transport->remote->follow_remote_head)) + if (set_head(remote_refs, transport->remote->follow_remote_head, + transport->remote->no_warn_branch)) ; /* * Way too many cases where this can go wrong diff --git a/remote.c b/remote.c index 0b18840d43..f0e1b1b76a 100644 --- a/remote.c +++ b/remote.c @@ -515,6 +515,7 @@ static int handle_config(const char *key, const char *value, return parse_transport_option(key, value, &remote->server_options); } else if (!strcmp(subkey, "followremotehead")) { + const char *no_warn_branch; if (!strcmp(value, "never")) remote->follow_remote_head = FOLLOW_REMOTE_NEVER; else if (!strcmp(value, "create")) @@ -523,6 +524,10 @@ static int handle_config(const char *key, const char *value, remote->follow_remote_head = FOLLOW_REMOTE_WARN; else if (!strcmp(value, "always")) remote->follow_remote_head = FOLLOW_REMOTE_ALWAYS; + else if (skip_prefix(value, "warn-if-not-", &no_warn_branch)) { + remote->follow_remote_head = FOLLOW_REMOTE_WARN_IF_NOT_BRANCH; + remote->no_warn_branch = no_warn_branch; + } } return 0; } diff --git a/remote.h b/remote.h index 184b35653d..75be3977ba 100644 --- a/remote.h +++ b/remote.h @@ -62,8 +62,9 @@ struct remote_state *remote_state_new(void); enum follow_remote_head_settings { FOLLOW_REMOTE_NEVER = -1, FOLLOW_REMOTE_CREATE = 0, - FOLLOW_REMOTE_WARN = 1, - FOLLOW_REMOTE_ALWAYS = 2, + FOLLOW_REMOTE_WARN_IF_NOT_BRANCH = 1, + FOLLOW_REMOTE_WARN = 2, + FOLLOW_REMOTE_ALWAYS = 3, }; struct remote { @@ -116,6 +117,7 @@ struct remote { struct string_list server_options; enum follow_remote_head_settings follow_remote_head; + const char *no_warn_branch; }; /** diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 2467027d34..be0c60be2c 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -124,7 +124,9 @@ test_expect_success "fetch test followRemoteHEAD warn no change" ' git fetch >output && echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \ "but we have ${SQ}other${SQ} locally." >expect && - echo "Run ${SQ}git remote set-head origin main${SQ} to follow the change." >>expect && + echo "Run ${SQ}git remote set-head origin main${SQ} to follow the change, or" >>expect && + echo "${SQ}git config set remote.origin.warn-if-not-main${SQ} to disable this warning" >>expect && + echo "until the remote changes HEAD again." >>expect && test_cmp expect output && head=$(git rev-parse refs/remotes/origin/HEAD) && branch=$(git rev-parse refs/remotes/origin/other) && @@ -161,7 +163,9 @@ test_expect_success "fetch test followRemoteHEAD warn detached" ' echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \ "but we have a detached HEAD pointing to" \ "${SQ}${HEAD}${SQ} locally." >expect && - echo "Run ${SQ}git remote set-head origin main${SQ} to follow the change." >>expect && + echo "Run ${SQ}git remote set-head origin main${SQ} to follow the change, or" >>expect && + echo "${SQ}git config set remote.origin.warn-if-not-main${SQ} to disable this warning" >>expect && + echo "until the remote changes HEAD again." >>expect && test_cmp expect output ) ' @@ -184,6 +188,47 @@ test_expect_success "fetch test followRemoteHEAD warn quiet" ' ) ' +test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is same" ' + test_when_finished "git config unset remote.origin.followRemoteHEAD" && + ( + cd "$D" && + cd two && + git rev-parse --verify refs/remotes/origin/other && + git remote set-head origin other && + git rev-parse --verify refs/remotes/origin/HEAD && + git rev-parse --verify refs/remotes/origin/main && + git config set remote.origin.followRemoteHEAD "warn-if-not-main" && + output=$(git fetch) && + test "z" = "z$output" && + head=$(git rev-parse refs/remotes/origin/HEAD) && + branch=$(git rev-parse refs/remotes/origin/other) && + test "z$head" = "z$branch" + ) +' + +test_expect_success "fetch test followRemoteHEAD warn-if-not-branch branch is different" ' + test_when_finished "git config unset remote.origin.followRemoteHEAD" && + ( + cd "$D" && + cd two && + git rev-parse --verify refs/remotes/origin/other && + git remote set-head origin other && + git rev-parse --verify refs/remotes/origin/HEAD && + git rev-parse --verify refs/remotes/origin/main && + git config set remote.origin.followRemoteHEAD "warn-if-not-some/different-branch" && + git fetch >output && + echo "${SQ}HEAD${SQ} at ${SQ}origin${SQ} is ${SQ}main${SQ}," \ + "but we have ${SQ}other${SQ} locally." >expect && + echo "Run ${SQ}git remote set-head origin main${SQ} to follow the change, or" >>expect && + echo "${SQ}git config set remote.origin.warn-if-not-main${SQ} to disable this warning" >>expect && + echo "until the remote changes HEAD again." >>expect && + test_cmp expect output && + head=$(git rev-parse refs/remotes/origin/HEAD) && + branch=$(git rev-parse refs/remotes/origin/other) && + test "z$head" = "z$branch" + ) +' + test_expect_success "fetch test followRemoteHEAD always" ' test_when_finished "git config unset remote.origin.followRemoteHEAD" && (
Currently if we want to have a remote/HEAD locally that is different from the one on the remote, but we still want to get a warning if remote changes HEAD, our only option is to have an indiscriminate warning with "follow_remote_head" set to "warn". Add a new option "warn-if-not-$branch", where $branch is a branch name we do not wish to get a warning about. If the remote HEAD is $branch do not warn, otherwise, behave as "warn". E.g. let's assume, that our remote origin has HEAD set to "master", but locally we have "git remote set-head origin seen". Setting 'remote.origin.followRemoteHEAD = "warn"' will always print a warning, even though the remote has not changed HEAD from "master". Setting 'remote.origin.followRemoteHEAD = "warn-if-not-master" will squelch the warning message, unless the remote changes HEAD from "master". Note, that should the remote change HEAD to "seen" (which we have locally), there will still be no warning. Improve the advice message in report_set_head to also include silencing the warning message with "warn-if-not-$branch". Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> --- builtin/fetch.c | 26 ++++++++++++++++++------- remote.c | 5 +++++ remote.h | 6 ++++-- t/t5510-fetch.sh | 49 ++++++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 75 insertions(+), 11 deletions(-)