search

json-schema-org / json-schema-spec

The JSON Schema specification
http://json-schema.org/
Other
3.82k stars 266 forks source link

Bring over changes from the patch release #1250

Closed jdesrosiers closed 2 years ago

jdesrosiers commented 2 years ago

Many of the changes in master for the patch release have not been applied to draft-next. This is an attempt to merge those changes. Weird things may have happened with the merge, so please review carefully.

Julian commented 2 years ago

I'll read this more carefully tomorrow, but I take it it's not easy to cherry pick or merge stuff from master to this branch?

Doing git diff origin/master to just see what changes remain after this PR shows some stuff that still should be on this branch (trivial stuff like badges, README changes).

Relequestual commented 2 years ago

Could a rebase might have been more appropriate? IMHO, maintaining commit history matters, as we can much easier find related PRs and Issues for a given change. We also need to bring over other changes such as the ADRs.

jdesrosiers commented 2 years ago

@Relequestual I'm a big fan of rebasing, but I don't think it's appropriate in this case. Remember that a rebase rewrites history meaning that everyone who currently has the draft-next branch checked out (including existing PRs) would then have an old version of history in a way that's not easy to recover for the average git user.

I originally did this as a merge, but the commit history was really wacky, so I dropped the merge commit and just added the changes in a new commit. The intention is that draft-next will be merged into master when it's released, so the commit history will not be lost. This was also why I left out the ADRs, because they will be there when we merge back the master. They don't need to be added in draft-next as well.

Rebasing is an option, but there are a few challenges associated with it. With a rebase, there are will be no PR which means it's more difficult to review and there's no record that it happened. Then we need to warn anyone who has the branch checked out and possibly assist them with the git-fu necessary to get their branch up-to-date with the changes.

handrews commented 2 years ago

@jdesrosiers I think that maybe what @Relequestual meant was to rebase the commits from the patch release onto draft-next instead of either merging or just reconstructing into a flat commit? The rebased commits are then PR'd like any other set of commits appended to a branch.

jdesrosiers commented 2 years ago

@handrews Do you mean applying commits from master on top of draft-next? That's not a rebase, but It's an option. The problem is that I don't think it will merge to master properly when the time comes because you would have the same commit applied twice in different points in history. We can get around that by simply replacing master with draft-next when the time comes instead of attempting to merge.

Julian commented 2 years ago

If you cherry pick, which it sounds like is what you're describing, git will know not to merge twice when the branch is later merged.

jdesrosiers commented 2 years ago

@Julian I'm not sure how that's possible with commits that had conflicts. It can't just skip the commit or it misses the merge conflicts. So, even if git is smart enough to not have a conflict, there would have to be duplicate commits in the history.

jdesrosiers commented 2 years ago

I'm ok with doing this however we all agree we want to do it. What are everyone's preference?

jdesrosiers commented 2 years ago

I had an idea that I think will work well.

  1. Create a branch off of master as something like 2020-12-patch.
  2. Rebase draft-next and merge it to master (via PR).
  3. Rename master to main and continue development for the next draft on main.
karenetheridge commented 2 years ago

Rebasing draft-next will rewrite the commits, meaning it won't be possible to see what the commits originally were (and PRs that were made to the draft-next branch will have their history altered as well). The only proper way to handle a situation like this is to merge, where the merge commit is not empty (as it is for uneventful merges), and all the conflicts resolved in that merge commit (which hopefully should be small).

This can be done fairly easily by saving a copy of the files as you have them here in this PR, and using that content as the resolved form of the files in the merge commit. The diffs from old-main to the head commit, and from draft-next to the head commit, should both make intuitive sense, when viewed separately (which is a good thing to do when reviewing this change).

It's not nice as a PR though - so I suggest making a new branch (say "new-main"), making the merge there, then pushing the branch and commenting in this PR what you called it so everyone can review using their favourite tools. Then when it's approved, simply git reset --hard new-main from the main branch, and git push --force-with-lease, which should be clean as main -> new-main will be a single commit.

jdesrosiers commented 2 years ago

Closing in favor of #1251