Rebase/cherry-picking idea

In using "git-rebase --interactive" to re-order commits you  
occasionally get conflicts and will see a message like this:

	When commiting, use the option '-c %s' to retain authorship and message

I was thinking that it might be nice to stash away this commit id  
somewhere in GIT_DIR so that the user didn't have to explicitly  
remember it, and add a new switch to git-commit that could be used to  
automatically use that stashed commit id, something like:

	git commit --retain

Although I most often see this kind of message in interactive  
rebasing, the message is generated in builtin-revert.c when cherry- 
picking, so you can also see it in any other situation where you're  
cherry picking and there's a conflict.

What do people think? Would this be a nice usability improvement? Or  
is it adding clutter?

Cheers,
Wincent

On Nov 26, 2007, at 10:02 AM, Wincent Colaiuta wrote:

In using "git-rebase --interactive" to re-order commits you  
occasionally get conflicts and will see a message like this:

	When commiting, use the option '-c %s' to retain authorship and  
message

I was thinking that it might be nice to stash away this commit id  
somewhere in GIT_DIR so that the user didn't have to explicitly  
remember it, and add a new switch to git-commit that could be used  
to automatically use that stashed commit id, something like:

	git commit --retain

Although I most often see this kind of message in interactive  
rebasing, the message is generated in builtin-revert.c when cherry- 
picking, so you can also see it in any other situation where you're  
cherry picking and there's a conflict.

What do people think? Would this be a nice usability improvement?  
Or is it adding clutter?

I'm not sure but I think this message is just some unwanted  
(misleading) noise, since when you rebase, once you solve the  
conflicts, you git-rebase --continue, you don't git-commit.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory

El 26/11/2007, a las 10:32, Benoit Sigoure escribió:

On Nov 26, 2007, at 10:02 AM, Wincent Colaiuta wrote:

quoted

In using "git-rebase --interactive" to re-order commits you  
occasionally get conflicts and will see a message like this:

	When commiting, use the option '-c %s' to retain authorship and  
message

I was thinking that it might be nice to stash away this commit id  
somewhere in GIT_DIR so that the user didn't have to explicitly  
remember it, and add a new switch to git-commit that could be used  
to automatically use that stashed commit id, something like:

	git commit --retain

Although I most often see this kind of message in interactive  
rebasing, the message is generated in builtin-revert.c when cherry- 
picking, so you can also see it in any other situation where you're  
cherry picking and there's a conflict.

What do people think? Would this be a nice usability improvement?  
Or is it adding clutter?

I'm not sure but I think this message is just some unwanted  
(misleading) noise, since when you rebase, once you solve the  
conflicts, you git-rebase --continue, you don't git-commit.

Looks like you're right. I just did a simple test and it turns out  
that after a conflict, this:

	git commit -c ...
	git rebase --continue

Produces exactly the same history as this:

	git rebase --continue

So I think that misleading noise needs to be suppressed or reworded  
when rebasing. Will look into it.

Cheers,
Wincent

El 26/11/2007, a las 12:27, Wincent Colaiuta escribió:

El 26/11/2007, a las 10:32, Benoit Sigoure escribió:

quoted

On Nov 26, 2007, at 10:02 AM, Wincent Colaiuta wrote:

quoted

In using "git-rebase --interactive" to re-order commits you  
occasionally get conflicts and will see a message like this:

	When commiting, use the option '-c %s' to retain authorship and  
message

I was thinking that it might be nice to stash away this commit id  
somewhere in GIT_DIR so that the user didn't have to explicitly  
remember it, and add a new switch to git-commit that could be used  
to automatically use that stashed commit id, something like:

	git commit --retain

Although I most often see this kind of message in interactive  
rebasing, the message is generated in builtin-revert.c when cherry- 
picking, so you can also see it in any other situation where  
you're cherry picking and there's a conflict.

What do people think? Would this be a nice usability improvement?  
Or is it adding clutter?

I'm not sure but I think this message is just some unwanted  
(misleading) noise, since when you rebase, once you solve the  
conflicts, you git-rebase --continue, you don't git-commit.

Looks like you're right. I just did a simple test and it turns out  
that after a conflict, this:

	git commit -c ...
	git rebase --continue

Produces exactly the same history as this:

	git rebase --continue

So I think that misleading noise needs to be suppressed or reworded  
when rebasing. Will look into it.

How about something like this? It would obviously be nice if we could  
avoid adding another option to builtin-revert; perhaps when/if git- 
rebase becomes a builtin we can avoid that. The other alternative, and  
probably one I like I bit more, would be to auto-detect that a rebase  
is in progress by looking inside the GIT_DIR, although that would also  
alter the behaviour of manual invocations of git-revert and git-cherry- 
pick during an interactive rebase (do people actually do that?). What  
do you think?

diff --git a/builtin-revert.c b/builtin-revert.c
index a0586f9..36e36c3 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c

@@ -30,7 +30,7 @@ static const char * const cherry_pick_usage[] = {
  	NULL
  };

-static int edit, no_replay, no_commit, mainline;
+static int edit, no_replay, no_commit, rebasing, mainline;
  static enum { REVERT, CHERRY_PICK } action;
  static struct commit *commit;

@@ -50,6 +50,7 @@ static void parse_args(int argc, const char **argv)
  		OPT_BOOLEAN('e', "edit", &edit, "edit the commit message"),
  		OPT_BOOLEAN('x', NULL, &no_replay, "append commit name when cherry- 

picking"),
  		OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
+		OPT_BOOLEAN(0, "rebasing", &rebasing, "use rebase mode"),
  		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
  		OPT_END(),
  	};

