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

From: Emily Shaffer <emilyshaffer@google.com>
To: george espinoza via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, George Espinoza <gespinoz2019@gmail.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
Date: Tue, 29 Oct 2019 13:42:28 -0700	[thread overview]
Message-ID: <20191029204228.GA229589@google.com> (raw)
In-Reply-To: <8c088194f604eac3a6b00c48a7fddfdf807571fc.1572306149.git.gitgitgadget@gmail.com>

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