[PATCH] ls-files: fix "--format" output of relative paths

[PATCH] ls-files: fix "--format" output of relative paths

[Adam Johnson via GitGitGadget]

From: Adam Johnson <me@adamj.eu>

Fix a bug introduced with the "--format" option in
ce74de931d, where relative paths were computed
using the output buffer, which could lead to
random data in the output.

Signed-off-by: Adam Johnson <me@adamj.eu>
---
    ls-files: fix "--format" output of relative paths
    
    ce74de931d introduced with the "--format" option to ls-files. This
    commit had a bug: using --format='%(path)' with the "top" pathspec from
    within a subdirectory would lead to random memory being added to the
    output. For example, within the Documentation/ directory in Git’s own
    repository:
    
    $ git ls-files --format='%(path)' ':/' | head -n 2
    ../.cirrus.yml�뻤��
    ../.clang-format�뻤��
    
    
    This is due to reuse of a string buffer for calculating the relative
    path. The attached patch fixes this by using a fresh buffer, the same
    pattern used in other places where relative paths are computed.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1465%2Fadamchainz%2Ffix-ls-files-format-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1465/adamchainz/fix-ls-files-format-v1
Pull-Request: https://github.com/git/git/pull/1465

 builtin/ls-files.c         |  5 ++++-
 t/t3013-ls-files-format.sh | 16 ++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 4cf8a236483..02b9bbe7eb4 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -89,12 +89,15 @@ static void write_name(const char *name)
 
 static void write_name_to_buf(struct strbuf *sb, const char *name)
 {
-	const char *rel = relative_path(name, prefix_len ? prefix : NULL, sb);
+	struct strbuf buf = STRBUF_INIT;
+	const char *rel = relative_path(name, prefix_len ? prefix : NULL, &buf);
 
 	if (line_terminator)
 		quote_c_style(rel, sb, NULL, 0);
 	else
 		strbuf_addstr(sb, rel);
+
+	strbuf_release(&buf);
 }
 
 static const char *get_tag(const struct cache_entry *ce, const char *tag)
diff --git a/t/t3013-ls-files-format.sh b/t/t3013-ls-files-format.sh
index efb7450bf1e..ef6fb53f7f1 100755
--- a/t/t3013-ls-files-format.sh
+++ b/t/t3013-ls-files-format.sh
@@ -54,6 +54,22 @@ test_expect_success 'git ls-files --format path v.s. -s' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git ls-files --format with relative path' '
+	cat >expect <<-\EOF &&
+	../o1.txt
+	../o2.txt
+	../o3.txt
+	../o4.txt
+	../o5.txt
+	../o6.txt
+	EOF
+	mkdir sub &&
+	cd sub &&
+	git ls-files --format="%(path)" ":/" >../actual &&
+	cd .. &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git ls-files --format with -m' '
 	echo change >o1.txt &&
 	cat >expect <<-\EOF &&

base-commit: 768bb238c4843bf52847773a621de4dffa6b9ab5
-- 
gitgitgadget

Re: [PATCH] ls-files: fix "--format" output of relative paths

[ZheNing Hu]

Adam Johnson via GitGitGadget <gitgitgadget@gmail.com> 于2023年3月10日周五 21:04写道：
>
> From: Adam Johnson <me@adamj.eu>
>
> Fix a bug introduced with the "--format" option in
> ce74de931d, where relative paths were computed
> using the output buffer, which could lead to
> random data in the output.
>
> Signed-off-by: Adam Johnson <me@adamj.eu>
> ---
>     ls-files: fix "--format" output of relative paths
>
>     ce74de931d introduced with the "--format" option to ls-files. This
>     commit had a bug: using --format='%(path)' with the "top" pathspec from
>     within a subdirectory would lead to random memory being added to the
>     output. For example, within the Documentation/ directory in Git’s own
>     repository:
>
>     $ git ls-files --format='%(path)' ':/' | head -n 2
>     ../.cirrus.yml�뻤��
>     ../.clang-format�뻤��
>
>
>     This is due to reuse of a string buffer for calculating the relative
>     path. The attached patch fixes this by using a fresh buffer, the same
>     pattern used in other places where relative paths are computed.
>

Good catch!

> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1465%2Fadamchainz%2Ffix-ls-files-format-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1465/adamchainz/fix-ls-files-format-v1
> Pull-Request: https://github.com/git/git/pull/1465
>
>  builtin/ls-files.c         |  5 ++++-
>  t/t3013-ls-files-format.sh | 16 ++++++++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 4cf8a236483..02b9bbe7eb4 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -89,12 +89,15 @@ static void write_name(const char *name)
>
>  static void write_name_to_buf(struct strbuf *sb, const char *name)
>  {
> -       const char *rel = relative_path(name, prefix_len ? prefix : NULL, sb);
> +       struct strbuf buf = STRBUF_INIT;
> +       const char *rel = relative_path(name, prefix_len ? prefix : NULL, &buf);
>
>         if (line_terminator)
>                 quote_c_style(rel, sb, NULL, 0);
>         else
>                 strbuf_addstr(sb, rel);
> +
> +       strbuf_release(&buf);
>  }
>

Here I misused the outer strbuf.

>  static const char *get_tag(const struct cache_entry *ce, const char *tag)
> diff --git a/t/t3013-ls-files-format.sh b/t/t3013-ls-files-format.sh
> index efb7450bf1e..ef6fb53f7f1 100755
> --- a/t/t3013-ls-files-format.sh
> +++ b/t/t3013-ls-files-format.sh
> @@ -54,6 +54,22 @@ test_expect_success 'git ls-files --format path v.s. -s' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'git ls-files --format with relative path' '
> +       cat >expect <<-\EOF &&
> +       ../o1.txt
> +       ../o2.txt
> +       ../o3.txt
> +       ../o4.txt
> +       ../o5.txt
> +       ../o6.txt
> +       EOF
> +       mkdir sub &&
> +       cd sub &&
> +       git ls-files --format="%(path)" ":/" >../actual &&
> +       cd .. &&
> +       test_cmp expect actual
> +'
> +
>  test_expect_success 'git ls-files --format with -m' '
>         echo change >o1.txt &&
>         cat >expect <<-\EOF &&
>
> base-commit: 768bb238c4843bf52847773a621de4dffa6b9ab5
> --
> gitgitgadget


I'm glad you caught that mistake.

Thanks.
--
ZheNing Hu

Re: [PATCH] ls-files: fix "--format" output of relative paths

[Junio C Hamano]

"Adam Johnson via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Adam Johnson <me@adamj.eu>
>
> Fix a bug introduced with the "--format" option in
> ce74de931d, where relative paths were computed
> using the output buffer, which could lead to
> random data in the output.

Good find.  Both the explanation and the patch text are crystal
clear.  Nicely done.

I'll replace "ce74de9313" above with "ce74de93 (ls-files: introduce
"--format" option, 2022-07-23)" while queuing, though.