Re: [PATCH] git-new-workdir: support submodules

Re: [PATCH] git-new-workdir: support submodules

[Craig Silverstein]

[Ack, I forgot to cc myself on the original patch so now I can't reply
to it normally.  Hopefully my workaround doesn't mess up the threading
too badly.]

Junio C Hamano <gitster <at> pobox.com> writes:
>
> Hmmmm, does that mean that the submodule S in the original
> repository O's working tree and its checkout in the secondary
> working tree W created from O using git-new-workdir share the same
> repository location?  More specifically:
>
>     O/.git/                 - original repository
>         O/.git/index            - worktree state in O
>         O/S                     - submodule S's checkout in O
>         O/S/.git                - a gitfile pointing to O/.git/modules/S
>     O/.git/modules/S        - submodule S's repository contents
>         O/.git/modules/S/config - submodule S's config
>
>     W/.git/                 - secondary working tree
>         W/.git/config           - symlink to O/.git/config
>         W/.git/index            - worktree state in W (independent of O)
>     W/S                     - submodule S's checkout in W (independent of O)
>     W/S/.git                - a gitfile pointing to O/.git/modules/S

Right until the last line.  The .git file holds a relative path (at
least, it always does in my experience), so W/S/.git will point to
W/.git/modules/S.

Also, to complete the story, my changes sets the following:

        W/.git/modules/S    - secondary working tree for S
             W/.git/modules/S/config   - symlink to O/.git/modules/S/config
             W/.git/modules/S/index    - worktree state in W's S
