rebase -i: drop, missing commits and static checks

rebase -i: drop, missing commits and static checks

[Galan Rémi]

Changes between versions:

In t3404:

Changed 'test_rebase_end' to 'rebase_setup_and_clean'.
Changed the indentation in 'rebase_setup_and_clean'.
Changed the names of the branches created in my tests (avoid names
like 'tmp').
Added 'test_might_fail' in front of 'git branch -D'.
Remove 'test_config rebase.missingCommitsCheck error' in the last test
('static check of bad SHA-1') because it was useless.

In git-rebase--interactive.sh:

Errors found in commands and SHA-1 (static check) are displayed on the
spot.
I used return values to signal to the calling functions if there is an
error.
The whole while block in 'check_bad_cmd_and_sha' with the return is in a 
'(
	[...]
)'
block because 'retval' would not have been correctly affected when
getting out of the loop since the while is in a pipe.

A thought occured to me:
Shouldn't all the checking also be called in a 'rebase --continue',
considering that it can be called after a 'rebase --edit-todo' ?
(Right now it is only called after closing the editor in 'rebase -i')

[PATCHv7 1/3] git-rebase -i: add command "drop" to remove a commit
[PATCHv7 2/3] git rebase -i: warn about removed commits
[PATCHv7 3/3] git rebase -i: add static check for commands and SHA-1

Rémi
(It seems that with 'send-email', 1 time out of 2 I get:
'5.7.8 Error: authentication failed: authentication failure')

[PATCHv7 1/3] 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    |  3 ++-
 t/lib-rebase.sh               |  4 ++--
 t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++
 4 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1d01baa..34bd070 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 the matching 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..72abf90 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.
 
@@ -505,7 +506,7 @@ do_next () {
 	rm -f "$msg" "$author_script" "$amend" "$state_dir"/stopped-sha || exit
 	read -r command sha1 rest < "$todo"
 	case "$command" in
-	"$comment_char"*|''|noop)
+	"$comment_char"*|''|noop|drop|d)
 		mark_action_done
 		;;
 	pick|p)
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..3d059e5 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1102,4 +1102,22 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
 	test $(git cat-file commit HEAD | sed -ne \$p) = I
 '
 
+rebase_setup_and_clean () {
+	test_when_finished "
+		git checkout master &&
+		test_might_fail git branch -D $1 &&
+		test_might_fail git rebase --abort
+	" &&
+	git checkout -b $1 master
+}
+
+test_expect_success 'drop' '
+	rebase_setup_and_clean drop-test &&
+	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.3.532.gf6210e5.dirty

[PATCHv7 2/3] 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 stop git rebase depending on the value of the
configuration variable rebase.missingCommitsCheck.

This patch gives the user the possibility to avoid silent loss of
information (losing a commit through deleting the line in this case)
if he wants.

Add the configuration variable rebase.missingCommitsCheck.
    - 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 stopped.
      (The user can then use 'git rebase --edit-todo' and
      'git rebase --continue', or 'git rebase --abort')

rebase.missingCommitsCheck defaults to "ignore".

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3e37b93..0360e7b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2169,6 +2169,17 @@ rebase.autoStash::
 	successful rebase might result in non-trivial conflicts.
 	Defaults to false.
 
+rebase.missingCommitsCheck::
+	If set to "warn", git rebase -i will print a warning if some
+	commits are removed (e.g. a line was deleted), however the
+	rebase will still proceed. If set to "error", it will print
+	the previous warning and stop the rebase, 'git rebase
+	--edit-todo' can then be used to correct the error. If set to
+	"ignore", no checking is done.
+	To drop a commit without warning or error, use the `drop`
+	command in the todo-list.
+	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 34bd070..2ca3b8d 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.missingCommitsCheck::
+	If set to "warn", print warnings about removed commits in
+	interactive mode. If set to "error", print the warnings and
+	stop 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 72abf90..66daacf 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -834,6 +834,104 @@ add_exec_commands () {
 	mv "$1.new" "$1"
 }
 
