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

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

[Galan Rémi]

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

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

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1d01baa..9cf3760 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -514,6 +514,9 @@ rebasing.
 If you just want to edit the commit message for a commit, replace the
 command "pick" with the command "reword".
 
+To drop a commit, replace the command "pick" with "drop", or just
+delete its line.
+
 If you want to fold two or more commits into one, replace the command
 "pick" for the second and subsequent commits with "squash" or "fixup".
 If the commits had different authors, the folded commit will be
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index bab0dcc..2882276 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" || 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..1bad068 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1102,4 +1102,15 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
 	test $(git cat-file commit HEAD | sed -ne \$p) = I
 '
 
+test_expect_success 'drop' '
+	git checkout master &&
+	test_when_finished "git checkout master" &&
+	git checkout -b dropBranchTest master &&
+	set_fake_editor &&
+	FAKE_LINES="1 drop 2 3 drop 4 5" git rebase -i --root &&
+	test E = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test C = $(git cat-file commit HEAD^ | sed -ne \$p) &&
+	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
+'
+
 test_done
-- 
2.4.1.363.g9535a9c

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

[Galan Rémi]

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

Add the configuration variable rebase.checkLevel.
    - 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.checkLevel defaults to "ignore".

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f76e8c..e2e5554 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2160,6 +2160,15 @@ rebase.autoStash::
 	successful rebase might result in non-trivial conflicts.
 	Defaults to false.
 
+rebase.checkLevel::
+	If set to "warn", git rebase -i will print a warning if some
+	commits are removed (i.e. a line was deleted) or if some
+	commits appear more than one time (e.g. the same commit is
+	picked twice), however the rebase will still proceed. If set
+	to "error", it will print the previous warnings and abort the
+	rebase. If set to "ignore", no checking is done.  Defaults to
+	"ignore".
+
 receive.advertiseAtomic::
 	By default, git-receive-pack will advertise the atomic push
 	capability to its clients. If you don't want to this capability
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9cf3760..d348ca2 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.checkLevel::
+	If set to "warn" print warnings about removed commits and
+	duplicated commits in interactive mode. If set to "error"
+	print the warnings and abort the rebase. 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 2882276..58da6ee 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -837,6 +837,109 @@ add_exec_commands () {
 	mv "$1.new" "$1"
 }
 
+# Print the list of the SHA-1 of the commits
+# from a todo list in a file.
+# $1: todo-file, $2: outfile
+todo_list_to_sha_list () {
+	todo_list=$(git stripspace --strip-comments <"$1")
+	temp_file=$(mktemp)
+	echo "$todo_list" >$temp_file
+	while read -r command sha1 rest
+	do
+		case $command in
+		x|"exec")
+			;;
+		*)
+			printf "%s\n" "$sha1"
+			;;
+		esac
+	done <$temp_file >"$2"
+	rm $temp_file
+}
+
+# Transforms SHA-1 list in argument
+# to a list of commits (in place)
+# Doesn't check if the SHA-1 are commits.
+# $1: file with long SHA-1 list
+long_sha_to_commit_list () {
+	short_missing=""
+	git_command="git show --oneline"
+	get_line_command="head -n 1"
+	temp_file=$(mktemp)
+	while read -r sha
+	do
+		if test -n "$sha"
+		then
+			commit=$($git_command $sha | $get_line_command)
+			if test -n "$commit"
+			then
+				printf "%s\n" "$commit"
+			fi
+		fi
+	done <"$1" >$temp_file
+	mv $temp_file "$1"
+}
+
+# Use warn for each line of a file
+# $1: file to warn
+warn_file () {
+	while read -r line
+	do
+		warn " - $line"
+	done <"$1"
+}
+
+# Check if the user dropped some commits by mistake
+# Behaviour determined by .gitconfig.
+check_commits () {
+	checkLevel=$(git config --get rebase.checkLevel)
+	checkLevel=${checkLevel:-ignore}
+	# To lowercase
+	checkLevel=$(echo "$checkLevel" | tr 'A-Z' 'a-z')
+
+	case "$checkLevel" in
+	warn|error)
+		# Get the SHA-1 of the commits
+		todo_list_to_sha_list "$todo".backup "$todo".oldsha1
+		todo_list_to_sha_list "$todo" "$todo".newsha1
+
+		# Sort the SHA-1 and compare them
+		echo "$(sort -u "$todo".oldsha1)" >"$todo".oldsha1
+		echo "$(sort -u "$todo".newsha1)" >"$todo".newsha1
+		echo "$(comm -2 -3 "$todo".oldsha1 "$todo".newsha1)" >"$todo".miss
+
+		# Make the list user-friendly
+		long_sha_to_commit_list "$todo".miss
+
+		# Check missing commits
+		if test -s "$todo".miss
+		then
+			warn "Warning : some commits may have been dropped" \
+				"accidentally."
+			warn "Dropped commits (in no relevent order):"
+			warn_file "$todo".miss
+			warn ""
+			warn "To avoid this message, use \"drop\" to" \
+				"explicitly remove a commit."
+			warn "Use git --config rebase.checkLevel 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.checkLevel in git rebase -i."
+		;;
+	esac
+}
+
 # The whole contents of this file is run by dot-sourcing it from
 # inside a shell function.  It used to be that "return"s we see
 # below were not inside any function, and expected to return
