refspec: add public parsing api by cynecx · Pull Request #4519 · libgit2/libgit2

cynecx · 2018-02-07T21:35:46Z

This PR adds public apis for parsing refspec strings. This already exists but they are all internal.
I am not sure if tests are required here because I guess that the internal api must have tests already and this PR just adds trivial wrapping. But I am happy to add tests if it's really requested.

tiennou · 2018-02-07T22:52:48Z


+int git_refspec_parse(git_refspec **refspec, const char *input, int is_fetch) {
+	git_refspec *new_refspec;
+	if (refspec == NULL || input == NULL) {


That should be an assert.

tiennou · 2018-02-07T22:54:07Z

+
+	is_fetch = !!is_fetch;
+	new_refspec = git__malloc(sizeof(git_refspec));
+	if (new_refspec == NULL) {


GITERR_CHECK_ALLOC is what you want here.

tiennou · 2018-02-07T22:59:50Z

+}
+
+void git_refspec_free(git_refspec *refspec) {
+	if (refspec == NULL) {


I'd be tempted to say that this test is not needed : git_refspec__free already handles it, and git__free doesn't care about NULL.

tiennou · 2018-02-07T23:00:09Z

+		return -1;
+	}
+
+	is_fetch = !!is_fetch;


I'd move that inside the git_refspec__parse call.

tiennou · 2018-02-07T23:02:54Z

It'd be 😍 if you squashed those commits, too !

cynecx · 2018-02-07T23:52:20Z

@tiennou Done!

pks-t

I'm a bit on the edge whether that API makes sense to expose. Not saying that it shouldn't, but I still would like to know of a certain use case where it would be handy. I'm especially coming from the angle that we have no functions exposed which make real use of the refspec.

I'd definitly like to see a test, though. While the new function mostly wraps the exisitng git_refspec__parse, it's still 14 lines of new code which are never being exercised.

pks-t · 2018-02-08T09:04:15Z

+ * @param refspec a pointer to hold the refspec handle
+ * @param input the refspec string
+ * @param is_fetch is this a refspec for a fetch
+ * @return 0 if the refspec string could be parsed, 1 otherwise


1 otherwise is a bit weird. We always return -1 for errors.

Sorry the function is actually returning -1, but this is obviously a documentation error.

pks-t · 2018-02-08T09:06:18Z

 	memset(refspec, 0x0, sizeof(git_refspec));
 }

+int git_refspec_parse(git_refspec **refspec, const char *input, int is_fetch) {


Well, this is definitly is a bit more than a "simple wrapper", regarding it has 14 lines of code. So it should be worth a test

While you're fixing up this PR with @pks-t 's changes, please put this { on a newline to match our code formatting rules. (Thanks!)

Ah, thanks! I noticed this, but forgot to write a comment

I'll push some tests right away.

@pks-t About the tests, would a simple valid/not-valid parsing case be sufficient (testing object creation) and a simple comment that the logic should already be tested in the internal version of the api suffice?

@cynex Should suffice in this case, yes.

pks-t · 2018-02-08T09:07:10Z

+	new_refspec = git__malloc(sizeof(git_refspec));
+	GITERR_CHECK_ALLOC(new_refspec);
+
+	if (!git_refspec__parse(new_refspec, input, is_fetch)) {


You shouldn't pass is_fetch as, as git_refspe__parse accepts a bool. So instead of moving is_fetch = !!is_fetch into git_refspec__parse, you should have that here.

@tiennou mentioned to move that line away. But I can move it back here.

Sorry, coding style is always a soft thing and different people have different views :P

On another note: you could also just move it into the git_refspec__parse line, without first assigning it to itself.

ethomson · 2018-02-08T10:58:40Z

+	return 0;
+}
+
+void git_refspec_free(git_refspec *refspec) {


Same with this {.

Sorry, didn't noticed that. Should be fixed.

ethomson · 2018-02-09T09:50:17Z

I'm a bit on the edge whether that API makes sense to expose. Not saying that it shouldn't, but I still would like to know of a certain use case where it would be handy. I'm especially coming from the angle that we have no functions exposed which make real use of the refspec.

Same. @cynecx, understanding your motivations here would help us get more excited about this. As I'm sure you can imagine, every new API we add is something else that we have to maintain, so we're loathe to add new public API unless it's warranted.

We don't want to just expose API for the sake of completeness. So understanding what you're trying to do (and how this helps you do it) is nice so that we know that we should take on this future maintenance burden in order to help you build your software. 😃

cynecx · 2018-02-10T17:40:21Z

Sorry about the delay.

@pks-t @ethomson Well, I am currently developing a custom git remote handler which accepts a command which accepts a refspec as an argument (and my handler obviously uses libgit2). I was actually aiming for reducing code duplication by reusing parsing code which already exists in libgit2. That's my main motivation here, I am not sure if this will convince you but it would definitely offload some parsing logic, which is always good IMO.

pks-t

Thanks for your scenario, makes sense to me. Disregarding the micro-nit about !!is_fetch, which you might fix if you feel like it, the only thing missing now is a test for this. As you proposed, two simple tests exercising a succeeding and failing parse should be sufficient.

pks-t · 2018-02-16T08:50:48Z

+	new_refspec = git__malloc(sizeof(git_refspec));
+	GITERR_CHECK_ALLOC(new_refspec);
+
+	if (!git_refspec__parse(new_refspec, input, is_fetch)) {


On another note: you could also just move it into the git_refspec__parse line, without first assigning it to itself.

cynecx · 2018-06-17T19:21:55Z

Sorry about the long delay. I've just rebased the branch and added a small test.

pks-t

No worries about the delay, we're usually not the quickest either ;) There's two small nits about out-parameter sanitization and cl_git_pass/cl_git_fail, which is why I also added some minor style recommendations. You don't necessarily need to address the style issues, but I'd be happy if you did :) I'd also ask you to squash the commits together afterwards, as I expect this to be the last round of reviews.

Thanks for your work!

pks-t · 2018-06-22T07:36:00Z

+
+	spec = NULL;
+
+	error = git_refspec_parse(&spec, "", 0);


You can get rid of the error for this and all the following invocations, as we're always using cl_git_pass and cl_git_fail to check for errors when the invoked function is conforming to libgit2's error style. So:

cl_git_fail(git_refspec_parse(&spec, "", 0)); cl_git_fail(git_refspec_parse(&spec, ":::", 0)); cl_git_pass(git_refspec_pares(&spec, "HEAD:", 1));

pks-t · 2018-06-22T07:37:12Z

+{
+	git_refspec *new_refspec;
+	assert(refspec && input);
+


I'd like to make sure that we always sanitize the out parameter. So the first thing this function does should be *refspec = NULL.

pks-t · 2018-06-22T07:37:51Z

+	GITERR_CHECK_ALLOC(new_refspec);
+
+	is_fetch = !!is_fetch;
+	if (git_refspec__parse(new_refspec, input, is_fetch) != 0) {


You could just pull the !!is_fetch into this function. So if (git_refspec__parse(new_refspec, input, !!is_fetch) != 0)

pks-t · 2018-06-22T07:38:52Z

 	memset(refspec, 0x0, sizeof(git_refspec));
 }

+int git_refspec_parse(git_refspec **refspec, const char *input, int is_fetch)


Minor nit that doesn't need addressing: we usually name out-parameters out or similar. In case you rename it to out, you could also just rename new_refspec to refspec.

cynecx · 2018-06-22T11:08:47Z

I've squashed them and addressed the styling issues :)

Fix typo Fix some type issues More fixes Address requested changes Add test Fix naming Fix condition and tests Address requested changes Fix typo

cynecx · 2018-06-26T20:57:50Z

ping.

pks-t · 2018-06-29T09:33:53Z

Kicking CI as it had some spurious failures.

pks-t · 2018-06-29T09:43:43Z

Thanks for your contribution!

cynecx mentioned this pull request Feb 7, 2018

Support Refspec generation by using libgit2's internal parsing api rust-lang/git2-rs#296

Closed

tiennou reviewed Feb 7, 2018

View reviewed changes

cynecx force-pushed the refspec-parsing branch from 3de7e01 to 5127cd5 Compare February 7, 2018 23:49

pks-t requested changes Feb 8, 2018

View reviewed changes

ethomson reviewed Feb 8, 2018

View reviewed changes

pks-t requested changes Feb 16, 2018

View reviewed changes

pks-t added the feedback provided label Mar 26, 2018

cynecx force-pushed the refspec-parsing branch from 7b3453e to 073c1bf Compare June 17, 2018 18:45

pks-t reviewed Jun 22, 2018

View reviewed changes

cynecx force-pushed the refspec-parsing branch 2 times, most recently from 6798dfa to 6dede7c Compare June 22, 2018 11:07

refspec: add public parsing api

630a673

Fix typo Fix some type issues More fixes Address requested changes Add test Fix naming Fix condition and tests Address requested changes Fix typo

cynecx force-pushed the refspec-parsing branch from d1b3260 to 630a673 Compare June 22, 2018 11:19

pks-t approved these changes Jun 29, 2018

View reviewed changes

pks-t merged commit 7192e26 into libgit2:master Jun 29, 2018

cynecx deleted the refspec-parsing branch August 30, 2018 02:37

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Uh oh!

Conversation

cynecx commented Feb 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou commented Feb 7, 2018

Uh oh!

cynecx commented Feb 7, 2018

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson commented Feb 9, 2018

Uh oh!

cynecx commented Feb 10, 2018

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cynecx commented Jun 17, 2018

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cynecx commented Jun 22, 2018

Uh oh!

cynecx commented Jun 26, 2018