"git push --force-with-lease=<branch>:<expect>" makes sure that there is no unexpected changes to the branch at the remote while you prepare a rewrite based on the old state of the branch. This feature came with an experimental option that allows :<expect> part to be omitted by using the tip of remote-tracking branch that corresponds to the <branch>.
It turns out that some people use third-party tools that fetch from remote and update the remote-tracking branches behind users' back, defeating the safety relying on the stability of the remote-tracking branches. We have some warning text that was meant to be scary sounding in our documentation, but nevertheless people seem to be bitten. cf. https://public-inbox.org/git/1491617750.2149.10.ca...@mattmccutchen.net/ for a recent example. Let's disable the form that relies on the stability of remote-tracking branches by default, and allow users who _know_ their remote-tracking branches are stable to enable it with a configuration variable. This problem was predicted from the very beginning; see 28f5d176 (remote.c: add command line option parser for "--force-with-lease", 2013-07-08). Signed-off-by: Junio C Hamano <gits...@pobox.com> --- * This is a bit overdue safety fix that we should have done long time ago. If we had this, I do not think it makes it riskier to forbid --force and tell people to use --force-with-lease. Documentation/config.txt | 5 +++++ Documentation/git-push.txt | 5 +++-- builtin/send-pack.c | 5 +++++ remote.c | 16 ++++++++++++---- remote.h | 2 ++ send-pack.c | 1 + t/t5533-push-cas.sh | 19 +++++++++++++++++-- transport-helper.c | 5 +++++ transport.c | 5 +++++ 9 files changed, 55 insertions(+), 8 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 06898a7498..2f929315a2 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2588,6 +2588,11 @@ new default). -- +push.allowLazyForceWithLease:: + If set to true, allow the `--force-with-lease` option + without the expected object name (i.e. expecting the objects + at the tip of corresponding remote-tracking branches). + push.followTags:: If set to true enable `--follow-tags` option by default. You may override this configuration at time of push by specifying diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 0a639664fd..1fa01210a2 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -212,8 +212,9 @@ must not already exist. + Note that all forms other than `--force-with-lease=<refname>:<expect>` that specifies the expected current value of the ref explicitly are -still experimental and their semantics may change as we gain experience -with this feature. +disabled by default. Read the note on safety below, and if you think +you are not affected, enable it with the `push.allowLazyForceWithLease` +configuration option (cf. linkgit:git-config[1]). + "--no-force-with-lease" will cancel all the previous --force-with-lease on the command line. diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 633e0c3cdd..c008f5b60f 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -68,6 +68,11 @@ static void print_helper_status(struct ref *ref) msg = "stale info"; break; + case REF_STATUS_REJECT_LAZY_CAS: + res = "error"; + msg = "lazy force-with-error"; + break; + case REF_STATUS_REJECT_ALREADY_EXISTS: res = "error"; msg = "already exists"; diff --git a/remote.c b/remote.c index d87482573d..2d3ee6020f 100644 --- a/remote.c +++ b/remote.c @@ -1517,7 +1517,9 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, int force_update) { struct ref *ref; + int allow_lazy_cas = 0; + git_config_get_bool("push.allowLazyForceWithLease", &allow_lazy_cas); for (ref = remote_refs; ref; ref = ref->next) { int force_ref_update = ref->force || force_update; int reject_reason = 0; @@ -1544,7 +1546,9 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, * branch. */ if (ref->expect_old_sha1) { - if (oidcmp(&ref->old_oid, &ref->old_oid_expect)) + if (!allow_lazy_cas && ref->lazy_cas) + reject_reason = REF_STATUS_REJECT_LAZY_CAS; + else if (oidcmp(&ref->old_oid, &ref->old_oid_expect)) reject_reason = REF_STATUS_REJECT_STALE; else /* If the ref isn't stale then force the update. */ @@ -2341,10 +2345,13 @@ static void apply_cas(struct push_cas_option *cas, if (!refname_match(entry->refname, ref->name)) continue; ref->expect_old_sha1 = 1; - if (!entry->use_tracking) + if (!entry->use_tracking) { hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect); - else if (remote_tracking(remote, ref->name, &ref->old_oid_expect)) - oidclr(&ref->old_oid_expect); + } else { + ref->lazy_cas = 1; + if (remote_tracking(remote, ref->name, &ref->old_oid_expect)) + oidclr(&ref->old_oid_expect); + } return; } @@ -2353,6 +2360,7 @@ static void apply_cas(struct push_cas_option *cas, return; ref->expect_old_sha1 = 1; + ref->lazy_cas = 1; if (remote_tracking(remote, ref->name, &ref->old_oid_expect)) oidclr(&ref->old_oid_expect); } diff --git a/remote.h b/remote.h index 6c28cd3e4b..22190f4e91 100644 --- a/remote.h +++ b/remote.h @@ -89,6 +89,7 @@ struct ref { force:1, forced_update:1, expect_old_sha1:1, + lazy_cas:1, deletion:1; enum { @@ -118,6 +119,7 @@ struct ref { REF_STATUS_REJECT_FETCH_FIRST, REF_STATUS_REJECT_NEEDS_FORCE, REF_STATUS_REJECT_STALE, + REF_STATUS_REJECT_LAZY_CAS, REF_STATUS_REJECT_SHALLOW, REF_STATUS_UPTODATE, REF_STATUS_REMOTE_REJECT, diff --git a/send-pack.c b/send-pack.c index 11d6f3d983..94f0ad2b14 100644 --- a/send-pack.c +++ b/send-pack.c @@ -248,6 +248,7 @@ static int check_to_send_update(const struct ref *ref, const struct send_pack_ar case REF_STATUS_REJECT_FETCH_FIRST: case REF_STATUS_REJECT_NEEDS_FORCE: case REF_STATUS_REJECT_STALE: + case REF_STATUS_REJECT_LAZY_CAS: case REF_STATUS_REJECT_NODELETE: return CHECK_REF_STATUS_REJECTED; case REF_STATUS_UPTODATE: diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh index d38ecee217..0527832ff5 100755 --- a/t/t5533-push-cas.sh +++ b/t/t5533-push-cas.sh @@ -17,7 +17,8 @@ test_expect_success setup ' # create template repository test_commit A && test_commit B && - test_commit C + test_commit C && + git config --global push.allowLazyForceWithLease true ' test_expect_success 'push to update (protected)' ' @@ -78,7 +79,6 @@ test_expect_success 'push to update (protected, tracking, forced)' ' ( cd dst && test_commit E && - git ls-remote . refs/remotes/origin/master >expect && git push --force --force-with-lease=master origin master ) && git ls-remote dst refs/heads/master >expect && @@ -91,6 +91,7 @@ test_expect_success 'push to update (allowed)' ' ( cd dst && test_commit D && + git config push.allowLazyForceWithLease false && git push --force-with-lease=master:master^ origin master ) && git ls-remote dst refs/heads/master >expect && @@ -103,6 +104,10 @@ test_expect_success 'push to update (allowed, tracking)' ' ( cd dst && test_commit D && + git config push.allowLazyForceWithLease false && + test_must_fail git push --force-with-lease=master origin master 2>err && + grep "lazy force-with-lease" err && + git config --unset push.allowLazyForceWithLease && git push --force-with-lease=master origin master 2>err && ! grep "forced update" err ) && @@ -151,6 +156,10 @@ test_expect_success 'push to delete (allowed)' ' setup_srcdst_basic && ( cd dst && + git config push.allowLazyForceWithLease false && + test_must_fail git push --force-with-lease=master origin :master 2>err && + grep "lazy force-with-lease" err && + git config --unset push.allowLazyForceWithLease && git push --force-with-lease=master origin :master 2>err && grep deleted err ) && @@ -183,6 +192,9 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' ' ( cd dst && git fetch && + git config push.allowLazyForceWithLease false && + test_must_fail git push --force-with-lease origin master master:naster && + git config --unset push.allowLazyForceWithLease && git push --force-with-lease origin master master:naster ) && git ls-remote dst refs/heads/master | @@ -196,6 +208,9 @@ test_expect_success 'new branch covered by force-with-lease' ' ( cd dst && git branch branch master && + git config push.allowLazyForceWithLease false && + test_must_fail git push --force-with-lease=branch origin branch && + git config --unset push.allowLazyForceWithLease && git push --force-with-lease=branch origin branch ) && git ls-remote dst refs/heads/branch >expect && diff --git a/transport-helper.c b/transport-helper.c index 33cff38cc0..e36ea5f578 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -736,6 +736,10 @@ static int push_update_ref_status(struct strbuf *buf, status = REF_STATUS_REJECT_STALE; FREE_AND_NULL(msg); } + else if (!strcmp(msg, "lazy force-with-error")) { + status = REF_STATUS_REJECT_LAZY_CAS; + FREE_AND_NULL(msg); + } else if (!strcmp(msg, "forced update")) { forced = 1; FREE_AND_NULL(msg); @@ -847,6 +851,7 @@ static int push_refs_with_push(struct transport *transport, switch (ref->status) { case REF_STATUS_REJECT_NONFASTFORWARD: case REF_STATUS_REJECT_STALE: + case REF_STATUS_REJECT_LAZY_CAS: case REF_STATUS_REJECT_ALREADY_EXISTS: case REF_STATUS_UPTODATE: continue; diff --git a/transport.c b/transport.c index d75ff0514d..25eeb99a36 100644 --- a/transport.c +++ b/transport.c @@ -418,6 +418,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, print_ref_status('!', "[rejected]", ref, ref->peer_ref, "stale info", porcelain, summary_width); break; + case REF_STATUS_REJECT_LAZY_CAS: + print_ref_status('!', "[rejected]", ref, ref->peer_ref, + "lazy force-with-lease", porcelain, summary_width); + break; case REF_STATUS_REJECT_SHALLOW: print_ref_status('!', "[rejected]", ref, ref->peer_ref, "new shallow roots not allowed", @@ -934,6 +938,7 @@ static int run_pre_push_hook(struct transport *transport, if (!r->peer_ref) continue; if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue; if (r->status == REF_STATUS_REJECT_STALE) continue; + if (r->status == REF_STATUS_REJECT_LAZY_CAS) continue; if (r->status == REF_STATUS_UPTODATE) continue; strbuf_reset(&buf); -- 2.13.2-765-ge5d8a7f768
