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

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