Re: [PATCH] git-explain

"J. Bruce Fields" [off-list ref] writes:

On Mon, Dec 04, 2006 at 10:55:49PM -0500, Nicolas Pitre wrote:

quoted

...

quoted

[PATCH] git-explain
...

What about calling it git-whatsup instead?

No, clearly it should be git-wtf.

Should I take these responses to mean that you two are negative
about the approach of spending extra cycles to commands that can
leave the working tree in a "in the middle of doing something"
state to help having a unified command to explain what the
situation is and suggest the user possible exits, or are you
saying that it might be a good idea but "git explain" is a bad
name?

An issue with this approach is that this can be the beginning of
hardwiring the official "right way of doing things" in the set
of tools.  Pursuing this approach would enhance the set of state
markers like "FAILED_MERGE" in the example, which means:

 - more commands would actively record what they were attempting
   to do, obviously;

 - over time "git explain" will learn about these state markers,
   and we would hardwire the "best current practice" exits from
   various states in the help messages;

 - also commands other than "git explain" would learn about the
   state markers of other commands, and change their behaviour.
   For example, "git am" might learn to refuse running while a
   merge in progress much earlier than with the current
   implementation.

The last point can easily become a double-edged sword.

Hardwiring the recommended workflow in the tools would reduce
chances of mistakes, but it could rob the flexibility from them
if we are not careful and forget to take into account some
useful combination of tools when adding such safety valves.

This is the stuff I mentioned I had been working on several months
before in a reply to the git-explain/git-wtf/git-whatsup thread.

I've rebased it against the current master and everything still seems
to work (I don't have unit tests for them).

This has been forgotten and abandoned for a while.  I especially don't
expect the changes to git-commit.sh (status) to be applied as-is, as it
should go into the new runstatus (my work predates runstatus).

 git-am.sh       |   18 ++++++++++++++----
 git-commit.sh   |   45 ++++++++++++++++++++++++++++++++++++++++++++-
 git-rebase.sh   |   43 ++++++++++++++++++++++++++++++-------------
 git-rerere.perl |   25 +++++++++++++++++++++++++
 4 files changed, 113 insertions(+), 18 deletions(-)

[PATCH 1/5] rerere: avoid misrecording on a skipped or aborted rebase/am
[PATCH 2/5] status: show files that would have resolutions recorded by rerere
[PATCH 3/5] am and rebase resolve states get picked up by status/commit
[PATCH 4/5] am: run git rerere to record resolution on successful --resolved
[PATCH 5/5] rerere: add the diff command

-- 

Data in rr-cache isn't valid after a patch application is
skipped or and aborted, so our next commit could be misrecorded
as a resolution of that skipped/failed commit, which is wrong.

Signed-off-by: Eric Wong <redacted>
---
 git-am.sh       |    4 ++++
 git-rebase.sh   |    8 ++++++++
 git-rerere.perl |   12 ++++++++++++
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index afe322b..28ccae3 100755
--- a/git-am.sh
+++ b/git-am.sh

@@ -246,6 +246,10 @@ last=`cat "$dotest/last"`
 this=`cat "$dotest/next"`
 if test "$skip" = t
 then
+	if test -d "$GIT_DIR/rr-cache"
+	then
+		git-rerere clear
+	fi
 	this=`expr "$this" + 1`
 	resume=
 fi

diff --git a/git-rebase.sh b/git-rebase.sh
index 25530df..2b4f347 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -139,6 +139,10 @@ do
 	--skip)
 		if test -d "$dotest"
 		then
+			if test -d "$GIT_DIR/rr-cache"
+			then
+				git-rerere clear
+			fi
 			prev_head="`cat $dotest/prev_head`"
 			end="`cat $dotest/end`"
 			msgnum="`cat $dotest/msgnum`"

@@ -157,6 +161,10 @@ do
 		exit
 		;;
 	--abort)
+		if test -d "$GIT_DIR/rr-cache"
+		then
+			git-rerere clear
+		fi
 		if test -d "$dotest"
 		then
 			rm -r "$dotest"

