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