[PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

2013-05-02 Thread Ramkumar Ramachandra
Broadly, there are two ways to resolve a command-line argument.  The
first approach is to treat it as a revision and resolve it using the
revision-parsing mechanism get_sha1().  The other approach is to treat
it as a ref, and use a reduced mechanism like branch_get().

Compare and contrast the implementations of @{N} and @{u} in
sha1_name.c to see where exactly branch_get() is useful:

In ...@{N}, the ... is passed through dwim_log() which first calls
substitute_branch_name() and then resolve_ref_unsafe() on that.  The
resolved ref could be any ref (symbolic/ non-symbolic), and not just a
refs/heads/* ref (branch).

However, in the ...@{u} case, the ... can only be a ref/heads/*
ref or a symbolic ref pointing to a refs/heads/* ref.  It uses
branch_get() to find the relevant ref.

Unfortunately, branch_get() doesn't call resolve_ref_unsafe() on what
is passed to it at all.  Instead it hard-codes the special four-letter
word HEAD (which is resolved by read_config() which is called
first), and rejects any other symbolic ref.  This is a historical
mistake because of which some callers of branch_get() suffer.  To put
it another way, the commands that accept HEAD (and resolve it into a
branch) can benefit from accepting any other symbolic ref that
resolves to a branch.  For example, the following fail:

$ git symbolic-ref M refs/heads/master
$ git show M@{u}
$ git branch -u ram/master M

This patch fixes branch_get() directly, making these work.

Notice that branch_get() calls read_config(), which in turn
preemptively calls resolve_ref_unsafe() on HEAD, when the caller
might not even be requesting the current branch.  After copying out
two critical tasks that are required for make_remote() to work
properly:

git_config(handle_config, NULL);
alias_all_urls();

we can remove read_config() and call resolve_ref_unsafe() on the
argument that is passed ourselves.  HEAD is therefore not a special
case anymore.

Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
 remote.c   | 23 ---
 t/t1508-at-combinations.sh |  2 +-
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/remote.c b/remote.c
index 68eb99b..3ac302f 100644
--- a/remote.c
+++ b/remote.c
@@ -1463,13 +1463,30 @@ void set_ref_status_for_push(struct ref *remote_refs, 
int send_mirror,
 
 struct branch *branch_get(const char *name)
 {
-   struct branch *ret;
+   struct branch *ret = NULL;
+
+   /* Setup */
+   git_config(handle_config, NULL);
+   alias_all_urls();
 
-   read_config();
if (!name || !*name || !strcmp(name, HEAD))
-   ret = current_branch;
+   name = HEAD;
else
ret = make_branch(name, 0);
+
+   if (!ret || !ret-remote_name) {
+   /* Is this a symbolic-ref like HEAD, pointing to a
+* valid branch?
+*/
+   const char *this_ref;
+   unsigned char sha1[20];
+   int flag;
+
+   this_ref = resolve_ref_unsafe(name, sha1, 0, flag);
+   if (this_ref  (flag  REF_ISSYMREF) 
+   !prefixcmp(this_ref, refs/heads/))
+   ret = make_branch(this_ref + strlen(refs/heads/), 0);
+   }
if (ret  ret-remote_name) {
ret-remote = remote_get(ret-remote_name);
if (ret-merge_nr) {
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index bb86c79..424caf5 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -63,7 +63,7 @@ nonsense @{-1}@{-1}
 git symbolic-ref H HEAD
 check H@{1} commit new-one
 check H@{now} commit new-two
-check H@{u} ref refs/heads/upstream-branch failure
+check H@{u} refs/heads/upstream-branch
 
 # To make sure that the @-parser isn't buggy, check things with the
 # symbolic-ref @
-- 
1.8.3.rc0.40.g09a0447

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

2013-05-02 Thread Ramkumar Ramachandra
Ramkumar Ramachandra wrote:
 [...]

So sorry about this, but this breaks some tests in t1507
(rev-parse-upstream).  I'm looking into this now.

In the meantime, reviewers can focus on the commit message.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

2013-05-02 Thread Ramkumar Ramachandra
Ramkumar Ramachandra wrote:
 So sorry about this, but this breaks some tests in t1507
 (rev-parse-upstream).  I'm looking into this now.

 In the meantime, reviewers can focus on the commit message.

So, it turns out that some callers expect it to read_config().

If we're still adamant about not touching branch_get(), the
alternative is to call resolve_ref_unsafe() before the branch_get()
call in interpret_branch_name().  We'll still retain the H@{u}, but
we'll lose the 'git branch -u ram/master M'.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

2013-05-02 Thread Felipe Contreras
On Thu, May 2, 2013 at 8:39 AM, Ramkumar Ramachandra artag...@gmail.com wrote:

 $ git symbolic-ref M refs/heads/master
 $ git show M@{u}
 $ git branch -u ram/master M

 This patch fixes branch_get() directly, making these work.

It's not a fix, it's implementing brand new functionality, and
changing old behaviors.

 --- a/remote.c
 +++ b/remote.c
 @@ -1463,13 +1463,30 @@ void set_ref_status_for_push(struct ref *remote_refs, 
 int send_mirror,

  struct branch *branch_get(const char *name)
  {
 -   struct branch *ret;
 +   struct branch *ret = NULL;
 +
 +   /* Setup */

I thought the preferred style for these kinds of comments was:

  /* setup */

 +   git_config(handle_config, NULL);
 +   alias_all_urls();

 -   read_config();
 if (!name || !*name || !strcmp(name, HEAD))

I still see HEAD there. Didn't you say your patch makes HEAD less
of a special ref? Why is it still here?

 -   ret = current_branch;
 +   name = HEAD;
 else
 ret = make_branch(name, 0);
 +
 +   if (!ret || !ret-remote_name) {
 +   /* Is this a symbolic-ref like HEAD, pointing to a
 +* valid branch?
 +*/

The style is:

  /*
   * This is the preferred style for multi-line
   * comments in the Linux kernel source code.
   * Please use it consistently.
   *
   * Description:  A column of asterisks on the left side,
   * with beginning and ending almost-blank lines.
   */

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html