Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add usage-strings ci check and amend remaining usage strings #1147

Closed
wants to merge 2 commits into from
Closed

add usage-strings ci check and amend remaining usage strings #1147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc200d0
amend remaining usage strings according to style guide
Abhra303 Feb 16, 2022
e1c5a32
parse-options.c: add style checks for usage-strings
Abhra303 Feb 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion builtin/bisect--helper.c
View file
Open in desktop
Expand Up @@ -1209,7 +1209,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
OPT_CMDMODE(0, "bisect-visualize", &cmdmode,
N_("visualize the bisection"), BISECT_VISUALIZE),
OPT_CMDMODE(0, "bisect-run", &cmdmode,
N_("use <cmd>... to automatically bisect."), BISECT_RUN),
N_("use <cmd>... to automatically bisect"), BISECT_RUN),
OPT_BOOL(0, "no-log", &nolog,
N_("no log for BISECT_WRITE")),
OPT_END()
Expand Down
6 changes: 3 additions & 3 deletions builtin/reflog.c
View file
Open in desktop
Expand Up @@ -600,7 +600,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
OPT_BIT(0, "updateref", &flags,
N_("update the reference to the value of the top reflog entry"),
EXPIRE_REFLOGS_UPDATE_REF),
OPT_BOOL(0, "verbose", &verbose, N_("print extra information on screen.")),
OPT_BOOL(0, "verbose", &verbose, N_("print extra information on screen")),
OPT_CALLBACK_F(0, "expire", &cmd, N_("timestamp"),
N_("prune entries older than the specified time"),
PARSE_OPT_NONEG,
Expand All @@ -613,7 +613,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
N_("prune any reflog entries that point to broken commits")),
OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")),
OPT_BOOL(1, "single-worktree", &all_worktrees,
N_("limits processing to reflogs from the current worktree only.")),
N_("limits processing to reflogs from the current worktree only")),
OPT_END()
};

Expand Down Expand Up @@ -736,7 +736,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
OPT_BIT(0, "updateref", &flags,
N_("update the reference to the value of the top reflog entry"),
EXPIRE_REFLOGS_UPDATE_REF),
OPT_BOOL(0, "verbose", &verbose, N_("print extra information on screen.")),
OPT_BOOL(0, "verbose", &verbose, N_("print extra information on screen")),
OPT_END()
};

Expand Down
2 changes: 1 addition & 1 deletion builtin/submodule--helper.c
View file
Open in desktop
Expand Up @@ -1875,7 +1875,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
OPT_STRING(0, "depth", &clone_data.depth,
N_("string"),
N_("depth for shallow clones")),
OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
OPT__QUIET(&quiet, "suppress output for cloning a submodule"),
OPT_BOOL(0, "progress", &progress,
N_("force cloning progress")),
OPT_BOOL(0, "require-init", &require_init,
Expand Down
2 changes: 1 addition & 1 deletion diff.c
View file
Open in desktop
Expand Up @@ -5630,7 +5630,7 @@ static void prep_parse_options(struct diff_options *options)
N_("select files by diff type"),
PARSE_OPT_NONEG, diff_opt_diff_filter),
{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
N_("Output to a specific file"),
N_("output to a specific file"),
PARSE_OPT_NONEG, NULL, 0, diff_opt_output },

OPT_END()
Expand Down
11 changes: 11 additions & 0 deletions parse-options.c
View file
Open in desktop
Expand Up @@ -492,6 +492,17 @@ static void parse_options_check(const struct option *opts)
default:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"Abhradeep Chakraborty via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +
> +		// OPTION_GROUP should be ignored
> +		// if the first two characters of the help string are uppercase, then assume it is an
> +		// acronym (i.e. "GPG") or special name (i.e. "HEAD"), thus allowed.
> +		// else assume the usage string is violating the style convention and throw error.

Style.

	/*
	 * This is how our multi-line comments
         * look like; with slash-asterisk that opens
         * and asterisk-slash that closes one on their
         * own lines.
	 */

Also avoid overly long lines.

> +		if (opts->type != OPTION_GROUP && opts->help &&
> +			opts->help[0] && isupper(opts->help[0]) &&
> +			!(opts->help[1] && isupper(opts->help[1])))
> +			err |= optbug(opts, xstrfmt("help should not start with capital letter unless needed: %s", opts->help));
> +		if (opts->help && !ends_with(opts->help, "...") && ends_with(opts->help, "."))
> +			err |= optbug(opts, xstrfmt("help should not end with a dot: %s", opts->help));

