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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 21, 2020, 18:48 UTC
Message-ID: <xmqqimc7ezk2.fsf@gitster.c.googlers.com>
In-Reply-To: <20200921084231.GA64896@mail.clickyotomy.dev>

Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:

Show 13 quoted lines

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

Show 10 quoted lines

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