threads / patch / 34598

patchcherry-pick: allow "-" as abbreviation of '@{-1}'

Subject: [PATCH] cherry-pick: allow "-" as abbreviation of '@{-1}'

## tl;dr

4 messages between Aug 3, 2013 and Aug 16, 2013. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Hiroshige Umino· Aug 3, 2013, 05:15 UTC · lore

As "git cherry-pick -" or "git merge -" is convenient to switch back to or merge the previous branch, "git cherry-pick -" is abbreviation of "git cherry-pick @{-1}" to pick up a commit from the previous branch conveniently.

Signed-off-by: Hiroshige Umino <hiroshige88@gmail.com>
---
 builtin/revert.c            |  2 ++
 t/t3512-cherry-pick-last.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100755 t/t3512-cherry-pick-last.sh
Show changes to 2 files +30 −0

builtin/revert.c, t/t3512-cherry-pick-last.sh

diff --git a/builtin/revert.c b/builtin/revert.c
index 1d2648b..cb403f8 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -234,6 +234,8 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	memset(&opts, 0, sizeof(opts));
 	opts.action = REPLAY_PICK;
 	git_config(git_default_config, NULL);
+	if (!strcmp(argv[1], "-"))
+		argv[1] = "@{-1}";
 	parse_args(argc, argv, &opts);
 	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
diff --git a/t/t3512-cherry-pick-last.sh b/t/t3512-cherry-pick-last.sh
new file mode 100755
index 0000000..8b05f81
--- /dev/null
+++ b/t/t3512-cherry-pick-last.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_description='Test cherry-pick -'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ echo hello >world &&
+ git add world &&
+ git commit -m initial &&
+ git branch other &&
+ echo "hello again" >>world &&
+ git add world &&
+ git commit -m second
+'
+
+test_expect_success '"cherry-pick -" does not work initially' '
+ test_must_fail git cherry-pick -
+'
+
+test_expect_success 'cherry-pick the commit in the previous branch' '
+ prev=$(git rev-parse HEAD) &&
+ git checkout other &&
+ git cherry-pick - &&
+ test "z$(git rev-parse HEAD)" = "z$prev"
+'
+
+test_done

-- 
1.8.3.3
Thomas Rast· Aug 3, 2013, 10:51 UTC · re: Hiroshige Umino · lore

Re: [PATCH] cherry-pick: allow "-" as abbreviation of '@{-1}'

Hiroshige Umino <hiroshige88@gmail.com> writes:
> As "git cherry-pick -" or "git merge -" is convenient to
> switch back to or merge the previous branch,
> "git cherry-pick -" is abbreviation of "git cherry-pick @{-1}"
> to pick up a commit from the previous branch conveniently.

The first line is confusing. Did you mean to invoke the existing 'git *checkout* -' and 'git merge -' functionality as a reason why 'git cherry-pick -' should exist?

What other commands could reasonably use the '-' shorthand?
[...]
> diff --git a/t/t3512-cherry-pick-last.sh b/t/t3512-cherry-pick-last.sh
Do you have to use a new test file for this?
[...]
> +test_expect_success 'setup' '
> + echo hello >world &&
> + git add world &&
(*)
> + git commit -m initial &&
> + git branch other &&
> + echo "hello again" >>world &&
> + git add world &&
(*)
> + git commit -m second
> +'

Our style is to indent the test snippets with a hard tab, not a single (or eight, for that matter) space.

[...]
> +test_expect_success 'cherry-pick the commit in the previous branch' '
> + prev=$(git rev-parse HEAD) &&
> + git checkout other &&
(*)
> + git cherry-pick - &&
> + test "z$(git rev-parse HEAD)" = "z$prev"
> +'
If you insert 'test_tick' in the places marked with (*), the test fails.

The tests run under a fake clock to ensure that everything, including the SHA1s produced, are deterministic. You never advance the clock, so all commits generated in this script share the same timestamp.

This means that the cherry-pick of 'second' has the same SHA1 as the original: its tree, parents, author, timestamp etc. all agree. If you advance the clock at the last (*), this fails. You should find some other way of checking what was picked, e.g., by looking at the file contents.

That said, please use test_commit in the 'setup' snippet instead of manually rolling the commits. It will lead to shorter code, and it handles test_tick for you. It is documented in t/README and in a comment in t/test-lib-functions.sh. (You still need test_tick immediately before the cherry-pick!)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Aug 5, 2013, 16:27 UTC · re: Thomas Rast · lore

