{"thread":{"id":"34598","subject":"[PATCH] cherry-pick: allow \"-\" as abbreviation of '@{-1}'","startedAt":"2013-08-03T05:15:13Z","lastAt":"2013-08-16T15:17:37Z","messageCount":4,"participants":["Hiroshige Umino","Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224486","messageId":"1375506913-3390-1-git-send-email-hiroshige88@gmail.com","threadId":"34598","inReplyTo":null,"subject":"[PATCH] cherry-pick: allow \"-\" as abbreviation of '@{-1}'","fromName":"Hiroshige Umino","fromEmail":"hiroshige88@gmail.com","sentAt":"2013-08-03T05:15:13Z","receivedAt":"2013-08-03T05:15:13Z","isPatch":true,"sender":{"key":"hiroshige88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/18807?v=4"},"body":"As \"git cherry-pick -\" or \"git merge -\" is convenient to\nswitch back to or merge the previous branch,\n\"git cherry-pick -\" is abbreviation of \"git cherry-pick @{-1}\"\nto pick up a commit from the previous branch conveniently.\n\nSigned-off-by: Hiroshige Umino <hiroshige88@gmail.com>\n---\n builtin/revert.c            |  2 ++\n t/t3512-cherry-pick-last.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+)\n create mode 100755 t/t3512-cherry-pick-last.sh\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1d2648b..cb403f8 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -234,6 +234,8 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \tmemset(&opts, 0, sizeof(opts));\n \topts.action = REPLAY_PICK;\n \tgit_config(git_default_config, NULL);\n+\tif (!strcmp(argv[1], \"-\"))\n+\t\targv[1] = \"@{-1}\";\n \tparse_args(argc, argv, &opts);\n \tres = sequencer_pick_revisions(&opts);\n \tif (res < 0)\ndiff --git a/t/t3512-cherry-pick-last.sh b/t/t3512-cherry-pick-last.sh\nnew file mode 100755\nindex 0000000..8b05f81\n--- /dev/null\n+++ b/t/t3512-cherry-pick-last.sh\n@@ -0,0 +1,28 @@\n+#!/bin/sh\n+\n+test_description='Test cherry-pick -'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+ echo hello >world &&\n+ git add world &&\n+ git commit -m initial &&\n+ git branch other &&\n+ echo \"hello again\" >>world &&\n+ git add world &&\n+ git commit -m second\n+'\n+\n+test_expect_success '\"cherry-pick -\" does not work initially' '\n+ test_must_fail git cherry-pick -\n+'\n+\n+test_expect_success 'cherry-pick the commit in the previous branch' '\n+ prev=$(git rev-parse HEAD) &&\n+ git checkout other &&\n+ git cherry-pick - &&\n+ test \"z$(git rev-parse HEAD)\" = \"z$prev\"\n+'\n+\n+test_done\n-- \n1.8.3.3\n"},{"id":"224505","messageId":"87wqo33v4f.fsf@hexa.v.cablecom.net","threadId":"34598","inReplyTo":"1375506913-3390-1-git-send-email-hiroshige88@gmail.com","subject":"Re: [PATCH] cherry-pick: allow \"-\" as abbreviation of '@{-1}'","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-08-03T10:51:28Z","receivedAt":"2013-08-03T10:51:28Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Hiroshige Umino <hiroshige88@gmail.com> writes:\n\n> As \"git cherry-pick -\" or \"git merge -\" is convenient to\n> switch back to or merge the previous branch,\n> \"git cherry-pick -\" is abbreviation of \"git cherry-pick @{-1}\"\n> to pick up a commit from the previous branch conveniently.\n\nThe first line is confusing.  Did you mean to invoke the existing 'git\n*checkout* -' and 'git merge -' functionality as a reason why 'git\ncherry-pick -' should exist?\n\nWhat other commands could reasonably use the '-' shorthand?\n\n[...]\n> diff --git a/t/t3512-cherry-pick-last.sh b/t/t3512-cherry-pick-last.sh\n\nDo you have to use a new test file for this?\n\n[...]\n> +test_expect_success 'setup' '\n> + echo hello >world &&\n> + git add world &&\n(*)\n> + git commit -m initial &&\n> + git branch other &&\n> + echo \"hello again\" >>world &&\n> + git add world &&\n(*)\n> + git commit -m second\n> +'\n\nOur style is to indent the test snippets with a hard tab, not a single\n(or eight, for that matter) space.\n\n[...]\n> +test_expect_success 'cherry-pick the commit in the previous branch' '\n> + prev=$(git rev-parse HEAD) &&\n> + git checkout other &&\n(*)\n> + git cherry-pick - &&\n> + test \"z$(git rev-parse HEAD)\" = \"z$prev\"\n> +'\n\nIf you insert 'test_tick' in the places marked with (*), the test fails.\n\nThe tests run under a fake clock to ensure that everything, including\nthe SHA1s produced, are deterministic.  You never advance the clock, so\nall commits generated in this script share the same timestamp.\n\nThis means that the cherry-pick of 'second' has the same SHA1 as the\noriginal: its tree, parents, author, timestamp etc. all agree.  If you\nadvance the clock at the last (*), this fails.  You should find some\nother way of checking what was picked, e.g., by looking at the file\ncontents.\n\nThat said, please use test_commit in the 'setup' snippet instead of\nmanually rolling the commits.  It will lead to shorter code, and it\nhandles test_tick for you.  It is documented in t/README and in a\ncomment in t/test-lib-functions.sh.  (You still need test_tick\nimmediately before the cherry-pick!)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"224598","messageId":"7vmwowm7bc.fsf@alter.siamese.dyndns.org","threadId":"34598","inReplyTo":"87wqo33v4f.fsf@hexa.v.cablecom.net","subject":"Re: [PATCH] cherry-pick: allow \"-\" as abbreviation of '@{-1}'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-05T16:27:35Z","receivedAt":"2013-08-05T16:27:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@inf.ethz.ch> writes:\n\n> Hiroshige Umino <hiroshige88@gmail.com> writes:\n>\n>> As \"git cherry-pick -\" or \"git merge -\" is convenient to\n>> switch back to or merge the previous branch,\n>> \"git cherry-pick -\" is abbreviation of \"git cherry-pick @{-1}\"\n>> to pick up a commit from the previous branch conveniently.\n>\n> The first line is confusing.  Did you mean to invoke the existing 'git\n> *checkout* -' and 'git merge -' functionality as a reason why 'git\n> cherry-pick -' should exist?\n\nI think that is what was meant.  Just like \"-\" abbreviation is handy\nfor users of \"checkout\" and \"merge\", \"cherry-pick\" might.  I am not\nsure if you would often cherry-pick from the previous branch, but\nfor the sake of completeness and uniformity, the guiding principle\ncould be \"at any point on the command line where a branch name is\naccepted, if a '-' could not possibly mean any other thing is wanted\n(e.g. doing something on the standard input), it should stand as the\nname of the previous branch\".\n\nI agree with everything you said in your review.  The patch is going\nin the right direction, but it needs a bit more work.\n\nThanks.\n"},{"id":"225348","messageId":"20130817.001737.1890599379619259228.xxx@gmail.com","threadId":"34598","inReplyTo":"7vmwowm7bc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] cherry-pick: allow \"-\" as abbreviation of '@{-1}'","fromName":"Hiroshige Umino","fromEmail":"hiroshige88@gmail.com","sentAt":"2013-08-16T15:17:37Z","receivedAt":"2013-08-16T15:17:37Z","isPatch":true,"sender":{"key":"hiroshige88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/18807?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Thomas Rast <trast@inf.ethz.ch> writes:\n> \n>> Hiroshige Umino <hiroshige88@gmail.com> writes:\n>>\n>>> As \"git cherry-pick -\" or \"git merge -\" is convenient to\n>>> switch back to or merge the previous branch,\n>>> \"git cherry-pick -\" is abbreviation of \"git cherry-pick @{-1}\"\n>>> to pick up a commit from the previous branch conveniently.\n>>\n>> The first line is confusing.  Did you mean to invoke the existing 'git\n>> *checkout* -' and 'git merge -' functionality as a reason why 'git\n>> cherry-pick -' should exist?\n> \n> I think that is what was meant.  Just like \"-\" abbreviation is handy\n> for users of \"checkout\" and \"merge\", \"cherry-pick\" might.\n\nYes I meant so and it would be useful at least for me.\nI don't know the usage of cherry-pick (pick up a commit from the previous\nbranch) is comon or not but it may be also good for consistency.\n\n\nThomas Rast <trast@inf.ethz.ch> wrote:\n> What other commands could reasonably use the '-' shorthand?\n\nI've wanted '-' shorthand only for commit, merge and cherry-pick\nbut 'git diff -' may make sense.\nWhat do you think of this and other candidates?\n\n> Do you have to use a new test file for this?\nNot have to so I'm moving the tests into t/t3500-cherry.sh.\n\n> [...]\n>> +test_expect_success 'setup' '\n>> + echo hello >world &&\n>> + git add world &&\n> (*)\n>> + git commit -m initial &&\n>> + git branch other &&\n>> + echo \"hello again\" >>world &&\n>> + git add world &&\n> (*)\n>> + git commit -m second\n>> +'\n> \n> Our style is to indent the test snippets with a hard tab, not a single\n> (or eight, for that matter) space.\n> \n> [...]\n>> +test_expect_success 'cherry-pick the commit in the previous branch' '\n>> + prev=$(git rev-parse HEAD) &&\n>> + git checkout other &&\n> (*)\n>> + git cherry-pick - &&\n>> + test \"z$(git rev-parse HEAD)\" = \"z$prev\"\n>> +'\n> \n> If you insert 'test_tick' in the places marked with (*), the test fails.\n> \n> The tests run under a fake clock to ensure that everything, including\n> the SHA1s produced, are deterministic.  You never advance the clock, so\n> all commits generated in this script share the same timestamp.\n> \n> This means that the cherry-pick of 'second' has the same SHA1 as the\n> original: its tree, parents, author, timestamp etc. all agree.  If you\n> advance the clock at the last (*), this fails.  You should find some\n> other way of checking what was picked, e.g., by looking at the file\n> contents.\n> \n> That said, please use test_commit in the 'setup' snippet instead of\n> manually rolling the commits.  It will lead to shorter code, and it\n> handles test_tick for you.  It is documented in t/README and in a\n> comment in t/test-lib-functions.sh.  (You still need test_tick\n> immediately before the cherry-pick!)\n\nI overlooked t/README, thank you for kindly guiding testing!\n\n--\nHiroshige UMINO @yaotti\n"}]}