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

From: Heba Waly <heba.waly@gmail.com>
Date: Jan 29, 2020, 01:09 UTC
Message-ID: <CACg5j26DEXuxwqRYHi5UOBUpRwsu_2A9LwgyKq4qB9wxqasD7g@mail.gmail.com>
In-Reply-To: <20200127235210.GC233139@google.com>

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