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

Commit Message

Srinidhi Kaushik Sept. 4, 2020, 6:51 p.m. UTC

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 <[e-mail]>
---

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/[e-mail]/

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(-)

--
2.28.0

Comments

Phillip Wood Sept. 7, 2020, 3:23 p.m. UTC | #1

Hi Srinidhi

Thanks for working on this, making --force-with-lease safer would be a 
valuable contribution

On 04/09/2020 19:51, Srinidhi Kaushik wrote:
> 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.

I think it would help to write out 
`--force-with-lease[=<refname>[:<expect>]]` so readers know what 
"<expect>" is referring to

> 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.

An addition safety measure would be to check the reflog of the local 
commit and the tip of the remote tracking branch dates overlap. 
Otherwise if there is an implicit fetch of a remote head that has been 
rewound we still push the local branch when we shouldn't.

> 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.

'implicit fetch' is a rather terse message - can we say something along 
the lines of "the remote has been updated since the last merge/push"?

> 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.

Making this available with features.experimental initially is probably a 
good idea, I hope it will become the default if in future versions.

> Signed-off-by: Srinidhi Kaushik <[e-mail]>
> ---
> 
> 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/[e-mail]/
> 
> 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;
> +

rather than using a global variable I think it would be better just to 
pass this value around using the cb_data argument of the reflog callback 
function

>   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)
> +{

using the callback data we would have something like

struct oid *remote_head = cb_data;
return oideq(noid, remote_head);

> +	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;

Our coding style is to declare all variables before any statements, so 
this should come above `cas_reflog_check_oid = *r_oid` but that line 
wants to go away anyway.

> +	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;

Rather than using a goto it would perhaps be better to do

if (!ret)
	ret = for_each_reflog_...

> +
> +	ret = for_each_reflog_ent_reverse(local_ref_name,
> +					  oid_in_reflog_ent,
> +					  NULL);

using the callback data we'd pass r_oid rather than NULL as the last 
argument

> +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;

I'm not too keen in jumping here, can't we just check `do_reflog_check` 
below?

Best Wishes

Phillip

>   	}
> 
>   	/* 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
>

Junio C Hamano Sept. 7, 2020, 4:14 p.m. UTC | #2

Srinidhi Kaushik <[e-mail]> writes:

> 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.

Hmph, I am not sure if we are on the same page as the problem with
the form of force-with-lease without <expect>.

In this sequence of end-user operation

    $ git checkout --detach origin/target
    ... edit working tree files ...
    $ git commit --amend -a
    $ git push origin +HEAD:target

the user wanted to fix the topmost commit at the 'target' branch at
the origin repository, and force-update it.

The --force-with-lease is a way to make sure that the only commit
being lost by the force-update is the commit the user wanted to
amend.  If other users pushed to the 'target' branch in the
meantime, the forced push at the last step will lose it.

    $ git checkout --detach origin/target
    $ TO_LOSE=$(git rev-parse HEAD)
    ... edit working tree files ...
    $ git commit --amend -a
    $ git push origin --force-with-lease=target:$TO_LOSE HEAD:target

So we say "I knew, when I started working on the replacement, I
started at the commit $TO_LOSE; please stop my forced push if the
tip of 'target' was moved by somebody else, away from $TO_LOSE".

The force-with-lease without the exact <expect> object name, i.e.

    $ git push origin --force-with-lease=target HEAD:target

would break if 'origin/target' was updated anytime between the time
when the first "git checkout --detach" step finishes and the time
the last "git push" is run, because 'origin/target' would be
different from $TO_LOSE and things that were pushed to 'origin/target'
by others in the meantime will be lost, in addition to $TO_LOSE commit
that the user is willing to discard and replace.

> 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.

Sorry, but it is unclear how reflog would help.

Before the "git checkout" step in the example, there would have been
a "git fetch" from the origin that brought the remote-tracking
branch 'origin/target' to the current state with a reflog entry for
it.  If an automated background process makes another fetch while
the user is editing files in the working tree, such a fetch may also
add another reflog entry for that action.

Unless you make a snapshot of the reflog state immediately after you
do "git checkout" in the example, you wouldn't know if there were
unexpected updates to the remote-tracking branch even if you check
the reflog.

Besides, you do not control the parenthood relationship between the
commits _other_ people push and update to 'target' branch at the
'origin' repository, so you cannot rely on the topology among them
to make any decision.  Other people may be force pushing to the
branch while you are preparing the commit to replace $TO_LOSE by
force pushing.

> +	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".

The problem with "expect-less" form is that it does not hold the
lease at all.  The point of "hold the lease" by giving an explicit
$TO_LOSE is to force a push that does not fast-forward, so if you
iterate over the remote-tracking branch at the time of "push", if
you find more commits built on top of $TO_LOSE because a background
fetch updated the branch, or if you find NO commits on top of $TO_LOSE
because no background fetch happened in the meantime, what would be
pushed would not fast-forward.  So I am not sure what the point of
iterating over reflog.

If we want an option to force a safer behaviour, I think adding a
configuration variable to forbid the form of force-with-lease
without <expect> would be one way to help.  Perhaps make it the
default, while allowing those who want to live dangerously to
explicitly turn it off.

Thanks.

Johannes Schindelin Sept. 7, 2020, 7:45 p.m. UTC | #3

Hi Srinidhi,

On Sat, 5 Sep 2020, Srinidhi Kaushik wrote:

> 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 <[e-mail]>
> ---
>
> 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/[e-mail]/

First of all: thank you for picking this up! The contribution is
pleasantly well-written, thank you also for that.

Now, to be honest, I thought that this mode would merit a new option
rather than piggy-backing on top of `--force-with-lease`. The reason is
that `--force-with-lease` targets a slightly different use case than mine:
it makes sure that we do not overwrite remote refs unless we already had a
chance to inspect them.

In contrast, my workflow uses `git pull --rebase` in two or more separate
worktrees, e.g. when developing a patch on two different Operating
Systems, I frequently forget to pull (to my public repository) on one
side, and I want to avoid force-pushing in that case, even if VS Code (or
I, via `git remote update`) fetched the ref (but failing to rebase the
local branch on top of it).

However, in other scenarios I very much do _not_ want to incorporate the
remote ref. For example, I often fetch
https://github.com/git-for-windows/git.wiki.git to check for the
occasional bogus change. Whenever I see such a bogus change, and it is at
the tip of the branch, I want to force-push _without_ incorporating the
bogus change into the local branch, yet I _do_ want to use
`--force-with-lease` because an independent change could have come in via
the Wiki in the meantime.

So I think that the original `--force-with-lease` and the mode you
implemented target subtly different use cases that are both valid, and
therefore I would like to request a separate option for the latter.

However, I have to admit that I could not think of a good name for that
option. "Implicit fetch" seems a bit too vague here, because the local
branch was not fetched, and certainly not implicitly, yet the logic
revolves around the local branch having been rebased to the
remote-tracking ref at some stage.

Even if we went with the config option to modify `--force-with-lease`'s
behavior, I would recommend separating out the `feature.experimental`
changes into their own patch, so that they can be reverted easily in case
the experimental feature is made the default.

A couple more comments:

> @@ -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 I were you, I would try to keep the original formatting, so that it
becomes more obvious that the part ", or we have [...]" was appended.

>  		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

Should this "or" not be an "and" instead?

> +				 * implicit fetch, force the update.
> +				 */
>  				force_ref_update = 1;
>  		}
> [...]
>  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;

