[PATCH] push: make `--force-with-lease[=<ref>]` safer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



The `--force-with-lease` option in `git-push`, makes sure that
refs on remote aren't clobbered by unexpected changes when the
"<expect>" ref value is explicitly specified.

For other cases (i.e., `--force-with-lease[=<ref>]`) where the tip
of the remote tracking branch is populated as the "<expect>" value,
there is a possibility of allowing unwanted overwrites on the remote
side when some tools that implicitly fetch remote-tracking refs in
the background are used with the repository. If a remote-tracking ref
was updated when a rewrite is happening locally and if those changes
are pushed by omitting the "<expect>" value in `--force-with-lease`,
any new changes from the updated tip will be lost locally and will
be overwritten on the remote.

This problem can be addressed by checking the `reflog` of the branch
that is being pushed and verify if there in a entry with the remote
tracking ref. By running this check, we can ensure that refs being
are fetched in the background while a "lease" is being held are not
overlooked before a push, and any new changes can be acknowledged
and (if necessary) integrated locally.

The new check will cause `git-push` to fail if it detects the presence
of any updated refs that we do not have locally and reject the push
stating `implicit fetch` as the reason.

An experimental configuration setting: `push.rejectImplicitFetch`
which defaults to `true` (when `features.experimental` is enabled)
has been added, to allow `git-push` to reject a push if the check
fails.

Signed-off-by: Srinidhi Kaushik <shrinidhi.kaushik@xxxxxxxxx>
---

Hello,
I picked this up from #leftoverbits over at GitHub [1] from the open
issues list. This idea [2], for a safer `--force-with-lease` was
originally proposed by Johannes on the mailing list.

[1]: https://github.com/gitgitgadget/git/issues/640
[2]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.1808272306271.73@xxxxxxxxxxxxxxxxx/

Thanks.

 Documentation/config/feature.txt |  3 +
 Documentation/config/push.txt    | 14 +++++
 Documentation/git-push.txt       |  6 ++
 builtin/send-pack.c              |  5 ++
 remote.c                         | 96 +++++++++++++++++++++++++++++---
 remote.h                         |  4 +-
 send-pack.c                      |  1 +
 t/t5533-push-cas.sh              | 86 ++++++++++++++++++++++++++++
 transport-helper.c               |  5 ++
 transport.c                      |  5 ++
 10 files changed, 217 insertions(+), 8 deletions(-)

diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt
index c0cbf2bb1c..f93e9fd898 100644
--- a/Documentation/config/feature.txt
+++ b/Documentation/config/feature.txt
@@ -18,6 +18,9 @@ skipping more commits at a time, reducing the number of round trips.
 * `protocol.version=2` speeds up fetches from repositories with many refs by
 allowing the client to specify which refs to list before the server lists
 them.
++
+* `push.rejectImplicitFetch=true` runs additional checks for linkgit:git-push[1]
+`--force-with-lease` to mitigate implicit updates of remote-tracking refs.

 feature.manyFiles::
 	Enable config options that optimize for repos with many files in the
diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
index f5e5b38c68..1a7184034d 100644
--- a/Documentation/config/push.txt
+++ b/Documentation/config/push.txt
@@ -114,3 +114,17 @@ push.recurseSubmodules::
 	specifying '--recurse-submodules=check|on-demand|no'.
 	If not set, 'no' is used by default, unless 'submodule.recurse' is
 	set (in which case a 'true' value means 'on-demand').
+
+push.rejectImplicitFetch::
+	If set to `true`, runs additional checks for the `--force-with-lease`
+	option when used with linkgit:git-push[1] if the expected value for
+	the remote ref is unspecified (`--force-with-lease[=<ref>]`), and
+	instead asked depend on the current value of the remote-tracking ref.
+	The check ensures that the commit at the tip of the remote-tracking
+	branch -- which may have been implicitly updated by tools that fetch
+	remote refs by running linkgit:git-fetch[1] in the background -- has
+	been integrated locally, when holding the "lease". If the new changes
+	from such remote-tracking refs have not been updated locally before
+	pushing, linkgit:git-push[1] will fail indicating the reject reason
+	as `implicit fetch`. Enabling `feature.experimental` makes this option
+	default to `true`.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 3b8053447e..2176a743f3 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -320,6 +320,12 @@ seen and are willing to overwrite, then rewrite history, and finally
 force push changes to `master` if the remote version is still at
 `base`, regardless of what your local `remotes/origin/master` has been
 updated to in the background.
