[PATCH 0/3] remote HEAD improvements

[PATCH 0/3] remote HEAD improvements

[Jay Soffian]

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

1) Teaches git remote a new "sethead" verb:

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

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

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

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) Documents the new sethead 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 (3):
  builtin-remote: move duplicated cleanup code its own function
  builtin-remote: teach show to display remote HEAD
  builtin-remote: add sethead verb

 Documentation/git-remote.txt |   20 +++++++-
 builtin-remote.c             |  108 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 118 insertions(+), 10 deletions(-)

[PATCH 1/3] builtin-remote: move duplicated cleanup code its own function

[Jay Soffian]

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

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

diff --git a/builtin-remote.c b/builtin-remote.c
index db18bcf..00e7ca5 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -632,6 +632,14 @@ 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)
+{
+	/* NEEDSWORK: free remote */
+	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 +746,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;
@@ -786,10 +791,7 @@ static int prune(int argc, const char **argv)
 			       abbrev_ref(refname, "refs/remotes/"));
 		}
 
-		/* 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.187.g9fcfb

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

[Jay Soffian]

The remote HEAD cannot be determined unambiguosly, so use the same
heuristics as builtin-clone's locate_head().

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

diff --git a/builtin-remote.c b/builtin-remote.c
index 00e7ca5..0be8bfd 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,38 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)
 	return 0;
 }
 
+static char *get_head_name(const struct ref *ref)
+{
+	const struct ref *remote_head = NULL;
+	const struct ref *remote_master = NULL;
+	const struct ref *r;
+	for (r = ref; r; r = r->next) {
+		if (!strcmp(r->name, "HEAD"))
+			remote_head = r;
+		if (!strcmp(r->name, "refs/heads/master"))
+			remote_master = r;
+	}
+
+	/* 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 xstrdup(abbrev_branch(remote_master->name));
+
+	/* Look for another ref that points there */
+	for (r = ref; r; r = r->next)
+		if (r != remote_head &&
+		    !hashcmp(r->old_sha1, remote_head->old_sha1) &&
+		    !prefixcmp(r->name, "refs/heads/"))
+			return xstrdup(abbrev_branch(r->name));
+
+	/* Nothing is the same */
+	return NULL;
+}
+
 struct known_remote {
 	struct known_remote *next;
 	struct remote *remote;
@@ -638,6 +671,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);
+	if (states->head_name)
+		free(states->head_name);
 }
 
 static int get_remote_ref_states(const char *name,
@@ -659,6 +694,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);
 	}
 
@@ -703,6 +739,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.187.g9fcfb

[PATCH 3/3] builtin-remote: add sethead verb

[Jay Soffian]

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

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

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 Documentation/git-remote.txt |   20 ++++++++++++++-
 builtin-remote.c             |   51 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index fad983e..fa7ec46 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 sethead' <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 sethead 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.
 
+'sethead'::
+
+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 0be8bfd..0715685 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 sethead <name> [-a | -d | <branch>]",
 	"git remote show [-n] <name>",
 	"git remote prune [-n | --dry-run] <name>",
 	"git remote [-v | --verbose] update [group]",
@@ -791,6 +792,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("sethead 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 sethead"))
+			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;
@@ -955,6 +1004,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], "sethead"))
+		result = sethead(argc, argv);
 	else if (!strcmp(argv[0], "show"))
 		result = show(argc, argv);
 	else if (!strcmp(argv[0], "prune"))
-- 
1.6.2.rc0.187.g9fcfb

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

[Jeff King]

On Wed, Feb 11, 2009 at 01:01:22AM -0500, Jay Soffian wrote:

> +static char *get_head_name(const struct ref *ref)
> +{
> +	const struct ref *remote_head = NULL;
> +	const struct ref *remote_master = NULL;
> +	const struct ref *r;
> +	for (r = ref; r; r = r->next) {
> +		if (!strcmp(r->name, "HEAD"))
> +			remote_head = r;
> +		if (!strcmp(r->name, "refs/heads/master"))
> +			remote_master = r;
> +	}
> +
> +	/* 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 xstrdup(abbrev_branch(remote_master->name));
> +
> +	/* Look for another ref that points there */
> +	for (r = ref; r; r = r->next)
> +		if (r != remote_head &&
> +		    !hashcmp(r->old_sha1, remote_head->old_sha1) &&
> +		    !prefixcmp(r->name, "refs/heads/"))
> +			return xstrdup(abbrev_branch(r->name));
> +
> +	/* Nothing is the same */
> +	return NULL;
> +}

Yuck. Cut and paste from builtin-clone.c. It's not so much the number of
lines here (although of course I don't like that, either) but that this
function encompasses a heuristic for matching the HEAD. Which means it
may change in the future, and I really don't want the clone behavior and
the remote behavior to diverge.

So can we refactor it into a library function?

I see that the inputs and outputs aren't exactly the same in both cases,
but I think you could do it like:

  struct ref *guess_head_ref(const struct ref *refs_with_head,
                             const struct ref *refs_that_might_match_head,
                             struct ref **remote_head_p);

and then just call:

  r = guess_head_ref(refs, refs, NULL);
  states->head_name = r ? xstrdup(abbrev_branch(r->name)) : NULL;

from git-remote, which at least keeps the changeable parts all
contained.

-Peff

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

[Jay Soffian]

On Wed, Feb 11, 2009 at 7:26 PM, Jeff King <peff@peff.net> wrote:

> Yuck.

Damn, I knew I wasn't going to slip that one by. :-)

> I see that the inputs and outputs aren't exactly the same in both cases,

Which is why I didn't refactor it. The extra code needed to massage
what builtin-remote.c has to the existing function in builtin-clone.c
would've been more LOC than the duplicate code (I think...).

BUT

I'll try. :-)

j.

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

[Jeff King]

On Wed, Feb 11, 2009 at 08:48:42PM -0500, Jay Soffian wrote:

> Damn, I knew I wasn't going to slip that one by. :-)

"Given enough eyeballs, all ugly hacks are shallow." :)

> > I see that the inputs and outputs aren't exactly the same in both cases,
> 
> Which is why I didn't refactor it. The extra code needed to massage
> what builtin-remote.c has to the existing function in builtin-clone.c
> would've been more LOC than the duplicate code (I think...).
> 
> BUT
> 
> I'll try. :-)

See what you can do. But personally, I am not as concerned with reducing
LOC as I am with encapsulating system logic like "this is how you guess
which ref is HEAD". And if you can do both, great. :)

-Peff

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

[Daniel Barkalow]

On Wed, 11 Feb 2009, Jay Soffian wrote:

> On Wed, Feb 11, 2009 at 7:26 PM, Jeff King <peff@peff.net> wrote:
> 
> > Yuck.
> 
> Damn, I knew I wasn't going to slip that one by. :-)
> 
> > I see that the inputs and outputs aren't exactly the same in both cases,
> 
> Which is why I didn't refactor it. The extra code needed to massage
> what builtin-remote.c has to the existing function in builtin-clone.c
> would've been more LOC than the duplicate code (I think...).

Isn't it just:

struct ref *head = locate_head(refs, refs, NULL);
return head ? xstrdup(abbrev_branch(head->name)) : NULL;

?

I'd somehow thought I'd moved locate_head() somewhere common, but it 
really ought to be done. There were periodic discussions of how you find 
out when the remote repo changes its HEAD and you might want to do 
something locally about it, and we never came up with a specific thing 
for git to do, but the facility is probably useful.

I have the vague memory, as well, that there's some way for a transport to 
report that it actually knows that HEAD is a symref to something in 
particular, and so git shouldn't guess.

	-Daniel
*This .sig left intentionally blank*

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

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> I have the vague memory, as well, that there's some way for a transport to 
> report that it actually knows that HEAD is a symref to something in 
> particular, and so git shouldn't guess.

