Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

worktree: add ability to create worktree with pre-existing branch #4524

Merged
merged 1 commit into from
Apr 17, 2018
Merged

worktree: add ability to create worktree with pre-existing branch #4524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion include/git2/worktree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,11 @@ typedef struct git_worktree_add_options {
unsigned int version;

int lock; /**< lock newly created worktree */
git_reference *ref; /**< reference to use for the new worktree HEAD */
} git_worktree_add_options;

#define GIT_WORKTREE_ADD_OPTIONS_VERSION 1
Copy link

Choose a reason for hiding this comment

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

When/how does the version get bumped?

Copy link
Member Author

Choose a reason for hiding this comment

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

The option versions shall get bumped whenever the structure changes, but this will only happen as soon as we have our first stable (1.0) release.

Copy link

Choose a reason for hiding this comment

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

I have a general question about changing this struct/the version and breaking changes, really for my own education.

In libgit2sharp the shape of the _options structs is effectively fixed, e.g. for git_blame_options
see https://github.com/libgit2/libgit2sharp/blob/3f9b415fa1edfc31ce1ec2b4b3d18441c34adfff/LibGit2Sharp/Core/GitBlame.cs#L44

[StructLayout(LayoutKind.Sequential)]
internal class git_blame_options
{
    public uint version = 1;
    public GitBlameOptionFlags flags;

    public UInt16 min_match_characters;
    public git_oid newest_commit;
    public git_oid oldest_commit;
    public UIntPtr min_line;
    public UIntPtr max_line;
}

vs in libgit2

libgit2/include/git2/blame.h

Line 70 in 89c332e

typedef struct git_blame_options { 

[StructLayout(LayoutKind.Sequential)]
internal class git_blame_options
{
    public uint version = 1;
    public GitBlameOptionFlags flags;

    public UInt16 min_match_characters;
    public git_oid newest_commit;
    public git_oid oldest_commit;
    public UIntPtr min_line;
    public UIntPtr max_line;
}

So the libgit2sharp code would need reworking if a new field was added to the struct in libgit2.

So my question is why is it ok to refactor the worktree options struct, without bumping the version, in libgit2 but not to add params to a method definition?

Why does adding a param to the method break the API but adding a field to the struct not break it?

Hope that makes sense. :)

Copy link
Member

Choose a reason for hiding this comment

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

You're very much right in that both are breaking API changes. The way that we extend things now is (generally) through the options interface instead of params. In the future it will always be through the options interface plus incrementing the version number.

We haven't been very responsible here about stabilizing. We should do that soon.

#define GIT_WORKTREE_ADD_OPTIONS_INIT {GIT_WORKTREE_ADD_OPTIONS_VERSION,0}
#define GIT_WORKTREE_ADD_OPTIONS_INIT {GIT_WORKTREE_ADD_OPTIONS_VERSION,0,NULL}

/**
* Initializes a `git_worktree_add_options` with default vaules.
Expand Down
31 changes: 24 additions & 7 deletions src/worktree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,13 +348,30 @@ int git_worktree_add(git_worktree **out, git_repository *repo,
|| (err = write_wtfile(gitdir.ptr, "gitdir", &buf)) < 0)
goto out;

/* Create new branch */
if ((err = git_repository_head(&head, repo)) < 0)
goto out;
if ((err = git_commit_lookup(&commit, repo, &head->target.oid)) < 0)
goto out;
if ((err = git_branch_create(&ref, repo, name, commit, false)) < 0)
goto out;
/* Set up worktree reference */
if (wtopts.ref) {
if (!git_reference_is_branch(wtopts.ref)) {
Copy link

Choose a reason for hiding this comment

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

My first comments on libgit2 code, apologies if I've misunderstood anything. :)

git worktree add allows using an existing branch name or commit, this code only supports branches? Should it add commit support now rather than adding that later?

Copy link
Member Author

Choose a reason for hiding this comment

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

Even if you misunderstand anything that is a worthwhile comment, as it hints at things that are hard to understand and could be improved. So don't worry about that ;)

That being said, I didn't really want to allow using committish things here. The main problem is actually the interface. In case we allow either a hash or a reference, I'd have to make sure what it is the user passed to me by parsing it. Same for branches vs tags -- in case the user passed a tag to me, I need to make sure to not let HEAD point at the tag, but instead create a detached HEAD.

So, if you ask me, it creates quite some complexity for rather minor usecases. At least I think these usecases are minor as long as I'm not convinced otherwise by a convincing scenario. Alternatively, I'd also be happy to implement that if somebody has an idea how the interface could look like such that it is trivial to implement and easy to use.

Copy link

Choose a reason for hiding this comment

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

To be honest I have never actually used a commit or tag. My personal usecase for worktrees is always checking out branches locally for PRs without disturbing my working copy. So this change helps me.

But I wonder how quickly requests would follow for tags/commits, perhaps it is enough to get there in incremental steps.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I definitly see where you're coming from. I mean I would already include the ability to use arbitrary commits, but I'm simply undecided on how that interface should look like.

So a long as nobody has any proposals I think I'll just stay with the current one for now and wait until people start complaining.

giterr_set(GITERR_WORKTREE, "reference is not a branch");
err = -1;
goto out;
}

if (git_branch_is_checked_out(wtopts.ref)) {
giterr_set(GITERR_WORKTREE, "reference is already checked out");
err = -1;
goto out;
}

if ((err = git_reference_dup(&ref, wtopts.ref)) < 0)
goto out;
} else {
if ((err = git_repository_head(&head, repo)) < 0)
goto out;
if ((err = git_commit_lookup(&commit, repo, &head->target.oid)) < 0)
goto out;
if ((err = git_branch_create(&ref, repo, name, commit, false)) < 0)
goto out;
}

/* Set worktree's HEAD */
if ((err = git_repository_create_head(gitdir.ptr, git_reference_name(ref))) < 0)
Expand Down
30 changes: 30 additions & 0 deletions tests/worktree/worktree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,36 @@ void test_worktree_worktree__init_existing_branch(void)
git_reference_free(branch);
}

void test_worktree_worktree__add_with_explicit_branch(void)
{
git_reference *head, *branch, *wthead;
git_commit *commit;
git_worktree *wt;
git_repository *wtrepo;
git_buf path = GIT_BUF_INIT;
git_worktree_add_options opts = GIT_WORKTREE_ADD_OPTIONS_INIT;

cl_git_pass(git_repository_head(&head, fixture.repo));
cl_git_pass(git_commit_lookup(&commit, fixture.repo, &head->target.oid));
cl_git_pass(git_branch_create(&branch, fixture.repo, "worktree-with-ref", commit, false));

opts.ref = branch;

cl_git_pass(git_buf_joinpath(&path, fixture.repo->workdir, "../worktree-with-different-name"));
cl_git_pass(git_worktree_add(&wt, fixture.repo, "worktree-with-different-name", path.ptr, &opts));
cl_git_pass(git_repository_open_from_worktree(&wtrepo, wt));
cl_git_pass(git_repository_head(&wthead, wtrepo));
cl_assert_equal_s(git_reference_name(wthead), "refs/heads/worktree-with-ref");

git_buf_free(&path);
git_commit_free(commit);
git_reference_free(head);
git_reference_free(branch);
git_reference_free(wthead);
git_repository_free(wtrepo);
}


void test_worktree_worktree__init_existing_worktree(void)
{
git_worktree *wt;
Expand Down