threads / patch / 39513

patch, 2 partsgit-rebase -i: add command "drop" to remove a commit

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

## tl;dr

11 messages between Jun 3, 2015 and Jun 6, 2015. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Galan Rémi· Jun 3, 2015, 11:44 UTC · lore

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(-)
Show changes to 4 files +33 −2

Documentation/git-rebase.txt, git-rebase--interactive.sh, t/lib-rebase.sh, t/t3404-rebase-interactive.sh

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.2.389.geaf7ccf
Galan Rémi· Jun 3, 2015, 11:44 UTC · re: Galan Rémi · lore

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

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

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 to.

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 aborted.
rebase.missingCommitsCheck defaults to "ignore".
Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>
---
 Documentation/config.txt      | 10 ++++++
 Documentation/git-rebase.txt  |  6 ++++
 git-rebase--interactive.sh    | 82 +++++++++++++++++++++++++++++++++++++++++++
 t/t3404-rebase-interactive.sh | 63 +++++++++++++++++++++++++++++++++
 4 files changed, 161 insertions(+)
Show changes to 4 files +161 −0

Documentation/config.txt, Documentation/git-rebase.txt, git-rebase--interactive.sh, t/t3404-rebase-interactive.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4d21ce1..b29cd8d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2160,6 +2160,16 @@ 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 abort the rebase. 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 9cf3760..6d413a1 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
+	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..26804dd 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -851,6 +851,86 @@ 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 () {
+	git stripspace --strip-comments <"$1" | while read -r command sha1 rest
+	do
+		case $command in
+		x|"exec")
+			;;
+		*)
+			printf "%s\n" "$sha1"
+			;;
+		esac
+	done >"$2"
+}
+
+# Use warn for each line of a file
+# $1: file
+warn_file () {
+	while read -r line
+	do
+		warn " - $line"
+	done <"$1"
+}
+
+# Check if the user dropped some commits by mistake
+# Behaviour determined by rebase.missingCommitsCheck.
+check_commits () {
+	checkLevel=$(git config --get rebase.missingCommitsCheck)
+	checkLevel=${checkLevel:-ignore}
+	# 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
+		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
+
+		# Make the list user-friendly
+		opt="--no-walk=sorted --format=oneline --abbrev-commit --stdin"
+		git rev-list $opt <"$todo".miss >"$todo".miss+
+		mv "$todo".miss+ "$todo".miss
+
+		# Check missing commits
+		if test -s "$todo".miss
+		then
+			warn "Warning: some commits may have been dropped" \
+				"accidentally."
+			warn "Dropped commits (newer to older):"
+			warn_file "$todo".miss
+			warn ""
+			warn "To avoid this message, use \"drop\" to" \
+				"explicitly remove a commit."
+			warn "Use git --config rebase.missingCommitsCheck 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.missingCommitsCheck."
+		;;
+	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 +1176,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..f369d2c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1112,4 +1112,67 @@ 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_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>warning &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_cmp warning expect
+'
+
+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 (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_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>warning &&
+	test D = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_cmp warning expect
+'
+
+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 (ignore, warn, error).
+
+Rebase aborted due to dropped commits.
+EOF
+
+test_expect_success 'rebase -i respects rebase.missingCommitsCheck=error' '
+	test_config rebase.missingCommitsCheck 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 4" \
+		git rebase -i --root 2>warning &&
+	test E = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test_cmp warning expect
+'
+
 test_done
-- 
2.4.2.389.geaf7ccf
Remi Galan Alfonso· Jun 3, 2015, 12:48 UTC · re: Galan Rémi · lore

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

Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
Show 8 quoted lines
> +                comm -2 -3 "$todo".oldsha1 "$todo".newsha1 >"$todo".miss
> +
> +                # Make the list user-friendly
> +                opt="--no-walk=sorted --format=oneline --abbrev-commit --stdin"
> +                git rev-list $opt <"$todo".miss >"$todo".miss+
> +                mv "$todo".miss+ "$todo".miss
> +
> +                # Check missing commits

Found a bug here, got an error message from git rev-list if "$todo".miss is empty.

Now it looks like:
Show 9 quoted lines
> 		# Check missing commits
> 		if test -s "$todo".miss
> 		then
> 			# Make the list user-friendly
> 			opt="--no-walk=sorted --format=oneline --abbrev-commit --stdin"
> 			git rev-list $opt <"$todo".miss >"$todo".miss+
> 			mv "$todo".miss+ "$todo".miss
> 
> 			warn "Warning: some commits may have been dropped" \
Thus the empty case is tested by the test -s of the warnings.

By the way, should I add --quiet to the options of the call to git rev-list?

Rémi
Matthieu Moy· Jun 3, 2015, 17:38 UTC · re: Galan Rémi · lore

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

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 depending on the value of the
> configuration variable rebase.missingCommits.
Spelled missingCommitsCheck everywhere else.
> +rebase.missingCommitsCheck::
> +	If set to "warn" print warnings about removed commits in
If set to warn, print (comma). Here and below
> +	interactive mode. If set to "error" print the warnings and
> +	abort the rebase. If set to "ignore" no checking is
> +	done. "ignore" by default.
Show 5 quoted lines
> +# 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 () {
> +	git stripspace --strip-comments <"$1" | while read -r command sha1 rest
I'd split the line in two:
	git stripspace --strip-comments <"$1" |
	while read -r command sha1 rest
Show 10 quoted lines
> +	do
> +		case $command in
> +		x|"exec")
> +			;;
> +		*)
> +			printf "%s\n" "$sha1"
> +			;;
> +		esac
> +	done >"$2"
> +}

