| Message ID | b120972a441d3081519af0e31bb0c639df148287.1647033303.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Builtin FSMonitor Part 2.5 | expand |
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Jeff Hostetler <jeffhost@microsoft.com> > > fixup! update-index: convert fsmonitor warnings to advise Same comment as 01/16 applies here. "convert ... back to ..." in the title refers to the fact that builtin-fsmonitor-part2 topic turned warning() into advise() without a good justification, I think. Flipping and flopping between warning and advise, without giving any justification going either direction, is not a good move. I only have looked at one eighth of this part 2.5, but it starts to look that ejecting part-2 and redoing it may result in a cleaner code that is easier to understand, perhaps? For example, instead of applying this patch, we can just get rid of 1a9241e1 (update-index: convert fsmonitor warnings to advise, 2022-03-01). As I read more, my impression will certainly change, I would expect. Let's see. > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> > --- > builtin/update-index.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/builtin/update-index.c b/builtin/update-index.c > index d335f1ac72a..75d646377cc 100644 > --- a/builtin/update-index.c > +++ b/builtin/update-index.c > @@ -1238,18 +1238,18 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) > if (fsmonitor > 0) { > enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r); > if (fsm_mode == FSMONITOR_MODE_DISABLED) { > - advise(_("core.fsmonitor is unset; " > - "set it if you really want to " > - "enable fsmonitor")); > + warning(_("core.fsmonitor is unset; " > + "set it if you really want to " > + "enable fsmonitor")); > } > add_fsmonitor(&the_index); > report(_("fsmonitor enabled")); > } else if (!fsmonitor) { > enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r); > if (fsm_mode > FSMONITOR_MODE_DISABLED) > - advise(_("core.fsmonitor is set; " > - "remove it if you really want to " > - "disable fsmonitor")); > + warning(_("core.fsmonitor is set; " > + "remove it if you really want to " > + "disable fsmonitor")); > remove_fsmonitor(&the_index); > report(_("fsmonitor disabled")); > }
On 3/14/22 2:08 AM, Junio C Hamano wrote: > "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Jeff Hostetler <jeffhost@microsoft.com> >> >> fixup! update-index: convert fsmonitor warnings to advise > > Same comment as 01/16 applies here. "convert ... back to ..." in > the title refers to the fact that builtin-fsmonitor-part2 topic > turned warning() into advise() without a good justification, I > think. Flipping and flopping between warning and advise, without > giving any justification going either direction, is not a good move. Sorry for not including the backstory. Ben wrote the original warning message back in 2017. In my original version of part 2 I added a more detailed warning message because of the new `core.useBuiltinFSMonitor` config settings and how it interacted with `core.fsmonitor`. And we talked about making that longer message advise rather than a warning. So I changed them to advise(). But then we decided to remove `core.useBuiltinFSMonitor` and overload `core.fsmonitor` to take a boolean, so the original text of the message was sufficient and correct. So to minimize the diff, I reverted the text change and kept the change from warning() to advise(). But then there were comments from AEvar on either changing all of the nearby warning() calls to advise() *and* to change them to use the `advise_type` enum. That seemed like a large/disruptive change at this point. Also, I wasn't really sure of the need for Ben's original warning message, so I'd rather leave it as is than expand the scope. So I basically reverted the change for this series. If (in a later series) we want to revisit all of the warnings in update-index.c, then we can think about this. Jeff
diff --git a/builtin/update-index.c b/builtin/update-index.c index d335f1ac72a..75d646377cc 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -1238,18 +1238,18 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) if (fsmonitor > 0) { enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r); if (fsm_mode == FSMONITOR_MODE_DISABLED) { - advise(_("core.fsmonitor is unset; " - "set it if you really want to " - "enable fsmonitor")); + warning(_("core.fsmonitor is unset; " + "set it if you really want to " + "enable fsmonitor")); } add_fsmonitor(&the_index); report(_("fsmonitor enabled")); } else if (!fsmonitor) { enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r); if (fsm_mode > FSMONITOR_MODE_DISABLED) - advise(_("core.fsmonitor is set; " - "remove it if you really want to " - "disable fsmonitor")); + warning(_("core.fsmonitor is set; " + "remove it if you really want to " + "disable fsmonitor")); remove_fsmonitor(&the_index); report(_("fsmonitor disabled")); }