Re: [PATCH 0/9] work-tree clean ups

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

so this is yet another revision of the work-tree clean ups (sorry to all 
those who grow tired of it; I feel with you: I am tired of it, too).

Junio rightfully pointed out that the tests do not succeed after each 
single step of the series.

Alas, after thinking about it for quite some time, I do not think there is 
any way around squashing all the earlier steps 4,6--9 into one step.

There is not really much that can be done about step 6/9: if we are in a 
work tree: that does not mean that we are _not_ in the git_dir.  (And no, 
this does not break git-clean, as a work tree is a work tree is a work 
tree.  If the user was stupid enough to specify the same directory as 
GIT_DIR and GIT_WORK_TREE, then that is _her_ problem.  Git is a powerful 
tool, and you can harm yourself with it.  Tough.)

Patch 7/9 is needed, because the old logic in git-init was wrong, and only 
hidden by the fact that the work-tree logic was implemented wrongly to 
begin with.

Patches 8 and 9/9 only updated the tests to ensure a sane logic, instead 
of an unsane one.  So they are needed, too.

Note: if you are in a bare repository (a repository which either says 
"core.bare = false" in the config, or which is a direct ancestor 
directory, i.e. ../[...]/.. of the current working directory) there will 
_not_ be an automatic working directory assignment.  You will be operating 
_without_ any work tree, unless you specify one.

I somehow feel that core.bare = true weighs more than core.worktree = 
/some/thing, and therefore I implemented it that way, but hey, if enough 
people disagree, then I'll change it.

Maybe I should add two comments?

Namely that setup_git_directory_gently() does _not_ check the config if 
the repository version is right, and where the working directory is, and 
if the repository is bare.  setup_git_directory() does...

And that setup_git_directory_gently() _does_ try to be smart about 
get_git_work_tree(), is_inside_git_dir() and is_inside_work_tree() by 
assigning their return values, and only if core.bare or core.worktree (or 
--work-tree=<something> or GIT_WORK_TREE) are set, get_git_work_tree() and 
is_inside_work_tree() are reset to recalculate what is happening...  
(actually, that is not completely true: if we _know_ that GIT_WORK_TREE is 
set, or --work-tree=<something> which is almost the same, we will defer 
the calculation until one of the functions get_git_work_tree() and 
is_inside_work_tree() is called.)

IMHO we should (probably after 1.5.3) change setup_git_directory_gently() 
to call check_repository_format() in every return path, so that we 
ascertain that the current repository is recent enough.  Because that 
function now checks also if the repo is bare, and if it has a worktree 
set, in addition to ensuring a valid repository.

In hindsight, I should have separated the "check .git/, then ./, and if no 
git_dir was found, continue with the parent directory" into a separate 
patch, but frankly, I am sick and tired of the work-tree series.  It was 
not done right in the first place, and it used hard-to-understand code to 
hide the fact.

Ciao,
Dscho

P.S.: After reading my patch to the tests, I have to disagree strongly 
with my notion that _not_ cleaning up the tests to use some sane syntax 
would make them clearer.  Nevertheless, I think I'll let them stand as an 
example how _not_ to write tests.