[PATCH 0/4] remote HEAD improvements take 2

[PATCH 0/4] remote HEAD improvements take 2

[Jay Soffian]

There is currently no porcelain for dealing with remote HEADs (i.e.
$GIT_DIR/remotes/<remote>/HEAD). This series:

1) Refactors locate_head() from builtin-clone.c to remote.c so it can be used
   by builtin-remote.c as well. I also renamed it to guess_remote_head().

   Daniel suggested having it specifically check that it returns a ref from
   refs/heads/, but I wasn't sure what impact that might have (good or
   bad...), so I punted on that change.

2) Teaches git remote show to display the remote HEAD:

  $ git remote show origin

* remote origin
  URL: git://git.kernel.org/pub/scm/git/git.git
  HEAD: master

3) Teaches git remote a new "set-head" verb:

  To set a remote HEAD explicitly:
  $ git remote set-head <name> <branch>

  To set a remote HEAD to match the upstream repo:
  $ git remote set-head <name> -a

  To delete a remote HEAD:
  $ git remote set-head <name> -d

  I changed it from "sethead" to "set-head" per Jeff.

  I also remembered to update git-completion.bash this time.

4) Documents the new set-head verb. I also correct the git remote man page
   w/respect to the "-m <master>" option. The man page implied that the remote
   HEAD was set automatically when adding a remote (a la git clone), but this
   is not true. And, since I couldn't find anywhere else that the point of
   having a remote HEAD is documented, I documented it here.

Jay Soffian (4):
  builtin-clone: move locate_head() to remote.c so it can be re-used
  builtin-remote: move duplicated cleanup code its own function
  builtin-remote: teach show to display remote HEAD
  builtin-remote: add set-head verb

 Documentation/git-remote.txt           |   20 ++++++-
 builtin-clone.c                        |   41 +-------------
 builtin-remote.c                       |   96 +++++++++++++++++++++++++++++---
 contrib/completion/git-completion.bash |    2 +-
 remote.c                               |   37 ++++++++++++
 remote.h                               |    9 +++
 6 files changed, 156 insertions(+), 49 deletions(-)

[PATCH 1/4] builtin-clone: move locate_head() to remote.c so it can be re-used

[Jay Soffian]

Move locate_head() to remote.c and rename it to guess_remote_head() to
more accurately reflect what it does. This is in preparation for being
able to call it from builtin-remote.c

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-clone.c |   41 +++--------------------------------------
 remote.c        |   37 +++++++++++++++++++++++++++++++++++++
 remote.h        |    9 +++++++++
 3 files changed, 49 insertions(+), 38 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index f73029e..280b866 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -20,6 +20,7 @@
 #include "dir.h"
 #include "pack-refs.h"
 #include "sigchain.h"
+#include "remote.h"
 
 /*
  * Overall FIXMEs:
@@ -293,43 +294,6 @@ static void remove_junk_on_signal(int signo)
 	raise(signo);
 }
 
-static const struct ref *locate_head(const struct ref *refs,
-				     const struct ref *mapped_refs,
-				     const struct ref **remote_head_p)
-{
-	const struct ref *remote_head = NULL;
-	const struct ref *remote_master = NULL;
-	const struct ref *r;
-	for (r = refs; r; r = r->next)
-		if (!strcmp(r->name, "HEAD"))
-			remote_head = r;
-
-	for (r = mapped_refs; r; r = r->next)
-		if (!strcmp(r->name, "refs/heads/master"))
-			remote_master = r;
-
-	if (remote_head_p)
-		*remote_head_p = remote_head;
-
-	/* If there's no HEAD value at all, never mind. */
-	if (!remote_head)
-		return NULL;
-
-	/* If refs/heads/master could be right, it is. */
-	if (remote_master && !hashcmp(remote_master->old_sha1,
-				      remote_head->old_sha1))
-		return remote_master;
-
-	/* Look for another ref that points there */
-	for (r = mapped_refs; r; r = r->next)
-		if (r != remote_head &&
-		    !hashcmp(r->old_sha1, remote_head->old_sha1))
-			return r;
-
-	/* Nothing is the same */
-	return NULL;
-}
-
 static struct ref *write_remote_refs(const struct ref *refs,
 		struct refspec *refspec, const char *reflog)
 {
@@ -532,7 +496,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 		mapped_refs = write_remote_refs(refs, &refspec, reflog_msg.buf);
 
-		head_points_at = locate_head(refs, mapped_refs, &remote_head);
+		head_points_at = guess_remote_head(refs, mapped_refs,
+						   &remote_head);
 	}
 	else {
 		warning("You appear to have cloned an empty repository.");
diff --git a/remote.c b/remote.c
index d7079c6..447f091 100644
--- a/remote.c
+++ b/remote.c
@@ -1376,3 +1376,40 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 			    base, num_ours, num_theirs);
 	return 1;
 }
+
+const struct ref *guess_remote_head(const struct ref *refs,
+				    const struct ref *mapped_refs,
+				    const struct ref **remote_head_p)
+{
+	const struct ref *remote_head = NULL;
+	const struct ref *remote_master = NULL;
+	const struct ref *r;
+	for (r = refs; r; r = r->next)
+		if (!strcmp(r->name, "HEAD"))
+			remote_head = r;
+
+	for (r = mapped_refs; r; r = r->next)
+		if (!strcmp(r->name, "refs/heads/master"))
+			remote_master = r;
+
+	if (remote_head_p)
+		*remote_head_p = remote_head;
+
+	/* If there's no HEAD value at all, never mind. */
+	if (!remote_head)
+		return NULL;
+
+	/* If refs/heads/master could be right, it is. */
+	if (remote_master && !hashcmp(remote_master->old_sha1,
+				      remote_head->old_sha1))
+		return remote_master;
+
+	/* Look for another ref that points there */
+	for (r = mapped_refs; r; r = r->next)
+		if (r != remote_head &&
+		    !hashcmp(r->old_sha1, remote_head->old_sha1))
+			return r;
+
+	/* Nothing is the same */
+	return NULL;
+}
diff --git a/remote.h b/remote.h
index a46a5be..cabb14a 100644
--- a/remote.h
+++ b/remote.h
@@ -137,4 +137,13 @@ enum match_refs_flags {
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs);
 int format_tracking_info(struct branch *branch, struct strbuf *sb);
 
+/*
+ * Look in refs for HEAD. Then look for a matching SHA1 in mapped_refs,
+ * first checking if refs/heads/master matches. Return NULL if nothing matches
+ * or if there is no HEAD in refs. remote_head_p is assigned HEAD if not NULL.
+ */
+const struct ref *guess_remote_head(const struct ref *refs,
+				    const struct ref *mapped_refs,
+				    const struct ref **remote_head_p);
+
 #endif
-- 
1.6.2.rc0.209.g7c178

[PATCH 2/4] builtin-remote: move duplicated cleanup code its own function

[Jay Soffian]

Moved some identical lines of code into their own function in
preparation for adding additional functionality which will use this
function as well.

Also removed a bogus NEEDSWORK comment per Daniel Barkalow:

  Actually, the comment is wrong; "remote" comes from remote_get(),
  which returns things from a cache in remote.c; there could be a
  remote_put() to let the code know that the caller is done with the
  object, but it wouldn't presently do anything.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-remote.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index ac69d37..b89a353 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -632,6 +632,13 @@ static void show_list(const char *title, struct string_list *list,
 		printf("    %s\n", list->items[i].string);
 }
 
+static void free_remote_ref_states(struct ref_states *states)
+{
+	string_list_clear(&states->new, 0);
+	string_list_clear(&states->stale, 0);
+	string_list_clear(&states->tracked, 0);
+}
+
 static int get_remote_ref_states(const char *name,
 				 struct ref_states *states,
 				 int query)
@@ -738,10 +745,7 @@ static int show(int argc, const char **argv)
 			}
 		}
 
-		/* NEEDSWORK: free remote */
-		string_list_clear(&states.new, 0);
-		string_list_clear(&states.stale, 0);
-		string_list_clear(&states.tracked, 0);
+		free_remote_ref_states(&states);
 	}
 
 	return result;
@@ -792,10 +796,7 @@ static int prune(int argc, const char **argv)
 			warn_dangling_symref(dangling_msg, refname);
 		}
 
-		/* NEEDSWORK: free remote */
-		string_list_clear(&states.new, 0);
-		string_list_clear(&states.stale, 0);
-		string_list_clear(&states.tracked, 0);
+		free_remote_ref_states(&states);
 	}
 
 	return result;
-- 
1.6.2.rc0.209.g7c178

[PATCH 3/4] builtin-remote: teach show to display remote HEAD

[Jay Soffian]

This is in preparation for teaching remote how to set
refs/remotes/<remote>/HEAD to match what HEAD is set to at <remote>, but
is useful in its own right.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-remote.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index b89a353..465c87a 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -212,6 +212,7 @@ static void read_branches(void)
 struct ref_states {
 	struct remote *remote;
 	struct string_list new, stale, tracked;
+	char *head_name;
 };
 
 static int handle_one_branch(const char *refname,
@@ -271,6 +272,26 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)
 	return 0;
 }
 
