{"thread":{"id":"39748","subject":"rebase -i: drop, missing commits and static checks","startedAt":"2015-06-29T20:20:29Z","lastAt":"2015-06-30T17:09:38Z","messageCount":14,"participants":["Galan Rémi","Matthieu Moy","Remi Galan Alfonso","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"265178","messageId":"1435609232-14232-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39748","inReplyTo":null,"subject":"rebase -i: drop, missing commits and static checks","fromName":"Galan Rémi","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-29T20:20:29Z","receivedAt":"2015-06-29T20:20:29Z","isPatch":false,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Changes between versions:\n\nIn t3404:\n\nChanged 'test_rebase_end' to 'rebase_setup_and_clean'.\nChanged the indentation in 'rebase_setup_and_clean'.\nChanged the names of the branches created in my tests (avoid names\nlike 'tmp').\nAdded 'test_might_fail' in front of 'git branch -D'.\nRemove 'test_config rebase.missingCommitsCheck error' in the last test\n('static check of bad SHA-1') because it was useless.\n\nIn git-rebase--interactive.sh:\n\nErrors found in commands and SHA-1 (static check) are displayed on the\nspot.\nI used return values to signal to the calling functions if there is an\nerror.\nThe whole while block in 'check_bad_cmd_and_sha' with the return is in a \n'(\n\t[...]\n)'\nblock because 'retval' would not have been correctly affected when\ngetting out of the loop since the while is in a pipe.\n\nA thought occured to me:\nShouldn't all the checking also be called in a 'rebase --continue',\nconsidering that it can be called after a 'rebase --edit-todo' ?\n(Right now it is only called after closing the editor in 'rebase -i')\n\n[PATCHv7 1/3] git-rebase -i: add command \"drop\" to remove a commit\n[PATCHv7 2/3] git rebase -i: warn about removed commits\n[PATCHv7 3/3] git rebase -i: add static check for commands and SHA-1\n\nRémi\n(It seems that with 'send-email', 1 time out of 2 I get:\n'5.7.8 Error: authentication failed: authentication failure')\n"},{"id":"265180","messageId":"1435609232-14232-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39748","inReplyTo":"1435609232-14232-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"[PATCHv7 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-29T20:20:30Z","receivedAt":"2015-06-29T20:20:30Z","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 \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 | 18 ++++++++++++++++++\n 4 files changed, 25 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 # \" \" -- 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..3d059e5 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1102,4 +1102,22 @@ 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+rebase_setup_and_clean () {\n+\ttest_when_finished \"\n+\t\tgit checkout master &&\n+\t\ttest_might_fail git branch -D $1 &&\n+\t\ttest_might_fail git rebase --abort\n+\t\" &&\n+\tgit checkout -b $1 master\n+}\n+\n+test_expect_success 'drop' '\n+\trebase_setup_and_clean drop-test &&\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.532.gf6210e5.dirty\n"},{"id":"265181","messageId":"1435609232-14232-3-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39748","inReplyTo":"1435609232-14232-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"[PATCHv7 2/3] git rebase -i: warn about removed commits","fromName":"Galan Rémi","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-29T20:20:31Z","receivedAt":"2015-06-29T20:20:31Z","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 \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 3e37b93..0360e7b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2169,6 +2169,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 ::\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 3d059e5..904a2d0 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1120,4 +1120,70 @@ test_expect_success 'drop' '\n \ttest A = $(git cat-file commit HEAD^^ | sed -ne \\$p)\n '\n \n+cat >expect <actual &&\n+\ttest D = $(git cat-file commit HEAD | sed -ne \\$p) &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <actual &&\n+\ttest_cmp expect actual &&\n+\ttest D = $(git cat-file commit HEAD | sed -ne \\$p)\n+'\n+\n+cat >expect <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.532.gf6210e5.dirty\n"},{"id":"265179","messageId":"1435609232-14232-4-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39748","inReplyTo":"1435609232-14232-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"[PATCHv7 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-29T20:20:32Z","receivedAt":"2015-06-29T20:20:32Z","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 \n---\n git-rebase--interactive.sh | 75 +++++++++++++++++++++++++++++++++++++++++++\n t/lib-rebase.sh | 5 +++\n t/t3404-rebase-interactive.sh | 39 ++++++++++++++++++++++\n 3 files changed, 119 insertions(+)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 66daacf..ec4a068 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -834,6 +834,73 @@ 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=0\n+\tif test -z $1\n+\tthen\n+\t\tbadsha=1\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=1\n+\t\tfi\n+\tfi\n+\n+\tif test $badsha -ne 0\n+\tthen\n+\t\twarn \"Warning: the SHA-1 is missing or isn't\" \\\n+\t\t\t\"a commit in the following line:\"\n+\t\twarn \" - $2\"\n+\t\twarn\n+\tfi\n+\n+\treturn $badsha\n+}\n+\n+# prints the bad commits and bad commands\n+# from the todolist in stdin\n+check_bad_cmd_and_sha () {\n+\tretval=0\n+\tgit stripspace --strip-comments |\n+\t(\n+\t\twhile read -r line\n+\t\tdo\n+\t\t\tIFS=' '\n+\t\t\tset x $line\n+\t\t\tshift\n+\t\t\tcommand=$1\n+\t\t\tsha1=$2\n+\n+\t\t\tcase $command in\n+\t\t\t''|noop|x|\"exec\")\n+\t\t\t\t# Doesn't expect a SHA-1\n+\t\t\t\t;;\n+\t\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n+\t\t\t\tcheck_commit_sha $sha1 \"$line\"\n+\t\t\t\tif test $? -ne 0\n+\t\t\t\tthen\n+\t\t\t\t\tretval=1\n+\t\t\t\tfi\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\twarn \"Warning: the command isn't recognized\" \\\n+\t\t\t\t\t\"in the following line:\"\n+\t\t\t\twarn \" - $line\"\n+\t\t\t\twarn\n+\t\t\t\tretval=1\n+\t\t\t\t;;\n+\t\t\tesac\n+\t\tdone\n+\n+\t\treturn $retval\n+\t)\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 +935,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 +988,12 @@ check_todo_list () {\n \t\t;;\n \tesac\n \n+\tcheck_bad_cmd_and_sha <\"$todo\"\n+\tif test $? -ne 0\n+\tthen\n+\t\traise_error=t\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 904a2d0..9b2c51c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1186,4 +1186,43 @@ 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 <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 <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.532.gf6210e5.dirty\n"},{"id":"265205","messageId":"1435655961-31263-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"39748","inReplyTo":"1435609232-14232-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"[PATCH 0/3] rebase -i: drop, missing commits and static checks","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-06-30T09:19:18Z","receivedAt":"2015-06-30T09:19:18Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Hi,\n\nHere are a few fixes to squash into the commits of the series. Other\nthan that, the series looks good to me.\n\nJunio: do you prefer a reroll or do you want to apply locally?\n\nMatthieu Moy (3):\n fixup! git rebase -i: add static check for commands and SHA-1\n fixup! git rebase -i: warn about removed commits\n fixup! git rebase -i: warn about removed commits\n\n git-rebase--interactive.sh | 32 +++++++++++++++++++++-----------\n t/t3404-rebase-interactive.sh | 4 ++--\n 2 files changed, 23 insertions(+), 13 deletions(-)\n\n-- \n2.5.0.rc0.10.g7792c2a\n"},{"id":"265206","messageId":"1435655961-31263-2-git-send-email-Matthieu.Moy@imag.fr","threadId":"39748","inReplyTo":"1435655961-31263-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH 1/3] fixup! git rebase -i: add static check for commands and SHA-1","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-06-30T09:19:19Z","receivedAt":"2015-06-30T09:19:19Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Signed-off-by: Matthieu Moy \n---\n git-rebase--interactive.sh | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex ec4a068..9041d15 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -871,8 +871,7 @@ check_bad_cmd_and_sha () {\n \t\twhile read -r line\n \t\tdo\n \t\t\tIFS=' '\n-\t\t\tset x $line\n-\t\t\tshift\n+\t\t\tset -- $line\n \t\t\tcommand=$1\n \t\t\tsha1=$2\n \n@@ -881,8 +880,7 @@ check_bad_cmd_and_sha () {\n \t\t\t\t# Doesn't expect a SHA-1\n \t\t\t\t;;\n \t\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n-\t\t\t\tcheck_commit_sha $sha1 \"$line\"\n-\t\t\t\tif test $? -ne 0\n+\t\t\t\tif ! check_commit_sha $sha1 \"$line\"\n \t\t\t\tthen\n \t\t\t\t\tretval=1\n \t\t\t\tfi\n@@ -988,8 +986,7 @@ check_todo_list () {\n \t\t;;\n \tesac\n \n-\tcheck_bad_cmd_and_sha <\"$todo\"\n-\tif test $? -ne 0\n+\tif ! check_bad_cmd_and_sha <\"$todo\"\n \tthen\n \t\traise_error=t\n \tfi\n-- \n2.5.0.rc0.10.g7792c2a\n"},{"id":"265207","messageId":"1435655961-31263-3-git-send-email-Matthieu.Moy@imag.fr","threadId":"39748","inReplyTo":"1435655961-31263-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH 2/3] fixup! git rebase -i: warn about removed commits","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-06-30T09:19:20Z","receivedAt":"2015-06-30T09:19:20Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Signed-off-by: Matthieu Moy \n---\n git-rebase--interactive.sh | 21 +++++++++++++++++----\n 1 file changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 9041d15..0117791 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -156,8 +156,17 @@ Commands:\n \n These lines can be re-ordered; they are executed from top to bottom.\n \n+EOF\n+\tif test $(get_missing_commit_check_level) = error\n+\tthen\n+\t\tgit stripspace --comment-lines >>\"$todo\" <<\\EOF\n+Do not remove any line. Use 'drop' explicitly to remove a commit.\n+EOF\n+\telse\n+\t\tgit stripspace --comment-lines >>\"$todo\" <<\\EOF\n If you remove a line here THAT COMMIT WILL BE LOST.\n EOF\n+\tfi\n }\n \n make_patch () {\n@@ -931,6 +940,13 @@ checkout_onto () {\n \tgit update-ref ORIG_HEAD $orig_head\n }\n \n+get_missing_commit_check_level () {\n+\tcheck_level=$(git config --get rebase.missingCommitsCheck)\n+\tcheck_level=${check_level:-ignore}\n+\t# Don't be case sensitive\n+\tprintf '%s' \"$check_level\" | tr 'A-Z' 'a-z'\n+}\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@@ -938,10 +954,7 @@ checkout_onto () {\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+\tcheck_level=$(get_missing_commit_check_level)\n \n \tcase \"$check_level\" in\n \twarn|error)\n-- \n2.5.0.rc0.10.g7792c2a\n"},{"id":"265208","messageId":"1435655961-31263-4-git-send-email-Matthieu.Moy@imag.fr","threadId":"39748","inReplyTo":"1435655961-31263-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH 3/3] fixup! git rebase -i: warn about removed commits","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-06-30T09:19:21Z","receivedAt":"2015-06-30T09:19:21Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Signed-off-by: Matthieu Moy \n---\n git-rebase--interactive.sh | 2 +-\n t/t3404-rebase-interactive.sh | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 0117791..8090d80 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -985,7 +985,7 @@ check_todo_list () {\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\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\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 9b2c51c..ebdab4b 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1140,7 +1140,7 @@ 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+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/missing-commit.\n@@ -1163,7 +1163,7 @@ Dropped commits (newer to older):\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+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-- \n2.5.0.rc0.10.g7792c2a\n"},{"id":"265238","messageId":"1849903896.89424.1435676774069.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39748","inReplyTo":"1435655961-31263-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-30T15:06:14Z","receivedAt":"2015-06-30T15:06:14Z","isPatch":true,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Matthieu Moy writes:\n> Hi,\n> \n> Here are a few fixes to squash into the commits of the series. Other\n> than that, the series looks good to me.\n> \n> Junio: do you prefer a reroll or do you want to apply locally?\n> \n> Matthieu Moy (3):\n> fixup! git rebase -i: add static check for commands and SHA-1\n> fixup! git rebase -i: warn about removed commits\n> fixup! git rebase -i: warn about removed commits\n> \n> git-rebase--interactive.sh | 32 +++++++++++++++++++++-----------\n> t/t3404-rebase-interactive.sh | 4 ++--\n> 2 files changed, 23 insertions(+), 13 deletions(-)\n\nThanks for the various fixes !\n\nHowever, I am still wondering about:\n\nGalan Rémi writes:\n> Shouldn't all the checking also be called in a 'rebase --continue',\n> considering that it can be called after a 'rebase --edit-todo' ?\n> (Right now it is only called after closing the editor in 'rebase -i')\n\nWhat's your opinion on it?\n\nShort example:\n\n'git rebase -i HEAD~2'\n\tpick commit_sha_1 commit_msg_1\n\ttick commit_sha_2 commit_msg_2\nAn error is raised before anything is done.\n'git rebase --edit-todo'\n\tpick commit_sha_1 commit_msg_1\n\ttick commit_sha_2 commit_msg_2\n(nothing changed)\n'git rebase --continue'\nAn error is raised after having picked the first commit.\n\nThe same is relevent with bad sha and missing commits (in fact even\nmore relevant with missing commits since that would be silent loss of\ninformation).\n\nThanks,\nRémi\n"},{"id":"265239","messageId":"vpqegkt434o.fsf@anie.imag.fr","threadId":"39748","inReplyTo":"1849903896.89424.1435676774069.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-30T15:39:19Z","receivedAt":"2015-06-30T15:39:19Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Remi Galan Alfonso writes:\n\n> Galan Rémi writes:\n>> Shouldn't all the checking also be called in a 'rebase --continue',\n>> considering that it can be called after a 'rebase --edit-todo' ?\n>> (Right now it is only called after closing the editor in 'rebase -i')\n>\n> What's your opinion on it?\n\nIt would probably be better to run the check when running \"git rebase\n--continue\". This way, we would have a guarantee that the todo-list is\nsyntactically correct when going through it.\n\nJust checking after --edit-todo would not guarantee that:\n\n $ git rebase --edit-todo\n # Add some syntax errors, save and quit\n Warning: the command isn't recognized ...\n \n # Hmm, let's ignore that warning\n $ git rebase --continue\n Unknown command: ...\n Please fix this using 'git rebase --edit-todo'\n\nBut in any case, the most important is the initial edition. It covers\n99.9% of use-cases. So, I'd say: if you have time to add the checks at\nthe other relevant places, good, but not doing it shouldn't block the\ninclusion of the series.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"265247","messageId":"xmqq7fql8949.fsf@gitster.dls.corp.google.com","threadId":"39748","inReplyTo":"1849903896.89424.1435676774069.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-30T16:16:22Z","receivedAt":"2015-06-30T16:16:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Remi Galan Alfonso \nwrites:\n\n> Galan Rémi writes:\n>> Shouldn't all the checking also be called in a 'rebase --continue',\n>> considering that it can be called after a 'rebase --edit-todo' ?\n>> (Right now it is only called after closing the editor in 'rebase -i')\n>\n> What's your opinion on it?\n>\n> Short example:\n>\n> 'git rebase -i HEAD~2'\n> \tpick commit_sha_1 commit_msg_1\n> \ttick commit_sha_2 commit_msg_2\n> An error is raised before anything is done.\n> 'git rebase --edit-todo'\n> \tpick commit_sha_1 commit_msg_1\n> \ttick commit_sha_2 commit_msg_2\n> (nothing changed)\n> 'git rebase --continue'\n> An error is raised after having picked the first commit.\n>\n> The same is relevent with bad sha and missing commits (in fact even\n> more relevant with missing commits since that would be silent loss of\n> information).\n\nThe place where an error can be introduced is (assuming that what\n\"rebase -i\" writes out itself is perfect ;-) where we allow the user\nto edit, so instead of checking before \"--continue\", I would expect\na sane design would check immediately after the editor we spawned\nreturns.\n\nThe codepath that allows the insn sheet getting edited and the\ncodepath that handles --edit-todo have to do many things the same\nway (i.e. use collapse_todo_ids to prepare the file for editing,\nspawn the editor, receive the result and use expand_todo_ids to\nprepare the file to be used by us), and I would have expected for\nthese two to be using a single helper---and a sanity check like the\nabove can and should be done when we receive the result from the\neditor, immediately before running expand_todo_ids perhaps.\n\nIf they are not using such a single helper right now, perhaps that\nis the preliminary restructuring of the code that is needed?\n"},{"id":"265251","messageId":"70428267.93232.1435683724659.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39748","inReplyTo":"xmqq7fql8949.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-30T17:02:04Z","receivedAt":"2015-06-30T17:02:04Z","isPatch":true,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Matthieu Moy writes:\n> Remi Galan Alfonso writes:\n> \n> > Galan Rémi writes:\n> >> Shouldn't all the checking also be called in a 'rebase --continue',\n> >> considering that it can be called after a 'rebase --edit-todo' ?\n> >> (Right now it is only called after closing the editor in 'rebase -i')\n> >\n> > What's your opinion on it?\n> \n> It would probably be better to run the check when running \"git rebase\n> --continue\". This way, we would have a guarantee that the todo-list is\n> syntactically correct when going through it.\n> \n> Just checking after --edit-todo would not guarantee that:\n\nThat's actually what I had in mind, my message wasn't clear enough, my\nbad.\n\n> But in any case, the most important is the initial edition. It covers\n> 99.9% of use-cases. So, I'd say: if you have time to add the checks at\n> the other relevant places, good, but not doing it shouldn't block the\n> inclusion of the series.\n\nAgreed, that's something that can be done in a future patch without\ninterfering with this one.\n\nJunio C Hamano writes:\n> The place where an error can be introduced is (assuming that what\n> \"rebase -i\" writes out itself is perfect ;-) where we allow the user\n> to edit, so instead of checking before \"--continue\", I would expect\n> a sane design would check immediately after the editor we spawned\n> returns.\n\nMakes sense but we would have the problem mentioned by Matthieu:\n> Warning: the command isn't recognized ...\n> \n> # Hmm, let's ignore that warning\n> $ git rebase --continue\n\nWhile in most cases it would be irrelevant (it would be the user that \nignored the error on purpose), I can imagine the following situation:\n - 'git rebase --edit-todo'\n - get an error\n - go do something else, forgot where I was when I come back\n - 'git status', \"you are in the middle of a rebasing\"\n - 'git rebase --continue'\n\n> The codepath that allows the insn sheet getting edited and the\n> codepath that handles --edit-todo have to do many things the same\n> way (i.e. use collapse_todo_ids to prepare the file for editing,\n> spawn the editor, receive the result and use expand_todo_ids to\n> prepare the file to be used by us), and I would have expected for\n> these two to be using a single helper---and a sanity check like the\n> above can and should be done when we receive the result from the\n> editor, immediately before running expand_todo_ids perhaps.\n> \n> If they are not using such a single helper right now, perhaps that\n> is the preliminary restructuring of the code that is needed?\n\nGood point, maybe I'll try doing that at a later date, however as\nMatthieu said, I think it doesn't hurt to add this patch (if there is\nno further corrections to do) and do additional things in a later\npatch.\n\nThanks,\nRémi\n"},{"id":"265252","messageId":"vpqy4j12knh.fsf@anie.imag.fr","threadId":"39748","inReplyTo":"70428267.93232.1435683724659.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-30T17:03:46Z","receivedAt":"2015-06-30T17:03:46Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Remi Galan Alfonso writes:\n\n> Junio C Hamano writes:\n>> The place where an error can be introduced is (assuming that what\n>> \"rebase -i\" writes out itself is perfect ;-) where we allow the user\n>> to edit, so instead of checking before \"--continue\", I would expect\n>> a sane design would check immediately after the editor we spawned\n>> returns.\n>\n> Makes sense but we would have the problem mentioned by Matthieu:\n>> Warning: the command isn't recognized ...\n>> \n>> # Hmm, let's ignore that warning\n>> $ git rebase --continue\n\nThere's an alternative:\n\n$ git rebase --edit-todo\n# Make mistakes, save and quit\nYour todo-list has the following issues:\n- ...\nDo you want to edit again (no aborts the rebase) [Y/n]?\n\nThere's a precedent with the 'e' command of \"git add -p\". I have a\nslight preference for non-interactive commands so I prefer not going\nthis way though.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"265255","messageId":"xmqqmvzh6s31.fsf@gitster.dls.corp.google.com","threadId":"39748","inReplyTo":"vpqy4j12knh.fsf@anie.imag.fr","subject":"Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-30T17:09:38Z","receivedAt":"2015-06-30T17:09:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> There's an alternative:\n>\n> $ git rebase --edit-todo\n> # Make mistakes, save and quit\n> Your todo-list has the following issues:\n> - ...\n> Do you want to edit again (no aborts the rebase) [Y/n]?\n>\n> There's a precedent with the 'e' command of \"git add -p\". I have a\n> slight preference for non-interactive commands so I prefer not going\n> this way though.\n\nedit-todo (and \"rebase -i\" in general) is all about going\ninteractive, isn't it? I was almost going to suggest to keep\nspawning the editor until the user gets it right, but that would\ninfinitely repeat when GIT_EDITOR is set to a broken script, so\nI didn't ;-).\n"}]}