[v3] add: use advice API to display hints
diff mbox series

Message ID pull.508.v3.git.1580346702203.gitgitgadget@gmail.com
State New
Headers show
Series
  • [v3] add: use advice API to display hints
Related show

Commit Message

Han-Wen Nienhuys via GitGitGadget Jan. 30, 2020, 1:11 a.m. UTC
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(-)


base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6

Comments

Junio C Hamano Jan. 30, 2020, 9:59 p.m. UTC | #1
"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.
Heba Waly Jan. 31, 2020, 11:16 a.m. UTC | #2
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.
Junio C Hamano Feb. 5, 2020, 9:18 p.m. UTC | #3
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.
Heba Waly Feb. 5, 2020, 10:05 p.m. UTC | #4
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.
Junio C Hamano Feb. 5, 2020, 10:18 p.m. UTC | #5
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, 11:05 p.m. UTC | #6
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
Junio C Hamano Feb. 5, 2020, 11:18 p.m. UTC | #7
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.\

Patch
diff mbox series

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'