[RFC/PATCH 0/2] CHERRY_HEAD

[RFC/PATCH 0/2] CHERRY_HEAD

[Jay Soffian]

The user-experience after a cherry-pick conflicts is suboptimal.
The user has two choices for committing the resolved state:

1. To retain the original authorship and commit message, the user can
   'git commit -c <original commit id>'. This is what cherry-pick itself
   advises. In this case MERGE_MSG (generated during the cherry-pick) is
   ignored. That's bad since MERGE_MSG contains the list of conflicts 
   which is nice to have in the new commit. If 'cherry pick -x' was used,
   the -x annotation is lost. Asking the user to remember the original
   commit id is also a bit harsh.

2. To reset the authorship, the user can 'git commit'. This will use the
   MERGE_MSG, but the original authorship is lost.

We fix both of these issues by recording the original commit in CHERRY_HEAD
if a conflict occurs during cherr-pick. We teach commit to use CHERRY_HEAD
to retrieve the original authorship (unless --reset-author is used), but
take the commit message from MERGE_MSG.

A further improvement would be to teach cherry-pick --continue and --abort
options a la rebase, but I think that should be prototyped in shell-script.

Jay Soffian (2):
  Introduce CHERRY_HEAD
  Teach commit to handle CHERRY_HEAD automatically

 Documentation/git-commit.txt           |    7 +++--
 branch.c                               |    1 +
 builtin/commit.c                       |   36 ++++++++++++++++++++++++++-----
 builtin/revert.c                       |   20 ++++++++++++++++-
 contrib/completion/git-completion.bash |    2 +
 5 files changed, 55 insertions(+), 11 deletions(-)

-- 
1.7.4.5.g9affb

[RFC/PATCH 1/2] Introduce CHERRY_HEAD

[Jay Soffian]

When a cherry-pick conflicts git advises to use:

 $ git commit -c <original commit id>

to preserve the original commit message and authorship. Instead,
let's record the original commit id in CHERRY_HEAD and advise to use:

  $ git commit -c CHERRY_HEAD

In the next commit, we teach git to handle the '-c CHERRY_HEAD' part.
---
 branch.c         |    1 +
 builtin/commit.c |    1 +
 builtin/revert.c |   23 ++++++++++++++++++++---
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/branch.c b/branch.c
index 93dc866..81bbf95 100644
--- a/branch.c
+++ b/branch.c
@@ -217,6 +217,7 @@ void create_branch(const char *head,
 
 void remove_branch_state(void)
 {
+	unlink(git_path("CHERRY_HEAD"));
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_RR"));
 	unlink(git_path("MERGE_MSG"));
diff --git a/builtin/commit.c b/builtin/commit.c
index 03cff5a..8850621 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1424,6 +1424,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		die("cannot update HEAD ref");
 	}
 
+	unlink(git_path("CHERRY_HEAD"));
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
 	unlink(git_path("MERGE_MODE"));
diff --git a/builtin/revert.c b/builtin/revert.c
index dc1b702..c373e69 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -248,6 +248,22 @@ static void set_author_ident_env(const char *message)
 			sha1_to_hex(commit->object.sha1));
 }
 
+static void write_cherry_head(void)
+{
+	int fd;
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
+
+	fd = open(git_path("CHERRY_HEAD"), O_WRONLY | O_CREAT, 0666);
+	if (fd < 0)
+		die_errno("Could not open '%s' for writing",
+			  git_path("CHERRY_HEAD"));
+	if (write_in_full(fd, buf.buf, buf.len) != buf.len)
+		die_errno("Could not write to '%s'", git_path("CHERRY_HEAD"));
+	close(fd);
+}
+
 static void advise(const char *advice, ...)
 {
 	va_list params;
@@ -269,9 +285,10 @@ static void print_advice(void)
 	advise("after resolving the conflicts, mark the corrected paths");
 	advise("with 'git add <paths>' or 'git rm <paths>'");
 
-	if (action == CHERRY_PICK)
-		advise("and commit the result with 'git commit -c %s'",
-		       find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
+	if (action == CHERRY_PICK) {
+		write_cherry_head();
+		advise("and commit the result with 'git commit -c CHERRY_HEAD'");
+	}
 }
 
 static void write_message(struct strbuf *msgbuf, const char *filename)
-- 
1.7.4.5.g9affb

Re: [RFC/PATCH 1/2] Introduce CHERRY_HEAD

[Junio C Hamano]

Introducing a new file that ends with _HEAD is fine, but shouldn't this be
CHERRY_PICK_HEAD instead?

Re: [RFC/PATCH 1/2] Introduce CHERRY_HEAD

[Jonathan Nieder]

Jay Soffian wrote:

> When a cherry-pick conflicts git advises to use:
>
>  $ git commit -c <original commit id>
>
> to preserve the original commit message and authorship. Instead,
> let's record the original commit id in CHERRY_HEAD and advise to use:
>
>   $ git commit -c CHERRY_HEAD
>
> In the next commit, we teach git to handle the '-c CHERRY_HEAD' part.

Nice!  Thanks for working on this.

I wonder if cherry-pick shouldn't also write MERGE_MSG or similar so
that gets taken care of automatically?  That would also allow options
like -x and -m to work better.

Sign-off?

> +++ b/branch.c
> @@ -217,6 +217,7 @@ void create_branch(const char *head,
>
>  void remove_branch_state(void)
>  {
> +	unlink(git_path("CHERRY_HEAD"));
>  	unlink(git_path("MERGE_HEAD"));
[...]
> +++ b/builtin/commit.c
> @@ -1424,6 +1424,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
[...]
>  
> +	unlink(git_path("CHERRY_HEAD"));
>  	unlink(git_path("MERGE_HEAD"));

Another piece of code that checks for MERGE_HEAD is cmd_merge's
"You have not concluded your merge" check.  Should it check for
CHERRY_HEAD, too?

> +++ b/builtin/revert.c
> @@ -248,6 +248,22 @@ static void set_author_ident_env(const char *message)
>  			sha1_to_hex(commit->object.sha1));
>  }
>  
> +static void write_cherry_head(void)
> +{
> +	int fd;
> +	struct strbuf buf = STRBUF_INIT;

Leaked, I think.  I would have been tempted to put it in a char buf[50] ---
we can be glad you are writing this code. :)

With whatever subset of the following seems useful,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-cherry-pick.txt |   19 +++++++++++++++++++
 Documentation/revisions.txt       |    5 ++++-
 builtin/merge.c                   |    7 +++++++
 builtin/revert.c                  |    4 ++--
 t/t3507-cherry-pick-conflict.sh   |   22 +++++++++++++++++++++-
 5 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 749d68a..e8db99b 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -16,6 +16,25 @@ Given one or more existing commits, apply the change each one
 introduces, recording a new commit for each.  This requires your
 working tree to be clean (no modifications from the HEAD commit).
 
+When it is not obvious how to apply a change, the following
+happens:
+
+1. The current branch and `HEAD` pointer stay at the last commit
+   successfully made.
+2. The `CHERRY_HEAD` ref is set to point at the commit that
+   introduced the change that is difficult to apply.
+3. Paths in which the change applied cleanly are updated both
+   in the index file and in your working tree.
+4. For conflicting paths, the index file records up to three
+   versions, as described in the "TRUE MERGE" section of
+   linkgit:git-merge[1].  The working tree files will include
+   a description of the conflict bracketed by the usual
+   conflict markers `<<<<<<<` and `>>>>>>>`.
+5. No other modifications are made.
+
+See linkgit:git-merge[1] for some hints on resolving such
+conflicts.
+
 OPTIONS
 -------
 <commit>...::
diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 9e92734..02f0813 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -25,7 +25,8 @@ blobs contained in a commit.
   first match in the following rules:
 
   . if `$GIT_DIR/<name>` exists, that is what you mean (this is usually
-    useful only for `HEAD`, `FETCH_HEAD`, `ORIG_HEAD` and `MERGE_HEAD`);
+    useful only for `HEAD`, `FETCH_HEAD`, `CHERRY_HEAD`, `ORIG_HEAD`
+    and `MERGE_HEAD`);
 
   . otherwise, `refs/<name>` if exists;
 
