[Outreachy] check-ref-format: parse-options #449
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
Prepare this command which currently handles its own argv to use parse-options instead. A lot of the commands already have parse-options and in git.c cmd_struct with the "NO_PARSEOPT" are the one's that still need it. Signed-off-by: george espinoza <[email protected]>
This command currently handles its own argv so by teaching it to use parse-options instead we can standardize the way commands handle user input across the project. As a consequence of using OPT_BOOL data structure on --normalize & --refspec-pattern, --no-normalize & --no-refspec-pattern has been can now be used. NO_PARSEOPT flag was also removed to update git.c with the conversion of the structure in this command. This is a rough draft and I need some advice if I'm doing this correctly since its being built but it is failing tests. Helped by: emily shaffer <[email protected]> Helped by: johannes schindelin <[email protected]> Signed-off-by: george espinoza <[email protected]>
check-ref-format :( Signed-off-by: george espinoza <[email protected]>
This command currently handles its own argv so by teaching it to use parse-options instead we can standardize the way commands handle user input across the project. As a consequence of using OPT_BOOL data structure on --normalize & --refspec-pattern, --no-normalize & --no-refspec-pattern can now be used. NO_PARSEOPT flag was also removed to update git.c with the conversion of the structure in this command. This is a rough draft and I need some advice if I'm doing this correctly since its being built but it is failing tests. Helped by: emily shaffer [email protected] Helped by: johannes schindelin [email protected] Signed-off-by: george espinoza <[email protected]>
|
I made this PR in the git repository instead of gitgitgadget so I made this next edit here. Made a ton of revisions with the help of @nasamuffin and @dscho I'm currently doing fixes because tests are failing with the -branch option. |
dscho
reviewed
Nov 5, 2019
builtin/check-ref-format.c
Outdated
| OPT_CMDMODE( 0 , "branch", &check_ref_format_branch, N_("branch"), CHECK_REF_FORMAT_BRANCH), | ||
| OPT_BOOL( 0 , "normalize", &normalize, N_("normalize tracked files")), | ||
| OPT_BIT( 0 , "allow-onelevel", &flags, N_("allow one level"), REFNAME_ALLOW_ONELEVEL), | ||
| OPT_NEGBIT( 0, "no-allow-onelevel", &flags, N_("no allow one level"), REFNAME_ALLOW_ONELEVEL), |
There was a problem hiding this comment.
Is this NEGBIT not implied by OPT_BIT? In other words, if you delete this line 75 and recompile, does ./git check-ref-format understand the --no-allow-onelevel option or does it complain?
There was a problem hiding this comment.
I deleted it and I don't see any make errors or test failures. I don't think it complains.
|
Wow thank you for all the suggestions @dscho. I had some real life stuff to take care of yesterday but I'm free now and checking these changes out. |
This command currently handles its own argv so by teaching it to use parse-options instead we can standardize the way commands handle user input across the project. Because OPT_BOOL data structure is being used on --normalize --no-normalize can now be utilized. NO_PARSEOPT flag was also removed to update git.c with the conversion of the structure in this command. I got all tests to pass at the moment. My next commit will be to check if OPT_NEGBIT isn't needed and moving around OPT_VERBOSE. Helped by: emily shaffer [email protected] Helped by: johannes schindelin [email protected] Signed-off-by: george espinoza <[email protected]>
|
I got all tests to pass at the moment. I'm really happy about that! |
This command currently handles its own argv so by teaching it to use parse-options instead we can standardize the way commands handle user input across the project. Because OPT_BOOL data structure is being used on --normalize --no-normalize can now be utilized. NO_PARSEOPT flag was also removed to update git.c with the conversion of the structure in this command. Helped by: emily shaffer [email protected] Helped by: johannes schindelin [email protected] Signed-off-by: george espinoza <[email protected]>
|
/submit |
|
Submitted as [email protected] |
Whoops. I don't think that you meant to keep 6 separate commits... Let's squash them? You can do so by running Oh, and before that, you probably want to send a "sorry, I meant to squash the commits first" reply to https://public-inbox.org/git/[email protected]/ (fall is squash season, after all). |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
builtin/merge-ours.c
Outdated
| @@ -11,6 +11,11 @@ | |||
| #include "git-compat-util.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"george espinoza via GitGitGadget" <[email protected]> writes:
> From: george espinoza <[email protected]>
>
> Prepare this command which currently handles its own argv to use
> parse-options instead. A lot of the commands already have
> parse-options and in git.c cmd_struct with the "NO_PARSEOPT" are
> the one's that still need it.
>
> Signed-off-by: george espinoza <[email protected]>
> ---
> builtin/merge-ours.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
> index 4594507420..3980f4899a 100644
> --- a/builtin/merge-ours.c
> +++ b/builtin/merge-ours.c
> @@ -11,6 +11,11 @@
> #include "git-compat-util.h"
> #include "builtin.h"
> #include "diff.h"
> +#include "parse-options.h"
> +
> +/* parse-options.h added to initiate replacement of manual option parsing
> + * with parse-options.
> + */
See Documentation/CodingGuidelines and learn how we write multi-line
comments.
Since there is no "manual option parsing" to be replaced, there is
no other change to this file, I guess (so what's the point of this
step?).
| @@ -6,10 +6,13 @@ | |||
| #include "refs.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"george espinoza via GitGitGadget" <[email protected]> writes:
> From: george espinoza <[email protected]>
>
> This command currently handles its own argv so by teaching it to
> use parse-options instead we can standardize the way commands
> handle user input across the project.
>
> As a consequence of using OPT_BOOL data structure on --normalize &
> --refspec-pattern, --no-normalize & --no-refspec-pattern has been
> can now be used.
>
> NO_PARSEOPT flag was also removed to update git.c with the
> conversion of the structure in this command.
>
> This is a rough draft and I need some advice if I'm doing this
> correctly since its being built but it is failing tests.
>
> Helped by: emily shaffer <[email protected]>
> Helped by: johannes schindelin <[email protected]>
I do not think they spell their name like the above. In general,
most of us do not spell our names in all lowercase around here. I
appreciate people with originality, but I'd rather see them to be
original not in how they spell their names but in more productive
ways ;-)
> Signed-off-by: george espinoza <[email protected]>
> ---
> builtin/check-ref-format.c | 49 +++++++++++++++++++-------------------
> git.c | 2 +-
> 2 files changed, 26 insertions(+), 25 deletions(-)
>
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index bc67d3f0a8..3fe0b5410a 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -6,10 +6,13 @@
> #include "refs.h"
> #include "builtin.h"
> #include "strbuf.h"
> +#include "parse-options.h"
>
> -static const char builtin_check_ref_format_usage[] =
> -"git check-ref-format [--normalize] [<options>] <refname>\n"
> -" or: git check-ref-format --branch <branchname-shorthand>";
> +static const char * const builtin_check_ref_format_usage[] = {
> + N_("git check-ref-format [--normalize] [<options>] <refname>\n"),
> + N_(" or: git check-ref-format --branch <branchname-shorthand>"),
> + NULL,
> +};
OK. This is the bog-standard prep for calling usage_with_options().
> @@ -53,31 +56,29 @@ static int check_ref_format_branch(const char *arg)
>
> int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> {
> - int i;
> - int normalize = 0;
> + enum {
> + CHECK_REF_FORMAT_BRANCH,
> + };
> +
> + int i = 0;
> + int verbose;
> + int normalize;
> + int allow_onelevel;
> + int refspec_pattern;
> int flags = 0;
> const char *refname;
Discard the blank line before "int i = 0" line, and keep the blank
line you have here between the last declaration and the first
statement.
> - if (argc == 2 && !strcmp(argv[1], "-h"))
> - usage(builtin_check_ref_format_usage);
> -
> - if (argc == 3 && !strcmp(argv[1], "--branch"))
> - return check_ref_format_branch(argv[2]);
> + struct option options[] = {
> + OPT__VERBOSE(&verbose, N_("be verbose")),
> + OPT_GROUP(""),
> + OPT_CMDMODE( 0 , "branch", &check_ref_format_branch, N_("branch"), CHECK_REF_FORMAT_BRANCH),
This is an iffy/tricky way to use CMDMODE. The way CMDMODE is
designed to be used is to have multiple ones that sets the same
target variable so that parse_options() can notice conflicting
command line request that gives more than one from the same set.
The command has two modes (i.e. the "--branch" mode and the unnamed
mode), so it is understandable that there is only one CMDMODE in the
options[] array, but I am not sure how well it would work for a
command like this. For example, "check-ref-format --branch
--normalize --allow-onelevel $v" should error out because --branch
is not compatible with any other options. I do not think a single
parse_options() call with a single options[] array can express that
kind of constraints.
Besides, shouldn't the third parameter of OPT_CMDMODE supposed to be
the address of the variable that would receive the value in its fifth
parameter? I do not see a decl for check_ref_format_branch variable
(isn't that the name of the function???).
Ahh, you said it builds but does not pass test. Of course, that is
because this part is completely bogus.
It appears that to your series the only thing that matters is the
shape of the tree after applying all of the patches, and individual
steps are not ready to be reviewd, so I'd stop here.
There was a problem hiding this comment.
On the Git mailing list, George Espinoza wrote (reply to this):
On Thu, Nov 7, 2019 at 2:25 AM Junio C Hamano <[email protected]> wrote:
>
> "george espinoza via GitGitGadget" <[email protected]> writes:
>
> > From: george espinoza <[email protected]>
> >
> > This command currently handles its own argv so by teaching it to
> > use parse-options instead we can standardize the way commands
> > handle user input across the project.
> >
> > As a consequence of using OPT_BOOL data structure on --normalize &
> > --refspec-pattern, --no-normalize & --no-refspec-pattern has been
> > can now be used.
> >
> > NO_PARSEOPT flag was also removed to update git.c with the
> > conversion of the structure in this command.
> >
> > This is a rough draft and I need some advice if I'm doing this
> > correctly since its being built but it is failing tests.
> >
> > Helped by: emily shaffer <[email protected]>
> > Helped by: johannes schindelin <[email protected]>
>
> I do not think they spell their name like the above. In general,
> most of us do not spell our names in all lowercase around here. I
> appreciate people with originality, but I'd rather see them to be
> original not in how they spell their names but in more productive
> ways ;-)
>
Ah, I see. I will use capital letters.
> > Signed-off-by: george espinoza <[email protected]>
> > ---
> > builtin/check-ref-format.c | 49 +++++++++++++++++++-------------------
> > git.c | 2 +-
> > 2 files changed, 26 insertions(+), 25 deletions(-)
> >
> > diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> > index bc67d3f0a8..3fe0b5410a 100644
> > --- a/builtin/check-ref-format.c
> > +++ b/builtin/check-ref-format.c
> > @@ -6,10 +6,13 @@
> > #include "refs.h"
> > #include "builtin.h"
> > #include "strbuf.h"
> > +#include "parse-options.h"
> >
> > -static const char builtin_check_ref_format_usage[] =
> > -"git check-ref-format [--normalize] [<options>] <refname>\n"
> > -" or: git check-ref-format --branch <branchname-shorthand>";
> > +static const char * const builtin_check_ref_format_usage[] = {
> > + N_("git check-ref-format [--normalize] [<options>] <refname>\n"),
> > + N_(" or: git check-ref-format --branch <branchname-shorthand>"),
> > + NULL,
> > +};
>
> OK. This is the bog-standard prep for calling usage_with_options().
I see, I will look into doing something more here as necessary.
>
> > @@ -53,31 +56,29 @@ static int check_ref_format_branch(const char *arg)
> >
> > int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> > {
> > - int i;
> > - int normalize = 0;
> > + enum {
> > + CHECK_REF_FORMAT_BRANCH,
> > + };
> > +
> > + int i = 0;
> > + int verbose;
> > + int normalize;
> > + int allow_onelevel;
> > + int refspec_pattern;
> > int flags = 0;
> > const char *refname;
>
> Discard the blank line before "int i = 0" line, and keep the blank
> line you have here between the last declaration and the first
> statement.
I meant to squash all my earlier commits but had an issue with git rebase -i.
It only says noop. I had an issue when I first started my other branch
and might of reset it. I'll look into fixing this before submitting my
next patch.
>
> > - if (argc == 2 && !strcmp(argv[1], "-h"))
> > - usage(builtin_check_ref_format_usage);
> > -
> > - if (argc == 3 && !strcmp(argv[1], "--branch"))
> > - return check_ref_format_branch(argv[2]);
> > + struct option options[] = {
> > + OPT__VERBOSE(&verbose, N_("be verbose")),
> > + OPT_GROUP(""),
> > + OPT_CMDMODE( 0 , "branch", &check_ref_format_branch, N_("branch"), CHECK_REF_FORMAT_BRANCH),
>
> This is an iffy/tricky way to use CMDMODE. The way CMDMODE is
> designed to be used is to have multiple ones that sets the same
> target variable so that parse_options() can notice conflicting
> command line request that gives more than one from the same set.
>
> The command has two modes (i.e. the "--branch" mode and the unnamed
> mode), so it is understandable that there is only one CMDMODE in the
> options[] array, but I am not sure how well it would work for a
> command like this. For example, "check-ref-format --branch
> --normalize --allow-onelevel $v" should error out because --branch
> is not compatible with any other options. I do not think a single
> parse_options() call with a single options[] array can express that
> kind of constraints.
Ok I will look into another data structure to use for branch.
>
> Besides, shouldn't the third parameter of OPT_CMDMODE supposed to be
> the address of the variable that would receive the value in its fifth
> parameter? I do not see a decl for check_ref_format_branch variable
> (isn't that the name of the function???).
>
> Ahh, you said it builds but does not pass test. Of course, that is
> because this part is completely bogus.
>
> It appears that to your series the only thing that matters is the
> shape of the tree after applying all of the patches, and individual
> steps are not ready to be reviewd, so I'd stop here.
>
I had only intended /submit to show the last commit I had made that had passed
all the tests. I will review everything and only submit again once im 100%
sure everything is in order. Sorry Junio!
|
On Wed, 6 Nov 2019, George Espinoza wrote:
Georgecanfly commented on this pull request.
> int normalize = 0;
int flags = 0;
const char *refname;
- if (argc == 2 && !strcmp(argv[1], "-h"))
- usage(builtin_check_ref_format_usage);
-
- if (argc == 3 && !strcmp(argv[1], "--branch"))
- return check_ref_format_branch(argv[2]);
+ struct option options[] = {
+ OPT__VERBOSE(&verbose, N_("be verbose")),
+ OPT_GROUP(""),
+ OPT_CMDMODE( 0 , "branch", &check_ref_format_branch, N_("branch"), CHECK_REF_FORMAT_BRANCH),
+ OPT_BOOL( 0 , "normalize", &normalize, N_("normalize tracked files")),
+ OPT_BIT( 0 , "allow-onelevel", &flags, N_("allow one level"), REFNAME_ALLOW_ONELEVEL),
+ OPT_NEGBIT( 0, "no-allow-onelevel", &flags, N_("no allow one level"), REFNAME_ALLOW_ONELEVEL),
I deleted it and I don't see any make errors or test failures. I don't think it complains.
A better test would be to build it and to test manually with `./git check-ref-format --no-allow-onelevel [...]` whether it works as intended.
|
Hi @dscho Ok yeah I noticed that too many patches went out and it was too late. I had tried doing |
We have that
You have to tell
It is tempting to do that, but I wouldn't, if I were you. If you learn how to fix issues like this now, it will serve you many, many times later. The first thing is to understand the state in which your branch is: it has a bunch of commits. When you say that you wanted to submit "only the last commit", I fear that you mean not the last commit, but the changes accumulated by all six commits. If you want a single commit to have all those changes, you will have to squash them all into a single one. As I pointed out, you can do that by using the |
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 command currently handles its own argv so by teaching it to
use parse-options instead we can standardize the way commands
handle user input across the project.
Because OPT_BOOL data structure is being used on --normalize
--no-normalize can now be utilized.
NO_PARSEOPT flag was also removed to update git.c with the
conversion of the structure in this command.
This is Micro-Project #3 that states: Teach a command which currently
handles its own argv how to use parse-options instead..
Helped by: emily shaffer [email protected]
Helped by: johannes schindelin [email protected]
Signed-off-by: george espinoza [email protected]