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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 13, 2016, 10:50 UTC
Message-ID: <alpine.DEB.2.20.1610131242100.197091@virtualbox>
In-Reply-To: <xmqq60ox5wxb.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Wed, 12 Oct 2016, Junio C Hamano wrote:

Show 12 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Hmph, didn't we recently add parse_key_value_squoted() to build
> >> read_author_script() in builtin/am.c on top of it, so that this
> >> piece of code can also take advantage of and share the parser?
> >
> > I already pointed out that the author-script file may *not* be quoted.
> 
> 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.

Come to think of it, there is a bigger problem here, as users might have abused the author-script to execute commands in rebase -i's own context. Not sure we can do anything about that.

But the point stands, if anybody used unquoted, or differently quoted, values in author-script, we should at least attempt within reason to support that.

> I am not sure what you meant by "nominally related", but the purpose
> of the author-script in these two codepaths is the same, isn't it?

The purpose is, but the means aren't. As I pointed out above, the interactive rebase is inherently much more interactive, and needs to be much more forgiving in its input, than `git am`.

Show 11 quoted lines

> Somebody leaves the author information from the source (either from
> an e-mailed patch or an existing commit), so that a later step can
> use that pieces of information left in the file when (re)creating a
> commit to record the tree made by using pieces of information from
> the source.
> 
> Are our use in the author-script in these two codepaths _already_
> inconsistent?  IOW, "am" never writes malformed unquoted values,
> while the sequencer writes out in a way that is randomly quoted or
> not quoted, iow, if you fed such an author-file to "am", it wouldn't
> understand it?

We heed Postel's Law: what the sequencer writes is in a very strict format, but what the sequencer accepts need not be.

Show 5 quoted lines

> I fully support your position to use different codepaths, if the
> file that has the same name and that is used for the same purpose
> uses different format in these two separate codepaths and the users
> already expect them to be different.  We obviously need to have two
> separate parsers.

Well, traditionally we *do* have separate parsers. I do not say that we need to keep that, but given what I said above, it might not be a bad idea to keep the lenient parser required by `git rebase -i` separate from the one used by `git am` so that the latter can be faster (by making assumptions the other parser cannot).

Show 13 quoted lines

> But if that is not the case, IOW, if "am"'s author-script shares the
> same issue (i.e. "'am' initially writes the file properly quoted,
> but this or that can happen to change its quoting and we need to
> read from such a file"), then perhaps sharing needs to happen the
> other way around?  This patch may prepare "rebase -i" side for the
> "this or that" (I still do not know what they are) to allow the
> resulting file read correctly, but the same "this or that" can break
> what "am" has used and is in use there if that is the case, no?
> 
> 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, and doesn't "am" share the
> same issue?

The fact that `git am` is *non-interactive*.

Ciao, Dscho