@@ -40,6 +41,8 @@ blobs contained in a commit.
 HEAD names the commit your changes in the working tree is based on.
 FETCH_HEAD records the branch you fetched from a remote repository
 with your last 'git fetch' invocation.
+CHERRY_HEAD records the commit you are applying the change from
+when you run 'git cherry-pick'.
 ORIG_HEAD is created by commands that moves your HEAD in a drastic
 way, to record the position of the HEAD before their operation, so that
 you can change the tip of the branch back to the state before you ran
diff --git a/builtin/merge.c b/builtin/merge.c
index 9403747..cabfc9c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -971,6 +971,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		else
 			die("You have not concluded your merge (MERGE_HEAD exists).");
 	}
+	if (file_exists(git_path("CHERRY_HEAD"))) {
+		if (advice_resolve_conflict)
+			die("You have not concluded your cherry-pick (CHERRY_HEAD exists).\n"
+			    "Please, commit your changes before you can merge.");
+		else
+			die("You have not concluded your cherry-pick (CHERRY_HEAD exists).");
+	}
 	resolve_undo_clear();
 
 	if (verbosity < 0)
diff --git a/builtin/revert.c b/builtin/revert.c
index c373e69..04da0e1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -259,9 +259,9 @@ static void write_cherry_head(void)
 	if (fd < 0)
 		die_errno("Could not open '%s' for writing",
 			  git_path("CHERRY_HEAD"));
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len)
+	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
 		die_errno("Could not write to '%s'", git_path("CHERRY_HEAD"));
-	close(fd);
+	strbuf_release(&buf);
 }
 
 static void advise(const char *advice, ...)
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 607bf25..71a2167 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -11,6 +11,12 @@ test_description='test cherry-pick and revert with conflicts
 
 . ./test-lib.sh
 
+test_cmp_rev () {
+	git rev-parse --verify "$1" >expect.rev &&
+	git rev-parse --verify "$2" >actual.rev &&
+	test_cmp expect.rev actual.rev
+}
+
 test_expect_success setup '
 
 	echo unrelated >unrelated &&
@@ -51,13 +57,27 @@ test_expect_success 'advice from failed cherry-pick' "
 	error: could not apply \$picked... picked
 	hint: after resolving the conflicts, mark the corrected paths
 	hint: with 'git add <paths>' or 'git rm <paths>'
-	hint: and commit the result with 'git commit -c \$picked'
+	hint: and commit the result with 'git commit -c CHERRY_HEAD'
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
 
 	test_cmp expected actual
 "
 
+test_expect_success 'failed cherry-pick sets CHERRY_HEAD' '
+
+	git checkout -f initial^0 &&
+	git read-tree -u --reset HEAD &&
+	git clean -d -f -f -q -x &&
+
+	git update-index --refresh &&
+	git diff-index --exit-code HEAD &&
+
+	test_must_fail git cherry-pick picked &&
+
+	test_cmp_rev picked CHERRY_HEAD
+'
+
 test_expect_success 'failed cherry-pick produces dirty index' '
 
 	git checkout -f initial^0 &&
-- 
1.7.4.1

Re: [RFC/PATCH 1/2] Introduce CHERRY_HEAD

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I wonder if cherry-pick shouldn't also write MERGE_MSG or similar so
> that gets taken care of automatically?  That would also allow options
> like -x and -m to work better.

Hm, that probably is a good idea.

> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index 749d68a..e8db99b 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -16,6 +16,25 @@ Given one or more existing commits, apply the change each one
>  introduces, recording a new commit for each.  This requires your
>  working tree to be clean (no modifications from the HEAD commit).
>  
> +When it is not obvious how to apply a change, the following
> +happens:
> +
> +1. The current branch and `HEAD` pointer stay at the last commit
> +   successfully made.
> +2. The `CHERRY_HEAD` ref is set to point at the commit that
> +   introduced the change that is difficult to apply.
> +3. Paths in which the change applied cleanly are updated both
> +   in the index file and in your working tree.
> +4. For conflicting paths, the index file records up to three
> +   versions, as described in the "TRUE MERGE" section of
> +   linkgit:git-merge[1].  The working tree files will include
> +   a description of the conflict bracketed by the usual
> +   conflict markers `<<<<<<<` and `>>>>>>>`.

What happened to the `=======`?  I thought you were copying and pasting
from the said section.

Re: [RFC/PATCH 1/2] Introduce CHERRY_HEAD

[Bert Wesarg]

On Tue, Feb 15, 2011 at 23:59, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> I wonder if cherry-pick shouldn't also write MERGE_MSG or similar so
>> that gets taken care of automatically?  That would also allow options
>> like -x and -m to work better.
>
> Hm, that probably is a good idea.
>
>> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
>> index 749d68a..e8db99b 100644
>> --- a/Documentation/git-cherry-pick.txt
>> +++ b/Documentation/git-cherry-pick.txt
>> @@ -16,6 +16,25 @@ Given one or more existing commits, apply the change each one
>>  introduces, recording a new commit for each.  This requires your
>>  working tree to be clean (no modifications from the HEAD commit).
>>
>> +When it is not obvious how to apply a change, the following
>> +happens:
>> +
>> +1. The current branch and `HEAD` pointer stay at the last commit
>> +   successfully made.
>> +2. The `CHERRY_HEAD` ref is set to point at the commit that
>> +   introduced the change that is difficult to apply.
>> +3. Paths in which the change applied cleanly are updated both
>> +   in the index file and in your working tree.
>> +4. For conflicting paths, the index file records up to three
>> +   versions, as described in the "TRUE MERGE" section of
>> +   linkgit:git-merge[1].  The working tree files will include
>> +   a description of the conflict bracketed by the usual
>> +   conflict markers `<<<<<<<` and `>>>>>>>`.
>
> What happened to the `=======`?  I thought you were copying and pasting
> from the said section.

