[PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

Instead of removing a line to remove the commit, you can use the key
word "drop" (just like "pick" or "edit"). It has the same effect as
deleting the line (removing the commit) except that you keep a visual
trace of your actions, allowing a better control and reducing the
possibility of removing a commit by mistake.

Signed-off-by: Galan Rémi <redacted>
---
 Documentation/git-rebase.txt  |  3 +++
 git-rebase--interactive.sh    |  4 ++++
 t/lib-rebase.sh               |  4 ++--
 t/t3404-rebase-interactive.sh | 11 +++++++++++
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1d01baa..3cd2ef2 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt

@@ -514,6 +514,9 @@ rebasing.
 If you just want to edit the commit message for a commit, replace the
 command "pick" with the command "reword".
 
+If you want to remove a commit, replace the command "pick" by the
+command "drop".
+
 If you want to fold two or more commits into one, replace the command
 "pick" for the second and subsequent commits with "squash" or "fixup".
 If the commits had different authors, the folded commit will be

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index dc3133f..cb749e8 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -152,6 +152,7 @@ Commands:
  s, squash = use commit, but meld into previous commit
  f, fixup = like "squash", but discard this commit's log message
  x, exec = run command (the rest of the line) using shell
+ d, drop = remove commit
 
 These lines can be re-ordered; they are executed from top to bottom.
 

@@ -515,6 +516,9 @@ do_next () {
 		do_pick $sha1 "$rest"
 		record_in_rewritten $sha1
 		;;
+	drop|d)
+		mark_action_done
+		;;
 	reword|r)
 		comment_for_reflog reword
 

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 6bd2522..fdbc900 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh

@@ -14,7 +14,7 @@
 #       specified line.
 #
 #   "<cmd> <lineno>" -- add a line with the specified command
-#       ("squash", "fixup", "edit", or "reword") and the SHA1 taken
+#       ("squash", "fixup", "edit", "reword" or "drop") and the SHA1 taken
 #       from the specified line.
 #
 #   "exec_cmd_with_args" -- add an "exec cmd with args" line.

@@ -46,7 +46,7 @@ set_fake_editor () {
 	action=pick
 	for line in $FAKE_LINES; do
 		case $line in
-		squash|fixup|edit|reword)
+		squash|fixup|edit|reword|drop)
 			action="$line";;
 		exec*)
 			echo "$line" | sed 's/_/ /g' >> "$1";;

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ac429a0..1bad068 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh

@@ -1102,4 +1102,15 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
 	test $(git cat-file commit HEAD | sed -ne \$p) = I
 '
 
+test_expect_success 'drop' '
+	git checkout master &&
+	test_when_finished "git checkout master" &&
+	git checkout -b dropBranchTest master &&
+	set_fake_editor &&
+	FAKE_LINES="1 drop 2 3 drop 4 5" git rebase -i --root &&
+	test E = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test C = $(git cat-file commit HEAD^ | sed -ne \$p) &&
+	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
+'
+
 test_done

-- 
2.4.1.174.g28bfe8e

Check if commits were removed (i.e. a line was deleted) or dupplicated
(e.g. the same commit is picked twice), can print warnings or abort
git rebase according to the value of the configuration variable
rebase.checkLevel.

Add the configuration variable rebase.checkLevel.
    - When unset or set to "IGNORED", no checking is done.
    - When set to "WARN", the commits are checked, warnings are
      displayed but git rebase still proceeds.
    - When set to "ERROR", the commits are checked, warnings are
      displayed and the rebase is aborted.

Signed-off-by: Galan Rémi <redacted>
---
 This part of the patch has no test yet, it is more for rfc.

 Documentation/config.txt     |  8 +++++
 Documentation/git-rebase.txt |  5 +++
 git-rebase--interactive.sh   | 76 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d44bc85..2152e27 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -2204,6 +2204,14 @@ rebase.autoStash::
 	successful rebase might result in non-trivial conflicts.
 	Defaults to false.
 
+rebase.checkLevel::
+	If set to "warn", git rebase -i will print a warning if some
+	commits are removed (i.e. a line was deleted) or if some
+	commits appear more than one time (e.g. the same commit is
+	picked twice), however the rebase will still proceed. If set
+	to "error", it will print the previous warnings and abort the
+	rebase.
+
 receive.advertiseAtomic::
 	By default, git-receive-pack will advertise the atomic push
 	capability to its clients. If you don't want to this capability

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3cd2ef2..cb05cbb 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt

@@ -213,6 +213,11 @@ rebase.autoSquash::
 rebase.autoStash::
 	If set to true enable '--autostash' option by default.
 
