Re: Comparing rebase --am with --interactive via p3400

> Hi Alban,
>  -%<-
> 
> Indeed. I offered these insights in #git-devel (slightly edited):
> 
> This `discard_index()` is in an awfully central location. I am rather
> certain that it would cause problems to just remove it.
> 
> Looking at `do_merge()`: it explicitly discards and re-reads the index if
> we had to spawn a `git merge` process (which we do if a strategy other
> than `recursive` was specified, or if it is an octopus merge). But I am
> wary of other code paths that might not be as careful.
> 
> I see that `do_exec()` is similarly careful.
> 

> One thing I cannot fail to notice: we do not re-read a changed index
> after running the `prepare-commit-msg` hook, or for that matter, any other
> hook. That could even be an old regression from the conversion of the
> interactive rebase to using the sequencer rather than a shell script.
> 
> Further, `reset_merge()` seems to spawn `git reset --merge` without
> bothering to re-read the possibly modified index. Its callees are
> `rollback_single_pick()`, `skip_single_pick()` and `sequencer_rollback()`,
> none of which seem to be careful, either, about checking whether the index
> was modified in the meantime.
> 
> Technically, the in-memory index should also be discarded
> in `apply_autostash()`, but so far, we do not have any callers of that
> function, I don't think, that wants to do anything but release resources
> and exit.
> 
> The `run_git_checkout()` function discards, as intended. I
> am not quite sure whether it needs to, though, unless the `.git/index`
> file _was_ modified (it _is_ possible, after all, to run `git rebase -i
> HEAD`, and I do have a use case for that where one of my scripts generates
> a todo script, sort of a `git cherry-pick --rebase-merges`, because
> `cherry-pick` does not support that mode).
> 
> The `continue_single_pick()` function spawns a `git
> commit` which could potentially modify the index through a hook, but the
> first call site does not care and the second one guards against that
> (erroring out...).
> 
> My biggest concern is with the `run_git_commit()` function: it does not
> re-read a potentially-modified index (think of hooks).

> 
> We will need to be very careful with this `discard_index()`, I think, and
> in my opinion there is a great opportunity here for cleaning up a little:
> rather than discarding and re-reading the in-memory index without seeing
> whether the on-disk index has changed at all appears a bit wasteful to me.
> 
> This could be refactored into a function that only discards and re-reads
> the index if the mtime of `.git/index` changed. That function could then
> also be taught to detect when the in-memory index has unwritten changes:
> that would constitute a bug.
>