Git development
 help / color / mirror / Atom feed
* [PATCH 3/4] git-gui: only except numbers in the goto-line input
From: Bert Wesarg @ 2011-10-13 13:48 UTC (permalink / raw)
  To: Pat Thoyts; +Cc: David Fries, git, Bert Wesarg
In-Reply-To: <1d1c91fdaa0bfd31067fd2e06f3f1ecf5597b8d3.1318513492.git.bert.wesarg@googlemail.com>

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
 lib/line.tcl |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/lib/line.tcl b/lib/line.tcl
index 692485a..70785e1 100644
--- a/lib/line.tcl
+++ b/lib/line.tcl
@@ -15,7 +15,11 @@ constructor new {i_w i_text args} {
 
 	${NS}::frame  $w
 	${NS}::label  $w.l       -text [mc "Goto Line:"]
-	entry  $w.ent -textvariable ${__this}::linenum -background lightgreen
+	entry  $w.ent \
+		-textvariable ${__this}::linenum \
+		-background lightgreen \
+		-validate key \
+		-validatecommand [cb _validate %P]
 	${NS}::button $w.bn      -text [mc Go] -command [cb _incrgoto]
 
 	pack   $w.l   -side left
@@ -26,7 +30,7 @@ constructor new {i_w i_text args} {
 	grid remove $w
 
 	bind $w.ent <Return> [cb _incrgoto]
-	bind $w.ent <Escape> [list linebar::hide $this]
+	bind $w.ent <Escape> [cb hide]
 
 	bind $w <Destroy> [list delete_this $this]
 	return $this
@@ -55,6 +59,14 @@ method editor {} {
 	return $w.ent
 }
 
+method _validate {P} {
+	# only accept numbers as input
+	if {[regexp {\d*} $P]} {
+		return 1
+	}
+	return 0
+}
+
 method _incrgoto {} {
 	if {$linenum ne {}} {
 		$ctext see $linenum.0
-- 
1.7.6.789.gb4599

^ permalink raw reply related

* [PATCH 2/4] git-gui: clear the goto line input when hiding
From: Bert Wesarg @ 2011-10-13 13:48 UTC (permalink / raw)
  To: Pat Thoyts; +Cc: David Fries, git, Bert Wesarg
In-Reply-To: <1d1c91fdaa0bfd31067fd2e06f3f1ecf5597b8d3.1318513492.git.bert.wesarg@googlemail.com>

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
 lib/line.tcl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lib/line.tcl b/lib/line.tcl
index 4913bdd..692485a 100644
--- a/lib/line.tcl
+++ b/lib/line.tcl
@@ -41,6 +41,7 @@ method show {} {
 
 method hide {} {
 	if {[visible $this]} {
+		$w.ent delete 0 end
 		focus $ctext
 		grid remove $w
 	}
-- 
1.7.6.789.gb4599

^ permalink raw reply related

* [RFC/PATCH 4/4] git-gui: incremental goto line in blame view
From: Bert Wesarg @ 2011-10-13 13:48 UTC (permalink / raw)
  To: Pat Thoyts; +Cc: David Fries, git, Bert Wesarg
In-Reply-To: <1d1c91fdaa0bfd31067fd2e06f3f1ecf5597b8d3.1318513492.git.bert.wesarg@googlemail.com>

The view jumps now to the given line number after each key press.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---

I didn't know this before, but gedits goto-line-dialog works this way.

 lib/line.tcl |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/lib/line.tcl b/lib/line.tcl
index 70785e1..0113e06 100644
--- a/lib/line.tcl
+++ b/lib/line.tcl
@@ -20,7 +20,7 @@ constructor new {i_w i_text args} {
 		-background lightgreen \
 		-validate key \
 		-validatecommand [cb _validate %P]
-	${NS}::button $w.bn      -text [mc Go] -command [cb _incrgoto]
+	${NS}::button $w.bn      -text [mc Go] -command [cb _goto]
 
 	pack   $w.l   -side left
 	pack   $w.bn  -side right
@@ -29,7 +29,8 @@ constructor new {i_w i_text args} {
 	eval grid conf $w -sticky we $args
 	grid remove $w
 
-	bind $w.ent <Return> [cb _incrgoto]
+	trace add variable linenum write [cb _goto_cb]
+	bind $w.ent <Return> [cb _goto]
 	bind $w.ent <Escape> [cb hide]
 
 	bind $w <Destroy> [list delete_this $this]
@@ -67,10 +68,16 @@ method _validate {P} {
 	return 0
 }
 
-method _incrgoto {} {
+method _goto_cb {name ix op} {
+	after idle [cb _goto 1]
+}
+
+method _goto {{nohide {0}}} {
 	if {$linenum ne {}} {
 		$ctext see $linenum.0
-		hide $this
+		if {!$nohide} {
+			hide $this
+		}
 	}
 }
 
-- 
1.7.6.789.gb4599

^ permalink raw reply related

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: arQon @ 2011-10-13 13:58 UTC (permalink / raw)
  To: git
In-Reply-To: <4E96D819.20905@op5.se>

Andreas Ericsson <ae <at> op5.se> writes:
> there's no reason to refuse the branch change.
> Partly because nothing will be lost

Actually, this isn't true either, because of the second bug: doing a revert
in branchA causes the changes in branchB to be lost. This can't possibly be
the intended behavior: again, it completely violates the integrity of branches
by allowing changes on one branch to impact a different branch.

Your interpretation of the manpage doubtless matches the actual behavior of git,
but I find it staggering if that truly is what was intended. It basically means
that if you have local modifications, git will Break Your Entire Tree. That
makes changing while you *do* have local mods more than a little undesirable,
to put it mildly, which is something that a literal reading of the manpage would
suggest is exactly what the "refuse to switch" is for. I guess only Linus knows
what he actually meant.  :)

Anyway, I guess it's all moot: call it a feature or call it a bug, this cross-
branch destruction is a deal-breaker for me, especially given the bug above that
actually loses data outright, rather than "only" putting multiple branches into
an incorrect state.

Thanks for your time and help.

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: Carlos Martín Nieto @ 2011-10-13 13:59 UTC (permalink / raw)
  To: arQon; +Cc: git
In-Reply-To: <loom.20111013T144822-277@post.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 3195 bytes --]

On Thu, 2011-10-13 at 13:09 +0000, arQon wrote:
> Andreas Ericsson <ae <at> op5.se> writes:
> [snip]
> > This means that if fileX on branchA is different from fileX on branchB and you
> > *also* have local modifications to fileX, git will refuse to switch branches.
> > If, on the other hand branchA:fileX == branchB:fileX and you have modifications
> > to fileX in your work tree, there's no reason to refuse the branch change.
> 
> There's an EXCELLENT reason to refuse the branch change: once it happens, what
> git is then telling is branchA, is not.

When you changed branches, git told you that a file had been changed in
the working tree. When you run 'git diff', it tells you the differences
between what you have in your working tree and what's in the branch[0].
Git is trying hard not to loose your modifications (maybe it was a
one-liner, maybe it was three hours of work) to the file.

> 
> > It's not a bug. You just read the manpage a bit wrong.
> [snip]
> > So yes, this is a feature, and it's a handy one.
> 
> Thanks for the explanation. Unfortunately, I still can't see it as anything but
> a critical bug. Consider this:
> 
> You're working on branchA and you have a bunch of uncommitted changes.
> You can't remember some detail of the bug you're fixing, so you switch branches
> to the master. You have to rebuild that branch, because your last build was from
> your branch. git now builds the master with sources that were NEVER committed
> to it. How is that not a total failure to maintain branch integrity?

It sound like you've misunderstood what a branch is for git. A branch is
only ever changed when you commit. What checkout does is change what the
current branch is. For a case like what you describe, the developer
would either do a temporary commit that they'd change later or stash the
changes[1]. You could also use git-new-workdir (from contrib/) so you
have two different directories that share the object storage. That has a
few rough edges, but if you restrict it to a broken branch, you
shouldn't have any problems.

> 
> If that's the way git is, then that's how it is; and if there isn't a setting
> that can make it actually preserve branches properly, then there isn't. Which
> sucks for me, because an SCCS that lies about what branch you're "really" on
> is worse than useless, so I'm stuck with SVN.  :(

Don't think of it as being "in" a branch. A checkout in git changes the
active branch. If there are any files that are different between the two
branches, they are changed. By switching branches with uncommitted
changes, you're telling git that you would rather use the other branch
to do your changes in. But git isn't doing this silently. After the
checkout, it lists the files that have local modifications, so the
developer can switch branches again and commit or stash the changes.

   cmn

[0] Really it's between the working tree and the index, but since you
just switched branches, the index is the same, and using it in that
sentence would just cause confusion.

[1] 'git stash' is a command that saves your uncommitted changes on a
stack so you can recover them later.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: Victor Engmark @ 2011-10-13 14:44 UTC (permalink / raw)
  To: arQon; +Cc: git
In-Reply-To: <loom.20111013T141239-151@post.gmane.org>

On Thu, Oct 13, 2011 at 12:42:42PM +0000, arQon wrote:
> Simple testcase:
> 
> >git init
> Initialized empty Git repository in C:/git-test/.git/
> >notepad file1
> >notepad file2
> >git st
>  # On branch master
>  # Initial commit
>  # Untracked files:
>  #   (use "git add <file>..." to include in what will be committed)
>  #       file1.txt
>  #       file2.txt
>  nothing added to commit but untracked files present (use "git add" to track)
> 
> >git add .
> >git st
>  # On branch master
>  # Initial commit
>  # Changes to be committed:
>  #       new file:   file1.txt
>  #       new file:   file2.txt
> 
> >git commit -am "init"
>   2 files changed, 2 insertions(+), 0 deletions(-)
>   create mode 100644 file1.txt
>   create mode 100644 file2.txt
> 
> >git co -b foo
>  Switched to a new branch 'foo'
> >notepad file1
> (edit stuff)
> >git st
>  # On branch foo
>  # Changes not staged for commit:
>  #       modified:   file1.txt
> 
> >git co master
>  M       file1.txt
> 
> file1 now has the wrong data in it for "master" branch.

The most important thing a VCS should do is to keep history intact.
That happens when you check out in Git: No branches were changed, only
the working space. The second most important thing a VCS should do is
not destroy any of your uncommitted work unless you tell it to. That
also happens when you check out in Git. The third most important thing a
VCS should do is facilitate the developer's workflow. One common thing
to do is to work on some thing, for example refactoring. During this
process you might realize that one of the changes actually fixed a bug
in the software. To keep things in their right place, you could now
either
1. `checkout master` and commit the fix there, then shift back and
continue working, or
2. commit the refactorings, `checkout master`, and commit the fix there.
Either of these are easy to do with Git. There really is no reason why
the changes in the workspace should be considered as "part of" the
currently active branch, because they *are* not.

Cheers,
V

-- 
terreActive AG
Kasinostrasse 30
CH-5001 Aarau
Tel: +41 62 834 00 55
Fax: +41 62 823 93 56
www.terreactive.ch

Wir sichern Ihren Erfolg - seit 15 Jahren

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: Carlos Martín Nieto @ 2011-10-13 14:46 UTC (permalink / raw)
  To: arQon; +Cc: git
In-Reply-To: <loom.20111013T152144-60@post.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 2570 bytes --]

On Thu, 2011-10-13 at 13:58 +0000, arQon wrote:
> Andreas Ericsson <ae <at> op5.se> writes:
> > there's no reason to refuse the branch change.
> > Partly because nothing will be lost
> 
> Actually, this isn't true either, because of the second bug: doing a revert
> in branchA causes the changes in branchB to be lost. This can't possibly be
> the intended behavior: again, it completely violates the integrity of branches
> by allowing changes on one branch to impact a different branch.

I have not seen a revert command in any of your messages. If a revert on
one branch changes another one, that would be a bug, but you haven't
shown this to happen.

> 
> Your interpretation of the manpage doubtless matches the actual behavior of git,
> but I find it staggering if that truly is what was intended. It basically means
> that if you have local modifications, git will Break Your Entire Tree. That
> makes changing while you *do* have local mods more than a little undesirable,
> to put it mildly, which is something that a literal reading of the manpage would
> suggest is exactly what the "refuse to switch" is for. I guess only Linus knows
> what he actually meant.  :)

Do not confuse a branch with a worktree. If you haven't committed yet,
those changes aren't in the branch (just like they wouldn't be in svn)

> 
> Anyway, I guess it's all moot: call it a feature or call it a bug, this cross-
> branch destruction is a deal-breaker for me, especially given the bug above that
> actually loses data outright, rather than "only" putting multiple branches into
> an incorrect state.

I've just asked some subversion developers to confirm this, and then
tried it out myself: Subversion (my locally-installed version is 1.6.17,
latest stable) behaves the same way. Local modifications are carried
over across branches.

$ svn copy ^/trunk ^/branches/somebranch # Create a new branch
$ $EDITOR somefile # which exists in trunk and somebranch
$ svn switch ^/branches/somebranch
$ svn diff # My local changes are there!

The reason this happens both in svn and git is that the most likely
cause for someone to change a branch mid-edit is that they decide
they're doing the changes on the wrong branch. What I did notice is that
svn doesn't tell you about the modifications being carried over
(presumably you're meant to use status and diff to figure out what's
going on). Therefore, the same workflow (with the only difference being
how to create and switch branches) works for svn and git in this case.

   cmn



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: Michael J Gruber @ 2011-10-13 15:09 UTC (permalink / raw)
  To: arQon; +Cc: git
In-Reply-To: <loom.20111013T094053-111@post.gmane.org>

arQon venit, vidit, dixit 13.10.2011 10:40:
> Which, as you'd expect, results in both the on-disk copies and other branches
> becoming corrupted.
> 
> Tested on git versions 1.7.6 and 1.7.7 (msysgit)
> 
> http://benno.id.au/blog/2011/10/01/git-recursive-merge-broken describes
> something that sounds similar, but that's supposedly fixed on 1.7.7,
> whereas this happens on that as well.
> 
> master is a tracking branch, "ttfcon" is the branch I was using to develop
> a change. Got to a good point on the branch, merged it in:
> 
> $ git co master
> $ git merge ttfcon
> Updating b9f0c75..6280b7a
> Fast-forward
>  .gitignore                |    2 ++
>  code/renderer/tr_font.cpp |   27 ++++++++-------------------
>  2 files changed, 10 insertions(+), 19 deletions(-)
> 
> $ git st
> # On branch master
> # Your branch is ahead of 'origin/master' by 3 commits.
> 
> back to the branch to mess around with a couple of things to be sure this
> is what i want to push
> $ git co ttfcon
> do stuff
> 
> $ git st
> # On branch ttfcon
> # Changes not staged for commit:
> #   (use "git add <file>..." to update what will be committed)
> #   (use "git checkout -- <file>..." to discard changes in working directory)
> #
> #       modified:   code/freetype-2.3.11/builds/win32/visualc/freetype.vcproj
> #       modified:   code/renderer/tr_font.cpp
> 
> so far so good...
> 
> $ git ci -m "blah" code/freetype-2.3.11/builds/win32/visualc/freetype.vcproj
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> note that tr_font is locally modified and still *not committed* at this point.

and neither staged for commit, exactly.

> $ git co master
> M       code/renderer/tr_font.cpp
> Switched to branch 'master'
> Your branch is ahead of 'origin/master' by 3 commits.
> 
> boom. instead of rejecting the branch change, git switches branches anyway,
> and doesn't do anything about the uncommitted changes in the file itself -

Exactly. git leaves them as they are, without changing what you have in
your work tree.

(This is possible because the switch ttfcon to master involves no
changes which conflict with the chnage that you have in your work tree).

> meaning they're now effectively "in" master because they're still on disk,
> so now the master is poisoned.

Not at all. They are on "disk" (work tree). Full stop. Not staged, not
committed, not at all "in master".

> 
> "git st" does show the change:
> 
> # On branch master
> # Changes not staged for commit:
> #       modified:   code/renderer/tr_font.cpp
> 
> but it's a change I never MADE on this branch (ie master), only on the
> other branch.

You never made it on the other branch either. You made it in the work
tree. And "git status" clearly says so: modified, not staged.

> "git diff" is just as confused as I am:
> 
> $ git diff ttfcon
> --- a/code/renderer/tr_font.cpp
> +++ b/code/renderer/tr_font.cpp
> +		// git branch bug

"git diff" shows you the change you have in your work tree, i.e. the
difference between index (which coincides with master since nothing is
staged) and work tree. The fact that there is a difference is equivalent
to saying "there are unstaged changes".

> So it's picking up the difference between the two branches, but as far as

No. The difference between the branches is the change to freetype.vcproj
because you committed that to ttfcon, not master.

> the *actual file* goes, master now has a line in it that shouldn't be there.

It's in the work tree, not master....

> I'm just trying out git as a possible replacement for SVN, so maybe I'm
> mistaken about what "should" happen, but AIUI git switching branches with
> uncommitted changes is a bug (and given that it poisoned a branch that I
> wasn't on, it certainly looks like one). A couple of days ago it DID complain
> when I tried to switch with uncommitted files still present, so it was working
> properly then. I have no idea what's made it happy to ignore them now:
> nothing's changed that I know of.

When switching branches, git tries to preserves the changes that you
have in your work tree. If it is possible (because there is no overlap,
as written above), it hapilly does just that. If not it barks.

I think you have to wrap your head around the Git model after unwinding
it from the svn model, which is normal ;)

Cheers,
Michael

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: arQon @ 2011-10-13 15:53 UTC (permalink / raw)
  To: git
In-Reply-To: <1318517194.4646.30.camel@centaur.lab.cmartin.tk>

Carlos Martín Nieto <cmn <at> elego.de> writes:
> I have not seen a revert command in any of your messages. If a revert on
> one branch changes another one, that would be a bug, but you haven't
> shown this to happen.

Sorry, it was in prose in the original post (near the end)
"At this point, reverting the master with "checkout --" also wipes out the
changes on the other branch. It's like the merge symlinked the two branches
rather than, well, merging them."

Based on the explanations here, and the git *st* message, it wiping out the
other branch is to be expected, because it's "the working directory", not
"the branch".

>git st
# On branch foo
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#       modified:   file1.txt
#
no changes added to commit (use "git add" and/or "git commit -a")

What makes this really interesting though is this: I tried to switch to
master to see if that gave the same warning, and NOW, I get the correct
error.

>git co master
error: Your local changes to the following files would be overwritten by
checkout:
        file1.txt
Please, commit your changes or stash them before you can switch branches.
Aborting

I'm sure if I thought about it enough (ie re-read Andreas's post a couple
more times) I'd be able to understand why git gets it right sometimes but
not other times, but I'm too tired right now. Even when I *am* awake and
grok it properly, I'm still going to be annoyed that it's so inconsistent,
but I can live with that if I have to.

> The reason this happens both in svn and git is that the most likely
> cause for someone to change a branch mid-edit is that they decide
> they're doing the changes on the wrong branch.

Lucky you. :P  The most likely reason for me is, I'm working on something
and I get interrupted and have to switch. Since the code may well not even
compile at this point, the last thing I want to do is commit it. git's
ability for that commit to be local is half the reason I'm trying to switch
to it. (I'm not particularly keen on having to commit broken code to even a
local repo, but that's still a hell of a lot better than having it pushed
upstream as well).

> svn doesn't tell you about the modifications being carried over
> (presumably you're meant to use status and diff to figure out what's
> going on). Therefore, the same workflow (with the only difference being
> how to create and switch branches) works for svn and git in this case.

I expect part of my confusion comes from using different workdirs for svn
branches, ie "clone" rather than "branch", because branching in svn is such
a PITA I just don't bother with it unless the branch is going to be
"heavyweight" enough to warrant a "proper" branch.
Good to be reminded of though, thanks.

^ permalink raw reply

* Re: [Bug] git pull doesn't recognize --work-tree parameter
From: Jeff King @ 2011-10-13 15:59 UTC (permalink / raw)
  To: Kirill Likhodedov; +Cc: Junio C Hamano, git
In-Reply-To: <E95C75ED-99F2-463C-A1AB-0F8152696739@jetbrains.com>

On Thu, Oct 13, 2011 at 12:38:53PM +0400, Kirill Likhodedov wrote:

> 'git pull' doesn't work from outside the working tree even if '--work-tree' is specified:
> 
> # git version
> git version 1.7.6
> # git --git-dir=/Users/loki/sandbox/git/child/.git --work-tree=/Users/loki/sandbox/git/child pull
> fatal: /opt/local/libexec/git-core/git-pull cannot be used without a working tree.
> 
> Note that  'git fetch' and 'git merge origin/master' work fine, so 'git pull' should be easy to fix :)
> 
> # git --git-dir=/Users/loki/sandbox/git/child/.git --work-tree=/Users/loki/sandbox/git/child merge origin/master
> Already up-to-date.

This is a known issue, and the fix is a one-liner, but it needed
somebody to look through the pull script to make sure it wasn't
introducing any new bugs. I just did this; the patch below should fix
your problem.

-- >8 --
Subject: [PATCH] pull,rebase: handle GIT_WORK_TREE better

You can't currently run git-pull or git-rebase from outside
of the work tree, even with GIT_WORK_TREE set, due to an
overeager require_work_tree function. Commit e2eb527
documents this problem and provides the infrastructure for a
fix, but left it to later commits to audit and update
individual scripts.

Changing these scripts to use require_work_tree_exists is
easy to verify. We immediately call cd_to_toplevel, anyway.
Therefore no matter which function we use, the state
afterwards is one of:

  1. We have a work tree, and we are at the top level.

  2. We don't have a work tree, and we have died.

The only catch is that we must also make sure no code that
ran before the cd_to_toplevel assumed that we were already
in the working tree.

In this case, we will only have included shell libraries and
called set_reflog_action, neither of which care about the
current working directory at all.

Signed-off-by: Jeff King <peff@peff.net>
---
This is the low-hanging, obviously correct fruit. git-am and
git-stash also immediately cd_to_toplevel, but they look at
$PWD or `git rev-parse --show-prefix` beforehand, which
means those uses have to be audited separately.

 git-pull.sh   |    2 +-
 git-rebase.sh |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 63da37b..902fc4a 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -11,7 +11,7 @@ OPTIONS_SPEC=
 . git-sh-setup
 . git-sh-i18n
 set_reflog_action "pull${1+ $*}"
-require_work_tree
+require_work_tree_exists
 cd_to_toplevel
 
 
diff --git a/git-rebase.sh b/git-rebase.sh
index 6759702..00ca7b9 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -63,7 +63,7 @@ skip!              skip current patch and continue
 "
 . git-sh-setup
 set_reflog_action rebase
-require_work_tree
+require_work_tree_exists
 cd_to_toplevel
 
 LF='
-- 
1.7.6.4.37.g43b58b

^ permalink raw reply related

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: Holger Hellmuth @ 2011-10-13 16:08 UTC (permalink / raw)
  To: arQon; +Cc: git
In-Reply-To: <loom.20111013T152144-60@post.gmane.org>

On 13.10.2011 15:58, arQon wrote:
> Andreas Ericsson<ae<at>  op5.se>  writes:
>> there's no reason to refuse the branch change.
>> Partly because nothing will be lost
>
> Actually, this isn't true either, because of the second bug: doing a revert
> in branchA causes the changes in branchB to be lost. This can't possibly be
> the intended behavior: again, it completely violates the integrity of branches
> by allowing changes on one branch to impact a different branch.

I assume you mean revert through 'git checkout' and not through 'git 
revert'. Git uses a different philosphy. It works best with small 
commits and commits done often. It assumes that when you switch 
branches, you don't switch your brain as well and still know for what 
purpose you changed tr_font.cpp (and even if you forget you always can 
check with git diff).
It also reminds you that tr_font.cpp is changed when you switch branches 
(remember the "M tr_font.cpp" printed when you switched to another branch).
It assumes that when you use 'git checkout --' to wipe out changed files 
without committing them anywhere(!) that you have thought about it the 
same way you have thought about before deleting or overwriting any file 
in the file system. The same way you have thought about before deleting 
or overwriting an uncommitted file in svn.

What you term integrity of the branch is a model you made of the 
workings of svn that you now try to pin onto a different model.

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: Alexey Shumkin @ 2011-10-13 16:17 UTC (permalink / raw)
  To: arQon; +Cc: git
In-Reply-To: <loom.20111013T171530-970@post.gmane.org>

> Lucky you. :P  The most likely reason for me is, I'm working on
> something and I get interrupted and have to switch. Since the code
> may well not even compile at this point, the last thing I want to do
> is commit it. 
"git stash" helps here
With Git you can/have_to/must change your SVN-based habits.
DO NOT BE AFRAID OF FREQUENT COMMITS!
There are local until you push them.

>git's ability for that commit to be local is half the
> reason I'm trying to switch to it.
You always have a chance to modify/reedit you commits
see "git commit --amend" and "git rebase [-i]"

I'm telling you it as an ex-SVN user.
>(I'm not particularly keen on
> having to commit broken code to even a local repo, but that's still a
> hell of a lot better than having it pushed upstream as well).

Again, do not be afraid to commit your changes. Be afraid of losing
your changes. Git makes everything (as other discussion participants
already described) to keep your changes within workflow when you
switch between branches often.

Read some books which are describe Git's usual (and effective) workflow,
ProGit - http://progit.org/book/
Version Contol by Example (there is a chapter about Git) -
http://git-scm.com/course/svn.html

Hope, you'll feel the power of Git ))

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: arQon @ 2011-10-13 16:17 UTC (permalink / raw)
  To: git
In-Reply-To: <20111013144450.GA2856@victor.terreactive.ch>

Victor Engmark <victor.engmark <at> terreactive.ch> writes:
> 1. `checkout master` and commit the fix there, then shift back and
> continue working

I absolutely agree. And it's far more common than any of us would like.
My point is, you *can't* do this in git without first staging your current branch
via either commit or stash, or you risk changes bleeding between the branches
and/or work being lost irretrievably. This is not something that you would
expect, and as you say:

> The second most important thing a VCS should do is not destroy any of your
uncommitted work unless you tell it to

... which is exactly what git does, and why I have a problem with it.
But the response here is uniformly "that's just how git is", so obviously it's
something you learn to become aware of over time, and avoid. It's not going to
get "fixed", because people who are used to git don't see it as a bug, so I just
have to decide whether I can live with it or not.

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: Holger Hellmuth @ 2011-10-13 16:32 UTC (permalink / raw)
  To: arQon; +Cc: git
In-Reply-To: <loom.20111013T171530-970@post.gmane.org>

On 13.10.2011 17:53, arQon wrote:
>> git st
> # On branch foo
> # Changes not staged for commit:
> #   (use "git add<file>..." to update what will be committed)
> #   (use "git checkout --<file>..." to discard changes in working directory)
> #
> #       modified:   file1.txt
> #
> no changes added to commit (use "git add" and/or "git commit -a")
>
> What makes this really interesting though is this: I tried to switch to
> master to see if that gave the same warning, and NOW, I get the correct
> error.
>
>> git co master
> error: Your local changes to the following files would be overwritten by
> checkout:
>          file1.txt
> Please, commit your changes or stash them before you can switch branches.
> Aborting

At the end of your example (in a previous email) you were on branch 
master, now in the beginning you are on foo. So you at least changed 
branch again inbetween. maybe you also committed something? Check out 
git log or gitk

I tried your example and I can checkout master and foo again and again 
and I never see the error message.

> Lucky you. :P  The most likely reason for me is, I'm working on something
> and I get interrupted and have to switch. Since the code may well not even
> compile at this point, the last thing I want to do is commit it. git's
> ability for that commit to be local is half the reason I'm trying to switch
> to it. (I'm not particularly keen on having to commit broken code to even a
> local repo, but that's still a hell of a lot better than having it pushed
> upstream as well).

As Alexey already said, just commit and later amend. Or stash. Git 
encourages you to commit small changes you can put a name to. You never 
should delay a commit because it produces unworkable code. Instead have 
a master branch (or branches) that always compiles and branches for the 
unfinished stuff. Then it won't matter if some branch is only half working.

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: Carlos Martín Nieto @ 2011-10-13 17:04 UTC (permalink / raw)
  To: arQon; +Cc: git
In-Reply-To: <loom.20111013T171530-970@post.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 4828 bytes --]

On Thu, 2011-10-13 at 15:53 +0000, arQon wrote:
> Carlos Martín Nieto <cmn <at> elego.de> writes:
> > I have not seen a revert command in any of your messages. If a revert on
> > one branch changes another one, that would be a bug, but you haven't
> > shown this to happen.
> 
> Sorry, it was in prose in the original post (near the end)
> "At this point, reverting the master with "checkout --" also wipes out the
> changes on the other branch. It's like the merge symlinked the two branches
> rather than, well, merging them."
> 
> Based on the explanations here, and the git *st* message, it wiping out the
> other branch is to be expected, because it's "the working directory", not
> "the branch".
> 
> >git st
> # On branch foo
> # Changes not staged for commit:
> #   (use "git add <file>..." to update what will be committed)
> #   (use "git checkout -- <file>..." to discard changes in working directory)
> #
> #       modified:   file1.txt
> #
> no changes added to commit (use "git add" and/or "git commit -a")
> 
> What makes this really interesting though is this: I tried to switch to
> master to see if that gave the same warning, and NOW, I get the correct
> error.
> 
> >git co master
> error: Your local changes to the following files would be overwritten by
> checkout:
>         file1.txt
> Please, commit your changes or stash them before you can switch branches.
> Aborting
> 
> I'm sure if I thought about it enough (ie re-read Andreas's post a couple
> more times) I'd be able to understand why git gets it right sometimes but
> not other times, but I'm too tired right now. Even when I *am* awake and
> grok it properly, I'm still going to be annoyed that it's so inconsistent,
> but I can live with that if I have to.

