threads / rfc / 52549

patch[Outreachy] [RFC] add: use advise function to display hints

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

## tl;dr

26 messages between Jan 2, 2020 and Feb 5, 2020. Diffs are folded; open one to read it.

replies: 25people: 4as markdown or json

Heba Waly via GitGitGadget· Jan 2, 2020, 03:04 UTC · lore

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
Heba Waly via GitGitGadget· Jan 2, 2020, 03:04 UTC · re: Heba Waly via GitGitGadget · lore

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

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(-)
Show changes to 2 files +3 −3

builtin/add.c, t/t3700-add.sh

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
Junio C Hamano· Jan 2, 2020, 19:54 UTC · re: Heba Waly via GitGitGadget · lore

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

"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> 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.

Show 28 quoted lines
>
> 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.
Junio C Hamano· Jan 2, 2020, 22:47 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> 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);
	}
Heba Waly· Jan 7, 2020, 10:54 UTC · re: Junio C Hamano · lore

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

On Fri, Jan 3, 2020 at 11:48 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
>
> 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
Junio C Hamano· Jan 7, 2020, 16:35 UTC · re: Heba Waly · lore

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

Heba Waly <heba.waly@gmail.com> writes:
Show 32 quoted lines
> 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.
Heba Waly· Jan 7, 2020, 23:32 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 8, 2020 at 5:35 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
>
> 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
Emily Shaffer· Jan 6, 2020, 23:13 UTC · re: Junio C Hamano · lore

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

On Thu, Jan 02, 2020 at 11:54:11AM -0800, Junio C Hamano wrote:
Show 22 quoted lines
> "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
Show 38 quoted lines
> 
> >
> > 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
Junio C Hamano· Jan 6, 2020, 23:18 UTC · re: Emily Shaffer · lore

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

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

Heba Waly· Jan 7, 2020, 04:19 UTC · re: Junio C Hamano · lore

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

On Fri, Jan 3, 2020 at 8:54 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
>
> "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.
Show 9 quoted lines
> 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.
Show 46 quoted lines
> >
> > 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.
Show 11 quoted lines
> 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
Emily Shaffer· Jan 6, 2020, 23:07 UTC · re: Heba Waly via GitGitGadget · lore

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

On Thu, Jan 02, 2020 at 03:04:01AM +0000, Heba Waly via GitGitGadget wrote:
Show 8 quoted lines
> 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
Junio C Hamano· Jan 6, 2020, 23:13 UTC · re: Emily Shaffer · lore

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

Emily Shaffer <emilyshaffer@google.com> writes:
Show 18 quoted lines
> 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.

Heba Waly via GitGitGadget· Jan 7, 2020, 23:12 UTC · re: Heba Waly via GitGitGadget · lore

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

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
Heba Waly via GitGitGadget· Jan 7, 2020, 23:12 UTC · re: Heba Waly via GitGitGadget · lore

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

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(-)
Show changes to 4 files +8 −3

advice.c, advice.h, builtin/add.c, t/t3700-add.sh

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
Emily Shaffer· Jan 27, 2020, 23:52 UTC · re: Heba Waly via GitGitGadget · lore

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

On Tue, Jan 07, 2020 at 11:12:32PM +0000, Heba Waly via GitGitGadget wrote:
Show 8 quoted lines
> 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! :)
Show 18 quoted lines
> 
> 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.
Show 8 quoted lines
>  
>  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 :)

Show 26 quoted lines
>  
>  	/* 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.

Show 10 quoted lines
>  		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.
Show 18 quoted lines
>  		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.
Heba Waly· Jan 29, 2020, 01:09 UTC · re: Emily Shaffer · lore

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

On Tue, Jan 28, 2020 at 12:52 PM Emily Shaffer <emilyshaffer@google.com> wrote:
Show 11 quoted lines
>
> 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.

Show 8 quoted lines
>
> 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.

Show 33 quoted lines
> >               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

Jonathan Tan· Jan 28, 2020, 00:00 UTC · re: Heba Waly via GitGitGadget · lore

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

Show 8 quoted lines
> 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/
Show 13 quoted lines
> 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.)

Heba Waly· Jan 29, 2020, 02:04 UTC · re: Jonathan Tan · lore

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

On Tue, Jan 28, 2020 at 1:00 PM Jonathan Tan <jonathantanmy@google.com> wrote:
Show 25 quoted lines
>
> > 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.
Show 25 quoted lines
> (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

Heba Waly via GitGitGadget· Jan 30, 2020, 01:11 UTC · re: Heba Waly via GitGitGadget · lore

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

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(-)
Show changes to 5 files +23 −3

Documentation/config/advice.txt, advice.c, advice.h, builtin/add.c, t/t3700-add.sh

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
Junio C Hamano· Jan 30, 2020, 21:59 UTC · re: Heba Waly via GitGitGadget · lore

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

"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 13 quoted lines
> 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.
Heba Waly· Jan 31, 2020, 11:16 UTC · re: Junio C Hamano · lore

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

On Fri, Jan 31, 2020 at 10:59 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
>
> "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.
Junio C Hamano· Feb 5, 2020, 21:18 UTC · re: Heba Waly · lore

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

Heba Waly <heba.waly@gmail.com> writes:
Show 22 quoted lines
> 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.
Heba Waly· Feb 5, 2020, 22:05 UTC · re: Junio C Hamano · lore

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

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:
Show 30 quoted lines
>
> 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.
Junio C Hamano· Feb 5, 2020, 22:18 UTC · re: Heba Waly · lore

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

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.
Heba Waly· Feb 5, 2020, 23:05 UTC · re: Junio C Hamano · lore

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

On Thu, Feb 6, 2020 at 11:18 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
>
> 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

Junio C Hamano· Feb 5, 2020, 23:18 UTC · re: Heba Waly · lore

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

Heba Waly <heba.waly@gmail.com> writes:
Show 13 quoted lines
> 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.\

← back to recent threads