Glen Choo <chooglen@xxxxxxxxxx> writes: > "Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxx 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. >> 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.