If file1.txt in the foo branch is different from the one in the master
branch, git will refuse to switch branches. 'git diff foo master' should
show that those two files are different.

> 
> > The reason this happens both in svn and git is that the most likely
> > cause for someone to change a branch mid-edit is that they decide
> > they're doing the changes on the wrong branch.
> 
> Lucky you. :P  The most likely reason for me is, I'm working on something
> and I get interrupted and have to switch. Since the code may well not even
> compile at this point, the last thing I want to do is commit it. git's
> ability for that commit to be local is half the reason I'm trying to switch
> to it. (I'm not particularly keen on having to commit broken code to even a
> local repo, but that's still a hell of a lot better than having it pushed
> upstream as well).

Yes, this is a great feature of distributed systems. A local repo is
where you experiment. Treat it as your own personal space to play around
with things. Committing non-working code is fine, as long as you don't
push it out.

> 
> > svn doesn't tell you about the modifications being carried over
> > (presumably you're meant to use status and diff to figure out what's
> > going on). Therefore, the same workflow (with the only difference being
> > how to create and switch branches) works for svn and git in this case.
> 
> I expect part of my confusion comes from using different workdirs for svn
> branches, ie "clone" rather than "branch", because branching in svn is such
> a PITA I just don't bother with it unless the branch is going to be
> "heavyweight" enough to warrant a "proper" branch.

