On Wed, Feb 16, 2011 at 5:43 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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 -- <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.
>> - Â Â 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html