Re: [PATCH] cherry-pick: allow "-" as abbreviation of '@{-1}'

Thomas Rast <trast@inf.ethz.ch> writes:
Show 10 quoted lines
> Hiroshige Umino <hiroshige88@gmail.com> writes:
>
>> As "git cherry-pick -" or "git merge -" is convenient to
>> switch back to or merge the previous branch,
>> "git cherry-pick -" is abbreviation of "git cherry-pick @{-1}"
>> to pick up a commit from the previous branch conveniently.
>
> The first line is confusing.  Did you mean to invoke the existing 'git
> *checkout* -' and 'git merge -' functionality as a reason why 'git
> cherry-pick -' should exist?

I think that is what was meant. Just like "-" abbreviation is handy for users of "checkout" and "merge", "cherry-pick" might. I am not sure if you would often cherry-pick from the previous branch, but for the sake of completeness and uniformity, the guiding principle could be "at any point on the command line where a branch name is accepted, if a '-' could not possibly mean any other thing is wanted (e.g. doing something on the standard input), it should stand as the name of the previous branch".

I agree with everything you said in your review. The patch is going in the right direction, but it needs a bit more work.

Thanks.
Hiroshige Umino· Aug 16, 2013, 15:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] cherry-pick: allow "-" as abbreviation of '@{-1}'

Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Thomas Rast <trast@inf.ethz.ch> writes:
> 
>> Hiroshige Umino <hiroshige88@gmail.com> writes:
>>
>>> As "git cherry-pick -" or "git merge -" is convenient to
>>> switch back to or merge the previous branch,
>>> "git cherry-pick -" is abbreviation of "git cherry-pick @{-1}"
>>> to pick up a commit from the previous branch conveniently.
>>
>> The first line is confusing.  Did you mean to invoke the existing 'git
>> *checkout* -' and 'git merge -' functionality as a reason why 'git
>> cherry-pick -' should exist?
> 
> I think that is what was meant.  Just like "-" abbreviation is handy
> for users of "checkout" and "merge", "cherry-pick" might.

Yes I meant so and it would be useful at least for me. I don't know the usage of cherry-pick (pick up a commit from the previous branch) is comon or not but it may be also good for consistency.

Thomas Rast <trast@inf.ethz.ch> wrote:
> What other commands could reasonably use the '-' shorthand?

I've wanted '-' shorthand only for commit, merge and cherry-pick but 'git diff -' may make sense. What do you think of this and other candidates?

> Do you have to use a new test file for this?
Not have to so I'm moving the tests into t/t3500-cherry.sh.
Show 42 quoted lines
> [...]
>> +test_expect_success 'setup' '
>> + echo hello >world &&
>> + git add world &&
> (*)
>> + git commit -m initial &&
>> + git branch other &&
>> + echo "hello again" >>world &&
>> + git add world &&
> (*)
>> + git commit -m second
>> +'
> 
> Our style is to indent the test snippets with a hard tab, not a single
> (or eight, for that matter) space.
> 
> [...]
>> +test_expect_success 'cherry-pick the commit in the previous branch' '
>> + prev=$(git rev-parse HEAD) &&
>> + git checkout other &&
> (*)
>> + git cherry-pick - &&
>> + test "z$(git rev-parse HEAD)" = "z$prev"
>> +'
> 
> If you insert 'test_tick' in the places marked with (*), the test fails.
> 
> The tests run under a fake clock to ensure that everything, including
> the SHA1s produced, are deterministic.  You never advance the clock, so
> all commits generated in this script share the same timestamp.
> 
> This means that the cherry-pick of 'second' has the same SHA1 as the
> original: its tree, parents, author, timestamp etc. all agree.  If you
> advance the clock at the last (*), this fails.  You should find some
> other way of checking what was picked, e.g., by looking at the file
> contents.
> 
> That said, please use test_commit in the 'setup' snippet instead of
> manually rolling the commits.  It will lead to shorter code, and it
> handles test_tick for you.  It is documented in t/README and in a
> comment in t/test-lib-functions.sh.  (You still need test_tick
> immediately before the cherry-pick!)
I overlooked t/README, thank you for kindly guiding testing!

-- Hiroshige UMINO @yaotti

← back to recent threads