[PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Galan Rémi]

Instead of removing a line to remove the commit, you can use the key
word "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    |  4 ++++
 t/lib-rebase.sh               |  4 ++--
 t/t3404-rebase-interactive.sh | 11 +++++++++++
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1d01baa..3cd2ef2 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".
 
+If you want to remove a commit, replace the command "pick" by the
+command "drop".
+
 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..cb749e8 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.
 
@@ -515,6 +516,9 @@ do_next () {
 		do_pick $sha1 "$rest"
 		record_in_rewritten $sha1
 		;;
+	drop|d)
+		mark_action_done
+		;;
 	reword|r)
 		comment_for_reflog reword
 
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.174.g28bfe8e

[PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Galan Rémi]

Check if commits were removed (i.e. a line was deleted) or dupplicated
(e.g. the same commit is picked twice), can 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 "IGNORED", 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.

Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>
---
 This part of the patch has no test yet, it is more for rfc.

 Documentation/config.txt     |  8 +++++
 Documentation/git-rebase.txt |  5 +++
 git-rebase--interactive.sh   | 76 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d44bc85..2152e27 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2204,6 +2204,14 @@ 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.
+
 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 3cd2ef2..cb05cbb 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -213,6 +213,11 @@ 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. No check by default.
+
 OPTIONS
 -------
 --onto <newbase>::
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index cb749e8..8a837ca 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -837,6 +837,80 @@ 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 < "$temp_file"
+	do
+		case "$command" in
+		x|"exec")
+			;;
+		*)
+			echo "$sha1" >> "$2"
+			;;
+		esac
+		sed -i '1d' "$temp_file"
+	done
+	rm "$temp_file"
+}
+
+# Check if the user dropped some commits by mistake
+# or if there are two identical commits.
+# Behaviour determined by .gitconfig.
+check_commits () {
+	checkLevel=$(git config --get rebase.checkLevel)
+	checkLevel=${checkLevel:-"IGNORE"}
+	# To uppercase
+	checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
+
+	case "$checkLevel" in
+	"WARN"|"ERROR")
+		todo_list_to_sha_list "$todo".backup "$todo".oldsha1
+		todo_list_to_sha_list "$todo" "$todo".newsha1
+
+		duplicates=$(sort "$todo".newsha1 | uniq -d)
+
+		echo "$(sort -u "$todo".oldsha1)" > "$todo".oldsha1
+		echo "$(sort -u "$todo".newsha1)" > "$todo".newsha1
+		missing=$(comm -2 -3 "$todo".oldsha1 "$todo".newsha1)
+
+		# check missing commits
+		if ! test -z "$missing"
+		then
+			warn "Warning : some commits may have been dropped accidentally."
+			warn "Dropped commits:"
+			warn "$missing"
+			warn "To avoid this message, use \"drop\" to explicitely remove a commit."
+			warn "Use git --config rebase.checkLevel to change"
+			warn "the level of warnings (ignore,warn,error)."
+			warn ""
+
+			if test "$checkLevel" = "ERROR"
+			then
+				die_abort "Rebase aborted due to dropped commits."
+			fi
+		fi
+
+		# check duplicate commits
+		if ! test -z "$duplicates"
+		then
+			warn "Warning : some commits have been used twice:"
+			warn "$duplicates"
+			warn ""
+		fi
+		;;
+	"IGNORE")
+		;;
+	*)
+		warn "Unrecognized setting 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
@@ -1082,6 +1156,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"
-- 
2.4.1.174.g28bfe8e

Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Eric Sunshine]

On Tue, May 26, 2015 at 5:38 PM, Galan Rémi
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> git rebase -i: Warn removed or dupplicated commits

s/dupplicated/duplicated/

Also, drop capitalization, and insert "about":

    git rebase -i: warn about removed or duplicated commits

> Check if commits were removed (i.e. a line was deleted) or dupplicated

s/dupplicated/duplicated/

