From: Paul Tan Date: Wed, 22 Jun 2016 09:23:27 GMT Subject: Re: [PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field Message-ID: In-Reply-To: Hi Johannes, On Tue, Jun 21, 2016 at 10:12 PM, Johannes Schindelin wrote: > Hi Paul, > > On Tue, 21 Jun 2016, Paul Tan wrote: > >> On Tue, Jun 21, 2016 at 6:34 PM, Johannes Schindelin >> 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