{"thread":{"id":"37210","subject":"[PATCH 0/4] Consolidate ref parsing code","startedAt":"2014-07-25T10:43:55Z","lastAt":"2014-09-26T10:30:38Z","messageCount":11,"participants":["Nguyễn Thái Ngọc Duy","Eric Sunshine","Ronnie Sahlberg","Duy Nguyen","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"246685","messageId":"1406285039-22469-1-git-send-email-pclouds@gmail.com","threadId":"37210","inReplyTo":null,"subject":"[PATCH 0/4] Consolidate ref parsing code","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-25T10:43:55Z","receivedAt":"2014-07-25T10:43:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jul 24, 2014 at 4:16 AM, Junio C Hamano wrote:\n> - .... three places now knows what a\n> textual symref looks like (i.e. begins with \"ref:\", zero or more\n> whitespaces, the target ref and then zero or more trailing\n> whitespaces). Perhaps we need to consolidate the code further,\n> so that this knowledge does not leak out of refs.c?\n\nI started on top of nd/multiple-work-trees but it conflicts badly with\nrs/ref-transaction-0 because this is basically code move. So I think\nwe should make it a separate topic instead, based on latest 'master'.\nJunio, you still hit conflicts when merging this with nd/multiple-work-trees,\nbut that's simpler to resolve (git_snpath -> strbuf_git_path). I\npromise to replace the \"ref:\" code in checkout.c later when both\ntopics graduate.\n\nSo.. first cut. The end result looks nice.\n\nNguyễn Thái Ngọc Duy (4):\n strbuf.c: keep errno in strbuf_read_file()\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 | 332 ++++++++++++++++++++++++++++++++-------------------------------\n strbuf.c | 7 +-\n 3 files changed, 188 insertions(+), 163 deletions(-)\n\n-- \n1.9.1.346.ga2b5940\n"},{"id":"246686","messageId":"1406285039-22469-2-git-send-email-pclouds@gmail.com","threadId":"37210","inReplyTo":"1406285039-22469-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/4] strbuf.c: keep errno in strbuf_read_file()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-25T10:43:56Z","receivedAt":"2014-07-25T10:43:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This function is used to replaced some code in the next patch that\ndoes this (i.e. keep the errno when read() fails)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n strbuf.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 33018d8..61d685d 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -454,15 +454,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-- \n1.9.1.346.ga2b5940\n"},{"id":"246687","messageId":"1406285039-22469-3-git-send-email-pclouds@gmail.com","threadId":"37210","inReplyTo":"1406285039-22469-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/4] refs.c: refactor resolve_ref_unsafe() to use strbuf internally","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-25T10:43:57Z","receivedAt":"2014-07-25T10:43:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In 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 \n---\n cache.h | 1 +\n refs.c | 122 +++++++++++++++++++++++++++++++++++-----------------------------\n 2 files changed, 68 insertions(+), 55 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex fcb511d..5ffbafb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1002,6 +1002,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 84b9070..bec2bb1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1506,10 +1506,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@@ -1522,45 +1522,53 @@ 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-/* 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\tconst char *ref = result->buf;\n \t\tstruct stat st;\n-\t\tchar *buf;\n-\t\tint fd;\n+\t\tconst char *buf;\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\", ref);\n \n \t\t/*\n \t\t * We might have to loop back here to avoid a race\n@@ -1574,27 +1582,25 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\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\t\treturn NULL;\n+\t\t\t\tret = handle_missing_loose_ref(ref, 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@@ -1604,69 +1610,75 @@ 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\t/*\n \t\t * Is it a symbolic ref?\n \t\t */\n-\t\tif (!starts_with(buffer, \"ref:\")) {\n+\t\tif (!skip_prefix(buffer.buf, \"ref:\", &buf)) {\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\t\tif (get_sha1_hex(buffer.buf, sha1) ||\n+\t\t\t (buffer.buf[40] != '\\0' && !isspace(buffer.buf[40]))) {\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\t} else\n+\t\t\t\tret = 0;\n+\t\t\tbreak;\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 \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\tbreak;\n \t\t}\n-\t\trefname = strcpy(refname_buffer, buf);\n+\t\tstrbuf_reset(result);\n+\t\tstrbuf_addstr(result, buf);\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+\tif (!resolve_ref(refname, &buf, sha1, reading, flag))\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 buf.buf;\n+\telse {\n+\t\tstrbuf_release(&buf);\n+\t\treturn NULL;\n+\t}\n }\n \n /* The argument to filter_refs */\n-- \n1.9.1.346.ga2b5940\n"},{"id":"246688","messageId":"1406285039-22469-4-git-send-email-pclouds@gmail.com","threadId":"37210","inReplyTo":"1406285039-22469-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/4] refs.c: move ref parsing code out of resolve_ref()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-25T10:43:58Z","receivedAt":"2014-07-25T10:43:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n cache.h | 11 ++++\n refs.c | 204 ++++++++++++++++++++++++++++++++++------------------------------\n 2 files changed, 120 insertions(+), 95 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 5ffbafb..40a63d9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1003,6 +1003,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, \"ref\" can no longer be trusted, \"flag\" may\n+ * be set. errno is valid.\n+ * 0 this is a symref, \"ref\" now contains the linked ref\n+ * +1 normal ref, \"sha1\" is valid\n+ */\n+extern int parse_ref(const char *path, struct strbuf *ref, 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 bec2bb1..2769f20 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1533,6 +1533,105 @@ static int handle_missing_loose_ref(const char *refname,\n \t}\n }\n \n+int parse_ref(const char *path, struct strbuf *ref,\n+\t unsigned char *sha1, int *flag)\n+{\n+\tstruct strbuf buffer = STRBUF_INIT;\n+\tstruct stat st;\n+\tconst char *buf;\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(ref);\n+\t\t\tstrbuf_addbuf(ref, &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+\t/*\n+\t * Is it a symbolic ref?\n+\t */\n+\tif (!skip_prefix(buffer.buf, \"ref:\", &buf)) {\n+\t\tint ret;\n+\t\t/*\n+\t\t * Please note that FETCH_HEAD has a second line\n+\t\t * 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\tret = -1;\n+\t\t} else\n+\t\t\tret = 1;\n+\t\tstrbuf_release(&buffer);\n+\t\treturn ret;\n+\t}\n+\tif (flag)\n+\t\t*flag |= REF_ISSYMREF;\n+\twhile (isspace(*buf))\n+\t\tbuf++;\n+\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n+\t\tif (flag)\n+\t\t\t*flag |= REF_ISBROKEN;\n+\t\tstrbuf_release(&buffer);\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\tstrbuf_reset(ref);\n+\tstrbuf_addstr(ref, buf);\n+\tstrbuf_release(&buffer);\n+\treturn 0;\n+}\n+\n /*\n * 'result' content will be destroyed. Its value may be undefined if\n * resolve_ref returns -1.\n@@ -1542,9 +1641,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@@ -1557,108 +1655,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\tconst char *ref = result->buf;\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\", ref);\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(ref, sha1,\n-\t\t\t\t\t\t\t reading, flag);\n-\t\t\tbreak;\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\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\t/*\n-\t\t * Is it a symbolic ref?\n-\t\t */\n-\t\tif (!skip_prefix(buffer.buf, \"ref:\", &buf)) {\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.buf, sha1) ||\n-\t\t\t (buffer.buf[40] != '\\0' && !isspace(buffer.buf[40]))) {\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} else\n-\t\t\t\tret = 0;\n-\t\t\tbreak;\n-\t\t}\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\terrno = EINVAL;\n-\t\t\tbreak;\n-\t\t}\n-\t\tstrbuf_reset(result);\n-\t\tstrbuf_addstr(result, buf);\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-- \n1.9.1.346.ga2b5940\n"},{"id":"246689","messageId":"1406285039-22469-5-git-send-email-pclouds@gmail.com","threadId":"37210","inReplyTo":"1406285039-22469-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 4/4] refs.c: rewrite resolve_gitlink_ref() to use parse_ref()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-25T10:43:59Z","receivedAt":"2014-07-25T10:43:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"resolve_gitlink_ref_recursive() is less strict than the old\nresolve_ref_unsafe() (which is now parse_ref()). It also has another\nrandom limit 128 bytes for symrefs.\n\nThis brings MAXREFLEN check to resolve_ref* family. It looks like an\narbitrary limit though (added in 0ebde32 (Add 'resolve_gitlink_ref()'\nhelper function - 2007-04-09)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n refs.c | 68 +++++++++++++++++++++++++-----------------------------------------\n 1 file changed, 26 insertions(+), 42 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2769f20..24503e5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1436,48 +1436,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 || strlen(refname) > MAXREFLEN)\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), retval = 0;\n+\tint depth = MAXDEPTH;\n \tchar *submodule;\n \tstruct ref_cache *refs;\n \n@@ -1489,8 +1452,24 @@ 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+\twhile (!retval) {\n+\t\tif (--depth < 0) {\n+\t\t\terrno = ELOOP;\n+\t\t\tretval = -1;\n+\t\t\tbreak;\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\tretval = parse_ref(path, &result, sha1, NULL);\n+\t\tif (retval == -2) {\n+\t\t\tretval = resolve_gitlink_packed_ref(refs, result.buf, sha1);\n+\t\t\tretval = retval ? -1 : 1;\n+\t\t}\n+\t}\n+\tstrbuf_release(&result);\n+\treturn retval > 0 ? 0 : -1;\n }\n \n /*\n@@ -1540,6 +1519,11 @@ int parse_ref(const char *path, struct strbuf *ref,\n \tstruct stat st;\n \tconst char *buf;\n \n+\tif (ref->len > MAXREFLEN) {\n+\t\terrno = ENAMETOOLONG;\n+\t\treturn -1;\n+\t}\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-- \n1.9.1.346.ga2b5940\n"},{"id":"246708","messageId":"CAPig+cS4CV+1nsab2yZCYbYsRfKJoE60Gh5KxjE-0qE-3ZuRtQ@mail.gmail.com","threadId":"37210","inReplyTo":"1406285039-22469-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/4] strbuf.c: keep errno in strbuf_read_file()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-07-25T15:41:53Z","receivedAt":"2014-07-25T15:41:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 25, 2014 at 6:43 AM, Nguyễn Thái Ngọc Duy wrote:\n> This function is used to replaced some code in the next patch that\n\ns/replaced/replace/\n\n> does this (i.e. keep the errno when read() fails)\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> strbuf.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index 33018d8..61d685d 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -454,15 +454,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> - int fd, len;\n> + int fd, len, saved_errno;\n>\n> fd = open(path, O_RDONLY);\n> if (fd < 0)\n> return -1;\n> len = strbuf_read(sb, fd, hint);\n> + saved_errno = errno;\n> close(fd);\n> - if (len < 0)\n> + if (len < 0) {\n> + errno = saved_errno;\n> return -1;\n> + }\n>\n> return len;\n> }\n> --\n> 1.9.1.346.ga2b5940\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"246709","messageId":"CAPig+cSvymyM3G+LujopPAVtaXVYN_mYJ0pxPkvb2pvLsq+e-A@mail.gmail.com","threadId":"37210","inReplyTo":"1406285039-22469-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/4] refs.c: refactor resolve_ref_unsafe() to use strbuf internally","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-07-25T15:55:10Z","receivedAt":"2014-07-25T15:55:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 25, 2014 at 6:43 AM, Nguyễn Thái Ngọc Duy wrote:\n> In the beginning, we had resolve_ref() that returns a buffer owned by\n> this function. Then we started to move away from that direction because\n> the buffer could be overwritten by the next resolve_ref() call and\n> introduced two new functions: resolve_ref_unsafe() and resolve_refdup().\n> The static buffer is still kept internally.\n>\n> This patch makes the core of resolve_ref use a strbuf instead of static\n> buffer. Which makes resolve_refdup() more efficient (no need to copy\n> from the static buffer to a new buffer). It also removes the (random?)\n> 256 char limit. In future, resolve_ref() could be used directly without\n> going through resolve_refdup() wrapper.\n>\n> A minor bonus. resolve_ref(dup) are now more thread-friendly (although I'm\n> not 100% sure if they are thread-safe yet).\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> cache.h | 1 +\n> refs.c | 122 +++++++++++++++++++++++++++++++++++-----------------------------\n> 2 files changed, 68 insertions(+), 55 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index fcb511d..5ffbafb 100644\n> --- a/cache.h\n> +++ b/cache.h\n> +const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n> +{\n> + static struct strbuf buf = STRBUF_INIT;\n> + if (!resolve_ref(refname, &buf, sha1, reading, flag))\n> + return buf.buf;\n> + else\n> + return NULL;\n> }\n>\n> char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)\n> {\n> - const char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);\n> - return ret ? xstrdup(ret) : NULL;\n> + struct strbuf buf = STRBUF_INIT;\n> + if (!resolve_ref(ref, &buf, sha1, reading, flag))\n> + return buf.buf;\n\nreturn strbuf_detach(&buf, NULL);\n\n> + else {\n> + strbuf_release(&buf);\n> + return NULL;\n> + }\n> }\n>\n> /* The argument to filter_refs */\n> --\n> 1.9.1.346.ga2b5940\n"},{"id":"246711","messageId":"CAL=YDWnWAoCrJn28vGKb=xrKKO=fCN=Uz+nM+eh+dYbHCFYpgQ@mail.gmail.com","threadId":"37210","inReplyTo":"1406285039-22469-4-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/4] refs.c: move ref parsing code out of resolve_ref()","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-07-25T16:12:53Z","receivedAt":"2014-07-25T16:12:53Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Nice.\n\nOn Fri, Jul 25, 2014 at 3:43 AM, Nguyễn Thái Ngọc Duy wrote:\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> cache.h | 11 ++++\n> refs.c | 204 ++++++++++++++++++++++++++++++++++------------------------------\n> 2 files changed, 120 insertions(+), 95 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 5ffbafb..40a63d9 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1003,6 +1003,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, \"ref\" can no longer be trusted, \"flag\" may\n> + * be set. errno is valid.\n> + * 0 this is a symref, \"ref\" now contains the linked ref\n> + * +1 normal ref, \"sha1\" is valid\n> + */\n> +extern int parse_ref(const char *path, struct strbuf *ref, 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);\n> diff --git a/refs.c b/refs.c\n> index bec2bb1..2769f20 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1533,6 +1533,105 @@ static int handle_missing_loose_ref(const char *refname,\n> }\n> }\n>\n> +int parse_ref(const char *path, struct strbuf *ref,\n> + unsigned char *sha1, int *flag)\n\nCan you make this function static?\nIt is not used by anything outside of this series and thus making it\nstatic avoids growing the public refs api.\n\n\n> +{\n> + struct strbuf buffer = STRBUF_INIT;\n> + struct stat st;\n> + const char *buf;\n> +\n> + /*\n> + * We might have to loop back here to avoid a race condition:\n> + * first we lstat() the file, then we try to read it as a link\n> + * or as a file. But if somebody changes the type of the file\n> + * (file <-> directory <-> symlink) between the lstat() and\n> + * reading, then we don't want to report that as an error but\n> + * rather try again starting with the lstat().\n> + */\n> +stat_ref:\n> + if (lstat(path, &st) < 0)\n> + return errno == ENOENT ? -2 : -1;\n> +\n> + /* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n> + if (S_ISLNK(st.st_mode)) {\n> + struct strbuf new_path = STRBUF_INIT;\n> + if (strbuf_readlink(&new_path, path, 256) < 0) {\n> + strbuf_release(&new_path);\n> + if (errno == ENOENT || errno == EINVAL)\n> + /* inconsistent with lstat; retry */\n> + goto stat_ref;\n> + else\n> + return -1;\n> + }\n> + if (starts_with(new_path.buf, \"refs/\") &&\n> + !check_refname_format(new_path.buf, 0)) {\n> + strbuf_reset(ref);\n> + strbuf_addbuf(ref, &new_path);\n> + if (flag)\n> + *flag |= REF_ISSYMREF;\n> + strbuf_release(&new_path);\n> + return 0;\n> + }\n> + strbuf_release(&new_path);\n> + }\n> +\n> + /* Is it a directory? */\n> + if (S_ISDIR(st.st_mode)) {\n> + errno = EISDIR;\n> + return -1;\n> + }\n> +\n> + /*\n> + * Anything else, just open it and try to use it as\n> + * a ref\n> + */\n> + if (strbuf_read_file(&buffer, path, 256) < 0) {\n> + strbuf_release(&buffer);\n> + if (errno == ENOENT)\n> + /* inconsistent with lstat; retry */\n> + goto stat_ref;\n> + else\n> + return -1;\n> + }\n> + strbuf_rtrim(&buffer);\n> +\n> + /*\n> + * Is it a symbolic ref?\n> + */\n> + if (!skip_prefix(buffer.buf, \"ref:\", &buf)) {\n> + int ret;\n> + /*\n> + * Please note that FETCH_HEAD has a second line\n> + * containing other data.\n> + */\n> + if (get_sha1_hex(buffer.buf, sha1) ||\n> + (buffer.buf[40] != '\\0' && !isspace(buffer.buf[40]))) {\n> + if (flag)\n> + *flag |= REF_ISBROKEN;\n> + errno = EINVAL;\n> + ret = -1;\n> + } else\n> + ret = 1;\n> + strbuf_release(&buffer);\n> + return ret;\n> + }\n> + if (flag)\n> + *flag |= REF_ISSYMREF;\n> + while (isspace(*buf))\n> + buf++;\n> + if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n> + if (flag)\n> + *flag |= REF_ISBROKEN;\n> + strbuf_release(&buffer);\n> + errno = EINVAL;\n> + return -1;\n> + }\n> + strbuf_reset(ref);\n> + strbuf_addstr(ref, buf);\n> + strbuf_release(&buffer);\n> + return 0;\n> +}\n> +\n> /*\n> * 'result' content will be destroyed. Its value may be undefined if\n> * resolve_ref returns -1.\n> @@ -1542,9 +1641,8 @@ static int handle_missing_loose_ref(const char *refname,\n> int resolve_ref(const char *refname, struct strbuf *result,\n> unsigned char *sha1, int reading, int *flag)\n> {\n> - struct strbuf buffer = STRBUF_INIT;\n> int depth = MAXDEPTH;\n> - int ret = -1;\n> + int ret = 0;\n>\n> if (flag)\n> *flag = 0;\n> @@ -1557,108 +1655,24 @@ int resolve_ref(const char *refname, struct strbuf *result,\n> strbuf_reset(result);\n> strbuf_addstr(result, refname);\n>\n> - for (;;) {\n> + while (!ret) {\n> char path[PATH_MAX];\n> - const char *ref = result->buf;\n> - struct stat st;\n> - const char *buf;\n>\n> if (--depth < 0) {\n> errno = ELOOP;\n> + ret = -1;\n> break;\n> }\n>\n> - git_snpath(path, sizeof(path), \"%s\", ref);\n> -\n> - /*\n> - * We might have to loop back here to avoid a race\n> - * condition: first we lstat() the file, then we try\n> - * to read it as a link or as a file. But if somebody\n> - * changes the type of the file (file <-> directory\n> - * <-> symlink) between the lstat() and reading, then\n> - * we don't want to report that as an error but rather\n> - * try again starting with the lstat().\n> - */\n> - stat_ref:\n> - if (lstat(path, &st) < 0) {\n> - if (errno == ENOENT)\n> - ret = handle_missing_loose_ref(ref, sha1,\n> - reading, flag);\n> - break;\n> + git_snpath(path, sizeof(path), \"%s\", result->buf);\n> + ret = parse_ref(path, result, sha1, flag);\n> + if (ret == -2) {\n> + ret = handle_missing_loose_ref(result->buf, sha1,\n> + reading, flag);\n> + ret = ret ? -1 : 1;\n> }\n> -\n> - /* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n> - if (S_ISLNK(st.st_mode)) {\n> - /* no need to reset buffer, strbuf_readlink does that */\n> - if (strbuf_readlink(&buffer, path, 256) < 0) {\n> - if (errno == ENOENT || errno == EINVAL)\n> - /* inconsistent with lstat; retry */\n> - goto stat_ref;\n> - else\n> - break;\n> - }\n> - if (starts_with(buffer.buf, \"refs/\") &&\n> - !check_refname_format(buffer.buf, 0)) {\n> - strbuf_reset(result);\n> - strbuf_addbuf(result, &buffer);\n> - if (flag)\n> - *flag |= REF_ISSYMREF;\n> - continue;\n> - }\n> - }\n> -\n> - /* Is it a directory? */\n> - if (S_ISDIR(st.st_mode)) {\n> - errno = EISDIR;\n> - break;\n> - }\n> -\n> - /*\n> - * Anything else, just open it and try to use it as\n> - * a ref\n> - */\n> - strbuf_reset(&buffer);\n> - if (strbuf_read_file(&buffer, path, 256) < 0) {\n> - if (errno == ENOENT)\n> - /* inconsistent with lstat; retry */\n> - goto stat_ref;\n> - else\n> - break;\n> - }\n> - strbuf_rtrim(&buffer);\n> -\n> - /*\n> - * Is it a symbolic ref?\n> - */\n> - if (!skip_prefix(buffer.buf, \"ref:\", &buf)) {\n> - /*\n> - * Please note that FETCH_HEAD has a second\n> - * line containing other data.\n> - */\n> - if (get_sha1_hex(buffer.buf, sha1) ||\n> - (buffer.buf[40] != '\\0' && !isspace(buffer.buf[40]))) {\n> - if (flag)\n> - *flag |= REF_ISBROKEN;\n> - errno = EINVAL;\n> - } else\n> - ret = 0;\n> - break;\n> - }\n> - if (flag)\n> - *flag |= REF_ISSYMREF;\n> - while (isspace(*buf))\n> - buf++;\n> - if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n> - if (flag)\n> - *flag |= REF_ISBROKEN;\n> - errno = EINVAL;\n> - break;\n> - }\n> - strbuf_reset(result);\n> - strbuf_addstr(result, buf);\n> }\n> - strbuf_release(&buffer);\n> - return ret;\n> + return ret > 0 ? 0 : -1;\n> }\n>\n> const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n> --\n> 1.9.1.346.ga2b5940\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"246754","messageId":"CACsJy8DWEuiyyj0HhnGttnbyQ2+tTWDAW5H67uBXeV4ESM6seg@mail.gmail.com","threadId":"37210","inReplyTo":"CAL=YDWnWAoCrJn28vGKb=xrKKO=fCN=Uz+nM+eh+dYbHCFYpgQ@mail.gmail.com","subject":"Re: [PATCH 3/4] refs.c: move ref parsing code out of resolve_ref()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-26T01:50:44Z","receivedAt":"2014-07-26T01:50:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jul 25, 2014 at 11:12 PM, Ronnie Sahlberg wrote:\n>> diff --git a/refs.c b/refs.c\n>> index bec2bb1..2769f20 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -1533,6 +1533,105 @@ static int handle_missing_loose_ref(const char *refname,\n>> }\n>> }\n>>\n>> +int parse_ref(const char *path, struct strbuf *ref,\n>> + unsigned char *sha1, int *flag)\n>\n> Can you make this function static?\n> It is not used by anything outside of this series and thus making it\n> static avoids growing the public refs api.\n\nIt's to be used by builtin/checkout.c in nd/multiple-work-trees. I\ncould mark it static now and unmark it later, but I'd need to add the\nstatic prototype back in refs.c because in the next patch\nresolve_gitlink_ref() uses this function and resolve_gitlink_ref() is\nbefore parse_ref().\n-- \nDuy\n"},{"id":"246991","messageId":"xmqqlhray668.fsf@gitster.dls.corp.google.com","threadId":"37210","inReplyTo":"CAPig+cSvymyM3G+LujopPAVtaXVYN_mYJ0pxPkvb2pvLsq+e-A@mail.gmail.com","subject":"Re: [PATCH 2/4] refs.c: refactor resolve_ref_unsafe() to use strbuf internally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-30T19:53:35Z","receivedAt":"2014-07-30T19:53:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)\n>> {\n>> - const char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);\n>> - return ret ? xstrdup(ret) : NULL;\n>> + struct strbuf buf = STRBUF_INIT;\n>> + if (!resolve_ref(ref, &buf, sha1, reading, flag))\n>> + return buf.buf;\n>\n> return strbuf_detach(&buf, NULL);\n\nYeah, the end result is the same, but it is a very good discipline.\n"},{"id":"249913","messageId":"5425404E.7050100@alum.mit.edu","threadId":"37210","inReplyTo":"1406285039-22469-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/4] strbuf.c: keep errno in strbuf_read_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-09-26T10:30:38Z","receivedAt":"2014-09-26T10:30:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/25/2014 12:43 PM, Nguyễn Thái Ngọc Duy wrote:\n> This function is used to replaced some code in the next patch that\n> does this (i.e. keep the errno when read() fails)\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> strbuf.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/strbuf.c b/strbuf.c\n> index 33018d8..61d685d 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -454,15 +454,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\nTheoretically close() can fail, though it seems a little far-fetched\n(and also uninteresting) if it fails for a file opened read-only. But if\nit did, you would not notice the error.\n\nSo I grepped through our code to see whether we typically bother to\ncheck the return value when close()ing a read-only file. And I found\nthat we rarely even check its return value when *writing* to a file.\n(Many of those places are probably bugs.)\n\nSo, carry on and forget I said anything :-)\n\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> \n\nReviewed-by: Michael Haggerty \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}