I didn't actually read the code. Instead, I started with the README and decided to provide both text and UX comments all mixed up. These are mostly my personal preferences; take them or leave them as you choose. I'm really excited about this tool and I think it's got great potential! On Tue, 2015-11-10 at 12:56 +0000, Richard Ipsum wrote: > Describes motivation for git-candidate and shows an example workflow. > > Signed-off-by: Richard Ipsum <richard.ipsum@xxxxxxxxxxxxxxx> > --- > contrib/git-candidate/README.md | 154 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 154 insertions(+) > create mode 100644 contrib/git-candidate/README.md > > diff --git a/contrib/git-candidate/README.md b/contrib/git-candidate/README.md > new file mode 100644 > index 0000000..d2d4437 > --- /dev/null > +++ b/contrib/git-candidate/README.md > @@ -0,0 +1,154 @@ > +git-candidate I have not heard the name "candidate" used this way. What about "git codereview"? > +============= > + > +git-candidate provides candidate review and patch tracking, > +it differs from other tools that provide this by storing _all_ > +content within git. > + > +## Why? I've made a few suggestions below that you might think are out of scope. If they are, it might be good to have a "non-goals" section so that people know what the scope of the tool is. > +Existing tools such as Github's pull-requests and Gerrit are already > +in wide use, why bother with something new? > + > +We who? > are concerned that whilst Today I learned: "whilst" can be used in the sense of "although" (I had previously thought only "while" could be used this way, but I was wrong! ) > git is a distributed version control > +system the systems used to store comments and reviews for content insert comma after "system" > +under version control are usually centralised, replace comma with period. > +git-candidate aims to solve this by storing > +all patch-tracking data in git proper. s/tracking/tracking and review/ ? Or something > +## Example review process > + > +### Contributor - Submits a candidate > + > + (hack hack hack) > + > + (feature)$ git commit -m "Add archived repo" > + (feature)$ git candidate create archivedrepo master > + -m "Add support for archived repo" > + Candidate archivedrepo created successfully. > + (feature)$ git candidate submit origin archivedrepo > + Candidate was submitted successfully. > +### Upstream - Reviews candidate What happens if a third party wants to review candidate? OR is this just the same as if upstream does it? > + (master)$ git candidate fetch origin > + (master)$ git candidate status origin/archiverepo > + Revision: 6239bd72d597357af901718becae91cee2a32b73 > + Ref: candidates/origin/archiverepo > + Status: active > + Land: master Could this be "Merge: master"? Or something that doesn't invent a new term? > + Add archived repo support > + > + lib/gitano/command.lua | 28 ++++++++++++++++++++++------ > + 1 file changed, 22 insertions(+), 6 deletions(-) > + > + (master)$ git show candidates/origin/archiverepo > + commit 2db28539c8fa7b81122382bcc526c6706c9e113a > + Author: Richard Ipsum <richard.ipsum@xxxxxxxxxxxxxxx> Probably better to use example.com addresses in the README rather than real people. Git traditionally uses "A U Thor" as the fake name. > + Date: Thu Oct 8 10:43:22 2015 +0100 > + > + Add support for archived repository masking in `ls` > + > + By setting `project.archived` to something truthy, a repository > + is thusly masked from `ls` output unless --all is passed in. > + > + Signed-off-by: Richard Ipsum <richard.ipsum@xxxxxxxxxxxxxxx> > + .... > + .... > + > + > + (master)$ git candidate review origin/archiverepo --vote -1 > + -m "Sorry, I'll need to see tests before I can accept this" Are per-line or per-commit comments supported? If so, please add an example of this. > + (master)$ git candidate submit origin archiverepo > + Review added successfully Is the contributor automatically (optionally) emailed on this? If not, consider this a feature request for this. > +### Contributor - Revises candidate > + > + (master)$ git candidate fetch origin > + (master)$ git candidate status origin/archiverepo > + Revision: 6239bd72d597357af901718becae91cee2a32b73 > + Ref: candidates/origin/archiverepo > + Status: active > + Land: master > + > + Add archived repo support > + > + lib/gitano/command.lua | 28 ++++++++++++++++++++++------ > + 1 file changed, 22 insertions(+), 6 deletions(-) > + > + -------------------------------------------------------------------------------- > + 1 review > + -------------------------------------------------------------------------------- > + > + Author: Emmet Hikory <persia@xxxxxxxxxxxx> > + Date: Tue Oct 13 10:09:45 2015 +0100 > + Vote: -1 > + > + Sorry, I'll need to see tests before I can accept this > + > + -------------------------------------------------------------------------------- > + > + (hack hack hack add tests) > + > + (feature_v2)$ git log --oneline -1 > + Ensure the `ls` yarn checks for archived repos > + > + (feature_v2)$ git candidate revise origin/archiverepo > + -m "Add archived repo support with tests" > + Candidate archiverepo revised successfully. > + > + (feature_v2)$ git candidate submit origin archiverepo > + Candidate was submitted successfully. > + > +### Upstream - Merges candidate > + > + (master)$ git candidate fetch origin > + (master)$ git candidate status origin/archiverepo > + Revision: 4cd3d1197d399005a713ca55f126a9086356a072 > + Ref: candidates/origin/archiverepo > + Status: active > + Land: master > + > + Add archived repo support with tests > + > + lib/gitano/command.lua | 28 ++++++++++++++++++++++------ > + testing/02-commands-ls.yarn | 19 +++++++++++++++++++ > + 2 files changed, 41 insertions(+), 6 deletions(-) "git candidate diff" might be nice too to show the diff between v1 and v2. You might even have "git candidate commit-diff" (or some better name) so you can see which commit has changed in a changeset containing multiple commits. > + (master)$ git candidate review origin/archiverepo --vote +2 > + -m "Looks good, merging. Thanks for your efforts" > + Review added successfully Is that +2 "+1 because I like it, +1 because I previously -1'd it?" If so, it might be nice to have --replace-vote so you don't have to track, "wait, I did -1, then +1, then -1 again..." > + (master)$ git candidate submit origin archiverepo > + Candidate was submitted successfully. I don't understand what the verb "submit" means here. Is it "mark this as accepted"? If so, "accept" might be a better word. > + (master)$ git merge candidates/origin/archiverepo I would like "git candidate merge" to do a submit+merge the way that pull does a fetch+merge. It seems like the common case. Also, if it turns out at this point that there's a merge conflict, I might want to back out the acceptance. > + (master)$ git push origin master > + > +### Contributor - Observes candidate has been accepted > + > + (feature_v2)$ git candidate fetch origin > + (feature_v2)$ git candidate status origin/archiverepo > + Revision: 4cd3d1197d399005a713ca55f126a9086356a072 > + Ref: candidates/origin/archiverepo > + Status: active > + Land: master > + > + Add archived repo support with tests > + > + lib/gitano/command.lua | 28 ++++++++++++++++++++++------ > + testing/02-commands-ls.yarn | 19 +++++++++++++++++++ > + 2 files changed, 41 insertions(+), 6 deletions(-) > + > + -------------------------------------------------------------------------------- > + 1 review > + -------------------------------------------------------------------------------- > + > + Author: Emmet Hikory <persia@xxxxxxxxxxxx> > + Date: Tue Oct 13 10:35:00 2015 +0100 > + Vote: +2 > + > + Looks good, merging. Thanks for your efforts > + > + -------------------------------------------------------------------------------- You should include here "git candidate remove archiverepo". And somewhere an example of "git candidate list". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html