git-bundle-server CLI Part 2: collapse base bundle and the update-all subcommand
#3
Conversation
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
Again, with the caveat that I am not actively using Go, this looks good, in particular the places where a missing err handling was introduced.
One thing I always find particularly invaluable when developing software quickly is a set of automated tests. Do you think it would make sense to start this soon, or would it rather slow you down at this stage?
As I'm building the features, I'm benefitting from the fact that every method prototype can be changed quickly. I have some end-to-end scripts that allow me to manually check that things are working as I expect, but until I have a more complete view of things I'll need to hold off on a test suite. In the (internal) tracking issue I have an item for creating a test strategy after we reach a prototype that can be used for evaluation. |
derrickstolee
force-pushed
the
stolee/cli-1
branch
from
735aba1 to
1b1f540
Compare
derrickstolee
force-pushed
the
stolee/cli-2
branch
from
c8bfb14 to
054e34f
Compare
All of that said, I've been playing around with getting automated builds so at least we get compiler checks as PRs are going in. However, I continue to struggle with Go's build environment and package rules. Work is ongoing in the |
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
These changes look simple enough that even I understand them, in particular with the help of the excellent commit messages. Thank you!
A last-minute replacement of parsing to use scanner.Scan() changed the newline behavior of the line reading. This change allows us to actually parse bundle headers.
derrickstolee
force-pushed
the
stolee/cli-2
branch
2 times, most recently
from
952a771 to
60195e1
Compare
mjcheetham
left a comment
mjcheetham
left a comment
There was a problem hiding this comment.
Looks good! Added some non-blocking comments
| "os/exec" | ||
| ) | ||
|
|
||
| type UpdateAll struct{} |
There was a problem hiding this comment.
I know I mentioned this offline, but mentioning it here too..
Having update-all as a separate subcommand that calls out to update in a loop will mean paying some cost to fork+exec, and possibly and other process setup, config, tracing etc.
Would the update --all loop being in the same subcommand (or even update-all that leverages the same update functions in a loop) not be preferable here?
There was a problem hiding this comment.
I think for now, the fact that <route> is a positional argument in the CLI makes me not want to swap this. Keep in mind that we are also spawning git fetch subcommands at minimum and git bundle create if there is new data, so the process startup cost is very small compared to what commands we are running under the hood.
There was a problem hiding this comment.
I've attempted the rewrite of update-all as update --all in this branch, but I'm again having local build issues (:old-man-shakes-fist-at-go:).
There was a problem hiding this comment.
Of course the build issues come from the fact that I removed the go.work workspace file and all of the go.mod files throughout my workspace. So... they are somehow critical to how I was able to get things working.
This constant of "5" will one day be replaced with a configurable value whose default is probably "30". It has other issues when we add the concept of "daily" and "hourly" bundles. For now, 5 is sufficient. Convert the oldest bundles together into a single bundle, now called "base-<token>.bundle". We need to be careful when defining the refs to use for each of these tips, so create a new 'refs/base/' ref namespace. Some TODOs are marked to use 'git merge-base --independent' to reduce the number of tips used by this base bundle. We need to be careful, but it is possible that we could also "GC" the base bundle if the tip commits are not reachable from any tips in the later bundles. In such a case, we might need to rebuild all of those bundles to ensure that the REF_DELTA bases exist as needed (perhaps this is part of the prereq commits, so it might not be a real worry).
The routes file stores the list of known routes. These correspond to directories in the repo root and web root. While here, update our use of core.Repository to be pointer-based.
The 'update-all' subcommand runs the 'update' subcommand on all registered routes.
We will later add --daily and --hourly arguments to the 'update' subcommand, but for now we will blindly pass the arguments down.
derrickstolee
force-pushed
the
stolee/cli-2
branch
from
60195e1 to
3d0746a
Compare
Based on #2.
The
updatesubcommand now notices when there are more than 5 bundles and squashes the oldest ones into a new "base" bundle. The bundle is renamed but keeps the maximum creation token from that group of bundles.Modified some of the parameters, especially those in the
gitpackage to avoid cyclic package dependencies. Also used struct pointers more often.Repository routes are now stored in a new
routesfile (currently plaintext with line-separated list of routes).The new
update-allsubcommand runs theupdatesubcommand on all registered routes. It also passes any remaining arguments down to the subcommand, which will help when we add the--dailyand--hourlyoptions.