WIP/RFC remote-vfs transport. #190
Conversation
There was a problem hiding this comment.
I left a couple of comments, and have to admit that I was quite a bit distracted by those ///// lines, unnecessary if (ret == HTTP_OK) ... blocks and it seems that at least parts of the code are not necessarily DRY (I also have to admit that reviewer fatigue kicked in around the do_post() function...).
| return HTTP_OK; | ||
|
|
||
| if (ret != HTTP_REAUTH) | ||
| return ret; |
There was a problem hiding this comment.
Is if (ret == HTTP_OK) return HTTP_OK; really necessary? That case is already handled by the ret != HTTP_REAUTH clause, so it just adds distracting code...
There was a problem hiding this comment.
this aids in debugging, since we can set breakpoints on either the success or failure cases independently.
| &http_response_code, error_message); | ||
| trace2_region_leave("remote-vfs", "GET/object/main2", NULL); | ||
| if (ret == HTTP_OK) | ||
| return HTTP_OK; |
There was a problem hiding this comment.
Again, deleting these two lines will make the code easier to read, without actually changing the behavior.
There was a problem hiding this comment.
again, this is for debugging and breakpoints. the compiler should be able to flatten it out.
There was a problem hiding this comment.
Sure. The compiler can optimize this. I, however, cannot optimize this, when reviewing, and my eyes and my mind tires from such repetition...
| return ret; | ||
|
|
||
| if (!rv_opts.param__allow_fallback) | ||
| return ret; |
There was a problem hiding this comment.
How about combining conditions into a single if, when the conditional action is the same anyway?
There was a problem hiding this comment.
combining a bunch of unrelated logic just to DRY a "return" statement IMHO obscures the meaning. and again, makes it more difficult to set specific breakpoints.
There was a problem hiding this comment.
Just a few small comments to get started while I had some downtime. I need to get this running on my machine for local testing, but then the end-to-end tests will be the real key to having confidence in the change. (the functional test suite will give it a real exercise.)
| // none := assume neither mode. | ||
| // | ||
| // The <mode> is used to look for mode-specific config settings: | ||
| // "<mode>.cache-server" and "core.<mode>". |
There was a problem hiding this comment.
Note: at the moment, we still use core.gvfs. This may change in the future, so having the comment here is helpful.
|
|
||
| odb = the_repository->objects->odb; | ||
| if (!odb) | ||
| return; |
There was a problem hiding this comment.
Should this be an error state? Or should we assign rv_opts.param__*_odb_path = the_repository->objects->path?
There was a problem hiding this comment.
Funny story. Turns out that prepare_alt_obj() will segfault before we get here if there isn't an ODB. The fix is to require the process to be inside a worktree.
|
abandoning this PR in favor of a new one after a major refactoring. |
Basic functionality. See remote-vfs.c for overview.