Request for comments: Some Head properties throw Exceptions on a freshly initialized repository #216

Closed
nulltoken opened this Issue Sep 19, 2012 · 16 comments

Comments

Projects
None yet
5 participants
@nulltoken
Member

nulltoken commented Sep 19, 2012

@aroben and @Haacked raise an interesting regression:

"In the latest libgit2, with a brand new repo, repo.Head property returns a Branch. But many of its properties now throw an exception."

The source of this mess should be 38fdee6.

Below a pseudo test allow to quickly "play" with this...

[Fact]
public void CanCreateBareRepo()
{
    SelfCleaningDirectory scd = BuildSelfCleaningDirectory();
    using (var repo = Repository.Init(scd.DirectoryPath, true))
    {
        var head = repo.Head;

        /* Those do not throw */
        var c = head.CanonicalName;
        var d = head.Commits.Count();
        var e = head.IsCurrentRepositoryHead;
        var f = head.IsRemote;
        var h = head.Name;
        var i = head.Tip;
        var k = head["huh?"];

        /* Boom! */
        var a = head.AheadBy;
        var b = head.BehindBy;
        var g = head.IsTracking;
        var j = head.TrackedBranch;
    }
}

This repository is in a rare state: the Head is pointing to an unborn branch.
An unborn branch is a reference which doesn't exist yet, a branch which is waiting for its very first root commit. Once can easily test this state by querying the repo.Info.IsHeadOrphanedproperty.

As such, It's not clear how the library should behave when the client code tries to value a, b, g and j.

For instance, one could argue that as the reference doesn't exist, it can't track anything and accessing the TrackedBranch property should throw, because this use case makes no sense.
On the other hand, one could say, that this branch doesn't currently track anything and that it might be ok to return a null TrackedBranch.

I keep balancing between those two states of mind without being able to make a final decision. Actually, although it's a bit stretched, I'd tend toward the second solution, because it's friendlier to the library consumer.

How do you feel about this?

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Sep 19, 2012

Contributor

I like null simply because there's no case where a null branch is different from a non-existent branch, right? If there was a different case where you had a null branch and needed to distinguish it from this scenario, I might change my mind. :)

Contributor

Haacked commented Sep 19, 2012

I like null simply because there's no case where a null branch is different from a non-existent branch, right? If there was a different case where you had a null branch and needed to distinguish it from this scenario, I might change my mind. :)

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Sep 19, 2012

Contributor

My previous comment is in reference to head.TrackedBranch

Contributor

Haacked commented Sep 19, 2012

My previous comment is in reference to head.TrackedBranch

@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Sep 20, 2012

Member

In my mind, there are two separate questions here:

  1. How should TrackedBranch and IsTracking behave if HEAD is unborn?

    I don't think they should care: ultimately tracking strikes me as independent of the local reference. Consider:

    git init
    

    Without any tracking config, IsTracking should be false and TrackedBranch should be null.

    git remote add origin http://github.com/nulltoken/TestGitRepository
    git config branch.master.remote origin
    git config branch.master.merge refs/heads/master
    

    At this point IsTracking could be true, since the branch is indeed set up for tracking, but TrackedBranch will still be null because the reference doesn't exist. One could argue that a tracking branch without a reference to track isn't much of a tracking branch, and I'd be hard-pressed not to agree. If IsTracking returns true, it seems reasonable to assume TrackedBranch wouldn't return null. (This is how I originally implemented it.)

    git fetch
    git show origin/master
    

    At this point IsTracking is obviously true and TrackedBranch should return repo.Branches["origin/master"], a valid tracked reference.

    git pull
    git show master
    

    At this point master will match origin/master, as expected for a tracking branch.

    If I'm on target with the above, git_branch_tracking() probably needs a counterpart that accepts a branch name instead of a branch reference.

  2. How should AheadBy and BehindBy behave if there's not a path between HEAD and the tracked branch?

    This could occur because HEAD is unborn, because the remote ref does not exist, or because the two references don't share history. In either case, I don't think throwing is a good experience for the consumer; it's not an invalid state, just an uncommon one. 0 doesn't feel right because it suggests that the tracker and tracked match when they do not. That leaves null: ahead/behind state is simply unknown.

Member

dahlbyk commented Sep 20, 2012

