From: Shawn O. Pearce Date: Sat, 17 Apr 2010 19:30:10 GMT Subject: Re: [PATCH v2 5/6] http-fetch: Use index-pack rather than verify-pack to check packs Message-ID: <20100417193010.GB15911@spearce.org> In-Reply-To: Tay Ray Chuan wrote: > > @@ -152,7 +152,6 @@ struct http_pack_request > >        struct packed_git *target; > >        struct packed_git **lst; > >        FILE *packfile; > > -       char filename[PATH_MAX]; > > Why this change? Just curious, nothing strong against it. Split into a new patch. I'll share my reasons in the commit message. :-) > > +       tmp_idx = xstrdup(preq->tmpfile); > > +       strcpy(tmp_idx + strlen(tmp_idx) - strlen(".pack.temp"), > > +              ".idx.temp"); > > Could we use a strbuf here? Doesn't seem worth it. I just started trying to rework this with a strbuf and I just don't see any benefit here. We know the tmpfile ends with ".pack.temp" when we created this request structure. So a strdup and overwrite of the tail just works. > > +       if (move_temp_to_file(preq->tmpfile, sha1_pack_name(p->sha1)) > > +        || move_temp_to_file(tmp_idx, sha1_pack_index_name(p->sha1))) { > > Hmm, when moving the pack index file, should we unlink() the old, > downloaded one first? Yup, good point, thanks. -- Shawn.