Re: [PATCH 2/2] contrib/git-candidate: Add README

[Sebastian Schuberth]

On 10.11.2015 13:56, Richard Ipsum wrote:


+Existing tools such as Github's pull-requests and Gerrit are already
+in wide use, why bother with something new?
+
+We are concerned that whilst git is a distributed version control
+system the systems used to store comments and reviews for content
+under version control are usually centralised,


I think it's a bit unjust to unconditionally mention Gerrit in this 
context as you seem to imply that Gerrit does not store *any* review 
data in Git.


Even without Dave's upcoming notedb, Gerrit already stores refs/changes 
in Git, and with the reviewnotes plugin [1] also the outcome of a review 
in refs/notes/review.


[1] 
https://gerrit.googlesource.com/plugins/reviewnotes/+/refs/heads/master/src/main/resources/Documentation/refs-notes-review.md


--
Sebastian Schuberth

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] contrib/git-candidate: Add README

[Richard Ipsum]

On Tue, Nov 10, 2015 at 03:19:11PM -0500, David Turner wrote:
> 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!

It's great to hear that, I think there is a need for a tool like this.

> 
> On Tue, 2015-11-10 at 12:56 +, Richard Ipsum wrote:
> > Describes motivation for git-candidate and shows an example workflow.
> > 
[snip]
> 
> I have not heard the name "candidate" used this way.  What about "git
> codereview"? 

I admit to being quite bad at naming things,
originally we were going to call this git-pull-request after the
initial concept provided by Daniel Silverstone[1]. We later realised
that we'd created something more flexible than pull-requests:
git-candidate can be used with a pull-request model or a more tranditional
patch submission model.

I certainly have nothing against renaming this tool if there's some
agreement on a new name, though I will point out that it's possible
that the content of a candidate is not code.

> 
> > +=
> > +
> > +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?  

At the moment the 'we' is Codethink who are sponsoring this work,
but I'd like to resolve the text to not need that.

> 
> > 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"

ack

> 
> > +under version control are usually centralised,
> 
> replace comma with period.

ack

> 
> > +git-candidate aims to solve this by storing
> > +all patch-tracking data in git proper.
> 
> s/tracking/tracking and review/ ?  Or something

"all patch-tracking and review data in git proper" would probably be better.

> 
> > +## 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?

Exactly, the third party follows the same process as upstream.

> 
> > +   (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?

Consider it done. :)

> 
> > +   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 
> 
> Probably better to use example.com addresses in the README rather than
> real people.  Git traditionally uses "A U Thor" as the fake name.

Will do.

> 
> > +   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 
> > +   
> > +   
> > +
> > +
> > +   (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.

That's work in progress, there will soon be a --line option to the
'comment-file' command, the status command will then render per-line
comments.

> 
> > +   (master)$ git candidate submit origin archiverepo
> > +   Review added successfully
> 
> Is the contributor automatically (optionally) emailed on this? 

Re: [PATCH 2/2] contrib/git-candidate: Add README

[David Turner]

On Wed, 2015-11-11 at 09:48 +, Richard Ipsum wrote:
> > 
> > > + (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.
> 
> There's no server integration of any kind at the moment,
> this is clearly something we will want to add.

I don't think this needs server integration.  It could just work like
git send-email and send the email from the local machine.

> > "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. 
> 
> Yes, we definitely want that. I think "git candidate diff" to diff
> between revisions would be sufficient, and it could take a list of files
> to diff as an arg?

That's a good start, but often I want to review per-patch, so it would
be nice (if complicated) to track the evolution of a patchset.

> > > + (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..."
> 
> Votes are per-review, perhaps they should simply be per-revision?
> Then --vote sets the vote for the revision and there's no need for
> a --replace-vote option?
> This would use user.name and user.email as identification.

I like votes being per-revision.

> > > + (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.  
> 
> I'm tempted to change this to 'push', 'submit' comes from gerrit.

SGTM.

> > > + (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.
> 
> There is currently no git-candidate-merge, I removed this recently
> because I decided that you can merge candidates with git-merge
> and that this is more flexible. Often a candidate will be rebased
> before it is merged, it would be nice to avoid having to create
> a merge command that needs to handle all the different cases for
> merging a candidate.

I like the convenience, but it could always be added later.

One more random note: it might be nice to have a Documentation/technical
article describing review storage.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] contrib/git-candidate: Add README

[David Turner]

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 +, Richard Ipsum wrote:
> Describes motivation for git-candidate and shows an example workflow.
> 
> Signed-off-by: Richard Ipsum 
> ---
>  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 000..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 

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 
> + 
> + 
> +
> +
> + (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 
> + Date:   Tue Oct 13 10:09:45 2015 +0100
> 

[PATCH 2/2] contrib/git-candidate: Add README

[Richard Ipsum]

Describes motivation for git-candidate and shows an example workflow.

Signed-off-by: Richard Ipsum 
---
 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 000..d2d4437
--- /dev/null
+++ b/contrib/git-candidate/README.md
@@ -0,0 +1,154 @@
+git-candidate
+=
+
+git-candidate provides candidate review and patch tracking,
+it differs from other tools that provide this by storing _all_
+content within git.
+
+## Why?
+
+Existing tools such as Github's pull-requests and Gerrit are already
+in wide use, why bother with something new?
+
+We are concerned that whilst git is a distributed version control
+system the systems used to store comments and reviews for content
+under version control are usually centralised,
+git-candidate aims to solve this by storing
+all patch-tracking data in git proper.
+
+## 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
+
+   (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(-)
+
+   (master)$ git show candidates/origin/archiverepo
+   commit 2db28539c8fa7b81122382bcc526c6706c9e113a
+   Author: Richard Ipsum 
+   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 
+   
+   
+
+
+   (master)$ git candidate review origin/archiverepo --vote -1
+   -m "Sorry, I'll need to see tests before I can accept this"
+   (master)$ git candidate submit origin archiverepo
+   Review added successfully
+
+### 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 
+   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(-)
+
+   (master)$ git candidate review origin/archiverepo --vote +2
+   -m "Looks good, merging.  Thanks for your efforts"
+   Review added successfully
+
+   (master)$ git candidate submit origin archiverepo
+   Candidate was submitted successfully.
+
+   (master)$ git merge candidates/origin/archiverepo
+   (master)$ git push origin master
+

[PATCH 2/2] contrib/git-candidate: Add README

[Richard Ipsum]

Describes motivation for git-candidate and shows an example workflow.

Signed-off-by: Richard Ipsum 
---
 contrib/git-candidate/README.md | 153 
 1 file changed, 153 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 000..3291a12
--- /dev/null
+++ b/contrib/git-candidate/README.md
@@ -0,0 +1,153 @@
+git-candidate
+=
+
+git-candidate provides candidate review and patch tracking,
+it differs from other tools that provide this by storing _all_
+content within git.
+
+## Why?
+
+Existing tools such as Github's pull-requests and Gerrit are already
+in wide use, why bother with something new?
+
+We are concerned that whilst git is a distributed version control
+system the systems used to store comments and reviews for content
+under version control are usually centralised,
+git-candidate aims to solve this by storing
+all patch-tracking data in git proper.
+
+## 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 archivedrepo origin
+   Candidate was submitted successfully.
+
+### Upstream - Reviews candidate
+
+   (master)$ git candidate fetch origin
+   (master)$ git candidate status archiverepo origin
+   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(-)
+
+   (master)$ git show candidates/origin/archiverepo
+   commit 2db28539c8fa7b81122382bcc526c6706c9e113a
+   Author: Richard Ipsum 
+   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 
+   
+   
+
+
+   (master)$ git candidate review archiverepo origin --vote -1
+   -m "Sorry, I'll need to see tests before I can accept this"
+   Review added successfully
+
+### Contributor - Revises candidate
+
+   (master)$ git candidate fetch origin
+   (master)$ git candidate status archiverepo origin
+   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 
+   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 archiverepo origin
+   -m "Add archived repo support with tests"
+   Candidate archiverepo revised successfully.
+
+   (feature_v2)$ git candidate submit archiverepo origin
+   Candidate was submitted successfully.
+
+### Upstream - Merges candidate
+
+   (master)$ git candidate fetch origin
+   (master)$ git candidate status archiverepo origin
+   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(-)
+
+   (master)$ git candidate review archiverepo origin --vote +2
+   -m "Looks good, merging.  Thanks for your efforts"
+   Review added successfully
+
+   (master)$ git candidate submit archiverepo origin
+   Candidate was submitted successfully.
+
+   (master)$ git merge candidates/origin/archiverepo
+   (master)$ git push origin master
+
+### Contributor - Observes candidate has been accepted
+
+