From: Jay Soffian Date: Thu, 17 Feb 2011 00:05:59 GMT Subject: Re: [PATCH 2/2] Teach commit about CHERRY_PICK_HEAD Message-ID: In-Reply-To: <20110216224349.GE2615@elie> On Wed, Feb 16, 2011 at 5:43 PM, Jonathan Nieder 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. >> --- 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). >> @@ -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. >> @@ -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. >> @@ -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 -- " 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. >> -     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.