| Message ID | 52958c3557c34992df59e9c10f098f457526702c.1694240177.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | d2be104085b24867e3dd9cb061c75b456071b351 |
| Headers | show |
| Series | Trailer readability cleanups | expand |
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Linus Arver <linusa@google.com> > > Do not use *_DEFAULT as a suffix to the enums, because the word > "default" is overloaded. The following are two examples of the ambiguity > of the word "default": In this case these are left unspecified to use the default; while it is not wrong per-se to say *_DEFAULT, using *_UNSPECIFIED makes it more obvious. > 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). It gets tempting to initialize a variable to the default and arrange the rest of the system so that the variable set to the default triggers the default activity. Such an obvious solution however cannot be used when (1) being left unspecified to use the default value and (2) explicitly set by the user to a value that happens to be the same as the default have to behave differently. I am not sure if that applies to the trailers system, though. Thanks. PS. Glen's old e-mail address is no longer valid and there is no forwarding done by @google.com mailservers, it seems. Can you tell GGG to drop the address (optionally replace it with his new address)?
Junio C Hamano <gitster@pobox.com> writes: > "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Linus Arver <linusa@google.com> >> >> [...] >> 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). > > It gets tempting to initialize a variable to the default and arrange > the rest of the system so that the variable set to the default > triggers the default activity. Such an obvious solution however > cannot be used when (1) being left unspecified to use the default > value and (2) explicitly set by the user to a value that happens to > be the same as the default have to behave differently. I am not > sure if that applies to the trailers system, though. > > Thanks. I get the feeling that you wrote the "Such an obvious ... differently" sentence after writing the last sentence in that paragraph, because when you say I am not sure if that applies to the trailers system, though. I read the "that" (emphasis added) in there as referring to the solution described in the first sentence, and not the conditions (1) and (2) you enumerated. IOW, you are OK with this patch. Am I parsing your paragraph correctly? > PS. Glen's old e-mail address is no longer valid and there is no > forwarding done by @google.com mailservers, it seems. Can you tell > GGG to drop the address (optionally replace it with his new address)? Thanks for catching this. I've updated the GGG PR [1] to use Glen's new address. [1] https://github.com/gitgitgadget/git/pull/1563#issue-1837440424
Linus Arver <linusa@google.com> writes: >> It gets tempting to initialize a variable to the default and arrange >> the rest of the system so that the variable set to the default >> triggers the default activity. Such an obvious solution however >> cannot be used when (1) being left unspecified to use the default >> value and (2) explicitly set by the user to a value that happens to >> be the same as the default have to behave differently. I am not >> sure if that applies to the trailers system, though. >> >> Thanks. > > I get the feeling that you wrote the "Such an obvious ... differently" > sentence after writing the last sentence in that paragraph, because when > you say > > I am not > sure if that applies to the trailers system, though. > > I read the "that" (emphasis added) in there as referring to the solution > described in the first sentence, and not the conditions (1) and (2) you > enumerated. IOW, you are OK with this patch. "that" refers to "the reason not to use such an obvious solution". I do not know if trailer subsystem wants to treat "left unspecified" and "set to the value that happens to be the same as the default" in a different way. If it does want to do so, then I do not see a strong reason not to use the "obvious solution". Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Linus Arver <linusa@google.com> writes: > >>> It gets tempting to initialize a variable to the default and arrange >>> the rest of the system so that the variable set to the default >>> triggers the default activity. Such an obvious solution however >>> cannot be used when (1) being left unspecified to use the default >>> value and (2) explicitly set by the user to a value that happens to >>> be the same as the default have to behave differently. I am not >>> sure if that applies to the trailers system, though. >>> >>> Thanks. >> >> I get the feeling that you wrote the "Such an obvious ... differently" >> sentence after writing the last sentence in that paragraph, because when >> you say >> >> I am not >> sure if that applies to the trailers system, though. >> >> I read the "that" (emphasis added) in there as referring to the solution >> described in the first sentence, and not the conditions (1) and (2) you >> enumerated. IOW, you are OK with this patch. > > "that" refers to "the reason not to use such an obvious solution". > I do not know if trailer subsystem wants to treat "left unspecified" > and "set to the value that happens to be the same as the default" in > a different way. If it does want to do so, then I do not see a > strong reason not to use the "obvious solution". Currently we set the defaults (these take effect absent any configuration or CLI options) in trailer.c like this: static void ensure_configured(void) { if (configured) return; /* Default config must be setup first */ default_conf_info.where = WHERE_END; default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR; default_conf_info.if_missing = MISSING_ADD; git_config(git_trailer_default_config, NULL); git_config(git_trailer_config, NULL); configured = 1; } So technically we already sort of do the "obvious solution". And then these get overriden by configuration (if any), and finally any CLI options that are passed in (e.g., "--where after"). The reason why I prefer the *_UNSPECIFIED style in this patch for these enums though is because of this (and other similar functions) in trailer.c: int trailer_set_where(enum trailer_where *item, const char *value) { if (!value) *item = WHERE_DEFAULT; else if (!strcasecmp("after", value)) *item = WHERE_AFTER; else if (!strcasecmp("before", value)) *item = WHERE_BEFORE; else if (!strcasecmp("end", value)) *item = WHERE_END; else if (!strcasecmp("start", value)) *item = WHERE_START; else return -1; return 0; } and this function is used as a callback to the "--where" flag, such that the WHERE_DEFAULT gets chosen if "--no-where" is where. I prefer the WHERE_UNSPECIFIED as in this patch because the WHERE_DEFAULT is ambiguous on its own (i.e., WHERE_DEFAULT could mean that we either use the default value WHERE_END in default_conf_info, or it could mean that we fall back to the configuration variables (where it could be something else)).
Linus Arver <linusa@google.com> writes: > ... I prefer the > WHERE_UNSPECIFIED as in this patch because the WHERE_DEFAULT is > ambiguous on its own (i.e., WHERE_DEFAULT could mean that we either use > the default value WHERE_END in default_conf_info, or it could mean that > we fall back to the configuration variables (where it could be something > else)). Yup. "Turning something that is left UNSPECIFIED after command line options and configuration files are processed into the hardcoded DEFAULT" is one mental model that is easy to explain. I however am not sure if it is easier than "Setting something to hardcoded DEFAULT before command line options and configuration files are allowed to tweak it, and if nobody touches it, then it gets the hardcoded DEFAULT value in the end", which is another valid mental model, though. If both can be used, I'd personally prefer the latter, and reserve the "UNSPECIFIED to DEFAULT" pattern to signal that we are dealing with a case where the simpler pattern without UNSPECIFIED cannot solve. The simpler pattern would not work, when the default is defined depending on a larger context. Imagine we have two Boolean variables, A and B, where A defaults to false, and B defaults to some value derived from the value of A (say, opposite of A). In the most natural implementation, you'd initialize A to false and B to unspecified, let command line options and configuration variables to set them to true or false, and after all that, you do not have to tweak A's value (it will be left to false that is the default unless the user or the configuration gave an explicit value), but you need to check if B is left unspecified and tweak it to true or false using the final value of A. For a variable with such a need like B, we cannot avoid having "unspecified". If you initialize it to false (or true), after the command line and the configuration files are read and you find B is set to false (or true), you cannot tell if the user or the configuration explicitly set B to false (or true), in which case you do not want to futz with its value based on what is in A, or it is false (or true) only because nobody touched it, in which case you need to compute its value based on what is in A. And that is why I asked if we need to special case "the user did not touch and the variable is left untouched" in the trailer subsystem. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Linus Arver <linusa@google.com> writes: > >> ... I prefer the >> WHERE_UNSPECIFIED as in this patch because the WHERE_DEFAULT is >> ambiguous on its own (i.e., WHERE_DEFAULT could mean that we either use >> the default value WHERE_END in default_conf_info, or it could mean that >> we fall back to the configuration variables (where it could be something >> else)). > > Yup. "Turning something that is left UNSPECIFIED after command line > options and configuration files are processed into the hardcoded > DEFAULT" is one mental model that is easy to explain. > > I however am not sure if it is easier than "Setting something to > hardcoded DEFAULT before command line options and configuration > files are allowed to tweak it, and if nobody touches it, then it > gets the hardcoded DEFAULT value in the end", which is another valid > mental model, though. True. > If both can be used, I'd personally prefer > the latter, and reserve the "UNSPECIFIED to DEFAULT" pattern to > signal that we are dealing with a case where the simpler pattern > without UNSPECIFIED cannot solve. SGTM. > The simpler pattern would not work, when the default is defined > depending on a larger context. Imagine we have two Boolean > variables, A and B, where A defaults to false, and B defaults to > some value derived from the value of A (say, opposite of A). > > In the most natural implementation, you'd initialize A to false and > B to unspecified, let command line options and configuration > variables to set them to true or false, and after all that, you do > not have to tweak A's value (it will be left to false that is the > default unless the user or the configuration gave an explicit > value), but you need to check if B is left unspecified and tweak it > to true or false using the final value of A. > > For a variable with such a need like B, we cannot avoid having > "unspecified". If you initialize it to false (or true), after the > command line and the configuration files are read and you find B is > set to false (or true), you cannot tell if the user or the > configuration explicitly set B to false (or true), in which case you > do not want to futz with its value based on what is in A, or it is > false (or true) only because nobody touched it, in which case you > need to compute its value based on what is in A. Thanks for the illustrative example! I don't think we have a case of a "B" variable here for trailers. > And that is why I asked if we need to special case "the user did not > touch and the variable is left untouched" in the trailer subsystem. I think the answer is "no, we don't need to special case". I'll be dropping this patch in the next re-roll. > Thanks.
diff --git a/trailer.c b/trailer.c index f646e484a23..6ad2fbca942 100644 --- a/trailer.c +++ b/trailer.c @@ -388,7 +388,7 @@ static void process_trailers_lists(struct list_head *head, int trailer_set_where(enum trailer_where *item, const char *value) { if (!value) - *item = WHERE_DEFAULT; + *item = WHERE_UNSPECIFIED; else if (!strcasecmp("after", value)) *item = WHERE_AFTER; else if (!strcasecmp("before", value)) @@ -405,7 +405,7 @@ int trailer_set_where(enum trailer_where *item, const char *value) int trailer_set_if_exists(enum trailer_if_exists *item, const char *value) { if (!value) - *item = EXISTS_DEFAULT; + *item = EXISTS_UNSPECIFIED; else if (!strcasecmp("addIfDifferent", value)) *item = EXISTS_ADD_IF_DIFFERENT; else if (!strcasecmp("addIfDifferentNeighbor", value)) @@ -424,7 +424,7 @@ int trailer_set_if_exists(enum trailer_if_exists *item, const char *value) int trailer_set_if_missing(enum trailer_if_missing *item, const char *value) { if (!value) - *item = MISSING_DEFAULT; + *item = MISSING_UNSPECIFIED; else if (!strcasecmp("doNothing", value)) *item = MISSING_DO_NOTHING; else if (!strcasecmp("add", value)) @@ -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; @@ -701,11 +704,11 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val, new_item->value = val; duplicate_conf(&new_item->conf, conf); if (new_trailer_item) { - if (new_trailer_item->where != WHERE_DEFAULT) + if (new_trailer_item->where != WHERE_UNSPECIFIED) new_item->conf.where = new_trailer_item->where; - if (new_trailer_item->if_exists != EXISTS_DEFAULT) + if (new_trailer_item->if_exists != EXISTS_UNSPECIFIED) new_item->conf.if_exists = new_trailer_item->if_exists; - if (new_trailer_item->if_missing != MISSING_DEFAULT) + if (new_trailer_item->if_missing != MISSING_UNSPECIFIED) new_item->conf.if_missing = new_trailer_item->if_missing; } list_add_tail(&new_item->list, arg_head); diff --git a/trailer.h b/trailer.h index ab2cd017567..a689d768c79 100644 --- a/trailer.h +++ b/trailer.h @@ -5,14 +5,14 @@ #include "strbuf.h" enum trailer_where { - WHERE_DEFAULT, + WHERE_UNSPECIFIED, WHERE_END, WHERE_AFTER, WHERE_BEFORE, WHERE_START }; enum trailer_if_exists { - EXISTS_DEFAULT, + EXISTS_UNSPECIFIED, EXISTS_ADD_IF_DIFFERENT_NEIGHBOR, EXISTS_ADD_IF_DIFFERENT, EXISTS_ADD, @@ -20,7 +20,7 @@ enum trailer_if_exists { EXISTS_DO_NOTHING }; enum trailer_if_missing { - MISSING_DEFAULT, + MISSING_UNSPECIFIED, MISSING_ADD, MISSING_DO_NOTHING };