+static char *get_head_name(const struct ref *refs)
+{
+	const struct ref *head_points_at;
+	struct ref *mapped_refs = NULL;
+	struct ref **tail = &mapped_refs;
+	struct refspec refspec;
+
+	refspec.force = 0;
+	refspec.pattern = 1;
+	refspec.src = refspec.dst = "refs/heads/";
+
+	get_fetch_map(refs, &refspec, &tail, 0);
+
+	head_points_at = guess_remote_head(refs, mapped_refs, NULL);
+	if (head_points_at)
+		return xstrdup(abbrev_branch(head_points_at->name));
+
+	return NULL;
+}
+
 struct known_remote {
 	struct known_remote *next;
 	struct remote *remote;
@@ -637,6 +658,7 @@ static void free_remote_ref_states(struct ref_states *states)
 	string_list_clear(&states->new, 0);
 	string_list_clear(&states->stale, 0);
 	string_list_clear(&states->tracked, 0);
+	free(states->head_name);
 }
 
 static int get_remote_ref_states(const char *name,
@@ -658,6 +680,7 @@ static int get_remote_ref_states(const char *name,
 		ref = transport_get_remote_refs(transport);
 		transport_disconnect(transport);
 
+		states->head_name = get_head_name(ref);
 		get_ref_states(ref, states);
 	}
 
@@ -702,6 +725,9 @@ static int show(int argc, const char **argv)
 		printf("* remote %s\n  URL: %s\n", *argv,
 			states.remote->url_nr > 0 ?
 				states.remote->url[0] : "(no URL)");
+		if (!no_query)
+			printf("  HEAD: %s\n", states.head_name ?
+				states.head_name : "(unknown)");
 
 		for (i = 0; i < branch_list.nr; i++) {
 			struct string_list_item *branch = branch_list.items + i;
-- 
1.6.2.rc0.209.g7c178

[PATCH 4/4] builtin-remote: add set-head verb

[Jay Soffian]

Provide a porcelain command for setting/deleting
$GIT_DIR/remotes/<remote>/HEAD.

While we're at it, document what $GIT_DIR/remotes/<remote>/HEAD is all
about.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 Documentation/git-remote.txt           |   20 ++++++++++-
 builtin-remote.c                       |   54 +++++++++++++++++++++++++++++++-
 contrib/completion/git-completion.bash |    2 +-
 3 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index fad983e..80f2cfe 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -13,6 +13,7 @@ SYNOPSIS
 'git remote add' [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>
 'git remote rename' <old> <new>
 'git remote rm' <name>
+'git remote set-head' <name> [-a | -d | <branch>]
 'git remote show' [-n] <name>
 'git remote prune' [-n | --dry-run] <name>
 'git remote update' [group]
@@ -53,8 +54,7 @@ is created.  You can give more than one `-t <branch>` to track
 multiple branches without grabbing all branches.
 +
 With `-m <master>` option, `$GIT_DIR/remotes/<name>/HEAD` is set
-up to point at remote's `<master>` branch instead of whatever
-branch the `HEAD` at the remote repository actually points at.
+up to point at remote's `<master>` branch. See also the set-head command.
 +
 In mirror mode, enabled with `\--mirror`, the refs will not be stored
 in the 'refs/remotes/' namespace, but in 'refs/heads/'.  This option
@@ -76,6 +76,22 @@ the configuration file format.
 Remove the remote named <name>. All remote tracking branches and
 configuration settings for the remote are removed.
 
+'set-head'::
+
+Sets or deletes the default branch (`$GIT_DIR/remotes/<name>/HEAD`) for
+the named remote. Having a default branch for a remote is not required,
+but allows the name of the remote to be specified in lieu of a specific
+branch. For example, if the default branch for `origin` is set to
+`master`, then `origin` may be specified wherever you would normally
+specify `origin/master`.
++
+With `-d`, `$GIT_DIR/remotes/<name>/HEAD` is deleted.
++
+With `-a`, the remote is queried to determine its `HEAD`, then
+`$GIT_DIR/remotes/<name>/HEAD` is set to the same branch.
++
+Use `<branch>` to set `$GIT_DIR/remotes/<name>/HEAD` explicitly.
+
 'show'::
 
 Gives some information about the remote <name>.
diff --git a/builtin-remote.c b/builtin-remote.c
index 465c87a..677e20e 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -12,6 +12,7 @@ static const char * const builtin_remote_usage[] = {
 	"git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>",
 	"git remote rename <old> <new>",
 	"git remote rm <name>",
+	"git remote set-head <name> [-a | -d | <branch>]",
 	"git remote show [-n] <name>",
 	"git remote prune [-n | --dry-run] <name>",
 	"git remote [-v | --verbose] update [group]",
@@ -658,7 +659,8 @@ static void free_remote_ref_states(struct ref_states *states)
 	string_list_clear(&states->new, 0);
 	string_list_clear(&states->stale, 0);
 	string_list_clear(&states->tracked, 0);
-	free(states->head_name);
+	if (states->head_name)
+		free(states->head_name);
 }
 
 static int get_remote_ref_states(const char *name,
@@ -777,6 +779,54 @@ static int show(int argc, const char **argv)
 	return result;
 }
 
+static int sethead(int argc, const char **argv)
+{
+	int opt_a = 0, opt_d = 0, result = 0;
+	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
+	char *head_name = NULL;
+
+	struct option options[] = {
+		OPT_GROUP("set-head specific options"),
+		OPT_BOOLEAN('a', 0, &opt_a,
+		            "set refs/remotes/<name>/HEAD according to remote"),
+		OPT_BOOLEAN('d', 0, &opt_d, "delete refs/remotes/<name>/HEAD"),
+		OPT_END()
+	};
+	argc = parse_options(argc, argv, options, builtin_remote_usage, 0);
+	if ((argc == 1 && !(opt_a || opt_d)) ||
+	   ((argc == 2 && (opt_a || opt_d))) || argc < 1 || argc > 2)
+		usage_with_options(builtin_remote_usage, options);
+
+	strbuf_addf(&buf, "refs/remotes/%s/HEAD", argv[0]);
+
+	if (opt_d) {
+		if (result |= delete_ref(buf.buf, NULL, REF_NODEREF))
+			error("Could not delete %s", buf.buf);
+	} else if (opt_a) {
+		struct ref_states states;
+		memset(&states, 0, sizeof(states));
+		get_remote_ref_states(argv[0], &states, 1);
+		head_name = xstrdup(states.head_name);
+		free_remote_ref_states(&states);
+	} else
+		head_name = xstrdup(argv[1]);
+
+	if (head_name) {
+		unsigned char sha1[20];
+		strbuf_addf(&buf2, "refs/remotes/%s/%s", argv[0], head_name);
+		/* make sure it's valid */
+		if (!resolve_ref(buf2.buf, sha1, 1, NULL))
+			result |= error("Not a valid ref: %s", buf2.buf);
+		else if (create_symref(buf.buf, buf2.buf, "remote set-head"))
+			result |= error("Could not setup %s", buf.buf);
+		free(head_name);
+	}
+
+	strbuf_release(&buf);
+	strbuf_release(&buf2);
+	return result;
+}
+
 static int prune(int argc, const char **argv)
 {
 	int dry_run = 0, result = 0;
@@ -947,6 +997,8 @@ int cmd_remote(int argc, const char **argv, const char *prefix)
 		result = mv(argc, argv);
 	else if (!strcmp(argv[0], "rm"))
 		result = rm(argc, argv);
+	else if (!strcmp(argv[0], "set-head"))
+		result = sethead(argc, argv);
 	else if (!strcmp(argv[0], "show"))
 		result = show(argc, argv);
 	else if (!strcmp(argv[0], "prune"))
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f44f63c..5de1922 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1414,7 +1414,7 @@ _git_config ()
 
 _git_remote ()
 {
-	local subcommands="add rename rm show prune update"
+	local subcommands="add rename rm show prune update set-head"
 	local subcommand="$(__git_find_subcommand "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
-- 
1.6.2.rc0.209.g7c178

Re: [PATCH 0/4] remote HEAD improvements take 2

[Jay Soffian]

On Fri, Feb 13, 2009 at 3:54 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> There is currently no porcelain for dealing with remote HEADs (i.e.
> $GIT_DIR/remotes/<remote>/HEAD). This series:

Crud, sorry for the duplicates.

And I just noticed a send-email bug -- it doesn't handle multiple Cc:
recipients the way they are formated by format-patch. e.g, format
patch does this:

Cc: firstperson@example.com,
    secondperson@example.com

But send-email only picks up the first address, and further, appears
to mangle it.

j.

Re: [PATCH 4/4] builtin-remote: add set-head verb

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> Provide a porcelain command for setting/deleting
> $GIT_DIR/remotes/<remote>/HEAD.

The entire series looks sane from a very cursory look; especially the
earlier ones are obviously good.

Calling the subcommand a "verb" is somewhat new, though.  Existing
documentation for git commands that take multiple actions seem to call
them subcommands, including "git-remote.txt" itself.

> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index fad983e..80f2cfe 100644
> --- a/Documentation/git-remote.txt
> +++ b/Documentation/git-remote.txt
> @@ -76,6 +76,22 @@ the configuration file format.
>  Remove the remote named <name>. All remote tracking branches and
>  configuration settings for the remote are removed.
>  
> +'set-head'::
> +
> +Sets or deletes the default branch (`$GIT_DIR/remotes/<name>/HEAD`) for
> +the named remote. Having a default branch for a remote is not required,
> +but allows the name of the remote to be specified in lieu of a specific
> +branch. For example, if the default branch for `origin` is set to
> +`master`, then `origin` may be specified wherever you would normally
> +specify `origin/master`.
> ++
> +With `-d`, `$GIT_DIR/remotes/<name>/HEAD` is deleted.
> ++
> +With `-a`, the remote is queried to determine its `HEAD`, then
> +`$GIT_DIR/remotes/<name>/HEAD` is set to the same branch.
> ++
> +Use `<branch>` to set `$GIT_DIR/remotes/<name>/HEAD` explicitly.
> +

Hmph, what does "-a" stand for?  I would have expected to see "-u" that
stands for "update" here.

Also it may be better to be more explicit about both the syntax and the
semantics of `<branch>`.  Do you expect "refs/remotes/<name>/master" or
just "master" (I assume the latter)?  Is it an error if the branch does
not exist in the specified hierarchy?  Can you force to set to a branch
that does not exist in your tracking side (yet) but you know exists on the
remote side already?

> diff --git a/builtin-remote.c b/builtin-remote.c
> index 465c87a..677e20e 100644
> --- a/builtin-remote.c
> +++ b/builtin-remote.c
> @@ -658,7 +659,8 @@ static void free_remote_ref_states(struct ref_states *states)
>  	string_list_clear(&states->new, 0);
>  	string_list_clear(&states->stale, 0);
>  	string_list_clear(&states->tracked, 0);
> -	free(states->head_name);
> +	if (states->head_name)
> +		free(states->head_name);
>  }

Regression?

> @@ -777,6 +779,54 @@ static int show(int argc, const char **argv)
>  	return result;
>  }
>  
> +static int sethead(int argc, const char **argv)

set_head()?

> +{
> +	int opt_a = 0, opt_d = 0, result = 0;
> +	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
> +	char *head_name = NULL;
> +
> +	struct option options[] = {
> +		OPT_GROUP("set-head specific options"),
> +		OPT_BOOLEAN('a', 0, &opt_a,
> +		            "set refs/remotes/<name>/HEAD according to remote"),
> +		OPT_BOOLEAN('d', 0, &opt_d, "delete refs/remotes/<name>/HEAD"),
> +		OPT_END()
> +	};
> +	argc = parse_options(argc, argv, options, builtin_remote_usage, 0);
> +	if ((argc == 1 && !(opt_a || opt_d)) ||
> +	   ((argc == 2 && (opt_a || opt_d))) || argc < 1 || argc > 2)
> +		usage_with_options(builtin_remote_usage, options);

The code will scale better, especially for a young subcommand that may acquire
new options, if the check is done by each codepath that deals with a
specific option to do this kind of check.  That is, e.g.

	if (opt_delete) {
        	error if the arg is not remote (alone)
                do the "delete" thing
	} else if (opt_update) {
        	error if the arg is not remote (alone)
                do the "update" thing
	} else {
        	error if the args are not (remote, branch)
		do the "set" thing
	}

Re: [PATCH 4/4] builtin-remote: add set-head verb

[Jay Soffian]

On Fri, Feb 13, 2009 at 5:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Calling the subcommand a "verb" is somewhat new, though.  Existing
> documentation for git commands that take multiple actions seem to call
> them subcommands, including "git-remote.txt" itself.

Okay.

> Hmph, what does "-a" stand for?  I would have expected to see "-u" that
> stands for "update" here.

--automatic -- as in figure out the name automatically from the other side.

> Also it may be better to be more explicit about both the syntax and the
> semantics of `<branch>`.

Okay.

> Do you expect "refs/remotes/<name>/master" or
> just "master" (I assume the latter)?

Yes, the latter.  If you did the wrong thing the error ought clue you in:

$ ./git remote set-head origin refs/remotes/origin/master
error: Not a valid ref: refs/remotes/origin/refs/remotes/origin/master

> Is it an error if the branch does
> not exist in the specified hierarchy?

Yes it is an error per-above. Well, at least on-top of next it is.

> Can you force to set to a branch
> that does not exist in your tracking side (yet) but you know exists on the
> remote side already?

No.

>> diff --git a/builtin-remote.c b/builtin-remote.c
>> index 465c87a..677e20e 100644
>> --- a/builtin-remote.c
>> +++ b/builtin-remote.c
>> @@ -658,7 +659,8 @@ static void free_remote_ref_states(struct ref_states *states)
>>       string_list_clear(&states->new, 0);
>>       string_list_clear(&states->stale, 0);
>>       string_list_clear(&states->tracked, 0);
>> -     free(states->head_name);
>> +     if (states->head_name)
>> +             free(states->head_name);
>>  }
>
> Regression?

Indeed.

> set_head()?

Yep.

> The code will scale better, especially for a young subcommand that may acquire
> new options, if the check is done by each codepath that deals with a
> specific option to do this kind of check.  That is, e.g.
>
>        if (opt_delete) {
>                error if the arg is not remote (alone)
>                do the "delete" thing
>        } else if (opt_update) {
>                error if the arg is not remote (alone)
>                do the "update" thing
>        } else {
>                error if the args are not (remote, branch)
>                do the "set" thing
>        }

Got it. I really am trying to match existing code, but it seems the
standards have gotten higher, so I need to do better than existing
code.

Thanks,

j.

Re: [PATCH 4/4] builtin-remote: add set-head verb

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Also it may be better to be more explicit about both the syntax and the
> semantics of `<branch>`.  Do you expect "refs/remotes/<name>/master" or
> just "master" (I assume the latter)?  Is it an error if the branch does
> not exist in the specified hierarchy?  Can you force to set to a branch
> that does not exist in your tracking side (yet) but you know exists on the
> remote side already?

A few things I forgot (and before I go to bed).

If remotes/<name>/HEAD already points at a branch frotz, and you peek the
remote (i.e. you do not actually run "fetch" to download objects, but just
"ls-remote" it) and find out that "HEAD" does not point at the same commit
as "frotz" but it now points at the same commit as another branch
"nitfol", you probably would want to update it to point at "nitfol", but
it was unclear from the description in the documentation if this option
was meant to perform this kind fo update, or only to set a missing HEAD.

If you meant to do an update, there is one thing to watch out for when you
reuse the logic used by clone.  It favors "master" if more than one
branches point at the same commit as HEAD.  You need to customize it to
favor the one that the HEAD points at on the local side before you start
(iow, try to keep the current value when you can).

For example, if it points at "frotz" locally when the command was started,
and you found out that HEAD now points at the commit at the tip of "frotz"
and "master" branches by peeking, you do not want to repoint HEAD from
"frotz" to "master".

Re: [PATCH 4/4] builtin-remote: add set-head verb

[Jay Soffian]

On Fri, Feb 13, 2009 at 5:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> A few things I forgot (and before I go to bed).
>
> If remotes/<name>/HEAD already points at a branch frotz, and you peek the
> remote (i.e. you do not actually run "fetch" to download objects, but just
> "ls-remote" it) and find out that "HEAD" does not point at the same commit
> as "frotz" but it now points at the same commit as another branch
> "nitfol", you probably would want to update it to point at "nitfol", but
> it was unclear from the description in the documentation if this option
> was meant to perform this kind fo update, or only to set a missing HEAD.

- set-head -d <remote> deletes
- set-head -a <remote> sets HEAD to whatever remote it
- set-head <remote> <foo> sets HEAD to foo

I don't know how to be more explicit than that I think that's what the
documentation indicates.

> If you meant to do an update, there is one thing to watch out for when you
> reuse the logic used by clone.  It favors "master" if more than one
> branches point at the same commit as HEAD.

Yes, I know.

> You need to customize it to
> favor the one that the HEAD points at on the local side before you start
> (iow, try to keep the current value when you can).
>
> For example, if it points at "frotz" locally when the command was started,
> and you found out that HEAD now points at the commit at the tip of "frotz"
> and "master" branches by peeking, you do not want to repoint HEAD from
> "frotz" to "master".

I think that is an unnecessary complication for what is really a
corner case. And anyway, it is just hacking around the fact that we
don't really know what the remote side is if multiple branch heads
have the same SHA1 as HEAD.

You proposed a series a while back so that git could unambiguously
determine what a remote symref points to, and I think that's the
better way to fix this problem.

If the user runs "git remote set-head -a <remote>" they will get
whatever "git show <remote>" indicates is the remote HEAD. If they
don't like that, they can set their <remote>/HEAD explicitly using the
alternate syntax.

j.

[PATCH v2 4/4] builtin-remote: add set-head subcommand

[Jay Soffian]

Provide a porcelain command for setting/deleting
$GIT_DIR/remotes/<remote>/HEAD.

While we're at it, document what $GIT_DIR/remotes/<remote>/HEAD is all
about.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
This replaces "[PATCH 4/4] builtin-remote: add set-head verb". The rest of
the series is unchanged.

 * Changed commit title to call it a subcommand, not a verb.
 * Clarified documentation slightly
 * Added --auto and --delete long options to make what -a and -d stand for
   more obvious.
 * Fixed small regression where I was checking a pointer for NULL before
   free()'ing it.
 * sethead() -> set_head()
 * Reworked the argument processing in set_head() to be more maintainable if
   additional switches are added later.

On Fri, Feb 13, 2009 at 5:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> You need to customize it to
> favor the one that the HEAD points at on the local side before you start
> (iow, try to keep the current value when you can).
>
> For example, if it points at "frotz" locally when the command was started,
> and you found out that HEAD now points at the commit at the tip of "frotz"
> and "master" branches by peeking, you do not want to repoint HEAD from
> "frotz" to "master".

This I did not do, and I still don't feel it is necessary. However, if you
feel strongly about it I can do it as an additional patch to this series. But
I'd like to see this in pu at least first. I really hope not doing it isn't a
roadblock for the whole series.

 Documentation/git-remote.txt           |   28 ++++++++++++++++-
 builtin-remote.c                       |   51 ++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |    2 +-
 3 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index fad983e..c9c0e6f 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -13,6 +13,7 @@ SYNOPSIS
 'git remote add' [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>
 'git remote rename' <old> <new>
 'git remote rm' <name>
+'git remote set-head' <name> [-a | -d | <branch>]
 'git remote show' [-n] <name>
 'git remote prune' [-n | --dry-run] <name>
 'git remote update' [group]
@@ -53,8 +54,7 @@ is created.  You can give more than one `-t <branch>` to track
 multiple branches without grabbing all branches.
 +
 With `-m <master>` option, `$GIT_DIR/remotes/<name>/HEAD` is set
-up to point at remote's `<master>` branch instead of whatever
-branch the `HEAD` at the remote repository actually points at.
+up to point at remote's `<master>` branch. See also the set-head command.
 +
 In mirror mode, enabled with `\--mirror`, the refs will not be stored
 in the 'refs/remotes/' namespace, but in 'refs/heads/'.  This option
@@ -76,6 +76,30 @@ the configuration file format.
 Remove the remote named <name>. All remote tracking branches and
 configuration settings for the remote are removed.
 
+'set-head'::
+
+Sets or deletes the default branch (`$GIT_DIR/remotes/<name>/HEAD`) for
+the named remote. Having a default branch for a remote is not required,
+but allows the name of the remote to be specified in lieu of a specific
+branch. For example, if the default branch for `origin` is set to
+`master`, then `origin` may be specified wherever you would normally
+specify `origin/master`.
++
+With `-d`, `$GIT_DIR/remotes/<name>/HEAD` is deleted.
++
+With `-a`, the remote is queried to determine its `HEAD`, then
+`$GIT_DIR/remotes/<name>/HEAD` is set to the same branch. e.g., if the remote
+`HEAD` is pointed at `next`, "`git remote set-head origin -a`" will set
+`$GIT_DIR/refs/remotes/origin/HEAD` to `refs/remotes/origin/next`. This will
+only work if `refs/remotes/origin/next` already exists; if not it must be
+fetched first.
++
+Use `<branch>` to set `$GIT_DIR/remotes/<name>/HEAD` explicitly. e.g., "git
+remote set-head origin master" will set `$GIT_DIR/refs/remotes/origin/HEAD` to
+`refs/remotes/origin/master`. This will only work if
+`refs/remotes/origin/master` already exists; if not it must be fetched first.
++
+
 'show'::
 
 Gives some information about the remote <name>.
diff --git a/builtin-remote.c b/builtin-remote.c
index 465c87a..fcb166b 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -12,6 +12,7 @@ static const char * const builtin_remote_usage[] = {
 	"git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>",
 	"git remote rename <old> <new>",
 	"git remote rm <name>",
+	"git remote set-head <name> [-a | -d | <branch>]",
 	"git remote show [-n] <name>",
 	"git remote prune [-n | --dry-run] <name>",
 	"git remote [-v | --verbose] update [group]",
@@ -777,6 +778,54 @@ static int show(int argc, const char **argv)
 	return result;
 }
 
+static int set_head(int argc, const char **argv)
+{
+	int opt_a = 0, opt_d = 0, result = 0;
+	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
+	char *head_name = NULL;
+
+	struct option options[] = {
+		OPT_GROUP("set-head specific options"),
+		OPT_BOOLEAN('a', "auto", &opt_a,
+			    "set refs/remotes/<name>/HEAD according to remote"),
+		OPT_BOOLEAN('d', "delete", &opt_d,
+			    "delete refs/remotes/<name>/HEAD"),
+		OPT_END()
+	};
+	argc = parse_options(argc, argv, options, builtin_remote_usage, 0);
+	if (argc)
+		strbuf_addf(&buf, "refs/remotes/%s/HEAD", argv[0]);
+
+	if (!opt_a && !opt_d && argc == 2) {
+		head_name = xstrdup(argv[1]);
+	} else if (opt_a && !opt_d && argc == 1) {
+		struct ref_states states;
+		memset(&states, 0, sizeof(states));
+		get_remote_ref_states(argv[0], &states, 1);
+		head_name = xstrdup(states.head_name);
+		free_remote_ref_states(&states);
+	} else if (opt_d && !opt_a && argc == 1) {
+		if (delete_ref(buf.buf, NULL, REF_NODEREF))
+			result |= error("Could not delete %s", buf.buf);
+	} else
+		usage_with_options(builtin_remote_usage, options);
+
+	if (head_name) {
+		unsigned char sha1[20];
+		strbuf_addf(&buf2, "refs/remotes/%s/%s", argv[0], head_name);
+		/* make sure it's valid */
+		if (!resolve_ref(buf2.buf, sha1, 1, NULL))
+			result |= error("Not a valid ref: %s", buf2.buf);
+		else if (create_symref(buf.buf, buf2.buf, "remote set-head"))
+			result |= error("Could not setup %s", buf.buf);
+		free(head_name);
+	}
+
+	strbuf_release(&buf);
+	strbuf_release(&buf2);
+	return result;
+}
+
 static int prune(int argc, const char **argv)
 {
 	int dry_run = 0, result = 0;
@@ -947,6 +996,8 @@ int cmd_remote(int argc, const char **argv, const char *prefix)
 		result = mv(argc, argv);
 	else if (!strcmp(argv[0], "rm"))
 		result = rm(argc, argv);
+	else if (!strcmp(argv[0], "set-head"))
+		result = set_head(argc, argv);
 	else if (!strcmp(argv[0], "show"))
 		result = show(argc, argv);
 	else if (!strcmp(argv[0], "prune"))
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f44f63c..5de1922 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1414,7 +1414,7 @@ _git_config ()
 
 _git_remote ()
 {
-	local subcommands="add rename rm show prune update"
+	local subcommands="add rename rm show prune update set-head"
 	local subcommand="$(__git_find_subcommand "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
-- 
1.6.2.rc0.67.g77afc

Re: [PATCH 4/4] builtin-remote: add set-head verb

[Jeff King]

On Fri, Feb 13, 2009 at 02:09:01AM -0800, Junio C Hamano wrote:

> The entire series looks sane from a very cursory look; especially the
> earlier ones are obviously good.

I also think it looks good. You raised a few style points below which I
thought were sensible and won't bother repeating.

> Hmph, what does "-a" stand for?  I would have expected to see "-u" that
> stands for "update" here.

It was immediately obvious to me as "auto" (I think I even suggested
"-a" in another thread, so maybe that is why it seems so sensible to
me). However, I think as a rule it is nice to always provide a "long"
alternative for every option. With parse-options, there really is no
downside; it is literally s/0/"auto"/ on the option line. And I think
it:

  - lets people who want to illustrate a command in a more readable
    manner do so (e.g., if they are showing it to somebody else)

  - makes it more obvious in the documentation and usage message what
    the command does. That is, I remember commands much better as "this
    is the --auto option, whose meaningful name reminds me that it does
    X, and -a is the obvious shorthand" rather than "-a does X".

  - enables extra parse-options syntax like automatic "--no-" handling
    (though I doubt anyone is likely to use "--no-auto" in this case,
    the point is that it is easier to allow such constructs consistently
    than to try to guess when people might want it)

Which is maybe more argument than you cared to read about this
particular option, but I want to make clear that I think this should be
the case for just about every command-line option we add.

> Also it may be better to be more explicit about both the syntax and the
> semantics of `<branch>`.  Do you expect "refs/remotes/<name>/master" or
> just "master" (I assume the latter)?  Is it an error if the branch does

I thought it was obvious that you would do:

  git remote set-head master

in the same way that you would do:

  git remote add -m master $remote $url

But I suppose clarifying it doesn't hurt.

-Peff

PS There is a thread elsewhere on the list discussing "what can I do to
make life easy for reviewers?". I think this series (and Jay's patches
in general) model many good behaviors: clearly labeled versions,
discussion of what changed in each version, proper threading, and cc'ing
people who have been involved.

Re: [PATCH 4/4] builtin-remote: add set-head verb

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Feb 13, 2009 at 02:09:01AM -0800, Junio C Hamano wrote:
>
>> The entire series looks sane from a very cursory look; especially the
>> earlier ones are obviously good.
>
> I also think it looks good. You raised a few style points below which I
> thought were sensible and won't bother repeating.
>
>> Hmph, what does "-a" stand for?  I would have expected to see "-u" that
>> stands for "update" here.
>
> It was immediately obvious to me as "auto" (I think I even suggested
> "-a" in another thread, so maybe that is why it seems so sensible to
> me).

Yeah, latest round has --auto in it.  Thanks, Jay.

> I thought it was obvious that you would do:
>
>   git remote set-head master
>
> in the same way that you would do:
>
>   git remote add -m master $remote $url
>
> But I suppose clarifying it doesn't hurt.

I do not care too deeply if an explicit request to "set-head --auto"
screws up and sets a HEAD that was pointing at the right branch to another
branch because the command is not taught to give preference to the branch
HEAD originally points at, so I do not think I have any more issues with
the series for now, even though I may notice things later.

I have this series queued to private topic branch but it still does not
pass tests (breaks #8 and #18 of t5505 at least) by itself; the previous
round was no better.  I think it is just the matter of updating the
expected output in the tests, but I didn't look further.

Re: [PATCH 4/4] builtin-remote: add set-head verb

[Jeff King]

On Fri, Feb 13, 2009 at 06:00:29PM -0800, Junio C Hamano wrote:

> > It was immediately obvious to me as "auto" (I think I even suggested
> > "-a" in another thread, so maybe that is why it seems so sensible to
> > me).
> 
> Yeah, latest round has --auto in it.  Thanks, Jay.

Oops. I even went back and double-checked to make sure that it was not
there, but it would have helped if I actually checked the latest
version. :(

> I do not care too deeply if an explicit request to "set-head --auto"
> screws up and sets a HEAD that was pointing at the right branch to another
> branch because the command is not taught to give preference to the branch
> HEAD originally points at, so I do not think I have any more issues with
> the series for now, even though I may notice things later.

I think that is reasonable; that is a separate enhancement which can
come later and is no reason to block the existing patches.

> I have this series queued to private topic branch but it still does not
> pass tests (breaks #8 and #18 of t5505 at least) by itself; the previous
> round was no better.  I think it is just the matter of updating the
> expected output in the tests, but I didn't look further.

Test #8 is just a matter of updating output. But #18 is explicitly about
checking that "remote show" does not show symbolic refs. But Jay's patch
is about explicitly showing symbolic refs (just doing so as a ref-name
instead of a sha1):

  * FAIL 18: "remote show" does not show symbolic refs
                git clone one three &&
                (cd three &&
                 git remote show origin > output &&
                 ! grep HEAD < output &&
                 ! grep -i stale < output)

I guess we could tighten the grep to

  ! egrep "HEAD: [0-9a-f]{40}" < output

but it may just make sense to get rid of the test; the exact output is
already covered by test #8. Squashable patch is below.

---
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index eb63718..8808580 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -136,6 +136,7 @@ EOF
 cat > test/expect << EOF
 * remote origin
   URL: $(pwd)/one
+  HEAD: master
   Remote branch merged with 'git pull' while on branch master
     master
   New remote branch (next fetch will store in remotes/origin)
@@ -338,16 +339,6 @@ test_expect_success 'update default (overridden, with funny whitespace)' '
 
 '
 
-test_expect_success '"remote show" does not show symbolic refs' '
-
-	git clone one three &&
-	(cd three &&
-	 git remote show origin > output &&
-	 ! grep HEAD < output &&
-	 ! grep -i stale < output)
-
-'
-
 test_expect_success 'reject adding remote with an invalid name' '
 
 	test_must_fail git remote add some:url desired-name

Re: [PATCH 4/4] builtin-remote: add set-head verb

[Jay Soffian]

On Fri, Feb 13, 2009 at 9:18 PM, Jeff King <peff@peff.net> wrote:
> Test #8 is just a matter of updating output. But #18 is explicitly about
> checking that "remote show" does not show symbolic refs. But Jay's patch
> is about explicitly showing symbolic refs (just doing so as a ref-name
> instead of a sha1):
>
>  * FAIL 18: "remote show" does not show symbolic refs
>                git clone one three &&
>                (cd three &&
>                 git remote show origin > output &&
>                 ! grep HEAD < output &&
>                 ! grep -i stale < output)
>
> I guess we could tighten the grep to
>
>  ! egrep "HEAD: [0-9a-f]{40}" < output
>
> but it may just make sense to get rid of the test; the exact output is
> already covered by test #8. Squashable patch is below.

Thank you Jeff. I swear I ran t5505-remote.sh successfully but
obviously I am misremembering.

j.

Re: [PATCH 4/4] builtin-remote: add set-head verb

[Jay Soffian]

On Fri, Feb 13, 2009 at 9:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I do not care too deeply if an explicit request to "set-head --auto"
> screws up and sets a HEAD that was pointing at the right branch to another
> branch because the command is not taught to give preference to the branch
> HEAD originally points at

So I don't mind fixing this, but here's the thing.

Say user has refs/remotes/origin/HEAD set to frotz. They then run "git
show remote origin" and we see that HEAD on the remote end could be
either master or frotz (both have the same SHA1).

What should we show in the output of "git remote show origin" next to
the HEAD line? master, or frotz? If we show master, then user might
wonder why "git remote set-head origin --auto" leaves
refs/remotes/origin/HEAD set to frotz. If we show frotz, then user
might wonder why when they cloned the repo in the first place they
ended up with HEAD set to master.

I'm bothered by that inconsistency, which is why I didn't follow-up
with another patch immediately.

But I will propose an alternative. In the output of "get remote show
origin", we show all matching branches. If the user does a set-head
--auto and we cannot determine HEAD unambiguously, we do something
like:

$ git remote set-head origin --auto
error: Multiple branches match HEAD. Please choose one explicitly with:
   git remote set-head origin master
   git remote set-head origin frotz

Hmm?

j.

Re: [PATCH 4/4] builtin-remote: add set-head verb

[Jeff King]

On Fri, Feb 13, 2009 at 09:59:06PM -0500, Jay Soffian wrote:

> What should we show in the output of "git remote show origin" next to
> the HEAD line? master, or frotz? If we show master, then user might
> wonder why "git remote set-head origin --auto" leaves
> refs/remotes/origin/HEAD set to frotz. If we show frotz, then user
> might wonder why when they cloned the repo in the first place they
> ended up with HEAD set to master.
> 
> I'm bothered by that inconsistency, which is why I didn't follow-up
> with another patch immediately.

Hrm. Yeah, I think to avoid surprising the user, "--auto" has to use
whatever we showed in "git remote show".

> But I will propose an alternative. In the output of "get remote show
> origin", we show all matching branches. If the user does a set-head
> --auto and we cannot determine HEAD unambiguously, we do something
> like:
> 
> $ git remote set-head origin --auto
> error: Multiple branches match HEAD. Please choose one explicitly with:
>    git remote set-head origin master
>    git remote set-head origin frotz

I like that proposal. It doesn't hide from the user that we are doing a
matching guess, which means we are less likely to surprise them in the
long run.

-Peff

[PATCH] builtin-remote: better handling of multiple remote HEADs

[Jay Soffian]

It is not currently possible to determine the remote HEAD unambiguously
when multiple remote branches share the same SHA1 as the remote HEAD.

In this situation, git remote set-head --auto should not try to guess
which HEAD the user wants. This patch causes set-head to provide a
useful error instead:

$ git remote set-head origin --auto
error: Multiple remote HEAD branches. Please choose one explicitly with:
  git remote set-head origin another
  git remote set-head origin master

Also, the output of git remote show now shows the multiple HEADs:

$ git remote show origin
* remote origin
  URL: ...
  HEAD branches:
    another
    master

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
On Fri, Feb 13, 2009 at 10:43 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Feb 13, 2009 at 09:59:06PM -0500, Jay Soffian wrote:
>
>> But I will propose an alternative. In the output of "get remote show
>> origin", we show all matching branches. If the user does a set-head
>> --auto and we cannot determine HEAD unambiguously, we do something
>> like:
>>
>> $ git remote set-head origin --auto
>> error: Multiple branches match HEAD. Please choose one explicitly with:
>>    git remote set-head origin master
>>    git remote set-head origin frotz
>
> I like that proposal. It doesn't hide from the user that we are doing a
> matching guess, which means we are less likely to surprise them in the
> long run.
>
> -Peff

Voilà

Junio - this obviously goes on-top of the rest of the builtin-remote
series I sent.

 builtin-clone.c   |    2 +-
 builtin-remote.c  |   56 +++++++++++++++++++++++++++++++++++------------------
 remote.c          |   28 ++++++++++++++++++-------
 remote.h          |    6 ++++-
 t/t5505-remote.sh |   53 ++++++++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 114 insertions(+), 31 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index d179d1c..d57818c 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -510,7 +510,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		mapped_refs = write_remote_refs(refs, &refspec, reflog_msg.buf);
 
 		head_points_at = guess_remote_head(refs, mapped_refs,
-						   &remote_head);
+						   &remote_head, NULL);
 	}
 	else {
 		warning("You appear to have cloned an empty repository.");
diff --git a/builtin-remote.c b/builtin-remote.c
index fcb166b..608c0f3 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -212,8 +212,7 @@ static void read_branches(void)
 
 struct ref_states {
 	struct remote *remote;
-	struct string_list new, stale, tracked;
-	char *head_name;
+	struct string_list new, stale, tracked, heads;
 };
 
 static int handle_one_branch(const char *refname,
@@ -273,24 +272,26 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)
 	return 0;
 }
 
-static char *get_head_name(const struct ref *refs)
+static int get_head_names(const struct ref *refs,
+	const char *remote_name, struct ref_states *states)
 {
-	const struct ref *head_points_at;
-	struct ref *mapped_refs = NULL;
-	struct ref **tail = &mapped_refs;
+	struct ref *ref, *matches = NULL;
+	struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map;
 	struct refspec refspec;
 
 	refspec.force = 0;
 	refspec.pattern = 1;
 	refspec.src = refspec.dst = "refs/heads/";
+	states->heads.strdup_strings = 1;
+	get_fetch_map(refs, &refspec, &fetch_map_tail, 0);
+	guess_remote_head(refs, fetch_map, NULL, &matches);
+	for(ref = matches; ref; ref = ref->next)
+		string_list_append(abbrev_branch(ref->name), &states->heads);
 
-	get_fetch_map(refs, &refspec, &tail, 0);
-
-	head_points_at = guess_remote_head(refs, mapped_refs, NULL);
-	if (head_points_at)
-		return xstrdup(abbrev_branch(head_points_at->name));
+	free_refs(fetch_map);
+	free_refs(matches);
 
-	return NULL;
+	return 0;
 }
 
 struct known_remote {
@@ -659,7 +660,7 @@ static void free_remote_ref_states(struct ref_states *states)
 	string_list_clear(&states->new, 0);
 	string_list_clear(&states->stale, 0);
 	string_list_clear(&states->tracked, 0);
-	free(states->head_name);
+	string_list_clear(&states->heads, 0);
 }
 
 static int get_remote_ref_states(const char *name,
@@ -681,7 +682,7 @@ static int get_remote_ref_states(const char *name,
 		ref = transport_get_remote_refs(transport);
 		transport_disconnect(transport);
 
-		states->head_name = get_head_name(ref);
+		get_head_names(ref, name, states);
 		get_ref_states(ref, states);
 	}
 
@@ -726,9 +727,15 @@ static int show(int argc, const char **argv)
 		printf("* remote %s\n  URL: %s\n", *argv,
 			states.remote->url_nr > 0 ?
 				states.remote->url[0] : "(no URL)");
-		if (!no_query)
-			printf("  HEAD: %s\n", states.head_name ?
-				states.head_name : "(unknown)");
+		if (no_query)
+			printf("  HEAD branch: (not queried)\n");
+		else if (!states.heads.nr)
+			printf("  HEAD branch: (unknown)\n");
+		else if (states.heads.nr == 1)
+			printf("  HEAD branch: %s\n",
+				states.heads.items[0].string);
+		else
+			show_list("  HEAD branch%s:", &states.heads, "");
 
 		for (i = 0; i < branch_list.nr; i++) {
 			struct string_list_item *branch = branch_list.items + i;
@@ -780,7 +787,7 @@ static int show(int argc, const char **argv)
 
 static int set_head(int argc, const char **argv)
 {
-	int opt_a = 0, opt_d = 0, result = 0;
+	int i, opt_a = 0, opt_d = 0, result = 0;
 	struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;
 	char *head_name = NULL;
 
@@ -802,7 +809,16 @@ static int set_head(int argc, const char **argv)
 		struct ref_states states;
 		memset(&states, 0, sizeof(states));
 		get_remote_ref_states(argv[0], &states, 1);
-		head_name = xstrdup(states.head_name);
+		if (!states.heads.nr)
+			result |= error("Cannot determine remote HEAD");
+		else if (states.heads.nr > 1) {
+			result |= error("Multiple remote HEAD branches. "
+					"Please choose one explicitly with:");
+			for (i = 0; i < states.heads.nr; i++)
+				fprintf(stderr, "  git remote set-head %s %s\n",
+					argv[0], states.heads.items[i].string);
+		} else
+			head_name = xstrdup(states.heads.items[0].string);
 		free_remote_ref_states(&states);
 	} else if (opt_d && !opt_a && argc == 1) {
 		if (delete_ref(buf.buf, NULL, REF_NODEREF))
@@ -818,6 +834,8 @@ static int set_head(int argc, const char **argv)
 			result |= error("Not a valid ref: %s", buf2.buf);
 		else if (create_symref(buf.buf, buf2.buf, "remote set-head"))
 			result |= error("Could not setup %s", buf.buf);
+		if (opt_a)
+			printf("%s/HEAD set to %s\n", argv[0], head_name);
 		free(head_name);
 	}
 
diff --git a/remote.c b/remote.c
index 447f091..6385a22 100644
--- a/remote.c
+++ b/remote.c
@@ -1379,18 +1379,23 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 
 const struct ref *guess_remote_head(const struct ref *refs,
 				    const struct ref *mapped_refs,
-				    const struct ref **remote_head_p)
+				    const struct ref **remote_head_p,
+				    struct ref **all_matches_p)
 {
 	const struct ref *remote_head = NULL;
 	const struct ref *remote_master = NULL;
+	const struct ref *ret = NULL;
 	const struct ref *r;
+	struct ref **tail = all_matches_p;
+
 	for (r = refs; r; r = r->next)
 		if (!strcmp(r->name, "HEAD"))
 			remote_head = r;
 
-	for (r = mapped_refs; r; r = r->next)
-		if (!strcmp(r->name, "refs/heads/master"))
-			remote_master = r;
+	if (!all_matches_p)
+		for (r = mapped_refs; r; r = r->next)
+			if (!strcmp(r->name, "refs/heads/master"))
+				remote_master = r;
 
 	if (remote_head_p)
 		*remote_head_p = remote_head;
@@ -1407,9 +1412,16 @@ const struct ref *guess_remote_head(const struct ref *refs,
 	/* Look for another ref that points there */
 	for (r = mapped_refs; r; r = r->next)
 		if (r != remote_head &&
-		    !hashcmp(r->old_sha1, remote_head->old_sha1))
-			return r;
+		    !hashcmp(r->old_sha1, remote_head->old_sha1)) {
+			struct ref *cpy;
+			if (!ret)
+				ret = r;
+			if (!all_matches_p)
+				break;
+			*tail = cpy = copy_ref(r);
+			cpy->peer_ref = NULL;
+			tail = &cpy->next;
+		}
 
-	/* Nothing is the same */
-	return NULL;
+	return ret;
 }
diff --git a/remote.h b/remote.h
index cabb14a..8409d42 100644
--- a/remote.h
+++ b/remote.h
@@ -141,9 +141,13 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb);
  * Look in refs for HEAD. Then look for a matching SHA1 in mapped_refs,
  * first checking if refs/heads/master matches. Return NULL if nothing matches
  * or if there is no HEAD in refs. remote_head_p is assigned HEAD if not NULL.
+ * If all_matches_p is NULL, return after the first possible match. Otherwise
+ * all_matches_p is set to a ref list of each branch head with the same SHA1 as
+ * HEAD.
  */
 const struct ref *guess_remote_head(const struct ref *refs,
 				    const struct ref *mapped_refs,
-				    const struct ref **remote_head_p);
+				    const struct ref **remote_head_p,
+				    struct ref **all_matches_p);
 
 #endif
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 8808580..49f99e9 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -136,7 +136,7 @@ EOF
 cat > test/expect << EOF
 * remote origin
   URL: $(pwd)/one
-  HEAD: master
+  HEAD branch: master
   Remote branch merged with 'git pull' while on branch master
     master
   New remote branch (next fetch will store in remotes/origin)
@@ -147,6 +147,11 @@ cat > test/expect << EOF
   Local branches pushed with 'git push'
     master:upstream
     +refs/tags/lastbackup
+* remote two
+  URL: ../two
+  HEAD branches:
+    another
+    master
 EOF
 
 test_expect_success 'show' '
@@ -155,6 +160,7 @@ test_expect_success 'show' '
 		refs/heads/master:refs/heads/upstream &&
 	 git fetch &&
 	 git branch -d -r origin/master &&
+	 git config --add remote.two.url ../two &&
 	 (cd ../one &&
 	  echo 1 > file &&
 	  test_tick &&
@@ -163,13 +169,14 @@ test_expect_success 'show' '
 		refs/heads/master:refs/heads/upstream &&
 	 git config --add remote.origin.push \
 		+refs/tags/lastbackup &&
-	 git remote show origin > output &&
+	 git remote show origin two > output &&
 	 test_cmp expect output)
 '
 
 cat > test/expect << EOF
 * remote origin
   URL: $(pwd)/one
+  HEAD branch: (not queried)
   Remote branch merged with 'git pull' while on branch master
     master
   Tracked remote branches
@@ -198,6 +205,48 @@ test_expect_success 'prune' '
 	 test_must_fail git rev-parse refs/remotes/origin/side)
 '
 
+test_expect_success 'set-head --delete' '
+	(cd test &&
+	 git symbolic-ref refs/remotes/origin/HEAD &&
+	 git remote set-head --delete origin &&
+	 test_must_fail git symbolic-ref refs/remotes/origin/HEAD)
+'
+
+cat > test/expect <<EOF
+origin/HEAD set to master
+EOF
+
+test_expect_success 'set-head --auto' '
+	(cd test &&
+	 git remote set-head --auto origin > output &&
+	 git symbolic-ref refs/remotes/origin/HEAD &&
+	test_cmp expect output)
+'
+
+cat >test/expect <<EOF
+error: Multiple remote HEAD branches. Please choose one explicitly with:
+  git remote set-head two another
+  git remote set-head two master
+EOF
+
+test_expect_success 'set-head --auto fails w/multiple HEADs' '
+	(cd test &&
+	 test_must_fail git remote set-head --auto two >& output &&
+	test_cmp expect output)
+'
+
+cat >test/expect <<EOF
+refs/remotes/origin/side2
+EOF
+
+test_expect_success 'set-head explicit' '
+	(cd test &&
+	 git remote set-head origin side2 &&
+	 git symbolic-ref refs/remotes/origin/HEAD >output &&
+	 git remote set-head origin master &&
+	 test_cmp expect output)
+'
+
 cat > test/expect << EOF
 Pruning origin
 URL: $(pwd)/one
-- 
1.6.2.rc0.238.g0c1fe

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Jeff King]

On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote:

> In this situation, git remote set-head --auto should not try to guess
> which HEAD the user wants. This patch causes set-head to provide a
> useful error instead:
> 
> $ git remote set-head origin --auto
> error: Multiple remote HEAD branches. Please choose one explicitly with:
>   git remote set-head origin another
>   git remote set-head origin master

Thanks. The patch looks good to me, with two comments and one style nit:

> +		else if (states.heads.nr == 1)
> +			printf("  HEAD branch: %s\n",
> +				states.heads.items[0].string);
> +		else
> +			show_list("  HEAD branch%s:", &states.heads, "");

I was happy to see the common case of "we unambiguously determined HEAD"
falls back to nicer output (though I admit I did a double-take seeing
both show_list and the states.heads.nr check, I see it is because
show_list always insists on a newline).

That should help current users with simple setups, but also support
unambiguous HEAD reporting in the future (and based on what Daniel said
earlier, http should just need a client patch to pass the information
up the callstack).

> +		if (opt_a)
> +			printf("%s/HEAD set to %s\n", argv[0], head_name);

This was a surprise based on reading the commit message, but I think it
is a sensible enhancement.

> +cat > test/expect <<EOF
> +origin/HEAD set to master
> +EOF
> +
> +test_expect_success 'set-head --auto' '
> +	(cd test &&
> +	 git remote set-head --auto origin > output &&
> +	 git symbolic-ref refs/remotes/origin/HEAD &&
> +	test_cmp expect output)
> +'

I had to read this test a few times to convince myself it was right,
since you throw away the output of symbolic-ref. I think it makes more
sense to just test the post-command state, which is what you actually
care about (and then you are also not dependent on the human-readable
output of "remote set-head"). I.e.:

cat > test/expect <<EOF
refs/remotes/origin/master
EOF

test_expect_success 'set-head --auto' '
	(cd test &&
	 git remote set-head --auto origin &&
	 git symbolic-ref refs/remotes/origin/HEAD > output &&
	test_cmp expect output)
'

-Peff

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Jay Soffian]

On Sat, Feb 14, 2009 at 12:54 PM, Jeff King <peff@peff.net> wrote:
>> +             if (opt_a)
>> +                     printf("%s/HEAD set to %s\n", argv[0], head_name);
>
> This was a surprise based on reading the commit message, but I think it
> is a sensible enhancement.

It seemed that when doing something "--automatically" it might be nice
to tell the user what we just did, but I'm confused why this was a
surprise.

>> +cat > test/expect <<EOF
>> +origin/HEAD set to master
>> +EOF
>> +
>> +test_expect_success 'set-head --auto' '
>> +     (cd test &&
>> +      git remote set-head --auto origin > output &&
>> +      git symbolic-ref refs/remotes/origin/HEAD &&
>> +     test_cmp expect output)
>> +'
>
> I had to read this test a few times to convince myself it was right,
> since you throw away the output of symbolic-ref. I think it makes more
> sense to just test the post-command state, which is what you actually
> care about (and then you are also not dependent on the human-readable
> output of "remote set-head"). I.e.:
>
> cat > test/expect <<EOF
> refs/remotes/origin/master
> EOF
>
> test_expect_success 'set-head --auto' '
>        (cd test &&
>         git remote set-head --auto origin &&
>         git symbolic-ref refs/remotes/origin/HEAD > output &&
>        test_cmp expect output)
> '

Right.

j.

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Jeff King]

On Sat, Feb 14, 2009 at 01:35:03PM -0500, Jay Soffian wrote:

> On Sat, Feb 14, 2009 at 12:54 PM, Jeff King <peff@peff.net> wrote:
> >> +             if (opt_a)
> >> +                     printf("%s/HEAD set to %s\n", argv[0], head_name);
> >
> > This was a surprise based on reading the commit message, but I think it
> > is a sensible enhancement.
> 
> It seemed that when doing something "--automatically" it might be nice
> to tell the user what we just did, but I'm confused why this was a
> surprise.

I just meant that the commit message did not mention changes in this
area, and it is largely orthogonal to the rest of the patch (you could
just as easily apply this hunk without the rest of your patch, and it
would have the same value). Thus I was surprised.

But I do think it is a good change.

> > cat > test/expect <<EOF
> > refs/remotes/origin/master
> > EOF
> >
> > test_expect_success 'set-head --auto' '
> >        (cd test &&
> >         git remote set-head --auto origin &&
> >         git symbolic-ref refs/remotes/origin/HEAD > output &&
> >        test_cmp expect output)
> > '
> 
> Right.

I suspect Junio can just fix this up during application if he agrees.

<random process musing>
Which made me think of something else, with all of this talk about
reviewers that has been going on. Junio is actually in a little bit of a
special position with small changes (like style issues) to say "I'll
apply this, but tweak these changes". But the rest of us are stuck
saying "I would change this one line" to the list; then either:

  - the original submitter re-rolls the patch, which takes their time
    and everyone else's time to look at the new patch, see that it is
    trivially changed, etc

    or

  - Junio has to read the followup comments, then go back and find the
    spot in the original patch to mark it up.

Which means that there is a transaction cost to little comments due to
the extra communication. And that cost can dwarf the actual time for the
change.

I don't know if there is a better method, or better tool support. I
guess reviewers could act like the maintainer, tweaking patches and then
publishing the result, which Junio would then pull. Or instead of
publishing the result, publishing an interdiff along with comments. But
basically putting the comments into a form that can be communicated and
applied more easily, which cuts down on the communication costs.

I don't know. Just thinking out loud.

-Peff

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Which made me think of something else, with all of this talk about
> reviewers that has been going on. Junio is actually in a little bit of a
> special position with small changes (like style issues) to say "I'll
> apply this, but tweak these changes".

It is not that I am special.

What is special is an otherwise obviously good patch with a few trivial
mistakes that I can fix locally without worrying the fix-up may be wrong.
It is not even per author, it is per patch, and it is a rare exception.

Often, I notice these things *after* I applied and reviewed the results,
so it already is in my work area.  I then judge the tradeoff between an
extra round (which as you stated needs another fresh review, patch
application and testing here) and the possibility that I may make a silly
mistake myself while attempting a fix-up (such a mistake by me will not
be seen on the list and others do not have chance to catch them).

For this reason, I try to keep these "will fix up no need for resend" to
the minimum and only to the most trivial cases.

> ... But the rest of us are stuck
> saying "I would change this one line" to the list; then either:
>
>   - the original submitter re-rolls the patch, which takes their time
>     and everyone else's time to look at the new patch, see that it is
>     trivially changed, etc
>
>     or
>
>   - Junio has to read the followup comments, then go back and find the
>     spot in the original patch to mark it up.

A third option is:

	"I would change this and that" review comment message, followed by
	a separate message "Here is how I would have done it", addressed
	To: the original submitter (with in-body From: line), Cc: to the
	list and me.

The original submitter can verify the latter one, and either agree to or
disagree with it.  If the reroll is good, then I can just pick it up.  I
think you have done that in the past yourself, and the process made my
life a lot easier.

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Daniel Barkalow]