@@ -1079,6 +1182,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 1bad068..d3a9ed5 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1103,7 +1103,6 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
 '
 
 test_expect_success 'drop' '
-	git checkout master &&
 	test_when_finished "git checkout master" &&
 	git checkout -b dropBranchTest master &&
 	set_fake_editor &&
@@ -1113,4 +1112,13 @@ test_expect_success 'drop' '
 	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
 '
 
+test_expect_success 'rebase -i respects rebase.checkLevel' '
+	test_config rebase.checkLevel error &&
+	test_when_finished "git checkout master" &&
+	git checkout -b tmp2 master &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 2 3 4" git rebase -i --root &&
+	test E = $(git cat-file commit HEAD | sed -ne \$p)
+'
+
 test_done
-- 
2.4.1.363.g9535a9c

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

[Eric Sunshine]

On Mon, Jun 1, 2015 at 7:52 AM, 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 according to the value of the
> configuration variable rebase.checkLevel.
>
> Add the configuration variable rebase.checkLevel.
>     - 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.checkLevel defaults to "ignore".
>
> Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 5f76e8c..e2e5554 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2160,6 +2160,15 @@ rebase.autoStash::
>         successful rebase might result in non-trivial conflicts.
>         Defaults to false.
>
> +rebase.checkLevel::
> +       If set to "warn", git rebase -i will print a warning if some
> +       commits are removed (i.e. a line was deleted) or if some
> +       commits appear more than one time (e.g. the same commit is
> +       picked twice), however the rebase will still proceed. If set

The cover letter says that v2 no longer checks for a duplicate,
however, this documentation still mentions it.

> +       to "error", it will print the previous warnings and abort the
> +       rebase. If set to "ignore", no checking is done.  Defaults to
> +       "ignore".
> +
>  receive.advertiseAtomic::
>         By default, git-receive-pack will advertise the atomic push
>         capability to its clients. If you don't want to this capability
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 9cf3760..d348ca2 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.checkLevel::
> +       If set to "warn" print warnings about removed commits and
> +       duplicated commits in interactive mode. If set to "error"

Same here.

> +       print the warnings and abort the rebase. If set to "ignore" no
> +       checking is done. "ignore" by default.
> +
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 1bad068..d3a9ed5 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1103,7 +1103,6 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
>  '
>
>  test_expect_success 'drop' '
> -       git checkout master &&
>         test_when_finished "git checkout master" &&
>         git checkout -b dropBranchTest master &&

This "cleanup" change might deserve its own test (or at least a
mention in the commit message).

