RFR: Improve "git sync" and "git fork" resilience

Magnus Ihse Bursie ihse at openjdk.java.net
Thu Sep 30 14:02:47 UTC 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.


Commit messages:
 - Improve verbose logging and normal output
 - Extracting all credential handlings into setupCredentials
 - Fix ssh vs https
 - -N does not match behavior in git fork; remove it
 - Refactorings and code improvements:
 - Rename for consistency
 - BUGFIX: Adding --no-remote would disable --sync and --setup-pre-push-hook
 - More readability refactoring
 - Restructure and extract to methods to increase readability
 - Remove support for undocumented mode of operation of "forking" into current directory with existing repo
 - ... and 16 more: https://git.openjdk.java.net/skara/compare/03e8d58d...255725ac

Changes: https://git.openjdk.java.net/skara/pull/1226/files
 Webrev: https://webrevs.openjdk.java.net/?repo=skara&pr=1226&range=00
  Stats: 850 lines in 2 files changed: 386 ins; 222 del; 242 mod
  Patch: https://git.openjdk.java.net/skara/pull/1226.diff
  Fetch: git fetch https://git.openjdk.java.net/skara pull/1226/head:pull/1226

PR: https://git.openjdk.java.net/skara/pull/1226

More information about the skara-dev mailing list