> (e.g. the same commit is picked twice), can print warnings or abort

s/can/and/, I think.

> git rebase according to the value of the configuration variable
> rebase.checkLevel.
>
> Add the configuration variable rebase.checkLevel.
>     - When unset or set to "IGNORED", no checking is done.

s/IGNORED/IGNORE/

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

Why uppercase for these names? Is there precedence for that? I think
lowercase is more common.

> Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>
> ---
>  This part of the patch has no test yet, it is more for rfc.
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d44bc85..2152e27 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2204,6 +2204,14 @@ 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.

The commit message talks about "ignore", but there is no mention here.

Also, what is the default behavior if not specified? That should be documented.

Finally, this talks about lowercase "warn" and "error", whereas the
commit message uses upper case "WARN" and "ERROR", as does the code.
Why the inconsistency?

>  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 3cd2ef2..cb05cbb 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -213,6 +213,11 @@ 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. No check by default.

Ditto: Fails to mention "ignore".

>  OPTIONS
>  -------
>  --onto <newbase>::
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index cb749e8..8a837ca 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -837,6 +837,80 @@ 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 < "$temp_file"

On this project it is typical to drop the space after redirection
operators (<, >, >>), however, git-rebase--interactive.sh is filled
with both styles (space and no space after redirection). New code
probably ought to drop the space.

> +       do
> +               case "$command" in
> +               x|"exec")
> +                       ;;
> +               *)
> +                       echo "$sha1" >> "$2"
> +                       ;;
> +               esac
> +               sed -i '1d' "$temp_file"
> +       done
> +       rm "$temp_file"
> +}
> +
> +# Check if the user dropped some commits by mistake
> +# or if there are two identical commits.
> +# Behaviour determined by .gitconfig.
> +check_commits () {
> +       checkLevel=$(git config --get rebase.checkLevel)
> +       checkLevel=${checkLevel:-"IGNORE"}

Minor aside: Unnecessary quoting increases the noise level, thus
making the code slightly more difficult to read. This could just as
well have been:

    checkLevel=${checkLevel:-IGNORE}

There are plenty of other places throughout this patch which exhibit
the same shortcoming, but I won't point them out individually.

> +       # To uppercase
> +       checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')

Is there precedence elsewhere for recognizing uppercase and lowercase
variants of config values?

> +       case "$checkLevel" in
> +       "WARN"|"ERROR")
> +               todo_list_to_sha_list "$todo".backup "$todo".oldsha1
> +               todo_list_to_sha_list "$todo" "$todo".newsha1
> +
> +               duplicates=$(sort "$todo".newsha1 | uniq -d)
> +
> +               echo "$(sort -u "$todo".oldsha1)" > "$todo".oldsha1
> +               echo "$(sort -u "$todo".newsha1)" > "$todo".newsha1
> +               missing=$(comm -2 -3 "$todo".oldsha1 "$todo".newsha1)
> +
> +               # check missing commits
> +               if ! test -z "$missing"

Isn't "! test -z" just a verbose way of saying "test -n"?

> +               then
> +                       warn "Warning : some commits may have been dropped accidentally."
> +                       warn "Dropped commits:"
> +                       warn "$missing"
> +                       warn "To avoid this message, use \"drop\" to explicitely remove a commit."

s/explicitely/explicitly/

> +                       warn "Use git --config rebase.checkLevel to change"
> +                       warn "the level of warnings (ignore,warn,error)."
> +                       warn ""
> +
> +                       if test "$checkLevel" = "ERROR"
> +                       then
> +                               die_abort "Rebase aborted due to dropped commits."
> +                       fi
> +               fi
> +
> +               # check duplicate commits
> +               if ! test -z "$duplicates"
> +               then
> +                       warn "Warning : some commits have been used twice:"
> +                       warn "$duplicates"
> +                       warn ""
> +               fi

Shouldn't this case also 'die' when rebase.checkLevel is "error"? And,
why doesn't the user get advice about configuring rebase.checkLevel in
this case?

In fact, the current logic flow seems a bit borked. I would have
expected it to be more like this:

    if test -n "$missing"
    then
        ...warn about accidental drops...
    fi

    if test -n "$duplicates"
    then
        ...warn about accidental duplicates...
    fi

    if test -n "$missing$duplicates"
    then
        ...show advice about configuring rebase.checkLevel...

        if test $checkLevel = ERROR
        then
            die_abort "..."
        fi
    fi

> +               ;;
> +       "IGNORE")
> +               ;;
> +       *)
> +               warn "Unrecognized setting for option rebase.checkLevel in git rebase -i"

This message might be more useful if it mentioned the actual unrecognized value.

> +               ;;
> +       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
> @@ -1082,6 +1156,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"
> --
> 2.4.1.174.g28bfe8e

Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Remi Galan Alfonso]

Thank you for reviewing the code. 

Eric Sunshine<sunshine@sunshineco.com> writes:
> > +       # To uppercase
> > +       checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
> 
> Is there precedence elsewhere for recognizing uppercase and lowercase
> variants of config values?

It seems to be commonly used when parsing options in the C files
through strcasecmp.  For exemple, in config.c:818 :
if (!strcmp(var, "core.safecrlf")) {
	if (value && !strcasecmp(value, "warn")) {
		[...]
However we didn't see any precedence in shell files. Do you think we
should remove it?

Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Eric Sunshine]

On Wed, May 27, 2015 at 9:19 AM, Remi Galan Alfonso
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> Eric Sunshine<sunshine@sunshineco.com> writes:
>> > +       # To uppercase
>> > +       checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
>>
>> Is there precedence elsewhere for recognizing uppercase and lowercase
>> variants of config values?
>
> It seems to be commonly used when parsing options in the C files
> through strcasecmp.  For exemple, in config.c:818 :
> if (!strcmp(var, "core.safecrlf")) {
>         if (value && !strcasecmp(value, "warn")) {
>                 [...]
> However we didn't see any precedence in shell files. Do you think we
> should remove it?

Precedence in C code is good enough for me, and it makes sense for
your new code to follow suit by being insensitive to case (as you have
already done).

However, it would be a good idea to be consistent in your use of
uppercase/lowercase in the commit message, documentation, and code,
rather than having a mix. I'd suggest sticking with lowercase
throughout since lowercase is more commonly used in the codebase (and
just easier to read).

Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Junio C Hamano]

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

> Thank you for reviewing the code. 
>
> Eric Sunshine<sunshine@sunshineco.com> writes:
>> > +       # To uppercase
>> > +       checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
>> 
>> Is there precedence elsewhere for recognizing uppercase and lowercase
>> variants of config values?
>
> It seems to be commonly used when parsing options in the C files
> through strcasecmp.  For exemple, in config.c:818 :
> if (!strcmp(var, "core.safecrlf")) {
> 	if (value && !strcasecmp(value, "warn")) {
> 		[...]
> However we didn't see any precedence in shell files. Do you think we
> should remove it?

I think there is a difference between (silently) accepting just to
be lenient and documenting and advocating mixed case uses.

Personally, I'd rather not to see gratuitous flexibility to allow
the same thing spelled in 47 different ways for no good reason.

Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Remi Galan Alfonso]

Junio C Hamano <gitster@pobox.com> writes:
> I think there is a difference between (silently) accepting just to
> be lenient and documenting and advocating mixed case uses.
> 
> Personally, I'd rather not to see gratuitous flexibility to allow
> the same thing spelled in 47 different ways for no good reason.

It was more of a mistake on our part rather than actually wanting to
document mixed case uses.

In the v2 of the patch (not sent to the mailing list yet since we want
to take into consideration the conclusion of this discussion before)
it is entirely in lower case in both the documentation and the code
while we silently allow upper and mixed case.

Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>> I think there is a difference between (silently) accepting just to
>> be lenient and documenting and advocating mixed case uses.
>> 
>> Personally, I'd rather not to see gratuitous flexibility to allow
>> the same thing spelled in 47 different ways for no good reason.
>
> It was more of a mistake on our part rather than actually wanting to
> document mixed case uses.
>
> In the v2 of the patch (not sent to the mailing list yet since we want
> to take into consideration the conclusion of this discussion before)
> it is entirely in lower case in both the documentation and the code
> while we silently allow upper and mixed case.

Understood; I am not sold on the whole "warning" business, though.

I think I saw you did 'tr [:upper:]' or something like that in the
patch; we tend to avoid [:class:] and [=equiv=] when not needed,
unless we know that the matching engine used supports them (i.e. it
is OK to use them in Perl scripts and it is OK to feed them to the
wildmatch-based matcher in Git itself, but not in general shell
scripts).  As the values can all be represented in US-ASCII, it
should be sufficient to do "tr 'A-Z' 'a-z'", I would think.

Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Remi Galan Alfonso]

Eric Sunshine<sunshine@sunshineco.com> writes:
> Shouldn't this case also 'die' when rebase.checkLevel is "error"? And,
> why doesn't the user get advice about configuring rebase.checkLevel in
> this case?
Stephen Kelly<steveire@gmail.com> writes:
> I sometimes duplicate commits deliberately if I want to split a commit in
> two.
Matthieu Moy<Matthieu.Moy@grenoble-inp.fr> writes:
> The more I think about it, the more I think we should either not warn at
> all on duplicate commits, or have a separate config variable.
Put in common because two config variables would have an effect on the
'die' and advise part.

In this patch we didn't put the 'die' in the duplicate commit part
since there was only one config variable and there are cases where the
user might want to duplicate commits.

After the code reviewing of Eric Sunshine and Stephen Kelly, we also
came to the conclusion that we should use two config variables, one
about missing commits and the other about duplicate commits.

This way if you deliberately want to use duplicate commits, you can
just set the value to 'ignore' for duplicate commits and still have
'warn'/'error' for missing commits. Moreover, each part would have its
'die' depending on the value of the corresponding config variable.

Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Stephen Kelly]

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

> 
> Check if commits were removed (i.e. a line was deleted) or dupplicated
> (e.g. the same commit is picked twice), can print warnings or abort
> git rebase according to the value of the configuration variable
> rebase.checkLevel.

I sometimes duplicate commits deliberately if I want to split a commit in
two. I move a copy up and fix the conflict, and I know that I'll still get
the right thing later even if I make a mistake with the conflict resolution.

Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Matthieu Moy]

