[Outreachy] check-ref-format: parse-options #666

Georgecanfly · 2019-11-03T05:27:14Z

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.

As a consequence of using OPT_BOOL data structure on --normalize &
--refspec-pattern, --no-normalize & --no-refspec-pattern can now be
used.

NO_PARSEOPT flag was also removed to update git.c with the
conversion of the structure in this command.

This is a rough draft and I need some advice if I'm doing this
correctly since its being built but it is failing tests.

Helped by: emily shaffer emilyshaffer@google.com
Helped by: johannes schindelin johannes.schindelin@gmx.de

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

Prepare this command which currently handles its own argv to use parse-options instead. A lot of the commands already have parse-options and in git.c cmd_struct with the "NO_PARSEOPT" are the one's that still need it. Signed-off-by: george espinoza <gespinoz2019@gmail.com>

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. As a consequence of using OPT_BOOL data structure on --normalize & --refspec-pattern, --no-normalize & --no-refspec-pattern has been can now be used. NO_PARSEOPT flag was also removed to update git.c with the conversion of the structure in this command. This is a rough draft and I need some advice if I'm doing this correctly since its being built but it is failing tests. Helped by: emily shaffer <emilyshaffer@google.com> Helped by: johannes schindelin <johannes.schindelin@gmx.de> Signed-off-by: george espinoza <gespinoz2019@gmail.com>

