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

From: Duy Nguyen <pclouds@gmail.com>
Date: May 14, 2019, 12:56 UTC
Message-ID: <CACsJy8CnCoMK7ahjRotKfiC2ic-U0hdhKuQAeF-TtVt8N+bU2Q@mail.gmail.com>
In-Reply-To: <20190514002332.121089-4-sandals@crustytoothpaste.net>

On Tue, May 14, 2019 at 7:24 AM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 25 quoted lines

> diff --git a/builtin/am.c b/builtin/am.c
> index 912d9821b1..340eacbd44 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -441,24 +441,8 @@ static int run_applypatch_msg_hook(struct am_state *state)
>   */
>  static int run_post_rewrite_hook(const struct am_state *state)
>  {
> -       struct child_process cp = CHILD_PROCESS_INIT;
> -       const char *hook = find_hook("post-rewrite");
> -       int ret;
> -
> -       if (!hook)
> -               return 0;
> -
> -       argv_array_push(&cp.args, hook);
> -       argv_array_push(&cp.args, "rebase");
> -
> -       cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
> -       cp.stdout_to_stderr = 1;
> -       cp.trace2_hook_name = "post-rewrite";
> -
> -       ret = run_command(&cp);
> -
> -       close(cp.in);

In the old code, we close cp.in...

Show 7 quoted lines

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

-- 
Duy