{"thread":{"id":"35106","subject":"[PATCH] status: show commit sha1 in \"You are currently cherry-picking\" message","startedAt":"2013-10-11T15:58:37Z","lastAt":"2013-10-15T13:35:16Z","messageCount":5,"participants":["Ralf Thielow","Matthieu Moy","Jonathan Nieder","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"228741","messageId":"1381507117-11519-1-git-send-email-ralf.thielow@gmail.com","threadId":"35106","inReplyTo":null,"subject":"[PATCH] status: show commit sha1 in \"You are currently cherry-picking\" message","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@gmail.com","sentAt":"2013-10-11T15:58:37Z","receivedAt":"2013-10-11T15:58:37Z","isPatch":true,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>\n---\n\nEspecially helpful when cherry-picking multiple commits.\n\n t/t7512-status-help.sh | 10 ++++++----\n wt-status.c            |  7 +++++--\n wt-status.h            |  1 +\n 3 files changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\nindex 0688d58..0a65db1 100755\n--- a/t/t7512-status-help.sh\n+++ b/t/t7512-status-help.sh\n@@ -626,9 +626,10 @@ test_expect_success 'prepare for cherry-pick conflicts' '\n test_expect_success 'status when cherry-picking before resolving conflicts' '\n \ttest_when_finished \"git cherry-pick --abort\" &&\n \ttest_must_fail git cherry-pick cherry_branch_second &&\n-\tcat >expected <<\\EOF &&\n+\tTO_CHERRY_PICK=$(git rev-parse --short CHERRY_PICK_HEAD) &&\n+\tcat >expected <<EOF\n On branch cherry_branch\n-You are currently cherry-picking.\n+You are currently cherry-picking commit $TO_CHERRY_PICK.\n   (fix conflicts and run \"git cherry-pick --continue\")\n   (use \"git cherry-pick --abort\" to cancel the cherry-pick operation)\n \n@@ -648,11 +649,12 @@ test_expect_success 'status when cherry-picking after resolving conflicts' '\n \tgit reset --hard cherry_branch &&\n \ttest_when_finished \"git cherry-pick --abort\" &&\n \ttest_must_fail git cherry-pick cherry_branch_second &&\n+\tTO_CHERRY_PICK=$(git rev-parse --short CHERRY_PICK_HEAD) &&\n \techo end >main.txt &&\n \tgit add main.txt &&\n-\tcat >expected <<\\EOF &&\n+\tcat >expected <<EOF\n On branch cherry_branch\n-You are currently cherry-picking.\n+You are currently cherry-picking commit $TO_CHERRY_PICK.\n   (all conflicts fixed: run \"git cherry-pick --continue\")\n   (use \"git cherry-pick --abort\" to cancel the cherry-pick operation)\n \ndiff --git a/wt-status.c b/wt-status.c\nindex cbdce72..b4e44ba 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -996,7 +996,8 @@ static void show_cherry_pick_in_progress(struct wt_status *s,\n \t\t\t\t\tstruct wt_status_state *state,\n \t\t\t\t\tconst char *color)\n {\n-\tstatus_printf_ln(s, color, _(\"You are currently cherry-picking.\"));\n+\tstatus_printf_ln(s, color, _(\"You are currently cherry-picking commit %s.\"),\n+\t\t\tfind_unique_abbrev(state->cherry_pick_head_sha1, DEFAULT_ABBREV));\n \tif (s->hints) {\n \t\tif (has_unmerged(s))\n \t\t\tstatus_printf_ln(s, color,\n@@ -1169,8 +1170,10 @@ void wt_status_get_state(struct wt_status_state *state,\n \t\t\tstate->rebase_in_progress = 1;\n \t\tstate->branch = read_and_strip_branch(\"rebase-merge/head-name\");\n \t\tstate->onto = read_and_strip_branch(\"rebase-merge/onto\");\n-\t} else if (!stat(git_path(\"CHERRY_PICK_HEAD\"), &st)) {\n+\t} else if (!stat(git_path(\"CHERRY_PICK_HEAD\"), &st) &&\n+\t\t\t!get_sha1(\"CHERRY_PICK_HEAD\", sha1)) {\n \t\tstate->cherry_pick_in_progress = 1;\n+\t\thashcpy(state->cherry_pick_head_sha1, sha1);\n \t}\n \tif (!stat(git_path(\"BISECT_LOG\"), &st)) {\n \t\tstate->bisect_in_progress = 1;\ndiff --git a/wt-status.h b/wt-status.h\nindex 9341c56..6c29e6f 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -88,6 +88,7 @@ struct wt_status_state {\n \tchar *detached_from;\n \tunsigned char detached_sha1[20];\n \tunsigned char revert_head_sha1[20];\n+\tunsigned char cherry_pick_head_sha1[20];\n };\n \n void wt_status_prepare(struct wt_status *s);\n-- \n1.8.4.652.g0d6e0ce\n"},{"id":"228742","messageId":"vpqtxgng5hb.fsf@anie.imag.fr","threadId":"35106","inReplyTo":"1381507117-11519-1-git-send-email-ralf.thielow@gmail.com","subject":"Re: [PATCH] status: show commit sha1 in \"You are currently cherry-picking\" message","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-10-11T16:03:12Z","receivedAt":"2013-10-11T16:03:12Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Ralf Thielow <ralf.thielow@gmail.com> writes:\n\n> Especially helpful when cherry-picking multiple commits.\n\nI think this would deserve to be in the commit message (but don't\nconsider that blocking).\n\nOther than that, looks good to me.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"228744","messageId":"20131011174210.GS9464@google.com","threadId":"35106","inReplyTo":"1381507117-11519-1-git-send-email-ralf.thielow@gmail.com","subject":"Re: [PATCH] status: show commit sha1 in \"You are currently cherry-picking\" message","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-10-11T17:42:10Z","receivedAt":"2013-10-11T17:42:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ralf Thielow wrote:\n\n> Especially helpful when cherry-picking multiple commits.\n\nNeat, thanks.\n\n[...]\n> --- a/t/t7512-status-help.sh\n> +++ b/t/t7512-status-help.sh\n> @@ -626,9 +626,10 @@ test_expect_success 'prepare for cherry-pick conflicts' '\n>  test_expect_success 'status when cherry-picking before resolving conflicts' '\n>  \ttest_when_finished \"git cherry-pick --abort\" &&\n>  \ttest_must_fail git cherry-pick cherry_branch_second &&\n> +\tTO_CHERRY_PICK=$(git rev-parse --short CHERRY_PICK_HEAD) &&\n> -\tcat >expected <<\\EOF &&\n> +\tcat >expected <<EOF\n\nDid you mean to drop the '&&'?\n\n[...]\n> @@ -648,11 +649,12 @@ test_expect_success 'status when cherry-picking after resolving conflicts' '\n>  \tgit reset --hard cherry_branch &&\n>  \ttest_when_finished \"git cherry-pick --abort\" &&\n>  \ttest_must_fail git cherry-pick cherry_branch_second &&\n> +\tTO_CHERRY_PICK=$(git rev-parse --short CHERRY_PICK_HEAD) &&\n>  \techo end >main.txt &&\n>  \tgit add main.txt &&\n> -\tcat >expected <<\\EOF &&\n> +\tcat >expected <<EOF\n\nLikewise.\n\n[...]\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -996,7 +996,8 @@ static void show_cherry_pick_in_progress(struct wt_status *s,\n>  \t\t\t\t\tstruct wt_status_state *state,\n>  \t\t\t\t\tconst char *color)\n>  {\n> -\tstatus_printf_ln(s, color, _(\"You are currently cherry-picking.\"));\n> +\tstatus_printf_ln(s, color, _(\"You are currently cherry-picking commit %s.\"),\n> +\t\t\tfind_unique_abbrev(state->cherry_pick_head_sha1, DEFAULT_ABBREV));\n\nThis function is only called when ->cherry_pick_in_progress is true, so\nwe know cherry_pick_head_sha1 is initialized.  Good.\n\nI would be tempted to check anyway, so that if we ever regress in this,\nthe cause will be clear and users know to report a bug:\n\n\tif (is_null_sha1(state->cherry_pick_head_sha1))\n\t\tdie(\"BUG: cherry-pick in progress but no valid CHERRY_PICK_HEAD?\");\n\tstatus_printf_ln(s, color, _(\"You are ...\n\nI dunno.\n\nApplied with the && fixes mentioned above on top of the following.\n\n-- >8 --\nSubject: status test: add missing && to <<EOF blocks\n\nWhen a test forgets to include && after each command, it is possible\nfor an early command to succeed but the test to fail, which can hide\nbugs.\n\nChecked using the following patch to the test harness:\n\n\t--- a/t/test-lib.sh\n\t+++ b/t/test-lib.sh\n\t@@ -425,7 +425,17 @@ test_eval_ () {\n\t\teval </dev/null >&3 2>&4 \"$*\"\n\t }\n\n\t+check_command_chaining_ () {\n\t+\teval >&3 2>&4 \"(exit 189) && $*\"\n\t+\teval_chain_ret=$?\n\t+\tif test \"$eval_chain_ret\" != 189\n\t+\tthen\n\t+\t\terror 'bug in test script: missing \"&&\" in test commands'\n\t+\tfi\n\t+}\n\t+\n\t test_run_ () {\n\t+\tcheck_command_chaining_ \"$1\"\n\t\ttest_cleanup=:\n\t\texpecting_failure=$2\n\t\tsetup_malloc_check\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n t/t7512-status-help.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\nindex 0688d58..9905d43 100755\n--- a/t/t7512-status-help.sh\n+++ b/t/t7512-status-help.sh\n@@ -669,7 +669,7 @@ EOF\n test_expect_success 'status showing detached at and from a tag' '\n \ttest_commit atag tagging &&\n \tgit checkout atag &&\n-\tcat >expected <<\\EOF\n+\tcat >expected <<\\EOF &&\n HEAD detached at atag\n nothing to commit (use -u to show untracked files)\n EOF\n@@ -677,7 +677,7 @@ EOF\n \ttest_i18ncmp expected actual &&\n \n \tgit reset --hard HEAD^ &&\n-\tcat >expected <<\\EOF\n+\tcat >expected <<\\EOF &&\n HEAD detached from atag\n nothing to commit (use -u to show untracked files)\n EOF\n@@ -695,7 +695,7 @@ test_expect_success 'status while reverting commit (conflicts)' '\n \ttest_commit new to-revert.txt &&\n \tTO_REVERT=$(git rev-parse --short HEAD^) &&\n \ttest_must_fail git revert $TO_REVERT &&\n-\tcat >expected <<EOF\n+\tcat >expected <<EOF &&\n On branch master\n You are currently reverting commit $TO_REVERT.\n   (fix conflicts and run \"git revert --continue\")\n@@ -716,7 +716,7 @@ EOF\n test_expect_success 'status while reverting commit (conflicts resolved)' '\n \techo reverted >to-revert.txt &&\n \tgit add to-revert.txt &&\n-\tcat >expected <<EOF\n+\tcat >expected <<EOF &&\n On branch master\n You are currently reverting commit $TO_REVERT.\n   (all conflicts fixed: run \"git revert --continue\")\n@@ -735,7 +735,7 @@ EOF\n \n test_expect_success 'status after reverting commit' '\n \tgit revert --continue &&\n-\tcat >expected <<\\EOF\n+\tcat >expected <<\\EOF &&\n On branch master\n nothing to commit (use -u to show untracked files)\n EOF\n-- \n1.8.4-50-g437ce60\n"},{"id":"228746","messageId":"CAN0XMOKXYBnuEceAwzzmi0PUy6BgWLOxsBjLoEzHpNynR5+b_A@mail.gmail.com","threadId":"35106","inReplyTo":"20131011174210.GS9464@google.com","subject":"Re: [PATCH] status: show commit sha1 in \"You are currently cherry-picking\" message","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@gmail.com","sentAt":"2013-10-11T18:14:49Z","receivedAt":"2013-10-11T18:14:49Z","isPatch":true,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"On Fri, Oct 11, 2013 at 7:42 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Ralf Thielow wrote:\n>\n>> Especially helpful when cherry-picking multiple commits.\n>\n> Neat, thanks.\n>\n> [...]\n>> --- a/t/t7512-status-help.sh\n>> +++ b/t/t7512-status-help.sh\n>> @@ -626,9 +626,10 @@ test_expect_success 'prepare for cherry-pick conflicts' '\n>>  test_expect_success 'status when cherry-picking before resolving conflicts' '\n>>       test_when_finished \"git cherry-pick --abort\" &&\n>>       test_must_fail git cherry-pick cherry_branch_second &&\n>> +     TO_CHERRY_PICK=$(git rev-parse --short CHERRY_PICK_HEAD) &&\n>> -     cat >expected <<\\EOF &&\n>> +     cat >expected <<EOF\n>\n> Did you mean to drop the '&&'?\n>\n\nNo. The important thing was actually the \"\\\" character. Sry\n\n> [...]\n>> @@ -648,11 +649,12 @@ test_expect_success 'status when cherry-picking after resolving conflicts' '\n>>       git reset --hard cherry_branch &&\n>>       test_when_finished \"git cherry-pick --abort\" &&\n>>       test_must_fail git cherry-pick cherry_branch_second &&\n>> +     TO_CHERRY_PICK=$(git rev-parse --short CHERRY_PICK_HEAD) &&\n>>       echo end >main.txt &&\n>>       git add main.txt &&\n>> -     cat >expected <<\\EOF &&\n>> +     cat >expected <<EOF\n>\n> Likewise.\n>\n> [...]\n>> --- a/wt-status.c\n>> +++ b/wt-status.c\n>> @@ -996,7 +996,8 @@ static void show_cherry_pick_in_progress(struct wt_status *s,\n>>                                       struct wt_status_state *state,\n>>                                       const char *color)\n>>  {\n>> -     status_printf_ln(s, color, _(\"You are currently cherry-picking.\"));\n>> +     status_printf_ln(s, color, _(\"You are currently cherry-picking commit %s.\"),\n>> +                     find_unique_abbrev(state->cherry_pick_head_sha1, DEFAULT_ABBREV));\n>\n> This function is only called when ->cherry_pick_in_progress is true, so\n> we know cherry_pick_head_sha1 is initialized.  Good.\n>\n> I would be tempted to check anyway, so that if we ever regress in this,\n> the cause will be clear and users know to report a bug:\n>\n>         if (is_null_sha1(state->cherry_pick_head_sha1))\n>                 die(\"BUG: cherry-pick in progress but no valid CHERRY_PICK_HEAD?\");\n>         status_printf_ln(s, color, _(\"You are ...\n>\n> I dunno.\n>\n> Applied with the && fixes mentioned above on top of the following.\n>\n\nThanks\n\n> -- >8 --\n> Subject: status test: add missing && to <<EOF blocks\n>\n> When a test forgets to include && after each command, it is possible\n> for an early command to succeed but the test to fail, which can hide\n> bugs.\n>\n> Checked using the following patch to the test harness:\n>\n>         --- a/t/test-lib.sh\n>         +++ b/t/test-lib.sh\n>         @@ -425,7 +425,17 @@ test_eval_ () {\n>                 eval </dev/null >&3 2>&4 \"$*\"\n>          }\n>\n>         +check_command_chaining_ () {\n>         +       eval >&3 2>&4 \"(exit 189) && $*\"\n>         +       eval_chain_ret=$?\n>         +       if test \"$eval_chain_ret\" != 189\n>         +       then\n>         +               error 'bug in test script: missing \"&&\" in test commands'\n>         +       fi\n>         +}\n>         +\n>          test_run_ () {\n>         +       check_command_chaining_ \"$1\"\n>                 test_cleanup=:\n>                 expecting_failure=$2\n>                 setup_malloc_check\n>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n>  t/t7512-status-help.sh | 10 +++++-----\n>  1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\n> index 0688d58..9905d43 100755\n> --- a/t/t7512-status-help.sh\n> +++ b/t/t7512-status-help.sh\n> @@ -669,7 +669,7 @@ EOF\n>  test_expect_success 'status showing detached at and from a tag' '\n>         test_commit atag tagging &&\n>         git checkout atag &&\n> -       cat >expected <<\\EOF\n> +       cat >expected <<\\EOF &&\n>  HEAD detached at atag\n>  nothing to commit (use -u to show untracked files)\n>  EOF\n> @@ -677,7 +677,7 @@ EOF\n>         test_i18ncmp expected actual &&\n>\n>         git reset --hard HEAD^ &&\n> -       cat >expected <<\\EOF\n> +       cat >expected <<\\EOF &&\n>  HEAD detached from atag\n>  nothing to commit (use -u to show untracked files)\n>  EOF\n> @@ -695,7 +695,7 @@ test_expect_success 'status while reverting commit (conflicts)' '\n>         test_commit new to-revert.txt &&\n>         TO_REVERT=$(git rev-parse --short HEAD^) &&\n>         test_must_fail git revert $TO_REVERT &&\n> -       cat >expected <<EOF\n> +       cat >expected <<EOF &&\n>  On branch master\n>  You are currently reverting commit $TO_REVERT.\n>    (fix conflicts and run \"git revert --continue\")\n> @@ -716,7 +716,7 @@ EOF\n>  test_expect_success 'status while reverting commit (conflicts resolved)' '\n>         echo reverted >to-revert.txt &&\n>         git add to-revert.txt &&\n> -       cat >expected <<EOF\n> +       cat >expected <<EOF &&\n>  On branch master\n>  You are currently reverting commit $TO_REVERT.\n>    (all conflicts fixed: run \"git revert --continue\")\n> @@ -735,7 +735,7 @@ EOF\n>\n>  test_expect_success 'status after reverting commit' '\n>         git revert --continue &&\n> -       cat >expected <<\\EOF\n> +       cat >expected <<\\EOF &&\n>  On branch master\n>  nothing to commit (use -u to show untracked files)\n>  EOF\n> --\n> 1.8.4-50-g437ce60\n>\n"},{"id":"229008","messageId":"20131015133516.GC26156@goldbirke","threadId":"35106","inReplyTo":"20131011174210.GS9464@google.com","subject":"on broken command chains in tests (was: Re: [PATCH] status: show commit sha1 in \"You are currently)","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2013-10-15T13:35:16Z","receivedAt":"2013-10-15T13:35:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Hi,\n\nOn Fri, Oct 11, 2013 at 10:42:10AM -0700, Jonathan Nieder wrote:\n> -- >8 --\n> Subject: status test: add missing && to <<EOF blocks\n> \n> When a test forgets to include && after each command, it is possible\n> for an early command to succeed but the test to fail, which can hide\n> bugs.\n\nSurely you meant \"succeed\" and \"fail\" the other way around :)\n\n> Checked using the following patch to the test harness:\n> \n> \t--- a/t/test-lib.sh\n> \t+++ b/t/test-lib.sh\n> \t@@ -425,7 +425,17 @@ test_eval_ () {\n> \t\teval </dev/null >&3 2>&4 \"$*\"\n> \t }\n> \n> \t+check_command_chaining_ () {\n> \t+\teval >&3 2>&4 \"(exit 189) && $*\"\n> \t+\teval_chain_ret=$?\n> \t+\tif test \"$eval_chain_ret\" != 189\n> \t+\tthen\n> \t+\t\terror 'bug in test script: missing \"&&\" in test commands'\n> \t+\tfi\n> \t+}\n> \t+\n> \t test_run_ () {\n> \t+\tcheck_command_chaining_ \"$1\"\n> \t\ttest_cleanup=:\n> \t\texpecting_failure=$2\n> \t\tsetup_malloc_check\n\nClever.\n\nIf I do a\n\n  -               error 'bug in test script: missing \"&&\" in test commands'\n  +               say_color error 'error: bug in test script: missing \"&&\" in test commands'\n\nto avoid erroring out and skipping the rest of the test script on the\nfirst broken command chain, then we can see that we have a lot of\nbroken command chains in the test suite:\n\n  $ for t in t[0-9][0-9][0-9][0-9]*.sh ; do ./$t ; done |grep -c '^error:.*missing \"&&\" in test commands$'\n  345\n\nAfter a cursory look most of them seem to be the simple \"missing &&\"\ntype, but there are some funny ones, too.\n\n\nGábor\n"}]}