Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve "git sync" and "git fork" resilience #1226

Closed
wants to merge 26 commits into from

Conversation

magicus
Copy link
Member

@magicus magicus commented Sep 30, 2021

The Skara CLI tools git sync and git fork are very much appreciated, but they are not fully designed to make sure you get the intended result. Part of this problem is that they are too general, and part is that they are too dumb. :-)

These changes make sure that git sync always syncs between your personal fork, and the upstream repository (determined by the Git Forge provider as the "parent" repo you created your fork from), and git fork always sets up your repository for such git sync usage.

If you really know what you are doing, you can still use git sync to sync between fully unrelated repos using the --to and --from arguments, but this is highly discouraged (and will require the --force flag as well, to be accepted).

Before syncing, git sync will check that you have a proper origin and upstream remote configured, and that the upstream is consistent with what the Git Forge provider says. If the upstream remote is missing, it will be configured for you (unless --no-remote is given).

git fork will now always set the upstream remote (unless given --no-remote).

If these sounds like obvious behavior, it is worth noting what happened before. If upstream was missing, git sync had no idea what to do. git fork had a second, undocumented *) mode, in which you could run git fork without any arguments in a directory that already contained a checked out repo. That would create a personal fork of that repo on the Git Forge provider, but it would still keep origin as the upstream repo URI, and instead add a fork remote for the newly created personal fork. This mode of operation was barely supported (and broken) on git sync. The backwards way of naming the remotes could lead to no end of troubles.

*) I say "undocumented" since there is no explicit documentation about this feature that I could found. However, the "upstream URI" argument to git fork is documented as "optional" (but all other documentation assumes that it is present, so the behavior when omitted is not specified).

When implementing these fixes, it was clear that the code was in desperate need of a massive cleanup. There were dead code, code duplication, unclear logic, misleading names, etc... So in the end the code is so different from what I started from that a side-by-side comparison might be impossible. You can see individual changes in the separate commits, or you can review the new version of the files as were they new additions. These refactorings also uncovered a bug in git fork, where adding --no-remote would disable --sync and --setup-pre-push-hooks. I fixed that also.

Unfortunately, we still don't have a testing story for our CLI tools. :-( I have however done extensive ad-hoc testing, with all possible combinations of flags, etc.


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/skara pull/1226/head:pull/1226
$ git checkout pull/1226

Update a local copy of the PR:
$ git checkout pull/1226
$ git pull https://git.openjdk.java.net/skara pull/1226/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1226

View PR using the GUI difftool:
$ git pr show -t 1226

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/skara/pull/1226.diff

magicus added 26 commits Sep 27, 2021
 * Use new URI instead of URI.create from user input, according to spec
 * Properly respect --no-remote for --sync
 * Tighten variable types from String to specialized types
 * Add/improve comments
(including moving GitCredentials.approve to an earlier location)
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 30, 2021

👋 Welcome back ihse! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Sep 30, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 30, 2021

Webrevs

@magicus
Copy link
Member Author

@magicus magicus commented Sep 30, 2021

And oh, I added --dry-run to check behavior without doing any changes (mainly used it for my own debugging, but I think it can be useful in real-world scenarios as well), and improved --verbose logging.

Copy link
Member

@erikj79 erikj79 left a comment

Looks good! I like the cleanups.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 30, 2021

@magicus This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

🔍 One or more changes in this pull request modifies files in areas of the source code that often require two reviewers. Please consider if this is the case for this pull request, and if so, await a second reviewer to approve this pull request before you integrate it.

🌎 Applicable reviewers for one or more changes in this pull request are spread across multiple different time zones. Please consider waiting with integrating this pull request until it has been out for review for at least 24 hours to give all reviewers a chance to review the pull request.

After integration, the commit message for the final commit will be:

Improve "git sync" and "git fork" resilience

Reviewed-by: erikj, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 4 new commits pushed to the master branch:

  • 620f889: 1079: Support ability to defer and delegate timing of an integration
  • acda266: 1197: Label changes from java.nio.** as nio
  • 68872aa: 1194: Implement git webrev --no-comments option
  • 91be684: 1191: CommitCommentWorkItem overwhelms scheduler

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Sep 30, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good to me, too.

@magicus
Copy link
Member Author

@magicus magicus commented Oct 1, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 1, 2021

Going to push as commit e6ce7a6.
Since your change was applied there have been 4 commits pushed to the master branch:

  • 620f889: 1079: Support ability to defer and delegate timing of an integration
  • acda266: 1197: Label changes from java.nio.** as nio
  • 68872aa: 1194: Implement git webrev --no-comments option
  • 91be684: 1191: CommitCommentWorkItem overwhelms scheduler

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 1, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 1, 2021

@magicus Pushed as commit e6ce7a6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@magicus magicus deleted the better-sync-command branch Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants