Add `setup-fork` command, reimplement `clone` command as its child #293

oxij · 2021-02-13T22:10:07Z

This command greatly simplifies setup for a previously clonned repository.

A somewhat unforturate consequence is that the name of the only hub.hookscript
had to be changed since invoking that hook is part setup-fork now.
Since I had to rename it anyway I also renamed it to match git's own hook
naming convention of using "-"es to separate words.

I tested this a bunch (as shown with newly added examples) but do test some more
before merging.

oxij · 2021-02-13T22:18:52Z

This closes #235 and #43. And probably closes #158, since running git-hub setup-fork will do the right thing in the most common case (and gracefully fail otherwise).

llucax

Thanks for taking the time to tackle so many big issues!

I'm very keen to merge this PR, but I have a few comments and change requests. Please look at the individual comments.

Also, if you could write some release notes for the change, it will make making the next release much easier for me. You should create a file like relnotes/setup-fork.feature.md with the following format:

### Short summary

Longer description of the new features (new command, etc.).

And maybe since this should deprecate postclone, also include some migration notes in relnotes/setup-fork.migration.md with the same format (short summary + a longer explanation directed to the users about what to do, in this case it should be enough to tell them to rename the postclone hook to post-setup-fork if they are using it).

If you want more details on how to write release notes, please have a look at https://github.com/sociomantic-tsunami/neptune/blob/v0.x.x/doc/library-contributor.rst#release-notes

Thanks again!

llucax · 2021-02-14T10:56:31Z

git-hub

    cmd_required_config = ['username', 'oauthtoken']
-    cmd_help = 'clone a GitHub repository (and fork as needed)'
-    cmd_usage = '%(prog)s [OPTIONS] [GIT CLONE OPTIONS] REPO [DEST]'
+    cmd_help = 'fork a GitHub repository'


This is still a maybe right? If I understood correctly, if the fork already exists it will just set up the local repo. If so, it might be better to make this more explicit here.

llucax · 2021-02-14T10:57:41Z

git-hub


    @classmethod
    def setup_parser(cls, parser):
-        parser.add_argument('repository', metavar='REPO',
+        parser.add_argument('repository', metavar='REPO', nargs='?',


The help here should probably mention what happens if this argument is omitted.

llucax · 2021-02-14T11:09:16Z

git-hub

-        os.chdir(dest)
-        fetchremote = args.forkremote if triangular else args.upstreamremote
-        remote_url = repo['parent'][urltype]
+        if dest is not None:


This code will only ever used when invoking this function from the clone command, right? If this is correct, wouldn't it make more sense to just move this code to the clone command?

If not, wouldn't it make sense to rename dest to clone_to or something like that to make it more explicit that a clone can happen here?

In general I'm not a fan of such a large method, so If you find any way to factor out some parts, it would be greatly appreciated. Maybe just remove some small parts as independent methods and then write slightly longer run methods for setup-fork and clone would make more sense to me.

llucax · 2021-02-14T11:16:48Z

git-hub

-                'Fetching from {} ({})'.format(fetchremote, remote_url))
-        run_hookscript('postclone', env=dict(
+        git_config('triangular', value='true' if triangular else 'false')
+        run_hookscript('post-setup-fork', env=dict(


This makes a lot of sense, but is a bit unfortunate that this PR comes right after v2.1.0 was released, because it now constitute a breaking change :( (and we are quite strict about following semver).

Making it a non-breaking change should be easy enough though. We can just deprecate postclone and accept both until the next major is released and we can get rid of the old name. So if you can accept postclone as a hook name too and emit a warning saying the name is deprecated and will be removed in the future, everything should be fine to put this change in v2.2.0.

You can remove the old name from the documentation, don't worry about that.

llucax · 2021-02-14T11:19:30Z

man.rst

@@ -581,6 +604,88 @@ from. These are the git config keys used:
 [1] https://developer.github.com/v3/pulls/#get-a-single-pull-request


+EXAMPLES
+========


Nice! I'm not sure if you saw we have a bunch of examples in the README too. Not sure what to do with this, because it is definitely nice to have example both in the "home page" and in the man, and AFAIK there is no easy way to include them in both without duplicating them. So feel free to chose what makes more sense to you, but it might be good to have some sort of consistency between both set of examples. Not sure if you looked at the examples in the README.

llucax · 2021-02-14T11:25:25Z

Oh, the CI is also failing. There seems to be a problem generating the man page, there is probably some issue with the man.rst ReST formatting.

llucax · 2021-02-16T09:38:51Z

git-hub

+            git_remotes = git('remote', 'show', '-n').split('\n')
+            remote = args.upstreamremote
+            if remote not in git_remotes:
+                die("No REPO specified, nor does `{}` remote exist", remote)


Suggested change

die("No REPO specified, nor does `{}` remote exist", remote)

parser.error("No REPO specified, nor does `{}` remote exist".format(remote))

This will use the parse to report the usage too.

llucax · 2021-02-16T09:42:14Z

git-hub

+        personal = False
+        if upstream is None:
+            if args.upstreamremote != args.forkremote:
+                die('You are setting up with a personal repository as upstream, '


Suggested change

die('You are setting up with a personal repository as upstream, '

parser.error('You are setting up with a personal repository as upstream, '

llucax · 2021-02-16T09:50:33Z

I'm trying this out with the case of using no fork and I'm having issues creating a PR. It seems the hub.user config variable is still used in the head property in the json sent to create the PR, which makes it fail as there is no fork.

llucax · 2021-02-17T14:28:43Z

I'm trying this out with the case of using no fork and I'm having issues creating a PR. It seems the hub.user config variable is still used in the head property in the json sent to create the PR, which makes it fail as there is no fork.

This is actually a different use case. I tried an only-upstream configuration, and it is actually possible to achieve by setting the git config hub.forkrepo <upstream>. I think it makes sense too to add another option to configure forkremote via setup-fork too, for example:

If you want to set up an *upstream-only* repo (no fork is present, centralized approach)::

    git clone https://github.com/sociomantic-tsunami/git-hub
    cd git-hub
    git-hub setup-fork -U origin -F origin -f sociomantic/git-hub

 This will create new branches in the upstream repo directly and no fork will be used. ``--triangular`` will be force to ``false``.

Where -f/--fork-remote sets the hub.forkremote git configuration.

llucax · 2021-03-19T14:20:05Z

ping @oxij ? 😢

oxij · 2021-03-21T16:20:39Z

Those are good suggestions, except I'm not sure how do I go about implementing the "hook"-related one, the hook script is a single string there. Anyway, I was thinking, it probably makes more sense to just collect all of those and other related commands into a subcommand, like `git hub repo`. E.g: `git hub repo fork <remote-url>`, `git hub repo setup <lots of options>`, `git hub repo clone <remote-url>`, etc.

llucax · 2021-03-21T18:54:57Z

Those are good suggestions, except I'm not sure how do I go about implementing the "hook"-related one, the hook script is a single string there.

You just accept both postclone and post-setup-fork and if postclone was used, print a warning about it being deprecated.

Anyway, I was thinking, it probably makes more sense to just collect all of those and other related commands into a subcommand, like git hub repo.

I'm neutral about this, it would be probably nicer if managing how the git repo is related to the GitHub repo gets more complicated but again it would be a breaking change, so at least git hub clone should be still supported (although deprecated). Because of this I don't mind keeping it dirty for now (and do a cleanup on a future major release). I prefer merging these changes sooner than later, even if not perfect.

Also, please let me know if I can help in any way, if you don't mind me pushing commits to your branch I can do some of the cleanups/changes I suggested.

oxij · 2021-03-23T17:02:29Z

> Those are good suggestions, except I'm not sure how do I go about implementing the "hook"-related one, the hook script is a single string there. You just accept both `postclone` and `post-setup-fork` and if `postclone` was used, print a warning about it being deprecated.

Yes, but the hook is a script. Do you want to run it with both arguments? How do you check it did anything at all to deprecate the old one? git's own hooks are files, there it's easy to check the file exists, the hook thing here is a shell script, you could do some primitive code analysis on it, but, well...

> Anyway, I was thinking, it probably makes more sense to just collect all of those and other related commands into a subcommand, like `git hub repo`. I'm neutral about this, it would be probably nicer if managing how the git repo is related to the GitHub repo gets more complicated but again it would be a breaking change, so at least `git hub clone` should be still supported (although deprecated). Because of this I don't mind keeping it dirty for now (and do a cleanup on a future major release). I prefer merging these changes sooner than later, even if not perfect.

Alright.

Also, please let me know if I can help in any way, if you don't mind me pushing commits to your branch I can do some of the cleanups/changes I suggested.

Sure, feel free to do whatever with the code here. I'm a bit busy with other stuff ATM.

This command greatly simplifies setup for a previously clonned repository. A somewhat unforturate consequence is that the name of the only `hub.hookscript` had to be changed since invoking that hook is part `setup-fork` now. Since I had to rename it anyway I also renamed it to match git's own hook naming convention of using "-"es to separate words.

llucax · 2021-03-23T18:05:03Z

Yes, but the hook is a script. Do you want to run it with both arguments? How do you check it did anything at all to deprecate the old one? git's own hooks are files, there it's easy to check the file exists, the hook thing here is a shell script, you could do some primitive code analysis on it, but, well...

Right, right, right. I completely forgot it was one script and the type of hook was passed as argument (my impression was this feature was too niche to deserve having many hook scripts).

Then yes, calling the script twice would be an option (although not very nice). To be honest the cost/benefit of the rename at this point doesn't seem to be worthy. Specially since the dash-style used by git is applied to files and in this case is an argument to the file that is configured by the user, the rename seems specially pointless, right?

Sure, feel free to do whatever with the code here. I'm a bit busy with other stuff ATM.

Cool, I will make some of the suggested change when I have some spare time!

llucax added the type-enhancement label Feb 13, 2021

llucax self-assigned this Feb 13, 2021

llucax self-requested a review February 13, 2021 23:07

llucax added the type-breaking This is a breaking change, so it should go to the next major version label Feb 13, 2021

llucax requested changes Feb 14, 2021

View reviewed changes

llucax added this to the v2.2.0 milestone Feb 14, 2021

This was linked to issues Feb 14, 2021

Convert conventially cloned repostiory #235

Open

clone: Allow cloning a repo that doesn't have a parent (a personal "blessed" repo) #43

Open

Convert repo to --triangular #158

Open

llucax requested changes Feb 16, 2021

View reviewed changes

llucax force-pushed the fork-command branch from e40d24d to db1ce4e Compare March 23, 2021 18:06

llucax mentioned this pull request Mar 31, 2021

git hub pull new should set tracking branch #266

Open

llucax modified the milestones: v2.2.0, Future Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `setup-fork` command, reimplement `clone` command as its child #293

Add `setup-fork` command, reimplement `clone` command as its child #293

oxij commented Feb 13, 2021

oxij commented Feb 13, 2021

llucax left a comment

llucax Feb 14, 2021

llucax Feb 14, 2021

llucax Feb 14, 2021

llucax Feb 14, 2021

llucax Feb 14, 2021

llucax commented Feb 14, 2021

llucax Feb 16, 2021

llucax Feb 16, 2021

llucax commented Feb 16, 2021

llucax commented Feb 17, 2021

llucax commented Mar 19, 2021

oxij commented Mar 21, 2021 via email

llucax commented Mar 21, 2021

oxij commented Mar 23, 2021 via email

llucax commented Mar 23, 2021

	die("No REPO specified, nor does `{}` remote exist", remote)
	parser.error("No REPO specified, nor does `{}` remote exist".format(remote))

	die('You are setting up with a personal repository as upstream, '
	parser.error('You are setting up with a personal repository as upstream, '

Add setup-fork command, reimplement clone command as its child #293

Are you sure you want to change the base?

Add setup-fork command, reimplement clone command as its child #293

Conversation

oxij commented Feb 13, 2021

oxij commented Feb 13, 2021

llucax left a comment

Choose a reason for hiding this comment

llucax Feb 14, 2021

Choose a reason for hiding this comment

llucax Feb 14, 2021

Choose a reason for hiding this comment

llucax Feb 14, 2021

Choose a reason for hiding this comment

llucax Feb 14, 2021

Choose a reason for hiding this comment

llucax Feb 14, 2021

Choose a reason for hiding this comment

llucax commented Feb 14, 2021

llucax Feb 16, 2021

Choose a reason for hiding this comment

llucax Feb 16, 2021

Choose a reason for hiding this comment

llucax commented Feb 16, 2021

llucax commented Feb 17, 2021

llucax commented Mar 19, 2021

oxij commented Mar 21, 2021 via email

llucax commented Mar 21, 2021

oxij commented Mar 23, 2021 via email

llucax commented Mar 23, 2021

Add `setup-fork` command, reimplement `clone` command as its child #293

Add `setup-fork` command, reimplement `clone` command as its child #293