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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

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