| Message ID | cover.1696021277.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Preliminary patches before git-std-lib | expand |
Hi Jonathan On 29/09/2023 22:20, Jonathan Tan wrote: > Calvin will be away for a few weeks and I'll be handling the git-std-lib > effort in the meantime. My goals will be: > > - Get the preliminary patches in Calvin's patch set (patches 1-4) merged > first. > > - Updating patches 5-6 based on reviewer feedback (including my > feedback). I have several aims including reducing or eliminating the > need for the GIT_STD_LIB preprocessor variable, and making stubs a test- > only concern (I think Phillip has some similar ideas [1] but I haven't > looked at their repo on GitHub yet). It sounds like we're thinking along similar lines, do feel free get in touch on or off the list if you want to ask anything about those patches I pushed to github. > [1] https://lore.kernel.org/git/98f3edcf-7f37-45ff-abd2-c0038d4e0589@gmail.com/ > > This patch set is in service of the first goal. Because the libification > patches are no longer included in this patch set, I have rewritten the > commit messages to justify the patches in terms of code organization. > There are no changes in the code itself. Also, I have retained Calvin's > name as the author. I agree it makes sense to get the preliminary patches merged on their own. I think the argument that they reduce the scope of includes is a reasonable justification on its own. I've left a couple of comments but they're looking pretty good. Best Wishes Phillip
phillip.wood123@gmail.com writes: > Hi Jonathan > > On 29/09/2023 22:20, Jonathan Tan wrote: > > Calvin will be away for a few weeks and I'll be handling the git-std-lib > > effort in the meantime. My goals will be: > > > > - Get the preliminary patches in Calvin's patch set (patches 1-4) merged > > first. > > > > - Updating patches 5-6 based on reviewer feedback (including my > > feedback). I have several aims including reducing or eliminating the > > need for the GIT_STD_LIB preprocessor variable, and making stubs a test- > > only concern (I think Phillip has some similar ideas [1] but I haven't > > looked at their repo on GitHub yet). > > It sounds like we're thinking along similar lines, do feel free get in > touch on or off the list if you want to ask anything about those patches > I pushed to github. Thanks. I'm updating patches 5-6 now and basing on your work, in fact. > > [1] https://lore.kernel.org/git/98f3edcf-7f37-45ff-abd2-c0038d4e0589@gmail.com/ > > > > This patch set is in service of the first goal. Because the libification > > patches are no longer included in this patch set, I have rewritten the > > commit messages to justify the patches in terms of code organization. > > There are no changes in the code itself. Also, I have retained Calvin's > > name as the author. > > I agree it makes sense to get the preliminary patches merged on their > own. I think the argument that they reduce the scope of includes is a > reasonable justification on its own. I've left a couple of comments but > they're looking pretty good. > > Best Wishes > > Phillip Thanks.
Calvin will be away for a few weeks and I'll be handling the git-std-lib effort in the meantime. My goals will be: - Get the preliminary patches in Calvin's patch set (patches 1-4) merged first. - Updating patches 5-6 based on reviewer feedback (including my feedback). I have several aims including reducing or eliminating the need for the GIT_STD_LIB preprocessor variable, and making stubs a test- only concern (I think Phillip has some similar ideas [1] but I haven't looked at their repo on GitHub yet). [1] https://lore.kernel.org/git/98f3edcf-7f37-45ff-abd2-c0038d4e0589@gmail.com/ This patch set is in service of the first goal. Because the libification patches are no longer included in this patch set, I have rewritten the commit messages to justify the patches in terms of code organization. There are no changes in the code itself. Also, I have retained Calvin's name as the author. Putting on my reviewer hat, if I was reviewing hex.h and config.h from scratch, I would have not thought twice about requesting the changes in these patches. But since we are not creating them from scratch but modifying existing files, a question does arise about whether it's worth the additional noise that someone looking through history needs to handle. In this case, I think it's worth it - I think the future code delver would appreciate being able to see the evolution of hex, hash algo, config, and parse functions as their own files rather than, when looking at one of them, having to filter out unrelated changes. Besides that, as Calvin has described in his other emails, these patches are prerequisites to being able to independently compile and use a certain subset of the .c files. With patches that solely refactor, there is sometimes a worry that the benefits are nebulous and that we would be moving code around for nothing, but I don't think that that applies here: there is still more work to be done on patches 5 and 6, but what we have in patches 5 and 6 now shows that the benefits are concrete and within reach. Calvin Wan (4): hex-ll: separate out non-hash-algo functions wrapper: reduce scope of remove_or_warn() config: correct bad boolean env value error message parse: separate out parsing functions from config.h Makefile | 2 + attr.c | 2 +- color.c | 2 +- config.c | 173 +---------------------------------- config.h | 14 +-- entry.c | 5 + entry.h | 6 ++ hex-ll.c | 49 ++++++++++ hex-ll.h | 27 ++++++ hex.c | 47 ---------- hex.h | 24 +---- mailinfo.c | 2 +- pack-objects.c | 2 +- pack-revindex.c | 2 +- parse-options.c | 3 +- parse.c | 182 +++++++++++++++++++++++++++++++++++++ parse.h | 20 ++++ pathspec.c | 2 +- preload-index.c | 2 +- progress.c | 2 +- prompt.c | 2 +- rebase.c | 2 +- strbuf.c | 2 +- t/helper/test-env-helper.c | 2 +- unpack-trees.c | 2 +- url.c | 2 +- urlmatch.c | 2 +- wrapper.c | 8 +- wrapper.h | 5 - write-or-die.c | 2 +- 30 files changed, 313 insertions(+), 284 deletions(-) create mode 100644 hex-ll.c create mode 100644 hex-ll.h create mode 100644 parse.c create mode 100644 parse.h Range-diff against v3: 1: fcce01bc19 ! 1: 02ecc00e9c hex-ll: split out functionality from hex @@ Metadata Author: Calvin Wan <calvinwan@google.com> ## Commit message ## - hex-ll: split out functionality from hex + hex-ll: separate out non-hash-algo functions - Separate out hex functionality that doesn't require a hash algo into - hex-ll.[ch]. Since the hash algo is currently a global that sits in - repository, this separation removes that dependency for files that only - need basic hex manipulation functions. + In order to further reduce all-in-one headers, separate out functions in + hex.h that do not operate on object hashes into its own file, hex-ll.h, + and update the include directives in the .c files that need only such + functions accordingly. Signed-off-by: Calvin Wan <calvinwan@google.com> - Signed-off-by: Junio C Hamano <gitster@pobox.com> + Signed-off-by: Jonathan Tan <jonathantanmy@google.com> ## Makefile ## @@ Makefile: LIB_OBJS += hash-lookup.o 2: 95a369d02b ! 2: c9e7cd7857 wrapper: remove dependency to Git-specific internal file @@ Metadata Author: Calvin Wan <calvinwan@google.com> ## Commit message ## - wrapper: remove dependency to Git-specific internal file + wrapper: reduce scope of remove_or_warn() - In order for wrapper.c to be built independently as part of a smaller - library, it cannot have dependencies to other Git specific - internals. remove_or_warn() creates an unnecessary dependency to - object.h in wrapper.c. Therefore move the function to entry.[ch] which - performs changes on the worktree based on the Git-specific file modes in - the index. + remove_or_warn() is only used by entry.c and apply.c, but it is + currently declared and defined in wrapper.{h,c}, so it has a scope much + greater than it needs. This needlessly large scope also causes wrapper.c + to need to include object.h, when this file is largely unconcerned with + Git objects. + + Move remove_or_warn() to entry.{h,c}. The file apply.c still has access + to it, since it already includes entry.h for another reason. Signed-off-by: Calvin Wan <calvinwan@google.com> - Signed-off-by: Junio C Hamano <gitster@pobox.com> + Signed-off-by: Jonathan Tan <jonathantanmy@google.com> ## entry.c ## @@ entry.c: void unlink_entry(const struct cache_entry *ce, const char *super_prefix) 3: 5348528865 = 3: e4c20a81f9 config: correct bad boolean env value error message 4: b5a8945c5c ! 4: 5d9f0b3de0 parse: create new library for parsing strings and env values @@ Metadata Author: Calvin Wan <calvinwan@google.com> ## Commit message ## - parse: create new library for parsing strings and env values + parse: separate out parsing functions from config.h - While string and environment value parsing is mainly consumed by - config.c, there are other files that only need parsing functionality and - not config functionality. By separating out string and environment value - parsing from config, those files can instead be dependent on parse, - which has a much smaller dependency chain than config. This ultimately - allows us to inclue parse.[ch] in an independent library since it - doesn't have dependencies to Git-specific internals unlike in - config.[ch]. + The files config.{h,c} contain functions that have to do with parsing, + but not config. - Move general string and env parsing functions from config.[ch] to - parse.[ch]. + In order to further reduce all-in-one headers, separate out functions in + config.c that do not operate on config into its own file, parse.h, + and update the include directives in the .c files that need only such + functions accordingly. Signed-off-by: Calvin Wan <calvinwan@google.com> - Signed-off-by: Junio C Hamano <gitster@pobox.com> + Signed-off-by: Jonathan Tan <jonathantanmy@google.com> ## Makefile ## @@ Makefile: LIB_OBJS += pack-write.o