git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] [Outreachy] merge-ours: include a parse-option
@ 2019-10-28 23:42 George Espinoza via GitGitGadget
  2019-10-28 23:42 ` [PATCH 1/1] [Outreachy] merge-ours: include parse-options george espinoza via GitGitGadget
  0 siblings, 1 reply; 9+ messages in thread
From: George Espinoza via GitGitGadget @ 2019-10-28 23:42 UTC (permalink / raw)
  To: git; +Cc: George Espinoza, Junio C Hamano

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.

Outreachy Micro-Project #3 

Signed-off-by: george espinoza gespinoz2019@gmail.com
[gespinoz2019@gmail.com]

george espinoza (1):
  [Outreachy] merge-ours: include parse-options

 builtin/merge-ours.c | 14 ++++++++++----
 git.c                |  2 +-
 2 files changed, 11 insertions(+), 5 deletions(-)


base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-425%2FGeorgecanfly%2Fmerge-ours-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-425/Georgecanfly/merge-ours-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/425
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/1] [Outreachy] merge-ours: include parse-options
  2019-10-28 23:42 [PATCH 0/1] [Outreachy] merge-ours: include a parse-option George Espinoza via GitGitGadget
@ 2019-10-28 23:42 ` george espinoza via GitGitGadget
  2019-10-29 12:56   ` Johannes Schindelin
                     ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: george espinoza via GitGitGadget @ 2019-10-28 23:42 UTC (permalink / raw)
  To: git; +Cc: George Espinoza, Junio C Hamano, george espinoza

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.

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
  2019-10-28 23:42 ` [PATCH 1/1] [Outreachy] merge-ours: include parse-options george espinoza via GitGitGadget
@ 2019-10-29 12:56   ` Johannes Schindelin
  2019-10-29 20:42   ` Emily Shaffer
  2019-10-30  2:05   ` Junio C Hamano
  2 siblings, 0 replies; 9+ messages in thread
From: Johannes Schindelin @ 2019-10-29 12:56 UTC (permalink / raw)
  To: george espinoza via GitGitGadget
  Cc: git, George Espinoza, Junio C Hamano, george espinoza

Hi george,

On Mon, 28 Oct 2019, 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.

The commit message and the patch look good to me!

Thanks,
Johannes

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
  2019-10-28 23:42 ` [PATCH 1/1] [Outreachy] merge-ours: include parse-options george espinoza via GitGitGadget
  2019-10-29 12:56   ` Johannes Schindelin
@ 2019-10-29 20:42   ` Emily Shaffer
  2019-10-30  9:43     ` George Espinoza
  2019-10-30  2:05   ` Junio C Hamano
  2 siblings, 1 reply; 9+ messages in thread
From: Emily Shaffer @ 2019-10-29 20:42 UTC (permalink / raw)
  To: george espinoza via GitGitGadget; +Cc: git, George Espinoza, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
  2019-10-28 23:42 ` [PATCH 1/1] [Outreachy] merge-ours: include parse-options george espinoza via GitGitGadget
  2019-10-29 12:56   ` Johannes Schindelin
  2019-10-29 20:42   ` Emily Shaffer
@ 2019-10-30  2:05   ` Junio C Hamano
  2019-10-30  9:53     ` George Espinoza
  2019-10-30 21:58     ` Johannes Schindelin
  2 siblings, 2 replies; 9+ messages in thread
From: Junio C Hamano @ 2019-10-30  2:05 UTC (permalink / raw)
  To: george espinoza via GitGitGadget; +Cc: git, George Espinoza

"george espinoza via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

Sorry, but why do we even want to do this?  merge-ours is an
implementation detail of "git merge" and is never run directly by
end-users.

I am not sure why it even needs "-h" in the first place, but that is
already there, so letting sleeping dog lie would be what I would
prefer.

Is there a plan to add some -Xmerge-backend-option to this program,
and would use of parse-options API make it easier?  That would be a
good reason to start using it in this program, but otherwise...


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
  2019-10-29 20:42   ` Emily Shaffer
@ 2019-10-30  9:43     ` George Espinoza
  0 siblings, 0 replies; 9+ messages in thread
