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

[Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>]

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/624d9e35-29b8-4012-a3d6-e9b00a9e4485@gmail.com/
--
Srinidhi Kaushik