>         set_fake_editor &&
> @@ -1113,4 +1112,13 @@ test_expect_success 'drop' '
>         test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
>  '
>
> +test_expect_success 'rebase -i respects rebase.checkLevel' '
> +       test_config rebase.checkLevel error &&
> +       test_when_finished "git checkout master" &&
> +       git checkout -b tmp2 master &&
> +       set_fake_editor &&
> +       test_must_fail env FAKE_LINES="1 2 3 4" git rebase -i --root &&
> +       test E = $(git cat-file commit HEAD | sed -ne \$p)
> +'

Shouldn't you also explicitly test "warn" and "ignore" modes?

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

[Eric Sunshine]

On Mon, Jun 1, 2015 at 7:16 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Jun 1, 2015 at 7:52 AM, Galan Rémi
> <remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
>>  test_expect_success 'drop' '
>> -       git checkout master &&
>>         test_when_finished "git checkout master" &&
>>         git checkout -b dropBranchTest master &&
>
> This "cleanup" change might deserve its own test (or at least a
> mention in the commit message).

"...might deserve its own _patch_..."

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

[Matthieu Moy]

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

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

checkLevel does not seem to be a good name, because it doesn't say
_what_ is checked. I don't have really good proposal in mind, but maybe

rebase.todoListCheckLevel
rebase.missinLinesCheckLevel

?

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

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

[Remi Galan Alfonso]

Eric Sunshine <sunshine@sunshineco.com> writes:
> > +rebase.checkLevel::
> > +       If set to "warn", git rebase -i will print a warning if some
> > +       commits are removed (i.e. a line was deleted) or if some
> > +       commits appear more than one time (e.g. the same commit is
> > +       picked twice), however the rebase will still proceed. If set
> 
> The cover letter says that v2 no longer checks for a duplicate,
> however, this documentation still mentions it.
> 
> > +rebase.checkLevel::
> > +       If set to "warn" print warnings about removed commits and
> > +       duplicated commits in interactive mode. If set to "error"
> 
> Same here.

I forgot to modify the documentation, really sorry about
that.
Corrected here.

Eric Sunshine <sunshine@sunshineco.com> writes:
> >  test_expect_success 'drop' '
> > -       git checkout master &&
> >         test_when_finished "git checkout master" &&
> >         git checkout -b dropBranchTest master &&
> 
> This "cleanup" change might deserve its own patch (or at least a
> mention in the commit message).

I will rather move the this cleanup to the first part of this patch,
where the test was added (it should cause no problem, since the patch
is still in discussion), it makes more sense. I actually thought that
was what I did but seems like I was wrong.

I forgot to double check my patch this time, and it is showing, my
bad.

Eric Sunshine <sunshine@sunshineco.com> writes:
> > +test_expect_success 'rebase -i respects rebase.checkLevel' '
> > +       test_config rebase.checkLevel error &&
> > +       test_when_finished "git checkout master" &&
> > +       git checkout -b tmp2 master &&
> > +       set_fake_editor &&
> > +       test_must_fail env FAKE_LINES="1 2 3 4" git rebase -i --root &&
> > +       test E = $(git cat-file commit HEAD | sed -ne \$p)
> > +'
> 
> Shouldn't you also explicitly test "warn" and "ignore" modes?

I don't think testing "ignore" is really necessary since it
corresponds to the default behaviour, it is thus silently tested by
the other tests.
Either way, I will add a test for "warn".

Rémi

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

[Remi Galan Alfonso]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> 
> > Check if commits were removed (i.e. a line was deleted) and print
> > warnings or abort git rebase according to the value of the
> > configuration variable rebase.checkLevel.
> 
> checkLevel does not seem to be a good name, because it doesn't say
> _what_ is checked. I don't have really good proposal in mind, but maybe
> 
> rebase.todoListCheckLevel
> rebase.missinLinesCheckLevel
> 
> ?

I agree that we could use a better name.

