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

Commit Message

Linus Arver via GitGitGadget Jan. 7, 2020, 11:12 p.m. UTC

From: Heba Waly <[e-mail]>

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 <[e-mail]>
---
 advice.c       | 2 ++
 advice.h       | 1 +
 builtin/add.c  | 6 ++++--
 t/t3700-add.sh | 2 +-
 4 files changed, 8 insertions(+), 3 deletions(-)

Comments

Emily Shaffer Jan. 27, 2020, 11:52 p.m. UTC | #1

On Tue, Jan 07, 2020 at 11:12:32PM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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.

Jonathan Tan Jan. 28, 2020, midnight UTC | #2

> From: Heba Waly <[e-mail]>
> 
> 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/[e-mail]/

> 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, 1:09 a.m. UTC | #3

On Tue, Jan 28, 2020 at 12:52 PM Emily Shaffer <[e-mail]> 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

Heba Waly Jan. 29, 2020, 2:04 a.m. UTC | #4

On Tue, Jan 28, 2020 at 1:00 PM Jonathan Tan <[e-mail]> wrote:
>
> > From: Heba Waly <[e-mail]>
> >
> > 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/[e-mail]/
>
> > 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

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'