[PATCH/RFCv3 1/2] git-rebase -i: add command "drop" to remove a commit

[PATCH/RFCv3 1/2] git-rebase -i: add command "drop" to remove a commit

[Galan Rémi]

Instead of removing a line to remove the commit, you can use the
command "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 <remi.galan-alfonso@ensimag.grenoble-inp.fr>
---
 Documentation/git-rebase.txt  |  3 +++
 git-rebase--interactive.sh    | 18 ++++++++++++++++++
 t/lib-rebase.sh               |  4 ++--
 t/t3404-rebase-interactive.sh | 10 ++++++++++
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1d01baa..9cf3760 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".
 
+To drop a commit, replace the command "pick" with "drop", or just
+delete its line.
+
 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..869cc60 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.
 
@@ -508,6 +509,23 @@ do_next () {
 	"$comment_char"*|''|noop)
 		mark_action_done
 		;;
+	drop|d)
+		if test -z $sha1
+		then
+			warn "Missing SHA-1 in 'drop' command."
+			die "Please fix this using 'git rebase --edit-todo'."
+		fi
+
+		sha1_verif="$(git rev-parse --verify --quiet $sha1^{commit})"
+		if test -z $sha1_verif
+		then
+			warn "'$sha1' is not a SHA-1 or does not represent" \
+				"a commit in 'drop' command."
+			die "Please fix this using 'git rebase --edit-todo'."
+		fi
+
+		mark_action_done
+		;;
 	pick|p)
 		comment_for_reflog pick
 
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..8960083 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1102,4 +1102,14 @@ 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' '
+	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.411.g9c4ad60

[PATCH/RFCv3 2/2] git rebase -i: warn about removed commits

[Galan Rémi]

Check if commits were removed (i.e. a line was deleted) and print
warnings or abort git rebase according to the value of the
configuration variable rebase.missingCommitsCheckLevel.

Add the configuration variable rebase.missingCommitsCheckLevel.
    - When unset or set to "ignore", 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.

rebase.missingCommitsCheckLevel defaults to "ignore".

Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>
---
 Documentation/config.txt      |   7 +++
 Documentation/git-rebase.txt  |   6 +++
 git-rebase--interactive.sh    | 105 ++++++++++++++++++++++++++++++++++++++++++
 t/t3404-rebase-interactive.sh |  25 ++++++++++
 4 files changed, 143 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4d21ce1..ec9011d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2160,6 +2160,13 @@ rebase.autoStash::
 	successful rebase might result in non-trivial conflicts.
 	Defaults to false.
 
+rebase.missingCommitsCheckLevel::
+	If set to "warn", git rebase -i will print a warning if some
+	commits are removed (i.e. a line was deleted), however the
+	rebase will still proceed. If set to "error", it will print
+	the previous warning and abort the rebase. If set to
+	"ignore", no checking is done.  Defaults to "ignore".
+
 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 9cf3760..71029f8 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -213,6 +213,12 @@ rebase.autoSquash::
 rebase.autoStash::
 	If set to true enable '--autostash' option by default.
 
