[Submodule] Git submodule dup by lolgear · Pull Request #5890 · libgit2/libgit2

lolgear · 2021-05-16T20:55:17Z

git_submodule_dup has been added.
[Submodule] Missing retain function #5889

neithernut

Disclaimer: not a maintainer.

neithernut · 2021-05-19T14:31:20Z

+git_submodule* git_submodule_dup(git_submodule *sm)
+{
+	if (!sm)
+		return;


I guess you meant to return either sm or, explicitly, NULL?

Sure, which one is more appropriate (in current codebase)?

I think that would be NULL.

I would emulate the existing dup functions like git_commit_dup and return an int and use an out parameter. This way we can change the implementation later in case we add some failure case.

@ethomson

Should I just use git_object_dup function?

int git_submodule_dup(git_submodule **out, git_submodule *source) { return git_object_dup((git_object **)out, (git_object *)source); }

tiennou · 2021-05-26T11:59:08Z


+int git_submodule_dup(git_submodule **out, git_submodule *source)
+{
+	return git_object_dup((git_object **)out, (git_object *)source);


That won't work — git_submodule is not a proper subtype/C "subclass" of git_object, so this will actually clobber memory in the underlying structure when incrementing the refcount.

IIRC your previous version (using GIT_REFCOUNT_INC) was correct but the function's prototype wasn't.

I can't figure out how to adjust *out parameter with previous implementation.
Something like this?

int git_submodule_dup(git_submodule **out, git_submodule *source) { if (!out) { return -1; } if (!source) { return -1; } *out = source; GIT_REFCOUNT_INC(*out); return 0; }

I'd (personally) go for this :

int git_submodule_dup(git_submodule **out, git_submodule *source) { GIT_ASSERT_ARG(out); if (source != NULL) GIT_REFCOUNT_INC(source); *out = source; return 0; }

Assert because allowing a NULL out could open up the API to misuse/programmer error, only incref if source is non-NULL so a NULL "submodule" can pass through, and always copy either an incref-ed source or NULL in out so it's always a known value and not uninitialized memory.

By the way, project has different styles in spaces/tabs and in nullability checks.
Is there any (current) guide to look at?

There's docs/coding-style.md, which is supposed to be current and seems to be. I'd maybe add something about the assertion macros (which are new-ish) and when to use them — ideally for "impossible" or "API misuse" conditions.

… has been added.

tiennou · 2021-05-26T14:27:24Z

+{
+	GIT_ASSERT_ARG(out);
+
+	if (source != NULL)


Yeah… if (source) would have been fine here. Sorry, another project of mine has "all conditions must be explicit", so…

There are a lot of occurrences of != NULL in this file. Should I change it to implicit check?

lolgear · 2021-05-31T15:50:43Z

Could anybody check this PR?

tiennou

Just a few minor docblock tweaks (I skimmed the header initially). A quick test that exercises the new function and check that both submodules work and can be freed separately would be ❤️, just to be sure nothing regresses.

tiennou · 2021-05-31T18:09:58Z

+ * Create an in-memory copy of a submodule. The copy must be explicitly
+ * free'd or it will leak.
+ *
+ * @param out Pointer to store the copy of the submodule


Suggested change

* @param out Pointer to store the copy of the submodule

* @param out Pointer to store the copy of the submodule. Cannot be NULL.

tiennou · 2021-05-31T18:10:21Z

+ * free'd or it will leak.
+ *
+ * @param out Pointer to store the copy of the submodule
+ * @param source Original tag to copy


Suggested change

* @param source Original tag to copy

* @param source Original submodule to copy.

lolgear · 2021-06-01T15:01:24Z

@tiennou
Could you show me an example of this test?
This function is so common that I can't figure out whether I should add test in separate file or add it in one of the existed files.

tiennou · 2021-06-02T11:01:10Z

Sure, I'd put it as test_submodule_lookup__can_be_dupped, which would git_submodule_lookup(), dup that, check that one of the "basic" properties work (name/owner/url, IIRC any one of them, as it's just about accessing the underlying storage), then free the first and assert whatever you checked at the beginning is still what we expect. In case anything goes wrong with memory in the future, the test would cause a segfault.

lolgear · 2021-06-03T11:04:06Z

@tiennou
Could you check latest changes?
I guess that it is done, but I can't run tests.
A lot of errors at "configure" phase.

ethomson

Overall looks great, a few minor suggestions.

ethomson · 2021-06-14T23:03:03Z

+
+	if (source != NULL)
+		GIT_REFCOUNT_INC(source);
+


Is there a reason source is optional? I would make it required.

Suggested change

if (source != NULL)

GIT_REFCOUNT_INC(source);

GIT_ASSERT_ARG(source);

GIT_REFCOUNT_INC(source);

@ethomson
Fixed

ethomson · 2021-06-14T23:04:59Z

+ * Create an in-memory copy of a submodule. The copy must be explicitly
+ * free'd or it will leak.
+ *
+ * @param out Pointer to store the copy of the submodule. Cannot be NULL.


Our docs generally tell you when a param is optional, instead of when a param is required.

Suggested change

* @param out Pointer to store the copy of the submodule. Cannot be NULL.

* @param out Pointer to store the copy of the submodule.

@ethomson
Fixed

ethomson · 2021-06-14T23:05:24Z

+	git_submodule *sm_duplicate;
+	const char *oid = "480095882d281ed676fe5b863569520e54a7d5c0";
+
+	// Check original


We don't use // comments in this project...

Suggested change

// Check original

/* Check original */

@ethomson
Fixed

ethomson · 2021-06-14T23:05:43Z

+	cl_assert(git_submodule_ignore(sm) == GIT_SUBMODULE_IGNORE_NONE);
+	cl_assert(git_submodule_update_strategy(sm) == GIT_SUBMODULE_UPDATE_CHECKOUT);
+
+	// Duplicate and free original


Suggested change

// Duplicate and free original

/* Duplicate and free original */

@ethomson
Fixed

ethomson · 2021-06-14T23:05:52Z

+	cl_assert(git_submodule_dup(&sm_duplicate, sm) == 0);
+	git_submodule_free(sm);
+
+	// Check duplicate


Suggested change

// Check duplicate

/* Check duplicate */

@ethomson
Fixed

…ndatory.

…nged.

lolgear · 2021-06-15T14:34:47Z

@ethomson Fixed.

ethomson · 2021-07-13T21:26:16Z

Thanks @lolgear!

ethomson · 2021-07-22T21:07:52Z

Oops, I didn't hit merge - my bad! Thanks again @lolgear!

submodule: git submodule dup has been added.

49f9941

lolgear changed the title ~~[Submodule] Git submodule dup.~~ [Submodule] Git submodule dup May 16, 2021

neithernut suggested changes May 19, 2021

View reviewed changes

submodule: git submodule dup guard statement has been fixed.

d6f57b9

lolgear requested a review from ethomson May 22, 2021 16:57

submodule: git submodule dup object dup has been added.

11aa20b

tiennou reviewed May 26, 2021

View reviewed changes

submodule: git submodule dup implementation has been reverted. Assert…

bf1616d

… has been added.

lolgear requested a review from tiennou May 26, 2021 12:34

tiennou reviewed May 26, 2021

View reviewed changes

lolgear requested a review from tiennou May 26, 2021 19:17

tiennou reviewed May 31, 2021

View reviewed changes

submodule: git submodule dup documentation has been fixed.

ca2ff5a

tests: submodule git lookup can be dupped test has been added.

11452ca

lolgear requested a review from tiennou June 7, 2021 10:48

ethomson requested changes Jun 14, 2021

View reviewed changes

lolgear added 2 commits June 15, 2021 17:33

submodule: git submodule dup out and source parameters have become ma…

428f1ac

…ndatory.

tests: submodule git lookup can be dupped test comments have been cha…

54a5245

…nged.

lolgear requested a review from ethomson June 15, 2021 14:34

ethomson merged commit 43b5075 into libgit2:main Jul 22, 2021

	* @param out Pointer to store the copy of the submodule
	* @param out Pointer to store the copy of the submodule. Cannot be NULL.

	* @param source Original tag to copy
	* @param source Original submodule to copy.

	// Duplicate and free original
	/* Duplicate and free original */

Uh oh!

Conversation

lolgear commented May 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neithernut left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lolgear May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lolgear May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lolgear commented May 31, 2021

Uh oh!

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

lolgear commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiennou commented Jun 2, 2021

Uh oh!

lolgear commented Jun 3, 2021

Uh oh!

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

lolgear commented Jun 15, 2021

Uh oh!

lolgear commented May 16, 2021 •

edited

Loading

neithernut left a comment •

edited

Loading

lolgear May 26, 2021 •

edited

Loading

lolgear May 26, 2021 •

edited

Loading

tiennou May 26, 2021 •

edited

Loading

lolgear commented Jun 1, 2021 •

edited

Loading