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

Georgecanfly · 2019-10-25T11:31:32Z

[Outreachy] merge-ours: include parse-options

This command currently handles its own argv so by teaching it to
use parse-options instead we can standardize the way commands
handle user input across the project. ex. -h.
NO_PARSEOPT flag was also removed to update git.c with the
conversion of the structure in this command.

Helped by: emily shaffer [email protected]
Helped by: johannes schindelin [email protected]

Signed-off-by: george espinoza [email protected]

dscho

Looks pretty good for starters!

If I were you, I would also try to make sure that every test that contains the word ours passes:

make -j$(nproc)
cd t
make -j$(nproc) $(git grep -nw ours t[0-9]*.sh)

builtin/merge-ours.c

nasamuffin · 2019-10-28T20:21:38Z

Yeah, this looks like it's lost the interesting part of your commit. All this PR consists of now is a header add.

Georgecanfly · 2019-10-28T20:40:48Z

Ok, I was able to push again thank you. Here I decided to revert my extreme first edits and start one bit a a time because I think I was breaking more things than fixing right away. I looked over both your comments very closely over the weekend and thought of different ways to implement all of the advice.
I adopted the first string usage completely and threw away the usage I had created.
I wanted to fix and include:

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

but I thought that the line that was already there was making this redundant and if i deleted the original one I got an error saying "builtin.h" was expecting:

static const char builtin_merge_ours_usage[]

so I left the one that was working. Unless I could just include both, with the first one I submitted being fixed.

I deleted this since there weren't any options being used, but I really thought of leaving it, which is why Im starting one step at a time instead.

	struct option options[] = {
		OPT_END()
	};

I also wanted to include:
argc = parse_options(argc, argv, prefix, options, merge-ours_usage, 0);

but I wasn't sure if that was me doing manual argv parsing (the wrong way, per the project description).
or if that was me giving more than the expected number of arguments after option parsing is 0.

I also wanted to delete since maybe this is what is doing the check for -h:


        if (argc == 2 && !strcmp(argv[1], "-h"))
                usage(builtin_merge_ours_usage);

but I wasn't sure if touching that would break something else. I had initially deleted -h by just doing


        if (argc == 2 && !strcmp(argv[1], " "))
                usage(builtin_merge_ours_usage);

But I got a test error so I decided to leave it in again. I wasn't sure how else to remove the check for -h.

nasamuffin · 2019-10-28T20:57:04Z

Ok, I was able to push again thank you. Here I decided to revert my extreme first edits and start one bit a a time because I think I was breaking more things than fixing right away. I looked over both your comments very closely over the weekend and thought of different ways to implement all of the advice.
I adopted the first string usage completely and threw away the usage I had created.
I wanted to fix and include:
static const char * const merge_ours_usage[] = {
	N_("git merge-ours [<base>...] -- <head> <merge-head>..."),
	NULL,
};
but I thought that the line that was already there was making this redundant and if i deleted the original one I got an error saying "builtin.h" was expecting:
static const char builtin_merge_ours_usage[]

It helps if you can share us the exact error so we can give a better hint.

so I left the one that was working. Unless I could just include both, with the first one I submitted being fixed.

I deleted this since there weren't any options being used, but I really thought of leaving it, which is why Im starting one step at a time instead.
	struct option options[] = {
		OPT_END()
	};

Don't delete this, the parse_options call needs it and the point is to convert to using this kind of structure.

I also wanted to include:
argc = parse_options(argc, argv, prefix, options, merge-ours_usage, 0);

but I wasn't sure if that was me doing manual argv parsing (the wrong way, per the project description).
or if that was me giving more than the expected number of arguments after option parsing is 0.

Using this line is the ask for this microproject. You should be using this line.

I also wanted to delete since maybe this is what is doing the check for -h:
        if (argc == 2 && !strcmp(argv[1], "-h"))
                usage(builtin_merge_ours_usage);

This part is the manual argv parsing. You can see we are doing a string compare on a member of argv dereferenced with a magic number, which is precisely what we mean by "manual parse".

but I wasn't sure if touching that would break something else. I had initially deleted -h by just doing
        if (argc == 2 && !strcmp(argv[1], " "))
                usage(builtin_merge_ours_usage);