These two calls to optbug() use xstrfmt() to grab allocated pieces
of memory and pass it as a parameter to the function, which means
the string is leaked without any chance to be freed.

Do we care?

>  		if (opts->argh &&
>  		    strcspn(opts->argh, " _") != strlen(opts->argh))
>  			err |= optbug(opts, "multi-word argh should use dash to separate words");

The existing use of optbug() we see here does not share such a
problem.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):

Junio C Hamano wrote:

> Style.
>
>	/*
>        * This is how our multi-line comments
>        * look like; with slash-asterisk that opens
>        * and asterisk-slash that closes one on their
>        * own lines.
>	 */
>
> Also avoid overly long lines.

Oh, sorry for that. I was in kind of a hurry ( today was
my semester exam), so I didn't look at the style guide.
Will fix it.

> These two calls to optbug() use xstrfmt() to grab allocated pieces
> of memory and pass it as a parameter to the function, which means
> the string is leaked without any chance to be freed.
>
> Do we care?
>
> >  		if (opts->argh &&
> >  		    strcspn(opts->argh, " _") != strlen(opts->argh))
> >  			err |= optbug(opts, "multi-word argh should use dash to separate words");
>
> The existing use of optbug() we see here does not share such a
> problem.

hmm, I wanted a formatting function to format (i.e. pass the
`opt->help` dynamically) the output string. The existing use of
`optbug()` that you specified has no `%s` formatter; it is a plain
string. That's why I used `xstrfmt()`. Moreover, it was in Ævar's
suggestion[1] -

> +		if (opts->help && ends_with(opts->help, "."))
> +			err |= optbug(opts, xstrfmt("argh should not end with a dot: %s", opts->help));

But I think, you're right. There is some memory leakage here.
Should I go with plain strings then? (i.e. "help should not end
with a dot" instead of `xstrfmt("help should not end with a dot:
%s", opts->help)`)

Thanks :)

[1] https://lore.kernel.org/git/220221.86tucsb4oy.gmgdl@evledraar.gmail.com/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> writes:

>> These two calls to optbug() use xstrfmt() to grab allocated pieces
>> of memory and pass it as a parameter to the function, which means
>> the string is leaked without any chance to be freed.
>>
>> Do we care?
>>
>> >  		if (opts->argh &&
>> >  		    strcspn(opts->argh, " _") != strlen(opts->argh))
>> >  			err |= optbug(opts, "multi-word argh should use dash to separate words");
>>
>> The existing use of optbug() we see here does not share such a
>> problem.
>
> hmm, I wanted a formatting function to format (i.e. pass the
> `opt->help` dynamically) the output string. The existing use of
> `optbug()` that you specified has no `%s` formatter; it is a plain
> string. That's why I used `xstrfmt()`. Moreover, it was in Ævar's
> suggestion[1] -
>
>> +		if (opts->help && ends_with(opts->help, "."))
>> +			err |= optbug(opts, xstrfmt("argh should not end with a dot: %s", opts->help));
>
> But I think, you're right. There is some memory leakage here.
> Should I go with plain strings then? (i.e. "help should not end
> with a dot" instead of `xstrfmt("help should not end with a dot:
> %s", opts->help)`)

Sorry that I've given you a trick question, when I know you are
quite new to the community.

I think the right answer to "Do we care?" is "In this case, because
we are about to call exit(), we don't care.  The extra complexity
and code necessary to retain the memory we get from xstrfmt and free
it is not worth it."  It's not like we do this in a loop that iterates
unbounded number of times before the exit() happens (in which case
we should care).

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):


Junio C Hamano <gitster@pobox.com> wrote:

> Sorry that I've given you a trick question, when I know you are
> quite new to the community.

There is nothing to say `sorry`. Every review comment is teaching me
new things. E.g. If you didn't ask me this question, I would not go to
the codebase and see the proper handling of `xstrfmt`. So, thanks.

> I think the right answer to "Do we care?" is "In this case, because
> we are about to call exit(), we don't care.  The extra complexity
> and code necessary to retain the memory we get from xstrfmt and free
> it is not worth it."  It's not like we do this in a loop that iterates
> unbounded number of times before the exit() happens (in which case
> we should care).
>
> Thanks.

Got it.

Thanks :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Abhradeep,

On Fri, 25 Feb 2022, Abhradeep Chakraborty via GitGitGadget wrote:

> From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
>
> `parse-options.c` doesn't check if the usage strings for option flags
> are following the style guide or not. Style convention says, usage
> strings should not start with capital letter (unless needed) and
> it should not end with `.`.
>
> Add checks to the `parse_options_check()` function to check usage
> strings against the style convention.

