Re: [PATCH v4 1/3] push: add reflog check for "--force-if-includes"

From: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v4 1/3] push: add reflog check for "--force-if-includes"
Date: Wed, 23 Sep 2020 15:52:15 +0530	[thread overview]
Message-ID: <20200923102215.GA70669@mail.clickyotomy.dev> (raw)
In-Reply-To: <xmqqimc7ezk2.fsf@gitster.c.googlers.com>

Hi Junio,

On 09/21/2020 11:48, Junio C Hamano wrote:
> Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:
> 
> >> If we were talking about older parts of the history, optional
> >> generation numbers could change the equation somewhat, but the
> >> common case for the workflow this series is trying to help is that
> >> these local commits ane the remote tip are relatively new and it is
> >> not unlikely that the generation numbers have not been computed for
> >> them, which is why I suspect that in_merges_many may be a win.
> >
> > Nice! We can definitely try batching commits from the reflog and
> > pass it along to "in_merge_bases_many()". As for being faster than
> > calling "in_merge_bases()" for each commit entry in the reflog --
> > I am not familiar with how the former works. Do we still keep the
> > "reflog_entry_exists()" part? It might still be faster to go through
> > the entries once to check with "oideq()" in the first run.
> 
> That is what I meant.  You go through local reflog entries until you
> find one that is older than the timestamp of the reflog entry of the
> remote-tracking branch, check with oideq() to see if the tip was ever
> directly checked out.  Then, using these same local reflog entries,
> you can make in_merge_bases_many() tranversal to see if any of them
> reach the tip.  I suspect that the number of local reflog entries you
> need to examine would not be too many, so if you can put them all in
> a single array of "struct commit *" pointers in the first "oideq()"
> phase, you may be able to do just a single in_merge_bases_many() batch
> to check for the reachability.

Gotcha.

> > Also, I was wondering if it is worth considering this:
> >   - check if the reflog of the HEAD has the remote ref
> 
> It would help the workflow I had in mind, but it would raise the
> risk of false positives according to Dscho and I tend to agree, so
> I do not know if it is overall a good idea.

Oh, right. This doesn't work when a "git pull --rebase" is run on
a different branch (and a few other cases, as mentioned by Johannes).

> >   - check if the reflog of the local branch has the remote ref
> 
> Isn't that the oideq() test?

Yes.

> >   - check if the remote ref is reachable from any of the local ref's
> >     "reflog" entries using "in_merge_bases_many()" in batches as
> >     suggested here.
> 
> I think it amounts to the same as "does any reflog entry of HEAD
> reach it?" and shares the same issues with false positives as the
> first one.

Hmm, isn't this the same as what was mentioned by you earlier (without
the timestamp:

> [...] Then, using these same local reflog entries,
> you can make in_merge_bases_many() tranversal to see if any of them
> reach the tip.

In v5 (the new patch) [1], the check does this:
  - go through the local reflog until it hits an entry with a timestamp
    older than the remote commit, and doing an "oideq()" check and
    collecting commits into a list along the way.

  - if an exact entry was found, the test passes; otherwise use
    the commit list and make a call to "in_merge_bases_many()" to
    check for reachability, and report it.

> >> > +		deletion:1,
> >> > +		if_includes:1, /* If "--force-with-includes" was specified. */
> >> 
> >> The description needs to be tightened.
> >> 
> >> > +		unreachable:1; /* For "if_includes"; unreachable in reflog. */
> >
> > OK, you're right. Perhaps, we could rename it to something like
> > "if_includes_for_tracking" and update the comment description
> > with saying something along the lines of:
> 
> That is overlong.  Let me try:
> 
> 		/* need to check if local reflog reaches the remote tip */
> 		check_reachable:1,
> 
> 		/* local reflog does not reach the remote tip */
> 		unreachable:1;
> 

I have updated the description in v5 [1]; thanks!

[1]: https://public-inbox.org/git/20200923073022.61293-1-shrinidhi.kaushik@gmail.com/

Thanks.
-- 
Srinidhi Kaushik