check-ref-format :( Signed-off-by: george espinoza <gespinoz2019@gmail.com>

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. As a consequence of using OPT_BOOL data structure on --normalize & --refspec-pattern, --no-normalize & --no-refspec-pattern has been can now be used. NO_PARSEOPT flag was also removed to update git.c with the conversion of the structure in this command. This is a rough draft and I need some advice if I'm doing this correctly since its being built but it is failing tests. Helped by: emily shaffer <emilyshaffer@google.com> Helped by: johannes schindelin <johannes.schindelin@gmx.de> Signed-off-by: george espinoza <gespinoz2019@gmail.com>

Georgecanfly · 2019-11-03T05:28:20Z

These are the tests my function edits are failing. @nasamuffin @dscho

https://pastebin.com/GDV7F2WP

Georgecanfly · 2019-11-03T05:30:30Z

When I was pushing it took an old commit and brought it into this fork as well, I tried rebase -i to delete it and ended up just manually putting merge-ours.c back to how it was before my edits.
I had seen it after I did this:

~/git$ git pull -r
First, rewinding head to replay your work on top of it...
Applying: [Outreachy] merge-ours: include parse-option.h

dscho

Only a couple minor comments.

dscho · 2019-11-04T11:57:25Z

builtin/check-ref-format.c

+	enum {
+		CHECK_REF_FORMAT_BRANCH,
+	};
+	int i = 0;


I think it would make more sense to get rid of i now. Its role was to iterate through the command-line options, which is now no longer performed in this function.

dscho · 2019-11-04T11:58:01Z

builtin/check-ref-format.c

@@ -53,31 +56,28 @@ static int check_ref_format_branch(const char *arg)

 int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 {
-	int i;
-	int normalize = 0;


I think normalize still needs to be initialized, lest it is used in an uninitialized state.

dscho · 2019-11-04T11:58:12Z

builtin/check-ref-format.c

+	int verbose;
+	int normalize;
+	int allow_onelevel;
+	int refspec_pattern;


All of these need to be initialized to default values.

dscho · 2019-11-04T12:04:33Z

These are the tests my function edits are failing. @nasamuffin @dscho

https://pastebin.com/GDV7F2WP

Better would have been to paste it here, in this bug tracker.

Even better would have been to run the test scripts with -i -v -x, as described e.g. here: https://github.com/git-for-windows/git/wiki/Running-Git's-regression-tests#running-individual-tests

This is essentially what the Azure Pipeline does, and when you follow the failed checks all the way to the tests tab, you will see this: https://dev.azure.com/git/git/_build/results?buildId=1300&view=ms.vss-test-web.build-test-results-tab&runId=1018498&resultId=104057&paneView=attachments (you will want to either scroll down in the right pane, all the way to the end, or click into that pane to set the focus, then search via Ctrl+F):

expecting success of 1402.6 'ref name 'foo/bar/baz' is valid': 
		git check-ref-format  'foo/bar/baz'
	
++ git check-ref-format foo/bar/baz
error: last command exited with $?=1
not ok 6 - ref name 'foo/bar/baz' is valid

#	
#			git check-ref-format  'foo/bar/baz'
#		
expecting success of 1402.7 'ref name 'foo/bar/baz' is valid with options --normalize': 
		git check-ref-format --normalize 'foo/bar/baz'
	
++ git check-ref-format --normalize foo/bar/baz
error: last command exited with $?=1

That is not really helpful. I suspect that all of these failures come from the fact that most of the newly-added variables are uninitialized.

Georgecanfly · 2019-11-04T20:33:10Z

Ok great @dscho I'll look into all your suggestions from your comments. Thank you! I also just realized I did a PR on git's normal repository and not through gitgitgadget. So I'll have to transfer over my work there.

dscho · 2019-11-05T13:00:21Z

So I'll have to transfer over my work there.

Okay! Please do close this here PR after that, and add a link to the other PR, so that lurkers are redirected to the correct place.

Georgecanfly · 2019-11-07T03:10:31Z

Correct place for a PR has been moved to GitGitGadget:

gitgitgadget#449

Closing this PR.

In 08809c0 (mingw: add a helper function to attach GDB to the current process, 2020-02-13), I added a declaration that was not needed. Back then, that did not matter, but now that the declaration of that symbol was changed in mingw-w64's headers, it causes the following compile error: CC compat/mingw.o compat/mingw.c: In function 'open_in_gdb': compat/mingw.c:35:9: error: function declaration isn't a prototype [-Werror=strict-prototypes] 35 | extern char *_pgmptr; | ^~~~~~ In file included from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/mm_malloc.h:27, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/xmmintrin.h:34, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/immintrin.h:31, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/lib/gcc/x86_64-w64-mingw32/14.1.0/include/x86intrin.h:32, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/winnt.h:1658, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/minwindef.h:163, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/windef.h:9, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/windows.h:69, from C:/git-sdk-64/usr/src/git/build-installers/mingw64/include/winsock2.h:23, from compat/../git-compat-util.h:215, from compat/mingw.c:1: compat/mingw.c:35:22: error: '__p__pgmptr' redeclared without dllimport attribute: previous dllimport ignored [-Werror=attributes] 35 | extern char *_pgmptr; | ^~~~~~~ Let's just drop the declaration and get rid of this compile error. This PR integrates the fix early that had been contributed in gitgitgadget#1752 and was already integrated early into Git for Windows via git-for-windows#5017.

Georgecanfly added 4 commits November 2, 2019 21:49

This file wasn't supposed to change during my git push for

078a19e

check-ref-format :( Signed-off-by: george espinoza <gespinoz2019@gmail.com>

mahmud1989 approved these changes Nov 3, 2019

View reviewed changes

dscho reviewed Nov 4, 2019

View reviewed changes

Georgecanfly closed this Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Outreachy] check-ref-format: parse-options #666

[Outreachy] check-ref-format: parse-options #666

Uh oh!

Georgecanfly commented Nov 3, 2019 •

edited

Loading

Uh oh!

Georgecanfly commented Nov 3, 2019

Uh oh!

Georgecanfly commented Nov 3, 2019

Uh oh!

dscho left a comment

Uh oh!

dscho Nov 4, 2019

Uh oh!

dscho Nov 4, 2019

Uh oh!

dscho Nov 4, 2019

Uh oh!

dscho commented Nov 4, 2019

Uh oh!

Georgecanfly commented Nov 4, 2019

Uh oh!

dscho commented Nov 5, 2019

Uh oh!

Georgecanfly commented Nov 7, 2019

Uh oh!

Uh oh!

[Outreachy] check-ref-format: parse-options #666

[Outreachy] check-ref-format: parse-options #666

Uh oh!

Conversation

Georgecanfly commented Nov 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Georgecanfly commented Nov 3, 2019

Uh oh!

Georgecanfly commented Nov 3, 2019

Uh oh!

dscho left a comment

Choose a reason for hiding this comment

Uh oh!

dscho Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

dscho Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

dscho Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

dscho commented Nov 4, 2019

Uh oh!

Georgecanfly commented Nov 4, 2019

Uh oh!

dscho commented Nov 5, 2019

Uh oh!

Georgecanfly commented Nov 7, 2019

Uh oh!

Uh oh!

Georgecanfly commented Nov 3, 2019 •

edited

Loading