[Outreachy] advice: revamp advise API #548

gitgitgadget · 2020-02-17T03:32:02Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

There seem to be a handful of lines with trailing whitespaces. I think I've fixed them all on the receiving end, but please proofread before you come up with the next round. Thanks.

On the Git mailing list, Heba Waly wrote (reply to this):

On Mon, Feb 17, 2020 at 4:28 PM Junio C Hamano <[email protected]> wrote: > >And I see that now you are going all the way to discard the >string-based keys to enumeration, I think this deserves to be called >"revamp advise API". Fair enough. > There seem to be a handful of lines with trailing whitespaces. I > think I've fixed them all on the receiving end, but please proofread > before you come up with the next round. > Sorry about that, I fixed this setting in my IDE a while ago but looks like it has a bug. Will keep an eye on it in the future, thanks > Thanks. Heba

gitgitgadget · 2020-02-19T10:04:32Z

On the Git mailing list, Heba Waly wrote (reply to this):

On Mon, Feb 17, 2020 at 10:39 AM Heba Waly via GitGitGadget <[email protected]> wrote: > > +int advice_enabled(enum advice_type type) > +{ > + int value = 1; > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > + git_config_get_bool(key, &value); > + return value; > +} > + > +int advice_push_update_rejected_enabled(void) > +{ > + return advice_enabled(PUSH_UPDATE_REJECTED) || > + advice_enabled(PUSH_UPDATE_REJECTED_ALIAS); > + > +} I just realized that the return value in this function should be && instead of || (if any of the config variables is 0, then the advice should be disabled), will fix that in the next round. I'll probably remove this function and add a switch case to advice_enabled() to handle this case as well as another one that I came across while migrating the rest of the calls to the new helpers. Will post an updated version soon. > -- > gitgitgadget >

gitgitgadget · 2020-02-20T01:41:44Z

On the Git mailing list, Emily Shaffer wrote (reply to this):

