Re: [PATCH 2/2] Teach commit about CHERRY_PICK_HEAD

On Wed, Feb 16, 2011 at 5:43 PM, Jonathan Nieder [off-list ref] wrote:

Might also be worth mentioning that it makes --amend fail in such a
situation (a change worth celebrating).

Never made that particular mistake myself, but okay.

quoted

--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -56,8 +56,10 @@ static const char empty_amend_advice[] =

 static unsigned char head_sha1[20];

-static char *use_message_buffer;
+static const char *use_message_buffer;
 static const char commit_editmsg[] = "COMMIT_EDITMSG";
+static const char cherry_pick_head[] = "CHERRY_PICK_HEAD";
+static const char merge_head[] = "MERGE_HEAD";

Hmm, these variables but not MERGE_MSG, MERGE_MODE, and SQUASH_MSG?

I cleaned up the ones my patch touched. Cleaning up the rest of
commit.c was out of my purview. :-)

I'll clean them up to be consistent, but I'll do it as a separate
patch (before this one).

quoted

@@ -68,6 +70,7 @@ static enum {

 static const char *logfile, *force_author;
 static const char *template_file;
+static const char *author_message, *author_message_buffer;

That's not a message at all, is it?  On first reading I thought it
would be a message about the author.  Maybe a comment can help.

       /* name and content of commit from which to copy authorship */

The name is consistent with the other similar purpose variables:
use_message, edit_message, squash_message, fixup_message, which all
take a committish and aren't actually messages. None of those others
have comments, but it's obvious in context how they are used.

quoted

@@ -88,7 +91,8 @@ static enum {

 } cleanup_mode;
 static char *cleanup_arg;

-static int use_editor = 1, initial_commit, in_merge, include_status = 1;
+static enum commit_whence whence;
+static int use_editor = 1, initial_commit, include_status = 1;

The name "whence" is not so self-explanatory but I don't have any
better ideas (I probably would have written "merge_or_cherry_pick"; we
can be glad you came up with something better).

Respectfully disagree. Whence means "from where something came" as in
"from where did this commit we're about to make originate?" and I
intentionally didn't use _ORIGIN as "origin" has another meaning
already in git context.

quoted

@@ -163,6 +167,36 @@ static struct option builtin_commit_options[] = {

      OPT_END()
 };

+static void determine_whence(struct wt_status *s)
+{
+     if (file_exists(git_path(merge_head)))
+             whence = FROM_MERGE;

Micronit: maybe COMMITTING_A_MERGE or COMMIT_DURING_MERGE to avoid
using valuable namespace?

Respectfully disagree.

Perhaps:

       else if (whence == CHERRY_PICK) {
               hook_arg1 = "commit";
               hook_arg2 = author_message;
       }

Perhaps.

Ok.  We probably should move away from having to suggest
"rm -f .git/whatever" in the future (maybe

       git update-ref -d %s

is simpler advice?  I dunno).

Out of scope for this patch. :-)

Nice.  Opening '{' should be in the first column.

Good catch.

Isn't the advice of using "git reset -- <paths>" still good in the
CHERRY_PICK case?

I don't know. I couldn't make up my mind. If it's a conflicted path
you've edited in the working copy, then the advice should be "checkout
--merge". I think. Maybe. I don't find wt-status.c to be very much
fun, so I punted.

quoted

-     if (s->in_merge)
+     if (s->whence != FROM_COMMIT)
              ; /* NEEDSWORK: use "git reset --unresolve"??? */

Likewise here.

Checkout that NEEDSWORK. Someone should get on that. :-)

Style: please use tabs to indent.

Who ate my tabs?

Might benefit from a comment.

       /* whether a merge or cherry-pick is in progress */
       enum commit_whence whence;

Agreed.

Thanks, very readable.

Good feedback.

j.