But I got a test error so I decided to leave it in again. I wasn't sure how else to remove the check for -h.

-h should be handled by the call to parse-options. You may wish to remove in git.c where NO_PARSEOPT is passed as a flag to the merge-ours registration - I imagine that has to do with the failure you're seeing.

dscho · 2019-10-28T21:02:35Z

Oh, I also remembered another thing: parse_options() will "eat" a double-dash if it sees one. This can happen when trying to merge unrelated histories, in which case there is no merge base.

So you definitely want to pass the flag to parse_options() where it keeps the double-dash.

Georgecanfly · 2019-10-28T21:52:22Z

Ok, I think this might be better :)
After the weekend's trials and errors and today's extra comments gave me more of a revelation on how to go about this. I went ahead and did a make and did a test after the changes.

Georgecanfly · 2019-10-28T21:52:47Z

As I typed this I do see some CI failures. So maybe I'll rebuild and re-test.

Georgecanfly · 2019-10-28T21:57:49Z

I have to admit that I did make and prove before changing git.c.

Now that I'm running make Im seeing this error:

builtin/merge-ours.c: In function ‘cmd_merge_ours’:
builtin/merge-ours.c:22:16: error: array type has incomplete element type ‘struct option’
22 | struct option options[] = {
| ^~~~~~~
builtin/merge-ours.c:23:3: error: implicit declaration of function ‘OPT_END’; did you mean ‘EXT_END’? [-Werror=implicit-function-declaration]
23 | OPT_END()
| ^~~~~~~
| EXT_END
builtin/merge-ours.c:26:9: error: implicit declaration of function ‘parse_options’; did you mean ‘SSL_set_options’? [-Werror=implicit-function-declaration]
26 | argc = parse_options(argc, argv, prefix, options, merge_ours_usage, 0);
| ^~~~~~~~~~~~~
| SSL_set_options
builtin/merge-ours.c:22:16: error: unused variable ‘options’ [-Werror=unused-variable]
22 | struct option options[] = {
| ^~~~~~~
cc1: all warnings being treated as errors
make: *** [Makefile:2378: builtin/merge-ours.o] Error 1

Is is because I removed:
| NO_PARSEOPT }

from

{ "merge-ours", cmd_merge_ours, RUN_SETUP | NO_PARSEOPT },

Georgecanfly · 2019-10-28T22:01:05Z

Oh, that's really weird. It deleted the #include "parse-options.h" and my comment.
While I was building I did see something that said rewind.
Ok I'll add that again.

Georgecanfly · 2019-10-28T22:15:00Z

I think i fixed it finally :) let me know what you both think. @nasamuffin @dscho Thank you so much for your help so far! I'm starting to feel more confident in adding parse-options to commands.

nasamuffin · 2019-10-28T22:28:26Z

My opinion is that you should:

try and make the commit message conform better to Git's style, which is to focus on the "why" rather than the what, and then
send it to the list, because the code looks ready, assuming the tests all pass nicely.

For 1), I suggest you read through some of the other commits (besides Junio's merge commits) to get a good idea of what they're trying to convey. A good rule is that you shouldn't just summarize your diff - you probably want to explain why you made the diff you did. So it's not enough to say "I made merge-ours use parse-options" - you want to say something like "...because parse-options helps make sure we handle user input like -h in a standardized way across the project". And rather than saying "I removed NO_PARSEOPT from git.c too" you can say "Since merge-ours now uses parse-options, update git.c accordingly" (note that now it says why you removed NO_PARSEOPT).

Georgecanfly · 2019-10-28T23:03:54Z

ok @nasamuffin! Thank you :)

Georgecanfly · 2019-10-28T23:41:33Z

/submit

gitgitgadget · 2019-10-28T23:42:43Z

Submitted as [email protected]

builtin/merge-ours.c

gitgitgadget · 2019-10-29T20:43:43Z

builtin/merge-ours.c

@@ -11,14 +11,20 @@
 #include "git-compat-util.h"


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

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

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

This command currently handles its own argv so by teaching it to use parse-options instead we can standardize the way commands handle user input across the project. ex. -h. NO_PARSEOPT flag was also removed to update git.c with the conversion of the structure in this command. Helped by: emily shaffer <[email protected]> Helped by: johannes schindelin <[email protected]> Signed-off-by: george espinoza <[email protected]>

