[PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[PATCHv6 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 | 16 ++++++++++++++++
 4 files changed, 23 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..ecd277c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1102,4 +1102,20 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
 	test $(git cat-file commit HEAD | sed -ne \$p) = I
 '
 
+test_rebase_end () {
+	test_when_finished "git checkout master &&
+	git branch -D $1 &&
+	test_might_fail git rebase --abort" &&
+	git checkout -b $1 master
+}
+
+test_expect_success 'drop' '
+	test_rebase_end dropTest &&
+	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.371.g8992f2a

[PATCHv6 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>
---
 On the contrary of v5, the SHA-1 that are read are the short one.
 (the todo-list is read directly after the user closes his editor
 and not after expansion of ids)

 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 4d21ce1..25b2a04 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2160,6 +2160,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 ecd277c..a92ae19 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1118,4 +1118,70 @@ test_expect_success 'drop' '
 	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
 '
 
+cat >expect <<EOF
+Successfully rebased and updated refs/heads/tmp2.
+EOF
+
+test_expect_success 'rebase -i respects rebase.missingCommitsCheck = ignore' '
+	test_config rebase.missingCommitsCheck ignore &&
+	test_rebase_end tmp2 &&
+	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/tmp2.
+EOF
+
+test_expect_success 'rebase -i respects rebase.missingCommitsCheck = warn' '
+	test_config rebase.missingCommitsCheck warn &&
+	test_rebase_end tmp2 &&
+	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 &&
+	test_rebase_end tmp2 &&
+	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.371.g8992f2a

[PATCHv6 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>
---
 I used:
   read -r command sha1 rest <<EOF
   $line
   EOF
 because
   printf '%s' "$line" | read -r command sha1 rest
 doesn't work (the 3 variables have no value as a result).
 There might be a better way to do this, but I don't have it right now.

 git-rebase--interactive.sh    | 70 +++++++++++++++++++++++++++++++++++++++++++
 t/lib-rebase.sh               |  5 ++++
 t/t3404-rebase-interactive.sh | 40 +++++++++++++++++++++++++
 3 files changed, 115 insertions(+)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 66daacf..6381686 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -834,6 +834,52 @@ 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=f
+	if test -z $1
+	then
+		badsha=t
+	else
+		sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
+		if test -z $sha1_verif
+		then
+			badsha=t
+		fi
+	fi
+
+	if test $badsha = t
+	then
+		printf '%s\n' "$2" >>"$todo".badsha
+	fi
+}
+
+# prints the bad commits and bad commands
+# from the todolist in stdin
+check_bad_cmd_and_sha () {
+	git stripspace --strip-comments |
+	while read -r line
+	do
+		read -r command sha1 rest <<EOF
+$line
+EOF
+		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"
+			;;
+		*)
+			printf '%s\n' "$line" >>"$todo".badcmd
+			;;
+		esac
+	done
+}
+
 # Print the list of the SHA-1 of the commits
 # from stdin to stdout
 todo_list_to_sha_list () {
@@ -868,6 +914,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 +967,28 @@ check_todo_list () {
 		;;
 	esac
 
+	check_bad_cmd_and_sha <"$todo"
+
+	if test -s "$todo".badsha
+	then
+		raise_error=t
+
+		warn "Warning: the SHA-1 is missing or isn't" \
+			"a commit in the following line(s):"
+		warn_lines <"$todo".badsha
+		warn
+	fi
+
+	if test -s "$todo".badcmd
+	then
+		raise_error=t
+
+		warn "Warning: the command isn't recognized" \
+			"in the following line(s):"
+		warn_lines <"$todo".badcmd
+		warn
+	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 a92ae19..d691b1c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1184,4 +1184,44 @@ 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(s):
+ - 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' '
+	test_rebase_end tmp2 &&
+	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(s):
+ - 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' '
+	test_config rebase.missingCommitsCheck error &&
+	test_rebase_end tmp2 &&
+	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.371.g8992f2a

Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[Eric Sunshine]

On Mon, Jun 22, 2015 at 5:42 PM, Galan Rémi
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> 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>
> ---
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ac429a0..ecd277c 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1102,4 +1102,20 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
>         test $(git cat-file commit HEAD | sed -ne \$p) = I
>  '
>
> +test_rebase_end () {
> +       test_when_finished "git checkout master &&
> +       git branch -D $1 &&
> +       test_might_fail git rebase --abort" &&
> +       git checkout -b $1 master
> +}

The way this is indented makes it difficult to see that lines 2 and 3
are continuations of 1. Perhaps format it like this instead?

    test_rebase_end () {
        test_when_finished "git checkout master &&
            git branch -D $1 &&
            test_might_fail git rebase --abort" &&
        git checkout -b $1 master
    }

> +
> +test_expect_success 'drop' '
> +       test_rebase_end dropTest &&
> +       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.371.g8992f2a

Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[Remi Galan Alfonso]

Eric Sunshine <sunshine@sunshineco.com> writes:
> > +test_rebase_end () {
> > +       test_when_finished "git checkout master &&
> > +       git branch -D $1 &&
> > +       test_might_fail git rebase --abort" &&
> > +       git checkout -b $1 master
> > +}
> 
> The way this is indented makes it difficult to see that lines 2 and 3
> are continuations of 1. Perhaps format it like this instead?
> 
>     test_rebase_end () {
>         test_when_finished "git checkout master &&
>             git branch -D $1 &&
>             test_might_fail git rebase --abort" &&
>         git checkout -b $1 master
>     }

I completely agree with you, moreover it was indented like this before.
I'll change it in my local version for now.

Ironically, it was modified after the following:

Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > > +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=ignore' '
> > > +       test_config rebase.missingCommitsCheck ignore &&
> > > +       test_when_finished "git checkout master &&
> > > +               git branch -D tmp2" &&
> >
> > Strange indentation.
> 
> Considering that 'git branch -D tmp2' is a part of test_when_finished,
> I wasn't sure of how it was supposed to be indented, so I did it this
> way to show that it was still within test_when_finished and not a
> separate command.
> >         test_when_finished "git checkout master &&
> >         git branch -D tmp2" &&
> Doesn't seem as clear, especially if you quickly read the lines.
> 
> For now, I have removed the tab.

:p

Thanks,
Rémi

Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[Matthieu Moy]

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

> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > +test_rebase_end () {
>> > +       test_when_finished "git checkout master &&
>> > +       git branch -D $1 &&
>> > +       test_might_fail git rebase --abort" &&
>> > +       git checkout -b $1 master
>> > +}
>> 
>> The way this is indented makes it difficult to see that lines 2 and 3
>> are continuations of 1. Perhaps format it like this instead?
>> 
>>     test_rebase_end () {
>>         test_when_finished "git checkout master &&
>>             git branch -D $1 &&
>>             test_might_fail git rebase --abort" &&
>>         git checkout -b $1 master
>>     }
>
> I completely agree with you, moreover it was indented like this before.
> I'll change it in my local version for now.

Perhaps to avoid confusion, stg like:

	test_when_finished "
		... &&
		...
	" &&
	git checkout

(the closing " alone on its line)

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

Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[Eric Sunshine]

On Tue, Jun 23, 2015 at 3:01 PM, Remi Galan Alfonso
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > +test_rebase_end () {
>> > +       test_when_finished "git checkout master &&
>> > +       git branch -D $1 &&
>> > +       test_might_fail git rebase --abort" &&
>> > +       git checkout -b $1 master
>> > +}
>>
>> The way this is indented makes it difficult to see that lines 2 and 3
>> are continuations of 1. Perhaps format it like this instead?
>>
>>     test_rebase_end () {
>>         test_when_finished "git checkout master &&
>>             git branch -D $1 &&
>>             test_might_fail git rebase --abort" &&
>>         git checkout -b $1 master
>>     }
>
> I completely agree with you, moreover it was indented like this before.
> I'll change it in my local version for now.
>
> Ironically, it was modified after the following:
>
> Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > > +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=ignore' '
>> > > +       test_config rebase.missingCommitsCheck ignore &&
>> > > +       test_when_finished "git checkout master &&
>> > > +               git branch -D tmp2" &&
>> >
>> > Strange indentation.

Clearly, that guy who made the "Strange indentation" review comment
didn't know what he was talking about. ;-)

In that earlier review, I must have missed the fact that the quoted
string spanned multiple lines, and, unfortunately, got too busy with
other things to say "ah, the indentation makes perfect sense" when
your response pointed out its true nature.

Matthieu's suggestion of formatting it like:

    test_when_finished "
        ... &&
        ...
    " &&

should help to avoid such misconceptions.

Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[Remi Galan Alfonso]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> 
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> >> > +test_rebase_end () {
> >> > +       test_when_finished "git checkout master &&
> >> > +       git branch -D $1 &&
> >> > +       test_might_fail git rebase --abort" &&
> >> > +       git checkout -b $1 master
> >> > +}
> >>
> >> The way this is indented makes it difficult to see that lines 2 and 3
> >> are continuations of 1. Perhaps format it like this instead?
> >>
> >>     test_rebase_end () {
> >>         test_when_finished "git checkout master &&
> >>             git branch -D $1 &&
> >>             test_might_fail git rebase --abort" &&
> >>         git checkout -b $1 master
> >>     }
> >
> > I completely agree with you, moreover it was indented like this before.
> > I'll change it in my local version for now.
> 
> Perhaps to avoid confusion, stg like:
> 
>         test_when_finished "
>                 ... &&
>                 ...
>         " &&
>         git checkout
> 
> (the closing " alone on its line)

I think that the indentation on its own is enough to avoid confusion
> test_rebase_end () {
> 	test_when_finished "git checkout master &&
> 		git branch -D $1 &&
> 		test_might_fail git rebase --abort" &&
> 	git checkout -b $1 master
> }
but your idea is fine as well, so I'm ok with either way.

Thanks,
Rémi

Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[Junio C Hamano]

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

> +test_rebase_end () {
> +	test_when_finished "git checkout master &&
> +	git branch -D $1 &&

Is this one guaranteed to succeed?  Do we want to consider it a
failure to remove "$1" (e.g. dropTest)?

    $ git branch -D no-such-branch ; echo $?
    error: branch 'no-such-branch' not found.
    1

If dropTest branch did not exist before the test that begins with
a call to this function, what happens?

Besides, a function that must be called at the beginning of a test
piece has a name that ends with _end?  That sounds funny, no?

> +	test_might_fail git rebase --abort" &&
> +	git checkout -b $1 master
> +}

I'm wondering if this is not sufficient.

	test_rebase_i_drop_prepare () {
		git reset --hard &&
	        git checkout -B "$1" master
	}

I am guessing that you named _end because it has when_finished, but
as far as I can tell, even after these three patches, the tests do
not really rely on the fact that it is on 'master' branch.  More
importantly, just being on 'master' branch is not a sufficient
cleanliness for the next test (and that is why you added these
"branch -D" and "might-fail rebase --abort" to this function in the
first place), it seems.  So...

> +test_expect_success 'drop' '
> +	test_rebase_end dropTest &&
> +	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

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

[Junio C Hamano]

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

>  I used:
>    read -r command sha1 rest <<EOF
>    $line
>    EOF
>  because
>    printf '%s' "$line" | read -r command sha1 rest
>  doesn't work (the 3 variables have no value as a result).
>  There might be a better way to do this, but I don't have it right now.

	while read line
        do
		(
			IFS=' '
                        set x $line
			shift
                        # now $1 is your command, $2 is sha1, $3 is remainder
			...
		)
	done

perhaps?

But more importantly, why do you even need to keep the bad ones in a
separate .badcmd and .badsha files?  Isn't that bloating your changes
unnecessarily, iow, if you issued your warning as you encounter them,
wouldn't the change become cleaner and easier to understand (and as
a side effect it may even become smaller)?  The _only_ thing that
you would get by keeping them in temporary files is that you can do
"one header and bunch of errors", but is it so common to make a bad
edit to the insn sheet that "a sequence of errors, one per line"
becomes more burdensome to the end user?

I would think

	stripspace |
	while read -r command sha1 rest
        do
        	...

and showing the warning as you detect inside that loop would be
sufficient.  Perhaps I am missing subtle details of what you are
doing.

Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[Remi Galan Alfonso]

Junio C Hamano <gitster@pobox.com> writes:
> > Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> > 
> > > +test_rebase_end () {
> > > +        test_when_finished "git checkout master &&
> > > +        git branch -D $1 &&
> > 
> > Is this one guaranteed to succeed?  Do we want to consider it a
> > failure to remove "$1" (e.g. dropTest)?
> > 
> >     $ git branch -D no-such-branch ; echo $?
> >     error: branch 'no-such-branch' not found.
> >     1
> > 
> > If dropTest branch did not exist before the test that begins with
> > a call to this function, what happens?
> 

Since the function is
> 	test_when_finished "git checkout master &&
> 		git branch -D $1 &&
> 		test_might_fail git rebase --abort" &&
> 	git checkout -b $1 master
If $1 doesn't exist, then it means that 'git checkout -b $1 master'
failed, thus the test would fail before reaching the part in
'test_when_finished'.
However I guess there are more reasons that could cause 'git branch -D
$1' to fail so I'll add a 'test_might_fail' in front of it.

> Besides, a function that must be called at the beginning of a test
> piece has a name that ends with _end?  That sounds funny, no?

I see your point but I'm not really sure how to call it, considering
that what it does is creating a branch to test on it, and taking care
of the cleaning at the end of the test.
Maybe something more generic like "setup_and_clean" ?
 
> > +        test_might_fail git rebase --abort" &&
> > +        git checkout -b $1 master
> > +}
> 
> I'm wondering if this is not sufficient.
> 
>         test_rebase_i_drop_prepare () {
>                 git reset --hard &&
>                 git checkout -B "$1" master
>         }
> 
> I am guessing that you named _end because it has when_finished, but
> as far as I can tell, even after these three patches, the tests do
> not really rely on the fact that it is on 'master' branch.  More
> importantly, just being on 'master' branch is not a sufficient
> cleanliness for the next test (and that is why you added these
> "branch -D" and "might-fail rebase --abort" to this function in the
> first place), it seems.  So...

I removed the branch in case someone used the same name when creating
a branch in a future test. However he would notice it when writing the
test and it's not something that would suddenly break when modifying
the code, so it might not be necessary.
The "might-fail rebase --abort" is there in case this test fails in
the middle (because of a future modification of rebase for example) to
avoid failling all the following tests that use rebase.

The name "test_rebase_i_drop_prepare" wouldn't be accurate since 2/3
and 3/3 uses the function as well and don't really have much to do
with 'drop'.

Thanks,
Rémi

Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[Matthieu Moy]

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

> I think that the indentation on its own is enough to avoid confusion
>> test_rebase_end () {
>> 	test_when_finished "git checkout master &&
>> 		git branch -D $1 &&
>> 		test_might_fail git rebase --abort" &&
>> 	git checkout -b $1 master
>> }
> but your idea is fine as well, so I'm ok with either way.

Read too quickly, it looks like a mis-indentation (I could laugh at Eric
here, but I made the same confusion when reading the code at first). By
"avoid the confusion" I mean "make it clear it's not a mis-indentation".

Anyway, we're just bikeshedding here. Any version is fine with me.

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

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

[Remi Galan Alfonso]

Junio C Hamano <gitster@pobox.com> writes:
> Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> 
> >  I used:
> >    read -r command sha1 rest <<EOF
> >    $line
> >    EOF
> >  because
> >    printf '%s' "$line" | read -r command sha1 rest
> >  doesn't work (the 3 variables have no value as a result).
> >  There might be a better way to do this, but I don't have it right now.
> 
>         while read line
>         do
>                 (
>                         IFS=' '
>                         set x $line
>                         shift
>                         # now $1 is your command, $2 is sha1, $3 is remainder
>                         ...
>                 )
>         done
> 
> perhaps?

Will try, thanks!

> But more importantly, why do you even need to keep the bad ones in a
> separate .badcmd and .badsha files?  Isn't that bloating your changes
> unnecessarily, iow, if you issued your warning as you encounter them,
> wouldn't the change become cleaner and easier to understand (and as
> a side effect it may even become smaller)?  The _only_ thing that
> you would get by keeping them in temporary files is that you can do
> "one header and bunch of errors", but is it so common to make a bad
> edit to the insn sheet that "a sequence of errors, one per line"
> becomes more burdensome to the end user?
> 
> I would think
> 
>         stripspace |
>         while read -r command sha1 rest
>         do
>                 ...
> 
> and showing the warning as you detect inside that loop would be
> sufficient.  Perhaps I am missing subtle details of what you are
> doing.

You're not missing subtle details, it is as you said, I tough it would
be clearer for the user to have "one header and a bunch of errors".
Moreover while it would make the patch smaller and easier to
understand, I am not sure about making it cleaner; I guess I will have
to try and see how it ends up.

What I'm not completely happy with your proposition is the fact that 
if there are multiple errors of the same kind, the output would look 
something like:
> Warning: the command isn't recognized in the following line:
> badcmd1 some_sha some_commit_message
> Warning: the command isn't recognized in the following line:
> badcmd2 some_sha some_commit_message
(I don't think it would be good to squash some understandable warning
message and the faulty line in one line, it would probably end up
being too long)
However as you say, such mistakes are uncommon so I guess it's fine.

Thanks,
Rémi

Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
>
>> +test_rebase_end () {
>> +	test_when_finished "git checkout master &&
>> +	git branch -D $1 &&
>
> Is this one guaranteed to succeed?  Do we want to consider it a
> failure to remove "$1" (e.g. dropTest)?
>
>     $ git branch -D no-such-branch ; echo $?
>     error: branch 'no-such-branch' not found.
>     1
>
> If dropTest branch did not exist before the test that begins with
> a call to this function, what happens?
>
> Besides, a function that must be called at the beginning of a test
> piece has a name that ends with _end?  That sounds funny, no?

Ah, scratch this last paragraph.  I didn't see this is a
multi-command "when_finished".

But other parts of what I said still stands.  For example, even in a
multi-command "when_finished", "git branch -D $1 &&" if the main
body of the test failed to create the branch "$1", that command
would fail and skip the remainder of the clean-up, so the first
point above is still suspect.

Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[Junio C Hamano]

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

> Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
>
>> I think that the indentation on its own is enough to avoid confusion
>>> test_rebase_end () {
>>> 	test_when_finished "git checkout master &&
>>> 		git branch -D $1 &&
>>> 		test_might_fail git rebase --abort" &&
>>> 	git checkout -b $1 master
>>> }
>> but your idea is fine as well, so I'm ok with either way.
>
> Read too quickly, it looks like a mis-indentation (I could laugh at Eric
> here, but I made the same confusion when reading the code at first). By
> "avoid the confusion" I mean "make it clear it's not a mis-indentation".

Yes, that stray " fooled me as well.  If it were following your
suggestion in the earlier message on this thread, i.e.

	test_when_finished "
		... &&
		...
	" &&
	git checkout

I wouldn't have to waste time commenting on it ;-)

Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

[Remi Galan Alfonso]

Junio C Hamano <gitster@pobox.com> writes:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
> > Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> >
> >> I think that the indentation on its own is enough to avoid confusion
> >>> test_rebase_end () {
> >>>         test_when_finished "git checkout master &&
> >>>                 git branch -D $1 &&
> >>>                 test_might_fail git rebase --abort" &&
> >>>         git checkout -b $1 master
> >>> }
> >> but your idea is fine as well, so I'm ok with either way.
> >
> > Read too quickly, it looks like a mis-indentation (I could laugh at Eric
> > here, but I made the same confusion when reading the code at first). By
> > "avoid the confusion" I mean "make it clear it's not a mis-indentation".
> 
> Yes, that stray " fooled me as well.  If it were following your
> suggestion in the earlier message on this thread, i.e.
> 
>         test_when_finished "
>                 ... &&
>                 ...
>         " &&
>         git checkout
> 
> I wouldn't have to waste time commenting on it ;-)

I will do it this way then. ;)

Thanks,
Rémi