{"thread":{"id":"51804","subject":"[PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","startedAt":"2019-09-05T08:25:43Z","lastAt":"2019-09-06T01:36:09Z","messageCount":23,"participants":["Stephan Beyer","René Scharfe","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"381862","messageId":"20190905082459.26816-1-s-beyer@gmx.net","threadId":"51804","inReplyTo":null,"subject":"[PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2019-09-05T08:24:59Z","receivedAt":"2019-09-05T08:25:43Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"Compiler heuristics for detection of potentially uninitialized variables\nmay change between compiler versions and enabling link-time optimization\nmay find new warnings.  Indeed, compiling with gcc 9.2.1 and enabled\nlink-time optimization feature resulted in a few hits that are fixed by\nthis patch in the most naïve way.  This allows to compile git using the\nDEVELOPER=1 switch (which sets -Werror) and using the -flto flag.\n\nSigned-off-by: Stephan Beyer <s-beyer@gmx.net>\n---\n builtin/am.c               | 2 +-\n builtin/pack-objects.c     | 2 +-\n bulk-checkin.c             | 2 ++\n fast-import.c              | 3 ++-\n t/helper/test-read-cache.c | 2 +-\n 5 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1aea657a7f..ab914fd46e 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1266,7 +1266,7 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail)\n static void get_commit_info(struct am_state *state, struct commit *commit)\n {\n \tconst char *buffer, *ident_line, *msg;\n-\tsize_t ident_len;\n+\tsize_t ident_len = 0;\n \tstruct ident_split id;\n\n \tbuffer = logmsg_reencode(commit, NULL, get_commit_output_encoding());\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 76ce906946..d0c03b0e9b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1171,7 +1171,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,\n {\n \tstruct packed_git *found_pack = NULL;\n \toff_t found_offset = 0;\n-\tuint32_t index_pos;\n+\tuint32_t index_pos = 0;\n\n \tdisplay_progress(progress_state, ++nr_seen);\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 39ee7d6107..87fa28c227 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -200,6 +200,8 @@ static int deflate_to_pack(struct bulk_checkin_state *state,\n \tstruct hashfile_checkpoint checkpoint;\n \tstruct pack_idx_entry *idx = NULL;\n\n+\tcheckpoint.offset = 0;\n+\n \tseekback = lseek(fd, 0, SEEK_CUR);\n \tif (seekback == (off_t) -1)\n \t\treturn error(\"cannot find the current offset\");\ndiff --git a/fast-import.c b/fast-import.c\nindex b44d6a467e..58f73f9105 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -903,7 +903,8 @@ static int store_object(\n \tstruct object_entry *e;\n \tunsigned char hdr[96];\n \tstruct object_id oid;\n-\tunsigned long hdrlen, deltalen;\n+\tunsigned long hdrlen;\n+\tunsigned long deltalen = 0;\n \tgit_hash_ctx c;\n \tgit_zstream s;\n\ndiff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c\nindex 7e79b555de..ef0963e2f4 100644\n--- a/t/helper/test-read-cache.c\n+++ b/t/helper/test-read-cache.c\n@@ -4,7 +4,7 @@\n\n int cmd__read_cache(int argc, const char **argv)\n {\n-\tint i, cnt = 1, namelen;\n+\tint i, cnt = 1, namelen = 0;\n \tconst char *name = NULL;\n\n \tif (argc > 1 && skip_prefix(argv[1], \"--print-and-refresh=\", &name)) {\n--\n2.23.0.38.g7ab3f3815a\n\n"},{"id":"381882","messageId":"6c6c4e71-d9e5-1ad4-74db-12c323da42f7@web.de","threadId":"51804","inReplyTo":"20190905082459.26816-1-s-beyer@gmx.net","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-05T17:08:49Z","receivedAt":"2019-09-05T17:09:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.09.19 um 10:24 schrieb Stephan Beyer:\n> Compiler heuristics for detection of potentially uninitialized variables\n> may change between compiler versions and enabling link-time optimization\n> may find new warnings.  Indeed, compiling with gcc 9.2.1 and enabled\n> link-time optimization feature resulted in a few hits that are fixed by\n> this patch in the most naïve way.  This allows to compile git using the\n> DEVELOPER=1 switch (which sets -Werror) and using the -flto flag.\n>\n> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>\n> ---\n>  builtin/am.c               | 2 +-\n>  builtin/pack-objects.c     | 2 +-\n>  bulk-checkin.c             | 2 ++\n>  fast-import.c              | 3 ++-\n>  t/helper/test-read-cache.c | 2 +-\n>  5 files changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 1aea657a7f..ab914fd46e 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1266,7 +1266,7 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail)\n>  static void get_commit_info(struct am_state *state, struct commit *commit)\n>  {\n>  \tconst char *buffer, *ident_line, *msg;\n> -\tsize_t ident_len;\n> +\tsize_t ident_len = 0;\n>  \tstruct ident_split id;\n>\n>  \tbuffer = logmsg_reencode(commit, NULL, get_commit_output_encoding());\n\nFurther context:\n\n\tident_line = find_commit_header(buffer, \"author\", &ident_len);\n\n\tif (split_ident_line(&id, ident_line, ident_len) < 0)\n\t\tdie(_(\"invalid ident line: %.*s\"), (int)ident_len, ident_line);\n\nfind_commit_header() can return NULL.  split_ident_line() won't handle\nthat well.  So I think what's missing here is a NULL check.  If the\ncompiler is smart enough then that should silence the initialization\nwarning.\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 76ce906946..d0c03b0e9b 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1171,7 +1171,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,\n>  {\n>  \tstruct packed_git *found_pack = NULL;\n>  \toff_t found_offset = 0;\n> -\tuint32_t index_pos;\n> +\tuint32_t index_pos = 0;\n>\n>  \tdisplay_progress(progress_state, ++nr_seen);\n>\n\nFurther context:\n\n\tif (have_duplicate_entry(oid, exclude, &index_pos))\n\t\treturn 0;\n\n\tif (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {\n\t\t/* The pack is missing an object, so it will not have closure */\n\t\tif (write_bitmap_index) {\n\t\t\tif (write_bitmap_index != WRITE_BITMAP_QUIET)\n\t\t\t\twarning(_(no_closure_warning));\n\t\t\twrite_bitmap_index = 0;\n\t\t}\n\t\treturn 0;\n\t}\n\n\tcreate_object_entry(oid, type, pack_name_hash(name),\n\t\t\t    exclude, name && no_try_delta(name),\n\t\t\t    index_pos, found_pack, found_offset);\n\nSo we call have_duplicate_entry() and if it returns 0 then we might\nend up using index_pos.  So when does it return 0?\n\nstatic int have_duplicate_entry(const struct object_id *oid,\n\t\t\t\tint exclude,\n\t\t\t\tuint32_t *index_pos)\n{\n\tstruct object_entry *entry;\n\n\tentry = packlist_find(&to_pack, oid, index_pos);\n\tif (!entry)\n\t\treturn 0;\n\nOK, it does that if packlist_find() returns NULL.  When does it do\nthat?\nstruct object_entry *packlist_find(struct packing_data *pdata,\n\t\t\t\t   const struct object_id *oid,\n\t\t\t\t   uint32_t *index_pos)\n{\n\tuint32_t i;\n\tint found;\n\n\tif (!pdata->index_size)\n\t\treturn NULL;\n\n\ti = locate_object_entry_hash(pdata, oid, &found);\n\n\tif (index_pos)\n\t\t*index_pos = i;\n\n\tif (!found)\n\t\treturn NULL;\n\nSo if the packing list is empty then it returns NULL without setting\nindex_pos.  Hmm.  It does set it in all other cases, no matter if oid is\nfound or not.  Is it really a good idea to make that exception?  I\nsuspect always setting index_pos here would silence the compiler as well\nand fix the issue closer to its root.\n\nBut I may be missing something, this code looks complicated.\n\n> diff --git a/bulk-checkin.c b/bulk-checkin.c\n> index 39ee7d6107..87fa28c227 100644\n> --- a/bulk-checkin.c\n> +++ b/bulk-checkin.c\n> @@ -200,6 +200,8 @@ static int deflate_to_pack(struct bulk_checkin_state *state,\n>  \tstruct hashfile_checkpoint checkpoint;\n>  \tstruct pack_idx_entry *idx = NULL;\n>\n> +\tcheckpoint.offset = 0;\n> +\n>  \tseekback = lseek(fd, 0, SEEK_CUR);\n>  \tif (seekback == (off_t) -1)\n>  \t\treturn error(\"cannot find the current offset\");\n\nOmitting further context, even though it would help, but this reply is\nlong enough already.  It seems the compiler got confused -- I can't see\nan execution path that would use an uninitialized offset.  If idx is\nNULL then the function is exited early, and if it's not then offset is\ninitialized.  But perhaps I'm missing something.\n\nAnyway, my points are that simply initializing might not always be the\nbest fix, and that more context would help reviewers of such a patch,\nbut only if functions are reasonably short and it's not necessary to\nfollow the rabbit into a call chain hole.\n\nDidn't check the other cases.\n\nRené\n"},{"id":"381884","messageId":"xmqqlfv2r66y.fsf@gitster-ct.c.googlers.com","threadId":"51804","inReplyTo":"20190905082459.26816-1-s-beyer@gmx.net","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-05T17:41:57Z","receivedAt":"2019-09-05T17:42:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephan Beyer <s-beyer@gmx.net> writes:\n\n> diff --git a/fast-import.c b/fast-import.c\n> index b44d6a467e..58f73f9105 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -903,7 +903,8 @@ static int store_object(\n>  \tstruct object_entry *e;\n>  \tunsigned char hdr[96];\n>  \tstruct object_id oid;\n> -\tunsigned long hdrlen, deltalen;\n> +\tunsigned long hdrlen;\n> +\tunsigned long deltalen = 0;\n>  \tgit_hash_ctx c;\n>  \tgit_zstream s;\n\n[in my attempt to imitate Réne...]\n\nIn this function, deltalen is used only when delta != NULL, i.e.\n\n\tif (delta) {\n\t\ts.next_in = delta;\n\t\ts.avail_in = deltalen;\n\t} else {\n\t\ts.next_in = (void *)dat->buf;\n\t\ts.avail_in = dat->len;\n\t}\n\t...\n\tif (delta) {\n\t\t...\n\t\thdrlen = encode_in_pack_object_header(hdr, sizeof(hdr),\n\t\t\t\t\t\t      OBJ_OFS_DELTA, deltalen);\n\t...\n\nCould delta become non-NULL without deltalen getting set?  We see\nthese before all uses of delta/deltalen in this function.\n\n\tif (last && last->data.len && last->data.buf && last->depth < max_depth\n\t\t&& dat->len > the_hash_algo->rawsz) {\n\n\t\tdelta_count_attempts_by_type[type]++;\n\t\tdelta = diff_delta(last->data.buf, last->data.len,\n\t\t\tdat->buf, dat->len,\n\t\t\t&deltalen, dat->len - the_hash_algo->rawsz);\n\t} else\n\t\tdelta = NULL;\n\nIf diff_delta() returns non-NULL without touching deltalen, we'd be\nin trouble.  We see this in delta.h\n\nstatic inline void *\ndiff_delta(const void *src_buf, unsigned long src_bufsize,\n\t   const void *trg_buf, unsigned long trg_bufsize,\n\t   unsigned long *delta_size, unsigned long max_delta_size)\n{\n\tstruct delta_index *index = create_delta_index(src_buf, src_bufsize);\n\tif (index) {\n\t\tvoid *delta = create_delta(index, trg_buf, trg_bufsize,\n\t\t\t\t\t   delta_size, max_delta_size);\n\t\tfree_delta_index(index);\n\t\treturn delta;\n\t}\n\treturn NULL;\n}\n\nso the question is if create_delta() can return non-NULL without\ntouching delta_size.  In diff-delta.c::create_delta(), *delta_size\nis assigned once at the very end, when the function returns a\npointer to an allocated memory 'out'.  All the \"return\" statement\nother than that last one literally returns \"NULL\".\n\nSo it seems that this is a case the compiler getting confused.\n\n\n\n\n"},{"id":"381888","messageId":"20190905175304.GA23663@sigill.intra.peff.net","threadId":"51804","inReplyTo":"6c6c4e71-d9e5-1ad4-74db-12c323da42f7@web.de","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T17:53:05Z","receivedAt":"2019-09-05T17:53:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 05, 2019 at 07:08:49PM +0200, René Scharfe wrote:\n\n> Anyway, my points are that simply initializing might not always be the\n> best fix, and that more context would help reviewers of such a patch,\n> but only if functions are reasonably short and it's not necessary to\n> follow the rabbit into a call chain hole.\n\nI started looking at these, too, and came to the same conclusion.  Some\nof these are pointing to real bugs, and just silencing the warnings\nmisses the opportunity to find them.\n\nI'll comment on the ones I did look at.\n\n> Further context:\n> \n> \tident_line = find_commit_header(buffer, \"author\", &ident_len);\n> \n> \tif (split_ident_line(&id, ident_line, ident_len) < 0)\n> \t\tdie(_(\"invalid ident line: %.*s\"), (int)ident_len, ident_line);\n> \n> find_commit_header() can return NULL.  split_ident_line() won't handle\n> that well.  So I think what's missing here is a NULL check.  If the\n> compiler is smart enough then that should silence the initialization\n> warning.\n\nYeah, this one is a segfault waiting to happen, I think, if the author\nline is missing.\n\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 76ce906946..d0c03b0e9b 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -1171,7 +1171,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,\n> >  {\n> >  \tstruct packed_git *found_pack = NULL;\n> >  \toff_t found_offset = 0;\n> > -\tuint32_t index_pos;\n> > +\tuint32_t index_pos = 0;\n> [...]\n> So if the packing list is empty then it returns NULL without setting\n> index_pos.  Hmm.  It does set it in all other cases, no matter if oid is\n> found or not.  Is it really a good idea to make that exception?  I\n> suspect always setting index_pos here would silence the compiler as well\n> and fix the issue closer to its root.\n\nYeah, this is a weird one. That index_pos is actually a position in the\ncurrent hash table. And in the first object we see in pack-objects'\ninput, we definitely _do_ end up with a nonsense index_pos, which then\ngets passed to packlist_alloc().\n\nBut since we also need to grow the hash table during that allocation, we\ndon't use index_pos at all. So setting index_pos to something known like\n\"0\" is kind of weird, in that we don't have a hash position at all (the\ntable doesn't exist!). But it's probably the least bad thing. If we do\nthat, it should happen in packlist_find().\n\nI have to agree this whole \"passing around index_pos\" thing looks\ncomplicated. It seems like we're just saving ourselves one hash lookup\non insertion (and it's not even an expensive hash, since we're reusing\nthe oid).\n\nI suspect the whole thing would also be a lot simpler using one of our\nexisting hash implementations.\n\n> Didn't check the other cases.\n\nThe only other one I looked at was:\n\n>> int cmd__read_cache(int argc, const char **argv)\n>> {\n>>-       int i, cnt = 1, namelen;\n>>+       int i, cnt = 1, namelen = 0;\n\nI actually saw this one the other day, because it triggered for me when\ncompiling with SANITIZE=address. AFAICT it's a false positive. \"name\" is\nalways NULL unless skip_prefix() returns true, in which case we always\nset \"namelen\". And we only look at \"namelen\" if \"name\" is non-NULL.\n\nThis one doesn't even require LTO, because skip_prefix() is an inline\nfunction. I'm not sure why the compiler gets confused here. I don't mind\ninitializing namelen to 0 to silence it, though (we already set name to\nNULL, so this would just match).\n\n-Peff\n"},{"id":"381889","messageId":"xmqqd0ger5j7.fsf@gitster-ct.c.googlers.com","threadId":"51804","inReplyTo":"20190905082459.26816-1-s-beyer@gmx.net","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-05T17:56:12Z","receivedAt":"2019-09-05T17:56:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephan Beyer <s-beyer@gmx.net> writes:\n\n> diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c\n> index 7e79b555de..ef0963e2f4 100644\n> --- a/t/helper/test-read-cache.c\n> +++ b/t/helper/test-read-cache.c\n> @@ -4,7 +4,7 @@\n>\n>  int cmd__read_cache(int argc, const char **argv)\n>  {\n> -\tint i, cnt = 1, namelen;\n> +\tint i, cnt = 1, namelen = 0;\n>  \tconst char *name = NULL;\n>\n>  \tif (argc > 1 && skip_prefix(argv[1], \"--print-and-refresh=\", &name)) {\n\t\tnamelen = strlen(name);\n\nThe above is the only assignment to namelen in this function, and\nnamelen is used like so:\n\n\t\tif (name) {\n\t\t\t...\n\t\t\tpos = index_name_pos(&the_index, name, namelen);\n\nSo somebody does not realize that skip_prefix() returns true only\nwhen it touches name.  But skip_prefix() is inline and visible to\nthe compiler, and it is quite clear that name is only touched when \nthe function returns non-zero.\n\nstatic inline int skip_prefix(const char *str, const char *prefix,\n\t\t\t      const char **out)\n{\n\tdo {\n\t\tif (!*prefix) {\n\t\t\t*out = str;\n\t\t\treturn 1;\n\t\t}\n\t} while (*str++ == *prefix++);\n\treturn 0;\n}\n\nSo it looks like it is another case of compiler getting confused.\n"},{"id":"381891","messageId":"20190905180348.GC23663@sigill.intra.peff.net","threadId":"51804","inReplyTo":"xmqqd0ger5j7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T18:03:48Z","receivedAt":"2019-09-05T18:03:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 05, 2019 at 10:56:12AM -0700, Junio C Hamano wrote:\n\n> Stephan Beyer <s-beyer@gmx.net> writes:\n> \n> > diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c\n> > index 7e79b555de..ef0963e2f4 100644\n> > --- a/t/helper/test-read-cache.c\n> > +++ b/t/helper/test-read-cache.c\n> > @@ -4,7 +4,7 @@\n> >\n> >  int cmd__read_cache(int argc, const char **argv)\n> >  {\n> > -\tint i, cnt = 1, namelen;\n> > +\tint i, cnt = 1, namelen = 0;\n> >  \tconst char *name = NULL;\n> >\n> >  \tif (argc > 1 && skip_prefix(argv[1], \"--print-and-refresh=\", &name)) {\n> \t\tnamelen = strlen(name);\n> \n> The above is the only assignment to namelen in this function, and\n> namelen is used like so:\n> \n> \t\tif (name) {\n> \t\t\t...\n> \t\t\tpos = index_name_pos(&the_index, name, namelen);\n> \n> So somebody does not realize that skip_prefix() returns true only\n> when it touches name.  But skip_prefix() is inline and visible to\n> the compiler, and it is quite clear that name is only touched when \n> the function returns non-zero.\n\nI said earlier that I wouldn't mind seeing \"namelen = 0\" here. But I\nthink there is a much more direct solution: keeping the assignment and\npoint of use closer together. That makes it more clear both to the\ncompiler and to a human when we expect the variable to be valid. In\nfact, since it's only used once, we can drop the variable altogther. :)\n\ndiff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c\nindex 7e79b555de..244977a29b 100644\n--- a/t/helper/test-read-cache.c\n+++ b/t/helper/test-read-cache.c\n@@ -4,11 +4,10 @@\n \n int cmd__read_cache(int argc, const char **argv)\n {\n-\tint i, cnt = 1, namelen;\n+\tint i, cnt = 1;\n \tconst char *name = NULL;\n \n \tif (argc > 1 && skip_prefix(argv[1], \"--print-and-refresh=\", &name)) {\n-\t\tnamelen = strlen(name);\n \t\targc--;\n \t\targv++;\n \t}\n@@ -24,7 +23,7 @@ int cmd__read_cache(int argc, const char **argv)\n \n \t\t\trefresh_index(&the_index, REFRESH_QUIET,\n \t\t\t\t      NULL, NULL, NULL);\n-\t\t\tpos = index_name_pos(&the_index, name, namelen);\n+\t\t\tpos = index_name_pos(&the_index, name, strlen(name));\n \t\t\tif (pos < 0)\n \t\t\t\tdie(\"%s not in index\", name);\n \t\t\tprintf(\"%s is%s up to date\\n\", name,\n"},{"id":"381893","messageId":"xmqq36har4aq.fsf@gitster-ct.c.googlers.com","threadId":"51804","inReplyTo":"20190905180348.GC23663@sigill.intra.peff.net","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-05T18:22:53Z","receivedAt":"2019-09-05T18:22:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I said earlier that I wouldn't mind seeing \"namelen = 0\" here. But I\n> think there is a much more direct solution: keeping the assignment and\n> point of use closer together. That makes it more clear both to the\n> compiler and to a human when we expect the variable to be valid. In\n> fact, since it's only used once, we can drop the variable altogther. :)\n\nYeah, that sounds like a nice solution.\n\n> diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c\n> index 7e79b555de..244977a29b 100644\n> --- a/t/helper/test-read-cache.c\n> +++ b/t/helper/test-read-cache.c\n> @@ -4,11 +4,10 @@\n>  \n>  int cmd__read_cache(int argc, const char **argv)\n>  {\n> -\tint i, cnt = 1, namelen;\n> +\tint i, cnt = 1;\n>  \tconst char *name = NULL;\n>  \n>  \tif (argc > 1 && skip_prefix(argv[1], \"--print-and-refresh=\", &name)) {\n> -\t\tnamelen = strlen(name);\n>  \t\targc--;\n>  \t\targv++;\n>  \t}\n> @@ -24,7 +23,7 @@ int cmd__read_cache(int argc, const char **argv)\n>  \n>  \t\t\trefresh_index(&the_index, REFRESH_QUIET,\n>  \t\t\t\t      NULL, NULL, NULL);\n> -\t\t\tpos = index_name_pos(&the_index, name, namelen);\n> +\t\t\tpos = index_name_pos(&the_index, name, strlen(name));\n>  \t\t\tif (pos < 0)\n>  \t\t\t\tdie(\"%s not in index\", name);\n>  \t\t\tprintf(\"%s is%s up to date\\n\", name,\n"},{"id":"381899","messageId":"b4f53049-ff50-8143-1653-d321cce6421c@web.de","threadId":"51804","inReplyTo":"20190905175304.GA23663@sigill.intra.peff.net","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-05T19:09:27Z","receivedAt":"2019-09-05T19:09:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.09.19 um 19:53 schrieb Jeff King:\n>>> int cmd__read_cache(int argc, const char **argv)\n>>> {\n>>> -       int i, cnt = 1, namelen;\n>>> +       int i, cnt = 1, namelen = 0;\n>\n> I actually saw this one the other day, because it triggered for me when\n> compiling with SANITIZE=address. AFAICT it's a false positive. \"name\" is\n> always NULL unless skip_prefix() returns true, in which case we always\n> set \"namelen\". And we only look at \"namelen\" if \"name\" is non-NULL.\n>\n> This one doesn't even require LTO, because skip_prefix() is an inline\n> function. I'm not sure why the compiler gets confused here.\n\nYes, that's curious.\n\n> I don't mind\n> initializing namelen to 0 to silence it, though (we already set name to\n> NULL, so this would just match).\n\nPushing the strlen() call into the loop and getting rid of namelen should\nwork as well -- and I'd be surprised if this had a measurable performance\nimpact.\n\nRené\n"},{"id":"381900","messageId":"xmqqr24upmtm.fsf@gitster-ct.c.googlers.com","threadId":"51804","inReplyTo":"b4f53049-ff50-8143-1653-d321cce6421c@web.de","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-05T19:25:41Z","receivedAt":"2019-09-05T19:25:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Am 05.09.19 um 19:53 schrieb Jeff King:\n>>>> int cmd__read_cache(int argc, const char **argv)\n>>>> {\n>>>> -       int i, cnt = 1, namelen;\n>>>> +       int i, cnt = 1, namelen = 0;\n>>\n>> I actually saw this one the other day, because it triggered for me when\n>> compiling with SANITIZE=address. AFAICT it's a false positive. \"name\" is\n>> always NULL unless skip_prefix() returns true, in which case we always\n>> set \"namelen\". And we only look at \"namelen\" if \"name\" is non-NULL.\n>>\n>> This one doesn't even require LTO, because skip_prefix() is an inline\n>> function. I'm not sure why the compiler gets confused here.\n>\n> Yes, that's curious.\n>\n>> I don't mind\n>> initializing namelen to 0 to silence it, though (we already set name to\n>> NULL, so this would just match).\n>\n> Pushing the strlen() call into the loop and getting rid of namelen should\n> work as well -- and I'd be surprised if this had a measurable performance\n> impact.\n\nYeah, we are making strlen() call on a constant \"name\" in a loop\nover argv[].  I do not think it matters in this case, either.\n\n"},{"id":"381905","messageId":"8d90d803-8a4b-2656-550d-ccee5156b643@web.de","threadId":"51804","inReplyTo":"xmqqr24upmtm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-05T19:39:38Z","receivedAt":"2019-09-05T19:39:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.09.19 um 21:25 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> Am 05.09.19 um 19:53 schrieb Jeff King:\n>>>>> int cmd__read_cache(int argc, const char **argv)\n>>>>> {\n>>>>> -       int i, cnt = 1, namelen;\n>>>>> +       int i, cnt = 1, namelen = 0;\n>>>\n>>> I actually saw this one the other day, because it triggered for me when\n>>> compiling with SANITIZE=address. AFAICT it's a false positive. \"name\" is\n>>> always NULL unless skip_prefix() returns true, in which case we always\n>>> set \"namelen\". And we only look at \"namelen\" if \"name\" is non-NULL.\n>>>\n>>> This one doesn't even require LTO, because skip_prefix() is an inline\n>>> function. I'm not sure why the compiler gets confused here.\n>>\n>> Yes, that's curious.\n>>\n>>> I don't mind\n>>> initializing namelen to 0 to silence it, though (we already set name to\n>>> NULL, so this would just match).\n>>\n>> Pushing the strlen() call into the loop and getting rid of namelen should\n>> work as well -- and I'd be surprised if this had a measurable performance\n>> impact.\n>\n> Yeah, we are making strlen() call on a constant \"name\" in a loop\n> over argv[].  I do not think it matters in this case, either.\n\nThe loop count is either 1 or argv[1] interpreted as a number, i.e. it could\nbe very high.  Its body consists of an index load and writing a number to a\nfile, though -- a strlen() call on the name of that file should go unnoticed\namid that activity.  (I didn't measure it, though.)\n\nRené\n\n"},{"id":"381907","messageId":"xmqqimq6plof.fsf@gitster-ct.c.googlers.com","threadId":"51804","inReplyTo":"8d90d803-8a4b-2656-550d-ccee5156b643@web.de","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-05T19:50:24Z","receivedAt":"2019-09-05T19:50:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> The loop count is either 1 or argv[1] interpreted as a number, i.e. it could\n> be very high.  Its body consists of an index load and writing a number to a\n> file, though -- a strlen() call on the name of that file should go unnoticed\n> amid that activity.  (I didn't measure it, though.)\n\nYup, I didn't either but we reasoned along the same line.\n"},{"id":"381928","messageId":"20190905224859.GA28660@sigill.intra.peff.net","threadId":"51804","inReplyTo":"20190905082459.26816-1-s-beyer@gmx.net","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T22:48:59Z","receivedAt":"2019-09-05T22:49:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 05, 2019 at 10:24:59AM +0200, Stephan Beyer wrote:\n\n> Compiler heuristics for detection of potentially uninitialized variables\n> may change between compiler versions and enabling link-time optimization\n> may find new warnings.  Indeed, compiling with gcc 9.2.1 and enabled\n> link-time optimization feature resulted in a few hits that are fixed by\n> this patch in the most naïve way.  This allows to compile git using the\n> DEVELOPER=1 switch (which sets -Werror) and using the -flto flag.\n\nLots of discussion in this thread. Let's try to turn it into some\npatches. :)\n\nAfter the patches below, I can compile cleanly with gcc 9.2.1 using\n-flto with both -O2 and -O3 (some of the cases only seemed to trigger\nfor me with -O3).\n\nI've ordered them in decreasing value. The first one is a real bugfix,\nthe second is a related cleanup. The next 3 are appeasing the compiler,\nbut I think are a good idea (but note I went more for root causes than\nyour originals). The last one is perhaps more controversial, but IMHO\nworth doing.\n\n  [1/6]: git-am: handle missing \"author\" when parsing commit\n  [2/6]: pack-objects: use object_id in packlist_alloc()\n  [3/6]: bulk-checkin: zero-initialize hashfile_checkpoint\n  [4/6]: diff-delta: set size out-parameter to 0 for NULL delta\n  [5/6]: test-read-cache: drop namelen variable\n  [6/6]: pack-objects: drop packlist index_pos optimization\n\n builtin/am.c               |  4 +++-\n builtin/pack-objects.c     | 33 ++++++++++++++-------------------\n bulk-checkin.c             |  2 +-\n diff-delta.c               |  2 ++\n pack-bitmap-write.c        |  2 +-\n pack-bitmap.c              |  2 +-\n pack-objects.c             | 20 ++++++++++----------\n pack-objects.h             |  6 ++----\n t/helper/test-read-cache.c |  5 ++---\n 9 files changed, 36 insertions(+), 40 deletions(-)\n\n-Peff\n"},{"id":"381933","messageId":"20190905225031.GA25657@sigill.intra.peff.net","threadId":"51804","inReplyTo":"20190905224859.GA28660@sigill.intra.peff.net","subject":"[PATCH 1/6] git-am: handle missing \"author\" when parsing commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T22:50:31Z","receivedAt":"2019-09-05T22:50:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We try to parse the \"author\" line out of a commit buffer. We handle the\ncase that split_ident_line() doesn't work, but we don't do any error\nchecking that we found an \"author\" line in the first place! This would\ncause us to segfault on such a corrupt object.\n\nLet's put in an explicit NULL check (we can just die(), which is what a\nbogus split would do, too). As a bonus, this silences a warning when\ncompiling with gcc 9.2.1 using \"-flto -O3\", which claims that ident_len\nmay be uninitialized (it would only be if we had a NULL here).\n\nReported-by: Stephan Beyer <s-beyer@gmx.net>\nHelped-by: René Scharfe <l.s.r@web.de>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1aea657a7f..ee7305eaa6 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1272,7 +1272,9 @@ static void get_commit_info(struct am_state *state, struct commit *commit)\n \tbuffer = logmsg_reencode(commit, NULL, get_commit_output_encoding());\n \n \tident_line = find_commit_header(buffer, \"author\", &ident_len);\n-\n+\tif (!ident_line)\n+\t\tdie(_(\"missing author line in commit %s\"),\n+\t\t      oid_to_hex(&commit->object.oid));\n \tif (split_ident_line(&id, ident_line, ident_len) < 0)\n \t\tdie(_(\"invalid ident line: %.*s\"), (int)ident_len, ident_line);\n \n-- \n2.23.0.463.g883b23b1c5\n\n"},{"id":"381934","messageId":"20190905225224.GB25657@sigill.intra.peff.net","threadId":"51804","inReplyTo":"20190905224859.GA28660@sigill.intra.peff.net","subject":"[PATCH 2/6] pack-objects: use object_id in packlist_alloc()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T22:52:25Z","receivedAt":"2019-09-05T22:52:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The only caller of packlist_alloc() already has a \"struct object_id\",\nand we immediately copy the hash they pass us into our own object_id.\nLet's avoid the unnecessary round-trip to a raw sha1 pointer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nJust noticed since I was touching that function.\n\nThis is the second-to-last raw sha1 in pack-objects.c. The final one is\nslightly tricky to get rid of, because it comes from the raw base_ref\npointer we parse out of the packfile's mmap. I left it out of this\nseries, but I wouldn't mind if somebody wants to take a stab at it.\n\n builtin/pack-objects.c | 2 +-\n pack-objects.c         | 4 ++--\n pack-objects.h         | 2 +-\n 3 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 76ce906946..dc2a7e9ac0 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1147,7 +1147,7 @@ static void create_object_entry(const struct object_id *oid,\n {\n \tstruct object_entry *entry;\n \n-\tentry = packlist_alloc(&to_pack, oid->hash, index_pos);\n+\tentry = packlist_alloc(&to_pack, oid, index_pos);\n \tentry->hash = hash;\n \toe_set_type(entry, type);\n \tif (exclude)\ndiff --git a/pack-objects.c b/pack-objects.c\nindex 52560293b6..c1df08df1a 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -153,7 +153,7 @@ void prepare_packing_data(struct repository *r, struct packing_data *pdata)\n }\n \n struct object_entry *packlist_alloc(struct packing_data *pdata,\n-\t\t\t\t    const unsigned char *sha1,\n+\t\t\t\t    const struct object_id *oid,\n \t\t\t\t    uint32_t index_pos)\n {\n \tstruct object_entry *new_entry;\n@@ -177,7 +177,7 @@ struct object_entry *packlist_alloc(struct packing_data *pdata,\n \tnew_entry = pdata->objects + pdata->nr_objects++;\n \n \tmemset(new_entry, 0, sizeof(*new_entry));\n-\thashcpy(new_entry->idx.oid.hash, sha1);\n+\toidcpy(&new_entry->idx.oid, oid);\n \n \tif (pdata->index_size * 3 <= pdata->nr_objects * 4)\n \t\trehash_objects(pdata);\ndiff --git a/pack-objects.h b/pack-objects.h\nindex 857d43850b..47bf7ebf86 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -183,7 +183,7 @@ static inline void packing_data_unlock(struct packing_data *pdata)\n }\n \n struct object_entry *packlist_alloc(struct packing_data *pdata,\n-\t\t\t\t    const unsigned char *sha1,\n+\t\t\t\t    const struct object_id *oid,\n \t\t\t\t    uint32_t index_pos);\n \n struct object_entry *packlist_find(struct packing_data *pdata,\n-- \n2.23.0.463.g883b23b1c5\n\n"},{"id":"381935","messageId":"20190905225249.GC25657@sigill.intra.peff.net","threadId":"51804","inReplyTo":"20190905224859.GA28660@sigill.intra.peff.net","subject":"[PATCH 3/6] bulk-checkin: zero-initialize hashfile_checkpoint","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T22:52:49Z","receivedAt":"2019-09-05T22:52:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We declare a \"struct hashfile_checkpoint\" but only sometimes actually\ncall hashfile_checkpoint() on it. That makes it not immediately obvious\nthat it's valid when we later access its members.\n\nIn fact, the code is fine: we fill it in unconditionally in the while(1)\nloop as long as \"idx\" is non-NULL. And then if \"idx\" is NULL, we exit\nearly from the function (because we're just computing the hash, not\nactually writing), before we look at the struct.\n\nHowever, this does seem to confuse gcc 9.2.1's -Wmaybe-uninitialized\nwhen compiled with \"-flto -O2\" (probably because with LTO it can now\nrealize that our call to hashfile_truncate() does not set the members\neither). Let's zero-initialize the struct to tell the compiler, as well\nas any readers of the code, that all is well.\n\nReported-by: Stephan Beyer <s-beyer@gmx.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nWe could also have a HASHFILE_CHECKPOINT_INIT, but it seemed like\noverkill. I dunno.\n\n bulk-checkin.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 39ee7d6107..583aacb9e3 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -197,7 +197,7 @@ static int deflate_to_pack(struct bulk_checkin_state *state,\n \tgit_hash_ctx ctx;\n \tunsigned char obuf[16384];\n \tunsigned header_len;\n-\tstruct hashfile_checkpoint checkpoint;\n+\tstruct hashfile_checkpoint checkpoint = {0};\n \tstruct pack_idx_entry *idx = NULL;\n \n \tseekback = lseek(fd, 0, SEEK_CUR);\n-- \n2.23.0.463.g883b23b1c5\n\n"},{"id":"381936","messageId":"20190905225336.GD25657@sigill.intra.peff.net","threadId":"51804","inReplyTo":"20190905224859.GA28660@sigill.intra.peff.net","subject":"[PATCH 4/6] diff-delta: set size out-parameter to 0 for NULL delta","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T22:53:37Z","receivedAt":"2019-09-05T22:53:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we cannot generate a delta, we return NULL but leave delta_size\nuntouched. This is generally OK, as callers rely on NULL to decide if\nthe output is usable or not. But it can confuse compilers; in\nparticular, gcc 9.2.1 with \"-flto -O3\" complains in fast-import's\nstore_object() that delta_len may be used uninitialized.\n\nLet's change the diff-delta code to set the size explicitly to 0 for a\nNULL return. That silences the compiler and makes it easier to reason\nabout the result.\n\nReported-by: Stephan Beyer <s-beyer@gmx.net>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI suspect this same pattern of \"if we return error, out-parameters are\nundefined\" is used in a lot of other functions, too. And I wouldn't\nnecessarily want to go around changing all of them. But the fact that\nthis tickles the compiler makes me think it's worthwhile.\n\n diff-delta.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/diff-delta.c b/diff-delta.c\nindex e49643353b..77fea08dfb 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -326,6 +326,8 @@ create_delta(const struct delta_index *index,\n \tconst unsigned char *ref_data, *ref_top, *data, *top;\n \tunsigned char *out;\n \n+\t*delta_size = 0;\n+\n \tif (!trg_buf || !trg_size)\n \t\treturn NULL;\n \n-- \n2.23.0.463.g883b23b1c5\n\n"},{"id":"381937","messageId":"963e5249-82ca-8961-917a-f45c20b1affe@gmx.net","threadId":"51804","inReplyTo":"20190905224859.GA28660@sigill.intra.peff.net","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2019-09-05T22:53:49Z","receivedAt":"2019-09-05T22:54:14Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"On 9/6/19 12:48 AM, Jeff King wrote:\n> On Thu, Sep 05, 2019 at 10:24:59AM +0200, Stephan Beyer wrote:\n>\n>> Compiler heuristics for detection of potentially uninitialized variables\n>> may change between compiler versions and enabling link-time optimization\n>> may find new warnings.  Indeed, compiling with gcc 9.2.1 and enabled\n>> link-time optimization feature resulted in a few hits that are fixed by\n>> this patch in the most naïve way.  This allows to compile git using the\n>> DEVELOPER=1 switch (which sets -Werror) and using the -flto flag.\n>\n> Lots of discussion in this thread. Let's try to turn it into some\n> patches. :)\n\nI thought the same and just sent my version of it. :D\n\nStephan\n"},{"id":"381938","messageId":"20190905225431.GE25657@sigill.intra.peff.net","threadId":"51804","inReplyTo":"20190905224859.GA28660@sigill.intra.peff.net","subject":"[PATCH 5/6] test-read-cache: drop namelen variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T22:54:31Z","receivedAt":"2019-09-05T22:54:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Early in the function we set \"namelen = strlen(name)\" if \"name\" is\nnon-NULL. Later, we use \"namelen\" only if \"name\" is non-NULL. However,\nit's hard to immediately see this, and it seems to confuse gcc 9.2.1\n(with \"-flto\" interestingly, though all of the involved logic is in\ninline functions; it also triggers when building with ASan).\n\nLet's simplify the code and remove the variable entirely. There's only\none use of namelen anyway, so we can just call strlen() then. It's true\nthis is in a loop, so we might execute strlen() more often. But:\n\n  - this is test code that only ever loops twice in our test suite (we\n    do loop 1000 times in a t/perf test, but without using this option).\n\n  - a decent compiler ought to be able to hoist that out of the loop\n    anyway (though I wouldn't count on gcc 9.2.1 doing so!)\n\nReported-by: Stephan Beyer <s-beyer@gmx.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/helper/test-read-cache.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c\nindex 7e79b555de..244977a29b 100644\n--- a/t/helper/test-read-cache.c\n+++ b/t/helper/test-read-cache.c\n@@ -4,11 +4,10 @@\n \n int cmd__read_cache(int argc, const char **argv)\n {\n-\tint i, cnt = 1, namelen;\n+\tint i, cnt = 1;\n \tconst char *name = NULL;\n \n \tif (argc > 1 && skip_prefix(argv[1], \"--print-and-refresh=\", &name)) {\n-\t\tnamelen = strlen(name);\n \t\targc--;\n \t\targv++;\n \t}\n@@ -24,7 +23,7 @@ int cmd__read_cache(int argc, const char **argv)\n \n \t\t\trefresh_index(&the_index, REFRESH_QUIET,\n \t\t\t\t      NULL, NULL, NULL);\n-\t\t\tpos = index_name_pos(&the_index, name, namelen);\n+\t\t\tpos = index_name_pos(&the_index, name, strlen(name));\n \t\t\tif (pos < 0)\n \t\t\t\tdie(\"%s not in index\", name);\n \t\t\tprintf(\"%s is%s up to date\\n\", name,\n-- \n2.23.0.463.g883b23b1c5\n\n"},{"id":"381939","messageId":"20190905225640.GF25657@sigill.intra.peff.net","threadId":"51804","inReplyTo":"20190905224859.GA28660@sigill.intra.peff.net","subject":"[PATCH 6/6] pack-objects: drop packlist index_pos optimization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T22:56:41Z","receivedAt":"2019-09-05T22:56:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once upon a time, the code to add an object to our packing list in\npack-objects all lived in a single function. It computed the position\nwithin the hash table once, then used it to check if the object was\nalready present, and if not, to add it.\n\nLater, in 2834bc27c1 (pack-objects: refactor the packing list,\n2013-10-24), this was split into two functions: packlist_find() and\npacklist_alloc(). We ended up with an \"index_pos\" variable that gets\npassed through several functions to make it from one to the other.\n\nThe resulting code is rather confusing to follow. The \"index_pos\"\nvariable is sometimes undefined, if we don't yet have a hash table. This\nworks out in practice because in that case packlist_alloc() won't use\nour index_pos at all, since it will have to create/grow the hash table\nfresh (and reassign the index for all entries). But it's hard to verify\nthat, and it does cause gcc 9.2.1's -Wmaybe-uninitialized to complain\nwhen compiled with \"-flto -O3\" (rightfully, since we do pass the\nuninitialized value as a function parameter, even if nobody ends up\nusing it).\n\nAll of this is to save computing the hash index again when we're\ninserting into the hash table, which I found doesn't make a measurable\ndifference in the program runtime (which is not surprising, since we're\ndoing all kinds of other heavyweight things for each object).\n\nLet's just drop this index_pos variable entirely, simplifying the code\n(and pleasing the compiler).\n\nWe might be better still refactoring this custom hash table to use one\nof our existing implementations (an oidmap, or a kh_oid_map). I stopped\nshort of that here, but this would be the likely first step towards that\nanyway.\n\nReported-by: Stephan Beyer <s-beyer@gmx.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI suspect one advantage of this custom hash is that it stores a uint32_t\ninstead of the hash, probably saving 16 bytes per object. But that's\nsomething we can think about when looking at converting the hash.\n\n builtin/pack-objects.c | 33 ++++++++++++++-------------------\n pack-bitmap-write.c    |  2 +-\n pack-bitmap.c          |  2 +-\n pack-objects.c         | 18 +++++++++---------\n pack-objects.h         |  6 ++----\n 5 files changed, 27 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex dc2a7e9ac0..9a8d935700 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -610,12 +610,12 @@ static int mark_tagged(const char *path, const struct object_id *oid, int flag,\n \t\t       void *cb_data)\n {\n \tstruct object_id peeled;\n-\tstruct object_entry *entry = packlist_find(&to_pack, oid, NULL);\n+\tstruct object_entry *entry = packlist_find(&to_pack, oid);\n \n \tif (entry)\n \t\tentry->tagged = 1;\n \tif (!peel_ref(path, &peeled)) {\n-\t\tentry = packlist_find(&to_pack, &peeled, NULL);\n+\t\tentry = packlist_find(&to_pack, &peeled);\n \t\tif (entry)\n \t\t\tentry->tagged = 1;\n \t}\n@@ -996,12 +996,11 @@ static int no_try_delta(const char *path)\n  * few lines later when we want to add the new entry.\n  */\n static int have_duplicate_entry(const struct object_id *oid,\n-\t\t\t\tint exclude,\n-\t\t\t\tuint32_t *index_pos)\n+\t\t\t\tint exclude)\n {\n \tstruct object_entry *entry;\n \n-\tentry = packlist_find(&to_pack, oid, index_pos);\n+\tentry = packlist_find(&to_pack, oid);\n \tif (!entry)\n \t\treturn 0;\n \n@@ -1141,13 +1140,12 @@ static void create_object_entry(const struct object_id *oid,\n \t\t\t\tuint32_t hash,\n \t\t\t\tint exclude,\n \t\t\t\tint no_try_delta,\n-\t\t\t\tuint32_t index_pos,\n \t\t\t\tstruct packed_git *found_pack,\n \t\t\t\toff_t found_offset)\n {\n \tstruct object_entry *entry;\n \n-\tentry = packlist_alloc(&to_pack, oid, index_pos);\n+\tentry = packlist_alloc(&to_pack, oid);\n \tentry->hash = hash;\n \toe_set_type(entry, type);\n \tif (exclude)\n@@ -1171,11 +1169,10 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,\n {\n \tstruct packed_git *found_pack = NULL;\n \toff_t found_offset = 0;\n-\tuint32_t index_pos;\n \n \tdisplay_progress(progress_state, ++nr_seen);\n \n-\tif (have_duplicate_entry(oid, exclude, &index_pos))\n+\tif (have_duplicate_entry(oid, exclude))\n \t\treturn 0;\n \n \tif (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {\n@@ -1190,7 +1187,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,\n \n \tcreate_object_entry(oid, type, pack_name_hash(name),\n \t\t\t    exclude, name && no_try_delta(name),\n-\t\t\t    index_pos, found_pack, found_offset);\n+\t\t\t    found_pack, found_offset);\n \treturn 1;\n }\n \n@@ -1199,17 +1196,15 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,\n \t\t\t\t\tint flags, uint32_t name_hash,\n \t\t\t\t\tstruct packed_git *pack, off_t offset)\n {\n-\tuint32_t index_pos;\n-\n \tdisplay_progress(progress_state, ++nr_seen);\n \n-\tif (have_duplicate_entry(oid, 0, &index_pos))\n+\tif (have_duplicate_entry(oid, 0))\n \t\treturn 0;\n \n \tif (!want_object_in_pack(oid, 0, &pack, &offset))\n \t\treturn 0;\n \n-\tcreate_object_entry(oid, type, name_hash, 0, 0, index_pos, pack, offset);\n+\tcreate_object_entry(oid, type, name_hash, 0, 0, pack, offset);\n \treturn 1;\n }\n \n@@ -1507,7 +1502,7 @@ static int can_reuse_delta(const unsigned char *base_sha1,\n \t * First see if we're already sending the base (or it's explicitly in\n \t * our \"excluded\" list).\n \t */\n-\tbase = packlist_find(&to_pack, &base_oid, NULL);\n+\tbase = packlist_find(&to_pack, &base_oid);\n \tif (base) {\n \t\tif (!in_same_island(&delta->idx.oid, &base->idx.oid))\n \t\t\treturn 0;\n@@ -2579,7 +2574,7 @@ static void add_tag_chain(const struct object_id *oid)\n \t * it was included via bitmaps, we would not have parsed it\n \t * previously).\n \t */\n-\tif (packlist_find(&to_pack, oid, NULL))\n+\tif (packlist_find(&to_pack, oid))\n \t\treturn;\n \n \ttag = lookup_tag(the_repository, oid);\n@@ -2603,7 +2598,7 @@ static int add_ref_tag(const char *path, const struct object_id *oid, int flag,\n \n \tif (starts_with(path, \"refs/tags/\") && /* is a tag? */\n \t    !peel_ref(path, &peeled)    && /* peelable? */\n-\t    packlist_find(&to_pack, &peeled, NULL))      /* object packed? */\n+\t    packlist_find(&to_pack, &peeled))      /* object packed? */\n \t\tadd_tag_chain(oid);\n \treturn 0;\n }\n@@ -2803,7 +2798,7 @@ static void show_object(struct object *obj, const char *name, void *data)\n \t\tfor (p = strchr(name, '/'); p; p = strchr(p + 1, '/'))\n \t\t\tdepth++;\n \n-\t\tent = packlist_find(&to_pack, &obj->oid, NULL);\n+\t\tent = packlist_find(&to_pack, &obj->oid);\n \t\tif (ent && depth > oe_tree_depth(&to_pack, ent))\n \t\t\toe_set_tree_depth(&to_pack, ent, depth);\n \t}\n@@ -3034,7 +3029,7 @@ static void loosen_unused_packed_objects(void)\n \n \t\tfor (i = 0; i < p->num_objects; i++) {\n \t\t\tnth_packed_object_oid(&oid, p, i);\n-\t\t\tif (!packlist_find(&to_pack, &oid, NULL) &&\n+\t\t\tif (!packlist_find(&to_pack, &oid) &&\n \t\t\t    !has_sha1_pack_kept_or_nonlocal(&oid) &&\n \t\t\t    !loosened_object_can_be_discarded(&oid, p->mtime))\n \t\t\t\tif (force_object_loose(&oid, p->mtime))\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex fa78a460c9..a7a4964b50 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -144,7 +144,7 @@ static inline void reset_all_seen(void)\n \n static uint32_t find_object_pos(const struct object_id *oid)\n {\n-\tstruct object_entry *entry = packlist_find(writer.to_pack, oid, NULL);\n+\tstruct object_entry *entry = packlist_find(writer.to_pack, oid);\n \n \tif (!entry) {\n \t\tdie(\"Failed to write bitmap index. Packfile doesn't have full closure \"\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ed2befaac6..84cd1bed4a 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1063,7 +1063,7 @@ int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,\n \n \t\tentry = &bitmap_git->pack->revindex[i];\n \t\tnth_packed_object_oid(&oid, bitmap_git->pack, entry->nr);\n-\t\toe = packlist_find(mapping, &oid, NULL);\n+\t\toe = packlist_find(mapping, &oid);\n \n \t\tif (oe)\n \t\t\treposition[i] = oe_in_pack_pos(mapping, oe) + 1;\ndiff --git a/pack-objects.c b/pack-objects.c\nindex c1df08df1a..3624090593 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -68,8 +68,7 @@ static void rehash_objects(struct packing_data *pdata)\n }\n \n struct object_entry *packlist_find(struct packing_data *pdata,\n-\t\t\t\t   const struct object_id *oid,\n-\t\t\t\t   uint32_t *index_pos)\n+\t\t\t\t   const struct object_id *oid)\n {\n \tuint32_t i;\n \tint found;\n@@ -79,9 +78,6 @@ struct object_entry *packlist_find(struct packing_data *pdata,\n \n \ti = locate_object_entry_hash(pdata, oid, &found);\n \n-\tif (index_pos)\n-\t\t*index_pos = i;\n-\n \tif (!found)\n \t\treturn NULL;\n \n@@ -153,8 +149,7 @@ void prepare_packing_data(struct repository *r, struct packing_data *pdata)\n }\n \n struct object_entry *packlist_alloc(struct packing_data *pdata,\n-\t\t\t\t    const struct object_id *oid,\n-\t\t\t\t    uint32_t index_pos)\n+\t\t\t\t    const struct object_id *oid)\n {\n \tstruct object_entry *new_entry;\n \n@@ -181,8 +176,13 @@ struct object_entry *packlist_alloc(struct packing_data *pdata,\n \n \tif (pdata->index_size * 3 <= pdata->nr_objects * 4)\n \t\trehash_objects(pdata);\n-\telse\n-\t\tpdata->index[index_pos] = pdata->nr_objects;\n+\telse {\n+\t\tint found;\n+\t\tuint32_t pos = locate_object_entry_hash(pdata,\n+\t\t\t\t\t\t\t&new_entry->idx.oid,\n+\t\t\t\t\t\t\t&found);\n+\t\tpdata->index[pos] = pdata->nr_objects;\n+\t}\n \n \tif (pdata->in_pack)\n \t\tpdata->in_pack[pdata->nr_objects - 1] = NULL;\ndiff --git a/pack-objects.h b/pack-objects.h\nindex 47bf7ebf86..6fe6ae5ee8 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -183,12 +183,10 @@ static inline void packing_data_unlock(struct packing_data *pdata)\n }\n \n struct object_entry *packlist_alloc(struct packing_data *pdata,\n-\t\t\t\t    const struct object_id *oid,\n-\t\t\t\t    uint32_t index_pos);\n+\t\t\t\t    const struct object_id *oid);\n \n struct object_entry *packlist_find(struct packing_data *pdata,\n-\t\t\t\t   const struct object_id *oid,\n-\t\t\t\t   uint32_t *index_pos);\n+\t\t\t\t   const struct object_id *oid);\n \n static inline uint32_t pack_name_hash(const char *name)\n {\n-- \n2.23.0.463.g883b23b1c5\n"},{"id":"381940","messageId":"20190905225802.GA19526@sigill.intra.peff.net","threadId":"51804","inReplyTo":"963e5249-82ca-8961-917a-f45c20b1affe@gmx.net","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-05T22:58:02Z","receivedAt":"2019-09-05T22:58:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 12:53:49AM +0200, Stephan Beyer wrote:\n\n> On 9/6/19 12:48 AM, Jeff King wrote:\n> > On Thu, Sep 05, 2019 at 10:24:59AM +0200, Stephan Beyer wrote:\n> >\n> >> Compiler heuristics for detection of potentially uninitialized variables\n> >> may change between compiler versions and enabling link-time optimization\n> >> may find new warnings.  Indeed, compiling with gcc 9.2.1 and enabled\n> >> link-time optimization feature resulted in a few hits that are fixed by\n> >> this patch in the most naïve way.  This allows to compile git using the\n> >> DEVELOPER=1 switch (which sets -Werror) and using the -flto flag.\n> >\n> > Lots of discussion in this thread. Let's try to turn it into some\n> > patches. :)\n> \n> I thought the same and just sent my version of it. :D\n\nYeah, I see that our mails crossed. :) I like some of my versions\nbetter, but I'm OK with either (or a mix-and-match).\n\n-Peff\n"},{"id":"381941","messageId":"6894a758-d4ee-92e8-1aab-fc44c9ddcdc3@gmx.net","threadId":"51804","inReplyTo":"20190905225802.GA19526@sigill.intra.peff.net","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Stephan Beyer","fromEmail":"s-beyer@gmx.net","sentAt":"2019-09-05T23:10:46Z","receivedAt":"2019-09-05T23:11:08Z","isPatch":true,"sender":{"key":"s-beyer@gmx.net","avatar":"https://avatars.githubusercontent.com/u/143889?v=4"},"body":"On 9/6/19 12:58 AM, Jeff King wrote:\n> On Fri, Sep 06, 2019 at 12:53:49AM +0200, Stephan Beyer wrote:\n>\n>> On 9/6/19 12:48 AM, Jeff King wrote:\n>>> On Thu, Sep 05, 2019 at 10:24:59AM +0200, Stephan Beyer wrote:\n>>>\n>>>> Compiler heuristics for detection of potentially uninitialized variables\n>>>> may change between compiler versions and enabling link-time optimization\n>>>> may find new warnings.  Indeed, compiling with gcc 9.2.1 and enabled\n>>>> link-time optimization feature resulted in a few hits that are fixed by\n>>>> this patch in the most naïve way.  This allows to compile git using the\n>>>> DEVELOPER=1 switch (which sets -Werror) and using the -flto flag.\n>>>\n>>> Lots of discussion in this thread. Let's try to turn it into some\n>>> patches. :)\n>>\n>> I thought the same and just sent my version of it. :D\n>\n> Yeah, I see that our mails crossed. :) I like some of my versions\n> better, but I'm OK with either (or a mix-and-match).\n\nI took a quick glance at yours. I also noticed the issue you address in\n[PATCH 2/6], but I was unsure if this is the way to go (I'm only\noccasionally reading on this list). I would prefer your patch series,\nwith maybe one exception...\n\nThe thing is: I had *exactly* the same commit like your [PATCH 6/6]\n(except for the commit message and for the number), but I dropped it.\nWhy? Because I had the feeling (no particular instance though) that the\nsecond locate_object_entry_hash() for each insertion *can* indeed take\n\"too much\" time. Also, I was wondering, if the \"found = 1\" case should\nbe catched as a BUG(\"should not happen\") or something.\n\nI don't care much, though. The performance impact should probably be\nchecked carefully.\n\nStephan\n"},{"id":"381942","messageId":"20190906012430.GA29104@sigill.intra.peff.net","threadId":"51804","inReplyTo":"6894a758-d4ee-92e8-1aab-fc44c9ddcdc3@gmx.net","subject":"Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T01:24:30Z","receivedAt":"2019-09-06T01:24:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 01:10:46AM +0200, Stephan Beyer wrote:\n\n> I took a quick glance at yours. I also noticed the issue you address in\n> [PATCH 2/6], but I was unsure if this is the way to go (I'm only\n> occasionally reading on this list). I would prefer your patch series,\n> with maybe one exception...\n\nYeah, we've been slowly removing these old \"unsigned char *\" references\n(see a7db4c193d98f for the latest round touching this same code -- I'm\nactually surprised I missed this one back then, as that's when\npacklist_find() got converted).\n\n> The thing is: I had *exactly* the same commit like your [PATCH 6/6]\n> (except for the commit message and for the number), but I dropped it.\n> Why? Because I had the feeling (no particular instance though) that the\n> second locate_object_entry_hash() for each insertion *can* indeed take\n> \"too much\" time. Also, I was wondering, if the \"found = 1\" case should\n> be catched as a BUG(\"should not happen\") or something.\n> \n> I don't care much, though. The performance impact should probably be\n> checked carefully.\n\nI did measure it, like this:\n\n  # Do the traversal separately. This would make any difference in the\n  # pack-objects hash code stand out _more_, plus it makes it cheaper to\n  # run multiple trials.\n  git rev-list --objects --all >input\n\n  # Make sure stderr is redirected to avoid progress, which again would\n  # amplify any differences.\n  git pack-objects --stdout --delta-base-offset <input >/dev/null 2>&1\n\nRunning on linux.git, I got:\n\n  [before]\n  Benchmark #1: git pack-objects --stdout --delta-base-offset <input >/dev/null 2>&1\n    Time (mean ± σ):     26.225 s ±  0.233 s    [User: 24.089 s, System: 4.867 s]\n    Range (min … max):   25.915 s … 26.555 s    10 runs\n\n  [after]\n  Benchmark #1: git pack-objects --stdout --delta-base-offset <input >/dev/null 2>&1\n    Time (mean ± σ):     26.129 s ±  0.170 s    [User: 24.003 s, System: 4.958 s]\n    Range (min … max):   25.974 s … 26.570 s    10 runs\n\nSo actually faster after, though not statistically significant. ;)\n\nThe BUG() on \"found==1\" might be worth doing.\n\n-Peff\n"},{"id":"381943","messageId":"20190906013604.GA32054@sigill.intra.peff.net","threadId":"51804","inReplyTo":"20190906012430.GA29104@sigill.intra.peff.net","subject":"[PATCH v2 6/6] pack-objects: drop packlist index_pos optimization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T01:36:05Z","receivedAt":"2019-09-06T01:36:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 05, 2019 at 09:24:30PM -0400, Jeff King wrote:\n\n> Running on linux.git, I got:\n> \n>   [before]\n>   Benchmark #1: git pack-objects --stdout --delta-base-offset <input >/dev/null 2>&1\n>     Time (mean ± σ):     26.225 s ±  0.233 s    [User: 24.089 s, System: 4.867 s]\n>     Range (min … max):   25.915 s … 26.555 s    10 runs\n> \n>   [after]\n>   Benchmark #1: git pack-objects --stdout --delta-base-offset <input >/dev/null 2>&1\n>     Time (mean ± σ):     26.129 s ±  0.170 s    [User: 24.003 s, System: 4.958 s]\n>     Range (min … max):   25.974 s … 26.570 s    10 runs\n> \n> So actually faster after, though not statistically significant. ;)\n\nBy the way, I wondered if LTO actually helped here. Those are both\ncompiled with -O2. Here's the same measurement with \"-flto -O2\":\n\n  Benchmark #1: git pack-objects --stdout --delta-base-offset <input >/dev/null 2>&1\n  Time (mean ± σ):     26.276 s ±  0.131 s    [User: 24.153 s, System: 4.900 s]\n  Range (min … max):   26.111 s … 26.469 s    10 runs\n\nSlightly slower, but still within the noise.\n\n> The BUG() on \"found==1\" might be worth doing.\n\nAnd here's a revised version of patch 6 with that (it matches the\nduplicate check that rehash_objects does, too, which is a nice\nsymmetry).\n\n-- >8 --\nSubject: [PATCH v2] pack-objects: drop packlist index_pos optimization\n\nOnce upon a time, the code to add an object to our packing list in\npack-objects all lived in a single function. It computed the position\nwithin the hash table once, then used it to check if the object was\nalready present, and if not, to add it.\n\nLater, in 2834bc27c1 (pack-objects: refactor the packing list,\n2013-10-24), this was split into two functions: packlist_find() and\npacklist_alloc(). We ended up with an \"index_pos\" variable that gets\npassed through several functions to make it from one to the other.\n\nThe resulting code is rather confusing to follow. The \"index_pos\"\nvariable is sometimes undefined, if we don't yet have a hash table. This\nworks out in practice because in that case packlist_alloc() won't use it\nat all, since it will have to create/grow the hash table. But it's hard\nto verify that, and it does cause gcc 9.2.1's -Wmaybe-uninitialized to\ncomplain when compiled with \"-flto -O3\" (rightfully, since we do pass\nthe uninitialized value as a function parameter, even if nobody ends up\nusing it).\n\nAll of this is to save computing the hash index again when we're\ninserting into the hash table, which I found doesn't make a measurable\ndifference in the program runtime (which is not surprising, since we're\ndoing all kinds of other heavyweight things for each object).\n\nLet's just drop this index_pos variable entirely, simplifying the code\n(and pleasing the compiler).\n\nWe might be better still refactoring this custom hash table to use one\nof our existing implementations (an oidmap, or a kh_oid_map). I stopped\nshort of that here, but this would be the likely first step towards that\nanyway.\n\nReported-by: Stephan Beyer <s-beyer@gmx.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c | 33 ++++++++++++++-------------------\n pack-bitmap-write.c    |  2 +-\n pack-bitmap.c          |  2 +-\n pack-objects.c         | 20 +++++++++++---------\n pack-objects.h         |  6 ++----\n 5 files changed, 29 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex dc2a7e9ac0..9a8d935700 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -610,12 +610,12 @@ static int mark_tagged(const char *path, const struct object_id *oid, int flag,\n \t\t       void *cb_data)\n {\n \tstruct object_id peeled;\n-\tstruct object_entry *entry = packlist_find(&to_pack, oid, NULL);\n+\tstruct object_entry *entry = packlist_find(&to_pack, oid);\n \n \tif (entry)\n \t\tentry->tagged = 1;\n \tif (!peel_ref(path, &peeled)) {\n-\t\tentry = packlist_find(&to_pack, &peeled, NULL);\n+\t\tentry = packlist_find(&to_pack, &peeled);\n \t\tif (entry)\n \t\t\tentry->tagged = 1;\n \t}\n@@ -996,12 +996,11 @@ static int no_try_delta(const char *path)\n  * few lines later when we want to add the new entry.\n  */\n static int have_duplicate_entry(const struct object_id *oid,\n-\t\t\t\tint exclude,\n-\t\t\t\tuint32_t *index_pos)\n+\t\t\t\tint exclude)\n {\n \tstruct object_entry *entry;\n \n-\tentry = packlist_find(&to_pack, oid, index_pos);\n+\tentry = packlist_find(&to_pack, oid);\n \tif (!entry)\n \t\treturn 0;\n \n@@ -1141,13 +1140,12 @@ static void create_object_entry(const struct object_id *oid,\n \t\t\t\tuint32_t hash,\n \t\t\t\tint exclude,\n \t\t\t\tint no_try_delta,\n-\t\t\t\tuint32_t index_pos,\n \t\t\t\tstruct packed_git *found_pack,\n \t\t\t\toff_t found_offset)\n {\n \tstruct object_entry *entry;\n \n-\tentry = packlist_alloc(&to_pack, oid, index_pos);\n+\tentry = packlist_alloc(&to_pack, oid);\n \tentry->hash = hash;\n \toe_set_type(entry, type);\n \tif (exclude)\n@@ -1171,11 +1169,10 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,\n {\n \tstruct packed_git *found_pack = NULL;\n \toff_t found_offset = 0;\n-\tuint32_t index_pos;\n \n \tdisplay_progress(progress_state, ++nr_seen);\n \n-\tif (have_duplicate_entry(oid, exclude, &index_pos))\n+\tif (have_duplicate_entry(oid, exclude))\n \t\treturn 0;\n \n \tif (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {\n@@ -1190,7 +1187,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,\n \n \tcreate_object_entry(oid, type, pack_name_hash(name),\n \t\t\t    exclude, name && no_try_delta(name),\n-\t\t\t    index_pos, found_pack, found_offset);\n+\t\t\t    found_pack, found_offset);\n \treturn 1;\n }\n \n@@ -1199,17 +1196,15 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,\n \t\t\t\t\tint flags, uint32_t name_hash,\n \t\t\t\t\tstruct packed_git *pack, off_t offset)\n {\n-\tuint32_t index_pos;\n-\n \tdisplay_progress(progress_state, ++nr_seen);\n \n-\tif (have_duplicate_entry(oid, 0, &index_pos))\n+\tif (have_duplicate_entry(oid, 0))\n \t\treturn 0;\n \n \tif (!want_object_in_pack(oid, 0, &pack, &offset))\n \t\treturn 0;\n \n-\tcreate_object_entry(oid, type, name_hash, 0, 0, index_pos, pack, offset);\n+\tcreate_object_entry(oid, type, name_hash, 0, 0, pack, offset);\n \treturn 1;\n }\n \n@@ -1507,7 +1502,7 @@ static int can_reuse_delta(const unsigned char *base_sha1,\n \t * First see if we're already sending the base (or it's explicitly in\n \t * our \"excluded\" list).\n \t */\n-\tbase = packlist_find(&to_pack, &base_oid, NULL);\n+\tbase = packlist_find(&to_pack, &base_oid);\n \tif (base) {\n \t\tif (!in_same_island(&delta->idx.oid, &base->idx.oid))\n \t\t\treturn 0;\n@@ -2579,7 +2574,7 @@ static void add_tag_chain(const struct object_id *oid)\n \t * it was included via bitmaps, we would not have parsed it\n \t * previously).\n \t */\n-\tif (packlist_find(&to_pack, oid, NULL))\n+\tif (packlist_find(&to_pack, oid))\n \t\treturn;\n \n \ttag = lookup_tag(the_repository, oid);\n@@ -2603,7 +2598,7 @@ static int add_ref_tag(const char *path, const struct object_id *oid, int flag,\n \n \tif (starts_with(path, \"refs/tags/\") && /* is a tag? */\n \t    !peel_ref(path, &peeled)    && /* peelable? */\n-\t    packlist_find(&to_pack, &peeled, NULL))      /* object packed? */\n+\t    packlist_find(&to_pack, &peeled))      /* object packed? */\n \t\tadd_tag_chain(oid);\n \treturn 0;\n }\n@@ -2803,7 +2798,7 @@ static void show_object(struct object *obj, const char *name, void *data)\n \t\tfor (p = strchr(name, '/'); p; p = strchr(p + 1, '/'))\n \t\t\tdepth++;\n \n-\t\tent = packlist_find(&to_pack, &obj->oid, NULL);\n+\t\tent = packlist_find(&to_pack, &obj->oid);\n \t\tif (ent && depth > oe_tree_depth(&to_pack, ent))\n \t\t\toe_set_tree_depth(&to_pack, ent, depth);\n \t}\n@@ -3034,7 +3029,7 @@ static void loosen_unused_packed_objects(void)\n \n \t\tfor (i = 0; i < p->num_objects; i++) {\n \t\t\tnth_packed_object_oid(&oid, p, i);\n-\t\t\tif (!packlist_find(&to_pack, &oid, NULL) &&\n+\t\t\tif (!packlist_find(&to_pack, &oid) &&\n \t\t\t    !has_sha1_pack_kept_or_nonlocal(&oid) &&\n \t\t\t    !loosened_object_can_be_discarded(&oid, p->mtime))\n \t\t\t\tif (force_object_loose(&oid, p->mtime))\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex fa78a460c9..a7a4964b50 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -144,7 +144,7 @@ static inline void reset_all_seen(void)\n \n static uint32_t find_object_pos(const struct object_id *oid)\n {\n-\tstruct object_entry *entry = packlist_find(writer.to_pack, oid, NULL);\n+\tstruct object_entry *entry = packlist_find(writer.to_pack, oid);\n \n \tif (!entry) {\n \t\tdie(\"Failed to write bitmap index. Packfile doesn't have full closure \"\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ed2befaac6..84cd1bed4a 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1063,7 +1063,7 @@ int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,\n \n \t\tentry = &bitmap_git->pack->revindex[i];\n \t\tnth_packed_object_oid(&oid, bitmap_git->pack, entry->nr);\n-\t\toe = packlist_find(mapping, &oid, NULL);\n+\t\toe = packlist_find(mapping, &oid);\n \n \t\tif (oe)\n \t\t\treposition[i] = oe_in_pack_pos(mapping, oe) + 1;\ndiff --git a/pack-objects.c b/pack-objects.c\nindex c1df08df1a..c6250d77f4 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -68,8 +68,7 @@ static void rehash_objects(struct packing_data *pdata)\n }\n \n struct object_entry *packlist_find(struct packing_data *pdata,\n-\t\t\t\t   const struct object_id *oid,\n-\t\t\t\t   uint32_t *index_pos)\n+\t\t\t\t   const struct object_id *oid)\n {\n \tuint32_t i;\n \tint found;\n@@ -79,9 +78,6 @@ struct object_entry *packlist_find(struct packing_data *pdata,\n \n \ti = locate_object_entry_hash(pdata, oid, &found);\n \n-\tif (index_pos)\n-\t\t*index_pos = i;\n-\n \tif (!found)\n \t\treturn NULL;\n \n@@ -153,8 +149,7 @@ void prepare_packing_data(struct repository *r, struct packing_data *pdata)\n }\n \n struct object_entry *packlist_alloc(struct packing_data *pdata,\n-\t\t\t\t    const struct object_id *oid,\n-\t\t\t\t    uint32_t index_pos)\n+\t\t\t\t    const struct object_id *oid)\n {\n \tstruct object_entry *new_entry;\n \n@@ -181,8 +176,15 @@ struct object_entry *packlist_alloc(struct packing_data *pdata,\n \n \tif (pdata->index_size * 3 <= pdata->nr_objects * 4)\n \t\trehash_objects(pdata);\n-\telse\n-\t\tpdata->index[index_pos] = pdata->nr_objects;\n+\telse {\n+\t\tint found;\n+\t\tuint32_t pos = locate_object_entry_hash(pdata,\n+\t\t\t\t\t\t\t&new_entry->idx.oid,\n+\t\t\t\t\t\t\t&found);\n+\t\tif (found)\n+\t\t\tBUG(\"duplicate object inserted into hash\");\n+\t\tpdata->index[pos] = pdata->nr_objects;\n+\t}\n \n \tif (pdata->in_pack)\n \t\tpdata->in_pack[pdata->nr_objects - 1] = NULL;\ndiff --git a/pack-objects.h b/pack-objects.h\nindex 47bf7ebf86..6fe6ae5ee8 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -183,12 +183,10 @@ static inline void packing_data_unlock(struct packing_data *pdata)\n }\n \n struct object_entry *packlist_alloc(struct packing_data *pdata,\n-\t\t\t\t    const struct object_id *oid,\n-\t\t\t\t    uint32_t index_pos);\n+\t\t\t\t    const struct object_id *oid);\n \n struct object_entry *packlist_find(struct packing_data *pdata,\n-\t\t\t\t   const struct object_id *oid,\n-\t\t\t\t   uint32_t *index_pos);\n+\t\t\t\t   const struct object_id *oid);\n \n static inline uint32_t pack_name_hash(const char *name)\n {\n-- \n2.23.0.463.g883b23b1c5\n\n"}]}