As I just pointed out in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2202251600210.11118@tvgsbejvaqbjf.bet/,
it seems that replacing the static check presented in v1 by a runtime
check needs to be reverted.

In addition to the example I presented, there is another compelling reason
to do so: with the static check, we can detect incorrect usage strings in
all code, even in code that is platform-dependent (such as in
`fsmonitor--daemon`).

Ciao,
Dscho

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> As I just pointed out in
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2202251600210.11118@tvgsbejv=
> aqbjf.bet/,
> it seems that replacing the static check presented in v1 by a runtime
> check needs to be reverted.
>
> In addition to the example I presented, there is another compelling reason
> to do so: with the static check, we can detect incorrect usage strings in
> all code, even in code that is platform-dependent (such as in
> `fsmonitor--daemon`).

First of all, thank you so much for putting so much time to look into
my PR. I appriciate your research about various possible outcomes of this
Patch request.

I saw your mail where you listed some of the disadvantages of the current
version. I also agree with the arguments you provided and it is also true
that one wouldn't find any clue by seeing the output of the `CI` link
you mentioned (i.e. https://github.com/git/git/runs/5312914410?check_suite_focus=true).

Let's see what Junio, Ævar and others say about this.

Thanks :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Add checks to the `parse_options_check()` function to check usage
>> strings against the style convention.
>
> As I just pointed out in
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2202251600210.11118@tvgsbejvaqbjf.bet/,
> it seems that replacing the static check presented in v1 by a runtime
> check needs to be reverted.

Sorry, but I am not sure how that conclusion follows from a breakage
in a topic in flight that was discovered by the check.

I do not know if a coccinelle based solution is sufficiently easy,
simple and robust enough to encourage us to scrap what has already
been proposed and reviewed, instead of leaving it as a topic for a
future incremental improvement that we can make on top.

> In addition to the example I presented, there is another compelling reason
> to do so: with the static check, we can detect incorrect usage strings in
> all code, even in code that is platform-dependent (such as in
> `fsmonitor--daemon`).

Yes and no.  

I would imagine that large enough platforms that have their own
conditionally compiled #ifdef/#endif block already have CI to build
their conditionally compiled block in practice.  I do not see the
above as a compelling reason to grow and shift the scope of these
two patches.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Junio C Hamano <gitster@pobox.com> writes:

> I would imagine that large enough platforms that have their own
> conditionally compiled #ifdef/#endif block already have CI to build
> their conditionally compiled block in practice.  I do not see the
> above as a compelling reason to grow and shift the scope of these
> two patches.

Let's instead drop [2/2] for now.  I do not want us to go back to
shell script that pretends to know about C language, and I do not
want to block [1/2] by waiting for a replacement.  Fixes in [1/2]
are pretty much uncontroversial ones that can even be fast-tracked
down to 'master'.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):


Junio C Hamano <gitster@pobox.com> wrote:

> Let's instead drop [2/2] for now.  I do not want us to go back to
> shell script that pretends to know about C language, and I do not
> want to block [1/2] by waiting for a replacement.  Fixes in [1/2]
> are pretty much uncontroversial ones that can even be fast-tracked
> down to 'master'.

Though, as a new contributor, I felt bad about dropping the last
patch, but if you think the last patch request needs more discussion
( which I think is needed) - I also in favour of dropping the last
commit.

Would you do this on your side or I will re-submit it with the first
commit?

Thanks :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):


Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> And since Abhradeep is a new contributor, I found it important to steer
> the direction toward sound advice that they can use over and over again
> over the course of their career: whenever possible, prefer static checks
> over runtime ones.

Thanks Johannes for the advice. I will always remember it ^^

> Of course, if we can convince Coccinelle (together with Python) to give us
> the static check, we might very well be able to port more of
> `parse_options_check()` from runtime checks to static ones, which would be
> a clear win.
>
> If that is possible, we could save ourselves a lot of time by skipping (2)
> altogether.
>
> And as I said, Julia's advice looked really good. If only I wasn't
> desperately short on time, I would have given it a try because it sounds
> not only fun but also very, very useful in Git's context.

Since Junio and you both have an interest in Coccinelle, if you allow,
I want to look into it.

Thanks :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
>> And since Abhradeep is a new contributor, I found it important to steer
>> the direction toward sound advice that they can use over and over again
>> over the course of their career: whenever possible, prefer static checks
>> over runtime ones.
>
> Thanks Johannes for the advice. I will always remember it ^^

