Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] advice: refactor advise API
@ 2020-02-10  5:04 Heba Waly via GitGitGadget
  2020-02-10 14:38 ` Derrick Stolee
                   ` (3 more replies)
  0 siblings, 4 replies; 24+ messages in thread
From: Heba Waly via GitGitGadget @ 2020-02-10  5:04 UTC (permalink / raw)
  To: git; +Cc: Heba Waly, Heba Waly

From: Heba Waly <heba.waly@gmail.com>

Add a new advise_ng function that can check the visibility of advice
messages before printing.

Currently it's very easy for the callers to miss checking the
visibility step. Also, it makes more sense for this step to be handled
by the advice library.

Also change the advise call in tag library from advise() to advise_ng()
to construct an example of the usage of the new API.

Signed-off-by: Heba Waly <heba.waly@gmail.com>
---
    [RFC][Outreachy] advice: refactor advise API
    
    The advice API is currently a little bit confusing to call. quoting from
    [1]:
    
    When introducing a new advice message, you would
    
     * come up with advice.frotz configuration variable
    
     * define and declare advice_frotz global variable that defaults to
       true
    
     * sprinkle calls like this:
    
      if (advice_frotz)
        advise(_("helpful message about frotz"));
    
    A new approach was suggested in [1] which this patch is based upon.
    
    A new advise_ng() is introduced to gradually replace advise()
    
    pros of the new advise():
    
     * The caller doesn't need to define a new global variable when
       introducing a new message.
     * The caller doesn't need to check the visibility of the message before
       calling advise_ng().
     * The caller still needs to come up with advice.frotz config variable
       and will call advice_ng as follows: advice_ng("advice.frotz",
       _("helpful message about frotz"));
    
    After this patch the plan is to migrate the rest of the advise calls to
    advise_ng and then finally remove advise() and rename advise_ng() to
    advise()
    
    [1] 
    https://public-inbox.org/git/xmqqzhf5cw69.fsf@gitster-ct.c.googlers.com/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-548%2FHebaWaly%2Fadvice_refactoring-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-548/HebaWaly/advice_refactoring-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/548

 Makefile               |  1 +
 advice.c               | 37 +++++++++++++++++++++++++++++++++++--
 advice.h               |  6 +++++-
 builtin/tag.c          |  4 ++--
 t/helper/test-advise.c | 15 +++++++++++++++
 t/helper/test-tool.c   |  1 +
 t/helper/test-tool.h   |  1 +
 t/t0018-advice.sh      | 29 +++++++++++++++++++++++++++++
 t/t7004-tag.sh         |  2 ++
 9 files changed, 91 insertions(+), 5 deletions(-)
 create mode 100644 t/helper/test-advise.c
 create mode 100755 t/t0018-advice.sh

diff --git a/Makefile b/Makefile
index 09f98b777c..ed923a3e81 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 249c60dcf3..4793e59223 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,7 +88,6 @@ 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 */
@@ -118,6 +116,41 @@ void advise(const char *advice, ...)
 	strbuf_release(&buf);
 }
 
