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

Already on GitHub? Sign in to your account

Merge/Rebase In Progress #128

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Member

dahlbyk commented Jun 9, 2012

Unless I'm missing something, there's no way to tell in libgit2(sharp) if the working directory has a change in progress (merge, rebase, bisect, etc). For posh-git I've ported most of https://github.com/msysgit/git/blob/devel/contrib/completion/git-completion.bash#L214-305 but it seems like other consumers might be interested in this information. Thoughts?

@dahlbyk dahlbyk referenced this pull request in dahlbyk/posh-git Apr 8, 2012

Closed

Use LibGit2Sharp #37

Member

nulltoken commented Apr 9, 2012

Sounds neat!

How do you foresee the API?

Member

dahlbyk commented Apr 10, 2012

Lacking a better name for now...

enum Operation {
    None,
    RebaseInteractive,
    RebaseMerge,
    Rebase,
    ApplyMailbox,  // AM
    RebaseInvalid, // AM/REBASE, no idea how this happens
    Merge,
    CherryPick,
    Bisect,
}

I can't think of a better place for it to live than RepositoryStatus, lazily loaded. Would also want a HeadCanonicalName to expose .git/rebase-merge/head-name or a better description of HEAD if it's not a sybolic ref.

Naming things is hard...

Member

nulltoken commented Apr 10, 2012

Lacking a better name for now [...] Naming things is hard

I do strongly strongly agree with this 😄

I can't think of a better place for it to live than RepositoryStatus, lazily loaded.

I'm a little bit concerned that retrieving the current ongoing Operation would require a full-blown git status which, on a huge repository, might take some time. There might be an option: adding this to Repository.Info. However, we might have to prevent it from being executed on bare repositories.

Would also want a HeadCanonicalName to expose .git/rebase-merge/head-name or a better description of HEAD if it's not a sybolic ref.

Would altering DetachedHead fit this need?

Member

dahlbyk commented Apr 10, 2012

Info feels odd to me too - values there don't change much, whereas Operation could. I agree that requiring a full status just to get the Operation is probably excessive, and it really doesn't fit that you'd have to go through Index to get there. I'll just tack it onto Repository for now.

Would also want a HeadCanonicalName to expose .git/rebase-merge/head-name or a better description of HEAD if it's not a sybolic ref.

Would altering DetachedHead fit this need?

I think it could.

Will see if I can put some of this into code and we can proceed from there.

Member

nulltoken commented Apr 10, 2012

Info feels odd to me too - values there don't change much, whereas Operation could

Agreed.

it really doesn't fit that you'd have to go through Index to get there.

Agreed as well. The Index is merely a pass-through from the standpoint of a rebase. RetrieveStatus barely belongs there.

I'm starting to wonder if we're not lacking a new namespace...

Member

nulltoken commented Apr 10, 2012

naming... how about a property named InteractiveState returning Rebasing, Bisecting ... ?

Member

dahlbyk commented Apr 11, 2012

In gerund form the compound names get weird... RebasingInteractively, RebasingMergedly? I think I prefer nouns with PendingOperation.

Re: lacking a new namespace... maybe, but I'm not sure what it is.

Member

dahlbyk commented Apr 21, 2012

Just getting started...any feedback so far? I started modifying DetachedHead but it felt awkward, so I pulled the interactive name alongside PendingOperation.

For testing I think I have to just write files directly into .git, which feels really brittle but I'm not sure there's a better option.

Member

nulltoken commented Apr 21, 2012

which feels really brittle but I'm not sure there's a better option.

I can't think of any other option either. Especially, as none of the operations above are implemented yet ;)

Member

dahlbyk commented Apr 29, 2012

Eh?

Member

nulltoken commented Jun 8, 2012

@dahlbyk Could you please rebase it on top of vNext and open a PR. It'll be easier to comment. Thanks!

Member

dahlbyk commented Jun 9, 2012

Cool, managed to attach my branch to the existing issue.

If anyone from GitHub is listening, it would be sweet if I didn't have to use the API for that. 😉

Member

nulltoken commented Jun 9, 2012

PR points to commit. Commit refers to PR by its number -> Indirect self referential achievement :)

Member

nulltoken commented Jun 9, 2012

Member

dahlbyk commented Jun 14, 2012

734adf7 could be cherry-picked into vNext by itself - lazily checking that reference is null does no good if canonicalNameSelector blows up.

👍 for folding Interactive.State into RepositoryInformation. I'm still not sold on repo.Head.Name not matching the real current HEAD, especially being in a different area of the API relative to PendingOperation. Just seems like it would be hard to communicate to consumers of this library exactly what's going on.

Member

nulltoken commented Jun 14, 2012

I'm still not sold on repo.Head.Name not matching the real current HEAD

I made a mistake. DetachedHead.HeadName is supposed to be private, not public. Could you please integrate this fix somewhere?

The way I see it, repo.Head.Name matches the real HEAD, but I might not understand your point. What makes you feel uncomfortable with the code below?

[Fact]
public void TadaDumTadadaDum()
{
    TemporaryCloneOfTestRepo path = BuildTemporaryCloneOfTestRepo();
    using (var repo = new Repository(path.RepositoryPath))
    {
        Assert.Equal("master", repo.Head.Name);
        Assert.False(repo.Info.IsHeadDetached);

        repo.Checkout("refs/tags/lw");

        Assert.Equal("(e90810b...)", repo.Head.Name);
        Assert.True(repo.Info.IsHeadDetached);
    }
}
Member

dahlbyk commented Jun 14, 2012

The problem is during a rebase:

C:\Dev\OSS\libgit2sharp [issue128|REBASE-i]> cat .git\HEAD
e701c252d215b65d42a2b28bedfe41462f040757
C:\Dev\OSS\libgit2sharp [issue128|REBASE-i]> cat .git\refs\heads\master
8220ec67f7c01bf6ef3dd3713b76ac3175e0bcbc

repo.Head.Name would return "issue128", but repo.Refs["issue128"] != repo.Refs["HEAD"].

Member

nulltoken commented Jun 16, 2012

Hmm... You're right.

Moreover, retrieving the canonical name of the Head would now return (e90810b...)... Which is messed up. :-/

Ok. Let's make a step back.

Could you please rollback the "Name" part of this PR and squash this spike into a commit with the PendingOperation part? Let's deliver this and take the time to think a bit about the badge/status.

Member

nulltoken commented Jun 16, 2012

734adf7 could be cherry-picked into vNext by itself - lazily checking that reference is null does no good if canonicalNameSelector blows up.

Done.

Member

nulltoken commented Aug 10, 2012

carlosmn/libgit2@e849aa8 looks like a nice native start to get PendingOperation exposed /cc @carlosmn

Owner

carlosmn commented Aug 10, 2012

I'll need to polish that up, as it was meant for something else which was solved in a different way in the end. Do notice that we can only ever know that we're in a cherry-pick if it the files failed to merge. The rest of the operations should be fairly straightforward.

As for naming, I don't feel like "pending" describes it, as it's not something we've yet to do, but something we're currently doing; after all, this status is to let the next git process know what we've started to do but had to stop so the user could fix the code.

Member

nulltoken commented Aug 10, 2012

we can only ever know that we're in a cherry-pick if it the files failed to merge.

@carlosmn Thanks for the feedback! As far as I know, @dahlbyk ported it from https://github.com/git/git/blob/master/contrib/completion/git-prompt.sh#L198-289

Member

nulltoken commented Aug 10, 2012

As for naming, I don't feel like "pending" describes it

@carlosmn Oh please, my dear NameWhisperer, help us with that :-) The idea behind the Pending thing was that something was being done, that the user and the repo were in the middle of something (rebasing, ...)

Owner

carlosmn commented Aug 11, 2012

In the C code it's called state, which IMO fits better with it being about the different parts of the repo being involved in this.

Member

nulltoken commented Aug 11, 2012

@carlosmn Why not. How would you name the 0 (zero) enum entry representing that nothing is "in progress"? Current proposal was PendingOperation.None, but I can't find a good name for State.

Owner

carlosmn commented Aug 16, 2012

You can call it None as well, as there is no particular state, or maybe Nominal. This is the current WIP enum:

typedef enum {
        GIT_REPOSITORY_STATE_NONE,
        GIT_REPOSITORY_STATE_MERGE,
        GIT_REPOSITORY_STATE_REVERT,
        GIT_REPOSITORY_STATE_CHERRY_PICK,
} git_repository_state_t;
Member

nulltoken commented Aug 16, 2012

Nominal fits me. Dammit! You're really good at naming things.

However, I'll stick with the gerund form for other entries (Merging, ...)

Member

dahlbyk commented Aug 17, 2012

Re: naming the states, I'll echo my comment from above:

In gerund form the compound names get weird... RebasingInteractively, RebasingMergedly?

I'm skeptical that "state" is discoverable enough, but I suppose that's what documentation is for.

Member

nulltoken commented Aug 17, 2012

@dahlbyk Hmmf.. You're right... However, I can't help but think that repo.Info.State.CherryPicking reads better that repo.Info.State.CherryPick.

OK Let's go with the non gerund form (once again :-) )

Member

dahlbyk commented Aug 18, 2012

Even if libgit2 calls it "state", I think a variation of operation reads better... Maybe repo.Info.CurrentOperation.CherryPick?

Member

nulltoken commented Oct 23, 2012

@ethomson added a nice state enum in libgit2/libgit2#1013

Member

nulltoken commented Nov 3, 2012

@dahlbyk libgit2/libgit2#1026 is merged! 🎉 How do you feel like about binding it as part of this PR?

Member

dahlbyk commented Nov 4, 2012

Spike has been refreshed but needs libgit2 update (still no VC++). Names have been adjusted a bit but I'm still not completely happy with them. Was torn about how much to duplicate the libgit2 state tests, so I went all out.

dahlbyk added a commit to dahlbyk/libgit2sharp that referenced this pull request Nov 4, 2012

Member

nulltoken commented Nov 6, 2012

I'm not sure about HeadName. Let's forget about it and the InteractiveState type for now.

Could you please move the PendingOperation property directly under the Repositoy.Info namespace?

Member

nulltoken commented Dec 5, 2012

@dahlbyk How about this?

Member

dahlbyk commented Dec 6, 2012

❤️! CurrentOperationFixture test names still reference InteractiveState (and suggest the tests would validate multiple values), otherwise I think we're good for the operation. I'd still like some way to retrieve the rebase HEAD if possible... maybe just Info.RebaseHead as implemented here?

Member

nulltoken commented Dec 6, 2012

@dahlbyk Thanks for the review! I tidied up the tests and merged it in vNext.

I'd still like some way to retrieve the rebase HEAD if possible... maybe just Info.RebaseHead as implemented here?

I'm not sure about this. Making it return the current Head name when not in detached mode feels a bit in contradiction with the name of the property. And I'm not sure making it return null would be really better.

I'd prefer to delay this addition until we have some vision of how would an interactive rebase should be dealt with from an api standpoint.

@nulltoken nulltoken closed this Dec 6, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment