Black smoke from git rebase -i exec

Black smoke from git rebase -i exec

[Ævar Arnfjörð Bjarmason]

There's some black smoke in pu after the git rebase -i series was
applied: http://smoke.git.nix.is/app/projects/report_details/14

Note: just the t3404-rebase-interactive.sh failure, not
t6040-tracking-info.sh, that's something else.

Here's the --verbose output from the test, hopefully that helps, if
not I can supply some additional info:

    Initialized empty Git repository in
/tmp/build-and-install-git-olpK/t/trash
directory.t3404-rebase-interactive/.git/
    expecting success:
    	test_commit A file1 &&
    	test_commit B file1 &&
    	test_commit C file2 &&
    	test_commit D file1 &&
    	test_commit E file3 &&
    	git checkout -b branch1 A &&
    	test_commit F file4 &&
    	test_commit G file1 &&
    	test_commit H file5 &&
    	git checkout -b branch2 F &&
    	test_commit I file6
    	git checkout -b conflict-branch A &&
    	for n in one two three four
    	do
    		test_commit $n conflict
    	done &&
    	git checkout -b no-conflict-branch A &&
    	for n in J K L M
    	do
    		test_commit $n file$n
    	done &&
    	git checkout -b no-ff-branch A &&
    	for n in N O P
    	do
    		test_commit $n file$n
    	done

    [master (root-commit) 6e62bf8] A
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file1
    [master 313fe96] B
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    [master d0f65f2] C
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file2
    [master 0547e3f] D
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    [master 8f99a4f] E
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file3
    Switched to a new branch 'branch1'
    [branch1 cfefd94] F
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file4
    [branch1 83751a6] G
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    [branch1 4373208] H
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file5
    Switched to a new branch 'branch2'
    [branch2 615be62] I
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 file6
    Switched to a new branch 'conflict-branch'
    [conflict-branch b895952] one
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 conflict
    [conflict-branch 766a798] two
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    [conflict-branch 1eadf03] three
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    [conflict-branch f91a2b3] four
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 1 deletions(-)
    Switched to a new branch 'no-conflict-branch'
    [no-conflict-branch 808874f] J
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileJ
    [no-conflict-branch 265b89e] K
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileK
    [no-conflict-branch 6b0f5e6] L
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileL
    [no-conflict-branch 3389558] M
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileM
    Switched to a new branch 'no-ff-branch'
    [no-ff-branch 53b4423] N
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileN
    [no-ff-branch cc47714] O
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileO
    [no-ff-branch faef1a5] P
     Author: A U Thor <author@example.com>
     1 files changed, 1 insertions(+), 0 deletions(-)
     create mode 100644 fileP
    ok 1 - setup

    expecting success:
    	git checkout master &&
    	FAKE_LINES="1 exec_touch_touch-one 2 exec_touch_touch-two
exec_false exec_touch_touch-three 3 4
    		exec_touch_\"touch-file__name_with_spaces\";_touch_touch-after-semicolon
5" \
    		test_must_fail git rebase -i A &&
    	test -f touch-one &&
    	test -f touch-two &&
    	! test -f touch-three &&
    	test $(git rev-parse C) = $(git rev-parse HEAD) || {
    		echo "Stopped at wrong revision:"
    		echo "($(git describe --tags HEAD) instead of C)"
    		false
    	} &&
    	git rebase --continue &&
    	test -f touch-three &&
    	test -f "touch-file  name with spaces" &&
    	test -f touch-after-semicolon &&
    	test $(git rev-parse master) = $(git rev-parse HEAD) || {
    		echo "Stopped at wrong revision:"
    		echo "($(git describe --tags HEAD) instead of master)"
    		false
    	} &&
    	rm -f touch-*

    Switched to a new branch 'master'
    Rebasing (4/4)
Successfully rebased and updated refs/heads/master.
    Stopped at wrong revision:
    (E instead of C)
    Stopped at wrong revision:
    (E instead of master)
    not ok - 2 rebase -i with the exec command
    #	
    #		git checkout master &&
    #		FAKE_LINES="1 exec_touch_touch-one 2 exec_touch_touch-two
exec_false exec_touch_touch-three 3 4
    #			exec_touch_\"touch-file__name_with_spaces\";_touch_touch-after-semicolon
5" \
    #			test_must_fail git rebase -i A &&
    #		test -f touch-one &&
    #		test -f touch-two &&
    #		! test -f touch-three &&
    #		test $(git rev-parse C) = $(git rev-parse HEAD) || {
    #			echo "Stopped at wrong revision:"
    #			echo "($(git describe --tags HEAD) instead of C)"
    #			false
    #		} &&
    #		git rebase --continue &&
    #		test -f touch-three &&
    #		test -f "touch-file  name with spaces" &&
    #		test -f touch-after-semicolon &&
    #		test $(git rev-parse master) = $(git rev-parse HEAD) || {
    #			echo "Stopped at wrong revision:"
    #			echo "($(git describe --tags HEAD) instead of master)"
    #			false
    #		} &&
    #		rm -f touch-*
    #	

    expecting success:
    	git checkout master &&
    	mkdir subdir && cd subdir &&
    	FAKE_LINES="1 exec_touch_touch-subdir" \
    		git rebase -i HEAD^ &&
    	cd .. &&
    	test -f touch-subdir &&
    	rm -fr subdir

    Already on 'master'
    rebase -i script before editing:
    pick 8f99a4f E

    rebase -i script after editing:
    pick 8f99a4f E
    exec touch touch-subdir
    Rebasing (2/2)
Executing: touch touch-subdir
    Successfully rebased and updated refs/heads/master.
    ok 3 - rebase -i with the exec command runs from tree root

    expecting success:
    	git checkout master &&
    	FAKE_LINES="exec_echo_foo_>file1 1" \
    		test_must_fail git rebase -i HEAD^ &&
    	test $(git rev-parse master^) = $(git rev-parse HEAD) || {
    		echo "Stopped at wrong revision:"
    		echo "($(git describe --tags HEAD) instead of master^)"
    		false
    	} &&
    	git reset --hard &&
    	git rebase --continue

    Already on 'master'
    Rebasing (1/1)
Successfully rebased and updated refs/heads/master.
    Stopped at wrong revision:
    (E instead of master^)
    not ok - 4 rebase -i with the exec command checks tree cleanness
    #	
    #		git checkout master &&
    #		FAKE_LINES="exec_echo_foo_>file1 1" \
    #			test_must_fail git rebase -i HEAD^ &&
    #		test $(git rev-parse master^) = $(git rev-parse HEAD) || {
    #			echo "Stopped at wrong revision:"
    #			echo "($(git describe --tags HEAD) instead of master^)"
    #			false
    #		} &&
    #		git reset --hard &&
    #		git rebase --continue
    #	

    # failed 2 among 4 test(s)
    1..4

(I modified the test to only run the failing tests)

Re: Black smoke from git rebase -i exec

[Matthieu Moy]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> There's some black smoke in pu after the git rebase -i series was
> applied: http://smoke.git.nix.is/app/projects/report_details/14

Strange, I can't reproduce this on my box (tried on RHEL x86_64 and
Debian i686).

>     expecting success:
...
>     	rm -f touch-*
>
>     Switched to a new branch 'master'

At this point, I get 

rebase -i script before editing:
pick 313fe96 B
pick d0f65f2 C
pick 0547e3f D
pick 8f99a4f E

rebase -i script after editing:
pick 313fe96 B
exec touch touch-one
pick d0f65f2 C
exec touch touch-two
exec false
exec touch touch-three
pick 0547e3f D
pick 8f99a4f E
exec touch "touch-file  name with spaces"; touch touch-after-semicolon

which you don't seem to get on your side. I get the same as you if I
comment out the "set_fake_editor" line at the top of the script. So, I
suspect there's something very wrong that prevents it from doing its
job.

Can you add some debug

echo "$EDITOR"
echo "$FAKE_EDITOR"

somewhere in the test to see what happens?

>     Rebasing (4/4)
> Successfully rebased and updated refs/heads/master.
>     Stopped at wrong revision:
>     (E instead of C)
>     Stopped at wrong revision:
>     (E instead of master)

(here, it's definitely doing as if the todolist had not been edited)

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

Re: Black smoke from git rebase -i exec

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 10, 2010 at 13:37, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Ęvar Arnfjörš Bjarmason <avarab@gmail.com> writes:
>
>> There's some black smoke in pu after the git rebase -i series was
>> applied: http://smoke.git.nix.is/app/projects/report_details/14
>
> Strange, I can't reproduce this on my box (tried on RHEL x86_64 and
> Debian i686).

Hi. The issue appears to be that there's some non-POSIX code in your
patch (but I didn't check what). The test works for me with bash, but
fails with dash (which is the Debian testing /bin/sh).

Can you try with dash or some other non-bash POSIX shell and see if it
fails?

Re: Black smoke from git rebase -i exec

[Johannes Sixt]

Am 8/10/2010 15:57, schrieb Ævar Arnfjörð Bjarmason:
> On Tue, Aug 10, 2010 at 13:37, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Ęvar Arnfjörš Bjarmason <avarab@gmail.com> writes:
>>
>>> There's some black smoke in pu after the git rebase -i series was
>>> applied: http://smoke.git.nix.is/app/projects/report_details/14
>>
>> Strange, I can't reproduce this on my box (tried on RHEL x86_64 and
>> Debian i686).
> 
> Hi. The issue appears to be that there's some non-POSIX code in your
> patch (but I didn't check what). The test works for me with bash, but
> fails with dash (which is the Debian testing /bin/sh).
> 
> Can you try with dash or some other non-bash POSIX shell and see if it
> fails?

The culprit is commands like these:

	FAKE_LINES="exec_echo_foo_>file1 1" \
		test_must_fail git rebase -i HEAD^ &&

You cannot apply single-command-export if the command is a shell function.
You must rewrite this as:

	(
		export FAKE_LINES="..." &&
		test_must_fail git rebase ....
	) &&

-- Hannes

Re: Black smoke from git rebase -i exec

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 10, 2010 at 14:12, Johannes Sixt <j.sixt@viscovery.net> wrote:
> You cannot apply single-command-export if the command is a shell function.
> You must rewrite this as:
>
>        (
>                export FAKE_LINES="..." &&
>                test_must_fail git rebase ....
>        ) &&

Except that's not portable either, it should be:

    FAKE_LINES="..." &&
    export FAKE_LINES &&
	test_must_fail git rebase ...

See the other examples in t3404-rebase-interactive.sh

Re: Black smoke from git rebase -i exec

[Matthieu Moy]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, Aug 10, 2010 at 14:12, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> You cannot apply single-command-export if the command is a shell function.
>> You must rewrite this as:
>>
>>        (
>>                export FAKE_LINES="..." &&
>>                test_must_fail git rebase ....
>>        ) &&
>
> Except that's not portable either, it should be:
>
>     FAKE_LINES="..." &&
>     export FAKE_LINES &&
> 	test_must_fail git rebase ...
>
> See the other examples in t3404-rebase-interactive.sh

Yes, I had found this. New patch comming soon.

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

[PATCH 1/2 (fix broken test)] rebase -i: add exec command to launch a shell command

[Matthieu Moy]

The typical usage pattern would be to run a test (or simply a compilation
command) at given points in history.

The shell command is ran (from the worktree root), and the rebase is
stopped when the command fails, to give the user an opportunity to fix
the problem before continuing with "git rebase --continue".

This needs a little rework of skip_unnecessary_picks, which wasn't robust
enough to deal with lines like

  exec >"file    name with many spaces"

in the todolist. The new version extracts command, sha1 and rest from
each line, but outputs the line itself verbatim to avoid changing the
whitespace layout.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
This fixes the non-POSIX behavior of the tests found by Ævar Arnfjörð
Bjarmason (FAKE_LINES=foo test_must_fail ... does not work).

Also, I replaced "touch foo" with ">foo" and found a small bug. This
is the skip_unnecessary_picks of the commit message and of the patch
below.

 Documentation/git-rebase.txt  |   24 ++++++++++++++++
 git-rebase--interactive.sh    |   38 +++++++++++++++++++++++--
 t/lib-rebase.sh               |    2 +
 t/t3404-rebase-interactive.sh |   61 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 122 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index be23ad2..9c68b66 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -459,6 +459,30 @@ sure that the current HEAD is "B", and call
 $ git rebase -i -p --onto Q O
 -----------------------------
 
+Reordering and editing commits usually creates untested intermediate
+steps.  You may want to check that your history editing did not break
+anything by running a test, or at least recompiling at intermediate
+points in history by using the "exec" command (shortcut "x").  You may
+do so by creating a todo list like this one:
+
+-------------------------------------------
+pick deadbee Implement feature XXX
+fixup f1a5c00 Fix to feature XXX
+exec make
+pick c0ffeee The oneline of the next commit
+edit deadbab The oneline of the commit after
+exec cd subdir; make test
+...
+-------------------------------------------
+
+The interactive rebase will stop when a command fails (i.e. exits with
+non-0 status) to give you an opportunity to fix the problem. You can
+continue with `git rebase --continue`.
+
+The "exec" command launches the command in a shell (the one specified
+in `$SHELL`, or the default shell if `$SHELL` is not set), so you can
+use shell features (like "cd", ">", ";" ...). The command is run from
+the root of the working tree.
 
 SPLITTING COMMITS
 -----------------
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b94c2a0..bf49b5b 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -537,6 +537,34 @@ do_next () {
 		esac
 		record_in_rewritten $sha1
 		;;
+	x|"exec")
+		read -r command rest < "$TODO"
+		mark_action_done
+		printf 'Executing: %s\n' "$rest"
+		# "exec" command doesn't take a sha1 in the todo-list.
+		# => can't just use $sha1 here.
+		git rev-parse --verify HEAD > "$DOTEST"/stopped-sha
+		${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
+		status=$?
+		if test "$status" -ne 0
+		then
+			warn "Execution failed: $rest"
+			warn "You can fix the problem, and then run"
+			warn
+			warn "	git rebase --continue"
+			warn
+			exit "$status"
+		fi
+		# Run in subshell because require_clean_work_tree can die.
+		if ! (require_clean_work_tree)
+		then
+			warn "Commit or stash your changes, and then run"
+			warn
+			warn "	git rebase --continue"
+			warn
+			exit 1
+		fi
+		;;
 	*)
 		warn "Unknown command: $command $sha1 $rest"
 		if git rev-parse --verify -q "$sha1" >/dev/null
@@ -591,10 +619,13 @@ do_rest () {
 # skip picking commits whose parents are unchanged
 skip_unnecessary_picks () {
 	fd=3
-	while read -r command sha1 rest
+	while read -r line
 	do
+		command=$(echo "$line" | sed 's/  */ /' | cut -d ' ' -f 1)
+		sha1=$(echo "$line"    | sed 's/  */ /' | cut -d ' ' -f 2)
+		rest=$(echo "$line"    | sed 's/  */ /' | cut -d ' ' -f 3-)
 		# fd=3 means we skip the command
-		case "$fd,$command,$(git rev-parse --verify --quiet $sha1^)" in
+		case "$fd,$command,$(git rev-parse --verify --quiet "$sha1"^)" in
 		3,pick,"$ONTO"*|3,p,"$ONTO"*)
 			# pick a commit whose parent is current $ONTO -> skip
 			ONTO=$sha1
@@ -606,7 +637,7 @@ skip_unnecessary_picks () {
 			fd=1
 			;;
 		esac
-		printf '%s\n' "$command${sha1:+ }$sha1${rest:+ }$rest" >&$fd
+		echo "$line" >&$fd
 	done <"$TODO" >"$TODO.new" 3>>"$DONE" &&
 	mv -f "$TODO".new "$TODO" &&
 	case "$(peek_next_command)" in
@@ -957,6 +988,7 @@ first and then run 'git rebase --continue' again."
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but discard this commit's log message
+#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 6aefe27..6ccf797 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -47,6 +47,8 @@ for line in $FAKE_LINES; do
 	case $line in
 	squash|fixup|edit|reword)
 		action="$line";;
+	exec*)
+		echo "$line" | sed 's/_/ /g' >> "$1";;
 	"#")
 		echo '# comment' >> "$1";;
 	">")
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9f03ce6..93b181e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -64,6 +64,67 @@ test_expect_success 'setup' '
 	done
 '
 
