{"thread":{"id":"39696","subject":"[PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","startedAt":"2015-06-22T21:42:47Z","lastAt":"2015-06-23T20:47:00Z","messageCount":16,"participants":["Galan Rémi","Eric Sunshine","Remi Galan Alfonso","Matthieu Moy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"264593","messageId":"1435009369-11496-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39696","inReplyTo":null,"subject":"[PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Galan Rémi","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-22T21:42:47Z","receivedAt":"2015-06-22T21:42:47Z","isPatch":false,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Instead of removing a line to remove the commit, you can use the\ncommand \"drop\" (just like \"pick\" or \"edit\"). It has the same effect as\ndeleting the line (removing the commit) except that you keep a visual\ntrace of your actions, allowing a better control and reducing the\npossibility of removing a commit by mistake.\n\nSigned-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>\n---\n Documentation/git-rebase.txt  |  3 +++\n git-rebase--interactive.sh    |  3 ++-\n t/lib-rebase.sh               |  4 ++--\n t/t3404-rebase-interactive.sh | 16 ++++++++++++++++\n 4 files changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 1d01baa..34bd070 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -514,6 +514,9 @@ rebasing.\n If you just want to edit the commit message for a commit, replace the\n command \"pick\" with the command \"reword\".\n \n+To drop a commit, replace the command \"pick\" with \"drop\", or just\n+delete the matching line.\n+\n If you want to fold two or more commits into one, replace the command\n \"pick\" for the second and subsequent commits with \"squash\" or \"fixup\".\n If the commits had different authors, the folded commit will be\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex dc3133f..72abf90 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -152,6 +152,7 @@ Commands:\n  s, squash = use commit, but meld into previous commit\n  f, fixup = like \"squash\", but discard this commit's log message\n  x, exec = run command (the rest of the line) using shell\n+ d, drop = remove commit\n \n These lines can be re-ordered; they are executed from top to bottom.\n \n@@ -505,7 +506,7 @@ do_next () {\n \trm -f \"$msg\" \"$author_script\" \"$amend\" \"$state_dir\"/stopped-sha || exit\n \tread -r command sha1 rest < \"$todo\"\n \tcase \"$command\" in\n-\t\"$comment_char\"*|''|noop)\n+\t\"$comment_char\"*|''|noop|drop|d)\n \t\tmark_action_done\n \t\t;;\n \tpick|p)\ndiff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\nindex 6bd2522..fdbc900 100644\n--- a/t/lib-rebase.sh\n+++ b/t/lib-rebase.sh\n@@ -14,7 +14,7 @@\n #       specified line.\n #\n #   \"<cmd> <lineno>\" -- add a line with the specified command\n-#       (\"squash\", \"fixup\", \"edit\", or \"reword\") and the SHA1 taken\n+#       (\"squash\", \"fixup\", \"edit\", \"reword\" or \"drop\") and the SHA1 taken\n #       from the specified line.\n #\n #   \"exec_cmd_with_args\" -- add an \"exec cmd with args\" line.\n@@ -46,7 +46,7 @@ set_fake_editor () {\n \taction=pick\n \tfor line in $FAKE_LINES; do\n \t\tcase $line in\n-\t\tsquash|fixup|edit|reword)\n+\t\tsquash|fixup|edit|reword|drop)\n \t\t\taction=\"$line\";;\n \t\texec*)\n \t\t\techo \"$line\" | sed 's/_/ /g' >> \"$1\";;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ac429a0..ecd277c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1102,4 +1102,20 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '\n \ttest $(git cat-file commit HEAD | sed -ne \\$p) = I\n '\n \n+test_rebase_end () {\n+\ttest_when_finished \"git checkout master &&\n+\tgit branch -D $1 &&\n+\ttest_might_fail git rebase --abort\" &&\n+\tgit checkout -b $1 master\n+}\n+\n+test_expect_success 'drop' '\n+\ttest_rebase_end dropTest &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"1 drop 2 3 drop 4 5\" git rebase -i --root &&\n+\ttest E = $(git cat-file commit HEAD | sed -ne \\$p) &&\n+\ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p) &&\n+\ttest A = $(git cat-file commit HEAD^^ | sed -ne \\$p)\n+'\n+\n test_done\n-- \n2.4.3.371.g8992f2a\n"},{"id":"264594","messageId":"1435009369-11496-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39696","inReplyTo":"1435009369-11496-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"[PATCHv6 2/3] git rebase -i: warn about removed commits","fromName":"Galan Rémi","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-22T21:42:48Z","receivedAt":"2015-06-22T21:42:48Z","isPatch":false,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Check if commits were removed (i.e. a line was deleted) and print\nwarnings or stop git rebase depending on the value of the\nconfiguration variable rebase.missingCommitsCheck.\n\nThis patch gives the user the possibility to avoid silent loss of\ninformation (losing a commit through deleting the line in this case)\nif he wants.\n\nAdd the configuration variable rebase.missingCommitsCheck.\n    - When unset or set to \"ignore\", no checking is done.\n    - When set to \"warn\", the commits are checked, warnings are\n      displayed but git rebase still proceeds.\n    - When set to \"error\", the commits are checked, warnings are\n      displayed and the rebase is stopped.\n      (The user can then use 'git rebase --edit-todo' and\n      'git rebase --continue', or 'git rebase --abort')\n\nrebase.missingCommitsCheck defaults to \"ignore\".\n\nSigned-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>\n---\n On the contrary of v5, the SHA-1 that are read are the short one.\n (the todo-list is read directly after the user closes his editor\n and not after expansion of ids)\n\n Documentation/config.txt      |  11 +++++\n Documentation/git-rebase.txt  |   6 +++\n git-rebase--interactive.sh    | 104 ++++++++++++++++++++++++++++++++++++++++--\n t/t3404-rebase-interactive.sh |  66 +++++++++++++++++++++++++++\n 4 files changed, 184 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4d21ce1..25b2a04 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2160,6 +2160,17 @@ rebase.autoStash::\n \tsuccessful rebase might result in non-trivial conflicts.\n \tDefaults to false.\n \n+rebase.missingCommitsCheck::\n+\tIf set to \"warn\", git rebase -i will print a warning if some\n+\tcommits are removed (e.g. a line was deleted), however the\n+\trebase will still proceed. If set to \"error\", it will print\n+\tthe previous warning and stop the rebase, 'git rebase\n+\t--edit-todo' can then be used to correct the error. If set to\n+\t\"ignore\", no checking is done.\n+\tTo drop a commit without warning or error, use the `drop`\n+\tcommand in the todo-list.\n+\tDefaults to \"ignore\".\n+\n receive.advertiseAtomic::\n \tBy default, git-receive-pack will advertise the atomic push\n \tcapability to its clients. If you don't want to this capability\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 34bd070..2ca3b8d 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -213,6 +213,12 @@ rebase.autoSquash::\n rebase.autoStash::\n \tIf set to true enable '--autostash' option by default.\n \n+rebase.missingCommitsCheck::\n+\tIf set to \"warn\", print warnings about removed commits in\n+\tinteractive mode. If set to \"error\", print the warnings and\n+\tstop the rebase. If set to \"ignore\", no checking is\n+\tdone. \"ignore\" by default.\n+\n OPTIONS\n -------\n --onto <newbase>::\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 72abf90..66daacf 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -834,6 +834,104 @@ add_exec_commands () {\n \tmv \"$1.new\" \"$1\"\n }\n \n+# Print the list of the SHA-1 of the commits\n+# from stdin to stdout\n+todo_list_to_sha_list () {\n+\tgit stripspace --strip-comments |\n+\twhile read -r command sha1 rest\n+\tdo\n+\t\tcase $command in\n+\t\t\"$comment_char\"*|''|noop|x|\"exec\")\n+\t\t\t;;\n+\t\t*)\n+\t\t\tlong_sha=$(git rev-list --no-walk \"$sha1\" 2>/dev/null)\n+\t\t\tprintf \"%s\\n\" \"$long_sha\"\n+\t\t\t;;\n+\t\tesac\n+\tdone\n+}\n+\n+# Use warn for each line in stdin\n+warn_lines () {\n+\twhile read -r line\n+\tdo\n+\t\twarn \" - $line\"\n+\tdone\n+}\n+\n+# Switch to the branch in $into and notify it in the reflog\n+checkout_onto () {\n+\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n+\toutput git checkout $onto || die_abort \"could not detach HEAD\"\n+\tgit update-ref ORIG_HEAD $orig_head\n+}\n+\n+# Check if the user dropped some commits by mistake\n+# Behaviour determined by rebase.missingCommitsCheck.\n+check_todo_list () {\n+\traise_error=f\n+\n+\tcheck_level=$(git config --get rebase.missingCommitsCheck)\n+\tcheck_level=${check_level:-ignore}\n+\t# Don't be case sensitive\n+\tcheck_level=$(printf '%s' \"$check_level\" | tr 'A-Z' 'a-z')\n+\n+\tcase \"$check_level\" in\n+\twarn|error)\n+\t\t# Get the SHA-1 of the commits\n+\t\ttodo_list_to_sha_list <\"$todo\".backup >\"$todo\".oldsha1\n+\t\ttodo_list_to_sha_list <\"$todo\" >\"$todo\".newsha1\n+\n+\t\t# Sort the SHA-1 and compare them\n+\t\tsort -u \"$todo\".oldsha1 >\"$todo\".oldsha1+\n+\t\tmv \"$todo\".oldsha1+ \"$todo\".oldsha1\n+\t\tsort -u \"$todo\".newsha1 >\"$todo\".newsha1+\n+\t\tmv \"$todo\".newsha1+ \"$todo\".newsha1\n+\t\tcomm -2 -3 \"$todo\".oldsha1 \"$todo\".newsha1 >\"$todo\".miss\n+\n+\t\t# Warn about missing commits\n+\t\tif test -s \"$todo\".miss\n+\t\tthen\n+\t\t\ttest \"$check_level\" = error && raise_error=t\n+\n+\t\t\twarn \"Warning: some commits may have been dropped\" \\\n+\t\t\t\t\"accidentally.\"\n+\t\t\twarn \"Dropped commits (newer to older):\"\n+\n+\t\t\t# Make the list user-friendly and display\n+\t\t\topt=\"--no-walk=sorted --format=oneline --abbrev-commit --stdin\"\n+\t\t\tgit rev-list $opt <\"$todo\".miss | warn_lines\n+\n+\t\t\twarn \"To avoid this message, use \\\"drop\\\" to\" \\\n+\t\t\t\t\"explicitly remove a commit.\"\n+\t\t\twarn\n+\t\t\twarn \"Use 'git --config rebase.missingCommitsCheck' to change\" \\\n+\t\t\t\t\"the level of warnings.\"\n+\t\t\twarn \"The possible behaviours are: ignore, warn, error.\"\n+\t\t\twarn\n+\t\tfi\n+\t\t;;\n+\tignore)\n+\t\t;;\n+\t*)\n+\t\twarn \"Unrecognized setting $check_level for option\" \\\n+\t\t\t\"rebase.missingCommitsCheck. Ignoring.\"\n+\t\t;;\n+\tesac\n+\n+\tif test $raise_error = t\n+\tthen\n+\t\t# Checkout before the first commit of the\n+\t\t# rebase: this way git rebase --continue\n+\t\t# will work correctly as it expects HEAD to be\n+\t\t# placed before the commit of the next action\n+\t\tcheckout_onto\n+\n+\t\twarn \"You can fix this with 'git rebase --edit-todo'.\"\n+\t\tdie \"Or you can abort the rebase with 'git rebase --abort'.\"\n+\tfi\n+}\n+\n # The whole contents of this file is run by dot-sourcing it from\n # inside a shell function.  It used to be that \"return\"s we see\n # below were not inside any function, and expected to return\n@@ -1077,13 +1175,13 @@ git_sequence_editor \"$todo\" ||\n has_action \"$todo\" ||\n \treturn 2\n \n+check_todo_list\n+\n expand_todo_ids\n \n test -d \"$rewritten\" || test -n \"$force_rebase\" || skip_unnecessary_picks\n \n-GIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n-output git checkout $onto || die_abort \"could not detach HEAD\"\n-git update-ref ORIG_HEAD $orig_head\n+checkout_onto\n do_rest\n \n }\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ecd277c..a92ae19 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1118,4 +1118,70 @@ test_expect_success 'drop' '\n \ttest A = $(git cat-file commit HEAD^^ | sed -ne \\$p)\n '\n \n+cat >expect <<EOF\n+Successfully rebased and updated refs/heads/tmp2.\n+EOF\n+\n+test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '\n+\ttest_config rebase.missingCommitsCheck ignore &&\n+\ttest_rebase_end tmp2 &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"1 2 3 4\" \\\n+\t\tgit rebase -i --root 2>actual &&\n+\ttest D = $(git cat-file commit HEAD | sed -ne \\$p) &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <<EOF\n+Warning: some commits may have been dropped accidentally.\n+Dropped commits (newer to older):\n+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n+To avoid this message, use \"drop\" to explicitly remove a commit.\n+\n+Use 'git --config rebase.missingCommitsCheck' to change the level of warnings.\n+The possible behaviours are: ignore, warn, error.\n+\n+Successfully rebased and updated refs/heads/tmp2.\n+EOF\n+\n+test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '\n+\ttest_config rebase.missingCommitsCheck warn &&\n+\ttest_rebase_end tmp2 &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"1 2 3 4\" \\\n+\t\tgit rebase -i --root 2>actual &&\n+\ttest_cmp expect actual &&\n+\ttest D = $(git cat-file commit HEAD | sed -ne \\$p)\n+'\n+\n+cat >expect <<EOF\n+Warning: some commits may have been dropped accidentally.\n+Dropped commits (newer to older):\n+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master)\n+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)\n+To avoid this message, use \"drop\" to explicitly remove a commit.\n+\n+Use 'git --config rebase.missingCommitsCheck' to change the level of warnings.\n+The possible behaviours are: ignore, warn, error.\n+\n+You can fix this with 'git rebase --edit-todo'.\n+Or you can abort the rebase with 'git rebase --abort'.\n+EOF\n+\n+test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '\n+\ttest_config rebase.missingCommitsCheck error &&\n+\ttest_rebase_end tmp2 &&\n+\tset_fake_editor &&\n+\ttest_must_fail env FAKE_LINES=\"1 2 4\" \\\n+\t\tgit rebase -i --root 2>actual &&\n+\ttest_cmp expect actual &&\n+\tcp .git/rebase-merge/git-rebase-todo.backup \\\n+\t\t.git/rebase-merge/git-rebase-todo &&\n+\tFAKE_LINES=\"1 2 drop 3 4 drop 5\" \\\n+\t\tgit rebase --edit-todo &&\n+\tgit rebase --continue &&\n+\ttest D = $(git cat-file commit HEAD | sed -ne \\$p) &&\n+\ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n+'\n+\n test_done\n-- \n2.4.3.371.g8992f2a\n"},{"id":"264595","messageId":"1435009369-11496-3-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39696","inReplyTo":"1435009369-11496-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"[PATCHv6 3/3] git rebase -i: add static check for commands and SHA-1","fromName":"Galan Rémi","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-22T21:42:49Z","receivedAt":"2015-06-22T21:42:49Z","isPatch":false,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Check before the start of the rebasing if the commands exists, and for\nthe commands expecting a SHA-1, check if the SHA-1 is present and\ncorresponds to a commit. In case of error, print the error, stop git\nrebase and prompt the user to fix with 'git rebase --edit-todo' or to\nabort.\n\nThis allows to avoid doing half of a rebase before finding an error\nand giving back what's left of the todo list to the user and prompt\nhim to fix when it might be too late for him to do so (he might have\nto abort and restart the rebase).\n\nSigned-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>\n---\n I used:\n   read -r command sha1 rest <<EOF\n   $line\n   EOF\n because\n   printf '%s' \"$line\" | read -r command sha1 rest\n doesn't work (the 3 variables have no value as a result).\n There might be a better way to do this, but I don't have it right now.\n\n git-rebase--interactive.sh    | 70 +++++++++++++++++++++++++++++++++++++++++++\n t/lib-rebase.sh               |  5 ++++\n t/t3404-rebase-interactive.sh | 40 +++++++++++++++++++++++++\n 3 files changed, 115 insertions(+)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 66daacf..6381686 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -834,6 +834,52 @@ add_exec_commands () {\n \tmv \"$1.new\" \"$1\"\n }\n \n+# Check if the SHA-1 passed as an argument is a\n+# correct one, if not then print $2 in \"$todo\".badsha\n+# $1: the SHA-1 to test\n+# $2: the line to display if incorrect SHA-1\n+check_commit_sha () {\n+\tbadsha=f\n+\tif test -z $1\n+\tthen\n+\t\tbadsha=t\n+\telse\n+\t\tsha1_verif=\"$(git rev-parse --verify --quiet $1^{commit})\"\n+\t\tif test -z $sha1_verif\n+\t\tthen\n+\t\t\tbadsha=t\n+\t\tfi\n+\tfi\n+\n+\tif test $badsha = t\n+\tthen\n+\t\tprintf '%s\\n' \"$2\" >>\"$todo\".badsha\n+\tfi\n+}\n+\n+# prints the bad commits and bad commands\n+# from the todolist in stdin\n+check_bad_cmd_and_sha () {\n+\tgit stripspace --strip-comments |\n+\twhile read -r line\n+\tdo\n+\t\tread -r command sha1 rest <<EOF\n+$line\n+EOF\n+\t\tcase $command in\n+\t\t''|noop|x|\"exec\")\n+\t\t\t# Doesn't expect a SHA-1\n+\t\t\t;;\n+\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n+\t\t\tcheck_commit_sha $sha1 \"$line\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\tprintf '%s\\n' \"$line\" >>\"$todo\".badcmd\n+\t\t\t;;\n+\t\tesac\n+\tdone\n+}\n+\n # Print the list of the SHA-1 of the commits\n # from stdin to stdout\n todo_list_to_sha_list () {\n@@ -868,6 +914,8 @@ checkout_onto () {\n \n # Check if the user dropped some commits by mistake\n # Behaviour determined by rebase.missingCommitsCheck.\n+# Check if there is an unrecognized command or a\n+# bad SHA-1 in a command.\n check_todo_list () {\n \traise_error=f\n \n@@ -919,6 +967,28 @@ check_todo_list () {\n \t\t;;\n \tesac\n \n+\tcheck_bad_cmd_and_sha <\"$todo\"\n+\n+\tif test -s \"$todo\".badsha\n+\tthen\n+\t\traise_error=t\n+\n+\t\twarn \"Warning: the SHA-1 is missing or isn't\" \\\n+\t\t\t\"a commit in the following line(s):\"\n+\t\twarn_lines <\"$todo\".badsha\n+\t\twarn\n+\tfi\n+\n+\tif test -s \"$todo\".badcmd\n+\tthen\n+\t\traise_error=t\n+\n+\t\twarn \"Warning: the command isn't recognized\" \\\n+\t\t\t\"in the following line(s):\"\n+\t\twarn_lines <\"$todo\".badcmd\n+\t\twarn\n+\tfi\n+\n \tif test $raise_error = t\n \tthen\n \t\t# Checkout before the first commit of the\ndiff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\nindex fdbc900..9a96e15 100644\n--- a/t/lib-rebase.sh\n+++ b/t/lib-rebase.sh\n@@ -54,6 +54,11 @@ set_fake_editor () {\n \t\t\techo '# comment' >> \"$1\";;\n \t\t\">\")\n \t\t\techo >> \"$1\";;\n+\t\tbad)\n+\t\t\taction=\"badcmd\";;\n+\t\tfakesha)\n+\t\t\techo \"$action XXXXXXX False commit\" >> \"$1\"\n+\t\t\taction=pick;;\n \t\t*)\n \t\t\tsed -n \"${line}s/^pick/$action/p\" < \"$1\".tmp >> \"$1\"\n \t\t\taction=pick;;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex a92ae19..d691b1c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1184,4 +1184,44 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '\n \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+cat >expect <<EOF\n+Warning: the command isn't recognized in the following line(s):\n+ - badcmd $(git rev-list --oneline -1 master~1)\n+\n+You can fix this with 'git rebase --edit-todo'.\n+Or you can abort the rebase with 'git rebase --abort'.\n+EOF\n+\n+test_expect_success 'static check of bad command' '\n+\ttest_rebase_end tmp2 &&\n+\tset_fake_editor &&\n+\ttest_must_fail env FAKE_LINES=\"1 2 3 bad 4 5\" \\\n+\t\tgit rebase -i --root 2>actual &&\n+\ttest_cmp expect actual &&\n+\tFAKE_LINES=\"1 2 3 drop 4 5\" git rebase --edit-todo &&\n+\tgit rebase --continue &&\n+\ttest E = $(git cat-file commit HEAD | sed -ne \\$p) &&\n+\ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p)\n+'\n+\n+cat >expect <<EOF\n+Warning: the SHA-1 is missing or isn't a commit in the following line(s):\n+ - edit XXXXXXX False commit\n+\n+You can fix this with 'git rebase --edit-todo'.\n+Or you can abort the rebase with 'git rebase --abort'.\n+EOF\n+\n+test_expect_success 'static check of bad SHA-1' '\n+\ttest_config rebase.missingCommitsCheck error &&\n+\ttest_rebase_end tmp2 &&\n+\tset_fake_editor &&\n+\ttest_must_fail env FAKE_LINES=\"1 2 edit fakesha 3 4 5 #\" \\\n+\t\tgit rebase -i --root 2>actual &&\n+\ttest_cmp expect actual &&\n+\tFAKE_LINES=\"1 2 4 5 6\" git rebase --edit-todo &&\n+\tgit rebase --continue &&\n+\ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n+'\n+\n test_done\n-- \n2.4.3.371.g8992f2a\n"},{"id":"264649","messageId":"CAPig+cT+idnGkR0V-UfwbHwvjd_U=zbjHx+so7Jz+4o1XU5eHw@mail.gmail.com","threadId":"39696","inReplyTo":"1435009369-11496-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-23T18:24:46Z","receivedAt":"2015-06-23T18:24:46Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 22, 2015 at 5:42 PM, Galan Rémi\n<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:\n> Instead of removing a line to remove the commit, you can use the\n> command \"drop\" (just like \"pick\" or \"edit\"). It has the same effect as\n> deleting the line (removing the commit) except that you keep a visual\n> trace of your actions, allowing a better control and reducing the\n> possibility of removing a commit by mistake.\n>\n> Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>\n> ---\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index ac429a0..ecd277c 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1102,4 +1102,20 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '\n>         test $(git cat-file commit HEAD | sed -ne \\$p) = I\n>  '\n>\n> +test_rebase_end () {\n> +       test_when_finished \"git checkout master &&\n> +       git branch -D $1 &&\n> +       test_might_fail git rebase --abort\" &&\n> +       git checkout -b $1 master\n> +}\n\nThe way this is indented makes it difficult to see that lines 2 and 3\nare continuations of 1. Perhaps format it like this instead?\n\n    test_rebase_end () {\n        test_when_finished \"git checkout master &&\n            git branch -D $1 &&\n            test_might_fail git rebase --abort\" &&\n        git checkout -b $1 master\n    }\n\n> +\n> +test_expect_success 'drop' '\n> +       test_rebase_end dropTest &&\n> +       set_fake_editor &&\n> +       FAKE_LINES=\"1 drop 2 3 drop 4 5\" git rebase -i --root &&\n> +       test E = $(git cat-file commit HEAD | sed -ne \\$p) &&\n> +       test C = $(git cat-file commit HEAD^ | sed -ne \\$p) &&\n> +       test A = $(git cat-file commit HEAD^^ | sed -ne \\$p)\n> +'\n> +\n>  test_done\n> --\n> 2.4.3.371.g8992f2a\n"},{"id":"264653","messageId":"1637608183.731706.1435086079947.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39696","inReplyTo":"CAPig+cT+idnGkR0V-UfwbHwvjd_U=zbjHx+so7Jz+4o1XU5eHw@mail.gmail.com","subject":"Re: [PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-23T19:01:19Z","receivedAt":"2015-06-23T19:01:19Z","isPatch":false,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n> > +test_rebase_end () {\n> > +       test_when_finished \"git checkout master &&\n> > +       git branch -D $1 &&\n> > +       test_might_fail git rebase --abort\" &&\n> > +       git checkout -b $1 master\n> > +}\n> \n> The way this is indented makes it difficult to see that lines 2 and 3\n> are continuations of 1. Perhaps format it like this instead?\n> \n>     test_rebase_end () {\n>         test_when_finished \"git checkout master &&\n>             git branch -D $1 &&\n>             test_might_fail git rebase --abort\" &&\n>         git checkout -b $1 master\n>     }\n\nI completely agree with you, moreover it was indented like this before.\nI'll change it in my local version for now.\n\nIronically, it was modified after the following:\n\nGalan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > > +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=ignore' '\n> > > +       test_config rebase.missingCommitsCheck ignore &&\n> > > +       test_when_finished \"git checkout master &&\n> > > +               git branch -D tmp2\" &&\n> >\n> > Strange indentation.\n> \n> Considering that 'git branch -D tmp2' is a part of test_when_finished,\n> I wasn't sure of how it was supposed to be indented, so I did it this\n> way to show that it was still within test_when_finished and not a\n> separate command.\n> >         test_when_finished \"git checkout master &&\n> >         git branch -D tmp2\" &&\n> Doesn't seem as clear, especially if you quickly read the lines.\n> \n> For now, I have removed the tab.\n\n:p\n\nThanks,\nRémi\n"},{"id":"264655","messageId":"vpq1th2p7ka.fsf@anie.imag.fr","threadId":"39696","inReplyTo":"1637608183.731706.1435086079947.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-23T19:07:33Z","receivedAt":"2015-06-23T19:07:33Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>> > +test_rebase_end () {\n>> > +       test_when_finished \"git checkout master &&\n>> > +       git branch -D $1 &&\n>> > +       test_might_fail git rebase --abort\" &&\n>> > +       git checkout -b $1 master\n>> > +}\n>> \n>> The way this is indented makes it difficult to see that lines 2 and 3\n>> are continuations of 1. Perhaps format it like this instead?\n>> \n>>     test_rebase_end () {\n>>         test_when_finished \"git checkout master &&\n>>             git branch -D $1 &&\n>>             test_might_fail git rebase --abort\" &&\n>>         git checkout -b $1 master\n>>     }\n>\n> I completely agree with you, moreover it was indented like this before.\n> I'll change it in my local version for now.\n\nPerhaps to avoid confusion, stg like:\n\n\ttest_when_finished \"\n\t\t... &&\n\t\t...\n\t\" &&\n\tgit checkout\n\n(the closing \" alone on its line)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"264657","messageId":"CAPig+cRbtNV8YfvOQNPD_wEiyEkyDeTtbahYA-Vd9Q+9uv6z9w@mail.gmail.com","threadId":"39696","inReplyTo":"1637608183.731706.1435086079947.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-23T19:17:08Z","receivedAt":"2015-06-23T19:17:08Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 23, 2015 at 3:01 PM, Remi Galan Alfonso\n<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>> > +test_rebase_end () {\n>> > +       test_when_finished \"git checkout master &&\n>> > +       git branch -D $1 &&\n>> > +       test_might_fail git rebase --abort\" &&\n>> > +       git checkout -b $1 master\n>> > +}\n>>\n>> The way this is indented makes it difficult to see that lines 2 and 3\n>> are continuations of 1. Perhaps format it like this instead?\n>>\n>>     test_rebase_end () {\n>>         test_when_finished \"git checkout master &&\n>>             git branch -D $1 &&\n>>             test_might_fail git rebase --abort\" &&\n>>         git checkout -b $1 master\n>>     }\n>\n> I completely agree with you, moreover it was indented like this before.\n> I'll change it in my local version for now.\n>\n> Ironically, it was modified after the following:\n>\n> Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n>> Eric Sunshine <sunshine@sunshineco.com> writes:\n>> > > +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=ignore' '\n>> > > +       test_config rebase.missingCommitsCheck ignore &&\n>> > > +       test_when_finished \"git checkout master &&\n>> > > +               git branch -D tmp2\" &&\n>> >\n>> > Strange indentation.\n\nClearly, that guy who made the \"Strange indentation\" review comment\ndidn't know what he was talking about. ;-)\n\nIn that earlier review, I must have missed the fact that the quoted\nstring spanned multiple lines, and, unfortunately, got too busy with\nother things to say \"ah, the indentation makes perfect sense\" when\nyour response pointed out its true nature.\n\nMatthieu's suggestion of formatting it like:\n\n    test_when_finished \"\n        ... &&\n        ...\n    \" &&\n\nshould help to avoid such misconceptions.\n"},{"id":"264656","messageId":"392249968.731834.1435087097209.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39696","inReplyTo":"vpq1th2p7ka.fsf@anie.imag.fr","subject":"Re: [PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-23T19:18:17Z","receivedAt":"2015-06-23T19:18:17Z","isPatch":false,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n> Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n> \n> > Eric Sunshine <sunshine@sunshineco.com> writes:\n> >> > +test_rebase_end () {\n> >> > +       test_when_finished \"git checkout master &&\n> >> > +       git branch -D $1 &&\n> >> > +       test_might_fail git rebase --abort\" &&\n> >> > +       git checkout -b $1 master\n> >> > +}\n> >>\n> >> The way this is indented makes it difficult to see that lines 2 and 3\n> >> are continuations of 1. Perhaps format it like this instead?\n> >>\n> >>     test_rebase_end () {\n> >>         test_when_finished \"git checkout master &&\n> >>             git branch -D $1 &&\n> >>             test_might_fail git rebase --abort\" &&\n> >>         git checkout -b $1 master\n> >>     }\n> >\n> > I completely agree with you, moreover it was indented like this before.\n> > I'll change it in my local version for now.\n> \n> Perhaps to avoid confusion, stg like:\n> \n>         test_when_finished \"\n>                 ... &&\n>                 ...\n>         \" &&\n>         git checkout\n> \n> (the closing \" alone on its line)\n\nI think that the indentation on its own is enough to avoid confusion\n> test_rebase_end () {\n> \ttest_when_finished \"git checkout master &&\n> \t\tgit branch -D $1 &&\n> \t\ttest_might_fail git rebase --abort\" &&\n> \tgit checkout -b $1 master\n> }\nbut your idea is fine as well, so I'm ok with either way.\n\nThanks,\nRémi\n"},{"id":"264659","messageId":"xmqqwpyucjj1.fsf@gitster.dls.corp.google.com","threadId":"39696","inReplyTo":"1435009369-11496-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-23T19:27:30Z","receivedAt":"2015-06-23T19:27:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n\n> +test_rebase_end () {\n> +\ttest_when_finished \"git checkout master &&\n> +\tgit branch -D $1 &&\n\nIs this one guaranteed to succeed?  Do we want to consider it a\nfailure to remove \"$1\" (e.g. dropTest)?\n\n    $ git branch -D no-such-branch ; echo $?\n    error: branch 'no-such-branch' not found.\n    1\n\nIf dropTest branch did not exist before the test that begins with\na call to this function, what happens?\n\nBesides, a function that must be called at the beginning of a test\npiece has a name that ends with _end?  That sounds funny, no?\n\n> +\ttest_might_fail git rebase --abort\" &&\n> +\tgit checkout -b $1 master\n> +}\n\nI'm wondering if this is not sufficient.\n\n\ttest_rebase_i_drop_prepare () {\n\t\tgit reset --hard &&\n\t        git checkout -B \"$1\" master\n\t}\n\nI am guessing that you named _end because it has when_finished, but\nas far as I can tell, even after these three patches, the tests do\nnot really rely on the fact that it is on 'master' branch.  More\nimportantly, just being on 'master' branch is not a sufficient\ncleanliness for the next test (and that is why you added these\n\"branch -D\" and \"might-fail rebase --abort\" to this function in the\nfirst place), it seems.  So...\n\n> +test_expect_success 'drop' '\n> +\ttest_rebase_end dropTest &&\n> +\tset_fake_editor &&\n> +\tFAKE_LINES=\"1 drop 2 3 drop 4 5\" git rebase -i --root &&\n> +\ttest E = $(git cat-file commit HEAD | sed -ne \\$p) &&\n> +\ttest C = $(git cat-file commit HEAD^ | sed -ne \\$p) &&\n> +\ttest A = $(git cat-file commit HEAD^^ | sed -ne \\$p)\n> +'\n> +\n>  test_done\n"},{"id":"264661","messageId":"xmqqpp4mciu7.fsf@gitster.dls.corp.google.com","threadId":"39696","inReplyTo":"1435009369-11496-3-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCHv6 3/3] git rebase -i: add static check for commands and SHA-1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-23T19:42:24Z","receivedAt":"2015-06-23T19:42:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n\n>  I used:\n>    read -r command sha1 rest <<EOF\n>    $line\n>    EOF\n>  because\n>    printf '%s' \"$line\" | read -r command sha1 rest\n>  doesn't work (the 3 variables have no value as a result).\n>  There might be a better way to do this, but I don't have it right now.\n\n\twhile read line\n        do\n\t\t(\n\t\t\tIFS=' '\n                        set x $line\n\t\t\tshift\n                        # now $1 is your command, $2 is sha1, $3 is remainder\n\t\t\t...\n\t\t)\n\tdone\n\nperhaps?\n\nBut more importantly, why do you even need to keep the bad ones in a\nseparate .badcmd and .badsha files?  Isn't that bloating your changes\nunnecessarily, iow, if you issued your warning as you encounter them,\nwouldn't the change become cleaner and easier to understand (and as\na side effect it may even become smaller)?  The _only_ thing that\nyou would get by keeping them in temporary files is that you can do\n\"one header and bunch of errors\", but is it so common to make a bad\nedit to the insn sheet that \"a sequence of errors, one per line\"\nbecomes more burdensome to the end user?\n\nI would think\n\n\tstripspace |\n\twhile read -r command sha1 rest\n        do\n        \t...\n\nand showing the warning as you detect inside that loop would be\nsufficient.  Perhaps I am missing subtle details of what you are\ndoing.\n"},{"id":"264664","messageId":"122907552.733385.1435090110363.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39696","inReplyTo":"xmqqwpyucjj1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-23T20:08:30Z","receivedAt":"2015-06-23T20:08:30Z","isPatch":false,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> > Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n> > \n> > > +test_rebase_end () {\n> > > +        test_when_finished \"git checkout master &&\n> > > +        git branch -D $1 &&\n> > \n> > Is this one guaranteed to succeed?  Do we want to consider it a\n> > failure to remove \"$1\" (e.g. dropTest)?\n> > \n> >     $ git branch -D no-such-branch ; echo $?\n> >     error: branch 'no-such-branch' not found.\n> >     1\n> > \n> > If dropTest branch did not exist before the test that begins with\n> > a call to this function, what happens?\n> \n\nSince the function is\n> \ttest_when_finished \"git checkout master &&\n> \t\tgit branch -D $1 &&\n> \t\ttest_might_fail git rebase --abort\" &&\n> \tgit checkout -b $1 master\nIf $1 doesn't exist, then it means that 'git checkout -b $1 master'\nfailed, thus the test would fail before reaching the part in\n'test_when_finished'.\nHowever I guess there are more reasons that could cause 'git branch -D\n$1' to fail so I'll add a 'test_might_fail' in front of it.\n\n> Besides, a function that must be called at the beginning of a test\n> piece has a name that ends with _end?  That sounds funny, no?\n\nI see your point but I'm not really sure how to call it, considering\nthat what it does is creating a branch to test on it, and taking care\nof the cleaning at the end of the test.\nMaybe something more generic like \"setup_and_clean\" ?\n \n> > +        test_might_fail git rebase --abort\" &&\n> > +        git checkout -b $1 master\n> > +}\n> \n> I'm wondering if this is not sufficient.\n> \n>         test_rebase_i_drop_prepare () {\n>                 git reset --hard &&\n>                 git checkout -B \"$1\" master\n>         }\n> \n> I am guessing that you named _end because it has when_finished, but\n> as far as I can tell, even after these three patches, the tests do\n> not really rely on the fact that it is on 'master' branch.  More\n> importantly, just being on 'master' branch is not a sufficient\n> cleanliness for the next test (and that is why you added these\n> \"branch -D\" and \"might-fail rebase --abort\" to this function in the\n> first place), it seems.  So...\n\nI removed the branch in case someone used the same name when creating\na branch in a future test. However he would notice it when writing the\ntest and it's not something that would suddenly break when modifying\nthe code, so it might not be necessary.\nThe \"might-fail rebase --abort\" is there in case this test fails in\nthe middle (because of a future modification of rebase for example) to\navoid failling all the following tests that use rebase.\n\nThe name \"test_rebase_i_drop_prepare\" wouldn't be accurate since 2/3\nand 3/3 uses the function as well and don't really have much to do\nwith 'drop'.\n\nThanks,\nRémi\n"},{"id":"264668","messageId":"vpq8ubamaz0.fsf@anie.imag.fr","threadId":"39696","inReplyTo":"392249968.731834.1435087097209.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-23T20:22:11Z","receivedAt":"2015-06-23T20:22:11Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n\n> I think that the indentation on its own is enough to avoid confusion\n>> test_rebase_end () {\n>> \ttest_when_finished \"git checkout master &&\n>> \t\tgit branch -D $1 &&\n>> \t\ttest_might_fail git rebase --abort\" &&\n>> \tgit checkout -b $1 master\n>> }\n> but your idea is fine as well, so I'm ok with either way.\n\nRead too quickly, it looks like a mis-indentation (I could laugh at Eric\nhere, but I made the same confusion when reading the code at first). By\n\"avoid the confusion\" I mean \"make it clear it's not a mis-indentation\".\n\nAnyway, we're just bikeshedding here. Any version is fine with me.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"264679","messageId":"1208569888.733725.1435091732761.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39696","inReplyTo":"xmqqpp4mciu7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv6 3/3] git rebase -i: add static check for commands and SHA-1","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-23T20:35:32Z","receivedAt":"2015-06-23T20:35:32Z","isPatch":false,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n> \n> >  I used:\n> >    read -r command sha1 rest <<EOF\n> >    $line\n> >    EOF\n> >  because\n> >    printf '%s' \"$line\" | read -r command sha1 rest\n> >  doesn't work (the 3 variables have no value as a result).\n> >  There might be a better way to do this, but I don't have it right now.\n> \n>         while read line\n>         do\n>                 (\n>                         IFS=' '\n>                         set x $line\n>                         shift\n>                         # now $1 is your command, $2 is sha1, $3 is remainder\n>                         ...\n>                 )\n>         done\n> \n> perhaps?\n\nWill try, thanks!\n\n> But more importantly, why do you even need to keep the bad ones in a\n> separate .badcmd and .badsha files?  Isn't that bloating your changes\n> unnecessarily, iow, if you issued your warning as you encounter them,\n> wouldn't the change become cleaner and easier to understand (and as\n> a side effect it may even become smaller)?  The _only_ thing that\n> you would get by keeping them in temporary files is that you can do\n> \"one header and bunch of errors\", but is it so common to make a bad\n> edit to the insn sheet that \"a sequence of errors, one per line\"\n> becomes more burdensome to the end user?\n> \n> I would think\n> \n>         stripspace |\n>         while read -r command sha1 rest\n>         do\n>                 ...\n> \n> and showing the warning as you detect inside that loop would be\n> sufficient.  Perhaps I am missing subtle details of what you are\n> doing.\n\nYou're not missing subtle details, it is as you said, I tough it would\nbe clearer for the user to have \"one header and a bunch of errors\".\nMoreover while it would make the patch smaller and easier to\nunderstand, I am not sure about making it cleaner; I guess I will have\nto try and see how it ends up.\n\nWhat I'm not completely happy with your proposition is the fact that \nif there are multiple errors of the same kind, the output would look \nsomething like:\n> Warning: the command isn't recognized in the following line:\n> badcmd1 some_sha some_commit_message\n> Warning: the command isn't recognized in the following line:\n> badcmd2 some_sha some_commit_message\n(I don't think it would be good to squash some understandable warning\nmessage and the faulty line in one line, it would probably end up\nbeing too long)\nHowever as you say, such mistakes are uncommon so I guess it's fine.\n\nThanks,\nRémi\n"},{"id":"264681","messageId":"xmqqegl2cga9.fsf@gitster.dls.corp.google.com","threadId":"39696","inReplyTo":"xmqqwpyucjj1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-23T20:37:34Z","receivedAt":"2015-06-23T20:37:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n>\n>> +test_rebase_end () {\n>> +\ttest_when_finished \"git checkout master &&\n>> +\tgit branch -D $1 &&\n>\n> Is this one guaranteed to succeed?  Do we want to consider it a\n> failure to remove \"$1\" (e.g. dropTest)?\n>\n>     $ git branch -D no-such-branch ; echo $?\n>     error: branch 'no-such-branch' not found.\n>     1\n>\n> If dropTest branch did not exist before the test that begins with\n> a call to this function, what happens?\n>\n> Besides, a function that must be called at the beginning of a test\n> piece has a name that ends with _end?  That sounds funny, no?\n\nAh, scratch this last paragraph.  I didn't see this is a\nmulti-command \"when_finished\".\n\nBut other parts of what I said still stands.  For example, even in a\nmulti-command \"when_finished\", \"git branch -D $1 &&\" if the main\nbody of the test failed to create the branch \"$1\", that command\nwould fail and skip the remainder of the clean-up, so the first\npoint above is still suspect.\n"},{"id":"264682","messageId":"xmqqa8vqcg7h.fsf@gitster.dls.corp.google.com","threadId":"39696","inReplyTo":"vpq8ubamaz0.fsf@anie.imag.fr","subject":"Re: [PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-23T20:39:14Z","receivedAt":"2015-06-23T20:39:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n>\n>> I think that the indentation on its own is enough to avoid confusion\n>>> test_rebase_end () {\n>>> \ttest_when_finished \"git checkout master &&\n>>> \t\tgit branch -D $1 &&\n>>> \t\ttest_might_fail git rebase --abort\" &&\n>>> \tgit checkout -b $1 master\n>>> }\n>> but your idea is fine as well, so I'm ok with either way.\n>\n> Read too quickly, it looks like a mis-indentation (I could laugh at Eric\n> here, but I made the same confusion when reading the code at first). By\n> \"avoid the confusion\" I mean \"make it clear it's not a mis-indentation\".\n\nYes, that stray \" fooled me as well.  If it were following your\nsuggestion in the earlier message on this thread, i.e.\n\n\ttest_when_finished \"\n\t\t... &&\n\t\t...\n\t\" &&\n\tgit checkout\n\nI wouldn't have to waste time commenting on it ;-)\n"},{"id":"264688","messageId":"460841845.733813.1435092420048.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39696","inReplyTo":"xmqqa8vqcg7h.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv6 1/3] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-23T20:47:00Z","receivedAt":"2015-06-23T20:47:00Z","isPatch":false,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n> \n> > Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n> >\n> >> I think that the indentation on its own is enough to avoid confusion\n> >>> test_rebase_end () {\n> >>>         test_when_finished \"git checkout master &&\n> >>>                 git branch -D $1 &&\n> >>>                 test_might_fail git rebase --abort\" &&\n> >>>         git checkout -b $1 master\n> >>> }\n> >> but your idea is fine as well, so I'm ok with either way.\n> >\n> > Read too quickly, it looks like a mis-indentation (I could laugh at Eric\n> > here, but I made the same confusion when reading the code at first). By\n> > \"avoid the confusion\" I mean \"make it clear it's not a mis-indentation\".\n> \n> Yes, that stray \" fooled me as well.  If it were following your\n> suggestion in the earlier message on this thread, i.e.\n> \n>         test_when_finished \"\n>                 ... &&\n>                 ...\n>         \" &&\n>         git checkout\n> \n> I wouldn't have to waste time commenting on it ;-)\n\nI will do it this way then. ;)\n\nThanks,\nRémi\n"}]}