| Message ID | 20210105110219.99610-1-phillip.wood123@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | worktree: add -z option for list subcommand | expand |
On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > Add a -z option to be used in conjunction with --porcelain that gives > NUL-terminated output. This enables 'worktree list --porcelain' to > handle worktree paths that contain newlines. Adding a -z mode makes a lot of sense. This, along with a fix to call quote_c_style() on paths in normal (not `-z`) porcelain mode, can easily be done after Rafael's series lands. Or they could be done before his series, though that might make a lot of extra work for him. > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > I found an old patch that added NUL termination. I've rebased it > but the new test fails as there seems to be another worktree thats > been added since I wrote this, anyway I thought it might be a useful > start for adding a `-z` option. The test failure is fallout from a "bug" in a test added by Rafael's earlier series[1] which was not caught by the reviewer[2]. In fact, his current series has a drive-by fix[3] for this bug in patch [6/7]. Applying that fix (or the refinement[4] I suggested in my review) makes your test work. [1]: https://lore.kernel.org/git/20201011101152.5291-2-rafaeloliveira.cs@gmail.com/ [2]: https://lore.kernel.org/git/CAPig+cS8hvX4GCsnfLBnQ4Q_AkUad=bw7rjVcaOqSEqcLZvx8w@mail.gmail.com/ [3]: https://lore.kernel.org/git/20210104162128.95281-7-rafaeloliveira.cs@gmail.com/ [4]: https://lore.kernel.org/git/CAPig+cRysXpK0e1xXOuVd+EtkeyTk8FR6MUrL=Bg3W4ve8jdNA@mail.gmail.com/ > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > @@ -217,7 +217,13 @@ This can also be set up as the default behaviour by using the > +-z:: > + When `--porcelain` is specified with `list` terminate each line with a > + NUL rather than a newline. This makes it possible to parse the output > + when a worktree path contains a newline character. If we fix normal-mode porcelain to call quote_c_style(), then we can drop the last sentence or refine it to say something along the lines of it being easier to deal with paths with embedded newlines than in normal porcelain mode. > diff --git a/builtin/worktree.c b/builtin/worktree.c > @@ -640,19 +640,25 @@ static int add(int ac, const char **av, const char *prefix) > +static void show_worktree_porcelain(struct worktree *wt, int line_terminator) > { > + printf("worktree %s", wt->path); > + fputc(line_terminator, stdout); > + if (wt->is_bare) { > + printf("bare"); > + fputc(line_terminator, stdout); > + } else { > + printf("HEAD %s", oid_to_hex(&wt->head_oid)); > + fputc(line_terminator, stdout); > + if (wt->is_detached) { > + printf("detached"); > + fputc(line_terminator, stdout); > + } else if (wt->head_ref) { > + printf("branch %s", wt->head_ref); > + fputc(line_terminator, stdout); > + } Perhaps this could all be done a bit more concisely with printf() alone rather than combining it with fputc(). For instance: printf("worktree %s%c", wt->path, line_terminator); and so on. > - printf("\n"); > + fputc(line_terminator, stdout); This use of fputc() makes sense. > @@ -720,15 +726,20 @@ static void pathsort(struct worktree **wt) > static int list(int ac, const char **av, const char *prefix) > { > + OPT_SET_INT('z', NULL, &line_terminator, > + N_("fields are separated with NUL character"), '\0'), "fields" sounds a little odd. "lines" might make more sense. "records" might be even better and matches the wording of some other Git commands accepting `-z`. > + else if (!line_terminator && !porcelain) > + die(_("'-z' requires '--porcelain'")); Other error messages in this file don't quote command-line options, so: die(_("-z requires --porcelain")); would be more consistent. > diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh > @@ -71,6 +71,28 @@ test_expect_success '"list" all worktrees with locked annotation' ' > +test_expect_success '"list" all worktrees --porcelain -z' ' > + test_when_finished "rm -rf here _actual actual expect && > + git worktree prune" && > + printf "worktree %sQHEAD %sQbranch %sQQ" \ > + "$(git rev-parse --show-toplevel)" \ > + "$(git rev-parse HEAD)" \ > + "$(git symbolic-ref HEAD)" >expect && > + git worktree add --detach here master && > + printf "worktree %sQHEAD %sQdetachedQQ" \ > + "$(git -C here rev-parse --show-toplevel)" \ > + "$(git rev-parse HEAD)" >>expect && > + git worktree list --porcelain -z >_actual && > + cat _actual | tr "\0" Q >actual && Or just use nul_to_q(): nul_to_q <_actual >actual && (And there's no need to `cat` the file.) > + test_cmp expect actual > +' > + > +test_expect_success '"list" -z fails without --porcelain' ' > + test_when_finished "rm -rf here && git worktree prune" && > + git worktree add --detach here master && > + test_must_fail git worktree list -z > +' I don't think there's any need for this test to create (and cleanup) a worktree. So, the entire test could collapse to: test_expect_success '"list" -z fails without --porcelain' ' test_must_fail git worktree list -z '
Hi Eric & Rafael On 07/01/2021 03:34, Eric Sunshine wrote: > On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> Add a -z option to be used in conjunction with --porcelain that gives >> NUL-terminated output. This enables 'worktree list --porcelain' to >> handle worktree paths that contain newlines. > > Adding a -z mode makes a lot of sense. This, along with a fix to call > quote_c_style() on paths in normal (not `-z`) porcelain mode, I'm concerned that starting to quote paths will break backwards compatibility. Even if we restricted the quoting to just those paths that contain '\n' there is no way to distinguish between a quoted path and one that begins and ends with ". This is the reason I prefer to add `-z` instead of taking Rafael's patch to quote the lock reason as that patch still leaves the output of `git worktree list --porcelain` ambiguous and it cannot be fixed without breaking existing users. A counter argument to all this is that there are thousands of users on file systems that cannot have newlines in paths and Rafael's patch is definitely a net win for them. > can > easily be done after Rafael's series lands. Or they could be done > before his series, though that might make a lot of extra work for him. I would definitely be easiest to wait for Rafael's series to land if we want to add `-z` separately >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> I found an old patch that added NUL termination. I've rebased it >> but the new test fails as there seems to be another worktree thats >> been added since I wrote this, anyway I thought it might be a useful >> start for adding a `-z` option. > > The test failure is fallout from a "bug" in a test added by Rafael's > earlier series[1] which was not caught by the reviewer[2]. In fact, > his current series has a drive-by fix[3] for this bug in patch [6/7]. > Applying that fix (or the refinement[4] I suggested in my review) > makes your test work. Thanks ever so much for taking the time to track down the cause of the test failure. Best Wishes Phillip > [1]: https://lore.kernel.org/git/20201011101152.5291-2-rafaeloliveira.cs@gmail.com/ > [2]: https://lore.kernel.org/git/CAPig+cS8hvX4GCsnfLBnQ4Q_AkUad=bw7rjVcaOqSEqcLZvx8w@mail.gmail.com/ > [3]: https://lore.kernel.org/git/20210104162128.95281-7-rafaeloliveira.cs@gmail.com/ > [4]: https://lore.kernel.org/git/CAPig+cRysXpK0e1xXOuVd+EtkeyTk8FR6MUrL=Bg3W4ve8jdNA@mail.gmail.com/ > >> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt >> @@ -217,7 +217,13 @@ This can also be set up as the default behaviour by using the >> +-z:: >> + When `--porcelain` is specified with `list` terminate each line with a >> + NUL rather than a newline. This makes it possible to parse the output >> + when a worktree path contains a newline character. > > If we fix normal-mode porcelain to call quote_c_style(), then we can > drop the last sentence or refine it to say something along the lines > of it being easier to deal with paths with embedded newlines than in > normal porcelain mode. > >> diff --git a/builtin/worktree.c b/builtin/worktree.c >> @@ -640,19 +640,25 @@ static int add(int ac, const char **av, const char *prefix) >> +static void show_worktree_porcelain(struct worktree *wt, int line_terminator) >> { >> + printf("worktree %s", wt->path); >> + fputc(line_terminator, stdout); >> + if (wt->is_bare) { >> + printf("bare"); >> + fputc(line_terminator, stdout); >> + } else { >> + printf("HEAD %s", oid_to_hex(&wt->head_oid)); >> + fputc(line_terminator, stdout); >> + if (wt->is_detached) { >> + printf("detached"); >> + fputc(line_terminator, stdout); >> + } else if (wt->head_ref) { >> + printf("branch %s", wt->head_ref); >> + fputc(line_terminator, stdout); >> + } > > Perhaps this could all be done a bit more concisely with printf() > alone rather than combining it with fputc(). For instance: > > printf("worktree %s%c", wt->path, line_terminator); > > and so on. > >> - printf("\n"); >> + fputc(line_terminator, stdout); > > This use of fputc() makes sense. > >> @@ -720,15 +726,20 @@ static void pathsort(struct worktree **wt) >> static int list(int ac, const char **av, const char *prefix) >> { >> + OPT_SET_INT('z', NULL, &line_terminator, >> + N_("fields are separated with NUL character"), '\0'), > > "fields" sounds a little odd. "lines" might make more sense. "records" > might be even better and matches the wording of some other Git > commands accepting `-z`. > >> + else if (!line_terminator && !porcelain) >> + die(_("'-z' requires '--porcelain'")); > > Other error messages in this file don't quote command-line options, so: > > die(_("-z requires --porcelain")); > > would be more consistent. > >> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh >> @@ -71,6 +71,28 @@ test_expect_success '"list" all worktrees with locked annotation' ' >> +test_expect_success '"list" all worktrees --porcelain -z' ' >> + test_when_finished "rm -rf here _actual actual expect && >> + git worktree prune" && >> + printf "worktree %sQHEAD %sQbranch %sQQ" \ >> + "$(git rev-parse --show-toplevel)" \ >> + "$(git rev-parse HEAD)" \ >> + "$(git symbolic-ref HEAD)" >expect && >> + git worktree add --detach here master && >> + printf "worktree %sQHEAD %sQdetachedQQ" \ >> + "$(git -C here rev-parse --show-toplevel)" \ >> + "$(git rev-parse HEAD)" >>expect && >> + git worktree list --porcelain -z >_actual && >> + cat _actual | tr "\0" Q >actual && > > Or just use nul_to_q(): > > nul_to_q <_actual >actual && > > (And there's no need to `cat` the file.) > >> + test_cmp expect actual >> +' >> + >> +test_expect_success '"list" -z fails without --porcelain' ' >> + test_when_finished "rm -rf here && git worktree prune" && >> + git worktree add --detach here master && >> + test_must_fail git worktree list -z >> +' > > I don't think there's any need for this test to create (and cleanup) a > worktree. So, the entire test could collapse to: > > test_expect_success '"list" -z fails without --porcelain' ' > test_must_fail git worktree list -z > ' >
On Fri, Jan 8, 2021 at 5:33 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 07/01/2021 03:34, Eric Sunshine wrote: > > On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > >> Add a -z option to be used in conjunction with --porcelain that gives > >> NUL-terminated output. This enables 'worktree list --porcelain' to > >> handle worktree paths that contain newlines. > > > > Adding a -z mode makes a lot of sense. This, along with a fix to call > > quote_c_style() on paths in normal (not `-z`) porcelain mode, > > I'm concerned that starting to quote paths will break backwards > compatibility. Even if we restricted the quoting to just those paths > that contain '\n' there is no way to distinguish between a quoted path > and one that begins and ends with ". Backward compatibility is a valid concern, though I haven't managed to convince myself that it would matter much in this case. In one sense, the failure of the porcelain format to properly quote/escape paths when needed can be viewed as an outright bug and, although we value backward compatibility, we also value correctness, and such bug fixes have been accepted in the past. Especially in a case such as this, it seems exceedingly unlikely that fixing the bug would be harmful or break existing tooling (though, of course that possibility always exists, even if remotely so). I can imagine ways in which tooling might be engineered to work around the shortcoming that `git worktree list --porcelain` doesn't properly handle newlines embedded in paths, but such tooling would almost certainly be so fragile anyhow that it would break as we add more keys to the extensible porcelain format. Coupled with the fact that newlines embedded in paths are so exceedingly unlikely, it's hard to imagine that fixing this bug would have a big impact on existing tooling. The case you mention about a path which happens to have a double-quote as its first character does concern me a bit more since existing tooling wouldn't have had to jump through hoops, or indeed do anything special, with such paths, unlike the embedded newline case. But then, too, it's pretty hard to imagine this coming up much, if at all, in practice. That's not to say that I can't imagine a path, in general, beginning with a quote, but keeping in mind that we're talking only about worktree paths, it seems exceedingly unlikely. My gut feeling (for what it's worth) is that worktree paths containing embedded newlines (or other special characters) or beginning with a double-quote is so unlikely to come in in actual practice that viewing this as a bug fix is probably a reasonable approach, whereas some other approach -- such as introducing porcelain-v2 or creating a new porcelain key, say "worktreepath" which supersedes "worktree" (without retiring "worktree") -- may be overkill. None of the above is an argument against a complementary `-z` mode, which I think is a very good idea. > This is the reason I prefer to add > `-z` instead of taking Rafael's patch to quote the lock reason as that > patch still leaves the output of `git worktree list --porcelain` > ambiguous and it cannot be fixed without breaking existing users. A > counter argument to all this is that there are thousands of users on > file systems that cannot have newlines in paths and Rafael's patch is > definitely a net win for them. Rafael's patch is quoting only the lock-reason, not the worktree path, so I think it's orthogonal to this discussion. Also, his patch is introducing `lock` as a new attribute in porcelain output, not modifying behavior of an existing `lock` attribute.
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index af06128cc9..a07b593cc3 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -10,7 +10,7 @@ SYNOPSIS -------- [verse] 'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<commit-ish>] -'git worktree list' [--porcelain] +'git worktree list' [--porcelain [-z]] 'git worktree lock' [--reason <string>] <worktree> 'git worktree move' <worktree> <new-path> 'git worktree prune' [-n] [-v] [--expire <expire>] @@ -217,7 +217,13 @@ This can also be set up as the default behaviour by using the --porcelain:: With `list`, output in an easy-to-parse format for scripts. This format will remain stable across Git versions and regardless of user - configuration. See below for details. + configuration. It is recommended to combine this with `-z`. + See below for details. + +-z:: + When `--porcelain` is specified with `list` terminate each line with a + NUL rather than a newline. This makes it possible to parse the output + when a worktree path contains a newline character. -q:: --quiet:: @@ -369,7 +375,8 @@ $ git worktree list Porcelain Format ~~~~~~~~~~~~~~~~ -The porcelain format has a line per attribute. Attributes are listed with a +The porcelain format has a line per attribute. If `-z` is given then the lines +are terminated with NUL rather than a newline. Attributes are listed with a label and value separated by a single space. Boolean attributes (like `bare` and `detached`) are listed as a label only, and are present only if the value is true. The first attribute of a working tree is always diff --git a/builtin/worktree.c b/builtin/worktree.c index 197fd24a55..0cd331873c 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -640,19 +640,25 @@ static int add(int ac, const char **av, const char *prefix) return add_worktree(path, branch, &opts); } -static void show_worktree_porcelain(struct worktree *wt) +static void show_worktree_porcelain(struct worktree *wt, int line_terminator) { - printf("worktree %s\n", wt->path); - if (wt->is_bare) - printf("bare\n"); - else { - printf("HEAD %s\n", oid_to_hex(&wt->head_oid)); - if (wt->is_detached) - printf("detached\n"); - else if (wt->head_ref) - printf("branch %s\n", wt->head_ref); + printf("worktree %s", wt->path); + fputc(line_terminator, stdout); + if (wt->is_bare) { + printf("bare"); + fputc(line_terminator, stdout); + } else { + printf("HEAD %s", oid_to_hex(&wt->head_oid)); + fputc(line_terminator, stdout); + if (wt->is_detached) { + printf("detached"); + fputc(line_terminator, stdout); + } else if (wt->head_ref) { + printf("branch %s", wt->head_ref); + fputc(line_terminator, stdout); + } } - printf("\n"); + fputc(line_terminator, stdout); } static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len) @@ -720,15 +726,20 @@ static void pathsort(struct worktree **wt) static int list(int ac, const char **av, const char *prefix) { int porcelain = 0; + int line_terminator = '\n'; struct option options[] = { OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")), + OPT_SET_INT('z', NULL, &line_terminator, + N_("fields are separated with NUL character"), '\0'), OPT_END() }; ac = parse_options(ac, av, prefix, options, worktree_usage, 0); if (ac) usage_with_options(worktree_usage, options); + else if (!line_terminator && !porcelain) + die(_("'-z' requires '--porcelain'")); else { struct worktree **worktrees = get_worktrees(); int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i; @@ -741,7 +752,8 @@ static int list(int ac, const char **av, const char *prefix) for (i = 0; worktrees[i]; i++) { if (porcelain) - show_worktree_porcelain(worktrees[i]); + show_worktree_porcelain(worktrees[i], + line_terminator); else show_worktree(worktrees[i], path_maxlen, abbrev); } diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh index 795ddca2e4..acd9f8ab84 100755 --- a/t/t2402-worktree-list.sh +++ b/t/t2402-worktree-list.sh @@ -71,6 +71,28 @@ test_expect_success '"list" all worktrees with locked annotation' ' ! grep "/unlocked *[0-9a-f].* locked$" out ' +test_expect_success '"list" all worktrees --porcelain -z' ' + test_when_finished "rm -rf here _actual actual expect && + git worktree prune" && + printf "worktree %sQHEAD %sQbranch %sQQ" \ + "$(git rev-parse --show-toplevel)" \ + "$(git rev-parse HEAD)" \ + "$(git symbolic-ref HEAD)" >expect && + git worktree add --detach here master && + printf "worktree %sQHEAD %sQdetachedQQ" \ + "$(git -C here rev-parse --show-toplevel)" \ + "$(git rev-parse HEAD)" >>expect && + git worktree list --porcelain -z >_actual && + cat _actual | tr "\0" Q >actual && + test_cmp expect actual +' + +test_expect_success '"list" -z fails without --porcelain' ' + test_when_finished "rm -rf here && git worktree prune" && + git worktree add --detach here master && + test_must_fail git worktree list -z +' + test_expect_success 'bare repo setup' ' git init --bare bare1 && echo "data" >file1 &&