Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options

From: George Espinoza <gespinoz2019@gmail.com>
To: Emily Shaffer <emilyshaffer@google.com>
Cc: george espinoza via GitGitGadget <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
Date: Wed, 30 Oct 2019 02:43:27 -0700	[thread overview]
Message-ID: <CAP_ANinZuD645D8CniRJxscCkE3Se-uwAHFCtJCwKgt31+Pcig@mail.gmail.com> (raw)
In-Reply-To: <20191029204228.GA229589@google.com>

On Tue, Oct 29, 2019 at 1:42 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> On Mon, Oct 28, 2019 at 11:42:29PM +0000, george espinoza via GitGitGadget wrote:
> > From: george espinoza <gespinoz2019@gmail.com>
> >
> > 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 <gespinoz2019@gmail.com>
> > ---
> >  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