[PATCH 0/1] [Outreachy] [RFC] add: use advise function to display hints

[PATCH 0/1] [Outreachy] [RFC] add: use advise function to display hints

[Heba Waly via GitGitGadget]

The advise function in advice.c provides a neat and a standard format for
hint messages, i.e: the text is colored in yellow and the line starts by the
word "hint:".

This patch suggests using this advise function whenever displaying hints to
improve the user experience, as the user's eyes will get used to the format
and will scan the screen for the yellow hints whenever confused instead of
reading all the output lines looking for advice.

Heba Waly (1):
  add: use advise function to display hints

 builtin/add.c  | 4 ++--
 t/t3700-add.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)


base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-508%2FHebaWaly%2Fformatting_hints-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-508/HebaWaly/formatting_hints-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/508
-- 
gitgitgadget

[PATCH 1/1] add: use advise function to display hints

[Heba Waly via GitGitGadget]

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

Use the advise function in advice.c to display hints to the users, as
it provides a neat and a standard format for hint messages, i.e: the
text is colored in yellow and the line starts by the word "hint:".

Signed-off-by: Heba Waly <heba.waly@gmail.com>
---
 builtin/add.c  | 4 ++--
 t/t3700-add.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 4c38aff419..eebf8d772b 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -390,7 +390,7 @@ static int add_files(struct dir_struct *dir, int flags)
 		fprintf(stderr, _(ignore_error));
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
-		fprintf(stderr, _("Use -f if you really want to add them.\n"));
+		advise(_("Use -f if you really want to add them.\n"));
 		exit_status = 1;
 	}
 
@@ -480,7 +480,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
+		advise( _("Maybe you wanted to say 'git add .'?\n"));
 		return 0;
 	}
 
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index c325167b90..a649805369 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -326,7 +326,7 @@ test_expect_success 'git add --dry-run of an existing file output' "
 cat >expect.err <<\EOF
 The following paths are ignored by one of your .gitignore files:
 ignored-file
-Use -f if you really want to add them.
+hint: Use -f if you really want to add them.
 EOF
 cat >expect.out <<\EOF
 add 'track-this'
-- 
gitgitgadget

Re: [PATCH 1/1] add: use advise function to display hints

[Junio C Hamano]

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

> From: Heba Waly <heba.waly@gmail.com>
>
> Use the advise function in advice.c to display hints to the users, as
> it provides a neat and a standard format for hint messages, i.e: the
> text is colored in yellow and the line starts by the word "hint:".

Use of advise() function is good for giving hints not just due to
its yellow coloring (which by the way I find not very readable,
perhaps because I use black ink on white paper).  One good thing in
using the advise() API is that the messages can also be squelched
with advice.* configuration variables.

And these two hints in "git add" are good chandidates to make
customizable (perhaps with "advice.addNothing"), so I tend to agree
with you that it makes sense to move these two messages to advise().
Unfortunately this patch goes only halfway and stops (see below).

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.

>
> Signed-off-by: Heba Waly <heba.waly@gmail.com>
> ---
>  builtin/add.c  | 4 ++--
>  t/t3700-add.sh | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 4c38aff419..eebf8d772b 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -390,7 +390,7 @@ static int add_files(struct dir_struct *dir, int flags)
>  		fprintf(stderr, _(ignore_error));
>  		for (i = 0; i < dir->ignored_nr; i++)
>  			fprintf(stderr, "%s\n", dir->ignored[i]->name);
> -		fprintf(stderr, _("Use -f if you really want to add them.\n"));
> +		advise(_("Use -f if you really want to add them.\n"));
>  		exit_status = 1;
>  	}
>  
> @@ -480,7 +480,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  
>  	if (require_pathspec && pathspec.nr == 0) {
>  		fprintf(stderr, _("Nothing specified, nothing added.\n"));
> -		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
> +		advise( _("Maybe you wanted to say 'git add .'?\n"));
>  		return 0;
>  	}

The final code for the above part would look like:

		if (advice_add_nothing)
			advise(_("Use -f if you really want to add them."));
		...
		if (advice_add_nothing)
			advise( _("Maybe you wanted to say 'git add .'?"));

and then you would

 * add defn of advice_add_nothing to advice.h
 * add decl of the same, initialized to 1(true), to advice.c
 * map "addNothing" to &advice_add_nothing in advice.c::advice_config[]

to complete the other half of this patch, if the config we choose to
use is named "advice.addNothing".

By the way, notice that the single-liner advise() messages do not
end with LF?  This is another difference between printf() family and
advise().  advise() cuts its message at LF and prefixes each piece
with "hint:" but after the final LF there is nothing but NUL, which
means the final LF is optional.

The warning()/error()/die() family is different from advise() in
that they do not chop the incoming message at LF.  This behaviour is
less i18n friendly, and it would be nice to eventually change them
to behave similarly to advise().

Thanks.

 

Re: [PATCH 1/1] add: use advise function to display hints

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Use of advise() function is good for giving hints not just due to
> its yellow coloring (which by the way I find not very readable,
> perhaps because I use black ink on white paper).  One good thing in
> using the advise() API is that the messages can also be squelched
> with advice.* configuration variables.

A side note.

Right now, the advise() API is a bit awkweard to use correctly.
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"));

I am wondering about two things:

 (1) if we can update the API so that the above can be reduced to
     just adding calls like:

	advise_ng("frotz", _("helpful message about frotz"));

 (2) if such a simplified advise_ng API is a good idea to begin
     with.