I'm thinking about
   > rebase.missingLinesCheckLevel
   or
   rebase.missingCommitsCheckLevel
but I am worried that these names are too long.
Maybe
   rebase.dropCheckLevel

I don't really have better propositions right now.

Rémi

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

[Eric Sunshine]

On Tue, Jun 2, 2015 at 3:42 AM, Remi Galan Alfonso
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > +test_expect_success 'rebase -i respects rebase.checkLevel' '
>> > +       test_config rebase.checkLevel error &&
>> > +       test_when_finished "git checkout master" &&
>> > +       git checkout -b tmp2 master &&
>> > +       set_fake_editor &&
>> > +       test_must_fail env FAKE_LINES="1 2 3 4" git rebase -i --root &&
>> > +       test E = $(git cat-file commit HEAD | sed -ne \$p)
>> > +'
>>
>> Shouldn't you also explicitly test "warn" and "ignore" modes?
>
> I don't think testing "ignore" is really necessary since it
> corresponds to the default behaviour, it is thus silently tested by
> the other tests.

Although the underlying behavior of "ignore" may be the same as the
default, you also want to check that higher-level machinery for
recognizing the "ignore" option is working correctly, which is why
checking "ignore" explicitly is a reasonable thing to do.

> Either way, I will add a test for "warn".

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

[Remi Galan Alfonso]

Eric Sunshine <sunshine@sunshineco.com> writes: 
> Although the underlying behavior of "ignore" may be the same as the 
> default, you also want to check that higher-level machinery for 
> recognizing the "ignore" option is working correctly, which is why 
> checking "ignore" explicitly is a reasonable thing to do. 

Noted, I'm adding a test for "ignore". 

Thank you, 

Rémi 

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

[Junio C Hamano]

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

> Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
>
>> Check if commits were removed (i.e. a line was deleted) and print
>> warnings or abort git rebase according to the value of the
>> configuration variable rebase.checkLevel.
>
> checkLevel does not seem to be a good name, because it doesn't say
> _what_ is checked. I don't have really good proposal in mind, but maybe
>
> rebase.todoListCheckLevel
> rebase.missinLinesCheckLevel

Yeah, with s/sin/sing/, but the above makes more sense.  It may be
that we probably do not even need "Level" there in the name.

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

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes: 
>> As long as what is given to 'drop' 
>> is checked when it matters (e.g. when the code in patch 2/2 tries 
>> see if some commits in the original list are no longer there in 
>> order to warn sees "drop foo bar" where "foo" is obviously not an 
>> object name in the original list, that should be checked), it is 
>> fine. And I agree 1/2 is not the place to do so, even though it may 
>> be easier from the implementation point of view (which is why I 
>> mentioned the possibility in the review of that patch). 
>
> I disagree, I think that that either the checking for the 'drop' 
> command should either be in the 1/2 where it is introduced or in the 
> function check_commits introduced by 2/2 but in a separate 
> commit/patch. 
> The 2/2 checks if there are removed commits to have the possibility to 
> avoid silent loss of information. It is not its role to check if the 
> SHA-1 following 'drop' are correct.

Suppose you started from this insn sheet:

    pick 2c9c1c5 gostak: distim doshes
    pick e3b601d pull: use git-rev-parse...
    pick eb2a8d9 pull: handle git-fetch'...

and then after letting the user edit, you got this back:

    pick 2c9c1c5 gostak: distim doshes
    drop e3b601d pull: use git-rev-parse...
    edit eb2a8d9 pull: handle git-fetch'...

In the new world order to punish those who simply remove lines to
signal that they want the commits omitted from replaying, you would
want to see all commit object names that was in the original insn
sheet appear in the post-edit insn sheet.  I'd presume that the way
to do so is to collect all the object names from each insn sheet and
compute the set difference.  The first one has three commit object
names, the same three commit object names appear in the second one,
and all is well.

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