Georgecanfly · 2019-10-29T23:36:17Z

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 On Tue, Oct 29, 2019 at 1:43 PM gitgitgadget[bot] <[email protected]> wrote:

***@***.***[bot]* commented on this pull request. ------------------------------ In builtin/merge-ours.c <#425 (comment)>: > @@ -11,14 +11,20 @@ #include "git-compat-util.h" On the Git mailing list ***@***.***>, Emily Shaffer wrote (reply to this <https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis>): On Mon, Oct 28, 2019 at 11:42:29PM +0000, george espinoza via GitGitGadget wrote: > From: george espinoza ***@***.***> > > 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 ***@***.***> > --- > 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 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#425?email_source=notifications&email_token=ALVJ6I23YS5KDHNMVNNDXNTQRCOABA5CNFSM4JFCFQGKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJUDYZY#pullrequestreview-308821095>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALVJ6IYHGSAO5LKF5TQ5JI3QRCOABANCNFSM4JFCFQGA> .

force-pushed version; review no longer applies.

Georgecanfly changed the title ~~merge-ours: include a parse-option~~ [Outreachy] merge-ours: include a parse-option Oct 25, 2019

dscho previously requested changes Oct 25, 2019

View reviewed changes

builtin/merge-ours.c Outdated Show resolved Hide resolved

builtin/merge-ours.c Outdated Show resolved Hide resolved

nasamuffin reviewed Oct 25, 2019

View reviewed changes

builtin/merge-ours.c Outdated Show resolved Hide resolved

builtin/merge-ours.c Outdated Show resolved Hide resolved

Georgecanfly force-pushed the merge-ours branch from 2e5e102 to 8c25564 Compare October 28, 2019 20:13

Georgecanfly force-pushed the merge-ours branch from 8c25564 to 73a00c9 Compare October 28, 2019 21:47

Georgecanfly force-pushed the merge-ours branch from 73a00c9 to ecdd854 Compare October 28, 2019 22:10

Georgecanfly force-pushed the merge-ours branch from ecdd854 to 8c08819 Compare October 28, 2019 23:12

gitgitgadget bot reviewed Oct 29, 2019

View reviewed changes

builtin/merge-ours.c Show resolved Hide resolved

gitgitgadget bot reviewed Oct 29, 2019

View reviewed changes

Georgecanfly force-pushed the merge-ours branch from 8c08819 to 085aeea Compare October 29, 2019 23:40

Georgecanfly changed the title ~~[Outreachy] merge-ours: include a parse-option~~ [Outreachy] merge-ours: iparse-option Nov 5, 2019

Georgecanfly changed the title ~~[Outreachy] merge-ours: iparse-option~~ [Outreachy] merge-ours: parse-option Nov 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Georgecanfly commented Oct 25, 2019 •

edited

Loading

dscho left a comment

nasamuffin commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019 •

edited

Loading

nasamuffin commented Oct 28, 2019

dscho commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

nasamuffin commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

gitgitgadget bot commented Oct 28, 2019

gitgitgadget bot Oct 29, 2019

Georgecanfly Oct 29, 2019

gitgitgadget bot Oct 30, 2019

Georgecanfly commented Oct 29, 2019 via email

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

Are you sure you want to change the base?

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

Conversation

Georgecanfly commented Oct 25, 2019 • edited Loading

dscho left a comment

Choose a reason for hiding this comment

nasamuffin commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019 • edited Loading

nasamuffin commented Oct 28, 2019

dscho commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

nasamuffin commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

Georgecanfly commented Oct 28, 2019

gitgitgadget bot commented Oct 28, 2019

gitgitgadget bot Oct 29, 2019

Choose a reason for hiding this comment

Georgecanfly Oct 29, 2019

Choose a reason for hiding this comment

gitgitgadget bot Oct 30, 2019

Choose a reason for hiding this comment

Georgecanfly commented Oct 29, 2019 via email

Georgecanfly commented Oct 25, 2019 •

edited

Loading

Georgecanfly commented Oct 28, 2019 •

edited

Loading