threads / patch / 42456

patchworktree: allow "-" short-hand for @{-1} in add command

Subject: [PATCH] worktree: allow "-" short-hand for @{-1} in add command

## tl;dr

7 messages between May 26, 2016 and May 27, 2016. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Jordan DE GEA· May 26, 2016, 11:54 UTC · lore
From: Jordan DE GEA <jordan.de-gea@ensimag.grenoble-inp.fr>

Since `git worktree add` uses `git checkout` when `[<branch>]` is used, and `git checkout -` is already supported, it makes sense to allow the same shortcut in `git worktree add`.

Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Signed-off-by: Jordan DE GEA <jordan.de-gea@grenoble-inp.org>
---
 Documentation/git-worktree.txt |  3 ++-
 builtin/worktree.c             |  3 +++
 t/t2025-worktree-add.sh        | 18 ++++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)
Show changes to 3 files +23 −1

Documentation/git-worktree.txt, builtin/worktree.c, t/t2025-worktree-add.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index c622345..48e5fdf 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -48,7 +48,8 @@ add <path> [<branch>]::
 
 Create `<path>` and checkout `<branch>` into it. The new working directory
 is linked to the current repository, sharing everything except working
-directory specific files such as HEAD, index, etc.
+directory specific files such as HEAD, index, etc. The last branch you 
+were on can be specify with `-` as `<branch>` which is synonymous with `"@{-1}"`.
 +
 If `<branch>` is omitted and neither `-b` nor `-B` nor `--detached` used,
 then, as a convenience, a new branch based at HEAD is created automatically,
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d8e3795..d800d47 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -340,6 +340,9 @@ static int add(int ac, const char **av, const char *prefix)
 	path = prefix ? prefix_filename(prefix, strlen(prefix), av[0]) : av[0];
 	branch = ac < 2 ? "HEAD" : av[1];
 
+	if (!strcmp(branch, "-"))
+		branch = "@{-1}";
+
 	opts.force_new_branch = !!new_branch_force;
 	if (opts.force_new_branch) {
 		struct strbuf symref = STRBUF_INIT;
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 3acb992..b713efb 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -18,6 +18,24 @@ test_expect_success '"add" an existing empty worktree' '
 	git worktree add --detach existing_empty master
 '
 
+test_expect_success '"add" using shorthand - fails when no previous branch' '
+	test_must_fail git worktree add existing -
+'
+
+test_expect_success '"add" using - shorthand' '
+	git checkout -b newbranch &&
+	echo hello >myworld &&
+	git add myworld &&
+	git commit -m myworld &&
+	git checkout master &&
+	git worktree add short-hand - &&
+	cd short-hand &&
+	test $(git rev-parse --symbolic-full-name HEAD) = "refs/heads/newbranch"
+	branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
+	test "$branch" = refs/heads/newbranch &&
+	cd ..
+'
+
 test_expect_success '"add" refuses to checkout locked branch' '
 	test_must_fail git worktree add zere master &&
 	! test -d zere &&

-- 
2.7.4 (Apple Git-66)
Junio C Hamano· May 26, 2016, 18:54 UTC · re: Jordan DE GEA · lore

Re: [PATCH] worktree: allow "-" short-hand for @{-1} in add command

Jordan DE GEA <jordan.de-gea@grenoble-inp.org> writes:
Show 5 quoted lines
> From: Jordan DE GEA <jordan.de-gea@ensimag.grenoble-inp.fr>
>
> Since `git worktree add` uses `git checkout` when `[<branch>]` is used,
> and `git checkout -` is already supported, it makes sense to allow the
> same shortcut in `git worktree add`.
OK.
Show 20 quoted lines
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Signed-off-by: Jordan DE GEA <jordan.de-gea@grenoble-inp.org>
> ---
>  Documentation/git-worktree.txt |  3 ++-
>  builtin/worktree.c             |  3 +++
>  t/t2025-worktree-add.sh        | 18 ++++++++++++++++++
>  3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index c622345..48e5fdf 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -48,7 +48,8 @@ add <path> [<branch>]::
>  
>  Create `<path>` and checkout `<branch>` into it. The new working directory
>  is linked to the current repository, sharing everything except working
> -directory specific files such as HEAD, index, etc.
> +directory specific files such as HEAD, index, etc. The last branch you 
> +were on can be specify with `-` as `<branch>` which is synonymous with `"@{-1}"`.

You meant "can be specified", I think. Fixing it would make the line a bit too long, so fold it around the word "synonymous".

Show 11 quoted lines
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 3acb992..b713efb 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -18,6 +18,24 @@ test_expect_success '"add" an existing empty worktree' '
>  	git worktree add --detach existing_empty master
>  '
>  
> +test_expect_success '"add" using shorthand - fails when no previous branch' '
> +	test_must_fail git worktree add existing -
> +'

Just an observation, but the error message we would see here might be interesting.

Show 7 quoted lines
> +test_expect_success '"add" using - shorthand' '
> +	git checkout -b newbranch &&
> +	echo hello >myworld &&
> +	git add myworld &&
> +	git commit -m myworld &&
> +	git checkout master &&
> +	git worktree add short-hand - &&
> +	cd short-hand &&
> +	test $(git rev-parse --symbolic-full-name HEAD) = "refs/heads/newbranch"
Broken &&-chain.
> +	branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
> +	test "$branch" = refs/heads/newbranch &&
> +	cd ..

If any of the command between "cd short-hand" and "cd .." failed, after correcting the broken &&-chain, the next test will end up running in short-hand directory, which it is not expecting. A canonical way to avoid this problem is to replace the above with:

	...
        git worktree add short-hand - &&
        (
		cd short-hand &&
                ...
                test "$branch" = refs/heads/newbranch
	)

In this particular case, alternatively, you could also do something like this:

        git worktree add short-hand - &&
	echo refs/heads/newbranch >expect &&
	git -C short-hand rev-parse --symbolic-full-name HEAD >actual &&
	test_cmp expect actual
and it would be sufficient.

It is not immediately obvious to me why you have two copies of the same test in your patch to see where HEAD points at. If the reason is because you suspect that "git -C $there" form may give subtly different behaviour and wanted to test both, then you could do something like this:

        git worktree add short-hand - &&
	echo refs/heads/newbranch >expect &&
	git -C short-hand rev-parse --symbolic-full-name HEAD >actual &&
	test_cmp expect actual &&
	(cd short-hand && git rev-parse --symbolic-full-name HEAD) >actual &&
	test_cmp expect actual

but I do not think that is necessary. This test is not about "does rev-parse --symbolic-full-name work correctly with 'git -C $there'?"

Thanks.
Matthieu Moy· May 27, 2016, 09:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] worktree: allow "-" short-hand for @{-1} in add command