+rebase.missingCommitsCheckLevel::
+	If set to "warn" print warnings about removed commits in
+	interactive mode. If set to "error" print the warnings and
+	abort the rebase. If set to "ignore" no checking is
+	done. "ignore" by default.
+
 OPTIONS
 -------
 --onto <newbase>::
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 869cc60..6391423 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -851,6 +851,109 @@ 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
+	do
+		case $command in
+		x|"exec")
+			;;
+		*)
+			printf "%s\n" "$sha1"
+			;;
+		esac
+	done <$temp_file >"$2"
+	rm $temp_file
+}
+
+# Transforms SHA-1 list in argument
+# to a list of commits (in place)
+# Doesn't check if the SHA-1 are commits.
+# $1: file with long SHA-1 list
+long_sha_to_commit_list () {
+	short_missing=""
+	git_command="git show --oneline"
+	get_line_command="head -n 1"
+	temp_file=$(mktemp)
+	while read -r sha
+	do
+		if test -n "$sha"
+		then
+			commit=$($git_command $sha | $get_line_command)
+			if test -n "$commit"
+			then
+				printf "%s\n" "$commit"
+			fi
+		fi
+	done <"$1" >$temp_file
+	mv $temp_file "$1"
+}
+
+# Use warn for each line of a file
+# $1: file to warn
+warn_file () {
+	while read -r line
+	do
+		warn " - $line"
+	done <"$1"
+}
+
+# Check if the user dropped some commits by mistake
+# Behaviour determined by .gitconfig.
+check_commits () {
+	checkLevel=$(git config --get rebase.missingCommitsCheckLevel)
+	checkLevel=${checkLevel:-ignore}
+	# To lowercase
+	checkLevel=$(echo "$checkLevel" | tr 'A-Z' 'a-z')
+
+	case "$checkLevel" in
+	warn|error)
+		# Get the SHA-1 of the commits
+		todo_list_to_sha_list "$todo".backup "$todo".oldsha1
+		todo_list_to_sha_list "$todo" "$todo".newsha1
+
+		# Sort the SHA-1 and compare them
+		echo "$(sort -u "$todo".oldsha1)" >"$todo".oldsha1
+		echo "$(sort -u "$todo".newsha1)" >"$todo".newsha1
+		echo "$(comm -2 -3 "$todo".oldsha1 "$todo".newsha1)" >"$todo".miss
+
+		# Make the list user-friendly
+		long_sha_to_commit_list "$todo".miss
+
+		# Check missing commits
+		if test -s "$todo".miss
+		then
+			warn "Warning : some commits may have been dropped" \
+				"accidentally."
+			warn "Dropped commits (in no relevent order):"
+			warn_file "$todo".miss
+			warn ""
+			warn "To avoid this message, use \"drop\" to" \
+				"explicitly remove a commit."
+			warn "Use git --config rebase.missingCommitsCheckLevel to change" \
+				"the level of warnings (ignore,warn,error)."
+			warn ""
+
+			if test "$checkLevel" = error
+			then
+				die_abort "Rebase aborted due to dropped commits."
+			fi
+		fi
+		;;
+	ignore)
+		;;
+	*)
+		warn "Unrecognized setting $checkLevel for option" \
+			"rebase.missingCommitsCheckLevel 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
@@ -1096,6 +1199,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"
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8960083..07a7158 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1112,4 +1112,29 @@ test_expect_success 'drop' '
 	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
 '
 
+test_expect_success 'rebase -i respects rebase.missingCommitsCheckLevel=error' '
+	test_config rebase.missingCommitsCheckLevel error &&
+	test_when_finished "git checkout master &&
+		git branch -D tmp2" &&
+	git checkout -b tmp2 master &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 2 3 4" \
+		git rebase -i --root &&
+	test E = $(git cat-file commit HEAD | sed -ne \$p)
+'
+
+test_expect_success 'rebase -i respects rebase.missingCommitsCheckLevel=warn' '
+	test_config rebase.missingCommitsCheckLevel warn &&
+	test_when_finished "git checkout master &&
+		git branch -D tmp2" &&
+	git checkout -b tmp2 master &&
+	set_fake_editor &&
+	FAKE_LINES="1 2 3 4" \
+		git rebase -i --root 2>warn.tmp &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	sed -n "1p" warn.tmp >warnl1.tmp &&
+	echo "Warning : some commits may have been dropped accidentally." >l1.tmp &&
+	test_cmp warnl1.tmp l1.tmp
+'
+
 test_done
-- 
2.4.1.411.g9c4ad60

Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits

[Matthieu Moy]

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

> Check if commits were removed (i.e. a line was deleted) and print
> warnings or abort git rebase according to the value of the

s/according to/depending on/

(although both translate to the same "selon" in french ;-))

> configuration variable rebase.missingCommitsCheckLevel.

Now I'm wondering whether rebase.missingCommits would be sufficient. The
full config would be

[rebase]
	missingCommits = warn

which reads like "in rebase, on missing commit, warn me".

> Add the configuration variable rebase.missingCommitsCheckLevel.
>     - When unset or set to "ignore", 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.
>
> rebase.missingCommitsCheckLevel defaults to "ignore".

This all describes well *what* the commit is doing (which is essentially
rendundant with the documentation actually), but fails to really explain
*why*, which is the most important question to adress in a commit
message.

I'm convinced that this is a good idea (partly because I'm the one who
suggested ^^), but not everybody is.

> +rebase.missingCommitsCheckLevel::
> +	If set to "warn", git rebase -i will print a warning if some
> +	commits are removed (i.e. a line was deleted), however the
> +	rebase will still proceed. If set to "error", it will print
> +	the previous warning and abort the rebase. If set to
> +	"ignore", no checking is done.  Defaults to "ignore".

I think the relationship with 'drop' should be clarified here.

