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

[Emily Shaffer <emilyshaffer@google.com>]

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.