Revision of TCW-20 re branching in TEIC GitHub Repo

[ebeshero]

We should revise the following portion of TCW 20, to better assist Council members in how to work in branches on TEIC GitHub repositories:

If you are working on a particularly complex change involving multiple files, it may be better to create a branch in which to work on your changes, and then to merge the branch when you are confident that the work is successfully completed. See any guide to git for instructions on how to approach that. Bear in mind that our Jenkins servers only build the dev branch, so you cannot depend on them to test-build your working branch.

We should revise this to provide an explicit method that works best for our TEIC GitHub repository, so we have our own custom preferred process for branch work. We should clarify:
a) When it's best to open a branch in the first place (and not be working in the dev branch),
b) When / how to update the branch with commits to dev,
c) When/ how to merge the branch back into dev. (Should this always happen by pull request and a request for review, for example?)
d) If we do think pull requests are the best way, we should also detail how/when best to handle pull requests. (Can a Council member approve her own pull request, for example?)

Regarding b): I've been reading about the "best" ways to keep branches updated, and the differences, for example, between rebase and merge for keeping a branch tip updated. Rebasing is pretty dangerous in that it "rewrites history" by resequencing commits, so I am not sure we ever want to use it for updating a branch from dev, before merging it back. Do we want to always advise, then, to update a branch by merging it with dev? I'm not 100% certain of that either.

[ebeshero]

PS: I'm self-assigning this ticket, but I think we probably want a few of us assigned to monitor/help with testing some approaches to see what's best. Who wants to help? :-)

[martindholmes]

Since we rarely investigate the precise history of commits in our repos (to my knowledge, anyway), I don't think there's any advantage in using rebase; I'd suggest using merge, with a strong recommendation to merge dev into your temporary branch frequently so that it doesn't get too far out of sync.

[hcayless]

As far as rebasing goes, I think it's fine to do it (I do it all the time), BUT, you should only use rebase when you're the only one who has a copy of that branch. If you've pushed it, and more particularly, if anyone else has a copy of it, then you should ONLY use merge. The reason being that rebase results in new commits with new hashes, and so you'd wind up with your local branch having a very different (and incompatible) history from the remote branch, making updates problematic.

[ebeshero]

Thanks, @hcayless : rebasing on a solitary branch does seem an easy way to update it. I'm specifically worried, though, about how best to update one's branch (with and/or without other branch contributors), having run amok of this in mysterious ways recently. I work in branches on other project repos all the time, and merging to update hasn't caused me problems there, but I've had some weird experiences here that I don't quite understand. Okay, one of those was my stupid fault for merging to master instead of dev (duh!) but the other I'm not so sure of. I tried merging dev into my branch, before doing a pull request on dev, and the pull request looked so weird (duplicating recent commits to dev) that I cancelled it and made a new branch holding the commits to the branch before my merge from dev. Could be the merge was foul somehow, but...ugh. I'd like to try to figure out what happened, or what can go wrong and how so it's not a mystery any more.

[duncdrum]

When reviewing PRs, rebasing tends to greatly simplify live for the reviewer. I would not take the warning about changing history to mean "don't do it".

Shouldn't travis be the new jenkins mentioned in the GL?

One thing to simplify contributions is having a dependable main branch (i.e. dev should be free of test breaking commits). Consequently direct work in dev should be the exception, instead I would encourage folks to work in branches / forks by default.

A common rule in open source projects is that nobody merges / approves their own pull-requests. This works pretty well in my experience, and might be a good strategy for the TEI repo.

[martindholmes]

@duncdrum I think of dev as the branch which will break things, while master is the branch that shouldn't break. Since it's quite difficult to run the complete suite of tests locally, it's often difficult for anyone to know exactly what will break until they commit and see the Jenkins builds, and in fact there are differences between the setups of Jenkins servers which occasionally throw up issues we need to deal with.

Up to now we haven't insisted that every change should be made in a branch; sometimes you just need to fix one example, or tweak the wording, and that shouldn't be in a branch. But it's often the case that what looks like a simple task turns out to be messy, and once you've started on the change in dev, you're stuck there (or you need to rewind before branching). Perhaps a simple rule that all changes are made in a branch would make things simpler. If so, we need to be cleaning up old branches regularly.

[duncdrum]

If so, we need to be cleaning up old branches regularly.

True, luckily thats is fairly easy, and can be managed going forward by reviewers cleaning up merged branches when merging. Some housekeeping seems about 15 years overdue…

Having failing HEAD in dev means that all contributors have to debug the state of HEAD first, find a commit that doesn't break, create a branch and then start working on their stuff. This if course assuming that they can run the tests locally. This seems very inefficient and a source of frustration. It's also about the message one sends to potential contributors, if the maintainers can't keep their default branch in order, why should I…

so here is my unsolicited take on Elisa's OP:
a) when in doubt open a branch (better yet open a branch in your own fork), only exceptionally simple fixes (typos) can go straight to dev
b) merging dev back into feature branch only comes into play when there are conflicts. In which case i would actually suggest rebasing so that the commit history of the PR is easier to review both now and in the future
c + d) PR will always trigger Travis, they should only be merged when they are conflict free and pass the testsuite. Nobody merges their own prs. Merging PRs should require one council review who performs the merge and deletes the branch. (You might want to set up a team on Github so one can @mention the council)

e) when in doubt cherry-pick

[peterstadler]

Thanks @duncdrum for you input. I think that's the way it should be.

(BTW, there is a team @TEIC/tei-technical-council )

[ebeshero]

I didn't get to this one for the July 2019 release, so I'm just nudging it forward for 3.7.0.

[ebeshero]

Removing the Guidelines milestone since this is unrelated to Guidelines.

[ebeshero]

I've added new detailed documentation to TCW-20 on working in branches including the advice here about rebasing, and how Council members should work with pull requests. @martinascholger @peterstadler @hcayless @martindholmes would you like to take a look and see if we should change anything here? If it looks good, we can close this ticket.

[ebeshero]

Here's the latest revision, and I'll really step aside and leave it for you all to read now: TEIC/Documentation@a7171ad

[ebeshero]

For comparison, here's the minimal discussion of branching we had before: https://github.com/TEIC/Documentation/blob/5099423f75e7ce4bb825d5a7f18ee92a0a20510d/TCW/tcw20.xml#L266

[ebeshero]

@martinascholger @peterstadler @hcayless If these revisions look okay to you, perhaps we can close this ticket?

[peterstadler]

@ebeshero Thanks for the write-up!
I'd only like to see Hugh's warning about rebasing added to the text and then it occurred to me that we do not have sort of a merge policy. Do we prefer simple merge commits or squash the commits first? My personal rule of thumb is that when the commits in the branch are predominantly 'trial and error', i.e. changing the same thing over and over I prefer to squash the commits, but when there's a relevant history of changes in the branch itself (that I want to retain) I prefer the merge commit.

[martinascholger]

Thanks @ebeshero!
I also found this documentation https://github.com/TEIC/TEI/blob/master/Documents/Git-README.md and wonder if we should add some bits of it - e.g. how to create/delete a temporary branch.

[hcayless]

FYI: I've added a rule banning force pushes to the dev branch (this is the sort of thing you might be invited to do if you rebased in dev and would potentially make life hard for others). This seems sensible to me, but I can remove it if people object. We already had one in place for master.

[martindholmes]

@peterstadler With everyone now (we hope) able to do local testing, we shouldn't see lots of trial-and-error commits. But if you're doing something long and complicated and can't do it in small steps, I guess there's a need to commit stuff that might be broken.

[ebeshero]

@martindholmes Having just pushed a short series of commits to introduce a new attribute class last night (slightly tricky work), I noticed some things that my local test environment (pulling from Jenkins a la @peterstadler ) did not alert me to. Pushing my branch activated Travis-CI which alerted me to a major thing I'd missed. (When I started work, my local build tests actually were passing, when they probably shouldn't have been! I'd forgotten to add that crucial include in a Guidelines chapter to make the new attribute class available for linking.)

So, I'm aware from just doing this last night that the local build tests are not always catching things we'd expect, or catching them in a different way than the Travis build. I needed a combination of both to show me what I was missing!

[martindholmes]

@ebeshero What exactly did the local tests miss?

[ebeshero]

@martindholmes I had forgotten to add an include pointing to the new attribute class, and the local build tests didn’t notice anything amiss about that. They also didn’t pick up anything wrong with the new inconsistencies I had introduced with @where, but I saw them in the Travis test after I pushed. That is, Travis’s error messages helped me to realize I needed to do what you see in line 928 of this commit:
a722cf5#diff-9e049251885d76c9f36304e11dd70803R928

[ebeshero]

@martindholmes Here's another thing that my local testing doesn't catch (and it's happened to me repeatedly this year): when I add new examples that use xml:ids, and I lazily duplicate the same ids in a related example elsewhere in the Guidelines. My local tests don't give me any errors about this, but the Travis build catches it.

[ebeshero]

@hcayless Question about force-pushing: What if we want to amend the most recent commit message? (I write this having just written one I want to amend...) Usually the easiest way to do this is to force push. I guess you could rebase in all cases...

[hcayless]

I assume you mean amend after pushing? If someone else has pulled after you pushed, and then committed on top of that, then they have a mess to sort out before they can push their commits. My recommendation would be don’t ever do it, but that’s probably a decision Council should make as policy. Amending before pushing is fine of course, and you can force push to other branches than dev and master.

[ebeshero]

@hcayless Indeed yes--and it's a pretty common reason to want to do a force-push. I agree, it's a mess on the dev branch, but fine if it's just in your own branch. I'd want to say something specifically about that because, having seen myself need badly to edit my lousy already-pushed commit message, I'm pretty sure this is a human condition thing that people will need to manage.

Another thought: have we addressed somewhere in TCW-20 about how to write a good commit message that identifies the ticket it's associated with?

[tuurma]

@ebeshero commit message style: the minimum would be to add issue number prefixed by fix, close, address etc, e.g. like Peter did for 44429fa

some people like semantic commit messages similar to https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-guidelines Personally I think it can be an overkill but basic structure like fix/feat/docs (scope) short_description fix/address #issue_number makes a lot of sense

[ebeshero]

Looks like that memo about how to write good commit messages has been prominently added, so I think this ticket is ready to close, if Council agrees!

[sydb]

Council agrees to close, but wonders if we shouldn’t also add information about branching.