Re: [PATCH 2/3] format-patch: pick up branch description when no ref is specified

From: Duy Nguyen <pclouds@gmail.com>
Date: Jan 2, 2013, 01:25 UTC
Message-ID: <CACsJy8C_rpJrfZLESB8dva4RXrVgcf_Fsnv3zkTw=rhN0JOGog@mail.gmail.com>
In-Reply-To: <7v38ykbpv3.fsf@alter.siamese.dyndns.org>

On Wed, Jan 2, 2013 at 3:38 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> find_branch_name() fails to detect "format-patch --cover-letter -3"
>> where no command line arguments are given and HEAD is automatically
>> added.
>
> Nicely spotted.
>
> That is not the only case that takes this codepath, though.
>
>     $ git format-patch --cover-letter master..
>
> will also give you the same (if you say it without "..", which is
> the more normal invocation of the command, then the caller already
> know you meant the current branch and this function is not called).
>
> And in that case you will have two tokens on cmdline.nr, one for
> "master.."  to show where he bottom is, and the other for the
> implied "HEAD";

Interesting. find_brach_name() handles this case wrong because rev->cmdline[positive].name is "HEAD" and it goes ahead prepending "refs/heads/" anyway. I'll fix it in the reroll. I was avoiding tests with an excuse that you did not write tests when you added this function either. But with this change, I think tests are required.

> I do not think this patch is a sufficient solution
> for the more general cases, but on the other hand I do not know how
> much it matters.

I think the best place to handle this is setup_revisions() for general cases. But we do not need branch detection anywhere else yet, I guess it's ok to fix it case by case here. We can move the code back to revisions.c when there are more use cases for it.

Show 10 quoted lines

>> -     if (positive < 0)
>> +     if (positive < 0) {
>> +             /*
>> +              * No actual ref from command line, but "HEAD" from
>> +              * rev->def was added in setup_revisions()
>> +              * e.g. format-patch --cover-letter -12
>> +              */
>
> That comment does not describe "positive < 0" case, but belongs to
> the conditional added in this patch, no?

It's meant as the comment for the following block, yes. I'll move it into the block for clarity.

Show 13 quoted lines

>> +             if (!rev->cmdline.nr &&
>> +                 rev->pending.nr == 1 &&
>> +                 !strcmp(rev->pending.objects[0].name, "HEAD")) {
>> +                     const char *ref;
>> +                     ref = resolve_ref_unsafe("HEAD", branch_sha1, 1, NULL);
>> +                     if (ref && !prefixcmp(ref, "refs/heads/"))
>> +                             return xstrdup(ref + strlen("refs/heads/"));
>> +             }
>>               return NULL;
>> +     }
>>       strbuf_addf(&buf, "refs/heads/%s", rev->cmdline.rev[positive].name);
>>       branch = resolve_ref_unsafe(buf.buf, branch_sha1, 1, NULL);
>>       if (!branch ||

-- 
Duy