[Outreachy] add: use advise API to display hints #508

gitgitgadget · 2020-01-27T23:57:34Z

On the Git mailing list, Emily Shaffer wrote (reply to this):

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

On the Git mailing list, Heba Waly wrote (reply to this):

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

gitgitgadget · 2020-01-28T00:06:40Z

On the Git mailing list, Jonathan Tan wrote (reply to this):

> From: Heba Waly <[email protected]> > > 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/[email protected]/ > 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.)

On the Git mailing list, Heba Waly wrote (reply to this):

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

gitgitgadget · 2020-01-02T19:57:10Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Heba Waly via GitGitGadget" <[email protected]> writes: > From: Heba Waly <[email protected]> > > 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. > > Signed-off-by: Heba Waly <[email protected]> > --- > 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 .'?")); and then you would * add defn of advice_add_nothing to advice.h * add decl of the same, initialized to 1(true), to advice.c * map "addNothing" to &advice_add_nothing in advice.c::advice_config[] to complete the other half of this patch, if the config we choose to use is named "advice.addNothing". By the way, notice that the single-liner advise() messages do not end with LF? This is another difference between printf() family and advise(). advise() cuts its message at LF and prefixes each piece with "hint:" but after the final LF there is nothing but NUL, which means the final LF is optional. The warning()/error()/die() family is different from advise() in that they do not chop the incoming message at LF. This behaviour is less i18n friendly, and it would be nice to eventually change them to behave similarly to advise(). Thanks.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <[email protected]> writes: > 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. A side note. Right now, the advise() API is a bit awkweard to use correctly. When introducing a new advice message, you would * come up with advice.frotz configuration variable * define and declare advice_frotz global variable that defaults to true * sprinkle calls like this: if (advice_frotz) advise(_("helpful message about frotz")); I am wondering about two things: (1) if we can update the API so that the above can be reduced to just adding calls like: advise_ng("frotz", _("helpful message about frotz")); (2) if such a simplified advise_ng API is a good idea to begin with. There are a few advantages the current API has, but it cuts both ways. - Any new advice toggle MUST be registered to the advice.c::advice_config[] table. This table can later be extended in the future to allow a list of the toggles to be produced at runtime. This can be seen as an easy mechanism to force programmers to keep the list up to date. Or it can also be seen as the source of extra work. - advise() calls can be made without being guarded by any advice.* configuration variable. In the overly simplified advise_ng() API shown above, we cannot expresss a pattern like this: if (advice_frotz) { ... make expensive computation to ... come up with values that need to be shown ... in the advise() message char *result = expensive_computation(...); advise(_("message %s about frotz", result)); free(result); } without adding another helper function. e.g. if (advise_ng_enabled("frotz")) { char *result = expensive_computation(...); /* * advise_ng("frotz", _("message %s about frotz", result)); * is fine as well, but slightly less efficient as * it would involve another call to *_enabled(), so use * the unconditional form of the call */ advise_ng_raw(_("message %s about frotz", result)); free(result); }

On the Git mailing list, Emily Shaffer wrote (reply to this):

On Thu, Jan 02, 2020 at 11:54:11AM -0800, Junio C Hamano wrote: > "Heba Waly via GitGitGadget" <[email protected]> writes: > > > From: Heba Waly <[email protected]> > > > > 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 > > > > > Signed-off-by: Heba Waly <[email protected]> > > --- > > 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

On the Git mailing list, Junio C Hamano wrote (reply to this):

Emily Shaffer <[email protected]> writes: > 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. ... and probably the other message in the thread from me would answer any remaining question you may have, I guess ;-)

On the Git mailing list, Heba Waly wrote (reply to this):

On Fri, Jan 3, 2020 at 8:54 AM Junio C Hamano <[email protected]> wrote: > > "Heba Waly via GitGitGadget" <[email protected]> writes: > > > From: Heba Waly <[email protected]> > > > > 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. > Got it, thanks. > 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. > Ok, we can address that in a separate patch. > > > > Signed-off-by: Heba Waly <[email protected]> > > --- > > 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 .'?")); > > and then you would > > * add defn of advice_add_nothing to advice.h > * add decl of the same, initialized to 1(true), to advice.c > * map "addNothing" to &advice_add_nothing in advice.c::advice_config[] > > to complete the other half of this patch, if the config we choose to > use is named "advice.addNothing". > Understood. > By the way, notice that the single-liner advise() messages do not > end with LF? This is another difference between printf() family and > advise(). advise() cuts its message at LF and prefixes each piece > with "hint:" but after the final LF there is nothing but NUL, which > means the final LF is optional. > > The warning()/error()/die() family is different from advise() in > that they do not chop the incoming message at LF. This behaviour is > less i18n friendly, and it would be nice to eventually change them > to behave similarly to advise(). > Thank you for the extra tip. > Thanks. > > Heba

On the Git mailing list, Heba Waly wrote (reply to this):

On Fri, Jan 3, 2020 at 11:48 AM Junio C Hamano <[email protected]> wrote: > > Junio C Hamano <[email protected]> writes: > > A side note. > > Right now, the advise() API is a bit awkweard to use correctly. > When introducing a new advice message, you would > > * come up with advice.frotz configuration variable > > * define and declare advice_frotz global variable that defaults to > true > > * sprinkle calls like this: > > if (advice_frotz) > advise(_("helpful message about frotz")); > > I am wondering about two things: > > (1) if we can update the API so that the above can be reduced to > just adding calls like: > > advise_ng("frotz", _("helpful message about frotz")); > > (2) if such a simplified advise_ng API is a good idea to begin > with. > That's a valid suggestion, I can investigate that in a new patch, I'd rather keep this one as simple as calling the existing advise function. Thanks, Heba

On the Git mailing list, Junio C Hamano wrote (reply to this):

Heba Waly <[email protected]> writes: > On Fri, Jan 3, 2020 at 11:48 AM Junio C Hamano <[email protected]> wrote: >> >> Junio C Hamano <[email protected]> writes: >> >> A side note. >> >> Right now, the advise() API is a bit awkweard to use correctly. >> When introducing a new advice message, you would >> >> * come up with advice.frotz configuration variable >> >> * define and declare advice_frotz global variable that defaults to >> true >> >> * sprinkle calls like this: >> >> if (advice_frotz) >> advise(_("helpful message about frotz")); >> >> I am wondering about two things: >> >> (1) if we can update the API so that the above can be reduced to >> just adding calls like: >> >> advise_ng("frotz", _("helpful message about frotz")); >> >> (2) if such a simplified advise_ng API is a good idea to begin >> with. >> > > That's a valid suggestion, I can investigate that in a new patch, I'd rather > keep this one as simple as calling the existing advise function. Yeah, the side note wasn't even a suggestion for improving _this_ topic, nor even specifically addressed to you. Let's stay focused. Thanks.

On the Git mailing list, Heba Waly wrote (reply to this):

On Wed, Jan 8, 2020 at 5:35 AM Junio C Hamano <[email protected]> wrote: > > Heba Waly <[email protected]> writes: > > > On Fri, Jan 3, 2020 at 11:48 AM Junio C Hamano <[email protected]> wrote: > >> > >> Junio C Hamano <[email protected]> writes: > >> > > That's a valid suggestion, I can investigate that in a new patch, I'd rather > > keep this one as simple as calling the existing advise function. > > Yeah, the side note wasn't even a suggestion for improving _this_ > topic, nor even specifically addressed to you. Let's stay focused. > Sending out this patch, not only do I want the community's say in using advise() in add.c but using it in general in more locations where hints are displayed using printf() or any other similar function. So when you pointed out that it's not supposed to be used without checking the corresponding configuration variable, I had a similar thought to yours, that it can be improved. Accordingly I might be interested in looking in to this once I finish what I have in hand. Thanks, Heba

gitgitgadget · 2020-01-06T23:31:24Z

On the Git mailing list, Emily Shaffer wrote (reply to this):

On Thu, Jan 02, 2020 at 03:04:01AM +0000, Heba Waly via GitGitGadget wrote: > From: Heba Waly <[email protected]> > > @@ -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")); In the vein of the rest of your project, for me I'd rather see a copy-pasteable response here: "Use 'git add -f " + name + "' if you really want to add them." That is, if you know the name of the file that was being added here, you could provide it so the user can simply copy and go, rather than retyping. - Emily

On the Git mailing list, Junio C Hamano wrote (reply to this):

Emily Shaffer <[email protected]> writes: > On Thu, Jan 02, 2020 at 03:04:01AM +0000, Heba Waly via GitGitGadget wrote: >> From: Heba Waly <[email protected]> >> >> @@ -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")); > > In the vein of the rest of your project, for me I'd rather see a > copy-pasteable response here: > > "Use 'git add -f " + name + "' if you really want to add them." > > That is, if you know the name of the file that was being added here, you > could provide it so the user can simply copy and go, rather than > retyping. Just being a devil's advocate, but you are opening a can of worms by suggesting so---the path needs to be quoted proporly (and the way to do so may be different depending on the shell in use), for example.

-Original file line number
+Diff line change
@@ Expand Up / @@ -110,4 +110,10 @@ advice.*:: @@
     	submoduleAlternateErrorStrategyDie:
     		Advice shown when a submodule.alternateErrorStrategy option
     		configured to "die" causes a fatal error.
+    	addIgnoredFile::
+    		Advice shown if a user attempts to add an ignored file to
+    		the index.
+    	addEmptyPathspec::
+    		Advice shown if a user runs the add command without providing
+    		the pathspec parameter.
     --

-Original file line number
+Diff line change
@@ Expand Up / @@ -31,6 +31,8 @@ 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_ignored_file;
+    extern int advice_add_empty_pathspec;
     int git_default_advice_config(const char *var, const char *value);
     __attribute__((format (printf, 1, 2)))
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Outreachy] add: use advise API to display hints #508

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gitgitgadget bot Jan 27, 2020

Uh oh!

gitgitgadget bot Jan 29, 2020

Uh oh!

gitgitgadget bot Jan 28, 2020

Uh oh!

gitgitgadget bot Jan 29, 2020

Uh oh!

gitgitgadget bot Jan 2, 2020

Uh oh!

gitgitgadget bot Jan 2, 2020

Uh oh!

gitgitgadget bot Jan 6, 2020

Uh oh!

gitgitgadget bot Jan 6, 2020

Uh oh!

gitgitgadget bot Jan 7, 2020

Uh oh!

gitgitgadget bot Jan 7, 2020

Uh oh!

gitgitgadget bot Jan 7, 2020

Uh oh!

gitgitgadget bot Jan 7, 2020

Uh oh!

gitgitgadget bot Jan 6, 2020

Uh oh!

gitgitgadget bot Jan 6, 2020

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up / @@ -31,6 +31,8 @@ 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_ignored_file = 1;
+    int advice_add_empty_pathspec = 1;
     static int advice_use_color = -1;
     static char advice_colors[][COLOR_MAXLEN] = {
@@ Expand Down Expand Up / @@ -91,6 +93,8 @@ static struct { @@
     	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
     	{ "nestedTag", &advice_nested_tag },
     	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
+    	{ "addIgnoredFile", &advice_add_ignored_file },
+    	{ "addEmptyPathspec", &advice_add_empty_pathspec },
     	/* make this an alias for backward compatibility */
     	{ "pushNonFastForward", &advice_push_update_rejected }
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -390,7 +390,10 @@ 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_ignored_file)
+    			advise(_("Use -f if you really want to add them.\n"
+    				 "Turn this message off by running\n"
+    				 "\"git config advice.addIgnoredFile false\""));
     		exit_status = 1;
     	}
@@ Expand Down Expand Up @@
     	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_empty_pathspec)
+    			advise( _("Maybe you wanted to say 'git add .'?\n"
+    				  "Turn this message off by running\n"
+    				  "\"git config advice.addEmptyPathspec false\""));
     		return 0;
     	}
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     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.
+    hint: Turn this message off by running
+    hint: "git config advice.addIgnoredFile false"
     EOF
     cat >expect.out <<\EOF
     add 'track-this'
@@ Expand Down @@

[Outreachy] add: use advise API to display hints #508

Uh oh!

[Outreachy] add: use advise API to display hints #508

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gitgitgadget bot Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 2, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 2, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 7, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 7, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 7, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 7, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!