threads / patch / 17770

patch, 4 partsbuiltin-clone: move locate_head() to remote.c so it can be re-used

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

## tl;dr

41 messages between Feb 13, 2009 and Feb 16, 2009. Diffs are folded; open one to read it.

replies: 40people: 4as markdown or json

Jay Soffian· Feb 13, 2009, 08:54 UTC · lore

[PATCH 0/4] remote HEAD improvements take 2

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(-)
Jay Soffian· Feb 13, 2009, 08:54 UTC · re: Jay Soffian · lore

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(-)
Show changes to 3 files +49 −38

builtin-clone.c, remote.c, remote.h

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
Jay Soffian· Feb 13, 2009, 08:54 UTC · re: Jay Soffian · lore

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

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(-)
Show changes to builtin-remote.c +9 −8
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
Jay Soffian· Feb 13, 2009, 08:54 UTC · re: Jay Soffian · lore

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

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(-)
Show changes to builtin-remote.c +26 −0
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
Jay Soffian· Feb 13, 2009, 08:54 UTC · re: Jay Soffian · lore

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

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(-)
Show changes to 3 files +72 −4

Documentation/git-remote.txt, builtin-remote.c, contrib/completion/git-completion.bash

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
Junio C Hamano· Feb 13, 2009, 10:09 UTC · re: Jay Soffian · lore

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

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.

Show 24 quoted lines
> 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?

Show 12 quoted lines
> 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?
Show 5 quoted lines
> @@ -777,6 +779,54 @@ static int show(int argc, const char **argv)
>  	return result;
>  }
>  
> +static int sethead(int argc, const char **argv)
set_head()?
Show 16 quoted lines
> +{
> +	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
	}
Jay Soffian· Feb 13, 2009, 10:21 UTC · re: Junio C Hamano · lore

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

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.
Show 14 quoted lines
>> 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.
Show 14 quoted lines
> 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.
Jay Soffian· Feb 13, 2009, 11:42 UTC · re: Jay Soffian · lore

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

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:
Show 8 quoted lines
> 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(-)
Show changes to 3 files +78 −3

Documentation/git-remote.txt, builtin-remote.c, contrib/completion/git-completion.bash

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
Junio C Hamano· Feb 13, 2009, 10:35 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> 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".

Jay Soffian· Feb 13, 2009, 10:52 UTC · re: Junio C Hamano · lore

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

On Fri, Feb 13, 2009 at 5:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> 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.
Show 8 quoted lines
> 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.
Jeff King· Feb 14, 2009, 00:22 UTC · re: Junio C Hamano · lore

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

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.

Junio C Hamano· Feb 14, 2009, 02:00 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> 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.
Show 9 quoted lines
> 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.

Jeff King· Feb 14, 2009, 02:18 UTC · re: Junio C Hamano · lore

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

On Fri, Feb 13, 2009 at 06:00:29PM -0800, Junio C Hamano wrote:
Show 5 quoted lines
> > 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. :(

Show 5 quoted lines
> 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.

---
Show changes to t/t5505-remote.sh +1 −10
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
Jay Soffian· Feb 14, 2009, 02:48 UTC · re: Jeff King · lore

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

On Fri, Feb 13, 2009 at 9:18 PM, Jeff King <peff@peff.net> wrote:
Show 18 quoted lines
> 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.
Jay Soffian· Feb 14, 2009, 02:59 UTC · re: Junio C Hamano · lore

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

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.
Jeff King· Feb 14, 2009, 03:43 UTC · re: Jay Soffian · lore

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

On Fri, Feb 13, 2009 at 09:59:06PM -0500, Jay Soffian wrote:
Show 9 quoted lines
> 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".

Show 9 quoted lines
> 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
Jay Soffian· Feb 14, 2009, 10:30 UTC · re: Jeff King · lore

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

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:
Show 17 quoted lines
> 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(-)
Show changes to 5 files +114 −31

builtin-clone.c, builtin-remote.c, remote.c, remote.h, t/t5505-remote.sh

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
Jeff King· Feb 14, 2009, 17:54 UTC · re: Jay Soffian · lore

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

On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote:
Show 8 quoted lines
> 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:
Show 5 quoted lines
> +		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.

Show 10 quoted lines
> +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
Jay Soffian· Feb 14, 2009, 18:35 UTC · re: Jeff King · lore

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

On Sat, Feb 14, 2009 at 12:54 PM, Jeff King <peff@peff.net> wrote:
Show 5 quoted lines
>> +             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.

Show 27 quoted lines
>> +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.
Jeff King· Feb 14, 2009, 18:54 UTC · re: Jay Soffian · lore

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

On Sat, Feb 14, 2009 at 01:35:03PM -0500, Jay Soffian wrote:
Show 10 quoted lines
> 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.
Show 12 quoted lines
> > 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
Junio C Hamano· Feb 14, 2009, 19:48 UTC · re: Jeff King · lore

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

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.

Show 11 quoted lines
> ... 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.

Daniel Barkalow· Feb 14, 2009, 20:21 UTC · re: Jeff King · lore

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

On Sat, 14 Feb 2009, Jeff King wrote:
Show 17 quoted lines
> 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*
Jeff King· Feb 14, 2009, 21:15 UTC · re: Daniel Barkalow · lore

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

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.
---
Show changes to remote.c +14 −0
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))
Jeff King· Feb 15, 2009, 06:08 UTC · re: Jeff King · lore

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

