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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 2, 2013 at 3:38 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@xxxxxxxxx> 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.

>> -     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.

>> +             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
--
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


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]