{"thread":{"id":"39478","subject":"[PATCH/RFCv2 1/2] git-rebase -i: add command \"drop\" to remove a commit","startedAt":"2015-06-01T11:52:02Z","lastAt":"2015-06-02T17:17:26Z","messageCount":10,"participants":["Galan Rémi","Eric Sunshine","Matthieu Moy","Remi Galan Alfonso","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"262585","messageId":"1433159523-6596-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39478","inReplyTo":null,"subject":"[PATCH/RFCv2 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-01T11:52:02Z","receivedAt":"2015-06-01T11:52:02Z","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    |  3 ++-\n t/lib-rebase.sh               |  4 ++--\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 4 files changed, 18 insertions(+), 3 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 bab0dcc..2882276 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\" || 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..1bad068 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1102,4 +1102,15 @@ 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+\tgit checkout master &&\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.363.g9535a9c\n"},{"id":"262586","messageId":"1433159523-6596-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39478","inReplyTo":"1433159523-6596-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"[PATCH/RFCv2 2/2] git rebase -i: warn about removed commits","fromName":"Galan Rémi","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-01T11:52:03Z","receivedAt":"2015-06-01T11:52:03Z","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.checkLevel.\n\nAdd the configuration variable rebase.checkLevel.\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.checkLevel defaults to \"ignore\".\n\nSigned-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>\n---\n Documentation/config.txt      |   9 ++++\n Documentation/git-rebase.txt  |   6 +++\n git-rebase--interactive.sh    | 105 ++++++++++++++++++++++++++++++++++++++++++\n t/t3404-rebase-interactive.sh |  10 +++-\n 4 files changed, 129 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 5f76e8c..e2e5554 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2160,6 +2160,15 @@ rebase.autoStash::\n \tsuccessful rebase might result in non-trivial conflicts.\n \tDefaults to false.\n \n+rebase.checkLevel::\n+\tIf set to \"warn\", git rebase -i will print a warning if some\n+\tcommits are removed (i.e. a line was deleted) or if some\n+\tcommits appear more than one time (e.g. the same commit is\n+\tpicked twice), however the rebase will still proceed. If set\n+\tto \"error\", it will print the previous warnings and abort the\n+\trebase. If set to \"ignore\", no checking is done.  Defaults to\n+\t\"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..d348ca2 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.checkLevel::\n+\tIf set to \"warn\" print warnings about removed commits and\n+\tduplicated commits in interactive mode. If set to \"error\"\n+\tprint the warnings and abort the rebase. If set to \"ignore\" no\n+\tchecking is done. \"ignore\" by default.\n+\n OPTIONS\n -------\n --onto <newbase>::\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 2882276..58da6ee 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -837,6 +837,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.checkLevel)\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.checkLevel 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.checkLevel 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@@ -1079,6 +1182,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 1bad068..d3a9ed5 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1103,7 +1103,6 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '\n '\n \n test_expect_success 'drop' '\n-\tgit checkout master &&\n \ttest_when_finished \"git checkout master\" &&\n \tgit checkout -b dropBranchTest master &&\n \tset_fake_editor &&\n@@ -1113,4 +1112,13 @@ 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.checkLevel' '\n+\ttest_config rebase.checkLevel error &&\n+\ttest_when_finished \"git checkout master\" &&\n+\tgit checkout -b tmp2 master &&\n+\tset_fake_editor &&\n+\ttest_must_fail env FAKE_LINES=\"1 2 3 4\" git rebase -i --root &&\n+\ttest E = $(git cat-file commit HEAD | sed -ne \\$p)\n+'\n+\n test_done\n-- \n2.4.1.363.g9535a9c\n"},{"id":"262661","messageId":"CAPig+cRGT4qgCNgVi_x-==GKze7VUPVbVPGHYui7y5ZXie72Qw@mail.gmail.com","threadId":"39478","inReplyTo":"1433159523-6596-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-01T23:16:11Z","receivedAt":"2015-06-01T23:16:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 1, 2015 at 7:52 AM, 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 according to the value of the\n> configuration variable rebase.checkLevel.\n>\n> Add the configuration variable rebase.checkLevel.\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.checkLevel defaults to \"ignore\".\n>\n> Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>\n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 5f76e8c..e2e5554 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2160,6 +2160,15 @@ rebase.autoStash::\n>         successful rebase might result in non-trivial conflicts.\n>         Defaults to false.\n>\n> +rebase.checkLevel::\n> +       If set to \"warn\", git rebase -i will print a warning if some\n> +       commits are removed (i.e. a line was deleted) or if some\n> +       commits appear more than one time (e.g. the same commit is\n> +       picked twice), however the rebase will still proceed. If set\n\nThe cover letter says that v2 no longer checks for a duplicate,\nhowever, this documentation still mentions it.\n\n> +       to \"error\", it will print the previous warnings and abort the\n> +       rebase. If set to \"ignore\", no checking is done.  Defaults to\n> +       \"ignore\".\n> +\n>  receive.advertiseAtomic::\n>         By default, git-receive-pack will advertise the atomic push\n>         capability 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 9cf3760..d348ca2 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -213,6 +213,12 @@ rebase.autoSquash::\n>  rebase.autoStash::\n>         If set to true enable '--autostash' option by default.\n>\n> +rebase.checkLevel::\n> +       If set to \"warn\" print warnings about removed commits and\n> +       duplicated commits in interactive mode. If set to \"error\"\n\nSame here.\n\n> +       print the warnings and abort the rebase. If set to \"ignore\" no\n> +       checking is done. \"ignore\" by default.\n> +\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 1bad068..d3a9ed5 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1103,7 +1103,6 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '\n>  '\n>\n>  test_expect_success 'drop' '\n> -       git checkout master &&\n>         test_when_finished \"git checkout master\" &&\n>         git checkout -b dropBranchTest master &&\n\nThis \"cleanup\" change might deserve its own test (or at least a\nmention in the commit message).\n\n>         set_fake_editor &&\n> @@ -1113,4 +1112,13 @@ test_expect_success 'drop' '\n>         test A = $(git cat-file commit HEAD^^ | sed -ne \\$p)\n>  '\n>\n> +test_expect_success 'rebase -i respects rebase.checkLevel' '\n> +       test_config rebase.checkLevel error &&\n> +       test_when_finished \"git checkout master\" &&\n> +       git checkout -b tmp2 master &&\n> +       set_fake_editor &&\n> +       test_must_fail env FAKE_LINES=\"1 2 3 4\" git rebase -i --root &&\n> +       test E = $(git cat-file commit HEAD | sed -ne \\$p)\n> +'\n\nShouldn't you also explicitly test \"warn\" and \"ignore\" modes?\n"},{"id":"262662","messageId":"CAPig+cRTNGzzGNTZzPCz1M_uHxT6fOBw+k=3njUOkyhUCdX8kg@mail.gmail.com","threadId":"39478","inReplyTo":"CAPig+cRGT4qgCNgVi_x-==GKze7VUPVbVPGHYui7y5ZXie72Qw@mail.gmail.com","subject":"Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-06-01T23:17:30Z","receivedAt":"2015-06-01T23:17:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 1, 2015 at 7:16 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Mon, Jun 1, 2015 at 7:52 AM, Galan Rémi\n> <remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:\n>>  test_expect_success 'drop' '\n>> -       git checkout master &&\n>>         test_when_finished \"git checkout master\" &&\n>>         git checkout -b dropBranchTest master &&\n>\n> This \"cleanup\" change might deserve its own test (or at least a\n> mention in the commit message).\n\n\"...might deserve its own _patch_...\"\n"},{"id":"262684","messageId":"vpq7frmy5nl.fsf@anie.imag.fr","threadId":"39478","inReplyTo":"1433159523-6596-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":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-02T06:40:30Z","receivedAt":"2015-06-02T06:40:30Z","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> configuration variable rebase.checkLevel.\n\ncheckLevel does not seem to be a good name, because it doesn't say\n_what_ is checked. I don't have really good proposal in mind, but maybe\n\nrebase.todoListCheckLevel\nrebase.missinLinesCheckLevel\n\n?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262686","messageId":"53678876.5326.1433230965582.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39478","inReplyTo":"CAPig+cRGT4qgCNgVi_x-==GKze7VUPVbVPGHYui7y5ZXie72Qw@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-02T07:42:45Z","receivedAt":"2015-06-02T07:42:45Z","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> > +rebase.checkLevel::\n> > +       If set to \"warn\", git rebase -i will print a warning if some\n> > +       commits are removed (i.e. a line was deleted) or if some\n> > +       commits appear more than one time (e.g. the same commit is\n> > +       picked twice), however the rebase will still proceed. If set\n> \n> The cover letter says that v2 no longer checks for a duplicate,\n> however, this documentation still mentions it.\n> \n> > +rebase.checkLevel::\n> > +       If set to \"warn\" print warnings about removed commits and\n> > +       duplicated commits in interactive mode. If set to \"error\"\n> \n> Same here.\n\nI forgot to modify the documentation, really sorry about\nthat.\nCorrected here.\n\nEric Sunshine <sunshine@sunshineco.com> writes:\n> >  test_expect_success 'drop' '\n> > -       git checkout master &&\n> >         test_when_finished \"git checkout master\" &&\n> >         git checkout -b dropBranchTest master &&\n> \n> This \"cleanup\" change might deserve its own patch (or at least a\n> mention in the commit message).\n\nI will rather move the this cleanup to the first part of this patch,\nwhere the test was added (it should cause no problem, since the patch\nis still in discussion), it makes more sense. I actually thought that\nwas what I did but seems like I was wrong.\n\nI forgot to double check my patch this time, and it is showing, my\nbad.\n\nEric Sunshine <sunshine@sunshineco.com> writes:\n> > +test_expect_success 'rebase -i respects rebase.checkLevel' '\n> > +       test_config rebase.checkLevel error &&\n> > +       test_when_finished \"git checkout master\" &&\n> > +       git checkout -b tmp2 master &&\n> > +       set_fake_editor &&\n> > +       test_must_fail env FAKE_LINES=\"1 2 3 4\" git rebase -i --root &&\n> > +       test E = $(git cat-file commit HEAD | sed -ne \\$p)\n> > +'\n> \n> Shouldn't you also explicitly test \"warn\" and \"ignore\" modes?\n\nI don't think testing \"ignore\" is really necessary since it\ncorresponds to the default behaviour, it is thus silently tested by\nthe other tests.\nEither way, I will add a test for \"warn\".\n\nRémi\n"},{"id":"262691","messageId":"991806665.10972.1433237028896.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39478","inReplyTo":"vpq7frmy5nl.fsf@anie.imag.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-02T09:23:48Z","receivedAt":"2015-06-02T09:23:48Z","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> \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> > configuration variable rebase.checkLevel.\n> \n> checkLevel does not seem to be a good name, because it doesn't say\n> _what_ is checked. I don't have really good proposal in mind, but maybe\n> \n> rebase.todoListCheckLevel\n> rebase.missinLinesCheckLevel\n> \n> ?\n\nI agree that we could use a better name.\n\nI'm thinking about\n   > rebase.missingLinesCheckLevel\n   or\n   rebase.missingCommitsCheckLevel\nbut I am worried that these names are too long.\nMaybe\n   rebase.dropCheckLevel\n\nI don't really have better propositions right now.\n\nRémi\n"},{"id":"262714","messageId":"CAPig+cQF6irJqYFZ=2YSevjF+ySUn25udYMKSg+VzXP0p0-eAw@mail.gmail.com","threadId":"39478","inReplyTo":"53678876.5326.1433230965582.JavaMail.zimbra@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-02T13:41:18Z","receivedAt":"2015-06-02T13:41:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 2, 2015 at 3:42 AM, Remi Galan Alfonso\n<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>> > +test_expect_success 'rebase -i respects rebase.checkLevel' '\n>> > +       test_config rebase.checkLevel error &&\n>> > +       test_when_finished \"git checkout master\" &&\n>> > +       git checkout -b tmp2 master &&\n>> > +       set_fake_editor &&\n>> > +       test_must_fail env FAKE_LINES=\"1 2 3 4\" git rebase -i --root &&\n>> > +       test E = $(git cat-file commit HEAD | sed -ne \\$p)\n>> > +'\n>>\n>> Shouldn't you also explicitly test \"warn\" and \"ignore\" modes?\n>\n> I don't think testing \"ignore\" is really necessary since it\n> corresponds to the default behaviour, it is thus silently tested by\n> the other tests.\n\nAlthough the underlying behavior of \"ignore\" may be the same as the\ndefault, you also want to check that higher-level machinery for\nrecognizing the \"ignore\" option is working correctly, which is why\nchecking \"ignore\" explicitly is a reasonable thing to do.\n\n> Either way, I will add a test for \"warn\".\n"},{"id":"262715","messageId":"829387582.31753.1433253063381.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39478","inReplyTo":"CAPig+cQF6irJqYFZ=2YSevjF+ySUn25udYMKSg+VzXP0p0-eAw@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-02T13:51:03Z","receivedAt":"2015-06-02T13:51:03Z","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> Although the underlying behavior of \"ignore\" may be the same as the \n> default, you also want to check that higher-level machinery for \n> recognizing the \"ignore\" option is working correctly, which is why \n> checking \"ignore\" explicitly is a reasonable thing to do. \n\nNoted, I'm adding a test for \"ignore\". \n\nThank you, \n\nRémi \n"},{"id":"262746","messageId":"xmqqfv6a3u8p.fsf@gitster.dls.corp.google.com","threadId":"39478","inReplyTo":"vpq7frmy5nl.fsf@anie.imag.fr","subject":"Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-02T17:17:26Z","receivedAt":"2015-06-02T17:17:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> 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>> configuration variable rebase.checkLevel.\n>\n> checkLevel does not seem to be a good name, because it doesn't say\n> _what_ is checked. I don't have really good proposal in mind, but maybe\n>\n> rebase.todoListCheckLevel\n> rebase.missinLinesCheckLevel\n\nYeah, with s/sin/sing/, but the above makes more sense.  It may be\nthat we probably do not even need \"Level\" there in the name.\n"}]}