On Sat, 14 Feb 2009, Jeff King wrote:

> On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote:
> 
> > +		else if (states.heads.nr == 1)
> > +			printf("  HEAD branch: %s\n",
> > +				states.heads.items[0].string);
> > +		else
> > +			show_list("  HEAD branch%s:", &states.heads, "");
> 
> I was happy to see the common case of "we unambiguously determined HEAD"
> falls back to nicer output (though I admit I did a double-take seeing
> both show_list and the states.heads.nr check, I see it is because
> show_list always insists on a newline).
> 
> That should help current users with simple setups, but also support
> unambiguous HEAD reporting in the future (and based on what Daniel said
> earlier, http should just need a client patch to pass the information
> up the callstack).

I haven't checked lately, but I think that what's actually needed is to 
have the locate_head() function notice if the struct ref for HEAD actually 
has the symref field non-NULL, and report that as the unambiguous answer. 
This should also allow it to automatically pick up any other 
disambiguation by future sources of lists of refs that include HEAD, 
whether that's git protocol extensions, filesystem access to the repo, or 
foreign VCSes where some branches is inherently primary, or whatever.

(The direct purpose of collecting the information for http was so that it 
could figure out the sha1 at all for the remote HEAD when it's a symref; 
the code just doesn't then explicitly throw the information away.)

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Jeff King]

