{"thread":{"id":"7882","subject":"[PATCH 2/5] Add remote functions","startedAt":"2007-04-28T17:05:09Z","lastAt":"2007-05-03T03:27:52Z","messageCount":3,"participants":["Daniel Barkalow","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"40659","messageId":"Pine.LNX.4.64.0704281303570.28708@iabervon.org","threadId":"7882","inReplyTo":null,"subject":"[PATCH 2/5] Add remote functions","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-04-28T17:05:09Z","receivedAt":"2007-04-28T17:05:09Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"Make a struct and a set of functions to handle the parsing of remote\nconfigurations (branches files, remotes files, and config sections),\nand do some simple operations on lists of refspecs in the struct.\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\n---\n Makefile |    4 +-\n remote.c |  241 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n remote.h |   25 +++++++\n 3 files changed, 268 insertions(+), 2 deletions(-)\n create mode 100644 remote.c\n create mode 100644 remote.h\n\ndiff --git a/Makefile b/Makefile\nindex 60c41fd..98916f7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -288,7 +288,7 @@ LIB_H = \\\n \tdiff.h object.h pack.h pkt-line.h quote.h refs.h list-objects.h sideband.h \\\n \trun-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \\\n \ttree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h \\\n-\tutf8.h reflog-walk.h patch-ids.h attr.h decorate.h progress.h\n+\tutf8.h reflog-walk.h patch-ids.h attr.h decorate.h progress.h remote.h\n \n DIFF_OBJS = \\\n \tdiff.o diff-lib.o diffcore-break.o diffcore-order.o \\\n@@ -310,7 +310,7 @@ LIB_OBJS = \\\n \twrite_or_die.o trace.o list-objects.o grep.o match-trees.o \\\n \talloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \\\n \tcolor.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \\\n-\tconvert.o attr.o decorate.o progress.o\n+\tconvert.o attr.o decorate.o progress.o remote.o\n \n BUILTIN_OBJS = \\\n \tbuiltin-add.o \\\ndiff --git a/remote.c b/remote.c\nnew file mode 100644\nindex 0000000..58e6a96\n--- /dev/null\n+++ b/remote.c\n@@ -0,0 +1,241 @@\n+#include \"cache.h\"\n+#include \"remote.h\"\n+\n+#define MAX_REMOTES 16\n+\n+static struct remote *remotes[MAX_REMOTES];\n+\n+#define BUF_SIZE (2084)\n+static char buffer[BUF_SIZE];\n+\n+static void add_push_refspec(struct remote *remote, const char *ref)\n+{\n+\tint nr = remote->push_refspec_nr + 1;\n+\tremote->push_refspec =\n+\t\txrealloc(remote->push_refspec, nr * sizeof(char *));\n+\tremote->push_refspec[nr-1] = ref;\n+\tremote->push_refspec_nr = nr;\n+}\n+\n+static void add_fetch_refspec(struct remote *remote, const char *ref)\n+{\n+\tint nr = remote->fetch_refspec_nr + 1;\n+\tremote->fetch_refspec =\n+\t\txrealloc(remote->fetch_refspec, nr * sizeof(char *));\n+\tremote->fetch_refspec[nr-1] = ref;\n+\tremote->fetch_refspec_nr = nr;\n+}\n+\n+static void add_uri(struct remote *remote, const char *uri)\n+{\n+\tint i;\n+\tfor (i = 0; i < MAX_REMOTE_URI; i++) {\n+\t\tif (!remote->uri[i]) {\n+\t\t\tremote->uri[i] = uri;\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\terror(\"ignoring excess uri\");\n+}\n+\n+static struct remote *make_remote(const char *name, int len)\n+{\n+\tint i, empty = -1;\n+\n+\tfor (i = 0; i < MAX_REMOTES; i++) {\n+\t\tif (!remotes[i]) {\n+\t\t\tif (empty < 0)\n+\t\t\t\tempty = i;\n+\t\t} else {\n+\t\t\tif (len ? !strncmp(name, remotes[i]->name, len) :\n+\t\t\t    !strcmp(name, remotes[i]->name))\n+\t\t\t\treturn remotes[i];\n+\t\t}\n+\t}\n+\n+\tif (empty < 0)\n+\t\tdie(\"Too many remotes\");\n+\tremotes[empty] = xcalloc(1, sizeof(struct remote));\n+\tif (len)\n+\t\tremotes[empty]->name = xstrndup(name, len);\n+\telse\n+\t\tremotes[empty]->name = xstrdup(name);\n+\treturn remotes[empty];\n+}\n+\n+static void read_remotes_file(struct remote *remote)\n+{\n+\tFILE *f = fopen(git_path(\"remotes/%s\", remote->name), \"r\");\n+\n+\tif (!f)\n+\t\treturn;\n+\twhile (fgets(buffer, BUF_SIZE, f)) {\n+\t\tint is_refspec;\n+\t\tchar *s, *p;\n+\n+\t\tif (!prefixcmp(buffer, \"URL:\")) {\n+\t\t\tis_refspec = 0;\n+\t\t\ts = buffer + 4;\n+\t\t} else if (!prefixcmp(buffer, \"Push:\")) {\n+\t\t\tis_refspec = 1;\n+\t\t\ts = buffer + 5;\n+\t\t} else if (!prefixcmp(buffer, \"Pull:\")) {\n+\t\t\tis_refspec = 2;\n+\t\t\ts = buffer + 5;\n+\t\t} else\n+\t\t\tcontinue;\n+\n+\t\twhile (isspace(*s))\n+\t\t\ts++;\n+\t\tif (!*s)\n+\t\t\tcontinue;\n+\n+\t\tp = s + strlen(s);\n+\t\twhile (isspace(p[-1]))\n+\t\t\t*--p = 0;\n+\n+\t\tswitch (is_refspec) {\n+\t\tcase 0:\n+\t\t\tadd_uri(remote, xstrdup(s));\n+\t\t\tbreak;\n+\t\tcase 1:\n+\t\t\tadd_push_refspec(remote, xstrdup(s));\n+\t\t\tbreak;\n+\t\tcase 2:\n+\t\t\tadd_fetch_refspec(remote, xstrdup(s));\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+}\n+\n+static void read_branches_file(struct remote *remote)\n+{\n+\tconst char *slash = strchr(remote->name, '/');\n+\tint n = slash ? slash - remote->name : 1000;\n+\tFILE *f = fopen(git_path(\"branches/%.*s\", n, remote->name), \"r\");\n+\tchar *s, *p;\n+\tint len;\n+\n+\tif (!f)\n+\t\treturn;\n+\ts = fgets(buffer, BUF_SIZE, f);\n+\tfclose(f);\n+\tif (!s)\n+\t\treturn;\n+\twhile (isspace(*s))\n+\t\ts++;\n+\tif (!*s)\n+\t\treturn;\n+\tp = s + strlen(s);\n+\twhile (isspace(p[-1]))\n+\t\t*--p = 0;\n+\tlen = p - s;\n+\tif (slash)\n+\t\tlen += strlen(slash);\n+\tp = xmalloc(len + 1);\n+\tstrcpy(p, s);\n+\tif (slash)\n+\t\tstrcat(p, slash);\n+\tremote->uri[0] = p;\n+}\n+\n+static const char *default_remote_name = NULL;\n+static const char *current_branch = NULL;\n+static int current_branch_len = 0;\n+\n+static int handle_config(const char *key, const char *value)\n+{\n+\tconst char *name;\n+\tconst char *subkey;\n+\tstruct remote *remote;\n+\tif (!prefixcmp(key, \"branch.\") && current_branch &&\n+\t    !strncmp(key + 7, current_branch, current_branch_len) &&\n+\t    !strcmp(key + 7 + current_branch_len, \".remote\")) {\n+\t\tdefault_remote_name = xstrdup(value);\n+\t}\n+\tif (prefixcmp(key,  \"remote.\"))\n+\t\treturn 0;\n+\tname = key + 7;\n+\tsubkey = strchr(name, '.');\n+\tif (!subkey)\n+\t\treturn error(\"Config with no key for remote %s\", name);\n+\tremote = make_remote(name, subkey - name);\n+\tif (!strcmp(subkey, \".url\")) {\n+\t\tadd_uri(remote, xstrdup(value));\n+\t} else if (!strcmp(subkey, \".push\")) {\n+\t\tadd_push_refspec(remote, xstrdup(value));\n+\t} else if (!strcmp(subkey, \".fetch\")) {\n+\t\tadd_fetch_refspec(remote, xstrdup(value));\n+\t} else if (!strcmp(subkey, \".receivepack\")) {\n+\t\tif (!remote->receivepack)\n+\t\t\tremote->receivepack = xstrdup(value);\n+\t\telse\n+\t\t\terror(\"more than one receivepack given, using the first\");\n+\t}\n+\treturn 0;\n+}\n+\n+static void read_config(void)\n+{\n+\tif (default_remote_name) // did this already\n+\t\treturn;\n+\tdefault_remote_name = xstrdup(\"origin\");\n+\tcurrent_branch = NULL;\n+\tgit_config(handle_config);\n+}\n+\n+struct remote *remote_get(const char *name)\n+{\n+\tstruct remote *ret;\n+\n+\tread_config();\n+\tif (!name)\n+\t\tname = default_remote_name;\n+\tret = make_remote(name, 0);\n+\tif (*name == '/')\n+\t\tret->uri[0] = name;\n+\tif (!ret->uri[0])\n+\t\tread_remotes_file(ret);\n+\tif (!ret->uri[0])\n+\t\tread_branches_file(ret);\n+\tif (!ret->uri[0])\n+\t\treturn NULL;\n+\treturn ret;\n+}\n+\n+int remote_has_uri(struct remote *remote, const char *uri)\n+{\n+\tint i;\n+\tfor (i = 0; i < MAX_REMOTE_URI; i++) {\n+\t\tif (remote->uri[i] && !strcmp(remote->uri[i], uri))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n+char *remote_fetch_to(struct remote *remote, const char *ref)\n+{\n+\tint i;\n+\tfor (i = 0; i < remote->fetch_refspec_nr; i++) {\n+\t\tconst char *refspec = remote->fetch_refspec[i];\n+\t\tconst char *cons = strchr(refspec, ':');\n+\t\tconst char *glob = strchr(refspec, '*');\n+\t\tif (*refspec == '+')\n+\t\t\trefspec++;\n+\t\tif (glob) {\n+\t\t\tif (!strncmp(ref, refspec, glob - refspec)) {\n+\t\t\t\tconst char *cons_glob = strchr(cons, '*');\n+\t\t\t\tchar *ret =\n+\t\t\t\t\txmalloc(cons_glob - cons + strlen(ref) - (glob - refspec));\n+\t\t\t\tmemcpy(ret, cons + 1, cons_glob - cons - 1);\n+\t\t\t\tmemcpy(ret + (cons_glob - cons) - 1,\n+\t\t\t\t       ref + (glob - refspec),\n+\t\t\t\t       strlen(ref) - (glob - refspec) + 1);\n+\t\t\t\treturn ret;\n+\t\t\t}\n+\t\t} else if (!strncmp(ref, refspec, cons - refspec)) {\n+\t\t\treturn xstrdup(cons + 1);\n+\t\t}\n+\t}\n+\treturn NULL;\n+}\ndiff --git a/remote.h b/remote.h\nnew file mode 100644\nindex 0000000..7e881f7\n--- /dev/null\n+++ b/remote.h\n@@ -0,0 +1,25 @@\n+#ifndef _REMOTE_H\n+#define _REMOTE_H\n+\n+#define MAX_REMOTE_URI 16\n+\n+struct remote {\n+\tconst char *name;\n+\tconst char *uri[MAX_REMOTE_URI];\n+\n+\tconst char **push_refspec;\n+\tint push_refspec_nr;\n+\n+\tconst char **fetch_refspec;\n+\tint fetch_refspec_nr;\n+\n+\tconst char *receivepack;\n+};\n+\n+struct remote *remote_get(const char *name);\n+\n+int remote_has_uri(struct remote *remote, const char *uri);\n+\n+char *remote_fetch_to(struct remote *remote, const char *ref);\n+\n+#endif\n-- \n1.5.1.2.255.g6ead4-dirty\n"},{"id":"40690","messageId":"7virbfg22c.fsf@assigned-by-dhcp.cox.net","threadId":"7882","inReplyTo":"Pine.LNX.4.64.0704281303570.28708@iabervon.org","subject":"Re: [PATCH 2/5] Add remote functions","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-29T05:52:11Z","receivedAt":"2007-04-29T05:52:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> Make a struct and a set of functions to handle the parsing of remote\n> configurations (branches files, remotes files, and config sections),\n> and do some simple operations on lists of refspecs in the struct.\n>\n> Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>\n\nI do not have any objection to making a generally useful library\nfunction to deal with branches/remotes/config, and this is\nprobably a good start.  Eventually we would want to do a built-in\ngit-remote command that supports interactive editing of remote\ninformation, not just fetch/push in C.\n\nBut I do not think I can apply this patch as is, even for 'pu'.\nComments below.\n\n> diff --git a/remote.c b/remote.c\n> new file mode 100644\n> index 0000000..58e6a96\n> --- /dev/null\n> +++ b/remote.c\n> @@ -0,0 +1,241 @@\n> +#include \"cache.h\"\n> +#include \"remote.h\"\n> +\n> +#define MAX_REMOTES 16\n> +\n> +static struct remote *remotes[MAX_REMOTES];\n\nI do not think the limitation of MAX_URI=16 in the original is\nunreasonable, but this MAX_REMOTES is way too low, if we want to\neventually support somebody like Andrew, who has to deal with a\nfew dozen git trees.\n\n> +static void add_uri(struct remote *remote, const char *uri)\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < MAX_REMOTE_URI; i++) {\n> +\t\tif (!remote->uri[i]) {\n> +\t\t\tremote->uri[i] = uri;\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\terror(\"ignoring excess uri\");\n> +}\n\nWhy not do this just like refspecs, as an array of URIs?  Doing\nso would also lift the MAX_REMOTE_URI limitation.  Although more\nthan one URIs per remote is not a norm but an exception (and\nmakes sense only for push), it would shrink the base structure,\nand probably make the code simpler.\n\n> +\n> +static struct remote *make_remote(const char *name, int len)\n> +{\n> +\tint i, empty = -1;\n> +\n> +\tfor (i = 0; i < MAX_REMOTES; i++) {\n> +\t\tif (!remotes[i]) {\n> +\t\t\tif (empty < 0)\n> +\t\t\t\tempty = i;\n> +\t\t} else {\n> +\t\t\tif (len ? !strncmp(name, remotes[i]->name, len) :\n> +\t\t\t    !strcmp(name, remotes[i]->name))\n> +\t\t\t\treturn remotes[i];\n> +\t\t}\n\nIf you have \"remote.foobar.url = /git/repo.git\", this is called\nwith \"foobar.url\" in name and 6 in len.  Do you want to match an\nexisting remote whose name is \"foobarbaz\"?\n\n> +static void read_remotes_file(struct remote *remote)\n> +{\n> +\tFILE *f = fopen(git_path(\"remotes/%s\", remote->name), \"r\");\n> +\n> +\tif (!f)\n> +\t\treturn;\n> +\twhile (fgets(buffer, BUF_SIZE, f)) {\n> +\t\tint is_refspec;\n\nThe original dealt with URL or refspec, so \"is_refspec\" made\nsome sense.  It isn't anymore as you differentiate between push\nand fetch refspecs.\n\n> +static void read_branches_file(struct remote *remote)\n> +{\n> +\tconst char *slash = strchr(remote->name, '/');\n> +\tint n = slash ? slash - remote->name : 1000;\n> +\tFILE *f = fopen(git_path(\"branches/%.*s\", n, remote->name), \"r\");\n> +\tchar *s, *p;\n> +\tint len;\n\nI know you inherited this from builtin-push.c, but can't we\nclean up this magic 1000 somehow, please?\n\n> +static const char *default_remote_name = NULL;\n> +static const char *current_branch = NULL;\n> +static int current_branch_len = 0;\n> +\n> +static int handle_config(const char *key, const char *value)\n> +{\n> +\tconst char *name;\n> +\tconst char *subkey;\n> +\tstruct remote *remote;\n> +\tif (!prefixcmp(key, \"branch.\") && current_branch &&\n> +\t    !strncmp(key + 7, current_branch, current_branch_len) &&\n> +\t    !strcmp(key + 7 + current_branch_len, \".remote\")) {\n> +\t\tdefault_remote_name = xstrdup(value);\n> +\t}\n\nWho sets current_branch to non-NULL value?  It is static and I\ndo not see anybody setting it in this file.\n\nAlso the original value of default_remote_name leaks here.  But\nyou cannot blindly free the original value, as you could end up\nstoring it in ret->uri[0] when somebody calls remote_get(NULL).\n\n> +\tif (prefixcmp(key,  \"remote.\"))\n> +\t\treturn 0;\n> +\tname = key + 7;\n> +\tsubkey = strchr(name, '.');\n> +\tif (!subkey)\n> +\t\treturn error(\"Config with no key for remote %s\", name);\n\nThis is not right.\n\n\t[section \"section name\"]\n                 variable = value\n\nallows dot in \"section name\" part, so you would want strrchr()\nto find out the end of it, not the first dot with strchr().  You\nfound an input error (missing subkey) if strrchr() finds the dot\nat the end of \"remote.\" (i.e. subkey == key + 6).\n\n> +char *remote_fetch_to(struct remote *remote, const char *ref)\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < remote->fetch_refspec_nr; i++) {\n> +\t\tconst char *refspec = remote->fetch_refspec[i];\n> +\t\tconst char *cons = strchr(refspec, ':');\n> +\t\tconst char *glob = strchr(refspec, '*');\n> +\t\tif (*refspec == '+')\n> +\t\t\trefspec++;\n\nI cannot offhand tell what this function is about...\n\nWould there be callers who are interested in finding out if\nrefspec is forcing or not?\n\nYou are not protecting yourself from refspecs without tracking\nbranch (i.e. sans any colon) and would segfault in the rest of\nthis function.\n\n> +\t\tif (glob) {\n> +\t\t\tif (!strncmp(ref, refspec, glob - refspec)) {\n> +\t\t\t\tconst char *cons_glob = strchr(cons, '*');\n> +\t\t\t\tchar *ret =\n> +\t\t\t\t\txmalloc(cons_glob - cons + strlen(ref) - (glob - refspec));\n> +\t\t\t\tmemcpy(ret, cons + 1, cons_glob - cons - 1);\n> +\t\t\t\tmemcpy(ret + (cons_glob - cons) - 1,\n> +\t\t\t\t       ref + (glob - refspec),\n> +\t\t\t\t       strlen(ref) - (glob - refspec) + 1);\n> +\t\t\t\treturn ret;\n> +\t\t\t}\n> +\t\t} else if (!strncmp(ref, refspec, cons - refspec)) {\n> +\t\t\treturn xstrdup(cons + 1);\n> +\t\t}\n> +\t}\n> +\treturn NULL;\n> +}\n\n> diff --git a/remote.h b/remote.h\n> new file mode 100644\n> index 0000000..7e881f7\n> --- /dev/null\n> +++ b/remote.h\n> @@ -0,0 +1,25 @@\n> +#ifndef _REMOTE_H\n> +#define _REMOTE_H\n\nWe do not seem to use leading underscore in most our header\nfiles.  Let's be consistent (we need to fix diffcore.h and\npath-list.h).\n"},{"id":"40933","messageId":"Pine.LNX.4.64.0705022303370.28708@iabervon.org","threadId":"7882","inReplyTo":"7virbfg22c.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/5] Add remote functions","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-05-03T03:27:52Z","receivedAt":"2007-05-03T03:27:52Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 28 Apr 2007, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > Make a struct and a set of functions to handle the parsing of remote\n> > configurations (branches files, remotes files, and config sections),\n> > and do some simple operations on lists of refspecs in the struct.\n> >\n> > Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>\n> \n> I do not have any objection to making a generally useful library\n> function to deal with branches/remotes/config, and this is\n> probably a good start.  Eventually we would want to do a built-in\n> git-remote command that supports interactive editing of remote\n> information, not just fetch/push in C.\n> \n> But I do not think I can apply this patch as is, even for 'pu'.\n> Comments below.\n> \n> > diff --git a/remote.c b/remote.c\n> > new file mode 100644\n> > index 0000000..58e6a96\n> > --- /dev/null\n> > +++ b/remote.c\n> > @@ -0,0 +1,241 @@\n> > +#include \"cache.h\"\n> > +#include \"remote.h\"\n> > +\n> > +#define MAX_REMOTES 16\n> > +\n> > +static struct remote *remotes[MAX_REMOTES];\n> \n> I do not think the limitation of MAX_URI=16 in the original is\n> unreasonable, but this MAX_REMOTES is way too low, if we want to\n> eventually support somebody like Andrew, who has to deal with a\n> few dozen git trees.\n\nSure; easy enough to fix.\n\n> > +static void add_uri(struct remote *remote, const char *uri)\n> > +{\n> > +\tint i;\n> > +\tfor (i = 0; i < MAX_REMOTE_URI; i++) {\n> > +\t\tif (!remote->uri[i]) {\n> > +\t\t\tremote->uri[i] = uri;\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\t}\n> > +\terror(\"ignoring excess uri\");\n> > +}\n> \n> Why not do this just like refspecs, as an array of URIs?  Doing\n> so would also lift the MAX_REMOTE_URI limitation.  Although more\n> than one URIs per remote is not a norm but an exception (and\n> makes sense only for push), it would shrink the base structure,\n> and probably make the code simpler.\n\nAgain easy enough. Actually, it might be nice to have some common \nfunctions for arrays of pointers, since I've now got 3 nearly identical \nfunctions, but not too important.\n\n> > +\n> > +static struct remote *make_remote(const char *name, int len)\n> > +{\n> > +\tint i, empty = -1;\n> > +\n> > +\tfor (i = 0; i < MAX_REMOTES; i++) {\n> > +\t\tif (!remotes[i]) {\n> > +\t\t\tif (empty < 0)\n> > +\t\t\t\tempty = i;\n> > +\t\t} else {\n> > +\t\t\tif (len ? !strncmp(name, remotes[i]->name, len) :\n> > +\t\t\t    !strcmp(name, remotes[i]->name))\n> > +\t\t\t\treturn remotes[i];\n> > +\t\t}\n> \n> If you have \"remote.foobar.url = /git/repo.git\", this is called\n> with \"foobar.url\" in name and 6 in len.  Do you want to match an\n> existing remote whose name is \"foobarbaz\"?\n\nYeah, I need to test !(existing remote)[len] also.\n\n> > +static void read_remotes_file(struct remote *remote)\n> > +{\n> > +\tFILE *f = fopen(git_path(\"remotes/%s\", remote->name), \"r\");\n> > +\n> > +\tif (!f)\n> > +\t\treturn;\n> > +\twhile (fgets(buffer, BUF_SIZE, f)) {\n> > +\t\tint is_refspec;\n> \n> The original dealt with URL or refspec, so \"is_refspec\" made\n> some sense.  It isn't anymore as you differentiate between push\n> and fetch refspecs.\n\n\"value_list\" instead.\n\n> > +static void read_branches_file(struct remote *remote)\n> > +{\n> > +\tconst char *slash = strchr(remote->name, '/');\n> > +\tint n = slash ? slash - remote->name : 1000;\n> > +\tFILE *f = fopen(git_path(\"branches/%.*s\", n, remote->name), \"r\");\n> > +\tchar *s, *p;\n> > +\tint len;\n> \n> I know you inherited this from builtin-push.c, but can't we\n> clean up this magic 1000 somehow, please?\n\nI'm a bit leary of touching this code; I don't have repositories with this \nsort of config to test against. Can I just move it and have someone how \nunderstands the code clean it up (before or after)?\n\n> > +static const char *default_remote_name = NULL;\n> > +static const char *current_branch = NULL;\n> > +static int current_branch_len = 0;\n> > +\n> > +static int handle_config(const char *key, const char *value)\n> > +{\n> > +\tconst char *name;\n> > +\tconst char *subkey;\n> > +\tstruct remote *remote;\n> > +\tif (!prefixcmp(key, \"branch.\") && current_branch &&\n> > +\t    !strncmp(key + 7, current_branch, current_branch_len) &&\n> > +\t    !strcmp(key + 7 + current_branch_len, \".remote\")) {\n> > +\t\tdefault_remote_name = xstrdup(value);\n> > +\t}\n> \n> Who sets current_branch to non-NULL value?  It is static and I\n> do not see anybody setting it in this file.\n\nI was going to set it when I figured out how to find the right value, but \nthen forgot. (This would also make \"git-push\" use the same remote as \n\"git-pull\" when the current branch has a \"remote\" setting, which is \nobviously less surprising, but is a change in behavior)\n\n> Also the original value of default_remote_name leaks here.  But\n> you cannot blindly free the original value, as you could end up\n> storing it in ret->uri[0] when somebody calls remote_get(NULL).\n\nActually, read_config() can only do anything once (since it returns with \ndefault_remote_name not null), so handle_config can't be called again \nafter remote_get() has potentially used it. So I can definitely free it.\n\n> > +\tif (prefixcmp(key,  \"remote.\"))\n> > +\t\treturn 0;\n> > +\tname = key + 7;\n> > +\tsubkey = strchr(name, '.');\n> > +\tif (!subkey)\n> > +\t\treturn error(\"Config with no key for remote %s\", name);\n> \n> This is not right.\n> \n> \t[section \"section name\"]\n>                  variable = value\n> \n> allows dot in \"section name\" part, so you would want strrchr()\n> to find out the end of it, not the first dot with strchr().  You\n> found an input error (missing subkey) if strrchr() finds the dot\n> at the end of \"remote.\" (i.e. subkey == key + 6).\n\nJust replacing strchr with strrchr does the right thing, right? (If I \nstart at name (== key + 7), it won't find that wrong dot.)\n\n> > +char *remote_fetch_to(struct remote *remote, const char *ref)\n> > +{\n> > +\tint i;\n> > +\tfor (i = 0; i < remote->fetch_refspec_nr; i++) {\n> > +\t\tconst char *refspec = remote->fetch_refspec[i];\n> > +\t\tconst char *cons = strchr(refspec, ':');\n> > +\t\tconst char *glob = strchr(refspec, '*');\n> > +\t\tif (*refspec == '+')\n> > +\t\t\trefspec++;\n> \n> I cannot offhand tell what this function is about...\n> \n> Would there be callers who are interested in finding out if\n> refspec is forcing or not?\n> \n> You are not protecting yourself from refspecs without tracking\n> branch (i.e. sans any colon) and would segfault in the rest of\n> this function.\n\nThis is to find the local tracking ref that a remote head would be fetched \nto, taking into account patterns and multiple items in the list. I should \nprobably actually be sucking this code out of builtin-fetch--tool and \nconnect, which I hadn't noticed handled this in C already.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}