I think this is clear enough, else you would need to mention the
'|||||||' and conflict_marker_size too.

Bert

Re: [RFC/PATCH 1/2] Introduce CHERRY_HEAD

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

>> What happened to the `=======`?  I thought you were copying and pasting
>> from the said section.
>
> I think this is clear enough, else you would need to mention the
> '|||||||' and conflict_marker_size too.

If so then probably the section this was copied from needs updating?

Re: [RFC/PATCH 1/2] Introduce CHERRY_HEAD

[Bert Wesarg]

On Wed, Feb 16, 2011 at 00:10, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>>> What happened to the `=======`?  I thought you were copying and pasting
>>> from the said section.
>>
>> I think this is clear enough, else you would need to mention the
>> '|||||||' and conflict_marker_size too.
>
> If so then probably the section this was copied from needs updating?
>

Right, the origin does not mention them too. It also uses just 3
characters, instead of the default 7. I think using '<=>'/'<|=>' as a
visual hint about conflict markers should suffice here plus
referencing the next section. This next section
HOW CONFLICTS ARE PRESENTED does describe both conflict styles but it
doesn't mention the conflict-marker-size option. There are more
references in the documentation where only the RCS style is mentioned.

Anyways, referencing "TRUE MERGE" alone is not enough, it should
mention the following sections too, namely HOW CONFLICTS ARE PRESENTED
and HOW TO RESOLVE CONFLICTS. In German we would do this by '"TRUE
MERGE" ff.' Where 'ff.' is the plural of the abbreviation for
'following'. I don't know how to do that in English.

Also I'm wondering, why the git repo does not set the
conflict-marker-size for its offending files itself. The current
offender would be:

$ git grep -l '^<<<<<<<' Documentation/
Documentation/git-merge-file.txt
Documentation/git-merge.txt
Documentation/user-manual.txt

I may provide a patch in the future, if no one beats me.

Bert

Re: [RFC/PATCH 1/2] Introduce CHERRY_HEAD

[Jay Soffian]

On Tue, Feb 15, 2011 at 5:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> I wonder if cherry-pick shouldn't also write MERGE_MSG or similar so
>> that gets taken care of automatically?  That would also allow options
>> like -x and -m to work better.
>
> Hm, that probably is a good idea.

I must not understand what either of you is asking. MERGE_MSG is
written by cherry-pick already, it's just that it's ignored if you use
commit -c, as cherry-pick advises. (Before this patch series anyway.)

j.

Re: [RFC/PATCH 1/2] Introduce CHERRY_HEAD

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> I wonder if cherry-pick shouldn't also write MERGE_MSG or similar so
>> that gets taken care of automatically?  That would also allow options
>> like -x and -m to work better.
>
> Hm, that probably is a good idea.

Turns out it already does. :)

>> +++ b/Documentation/git-cherry-pick.txt
>> @@ -16,6 +16,25 @@ Given one or more existing commits, apply the change each one
[...]
>> +When it is not obvious how to apply a change, the following
>> +happens:
>> +
>> +1. The current branch and `HEAD` pointer stay at the last commit
>> +   successfully made.
>> +2. The `CHERRY_HEAD` ref is set to point at the commit that
>> +   introduced the change that is difficult to apply.
>> +3. Paths in which the change applied cleanly are updated both
>> +   in the index file and in your working tree.
>> +4. For conflicting paths, the index file records up to three
>> +   versions, as described in the "TRUE MERGE" section of
>> +   linkgit:git-merge[1].  The working tree files will include
>> +   a description of the conflict bracketed by the usual
>> +   conflict markers `<<<<<<<` and `>>>>>>>`.
>
> What happened to the `=======`?  I thought you were copying and pasting
> from the said section.

Hmm, maybe as a temporary cop-out:

	What it is not obvious how to apply a change, the following
	happens:

	1. The current branch ...
	...
	3. Paths in which the change applied cleanly are updated both
	   in the index file and in your working tree.
	4. For conflicting paths, the index file and working tree
	   files record a description of the conflict, as described
	   in the "TRUE MERGE" section of linkgit:git-merge[1].

I didn't want to copy and paste wholesale to avoid dull reading and to
avoid the two copies falling out of sync.  Maybe there should be a
page on conflicts and their resolution for git-merge(1),
git-rebase(1), git-cherry-pick(1), and git-revert(1) to refer to?

[RFC/PATCH 2/2] Teach commit to handle CHERRY_HEAD automatically

[Jay Soffian]

Now that cherry-pick records the original commit id in CHERRY_HEAD
following a conflict, we can teach commit to handle it automatically.

The primary difference between this and 'commit -c CHERRY_HEAD'
(besides saving the user some typing) is that we take only the
authorship from CHERRY_HEAD, while taking the commit message
from MERGE_MSG (which contains the list of conflicts as well
as cherry-pick -x's annotation, if -x was used).

--reset-author can be used to reset authorship.
---
There are several subtle interactions between the sundry commit options that
make my head hurt. I did my best to integrate what happens when there is
a CHERRY_HEAD, but I'm really not sure how CHERRY_HEAD ought to interact
with things like --squash and --fixup, or the message specifying options.

For example, it should probably be an error to use --squash/--fixup when
there's a CHERRY_HEAD, but then again, it should probably be an error to
use those options when there's a MERGE_HEAD (but it's not).

OTOH, it seems valid to specify a commit message manually, so I allow that.

 Documentation/git-commit.txt           |    7 +++--
 builtin/commit.c                       |   35 ++++++++++++++++++++++++++-----
 builtin/revert.c                       |    5 +--
 contrib/completion/git-completion.bash |    2 +
 4 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index b586c0f..fd6a1f7 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -84,9 +84,10 @@ OPTIONS
 	linkgit:git-rebase[1] for details.
 
 --reset-author::
-	When used with -C/-c/--amend options, declare that the
-	authorship of the resulting commit now belongs of the committer.
-	This also renews the author timestamp.
+	When used with -C/-c/--amend options, or when committing after a
+	a conflicting cherry-pick, declare that the authorship of the
+	resulting commit now belongs of the committer. This also renews
+	the author timestamp.
 
 --short::
 	When doing a dry-run, give the output in the short-format. See
diff --git a/builtin/commit.c b/builtin/commit.c
index 8850621..2f0a8fc 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -70,7 +70,7 @@ static const char *logfile, *force_author;
 static const char *template_file;
 static char *edit_message, *use_message;
 static char *fixup_message, *squash_message;
-static int all, edit_flag, also, interactive, only, amend, signoff;
+static int all, edit_flag, also, interactive, only, amend, signoff, cherry_pick;
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
@@ -609,7 +609,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			die_errno("could not read log file '%s'",
 				  logfile);
 		hook_arg1 = "message";