On Sat, Feb 14, 2009 at 03:21:30PM -0500, Daniel Barkalow wrote:

> I haven't checked lately, but I think that what's actually needed is to 
> have the locate_head() function notice if the struct ref for HEAD actually 
> has the symref field non-NULL, and report that as the unambiguous answer. 

Indeed. Something like the patch below works (on top of Jay's patches).
But it has two shortcomings:

  1. There is no test script, since we have no infrastructure for
     testing over http. I might be able to build something on top of
     what's in the http-push tests.

     I was hoping we could do the same trick for local file repos, which
     would be easy to test. But the transport code just treats them as
     regular pack uploaders; only some specialized code in clone cares
     about the difference.

     In theory we could add a new transport for local repos. I don't
     think it would make sense for its get_remote_refs function to get
     _all_ of the refs, but it could specially peek at HEAD and set the
     symref field appropriately.

  2. The guess_remote_head function is getting a little long. I think it
     would help to refactor it into two functions; one for finding the
     remote HEAD in the refs list, and the other for guessing at a ref
     which matches the HEAD.

I will try to make something a little neater later today.

> This should also allow it to automatically pick up any other 
> disambiguation by future sources of lists of refs that include HEAD, 
> whether that's git protocol extensions, filesystem access to the repo, or 
> foreign VCSes where some branches is inherently primary, or whatever.

Yes, I think the symref field for the ref is a very sensible way of
communicating the information for that reason.

Patch is below.

---
diff --git a/remote.c b/remote.c
index 6385a22..afbaccc 100644
--- a/remote.c
+++ b/remote.c
@@ -1404,6 +1404,20 @@ const struct ref *guess_remote_head(const struct ref *refs,
 	if (!remote_head)
 		return NULL;
 
+	/* if the underlying transport can represent symrefs,
+	 * then we don't need to guess at all */
+	if (remote_head->symref) {
+		for (r = mapped_refs; r; r = r->next) {
+			if (!strcmp(r->name, remote_head->symref)) {
+				if (all_matches_p) {
+					*all_matches_p = copy_ref(r);
+					(*all_matches_p)->peer_ref = NULL;
+				}
+				return r;
+			}
+		}
+	}
+
 	/* If refs/heads/master could be right, it is. */
 	if (remote_master && !hashcmp(remote_master->old_sha1,
 				      remote_head->old_sha1))

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Jeff King]

On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote:

> +test_expect_success 'set-head --auto fails w/multiple HEADs' '
> +	(cd test &&
> +	 test_must_fail git remote set-head --auto two >& output &&
> +	test_cmp expect output)

I missed it before, but there is a typo in this test (>&) that causes it
to barf.

-Peff

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Jeff King]