Then the issue is that you've changed the workflow but haven't adjusted
for it. You can do this as well with the git-new-workdir. As I mentioned
it has a few rough edges, but if you're going to use it to have a
checkout of a particular branch, it shouldn't present any problems. That
would be like your current workflow.

Another option is to clone with a reference which will create a brand
new clone but will use the objects that you've already downloaded (or
just clone locally). This can be more comfortable than using the
new-workdir and will hardly put any strain on the filesystem.

The bigger problem seems to be your reluctance to accept that git is
different from subversion, as you keep saying "that's just how git is"
to back your claim that you can't trust git on a feature where
subversion behaves the same way. If you'd rather use different
directories for different branches, you can. That is not an aspect which
you can point to and say that you can't migrate to git for that reason.
If you're more comfortable with subversion, that's fine also, it's an
excellent piece of software[0], but don't go around saying that git
corrupts branches when that's blatantly not true.

   cmn

[0] Whatever one may think about the merits of CVCS vs DVCS; that
shouldn't come into the quality of the software.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply

* Re: [CLOSED] git checkout <branch> allowed with uncommitted changes
From: arQon @ 2011-10-13 17:09 UTC (permalink / raw)
  To: git
In-Reply-To: <1318514356.4646.16.camel@centaur.lab.cmartin.tk>

