Re: [PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field

From: Paul Tan <pyokagan@gmail.com>
Date: Jun 22, 2016, 09:23 UTC
Message-ID: <CACRoPnTYjZzVe7iKTu_nzosbgm19fAq9csFiZs3rvEPqkAusPQ@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1606211555380.22630@virtualbox>

Hi Johannes,

On Tue, Jun 21, 2016 at 10:12 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 19 quoted lines

> Hi Paul,
>
> On Tue, 21 Jun 2016, Paul Tan wrote:
>
>> On Tue, Jun 21, 2016 at 6:34 PM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>> > - this uncovered a problem with builtin am, where it asked the diff
>> >   machinery to close the file stream, but actually called the log_tree
>> >   machinery (which might mean that this patch series inadvertently fixes
>> >   a bug where `git am --rebasing` would write the commit message to
>> >   stdout instead of the `patch` file when erroring out)
>>
>> Please correct me if I'm wrong: looking at log-tree.c, the commit
>> message will not be printed when no_commit_id = 1, isn't it?
>> This is because we do not hit the code paths that write to stdout since
>> show_log() is not called.
>
> Why does builtin/am.c use log_tree_commit(), then? Why not simply run
> things through the diff machinery?

It's because git-am.sh called "git diff-tree", which in turn calls log_tree_commit(), so to be safe I used the same function to ensure that there were no unintended behavioral changes. e.g. what happened with 3ecc704 (am --skip/--abort: merge HEAD/ORIG_HEAD tree into index, 2015-08-19)

Of course, it may be true that the diff machinery should be called directly, although the code is quite involved so I can't really tell the impact the change will have.

Thanks, Paul