You're using $1 and $2 only to redirect input and output. I would find it more elegant to write todo_list_to_sha_list as a filter, and do the redirection in the call site (to keep the option of using todo_list_to_sha_list in a pipe).

> +check_commits
You're not really checking commits, but rather the todo-list.
> +Use git --config rebase.missingCommitsCheck to change the level of warnings (ignore, warn, error).
Long line. Please, wrap the message to <80 columns.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Remi Galan Alfonso· Jun 3, 2015, 17:53 UTC · re: Matthieu Moy · lore

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

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> You're using $1 and $2 only to redirect input and output. I would find
> it more elegant to write todo_list_to_sha_list as a filter, and do the
> redirection in the call site (to keep the option of using
> todo_list_to_sha_list in a pipe).
If I understood correctly, then the calling line would look like:
> todo_list_to_sha_list <"$todo".backup >"$todo".oldsha1
?

Should I do the same for the function warn_file ? Meaning

> warn_file <"$todo".miss
instead of
> warn_file "$todo".miss
The other points mentioned have been corrected.

Thanks, Rémi

Matthieu Moy· Jun 3, 2015, 18:06 UTC · re: Remi Galan Alfonso · lore

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

Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
Show 9 quoted lines
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>> You're using $1 and $2 only to redirect input and output. I would find
>> it more elegant to write todo_list_to_sha_list as a filter, and do the
>> redirection in the call site (to keep the option of using
>> todo_list_to_sha_list in a pipe).
>
> If I understood correctly, then the calling line would look like:
>> todo_list_to_sha_list <"$todo".backup >"$todo".oldsha1
> ?
Yes.
Show 5 quoted lines
> Should I do the same for the function warn_file ?
> Meaning
>> warn_file <"$todo".miss
> instead of
>> warn_file "$todo".miss

If you do so, then warn_file won't be a good name anymore since you'd be able to read from a pipe too.

Anyway, it's really a matter of personnal preference. I would keep warn_file as-is and change todo_list_to_sha_list, but feel free to ignore this point if you disagree.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Eric Sunshine· Jun 3, 2015, 19:14 UTC · re: Galan Rémi · lore

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

On Wednesday, June 3, 2015, Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:

> Check if commits were removed (i.e. a line was deleted) and print
> warnings or abort git rebase depending on the value of the
> configuration variable rebase.missingCommits.
A few comments below in addition to those already made by Matthieu...
Show 16 quoted lines
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8960083..f369d2c 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1112,4 +1112,67 @@ 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_when_finished "git checkout master &&
> +               git branch -D tmp2" &&
Strange indentation.
> +       git checkout -b tmp2 master &&
> +       set_fake_editor &&
> +       FAKE_LINES="1 2 3 4" \
> +               git rebase -i --root 2>warning &&
The file containing the actual output is usually spelled "actual".
> +       test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> +       test_cmp warning expect

The arguments to test_cmp are usually reversed so that 'expect' comes before 'actual', which results in a more natural-feeling diff when test_cmp detects that the files differ.

