From 8813da5b1375218041c4eaab3973da2b4dabe4bf Mon Sep 17 00:00:00 2001 From: Omar Jarjur Date: Mon, 13 Aug 2018 16:25:07 -0700 Subject: [PATCH 01/37] Add docs for working with forks. This change is the first step in making `git-appraise` support the concept of `forks`: remote repositories used by contributors who cannot push to the main repository. This initial change only adds the user documentation around forks. This allows us to scope out the feature while making sure that the end user experience is simple, consistent, and complete. Later commits will add tests and implement the new feature. --- README.md | 11 ++++++++ docs/forks.md | 68 ++++++++++++++++++++++++++++++++++++++++++++++++ docs/tutorial.md | 9 +++++++ 3 files changed, 88 insertions(+) create mode 100644 docs/forks.md diff --git a/README.md b/README.md index 0227c413..85446c33 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,10 @@ This design removes the need for any sort of server-side setup. As a result, this tool can work with any git hosting provider, and the only setup required is installing the client on your workstation. +Additionally, code reviews can be conducted across multiple hosting providers. +The list of forks of a repository is also stored in the repository as git +objects, allowing code reviews to be pulled from every registered fork. + ## Installation Assuming you have the [Go tools installed](https://golang.org/doc/install), run @@ -80,8 +84,15 @@ Submitting the current review: git appraise submit [--merge | --rebase] +Adding a fork: + + git appraise fork add [--owner ""]+ + A more detailed getting started doc is available [here](docs/tutorial.md). +Instructions for using `git-appraise` with multiple forks can be found +[here](docs/forks.md). + ## Metadata The code review data is stored in [git-notes](https://git-scm.com/docs/git-notes), diff --git a/docs/forks.md b/docs/forks.md new file mode 100644 index 00000000..351ccf85 --- /dev/null +++ b/docs/forks.md @@ -0,0 +1,68 @@ +# Working with multiple forks + +This file documents how to use `git-appraise` with contributors who do not have +permission to push to the main repository. + +In this model, each contributor sets up their own personal fork of the +repository. They then push to their fork instead of the main repository. + +The list of these forks is stored inside the repository itself, in a special +ref. The tool uses that list to pull changes and reviews from all the +registered forks. + +## Adding a fork + +The owner of the main repository can add a fork with the following command, +followed by `git appraise push`: + +```shell +git appraise fork add [--owner ""]+ +``` + +## Using a fork to request a review + +The owner of a fork can request reviews for any branches they create as long as +the following rules hold: + +1. The name of the branch is prefixed with the name of their fork. +2. The committer email field of the commit to be reviewed matches + one of the owner email addresses of the fork. + +To request the review, run the usual `git appraise request ...` command, and +then use `git appraise push ...` to push it to the fork. + +## Using a fork to comment on reviews + +The owner of a fork can comment on any code review. The only requirement is +that the email address they use for git is listed as one of the owner email +addresses for the fork. + +## Pulling code reviews from forks + +When running `git appraise pull`, the tool will automatically fetch and merge +reviews from the forks. This behavior can be controlled using the +`--[no-]include-forks` flag. Alternatively, it can be configured on a +per-remote basis using the `appraise.remote..includeForks` setting. + +## Dealing with abusive fork owners + +If the owner of a fork is acting inappropriately, both the owner of the main +repository and the cloners of that repository have tools to exclude reviews and +comments from that abusive owner's fork: + +### Removing a fork + +The owner of the main repository can remove a fork by running the following +command, followed by `git appraise push`: + +```shell +git appraise fork remove +``` + +### Excluding a specific fork + +Anyone who runs `git appraise pull --include-forks` can configure a list of +forks to be excluded from the pull. This is controlled by the config settings +`appraise.remote..excludeFork` and `appraise.excludeFork`. The first +operates on a per-remote basis and uses either the fork name or URL. The second +applies to every remote and uses the fork's URL. diff --git a/docs/tutorial.md b/docs/tutorial.md index 6f95bb7e..f2548863 100644 --- a/docs/tutorial.md +++ b/docs/tutorial.md @@ -402,3 +402,12 @@ Now that our feature branch has been merged into master, we can delete it: git branch -d ${USER}/getting-started git push origin --delete ${USER}/getting-started ``` + +# Working with external contributors + +The simple workflow above shows how to use `git-appraise` in a single +repository. However, in practice you do not want to share a single repository +with every contributor. + +Instead, `git-appraise` supports the concept of "forks". Instructions on how to +use `git-appraise` with multiple forks can be found [here](forks.md) From a3b43085b453cd8fe9b2e67e711ed29ce48e9129 Mon Sep 17 00:00:00 2001 From: Omar Jarjur Date: Mon, 13 Aug 2018 18:16:28 -0700 Subject: [PATCH 02/37] Add skeletons for the various `git appraise fork ...` commands --- commands/commands.go | 28 +++++++++++ commands/commands_test.go | 50 ++++++++++++++++++++ commands/fork.go | 91 ++++++++++++++++++++++++++++++++++++ git-appraise/git-appraise.go | 17 ++----- 4 files changed, 173 insertions(+), 13 deletions(-) create mode 100644 commands/commands_test.go create mode 100644 commands/fork.go diff --git a/commands/commands.go b/commands/commands.go index 75b8c72d..5687c048 100644 --- a/commands/commands.go +++ b/commands/commands.go @@ -18,6 +18,8 @@ limitations under the License. package commands import ( + "fmt" + "github.com/google/git-appraise/repository" ) @@ -44,6 +46,9 @@ var CommandMap = map[string]*Command{ "abandon": abandonCmd, "accept": acceptCmd, "comment": commentCmd, + "fork add": addForkCmd, + "fork list": listForksCmd, + "fork remove": removeForkCmd, "list": listCmd, "pull": pullCmd, "push": pushCmd, @@ -53,3 +58,26 @@ var CommandMap = map[string]*Command{ "show": showCmd, "submit": submitCmd, } + +// FindSubcommand parses the subcommand from the list of arguments. +// +// The args parameter is the list of command line args after the program name. +// +// The return result are the matching command (if found), whether or not the +// command was found, and the list of remaining command line arguments that +// followed the subcommand. +func FindSubcommand(args []string) (*Command, bool, []string) { + if len(args) < 1 { + subcommand, ok := CommandMap["list"] + return subcommand, ok, []string{} + } + subcommand, ok := CommandMap[args[0]] + if ok { + return subcommand, ok, args[1:] + } + if len(args) > 1 { + subcommand, ok := CommandMap[fmt.Sprintf("%s %s", args[0], args[1])] + return subcommand, ok, args[2:] + } + return nil, false, []string{} +} diff --git a/commands/commands_test.go b/commands/commands_test.go new file mode 100644 index 00000000..7bb527da --- /dev/null +++ b/commands/commands_test.go @@ -0,0 +1,50 @@ +/* +Copyright 2018 Google Inc. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package commands + +import ( + "strings" + "testing" +) + +func TestFindSubcommandBuiltins(t *testing.T) { + for name, cmd := range CommandMap { + additionalArg := "foo" + matchingArgs := append(strings.Split(name, " "), additionalArg) + subcommand, ok, remainingArgs := FindSubcommand(matchingArgs) + if !ok { + t.Errorf("Failed to find the built-in subcommand %q", name) + } else if subcommand != cmd { + t.Errorf("Return the wrong subcommand for %q", name) + } else if len(remainingArgs) != 1 || remainingArgs[0] != additionalArg { + t.Errorf("Failed to return the remaining arguments for %q", name) + } + } +} + +func TestFindSubcommandEmpty(t *testing.T) { + subcommand, ok, remaining := FindSubcommand([]string{}) + if !ok { + t.Fatalf("Failed to return a default subcommand") + } + if subcommand != CommandMap["list"] { + t.Fatalf("Failed to return `list` as the default subcommand") + } + if len(remaining) != 0 { + t.Fatalf("Unexpected remaining arguments for an empty command: %q", remaining) + } +} diff --git a/commands/fork.go b/commands/fork.go new file mode 100644 index 00000000..afabb3d9 --- /dev/null +++ b/commands/fork.go @@ -0,0 +1,91 @@ +/* +Copyright 2018 Google Inc. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package commands + +import ( + "errors" + "flag" + "fmt" + + "github.com/google/git-appraise/repository" +) + +var ( + addForkFlagSet = flag.NewFlagSet("addFork", flag.ExitOnError) + addForkOwnerEmail = addForkFlagSet.String("owner-email", "", "Email address of the owner of the fork") +) + +// addFork updates the local git repository to include the specified fork. +func addFork(repo repository.Repo, args []string) error { + addForkFlagSet.Parse(args) + args = addForkFlagSet.Args() + + if len(args) < 2 { + return errors.New("The name and URL of the fork must be specified.") + } + if len(args) > 2 { + return errors.New("Only the name and URL of the fork may be specified.") + } + return errors.New("Not yet implemented.") +} + +// listForks lists the forks registered in the local git repository. +func listForks(repo repository.Repo, args []string) error { + return errors.New("Not yet implemented.") +} + +// removeFork updates the local git repository to no longer include the specified fork. +func removeFork(repo repository.Repo, args []string) error { + if len(args) < 1 { + return errors.New("The name of the fork must be specified.") + } + if len(args) > 1 { + return errors.New("Only the name of the fork may be specified.") + } + return errors.New("Not yet implemented.") +} + +// addForkCmd defines the `fork add` command. +var addForkCmd = &Command{ + Usage: func(arg0 string) { + fmt.Printf("Usage: %s fork add [