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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html