{"thread":{"id":"39476","subject":"[PATCH/RFCv2 0/2] rebase -i : drop command and removed commits","startedAt":"2015-06-01T09:57:21Z","lastAt":"2015-06-03T17:52:14Z","messageCount":12,"participants":["Galan Rémi","Junio C Hamano","Matthieu Moy","Remi Galan Alfonso"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"262569","messageId":"1433152643-4292-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39476","inReplyTo":null,"subject":"[PATCH/RFCv2 0/2] rebase -i : drop command and removed commits","fromName":"Galan Rémi","fromEmail":"remi.galan-alfonso@ensimag.grenoble-inp.fr","sentAt":"2015-06-01T09:57:21Z","receivedAt":"2015-06-01T09:57:21Z","isPatch":true,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":" - [PATCH 1/2] git-rebase -i: add command \"drop\" to remove a commit\nThe 'drop' command is added as a way to explicitely remove commits in\ngit rebase -i.\nThis patch does not prevent users from commenting the line, removing\nthe line or using the 'noop' command if they want to.\nWhile the 'noop' command has the same effect, it doesn't seem to be\nthe right use for it since 'noop' merely ignores everything that\nfollows, in particular writing 'noop <SHA-1> <text>' can be confusing.\nCommenting a line also has the same effect but the 2/2 part of the\npatch is not made for working correctly with comments (though it still\nworks with 'noop').\nThis command is inspired by the command 'drop' in histedit of\nMercurial (though in the idea, it is not made to make migrants from\nMercurial feel better but rather because it seems like a good idea).\nFor the 'drop' command, maybe instead of just doing the same thing as\nnoop, checking if the SHA-1 that supposedly follow does exist could be\na good idea.\n\n - [PATCH 2/2] git rebase -i: warn about removed commits \nSince the default behaviour is 'ignore', meaning that no checking is\ndone and no warning is displayed, users that are used to remove the\nlines to remove a commit can still do so without changing their\nconfiguration.\nHowever this patch gives the user the possibility to avoid silent loss\nof information if he wants.\nRegarding the comments of v1 : The dupplicated commits are not checked\nanymore ((about the review by Eric Sunshine) thus the error case stays\nwithin the warning test, now that we only have one test for warnings).\nThe list of removed commits is now shown in the format '<short SHA-1>\n<commit-message>' instead of '<full SHA-1>', this was done following\nan idea given by Matthieu Moy outside of the mailing list. \nBecause of this some code have been added and modified and thus need\nreviewing.\nA test has been added to check the correct behaviour of the error\nconfiguration.\n"},{"id":"262571","messageId":"1433152643-4292-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39476","inReplyTo":"1433152643-4292-1-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","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-01T09:57:22Z","receivedAt":"2015-06-01T09:57:22Z","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":"262570","messageId":"1433152643-4292-3-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","threadId":"39476","inReplyTo":"1433152643-4292-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-01T09:57:23Z","receivedAt":"2015-06-01T09:57:23Z","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":"262616","messageId":"xmqqvbf7757q.fsf@gitster.dls.corp.google.com","threadId":"39476","inReplyTo":"1433152643-4292-2-git-send-email-remi.galan-alfonso@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv2 1/2] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T16:39:53Z","receivedAt":"2015-06-01T16:39:53Z","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> Instead of removing a line to remove the commit, you can use the\n> command \"drop\" (just like \"pick\" or \"edit\"). It has the same effect as\n> deleting the line (removing the commit) except that you keep a visual\n> trace of your actions, allowing a better control and reducing the\n> possibility of removing a commit by mistake.\n>\n> Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>\n> ---\n\nMatthieu, is this part of your class project?\n\nI vaguely recall that your school wants your sign-off to release\npatches to us or something like that, and that I saw some other\npatches came with your sign-off, so I am being curious.\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)\n\nIs this sufficient?\n\nIf you are going to do something in 2/2 that relies on the format of\nthis line being correct (as opposed to \"noop\" or \"#\" that can have\nany garbage on the remainder of the line), wouldn't you want to at\nleast check $sha1 is sensible?\n"},{"id":"262619","messageId":"vpqfv6b7403.fsf@anie.imag.fr","threadId":"39476","inReplyTo":"xmqqvbf7757q.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFCv2 1/2] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-01T17:06:04Z","receivedAt":"2015-06-01T17:06:04Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n>\n>> Instead of removing a line to remove the commit, you can use the\n>> command \"drop\" (just like \"pick\" or \"edit\"). It has the same effect as\n>> deleting the line (removing the commit) except that you keep a visual\n>> trace of your actions, allowing a better control and reducing the\n>> possibility of removing a commit by mistake.\n>>\n>> Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>\n>> ---\n>\n> Matthieu, is this part of your class project?\n\nYes it is.\n\n> I vaguely recall that your school wants your sign-off to release\n> patches to us or something like that, and that I saw some other\n> patches came with your sign-off, so I am being curious.\n\nThere's no strict requirement, but since the students are working under\nmy guidance I think it makes sense to have my sign-off. The rule which\napplies by default for students is \"don't distribute your code\"\n(symmetrical to \"don't use other students code\"), which obviously does\nnot apply for this project: I do allow them to contribute their code!\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262627","messageId":"563732680.250935.1433180720935.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39476","inReplyTo":"xmqqvbf7757q.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFCv2 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-01T17:45:20Z","receivedAt":"2015-06-01T17:45:20Z","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> Is this sufficient?\n\n> If you are going to do something in 2/2 that relies on the format of\n> this line being correct (as opposed to \"noop\" or \"#\" that can have\n> any garbage on the remainder of the line), wouldn't you want to at\n> least check $sha1 is sensible?\n\nThat's also something that I was wondering, I wrote about it in the\n0/2 part of this patch, I wanted some opinion about it.\nIf there is no opposition on the subject, I will have it ready for\nthe v3 of the patch.\n\nQuote of that part of the 0/2 for more clarity:\nGalan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:\n> For the 'drop' command, maybe instead of just doing the same thing as\n> noop, checking if the SHA-1 that supposedly follow does exist could be\n> a good idea.\n"},{"id":"262635","messageId":"vpqoakz1dke.fsf@anie.imag.fr","threadId":"39476","inReplyTo":"563732680.250935.1433180720935.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv2 1/2] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-01T18:36:01Z","receivedAt":"2015-06-01T18:36:01Z","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> Junio C Hamano <gitster@pobox.com> writes:\n>> Is this sufficient?\n>\n>> If you are going to do something in 2/2 that relies on the format of\n>> this line being correct (as opposed to \"noop\" or \"#\" that can have\n>> any garbage on the remainder of the line), wouldn't you want to at\n>> least check $sha1 is sensible?\n>\n> That's also something that I was wondering, I wrote about it in the\n> 0/2 part of this patch, I wanted some opinion about it.\n\nIdeally, I think we should do a sanity check before starting the rebase,\nand error out if we encounter an invalid command, a command that should\nbe followed by a valid sha1 and does not, ...\n\nBut currently, we do the verification while applying commands, and I\ndon't think there's anything really helpful to do if we encounter a\n\"drop this-is-not-a-sha1\" command.\n\nSo, IMHO, why not do this check, but I'm not sure it will actually help\nmuch.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262685","messageId":"150689518.4344.1433229813195.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39476","inReplyTo":"vpqoakz1dke.fsf@anie.imag.fr","subject":"Re: [PATCH/RFCv2 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-02T07:23:33Z","receivedAt":"2015-06-02T07:23:33Z","isPatch":true,"sender":{"key":"remi.galan-alfonso@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12509162?v=4"},"body":"> Ideally, I think we should do a sanity check before starting the rebase,\n> and error out if we encounter an invalid command, a command that should\n> be followed by a valid sha1 and does not, ...\n> \n> But currently, we do the verification while applying commands, and I\n> don't think there's anything really helpful to do if we encounter a\n> \"drop this-is-not-a-sha1\" command.\n\nWhile I agree that doing a sanity check before starting the rebase is\na better idea, it would not be logic to do so only for the 'drop'\ncommand, it should be done for all of the various commands (as you\nsaid, checking if the commands are valid, checking if the SHA-1\nfollowing the command is valid if it was expecting one...).\n\nHowever I think that it should be in a different patch, changing the\nwhole checking system of rebase is not directly linked to the idea of\nthe 'drop' command.\n\nRémi\n"},{"id":"262687","messageId":"vpqa8wiwo33.fsf@anie.imag.fr","threadId":"39476","inReplyTo":"150689518.4344.1433229813195.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv2 1/2] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-02T07:45:20Z","receivedAt":"2015-06-02T07:45:20Z","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>> Ideally, I think we should do a sanity check before starting the rebase,\n>> and error out if we encounter an invalid command, a command that should\n>> be followed by a valid sha1 and does not, ...\n>> \n>> But currently, we do the verification while applying commands, and I\n>> don't think there's anything really helpful to do if we encounter a\n>> \"drop this-is-not-a-sha1\" command.\n>\n> While I agree that doing a sanity check before starting the rebase is\n> a better idea, it would not be logic to do so only for the 'drop'\n> command, it should be done for all of the various commands (as you\n> said, checking if the commands are valid, checking if the SHA-1\n> following the command is valid if it was expecting one...).\n\nYes, that's what I had in mind.\n\n> However I think that it should be in a different patch, changing the\n> whole checking system of rebase is not directly linked to the idea of\n> the 'drop' command.\n\nAgreed.\n\nDiscussions on this list often lead to \"Oh, BTW, shall we do XYZ also?\",\nbut you shouldn't take this kind of remark as blocking (as long as XYZ\nis not incompatible with your patch, which is the case here).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"262745","messageId":"xmqqk2vm3udy.fsf@gitster.dls.corp.google.com","threadId":"39476","inReplyTo":"vpqa8wiwo33.fsf@anie.imag.fr","subject":"Re: [PATCH/RFCv2 1/2] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-02T17:14:17Z","receivedAt":"2015-06-02T17:14:17Z","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> Discussions on this list often lead to \"Oh, BTW, shall we do XYZ also?\",\n> but you shouldn't take this kind of remark as blocking (as long as XYZ\n> is not incompatible with your patch, which is the case here).\n\nYeah, thanks for clarification.  As long as what is given to 'drop'\nis checked when it matters (e.g. when the code in patch 2/2 tries\nsee if some commits in the original list are no longer there in\norder to warn sees \"drop foo bar\" where \"foo\" is obviously not an\nobject name in the original list, that should be checked), it is\nfine.  And I agree 1/2 is not the place to do so, even though it may\nbe easier from the implementation point of view (which is why I\nmentioned the possibility in the review of that patch).\n"},{"id":"262853","messageId":"787870724.57987.1433322837954.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39476","inReplyTo":"xmqqk2vm3udy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFCv2 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-03T09:13:57Z","receivedAt":"2015-06-03T09:13:57Z","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> As long as what is given to 'drop' \n> is checked when it matters (e.g. when the code in patch 2/2 tries \n> see if some commits in the original list are no longer there in \n> order to warn sees \"drop foo bar\" where \"foo\" is obviously not an \n> object name in the original list, that should be checked), it is \n> fine. And I agree 1/2 is not the place to do so, even though it may \n> be easier from the implementation point of view (which is why I \n> mentioned the possibility in the review of that patch). \n\nI disagree, I think that that either the checking for the 'drop' \ncommand should either be in the 1/2 where it is introduced or in the \nfunction check_commits introduced by 2/2 but in a separate \ncommit/patch. \nThe 2/2 checks if there are removed commits to have the possibility to \navoid silent loss of information. It is not its role to check if the \nSHA-1 following 'drop' are correct. \n\nWhat I think should be the best for now is checking the SHA-1 \nfollowing 'drop' in 1/2 (or not checking at all) and change the whole \nchecking system of rebase in a later patch (e.g. check beforehand \n(probably in check_commits) if the commands exist, if the SHA-1 are \ncorrect and possibly other things). \n\nRémi \n"},{"id":"262893","messageId":"xmqqd21cy90x.fsf@gitster.dls.corp.google.com","threadId":"39476","inReplyTo":"787870724.57987.1433322837954.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH/RFCv2 1/2] git-rebase -i: add command \"drop\" to remove a commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-03T17:52:14Z","receivedAt":"2015-06-03T17:52:14Z","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> Junio C Hamano <gitster@pobox.com> writes: \n>> As long as what is given to 'drop' \n>> is checked when it matters (e.g. when the code in patch 2/2 tries \n>> see if some commits in the original list are no longer there in \n>> order to warn sees \"drop foo bar\" where \"foo\" is obviously not an \n>> object name in the original list, that should be checked), it is \n>> fine. And I agree 1/2 is not the place to do so, even though it may \n>> be easier from the implementation point of view (which is why I \n>> mentioned the possibility in the review of that patch). \n>\n> I disagree, I think that that either the checking for the 'drop' \n> command should either be in the 1/2 where it is introduced or in the \n> function check_commits introduced by 2/2 but in a separate \n> commit/patch. \n> The 2/2 checks if there are removed commits to have the possibility to \n> avoid silent loss of information. It is not its role to check if the \n> SHA-1 following 'drop' are correct.\n\nSuppose you started from this insn sheet:\n\n    pick 2c9c1c5 gostak: distim doshes\n    pick e3b601d pull: use git-rev-parse...\n    pick eb2a8d9 pull: handle git-fetch'...\n\nand then after letting the user edit, you got this back:\n\n    pick 2c9c1c5 gostak: distim doshes\n    drop e3b601d pull: use git-rev-parse...\n    edit eb2a8d9 pull: handle git-fetch'...\n\nIn the new world order to punish those who simply remove lines to\nsignal that they want the commits omitted from replaying, you would\nwant to see all commit object names that was in the original insn\nsheet appear in the post-edit insn sheet.  I'd presume that the way\nto do so is to collect all the object names from each insn sheet and\ncompute the set difference.  The first one has three commit object\nnames, the same three commit object names appear in the second one,\nand all is well.\n\nBut 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\nAs a part of \"collecting object names from the list before and after\nediting into two separate sets, and computing the set difference in\norder to notice potential mistakes\", you would need to make sure\nthat you got these two sets collected _correctly_, but you do not\nknow from the above sample input what the user wanted to do with the\nfirst line.  Did the user tried to drop something else but the\nobject name has gone missing by mistake?  Did the user wanted to\ndrop the first one but made mistake while editing 'pick' away into\n'drop'?\n\nNoticing and flagging malformed 'drop' lines (or line with any\ncommand, for that matter) as such is part of that process to make\nsure you collected the object names from the \"after\" image\ncorrectly, which is the job of 2/2 in your series (if I am reading\nthe description of your series right).\n\nSo logically I would think 2/2 is where the verification should\nhappen, but doing it as a part of 1/2 may be easier to do.  The end\nresult would not make a difference, and that is why I said it would\nbe OK either way.\n\nI am puzzled as to what you are disagreeing with, and why.\n"}]}