Re: [PATCH v1 8/8] sequencer: try to commit without forking 'git commit'

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 7, 2017, 14:09 UTC
Message-ID: <alpine.DEB.2.21.1.1711071509120.6482@virtualbox>
In-Reply-To: <1975badb-7728-45fe-3e8a-8755d85da89a@talktalk.net>

Hi Phillip,

On Tue, 7 Nov 2017, Phillip Wood wrote:

Show 15 quoted lines

> On 07/11/17 01:36, Johannes Schindelin wrote:
> > 
> > On Mon, 6 Nov 2017, Phillip Wood wrote:
> > 
> >> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >>
> >> +static int try_to_commit(struct strbuf *msg, const char *author,
> >> +			 struct replay_opts *opts, unsigned int flags,
> >> +			 struct object_id *oid)
> > 
> > Since this is a file-local function, i.e. not in any way tied to a
> > process exit status, it should probably return -1 in the case of errors,
> > as Git does elsewhere, too.
> 
> It returns -1 in case of error and 1 if it wants git commit to be run.

Ah, that explains a lot! I would like to ask for a code comment above the `try_to_commit()` function to explain that, so that future me will avoid confusion when staring at this code. I would also like to ask for a code comment at the `return 1;` statements, saying e.g. We could not commit in-process, caller should try forking `git commit`.

Show 7 quoted lines

> >> +	if (flags & AMEND_MSG) {
> >> +		const char *exclude_gpgsig[2] = { "gpgsig", NULL };
> > 
> > Git's current source code seems to prefer to infer the array length; The
> > `2` is unnecessary here.
> 
> Right, I copied it from builtin/commit.c but I can change it

Sorry about that. It still would be good to change it, I just wish that you had better code at your fingertips to copy/edit ;-)

Show 9 quoted lines

> >> +	if (commit_tree_extended(msg->buf, msg->len, tree.hash, parents,
> >> +				 oid->hash, author, gpg_sign, extra)) {
> >> +		res = error(_("failed to write commit object"));
> >> +		goto out;
> >> +	}
> 
> Looking more deeply, this can die in write_loose_object(), hopefully
> that is unlikely. git am commits without forking as well so I think it
> is subject to the same problem.

Yes. We will have to address those die() issues. But not necessarily in your patch series; as I said before, this mess is not your fault.

Thanks, Dscho