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