* [PATCH] ls-tree: fix expansion of repeated %(path)
@ 2023-01-14 14:37 René Scharfe
2023-01-14 15:03 ` [BONUS][PATCH] ls-tree: remove dead store and strbuf for quote_c_style() René Scharfe
2023-01-14 16:46 ` [PATCH] ls-tree: fix expansion of repeated %(path) Junio C Hamano
0 siblings, 2 replies; 4+ messages in thread
From: René Scharfe @ 2023-01-14 14:37 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason
expand_show_tree() borrows the base strbuf given to us by read_tree() to
build the full path of the current entry when handling %(path). Only
its indirect caller, show_tree_fmt(), removes the added entry name.
That works fine as long as %(path) is only included once in the format
string, but accumulates duplicates if it's repeated:
$ git ls-tree --format='%(path) %(path) %(path)' HEAD M*
Makefile MakefileMakefile MakefileMakefileMakefile
Reset the length after each use to get the same expansion every time;
here's the behavior with this patch:
$ ./git ls-tree --format='%(path) %(path) %(path)' HEAD M*
Makefile Makefile Makefile
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/ls-tree.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index c3ea09281a..120fff9fa0 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -98,9 +98,11 @@ static size_t expand_show_tree(struct strbuf *sb, const char *start,
const char *prefix = chomp_prefix ? ls_tree_prefix : NULL;
struct strbuf quoted = STRBUF_INIT;
struct strbuf sbuf = STRBUF_INIT;
+ size_t baselen = data->base->len;
strbuf_addstr(data->base, data->pathname);
name = relative_path(data->base->buf, prefix, &sbuf);
quote_c_style(name, "ed, NULL, 0);
+ strbuf_setlen(data->base, baselen);
strbuf_addbuf(sb, "ed);
strbuf_release(&sbuf);
strbuf_release("ed);
@@ -144,7 +146,6 @@ static int show_recursive(const char *base, size_t baselen, const char *pathname
static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
const char *pathname, unsigned mode, void *context UNUSED)
{
- size_t baselen;
int recurse = 0;
struct strbuf sb = STRBUF_INIT;
enum object_type type = object_type(mode);
@@ -164,12 +165,10 @@ static int show_tree_fmt(const struct object_id *oid, struct strbuf *base,
if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
return 0;
- baselen = base->len;
strbuf_expand(&sb, format, expand_show_tree, &data);
strbuf_addch(&sb, line_termination);
fwrite(sb.buf, sb.len, 1, stdout);
strbuf_release(&sb);
- strbuf_setlen(base, baselen);
return recurse;
}
--
2.39.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [BONUS][PATCH] ls-tree: remove dead store and strbuf for quote_c_style()
2023-01-14 14:37 [PATCH] ls-tree: fix expansion of repeated %(path) René Scharfe
@ 2023-01-14 15:03 ` René Scharfe
2023-01-14 16:46 ` [PATCH] ls-tree: fix expansion of repeated %(path) Junio C Hamano
1 sibling, 0 replies; 4+ messages in thread
From: René Scharfe @ 2023-01-14 15:03 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason
Stop initializing "name" because it is set again before use.
Let quote_c_style() write directly to "sb" instead of taking a detour
through "quoted". This avoids an allocation and a string copy. The
result is the same because the function only appends.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/ls-tree.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 120fff9fa0..9b804cd13b 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -94,18 +94,15 @@ static size_t expand_show_tree(struct strbuf *sb, const char *start,
} else if (skip_prefix(start, "(objectname)", &p)) {
strbuf_add_unique_abbrev(sb, data->oid, abbrev);
} else if (skip_prefix(start, "(path)", &p)) {
- const char *name = data->base->buf;
+ const char *name;
const char *prefix = chomp_prefix ? ls_tree_prefix : NULL;
- struct strbuf quoted = STRBUF_INIT;
struct strbuf sbuf = STRBUF_INIT;
size_t baselen = data->base->len;
strbuf_addstr(data->base, data->pathname);
name = relative_path(data->base->buf, prefix, &sbuf);
- quote_c_style(name, "ed, NULL, 0);
+ quote_c_style(name, sb, NULL, 0);
strbuf_setlen(data->base, baselen);
- strbuf_addbuf(sb, "ed);
strbuf_release(&sbuf);
- strbuf_release("ed);
} else {
errlen = (unsigned long)len;
die(_("bad ls-tree format: %%%.*s"), errlen, start);
--
2.39.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ls-tree: fix expansion of repeated %(path)
2023-01-14 14:37 [PATCH] ls-tree: fix expansion of repeated %(path) René Scharfe
2023-01-14 15:03 ` [BONUS][PATCH] ls-tree: remove dead store and strbuf for quote_c_style() René Scharfe
@ 2023-01-14 16:46 ` Junio C Hamano
2023-01-14 18:24 ` René Scharfe
1 sibling, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2023-01-14 16:46 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Ævar Arnfjörð Bjarmason
René Scharfe <l.s.r@web.de> writes:
> expand_show_tree() borrows the base strbuf given to us by read_tree() to
> build the full path of the current entry when handling %(path). Only
> its indirect caller, show_tree_fmt(), removes the added entry name.
> That works fine as long as %(path) is only included once in the format
> string, but accumulates duplicates if it's repeated:
>
> $ git ls-tree --format='%(path) %(path) %(path)' HEAD M*
> Makefile MakefileMakefile MakefileMakefileMakefile
>
> Reset the length after each use to get the same expansion every time;
> here's the behavior with this patch:
>
> $ ./git ls-tree --format='%(path) %(path) %(path)' HEAD M*
> Makefile Makefile Makefile
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> builtin/ls-tree.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
I wonder if this was broken from its introduction at 455923e0
(ls-tree: introduce "--format" option, 2022-03-23)?
It seems to be the case. With the following applied on top of
455923e0, the new test fails as expected, and your patch fixes
the breakage, so I am tempted to squash the tests in ;-)
Thanks.
t/t3104-ls-tree-format.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git c/t/t3104-ls-tree-format.sh w/t/t3104-ls-tree-format.sh
index 7f1eb699d3..7e6c4dc5da 100755
--- c/t/t3104-ls-tree-format.sh
+++ w/t/t3104-ls-tree-format.sh
@@ -37,6 +37,12 @@ test_ls_tree_format () {
'
}
+test_expect_success "ls-tree --format='%(path) %(path) %(path)' HEAD top-file" '
+ git ls-tree --format="%(path) %(path) %(path)" HEAD top-file.t >actual &&
+ echo top-file.t top-file.t top-file.t >expect &&
+ test_cmp expect actual
+'
+
test_ls_tree_format \
"%(objectmode) %(objecttype) %(objectname)%x09%(path)" \
""
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ls-tree: fix expansion of repeated %(path)
2023-01-14 16:46 ` [PATCH] ls-tree: fix expansion of repeated %(path) Junio C Hamano
@ 2023-01-14 18:24 ` René Scharfe
0 siblings, 0 replies; 4+ messages in thread
From: René Scharfe @ 2023-01-14 18:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Ævar Arnfjörð Bjarmason
Am 14.01.2023 um 17:46 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> expand_show_tree() borrows the base strbuf given to us by read_tree() to
>> build the full path of the current entry when handling %(path). Only
>> its indirect caller, show_tree_fmt(), removes the added entry name.
>> That works fine as long as %(path) is only included once in the format
>> string, but accumulates duplicates if it's repeated:
>>
>> $ git ls-tree --format='%(path) %(path) %(path)' HEAD M*
>> Makefile MakefileMakefile MakefileMakefileMakefile
>>
>> Reset the length after each use to get the same expansion every time;
>> here's the behavior with this patch:
>>
>> $ ./git ls-tree --format='%(path) %(path) %(path)' HEAD M*
>> Makefile Makefile Makefile
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> builtin/ls-tree.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> I wonder if this was broken from its introduction at 455923e0
> (ls-tree: introduce "--format" option, 2022-03-23)?
Yes.
> It seems to be the case. With the following applied on top of
> 455923e0, the new test fails as expected, and your patch fixes
> the breakage, so I am tempted to squash the tests in ;-)
I didn't include a test because it's an odd bug which I didn't expect to
ever return. But its current existence proves that it can happen. So I
don't mind this addition.
>
> Thanks.
>
> t/t3104-ls-tree-format.sh | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git c/t/t3104-ls-tree-format.sh w/t/t3104-ls-tree-format.sh
> index 7f1eb699d3..7e6c4dc5da 100755
> --- c/t/t3104-ls-tree-format.sh
> +++ w/t/t3104-ls-tree-format.sh
> @@ -37,6 +37,12 @@ test_ls_tree_format () {
> '
> }
>
> +test_expect_success "ls-tree --format='%(path) %(path) %(path)' HEAD top-file" '
> + git ls-tree --format="%(path) %(path) %(path)" HEAD top-file.t >actual &&
> + echo top-file.t top-file.t top-file.t >expect &&
> + test_cmp expect actual
> +'
> +
> test_ls_tree_format \
> "%(objectmode) %(objecttype) %(objectname)%x09%(path)" \
> ""
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-14 18:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-14 14:37 [PATCH] ls-tree: fix expansion of repeated %(path) René Scharfe
2023-01-14 15:03 ` [BONUS][PATCH] ls-tree: remove dead store and strbuf for quote_c_style() René Scharfe
2023-01-14 16:46 ` [PATCH] ls-tree: fix expansion of repeated %(path) Junio C Hamano
2023-01-14 18:24 ` René Scharfe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).