{"thread":{"id":"17715","subject":"[PATCH 0/3] remote HEAD improvements","startedAt":"2009-02-11T06:01:20Z","lastAt":"2009-02-12T22:55:04Z","messageCount":17,"participants":["Jay Soffian","Jeff King","Daniel Barkalow","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"104142","messageId":"1234332083-45147-1-git-send-email-jaysoffian@gmail.com","threadId":"17715","inReplyTo":null,"subject":"[PATCH 0/3] remote HEAD improvements","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-11T06:01:20Z","receivedAt":"2009-02-11T06:01:20Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"There is currently no porcelain for dealing with remote HEADs (i.e.\n$GIT_DIR/remotes//HEAD). This series:\n\n1) Teaches git remote a new \"sethead\" verb:\n\n To set a remote HEAD explicitly:\n $ git remote sethead \n\n To set a remote HEAD to match the upstream repo:\n $ git remote sethead -a\n\n To delete a remote HEAD:\n $ git remote sethead -d\n\n2) Teaches git remote show to display the remote HEAD:\n\n $ git remote show origin\n\n* remote origin\n URL: git://git.kernel.org/pub/scm/git/git.git\n HEAD: master\n\n3) Documents the new sethead verb. I also correct the git remote man page\nw/respect to the \"-m \" option. The man page implied that the remote\nHEAD was set automatically when adding a remote (a la git clone), but this is\nnot true. And, since I couldn't find anywhere else that the point of having a\nremote HEAD is documented, I documented it here.\n\nJay Soffian (3):\n builtin-remote: move duplicated cleanup code its own function\n builtin-remote: teach show to display remote HEAD\n builtin-remote: add sethead verb\n\n Documentation/git-remote.txt | 20 +++++++-\n builtin-remote.c | 108 ++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 118 insertions(+), 10 deletions(-)\n"},{"id":"104143","messageId":"1234332083-45147-2-git-send-email-jaysoffian@gmail.com","threadId":"17715","inReplyTo":"1234332083-45147-1-git-send-email-jaysoffian@gmail.com","subject":"[PATCH 1/3] builtin-remote: move duplicated cleanup code its own function","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-11T06:01:21Z","receivedAt":"2009-02-11T06:01:21Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"Moved some identical lines of code into their own function in\npreparation for adding additional functionality which will use this\nfunction as well.\n\nSigned-off-by: Jay Soffian \n---\n builtin-remote.c | 18 ++++++++++--------\n 1 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex db18bcf..00e7ca5 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -632,6 +632,14 @@ static void show_list(const char *title, struct string_list *list,\n \t\tprintf(\" %s\\n\", list->items[i].string);\n }\n \n+static void free_remote_ref_states(struct ref_states *states)\n+{\n+\t/* NEEDSWORK: free remote */\n+\tstring_list_clear(&states->new, 0);\n+\tstring_list_clear(&states->stale, 0);\n+\tstring_list_clear(&states->tracked, 0);\n+}\n+\n static int get_remote_ref_states(const char *name,\n \t\t\t\t struct ref_states *states,\n \t\t\t\t int query)\n@@ -738,10 +746,7 @@ static int show(int argc, const char **argv)\n \t\t\t}\n \t\t}\n \n-\t\t/* NEEDSWORK: free remote */\n-\t\tstring_list_clear(&states.new, 0);\n-\t\tstring_list_clear(&states.stale, 0);\n-\t\tstring_list_clear(&states.tracked, 0);\n+\t\tfree_remote_ref_states(&states);\n \t}\n \n \treturn result;\n@@ -786,10 +791,7 @@ static int prune(int argc, const char **argv)\n \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n \t\t}\n \n-\t\t/* NEEDSWORK: free remote */\n-\t\tstring_list_clear(&states.new, 0);\n-\t\tstring_list_clear(&states.stale, 0);\n-\t\tstring_list_clear(&states.tracked, 0);\n+\t\tfree_remote_ref_states(&states);\n \t}\n \n \treturn result;\n-- \n1.6.2.rc0.187.g9fcfb\n"},{"id":"104144","messageId":"1234332083-45147-3-git-send-email-jaysoffian@gmail.com","threadId":"17715","inReplyTo":"1234332083-45147-2-git-send-email-jaysoffian@gmail.com","subject":"[PATCH 2/3] builtin-remote: teach show to display remote HEAD","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-11T06:01:22Z","receivedAt":"2009-02-11T06:01:22Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"The remote HEAD cannot be determined unambiguosly, so use the same\nheuristics as builtin-clone's locate_head().\n\nSigned-off-by: Jay Soffian \n---\n builtin-remote.c | 39 +++++++++++++++++++++++++++++++++++++++\n 1 files changed, 39 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 00e7ca5..0be8bfd 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -212,6 +212,7 @@ static void read_branches(void)\n struct ref_states {\n \tstruct remote *remote;\n \tstruct string_list new, stale, tracked;\n+\tchar *head_name;\n };\n \n static int handle_one_branch(const char *refname,\n@@ -271,6 +272,38 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)\n \treturn 0;\n }\n \n+static char *get_head_name(const struct ref *ref)\n+{\n+\tconst struct ref *remote_head = NULL;\n+\tconst struct ref *remote_master = NULL;\n+\tconst struct ref *r;\n+\tfor (r = ref; r; r = r->next) {\n+\t\tif (!strcmp(r->name, \"HEAD\"))\n+\t\t\tremote_head = r;\n+\t\tif (!strcmp(r->name, \"refs/heads/master\"))\n+\t\t\tremote_master = r;\n+\t}\n+\n+\t/* If there's no HEAD value at all, never mind. */\n+\tif (!remote_head)\n+\t\treturn NULL;\n+\n+\t/* If refs/heads/master could be right, it is. */\n+\tif (remote_master && !hashcmp(remote_master->old_sha1,\n+\t\t\t\t remote_head->old_sha1))\n+\t\treturn xstrdup(abbrev_branch(remote_master->name));\n+\n+\t/* Look for another ref that points there */\n+\tfor (r = ref; r; r = r->next)\n+\t\tif (r != remote_head &&\n+\t\t !hashcmp(r->old_sha1, remote_head->old_sha1) &&\n+\t\t !prefixcmp(r->name, \"refs/heads/\"))\n+\t\t\treturn xstrdup(abbrev_branch(r->name));\n+\n+\t/* Nothing is the same */\n+\treturn NULL;\n+}\n+\n struct known_remote {\n \tstruct known_remote *next;\n \tstruct remote *remote;\n@@ -638,6 +671,8 @@ static void free_remote_ref_states(struct ref_states *states)\n \tstring_list_clear(&states->new, 0);\n \tstring_list_clear(&states->stale, 0);\n \tstring_list_clear(&states->tracked, 0);\n+\tif (states->head_name)\n+\t\tfree(states->head_name);\n }\n \n static int get_remote_ref_states(const char *name,\n@@ -659,6 +694,7 @@ static int get_remote_ref_states(const char *name,\n \t\tref = transport_get_remote_refs(transport);\n \t\ttransport_disconnect(transport);\n \n+\t\tstates->head_name = get_head_name(ref);\n \t\tget_ref_states(ref, states);\n \t}\n \n@@ -703,6 +739,9 @@ static int show(int argc, const char **argv)\n \t\tprintf(\"* remote %s\\n URL: %s\\n\", *argv,\n \t\t\tstates.remote->url_nr > 0 ?\n \t\t\t\tstates.remote->url[0] : \"(no URL)\");\n+\t\tif (!no_query)\n+\t\t\tprintf(\" HEAD: %s\\n\", states.head_name ?\n+\t\t\t\tstates.head_name : \"(unknown)\");\n \n \t\tfor (i = 0; i < branch_list.nr; i++) {\n \t\t\tstruct string_list_item *branch = branch_list.items + i;\n-- \n1.6.2.rc0.187.g9fcfb\n"},{"id":"104145","messageId":"1234332083-45147-4-git-send-email-jaysoffian@gmail.com","threadId":"17715","inReplyTo":"1234332083-45147-3-git-send-email-jaysoffian@gmail.com","subject":"[PATCH 3/3] builtin-remote: add sethead verb","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-11T06:01:23Z","receivedAt":"2009-02-11T06:01:23Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"Provide a porcelain command for setting/deleting\n$GIT_DIR/remotes//HEAD.\n\nWhile we're at it, document what $GIT_DIR/remotes//HEAD is all\nabout.\n\nSigned-off-by: Jay Soffian \n---\n Documentation/git-remote.txt | 20 ++++++++++++++-\n builtin-remote.c | 51 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\nindex fad983e..fa7ec46 100644\n--- a/Documentation/git-remote.txt\n+++ b/Documentation/git-remote.txt\n@@ -13,6 +13,7 @@ SYNOPSIS\n 'git remote add' [-t ] [-m ] [-f] [--mirror] \n 'git remote rename' \n 'git remote rm' \n+'git remote sethead' [-a | -d | ]\n 'git remote show' [-n] \n 'git remote prune' [-n | --dry-run] \n 'git remote update' [group]\n@@ -53,8 +54,7 @@ is created. You can give more than one `-t ` to track\n multiple branches without grabbing all branches.\n +\n With `-m ` option, `$GIT_DIR/remotes//HEAD` is set\n-up to point at remote's `` branch instead of whatever\n-branch the `HEAD` at the remote repository actually points at.\n+up to point at remote's `` branch. See also the sethead command.\n +\n In mirror mode, enabled with `\\--mirror`, the refs will not be stored\n in the 'refs/remotes/' namespace, but in 'refs/heads/'. This option\n@@ -76,6 +76,22 @@ the configuration file format.\n Remove the remote named . All remote tracking branches and\n configuration settings for the remote are removed.\n \n+'sethead'::\n+\n+Sets or deletes the default branch (`$GIT_DIR/remotes//HEAD`) for\n+the named remote. Having a default branch for a remote is not required,\n+but allows the name of the remote to be specified in lieu of a specific\n+branch. For example, if the default branch for `origin` is set to\n+`master`, then `origin` may be specified wherever you would normally\n+specify `origin/master`.\n++\n+With `-d`, `$GIT_DIR/remotes//HEAD` is deleted.\n++\n+With `-a`, the remote is queried to determine its `HEAD`, then\n+`$GIT_DIR/remotes//HEAD` is set to the same branch.\n++\n+Use `` to set `$GIT_DIR/remotes//HEAD` explicitly.\n+\n 'show'::\n \n Gives some information about the remote .\ndiff --git a/builtin-remote.c b/builtin-remote.c\nindex 0be8bfd..0715685 100644\n--- a/builtin-remote.c\n+++ b/builtin-remote.c\n@@ -12,6 +12,7 @@ static const char * const builtin_remote_usage[] = {\n \t\"git remote add [-t ] [-m ] [-f] [--mirror] \",\n \t\"git remote rename \",\n \t\"git remote rm \",\n+\t\"git remote sethead [-a | -d | ]\",\n \t\"git remote show [-n] \",\n \t\"git remote prune [-n | --dry-run] \",\n \t\"git remote [-v | --verbose] update [group]\",\n@@ -791,6 +792,54 @@ static int show(int argc, const char **argv)\n \treturn result;\n }\n \n+static int sethead(int argc, const char **argv)\n+{\n+\tint opt_a = 0, opt_d = 0, result = 0;\n+\tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;\n+\tchar *head_name = NULL;\n+\n+\tstruct option options[] = {\n+\t\tOPT_GROUP(\"sethead specific options\"),\n+\t\tOPT_BOOLEAN('a', 0, &opt_a,\n+\t\t \"set refs/remotes//HEAD according to remote\"),\n+\t\tOPT_BOOLEAN('d', 0, &opt_d, \"delete refs/remotes//HEAD\"),\n+\t\tOPT_END()\n+\t};\n+\targc = parse_options(argc, argv, options, builtin_remote_usage, 0);\n+\tif ((argc == 1 && !(opt_a || opt_d)) ||\n+\t ((argc == 2 && (opt_a || opt_d))) || argc < 1 || argc > 2)\n+\t\tusage_with_options(builtin_remote_usage, options);\n+\n+\tstrbuf_addf(&buf, \"refs/remotes/%s/HEAD\", argv[0]);\n+\n+\tif (opt_d) {\n+\t\tif (result |= delete_ref(buf.buf, NULL, REF_NODEREF))\n+\t\t\terror(\"Could not delete %s\", buf.buf);\n+\t} else if (opt_a) {\n+\t\tstruct ref_states states;\n+\t\tmemset(&states, 0, sizeof(states));\n+\t\tget_remote_ref_states(argv[0], &states, 1);\n+\t\thead_name = xstrdup(states.head_name);\n+\t\tfree_remote_ref_states(&states);\n+\t} else\n+\t\thead_name = xstrdup(argv[1]);\n+\n+\tif (head_name) {\n+\t\tunsigned char sha1[20];\n+\t\tstrbuf_addf(&buf2, \"refs/remotes/%s/%s\", argv[0], head_name);\n+\t\t/* make sure it's valid */\n+\t\tif (!resolve_ref(buf2.buf, sha1, 1, NULL))\n+\t\t\tresult |= error(\"Not a valid ref: %s\", buf2.buf);\n+\t\telse if (create_symref(buf.buf, buf2.buf, \"remote sethead\"))\n+\t\t\tresult |= error(\"Could not setup %s\", buf.buf);\n+\t\tfree(head_name);\n+\t}\n+\n+\tstrbuf_release(&buf);\n+\tstrbuf_release(&buf2);\n+\treturn result;\n+}\n+\n static int prune(int argc, const char **argv)\n {\n \tint dry_run = 0, result = 0;\n@@ -955,6 +1004,8 @@ int cmd_remote(int argc, const char **argv, const char *prefix)\n \t\tresult = mv(argc, argv);\n \telse if (!strcmp(argv[0], \"rm\"))\n \t\tresult = rm(argc, argv);\n+\telse if (!strcmp(argv[0], \"sethead\"))\n+\t\tresult = sethead(argc, argv);\n \telse if (!strcmp(argv[0], \"show\"))\n \t\tresult = show(argc, argv);\n \telse if (!strcmp(argv[0], \"prune\"))\n-- \n1.6.2.rc0.187.g9fcfb\n"},{"id":"104316","messageId":"20090212001717.GA30231@coredump.intra.peff.net","threadId":"17715","inReplyTo":"1234332083-45147-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH 0/3] remote HEAD improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-12T00:17:17Z","receivedAt":"2009-02-12T00:17:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 11, 2009 at 01:01:20AM -0500, Jay Soffian wrote:\n\n> 1) Teaches git remote a new \"sethead\" verb:\n> \n> To set a remote HEAD explicitly:\n> $ git remote sethead \n> \n> To set a remote HEAD to match the upstream repo:\n> $ git remote sethead -a\n> \n> To delete a remote HEAD:\n> $ git remote sethead -d\n\nI like these semantics a lot. I will do worse than bikeshed, though, and\nsay that I don't like the color you painted it without even proposing a\ncolor of my own. Which is to say, I think \"sethead\" is not the best\nname. But I like it better than the alternatives I've seen, so maybe it\nis OK.\n\nPerhaps it is just the run-together word that makes it worse. Something\nlike \"set-head\" might be better (I guess I couldn't resist suggesting a\ncolor, after all).\n\nI have a few comments, which I will post in reply to the individual\npatches.\n\n-Peff\n"},{"id":"104317","messageId":"20090212001836.GB30231@coredump.intra.peff.net","threadId":"17715","inReplyTo":"1234332083-45147-2-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH 1/3] builtin-remote: move duplicated cleanup code its own function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-12T00:18:36Z","receivedAt":"2009-02-12T00:18:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 11, 2009 at 01:01:21AM -0500, Jay Soffian wrote:\n\n> +static void free_remote_ref_states(struct ref_states *states)\n> +{\n> +\t/* NEEDSWORK: free remote */\n> +\tstring_list_clear(&states->new, 0);\n> +\tstring_list_clear(&states->stale, 0);\n> +\tstring_list_clear(&states->tracked, 0);\n> +}\n\nHmm. I don't know anything about this code, so maybe it is not trivial.\nBut anytime you are touching an area that NEEDSWORK, I think it is worth\nlooking at whether you can fix that problem (since you have already\nspent a few brain cycles understanding what is going on in general).\n\n-Peff\n"},{"id":"104318","messageId":"20090212002612.GC30231@coredump.intra.peff.net","threadId":"17715","inReplyTo":"1234332083-45147-3-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH 2/3] builtin-remote: teach show to display remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-12T00:26:12Z","receivedAt":"2009-02-12T00:26:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 11, 2009 at 01:01:22AM -0500, Jay Soffian wrote:\n\n> +static char *get_head_name(const struct ref *ref)\n> +{\n> +\tconst struct ref *remote_head = NULL;\n> +\tconst struct ref *remote_master = NULL;\n> +\tconst struct ref *r;\n> +\tfor (r = ref; r; r = r->next) {\n> +\t\tif (!strcmp(r->name, \"HEAD\"))\n> +\t\t\tremote_head = r;\n> +\t\tif (!strcmp(r->name, \"refs/heads/master\"))\n> +\t\t\tremote_master = r;\n> +\t}\n> +\n> +\t/* If there's no HEAD value at all, never mind. */\n> +\tif (!remote_head)\n> +\t\treturn NULL;\n> +\n> +\t/* If refs/heads/master could be right, it is. */\n> +\tif (remote_master && !hashcmp(remote_master->old_sha1,\n> +\t\t\t\t remote_head->old_sha1))\n> +\t\treturn xstrdup(abbrev_branch(remote_master->name));\n> +\n> +\t/* Look for another ref that points there */\n> +\tfor (r = ref; r; r = r->next)\n> +\t\tif (r != remote_head &&\n> +\t\t !hashcmp(r->old_sha1, remote_head->old_sha1) &&\n> +\t\t !prefixcmp(r->name, \"refs/heads/\"))\n> +\t\t\treturn xstrdup(abbrev_branch(r->name));\n> +\n> +\t/* Nothing is the same */\n> +\treturn NULL;\n> +}\n\nYuck. Cut and paste from builtin-clone.c. It's not so much the number of\nlines here (although of course I don't like that, either) but that this\nfunction encompasses a heuristic for matching the HEAD. Which means it\nmay change in the future, and I really don't want the clone behavior and\nthe remote behavior to diverge.\n\nSo can we refactor it into a library function?\n\nI see that the inputs and outputs aren't exactly the same in both cases,\nbut I think you could do it like:\n\n struct ref *guess_head_ref(const struct ref *refs_with_head,\n const struct ref *refs_that_might_match_head,\n struct ref **remote_head_p);\n\nand then just call:\n\n r = guess_head_ref(refs, refs, NULL);\n states->head_name = r ? xstrdup(abbrev_branch(r->name)) : NULL;\n\nfrom git-remote, which at least keeps the changeable parts all\ncontained.\n\n-Peff\n"},{"id":"104323","messageId":"76718490902111744p27e83238x34cb7004d8e3e48f@mail.gmail.com","threadId":"17715","inReplyTo":"20090212001836.GB30231@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] builtin-remote: move duplicated cleanup code its own function","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-12T01:44:13Z","receivedAt":"2009-02-12T01:44:13Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Feb 11, 2009 at 7:18 PM, Jeff King wrote:\n> On Wed, Feb 11, 2009 at 01:01:21AM -0500, Jay Soffian wrote:\n>\n>> +static void free_remote_ref_states(struct ref_states *states)\n>> +{\n>> + /* NEEDSWORK: free remote */\n>> + string_list_clear(&states->new, 0);\n>> + string_list_clear(&states->stale, 0);\n>> + string_list_clear(&states->tracked, 0);\n>> +}\n>\n> Hmm. I don't know anything about this code, so maybe it is not trivial.\n> But anytime you are touching an area that NEEDSWORK, I think it is worth\n> looking at whether you can fix that problem (since you have already\n> spent a few brain cycles understanding what is going on in general).\n\nI spent about 5 minutes which was enough time for me to realize that\nthe reason the previous author left it as \"NEEDSWORK\" is because\nfixing it is more than 5 minutes of work. This is the remote object --\nmaybe you could offer me some clues that allow me to know which of its\nfields need to be freed individually:\n\nstruct remote {\n\tconst char *name;\n\tint origin;\n\n\tconst char **url;\n\tint url_nr;\n\tint url_alloc;\n\n\tconst char **push_refspec;\n\tstruct refspec *push;\n\tint push_refspec_nr;\n\tint push_refspec_alloc;\n\n\tconst char **fetch_refspec;\n\tstruct refspec *fetch;\n\tint fetch_refspec_nr;\n\tint fetch_refspec_alloc;\n\n\t/*\n\t * -1 to never fetch tags\n\t * 0 to auto-follow tags on heuristic (default)\n\t * 1 to always auto-follow tags\n\t * 2 to always fetch tags\n\t */\n\tint fetch_tags;\n\tint skip_default_update;\n\tint mirror;\n\n\tconst char *receivepack;\n\tconst char *uploadpack;\n\n\t/*\n\t * for curl remotes only\n\t */\n\tchar *http_proxy;\n};\n\nI *think* const is a clue that the field need not be freed, because\nthe pointer is to storage that is on the stack. But I wasn't sure, esp\nwith the double pointers. And I really wasn't sure about the struct\npointers.\n\nReally, I only pretend to know C. :-)\n\nj.\n"},{"id":"104324","messageId":"76718490902111748j58f80591ma149f8ec9fb8b352@mail.gmail.com","threadId":"17715","inReplyTo":"20090212002612.GC30231@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] builtin-remote: teach show to display remote HEAD","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-12T01:48:42Z","receivedAt":"2009-02-12T01:48:42Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Feb 11, 2009 at 7:26 PM, Jeff King wrote:\n\n> Yuck.\n\nDamn, I knew I wasn't going to slip that one by. :-)\n\n> I see that the inputs and outputs aren't exactly the same in both cases,\n\nWhich is why I didn't refactor it. The extra code needed to massage\nwhat builtin-remote.c has to the existing function in builtin-clone.c\nwould've been more LOC than the duplicate code (I think...).\n\nBUT\n\nI'll try. :-)\n\nj.\n"},{"id":"104325","messageId":"20090212015007.GA3187@sigill.intra.peff.net","threadId":"17715","inReplyTo":"76718490902111744p27e83238x34cb7004d8e3e48f@mail.gmail.com","subject":"Re: [PATCH 1/3] builtin-remote: move duplicated cleanup code its own function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-12T01:50:07Z","receivedAt":"2009-02-12T01:50:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 11, 2009 at 08:44:13PM -0500, Jay Soffian wrote:\n\n> I spent about 5 minutes which was enough time for me to realize that\n> the reason the previous author left it as \"NEEDSWORK\" is because\n> fixing it is more than 5 minutes of work. This is the remote object --\n> maybe you could offer me some clues that allow me to know which of its\n> fields need to be freed individually:\n> [...]\n> I *think* const is a clue that the field need not be freed, because\n> the pointer is to storage that is on the stack. But I wasn't sure, esp\n> with the double pointers. And I really wasn't sure about the struct\n> pointers.\n\nOK, I am satisfied that it is not trivial, and doesn't need to be part\nof this patch series. :)\n\nLike I said, I don't actually know this corner of the code very well,\nbut since you hadn't mentioned it in your cover letter, I didn't know if\nit was \"too lazy to do cleanups\" or \"code is too scary to be cleaned\nup\".\n\n-Peff\n"},{"id":"104326","messageId":"20090212015559.GB3187@sigill.intra.peff.net","threadId":"17715","inReplyTo":"76718490902111748j58f80591ma149f8ec9fb8b352@mail.gmail.com","subject":"Re: [PATCH 2/3] builtin-remote: teach show to display remote HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-12T01:56:00Z","receivedAt":"2009-02-12T01:56:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 11, 2009 at 08:48:42PM -0500, Jay Soffian wrote:\n\n> Damn, I knew I wasn't going to slip that one by. :-)\n\n\"Given enough eyeballs, all ugly hacks are shallow.\" :)\n\n> > I see that the inputs and outputs aren't exactly the same in both cases,\n> \n> Which is why I didn't refactor it. The extra code needed to massage\n> what builtin-remote.c has to the existing function in builtin-clone.c\n> would've been more LOC than the duplicate code (I think...).\n> \n> BUT\n> \n> I'll try. :-)\n\nSee what you can do. But personally, I am not as concerned with reducing\nLOC as I am with encapsulating system logic like \"this is how you guess\nwhich ref is HEAD\". And if you can do both, great. :)\n\n-Peff\n"},{"id":"104406","messageId":"alpine.LNX.1.00.0902121458450.19665@iabervon.org","threadId":"17715","inReplyTo":"76718490902111744p27e83238x34cb7004d8e3e48f@mail.gmail.com","subject":"Re: [PATCH 1/3] builtin-remote: move duplicated cleanup code its own function","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-02-12T20:13:31Z","receivedAt":"2009-02-12T20:13:31Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 11 Feb 2009, Jay Soffian wrote:\n\n> On Wed, Feb 11, 2009 at 7:18 PM, Jeff King wrote:\n> > On Wed, Feb 11, 2009 at 01:01:21AM -0500, Jay Soffian wrote:\n> >\n> >> +static void free_remote_ref_states(struct ref_states *states)\n> >> +{\n> >> + /* NEEDSWORK: free remote */\n> >> + string_list_clear(&states->new, 0);\n> >> + string_list_clear(&states->stale, 0);\n> >> + string_list_clear(&states->tracked, 0);\n> >> +}\n> >\n> > Hmm. I don't know anything about this code, so maybe it is not trivial.\n> > But anytime you are touching an area that NEEDSWORK, I think it is worth\n> > looking at whether you can fix that problem (since you have already\n> > spent a few brain cycles understanding what is going on in general).\n> \n> I spent about 5 minutes which was enough time for me to realize that\n> the reason the previous author left it as \"NEEDSWORK\" is because\n> fixing it is more than 5 minutes of work. This is the remote object --\n> maybe you could offer me some clues that allow me to know which of its\n> fields need to be freed individually:\n> \n> struct remote {\n> \tconst char *name;\n> \tint origin;\n> \n> \tconst char **url;\n> \tint url_nr;\n> \tint url_alloc;\n> \n> \tconst char **push_refspec;\n> \tstruct refspec *push;\n> \tint push_refspec_nr;\n> \tint push_refspec_alloc;\n> \n> \tconst char **fetch_refspec;\n> \tstruct refspec *fetch;\n> \tint fetch_refspec_nr;\n> \tint fetch_refspec_alloc;\n> \n> \t/*\n> \t * -1 to never fetch tags\n> \t * 0 to auto-follow tags on heuristic (default)\n> \t * 1 to always auto-follow tags\n> \t * 2 to always fetch tags\n> \t */\n> \tint fetch_tags;\n> \tint skip_default_update;\n> \tint mirror;\n> \n> \tconst char *receivepack;\n> \tconst char *uploadpack;\n> \n> \t/*\n> \t * for curl remotes only\n> \t */\n> \tchar *http_proxy;\n> };\n> \n> I *think* const is a clue that the field need not be freed, because\n> the pointer is to storage that is on the stack. But I wasn't sure, esp\n> with the double pointers. And I really wasn't sure about the struct\n> pointers.\n\nActually, the comment is wrong; \"remote\" comes from remote_get(), which \nreturns things from a cache in remote.c; there could be a remote_put() to \nlet the code know that the caller is done with the object, but it wouldn't \npresently do anything.\n\n(The code actually reads the config files once, generating info for all of \nthe configured remotes, and just returns them, except that it will \ngenerate a new object for unconfigured, individually requested URLs)\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"104409","messageId":"alpine.LNX.1.00.0902121519160.19665@iabervon.org","threadId":"17715","inReplyTo":"76718490902111748j58f80591ma149f8ec9fb8b352@mail.gmail.com","subject":"Re: [PATCH 2/3] builtin-remote: teach show to display remote HEAD","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-02-12T20:27:13Z","receivedAt":"2009-02-12T20:27:13Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 11 Feb 2009, Jay Soffian wrote:\n\n> On Wed, Feb 11, 2009 at 7:26 PM, Jeff King wrote:\n> \n> > Yuck.\n> \n> Damn, I knew I wasn't going to slip that one by. :-)\n> \n> > I see that the inputs and outputs aren't exactly the same in both cases,\n> \n> Which is why I didn't refactor it. The extra code needed to massage\n> what builtin-remote.c has to the existing function in builtin-clone.c\n> would've been more LOC than the duplicate code (I think...).\n\nIsn't it just:\n\nstruct ref *head = locate_head(refs, refs, NULL);\nreturn head ? xstrdup(abbrev_branch(head->name)) : NULL;\n\n?\n\nI'd somehow thought I'd moved locate_head() somewhere common, but it \nreally ought to be done. There were periodic discussions of how you find \nout when the remote repo changes its HEAD and you might want to do \nsomething locally about it, and we never came up with a specific thing \nfor git to do, but the facility is probably useful.\n\nI have the vague memory, as well, that there's some way for a transport to \nreport that it actually knows that HEAD is a symref to something in \nparticular, and so git shouldn't guess.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"104429","messageId":"7vfxijjqlb.fsf@gitster.siamese.dyndns.org","threadId":"17715","inReplyTo":"alpine.LNX.1.00.0902121519160.19665@iabervon.org","subject":"Re: [PATCH 2/3] builtin-remote: teach show to display remote HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-12T21:24:48Z","receivedAt":"2009-02-12T21:24:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n> I have the vague memory, as well, that there's some way for a transport to \n> report that it actually knows that HEAD is a symref to something in \n> particular, and so git shouldn't guess.\n\nI think you are thinking about:\n\n http://thread.gmane.org/gmane.comp.version-control.git/102039\n"},{"id":"104430","messageId":"alpine.LNX.1.00.0902121627520.19665@iabervon.org","threadId":"17715","inReplyTo":"7vfxijjqlb.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] builtin-remote: teach show to display remote HEAD","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-02-12T21:34:01Z","receivedAt":"2009-02-12T21:34:01Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 12 Feb 2009, Junio C Hamano wrote:\n\n> Daniel Barkalow writes:\n> \n> > I have the vague memory, as well, that there's some way for a transport to \n> > report that it actually knows that HEAD is a symref to something in \n> > particular, and so git shouldn't guess.\n> \n> I think you are thinking about:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/102039\n\nActually, I think I missed that thread entirely; I was thinking of earlier \nwork to allow the http transport, when it finds that HEAD is a symref \nfile, to report that there's a ref named \"HEAD\", with a particular value, \nand that it's incidentally known to be a symref to another particular ref.\n\nEnded up in be885d96fe0ebed47e637f3b0dd24fc5902f7081, but I don't think \nanything was set up to actually do anything with the information.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"104432","messageId":"76718490902121337l4b7f6f1dh25e6daa9adb48160@mail.gmail.com","threadId":"17715","inReplyTo":"alpine.LNX.1.00.0902121519160.19665@iabervon.org","subject":"Re: [PATCH 2/3] builtin-remote: teach show to display remote HEAD","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-02-12T21:37:27Z","receivedAt":"2009-02-12T21:37:27Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Feb 12, 2009 at 3:27 PM, Daniel Barkalow wrote:\n>\n> struct ref *head = locate_head(refs, refs, NULL);\n> return head ? xstrdup(abbrev_branch(head->name)) : NULL;\n>\n> ?\n\nNo, I don't _think_ so. refs is everything from the remote side (tags,\netc). I want to only match those things under refs/heads.\n\nI think I have to do something like this (this is more or less what\nbuiltin-clone does):\n\nstruct ref *remote_refs, *mapped_refs = NULL;\nstruct refspec branch_refspec;\n\nbranch_refspec.force = 0;\nbranch_refspec.src = branch_refspec.dst = \"refs/heads\";\n\nremote_refs = transport_get_remote_refs(transport);\nget_fetch_map(remote_refs, branch_refspec, &mapped_refs, 0);\nhead_points_at = locate_head(refs, mapped_refs, NULL);\n\n> I'd somehow thought I'd moved locate_head() somewhere common, but it\n> really ought to be done.\n\nI plan to move it into remote.c.\n\n> There were periodic discussions of how you find\n> out when the remote repo changes its HEAD and you might want to do\n> something locally about it, and we never came up with a specific thing\n> for git to do, but the facility is probably useful.\n\nThus \"git remote set-head -a\" is the best I could come up with for\nsetting it to what the remote has.\n\n> I have the vague memory, as well, that there's some way for a transport to\n> report that it actually knows that HEAD is a symref to something in\n> particular, and so git shouldn't guess.\n\nI think for http://, but not for git://, but I'm *far* from an expert\nin this area.\n\nj.\n"},{"id":"104443","messageId":"alpine.LNX.1.00.0902121656570.19665@iabervon.org","threadId":"17715","inReplyTo":"76718490902121337l4b7f6f1dh25e6daa9adb48160@mail.gmail.com","subject":"Re: [PATCH 2/3] builtin-remote: teach show to display remote HEAD","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-02-12T22:55:04Z","receivedAt":"2009-02-12T22:55:04Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 12 Feb 2009, Jay Soffian wrote:\n\n> On Thu, Feb 12, 2009 at 3:27 PM, Daniel Barkalow wrote:\n> >\n> > struct ref *head = locate_head(refs, refs, NULL);\n> > return head ? xstrdup(abbrev_branch(head->name)) : NULL;\n> >\n> > ?\n> \n> No, I don't _think_ so. refs is everything from the remote side (tags,\n> etc). I want to only match those things under refs/heads.\n\nI was looking at your code and comparing with locate_head; it looks to me \nlike you're using \"ref\" for both of the lists of refs that locate_head() \ngets. I think, actually, that checking for refs/heads in locate_head() \nwould be a reasonable thing in general, and might avoid getting surprising \nresults in clone with --mirror and odd refs or such.\n\n> I think I have to do something like this (this is more or less what\n> builtin-clone does):\n> \n> struct ref *remote_refs, *mapped_refs = NULL;\n> struct refspec branch_refspec;\n> \n> branch_refspec.force = 0;\n> branch_refspec.src = branch_refspec.dst = \"refs/heads\";\n> \n> remote_refs = transport_get_remote_refs(transport);\n> get_fetch_map(remote_refs, branch_refspec, &mapped_refs, 0);\n> head_points_at = locate_head(refs, mapped_refs, NULL);\n\nYou should be able to filter the ref list you already have, rather than \nrefetching, if nothing else.\n\n> > I'd somehow thought I'd moved locate_head() somewhere common, but it\n> > really ought to be done.\n> \n> I plan to move it into remote.c.\n\nGood.\n\n> > There were periodic discussions of how you find\n> > out when the remote repo changes its HEAD and you might want to do\n> > something locally about it, and we never came up with a specific thing\n> > for git to do, but the facility is probably useful.\n> \n> Thus \"git remote set-head -a\" is the best I could come up with for\n> setting it to what the remote has.\n\nYeah, that seems like a good interface.\n\n> > I have the vague memory, as well, that there's some way for a transport to\n> > report that it actually knows that HEAD is a symref to something in\n> > particular, and so git shouldn't guess.\n> \n> I think for http://, but not for git://, but I'm *far* from an expert\n> in this area.\n\nYes, it was information only available for http, but there's no reason to \nassume that other protocols couldn't provide it, and Junio mentioned a \nseries from December (maybe never applied) to get the same info for the \nnative git protocol.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}