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

On Wed, Jan 2, 2013 at 3:38 AM, Junio C Hamano [off-list ref] wrote:

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

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.

quoted

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

quoted

+             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