Issue 2100: Explanation of branches for CG

Issue 2100: Explanation of branches for CG

Explain branches, by Graham Percival
Changes to git commands by Carl Sorensen

[Carl, 2012-01-14 22:56:32]

Here's a revision of Graham's info for the CG on branches.  I think it can go in
as-is.

Please review.

Thanks,

Carl

[Ian Hulin (gmail), 2012-01-15 01:53:14]

On 2012/01/14 22:56:32, Carl wrote:
> Here's a revision of Graham's info for the CG on branches.  I think it can go
in
> as-is.
> 
> Please review.
> 
> Thanks,
> 
> Carl

LGTM

Ian

[Graham Percival, 2012-01-15 08:05:35]

http://codereview.appspot.com/5539062/diff/1/Documentation/contributor/source...
File Documentation/contributor/source-code.itexi (right):

http://codereview.appspot.com/5539062/diff/1/Documentation/contributor/source...
Documentation/contributor/source-code.itexi:450: git rebase origin/staging
Problem: approximately once every 2 weeks, origin/staging is broken.  As a
result, we delete the existing origin/staging branch, test a subset of patches,
push them, etc etc.  You've seen the mess that happens.  With this recipe, the
broken-staging will be in the developer's personal dev/cg branch.

Is there any way we can avoid this?  The only way I've thought of so far is to
produce an *additional* staging-dev/cg branch from dev/cg, rebase staging-dev/cg
on origin/staging, then merge (or rebase) staging-dev/cg on staging and push
that.

I know it really complicates stuff, but given our history of bad commits in
staging, it would be sheer folly to assume that it will never happen again.

I still wish that there was some way of using staging for this purpose -- then
the developer would always know that dev/cg is his (unspoiled) work,
origin/staging is where he wants to put it, and staging is a temporary storage
location.

[Graham Percival, 2012-01-15 08:17:35]

http://codereview.appspot.com/5539062/diff/1/Documentation/contributor/source...
File Documentation/contributor/source-code.itexi (right):

http://codereview.appspot.com/5539062/diff/1/Documentation/contributor/source...
Documentation/contributor/source-code.itexi:491: git branch -D dev/cg
I know this is from what I wrote originally, but I've changed my mind: it's too
easy for somebody to copy&paste the wrong set of commands.

Could this entire @subsubheading be changed to read:


@c don't give explicit commands here, to avoid accidental copy&paste
Sometimes everything goes wrong.  If you want to remove a branch regardless of
losing your own work, follow the instructions in @qq{Delete your branch (safe)}
but replace the @code{-d} with a @code{-D} on the final line.

[Carl, 2012-01-15 12:18:57]

On 2012/01/15 08:05:35, Graham Percival wrote:
>
http://codereview.appspot.com/5539062/diff/1/Documentation/contributor/source...
> File Documentation/contributor/source-code.itexi (right):
> 
>
http://codereview.appspot.com/5539062/diff/1/Documentation/contributor/source...
> Documentation/contributor/source-code.itexi:450: git rebase origin/staging
> Problem: approximately once every 2 weeks, origin/staging is broken.  As a
> result, we delete the existing origin/staging branch, test a subset of
patches,
> push them, etc etc.  You've seen the mess that happens.  With this recipe, the
> broken-staging will be in the developer's personal dev/cg branch.
> 

I think you misunderstand what git rebase does.  git rebase origin/staging does
*not* bring origin/staging into dev/cg; it creates commits that are necessary to
get dev/cg from origin/staging.  If origin/staging is bad, and then deleted, and
then built again as a new origin/staging, the developer can get the appropriate
set of commits by just reissuing git rebase origin/staging.

It works the way you want it to work.  dev/cg *always* holds the pristine copy
of the developer's work.  git rebase origin/staging doesn't change dev/cg one
bit.  At any time I can do git rebase master and I'll get the set of commits
necessary to develop dev/cg from master.

The only time this fails is if two people have been working on the same lines of
the same file, in which case there will be a merge conflict, and it will need to
be resolved manually.  But there is nothing that can be done about this.  We
have to resolve the merge conflict when we do the rebase -- but it's not an
artifact of git, it's a real issue.

> 
> I still wish that there was some way of using staging for this purpose -- then
> the developer would always know that dev/cg is his (unspoiled) work,
> origin/staging is where he wants to put it, and staging is a temporary storage
> location.

Why do we need a temporary storage location? The only difference between staging
and origin/staging is a timing issue -- staging may be behind origin/staging
because changes have happened when I did git fetch.  But I can't push to
origin/staging until I rebase to origin/staging.

It works just the way you want it to out of the box.

When I went and gathered James's commits to push to staging, I didn't make *any*
changes to his work.  I just created a branch, applied his patch, rebased to
origin/staging, and pushed.  And it went seamlessly.

[Graham Percival, 2012-01-15 12:32:21]

On Sun, Jan 15, 2012 at 12:18:57PM +0000, Carl.D.Sorensen@gmail.com wrote:
> 
> I think you misunderstand what git rebase does.  git rebase
> origin/staging does *not* bring origin/staging into dev/cg;

really?  wow, I completely misunderstood that.

> If origin/staging is bad, and then deleted, and then built again
> as a new origin/staging, the developer can get the appropriate
> set of commits by just reissuing git rebase origin/staging.

ok, sounds great!

- Graham

[Carl, 2012-01-15 12:40:50]

On 2012/01/15 08:17:35, Graham Percival wrote:

> 
> Could this entire @subsubheading be changed to read:

Done

[dak, 2012-01-15 12:42:32]

On 2012/01/15 12:18:57, Carl wrote:
> On 2012/01/15 08:05:35, Graham Percival wrote:
> >
>
http://codereview.appspot.com/5539062/diff/1/Documentation/contributor/source...
> > File Documentation/contributor/source-code.itexi (right):
> > 
> >
>
http://codereview.appspot.com/5539062/diff/1/Documentation/contributor/source...
> > Documentation/contributor/source-code.itexi:450: git rebase origin/staging
> > Problem: approximately once every 2 weeks, origin/staging is broken.  As a
> > result, we delete the existing origin/staging branch, test a subset of
> patches,
> > push them, etc etc.  You've seen the mess that happens.  With this recipe,
the
> > broken-staging will be in the developer's personal dev/cg branch.
> > 
> 
> I think you misunderstand what git rebase does.

I am afraid that this honor is entirely on your side.

> git rebase origin/staging does
> *not* bring origin/staging into dev/cg; it creates commits that are necessary
to
> get dev/cg from origin/staging.

If dev/cg is the current branch, git rebase origin/staging _does_
bring origin/staging into dev/cg, and on top of that puts all those
commits that have been in dev/cg but not in origin/staging.
Afterwards, dev/cg is a proper descendant of origin/staging, and the
commits that have been in origin/staging are an integral part of it
indistinguishable from original dev/cg commits in itself.

> If origin/staging is bad, and then deleted, and
> then built again as a new origin/staging, the developer can get the
appropriate
> set of commits by just reissuing git rebase origin/staging.

But those commits that have been excised from origin/staging are now
in the set origin/staging..dev/cg and will in consequence get
_reapplied_ on top of the fresh origin/staging.  There are two ways
out.  The hairy one:

git rebase the-old-origin-staging-commit-id --onto origin/staging dev/cg

This will take just the commits in dev/cg that are on top of the old
origin/staging and apply them on top of the new origin/staging.  This
requires you to keep track of what commits are your own, and which
commits have sneaked into your branch from an excised staging.

The less hairy one is not to rebase dev/cg on staging ever.  Instead,
rebase on origin (which never gets rewound):

git rebase origin dev/cg

And if staging is _beyond_ origin, you can now do

git rebase origin/staging dev/cg~0

before testing and pushing.  Note that dev/cg~0 does _not_ rebase
dev/cg, but does the rebase operation on a "detached HEAD".  dev/cg
proper remains rebased on origin/master.

> It works the way you want it to work.  dev/cg *always* holds the
> pristine copy of the developer's work.  git rebase origin/staging
> doesn't change dev/cg one bit.

If dev/cg is the currently checked out branch, it most certainly will.

[Carl, 2012-01-15 12:43:03]

On 2012/01/15 12:32:21, Graham Percival wrote:
> On Sun, Jan 15, 2012 at 12:18:57PM +0000, mailto:Carl.D.Sorensen@gmail.com
wrote:
> > 
> > I think you misunderstand what git rebase does.  git rebase
> > origin/staging does *not* bring origin/staging into dev/cg;
> 
> really?  wow, I completely misunderstood that.

Yes.  git rebase just changes the *attachment point* of the branch.  So although
the source code in the current directory will change with git rebase, the branch
itself won't.

[dak, 2012-01-15 12:50:21]

On 2012/01/15 12:43:03, Carl wrote:
> On 2012/01/15 12:32:21, Graham Percival wrote:
> > On Sun, Jan 15, 2012 at 12:18:57PM +0000, mailto:Carl.D.Sorensen@gmail.com
> wrote:
> > > 
> > > I think you misunderstand what git rebase does.  git rebase
> > > origin/staging does *not* bring origin/staging into dev/cg;
> > 
> > really?  wow, I completely misunderstood that.
> 
> Yes.  git rebase just changes the *attachment point* of the branch.  So
although
> the source code in the current directory will change with git rebase, the
branch
> itself won't.

Carl, you are wrong.  You can, at your choice, read the manual page of git
rebase, or do a few experiments on local branches.

git rebase most certainly brings the commits of the specified branch into the
checked-out branch, and puts the non-common commits in the checked-out branch on
top.  There is no way that the commits will get out again by a pure rebase
(without --onto option), so if the commits get removed from origin/staging for
some manner, they most certainly will stay in dev/cg if that has been rebased on
the old origin/staging.

[Carl, 2012-01-15 12:53:52]

On 2012/01/15 12:50:21, dak wrote:
> On 2012/01/15 12:43:03, Carl wrote:
> > On 2012/01/15 12:32:21, Graham Percival wrote:
> > > On Sun, Jan 15, 2012 at 12:18:57PM +0000, mailto:Carl.D.Sorensen@gmail.com
> > wrote:
> > > > 
> > > > I think you misunderstand what git rebase does.  git rebase
> > > > origin/staging does *not* bring origin/staging into dev/cg;
> > > 
> > > really?  wow, I completely misunderstood that.
> > 
> > Yes.  git rebase just changes the *attachment point* of the branch.  So
> although
> > the source code in the current directory will change with git rebase, the
> branch
> > itself won't.
> 
> Carl, you are wrong.  You can, at your choice, read the manual page of git
> rebase, or do a few experiments on local branches.

I believe you, because you are never wrong on these things.  I have read the
manual page on git rebase, but apparently didn't understand it.  I am currently
doing the local experiments.

Thank you for keeping me straight.

Carl

[Carl, 2012-01-15 13:37:13]

After doing some testing, it appears that the following should be able to get my
dev/cg applied to origin/staging, while preserving my dev/cg:


git rebase origin/staging dev/cg~0
git push origin HEAD:staging

David, is this correct?

Thanks,

Carl

[dak, 2012-01-15 14:54:22]

On 2012/01/15 13:37:13, Carl wrote:
> After doing some testing, it appears that the following should be able to get
my
> dev/cg applied to origin/staging, while preserving my dev/cg:
> 
> 
> git rebase origin/staging dev/cg~0
> git push origin HEAD:staging
> 
> David, is this correct?

As far as my painfully gained understanding goes, yes.  If your dev/cg gets too
far behind origin/staging, the rebase might require manual work.  It may be
possible to reduce the repetitive amount of work by working with git rerere
(with which I have no experience), but an occasional

git rebase origin dev/cg

(which permanently rebases dev/cg on origin/master which we don't reset ever by
gentlemen's agreement, meaning that if somebody does that, everybody else stops
being a gentleman) should take care that most of the work needs to be done just
once.

[Keith, 2012-01-15 19:47:10]

LGTM 

On 2012/01/15 08:05:35, Graham Percival wrote:
> With this recipe, the
> broken-staging will be in the developer's personal dev/cg branch.
> 
> Is there any way we can avoid this?  

1) We could accept it as a consequence of re-writing the public history on
origin/staging.  When the developer does his next rebase -i he should recognize
that some commits are not his, and remove them from the rebase  --or test them
and fix the problem that forced the rewind of staging.

2) We could introduce both 'merge' and 'rebase' in the guide: 
 git checkout staging
 git reset --hard origin/staging # reset local staging to the repository
 git merge dev/cg               # bring your new commits into local staging
 git rebase -i origin/staging # rewrite local history to put the new commits
last
 gitk                 # check that the commits look as you intended
 git push origin/staging

On 2012/01/15 13:37:13, Carl wrote:
> 
> git rebase origin/staging dev/cg~0
> git push origin HEAD:staging

3) The example above is certainly shorter. I find it difficult to understand. It
implicitly checks out the last commit in dev/cg (leaving git not on any branch).
I would not have guessed that this would be supported, based on `man git
rebase`.  Then the command rebases that non-on-any-branch history to
origin/staging and pushes that history to the repository.  It leaves git in the
state of being not on any branch.

I think (2) is the best example for the guide.

[Carl, 2012-01-15 23:12:49]

On 2012/01/15 19:47:10, Keith wrote:
> LGTM 
> 
> On 2012/01/15 08:05:35, Graham Percival wrote:
> > With this recipe, the
> > broken-staging will be in the developer's personal dev/cg branch.
> > 
> > Is there any way we can avoid this?  
> 
> 1) We could accept it as a consequence of re-writing the public history on
> origin/staging.  When the developer does his next rebase -i he should
recognize
> that some commits are not his, and remove them from the rebase  --or test them
> and fix the problem that forced the rewind of staging.

But we should not have all developers have to suffer from one developer's
mistake on staging, when there is an easy way around it.

> 
> 2) We could introduce both 'merge' and 'rebase' in the guide: 
>  git checkout staging
>  git reset --hard origin/staging # reset local staging to the repository
>  git merge dev/cg               # bring your new commits into local staging
>  git rebase -i origin/staging # rewrite local history to put the new commits
> last
>  gitk                 # check that the commits look as you intended
>  git push origin/staging

This would work, as long as the developer is completely aware of which commits
are his and which are not.  As long as the developer has abided by the "one
commit per patch" rule, it's easy.

> 
> On 2012/01/15 13:37:13, Carl wrote:
> > 
> > git rebase origin/staging dev/cg~0
> > git push origin HEAD:staging
> 
> 3) The example above is certainly shorter. I find it difficult to understand.
It
> implicitly checks out the last commit in dev/cg (leaving git not on any
branch).
> I would not have guessed that this would be supported, based on `man git
> rebase`.  Then the command rebases that non-on-any-branch history to
> origin/staging and pushes that history to the repository.  It leaves git in
the
> state of being not on any branch.

Yes, but I wouldn't leave it in that state.  The full set of commands is

git rebase origin/staging dev/cg~0
git push origin HEAD:staging
git checkout dev/cg

At this point, you have pushed dev/cg to staging without polluting dev/cg with
staging.  And you can continue to rebase dev/cg with master as needed/desired. 
You never have to worry about accidentally putting some reverted commit back
into staging.  It's completely foolproof.

The examples for the guide are intended to be copy and pastable, not necessarily
to teach how to use git in general.  Working commands that do what we want
should be preferred to commands that require a developer to follow through some
steps and make it right.

The way I've actually recovered commits from broken staging in the past is with
cherry-picks.  But those weren't my commits.  Even if they were mine, probably a
new branch and cherry-picking off the broken staging is easier/cleaner than
rebase -i.

But the incomprehensible set of commands works perfectly on a detached head, and
leaves the rebased tree in garbage collection where it will eventually be
automatically eliminated, which is the appropriate place.  I think it is the
right set of commands.

Thanks,

Carl


> 
> I think (2) is the best example for the guide.

[Carl, 2012-01-15 23:15:01]

On 2012/01/15 14:54:22, dak wrote:

> An occasional
> 
> git rebase origin dev/cg
> 
> (which permanently rebases dev/cg on origin/master which we don't reset ever
by
> gentlemen's agreement, meaning that if somebody does that, everybody else
stops
> being a gentleman) should take care that most of the work needs to be done
just
> once.

My thoughts (almost) exactly.  My set of commands would be

git checkout dev/cg
git rebase origin/master

which does exactly the same thing, only with different words.

[dak, 2012-01-16 07:37:44]

On 2012/01/15 23:15:01, Carl wrote:
> On 2012/01/15 14:54:22, dak wrote:

> > git rebase origin dev/cg
> 
> My thoughts (almost) exactly.  My set of commands would be
> 
> git checkout dev/cg
> git rebase origin/master
> 
> which does exactly the same thing, only with different words.

I think it reaches the same work directory state, but possibly touching fewer
files.  If you have recently worked with something closer to master than dev/cg,
that might save rebuild time.

[Graham Percival, 2012-01-16 07:54:41]

On Sun, Jan 15, 2012 at 11:12:49PM +0000, Carl.D.Sorensen@gmail.com wrote:
> Yes, but I wouldn't leave it in that state.  The full set of commands is
> 
> git rebase origin/staging dev/cg~0
> git push origin HEAD:staging
> git checkout dev/cg

LGTM

> At this point, you have pushed dev/cg to staging without polluting
> dev/cg with staging.  And you can continue to rebase dev/cg with master
> as needed/desired.  You never have to worry about accidentally putting
> some reverted commit back into staging.  It's completely foolproof.

+1

> The examples for the guide are intended to be copy and pastable, not
> necessarily to teach how to use git in general.  Working commands that
> do what we want should be preferred to commands that require a developer
> to follow through some steps and make it right.

+10

- Graham

[Carl, 2012-01-16 16:16:40]

On 2012/01/16 07:54:41, Graham Percival wrote:
> On Sun, Jan 15, 2012 at 11:12:49PM +0000, mailto:Carl.D.Sorensen@gmail.com
wrote:
> > At this point, you have pushed dev/cg to staging without polluting
> > dev/cg with staging.  And you can continue to rebase dev/cg with master
> > as needed/desired.  You never have to worry about accidentally putting
> > some reverted commit back into staging.  It's completely foolproof.

Having just spent half an hour fixing up my lilygit.tcl branch, I have
personally validated the benefit of the approach now reflected in this patch.

I will *never* again rebase a working branch to origin/staging.  In fact, I will
never again have a local branch staging.

Instead, I will only rebase my working branches to origin/master, and do a
detached head rebase to origin/staging for pushing purposes.  I assume this will
save me time periodically.  And although the time saved may not be huge, the
aggravation saved will  be.

Thanks for getting me to work on this, Graham.  The time has now been all worth
it.

[mail_philholmes.net, 2012-01-16 16:26:10]

----- Original Message ----- 
From: <Carl.D.Sorensen@gmail.com>
To: <graham@percival-music.ca>; <k-ohara5a5a@oco.net>; 
<janek.lilypond@gmail.com>; <ianhulin44@gmail.com>; <dak@gnu.org>
Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Monday, January 16, 2012 4:16 PM
Subject: Re: Issue 2100: Explanation of branches for CG (issue 5539062)


> On 2012/01/16 07:54:41, Graham Percival wrote:
>> On Sun, Jan 15, 2012 at 11:12:49PM +0000,
> mailto:Carl.D.Sorensen@gmail.com wrote:
>> > At this point, you have pushed dev/cg to staging without polluting
>> > dev/cg with staging.  And you can continue to rebase dev/cg with
> master
>> > as needed/desired.  You never have to worry about accidentally
> putting
>> > some reverted commit back into staging.  It's completely foolproof.
>
> Having just spent half an hour fixing up my lilygit.tcl branch, I have
> personally validated the benefit of the approach now reflected in this
> patch.
>
> I will *never* again rebase a working branch to origin/staging.  In
> fact, I will never again have a local branch staging.
>
> Instead, I will only rebase my working branches to origin/master, and do
> a detached head rebase to origin/staging for pushing purposes.  I assume
> this will save me time periodically.  And although the time saved may
> not be huge, the aggravation saved will  be.
>
> Thanks for getting me to work on this, Graham.  The time has now been
> all worth it.
>
>
> http://codereview.appspot.com/5539062/

Based on my normal fear of git, I always adopt the same routine for pushing, 
which I derived from David's excellent guidance.:

Before you start thinking about pushing a patch to staging, you
need to ensure you have the correct local branches up to date.
One time only, edit the .git/config file to look like this (there
will be other lines and sections, don't touch these):

@example
[remote "origin"]
 fetch = +refs/heads/*:refs/remotes/origin/*
 url = ssh://git.sv.gnu.org/srv/git/lilypond.git
@end example

Now, each time you want to make a change and push to staging, do
the following:

@example
git fetch # (to be sure you have the current version of staging)
git checkout origin/staging
# ... make and commit your change ... e.g. with git am patchname
git push origin HEAD:staging
@end example

Note that this does not work well for complex changes on other
branches - if you're doing this you'll need to have better git
understanding.  However, "make your change" could include applying
a patch previously you've made.



--
Phil Holmes

[dak, 2012-01-16 16:33:38]

On 2012/01/16 16:16:40, Carl wrote:
> 
> Having just spent half an hour fixing up my lilygit.tcl branch, I
> have personally validated the benefit of the approach now reflected
> in this patch.


> I will *never* again rebase a working branch to origin/staging.  In
> fact, I will never again have a local branch staging.

Well, YMMV.  I get by with repairing things from time to time via
git rebase --onto ...
and so I do rebase to origin/staging sometimes in order to get testable results
without having to take care of not losing work done on a detached branch when
going elsewhere.  It does mean you need to be careful about what you commit, and
occasionally high-octane repair work (git has the tools for that, but it is not
exactly straightforward).  But what I indeed don't use is a local branch
staging.  Maintaining it in a useful and consistent state is more trouble than
it is worth to me.

> Instead, I will only rebase my working branches to origin/master, and do a
> detached head rebase to origin/staging for pushing purposes.  I assume this
will
> save me time periodically.  And although the time saved may not be huge, the
> aggravation saved will  be.
> 
> Thanks for getting me to work on this, Graham.  The time has now been all
worth
> it.

"I survived LilyPond development" sounds like a good thing to put on your
resume.

[c_sorensen, 2012-01-16 16:40:02]

On 1/16/12 9:26 AM, "Phil Holmes" <mail@philholmes.net> wrote:

>
>Before you start thinking about pushing a patch to staging, you
>need to ensure you have the correct local branches up to date.
>One time only, edit the .git/config file to look like this (there
>will be other lines and sections, don't touch these):
>
>@example
>[remote "origin"]
> fetch = +refs/heads/*:refs/remotes/origin/*
> url = ssh://git.sv.gnu.org/srv/git/lilypond.git
>@end example
>
>Now, each time you want to make a change and push to staging, do
>the following:
>
>@example
>git fetch # (to be sure you have the current version of staging)
>git checkout origin/staging
># ... make and commit your change ... e.g. with git am patchname
>git push origin HEAD:staging
>@end example
>
>Note that this does not work well for complex changes on other
>branches - if you're doing this you'll need to have better git
>understanding.  However, "make your change" could include applying
>a patch previously you've made.

This set of instructions works great if you are maintaining your work in
patches.  Getting the latest staging and then applying patches to it
before pushing will always work.

Of course if your patches are not based on staging, then they may not
apply.

The current instructions on working with branches in the cg are ideal for
working with  a staging that may be destroyed at any time.  I think that
they are sound, and can be effectively used by anybody.

Although Graham may not appreciate it, I think I may modify lilygit.tcl to
create an advanced mode that readily supports switching branches, posting
for review, and pushing to staging.

Thanks,

Carl

[Graham Percival, 2012-01-16 16:53:19]

On Mon, Jan 16, 2012 at 04:26:04PM -0000, Phil Holmes wrote:
> @example
> [remote "origin"]
> fetch = +refs/heads/*:refs/remotes/origin/*
> url = ssh://git.sv.gnu.org/srv/git/lilypond.git
> @end example

This is precisely what we should avoid.  Explaining how to modify
.git/config by hand is tedious, complicated, and will almost
always lead to questions and getting "talked through" the changes.
We've seen this at least 4 times so far on -devel.

If there is *any* way of making a recipe that works "out of the
box" starting from a simple git clone, we should use that.  Manual
modifications to .git/config should be an absolute last resort;
AFAIK this is only needed once when somebody gets ssh push
ability.

> Note that this does not work well for complex changes on other
> branches - if you're doing this you'll need to have better git
> understanding.  However, "make your change" could include applying
> a patch previously you've made.

The new instructions really push (no pun intended) the use of
branches, even for a single small doc-typo fix.  We're still
simplifying and streamlining those instructions, but once they're
done I really think they will make everybody's life much simpler.

- Graham

[janek, 2012-01-17 20:24:34]

Some thoughts on making all this less confusing to beginners.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
File Documentation/contributor/source-code.itexi (right):

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:291: @code{dev/}.  Branch names that
don't begin with @code{dev/} are
could we change this (and other similar) prefix so that it doesn't contain a
slash?  I mean, change dev/ to dev- or something like that.
The slash confused me a lot, because it's also used to separate a remote name
from the branch name, like in origin/master.  I'm sure that if we will adopt
"dev/blahblah" naming, many people will mistakenly believe that "dev" is
something like "origin", and they will be very confused.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:297: git branch dev/cg
I think it would be good to be verbose, because it will give people more
information about using git (and they won't have to ask certain questions).  In
this case i would suggest 

git branch dev/cg --track origin/master

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:318: @qq{profit}, I mean @qq{push
stuff to staging}.
i think this fragment is irrelevant here.  It confuses me.

I'd write something like 
"You can switch to your local branches and to the remote branches as well"
instead.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:345: Add a file, then commit it:
I'd write
"by default, git commit -a only commits changes to the files that existed before
you made your changes.  If you want to include a file created by you in the
commit, use git add:"

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:360: @subsubheading Save commits
manually (optional)
I suggest changing this to 
"save commits to external files" or sth. like that.
Now it looks like this section will be about doing what was described above by
hand (i.e. fiddling with git internals)

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:362: Branches are nerve-wracking
until you get used to them.  You can
Insert "After you committed your changes, you can..."

(format-patch doesn't work with uncommitted changes)

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:377: Update your branch with the
latest master:
Maybe we should mention that there shouldn't be any uncommitted changes before
contributor tries to checkout master?  IIRC, when someone is on some branch,
makes changes but doesn't commit them, and checks out another branch, these
changes will "follow him" to this branch.  And with a dirty tree he won't be
able to pull.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:396: Make sure that you're on your
branch, then upload:
"Make sure that you're on your branch and upload:"

I think this won't make an impression that checkout command is a part of actual
uploading process.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:413: @subsubheading Rewrite history
(optional unless you have broken commits)
i don't think this title is clear (what history is it and how can i tell if i
have broken commits?)

Maybe "rearranging commits"?

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:463: If everything looks good, push
it:
maybe "push it to the staging branch located on remote origin"?  This will give
contributor more information about what he is doing (= less questions to
answer).

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:475: @warning{It is a best practice
to never rebase one of your branches
"never rebase your branches"?

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:480: changing your working branch.}
Maybe add
"(notice ~0 added to rebase command)"

[Graham Percival, 2012-01-17 20:31:24]

On Tue, Jan 17, 2012 at 08:24:35PM +0000, janek.lilypond@gmail.com wrote:
> could we change this (and other similar) prefix so that it doesn't
> contain a slash?  I mean, change dev/ to dev- or something like that.
> The slash confused me a lot, because it's also used to separate a remote
> name from the branch name, like in origin/master.  I'm sure that if we
> will adopt "dev/blahblah" naming, many people will mistakenly believe
> that "dev" is something like "origin", and they will be very confused.

Good point!  I like it.

>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> Documentation/contributor/source-code.itexi:297: git branch dev/cg
> I think it would be good to be verbose, because it will give people more
> information about using git (and they won't have to ask certain
> questions).  In this case i would suggest
> 
> git branch dev/cg --track origin/master

But we don't want it to track origin/master, do we?  People should
merge from master manually (covered in this section).

>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> Documentation/contributor/source-code.itexi:318: @qq{profit}, I mean
> @qq{push stuff to staging}.
> i think this fragment is irrelevant here.  It confuses me.

I don't mind removing it...

> I'd write something like
> "You can switch to your local branches and to the remote branches as
> well"
> instead.

... but *this* confuses me.  How can git switch to a remote
branch?  Aren't all branches local?  I mean, whenever you switch
to a "remote" branch, doesn't that just create a local copy of the
remote branch, then put you on that local branch?

>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> Documentation/contributor/source-code.itexi:345: Add a file, then commit
> it:
> I'd write
> "by default, git commit -a only commits changes to the files that
> existed before you made your changes.  If you want to include a file
> created by you in the commit, use git add:"

That's much more verbose, and it only affects 1% of git usage.
There's certainly a better wording than what is written currently,
but I don't think this is it.

>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> Documentation/contributor/source-code.itexi:360: @subsubheading Save
> commits manually (optional)
> I suggest changing this to
> "save commits to external files" or sth. like that.

Good idea!

>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> Documentation/contributor/source-code.itexi:362: Branches are
> nerve-wracking until you get used to them.  You can
> Insert "After you committed your changes, you can..."
> 
> (format-patch doesn't work with uncommitted changes)

+1

>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> Documentation/contributor/source-code.itexi:463: If everything looks
> good, push it:
> maybe "push it to the staging branch located on remote origin"?  This
> will give contributor more information about what he is doing (= less
> questions to answer).

I think that mentioning "local" or "remote" will only add
confusion.  The goal is not to explain how to use git; the goal is
to let people use git as painlessly as possible.

- Graham

[janek, 2012-01-17 21:01:21]

2012/1/17 Graham Percival <graham@percival-music.ca>:
>>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
>> Documentation/contributor/source-code.itexi:297: git branch dev/cg
>> I think it would be good to be verbose, because it will give people more
>> information about using git (and they won't have to ask certain
>> questions).  In this case i would suggest
>>
>> git branch dev/cg --track origin/master
>
> But we don't want it to track origin/master, do we?  People should
> merge from master manually (covered in this section).

If i understand git manual correctly, --track only tells git from
which remote branch it should pull.  It doesn't tell git to pull
automatically.
I've created a branch with --track, i'll see if anything happens to it
automatically.

>> I'd write something like
>> "You can switch to your local branches and to the remote branches as
>> well"
>> instead.
>
> ... but *this* confuses me.  How can git switch to a remote
> branch?  Aren't all branches local?  I mean, whenever you switch
> to a "remote" branch, doesn't that just create a local copy of the
> remote branch, then put you on that local branch?

Yes, i think it works like that (and still these are called "remote
branches").  My wording was misleading.
More about remote branches here http://progit.org/book/ch3-5.html
The first paragraph gives a very nice explanation.

cheers,
Janek

[janek, 2012-01-17 21:31:01]

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
File Documentation/contributor/source-code.itexi (right):

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:297: git branch dev/cg
There is one possible downside of tracking master explicitly (if i understand
correctly how this works): if someone makes a mistake and calls 'git push' while
being on local branch which tracks master, this would result in commits pushed
to master.

[dak, 2012-01-18 10:37:02]

On 2012/01/17 21:31:01, janek wrote:
>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> File Documentation/contributor/source-code.itexi (right):
> 
>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> Documentation/contributor/source-code.itexi:297: git branch dev/cg
> There is one possible downside of tracking master explicitly (if i understand
> correctly how this works): if someone makes a mistake and calls 'git push'
while
> being on local branch which tracks master, this would result in commits pushed
> to master.

Not quite.  "git push" pushes all tracking branches with a name that is the same
in the local repository as well as in the remote repository.

So if you have a local branch "master" tracking origin/master, this will get
pushed upstream when saying "git push" even if the currently checked-out branch
is a different one.

I only push with explicit branch specifications for that reason.  Got bitten
more than once.

[Jean-Charles, 2012-01-18 18:31:51]

Le 17/01/2012 21:31, Graham Percival disait :
> On Tue, Jan 17, 2012 at 08:24:35PM +0000, janek.lilypond@gmail.com wrote:
>> could we change this (and other similar) prefix so that it doesn't
>> contain a slash?  I mean, change dev/ to dev- or something like that.
>> The slash confused me a lot, because it's also used to separate a remote
>> name from the branch name, like in origin/master.  I'm sure that if we
>> will adopt "dev/blahblah" naming, many people will mistakenly believe
>> that "dev" is something like "origin", and they will be very confused.
>
> Good point!  I like it.
>

Thanks to git-completion, I don't have to type the 20 characters when 
switching off/to our translators' branch... and got used to it.


Cheers,
Jean-Charles

[Carl, 2012-01-19 16:07:59]

On 2012/01/17 20:31:24, Graham Percival wrote:
> On Tue, Jan 17, 2012 at 08:24:35PM +0000, mailto:janek.lilypond@gmail.com
wrote:
> > could we change this (and other similar) prefix so that it doesn't
> > contain a slash?  I mean, change dev/ to dev- or something like that.
> > The slash confused me a lot, because it's also used to separate a remote
> > name from the branch name, like in origin/master.  I'm sure that if we
> > will adopt "dev/blahblah" naming, many people will mistakenly believe
> > that "dev" is something like "origin", and they will be very confused.
> 
> Good point!  I like it.
> 

I don't.  We have / as a directory indicator in file names, and we have a whole
bunch of branches on git that use / as the equivalent thing -- a grouping of
like branches (see, for example, stable/2.12, stable/2.14, etc.

The / is perfectly consistent as a separator, and it works very similar to / in
unix.  dev/cg is a local branch dev/cg; origin/dev/cg is the remote branch on
origin.

I'd be fine with eliminating the separator, and just calling the branch
local-working (for example).  But at the origin of this all, Graham wanted
people used to putting stuff in dev/*, since origin/dev/* is where individual
users store there work on the public repository if they want to.  Changing this
to origin/dev- would be a mistake, in my opinion.

>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> > Documentation/contributor/source-code.itexi:297: git branch dev/cg
> > I think it would be good to be verbose, because it will give people more
> > information about using git (and they won't have to ask certain
> > questions).  In this case i would suggest
> > 
> > git branch dev/cg --track origin/master
> 
> But we don't want it to track origin/master, do we?  People should
> merge from master manually (covered in this section).
>

We absolutely do *not* want tracking, in my opinon.  The only branch I want
tracking in a novice's repository is master.  And we give instructions that they
never merge to master -- only to staging.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> > Documentation/contributor/source-code.itexi:318: @qq{profit}, I mean
> > @qq{push stuff to staging}.
> > i think this fragment is irrelevant here.  It confuses me.
> 
> I don't mind removing it...

I'll come up with some other wording.  I liked the light-hearted words, but can
see that non-native speakers would miss the idiom.

> 
> > I'd write something like
> > "You can switch to your local branches and to the remote branches as
> > well"
> > instead.
> 
> ... but *this* confuses me.  How can git switch to a remote
> branch?  Aren't all branches local?  I mean, whenever you switch
> to a "remote" branch, doesn't that just create a local copy of the
> remote branch, then put you on that local branch?

This isn't a git tutorial, but a using git with lilypond tutorial.  I'll add
something like
"You can create a local branch with your work on a particular issue to
facilitate patching, review, and merging with the main repository."
> 
> >
>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> > Documentation/contributor/source-code.itexi:345: Add a file, then commit
> > it:
> > I'd write
> > "by default, git commit -a only commits changes to the files that
> > existed before you made your changes.  If you want to include a file
> > created by you in the commit, use git add:"
> 
> That's much more verbose, and it only affects 1% of git usage.
> There's certainly a better wording than what is written currently,
> but I don't think this is it.

I'll think about it and propose changes.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> > Documentation/contributor/source-code.itexi:360: @subsubheading Save
> > commits manually (optional)
> > I suggest changing this to
> > "save commits to external files" or sth. like that.
> 
> Good idea!

Will make the change.

> 
> >
>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> > Documentation/contributor/source-code.itexi:362: Branches are
> > nerve-wracking until you get used to them.  You can
> > Insert "After you committed your changes, you can..."
> > 
> > (format-patch doesn't work with uncommitted changes)
> 
> +1
>

Will fix.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> > Documentation/contributor/source-code.itexi:463: If everything looks
> > good, push it:
> > maybe "push it to the staging branch located on remote origin"?  This
> > will give contributor more information about what he is doing (= less
> > questions to answer).
> 
> I think that mentioning "local" or "remote" will only add
> confusion.  The goal is not to explain how to use git; the goal is
> to let people use git as painlessly as possible.

IMO, the goal is to let people use git *to work on lilypond* as painlessly as
possible.  So it's fair (and desirable) to put in specifics for the organization
of our repositories.

Thanks,

Carl

[janek, 2012-01-20 00:19:33]

On 2012/01/19 16:07:59, Carl wrote:
> On 2012/01/17 20:31:24, Graham Percival wrote:
> > On Tue, Jan 17, 2012 at 08:24:35PM +0000, mailto:janek.lilypond@gmail.com
> wrote:
> > > could we change this (and other similar) prefix so that it doesn't
> > > contain a slash?  I mean, change dev/ to dev- or something like that.
> > > The slash confused me a lot, because it's also used to separate a remote
> > > name from the branch name, like in origin/master.  I'm sure that if we
> > > will adopt "dev/blahblah" naming, many people will mistakenly believe
> > > that "dev" is something like "origin", and they will be very confused.
> > 
> > Good point!  I like it.
> > 
> 
> I don't.  We have / as a directory indicator in file names, and we have a
whole
> bunch of branches on git that use / as the equivalent thing -- a grouping of
> like branches (see, for example, stable/2.12, stable/2.14, etc.
> 
> The / is perfectly consistent as a separator, and it works very similar to /
in
> unix.  dev/cg is a local branch dev/cg; origin/dev/cg is the remote branch on
> origin.

If we were using origin everywhere and it would be discernible from context what
does what, i would agree.  But it's not like that as far as i see.

On 2012/01/19 16:07:59, Carl wrote:
>
http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
> > > Documentation/contributor/source-code.itexi:297: git branch dev/cg
> > > I think it would be good to be verbose, because it will give people
> > > more information about using git (and they won't have to ask
> > > certain questions).  In this case i would suggest
> > > 
> > > git branch dev/cg --track origin/master
> > 
> > But we don't want it to track origin/master, do we?  People should
> > merge from master manually (covered in this section).
> >
> 
> We absolutely do *not* want tracking, in my opinon.  The only branch I want
> tracking in a novice's repository is master.  And we give instructions that
they
> never merge to master -- only to staging.

Just to make sure we understood ourselves - i'm not suggesting that anyone
merges his master branch with anything.  I'm talking about pulling from master. 
Usually that's the default, but occasionally it won't work and this tracking
setting would prevent problems, bt it may not be worth the fuss.

[Carl, 2012-01-20 05:36:50]

I've responded to comments.  Thanks for all the input.

Carl

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
File Documentation/contributor/source-code.itexi (right):

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:291: @code{dev/}.  Branch names that
don't begin with @code{dev/} are
As I said before, I like the /.  I have added text to describe the special
meaning of branches beginning with origin/.  I think that will minimize
confusion.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:297: git branch dev/cg
I have made the decision not to add the tracking.  I want rebasing to be done
manually, so the user knows when it has been done.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:318: @qq{profit}, I mean @qq{push
stuff to staging}.
On 2012/01/17 20:24:35, janek wrote:
> i think this fragment is irrelevant here.  It confuses me.
> 
> I'd write something like 
> "You can switch to your local branches and to the remote branches as well"
> instead.

Done.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:345: Add a file, then commit it:
On 2012/01/17 20:24:35, janek wrote:
> I'd write
> "by default, git commit -a only commits changes to the files that existed
before
> you made your changes.  If you want to include a file created by you in the
> commit, use git add:"

Done.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:360: @subsubheading Save commits
manually (optional)
On 2012/01/17 20:24:35, janek wrote:
> I suggest changing this to 
> "save commits to external files" or sth. like that.
> Now it looks like this section will be about doing what was described above by
> hand (i.e. fiddling with git internals)

Done.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:362: Branches are nerve-wracking
until you get used to them.  You can
On 2012/01/17 20:24:35, janek wrote:
> Insert "After you committed your changes, you can..."
> 
> (format-patch doesn't work with uncommitted changes)

Done.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:377: Update your branch with the
latest master:
On 2012/01/17 20:24:35, janek wrote:
> Maybe we should mention that there shouldn't be any uncommitted changes before
> contributor tries to checkout master?  IIRC, when someone is on some branch,
> makes changes but doesn't commit them, and checks out another branch, these
> changes will "follow him" to this branch.  And with a dirty tree he won't be
> able to pull.

Done.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:377: Update your branch with the
latest master:
On 2012/01/17 20:24:35, janek wrote:
> Maybe we should mention that there shouldn't be any uncommitted changes before
> contributor tries to checkout master?  IIRC, when someone is on some branch,
> makes changes but doesn't commit them, and checks out another branch, these
> changes will "follow him" to this branch.  And with a dirty tree he won't be
> able to pull.

Done.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:396: Make sure that you're on your
branch, then upload:
I prefer the original wording.  I think the "then" actually indicates the
sequential nature of the steps better than "and".

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:413: @subsubheading Rewrite history
(optional unless you have broken commits)
I chose "Combining commits"

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:463: If everything looks good, push
it:
I decided that adding the information is more likely to raise questions than
answer them.  This is just a recipe, or a magic spell.

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:475: @warning{It is a best practice
to never rebase one of your branches
"avoid rebasing any of your branches"

http://codereview.appspot.com/5539062/diff/3004/Documentation/contributor/sou...
Documentation/contributor/source-code.itexi:480: changing your working branch.}
I decided it's too much information.  We're looking at copy and paste here.

[janek, 2012-01-20 13:39:45]

LGTM. Good job, Carl!

[janek, 2012-02-26 16:34:46]

Carl, could you close this Rietveld issue?
Janek

[c_sorensen, 2012-02-26 23:40:38]

On 2/26/12 9:34 AM, "janek.lilypond@gmail.com" <janek.lilypond@gmail.com>
wrote:

>Carl, could you close this Rietveld issue?
>Janek
>
>http://codereview.appspot.com/5539062/
>

Done, thanks.

Carl