Stephen Kelly <steveire@gmail.com> writes:

> Galan Rémi <remi.galan-alfonso <at> ensimag.grenoble-inp.fr> writes:
>
>> 
>> Check if commits were removed (i.e. a line was deleted) or dupplicated
>> (e.g. the same commit is picked twice), can print warnings or abort
>> git rebase according to the value of the configuration variable
>> rebase.checkLevel.
>
> I sometimes duplicate commits deliberately if I want to split a commit in
> two. I move a copy up and fix the conflict, and I know that I'll still get
> the right thing later even if I make a mistake with the conflict
> resolution.

The more I think about it, the more I think we should either not warn at
all on duplicate commits, or have a separate config variable.

It's rare to duplicate by mistake, and when you do so, it's already easy
to notice: you get conflicts, and you can git rebase --skip the second
occurence. Accidentally dropped commits are another story: it's rather
easy to cut-and-forget-to-paste, and the consequence currently is silent
data loss ...

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

Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits

[Junio C Hamano]

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

> Stephen Kelly <steveire@gmail.com> writes:
>
>> Galan Rémi <remi.galan-alfonso <at> ensimag.grenoble-inp.fr> writes:
>>
>>> 
>>> Check if commits were removed (i.e. a line was deleted) or dupplicated
>>> (e.g. the same commit is picked twice), can print warnings or abort
>>> git rebase according to the value of the configuration variable
>>> rebase.checkLevel.
>>
>> I sometimes duplicate commits deliberately if I want to split a commit in
>> two. I move a copy up and fix the conflict, and I know that I'll still get
>> the right thing later even if I make a mistake with the conflict
>> resolution.
>
> The more I think about it, the more I think we should either not warn at
> all on duplicate commits, or have a separate config variable.