+# Print the list of the SHA-1 of the commits
+# from stdin to stdout
+todo_list_to_sha_list () {
+	git stripspace --strip-comments |
+	while read -r command sha1 rest
+	do
+		case $command in
+		"$comment_char"*|''|noop|x|"exec")
+			;;
+		*)
+			long_sha=$(git rev-list --no-walk "$sha1" 2>/dev/null)
+			printf "%s\n" "$long_sha"
+			;;
+		esac
+	done
+}
+
+# Use warn for each line in stdin
+warn_lines () {
+	while read -r line
+	do
+		warn " - $line"
+	done
+}
+
+# Switch to the branch in $into and notify it in the reflog
+checkout_onto () {
+	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
+	output git checkout $onto || die_abort "could not detach HEAD"
+	git update-ref ORIG_HEAD $orig_head
+}
+
+# Check if the user dropped some commits by mistake
+# Behaviour determined by rebase.missingCommitsCheck.
+check_todo_list () {
+	raise_error=f
+
+	check_level=$(git config --get rebase.missingCommitsCheck)
+	check_level=${check_level:-ignore}
+	# Don't be case sensitive
+	check_level=$(printf '%s' "$check_level" | tr 'A-Z' 'a-z')
+
+	case "$check_level" 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
+		sort -u "$todo".oldsha1 >"$todo".oldsha1+
+		mv "$todo".oldsha1+ "$todo".oldsha1
+		sort -u "$todo".newsha1 >"$todo".newsha1+
+		mv "$todo".newsha1+ "$todo".newsha1
+		comm -2 -3 "$todo".oldsha1 "$todo".newsha1 >"$todo".miss
+
+		# Warn about missing commits
+		if test -s "$todo".miss
+		then
+			test "$check_level" = error && raise_error=t
+
+			warn "Warning: some commits may have been dropped" \
+				"accidentally."
+			warn "Dropped commits (newer to older):"
+
+			# Make the list user-friendly and display
+			opt="--no-walk=sorted --format=oneline --abbrev-commit --stdin"
+			git rev-list $opt <"$todo".miss | warn_lines
+
+			warn "To avoid this message, use \"drop\" to" \
+				"explicitly remove a commit."
+			warn
+			warn "Use 'git --config rebase.missingCommitsCheck' to change" \
+				"the level of warnings."
+			warn "The possible behaviours are: ignore, warn, error."
+			warn
+		fi
+		;;
+	ignore)
+		;;
+	*)
+		warn "Unrecognized setting $check_level for option" \
+			"rebase.missingCommitsCheck. Ignoring."
+		;;
+	esac
+
+	if test $raise_error = t
+	then
+		# Checkout before the first commit of the
+		# rebase: this way git rebase --continue
+		# will work correctly as it expects HEAD to be
+		# placed before the commit of the next action
+		checkout_onto
+
+		warn "You can fix this with 'git rebase --edit-todo'."
+		die "Or you can abort the rebase with 'git rebase --abort'."
+	fi
+}
+
 # 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
@@ -1077,13 +1175,13 @@ git_sequence_editor "$todo" ||
 has_action "$todo" ||
 	return 2
 
+check_todo_list
+
 expand_todo_ids
 
 test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
 
-GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
-output git checkout $onto || die_abort "could not detach HEAD"
-git update-ref ORIG_HEAD $orig_head
+checkout_onto
 do_rest
 
 }
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3d059e5..904a2d0 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1120,4 +1120,70 @@ test_expect_success 'drop' '
 	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
 '
 