I think you are thinking about:

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

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

[Daniel Barkalow]

On Thu, 12 Feb 2009, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > I have the vague memory, as well, that there's some way for a transport to 
> > report that it actually knows that HEAD is a symref to something in 
> > particular, and so git shouldn't guess.
> 
> I think you are thinking about:
> 
>     http://thread.gmane.org/gmane.comp.version-control.git/102039

Actually, I think I missed that thread entirely; I was thinking of earlier 
work to allow the http transport, when it finds that HEAD is a symref 
file, to report that there's a ref named "HEAD", with a particular value, 
and that it's incidentally known to be a symref to another particular ref.

Ended up in be885d96fe0ebed47e637f3b0dd24fc5902f7081, but I don't think 
anything was set up to actually do anything with the information.

	-Daniel
*This .sig left intentionally blank*

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

[Jay Soffian]

On Thu, Feb 12, 2009 at 3:27 PM, Daniel Barkalow <barkalow@iabervon.org> wrote:
>
> struct ref *head = locate_head(refs, refs, NULL);
> return head ? xstrdup(abbrev_branch(head->name)) : NULL;
>
> ?

No, I don't _think_ so. refs is everything from the remote side (tags,
etc). I want to only match those things under refs/heads.

I think I have to do something like this (this is more or less what
builtin-clone does):

struct ref *remote_refs, *mapped_refs = NULL;
struct refspec branch_refspec;

branch_refspec.force = 0;
branch_refspec.src = branch_refspec.dst = "refs/heads";

remote_refs = transport_get_remote_refs(transport);
get_fetch_map(remote_refs, branch_refspec, &mapped_refs, 0);
head_points_at = locate_head(refs, mapped_refs, NULL);

> I'd somehow thought I'd moved locate_head() somewhere common, but it
> really ought to be done.

I plan to move it into remote.c.

> There were periodic discussions of how you find
> out when the remote repo changes its HEAD and you might want to do
> something locally about it, and we never came up with a specific thing
> for git to do, but the facility is probably useful.

Thus "git remote set-head -a" is the best I could come up with for
setting it to what the remote has.

> I have the vague memory, as well, that there's some way for a transport to
> report that it actually knows that HEAD is a symref to something in
> particular, and so git shouldn't guess.

I think for http://, but not for git://, but I'm *far* from an expert
in this area.

j.

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

[Daniel Barkalow]

On Thu, 12 Feb 2009, Jay Soffian wrote:

> On Thu, Feb 12, 2009 at 3:27 PM, Daniel Barkalow <barkalow@iabervon.org> wrote:
> >
> > struct ref *head = locate_head(refs, refs, NULL);
> > return head ? xstrdup(abbrev_branch(head->name)) : NULL;
> >
> > ?
> 
> No, I don't _think_ so. refs is everything from the remote side (tags,
> etc). I want to only match those things under refs/heads.

I was looking at your code and comparing with locate_head; it looks to me 
like you're using "ref" for both of the lists of refs that locate_head() 
gets. I think, actually, that checking for refs/heads in locate_head() 
would be a reasonable thing in general, and might avoid getting surprising 
results in clone with --mirror and odd refs or such.

> I think I have to do something like this (this is more or less what
> builtin-clone does):
> 
> struct ref *remote_refs, *mapped_refs = NULL;
> struct refspec branch_refspec;
> 
> branch_refspec.force = 0;
> branch_refspec.src = branch_refspec.dst = "refs/heads";
> 
> remote_refs = transport_get_remote_refs(transport);
> get_fetch_map(remote_refs, branch_refspec, &mapped_refs, 0);
> head_points_at = locate_head(refs, mapped_refs, NULL);

You should be able to filter the ref list you already have, rather than 
refetching, if nothing else.

> > I'd somehow thought I'd moved locate_head() somewhere common, but it
> > really ought to be done.
> 
> I plan to move it into remote.c.

Good.

