Remote creation API #4667

tiennou · 2018-06-01T13:47:22Z

This is the PR for the code I've written for #4658.

It adds a git_remote_create_ext with an option structure for handling the 3 other _create calls (ie. _with_fetchspec, _anonymous, _detached).

I've not pondered too much about the difference between anonymous & detached remotes. AFAICT, both are in-memory, and anonymous ones can make use of an underlying repository, so config will be respected.

vors · 2018-06-01T15:03:47Z

😍 thank you for taking a stub at it!

pks-t

I think this PR is great. It fixes our remote creation API and consolidates all available functions into one extensible one. Thanks for working on it!

pks-t · 2018-06-07T10:32:35Z

src/remote.c

+
+	giterr_set(
+			   GITERR_CONFIG,
+			   "remote '%s' already exists", name);


Indentation here is a bit funny.

pks-t · 2018-06-07T10:33:20Z

include/git2/remote.h

+#define GIT_REMOTE_CREATE_OPTIONS_VERSION 1
+#define GIT_REMOTE_CREATE_OPTIONS_INIT {GIT_REMOTE_CREATE_OPTIONS_VERSION}
+
+GIT_EXTERN(int) git_remote_create_init_options(


This function is missing its documentation.

pks-t · 2018-06-07T10:35:03Z

include/git2/remote.h

+typedef struct git_remote_create_options {
+	unsigned int version;
+
+	/** The repository that should own the remote.


Our struct member doc comment style is to either have a single /** foobar */ line or start with an empty /** line.

pks-t · 2018-06-07T10:37:01Z

include/git2/remote.h

+ * @param opts the remote creation options
+ * @return 0 on success, WIP
+ */
+GIT_EXTERN(int) git_remote_create_ext(


Bikeshedding: create_ext doesn't make it clear what the "extended" nature of this function is. Instead, I'd vote for git_remote_create_with_opts, which clearly states that it takes an opts structure.

pks-t · 2018-06-07T10:38:29Z

include/git2/remote.h

+ *
+ * @param out the resulting remote
+ * @param url the remote's url
+ * @param opts the remote creation options


You should mention that opts is optional and what will happen in that case. (It is optional, isn't it? If not, I'd propose to just create an anonymous remote in that case)

pks-t · 2018-06-07T10:42:31Z

src/remote.c

-		if (repo && (error = lookup_remote_prune_config(remote, config_ro, name)) < 0)
+		if (!is_anonymous &&
+			((error = write_add_refspec(opts->repository, opts->name, opts->fetchspec, true)) < 0 ||
+			(error = lookup_remote_prune_config(remote, config_ro, opts->name)) < 0))


Wait, this is different from what we did before. Now we only call lookup_remote_prune_config if both opts->repository and opts->name are set. Previously, we were also calling it in case opts->name is unset. Is this intentional and, if so, why can we avoid calling lookup_remote_prune_config now?

Yeah, the tests around detached/anonymous/realized remotes are confusing here. My is_anonymous variable should be named is_not_realized maybe (bikeshedding welcome on the name).

The original code would attempt to load a remote..prune config option, which IMHO doesn't make sense (and I remember a test broke before I changed it to this).

Confirmed. Should I keep the test as is ?

1) Failure: network::remote::remotes::cannot_add_a_nameless_remote [/Users/tiennou/Projects/libgit2/tests/network/remote/remotes.c:321] GIT_EINVALIDSPEC != git_remote_create(&remote, _repo, NULL, "git://github.com/libgit2/libgit2") -12 != 0

You're right, the original code doesn't make a lot of sense. So this change looks correct to me

Note that most of what I said above about the broken test is wrong. The test failure was caused when I removed the name & repo existence checks from git_remote_create (since it allows NULL).

AFAICT, the original code is extra-confusing about what actually needs a repo/name because it indirectly uses the config, so I hope this is better. I'm still dubious of the whole config_ro/config_rw dance that's happening, because we start by doing a snapshot, then we re-grab a "live" config. I might be just paranoid/unaware of the semantics of config__weakptr, but I'm tempted to change it to : grab a "live" version IF we have a repo, snapshot it so it doesn't change when applying url.insteadof, and set the various other settings on the first live one.

pks-t · 2018-06-07T10:43:35Z

src/remote.c

@@ -271,7 +292,7 @@ static int create_internal(git_remote **out, git_repository *repo, const char *n
 	}

 	/* A remote without a name doesn't download tags */
-	if (!name)
+	if (is_anonymous)


This is also different from what we did before, as now an unnamed remote with an associated repository will not fetch tags anymore. Is this intended?

You're right, I went too far trying to make sense of the 3 types of remotes. Reverting.

pks-t · 2018-06-07T10:44:03Z

src/remote.c

-	return 0;
+	opts.repository = repo;
+	opts.name = name;
+	opts.fetchspec = fetch;


This conversion is great

pks-t · 2018-06-07T10:44:36Z

src/remote.c

 	int error;
+	git_remote_create_options opts = GIT_REMOTE_CREATE_OPTIONS_INIT;

 	if ((error = ensure_remote_name_is_valid(name)) < 0)
 		return error;

 	if ((error = ensure_remote_doesnot_exist(repo, name)) < 0)
 		return error;


These two calls are already done in create_remote_internal now. I'd like to avoid this duplication

pks-t · 2018-06-07T10:46:12Z

include/git2/remote.h

@@ -63,6 +73,9 @@ typedef struct git_remote_create_options {

 	/** The fetchspec the remote should use. */
 	const char *fetchspec;
+
+	/** Additional flags for the remote. See git_remote_create_flags. */
+	int flags;


This should be unsigned, shouldn't it?

tiennou · 2018-06-11T19:14:25Z

src/remote.c

 		return error;

 	remote = git__calloc(1, sizeof(git_remote));
 	GITERR_CHECK_ALLOC(remote);

-	remote->repo = repo;
+	remote->repo = opts->repository;

 	if ((error = git_vector_init(&remote->refs, 32, NULL)) < 0 ||


Just a heads up, I was pretty surprised about that 32 here. Should I lower that to something like 4 ?

I think 4 is really rather low, and you said that 8 is the minimum size for vectors anyway. So we could lower it to 8, but I'm rather indifferent.

tiennou · 2018-06-11T19:21:16Z

Rebased, with all review comments taken care of apart from the change of behavior here.

pks-t · 2018-06-15T10:24:14Z

tests/network/remote/remotes.c

@@ -318,7 +318,7 @@ void test_network_remote_remotes__cannot_add_a_nameless_remote(void)

 	cl_assert_equal_i(
 		GIT_EINVALIDSPEC,
-		git_remote_create(&remote, _repo, NULL, "git://github.com/libgit2/libgit2"));
+		git_remote_create(&remote, _repo, "", "git://github.com/libgit2/libgit2"));


This is the test you've been talking about, right? I'd say we should just keep this test as it was originally, just to be sure we're not altering behaviour.

Yeah, I'm not fan of changing that test, but keeping it that way makes the implementation waaay harder to follow, as I now have to accept a NULL argument here instead of assert()-ing, which means I need the generic call to differentiate a git_remote_create call with an unexpected NULL or a git_remote_create_detached/anonymous().

Your call, I just chose the easy way first.

pks-t · 2018-06-15T10:24:51Z

include/git2/remote.h

+ * Remote creation options flags
+ */
+typedef enum {
+	GIT_REMOTE_CREATE_DEFAULT = 0,


Why is this flag necessary? We don't have default flags for all the other option enums, do we?

tiennou · 2018-06-17T17:18:43Z

So, major rework of this. I've taken the time to strengthen the test suite because I kept discovering "edge-cases" (like with_fetchspec not building a default fetchspec), so all 3 basic calls are now more cautiously tested (for invalid parameters, config changes, and whatever differing behavior I stumbled upon).

New from last time :

I've added a flag to prevent building a default fetchspec. I've also re-duplicated code either for backward-compatibility reasons (git_remote_create actually shadows its error codes) or simplicity in create_internal.
I've switched the config-management code to use transactions (so the setup/teardown cycle is simpler).

Left to do/open questions :

check that we set error messages correctly. I noticed a few <no message> failures.
maybe have the const char *fetchspec option be a size_t count, const char fetchspecs[] ? I didn't notice at first, but using git_remote_add_fetch will need a relookup so the refspecs are "in the correct place" (see dwim_refspecs at the end of create_internal).

pks-t

Just skipped quickly through the reworked commits, as they still seem to be WIP (at least CI is segfaulting on all platforms).

pks-t · 2018-06-22T10:45:50Z

tests/remote/create.c

+
+void test_remote_create__named_fail_on_invalid_name(void)
+{
+	cl_git_assert_cannot_create_remote(GIT_EINVALIDSPEC, git_remote_create(&r, _repo, NULL, TEST_URL));


I'd actually prefer the same as in the other PR. Instead of using a macro and repeating that multiple times, you should just use an array of strings here and iterate through them. That'd be a lot simpler to read and produce less code.

pks-t · 2018-06-22T10:53:11Z

src/remote.c

@@ -232,7 +232,7 @@ static int create_internal(git_remote **out, git_repository *repo, const char *n

 	remote->repo = repo;

-	if ((error = git_vector_init(&remote->refs, 32, NULL)) < 0 ||
+	if ((error = git_vector_init(&remote->refs, 8, NULL)) < 0 ||


I'd really like to see some reasoning in the commit message as to why this is a good thing.

Memory bloat ? That's enough memory for 32 different refspecs. I expect this is far larger than any "normal" usage of a remote. 8 is selected because it's the minimum allocation size of git_vector.

The minimum allocation size is the interesting information I've been looking for :) I wasn't really wondering why the count was removed, but rather why 8 was picked. Thanks for clarifying

pks-t · 2018-06-22T10:54:43Z

src/remote.c

 	*out = remote;
 	error = 0;

 on_error:
 	if (error)
 		git_remote_free(remote);

-	git_config_free(config_ro);
+	git_config_free(config);


Don't we have to unlock the config on error, too?

Will fix, I was misusing the transaction config API (hence the CI failure). The transaction handles unlocking when it's freed.

pks-t

And another feedback round. Comments are getting less and more focussed on style, so this might be the last round of comments from me. Thanks for your work!

pks-t · 2018-07-06T08:54:44Z

src/remote.c

+	if (repo) {
+		if ((error = git_repository_config__weakptr(&config, repo)) < 0 ||
+			(error = git_config_lock(&txn, config)) < 0)
+		goto on_error;


This indentation is really confusing. The goto on_error doesn't belong to the repo conditional, but to the other one. When fixing you should also indent the git_config_lock by four spaces instead of a tab :)

pks-t · 2018-07-06T08:59:18Z

src/remote.c

 		return error;

 	remote = git__calloc(1, sizeof(git_remote));
 	GITERR_CHECK_ALLOC(remote);

-	remote->repo = repo;
+	remote->repo = opts->repository;

 	if ((error = git_vector_init(&remote->refs, 32, NULL)) < 0 ||


I think 4 is really rather low, and you said that 8 is the minimum size for vectors anyway. So we could lower it to 8, but I'm rather indifferent.

pks-t · 2018-07-06T09:01:13Z

src/remote.c

+		return error;
+
+	if (canonicalize_url(&buf, url) < 0)
+		return GIT_ERROR;


What kind of backwards compatibility do you refer to? Do callers expect a specific error code here?

I found out while writing this test that this was "misbehaving", in that it never returns what create_internal says when it fails. It depends on what kind of backward compatibility we want (I realize I may be too zealous, but maybe this should be discussed separately)…

The ensure_remote_name_is_valid is duplicated in case name is NULL (a NULL name in the with_opts version would cause an anonymous remote to be created instead, ie. it's not a failure anymore). An alternative would be to assert on the mandatory parameters to be correct…

Yeah. I guess we should fix this, but that can be done in a later PR if you wish.

pks-t · 2018-07-06T09:03:20Z

src/remote.c

@@ -263,7 +263,7 @@ static int create_internal(git_remote **out, const char *url, const git_remote_c
 		(error = canonicalize_url(&canonical_url, url)) < 0)
 		goto on_error;

-	if (config) {
+	if (config && (opts->flags & GIT_REMOTE_CREATE_SKIP_INSTEADOF) != GIT_REMOTE_CREATE_SKIP_INSTEADOF) {


if (config && !(opts->flags & GIT_REMOTE_CREATE_SKIP_INSTEADOF))

pks-t · 2018-07-06T09:04:07Z

src/remote.c

-	if (opts->fetchspec != NULL) {
-		if ((error = add_refspec(remote, opts->fetchspec, true)) < 0)
+	if (opts->fetchspec != NULL ||
+	    (opts->name && (opts->flags & GIT_REMOTE_CREATE_SKIP_DEFAULT_FETCHSPEC) != GIT_REMOTE_CREATE_SKIP_DEFAULT_FETCHSPEC)) {


(opts->name && !(opts->flags & GIT_REMOTE_CREATE_SKIP_DEFAULT_FETCHSPEC))

tiennou · 2018-07-06T20:01:16Z

Should be good now. I've lowered the vector size again, because (if my sizeof math is correct), we have 32 * sizeof(void *) => 256bytes on 64bits, which makes 1Mb lost to mostly empty vectors for every 4 remote you create.

Thanks @pks-t for the reviews and ever helpful comments.

pks-t · 2018-07-13T08:11:36Z

src/remote.c

-	git_buf_dispose(&var);
-
+	if (txn)
+		git_transaction_commit(txn);


Sorry to not have catched this earlier, but you need to do error checking here. Otherwise we might end up silently not writing the config changes.

pks-t · 2018-07-13T08:15:18Z

src/remote.c

+		return error;
+
+	if (canonicalize_url(&buf, url) < 0)
+		return GIT_ERROR;


Yeah. I guess we should fix this, but that can be done in a later PR if you wish.

tiennou · 2018-07-28T09:58:57Z

Just a thought : per https://stackoverflow.com/a/51570151/444323, if I kept the new function's prototype compatible with clone's git_remote_create_cb, that would make a nice one-shot customization function, since git_remote_create_opts could be passed as payload.

EDIT To elaborate on that, that would make the function's prototype & options look like this :

typedef enum {
    GIT_REMOTE_CREATE_ANONYMOUS,
    GIT_REMOTE_CREATE_DETACHED,
    GIT_REMOTE_CREATE_NO_DEFAULT_FETCHSPEC,
    GIT_REMOTE_CREATE_SKIP_INSTEAD_OF,
} git_remote_create_flags;

typedef struct {
    int version;
    const char *fetchspec;
    int flags;
} git_remote_create_opts;

/**
 * repo mandatory unless CREATE_DETACHED
 * name mandatory unless CREATE_ANONYMOUS
 * no opts means mandatory
 */
int git_remote_create_with_opts(git_remote **remote,
    git_repository *repo,
    const char *name,
    const char *url,
    const git_remote_create_opts *opts);

tiennou · 2018-09-11T14:04:28Z

@pks-t If you don't mind (and your workqueue is not saturated), I'd like to have this one reviewed 😉.

pks-t

So I've finally found some time to do another review. Everything looks alright to me, except for one potential memory leak. Please take the other three comments as nice to have -- feel free to fix them if you want, but they won't keep this from getting merged.

include/git2/remote.h

pks-t · 2018-10-04T09:55:48Z

src/remote.c

+				goto on_error;
+
+			fetch = git_buf_cstr(&specbuf);
+		}


Doesn't this leak memory? In case where we create the fetchspec ourselves, we need to allocate memory, in the other case we just us the pointer from opts->fetchspec witouth allocating memory. So in the former case we'd have to also free it, right?

I don't expect it does : the on_error "handler" unconditionally frees the buffer, so it doesn't matter if it's a self-built fetchspec or not. Arguably, I could rename that to cleanup…

Oh, yeah. That was my mistake, I somehow expected a git_buf_detach here. git_buf_cstr doesn't transfer memory ownership, so this is fine

src/remote.c

tests/remote/create.c

tiennou · 2018-10-08T21:47:45Z

/rebuild

libgit2-azure-pipelines · 2018-10-08T21:47:51Z

Okay, @tiennou, I started to rebuild this pull request.

tiennou · 2018-10-08T22:12:53Z

Okay, something reaally weird is going on after rebasing to master… Just to be sure, I'll submit the rebased version as another "dummy" PR, and I'm restoring this one to the pre-rebase version (though the review comments have been taken care of) because I'd like to make sure it's still green (IIRC it was, but 🤷‍♂️ ). Please don't merge this while investigations are in progress…

src/remote.c

We need a repo/config and a name to be able to do anything to the configuration. As such, those two tests can be merged so their conditions are shared.

This supersedes the functionality of remote_create_with_fetchspec, remote_create_anonymous and remote_create_detached.

As it is, this is space for 32 refs pointers, which feels a little much. Lower it to 8, as it is the minimum vector size anyway.

pks-t · 2018-11-13T12:40:49Z

Thanks a lot @tiennou for this awesome work!

vors mentioned this pull request Jun 1, 2018

git_remote_create without insteadof rewrite rules #4658

Closed

pks-t requested changes Jun 7, 2018

View reviewed changes

tiennou commented Jun 11, 2018

View reviewed changes

tiennou force-pushed the feature/remote-create-api branch from 9206aae to 64a153c Compare June 11, 2018 19:17

tiennou force-pushed the feature/remote-create-api branch from 64a153c to 9cdd275 Compare June 12, 2018 17:46

pks-t reviewed Jun 15, 2018

View reviewed changes

tiennou force-pushed the feature/remote-create-api branch from 9cdd275 to 5ee8b5d Compare June 17, 2018 17:04

tiennou force-pushed the feature/remote-create-api branch 2 times, most recently from 21238e7 to c784d47 Compare June 20, 2018 00:27

pks-t reviewed Jun 22, 2018

View reviewed changes

tiennou force-pushed the feature/remote-create-api branch 3 times, most recently from 0484a27 to 66bcc02 Compare June 24, 2018 16:39

pks-t requested changes Jul 6, 2018

View reviewed changes

tiennou force-pushed the feature/remote-create-api branch from 66bcc02 to eb72511 Compare July 6, 2018 19:40

pks-t requested changes Jul 13, 2018

View reviewed changes

tiennou force-pushed the feature/remote-create-api branch from 70f392d to a0f91ab Compare July 28, 2018 10:08

pks-t requested changes Oct 4, 2018

View reviewed changes

tiennou force-pushed the feature/remote-create-api branch 2 times, most recently from 31fcfc8 to b61a7bf Compare October 8, 2018 19:15

tiennou force-pushed the feature/remote-create-api branch from b61a7bf to 1d990c5 Compare October 8, 2018 22:09

pks-t requested changes Nov 2, 2018

View reviewed changes

src/remote.c Outdated Show resolved Hide resolved

tiennou added 18 commits November 2, 2018 14:58

tests: rename remote creation test suite

798be87

tests: consolidate all remote creation tests in one test suite

10fa2dd

tests: git_remote_create_with_fetchspec

fa69195

tests: git_remote_create_anonymous

f778af6

tests: check what happens with the remote. section counts

4e0da45

tests: git_remote_create_detached

f8fc987

tests: count config section helper already exists

0e5a27c

remote: move static method

de2af3c

config: add asserts for non-null parameters in lock/unlock

b2640c3

remote: add a helper for generating the default fetchspec

b741bb8

remote: merge if-statements

43b4b2f

We need a repo/config and a name to be able to do anything to the configuration. As such, those two tests can be merged so their conditions are shared.

remote: provide a generic API for creating remotes

3cbaebd

This supersedes the functionality of remote_create_with_fetchspec, remote_create_anonymous and remote_create_detached.

remote: add a creation flag for ignoring url.insteadOf

fdb116b

remote: add a flag to prevent generation of the default fetchspec

d365029

tests: remote/create: remove macro and unroll tests

3652b83

remote: lower the default vector size to 8

10cba76

As it is, this is space for 32 refs pointers, which feels a little much. Lower it to 8, as it is the minimum vector size anyway.

remote: remove static create_internal function

37acffa

tests: unwarranted NULL-ification

666c7bd

tiennou force-pushed the feature/remote-create-api branch from 68f0ce5 to 666c7bd Compare November 2, 2018 13:59

pks-t approved these changes Nov 13, 2018

View reviewed changes

pks-t merged commit 20cb30b into libgit2:master Nov 13, 2018

tiennou deleted the feature/remote-create-api branch November 13, 2018 15:28

tiennou mentioned this pull request Jan 14, 2019

Add sign capability to git_rebase_commit #4913

Merged

snyk-bot mentioned this pull request Feb 23, 2020

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

Open

jasonkarns mentioned this pull request Mar 18, 2020

support defaulting to SSH remotes cli/cli#546

Closed

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

vors mentioned this pull request Apr 23, 2021

Crates.io index update failing due to conflict with ~/.gitconfig rust-lang/cargo#8172

Open

Remote creation API #4667

Remote creation API #4667

Conversation

tiennou commented Jun 1, 2018

vors commented Jun 1, 2018

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiennou commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiennou commented Jun 17, 2018

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiennou commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiennou commented Jul 28, 2018 • edited Loading

tiennou commented Sep 11, 2018

pks-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiennou commented Oct 8, 2018

libgit2-azure-pipelines bot commented Oct 8, 2018

tiennou commented Oct 8, 2018

pks-t commented Nov 13, 2018

tiennou commented Jul 28, 2018 •

edited

Loading