-	} else if (use_message) {
+	} else if (use_message && !cherry_pick) {
 		buffer = strstr(use_message_buffer, "\n\n");
 		if (!buffer || buffer[2] == '\0')
 			die("commit has empty message");
@@ -704,6 +704,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				"#\n",
 				git_path("MERGE_HEAD"));
 
+		if (cherry_pick)
+			fprintf(fp,
+				"#\n"
+				"# It looks like you may be committing a cherry-pick.\n"
+				"# If this is not correct, please remove the file\n"
+				"#	%s\n"
+				"# and try again.\n"
+				"#\n",
+				git_path("CHERRY_HEAD"));
 		fprintf(fp,
 			"\n"
 			"# Please enter the commit message for your changes.");
@@ -898,6 +907,7 @@ static void handle_untracked_files_arg(struct wt_status *s)
 		die("Invalid untracked files mode '%s'", untracked_files_arg);
 }
 
+
 static int parse_and_validate_options(int argc, const char *argv[],
 				      const char * const usage[],
 				      const char *prefix,
@@ -929,6 +939,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		die("You have nothing to amend.");
 	if (amend && in_merge)
 		die("You are in the middle of a merge -- cannot amend.");
+	if (amend && cherry_pick)
+		die("You are in the middle of a cherry-pick -- cannot amend.");
 	if (fixup_message && squash_message)
 		die("Options --squash and --fixup cannot be used together");
 	if (use_message)
@@ -943,11 +955,19 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		die("Only one of -c/-C/-F/--fixup can be used.");
 	if (message.len && f > 0)
 		die("Option -m cannot be combined with -c/-C/-F/--fixup.");
+	if (cherry_pick) {
+		/* Let message-specifying options override CHERRY_HEAD */
+		if (f > 0 || message.len)
+			cherry_pick = 0;
+		else
+			/* used for authorship side-effect only */
+			use_message = "CHERRY_HEAD";
+	}
 	if (edit_message)
 		use_message = edit_message;
 	if (amend && !use_message && !fixup_message)
 		use_message = "HEAD";
-	if (!use_message && renew_authorship)
+	if (!use_message && !cherry_pick && renew_authorship)
 		die("--reset-author can be used only with -C, -c or --amend.");
 	if (use_message) {
 		const char *out_enc;
@@ -1118,6 +1138,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	gitmodules_config();
 	git_config(git_status_config, &s);
 	in_merge = file_exists(git_path("MERGE_HEAD"));
+	cherry_pick = file_exists(git_path("CHERRY_HEAD"));
 	argc = parse_options(argc, argv, prefix,
 			     builtin_status_options,
 			     builtin_status_usage, 0);
@@ -1140,7 +1161,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	}
 
 	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
-	s.in_merge = in_merge;
+	s.in_merge = in_merge || cherry_pick;
 	s.ignore_submodule_arg = ignore_submodule_arg;
 	wt_status_collect(&s);
 
@@ -1303,7 +1324,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	wt_status_prepare(&s);
 	git_config(git_commit_config, &s);
 	in_merge = file_exists(git_path("MERGE_HEAD"));
-	s.in_merge = in_merge;
+	cherry_pick = file_exists(git_path("CHERRY_HEAD"));
+	s.in_merge = in_merge || cherry_pick;
 
 	if (s.use_color == -1)
 		s.use_color = git_use_color_default;
@@ -1369,7 +1391,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			parents = reduce_heads(parents);
 	} else {
 		if (!reflog_msg)
-			reflog_msg = "commit";
+			reflog_msg = cherry_pick ? "commit (cherry-pick)"
+						 : "commit";
 		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
 	}
 
diff --git a/builtin/revert.c b/builtin/revert.c
index c373e69..678fc8e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -284,11 +284,10 @@ static void print_advice(void)
 
 	advise("after resolving the conflicts, mark the corrected paths");
 	advise("with 'git add <paths>' or 'git rm <paths>'");
+	advice("and commit the result with 'git commit'");
 
-	if (action == CHERRY_PICK) {
+	if (action == CHERRY_PICK)
 		write_cherry_head();
-		advise("and commit the result with 'git commit -c CHERRY_HEAD'");
-	}
 }
 
 static void write_message(struct strbuf *msgbuf, const char *filename)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 893b771..1d446cf 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -246,6 +246,8 @@ __git_ps1 ()
 				fi
 			elif [ -f "$g/MERGE_HEAD" ]; then
 				r="|MERGING"
+			elif [ -f "$g/CHERRY_HEAD" ]; then
+				r="|CHERRY-PICKING"
 			elif [ -f "$g/BISECT_LOG" ]; then
 				r="|BISECTING"
 			fi
-- 
1.7.4.5.g9affb

Re: [RFC/PATCH 2/2] Teach commit to handle CHERRY_HEAD automatically

[Jay Soffian]

On Tue, Feb 15, 2011 at 4:23 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -284,11 +284,10 @@ static void print_advice(void)
>
>        advise("after resolving the conflicts, mark the corrected paths");
>        advise("with 'git add <paths>' or 'git rm <paths>'");
> +       advice("and commit the result with 'git commit'");

Obvious typo, I amended it but apparently ran format-patch before the
amend. Assuming the series is otherwise okay it'll be fixed in the
re-roll with tests and SoB.

j.

Re: [RFC/PATCH 2/2] Teach commit to handle CHERRY_HEAD automatically

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 8850621..2f0a8fc 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -70,7 +70,7 @@ static const char *logfile, *force_author;
>  static const char *template_file;
>  static char *edit_message, *use_message;
>  static char *fixup_message, *squash_message;
> -static int all, edit_flag, also, interactive, only, amend, signoff;
> +static int all, edit_flag, also, interactive, only, amend, signoff, cherry_pick;

This doesn't belong here; it should come next to "in_merge" that marks us
to be "in the middle of concluding a merge", and it probably is better to
call it "in_cherry_pick" to be consistent.

> @@ -704,6 +704,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  				"#\n",
>  				git_path("MERGE_HEAD"));
>  
> +		if (cherry_pick)
> +			fprintf(fp,
> +				"#\n"
> +				"# It looks like you may be committing a cherry-pick.\n"
> +				"# If this is not correct, please remove the file\n"
> +				"#	%s\n"
> +				"# and try again.\n"
> +				"#\n",
> +				git_path("CHERRY_HEAD"));

Yeah, this shows clearly that in_merge is very similar to this new mode of
operation.

> @@ -929,6 +939,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  		die("You have nothing to amend.");
>  	if (amend && in_merge)
>  		die("You are in the middle of a merge -- cannot amend.");
> +	if (amend && cherry_pick)
> +		die("You are in the middle of a cherry-pick -- cannot amend.");
>  	if (fixup_message && squash_message)
>  		die("Options --squash and --fixup cannot be used together");
>  	if (use_message)

So does this.

Makes one wonder why the hunk that begins at line 609 special cases only
this new mode, no?

