Re: [PATCH 5/5] trailer: rename *_DEFAULT enums to *_UNSPECIFIED
- From
- Linus Arver <linusa@google.com>
- Date
- Aug 11, 2023, 18:00 UTC
- Message-ID
- <owlycyztfohb.fsf@fine.c.googlers.com>
- In-Reply-To
- <kl6lv8dq8li0.fsf@chooglen-macbookpro.roam.corp.google.com>
Glen Choo <chooglen@google.com> writes:
Show 15 quoted lines
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> (2) "Default" can also mean the "trailer.*" configurations themselves, >> because these configurations are used by "default" (ahead of the >> hardcoded defaults in (1)) if no command line arguments are >> provided. > > Interesting, I would have never thought of config as 'default'. In fact, > I would have thought that this de facto behavior (which you also > clarified in [1]) is a bug if not for the fact that in an internal > version of this series, you cited a commit message that describes this > as expected behavior. That context would be very welcome in the ML, I > think. > > [1] https://lore.kernel.org/git/6b427b4b1e82b1f01640f1f49fe8d1c2fd02111e.1691210737.git.gitgitgadget@gmail.com
I forget the internal version/discussion, but I assume you're thinking of 0ea5292e6b (interpret-trailers: add options for actions, 2017-08-01). I will reroll and mention it to the commit message.
Show 32 quoted lines
>> In addition, the corresponding *_DEFAULT values are chosen when the user >> provides the "--no-where", "--no-if-exists", or "--no-if-missing" flags >> on the command line. These "--no-*" flags are used to clear previously >> provided flags of the form "--where", "--if-exists", and "--if-missing". >> Using these "--no-*" flags undoes the specifying of these flags (if >> any), so using the word "UNSPECIFIED" is more natural here. >> >> So instead of using "*_DEFAULT", use "*_UNSPECIFIED" because this >> signals to the reader that the *_UNSPECIFIED value by itself carries no >> meaning (it's a zero value and by itself does not "default" to anything, >> necessitating the need to have some other way of getting to a useful >> value). > > Makse sense. This seems like a good change. > >> @@ -586,7 +586,10 @@ static void ensure_configured(void) >> if (configured) >> return; >> >> - /* Default config must be setup first */ >> + /* >> + * Default config must be setup first. These defaults are used if there >> + * are no "trailer.*" or "trailer.<token>.*" options configured. >> + */ >> default_conf_info.where = WHERE_END; >> default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR; >> default_conf_info.if_missing = MISSING_ADD; > > As mentioned earlier, I find it a bit odd that we're calling config > 'default' (and also that we're calling CLI args config), but > renaming default_conf_info to config_conf_info sounds worse, so let's > leave it as-is.
SGTM. Although, we could also just rename it to "conf_info" (same name as the struct name). Unless such same-variable-name-as-the-struct is discouraged in the codebase.