On Wed, Feb 19, 2020 at 08:34:00PM +0000, Heba Waly via GitGitGadget wrote: > From: Heba Waly <[email protected]> > > Currently it's very easy for the advice library's callers to miss > checking the visibility step before printing an advice. Also, it makes > more sense for this step to be handled by the advice library. > > Add a new advise_if_enabled function that checks the visibility of > advice messages before printing. > > Add a new helper advise_enabled to check the visibility of the advice > if the caller needs to carry out complicated processing based on that > value. > > A list of config variables 'advice_config_keys' is added to be used by > list_config_advices() instead of 'advice_config[]' because we'll get > rid of 'advice_config[]' and the global variables once we migrate all > the callers to use the new APIs. > > Also change the advise call in tag library from advise() to > advise_if_enabled() to construct an example of the usage of the new > API. > > Signed-off-by: Heba Waly <[email protected]> > --- > Makefile | 1 + > advice.c | 92 ++++++++++++++++++++++++++++++++++++++++-- > advice.h | 53 +++++++++++++++++++++++- > builtin/tag.c | 4 +- > t/helper/test-advise.c | 16 ++++++++ > t/helper/test-tool.c | 1 + > t/helper/test-tool.h | 1 + > t/t0018-advice.sh | 28 +++++++++++++ > t/t7004-tag.sh | 1 + > 9 files changed, 190 insertions(+), 7 deletions(-) > create mode 100644 t/helper/test-advise.c > create mode 100755 t/t0018-advice.sh > > diff --git a/Makefile b/Makefile > index 09f98b777ca..ed923a3e818 100644 > --- a/Makefile > +++ b/Makefile > @@ -695,6 +695,7 @@ X = > > PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS)) > > +TEST_BUILTINS_OBJS += test-advise.o > TEST_BUILTINS_OBJS += test-chmtime.o > TEST_BUILTINS_OBJS += test-config.o > TEST_BUILTINS_OBJS += test-ctype.o > diff --git a/advice.c b/advice.c > index 249c60dcf32..345319005ac 100644 > --- a/advice.c > +++ b/advice.c > @@ -29,7 +29,6 @@ int advice_ignored_hook = 1; > int advice_waiting_for_editor = 1; > int advice_graft_file_deprecated = 1; > int advice_checkout_ambiguous_remote_branch_name = 1; > -int advice_nested_tag = 1; > int advice_submodule_alternate_error_strategy_die = 1; > > static int advice_use_color = -1; > @@ -89,13 +88,46 @@ static struct { > { "waitingForEditor", &advice_waiting_for_editor }, > { "graftFileDeprecated", &advice_graft_file_deprecated }, > { "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name }, > - { "nestedTag", &advice_nested_tag }, > { "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die }, > > /* make this an alias for backward compatibility */ > { "pushNonFastForward", &advice_push_update_rejected } > }; > > +static const char *advice_config_keys[] = { > + [FETCH_SHOW_FORCED_UPDATES] = "fetchShowForcedUpdates", > + [PUSH_UPDATE_REJECTED] = "pushUpdateRejected", > + /* make this an alias for backward compatibility */ > + [PUSH_UPDATE_REJECTED_ALIAS] = "pushNonFastForward", > + > + [PUSH_NON_FF_CURRENT] = "pushNonFFCurrent", > + [PUSH_NON_FF_MATCHING] = "pushNonFFMatching", > + [PUSH_ALREADY_EXISTS] = "pushAlreadyExists", > + [PUSH_FETCH_FIRST] = "pushFetchFirst", > + [PUSH_NEEDS_FORCE] = "pushNeedsForce", > + [PUSH_UNQUALIFIED_REF_NAME] = "pushUnqualifiedRefName", > + [STATUS_HINTS] = "statusHints", > + [STATUS_U_OPTION] = "statusUoption", > + [STATUS_AHEAD_BEHIND_WARNING] = "statusAheadBehindWarning", > + [COMMIT_BEFORE_MERGE] = "commitBeforeMerge", > + [RESET_QUIET_WARNING] = "resetQuiet", > + [RESOLVE_CONFLICT] = "resolveConflict", > + [SEQUENCER_IN_USE] = "sequencerInUse", > + [IMPLICIT_IDENTITY] = "implicitIdentity", > + [DETACHED_HEAD] = "detachedHead", > + [SET_UPSTREAM_FAILURE] = "setupStreamFailure", > + [OBJECT_NAME_WARNING] = "objectNameWarning", > + [AMWORKDIR] = "amWorkDir", > + [RM_HINTS] = "rmHints", > + [ADD_EMBEDDED_REPO] = "addEmbeddedRepo", > + [IGNORED_HOOK] = "ignoredHook", > + [WAITING_FOR_EDITOR] = "waitingForEditor", > + [GRAFT_FILE_DEPRECATED] = "graftFileDeprecated", > + [CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = "checkoutAmbiguousRemoteBranchName", > + [NESTED_TAG] = "nestedTag", > + [SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = "submoduleAlternateErrorStrategyDie" > +}; > + > void advise(const char *advice, ...) > { > struct strbuf buf = STRBUF_INIT; > @@ -118,6 +150,58 @@ void advise(const char *advice, ...) > strbuf_release(&buf); > } > > +static int get_config_value(enum advice_type type) > +{ > + int value = 1; > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); Same comment as your other call for xstrfmt. I think you need to manage the output. > + git_config_get_bool(key, &value); > + return value; > +} > + > +int advice_enabled(enum advice_type type) > +{ > + switch(type) { > + case PUSH_UPDATE_REJECTED: > + return get_config_value(PUSH_UPDATE_REJECTED) && > + get_config_value(PUSH_UPDATE_REJECTED_ALIAS); > + default: > + return get_config_value(type); > + } > +} > + > +static const char turn_off_instructions[] = > +N_("\n" > + "Disable this message with \"git config %s false\""); > + > +void advise_if_enabled(enum advice_type type, const char *advice, ...) > +{ > + struct strbuf buf = STRBUF_INIT; > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); Hmm, doesn't this leak? On the surface it looks like xstrfmt can save you a strbuf allocation, but if you check in strbuf.c, it actually allocates and detaches a strbuf for you anyways. I'd argue that it's easier to tell whether you're leaking a strbuf than the result of this call, so you might as well do it that way. > + va_list params; > + const char *cp, *np; > + > + if(!advice_enabled(type)) > + return; > + > + va_start(params, advice); > + strbuf_vaddf(&buf, advice, params); > + va_end(params); > + > + strbuf_addf(&buf, turn_off_instructions, key); > + > + for (cp = buf.buf; *cp; cp = np) { > + np = strchrnul(cp, '\n'); > + fprintf(stderr, _("%shint: %.*s%s\n"), > + advise_get_color(ADVICE_COLOR_HINT), > + (int)(np - cp), cp, > + advise_get_color(ADVICE_COLOR_RESET)); > + if (*np) > + np++; > + } Hm. This seems like something the project would use if strbuf knew how to do it. Something like strbuf_prefix_lines(strbuf, prefix, delimiter) and strbuf_suffix_lines(strbuf, suffix, delimiter)? I think there are other types of output which need to prepend each line like this? > + strbuf_release(&buf); > + > +} > + > int git_default_advice_config(const char *var, const char *value) > { > const char *k, *slot_name; > @@ -154,8 +238,8 @@ void list_config_advices(struct string_list *list, const char *prefix) > { > int i; > > - for (i = 0; i < ARRAY_SIZE(advice_config); i++) > - list_config_item(list, prefix, advice_config[i].name); > + for (i = 0; i < ARRAY_SIZE(advice_config_keys); i++) > + list_config_item(list, prefix, advice_config_keys[i]); > } > > int error_resolve_conflict(const char *me) > diff --git a/advice.h b/advice.h > index b706780614d..c8be662c4b1 100644 > --- a/advice.h > +++ b/advice.h > @@ -29,12 +29,63 @@ extern int advice_ignored_hook; > extern int advice_waiting_for_editor; > extern int advice_graft_file_deprecated; > extern int advice_checkout_ambiguous_remote_branch_name; > -extern int advice_nested_tag; > extern int advice_submodule_alternate_error_strategy_die; > > +/** > + To add a new advice, you need to: > + - Define an advice_type. > + - Add a new entry to advice_config_keys list. > + - Add the new config variable to Documentation/config/advice.txt. > + - Call advise_if_enabled to print your advice. > + */ > +enum advice_type { > + FETCH_SHOW_FORCED_UPDATES = 0, > + PUSH_UPDATE_REJECTED = 1, > + PUSH_UPDATE_REJECTED_ALIAS = 2, > + PUSH_NON_FF_CURRENT = 3, > + PUSH_NON_FF_MATCHING = 4, > + PUSH_ALREADY_EXISTS = 5, > + PUSH_FETCH_FIRST = 6, > + PUSH_NEEDS_FORCE = 7, > + PUSH_UNQUALIFIED_REF_NAME = 8, > + STATUS_HINTS = 9, > + STATUS_U_OPTION = 10, > + STATUS_AHEAD_BEHIND_WARNING = 11, > + COMMIT_BEFORE_MERGE = 12, > + RESET_QUIET_WARNING = 13, > + RESOLVE_CONFLICT = 14, > + SEQUENCER_IN_USE = 15, > + IMPLICIT_IDENTITY = 16, > + DETACHED_HEAD = 17, > + SET_UPSTREAM_FAILURE = 18, > + OBJECT_NAME_WARNING = 19, > + AMWORKDIR = 20, > + RM_HINTS = 21, > + ADD_EMBEDDED_REPO = 22, > + IGNORED_HOOK = 23, > + WAITING_FOR_EDITOR = 24, > + GRAFT_FILE_DEPRECATED = 25, > + CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME = 26, > + NESTED_TAG = 27, > + SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE = 28, > +}; Hmm. I wanted to say, "Our codebase uses ALL_CAPS or snake_case in enums so you could use lowers if you wanted" - but based on 'git grep -A4 "^enum"' it's actually pretty unusual to see enums with lower-case members. Dang :) > + > + > int git_default_advice_config(const char *var, const char *value); > __attribute__((format (printf, 1, 2))) > void advise(const char *advice, ...); > + > +/** > + Checks if advice type is enabled (can be printed to the user). > + Should be called before advise(). > + */ > +int advice_enabled(enum advice_type type); > + > +/** > + Checks the visibility of the advice before printing. > + */ > +void advise_if_enabled(enum advice_type type, const char *advice, ...); > + > int error_resolve_conflict(const char *me); > void NORETURN die_resolve_conflict(const char *me); > void NORETURN die_conclude_merge(void); > diff --git a/builtin/tag.c b/builtin/tag.c > index e0a4c253828..247d9075e19 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -231,8 +231,8 @@ static void create_tag(const struct object_id *object, const char *object_ref, > if (type <= OBJ_NONE) > die(_("bad object type.")); > > - if (type == OBJ_TAG && advice_nested_tag) > - advise(_(message_advice_nested_tag), tag, object_ref); > + if (type == OBJ_TAG) > + advise_if_enabled(NESTED_TAG, _(message_advice_nested_tag), tag, object_ref); Hm, if it was me, I would have put this bit in its own commit. But I see that it's a pretty tiny change... > > strbuf_addf(&header, > "object %s\n" > diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c > new file mode 100644 > index 00000000000..6d28c9cd5aa > --- /dev/null > +++ b/t/helper/test-advise.c > @@ -0,0 +1,16 @@ > +#include "test-tool.h" > +#include "cache.h" > +#include "advice.h" > + > +int cmd__advise_if_enabled(int argc, const char **argv) > +{ > + if (!argv[1]) > + die("usage: %s <advice>", argv[0]); > + > + setup_git_directory(); > + > + //use any advice type for testing I think this might be misleading - your t0018 does quite a few checks explicitly for the NESTED_TAG advice. Maybe it's better to say something like "Make sure this agrees with t0018"? Also nit, I've been told off a few times for using //c++ style comments. > + advise_if_enabled(NESTED_TAG, argv[1]); > + > + return 0; > +} > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c > index f20989d4497..6977badc690 100644 > --- a/t/helper/test-tool.c > +++ b/t/helper/test-tool.c > @@ -14,6 +14,7 @@ struct test_cmd { > }; > > static struct test_cmd cmds[] = { > + { "advise", cmd__advise_if_enabled }, > { "chmtime", cmd__chmtime }, > { "config", cmd__config }, > { "ctype", cmd__ctype }, > diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h > index 8ed2af71d1b..ca5e33b842f 100644 > --- a/t/helper/test-tool.h > +++ b/t/helper/test-tool.h > @@ -4,6 +4,7 @@ > #define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "git-compat-util.h" > > +int cmd__advise_if_enabled(int argc, const char **argv); > int cmd__chmtime(int argc, const char **argv); > int cmd__config(int argc, const char **argv); > int cmd__ctype(int argc, const char **argv); > diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh > new file mode 100755 > index 00000000000..f4cdb649d51 > --- /dev/null > +++ b/t/t0018-advice.sh > @@ -0,0 +1,28 @@ > +#!/bin/sh > + > +test_description='Test advise_if_enabled functionality' > + > +. ./test-lib.sh > + > +cat > expected <<EOF > +hint: This is a piece of advice > +hint: Disable this message with "git config advice.nestedTag false" > +EOF > +test_expect_success 'advise should be printed when config variable is unset' ' > + test-tool advise "This is a piece of advice" 2>actual && > + test_i18ncmp expected actual > +' > + > +test_expect_success 'advise should be printed when config variable is set to true' ' > + test_config advice.nestedTag true && > + test-tool advise "This is a piece of advice" 2>actual && > + test_i18ncmp expected actual > +' > + > +test_expect_success 'advise should not be printed when config variable is set to false' ' > + test_config advice.nestedTag false && > + test-tool advise "This is a piece of advice" 2>actual && > + test_must_be_empty actual > +' > + > +test_done > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > index 6db92bd3ba6..74b637deb25 100755 > --- a/t/t7004-tag.sh > +++ b/t/t7004-tag.sh > @@ -1726,6 +1726,7 @@ test_expect_success 'recursive tagging should give advice' ' > hint: already a tag. If you meant to tag the object that it points to, use: > hint: | > hint: git tag -f nested annotated-v4.0^{} > + hint: Disable this message with "git config advice.nestedTag false" > EOF > git tag -m nested nested annotated-v4.0 2>actual && > test_i18ncmp expect actual > -- > gitgitgadget >

On the Git mailing list, Heba Waly wrote (reply to this):

On Thu, Feb 20, 2020 at 2:37 PM Emily Shaffer <[email protected]> wrote: > > On Wed, Feb 19, 2020 at 08:34:00PM +0000, Heba Waly via GitGitGadget wrote: > > From: Heba Waly <[email protected]> > > > > +static int get_config_value(enum advice_type type) > > +{ > > + int value = 1; > > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > > Same comment as your other call for xstrfmt. I think you need to manage > the output. Got it. > > +void advise_if_enabled(enum advice_type type, const char *advice, ...) > > +{ > > + struct strbuf buf = STRBUF_INIT; > > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > > Hmm, doesn't this leak? > > On the surface it looks like xstrfmt can save you a strbuf allocation, > but if you check in strbuf.c, it actually allocates and detaches a > strbuf for you anyways. I'd argue that it's easier to tell whether > you're leaking a strbuf than the result of this call, so you might as > well do it that way. > You're right. > > + SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE = 28, > > +}; > > Hmm. I wanted to say, "Our codebase uses ALL_CAPS or snake_case in enums > so you could use lowers if you wanted" - but based on 'git grep -A4 > "^enum"' it's actually pretty unusual to see enums with lower-case > members. Dang :) > Yeah I thought the same at first too but reached the same result. > > + > > + advise_if_enabled(NESTED_TAG, _(message_advice_nested_tag), tag, object_ref); > > Hm, if it was me, I would have put this bit in its own commit. But I see > that it's a pretty tiny change... > That'd have been better of course, don't know why I didn't think of that :) > > + //use any advice type for testing > I think this might be misleading - your t0018 does quite a few checks > explicitly for the NESTED_TAG advice. Maybe it's better to say something > like "Make sure this agrees with t0018"? I see your point, I'll need to think more about this one. > Also nit, I've been told off a > few times for using //c++ style comments. Oh ok. Thanks a lot, Emily Heba

gitgitgadget · 2020-02-24T22:06:40Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Heba Waly via GitGitGadget" <[email protected]> writes: > -static void vadvise(const char *advice, va_list params) > +static const char *advice_config_keys[] = { > + [FETCH_SHOW_FORCED_UPDATES] = "fetchShowForcedUpdates", > + [PUSH_UPDATE_REJECTED] = "pushUpdateRejected", > + /* make this an alias for backward compatibility */ > + [PUSH_UPDATE_REJECTED_ALIAS] = "pushNonFastForward", > +... > + [CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = "checkoutAmbiguousRemoteBranchName", > + [NESTED_TAG] = "nestedTag", > + [SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = "submoduleAlternateErrorStrategyDie" > +}; Terminate the last entry of the array with a trailing comma ',', so that the next person who adds one new advise key to the table at the end has to just add only one line without changing any existing lines. As you are using the designated initializers for this array, we are free to order the lines in any way that makes most sense to us and do not have to order the lines in numerical order. In what order are these lines sorted right now? I am tempted to suggest that we should sort alphabetically on the values, i.e. run the contents of the table through "LC_ALL=C sort -k2,2 -t=". > + > +static const char turn_off_instructions[] = > +N_("\n" > + "Disable this message with \"git config %s false\""); > + > +static void vadvise(const char *advice, va_list params, > + int display_instructions, char *key) It may be just me, but I feel uneasy when I see va_list in the middle of the parameter list. As it is a mechanism to allow us handle "the remainder of the arguments", it logically makes more sense to have it as the last parameter. > { > struct strbuf buf = STRBUF_INIT; > const char *cp, *np; > > strbuf_vaddf(&buf, advice, params); > > + if(display_instructions) > + strbuf_addf(&buf, turn_off_instructions, key); Style. We always have one SP between a syntactic keyword like "if" and open parenthesis. > + > for (cp = buf.buf; *cp; cp = np) { > np = strchrnul(cp, '\n'); > fprintf(stderr, _("%shint: %.*s%s\n"), > @@ -119,8 +161,42 @@ void advise(const char *advice, ...) > { > va_list params; > va_start(params, advice); > - vadvise(advice, params); > + vadvise(advice, params, 0, ""); > + va_end(params); > +} > + > +static int get_config_value(enum advice_type type) > +{ > + int value = 1; > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); Have a blank line between the decl and the first statement, i.e. here. > + git_config_get_bool(key, &value); > + free(key); > + return value; > +} > + > +int advice_enabled(enum advice_type type) > +{ > + switch(type) { Style. > + case PUSH_UPDATE_REJECTED: > + return get_config_value(PUSH_UPDATE_REJECTED) && > + get_config_value(PUSH_UPDATE_REJECTED_ALIAS); > + default: > + return get_config_value(type); > + } > +} > + > +void advise_if_enabled(enum advice_type type, const char *advice, ...) > +{ > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > + va_list params; > + > + if(!advice_enabled(type)) Stile. > + return; > + > + va_start(params, advice); > + vadvise(advice, params, 1, key); > va_end(params); > + free(key); > } > > int git_default_advice_config(const char *var, const char *value) > @@ -159,8 +235,8 @@ void list_config_advices(struct string_list *list, const char *prefix) > { > int i; > > - for (i = 0; i < ARRAY_SIZE(advice_config); i++) > - list_config_item(list, prefix, advice_config[i].name); > + for (i = 0; i < ARRAY_SIZE(advice_config_keys); i++) > + list_config_item(list, prefix, advice_config_keys[i]); > } > > int error_resolve_conflict(const char *me) > diff --git a/advice.h b/advice.h > index b706780614d..61a7ee82827 100644 > --- a/advice.h > +++ b/advice.h > @@ -32,9 +32,61 @@ extern int advice_checkout_ambiguous_remote_branch_name; > extern int advice_nested_tag; > extern int advice_submodule_alternate_error_strategy_die; > > +/** > + To add a new advice, you need to: > + - Define an advice_type. > + - Add a new entry to advice_config_keys list. > + - Add the new config variable to Documentation/config/advice.txt. > + - Call advise_if_enabled to print your advice. > + */ /* * Our multi-line comments should look * more like this (multiple style violations * in this patch). */ > +enum advice_type { > + FETCH_SHOW_FORCED_UPDATES = 0, > + PUSH_UPDATE_REJECTED = 1, > + PUSH_UPDATE_REJECTED_ALIAS = 2, > + PUSH_NON_FF_CURRENT = 3, Do we need to spell out the values, or is it sufficient to let the compiler automatically count up? Does any code depend on the exact numeric value of the advice type, or at least at the source code level the only thing we care about them is that they are distinct? I'd really want to get rid of these exact value assignments---they are source of unnecessary conflicts when two or more topics want to add new advice types of their own. I also suggest that these are sorted alphabetically. > + ... > + SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE = 28, > +}; > +++ b/t/t0018-advice.sh > @@ -0,0 +1,28 @@ > +#!/bin/sh > + > +test_description='Test advise_if_enabled functionality' > + > +. ./test-lib.sh > + > +cat > expected <<EOF > +hint: This is a piece of advice > +hint: Disable this message with "git config advice.nestedTag false" > +EOF > +test_expect_success 'advise should be printed when config variable is unset' ' > + test-tool advise "This is a piece of advice" 2>actual && > + test_i18ncmp expected actual > +' - Prepare the expected output inside test_expect_success block that uses it. - There should be no SP between a redirection operator and the filename. - Here-doc that does not use parameter expansion should use a quoted EOF marker. - The file that gets compared with "actual" is by convention called "expect", not "expected". i.e. test_expect_success 'advise should be printed when config variable is unset' ' cat >expect <<-\EOF && hint: ... hint: ... EOF test-tool advise "This is a piece of advice" 2>actual && test_i18ncmp expected actual '

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Mon, Feb 24, 2020 at 5:05 PM Junio C Hamano <[email protected]> wrote: > "Heba Waly via GitGitGadget" <[email protected]> writes: > > +test_expect_success 'advise should be printed when config variable is unset' ' > > + test-tool advise "This is a piece of advice" 2>actual && > > + test_i18ncmp expected actual > > +' > > - Prepare the expected output inside test_expect_success block that > uses it. > - There should be no SP between a redirection operator and the > filename. > - Here-doc that does not use parameter expansion should use a > quoted EOF marker. > - The file that gets compared with "actual" is by convention called > "expect", not "expected". > > test_expect_success 'advise should be printed when config variable is unset' ' Also, s/advise/advice/ in the test title. > cat >expect <<-\EOF && > hint: ... > hint: ... > EOF > test-tool advise "This is a piece of advice" 2>actual && > test_i18ncmp expected actual > ' s/expected/expect/

On the Git mailing list, Heba Waly wrote (reply to this):

On Tue, Feb 25, 2020 at 11:05 AM Junio C Hamano <[email protected]> wrote: > > "Heba Waly via GitGitGadget" <[email protected]> writes: > > > -static void vadvise(const char *advice, va_list params) > > +static const char *advice_config_keys[] = { > > + [FETCH_SHOW_FORCED_UPDATES] = "fetchShowForcedUpdates", > > + [PUSH_UPDATE_REJECTED] = "pushUpdateRejected", > > + /* make this an alias for backward compatibility */ > > + [PUSH_UPDATE_REJECTED_ALIAS] = "pushNonFastForward", > > +... > > + [CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = "checkoutAmbiguousRemoteBranchName", > > + [NESTED_TAG] = "nestedTag", > > + [SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = "submoduleAlternateErrorStrategyDie" > > +}; > > Terminate the last entry of the array with a trailing comma ',', so > that the next person who adds one new advise key to the table at the > end has to just add only one line without changing any existing lines. > Sure, thank you for the explanation, that makes sense. > As you are using the designated initializers for this array, we are > free to order the lines in any way that makes most sense to us and > do not have to order the lines in numerical order. In what order > are these lines sorted right now? I am tempted to suggest that we > should sort alphabetically on the values, i.e. run the contents of > the table through "LC_ALL=C sort -k2,2 -t=". > Currently it's sorted in the same order that was originally used for advice_config[] and the global variables, which I assume is the order in which every config variable was added to the code. Sorting alphabetically will be neater of course. > > + > > +static const char turn_off_instructions[] = > > +N_("\n" > > + "Disable this message with \"git config %s false\""); > > + > > +static void vadvise(const char *advice, va_list params, > > + int display_instructions, char *key) > > It may be just me, but I feel uneasy when I see va_list in the > middle of the parameter list. As it is a mechanism to allow us > handle "the remainder of the arguments", it logically makes more > sense to have it as the last parameter. > I don't mind it either way, so yeah no problem, I'll change that. > > { > > struct strbuf buf = STRBUF_INIT; > > const char *cp, *np; > > > > strbuf_vaddf(&buf, advice, params); > > > > + if(display_instructions) > > + strbuf_addf(&buf, turn_off_instructions, key); > > Style. We always have one SP between a syntactic keyword like "if" > and open parenthesis. Noted. > > > + > > for (cp = buf.buf; *cp; cp = np) { > > np = strchrnul(cp, '\n'); > > fprintf(stderr, _("%shint: %.*s%s\n"), > > @@ -119,8 +161,42 @@ void advise(const char *advice, ...) > > { > > va_list params; > > va_start(params, advice); > > - vadvise(advice, params); > > + vadvise(advice, params, 0, ""); > > + va_end(params); > > +} > > + > > +static int get_config_value(enum advice_type type) > > +{ > > + int value = 1; > > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > > Have a blank line between the decl and the first statement, i.e. here. > Right, I missed this one. > > + git_config_get_bool(key, &value); > > + free(key); > > + return value; > > +} > > + > > +int advice_enabled(enum advice_type type) > > +{ > > + switch(type) { > > Style. Noted. > > > + case PUSH_UPDATE_REJECTED: > > + return get_config_value(PUSH_UPDATE_REJECTED) && > > + get_config_value(PUSH_UPDATE_REJECTED_ALIAS); > > + default: > > + return get_config_value(type); > > + } > > +} > > + > > +void advise_if_enabled(enum advice_type type, const char *advice, ...) > > +{ > > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > > + va_list params; > > + > > + if(!advice_enabled(type)) > > Stile. Takes time changing the style I've been using for years, sorry. > > > + return; > > + > > + va_start(params, advice); > > + vadvise(advice, params, 1, key); > > va_end(params); > > + free(key); > > } > > > > int git_default_advice_config(const char *var, const char *value) > > @@ -159,8 +235,8 @@ void list_config_advices(struct string_list *list, const char *prefix) > > { > > int i; > > > > - for (i = 0; i < ARRAY_SIZE(advice_config); i++) > > - list_config_item(list, prefix, advice_config[i].name); > > + for (i = 0; i < ARRAY_SIZE(advice_config_keys); i++) > > + list_config_item(list, prefix, advice_config_keys[i]); > > } > > > > int error_resolve_conflict(const char *me) > > diff --git a/advice.h b/advice.h > > index b706780614d..61a7ee82827 100644 > > --- a/advice.h > > +++ b/advice.h > > @@ -32,9 +32,61 @@ extern int advice_checkout_ambiguous_remote_branch_name; > > extern int advice_nested_tag; > > extern int advice_submodule_alternate_error_strategy_die; > > > > +/** > > + To add a new advice, you need to: > > + - Define an advice_type. > > + - Add a new entry to advice_config_keys list. > > + - Add the new config variable to Documentation/config/advice.txt. > > + - Call advise_if_enabled to print your advice. > > + */ > > /* > * Our multi-line comments should look > * more like this (multiple style violations > * in this patch). > */ > Got it. > > +enum advice_type { > > + FETCH_SHOW_FORCED_UPDATES = 0, > > + PUSH_UPDATE_REJECTED = 1, > > + PUSH_UPDATE_REJECTED_ALIAS = 2, > > + PUSH_NON_FF_CURRENT = 3, > > Do we need to spell out the values, or is it sufficient to let the > compiler automatically count up? Does any code depend on the exact > numeric value of the advice type, or at least at the source code > level the only thing we care about them is that they are distinct? > > I'd really want to get rid of these exact value assignments---they > are source of unnecessary conflicts when two or more topics want to > add new advice types of their own. > Yes, we can get rid of them. > I also suggest that these are sorted alphabetically. As we'll get rid of the value assignments, so sorting alphabetically should not introduce problems when adding new advice types, will do. > > + ... > > + SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE = 28, > > +}; > > > +++ b/t/t0018-advice.sh > > @@ -0,0 +1,28 @@ > > +#!/bin/sh > > + > > +test_description='Test advise_if_enabled functionality' > > + > > +. ./test-lib.sh > > + > > +cat > expected <<EOF > > +hint: This is a piece of advice > > +hint: Disable this message with "git config advice.nestedTag false" > > +EOF > > +test_expect_success 'advise should be printed when config variable is unset' ' > > + test-tool advise "This is a piece of advice" 2>actual && > > + test_i18ncmp expected actual > > +' > > - Prepare the expected output inside test_expect_success block that > uses it. > > - There should be no SP between a redirection operator and the > filename. > > - Here-doc that does not use parameter expansion should use a > quoted EOF marker. > > - The file that gets compared with "actual" is by convention called > "expect", not "expected". > > i.e. > > test_expect_success 'advise should be printed when config variable is unset' ' > cat >expect <<-\EOF && > hint: ... > hint: ... > EOF > test-tool advise "This is a piece of advice" 2>actual && > test_i18ncmp expected actual > ' Noted. Will send an updated version soon. Thank you, Heba

On the Git mailing list, Heba Waly wrote (reply to this):

On Tue, Feb 25, 2020 at 11:11 AM Eric Sunshine <[email protected]> wrote: > > On Mon, Feb 24, 2020 at 5:05 PM Junio C Hamano <[email protected]> wrote: > > "Heba Waly via GitGitGadget" <[email protected]> writes: > > > +test_expect_success 'advise should be printed when config variable is unset' ' > > > + test-tool advise "This is a piece of advice" 2>actual && > > > + test_i18ncmp expected actual > > > +' > > > > - Prepare the expected output inside test_expect_success block that > > uses it. > > - There should be no SP between a redirection operator and the > > filename. > > - Here-doc that does not use parameter expansion should use a > > quoted EOF marker. > > - The file that gets compared with "actual" is by convention called > > "expect", not "expected". > > > > test_expect_success 'advise should be printed when config variable is unset' ' > > Also, s/advise/advice/ in the test title. > > > cat >expect <<-\EOF && > > hint: ... > > hint: ... > > EOF > > test-tool advise "This is a piece of advice" 2>actual && > > test_i18ncmp expected actual > > ' > > s/expected/expect/ Noted, thank you. Heba

gitgitgadget · 2020-02-24T23:48:49Z

On the Git mailing list, Emily Shaffer wrote (reply to this):

On Mon, Feb 24, 2020 at 03:13:17PM +0000, Heba Waly via GitGitGadget wrote: > From: Heba Waly <[email protected]> > > Currently it's very easy for the advice library's callers to miss > checking the visibility step before printing an advice. Also, it makes > more sense for this step to be handled by the advice library. > > Add a new advise_if_enabled function that checks the visibility of > advice messages before printing. > > Add a new helper advise_enabled to check the visibility of the advice > if the caller needs to carry out complicated processing based on that > value. > > A list of config variables 'advice_config_keys' is added to be used by > list_config_advices() instead of 'advice_config[]' because we'll get > rid of 'advice_config[]' and the global variables once we migrate all > the callers to use the new APIs. > > Also change the advise call in tag library from advise() to > advise_if_enabled() to construct an example of the usage of the new > API. > > Signed-off-by: Heba Waly <[email protected]> I read Junio's review and agree with that too; but here are some more thoughts. > +static int get_config_value(enum advice_type type) > +{ > + int value = 1; So we default to true if the config is unset... > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > + git_config_get_bool(key, &value); ...and per config.h, "when the configuration variable `key` is now found, returns 1 without touching `dest`. Nice, so the default-true works. If some problem is found when converting the value to a bool, this function die()s, so you don't have to check the return value. > + free(key); > + return value; > +} > + > +int advice_enabled(enum advice_type type) > +{ > + switch(type) { > + case PUSH_UPDATE_REJECTED: > + return get_config_value(PUSH_UPDATE_REJECTED) && > + get_config_value(PUSH_UPDATE_REJECTED_ALIAS); So I can ask advice_enabled(PUSH_UPDATE_REJECTED) and still be told 'false' if earlier I set "advice.pushNonfastForward" to 0. I wondered if this was really identical behavior to how this thing worked before. Before, it looks like we use the older config callback method (git_default_advice_config()): we read each config option the user has, in order, and then we check it against each member of advice_config, and if it's a match, we set the appropriate value. That means that advice_push_update_rejected is determined by whichever config is set last, e.g. a config like so: global: advice.pushUpdateRejected = 1 local: advice.pushNonFastForward = 0 results in advice_push_update_rejected == 0. Now, though, you consider the values of both. In the example above, you have the same value; but if you reverse the values: global: advice.pushUpdateRejected = 0 local: advice.pushNonFastForward = 1 then your new code says advice_enabled(PUSH_UPDATE_REJECTED) == 0, and the old codepath says advice_push_update_rejected = 1. Although, using the config lookup methods as you are, I don't think you can make it stay the same - I don't think there's a way to compare the relative position of two different configs, is there? Is this such a big deal? My gut says no; my gut says if someone had advice.pushNonFastForward set, they aren't touching their config to unset it, which means they aren't touching their config to fiddle with advice.pushUpdateRejected either. Maybe someone who was around when this alias was added can provide some context? - Emily

gitgitgadget · 2020-02-25T17:43:22Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Heba Waly via GitGitGadget" <[email protected]> writes: > From: Heba Waly <[email protected]> > > Currently it's very easy for the advice library's callers to miss > checking the visibility step before printing an advice. Also, it makes > more sense for this step to be handled by the advice library. > > Add a new advise_if_enabled function that checks the visibility of > advice messages before printing. > > Add a new helper advise_enabled to check the visibility of the advice > if the caller needs to carry out complicated processing based on that > value. > > A list of config variables 'advice_config_keys' is added to be used by > list_config_advices() instead of 'advice_config[]' because we'll get > rid of 'advice_config[]' and the global variables once we migrate all > the callers to use the new APIs. > > Also change the advise call in tag library from advise() to > advise_if_enabled() to construct an example of the usage of the new > API. This is for step [3/3], isn't it? I'll discard this paragraph. > > Signed-off-by: Heba Waly <[email protected]> > --- > Makefile | 1 + > advice.c | 86 ++++++++++++++++++++++++++++++++++++++++-- > advice.h | 52 +++++++++++++++++++++++++ > t/helper/test-advise.c | 19 ++++++++++ > t/helper/test-tool.c | 1 + > t/helper/test-tool.h | 1 + > t/t0018-advice.sh | 32 ++++++++++++++++ > 7 files changed, 188 insertions(+), 4 deletions(-) > create mode 100644 t/helper/test-advise.c > create mode 100755 t/t0018-advice.sh > > diff --git a/Makefile b/Makefile > index 09f98b777ca..ed923a3e818 100644 > --- a/Makefile > +++ b/Makefile > @@ -695,6 +695,7 @@ X = > > PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS)) > > +TEST_BUILTINS_OBJS += test-advise.o > TEST_BUILTINS_OBJS += test-chmtime.o > TEST_BUILTINS_OBJS += test-config.o > TEST_BUILTINS_OBJS += test-ctype.o > diff --git a/advice.c b/advice.c > index fd836332dad..5c2068b8f8a 100644 > --- a/advice.c > +++ b/advice.c > @@ -96,13 +96,56 @@ static struct { > { "pushNonFastForward", &advice_push_update_rejected } > }; > > -static void vadvise(const char *advice, va_list params) > +static const char *advice_config_keys[] = { > + [ADD_EMBEDDED_REPO] = "addEmbeddedRepo", > + [AMWORKDIR] = "amWorkDir", > + [CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = "checkoutAmbiguousRemoteBranchName", > + [COMMIT_BEFORE_MERGE] = "commitBeforeMerge", > + [DETACHED_HEAD] = "detachedHead", > + [FETCH_SHOW_FORCED_UPDATES] = "fetchShowForcedUpdates", > + [GRAFT_FILE_DEPRECATED] = "graftFileDeprecated", > + [IGNORED_HOOK] = "ignoredHook", > + [IMPLICIT_IDENTITY] = "implicitIdentity", > + [NESTED_TAG] = "nestedTag", > + [OBJECT_NAME_WARNING] = "objectNameWarning", > + [PUSH_ALREADY_EXISTS] = "pushAlreadyExists", > + [PUSH_FETCH_FIRST] = "pushFetchFirst", > + [PUSH_NEEDS_FORCE] = "pushNeedsForce", > + > + /* make this an alias for backward compatibility */ > + [PUSH_UPDATE_REJECTED_ALIAS] = "pushNonFastForward", > + > + [PUSH_NON_FF_CURRENT] = "pushNonFFCurrent", > + [PUSH_NON_FF_MATCHING] = "pushNonFFMatching", > + [PUSH_UNQUALIFIED_REF_NAME] = "pushUnqualifiedRefName", > + [PUSH_UPDATE_REJECTED] = "pushUpdateRejected", > + [RESET_QUIET_WARNING] = "resetQuiet", > + [RESOLVE_CONFLICT] = "resolveConflict", > + [RM_HINTS] = "rmHints", > + [SEQUENCER_IN_USE] = "sequencerInUse", > + [SET_UPSTREAM_FAILURE] = "setupStreamFailure", > + [STATUS_AHEAD_BEHIND_WARNING] = "statusAheadBehindWarning", > + [STATUS_HINTS] = "statusHints", > + [STATUS_U_OPTION] = "statusUoption", > + [SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = "submoduleAlternateErrorStrategyDie", > + [WAITING_FOR_EDITOR] = "waitingForEditor", > +}; > + > +static const char turn_off_instructions[] = > +N_("\n" > + "Disable this message with \"git config %s false\""); > + > +static void vadvise(const char *advice, int display_instructions, > + char *key, va_list params) > { > struct strbuf buf = STRBUF_INIT; > const char *cp, *np; > > strbuf_vaddf(&buf, advice, params); > > + if (display_instructions) > + strbuf_addf(&buf, turn_off_instructions, key); > + > for (cp = buf.buf; *cp; cp = np) { > np = strchrnul(cp, '\n'); > fprintf(stderr, _("%shint: %.*s%s\n"), > @@ -119,8 +162,43 @@ void advise(const char *advice, ...) > { > va_list params; > va_start(params, advice); > - vadvise(advice, params); > + vadvise(advice, 0, "", params); > + va_end(params); > +} > + > +static int get_config_value(enum advice_type type) > +{ > + int value = 1; > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > + > + git_config_get_bool(key, &value); > + free(key); > + return value; > +} So, in this hypothetical but quite realistic example: if (advice_enabled(ADVICE_FOO)) { char *foo = expensive_preparation(); advice_if_enabled(ADVICE_FOO, "use of %s is discouraged", foo); } we end up formulating the "advice.*" key twice and ask git_config_get_bool() about the same key twice? > +int advice_enabled(enum advice_type type) > +{ > + switch (type) { > + case PUSH_UPDATE_REJECTED: > + return get_config_value(PUSH_UPDATE_REJECTED) && > + get_config_value(PUSH_UPDATE_REJECTED_ALIAS); > + default: > + return get_config_value(type); > + } > +} Also, as "enum advice_type" will be part of the public API, and there is little type checking for enums, we shouldn't be naming them randomly like these---we'd at least want to use a common prefix, like "ADVICE_", in front of them. Those who are focused only on advice subsystem may feel that names like PUSH_UPDATE_REJECTED are sufficiently clear, but within the context of the whole system, there is no cue that these UPCASED_WORDS identifiers belong to the advice subsystem or somewhere else. > +void advise_if_enabled(enum advice_type type, const char *advice, ...) > +{ > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > + va_list params; > + > + if (!advice_enabled(type)) > + return; Oh, no, make the number of calls to xstrfmr() three times, not twice, as I said in the previous example. I wonder if it would make the implementation better to do these: - Rename advice_config_keys[] to advice_setting[] that does not imply it is only about the keys; - This table will know, for each enum advice_type, which configuration variable enables it, *and* if it is enabled. i.e. static struct { const char *config_key; int disabled; } advice_setting[] = { [ADVICE_ADD_EMBEDED_REPO] = { "addEmbeddedRepo" }, [ADVICE_AM_WORK_DIR] = { "amWorkDir" }, ... [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" }, }; Side Note: you have AMWORKDIR that is unreadable. If the config name uses camelCase by convention, the UPCASED_WORDS should be separated with underscore at the same word boundary. Then, upon the first call to advice_enabled(), call git_config() with a callback like static int populate_advice_settings(const char *var, const char *value, void *cb) { int advice_type; const char *name; if (!skip_prefix(var, "advice.", &name)) return 0; advice_type = find_advice_type_by_name(advice_setting, name); if (advice_type < 0) return 0; /* unknown advice.* variable */ /* advice.foo=false means advice.foo is disabled */ advice_setting[advice_type].disabled = !git_config_bool(var, value); } only once. Your get_config_value() would then become a mere lookup in advice_setting[] array, e.g. int advice_enabled(unsigned advice_type) { static int initialized; if (!initialized) { initialized = 1; git_config(populate_advice_settings, NULL); } if (ARRAY_SIZE(advice_setting) <= advice_type) BUG("OOB advice type requested???"); return !advice_setting[advice_type].disabled; } with your "push-update-rejected has two names" twist added. Hmm?

On the Git mailing list, Emily Shaffer wrote (reply to this):

On Tue, Feb 25, 2020 at 09:40:28AM -0800, Junio C Hamano wrote: > "Heba Waly via GitGitGadget" <[email protected]> writes: > > +static int get_config_value(enum advice_type type) > > +{ > > + int value = 1; > > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > > + > > + git_config_get_bool(key, &value); > > + free(key); > > + return value; > > +} > > So, in this hypothetical but quite realistic example: > > if (advice_enabled(ADVICE_FOO)) { > char *foo = expensive_preparation(); > advice_if_enabled(ADVICE_FOO, "use of %s is discouraged", foo); > } > > we end up formulating the "advice.*" key twice and ask git_config_get_bool() > about the same key twice? > > > +void advise_if_enabled(enum advice_type type, const char *advice, ...) > > +{ > > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > > + va_list params; > > + > > + if (!advice_enabled(type)) > > + return; > > Oh, no, make the number of calls to xstrfmr() three times, not > twice, as I said in the previous example. > > I wonder if it would make the implementation better to do these: > > - Rename advice_config_keys[] to advice_setting[] that does not > imply it is only about the keys; > > - This table will know, for each enum advice_type, which > configuration variable enables it, *and* if it is enabled. > > i.e. > > static struct { > const char *config_key; > int disabled; > } advice_setting[] = { > [ADVICE_ADD_EMBEDED_REPO] = { "addEmbeddedRepo" }, > [ADVICE_AM_WORK_DIR] = { "amWorkDir" }, > ... > [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" }, > }; > > > Side Note: you have AMWORKDIR that is unreadable. If the config > name uses camelCase by convention, the UPCASED_WORDS > should be separated with underscore at the same word > boundary. > > Then, upon the first call to advice_enabled(), call git_config() > with a callback like > > static int populate_advice_settings(const char *var, const char *value, void *cb) > { > int advice_type; > const char *name; > > if (!skip_prefix(var, "advice.", &name)) > return 0; > advice_type = find_advice_type_by_name(advice_setting, name); > if (advice_type < 0) > return 0; /* unknown advice.* variable */ > /* advice.foo=false means advice.foo is disabled */ > advice_setting[advice_type].disabled = !git_config_bool(var, value); > } > > only once. Your get_config_value() would then become a mere lookup > in advice_setting[] array, e.g. > > int advice_enabled(unsigned advice_type) > { > static int initialized; > > if (!initialized) { > initialized = 1; > git_config(populate_advice_settings, NULL); > } > if (ARRAY_SIZE(advice_setting) <= advice_type) > BUG("OOB advice type requested???"); > return !advice_setting[advice_type].disabled; > } > > with your "push-update-rejected has two names" twist added. I'm a little confused about the need to cache the result of git_config_get_bool() - isn't that a lookup from a hashmap which is already populated at setup time, and therefore inexpensive? I would think the only expensive part here is the xstrfmt() calls, which it seems like would be easy to do away with by storing the fully-qualified advice key in the array instead. What am I missing? - Emily

On the Git mailing list, Junio C Hamano wrote (reply to this):

Emily Shaffer <[email protected]> writes: >> ... Your get_config_value() would then become a mere lookup >> in advice_setting[] array, e.g. >> >> int advice_enabled(unsigned advice_type) >> { >> static int initialized; >> >> if (!initialized) { >> initialized = 1; >> git_config(populate_advice_settings, NULL); >> } >> if (ARRAY_SIZE(advice_setting) <= advice_type) >> BUG("OOB advice type requested???"); >> return !advice_setting[advice_type].disabled; >> } >> >> with your "push-update-rejected has two names" twist added. > > I'm a little confused about the need to cache the result of > git_config_get_bool() - isn't that a lookup from a hashmap which is > already populated at setup time, and therefore inexpensive? Looking up from hashmap with a string key is always more expensive than indexing into a linear array with the array index. Also, the suggested arrangement makes the advice API implementation more self contained, I'd think.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <[email protected]> writes: > Emily Shaffer <[email protected]> writes: > >>> ... Your get_config_value() would then become a mere lookup >>> in advice_setting[] array, e.g. >>> >>> int advice_enabled(unsigned advice_type) >>> { >>> static int initialized; >>> >>> if (!initialized) { >>> initialized = 1; >>> git_config(populate_advice_settings, NULL); >>> } >>> if (ARRAY_SIZE(advice_setting) <= advice_type) >>> BUG("OOB advice type requested???"); >>> return !advice_setting[advice_type].disabled; >>> } >>> >>> with your "push-update-rejected has two names" twist added. One beauty of the approach is that the "twist" can be done in the initialization codepath, e.g. int advice_enabled(unsigned advice_type) { static int initialized; if (!initialized) { initialized = 1; git_config(populate_advice_settings, NULL); advice_setting[ADVICE_PUSH_UPDATE_REJECTED] &= advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]; } if (ARRAY_SIZE(advice_setting) <= advice_type) BUG("OOB advice type requested???"); return !advice_setting[advice_type].disabled; } which means that the function literally becomes an array access that is guarded for out-of-bounds index. Thanks, Emily, for making me look at the suggested code again to realize this ;-)

On the Git mailing list, Heba Waly wrote (reply to this):

On Wed, Feb 26, 2020 at 6:40 AM Junio C Hamano <[email protected]> wrote: > > "Heba Waly via GitGitGadget" <[email protected]> writes: > > > From: Heba Waly <[email protected]> > > > > Currently it's very easy for the advice library's callers to miss > > checking the visibility step before printing an advice. Also, it makes > > more sense for this step to be handled by the advice library. > > > > Add a new advise_if_enabled function that checks the visibility of > > advice messages before printing. > > > > Add a new helper advise_enabled to check the visibility of the advice > > if the caller needs to carry out complicated processing based on that > > value. > > > > A list of config variables 'advice_config_keys' is added to be used by > > list_config_advices() instead of 'advice_config[]' because we'll get > > rid of 'advice_config[]' and the global variables once we migrate all > > the callers to use the new APIs. > > > > > > Also change the advise call in tag library from advise() to > > advise_if_enabled() to construct an example of the usage of the new > > API. > > This is for step [3/3], isn't it? I'll discard this paragraph. Yes, should have been discarded. > > > > Signed-off-by: Heba Waly <[email protected]> > > --- > > Makefile | 1 + > > advice.c | 86 ++++++++++++++++++++++++++++++++++++++++-- > > advice.h | 52 +++++++++++++++++++++++++ > > t/helper/test-advise.c | 19 ++++++++++ > > t/helper/test-tool.c | 1 + > > t/helper/test-tool.h | 1 + > > t/t0018-advice.sh | 32 ++++++++++++++++ > > 7 files changed, 188 insertions(+), 4 deletions(-) > > create mode 100644 t/helper/test-advise.c > > create mode 100755 t/t0018-advice.sh > > > > diff --git a/Makefile b/Makefile > > index 09f98b777ca..ed923a3e818 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -695,6 +695,7 @@ X = > > > > PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS)) > > > > +TEST_BUILTINS_OBJS += test-advise.o > > TEST_BUILTINS_OBJS += test-chmtime.o > > TEST_BUILTINS_OBJS += test-config.o > > TEST_BUILTINS_OBJS += test-ctype.o > > diff --git a/advice.c b/advice.c > > index fd836332dad..5c2068b8f8a 100644 > > --- a/advice.c > > +++ b/advice.c > > @@ -96,13 +96,56 @@ static struct { > > { "pushNonFastForward", &advice_push_update_rejected } > > }; > > > > -static void vadvise(const char *advice, va_list params) > > +static const char *advice_config_keys[] = { > > + [ADD_EMBEDDED_REPO] = "addEmbeddedRepo", > > + [AMWORKDIR] = "amWorkDir", > > + [CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = "checkoutAmbiguousRemoteBranchName", > > + [COMMIT_BEFORE_MERGE] = "commitBeforeMerge", > > + [DETACHED_HEAD] = "detachedHead", > > + [FETCH_SHOW_FORCED_UPDATES] = "fetchShowForcedUpdates", > > + [GRAFT_FILE_DEPRECATED] = "graftFileDeprecated", > > + [IGNORED_HOOK] = "ignoredHook", > > + [IMPLICIT_IDENTITY] = "implicitIdentity", > > + [NESTED_TAG] = "nestedTag", > > + [OBJECT_NAME_WARNING] = "objectNameWarning", > > + [PUSH_ALREADY_EXISTS] = "pushAlreadyExists", > > + [PUSH_FETCH_FIRST] = "pushFetchFirst", > > + [PUSH_NEEDS_FORCE] = "pushNeedsForce", > > + > > + /* make this an alias for backward compatibility */ > > + [PUSH_UPDATE_REJECTED_ALIAS] = "pushNonFastForward", > > + > > + [PUSH_NON_FF_CURRENT] = "pushNonFFCurrent", > > + [PUSH_NON_FF_MATCHING] = "pushNonFFMatching", > > + [PUSH_UNQUALIFIED_REF_NAME] = "pushUnqualifiedRefName", > > + [PUSH_UPDATE_REJECTED] = "pushUpdateRejected", > > + [RESET_QUIET_WARNING] = "resetQuiet", > > + [RESOLVE_CONFLICT] = "resolveConflict", > > + [RM_HINTS] = "rmHints", > > + [SEQUENCER_IN_USE] = "sequencerInUse", > > + [SET_UPSTREAM_FAILURE] = "setupStreamFailure", > > + [STATUS_AHEAD_BEHIND_WARNING] = "statusAheadBehindWarning", > > + [STATUS_HINTS] = "statusHints", > > + [STATUS_U_OPTION] = "statusUoption", > > + [SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = "submoduleAlternateErrorStrategyDie", > > + [WAITING_FOR_EDITOR] = "waitingForEditor", > > +}; > > + > > +static const char turn_off_instructions[] = > > +N_("\n" > > + "Disable this message with \"git config %s false\""); > > + > > +static void vadvise(const char *advice, int display_instructions, > > + char *key, va_list params) > > { > > struct strbuf buf = STRBUF_INIT; > > const char *cp, *np; > > > > strbuf_vaddf(&buf, advice, params); > > > > + if (display_instructions) > > + strbuf_addf(&buf, turn_off_instructions, key); > > + > > for (cp = buf.buf; *cp; cp = np) { > > np = strchrnul(cp, '\n'); > > fprintf(stderr, _("%shint: %.*s%s\n"), > > @@ -119,8 +162,43 @@ void advise(const char *advice, ...) > > { > > va_list params; > > va_start(params, advice); > > - vadvise(advice, params); > > + vadvise(advice, 0, "", params); > > + va_end(params); > > +} > > + > > +static int get_config_value(enum advice_type type) > > +{ > > + int value = 1; > > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > > + > > + git_config_get_bool(key, &value); > > + free(key); > > + return value; > > +} > > So, in this hypothetical but quite realistic example: > > if (advice_enabled(ADVICE_FOO)) { > char *foo = expensive_preparation(); > advice_if_enabled(ADVICE_FOO, "use of %s is discouraged", foo); > } > > we end up formulating the "advice.*" key twice and ask git_config_get_bool() > about the same key twice? No, in the above example, advise() should be called not advise_if_enabled(). As we discussed in the beginning of this thread. https://public-inbox.org/git/[email protected]/ > > > +int advice_enabled(enum advice_type type) > > +{ > > + switch (type) { > > + case PUSH_UPDATE_REJECTED: > > + return get_config_value(PUSH_UPDATE_REJECTED) && > > + get_config_value(PUSH_UPDATE_REJECTED_ALIAS); > > + default: > > + return get_config_value(type); > > + } > > +} > > Also, as "enum advice_type" will be part of the public API, and > there is little type checking for enums, we shouldn't be naming them > randomly like these---we'd at least want to use a common prefix, > like "ADVICE_", in front of them. Those who are focused only on > advice subsystem may feel that names like PUSH_UPDATE_REJECTED are > sufficiently clear, but within the context of the whole system, > there is no cue that these UPCASED_WORDS identifiers belong to the > advice subsystem or somewhere else. > I agree. > > +void advise_if_enabled(enum advice_type type, const char *advice, ...) > > +{ > > + char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]); > > + va_list params; > > + > > + if (!advice_enabled(type)) > > + return; > > Oh, no, make the number of calls to xstrfmr() three times, not > twice, as I said in the previous example. > > I wonder if it would make the implementation better to do these: > > - Rename advice_config_keys[] to advice_setting[] that does not > imply it is only about the keys; > > - This table will know, for each enum advice_type, which > configuration variable enables it, *and* if it is enabled. > > i.e. > > static struct { > const char *config_key; > int disabled; > } advice_setting[] = { > [ADVICE_ADD_EMBEDED_REPO] = { "addEmbeddedRepo" }, > [ADVICE_AM_WORK_DIR] = { "amWorkDir" }, > ... > [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" }, > }; > > > Side Note: you have AMWORKDIR that is unreadable. If the config > name uses camelCase by convention, the UPCASED_WORDS > should be separated with underscore at the same word > boundary. I followed the original global variable name, which is `advice_amworkdir`, we can change that. > > Then, upon the first call to advice_enabled(), call git_config() > with a callback like > > static int populate_advice_settings(const char *var, const char *value, void *cb) > { > int advice_type; > const char *name; > > if (!skip_prefix(var, "advice.", &name)) > return 0; > advice_type = find_advice_type_by_name(advice_setting, name); > if (advice_type < 0) > return 0; /* unknown advice.* variable */ > /* advice.foo=false means advice.foo is disabled */ > advice_setting[advice_type].disabled = !git_config_bool(var, value); > } > > only once. Your get_config_value() would then become a mere lookup > in advice_setting[] array, e.g. > > int advice_enabled(unsigned advice_type) > { > static int initialized; > > if (!initialized) { > initialized = 1; > git_config(populate_advice_settings, NULL); > } > if (ARRAY_SIZE(advice_setting) <= advice_type) > BUG("OOB advice type requested???"); > return !advice_setting[advice_type].disabled; > } > > with your "push-update-rejected has two names" twist added. > > Hmm? I wasn't very happy about having to keep the list of config keys in memory, but that was a good enough solution for now. I also agree that there could be benefits for caching the values, as you mentioned it will be less expensive than looking up from the hashmap, but this array will grow with every new advice added to the system. And this data is already loaded in the hashmap, so we are duplicating it. So are the benefits worth the duplication? I don't know. Thanks, Heba

On the Git mailing list, Heba Waly wrote (reply to this):

On Wed, Feb 26, 2020 at 11:02 AM Junio C Hamano <[email protected]> wrote: > > Heba Waly <[email protected]> writes: > > > I wasn't very happy about having to keep the list of config keys in > > memory, but that was a good enough solution for now. > > If you force your programmers to specify the advice_type as a small > integer, and the setting is stored in the config as string keys, > somebody MUST have a table to convert from one to the other. So I > am not sure if it is even sensible to feel unhappy about having to > have a list in the first place. Are we looking for some kind of > miracles ;-)? > The reason I had to add the list of keys wasn't the enums, but because it's needed by list_config_advices() which returns all the advice config variables names. This is used when the user runs `git help --config`. And as a result, I added the enum to utilize it in accessing the list and avoid hard coded strings in functions calls. > On the other hand, it does bother my sense of aesthetics a lot it > our code forces our programmers to give a small integer to us, only > so that we convert that integer to a string and use the string to > look up a value in a hashtable, every time the program wants a > lookup. Performance-wise, that's not a huge downside. It just rubs > my sense of code hygiene the wrong way. > > Especially when the primary way for our programmers to specify which > advice they are talking about is by passing an integer, and if we > need to have a table indexed by that integer in the program anyway. > > We could instead do something like: > > /* advice.h */ > #ifndef _ADVICE_H_ > #define _ADVICE_H_ 1 > extern const char ADVICE_ADD_EMBEDDED_REPO[]; > extern const char ADVICE_AM_WORK_DIR[]; > ... > #endif > > /* advice.c */ > const char ADVICE_ADD_EMBEDDED_REPO[] = "advice.addEmbeddedRepo"; > const char ADVICE_ADD_AM_WORK_DIR[] = "advice.amWorkDir"; > ... > > and the callers can still do > > advise_if_enabled(ADVICE_NESTED_TAG, > _(message_advice_nested_tag), tag, object_ref); > > with the benefit of compiler catching a silly typo, without having > to have any "enum-to-string" table while letting the config API > layer do any caching transparently. As these calls will never be > placed in a performance critical codepath, that might be more > appropriate. > I'm not against this approach as well, but as I mentioned above, we need a list of keys to be returned by list_config_advices(), that's why defining the constant strings will not be sufficient in our case. Thanks, Heba

On the Git mailing list, Junio C Hamano wrote (reply to this):

Heba Waly <[email protected]> writes: > I'm not against this approach as well, but as I mentioned above, we > need a list of keys to be returned by list_config_advices(), that's > why defining the constant strings will not be sufficient in our case. Sorry, but I do not get it. Either you use enum or a bunch of variables of type const char [], "list all of them" would need an array whose elements are all of them, so const char ADVICE_FOO[] = "advice.foo"; const char ADVICE_BAR[] = "advice.bar"; ... static const char *all_advice_type[] = { ADVICE_FOO, ADVICE_BAR, ... }; void for_each_advice_type(int (*fn)(const char *name)) { int i; for (i = 0; i < ARRAY_SIZE(all_advice_type); i++) fn(all_advice_type[i]); } would be sufficient, and I do not think it takes any more effort to create and manage than using an array indexed with the enum, no?

On the Git mailing list, Heba Waly wrote (reply to this):

On Wed, Feb 26, 2020 at 4:03 PM Junio C Hamano <[email protected]> wrote: > > Heba Waly <[email protected]> writes: > > > I'm not against this approach as well, but as I mentioned above, we > > need a list of keys to be returned by list_config_advices(), that's > > why defining the constant strings will not be sufficient in our case. > > Sorry, but I do not get it. > > Either you use enum or a bunch of variables of type const char [], > "list all of them" would need an array whose elements are all of > them, so > > const char ADVICE_FOO[] = "advice.foo"; > const char ADVICE_BAR[] = "advice.bar"; > ... > > static const char *all_advice_type[] = { > ADVICE_FOO, ADVICE_BAR, ... > }; > > void for_each_advice_type(int (*fn)(const char *name)) > { > int i; > for (i = 0; i < ARRAY_SIZE(all_advice_type); i++) > fn(all_advice_type[i]); > } > > would be sufficient, and I do not think it takes any more effort to > create and manage than using an array indexed with the enum, no? > hmm, you're right, I just personally prefer having related variables collected in one data structure (whenever possible) like a list (or enum in this case) rather than defining each independently as a const variable. On the other hand, I understand that you'd prefer to skip the extra step of converting the enum to string. hmmm ok, I'll change the enum and send a new version soon. Thanks, Heba

On the Git mailing list, Junio C Hamano wrote (reply to this):

Heba Waly <[email protected]> writes: > variable. On the other hand, I understand that you'd prefer to skip > the extra step of converting the enum to string. > hmmm ok, I'll change the enum and send a new version soon. To avoid misunderstanding, I do not object to enum based approach at all. In fact, I'd rather prefer it over bunch of const strings that can be checked by the compiler. What I do not prefer compared to either of these approaches is to accept enum from the caller and convert it to string to consult config API every time, which is worse than "bunch of const strings". Thanks.

On the Git mailing list, Jonathan Tan wrote (reply to this):

> Heba Waly <[email protected]> writes: > > > I'm not against this approach as well, but as I mentioned above, we > > need a list of keys to be returned by list_config_advices(), that's > > why defining the constant strings will not be sufficient in our case. > > Sorry, but I do not get it. > > Either you use enum or a bunch of variables of type const char [], > "list all of them" would need an array whose elements are all of > them, so > > const char ADVICE_FOO[] = "advice.foo"; > const char ADVICE_BAR[] = "advice.bar"; > ... > > static const char *all_advice_type[] = { > ADVICE_FOO, ADVICE_BAR, ... > }; > > void for_each_advice_type(int (*fn)(const char *name)) > { > int i; > for (i = 0; i < ARRAY_SIZE(all_advice_type); i++) > fn(all_advice_type[i]); > } > > would be sufficient, and I do not think it takes any more effort to > create and manage than using an array indexed with the enum, no? With the enum: (.h) enum advice_type { ADVICE_FOO, ADVICE_BAR }; (.c) static const char *advice_config_keys[] = { [ADVICE_FOO] = "advice.foo", [ADVICE_BAR] = "advice.bar" }; /* No need for all_advice_type because we can loop over advice_config_keys */ With the bunch of variables of type const char []: (.h) extern const char ADVICE_FOO[]; extern const char ADVICE_BAR[]; (.c) const char ADVICE_FOO[] = "advice.foo"; const char ADVICE_BAR[] = "advice.bar"; static const char *all_advice_type[] = { ADVICE_FOO, ADVICE_BAR }; Junio, is this what you meant? It seems to me that there is an extra array to be managed in the latter case. Admittedly, this is a tradeoff against needing to convert the enum to a string when checking config, as you describe [1]. [1] https://lore.kernel.org/git/[email protected]/

gitgitgadget · 2020-02-27T20:50:12Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Heba Waly via GitGitGadget" <[email protected]> writes: > diff --git a/advice.c b/advice.c > index 258cc9ba7af..8d9f2910663 100644 > --- a/advice.c > +++ b/advice.c > @@ -32,6 +32,40 @@ int advice_checkout_ambiguous_remote_branch_name = 1; > int advice_nested_tag = 1; > int advice_submodule_alternate_error_strategy_die = 1; > > +const char ADVICE_ADD_EMBEDDED_REPO[] = "advice.addEmbeddedRepo"; > +const char ADVICE_AM_WORK_DIR[] = "advice.amWorkDir"; > ... > +static const char *advice_config_keys[] = { > + ADVICE_ADD_EMBEDDED_REPO, > + ADVICE_AM_WORK_DIR, > ... > +/* > + * To add a new advice, you need to: > + * Define a new const char array. > + * Add a new entry to advice_config_keys list. > + * Add the new config variable to Documentation/config/advice.txt. > + * Call advise_if_enabled to print your advice. > + */ > +extern const char ADVICE_ADD_EMBEDDED_REPO[]; > +extern const char ADVICE_AM_WORK_DIR[]; > ... Hmph. Even though I said that I would prefer it over the current one, in that it allows the compilers to catch typo, and over the one in v5 which uses enum, in that we do not have to go through enum->string->hash conversion all the time, I have to say that I am not very happy that we'd need to make a consistent change to three separate places. What's the ideal long-term outcome? The reason why I suggested during the v5 review an array of structure, a field in which can be the .disabled field, was because it would allow us to later extend the struct to help users. Wouldn't it be nice if we can do something like: $ git advice --list "^fetch" fetchShowForcedUpdates enabled $ git advice --list --verbose fetchShowForcedUpdates fetchShowForcedUpdates enabled "git fetch" by default spends cycles to compute which branches were forcibly updated, but it can be turned off. To avoid mistaken sense of safety, however, a reminder message is issued instead when it is turned off. The reminder can be turned off with this advice key. Such a future enhancement will become possible by assiciating a help text for each advice key, and the data structure introduced in <[email protected]> was meant to be the beginning of it. I dunno.

On the Git mailing list, Heba Waly wrote (reply to this):

On Fri, Feb 28, 2020 at 9:49 AM Junio C Hamano <[email protected]> wrote: > > "Heba Waly via GitGitGadget" <[email protected]> writes: > > > diff --git a/advice.c b/advice.c > > index 258cc9ba7af..8d9f2910663 100644 > > --- a/advice.c > > +++ b/advice.c > > @@ -32,6 +32,40 @@ int advice_checkout_ambiguous_remote_branch_name = 1; > > int advice_nested_tag = 1; > > int advice_submodule_alternate_error_strategy_die = 1; > > > > +const char ADVICE_ADD_EMBEDDED_REPO[] = "advice.addEmbeddedRepo"; > > +const char ADVICE_AM_WORK_DIR[] = "advice.amWorkDir"; > > ... > > +static const char *advice_config_keys[] = { > > + ADVICE_ADD_EMBEDDED_REPO, > > + ADVICE_AM_WORK_DIR, > > ... > > +/* > > + * To add a new advice, you need to: > > + * Define a new const char array. > > + * Add a new entry to advice_config_keys list. > > + * Add the new config variable to Documentation/config/advice.txt. > > + * Call advise_if_enabled to print your advice. > > + */ > > +extern const char ADVICE_ADD_EMBEDDED_REPO[]; > > +extern const char ADVICE_AM_WORK_DIR[]; > > ... > > Hmph. > > Even though I said that I would prefer it over the current one, > in that it allows the compilers to catch typo, and over the one > in v5 which uses enum, in that we do not have to go through > enum->string->hash conversion all the time, I have to say that I > am not very happy that we'd need to make a consistent change to > three separate places. > Unfortunately we'll have to make changes to three separate places until we implement a way to generate the documentation from the main list or vice versa, which we agreed in v2 not to do that now. https://lore.kernel.org/git/[email protected]/ > What's the ideal long-term outcome? The reason why I suggested > during the v5 review an array of structure, a field in which can > be the .disabled field, was because it would allow us to later > extend the struct to help users. Wouldn't it be nice if we can > do something like: > > $ git advice --list "^fetch" > fetchShowForcedUpdates enabled > $ git advice --list --verbose fetchShowForcedUpdates > fetchShowForcedUpdates enabled > "git fetch" by default spends cycles to compute which > branches were forcibly updated, but it can be turned off. > To avoid mistaken sense of safety, however, a reminder > message is issued instead when it is turned off. The > reminder can be turned off with this advice key. > > Such a future enhancement will become possible by assiciating a > help text for each advice key, and the data structure introduced > in <[email protected]> was meant to be > the beginning of it. Ok, you convinced me, will send a new version. Thanks, Heba

gitgitgadget · 2020-03-02T21:06:17Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Heba Waly via GitGitGadget" <[email protected]> writes: > +static struct { > + const char *key; > + int enabled; > +} advice_setting[] = { > + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 }, It would be nicer to future developers to flip the polarity, as we do not have to write 1 all over the place, especially if we plan to extend the structure over time and to use designated initializers for only certain fields: static struct { const char *key; int disabled; } advice_setting[] = { [ADDVICE_ADD_EMBEDDED_REPO] = { .key = "addEmbeddedRepo" }, > @@ -149,6 +218,13 @@ int git_default_advice_config(const char *var, const char *value) > if (strcasecmp(k, advice_config[i].name)) > continue; > *advice_config[i].preference = git_config_bool(var, value); > + break; > + } > + > + for (i = 0; i < ARRAY_SIZE(advice_setting); i++) { > + if (strcasecmp(k, advice_setting[i].key)) > + continue; > + advice_setting[i].enabled = git_config_bool(var, value); > return 0; Turning this into "break;" would make it similar to the loop before this one, and will allow other people to add more code after this loop later. > +int cmd__advise_if_enabled(int argc, const char **argv) > +{ > + if (!argv[1]) > + die("usage: %s <advice>", argv[0]); > + > + setup_git_directory(); > + git_config(git_default_config, NULL); > + > + /* > + Any advice type can be used for testing, but NESTED_TAG was selected > + here and in t0018 where this command is being executed. > + */ Style (will fix up locally). Thanks. I think this is reasonable with or without the suggested fixes.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <[email protected]> writes: > "Heba Waly via GitGitGadget" <[email protected]> writes: > >> +static struct { >> + const char *key; >> + int enabled; >> +} advice_setting[] = { >> + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 }, > > It would be nicer to future developers to flip the polarity, as we > do not have to write 1 all over the place, especially if we plan to > extend the structure over time and to use designated initializers > for only certain fields... Just to avoid needless churn, I think this does not matter in the longer term, so .enabled is OK as-is. The reason I say so is because, even though renaming to .disabled to allow initializers to default it to 0 is nicer for those who write the initializers manually, and it especially is true when we have more fields in the struct (we may add descriptive text so that we can issue an on-line help, for example), but I expect that would happen much later than we start generating these parts of the source code in two places (the initializer for advice_setting[] and the advice_type enum) from a single source by mechanical process. And the auto-generation will eliminate the burden of writing 1 manually.

On the Git mailing list, Heba Waly wrote (reply to this):

On Wed, Mar 4, 2020 at 3:16 AM Junio C Hamano <[email protected]> wrote: > > Junio C Hamano <[email protected]> writes: > > > "Heba Waly via GitGitGadget" <[email protected]> writes: > > > >> +static struct { > >> + const char *key; > >> + int enabled; > >> +} advice_setting[] = { > >> + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 }, > > > > It would be nicer to future developers to flip the polarity, as we > > do not have to write 1 all over the place, especially if we plan to > > extend the structure over time and to use designated initializers > > for only certain fields... > > Just to avoid needless churn, I think this does not matter in the > longer term, so .enabled is OK as-is. The reason I say so is > because, even though renaming to .disabled to allow initializers to > default it to 0 is nicer for those who write the initializers > manually, and it especially is true when we have more fields in the > struct (we may add descriptive text so that we can issue an on-line > help, for example), but I expect that would happen much later than > we start generating these parts of the source code in two places > (the initializer for advice_setting[] and the advice_type enum) from > a single source by mechanical process. And the auto-generation will > eliminate the burden of writing 1 manually. Agree. > Turning this into "break;" would make it similar to the loop before > this one, and will allow other people to add more code after this > loop later. The loop with break is redundant and will be removed in the next patch, but the rest of the loops in this function end with return which I think makes more sense assuming any new code/loop that will need to be added in the future is expected to handle a different configuration group. > > + /* > > + Any advice type can be used for testing, but NESTED_TAG was selected > > + here and in t0018 where this command is being executed. > > + */ > > Style (will fix up locally). > > Thanks. I think this is reasonable with or without the suggested > fixes. Great, thanks. Heba

gitgitgadget · 2020-02-20T01:44:59Z

On the Git mailing list, Emily Shaffer wrote (reply to this):

On Wed, Feb 19, 2020 at 08:34:01PM +0000, Heba Waly via GitGitGadget wrote: > From: Heba Waly <[email protected]> > > extract a version of advise() that uses an explict 'va_list' parameter. > Call it from advise() and advise_if_enabled() for a functionally > equivalent version. Hm, I'd put this patch before the advise_if_enabled() one, so that each commit makes sense by itself (rather than adding a bunch of code last patch only to remove it in this patch). > > Signed-off-by: Derrick Stolee <[email protected]> > Signed-off-by: Heba Waly <[email protected]> > --- > - for (cp = buf.buf; *cp; cp = np) { > - np = strchrnul(cp, '\n'); > - fprintf(stderr, _("%shint: %.*s%s\n"), > - advise_get_color(ADVICE_COLOR_HINT), > - (int)(np - cp), cp, > - advise_get_color(ADVICE_COLOR_RESET)); > - if (*np) > - np++; > - } I see - this hunk that I commented on in the other review is actually duplicated from advise(). Hm, I still think it'd be useful to put this functionality into strbuf, but I guess since it's not new code you're adding there's not a lot of need to sweat about it. - Emily

On the Git mailing list, Heba Waly wrote (reply to this):

On Thu, Feb 20, 2020 at 2:42 PM Emily Shaffer <[email protected]> wrote: > > On Wed, Feb 19, 2020 at 08:34:01PM +0000, Heba Waly via GitGitGadget wrote: > > From: Heba Waly <[email protected]> > > > > extract a version of advise() that uses an explict 'va_list' parameter. > > Call it from advise() and advise_if_enabled() for a functionally > > equivalent version. > > Hm, I'd put this patch before the advise_if_enabled() one, so that each > commit makes sense by itself (rather than adding a bunch of code last > patch only to remove it in this patch). > You're right, that was me avoiding the commits re-order conflicts but I'll give it a second try. > > > > Signed-off-by: Derrick Stolee <[email protected]> > > Signed-off-by: Heba Waly <[email protected]> > > --- > > - for (cp = buf.buf; *cp; cp = np) { > > - np = strchrnul(cp, '\n'); > > - fprintf(stderr, _("%shint: %.*s%s\n"), > > - advise_get_color(ADVICE_COLOR_HINT), > > - (int)(np - cp), cp, > > - advise_get_color(ADVICE_COLOR_RESET)); > > - if (*np) > > - np++; > > - } > > I see - this hunk that I commented on in the other review is actually > duplicated from advise(). Hm, I still think it'd be useful to put this > functionality into strbuf, but I guess since it's not new code you're > adding there's not a lot of need to sweat about it. > Agree. > - Emily Thank you Emily Heba

gitgitgadget · 2020-02-24T22:09:39Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Heba Waly via GitGitGadget" <[email protected]> writes: > From: Heba Waly <[email protected]> > > Following the new helpers added to the advice library, > replace the global variable check approach by the new > API calls > > Signed-off-by: Heba Waly <[email protected]> > --- > advice.c | 2 -- > advice.h | 1 - > builtin/tag.c | 5 +++-- > t/t7004-tag.sh | 1 + > 4 files changed, 4 insertions(+), 5 deletions(-) Nice.

gitgitgadget · 2020-02-24T23:48:50Z

On the Git mailing list, Emily Shaffer wrote (reply to this):

On Mon, Feb 24, 2020 at 03:13:18PM +0000, Heba Waly via GitGitGadget wrote: > From: Heba Waly <[email protected]> > > Following the new helpers added to the advice library, > replace the global variable check approach by the new > API calls > > Signed-off-by: Heba Waly <[email protected]> > --- > advice.c | 2 -- > advice.h | 1 - > builtin/tag.c | 5 +++-- > t/t7004-tag.sh | 1 + > 4 files changed, 4 insertions(+), 5 deletions(-) More deleted lines than added lines always makes me a little happier ;) Reviewed-by: Emily Shaffer <[email protected]>

gitgitgadget · 2020-02-25T19:10:12Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Heba Waly via GitGitGadget" <[email protected]> writes: > From: Heba Waly <[email protected]> > > Following the new helpers added to the advice library, > replace the global variable check approach by the new > API calls The last paragraph of the proposed log message you had for [2/3] described that this step is just an example better than the above one, which would leave readers puzzled what our plans are for dozens of existing advise() calls. > + if (type == OBJ_TAG) > + advise_if_enabled(NESTED_TAG, _(message_advice_nested_tag), > + tag, object_ref); This is probably a good enough example why OBJ_TAG is a good name but NESTED_TAG is not---type could be something different from OBJ_TAG but the other possiblities are all OBJ_<object-type>. We want the advice types to have the same property.

gitgitgadget · 2020-02-27T17:41:08Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Heba Waly via GitGitGadget" <[email protected]> writes: > From: Heba Waly <[email protected]> > > fb6fbffbda (advice: keep config name in camelCase in advice_config[], > 2018-05-26) changed the config names to camelCase, but one of the names > wasn't changed correctly. Fix it. > > Signed-off-by: Heba Waly <[email protected]> > --- > advice.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/advice.c b/advice.c > index fd836332dad..258cc9ba7af 100644 > --- a/advice.c > +++ b/advice.c > @@ -80,7 +80,7 @@ static struct { > { "sequencerInUse", &advice_sequencer_in_use }, > { "implicitIdentity", &advice_implicit_identity }, > { "detachedHead", &advice_detached_head }, > - { "setupStreamFailure", &advice_set_upstream_failure }, > + { "setUpstreamFailure", &advice_set_upstream_failure }, The mistake is sort-of understandable, when "setup" is taken as a verb. Well spotted ;-) > { "objectNameWarning", &advice_object_name_warning }, > { "amWorkDir", &advice_amworkdir }, > { "rmHints", &advice_rm_hints },

gitgitgadget · 2020-02-24T22:06:38Z

On the Git mailing list, Emily Shaffer wrote (reply to this):

On Mon, Feb 24, 2020 at 03:13:16PM +0000, Heba Waly via GitGitGadget wrote: > From: Heba Waly <[email protected]> > > In preparation for a new advice method, extract a version of advise() > that uses an explict 'va_list' parameter. Call it from advise() for a > functionally equivalent version. > > Signed-off-by: Derrick Stolee <[email protected]> > Signed-off-by: Heba Waly <[email protected]> This seems very straightforward and now appears to be in the right commit order. Reviewed-by: Emily Shaffer <[email protected]> > --- > advice.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/advice.c b/advice.c > index 249c60dcf32..fd836332dad 100644 > --- a/advice.c > +++ b/advice.c > @@ -96,15 +96,12 @@ static struct { > { "pushNonFastForward", &advice_push_update_rejected } > }; > > -void advise(const char *advice, ...) > +static void vadvise(const char *advice, va_list params) > { > struct strbuf buf = STRBUF_INIT; > - va_list params; > const char *cp, *np; > > - va_start(params, advice); > strbuf_vaddf(&buf, advice, params); > - va_end(params); > > for (cp = buf.buf; *cp; cp = np) { > np = strchrnul(cp, '\n'); > @@ -118,6 +115,14 @@ void advise(const char *advice, ...) > strbuf_release(&buf); > } > > +void advise(const char *advice, ...) > +{ > + va_list params; > + va_start(params, advice); > + vadvise(advice, params); > + va_end(params); > +} > + > int git_default_advice_config(const char *var, const char *value) > { > const char *k, *slot_name; > -- > gitgitgadget >

-Original file line number
+Diff line change
@@ Expand Up / @@ -29,12 +29,62 @@ extern int advice_ignored_hook; @@
     extern int advice_waiting_for_editor;
     extern int advice_graft_file_deprecated;
     extern int advice_checkout_ambiguous_remote_branch_name;
-    extern int advice_nested_tag;
     extern int advice_submodule_alternate_error_strategy_die;
+    /*
+     * To add a new advice, you need to:
+     * Define a new advice_type.
+     * Add a new entry to advice_setting array.
+     * Add the new config variable to Documentation/config/advice.txt.
+     * Call advise_if_enabled to print your advice.
+     */
+     enum advice_type {
+    	ADVICE_ADD_EMBEDDED_REPO,
+    	ADVICE_AM_WORK_DIR,
+    	ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
+    	ADVICE_COMMIT_BEFORE_MERGE,
+    	ADVICE_DETACHED_HEAD,
+    	ADVICE_FETCH_SHOW_FORCED_UPDATES,
+    	ADVICE_GRAFT_FILE_DEPRECATED,
+    	ADVICE_IGNORED_HOOK,
+    	ADVICE_IMPLICIT_IDENTITY,
+    	ADVICE_NESTED_TAG,
+    	ADVICE_OBJECT_NAME_WARNING,
+    	ADVICE_PUSH_ALREADY_EXISTS,
+    	ADVICE_PUSH_FETCH_FIRST,
+    	ADVICE_PUSH_NEEDS_FORCE,
+    	ADVICE_PUSH_NON_FF_CURRENT,
+    	ADVICE_PUSH_NON_FF_MATCHING,
+    	ADVICE_PUSH_UNQUALIFIED_REF_NAME,
+    	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
+    	ADVICE_PUSH_UPDATE_REJECTED,
+    	ADVICE_RESET_QUIET_WARNING,
+    	ADVICE_RESOLVE_CONFLICT,
+    	ADVICE_RM_HINTS,
+    	ADVICE_SEQUENCER_IN_USE,
+    	ADVICE_SET_UPSTREAM_FAILURE,
+    	ADVICE_STATUS_AHEAD_BEHIND_WARNING,
+    	ADVICE_STATUS_HINTS,
+    	ADVICE_STATUS_U_OPTION,
+    	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
+    	ADVICE_WAITING_FOR_EDITOR,
+    };
     int git_default_advice_config(const char *var, const char *value);
     __attribute__((format (printf, 1, 2)))
     void advise(const char *advice, ...);
+    /**
+     * Checks if advice type is enabled (can be printed to the user).
+     * Should be called before advise().
+     */
+    int advice_enabled(enum advice_type type);
+    /**
+     * Checks the visibility of the advice before printing.
+     */
+    void advise_if_enabled(enum advice_type type, const char *advice, ...);
     int error_resolve_conflict(const char *me);
     void NORETURN die_resolve_conflict(const char *me);
     void NORETURN die_conclude_merge(void);
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -695,6 +695,7 @@ X = @@
     PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
+    TEST_BUILTINS_OBJS += test-advise.o
     TEST_BUILTINS_OBJS += test-chmtime.o
     TEST_BUILTINS_OBJS += test-config.o
     TEST_BUILTINS_OBJS += test-ctype.o
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -29,7 +29,6 @@ int advice_ignored_hook = 1; @@
     int advice_waiting_for_editor = 1;
     int advice_graft_file_deprecated = 1;
     int advice_checkout_ambiguous_remote_branch_name = 1;
-    int advice_nested_tag = 1;
     int advice_submodule_alternate_error_strategy_die = 1;
     static int advice_use_color = -1;
@@ Expand Down Expand Up / @@ -80,7 +79,7 @@ static struct { @@
     	{ "sequencerInUse", &advice_sequencer_in_use },
     	{ "implicitIdentity", &advice_implicit_identity },
     	{ "detachedHead", &advice_detached_head },
-    	{ "setupStreamFailure", &advice_set_upstream_failure },
+    	{ "setUpstreamFailure", &advice_set_upstream_failure },
     	{ "objectNameWarning", &advice_object_name_warning },
     	{ "amWorkDir", &advice_amworkdir },
     	{ "rmHints", &advice_rm_hints },
@@ Expand All / @@ -89,22 +88,64 @@ static struct { @@
     	{ "waitingForEditor", &advice_waiting_for_editor },
     	{ "graftFileDeprecated", &advice_graft_file_deprecated },
     	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
-    	{ "nestedTag", &advice_nested_tag },
     	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
     	/* make this an alias for backward compatibility */
     	{ "pushNonFastForward", &advice_push_update_rejected }
     };
-    void advise(const char *advice, ...)
+    static struct {
+    	const char *key;
+    	int enabled;
+    } advice_setting[] = {
+    	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo", 1 },
+    	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir", 1 },
+    	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName", 1 },
+    	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge", 1 },
+    	[ADVICE_DETACHED_HEAD]				= { "detachedHead", 1 },
+    	[ADVICE_FETCH_SHOW_FORCED_UPDATES]		= { "fetchShowForcedUpdates", 1 },
+    	[ADVICE_GRAFT_FILE_DEPRECATED]			= { "graftFileDeprecated", 1 },
+    	[ADVICE_IGNORED_HOOK]				= { "ignoredHook", 1 },
+    	[ADVICE_IMPLICIT_IDENTITY]			= { "implicitIdentity", 1 },
+    	[ADVICE_NESTED_TAG]				= { "nestedTag", 1 },
+    	[ADVICE_OBJECT_NAME_WARNING]			= { "objectNameWarning", 1 },
+    	[ADVICE_PUSH_ALREADY_EXISTS]			= { "pushAlreadyExists", 1 },
+    	[ADVICE_PUSH_FETCH_FIRST]			= { "pushFetchFirst", 1 },
+    	[ADVICE_PUSH_NEEDS_FORCE]			= { "pushNeedsForce", 1 },
+    	/* make this an alias for backward compatibility */
+    	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward", 1 },
+    	[ADVICE_PUSH_NON_FF_CURRENT]			= { "pushNonFFCurrent", 1 },
+    	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching", 1 },
+    	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName", 1 },
+    	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected", 1 },
+    	[ADVICE_RESET_QUIET_WARNING]			= { "resetQuiet", 1 },
+    	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict", 1 },
+    	[ADVICE_RM_HINTS]				= { "rmHints", 1 },
+    	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse", 1 },
+    	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure", 1 },
+    	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning", 1 },
+    	[ADVICE_STATUS_HINTS]				= { "statusHints", 1 },
+    	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
+    	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
+    	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
+    };
+    static const char turn_off_instructions[] =
+    N_("\n"
+       "Disable this message with \"git config advice.%s false\"");
+    static void vadvise(const char *advice, int display_instructions,
+    		    const char *key, va_list params)
     {
     	struct strbuf buf = STRBUF_INIT;
-    	va_list params;
     	const char *cp, *np;
-    	va_start(params, advice);
     	strbuf_vaddf(&buf, advice, params);
-    	va_end(params);
+    	if (display_instructions)
+    		strbuf_addf(&buf, turn_off_instructions, key);
     	for (cp = buf.buf; *cp; cp = np) {
     		np = strchrnul(cp, '\n');
@@ Expand All / @@ -118,6 +159,37 @@ void advise(const char *advice, ...) @@
     	strbuf_release(&buf);
     }
+    void advise(const char *advice, ...)
+    {
+    	va_list params;
+    	va_start(params, advice);
+    	vadvise(advice, 0, "", params);
+    	va_end(params);
+    }
+    int advice_enabled(enum advice_type type)
+    {
+    	switch(type) {
+    	case ADVICE_PUSH_UPDATE_REJECTED:
+    		return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
+    		       advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
+    	default:
+    		return advice_setting[type].enabled;
+    	}
+    }
+    void advise_if_enabled(enum advice_type type, const char *advice, ...)
+    {
+    	va_list params;
+    	if (!advice_enabled(type))
+    		return;
+    	va_start(params, advice);
+    	vadvise(advice, 1, advice_setting[type].key, params);
+    	va_end(params);
+    }
     int git_default_advice_config(const char *var, const char *value)
     {
     	const char *k, *slot_name;
@@ Expand All @@
     		if (strcasecmp(k, advice_config[i].name))
     			continue;
     		*advice_config[i].preference = git_config_bool(var, value);
+    		break;
+    	}
+    	for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
+    		if (strcasecmp(k, advice_setting[i].key))
+    			continue;
+    		advice_setting[i].enabled = git_config_bool(var, value);
     		return 0;
     	}
@@ Expand All @@
     {
     	int i;
-    	for (i = 0; i < ARRAY_SIZE(advice_config); i++)
-    		list_config_item(list, prefix, advice_config[i].name);
+    	for (i = 0; i < ARRAY_SIZE(advice_setting); i++)
+    		list_config_item(list, prefix, advice_setting[i].key);
     }
     int error_resolve_conflict(const char *me)
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     	if (type <= OBJ_NONE)
     		die(_("bad object type."));
-    	if (type == OBJ_TAG && advice_nested_tag)
-    		advise(_(message_advice_nested_tag), tag, object_ref);
+    	if (type == OBJ_TAG)
+    		advise_if_enabled(ADVICE_NESTED_TAG, _(message_advice_nested_tag),
+    				  tag, object_ref);
     	strbuf_addf(&header,
     		    "object %s\n"
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -0,0 +1,21 @@
+    #include "test-tool.h"
+    #include "cache.h"
+    #include "advice.h"
+    #include "config.h"
+    int cmd__advise_if_enabled(int argc, const char **argv)
+    {
+    	if (!argv[1])
+    	die("usage: %s <advice>", argv[0]);
+    	setup_git_directory();
+    	git_config(git_default_config, NULL);
+    	/*
+    	  Any advice type can be used for testing, but NESTED_TAG was selected
+    	  here and in t0018 where this command is being executed.
+    	 */
+    	advise_if_enabled(ADVICE_NESTED_TAG, argv[1]);
+    	return 0;
+    }

-Original file line number
+Diff line change
@@ Expand Up / @@ -14,6 +14,7 @@ struct test_cmd { @@
     };
     static struct test_cmd cmds[] = {
+    	{ "advise", cmd__advise_if_enabled },
     	{ "chmtime", cmd__chmtime },
     	{ "config", cmd__config },
     	{ "ctype", cmd__ctype },
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -4,6 +4,7 @@ @@
     #define USE_THE_INDEX_COMPATIBILITY_MACROS
     #include "git-compat-util.h"
+    int cmd__advise_if_enabled(int argc, const char **argv);
     int cmd__chmtime(int argc, const char **argv);
     int cmd__config(int argc, const char **argv);
     int cmd__ctype(int argc, const char **argv);
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -0,0 +1,32 @@
+    #!/bin/sh
+    test_description='Test advise_if_enabled functionality'
+    . ./test-lib.sh
+    test_expect_success 'advice should be printed when config variable is unset' '
+    	cat >expect <<-\EOF &&
+    	hint: This is a piece of advice
+    	hint: Disable this message with "git config advice.nestedTag false"
+    	EOF
+    	test-tool advise "This is a piece of advice" 2>actual &&
+    	test_i18ncmp expect actual
+    '
+    test_expect_success 'advice should be printed when config variable is set to true' '
+    	cat >expect <<-\EOF &&
+    	hint: This is a piece of advice
+    	hint: Disable this message with "git config advice.nestedTag false"
+    	EOF
+    	test_config advice.nestedTag true &&
+    	test-tool advise "This is a piece of advice" 2>actual &&
+    	test_i18ncmp expect actual
+    '
+    test_expect_success 'advice should not be printed when config variable is set to false' '
+    	test_config advice.nestedTag false &&
+    	test-tool advise "This is a piece of advice" 2>actual &&
+    	test_must_be_empty actual
+    '
+    test_done

[Outreachy] advice: revamp advise API #548

Uh oh!

[Outreachy] advice: revamp advise API #548

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gitgitgadget bot Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Feb 29, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!