{"thread":{"id":"39494","subject":"[PATCH/RFCv3 1/2] git-rebase -i: add command \"drop\" to remove a commit","startedAt":"2015-06-02T13:36:19Z","lastAt":"2015-06-03T08:25:04Z","messageCount":10,"participants":["Galan Rémi","Matthieu Moy","Remi Galan Alfonso","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"262711","messageId":"1433252180-25591-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39494","inReplyTo":null,"subject":"[PATCH/RFCv3 1/2] 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-02T13:36:19Z","receivedAt":"2015-06-02T13:36:19Z","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 Documentation/git-rebase.txt  |  3 +++\n git-rebase--interactive.sh    | 18 ++++++++++++++++++\n t/lib-rebase.sh               |  4 ++--\n t/t3404-rebase-interactive.sh | 10 ++++++++++\n 4 files changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 1d01baa..9cf3760 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 its 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..869cc60 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@@ -508,6 +509,23 @@ do_next () {\n \t\"$comment_char\"*|''|noop)\n \t\tmark_action_done\n \t\t;;\n+\tdrop|d)\n+\t\tif test -z $sha1\n+\t\tthen\n+\t\t\twarn \"Missing SHA-1 in 'drop' command.\"\n+\t\t\tdie \"Please fix this using 'git rebase --edit-todo'.\"\n+\t\tfi\n+\n+\t\tsha1_verif=\"$(git rev-parse --verify --quiet $sha1^{commit})\"\n+\t\tif test -z $sha1_verif\n+\t\tthen\n+\t\t\twarn \"'$sha1' is not a SHA-1 or does not represent\" \\\n+\t\t\t\t\"a commit in 'drop' command.\"\n+\t\t\tdie \"Please fix this using 'git rebase --edit-todo'.\"\n+\t\tfi\n+\n+\t\tmark_action_done\n+\t\t;;\n \tpick|p)\n \t\tcomment_for_reflog pick\n \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..8960083 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1102,4 +1102,14 @@ 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_expect_success 'drop' '\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout -b dropBranchTest master &&\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.1.411.g9c4ad60\n"},{"id":"262712","messageId":"1433252180-25591-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39494","inReplyTo":"1433252180-25591-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"[PATCH/RFCv3 2/2] git rebase -i: warn about removed commits","fromName":"Galan Rémi","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-02T13:36:20Z","receivedAt":"2015-06-02T13:36:20Z","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 abort git rebase according to the value of the\nconfiguration variable rebase.missingCommitsCheckLevel.\n\nAdd the configuration variable rebase.missingCommitsCheckLevel.\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 aborted.\n\nrebase.missingCommitsCheckLevel defaults to \"ignore\".\n\nSigned-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>\n---\n Documentation/config.txt      |   7 +++\n Documentation/git-rebase.txt  |   6 +++\n git-rebase--interactive.sh    | 105 ++++++++++++++++++++++++++++++++++++++++++\n t/t3404-rebase-interactive.sh |  25 ++++++++++\n 4 files changed, 143 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4d21ce1..ec9011d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2160,6 +2160,13 @@ rebase.autoStash::\n \tsuccessful rebase might result in non-trivial conflicts.\n \tDefaults to false.\n \n+rebase.missingCommitsCheckLevel::\n+\tIf set to \"warn\", git rebase -i will print a warning if some\n+\tcommits are removed (i.e. a line was deleted), however the\n+\trebase will still proceed. If set to \"error\", it will print\n+\tthe previous warning and abort the rebase. If set to\n+\t\"ignore\", no checking is done.  Defaults 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 9cf3760..71029f8 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.missingCommitsCheckLevel::\n+\tIf set to \"warn\" print warnings about removed commits in\n+\tinteractive mode. If set to \"error\" print the warnings and\n+\tabort 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 869cc60..6391423 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -851,6 +851,109 @@ add_exec_commands () {\n \tmv \"$1.new\" \"$1\"\n }\n \n+# Print the list of the SHA-1 of the commits\n+# from a todo list in a file.\n+# $1: todo-file, $2: outfile\n+todo_list_to_sha_list () {\n+\ttodo_list=$(git stripspace --strip-comments <\"$1\")\n+\ttemp_file=$(mktemp)\n+\techo \"$todo_list\" >$temp_file\n+\twhile read -r command sha1 rest\n+\tdo\n+\t\tcase $command in\n+\t\tx|\"exec\")\n+\t\t\t;;\n+\t\t*)\n+\t\t\tprintf \"%s\\n\" \"$sha1\"\n+\t\t\t;;\n+\t\tesac\n+\tdone <$temp_file >\"$2\"\n+\trm $temp_file\n+}\n+\n+# Transforms SHA-1 list in argument\n+# to a list of commits (in place)\n+# Doesn't check if the SHA-1 are commits.\n+# $1: file with long SHA-1 list\n+long_sha_to_commit_list () {\n+\tshort_missing=\"\"\n+\tgit_command=\"git show --oneline\"\n+\tget_line_command=\"head -n 1\"\n+\ttemp_file=$(mktemp)\n+\twhile read -r sha\n+\tdo\n+\t\tif test -n \"$sha\"\n+\t\tthen\n+\t\t\tcommit=$($git_command $sha | $get_line_command)\n+\t\t\tif test -n \"$commit\"\n+\t\t\tthen\n+\t\t\t\tprintf \"%s\\n\" \"$commit\"\n+\t\t\tfi\n+\t\tfi\n+\tdone <\"$1\" >$temp_file\n+\tmv $temp_file \"$1\"\n+}\n+\n+# Use warn for each line of a file\n+# $1: file to warn\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 .gitconfig.\n+check_commits () {\n+\tcheckLevel=$(git config --get rebase.missingCommitsCheckLevel)\n+\tcheckLevel=${checkLevel:-ignore}\n+\t# To lowercase\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\techo \"$(sort -u \"$todo\".oldsha1)\" >\"$todo\".oldsha1\n+\t\techo \"$(sort -u \"$todo\".newsha1)\" >\"$todo\".newsha1\n+\t\techo \"$(comm -2 -3 \"$todo\".oldsha1 \"$todo\".newsha1)\" >\"$todo\".miss\n+\n+\t\t# Make the list user-friendly\n+\t\tlong_sha_to_commit_list \"$todo\".miss\n+\n+\t\t# Check missing commits\n+\t\tif test -s \"$todo\".miss\n+\t\tthen\n+\t\t\twarn \"Warning : some commits may have been dropped\" \\\n+\t\t\t\t\"accidentally.\"\n+\t\t\twarn \"Dropped commits (in no relevent order):\"\n+\t\t\twarn_file \"$todo\".miss\n+\t\t\twarn \"\"\n+\t\t\twarn \"To avoid this message, use \\\"drop\\\" to\" \\\n+\t\t\t\t\"explicitly remove a commit.\"\n+\t\t\twarn \"Use git --config rebase.missingCommitsCheckLevel to change\" \\\n+\t\t\t\t\"the level of warnings (ignore,warn,error).\"\n+\t\t\twarn \"\"\n+\n+\t\t\tif test \"$checkLevel\" = error\n+\t\t\tthen\n+\t\t\t\tdie_abort \"Rebase aborted due to dropped commits.\"\n+\t\t\tfi\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.missingCommitsCheckLevel in git rebase -i.\"\n+\t\t;;\n+\tesac\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@@ -1096,6 +1199,8 @@ has_action \"$todo\" ||\n \n expand_todo_ids\n \n+check_commits\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 8960083..07a7158 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1112,4 +1112,29 @@ test_expect_success 'drop' '\n \ttest A = $(git cat-file commit HEAD^^ | sed -ne \\$p)\n '\n \n+test_expect_success 'rebase -i respects rebase.missingCommitsCheckLevel=error' '\n+\ttest_config rebase.missingCommitsCheckLevel error &&\n+\ttest_when_finished \"git checkout master &&\n+\t\tgit branch -D tmp2\" &&\n+\tgit checkout -b tmp2 master &&\n+\tset_fake_editor &&\n+\ttest_must_fail env FAKE_LINES=\"1 2 3 4\" \\\n+\t\tgit rebase -i --root &&\n+\ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n+'\n+\n+test_expect_success 'rebase -i respects rebase.missingCommitsCheckLevel=warn' '\n+\ttest_config rebase.missingCommitsCheckLevel warn &&\n+\ttest_when_finished \"git checkout master &&\n+\t\tgit branch -D tmp2\" &&\n+\tgit checkout -b tmp2 master &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"1 2 3 4\" \\\n+\t\tgit rebase -i --root 2>warn.tmp &&\n+\ttest D = $(git cat-file commit HEAD | sed -ne \\$p) &&\n+\tsed -n \"1p\" warn.tmp >warnl1.tmp &&\n+\techo \"Warning : some commits may have been dropped accidentally.\" >l1.tmp &&\n+\ttest_cmp warnl1.tmp l1.tmp\n+'\n+\n test_done\n-- \n2.4.1.411.g9c4ad60\n"},{"id":"262721","messageId":"vpqeglujisv.fsf@anie.imag.fr","threadId":"39494","inReplyTo":"1433252180-25591-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv3 1/2] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-02T14:17:52Z","receivedAt":"2015-06-02T14:17:52Z","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> +test_expect_success 'drop' '\n\nPlease, be more descriptive in the first argument of\ntest_expect_success. It's usually a good thing to say not only what the\ntest stresses but also what the expected behavior is.\n\ntest_expect_success 'drop actually drops the lines' '\n\t...\n'\n\n?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262731","messageId":"49957049.34247.1433255547565.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39494","inReplyTo":"vpqeglujisv.fsf@anie.imag.fr","subject":"Re: [PATCH/RFCv3 1/2] 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-02T14:32:27Z","receivedAt":"2015-06-02T14:32:27Z","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> > +test_expect_success 'drop' '\n> \n> Please, be more descriptive in the first argument of\n> test_expect_success. It's usually a good thing to say not only what the\n> test stresses but also what the expected behavior is.\n> \n> test_expect_success 'drop actually drops the lines' '\n\nFor this one I've based myself on the first argument of some other\ntests that tests commands for git rebase -i :\n> t3404-rebase-interactive.sh:240\n> test_expect_success 'squash' '\n> t3404-rebase-interactive.sh:679\n> test_expect_success 'reword' '\n\nRémi\n"},{"id":"262734","messageId":"vpqzj4ignwe.fsf@anie.imag.fr","threadId":"39494","inReplyTo":"1433252180-25591-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-02T14:56:01Z","receivedAt":"2015-06-02T14:56:01Z","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 abort git rebase according to the value of the\n\ns/according to/depending on/\n\n(although both translate to the same \"selon\" in french ;-))\n\n> configuration variable rebase.missingCommitsCheckLevel.\n\nNow I'm wondering whether rebase.missingCommits would be sufficient. The\nfull config would be\n\n[rebase]\n\tmissingCommits = warn\n\nwhich reads like \"in rebase, on missing commit, warn me\".\n\n> Add the configuration variable rebase.missingCommitsCheckLevel.\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 aborted.\n>\n> rebase.missingCommitsCheckLevel defaults to \"ignore\".\n\nThis all describes well *what* the commit is doing (which is essentially\nrendundant with the documentation actually), but fails to really explain\n*why*, which is the most important question to adress in a commit\nmessage.\n\nI'm convinced that this is a good idea (partly because I'm the one who\nsuggested ^^), but not everybody is.\n\n> +rebase.missingCommitsCheckLevel::\n> +\tIf set to \"warn\", git rebase -i will print a warning if some\n> +\tcommits are removed (i.e. a line was deleted), however the\n> +\trebase will still proceed. If set to \"error\", it will print\n> +\tthe previous warning and abort the rebase. If set to\n> +\t\"ignore\", no checking is done.  Defaults to \"ignore\".\n\nI think the relationship with 'drop' should be clarified here.\n\n\"To drop a commit without warning or error, use the `drop` command in the\ntodo-list.\"\n\n?\n\n> +# Print the list of the SHA-1 of the commits\n> +# from a todo list in a file.\n> +# $1: todo-file, $2: outfile\n> +todo_list_to_sha_list () {\n> +\ttodo_list=$(git stripspace --strip-comments <\"$1\")\n> +\ttemp_file=$(mktemp)\n\nc5770f7 (contrib/diffall: create tmp dirs without mktemp, 2012-03-14)\nsays \"mktemp is not available on all platforms.\" ...\n\n> +\techo \"$todo_list\" >$temp_file\n\nDon't use echo on user-supplied data. It's not portable (think what\nhappens if $todo_list starts with a -).\n\nprintf '%s' \"$todo_list\"\n\nYou don't need all these intermediate variables/files. It looks like you\nwant\n\ngit stripspace --strip-comments | while read -r command sha1 rest\ndo\n...\n\n> +# Transforms SHA-1 list in argument\n> +# to a list of commits (in place)\n> +# Doesn't check if the SHA-1 are commits.\n\ns/if/whether/\n\n> +# $1: file with long SHA-1 list\n> +long_sha_to_commit_list () {\n> +\tshort_missing=\"\"\n> +\tgit_command=\"git show --oneline\"\n> +\tget_line_command=\"head -n 1\"\n> +\ttemp_file=$(mktemp)\n> +\twhile read -r sha\n> +\tdo\n> +\t\tif test -n \"$sha\"\n> +\t\tthen\n> +\t\t\tcommit=$($git_command $sha | $get_line_command)\n\nWhy not git show --oneline $sha without using this $git_command?\n\nTo me\n\n  git show --oneline $sha | head -n 1\n\nsays all that has to be said, while\n\n  $git_command $sha | $get_line_command\n\ndoes not say anything (although it uses more characters).\n\nBut actually, computing the diff with `git show` and eliminating it with\n`head` seems backwards.\n\nI guess you want something like:\n\n  git rev-list --pretty=oneline -1 $sha\n\nAnd the whole long_sha_to_commit_list is more or less equivalent to\n\n  git rev-list --stdin --no-walk --format=oneline --abbrev-commit\n\n(Yes, git was _meant_ to be scriptable, and it really is)\n\n> +# Use warn for each line of a file\n> +# $1: file to warn\n\nI don't parse \"file to warn\". I'd rather warn the user than a file ;-).\n\n> +# Check if the user dropped some commits by mistake\n> +# Behaviour determined by .gitconfig.\n\nnot necessarily .gitconfig, there are other names. Say\nrebase.missingCommitsCheckLevel if you want to be technically accurate.\n\n> +check_commits () {\n> +\tcheckLevel=$(git config --get rebase.missingCommitsCheckLevel)\n> +\tcheckLevel=${checkLevel:-ignore}\n> +\t# To lowercase\n> +\tcheckLevel=$(echo \"$checkLevel\" | tr 'A-Z' 'a-z')\n\nGood comments insist on _why_ not _what_. I'd write:\n\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\techo \"$(sort -u \"$todo\".oldsha1)\" >\"$todo\".oldsha1\n> +\t\techo \"$(sort -u \"$todo\".newsha1)\" >\"$todo\".newsha1\n\nUseless uses of echo.\n\necho $(foo) -> foo\n\n> +\t\t\twarn \"Warning : some commits may have been dropped\" \\\n\nNo space before : in english (hmm, didn't I already notice this one?)\n\n> +\t\t\twarn \"Use git --config rebase.missingCommitsCheckLevel to change\" \\\n> +\t\t\t\t\"the level of warnings (ignore,warn,error).\"\n\nSpaces after commas, or (ignore/warn/error).\n\n> +\t\twarn \"Unrecognized setting $checkLevel for option\" \\\n> +\t\t\t\"rebase.missingCommitsCheckLevel in git rebase -i.\"\n\nI'd drop the \"in git rebase -i\" part. The user may have run \"git rebase\n--interactive\" and not know -i, and normally already knows which command\nis executed.\n\n> +test_expect_success 'rebase -i respects rebase.missingCommitsCheckLevel=error' '\n> +\ttest_config rebase.missingCommitsCheckLevel error &&\n> +\ttest_when_finished \"git checkout master &&\n> +\t\tgit branch -D tmp2\" &&\n> +\tgit checkout -b tmp2 master &&\n> +\tset_fake_editor &&\n> +\ttest_must_fail env FAKE_LINES=\"1 2 3 4\" \\\n> +\t\tgit rebase -i --root &&\n> +\ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n> +'\n\nYou should test also that\n\ngit rebase --edit-todo # playing with $EDITOR to restore the original lines.\ngit rebase --continue\n\nactually continues. You did have problems with this in early\nimplementations, so it's not straightforward, so it deserves a test.\n\nYou should check the output of git rebase -i too.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262742","messageId":"133653833.39653.1433262736640.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39494","inReplyTo":"vpqzj4ignwe.fsf@anie.imag.fr","subject":"Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-02T16:32:16Z","receivedAt":"2015-06-02T16:32:16Z","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> > Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n> > +                # Sort the SHA-1 and compare them\n> > +                echo \"$(sort -u \"$todo\".oldsha1)\" >\"$todo\".oldsha1\n> > +                echo \"$(sort -u \"$todo\".newsha1)\" >\"$todo\".newsha1\n> \n> Useless uses of echo.\n> \n> echo $(foo) -> foo\n\nIn this case it is not true, because of the infile and outfile being\nidentical. However sort does have a -o (-output) that I missed that\nallows avoiding using echo or writing in another file; I'm correcting\nwith this.\n\n> You should test also that\n> \n> git rebase --edit-todo # playing with $EDITOR to restore the original lines.\n> git rebase --continue\n> \n> actually continues. You did have problems with this in early\n> implementations, so it's not straightforward, so it deserves a test.\n\nIn this patch, the error is dealt with a die_abort and not die, thus\nthere is no rebase --edit-todo or rebase --continue possible\nafterward.\n\n> You should check the output of git rebase -i too.\n\nChecking that the warning was correctly displayed like in the test for\n\"warn\" if I understood correctly. About that, is checking that the\nfirst line is \"Warning: some commits may have been dropped\naccidentally.\" (like in the test for \"warn\") enough, or should I check\nthat the commit displayed as removed is the correct one?\n\nThe other points have been corrected, or their correction is in\nprogress.\n\nThanks,\n\nRémi\n"},{"id":"262759","messageId":"xmqqoaky2co4.fsf@gitster.dls.corp.google.com","threadId":"39494","inReplyTo":"133653833.39653.1433262736640.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-02T18:22:19Z","receivedAt":"2015-06-02T18:22:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>\nwrites:\n\n> In this case it is not true, because of the infile and outfile being\n> identical. However sort does have a -o (-output) that I missed that\n> allows avoiding using echo or writing in another file; I'm correcting\n> with this.\n\nEven though it is in POSIX, some implementation may lack it, so our\ncode tend to avoid \"sort -o\".  \"echo $(sort)\" is also ugly and\ninefficient.  The obvious and old-fashioned would be sufficient here:\n\n\tsort -u \"$todo\".oldsha1 >\"$todo\".oldsha1+ &&\n        mv \"$todo\".oldsha1+ \"$todo\".oldsha1\n"},{"id":"262816","messageId":"vpqiob5fg24.fsf@anie.imag.fr","threadId":"39494","inReplyTo":"133653833.39653.1433262736640.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-03T06:42:59Z","receivedAt":"2015-06-03T06:42:59Z","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> Checking that the warning was correctly displayed like in the test for\n> \"warn\" if I understood correctly. About that, is checking that the\n> first line is \"Warning: some commits may have been dropped\n> accidentally.\" (like in the test for \"warn\") enough, or should I check\n> that the commit displayed as removed is the correct one?\n\nIdeally, you would check the list of commits displayed too. If the\ncommits sha1 are stable, this should be easy to do. If it's too hard to\ntest, I'd say its not worth the trouble, but others may disagree.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262841","messageId":"921357851.51732.1433316800782.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39494","inReplyTo":"vpqiob5fg24.fsf@anie.imag.fr","subject":"Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-03T07:33:20Z","receivedAt":"2015-06-03T07:33:20Z","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> Ideally, you would check the list of commits displayed too. If the\n> commits sha1 are stable, this should be easy to do. If it's too hard to\n> test, I'd say its not worth the trouble, but others may disagree.\n\nOriginally I chose not to check if the SHA-1 were corrects since\ncheck_commits was called right after expand_todo_ids and I thought\nthat expand_todo_ids checked them, but from what I understand, it\ndoesn't seem to check if the SHA-1 are commits, I could be wrong\nthough.\n\nRémi\n"},{"id":"262846","messageId":"276406458.54566.1433319904220.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39494","inReplyTo":"921357851.51732.1433316800782.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-03T08:25:04Z","receivedAt":"2015-06-03T08:25:04Z","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> > Ideally, you would check the list of commits displayed too. If the \n> > commits sha1 are stable, this should be easy to do. If it's too hard to \n> > test, I'd say its not worth the trouble, but others may disagree. \n> \n> Originally I chose not to check if the SHA-1 were corrects since \n> check_commits was called right after expand_todo_ids and I thought \n> that expand_todo_ids checked them, but from what I understand, it \n> doesn't seem to check if the SHA-1 are commits, I could be wrong \n> though. \n\nIgnore this email, I completely misunderstood the email I was\nresponding to. \n(Mailer that doesn't show the quotes by default) \n\nRémi \n"}]}