Hmm. I do not condemn `goto` statements in general, but this one makes the
flow harder to follow. I would prefer something like this:

-- snip --
 		else if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
 			oidclr(&ref->old_oid_expect);
+		else if (local_ref && !read_ref(local_ref->name, &oid))
+			ref->implicit_fetch =
+				!remote_ref_in_reflog(&ref->old_oid, &oid,
+						      local_ref->name);
 		return;
-- snap --

Again, thank you so much for working on this!

Ciao,
Dscho

Srinidhi Kaushik Sept. 8, 2020, 3:48 p.m. UTC | #4

Hi Phillip,

On 09/07/2020 16:23, Phillip Wood wrote:
> [...]
> Thanks for working on this, making --force-with-lease safer would be a
> valuable contribution

:)

> > 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.
>
> I think it would help to write out
> `--force-with-lease[=<refname>[:<expect>]]` so readers know what
> "<expect>" is referring to

That makes sense; noted.

> > 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.

> An addition safety measure would be to check the reflog of the local
> commit and the tip of the remote tracking branch dates overlap.
> Otherwise if there is an implicit fetch of a remote head that has been
> rewound we still push the local branch when we shouldn't.

This sounds much better. My initial description of the check was perhaps
a bit confusing.

> > 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.
>
> 'implicit fetch' is a rather terse message - can we say something along
> the lines of "the remote has been updated since the last merge/push"?

I was going by the "two-word" approach like "stale info", "fetch first",
"no match", and so on. But, I'll look into wording the reject reason
along those lines.

> > 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.
>
> Making this available with features.experimental initially is probably a
> good idea, I hope it will become the default if in future versions.

I hope so. :)

> > [...]
> > +		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;
> > +
>
> rather than using a global variable I think it would be better just to
> pass this value around using the cb_data argument of the reflog callback
> function

I have to admit that I was hesitant to use the global variable when
writing this. For some reason I thought the callback data was used to
store results from the called function only and not pass arguments.
Will fix that in v2.