@@ -352,11 +353,16 @@ 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);
+		if (rebasing)
+			message = "run 'git rebase --continue' "
+			    "or 'git rebase --abort'";
+		else
+			message = "commit the result";
  		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, message);
+		if (action == CHERRY_PICK && !rebasing) {
  			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..5afb843 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -117,7 +117,7 @@ pick_one () {
  		sha1=$(git rev-parse --short $sha1)
  		output warn Fast forward to $sha1
  	else
-		output git cherry-pick "$@"
+		output git cherry-pick --rebasing "$@"
  	fi
  }

@@ -187,7 +187,7 @@ pick_one_preserving_merges () {
  			fi
  			;;
  		*)
-			output git cherry-pick "$@" ||
+			output git cherry-pick --rebasing "$@" ||
  				die_with_patch $sha1 "Could not pick $sha1"
  			;;
  		esac

On Nov 26, 2007, at 1:34 PM, Wincent Colaiuta wrote:

How about something like this? It would obviously be nice if we  
could avoid adding another option to builtin-revert; perhaps when/ 
if git-rebase becomes a builtin we can avoid that. The other  
alternative, and probably one I like I bit more, would be to auto- 
detect that a rebase is in progress by looking inside the GIT_DIR,  
although that would also alter the behaviour of manual invocations  
of git-revert and git-cherry-pick during an interactive rebase (do  
people actually do that?). What do you think?

Hmm yeah, I agree that it's a little bit of a dirty workaround but,  
as you pointed out, until rebase is builtinified, this looks like the  
best/easiest alternative.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory

Wincent Colaiuta schrieb:

El 26/11/2007, a las 12:27, Wincent Colaiuta escribió:

quoted

So I think that misleading noise needs to be suppressed or reworded 
when rebasing. Will look into it.

How about something like this? It would obviously be nice if we could 
avoid adding another option to builtin-revert; perhaps when/if 
git-rebase becomes a builtin we can avoid that. The other alternative, 
and probably one I like I bit more, would be to auto-detect that a 
rebase is in progress by looking inside the GIT_DIR, although that would 
also alter the behaviour of manual invocations of git-revert and 
git-cherry-pick during an interactive rebase (do people actually do 
that?). What do you think?

Introduce an environment variable _GIT_CHERRY_PICK_HELP (note the leading 
underscore), which git-rebase sets; if it's set, git-cherry-pick uses that 
text instead of the usual one.

-- Hannes

El 26/11/2007, a las 13:51, Johannes Sixt escribió:

Wincent Colaiuta schrieb:

quoted

El 26/11/2007, a las 12:27, Wincent Colaiuta escribió:

quoted

So I think that misleading noise needs to be suppressed or  
reworded when rebasing. Will look into it.

How about something like this? It would obviously be nice if we  
could avoid adding another option to builtin-revert; perhaps when/ 
if git-rebase becomes a builtin we can avoid that. The other  
alternative, and probably one I like I bit more, would be to auto- 
detect that a rebase is in progress by looking inside the GIT_DIR,  
although that would also alter the behaviour of manual invocations  
of git-revert and git-cherry-pick during an interactive rebase (do  
people actually do that?). What do you think?

Introduce an environment variable _GIT_CHERRY_PICK_HELP (note the  
leading underscore), which git-rebase sets; if it's set, git-cherry- 
pick uses that text instead of the usual one.

Good idea, quite a bit less cruddy:

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

Hi,

On Mon, 26 Nov 2007, Benoit Sigoure wrote:

On Nov 26, 2007, at 10:02 AM, Wincent Colaiuta wrote:

quoted

In using "git-rebase --interactive" to re-order commits you occasionally get
conflicts and will see a message like this:

	When commiting, use the option '-c %s' to retain authorship and
message

I was thinking that it might be nice to stash away this commit id somewhere
in GIT_DIR so that the user didn't have to explicitly remember it, and add a
new switch to git-commit that could be used to automatically use that
stashed commit id, something like:

	git commit --retain

Although I most often see this kind of message in interactive rebasing, the
message is generated in builtin-revert.c when cherry-picking, so you can
also see it in any other situation where you're cherry picking and there's a
conflict.

What do people think? Would this be a nice usability improvement? Or is it
adding clutter?

I'm not sure but I think this message is just some unwanted (misleading)
noise, since when you rebase, once you solve the conflicts, you git-rebase
--continue, you don't git-commit.

Yep.  It is on my TODO list since a long time, but I am just as glad 
somebody else is doing it.  But I have to agree with Hannes that using an 
environment variable is cleaner, more elegant and shorter.

Ciao,
Dscho

Hi,

On Mon, 26 Nov 2007, Wincent Colaiuta wrote:

+		help_message = getenv("_GIT_CHERRY_PICK_HELP");

Why on earth do you have a leading underscore?  No existing git 
environment variable does it that way.

Ciao,
Dscho

El 26/11/2007, a las 14:41, Johannes Schindelin escribió:

Hi,

On Mon, 26 Nov 2007, Wincent Colaiuta wrote:

quoted

+		help_message = getenv("_GIT_CHERRY_PICK_HELP");

Why on earth do you have a leading underscore?  No existing git
environment variable does it that way.

I was following the suggestion of Johannes Sixt:

El 26/11/2007, a las 13:51, Johannes Sixt escribió:

Introduce an environment variable _GIT_CHERRY_PICK_HELP (note the  
leading underscore), which git-rebase sets; if it's set, git-cherry- 
pick uses that text instead of the usual one.

I imagine that he proposed it that way because it's an "internal use  
only" thing.

Once I get a clear idea of what kind of change is likely to actually  
get accepted I'll submit a proper patch.

Cheers,
Wincent