(independent of O and O's S)

> Doesn't a submodule checkout keep some state tied to the working
> tree in its repository configuration file?

Do you mean, in 'config' itself?  If so, I don't see it.  (Though it's
possible there are ways to use submodules that do keep working-tree
state in the config file, and we just happen not to use those ways.)
Here's what my webapp/.git/modules/khan-exercises/config looks like:
---
[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
        worktree = ../../../khan-exercises
[remote "origin"]
        url = http://github.com/Khan/khan-exercises.git
        fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
        remote = origin
        merge = refs/heads/master
        rebase = true
[submodule "test/qunit"]
        url = https://github.com/jquery/qunit.git
--

The only thing that seems vaguely working-tree related is the
'worktree' field, which is the field that motivated me to set up my
patch the way it is.

> Wouldn't this change
> introduce problems by sharing O/.git/modules/S/config between the
> two checkouts?

It's true that this change does result in sharing that file, so if
that's problematic then you're right.  I'm afraid I don't know all the
things that can go into a submodule config file.

(There are other things I don't know as well, such as: do we see .git
files with 'gitdir: ...' contents only for submodules, or are there
other ways to create them as well?  Are 'gitdir' paths always
relative?  Are there special files in .git (or rather .git/modules/S)
that exist only for submodules and not for 'normal' repos, that we
need to worry about symlinking?  I apologize for not knowing all these
git internals, and hope you guys can help point out any gaps that
affect this patch!)

craig

Re: [PATCH] git-new-workdir: support submodules

[Craig Silverstein]

Ping! (now that the holidays are past)

craig

On Tue, Dec 23, 2014 at 1:51 PM, Craig Silverstein
<csilvers@khanacademy.org> wrote:
> [Ack, I forgot to cc myself on the original patch so now I can't reply
> to it normally.  Hopefully my workaround doesn't mess up the threading
> too badly.]
>
> Junio C Hamano <gitster <at> pobox.com> writes:
>>
>> Hmmmm, does that mean that the submodule S in the original
>> repository O's working tree and its checkout in the secondary
>> working tree W created from O using git-new-workdir share the same
>> repository location?  More specifically:
>>
>>     O/.git/                 - original repository
>>         O/.git/index            - worktree state in O
>>         O/S                     - submodule S's checkout in O
>>         O/S/.git                - a gitfile pointing to O/.git/modules/S
>>     O/.git/modules/S        - submodule S's repository contents
>>         O/.git/modules/S/config - submodule S's config
>>
>>     W/.git/                 - secondary working tree
>>         W/.git/config           - symlink to O/.git/config
>>         W/.git/index            - worktree state in W (independent of O)
>>     W/S                     - submodule S's checkout in W (independent of O)
>>     W/S/.git                - a gitfile pointing to O/.git/modules/S
>
> Right until the last line.  The .git file holds a relative path (at
> least, it always does in my experience), so W/S/.git will point to
> W/.git/modules/S.
>
> Also, to complete the story, my changes sets the following:
>
>         W/.git/modules/S    - secondary working tree for S
>              W/.git/modules/S/config   - symlink to O/.git/modules/S/config
>              W/.git/modules/S/index    - worktree state in W's S
> (independent of O and O's S)
>
>> Doesn't a submodule checkout keep some state tied to the working
>> tree in its repository configuration file?
>
> Do you mean, in 'config' itself?  If so, I don't see it.  (Though it's
> possible there are ways to use submodules that do keep working-tree
> state in the config file, and we just happen not to use those ways.)
> Here's what my webapp/.git/modules/khan-exercises/config looks like:
> ---
> [core]
>         repositoryformatversion = 0
>         filemode = true
>         bare = false
>         logallrefupdates = true
>         worktree = ../../../khan-exercises
> [remote "origin"]
>         url = http://github.com/Khan/khan-exercises.git
>         fetch = +refs/heads/*:refs/remotes/origin/*
> [branch "master"]
>         remote = origin
>         merge = refs/heads/master
>         rebase = true
> [submodule "test/qunit"]
>         url = https://github.com/jquery/qunit.git
> --
>
> The only thing that seems vaguely working-tree related is the
> 'worktree' field, which is the field that motivated me to set up my
> patch the way it is.
>
>> Wouldn't this change
>> introduce problems by sharing O/.git/modules/S/config between the
>> two checkouts?
>
> It's true that this change does result in sharing that file, so if
> that's problematic then you're right.  I'm afraid I don't know all the
> things that can go into a submodule config file.
>
> (There are other things I don't know as well, such as: do we see .git
> files with 'gitdir: ...' contents only for submodules, or are there
> other ways to create them as well?  Are 'gitdir' paths always
> relative?  Are there special files in .git (or rather .git/modules/S)
> that exist only for submodules and not for 'normal' repos, that we
> need to worry about symlinking?  I apologize for not knowing all these
> git internals, and hope you guys can help point out any gaps that
> affect this patch!)
>
> craig

Re: [PATCH] git-new-workdir: support submodules

[Junio C Hamano]

Craig Silverstein <csilvers@khanacademy.org> writes:

>>> Doesn't a submodule checkout keep some state tied to the working
>>> tree in its repository configuration file?
>>
>> Do you mean, in 'config' itself?  If so, I don't see it.  (Though it's
>> possible there are ways to use submodules that do keep working-tree
>> state in the config file, and we just happen not to use those ways.)
>> Here's what my webapp/.git/modules/khan-exercises/config looks like:
>> ---
>> [core]
>>         repositoryformatversion = 0
>>         filemode = true
>>         bare = false
>>         logallrefupdates = true
>>         worktree = ../../../khan-exercises
>> [remote "origin"]
>>         url = http://github.com/Khan/khan-exercises.git
>>         fetch = +refs/heads/*:refs/remotes/origin/*
>> [branch "master"]
>>         remote = origin
>>         merge = refs/heads/master
>>         rebase = true
>> [submodule "test/qunit"]
>>         url = https://github.com/jquery/qunit.git
>> --
>>
>> The only thing that seems vaguely working-tree related is the
>> 'worktree' field, which is the field that motivated me to set up my
>> patch the way it is.

That is the location of the working tree of the top-level
superproject.  Tied to the state of the submodule working tree
appear in [submodule "test/qunit"] part.

In one new-workdir checkout, that submodule may be "submodule
init"ed, while another one, it may not be.

Or one new-workdir checkout's branch may check out a top-level
project from today while the other one may have a top-level project
from two years ago, and between these two checkouts of the top-level
project, the settings of submodule."test/qunit".* variables may have
to be different (perhaps even URL may have to point at two different
repositories, one historical one to grab the state two years ago,
the other current one).

So sharing config between top-level checkouts may not be enough to
"support submodules" (the patch title).

Re: [PATCH] git-new-workdir: support submodules

[Craig Silverstein]

} Or one new-workdir checkout's branch may check out a top-level
} project from today while the other one may have a top-level project
} from two years ago,

This is also true, but just as much a problem with the 'git
new-workdir' script as it existed before my change.  It already
symlinks the top-level .git/config directory, which lists a remote,
submodules, and many other things.  Does symlinking the config file
for submodules add any new wrinkles, that symlinking the config file
for the top-level repository does not?

craig

On Fri, Jan 23, 2015 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Craig Silverstein <csilvers@khanacademy.org> writes:
>
>>>> Doesn't a submodule checkout keep some state tied to the working
>>>> tree in its repository configuration file?
>>>
>>> Do you mean, in 'config' itself?  If so, I don't see it.  (Though it's
>>> possible there are ways to use submodules that do keep working-tree
>>> state in the config file, and we just happen not to use those ways.)
>>> Here's what my webapp/.git/modules/khan-exercises/config looks like:
>>> ---
>>> [core]
>>>         repositoryformatversion = 0
>>>         filemode = true
>>>         bare = false
>>>         logallrefupdates = true
>>>         worktree = ../../../khan-exercises
>>> [remote "origin"]
>>>         url = http://github.com/Khan/khan-exercises.git
>>>         fetch = +refs/heads/*:refs/remotes/origin/*
>>> [branch "master"]
>>>         remote = origin
>>>         merge = refs/heads/master
>>>         rebase = true
>>> [submodule "test/qunit"]
>>>         url = https://github.com/jquery/qunit.git
>>> --
>>>
>>> The only thing that seems vaguely working-tree related is the
>>> 'worktree' field, which is the field that motivated me to set up my
>>> patch the way it is.
>
> That is the location of the working tree of the top-level
> superproject.  Tied to the state of the submodule working tree
> appear in [submodule "test/qunit"] part.
>
> In one new-workdir checkout, that submodule may be "submodule
> init"ed, while another one, it may not be.
>
> Or one new-workdir checkout's branch may check out a top-level
> project from today while the other one may have a top-level project
> from two years ago, and between these two checkouts of the top-level
> project, the settings of submodule."test/qunit".* variables may have
> to be different (perhaps even URL may have to point at two different
> repositories, one historical one to grab the state two years ago,
> the other current one).
>
> So sharing config between top-level checkouts may not be enough to
> "support submodules" (the patch title).

Re: [PATCH] git-new-workdir: support submodules

[Junio C Hamano]

Craig Silverstein <csilvers@khanacademy.org> writes:

> } Or one new-workdir checkout's branch may check out a top-level
> } project from today while the other one may have a top-level project
> } from two years ago,
>
> This is also true, but just as much a problem with the 'git
> new-workdir' script as it existed before my change.  It already
> symlinks the top-level .git/config directory, which lists a remote,
> submodules, and many other things.  Does symlinking the config file
> for submodules add any new wrinkles, that symlinking the config file
> for the top-level repository does not?

The update under discussion is labeled as "support submodules";
presumably the only reason that such an update is good is because it
will fix existing problems that makes the use of the script break
when submodules are involved --- submodules are not supported with
the current code, and this patch fixes the code to support it.

But then, you are saying that the update does not fix these existing
issues around submodule support.  So...?

Re: [PATCH] git-new-workdir: support submodules

[Craig Silverstein]

> But then, you are saying that the update does not fix these existing
> issues around submodule support.  So...?

I guess my point is that the existing contrib script has proven to be
useful to people, even though it imposes these constraints on clients
wrt the config file (namely, you can't have multiple workdirs that
need different values in the config file).  This patch, in adding
submodule support, I expect would be similarly useful to people even
though it, also, imposes those same constraints to the submodule's
config files.

I guess you'd rather see these config file issues fixed for all use
cases?  If so, I'm probably not the right person since I do not know
enough about how config files are used in git -- I fear any changes I
made would make some things worse for (some) existing clients of the
script, which is not what I want.  It sounds like this functionality
is being reimplemented in git proper in any case, so perhaps it's best
just to wait for that.  I don't know what its submodule support will
be, though.

craig

Re: [PATCH] git-new-workdir: support submodules

[Junio C Hamano]

Craig Silverstein <csilvers@khanacademy.org> writes:

> This patch, in adding submodule support, I expect would be
> similarly useful to people even though it, also, imposes those
> same constraints to the submodule's config files.

I would expect that you would see exactly the same issue with Duy's
multiple work tree series.  This is not limited to new-workdir.

The right way to look at this is to fix what "git submodule" does;
its use of "config" that is shared across branches is the root cause
of the trouble.  No other part of Git keeps data that needs to be
per-branch (or more specifically "tied to the working tree state")
in .git/config in such a way that leaving it stale when the working
tree state changes breaks the system.

One way to do so might be to move the bits it stores in the config
file to somewhere else that is more closely tied to the checkout
state and handle that similar to .git/index and .git/HEAD when it
comes to multiple work-trees.

Re: [PATCH] git-new-workdir: support submodules

[Jens Lehmann]

Am 26.01.2015 um 06:39 schrieb Junio C Hamano:
> Craig Silverstein <csilvers@khanacademy.org> writes:
>
>> This patch, in adding submodule support, I expect would be
>> similarly useful to people even though it, also, imposes those
>> same constraints to the submodule's config files.
>
> I would expect that you would see exactly the same issue with Duy's
> multiple work tree series.  This is not limited to new-workdir.
>
> The right way to look at this is to fix what "git submodule" does;
> its use of "config" that is shared across branches is the root cause
> of the trouble.  No other part of Git keeps data that needs to be
> per-branch (or more specifically "tied to the working tree state")
> in .git/config in such a way that leaving it stale when the working
> tree state changes breaks the system.

I'm not sure what submodule config shared across branches you are
referring to here, the only two things that submodules by default
store in .git/config are the url (to mark the submodule initialized)
and the update setting (which I believe should be copied there in
the first place, but changing that in a correct and backwards
compatible would take some effort). This means you'll init (or
deinit) a submodule in all multiple worktrees at the same time, but
having the url setting in .git/config is a feature, not a bug.

And if you are talking about the core.worktree setting in
.git/modules/<submodule_name>/config, I proposed way to get rid of
that when we moved the submodule .git directory there (but that
wasn't accepted):

    http://permalink.gmane.org/gmane.comp.version-control.git/188007

Additionally I suspect submodules are just the first to suffer,
other worktree specific config (like e.g. EOL settings) affecting
all multiple worktrees at the same time when changed in a single
worktree might easily confuse other users too.

> One way to do so might be to move the bits it stores in the config
> file to somewhere else that is more closely tied to the checkout
> state and handle that similar to .git/index and .git/HEAD when it
> comes to multiple work-trees.

Yup, the idea of separating config entries into worktree and repo
specific files sounds like the solution for these problems.

Re: [PATCH] git-new-workdir: support submodules

[Duy Nguyen]

On Tue, Jan 27, 2015 at 06:35:41PM +0100, Jens Lehmann wrote:
> > One way to do so might be to move the bits it stores in the config
> > file to somewhere else that is more closely tied to the checkout
> > state and handle that similar to .git/index and .git/HEAD when it
> > comes to multiple work-trees.
> 
> Yup, the idea of separating config entries into worktree and repo
> specific files sounds like the solution for these problems.

OK, would this work? This PoC patch adds a config key namespace
local.* that is written to $GIT_DIR/config.local instead of
$GIT_DIR/config (I still need to ban local.* in global config
files). This config.local is loaded togethr with $GIT_DIR/config.

For nd/multiple-work-trees we can transparently map
$GIT_DIR/config.local to $SUPER/worktrees/<id>/config.local. For
git-new-workdir.sh, perhaps we can teach include.path to make
config.local path relative to where the config symlink is, not where
the symlink target is.

We can then teach setup.c to read local.core.worktree if core.worktree
is not present, and more apdation in git-submodule.sh.. I don't expect
big changes because git-submodule.sh just needs to read some other
config keys and dont need to care if git-new-workdir.sh or
nd/multiple-work-trees is being used.

-- 8< --
diff --git a/config.c b/config.c
index 752e2e2..237bd8e 100644
--- a/config.c
+++ b/config.c
@@ -1177,6 +1177,15 @@ int git_config_system(void)
 	return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
 }
 
+static int config_local_filter(const char *var, const char *value, void *data)
+{
+	struct config_include_data *inc = data;
+
+	if (!starts_with(var, "local."))
+		return error("$GIT_DIR/config.local can only contain local.*");
+	return inc->fn(var, value, inc->data);
+}
+
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
 	int ret = 0, found = 0;
@@ -1202,8 +1211,19 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 	}
 
 	if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
+		char *wt_config;
 		ret += git_config_from_file(fn, repo_config, data);
 		found += 1;
+		wt_config = git_pathdup("config.local");
+		if (!access_or_die(wt_config, R_OK, 0)) {
+			struct config_include_data inc = CONFIG_INCLUDE_INIT;
+			inc.fn = fn;
+			inc.data = data;
+			ret += git_config_from_file(config_local_filter,
+						    wt_config, &inc);
+			found += 1;
+		}
+		free(wt_config);
 	}
 
 	switch (git_config_from_parameters(fn, data)) {
@@ -1942,8 +1962,12 @@ int git_config_set_multivar_in_file(const char *config_filename,
 
 	store.multi_replace = multi_replace;
 
-	if (!config_filename)
-		config_filename = filename_buf = git_pathdup("config");
+	if (!config_filename) {
+		if (starts_with(key, "local."))
+			config_filename = filename_buf = git_pathdup("config.local");
+		else
+			config_filename = filename_buf = git_pathdup("config");
+	}
 
 	/*
 	 * The lock serves a purpose in addition to locking: the new
-- 8< --

Re: [PATCH] git-new-workdir: support submodules

[Duy Nguyen]

On Wed, Jan 28, 2015 at 5:50 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> For git-new-workdir.sh, perhaps we can teach include.path to make
> config.local path relative to where the config symlink is, not where
> the symlink target is.

Ignore this part. I originally wanted to use include.path to load
config.local, but there was an easier way. With this patch, I think
config.local is already per worktree that is created by
git-new-workdir.sh.
-- 
Duy

Re: [PATCH] git-new-workdir: support submodules

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Jan 28, 2015 at 5:50 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> For git-new-workdir.sh, perhaps we can teach include.path to make
>> config.local path relative to where the config symlink is, not where
>> the symlink target is.
>
> Ignore this part. I originally wanted to use include.path to load
> config.local, but there was an easier way. With this patch, I think
> config.local is already per worktree that is created by
> git-new-workdir.sh.

I think this may give us a cleaner approach to support a "keep
multiple working trees from the same project that are configured
differently" setting.

I am not sure how well it meshes to call this as ".local" with the
"--local" option of "git config" (read: "local" is a loaded word,
and the way we have used it have been "per repository", but now you
are using it to mean "per working tree" that is finer grained),
though.

I hope that there aren't people that are crazy enough to bind the
same repository to two different places in the top-level project,
and insist that we (re)use the submodule repository ;-).

Re: [PATCH] git-new-workdir: support submodules

[Junio C Hamano]

Craig Silverstein <csilvers.mail@gmail.com> writes:

> The basic problem with submodules, from git-new-workdir's point of
> view, is that instead of having a .git directory, they have a .git
> file with contents `gitdir: <some other path>`.  This is a problem
> because the submodule's config file has an entry like `worktree =
> ../../../khan-exercises` which is relative to "<some other path>"
> rather than to "submodule_dir/.git".
>
> As a result, if we want the new workdir to work properly, it needs to
> keep the same directory structure as the original repository: it
> should also contain a .git file with a 'gitdir', and the actual .git
> contents should be in the place mentioned therein.

Hmmmm, does that mean that the submodule S in the original
repository O's working tree and its checkout in the secondary
working tree W created from O using git-new-workdir share the same
repository location?  More specifically:

	O/.git/                 - original repository
        O/.git/index            - worktree state in O
        O/S                     - submodule S's checkout in O
        O/S/.git                - a gitfile pointing to O/.git/modules/S
	O/.git/modules/S        - submodule S's repository contents
        O/.git/modules/S/config - submodule S's config

	W/.git/                 - secondary working tree
        W/.git/config           - symlink to O/.git/config
        W/.git/index            - worktree state in W (independent of O)
	W/S                     - submodule S's checkout in W (independent of O)
	W/S/.git                - a gitfile pointing to O/.git/modules/S

Doesn't a submodule checkout keep some state tied to the working
tree in its repository configuration file?  Wouldn't this change
introduce problems by sharing O/.git/modules/S/config between the
two checkouts?

[PATCH] git-new-workdir: support submodules

[Craig Silverstein]

The basic problem with submodules, from git-new-workdir's point of
view, is that instead of having a .git directory, they have a .git
file with contents `gitdir: <some other path>`.  This is a problem
because the submodule's config file has an entry like `worktree =
../../../khan-exercises` which is relative to "<some other path>"
rather than to "submodule_dir/.git".

As a result, if we want the new workdir to work properly, it needs to
keep the same directory structure as the original repository: it
should also contain a .git file with a 'gitdir', and the actual .git
contents should be in the place mentioned therein.  (There are other
ways we could have solved this problem, including modifying the
'config' file, but this seemed most in keeping with the symlink-y
philosophy of git-new-branch.)

This commit implements this, by detecting if the source .git directory
is actually a .git file with 'gitdir: ...' as its contents, and if so
reproducing the .git file + gitdir structure in the destination
work-tree.

Test plan:
On a repo (~/khan/webapp) with a submodule, ran

    git new-workdir ~/khan/webapp /tmp/webapp      # the main module
    git new-workdir ~/khan/webapp/khan-exercises /tmp/webapp/khan-exercises

and saw that /tmp/webapp/khan-exercises was populated correctly,
/tmp/webapp/.git/modules/khan-exercises existed with symlinks, and
/tmp/webapp/khan-exercises/.git was a file with a 'gitdir:' entry
pointing to the .git/modules directory.

Signed-off-by: Craig Silverstein <csilvers@khanacademy.org>
---
 contrib/workdir/git-new-workdir | 55 +++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 19 deletions(-)

diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
index 888c34a..3cc50de 100755
--- a/contrib/workdir/git-new-workdir
+++ b/contrib/workdir/git-new-workdir
@@ -52,26 +52,45 @@ then
 		"a complete repository."
 fi
 
+# don't modify an existing directory, unless it's empty
+if test -d "$new_workdir" && test $(ls -a1 "$new_workdir/." | wc -l) -ne 2
+then
+	die "destination directory '$new_workdir' is not empty."
+fi
+
 # make sure the links in the workdir have full paths to the original repo
 git_dir=$(cd "$git_dir" && pwd) || exit 1
 
-# don't recreate a workdir over an existing directory, unless it's empty
-if test -d "$new_workdir"
+new_git_dir="$new_workdir/.git"
+
+# if $orig_git is a .git file with a 'gitdir' entry (as is the case for
+# submodules), have the new git dir follow that same pattern.  otherwise
+# the 'worktree' entry in .git/config, which is a relative path, will
+# not resolve properly because we're not in the expected subdirectory.
+gitdir_text=$(sed -ne 's/^gitdir: *//p' "$orig_git/.git" 2>/dev/null)
+if test -n "$gitdir_text"; then
+	ln -s "$orig_git/.git" "$new_workdir/.git" || failed
+	new_git_dir="$new_workdir/$gitdir_text"
+fi
+
+# if new_workdir already exists, leave it along in case of error
+if ! test -d "$new_workdir"
 then
-	if test $(ls -a1 "$new_workdir/." | wc -l) -ne 2
-	then
-		die "destination directory '$new_workdir' is not empty."
-	fi
-	cleandir="$new_workdir/.git"
-else
-	cleandir="$new_workdir"
+	clean_new_workdir=true
 fi
 
-mkdir -p "$new_workdir/.git" || failed
-cleandir=$(cd "$cleandir" && pwd) || failed
+mkdir -p "$new_git_dir" || failed
 
+cleandir=$(cd "$cleandir" && pwd) || failed
 cleanup () {
-	rm -rf "$cleandir"
+	if test z"$clean_new_workdir" = ztrue
+	then
+		rm -rf "$new_workdir"
+	fi
+	# this may (or may not) be a noop if new_workdir was already deleted.
+	rm -rf "$new_git_dir"
+	# this is a noop unless .git is a 'gitdir: ...' file.
+	rm -f "$new_workdir/.git"
 }
 siglist="0 1 2 15"
 trap cleanup $siglist
@@ -84,22 +103,20 @@ do
 	# create a containing directory if needed
 	case $x in
 	*/*)
-		mkdir -p "$new_workdir/.git/${x%/*}"
+		mkdir -p "$new_git_dir/${x%/*}"
 		;;
 	esac
 
-	ln -s "$git_dir/$x" "$new_workdir/.git/$x" || failed
+	ln -s "$git_dir/$x" "$new_git_dir/$x" || failed
 done
 
-# commands below this are run in the context of the new workdir
-cd "$new_workdir" || failed
-
 # copy the HEAD from the original repository as a default branch
-cp "$git_dir/HEAD" .git/HEAD || failed
+cp "$git_dir/HEAD" "$new_git_dir/HEAD" || failed
 
 # the workdir is set up.  if the checkout fails, the user can fix it.
 trap - $siglist
 
 # checkout the branch (either the same as HEAD from the original repository,
-# or the one that was asked for)
+# or the one that was asked for).  we must be in the new workdir for this.
+cd "$new_workdir" || failed
 git checkout -f $branch
-- 
2.2.1