Skip to content

repository: do not initialize HEAD if it's provided by templates#5176

Merged
ethomson merged 6 commits intolibgit2:masterfrom
pks-t:pks/repo-template-head
Jul 20, 2019
Merged

repository: do not initialize HEAD if it's provided by templates#5176
ethomson merged 6 commits intolibgit2:masterfrom
pks-t:pks/repo-template-head

Conversation

@pks-t
Copy link
Member

@pks-t pks-t commented Jul 19, 2019

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.


This PR also includes some refactorings to update code style in several places which I was touching.

Fixes #5148

pks-t added 3 commits July 19, 2019 11:02
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.
Copy link
Contributor

@tiennou tiennou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; only a couple of minor stylistic comments. Thanks for tackling this @pks-t!

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commitmsg: s/calles/calls/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pks-t added 3 commits July 19, 2019 13:02
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 pks-t force-pushed the pks/repo-template-head branch from ac2ea77 to 9d46f16 Compare July 19, 2019 11:06
@pks-t
Copy link
Member Author

pks-t commented Jul 19, 2019

Thanks for your review, I've amended fixes

@ethomson
Copy link
Member

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

git_repository_init_ext does not respect an existing head when not given a specific initial_head to create

3 participants