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

From: Junio C Hamano <gitster@pobox.com>
To: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v4 1/3] push: add reflog check for "--force-if-includes"
Date: Mon, 21 Sep 2020 11:48:29 -0700	[thread overview]
Message-ID: <xmqqimc7ezk2.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200921084231.GA64896@mail.clickyotomy.dev> (Srinidhi Kaushik's message of "Mon, 21 Sep 2020 14:12:31 +0530")

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.

> 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.

>   - check if the reflog of the local branch has the remote ref

Isn't that the oideq() test?

>   - 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.

>> > +		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;

Thans.