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

From: Heba Waly <heba.waly@gmail.com>
To: Emily Shaffer <emilyshaffer@google.com>
Cc: Heba Waly via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 1/1] add: use advise function to display hints
Date: Wed, 29 Jan 2020 14:09:21 +1300	[thread overview]
Message-ID: <CACg5j26DEXuxwqRYHi5UOBUpRwsu_2A9LwgyKq4qB9wxqasD7g@mail.gmail.com> (raw)
In-Reply-To: <20200127235210.GC233139@google.com>

On Tue, Jan 28, 2020 at 12:52 PM Emily Shaffer <emilyshaffer@google.com> 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