diff --git a/git-rerere.perl b/git-rerere.perl
index d3664ff..dd86577 100755
--- a/git-rerere.perl
+++ b/git-rerere.perl

@@ -172,6 +172,18 @@ sub merge {
 -d "$rr_dir" || exit(0);
 
 read_rr();
+
+if (@ARGV && $ARGV[0] eq 'clear') {
+	for my $path (keys %merge_rr) {
+		my $name = $merge_rr{$path};
+		if (-d "$rr_dir/$name") {
+			rmtree(["$rr_dir/$name"]);
+		}
+	}
+	unlink $merge_rr;
+	exit 0;
+}
+
 my %conflict = map { $_ => 1 } find_conflict();
 
 # MERGE_RR records paths with conflicts immediately after merge

-- 
1.4.4.2.g860f4

An issue with this approach is that this can be the beginning 
of hardwiring the official "right way of doing things" in the 
set of tools.  Pursuing this approach would enhance the set 
of state markers like "FAILED_MERGE" in the example, which means:

Wouldn't it be better to create some kind of action-log (that's
cleared at the end of the command if everything was all right)
instead of creating special markers for different commands?

That way there would be only 1 place to check for what happened ...

-- 
best regards

  Ray

Signed-off-by: Eric Wong <redacted>
---
 git-commit.sh   |   17 ++++++++++++++++-
 git-rerere.perl |   18 ++++++++++++------
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/git-commit.sh b/git-commit.sh
index 81c3a0c..9f6d1ef 100755
--- a/git-commit.sh
+++ b/git-commit.sh

@@ -48,7 +48,22 @@ run_status () {
 		GIT_INDEX_FILE="$NEXT_INDEX"
 		export GIT_INDEX_FILE
 	fi
-
+	if test -d "$GIT_DIR/rr-cache"
+	then
+	    rr_shown=
+	    git-rerere status | while read line; do
+		if [ -z "$rr_shown" ]; then
+		    echo '#'
+		    echo '# Resolutions to be recorded for files:'
+		    echo '#   (git-rerere will automatically record' \
+			 'conflict resolutions'
+		    echo '#    when these files are committed)'
+		    echo '#'
+		    rr_shown=1
+		fi
+		echo  "#	$line"
+	    done
+	fi
 	case "$status_only" in
 	t) color= ;;
 	*) color=--nocolor ;;

diff --git a/git-rerere.perl b/git-rerere.perl
index dd86577..b78194a 100755
--- a/git-rerere.perl
+++ b/git-rerere.perl