+rebase.checkLevel::
+	If set to "warn" print warnings about removed commits and
+	duplicated commits in interactive mode. If set to "error"
+	print the warnings and abort the rebase. No check by default.
+
 OPTIONS
 -------
 --onto <newbase>::

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index cb749e8..8a837ca 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -837,6 +837,80 @@ add_exec_commands () {
 	mv "$1.new" "$1"
 }
 
+# Print the list of the sha-1 of the commits
+# from a todo list in a file.
+# $1 : todo-file, $2 : outfile
+todo_list_to_sha_list () {
+	todo_list=$(git stripspace --strip-comments < "$1")
+	temp_file=$(mktemp)
+	echo "$todo_list" > "$temp_file"
+	while read -r command sha1 rest < "$temp_file"
+	do
+		case "$command" in
+		x|"exec")
+			;;
+		*)
+			echo "$sha1" >> "$2"
+			;;
+		esac
+		sed -i '1d' "$temp_file"
+	done
+	rm "$temp_file"
+}
+
+# Check if the user dropped some commits by mistake
+# or if there are two identical commits.
+# Behaviour determined by .gitconfig.
+check_commits () {
+	checkLevel=$(git config --get rebase.checkLevel)
+	checkLevel=${checkLevel:-"IGNORE"}
+	# To uppercase
+	checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
+
+	case "$checkLevel" in
+	"WARN"|"ERROR")
+		todo_list_to_sha_list "$todo".backup "$todo".oldsha1
+		todo_list_to_sha_list "$todo" "$todo".newsha1
+
+		duplicates=$(sort "$todo".newsha1 | uniq -d)
+
+		echo "$(sort -u "$todo".oldsha1)" > "$todo".oldsha1
+		echo "$(sort -u "$todo".newsha1)" > "$todo".newsha1
+		missing=$(comm -2 -3 "$todo".oldsha1 "$todo".newsha1)
+
+		# check missing commits
+		if ! test -z "$missing"
+		then
+			warn "Warning : some commits may have been dropped accidentally."
+			warn "Dropped commits:"
+			warn "$missing"
+			warn "To avoid this message, use \"drop\" to explicitely remove a commit."
+			warn "Use git --config rebase.checkLevel to change"
+			warn "the level of warnings (ignore,warn,error)."
+			warn ""
+
+			if test "$checkLevel" = "ERROR"
+			then
+				die_abort "Rebase aborted due to dropped commits."
+			fi
+		fi
+
+		# check duplicate commits
+		if ! test -z "$duplicates"
+		then
+			warn "Warning : some commits have been used twice:"
+			warn "$duplicates"
+			warn ""
+		fi
+		;;
+	"IGNORE")
+		;;
+	*)
+		warn "Unrecognized setting for option rebase.checkLevel in git rebase -i"
+		;;
+	esac
+}
+
 # The whole contents of this file is run by dot-sourcing it from
 # inside a shell function.  It used to be that "return"s we see
 # below were not inside any function, and expected to return

@@ -1082,6 +1156,8 @@ has_action "$todo" ||
 
 expand_todo_ids
 
+check_commits
+
 test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
 
 GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"

-- 
2.4.1.174.g28bfe8e

On Tue, May 26, 2015 at 5:38 PM, Galan Rémi
[off-list ref] wrote:

git-rebase -i: Add key word "drop" to remove a commit

"key word" is unusual. More typical is "keyword". However, perhaps
"command" might be even better. Also, custom on this project is not to
capitalize, so:

    git-rebase -i: add command "drop" to remove a commit

Instead of removing a line to remove the commit, you can use the key
word "drop" (just like "pick" or "edit"). It has the same effect as
deleting the line (removing the commit) except that you keep a visual
trace of your actions, allowing a better control and reducing the
possibility of removing a commit by mistake.

Nicely explained.

Ditto regarding "key word".

I think the existing method of removing a commit merits mention here. Perhaps:

    To drop a commit, delete its line or replace the command
    "pick" with "drop".

Or, if you want to emphasize "drop":

    To drop a commit, replace the command "pick" with "drop",
    or just delete its line.

On Tue, May 26, 2015 at 5:38 PM, Galan Rémi
[off-list ref] wrote:

git rebase -i: Warn removed or dupplicated commits

s/dupplicated/duplicated/

Also, drop capitalization, and insert "about":

    git rebase -i: warn about removed or duplicated commits

Check if commits were removed (i.e. a line was deleted) or dupplicated

s/dupplicated/duplicated/

(e.g. the same commit is picked twice), can print warnings or abort

s/can/and/, I think.

