{"thread":{"id":"16769","subject":"[PATCH 0/5] Be careful about lstat()-vs-readlink()","startedAt":"2008-12-17T18:42:02Z","lastAt":"2009-01-07T21:19:19Z","messageCount":33,"participants":["Linus Torvalds","Junio C Hamano","Jay Soffian","Mark Burton","René Scharfe","Olivier Galibert","Pierre Habouzit"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"98157","messageId":"alpine.LFD.2.00.0812171034520.14014@localhost.localdomain","threadId":"16769","inReplyTo":null,"subject":"[PATCH 0/5] Be careful about lstat()-vs-readlink()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-17T18:42:02Z","receivedAt":"2008-12-17T18:42:02Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nThis series of five patches makes us a lot more careful about readlink(), \nand in particular it avoids the code that depends on doing an lstat(), and \nthen using st_size as the length of the link. That's what POSIX says \n_should_ happen, but Ramon Tayag reported git failing on NTFS under Linux \nbecause st_size doesn't match readlink() return value.\n\nIt doesn't seem to be unknown elsewhere either, since coreutils (through \ngnulib's areadlink_with_size) also has a big compatibility layer around \nreadlink().\n\nThis series has been 'tested' by running it with the appended totally \nhacky patch that fakes out the lstat() st_size values for symlinks, so it \nhas actually had some testing. \n\nThe complete series does:\n\nLinus Torvalds (5):\n  Add generic 'strbuf_readlink()' helper function\n  Make 'ce_compare_link()' use the new 'strbuf_readlink()'\n  Make 'index_path()' use 'strbuf_readlink()'\n  Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'\n  Make 'prepare_temp_file()' ignore st_size for symlinks\n\n builtin-apply.c |    6 ++----\n diff.c          |   25 +++++++++++--------------\n read-cache.c    |   22 ++++++++--------------\n sha1_file.c     |   14 +++++---------\n strbuf.c        |   28 ++++++++++++++++++++++++++++\n strbuf.h        |    1 +\n 6 files changed, 55 insertions(+), 41 deletions(-)\n\nand the hacky test-patch (which is obviously _not_ meant to be applied) is \nas follows...\n\n\t\tLinus\n\n---\ndiff --git a/cache.h b/cache.h\nindex 231c06d..2d85fca 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -943,4 +943,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix);\n char *alias_lookup(const char *alias);\n int split_cmdline(char *cmdline, const char ***argv);\n \n+extern int gitlstat(const char *, struct stat *);\n+#define lstat gitlstat\n+\n #endif /* CACHE_H */\ndiff --git a/read-cache.c b/read-cache.c\nindex b1475ff..defbb20 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -15,6 +15,16 @@\n #include \"revision.h\"\n #include \"blob.h\"\n \n+#undef lstat\n+int gitlstat(const char *path, struct stat *buf)\n+{\n+\tint retval = lstat(path, buf);\n+\tif (!retval && S_ISLNK(buf->st_mode))\n+\t\tbuf->st_size = 1;\n+\treturn retval;\n+}\n+#define lstat gitlstat\n+\n /* Index extensions.\n  *\n  * The first letter should be 'A'..'Z' for extensions that are not\n"},{"id":"98158","messageId":"alpine.LFD.2.00.0812171042120.14014@localhost.localdomain","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171034520.14014@localhost.localdomain","subject":"[PATCH 1/5] Add generic 'strbuf_readlink()' helper function","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-17T18:42:43Z","receivedAt":"2008-12-17T18:42:43Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds <torvalds@linux-foundation.org>\nDate: Wed, 17 Dec 2008 09:36:40 -0800\n\nIt was already what 'git apply' did in read_old_data(), just export it\nas a real function, and make it be more generic.\n\nIn particular, this handles the case of the lstat() st_size data not\nmatching the readlink() return value properly (which apparently happens\nat least on NTFS under Linux).  But as a result of this you could also\nuse the new function without even knowing how big the link is going to\nbe, and it will allocate an appropriately sized buffer.\n\nSo we pass in the st_size of the link as just a hint, rather than a\nfixed requirement.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n builtin-apply.c |    6 ++----\n strbuf.c        |   28 ++++++++++++++++++++++++++++\n strbuf.h        |    1 +\n 3 files changed, 31 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 4c4d1e1..07244b0 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1559,10 +1559,8 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)\n {\n \tswitch (st->st_mode & S_IFMT) {\n \tcase S_IFLNK:\n-\t\tstrbuf_grow(buf, st->st_size);\n-\t\tif (readlink(path, buf->buf, st->st_size) != st->st_size)\n-\t\t\treturn -1;\n-\t\tstrbuf_setlen(buf, st->st_size);\n+\t\tif (strbuf_readlink(buf, path, st->st_size) < 0)\n+\t\t\treturn error(\"unable to read symlink %s\", path);\n \t\treturn 0;\n \tcase S_IFREG:\n \t\tif (strbuf_read_file(buf, path, st->st_size) != st->st_size)\ndiff --git a/strbuf.c b/strbuf.c\nindex 13be67e..904a2b0 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -288,6 +288,34 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \treturn sb->len - oldlen;\n }\n \n+#define STRBUF_MAXLINK (2*PATH_MAX)\n+\n+int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n+{\n+\tif (hint < 32)\n+\t\thint = 32;\n+\n+\twhile (hint < STRBUF_MAXLINK) {\n+\t\tint len;\n+\n+\t\tstrbuf_grow(sb, hint);\n+\t\tlen = readlink(path, sb->buf, hint);\n+\t\tif (len < 0) {\n+\t\t\tif (errno != ERANGE)\n+\t\t\t\tbreak;\n+\t\t} else if (len < hint) {\n+\t\t\tstrbuf_setlen(sb, len);\n+\t\t\treturn 0;\n+\t\t}\n+\n+\t\t/* .. the buffer was too small - try again */\n+\t\thint *= 2;\n+\t\tcontinue;\n+\t}\n+\tstrbuf_release(sb);\n+\treturn -1;\n+}\n+\n int strbuf_getline(struct strbuf *sb, FILE *fp, int term)\n {\n \tint ch;\ndiff --git a/strbuf.h b/strbuf.h\nindex b1670d9..89bd36e 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -124,6 +124,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n /* XXX: if read fails, any partial read is undone */\n extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n+extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n \n extern int strbuf_getline(struct strbuf *, FILE *, int);\n \n-- \n1.6.1.rc3.3.gcc3e3\n"},{"id":"98159","messageId":"alpine.LFD.2.00.0812171042500.14014@localhost.localdomain","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171042120.14014@localhost.localdomain","subject":"[PATCH 2/5] Make 'ce_compare_link()' use the new 'strbuf_readlink()'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-17T18:43:12Z","receivedAt":"2008-12-17T18:43:12Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds <torvalds@linux-foundation.org>\nDate: Wed, 17 Dec 2008 09:47:27 -0800\n\nThis simplifies the code, and also makes ce_compare_link now able to\nhandle filesystems with odd 'st_size' return values for symlinks.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n read-cache.c |   22 ++++++++--------------\n 1 files changed, 8 insertions(+), 14 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex c14b562..b1475ff 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -99,27 +99,21 @@ static int ce_compare_data(struct cache_entry *ce, struct stat *st)\n static int ce_compare_link(struct cache_entry *ce, size_t expected_size)\n {\n \tint match = -1;\n-\tchar *target;\n \tvoid *buffer;\n \tunsigned long size;\n \tenum object_type type;\n-\tint len;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n-\ttarget = xmalloc(expected_size);\n-\tlen = readlink(ce->name, target, expected_size);\n-\tif (len != expected_size) {\n-\t\tfree(target);\n+\tif (strbuf_readlink(&sb, ce->name, expected_size))\n \t\treturn -1;\n-\t}\n+\n \tbuffer = read_sha1_file(ce->sha1, &type, &size);\n-\tif (!buffer) {\n-\t\tfree(target);\n-\t\treturn -1;\n+\tif (buffer) {\n+\t\tif (size == sb.len)\n+\t\t\tmatch = memcmp(buffer, sb.buf, size);\n+\t\tfree(buffer);\n \t}\n-\tif (size == expected_size)\n-\t\tmatch = memcmp(buffer, target, size);\n-\tfree(buffer);\n-\tfree(target);\n+\tstrbuf_release(&sb);\n \treturn match;\n }\n \n-- \n1.6.1.rc3.3.gcc3e3\n"},{"id":"98160","messageId":"alpine.LFD.2.00.0812171043180.14014@localhost.localdomain","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171042500.14014@localhost.localdomain","subject":"[PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-17T18:43:40Z","receivedAt":"2008-12-17T18:43:40Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds <torvalds@linux-foundation.org>\nDate: Wed, 17 Dec 2008 09:51:53 -0800\n\nThis makes us able to properly index symlinks even on filesystems where\nst_size doesn't match the true size of the link.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n sha1_file.c |   14 +++++---------\n 1 files changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 0e021c5..52d1ead 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2523,8 +2523,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object)\n {\n \tint fd;\n-\tchar *target;\n-\tsize_t len;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n \tswitch (st->st_mode & S_IFMT) {\n \tcase S_IFREG:\n@@ -2537,20 +2536,17 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, int write\n \t\t\t\t     path);\n \t\tbreak;\n \tcase S_IFLNK:\n-\t\tlen = xsize_t(st->st_size);\n-\t\ttarget = xmalloc(len + 1);\n-\t\tif (readlink(path, target, len + 1) != st->st_size) {\n+\t\tif (strbuf_readlink(&sb, path, st->st_size)) {\n \t\t\tchar *errstr = strerror(errno);\n-\t\t\tfree(target);\n \t\t\treturn error(\"readlink(\\\"%s\\\"): %s\", path,\n \t\t\t             errstr);\n \t\t}\n \t\tif (!write_object)\n-\t\t\thash_sha1_file(target, len, blob_type, sha1);\n-\t\telse if (write_sha1_file(target, len, blob_type, sha1))\n+\t\t\thash_sha1_file(sb.buf, sb.len, blob_type, sha1);\n+\t\telse if (write_sha1_file(sb.buf, sb.len, blob_type, sha1))\n \t\t\treturn error(\"%s: failed to insert into database\",\n \t\t\t\t     path);\n-\t\tfree(target);\n+\t\tstrbuf_release(&sb);\n \t\tbreak;\n \tcase S_IFDIR:\n \t\treturn resolve_gitlink_ref(path, \"HEAD\", sha1);\n-- \n1.6.1.rc3.3.gcc3e3\n"},{"id":"98161","messageId":"alpine.LFD.2.00.0812171043440.14014@localhost.localdomain","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171043180.14014@localhost.localdomain","subject":"[PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-17T18:44:06Z","receivedAt":"2008-12-17T18:44:06Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds <torvalds@linux-foundation.org>\nDate: Wed, 17 Dec 2008 10:26:13 -0800\n\nThis makes all tests pass on a system where 'lstat()' has been hacked to\nreturn bogus data in st_size for symlinks.\n\nOf course, the test coverage isn't complete, but it's a good baseline.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n diff.c |   16 +++++++---------\n 1 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex afefe08..4b2029c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1773,19 +1773,17 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\ts->size = xsize_t(st.st_size);\n \t\tif (!s->size)\n \t\t\tgoto empty;\n-\t\tif (size_only)\n-\t\t\treturn 0;\n \t\tif (S_ISLNK(st.st_mode)) {\n-\t\t\tint ret;\n-\t\t\ts->data = xmalloc(s->size);\n-\t\t\ts->should_free = 1;\n-\t\t\tret = readlink(s->path, s->data, s->size);\n-\t\t\tif (ret < 0) {\n-\t\t\t\tfree(s->data);\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\t\tif (strbuf_readlink(&sb, s->path, s->size))\n \t\t\t\tgoto err_empty;\n-\t\t\t}\n+\t\t\ts->data = strbuf_detach(&sb, &s->size);\n+\t\t\ts->should_free = 1;\n \t\t\treturn 0;\n \t\t}\n+\t\tif (size_only)\n+\t\t\treturn 0;\n \t\tfd = open(s->path, O_RDONLY);\n \t\tif (fd < 0)\n \t\t\tgoto err_empty;\n-- \n1.6.1.rc3.3.gcc3e3\n"},{"id":"98162","messageId":"alpine.LFD.2.00.0812171044110.14014@localhost.localdomain","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171043440.14014@localhost.localdomain","subject":"[PATCH 5/5] Make 'prepare_temp_file()' ignore st_size for symlinks","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-17T18:45:50Z","receivedAt":"2008-12-17T18:45:50Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds <torvalds@linux-foundation.org>\nDate: Wed, 17 Dec 2008 10:31:36 -0800\n\nThe code was already set up to not really need it, so this just massages\nit a bit to remove the use entirely.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nThere's a few raw 'readlink()' calls left, but they all seem to just have \na big buffer and rely on the return value of readlink() rather than look \ntoo closely at st_size. But it would probably be good if somebody \ndouble-checked it all. \n\nEven so, this should all be better than what we used to have, though.\n\n diff.c |    9 ++++-----\n 1 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 4b2029c..f160c1a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1881,13 +1881,12 @@ static void prepare_temp_file(const char *name,\n \t\tif (S_ISLNK(st.st_mode)) {\n \t\t\tint ret;\n \t\t\tchar buf[PATH_MAX + 1]; /* ought to be SYMLINK_MAX */\n-\t\t\tsize_t sz = xsize_t(st.st_size);\n-\t\t\tif (sizeof(buf) <= st.st_size)\n-\t\t\t\tdie(\"symlink too long: %s\", name);\n-\t\t\tret = readlink(name, buf, sz);\n+\t\t\tret = readlink(name, buf, sizeof(buf));\n \t\t\tif (ret < 0)\n \t\t\t\tdie(\"readlink(%s)\", name);\n-\t\t\tprep_temp_blob(temp, buf, sz,\n+\t\t\tif (ret == sizeof(buf))\n+\t\t\t\tdie(\"symlink too long: %s\", name);\n+\t\t\tprep_temp_blob(temp, buf, ret,\n \t\t\t\t       (one->sha1_valid ?\n \t\t\t\t\tone->sha1 : null_sha1),\n \t\t\t\t       (one->sha1_valid ?\n-- \n1.6.1.rc3.3.gcc3e3\n"},{"id":"98176","messageId":"7vskomler1.fsf@gitster.siamese.dyndns.org","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171043180.14014@localhost.localdomain","subject":"Re: [PATCH 3/5] Make 'index_path()' use 'strbuf_readlink()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-17T20:37:38Z","receivedAt":"2008-12-17T20:37:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> @@ -2537,20 +2536,17 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, int write\n>  \t\t\t\t     path);\n>  \t\tbreak;\n>  \tcase S_IFLNK:\n> -\t\tlen = xsize_t(st->st_size);\n> -\t\ttarget = xmalloc(len + 1);\n> -\t\tif (readlink(path, target, len + 1) != st->st_size) {\n> +\t\tif (strbuf_readlink(&sb, path, st->st_size)) {\n>  \t\t\tchar *errstr = strerror(errno);\n> -\t\t\tfree(target);\n>  \t\t\treturn error(\"readlink(\\\"%s\\\"): %s\", path,\n>  \t\t\t             errstr);\n\nThanks; as strbuf_readlink() does not do any iffy library calls that would\nstomp on errno, the error reporting should still be valid here.\n"},{"id":"98177","messageId":"7vmyeuleqw.fsf@gitster.siamese.dyndns.org","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171043440.14014@localhost.localdomain","subject":"Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-17T20:37:43Z","receivedAt":"2008-12-17T20:37:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> diff --git a/diff.c b/diff.c\n> index afefe08..4b2029c 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1773,19 +1773,17 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n>  \t\ts->size = xsize_t(st.st_size);\n>  \t\tif (!s->size)\n>  \t\t\tgoto empty;\n> -\t\tif (size_only)\n> -\t\t\treturn 0;\n>  \t\tif (S_ISLNK(st.st_mode)) {\n>  ...\n>  \t\t}\n> +\t\tif (size_only)\n> +\t\t\treturn 0;\n\nIt is unfortunate that we need to always readlink even when we only would\nwant to cull differences early (e.g. --raw without any fancy filters such\nas rename detection), but symbolic links should be minorities in any sane\nrepo, and it should not be worth trying to optimize this for sane\nfilesystems by making it conditional.\n"},{"id":"98179","messageId":"7vhc52leqr.fsf@gitster.siamese.dyndns.org","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171044110.14014@localhost.localdomain","subject":"[PATCH 6/5] make_absolute_path(): check bounds when seeing an overlong symlink","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-17T20:37:48Z","receivedAt":"2008-12-17T20:37:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n abspath.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git i/abspath.c w/abspath.c\nindex 8194ce1..649f34f 100644\n--- i/abspath.c\n+++ w/abspath.c\n@@ -64,6 +64,8 @@ const char *make_absolute_path(const char *path)\n \t\t\tlen = readlink(buf, next_buf, PATH_MAX);\n \t\t\tif (len < 0)\n \t\t\t\tdie (\"Invalid symlink: %s\", buf);\n+\t\t\tif (PATH_MAX <= len)\n+\t\t\t\tdie(\"symbolic link too long: %s\", buf);\n \t\t\tnext_buf[len] = '\\0';\n \t\t\tbuf = next_buf;\n \t\t\tbuf_index = 1 - buf_index;\n"},{"id":"98178","messageId":"7vbpvaleqm.fsf@gitster.siamese.dyndns.org","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171044110.14014@localhost.localdomain","subject":"[PATCH 7/5] builtin-blame.c: use strbuf_readlink()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-17T20:37:53Z","receivedAt":"2008-12-17T20:37:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When faking a commit out of the work tree contents, use strbuf_readlink()\nto read the contents of symbolic links.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-blame.c |    5 +----\n 1 files changed, 1 insertions(+), 4 deletions(-)\n\ndiff --git i/builtin-blame.c w/builtin-blame.c\nindex a0d6014..aae14ef 100644\n--- i/builtin-blame.c\n+++ w/builtin-blame.c\n@@ -1996,7 +1996,6 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \tif (!contents_from || strcmp(\"-\", contents_from)) {\n \t\tstruct stat st;\n \t\tconst char *read_from;\n-\t\tunsigned long fin_size;\n \n \t\tif (contents_from) {\n \t\t\tif (stat(contents_from, &st) < 0)\n@@ -2008,7 +2007,6 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \t\t\t\tdie(\"Cannot lstat %s\", path);\n \t\t\tread_from = path;\n \t\t}\n-\t\tfin_size = xsize_t(st.st_size);\n \t\tmode = canon_mode(st.st_mode);\n \t\tswitch (st.st_mode & S_IFMT) {\n \t\tcase S_IFREG:\n@@ -2016,9 +2014,8 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \t\t\t\tdie(\"cannot open or read %s\", read_from);\n \t\t\tbreak;\n \t\tcase S_IFLNK:\n-\t\t\tif (readlink(read_from, buf.buf, buf.alloc) != fin_size)\n+\t\t\tif (strbuf_readlink(&buf, read_from, st.st_size) < 0)\n \t\t\t\tdie(\"cannot readlink %s\", read_from);\n-\t\t\tbuf.len = fin_size;\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tdie(\"unsupported file type %s\", read_from);\n"},{"id":"98180","messageId":"7v63lileqh.fsf@gitster.siamese.dyndns.org","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171044110.14014@localhost.localdomain","subject":"[PATCH 8/5] combine-diff.c: use strbuf_readlink()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-17T20:37:58Z","receivedAt":"2008-12-17T20:37:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When showing combined diff using work tree contents, use strbuf_readlink()\nto read symbolic links.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n combine-diff.c |   10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git i/combine-diff.c w/combine-diff.c\nindex ec8df39..bccc018 100644\n--- i/combine-diff.c\n+++ w/combine-diff.c\n@@ -703,15 +703,15 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tgoto deleted_file;\n \n \t\tif (S_ISLNK(st.st_mode)) {\n-\t\t\tsize_t len = xsize_t(st.st_size);\n-\t\t\tresult_size = len;\n-\t\t\tresult = xmalloc(len + 1);\n-\t\t\tif (result_size != readlink(elem->path, result, len)) {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\t\tif (strbuf_readlink(&buf, elem->path, st.st_size) < 0) {\n \t\t\t\terror(\"readlink(%s): %s\", elem->path,\n \t\t\t\t      strerror(errno));\n \t\t\t\treturn;\n \t\t\t}\n-\t\t\tresult[len] = 0;\n+\t\t\tresult_size = buf.len;\n+\t\t\tresult = strbuf_detach(&buf, NULL);\n \t\t\telem->mode = canon_mode(st.st_mode);\n \t\t}\n \t\telse if (0 <= (fd = open(elem->path, O_RDONLY)) &&\n"},{"id":"98183","messageId":"alpine.LFD.2.00.0812171300070.14014@localhost.localdomain","threadId":"16769","inReplyTo":"7v63lileqh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 8/5] combine-diff.c: use strbuf_readlink()","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-17T21:02:42Z","receivedAt":"2008-12-17T21:02:42Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 17 Dec 2008, Junio C Hamano wrote:\n> -\t\t\tresult[len] = 0;\n> +\t\t\tresult_size = buf.len;\n> +\t\t\tresult = strbuf_detach(&buf, NULL);\n\nIf result_size was made size_t, this would be\n\n\tresult = strbuf_detach(&buf, &result_size);\n\nBut whether it makes any difference, I dunno.\n\nAnyway, Ack on the 6-8 additions.\n\n\t\tLinus\n"},{"id":"98185","messageId":"76718490812171326q4d8896c1yb535873c71eec23f@mail.gmail.com","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171042120.14014@localhost.localdomain","subject":"Re: [PATCH 1/5] Add generic 'strbuf_readlink()' helper function","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2008-12-17T21:26:36Z","receivedAt":"2008-12-17T21:26:36Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Dec 17, 2008 at 1:42 PM, Linus Torvalds\n<torvalds@linux-foundation.org> wrote:\n> +       while (hint < STRBUF_MAXLINK) {\n> +               int len;\n> +\n> +               strbuf_grow(sb, hint);\n> +               len = readlink(path, sb->buf, hint);\n> +               if (len < 0) {\n> +                       if (errno != ERANGE)\n> +                               break;\n> +               } else if (len < hint) {\n> +                       strbuf_setlen(sb, len);\n> +                       return 0;\n> +               }\n> +\n> +               /* .. the buffer was too small - try again */\n> +               hint *= 2;\n> +               continue;\n> +       }\n\nWhy the continue statement at the end of the loop?\n\nj.\n"},{"id":"98187","messageId":"7vmyeujxjp.fsf@gitster.siamese.dyndns.org","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171300070.14014@localhost.localdomain","subject":"Re: [PATCH 8/5] combine-diff.c: use strbuf_readlink()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-17T21:34:34Z","receivedAt":"2008-12-17T21:34:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Wed, 17 Dec 2008, Junio C Hamano wrote:\n>> -\t\t\tresult[len] = 0;\n>> +\t\t\tresult_size = buf.len;\n>> +\t\t\tresult = strbuf_detach(&buf, NULL);\n>\n> If result_size was made size_t, this would be\n>\n> \tresult = strbuf_detach(&buf, &result_size);\n>\n> But whether it makes any difference, I dunno.\n\nYeah, use of \"unsigned long\" where \"size_t\" could be more appropriate\nstems from the very initial commit e83c516 (Initial revision of \"git\", the\ninformation manager from hell, 2005-04-07) and it is everywhere, and\nupdating them one by one like you suggest would take forever ;-)\n\nPerhaps libgit2 would settle with a better typing system.  The original\ndraft by Shawn looked a bit too overengineered in its use of typedefs, but\nif I recall correctly later revisions were made saner.  I haven't checked\nits current status.\n"},{"id":"98188","messageId":"alpine.LFD.2.00.0812171343230.14014@localhost.localdomain","threadId":"16769","inReplyTo":"76718490812171326q4d8896c1yb535873c71eec23f@mail.gmail.com","subject":"Re: [PATCH 1/5] Add generic 'strbuf_readlink()' helper function","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-17T21:44:12Z","receivedAt":"2008-12-17T21:44:12Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 17 Dec 2008, Jay Soffian wrote:\n> > +               /* .. the buffer was too small - try again */\n> > +               hint *= 2;\n> > +               continue;\n> > +       }\n> \n> Why the continue statement at the end of the loop?\n\nOh, it's unnecessary left-over from me originally writing that part inside \nan if-statement, and then moving things around.\n\nSo it could/should be deleted. Good eyes.\n\n\t\tLinus\n"},{"id":"98248","messageId":"20081218121118.3635c53c@crow","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171043440.14014@localhost.localdomain","subject":"Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'","fromName":"Mark Burton","fromEmail":"markb@ordern.com","sentAt":"2008-12-18T12:11:18Z","receivedAt":"2008-12-18T12:11:18Z","isPatch":true,"sender":{"key":"markb@ordern.com","avatar":null},"body":"\nHowdy folks,\n\nWhen I compile this latest version of diff.c on a i686 dual-core Pentium box\nI see:\n\ndiff.c: In function ‘diff_populate_filespec’:\ndiff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type\n\nThe same code compiles without warning on a x86_64 AMD box. Both\nmachines are running stock Ubuntu 8.04.\n\nDoes it need a cast on some architectures?\n\nCheers,\n\nMark\n"},{"id":"98263","messageId":"alpine.LFD.2.00.0812180851120.14014@localhost.localdomain","threadId":"16769","inReplyTo":"20081218121118.3635c53c@crow","subject":"Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-18T16:55:50Z","receivedAt":"2008-12-18T16:55:50Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 18 Dec 2008, Mark Burton wrote:\n> \n> Does it need a cast on some architectures?\n\nGaah. My bad. It should work fine (\"unsigned long\" is physically the same \ntype as \"size_t\" in your case), but on 32-bit x86, size_t is generally \n\"unsigned int\" - which is the same physical type there (both int and long \nare 32-bit) but causes a valid warning.\n\nI think we should just make the \"size\" member \"size_t\". I use \"unsigned \nlong\" out of much too long habit, since we traditionally avoided \"size_t\" \nin the kernel due to it just being another unnecessary architecture- \nspecific detail.\n\nSo the proper patch is probably just the following. Sorry about that,\n\n\t\tLinus\n---\n diffcore.h |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/diffcore.h b/diffcore.h\nindex 5b63458..16a73e6 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -30,7 +30,7 @@ struct diff_filespec {\n \tvoid *data;\n \tvoid *cnt_data;\n \tconst char *funcname_pattern_ident;\n-\tunsigned long size;\n+\tsize_t size;\n \tint count;               /* Reference count */\n \tint xfrm_flags;\t\t /* for use by the xfrm */\n \tint rename_used;         /* Count of rename users */\n"},{"id":"98264","messageId":"494A80D3.7070605@lsrfire.ath.cx","threadId":"16769","inReplyTo":"20081218121118.3635c53c@crow","subject":"Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2008-12-18T16:56:51Z","receivedAt":"2008-12-18T16:56:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Mark Burton schrieb:\n> Howdy folks,\n> \n> When I compile this latest version of diff.c on a i686 dual-core Pentium box\n> I see:\n> \n> diff.c: In function ‘diff_populate_filespec’:\n> diff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type\n> \n> The same code compiles without warning on a x86_64 AMD box. Both\n> machines are running stock Ubuntu 8.04.\n> \n> Does it need a cast on some architectures?\n\nThe type of the size member of struct stat is off_t, while strbuf_detach expects\na size_t pointer.  This patch should fix the warning:\n\ndiff --git a/diff.c b/diff.c\nindex f160c1a..0484601 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1778,7 +1778,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \n \t\t\tif (strbuf_readlink(&sb, s->path, s->size))\n \t\t\t\tgoto err_empty;\n-\t\t\ts->data = strbuf_detach(&sb, &s->size);\n+\t\t\ts->size = sb.len;\n+\t\t\ts->data = strbuf_detach(&sb, NULL);\n \t\t\ts->should_free = 1;\n \t\t\treturn 0;\n \t\t}\n"},{"id":"98266","messageId":"494A8822.8090601@lsrfire.ath.cx","threadId":"16769","inReplyTo":"494A80D3.7070605@lsrfire.ath.cx","subject":"Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2008-12-18T17:28:02Z","receivedAt":"2008-12-18T17:28:02Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"René Scharfe schrieb:\n> Mark Burton schrieb:\n>> Howdy folks,\n>>\n>> When I compile this latest version of diff.c on a i686 dual-core Pentium box\n>> I see:\n>>\n>> diff.c: In function ‘diff_populate_filespec’:\n>> diff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type\n>>\n>> The same code compiles without warning on a x86_64 AMD box. Both\n>> machines are running stock Ubuntu 8.04.\n>>\n>> Does it need a cast on some architectures?\n> \n> The type of the size member of struct stat is off_t, while strbuf_detach expects\n> a size_t pointer.  This patch should fix the warning:\n\nNevermind, I somehow missed the last \"t\" in line 1760..  The patch works\nnevertheless. 8-)\n\nRené\n"},{"id":"98268","messageId":"494A8B57.6070106@lsrfire.ath.cx","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812180851120.14014@localhost.localdomain","subject":"Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2008-12-18T17:41:43Z","receivedAt":"2008-12-18T17:41:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Linus Torvalds schrieb:\n> \n> On Thu, 18 Dec 2008, Mark Burton wrote:\n>> Does it need a cast on some architectures?\n> \n> Gaah. My bad. It should work fine (\"unsigned long\" is physically the same \n> type as \"size_t\" in your case), but on 32-bit x86, size_t is generally \n> \"unsigned int\" - which is the same physical type there (both int and long \n> are 32-bit) but causes a valid warning.\n> \n> I think we should just make the \"size\" member \"size_t\". I use \"unsigned \n> long\" out of much too long habit, since we traditionally avoided \"size_t\" \n> in the kernel due to it just being another unnecessary architecture- \n> specific detail.\n> \n> So the proper patch is probably just the following. Sorry about that,\n> \n> \t\tLinus\n> ---\n>  diffcore.h |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/diffcore.h b/diffcore.h\n> index 5b63458..16a73e6 100644\n> --- a/diffcore.h\n> +++ b/diffcore.h\n> @@ -30,7 +30,7 @@ struct diff_filespec {\n>  \tvoid *data;\n>  \tvoid *cnt_data;\n>  \tconst char *funcname_pattern_ident;\n> -\tunsigned long size;\n> +\tsize_t size;\n>  \tint count;               /* Reference count */\n>  \tint xfrm_flags;\t\t /* for use by the xfrm */\n>  \tint rename_used;         /* Count of rename users */\n\nYes, but now I get two new warnings:\n\ndiff.c: In function `diff_populate_filespec':\ndiff.c:1809: warning: passing arg 2 of `sha1_object_info' from\nincompatible pointer type\ndiff.c:1811: warning: passing arg 3 of `read_sha1_file' from\nincompatible pointer type\n\nIf we followed that way along we'd convert just about everything to use\nsize_t, which is going a bit too far during the -rc phase..\n\nRené\n\n\nPS: In the other subthread, I was missing the \"t\" in \"st\" in line 1757,\nnot 1760.  Ahem.\n"},{"id":"98269","messageId":"alpine.LFD.2.00.0812180945330.14014@localhost.localdomain","threadId":"16769","inReplyTo":"494A8B57.6070106@lsrfire.ath.cx","subject":"Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-18T17:49:23Z","receivedAt":"2008-12-18T17:49:23Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 18 Dec 2008, René Scharfe wrote:\n> \n> Yes, but now I get two new warnings:\n> \n> diff.c: In function `diff_populate_filespec':\n> diff.c:1809: warning: passing arg 2 of `sha1_object_info' from\n> incompatible pointer type\n> diff.c:1811: warning: passing arg 3 of `read_sha1_file' from\n> incompatible pointer type\n\nYeah, yeah, we should probably fix it all, but you're right, your patch \navoids the pain.\n\nThe good news is that \"size_t\" and \"unsigned long\" really are the same \nsize on all sane platforms. You'll see differences just in the odd 16-bit \nworld (\"small\" memory model with 16-bit size_t and possibly 32-bit \"long\") \n\nAlthough I guess some really odd more modern case might have a 32-bit \npointer (and size_t) and 64-bit long. If it's possible to screw the type \nsystem up, history shows that somebody (usually Windows) will do it.\n\n\t\tLinus\n"},{"id":"98270","messageId":"20081218175601.GC29040@dspnet.fr.eu.org","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812180945330.14014@localhost.localdomain","subject":"Re: [PATCH 4/5] Make 'diff_populate_filespec()' use the new 'strbuf_readlink()'","fromName":"Olivier Galibert","fromEmail":"galibert@pobox.com","sentAt":"2008-12-18T17:56:01Z","receivedAt":"2008-12-18T17:56:01Z","isPatch":true,"sender":{"key":"galibert@pobox.com","avatar":null},"body":"On Thu, Dec 18, 2008 at 09:49:23AM -0800, Linus Torvalds wrote:\n> If it's possible to screw the type system up, history shows that\n> somebody (usually Windows) will do it.\n\nAFAIK, the Win64 long is 32 bits and size_t/void * is 64 bits.  I'll\nleave you to your groaning.\n\n  OG.\n"},{"id":"98378","messageId":"494C1BE8.20607@lsrfire.ath.cx","threadId":"16769","inReplyTo":"20081218121118.3635c53c@crow","subject":"[PATCH] diff.c: fix pointer type warning","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2008-12-19T22:10:48Z","receivedAt":"2008-12-19T22:10:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"As Mark Burton noted, the conversion to strbuf_readlink() caused a\ncompile warning on some architectures:\n\n> diff.c: In function ‘diff_populate_filespec’:\n> diff.c:1781: warning: passing argument 2 of ‘strbuf_detach’ from incompatible pointer type\n\nA pointer to an unsigned long is given while a pointer to a size_t is\nexpected; the two types are not considered to be equivalent everywhere.\n\nThe real fix would be to change the type of the size member of struct\ndiff_filespec to size_t, but that would cause other warnings in\nconnection with functions expecting unsigned long, and attempts to fix\nthem might loose an avalanche of changes.  Later.  This patch just\nsilences the warning by adding an (implicit) casting step.\n\nReported-by: Mark Burton <markb@ordern.com>\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n diff.c |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f160c1a..0484601 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1778,7 +1778,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \n \t\t\tif (strbuf_readlink(&sb, s->path, s->size))\n \t\t\t\tgoto err_empty;\n-\t\t\ts->data = strbuf_detach(&sb, &s->size);\n+\t\t\ts->size = sb.len;\n+\t\t\ts->data = strbuf_detach(&sb, NULL);\n \t\t\ts->should_free = 1;\n \t\t\treturn 0;\n \t\t}\n"},{"id":"98387","messageId":"7vd4fn7oe8.fsf@gitster.siamese.dyndns.org","threadId":"16769","inReplyTo":"494C1BE8.20607@lsrfire.ath.cx","subject":"Re: [PATCH] diff.c: fix pointer type warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-19T23:09:51Z","receivedAt":"2008-12-19T23:09:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; I think I already have it in my tree from your yesterday's\ne-mail.  I just have been too busy to whip the other branches into shape\nto push the results out.\n"},{"id":"299087","messageId":"1230026749-25360-1-git-send-email-madcoder@debian.org","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812171042120.14014@localhost.localdomain","subject":"[PATCH] strbuf_readlink semantics update.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-12-23T10:05:49Z","receivedAt":"2008-12-23T10:05:49Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"strbuf_* operations are meant to append their results to a current buffer\nrather than _replace_ its content. Modify strbuf_readlink accordingly.\n\nCurrent callers only operate on empty buffers at the moment and this\nsemantic change doesn't break any current code.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n strbuf.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex bdf4954..254a7ee 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -299,12 +299,12 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n \t\tint len;\n \n \t\tstrbuf_grow(sb, hint);\n-\t\tlen = readlink(path, sb->buf, hint);\n+\t\tlen = readlink(path, sb->buf + sb->len, hint);\n \t\tif (len < 0) {\n \t\t\tif (errno != ERANGE)\n \t\t\t\tbreak;\n \t\t} else if (len < hint) {\n-\t\t\tstrbuf_setlen(sb, len);\n+\t\t\tstrbuf_setlen(sb, sb->len + len);\n \t\t\treturn 0;\n \t\t}\n \n-- \n1.6.1.rc4.304.g3da087\n\n"},{"id":"98596","messageId":"20081223102127.GA21485@artemis.corp","threadId":"16769","inReplyTo":"1230026749-25360-1-git-send-email-madcoder@debian.org","subject":"Re: [PATCH] strbuf_readlink semantics update.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-12-23T10:21:27Z","receivedAt":"2008-12-23T10:21:27Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"when readlink fails, the strbuf shall not be destroyed. It's not how\nread_file_or_gitlink works for example.\n\nFix strbuf_readlink callers to destroy the buffer when appropriate.\n\nFix read_old_data possible leaks in case of errors, since even when no\ndata has been read, the strbufs may have grown to prepare the reads.\nstrbuf_release must be called on them.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n\n  I know it somehow add lines to the callers, but it's actually more\n  important to keep consistency among the strbuf APIs than to save 10\n  SLOCs.\n\n\n builtin-apply.c |    8 ++++++--\n combine-diff.c  |    1 +\n diff.c          |    4 +++-\n read-cache.c    |    4 +++-\n sha1_file.c     |    1 +\n strbuf.c        |    2 +-\n 6 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 07244b0..c1fe9ca 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2306,8 +2306,10 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *\n \t\t/* We have a patched copy in memory use that */\n \t\tstrbuf_add(&buf, tpatch->result, tpatch->resultsize);\n \t} else if (cached) {\n-\t\tif (read_file_or_gitlink(ce, &buf))\n+\t\tif (read_file_or_gitlink(ce, &buf)) {\n+\t\t\tstrbuf_release(&buf);\n \t\t\treturn error(\"read of %s failed\", patch->old_name);\n+\t\t}\n \t} else if (patch->old_name) {\n \t\tif (S_ISGITLINK(patch->old_mode)) {\n \t\t\tif (ce) {\n@@ -2320,8 +2322,10 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *\n \t\t\t\tpatch->fragments = NULL;\n \t\t\t}\n \t\t} else {\n-\t\t\tif (read_old_data(st, patch->old_name, &buf))\n+\t\t\tif (read_old_data(st, patch->old_name, &buf)) {\n+\t\t\t\tstrbuf_release(&buf);\n \t\t\t\treturn error(\"read of %s failed\", patch->old_name);\n+\t\t\t}\n \t\t}\n \t}\n \ndiff --git a/combine-diff.c b/combine-diff.c\nindex bccc018..674745d 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -706,6 +706,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n \n \t\t\tif (strbuf_readlink(&buf, elem->path, st.st_size) < 0) {\n+\t\t\t\tstrbuf_release(&buf);\n \t\t\t\terror(\"readlink(%s): %s\", elem->path,\n \t\t\t\t      strerror(errno));\n \t\t\t\treturn;\ndiff --git a/diff.c b/diff.c\nindex b57d9ac..41f7e1c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1778,8 +1778,10 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\tif (S_ISLNK(st.st_mode)) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n \n-\t\t\tif (strbuf_readlink(&sb, s->path, s->size))\n+\t\t\tif (strbuf_readlink(&sb, s->path, s->size)) {\n+\t\t\t\tstrbuf_release(&sb);\n \t\t\t\tgoto err_empty;\n+\t\t\t}\n \t\t\ts->size = sb.len;\n \t\t\ts->data = strbuf_detach(&sb, NULL);\n \t\t\ts->should_free = 1;\ndiff --git a/read-cache.c b/read-cache.c\nindex db166da..9673d91 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -104,8 +104,10 @@ static int ce_compare_link(struct cache_entry *ce, size_t expected_size)\n \tenum object_type type;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tif (strbuf_readlink(&sb, ce->name, expected_size))\n+\tif (strbuf_readlink(&sb, ce->name, expected_size)) {\n+\t\tstrbuf_release(&sb);\n \t\treturn -1;\n+\t}\n \n \tbuffer = read_sha1_file(ce->sha1, &type, &size);\n \tif (buffer) {\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 52d1ead..a62b53d 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2538,6 +2538,7 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, int write\n \tcase S_IFLNK:\n \t\tif (strbuf_readlink(&sb, path, st->st_size)) {\n \t\t\tchar *errstr = strerror(errno);\n+\t\t\tstrbuf_release(&sb);\n \t\t\treturn error(\"readlink(\\\"%s\\\"): %s\", path,\n \t\t\t             errstr);\n \t\t}\ndiff --git a/strbuf.c b/strbuf.c\nindex 254a7ee..b1f2a97 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -311,7 +311,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n \t\t/* .. the buffer was too small - try again */\n \t\thint *= 2;\n \t}\n-\tstrbuf_release(sb);\n+\tsb->buf[sb->len] = '\\0';\n \treturn -1;\n }\n \n-- \n1.6.1.rc4.306.g849c2\n\n"},{"id":"98635","messageId":"alpine.LFD.2.00.0812231009220.3535@localhost.localdomain","threadId":"16769","inReplyTo":"20081223102127.GA21485@artemis.corp","subject":"Re: [PATCH] strbuf_readlink semantics update.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-12-23T18:16:01Z","receivedAt":"2008-12-23T18:16:01Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 23 Dec 2008, Pierre Habouzit wrote:\n>\n> when readlink fails, the strbuf shall not be destroyed. It's not how\n> read_file_or_gitlink works for example.\n\nI disagree.\n\nThis patch just makes things worse. Just leave the \"strbuf_release()\" in \n_one_ place.\n\nLook:\n\n   6 files changed, 15 insertions(+), 5 deletions(-)\n\nyou added ten unnecessary lines, and you made the interface harder to use. \nWhat was the gain here?\n\n> Fix read_old_data possible leaks in case of errors, since even when no\n> data has been read, the strbufs may have grown to prepare the reads.\n> strbuf_release must be called on them.\n\nThat's a separate error, and quite frankly, the best approach to that is \nlikely to instead of breaking strbuf_readlink(), just make the S_IFREG() \ncase release it.\n\nI'd suggest that strbuf_read_file() should probably also do a \nstrbuf_release() if it returns a negative error value, but that's a \nseparate issue (and still leaves \"read_old_data()\" having to release \nthings, since read_old_data() wants to see exactly st_size bytes. Although \nI suspect we might want to change that, and just make it test for \nnegative too).\n\n\t\tLinus\n"},{"id":"98647","messageId":"20081224101146.GA10008@artemis.corp","threadId":"16769","inReplyTo":"alpine.LFD.2.00.0812231009220.3535@localhost.localdomain","subject":"Re: [PATCH] strbuf_readlink semantics update.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-12-24T10:11:46Z","receivedAt":"2008-12-24T10:11:46Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:\n> \n> \n> On Tue, 23 Dec 2008, Pierre Habouzit wrote:\n> >\n> > when readlink fails, the strbuf shall not be destroyed. It's not how\n> > read_file_or_gitlink works for example.\n> \n> I disagree.\n> \n> This patch just makes things worse. Just leave the \"strbuf_release()\" in \n> _one_ place.\n\nThe \"problem\" is that the strbuf API usually works that way: functions\nappend things to a buffer, or do nothing, but always keep the buffer in\na state where you can append more stuff to it.\n\nIf read_file_or_gitlink or strbuf_readlink destroy the buffer, then you\nbreak the second expectation people (should) have about the strbuf API.\n\nThe reason is that if you built things in the buffer, you really don't\nwant it to be undone just because the last bit you add went wrong for\nsome reason. Or if you have a buffer that is reused in a loop, you don't\nwant the buffer you allocated to be dropped just because one error\noccurred.\n\n\nAlternatively, we could pass a flag to tell function performing reads\n(fread, read, readlink, whatever) that those should destroy the buffer\non error or just report it. I don't really know. It sounds like\nover-engineering though.\n\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"98649","messageId":"4952532F.5050704@lsrfire.ath.cx","threadId":"16769","inReplyTo":"20081224101146.GA10008@artemis.corp","subject":"Re: [PATCH] strbuf_readlink semantics update.","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2008-12-24T15:20:15Z","receivedAt":"2008-12-24T15:20:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Pierre Habouzit schrieb:\n> On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:\n>>\n>> On Tue, 23 Dec 2008, Pierre Habouzit wrote:\n>>> when readlink fails, the strbuf shall not be destroyed. It's not how\n>>> read_file_or_gitlink works for example.\n>> I disagree.\n>>\n>> This patch just makes things worse. Just leave the \"strbuf_release()\" in \n>> _one_ place.\n> \n> The \"problem\" is that the strbuf API usually works that way: functions\n> append things to a buffer, or do nothing, but always keep the buffer in\n> a state where you can append more stuff to it.\n> \n> If read_file_or_gitlink or strbuf_readlink destroy the buffer, then you\n> break the second expectation people (should) have about the strbuf API.\n\nThe \"append or do nothing\" rule is broken by strbuf_getline(), but I agree\nto your reasoning.  How about refining this rule a bit to \"do your thing\nand roll back changes if an error occurs\"?  I think it's not worth to undo\nallocation extensions, but making reverting first time allocations seems\nlike a good idea.  Something like this?\n\nRené\n\n\nPS: only nine lines! ;-)\n\n\n strbuf.c |   17 +++++++++++++----\n 1 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex bdf4954..6ed0684 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -256,18 +256,21 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,\n size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)\n {\n \tsize_t res;\n+\tsize_t oldalloc = sb->alloc;\n \n \tstrbuf_grow(sb, size);\n \tres = fread(sb->buf + sb->len, 1, size, f);\n-\tif (res > 0) {\n+\tif (res > 0)\n \t\tstrbuf_setlen(sb, sb->len + res);\n-\t}\n+\telse if (res < 0 && oldalloc == 0)\n+\t\tstrbuf_release(sb);\n \treturn res;\n }\n \n ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n {\n \tsize_t oldlen = sb->len;\n+\tsize_t oldalloc = sb->alloc;\n \n \tstrbuf_grow(sb, hint ? hint : 8192);\n \tfor (;;) {\n@@ -275,7 +278,10 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \n \t\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n \t\tif (cnt < 0) {\n-\t\t\tstrbuf_setlen(sb, oldlen);\n+\t\t\tif (oldalloc == 0)\n+\t\t\t\tstrbuf_release(sb);\n+\t\t\telse\n+\t\t\t\tstrbuf_setlen(sb, oldlen);\n \t\t\treturn -1;\n \t\t}\n \t\tif (!cnt)\n@@ -292,6 +298,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \n int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n {\n+\tsize_t oldalloc = sb->alloc;\n+\n \tif (hint < 32)\n \t\thint = 32;\n \n@@ -311,7 +319,8 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n \t\t/* .. the buffer was too small - try again */\n \t\thint *= 2;\n \t}\n-\tstrbuf_release(sb);\n+\tif (oldalloc == 0)\n+\t\tstrbuf_release(sb);\n \treturn -1;\n }\n \n"},{"id":"98684","messageId":"7viqp8afap.fsf@gitster.siamese.dyndns.org","threadId":"16769","inReplyTo":"4952532F.5050704@lsrfire.ath.cx","subject":"Re: [PATCH] strbuf_readlink semantics update.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-25T07:23:58Z","receivedAt":"2008-12-25T07:23:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Pierre Habouzit schrieb:\n>> On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:\n>>>\n>>> On Tue, 23 Dec 2008, Pierre Habouzit wrote:\n>>>> when readlink fails, the strbuf shall not be destroyed. It's not how\n>>>> read_file_or_gitlink works for example.\n>>> I disagree.\n>>>\n>>> This patch just makes things worse. Just leave the \"strbuf_release()\" in \n>>> _one_ place.\n> ...\n> The \"append or do nothing\" rule is broken by strbuf_getline(), but I agree\n> to your reasoning.  How about refining this rule a bit to \"do your thing\n> and roll back changes if an error occurs\"?  I think it's not worth to undo\n> allocation extensions, but making reverting first time allocations seems\n> like a good idea.  Something like this?\n\nI think this is much better than Pierre's.  Pierre's \"if it is called\nstrbuf_*, it should always append\" is a good uniformity to have in an API,\nbut making the caller suffer for clean-up is going backwards.  The reason\nwe use strbuf when we can is so that the callers do not have to worry\nabout memory allocation issues too much.\n"},{"id":"99295","messageId":"20090104122108.GC29325@artemis.corp","threadId":"16769","inReplyTo":"7viqp8afap.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] strbuf_readlink semantics update.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2009-01-04T12:21:08Z","receivedAt":"2009-01-04T12:21:08Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Dec 25, 2008 at 07:23:58AM +0000, Junio C Hamano wrote:\n> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n> \n> > Pierre Habouzit schrieb:\n> >> On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:\n> >>>\n> >>> On Tue, 23 Dec 2008, Pierre Habouzit wrote:\n> >>>> when readlink fails, the strbuf shall not be destroyed. It's not how\n> >>>> read_file_or_gitlink works for example.\n> >>> I disagree.\n> >>>\n> >>> This patch just makes things worse. Just leave the \"strbuf_release()\" in \n> >>> _one_ place.\n> > ...\n> > The \"append or do nothing\" rule is broken by strbuf_getline(), but I agree\n> > to your reasoning.  How about refining this rule a bit to \"do your thing\n> > and roll back changes if an error occurs\"?  I think it's not worth to undo\n> > allocation extensions, but making reverting first time allocations seems\n> > like a good idea.  Something like this?\n> \n> I think this is much better than Pierre's.\n\nI agree it's a fine semantics.\n\n> Pierre's \"if it is called strbuf_*, it should always append\" is a good\n> uniformity to have in an API, but making the caller suffer for\n> clean-up is going backwards.  The reason we use strbuf when we can is\n> so that the callers do not have to worry about memory allocation\n> issues too much.\n\nAck.\n\nSorry for the delay I was on vacation.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"99508","messageId":"4963C1EA.504@lsrfire.ath.cx","threadId":"16769","inReplyTo":"20090104122108.GC29325@artemis.corp","subject":"[PATCH] strbuf: instate cleanup rule in case of non-memory errors","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-01-06T20:41:14Z","receivedAt":"2009-01-06T20:41:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Make all strbuf functions that can fail free() their memory on error if\nthey have allocated it.  They don't shrink buffers that have been grown,\nthough.\n\nThis allows for easier error handling, as callers only need to call\nstrbuf_release() if A) the command succeeded or B) if they would have had\nto do so anyway because they added something to the strbuf themselves.\n\nBonus hunk: document strbuf_readlink.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n Documentation/technical/api-strbuf.txt |   11 +++++++++--\n strbuf.c                               |   17 +++++++++++++----\n 2 files changed, 22 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\nindex a8ee2fe..9a4e3ea 100644\n--- a/Documentation/technical/api-strbuf.txt\n+++ b/Documentation/technical/api-strbuf.txt\n@@ -133,8 +133,10 @@ Functions\n \n * Adding data to the buffer\n \n-NOTE: All of these functions in this section will grow the buffer as\n-      necessary.\n+NOTE: All of the functions in this section will grow the buffer as necessary.\n+If they fail for some reason other than memory shortage and the buffer hadn't\n+been allocated before (i.e. the `struct strbuf` was set to `STRBUF_INIT`),\n+then they will free() it.\n \n `strbuf_addch`::\n \n@@ -235,6 +237,11 @@ same behaviour as well.\n \tRead the contents of a file, specified by its path. The third argument\n \tcan be used to give a hint about the file size, to avoid reallocs.\n \n+`strbuf_readlink`::\n+\n+\tRead the target of a symbolic link, specified by its path.  The third\n+\targument can be used to give a hint about the size, to avoid reallocs.\n+\n `strbuf_getline`::\n \n \tRead a line from a FILE* pointer. The second argument specifies the line\ndiff --git a/strbuf.c b/strbuf.c\nindex bdf4954..6ed0684 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -256,18 +256,21 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,\n size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)\n {\n \tsize_t res;\n+\tsize_t oldalloc = sb->alloc;\n \n \tstrbuf_grow(sb, size);\n \tres = fread(sb->buf + sb->len, 1, size, f);\n-\tif (res > 0) {\n+\tif (res > 0)\n \t\tstrbuf_setlen(sb, sb->len + res);\n-\t}\n+\telse if (res < 0 && oldalloc == 0)\n+\t\tstrbuf_release(sb);\n \treturn res;\n }\n \n ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n {\n \tsize_t oldlen = sb->len;\n+\tsize_t oldalloc = sb->alloc;\n \n \tstrbuf_grow(sb, hint ? hint : 8192);\n \tfor (;;) {\n@@ -275,7 +278,10 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \n \t\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n \t\tif (cnt < 0) {\n-\t\t\tstrbuf_setlen(sb, oldlen);\n+\t\t\tif (oldalloc == 0)\n+\t\t\t\tstrbuf_release(sb);\n+\t\t\telse\n+\t\t\t\tstrbuf_setlen(sb, oldlen);\n \t\t\treturn -1;\n \t\t}\n \t\tif (!cnt)\n@@ -292,6 +298,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \n int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n {\n+\tsize_t oldalloc = sb->alloc;\n+\n \tif (hint < 32)\n \t\thint = 32;\n \n@@ -311,7 +319,8 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n \t\t/* .. the buffer was too small - try again */\n \t\thint *= 2;\n \t}\n-\tstrbuf_release(sb);\n+\tif (oldalloc == 0)\n+\t\tstrbuf_release(sb);\n \treturn -1;\n }\n \n-- \n1.6.1\n"},{"id":"99616","messageId":"7vtz8a95m0.fsf@gitster.siamese.dyndns.org","threadId":"16769","inReplyTo":"4963C1EA.504@lsrfire.ath.cx","subject":"Re: [PATCH] strbuf: instate cleanup rule in case of non-memory errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-07T21:19:19Z","receivedAt":"2009-01-07T21:19:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Make all strbuf functions that can fail free() their memory on error if\n> they have allocated it.  They don't shrink buffers that have been grown,\n> though.\n\nThanks; applied.\n"}]}