Re: [PATCH 1/5] check-ignore: move setup into cmd_check_ignore()

From: Jeff King <peff@peff.net>
Date: Apr 11, 2013, 05:25 UTC
Message-ID: <20130411052553.GA28915@sigill.intra.peff.net>
In-Reply-To: <1365645575-11428-1-git-send-email-git@adamspiers.org>

On Thu, Apr 11, 2013 at 02:59:31AM +0100, Adam Spiers wrote:

Show 26 quoted lines

> Initialisation of the dir_struct and path_exclude_check structs was
> previously done within check_ignore().  This was acceptable since
> check_ignore() was only called once per check-ignore invocation;
> however the next commit will convert it into an inner loop which is
> called once per line of STDIN when --stdin is given.  Therefore moving
> the initialisation code out into cmd_check_ignore() ensures that
> initialisation is still only performed once per check-ignore
> invocation, and consequently that the output is identical whether
> pathspecs are provided as CLI arguments or via STDIN.
> 
> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>  builtin/check-ignore.c | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
> index 0240f99..0a4eef1 100644
> --- a/builtin/check-ignore.c
> +++ b/builtin/check-ignore.c
> @@ -53,30 +53,20 @@ static void output_exclude(const char *path, struct exclude *exclude)
>  	}
>  }
>  
> -static int check_ignore(const char *prefix, const char **pathspec)
> +static int check_ignore(struct path_exclude_check check,
> +			const char *prefix, const char **pathspec)

Did you mean to pass the struct by value here? If it is truly a per-path value, shouldn't it be declared and initialized inside here? Otherwise you risk one invocation munging things that the struct points to, but the caller's copy does not know about the change.

In particular, I see that the struct includes a strbuf. What happens when one invocation of check_ignore grows the strbuf, then returns? The copy of the struct in the caller will not know that the buffer it is pointing to is now bogus.

> -static int check_ignore_stdin_paths(const char *prefix)
> +static int check_ignore_stdin_paths(struct path_exclude_check check, const char *prefix)

Ditto here.

-Peff