Yeah, I'd say we shouldn't warn, without configuration to keep
things simple.

>
> It's rare to duplicate by mistake, and when you do so, it's already easy
> to notice: you get conflicts, and you can git rebase --skip the second
> occurence. Accidentally dropped commits are another story: it's rather
> easy to cut-and-forget-to-paste, and the consequence currently is silent
> data loss ...

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Eric Sunshine]

On Tue, May 26, 2015 at 5:38 PM, Galan Rémi
<remi.galan-alfonso@ensimag.grenoble-inp.fr> wrote:
> git-rebase -i: Add key word "drop" to remove a commit

"key word" is unusual. More typical is "keyword". However, perhaps
"command" might be even better. Also, custom on this project is not to
capitalize, so:

    git-rebase -i: add command "drop" to remove a commit

> Instead of removing a line to remove the commit, you can use the key
> word "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.

Nicely explained.

Ditto regarding "key word".

> Signed-off-by: Galan Rémi <remi.galan-alfonso@ensimag.grenoble-inp.fr>
> ---
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 1d01baa..3cd2ef2 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".
>
> +If you want to remove a commit, replace the command "pick" by the
> +command "drop".

I think the existing method of removing a commit merits mention here. Perhaps:

    To drop a commit, delete its line or replace the command
    "pick" with "drop".

Or, if you want to emphasize "drop":

    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..cb749e8 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Johannes Schindelin]

Hi Rémi,

On 2015-05-26 23:38, Galan Rémi wrote:
> Instead of removing a line to remove the commit, you can use the key
> word "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.

Please note that you can already just comment-out the line if you need to keep a visual trace.

Alternatively, you can replace the `pick` command by `noop`.

If you really need the `drop` command (with which I am not 100% happy because I already envisage users appending a `drop A` to an edit script "pick A; pick B; pick C" and expecting A *not to be picked*), then it is better to just add the `drop` part to the already existing `noop` clause:

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f7deeb0..8355be8 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -489,7 +489,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)
 		mark_action_done
 		;;
 	pick|p)

Ciao,
Johannes

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Remi Galan Alfonso]

Thank you for reviewing the code.

Johannes Schindelin<johannes.schindelin@gmx.de> writes:
> Please note that you can already just comment-out the line if you need to keep a visual trace.
> 
> Alternatively, you can replace the `pick` command by `noop`.
> 
> If you really need the `drop` command (with which I am not 100%
> happy because I already envisage users appending a `drop A` to an
> edit script "pick A; pick B; pick C" and expecting A *not to be
> picked*)

It is true that drop has the same effect as noop or commenting,
however we thought that drop is more understandable for average users of
git. Moreover when using git rebase -i, the 'help' displayed below the
list of commits doesn't mention neither the noop command nor the
effect of commenting the line (though considering what removing a line
does, it can be easily deduced).

The drop command was inspired by the drop command from histedit in
mercurial.

It also has some effects with the second part of this patch (checks
removed and/or duplicated commits): if you comment the line, the
commit will be considered as removed, thus ending in a warning if the
config variable is set to warn/error; however this problem won't
appear with noop.

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Matthieu Moy]

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

> It also has some effects with the second part of this patch (checks
> removed and/or duplicated commits): if you comment the line, the
> commit will be considered as removed, thus ending in a warning if the
> config variable is set to warn/error; however this problem won't
> appear with noop.

Indeed, that's the whole point of having a "drop" command.