On Sat, Feb 14, 2009 at 04:15:48PM -0500, Jeff King wrote:
Show 9 quoted lines
> 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
Jeff King· Feb 15, 2009, 06:10 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +8 −9

t/lib-httpd.sh, t/t5540-http-push.sh

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
Jeff King· Feb 15, 2009, 06:12 UTC · re: Jeff King · lore

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

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
Show changes to 3 files +48 −0

Makefile, t/t5540-http-push.sh, 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
Junio C Hamano· Feb 15, 2009, 08:01 UTC · re: Jeff King · lore

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

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.

Jeff King· Feb 15, 2009, 06:12 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +3 −3

cache.h, refs.c

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
Jeff King· Feb 15, 2009, 06:16 UTC · re: Jeff King · lore

[PATCH 4/5] remote: refactor guess_remote_head

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(-)
Show changes to 4 files +38 −51

builtin-clone.c, builtin-remote.c, remote.c, remote.h

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
Jeff King· Feb 15, 2009, 06:18 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +21 −0

remote.c, t/t5550-http-fetch.sh

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
Jay Soffian· Feb 15, 2009, 15:22 UTC · re: Jeff King · lore

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

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.
Jeff King· Feb 15, 2009, 19:58 UTC · re: Jeff King · lore

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

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
Jeff King· Feb 15, 2009, 20:00 UTC · re: Jeff King · lore

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

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(-)
Show changes to transport.c +2 −4
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
Jeff King· Feb 15, 2009, 20:01 UTC · re: Jeff King · lore

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

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(-)
Show changes to transport.c +39 −0
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
Jeff King· Feb 15, 2009, 05:27 UTC · re: Jay Soffian · lore

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

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
Jeff King· Feb 15, 2009, 05:34 UTC · re: Jeff King · lore

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

On Sun, Feb 15, 2009 at 12:27:40AM -0500, Jeff King wrote:
Show 9 quoted lines
> 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.
---
Show changes to t/t5505-remote.sh +1 −1
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)
 '
 
Jay Soffian· Feb 15, 2009, 14:13 UTC · re: Jeff King · lore

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

On Sun, Feb 15, 2009 at 12:27 AM, Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
> 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.
Jeff King· Feb 15, 2009, 15:12 UTC · re: Jay Soffian · lore

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

On Sun, Feb 15, 2009 at 09:13:09AM -0500, Jay Soffian wrote:
Show 10 quoted lines
> >> +     (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
Junio C Hamano· Feb 16, 2009, 02:21 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:
Show 16 quoted lines
>        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.

Jay Soffian· Feb 16, 2009, 02:58 UTC · re: Junio C Hamano · lore

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

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.

Show 5 quoted lines
> 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.
Jay Soffian· Feb 13, 2009, 08:57 UTC · re: Jay Soffian · lore

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

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.

← back to recent threads