{"thread":{"id":"37736","subject":"[PATCH v2 03/11] resolve_ref_unsafe(): reverse the logic of the symref conditional","startedAt":"2014-10-15T15:06:12Z","lastAt":"2014-10-16T23:53:09Z","messageCount":16,"participants":["Michael Haggerty","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":2,"patchTotal":11},"messages":[{"id":"250688","messageId":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":null,"subject":"[PATCH v2 00/11] Consolidate ref parsing code","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:12Z","receivedAt":"2014-10-15T15:06:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is a rif on Duy's oldish patch series [1]. I started reviewing\nhis patch series, but found that some of his patches did multiple\nthings, making it harder to review. I started pulling it apart into\nsmaller changes to aid my review, and I guess I got carried away :-/\n\nAs far as I know, Duy isn't actively working on this, so I hope my\nreroll is not unwelcome.\n\nAs far as I can tell, Duy's patch series was correct aside from a\ncouple of minor cosmetic blemishes [2]. So if you want to accept Duy's\noriginal patch series, I hereby endorse it.\n\nThis version does the following things beyond Duy's original:\n\n* Split commits up into smaller pieces.\n\n* Get rid of the MAXREFLEN limitation.\n\n* Rename the parse_ref() parameter from \"ref\" to \"refname\".\n\n* Inline resolve_gitlink_packed_ref() and handle_missing_loose_ref().\n\n* Invert the \"if\" statement for dealing with symbolic references in\n parse_ref() to make the logic flow more linear.\n\n* Change a couple of \"while\" loops to \"do..while\".\n\n* Change resolve_refdup() to return strbuf_detach(&buf, NULL) instead\n of buf.buf directly (as suggested by Eric Sunshine).\n\nI retained Duy as author on commits that are derived straightforwardly\nfrom his. I hope I haven't broken any of them. I am calling this patch\nseries v2 because I propose it as a successor to Duy's version.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/254203/focus=254203\n\n[2] Aside from the couple of things pointed out on the mailing list,\n\n * The parse_ref() parameter should be named \"refname\" instead of\n \"ref\", for consistency with other refs code.\n\n * The local variable \"ref = result->buf\" in resolve_ref() just\n obscures things and should be inlined.\n\nMichael Haggerty (5):\n resolve_ref_unsafe(): reverse the logic of the symref conditional\n handle_missing_loose_ref(): inline function\n resolve_gitlink_ref_recursive(): drop arbitrary refname length limit\n resove_gitlink_packed_ref(): inline function\n resolve_gitlink_ref(): remove redundant test\n\nNguyễn Thái Ngọc Duy (6):\n strbuf_read_file(): preserve errno on failure\n handle_missing_loose_ref(): return an int\n resolve_ref_unsafe(): use skip_prefix() to skip over \"ref:\"\n refs.c: refactor resolve_ref_unsafe() to use strbuf internally\n refs.c: move ref parsing code out of resolve_ref()\n refs.c: rewrite resolve_gitlink_ref() to use parse_ref()\n\n cache.h | 12 +++\n refs.c | 371 +++++++++++++++++++++++++++++++--------------------------------\n strbuf.c | 7 +-\n 3 files changed, 200 insertions(+), 190 deletions(-)\n\n-- \n2.1.1\n"},{"id":"250683","messageId":"1413385583-4872-2-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 01/11] strbuf_read_file(): preserve errno on failure","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:13Z","receivedAt":"2014-10-15T15:06:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nThis will allow the function to be used in a later patch.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Michael Haggerty \n---\n strbuf.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 0346e74..f1fec58 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -482,15 +482,18 @@ int strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term)\n \n int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)\n {\n-\tint fd, len;\n+\tint fd, len, saved_errno;\n \n \tfd = open(path, O_RDONLY);\n \tif (fd < 0)\n \t\treturn -1;\n \tlen = strbuf_read(sb, fd, hint);\n+\tsaved_errno = errno;\n \tclose(fd);\n-\tif (len < 0)\n+\tif (len < 0) {\n+\t\terrno = saved_errno;\n \t\treturn -1;\n+\t}\n \n \treturn len;\n }\n-- \n2.1.1\n"},{"id":"250680","messageId":"1413385583-4872-3-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 02/11] handle_missing_loose_ref(): return an int","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:14Z","receivedAt":"2014-10-15T15:06:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nThe return value of handle_missing_loose_ref() was either NULL to\nsignify an error or the input parameter refname on success. So instead\nof returning a string, just return a 0 on success or -1 on error, so\nthe reader doesn't have to wonder what string the return value points\nat.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 24 +++++++++++++-----------\n 1 file changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ffd45e9..ceba23c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1370,10 +1370,10 @@ static struct ref_entry *get_packed_ref(const char *refname)\n * A loose ref file doesn't exist; check for a packed ref. The\n * options are forwarded from resolve_safe_unsafe().\n */\n-static const char *handle_missing_loose_ref(const char *refname,\n-\t\t\t\t\t unsigned char *sha1,\n-\t\t\t\t\t int reading,\n-\t\t\t\t\t int *flag)\n+static int handle_missing_loose_ref(const char *refname,\n+\t\t\t\t unsigned char *sha1,\n+\t\t\t\t int reading,\n+\t\t\t\t int *flag)\n {\n \tstruct ref_entry *entry;\n \n@@ -1386,14 +1386,14 @@ static const char *handle_missing_loose_ref(const char *refname,\n \t\thashcpy(sha1, entry->u.value.sha1);\n \t\tif (flag)\n \t\t\t*flag |= REF_ISPACKED;\n-\t\treturn refname;\n+\t\treturn 0;\n \t}\n \t/* The reference is not a packed reference, either. */\n \tif (reading) {\n-\t\treturn NULL;\n+\t\treturn -1;\n \t} else {\n \t\thashclr(sha1);\n-\t\treturn refname;\n+\t\treturn 0;\n \t}\n }\n \n@@ -1437,10 +1437,12 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t */\n \tstat_ref:\n \t\tif (lstat(path, &st) < 0) {\n-\t\t\tif (errno == ENOENT)\n-\t\t\t\treturn handle_missing_loose_ref(refname, sha1,\n-\t\t\t\t\t\t\t\treading, flag);\n-\t\t\telse\n+\t\t\tif (errno == ENOENT) {\n+\t\t\t\tif (handle_missing_loose_ref(refname, sha1,\n+\t\t\t\t\t\t\t reading, flag))\n+\t\t\t\t\treturn NULL;\n+\t\t\t\treturn refname;\n+\t\t\t} else\n \t\t\t\treturn NULL;\n \t\t}\n \n-- \n2.1.1\n"},{"id":"250679","messageId":"1413385583-4872-4-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 03/11] resolve_ref_unsafe(): reverse the logic of the symref conditional","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:15Z","receivedAt":"2014-10-15T15:06:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is clearer if the check whether a loose reference file is a symref\nis followed immediately by the code to handle the symref, rather than\nthe current code, which has the if statement the other way around.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 36 ++++++++++++++++++------------------\n 1 file changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ceba23c..771941b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1497,35 +1497,35 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t\tlen--;\n \t\tbuffer[len] = '\\0';\n \n-\t\t/*\n-\t\t * Is it a symbolic ref?\n-\t\t */\n-\t\tif (!starts_with(buffer, \"ref:\")) {\n-\t\t\t/*\n-\t\t\t * Please note that FETCH_HEAD has a second\n-\t\t\t * line containing other data.\n-\t\t\t */\n-\t\t\tif (get_sha1_hex(buffer, sha1) ||\n-\t\t\t (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n+\t\tif (starts_with(buffer, \"ref:\")) {\n+\t\t\t/* It is a symbolic ref */\n+\t\t\tif (flag)\n+\t\t\t\t*flag |= REF_ISSYMREF;\n+\t\t\tbuf = buffer + 4;\n+\t\t\twhile (isspace(*buf))\n+\t\t\t\tbuf++;\n+\t\t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n \t\t\t\tif (flag)\n \t\t\t\t\t*flag |= REF_ISBROKEN;\n \t\t\t\terrno = EINVAL;\n \t\t\t\treturn NULL;\n \t\t\t}\n-\t\t\treturn refname;\n+\t\t\trefname = strcpy(refname_buffer, buf);\n+\t\t\tcontinue;\n \t\t}\n-\t\tif (flag)\n-\t\t\t*flag |= REF_ISSYMREF;\n-\t\tbuf = buffer + 4;\n-\t\twhile (isspace(*buf))\n-\t\t\tbuf++;\n-\t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n+\n+\t\t/*\n+\t\t * It must be a normal ref. Please note that\n+\t\t * FETCH_HEAD has a second line containing other data.\n+\t\t */\n+\t\tif (get_sha1_hex(buffer, sha1) ||\n+\t\t (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n \t\t\tif (flag)\n \t\t\t\t*flag |= REF_ISBROKEN;\n \t\t\terrno = EINVAL;\n \t\t\treturn NULL;\n \t\t}\n-\t\trefname = strcpy(refname_buffer, buf);\n+\t\treturn refname;\n \t}\n }\n \n-- \n2.1.1\n"},{"id":"250690","messageId":"1413385583-4872-5-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 04/11] resolve_ref_unsafe(): use skip_prefix() to skip over \"ref:\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:16Z","receivedAt":"2014-10-15T15:06:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nThis requires buf to be declared (const char *), which is how it is\nused anyway.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 771941b..020ee3f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1416,7 +1416,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \tfor (;;) {\n \t\tchar path[PATH_MAX];\n \t\tstruct stat st;\n-\t\tchar *buf;\n+\t\tconst char *buf;\n \t\tint fd;\n \n \t\tif (--depth < 0) {\n@@ -1497,11 +1497,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t\tlen--;\n \t\tbuffer[len] = '\\0';\n \n-\t\tif (starts_with(buffer, \"ref:\")) {\n+\t\tif (skip_prefix(buffer, \"ref:\", &buf)) {\n \t\t\t/* It is a symbolic ref */\n \t\t\tif (flag)\n \t\t\t\t*flag |= REF_ISSYMREF;\n-\t\t\tbuf = buffer + 4;\n \t\t\twhile (isspace(*buf))\n \t\t\t\tbuf++;\n \t\t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n-- \n2.1.1\n"},{"id":"250681","messageId":"1413385583-4872-6-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 05/11] refs.c: refactor resolve_ref_unsafe() to use strbuf internally","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:17Z","receivedAt":"2014-10-15T15:06:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nIn the beginning, we had resolve_ref() that returns a buffer owned by\nthis function. Then we started to move away from that direction because\nthe buffer could be overwritten by the next resolve_ref() call and\nintroduced two new functions: resolve_ref_unsafe() and resolve_refdup().\nThe static buffer is still kept internally.\n\nThis patch makes the core of resolve_ref use a strbuf instead of static\nbuffer. Which makes resolve_refdup() more efficient (no need to copy\nfrom the static buffer to a new buffer). It also removes the (random?)\n256 char limit. In future, resolve_ref() could be used directly without\ngoing through resolve_refdup() wrapper.\n\nA minor bonus. resolve_ref(dup) are now more thread-friendly (although I'm\nnot 100% sure if they are thread-safe yet).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Michael Haggerty \n---\n cache.h | 1 +\n refs.c | 115 +++++++++++++++++++++++++++++++++++++---------------------------\n 2 files changed, 67 insertions(+), 49 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3e6a914..e36084d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1004,6 +1004,7 @@ extern int read_ref(const char *refname, unsigned char *sha1);\n */\n extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);\n extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);\n+extern int resolve_ref(const char *refname, struct strbuf *result, unsigned char *sha1, int reading, int *flag);\n \n extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\n extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\ndiff --git a/refs.c b/refs.c\nindex 020ee3f..29ea7e0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1397,34 +1397,41 @@ static int handle_missing_loose_ref(const char *refname,\n \t}\n }\n \n-/* This function needs to return a meaningful errno on failure */\n-const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n+/*\n+ * 'result' content will be destroyed. Its value may be undefined if\n+ * resolve_ref returns -1.\n+ *\n+ * This function needs to return a meaningful errno on failure\n+ */\n+int resolve_ref(const char *refname, struct strbuf *result,\n+\t\tunsigned char *sha1, int reading, int *flag)\n {\n+\tstruct strbuf buffer = STRBUF_INIT;\n \tint depth = MAXDEPTH;\n-\tssize_t len;\n-\tchar buffer[256];\n-\tstatic char refname_buffer[256];\n+\tint ret = -1;\n \n \tif (flag)\n \t\t*flag = 0;\n \n \tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n \t\terrno = EINVAL;\n-\t\treturn NULL;\n+\t\treturn -1;\n \t}\n \n+\tstrbuf_reset(result);\n+\tstrbuf_addstr(result, refname);\n+\n \tfor (;;) {\n \t\tchar path[PATH_MAX];\n \t\tstruct stat st;\n \t\tconst char *buf;\n-\t\tint fd;\n \n \t\tif (--depth < 0) {\n \t\t\terrno = ELOOP;\n-\t\t\treturn NULL;\n+\t\t\tbreak;\n \t\t}\n \n-\t\tgit_snpath(path, sizeof(path), \"%s\", refname);\n+\t\tgit_snpath(path, sizeof(path), \"%s\", result->buf);\n \n \t\t/*\n \t\t * We might have to loop back here to avoid a race\n@@ -1437,30 +1444,26 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t */\n \tstat_ref:\n \t\tif (lstat(path, &st) < 0) {\n-\t\t\tif (errno == ENOENT) {\n-\t\t\t\tif (handle_missing_loose_ref(refname, sha1,\n-\t\t\t\t\t\t\t reading, flag))\n-\t\t\t\t\treturn NULL;\n-\t\t\t\treturn refname;\n-\t\t\t} else\n-\t\t\t\treturn NULL;\n+\t\t\tif (errno == ENOENT)\n+\t\t\t\tret = handle_missing_loose_ref(result->buf, sha1,\n+\t\t\t\t\t\t\t reading, flag);\n+\t\t\tbreak;\n \t\t}\n \n \t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n \t\tif (S_ISLNK(st.st_mode)) {\n-\t\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n-\t\t\tif (len < 0) {\n+\t\t\t/* no need to reset buffer, strbuf_readlink does that */\n+\t\t\tif (strbuf_readlink(&buffer, path, 256) < 0) {\n \t\t\t\tif (errno == ENOENT || errno == EINVAL)\n \t\t\t\t\t/* inconsistent with lstat; retry */\n \t\t\t\t\tgoto stat_ref;\n \t\t\t\telse\n-\t\t\t\t\treturn NULL;\n+\t\t\t\t\tbreak;\n \t\t\t}\n-\t\t\tbuffer[len] = 0;\n-\t\t\tif (starts_with(buffer, \"refs/\") &&\n-\t\t\t\t\t!check_refname_format(buffer, 0)) {\n-\t\t\t\tstrcpy(refname_buffer, buffer);\n-\t\t\t\trefname = refname_buffer;\n+\t\t\tif (starts_with(buffer.buf, \"refs/\") &&\n+\t\t\t !check_refname_format(buffer.buf, 0)) {\n+\t\t\t\tstrbuf_reset(result);\n+\t\t\t\tstrbuf_addbuf(result, &buffer);\n \t\t\t\tif (flag)\n \t\t\t\t\t*flag |= REF_ISSYMREF;\n \t\t\t\tcontinue;\n@@ -1470,34 +1473,24 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t/* Is it a directory? */\n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\terrno = EISDIR;\n-\t\t\treturn NULL;\n+\t\t\tbreak;\n \t\t}\n \n \t\t/*\n \t\t * Anything else, just open it and try to use it as\n \t\t * a ref\n \t\t */\n-\t\tfd = open(path, O_RDONLY);\n-\t\tif (fd < 0) {\n+\t\tstrbuf_reset(&buffer);\n+\t\tif (strbuf_read_file(&buffer, path, 256) < 0) {\n \t\t\tif (errno == ENOENT)\n \t\t\t\t/* inconsistent with lstat; retry */\n \t\t\t\tgoto stat_ref;\n \t\t\telse\n-\t\t\t\treturn NULL;\n-\t\t}\n-\t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n-\t\tif (len < 0) {\n-\t\t\tint save_errno = errno;\n-\t\t\tclose(fd);\n-\t\t\terrno = save_errno;\n-\t\t\treturn NULL;\n+\t\t\t\tbreak;\n \t\t}\n-\t\tclose(fd);\n-\t\twhile (len && isspace(buffer[len-1]))\n-\t\t\tlen--;\n-\t\tbuffer[len] = '\\0';\n+\t\tstrbuf_rtrim(&buffer);\n \n-\t\tif (skip_prefix(buffer, \"ref:\", &buf)) {\n+\t\tif (skip_prefix(buffer.buf, \"ref:\", &buf)) {\n \t\t\t/* It is a symbolic ref */\n \t\t\tif (flag)\n \t\t\t\t*flag |= REF_ISSYMREF;\n@@ -1507,9 +1500,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t\t\tif (flag)\n \t\t\t\t\t*flag |= REF_ISBROKEN;\n \t\t\t\terrno = EINVAL;\n-\t\t\t\treturn NULL;\n+\t\t\t\tbreak;\n \t\t\t}\n-\t\t\trefname = strcpy(refname_buffer, buf);\n+\t\t\tstrbuf_reset(result);\n+\t\t\tstrbuf_add(result, buf, buffer.buf + buffer.len - buf);\n \t\t\tcontinue;\n \t\t}\n \n@@ -1517,21 +1511,44 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t * It must be a normal ref. Please note that\n \t\t * FETCH_HEAD has a second line containing other data.\n \t\t */\n-\t\tif (get_sha1_hex(buffer, sha1) ||\n-\t\t (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n+\t\tif (get_sha1_hex(buffer.buf, sha1) ||\n+\t\t (buffer.buf[40] != '\\0' && !isspace(buffer.buf[40]))) {\n \t\t\tif (flag)\n \t\t\t\t*flag |= REF_ISBROKEN;\n \t\t\terrno = EINVAL;\n-\t\t\treturn NULL;\n-\t\t}\n-\t\treturn refname;\n+\t\t} else\n+\t\t\tret = 0;\n+\t\tbreak;\n \t}\n+\tstrbuf_release(&buffer);\n+\treturn ret;\n+}\n+\n+const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n+{\n+\tstatic struct strbuf buf = STRBUF_INIT;\n+\n+\tif (!resolve_ref(refname, &buf, sha1, reading, flag))\n+\t\t/*\n+\t\t * Please note: true to the \"unsafe\" name of this\n+\t\t * function, we return a pointer to our internal\n+\t\t * memory here rather than passing ownership to the\n+\t\t * caller by calling strbuf_detach():\n+\t\t */\n+\t\treturn buf.buf;\n+\telse\n+\t\treturn NULL;\n }\n \n char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)\n {\n-\tconst char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);\n-\treturn ret ? xstrdup(ret) : NULL;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tif (!resolve_ref(ref, &buf, sha1, reading, flag))\n+\t\treturn strbuf_detach(&buf, NULL);\n+\telse {\n+\t\tstrbuf_release(&buf);\n+\t\treturn NULL;\n+\t}\n }\n \n /* The argument to filter_refs */\n-- \n2.1.1\n"},{"id":"250686","messageId":"1413385583-4872-7-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 06/11] refs.c: move ref parsing code out of resolve_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:18Z","receivedAt":"2014-10-15T15:06:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nThis function will soon have a second caller.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Michael Haggerty \n---\n\nI leave parse_ref() a public function for the reason explained by\nDuy [1].\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/254274\n\n cache.h | 11 ++++\n refs.c | 200 ++++++++++++++++++++++++++++++++++------------------------------\n 2 files changed, 118 insertions(+), 93 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e36084d..d2b8399 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1005,6 +1005,17 @@ extern int read_ref(const char *refname, unsigned char *sha1);\n extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);\n extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);\n extern int resolve_ref(const char *refname, struct strbuf *result, unsigned char *sha1, int reading, int *flag);\n+/*\n+ * Given a ref in \"ref\" and its path, returns\n+ *\n+ * -2 failed to open with ENOENT, the caller is responsible for\n+ * checking missing loose ref (see resolve_ref for example)\n+ * -1 if there's an error, \"refname\" can no longer be trusted, \"flag\" may\n+ * be set. errno is valid.\n+ * 0 this is a symref, \"refname\" now contains the linked ref\n+ * +1 normal ref, \"sha1\" is valid\n+ */\n+extern int parse_ref(const char *path, struct strbuf *refname, unsigned char *sha1, int *flag);\n \n extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\n extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\ndiff --git a/refs.c b/refs.c\nindex 29ea7e0..3acd83e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1397,6 +1397,104 @@ static int handle_missing_loose_ref(const char *refname,\n \t}\n }\n \n+int parse_ref(const char *path, struct strbuf *refname,\n+\t unsigned char *sha1, int *flag)\n+{\n+\tstruct strbuf buffer = STRBUF_INIT;\n+\tstruct stat st;\n+\tconst char *buf;\n+\tint ret;\n+\n+\t/*\n+\t * We might have to loop back here to avoid a race condition:\n+\t * first we lstat() the file, then we try to read it as a link\n+\t * or as a file. But if somebody changes the type of the file\n+\t * (file <-> directory <-> symlink) between the lstat() and\n+\t * reading, then we don't want to report that as an error but\n+\t * rather try again starting with the lstat().\n+\t */\n+stat_ref:\n+\tif (lstat(path, &st) < 0)\n+\t\treturn errno == ENOENT ? -2 : -1;\n+\n+\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n+\tif (S_ISLNK(st.st_mode)) {\n+\t\tstruct strbuf new_path = STRBUF_INIT;\n+\t\tif (strbuf_readlink(&new_path, path, 256) < 0) {\n+\t\t\tstrbuf_release(&new_path);\n+\t\t\tif (errno == ENOENT || errno == EINVAL)\n+\t\t\t\t/* inconsistent with lstat; retry */\n+\t\t\t\tgoto stat_ref;\n+\t\t\telse\n+\t\t\t\treturn -1;\n+\t\t}\n+\t\tif (starts_with(new_path.buf, \"refs/\") &&\n+\t\t !check_refname_format(new_path.buf, 0)) {\n+\t\t\tstrbuf_reset(refname);\n+\t\t\tstrbuf_addbuf(refname, &new_path);\n+\t\t\tif (flag)\n+\t\t\t\t*flag |= REF_ISSYMREF;\n+\t\t\tstrbuf_release(&new_path);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tstrbuf_release(&new_path);\n+\t}\n+\n+\t/* Is it a directory? */\n+\tif (S_ISDIR(st.st_mode)) {\n+\t\terrno = EISDIR;\n+\t\treturn -1;\n+\t}\n+\n+\t/*\n+\t * Anything else, just open it and try to use it as\n+\t * a ref\n+\t */\n+\tif (strbuf_read_file(&buffer, path, 256) < 0) {\n+\t\tstrbuf_release(&buffer);\n+\t\tif (errno == ENOENT)\n+\t\t\t/* inconsistent with lstat; retry */\n+\t\t\tgoto stat_ref;\n+\t\telse\n+\t\t\treturn -1;\n+\t}\n+\tstrbuf_rtrim(&buffer);\n+\n+\tif (skip_prefix(buffer.buf, \"ref:\", &buf)) {\n+\t\t/* It is a symbolic ref */\n+\t\tif (flag)\n+\t\t\t*flag |= REF_ISSYMREF;\n+\t\twhile (isspace(*buf))\n+\t\t\tbuf++;\n+\t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n+\t\t\tif (flag)\n+\t\t\t\t*flag |= REF_ISBROKEN;\n+\t\t\tstrbuf_release(&buffer);\n+\t\t\terrno = EINVAL;\n+\t\t\treturn -1;\n+\t\t}\n+\t\tstrbuf_reset(refname);\n+\t\tstrbuf_add(refname, buf, buffer.buf + buffer.len - buf);\n+\t\tstrbuf_release(&buffer);\n+\t\treturn 0;\n+\t}\n+\n+\t/*\n+\t * Please note that FETCH_HEAD has a second line\n+\t * containing other data.\n+\t */\n+\tif (get_sha1_hex(buffer.buf, sha1) ||\n+\t (buffer.buf[40] != '\\0' && !isspace(buffer.buf[40]))) {\n+\t\tif (flag)\n+\t\t\t*flag |= REF_ISBROKEN;\n+\t\terrno = EINVAL;\n+\t\tret = -1;\n+\t} else\n+\t\tret = 1;\n+\tstrbuf_release(&buffer);\n+\treturn ret;\n+}\n+\n /*\n * 'result' content will be destroyed. Its value may be undefined if\n * resolve_ref returns -1.\n@@ -1406,9 +1504,8 @@ static int handle_missing_loose_ref(const char *refname,\n int resolve_ref(const char *refname, struct strbuf *result,\n \t\tunsigned char *sha1, int reading, int *flag)\n {\n-\tstruct strbuf buffer = STRBUF_INIT;\n \tint depth = MAXDEPTH;\n-\tint ret = -1;\n+\tint ret = 0;\n \n \tif (flag)\n \t\t*flag = 0;\n@@ -1421,107 +1518,24 @@ int resolve_ref(const char *refname, struct strbuf *result,\n \tstrbuf_reset(result);\n \tstrbuf_addstr(result, refname);\n \n-\tfor (;;) {\n+\twhile (!ret) {\n \t\tchar path[PATH_MAX];\n-\t\tstruct stat st;\n-\t\tconst char *buf;\n \n \t\tif (--depth < 0) {\n \t\t\terrno = ELOOP;\n+\t\t\tret = -1;\n \t\t\tbreak;\n \t\t}\n \n \t\tgit_snpath(path, sizeof(path), \"%s\", result->buf);\n-\n-\t\t/*\n-\t\t * We might have to loop back here to avoid a race\n-\t\t * condition: first we lstat() the file, then we try\n-\t\t * to read it as a link or as a file. But if somebody\n-\t\t * changes the type of the file (file <-> directory\n-\t\t * <-> symlink) between the lstat() and reading, then\n-\t\t * we don't want to report that as an error but rather\n-\t\t * try again starting with the lstat().\n-\t\t */\n-\tstat_ref:\n-\t\tif (lstat(path, &st) < 0) {\n-\t\t\tif (errno == ENOENT)\n-\t\t\t\tret = handle_missing_loose_ref(result->buf, sha1,\n-\t\t\t\t\t\t\t reading, flag);\n-\t\t\tbreak;\n+\t\tret = parse_ref(path, result, sha1, flag);\n+\t\tif (ret == -2) {\n+\t\t\tret = handle_missing_loose_ref(result->buf, sha1,\n+\t\t\t\t\t\t reading, flag);\n+\t\t\tret = ret ? -1 : 1;\n \t\t}\n-\n-\t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n-\t\tif (S_ISLNK(st.st_mode)) {\n-\t\t\t/* no need to reset buffer, strbuf_readlink does that */\n-\t\t\tif (strbuf_readlink(&buffer, path, 256) < 0) {\n-\t\t\t\tif (errno == ENOENT || errno == EINVAL)\n-\t\t\t\t\t/* inconsistent with lstat; retry */\n-\t\t\t\t\tgoto stat_ref;\n-\t\t\t\telse\n-\t\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (starts_with(buffer.buf, \"refs/\") &&\n-\t\t\t !check_refname_format(buffer.buf, 0)) {\n-\t\t\t\tstrbuf_reset(result);\n-\t\t\t\tstrbuf_addbuf(result, &buffer);\n-\t\t\t\tif (flag)\n-\t\t\t\t\t*flag |= REF_ISSYMREF;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t}\n-\n-\t\t/* Is it a directory? */\n-\t\tif (S_ISDIR(st.st_mode)) {\n-\t\t\terrno = EISDIR;\n-\t\t\tbreak;\n-\t\t}\n-\n-\t\t/*\n-\t\t * Anything else, just open it and try to use it as\n-\t\t * a ref\n-\t\t */\n-\t\tstrbuf_reset(&buffer);\n-\t\tif (strbuf_read_file(&buffer, path, 256) < 0) {\n-\t\t\tif (errno == ENOENT)\n-\t\t\t\t/* inconsistent with lstat; retry */\n-\t\t\t\tgoto stat_ref;\n-\t\t\telse\n-\t\t\t\tbreak;\n-\t\t}\n-\t\tstrbuf_rtrim(&buffer);\n-\n-\t\tif (skip_prefix(buffer.buf, \"ref:\", &buf)) {\n-\t\t\t/* It is a symbolic ref */\n-\t\t\tif (flag)\n-\t\t\t\t*flag |= REF_ISSYMREF;\n-\t\t\twhile (isspace(*buf))\n-\t\t\t\tbuf++;\n-\t\t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n-\t\t\t\tif (flag)\n-\t\t\t\t\t*flag |= REF_ISBROKEN;\n-\t\t\t\terrno = EINVAL;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tstrbuf_reset(result);\n-\t\t\tstrbuf_add(result, buf, buffer.buf + buffer.len - buf);\n-\t\t\tcontinue;\n-\t\t}\n-\n-\t\t/*\n-\t\t * It must be a normal ref. Please note that\n-\t\t * FETCH_HEAD has a second line containing other data.\n-\t\t */\n-\t\tif (get_sha1_hex(buffer.buf, sha1) ||\n-\t\t (buffer.buf[40] != '\\0' && !isspace(buffer.buf[40]))) {\n-\t\t\tif (flag)\n-\t\t\t\t*flag |= REF_ISBROKEN;\n-\t\t\terrno = EINVAL;\n-\t\t} else\n-\t\t\tret = 0;\n-\t\tbreak;\n \t}\n-\tstrbuf_release(&buffer);\n-\treturn ret;\n+\treturn ret > 0 ? 0 : -1;\n }\n \n const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n-- \n2.1.1\n"},{"id":"250689","messageId":"1413385583-4872-8-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 07/11] handle_missing_loose_ref(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:19Z","receivedAt":"2014-10-15T15:06:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It only had one remaining caller.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 70 ++++++++++++++++++++++++++++--------------------------------------\n 1 file changed, 30 insertions(+), 40 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3acd83e..9f2a0f8 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1366,37 +1366,6 @@ static struct ref_entry *get_packed_ref(const char *refname)\n \treturn find_ref(get_packed_refs(&ref_cache), refname);\n }\n \n-/*\n- * A loose ref file doesn't exist; check for a packed ref. The\n- * options are forwarded from resolve_safe_unsafe().\n- */\n-static int handle_missing_loose_ref(const char *refname,\n-\t\t\t\t unsigned char *sha1,\n-\t\t\t\t int reading,\n-\t\t\t\t int *flag)\n-{\n-\tstruct ref_entry *entry;\n-\n-\t/*\n-\t * The loose reference file does not exist; check for a packed\n-\t * reference.\n-\t */\n-\tentry = get_packed_ref(refname);\n-\tif (entry) {\n-\t\thashcpy(sha1, entry->u.value.sha1);\n-\t\tif (flag)\n-\t\t\t*flag |= REF_ISPACKED;\n-\t\treturn 0;\n-\t}\n-\t/* The reference is not a packed reference, either. */\n-\tif (reading) {\n-\t\treturn -1;\n-\t} else {\n-\t\thashclr(sha1);\n-\t\treturn 0;\n-\t}\n-}\n-\n int parse_ref(const char *path, struct strbuf *refname,\n \t unsigned char *sha1, int *flag)\n {\n@@ -1505,7 +1474,7 @@ int resolve_ref(const char *refname, struct strbuf *result,\n \t\tunsigned char *sha1, int reading, int *flag)\n {\n \tint depth = MAXDEPTH;\n-\tint ret = 0;\n+\tint ret;\n \n \tif (flag)\n \t\t*flag = 0;\n@@ -1518,24 +1487,45 @@ int resolve_ref(const char *refname, struct strbuf *result,\n \tstrbuf_reset(result);\n \tstrbuf_addstr(result, refname);\n \n-\twhile (!ret) {\n+\tdo {\n \t\tchar path[PATH_MAX];\n \n \t\tif (--depth < 0) {\n \t\t\terrno = ELOOP;\n-\t\t\tret = -1;\n-\t\t\tbreak;\n+\t\t\treturn -1;\n \t\t}\n \n \t\tgit_snpath(path, sizeof(path), \"%s\", result->buf);\n \t\tret = parse_ref(path, result, sha1, flag);\n-\t\tif (ret == -2) {\n-\t\t\tret = handle_missing_loose_ref(result->buf, sha1,\n-\t\t\t\t\t\t reading, flag);\n-\t\t\tret = ret ? -1 : 1;\n+\t} while (!ret);\n+\n+\tif (ret == 1) {\n+\t\treturn 0;\n+\t} else if (ret == -2) {\n+\t\t/*\n+\t\t * The loose reference file does not exist; check for a packed\n+\t\t * reference.\n+\t\t */\n+\t\tstruct ref_entry *entry;\n+\n+\t\tentry = get_packed_ref(result->buf);\n+\t\tif (entry) {\n+\t\t\thashcpy(sha1, entry->u.value.sha1);\n+\t\t\tif (flag)\n+\t\t\t\t*flag |= REF_ISPACKED;\n+\t\t\treturn 0;\n+\t\t}\n+\n+\t\t/* The reference is not a packed reference, either. */\n+\t\tif (reading) {\n+\t\t\treturn -1;\n+\t\t} else {\n+\t\t\thashclr(sha1);\n+\t\t\treturn 0;\n \t\t}\n+\t} else {\n+\t\treturn -1;\n \t}\n-\treturn ret > 0 ? 0 : -1;\n }\n \n const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n-- \n2.1.1\n"},{"id":"250685","messageId":"1413385583-4872-9-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 08/11] resolve_gitlink_ref_recursive(): drop arbitrary refname length limit","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:20Z","receivedAt":"2014-10-15T15:06:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This limit was added in\n\n 0ebde32 (Add 'resolve_gitlink_ref()' helper function - 2007-04-09)\n\nSigned-off-by: Michael Haggerty \n---\nTheoretically, somebody else might be relying on\nresolve_gitlink_ref_recursive() to fail for too-long reference names\nto prevent path.c's pitiful error handling from returning \"/bad-path/\"\nand causing a nonsensical file lookup. I doubt it, but if somebody is\nworried about it we could leave out this patch and instead build the\nMAXREFLEN check into parse_ref().\n\nLong-term, I think we should fix up path.c to remove its PATH_MAX\nlimits. I've started working on that.\n\n refs.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9f2a0f8..e1aa6a4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1279,7 +1279,6 @@ static struct ref_dir *get_loose_refs(struct ref_cache *refs)\n \n /* We allow \"recursive\" symbolic refs. Only within reason, though */\n #define MAXDEPTH 5\n-#define MAXREFLEN (1024)\n \n /*\n * Called by resolve_gitlink_ref_recursive() after it failed to read\n@@ -1308,7 +1307,7 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs,\n \tchar buffer[128], *p;\n \tchar *path;\n \n-\tif (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN)\n+\tif (recursion > MAXDEPTH)\n \t\treturn -1;\n \tpath = *refs->name\n \t\t? git_path_submodule(refs->name, \"%s\", refname)\n-- \n2.1.1\n"},{"id":"250682","messageId":"1413385583-4872-10-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 09/11] refs.c: rewrite resolve_gitlink_ref() to use parse_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:21Z","receivedAt":"2014-10-15T15:06:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nresolve_gitlink_ref_recursive() did about the same thing as\nparse_ref(), but didn't know as many tricks. It also had another\nrandom limit of 128 bytes for symrefs. So base resolve_gitlink_ref()\non parse_ref() instead.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Michael Haggerty \n---\n refs.c | 69 ++++++++++++++++++++++++++----------------------------------------\n 1 file changed, 27 insertions(+), 42 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e1aa6a4..a7c8abd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1299,48 +1299,11 @@ static int resolve_gitlink_packed_ref(struct ref_cache *refs,\n \treturn 0;\n }\n \n-static int resolve_gitlink_ref_recursive(struct ref_cache *refs,\n-\t\t\t\t\t const char *refname, unsigned char *sha1,\n-\t\t\t\t\t int recursion)\n-{\n-\tint fd, len;\n-\tchar buffer[128], *p;\n-\tchar *path;\n-\n-\tif (recursion > MAXDEPTH)\n-\t\treturn -1;\n-\tpath = *refs->name\n-\t\t? git_path_submodule(refs->name, \"%s\", refname)\n-\t\t: git_path(\"%s\", refname);\n-\tfd = open(path, O_RDONLY);\n-\tif (fd < 0)\n-\t\treturn resolve_gitlink_packed_ref(refs, refname, sha1);\n-\n-\tlen = read(fd, buffer, sizeof(buffer)-1);\n-\tclose(fd);\n-\tif (len < 0)\n-\t\treturn -1;\n-\twhile (len && isspace(buffer[len-1]))\n-\t\tlen--;\n-\tbuffer[len] = 0;\n-\n-\t/* Was it a detached head or an old-fashioned symlink? */\n-\tif (!get_sha1_hex(buffer, sha1))\n-\t\treturn 0;\n-\n-\t/* Symref? */\n-\tif (strncmp(buffer, \"ref:\", 4))\n-\t\treturn -1;\n-\tp = buffer + 4;\n-\twhile (isspace(*p))\n-\t\tp++;\n-\n-\treturn resolve_gitlink_ref_recursive(refs, p, sha1, recursion+1);\n-}\n-\n int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1)\n {\n-\tint len = strlen(path), retval;\n+\tstruct strbuf result = STRBUF_INIT;\n+\tint len = strlen(path), parseval, ret;\n+\tint depth = MAXDEPTH;\n \tchar *submodule;\n \tstruct ref_cache *refs;\n \n@@ -1352,8 +1315,30 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh\n \trefs = get_ref_cache(submodule);\n \tfree(submodule);\n \n-\tretval = resolve_gitlink_ref_recursive(refs, refname, sha1, 0);\n-\treturn retval;\n+\tstrbuf_addstr(&result, refname);\n+\tdo {\n+\t\tif (--depth < 0) {\n+\t\t\terrno = ELOOP;\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n+\t\tpath = *refs->name\n+\t\t\t? git_path_submodule(refs->name, \"%s\", result.buf)\n+\t\t\t: git_path(\"%s\", result.buf);\n+\t\tparseval = parse_ref(path, &result, sha1, NULL);\n+\t} while (!parseval);\n+\n+\tif (parseval == 1) {\n+\t\tret = 0;\n+\t} else if (parseval == -2) {\n+\t\tret = resolve_gitlink_packed_ref(refs, result.buf, sha1) ? -1 : 0;\n+\t} else {\n+\t\tret = -1;\n+\t}\n+\n+out:\n+\tstrbuf_release(&result);\n+\treturn ret;\n }\n \n /*\n-- \n2.1.1\n"},{"id":"250687","messageId":"1413385583-4872-11-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 10/11] resove_gitlink_packed_ref(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:22Z","receivedAt":"2014-10-15T15:06:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n refs.c | 31 +++++++++++--------------------\n 1 file changed, 11 insertions(+), 20 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex a7c8abd..8edcc3b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1280,25 +1280,6 @@ static struct ref_dir *get_loose_refs(struct ref_cache *refs)\n /* We allow \"recursive\" symbolic refs. Only within reason, though */\n #define MAXDEPTH 5\n \n-/*\n- * Called by resolve_gitlink_ref_recursive() after it failed to read\n- * from the loose refs in ref_cache refs. Find in the\n- * packed-refs file for the submodule.\n- */\n-static int resolve_gitlink_packed_ref(struct ref_cache *refs,\n-\t\t\t\t const char *refname, unsigned char *sha1)\n-{\n-\tstruct ref_entry *ref;\n-\tstruct ref_dir *dir = get_packed_refs(refs);\n-\n-\tref = find_ref(dir, refname);\n-\tif (ref == NULL)\n-\t\treturn -1;\n-\n-\thashcpy(sha1, ref->u.value.sha1);\n-\treturn 0;\n-}\n-\n int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1)\n {\n \tstruct strbuf result = STRBUF_INIT;\n@@ -1331,7 +1312,17 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh\n \tif (parseval == 1) {\n \t\tret = 0;\n \t} else if (parseval == -2) {\n-\t\tret = resolve_gitlink_packed_ref(refs, result.buf, sha1) ? -1 : 0;\n+\t\t/* Loose ref doesn't exist; check for a packed ref */\n+\t\tstruct ref_entry *entry;\n+\t\tstruct ref_dir *dir = get_packed_refs(refs);\n+\n+\t\tentry = find_ref(dir, result.buf);\n+\t\tif (entry) {\n+\t\t\thashcpy(sha1, entry->u.value.sha1);\n+\t\t\tret = 0;\n+\t\t} else {\n+\t\t\tret = -1;\n+\t\t}\n \t} else {\n \t\tret = -1;\n \t}\n-- \n2.1.1\n"},{"id":"250684","messageId":"1413385583-4872-12-git-send-email-mhagger@alum.mit.edu","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 11/11] resolve_gitlink_ref(): remove redundant test","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-15T15:06:23Z","receivedAt":"2014-10-15T15:06:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"At this point we know that refs->name is a non-empty string, so we\nknow we don't have to look up the reference in the main repository.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8edcc3b..3f4b95a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1303,9 +1303,7 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sh\n \t\t\tret = -1;\n \t\t\tgoto out;\n \t\t}\n-\t\tpath = *refs->name\n-\t\t\t? git_path_submodule(refs->name, \"%s\", result.buf)\n-\t\t\t: git_path(\"%s\", result.buf);\n+\t\tpath = git_path_submodule(refs->name, \"%s\", result.buf);\n \t\tparseval = parse_ref(path, &result, sha1, NULL);\n \t} while (!parseval);\n \n-- \n2.1.1\n"},{"id":"250771","messageId":"xmqqwq7zspwm.fsf@gitster.dls.corp.google.com","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/11] Consolidate ref parsing code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-16T20:47:21Z","receivedAt":"2014-10-16T20:47:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This is a rif on Duy's oldish patch series [1]. I started reviewing\n> his patch series, but found that some of his patches did multiple\n> things, making it harder to review. I started pulling it apart into\n> smaller changes to aid my review, and I guess I got carried away :-/\n\nHmmm...\n\nYou are aware of another large change in flight in the same area,\nand after having reviewed that series a few times I was hoping that\nyou would have a better understanding of how ready the other topic\nis. And then I see this series that conflicts with that other\ntopic. Is this your way to say that the other topic is not quite\nready?\n"},{"id":"250778","messageId":"xmqqd29rsmx6.fsf@gitster.dls.corp.google.com","threadId":"37736","inReplyTo":"xmqqwq7zspwm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 00/11] Consolidate ref parsing code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-16T21:51:49Z","receivedAt":"2014-10-16T21:51:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Michael Haggerty writes:\n>\n>> This is a rif on Duy's oldish patch series [1]. I started reviewing\n>> his patch series, but found that some of his patches did multiple\n>> things, making it harder to review. I started pulling it apart into\n>> smaller changes to aid my review, and I guess I got carried away :-/\n>\n> Hmmm...\n>\n> You are aware of another large change in flight in the same area,\n> and after having reviewed that series a few times I was hoping that\n> you would have a better understanding of how ready the other topic\n> is. And then I see this series that conflicts with that other\n> topic. Is this your way to say that the other topic is not quite\n> ready?\n\nThe last one was a rhetorical question and I regret that it came out\na bit harsher than I intended to. I just wanted to see a bit better\ninter-developer coordination, that's all.\n\nThanks.\n"},{"id":"250781","messageId":"54405358.3040803@alum.mit.edu","threadId":"37736","inReplyTo":"xmqqwq7zspwm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 00/11] Consolidate ref parsing code","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-10-16T23:23:04Z","receivedAt":"2014-10-16T23:23:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/16/2014 10:47 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> This is a rif on Duy's oldish patch series [1]. I started reviewing\n>> his patch series, but found that some of his patches did multiple\n>> things, making it harder to review. I started pulling it apart into\n>> smaller changes to aid my review, and I guess I got carried away :-/\n> \n> Hmmm...\n> \n> You are aware of another large change in flight in the same area,\n> and after having reviewed that series a few times I was hoping that\n> you would have a better understanding of how ready the other topic\n> is. And then I see this series that conflicts with that other\n> topic. Is this your way to say that the other topic is not quite\n> ready?\n\nNo, not at all. To be honest, I thought that the changes in this patch\nseries were localized in an area different than Ronnie and Jonathan's\npatches were touching, but stupidly I didn't check for conflicts. Sorry\nabout that.\n\nThere is nothing urgent in this patch series, so I suggest we just put\nit back on ice and I will reroll after the dust has settled on\nrs/ref-transactions. The conflicts shouldn't be super hard to resolve\n(the series don't overlap much *conceptually*), but I'd rather not have\nto do it multiple times.\n\nRegarding rs/ref-transaction, I will reply on that thread.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"250783","messageId":"CACsJy8DOxFiq5S7o89KSXywqonXei6fArWqHMbv5ivpyZWiEpw@mail.gmail.com","threadId":"37736","inReplyTo":"1413385583-4872-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/11] Consolidate ref parsing code","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-10-16T23:53:09Z","receivedAt":"2014-10-16T23:53:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 15, 2014 at 10:06 PM, Michael Haggerty wrote:\n> As far as I know, Duy isn't actively working on this, so I hope my\n> reroll is not unwelcome.\n\nI couldn't be happier that someone does the work for me and I still\nbenefit from it ;)\n-- \nDuy\n"}]}