There are a few advantages the current API has, but it cuts both
ways.

 - Any new advice toggle MUST be registered to the
   advice.c::advice_config[] table.  This table can later be
   extended in the future to allow a list of the toggles to be
   produced at runtime.

   This can be seen as an easy mechanism to force programmers to
   keep the list up to date.  Or it can also be seen as the source
   of extra work.

 - advise() calls can be made without being guarded by any advice.*
   configuration variable.  In the overly simplified advise_ng() API
   shown above, we cannot expresss a pattern like this:

	if (advice_frotz) {
		... make expensive computation to
		... come up with values that need to be shown
		... in the advise() message
		char *result = expensive_computation(...);

		advise(_("message %s about frotz", result));
		free(result);
	}

   without adding another helper function. e.g.

	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);
	}

Re: [PATCH 1/1] add: use advise function to display hints

[Heba Waly]

On Fri, Jan 3, 2020 at 11:48 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> A side note.
>
> Right now, the advise() API is a bit awkweard to use correctly.
> 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"));
>
> I am wondering about two things:
>
>  (1) if we can update the API so that the above can be reduced to
>      just adding calls like:
>
>         advise_ng("frotz", _("helpful message about frotz"));
>
>  (2) if such a simplified advise_ng API is a good idea to begin
>      with.
>

That's a valid suggestion, I can investigate that in a new patch, I'd rather
keep this one as simple as calling the existing advise function.

Thanks,

Heba

Re: [PATCH 1/1] add: use advise function to display hints

[Junio C Hamano]

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

> On Fri, Jan 3, 2020 at 11:48 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> A side note.
>>
>> Right now, the advise() API is a bit awkweard to use correctly.
>> 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"));
>>
>> I am wondering about two things:
>>
>>  (1) if we can update the API so that the above can be reduced to
>>      just adding calls like:
>>
>>         advise_ng("frotz", _("helpful message about frotz"));
>>
>>  (2) if such a simplified advise_ng API is a good idea to begin
>>      with.
>>
>
> That's a valid suggestion, I can investigate that in a new patch, I'd rather
> keep this one as simple as calling the existing advise function.

Yeah, the side note wasn't even a suggestion for improving _this_
topic, nor even specifically addressed to you.  Let's stay focused.

Thanks.

Re: [PATCH 1/1] add: use advise function to display hints

[Heba Waly]

On Wed, Jan 8, 2020 at 5:35 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Heba Waly <heba.waly@gmail.com> writes:
>
> > On Fri, Jan 3, 2020 at 11:48 AM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> > That's a valid suggestion, I can investigate that in a new patch, I'd rather
> > keep this one as simple as calling the existing advise function.
>
> Yeah, the side note wasn't even a suggestion for improving _this_
> topic, nor even specifically addressed to you.  Let's stay focused.
>

Sending out this patch, not only do I want the community's say in using advise()
in add.c but using it in general in more locations where hints are
displayed using
printf() or any other similar function. So when you pointed out that
it's not supposed
to be used without checking the corresponding configuration variable,
I had a similar
thought to yours, that it can be improved.
Accordingly I might be interested in looking in to this once I finish
what I have in hand.

Thanks,

Heba

Re: [PATCH 1/1] add: use advise function to display hints

[Emily Shaffer]

On Thu, Jan 02, 2020 at 11:54:11AM -0800, Junio C Hamano wrote:
> "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > Use the advise function in advice.c to display hints to the users, as
> > it provides a neat and a standard format for hint messages, i.e: the
> > text is colored in yellow and the line starts by the word "hint:".
> 
> Use of advise() function is good for giving hints not just due to
> its yellow coloring (which by the way I find not very readable,
> perhaps because I use black ink on white paper).  One good thing in
> using the advise() API is that the messages can also be squelched
> with advice.* configuration variables.
> 
> And these two hints in "git add" are good chandidates to make
> customizable (perhaps with "advice.addNothing"), so I tend to agree
> with you that it makes sense to move these two messages to advise().
> Unfortunately this patch goes only halfway and stops (see below).
> 
> 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.

Maybe this is my C++ habits not dying when they should :) but to me,
this begs the question, "why doesn't advise() check the toggles for me?"

Are advice messages 1:1 with advice settings? Is there a reason that
advise() doesn't look up its corresponding config for itself?

 - Emily

> 
> >
> > Signed-off-by: Heba Waly <heba.waly@gmail.com>
> > ---
> >  builtin/add.c  | 4 ++--
> >  t/t3700-add.sh | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/add.c b/builtin/add.c
> > index 4c38aff419..eebf8d772b 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -390,7 +390,7 @@ static int add_files(struct dir_struct *dir, int flags)
> >  		fprintf(stderr, _(ignore_error));
> >  		for (i = 0; i < dir->ignored_nr; i++)
> >  			fprintf(stderr, "%s\n", dir->ignored[i]->name);
> > -		fprintf(stderr, _("Use -f if you really want to add them.\n"));
> > +		advise(_("Use -f if you really want to add them.\n"));
> >  		exit_status = 1;
> >  	}
> >  
> > @@ -480,7 +480,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> >  
> >  	if (require_pathspec && pathspec.nr == 0) {
> >  		fprintf(stderr, _("Nothing specified, nothing added.\n"));
> > -		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
> > +		advise( _("Maybe you wanted to say 'git add .'?\n"));
> >  		return 0;
> >  	}
> 
> The final code for the above part would look like:
> 
> 		if (advice_add_nothing)
> 			advise(_("Use -f if you really want to add them."));
> 		...
> 		if (advice_add_nothing)
> 			advise( _("Maybe you wanted to say 'git add .'?"));
> 

Hm, I guess this answers my question above about them being 1:1. But I
suppose it doesn't necessarily preclude advise() from associating a
single config with multiple advice messages.

 - Emily

Re: [PATCH 1/1] add: use advise function to display hints

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> Hm, I guess this answers my question above about them being 1:1. But I
> suppose it doesn't necessarily preclude advise() from associating a
> single config with multiple advice messages.

... and probably the other message in the thread from me would
answer any remaining question you may have, I guess ;-)