Junio C Hamano <gitster@pobox.com> writes:
Show 18 quoted lines
> Jordan DE GEA <jordan.de-gea@grenoble-inp.org> writes:
>
>> +	branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
>> +	test "$branch" = refs/heads/newbranch &&
>> +	cd ..
>
> If any of the command between "cd short-hand" and "cd .." failed,
> after correcting the broken &&-chain, the next test will end up
> running in short-hand directory, which it is not expecting.  A
> canonical way to avoid this problem is to replace the above with:
>
> 	...
>         git worktree add short-hand - &&
>         (
> 		cd short-hand &&
>                 ...
>                 test "$branch" = refs/heads/newbranch
> 	)

Actually, $(...) implicitly does a subshell, so the "cd .." was just useless.

> 	git -C short-hand rev-parse --symbolic-full-name HEAD >actual &&
Indeed, git -C is an even better way to say "cd .. && git ..."
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· May 27, 2016, 17:52 UTC · re: Matthieu Moy · lore

Re: [PATCH] worktree: allow "-" short-hand for @{-1} in add command

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 23 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jordan DE GEA <jordan.de-gea@grenoble-inp.org> writes:
>>
>>> +	branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
>>> +	test "$branch" = refs/heads/newbranch &&
>>> +	cd ..
>>
>> If any of the command between "cd short-hand" and "cd .." failed,
>> after correcting the broken &&-chain, the next test will end up
>> running in short-hand directory, which it is not expecting.  A
>> canonical way to avoid this problem is to replace the above with:
>>
>> 	...
>>         git worktree add short-hand - &&
>>         (
>> 		cd short-hand &&
>>                 ...
>>                 test "$branch" = refs/heads/newbranch
>> 	)
>
> Actually, $(...) implicitly does a subshell, so the "cd .." was just
> useless.

You trimmed my message a bit too aggressively while composing your response, and I think that is what ended up confusing you. 

Here is what I wrote:
| > +	cd short-hand &&
| > +	test $(git rev-parse --symbolic-full-name HEAD) = "refs/heads/newbranch"
| 
| Broken &&-chain.
| 
| > +	branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
| > +	test "$branch" = refs/heads/newbranch &&
| > +	cd ..
| 
| If any of the command between "cd short-hand" and "cd .." failed,
| ...

The problematic "cd short-hand" is the one a few lines above where you started quoting, not the one you saw in the $().

Jordan DE GEA· May 27, 2016, 12:43 UTC · re: Junio C Hamano · lore

Re: [PATCH] worktree: allow "-" short-hand for @{-1} in add command

