Merge/Rebase In Progress

[dahlbyk]

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?

[nulltoken]

Sounds neat!

How do you foresee the API?

[dahlbyk]

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

[nulltoken]

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?

[dahlbyk]

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.

[nulltoken]

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

[nulltoken]

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

[dahlbyk]

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.

[dahlbyk]

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.

[nulltoken]

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

[dahlbyk]

Eh?

[nulltoken]

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

[dahlbyk]

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

[nulltoken]

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

[nulltoken]

@dahlbyk How about nulltoken@526e589 ?

[dahlbyk]

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.

[nulltoken]

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);
    }
}

[dahlbyk]

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"].

[nulltoken]

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.

[nulltoken]

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

Done.

[nulltoken]

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

[carlosmn]

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.

[nulltoken]

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

[nulltoken]

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

[carlosmn]

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.

[nulltoken]

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

[carlosmn]

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;

[nulltoken]

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

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

[dahlbyk]

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.

[nulltoken]

@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 :-) )

[dahlbyk]

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

[nulltoken]

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

[nulltoken]

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

[dahlbyk]

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.

[nulltoken]

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?

[nulltoken]

@dahlbyk How about this?

[dahlbyk]

❤️! 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?

[nulltoken]

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