On Sun, Feb 15, 2009 at 12:27:40AM -0500, Jeff King wrote:

> On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote:
> 
> > +test_expect_success 'set-head --auto fails w/multiple HEADs' '
> > +	(cd test &&
> > +	 test_must_fail git remote set-head --auto two >& output &&
> > +	test_cmp expect output)
> 
> I missed it before, but there is a typo in this test (>&) that causes it
> to barf.

Actually, it should be "> output 2>&1" to pick up the error message we
are expecting.  Squashable diff is below.

The test script actually passes for me now.

---
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 49f99e9..f0be105 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -231,7 +231,7 @@ EOF
 
 test_expect_success 'set-head --auto fails w/multiple HEADs' '
 	(cd test &&
-	 test_must_fail git remote set-head --auto two >& output &&
+	 test_must_fail git remote set-head --auto two > output 2>&1 &&
 	test_cmp expect output)
 '
 

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Jeff King]

On Sat, Feb 14, 2009 at 04:15:48PM -0500, Jeff King wrote:

> On Sat, Feb 14, 2009 at 03:21:30PM -0500, Daniel Barkalow wrote:
> 
> > I haven't checked lately, but I think that what's actually needed is to 
> > have the locate_head() function notice if the struct ref for HEAD actually 
> > has the symref field non-NULL, and report that as the unambiguous answer. 
> 
> Indeed. Something like the patch below works (on top of Jay's patches).
> [...]
> I will try to make something a little neater later today.

Here is a cleaner series. It depends on all of Jay's remote patches,
including the set-head one. I can resend once the dust is a little more
settled on those patches.

The first two are prep for adding a test in 5/5:

   1/5: test scripts: refactor start_httpd helper
   2/5: add basic http clone/fetch tests

These ones are code cleanup for 5/5:

   3/5: refactor find_refs_by_name to accept const list
   4/5: remote: refactor guess_remote_head

And this is the useful one.

   5/5: remote: use exact HEAD lookup if it is available

-Peff

[PATCH 1/5] test scripts: refactor start_httpd helper

[Jeff King]

There are some redirects and some error checking that need
to be done by the caller; let's move both into the
start_httpd function so that all callers don't have to
repeat them (there is only one caller now, but another will
follow in this series).

This doesn't violate any assumptions that aren't already
being made by lib-httpd, which is happy to say "skipping"
and call test_done for a number of other cases.

Signed-off-by: Jeff King <peff@peff.net>
---
Cleanup for the next patch.

 t/lib-httpd.sh       |    9 +++++++--
 t/t5540-http-push.sh |    8 +-------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 3824020..88cfc51 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -82,13 +82,18 @@ prepare_httpd() {
 }
 
 start_httpd() {
-	prepare_httpd
+	prepare_httpd >&3 2>&4
 
 	trap 'stop_httpd; die' EXIT
 
 	"$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \
 		-f "$TEST_PATH/apache.conf" $HTTPD_PARA \
-		-c "Listen 127.0.0.1:$LIB_HTTPD_PORT" -k start
+		-c "Listen 127.0.0.1:$LIB_HTTPD_PORT" -k start \
+		>&3 2>&4
+	if ! test $? = 0; then
+		say "skipping test, web server setup failed"
+		test_done
+	fi
 }
 
 stop_httpd() {
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index c236b5e..5b93e5a 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -20,13 +20,7 @@ then
 fi
 
 . "$TEST_DIRECTORY"/lib-httpd.sh
-
-if ! start_httpd >&3 2>&4
-then
-	say "skipping test, web server setup failed"
-	test_done
-	exit
-fi
+start_httpd
 
 test_expect_success 'setup remote repository' '
 	cd "$ROOT_PATH" &&
-- 
1.6.2.rc0.256.gf004c.dirty

[PATCH 2/5] add basic http clone/fetch tests

[Jeff King]

This was mostly being tested implicitly by the "http push"
tests. But making a separate test script means that:

  - we will run fetch tests even when http pushing support
    is not built

  - when there are failures on fetching, they are easier to
    see and isolate, as they are not in the middle of push
    tests

This script defaults to running the webserver on port 5550,
and puts the original t5540 on port 5540, so that the two
can be run simultaneously without conflict (but both still
respect an externally set LIB_HTTPD_PORT).

Signed-off-by: Jeff King <peff@peff.net>
---
I started this as test infrastructure for the final patch in the series,
but I think it's nice to have a few http sanity checks in general. Of
course, one must use GIT_TEST_HTTPD to enable them. Junio, is that part
of your usual integration testing?

 Makefile              |    1 +
 t/t5540-http-push.sh  |    1 +
 t/t5550-http-fetch.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 0 deletions(-)
 create mode 100755 t/t5550-http-fetch.sh

diff --git a/Makefile b/Makefile
index ffef867..d689654 100644
--- a/Makefile
+++ b/Makefile
@@ -1368,6 +1368,7 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS
 GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
 	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@
 	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
+	@echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@
 
 ### Detect Tck/Tk interpreter path changes
 ifndef NO_TCLTK
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 5b93e5a..072ef1b 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -11,6 +11,7 @@ This test runs various sanity checks on http-push.'
 
 ROOT_PATH="$PWD"
 LIB_HTTPD_DAV=t
+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5540'}
 
 if git http-push > /dev/null 2>&1 || [ $? -eq 128 ]
 then
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
new file mode 100755
index 0000000..b6e6ec9
--- /dev/null
+++ b/t/t5550-http-fetch.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='test fetching over http'
+. ./test-lib.sh
+
+if test -n "$NO_CURL"; then
+	say 'skipping test, git built without http support'
+	test_done
+fi
+
+. "$TEST_DIRECTORY"/lib-httpd.sh
+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}
+start_httpd
+
+test_expect_success 'setup repository' '
+	echo content >file &&
+	git add file &&
+	git commit -m one
+'
+
+test_expect_success 'create http-accessible bare repository' '
+	mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	 git --bare init &&
+	 echo "exec git update-server-info" >hooks/post-update &&
+	 chmod +x hooks/post-update
+	) &&
+	git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	git push public master:master
+'
+
+test_expect_success 'clone http repository' '
+	git clone $HTTPD_URL/repo.git clone &&
+	test_cmp file clone/file
+'
+
+test_expect_success 'fetch changes via http' '
+	echo content >>file &&
+	git commit -a -m two &&
+	git push public
+	(cd clone && git pull) &&
+	test_cmp file clone/file
+'
+
+stop_httpd
+test_done
-- 
1.6.2.rc0.256.gf004c.dirty

