Allow creating a remote without a repository - #3855

Maintainerfolk: is adding a new function like this a good idea -- Or it be better to re-do and change the behavior of create_internal() to work with a null repo? I'm afraid create_internal would become quite a twisty maze of conditionals, but having less API surface area also makes sense. Would appreciate your feedback :)

Hi @heavenlyhash ! Welcome and thanks for the nice first PR - this is delightful.

I agree that I would like to avoid less API surface and make repo optional to the existing create_anonymous. I suspect that you'll be able to be able to get surgical with create_internal, perhaps pulling out the repo-specific stuff into its own function? Let me know if it gets gnarly, but it looks like you've got a good handle on this. 😀

Would you mind hacking on a unit tests while you're in here?

Do we need to beef up any other parts of the code base that assume that a remote has a repo?
/cc @carlosmn

Right, the idea is to make passing in a repsitory optional, rather than adding new APIs. In the git_remote namespace there'll be quite a few things that try to access the repository. We definitely have to make sure that trying to run fetch on such a git_remote does not crash but returns an error.

I might need a little help getting the test coverage and additional checks in place. 0% skill navigating a large project in C (much less convincing myself I've done so exhaustively/correctly..) Will try to get a start though (especially since you have such lovely CI set up).

The first thing that jumps out at me as a liiittle odd is that the apply_insteadof call in create_internal returns new mem. Is it reasonable to have an if|else in create_internal that does strdup if there's no insteadof configuration to apply?

I might need a little help getting the test coverage and additional checks in place. 0% skill navigating a large project in C (much less convincing myself I've done so exhaustively/correctly..) Will try to get a start though (especially since you have such lovely CI set up).

Yeah, it's no joke. We've got a lot of unit tests, and our own test runner framework to boot. So there's a lot of overhead. If you want to get a start poking around with it, that would be great, and feel free to ask questions (here or in slack).

Is it reasonable to have an if|else in create_internal that does strdup if there's no insteadof configuration to apply?

Yeah, that's definitely what I would do, too. 😄

Okay, great, thanks! Really appreciate the quick guidance on memory stuff

Tests: looks like tests/{.,network,online}/{remotes,insteadof}.c is the target area? The main thing I'm thinking of writing would probably be an offline test that uses a local filesystem repo to as a remote and exercises git_remote_ls to make sure the data saved by create_internal is operable.

Error on fetch on a repoless remote: EINVALID seems like the closest match?

Combinations of valid parameters: a name param doesn't make much sense if there's no repo config to interact with, should providing name with a null repo be an error of some kind? Not sure on fetchspec -- git ls-remote [url] refs/unusual/* is a thing in exec'ing cgit, but that's not a fetchspec, so maybe fetchspecs don't make sense without a repo either?

Follow up on that last question about parameter validation: yeah, what's the convention to follow around combinatorially valid parameters and null checks? I fixed the repetitive code; all goes through create_internal now. But of the existing functions, git_remote_create and [...]_with_fetchspec are both chock full of parameters that don't make any sense if there's no repo (and the only thing those functions do is... check params); only git_remote_create_anonymous really makes any sense with a null repo.

• Option 1: allow null repos through any of the funcs; just make sure create_internal never crashes on any of them.
• Option 2: Move the assert against null repos that I removed from create_internal into the other exported functions clearly need a repo.

I tried Option 1 but ISTM it spirals towards messiness. For example, if create_internal started returning EINVALID to indicate bad usage like providing a fetchspec with a null repo, it would mean git_remote_create would still never DTRT with a null repo, because it hands down a default fetchspec! Fixing it creates epicycles; ignoring it means real incorrect usage from a client would be silent, which is not great.

Option 2 seems a little better, but means two of the functions panic on null repos, and the third one is fine. Maybe that's ok, but that seems like a api style and consistency thing I should check with you folks on?

... or I guess Option 3: make all three existing functions assert against null repos (as they do before this PR) and add a new function that has no repo param. (At least the implementation would be DRY now :3) Adds to API surface area; but seems most consistent.

Preferences?

Ping :)

I put this down for a while and my memory's already getting dusty, but I think last time I looked, my favor was still towards adding a new function, so that the others can continue to reject null values as a whole instead of needing a matrix of conditions to be documented. The current state of the branch is with such a new function, and the implementation cleaned up.

So ... is this being worked on or is it dead?

Closed via #4233

Thanks @heavenlyhash !