| Message ID | 20190107083150.GC21362@sigill.intra.peff.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | jk/loose-object-cache sha1/object_id fixups | expand |
Am 07.01.2019 um 09:31 schrieb Jeff King: > I also cleaned up my sha1/object_id patch and rebased it on top of what > you have here. Though as I worked on it, it expanded in scope a bit. > Possibly it should be a separate series entirely, but that would create > some annoying textual conflicts on merge. > > [01/11]: sha1-file: fix outdated sha1 comment references > [02/11]: update comment references to sha1_object_info() > [03/11]: http: use struct object_id instead of bare sha1 > [04/11]: sha1-file: modernize loose object file functions > [05/11]: sha1-file: modernize loose header/stream functions > [06/11]: sha1-file: convert pass-through functions to object_id > [07/11]: convert has_sha1_file() callers to has_object_file() > [08/11]: sha1-file: drop has_sha1_file() > [09/11]: sha1-file: prefer "loose object file" to "sha1 file" in messages > [10/11]: sha1-file: avoid "sha1 file" for generic use in messages > [11/11]: prefer "hash mismatch" to "sha1 mismatch" I skimmed them; they look good to me. 6 and 8 are particularly satisfying; getting rid of hash copy operations just feels nice. :) Junio only took 1 to 5 into pu; 6, 7 and its sidekick 8, 10 and 11 conflict with sb/more-repo-in-api; 9 could go in unmodified. René
René Scharfe <l.s.r@web.de> writes: > Am 07.01.2019 um 09:31 schrieb Jeff King: >> I also cleaned up my sha1/object_id patch and rebased it on top of what >> you have here. Though as I worked on it, it expanded in scope a bit. >> Possibly it should be a separate series entirely, but that would create >> some annoying textual conflicts on merge. >> >> [01/11]: sha1-file: fix outdated sha1 comment references >> [02/11]: update comment references to sha1_object_info() >> [03/11]: http: use struct object_id instead of bare sha1 >> [04/11]: sha1-file: modernize loose object file functions >> [05/11]: sha1-file: modernize loose header/stream functions >> [06/11]: sha1-file: convert pass-through functions to object_id >> [07/11]: convert has_sha1_file() callers to has_object_file() >> [08/11]: sha1-file: drop has_sha1_file() >> [09/11]: sha1-file: prefer "loose object file" to "sha1 file" in messages >> [10/11]: sha1-file: avoid "sha1 file" for generic use in messages >> [11/11]: prefer "hash mismatch" to "sha1 mismatch" > > I skimmed them; they look good to me. 6 and 8 are particularly > satisfying; getting rid of hash copy operations just feels nice. :) > > Junio only took 1 to 5 into pu; 6, 7 and its sidekick 8, 10 and 11 > conflict with sb/more-repo-in-api; 9 could go in unmodified. I think these later steps are based on something a lot newer than the result of applying your updates to the jk/loose-object-cache series they fix. I think I untangled and backported one of the earlier commits but then I stopped after 05/11. I do not think it is important to keep jk/loose-object-cache and these two follow-up topics mergeable to the maintenance track, so I'll see if the patches behave better if queued directly on top of 3b2f8a02 ("Merge branch 'jk/loose-object-cache'", 2019-01-04), or even a yet newer random point on 'master'. Thanks.
On Tue, Jan 08, 2019 at 09:39:48AM -0800, Junio C Hamano wrote: > > I skimmed them; they look good to me. 6 and 8 are particularly > > satisfying; getting rid of hash copy operations just feels nice. :) > > > > Junio only took 1 to 5 into pu; 6, 7 and its sidekick 8, 10 and 11 > > conflict with sb/more-repo-in-api; 9 could go in unmodified. > > I think these later steps are based on something a lot newer than > the result of applying your updates to the jk/loose-object-cache > series they fix. I think I untangled and backported one of the > earlier commits but then I stopped after 05/11. Sorry, I applied René's patches on top of master and then built on that, treating it like a new topic. But of course your jk/loose-object-cache is older. > I do not think it is important to keep jk/loose-object-cache and > these two follow-up topics mergeable to the maintenance track, so > I'll see if the patches behave better if queued directly on top of > 3b2f8a02 ("Merge branch 'jk/loose-object-cache'", 2019-01-04), or > even a yet newer random point on 'master'. Yeah, they should. I think one of them will need René's patch, which changes the body of quick_has_loose(). I can roll it as a separate topic if that's easier (or just wait a week or so until René's cleanups graduate). -Peff
Jeff King <peff@peff.net> writes: > Yeah, they should. I think one of them will need René's patch, which > changes the body of quick_has_loose(). I can roll it as a separate topic > if that's easier (or just wait a week or so until René's cleanups > graduate). Nah, what I got is already good to work with. Both series are straight-forward and I do not expect them needing long fermentation.
On 1/8/2019 1:07 PM, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > >> Yeah, they should. I think one of them will need René's patch, which >> changes the body of quick_has_loose(). I can roll it as a separate topic >> if that's easier (or just wait a week or so until René's cleanups >> graduate). > Nah, what I got is already good to work with. Both series are > straight-forward and I do not expect them needing long fermentation. I'm just chiming in to say that this series was a very satisfying read, and the changes were clear-cut and mechanical. Thanks! -Stolee
Junio C Hamano <gitster@pobox.com> writes: > Jeff King <peff@peff.net> writes: > >> Yeah, they should. I think one of them will need René's patch, which >> changes the body of quick_has_loose(). I can roll it as a separate topic >> if that's easier (or just wait a week or so until René's cleanups >> graduate). > > Nah, what I got is already good to work with. Both series are > straight-forward and I do not expect them needing long fermentation. Yikes, the conflicts with sb/more-repo-in-api is quite irritating. I think I'll postpone the later parts of this series and ask this to be sent after sb/more-repo-in-api matures a bit mroe.
On Tue, Jan 08, 2019 at 10:52:19AM -0800, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Jeff King <peff@peff.net> writes: > > > >> Yeah, they should. I think one of them will need René's patch, which > >> changes the body of quick_has_loose(). I can roll it as a separate topic > >> if that's easier (or just wait a week or so until René's cleanups > >> graduate). > > > > Nah, what I got is already good to work with. Both series are > > straight-forward and I do not expect them needing long fermentation. > > Yikes, the conflicts with sb/more-repo-in-api is quite irritating. > I think I'll postpone the later parts of this series and ask this to > be sent after sb/more-repo-in-api matures a bit mroe. There were several conflicts, but it was mostly just tedious textual fixups. I pushed the result to: https://github.com/peff/git.git resolve-oid-more-repo But I'm happy to wait and rebase if sb/more-repo-in-api is close to graduating. -Peff
> > Yikes, the conflicts with sb/more-repo-in-api is quite irritating. > > I think I'll postpone the later parts of this series and ask this to > > be sent after sb/more-repo-in-api matures a bit mroe. > > There were several conflicts, but it was mostly just tedious textual > fixups. I pushed the result to: > > https://github.com/peff/git.git resolve-oid-more-repo > > But I'm happy to wait and rebase if sb/more-repo-in-api is close to > graduating. The merge looks good to me, though I just looked quickly. The series itself is also a pleasant read. Stefan
On Wed, Jan 9, 2019 at 1:37 PM Stefan Beller <sbeller@google.com> wrote: > > > > Yikes, the conflicts with sb/more-repo-in-api is quite irritating. > > > I think I'll postpone the later parts of this series and ask this to > > > be sent after sb/more-repo-in-api matures a bit mroe. > > > > There were several conflicts, but it was mostly just tedious textual > > fixups. I pushed the result to: > > > > https://github.com/peff/git.git resolve-oid-more-repo > > > > But I'm happy to wait and rebase if sb/more-repo-in-api is close to > > graduating. > > The merge looks good to me, though I just looked quickly. > The series itself is also a pleasant read. Compiling this leads to: sha1-file.c:1424:33: error: incompatible pointer types passing 'const struct object_id *' to parameter of type 'const unsigned char *' [-Werror,-Wincompatible-pointer-types] if ((p = has_packed_and_bad(r, repl)) != NULL) ^~~~ ./packfile.h:149:95: note: passing argument to parameter 'sha1' here extern const struct packed_git *has_packed_and_bad(struct repository *r, const unsigned char *sha1);
On Wed, Jan 09, 2019 at 02:42:28PM -0800, Stefan Beller wrote: > On Wed, Jan 9, 2019 at 1:37 PM Stefan Beller <sbeller@google.com> wrote: > > > > > > Yikes, the conflicts with sb/more-repo-in-api is quite irritating. > > > > I think I'll postpone the later parts of this series and ask this to > > > > be sent after sb/more-repo-in-api matures a bit mroe. > > > > > > There were several conflicts, but it was mostly just tedious textual > > > fixups. I pushed the result to: > > > > > > https://github.com/peff/git.git resolve-oid-more-repo > > > > > > But I'm happy to wait and rebase if sb/more-repo-in-api is close to > > > graduating. > > > > The merge looks good to me, though I just looked quickly. > > The series itself is also a pleasant read. > > Compiling this leads to: > > sha1-file.c:1424:33: error: incompatible pointer types passing 'const > struct object_id *' to parameter of type 'const unsigned char *' > [-Werror,-Wincompatible-pointer-types] > if ((p = has_packed_and_bad(r, repl)) != NULL) > ^~~~ > ./packfile.h:149:95: note: passing argument to parameter 'sha1' here > extern const struct packed_git *has_packed_and_bad(struct repository > *r, const unsigned char *sha1); Eek, sorry about that. I did the merge on a detached HEAD, and my config.mak relaxes compilation warnings in that case (since I am often sight-seeing to old versions that have warnings which have since been fixed). And the result passes the tests since "repl" and "repl->hash" are effectively the same pointer. I've pushed up the fix (s/repl/repl->hash/). Thanks for noticing. -Peff