[PATCH 3/5] refactor find_refs_by_name to accept const list

[Jeff King]

Since it doesn't actually touch its argument, this makes
sense.

However, we still want to return a non-const version (which
requires a cast) so that this:

  struct ref *a, *b;
  a = find_ref_by_name(b);

works. Unfortunately, you can also silently strip the const
from a variable:

  struct ref *a;
  const struct ref *b;
  a = find_ref_by_name(b);

This is a classic C const problem because there is no way to
say "return the type with the same constness that was passed
to us"; we provide the same semantics as standard library
functions like strchr.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h |    2 +-
 refs.c  |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 7f1a6e8..ba6c070 100644
--- a/cache.h
+++ b/cache.h
@@ -801,7 +801,7 @@ struct ref {
 #define REF_HEADS	(1u << 1)
 #define REF_TAGS	(1u << 2)
 
-extern struct ref *find_ref_by_name(struct ref *list, const char *name);
+extern struct ref *find_ref_by_name(const struct ref *list, const char *name);
 
 #define CONNECT_VERBOSE       (1u << 0)
 extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);
diff --git a/refs.c b/refs.c
index 6eb5f53..b2a37e1 100644
--- a/refs.c
+++ b/refs.c
@@ -1628,10 +1628,10 @@ int update_ref(const char *action, const char *refname,
 	return 0;
 }
 
-struct ref *find_ref_by_name(struct ref *list, const char *name)
+struct ref *find_ref_by_name(const struct ref *list, const char *name)
 {
 	for ( ; list; list = list->next)
 		if (!strcmp(list->name, name))
-			return list;
+			return (struct ref *)list;
 	return NULL;
 }
-- 
1.6.2.rc0.256.gf004c.dirty

[PATCH 4/5] remote: refactor guess_remote_head

[Jeff King]

This function had a lot of complications which made it hard
to read and extend, and confusing to call (because of
optional unused parameters). This patch attempts to address
that:

  - we used to manually search through the ref lists; this
    should be a one-line call to find_ref_by_name

  - guess_remote_head used to do two things: find the HEAD
    ref, and then find a matching ref. And only one of the
    two callers actually cared about returning the HEAD ref.
    Since it's a one-liner, just have the caller do it
    themselves (and remember it or not as they wish).

  - there were two ways of getting results out of the
    function (a return value, and an "all_matched" out
    parameter), but no caller cared about both. One of them
    returned a pointer into the passed-in ref list and one
    of them returned a newly allocated list (with the
    peer_ref fields stripped). Let's be simple and
    consistent: the return value is always a ref copy with
    a valid peer_ref. If "all" is requested, all candidates
    are returned. Otherwise, the best candidate is returned.

Signed-off-by: Jeff King <peff@peff.net>
---
The diff is horrible to read, and would look much better as a whole
function replacement. It would make a great test case for a "short lines
are uninteresting" diff feature.

 builtin-clone.c  |    5 +--
 builtin-remote.c |    5 ++-
 remote.c         |   62 ++++++++++++++++++++++-------------------------------
 remote.h         |   17 ++++++--------
 4 files changed, 38 insertions(+), 51 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index d57818c..7130cab 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -508,9 +508,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		clear_extra_refs();
 
 		mapped_refs = write_remote_refs(refs, &refspec, reflog_msg.buf);
-
-		head_points_at = guess_remote_head(refs, mapped_refs,
-						   &remote_head, NULL);
+		remote_head = find_ref_by_name(refs, "HEAD");
+		head_points_at = guess_remote_head(remote_head, mapped_refs, 0);
 	}
 	else {
 		warning("You appear to have cloned an empty repository.");
diff --git a/builtin-remote.c b/builtin-remote.c
index 608c0f3..d6958d4 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -275,7 +275,7 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)
 static int get_head_names(const struct ref *refs,
 	const char *remote_name, struct ref_states *states)
 {
-	struct ref *ref, *matches = NULL;
+	struct ref *ref, *matches;
 	struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map;
 	struct refspec refspec;
 
@@ -284,7 +284,8 @@ static int get_head_names(const struct ref *refs,
 	refspec.src = refspec.dst = "refs/heads/";
 	states->heads.strdup_strings = 1;
 	get_fetch_map(refs, &refspec, &fetch_map_tail, 0);
-	guess_remote_head(refs, fetch_map, NULL, &matches);
+	matches = guess_remote_head(find_ref_by_name(refs, "HEAD"),
+					fetch_map, 1);
 	for(ref = matches; ref; ref = ref->next)
 		string_list_append(abbrev_branch(ref->name), &states->heads);
 
diff --git a/remote.c b/remote.c
index 6385a22..7ba1bff 100644
--- a/remote.c
+++ b/remote.c
@@ -1377,51 +1377,41 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 	return 1;
 }
 
-const struct ref *guess_remote_head(const struct ref *refs,
-				    const struct ref *mapped_refs,
-				    const struct ref **remote_head_p,
-				    struct ref **all_matches_p)
+struct ref *copy_ref_with_peer(const struct ref *src)
 {
-	const struct ref *remote_head = NULL;
-	const struct ref *remote_master = NULL;
-	const struct ref *ret = NULL;
-	const struct ref *r;
-	struct ref **tail = all_matches_p;
-
-	for (r = refs; r; r = r->next)
-		if (!strcmp(r->name, "HEAD"))
-			remote_head = r;
-
-	if (!all_matches_p)
-		for (r = mapped_refs; r; r = r->next)
-			if (!strcmp(r->name, "refs/heads/master"))
-				remote_master = r;
+	struct ref *dst = copy_ref(src);
+	dst->peer_ref = copy_ref(src->peer_ref);
+	return dst;
+}
 
-	if (remote_head_p)
-		*remote_head_p = remote_head;
+struct ref *guess_remote_head(const struct ref *head,
+			      const struct ref *refs,
+			      int all)
+{
+	const struct ref *r;
+	struct ref *list = NULL;
+	struct ref **tail = &list;
 
-	/* If there's no HEAD value at all, never mind. */
-	if (!remote_head)
+	if (!head)
 		return NULL;
 
 	/* If refs/heads/master could be right, it is. */
-	if (remote_master && !hashcmp(remote_master->old_sha1,
-				      remote_head->old_sha1))
-		return remote_master;
+	if (!all) {
+		const struct ref *m;
+		m = find_ref_by_name(refs, "refs/heads/master");
+		if (m && !hashcmp(m->old_sha1, head->old_sha1))
+			return copy_ref_with_peer(m);
+	}
 
 	/* Look for another ref that points there */
-	for (r = mapped_refs; r; r = r->next)
-		if (r != remote_head &&
-		    !hashcmp(r->old_sha1, remote_head->old_sha1)) {
-			struct ref *cpy;
-			if (!ret)
-				ret = r;
-			if (!all_matches_p)
+	for (r = refs; r; r = r->next) {
+		if (r != head && !hashcmp(r->old_sha1, head->old_sha1)) {
+			*tail = copy_ref_with_peer(r);
+			tail = &((*tail)->next);
+			if (!all)
 				break;
-			*tail = cpy = copy_ref(r);
-			cpy->peer_ref = NULL;
-			tail = &cpy->next;
 		}
+	}
 
-	return ret;
+	return list;
 }
diff --git a/remote.h b/remote.h
index 8409d42..111c4ba 100644
--- a/remote.h
+++ b/remote.h
@@ -138,16 +138,13 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs);
 int format_tracking_info(struct branch *branch, struct strbuf *sb);
 
 /*
- * Look in refs for HEAD. Then look for a matching SHA1 in mapped_refs,
- * first checking if refs/heads/master matches. Return NULL if nothing matches
- * or if there is no HEAD in refs. remote_head_p is assigned HEAD if not NULL.
- * If all_matches_p is NULL, return after the first possible match. Otherwise
- * all_matches_p is set to a ref list of each branch head with the same SHA1 as
- * HEAD.
+ * Find refs from a list which are likely to be pointed to by the given HEAD
+ * ref. If 'all' is false, returns the most likely ref; otherwise, returns a
+ * list of all candidate refs. If no match is found (or 'head' is NULL),
+ * returns NULL. All returns are newly allocated and should be freed.
  */
-const struct ref *guess_remote_head(const struct ref *refs,
-				    const struct ref *mapped_refs,
-				    const struct ref **remote_head_p,
-				    struct ref **all_matches_p);
+struct ref *guess_remote_head(const struct ref *head,
+			      const struct ref *refs,
+			      int all);
 
 #endif
-- 
1.6.2.rc0.256.gf004c.dirty

[PATCH 5/5] remote: use exact HEAD lookup if it is available

[Jeff King]

Our usual method for determining the ref pointed to by HEAD
is to compare HEAD's sha1 to the sha1 of all refs, trying to
find a unique match.

However, some transports actually get to look at HEAD
directly; we should make use of that information when it is
available.  Currently, only http remotes support this
feature.

Signed-off-by: Jeff King <peff@peff.net>
---
A possible 6/5 would be to do something similar for local repos (or
resurrecting the HEAD proposal).

 remote.c              |   10 ++++++++++
 t/t5550-http-fetch.sh |   11 +++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/remote.c b/remote.c
index 7ba1bff..a96a910 100644
--- a/remote.c
+++ b/remote.c
@@ -1395,6 +1395,16 @@ struct ref *guess_remote_head(const struct ref *head,
 	if (!head)
 		return NULL;
 
+	/*
+	 * Some transports support directly peeking at
+	 * where HEAD points; if that is the case, then
+	 * we don't have to guess.
+	 */
+	if (head->symref) {
+		r = find_ref_by_name(refs, head->symref);
+		return r ? copy_ref_with_peer(r) : NULL;
+	}
+
 	/* If refs/heads/master could be right, it is. */
 	if (!all) {
 		const struct ref *m;
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index b6e6ec9..05b1b62 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -42,5 +42,16 @@ test_expect_success 'fetch changes via http' '
 	test_cmp file clone/file
 '
 
+test_expect_success 'http remote detects correct HEAD' '
+	git push public master:other &&
+	(cd clone &&
+	 git remote set-head origin -d &&
+	 git remote set-head origin -a &&
+	 git symbolic-ref refs/remotes/origin/HEAD > output &&
+	 echo refs/remotes/origin/master > expect &&
+	 test_cmp expect output
+	)
+'
+
 stop_httpd
 test_done
-- 
1.6.2.rc0.256.gf004c.dirty

Re: [PATCH 2/5] add basic http clone/fetch tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... Of
> course, one must use GIT_TEST_HTTPD to enable them. Junio, is that part
> of your usual integration testing?

No.  I do not run server tests myself.  I could enable it for tests run on
my private machine, but the final pre-pushout testing don at k.org is more
or less unattended, and it is a shared machine where I do not want to run
server-ish tests.

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Jay Soffian]

On Sun, Feb 15, 2009 at 12:27 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote:
>
>> +test_expect_success 'set-head --auto fails w/multiple HEADs' '
>> +     (cd test &&
>> +      test_must_fail git remote set-head --auto two >& output &&
>> +     test_cmp expect output)
>
> I missed it before, but there is a typo in this test (>&) that causes it
> to barf.

Didn't barf for me, but it turns out it's because it's a bash'ism[1],
and that's the default /bin/sh on OS X. Out of curiosity, on what
platform did it fail for you?

[1] Redirecting Standard Output and Standard Error
       Bash allows both the standard output (file descriptor 1) and
the  standard  error
       output  (file  descriptor 2) to be redirected to the file whose
name is the expan-
       sion of word with this construct.

       There are two formats for redirecting standard output and standard error:

              &>word
       and
              >&word

       Of the two forms, the first is preferred.  This is semantically
equivalent to

              >word 2>&1

j.

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Jeff King]

On Sun, Feb 15, 2009 at 09:13:09AM -0500, Jay Soffian wrote:

> >> +     (cd test &&
> >> +      test_must_fail git remote set-head --auto two >& output &&
> >> +     test_cmp expect output)
> >
> > I missed it before, but there is a typo in this test (>&) that causes it
> > to barf.
> 
> Didn't barf for me, but it turns out it's because it's a bash'ism[1],
> and that's the default /bin/sh on OS X. Out of curiosity, on what
> platform did it fail for you?

Ah, I didn't realize that was valid under any shell. ;) My /bin/sh is
dash, which explains it.

-Peff

Re: [PATCH 5/5] remote: use exact HEAD lookup if it is available

[Jay Soffian]

On Sun, Feb 15, 2009 at 1:18 AM, Jeff King <peff@peff.net> wrote:
> A possible 6/5 would be to do something similar for local repos (or
> resurrecting the HEAD proposal).

http://thread.gmane.org/gmane.comp.version-control.git/102039

I read that thread, and it wasn't clear to me why it died. Was there a
v3 patch round that I cannot find?  The only unaddressed reply I see
is from Jeff:

http://thread.gmane.org/gmane.comp.version-control.git/102039

But that seems tangential to the proposal.

j.

Re: [PATCH 5/5] remote: use exact HEAD lookup if it is available

[Jeff King]

On Sun, Feb 15, 2009 at 01:18:18AM -0500, Jeff King wrote:

> A possible 6/5 would be to do something similar for local repos (or
> resurrecting the HEAD proposal).

Here is a quick and dirty series to unambiguously determine the HEAD for
local repos, but I am undecided on whether this is actually a good idea.

Note that this fails Jay's tests in t5505 which expect the ambiguity; to
be considered for inclusion, it would need to test "../two" as a remote
as well as "file://$(pwd)/../two", making sure each behaved correctly.

But I am posting it here to stimulate discussion on whether it is even
something we want.

  1/2: transport: cleanup duplicated ref fetching code
  2/2: transport: unambiguously determine local HEAD

-Peff

[PATCH 1/2] transport: cleanup duplicated ref fetching code

[Jeff King]

When fetching refs through the git protocol, the
fetch_refs_via_pack will establish the connection and get
the ref list if it has not already been done.

Since the code is only two lines, it was done inline rather
than calling the transport's get_refs function. However,
calling that function better matches the intent, and is
future-proof against enhancements in get_refs_via_connect.

Signed-off-by: Jeff King <peff@peff.net>
---
Enhancements like the one that is coming in the next patch.

Though I think that fetch_pack doesn't currently care if the HEAD symref
is set up, it makes sense to me to be consistent.

 transport.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/transport.c b/transport.c
index 9ad4a16..c9f31f6 100644
--- a/transport.c
+++ b/transport.c
@@ -646,10 +646,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 	for (i = 0; i < nr_heads; i++)
 		origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
 
-	if (!data->conn) {
-		connect_setup(transport);
-		get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0, NULL);
-	}
+	if (!data->conn)
+		refs_tmp = transport->get_refs_list(transport);
 
 	refs = fetch_pack(&args, data->fd, data->conn,
 			  refs_tmp ? refs_tmp : transport->remote_refs,
-- 
1.6.2.rc0.258.gcef3.dirty

[PATCH 2/2] transport: unambiguously determine local HEAD

[Jeff King]

When we fetch refs using the git protocol, we have to guess
at which ref is pointed to by the HEAD. In the case of a
local filesystem repo, however, we can cheat by going to
that repo and peeking directly at the contents of HEAD.

Signed-off-by: Jeff King <peff@peff.net>
---
Again, this fails tests in t5505, and is not meant for inclusion.

 transport.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/transport.c b/transport.c
index c9f31f6..e575203 100644
--- a/transport.c
+++ b/transport.c
@@ -12,6 +12,8 @@
 #include "dir.h"
 #include "refs.h"
 
+static int is_local(const char *url);
+
 /* rsync support */
 
 /*
@@ -609,6 +611,40 @@ static int connect_setup(struct transport *transport)
 	return 0;
 }
 
+static void mark_head_via_local(const char *url, struct ref *refs)
+{
+	static const char *argv[] = { "symbolic-ref", "HEAD", NULL };
+	static const char *env[] = { GIT_DIR_ENVIRONMENT, NULL };
+	struct child_process cmd;
+	struct ref *head;
+	struct strbuf buf = STRBUF_INIT;
+
+	head = find_ref_by_name(refs, "HEAD");
+	if (!head)
+		return;
+
+	memset(&cmd, 0, sizeof cmd);
+	cmd.argv = argv;
+	cmd.env = env;
+	cmd.dir = url;
+	cmd.git_cmd = 1;
+	cmd.no_stdin = 1;
+	cmd.no_stderr = 1;
+	cmd.out = -1;
+
+	if (start_command(&cmd) < 0)
+		return;
+	if (strbuf_read(&buf, cmd.out, 64) < 0)
+		return;
+	if (finish_command(&cmd) != 0) {
+		strbuf_release(&buf);
+		return;
+	}
+
+	strbuf_trim(&buf);
+	head->symref = strbuf_detach(&buf, NULL);
+}
+
 static struct ref *get_refs_via_connect(struct transport *transport)
 {
 	struct git_transport_data *data = transport->data;
@@ -617,6 +653,9 @@ static struct ref *get_refs_via_connect(struct transport *transport)
 	connect_setup(transport);
 	get_remote_heads(data->fd[0], &refs, 0, NULL, 0, NULL);
 
+	if (is_local(transport->url))
+		mark_head_via_local(transport->url, refs);
+
 	return refs;
 }
 
-- 
1.6.2.rc0.258.gcef3.dirty

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

>        Bash allows both the standard output (file descriptor 1) and
>        the standard  error
>        output  (file  descriptor 2) to be redirected to the file whose
>        name is the expansion of word with this construct.
>
>        There are two formats for redirecting standard output and
>        standard error:
>
>               &>word
>        and
>               >&word
>
>        Of the two forms, the first is preferred.  This is semantically
>        equivalent to
>
>               >word 2>&1

Just to clarify, the above is not a recommendation for shell scripts in
git project.  The last one is the only one we recommend in our scripts.

By the say, does anybody know why bash people recommend &>word form?

Neither &>word nor >&word to send both stderr and stdout to the file is
from true Bourne, but at least the use of >&word form for this purpose is
more familiar to people who are used to Csh.

Re: [PATCH] builtin-remote: better handling of multiple remote HEADs

[Jay Soffian]

On Sun, Feb 15, 2009 at 9:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>               &>word
>>               >&word
> Just to clarify, the above is not a recommendation for shell scripts in
> git project.

Indeed not! My use of it wasn't even intentional, I was a tcsh
(cringe...) user for a long time and I only finally switched to bash
about a year ago. It must've been muscle memory that made me type it,
but a pox on bash for not rejecting it outright -- the bash man page
claims posix'ish compliance when invoked as /bin/sh, so I don't know
why it allows such syntax in its posix'ish mode.

> By the say, does anybody know why bash people recommend &>word form?
>
> Neither &>word nor >&word to send both stderr and stdout to the file is
> from true Bourne, but at least the use of >&word form for this purpose is
> more familiar to people who are used to Csh.

<tongue in cheek>it is exactly because it is familiar to csh people
that they recommend the opposite</tongue in cheek>

j.