Carlos Martín Nieto <cmn <at> elego.de> writes:
> When you changed branches, git told you that a file had been changed in
> the working tree.

That's a very good point, if it was actually documented at all that that's
what it meant; and / or presented some warning that "BTW, if you edit this
file again now, it'll screw up your whole tree" instead of an innocuous "M";
and didn't appear to contradict what the manpage says about "local
modifications", we could probably have avoided half of this confusion.  :P
As it was, when I saw that M suddenly appear after several days of bouncing
between branches without it and with everything working, I just thought
"oh great, git's managed to break this tree", because I remembered the same
thing from the previous trial run.

That there IS an indication though might just be enough for me to be able to
deal with it. Realistically, switching branches with uncommitted changes
(unless you're doing it because you've ALREADY screwed up and are changing
the wrong branch) is basically a trainwreck waiting to happen.

git stash appears to be useless for any nontrivial change on the *other*
branch, since there's no indication when you return to the stashed branch
there's a stash sitting around, which is not something you're going to
remember the next morning if fixing the master took the rest of the day,
and you're not going to use "stash list" by then either.

But as long as you get the "warning", an alias that does a "commit -am 'temp
commit to avoid git breaking the tree'" is something I think I can probably
live with.

Thanks for all the help guys - very much appreciated.
As far as I'm concerned, this topic's done.