+static const char turn_off_instructions[] =
+N_("\n"
+"Turn this message off by running\n"
+"\"git config %s false\"");
+
+void advise_ng(const char *key, const char *advice, ...)
+{
+	int value = 1;
+	struct strbuf buf = STRBUF_INIT;
+	va_list params;
+	const char *cp, *np;
+	
+	git_config_get_bool(key, &value);
+	
+	if(value)
+	{
+		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++;
+		}
+		strbuf_release(&buf);
+	}
+}
+
 int git_default_advice_config(const char *var, const char *value)
 {
 	const char *k, *slot_name;
diff --git a/advice.h b/advice.h
index b706780614..ad4da2d65d 100644
--- a/advice.h
+++ b/advice.h
@@ -29,12 +29,16 @@ 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;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
 void advise(const char *advice, ...);
+
+/**
+ Checks the visibility of the advice before priniting.
+ */
+void advise_ng(const char *key, 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 e0a4c25382..7fe1ff3ed0 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_ng("advice.nestedTag", _(message_advice_nested_tag), tag, object_ref);
 
 	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 0000000000..b6ec90fd18
--- /dev/null
+++ b/t/helper/test-advise.c
@@ -0,0 +1,15 @@
+#include "test-tool.h"
+#include "cache.h"
+#include "advice.h"
+
+int cmd__advise_ng(int argc, const char **argv)
+{
+	if (!argv[1] || !argv[2])
+	die("usage: %s <key> <advice>", argv[0]);
+
+	setup_git_directory();
+
+	advise_ng(argv[1], argv[2]);
+
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index f20989d449..f903f32bb6 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_ng },
 	{ "chmtime", cmd__chmtime },
 	{ "config", cmd__config },
 	{ "ctype", cmd__ctype },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8ed2af71d1..e5e955beb9 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_ng(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 0000000000..d012db515d
--- /dev/null
+++ b/t/t0018-advice.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='Test advise_ng functionality'
+
+. ./test-lib.sh
+
+cat > expected <<EOF
+hint: This is a piece of advice
+hint: Turn this message off by running
+hint: "git config advice.configVariable false"
+EOF
+test_expect_success 'advise should be printed when config variable is unset' '
+	test-tool advise "advice.configVariable" "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.configVariable true &&
+	test-tool advise "advice.configVariable" "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.configVariable false &&
+	test-tool advise "advice.configVariable" "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 6db92bd3ba..b7c8d41899 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1726,6 +1726,8 @@ 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: Turn this message off by running
+	hint: "git config advice.nestedTag false"
 	EOF
 	git tag -m nested nested annotated-v4.0 2>actual &&
 	test_i18ncmp expect actual

base-commit: c7a62075917b3340f908093f63f1161c44ed1475
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10  5:04 [PATCH] advice: refactor advise API Heba Waly via GitGitGadget
@ 2020-02-10 14:38 ` Derrick Stolee
  2020-02-10 19:30   ` Junio C Hamano
  2020-02-10 23:56   ` Heba Waly
  2020-02-10 20:37 ` Jeff King
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 24+ messages in thread
From: Derrick Stolee @ 2020-02-10 14:38 UTC (permalink / raw)
  To: Heba Waly via GitGitGadget, git; +Cc: Heba Waly

On 2/10/2020 12:04 AM, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
> 
> Add a new advise_ng function that can check the visibility of advice
> messages before printing.
>
> Currently it's very easy for the callers to miss checking the
> visibility step. Also, it makes more sense for this step to be handled
> by the advice library.

This makes the advice API much easier and its uses much cleaner. Thanks!
 
> Also change the advise call in tag library from advise() to advise_ng()
> to construct an example of the usage of the new API.

This is a good example case.

> +static const char turn_off_instructions[] =
> +N_("\n"
> +"Turn this message off by running\n"
> +"\"git config %s false\"");

I have mixed feelings on the use of these instructions. Perhaps at
minimum the addition of these instructions could be left to a
separate patch than the creation of advise_ng().

My biggest concern is that this adds unexpected noise to users who
want the advice to stay. I'm calling attention to it, because this
part isn't a simple refactor like the rest of the patch.

If it _does_ stay, then I recommend condensing the message to
a single line. For example:

	Disable this message with "git config %d false"

> +void advise_ng(const char *key, const char *advice, ...)
> +{
> +	int value = 1;
> +	struct strbuf buf = STRBUF_INIT;
> +	va_list params;
> +	const char *cp, *np;
> +	
> +	git_config_get_bool(key, &value);
> +	
> +	if(value)
> +	{

Style: spacing, and opening braces are on the same line as the if:

	if (value) {

But also, this method would be simpler if the opposite case was
an early return:

	if (!value)
		return;

Then the rest could have one less indentation.

> +		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++;
> +		}
> +		strbuf_release(&buf);

This loop looks like it was copied from advise(). Perhaps we could
re-use that code better by creating a new vadvise() method that
takes a va_list, and have advise() and advise_ng() call it instead?
I include a patch at the end of this method that does this conversion.
(Feel free to incorporate it into your next version, if you want, but
be sure to add your sign-off.) Then, your advise_ng() can call these:

	vadvise(advice, params);
	advise(turn_off_instructions, key);

removing the need to re-implement the for loop.

> diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
> new file mode 100644
> index 0000000000..b6ec90fd18
> --- /dev/null
> +++ b/t/helper/test-advise.c
> @@ -0,0 +1,15 @@
> +#include "test-tool.h"
> +#include "cache.h"
> +#include "advice.h"
> +
> +int cmd__advise_ng(int argc, const char **argv)
> +{
> +	if (!argv[1] || !argv[2])
> +	die("usage: %s <key> <advice>", argv[0]);
> +
> +	setup_git_directory();
> +
> +	advise_ng(argv[1], argv[2]);
> +
> +	return 0;
> +}

I definitely tend to recommend more tests than most, but perhaps this
unit test is overkill? You demonstrate a good test below using a real
Git command, which should be sufficient. If the "turn this message off"
part gets removed, then you will still have coverage of your method.
It just won't require a test change because it would not modify behavior.

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 6db92bd3ba..b7c8d41899 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1726,6 +1726,8 @@ 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: Turn this message off by running
> +	hint: "git config advice.nestedTag false"
>  	EOF
>  	git tag -m nested nested annotated-v4.0 2>actual &&
>  	test_i18ncmp expect actual
> 
> base-commit: c7a62075917b3340f908093f63f1161c44ed1475

Thanks,
-Stolee

-->8--

From: Derrick Stolee <dstolee@microsoft.com>
Date: Mon, 10 Feb 2020 09:33:20 -0500
Subject: [PATCH] advice: extract vadvise() from advise()

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 <dstolee@microsoft.com>
---
 advice.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/advice.c b/advice.c
index 249c60dcf3..fd836332da 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;
-- 
2.25.0.vfs.1.1.1.g9906319d24.dirty




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10 14:38 ` Derrick Stolee
@ 2020-02-10 19:30   ` Junio C Hamano
  2020-02-10 19:42     ` Taylor Blau
  2020-02-10 23:56   ` Heba Waly
  1 sibling, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2020-02-10 19:30 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: Heba Waly via GitGitGadget, git, Heba Waly

Derrick Stolee <stolee@gmail.com> writes:

>> +static const char turn_off_instructions[] =
>> +N_("\n"
>> +"Turn this message off by running\n"
>> +"\"git config %s false\"");
>
> I have mixed feelings on the use of these instructions. Perhaps at
> minimum the addition of these instructions could be left to a
> separate patch than the creation of advise_ng().
>
> My biggest concern is that this adds unexpected noise to users who
> want the advice to stay. I'm calling attention to it, because this
> part isn't a simple refactor like the rest of the patch.
> ...
> I definitely tend to recommend more tests than most, but perhaps this
> unit test is overkill? You demonstrate a good test below using a real
> Git command, which should be sufficient. If the "turn this message off"
> part gets removed, then you will still have coverage of your method.
> It just won't require a test change because it would not modify behavior.
> ...

All good suggestions.  Thanks for an excellent review.

Another thing.  

advise_ng() may have been a good name for illustration but is a
horrible name for real-world use (imagine we need to revamp the API
one more time in the future---what would it be called, which has to
say that it is newer than the "next generation"?
advise_3rd_try()?).

Thanks.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10 19:30   ` Junio C Hamano
@ 2020-02-10 19:42     ` Taylor Blau
  2020-02-10 22:29       ` Emily Shaffer
  2020-02-11  0:08       ` Heba Waly
  0 siblings, 2 replies; 24+ messages in thread
From: Taylor Blau @ 2020-02-10 19:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Derrick Stolee, Heba Waly via GitGitGadget, git, Heba Waly

On Mon, Feb 10, 2020 at 11:30:46AM -0800, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>
> >> +static const char turn_off_instructions[] =
> >> +N_("\n"
> >> +"Turn this message off by running\n"
> >> +"\"git config %s false\"");
> >
> > I have mixed feelings on the use of these instructions. Perhaps at
> > minimum the addition of these instructions could be left to a
> > separate patch than the creation of advise_ng().
> >
> > My biggest concern is that this adds unexpected noise to users who
> > want the advice to stay. I'm calling attention to it, because this
> > part isn't a simple refactor like the rest of the patch.
> > ...
> > I definitely tend to recommend more tests than most, but perhaps this
> > unit test is overkill? You demonstrate a good test below using a real
> > Git command, which should be sufficient. If the "turn this message off"
> > part gets removed, then you will still have coverage of your method.
> > It just won't require a test change because it would not modify behavior.
> > ...
>
> All good suggestions.  Thanks for an excellent review.
>
> Another thing.
>
> advise_ng() may have been a good name for illustration but is a
> horrible name for real-world use (imagine we need to revamp the API
> one more time in the future---what would it be called, which has to
> say that it is newer than the "next generation"?
> advise_3rd_try()?).

What about calling this new API 'advise()'? The first patch could call
it 'advise_ng' or whatever other temporary name we feel comfortable
using, and then each subsequent patch would update callers of 'advise()'
to use 'advise_ng()'. Once those patches have been applied, and no other
callers of 'advise()' exist, a final patch can be applied on top to
rename 'advise_ng()' to 'advise()', and update the names of all of the
callers.

This makes for a rather noisy final patch, but the intermediate states
are much clearer, and it would make this series rather self-contained.

On the other hand, having a version of 'advise_ng()' on master makes
this topic more incremental, meaning that we can pick it up and put it
down at ease and have more self-contained projects.

I don't really have a preference between the two approaches, but if we
go with the latter, I do think we need something better than
'advise_ng'. Maybe 'advise_warn'? I don't know.

> Thanks.

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10  5:04 [PATCH] advice: refactor advise API Heba Waly via GitGitGadget
  2020-02-10 14:38 ` Derrick Stolee
@ 2020-02-10 20:37 ` Jeff King
  2020-02-10 22:55   ` Emily Shaffer
  2020-02-11  2:20   ` Heba Waly
  2020-02-10 22:46 ` Junio C Hamano
  2020-02-16 21:39 ` [PATCH v2 0/2] [RFC][Outreachy] " Heba Waly via GitGitGadget
  3 siblings, 2 replies; 24+ messages in thread
From: Jeff King @ 2020-02-10 20:37 UTC (permalink / raw)
  To: Heba Waly via GitGitGadget; +Cc: git, Heba Waly

On Mon, Feb 10, 2020 at 05:04:09AM +0000, Heba Waly via GitGitGadget wrote:

>     The advice API is currently a little bit confusing to call. quoting from
>     [1]:
>     
>     When introducing a new advice message, you would
>     
>      * come up with advice.frotz configuration variable
>     
>      * define and declare advice_frotz global variable that defaults to
>        true
>     
>      * sprinkle calls like this:
>     
>       if (advice_frotz)
>         advise(_("helpful message about frotz"));
>     
>     A new approach was suggested in [1] which this patch is based upon.

I agree that the current procedure is a bit painful, and I think this is
a step in the right direction. But...

>     After this patch the plan is to migrate the rest of the advise calls to
>     advise_ng and then finally remove advise() and rename advise_ng() to
>     advise()

...this step may not be possible, for a few reasons:

  1. Some of the sites do more than just advise(). E.g., branch.c checks
     the flag and calls both error() and advise().

  2. Some callers may have to do work to generate the arguments. If I
     have:

       advise("advice.foo", "some data: %s", generate_data());

     then we'll call generate_data() even if we'll throw away the result
     in the end.

Similarly, some users of advice_* variables do not call advise() at all
(some call die(), some like builtin/rm.c stuff the result in a strbuf,
and I don't even know what's going on with wt_status.hints. :)

So I think you may need to phase it in a bit more, like:

  a. introduce want_advice() which decides whether or not to show the
     advice based on a config key. I'd also suggest making the "advice."
     part of the key implicit, just to make life easier for the callers.

  b. introduce advise_key() which uses want_advice() and advise() under
     the hood to do what your advise_ng() is doing here.

  c. convert simple patterns of:

       if (advice_foo)
          advise("bar");

     into:

       advise_key("foo", "bar");

     and drop advice_foo where possible.

  d. handle more complex cases one-by-one. For example, with something
     like:

       if (advice_foo)
         die("bar");

     we probably want:

       if (want_advice("foo"))
         die("bar");

     instead. Using string literals is more accident-prone than
     variables (because the compiler doesn't notice if we misspell them)
     but I think is OK for cases where we just refer to the key once.
     For others (e.g., advice_commit_before_merge has 13 mentions),
     either keep the variable. Or alternatively make a wrapper like:

       int want_advice_commit_before_merge(void)
       {
               return want_advice("commitbeforemerge");
       }

     if we want to drop the existing mechanism to load all of the
     variables at the beginning.

-Peff

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10 19:42     ` Taylor Blau
@ 2020-02-10 22:29       ` Emily Shaffer
  2020-02-11  0:08       ` Heba Waly
  1 sibling, 0 replies; 24+ messages in thread
From: Emily Shaffer @ 2020-02-10 22:29 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Junio C Hamano, Derrick Stolee, Heba Waly via GitGitGadget, git,
	Heba Waly

On Mon, Feb 10, 2020 at 11:42:53AM -0800, Taylor Blau wrote:
> On Mon, Feb 10, 2020 at 11:30:46AM -0800, Junio C Hamano wrote:
> > Derrick Stolee <stolee@gmail.com> writes:
> >
> > >> +static const char turn_off_instructions[] =
> > >> +N_("\n"
> > >> +"Turn this message off by running\n"
> > >> +"\"git config %s false\"");
> > >
> > > I have mixed feelings on the use of these instructions. Perhaps at
> > > minimum the addition of these instructions could be left to a
> > > separate patch than the creation of advise_ng().
> > >
> > > My biggest concern is that this adds unexpected noise to users who
> > > want the advice to stay. I'm calling attention to it, because this
> > > part isn't a simple refactor like the rest of the patch.
> > > ...
> > > I definitely tend to recommend more tests than most, but perhaps this
> > > unit test is overkill? You demonstrate a good test below using a real
> > > Git command, which should be sufficient. If the "turn this message off"
> > > part gets removed, then you will still have coverage of your method.
> > > It just won't require a test change because it would not modify behavior.
> > > ...
> >
> > All good suggestions.  Thanks for an excellent review.
> >
> > Another thing.
> >
> > advise_ng() may have been a good name for illustration but is a
> > horrible name for real-world use (imagine we need to revamp the API
> > one more time in the future---what would it be called, which has to
> > say that it is newer than the "next generation"?
> > advise_3rd_try()?).
> 
> What about calling this new API 'advise()'? The first patch could call
> it 'advise_ng' or whatever other temporary name we feel comfortable
> using, and then each subsequent patch would update callers of 'advise()'
> to use 'advise_ng()'. Once those patches have been applied, and no other
> callers of 'advise()' exist, a final patch can be applied on top to
> rename 'advise_ng()' to 'advise()', and update the names of all of the
> callers.

I think this is the precise strategy called out in the patch
description.
https://lore.kernel.org/git/pull.548.git.1581311049547.gitgitgadget@gmail.com

> This makes for a rather noisy final patch, but the intermediate states
> are much clearer, and it would make this series rather self-contained.
> 
> On the other hand, having a version of 'advise_ng()' on master makes
> this topic more incremental, meaning that we can pick it up and put it
> down at ease and have more self-contained projects.
> 
> I don't really have a preference between the two approaches, but if we
> go with the latter, I do think we need something better than
> 'advise_ng'. Maybe 'advise_warn'? I don't know.

I like that this opens up the possibility of advise_err(), advise_die(),
whatever to meet Peff's suggestion.

 - Emily

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10  5:04 [PATCH] advice: refactor advise API Heba Waly via GitGitGadget
  2020-02-10 14:38 ` Derrick Stolee
  2020-02-10 20:37 ` Jeff King
@ 2020-02-10 22:46 ` Junio C Hamano
  2020-02-11  2:01   ` Heba Waly
  2020-02-16 21:39 ` [PATCH v2 0/2] [RFC][Outreachy] " Heba Waly via GitGitGadget
  3 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2020-02-10 22:46 UTC (permalink / raw)
  To: Heba Waly via GitGitGadget; +Cc: git, Heba Waly

"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:

>     A new advise_ng() is introduced to gradually replace advise()
>     
>     pros of the new advise():
>     
>      * The caller doesn't need to define a new global variable when
>        introducing a new message.
>      * The caller doesn't need to check the visibility of the message before
>        calling advise_ng().
>      * The caller still needs to come up with advice.frotz config variable
>        and will call advice_ng as follows: advice_ng("advice.frotz",
>        _("helpful message about frotz"));

Readers would expect to see "cons of the same" to follow "pros".

>     After this patch the plan is to migrate the rest of the advise calls to
>     advise_ng and then finally remove advise() and rename advise_ng() to
>     advise()

As I outlined in [1], I think the over-simplified
"advise_ng(<advise.key>, _(<message>), ...)"  would be too limited
to replace the current users, without a pair of helper functions,
one to just check for the guarding advise.key, and the other to
unconditionally show the message (i.e. the latter is what the
current advise() is).

>     [1] 
>     https://public-inbox.org/git/xmqqzhf5cw69.fsf@gitster-ct.c.googlers.com/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10 20:37 ` Jeff King
@ 2020-02-10 22:55   ` Emily Shaffer
  2020-02-11  2:35     ` Heba Waly
  2020-02-11 19:49     ` Jeff King
  2020-02-11  2:20   ` Heba Waly
  1 sibling, 2 replies; 24+ messages in thread
From: Emily Shaffer @ 2020-02-10 22:55 UTC (permalink / raw)
  To: Jeff King; +Cc: Heba Waly via GitGitGadget, git, Heba Waly

On Mon, Feb 10, 2020 at 03:37:25PM -0500, Jeff King wrote:
> On Mon, Feb 10, 2020 at 05:04:09AM +0000, Heba Waly via GitGitGadget wrote:
> 
> >     The advice API is currently a little bit confusing to call. quoting from
> >     [1]:
> >     
> >     When introducing a new advice message, you would
> >     
> >      * come up with advice.frotz configuration variable
> >     
> >      * define and declare advice_frotz global variable that defaults to
> >        true
> >     
> >      * sprinkle calls like this:
> >     
> >       if (advice_frotz)
> >         advise(_("helpful message about frotz"));
> >     
> >     A new approach was suggested in [1] which this patch is based upon.
> 
> I agree that the current procedure is a bit painful, and I think this is
> a step in the right direction. But...
> 
> >     After this patch the plan is to migrate the rest of the advise calls to
> >     advise_ng and then finally remove advise() and rename advise_ng() to
> >     advise()
> 
> ...this step may not be possible, for a few reasons:
> 
>   1. Some of the sites do more than just advise(). E.g., branch.c checks
>      the flag and calls both error() and advise().
> 
>   2. Some callers may have to do work to generate the arguments. If I
>      have:
> 
>        advise("advice.foo", "some data: %s", generate_data());
> 
>      then we'll call generate_data() even if we'll throw away the result
>      in the end.
> 
> Similarly, some users of advice_* variables do not call advise() at all
> (some call die(), some like builtin/rm.c stuff the result in a strbuf,
> and I don't even know what's going on with wt_status.hints. :)
> 
> So I think you may need to phase it in a bit more, like:
> 
>   a. introduce want_advice() which decides whether or not to show the
>      advice based on a config key. I'd also suggest making the "advice."
>      part of the key implicit, just to make life easier for the callers.
> 
>   b. introduce advise_key() which uses want_advice() and advise() under
>      the hood to do what your advise_ng() is doing here.
> 
>   c. convert simple patterns of:
> 
>        if (advice_foo)
>           advise("bar");
> 
>      into:
> 
>        advise_key("foo", "bar");
> 
>      and drop advice_foo where possible.
> 
>   d. handle more complex cases one-by-one. For example, with something
>      like:
> 
>        if (advice_foo)
>          die("bar");
> 
>      we probably want:
> 
>        if (want_advice("foo"))
>          die("bar");
> 
>      instead. Using string literals is more accident-prone than
>      variables (because the compiler doesn't notice if we misspell them)
>      but I think is OK for cases where we just refer to the key once.
>      For others (e.g., advice_commit_before_merge has 13 mentions),
>      either keep the variable. Or alternatively make a wrapper like:
> 
>        int want_advice_commit_before_merge(void)
>        {
>                return want_advice("commitbeforemerge");
>        }
> 
>      if we want to drop the existing mechanism to load all of the
>      variables at the beginning.

I tend to disagree on both counts. I'd personally rather see something
like 'void advise_key(enum advice, char *format, ...)'.

As I understand it, Heba wanted to avoid that pattern so that people
adding a new advice didn't need to modify the advice library. However, I
think there's value to having a centralized list of all possible advices
(besides the documentation). The per-advice wrapper is harder to iterate
than an enum, and will also result in a lot of samey code if we decide
we want to use that pattern for more advices.

(In fact, with git-bugreport I'm running into a lot of regret that hooks
are invoked in the way Peff describes - 'find_hook("pre-commit")' -
rather than with an enum naming the hook; it's very hard to check all
possible hooks, and hard to examine the codebase and determine which
hooks do and don't exist.)

When Heba began to describe this project I had hoped for a final product
like 'void show_advice(enum advice_config)' which looked up the
appropriate string from the advice library instead of asking the caller
to provide it, although seeing the need for varargs has demonstrated to
me that that's not feasible :) But I think taking the advice config key
as an argument is possibly too far the other direction. At that point,
it starts to beg the question, "why isn't this function in config.h and
called print_if_configured(cfgname, message, ...)?"

Although, take this all with a grain of salt. I think I lean towards
this much encapsulation after a sordid history with C++ and an
enlightened C developer may not choose it ;)

 - Emily

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10 14:38 ` Derrick Stolee
  2020-02-10 19:30   ` Junio C Hamano
@ 2020-02-10 23:56   ` Heba Waly
  2020-02-11  2:39     ` Derrick Stolee
  1 sibling, 1 reply; 24+ messages in thread
From: Heba Waly @ 2020-02-10 23:56 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: Heba Waly via GitGitGadget, Git Mailing List

On Tue, Feb 11, 2020 at 3:38 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 2/10/2020 12:04 AM, Heba Waly via GitGitGadget wrote:
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > Add a new advise_ng function that can check the visibility of advice
> > messages before printing.
> >
> > Currently it's very easy for the callers to miss checking the
> > visibility step. Also, it makes more sense for this step to be handled
> > by the advice library.
>
> This makes the advice API much easier and its uses much cleaner. Thanks!
>
> > Also change the advise call in tag library from advise() to advise_ng()
> > to construct an example of the usage of the new API.
>
> This is a good example case.
>
> > +static const char turn_off_instructions[] =
> > +N_("\n"
> > +"Turn this message off by running\n"
> > +"\"git config %s false\"");
>
> I have mixed feelings on the use of these instructions. Perhaps at
> minimum the addition of these instructions could be left to a
> separate patch than the creation of advise_ng().
>
> My biggest concern is that this adds unexpected noise to users who
> want the advice to stay. I'm calling attention to it, because this
> part isn't a simple refactor like the rest of the patch.
>
> If it _does_ stay, then I recommend condensing the message to
> a single line. For example:
>
>         Disable this message with "git config %d false"
>

I agree with you, I had mixed feelings about it too when suggested on
a previous patch [2].
But then I realized that it's hard for the user to find the right
config variable to turn off from the doc only.
So I like the compromise of condensing it to a single line.

> > +     if(value)
> > +     {
>
> Style: spacing, and opening braces are on the same line as the if:
>
>         if (value) {
>
> But also, this method would be simpler if the opposite case was
> an early return:
>
>         if (!value)
>                 return;
> Then the rest could have one less indentation.

Agree

> This loop looks like it was copied from advise(). Perhaps we could
> re-use that code better by creating a new vadvise() method that
> takes a va_list, and have advise() and advise_ng() call it instead?
> I include a patch at the end of this method that does this conversion.
> (Feel free to incorporate it into your next version, if you want, but
> be sure to add your sign-off.) Then, your advise_ng() can call these:
>
>         vadvise(advice, params);
>         advise(turn_off_instructions, key);
>
> removing the need to re-implement the for loop.

My intention was to replace advise() by advise_ng(), so I didn't mind
a temp code repetition during the transition phase.
But as it seems like some folks would rather keep both, then yes of
course a vadvise() function is the way to go, thanks.

> > diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
> > new file mode 100644
> > index 0000000000..b6ec90fd18
> > --- /dev/null
> > +++ b/t/helper/test-advise.c
> > @@ -0,0 +1,15 @@
> > +#include "test-tool.h"
> > +#include "cache.h"
> > +#include "advice.h"
> > +
> > +int cmd__advise_ng(int argc, const char **argv)
> > +{
> > +     if (!argv[1] || !argv[2])
> > +     die("usage: %s <key> <advice>", argv[0]);
> > +
> > +     setup_git_directory();
> > +
> > +     advise_ng(argv[1], argv[2]);
> > +
> > +     return 0;
> > +}
>
> I definitely tend to recommend more tests than most, but perhaps this
> unit test is overkill? You demonstrate a good test below using a real
> Git command, which should be sufficient. If the "turn this message off"
> part gets removed, then you will still have coverage of your method.
> It just won't require a test change because it would not modify behavior.
>

I see your point but I wanted to make sure advise_ng honors the config
variable using tests 2 & 3 in `t0018-advice.sh`
and `t7004-tag.sh` didn't seem like a good place to add these tests.

> > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> > index 6db92bd3ba..b7c8d41899 100755
> > --- a/t/t7004-tag.sh
> > +++ b/t/t7004-tag.sh
> > @@ -1726,6 +1726,8 @@ 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: Turn this message off by running
> > +     hint: "git config advice.nestedTag false"
> >       EOF
> >       git tag -m nested nested annotated-v4.0 2>actual &&
> >       test_i18ncmp expect actual
> >
> > base-commit: c7a62075917b3340f908093f63f1161c44ed1475
>
> Thanks,
> -Stolee
>
> -->8--
>
> From: Derrick Stolee <dstolee@microsoft.com>
> Date: Mon, 10 Feb 2020 09:33:20 -0500
> Subject: [PATCH] advice: extract vadvise() from advise()
>
> 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 <dstolee@microsoft.com>
> ---
>  advice.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index 249c60dcf3..fd836332da 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;
> --
> 2.25.0.vfs.1.1.1.g9906319d24.dirty
>
>
>

[2] https://lore.kernel.org/git/CACg5j26DEXuxwqRYHi5UOBUpRwsu_2A9LwgyKq4qB9wxqasD7g@mail.gmail.com/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10 19:42     ` Taylor Blau
  2020-02-10 22:29       ` Emily Shaffer
@ 2020-02-11  0:08       ` Heba Waly
  2020-02-12 20:57         ` Taylor Blau
  1 sibling, 1 reply; 24+ messages in thread
From: Heba Waly @ 2020-02-11  0:08 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Junio C Hamano, Derrick Stolee, Heba Waly via GitGitGadget,
	Git Mailing List

On Tue, Feb 11, 2020 at 8:42 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Feb 10, 2020 at 11:30:46AM -0800, Junio C Hamano wrote:
> > Another thing.
> >
> > advise_ng() may have been a good name for illustration but is a
> > horrible name for real-world use (imagine we need to revamp the API
> > one more time in the future---what would it be called, which has to
> > say that it is newer than the "next generation"?
> > advise_3rd_try()?).

As I mentioned earlier, this patch is meant to be used as a transition
between the current advise() and the refactored one.
So the name is just temporary and it'll be renamed to advise() once
the transition is done.
But if we want to keep both functions, or want a better name because
it's an open-source project and the author might not complete the
transition, then I'll try to think of another name.

> What about calling this new API 'advise()'? The first patch could call
> it 'advise_ng' or whatever other temporary name we feel comfortable
> using, and then each subsequent patch would update callers of 'advise()'
> to use 'advise_ng()'. Once those patches have been applied, and no other
> callers of 'advise()' exist, a final patch can be applied on top to
> rename 'advise_ng()' to 'advise()', and update the names of all of the
> callers.
>

Yes, that's what I would like to do.

> This makes for a rather noisy final patch, but the intermediate states
> are much clearer, and it would make this series rather self-contained.
>
> On the other hand, having a version of 'advise_ng()' on master makes
> this topic more incremental, meaning that we can pick it up and put it
> down at ease and have more self-contained projects.
>
> I don't really have a preference between the two approaches, but if we
> go with the latter, I do think we need something better than
> 'advise_ng'. Maybe 'advise_warn'? I don't know.
>
> > Thanks.
>
> Thanks,
> Taylor

Thanks,
Heba

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10 22:46 ` Junio C Hamano
@ 2020-02-11  2:01   ` Heba Waly
  2020-02-11  6:08     ` Junio C Hamano
  0 siblings, 1 reply; 24+ messages in thread
From: Heba Waly @ 2020-02-11  2:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Heba Waly via GitGitGadget, Git Mailing List

On Tue, Feb 11, 2020 at 11:46 AM Junio C Hamano <gitster@pobox.com> wrote:
>
>
> As I outlined in [1], I think the over-simplified
> "advise_ng(<advise.key>, _(<message>), ...)"  would be too limited
> to replace the current users, without a pair of helper functions,
> one to just check for the guarding advise.key, and the other to
> unconditionally show the message (i.e. the latter is what the
> current advise() is).
>

I agree with adding the first helper, specially after Peff's comments,
but I don't see why we would keep the current advise() which
unconditionally shows the message...
As far as I understand from a previous discussion [3], that's a wrong
usage that we need to avoid, quoting from [3]:
```
If there are many other places that calls to advise() are made
without getting guarded by the toggles defined in advice.c, we
should fix them, I think.
```

> >     [1]
> >     https://public-inbox.org/git/xmqqzhf5cw69.fsf@gitster-ct.c.googlers.com/
[3] https://public-inbox.org/git/xmqqpng1eisc.fsf@gitster-ct.c.googlers.com/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10 20:37 ` Jeff King
  2020-02-10 22:55   ` Emily Shaffer
@ 2020-02-11  2:20   ` Heba Waly
  1 sibling, 0 replies; 24+ messages in thread
From: Heba Waly @ 2020-02-11  2:20 UTC (permalink / raw)
  To: Jeff King; +Cc: Heba Waly via GitGitGadget, Git Mailing List

On Tue, Feb 11, 2020 at 9:37 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Feb 10, 2020 at 05:04:09AM +0000, Heba Waly via GitGitGadget wrote:
>
> >     The advice API is currently a little bit confusing to call. quoting from
> >     [1]:
> >
> >     When introducing a new advice message, you would
> >
> >      * come up with advice.frotz configuration variable
> >
> >      * define and declare advice_frotz global variable that defaults to
> >        true
> >
> >      * sprinkle calls like this:
> >
> >       if (advice_frotz)
> >         advise(_("helpful message about frotz"));
> >
> >     A new approach was suggested in [1] which this patch is based upon.
>
> I agree that the current procedure is a bit painful, and I think this is
> a step in the right direction. But...
>
> >     After this patch the plan is to migrate the rest of the advise calls to
> >     advise_ng and then finally remove advise() and rename advise_ng() to
> >     advise()
>
> ...this step may not be possible, for a few reasons:
>
>   1. Some of the sites do more than just advise(). E.g., branch.c checks
>      the flag and calls both error() and advise().
>
>   2. Some callers may have to do work to generate the arguments. If I
>      have:
>
>        advise("advice.foo", "some data: %s", generate_data());
>
>      then we'll call generate_data() even if we'll throw away the result
>      in the end.
>
> Similarly, some users of advice_* variables do not call advise() at all
> (some call die(), some like builtin/rm.c stuff the result in a strbuf,
> and I don't even know what's going on with wt_status.hints. :)
>
> So I think you may need to phase it in a bit more, like:
>
>   a. introduce want_advice() which decides whether or not to show the
>      advice based on a config key. I'd also suggest making the "advice."
>      part of the key implicit, just to make life easier for the callers.
>

yes, I agree.

>   b. introduce advise_key() which uses want_advice() and advise() under
>      the hood to do what your advise_ng() is doing here.
>
>   c. convert simple patterns of:
>
>        if (advice_foo)
>           advise("bar");
>
>      into:
>
>        advise_key("foo", "bar");
>
>      and drop advice_foo where possible.
>
>   d. handle more complex cases one-by-one. For example, with something
>      like:
>
>        if (advice_foo)
>          die("bar");
>
>      we probably want:
>
>        if (want_advice("foo"))
>          die("bar");
>
>      instead. Using string literals is more accident-prone than
>      variables (because the compiler doesn't notice if we misspell them)
>      but I think is OK for cases where we just refer to the key once.
>      For others (e.g., advice_commit_before_merge has 13 mentions),
>      either keep the variable. Or alternatively make a wrapper like:
>
>        int want_advice_commit_before_merge(void)
>        {
>                return want_advice("commitbeforemerge");
>        }
>
>      if we want to drop the existing mechanism to load all of the
>      variables at the beginning.
>

All make sense to me, thanks for the feedback.

> -Peff

Heba

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10 22:55   ` Emily Shaffer
@ 2020-02-11  2:35     ` Heba Waly
  2020-02-11 19:49     ` Jeff King
  1 sibling, 0 replies; 24+ messages in thread
From: Heba Waly @ 2020-02-11  2:35 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: Jeff King, Heba Waly via GitGitGadget, Git Mailing List

On Tue, Feb 11, 2020 at 11:55 AM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> On Mon, Feb 10, 2020 at 03:37:25PM -0500, Jeff King wrote:
> >
> >      instead. Using string literals is more accident-prone than
> >      variables (because the compiler doesn't notice if we misspell them)
> >      but I think is OK for cases where we just refer to the key once.
> >      For others (e.g., advice_commit_before_merge has 13 mentions),
> >      either keep the variable. Or alternatively make a wrapper like:
> >
> >        int want_advice_commit_before_merge(void)
> >        {
> >                return want_advice("commitbeforemerge");
> >        }
> >
> >      if we want to drop the existing mechanism to load all of the
> >      variables at the beginning.
>
> I tend to disagree on both counts. I'd personally rather see something
> like 'void advise_key(enum advice, char *format, ...)'.
>
> As I understand it, Heba wanted to avoid that pattern so that people
> adding a new advice didn't need to modify the advice library. However, I
> think there's value to having a centralized list of all possible advices
> (besides the documentation). The per-advice wrapper is harder to iterate
> than an enum, and will also result in a lot of samey code if we decide
> we want to use that pattern for more advices.
>

I think Peff is suggesting the wrapper for only those rare cases where
a single advice config variable is being checked several times around
the code base, but this doesn't mean we'll have many of them. In my
own opinion, I don't see the need for the list of advices, I think
it'll add unneeded complexity to the advice library and the
introduction of new advice messages. Mainly because I don't see a
scenario where we'd need to iterate through them, so I don't know ...

> (In fact, with git-bugreport I'm running into a lot of regret that hooks
> are invoked in the way Peff describes - 'find_hook("pre-commit")' -
> rather than with an enum naming the hook; it's very hard to check all
> possible hooks, and hard to examine the codebase and determine which
> hooks do and don't exist.)
>
> When Heba began to describe this project I had hoped for a final product
> like 'void show_advice(enum advice_config)' which looked up the
> appropriate string from the advice library instead of asking the caller
> to provide it, although seeing the need for varargs has demonstrated to
> me that that's not feasible :) But I think taking the advice config key
> as an argument is possibly too far the other direction. At that point,
> it starts to beg the question, "why isn't this function in config.h and
> called print_if_configured(cfgname, message, ...)?"
>
> Although, take this all with a grain of salt. I think I lean towards
> this much encapsulation after a sordid history with C++ and an
> enlightened C developer may not choose it ;)
>
>  - Emily

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10 23:56   ` Heba Waly
@ 2020-02-11  2:39     ` Derrick Stolee
  0 siblings, 0 replies; 24+ messages in thread
From: Derrick Stolee @ 2020-02-11  2:39 UTC (permalink / raw)
  To: Heba Waly; +Cc: Heba Waly via GitGitGadget, Git Mailing List

On 2/10/2020 6:56 PM, Heba Waly wrote:
> On Tue, Feb 11, 2020 at 3:38 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> I definitely tend to recommend more tests than most, but perhaps this
>> unit test is overkill? You demonstrate a good test below using a real
>> Git command, which should be sufficient. If the "turn this message off"
>> part gets removed, then you will still have coverage of your method.
>> It just won't require a test change because it would not modify behavior.
>>
> 
> I see your point but I wanted to make sure advise_ng honors the config
> variable using tests 2 & 3 in `t0018-advice.sh`
> and `t7004-tag.sh` didn't seem like a good place to add these tests.

You're right. I wasn't considering the case of not showing the message
with respect to the config.

-Stolee

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-11  2:01   ` Heba Waly
@ 2020-02-11  6:08     ` Junio C Hamano
  0 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2020-02-11  6:08 UTC (permalink / raw)
  To: Heba Waly; +Cc: Heba Waly via GitGitGadget, Git Mailing List

Heba Waly <heba.waly@gmail.com> writes:

> On Tue, Feb 11, 2020 at 11:46 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>>
>> As I outlined in [1], I think the over-simplified
>> "advise_ng(<advise.key>, _(<message>), ...)"  would be too limited
>> to replace the current users, without a pair of helper functions,
>> one to just check for the guarding advise.key, and the other to
>> unconditionally show the message (i.e. the latter is what the
>> current advise() is).
>>
>
> I agree with adding the first helper, specially after Peff's comments,
> but I don't see why we would keep the current advise() which
> unconditionally shows the message...

Look again at the message you referenced in your message that
started this round, and read its comment:

	if (advise_ng_enabled("frotz")) {
		char *result = expensive_computation(...);

		/*
                 * advise_ng("frotz", _("message %s about frotz", result));
                 * is fine as well, but slightly less efficient as
                 * it would involve another call to *_enabled(), so use
		 * the unconditional form of the call
		 */
		advise_ng_raw(_("message %s about frotz", result));

		free(result);
	}


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-10 22:55   ` Emily Shaffer
  2020-02-11  2:35     ` Heba Waly
@ 2020-02-11 19:49     ` Jeff King
  2020-02-11 19:51       ` Jeff King
  1 sibling, 1 reply; 24+ messages in thread
From: Jeff King @ 2020-02-11 19:49 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: Heba Waly via GitGitGadget, git, Heba Waly

On Mon, Feb 10, 2020 at 02:55:28PM -0800, Emily Shaffer wrote:

> I tend to disagree on both counts. I'd personally rather see something
> like 'void advise_key(enum advice, char *format, ...)'.

Yeah, I don't mind that at all. It's mutually exclusive with not making
people add new enums when they want to add new advice types, but as you
note we do get some code maintenance benefit by having the variables,
even if _adding_ a new one is a slightly larger pain.

-Peff

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-11 19:49     ` Jeff King
@ 2020-02-11 19:51       ` Jeff King
  0 siblings, 0 replies; 24+ messages in thread
From: Jeff King @ 2020-02-11 19:51 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: Heba Waly via GitGitGadget, git, Heba Waly

On Tue, Feb 11, 2020 at 02:49:43PM -0500, Jeff King wrote:

> On Mon, Feb 10, 2020 at 02:55:28PM -0800, Emily Shaffer wrote:
> 
> > I tend to disagree on both counts. I'd personally rather see something
> > like 'void advise_key(enum advice, char *format, ...)'.
> 
> Yeah, I don't mind that at all. It's mutually exclusive with not making
> people add new enums when they want to add new advice types, but as you
> note we do get some code maintenance benefit by having the variables,
> even if _adding_ a new one is a slightly larger pain.

Thinking on it a bit more, one argument in favor of having enums or
variables or whatever is that we _do_ need a master list of all of the
variables in one spot: the documentation.

So one direction we _could_ go is either generating the enum from the
documentation or vice versa (or generating both from a master list).
That would give us one place to define a new key along with its
description.

-Peff

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] advice: refactor advise API
  2020-02-11  0:08       ` Heba Waly
@ 2020-02-12 20:57         ` Taylor Blau
  0 siblings, 0 replies; 24+ messages in thread
From: Taylor Blau @ 2020-02-12 20:57 UTC (permalink / raw)
  To: Heba Waly
  Cc: Taylor Blau, Junio C Hamano, Derrick Stolee,
	Heba Waly via GitGitGadget, Git Mailing List

On Tue, Feb 11, 2020 at 01:08:48PM +1300, Heba Waly wrote:
> On Tue, Feb 11, 2020 at 8:42 AM Taylor Blau <me@ttaylorr.com> wrote:
> >
> > On Mon, Feb 10, 2020 at 11:30:46AM -0800, Junio C Hamano wrote:
> > > Another thing.
> > >
> > > advise_ng() may have been a good name for illustration but is a
> > > horrible name for real-world use (imagine we need to revamp the API
> > > one more time in the future---what would it be called, which has to
> > > say that it is newer than the "next generation"?
> > > advise_3rd_try()?).
>
> As I mentioned earlier, this patch is meant to be used as a transition
> between the current advise() and the refactored one.

Ah, thanks for pointing it out, and I'm sorry that I missed reading it
in my first review. Your idea sounds quite good to me, and thanks for
working on this.

> So the name is just temporary and it'll be renamed to advise() once
> the transition is done.
> But if we want to keep both functions, or want a better name because
> it's an open-source project and the author might not complete the
> transition, then I'll try to think of another name.
>
> > What about calling this new API 'advise()'? The first patch could call
> > it 'advise_ng' or whatever other temporary name we feel comfortable
> > using, and then each subsequent patch would update callers of 'advise()'
> > to use 'advise_ng()'. Once those patches have been applied, and no other
> > callers of 'advise()' exist, a final patch can be applied on top to
> > rename 'advise_ng()' to 'advise()', and update the names of all of the
> > callers.
> >
>
> Yes, that's what I would like to do.
>
> > This makes for a rather noisy final patch, but the intermediate states
> > are much clearer, and it would make this series rather self-contained.
> >
> > On the other hand, having a version of 'advise_ng()' on master makes
> > this topic more incremental, meaning that we can pick it up and put it
> > down at ease and have more self-contained projects.
> >
> > I don't really have a preference between the two approaches, but if we
> > go with the latter, I do think we need something better than
> > 'advise_ng'. Maybe 'advise_warn'? I don't know.
> >
> > > Thanks.
> >
> > Thanks,
> > Taylor
>
> Thanks,
> Heba

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 0/2] [RFC][Outreachy] advice: refactor advise API
  2020-02-10  5:04 [PATCH] advice: refactor advise API Heba Waly via GitGitGadget
                   ` (2 preceding siblings ...)
  2020-02-10 22:46 ` Junio C Hamano
@ 2020-02-16 21:39 ` " Heba Waly via GitGitGadget
  2020-02-16 21:39   ` [PATCH v2 1/2] " Heba Waly via GitGitGadget
                     ` (2 more replies)
  3 siblings, 3 replies; 24+ messages in thread
From: Heba Waly via GitGitGadget @ 2020-02-16 21:39 UTC (permalink / raw)
  To: git; +Cc: Heba Waly

Main changes in V2: 1- Rename advise_ng to advise_if_enabled. 2- Add a new
advise_enabled() helper. 3- Add a list of config variables names to replace
advice_config[] (used by list_config_advices()). 4- Send an enum parameter
to the new advise helpers instead of strings. 5- Extract vadvise() from
advise() and advise_if enabled().


----------------------------------------------------------------------------

The advice API is currently a little bit confusing to call. quoting from
[1]:

When introducing a new advice message, you would

 * come up with advice.frotz configuration variable

 * define and declare advice_frotz global variable that defaults to
   true

 * sprinkle calls like this:

  if (advice_frotz)
    advise(_("helpful message about frotz"));

A new approach was suggested in [1] which this patch is based upon.

A new advise_if_enabled() is introduced to gradually replace advise()
advice_enabled() helper is also introduced to be used by those callers who:

 * Only need to check the visibility without calling advise() (they call
   die() or error() instead for example)
 * Need to carry out some heavy processing to display an advice, in this
   case they'll do: if(advice_enabled(advice_type))  advise("some advice message");
   
   

To introduce a new advice message, the caller needs to:

 * Add a new_advice_type to 'enum advice_type'
 * Come up with a new config variable name and add this name to 
   advice_config_keys[]
 * Call advise_if_enabled(new_advice_type, "advice message to be printed")
 * Or call advice_enabled(new_advice_type) first and then follow is by
   advice("advice message to be printed") as explained earlier.
 * Add the new config variable to Documentation/config/advice.txt

The reason a new list of configuration variables was added to the library is
to be used by the list_config_advices() function instead of advice_config[].
And we should get rid of advice_config[] once we migrate all the callers to
use the new APIs instead of checking the global variables (which we'll get
rid of as well).

In the future, we can investigate generating the documentation from the list
of config variables or vice versa to make introducing a new advice much
easier, but this approach will do it for now.

V2 makes the process of introducing a new advice longer than V1 and almost
as long as the original library, but having the advice library responsible
for checking the message visibility is still an improvement and in my own
opinion the new structure makes better sense and makes the library less
confusing to use.

After this patch the plan is to change the advise() calls to
advise_if_enabled() whenever possible, or at least replace the global
variables checks by advise_enabled() when advise_if_enabled() is not
suitable.

[1] https://public-inbox.org/git/xmqqzhf5cw69.fsf@gitster-ct.c.googlers.com/

Heba Waly (2):
  advice: refactor advise API
  advice: extract vadvise() from advise()

 Makefile               |  1 +
 advice.c               | 89 ++++++++++++++++++++++++++++++++++++++----
 advice.h               | 58 ++++++++++++++++++++++++++-
 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, 188 insertions(+), 11 deletions(-)
 create mode 100644 t/helper/test-advise.c
 create mode 100755 t/t0018-advice.sh


base-commit: c7a62075917b3340f908093f63f1161c44ed1475
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-548%2FHebaWaly%2Fadvice_refactoring-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-548/HebaWaly/advice_refactoring-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/548

Range-diff vs v1:

 1:  0dcab7b4eb3 ! 1:  080d12b5c69 advice: refactor advise API
     @@ -2,15 +2,25 @@
      
          advice: refactor advise API
      
     -    Add a new advise_ng function that can check the visibility of advice
     -    messages before printing.
     +    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.
      
     -    Currently it's very easy for the callers to miss checking the
     -    visibility step. 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.
      
     -    Also change the advise call in tag library from advise() to advise_ng()
     -    to construct an example of the usage of the new API.
     +    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 <heba.waly@gmail.com>
      
     @@ -45,48 +55,112 @@
       	{ "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;
      @@
       	strbuf_release(&buf);
       }
       
     ++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);
     ++	
     ++}
     ++
      +static const char turn_off_instructions[] =
      +N_("\n"
     -+"Turn this message off by running\n"
     -+"\"git config %s false\"");
     ++   "Disable this message with \"git config %s false\"");
      +
     -+void advise_ng(const char *key, const char *advice, ...)
     ++void advise_if_enabled(enum advice_type type, const char *advice, ...)
      +{
     -+	int value = 1;
      +	struct strbuf buf = STRBUF_INIT;
     ++	char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]);
      +	va_list params;
      +	const char *cp, *np;
      +	
     -+	git_config_get_bool(key, &value);
     ++	if(!advice_enabled(type))
     ++		return;
     ++
     ++	va_start(params, advice);
     ++	strbuf_vaddf(&buf, advice, params);
     ++	va_end(params);
     ++
     ++	strbuf_addf(&buf, turn_off_instructions, key);
      +	
     -+	if(value)
     -+	{
     -+		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++;
     -+		}
     -+		strbuf_release(&buf);
     ++	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++;
      +	}
     ++	strbuf_release(&buf);
     ++
      +}
      +
       int git_default_advice_config(const char *var, const char *value)
       {
       	const char *k, *slot_name;
     +@@
     + {
     + 	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
       --- a/advice.h
     @@ -98,14 +172,66 @@
      -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,
     ++};
     ++
     ++
       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 if PUSH_UPDATE_REJECTED advice type is enabled.
     ++ */
     ++int advice_push_update_rejected_enabled(void);
     ++
     ++/**
      + Checks the visibility of the advice before priniting.
      + */
     -+void advise_ng(const char *key, const char *advice, ...);
     ++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);
     @@ -120,7 +246,7 @@
      -	if (type == OBJ_TAG && advice_nested_tag)
      -		advise(_(message_advice_nested_tag), tag, object_ref);
      +	if (type == OBJ_TAG)
     -+		advise_ng("advice.nestedTag", _(message_advice_nested_tag), tag, object_ref);
     ++		advise_if_enabled(NESTED_TAG, _(message_advice_nested_tag), tag, object_ref);
       
       	strbuf_addf(&header,
       		    "object %s\n"
     @@ -134,14 +260,15 @@
      +#include "cache.h"
      +#include "advice.h"
      +
     -+int cmd__advise_ng(int argc, const char **argv)
     ++int cmd__advise_if_enabled(int argc, const char **argv)
      +{
     -+	if (!argv[1] || !argv[2])
     -+	die("usage: %s <key> <advice>", argv[0]);
     ++	if (!argv[1])
     ++	die("usage: %s <advice>", argv[0]);
      +
      +	setup_git_directory();
     -+
     -+	advise_ng(argv[1], argv[2]);
     ++	
     ++	//use any advice type for testing
     ++	advise_if_enabled(NESTED_TAG, argv[1]);
      +
      +	return 0;
      +}
     @@ -153,7 +280,7 @@
       };
       
       static struct test_cmd cmds[] = {
     -+	{ "advise", cmd__advise_ng },
     ++	{ "advise", cmd__advise_if_enabled },
       	{ "chmtime", cmd__chmtime },
       	{ "config", cmd__config },
       	{ "ctype", cmd__ctype },
     @@ -165,7 +292,7 @@
       #define USE_THE_INDEX_COMPATIBILITY_MACROS
       #include "git-compat-util.h"
       
     -+int cmd__advise_ng(int argc, const char **argv);
     ++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);
     @@ -177,29 +304,28 @@
      @@
      +#!/bin/sh
      +
     -+test_description='Test advise_ng functionality'
     ++test_description='Test advise_if_enabled functionality'
      +
      +. ./test-lib.sh
      +
      +cat > expected <<EOF
      +hint: This is a piece of advice
     -+hint: Turn this message off by running
     -+hint: "git config advice.configVariable false"
     ++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 "advice.configVariable" "This is a piece of advice" 2>actual &&
     ++	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.configVariable true &&
     -+	test-tool advise "advice.configVariable" "This is a piece of advice" 2>actual &&
     ++	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.configVariable false &&
     -+	test-tool advise "advice.configVariable" "This is a piece of advice" 2>actual &&
     ++	test_config advice.nestedTag false &&
     ++	test-tool advise "This is a piece of advice" 2>actual &&
      +	test_must_be_empty actual
      +'
      +
     @@ -212,8 +338,7 @@
       	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: Turn this message off by running
     -+	hint: "git config advice.nestedTag false"
     ++	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
 -:  ----------- > 2:  3e4f52e5526 advice: extract vadvise() from advise()

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 1/2] advice: refactor advise API
  2020-02-16 21:39 ` [PATCH v2 0/2] [RFC][Outreachy] " Heba Waly via GitGitGadget
@ 2020-02-16 21:39   ` " Heba Waly via GitGitGadget
  2020-02-17  3:28     ` Junio C Hamano
  2020-02-16 21:39   ` [PATCH v2 2/2] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
  2020-02-17  0:09   ` [PATCH v2 0/2] [RFC][Outreachy] advice: refactor advise API Junio C Hamano
  2 siblings, 1 reply; 24+ messages in thread
From: Heba Waly via GitGitGadget @ 2020-02-16 21:39 UTC (permalink / raw)
  To: git; +Cc: Heba Waly, Heba Waly

From: Heba Waly <heba.waly@gmail.com>

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 <heba.waly@gmail.com>
---
 Makefile               |  1 +
 advice.c               | 88 ++++++++++++++++++++++++++++++++++++++++--
 advice.h               | 58 +++++++++++++++++++++++++++-
 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, 191 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..8cedc649afa 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,54 @@ void advise(const char *advice, ...)
 	strbuf_release(&buf);
 }
 
+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);
+	
+}
+
+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]);
+	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++;
+	}
+	strbuf_release(&buf);
+
+}
+
 int git_default_advice_config(const char *var, const char *value)
 {
 	const char *k, *slot_name;
@@ -154,8 +234,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..d2ac3084067 100644
--- a/advice.h
+++ b/advice.h
@@ -29,12 +29,68 @@ 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,
+};
+
+
 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 if PUSH_UPDATE_REJECTED advice type is enabled.
+ */
+int advice_push_update_rejected_enabled(void);
+
+/**
+ Checks the visibility of the advice before priniting.
+ */
+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);
 
 	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..c49be89f970
--- /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
+	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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 2/2] advice: extract vadvise() from advise()
  2020-02-16 21:39 ` [PATCH v2 0/2] [RFC][Outreachy] " Heba Waly via GitGitGadget
  2020-02-16 21:39   ` [PATCH v2 1/2] " Heba Waly via GitGitGadget
@ 2020-02-16 21:39   ` Heba Waly via GitGitGadget
  2020-02-17  0:09   ` [PATCH v2 0/2] [RFC][Outreachy] advice: refactor advise API Junio C Hamano
  2 siblings, 0 replies; 24+ messages in thread
From: Heba Waly via GitGitGadget @ 2020-02-16 21:39 UTC (permalink / raw)
  To: git; +Cc: Heba Waly, Heba Waly

From: Heba Waly <heba.waly@gmail.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Heba Waly <heba.waly@gmail.com>
---
 advice.c | 45 +++++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/advice.c b/advice.c
index 8cedc649afa..6c0be19a7c5 100644
--- a/advice.c
+++ b/advice.c
@@ -128,15 +128,20 @@ static const char *advice_config_keys[] = {
 	[SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = "submoduleAlternateErrorStrategyDie"
 };
 
-void advise(const char *advice, ...)
+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)
 {
 	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');
@@ -165,37 +170,25 @@ int advice_push_update_rejected_enabled(void)
 	
 }
 
-static const char turn_off_instructions[] =
-N_("\n"
-   "Disable this message with \"git config %s false\"");
+void advise(const char *advice, ...)
+{
+	va_list params;
+	va_start(params, advice);
+	vadvise(advice, params, 0, "");
+	va_end(params);
+}
 
 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]);
 	va_list params;
-	const char *cp, *np;
-	
+	char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]);
+
 	if(!advice_enabled(type))
 		return;
 
 	va_start(params, advice);
-	strbuf_vaddf(&buf, advice, params);
+	vadvise(advice, params, 1, key);
 	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++;
-	}
-	strbuf_release(&buf);
-
 }
 
 int git_default_advice_config(const char *var, const char *value)
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 0/2] [RFC][Outreachy] advice: refactor advise API
  2020-02-16 21:39 ` [PATCH v2 0/2] [RFC][Outreachy] " Heba Waly via GitGitGadget
  2020-02-16 21:39   ` [PATCH v2 1/2] " Heba Waly via GitGitGadget
  2020-02-16 21:39   ` [PATCH v2 2/2] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
@ 2020-02-17  0:09   ` Junio C Hamano
  2 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2020-02-17  0:09 UTC (permalink / raw)
  To: Heba Waly via GitGitGadget; +Cc: git, Heba Waly

"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Main changes in V2: 1- Rename advise_ng to advise_if_enabled. 2- Add a new
> advise_enabled() helper. 3- Add a list of config variables names to replace
> advice_config[] (used by list_config_advices()). 4- Send an enum parameter
> to the new advise helpers instead of strings. 5- Extract vadvise() from
> advise() and advise_if enabled().


Nice.  There is not much point for me to correct mistakes in the
title of the cover letter, but anyway... I think these changes are
not "refactoring" the API, but these are enhancing, extending or
even revamping the API.  

> To introduce a new advice message, the caller needs to:
>
>  * Add a new_advice_type to 'enum advice_type'
>  * Come up with a new config variable name and add this name to 
>    advice_config_keys[]
>  * Call advise_if_enabled(new_advice_type, "advice message to be printed")
>  * Or call advice_enabled(new_advice_type) first and then follow is by
>    advice("advice message to be printed") as explained earlier.
>  * Add the new config variable to Documentation/config/advice.txt

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".

> In the future, we can investigate generating the documentation from the list
> of config variables or vice versa to make introducing a new advice much
> easier, but this approach will do it for now.

Yup.  One step at a time.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/2] advice: refactor advise API
  2020-02-16 21:39   ` [PATCH v2 1/2] " Heba Waly via GitGitGadget
@ 2020-02-17  3:28     ` Junio C Hamano
  2020-02-17 10:03       ` Heba Waly
  0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2020-02-17  3:28 UTC (permalink / raw)
  To: Heba Waly via GitGitGadget; +Cc: git, Heba Waly

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.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/2] advice: refactor advise API
  2020-02-17  3:28     ` Junio C Hamano
@ 2020-02-17 10:03       ` Heba Waly
  0 siblings, 0 replies; 24+ messages in thread
From: Heba Waly @ 2020-02-17 10:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Heba Waly via GitGitGadget, Git Mailing List

On Mon, Feb 17, 2020 at 4:28 PM Junio C Hamano <gitster@pobox.com> 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

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, back to index

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10  5:04 [PATCH] advice: refactor advise API Heba Waly via GitGitGadget
2020-02-10 14:38 ` Derrick Stolee
2020-02-10 19:30   ` Junio C Hamano
2020-02-10 19:42     ` Taylor Blau
2020-02-10 22:29       ` Emily Shaffer
2020-02-11  0:08       ` Heba Waly
2020-02-12 20:57         ` Taylor Blau
2020-02-10 23:56   ` Heba Waly
2020-02-11  2:39     ` Derrick Stolee
2020-02-10 20:37 ` Jeff King
2020-02-10 22:55   ` Emily Shaffer
2020-02-11  2:35     ` Heba Waly
2020-02-11 19:49     ` Jeff King
2020-02-11 19:51       ` Jeff King
2020-02-11  2:20   ` Heba Waly
2020-02-10 22:46 ` Junio C Hamano
2020-02-11  2:01   ` Heba Waly
2020-02-11  6:08     ` Junio C Hamano
2020-02-16 21:39 ` [PATCH v2 0/2] [RFC][Outreachy] " Heba Waly via GitGitGadget
2020-02-16 21:39   ` [PATCH v2 1/2] " Heba Waly via GitGitGadget
2020-02-17  3:28     ` Junio C Hamano
2020-02-17 10:03       ` Heba Waly
2020-02-16 21:39   ` [PATCH v2 2/2] advice: extract vadvise() from advise() Heba Waly via GitGitGadget
2020-02-17  0:09   ` [PATCH v2 0/2] [RFC][Outreachy] advice: refactor advise API Junio C Hamano

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git