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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 21, 2016, 14:12 UTC
Message-ID: <alpine.DEB.2.20.1606211555380.22630@virtualbox>
In-Reply-To: <CACRoPnRvp7oguE2w2mcsEZfaX_fni8UhFCdsGQ3ZaijQprSHog@mail.gmail.com>

Hi Paul,

On Tue, 21 Jun 2016, Paul Tan wrote:

Show 12 quoted lines

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

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

It is not really a boolean, no. Sure, at the moment, it happens to return either 0 or 1. You can figure that out by following the call paths all the way to do_diff_combined() or line_log_print().

The key words are: at the moment.

We do find more and more places where library functions call die() in case of errors, and it hurts us. Badly. That is why I, among others, try to remedy the situation by converting these calls to "return error()" statements.

The log_tree functions are prepared for that: they return non-negative values in case of success.

The callers are not really prepared for that, hence my complaints.

Show 8 quoted lines

> > 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 run_diff_cache(). And read_ref_at(). And rerere(). And setup_revisions(). And get_sha1().

> > and several calls to write_state_text() and write_state_bool() with
> > the same issue.
> 
> These functions will die() on error

Indeed. And I do not think that is a good practice.

Show 10 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 :-(

You misunderstood. I am not disappointed in you. *I* did a lousy job. Not only mentoring, but I also obviously failed to make things fun enough for you.

My apologies, Dscho