Re: [PATCH v2 3/7] rebase: add support for multiple hooks

[Duy Nguyen <pclouds@gmail.com>]

On Thu, May 16, 2019 at 5:55 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On Tue, May 14, 2019 at 07:56:49PM +0700, Duy Nguyen wrote:
> > On Tue, May 14, 2019 at 7:24 AM brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> > > -       close(cp.in);
> >
> > In the old code, we close cp.in...
> >
> > > +int post_rewrite_rebase_hook(const char *name, const char *path, void *input)
> > > +{
> > > +       struct child_process child = CHILD_PROCESS_INIT;
> > > +
> > > +       child.in = open(input, O_RDONLY);
> > > +       child.stdout_to_stderr = 1;
> > > +       child.trace2_hook_name = "post-rewrite";
> >
> > maybe use "name" and avoid hard coding "post-rewrite".
> >
> > > +       argv_array_push(&child.args, path);
> > > +       argv_array_push(&child.args, "rebase");
> > > +       return run_command(&child);
> >
> > ... but in the new one we don't. Smells fd leaking to me.
>
> Ah, good point.  Will fix.

Actually I think Johannes is right (in the other mail, same thread)
that the old code is buggy.

The only good thing is, I don't think the old code could actually
result in double close() problem. It would just return EBADF, which we
don't care. So no hurry fixing the old code until you replace it with
a better one.

-- 
Duy