Re: [PATCH 2/2] Teach commit about CHERRY_PICK_HEAD
- From
Jay Soffian <jaysoffian@gmail.com>
- Date
- Feb 17, 2011, 00:05 UTC
- Message-ID
- <AANLkTinUec=HJVpGL_3v=4tvkGiMiNT1QLzWsChF411Z@mail.gmail.com>
- In-Reply-To
- <20110216224349.GE2615@elie>
On Wed, Feb 16, 2011 at 5:43 PM, Jonathan Nieder <jrnieder@gmail.com> 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.
Show 13 quoted lines
>> --- 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).
Show 10 quoted lines
>> @@ -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.
Show 11 quoted lines
>> @@ -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.
Show 11 quoted lines
>> @@ -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.
Show 6 quoted lines
> Perhaps:
>
> else if (whence == CHERRY_PICK) {
> hook_arg1 = "commit";
> hook_arg2 = author_message;
> }Perhaps.
Show 6 quoted lines
> 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.
Show 5 quoted lines
>> - 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.