As an advice for your next submission: use "git send-email
--cover-letter", and explain the overall idea before the patches.

I personally prefer "drop" to "noop" as a command name: I understand
"noop" as a command without argument (useful to say "this is actually an
empty list of commands, not an empty file to ask rebase to abort"), but
I find it weird to write

noop <sha1> <title>

As Remi wrote, the inspiration comes from Mercurial. Perhaps we should
ask on the mercurial ml how happy they are with the name.

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

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Junio C Hamano]

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

> I find it weird to write
>
> noop <sha1> <title>

True, but then it can be spelled

    # <sha1> <title>

too, so do we still want 'drop'?  Unless we have a strong reason to
believe migrants from Hg cannot be (re)trained, personally, I'd feel
that we do not need this 'drop' thing.

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> I find it weird to write
>>
>> noop <sha1> <title>
>
> True, but then it can be spelled
>
>     # <sha1> <title>

I do find it weird too. "#" means "comment", which means "do as if it
was not there" to me. And in this case it does change the semantics once
you activate the safety feature: error out without the "# <sha1>
<title>", rebase dropping the commit if the comment is present.

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

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>
>>> I find it weird to write
>>>
>>> noop <sha1> <title>
>>
>> True, but then it can be spelled
>>
>>     # <sha1> <title>
>
> I do find it weird too. "#" means "comment", which means "do as if it
> was not there" to me. And in this case it does change the semantics once
> you activate the safety feature: error out without the "# <sha1>
> <title>", rebase dropping the commit if the comment is present.

Well, I do not agree with the premise that "A line was removed, the
user may have made a mistake, we need to warn about it" is a good
idea in the first place.  Removing an insn that is not wanted has
been the way to skip and not replay a change from the beginning of
the time, and users shouldn't be trained into thinking that somehow
is a bad practice by having such an option that warns.

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Stefan Beller]

On Wed, May 27, 2015 at 1:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>>
>>>> I find it weird to write
>>>>
>>>> noop <sha1> <title>
>>>
>>> True, but then it can be spelled
>>>
>>>     # <sha1> <title>
>>
>> I do find it weird too. "#" means "comment", which means "do as if it
>> was not there" to me. And in this case it does change the semantics once
>> you activate the safety feature: error out without the "# <sha1>
>> <title>", rebase dropping the commit if the comment is present.
>
> Well, I do not agree with the premise that "A line was removed, the
> user may have made a mistake, we need to warn about it" is a good
> idea in the first place.  Removing an insn that is not wanted has
> been the way to skip and not replay a change from the beginning of
> the time, and users shouldn't be trained into thinking that somehow
> is a bad practice by having such an option that warns.

Talking about ideas:
I sometimes have the wrong branch checked out when doing a small
fixup commit. So I want to drop that patch from the current branch
and apply it to another branch. Maybe an instruction like
cherry-pick-to-branch-(and-do-not-apply-here) would help me there.

On the other hand I do understand the reasoning for having more
safety features in rebase as that exposes lots of power and many people
find the power a bit daunting.

So maybe you don't want to check the rebase instructions, but rather
after the fact, when the rebase is done:

$ git rebase -i origin/master
Successfully rebased and updated refs/heads/mytopic
Rebased the following commits:
    0e33744 Document protocol version 2
    6b6e3a7 t5544: add a test case for the new protocol
    d6aff73 transport: get_refs_via_connect exchanges capabilities before refs.
    cbb6089 transport: connect_setup appends protocol version number
    0b86fa1 fetch-pack: use the configured transport protocol
    23ed0ff remote.h: add get_remote_capabilities, request_capabilities
    e18b6dc transport: add infrastructure to support a protocol version number
    fd8d40d upload-pack-2: Implement the version 2 of upload-pack
    bf781ae upload-pack: move capabilities out of send_ref
    4c9cb59 upload-pack: make client capability parsing code a separate function
Dropped the following commits:
    deadbee upload-pack: only accept capabilities on the first "want" line
