threads / patch / 11025

patchReplace misleading message during interactive rebasing

Subject: [PATCH] Replace misleading message during interactive rebasing

## tl;dr

11 messages between Nov 26, 2007 and Nov 28, 2007. Diffs are folded; open one to read it.

replies: 10people: 6as markdown or json

Wincent Colaiuta· Nov 26, 2007, 21:18 UTC · lore

git-rebase--interactive uses git-cherry-pick under the covers to reorder commits, which in turn means that in the event of a conflict a message will be shown advising the user to commit the results and use the -c switch to retain authorship after fixing the conflict.

The message is misleading because what the user really needs to do is run "git rebase --continue"; the committing is handled by git-rebase and the authorship of the commit message is retained automatically.

We solve this problem by using an environment variable to communicate to git-cherry-pick that rebasing is underway and replace the misleading error message with a more helpful one.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
This applies on top of "master".

The question of whether the environment variable should have a leading underscore came up on the mailing list. I don't really care at all either way, I'd just like to see the misleading message go away. I'll leave it up to others to decide.

Another thing to decide is whether the help text should be more than just "run 'git rebase --continue'", but should mention "git rebase --abort" as well. Junio, please feel free to modify the patch if you think it would be appropriate.

 builtin-revert.c           |    8 +++++---
 git-rebase--interactive.sh |    2 ++
 2 files changed, 7 insertions(+), 3 deletions(-)
Show changes to 2 files +7 −3

builtin-revert.c, git-rebase--interactive.sh

diff --git a/builtin-revert.c b/builtin-revert.c
index a0586f9..5a57574 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -229,7 +229,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
 	int i;
-	char *oneline, *reencoded_message = NULL;
+	char *oneline, *reencoded_message = NULL, *help_message;
 	const char *message, *encoding;
 	const char *defmsg = xstrdup(git_path("MERGE_MSG"));
 
@@ -352,11 +352,13 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		}
 		if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
 			die ("Error wrapping up %s", defmsg);
