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 <shrinidhi.kaushik@xxxxxxxxx> > --- > > Hello, > I picked this up from #leftoverbits over at GitHub [1] from the open > issues list. This idea [2], for a safer `--force-with-lease` was > originally proposed by Johannes on the mailing list. > > [1]: https://github.com/gitgitgadget/git/issues/640 > [2]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.1808272306271.73@xxxxxxxxxxxxxxxxx/ 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