New commits: (due to rewording, double picking, etc)
    c0ffee1 More Documentation

I'd guess you would construct the information from the reflog
(The line before "rebase -i (start)" in the reflog) delta'd against HEAD,
so it's a crude incantation of git log maybe?

Also we need to turn this off for the power users, though I'd welcome if
we'd make it default on in git 3. (Being maximally verbose is good for new
users I assume, and turning it off is easy for advanced folks, so we can do
that for all porcelain commands?)

>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Johannes Schindelin]

Hi Stefan,

On 2015-05-27 23:47, Stefan Beller wrote:
> On Wed, May 27, 2015 at 1:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Talking about ideas:
> I sometimes have the wrong branch checked out when doing a small
> fixup commit. So I want to drop that patch from the current branch
> and apply it to another branch. Maybe an instruction like
> cherry-pick-to-branch-(and-do-not-apply-here) would help me there.

Oh, is it wish-anything time? *claps-his-hands*

I would wish for a graphical tool which visualizes the commit graph in a visually pleasing manner, where I can select one or more commits and drop them onto a commit in the graph, and then it goes and magically cherry-picks-and-drops them.

:-)

Ciao,
Dscho

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Stefan Beller]

On Thu, May 28, 2015 at 10:06 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Hi Stefan,
>
> On 2015-05-27 23:47, Stefan Beller wrote:
>> On Wed, May 27, 2015 at 1:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Talking about ideas:
>> I sometimes have the wrong branch checked out when doing a small
>> fixup commit. So I want to drop that patch from the current branch
>> and apply it to another branch. Maybe an instruction like
>> cherry-pick-to-branch-(and-do-not-apply-here) would help me there.
>
> Oh, is it wish-anything time? *claps-his-hands*

Maybe my wording was bad, sorry about that.
I think throwing around ideas (which are closely related to what
is trying to be accomplished here IMHO) is not necessarily bad,
but the exchange of ideas helps in understanding the issue better
("I like your idea as I have not thought about it that way.", "What about
use case X", Your idea is nuts because Y")

>
> I would wish for a graphical tool which visualizes the commit graph in a
> visually pleasing manner, where I can select one or more commits and drop
> them onto a commit in the graph, and then it goes and magically cherry-picks-and-drops them.

Drag and Drop, I get it. ;)

Additionally, if dropped on an unnamed branch, it should come up with
a reasonable
new branch name.

>
> :-)
>
> Ciao,
> Dscho
>

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Matthieu Moy]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Hi Stefan,
>
> On 2015-05-27 23:47, Stefan Beller wrote:
>> On Wed, May 27, 2015 at 1:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Talking about ideas:
>> I sometimes have the wrong branch checked out when doing a small
>> fixup commit. So I want to drop that patch from the current branch
>> and apply it to another branch. Maybe an instruction like
>> cherry-pick-to-branch-(and-do-not-apply-here) would help me there.
>
> Oh, is it wish-anything time? *claps-his-hands*
>
> I would wish for a graphical tool which visualizes the commit graph in
> a visually pleasing manner, where I can select one or more commits and
> drop them onto a commit in the graph, and then it goes and magically
> cherry-picks-and-drops them.

You need to argue a bit more to convince my students to schedule this
for the end of their projects ;-).

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

Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> I find it weird to write
>>
>> noop <sha1> <title>
>
> True, but then it can be spelled
>
>    # <sha1> <title>
>
> too, so do we still want 'drop'?  Unless we have a strong reason to
> believe migrants from Hg cannot be (re)trained, personally, I'd feel
> that we do not need this 'drop' thing.
>
To me, the addition of "drop" would be a better completion of the list 
of action verbs for 'normal' users.

Training/Retraining users to use atypical techniques is a never ending 
task, so making drop a synonym for the existing noop appeals to my 
experience of users (of all sorts of tools, including personal 
experience ;-).

--
Philip