From: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v3 1/7] remote: add reflog check for "force-if-includes"
Date: Sat, 19 Sep 2020 22:31:14 +0530 [thread overview]
Message-ID: <20200919170114.GA81372@mail.clickyotomy.dev> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2009161356560.56@tvgsbejvaqbjf.bet>
Hi Johannes,
On 09/16/2020 14:35, Johannes Schindelin wrote:
> > [...]
> > + struct commit *loc = lookup_commit_reference(the_repository,
> > + n_oid);
> > + struct commit *rem = lookup_commit_reference(the_repository,
> > + r_oid);
> > + ret = (loc && rem) ? in_merge_bases(rem, loc) : 0;
> > + }
>
> This chooses the strategy of iterating over the reflog just once, and at
> every step first testing whether the respective reflog entry is identical
> to the remote-tracking branch tip. Only when they are different do we test
> whether the remote-tracking branch tip is at least reachable from the
> reflog entry.
>
> Let's assume that our local branch has 20 reflog entries, and the 4th one
> is identical to the current tip of the remote-tracking branch. Then we
> tested reachability 3 times. But that test is rather expensive.
>
> Therefore, I would have preferred to have a call to
> `for_each_reflog_ent_reverse()` with a callback function that only returns
> the `oideq()` result, and only if the return value of that call is 0, I
> would have wanted to see another call to `for_each_reflog_ent_reverse()`
> to go through, this time looking for reachability.
OK, you're right about this. Will update check to use the two-step
approach.
> > [...]
> > + int ret = 0;
> > + struct commit *r_commit, *l_commit;
> > +
> > + l_commit = lookup_commit_reference(the_repository, l_oid);
> > + r_commit = lookup_commit_reference(the_repository, r_oid);
>
> At this point, we already LOOked up `r_commit`. But we don't pass that to
> `ref_reachable()` at any point (instead passing only `r_oid`), so we have
> to perform the lookup again.
>
> That's wasteful. Shouldn't we pass `r_commit` directly?
Hmm, I suppose we can. Not sure why I did that in the first place.
> With the two-pass strategy I outlined above, the first pass would use
> `r_oid`, and only when the second pass is necessary would we resort to
> calling the expensive reachability check.
>
> > +
> > + /*
> > + * If the remote-tracking ref is an ancestor of the local
> > + * ref (a merge, for instance) there is no need to iterate
> > + * through the reflog entries to ensure reachability; it
> > + * can be skipped to return early instead.
> > + */
> > + ret = (r_commit && l_commit) ? in_merge_bases(r_commit, l_commit) : 0;
>
> Correct me if I am wrong, but isn't the first reflog entry
> (`<remote-branch>@{0}`) identical to `r_commit`? In that case, the first
> iteration of the second pass over the reflog would trivially perform this
> check, and we do not need to duplicate the logic here.
That's a correct assumption; the second pass will check for this.
> > + if (!ret)
> > + ret = for_each_reflog_ent_reverse(local_ref_name, ref_reachable,
> > + (struct object_id *)r_oid);
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > + * Check for reachability of a remote-tracking
> > + * ref in the reflog entries of its local ref.
> > + */
> > +void check_reflog_for_ref(struct ref *r_ref)
> > +{
> > + struct object_id r_oid;
> > + struct ref *l_ref = get_local_ref(r_ref->name);
> > +
> > + if (r_ref->if_includes && l_ref && !read_ref(l_ref->name, &r_oid))
>
> If `r_ref->if_includes` is 0, we do not even have to get the local ref,
> correct? It would make `check_reflog_for_ref()` much easier to read for me
> if it was only called when that flag was already verified to be 1, and
> then followed this structure:
>
> if (!l_ref)
> return;
> if (read_ref(...))
> warning(_("ignoring stale remote branch information ..."));
> else
> r_ref->unreachable = ...
>
> Also, it might make a lot more sense to rename `check_reflog_for_ref()` to
> `check_if_includes_upstream()`, and to rename `r_ref` to `local` and
> `l_ref` to `remote_tracking` or something like that: nothing is inherently
> "left" or "right" about those refs.
Now that I think about it, we don't need te call to "read_ref()" at all
because the reflog will have the OID we need for checking. As to
"{l,r}_ref", they were meant to be [l]ocal and [r]emote. :)
> > + r_ref->unreachable = !ref_reachable_from_reflog(&r_ref->old_oid,
> > + &r_oid,
> > + l_ref->name);
> > +}
> > +
> > static void apply_cas(struct push_cas_option *cas,
> > struct remote *remote,
> > struct ref *ref)
> > {
> > - int i;
> > + int i, is_tracking = 0;
> >
> > /* Find an explicit --<option>=<name>[:<value>] entry */
> > for (i = 0; i < cas->nr; i++) {
> > @@ -2288,16 +2381,26 @@ static void apply_cas(struct push_cas_option *cas,
> > 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
> > + is_tracking = 1;
>
> As part of `remote_tracking()`, we already looked up the branch name.
> Since we need it in the `is_tracking` case, maybe this should not be a
> Boolean anymore but store a copy of the remote-tracking branch name
> instead?
>
> Oh, following the code path all the way down to
> `match_name_with_pattern()`, it seems that `remote_tracking()`'s `dst`
> variable _already_ contains a copy (which means that that memory is
> leaked, right?).
I'm not sure I understand what you mean. The call to "remote_tracking()"
gets the OID of the remote ref and writes it to "old_oid_expect".
We don't need "dst" from there because "old_oid" is sufficient for
checking the reflog entries. However, calling "get_local_ref()" is
necessary to get the local branch name. Also, why does "is_tracking"
have to be a Boolean? We already have "ref->name", right?
Anyway, I think we can get rid of "is_tracking" altogether and just use
"if_includes".
> > [...]
> > +/*
> > + * Runs when "--force-if-includes" is specified.
> > + * Checks if the remote-tracking ref was updated (since checkout)
> > + * implicitly in the background and verify that changes from the
> > + * updated tip have been integrated locally, before pushing.
> > + */
> > +void apply_push_force_if_includes(struct ref*, int);
>
> This function is not even hooked up in this patch, right? I don't think
> that it makes sense to introduce it without a caller, in particular since
> it makes it harder to guess what those parameters might be used for.
>
> In general, it appears to me as if the code worked way too hard to
> accomplish something that should be a lot simpler: when
> `--force-if-includes` is passed, it should piggy-back on top of the
> `--force-with-lease` code path, and just add yet another check on top.
Well, it isn't included in this commit, because it was called from
"transport.c" and "send-pack.c", I decided to put that in another
commit. I will fix that in the next patch series.
> With that in mind, I would have expected something more in line with this:
>
> -- snip --
> struct push_cas_option {
> unsigned use_tracking_for_rest:1;
> struct push_cas {
> struct object_id expect;
> - unsigned use_tracking:1;
> + enum {
> + PUSH_NO_CAS = 0,
> + PUSH_CAS_USE_TRACKING,
> + PUSH_CAS_IF_INCLUDED
> + } mode;
> char *refname;
> } *entry;
> int nr;
> int alloc;
> };
> -- snap --
>
> and then adjusting the respective code paths accordingly.
OK, I will clean it up and get rid of the redundant/wasteful
calls and data being passed around. Regarding the new "enum",
I feel that just having a new bit-field "use_force_if_includes"
might be sufficient for "push_cas_option". The idea is to set it
to "1" when "--force-if-includes" is specified along with
"--force-with-lease", and "apply_push_cas()" will run the check
and set "ref->unreachable" depending on the check status.
To clarify, you would have to specify something like this to
enable the check:
git push --force-if-includes --force-with-lease=master [...]
Then, having a configuration setting "push.useForceIfIncludes" would
be the same as running "git-push" like mentioned above. This new option
will be a "no-op" if specified otherwise. Would that be acceptable?
Thanks again, for a thorough review.
--
Srinidhi Kaushik
next prev parent reply other threads:[~2020-09-19 17:01 UTC|newest]
Thread overview: 120+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-04 18:51 [PATCH] push: make `--force-with-lease[=<ref>]` safer 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
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 [this message]
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=20200919170114.GA81372@mail.clickyotomy.dev \
--to=shrinidhi.kaushik@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public 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).