From: George Espinoza @ 2019-10-30  9:43 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: george espinoza via GitGitGadget, Git List, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
  2019-10-30  2:05   ` Junio C Hamano
@ 2019-10-30  9:53     ` George Espinoza
  2019-10-30 21:58     ` Johannes Schindelin
  1 sibling, 0 replies; 9+ messages in thread
From: George Espinoza @ 2019-10-30  9:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: george espinoza via GitGitGadget, Git List

On Tue, Oct 29, 2019 at 7:05 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "george espinoza via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.
>
> Sorry, but why do we even want to do this?  merge-ours is an
> implementation detail of "git merge" and is never run directly by
> end-users.
>

Hello Junio, this is my mistake. I am a first time contributor.
I'm currently working on Micro Project #3 as an Outreachy applicant
that states, "Teach a command which currently handles its own argv
how to use parse-options instead."

I was under the impression that all commands with the NOPARSE_OPT
flag in git.c needed parse-options added. I now see that I should take into
account for commands that are only ran by end-users :)

> I am not sure why it even needs "-h" in the first place, but that is
> already there, so letting sleeping dog lie would be what I would
> prefer.
>
> Is there a plan to add some -Xmerge-backend-option to this program,
> and would use of parse-options API make it easier?  That would be a
> good reason to start using it in this program, but otherwise...
>

I will look into finding another command to contribute to instead after
your feedback so I can fulfill Micro Project #3 since I had not thought of
further plans past the parse-option replacement. Thank you for your input!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
  2019-10-30  2:05   ` Junio C Hamano
  2019-10-30  9:53     ` George Espinoza
@ 2019-10-30 21:58     ` Johannes Schindelin
  2019-11-02  4:14       ` Junio C Hamano
  1 sibling, 1 reply; 9+ messages in thread
From: Johannes Schindelin @ 2019-10-30 21:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: george espinoza via GitGitGadget, git, George Espinoza

Hi Junio,

On Wed, 30 Oct 2019, Junio C Hamano wrote:

> "george espinoza via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.
>
> Sorry, but why do we even want to do this?

It _is_ a command you can run via `git merge-ours` by mistake. Don't you
think it would be nice for users to at least get a synopsis?

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
  2019-10-30 21:58     ` Johannes Schindelin
@ 2019-11-02  4:14       ` Junio C Hamano
  0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2019-11-02  4:14 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: george espinoza via GitGitGadget, git, George Espinoza

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Wed, 30 Oct 2019, Junio C Hamano wrote:
>
>> "george espinoza via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>> > 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.
>>
>> Sorry, but why do we even want to do this?
>
> It _is_ a command you can run via `git merge-ours` by mistake. Don't you
> think it would be nice for users to at least get a synopsis?

I think it would be good to tell users that the subcommand is not
what they want to run directly, instead of the synopsis to tell them
how to run it ;-).

So no.

But if merge-ours needs to learn its own -Xoption, it would make
sense to first convert it to use parse-options API and then add the
backend option support on top of it.  And the patch under discussion
in a polished form (by the way, has anybody pointed out that the use
of the verb "include" is a bit strange there on the patch title?)
would serve as a good first step for such a topic.

Thanks.



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-11-02  4:14 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-28 23:42 [PATCH 0/1] [Outreachy] merge-ours: include a parse-option George Espinoza via GitGitGadget
2019-10-28 23:42 ` [PATCH 1/1] [Outreachy] merge-ours: include parse-options george espinoza via GitGitGadget
2019-10-29 12:56   ` Johannes Schindelin
2019-10-29 20:42   ` Emily Shaffer
2019-10-30  9:43     ` George Espinoza
2019-10-30  2:05   ` Junio C Hamano
2019-10-30  9:53     ` George Espinoza
2019-10-30 21:58     ` Johannes Schindelin
2019-11-02  4:14       ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).