> > [...]
> > +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)
> > +{
>
> using the callback data we would have something like
>
> struct oid *remote_head = cb_data;
> return oideq(noid, remote_head);

Got it; this and the callback argument to the reflog entry function
will be updated accordingly.

> > [...]
> > +{
> > +	int ret = 0;
> > +	cas_reflog_check_oid = *r_oid;
> > +
> > +	struct commit *r_commit, *l_commit;
>
> Our coding style is to declare all variables before any statements, so
> this should come above `cas_reflog_check_oid = *r_oid` but that line
> wants to go away anyway.

Noted; `cas_reflog_check_oid` will go away as suggested above.

> > [...]
> > +	ret = (r_commit && l_commit) ? in_merge_bases(r_commit, l_commit) : 0;
> > +	if (ret)
> > +		goto skip;
>
> Rather than using a goto it would perhaps be better to do
>
> if (!ret)
>	ret = for_each_reflog_...
>
>

OK, yes. The `goto` can be avoided here.

> > +
> > +	ret = for_each_reflog_ent_reverse(local_ref_name,
> > +					  oid_in_reflog_ent,
> > +					  NULL);
>
> using the callback data we'd pass r_oid rather than NULL as the last
> argument


> > [...]
> >   		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;
>
> I'm not too keen in jumping here, can't we just check `do_reflog_check`
> below?

Yes, of course. I was trying conserve the original flow of returning
from the function right after the loop. Since this is just one condition
to check if `do_reflog_check` is set to 1; we can get rid of the `goto`
and use a `break` instead.

> [...]

Thanks for a thorough review.
--
Srinidhi Kaushik

Junio C Hamano Sept. 8, 2020, 3:58 p.m. UTC | #5

Johannes Schindelin <[e-mail]> writes:

> Now, to be honest, I thought that this mode would merit a new option
> rather than piggy-backing on top of `--force-with-lease`. The reason is
> that `--force-with-lease` targets a slightly different use case than mine:
> it makes sure that we do not overwrite remote refs unless we already had a
> chance to inspect them.
>
> In contrast, my workflow uses `git pull --rebase` in two or more separate
> worktrees, e.g. when developing a patch on two different Operating
> Systems, I frequently forget to pull (to my public repository) on one
> side, and I want to avoid force-pushing in that case, even if VS Code (or
> I, via `git remote update`) fetched the ref (but failing to rebase the
> local branch on top of it).
>
> So I think that the original `--force-with-lease` and the mode you
> implemented target subtly different use cases that are both valid, and
> therefore I would like to request a separate option for the latter.

I tend to agree that that particular use case does not fit what the
"force with lease" option is meant to solve.  It should be a different
option, as you do not even want to be forcing.

But probably I am not getting your use case well enough to give a
good name suggestion.  "git push" without any form of "force" would
safely fail in your "I rebased on one side, and the other one is now
out of sync" situation already, so...

Srinidhi Kaushik Sept. 8, 2020, 4 p.m. UTC | #6

Hi Junio,
Thanks for taking the time to review this patch.

On 09/07/2020 09:14, Junio C Hamano wrote:
>> 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.
>
> Hmph, I am not sure if we are on the same page as the problem with
> the form of force-with-lease without <expect>.
>
> In this sequence of end-user operation
>
>    $ git checkout --detach origin/target
>    ... edit working tree files ...
>    $ git commit --amend -a
>    $ git push origin +HEAD:target
>
> the user wanted to fix the topmost commit at the 'target' branch at
> the origin repository, and force-update it.
>
> The --force-with-lease is a way to make sure that the only commit
> being lost by the force-update is the commit the user wanted to
> amend.  If other users pushed to the 'target' branch in the
> meantime, the forced push at the last step will lose it.
>
>    $ git checkout --detach origin/target
>    $ TO_LOSE=$(git rev-parse HEAD)
>    ... edit working tree files ...
>    $ git commit --amend -a
>    $ git push origin --force-with-lease=target:$TO_LOSE HEAD:target
>
> So we say "I knew, when I started working on the replacement, I
> started at the commit $TO_LOSE; please stop my forced push if the
> tip of 'target' was moved by somebody else, away from $TO_LOSE".
>
> The force-with-lease without the exact <expect> object name, i.e.
>
>    $ git push origin --force-with-lease=target HEAD:target
>
> would break if 'origin/target' was updated anytime between the time
> when the first "git checkout --detach" step finishes and the time
> the last "git push" is run, because 'origin/target' would be
> different from $TO_LOSE and things that were pushed to 'origin/target'
> by others in the meantime will be lost, in addition to $TO_LOSE commit
> that the user is willing to discard and replace.

Sorry, that commit message should have been worded in a better way.
What I originally meant to say was: losing any _new_ changes that
were made on the remote during a rewrite, i.e., when we have a starting
point ($TO_LOSE) from the remote-tracking ref that we want to base our
rewrite on (after checkout) and then if the remote-tracking ref gets
updated by a push from another user, _and_ we don't use an "<expect>"
for `--force-with-lease`, there is a possibility that changes from the
other push would be lost because we're basing our rewrite on an older
version (?) of the remote-tracking ref, and our push would overwrite
its new changes because the value "<expect>" when omitted would point
to the updated tip of the remote instead. The condition:

	if (!oideq(&ref->old_oid, &ref->old_oid_expect))

would evaluate false since we're using `use_tracking`. This essentially
reduces the behavior of `--force-with-lease=<ref>` to `--force` in this
scenario.

>> 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.
> Sorry, but it is unclear how reflog would help.
>
> Before the "git checkout" step in the example, there would have been
> a "git fetch" from the origin that brought the remote-tracking
> branch 'origin/target' to the current state with a reflog entry for
> it.  If an automated background process makes another fetch while
> the user is editing files in the working tree, such a fetch may also
> add another reflog entry for that action.
>
> Unless you make a snapshot of the reflog state immediately after you
> do "git checkout" in the example, you wouldn't know if there were
> unexpected updates to the remote-tracking branch even if you check
> the reflog.
>
> Besides, you do not control the parenthood relationship between the
> commits _other_ people push and update to 'target' branch at the
> 'origin' repository, so you cannot rely on the topology among them
> to make any decision.  Other people may be force pushing to the
> branch while you are preparing the commit to replace $TO_LOSE by
> force pushing.
>
>> +     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".
>
> The problem with "expect-less" form is that it does not hold the
> lease at all.  The point of "hold the lease" by giving an explicit
> $TO_LOSE is to force a push that does not fast-forward, so if you
> iterate over the remote-tracking branch at the time of "push", if
> you find more commits built on top of $TO_LOSE because a background
> fetch updated the branch, or if you find NO commits on top of $TO_LOSE
> because no background fetch happened in the meantime, what would be
> pushed would not fast-forward.  So I am not sure what the point of
> iterating over reflog.

Right, I agree with what is described above. But, in this patch, we are
looking at the reflog of the _local_ branch that is going to be updated
on the remote side. The point of going through the reflog is to see if
the current tip its remote-tracking branch is present in one of the
reflog entries implying that any new changes (pushes from another user)
in the meantime aren't ignored and overwritten with our push.
Would that be an incorrect assumption?

> If we want an option to force a safer behaviour, I think adding a
> configuration variable to forbid the form of force-with-lease
> without <expect> would be one way to help.  Perhaps make it the
> default, while allowing those who want to live dangerously to
> explicitly turn it off.

Yes, that sounds like a good way to mitigate this issue; but that being
said, setups where `--force-with-lease` is being used as an alias for
`--force` should probably be taken into consideration though.

Thanks.
--
Srinidhi Kaushik

Srinidhi Kaushik Sept. 8, 2020, 4:59 p.m. UTC | #7

Hi Johannes,

On 09/07/2020 21:45, Johannes Schindelin wrote:
>> [...]
> First of all: thank you for picking this up! The contribution is
> pleasantly well-written, thank you also for that.

Thank you for the kind words.

> Now, to be honest, I thought that this mode would merit a new option
> rather than piggy-backing on top of `--force-with-lease`. The reason is
> that `--force-with-lease` targets a slightly different use case than mine:
> it makes sure that we do not overwrite remote refs unless we already had a
> chance to inspect them.
>
> In contrast, my workflow uses `git pull --rebase` in two or more separate
> worktrees, e.g. when developing a patch on two different Operating
> Systems, I frequently forget to pull (to my public repository) on one
> side, and I want to avoid force-pushing in that case, even if VS Code (or
> I, via `git remote update`) fetched the ref (but failing to rebase the
> local branch on top of it).
>
> However, in other scenarios I very much do _not_ want to incorporate the
> remote ref. For example, I often fetch
> https://github.com/git-for-windows/git.wiki.git to check for the
> occasional bogus change. Whenever I see such a bogus change, and it is at
> the tip of the branch, I want to force-push _without_ incorporating the
> bogus change into the local branch, yet I _do_ want to use
> `--force-with-lease` because an independent change could have come in via
> the Wiki in the meantime.

I realize that this new check would not be helpful if we deliberately
choose not to include an unwanted change from the updated remote's tip.
In that case, we would have to use `--force` make it work, and that
defeats the use of `--force-with-lease`.

> So I think that the original `--force-with-lease` and the mode you
> implemented target subtly different use cases that are both valid, and
> therefore I would like to request a separate option for the latter.

OK. So, I am assuming that you are suggesting to add a new function that
is separate from `apply_push_cas()` and run the check on each of the
remote refs. Would that be correct?

If that's the case, how does it work along with `--force-with-lease`?
On one hand we have `--force-with-lease` to ensure we rewrite the remote
that we have _already_ seen, and on the other, a new option that checks
reflog of the local branch to see if it is missing any updates from the
remote that may have happened in the meantime. If we pass both of them
for `push` and if the former doesn't complain, and the latter check
fails, should the `push` still go through?

I feel that this check included with `--force-with-lease` only when
the `use_tracking` or `use_tracking_for_rest` options are enabled
would give a heads-up the the user about the background fetch. If
they decide that they don't need new updates, then supplying the
new "<expect>" value in the next push would imply they've seen the
new update, and choose to overwrite it anyway. The check would not
run in this case. But again, I wonder if the this "two-step" process
makes `push` cumbersome.

> However, I have to admit that I could not think of a good name for that
> option. "Implicit fetch" seems a bit too vague here, because the local
> branch was not fetched, and certainly not implicitly, yet the logic
> revolves around the local branch having been rebased to the
> remote-tracking ref at some stage.

The message "implicit fetch" was in context of the remote ref. But yes,
the current reject reason is not clear and implies that local branch
was fetched, which isn't the case.

> Even if we went with the config option to modify `--force-with-lease`'s
> behavior, I would recommend separating out the `feature.experimental`
> changes into their own patch, so that they can be reverted easily in case
> the experimental feature is made the default.

Good idea!

> A couple more comments:
>
>> @@ -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 I were you, I would try to keep the original formatting, so that it
> becomes more obvious that the part ", or we have [...]" was appended.

Alright, I will append the new comment in a new line instead.

>
>>               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
>
> Should this "or" not be an "and" instead?

D'oh, you are right. It should have been an "and".

>
>> +                              * implicit fetch, force the update.
>> +                              */
>>                               force_ref_update = 1;
>>               }
>> [...]
>>  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;
>
> Hmm. I do not condemn `goto` statements in general, but this one makes the
> flow harder to follow. I would prefer something like this:
>
> -- snip --
>               else if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
>                       oidclr(&ref->old_oid_expect);
> +             else if (local_ref && !read_ref(local_ref->name, &oid))
> +                     ref->implicit_fetch =
> +                             !remote_ref_in_reflog(&ref->old_oid, &oid,
> +                                                   local_ref->name);
>               return;
> -- snap --

Adding this condition looks cleaner instead of the `goto`. A similar
suggestion was made in the other thread [1] as well; this will be
addressed in v2.

> Again, thank you so much for working on this!

Thanks again, for taking the time to review this.

[1]: https://public-inbox.org/git/[e-mail]/
--
Srinidhi Kaushik

Junio C Hamano Sept. 8, 2020, 7:34 p.m. UTC | #8

Johannes Schindelin <[e-mail]> writes:

> Now, to be honest, I thought that this mode would merit a new option
> rather than piggy-backing on top of `--force-with-lease`. The reason is
> that `--force-with-lease` targets a slightly different use case than mine:
> it makes sure that we do not overwrite remote refs unless we already had a
> chance to inspect them.
>
> In contrast, my workflow uses `git pull --rebase` in two or more separate
> worktrees, e.g. when developing a patch on two different Operating
> Systems, I frequently forget to pull (to my public repository) on one
> side, and I want to avoid force-pushing in that case, even if VS Code (or
> I, via `git remote update`) fetched the ref (but failing to rebase the
> local branch on top of it).
> ...
> So I think that the original `--force-with-lease` and the mode you
> implemented target subtly different use cases that are both valid, and
> therefore I would like to request a separate option for the latter.

I agree that the use case in the second paragraph above does not fit
what the "force with lease" option is meant to solve.  You do not
even want to be forcing in the workflow so "--force-with-anything"
is a bad name for the mode of operation, if I am reading you right.

Junio C Hamano Sept. 8, 2020, 9 p.m. UTC | #9

Srinidhi Kaushik <[e-mail]> writes:

> to the updated tip of the remote instead. The condition:
>
> 	if (!oideq(&ref->old_oid, &ref->old_oid_expect))
>
> would evaluate false since we're using `use_tracking`. This essentially
> reduces the behavior of `--force-with-lease=<ref>` to `--force` in this
> scenario.

Yes, that is exactly why I have kept saying that the form without
<expect> is not safe and cannot be made safer especially when
auto-fetching is involved.

> Right, I agree with what is described above. But, in this patch, we are
> looking at the reflog of the _local_ branch that is going to be updated
> on the remote side. The point of going through the reflog is to see if
> the current tip its remote-tracking branch is present in one of the
> reflog entries implying that any new changes (pushes from another user)
> in the meantime aren't ignored and overwritten with our push.
> Would that be an incorrect assumption?

I am afraid it is.  You may have looked at it and even kept it
locally, to be looked at later, without letting it affect what you
are going ot force-push in any way.  You might later come back and
resurrect their change on top of what you force pushed, or more
likely you may simply forget, especially if the forced push goes
through without failing.

> Yes, that sounds like a good way to mitigate this issue; but that being
> said, setups where `--force-with-lease` is being used as an alias for
> `--force` should probably be taken into consideration though.

We cannot help, and it is not our job to dispel, misconfigurations
and misconceptions caused by following bad pieces of advice other
people gave our users, though.

Johannes Schindelin Sept. 9, 2020, 3:40 a.m. UTC | #10

Hi Junio,

On Tue, 8 Sep 2020, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > Now, to be honest, I thought that this mode would merit a new option
> > rather than piggy-backing on top of `--force-with-lease`. The reason is
> > that `--force-with-lease` targets a slightly different use case than mine:
> > it makes sure that we do not overwrite remote refs unless we already had a
> > chance to inspect them.
> >
> > In contrast, my workflow uses `git pull --rebase` in two or more separate
> > worktrees, e.g. when developing a patch on two different Operating
> > Systems, I frequently forget to pull (to my public repository) on one
> > side, and I want to avoid force-pushing in that case, even if VS Code (or
> > I, via `git remote update`) fetched the ref (but failing to rebase the
> > local branch on top of it).
> >
> > So I think that the original `--force-with-lease` and the mode you
> > implemented target subtly different use cases that are both valid, and
> > therefore I would like to request a separate option for the latter.
>
> I tend to agree that that particular use case does not fit what the
> "force with lease" option is meant to solve.  It should be a different
> option, as you do not even want to be forcing.

Actually, I _do_ want to be forcing, but _only_ if the recorded remote tip
has been integrated into the local branch at some point (even if it has
been rebased or amended away).

And even then, I want to only force when the recorded remote tip still
matches the actual remote tip, i.e. I want to force "with lease".

> But probably I am not getting your use case well enough to give a
> good name suggestion.  "git push" without any form of "force" would
> safely fail in your "I rebased on one side, and the other one is now
> out of sync" situation already, so...

Yes, it would safely fail, but it would also fail in situations where I do
_not_ want it to fail.

Imagine that you and I work together on a patch series, and we actually
share the same public repository. Let's say that we also want to allow for
some crossing emails that might be missed by the other when pushing our
changes.

Since we are working on a patch series, there will be a lot of amending
and rewording and rebasing going on.

So let's assume that you just did another round of amending and rewording
and want to push the branch. What you will want to avoid is to overwrite
commits that I might have just pushed. `--force-with-lease` to the rescue!

Since you did not fetch in between pulling and amending, that is safe.

But what if, maybe even by mistake, you called `git fetch` in between? And
then you _did_ get my updates, but only in the remote-tracking branch (the
"lease"), yet those updates never made it into the local branch. And since
`git push --force-with-lease` did not fail, you failed to notice that I
had made some edits, didn't `range-diff` and took care of amending your
local changes accordingly, and therefore you overwrote my changes.

That is the scenario this new mode wants to address.

Note that a similar scenario can occur if you work on a patch series on
Linux, then push it to your public repository, then try to make it work
on, say, an HP/UX machine you happen to have access to (maybe because your
employer needs you to make sure that this particular patch series solves a
particular problem on that particular machine), and you need to amend the
changes there. If you're very disciplined, you will only work on one
machine until you're done, then push, then fetch on the other machine.
However, if you're less disciplined (or "just need to check out something
real quick on the other machine"), this new mode will come in quite handy.

Ciao,
Dscho

Johannes Schindelin Sept. 9, 2020, 3:44 a.m. UTC | #11

Hi Junio,

On Tue, 8 Sep 2020, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > Now, to be honest, I thought that this mode would merit a new option
> > rather than piggy-backing on top of `--force-with-lease`. The reason is
> > that `--force-with-lease` targets a slightly different use case than mine:
> > it makes sure that we do not overwrite remote refs unless we already had a
> > chance to inspect them.
> >
> > In contrast, my workflow uses `git pull --rebase` in two or more separate
> > worktrees, e.g. when developing a patch on two different Operating
> > Systems, I frequently forget to pull (to my public repository) on one
> > side, and I want to avoid force-pushing in that case, even if VS Code (or
> > I, via `git remote update`) fetched the ref (but failing to rebase the
> > local branch on top of it).
> > ...
> > So I think that the original `--force-with-lease` and the mode you
> > implemented target subtly different use cases that are both valid, and
> > therefore I would like to request a separate option for the latter.
>
> I agree that the use case in the second paragraph above does not fit
> what the "force with lease" option is meant to solve.  You do not
> even want to be forcing in the workflow so "--force-with-anything"
> is a bad name for the mode of operation, if I am reading you right.

No, you _have_ to force the push.

If you don't have to force the push, i.e. if it is a fast-forward, there
absolutely is no need for any of this.

In contrast, when you want to make sure that you _actually_ incorporated
the revision that is currently the remote tip, e.g. via `git pull
--rebase` with a possible additional rebase on top that makes this _not_ a
fast-forward, you totally have to force the push, otherwise it won't work.

Ciao,
Dscho

Johannes Schindelin Sept. 10, 2020, 10:22 a.m. UTC | #12

Hi,

On Wed, 9 Sep 2020, Johannes Schindelin wrote:

> On Tue, 8 Sep 2020, Junio C Hamano wrote:
>
> > Johannes Schindelin <[e-mail]> writes:
> >
> > > Now, to be honest, I thought that this mode would merit a new option
> > > rather than piggy-backing on top of `--force-with-lease`. The reason is
> > > that `--force-with-lease` targets a slightly different use case than mine:
> > > it makes sure that we do not overwrite remote refs unless we already had a
> > > chance to inspect them.
> > >
> > > In contrast, my workflow uses `git pull --rebase` in two or more separate
> > > worktrees, e.g. when developing a patch on two different Operating
> > > Systems, I frequently forget to pull (to my public repository) on one
> > > side, and I want to avoid force-pushing in that case, even if VS Code (or
> > > I, via `git remote update`) fetched the ref (but failing to rebase the
> > > local branch on top of it).
> > > ...
> > > So I think that the original `--force-with-lease` and the mode you
> > > implemented target subtly different use cases that are both valid, and
> > > therefore I would like to request a separate option for the latter.
> >
> > I agree that the use case in the second paragraph above does not fit
> > what the "force with lease" option is meant to solve.  You do not
> > even want to be forcing in the workflow so "--force-with-anything"
> > is a bad name for the mode of operation, if I am reading you right.
>
> No, you _have_ to force the push.
>
> If you don't have to force the push, i.e. if it is a fast-forward, there
> absolutely is no need for any of this.
>
> In contrast, when you want to make sure that you _actually_ incorporated
> the revision that is currently the remote tip, e.g. via `git pull
> --rebase` with a possible additional rebase on top that makes this _not_ a
> fast-forward, you totally have to force the push, otherwise it won't work.

Maybe `--force-if-incorporated`? Originally, I had in mind to call it
`--safe-force`, but that might be too vague.

BTW I think the patch needs to cover a bit more, still: after I run `git
pull --rebase`, the local branch will never have been at the same revision
as the fetched one: `git rebase` moves to an unnamed branch before
replaying the patches. So I think we need to see whether the remote tip
was _reachable_ from (not necessarily identical to) any of the reflog's
revisions.

Ciao,
Dscho

Srinidhi Kaushik Sept. 10, 2020, 2:44 p.m. UTC | #13

Hello,

On 09/10/2020 12:22, Johannes Schindelin wrote:
> Hi
>> [...] 
> Maybe `--force-if-incorporated`? Originally, I had in mind to call it
> `--safe-force`, but that might be too vague.

That's nice. I haven't been able to come up with a good name for this
option. So far, I have: `--check-updated-remote-refs` which is really
long and is probably confusing.
 
> BTW I think the patch needs to cover a bit more, still: after I run `git
> pull --rebase`, the local branch will never have been at the same revision
> as the fetched one: `git rebase` moves to an unnamed branch before
> replaying the patches. So I think we need to see whether the remote tip
> was _reachable_ from (not necessarily identical to) any of the reflog's
> revisions.

Good catch. Would adding in_merge_bases() along with checking if OIDs
are equal for each reflog entry in oid_in_reflog_ent() address the
problem? That way, we would check if remote ref is reachable from
one of the entries?

Thanks.

-- >8 --
+ 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)
+ {
+	struct object_id *remote_oid = cb_data;
+	struct commit *a = lookup_commit_reference(the_repository, noid);
+	struct commit *b = lookup_commit_reference(the_repository, remote_oid);
+	return oideq(noid, remote_oid) || in_merge_bases(b, a);
+ }

