add usage-strings ci check and amend remaining usage strings #1147
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Abhra303
force-pushed
the
usage_command_amend
branch
from
573d95e
to
ea0ba45
Compare
|
/preview |
|
Preview email sent as pull.1147.git.1645030754096.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1147.git.1645030949730.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Abhradeep Chakraborty wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Abhradeep Chakraborty wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Abhradeep Chakraborty wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
User |
|
On the Git mailing list, Julia Lawall wrote (reply to this): |
|
User |
|
On the Git mailing list, Abhradeep Chakraborty wrote (reply to this): |
Abhra303
force-pushed
the
usage_command_amend
branch
from
ea0ba45
to
902937e
Compare
|
On the Git mailing list, Abhradeep Chakraborty wrote (reply to this): |
|
/preview |
|
Preview email sent as pull.1147.v2.git.1645545263554.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1147.v2.git.1645545507689.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Eric Sunshine wrote (reply to this): |
|
User |
|
On the Git mailing list, Abhradeep Chakraborty wrote (reply to this): |
Abhra303
force-pushed
the
usage_command_amend
branch
from
902937e
to
9d42bdb
Compare
|
/preview |
|
Preview email sent as pull.1147.v3.git.1645626290.gitgitgadget@gmail.com |
|
Submitted as pull.1147.v3.git.1645626455.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
This patch series was integrated into seen via git@cd9cc5a. |
|
There was a status update in the "New Topics" section about the branch source: <pull.1147.v3.git.1645626455.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@beadd0e. |
Usage strings for git (sub)command flags has a style guide that suggests - first letter should not capitalized (unless required) and it should skip full-stop at the end of line. But there are some files where usage-strings do not follow the above mentioned guide. Amend the usage strings that don't follow the style convention/guide. Signed-off-by: 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. Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
Abhra303
force-pushed
the
usage_command_amend
branch
from
9d42bdb
to
e1c5a32
Compare
|
/preview |
|
Preview email sent as pull.1147.v4.git.1645766421.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1147.v4.git.1645766599.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -492,6 +492,17 @@ static void parse_options_check(const struct option *opts) | |||
| default: | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Julia Lawall wrote (reply to this): |
| @@ -492,6 +492,17 @@ static void parse_options_check(const struct option *opts) | |||
| default: | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 ;-)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ^^
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
This patch series was integrated into seen via git@93d5e4d. |
|
There was a status update in the "New Topics" section about the branch Usage-string normalization, plus runtime enforcement. Will merge to 'next'? source: <pull.1147.v3.git.1645626455.gitgitgadget@gmail.com> |
|
On the Git mailing list, Abhradeep Chakraborty wrote (reply to this): |
|
On the Git mailing list, Julia Lawall wrote (reply to this): |
|
There was a status update in the "Cooking" section about the branch Usage-string normalization. Will merge to 'master'. source: <f425e36b7ea4a310a8ad93d47ead4c1713117388.1645626455.git.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch Usage-string normalization. Will merge to 'master'. source: <f425e36b7ea4a310a8ad93d47ead4c1713117388.1645626455.git.gitgitgadget@gmail.com> |
|
There was a status update in the "Graduated to 'master'" section about the branch Usage-string normalization. source: <f425e36b7ea4a310a8ad93d47ead4c1713117388.1645626455.git.gitgitgadget@gmail.com> |
|
Closed manually because GitGitGadget had no way of knowing that the tip commit was dropped intentionally. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch series completely fixes #636.
The issue is about amending the usage-strings (for command flags such as
-h,-vetc.) which do not follow the style convention/guide. There was a PR addressing this issue but as Johannes said in his comment, there are some files that still have those kind of usage strings. Johannes also suggested to add a CI check underci/test-documentation.shto check the usage strings.In this version, comments added and the
Fromfield of the first commit message is updated (i.e. "Abhradeep Chakraborty" instead of "Abhra303")Changes since v2:
parse_options_check())Changes since v1:
check-usage-strings.shCIcheckparse-options.ct/t1502-rev-parse-parseopt.shto pass the testUntil v1:
A shell script
check-usage-strings.shwas introduced to check the usage-strings.CIcheck for the same was also introduced.cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: Eric Sunshine sunshine@sunshineco.com
cc: Junio C Hamano gitster@pobox.com