"To drop a commit without warning or error, use the `drop` command in the
todo-list."

?

> +# 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)

c5770f7 (contrib/diffall: create tmp dirs without mktemp, 2012-03-14)
says "mktemp is not available on all platforms." ...

> +	echo "$todo_list" >$temp_file

Don't use echo on user-supplied data. It's not portable (think what
happens if $todo_list starts with a -).

printf '%s' "$todo_list"

You don't need all these intermediate variables/files. It looks like you
want

git stripspace --strip-comments | while read -r command sha1 rest
do
...

> +# Transforms SHA-1 list in argument
> +# to a list of commits (in place)
> +# Doesn't check if the SHA-1 are commits.

s/if/whether/

> +# $1: file with long SHA-1 list
> +long_sha_to_commit_list () {
> +	short_missing=""
> +	git_command="git show --oneline"
> +	get_line_command="head -n 1"
> +	temp_file=$(mktemp)
> +	while read -r sha
> +	do
> +		if test -n "$sha"
> +		then
> +			commit=$($git_command $sha | $get_line_command)

Why not git show --oneline $sha without using this $git_command?

To me

  git show --oneline $sha | head -n 1

says all that has to be said, while

  $git_command $sha | $get_line_command

does not say anything (although it uses more characters).

But actually, computing the diff with `git show` and eliminating it with
`head` seems backwards.

I guess you want something like:

  git rev-list --pretty=oneline -1 $sha

And the whole long_sha_to_commit_list is more or less equivalent to

  git rev-list --stdin --no-walk --format=oneline --abbrev-commit

(Yes, git was _meant_ to be scriptable, and it really is)

> +# Use warn for each line of a file
> +# $1: file to warn

I don't parse "file to warn". I'd rather warn the user than a file ;-).

> +# Check if the user dropped some commits by mistake
> +# Behaviour determined by .gitconfig.

not necessarily .gitconfig, there are other names. Say
rebase.missingCommitsCheckLevel if you want to be technically accurate.

> +check_commits () {
> +	checkLevel=$(git config --get rebase.missingCommitsCheckLevel)
> +	checkLevel=${checkLevel:-ignore}
> +	# To lowercase
> +	checkLevel=$(echo "$checkLevel" | tr 'A-Z' 'a-z')

Good comments insist on _why_ not _what_. I'd write:

	# Don't be case sensitive
	checkLevel=$(echo "$checkLevel" | tr 'A-Z' 'a-z')

> +	case "$checkLevel" in
> +	warn|error)
> +		# Get the SHA-1 of the commits
> +		todo_list_to_sha_list "$todo".backup "$todo".oldsha1
> +		todo_list_to_sha_list "$todo" "$todo".newsha1
> +
> +		# Sort the SHA-1 and compare them
> +		echo "$(sort -u "$todo".oldsha1)" >"$todo".oldsha1
> +		echo "$(sort -u "$todo".newsha1)" >"$todo".newsha1

Useless uses of echo.

echo $(foo) -> foo

> +			warn "Warning : some commits may have been dropped" \