Junio C Hamano Sept. 10, 2020, 2:46 p.m. UTC | #14

Johannes Schindelin <[e-mail]> writes:

>> In contrast, when you want to make sure that you _actually_ incorporated
>> the revision that is currently the remote tip, e.g. via `git pull
>> --rebase` with a possible additional rebase on top that makes this _not_ a
>> fast-forward, you totally have to force the push, otherwise it won't work.
>
> Maybe `--force-if-incorporated`? Originally, I had in mind to call it
> `--safe-force`, but that might be too vague.

Yup.  "safe force" indeed feels like a misnomer.  The assumption of
safety relies heavily on the workflow.

I might even say --force-if-merged even if the way the to-be-lost
changes have become part of what you are pushing out is not
technically a merge, but there may be shorter and sweeter way to
express it than 'merge' and 'incorporate'.

Johannes Schindelin Sept. 11, 2020, 10:16 p.m. UTC | #15

Hi Srinidhi,

On Thu, 10 Sep 2020, Srinidhi Kaushik wrote:

> On 09/10/2020 12:22, Johannes Schindelin wrote:
>
> > BTW I think the patch needs to cover a bit more, still: after I run `git
> > pull --rebase`, the local branch will never have been at the same revision
> > as the fetched one: `git rebase` moves to an unnamed branch before
> > replaying the patches. So I think we need to see whether the remote tip
> > was _reachable_ from (not necessarily identical to) any of the reflog's
> > revisions.
>
> Good catch. Would adding in_merge_bases() along with checking if OIDs
> are equal for each reflog entry in oid_in_reflog_ent() address the
> problem? That way, we would check if remote ref is reachable from
> one of the entries?
>
> Thanks.
>
> -- >8 --
> + 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)
> + {
> +	struct object_id *remote_oid = cb_data;
> +	struct commit *a = lookup_commit_reference(the_repository, noid);
> +	struct commit *b = lookup_commit_reference(the_repository, remote_oid);
> +	return oideq(noid, remote_oid) || in_merge_bases(b, a);
> + }