git rebase according to the value of the configuration variable
rebase.checkLevel.

Add the configuration variable rebase.checkLevel.
    - When unset or set to "IGNORED", no checking is done.

s/IGNORED/IGNORE/

    - When set to "WARN", the commits are checked, warnings are
      displayed but git rebase still proceeds.
    - When set to "ERROR", the commits are checked, warnings are
      displayed and the rebase is aborted.

Why uppercase for these names? Is there precedence for that? I think
lowercase is more common.

The commit message talks about "ignore", but there is no mention here.

Also, what is the default behavior if not specified? That should be documented.

Finally, this talks about lowercase "warn" and "error", whereas the
commit message uses upper case "WARN" and "ERROR", as does the code.
Why the inconsistency?

Ditto: Fails to mention "ignore".

On this project it is typical to drop the space after redirection
operators (<, >, >>), however, git-rebase--interactive.sh is filled
with both styles (space and no space after redirection). New code
probably ought to drop the space.

+       do
+               case "$command" in
+               x|"exec")
+                       ;;
+               *)
+                       echo "$sha1" >> "$2"
+                       ;;
+               esac
+               sed -i '1d' "$temp_file"
+       done
+       rm "$temp_file"
+}
+
+# Check if the user dropped some commits by mistake
+# or if there are two identical commits.
+# Behaviour determined by .gitconfig.
+check_commits () {
+       checkLevel=$(git config --get rebase.checkLevel)
+       checkLevel=${checkLevel:-"IGNORE"}

Minor aside: Unnecessary quoting increases the noise level, thus
making the code slightly more difficult to read. This could just as
well have been:

    checkLevel=${checkLevel:-IGNORE}

There are plenty of other places throughout this patch which exhibit
the same shortcoming, but I won't point them out individually.

+       # To uppercase
+       checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')

Is there precedence elsewhere for recognizing uppercase and lowercase
variants of config values?

+       case "$checkLevel" in
+       "WARN"|"ERROR")
+               todo_list_to_sha_list "$todo".backup "$todo".oldsha1
+               todo_list_to_sha_list "$todo" "$todo".newsha1
+
+               duplicates=$(sort "$todo".newsha1 | uniq -d)
+
+               echo "$(sort -u "$todo".oldsha1)" > "$todo".oldsha1
+               echo "$(sort -u "$todo".newsha1)" > "$todo".newsha1
+               missing=$(comm -2 -3 "$todo".oldsha1 "$todo".newsha1)
+
+               # check missing commits
+               if ! test -z "$missing"

Isn't "! test -z" just a verbose way of saying "test -n"?

+               then
+                       warn "Warning : some commits may have been dropped accidentally."
+                       warn "Dropped commits:"
+                       warn "$missing"
+                       warn "To avoid this message, use \"drop\" to explicitely remove a commit."

s/explicitely/explicitly/

+                       warn "Use git --config rebase.checkLevel to change"
+                       warn "the level of warnings (ignore,warn,error)."
+                       warn ""
+
+                       if test "$checkLevel" = "ERROR"
+                       then
+                               die_abort "Rebase aborted due to dropped commits."
+                       fi
+               fi
+
+               # check duplicate commits
+               if ! test -z "$duplicates"
+               then
+                       warn "Warning : some commits have been used twice:"
+                       warn "$duplicates"
+                       warn ""
+               fi

Shouldn't this case also 'die' when rebase.checkLevel is "error"? And,
why doesn't the user get advice about configuring rebase.checkLevel in
this case?

In fact, the current logic flow seems a bit borked. I would have
expected it to be more like this:

    if test -n "$missing"
    then
        ...warn about accidental drops...
    fi

    if test -n "$duplicates"
    then
        ...warn about accidental duplicates...
    fi

    if test -n "$missing$duplicates"
    then
        ...show advice about configuring rebase.checkLevel...

        if test $checkLevel = ERROR
        then
            die_abort "..."
        fi
    fi

+               ;;
+       "IGNORE")
+               ;;
+       *)
+               warn "Unrecognized setting for option rebase.checkLevel in git rebase -i"

This message might be more useful if it mentioned the actual unrecognized value.

Hi Rémi,

On 2015-05-26 23:38, Galan Rémi wrote:

Instead of removing a line to remove the commit, you can use the key
word "drop" (just like "pick" or "edit"). It has the same effect as
deleting the line (removing the commit) except that you keep a visual
trace of your actions, allowing a better control and reducing the
possibility of removing a commit by mistake.

Please note that you can already just comment-out the line if you need to keep a visual trace.

Alternatively, you can replace the `pick` command by `noop`.