No space before : in english (hmm, didn't I already notice this one?)

> +			warn "Use git --config rebase.missingCommitsCheckLevel to change" \
> +				"the level of warnings (ignore,warn,error)."

Spaces after commas, or (ignore/warn/error).

> +		warn "Unrecognized setting $checkLevel for option" \
> +			"rebase.missingCommitsCheckLevel in git rebase -i."

I'd drop the "in git rebase -i" part. The user may have run "git rebase
--interactive" and not know -i, and normally already knows which command
is executed.

> +test_expect_success 'rebase -i respects rebase.missingCommitsCheckLevel=error' '
> +	test_config rebase.missingCommitsCheckLevel error &&
> +	test_when_finished "git checkout master &&
> +		git branch -D tmp2" &&
> +	git checkout -b tmp2 master &&
> +	set_fake_editor &&
> +	test_must_fail env FAKE_LINES="1 2 3 4" \
> +		git rebase -i --root &&
> +	test E = $(git cat-file commit HEAD | sed -ne \$p)
> +'

You should test also that

git rebase --edit-todo # playing with $EDITOR to restore the original lines.
git rebase --continue

actually continues. You did have problems with this in early
implementations, so it's not straightforward, so it deserves a test.

You should check the output of git rebase -i too.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits

[Remi Galan Alfonso]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> > Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> > +                # Sort the SHA-1 and compare them
> > +                echo "$(sort -u "$todo".oldsha1)" >"$todo".oldsha1
> > +                echo "$(sort -u "$todo".newsha1)" >"$todo".newsha1
> 
> Useless uses of echo.
> 
> echo $(foo) -> foo

In this case it is not true, because of the infile and outfile being
identical. However sort does have a -o (-output) that I missed that
allows avoiding using echo or writing in another file; I'm correcting
with this.

> You should test also that
> 
> git rebase --edit-todo # playing with $EDITOR to restore the original lines.
> git rebase --continue
> 
> actually continues. You did have problems with this in early
> implementations, so it's not straightforward, so it deserves a test.

In this patch, the error is dealt with a die_abort and not die, thus
there is no rebase --edit-todo or rebase --continue possible
afterward.

> You should check the output of git rebase -i too.

Checking that the warning was correctly displayed like in the test for
"warn" if I understood correctly. About that, is checking that the
first line is "Warning: some commits may have been dropped
accidentally." (like in the test for "warn") enough, or should I check
that the commit displayed as removed is the correct one?

The other points have been corrected, or their correction is in
progress.

Thanks,

Rémi

Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits

[Junio C Hamano]

Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>
writes:

> In this case it is not true, because of the infile and outfile being
> identical. However sort does have a -o (-output) that I missed that
> allows avoiding using echo or writing in another file; I'm correcting
> with this.

Even though it is in POSIX, some implementation may lack it, so our
code tend to avoid "sort -o".  "echo $(sort)" is also ugly and
inefficient.  The obvious and old-fashioned would be sufficient here:

	sort -u "$todo".oldsha1 >"$todo".oldsha1+ &&
        mv "$todo".oldsha1+ "$todo".oldsha1

Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits

[Matthieu Moy]

Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:

> Checking that the warning was correctly displayed like in the test for
> "warn" if I understood correctly. About that, is checking that the
> first line is "Warning: some commits may have been dropped
> accidentally." (like in the test for "warn") enough, or should I check
> that the commit displayed as removed is the correct one?

Ideally, you would check the list of commits displayed too. If the
commits sha1 are stable, this should be easy to do. If it's too hard to
test, I'd say its not worth the trouble, but others may disagree.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits

[Remi Galan Alfonso]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Ideally, you would check the list of commits displayed too. If the
> commits sha1 are stable, this should be easy to do. If it's too hard to
> test, I'd say its not worth the trouble, but others may disagree.

Originally I chose not to check if the SHA-1 were corrects since
check_commits was called right after expand_todo_ids and I thought
that expand_todo_ids checked them, but from what I understand, it
doesn't seem to check if the SHA-1 are commits, I could be wrong
though.

Rémi

Re: [PATCH/RFCv3 2/2] git rebase -i: warn about removed commits

[Remi Galan Alfonso]

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: 
> > Ideally, you would check the list of commits displayed too. If the 
> > commits sha1 are stable, this should be easy to do. If it's too hard to 
> > test, I'd say its not worth the trouble, but others may disagree. 
> 
> Originally I chose not to check if the SHA-1 were corrects since 
> check_commits was called right after expand_todo_ids and I thought 
> that expand_todo_ids checked them, but from what I understand, it 
> doesn't seem to check if the SHA-1 are commits, I could be wrong 
> though. 

Ignore this email, I completely misunderstood the email I was
responding to. 
(Mailer that doesn't show the quotes by default) 

Rémi 

Re: [PATCH/RFCv3 1/2] git-rebase -i: add command "drop" to remove a commit

[Matthieu Moy]

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

> +test_expect_success 'drop' '

Please, be more descriptive in the first argument of
test_expect_success. It's usually a good thing to say not only what the
test stresses but also what the expected behavior is.

test_expect_success 'drop actually drops the lines' '
	...
'

?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH/RFCv3 1/2] git-rebase -i: add command "drop" to remove a commit

[Remi Galan Alfonso]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> : writes
> > +test_expect_success 'drop' '
> 
> Please, be more descriptive in the first argument of
> test_expect_success. It's usually a good thing to say not only what the
> test stresses but also what the expected behavior is.
> 
> test_expect_success 'drop actually drops the lines' '

For this one I've based myself on the first argument of some other
tests that tests commands for git rebase -i :
> t3404-rebase-interactive.sh:240
> test_expect_success 'squash' '
> t3404-rebase-interactive.sh:679
> test_expect_success 'reword' '

Rémi