Re: [PATCH v2 0/5] getting rid of most "static struct lock_file"s
On 10 May 2018 at 08:01, Junio C Hamano wrote: > Jeff King writes: > >> I don't think it's worth re-rolling, but one thing to think about for >> future cleanups: you split the patches by touched area, not by >> functionality. So the first three patches have a "while we're here..." >> that has to explain why dropping the "static" is the right thing over >> and over. If you instead did the error-handling fixes independently >> first, then you could lump the "static" cleanups together with one >> explanation (possibly even just as part of the 4th patch). > > Thanks Peff for a good pice of advice. I agree with the assessment > after reading the series through (includng "not worth rerolling" > part). Right. In the first version, the while-at-its were really while-at-its -- and it turned out it needed some motivation. So, in the reroll, I focused on expanding the commit messages. Any benefit from making patches four and five somewhat smaller definitely got lost in the blown up first three commit messages. Thanks for pointing it out. Martin
Re: [PATCH v2 0/5] getting rid of most "static struct lock_file"s
Jeff King writes: > I don't think it's worth re-rolling, but one thing to think about for > future cleanups: you split the patches by touched area, not by > functionality. So the first three patches have a "while we're here..." > that has to explain why dropping the "static" is the right thing over > and over. If you instead did the error-handling fixes independently > first, then you could lump the "static" cleanups together with one > explanation (possibly even just as part of the 4th patch). Thanks Peff for a good pice of advice. I agree with the assessment after reading the series through (includng "not worth rerolling" part). Thanks, Martin.
Re: [PATCH v2 0/5] getting rid of most "static struct lock_file"s
On Wed, May 09, 2018 at 10:55:34PM +0200, Martin Ågren wrote: > This is take two of my attempt at making almost all `struct lock_file`s > non-static. All patches have been equipped with more detailed commit > messages. The only diff that has changed is patch 3/5, where I now take > a small step towards gentle error-handling, rather than going in the > opposite direction. > > Thanks all for the valuable feedback on v1. I could have saved everyone > some trouble by writing better commit messages from the start, and > probably also by using `--thread` when formatting the patches... Indeed, the world would be a more efficient place if we all did everything perfectly all the time. Sadly, that's not how it works. :) This version looks good to me. Thanks for modernizing things. I don't think it's worth re-rolling, but one thing to think about for future cleanups: you split the patches by touched area, not by functionality. So the first three patches have a "while we're here..." that has to explain why dropping the "static" is the right thing over and over. If you instead did the error-handling fixes independently first, then you could lump the "static" cleanups together with one explanation (possibly even just as part of the 4th patch). -Peff
[PATCH v2 0/5] getting rid of most "static struct lock_file"s
This is take two of my attempt at making almost all `struct lock_file`s non-static. All patches have been equipped with more detailed commit messages. The only diff that has changed is patch 3/5, where I now take a small step towards gentle error-handling, rather than going in the opposite direction. Thanks all for the valuable feedback on v1. I could have saved everyone some trouble by writing better commit messages from the start, and probably also by using `--thread` when formatting the patches... Martin Martin Ågren (5): t/helper/test-write-cache: clean up lock-handling refs.c: do not die if locking fails in `write_pseudoref()` refs.c: do not die if locking fails in `delete_pseudoref()` lock_file: make function-local locks non-static lock_file: move static locks into functions t/helper/test-scrap-cache-tree.c | 4 ++-- t/helper/test-write-cache.c | 14 +- apply.c | 2 +- builtin/add.c| 3 +-- builtin/describe.c | 2 +- builtin/difftool.c | 2 +- builtin/gc.c | 2 +- builtin/merge.c | 4 ++-- builtin/mv.c | 2 +- builtin/read-tree.c | 3 +-- builtin/receive-pack.c | 2 +- builtin/rm.c | 3 +-- bundle.c | 2 +- fast-import.c| 2 +- refs.c | 16 +--- refs/files-backend.c | 2 +- rerere.c | 3 +-- shallow.c| 2 +- 18 files changed, 32 insertions(+), 38 deletions(-) -- 2.17.0.411.g9fd64c8e46