> > There were periodic discussions of how you find
> > out when the remote repo changes its HEAD and you might want to do
> > something locally about it, and we never came up with a specific thing
> > for git to do, but the facility is probably useful.
> 
> Thus "git remote set-head -a" is the best I could come up with for
> setting it to what the remote has.

Yeah, that seems like a good interface.

> > I have the vague memory, as well, that there's some way for a transport to
> > report that it actually knows that HEAD is a symref to something in
> > particular, and so git shouldn't guess.
> 
> I think for http://, but not for git://, but I'm *far* from an expert
> in this area.

Yes, it was information only available for http, but there's no reason to 
assume that other protocols couldn't provide it, and Junio mentioned a 
series from December (maybe never applied) to get the same info for the 
native git protocol.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 1/3] builtin-remote: move duplicated cleanup code its own function

[Jeff King]

On Wed, Feb 11, 2009 at 01:01:21AM -0500, Jay Soffian wrote:

> +static void free_remote_ref_states(struct ref_states *states)
> +{
> +	/* NEEDSWORK: free remote */
> +	string_list_clear(&states->new, 0);
> +	string_list_clear(&states->stale, 0);
> +	string_list_clear(&states->tracked, 0);
> +}

Hmm. I don't know anything about this code, so maybe it is not trivial.
But anytime you are touching an area that NEEDSWORK, I think it is worth
looking at whether you can fix that problem (since you have already
spent a few brain cycles understanding what is going on in general).

-Peff

Re: [PATCH 1/3] builtin-remote: move duplicated cleanup code its own function

[Jay Soffian]

On Wed, Feb 11, 2009 at 7:18 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 11, 2009 at 01:01:21AM -0500, Jay Soffian wrote:
>
>> +static void free_remote_ref_states(struct ref_states *states)
>> +{
>> +     /* NEEDSWORK: free remote */
>> +     string_list_clear(&states->new, 0);
>> +     string_list_clear(&states->stale, 0);
>> +     string_list_clear(&states->tracked, 0);
>> +}
>
> Hmm. I don't know anything about this code, so maybe it is not trivial.
> But anytime you are touching an area that NEEDSWORK, I think it is worth
> looking at whether you can fix that problem (since you have already
> spent a few brain cycles understanding what is going on in general).

I spent about 5 minutes which was enough time for me to realize that
the reason the previous author left it as "NEEDSWORK" is because
fixing it is more than 5 minutes of work. This is the remote object --
maybe you could offer me some clues that allow me to know which of its
fields need to be freed individually:

struct remote {
	const char *name;
	int origin;

	const char **url;
	int url_nr;
	int url_alloc;

	const char **push_refspec;
	struct refspec *push;
	int push_refspec_nr;
	int push_refspec_alloc;

	const char **fetch_refspec;
	struct refspec *fetch;
	int fetch_refspec_nr;
	int fetch_refspec_alloc;

	/*
	 * -1 to never fetch tags
	 * 0 to auto-follow tags on heuristic (default)
	 * 1 to always auto-follow tags
	 * 2 to always fetch tags
	 */
	int fetch_tags;
	int skip_default_update;
	int mirror;

	const char *receivepack;
	const char *uploadpack;

	/*
	 * for curl remotes only
	 */
	char *http_proxy;
};

I *think* const is a clue that the field need not be freed, because
the pointer is to storage that is on the stack. But I wasn't sure, esp
with the double pointers. And I really wasn't sure about the struct
pointers.

Really, I only pretend to know C. :-)

j.

Re: [PATCH 1/3] builtin-remote: move duplicated cleanup code its own function

[Jeff King]

On Wed, Feb 11, 2009 at 08:44:13PM -0500, Jay Soffian wrote:

> I spent about 5 minutes which was enough time for me to realize that
> the reason the previous author left it as "NEEDSWORK" is because
> fixing it is more than 5 minutes of work. This is the remote object --
> maybe you could offer me some clues that allow me to know which of its
> fields need to be freed individually:
> [...]
> I *think* const is a clue that the field need not be freed, because
> the pointer is to storage that is on the stack. But I wasn't sure, esp
> with the double pointers. And I really wasn't sure about the struct
> pointers.