As a part of "collecting object names from the list before and after
editing into two separate sets, and computing the set difference in
order to notice potential mistakes", you would need to make sure
that you got these two sets collected _correctly_, but you do not
know from the above sample input what the user wanted to do with the
first line.  Did the user tried to drop something else but the
object name has gone missing by mistake?  Did the user wanted to
drop the first one but made mistake while editing 'pick' away into
'drop'?

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

So logically I would think 2/2 is where the verification should
happen, but doing it as a part of 1/2 may be easier to do.  The end
result would not make a difference, and that is why I said it would
be OK either way.

I am puzzled as to what you are disagreeing with, and why.

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

[Remi Galan Alfonso]

Junio C Hamano <gitster@pobox.com> writes: 
> As long as what is given to 'drop' 
> is checked when it matters (e.g. when the code in patch 2/2 tries 
> see if some commits in the original list are no longer there in 
> order to warn sees "drop foo bar" where "foo" is obviously not an 
> object name in the original list, that should be checked), it is 
> fine. And I agree 1/2 is not the place to do so, even though it may 
> be easier from the implementation point of view (which is why I 
> mentioned the possibility in the review of that patch). 

I disagree, I think that that either the checking for the 'drop' 
command should either be in the 1/2 where it is introduced or in the 
function check_commits introduced by 2/2 but in a separate 
commit/patch. 
The 2/2 checks if there are removed commits to have the possibility to 
avoid silent loss of information. It is not its role to check if the 
SHA-1 following 'drop' are correct. 

What I think should be the best for now is checking the SHA-1 
following 'drop' in 1/2 (or not checking at all) and change the whole 
checking system of rebase in a later patch (e.g. check beforehand 
(probably in check_commits) if the commands exist, if the SHA-1 are 
correct and possibly other things). 

Rémi 

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

[Junio C Hamano]

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

> Discussions on this list often lead to "Oh, BTW, shall we do XYZ also?",
> but you shouldn't take this kind of remark as blocking (as long as XYZ
> is not incompatible with your patch, which is the case here).

Yeah, thanks for clarification.  As long as what is given to 'drop'
is checked when it matters (e.g. when the code in patch 2/2 tries
see if some commits in the original list are no longer there in
order to warn sees "drop foo bar" where "foo" is obviously not an
object name in the original list, that should be checked), it is
fine.  And I agree 1/2 is not the place to do so, even though it may
be easier from the implementation point of view (which is why I
mentioned the possibility in the review of that patch).

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

[Matthieu Moy]

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

>> Ideally, I think we should do a sanity check before starting the rebase,
>> and error out if we encounter an invalid command, a command that should
>> be followed by a valid sha1 and does not, ...
>> 
>> But currently, we do the verification while applying commands, and I
>> don't think there's anything really helpful to do if we encounter a
>> "drop this-is-not-a-sha1" command.
>
> While I agree that doing a sanity check before starting the rebase is
> a better idea, it would not be logic to do so only for the 'drop'
> command, it should be done for all of the various commands (as you
> said, checking if the commands are valid, checking if the SHA-1
> following the command is valid if it was expecting one...).

Yes, that's what I had in mind.

> However I think that it should be in a different patch, changing the
> whole checking system of rebase is not directly linked to the idea of
> the 'drop' command.

Agreed.

Discussions on this list often lead to "Oh, BTW, shall we do XYZ also?",
but you shouldn't take this kind of remark as blocking (as long as XYZ
is not incompatible with your patch, which is the case here).

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

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

[Remi Galan Alfonso]

> Ideally, I think we should do a sanity check before starting the rebase,
> and error out if we encounter an invalid command, a command that should
> be followed by a valid sha1 and does not, ...
> 
> But currently, we do the verification while applying commands, and I
> don't think there's anything really helpful to do if we encounter a
> "drop this-is-not-a-sha1" command.

