Matthias Lederhofer <matled@xxxxxxx> writes:
> Here is a new patch series replacing ml/workdir.
>
> Changes:
> * rename GIT_WORK_DIR to GIT_WORK_TREE, --work-dir to --work-tree and
> core.workdir to core.worktree
> * use getcwd and prefixcmp instead of stat to check if cwd is below
> GIT_DIR or GIT_WORK_TREE
> * rename 'has_working_directory' to 'inside_working_tree'
>
> The first two patches have not changed except for their order in the
> series but I repost them anyway for easier review.
Thanks. I somehow felt funny to see my sign-off on some of your
patches, but it makes perfect sense with the above explanation.
A word of caution when working with me.
Almost always, I haven't thought things through more thoroughly
than the contributor did, so many of my comments of course show
a lot of cluelessness. Your explanation on the reason why you
structured is_bare_repository() check that way illustrated
issues to me and the list very well.
I often mix comments that are not serious suggestions and
questions, just to play devil's advocate. An earlier example
was "should the relative directory (either --work-dir option or
environment) be taken relative to GIT_DIR?" The expected answer
was "it should be relative to `pwd`, because expected use of
relative path at runtime (as opposed to setting in
$GIT_DIR/config) is one-shot, like this:
$ cd top-of-project/some/where
$ git --work-dir=../.. do-something
and making it relative to $GIT_DIR is just crazy, because the
top may not be related with $GIT_DIR in any easy-to-remember way
at all." I want to hear such rebuttal to make sure that the
contributor thought things through. Not amending certain parts
of the patch, with clear explanation to defend it, is always
better. Amending a patch only to satisfy such a non-serious
comment is to fail the test.
> Things I'm not sure about how/if to change (see my last mail too)
> * is_bare_repository() uses the old check
> /* definitely bare */
> if (is_bare_repository_cfg == 1)
> return 1;
> /* GIT_WORK_TREE is set, bare if cwd is outside */
> if (inside_working_tree >= 0)
> return !inside_working_tree;
> /* configuration says it is not bare */
> if (is_bare_repository_cfg == 0)
> return 0;
> * is_bare_repository() in general
There is a bit of chicken and egg involved in is_bare, because
we need to find out where GIT_DIR is in order to find where to
read GIT_DIR/config from, and only after reading the file we
would know if the user explicitly told us the repository is
bare, and setup_gently does not want to cd-up if the repository
is bare (i.e. there is no "top" to move to).
> * git init does not show the expansion of GIT_WORK_TREE to an
> absolute path, this might confuse the user
If the feature might confuse the user without extra output, it
probably is confusing to begin with, with or without it, so one
option might be to refuse relative path when running git-init.
However, as long as the expansion to absolute path is done
correctly, I do not think there is any room for confusion. Who
would want a feature that lets you set work-tree to ../.., so
that no matter where in the working tree you cd around, git
assumes that the toplevel is two level up? If you record the
work tree location in the configuration, you would want it to be
stable, and I do not think anybody would expect it be stored as
relative. So I thought having the printf() would be a good way
for debugging the absolute expansion, but after that I do not
think it is needed (but again, I may well be missing some issues
you've thought about, so please tell me otherwise).
-
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