> @@ -943,11 +955,19 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  		die("Only one of -c/-C/-F/--fixup can be used.");
>  	if (message.len && f > 0)
>  		die("Option -m cannot be combined with -c/-C/-F/--fixup.");
> +	if (cherry_pick) {
> +		/* Let message-specifying options override CHERRY_HEAD */
> +		if (f > 0 || message.len)
> +			cherry_pick = 0;
> +		else
> +			/* used for authorship side-effect only */
> +			use_message = "CHERRY_HEAD";
> +	}
>  	if (edit_message)
>  		use_message = edit_message;
>  	if (amend && !use_message && !fixup_message)
>  		use_message = "HEAD";
> -	if (!use_message && renew_authorship)
> +	if (!use_message && !cherry_pick && renew_authorship)
>  		die("--reset-author can be used only with -C, -c or --amend.");
>  	if (use_message) {
>  		const char *out_enc;

Likewise.  Perhaps these show that the way updated code uses the
use_message variable needs some rethinking.

> @@ -1118,6 +1138,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  	gitmodules_config();
>  	git_config(git_status_config, &s);
>  	in_merge = file_exists(git_path("MERGE_HEAD"));
> +	cherry_pick = file_exists(git_path("CHERRY_HEAD"));
>  	argc = parse_options(argc, argv, prefix,
>  			     builtin_status_options,
>  			     builtin_status_usage, 0);
> @@ -1140,7 +1161,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
> -	s.in_merge = in_merge;
> +	s.in_merge = in_merge || cherry_pick;

Ugly.  What does s.in_merge _MEAN_ after this patch gets applied?

I am not at all opposed to extending the semantics of an existing field of
the structure (i.e. "doing this and that when concluding a conflicted
merge made sense, and now we realize that doing exactly the same this and
that makes sense when concluding a conflicted cherry-pick" is perfectly
fine), but then that updated semantics should get a new name to cover both
old and new use scenario.  You are _not_ in "in-merge" anymore but trying
to get a behaviour from other parts of the system that is similar to what
you would get when "in-merge".  What is it?  That is what you should base
the new name for the field on.

> @@ -1369,7 +1391,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  			parents = reduce_heads(parents);
>  	} else {
>  		if (!reflog_msg)
> -			reflog_msg = "commit";
> +			reflog_msg = cherry_pick ? "commit (cherry-pick)"
> +						 : "commit";

This seems to indicate that we don't say "commit (merge)" when concluding
a conflicted merge.  Shouldn't we?

Re: [RFC/PATCH 2/2] Teach commit to handle CHERRY_HEAD automatically

[Jonathan Nieder]

Hi,

Some quick thoughts.

Jay Soffian wrote:

> Now that cherry-pick records the original commit id in CHERRY_HEAD
> following a conflict, we can teach commit to handle it automatically.
> 
> The primary difference between this and 'commit -c CHERRY_HEAD'
> (besides saving the user some typing) is that we take only the
> authorship from CHERRY_HEAD, while taking the commit message
> from MERGE_MSG

This answers my question from before.

[...]
> For example, it should probably be an error to use --squash/--fixup when
> there's a CHERRY_HEAD, but then again, it should probably be an error to
> use those options when there's a MERGE_HEAD (but it's not).

I see what you're saying about MERGE_HEAD --- it is not clear what is
intended when a person asks "rebase --autosquash" to squash in a merge
commit.  With CHERRY_PICK_HEAD, it means "after dealing with the
conflict, I understand this commit better, and it ought to have been
squashed in with commit X", no?

There is some potential for confusion from another direction.  The
uncareful patch series maintainer can be tempted to try, after
conflict resolution,

 - "git commit --amend" to say "I'm done fixing the broken thing".

 - "git commit --fixup=HEAD/--squash=HEAD" to say "done fixing;
   let's look at this again later and squash it when I am less
   confused".

Both are mistakes if HEAD is the previous and good commit rather than
the broken one.  Maybe there is some simple safety that could protect
against them?

> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -84,9 +84,10 @@ OPTIONS
>  	linkgit:git-rebase[1] for details.
>
>  --reset-author::
> -	When used with -C/-c/--amend options, declare that the
> -	authorship of the resulting commit now belongs of the committer.
> -	This also renews the author timestamp.
> +	When used with -C/-c/--amend options, or when committing after a
> +	a conflicting cherry-pick,

or when committing after a conflicted merge, no?

>                                  declare that the authorship of the
> +	resulting commit now belongs of the committer. This also renews
> +	the author timestamp.

How does it interact with --author?

> +++ b/builtin/commit.c
[...]
> @@ -609,7 +609,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  			die_errno("could not read log file '%s'",
>  				  logfile);
>  		hook_arg1 = "message";
> -	} else if (use_message) {
> +	} else if (use_message && !cherry_pick) {

Wouldn't this make

	git commit -C foo

after a failed cherry-pick use MERGE_MSG instead of the message the
user requested?

> @@ -704,6 +704,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  				"#\n",
>  				git_path("MERGE_HEAD"));
>  
> +		if (cherry_pick)
> +			fprintf(fp,
> +				"#\n"
> +				"# It looks like you may be committing a cherry-pick.\n"

Aside: shouldn't we suggest some porcelain-ish command (git merge
--clear?  git commit --no-merge?) to remove .git/MERGE_HEAD instead of
asking the committer to do it?

This section is used to give a preview and sanity check for the
commit.

 - if we are on the wrong branch, that might be a mistake.
 - if the author is someone else, that might be a mistake.
 - if there are multiple parents, that might be a mistake.
 - if there are changes included, some might be mistakes.
 - if there are changes excluded, some might be mistakes,
 - if there are untracked files, some might be mistakes.

Where does committing a cherry-pick fall into that picture?  Maybe

	# Author:    A U Thor <author@example.com>
	# Date:      Tue Beb 9 01:23:45 1911 -0500
	#
	# If this is not correct, please try again using the
	# --author and --date or --reset-author options.

> @@ -898,6 +907,7 @@ static void handle_untracked_files_arg(struct wt_status *s)
>  		die("Invalid untracked files mode '%s'", untracked_files_arg);
>  }
>  
> +

Stray whitespace change?

> @@ -929,6 +939,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  		die("You have nothing to amend.");
>  	if (amend && in_merge)
>  		die("You are in the middle of a merge -- cannot amend.");
> +	if (amend && cherry_pick)
> +		die("You are in the middle of a cherry-pick -- cannot amend.");

Ah, this addresses the worry mentioned above.

How can I get out of this state if I really do want to amend?

> @@ -943,11 +955,19 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  		die("Only one of -c/-C/-F/--fixup can be used.");
>  	if (message.len && f > 0)
>  		die("Option -m cannot be combined with -c/-C/-F/--fixup.");
> +	if (cherry_pick) {
> +		/* Let message-specifying options override CHERRY_HEAD */
> +		if (f > 0 || message.len)
> +			cherry_pick = 0;
> +		else
> +			/* used for authorship side-effect only */
> +			use_message = "CHERRY_HEAD";
> +	}

