{"thread":{"id":"39583","subject":"[PATCH/RFCv5 1/3] git-rebase -i: add command \"drop\" to remove a commit","startedAt":"2015-06-10T10:10:33Z","lastAt":"2015-06-15T08:25:44Z","messageCount":12,"participants":["Galan Rémi","Matthieu Moy","Remi Galan Alfonso"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"263458","messageId":"1433931035-20011-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39583","inReplyTo":null,"subject":"[PATCH/RFCv5 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-10T10:10:33Z","receivedAt":"2015-06-10T10:10:33Z","isPatch":true,"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 In t3404, test_rebase_end is introduced, mainly because it will be\n reused in future tests (in 2/3 and 3/3).\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.2.496.gdc9319a\n"},{"id":"263459","messageId":"1433931035-20011-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39583","inReplyTo":"1433931035-20011-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"[PATCH/RFCv5 2/3] git rebase -i: warn about removed commits","fromName":"Galan Rémi","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-10T10:10:34Z","receivedAt":"2015-06-10T10:10:34Z","isPatch":true,"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 In git-rebase--interactive, in the error case of check_todo_list, I\n added 'git checkout $onto' so that using 'die' for the error allows\n to use 'git rebase --edit-todo' (otherwise HEAD would not have been\n changed and it still would be placed after the commits of the\n rebase).\n Since now it doesn't abort the rebase, the documentation and the\n messages in the case error have changed.\n I moved the error case away from the initial test case for missing\n commits as to prepare for 3/3 part of the patch. It is something that\n was advised by Eric Sunshine when I checked both missing and\n duplicated commits, but that I removed it when removing the checking\n for duplicated commits since there was only one test. However I add\n it again since 3/3 will add more checking.\n I use the variable raiseError that I affect if the error must be\n raised instead of testing directly because I think it makes more\n sense with 3/3 and if we add other check in the future since it adds\n more possible errors (the test for the error case if not something\n like 'if (test checkLevel = error && test -s todo.miss) || test cond2\n || test cond3 || ...').\n I am wondering if a check_todo_list call should be added in the\n '--continue' part of the code: with this patch, the checking is only\n done once, if the user doesn't edit correctly with 'git rebase\n --edit-todo', it won't be picked by this.\n In the tests in t3404 I now also test that the warning/error messages\n are correct.\n I tried to not change this patch too much since it was already\n heavily reviewed, but there are some changes (mostly the ones\n mentionned above).\n\n Documentation/config.txt      | 11 +++++\n Documentation/git-rebase.txt  |  6 +++\n git-rebase--interactive.sh    | 96 +++++++++++++++++++++++++++++++++++++++++++\n t/t3404-rebase-interactive.sh | 66 +++++++++++++++++++++++++++++\n 4 files changed, 179 insertions(+)\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..68a71d0 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -834,6 +834,100 @@ 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\tprintf \"%s\\n\" \"$sha1\"\n+\t\t\t;;\n+\t\tesac\n+\tdone\n+}\n+\n+# Use warn for each line of a file\n+# $1: file\n+warn_file () {\n+\twhile read -r line\n+\tdo\n+\t\twarn \" - $line\"\n+\tdone <\"$1\"\n+}\n+\n+# Check if the user dropped some commits by mistake\n+# Behaviour determined by rebase.missingCommitsCheck.\n+check_todo_list () {\n+\traiseError=f\n+\n+\tcheckLevel=$(git config --get rebase.missingCommitsCheck)\n+\tcheckLevel=${checkLevel:-ignore}\n+\t# Don't be case sensitive\n+\tcheckLevel=$(echo \"$checkLevel\" | tr 'A-Z' 'a-z')\n+\n+\tcase \"$checkLevel\" 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 \"$checkLevel\" = error && raiseError=t\n+\n+\t\t\t# Make the list user-friendly\n+\t\t\topt=\"--no-walk=sorted --format=oneline --abbrev-commit --stdin\"\n+\t\t\tgit rev-list $opt <\"$todo\".miss >\"$todo\".miss+\n+\t\t\tmv \"$todo\".miss+ \"$todo\".miss\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+\t\t\twarn_file \"$todo\".miss\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 $checkLevel for option\" \\\n+\t\t\t\"rebase.missingCommitsCheck. Ignoring.\"\n+\t\t;;\n+\tesac\n+\n+\tif test $raiseError = 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\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n+\t\toutput git checkout $onto || die_abort \"could not detach HEAD\"\n+\t\tgit update-ref ORIG_HEAD $orig_head\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@@ -1079,6 +1173,8 @@ has_action \"$todo\" ||\n \n expand_todo_ids\n \n+check_todo_list\n+\n test -d \"$rewritten\" || test -n \"$force_rebase\" || skip_unnecessary_picks\n \n GIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\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.2.496.gdc9319a\n"},{"id":"263460","messageId":"1433931035-20011-3-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39583","inReplyTo":"1433931035-20011-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"[PATCH/RFCv5 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-10T10:10:35Z","receivedAt":"2015-06-10T10:10:35Z","isPatch":true,"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 git-rebase--interactive.sh    | 63 +++++++++++++++++++++++++++++++++++++++++++\n t/lib-rebase.sh               |  5 ++++\n t/t3404-rebase-interactive.sh | 40 +++++++++++++++++++++++++++\n 3 files changed, 108 insertions(+)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 68a71d0..226a8a8 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -834,6 +834,47 @@ add_exec_commands () {\n \tmv \"$1.new\" \"$1\"\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 command sha1 rest\n+\tdo\n+\t\tcase $command in\n+\t\t''|noop|x|\"exec\")\n+\t\t\t;;\n+\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n+\t\t\tif test -z $sha1\n+\t\t\tthen\n+\t\t\t\techo \"$command $rest\" >>\"$todo\".badsha\n+\t\t\telse\n+\t\t\t\tsha1_verif=\"$(git rev-parse --verify --quiet $sha1^{commit})\"\n+\t\t\t\tif test -z $sha1_verif\n+\t\t\t\tthen\n+\t\t\t\t\techo \"$command $sha1 $rest\" \\\n+\t\t\t\t\t\t>>\"$todo\".badsha\n+\t\t\t\tfi\n+\t\t\tfi\n+\t\t\t;;\n+\t\t*)\n+\t\t\tif test -z $sha1\n+\t\t\tthen\n+\t\t\t\techo \"$command\" >>\"$todo\".badcmd\n+\t\t\telse\n+\t\t\t\tcommit=\"$(git rev-list --oneline -1 --ignore-missing $sha1 2>/dev/null)\"\n+\t\t\t\tif test -z \"$commit\"\n+\t\t\t\tthen\n+\t\t\t\t\techo \"$command $sha1 $rest\" \\\n+\t\t\t\t\t\t>>\"$todo\".badcmd\n+\t\t\t\telse\n+\t\t\t\t\techo \"$command $commit\" >>\"$todo\".badcmd\n+\t\t\t\tfi\n+\t\t\tfi\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@@ -913,6 +954,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\traiseError=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_file \"$todo\".badsha\n+\t\twarn\n+\tfi\n+\n+\tif test -s \"$todo\".badcmd\n+\tthen\n+\t\traiseError=t\n+\n+\t\twarn \"Warning: the command isn't recognized\" \\\n+\t\t\t\"in the following line(s):\"\n+\t\twarn_file \"$todo\".badcmd\n+\t\twarn\n+\tfi\n+\n \tif test $raiseError = 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.2.496.gdc9319a\n"},{"id":"263470","messageId":"vpqvbevty1f.fsf@anie.imag.fr","threadId":"39583","inReplyTo":"1433931035-20011-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv5 2/3] git rebase -i: warn about removed commits","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-10T14:53:32Z","receivedAt":"2015-06-10T14:53:32Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n\n> Check if commits were removed (i.e. a line was deleted) and print\n> warnings or stop git rebase depending on the value of the\n> configuration variable rebase.missingCommitsCheck.\n>\n> This patch gives the user the possibility to avoid silent loss of\n> information (losing a commit through deleting the line in this case)\n> if he wants.\n>\n> Add 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>\n> rebase.missingCommitsCheck defaults to \"ignore\".\n>\n> Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>\n> ---\n>  In git-rebase--interactive, in the error case of check_todo_list, I\n>  added 'git checkout $onto' so that using 'die' for the error allows\n>  to use 'git rebase --edit-todo' (otherwise HEAD would not have been\n>  changed and it still would be placed after the commits of the\n>  rebase).\n>  Since now it doesn't abort the rebase, the documentation and the\n>  messages in the case error have changed.\n>  I moved the error case away from the initial test case for missing\n>  commits as to prepare for 3/3 part of the patch. It is something that\n>  was advised by Eric Sunshine when I checked both missing and\n>  duplicated commits, but that I removed it when removing the checking\n>  for duplicated commits since there was only one test. However I add\n>  it again since 3/3 will add more checking.\n>  I use the variable raiseError that I affect if the error must be\n>  raised instead of testing directly because I think it makes more\n>  sense with 3/3 and if we add other check in the future since it adds\n>  more possible errors (the test for the error case if not something\n>  like 'if (test checkLevel = error && test -s todo.miss) || test cond2\n>  || test cond3 || ...').\n>  I am wondering if a check_todo_list call should be added in the\n>  '--continue' part of the code: with this patch, the checking is only\n>  done once, if the user doesn't edit correctly with 'git rebase\n>  --edit-todo', it won't be picked by this.\n>  In the tests in t3404 I now also test that the warning/error messages\n>  are correct.\n>  I tried to not change this patch too much since it was already\n>  heavily reviewed, but there are some changes (mostly the ones\n>  mentionned above).\n>\n>  Documentation/config.txt      | 11 +++++\n>  Documentation/git-rebase.txt  |  6 +++\n>  git-rebase--interactive.sh    | 96 +++++++++++++++++++++++++++++++++++++++++++\n>  t/t3404-rebase-interactive.sh | 66 +++++++++++++++++++++++++++++\n>  4 files changed, 179 insertions(+)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 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\n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 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>::\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 72abf90..68a71d0 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -834,6 +834,100 @@ 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\tprintf \"%s\\n\" \"$sha1\"\n> +\t\t\t;;\n> +\t\tesac\n> +\tdone\n> +}\n> +\n> +# Use warn for each line of a file\n> +# $1: file\n> +warn_file () {\n> +\twhile read -r line\n> +\tdo\n> +\t\twarn \" - $line\"\n> +\tdone <\"$1\"\n> +}\n> +\n> +# Check if the user dropped some commits by mistake\n> +# Behaviour determined by rebase.missingCommitsCheck.\n> +check_todo_list () {\n> +\traiseError=f\n> +\n> +\tcheckLevel=$(git config --get rebase.missingCommitsCheck)\n> +\tcheckLevel=${checkLevel:-ignore}\n> +\t# Don't be case sensitive\n> +\tcheckLevel=$(echo \"$checkLevel\" | tr 'A-Z' 'a-z')\n\nAvoid echo on user-supplied data. If $checkLevel starts with a \"-\", the\nbehavior is platform-dependant. Should not happen if the user is\nsensible, but using\n\n  printf '%s' \"$checkLevel\"\n\nis safer.\n\n> +\t\t\topt=\"--no-walk=sorted --format=oneline --abbrev-commit --stdin\"\n> +\t\t\tgit rev-list $opt <\"$todo\".miss >\"$todo\".miss+\n> +\t\t\tmv \"$todo\".miss+ \"$todo\".miss\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> +\t\t\twarn_file \"$todo\".miss\n\nI would find it more elegant with less intermediate files, like\n\ngit rev-list $opt <\"$todo\".miss | while read -r line\ndo\n\twarn \" - $line\"\ndone\n\n> +\tif test $raiseError = 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\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n> +\t\toutput git checkout $onto || die_abort \"could not detach HEAD\"\n> +\t\tgit update-ref ORIG_HEAD $orig_head\n\nThis is cut-and-pasted from below in the same file. It would deserve a\nhelper function I think.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"263478","messageId":"vpq8ubrtws0.fsf@anie.imag.fr","threadId":"39583","inReplyTo":"1433931035-20011-3-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv5 3/3] git rebase -i: add static check for commands and SHA-1","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-10T15:20:47Z","receivedAt":"2015-06-10T15:20:47Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n\n> +# from the todolist in stdin\n> +check_bad_cmd_and_sha () {\n> +\tgit stripspace --strip-comments |\n> +\twhile read -r command sha1 rest\n> +\tdo\n> +\t\tcase $command in\n> +\t\t''|noop|x|\"exec\")\n> +\t\t\t;;\n> +\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n> +\t\t\tif test -z $sha1\n> +\t\t\tthen\n> +\t\t\t\techo \"$command $rest\" >>\"$todo\".badsha\n> +\t\t\telse\n> +\t\t\t\tsha1_verif=\"$(git rev-parse --verify --quiet $sha1^{commit})\"\n> +\t\t\t\tif test -z $sha1_verif\n> +\t\t\t\tthen\n> +\t\t\t\t\techo \"$command $sha1 $rest\" \\\n> +\t\t\t\t\t\t>>\"$todo\".badsha\n\nWhen you reach the right end of your screen because of indentation,\ncutting lines with \\ is rarely the best option. Having 5 levels of\nindentation is a sign that you should make more functions.\n\nHow about:\n\ncheck_bad_cmd_and_sha () {\n\tgit stripspace --strip-comments |\n\twhile read -r command sha1 rest\n\tdo\n\t\tcase $command in\n\t\t''|noop|x|\"exec\")\n\t\t\t;;\n\t\tpick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n\t\t\tcheck_commit_id $sha1\n\t\t\t;;\n\t\t*)\n\t\t\trecord_bad_command $sha1\n\t\t\t;;\n\tesac\n}\n\n?\n\n> +\t\t*)\n> +\t\t\tif test -z $sha1\n> +\t\t\tthen\n> +\t\t\t\techo \"$command\" >>\"$todo\".badcmd\n\nAvoid echo on user-supplied data.\n\n> +\t\t\telse\n> +\t\t\t\tcommit=\"$(git rev-list --oneline -1 --ignore-missing $sha1 2>/dev/null)\"\n> +\t\t\t\tif test -z \"$commit\"\n> +\t\t\t\tthen\n> +\t\t\t\t\techo \"$command $sha1 $rest\" \\\n> +\t\t\t\t\t\t>>\"$todo\".badcmd\n> +\t\t\t\telse\n> +\t\t\t\t\techo \"$command $commit\" >>\"$todo\".badcmd\n> +\t\t\t\tfi\n> +\t\t\tfi\n\nWhat are you trying to do here? It seems that you are trying to recover\nthe line, but the line is your input, you shouldn't have to recompute\nit.\n\nWhy isn't printf '%s %s %s' \"$command\" \"$sha1\" \"$rest\" sufficient in all\ncases?\n\nMaybe it would be better to read line by line (to avoid loosing\nwhitespace information for example), like\n\n\twhile read -r line\n\tdo\n\t\tprintf '%s' \"$line\" | read -r cmd sha1 rest\n\t\tcase $sha1 in\n\t\t\t...\n\nor maybe it's overkill.\n\n> +\tcheck_bad_cmd_and_sha <\"$todo\"\n> +\n> +\tif test -s \"$todo\".badsha\n> +\tthen\n> +\t\traiseError=t\n\nWe usually don't use camelCase in shell-scripts. raise_error would be\nthe usual way to spell in in Git's codebase.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"263487","messageId":"1634653813.334490.1433951239962.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39583","inReplyTo":"vpqvbevty1f.fsf@anie.imag.fr","subject":"Re: [PATCH/RFCv5 2/3] git rebase -i: warn about removed commits","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-10T15:47:19Z","receivedAt":"2015-06-10T15:47:19Z","isPatch":true,"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> > +                        warn_file \"$todo\".miss\n> \n> I would find it more elegant with less intermediate files, like\n> \n> git rev-list $opt <\"$todo\".miss | while read -r line\n> do\n>         warn \" - $line\"\n> done\n\nI am not really sure since I also use warn_file to display the bad\ncommands and SHA-1 in 3/3.\n\nNoted for the other points.\n\nThanks,\nRémi\n"},{"id":"263489","messageId":"vpqlhfr4kxn.fsf@anie.imag.fr","threadId":"39583","inReplyTo":"1634653813.334490.1433951239962.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv5 2/3] git rebase -i: warn about removed commits","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-10T15:55:48Z","receivedAt":"2015-06-10T15:55:48Z","isPatch":true,"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> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n>> > +                        warn_file \"$todo\".miss\n>> \n>> I would find it more elegant with less intermediate files, like\n>> \n>> git rev-list $opt <\"$todo\".miss | while read -r line\n>> do\n>>         warn \" - $line\"\n>> done\n>\n> I am not really sure since I also use warn_file to display the bad\n> commands and SHA-1 in 3/3.\n\nI noticed this later indeed. But had the function been eg. warn_pipe,\nyou could have written\n\ngit rev-list $opt <\"$todo\".miss | warn_pipe\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"263488","messageId":"58099623.334723.1433951804504.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39583","inReplyTo":"vpq8ubrtws0.fsf@anie.imag.fr","subject":"Re: [PATCH/RFCv5 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-10T15:56:44Z","receivedAt":"2015-06-10T15:56:44Z","isPatch":true,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"> > +        git stripspace --strip-comments |\n> > +        while read -r command sha1 rest\n> > +        do\n> > +                case $command in\n> > +                ''|noop|x|\"exec\")\n> > +                        ;;\n> > +                pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)\n> > +                        if test -z $sha1\n> > +                        then\n> > +                                echo \"$command $rest\" >>\"$todo\".badsha\n> > +                        else\n> > +                                sha1_verif=\"$(git rev-parse --verify --quiet $sha1^{commit})\"\n> > +                                if test -z $sha1_verif\n> > +                                then\n> > +                                        echo \"$command $sha1 $rest\" \\\n> > +                                                >>\"$todo\".badsha\n> \n> When you reach the right end of your screen because of indentation,\n> cutting lines with \\ is rarely the best option. Having 5 levels of\n> indentation is a sign that you should make more functions.\n\nYeah, I wasn't overly happy with that either, I will try to add some\nfunctions (your example seems like a good way of refactoring).\n\n> > +                                commit=\"$(git rev-list --oneline -1 --ignore-missing $sha1 2>/dev/null)\"\n> > +                                if test -z \"$commit\"\n> > +                                then\n> > +                                        echo \"$command $sha1 $rest\" \\\n> > +                                                >>\"$todo\".badcmd\n> > +                                else\n> > +                                        echo \"$command $commit\" >>\"$todo\".badcmd\n> > +                                fi\n> > +                        fi\n> \n> What are you trying to do here? It seems that you are trying to recover\n> the line, but the line is your input, you shouldn't have to recompute\n> it.\n> \n> Why isn't printf '%s %s %s' \"$command\" \"$sha1\" \"$rest\" sufficient in all\n> cases?\n\nIt is mainly because here the SHA-1 is a long one (40 chars), however\nI agree that computing short_sha1 and then printf '%s %s %s'\n\"$command\" \"$short_sha1\" \"$rest\" should be good in this case.\n\n> Maybe it would be better to read line by line (to avoid loosing\n> whitespace information for example), like\n> \n>         while read -r line\n>         do\n>                 printf '%s' \"$line\" | read -r cmd sha1 rest\n>                 case $sha1 in\n>                         ...\n> \n> or maybe it's overkill.\n\nCould be a good idea, though I am not completely convinced about it\nyet.\n\nNoted for the other points.\n\nThanks,\nRémi\n"},{"id":"263490","messageId":"868859991.334769.1433951969453.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39583","inReplyTo":"vpqlhfr4kxn.fsf@anie.imag.fr","subject":"Re: [PATCH/RFCv5 2/3] git rebase -i: warn about removed commits","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-10T15:59:29Z","receivedAt":"2015-06-10T15:59:29Z","isPatch":true,"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> > Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n> >> > +                        warn_file \"$todo\".miss\n> >>\n> >> I would find it more elegant with less intermediate files, like\n> >>\n> >> git rev-list $opt <\"$todo\".miss | while read -r line\n> >> do\n> >>         warn \" - $line\"\n> >> done\n> >\n> > I am not really sure since I also use warn_file to display the bad\n> > commands and SHA-1 in 3/3.\n> \n> I noticed this later indeed. But had the function been eg. warn_pipe,\n> you could have written\n> \n> git rev-list $opt <\"$todo\".miss | warn_pipe\n\nInteresting, I'll try to do something similar to this.\n\nRémi\n"},{"id":"263492","messageId":"vpqa8w71r80.fsf@anie.imag.fr","threadId":"39583","inReplyTo":"58099623.334723.1433951804504.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv5 3/3] git rebase -i: add static check for commands and SHA-1","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-10T16:08:15Z","receivedAt":"2015-06-10T16:08:15Z","isPatch":true,"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> It is mainly because here the SHA-1 is a long one (40 chars)\n\nOK, but then the minimum would be to add a comment saying that.\n\nNow, this makes me wonder why you are doing the check after the sha1\nexpansion and not before. Also, when running `git bisect --edit-todo`, I\ndo get the short sha1. So, there's a piece of code doing what you want\nsomewhere already. You may want to use it.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"263777","messageId":"775816946.447663.1434237425837.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39583","inReplyTo":"vpqa8w71r80.fsf@anie.imag.fr","subject":"Re: [PATCH/RFCv5 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-13T23:17:05Z","receivedAt":"2015-06-13T23:17:05Z","isPatch":true,"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> > It is mainly because here the SHA-1 is a long one (40 chars)\n> \n> OK, but then the minimum would be to add a comment saying that.\n> \n> Now, this makes me wonder why you are doing the check after the sha1\n> expansion and not before. Also, when running `git bisect --edit-todo`, I\n> do get the short sha1. So, there's a piece of code doing what you want\n> somewhere already. You may want to use it.\n\nOriginally I did the whole checking after the expansion because I\nthough that it was a better idea to avoid doing it myself (Comparing\nthe whole SHA-1 instead of partial ones to find missing ones made more\nsense for me since otherwise I would have to check if one is the\nprefix of the other or expand to the same size before comparing).\n\nHowever I agree that adding a comment would make things clearer. Will\nprobably do that.\n\nThank you,\nRémi\n"},{"id":"263837","messageId":"vpqzj4174zb.fsf@anie.imag.fr","threadId":"39583","inReplyTo":"775816946.447663.1434237425837.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv5 3/3] git rebase -i: add static check for commands and SHA-1","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-15T08:25:44Z","receivedAt":"2015-06-15T08:25:44Z","isPatch":true,"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> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n>> Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n>> \n>> > It is mainly because here the SHA-1 is a long one (40 chars)\n>> \n>> OK, but then the minimum would be to add a comment saying that.\n>> \n>> Now, this makes me wonder why you are doing the check after the sha1\n>> expansion and not before. Also, when running `git bisect --edit-todo`, I\n>> do get the short sha1. So, there's a piece of code doing what you want\n>> somewhere already. You may want to use it.\n>\n> Originally I did the whole checking after the expansion because I\n> though that it was a better idea to avoid doing it myself (Comparing\n> the whole SHA-1 instead of partial ones to find missing ones made more\n> sense for me since otherwise I would have to check if one is the\n> prefix of the other or expand to the same size before comparing).\n\nChecking the missing commits after expansion makes sense (but it is only\na matter of adding \"| git rev-list --no-walk --stdin\" somewhere in the\npipeline).\n\nBut IMHO, checking the syntax errors is better done as early as possible\nif you want accurate error messages. This way you still have what the\nuser typed available.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}