apply: strip ./ prefix from --directory argument

From: Joaquim Rocha <[email protected]>

When passing a relative path like --directory=./some/sub, the leading "./" caused apply to prepend it literally to patch filenames, resulting in an error (invalid path).

Since using "./" is almost memory muscle for many, strip the "./" prefix so it behaves the same as --directory=some/sub.

---
apply: strip ./ prefix from --directory argument

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2198%2Fjoaquimrocha%2Fapply-directory-dot-slash-prefix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2198/joaquimrocha/apply-directory-dot-slash-prefix-v1
Pull-Request: https://github.com/git/git/pull/2198

 apply.c               | 4 ++++
 t/t4128-apply-root.sh | 9 +++++++++
 2 files changed, 13 insertions(+)

index 3de4aa4d2e..a44c54077c 100644
@@ -5001,6 +5001,10 @@ static int apply_option_parse_directory(const struct option *opt,
 	BUG_ON_OPT_NEG(unset);
 
 	strbuf_reset(&state->root);
+
+	if (starts_with(arg, "./"))
+		arg += 2;
+
 	strbuf_addstr(&state->root, arg);
 	strbuf_complete(&state->root, '/');
 	return 0;

index f6db5a79dd..2f446a4d69 100755
@@ -43,6 +43,15 @@ test_expect_success 'apply --directory -p (2) ' '
 
 '
 
+test_expect_success 'apply --directory (./ prefix)' '
+	git reset --hard initial &&
+	git apply --directory=./some/sub -p3 --index patch &&
+	echo Bello >expect &&
+	git show :some/sub/dir/file >actual &&
+	test_cmp expect actual &&
+	test_cmp expect some/sub/dir/file
+'
+
 cat > patch << EOF
 diff --git a/newfile b/newfile
 new file mode 100644

base-commit: 6fcee4785280a08e7f271bd015a4dc33753e2886

gitgitgadget

Isn't the problem wider than that though? For example, if you had "././some/sub" it would break again. Or if you had "some/./sub", or "some/sub/../sub", or "some//sub".

index 9de2eb953e..8946b133a3 100644
@@ -5002,6 +5002,7 @@ static int apply_option_parse_directory(const struct option *opt,
 
 	strbuf_reset(&state->root);
 	strbuf_addstr(&state->root, arg);
+	strbuf_normalize_path(&state->root);
 	strbuf_complete(&state->root, '/');
 	return 0;
 }

Sorry, but I am confused. Why isn't "don't do it then" a good answer for a case like this?

From: Joaquim Rocha <[email protected]>

When passing a relative path like --directory=./some/sub, the leading "./" caused apply to prepend it literally to patch filenames, resulting in an error (invalid path).
There may be more cases like this where users pass some/./path to the directory which can easily be normalized to an acceptable path, so these changes try to normalize the path before using it.

---
apply: strip ./ prefix from --directory argument

Changes since v1:

* Normalized the path as Patrick recommended
* Return error is we have ../ at the beginning of the --directory argument

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2198%2Fjoaquimrocha%2Fapply-directory-dot-slash-prefix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2198/joaquimrocha/apply-directory-dot-slash-prefix-v2
Pull-Request: https://github.com/git/git/pull/2198

Range-diff vs v1:

1: 82e24c3471 ! 1: 8ac66a876d apply: strip ./ prefix from --directory argument

     @@ Metadata
      Author: Joaquim Rocha <[email protected]>
      
       ## Commit message ##
     -    apply: strip ./ prefix from --directory argument
     +    apply: normalize path in --directory argument
      
          When passing a relative path like --directory=./some/sub, the leading
          "./" caused apply to prepend it literally to patch filenames, resulting
          in an error (invalid path).
     -
     -    Since using "./" is almost memory muscle for many, strip the "./"
     -    prefix so it behaves the same as --directory=some/sub.
     +    There may be more cases like this where users pass some/./path to the
     +    directory which can easily be normalized to an acceptable path, so
     +    these changes try to normalize the path before using it.
      
          Signed-off-by: Joaquim Rocha <[email protected]>
      
       ## apply.c ##
      @@ apply.c: static int apply_option_parse_directory(const struct option *opt,
     - 	BUG_ON_OPT_NEG(unset);
       
       	strbuf_reset(&state->root);
     + 	strbuf_addstr(&state->root, arg);
      +
     -+	if (starts_with(arg, "./"))
     -+		arg += 2;
     ++	if (strbuf_normalize_path(&state->root) < 0)
     ++		return error(_("unable to normalize directory: '%s'"), arg);
      +
     - 	strbuf_addstr(&state->root, arg);
       	strbuf_complete(&state->root, '/');
       	return 0;
     + }
      
       ## t/t4128-apply-root.sh ##
      @@ t/t4128-apply-root.sh: test_expect_success 'apply --directory -p (2) ' '
     @@ t/t4128-apply-root.sh: test_expect_success 'apply --directory -p (2) ' '
      +	test_cmp expect actual &&
      +	test_cmp expect some/sub/dir/file
      +'
     ++
     ++test_expect_success 'apply --directory (double slash)' '
     ++	git reset --hard initial &&
     ++	git apply --directory=some//sub -p3 --index patch &&
     ++	echo Bello >expect &&
     ++	git show :some/sub/dir/file >actual &&
     ++	test_cmp expect actual &&
     ++	test_cmp expect some/sub/dir/file
     ++'
     ++
     ++test_expect_success 'apply --directory (./ in the middle)' '
     ++	git reset --hard initial &&
     ++	git apply --directory=some/./sub -p3 --index patch &&
     ++	echo Bello >expect &&
     ++	git show :some/sub/dir/file >actual &&
     ++	test_cmp expect actual &&
     ++	test_cmp expect some/sub/dir/file
     ++'
     ++
     ++test_expect_success 'apply --directory (../ in the middle)' '
     ++	git reset --hard initial &&
     ++	git apply --directory=some/../some/sub -p3 --index patch &&
     ++	echo Bello >expect &&
     ++	git show :some/sub/dir/file >actual &&
     ++	test_cmp expect actual &&
     ++	test_cmp expect some/sub/dir/file
     ++'
     ++
     ++test_expect_success 'apply --directory rejects leading ../' '
     ++	test_must_fail git apply --directory=../foo -p3 patch 2>err &&
     ++	test_grep "unable to normalize directory" err
     ++'
      +
       cat > patch << EOF
       diff --git a/newfile b/newfile

 apply.c               |  4 ++++
 t/t4128-apply-root.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

index 3de4aa4d2e..7f0ce5918b 100644
@@ -5002,6 +5002,10 @@ static int apply_option_parse_directory(const struct option *opt,
 
 	strbuf_reset(&state->root);
 	strbuf_addstr(&state->root, arg);
+
+	if (strbuf_normalize_path(&state->root) < 0)
+		return error(_("unable to normalize directory: '%s'"), arg);
+
 	strbuf_complete(&state->root, '/');
 	return 0;
 }

base-commit: 6fcee4785280a08e7f271bd015a4dc33753e2886

gitgitgadget

I guess that's fair, but especially with command line completion it might be easy to arrive at such paths. That may or may not be a good argument, I'm not sure myself.

Patrick

I understand that. TBH, while not at your (plural) level of expertise in git, I am certainly an experienced user and still the error from the ./ use got me confused. So my proposal goes towards developer experience, like the similar command suggestions it also has. If there's no down side to it, I think it makes sense to have it.

An alternative would be to have a better error message if we detect there are redundant path artifacts, but in that case why wouldn't we just normalize the path as it's the user's clear intention?

Cheers,

Joaquim Rocha

Sounds like a sensible direction to go.

Will queue. Thanks. (of course, further reviews welcome).