Since `in_merge_bases()` is quite a bit more expensive than `oideq()`,
personally, I would actually walk the reflog with the `oideq()` check
first (stopping early in case a match was found), and only fall back to
looking for a merge base in the reflog if the first reflog walk did not
find a match.

Ciao,
Dscho

Johannes Schindelin Sept. 11, 2020, 10:17 p.m. UTC | #16

Hi Junio,

On Thu, 10 Sep 2020, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> >> In contrast, when you want to make sure that you _actually_ incorporated
> >> the revision that is currently the remote tip, e.g. via `git pull
> >> --rebase` with a possible additional rebase on top that makes this _not_ a
> >> fast-forward, you totally have to force the push, otherwise it won't work.
> >
> > Maybe `--force-if-incorporated`? Originally, I had in mind to call it
> > `--safe-force`, but that might be too vague.
>
> Yup.  "safe force" indeed feels like a misnomer.  The assumption of
> safety relies heavily on the workflow.
>
> I might even say --force-if-merged even if the way the to-be-lost
> changes have become part of what you are pushing out is not
> technically a merge, but there may be shorter and sweeter way to
> express it than 'merge' and 'incorporate'.

You're right, `--force-if-merged` is a much better way to put it.

Thanks,
Dscho

Srinidhi Kaushik Sept. 14, 2020, 11:06 a.m. UTC | #17

