Re: [PATCH v3 13/25] sequencer: prepare for rebase -i's commit functionality

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I think my puzzlement comes from here.  What makes it OK for "am" to
>> expect the contents of author-script file to be quoted but it is not
>> OK to expect the same here?  What makes it not quoted for _this_
>> reader, in other words?
>
> The `git am` command is inherently *not* interactive, while the
> interactive rebase, well, is.
>
> As such, we must assume that enterprisey users did come up with scripts
> that edit, or create, author-script files, and exploited the fact that the
> interactive rebase previously sourced them.

It's like saying "While the program is waiting for user interaction,
the user can attach a debugger to tweak the contents of this string
variable.  Hence we must not assume that the string is always NUL
terminated to protect ourselves."

A correct response to such a user is to tell them not to do that,
and do so at two levels.  

The first level may be "it may be physically possible to tweak the
string via debugger but don't do that in such a way that breaks the
invariants our program relies on, or you are on your own", but more
important is the response at the second level.  Why is the user
futzing with that string in the first place with the debugger?  In
the context of author-script, the question is "Why is the user
futzing with the author-script file"?  To attribute the resulting
commit to a different author, of course.  But we need to step back
and think why the user needs to resort to editing that file to
achieve that.

If that is a common thing users would want to do, then we should
offer an official way to do so, and it should not be "you can futz
with author-script file with your editor".  Something like "have
'exec git commit --amend' in the todo file" may be more appropriate
and if it is important enough, the sequencer command language may
want to learn an extra verb to update the author.

Besides, the opportunity easiest for the user to futz with the
contents of author-script file (or "attach the debugger while we
wait for user interaction") arises when "rebase -i" or "am" stops
waiting for conflict resolution.  Even when you run "am" without its
"-i" option, it is equally susceptible to the "user futzing with the
file", which means your "am is not interactive but 'rebase -i' is"
is irrelevant to the issue.  Oh, also, did I say "am" has "-i"
option, which is a short-hand for "--interactive"?

What disturbs me the most is that I know you know the system well
enough to realize how bogus your argument to claim that "rebase -i"
and "am" are different was and to come up with what I wrote above
yourself.  Which means that I need to conclude that you have some
other reasons why you want to keep this parser different, but I
still do not know what they are.

I guess it entirely is possible that one of the reasons is because
some later patches in the larger "rebase-i to sequencer" series
writes author-script file in a syntax that cannot be read by the
recently refactored code "am" uses to read the author-script file,
and reusing the existing code may end up breaking the endgame
"rebase -i" you have.

As I do not feel like arguing with you on this any longer, and as I
certainly do not want to be blamed for breaking your "rebase -i", I
do not insist the code to be refactored to share with the existing
codepath.  But still I do not see the need to keep them separate (as
I already said in the previous message, I am OK if the one used in
"am" is updated to match).