{"thread":{"id":"36100","subject":"git 1.9.0 segfault","startedAt":"2014-03-08T16:23:43Z","lastAt":"2014-03-18T22:31:26Z","messageCount":21,"participants":["Guillaume Gelin","brian m. carlson","John Keeping","Jeff King","Junio C Hamano","Thomas Rast","Michael Haggerty","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"236320","messageId":"CAPn4x+oTTzYMSFzqUmJ8tOO0DdqR+HJJdoeXFZxhABu6B=QmBQ@mail.gmail.com","threadId":"36100","inReplyTo":null,"subject":"git 1.9.0 segfault","fromName":"Guillaume Gelin","fromEmail":"contact@ramnes.eu","sentAt":"2014-03-08T16:23:43Z","receivedAt":"2014-03-08T16:23:43Z","isPatch":false,"sender":{"key":"contact@ramnes.eu","avatar":null},"body":"Hi,\n\nhttp://pastebin.com/Np7L54ar\n\nCheers,\n\n-- \nGuillaume Gelin\n"},{"id":"236321","messageId":"20140308164651.GA32213@vauxhall.crustytoothpaste.net","threadId":"36100","inReplyTo":"CAPn4x+oTTzYMSFzqUmJ8tOO0DdqR+HJJdoeXFZxhABu6B=QmBQ@mail.gmail.com","subject":"Re: git 1.9.0 segfault","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-03-08T16:46:51Z","receivedAt":"2014-03-08T16:46:51Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Mar 08, 2014 at 04:23:43PM +0000, Guillaume Gelin wrote:\n> Hi,\n>\n> http://pastebin.com/Np7L54ar\n\nI can confirm this. I get the following backtrace:\n\n Core was generated by `/home/bmc/checkouts/git/git mv packages/ lisp'.\n Program terminated with signal 11, Segmentation fault.\n #0 0x00007fe31a4371b2 in _IO_vfprintf_internal (s=s@entry=0x7fffa330d2e0, format=, format@entry=0x7fffa330e5b0 \"renaming '%s' failed: Bad address\", ap=ap@entry=0x7fffa330e498)\n at vfprintf.c:1649\n 1649\tvfprintf.c: No such file or directory.\n (gdb) bt\n #0 0x00007fe31a4371b2 in _IO_vfprintf_internal (s=s@entry=0x7fffa330d2e0, format=, format@entry=0x7fffa330e5b0 \"renaming '%s' failed: Bad address\", ap=ap@entry=0x7fffa330e498)\n at vfprintf.c:1649\n #1 0x00007fe31a4e2315 in ___vsnprintf_chk (s=s@entry=0x7fffa330d450 \"renaming '0\\243\\377\\177\", maxlen=, maxlen@entry=4096, flags=flags@entry=1, slen=slen@entry=4096,\n format=0x7fffa330e5b0 \"renaming '%s' failed: Bad address\", format@entry=0x544fe5 \"fatal: \", args=0x7fffa330e498) at vsnprintf_chk.c:63\n #2 0x00000000005041cb in vsnprintf (__ap=, __fmt=0x544fe5 \"fatal: \", __n=4096, __s=0x7fffa330d450 \"renaming '0\\243\\377\\177\") at /usr/include/x86_64-linux-gnu/bits/stdio2.h:77\n #3 vreportf (prefix=prefix@entry=0x544fe5 \"fatal: \", err=, params=) at usage.c:12\n #4 0x0000000000504224 in die_builtin (err=, params=) at usage.c:36\n #5 0x0000000000504650 in die_errno (fmt=0x52be9a \"renaming '%s' failed\") at usage.c:137\n #6 0x000000000044cb4d in cmd_mv (argc=, argv=, prefix=) at builtin/mv.c:246\n #7 0x000000000040602d in run_builtin (argv=0x7fffa330ef90, argc=3, p=0x779d40 ) at git.c:314\n #8 handle_builtin (argc=3, argv=0x7fffa330ef90) at git.c:487\n #9 0x00000000004052e1 in run_argv (argv=0x7fffa330ee48, argcp=0x7fffa330ee2c) at git.c:533\n #10 main (argc=3, av=) at git.c:616\n\nWe're failing to rename because we got an EFAULT, and then we try to\nprint the failing filename, and we get a segfault right here:\n\n\t\t\tif (rename(src, dst) < 0 && !ignore_errors)\n\t\t\t\tdie_errno (_(\"renaming '%s' failed\"), src);\n\nI don't know yet if dst is also bad, but clearly src is. I'm looking\ninto it.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"236323","messageId":"20140308181218.GG18371@serenity.lan","threadId":"36100","inReplyTo":"20140308164651.GA32213@vauxhall.crustytoothpaste.net","subject":"Re: git 1.9.0 segfault","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-03-08T18:12:18Z","receivedAt":"2014-03-08T18:12:18Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sat, Mar 08, 2014 at 04:46:51PM +0000, brian m. carlson wrote:\n> On Sat, Mar 08, 2014 at 04:23:43PM +0000, Guillaume Gelin wrote:\n> > Hi,\n> >\n> > http://pastebin.com/Np7L54ar\n> We're failing to rename because we got an EFAULT, and then we try to\n> print the failing filename, and we get a segfault right here:\n> \n> \t\t\tif (rename(src, dst) < 0 && !ignore_errors)\n> \t\t\t\tdie_errno (_(\"renaming '%s' failed\"), src);\n> \n> I don't know yet if dst is also bad, but clearly src is. I'm looking\n> into it.\n\nThe problem seems to be that we change argc when we append nested\ndirectories to the list and then continue looping over 'source' which\nhas been realloc'd to be larger. But we do not realloc\nsubmodule_gitfile at the same time so we start writing beyond the end of\nthe submodule_gitfile array.\n\nThe particular behaviour of glibc's malloc happens to mean (at least on\nmy system) that this starts overwriting 'src'.\n\nThis fixes it for me:\n\n-- >8 --\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 7e26eb5..23f119a 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -180,6 +180,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\tmodes = xrealloc(modes,\n \t\t\t\t\t\t\t\t(argc + last - first)\n \t\t\t\t\t\t\t\t* sizeof(enum update_mode));\n+\t\t\t\t\t\tsubmodule_gitfile = xrealloc(submodule_gitfile,\n+\t\t\t\t\t\t\t\t(argc + last - first)\n+\t\t\t\t\t\t\t\t* sizeof(char *));\n \t\t\t\t\t}\n \n \t\t\t\t\tdst = add_slash(dst);\n"},{"id":"236324","messageId":"20140308183501.GH18371@serenity.lan","threadId":"36100","inReplyTo":"20140308181218.GG18371@serenity.lan","subject":"[PATCH] builtin/mv: fix out of bounds write","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-03-08T18:35:01Z","receivedAt":"2014-03-08T18:35:01Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When commit a88c915 (mv: move submodules using a gitfile, 2013-07-30)\nadded the submodule_gitfile array, it was not added to the block that\nenlarges the arrays when we are moving a directory so that we do not\nhave to worry about it being a directory when we perform the actual\nmove. After this, the loop continues over the enlarged set of sources.\n\nSince we assume that submodule_gitfile has size argc, if any of the\nitems in the source directory are submodules we are guaranteed to write\nbeyond the end of submodule_gitfile.\n\nFix this by realloc'ing submodule_gitfile at the same time as the other\narrays.\n\nReported-by: Guillaume Gelin \nSigned-off-by: John Keeping \n---\nOn Sat, Mar 08, 2014 at 06:12:18PM +0000, John Keeping wrote:\n> This fixes it for me:\n\nHere it is as a proper patch.\n\n builtin/mv.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 21c46d1..f99c91e 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -179,6 +179,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\tmodes = xrealloc(modes,\n \t\t\t\t\t\t\t\t(argc + last - first)\n \t\t\t\t\t\t\t\t* sizeof(enum update_mode));\n+\t\t\t\t\t\tsubmodule_gitfile = xrealloc(submodule_gitfile,\n+\t\t\t\t\t\t\t\t(argc + last - first)\n+\t\t\t\t\t\t\t\t* sizeof(char *));\n \t\t\t\t\t}\n \n \t\t\t\t\tdst = add_slash(dst);\n-- \n1.9.0.6.g037df60.dirty\n"},{"id":"236325","messageId":"20140308191542.GB32213@vauxhall.crustytoothpaste.net","threadId":"36100","inReplyTo":"20140308183501.GH18371@serenity.lan","subject":"Re: [PATCH] builtin/mv: fix out of bounds write","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-03-08T19:15:42Z","receivedAt":"2014-03-08T19:15:42Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Mar 08, 2014 at 06:35:01PM +0000, John Keeping wrote:\n> When commit a88c915 (mv: move submodules using a gitfile, 2013-07-30)\n> added the submodule_gitfile array, it was not added to the block that\n> enlarges the arrays when we are moving a directory so that we do not\n> have to worry about it being a directory when we perform the actual\n> move. After this, the loop continues over the enlarged set of sources.\n> \n> Since we assume that submodule_gitfile has size argc, if any of the\n> items in the source directory are submodules we are guaranteed to write\n> beyond the end of submodule_gitfile.\n> \n> Fix this by realloc'ing submodule_gitfile at the same time as the other\n> arrays.\n> \n> Reported-by: Guillaume Gelin \n> Signed-off-by: John Keeping \n> ---\n> On Sat, Mar 08, 2014 at 06:12:18PM +0000, John Keeping wrote:\n> > This fixes it for me:\n> \n> Here it is as a proper patch.\n> \n> builtin/mv.c | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index 21c46d1..f99c91e 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -179,6 +179,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\t\tmodes = xrealloc(modes,\n> \t\t\t\t\t\t\t\t(argc + last - first)\n> \t\t\t\t\t\t\t\t* sizeof(enum update_mode));\n> +\t\t\t\t\t\tsubmodule_gitfile = xrealloc(submodule_gitfile,\n> +\t\t\t\t\t\t\t\t(argc + last - first)\n> +\t\t\t\t\t\t\t\t* sizeof(char *));\n> \t\t\t\t\t}\n> \n> \t\t\t\t\tdst = add_slash(dst);\n\nYup, that's the same conclusion I came to. There are also two cases\nwhere we don't shrink the array properly. I'll rebase my patch on top\nof this one and send it.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"236326","messageId":"1394306499-50871-1-git-send-email-sandals@crustytoothpaste.net","threadId":"36100","inReplyTo":"20140308183501.GH18371@serenity.lan","subject":"[PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-03-08T19:21:39Z","receivedAt":"2014-03-08T19:21:39Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"We shrink the source and destination arrays, but not the modes or\nsubmodule_gitfile arrays, resulting in potentially mismatched data. Shrink\nall the arrays at the same time to prevent this.\n\nSigned-off-by: brian m. carlson \n---\n builtin/mv.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex f99c91e..b20cd95 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tmemmove(destination + i,\n \t\t\t\t\t\tdestination + i + 1,\n \t\t\t\t\t\t(argc - i) * sizeof(char *));\n+\t\t\t\t\tmemmove(modes + i, modes + i + 1,\n+\t\t\t\t\t\t(argc - i) * sizeof(char *));\n+\t\t\t\t\tmemmove(submodule_gitfile + i,\n+\t\t\t\t\t\tsubmodule_gitfile + i + 1,\n+\t\t\t\t\t\t(argc - i) * sizeof(char *));\n \t\t\t\t\ti--;\n \t\t\t\t}\n \t\t\t} else\n-- \n1.9.0.1010.g6633b85.dirty\n"},{"id":"236327","messageId":"20140308192916.GI18371@serenity.lan","threadId":"36100","inReplyTo":"20140308191542.GB32213@vauxhall.crustytoothpaste.net","subject":"[PATCH v2] builtin/mv: fix out of bounds write","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-03-08T19:29:17Z","receivedAt":"2014-03-08T19:29:17Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When commit a88c915 (mv: move submodules using a gitfile, 2013-07-30)\nadded the submodule_gitfile array, it was not added to the block that\nenlarges the arrays when we are moving a directory so that we do not\nhave to worry about it being a directory when we perform the actual\nmove. After this, the loop continues over the enlarged set of sources.\n\nSince we assume that submodule_gitfile has size argc, if any of the\nitems in the source directory are submodules we are guaranteed to write\nbeyond the end of submodule_gitfile.\n\nFix this by realloc'ing submodule_gitfile at the same time as the other\narrays.\n\nReported-by: Guillaume Gelin \nSigned-off-by: John Keeping \n---\nOn Sat, Mar 08, 2014 at 07:15:42PM +0000, brian m. carlson wrote:\n> Yup, that's the same conclusion I came to. There are also two cases\n> where we don't shrink the array properly. I'll rebase my patch on top\n> of this one and send it.\n\nNice catch. While looking at that, I spotted that I forgot to\ninitialize the new values in submodule_gitfile when it grows.\nGuillaume's test case doesn't catch that because all the subdirectories\nare submodules.\n\n builtin/mv.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 21c46d1..5258077 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -179,6 +179,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\tmodes = xrealloc(modes,\n \t\t\t\t\t\t\t\t(argc + last - first)\n \t\t\t\t\t\t\t\t* sizeof(enum update_mode));\n+\t\t\t\t\t\tsubmodule_gitfile = xrealloc(submodule_gitfile,\n+\t\t\t\t\t\t\t\t(argc + last - first)\n+\t\t\t\t\t\t\t\t* sizeof(char *));\n \t\t\t\t\t}\n \n \t\t\t\t\tdst = add_slash(dst);\n@@ -192,6 +195,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t\tprefix_path(dst, dst_len,\n \t\t\t\t\t\t\t\tpath + length + 1);\n \t\t\t\t\t\tmodes[argc + j] = INDEX;\n+\t\t\t\t\t\tsubmodule_gitfile[argc + j] = NULL;\n \t\t\t\t\t}\n \t\t\t\t\targc += last - first;\n \t\t\t\t}\n-- \n1.9.0.6.g037df60.dirty\n"},{"id":"236477","messageId":"20140311015603.GA12180@sigill.intra.peff.net","threadId":"36100","inReplyTo":"1394306499-50871-1-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-11T01:56:03Z","receivedAt":"2014-03-11T01:56:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 08, 2014 at 07:21:39PM +0000, brian m. carlson wrote:\n\n> We shrink the source and destination arrays, but not the modes or\n> submodule_gitfile arrays, resulting in potentially mismatched data. Shrink\n> all the arrays at the same time to prevent this.\n> \n> Signed-off-by: brian m. carlson \n> ---\n> builtin/mv.c | 5 +++++\n> 1 file changed, 5 insertions(+)\n> \n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index f99c91e..b20cd95 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\tmemmove(destination + i,\n> \t\t\t\t\t\tdestination + i + 1,\n> \t\t\t\t\t\t(argc - i) * sizeof(char *));\n> +\t\t\t\t\tmemmove(modes + i, modes + i + 1,\n> +\t\t\t\t\t\t(argc - i) * sizeof(char *));\n> +\t\t\t\t\tmemmove(submodule_gitfile + i,\n> +\t\t\t\t\t\tsubmodule_gitfile + i + 1,\n> +\t\t\t\t\t\t(argc - i) * sizeof(char *));\n\nI haven't looked that closely, but would it be crazy to suggest that\nthese arrays all be squashed into one array-of-struct? It would be less\nerror prone and perhaps more readable.\n\n-Peff\n"},{"id":"236478","messageId":"20140311020003.GE4271@vauxhall.crustytoothpaste.net","threadId":"36100","inReplyTo":"20140311015603.GA12180@sigill.intra.peff.net","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-03-11T02:00:04Z","receivedAt":"2014-03-11T02:00:04Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Mar 10, 2014 at 09:56:03PM -0400, Jeff King wrote:\n> On Sat, Mar 08, 2014 at 07:21:39PM +0000, brian m. carlson wrote:\n> \n> > We shrink the source and destination arrays, but not the modes or\n> > submodule_gitfile arrays, resulting in potentially mismatched data. Shrink\n> > all the arrays at the same time to prevent this.\n> > \n> > Signed-off-by: brian m. carlson \n> > ---\n> > builtin/mv.c | 5 +++++\n> > 1 file changed, 5 insertions(+)\n> > \n> > diff --git a/builtin/mv.c b/builtin/mv.c\n> > index f99c91e..b20cd95 100644\n> > --- a/builtin/mv.c\n> > +++ b/builtin/mv.c\n> > @@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> > \t\t\t\t\tmemmove(destination + i,\n> > \t\t\t\t\t\tdestination + i + 1,\n> > \t\t\t\t\t\t(argc - i) * sizeof(char *));\n> > +\t\t\t\t\tmemmove(modes + i, modes + i + 1,\n> > +\t\t\t\t\t\t(argc - i) * sizeof(char *));\n> > +\t\t\t\t\tmemmove(submodule_gitfile + i,\n> > +\t\t\t\t\t\tsubmodule_gitfile + i + 1,\n> > +\t\t\t\t\t\t(argc - i) * sizeof(char *));\n> \n> I haven't looked that closely, but would it be crazy to suggest that\n> these arrays all be squashed into one array-of-struct? It would be less\n> error prone and perhaps more readable.\n\nI was thinking of doing exactly that, but the way internal_copy_pathspec\nis written, I'd need to use offsetof to have it write to the right\nstructure members. That's a bit more gross than I wanted, but I'll\nprobably implement it at some point during the upcoming week.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"236542","messageId":"xmqqfvmoo1d4.fsf@gitster.dls.corp.google.com","threadId":"36100","inReplyTo":"1394306499-50871-1-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-11T21:45:59Z","receivedAt":"2014-03-11T21:45:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> We shrink the source and destination arrays, but not the modes or\n> submodule_gitfile arrays, resulting in potentially mismatched data. Shrink\n> all the arrays at the same time to prevent this.\n>\n> Signed-off-by: brian m. carlson \n> ---\n> builtin/mv.c | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index f99c91e..b20cd95 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\tmemmove(destination + i,\n> \t\t\t\t\t\tdestination + i + 1,\n> \t\t\t\t\t\t(argc - i) * sizeof(char *));\n> +\t\t\t\t\tmemmove(modes + i, modes + i + 1,\n> +\t\t\t\t\t\t(argc - i) * sizeof(char *));\n> +\t\t\t\t\tmemmove(submodule_gitfile + i,\n> +\t\t\t\t\t\tsubmodule_gitfile + i + 1,\n> +\t\t\t\t\t\t(argc - i) * sizeof(char *));\n> \t\t\t\t\ti--;\n> \t\t\t\t}\n> \t\t\t} else\n\nThanks. Neither this nor John's seems to describe the user-visible\nway to trigger the symptom. Can we have tests for them?\n"},{"id":"236649","messageId":"20140312232126.GG4271@vauxhall.crustytoothpaste.net","threadId":"36100","inReplyTo":"xmqqfvmoo1d4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-03-12T23:21:26Z","receivedAt":"2014-03-12T23:21:26Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Mar 11, 2014 at 02:45:59PM -0700, Junio C Hamano wrote:\n> Thanks. Neither this nor John's seems to describe the user-visible\n> way to trigger the symptom. Can we have tests for them?\n\nI'll try to get to writing some test today or tomorrow. I just noticed\nthe bugginess by looking at the code, so I'll need to actually spend\ntime reproducing the problem.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"236792","messageId":"8738ijzbue.fsf@thomasrast.ch","threadId":"36100","inReplyTo":"1394306499-50871-1-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2014-03-15T16:05:29Z","receivedAt":"2014-03-15T16:05:29Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"\"brian m. carlson\" writes:\n\n> We shrink the source and destination arrays, but not the modes or\n> submodule_gitfile arrays, resulting in potentially mismatched data. Shrink\n> all the arrays at the same time to prevent this.\n>\n> Signed-off-by: brian m. carlson \n> ---\n> builtin/mv.c | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index f99c91e..b20cd95 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\tmemmove(destination + i,\n> \t\t\t\t\t\tdestination + i + 1,\n> \t\t\t\t\t\t(argc - i) * sizeof(char *));\n> +\t\t\t\t\tmemmove(modes + i, modes + i + 1,\n> +\t\t\t\t\t\t(argc - i) * sizeof(char *));\n\nThis isn't right -- you are computing the size of things to be moved\nbased on a type of char*, but 'modes' is an enum.\n\n(Valgrind spotted this.)\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"236799","messageId":"1394909812-92472-1-git-send-email-sandals@crustytoothpaste.net","threadId":"36100","inReplyTo":"1394306499-50871-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2] mv: prevent mismatched data when ignoring errors.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-03-15T18:56:52Z","receivedAt":"2014-03-15T18:56:52Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"We shrink the source and destination arrays, but not the modes or\nsubmodule_gitfile arrays, resulting in potentially mismatched data. Shrink\nall the arrays at the same time to prevent this. Add tests to ensure the\nproblem does not recur.\n\nSigned-off-by: brian m. carlson \n---\n\nI attempted to come up with a second patch that would refactor out the\nfour different arrays into one array of struct, as Jeff suggested, but\nit became very ugly very quickly. So this patch simply fixes the\nproblem and adds tests.\n\n builtin/mv.c | 5 +++++\n t/t7001-mv.sh | 13 ++++++++++++-\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex f99c91e..09bbc63 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tmemmove(destination + i,\n \t\t\t\t\t\tdestination + i + 1,\n \t\t\t\t\t\t(argc - i) * sizeof(char *));\n+\t\t\t\t\tmemmove(modes + i, modes + i + 1,\n+\t\t\t\t\t\t(argc - i) * sizeof(enum update_mode));\n+\t\t\t\t\tmemmove(submodule_gitfile + i,\n+\t\t\t\t\t\tsubmodule_gitfile + i + 1,\n+\t\t\t\t\t\t(argc - i) * sizeof(char *));\n \t\t\t\t\ti--;\n \t\t\t\t}\n \t\t\t} else\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex e3c8c2c..215d43d 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -294,7 +294,8 @@ test_expect_success 'setup submodule' '\n \tgit submodule add ./. sub &&\n \techo content >file &&\n \tgit add file &&\n-\tgit commit -m \"added sub and file\"\n+\tgit commit -m \"added sub and file\" &&\n+\tgit branch submodule\n '\n \n test_expect_success 'git mv cannot move a submodule in a file' '\n@@ -463,4 +464,14 @@ test_expect_success 'checking out a commit before submodule moved needs manual u\n \t! test -s actual\n '\n \n+test_expect_success 'mv -k does not accidentally destroy submodules' '\n+\tgit checkout submodule &&\n+\tmkdir dummy dest &&\n+\tgit mv -k dummy sub dest &&\n+\tgit status --porcelain >actual &&\n+\tgrep \"^R sub -> dest/sub\" actual &&\n+\tgit reset --hard &&\n+\tgit checkout .\n+'\n+\n test_done\n-- \n1.9.0.1010.g6633b85.dirty\n"},{"id":"236814","messageId":"20140316020018.GA20019@sigill.intra.peff.net","threadId":"36100","inReplyTo":"8738ijzbue.fsf@thomasrast.ch","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-16T02:00:19Z","receivedAt":"2014-03-16T02:00:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 15, 2014 at 05:05:29PM +0100, Thomas Rast wrote:\n\n> > diff --git a/builtin/mv.c b/builtin/mv.c\n> > index f99c91e..b20cd95 100644\n> > --- a/builtin/mv.c\n> > +++ b/builtin/mv.c\n> > @@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> > \t\t\t\t\tmemmove(destination + i,\n> > \t\t\t\t\t\tdestination + i + 1,\n> > \t\t\t\t\t\t(argc - i) * sizeof(char *));\n> > +\t\t\t\t\tmemmove(modes + i, modes + i + 1,\n> > +\t\t\t\t\t\t(argc - i) * sizeof(char *));\n> \n> This isn't right -- you are computing the size of things to be moved\n> based on a type of char*, but 'modes' is an enum.\n> \n> (Valgrind spotted this.)\n\nMaybe using sizeof(*destination) and sizeof(*modes) would make this less\nerror-prone?\n\n-Peff\n"},{"id":"236815","messageId":"20140316020051.GB20019@sigill.intra.peff.net","threadId":"36100","inReplyTo":"1394909812-92472-1-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2] mv: prevent mismatched data when ignoring errors.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-16T02:00:52Z","receivedAt":"2014-03-16T02:00:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 15, 2014 at 06:56:52PM +0000, brian m. carlson wrote:\n\n> We shrink the source and destination arrays, but not the modes or\n> submodule_gitfile arrays, resulting in potentially mismatched data. Shrink\n> all the arrays at the same time to prevent this. Add tests to ensure the\n> problem does not recur.\n> \n> Signed-off-by: brian m. carlson \n> ---\n> \n> I attempted to come up with a second patch that would refactor out the\n> four different arrays into one array of struct, as Jeff suggested, but\n> it became very ugly very quickly. So this patch simply fixes the\n> problem and adds tests.\n\n>From my brief look, I feared that might be the case. Oh well, thanks for\ntrying.\n\n-Peff\n"},{"id":"236841","messageId":"7v1ty14z8x.fsf@alter.siamese.dyndns.org","threadId":"36100","inReplyTo":"20140316020018.GA20019@sigill.intra.peff.net","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-16T21:20:14Z","receivedAt":"2014-03-16T21:20:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Mar 15, 2014 at 05:05:29PM +0100, Thomas Rast wrote:\n>\n>> > diff --git a/builtin/mv.c b/builtin/mv.c\n>> > index f99c91e..b20cd95 100644\n>> > --- a/builtin/mv.c\n>> > +++ b/builtin/mv.c\n>> > @@ -230,6 +230,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n>> > \t\t\t\t\tmemmove(destination + i,\n>> > \t\t\t\t\t\tdestination + i + 1,\n>> > \t\t\t\t\t\t(argc - i) * sizeof(char *));\n>> > +\t\t\t\t\tmemmove(modes + i, modes + i + 1,\n>> > +\t\t\t\t\t\t(argc - i) * sizeof(char *));\n>> \n>> This isn't right -- you are computing the size of things to be moved\n>> based on a type of char*, but 'modes' is an enum.\n>> \n>> (Valgrind spotted this.)\n>\n> Maybe using sizeof(*destination) and sizeof(*modes) would make this less\n> error-prone?\n>\n> -Peff\n\nWould it make sense to go one step further to introduce two macros\nto make this kind of screw-up less likely?\n\n 1. \"array\" is an array that holds \"nr\" elements. Move \"count\"\n elements starting at index \"at\" down to remove them.\n\n #define MOVE_DOWN(array, nr, at, count)\n\n The implementation should take advantage of sizeof(*array) to\n come up with the number of bytes to move.\n\n\n 2. \"array\" is an array that holds \"nr\" elements. Move \"count\"\n elements starting at index \"at\" up to make room to copy new\n elements in.\n\n #define MOVE_UP(array, nr, at, count)\n\n The implementation should take advantage of sizeof(*array) to\n come up with the number of bytes to move.\n\nOptionally, to make 2. even safer, these macros could take \"alloc\"\nto say that \"array\" has memory allocated to hold \"alloc\" elements,\nand the implementation may check \"nr + count\" does not overflow\n\"alloc\". This would make 1. and 2. asymmetric (move-down can do no\nvalidation using \"alloc\", but move-up would be helped), so I am not\nsure it is a good idea.\n\nAfter letting my eyes coast over hits from \"git grep memmove\", there\ndo seem to be some places that these would help readability, but not\nvery many.\n"},{"id":"236853","messageId":"7vtxax2v1q.fsf@alter.siamese.dyndns.org","threadId":"36100","inReplyTo":"7v1ty14z8x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-17T06:33:53Z","receivedAt":"2014-03-17T06:33:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Would it make sense to go one step further to introduce two macros\n> to make this kind of screw-up less likely?\n> ...\n> After letting my eyes coast over hits from \"git grep memmove\", there\n> do seem to be some places that these would help readability, but not\n> very many.\n\nI see quite a many hits that follow this pattern\n\n\tmemmove(array + pos, array + pos + 1, sizeof(*array) * (nr - pos))\n\nto make a single slot in a middle of array available, which would be\ngood candidates to use MOVE_DOWN(). Just to show a few:\n\nbuiltin/mv.c:226:\tmemmove(source + i, source + i + 1,\nbuiltin/mv.c-227-\t\t(argc - i) * sizeof(char *));\nbuiltin/mv.c:228:\tmemmove(destination + i,\nbuiltin/mv.c-229-\t\tdestination + i + 1,\nbuiltin/mv.c-230-\t\t(argc - i) * sizeof(char *));\ncache-tree.c:92:\tmemmove(it->down + pos + 1,\ncache-tree.c-93-\t\tit->down + pos,\ncache-tree.c-94-\t\tsizeof(down) * (it->subtree_nr - pos - 1));\n\n\nPerhaps something like this patch to start off; I am not sure\nMOVE_DOWN_BOUNDED is needed, though.\n\n cache.h | 33 +++++++++++++++++++++++++++++++++\n 1 file changed, 33 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex b66cb49..b2615ab 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -455,6 +455,39 @@ extern int daemonize(void);\n \t\t} \\\n \t} while (0)\n \n+/*\n+ * With an array \"array\" that currently holds \"nr\" elements, move\n+ * elements at \"at\" and later down by \"count\" elements to make room to\n+ * add in new elements. The caller is responsible for making sure\n+ * that the array has enough room to hold \"nr\" + \"count\" slots.\n+ */\n+#define MOVE_DOWN(array, nr, at, count)\t\t\t\\\n+\tmemmove((array) + (at) + (count),\t\t\\\n+\t\t(array) + (at),\t\t\t\t\\\n+\t\tsizeof((array)[0]) * ((nr) - (at)))\n+\n+/*\n+ * With an array \"array\" that has enough memory to hold \"alloc\"\n+ * elements allocated and currently holds \"nr\" elements, move elements\n+ * at \"at\" and later down by \"count\" elements to make room to add in\n+ * new elements.\n+ */\n+#define MOVE_DOWN_BOUNDED(array, nr, at, count, alloc)\t\t \\\n+\tdo {\t\t\t\t\t\t\t \\\n+\t\tif ((alloc) <= (nr) + (count))\t\t\t \\\n+\t\t\tBUG(\"MOVE_DOWN beyond the end of an array\"); \\\n+\t\tMOVE_DOWN((array), (nr), (at), (count));\t \\\n+\t} while (0)\n+\n+/*\n+ * With an array \"array\" that curently holds \"nr\" elements, move elements\n+ * at \"at\" + \"count\" and later down by \"count\" elements, removing the\n+ * elements between \"at\" and \"at\" + \"count\".\n+ */\n+#define MOVE_UP(array, nr, at, count)\t\t\t\t\\\n+\tmemmove((array) + (at), (array) + (at) + (count),\t\\\n+\t\tsizeof((array)[0]) * ((nr) - ((at) + (count))))\n+\n /* Initialize and use the cache information */\n extern int read_index(struct index_state *);\n extern int read_index_preload(struct index_state *, const struct pathspec *pathspec);\n"},{"id":"236884","messageId":"53270FC2.2030701@alum.mit.edu","threadId":"36100","inReplyTo":"7vtxax2v1q.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-17T15:07:46Z","receivedAt":"2014-03-17T15:07:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/17/2014 07:33 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Would it make sense to go one step further to introduce two macros\n>> to make this kind of screw-up less likely?\n>> ...\n>> After letting my eyes coast over hits from \"git grep memmove\", there\n>> do seem to be some places that these would help readability, but not\n>> very many.\n> \n> I see quite a many hits that follow this pattern\n> \n> \tmemmove(array + pos, array + pos + 1, sizeof(*array) * (nr - pos))\n> \n> to make a single slot in a middle of array available, which would be\n> good candidates to use MOVE_DOWN(). Just to show a few:\n> \n> builtin/mv.c:226:\tmemmove(source + i, source + i + 1,\n> builtin/mv.c-227-\t\t(argc - i) * sizeof(char *));\n> builtin/mv.c:228:\tmemmove(destination + i,\n> builtin/mv.c-229-\t\tdestination + i + 1,\n> builtin/mv.c-230-\t\t(argc - i) * sizeof(char *));\n> cache-tree.c:92:\tmemmove(it->down + pos + 1,\n> cache-tree.c-93-\t\tit->down + pos,\n> cache-tree.c-94-\t\tsizeof(down) * (it->subtree_nr - pos - 1));\n> \n> \n> Perhaps something like this patch to start off; I am not sure\n> MOVE_DOWN_BOUNDED is needed, though.\n> \n> cache.h | 33 +++++++++++++++++++++++++++++++++\n> 1 file changed, 33 insertions(+)\n> \n> diff --git a/cache.h b/cache.h\n> index b66cb49..b2615ab 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -455,6 +455,39 @@ extern int daemonize(void);\n> \t\t} \\\n> \t} while (0)\n> \n> +/*\n> + * With an array \"array\" that currently holds \"nr\" elements, move\n> + * elements at \"at\" and later down by \"count\" elements to make room to\n> + * add in new elements. The caller is responsible for making sure\n> + * that the array has enough room to hold \"nr\" + \"count\" slots.\n> + */\n> +#define MOVE_DOWN(array, nr, at, count)\t\t\t\\\n> +\tmemmove((array) + (at) + (count),\t\t\\\n> +\t\t(array) + (at),\t\t\t\t\\\n> +\t\tsizeof((array)[0]) * ((nr) - (at)))\n> +\n> +/*\n> + * With an array \"array\" that has enough memory to hold \"alloc\"\n> + * elements allocated and currently holds \"nr\" elements, move elements\n> + * at \"at\" and later down by \"count\" elements to make room to add in\n> + * new elements.\n> + */\n> +#define MOVE_DOWN_BOUNDED(array, nr, at, count, alloc)\t\t \\\n> +\tdo {\t\t\t\t\t\t\t \\\n> +\t\tif ((alloc) <= (nr) + (count))\t\t\t \\\n> +\t\t\tBUG(\"MOVE_DOWN beyond the end of an array\"); \\\n> +\t\tMOVE_DOWN((array), (nr), (at), (count));\t \\\n> +\t} while (0)\n> +\n> +/*\n> + * With an array \"array\" that curently holds \"nr\" elements, move elements\n> + * at \"at\" + \"count\" and later down by \"count\" elements, removing the\n> + * elements between \"at\" and \"at\" + \"count\".\n> + */\n> +#define MOVE_UP(array, nr, at, count)\t\t\t\t\\\n> +\tmemmove((array) + (at), (array) + (at) + (count),\t\\\n> +\t\tsizeof((array)[0]) * ((nr) - ((at) + (count))))\n> +\n> /* Initialize and use the cache information */\n> extern int read_index(struct index_state *);\n> extern int read_index_preload(struct index_state *, const struct pathspec *pathspec);\n\nI had recently been thinking along the same lines. In many of the\npotential callers that I noticed, ALLOC_GROW() was used immediately\nbefore making space in the array for a new element. So I suggest\nsomething more like\n\n+#define MOVE_DOWN(array, nr, at, count)\t\\\n+\tmemmove((array) + (at) + (count),\t\t\\\n+\t\t(array) + (at),\t\t\t\t\\\n+\t\tsizeof((array)[0]) * ((nr) - (at)))\n+#define ALLOC_INSERT_GAP(array, nr, at, count, alloc)\t\t \\\n+\tdo {\t\t\t\t\t\t\t \\\n+\t\tALLOC_GROW((array), (nr) + (count), (alloc)); \\\n+\t\tMOVE_DOWN((array), (nr), (at), (count));\t \\\n+\t} while (0)\n\nAlso, count==1 is so frequent that this special case might deserve its\nown macro pair.\n\nI'm not inspired by these macro names, though.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"236897","messageId":"CAPig+cTs23=j_1OsB4FUUb1PZWubhad+XBCa1iEx4jChZE2x4w@mail.gmail.com","threadId":"36100","inReplyTo":"53270FC2.2030701@alum.mit.edu","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-17T19:06:02Z","receivedAt":"2014-03-17T19:06:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 17, 2014 at 11:07 AM, Michael Haggerty wrote:\n> On 03/17/2014 07:33 AM, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>>> Would it make sense to go one step further to introduce two macros\n>>> to make this kind of screw-up less likely?\n> potential callers that I noticed, ALLOC_GROW() was used immediately\n> before making space in the array for a new element. So I suggest\n> something more like\n>\n> +#define MOVE_DOWN(array, nr, at, count) \\\n> + memmove((array) + (at) + (count), \\\n> + (array) + (at), \\\n> + sizeof((array)[0]) * ((nr) - (at)))\n\nEach time I read these, my brain (for whatever reason) interprets the\nnames UP and DOWN opposite of the intended meaning, which makes them\nconfusing. Perhaps INSERT_GAP and CLOSE_GAP would avoid such problems,\nand be more consistent with Michael's proposed ALLOC_INSERT_GAP.\n\n> +#define ALLOC_INSERT_GAP(array, nr, at, count, alloc) \\\n> + do { \\\n> + ALLOC_GROW((array), (nr) + (count), (alloc)); \\\n> + MOVE_DOWN((array), (nr), (at), (count)); \\\n> + } while (0)\n>\n> Also, count==1 is so frequent that this special case might deserve its\n> own macro pair.\n>\n> I'm not inspired by these macro names, though.\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n"},{"id":"236927","messageId":"20140317220427.GA19300@sigill.intra.peff.net","threadId":"36100","inReplyTo":"CAPig+cTs23=j_1OsB4FUUb1PZWubhad+XBCa1iEx4jChZE2x4w@mail.gmail.com","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-17T22:04:27Z","receivedAt":"2014-03-17T22:04:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 17, 2014 at 03:06:02PM -0400, Eric Sunshine wrote:\n\n> On Mon, Mar 17, 2014 at 11:07 AM, Michael Haggerty wrote:\n> > On 03/17/2014 07:33 AM, Junio C Hamano wrote:\n> >> Junio C Hamano writes:\n> >>\n> >>> Would it make sense to go one step further to introduce two macros\n> >>> to make this kind of screw-up less likely?\n> > potential callers that I noticed, ALLOC_GROW() was used immediately\n> > before making space in the array for a new element. So I suggest\n> > something more like\n> >\n> > +#define MOVE_DOWN(array, nr, at, count) \\\n> > + memmove((array) + (at) + (count), \\\n> > + (array) + (at), \\\n> > + sizeof((array)[0]) * ((nr) - (at)))\n> \n> Each time I read these, my brain (for whatever reason) interprets the\n> names UP and DOWN opposite of the intended meaning, which makes them\n> confusing. Perhaps INSERT_GAP and CLOSE_GAP would avoid such problems,\n> and be more consistent with Michael's proposed ALLOC_INSERT_GAP.\n\nYeah, the UP/DOWN are very confusing to me. Something like SHRINK/EXPAND\n(with the latter handling the ALLOC_GROW for us) makes more sense to me.\nThose terms do not explicitly specify that we are doing it in the middle\n(whereas GAP does), but I think it is fairly obvious from the parameters\nwhat each is used for.\n\nSide note: I had _almost_ added something to my original email\nsuggesting more use of macros to embody common idioms. For example, in\nthe vast majority of malloc cases, you could using something like:\n\n #define ALLOC_OBJS(x,n) do { x = xmalloc(sizeof(*x) * (n)); } while(0)\n #define ALLOC_OBJ(x) ALLOC_OBJS(x,1)\n\nThat eliminates a whole possible class of errors. But it's also\nun-idiomatic as hell, and the resulting confusion can cause its own\nproblems. So I refrained from suggesting it.\n\nI think as long as a macro is expressing a more high-level intent,\nthough, paying that cost can be worth it. By itself, wrapping memmove\nto use sizeof(*array) does not seem all that exciting. But wrapping a\nfew specific cases like shrink/expand probably does make the code more\nreadable.\n\n-Peff\n"},{"id":"237019","messageId":"xmqqtxav5ebl.fsf@gitster.dls.corp.google.com","threadId":"36100","inReplyTo":"53270FC2.2030701@alum.mit.edu","subject":"Re: [PATCH] mv: prevent mismatched data when ignoring errors.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-18T22:31:26Z","receivedAt":"2014-03-18T22:31:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I had recently been thinking along the same lines. In many of the\n> potential callers that I noticed, ALLOC_GROW() was used immediately\n> before making space in the array for a new element. So I suggest\n> something more like\n>\n> +#define MOVE_DOWN(array, nr, at, count)\t\\\n> +\tmemmove((array) + (at) + (count),\t\t\\\n> +\t\t(array) + (at),\t\t\t\t\\\n> +\t\tsizeof((array)[0]) * ((nr) - (at)))\n> +#define ALLOC_INSERT_GAP(array, nr, at, count, alloc)\t\t \\\n> +\tdo {\t\t\t\t\t\t\t \\\n> +\t\tALLOC_GROW((array), (nr) + (count), (alloc)); \\\n> +\t\tMOVE_DOWN((array), (nr), (at), (count));\t \\\n> +\t} while (0)\n>\n> Also, count==1 is so frequent that this special case might deserve its\n> own macro pair.\n\nYeah, probably.\n\n> I'm not inspired by these macro names, though.\n\nMe neither, about ups and downs.\n\nPeff's suggestion to name these around the concept of \"gap\" sounded\nsensible.\n"}]}