+cat >expect <<EOF
+Successfully rebased and updated refs/heads/missing-commit.
+EOF
+
+test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
+	test_config rebase.missingCommitsCheck ignore &&
+	rebase_setup_and_clean missing-commit &&
+	set_fake_editor &&
+	FAKE_LINES="1 2 3 4" \
+		git rebase -i --root 2>actual &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_cmp expect actual
+'
+
+cat >expect <<EOF
+Warning: some commits may have been dropped accidentally.
+Dropped commits (newer to older):
+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
+To avoid this message, use "drop" to explicitly remove a commit.
+
+Use 'git --config rebase.missingCommitsCheck' to change the level of warnings.
+The possible behaviours are: ignore, warn, error.
+
+Successfully rebased and updated refs/heads/missing-commit.
+EOF
+
+test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
+	test_config rebase.missingCommitsCheck warn &&
+	rebase_setup_and_clean missing-commit &&
+	set_fake_editor &&
+	FAKE_LINES="1 2 3 4" \
+		git rebase -i --root 2>actual &&
+	test_cmp expect actual &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p)
+'
+
+cat >expect <<EOF
+Warning: some commits may have been dropped accidentally.
+Dropped commits (newer to older):
+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
+ - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
+To avoid this message, use "drop" to explicitly remove a commit.
+
+Use 'git --config rebase.missingCommitsCheck' to change the level of warnings.
+The possible behaviours are: ignore, warn, error.
+
+You can fix this with 'git rebase --edit-todo'.
+Or you can abort the rebase with 'git rebase --abort'.
+EOF
+
+test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
+	test_config rebase.missingCommitsCheck error &&
+	rebase_setup_and_clean missing-commit &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 2 4" \
+		git rebase -i --root 2>actual &&
+	test_cmp expect actual &&
+	cp .git/rebase-merge/git-rebase-todo.backup \
+		.git/rebase-merge/git-rebase-todo &&
+	FAKE_LINES="1 2 drop 3 4 drop 5" \
+		git rebase --edit-todo &&
+	git rebase --continue &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
+'
+
 test_done
-- 
2.4.3.532.gf6210e5.dirty

[PATCHv7 3/3] git rebase -i: add static check for commands and SHA-1

[Galan Rémi]

Check before the start of the rebasing if the commands exists, and for
the commands expecting a SHA-1, check if the SHA-1 is present and
corresponds to a commit. In case of error, print the error, stop git
rebase and prompt the user to fix with 'git rebase --edit-todo' or to
abort.

This allows to avoid doing half of a rebase before finding an error
and giving back what's left of the todo list to the user and prompt
him to fix when it might be too late for him to do so (he might have
to abort and restart the rebase).

Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>
---
 git-rebase--interactive.sh    | 75 +++++++++++++++++++++++++++++++++++++++++++
 t/lib-rebase.sh               |  5 +++
 t/t3404-rebase-interactive.sh | 39 ++++++++++++++++++++++
 3 files changed, 119 insertions(+)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 66daacf..ec4a068 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -834,6 +834,73 @@ add_exec_commands () {
 	mv "$1.new" "$1"
 }
 
