Refactor'n'Feature

[scunz]

• Change Submodule in a way that it inherits RepoObject and change the semantics for accessing parent/child repository. Also give it a Private class header and remote the constructor that initializes with Repository and SM-Name.
• Make opening sub-repositories implicit.
• Change Index to follow the Nothing-Useful-Through-Constructor rule. That means: create static methods: Index::createInMemory and Index::open. Switch the constructor to do nothing then.
• Change IndexConflict in the same way: Just turn the 3-parameter constructor into IndexConflict::create.
• When these are done, I will remove all constructors, operator= operator== and such things and put them into a macro, so that they are really the same for all of the Base/RepoObjects. This is also the only way to get most of them inline and give the compiler enough hints to get things "good".
• Move the physically creating method out of Repository and make them static in their respective classes. I think this is better API.
• Implement the union of RefName and Reference
• Add handling of notes to RefName; even though RefName is now a RepoObject, skip reading all the notes relevant config settings.
• Create separate BranchRef, TagRef and NoteRef, objects that inherit Reference.
• Add static creators to Submodule, BranchRef and TagRef. Deliberately skip NoteRef. This is too complex for now and requires integration all over the place.
• Implement whatever "setters" are required to write enough testing for the repoman/refactor code.
• Create a basic CheckoutOperation
• Cleanup lots of internal stuff
• Cleanup the API

[scunz]

@antis81, should I remove Reference::updateHEAD()? I can barely imagine that this was really meant as a public method...

[antis81]

should I remove Reference::updateHEAD()? I can barely imagine that this was really meant as a public method...

Yes, it can be removed when Reference::checkout() still works with remote references. This was the only reason for existance. But I think you changed it that way either.

[scunz]

Yes, it can be removed when Reference::checkout() still works with remote references.

I am not very sure, if all the checkouts we have that can updateHEAD ever worked correctly. But that depends: If one defines that Reference::checkout() shall operate on the lowest possible level, then it might be right to checkout a remote branch as a detached head. But i.e. the command line level of git would create a local branch pointing to the same commit and instead update HEAD to point there.

The MGV user interface should of course do the latter (which it never did). But a random tool developed using libGitWrap might want to simply checkout a remote reference (though first of all, it could do that in many different ways and second, it would be a bad design choice - since almost anything it could do with the checkout would be much faster with a temporary index).

But to be complete: I actually think that exactly this behavior is broken by the change.

[scunz]

However, let's ask the counter-question, too: Would it make sense for setAsHEAD() to change HEAD to a detached HEAD, if the reference is pointing to a remote branch (instead of failing).

[antis81]

Yes, I'd say so. This is actually what updateHEAD() was intended to do But to make it complete: Why not let a bool parameter failOnDetachedHEAD decide that? Default should be false.

[antis81]

... But i.e. the command line level of git would create a local branch pointing to the same commit and instead update HEAD to point there.
The MGV user interface should of course do the latter (which it never did).

Please have a closer look at setAsHEAD() again - it should not work 🎱. If you look at the methods isLocal() and isBranch() you'll find, that they're more or less exact copies - which is the point. The updateHEAD() method does actually do the right thing, because git_reference_is_branch() only returns true on local branches (that's also what the lg2 docs say). Otherwise it is not local (either tag, remote or other reference).

EDIT: Rethinking that, this method should be private. It moves the HEAD being either symbolic or sha1. Keep in mind, that Reference::checkout() is a "high level" method, doing what git checkout <branch> does. A similar thing is implemented in Commit::checkout making HEAD always detached.

[scunz]

1. Commit::checkout shells out to Tree::checkout in my local code. (And as an aside in my local code it is really named Commit 😄)
2. Oh my dear. You're bloody right about the isLocal() vs. isBranch(). This is of course totally wrong. However, with isLocal() == isBranch(), setAsHEAD() should be totally equal to updateHEAD().
3. Within the context of Branch class  #44 and Tag class #45, these methods should go away in the Reference class anyway. Reference/RefName #43 will do that.
4. After all, what I wanted to achieve is that I want to get rid of the updateHEAD() totally and replace it with something that is both public and makes sense.
5. The point is, that Reference::checkout() as you implemented and want it to be, is exactly not git checkout <branch>. For any reference that doesn't start with refs/heads, it is at most:

git read-tree $REFNAME && \
    git checkout-index && \
    git update-ref HEAD $(git rev-parse $REFNAME)

We should change that. What about giving it a QFlags<CheckoutOption> opts parameter and

• if opts.testFlag(CheckoutOption::updateHEAD) is not set, don't bother to update anything at all.
• if opts.testFlag(CheckoutOption::allowDetachHEAD) is set, detach if required.
• if opts.testFlag(CheckoutOption::alwaysDetachHEAD) is set, detach even if not required.
• if opts.testFlag(CheckoutOption::createLocalBranch) and it points to a remote branch, do what git checkout would do (probably in a method createDownstreamBranch())

[scunz]

@antis81 Sorry for all this refactoring. Hope that the cries of dismay may be evanescent somewhat soonish...

I just feel like, we should do that ^^

[scunz]

@antis81,

currently, there is a mess in the object construction API:

• Most objects don't allow a useful construction via the constructor => Most objects cannot be created themselves - they have to be given to the user from someone else; actually this the repository most of the time (but can also be Commit, Tree, TreeBuilder or Index).
• There are some objects that don't follow this rule: IndexConflict, Index and Submodule.
• We have some small input/output objects that don't follow this at all: Signature, RefSpec, FileInfo. I think this is okay, as these are really small temporary things.
• For some classes, we provide a creator method in Repository to create physically new objects of that type. For others we provide static methods, like Reference::create.
• For most classes we don't provide anything to create these things physically (That is mostly, why I moved from Core over to working on GW)

So, what I want to do is:

1. Change Submodule in a way that it inherits RepoObject and change the semantics for accessing parent/child repository. Also give it a Private class header and remote the constructor that initializes with Repository and SM-Name.
2. Change Index to follow the Nothing-Useful-Through-Constructor rule. That means: create static methods: Index::createInMemory and Index::open. Switch the constructor to do nothing then.
3. Change IndexConflict in the same way: Just turn the 3-parameter constructor into IndexConflict::create.
4. When these are done, I will remove all constructors, operator= operator== and such things and put them into a macro, so that they are really the same for all of the Base-/RepoObjects. This is also the only way to get most of them inline and give the compiler enough hints to get things "good".
5. Move the physically creating method out of Repository and make them static in their respective classes. I think this is better API.
6. Implement the union of RefName and Reference
7. Create separate BranchRef and TagRef objects that inherit Reference.
8. Add static creators to Submodule, BranchRef and TagRef.
9. Implement whatever "setters" are required to write enough testing for the repoman/refactor code.

But actually, I have to finish the thing that I'm currently working on.

Any comments on this plan?

[scunz]

Rebasing to development, because ded8415 has been cherry-pick'ed to development as a2c94f3

[scunz]

And rebased again to get the GW_DEPRECATED back in again.

[scunz]

Merging this, as it contains lots of good stuff and everything get's blocked again.