Re: Code review tool? (and [PATCH] fix for stencil rotating)

[Reinhold Kainhofer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am Freitag, 12. September 2008 schrieb Evan Martin:
> On Thu, Sep 11, 2008 at 1:17 PM, Reinhold Kainhofer
>
> <address@hidden> wrote:
> >> The last argument to "git cl upload" is passed along to "git diff", so
> >> something like
> >>   git cl upload HEAD^^ HEAD^
> >> would upload just the patch one version back.
> >
> > Okay, that works, but (1) is awkward if you want to upload a patch a few
> > versions back (you basically have to lookup the hashes of both the commit
> > you want to upload and the one before) and (2) completely ignores the
> > commit message for that commit. After all, I already described my commit
> > in the commit message, so git-cl could/should reuse that.
>
> I've extended git-cl to use the commit messages for the description of
> the patch in the initial upload.  Please let me know how it works for
> you if you try it!

Hmm, somehow I can't seem to make it work with the latest version from git.
Ah, looking at the code it seems it only works when you don't have an issue in 
that branch already... If you try to upload an updated patch, git-cl will 
still ask you for a new description. I think in that case, it should also 
take the commit message and let you modify this (one might like to prepend 
something like "CORRECTED: ", but the commit message is still a good starting 
point).

BTW, I couldn't find a way to upload a second patch from the same branch as a 
new issue. Is there any possibility to do this, or can there only be one 
issue per branch? I would find the later quite unfortunate, because one patch 
might implement some small feature, while another patch might implement 
another huge, completely different feature, which however relies on the first 
feature, so it must be in the same git branch...

> > How about extending git-cl to allow uploading a single commit (not even
> > necessarily in the same branch as you are currently on)? Something like
> >      git-cl upload-commit HERECOMETHEHASHES
> > git-cl would then use the first line of the commit message as summary and
> > the rest as the long description... if multiple commits are given, they
> > are uploaded as separate patch sets to the same issue (or maybe as
> > separate issues?).
>
> That is a great idea!  I am a little frustrated that git diff and git
> log take slightly different syntaxes for specifying commits, because
> it would seem natural for the argument here to be the argument to "git
> log", as that has lots of supported syntax for specifying ranges of
> commits.
>
> Perhaps upload should take a flag like "--bomb" that means "don't
> upload this range of patches as one huge patch, but rather separately
> like git-send-email"?  Maybe that should be the default?

Yes, I think that should be the default (see below for some reasons). And I 
would name that option --squash and --no-squash.


> My git workflow is to do one feature in a branch at a time, with
> frequent sub-commits that have unhelpful info like "work in progress"
> as the commit message.  Once the branch is reviewed, I squash it into
> one commit.  Maybe my workflow is weird and shouldn't be the default.

My workflow is similar, although I usually don't create sub-commits, but merge 
my latest changes directly into the previous commit with git commit --amend.

However, I usually split my feature up into multiple small sub-features, which 
depend on each other (and thus have to be in the same branch). Each of the 
sub-features gets one atomic commit implementing only that small feature. Of 
course, later commits depend on previous ones. 
For reviewing, each of those small commits should be uploaded separately, 
because it's also much easier for a reviewer to review multiple almost 
trivial commits, than one huge diff where everything implementing different 
small features is mixed together.

That's the main reason why I'm pressing so much about the possibility to 
upload all commits in one branch separately.

> > Of course, this could even be extended to automatically upload all
> > commits in the current branch (starting at some commit that needs to be
> > further defined...)
> > That would feel much more git-like than the current "git-cl upload".
>
> Since upload works like git-diff, you can do
>   git cl upload base_branch
> to upload diffs since base_branch.  (Internally it effectively runs
> "git diff [whatever args you pass]".)
>
> Additionally, if you have an "upstream" branch (as connected with the
> --track in "git checkout --track -b mybranch upstream"), it uses that
> as the default target to diff against, so simply
>   git cl upload
> will do the right thing in that case.

Except that it will squash all features implemented in that branch into one 
huge patch rather than multiple small patches, which are easier to review.

Cheers,
Reinhold


- -- 
- ------------------------------------------------------------------
Reinhold Kainhofer, Vienna University of Technology, Austria
email: address@hidden, http://reinhold.kainhofer.com/
 * Financial and Actuarial Mathematics, TU Wien, http://www.fam.tuwien.ac.at/
 * K Desktop Environment, http://www.kde.org, KOrganizer maintainer
 * Chorvereinigung "Jung-Wien", http://www.jung-wien.at/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFIyjjwTqjEwhXvPN0RAnb8AKCS2Zl2SN4Ah83+WT9D6enQo/MWfgCgu32a
UHJ/nWuzhrS9ZJpBpk/OTN4=
=jTP8
-----END PGP SIGNATURE-----