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

[Outreachy] merge-ours: parse-option #425

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+11 −5
Open

[Outreachy] merge-ours: parse-option #425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 10 additions & 4 deletions builtin/merge-ours.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,20 @@
#include "git-compat-util.h"
dscho marked this conversation as resolved.
Show resolved Hide resolved
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, Emily Shaffer wrote (reply to this):

On Mon, Oct 28, 2019 at 11:42:29PM +0000, george espinoza via GitGitGadget wrote:
> From: george espinoza <[email protected]>
> 
> Teach this command which currently handles its own argv to use
> parse-options instead because parse-options helps make sure we handle
> user input like -h in a standardized way across the project.
> Also deleted the NO_PARSEOPT flag from git.c to coincide with
> the conversion of the structure in this command since merge-ours
> now uses parse-options and needed to update git.c accordingly.

Hmm, I could still wish for some rephrasing on this commit message. Now
that you took my example suggestions and pasted them directly into your
previous sentences, it doesn't flow very nicely. The point comes across
but it's a little redundant (for example, "also <verb> from git.c ....
and needed to update git.c" is redundant).

When significant assistance and advice is received it's often good form
to include a "Helped-by:" line which looks similar to the signoff line,
to provide credit. Maybe you will consider it as myself and dscho spent
quite some time helping you in the GitGitGadget PR as well as via
IRC/mail? :)

Otherwise, the code looks OK to me.

 - Emily