Hmm, what if I have commits F and F' and after trying to cherry-pick F
I decide I want the message from F'?

> @@ -1118,6 +1138,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  	gitmodules_config();
>  	git_config(git_status_config, &s);
>  	in_merge = file_exists(git_path("MERGE_HEAD"));
> +	cherry_pick = file_exists(git_path("CHERRY_HEAD"));

Is it possible to be both in_merge and cherry_pick?

> @@ -1369,7 +1391,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  			parents = reduce_heads(parents);
>  	} else {
>  		if (!reflog_msg)
> -			reflog_msg = "commit";
> +			reflog_msg = cherry_pick ? "commit (cherry-pick)"
> +						 : "commit";

Nice.  Probably worth mentioning in the commit message.

> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -246,6 +246,8 @@ __git_ps1 ()
>  				fi
>  			elif [ -f "$g/MERGE_HEAD" ]; then
>  				r="|MERGING"
> +			elif [ -f "$g/CHERRY_HEAD" ]; then
> +				r="|CHERRY-PICKING"

Likewise.

Re: [RFC/PATCH 2/2] Teach commit to handle CHERRY_HEAD automatically

[Jay Soffian]

On Tue, Feb 15, 2011 at 6:00 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>  - "git commit --amend" to say "I'm done fixing the broken thing".
>
>  - "git commit --fixup=HEAD/--squash=HEAD" to say "done fixing;
>   let's look at this again later and squash it when I am less
>   confused".
>
> Both are mistakes if HEAD is the previous and good commit rather than
> the broken one.  Maybe there is some simple safety that could protect
> against them?

As you see below, this is already protected against.

>>  --reset-author::
>> -     When used with -C/-c/--amend options, declare that the
>> -     authorship of the resulting commit now belongs of the committer.
>> -     This also renews the author timestamp.
>> +     When used with -C/-c/--amend options, or when committing after a
>> +     a conflicting cherry-pick,
>
> or when committing after a conflicted merge, no?

No. The person committing a merge is already the author of the merge,
why would they use --reset-author?

>
>>                                  declare that the authorship of the
>> +     resulting commit now belongs of the committer. This also renews
>> +     the author timestamp.
>
> How does it interact with --author?

No change from before, --author forces the author of the new commit.

>> +++ b/builtin/commit.c
> [...]
>> @@ -609,7 +609,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>>                       die_errno("could not read log file '%s'",
>>                                 logfile);
>>               hook_arg1 = "message";
>> -     } else if (use_message) {
>> +     } else if (use_message && !cherry_pick) {
>
> Wouldn't this make
>
>        git commit -C foo
>
> after a failed cherry-pick use MERGE_MSG instead of the message the
> user requested?

No, because the -C will force the cherry_pick flag to 0 in
parse_and_validate_options:

		/* Let message-specifying options override CHERRY_HEAD */

I'll make this logic clearer though as I need to address Junio's
earlier message.

>> +             if (cherry_pick)
>> +                     fprintf(fp,
>> +                             "#\n"
>> +                             "# It looks like you may be committing a cherry-pick.\n"
>
> Aside: shouldn't we suggest some porcelain-ish command (git merge
> --clear?  git commit --no-merge?) to remove .git/MERGE_HEAD instead of
> asking the committer to do it?

We have git merge --reset as an alias for reset --merge. Since reset
--merge takes care of this case too (I think, I'll check) we can
suggest that for both.

> This section is used to give a preview and sanity check for the
> commit.
>
>  - if we are on the wrong branch, that might be a mistake.
>  - if the author is someone else, that might be a mistake.
>  - if there are multiple parents, that might be a mistake.
>  - if there are changes included, some might be mistakes.
>  - if there are changes excluded, some might be mistakes,
>  - if there are untracked files, some might be mistakes.
>
> Where does committing a cherry-pick fall into that picture?  Maybe
>
>        # Author:    A U Thor <author@example.com>
>        # Date:      Tue Beb 9 01:23:45 1911 -0500
>        #
>        # If this is not correct, please try again using the
>        # --author and --date or --reset-author options.

Okay.

>> @@ -898,6 +907,7 @@ static void handle_untracked_files_arg(struct wt_status *s)
>>               die("Invalid untracked files mode '%s'", untracked_files_arg);
>>  }
>>
>> +
>
> Stray whitespace change?

Indeed.

> How can I get out of this state if I really do want to amend?

git reset, same as it ever was?

>> @@ -943,11 +955,19 @@ static int parse_and_validate_options(int argc, const char *argv[],
>>               die("Only one of -c/-C/-F/--fixup can be used.");
>>       if (message.len && f > 0)
>>               die("Option -m cannot be combined with -c/-C/-F/--fixup.");
>> +     if (cherry_pick) {
>> +             /* Let message-specifying options override CHERRY_HEAD */
>> +             if (f > 0 || message.len)
>> +                     cherry_pick = 0;
>> +             else
>> +                     /* used for authorship side-effect only */
>> +                     use_message = "CHERRY_HEAD";
>> +     }
>
> Hmm, what if I have commits F and F' and after trying to cherry-pick F
> I decide I want the message from F'?

I don't think I understand. commit -c F', or just commit (w/o options)
and you get MERGE_MSG generated by cherry-pick.

>> @@ -1118,6 +1138,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>>       gitmodules_config();
>>       git_config(git_status_config, &s);
>>       in_merge = file_exists(git_path("MERGE_HEAD"));
>> +     cherry_pick = file_exists(git_path("CHERRY_HEAD"));
>
> Is it possible to be both in_merge and cherry_pick?

No, I need to rework this into an enum, maybe enum { CONFLICT_NONE,
CONFLICT_MERGE, CONFLICT_CHERRY_PICK } conflict_type.

>> -                     reflog_msg = "commit";
>> +                     reflog_msg = cherry_pick ? "commit (cherry-pick)"
>> +                                              : "commit";
>
> Nice.  Probably worth mentioning in the commit message.
>
>> +                     elif [ -f "$g/CHERRY_HEAD" ]; then
>> +                             r="|CHERRY-PICKING"
>
> Likewise.

Right.

Thanks for the feedback!

j.

Re: [RFC/PATCH 2/2] Teach commit to handle CHERRY_HEAD automatically

[Jonathan Nieder]

Hi,

Thanks for a quick response.  Some small clarifications.

Jay Soffian wrote:
> On Tue, Feb 15, 2011 at 6:00 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>  - "git commit --amend" to say "I'm done fixing the broken thing".
>>
>>  - "git commit --fixup=HEAD/--squash=HEAD" to say "done fixing;
>>   let's look at this again later and squash it when I am less
>>   confused".
>>
>> Both are mistakes if HEAD is the previous and good commit rather than
>> the broken one.  Maybe there is some simple safety that could protect
>> against them?
>
> As you see below, this is already protected against.

The first mistake is protected against[*], the second left alone.
(The second seems mostly harmless; just mentioning it for reference.)

[*] which I think is my favorite part of this patch

>>>  --reset-author::
>>> +     When used with -C/-c/--amend options, or when committing after a
>>> +     a conflicting cherry-pick,
>>
>> or when committing after a conflicted merge, no?
>
> No. The person committing a merge is already the author of the merge,
> why would they use --reset-author?

You're right.  I find myself occasionally doing the following

	git merge --no-commit $branch
	... update version number; walk away for a while  ...
	git commit;	# will this use the old timestamp?
	git commit --amend --reset-author

out of a vague fear, but I think I was just confused.

>>>                                  declare that the authorship of the
>>> +     resulting commit now belongs of the committer. This also renews
>>> +     the author timestamp.
>>
>> How does it interact with --author?
>
> No change from before, --author forces the author of the new commit.

Patch below.

[...]
> 		/* Let message-specifying options override CHERRY_HEAD */
>
> I'll make this logic clearer though as I need to address Junio's
> earlier message.

Ah, good to hear.

>>> +             if (cherry_pick)
>>> +                     fprintf(fp,
>>> +                             "#\n"
>>> +                             "# It looks like you may be committing a cherry-pick.\n"
>>
>> Aside: shouldn't we suggest some porcelain-ish command (git merge
>> --clear?  git commit --no-merge?) to remove .git/MERGE_HEAD instead of
>> asking the committer to do it?
>
> We have git merge --reset as an alias for reset --merge. Since reset
> --merge takes care of this case too (I think, I'll check) we can
> suggest that for both.

No, I think "git merge --abort" does too much.  If we were ready to
commit, we almost surely have precious staged changes that it would
remove.

The cure to a lingering MERGE_HEAD is still "rm -f .git/MERGE_HEAD",
I fear.  "git commit --no-merge" (meaning "ignore MERGE_HEAD") seems
tempting.

>> How can I get out of this state if I really do want to amend?
>
> git reset, same as it ever was?

Not obvious at all.  Maybe the manpage to cherry-pick could mention
that CHERRY_HEAD is cleared away by git reset?

>> Hmm, what if I have commits F and F' and after trying to cherry-pick F
>> I decide I want the message from F'?
>
> I don't think I understand. commit -c F', or just commit (w/o options)
> and you get MERGE_MSG generated by cherry-pick.

I meant the following sequence of operations:

	# by the way, does this set CHERRY_PICK_HEAD?
	git cherry-pick --no-commit F
	git commit -C F-prime

But that was a silly example --- -C takes care of authorship on its
own.  A better example might be

	git cherry-pick --no-commit F
	git commit -F the-message

or

	git cherry-pick --no-commit F
	git commit --amend -C F-prime

> Thanks for the feedback!

Thanks for making it happen. :)
Jonathan

Re: [RFC/PATCH 2/2] Teach commit to handle CHERRY_HEAD automatically

[Jay Soffian]

On Tue, Feb 15, 2011 at 6:47 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> I meant the following sequence of operations:
>
>        # by the way, does this set CHERRY_PICK_HEAD?
>        git cherry-pick --no-commit F
>        git commit -C F-prime

CHERRY_PICK_HEAD is not written if you use --no-commit.  I'm going to
punt here and say that's not my itch, thus...

> But that was a silly example --- -C takes care of authorship on its
> own.  A better example might be
>
>        git cherry-pick --no-commit F
>        git commit -F the-message
>
> or
>
>        git cherry-pick --no-commit F
>        git commit --amend -C F-prime


Because CHERRY_PICK_HEAD wasn't written, commit in this case behaves
as it did before this series.

j.

Re: [RFC/PATCH 2/2] Teach commit to handle CHERRY_HEAD automatically

[Jonathan Nieder]

Jay Soffian wrote:

> CHERRY_PICK_HEAD is not written if you use --no-commit.  I'm going to
> punt here and say that's not my itch, thus...
>
> On Tue, Feb 15, 2011 at 6:47 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> But that was a silly example --- -C takes care of authorship on its
>> own.  A better example might be
>>
>>        git cherry-pick --no-commit F
>>        git commit -F the-message
>>
>> or
>>
>>        git cherry-pick --no-commit F
>>        git commit --amend -C F-prime
>
> Because CHERRY_PICK_HEAD wasn't written, commit in this case behaves
> as it did before this series.

I'll just wait for the reroll and try it.  But surely it is clear what
I am asking?

	git cherry-pick conflicting-thing
	... fix conflict ...
	git commit -F message

should either error out or respect the user's wish, and likewise

	git cherry-pick conflicting-thing
	... fix conflict ...
	git commit --amend -C something-else

should error out.

[PATCH] Documentation: clarify interaction of --reset-author with --author

[Jonathan Nieder]

The --author and --date options override the author and date from
--reset-author.

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

> Patch below.

Sane?

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

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index b586c0f..f766d53 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -84,9 +84,12 @@ OPTIONS
 	linkgit:git-rebase[1] for details.
 
 --reset-author::
-	When used with -C/-c/--amend options, declare that the
-	authorship of the resulting commit now belongs of the committer.
+	Despite use of the -c, -C, or --amend option, declare that the
+	authorship of the new commit belongs to the committer.
 	This also renews the author timestamp.
++
+Can be combined with `--author` or `--date` to claim authorship using
+some specific name and email address or date.
 
 --short::
 	When doing a dry-run, give the output in the short-format. See
-- 
1.7.4.1

Re: [PATCH] Documentation: clarify interaction of --reset-author with --author

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The --author and --date options override the author and date from
> --reset-author.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Jonathan Nieder wrote:
>
>> Patch below.
>
> Sane?

The option is about reusing only the message and determine authorship
information from usual means, so in that sense it is sane.  A command line
"commit -C $that_one --reset-author --date=yesterday --author=$him" would
make sense in practice, but if you give both explicitly, you don't need
an explicit --reset-author anymore, so overall it makes sense in a funny
way.

>  Documentation/git-commit.txt |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index b586c0f..f766d53 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -84,9 +84,12 @@ OPTIONS
>  	linkgit:git-rebase[1] for details.
>  
>  --reset-author::
> -	When used with -C/-c/--amend options, declare that the
> -	authorship of the resulting commit now belongs of the committer.
> +	Despite use of the -c, -C, or --amend option, declare that the
> +	authorship of the new commit belongs to the committer.
>  	This also renews the author timestamp.
> ++
> +Can be combined with `--author` or `--date` to claim authorship using
> +some specific name and email address or date.
>  
>  --short::
>  	When doing a dry-run, give the output in the short-format. See
> -- 
> 1.7.4.1

Re: [RFC/PATCH 0/2] CHERRY_HEAD

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 15, 2011 at 22:23, Jay Soffian <jaysoffian@gmail.com> wrote:
> The user-experience after a cherry-pick conflicts is suboptimal.
> The user has two choices for committing the resolved state:

I've read this over, haven't run it, but I really like the idea. It
sucks that you have to save away the commit sha1 somwhere after a
failed cherry-pick to use it again. It should just behave like `git
rebase --continue`, which this implements.

It'll need some tests as a non-RFC, but otherwise it looks good.

Re: [RFC/PATCH 0/2] CHERRY_HEAD

[Junio C Hamano]

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

> I've read this over, haven't run it, but I really like the idea. It
> sucks that you have to save away the commit sha1 somwhere after a
> failed cherry-pick to use it again. It should just behave like `git
> rebase --continue`, which this implements.

I don't understand.  What do you think rebase does to be able to continue?
Doesn't it have to save the commit object name away somewhere?

Re: [RFC/PATCH 0/2] CHERRY_HEAD

[Jay Soffian]

On Tue, Feb 15, 2011 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I've read this over, haven't run it, but I really like the idea. It
>> sucks that you have to save away the commit sha1 somwhere after a
>> failed cherry-pick to use it again. It should just behave like `git
>> rebase --continue`, which this implements.
>
> I don't understand.  What do you think rebase does to be able to continue?
> Doesn't it have to save the commit object name away somewhere?

I took it to mean that the behavior after a conflict should be 'add'
followed by 'cherry-pick --continue', not 'add' followed by 'commit'.
Not that I disagree, but that's a lot more work, see my reply to Ævar
just before this.

j.

Re: [RFC/PATCH 0/2] CHERRY_HEAD

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 15, 2011 at 23:13, Jay Soffian <jaysoffian@gmail.com> wrote:
> On Tue, Feb 15, 2011 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> I've read this over, haven't run it, but I really like the idea. It
>>> sucks that you have to save away the commit sha1 somwhere after a
>>> failed cherry-pick to use it again. It should just behave like `git
>>> rebase --continue`, which this implements.
>>
>> I don't understand.  What do you think rebase does to be able to continue?
>> Doesn't it have to save the commit object name away somewhere?
>
> I took it to mean that the behavior after a conflict should be 'add'
> followed by 'cherry-pick --continue', not 'add' followed by 'commit'.
> Not that I disagree, but that's a lot more work, see my reply to Ævar
> just before this.

I just meant that when git-rebase conflicts it remembers the author
information when you do --continue without you having to do `git
commit -c sha1-that-failed` or something.

This patch adds a similar thing to cherry-pick, which I like. It's a
minor UI issue that's annoyed me in the past.

Re: [RFC/PATCH 0/2] CHERRY_HEAD

[Jay Soffian]

On Tue, Feb 15, 2011 at 4:51 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> I've read this over, haven't run it, but I really like the idea. It
> sucks that you have to save away the commit sha1 somwhere after a
> failed cherry-pick to use it again. It should just behave like `git
> rebase --continue`, which this implements.

I agree and I said as much. The problem is that cherry-pick has two
modes of behavior:

1. Given a single commit. Historically this was the only way to use
it. In this case, the behavior after a conflict should be the same as
after a merge conflict. You resolve the conflicts then use git commit.

2. Given a rev-list. This is relatively recent addition to cherry-pick
(7e2bfd3 revert: allow cherry-picking more than one commit,
2010-06-02). Here's where I'd expect to have a more rebase-like
behavior, using --continue/abort to work through the sequence. But
frankly, I consider 7e2bfd3 a mistake. I think a better implementation
would be to make cherry-pick be plumbing, and re-use rebase's logic
for walking through the series of commit.

I'd like to do (2) eventually[*] but I think in the mean time this is
a nice incremental improvement.

[*] is the sequencer project dead?

> It'll need some tests as a non-RFC, but otherwise it looks good.

Yep, I wanted to make sure I wasn't off in the weeds first. :-)

Thanks,

j.

Re: [RFC/PATCH 0/2] CHERRY_HEAD

[Miles Bader]

Jay Soffian <jaysoffian@gmail.com> writes:
> 2. Given a rev-list. This is relatively recent addition to cherry-pick
> (7e2bfd3 revert: allow cherry-picking more than one commit,
> 2010-06-02). Here's where I'd expect to have a more rebase-like
> behavior, using --continue/abort to work through the sequence. But
> frankly, I consider 7e2bfd3 a mistake. I think a better implementation
> would be to make cherry-pick be plumbing, and re-use rebase's logic
> for walking through the series of commit.

I'd also love this functionality ... 

Can basically do it with:

  # remember current branch name CHERRY_PICK_TARGET
  git checkout REV_LIST_END
  git rebase --onto CHERRY_PICK_TARGET  REV_LIST_START
  [+ git rebase --continue etc]

but man, is that confusing to remember...

I wonder if a wrapper would be robust enough.

-Miles

-- 
Cannon, n. An instrument employed in the rectification of national boundaries.

Re: [RFC/PATCH 0/2] CHERRY_HEAD

[Christian Couder]

Hi,

On Tue, Feb 15, 2011 at 11:11 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> On Tue, Feb 15, 2011 at 4:51 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> I've read this over, haven't run it, but I really like the idea. It
>> sucks that you have to save away the commit sha1 somwhere after a
>> failed cherry-pick to use it again. It should just behave like `git
>> rebase --continue`, which this implements.
>
> I agree and I said as much. The problem is that cherry-pick has two
> modes of behavior:
>
> 1. Given a single commit. Historically this was the only way to use
> it. In this case, the behavior after a conflict should be the same as
> after a merge conflict. You resolve the conflicts then use git commit.
>
> 2. Given a rev-list. This is relatively recent addition to cherry-pick
> (7e2bfd3 revert: allow cherry-picking more than one commit,
> 2010-06-02). Here's where I'd expect to have a more rebase-like
> behavior, using --continue/abort to work through the sequence. But
> frankly, I consider 7e2bfd3 a mistake. I think a better implementation
> would be to make cherry-pick be plumbing, and re-use rebase's logic
> for walking through the series of commit.

Many people wanted to be able to cherry pick many commits, so it seems
logical to make cherry-pick accept a range of commits.

About the implementation, it's better if it's in C. And it's true that
it would be better if rebase and cherry-pick could use the same logic
and even share the same code, but that's exactly what the goal of the
sequencer project has been.

I started to implement cherry-pick --continue and posted some RFC WIP
patches last november:

http://thread.gmane.org/gmane.comp.version-control.git/162183/focus=162197

the goal is to have something that in the end can be reused by rebase,
am and perhaps other commands.

I did some work on it after that, but I got stalled and had not much
time during the last months to move it further. I will see if I can
find time to work on it during the next weeks and/or publish/post the
current state.

> I'd like to do (2) eventually[*] but I think in the mean time this is
> a nice incremental improvement.
>
> [*] is the sequencer project dead?

If you are interested, perhaps you can have a look at what I posted
last november. And if you want to work on it you can ask me to publish
the current state, and eventually base some of your work on that.

Thanks,
Christian.