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

[ZheNing Hu <adlternative@gmail.com>]

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