Redesign 1: GitWrapperBase becomes GitRepo

[jpichon]

I think it'd be better to wait for the whole thing to be ready before merging as the structure is a bit weird while it's halfway, but posting this as a PR to make it easier to see what it looks like and discuss/share early feedback.

See related issue #36.

[jpichon]

This looks like good progress, though I have to admit I found it a bit hard to take in due to the scope. You may be too far along to consider this, but as I was reading through, it occurred to me that it might be possible to do this in stages if we were to start with simply renaming GitWrapperBase to GitRepo, and updating all references. Then, assuming tests pass, change it over to some of the new design ideas one chunk at a time - we would not do a release until all that was done, but I don't see why the work could not be done incrementally, and it would make it much easier to parse the code submitted for reviews.

Thanks for all the comments! May I ask, are you looking at the single view with all the changes? I've been doing what you suggest with the individual commits to make it easier to take in (the first one is exactly that name change), and the tests pass for every individual commit.

I'm less sure about merging as we go, because I keep finding issues that require modifying things at the beginning again and I suspect it may end up even messier if we start merging now. Perhaps I just need to get a little bit further along first to iron out the last few larger issues. Thanks again for the comments, they are helpful.

[jguiditta]

Thanks for all the comments! May I ask, are you looking at the single view with all the changes? I've been doing what you suggest with the individual commits to make it easier to take in (the first one is exactly that name change), and the tests pass for every individual commit.

Yes, I did not click through the individual commits, I just went to the 'files changed' tab. It may have been easier to read if I had looked at the individual commits, I will try again that way tomorrow.

I'm less sure about merging as we go, because I keep finding issues that require modifying things at the beginning again and I suspect it may end up even messier if we start merging now. Perhaps I just need to get a little bit further along first to iron out the last few larger issues. Thanks again for the comments, they are helpful.

Again, you may be too far along to do that anyway, but in general, I am not a fan of having a sweeping redesign off in a separate branch and then merged in all at once. IMHO, it is easier to get a sense of where it is going, and spot problems earlier on, if the work is done incrementally. I understand not everyone feels this way, I am just explaining my thought process a bit.

[jpichon]

Again, you may be too far along to do that anyway, but in general, I am not a fan of having a sweeping redesign off in a separate branch and then merged in all at once. IMHO, it is easier to get a sense of where it is going, and spot problems earlier on, if the work is done incrementally. I understand not everyone feels this way, I am just explaining my thought process a bit.

Yes, I understand. I wish GitHub PRs worked a bit more like Gerrit where patches in a series can still be reviewed/approved individually, and the first few can merge even if there are issues with the later ones. I'm trying to keep the individual commits small, self-contained and independently reviewable. I don't mind having to rework future patches as comments come up; I also think some things are easier to reason about when they're laid out in front of you.

@jguiditta I could try to create multiple branches on my side and have one PR per commit, and number them so they can be reviewed in order. Do you think that would help?

[jguiditta]

Yes, I understand. I wish GitHub PRs worked a bit more like Gerrit where patches in a series can still be reviewed/approved individually, and the first few can merge even if there are issues with the later ones. I'm trying to keep the individual commits small, self-contained and independently reviewable. I don't mind having to rework future patches as comments come up; I also think some things are easier to reason about when they're laid out in front of you.

Agree, the patch series concept from gerrit is a good one.

@jguiditta I could try to create multiple branches on my side and have one PR per commit, and number them so they can be reviewed in order. Do you think that would help?

Hmm, I kind of like the idea, but I fear it would make the work cumbersome, which I certainly do not want - it is enough work already without adding unneeded manual steps on top (rebase of all those branches every time you change an earlier branch, for example). I will leave it up to your discretion. If you feel it would be a major benefit to go this route, and the overhead would not be as bad as I fear, go for it. Otherwise, I can certainly just review one commit at a time in this PR.

[jpichon]

Hmm, I kind of like the idea, but I fear it would make the work cumbersome, which I certainly do not want - it is enough work already without adding unneeded manual steps on top (rebase of all those branches every time you change an earlier branch, for example). I will leave it up to your discretion. If you feel it would be a major benefit to go this route, and the overhead would not be as bad as I fear, go for it. Otherwise, I can certainly just review one commit at a time in this PR.

I will give it a shot. Nothing can really move until the redesign is completed and there's nothing more I can do at this point so if this can make reviewers' life easier to get it merged faster, I'm all for it.

[jpichon]

Ok, done! You'll still have to use the "Commits" tabs to be able to review one/the latest commit only at a time on the later PRs. Someone who'd rather get a feel for how the whole lot looks together can use Redesign 9. If it turns out not to be all that helpful to reviewers after all we can close the other PRs and go back to a single one. In the meantime let's make sure they don't merge out of order and approve only one at a time!! :)