In my mind, there are two separate questions here:

  1. How should TrackedBranch and IsTracking behave if HEAD is unborn?

    I don't think they should care: ultimately tracking strikes me as independent of the local reference. Consider:

    git init
    

    Without any tracking config, IsTracking should be false and TrackedBranch should be null.

    git remote add origin http://github.com/nulltoken/TestGitRepository
    git config branch.master.remote origin
    git config branch.master.merge refs/heads/master
    

    At this point IsTracking could be true, since the branch is indeed set up for tracking, but TrackedBranch will still be null because the reference doesn't exist. One could argue that a tracking branch without a reference to track isn't much of a tracking branch, and I'd be hard-pressed not to agree. If IsTracking returns true, it seems reasonable to assume TrackedBranch wouldn't return null. (This is how I originally implemented it.)

    git fetch
    git show origin/master
    

    At this point IsTracking is obviously true and TrackedBranch should return repo.Branches["origin/master"], a valid tracked reference.

    git pull
    git show master
    

    At this point master will match origin/master, as expected for a tracking branch.

    If I'm on target with the above, git_branch_tracking() probably needs a counterpart that accepts a branch name instead of a branch reference.

  2. How should AheadBy and BehindBy behave if there's not a path between HEAD and the tracked branch?

    This could occur because HEAD is unborn, because the remote ref does not exist, or because the two references don't share history. In either case, I don't think throwing is a good experience for the consumer; it's not an invalid state, just an uncommon one. 0 doesn't feel right because it suggests that the tracker and tracked match when they do not. That leaves null: ahead/behind state is simply unknown.

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Sep 20, 2012

Contributor

Well given that AheadBy and BehindBy are int properties, they can't be null. I don't think they should return -1. Either they should be nullable ints (int?) or they should throw.

Contributor

Haacked commented Sep 20, 2012

Well given that AheadBy and BehindBy are int properties, they can't be null. I don't think they should return -1. Either they should be nullable ints (int?) or they should throw.

@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Sep 20, 2012

Member

I do in fact know that int values can't be null... ;)

Member

dahlbyk commented Sep 20, 2012

I do in fact know that int values can't be null... ;)

@nulltoken

This comment has been minimized.

Show comment
Hide comment
@nulltoken

nulltoken Sep 20, 2012

Member

@dahlbyk Hmm. Previously, I was only dealing with two potential solutions. Now, with some git config trickery, you bring up a whole new debate. That's not very nice of you :)

First of all, I'd like to thank you:

  • This is an amazingly well documented proposal
  • It... works

However:

  • It slightly looks like a clone "exploit", doesn't it?
  • Strangely, I can't help but feeling a little uneasy with an unborn branch tracking a ghostly remote one. Reminds me of some horror movies I saw when I was younger...
  • Having IsTracking == true and TrackedBranch == null looks like an unstable state, a potential magnet for a SchrödingerBranch type ;-)

I agree with you that throwing doesn't provide one with a very rich experience. How about the following proposal which might be less confusing for the user?

When Head points to an unborn branch, even if some remote configuration exists

  • TrackedBranch -> null
  • IsTracking -> False
  • AheadBy and BehindBy -> null

If I'm on target with the above, git_branch_tracking() probably needs a counterpart that accepts a branch name instead of a branch reference.

/cc @carlosmn

Member

nulltoken commented Sep 20, 2012

@dahlbyk Hmm. Previously, I was only dealing with two potential solutions. Now, with some git config trickery, you bring up a whole new debate. That's not very nice of you :)

First of all, I'd like to thank you:

  • This is an amazingly well documented proposal
  • It... works

However:

  • It slightly looks like a clone "exploit", doesn't it?
  • Strangely, I can't help but feeling a little uneasy with an unborn branch tracking a ghostly remote one. Reminds me of some horror movies I saw when I was younger...
  • Having IsTracking == true and TrackedBranch == null looks like an unstable state, a potential magnet for a SchrödingerBranch type ;-)

I agree with you that throwing doesn't provide one with a very rich experience. How about the following proposal which might be less confusing for the user?

When Head points to an unborn branch, even if some remote configuration exists

  • TrackedBranch -> null
  • IsTracking -> False
  • AheadBy and BehindBy -> null

If I'm on target with the above, git_branch_tracking() probably needs a counterpart that accepts a branch name instead of a branch reference.

/cc @carlosmn

@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Sep 20, 2012

Member

When Head points to an unborn branch, even if some remote configuration exists

  • TrackedBranch -> null
  • IsTracking -> False
  • AheadBy and BehindBy -> null

That works for me.

Note that a robust AheadBy/BehindBy implementation likely requires returning null if either TrackedBranch or Tip are null.

Member

dahlbyk commented Sep 20, 2012

When Head points to an unborn branch, even if some remote configuration exists

  • TrackedBranch -> null
  • IsTracking -> False
  • AheadBy and BehindBy -> null

That works for me.

Note that a robust AheadBy/BehindBy implementation likely requires returning null if either TrackedBranch or Tip are null.

@nulltoken

This comment has been minimized.

Show comment
Hide comment
@nulltoken

nulltoken Sep 21, 2012

Member

Note that a robust AheadBy/BehindBy implementation likely requires returning null if either TrackedBranch or Tip are null.

@dahlbyk 👌

@Haacked @aroben Would this suit your need?

Member

nulltoken commented Sep 21, 2012

Note that a robust AheadBy/BehindBy implementation likely requires returning null if either TrackedBranch or Tip are null.

@dahlbyk 👌

@Haacked @aroben Would this suit your need?

@carlosmn

This comment has been minimized.

Show comment
Hide comment
@carlosmn

carlosmn Sep 21, 2012

Member

The "config trickry" is irrelevant, because the branch that's being configured doesn't exist. You might as well run git config branch.ð¶øéöëóå.remote origin for all the good it's doing there (it will become relevant once a branch with that name does exist but that case works already, I gather). You should have attributes return 0 or null when asking for details. IMO AheadBy and BehindBy should do whatever they do for any existing branch that doesn't have any upstream information configured, as the error would be equivalent.

If I'm on target with the above, git_branch_tracking() probably needs a counterpart that accepts a branch name instead of a branch reference.

If you can't get a reference to it, you can't make it track anything.

Member

carlosmn commented Sep 21, 2012

The "config trickry" is irrelevant, because the branch that's being configured doesn't exist. You might as well run git config branch.ð¶øéöëóå.remote origin for all the good it's doing there (it will become relevant once a branch with that name does exist but that case works already, I gather). You should have attributes return 0 or null when asking for details. IMO AheadBy and BehindBy should do whatever they do for any existing branch that doesn't have any upstream information configured, as the error would be equivalent.

If I'm on target with the above, git_branch_tracking() probably needs a counterpart that accepts a branch name instead of a branch reference.

If you can't get a reference to it, you can't make it track anything.

@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Sep 21, 2012

Member

There are lots of edge cases: somebody deletes remote master, I git remote prune origin, now I'm configured to track a branch that doesn't exist. Uncommon, but we should still handle them gracefully if we can. I think false/null/null/null for a nonexistent tracked reference is as graceful as any other option.

Member

dahlbyk commented Sep 21, 2012

There are lots of edge cases: somebody deletes remote master, I git remote prune origin, now I'm configured to track a branch that doesn't exist. Uncommon, but we should still handle them gracefully if we can. I think false/null/null/null for a nonexistent tracked reference is as graceful as any other option.

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Sep 21, 2012

Contributor

AheadBy and BehindBy simply returning 0 is most friendly to clients.

0 doesn't feel right because it suggests that the tracker and tracked match when they do not.

Another way to interpret both these values being 0 is that the branch is neither ahead nor behind the tracking branch if it exists. Which is true. For UIs, it reduces the need to special case certain displays which only light up when these values are not 0.

I don't feel to strongly about this at the moment either way though.

Contributor

Haacked commented Sep 21, 2012

AheadBy and BehindBy simply returning 0 is most friendly to clients.

0 doesn't feel right because it suggests that the tracker and tracked match when they do not.

Another way to interpret both these values being 0 is that the branch is neither ahead nor behind the tracking branch if it exists. Which is true. For UIs, it reduces the need to special case certain displays which only light up when these values are not 0.

I don't feel to strongly about this at the moment either way though.

@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Sep 21, 2012

Member

That's true. From posh-git's standpoint, 0/0 or null/null will render the same. And if you do want to render tracked differently, you can check IsTracked.

Thanks for the sanity check. :)

Member

dahlbyk commented Sep 21, 2012

That's true. From posh-git's standpoint, 0/0 or null/null will render the same. And if you do want to render tracked differently, you can check IsTracked.

Thanks for the sanity check. :)

@nulltoken

This comment has been minimized.

Show comment
Hide comment
@nulltoken

nulltoken Sep 22, 2012

Member

"Another way to interpret both these values being 0 is that the branch is neither ahead nor behind the tracking branch if it exists"

I'm not very fond of the "if it exists" part.

Let's make AheadBy and BehindBy nullable ints. This way the 0 value will bear only one meaning.

Member

nulltoken commented Sep 22, 2012

"Another way to interpret both these values being 0 is that the branch is neither ahead nor behind the tracking branch if it exists"

I'm not very fond of the "if it exists" part.

Let's make AheadBy and BehindBy nullable ints. This way the 0 value will bear only one meaning.

@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Sep 22, 2012

Member

I think it's sufficient that 0 just means "not ahead/behind". That might be because it matches the tracked remote, or because there isn't a tracked remote; if your logic requires differentiating between the two, IsTracked has that answer.

Member

dahlbyk commented Sep 22, 2012

I think it's sufficient that 0 just means "not ahead/behind". That might be because it matches the tracked remote, or because there isn't a tracked remote; if your logic requires differentiating between the two, IsTracked has that answer.

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Sep 22, 2012

Contributor

What @dahlbyk said. :) Nullable primitives can be a pain to a consumer sometimes. I'd only use a nullable if the same information can't be attained any other way. As Keith points out, the IsTracked property gives us the other information.

Contributor

Haacked commented Sep 22, 2012

What @dahlbyk said. :) Nullable primitives can be a pain to a consumer sometimes. I'd only use a nullable if the same information can't be attained any other way. As Keith points out, the IsTracked property gives us the other information.

@yorah

This comment has been minimized.

Show comment
Hide comment
@yorah

yorah Sep 25, 2012

Contributor

Unless somebody is already working on it, I'm going to send a PR for this.

Contributor

yorah commented Sep 25, 2012

Unless somebody is already working on it, I'm going to send a PR for this.

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