Re: [PATCH v2] pathspec: honor `PATHSPEC_PREFIX_ORIGIN` with empty prefix

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3912 bytes --]

On Tue, Apr 04, 2017 at 11:16:56AM +0200, Patrick Steinhardt wrote:
> Previous to commit 5d8f084a5 (pathspec: simpler logic to prefix original
> pathspec elements, 2017-01-04), we were always using the computed
> `match` variable to perform pathspec matching whenever
> `PATHSPEC_PREFIX_ORIGIN` is set. This is for example useful when passing
> the parsed pathspecs to other commands, as the computed `match` may
> contain a pathspec relative to the repository root. The commit changed
> this logic to only do so when we do have an actual prefix and when
> literal pathspecs are deactivated.
> 
> But this change may actually break some commands which expect passed
> pathspecs to be relative to the repository root. One such case is `git
> add --patch`, which now fails when using relative paths from a
> subdirectory. For example if executing "git add -p ../foo.c" in a
> subdirectory, the `git-add--interactive` command will directly pass
> "../foo.c" to `git-ls-files`. As ls-files is executed at the
> repository's root, the command will notice that "../foo.c" is outside
> the repository and fail.
> 
> Fix the issue by again using the computed `match` variable when
> `PATHSPEC_PREFIX_ORIGIN` is set and global literal pathspecs are
> deactivated. Note that in contrast to previous behavior, we will now
> always call `prefix_magic` regardless of whether a prefix is actually
> set. But this is the right thing to do: when the `match` variable has
> been resolved to the repository's root, it will be set to an empty
> string. When passing the empty string directly to other commands, it
> will result in a warning regarding deprecated empty pathspecs. By always
> adding the prefix magic, we will end up with at least the string
> ":(prefix:0)" and thus avoid the warning.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

Just a short reminder on this patch, as I haven't seen it or
v1 being picked up by the What's Cooking reports. Am I simply
being too eager or was this an oversight?

Thanks
Patrick

> 
> This is the second version of [1]. It fixes a bug catched by
> Brandon when the pathspec is resolved to the empty string and
> improves the test a bit to actually catch this issue.
> 
> [1]: http://public-inbox.org/git/1556910880cfce391bdca2d8f0cbcb8c71371691.1491206540.git.ps@pks.im/T/#u
> 
> 
>  pathspec.c                 |  2 +-
>  t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/pathspec.c b/pathspec.c
> index 303efda83..3079a817a 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -505,7 +505,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
>  	 * original. Useful for passing to another command.
>  	 */
>  	if ((flags & PATHSPEC_PREFIX_ORIGIN) &&
> -	    prefixlen && !get_literal_global()) {
> +	    !get_literal_global()) {
>  		struct strbuf sb = STRBUF_INIT;
>  
>  		/* Preserve the actual prefix length of each pattern */
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index f9528fa00..2ecb43a61 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -436,6 +436,28 @@ test_expect_success 'add -p handles globs' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'add -p handles relative paths' '
> +	git reset --hard &&
> +
> +	echo base >relpath.c &&
> +	git add "*.c" &&
> +	git commit -m relpath &&
> +
> +	echo change >relpath.c &&
> +	mkdir -p subdir &&
> +	git -C subdir add -p .. 2>error <<-\EOF &&
> +	y
> +	EOF
> +
> +	test_must_be_empty error &&
> +
> +	cat >expect <<-\EOF &&
> +	relpath.c
> +	EOF
> +	git diff --cached --name-only >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'add -p does not expand argument lists' '
>  	git reset --hard &&
>  
> -- 
> 2.12.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]