{"thread":{"id":"39513","subject":"[PATCH/RFCv4 1/2] git-rebase -i: add command \"drop\" to remove a commit","startedAt":"2015-06-03T11:44:18Z","lastAt":"2015-06-06T23:45:34Z","messageCount":11,"participants":["Galan Rémi","Remi Galan Alfonso","Matthieu Moy","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"262859","messageId":"1433331859-24832-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39513","inReplyTo":null,"subject":"[PATCH/RFCv4 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-03T11:44:18Z","receivedAt":"2015-06-03T11:44:18Z","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.2.389.geaf7ccf\n"},{"id":"262860","messageId":"1433331859-24832-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39513","inReplyTo":"1433331859-24832-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"[PATCH/RFCv4 2/2] git rebase -i: warn about removed commits","fromName":"Galan Rémi","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-03T11:44:19Z","receivedAt":"2015-06-03T11:44:19Z","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 depending on the value of the\nconfiguration variable rebase.missingCommits.\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 to.\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 aborted.\n\nrebase.missingCommitsCheck defaults to \"ignore\".\n\nSigned-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>\n---\n Documentation/config.txt      | 10 ++++++\n Documentation/git-rebase.txt  |  6 ++++\n git-rebase--interactive.sh    | 82 +++++++++++++++++++++++++++++++++++++++++++\n t/t3404-rebase-interactive.sh | 63 +++++++++++++++++++++++++++++++++\n 4 files changed, 161 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4d21ce1..b29cd8d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2160,6 +2160,16 @@ 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 abort the rebase. 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 9cf3760..6d413a1 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+\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..26804dd 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -851,6 +851,86 @@ 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+\tgit stripspace --strip-comments <\"$1\" | while 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 >\"$2\"\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_commits () {\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# Make the list user-friendly\n+\t\topt=\"--no-walk=sorted --format=oneline --abbrev-commit --stdin\"\n+\t\tgit rev-list $opt <\"$todo\".miss >\"$todo\".miss+\n+\t\tmv \"$todo\".miss+ \"$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 (newer to older):\"\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.missingCommitsCheck 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.missingCommitsCheck.\"\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 +1176,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..f369d2c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1112,4 +1112,67 @@ 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_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>warning &&\n+\ttest D = $(git cat-file commit HEAD | sed -ne \\$p) &&\n+\ttest_cmp warning expect\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+\n+To avoid this message, use \"drop\" to explicitly remove a commit.\n+Use git --config rebase.missingCommitsCheck to change the level of warnings (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_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>warning &&\n+\ttest D = $(git cat-file commit HEAD | sed -ne \\$p) &&\n+\ttest_cmp warning expect\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+\n+To avoid this message, use \"drop\" to explicitly remove a commit.\n+Use git --config rebase.missingCommitsCheck to change the level of warnings (ignore, warn, error).\n+\n+Rebase aborted due to dropped commits.\n+EOF\n+\n+test_expect_success 'rebase -i respects rebase.missingCommitsCheck=error' '\n+\ttest_config rebase.missingCommitsCheck 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 4\" \\\n+\t\tgit rebase -i --root 2>warning &&\n+\ttest E = $(git cat-file commit HEAD | sed -ne \\$p) &&\n+\ttest_cmp warning expect\n+'\n+\n test_done\n-- \n2.4.2.389.geaf7ccf\n"},{"id":"262862","messageId":"1592132582.69681.1433335688516.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39513","inReplyTo":"1433331859-24832-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv4 2/2] git rebase -i: warn about removed commits","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-03T12:48:08Z","receivedAt":"2015-06-03T12:48:08Z","isPatch":true,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n> +                comm -2 -3 \"$todo\".oldsha1 \"$todo\".newsha1 >\"$todo\".miss\n> +\n> +                # Make the list user-friendly\n> +                opt=\"--no-walk=sorted --format=oneline --abbrev-commit --stdin\"\n> +                git rev-list $opt <\"$todo\".miss >\"$todo\".miss+\n> +                mv \"$todo\".miss+ \"$todo\".miss\n> +\n> +                # Check missing commits\n\nFound a bug here, got an error message from git rev-list if\n\"$todo\".miss is empty.\n\nNow it looks like:\n> \t\t# Check missing commits\n> \t\tif test -s \"$todo\".miss\n> \t\tthen\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\nThus the empty case is tested by the test -s of the warnings.\n\nBy the way, should I add --quiet to the options of the call to git\nrev-list?\n\nRémi\n"},{"id":"262889","messageId":"vpq1thsbsko.fsf@anie.imag.fr","threadId":"39513","inReplyTo":"1433331859-24832-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv4 2/2] git rebase -i: warn about removed commits","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-03T17:38:31Z","receivedAt":"2015-06-03T17:38:31Z","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 depending on the value of the\n> configuration variable rebase.missingCommits.\n\nSpelled missingCommitsCheck everywhere else.\n\n> +rebase.missingCommitsCheck::\n> +\tIf set to \"warn\" print warnings about removed commits in\n\nIf set to warn, print (comma). Here and below\n\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\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> +\tgit stripspace --strip-comments <\"$1\" | while read -r command sha1 rest\n\nI'd split the line in two:\n\n\tgit stripspace --strip-comments <\"$1\" |\n\twhile read -r command sha1 rest\n\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 >\"$2\"\n> +}\n\nYou're using $1 and $2 only to redirect input and output. I would find\nit more elegant to write todo_list_to_sha_list as a filter, and do the\nredirection in the call site (to keep the option of using\ntodo_list_to_sha_list in a pipe).\n\n> +check_commits\n\nYou're not really checking commits, but rather the todo-list.\n\n> +Use git --config rebase.missingCommitsCheck to change the level of warnings (ignore, warn, error).\n\nLong line. Please, wrap the message to <80 columns.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262894","messageId":"758899838.77480.1433354006473.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39513","inReplyTo":"vpq1thsbsko.fsf@anie.imag.fr","subject":"Re: [PATCH/RFCv4 2/2] git rebase -i: warn about removed commits","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-03T17:53:26Z","receivedAt":"2015-06-03T17:53:26Z","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> You're using $1 and $2 only to redirect input and output. I would find\n> it more elegant to write todo_list_to_sha_list as a filter, and do the\n> redirection in the call site (to keep the option of using\n> todo_list_to_sha_list in a pipe).\n\nIf I understood correctly, then the calling line would look like:\n> todo_list_to_sha_list <\"$todo\".backup >\"$todo\".oldsha1\n?\n\nShould I do the same for the function warn_file ?\nMeaning\n> warn_file <\"$todo\".miss\ninstead of\n> warn_file \"$todo\".miss\n\nThe other points mentioned have been corrected.\n\nThanks,\nRémi\n"},{"id":"262895","messageId":"xmqq8uc0y8jw.fsf@gitster.dls.corp.google.com","threadId":"39513","inReplyTo":"1433331859-24832-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv4 1/2] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-03T18:02:27Z","receivedAt":"2015-06-03T18:02:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 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\nIs this a sensible piece of advice, though?  The user edited the\nline away and it does not have the commit object name anymore.\nHow does she \"fix\" it in the editor?  The same puzzlement applies\nto the other message.\n\n> +\t\tsha1_verif=\"$(git rev-parse --verify --quiet $sha1^{commit})\"\n> +\t\tif test -z $sha1_verif\n\nAs you are not using the returned string at all, instead of wasting\na shell variable, it would be better to decide to come into this\nblock by using the exit status from rev-parse (it exits with\nnon-zero status upon failure).\n\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"},{"id":"262896","messageId":"vpqd21c8y5j.fsf@anie.imag.fr","threadId":"39513","inReplyTo":"758899838.77480.1433354006473.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv4 2/2] git rebase -i: warn about removed commits","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-03T18:06:16Z","receivedAt":"2015-06-03T18:06:16Z","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>> You're using $1 and $2 only to redirect input and output. I would find\n>> it more elegant to write todo_list_to_sha_list as a filter, and do the\n>> redirection in the call site (to keep the option of using\n>> todo_list_to_sha_list in a pipe).\n>\n> If I understood correctly, then the calling line would look like:\n>> todo_list_to_sha_list <\"$todo\".backup >\"$todo\".oldsha1\n> ?\n\nYes.\n\n> Should I do the same for the function warn_file ?\n> Meaning\n>> warn_file <\"$todo\".miss\n> instead of\n>> warn_file \"$todo\".miss\n\nIf you do so, then warn_file won't be a good name anymore since you'd be\nable to read from a pipe too.\n\nAnyway, it's really a matter of personnal preference. I would keep\nwarn_file as-is and change todo_list_to_sha_list, but feel free to\nignore this point if you disagree.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262906","messageId":"CAPig+cR=xKy-WSZSRgC8UbEbyY+=t7yDZo3O-2GquWXWWPrGVA@mail.gmail.com","threadId":"39513","inReplyTo":"1433331859-24832-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-03T19:14:17Z","receivedAt":"2015-06-03T19:14:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wednesday, June 3, 2015, Galan Rémi\n<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:\n> Check if commits were removed (i.e. a line was deleted) and print\n> warnings or abort git rebase depending on the value of the\n> configuration variable rebase.missingCommits.\n\nA few comments below in addition to those already made by Matthieu...\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 8960083..f369d2c 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1112,4 +1112,67 @@ test_expect_success 'drop' '\n>         test 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> +       test_config rebase.missingCommitsCheck ignore &&\n> +       test_when_finished \"git checkout master &&\n> +               git branch -D tmp2\" &&\n\nStrange indentation.\n\n> +       git checkout -b tmp2 master &&\n> +       set_fake_editor &&\n> +       FAKE_LINES=\"1 2 3 4\" \\\n> +               git rebase -i --root 2>warning &&\n\nThe file containing the actual output is usually spelled \"actual\".\n\n> +       test D = $(git cat-file commit HEAD | sed -ne \\$p) &&\n> +       test_cmp warning expect\n\nThe arguments to test_cmp are usually reversed so that 'expect' comes\nbefore 'actual', which results in a more natural-feeling diff when\ntest_cmp detects that the files differ.\n\nThese comments apply to remaining new tests, as well.\n\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> +\n> +To avoid this message, use \"drop\" to explicitly remove a commit.\n> +Use git --config rebase.missingCommitsCheck to change the level of warnings (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> +       test_config rebase.missingCommitsCheck warn &&\n> +       test_when_finished \"git checkout master &&\n> +               git branch -D tmp2\" &&\n> +       git checkout -b tmp2 master &&\n> +       set_fake_editor &&\n> +       FAKE_LINES=\"1 2 3 4\" \\\n> +               git rebase -i --root 2>warning &&\n> +       test D = $(git cat-file commit HEAD | sed -ne \\$p) &&\n> +       test_cmp warning expect\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> +\n> +To avoid this message, use \"drop\" to explicitly remove a commit.\n> +Use git --config rebase.missingCommitsCheck to change the level of warnings (ignore, warn, error).\n> +\n> +Rebase aborted due to dropped commits.\n> +EOF\n> +\n> +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=error' '\n> +       test_config rebase.missingCommitsCheck error &&\n> +       test_when_finished \"git checkout master &&\n> +               git branch -D tmp2\" &&\n> +       git checkout -b tmp2 master &&\n> +       set_fake_editor &&\n> +       test_must_fail env FAKE_LINES=\"1 2 4\" \\\n> +               git rebase -i --root 2>warning &&\n> +       test E = $(git cat-file commit HEAD | sed -ne \\$p) &&\n> +       test_cmp warning expect\n> +'\n> +\n>  test_done\n> --\n> 2.4.2.389.geaf7ccf\n"},{"id":"262907","messageId":"739894117.78611.1433359427863.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39513","inReplyTo":"xmqq8uc0y8jw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFCv4 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-03T19:23:47Z","receivedAt":"2015-06-03T19:23:47Z","isPatch":true,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> But what if you got this back after the user edits?\n> \n>     drop\n>     pick 2c9c1c5 gostak: distim doshes\n>     drop e3b601d pull: use git-rev-parse...\n>     edit eb2a8d9 pull: handle git-fetch'...\n> \n> [...]\n> Did the user tried to drop something else but the\n> object name has gone missing by mistake?\n\nIf the user tried to drop something but the SHA-1 has gone missing, it\nwould be picked up by 2/2 if rebase.missingCommitsCheck is set to warn\nor error.\n\n> Did the user wanted to drop the first one but made mistake while\n> editing 'pick' away into 'drop'?\n\nI see your point here.\n\n> Noticing and flagging malformed 'drop' lines (or line with any\n> command, for that matter) as such is part of that process to make\n> sure you collected the object names from the \"after\" image\n> correctly, which is the job of 2/2 in your series (if I am reading\n> the description of your series right).\n\nI agree on the fact that noticing and flagging malformed lines for the\nvarious commands is a part of the process to make sure the collected\nobject names are correct.\nHowever, the original job of 2/2 was to avoid the silent loss of\ninformation caused by deleting a line by mistake, not to check the\ncorrectness of the whole todo-file (though I think that it is a good\nidea, to develop in another commit of this patch/in another patch).\nOn the opposite, in some way it could be seen as some loss of information, as your previous example suggests:\n> Did the user wanted to drop the first one but made mistake while\n> editing 'pick' away into 'drop'?\nWe lose the information that the user wanted to drop the line and not\npick it.\n\nAside from that, checking specifically the 'drop' command beforehand\n(that's what happens in 2/2) while not doing any checking on the other\ncommands (or checking on the fly) doesn't really makes sense, I think\nthe commands should be treated equally on the matter of checking.\nIn doing so, checking that the various commands exist, that they are\nfollowed by the correct argument and that their argument is of the\nright type (SHA-1 representing a commit for example), in top of\nchecking that no commit is removed, without forgetting tests to make\nsure everything has the right behaviour, I am afraid that it would\nmake this part of the patch far too long thus why I think it should be\nin another commit/patch.\n\nBy the way\n> In the new world order to punish those who simply remove lines to\n> signal that they want the commits omitted from replaying\n\nAside from liking the wording here, I don't think it can really be\nconsidered as a punishment since it is the user that chooses if he\nwants to be \"punished\" or not; I guess it can be considered BDSM\nthough.\n\nJunio C Hamano <gitster@pobox.com> writes:\n> > +        drop|d)\n> > +                if test -z $sha1\n> > +                then\n> > +                        warn \"Missing SHA-1 in 'drop' command.\"\n> > +                        die \"Please fix this using 'git rebase --edit-todo'.\"\n> \n> Is this a sensible piece of advice, though?  The user edited the\n> line away and it does not have the commit object name anymore.\n> How does she \"fix\" it in the editor?  The same puzzlement applies\n> to the other message.\n\nFor a missing SHA-1, if it is a 'drop' that he forgot to remove after\nchanging his mind, then it can be fixed.\nFor an incorrect SHA-1, maybe he can find it back using git log or\nothers commands of the kind, though I don't think this way of fixing\nthings is user-friendly at all.\nHowever, as you point out, in most cases it will be a line edited\naway, I agree that the \"fix it\" doesn't really help.\n\nThe only alternative I see right now (in 1/2) is aborting the rebase\n(die_abort) though it seems overkill, as the user I wouldn't want to\nlose all of the modifications I have done on the todo-list.\n\nThrough this I see your point about checking in 2/2 since we would\nhave more information (for example if he has an error because a drop\nhas no SHA-1 or the SHA-1 is incorrect and at the same time an error\nbecause a SHA-1 has disappeared, it would make more sense to give him\n\"fix it\" since he would have most of the informations he needs to do\nso). However, right now, I think that my previous points about 2/2\nstill stand.\n\nThanks,\nRémi\n"},{"id":"262913","messageId":"1105867541.79324.1433362150592.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39513","inReplyTo":"CAPig+cR=xKy-WSZSRgC8UbEbyY+=t7yDZo3O-2GquWXWWPrGVA@mail.gmail.com","subject":"Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-03T20:09:10Z","receivedAt":"2015-06-03T20:09:10Z","isPatch":true,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> > +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=ignore' '\n> > +       test_config rebase.missingCommitsCheck ignore &&\n> > +       test_when_finished \"git checkout master &&\n> > +               git branch -D tmp2\" &&\n> \n> Strange indentation.\n\nConsidering that 'git branch -D tmp2' is a part of test_when_finished,\nI wasn't sure of how it was supposed to be indented, so I did it this\nway to show that it was still within test_when_finished and not a\nseparate command.\n> \ttest_when_finished \"git checkout master &&\n> \tgit branch -D tmp2\" &&\nDoesn't seem as clear, especially if you quickly read the lines.\n\nFor now, I have removed the tab.\nAlso the other points have been corrected.\n\nThank you,\nRémi\n"},{"id":"263135","messageId":"876005532.186122.1433634334986.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39513","inReplyTo":"1105867541.79324.1433362150592.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits","fromName":"Remi Galan Alfonso","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-06T23:45:34Z","receivedAt":"2015-06-06T23:45:34Z","isPatch":true,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"I'm going to try to change the die_abort in this patch by a die, so\nthat the user can use rebase --edit-todo afterward. This way, adding\nthe checking on the SHA-1 for the 'drop' command (discussed in 1/2)\n(and also maybe the other commands requiring a correct SHA-1\ncorresponding to a commit) to the 2/2 part would make a bit more\nsense. Though I still see some other issues with this, I agree that it\nmakes more sense in 2/2 rather than in 1/2 (some more checking in a\nfuture patch would be a good idea).\n\n(So far I've tried rather quickly, but it doesn't seem as easy as I\noriginally though, working on it though)\n\nRémi\n"}]}