Yup, if we can have static and dynamic checks of the same quality,
static checks are always better alternative.  In this case, runtime
check would probably be an expedite solution suitable for a shorter
term to fill the gap, as a static check with the same quality as it
would probably need some time to develop.

> Since Junio and you both have an interest in Coccinelle, if you allow,
> I want to look into it.

I do not have any particular interest.  If it is a tool fit for the
task, it would be good to use it, that's all ;-)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):


Junio C Hamano <<gitster@pobox.com> wrote:

> Yup, if we can have static and dynamic checks of the same quality,
> static checks are always better alternative.  In this case, runtime
> check would probably be an expedite solution suitable for a shorter
> term to fill the gap, as a static check with the same quality as it
> would probably need some time to develop.

Got it!

> I do not have any particular interest.  If it is a tool fit for the
> task, it would be good to use it, that's all ;-)

Okay, then I would like to research if that is a good fit. Johannes
is pretty confident about it though.

Thanks :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi,

On Fri, 4 Mar 2022, Abhradeep Chakraborty wrote:

> Junio C Hamano <<gitster@pobox.com> wrote:
>
> > Yup, if we can have static and dynamic checks of the same quality,
> > static checks are always better alternative.  In this case, runtime
> > check would probably be an expedite solution suitable for a shorter
> > term to fill the gap, as a static check with the same quality as it
> > would probably need some time to develop.
>
> Got it!

While the runtime check would address the concern in the short run, paving
the path for future static checks revolving around the same area will pay
off quite happily.

> > I do not have any particular interest.  If it is a tool fit for the
> > task, it would be good to use it, that's all ;-)
>
> Okay, then I would like to research if that is a good fit. Johannes
> is pretty confident about it though.

Yes, he is.

And he wishes he had the time to work on it himself because it sounds like
a really fun (if challenging) project.

In other words: If you ever get stuck somewhere along the lines, please do
push up a work-in-progress branch and reach out here so that I or others
can help.

Ciao,
Dscho

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):


Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> Yes, he is.

> And he wishes he had the time to work on it himself because it sounds like
> a really fun (if challenging) project.
>
> In other words: If you ever get stuck somewhere along the lines, please do
> push up a work-in-progress branch and reach out here so that I or others
> can help.

Thank you so much ^^

; /* ok. (usually accepts an argument) */
}

// OPTION_GROUP should be ignored
// if the first two characters of the help string are uppercase, then assume it is an
// acronym (i.e. "GPG") or special name (i.e. "HEAD"), thus allowed.
// else assume the usage string is violating the style convention and throw error.
if (opts->type != OPTION_GROUP && opts->help &&
opts->help[0] && isupper(opts->help[0]) &&
!(opts->help[1] && isupper(opts->help[1])))
err |= optbug(opts, xstrfmt("help should not start with capital letter unless needed: %s", opts->help));
if (opts->help && !ends_with(opts->help, "...") && ends_with(opts->help, "."))
err |= optbug(opts, xstrfmt("help should not end with a dot: %s", opts->help));
if (opts->argh &&
strcspn(opts->argh, " _") != strlen(opts->argh))
err |= optbug(opts, "multi-word argh should use dash to separate words");
Expand Down
6 changes: 3 additions & 3 deletions t/helper/test-run-command.c
View file
Open in desktop
Expand Up @@ -221,9 +221,9 @@ static int quote_stress_test(int argc, const char **argv)
struct strbuf out = STRBUF_INIT;
struct strvec args = STRVEC_INIT;
struct option options[] = {
OPT_INTEGER('n', "trials", &trials, "Number of trials"),
OPT_INTEGER('s', "skip", &skip, "Skip <n> trials"),
OPT_BOOL('m', "msys2", &msys2, "Test quoting for MSYS2's sh"),
OPT_INTEGER('n', "trials", &trials, "number of trials"),
OPT_INTEGER('s', "skip", &skip, "skip <n> trials"),
OPT_BOOL('m', "msys2", &msys2, "test quoting for MSYS2's sh"),
OPT_END()
};
const char * const usage[] = {
Expand Down
4 changes: 2 additions & 2 deletions t/t1502-rev-parse-parseopt.sh
View file
Open in desktop
Expand Up @@ -53,7 +53,7 @@ test_expect_success 'setup optionspec-only-hidden-switches' '
|
|some-command does foo and bar!
|--
|hidden1* A hidden switch
|hidden1* a hidden switch
EOF
'

Expand Down Expand Up @@ -131,7 +131,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
|
| some-command does foo and bar!
|
| --hidden1 A hidden switch
| --hidden1 a hidden switch
|
|EOF
END_EXPECT
Expand Down