| Message ID | 20210119212739.77882-6-rafaeloliveira.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | teach `worktree list` verbose mode and prunable annotations | expand |
Hi Rafael Thanks for reworking this to use c_quote_path(). I have a couple of comments below. On 19/01/2021 21:27, Rafael Silva wrote: > Commit c57b3367be (worktree: teach `list` to annotate locked worktree, > 2020-10-11) taught "git worktree list" to annotate locked worktrees by > appending "locked" text to its output, however, this is not listed in > the --porcelain format. > > Teach "list --porcelain" to do the same and add a "locked" attribute > followed by its reason, thus making both default and porcelain format > consistent. If the locked reason is not available then only "locked" > is shown. > > The output of the "git worktree list --porcelain" becomes like so: > > $ git worktree list --porcelain > ... > worktree /path/to/locked > HEAD 123abcdea123abcd123acbd123acbda123abcd12 > detached > locked > > worktree /path/to/locked-with-reason > HEAD abc123abc123abc123abc123abc123abc123abc1 > detached > locked reason why it is locked > ... > > In porcelain mode, if the lock reason contains special characters > such as newlines, they are escaped with backslashes and the entire > reason is enclosed in double quotes. For example: > > $ git worktree list --porcelain > ... > locked "worktree's path mounted in\nremovable device" > ... > > Furthermore, let's update the documentation to state that some > attributes in the porcelain format might be listed alone or together > with its value depending whether the value is available or not. Thus > documenting the case of the new "locked" attribute. > > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> > --- > Documentation/git-worktree.txt | 16 ++++++++++++++-- > builtin/worktree.c | 13 +++++++++++++ > t/t2402-worktree-list.sh | 30 ++++++++++++++++++++++++++++++ > 3 files changed, 57 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 02a706c4c0..7cb8124f28 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -377,8 +377,10 @@ Porcelain Format > The porcelain format has a line per attribute. 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 > -`worktree`, an empty line indicates the end of the record. For example: > +if the value is true. Some attributes (like `locked`) can be listed as a label > +only or with a value depending upon whether a reason is available. The first > +attribute of a working tree is always `worktree`, an empty line indicates the > +end of the record. For example: I think it would be helpful to document that the reasons are quoted according core.quotePath. I'm not sure if it is worth changing this but I wonder if it would be easier to parse the output if the names of attributes with optional reasons were always followed by a space even when there is no reason, otherwise the code that parses the output has to check for the name followed by a space or newline. A script that only cares if the worktree is locked can parse the output with l.starts_with("locked ") rather than having to do l.starts_with("locked ") || l == "locked\n" Best Wishes Phillip > ------------ > $ git worktree list --porcelain > @@ -393,6 +395,16 @@ worktree /path/to/other-linked-worktree > HEAD 1234abc1234abc1234abc1234abc1234abc1234a > detached > > +worktree /path/to/linked-worktree-locked-no-reason > +HEAD 5678abc5678abc5678abc5678abc5678abc5678c > +branch refs/heads/locked-no-reason > +locked > + > +worktree /path/to/linked-worktree-locked-with-reason > +HEAD 3456def3456def3456def3456def3456def3456b > +branch refs/heads/locked-with-reason > +locked reason why is locked > + > ------------ > > EXAMPLES > diff --git a/builtin/worktree.c b/builtin/worktree.c > index df90a5acca..98177f91d4 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -12,6 +12,7 @@ > #include "submodule.h" > #include "utf8.h" > #include "worktree.h" > +#include "quote.h" > > static const char * const worktree_usage[] = { > N_("git worktree add [<options>] <path> [<commit-ish>]"), > @@ -569,6 +570,8 @@ static int add(int ac, const char **av, const char *prefix) > > static void show_worktree_porcelain(struct worktree *wt) > { > + const char *reason; > + > printf("worktree %s\n", wt->path); > if (wt->is_bare) > printf("bare\n"); > @@ -579,6 +582,16 @@ static void show_worktree_porcelain(struct worktree *wt) > else if (wt->head_ref) > printf("branch %s\n", wt->head_ref); > } > + > + reason = worktree_lock_reason(wt); > + if (reason && *reason) { > + struct strbuf sb = STRBUF_INIT; > + quote_c_style(reason, &sb, NULL, 0); > + printf("locked %s\n", sb.buf); > + strbuf_release(&sb); > + } else if (reason) > + printf("locked\n"); > + > printf("\n"); > } > > diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh > index 1866ea09f6..1fe53c3309 100755 > --- a/t/t2402-worktree-list.sh > +++ b/t/t2402-worktree-list.sh > @@ -72,6 +72,36 @@ test_expect_success '"list" all worktrees with locked annotation' ' > ! grep "/unlocked *[0-9a-f].* locked$" out > ' > > +test_expect_success '"list" all worktrees --porcelain with locked' ' > + test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" && > + echo "locked" >expect && > + echo "locked with reason" >>expect && > + git worktree add --detach locked1 && > + git worktree add --detach locked2 && > + # unlocked worktree should not be annotated with "locked" > + git worktree add --detach unlocked && > + git worktree lock locked1 && > + git worktree lock locked2 --reason "with reason" && > + test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" && > + git worktree list --porcelain >out && > + grep "^locked" out >actual && > + test_cmp expect actual > +' > + > +test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' ' > + test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" && > + printf "locked \"locked\\\\r\\\\nreason\"\n" >expect && > + printf "locked \"locked\\\\nreason\"\n" >>expect && > + git worktree add --detach locked_lf && > + git worktree add --detach locked_crlf && > + git worktree lock locked_lf --reason "$(printf "locked\nreason")" && > + git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" && > + test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" && > + git worktree list --porcelain >out && > + grep "^locked" out >actual && > + test_cmp expect actual > +' > + > test_expect_success 'bare repo setup' ' > git init --bare bare1 && > echo "data" >file1 && >
Phillip Wood <phillip.wood123@gmail.com> writes: > l.starts_with("locked ") > rather than having to do > l.starts_with("locked ") || l == "locked\n" l.regexp_matches("^locked[\s\n]")? Jokes aside, isn't the "space separated list" meant to be used more like: attrs = l.splitAtEach(" ") if "locked" in attrs: ... yeah it is locked ... if "broken" in attrs: ... ouch, it is broken ... so I am not sure if having always a trailing whitespace is a good idea to begin with (the last element may become an empty string if the splitting is done naively).
Hi Phillip, Phillip Wood writes: > Hi Rafael > > Thanks for reworking this to use c_quote_path(). I have a couple of > comments below. > Thanks for reviewing this patch. > On 19/01/2021 21:27, Rafael Silva wrote: >> Commit c57b3367be (worktree: teach `list` to annotate locked worktree, >> 2020-10-11) taught "git worktree list" to annotate locked worktrees by >> appending "locked" text to its output, however, this is not listed in >> the --porcelain format. >> Teach "list --porcelain" to do the same and add a "locked" attribute >> followed by its reason, thus making both default and porcelain format >> consistent. If the locked reason is not available then only "locked" >> is shown. >> The output of the "git worktree list --porcelain" becomes like so: >> $ git worktree list --porcelain >> ... >> worktree /path/to/locked >> HEAD 123abcdea123abcd123acbd123acbda123abcd12 >> detached >> locked >> worktree /path/to/locked-with-reason >> HEAD abc123abc123abc123abc123abc123abc123abc1 >> detached >> locked reason why it is locked >> ... >> In porcelain mode, if the lock reason contains special characters >> such as newlines, they are escaped with backslashes and the entire >> reason is enclosed in double quotes. For example: >> $ git worktree list --porcelain >> ... >> locked "worktree's path mounted in\nremovable device" >> ... >> Furthermore, let's update the documentation to state that some >> attributes in the porcelain format might be listed alone or together >> with its value depending whether the value is available or not. Thus >> documenting the case of the new "locked" attribute. >> Helped-by: Eric Sunshine <sunshine@sunshineco.com> >> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> >> --- >> Documentation/git-worktree.txt | 16 ++++++++++++++-- >> builtin/worktree.c | 13 +++++++++++++ >> t/t2402-worktree-list.sh | 30 ++++++++++++++++++++++++++++++ >> 3 files changed, 57 insertions(+), 2 deletions(-) >> diff --git a/Documentation/git-worktree.txt >> b/Documentation/git-worktree.txt >> index 02a706c4c0..7cb8124f28 100644 >> --- a/Documentation/git-worktree.txt >> +++ b/Documentation/git-worktree.txt >> @@ -377,8 +377,10 @@ Porcelain Format >> The porcelain format has a line per attribute. 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 >> -`worktree`, an empty line indicates the end of the record. For example: >> +if the value is true. Some attributes (like `locked`) can be listed as a label >> +only or with a value depending upon whether a reason is available. The first >> +attribute of a working tree is always `worktree`, an empty line indicates the >> +end of the record. For example: > > I think it would be helpful to document that the reasons are quoted > according core.quotePath. > Good point. I'll include this addition on the next revision.
On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > [...] > Teach "list --porcelain" to do the same and add a "locked" attribute > followed by its reason, thus making both default and porcelain format > consistent. If the locked reason is not available then only "locked" > is shown. > [...] > Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> > --- > diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh > @@ -72,6 +72,36 @@ test_expect_success '"list" all worktrees with locked annotation' ' > +test_expect_success '"list" all worktrees --porcelain with locked' ' > + test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" && > + echo "locked" >expect && > + echo "locked with reason" >>expect && > + git worktree add --detach locked1 && > + git worktree add --detach locked2 && > + # unlocked worktree should not be annotated with "locked" > + git worktree add --detach unlocked && > + git worktree lock locked1 && > + git worktree lock locked2 --reason "with reason" && > + test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" && There's a minor problem here. If the second `git worktree lock` command fails, test_when_finished() will never be invoked, which means that the first lock won't get cleaned up, thus the worktree won't get pruned. To fix, you'd want: git worktree lock locked1 && test_when_finished "git worktree unlock locked1" && git worktree lock locked2 --reason "with reason" && test_when_finished "git worktree unlock locked2" && > + git worktree list --porcelain >out && > + grep "^locked" out >actual && > + test_cmp expect actual > +' > + > +test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' ' > + test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" && > + printf "locked \"locked\\\\r\\\\nreason\"\n" >expect && > + printf "locked \"locked\\\\nreason\"\n" >>expect && > + git worktree add --detach locked_lf && > + git worktree add --detach locked_crlf && > + git worktree lock locked_lf --reason "$(printf "locked\nreason")" && > + git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" && > + test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" && Same issue as above. > + git worktree list --porcelain >out && > + grep "^locked" out >actual && > + test_cmp expect actual > +'
On Wed, Jan 20, 2021 at 6:00 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 19/01/2021 21:27, Rafael Silva wrote: > > The porcelain format has a line per attribute. 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. Some attributes (like `locked`) can be listed as a label > > +only or with a value depending upon whether a reason is available. The first > > +attribute of a working tree is always `worktree`, an empty line indicates the > > +end of the record. For example: > > I think it would be helpful to document that the reasons are quoted > according core.quotePath. Good idea. > I'm not sure if it is worth changing this but I wonder if it would be > easier to parse the output if the names of attributes with optional > reasons were always followed by a space even when there is no reason, > otherwise the code that parses the output has to check for the name > followed by a space or newline. A script that only cares if the worktree > is locked can parse the output with > l.starts_with("locked ") > rather than having to do > l.starts_with("locked ") || l == "locked\n" I see where you're coming from with this suggestion, though my knee-jerk reaction is that it would be undesirable. Even after mulling it over for a few days, I still haven't managed to convince myself that it would be a good idea. There are a couple reasons (at least) for my negative reaction. The primary reason is that the trailing space is "invisible", and as such could end up being as confusing as it is helpful for the simple parsing case (taking into consideration that people often don't consult documentation). The second reason is that we're already expecting clients to be able to parse C-style quoting/escaping of the reason, so asking them to also distinguish between a single token `locked` and a `locked reason-for-lock` seems like very, very minor extra complexity. (It also just feels a bit sloppy to have that trailing space, but that's a quite minor concern.)
Eric Sunshine writes: > On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva > <rafaeloliveira.cs@gmail.com> wrote: >> [...] >> Teach "list --porcelain" to do the same and add a "locked" attribute >> followed by its reason, thus making both default and porcelain format >> consistent. If the locked reason is not available then only "locked" >> is shown. >> [...] >> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> >> --- >> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh >> @@ -72,6 +72,36 @@ test_expect_success '"list" all worktrees with locked annotation' ' >> +test_expect_success '"list" all worktrees --porcelain with locked' ' >> + test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" && >> + echo "locked" >expect && >> + echo "locked with reason" >>expect && >> + git worktree add --detach locked1 && >> + git worktree add --detach locked2 && >> + # unlocked worktree should not be annotated with "locked" >> + git worktree add --detach unlocked && >> + git worktree lock locked1 && >> + git worktree lock locked2 --reason "with reason" && >> + test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" && > > There's a minor problem here. If the second `git worktree lock` > command fails, test_when_finished() will never be invoked, which means > that the first lock won't get cleaned up, thus the worktree won't get > pruned. To fix, you'd want: > > git worktree lock locked1 && > test_when_finished "git worktree unlock locked1" && > git worktree lock locked2 --reason "with reason" && > test_when_finished "git worktree unlock locked2" && > Excellent point. This case didn't occur to me when I was working on v3, I will make this change in the next revision. >> + git worktree list --porcelain >out && >> + grep "^locked" out >actual && >> + test_cmp expect actual >> +' >> + >> +test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' ' >> + test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" && >> + printf "locked \"locked\\\\r\\\\nreason\"\n" >expect && >> + printf "locked \"locked\\\\nreason\"\n" >>expect && >> + git worktree add --detach locked_lf && >> + git worktree add --detach locked_crlf && >> + git worktree lock locked_lf --reason "$(printf "locked\nreason")" && >> + git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" && >> + test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" && > > Same issue as above. > >> + git worktree list --porcelain >out && >> + grep "^locked" out >actual && >> + test_cmp expect actual >> +'
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 02a706c4c0..7cb8124f28 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -377,8 +377,10 @@ Porcelain Format The porcelain format has a line per attribute. 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 -`worktree`, an empty line indicates the end of the record. For example: +if the value is true. Some attributes (like `locked`) can be listed as a label +only or with a value depending upon whether a reason is available. The first +attribute of a working tree is always `worktree`, an empty line indicates the +end of the record. For example: ------------ $ git worktree list --porcelain @@ -393,6 +395,16 @@ worktree /path/to/other-linked-worktree HEAD 1234abc1234abc1234abc1234abc1234abc1234a detached +worktree /path/to/linked-worktree-locked-no-reason +HEAD 5678abc5678abc5678abc5678abc5678abc5678c +branch refs/heads/locked-no-reason +locked + +worktree /path/to/linked-worktree-locked-with-reason +HEAD 3456def3456def3456def3456def3456def3456b +branch refs/heads/locked-with-reason +locked reason why is locked + ------------ EXAMPLES diff --git a/builtin/worktree.c b/builtin/worktree.c index df90a5acca..98177f91d4 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -12,6 +12,7 @@ #include "submodule.h" #include "utf8.h" #include "worktree.h" +#include "quote.h" static const char * const worktree_usage[] = { N_("git worktree add [<options>] <path> [<commit-ish>]"), @@ -569,6 +570,8 @@ static int add(int ac, const char **av, const char *prefix) static void show_worktree_porcelain(struct worktree *wt) { + const char *reason; + printf("worktree %s\n", wt->path); if (wt->is_bare) printf("bare\n"); @@ -579,6 +582,16 @@ static void show_worktree_porcelain(struct worktree *wt) else if (wt->head_ref) printf("branch %s\n", wt->head_ref); } + + reason = worktree_lock_reason(wt); + if (reason && *reason) { + struct strbuf sb = STRBUF_INIT; + quote_c_style(reason, &sb, NULL, 0); + printf("locked %s\n", sb.buf); + strbuf_release(&sb); + } else if (reason) + printf("locked\n"); + printf("\n"); } diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh index 1866ea09f6..1fe53c3309 100755 --- a/t/t2402-worktree-list.sh +++ b/t/t2402-worktree-list.sh @@ -72,6 +72,36 @@ test_expect_success '"list" all worktrees with locked annotation' ' ! grep "/unlocked *[0-9a-f].* locked$" out ' +test_expect_success '"list" all worktrees --porcelain with locked' ' + test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" && + echo "locked" >expect && + echo "locked with reason" >>expect && + git worktree add --detach locked1 && + git worktree add --detach locked2 && + # unlocked worktree should not be annotated with "locked" + git worktree add --detach unlocked && + git worktree lock locked1 && + git worktree lock locked2 --reason "with reason" && + test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" && + git worktree list --porcelain >out && + grep "^locked" out >actual && + test_cmp expect actual +' + +test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' ' + test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" && + printf "locked \"locked\\\\r\\\\nreason\"\n" >expect && + printf "locked \"locked\\\\nreason\"\n" >>expect && + git worktree add --detach locked_lf && + git worktree add --detach locked_crlf && + git worktree lock locked_lf --reason "$(printf "locked\nreason")" && + git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" && + test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" && + git worktree list --porcelain >out && + grep "^locked" out >actual && + test_cmp expect actual +' + test_expect_success 'bare repo setup' ' git init --bare bare1 && echo "data" >file1 &&
Commit c57b3367be (worktree: teach `list` to annotate locked worktree, 2020-10-11) taught "git worktree list" to annotate locked worktrees by appending "locked" text to its output, however, this is not listed in the --porcelain format. Teach "list --porcelain" to do the same and add a "locked" attribute followed by its reason, thus making both default and porcelain format consistent. If the locked reason is not available then only "locked" is shown. The output of the "git worktree list --porcelain" becomes like so: $ git worktree list --porcelain ... worktree /path/to/locked HEAD 123abcdea123abcd123acbd123acbda123abcd12 detached locked worktree /path/to/locked-with-reason HEAD abc123abc123abc123abc123abc123abc123abc1 detached locked reason why it is locked ... In porcelain mode, if the lock reason contains special characters such as newlines, they are escaped with backslashes and the entire reason is enclosed in double quotes. For example: $ git worktree list --porcelain ... locked "worktree's path mounted in\nremovable device" ... Furthermore, let's update the documentation to state that some attributes in the porcelain format might be listed alone or together with its value depending whether the value is available or not. Thus documenting the case of the new "locked" attribute. Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> --- Documentation/git-worktree.txt | 16 ++++++++++++++-- builtin/worktree.c | 13 +++++++++++++ t/t2402-worktree-list.sh | 30 ++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-)