Hi Johannes,

On 09/12/2020 00:16, Johannes Schindelin wrote:
> > + 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)
> > + {
> > +	struct object_id *remote_oid = cb_data;
> > +	struct commit *a = lookup_commit_reference(the_repository, noid);
> > +	struct commit *b = lookup_commit_reference(the_repository, remote_oid);
> > +	return oideq(noid, remote_oid) || in_merge_bases(b, a);
> > + }
> 
> Since `in_merge_bases()` is quite a bit more expensive than `oideq()`,
> personally, I would actually walk the reflog with the `oideq()` check
> first (stopping early in case a match was found), and only fall back to
> looking for a merge base in the reflog if the first reflog walk did not
> find a match.

Thanks for the suggestion; the updated the patch set (v3) - [1] does
something similar to that, along with some other changes. Can you please
take a look?

[1]: https://public-inbox.org/git/[e-mail]/

Thanks.

Junio C Hamano Sept. 14, 2020, 8:07 p.m. UTC | #18

Johannes Schindelin <[e-mail]> writes:

> Hi Junio,
>
> On Thu, 10 Sep 2020, Junio C Hamano wrote:
>
>> Johannes Schindelin <[e-mail]> writes:
>>
>> >> In contrast, when you want to make sure that you _actually_ incorporated
>> >> the revision that is currently the remote tip, e.g. via `git pull
>> >> --rebase` with a possible additional rebase on top that makes this _not_ a
>> >> fast-forward, you totally have to force the push, otherwise it won't work.
>> >
>> > Maybe `--force-if-incorporated`? Originally, I had in mind to call it
>> > `--safe-force`, but that might be too vague.
>>
>> Yup.  "safe force" indeed feels like a misnomer.  The assumption of
>> safety relies heavily on the workflow.
>>
>> I might even say --force-if-merged even if the way the to-be-lost
>> changes have become part of what you are pushing out is not
>> technically a merge, but there may be shorter and sweeter way to
>> express it than 'merge' and 'incorporate'.
>
> You're right, `--force-if-merged` is a much better way to put it.

I am pretty happy that Srinidhi chose 'included', which is what
seems the best description without being a white-lie that is
technically incorrect.

Junio C Hamano Sept. 14, 2020, 8:08 p.m. UTC | #19

Johannes Schindelin <[e-mail]> writes:

