On 7/1/21 6:36 PM, Ævar Arnfjörð Bjarmason wrote:
On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote: A general comment on this series (including previous patches). We've usually tried to bend over backwards in git's codebase not to have big ifdef blocks, so we compile most code the same everywhere. We waste a bit of object code, but that's fine. See 9c897c5c2ad (pack-objects: remove #ifdef NO_PTHREADS, 2018-11-03) for a good exmaple of bad code being turned to good. E.g. in this case:+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND + +int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix) +{ + const char *subcmd; + + struct option options[] = { + OPT_END() + }; + + if (argc < 2) + usage_with_options(builtin_fsmonitor__daemon_usage, options); + + if (argc == 2 && !strcmp(argv[1], "-h")) + usage_with_options(builtin_fsmonitor__daemon_usage, options); + + git_config(git_default_config, NULL); + + subcmd = argv[1]; + argv--; + argc++; + + argc = parse_options(argc, argv, prefix, options, + builtin_fsmonitor__daemon_usage, 0); + + die(_("Unhandled subcommand '%s'"), subcmd); +} + +#else +int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix) +{ + struct option options[] = { + OPT_END() + }; + + if (argc == 2 && !strcmp(argv[1], "-h")) + usage_with_options(builtin_fsmonitor__daemon_usage, options); + + die(_("fsmonitor--daemon not supported on this platform")); +} +#endifThis whole thing could really just be a -DHAVE_FSMONITOR_DAEMON_BACKEND=1 or -DHAVE_FSMONITOR_DAEMON_BACKEND=0 somewhere (depending), and then somewhere in the middle of the first function: if (!HAVE_FSMONITOR_DAEMON_BACKEND) die(_("fsmonitor--daemon not supported on this platform"));
This whole file will be filled up with ~1500 lines of static functions that only make sense when the daemon is supported and that make calls to platform-specific backends. I suppose we could stub in an empty backend (something like that in 11/34 and 12/34) and hack in all stuff in the makefile to link to it in the unsupported case, but that seems like a lot of effort just to avoid an ifdef here. I mean, the intent of the #else block is quite clear and we're not fooling the reader with a large source file of code that will never be used on their platform. We could consider splitting this source file into a supported and unsupported version and have the makefile select the right .c file. We'd have to move the usage and stuff to a shared header and etc. That would eliminate the ifdef, but it would break the convention of the source filename matching the command name. I'm not sure it's worth the bother TBH. Jeff
