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

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

[Hiroshige Umino]

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

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

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

[Thomas Rast]

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

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

[Junio C Hamano]

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.  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.

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

[Hiroshige Umino]

Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> [...]
>> +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