@@ -173,14 +173,20 @@ sub merge {
 
 read_rr();
 
-if (@ARGV && $ARGV[0] eq 'clear') {
-	for my $path (keys %merge_rr) {
-		my $name = $merge_rr{$path};
-		if (-d "$rr_dir/$name") {
-			rmtree(["$rr_dir/$name"]);
+if (my $arg = shift @ARGV) {
+	if ($arg eq 'clear') {
+		for my $path (keys %merge_rr) {
+			my $name = $merge_rr{$path};
+			if (-d "$rr_dir/$name") {
+				rmtree(["$rr_dir/$name"]);
+			}
+		}
+		unlink $merge_rr;
+	} elsif ($arg eq 'status') {
+		for my $path (keys %merge_rr) {
+			print $path, "\n";
 		}
 	}
-	unlink $merge_rr;
 	exit 0;
 }
 

-- 
1.4.4.2.g860f4

Eric Wong wrote:

Sometimes I like to see what I'm recording resolutions for and
what's changed during a resolution.

Signed-off-by: Eric Wong <redacted>
---
 git-rerere.perl |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

Documentation, please?
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Signed-off-by: Eric Wong <redacted>
---
 git-am.sh |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 179b967..d0714c6 100755
--- a/git-am.sh
+++ b/git-am.sh

@@ -414,6 +414,10 @@ do
 			stop_here_user_resolve $this
 		fi
 		apply_status=0
+		if test -d "$GIT_DIR/rr-cache"
+		then
+			git rerere
+		fi
 		;;
 	esac
 

-- 
1.4.4.2.g860f4

On Mon, Dec 04, 2006 at 10:09:17PM -0800, Junio C Hamano wrote:

Should I take these responses to mean that you two are negative
about the approach of spending extra cycles to commands that can
leave the working tree in a "in the middle of doing something"
state to help having a unified command to explain what the
situation is and suggest the user possible exits, or are you
saying that it might be a good idea but "git explain" is a bad
name?

It seems like the point of this command is to show some state
information which would otherwise be hard to see. I think of 'git
status' as the way to look at the repository state. Perhaps we should
enhance the output of 'git status' to note things such as failed merges,
whether we're bisecting, in the middle of applying a patch series, etc.
There could be an optional verbosity switch to give "full explanations"
including recommended ways to deal with the situation.

Hardwiring the recommended workflow in the tools would reduce
chances of mistakes, but it could rob the flexibility from them
if we are not careful and forget to take into account some
useful combination of tools when adding such safety valves.

As long as the safety valves don't come up _routinely_ in certain
workflows, it seems OK to bypass them with a '-f' force switch. I
suspect the best way to figure out if such workflows are in use is to
put in the safety valves and see who complains; otherwise we're stuck
with brainstorming workflows and deciding whether they make sense.

This should help warn of accidental commits in the middle of a
rebase operation.  It also saves messages in $dotest/resolvemsg
and shows it in "git status" so the user can be reminded of
how to continue the am or rebase operation.

Signed-off-by: Eric Wong <redacted>
---
 git-am.sh     |   10 ++++++----
 git-commit.sh |   28 ++++++++++++++++++++++++++++
 git-rebase.sh |   35 ++++++++++++++++++++++-------------
 3 files changed, 56 insertions(+), 17 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 28ccae3..179b967 100755
--- a/git-am.sh
+++ b/git-am.sh

@@ -16,7 +16,7 @@ stop_here () {
 
 stop_here_user_resolve () {
     if [ -n "$resolvemsg" ]; then
-	    echo "$resolvemsg"
+	    echo "$resolvemsg" | tee "$dotest/resolvemsg"
 	    stop_here $1
     fi
     cmdline=$(basename $0)

@@ -32,9 +32,11 @@ stop_here_user_resolve () {
     then
         cmdline="$cmdline -d=$dotest"
     fi
-    echo "When you have resolved this problem run \"$cmdline --resolved\"."
-    echo "If you would prefer to skip this patch, instead run \"$cmdline --skip\"."
-
+    cat > "$dotest/resolvemsg" <<EOF
+When you have resolved this problem run \"$cmdline --resolved\".
+If you would prefer to skip this patch, instead run \"$cmdline --skip\".
+EOF
+    cat "$dotest/resolvemsg"
     stop_here $1
 }
 

diff --git a/git-commit.sh b/git-commit.sh
index 9f6d1ef..4691835 100755
--- a/git-commit.sh
+++ b/git-commit.sh

@@ -32,6 +32,33 @@ save_index () {
 	cp -p "$THIS_INDEX" "$NEXT_INDEX"
 }
 
+check_dotest () {
+	if test -d .dotest
+	then
+		echo ''
+		if test -f .dotest/resolvemsg
+		then
+			cat .dotest/resolvemsg
+		else
+			echo 'A .dotest directory exists.'
+			echo 'Either a "git rebase" or "git am"' \
+					'operation is in progress'
+		fi
+	fi
+	if test -d "$GIT_DIR/.dotest-merge"
+	then
+		echo ''
+		if test -f "$GIT_DIR/.dotest-merge/resolvemsg"
+		then
+			cat "$GIT_DIR/.dotest-merge/resolvemsg"
+		else
+			echo "A $GIT_DIR/.dotest-merge/resolvemsg " \
+				'directory exists.'
+			echo 'A "git rebase --merge" operation is in progress'
+		fi
+	fi
+}
+
 run_status () {
 	# If TMP_INDEX is defined, that means we are doing
 	# "--only" partial commit, and that index file is used

@@ -64,6 +91,7 @@ run_status () {
 		echo  "#	$line"
 	    done
 	fi
+	check_dotest | sed -e 's/^/# /'
 	case "$status_only" in
 	t) color= ;;
 	*) color=--nocolor ;;

diff --git a/git-rebase.sh b/git-rebase.sh
index 2b4f347..53f3919 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -41,6 +41,16 @@ dotest=$GIT_DIR/.dotest-merge
 prec=4
 verbose=
 
+die_msg () {
+	> "$dotest/resolvemsg"
+	for i in "$@"
+	do
+		echo "$i" | tee -a "$dotest/resolvemsg" >&2
+	done
+	echo "$RESOLVEMSG" >> "$dotest/resolvemsg"
+	die "$RESOLVEMSG"
+}
+
 continue_merge () {
 	test -n "$prev_head" || die "prev_head must be defined"
 	test -d "$dotest" || die "$dotest directory does not exist"

@@ -48,18 +58,17 @@ continue_merge () {
 	unmerged=$(git-ls-files -u)
 	if test -n "$unmerged"
 	then
-		echo "You still have unmerged paths in your index"
-		echo "did you forget update-index?"
-		die "$RESOLVEMSG"
+		die_msg "You still have unmerged paths in your index" \
+				"did you forget update-index?"
 	fi
 
 	if test -n "`git-diff-index HEAD`"
 	then
 		if ! git-commit -C "`cat $dotest/current`"
 		then
-			echo "Commit failed, please do not call \"git commit\""
-			echo "directly, but instead do one of the following: "
-			die "$RESOLVEMSG"
+			die_msg \
+			"Commit failed, please do not call \"git commit\"" \
+			"directly, but instead do one of the following: "
 		fi
 		printf "Committed: %0${prec}d" $msgnum
 	else

@@ -73,6 +82,7 @@ continue_merge () {
 	echo "$prev_head" > "$dotest/prev_head"
 
 	# onto the next patch:
+	rm -f "$dotest/resolvemsg"
 	msgnum=$(($msgnum + 1))
 	echo "$msgnum" >"$dotest/msgnum"
 }

@@ -88,14 +98,13 @@ call_merge () {
 		;;
 	1)
 		test -d "$GIT_DIR/rr-cache" && git-rerere
-		die "$RESOLVEMSG"
+		die_msg
 		;;
 	2)
-		echo "Strategy: $rv $strategy failed, try another" 1>&2
-		die "$RESOLVEMSG"
+		die_msg "Strategy: $rv $strategy failed, try another"
 		;;
 	*)
-		die "Unknown exit code ($rv) from command:" \
+		die_msg "Unknown exit code ($rv) from command:" \
 			"git-merge-$strategy $cmt^ -- HEAD $cmt"
 		;;
 	esac

@@ -112,9 +121,8 @@ do
 	--continue)
 		diff=$(git-diff-files)
 		case "$diff" in
-		?*)	echo "You must edit all merge conflicts and then"
-			echo "mark them as resolved using git update-index"
-			exit 1
+		?*)	die_msg "You must edit all merge conflicts and then" \
+				"mark them as resolved using git update-index"
 			;;
 		esac
 		if test -d "$dotest"

@@ -143,6 +151,7 @@ do
 			then
 				git-rerere clear
 			fi
+			rm -f "$dotest/resolvemsg"
 			prev_head="`cat $dotest/prev_head`"
 			end="`cat $dotest/end`"
 			msgnum="`cat $dotest/msgnum`"

-- 
1.4.4.2.g860f4

Sometimes I like to see what I'm recording resolutions for and
what's changed during a resolution.

Signed-off-by: Eric Wong <redacted>
---
 git-rerere.perl |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/git-rerere.perl b/git-rerere.perl
index b78194a..7a3ae84 100755
--- a/git-rerere.perl
+++ b/git-rerere.perl

@@ -186,6 +186,13 @@ if (my $arg = shift @ARGV) {
 		for my $path (keys %merge_rr) {
 			print $path, "\n";
 		}
+	} elsif ($arg eq 'diff') {
+		for my $path (keys %merge_rr) {
+			my $name = $merge_rr{$path};
+			system(qw/diff/, @ARGV,
+				'-L', "a/$path", '-L', "b/$path",
+				"$rr_dir/$name/preimage", $path);
+		}
 	}
 	exit 0;
 }

-- 
1.4.4.2.g860f4

Jeff King [off-list ref] wrote:

On Mon, Dec 04, 2006 at 10:09:17PM -0800, Junio C Hamano wrote:

quoted

Should I take these responses to mean that you two are negative
about the approach of spending extra cycles to commands that can
leave the working tree in a "in the middle of doing something"
state to help having a unified command to explain what the
situation is and suggest the user possible exits, or are you
saying that it might be a good idea but "git explain" is a bad
name?

It seems like the point of this command is to show some state
information which would otherwise be hard to see. I think of 'git
status' as the way to look at the repository state. Perhaps we should
enhance the output of 'git status' to note things such as failed merges,
whether we're bisecting, in the middle of applying a patch series, etc.
There could be an optional verbosity switch to give "full explanations"
including recommended ways to deal with the situation.

I wholeheartedly agree that 'git status' should show something like
this.  I actually had some stuff that was a work-in-progress several
months ago that enhanced status with several things like this; but got
distracted and forgot about that repository.  I'll try to dig it out
sometime tomorrow.  I remember my work started from wanting to know
what 'git-rerere' would be recording.

-- 

Hi,

On Mon, 4 Dec 2006, Junio C Hamano wrote:

"J. Bruce Fields" [off-list ref] writes:

quoted

On Mon, Dec 04, 2006 at 10:55:49PM -0500, Nicolas Pitre wrote:

quoted

...

quoted

[PATCH] git-explain
...

What about calling it git-whatsup instead?

No, clearly it should be git-wtf.

Should I take these responses to mean that you two are negative
about the approach [...]

I think they just were in the mood for some slashdot style 
unimportant-aspects-in-a-funny-way discussion.

An issue with this approach is that this can be the beginning of
hardwiring the official "right way of doing things" in the set
of tools.  Pursuing this approach would enhance the set of state
markers like "FAILED_MERGE" in the example, which means:

 - more commands would actively record what they were attempting
   to do, obviously;

... which is a good thing.

 - over time "git explain" will learn about these state markers,
   and we would hardwire the "best current practice" exits from
   various states in the help messages;

... which is also a good thing.

 - also commands other than "git explain" would learn about the
   state markers of other commands, and change their behaviour.
   For example, "git am" might learn to refuse running while a
   merge in progress much earlier than with the current
   implementation.

If the other commands are outside of git, it will be a problem.

The last point [git-am refusing to run during a merge] can easily become 
a double-edged sword.

This particular behaviour seems like a good thing, too!

Hardwiring the recommended workflow in the tools would reduce chances of 
mistakes, but it could rob the flexibility from them if we are not 
careful and forget to take into account some useful combination of tools 
when adding such safety valves.

As has been the case not at all long ago, a saftey valve which no longer 
made sense was just removed.

As for the inflexibility of a recommended workflow: by now, long-time 
gitsters have had enough time to fiddle around with git and to develop a 
workflow which Just Works. It is just a nice gesture of old-time users 
towards new-time users to pass that knowledge. And new-time users are 
often not in the least interested in learning the ropes the hard way.

Besides, the recommended workflow(s) can be changed/replaced by other 
porcelainish commands, because only those will contain the safety valves, 
right?

Ciao,
Dscho

On Tue, Dec 05, 2006 at 09:58:25AM +0100, Johannes Schindelin wrote:

On Mon, 4 Dec 2006, Junio C Hamano wrote:

quoted

"J. Bruce Fields" [off-list ref] writes:

quoted

On Mon, Dec 04, 2006 at 10:55:49PM -0500, Nicolas Pitre wrote:

quoted

...

quoted

[PATCH] git-explain
...

What about calling it git-whatsup instead?

No, clearly it should be git-wtf.

Should I take these responses to mean that you two are negative
about the approach [...]

I think they just were in the mood for some slashdot style 
unimportant-aspects-in-a-funny-way discussion.

Yeah, I was just being silly, apologies.