Re: [PATCH 1/1] add: use advise function to display hints
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Jan 6, 2020, 23:13 UTC
- Message-ID
- <20200106231327.GB181522@google.com>
- In-Reply-To
- <xmqqpng1eisc.fsf@gitster-ct.c.googlers.com>
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