Skip to content

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

Merged
ethomson merged 1 commit intolibgit2:masterfrom
pks-t:pks/worktree-refs
Apr 17, 2018
Merged

worktree: add ability to create worktree with pre-existing branch#4524
ethomson merged 1 commit intolibgit2:masterfrom
pks-t:pks/worktree-refs

Conversation

@pks-t
Copy link
Copy Markdown
Member

@pks-t pks-t commented Feb 9, 2018

Currently, we always create a new branch after the new worktree's name
when creating a worktree. In some workflows, though, the caller may want
to check out an already existing reference instead of creating a new
one, which is impossible to do right now.

Add a new option ref to the options structure for adding worktrees. In
case it is set, a branch and not already checked out by another
worktree, we will re-use this reference instead of creating a new one.

Currently, we always create a new branch after the new worktree's name
when creating a worktree. In some workflows, though, the caller may want
to check out an already existing reference instead of creating a new
one, which is impossible to do right now.

Add a new option `ref` to the options structure for adding worktrees. In
case it is set, a branch and not already checked out by another
worktree, we will re-use this reference instead of creating a new one.
Comment thread src/worktree.c
goto out;
/* Set up worktree reference */
if (wtopts.ref) {
if (!git_reference_is_branch(wtopts.ref)) {
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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.

Comment thread include/git2/worktree.h
git_reference *ref; /**< reference to use for the new worktree HEAD */
} git_worktree_add_options;

#define GIT_WORKTREE_ADD_OPTIONS_VERSION 1
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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

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
Copy Markdown
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.

@mminns
Copy link
Copy Markdown

mminns commented Mar 6, 2018

Hi

Do we know if this is likely to be approved?

@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Mar 13, 2018

@mminns: As I'm the only one really caring about worktrees in libgit2 this will most certainly get approved. Only question is how the API will look like exactly. And this definitly won't make it in for v0.27.0.

@ethomson
Copy link
Copy Markdown
Member

However, since (I think) you're interested in LibGit2Sharp, we can pull this in for a prerelease there as soon as it's merged here.

@mminns
Copy link
Copy Markdown

mminns commented Mar 13, 2018

Yes, I'm primarily interested in libgit2sharp.

@ethomson ethomson merged commit 8529ac9 into libgit2:master Apr 17, 2018
@ethomson
Copy link
Copy Markdown
Member

Thanks, as always, @pks-t

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.

3 participants