+# "exec" commands are ran with the user shell by default, but this may
+# be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
+# to create a file. Unseting SHELL avoids such non-portable behavior
+# in tests.
+SHELL=
+
+test_expect_success 'rebase -i with the exec command' '
+	git checkout master &&
+	(
+	FAKE_LINES="1 exec_>touch-one
+		2 exec_>touch-two exec_false exec_>touch-three
+		3 4 exec_>\"touch-file__name_with_spaces\";_>touch-after-semicolon 5" &&
+	export FAKE_LINES &&
+	test_must_fail git rebase -i A
+	) &&
+	test -f touch-one &&
+	test -f touch-two &&
+	! test -f touch-three &&
+	test $(git rev-parse C) = $(git rev-parse HEAD) || {
+		echo "Stopped at wrong revision:"
+		echo "($(git describe --tags HEAD) instead of C)"
+		false
+	} &&
+	git rebase --continue &&
+	test -f touch-three &&
+	test -f "touch-file  name with spaces" &&
+	test -f touch-after-semicolon &&
+	test $(git rev-parse master) = $(git rev-parse HEAD) || {
+		echo "Stopped at wrong revision:"
+		echo "($(git describe --tags HEAD) instead of master)"
+		false
+	} &&
+	rm -f touch-*
+'
+
+test_expect_success 'rebase -i with the exec command runs from tree root' '
+	git checkout master &&
+	mkdir subdir && cd subdir &&
+	FAKE_LINES="1 exec_>touch-subdir" \
+		git rebase -i HEAD^ &&
+	cd .. &&
+	test -f touch-subdir &&
+	rm -fr subdir
+'
+
+test_expect_success 'rebase -i with the exec command checks tree cleanness' '
+	git checkout master &&
+	(
+	FAKE_LINES="exec_echo_foo_>file1 1" &&
+	export FAKE_LINES &&
+	test_must_fail git rebase -i HEAD^
+	) &&
+	test $(git rev-parse master^) = $(git rev-parse HEAD) || {
+		echo "Stopped at wrong revision:"
+		echo "($(git describe --tags HEAD) instead of master^)"
+		false
+	} &&
+	git reset --hard &&
+	git rebase --continue
+'
+
 test_expect_success 'no changes are a nop' '
 	git checkout branch2 &&
 	git rebase -i F &&
-- 
1.7.2.1.52.g95e25.dirty

Re: [PATCH 1/2 (fix broken test)] rebase -i: add exec command to launch a shell command

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> The typical usage pattern would be to run a test (or simply a compilation
> command) at given points in history.
>
> The shell command is ran (from the worktree root), and the rebase is
> stopped when the command fails, to give the user an opportunity to fix
> the problem before continuing with "git rebase --continue".
>
> This needs a little rework of skip_unnecessary_picks, which wasn't robust
> enough to deal with lines like
>
>   exec >"file    name with many spaces"
>
> in the todolist. The new version extracts command, sha1 and rest from
> each line, but outputs the line itself verbatim to avoid changing the
> whitespace layout.

Thanks.

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 9f03ce6..93b181e 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -64,6 +64,67 @@ test_expect_success 'setup' '
>  	done
>  '
>  
> +# "exec" commands are ran with the user shell by default, but this may
> +# be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
> +# to create a file. Unseting SHELL avoids such non-portable behavior
> +# in tests.
> +SHELL=

Tricky but true.

Do we have other callouts that we use $SHELL from the environment?
execv_shell_cmd() just runs "sh -c" from $PATH so diff (when running
external diff) nor ll-merge (when running external merge driver) that call
it via run_command_v_opt(RUN_USING_SHELL) are immune to this issue.

Re: [PATCH 1/2 (fix broken test)] rebase -i: add exec command to launch a shell command

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> +# "exec" commands are ran with the user shell by default, but this may
>> +# be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
>> +# to create a file. Unseting SHELL avoids such non-portable behavior
>> +# in tests.
>> +SHELL=
>
> Tricky but true.
>
> Do we have other callouts that we use $SHELL from the environment?

Not as far as I know. "git grep SHELL" show mostly "SHELL_PATH", and
the testsuite passes for me with SHELL=zsh.

This exec command is a bit of a special case: I wanted the user to
keep the advanced features of his shell (for example, the ** wildcard
of zsh and recent bash), not just allow executing commands.

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

[PATCH 0/2] rebase -i: in-editor documentation nits

[Jonathan Nieder]

Matthieu Moy wrote:

> +++ b/git-rebase--interactive.sh
> @@ -957,6 +988,7 @@ first and then run 'git rebase --continue' again."
>  #  e, edit = use commit, but stop for amending
>  #  s, squash = use commit, but meld into previous commit
>  #  f, fixup = like "squash", but discard this commit's log message
> +#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
>  #
>  # If you remove a line here THAT COMMIT WILL BE LOST.
>  # However, if you remove everything, the rebase will be aborted.

Nit: the "exec" command is formatted differently from the commands
around it, making it stand out (which I don't think is intended).

While we're there, patch 2 adds some brief documentation for the
"noop" command.

Roughly based on [1] (which might be a nice patch to revive, by the
way).  Sane?

Jonathan Nieder (2):
  rebase -i: reword in-editor documentation of "exec"
  rebase -i: explain how to discard all commits

 git-rebase--interactive.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

[1] http://thread.gmane.org/gmane.comp.version-control.git/161120/focus=162079

[PATCH 1/2] rebase -i: reword in-editor documentation of "exec"

[Jonathan Nieder]

The argument to the "exec" insn represents a command to be passed to
the user's shell.  (At first I misread the description as meaning it
should itself be the name of a shell.)

While fixing that, format the description to more closely parallel
the descriptions of other commands.

Before:

 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but [...]
 #  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.

After:

 [...]
 #  f, fixup = like "squash", but [...]
 #  x, exec = run command using shell, and stop if it fails
 #
 # If you remove a line [...]

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
It would be nice to say "stop for amending if it fails" (or similar)
to make the relationship to the edit insn clearer, but it is not clear
how to make room for that.

 git-rebase--interactive.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a5ffd9a..09aeecf 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1021,7 +1021,7 @@ first and then run 'git rebase --continue' again."
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but discard this commit's log message
-#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
+#  x, exec = run command using shell, and stop if it fails
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
-- 
1.7.4.rc2

Re: [PATCH 1/2] rebase -i: reword in-editor documentation of "exec"

[Matthieu Moy]

Jonathan Nieder <jrnieder@gmail.com> writes:

> -#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
> +#  x, exec = run command using shell, and stop if it fails

I don't think this is a good change to remove the <cmd> part. All
other commands are used with

<command> <sha1> <subject line>

and I don't think the user would be able to guess that exec is
different without a hint.

If the problem is the wording of the sentence that may imply that
<cmd> should be the shell itself, then why not

x <cmd>, exec <cmd> = run command <cmd> using shell, and stop if it fails

?

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

Re: [PATCH 1/2] rebase -i: reword in-editor documentation of "exec"

[Junio C Hamano]

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

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> -#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
>> +#  x, exec = run command using shell, and stop if it fails
>
> I don't think this is a good change to remove the <cmd> part. All
> other commands are used with
>
> <command> <sha1> <subject line>
>
> and I don't think the user would be able to guess that exec is
> different without a hint.

I tend to agree with you here.

Re: [PATCH 1/2] rebase -i: reword in-editor documentation of "exec"

[Jonathan Nieder]

Matthieu Moy wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> -#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
>> +#  x, exec = run command using shell, and stop if it fails
>
> I don't think this is a good change to remove the <cmd> part. All
> other commands are used with
>
> <command> <sha1> <subject line>
>
> and I don't think the user would be able to guess that exec is
> different without a hint.
> 
> If the problem is the wording of the sentence that may imply that
> <cmd> should be the shell itself, then why not

Yes, sorry, I combined two problems into a single patch.  That was a
mistake.  The current cheat sheet says:

# Rebase 3f14246..a1d7e01 onto 3f14246
#
# Commands:
#  p, pick = use commit
#  r, reword = use commit, but edit the commit message
#  e, edit = use commit, but stop for amending
#  s, squash = use commit, but meld into previous commit
#  f, fixup = like "squash", but discard this commit's log message
#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
#
# If you remove a line here THAT COMMIT WILL BE LOST.
# However, if you remove everything, the rebase will be aborted.
#

This does not make it clear that the format of each line is

	<instruction> <commit id> <explanatory text that will be printed>

but the reader will probably infer that from the automatically
generated pick examples above.

What about the "exec" instruction?  By analogy, I might imagine that
the format of that line is

	exec <command> <explanatory text that will be printed>

So the "<cmd>" does not address that question for me.  It does succeed
in clarifying that "a shell command" does not mean an arbitrary shell
command but a user-specified one.

Meanwhile, it makes the cheat sheet harder to visually scan as a table

 i, instruction = action performed by instruction

Maybe "exec" should be explained outside this table?  For example,
maybe something along the lines of

	 x, exec = run an arbitrary command (see below)

	A line of the form "exec <command>" will run <command> using your
	shell and stop for investigation or amending if the command fails.

	If you remove a line here, THAT COMMIT WILL BE LOST.
	However, if you remove everything, the rebase will be aborted.

Re: [PATCH 1/2] rebase -i: reword in-editor documentation of "exec"

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Maybe "exec" should be explained outside this table?  For example,
> maybe something along the lines of
>
> 	 x, exec = run an arbitrary command (see below)

Ok, none of the other insns in the insn sheet mention what the argument to
the command means anyway (e.g. "p, pick = replay the commit" doesn't say
explicitly where the commit comes from), so I think the original patch is
probably fine.

If we wanted to be more helpful, perhaps s/(see below)/specified on the
rest of the line/ should be sufficient without adding extra lines.

[PATCH 1/2 v2] rebase -i: clarify in-editor documentation of "exec"

[Jonathan Nieder]

The hints in the current "instruction sheet" template look like so:

 # Rebase 3f14246..a1d7e01 onto 3f14246
 #
 # Commands:
 #  p, pick = use commit
 #  r, reword = use commit, but edit the commit message
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but discard this commit's log message
 #  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
 #

This does not make it clear that the format of each line is

	<insn> <commit id> <explanatory text that will be printed>

but the reader will probably infer that from the automatically
generated pick examples above it.

What about the "exec" instruction?  By analogy, I might imagine that
the format of that line is "exec <command> <explanatory text>", and
the "x <cmd>" hint does not address that question (at first I read it
as taking an argument <cmd> that is the name of a shell).  Meanwhile,
the mention of <cmd> makes the hints harder to scan as a table.

So remove the <cmd> and add some words to remind the reader that
"exec" runs a command named by the rest of the line.  To make room, it
is left to the manpage to explain that that command is run using
$SHELL and that nonzero status from that command will pause the
rebase.

Wording from Junio.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:

> If we wanted to be more helpful, perhaps s/(see below)/specified on the
> rest of the line/ should be sufficient without adding extra lines.

Sounds good.

 git-rebase--interactive.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a5ffd9a..a18c9b1 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1021,7 +1021,7 @@ first and then run 'git rebase --continue' again."
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
 #  f, fixup = like "squash", but discard this commit's log message
-#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
+#  x, exec = run command specified on the rest of the line
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
-- 
1.7.4.rc2

Re: [PATCH 1/2 v2] rebase -i: clarify in-editor documentation of "exec"

[Matthieu Moy]

Jonathan Nieder <jrnieder@gmail.com> writes:

> -#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
> +#  x, exec = run command specified on the rest of the line

I don't think dropping "shell" is a good idea. In this context,
"command" could mean "one of pick/fixup/squash/...", a Git command,
and at last, an arbitrary line of shell.

I agree that my "shell command" wording was confusing too, but maybe
just adding "using the shell" at the end of line would do it.
Otherwise, I prefered the "see below + 2 lines explanation" proposal
above.

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

Re: [PATCH 1/2 v2] rebase -i: clarify in-editor documentation of "exec"

[Jonathan Nieder]

Matthieu Moy wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> -#  x <cmd>, exec <cmd> = Run a shell command <cmd>, and stop if it fails
>> +#  x, exec = run command specified on the rest of the line
>
> I don't think dropping "shell" is a good idea. In this context,
> "command" could mean "one of pick/fixup/squash/...", a Git command,
> and at last, an arbitrary line of shell.

Hmm.  I suppose

# Commands:
#  p, pick = use commit
#  r, reword = use commit, but edit the commit message
#  e, edit = use commit, but stop for amending
#  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

would do?

Sorry to take so long to get this right.

Re: [PATCH 1/2 v2] rebase -i: clarify in-editor documentation of "exec"

[Matthieu Moy]

Jonathan Nieder <jrnieder@gmail.com> writes:

> # Commands:
> #  p, pick = use commit
> #  r, reword = use commit, but edit the commit message
> #  e, edit = use commit, but stop for amending
> #  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

I'm fine with that.

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

[PATCH 2/2] rebase -i: explain how to discard all commits

[Jonathan Nieder]

Preparing a patch series for submission (as explained under
INTERACTIVE MODE in the git rebase manual) sometimes involves
discarding commits representing changes that turned out to be a bad
idea.  Usually this is quite simple to do by deleting the appropriate
"pick" lines, but if all commits are removed then the "remove
everything means abort" logic kicks in and the rebase is cancelled.
One can override that behavior by adding a line with the text "noop".

This is a follow-up to v1.6.0.3~21 (rebase -i: do not fail when there
is no commit to cherry-pick, 2008-10-10).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-rebase--interactive.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 09aeecf..d9dfc75 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1025,6 +1025,7 @@ first and then run 'git rebase --continue' again."
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
+# Use the "noop" command if you really want to remove all commits.
 #
 EOF
 
-- 
1.7.4.rc2

[PATCH 2/2] Re: rebase -i: explain how to discard all commits

[Nicolas Sebrecht]

The 15/01/11, Jonathan Nieder wrote:
> Preparing a patch series for submission (as explained under
> INTERACTIVE MODE in the git rebase manual) sometimes involves
> discarding commits representing changes that turned out to be a bad
> idea.  Usually this is quite simple to do by deleting the appropriate
> "pick" lines, but if all commits are removed then the "remove
> everything means abort" logic kicks in and the rebase is cancelled.
> One can override that behavior by adding a line with the text "noop".
> 
> This is a follow-up to v1.6.0.3~21 (rebase -i: do not fail when there
> is no commit to cherry-pick, 2008-10-10).
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  git-rebase--interactive.sh |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 09aeecf..d9dfc75 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -1025,6 +1025,7 @@ first and then run 'git rebase --continue' again."
>  #
>  # If you remove a line here THAT COMMIT WILL BE LOST.
>  # However, if you remove everything, the rebase will be aborted.
> +# Use the "noop" command if you really want to remove all commits.
>  #
>  EOF

Sorry, I think it is confusing. With this help we could understand that
the "noop" will either

  (a) discard the interactive rebase

or

  (b) _really remove commits_ from that branch

I'm not sure to know how it will act myself. If (a), we could use
something like

  "However, if you remove everything or use the "noop" command, the rebase will be aborted."

but if we are in case (b), I guess it is not necessary and we should
point to the 'git reset' command.

-- 
Nicolas Sebrecht

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

[Jonathan Nieder]

Nicolas Sebrecht wrote:
> The 15/01/11, Jonathan Nieder wrote:

>> This is a follow-up to v1.6.0.3~21 (rebase -i: do not fail when there
>> is no commit to cherry-pick, 2008-10-10).
[...]
>>  # However, if you remove everything, the rebase will be aborted.
>> +# Use the "noop" command if you really want to remove all commits.
[...]
> Sorry, I think it is confusing. With this help we could understand that
> the "noop" will either
>
>   (a) discard the interactive rebase
>
> or
>
>   (b) _really remove commits_ from that branch
>
> I'm not sure to know how it will act myself. If (a), we could use
> something like
>
>   "However, if you remove everything or use the "noop" command, the rebase will be aborted."
>
> but if we are in case (b), I guess it is not necessary and we should
> point to the 'git reset' command.

Okay.  I agree that my particular wording was confusing.  Are you
saying the "noop" command in general is confusing?

The "noop" is itself a non-operation; if you combine "noop" with other
instructions then the noop itself will have no effect.  Meanwhile if
you have _no_ instructions then the rebase is cancelled, while if you
have a single "noop" instruction, that means "I have discarded all the
commits, but please rebase anyway".

Jonathan

[PATCH 2/2] Re: rebase -i: explain how to discard all commits

[Nicolas Sebrecht]

The 20/01/11, Jonathan Nieder wrote:

> Okay.  I agree that my particular wording was confusing.  Are you
> saying the "noop" command in general is confusing?
> 
> The "noop" is itself a non-operation; if you combine "noop" with other
> instructions then the noop itself will have no effect.  Meanwhile if
> you have _no_ instructions then the rebase is cancelled, while if you
> have a single "noop" instruction, that means "I have discarded all the
> commits, but please rebase anyway".

Ok, I think I get it now. What about adding

  Use "noop" with no other instruction to fallback to a non-interactive
  rebase. If other instructions are present, "noop" has no effect.

?

-- 
Nicolas Sebrecht

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

[Thomas Rast]

Nicolas Sebrecht wrote:
> The 20/01/11, Jonathan Nieder wrote:
> 
> > if you
> > have a single "noop" instruction, that means "I have discarded all the
> > commits, but please rebase anyway".
> 
> Ok, I think I get it now. What about adding
> 
>   Use "noop" with no other instruction to fallback to a non-interactive
>   rebase. If other instructions are present, "noop" has no effect.
> 
> ?

No, that's quite wrong.

The TODO list is the list of all commits that need to be rebased.  It
does not contain commits that (according to patch-id) are already
contained in the upstream (i.e., the base you are rebasing on).  If
the list is empty after filtering out such commits, rebase puts 'noop'
as the only command since "empty TODO" is already taken to mean
"abort"

If you then accept this 'noop' rebase, this effectively makes the
rebased branch the same as the base branch, sort of like resetting.

(I for one have never accepted such a rebase; if the TODO only
consists of noop, that means I made a mistake.)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> (I for one have never accepted such a rebase; if the TODO only
> consists of noop, that means I made a mistake.)

Wouldn't that suggest us that if we were to do anything to this message it
would be a good idea to teach the user to "reset --hard" the branch if no
commits truly needs to be replayed on top of the onto-commit?

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

[Johannes Schindelin]

Hi,

On Thu, 20 Jan 2011, Junio C Hamano wrote:

> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > (I for one have never accepted such a rebase; if the TODO only 
> > consists of noop, that means I made a mistake.)
> 
> Wouldn't that suggest us that if we were to do anything to this message 
> it would be a good idea to teach the user to "reset --hard" the branch 
> if no commits truly needs to be replayed on top of the onto-commit?

The important difference between rebase -i && noop on the one, and reset 
--hard on the other hand is that the latter is completely unsafe. I mean, 
utterly completely super-unsafe. And I say that because _this here 
developer_ who is not exactly a Git noob lost stuff that way.

rebase -i checks that all is well and we could come back to the current 
status later if we realized that things went horribly wrong.

reset --hard does not do that. No safety net. No reflog. Nada.

Hth,
Dscho

[PATCH] Documentation: suggest "reset --keep" to undo a commit

[Jonathan Nieder]

When one's only goal is to move from one commit to another, reset
--keep is simply better than reset --hard, since it preserves local
changes in the index and worktree when easy and errors out without
doing anything when not.  Update the two "how to remove commits"
examples in this vein.  "reset --hard" is still explained in a later
example about cleaning up during a merge.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Johannes Schindelin wrote:

> rebase -i checks that all is well and we could come back to the current 
> status later if we realized that things went horribly wrong.
>
> reset --hard does not do that. No safety net. No reflog. Nada.

Right.  I think we should encourage people to use "reset --keep" more
often.  (In general.  The particular "rebase to pull" example just
mentioned is less obvious.)

 Documentation/git-reset.txt |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index fd72976..1f13a1e 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -148,7 +148,7 @@ Undo a commit, making it a topic branch::
 +
 ------------
 $ git branch topic/wip     <1>
-$ git reset --hard HEAD~3  <2>
+$ git reset --keep HEAD~3  <2>
 $ git checkout topic/wip   <3>
 ------------
 +
@@ -163,7 +163,7 @@ Undo commits permanently::
 +
 ------------
 $ git commit ...
-$ git reset --hard HEAD~3   <1>
+$ git reset --keep HEAD~3   <1>
 ------------
 +
 <1> The last three commits (HEAD, HEAD^, and HEAD~2) were bad
-- 
1.7.4.rc2

Re: [PATCH] Documentation: suggest "reset --keep" to undo a commit

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> When one's only goal is to move from one commit to another, reset
> --keep is simply better than reset --hard, since it preserves local
> changes in the index and worktree when easy and errors out without
> doing anything when not.  Update the two "how to remove commits"
> examples in this vein.  "reset --hard" is still explained in a later
> example about cleaning up during a merge.

I agree with the first sentence but I do not think its conclusion would
lead to changes to "how to _remove_ commits".  The examples were written
in contexts (explanatory text <$n>) where hard makes sense, and the
context needs tweaking to make keep makes more sense than hard does.

For example, the first one's original sequence is this:

    $ git branch topic/wip
    $ git reset --hard HEAD~3
    $ git checkout topic/wip

The text explains the motivation behind these series of commands in <1>
but it has one untold assumption behind it; the user did the review to
reach the conclusion that the recent changes are premature after fully
committing (i.e. the working tree is clean).  That is why "hard" worked
just fine.

But the user could do the reviewing and thinking with some local changes
still in the working tree (they are incredients for the fourth commit yet
to be made) and decide to branch at that point.  The description in <1>
needs to be updated to hint that there can be uncommitted changes, e.g.

	You have worked for some time, made a few commits, and may have
	uncommitted changes.  After reviewing the current state, you
	realized that ...

Using --keep may help the user do so, but only if the local changes do not
conflict with the changes in the recent commits to be discarded, right?

    Side note: Regardless of any of the above, the section header needs to
    be updated---it is not "Undo *A* commit", we are excluding three from
    the current branch.

By the way, a more natural way to do this would actually be:

    $ git checkout -b topic/wip
    $ git branch -f @{-1} HEAD~3

or using the stash:

    $ git stash ;# save local changes
    $ git branch topic/wip ;# and mark the tip before rewinding
    $ git reset --hard HEAD~3 ;# you could say --keep here too
    $ git checkout topic/wip ;# and then continue
    $ git stash pop ;# with the local changes

The branch/reset/checkout sequence itself, either with hard or keep, looks
more like a contrived example to show "reset" than the best way to solve
the problem in the scenario presented there.  Probably we would want to
drop this one altogether, or keep the scenario and explain the best
solution in somewhere else (e.g. tutorial).

> @@ -163,7 +163,7 @@ Undo commits permanently::
>  +
>  ------------
>  $ git commit ...
> -$ git reset --hard HEAD~3   <1>
> +$ git reset --keep HEAD~3   <1>
>  ------------
>  +
>  <1> The last three commits (HEAD, HEAD^, and HEAD~2) were bad

Please tell a story where keep makes more sense than hard by enhancing the
explanatory text <1> associated with this section.  The current text says
that the three topmost commit representing what you have recently worked
so far are all unwanted, strongly hinting that hard is more appropriate
thing to do than keep, which is not what we want if we are changing the
example to use keep.

It would be sufficient to just hint that the uncommitted changes that you
have in your working tree are unrelated to what these three commits wanted
to do (e.g. you always keep small changes around, such as debugging
printf's and a change to the version string in Makefile---you do not
intend to commit them and they are unrelated to the commits you are
discarding), and you do want to keep them around if you can.

Re: [PATCH] Documentation: suggest "reset --keep" to undo a commit

[Jonathan Nieder]

Junio C Hamano wrote:

> But the user could do the reviewing and thinking with some local changes
> still in the working tree (they are incredients for the fourth commit yet
> to be made) and decide to branch at that point.  The description in <1>
> needs to be updated to hint that there can be uncommitted changes, e.g.
>
> 	You have worked for some time, made a few commits, and may have
> 	uncommitted changes.  After reviewing the current state, you
> 	realized that ...
>
> Using --keep may help the user do so, but only if the local changes do not
> conflict with the changes in the recent commits to be discarded, right?

I think this explanation misses out on something.

I may be abusing git in a certain way, but I find myself in the
following situation fairly often:

	... hack hack hack ...
	git add -p;	# hmm, looks like multiple features.
	git stash -k
	... test ...
	git commit;	# commit feature #1
	git stash pop

	git add -p
	git stash -k
	... test ...
	git commit; # commit feature #2
	git stash pop

	# hmm, feature #2 is not suitable for this branch.
	git branch wip/feature-2
	git reset --keep HEAD^;	# <*>
	git add -p
	git stash -k
	... test ...
	git commit; # commit feature #3

On line <*>, I am just not thinking about the uncommitted changes.
They may be there or they may not.  If they are in the way of what I
am trying to do, "git reset --keep" will politely inform me so I can
act accordingly (usually stash, commit, or discard them).

> By the way, a more natural way to do this would actually be:
>
>     $ git checkout -b topic/wip
>     $ git branch -f @{-1} HEAD~3

True.  (I think the intended scenario was

	git branch topic/wip; # save the tip for later
	git reset --keep HEAD~3
	# now what was I working on?
	... hack hack hack ...

	# okay, now we have time for that diversion.
	git checkout topic/wip

but it would be nice to contrast it with the one you described.)

> or using the stash:
>
>     $ git stash ;# save local changes
>     $ git branch topic/wip ;# and mark the tip before rewinding
>     $ git reset --hard HEAD~3 ;# you could say --keep here too
>     $ git checkout topic/wip ;# and then continue
>     $ git stash pop ;# with the local changes

This approach leaves more files touched and more targets to be rebuilt
by "make".

> Please tell a story where keep makes more sense than hard by enhancing the
> explanatory text <1> associated with this section.  The current text says
> that the three topmost commit representing what you have recently worked
> so far are all unwanted, strongly hinting that hard is more appropriate
> thing to do than keep, which is not what we want if we are changing the
> example to use keep.

Maybe the best story would be "you have just explored a blind alley
and decided the last three commits are not a good idea at all", with
reference to a new section explaining that

 * --soft is for when the commit in preparation has the right content
   but should be on top of a different parent (e.g., squashing commits)

 * --keep is for transporting your local changes to a different commit
   (e.g., rewinding a branch or transplanting changes)

 - --merge is a limited and low-level tool for recovering from a
   conflicted merge and most often will take ORIG_HEAD as its argument.

   Maybe in the future merges will save more information so reset --merge
   can error out more often.

 - --hard is for resetting to a known state

 - --mixed is for resetting to a known state but leaving the worktree
   alone

> It would be sufficient to just hint that the uncommitted changes that you
> have in your working tree are unrelated to what these three commits wanted
> to do (e.g. you always keep small changes around, such as debugging
> printf's

That use case is less interesting to me --- it is relatively harmless
to clobber such content.

Re: [PATCH] Documentation: suggest "reset --keep" to undo a commit

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> Please tell a story where keep makes more sense than hard by enhancing the
>> explanatory text <1> associated with this section.  The current text says
>> that the three topmost commit representing what you have recently worked
>> so far are all unwanted, strongly hinting that hard is more appropriate
>> thing to do than keep, which is not what we want if we are changing the
>> example to use keep.
>
> Maybe the best story would be "you have just explored a blind alley
> and decided the last three commits are not a good idea at all", with

That unfortunately does not seem to describe the nature of the local
changes at all, which I think is the whole point of this topic to
encourage use of --keep over --hard.

>> It would be sufficient to just hint that the uncommitted changes that you
>> have in your working tree are unrelated to what these three commits wanted
>> to do (e.g. you always keep small changes around, such as debugging
>> printf's
>
> That use case is less interesting to me --- it is relatively harmless
> to clobber such content.

Actually I think that is the primary use case of the feature, as --keep
was done as a parallel to the behaviour of checkout that checks out a
different branch while keeping local changes.

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

[Junio C Hamano]

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

>> Wouldn't that suggest us that if we were to do anything to this message 
>> it would be a good idea to teach the user to "reset --hard" the branch 
>> if no commits truly needs to be replayed on top of the onto-commit?
>
> The important difference between rebase -i && noop on the one, and reset 
> --hard on the other hand is that the latter is completely unsafe. I mean, 
> utterly completely super-unsafe. And I say that because _this here 
> developer_ who is not exactly a Git noob lost stuff that way.

I think "rebase" already checks that the index and the working tree is
clean before starting, so referring to "reset --hard" when "rebase -i"
notices there is absolutely nothing to do is _not_ unsafe, no?

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

[Matthieu Moy]

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> Wouldn't that suggest us that if we were to do anything to this message 
>>> it would be a good idea to teach the user to "reset --hard" the branch 
>>> if no commits truly needs to be replayed on top of the onto-commit?
>>
>> The important difference between rebase -i && noop on the one, and reset 
>> --hard on the other hand is that the latter is completely unsafe. I mean, 
>> utterly completely super-unsafe. And I say that because _this here 
>> developer_ who is not exactly a Git noob lost stuff that way.
>
> I think "rebase" already checks that the index and the working tree is
> clean before starting, so referring to "reset --hard" when "rebase -i"
> notices there is absolutely nothing to do is _not_ unsafe, no?

The point is not about letting rebase do a "reset --hard", but to tell
the user s/he should have ran "reset --hard" instead of rebase. The
danger is to teach the user's fingers to type "reset --hard" too
often, which is unsafe ;-).

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

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

[Joshua Jensen]

----- Original Message -----
From: Matthieu Moy
Date: 1/21/2011 10:05 AM
> Junio C Hamano<gitster@pobox.com>  writes:
>
>> Johannes Schindelin<Johannes.Schindelin@gmx.de>  writes:
>>
>>>> Wouldn't that suggest us that if we were to do anything to this message
>>>> it would be a good idea to teach the user to "reset --hard" the branch
>>>> if no commits truly needs to be replayed on top of the onto-commit?
>>> The important difference between rebase -i&&  noop on the one, and reset
>>> --hard on the other hand is that the latter is completely unsafe. I mean,
>>> utterly completely super-unsafe. And I say that because _this here
>>> developer_ who is not exactly a Git noob lost stuff that way.
>> I think "rebase" already checks that the index and the working tree is
>> clean before starting, so referring to "reset --hard" when "rebase -i"
>> notices there is absolutely nothing to do is _not_ unsafe, no?
> The point is not about letting rebase do a "reset --hard", but to tell
> the user s/he should have ran "reset --hard" instead of rebase. The
> danger is to teach the user's fingers to type "reset --hard" too
> often, which is unsafe ;-).
I've always wished "git reset --hard" would tell me there are modified 
files and force me to type "git reset --hard --force" to overwrite 
them.  It is a dangerous command, and I stupidly run it sometimes 
without running "git status" first.

-Josh

[PATCH] Documentation: do not treat reset --keep as a special case

[Jonathan Nieder]

The current treatment of "git reset --keep" emphasizes how it
differs from --hard (treatment of local changes) and how it breaks
down into plumbing (git read-tree -m -u HEAD <commit> followed by git
update-ref HEAD <commit>).  This can discourage people from using
it, since it might seem to be a complex or niche option.

Better to emphasize what the --keep flag is intended for --- moving
the index and worktree from one commit to another, like "git checkout"
would --- so the reader can make a more informed decision about the
appropriate situations in which to use it.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Joshua Jensen wrote:

> I've always wished "git reset --hard" would tell me there are
> modified files and force me to type "git reset --hard --force" to
> overwrite them.  It is a dangerous command, and I stupidly run it
> sometimes without running "git status" first.

Have you tried "git reset --keep"?  How does it compare to your
wish?

 Documentation/git-reset.txt |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index fd72976..927ecee 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -76,15 +76,10 @@ In other words, --merge does something like a 'git read-tree -u -m <commit>',
 but carries forward unmerged index entries.
 
 --keep::
-	Resets the index, updates files in the working tree that are
-	different between <commit> and HEAD, but keeps those
-	which are different between HEAD and the working tree (i.e.
-	which have local changes).
+	Resets index entries and updates files in the working tree that are
+	different between <commit> and HEAD.
 	If a file that is different between <commit> and HEAD has local changes,
 	reset is aborted.
-+
-In other words, --keep does a 2-way merge between <commit> and HEAD followed by
-'git reset --mixed <commit>'.
 --
 
 If you want to undo a commit other than the latest on a branch,
-- 
1.7.4.rc2

Re: [PATCH] Documentation: do not treat reset --keep as a special case

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The current treatment of "git reset --keep" emphasizes how it
> differs from --hard (treatment of local changes) and how it breaks
> down into plumbing (git read-tree -m -u HEAD <commit> followed by git
> update-ref HEAD <commit>).  This can discourage people from using
> it, since it might seem to be a complex or niche option.
>
> Better to emphasize what the --keep flag is intended for --- moving
> the index and worktree from one commit to another, like "git checkout"
> would --- so the reader can make a more informed decision about the
> appropriate situations in which to use it.

The updated text makes quite a lot of sense ;-) while the old text
doesn't.  What were we smoking when we wrote it and passed it through the
review?

> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>  Documentation/git-reset.txt |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index fd72976..927ecee 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -76,15 +76,10 @@ In other words, --merge does something like a 'git read-tree -u -m <commit>',
>  but carries forward unmerged index entries.
>  
>  --keep::
> -	Resets the index, updates files in the working tree that are
> -	different between <commit> and HEAD, but keeps those
> -	which are different between HEAD and the working tree (i.e.
> -	which have local changes).
> +	Resets index entries and updates files in the working tree that are
> +	different between <commit> and HEAD.
>  	If a file that is different between <commit> and HEAD has local changes,
>  	reset is aborted.

I saw "updates files" and one question immediately came to mind: update
how?  "... to match what is in HEAD"?  "Resets index entries" is less of a
problem as the word "reset" already strongly suggests that the current
state does not matter as much as the target state, though.

Thanks.

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

[Jay Soffian]

On Fri, Jan 21, 2011 at 12:57 PM, Joshua Jensen
<jjensen@workspacewhiz.com> wrote:
> I've always wished "git reset --hard" would tell me there are modified files
> and force me to type "git reset --hard --force" to overwrite them.  It is a
> dangerous command, and I stupidly run it sometimes without running "git
> status" first.

Something analogous to clean.requireForce?

j.

Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits

[Johannes Schindelin]

Hi,

On Fri, 21 Jan 2011, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Wouldn't that suggest us that if we were to do anything to this 
> >> message it would be a good idea to teach the user to "reset --hard" 
> >> the branch if no commits truly needs to be replayed on top of the 
> >> onto-commit?
> >
> > The important difference between rebase -i && noop on the one, and 
> > reset --hard on the other hand is that the latter is completely 
> > unsafe. I mean, utterly completely super-unsafe. And I say that 
> > because _this here developer_ who is not exactly a Git noob lost stuff 
> > that way.
> 
> I think "rebase" already checks that the index and the working tree is 
> clean before starting, so referring to "reset --hard" when "rebase -i" 
> notices there is absolutely nothing to do is _not_ unsafe, no?

Oh, so you want to suggest using "reset --hard" but warn at the same time 
that this command on its own is dangerous unless you run rebase first? :-)

Ciao,
Johannes

[PATCH 2/2] test-lib: user-friendly alternatives to test [-d|-f|-e]

[Matthieu Moy]

The helper functions are implemented, documented, and used in a few
places to validate them, but not everywhere to avoid useless code churn.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Just resending this one since I modified PATCH 1/2 and had to resolve
a minor conflict.

 t/README                      |    7 +++++++
 t/t3404-rebase-interactive.sh |   18 +++++++++---------
 t/t3407-rebase-abort.sh       |    6 +++---
 t/test-lib.sh                 |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/t/README b/t/README
index 0d1183c..410499a 100644
--- a/t/README
+++ b/t/README
@@ -467,6 +467,13 @@ library for your script to use.
    <expected> file.  This behaves like "cmp" but produces more
    helpful output when the test is run with "-v" option.
 
+ - test_path_is_file <file> [<diagnosis>]
+   test_path_is_dir <dir> [<diagnosis>]
+   test_path_is_missing <path> [<diagnosis>]
+
+   Check whether a file/directory exists or doesn't. <diagnosis> will
+   be displayed if the test fails.
+
  - test_when_finished <script>
 
    Prepend <script> to a list of commands to run to clean up
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 93b181e..fa02eb3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -79,18 +79,18 @@ test_expect_success 'rebase -i with the exec command' '
 	export FAKE_LINES &&
 	test_must_fail git rebase -i A
 	) &&
-	test -f touch-one &&
-	test -f touch-two &&
-	! test -f touch-three &&
+	test_path_is_file touch-one &&
+	test_path_is_file touch-two &&
+	test_path_is_missing touch-three " (should have stopped before)" &&
 	test $(git rev-parse C) = $(git rev-parse HEAD) || {
 		echo "Stopped at wrong revision:"
 		echo "($(git describe --tags HEAD) instead of C)"
 		false
 	} &&
 	git rebase --continue &&
-	test -f touch-three &&
-	test -f "touch-file  name with spaces" &&
-	test -f touch-after-semicolon &&
+	test_path_is_file touch-three &&
+	test_path_is_file "touch-file  name with spaces" &&
+	test_path_is_file touch-after-semicolon &&
 	test $(git rev-parse master) = $(git rev-parse HEAD) || {
 		echo "Stopped at wrong revision:"
 		echo "($(git describe --tags HEAD) instead of master)"
@@ -105,7 +105,7 @@ test_expect_success 'rebase -i with the exec command runs from tree root' '
 	FAKE_LINES="1 exec_>touch-subdir" \
 		git rebase -i HEAD^ &&
 	cd .. &&
-	test -f touch-subdir &&
+	test_path_is_file touch-subdir &&
 	rm -fr subdir
 '
 
@@ -204,7 +204,7 @@ test_expect_success 'abort' '
 	git rebase --abort &&
 	test $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
-	! test -d .git/rebase-merge
+	test_path_is_missing .git/rebase-merge
 '
 
 test_expect_success 'abort with error when new base cannot be checked out' '
@@ -213,7 +213,7 @@ test_expect_success 'abort with error when new base cannot be checked out' '
 	test_must_fail git rebase -i master > output 2>&1 &&
 	grep "Untracked working tree file .file1. would be overwritten" \
 		output &&
-	! test -d .git/rebase-merge &&
+	test_path_is_missing .git/rebase-merge &&
 	git reset --hard HEAD^
 '
 
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 2999e78..fbb3f2e 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -38,7 +38,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_path_is_dir "$dotest" &&
 		git rebase --abort &&
 		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
 		test ! -d "$dotest"
@@ -49,7 +49,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_path_is_dir "$dotest" &&
 		test_must_fail git rebase --skip &&
 		test $(git rev-parse HEAD) = $(git rev-parse master) &&
 		git rebase --abort &&
@@ -62,7 +62,7 @@ testrebase() {
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type master &&
-		test -d "$dotest" &&
+		test_path_is_dir "$dotest" &&
 		echo c > a &&
 		echo d >> a &&
 		git add a &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e8f21d5..d584194 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -541,6 +541,38 @@ test_external_without_stderr () {
 	fi
 }
 
+# debugging-friendly alternatives to "test [-f|-d|-e]"
+# The commands test the existence or non-existence of $1. $2 can be
+# given to provide a more precise diagnosis.
+test_path_is_file () {
+	if ! [ -f "$1" ]
+	then
+		echo "File $1 doesn't exist. $*"
+		false
+	fi
+}
+
+test_path_is_dir () {
+	if ! [ -d "$1" ]
+	then
+		echo "Directory $1 doesn't exist. $*"
+		false
+	fi
+}
+
+test_path_is_missing () {
+	if [ -e "$1" ]
+	then
+		echo "Path exists:"
+		ls -ld "$1"
+		if [ $# -ge 1 ]; then
+			echo "$*"
+		fi
+		false
+	fi
+}
+
+
 # This is not among top-level (test_expect_success | test_expect_failure)
 # but is a prefix that can be used in the test script, like:
 #
-- 
1.7.2.1.52.g95e25.dirty