> Hi Srinidhi,
>
> On Thu, 10 Sep 2020, Srinidhi Kaushik wrote:
>
>> On 09/10/2020 12:22, Johannes Schindelin wrote:
>>
>> > BTW I think the patch needs to cover a bit more, still: after I run `git
>> > pull --rebase`, the local branch will never have been at the same revision
>> > as the fetched one: `git rebase` moves to an unnamed branch before
>> > replaying the patches. So I think we need to see whether the remote tip
>> > was _reachable_ from (not necessarily identical to) any of the reflog's
>> > revisions.
>>
>> Good catch. Would adding in_merge_bases() along with checking if OIDs
>> are equal for each reflog entry in oid_in_reflog_ent() address the
>> problem? That way, we would check if remote ref is reachable from
>> one of the entries?

That sounds very expensive.  

If we switched to check the reflog of HEAD (and not any particular
local branch's reflog), then "has this ever been checked out", tests
would suffice, no?  We detach at the tip of the remote-tracking
branch and then reapply our work one by one in such a case, so we
should have the record of having their tip in the working tree at
some point (i.e. at the beginning of the "rebase" phase of the "pull
--rebase" operation).

Srinidhi Kaushik Sept. 16, 2020, 5:31 a.m. UTC | #20

Hi Junio,

On 09/14/2020 13:08, Junio C Hamano wrote:
>>> [...]
>>>
>>> Good catch. Would adding in_merge_bases() along with checking if OIDs
>>> are equal for each reflog entry in oid_in_reflog_ent() address the
>>> problem? That way, we would check if remote ref is reachable from
>>> one of the entries?
> 
> That sounds very expensive.  

Yes, you're right about that.
 
> If we switched to check the reflog of HEAD (and not any particular
> local branch's reflog), then "has this ever been checked out", tests
> would suffice, no?  We detach at the tip of the remote-tracking
> branch and then reapply our work one by one in such a case, so we
> should have the record of having their tip in the working tree at
> some point (i.e. at the beginning of the "rebase" phase of the "pull
> --rebase" operation).
 
Interesting, I think that might work! Since HEAD moves around and
records all the movements, if the remote was ever checked out there
should be an entry in the reflog. I guess we could stick to "oideq()"
for each entry if we're going this way. However, I think we should
keep that one "in_merge_bases()" call at the beginning to check if
we can can skip checking the reflog if the remote ref is an ancestor
of the local ref.

>>>> On 09/10/2020 12:22, Johannes Schindelin wrote:
>>>>
>>>> BTW I think the patch needs to cover a bit more, still: after I run `git
>>>> pull --rebase`, the local branch will never have been at the same revision
>>>> as the fetched one: `git rebase` moves to an unnamed branch before
>>>> replaying the patches. So I think we need to see whether the remote tip
>>>> was _reachable_ from (not necessarily identical to) any of the reflog's
>>>> revision

Also, there would be an entry for what Johannes describes above -- when
a `git pull --rebase` is run. Will change to this in the next series.

Thanks.

Johannes Schindelin Sept. 16, 2020, 10:20 a.m. UTC | #21

Hi,

On Wed, 16 Sep 2020, Srinidhi Kaushik wrote:

> On 09/14/2020 13:08, Junio C Hamano wrote:
> >>> [...]
> >>>
> >>> Good catch. Would adding in_merge_bases() along with checking if OIDs
> >>> are equal for each reflog entry in oid_in_reflog_ent() address the
> >>> problem? That way, we would check if remote ref is reachable from
> >>> one of the entries?
> >
> > That sounds very expensive.
>
> Yes, you're right about that.
>
> > If we switched to check the reflog of HEAD (and not any particular
> > local branch's reflog), then "has this ever been checked out", tests
> > would suffice, no?  We detach at the tip of the remote-tracking
> > branch and then reapply our work one by one in such a case, so we
> > should have the record of having their tip in the working tree at
> > some point (i.e. at the beginning of the "rebase" phase of the "pull
> > --rebase" operation).
>
> Interesting, I think that might work! Since HEAD moves around and
> records all the movements, if the remote was ever checked out there
> should be an entry in the reflog.

No, I don't think that would suffice. For several reasons:

- the user could have checked out the remote-tracking branch directly
  (`git switch --detach origin/my-branch`), as opposed to actually
  integrating the revision into the branch at some stage. I know that I do
  that sometimes.

- even if the remote-tracking branch has not been checked out directly, it
  might have been `git pull --rebase`d into a _different_ branch, in which
  case the reflog of `HEAD` would say "yep, I saw this commit!" but that
  would not mean that it was integrated into the branch we want to
  (force-)push.

- the reflog of the `HEAD` is worktree-specific, and if the user pushes
  from a different worktree (e.g. to push multiple branches at the same
  time, or because the push is scripted and run from a different working
  directory), it would be missed.

- and if we take a little step back, we do see that the reflog of `HEAD`
  _does_ answer a different question from what we actually asked.

Sure, it would be faster, but is that worth the consequences?

I really think we need to stick to looking at the reflog of the asked-for
branch. And to make that faster, we should have a first loop using
`oideq()` and failing that check, run a second loop using
`is_in_merge_bases()`.

Ciao,
Dscho

Johannes Schindelin Sept. 16, 2020, 11:55 a.m. UTC | #22

Hi Srinidhi,

On Tue, 8 Sep 2020, Srinidhi Kaushik wrote:

> On 09/07/2020 21:45, Johannes Schindelin wrote:
> >> [...]
>
> > Now, to be honest, I thought that this mode would merit a new option
> > rather than piggy-backing on top of `--force-with-lease`. The reason is
> > that `--force-with-lease` targets a slightly different use case than mine:
> > it makes sure that we do not overwrite remote refs unless we already had a
> > chance to inspect them.
> >
> > In contrast, my workflow uses `git pull --rebase` in two or more separate
> > worktrees, e.g. when developing a patch on two different Operating
> > Systems, I frequently forget to pull (to my public repository) on one
> > side, and I want to avoid force-pushing in that case, even if VS Code (or
> > I, via `git remote update`) fetched the ref (but failing to rebase the
> > local branch on top of it).
> >
> > However, in other scenarios I very much do _not_ want to incorporate the
> > remote ref. For example, I often fetch
> > https://github.com/git-for-windows/git.wiki.git to check for the
> > occasional bogus change. Whenever I see such a bogus change, and it is at
> > the tip of the branch, I want to force-push _without_ incorporating the
> > bogus change into the local branch, yet I _do_ want to use
> > `--force-with-lease` because an independent change could have come in via
> > the Wiki in the meantime.
>
> I realize that this new check would not be helpful if we deliberately
> choose not to include an unwanted change from the updated remote's tip.
> In that case, we would have to use `--force` make it work, and that
> defeats the use of `--force-with-lease`.

I would characterize it as "somewhat stricter" than `--force-with-lease`.
The check would not make sense without the lease.

> > So I think that the original `--force-with-lease` and the mode you
> > implemented target subtly different use cases that are both valid, and
> > therefore I would like to request a separate option for the latter.
>
> OK. So, I am assuming that you are suggesting to add a new function that
> is separate from `apply_push_cas()` and run the check on each of the
> remote refs. Would that be correct?

Oh, I don't really know the code well enough to make a suggestion. I guess
if it is easy enough to modify the existing code path (e.g. extend the
function signature in a minimal manner), then that's what I would go for,
rather than a completely separate code path.

> If that's the case, how does it work along with `--force-with-lease`?

In my mind, the new mode implies `--force-with-lease`.

Thanks,
Dscho

> On one hand we have `--force-with-lease` to ensure we rewrite the remote
> that we have _already_ seen, and on the other, a new option that checks
> reflog of the local branch to see if it is missing any updates from the
> remote that may have happened in the meantime. If we pass both of them
> for `push` and if the former doesn't complain, and the latter check
> fails, should the `push` still go through?
>
> I feel that this check included with `--force-with-lease` only when
> the `use_tracking` or `use_tracking_for_rest` options are enabled
> would give a heads-up the the user about the background fetch. If
> they decide that they don't need new updates, then supplying the
> new "<expect>" value in the next push would imply they've seen the
> new update, and choose to overwrite it anyway. The check would not
> run in this case. But again, I wonder if the this "two-step" process
> makes `push` cumbersome.
>
> > However, I have to admit that I could not think of a good name for that
> > option. "Implicit fetch" seems a bit too vague here, because the local
> > branch was not fetched, and certainly not implicitly, yet the logic
> > revolves around the local branch having been rebased to the
> > remote-tracking ref at some stage.
>
> The message "implicit fetch" was in context of the remote ref. But yes,
> the current reject reason is not clear and implies that local branch
> was fetched, which isn't the case.
>
> > Even if we went with the config option to modify `--force-with-lease`'s
> > behavior, I would recommend separating out the `feature.experimental`
> > changes into their own patch, so that they can be reverted easily in case
> > the experimental feature is made the default.
>
> Good idea!
>
> > A couple more comments:
> >
> >> @@ -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 I were you, I would try to keep the original formatting, so that it
> > becomes more obvious that the part ", or we have [...]" was appended.
>
> Alright, I will append the new comment in a new line instead.
>
> >
> >>               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
> >
> > Should this "or" not be an "and" instead?
>
> D'oh, you are right. It should have been an "and".
>
> >
> >> +                              * implicit fetch, force the update.
> >> +                              */
> >>                               force_ref_update = 1;
> >>               }
> >> [...]
> >>  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;
> >
> > Hmm. I do not condemn `goto` statements in general, but this one makes the
> > flow harder to follow. I would prefer something like this:
> >
> > -- snip --
> >               else if (remote_tracking(remote, ref->name, &ref->old_oid_expect))
> >                       oidclr(&ref->old_oid_expect);
> > +             else if (local_ref && !read_ref(local_ref->name, &oid))
> > +                     ref->implicit_fetch =
> > +                             !remote_ref_in_reflog(&ref->old_oid, &oid,
> > +                                                   local_ref->name);
> >               return;
> > -- snap --
>
> Adding this condition looks cleaner instead of the `goto`. A similar
> suggestion was made in the other thread [1] as well; this will be
> addressed in v2.
>
> > Again, thank you so much for working on this!
>
> Thanks again, for taking the time to review this.
>
> [1]: https://public-inbox.org/git/[e-mail]/
> --
> Srinidhi Kaushik
>

Junio C Hamano Sept. 19, 2020, 5:48 p.m. UTC | #23

Johannes Schindelin <[e-mail]> writes:

> No, I don't think that would suffice. For several reasons:
>
> - the user could have checked out the remote-tracking branch directly
>   (`git switch --detach origin/my-branch`), as opposed to actually
>   integrating the revision into the branch at some stage. I know that I do
>   that sometimes.
>
> - even if the remote-tracking branch has not been checked out directly, it
>   might have been `git pull --rebase`d into a _different_ branch, in which
>   case the reflog of `HEAD` would say "yep, I saw this commit!" but that
>   would not mean that it was integrated into the branch we want to
>   (force-)push.
>
> - the reflog of the `HEAD` is worktree-specific, and if the user pushes
>   from a different worktree (e.g. to push multiple branches at the same
>   time, or because the push is scripted and run from a different working
>   directory), it would be missed.
>
> - and if we take a little step back, we do see that the reflog of `HEAD`
>   _does_ answer a different question from what we actually asked.
>
> Sure, it would be faster, but is that worth the consequences?

It is not about "would be faster" but is trying to be more inclusive
of different workflows.

One minor downside consequence of not looking at the reflog of HEAD
is that it does not make the new option useful for those who could
benefit from it.

They detach HEAD at the tip of the remote-tracking branch, work on
the replacement history while on the detached HEAD, and then force
update the local branch from there once they are done (this is a
variant to the first one you said you "do sometimes").  This is a
handy technique to allow them to always compare the previous round
of their effort (which is untouched while they prepare the new
iteration on detached HEAD) with what they achieved so far on HEAD.

After finishing, they would go on to deal with other branches, so
the local branch prepared to be force-pushed may no longer be the
current one.  Without checking with the reflog of HEAD, there is no
way to notice that the local branch to be force-pushed (which no
longer is the current branch) was built using the commit at the tip
of the remote-tracking branch we are losing, because the branch's
reflog would have only one entry that records where it was before
adjusting for the updated remote-tracking branch, and where it is
now after rebuilding.

It is inevitable for a heuristics like the one that was proposed to
work only for those with some workflows while excluding those with
some other workflows, and that is why I said it is "minor" downside
that it does not make the feature useful for some people who could
potentially benefit and also that is why I think it is perfectly OK
to draw the line there and not support them.  After all, the line
has to be drawn somewhere, and the looser the heuristics are, the
wider the potential audience *and* at the same time the wider the
potential of false positives.

Where we drew the line and its consequences must be communicated
clearly in the documentation, though.

Thanks.

Patch

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);