repository: do not initialize HEAD if it's provided by templates #5176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There's quite a lot of supporting code for our templates and they are an obvious standalone feature. Thus, let's extract those tests into their own suite to also make refactoring of them easier.
The repo::template test suite makes use of quite a few local variables that could be consolidated. Do so to make the code easier to read.
All tests in repo::template have a common pattern of first setting up templates, then settung up the repository that makes use of those templates via several init options. Refactor this pattern into two functions `setup_templates` and `setup_repo` that handle most of that logic to make it easier to spot what a test actually wants to check. Furthermore, this also refactors how we clean up after the tests. Previously, it was a combination of manually calling `cl_fixture_cleanup` and `cl_set_cleanup`, which really is kind of hard to read. This commit refactors this to instead provide the cleaning parameters in the setup functions. All cleanups are then performed in the suite's cleanup function.
tiennou
approved these changes
Jul 19, 2019
src/repository.c
Outdated
| int error; | ||
| git_buf repo_path = GIT_BUF_INIT, wd_path = GIT_BUF_INIT, | ||
| common_path = GIT_BUF_INIT; | ||
| git_buf repo_path = GIT_BUF_INIT, wd_path = GIT_BUF_INIT, common_path = GIT_BUF_INIT; |
There was a problem hiding this comment.
commitmsg: s/calles/calls/
src/repository.c
Outdated
| (error = repo_init_config(repo_path.ptr, wd, opts->flags, opts->mode)) < 0) | ||
| goto out; | ||
|
|
||
| if ((error = git_buf_joinpath(&head_path, repo_path.ptr, GIT_HEAD_FILE)) < 0) |
There was a problem hiding this comment.
Style: I'd merge that if with the one above.
(I'm also still worried that every time a GIT_HEAD_FILE occurrence is added, it usually means custom refdb won't know about it).
There was a problem hiding this comment.
Style: I'd merge that if with the one above.
Fixed
(I'm also still worried that every time a GIT_HEAD_FILE occurrence is added, it usually means custom refdb won't know about it).
Yup, I thought about that, too. In this case it's fine though, as we're creating a completely new repository with git_repository_init_ext, which doesn't support custom backends anyway. So we need to have GIT_HEAD_FILE created here. But, I think in the long term we should fix git_repository_create_head. This one is a generic version and should thus use the refdb.
The error handling in `git_repository_create_head` completely swallows all error codes. While probably not too much of a problem, this also violates our usual coding style. Refactor the code to use a local `error` variable with the typical `goto out` statements.
Update `git_repository_init_ext` to use our typical style of error handling. The function had multiple statements which didn't `goto out` immediately but instead deferred it to later calls combined with `if` statements.
When using templates to initialize a git repository, then git-init(1) will copy over all contents of the template directory. These will be preferred over the default ones created by git-init(1). While we mostly do the same, there is the exception of "HEAD". While we do copy over the template's HEAD file, afterwards we'll immediately re-initialize its contents with either the default "ref: refs/origin/master" or the init option's `initial_head` field. Let's fix the inconsistency with upstream git-init(1) by not overwriting the template HEAD, but only if the user hasn't set `opts.initial_head`. If the `initial_head` field has been supplied, we should use that indifferent from whether the template contained a HEAD file or not. Add tests to verify we correctly use the template directory's HEAD file and that `initial_head` overrides the template.
pks-t
force-pushed
the
pks/repo-template-head
branch
from
ac2ea77
to
9d46f16
Compare
|
Thanks for your review, I've amended fixes |
|
Thanks for tackling this, @pks-t. Really appreciate all the cleanups with the fix. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using templates to initialize a git repository, then git-init(1)
will copy over all contents of the template directory. These will be
preferred over the default ones created by git-init(1). While we mostly
do the same, there is the exception of "HEAD". While we do copy over the
template's HEAD file, afterwards we'll immediately re-initialize its
contents with either the default "ref: refs/origin/master" or the init
option's
initial_headfield.Let's fix the inconsistency with upstream git-init(1) by not overwriting
the template HEAD, but only if the user hasn't set
opts.initial_head.If the
initial_headfield has been supplied, we should use thatindifferent from whether the template contained a HEAD file or not. Add
tests to verify we correctly use the template directory's HEAD file and
that
initial_headoverrides the template.This PR also includes some refactorings to update code style in several places which I was touching.
Fixes #5148