If you really need the `drop` command (with which I am not 100% happy because I already envisage users appending a `drop A` to an edit script "pick A; pick B; pick C" and expecting A *not to be picked*), then it is better to just add the `drop` part to the already existing `noop` clause:

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f7deeb0..8355be8 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -489,7 +489,7 @@ do_next () {
 	rm -f "$msg" "$author_script" "$amend" || exit
 	read -r command sha1 rest < "$todo"
 	case "$command" in
-	"$comment_char"*|''|noop)
+	"$comment_char"*|''|noop|drop)
 		mark_action_done
 		;;
 	pick|p)

Ciao,
Johannes

Galan Rémi <remi.galan-alfonso <at> ensimag.grenoble-inp.fr> writes:

Check if commits were removed (i.e. a line was deleted) or dupplicated
(e.g. the same commit is picked twice), can print warnings or abort
git rebase according to the value of the configuration variable
rebase.checkLevel.

I sometimes duplicate commits deliberately if I want to split a commit in
two. I move a copy up and fix the conflict, and I know that I'll still get
the right thing later even if I make a mistake with the conflict resolution.

Thank you for reviewing the code. 

Eric Sunshine[off-list ref] writes:

quoted

+       # To uppercase
+       checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')

Is there precedence elsewhere for recognizing uppercase and lowercase
variants of config values?

It seems to be commonly used when parsing options in the C files
through strcasecmp.  For exemple, in config.c:818 :
if (!strcmp(var, "core.safecrlf")) {
	if (value && !strcasecmp(value, "warn")) {
		[...]
However we didn't see any precedence in shell files. Do you think we
should remove it?

Eric Sunshine[off-list ref] writes:

Shouldn't this case also 'die' when rebase.checkLevel is "error"? And,
why doesn't the user get advice about configuring rebase.checkLevel in
this case?

Stephen Kelly[off-list ref] writes:

I sometimes duplicate commits deliberately if I want to split a commit in
two.

Matthieu Moy[off-list ref] writes:

The more I think about it, the more I think we should either not warn at
all on duplicate commits, or have a separate config variable.

Put in common because two config variables would have an effect on the
'die' and advise part.

In this patch we didn't put the 'die' in the duplicate commit part
since there was only one config variable and there are cases where the
user might want to duplicate commits.

After the code reviewing of Eric Sunshine and Stephen Kelly, we also
came to the conclusion that we should use two config variables, one
about missing commits and the other about duplicate commits.

This way if you deliberately want to use duplicate commits, you can
just set the value to 'ignore' for duplicate commits and still have
'warn'/'error' for missing commits. Moreover, each part would have its
'die' depending on the value of the corresponding config variable.

Thank you for reviewing the code.

Johannes Schindelin[off-list ref] writes:

Please note that you can already just comment-out the line if you need to keep a visual trace.

Alternatively, you can replace the `pick` command by `noop`.

If you really need the `drop` command (with which I am not 100%
happy because I already envisage users appending a `drop A` to an
edit script "pick A; pick B; pick C" and expecting A *not to be
picked*)

It is true that drop has the same effect as noop or commenting,
however we thought that drop is more understandable for average users of
git. Moreover when using git rebase -i, the 'help' displayed below the
list of commits doesn't mention neither the noop command nor the
effect of commenting the line (though considering what removing a line
does, it can be easily deduced).

The drop command was inspired by the drop command from histedit in
mercurial.

It also has some effects with the second part of this patch (checks
removed and/or duplicated commits): if you comment the line, the
commit will be considered as removed, thus ending in a warning if the
config variable is set to warn/error; however this problem won't
appear with noop.

On Wed, May 27, 2015 at 9:19 AM, Remi Galan Alfonso
[off-list ref] wrote:

Eric Sunshine[off-list ref] writes:

quoted

quoted

+       # To uppercase
+       checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')

Is there precedence elsewhere for recognizing uppercase and lowercase
variants of config values?

It seems to be commonly used when parsing options in the C files
through strcasecmp.  For exemple, in config.c:818 :
if (!strcmp(var, "core.safecrlf")) {
        if (value && !strcasecmp(value, "warn")) {
                [...]
However we didn't see any precedence in shell files. Do you think we
should remove it?

Precedence in C code is good enough for me, and it makes sense for
your new code to follow suit by being insensitive to case (as you have
already done).

However, it would be a good idea to be consistent in your use of
uppercase/lowercase in the commit message, documentation, and code,
rather than having a mix. I'd suggest sticking with lowercase
throughout since lowercase is more commonly used in the codebase (and
just easier to read).