{"thread":{"id":"46396","subject":"[PATCH] ls-files: don't try to prune an empty index","startedAt":"2017-07-15T20:11:39Z","lastAt":"2017-07-16T11:32:36Z","messageCount":9,"participants":["René Scharfe","Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"324590","messageId":"bbc51fb0-70ce-3b02-274b-31b5dc035e29@web.de","threadId":"46396","inReplyTo":null,"subject":"[PATCH] ls-files: don't try to prune an empty index","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-15T20:11:20Z","receivedAt":"2017-07-15T20:11:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Exit early when asked to prune an index that contains no\nentries to begin with. This avoids pointer arithmetic on\nistate->cache, which is possibly NULL in that case.\n\nFound with Clang's UBSan.\n\nSigned-off-by: Rene Scharfe \n---\n builtin/ls-files.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex b8514a0029..adf572da68 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate,\n \tint pos;\n \tunsigned int first, last;\n\n-\tif (!prefix)\n+\tif (!prefix || !istate->cache_nr)\n \t\treturn;\n \tpos = index_name_pos(istate, prefix, prefixlen);\n \tif (pos < 0)\n-- \n2.13.3\n"},{"id":"324593","messageId":"8fdcc21d-ccf9-7594-4d69-dfcad580da32@ramsayjones.plus.com","threadId":"46396","inReplyTo":"bbc51fb0-70ce-3b02-274b-31b5dc035e29@web.de","subject":"Re: [PATCH] ls-files: don't try to prune an empty index","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-07-16T00:28:04Z","receivedAt":"2017-07-16T00:28:10Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 15/07/17 21:11, René Scharfe wrote:\n> Exit early when asked to prune an index that contains no\n> entries to begin with. This avoids pointer arithmetic on\n> istate->cache, which is possibly NULL in that case.\n> \n> Found with Clang's UBSan.\n> \n> Signed-off-by: Rene Scharfe \n> ---\n> builtin/ls-files.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index b8514a0029..adf572da68 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate,\n> int pos;\n> unsigned int first, last;\n> \n> - if (!prefix)\n> + if (!prefix || !istate->cache_nr)\n> return;\n> pos = index_name_pos(istate, prefix, prefixlen);\n> if (pos < 0)\n\nMy patch looked like:\n\n- if (!prefix)\n+ if (!prefix || !istate->cache || istate->cache_nr == 0)\n\n... which is probably a bit 'belt-n-braces'. ;-)\n\nATB,\nRamsay Jones\n\n\n"},{"id":"324596","messageId":"ae5359cf-a480-f357-cb51-78f9d4cc431c@web.de","threadId":"46396","inReplyTo":"8fdcc21d-ccf9-7594-4d69-dfcad580da32@ramsayjones.plus.com","subject":"Re: [PATCH] ls-files: don't try to prune an empty index","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-16T03:52:42Z","receivedAt":"2017-07-16T03:53:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.07.2017 um 02:28 schrieb Ramsay Jones:\n> \n> \n> On 15/07/17 21:11, René Scharfe wrote:\n>> Exit early when asked to prune an index that contains no\n>> entries to begin with. This avoids pointer arithmetic on\n>> istate->cache, which is possibly NULL in that case.\n>>\n>> Found with Clang's UBSan.\n>>\n>> Signed-off-by: Rene Scharfe \n>> ---\n>> builtin/ls-files.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n>> index b8514a0029..adf572da68 100644\n>> --- a/builtin/ls-files.c\n>> +++ b/builtin/ls-files.c\n>> @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate,\n>> int pos;\n>> unsigned int first, last;\n>>\n>> - if (!prefix)\n>> + if (!prefix || !istate->cache_nr)\n>> return;\n>> pos = index_name_pos(istate, prefix, prefixlen);\n>> if (pos < 0)\n> \n> My patch looked like:\n> \n> - if (!prefix)\n> + if (!prefix || !istate->cache || istate->cache_nr == 0)\n> \n> ... which is probably a bit 'belt-n-braces'. ;-)\n\nNot checking for !istate->cache at this point is a good thing, I think.\nIf we have entries, then ->cache must not be NULL, and if it is we'd get\na segfault, notifying us that we have a bug. We could add an assert to\nstate this requirement explicitly, but that would be the topic of a\ndifferent patch.\n\nRené\n"},{"id":"324608","messageId":"20170716104114.7tfcuilsy3ab74n6@sigill.intra.peff.net","threadId":"46396","inReplyTo":"bbc51fb0-70ce-3b02-274b-31b5dc035e29@web.de","subject":"Re: [PATCH] ls-files: don't try to prune an empty index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-16T10:41:14Z","receivedAt":"2017-07-16T10:41:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 15, 2017 at 10:11:20PM +0200, René Scharfe wrote:\n\n> Exit early when asked to prune an index that contains no\n> entries to begin with. This avoids pointer arithmetic on\n> istate->cache, which is possibly NULL in that case.\n> \n> Found with Clang's UBSan.\n\nMakes sense. We could use MOVE_ARRAY() here, but this is a sensible\nshort-circuit to the whole function.\n\nLooks like a good solution.\n\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index b8514a0029..adf572da68 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate,\n> \tint pos;\n> \tunsigned int first, last;\n> \n> -\tif (!prefix)\n> +\tif (!prefix || !istate->cache_nr)\n> \t\treturn;\n> \tpos = index_name_pos(istate, prefix, prefixlen);\n> \tif (pos < 0)\n\n\"git am\" complained that this does not apply to its blobs. Did you\nhand-edit?\n\n-Peff\n"},{"id":"324612","messageId":"e91c204d-0bcb-9614-b62d-cd87c03b92e1@web.de","threadId":"46396","inReplyTo":"20170716104114.7tfcuilsy3ab74n6@sigill.intra.peff.net","subject":"Re: [PATCH] ls-files: don't try to prune an empty index","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-16T11:06:45Z","receivedAt":"2017-07-16T11:06:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.07.2017 um 12:41 schrieb Jeff King:\n> On Sat, Jul 15, 2017 at 10:11:20PM +0200, René Scharfe wrote:\n> \n>> Exit early when asked to prune an index that contains no\n>> entries to begin with. This avoids pointer arithmetic on\n>> istate->cache, which is possibly NULL in that case.\n>>\n>> Found with Clang's UBSan.\n> \n> Makes sense. We could use MOVE_ARRAY() here, but this is a sensible\n> short-circuit to the whole function.\n> \n> Looks like a good solution.\n> \n>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n>> index b8514a0029..adf572da68 100644\n>> --- a/builtin/ls-files.c\n>> +++ b/builtin/ls-files.c\n>> @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate,\n>> \tint pos;\n>> \tunsigned int first, last;\n>>\n>> -\tif (!prefix)\n>> +\tif (!prefix || !istate->cache_nr)\n>> \t\treturn;\n>> \tpos = index_name_pos(istate, prefix, prefixlen);\n>> \tif (pos < 0)\n> \n> \"git am\" complained that this does not apply to its blobs. Did you\n> hand-edit?\n\nI didn't, but perhaps I messed up the order of patches? MOVE_ARRAY\npatch 2 touches the same file, but I wouldn't expect the two changes to\nconflict. So not sure what's going on.\n\nRené\n"},{"id":"324613","messageId":"20170716110831.c442ihan6tzf7gmn@sigill.intra.peff.net","threadId":"46396","inReplyTo":"e91c204d-0bcb-9614-b62d-cd87c03b92e1@web.de","subject":"Re: [PATCH] ls-files: don't try to prune an empty index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-16T11:08:31Z","receivedAt":"2017-07-16T11:08:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 16, 2017 at 01:06:45PM +0200, René Scharfe wrote:\n\n> > > diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> > > index b8514a0029..adf572da68 100644\n> > > --- a/builtin/ls-files.c\n> > > +++ b/builtin/ls-files.c\n> > > @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate,\n> > > \tint pos;\n> > > \tunsigned int first, last;\n> > > \n> > > -\tif (!prefix)\n> > > +\tif (!prefix || !istate->cache_nr)\n> > > \t\treturn;\n> > > \tpos = index_name_pos(istate, prefix, prefixlen);\n> > > \tif (pos < 0)\n> > \n> > \"git am\" complained that this does not apply to its blobs. Did you\n> > hand-edit?\n> \n> I didn't, but perhaps I messed up the order of patches? MOVE_ARRAY\n> patch 2 touches the same file, but I wouldn't expect the two changes to\n> conflict. So not sure what's going on.\n\nFor some reason there's an extra space before the tab on each of the\ncontext lines. MUA issue or cut-and-paste, maybe?\n\n-Peff\n"},{"id":"324615","messageId":"4522dcd2-a896-4c41-89fd-c3f33a23a3d6@web.de","threadId":"46396","inReplyTo":"bbc51fb0-70ce-3b02-274b-31b5dc035e29@web.de","subject":"[PATCH (resend)] ls-files: don't try to prune an empty index","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-16T11:16:01Z","receivedAt":"2017-07-16T11:16:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Exit early when asked to prune an index that contains no entries to\nbegin with. This avoids pointer arithmetic on istate->cache, which is\npossibly NULL in that case.\n\nFound with Clang's UBSan.\n\nSigned-off-by: Rene Scharfe \n---\nMessed up whitespace when sending this the first time.\n\n builtin/ls-files.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex b8514a0029..adf572da68 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate,\n \tint pos;\n \tunsigned int first, last;\n \n-\tif (!prefix)\n+\tif (!prefix || !istate->cache_nr)\n \t\treturn;\n \tpos = index_name_pos(istate, prefix, prefixlen);\n \tif (pos < 0)\n-- \n2.13.3\n"},{"id":"324616","messageId":"31e6bebd-f2f5-b85d-2865-49611aecb2b8@web.de","threadId":"46396","inReplyTo":"20170716110831.c442ihan6tzf7gmn@sigill.intra.peff.net","subject":"Re: [PATCH] ls-files: don't try to prune an empty index","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-16T11:15:58Z","receivedAt":"2017-07-16T11:16:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.07.2017 um 13:08 schrieb Jeff King:\n> On Sun, Jul 16, 2017 at 01:06:45PM +0200, René Scharfe wrote:\n> \n>>>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n>>>> index b8514a0029..adf572da68 100644\n>>>> --- a/builtin/ls-files.c\n>>>> +++ b/builtin/ls-files.c\n>>>> @@ -362,7 +362,7 @@ static void prune_index(struct index_state *istate,\n>>>> \tint pos;\n>>>> \tunsigned int first, last;\n>>>>\n>>>> -\tif (!prefix)\n>>>> +\tif (!prefix || !istate->cache_nr)\n>>>> \t\treturn;\n>>>> \tpos = index_name_pos(istate, prefix, prefixlen);\n>>>> \tif (pos < 0)\n>>>\n>>> \"git am\" complained that this does not apply to its blobs. Did you\n>>> hand-edit?\n>>\n>> I didn't, but perhaps I messed up the order of patches? MOVE_ARRAY\n>> patch 2 touches the same file, but I wouldn't expect the two changes to\n>> conflict. So not sure what's going on.\n> \n> For some reason there's an extra space before the tab on each of the\n> context lines. MUA issue or cut-and-paste, maybe?\n\nThat's possible. Will resend.\n"},{"id":"324618","messageId":"c408be84-c35f-bf3a-4699-9dd0b5de93ef@web.de","threadId":"46396","inReplyTo":"31e6bebd-f2f5-b85d-2865-49611aecb2b8@web.de","subject":"Re: [PATCH] ls-files: don't try to prune an empty index","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-16T11:32:19Z","receivedAt":"2017-07-16T11:32:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.07.2017 um 13:15 schrieb René Scharfe:\n> Am 16.07.2017 um 13:08 schrieb Jeff King:\n>> On Sun, Jul 16, 2017 at 01:06:45PM +0200, René Scharfe wrote:\n>>\n>>>>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n>>>>> index b8514a0029..adf572da68 100644\n>>>>> --- a/builtin/ls-files.c\n>>>>> +++ b/builtin/ls-files.c\n>>>>> @@ -362,7 +362,7 @@ static void prune_index(struct index_state \n>>>>> *istate,\n>>>>> int pos;\n>>>>> unsigned int first, last;\n>>>>>\n>>>>> - if (!prefix)\n>>>>> + if (!prefix || !istate->cache_nr)\n>>>>> return;\n>>>>> pos = index_name_pos(istate, prefix, prefixlen);\n>>>>> if (pos < 0)\n>>>>\n>>>> \"git am\" complained that this does not apply to its blobs. Did you\n>>>> hand-edit?\n>>>\n>>> I didn't, but perhaps I messed up the order of patches? MOVE_ARRAY\n>>> patch 2 touches the same file, but I wouldn't expect the two changes to\n>>> conflict. So not sure what's going on.\n>>\n>> For some reason there's an extra space before the tab on each of the\n>> context lines. MUA issue or cut-and-paste, maybe?\n> \n> That's possible. Will resend.\n\n... pressed Send to fast. Thanks for reporting the broken patch!\n\nI use the extension Toggle Word Wrap with Thunderbird, but it wraps by\ndefault with no way to change that, so I forgot toggling this time.\nGrr, I've had enough of this! Went past the warranty warning and set\nmailnews.wraplength=0 now.\n\nRené\n"}]}