While I agree that doing a sanity check before starting the rebase is
a better idea, it would not be logic to do so only for the 'drop'
command, it should be done for all of the various commands (as you
said, checking if the commands are valid, checking if the SHA-1
following the command is valid if it was expecting one...).

However I think that it should be in a different patch, changing the
whole checking system of rebase is not directly linked to the idea of
the 'drop' command.

Rémi

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

[Matthieu Moy]

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

> Junio C Hamano <gitster@pobox.com> writes:
>> Is this sufficient?
>
>> If you are going to do something in 2/2 that relies on the format of
>> this line being correct (as opposed to "noop" or "#" that can have
>> any garbage on the remainder of the line), wouldn't you want to at
>> least check $sha1 is sensible?
>
> That's also something that I was wondering, I wrote about it in the
> 0/2 part of this patch, I wanted some opinion about it.

Ideally, I think we should do a sanity check before starting the rebase,
and error out if we encounter an invalid command, a command that should
be followed by a valid sha1 and does not, ...

But currently, we do the verification while applying commands, and I
don't think there's anything really helpful to do if we encounter a
"drop this-is-not-a-sha1" command.

So, IMHO, why not do this check, but I'm not sure it will actually help
much.

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

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

[Remi Galan Alfonso]

Junio C Hamano <gitster@pobox.com> writes:
> Is this sufficient?

> If you are going to do something in 2/2 that relies on the format of
> this line being correct (as opposed to "noop" or "#" that can have
> any garbage on the remainder of the line), wouldn't you want to at
> least check $sha1 is sensible?

That's also something that I was wondering, I wrote about it in the
0/2 part of this patch, I wanted some opinion about it.
If there is no opposition on the subject, I will have it ready for
the v3 of the patch.

Quote of that part of the 0/2 for more clarity:
Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> For the 'drop' command, maybe instead of just doing the same thing as
> noop, checking if the SHA-1 that supposedly follow does exist could be
> a good idea.

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

[Matthieu Moy]

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

> Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
>
>> 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>
>> ---
>
> Matthieu, is this part of your class project?

Yes it is.

> I vaguely recall that your school wants your sign-off to release
> patches to us or something like that, and that I saw some other
> patches came with your sign-off, so I am being curious.

There's no strict requirement, but since the students are working under
my guidance I think it makes sense to have my sign-off. The rule which
applies by default for students is "don't distribute your code"
(symmetrical to "don't use other students code"), which obviously does
not apply for this project: I do allow them to contribute their code!

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

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

[Junio C Hamano]

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

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

Matthieu, is this part of your class project?

I vaguely recall that your school wants your sign-off to release
patches to us or something like that, and that I saw some other
patches came with your sign-off, so I am being curious.

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

Is this sufficient?

If you are going to do something in 2/2 that relies on the format of
this line being correct (as opposed to "noop" or "#" that can have
any garbage on the remainder of the line), wouldn't you want to at
least check $sha1 is sensible?

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

[Galan Rémi]

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

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

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1d01baa..9cf3760 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -514,6 +514,9 @@ rebasing.
 If you just want to edit the commit message for a commit, replace the
 command "pick" with the command "reword".
 
+To drop a commit, replace the command "pick" with "drop", or just
+delete its line.
+
 If you want to fold two or more commits into one, replace the command
 "pick" for the second and subsequent commits with "squash" or "fixup".
 If the commits had different authors, the folded commit will be
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index bab0dcc..2882276 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" || 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..1bad068 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1102,4 +1102,15 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
 	test $(git cat-file commit HEAD | sed -ne \$p) = I
 '
 
+test_expect_success 'drop' '
+	git checkout master &&
+	test_when_finished "git checkout master" &&
+	git checkout -b dropBranchTest master &&
+	set_fake_editor &&
+	FAKE_LINES="1 drop 2 3 drop 4 5" git rebase -i --root &&
+	test E = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test C = $(git cat-file commit HEAD^ | sed -ne \$p) &&
+	test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
+'
+
 test_done
-- 
2.4.1.363.g9535a9c