(Though if someone can come up with a script / hook / whatever that improves
the "visibility" of stash, that would be awesome. Or one that makes the
refusal to switch branches consistent).

Looking at the manpage for checkout in the hope that there might be a "--safe"
switch, I don't understand why
  "-f  Proceed even if the index *or the working tree* differs from HEAD."
even exists, since it proceeds under those conditions anyway.
"--safe" appears to be exactly what the behavior should be if you DON'T
specify -f, except that -f nukes the working tree outright rather than just
bleeding it across. Hopefully it'll be clearer after some sleep.  :)

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: Sergei Organov @ 2011-10-13 17:06 UTC (permalink / raw)
  To: arQon; +Cc: git
In-Reply-To: <loom.20111013T171530-970@post.gmane.org>

arQon <arqon@gmx.com> writes:

[...]

> Lucky you. :P  The most likely reason for me is, I'm working on something
> and I get interrupted and have to switch. Since the code may well not even
> compile at this point, the last thing I want to do is commit it.

'git stash' is exactly what you need then.

-- Sergei.

^ permalink raw reply

* Re: [PATCH 2/9] completion: optimize refs completion
From: Junio C Hamano @ 2011-10-13 17:28 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git, Shawn O. Pearce, Jonathan Nieder
In-Reply-To: <20111013104047.GA15379@goldbirke>