Re: [PATCH 1/1] add: use advise function to display hints

[Heba Waly]

On Fri, Jan 3, 2020 at 8:54 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > Use the advise function in advice.c to display hints to the users, as
> > it provides a neat and a standard format for hint messages, i.e: the
> > text is colored in yellow and the line starts by the word "hint:".
>
> Use of advise() function is good for giving hints not just due to
> its yellow coloring (which by the way I find not very readable,
> perhaps because I use black ink on white paper).  One good thing in
> using the advise() API is that the messages can also be squelched
> with advice.* configuration variables.
>

Got it, thanks.

> And these two hints in "git add" are good chandidates to make
> customizable (perhaps with "advice.addNothing"), so I tend to agree
> with you that it makes sense to move these two messages to advise().
> Unfortunately this patch goes only halfway and stops (see below).
>
> 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.
>

Ok, we can address that in a separate patch.

> >
> > Signed-off-by: Heba Waly <heba.waly@gmail.com>
> > ---
> >  builtin/add.c  | 4 ++--
> >  t/t3700-add.sh | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/add.c b/builtin/add.c
> > index 4c38aff419..eebf8d772b 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -390,7 +390,7 @@ static int add_files(struct dir_struct *dir, int flags)
> >               fprintf(stderr, _(ignore_error));
> >               for (i = 0; i < dir->ignored_nr; i++)
> >                       fprintf(stderr, "%s\n", dir->ignored[i]->name);
> > -             fprintf(stderr, _("Use -f if you really want to add them.\n"));
> > +             advise(_("Use -f if you really want to add them.\n"));
> >               exit_status = 1;
> >       }
> >
> > @@ -480,7 +480,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> >
> >       if (require_pathspec && pathspec.nr == 0) {
> >               fprintf(stderr, _("Nothing specified, nothing added.\n"));
> > -             fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
> > +             advise( _("Maybe you wanted to say 'git add .'?\n"));
> >               return 0;
> >       }
>
> The final code for the above part would look like:
>
>                 if (advice_add_nothing)
>                         advise(_("Use -f if you really want to add them."));
>                 ...
>                 if (advice_add_nothing)
>                         advise( _("Maybe you wanted to say 'git add .'?"));
>
> and then you would
>
>  * add defn of advice_add_nothing to advice.h
>  * add decl of the same, initialized to 1(true), to advice.c
>  * map "addNothing" to &advice_add_nothing in advice.c::advice_config[]
>
> to complete the other half of this patch, if the config we choose to
> use is named "advice.addNothing".
>

Understood.


> By the way, notice that the single-liner advise() messages do not
> end with LF?  This is another difference between printf() family and
> advise().  advise() cuts its message at LF and prefixes each piece
> with "hint:" but after the final LF there is nothing but NUL, which
> means the final LF is optional.
>
> The warning()/error()/die() family is different from advise() in
> that they do not chop the incoming message at LF.  This behaviour is
> less i18n friendly, and it would be nice to eventually change them
> to behave similarly to advise().
>

Thank you for the extra tip.

> Thanks.
>
>

Heba

Re: [PATCH 1/1] add: use advise function to display hints

[Emily Shaffer]

On Thu, Jan 02, 2020 at 03:04:01AM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
> 
> @@ -390,7 +390,7 @@ static int add_files(struct dir_struct *dir, int flags)
>  		fprintf(stderr, _(ignore_error));
>  		for (i = 0; i < dir->ignored_nr; i++)
>  			fprintf(stderr, "%s\n", dir->ignored[i]->name);
> -		fprintf(stderr, _("Use -f if you really want to add them.\n"));
> +		advise(_("Use -f if you really want to add them.\n"));

In the vein of the rest of your project, for me I'd rather see a
copy-pasteable response here:

"Use 'git add -f " + name + "' if you really want to add them."

That is, if you know the name of the file that was being added here, you
could provide it so the user can simply copy and go, rather than
retyping.


 - Emily

Re: [PATCH 1/1] add: use advise function to display hints

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> On Thu, Jan 02, 2020 at 03:04:01AM +0000, Heba Waly via GitGitGadget wrote:
>> From: Heba Waly <heba.waly@gmail.com>
>> 
>> @@ -390,7 +390,7 @@ static int add_files(struct dir_struct *dir, int flags)
>>  		fprintf(stderr, _(ignore_error));
>>  		for (i = 0; i < dir->ignored_nr; i++)
>>  			fprintf(stderr, "%s\n", dir->ignored[i]->name);
>> -		fprintf(stderr, _("Use -f if you really want to add them.\n"));
>> +		advise(_("Use -f if you really want to add them.\n"));
>
> In the vein of the rest of your project, for me I'd rather see a
> copy-pasteable response here:
>
> "Use 'git add -f " + name + "' if you really want to add them."
>
> That is, if you know the name of the file that was being added here, you
> could provide it so the user can simply copy and go, rather than
> retyping.

Just being a devil's advocate, but you are opening a can of worms by
suggesting so---the path needs to be quoted proporly (and the way to
do so may be different depending on the shell in use), for example.

[PATCH v2 0/1] [Outreachy] add: use advise function to display hints

[Heba Waly via GitGitGadget]

The advise function in advice.c provides a neat and a standard format for
hint messages, i.e: the text is colored in yellow and the line starts by the
word "hint:". Also this will allow us to control the hint messages based on
advice.* configuration variables.

This patch suggests using this advise function whenever displaying hints to
improve the user experience, as the user's eyes will get used to the format
and will scan the screen for the yellow hints whenever confused instead of
reading all the output lines looking for advice.

Heba Waly (1):
  add: use advise function to display hints

 advice.c       | 2 ++
 advice.h       | 1 +
 builtin/add.c  | 6 ++++--
 t/t3700-add.sh | 2 +-
 4 files changed, 8 insertions(+), 3 deletions(-)


base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-508%2FHebaWaly%2Fformatting_hints-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-508/HebaWaly/formatting_hints-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/508

Range-diff vs v1:

 1:  90608636bf ! 1:  9f9febd3f4 add: use advise function to display hints
     @@ -6,8 +6,43 @@
          it provides a neat and a standard format for hint messages, i.e: the
          text is colored in yellow and the line starts by the word "hint:".
      
     +    Also this will enable us to control the messages using advice.*
     +    configuration variables.
     +
          Signed-off-by: Heba Waly <heba.waly@gmail.com>
      
     + diff --git a/advice.c b/advice.c
     + --- a/advice.c
     + +++ b/advice.c
     +@@
     + int advice_checkout_ambiguous_remote_branch_name = 1;
     + int advice_nested_tag = 1;
     + int advice_submodule_alternate_error_strategy_die = 1;
     ++int advice_add_nothing = 1;
     + 
     + static int advice_use_color = -1;
     + static char advice_colors[][COLOR_MAXLEN] = {
     +@@
     + 	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
     + 	{ "nestedTag", &advice_nested_tag },
     + 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
     ++	{ "addNothing", &advice_add_nothing },
     + 
     + 	/* make this an alias for backward compatibility */
     + 	{ "pushNonFastForward", &advice_push_update_rejected }
     +
     + diff --git a/advice.h b/advice.h
     + --- a/advice.h
     + +++ b/advice.h
     +@@
     + extern int advice_checkout_ambiguous_remote_branch_name;
     + extern int advice_nested_tag;
     + extern int advice_submodule_alternate_error_strategy_die;
     ++extern int advice_add_nothing;
     + 
     + int git_default_advice_config(const char *var, const char *value);
     + __attribute__((format (printf, 1, 2)))
     +
       diff --git a/builtin/add.c b/builtin/add.c
       --- a/builtin/add.c
       +++ b/builtin/add.c
     @@ -16,7 +51,8 @@
       		for (i = 0; i < dir->ignored_nr; i++)
       			fprintf(stderr, "%s\n", dir->ignored[i]->name);
      -		fprintf(stderr, _("Use -f if you really want to add them.\n"));
     -+		advise(_("Use -f if you really want to add them.\n"));
     ++		if (advice_add_nothing)
     ++			advise(_("Use -f if you really want to add them.\n"));
       		exit_status = 1;
       	}
       
     @@ -25,7 +61,8 @@
       	if (require_pathspec && pathspec.nr == 0) {
       		fprintf(stderr, _("Nothing specified, nothing added.\n"));
      -		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
     -+		advise( _("Maybe you wanted to say 'git add .'?\n"));
     ++		if (advice_add_nothing)
     ++			advise( _("Maybe you wanted to say 'git add .'?\n"));
       		return 0;
       	}
       

-- 
gitgitgadget

[PATCH v2 1/1] add: use advise function to display hints

[Heba Waly via GitGitGadget]

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

Use the advise function in advice.c to display hints to the users, as
it provides a neat and a standard format for hint messages, i.e: the
text is colored in yellow and the line starts by the word "hint:".

Also this will enable us to control the messages using advice.*
configuration variables.

Signed-off-by: Heba Waly <heba.waly@gmail.com>
---
 advice.c       | 2 ++
 advice.h       | 1 +
 builtin/add.c  | 6 ++++--
 t/t3700-add.sh | 2 +-
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/advice.c b/advice.c
index 249c60dcf3..098ac0abea 100644
--- a/advice.c
+++ b/advice.c
@@ -31,6 +31,7 @@ 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;
+int advice_add_nothing = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -91,6 +92,7 @@ static struct {
 	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
 	{ "nestedTag", &advice_nested_tag },
 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
+	{ "addNothing", &advice_add_nothing },
 
 	/* make this an alias for backward compatibility */
 	{ "pushNonFastForward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index b706780614..83287b0594 100644
--- a/advice.h
+++ b/advice.h
@@ -31,6 +31,7 @@ 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;
+extern int advice_add_nothing;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/builtin/add.c b/builtin/add.c
index 4c38aff419..57b3186f69 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -390,7 +390,8 @@ static int add_files(struct dir_struct *dir, int flags)
 		fprintf(stderr, _(ignore_error));
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
-		fprintf(stderr, _("Use -f if you really want to add them.\n"));
+		if (advice_add_nothing)
+			advise(_("Use -f if you really want to add them.\n"));
 		exit_status = 1;
 	}
 
@@ -480,7 +481,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
+		if (advice_add_nothing)
+			advise( _("Maybe you wanted to say 'git add .'?\n"));
 		return 0;
 	}
 
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index c325167b90..a649805369 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -326,7 +326,7 @@ test_expect_success 'git add --dry-run of an existing file output' "
 cat >expect.err <<\EOF
 The following paths are ignored by one of your .gitignore files:
 ignored-file
-Use -f if you really want to add them.
+hint: Use -f if you really want to add them.
 EOF
 cat >expect.out <<\EOF
 add 'track-this'
-- 
gitgitgadget

Re: [PATCH v2 1/1] add: use advise function to display hints

[Emily Shaffer]

On Tue, Jan 07, 2020 at 11:12:32PM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
> 
> Use the advise function in advice.c to display hints to the users, as
> it provides a neat and a standard format for hint messages, i.e: the
> text is colored in yellow and the line starts by the word "hint:".
> 
> Also this will enable us to control the messages using advice.*
> configuration variables.

Looks like this slipped through the cracks over the holidays. Sorry! :)

> 
> Signed-off-by: Heba Waly <heba.waly@gmail.com>
> ---
>  advice.c       | 2 ++
>  advice.h       | 1 +
>  builtin/add.c  | 6 ++++--
>  t/t3700-add.sh | 2 +-
>  4 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/advice.c b/advice.c
> index 249c60dcf3..098ac0abea 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -31,6 +31,7 @@ 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;
> +int advice_add_nothing = 1;

Here's the global advice setting we can look at.

>  
>  static int advice_use_color = -1;
>  static char advice_colors[][COLOR_MAXLEN] = {
> @@ -91,6 +92,7 @@ static struct {
>  	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
>  	{ "nestedTag", &advice_nested_tag },
>  	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
> +	{ "addNothing", &advice_add_nothing },

Here's the name of the advice config, e.g. advice.addNothing.

Hmm, I wonder if addNothing really makes sense/is understandable when
I'm configuring? I see two cases you're addressing; first, adding an
ignored file ("Use -f if you really want to add") - which "addNothing"
doesn't really make sense for - and second, "add" with nothing
specified ("did you mean 'git add .'"), where "addNothing" makes sense
in context. Out of context though, perhaps "hint.addIgnoredFile" and
"hint.addEmptyPathspec" make more sense? Of course naming is one of the
two hardest problems in computer science (next to race conditions and
off-by-one errors) so probably someone else can suggest a better name :)

>  
>  	/* make this an alias for backward compatibility */
>  	{ "pushNonFastForward", &advice_push_update_rejected }
> diff --git a/advice.h b/advice.h
> index b706780614..83287b0594 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -31,6 +31,7 @@ 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;
> +extern int advice_add_nothing;
>  
>  int git_default_advice_config(const char *var, const char *value);
>  __attribute__((format (printf, 1, 2)))
> diff --git a/builtin/add.c b/builtin/add.c
> index 4c38aff419..57b3186f69 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -390,7 +390,8 @@ static int add_files(struct dir_struct *dir, int flags)
>  		fprintf(stderr, _(ignore_error));
>  		for (i = 0; i < dir->ignored_nr; i++)
>  			fprintf(stderr, "%s\n", dir->ignored[i]->name);
> -		fprintf(stderr, _("Use -f if you really want to add them.\n"));
> +		if (advice_add_nothing)
> +			advise(_("Use -f if you really want to add them.\n"));

Here's where we add the guard, and use the new config.

As mentioned earlier, I'm not sure that tying this advice to the same
config as the next one you change really makes sense.

Nitwise, it's somewhat common for advice hints to also tell you how to
disable them; see sha1-name.c:get_oid_basic's 'object_name_msg' for an
example.

>  		exit_status = 1;
>  	}
>  
> @@ -480,7 +481,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  
>  	if (require_pathspec && pathspec.nr == 0) {
>  		fprintf(stderr, _("Nothing specified, nothing added.\n"));
> -		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
> +		if (advice_add_nothing)
> +			advise( _("Maybe you wanted to say 'git add .'?\n"));

Same nit as above.

>  		return 0;
>  	}
>  
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index c325167b90..a649805369 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -326,7 +326,7 @@ test_expect_success 'git add --dry-run of an existing file output' "
>  cat >expect.err <<\EOF
>  The following paths are ignored by one of your .gitignore files:
>  ignored-file
> -Use -f if you really want to add them.
> +hint: Use -f if you really want to add them.
>  EOF
>  cat >expect.out <<\EOF
>  add 'track-this'
> -- 
> gitgitgadget

Finally, you'd better update Documentation/config/advice.txt too.

Re: [PATCH v2 1/1] add: use advise function to display hints

[Heba Waly]

On Tue, Jan 28, 2020 at 12:52 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> Hmm, I wonder if addNothing really makes sense/is understandable when
> I'm configuring? I see two cases you're addressing; first, adding an
> ignored file ("Use -f if you really want to add") - which "addNothing"
> doesn't really make sense for - and second, "add" with nothing
> specified ("did you mean 'git add .'"), where "addNothing" makes sense
> in context. Out of context though, perhaps "hint.addIgnoredFile" and
> "hint.addEmptyPathspec" make more sense? Of course naming is one of the
> two hardest problems in computer science (next to race conditions and
> off-by-one errors) so probably someone else can suggest a better name :)
>

I agree, as this patch was my first interaction with the advice
library, but now after many discussions on different threads it makes
more sense to add two config variables for the two messages.

>
> As mentioned earlier, I'm not sure that tying this advice to the same
> config as the next one you change really makes sense.
>
> Nitwise, it's somewhat common for advice hints to also tell you how to
> disable them; see sha1-name.c:get_oid_basic's 'object_name_msg' for an
> example.
>

I can see that this was followed in only three locations around the
code base, which means that not telling the user how to disable the
hint is more common.
Initially I tended to think of it as noise as I suspect the user will
ignore this extra line about disabling the message more often. But
after taking a second look at Documentation/config/advice.txt I
realized how hard it will be for the user to find the corresponding
configuration variable to the message that he/she would like to turn
off,
specially when the list is getting longer. So seems like displaying
the extra note will make the user's life easier *when* s/he wants to
turn it off.

> >               exit_status = 1;
> >       }
> >
> > @@ -480,7 +481,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> >
> >       if (require_pathspec && pathspec.nr == 0) {
> >               fprintf(stderr, _("Nothing specified, nothing added.\n"));
> > -             fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
> > +             if (advice_add_nothing)
> > +                     advise( _("Maybe you wanted to say 'git add .'?\n"));
>
> Same nit as above.
>
> >               return 0;
> >       }
> >
> > diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> > index c325167b90..a649805369 100755
> > --- a/t/t3700-add.sh
> > +++ b/t/t3700-add.sh
> > @@ -326,7 +326,7 @@ test_expect_success 'git add --dry-run of an existing file output' "
> >  cat >expect.err <<\EOF
> >  The following paths are ignored by one of your .gitignore files:
> >  ignored-file
> > -Use -f if you really want to add them.
> > +hint: Use -f if you really want to add them.
> >  EOF
> >  cat >expect.out <<\EOF
> >  add 'track-this'
> > --
> > gitgitgadget
>
> Finally, you'd better update Documentation/config/advice.txt too.

Yeah, got that on my todo list :)

Thanks,
Heba

Re: [PATCH v2 1/1] add: use advise function to display hints

[Jonathan Tan]

> From: Heba Waly <heba.waly@gmail.com>
> 
> Use the advise function in advice.c to display hints to the users, as
> it provides a neat and a standard format for hint messages, i.e: the
> text is colored in yellow and the line starts by the word "hint:".
> 
> Also this will enable us to control the messages using advice.*
> configuration variables.

Firstly, sorry for getting back to this so late.

As written, this gives me the impression that advise() is what enables
us to control the messages using configuration variables, but that's not
true - that's done by a separate mechanism in advise.c and .h.
Paraphrasing what Junio wrote [1], the commit message might be better
written as:

  In the "add" command, use the advice API instead of fprintf() for the
  hint shown when nothing was added. Thus, this hint message follows the
  standard hint message format, and its visibility is made configurable.

(Note that I mentioned the "add" command and called it the advice API
instead of the advise() function.)

(Feel free to use this or write your own.)

[1] https://lore.kernel.org/git/xmqqpng1eisc.fsf@gitster-ct.c.googlers.com/

> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index c325167b90..a649805369 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -326,7 +326,7 @@ test_expect_success 'git add --dry-run of an existing file output' "
>  cat >expect.err <<\EOF
>  The following paths are ignored by one of your .gitignore files:
>  ignored-file
> -Use -f if you really want to add them.
> +hint: Use -f if you really want to add them.
>  EOF
>  cat >expect.out <<\EOF
>  add 'track-this'

Also add a test that checks what happens if advice.addNothing is set.
(It seems that we generally don't test what happens when advice is
suppressed. If we consider solely this patch, I'm on the fence of the
usefulness of this test, but if we plan to refactor the advise()
function to take care of checking the config variable itself, for
example, then we will need such a test anyway, so I think we might as
well include at least one such advice test now.)

Re: [PATCH v2 1/1] add: use advise function to display hints

[Heba Waly]

On Tue, Jan 28, 2020 at 1:00 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > Use the advise function in advice.c to display hints to the users, as
> > it provides a neat and a standard format for hint messages, i.e: the
> > text is colored in yellow and the line starts by the word "hint:".
> >
> > Also this will enable us to control the messages using advice.*
> > configuration variables.
>
> Firstly, sorry for getting back to this so late.
>
> As written, this gives me the impression that advise() is what enables
> us to control the messages using configuration variables, but that's not
> true - that's done by a separate mechanism in advise.c and .h.
> Paraphrasing what Junio wrote [1], the commit message might be better
> written as:
>
>   In the "add" command, use the advice API instead of fprintf() for the
>   hint shown when nothing was added. Thus, this hint message follows the
>   standard hint message format, and its visibility is made configurable.
>
> (Note that I mentioned the "add" command and called it the advice API
> instead of the advise() function.)
>

That makes sense.

> (Feel free to use this or write your own.)
>
> [1] https://lore.kernel.org/git/xmqqpng1eisc.fsf@gitster-ct.c.googlers.com/
>
> > diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> > index c325167b90..a649805369 100755
> > --- a/t/t3700-add.sh
> > +++ b/t/t3700-add.sh
> > @@ -326,7 +326,7 @@ test_expect_success 'git add --dry-run of an existing file output' "
> >  cat >expect.err <<\EOF
> >  The following paths are ignored by one of your .gitignore files:
> >  ignored-file
> > -Use -f if you really want to add them.
> > +hint: Use -f if you really want to add them.
> >  EOF
> >  cat >expect.out <<\EOF
> >  add 'track-this'
>
> Also add a test that checks what happens if advice.addNothing is set.
> (It seems that we generally don't test what happens when advice is
> suppressed. If we consider solely this patch, I'm on the fence of the
> usefulness of this test, but if we plan to refactor the advise()
> function to take care of checking the config variable itself, for
> example, then we will need such a test anyway, so I think we might as
> well include at least one such advice test now.)

I'm tempted to say let's worry about it when refactoring advise(),
maybe then we'll
find a more suitable place for this test. as it'll be advice-related,
not caller-related.

Thanks,
Heba

[PATCH v3] add: use advice API to display hints

[Heba Waly via GitGitGadget]

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

In the "add" command, use the advice API to display hints to users,
as it provides a neat and a standard format for hint messages, and
the message visibility will be configurable.

Signed-off-by: Heba Waly <heba.waly@gmail.com>
---
    [Outreachy] add: use advise API to display hints
    
    In the "add" command, use the advice API to display hints to users, as
    it provides a neat and a standard format for hint messages, and the
    message visibility will be configurable.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-508%2FHebaWaly%2Fformatting_hints-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-508/HebaWaly/formatting_hints-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/508

Range-diff vs v2:

 1:  9f9febd3f4 ! 1:  410a66953d add: use advise function to display hints
     @@ -1,16 +1,28 @@
      Author: Heba Waly <heba.waly@gmail.com>
      
     -    add: use advise function to display hints
     +    add: use advice API to display hints
      
     -    Use the advise function in advice.c to display hints to the users, as
     -    it provides a neat and a standard format for hint messages, i.e: the
     -    text is colored in yellow and the line starts by the word "hint:".
     -
     -    Also this will enable us to control the messages using advice.*
     -    configuration variables.
     +    In the "add" command, use the advice API to display hints to users,
     +    as it provides a neat and a standard format for hint messages, and
     +    the message visibility will be configurable.
      
          Signed-off-by: Heba Waly <heba.waly@gmail.com>
      
     + diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
     + --- a/Documentation/config/advice.txt
     + +++ b/Documentation/config/advice.txt
     +@@
     + 	submoduleAlternateErrorStrategyDie:
     + 		Advice shown when a submodule.alternateErrorStrategy option
     + 		configured to "die" causes a fatal error.
     ++	addIgnoredFile::
     ++		Advice shown if a user attempts to add an ignored file to
     ++		the index.
     ++	addEmptyPathspec::
     ++		Advice shown if a user runs the add command without providing
     ++		the pathspec parameter.
     + --
     +
       diff --git a/advice.c b/advice.c
       --- a/advice.c
       +++ b/advice.c
     @@ -18,7 +30,8 @@
       int advice_checkout_ambiguous_remote_branch_name = 1;
       int advice_nested_tag = 1;
       int advice_submodule_alternate_error_strategy_die = 1;
     -+int advice_add_nothing = 1;
     ++int advice_add_ignored_file = 1;
     ++int advice_add_empty_pathspec = 1;
       
       static int advice_use_color = -1;
       static char advice_colors[][COLOR_MAXLEN] = {
     @@ -26,7 +39,8 @@
       	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
       	{ "nestedTag", &advice_nested_tag },
       	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
     -+	{ "addNothing", &advice_add_nothing },
     ++	{ "addIgnoredFile", &advice_add_ignored_file },
     ++	{ "addEmptyPathspec", &advice_add_empty_pathspec },
       
       	/* make this an alias for backward compatibility */
       	{ "pushNonFastForward", &advice_push_update_rejected }
     @@ -38,7 +52,8 @@
       extern int advice_checkout_ambiguous_remote_branch_name;
       extern int advice_nested_tag;
       extern int advice_submodule_alternate_error_strategy_die;
     -+extern int advice_add_nothing;
     ++extern int advice_add_ignored_file;
     ++extern int advice_add_empty_pathspec;
       
       int git_default_advice_config(const char *var, const char *value);
       __attribute__((format (printf, 1, 2)))
     @@ -51,8 +66,10 @@
       		for (i = 0; i < dir->ignored_nr; i++)
       			fprintf(stderr, "%s\n", dir->ignored[i]->name);
      -		fprintf(stderr, _("Use -f if you really want to add them.\n"));
     -+		if (advice_add_nothing)
     -+			advise(_("Use -f if you really want to add them.\n"));
     ++		if (advice_add_ignored_file)
     ++			advise(_("Use -f if you really want to add them.\n"
     ++				 "Turn this message off by running\n"
     ++				 "\"git config advice.addIgnoredFile false\""));
       		exit_status = 1;
       	}
       
     @@ -61,8 +78,10 @@
       	if (require_pathspec && pathspec.nr == 0) {
       		fprintf(stderr, _("Nothing specified, nothing added.\n"));
      -		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
     -+		if (advice_add_nothing)
     -+			advise( _("Maybe you wanted to say 'git add .'?\n"));
     ++		if (advice_add_empty_pathspec)
     ++			advise( _("Maybe you wanted to say 'git add .'?\n"
     ++				  "Turn this message off by running\n"
     ++				  "\"git config advice.addEmptyPathspec false\""));
       		return 0;
       	}
       
     @@ -76,6 +95,8 @@
       ignored-file
      -Use -f if you really want to add them.
      +hint: Use -f if you really want to add them.
     ++hint: Turn this message off by running
     ++hint: "git config advice.addIgnoredFile false"
       EOF
       cat >expect.out <<\EOF
       add 'track-this'


 Documentation/config/advice.txt |  6 ++++++
 advice.c                        |  4 ++++
 advice.h                        |  2 ++
 builtin/add.c                   | 10 ++++++++--
 t/t3700-add.sh                  |  4 +++-
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index d4e698cd3f..a72615c68d 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -110,4 +110,10 @@ advice.*::
 	submoduleAlternateErrorStrategyDie:
 		Advice shown when a submodule.alternateErrorStrategy option
 		configured to "die" causes a fatal error.
+	addIgnoredFile::
+		Advice shown if a user attempts to add an ignored file to
+		the index.
+	addEmptyPathspec::
+		Advice shown if a user runs the add command without providing
+		the pathspec parameter.
 --
diff --git a/advice.c b/advice.c
index 249c60dcf3..97f3f981b4 100644
--- a/advice.c
+++ b/advice.c
@@ -31,6 +31,8 @@ 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;
+int advice_add_ignored_file = 1;
+int advice_add_empty_pathspec = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -91,6 +93,8 @@ static struct {
 	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
 	{ "nestedTag", &advice_nested_tag },
 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
+	{ "addIgnoredFile", &advice_add_ignored_file },
+	{ "addEmptyPathspec", &advice_add_empty_pathspec },
 
 	/* make this an alias for backward compatibility */
 	{ "pushNonFastForward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index b706780614..0e6e58d9f8 100644
--- a/advice.h
+++ b/advice.h
@@ -31,6 +31,8 @@ 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;
+extern int advice_add_ignored_file;
+extern int advice_add_empty_pathspec;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/builtin/add.c b/builtin/add.c
index 4c38aff419..37b6cbac53 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -390,7 +390,10 @@ static int add_files(struct dir_struct *dir, int flags)
 		fprintf(stderr, _(ignore_error));
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
-		fprintf(stderr, _("Use -f if you really want to add them.\n"));
+		if (advice_add_ignored_file)
+			advise(_("Use -f if you really want to add them.\n"
+				 "Turn this message off by running\n"
+				 "\"git config advice.addIgnoredFile false\""));
 		exit_status = 1;
 	}
 
@@ -480,7 +483,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		fprintf(stderr, _("Maybe you wanted to say 'git add .'?\n"));
+		if (advice_add_empty_pathspec)
+			advise( _("Maybe you wanted to say 'git add .'?\n"
+				  "Turn this message off by running\n"
+				  "\"git config advice.addEmptyPathspec false\""));
 		return 0;
 	}
 
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index c325167b90..88bc799807 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -326,7 +326,9 @@ test_expect_success 'git add --dry-run of an existing file output' "
 cat >expect.err <<\EOF
 The following paths are ignored by one of your .gitignore files:
 ignored-file
-Use -f if you really want to add them.
+hint: Use -f if you really want to add them.
+hint: Turn this message off by running
+hint: "git config advice.addIgnoredFile false"
 EOF
 cat >expect.out <<\EOF
 add 'track-this'

base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
-- 
gitgitgadget

Re: [PATCH v3] add: use advice API to display hints

[Junio C Hamano]

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

> From: Heba Waly <heba.waly@gmail.com>
>
> In the "add" command, use the advice API to display hints to users,
> as it provides a neat and a standard format for hint messages, and
> the message visibility will be configurable.
>
> Signed-off-by: Heba Waly <heba.waly@gmail.com>
> ---
>     [Outreachy] add: use advise API to display hints
>     
>     In the "add" command, use the advice API to display hints to users, as
>     it provides a neat and a standard format for hint messages, and the
>     message visibility will be configurable.

The topic has been in 'next' for the past week or so already.  If we
need to make further changes, please do so incrementally.

Thanks.

Re: [PATCH v3] add: use advice API to display hints

[Heba Waly]

On Fri, Jan 31, 2020 at 10:59 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > In the "add" command, use the advice API to display hints to users,
> > as it provides a neat and a standard format for hint messages, and
> > the message visibility will be configurable.
> >
> > Signed-off-by: Heba Waly <heba.waly@gmail.com>
> > ---
> >     [Outreachy] add: use advise API to display hints
> >
> >     In the "add" command, use the advice API to display hints to users, as
> >     it provides a neat and a standard format for hint messages, and the
> >     message visibility will be configurable.
>
> The topic has been in 'next' for the past week or so already.  If we
> need to make further changes, please do so incrementally.
>

Will do, thanks.

> Thanks.

Re: [PATCH v3] add: use advice API to display hints

[Junio C Hamano]

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

> On Fri, Jan 31, 2020 at 10:59 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>> > From: Heba Waly <heba.waly@gmail.com>
>> >
>> > In the "add" command, use the advice API to display hints to users,
>> > as it provides a neat and a standard format for hint messages, and
>> > the message visibility will be configurable.
>> >
>> > Signed-off-by: Heba Waly <heba.waly@gmail.com>
>> > ---
>> >     [Outreachy] add: use advise API to display hints
>> >
>> >     In the "add" command, use the advice API to display hints to users, as
>> >     it provides a neat and a standard format for hint messages, and the
>> >     message visibility will be configurable.
>>
>> The topic has been in 'next' for the past week or so already.  If we
>> need to make further changes, please do so incrementally.
>
> Will do, thanks.

I was reviewing the draft of the "What's cooking" report and noticed
that this update is not there---did I miss one?

Thanks.

Re: [PATCH v3] add: use advice API to display hints

[Heba Waly]

No, I agreed with my mentors to wait on this update until that branch
is merged in master.
So no need to worry about it.

Thanks,
Heba

On Thu, Feb 6, 2020 at 10:18 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Heba Waly <heba.waly@gmail.com> writes:
>
> > On Fri, Jan 31, 2020 at 10:59 AM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >>
> >> > From: Heba Waly <heba.waly@gmail.com>
> >> >
> >> > In the "add" command, use the advice API to display hints to users,
> >> > as it provides a neat and a standard format for hint messages, and
> >> > the message visibility will be configurable.
> >> >
> >> > Signed-off-by: Heba Waly <heba.waly@gmail.com>
> >> > ---
> >> >     [Outreachy] add: use advise API to display hints
> >> >
> >> >     In the "add" command, use the advice API to display hints to users, as
> >> >     it provides a neat and a standard format for hint messages, and the
> >> >     message visibility will be configurable.
> >>
> >> The topic has been in 'next' for the past week or so already.  If we
> >> need to make further changes, please do so incrementally.
> >
> > Will do, thanks.
>
> I was reviewing the draft of the "What's cooking" report and noticed
> that this update is not there---did I miss one?
>
> Thanks.

Re: [PATCH v3] add: use advice API to display hints

[Junio C Hamano]

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

> No, I agreed with my mentors to wait on this update until that branch
> is merged in master.

The users will first has to set advise.addnothing and then later has
to set something different if you do so, no?

I do not think that is a good decision, and I am not happy to see
people making such a decision that would hurt our users off list.

> So no need to worry about it.

Yes, I do have to worry about our users.

Re: [PATCH v3] add: use advice API to display hints

[Heba Waly]

On Thu, Feb 6, 2020 at 11:18 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Heba Waly <heba.waly@gmail.com> writes:
>
> > No, I agreed with my mentors to wait on this update until that branch
> > is merged in master.
>
> The users will first has to set advise.addnothing and then later has
> to set something different if you do so, no?
>

You're right, I missed that point, will send an update based on the
pickup branch shortly.

Thanks,
Heba

Re: [PATCH v3] add: use advice API to display hints

[Junio C Hamano]

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

> On Thu, Feb 6, 2020 at 11:18 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Heba Waly <heba.waly@gmail.com> writes:
>>
>> > No, I agreed with my mentors to wait on this update until that branch
>> > is merged in master.
>>
>> The users will first has to set advise.addnothing and then later has
>> to set something different if you do so, no?
>>
>
> You're right, I missed that point, will send an update based on the
> pickup branch shortly.

Thanks.  

I'll keep the topic in 'next' while letting some handful of other
topics graduate to 'master' in today's integration cycle.  Hopefully
it can join 'master' shortly.\