merge-ours: include a parse-option
cover merge-ours: include a parse-option
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
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
Signature · 1 line
gitgitgadget
PATCH 1/1 merge-ours: include parse-options
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.
--- builtin/merge-ours.c | 14 ++++++++++---- git.c | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-)
builtin/merge-ours.c +10 -4
@@ -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
git.c +1 -1
@@ -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 },
Signature · 1 line
gitgitgadget
Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
Hi george,
Quoted 8 lines · george espinoza via GitGitGadget
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_PARSEOPTflag fromgit.cto coincide with the conversion of the structure in this command since merge-ours now uses parse-options and needed to updategit.caccordingly.
The commit message and the patch look good to me!
Thanks,
Johannes
Quoted 52 lines
Signed-off-by george espinoza <[email protected]>--- builtin/merge-ours.c | 14 ++++++++++---- git.c | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-)builtin/merge-ours.c +10
-4@@ -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 git.c +1
-2@@ -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
Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
In reply to george espinoza via GitGitGadgetQuoted 8 lines · george espinoza via GitGitGadget
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_PARSEOPTflag fromgit.cto coincide with the conversion of the structure in this command since merge-ours now uses parse-options and needed to updategit.caccordingly.
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
Quoted 51 lines
Signed-off-by george espinoza <[email protected]>--- builtin/merge-ours.c | 14 ++++++++++---- git.c | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-)builtin/merge-ours.c +10
-4@@ -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 git.c +1
-1@@ -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 },Signature · 1 line
gitgitgadget
Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
Quoted 70 lines · Emily Shaffer
Quoted 8 lines · george espinoza via GitGitGadget
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_PARSEOPTflag fromgit.cto coincide with the conversion of the structure in this command since merge-ours now uses parse-options and needed to updategit.caccordingly.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 updategit.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
Quoted 43 lines
Signed-off-by george espinoza <[email protected]>--- builtin/merge-ours.c | 14 ++++++++++---- git.c | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-)builtin/merge-ours.c +10
-4@@ -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 git.c +0
-0@@ -527,7 +527,7 @@ static struct cmd_struct commands[] = { { "merge-base", cmd_merge_base, RUN_SETUP }, { "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
Quoted 8 lines
Quoted 8 lines
git.c +1
-2{ "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
Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
In reply to george espinoza via GitGitGadgetQuoted 5 lines · george espinoza via GitGitGadget
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.
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...
Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
Quoted 5 lines · Junio C Hamano
Quoted 2 lines · george espinoza via GitGitGadget
From: george espinoza <[email protected]>
Quoted 8 lines
Quoted 3 lines
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 :)
Quoted 8 lines
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!
Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
In reply to Junio C HamanoHi Junio,
Quoted 9 lines · Junio C Hamano
Quoted 5 lines · george espinoza via GitGitGadget
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.
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
Re: [PATCH 1/1] [Outreachy] merge-ours: include parse-options
Quoted 8 lines · Johannes Schindelin
Hi Junio,
Quoted 4 lines · Junio C Hamano
Quoted 2 lines · george espinoza via GitGitGadget
From: george espinoza <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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-oursby 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.