These comments apply to remaining new tests, as well.
Show 52 quoted lines
> +'
> +
> +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 (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_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>warning &&
> +       test D = $(git cat-file commit HEAD | sed -ne \$p) &&
> +       test_cmp warning expect
> +'
> +
> +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 (ignore, warn, error).
> +
> +Rebase aborted due to dropped commits.
> +EOF
> +
> +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=error' '
> +       test_config rebase.missingCommitsCheck 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 4" \
> +               git rebase -i --root 2>warning &&
> +       test E = $(git cat-file commit HEAD | sed -ne \$p) &&
> +       test_cmp warning expect
> +'
> +
>  test_done
> --
> 2.4.2.389.geaf7ccf
Remi Galan Alfonso· Jun 3, 2015, 20:09 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 6 quoted lines
> > +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. Also the other points have been corrected.

Thank you, Rémi

Remi Galan Alfonso· Jun 6, 2015, 23:45 UTC · re: Remi Galan Alfonso · lore

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

I'm going to try to change the die_abort in this patch by a die, so that the user can use rebase --edit-todo afterward. This way, adding the checking on the SHA-1 for the 'drop' command (discussed in 1/2) (and also maybe the other commands requiring a correct SHA-1 corresponding to a commit) to the 2/2 part would make a bit more sense. Though I still see some other issues with this, I agree that it makes more sense in 2/2 rather than in 1/2 (some more checking in a future patch would be a good idea).

(So far I've tried rather quickly, but it doesn't seem as easy as I originally though, working on it though)

Rémi
Junio C Hamano· Jun 3, 2015, 18:02 UTC · re: Galan Rémi · lore

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

Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
Show 21 quoted lines
> 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'."

Is this a sensible piece of advice, though? The user edited the line away and it does not have the commit object name anymore. How does she "fix" it in the editor? The same puzzlement applies to the other message.

> +		sha1_verif="$(git rev-parse --verify --quiet $sha1^{commit})"
> +		if test -z $sha1_verif

As you are not using the returned string at all, instead of wasting a shell variable, it would be better to decide to come into this block by using the exit status from rev-parse (it exits with non-zero status upon failure).

Show 10 quoted lines
> +		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
Remi Galan Alfonso· Jun 3, 2015, 19:23 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> But what if you got this back after the user edits?
> 
>     drop
>     pick 2c9c1c5 gostak: distim doshes
>     drop e3b601d pull: use git-rev-parse...
>     edit eb2a8d9 pull: handle git-fetch'...
> 
> [...]
> Did the user tried to drop something else but the
> object name has gone missing by mistake?

If the user tried to drop something but the SHA-1 has gone missing, it would be picked up by 2/2 if rebase.missingCommitsCheck is set to warn or error.

> Did the user wanted to drop the first one but made mistake while
> editing 'pick' away into 'drop'?
I see your point here.
Show 5 quoted lines
> Noticing and flagging malformed 'drop' lines (or line with any
> command, for that matter) as such is part of that process to make
> sure you collected the object names from the "after" image
> correctly, which is the job of 2/2 in your series (if I am reading
> the description of your series right).

I agree on the fact that noticing and flagging malformed lines for the various commands is a part of the process to make sure the collected object names are correct. However, the original job of 2/2 was to avoid the silent loss of information caused by deleting a line by mistake, not to check the correctness of the whole todo-file (though I think that it is a good idea, to develop in another commit of this patch/in another patch). On the opposite, in some way it could be seen as some loss of information, as your previous example suggests:

> Did the user wanted to drop the first one but made mistake while
> editing 'pick' away into 'drop'?

We lose the information that the user wanted to drop the line and not pick it.

Aside from that, checking specifically the 'drop' command beforehand (that's what happens in 2/2) while not doing any checking on the other commands (or checking on the fly) doesn't really makes sense, I think the commands should be treated equally on the matter of checking. In doing so, checking that the various commands exist, that they are followed by the correct argument and that their argument is of the right type (SHA-1 representing a commit for example), in top of checking that no commit is removed, without forgetting tests to make sure everything has the right behaviour, I am afraid that it would make this part of the patch far too long thus why I think it should be in another commit/patch.

By the way
> In the new world order to punish those who simply remove lines to
> signal that they want the commits omitted from replaying

Aside from liking the wording here, I don't think it can really be considered as a punishment since it is the user that chooses if he wants to be "punished" or not; I guess it can be considered BDSM though.

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> > +        drop|d)
> > +                if test -z $sha1
> > +                then
> > +                        warn "Missing SHA-1 in 'drop' command."
> > +                        die "Please fix this using 'git rebase --edit-todo'."
> 
> Is this a sensible piece of advice, though?  The user edited the
> line away and it does not have the commit object name anymore.
> How does she "fix" it in the editor?  The same puzzlement applies
> to the other message.

For a missing SHA-1, if it is a 'drop' that he forgot to remove after changing his mind, then it can be fixed. For an incorrect SHA-1, maybe he can find it back using git log or others commands of the kind, though I don't think this way of fixing things is user-friendly at all. However, as you point out, in most cases it will be a line edited away, I agree that the "fix it" doesn't really help.

The only alternative I see right now (in 1/2) is aborting the rebase (die_abort) though it seems overkill, as the user I wouldn't want to lose all of the modifications I have done on the todo-list.

Through this I see your point about checking in 2/2 since we would have more information (for example if he has an error because a drop has no SHA-1 or the SHA-1 is incorrect and at the same time an error because a SHA-1 has disappeared, it would make more sense to give him "fix it" since he would have most of the informations he needs to do so). However, right now, I think that my previous points about 2/2 still stand.

Thanks, Rémi

← back to recent threads