Hi Phillip,
On Tue, 7 Nov 2017, Phillip Wood wrote:
> On 07/11/17 01:36, Johannes Schindelin wrote:
> >
> > On Mon, 6 Nov 2017, Phillip Wood wrote:
> >
> >> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> >>
> >> +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`.
> >> + 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 ;-)
> >> + 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
