Page MenuHomePhorge

Catch up the master branch to upstream
Open, Needs TriagePublic

Description

I've got a local branch that catches up changes from upstream... but not sure where to put it for review and testing... most of the commits represent subsections, like this:

image.png (691×369 px, 26 KB)

Event Timeline

It looks like upstream has issued a number of updates which we'll want to pull in. From {E4} we discussed doing the following:

  1. Pull in all commits from upstream/master into origin/phab-master
  2. Merge origin/master with origin/phab-master
  3. Force-push origin/master

Alternatively instead of merging we could re-land all currently landed revisions as there aren't that many of them.

If we merge, a force-push should not be required - unless you mean something other than standard git merge here. (Force-push is required when rewriting already pushed history - git merge simply adds a new commit that applies the changes on top of the branch)

Here is one thing I noticed... In at least a couple of the files, there may be changes that:

  1. Have to be introduced from both upstream and Phorge
  2. Have to be introduced from upstream, and reverted from Phorge
  3. Possibly other combinations

Using an annotation of git blame in an IDE, it is possible to see date stamps for each line and use those to help determine what is should be merged or not

An example of case 2 appears to be in Harbormaster. What it looks like to me is that there are commits to Phorge that update Harbormaster, but then later upstream has a number of changes that appear to accomplish something similar, but extend notably further... So in that case it looks like we would pull in upstream changes, but revert the Phorge changes in the cases where they are overlapping in function...

This would be a legitimately good exercise to try and do "properly"... although, the thought of not doing it optimally can be a bit of a barrier to starting..

Given the edge cases outlined in T15094#2279, would there be cases in step 2 (or 1?) from T15094#2259 that might benefit from Git cherry-picking? @golyalpha, any thoughts on that? I nearly never have to use cherry-picking, or maybe I should, but either way I'm not very familiar with it other than I'm wondering if it may be relevant

After some reading I'm finding that, as far as I can tell, it's not designed to pick/integrate *specific lines* from a diff, but rather a specific whole commit (from any local or remote branch most likely).. if I'm understanding it correctly

But, perhaps, it could still have the same effect as removing lines from one, and keeping lines from the other when grabbing specific whole commits

The more I think about this the more I'm confusing myself, but hopefully some fraction of this makes sense

Merged the arcanist repository in D25039

Merged the phabricator repository in D25040

Any concerns about landing those changes? Once I land I'll see about updating this instance which should make accessing the repositories possible again.

Though it does appear additional work has been landing upstream today

This comment was removed by dcog.

Oh nice!!

Did you run into anything weird with Harbormaster? Is there a test suite built in where we can confirm that nothing breaks?

My vote is that if tests pass we go ahead and do the thing.... More changes in upstream seems fine, and moving forward if we keep up it should get easier and easier hopefully

I see it looks Harbormaster itself does the testing?

As far as I can see, Unit tests passed and Linting failed... If Unit tests pass I think that's the critical thing here, and that Linting tests would have failed anyway and we can fix that later?

I would think that your method produced the results we want... though I was noticing this:

{D25036}

image.png (432×401 px, 47 KB)

D25040: merge phab/master -> phorge/master

image.png (355×418 px, 45 KB)

I might be wrong, but it looked like there were changes started in Phorge on Harbormaster that were later started and continued in upstream... I'm not sure if there's anything conflicting but it looked like the changes in there from Phorge became outdated

But I think that getting it caught it up, and doing it this cleaner way is well worth it.. since we're using Harbormaster we should notice if something's weird

Hopefully we can get more input, but I say merge and use, will see if anyone has any concerns about just going ahead and merging it

I did not think we had Harbormaster set up to run unit tests - I think that involves configuring both Harbormaster and Drydock, and possibly Almanac which I don't think anyone has done.

I'll go back and review those Harbormaster file changes. Thanks for pointing that out!

In T15094#2281, @dcog wrote:

This would be a legitimately good exercise to try and do "properly"... although, the thought of not doing it optimally can be a bit of a barrier to starting..

Given the edge cases outlined in T15094#2279, would there be cases in step 2 (or 1?) from T15094#2259 that might benefit from Git cherry-picking? @golyalpha, any thoughts on that? I nearly never have to use cherry-picking, or maybe I should, but either way I'm not very familiar with it other than I'm wondering if it may be relevant

After some reading I'm finding that, as far as I can tell, it's not designed to pick/integrate *specific lines* from a diff, but rather a specific whole commit (from any local or remote branch most likely).. if I'm understanding it correctly

But, perhaps, it could still have the same effect as removing lines from one, and keeping lines from the other when grabbing specific whole commits

The more I think about this the more I'm confusing myself, but hopefully some fraction of this makes sense

Sorry, I completely missed that mention there. Standard git merge logic is probably more useful here, with some manual conflict resolution (if necessary).

Cherry-picking is more useful when you're, for example, backporting security fixes, or selectively merging commits from a different branch. (Both of which are generally rare.)

In T15094#2292, @speck wrote:

I did not think we had Harbormaster set up to run unit tests - I think that involves configuring both Harbormaster and Drydock, and possibly Almanac which I don't think anyone has done.

I'll go back and review those Harbormaster file changes. Thanks for pointing that out!

Do we even have servers to run the tests on?

Do we even have servers to run the tests on?

We do not. For D25039 (arcanist) the unit tests ran locally on my machine as part of the arc diff, and all tests pass. For D25040 (phorge/phab) I had to use arc diff --no-unit because running tests failed when trying to connect to a local mysql/mariadb database, so unit tests were not run.

@dcog I think the differences with the Harbormaster changes are due to the different approach taken. We planned to do the approach which you took in D25036 which re-played the Phorge diffs on top of phabricator, however in D25040 I just did a merge of the phab/master branch into phorge/master where the Harbormaster changes already existed. Since upstream didn't modify the same Harbormaster files there were no conflicts and things merged appropriately. I did a sanity check of files changed on D25005 with the files changed on D25040.