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

From: Paul Tan <pyokagan@gmail.com>
Date: Jun 21, 2016, 13:47 UTC
Message-ID: <CACRoPnRvp7oguE2w2mcsEZfaX_fni8UhFCdsGQ3ZaijQprSHog@mail.gmail.com>
In-Reply-To: <cover.1466505222.git.johannes.schindelin@gmx.de>

Hi Johannes,

On Tue, Jun 21, 2016 at 6:34 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 5 quoted lines

> - 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.

Also, the return value of log_tree_commit() is actually a boolean value, not an error status value, isn't it?

> This last point is a bigger issue, actually. There seem to be quite a
> few function calls in builtin/am.c whose return values that might
> indicate errors are flatly ignored. I see two calls to run_diff_index()
> whose return value then goes poof unchecked,

Thanks, future-proofing the builtin/am.c code is good, in case run_diff_index() is updated to not call exit(128) on error in the future.

> and several calls to
> write_state_text() and write_state_bool() with the same issue.

These functions will die() on error, so checking their error code is not really useful. I'm not opposed to changing them to use the write_file_gently() version though, although I don't see a need to unless builtin/am.c is being libified.

Show 9 quoted lines

> And I did
> not even try to review the code to that end, all I wanted was to verify
> that builtin am only has the close_file issue once (it does use it a
> second time, but that one is okay because it then calls
> run_diff_index(), i.e. the diff machinery).
>
> I am embarrassed to admit that these builtin am problems demonstrate
> that I, as a mentor of the builtin am project, failed to help make the
> patches as good as I expected myself to do.

Sorry to disappoint you :-(

Regards, Paul