+		help_message = getenv("_GIT_CHERRY_PICK_HELP");
 		fprintf(stderr, "Automatic %s failed.  "
 			"After resolving the conflicts,\n"
 			"mark the corrected paths with 'git add <paths>' "
-			"and commit the result.\n", me);
-		if (action == CHERRY_PICK) {
+			"and %s.\n", me,
+			help_message ? help_message : "commit the result");
+		if (action == CHERRY_PICK && !help_message) {
 			fprintf(stderr, "When commiting, use the option "
 				"'-c %s' to retain authorship and message.\n",
 				find_unique_abbrev(commit->object.sha1,
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index bf44b6a..e5f9810 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -117,6 +117,7 @@ pick_one () {
 		sha1=$(git rev-parse --short $sha1)
 		output warn Fast forward to $sha1
 	else
+		export _GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
 		output git cherry-pick "$@"
 	fi
 }
@@ -187,6 +188,7 @@ pick_one_preserving_merges () {
 			fi
 			;;
 		*)
+			export _GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
 			output git cherry-pick "$@" ||
 				die_with_patch $sha1 "Could not pick $sha1"
 			;;
-- 
1.5.3.6.952.g84ef
Johannes Sixt· Nov 27, 2007, 08:31 UTC · re: Wincent Colaiuta · lore

Re: [PATCH] Replace misleading message during interactive rebasing

Wincent Colaiuta schrieb:
> +		export _GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
Isn't this a bashism? Should be:
		_GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
		export _GIT_CHERRY_PICK_HELP
(and the second instance of this as well, of course)
-- Hannes
Wincent Colaiuta· Nov 27, 2007, 08:49 UTC · re: Johannes Sixt · lore

Re: [PATCH] Replace misleading message during interactive rebasing

El 27/11/2007, a las 9:31, Johannes Sixt escribió:
Show 9 quoted lines
> Wincent Colaiuta schrieb:
>> +		export _GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
>
> Isn't this a bashism? Should be:
>
> 		_GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
> 		export _GIT_CHERRY_PICK_HELP
>
> (and the second instance of this as well, of course)

I wondered that myself before submitting the patch, but then saw that the same pattern was used at other places as well:

git-clone.sh:
W=$(cd "$GIT_WORK_TREE" && pwd) && export GIT_WORK_TREE="$W"
git-filter-branch.sh:

export GIT_INDEX_FILE="$(pwd)/../index" export GIT_COMMIT=$commit export GIT_COMMIT="$sha1"

git-quiltimport.sh:

export GIT_AUTHOR_NAME=$(sed -ne 's/Author: //p' "$tmp_info") export GIT_AUTHOR_EMAIL=$(sed -ne 's/Email: //p' "$tmp_info") export GIT_AUTHOR_DATE=$(sed -ne 's/Date: //p' "$tmp_info") export SUBJECT=$(sed -ne 's/Subject: //p' "$tmp_info")

So if this is a problem, those sites will need to be changed as well.

Cheers, Wincent

Jeff King· Nov 27, 2007, 09:46 UTC · re: Johannes Sixt · lore

Re: [PATCH] Replace misleading message during interactive rebasing

On Tue, Nov 27, 2007 at 09:31:06AM +0100, Johannes Sixt wrote:
Show 6 quoted lines
> Isn't this a bashism? Should be:
>
> 		_GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
> 		export _GIT_CHERRY_PICK_HELP
>
> (and the second instance of this as well, of course)
No, it's in POSIX:
http://www.opengroup.org/onlinepubs/009695399/utilities/export.html

However, I'm sure you will be shocked to learn that /bin/sh on Solaris doesn't understand it:

$ export foo=bar foo=bar: is not an identifier

-Peff
Jakub Narebski· Nov 27, 2007, 09:49 UTC · re: Jeff King · lore

Re: [PATCH] Replace misleading message during interactive rebasing

Jeff King wrote:
Show 18 quoted lines
> On Tue, Nov 27, 2007 at 09:31:06AM +0100, Johannes Sixt wrote:
> 
>> Isn't this a bashism? Should be:
>>
>>              _GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
>>              export _GIT_CHERRY_PICK_HELP
>>
>> (and the second instance of this as well, of course)
> 
> No, it's in POSIX:
> 
> http://www.opengroup.org/onlinepubs/009695399/utilities/export.html
> 
> However, I'm sure you will be shocked to learn that /bin/sh on Solaris
> doesn't understand it:
> 
> $ export foo=bar
> foo=bar: is not an identifier

If I remember correctly /bin/sh on Solaris cannot be used because of other issues (like $(...) and such).

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Jeff King· Nov 27, 2007, 09:52 UTC · re: Jakub Narebski · lore

Re: [PATCH] Replace misleading message during interactive rebasing

On Tue, Nov 27, 2007 at 10:49:03AM +0100, Jakub Narebski wrote:
Show 8 quoted lines
> > However, I'm sure you will be shocked to learn that /bin/sh on Solaris
> > doesn't understand it:
> > 
> > $ export foo=bar
> > foo=bar: is not an identifier
> 
> If I remember correctly /bin/sh on Solaris cannot be used because
> of other issues (like $(...) and such).

Yes, my response was not "we can't use this" (because clearly we have been for some time) but rather "look how crappy the Solaris shell is. Maybe you were thinking of it?"

IOW, the construct is fine to the best of my knowledge.
-Peff
Jeff King· Nov 27, 2007, 10:15 UTC · re: Jakub Narebski · lore

Re: [PATCH] Replace misleading message during interactive rebasing

On Tue, Nov 27, 2007 at 10:49:03AM +0100, Jakub Narebski wrote:
> If I remember correctly /bin/sh on Solaris cannot be used because
> of other issues (like $(...) and such).

BTW, your new mail/news gateway solution seems to send two messages. I got one that was addressed _only_ to me, and then this one came to the list (and was addressed only to the list). That means that I had to manually re-add the list when responding to the first one (or just assume that you responded to me off-list). Personally, I think this is worse than the previous "send only to the list" behavior because senders are inevitably going to fail to re-add the list (but then, I was never as bothered by others not to be cc'd in the first place).

-Peff
Junio C Hamano· Nov 28, 2007, 04:37 UTC · re: Johannes Sixt · lore

Re: [PATCH] Replace misleading message during interactive rebasing

Johannes Sixt <j.sixt@viscovery.net> writes:
> Wincent Colaiuta schrieb:
>> +		export _GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
>
> Isn't this a bashism?

Being an old-fashioned shell programmer myself, "export VAR=VAL" does raise my eyebrows. It is in POSIX but are there shells that we do support (dash, bash, ksh -- /bin/sh on Solaris does not count) that want their exports old-fashioned way?

Wincent Colaiuta· Nov 28, 2007, 08:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] Replace misleading message during interactive rebasing

El 28/11/2007, a las 5:37, Junio C Hamano escribió:
Show 11 quoted lines
> Johannes Sixt <j.sixt@viscovery.net> writes:
>
>> Wincent Colaiuta schrieb:
>>> +		export _GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
>>
>> Isn't this a bashism?
>
> Being an old-fashioned shell programmer myself, "export VAR=VAL" does
> raise my eyebrows.  It is in POSIX but are there shells that we do
> support (dash, bash, ksh -- /bin/sh on Solaris does not count) that
> want their exports old-fashioned way?

As I noted earlier in thread, the idiom is used elsewhere in Git already. Below is a list of the instances I found where shell scripts use the idiom. There are also other instances (documentation, test scripts, even a tcl file) where the idiom is used. As far as its suitability I only have directory experience with Bash, and it's always worked in all versions that I've tried in the 2.x and 3.x range. The other shells, I don't know.

In any case, below I'll paste in a revised patch that doesn't use the export FOO=... idiom. I can also submit another patch changing the other usages of the idiom elsewhere in the tree, but I'd need guidance on how far you'd want to go (only the Git commands? also the tests? don't worry about the docs? etc).

El 27/11/2007, a las 9:49, Wincent Colaiuta escribió:
Show 24 quoted lines
> I wondered that myself before submitting the patch, but then saw  
> that the same pattern was used at other places as well:
>
> git-clone.sh:
>
> W=$(cd "$GIT_WORK_TREE" && pwd) && export GIT_WORK_TREE="$W"
>
> git-filter-branch.sh:
>
> export GIT_INDEX_FILE="$(pwd)/../index"
> export GIT_COMMIT=$commit
> export GIT_COMMIT="$sha1"
>
> git-quiltimport.sh:
>
> export GIT_AUTHOR_NAME=$(sed -ne 's/Author: //p' "$tmp_info")
> export GIT_AUTHOR_EMAIL=$(sed -ne 's/Email: //p' "$tmp_info")
> export GIT_AUTHOR_DATE=$(sed -ne 's/Date: //p' "$tmp_info")
> export SUBJECT=$(sed -ne 's/Subject: //p' "$tmp_info")
>
> So if this is a problem, those sites will need to be changed as well.
>
> Cheers,
> Wincent
Here's the revised patch:
--------8<---------
 From cd11e1355011796fe16d0eb7116fae4070f2a30d Mon Sep 17 00:00:00 2001
From: Wincent Colaiuta <win@wincent.com>
Date: Mon, 26 Nov 2007 13:35:46 +0100
Subject: [PATCH] Replace misleading message during interactive rebasing

git-rebase--interactive uses git-cherry-pick under the covers to reorder commits, which in turn means that in the event of a conflict a message will be shown advising the user to commit the results and use the -c switch to retain authorship after fixing the conflict.

The message is misleading because what the user really needs to do is run "git rebase --continue"; the committing is handled by git-rebase and the authorship of the commit message is retained automatically.

We solve this problem by using an environment variable to communicate to git-cherry-pick that rebasing is underway and replace the misleading error message with a more helpful one.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
  builtin-revert.c           |    8 +++++---
  git-rebase--interactive.sh |    4 ++++
  2 files changed, 9 insertions(+), 3 deletions(-)
Show changes to 2 files +9 −3

builtin-revert.c, git-rebase--interactive.sh

diff --git a/builtin-revert.c b/builtin-revert.c
index a0586f9..5a57574 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -229,7 +229,7 @@ static int revert_or_cherry_pick(int argc, const  
char **argv)
  	unsigned char head[20];
  	struct commit *base, *next, *parent;
  	int i;
-	char *oneline, *reencoded_message = NULL;
+	char *oneline, *reencoded_message = NULL, *help_message;
  	const char *message, *encoding;
  	const char *defmsg = xstrdup(git_path("MERGE_MSG"));

@@ -352,11 +352,13 @@ static int revert_or_cherry_pick(int argc, const  
char **argv)
  		}
  		if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
  			die ("Error wrapping up %s", defmsg);
+		help_message = getenv("_GIT_CHERRY_PICK_HELP");
  		fprintf(stderr, "Automatic %s failed.  "
  			"After resolving the conflicts,\n"
  			"mark the corrected paths with 'git add <paths>' "
-			"and commit the result.\n", me);
-		if (action == CHERRY_PICK) {
+			"and %s.\n", me,
+			help_message ? help_message : "commit the result");
+		if (action == CHERRY_PICK && !help_message) {
  			fprintf(stderr, "When commiting, use the option "
  				"'-c %s' to retain authorship and message.\n",
  				find_unique_abbrev(commit->object.sha1,
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index bf44b6a..3552c89 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -117,6 +117,8 @@ pick_one () {
  		sha1=$(git rev-parse --short $sha1)
  		output warn Fast forward to $sha1
  	else
+		_GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
+		export _GIT_CHERRY_PICK_HELP &&
  		output git cherry-pick "$@"
  	fi
  }
@@ -187,6 +189,8 @@ pick_one_preserving_merges () {
  			fi
  			;;
  		*)
+			_GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
+			export _GIT_CHERRY_PICK_HELP &&
  			output git cherry-pick "$@" ||
  				die_with_patch $sha1 "Could not pick $sha1"
  			;;
-- 
1.5.3.6.953.gdffc
Johannes Schindelin· Nov 28, 2007, 12:41 UTC · re: Wincent Colaiuta · lore

Re: [PATCH] Replace misleading message during interactive rebasing

Hi,
On Wed, 28 Nov 2007, Wincent Colaiuta wrote:
Show 17 quoted lines
> @@ -352,11 +352,13 @@ static int revert_or_cherry_pick(int argc, const char
> **argv)
> 		}
> 		if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
> 			die ("Error wrapping up %s", defmsg);
> +		help_message = getenv("_GIT_CHERRY_PICK_HELP");
> 		fprintf(stderr, "Automatic %s failed.  "
> 			"After resolving the conflicts,\n"
> 			"mark the corrected paths with 'git add <paths>' "
> -			"and commit the result.\n", me);
> -		if (action == CHERRY_PICK) {
> +			"and %s.\n", me,
> +			help_message ? help_message : "commit the result");
> +		if (action == CHERRY_PICK && !help_message) {
> 			fprintf(stderr, "When commiting, use the option "
> 				"'-c %s' to retain authorship and message.\n",
> 				find_unique_abbrev(commit->object.sha1,

What about Junio's remark that _GIT_CHERRY_PICK_HELP should rather replace the _complete_ message?

You could still provide the "me" and unique_abbrev parameters, so that the first %s in _GIT_CHERRY_PICK_HELP would be replaced by the operation, and the second by the sha1. Hmm?

Ciao, Dscho

Johannes Sixt· Nov 28, 2007, 12:53 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Replace misleading message during interactive rebasing

Johannes Schindelin schrieb:
> You could still provide the "me" and unique_abbrev parameters, so that the 
> first %s in _GIT_CHERRY_PICK_HELP would be replaced by the operation, and 
> the second by the sha1.  Hmm?

Gaah! First rule to safe programming: Don't use user input as format strings. getenv(_GIT_CHERRY_PICK_HELP) *is* user input. In this case, the caller knows exactly what the put into the help string, so it should do so.

-- Hannes

← back to recent threads