{"thread":{"id":"59090","subject":"[PATCH] ls-tree: fix expansion of repeated %(path)","startedAt":"2023-01-14T14:38:05Z","lastAt":"2023-01-14T18:24:32Z","messageCount":4,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470340","messageId":"59f0a3f8-2dae-db47-5075-0cf50aada335@web.de","threadId":"59090","inReplyTo":null,"subject":"[PATCH] ls-tree: fix expansion of repeated %(path)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-14T14:37:53Z","receivedAt":"2023-01-14T14:38:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"expand_show_tree() borrows the base strbuf given to us by read_tree() to\nbuild the full path of the current entry when handling %(path). Only\nits indirect caller, show_tree_fmt(), removes the added entry name.\nThat works fine as long as %(path) is only included once in the format\nstring, but accumulates duplicates if it's repeated:\n\n $ git ls-tree --format='%(path) %(path) %(path)' HEAD M*\n Makefile MakefileMakefile MakefileMakefileMakefile\n\nReset the length after each use to get the same expansion every time;\nhere's the behavior with this patch:\n\n $ ./git ls-tree --format='%(path) %(path) %(path)' HEAD M*\n Makefile Makefile Makefile\n\nSigned-off-by: René Scharfe \n---\n builtin/ls-tree.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex c3ea09281a..120fff9fa0 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -98,9 +98,11 @@ static size_t expand_show_tree(struct strbuf *sb, const char *start,\n \t\tconst char *prefix = chomp_prefix ? ls_tree_prefix : NULL;\n \t\tstruct strbuf quoted = STRBUF_INIT;\n \t\tstruct strbuf sbuf = STRBUF_INIT;\n+\t\tsize_t baselen = data->base->len;\n \t\tstrbuf_addstr(data->base, data->pathname);\n \t\tname = relative_path(data->base->buf, prefix, &sbuf);\n \t\tquote_c_style(name, "ed, NULL, 0);\n+\t\tstrbuf_setlen(data->base, baselen);\n \t\tstrbuf_addbuf(sb, "ed);\n \t\tstrbuf_release(&sbuf);\n \t\tstrbuf_release("ed);\n@@ -144,7 +146,6 @@ static int show_recursive(const char *base, size_t baselen, const char *pathname\n static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,\n \t\t\t const char *pathname, unsigned mode, void *context UNUSED)\n {\n-\tsize_t baselen;\n \tint recurse = 0;\n \tstruct strbuf sb = STRBUF_INIT;\n \tenum object_type type = object_type(mode);\n@@ -164,12 +165,10 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,\n \tif (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))\n \t\treturn 0;\n\n-\tbaselen = base->len;\n \tstrbuf_expand(&sb, format, expand_show_tree, &data);\n \tstrbuf_addch(&sb, line_termination);\n \tfwrite(sb.buf, sb.len, 1, stdout);\n \tstrbuf_release(&sb);\n-\tstrbuf_setlen(base, baselen);\n \treturn recurse;\n }\n\n--\n2.39.0\n"},{"id":"470345","messageId":"55ae7333-3a13-0575-93ed-f858a1c2877e@web.de","threadId":"59090","inReplyTo":"59f0a3f8-2dae-db47-5075-0cf50aada335@web.de","subject":"[BONUS][PATCH] ls-tree: remove dead store and strbuf for quote_c_style()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-14T15:03:16Z","receivedAt":"2023-01-14T15:03:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Stop initializing \"name\" because it is set again before use.\n\nLet quote_c_style() write directly to \"sb\" instead of taking a detour\nthrough \"quoted\". This avoids an allocation and a string copy. The\nresult is the same because the function only appends.\n\nSigned-off-by: René Scharfe \n---\n builtin/ls-tree.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex 120fff9fa0..9b804cd13b 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -94,18 +94,15 @@ static size_t expand_show_tree(struct strbuf *sb, const char *start,\n \t} else if (skip_prefix(start, \"(objectname)\", &p)) {\n \t\tstrbuf_add_unique_abbrev(sb, data->oid, abbrev);\n \t} else if (skip_prefix(start, \"(path)\", &p)) {\n-\t\tconst char *name = data->base->buf;\n+\t\tconst char *name;\n \t\tconst char *prefix = chomp_prefix ? ls_tree_prefix : NULL;\n-\t\tstruct strbuf quoted = STRBUF_INIT;\n \t\tstruct strbuf sbuf = STRBUF_INIT;\n \t\tsize_t baselen = data->base->len;\n \t\tstrbuf_addstr(data->base, data->pathname);\n \t\tname = relative_path(data->base->buf, prefix, &sbuf);\n-\t\tquote_c_style(name, "ed, NULL, 0);\n+\t\tquote_c_style(name, sb, NULL, 0);\n \t\tstrbuf_setlen(data->base, baselen);\n-\t\tstrbuf_addbuf(sb, "ed);\n \t\tstrbuf_release(&sbuf);\n-\t\tstrbuf_release("ed);\n \t} else {\n \t\terrlen = (unsigned long)len;\n \t\tdie(_(\"bad ls-tree format: %%%.*s\"), errlen, start);\n--\n2.39.0\n"},{"id":"470353","messageId":"xmqqmt6lkqrp.fsf@gitster.g","threadId":"59090","inReplyTo":"59f0a3f8-2dae-db47-5075-0cf50aada335@web.de","subject":"Re: [PATCH] ls-tree: fix expansion of repeated %(path)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-14T16:46:50Z","receivedAt":"2023-01-14T16:46:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> expand_show_tree() borrows the base strbuf given to us by read_tree() to\n> build the full path of the current entry when handling %(path). Only\n> its indirect caller, show_tree_fmt(), removes the added entry name.\n> That works fine as long as %(path) is only included once in the format\n> string, but accumulates duplicates if it's repeated:\n>\n> $ git ls-tree --format='%(path) %(path) %(path)' HEAD M*\n> Makefile MakefileMakefile MakefileMakefileMakefile\n>\n> Reset the length after each use to get the same expansion every time;\n> here's the behavior with this patch:\n>\n> $ ./git ls-tree --format='%(path) %(path) %(path)' HEAD M*\n> Makefile Makefile Makefile\n>\n> Signed-off-by: René Scharfe \n> ---\n> builtin/ls-tree.c | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n\nI wonder if this was broken from its introduction at 455923e0\n(ls-tree: introduce \"--format\" option, 2022-03-23)?\n\nIt seems to be the case. With the following applied on top of\n455923e0, the new test fails as expected, and your patch fixes\nthe breakage, so I am tempted to squash the tests in ;-)\n\nThanks.\n\n t/t3104-ls-tree-format.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git c/t/t3104-ls-tree-format.sh w/t/t3104-ls-tree-format.sh\nindex 7f1eb699d3..7e6c4dc5da 100755\n--- c/t/t3104-ls-tree-format.sh\n+++ w/t/t3104-ls-tree-format.sh\n@@ -37,6 +37,12 @@ test_ls_tree_format () {\n \t'\n }\n \n+test_expect_success \"ls-tree --format='%(path) %(path) %(path)' HEAD top-file\" '\n+\tgit ls-tree --format=\"%(path) %(path) %(path)\" HEAD top-file.t >actual &&\n+\techo top-file.t top-file.t top-file.t >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_ls_tree_format \\\n \t\"%(objectmode) %(objecttype) %(objectname)%x09%(path)\" \\\n \t\"\"\n"},{"id":"470360","messageId":"442edda0-6988-a326-a25f-a84969dbb2c7@web.de","threadId":"59090","inReplyTo":"xmqqmt6lkqrp.fsf@gitster.g","subject":"Re: [PATCH] ls-tree: fix expansion of repeated %(path)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-01-14T18:24:12Z","receivedAt":"2023-01-14T18:24:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.01.2023 um 17:46 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> expand_show_tree() borrows the base strbuf given to us by read_tree() to\n>> build the full path of the current entry when handling %(path). Only\n>> its indirect caller, show_tree_fmt(), removes the added entry name.\n>> That works fine as long as %(path) is only included once in the format\n>> string, but accumulates duplicates if it's repeated:\n>>\n>> $ git ls-tree --format='%(path) %(path) %(path)' HEAD M*\n>> Makefile MakefileMakefile MakefileMakefileMakefile\n>>\n>> Reset the length after each use to get the same expansion every time;\n>> here's the behavior with this patch:\n>>\n>> $ ./git ls-tree --format='%(path) %(path) %(path)' HEAD M*\n>> Makefile Makefile Makefile\n>>\n>> Signed-off-by: René Scharfe \n>> ---\n>> builtin/ls-tree.c | 5 ++---\n>> 1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> I wonder if this was broken from its introduction at 455923e0\n> (ls-tree: introduce \"--format\" option, 2022-03-23)?\n\nYes.\n\n> It seems to be the case. With the following applied on top of\n> 455923e0, the new test fails as expected, and your patch fixes\n> the breakage, so I am tempted to squash the tests in ;-)\n\nI didn't include a test because it's an odd bug which I didn't expect to\never return. But its current existence proves that it can happen. So I\ndon't mind this addition.\n\n>\n> Thanks.\n>\n> t/t3104-ls-tree-format.sh | 6 ++++++\n> 1 file changed, 6 insertions(+)\n>\n> diff --git c/t/t3104-ls-tree-format.sh w/t/t3104-ls-tree-format.sh\n> index 7f1eb699d3..7e6c4dc5da 100755\n> --- c/t/t3104-ls-tree-format.sh\n> +++ w/t/t3104-ls-tree-format.sh\n> @@ -37,6 +37,12 @@ test_ls_tree_format () {\n> \t'\n> }\n>\n> +test_expect_success \"ls-tree --format='%(path) %(path) %(path)' HEAD top-file\" '\n> +\tgit ls-tree --format=\"%(path) %(path) %(path)\" HEAD top-file.t >actual &&\n> +\techo top-file.t top-file.t top-file.t >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_ls_tree_format \\\n> \t\"%(objectmode) %(objecttype) %(objectname)%x09%(path)\" \\\n> \t\"\"\n"}]}