Subject: Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend

From: Junio C Hamano <[email-masked]>
Date: Mon, 09 Jan 2017 14:26:46 -0800

Johannes Schindelin <[email-masked]> writes:

> Changes since v2:
>
> - fixed a TRANSLATORS: comment
> ...
> - replaced a spawned `diff-tree` command by code using the diff functions
>   directly

I just finished skimming the interdiff (the difference between the
result of merging the v2 into 'master' and the result of applying
this series on 'master').  I didn't cross reference it with all the
review comments that I saw on the list for the previous round, I
wouldn't have noticed even if there are issues that still haven't
been addressed.  But what I saw in the interdiff were mostly good
changes.

I however think that the renaming of read_author_script() is totally
backwards from maintainability's point of view.  

Both "am" and "rebase" use an external file whose name is the same
"author-script" (because they are meant to serve the same purpose),
and introduction of new code that is different from what "am" uses
to read it in v2 was bad enough.  If the rename of the function in
this round really means "author-script of 'am' can hold only author
information, but that of 'rebase' can hold anything the end user
desires", it makes things even worse.  If 'rebase' side extends the
file to hold other environment variables (in its own code), users
would wonder why the same extension will not be available in 'am'
side (and vice versa).

I ran out of time for the day and haven't looked at individual
patches yet.  I may find other issues, but from the interdiff, that
was the only thing that I found even worse than the previous round,
and everything else was either the same or has improved.

Thanks.