Re: [PATCH 2/3] stripspace: respect repository config

[Jeff King <peff@peff.net>]

On Tue, Nov 22, 2016 at 01:22:24PM -0800, Junio C Hamano wrote:

> OK.  The "mailinfo" part turns out a bit more than RUN_SETUP_GENTLY
> as it takes paths from the command line that needs to be adjusted
> for the prefix.

I wondered at first if our lack of parse-options here was holding us
back from using OPT_FILENAME. Though even if that was the case, it is
probably better to do the minimal fix in this instance, rather than
converting the option parsing.

However, the paths which need munged are not even options, they are
arguments. So we wouldn't be able to rely on OPT_FILENAME anyway. I'm
surprised we haven't had to deal with this elsewhere, but I guess most
commands don't take arbitrary filenames (things like `add` take
pathspecs, and then the pathspec machinery takes care of the details).

The only other case I could think of is git-apply, and it does use
prefix_filename() correctly.

> +static char *prefix_copy(const char *prefix, const char *filename)
> +{
> +	if (!prefix || is_absolute_path(filename))
> +		return xstrdup(filename);
> +	return xstrdup(prefix_filename(prefix, strlen(prefix), filename));
> +}

So this is more or less a copy of parse-option's fix_filename(), which
makes sense.

I noticed that git-apply does not check is_absolute_path(), but that's
OK, as prefix_filename() does. So I think it would be correct to drop it
here, but it doesn't matter much either way.

>  int cmd_mailinfo(int argc, const char **argv, const char *prefix)
>  {
>  	const char *def_charset;
>  	struct mailinfo mi;
>  	int status;
> +	char *msgfile, *patchfile;
>  
> -	/* NEEDSWORK: might want to do the optional .git/ directory
> -	 * discovery
> -	 */
>  	setup_mailinfo(&mi);

This part looks straightforward and correct. Dropping the NEEDSWORK is a
nice bonus.

> +test_expect_success 'mailinfo with mailinfo.scissors config' '
> +	test_config mailinfo.scissors true &&
> +	(
> +		mkdir sub &&
> +		cd sub &&
> +		git mailinfo ../msg0014.sc ../patch0014.sc <../0014 >../info0014.sc
> +	) &&
> +	test_cmp "$DATA/msg0014--scissors" msg0014.sc &&
> +	test_cmp "$DATA/patch0014--scissors" patch0014.sc &&
> +	test_cmp "$DATA/info0014--scissors" info0014.sc
> +'

And this test makes sense. Even without "sub", it would show the
regression, but it's a good idea to test the sub-directory case to cover
the path-munging.

In the "archive --remote" test I added, we may want to do the same to
show that "--output" points at the correct path.

-Peff