| Message ID | 20210104162128.95281-6-rafaeloliveira.cs@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | teach `worktree list` verbose mode and prunable annotations | expand |
Hi Rafael Thanks for working on this On 04/01/2021 16:21, Rafael Silva wrote: > "git worktree list" porcelain format shows one attribute per line, each > attribute is listed with a label and value separated by a single space. > If any worktree is locked, a "locked" attribute is listed followed by the > reason, if available, otherwise only "locked" is shown. > > In case the lock reason contains newline characters (i.e: LF or CRLF) > this will cause the format to break as each line should correspond to > one attribute. This leads to unwanted behavior, specially as the > porcelain is intended to be machine-readable. To address this shortcoming > let's teach "git worktree list" to escape any newline character before > outputting the locked reason for porcelain format. As the porcelain output format cannot handle worktree paths that contain newlines either I think it would be better to add a `-z` flag which uses NUL termination as we have for many other commands (diff, ls-files etc). This would fix the problem forever without having to worry about quoting each time a new field is added. If you really need to quote the lock reason then please use one of the path quoting routines (probably quote_c_style() or write_name_quoted()) in quote.c rather than rolling your own - the code below gives the same output for a string that contains the two characters `\` followed by `n` as it does for the single character `\n`. People are (hopefully) used to dequoting our path quoting and there are routines out there to do it (we have one git Git.pm) using a function in quote.c will allow them to use those routines here. Best Wishes Phillip > Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> > --- > builtin/worktree.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/builtin/worktree.c b/builtin/worktree.c > index dedd4089e5..9156ccd67e 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -567,6 +567,24 @@ static int add(int ac, const char **av, const char *prefix) > return add_worktree(path, branch, &opts); > } > > +static char *worktree_escape_reason(char *reason) > +{ > + struct strbuf escaped = STRBUF_INIT; > + char *r; > + > + for (r = reason; *r; r++) { > + if (*r == '\r' && *(r + 1) && *(r + 1) == '\n') { > + strbuf_addstr(&escaped, "\\r\\n"); > + r++; > + } else if (*r == '\n') > + strbuf_addstr(&escaped, "\\n"); > + else > + strbuf_addch(&escaped, *r); > + } > + > + return strbuf_detach(&escaped, NULL); > +} > + > static void show_worktree_porcelain(struct worktree *wt) > { > printf("worktree %s\n", wt->path); > @@ -580,9 +598,11 @@ static void show_worktree_porcelain(struct worktree *wt) > printf("branch %s\n", wt->head_ref); > > if (worktree_lock_reason(wt)) { > - if (*wt->lock_reason) > - printf("locked %s\n", wt->lock_reason); > - else > + if (*wt->lock_reason) { > + char *reason = worktree_escape_reason(wt->lock_reason); > + printf("locked %s\n", reason); > + free(reason); > + } else > printf("locked\n"); > } > >
On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > "git worktree list" porcelain format shows one attribute per line, each > attribute is listed with a label and value separated by a single space. > If any worktree is locked, a "locked" attribute is listed followed by the > reason, if available, otherwise only "locked" is shown. > > In case the lock reason contains newline characters (i.e: LF or CRLF) > this will cause the format to break as each line should correspond to > one attribute. This leads to unwanted behavior, specially as the > porcelain is intended to be machine-readable. To address this shortcoming > let's teach "git worktree list" to escape any newline character before > outputting the locked reason for porcelain format. s/specially/especially/ > Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> > --- > diff --git a/builtin/worktree.c b/builtin/worktree.c > @@ -567,6 +567,24 @@ static int add(int ac, const char **av, const char *prefix) > +static char *worktree_escape_reason(char *reason) > +{ > + struct strbuf escaped = STRBUF_INIT; > + char *r; > + > + for (r = reason; *r; r++) { > + if (*r == '\r' && *(r + 1) && *(r + 1) == '\n') { The `*(r +1)` in the middle of the condition is redundant. The same case is already handled by the `*(r + 1) == '\n'` which follows it since EOL ('\0') won't match '\n'. > + strbuf_addstr(&escaped, "\\r\\n"); > + r++; > + } else if (*r == '\n') > + strbuf_addstr(&escaped, "\\n"); > + else > + strbuf_addch(&escaped, *r); > + } > + > + return strbuf_detach(&escaped, NULL); > +} As Phillip already mentioned upstream, we can use one of the functions from quote.c rather than rolling out own here. quote_c_style(), as Phillip suggested, is almost certainly the best choice for a couple reasons. First, when called with CQUOTE_NODQ, it adds quotes around the string if there are any characters which need to be escaped, but doesn't quote the string if it contains no special characters. This makes the output more nicely readable in the typical case when there are no special characters, and makes it possible to distinguish the case Phillip pointed out between literal two characters "\n" in a string and '\n' representing a newline. Second, consumers of our porcelain are already used to consuming strings produced by quote_c_style(). > @@ -580,9 +598,11 @@ static void show_worktree_porcelain(struct worktree *wt) > if (worktree_lock_reason(wt)) { > - if (*wt->lock_reason) > - printf("locked %s\n", wt->lock_reason); > - else > + if (*wt->lock_reason) { > + char *reason = worktree_escape_reason(wt->lock_reason); > + printf("locked %s\n", reason); > + free(reason); > + } else It would be preferable to fold this change directly into the preceding patch, thus eliminating this patch altogether. That way this patch series doesn't introduce a state in which the behavior is knowingly "broken", but instead is correct from the start. Also, since you can use the existing quote_c_style(), the change made by this patch becomes tiny, thus is easily folded into the earlier patch.
On Tue, Jan 5, 2021 at 5:29 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 04/01/2021 16:21, Rafael Silva wrote: > > In case the lock reason contains newline characters (i.e: LF or CRLF) > > this will cause the format to break as each line should correspond to > > one attribute. This leads to unwanted behavior, specially as the > > porcelain is intended to be machine-readable. To address this shortcoming > > let's teach "git worktree list" to escape any newline character before > > outputting the locked reason for porcelain format. > > As the porcelain output format cannot handle worktree paths that contain > newlines either I think it would be better to add a `-z` flag which uses > NUL termination as we have for many other commands (diff, ls-files etc). > This would fix the problem forever without having to worry about quoting > each time a new field is added. Adding a `-z` flag makes sense, but that doesn't mean the existing `--porcelain` is meaningless without it. I see `-z` as a good complement to `--porcelain`. Yes, we should fix the problem that the existing porcelain output doesn't escape the path, but that can be done as a separate bug fix; it doesn't need to be rolled into this series. > If you really need to quote the lock reason then please use one of the > path quoting routines (probably quote_c_style() or write_name_quoted()) > in quote.c rather than rolling your own - the code below gives the same > output for a string that contains the two characters `\` followed by `n` > as it does for the single character `\n`. People are (hopefully) used to > dequoting our path quoting and there are routines out there to do it (we > have one git Git.pm) using a function in quote.c will allow them to use > those routines here. I was going to suggest the same. Thanks.
Hi Phillip, Thank you for reviewing the patch and the suggestions. Phillip Wood writes: > On 04/01/2021 16:21, Rafael Silva wrote: >> "git worktree list" porcelain format shows one attribute per line, each >> attribute is listed with a label and value separated by a single space. >> If any worktree is locked, a "locked" attribute is listed followed by the >> reason, if available, otherwise only "locked" is shown. >> In case the lock reason contains newline characters (i.e: LF or >> CRLF) >> this will cause the format to break as each line should correspond to >> one attribute. This leads to unwanted behavior, specially as the >> porcelain is intended to be machine-readable. To address this shortcoming >> let's teach "git worktree list" to escape any newline character before >> outputting the locked reason for porcelain format. > > As the porcelain output format cannot handle worktree paths that > contain newlines either I think it would be better to add a `-z` flag > which uses NUL termination as we have for many other commands (diff, > ls-files etc). This would fix the problem forever without having to > worry about quoting each time a new field is added. > This is a good point and it seems to be good addition for the porcelain format. Perhaps it make more sense to apply as separate patch though, something that Eric Sunshine also mentioned on this message reply. > If you really need to quote the lock reason then please use one of the > path quoting routines (probably quote_c_style() or > write_name_quoted()) in quote.c rather than rolling your own - the > code below gives the same output for a string that contains the two > characters `\` followed by `n` as it does for the single character > `\n`. People are (hopefully) used to dequoting our path quoting and > there are routines out there to do it (we have one git Git.pm) using a > function in quote.c will allow them to use those routines here. > > Best Wishes > > Phillip > That's great, thank you for pointing this out to me. I will definitely change this into the next revision by dropping the function written from scratch on this patch and reuse one of the existing functions. For what is worth, I just added the function because I didn't know about the existing ones. So, there is no reason to add the new function.
diff --git a/builtin/worktree.c b/builtin/worktree.c index dedd4089e5..9156ccd67e 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -567,6 +567,24 @@ static int add(int ac, const char **av, const char *prefix) return add_worktree(path, branch, &opts); } +static char *worktree_escape_reason(char *reason) +{ + struct strbuf escaped = STRBUF_INIT; + char *r; + + for (r = reason; *r; r++) { + if (*r == '\r' && *(r + 1) && *(r + 1) == '\n') { + strbuf_addstr(&escaped, "\\r\\n"); + r++; + } else if (*r == '\n') + strbuf_addstr(&escaped, "\\n"); + else + strbuf_addch(&escaped, *r); + } + + return strbuf_detach(&escaped, NULL); +} + static void show_worktree_porcelain(struct worktree *wt) { printf("worktree %s\n", wt->path); @@ -580,9 +598,11 @@ static void show_worktree_porcelain(struct worktree *wt) printf("branch %s\n", wt->head_ref); if (worktree_lock_reason(wt)) { - if (*wt->lock_reason) - printf("locked %s\n", wt->lock_reason); - else + if (*wt->lock_reason) { + char *reason = worktree_escape_reason(wt->lock_reason); + printf("locked %s\n", reason); + free(reason); + } else printf("locked\n"); }
"git worktree list" porcelain format shows one attribute per line, each attribute is listed with a label and value separated by a single space. If any worktree is locked, a "locked" attribute is listed followed by the reason, if available, otherwise only "locked" is shown. In case the lock reason contains newline characters (i.e: LF or CRLF) this will cause the format to break as each line should correspond to one attribute. This leads to unwanted behavior, specially as the porcelain is intended to be machine-readable. To address this shortcoming let's teach "git worktree list" to escape any newline character before outputting the locked reason for porcelain format. Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> --- builtin/worktree.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)