Show 6 quoted lines
>> +test_expect_success '"add" using shorthand - fails when no previous branch' '
>> +	test_must_fail git worktree add existing -
>> +'
> 
> Just an observation, but the error message we would see here might
> be interesting.
Of course, that’s useful to be sure of the error, I will do in next preroll.
Show 25 quoted lines
> 
>> +	branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
>> +	test "$branch" = refs/heads/newbranch &&
>> +	cd ..
> 
> If any of the command between "cd short-hand" and "cd .." failed,
> after correcting the broken &&-chain, the next test will end up
> running in short-hand directory, which it is not expecting.  A
> canonical way to avoid this problem is to replace the above with:
> 
> 	...
>        git worktree add short-hand - &&
>        (
> 		cd short-hand &&
>                ...
>                test "$branch" = refs/heads/newbranch
> 	)
> 
> In this particular case, alternatively, you could also do something
> like this:
> 
>        git worktree add short-hand - &&
> 	echo refs/heads/newbranch >expect &&
> 	git -C short-hand rev-parse --symbolic-full-name HEAD >actual &&
> 	test_cmp expect actual
Yes, that’s a good idea. I take these lines.
Jordan DE GEA· May 27, 2016, 13:17 UTC · re: Jordan DE GEA · lore

[PATCH v2] worktree: allow "-" short-hand for @{-1} in add command

Since `git worktree add` uses `git checkout` when `[<branch>]` is used, and `git checkout -` is already supported, it makes sense to allow the same shortcut in `git worktree add`.

Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Signed-off-by: Jordan DE GEA <jordan.de-gea@grenoble-inp.org>
---
Changes since v1:
  - improved tests.
  - improved documentation.
 Documentation/git-worktree.txt |  3 ++-
 builtin/worktree.c             |  3 +++
 t/t2025-worktree-add.sh        | 18 ++++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)
Show changes to 3 files +23 −1

Documentation/git-worktree.txt, builtin/worktree.c, t/t2025-worktree-add.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index c622345..8358a3e 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -48,7 +48,8 @@ add <path> [<branch>]::
 
 Create `<path>` and checkout `<branch>` into it. The new working directory
 is linked to the current repository, sharing everything except working
-directory specific files such as HEAD, index, etc.
+directory specific files such as HEAD, index, etc. `-` may also be 
+specified as `<branch>`; it is synonymous with `@{-1}`.
 +
 If `<branch>` is omitted and neither `-b` nor `-B` nor `--detached` used,
 then, as a convenience, a new branch based at HEAD is created automatically,
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d8e3795..d800d47 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -340,6 +340,9 @@ static int add(int ac, const char **av, const char *prefix)
 	path = prefix ? prefix_filename(prefix, strlen(prefix), av[0]) : av[0];
 	branch = ac < 2 ? "HEAD" : av[1];
 
+	if (!strcmp(branch, "-"))
+		branch = "@{-1}";
+
 	opts.force_new_branch = !!new_branch_force;
 	if (opts.force_new_branch) {
 		struct strbuf symref = STRBUF_INIT;
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 3acb992..c4f5177 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -18,6 +18,24 @@ test_expect_success '"add" an existing empty worktree' '
 	git worktree add --detach existing_empty master
 '
 
+test_expect_success '"add" using shorthand - fails when no previous branch' '
+	echo "fatal: invalid reference: @{-1}" >expect &&
+	test_must_fail git worktree add existing_short - 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '"add" using - shorthand' '
+	git checkout -b newbranch &&
+	echo hello >myworld &&
+	git add myworld &&
+	git commit -m myworld &&
+	git checkout master &&
+	git worktree add short-hand - &&
+	echo refs/heads/newbranch >expect &&
+	git -C short-hand rev-parse --symbolic-full-name HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '"add" refuses to checkout locked branch' '
 	test_must_fail git worktree add zere master &&
 	! test -d zere &&

-- 
2.7.4 (Apple Git-66)
Junio C Hamano· May 27, 2016, 18:01 UTC · re: Jordan DE GEA · lore

Re: [PATCH] worktree: allow "-" short-hand for @{-1} in add command

Jordan DE GEA <jordan.de-gea@grenoble-inp.org> writes:
Show 8 quoted lines
>>> +test_expect_success '"add" using shorthand - fails when no previous branch' '
>>> +	test_must_fail git worktree add existing -
>>> +'
>> 
>> Just an observation, but the error message we would see here might
>> be interesting.
>
> Of course, that’s useful to be sure of the error, I will do in next preroll.

That was not what I meant. The exit status being non-zero is what we primarily care about. The exact phrasing of the error message is much less important and in general we shouldn't enforce "the error message must remain so" in the test.

If you observe the error message from this test, e.g. by running it with "-v", I suspect that you would see the message would complain about "@{-1}".

I just wanted to make sure that you saw it and thought about its ramifications.

It is perfectly fine by me (others might disagree, though) if your conclusion after thinking about it is "Even though the user may be surprised to get complaints for "@{-1}" that she never gave to the command (she gave "-"), because we clearly document that "-" is a mere synonym/short-hand for @{-1}, it is OK". I still want to see that behaviour justified in the proposed commit log message.

And that is why I said it "might be interesting".

← back to recent threads