SZEDER Gábor <szeder@ira.uka.de> writes:

> ... If we use the default IFS containing an SP and append a SP to
> possible completion words by 'compgen -S " "' (or by word="$word ", as
> in __gitcomp_1()), then that SP will be promply stripped off when
> compgen's output is stored in the COMPREPLY array.  Using an IFS
> without SP keeps those SP suffixes.

Ahh, then I am perfectly fine with the "nl" in its name.

This is a function that unconditionally completes with trailing separator,
avoiding the cost of having to inspect each element, but the user needs to
keep in mind that the elements in the input must be separated with NL, so
ideally the name needs to convey both of these points---dropping "nl" was
a bad suggestion.

Thanks for a clear explanation.

^ permalink raw reply

* Re: Git attributes ignored for root directory
From: Junio C Hamano @ 2011-10-13 17:38 UTC (permalink / raw)
  To: Gioele Barabucci; +Cc: Michael Haggerty, git
In-Reply-To: <4E96C220.5050601@svario.it>

Gioele Barabucci <gioele@svario.it> writes:

> On 13/10/2011 00:12, Junio C Hamano wrote:
> ...
>> How would you say the same thing if the directory to be ignored weren't
>> "foo" but at the top-level of the working tree? There is no such level
>> higher than the top-level where you can say "<the name of your project>/"
>> in its .gitignore file.
>
> Shouldn't `/.` or `/./` work?
>
> I see that `/*/` in `.gitignores` successfully ignores all the
> non-hidden directories in the root project directory. Another
> accidental success? :)