> 
> Signed-off-by: george espinoza <[email protected]>
> ---
>  builtin/merge-ours.c | 14 ++++++++++----
>  git.c                |  2 +-
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
> index 4594507420..fb3674a384 100644
> --- a/builtin/merge-ours.c
> +++ b/builtin/merge-ours.c
> @@ -11,14 +11,20 @@
>  #include "git-compat-util.h"
>  #include "builtin.h"
>  #include "diff.h"
> +#include "parse-options.h"
>  
> -static const char builtin_merge_ours_usage[] =
> -	"git merge-ours <base>... -- HEAD <remote>...";
> +static const char * const merge_ours_usage[] = {
> +	N_("git merge-ours [<base>...] -- <head> <merge-head>..."),
> +	NULL,
> +};
>  
>  int cmd_merge_ours(int argc, const char **argv, const char *prefix)
>  {
> -	if (argc == 2 && !strcmp(argv[1], "-h"))
> -		usage(builtin_merge_ours_usage);
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, merge_ours_usage, 0);
>  
>  	/*
>  	 * The contents of the current index becomes the tree we
> diff --git a/git.c b/git.c
> index ce6ab0ece2..6aee0e9477 100644
> --- a/git.c
> +++ b/git.c
> @@ -527,7 +527,7 @@ static struct cmd_struct commands[] = {
>  	{ "merge-base", cmd_merge_base, RUN_SETUP },
>  	{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
>  	{ "merge-index", cmd_merge_index, RUN_SETUP | NO_PARSEOPT },
> -	{ "merge-ours", cmd_merge_ours, RUN_SETUP | NO_PARSEOPT },
> +	{ "merge-ours", cmd_merge_ours, RUN_SETUP },
>  	{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
>  	{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
>  	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> -- 
> gitgitgadget

Copy link
Author

Choose a reason for hiding this comment

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

Ah, my sincerest apologies. I should have been more thorough in evaluating the importance of a clean original commit message before submitting it. I will certainly keep all of that in mind for this project and any future projects and contributions. I will edit it accordingly to your advice and include credit for the substantial and significant assistance you and dscho provided. :) ty.

-George

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 Tue, Oct 29, 2019 at 1:42 PM Emily Shaffer <[email protected]> wrote:
>
> On Mon, Oct 28, 2019 at 11:42:29PM +0000, george espinoza via GitGitGadget wrote:
> > From: george espinoza <[email protected]>
> >
> > Teach this command which currently handles its own argv to use
> > parse-options instead because parse-options helps make sure we handle
> > user input like -h in a standardized way across the project.
> > Also deleted the NO_PARSEOPT flag from git.c to coincide with
> > the conversion of the structure in this command since merge-ours
> > now uses parse-options and needed to update git.c accordingly.
>
> Hmm, I could still wish for some rephrasing on this commit message. Now
> that you took my example suggestions and pasted them directly into your
> previous sentences, it doesn't flow very nicely. The point comes across
> but it's a little redundant (for example, "also <verb> from git.c ....
> and needed to update git.c" is redundant).
>
> When significant assistance and advice is received it's often good form
> to include a "Helped-by:" line which looks similar to the signoff line,
> to provide credit. Maybe you will consider it as myself and dscho spent
> quite some time helping you in the GitGitGadget PR as well as via
> IRC/mail? :)
>
> Otherwise, the code looks OK to me.
>
>  - Emily
>
> >
> > Signed-off-by: george espinoza <[email protected]>
> > ---
> >  builtin/merge-ours.c | 14 ++++++++++----
> >  git.c                |  2 +-
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
> > index 4594507420..fb3674a384 100644
> > --- a/builtin/merge-ours.c
> > +++ b/builtin/merge-ours.c
> > @@ -11,14 +11,20 @@
> >  #include "git-compat-util.h"
> >  #include "builtin.h"
> >  #include "diff.h"
> > +#include "parse-options.h"
> >
> > -static const char builtin_merge_ours_usage[] =
> > -     "git merge-ours <base>... -- HEAD <remote>...";
> > +static const char * const merge_ours_usage[] = {
> > +     N_("git merge-ours [<base>...] -- <head> <merge-head>..."),
> > +     NULL,
> > +};
> >
> >  int cmd_merge_ours(int argc, const char **argv, const char *prefix)
> >  {
> > -     if (argc == 2 && !strcmp(argv[1], "-h"))
> > -             usage(builtin_merge_ours_usage);
> > +     struct option options[] = {
> > +             OPT_END()
> > +     };
> > +
> > +     argc = parse_options(argc, argv, prefix, options, merge_ours_usage, 0);
> >
> >       /*
> >        * The contents of the current index becomes the tree we
> > diff --git a/git.c b/git.c
> > index ce6ab0ece2..6aee0e9477 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -527,7 +527,7 @@ static struct cmd_struct commands[] = {
> >       { "merge-base", cmd_merge_base, RUN_SETUP },
> >       { "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
> >       { "merge-index", cmd_merge_index, RUN_SETUP | NO_PARSEOPT },
> > -     { "merge-ours", cmd_merge_ours, RUN_SETUP | NO_PARSEOPT },
> > +     { "merge-ours", cmd_merge_ours, RUN_SETUP },
> >       { "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> >       { "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> >       { "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> > --
> > gitgitgadget

Ah, my sincerest apologies. I should have been more thorough in
evaluating the importance of a clean original commit message before
submitting it. I will certainly keep all of that in mind for this
project and any future projects and contributions. I will edit it
accordingly to your advice and include credit for the substantial and
significant assistance you and dscho provided. :) ty.

-George

#include "builtin.h"
#include "diff.h"
#include "parse-options.h"

static const char builtin_merge_ours_usage[] =
"git merge-ours <base>... -- HEAD <remote>...";
static const char * const merge_ours_usage[] = {
N_("git merge-ours [<base>...] -- <head> <merge-head>..."),
NULL,
};

int cmd_merge_ours(int argc, const char **argv, const char *prefix)
{
if (argc == 2 && !strcmp(argv[1], "-h"))
usage(builtin_merge_ours_usage);
struct option options[] = {
OPT_END()
};

argc = parse_options(argc, argv, prefix, options, merge_ours_usage, 0);

/*
* The contents of the current index becomes the tree we
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 @@ -527,7 +527,7 @@ static struct cmd_struct commands[] = {
{ "merge-base", cmd_merge_base, RUN_SETUP },
{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
{ "merge-index", cmd_merge_index, RUN_SETUP | NO_PARSEOPT },
{ "merge-ours", cmd_merge_ours, RUN_SETUP | NO_PARSEOPT },
{ "merge-ours", cmd_merge_ours, RUN_SETUP },
{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
Expand Down