git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH] push: disable lazy --force-with-lease by default

From
Junio C Hamano <gitster@pobox.com>
Date
Jul 6, 2017, 18:56 UTC
Message-ID
<xmqq37a9fl8a.fsf_-_@gitster.mtv.corp.google.com>
In-Reply-To
<CAPc5daXVYA8MsseJxge6Qo6ASc=CL6ySt2K61LpOtZ=3H3gWuw@mail.gmail.com>

"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
Previous: Stefan HallerNext: Stefan Beller
Message 22 of 49 in “Tools that do an automatic fetch defeat "git push --force-with-lease"”
  1. Matt McCutchenApr 8, 2017
  2. Stefan HallerApr 8, 2017
  3. Ævar Arnfjörð BjarmasonApr 8, 2017
  4. Jeff KingApr 8, 2017
  5. Jakub NarębskiApr 8, 2017
  6. push: document & test --force-with-lease with multiple remotesÆvar Arnfjörð Bjarmason, Apr 8, 2017
  7. Simon RuderichApr 9, 2017
  8. Ævar Arnfjörð BjarmasonApr 9, 2017
  9. Junio C HamanoApr 17, 2017
  10. push: document & test --force-with-lease with multiple remotesÆvar Arnfjörð Bjarmason, Apr 19, 2017
  11. Jacob KellerApr 8, 2017
  12. Jeff KingApr 8, 2017
  13. Jacob KellerApr 8, 2017
  14. Stefan HallerApr 9, 2017
  15. Jacob KellerApr 9, 2017
  16. Stefan HallerApr 9, 2017
  17. Jacob KellerApr 10, 2017
  18. Ævar Arnfjörð BjarmasonApr 10, 2017
  19. Jacob KellerApr 10, 2017
  20. Junio C HamanoApr 11, 2017
  21. Stefan HallerApr 12, 2017
  22. push: disable lazy --force-with-lease by defaultJunio C Hamano, Jul 6, 2017
  23. Stefan BellerJul 6, 2017
  24. Junio C HamanoJul 6, 2017
  25. Stefan BellerJul 6, 2017
  26. Stefan BellerJul 10, 2017
  27. Stefan HallerJul 7, 2017
  28. Jeff KingJul 7, 2017
  29. Ævar Arnfjörð BjarmasonJul 7, 2017
  30. Junio C HamanoJul 7, 2017
  31. Ævar Arnfjörð BjarmasonJul 15, 2017
  32. Junio C HamanoJul 17, 2017
  33. Ævar Arnfjörð BjarmasonJul 7, 2017
  34. Stefan HallerApr 11, 2017
  35. Stefan HallerApr 11, 2017
  36. Jeff KingApr 10, 2017
  37. Stefan HallerApr 11, 2017
  38. Jeff KingApr 11, 2017
  39. Stefan HallerApr 12, 2017
  40. Stefan HallerApr 9, 2017
  41. Jacob KellerApr 9, 2017
  42. Jacob KellerApr 8, 2017
  43. Jeff KingApr 8, 2017
  44. Stefan HallerApr 8, 2017
  45. Jeff KingApr 8, 2017
  46. Stefan HallerApr 8, 2017
  47. Ævar Arnfjörð BjarmasonApr 8, 2017
  48. Stefan HallerApr 8, 2017
  49. Stefan HallerApr 12, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.