Hannes correctly explained how /*/ works already; armed with that
knowledge, we can understand that:

        /.      would mean "what matches dot only in this directory."

        /./     would mean "what matches dot only in this directory
                but the thing that matches must be a directory."

        ./      would mean "what matches dot in this directory and its
                subdirectories, but the thing that matches must be a
                directory."

Given that "." does _not_ match the directory that has the .gitignore or
the .gitattribute file with the current system, none of the above patterns
can be used to match everything in the top level directory in a way
similar to how you can say "foo/" to match everything in "foo" directory
from the top-level directory.

The answer to your question is no, it shouldn't work.

Without adding a rule to the "Pattern Format" section as I suggested in my
message that Michael quoted in his question, that is.

^ permalink raw reply

* Re: [Bug] git pull doesn't recognize --work-tree parameter
From: Junio C Hamano @ 2011-10-13 18:01 UTC (permalink / raw)
  To: Jeff King; +Cc: Kirill Likhodedov, git
In-Reply-To: <20111013155923.GA13134@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Changing these scripts to use require_work_tree_exists is
> easy to verify. We immediately call cd_to_toplevel, anyway.
> Therefore no matter which function we use, the state
> afterwards is one of:
>
>   1. We have a work tree, and we are at the top level.
>
>   2. We don't have a work tree, and we have died.
>
> The only catch is that we must also make sure no code that
> ran before the cd_to_toplevel assumed that we were already
> in the working tree.
>
> In this case, we will only have included shell libraries and
> called set_reflog_action, neither of which care about the
> current working directory at all.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This is the low-hanging, obviously correct fruit.

I am not absolutely sure about "obviously correct", given that you assume
that cd_to_toplevel does what its name makes you think it does.  I've been
wondering if we want to do give a bit more sanity to "cd_to_toplevel" and
"rev-parse --show-toplevel".

    $ pwd
    /srv/project/git/git.git
    $ (cd Documentation/howto && git rev-parse --show-toplevel); echo $?
    /srv/project/git/git.git
    0

So far so good, however:

    $ (cd .git/refs/heads && git rev-parse --show-toplevel); echo $?
    0

I do not think this is quite right.

We would probably want to add "rev-parse --show-work-tree", but we would
need to audit the users of cd_to_toplevel before starting to use it.  I
wouldn't be surprised if there is a script that creates a temporary work
tree in .git/some/where and runs the scripted Porcelains without setting
GIT_WORK_TREE, relying on the historical behaviour of cd_to_toplevel that
does not really go to the top level.

^ permalink raw reply

* Re: [PATCH 0/2] Do not search submodules in deep recursive merge
From: Junio C Hamano @ 2011-10-13 18:15 UTC (permalink / raw)
  To: Brad King; +Cc: git, Junio C Hamano, Heiko Voigt
In-Reply-To: <cover.1318509069.git.brad.king@kitware.com>

Brad King <brad.king@kitware.com> writes:

> On 10/12/2011 2:48 PM, Junio C Hamano wrote:
>> [Stalled]
>>
>> * hv/submodule-merge-search (2011-08-26) 5 commits
>>   - submodule: Search for merges only at end of recursive merge
>>   - allow multiple calls to submodule merge search for the same path
>>   - submodule: Demonstrate known breakage during recursive merge
>>   - push: Don't push a repository with unpushed submodules
>>   - push: teach --recurse-submodules the on-demand option
>>   (this branch is tangled with fg/submodule-auto-push.)
>
> AFAICT these two topics are tangled due revision traversal interactions.
> I've untangled the two "submodule:" commits from this stalled topic and
> rebased on master (34c4461a) resolving one conflict.

Thanks.

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: arQon @ 2011-10-13 18:19 UTC (permalink / raw)
  To: git
In-Reply-To: <1318525486.4646.53.camel@centaur.lab.cmartin.tk>

Carlos Martín Nieto <cmn <at> elego.de> writes:
> If file1.txt in the foo branch is different from the one in the master
> branch, git will refuse to switch branches. 'git diff foo master' should
> show that those two files are different.

Right, but only for a definition of "branch" that is actually "a fully
committed branch", hence the confusion and the mention of "uncommitted
changes" in the topic.

An expectation that "co branch" should be analogous to "cd ../branch/" is by
no means unreasonable. YOU may know better, but it's surprisingly non-obvious,
especially considering the -f option on checkout and the wording of -m, both
of which strongly suggest that, in the absence of either of those flags, git
WILL preserve the worktree by refusing to switch until that potentially-
harmful situation is resolved by the user.

> Committing non-working code is fine, as long as you don't push it out.

Right, but for the problem I was describing it's actually "committing
non-working code is a requirement, in this situation, if you don't want your
tree to get eaten". Going from "you absolutely must not do this" to "you must
do this" takes some mental adjustment, but you also have to be *aware* that
you now have to do something that was previously prohibited, which I wasn't.

> The bigger problem seems to be your reluctance to accept that git is
> different from subversion

Not at all. If I didn't WANT something different, I wouldn't have been trying
to move to git in the first place.  :)

> but don't go around saying that git
> corrupts branches when that's blatantly not true.

See my first para in this post (or indeed, the original post). It's "not true"
provided all branches are fully committed when you switch between them.
It blatantly IS true if you switch from a dirty branch.
Redefining "branch" to mean "fully committed branch" makes it "not true" in
that context, but so does redefining green to be red and saying that grass is
red in that context: it may be correct from a certain POV, but it's
incomprehensible to anyone who isn't aware of that semantic change.

Anyway, I think we're done with this thread. Thanks for your (key) observation
earlier and several clarifications, and hopefully this will help the next guy
who runs into this problem and gets confused like I did.  :)

^ permalink raw reply

* Re: [PATCH] daemon: return "access denied" if a service is not allowed
From: Jeff King @ 2011-10-13 18:28 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy
  Cc: git, Ilari Liusvaara, Junio C Hamano, Johannes Sixt,
	Jonathan Nieder
In-Reply-To: <20111013044544.GA27890@duynguyen-vnpc.dek-tpc.internal>

On Thu, Oct 13, 2011 at 03:45:44PM +1100, Nguyen Thai Ngoc Duy wrote:

> On Wed, Oct 12, 2011 at 04:09:16PM -0400, Jeff King wrote:
> > On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:
> > 
> > > The message is chosen to avoid leaking information, yet let users know
> > > that they are deliberately not allowed to use the service, not a fault
> > > in service configuration or the service itself.
> > 
> > I do think this is an improvement, but I wonder if the verbosity should
> > be configurable. Then open sites like kernel.org could be friendlier to
> > their users. Something like this instead:
> 
> How about allow users to select which messages they want to print? We
> can even go further, allowing users to specify the messages themselves..

I thought about that, but it just seemed like it was making things way
more complex than it needed to be. GitHub does do this kind of
customization, but we also have a custom layer that intercepts git://
connections, anyway, so we added the relevant code there.

I don't know if medium-sized sites (i.e., ones that aren't so big they
are running custom proxies on the frontend) would care about adding
custom messages here or not.

> I don't know. I'm not a real server admin so maybe I'm just too
> paranoid. Any admins care to speak up?

I doubt anybody would care that much about turning individual messages
on and off. I think the real value is in being able to say "don't push
by git://. The right way to push to this site is...".

But your patch kind of falls short of what people would want to do for
two reasons:

  1. The message isn't dynamic at all. So I can't say:

        You tried to push to git://host.tld/foo.git. The right way to do
        that is:

          git push https://host.tld/foo.git

     That's what the GitHub message does if you try to push over git://;
     it gives you a new remote name that will actually work, customized
     to the repo you wanted to push to.

  2. Tweaking just the message for anything but "service not enabled"
     isn't all that useful. What do you say about "no such repository"
     in a simple message, even with placeholders?

     If you _really_ want to get fancy, a server could do a fuzzy
     search on the available repos and say "did you mean...?".
     But now we are talking about hooking arbitrary code into the
     message.

So if we want to do anything, I would think it would be a hook. Except
that we may or may not have a repo, so it would not be a hook in
$GIT_DIR/hooks, but rather some script to be run passed on the command
line, like:

  git daemon --informative-errors=/path/to/hook

-Peff

^ permalink raw reply

* Re: [BUG] git checkout <branch> allowed with uncommitted changes
From: Junio C Hamano @ 2011-10-13 18:28 UTC (permalink / raw)
  To: arQon; +Cc: git
In-Reply-To: <loom.20111013T193054-868@post.gmane.org>

arQon <arqon@gmx.com> writes:

> ..., in the absence of either of those flags, git
> WILL preserve the worktree by refusing to switch until that potentially-
> harmful situation is resolved by the user.

Perhaps you can prepare a documentation patch to make it clear that git
WILL preserve the LOCAL CHANGES to the working tree?

As it would already be clear to anybody reading this thread so far, local
changes made to the working tree do not belong to any particular branch.
They are floating on top, and it is up to the user what to do with these
floating changes when they conflict with the differences between the
branches you are switching across (i.e. you cannot switch so you need to
clean up by either committing, stashing, or deciding not to switch and
instead complete the work before you switch), and when they do not
conflict with the differences between the branches you are switching
across (i.e. you will carry them to the new working tree. It may be that
you made these changes and then realized that they do not belong to the
goal the current branch aims to achieve and that is why you decided to
switch to another branch, in which case you do not have to do anything
special in order to continue to work and complete it to commit to the
switched branch. It may be that you made these changes but needed to tend
to unrelated business on an unrelated branch and that is why you switched,
in which case you would want to clear them away, which is exactly what
stash was invented for).

^ permalink raw reply

* Re: [Bug] git pull doesn't recognize --work-tree parameter
From: Jeff King @ 2011-10-13 18:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Kirill Likhodedov, git
In-Reply-To: <7vbotk6aae.fsf@alter.siamese.dyndns.org>

On Thu, Oct 13, 2011 at 11:01:13AM -0700, Junio C Hamano wrote:

> I am not absolutely sure about "obviously correct", given that you assume
> that cd_to_toplevel does what its name makes you think it does.  I've been
> wondering if we want to do give a bit more sanity to "cd_to_toplevel" and
> "rev-parse --show-toplevel".
> 
>     $ pwd
>     /srv/project/git/git.git
>     $ (cd Documentation/howto && git rev-parse --show-toplevel); echo $?
>     /srv/project/git/git.git
>     0
> 
> So far so good, however:
> 
>     $ (cd .git/refs/heads && git rev-parse --show-toplevel); echo $?
>     0
> 
> I do not think this is quite right.

Ugh. You are right. I for some reason assumed that cd_to_toplevel would,
of all things, cd to the toplevel.  I think the right solution is to
introduce a "cd_to_work_tree_toplevel" (or similarly named) command that
always moves to the root of the work tree.

And then convert the two scripts in my patch to use it (along with the
change to require_work_tree_exists).  That would make my prior analysis
hold, then, as the annoying do-nothing behavior of "cd_to_toplevel" only
kicks in when we are outside the work tree (i.e., it could not have
happened before in those scripts, because the existing require_work_tree
call would cause us to die).

> We would probably want to add "rev-parse --show-work-tree", but we would
> need to audit the users of cd_to_toplevel before starting to use it.  I
> wouldn't be surprised if there is a script that creates a temporary work
> tree in .git/some/where and runs the scripted Porcelains without setting
> GIT_WORK_TREE, relying on the historical behaviour of cd_to_toplevel that
> does not really go to the top level.

Right. I suspect the proposed behavior for cd_to_toplevel is what they
all would want eventually, but some scripts may need minor tweaks. I
think we should follow the same path as require_work_tree_exists, and
introduce the new function, use it where we know it's safe, and then
eventually get rid of the old one.

The real trick is coming up with a good name, because cd_to_toplevel is
already taken. :)

-Peff

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox