Re: Update lilygit.tcl (Issue 2092) (issue 5504092)

[Carl . D . Sorensen]

Nice review.

I will make changes in response to your comments.

Thanks,

Carl



http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tcl
File scripts/auxiliar/lily-git.tcl (right):

http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tcl#newcode222
scripts/auxiliar/lily-git.tcl:222: proc update_lilypond_norebase {} {
On 2012/01/21 21:12:58, janek wrote:
> this isn't used anywhere, we keep it just in case?

Looks like it can go.  Thanks for the catch.

http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tcl#newcode252
scripts/auxiliar/lily-git.tcl:252: if {$workingBranch != ""} {
On 2012/01/21 21:12:58, janek wrote:
> Do i understand correctly that we check workingBranch and switch to
working on
> it if it's not empty?  So, if we don't want to work on workingBranch,
we set it
> to null and lily-git will remain on OriginHead (i.e. master)?

No.  workingBranch should never be null.  If it is, we'll get lots of
git errors.  So if workingBranch is null, we don't do any checking out.

No.  $workingBranch should never be null.  If you try to ch

http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tcl#newcode253
scripts/auxiliar/lily-git.tcl:253: add_working_branch
On 2012/01/21 21:12:58, janek wrote:
> Won't this reset our workingBranch to be a copy of master every time
we update
> repository?
> Wait, i see: it's done only when creating new repository?

Precisely.

http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tcl#newcode270
scripts/auxiliar/lily-git.tcl:270: git rebase origin/$originHead
On 2012/01/21 21:12:58, janek wrote:
> shouldn't we update local master too?
> If we don't do this, how can we not run into trouble in line 309?

Ahh.  I see what you mean.  Before we were only rebasing master to
origin/master.  Then we changed to rebasing workingBranch to
origin/master.

We should be rebasing master to origin/master, and then workingBranch to
master.

Thanks for the catch.

http://codereview.appspot.com/5504092/diff/14001/scripts/auxiliar/lily-git.tcl#newcode560
scripts/auxiliar/lily-git.tcl:560: if [catch {set workingBranch
$env(LILYPOND_BRANCH)}] {
On 2012/01/21 21:12:58, janek wrote:
> does this mean "it there's no external variable to control
workingBranch"?

Yes.  Strictly speaking it means "If there is an error in setting the
value of the local variable workingBranch to the environment variable
LILYPOND_BRANCH".

http://codereview.appspot.com/5504092/