Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 21, 2017, 05:10 UTC
Message-ID: <xmqq7ewsljrl.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170921044112.GB91069@aiede.mtv.corp.google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 33 quoted lines

> diff --git a/pathspec.c b/pathspec.c
> index e2a23ebc96..cdefdc7cc0 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -526,10 +526,6 @@ static void NORETURN unsupported_magic(const char *pattern,
>  	    pattern, sb.buf);
>  }
>  
> -/*
> - * Given command line arguments and a prefix, convert the input to
> - * pathspec. die() if any magic in magic_mask is used.
> - */
>  void parse_pathspec(struct pathspec *pathspec,
>  		    unsigned magic_mask, unsigned flags,
>  		    const char *prefix, const char **argv)
> diff --git a/pathspec.h b/pathspec.h
> index 60e6500401..6420d1080a 100644
> --- a/pathspec.h
> +++ b/pathspec.h
> @@ -70,6 +70,13 @@ struct pathspec {
>   */
>  #define PATHSPEC_LITERAL_PATH (1<<6)
>  
> +/*
> + * Given command line arguments and a prefix, convert the input to
> + * pathspec. die() if any magic in magic_mask is used.
> + *
> + * Any arguments used are copied. It is safe for the caller to modify
> + * or free 'prefix' and 'args' after calling this function.
> + */
>  extern void parse_pathspec(struct pathspec *pathspec,
>  			   unsigned magic_mask,
>  			   unsigned flags,

Obviously the extra text is better than not having any, but I somehow found "Any arguments used" a bit unsatisfactory. magic_mask and flags are probably also copied ;-) but I wonder if *pathspec is also copied? The second sentence that singles out 'prefix' and 'args' helps to remove such a confusion from a clueless reader like me, and makes me wonder if can take advantage of the existence of them in a more direct way.

	It is safe for the caller to modify or free 'prefix' and
	'args' after calling this function, as copies of them are
	stored in the pathspec structure.

or something like that, perhaps. Adding "(which is freed by calling clear_pathspec())" at the end might even better, as that function does not currently have any docstring.