Re: [RFC PATCH 3/3] git-rebase.sh: make git-rebase--interactive the default

Hi Elijah,

On Sat, 9 Jun 2018, Elijah Newren wrote:

On Sat, Jun 9, 2018 at 3:11 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

On Thu, 7 Jun 2018, Elijah Newren wrote:

quoted

am-based rebases suffer from an reduced ability to detect directory
renames upstream, which is fundamental to the fact that it throws
away information about the history: in particular, it dispenses with
the original commits involved by turning them into patches, and
without the knowledge of the original commits we cannot determine a
proper merge base.

The am-based rebase will proceed by first trying git-apply, and, only
if it fails, will try to reconstruct a provisional merge base using
build_fake_ancestor().  This fake ancestor will ONLY contain the
files modified in the patch; without the full list of files in the
real merge base, renames of any missing files cannot be detected.
Directory rename detection works by looking at individual file
renames and deducing when a full directory has been renamed.

Trying to modify build_fake_ancestor() to instead create a merge base
that includes common file information by looking for a commit that
contained all the same blobs as the pre-image of the patch would
require a very expensive search through history, may find the wrong
commit, and outside of rebase may not find any commit that matches.

But we had all the relevant information to start.  So, instead of
attempting to fix am which just doesn't have the relevant
information, instead note its strength and weaknesses, and change the
default rebase machinery to interactive since it does not suffer from
the same problems.

I'll let Eric comment on the grammar, and I'll comment on the idea
behind this commit instead.

Going to dump the hard job on Eric, eh?  ;-)

Just trying to learn how to delegate.

:0)

quoted

IMHO `git rebase -i` is still too slow to be a true replacement for
`git rebase --am` for the cases where it serves the user well. Maybe
we should work on making `rebase -i` faster, first?

That sounds fair.

quoted

I imagine, for example, that it might make *tons* of sense to avoid
writing out the index and worktree files all the time. That was
necessary in the shell script version because if the ridiculous
limitations we subjected ourselves to, such as: no object-oriented
state worth mentioning, only string-based processing, etc. But we
could now start to do everything in memory (*maybe* write out the new
blob/tree/commit objects immediately, but maybe not) until the time
when we either have succeeded in the rebase, or when there was a
problem and we have to exit with an error. And only then write the
files and the index.

Hmm...are you still planning on using cherry-pick (internally rather
than forking, of course)?

The sequencer side-steps the cherry-pick command by calling
merge_recursive() directly. But yes, this is what the interactive rebase
will always use.

 Because cherry-pick uses the merge-recursive machinery, and the
 merge-recursive machinery doesn't have a nice way of avoiding writing
 to the working tree or index.

Exactly. I think it could be taught to perform its magic in memory. I am
not saying that it is necessarily easy to teach merge-recursive.c this
trick, but it should be possible.

Fixing that is on my radar; see the first block of
https://public-inbox.org/git/CABPp-BG2fZHm3s-yrzxyGj3Eh+O7_LHLz5pgstHhG2drigSyRA@mail.gmail.com/

Great!

(reading up until "At this point, I'd rather just fix the design flaw
rather than complicate the code further.")

However, also covered in my plans is a few things to speed up the
merge-recursive machinery, which should provide some other performance
benefits for interactive rebases.

I am looking forward to see this materialize.

quoted

In any case, I think that the rather noticeable change of the default
would probably necessitate a deprecation phase.

Why is it a "rather noticable change of the default"?

I was really referring to speed. But I have to admit that I do not have
any current numbers.

Another issue just hit me, though: rebase --am does not need to look at as
many Git objects as rebase --merge or rebase -i. Therefore, GVFS users
will still want to use --am wherever possible, to avoid "hydrating"
many objects during their rebase.

If we popped up the editor and asked the user to edit the list of
options, I'd agree, or if folks thought that it was significantly slower
by a big enough margin (though you already suggested waiting and making
sure we don't do that).  What else remains that qualifies?

(Okay, the default behavior to just skip empty patches rather than halt
the rebase and ask the user to advise is different, but we could fix
that up too.  Is there anything else?)

That is indeed a change in behavior that is rather easy to address.

As to speed: that might be harder. But then, the performance might already
be good enough. I do not have numbers (nor the time to generate them) to
back up my hunch that --am is substantially faster than --merge.

Ciao,
Dscho