From: Junio C Hamano <gitster@pobox•com>
To: git <git@vger•kernel.org>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail•com>,
"Stefan Haller" <haller@ableton•com>, "Jeff King" <peff@peff•net>,
"Matt McCutchen" <matt@mattmccutchen•net>,
"Jacob Keller" <jacob.keller@gmail•com>,
"Mike Rappazzo" <rappazzo@gmail•com>,
"Francesco Mazzoli" <f@mazzo•li>
Subject: [PATCH] push: disable lazy --force-with-lease by default
Date: Thu, 06 Jul 2017 11:56:05 -0700 [thread overview]
Message-ID: <xmqq37a9fl8a.fsf_-_@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAPc5daXVYA8MsseJxge6Qo6ASc=CL6ySt2K61LpOtZ=3H3gWuw@mail.gmail.com> (Junio C. Hamano's message of "Tue, 11 Apr 2017 17:51:47 +0900")
"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.camel@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 <gitster@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
next prev parent reply other threads:[~2017-07-06 18:56 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-08 2:15 Tools that do an automatic fetch defeat "git push --force-with-lease" Matt McCutchen
2017-04-08 7:24 ` Stefan Haller
2017-04-08 7:35 ` Ævar Arnfjörð Bjarmason
2017-04-08 9:29 ` Jeff King
2017-04-08 10:10 ` Jakub Narębski
2017-04-08 11:41 ` [PATCH] push: document & test --force-with-lease with multiple remotes Ævar Arnfjörð Bjarmason
2017-04-09 9:55 ` Simon Ruderich
2017-04-09 11:40 ` Ævar Arnfjörð Bjarmason
2017-04-17 3:56 ` Junio C Hamano
2017-04-19 9:22 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2017-04-08 21:54 ` Tools that do an automatic fetch defeat "git push --force-with-lease" Jacob Keller
2017-04-08 22:13 ` Jeff King
2017-04-08 22:21 ` Jacob Keller
2017-04-09 8:38 ` Stefan Haller
2017-04-09 8:49 ` Jacob Keller
2017-04-09 11:00 ` Stefan Haller
2017-04-10 8:08 ` Jacob Keller
2017-04-10 9:58 ` Ævar Arnfjörð Bjarmason
2017-04-10 23:33 ` Jacob Keller
2017-04-11 8:51 ` Junio C Hamano
2017-04-12 9:11 ` Stefan Haller
2017-07-06 18:56 ` Junio C Hamano [this message]
2017-07-06 19:38 ` [PATCH] push: disable lazy --force-with-lease by default Stefan Beller
2017-07-06 22:39 ` Junio C Hamano
2017-07-06 22:42 ` Stefan Beller
2017-07-10 22:32 ` Stefan Beller
2017-07-07 9:24 ` Stefan Haller
2017-07-07 9:42 ` Jeff King
2017-07-07 9:54 ` Ævar Arnfjörð Bjarmason
2017-07-07 15:15 ` Junio C Hamano
2017-07-15 10:45 ` Ævar Arnfjörð Bjarmason
2017-07-17 17:28 ` Junio C Hamano
2017-07-07 9:39 ` Ævar Arnfjörð Bjarmason
2017-04-11 12:37 ` Tools that do an automatic fetch defeat "git push --force-with-lease" Stefan Haller
2017-04-11 12:37 ` Stefan Haller
2017-04-10 18:31 ` Jeff King
2017-04-11 12:37 ` Stefan Haller
2017-04-11 12:50 ` Jeff King
2017-04-12 9:11 ` Stefan Haller
2017-04-09 8:38 ` Stefan Haller
2017-04-09 8:46 ` Jacob Keller
2017-04-08 8:25 ` Jacob Keller
2017-04-08 9:31 ` Jeff King
2017-04-08 15:03 ` Stefan Haller
2017-04-08 22:03 ` Jeff King
2017-04-08 15:03 ` Stefan Haller
2017-04-08 16:04 ` Ævar Arnfjörð Bjarmason
2017-04-08 17:28 ` Stefan Haller
2017-04-12 9:11 ` Stefan Haller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq37a9fl8a.fsf_-_@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=avarab@gmail$(echo .)com \
--cc=f@mazzo$(echo .)li \
--cc=git@vger$(echo .)kernel.org \
--cc=haller@ableton$(echo .)com \
--cc=jacob.keller@gmail$(echo .)com \
--cc=matt@mattmccutchen$(echo .)net \
--cc=peff@peff$(echo .)net \
--cc=rappazzo@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox