git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] push: make `--force-with-lease[=<ref>]` safer
Date: Tue, 8 Sep 2020 22:29:00 +0530	[thread overview]
Message-ID: <20200908165900.GC40807@mail.clickyotomy.dev> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2009072119350.56@tvgsbejvaqbjf.bet>

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

  parent reply	other threads:[~2020-09-08 16:59 UTC|newest]

Thread overview: 120+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-04 18:51 Srinidhi Kaushik
2020-09-07 15:23 ` Phillip Wood
2020-09-08 15:48   ` Srinidhi Kaushik
2020-09-07 16:14 ` Junio C Hamano
2020-09-08 16:00   ` Srinidhi Kaushik
2020-09-08 21:00     ` Junio C Hamano
2020-09-07 19:45 ` Johannes Schindelin
2020-09-08 15:58   ` Junio C Hamano
2020-09-09  3:40     ` Johannes Schindelin
2020-09-08 16:59   ` Srinidhi Kaushik [this message]
2020-09-16 11:55     ` Johannes Schindelin
2020-09-08 19:34   ` Junio C Hamano
2020-09-09  3:44     ` Johannes Schindelin
2020-09-10 10:22       ` Johannes Schindelin
2020-09-10 14:44         ` Srinidhi Kaushik
2020-09-11 22:16           ` Johannes Schindelin
2020-09-14 11:06             ` Srinidhi Kaushik
2020-09-14 20:08             ` Junio C Hamano
2020-09-16  5:31               ` Srinidhi Kaushik
2020-09-16 10:20                 ` Johannes Schindelin
2020-09-19 17:48                   ` Junio C Hamano
2020-09-10 14:46         ` Junio C Hamano
2020-09-11 22:17           ` Johannes Schindelin
2020-09-14 20:07             ` Junio C Hamano
2020-09-12 15:04 ` [PATCH v2 0/2] push: make "--force-with-lease" safer Srinidhi Kaushik
2020-09-12 15:04   ` [PATCH v2 1/2] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-12 18:20     ` Junio C Hamano
2020-09-12 21:25       ` Srinidhi Kaushik
2020-09-12 15:04   ` [PATCH v2 2/2] push: enable "forceIfIncludesWithLease" by default Srinidhi Kaushik
2020-09-12 18:22     ` Junio C Hamano
2020-09-12 18:15   ` [PATCH v2 0/2] push: make "--force-with-lease" safer Junio C Hamano
2020-09-12 21:03     ` Srinidhi Kaushik
2020-09-13 14:54   ` [PATCH v3 0/7] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 1/7] remote: add reflog check for "force-if-includes" Srinidhi Kaushik
2020-09-14 20:17       ` Junio C Hamano
2020-09-16 10:51         ` Srinidhi Kaushik
2020-09-14 20:31       ` Junio C Hamano
2020-09-14 21:13       ` Junio C Hamano
2020-09-16 12:35       ` Johannes Schindelin
2020-09-19 17:01         ` Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 2/7] transport: add flag for "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 3/7] send-pack: check ref status for "force-if-includes" Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 4/7] transport-helper: update " Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 5/7] builtin/push: add option "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-16 12:36       ` Johannes Schindelin
2020-09-13 14:54     ` [PATCH v3 6/7] doc: add reference for "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-14 21:01       ` Junio C Hamano
2020-09-16  5:35         ` Srinidhi Kaushik
2020-09-13 14:54     ` [PATCH v3 7/7] t: add tests for "force-if-includes" Srinidhi Kaushik
2020-09-16 12:47     ` [PATCH v3 0/7] push: add "--[no-]force-if-includes" Johannes Schindelin
2020-09-19 17:03   ` [PATCH v4 0/3] " Srinidhi Kaushik
2020-09-19 17:03     ` [PATCH v4 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-09-19 20:03       ` Junio C Hamano
2020-09-21  8:42         ` Srinidhi Kaushik
2020-09-21 18:48           ` Junio C Hamano
2020-09-23 10:22             ` Srinidhi Kaushik
2020-09-23 16:47               ` Junio C Hamano
2020-09-21 13:19         ` Phillip Wood
2020-09-21 16:12           ` Junio C Hamano
2020-09-21 18:11             ` Junio C Hamano
2020-09-23 10:27           ` Srinidhi Kaushik
2020-09-19 17:03     ` [PATCH v4 2/3] push: parse and set flag " Srinidhi Kaushik
2020-09-19 20:26       ` Junio C Hamano
2020-09-19 17:03     ` [PATCH v4 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-09-19 20:42       ` Junio C Hamano
2020-09-23  7:30     ` [PATCH v5 0/3] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-23  7:30       ` [PATCH v5 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-09-23 10:18         ` Phillip Wood
2020-09-23 11:26           ` Srinidhi Kaushik
2020-09-23 16:24           ` Junio C Hamano
2020-09-23 16:29         ` Junio C Hamano
2020-09-23  7:30       ` [PATCH v5 2/3] push: parse and set flag " Srinidhi Kaushik
2020-09-23  7:30       ` [PATCH v5 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-09-23 10:24         ` Phillip Wood
2020-09-26 10:13       ` [PATCH v6 0/3] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-26 10:13         ` [PATCH v6 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-09-26 10:13         ` [PATCH v6 2/3] push: parse and set flag " Srinidhi Kaushik
2020-09-26 10:13         ` [PATCH v6 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-09-26 10:21         ` [PATCH v6 0/3] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-26 11:46         ` [PATCH v7 " Srinidhi Kaushik
2020-09-26 11:46           ` [PATCH v7 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-09-26 23:42             ` Junio C Hamano
2020-09-27 12:27               ` Srinidhi Kaushik
2020-09-26 11:46           ` [PATCH v7 2/3] push: parse and set flag " Srinidhi Kaushik
2020-09-26 11:46           ` [PATCH v7 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-09-27 14:17           ` [PATCH v8 0/3] push: add "--[no-]force-if-includes" Srinidhi Kaushik
2020-09-27 14:17             ` [PATCH v8 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-09-27 14:17             ` [PATCH v8 2/3] push: parse and set flag " Srinidhi Kaushik
2020-09-27 14:17             ` [PATCH v8 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-09-30 12:54               ` Philip Oakley
2020-09-30 14:27                 ` Srinidhi Kaushik
2020-09-28 17:31             ` [PATCH v8 0/3] push: add "--[no-]force-if-includes" Junio C Hamano
2020-09-28 17:46               ` SZEDER Gábor
2020-09-28 19:34                 ` Srinidhi Kaushik
2020-09-28 19:51                   ` Junio C Hamano
2020-09-28 20:00                 ` Junio C Hamano
2020-10-01  8:21             ` [PATCH v9 " Srinidhi Kaushik
2020-10-01  8:21               ` [PATCH v9 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-10-02 13:52                 ` Johannes Schindelin
2020-10-02 14:50                   ` Johannes Schindelin
2020-10-02 16:22                     ` Junio C Hamano
2020-10-02 15:07                   ` Srinidhi Kaushik
2020-10-02 16:41                     ` Junio C Hamano
2020-10-02 19:39                       ` Srinidhi Kaushik
2020-10-02 20:14                         ` Junio C Hamano
2020-10-02 20:58                           ` Srinidhi Kaushik
2020-10-02 21:36                             ` Junio C Hamano
2020-10-02 16:26                   ` Junio C Hamano
2020-10-01  8:21               ` [PATCH v9 2/3] push: parse and set flag " Srinidhi Kaushik
2020-10-01  8:21               ` [PATCH v9 3/3] t, doc: update tests, reference " Srinidhi Kaushik
2020-10-01 15:46               ` [PATCH v9 0/3] push: add "--[no-]force-if-includes" Junio C Hamano
2020-10-01 17:12                 ` Junio C Hamano
2020-10-01 17:54                   ` Srinidhi Kaushik
2020-10-01 18:32                     ` Junio C Hamano
2020-10-02 16:50                     ` Junio C Hamano
2020-10-02 19:42                       ` Srinidhi Kaushik
2020-10-03 12:10               ` [PATCH v10 " Srinidhi Kaushik
2020-10-03 12:10                 ` [PATCH v10 1/3] push: add reflog check for "--force-if-includes" Srinidhi Kaushik
2020-10-03 12:10                 ` [PATCH v10 2/3] push: parse and set flag " Srinidhi Kaushik
2020-10-03 12:10                 ` [PATCH v10 3/3] t, doc: update tests, reference " Srinidhi Kaushik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200908165900.GC40807@mail.clickyotomy.dev \
    --to=shrinidhi.kaushik@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --subject='Re: [PATCH] push: make `--force-with-lease[=<ref>]` safer' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).