Re: [PATCH 2/3] stripspace: respect repository config
- From
Jeff King <peff@peff.net>
- Date
- Nov 22, 2016, 21:43 UTC
- Message-ID
- <20161122214305.yrn4uqh4dzzafkd2@sigill.intra.peff.net>
- In-Reply-To
- <xmqqeg23p5v3.fsf@gitster.mtv.corp.google.com>
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.
Show 6 quoted lines
> +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.
Show 11 quoted lines
> 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.
Show 11 quoted lines
> +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