Skip to content

[Outreachy] check-ref-format: parse-options #449

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[Outreachy] check-ref-format: parse-options #449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5e3b370
[Outreachy] merge-ours: include parse-option.h
Georgecanfly Oct 28, 2019
e8bca09
[Outreachy] check-ref-format: parse-options
Georgecanfly Nov 3, 2019
078a19e
This file wasn't supposed to change during my git push for
Georgecanfly Nov 3, 2019
0a1c8e2
[Outreachy] check-ref-format: parse options
Georgecanfly Nov 5, 2019
aeff5d9
[Outreachy] check-ref-format: parse-options
Georgecanfly Nov 7, 2019
d224564
[Outreachy] check-ref-format: parse-options
Georgecanfly Nov 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 23 additions & 24 deletions builtin/check-ref-format.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@
#include "refs.h"
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):

"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.

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, 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!

#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,
};

/*
* Return a copy of refname but with leading slashes removed and runs
Expand Down Expand Up @@ -53,33 +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;
enum {
CHECK_REFNAME_FORMAT = 0,
CHECK_REF_FORMAT_BRANCH,
} mode = CHECK_REFNAME_FORMAT;

int normalize = 0;
int flags = 0;
const char *refname;

if (argc == 2 && !strcmp(argv[1], "-h"))
usage(builtin_check_ref_format_usage);
struct option options[] = {
OPT_GROUP(""),
OPT_CMDMODE(0 , "branch", &mode, N_("check for valid branch name"), 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_BIT(0 , "refspec-pattern", &flags, N_("refspec pattern"), REFNAME_REFSPEC_PATTERN),
OPT_END(),
};

if (argc == 3 && !strcmp(argv[1], "--branch"))
return check_ref_format_branch(argv[2]);
argc = parse_options(argc, argv, prefix, options, builtin_check_ref_format_usage, 0);

for (i = 1; i < argc && argv[i][0] == '-'; i++) {
if (!strcmp(argv[i], "--normalize") || !strcmp(argv[i], "--print"))
normalize = 1;
else if (!strcmp(argv[i], "--allow-onelevel"))
flags |= REFNAME_ALLOW_ONELEVEL;
else if (!strcmp(argv[i], "--no-allow-onelevel"))
flags &= ~REFNAME_ALLOW_ONELEVEL;
else if (!strcmp(argv[i], "--refspec-pattern"))
flags |= REFNAME_REFSPEC_PATTERN;
else
usage(builtin_check_ref_format_usage);
}
if (! (i == argc - 1))
usage(builtin_check_ref_format_usage);

refname = argv[i];
refname = argv[0];
if (mode)
return check_ref_format_branch(argv[2]);
if (normalize)
refname = collapse_slashes(refname);
if (check_refname_format(refname, flags))
Expand Down
2 changes: 1 addition & 1 deletion git.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ static struct cmd_struct commands[] = {
{ "check-attr", cmd_check_attr, RUN_SETUP },
{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
{ "check-mailmap", cmd_check_mailmap, RUN_SETUP },
{ "check-ref-format", cmd_check_ref_format, NO_PARSEOPT },
{ "check-ref-format", cmd_check_ref_format },
{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
{ "checkout-index", cmd_checkout_index,
RUN_SETUP | NEED_WORK_TREE},
Expand Down