+# Check if the SHA-1 passed as an argument is a
+# correct one, if not then print $2 in "$todo".badsha
+# $1: the SHA-1 to test
+# $2: the line to display if incorrect SHA-1
+check_commit_sha () {
+	badsha=0
+	if test -z $1
+	then
+		badsha=1
+	else
+		sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
+		if test -z $sha1_verif
+		then
+			badsha=1
+		fi
+	fi
+
+	if test $badsha -ne 0
+	then
+		warn "Warning: the SHA-1 is missing or isn't" \
+			"a commit in the following line:"
+		warn " - $2"
+		warn
+	fi
+
+	return $badsha
+}
+
+# prints the bad commits and bad commands
+# from the todolist in stdin
+check_bad_cmd_and_sha () {
+	retval=0
+	git stripspace --strip-comments |
+	(
+		while read -r line
+		do
+			IFS=' '
+			set x $line
+			shift
+			command=$1
+			sha1=$2
+
+			case $command in
+			''|noop|x|"exec")
+				# Doesn't expect a SHA-1
+				;;
+			pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
+				check_commit_sha $sha1 "$line"
+				if test $? -ne 0
+				then
+					retval=1
+				fi
+				;;
+			*)
+				warn "Warning: the command isn't recognized" \
+					"in the following line:"
+				warn " - $line"
+				warn
+				retval=1
+				;;
+			esac
+		done
+
+		return $retval
+	)
+}
+
 # Print the list of the SHA-1 of the commits
 # from stdin to stdout
 todo_list_to_sha_list () {
@@ -868,6 +935,8 @@ checkout_onto () {
 
 # Check if the user dropped some commits by mistake
 # Behaviour determined by rebase.missingCommitsCheck.
+# Check if there is an unrecognized command or a
+# bad SHA-1 in a command.
 check_todo_list () {
 	raise_error=f
 
@@ -919,6 +988,12 @@ check_todo_list () {
 		;;
 	esac
 
+	check_bad_cmd_and_sha <"$todo"
+	if test $? -ne 0
+	then
+		raise_error=t
+	fi
+
 	if test $raise_error = t
 	then
 		# Checkout before the first commit of the
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index fdbc900..9a96e15 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -54,6 +54,11 @@ set_fake_editor () {
 			echo '# comment' >> "$1";;
 		">")
 			echo >> "$1";;
+		bad)
+			action="badcmd";;
+		fakesha)
+			echo "$action XXXXXXX False commit" >> "$1"
+			action=pick;;
 		*)
 			sed -n "${line}s/^pick/$action/p" < "$1".tmp >> "$1"
 			action=pick;;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 904a2d0..9b2c51c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1186,4 +1186,43 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
+cat >expect <<EOF
+Warning: the command isn't recognized in the following line:
+ - badcmd $(git rev-list --oneline -1 master~1)
+
+You can fix this with 'git rebase --edit-todo'.
+Or you can abort the rebase with 'git rebase --abort'.
+EOF
+
+test_expect_success 'static check of bad command' '
+	rebase_setup_and_clean bad-cmd &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 2 3 bad 4 5" \
+		git rebase -i --root 2>actual &&
+	test_cmp expect actual &&
+	FAKE_LINES="1 2 3 drop 4 5" git rebase --edit-todo &&
+	git rebase --continue &&
+	test E = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test C = $(git cat-file commit HEAD^ | sed -ne \$p)
+'
+
+cat >expect <<EOF
+Warning: the SHA-1 is missing or isn't a commit in the following line:
+ - edit XXXXXXX False commit
+
+You can fix this with 'git rebase --edit-todo'.
+Or you can abort the rebase with 'git rebase --abort'.
+EOF
+
+test_expect_success 'static check of bad SHA-1' '
+	rebase_setup_and_clean bad-sha &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 2 edit fakesha 3 4 5 #" \
+		git rebase -i --root 2>actual &&
+	test_cmp expect actual &&
+	FAKE_LINES="1 2 4 5 6" git rebase --edit-todo &&
+	git rebase --continue &&
+	test E = $(git cat-file commit HEAD | sed -ne \$p)
+'
+
 test_done
-- 
2.4.3.532.gf6210e5.dirty

[PATCH 0/3] rebase -i: drop, missing commits and static checks

[Matthieu Moy]

Hi,

Here are a few fixes to squash into the commits of the series. Other
than that, the series looks good to me.

Junio: do you prefer a reroll or do you want to apply locally?

Matthieu Moy (3):
  fixup! git rebase -i: add static check for commands and SHA-1
  fixup! git rebase -i: warn about removed commits
  fixup! git rebase -i: warn about removed commits

 git-rebase--interactive.sh    | 32 +++++++++++++++++++++-----------
 t/t3404-rebase-interactive.sh |  4 ++--
 2 files changed, 23 insertions(+), 13 deletions(-)

-- 
2.5.0.rc0.10.g7792c2a

[PATCH 1/3] fixup! git rebase -i: add static check for commands and SHA-1

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 git-rebase--interactive.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ec4a068..9041d15 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -871,8 +871,7 @@ check_bad_cmd_and_sha () {
 		while read -r line
 		do
 			IFS=' '
-			set x $line
-			shift
+			set -- $line
 			command=$1
 			sha1=$2
 
@@ -881,8 +880,7 @@ check_bad_cmd_and_sha () {
 				# Doesn't expect a SHA-1
 				;;
 			pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
-				check_commit_sha $sha1 "$line"
-				if test $? -ne 0
+				if ! check_commit_sha $sha1 "$line"
 				then
 					retval=1
 				fi
@@ -988,8 +986,7 @@ check_todo_list () {
 		;;
 	esac
 
-	check_bad_cmd_and_sha <"$todo"
-	if test $? -ne 0
+	if ! check_bad_cmd_and_sha <"$todo"
 	then
 		raise_error=t
 	fi
-- 
2.5.0.rc0.10.g7792c2a

[PATCH 2/3] fixup! git rebase -i: warn about removed commits

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 git-rebase--interactive.sh | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9041d15..0117791 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -156,8 +156,17 @@ Commands:
 
 These lines can be re-ordered; they are executed from top to bottom.
 
+EOF
+	if test $(get_missing_commit_check_level) = error
+	then
+		git stripspace --comment-lines >>"$todo" <<\EOF
+Do not remove any line. Use 'drop' explicitly to remove a commit.
+EOF
+	else
+		git stripspace --comment-lines >>"$todo" <<\EOF
 If you remove a line here THAT COMMIT WILL BE LOST.
 EOF
+	fi
 }
 
 make_patch () {
@@ -931,6 +940,13 @@ checkout_onto () {
 	git update-ref ORIG_HEAD $orig_head
 }
 
+get_missing_commit_check_level () {
+	check_level=$(git config --get rebase.missingCommitsCheck)
+	check_level=${check_level:-ignore}
+	# Don't be case sensitive
+	printf '%s' "$check_level" | tr 'A-Z' 'a-z'
+}
+
 # Check if the user dropped some commits by mistake
 # Behaviour determined by rebase.missingCommitsCheck.
 # Check if there is an unrecognized command or a
@@ -938,10 +954,7 @@ checkout_onto () {
 check_todo_list () {
 	raise_error=f
 
-	check_level=$(git config --get rebase.missingCommitsCheck)
-	check_level=${check_level:-ignore}
-	# Don't be case sensitive
-	check_level=$(printf '%s' "$check_level" | tr 'A-Z' 'a-z')
+	check_level=$(get_missing_commit_check_level)
 
 	case "$check_level" in
 	warn|error)
-- 
2.5.0.rc0.10.g7792c2a

[PATCH 3/3] fixup! git rebase -i: warn about removed commits

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 git-rebase--interactive.sh    | 2 +-
 t/t3404-rebase-interactive.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0117791..8090d80 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -985,7 +985,7 @@ check_todo_list () {
 			warn "To avoid this message, use \"drop\" to" \
 				"explicitly remove a commit."
 			warn
-			warn "Use 'git --config rebase.missingCommitsCheck' to change" \
+			warn "Use 'git config rebase.missingCommitsCheck' to change" \
 				"the level of warnings."
 			warn "The possible behaviours are: ignore, warn, error."
 			warn
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9b2c51c..ebdab4b 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1140,7 +1140,7 @@ Dropped commits (newer to older):
  - $(git rev-list --pretty=oneline --abbrev-commit -1 master)
 To avoid this message, use "drop" to explicitly remove a commit.
 
-Use 'git --config rebase.missingCommitsCheck' to change the level of warnings.
+Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
 The possible behaviours are: ignore, warn, error.
 
 Successfully rebased and updated refs/heads/missing-commit.
@@ -1163,7 +1163,7 @@ Dropped commits (newer to older):
  - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2)
 To avoid this message, use "drop" to explicitly remove a commit.
 
-Use 'git --config rebase.missingCommitsCheck' to change the level of warnings.
+Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
 The possible behaviours are: ignore, warn, error.
 
 You can fix this with 'git rebase --edit-todo'.
-- 
2.5.0.rc0.10.g7792c2a

Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks

[Remi Galan Alfonso]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> Hi,
> 
> Here are a few fixes to squash into the commits of the series. Other
> than that, the series looks good to me.
> 
> Junio: do you prefer a reroll or do you want to apply locally?
> 
> Matthieu Moy (3):
>   fixup! git rebase -i: add static check for commands and SHA-1
>   fixup! git rebase -i: warn about removed commits
>   fixup! git rebase -i: warn about removed commits
> 
>  git-rebase--interactive.sh    | 32 +++++++++++++++++++++-----------
>  t/t3404-rebase-interactive.sh |  4 ++--
>  2 files changed, 23 insertions(+), 13 deletions(-)

Thanks for the various fixes !

However, I am still wondering about:

Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> Shouldn't all the checking also be called in a 'rebase --continue',
> considering that it can be called after a 'rebase --edit-todo' ?
> (Right now it is only called after closing the editor in 'rebase -i')

What's your opinion on it?

Short example:

'git rebase -i HEAD~2'
	pick commit_sha_1 commit_msg_1
	tick commit_sha_2 commit_msg_2
An error is raised before anything is done.
'git rebase --edit-todo'
	pick commit_sha_1 commit_msg_1
	tick commit_sha_2 commit_msg_2
(nothing changed)
'git rebase --continue'
An error is raised after having picked the first commit.

The same is relevent with bad sha and missing commits (in fact even
more relevant with missing commits since that would be silent loss of
information).

Thanks,
Rémi

Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks

[Matthieu Moy]

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

> Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
>> Shouldn't all the checking also be called in a 'rebase --continue',
>> considering that it can be called after a 'rebase --edit-todo' ?
>> (Right now it is only called after closing the editor in 'rebase -i')
>
> What's your opinion on it?

It would probably be better to run the check when running "git rebase
--continue". This way, we would have a guarantee that the todo-list is
syntactically correct when going through it.

Just checking after --edit-todo would not guarantee that:

  $ git rebase --edit-todo
  # Add some syntax errors, save and quit
  Warning: the command isn't recognized ...
  
  # Hmm, let's ignore that warning
  $ git rebase --continue
  Unknown command: ...
  Please fix this using 'git rebase --edit-todo'

But in any case, the most important is the initial edition. It covers
99.9% of use-cases. So, I'd say: if you have time to add the checks at
the other relevant places, good, but not doing it shouldn't block the
inclusion of the series.

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

Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks

[Junio C Hamano]

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

> Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
>> Shouldn't all the checking also be called in a 'rebase --continue',
>> considering that it can be called after a 'rebase --edit-todo' ?
>> (Right now it is only called after closing the editor in 'rebase -i')
>
> What's your opinion on it?
>
> Short example:
>
> 'git rebase -i HEAD~2'
> 	pick commit_sha_1 commit_msg_1
> 	tick commit_sha_2 commit_msg_2
> An error is raised before anything is done.
> 'git rebase --edit-todo'
> 	pick commit_sha_1 commit_msg_1
> 	tick commit_sha_2 commit_msg_2
> (nothing changed)
> 'git rebase --continue'
> An error is raised after having picked the first commit.
>
> The same is relevent with bad sha and missing commits (in fact even
> more relevant with missing commits since that would be silent loss of
> information).

The place where an error can be introduced is (assuming that what
"rebase -i" writes out itself is perfect ;-) where we allow the user
to edit, so instead of checking before "--continue", I would expect
a sane design would check immediately after the editor we spawned
returns.

The codepath that allows the insn sheet getting edited and the
codepath that handles --edit-todo have to do many things the same
way (i.e. use collapse_todo_ids to prepare the file for editing,
spawn the editor, receive the result and use expand_todo_ids to
prepare the file to be used by us), and I would have expected for
these two to be using a single helper---and a sanity check like the
above can and should be done when we receive the result from the
editor, immediately before running expand_todo_ids perhaps.

If they are not using such a single helper right now, perhaps that
is the preliminary restructuring of the code that is needed?

Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks

[Remi Galan Alfonso]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> 
> > Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> >> Shouldn't all the checking also be called in a 'rebase --continue',
> >> considering that it can be called after a 'rebase --edit-todo' ?
> >> (Right now it is only called after closing the editor in 'rebase -i')
> >
> > What's your opinion on it?
> 
> It would probably be better to run the check when running "git rebase
> --continue". This way, we would have a guarantee that the todo-list is
> syntactically correct when going through it.
> 
> Just checking after --edit-todo would not guarantee that:

That's actually what I had in mind, my message wasn't clear enough, my
bad.

> But in any case, the most important is the initial edition. It covers
> 99.9% of use-cases. So, I'd say: if you have time to add the checks at
> the other relevant places, good, but not doing it shouldn't block the
> inclusion of the series.

Agreed, that's something that can be done in a future patch without
interfering with this one.

Junio C Hamano <gitster@pobox.com> writes:
> The place where an error can be introduced is (assuming that what
> "rebase -i" writes out itself is perfect ;-) where we allow the user
> to edit, so instead of checking before "--continue", I would expect
> a sane design would check immediately after the editor we spawned
> returns.

Makes sense but we would have the problem mentioned by Matthieu:
> Warning: the command isn't recognized ...
>   
> # Hmm, let's ignore that warning
> $ git rebase --continue

While in most cases it would be irrelevant (it would be the user that 
ignored the error on purpose), I can imagine the following situation:
 - 'git rebase --edit-todo'
 - get an error
 - go do something else, forgot where I was when I come back
 - 'git status', "you are in the middle of a rebasing"
 - 'git rebase --continue'

> The codepath that allows the insn sheet getting edited and the
> codepath that handles --edit-todo have to do many things the same
> way (i.e. use collapse_todo_ids to prepare the file for editing,
> spawn the editor, receive the result and use expand_todo_ids to
> prepare the file to be used by us), and I would have expected for
> these two to be using a single helper---and a sanity check like the
> above can and should be done when we receive the result from the
> editor, immediately before running expand_todo_ids perhaps.
> 
> If they are not using such a single helper right now, perhaps that
> is the preliminary restructuring of the code that is needed?

Good point, maybe I'll try doing that at a later date, however as
Matthieu said, I think it doesn't hurt to add this patch (if there is
no further corrections to do) and do additional things in a later
patch.

Thanks,
Rémi

Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks

[Matthieu Moy]

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

> Junio C Hamano <gitster@pobox.com> writes:
>> The place where an error can be introduced is (assuming that what
>> "rebase -i" writes out itself is perfect ;-) where we allow the user
>> to edit, so instead of checking before "--continue", I would expect
>> a sane design would check immediately after the editor we spawned
>> returns.
>
> Makes sense but we would have the problem mentioned by Matthieu:
>> Warning: the command isn't recognized ...
>>   
>> # Hmm, let's ignore that warning
>> $ git rebase --continue

There's an alternative:

$ git rebase --edit-todo
# Make mistakes, save and quit
Your todo-list has the following issues:
- ...
Do you want to edit again (no aborts the rebase) [Y/n]?

There's a precedent with the 'e' command of "git add -p". I have a
slight preference for non-interactive commands so I prefer not going
this way though.

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

Re: [PATCH 0/3] rebase -i: drop, missing commits and static checks

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> There's an alternative:
>
> $ git rebase --edit-todo
> # Make mistakes, save and quit
> Your todo-list has the following issues:
> - ...
> Do you want to edit again (no aborts the rebase) [Y/n]?
>
> There's a precedent with the 'e' command of "git add -p". I have a
> slight preference for non-interactive commands so I prefer not going
> this way though.

edit-todo (and "rebase -i" in general) is all about going
interactive, isn't it?  I was almost going to suggest to keep
spawning the editor until the user gets it right, but that would
infinitely repeat when GIT_EDITOR is set to a broken script, so
I didn't ;-).