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

From: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
Date: Sep 8, 2020, 15:48 UTC
Message-ID: <20200908154859.GA40807@mail.clickyotomy.dev>
In-Reply-To: <624d9e35-29b8-4012-a3d6-e9b00a9e4485@gmail.com>

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

:)

Show 7 quoted lines

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

Show 16 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted 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. :)

Show 23 quoted lines

> > [...]
> > +		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.

Show 10 quoted lines

> > [...]
> > +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.

Show 10 quoted lines

> > [...]
> > +{
> > +	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.

Show 11 quoted lines

> > [...]
> > +	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.

Show 7 quoted lines

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

Show 11 quoted lines

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