++
+Alternatively, setting the (experimental) `push.rejectImplicitFetch` option
+to `true` will ensure changes from remote-tracking refs that are updated in the
+background using linkgit:git-fetch[1] are accounted for (either by integrating
+them locally, or explicitly specifying an overwrite), by rejecting to update
+such refs.

 -f::
 --force::
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 2b9610f121..6500a8267a 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -69,6 +69,11 @@ static void print_helper_status(struct ref *ref)
 			msg = "stale info";
 			break;

+		case REF_STATUS_REJECT_IMPLICIT_FETCH:
+			res = "error";
+			msg = "implicit fetch";
+			break;
+
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
 			res = "error";
 			msg = "already exists";
diff --git a/remote.c b/remote.c
index c5ed74f91c..ee2dedd15b 100644
--- a/remote.c
+++ b/remote.c
@@ -49,6 +49,8 @@ static const char *pushremote_name;
 static struct rewrites rewrites;
 static struct rewrites rewrites_push;

+static struct object_id cas_reflog_check_oid;
+
 static int valid_remote(const struct remote *remote)
 {
 	return (!!remote->url) || (!!remote->foreign_vcs);
@@ -1446,6 +1448,22 @@ int match_push_refs(struct ref *src, struct ref **dst,
 	return 0;
 }

+/*
+ * Consider `push.rejectImplicitFetch` to be set to true if experimental
+ * features are enabled; use user-defined value if set explicitly.
+ */
+int reject_implicit_fetch()
+{
+	int conf = 0;
+	if (!git_config_get_bool("push.rejectImplicitFetch", &conf))
+		return conf;
+
+	if (!git_config_get_bool("feature.experimental", &conf))
+		return conf;
+
+	return conf;
+}
+
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 			     int force_update)
 {
@@ -1471,16 +1489,21 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 * If the remote ref has moved and is now different
 		 * from what we expect, reject any push.
 		 *
-		 * It also is an error if the user told us to check
-		 * with the remote-tracking branch to find the value
-		 * to expect, but we did not have such a tracking
-		 * branch.
+		 * It also is an error if the user told us to check with the
+		 * remote-tracking branch to find the value to expect, but we
+		 * did not have such a tracking branch, or we have one that
+		 * has new changes.
 		 */
 		if (ref->expect_old_sha1) {
 			if (!oideq(&ref->old_oid, &ref->old_oid_expect))
 				reject_reason = REF_STATUS_REJECT_STALE;
+			else if (reject_implicit_fetch() && ref->implicit_fetch)
+				reject_reason = REF_STATUS_REJECT_IMPLICIT_FETCH;
 			else
-				/* If the ref isn't stale then force the update. */
+				/*
+				 * If the ref isn't stale, or there was no
+				 * implicit fetch, force the update.
+				 */
 				force_ref_update = 1;
 		}

@@ -2272,23 +2295,67 @@ static int remote_tracking(struct remote *remote, const char *refname,
 	return 0;
 }

+static int oid_in_reflog_ent(struct object_id *ooid, struct object_id *noid,
+			     const char *ident, timestamp_t timestamp, int tz,
+			     const char *message, void *cb_data)
+{
+	return oideq(noid, &cas_reflog_check_oid);
+}
+
+/*
+ * Iterate through the reflog of a local branch and check if the tip of the
+ * remote-tracking branch is reachable from one of the entries.
+ */
+static int remote_ref_in_reflog(const struct object_id *r_oid,
+				const struct object_id *l_oid,
+				const char *local_ref_name)
+{
+	int ret = 0;
+	cas_reflog_check_oid = *r_oid;
+
+	struct commit *r_commit, *l_commit;
+	l_commit = lookup_commit_reference(the_repository, l_oid);
+	r_commit = lookup_commit_reference(the_repository, r_oid);
+
+	/*
+	 * If the remote-tracking ref is an ancestor of the local ref (a merge,
+	 * for instance) there is no need to iterate through the reflog entries
+	 * to ensure reachability; it can be skipped to return early instead.
+	 */
+	ret = (r_commit && l_commit) ? in_merge_bases(r_commit, l_commit) : 0;
+	if (ret)
+		goto skip;
+
+	ret = for_each_reflog_ent_reverse(local_ref_name,
+					  oid_in_reflog_ent,
+					  NULL);
+skip:
+	return ret;
+}
+
 static void apply_cas(struct push_cas_option *cas,
 		      struct remote *remote,
 		      struct ref *ref)
 {
-	int i;
+	int i, do_reflog_check = 0;
+	struct object_id oid;
+	struct ref *local_ref = get_local_ref(ref->name);

 	/* Find an explicit --<option>=<name>[:<value>] entry */
 	for (i = 0; i < cas->nr; i++) {
 		struct push_cas *entry = &cas->entry[i];
 		if (!refname_match(entry->refname, ref->name))
 			continue;
+
 		ref->expect_old_sha1 = 1;
 		if (!entry->use_tracking)
 			oidcpy(&ref->old_oid_expect, &entry->expect);
 		else if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
 			oidclr(&ref->old_oid_expect);
-		return;
+		else
+			do_reflog_check = 1;
+
+		goto reflog_check;
 	}

 	/* Are we using "--<option>" to cover all? */
@@ -2298,6 +2365,21 @@ static void apply_cas(struct push_cas_option *cas,
 	ref->expect_old_sha1 = 1;
 	if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
 		oidclr(&ref->old_oid_expect);
+	else
+		do_reflog_check = 1;
+
+reflog_check:
+	/*
+	 * For cases where "--force-with-lease[=<refname>]" i.e., when the
+	 * "<expect>" value is unspecified, run additional checks to verify
+	 * if the tip of the remote-tracking branch (if implicitly updated
+	 * when a "lease" is being held) is reachable from at least one entry
+	 * in the reflog of the local branch that is being pushed, ensuring
+	 * new changes (if any) have been integrated locally.
+	 */
+	if (do_reflog_check && local_ref && !read_ref(local_ref->name, &oid))
+		ref->implicit_fetch = !remote_ref_in_reflog(&ref->old_oid, &oid,
+							    local_ref->name);
 }

 void apply_push_cas(struct push_cas_option *cas,
diff --git a/remote.h b/remote.h
index 5e3ea5a26d..f859fa5fed 100644
--- a/remote.h
+++ b/remote.h
@@ -104,7 +104,8 @@ struct ref {
 		forced_update:1,
 		expect_old_sha1:1,
 		exact_oid:1,
-		deletion:1;
+		deletion:1,
+		implicit_fetch:1;

 	enum {
 		REF_NOT_MATCHED = 0, /* initial value */
@@ -133,6 +134,7 @@ struct ref {
 		REF_STATUS_REJECT_FETCH_FIRST,
 		REF_STATUS_REJECT_NEEDS_FORCE,
 		REF_STATUS_REJECT_STALE,
+		REF_STATUS_REJECT_IMPLICIT_FETCH,
 		REF_STATUS_REJECT_SHALLOW,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
diff --git a/send-pack.c b/send-pack.c
index 632f1580ca..fe7f14add4 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -240,6 +240,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_IMPLICIT_FETCH:
 	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 0b0eb1d025..840b2a95f9 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -13,6 +13,41 @@ setup_srcdst_basic () {
 	)
 }

+setup_implicit_fetch () {
+	rm -fr src dup dst &&
+	git init --bare dst &&
+	git clone --no-local dst src &&
+	git clone --no-local dst dup
+	(
+		cd src &&
+		test_commit A &&
+		git push
+	) &&
+	(
+		cd dup &&
+		git fetch &&
+		git merge origin/master &&
+		test_commit B &&
+		git switch -c branch master~1 &&
+		test_commit C &&
+		test_commit D &&
+		git push --all
+	) &&
+	(
+		cd src &&
+		git switch master &&
+		git fetch --all &&
+		git branch branch --track origin/branch &&
+		git merge origin/master
+	) &&
+	(
+		cd dup &&
+		git switch master &&
+		test_commit E &&
+		git push origin master:master
+	)
+}
+
 test_expect_success setup '
 	# create template repository
 	test_commit A &&
@@ -256,4 +291,55 @@ test_expect_success 'background updates of REMOTE can be mitigated with a non-up
 	)
 '

+test_expect_success 'implicit updates to remote-tracking refs with `push.rejectImplicitFetch` set (protected, all refs)' '
+	setup_implicit_fetch &&
+	test_when_finished "rm -fr dst src dup" &&
+	git ls-remote dst refs/heads/master >expect.master &&
+	git ls-remote dst refs/heads/master >expect.branch &&
+	(
+		cd src &&
+		git switch master &&
+		test_commit G &&
+		git switch branch &&
+		test_commit H &&
+		git fetch --all &&
+		git config --local feature.experimental true &&
+		test_must_fail git push --force-with-lease --all 2>err &&
+		grep "implicit fetch" err
+	) &&
+	git ls-remote dst refs/heads/master >actual.master &&
+	git ls-remote dst refs/heads/master >actual.branch &&
+	test_cmp expect.master actual.master &&
+	test_cmp expect.branch actual.branch &&
+	(
+		cd src &&
+		git config --local feature.experimental false &&
+		git push --force-with-lease --all 2>err &&
+		grep "forced update" err
+	)
+'
+
+test_expect_success 'implicit updates to remote-tracking refs with `push.rejectImplicitFetch` set (protected, specific ref)' '
+	setup_implicit_fetch &&
+	git ls-remote dst refs/heads/master >actual &&
+	(
+		cd src &&
+		git switch branch &&
+		test_commit F &&
+		git switch master &&
+		test_commit G &&
+		git fetch  &&
+		git config --local push.rejectImplicitFetch true &&
+		test_must_fail git push --force-with-lease=master --all 2>err &&
+		grep "implicit fetch" err
+	) &&
+	git ls-remote dst refs/heads/master >expect &&
+	test_cmp expect actual &&
+	(
+		cd src &&
+		git push --force --force-with-lease --all 2>err &&
+		grep "forced update" err
+	)
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index c52c99d829..75b4c1b758 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -779,6 +779,10 @@ static int push_update_ref_status(struct strbuf *buf,
 			status = REF_STATUS_REJECT_STALE;
 			FREE_AND_NULL(msg);
 		}
+		else if (!strcmp(msg, "ignored fetch")) {
+			status = REF_STATUS_REJECT_IMPLICIT_FETCH;
+			FREE_AND_NULL(msg);
+		}
 		else if (!strcmp(msg, "forced update")) {
 			forced = 1;
 			FREE_AND_NULL(msg);
@@ -896,6 +900,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_IMPLICIT_FETCH:
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
 			if (atomic) {
 				reject_atomic_push(remote_refs, mirror);
diff --git a/transport.c b/transport.c
index 43e24bf1e5..588575498f 100644
--- a/transport.c
+++ b/transport.c
@@ -567,6 +567,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_IMPLICIT_FETCH:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+				 "implicit fetch", porcelain, summary_width);
+		break;
 	case REF_STATUS_REJECT_SHALLOW:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 				 "new shallow roots not allowed",
@@ -1101,6 +1105,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_IMPLICIT_FETCH) continue;
 		if (r->status == REF_STATUS_UPTODATE) continue;

 		strbuf_reset(&buf);
--
2.28.0



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux