| Message ID | 6d89c23a248d1c11db0e92c4f06392272f00c3a2.1640647438.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Factorization of messages with similar meaning | expand |
"Jean-Noël Avila via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@free.fr> > > Signed-off-by: Jean-Noël Avila <jn.avila@free.fr> > --- > builtin/tag.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/builtin/tag.c b/builtin/tag.c > index 6f7cd0e3ef5..a2ab2b15304 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -483,6 +483,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) > OPT_END() > }; > int ret = 0; > + const char *only_in_list = NULL; > > setup_ref_filter_porcelain_msg(); > > @@ -542,13 +543,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix) > goto cleanup; > } > if (filter.lines != -1) > - die(_("-n option is only allowed in list mode")); > + only_in_list = "-n"; > if (filter.with_commit) > - die(_("--contains option is only allowed in list mode")); > + only_in_list = "--contains"; > if (filter.no_commit) > - die(_("--no-contains option is only allowed in list mode")); > + only_in_list = "--no-contains"; > if (filter.points_at.nr) > - die(_("--points-at option is only allowed in list mode")); > + only_in_list = "--points-at"; > + if (only_in_list) > + die("the '%s' option is only allowed in list mode", only_in_list); > if (filter.reachable_from || filter.unreachable_from) > die(_("--merged and --no-merged options are only allowed in list mode")); > if (cmdmode == 'd') { The original died with the first problematic condition that was detected, so it was possible to detect a condition and die, and check a different condition in a way that may segfault when the first condition was true, because we would have called die() before making such a risky check for the second condition. In the above cascade, however, there is luckily no such dependency, so the above change is safe. But it still changes the semantics. Given "tag -d -n 4 --with master", we would have complained about "-n", but now we will complain about the "--contains", no? We can fix both of the above issues by making these into an if/else if/ cascade, i.e. if (filter.lines != -1) only_in_list = "-n"; else if (filter.with_commit) only_in_list = "--contains"; ... if (only_in_list) die(_("the '%s' option is only allowed..."), only_in_list); And I think you forgot to mark the message that was factored out for translation.
On Mon, Dec 27 2021, Junio C Hamano wrote: > "Jean-Noël Avila via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@free.fr> >> >> Signed-off-by: Jean-Noël Avila <jn.avila@free.fr> >> --- >> builtin/tag.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/builtin/tag.c b/builtin/tag.c >> index 6f7cd0e3ef5..a2ab2b15304 100644 >> --- a/builtin/tag.c >> +++ b/builtin/tag.c >> @@ -483,6 +483,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) >> OPT_END() >> }; >> int ret = 0; >> + const char *only_in_list = NULL; >> >> setup_ref_filter_porcelain_msg(); >> >> @@ -542,13 +543,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix) >> goto cleanup; >> } >> if (filter.lines != -1) >> - die(_("-n option is only allowed in list mode")); >> + only_in_list = "-n"; >> if (filter.with_commit) >> - die(_("--contains option is only allowed in list mode")); >> + only_in_list = "--contains"; >> if (filter.no_commit) >> - die(_("--no-contains option is only allowed in list mode")); >> + only_in_list = "--no-contains"; >> if (filter.points_at.nr) >> - die(_("--points-at option is only allowed in list mode")); >> + only_in_list = "--points-at"; >> + if (only_in_list) >> + die("the '%s' option is only allowed in list mode", only_in_list); >> if (filter.reachable_from || filter.unreachable_from) >> die(_("--merged and --no-merged options are only allowed in list mode")); >> if (cmdmode == 'd') { > > The original died with the first problematic condition that was > detected, so it was possible to detect a condition and die, and > check a different condition in a way that may segfault when the > first condition was true, because we would have called die() before > making such a risky check for the second condition. In the above > cascade, however, there is luckily no such dependency, so the above > change is safe. > > But it still changes the semantics. Given "tag -d -n 4 --with master", > we would have complained about "-n", but now we will complain about > the "--contains", no? > > We can fix both of the above issues by making these into an if/else > if/ cascade, i.e. > > if (filter.lines != -1) > only_in_list = "-n"; > else if (filter.with_commit) > only_in_list = "--contains"; > ... > if (only_in_list) > die(_("the '%s' option is only allowed..."), only_in_list); > > And I think you forgot to mark the message that was factored out for > translation. Does it really matter? I.e. we've got plenty of options parsing code in various places that might complain about issues A and B with your command-line, but will only emit one of those at a time. So if you've got A and B you'll need to play the whack-a-mole game of fixing A, only to then run into B. It really doesn't change things from a practical POV that we'll now complain about B first, only to complain about A later. Ideally we'd spot and emit all of the issues, as e.g. my [1] does for some unrelated code. But until such an improvement something as trivial as minimizing the diff size (i.e. not needing "if" -> "else if") seems preferrable to slavishly maintaining compatibility with the exact sequence of errors we'd emit before. 1. https://lore.kernel.org/git/RFC-patch-09.21-9c6af87c6c9-20211115T220831Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Does it really matter? I.e. we've got plenty of options parsing code in > various places that might complain about issues A and B with your > command-line, but will only emit one of those at a time. > ... > But until such an improvement something as trivial as minimizing the > diff size (i.e. not needing "if" -> "else if") seems preferrable to > slavishly maintaining compatibility with the exact sequence of errors > we'd emit before. The patch is already touching every other line. Making the change into a replacement of solid block into another solid block would probably make the resulting patch easier to read. And by doing so, the submitter demonstrates to reviewers that they _care_, as such an attention to detail is a sign that the submitter thought the ramifications of changing an early die() into an assignment that keeps going. So, yes, it does matter.
On Tuesday, 28 December 2021 01:16:40 CET Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > Does it really matter? I.e. we've got plenty of options parsing code in > > various places that might complain about issues A and B with your > > command-line, but will only emit one of those at a time. > > ... > > But until such an improvement something as trivial as minimizing the > > diff size (i.e. not needing "if" -> "else if") seems preferrable to > > slavishly maintaining compatibility with the exact sequence of errors > > we'd emit before. > > The patch is already touching every other line. Making the change > into a replacement of solid block into another solid block would > probably make the resulting patch easier to read. > > And by doing so, the submitter demonstrates to reviewers that they > _care_, as such an attention to detail is a sign that the submitter > thought the ramifications of changing an early die() into an > assignment that keeps going. > > So, yes, it does matter. > Sorry. The developer hat was taken off ; will reroll shortly.
diff --git a/builtin/tag.c b/builtin/tag.c index 6f7cd0e3ef5..a2ab2b15304 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -483,6 +483,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) OPT_END() }; int ret = 0; + const char *only_in_list = NULL; setup_ref_filter_porcelain_msg(); @@ -542,13 +543,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix) goto cleanup; } if (filter.lines != -1) - die(_("-n option is only allowed in list mode")); + only_in_list = "-n"; if (filter.with_commit) - die(_("--contains option is only allowed in list mode")); + only_in_list = "--contains"; if (filter.no_commit) - die(_("--no-contains option is only allowed in list mode")); + only_in_list = "--no-contains"; if (filter.points_at.nr) - die(_("--points-at option is only allowed in list mode")); + only_in_list = "--points-at"; + if (only_in_list) + die("the '%s' option is only allowed in list mode", only_in_list); if (filter.reachable_from || filter.unreachable_from) die(_("--merged and --no-merged options are only allowed in list mode")); if (cmdmode == 'd') {