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

From: David Turner <dturner@twopensource.com>
Date: Nov 11, 2015, 20:15 UTC
Message-ID: <1447272954.20147.36.camel@twopensource.com>
In-Reply-To: <20151111094816.GA2949@salo>

On Wed, 2015-11-11 at 09:48 +0000, Richard Ipsum wrote:

Show 9 quoted lines

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

Show 8 quoted lines

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

Show 12 quoted lines

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

Show 7 quoted lines

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

Show 13 quoted lines

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