OK, I am satisfied that it is not trivial, and doesn't need to be part
of this patch series. :)

Like I said, I don't actually know this corner of the code very well,
but since you hadn't mentioned it in your cover letter, I didn't know if
it was "too lazy to do cleanups" or "code is too scary to be cleaned
up".

-Peff

Re: [PATCH 1/3] builtin-remote: move duplicated cleanup code its own function

[Daniel Barkalow]

On Wed, 11 Feb 2009, Jay Soffian wrote:

> On Wed, Feb 11, 2009 at 7:18 PM, Jeff King <peff@peff.net> wrote:
> > On Wed, Feb 11, 2009 at 01:01:21AM -0500, Jay Soffian wrote:
> >
> >> +static void free_remote_ref_states(struct ref_states *states)
> >> +{
> >> +     /* NEEDSWORK: free remote */
> >> +     string_list_clear(&states->new, 0);
> >> +     string_list_clear(&states->stale, 0);
> >> +     string_list_clear(&states->tracked, 0);
> >> +}
> >
> > Hmm. I don't know anything about this code, so maybe it is not trivial.
> > But anytime you are touching an area that NEEDSWORK, I think it is worth
> > looking at whether you can fix that problem (since you have already
> > spent a few brain cycles understanding what is going on in general).
> 
> I spent about 5 minutes which was enough time for me to realize that
> the reason the previous author left it as "NEEDSWORK" is because
> fixing it is more than 5 minutes of work. This is the remote object --
> maybe you could offer me some clues that allow me to know which of its
> fields need to be freed individually:
> 
> struct remote {
> 	const char *name;
> 	int origin;
> 
> 	const char **url;
> 	int url_nr;
> 	int url_alloc;
> 
> 	const char **push_refspec;
> 	struct refspec *push;
> 	int push_refspec_nr;
> 	int push_refspec_alloc;
> 
> 	const char **fetch_refspec;
> 	struct refspec *fetch;
> 	int fetch_refspec_nr;
> 	int fetch_refspec_alloc;
> 
> 	/*
> 	 * -1 to never fetch tags
> 	 * 0 to auto-follow tags on heuristic (default)
> 	 * 1 to always auto-follow tags
> 	 * 2 to always fetch tags
> 	 */
> 	int fetch_tags;
> 	int skip_default_update;
> 	int mirror;
> 
> 	const char *receivepack;
> 	const char *uploadpack;
> 
> 	/*
> 	 * for curl remotes only
> 	 */
> 	char *http_proxy;
> };
> 
> I *think* const is a clue that the field need not be freed, because
> the pointer is to storage that is on the stack. But I wasn't sure, esp
> with the double pointers. And I really wasn't sure about the struct
> pointers.

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.

(The code actually reads the config files once, generating info for all of 
the configured remotes, and just returns them, except that it will 
generate a new object for unconfigured, individually requested URLs)

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 0/3] remote HEAD improvements

[Jeff King]

On Wed, Feb 11, 2009 at 01:01:20AM -0500, Jay Soffian wrote:

> 1) Teaches git remote a new "sethead" verb:
> 
>   To set a remote HEAD explicitly:
>   $ git remote sethead <name> <branch>
> 
>   To set a remote HEAD to match the upstream repo:
>   $ git remote sethead <name> -a
> 
>   To delete a remote HEAD:
>   $ git remote sethead <name> -d

I like these semantics a lot. I will do worse than bikeshed, though, and
say that I don't like the color you painted it without even proposing a
color of my own. Which is to say, I think "sethead" is not the best
name. But I like it better than the alternatives I've seen, so maybe it
is OK.

Perhaps it is just the run-together word that makes it worse. Something
like "set-head" might be better (I guess I couldn't resist suggesting a
color, after all).

I have a few comments, which I will post in reply to the individual
patches.

-Peff