[RFC/PATCH] Add multiple workdir support to branch/checkout

[RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

When using 'git new-workdir', there is no safety mechanism to prevent the
same branch from being checked out twice, nor to prevent a checked out
branch from being deleted.

By teaching 'checkout' to record the workdir path using
'branch.<name>.checkout' when switching branches, we can easily check if a
branch is already checked out in another workdir before switching to that
branch. Similarly, we can now add a check before deleting a branch.

Allow 'checkout -f' to force the checkout and issue a warning
instead of an error.

Guard this behavior behind 'core.recordCheckouts', which we will
teach 'git new-workdir' to set in a followup commit.

Note: when switching away from a branch, we set 'branch.<name>.checkout'
to the empty string, instead of deleting it entirely, since git_config()
otherwise leaves behind an empty section which it does not re-use.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin/branch.c   |   10 ++++++++++
 builtin/checkout.c |   39 +++++++++++++++++++++++++++++++++++++++
 remote.c           |    4 ++++
 remote.h           |    1 +
 4 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index f49596f826..6ce1a5b133 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -182,6 +182,16 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 			ret = 1;
 			continue;
 		}
+		if (kinds == REF_LOCAL_BRANCH) {
+			struct branch *branch = branch_get(bname.buf);
+			if (branch->work_tree && strlen(branch->work_tree)) {
+				error(_("Cannot delete the branch '%s' "
+					"which is currently checked out in '%s'"),
+				      bname.buf, branch->work_tree);
+				ret = 1;
+				continue;
+			}
+		}
 
 		free(name);
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5e356a6c61..26259a41a7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -33,6 +33,7 @@ struct checkout_opts {
 	int force_detach;
 	int writeout_stage;
 	int writeout_error;
+	int record_checkouts;
 
 	/* not set by parse_options */
 	int branch_exists;
@@ -709,12 +710,35 @@ static void orphaned_commit_warning(struct commit *commit)
 	for_each_ref(clear_commit_marks_from_one_ref, NULL);
 }
 
+static void record_checkout(const char *name, const char *work_tree)
+{
+	struct strbuf key = STRBUF_INIT;
+	strbuf_addf(&key, "branch.%s.checkout", name);
+	git_config_set(key.buf, work_tree);
+	strbuf_release(&key);
+}
+
+static void check_if_checked_out(struct checkout_opts *opts, const char *name)
+{
+	struct branch *branch = branch_get(name);
+	if (branch->work_tree && strlen(branch->work_tree) &&
+	    strcmp(branch->work_tree, get_git_work_tree())) {
+		if (opts->force)
+			warning(_("branch '%s' is currently checked out"
+				  " in '%s'"), name, branch->work_tree);
+		else
+			die(_("branch '%s' is currently checked out"
+			      " in '%s'"), name, branch->work_tree);
+	}
+}
+
 static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 {
 	int ret = 0;
 	struct branch_info old;
 	unsigned char rev[20];
 	int flag;
+
 	memset(&old, 0, sizeof(old));
 	old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag));
 	old.commit = lookup_commit_reference_gently(rev, 1);
@@ -734,6 +758,9 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 		parse_commit(new->commit);
 	}
 
+	if (opts->record_checkouts)
+		check_if_checked_out(opts, new->name);
+
 	ret = merge_working_tree(opts, &old, new);
 	if (ret)
 		return ret;
@@ -743,6 +770,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 
 	update_refs_for_switch(opts, &old, new);
 
+	if (opts->record_checkouts) {
+		const char *work_tree = get_git_work_tree();
+		struct branch *branch = branch_get(old.name);
+		if (branch->work_tree && !strcmp(branch->work_tree, work_tree))
+			record_checkout(old.name, "");
+		record_checkout(new->name, work_tree);
+	}
+
 	ret = post_checkout_hook(old.commit, new->commit, 1);
 	free((char *)old.path);
 	return ret || opts->writeout_error;
@@ -756,6 +791,10 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.recordcheckouts")) {
+		struct checkout_opts *opts = cb;
+		opts->record_checkouts = git_config_bool(var, value);
+	}
 	if (!prefixcmp(var, "submodule."))
 		return parse_submodule_config_option(var, value);
 
diff --git a/remote.c b/remote.c
index b8ecfa5d95..2bc063dae8 100644
--- a/remote.c
+++ b/remote.c
@@ -364,6 +364,10 @@ static int handle_config(const char *key, const char *value, void *cb)
 			if (!value)
 				return config_error_nonbool(key);
 			add_merge(branch, xstrdup(value));
+		} else if (!strcmp(subkey, ".checkout")) {
+			if (!value)
+				return config_error_nonbool(key);
+			branch->work_tree = xstrdup(value);
 		}
 		return 0;
 	}
diff --git a/remote.h b/remote.h
index 9a30a9dba6..4103ec7e31 100644
--- a/remote.h
+++ b/remote.h
@@ -126,6 +126,7 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec);
 struct branch {
 	const char *name;
 	const char *refname;
+	const char *work_tree;
 
 	const char *remote_name;
 	struct remote *remote;
-- 
1.7.7.4.g39e02c

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Tue, Oct 4, 2011 at 11:43 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> When using 'git new-workdir', there is no safety mechanism to prevent the
> same branch from being checked out twice, nor to prevent a checked out
> branch from being deleted.
>
> By teaching 'checkout' to record the workdir path using
> 'branch.<name>.checkout' when switching branches, we can easily check if a
> branch is already checked out in another workdir before switching to that
> branch. Similarly, we can now add a check before deleting a branch.
>
> Allow 'checkout -f' to force the checkout and issue a warning
> instead of an error.
>
> Guard this behavior behind 'core.recordCheckouts', which we will
> teach 'git new-workdir' to set in a followup commit.

Well, depending upon what folks think of this RFC, anyway.

> Note: when switching away from a branch, we set 'branch.<name>.checkout'
> to the empty string, instead of deleting it entirely, since git_config()
> otherwise leaves behind an empty section which it does not re-use.

Maybe this is a bug in git_config()? It seems like if it's removed the
last item from a section, it should remove the whole section OR it
should re-use an empty section.

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Nguyen Thai Ngoc Duy]

On Wed, Oct 5, 2011 at 2:43 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> When using 'git new-workdir', there is no safety mechanism to prevent the
> same branch from being checked out twice, nor to prevent a checked out
> branch from being deleted.
>
> By teaching 'checkout' to record the workdir path using
> 'branch.<name>.checkout' when switching branches, we can easily check if a
> branch is already checked out in another workdir before switching to that
> branch. Similarly, we can now add a check before deleting a branch.
>
> Allow 'checkout -f' to force the checkout and issue a warning
> instead of an error.
>
> Guard this behavior behind 'core.recordCheckouts', which we will
> teach 'git new-workdir' to set in a followup commit.

I've wanted to to something like this, but you beat me to it ;)

Could you please consider a more generic approach? What I have in mind
is a mechanism to "lock" a branch, so that only commands that have the
key can update it.

So instead of branch.<name>.checkout, I would have something like
branch.<name>.locked = <key>, where <key> is just a string. Only
commands that provide the matching <key> are allowed to update the
branch. In checkout case, <key> could be "checkout: worktree".

This approach addresses more cases than just multiple workdir. We
could relax restrictions on pushing to a non-bare repository: we only
disallow pushing to locked branches. We can also use this to prevent
users from checking out another branch (by locking HEAD) while in the
middle of interactive rebase/bisect/...
-- 
Duy

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Wed, Oct 5, 2011 at 12:02 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> Could you please consider a more generic approach? What I have in mind
> is a mechanism to "lock" a branch, so that only commands that have the
> key can update it.
>
> So instead of branch.<name>.checkout, I would have something like
> branch.<name>.locked = <key>, where <key> is just a string. Only
> commands that provide the matching <key> are allowed to update the
> branch. In checkout case, <key> could be "checkout: worktree".

In this case, each workdir needs its own key, so I'd have to record
the key somewhere, unless you meant using a key of "checkout:
</path/to/workdir>".

> This approach addresses more cases than just multiple workdir. We
> could relax restrictions on pushing to a non-bare repository: we only
> disallow pushing to locked branches.

Isn't that another case where you only care if the branch is checked
out and where? So using "branch.<name>.checkout = </path/to/workdir>"
should be fine there too.

> We can also use this to prevent
> users from checking out another branch (by locking HEAD) while in the
> middle of interactive rebase/bisect/...

I dunno, that seems like a really different use case.

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Wed, Oct 5, 2011 at 12:02 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>> Could you please consider a more generic approach? What I have in mind
>> is a mechanism to "lock" a branch, so that only commands that have the
>> key can update it.
>>
>> So instead of branch.<name>.checkout, I would have something like
>> branch.<name>.locked = <key>, where <key> is just a string. Only
>> commands that provide the matching <key> are allowed to update the
>> branch. In checkout case, <key> could be "checkout: worktree".
>
> In this case, each workdir needs its own key, so I'd have to record
> the key somewhere, unless you meant using a key of "checkout:
> </path/to/workdir>".

That actually is how I read his message.

I do not think "we cannot off the top of our heads think of the reason
other than the branch is checked out that we might want to forbid its
update" is a very good excuse to cast the word "checkout" in the UI; you
would paint yourself in a difficult corner that you have to expend more
energy to get out of by later adding backward compatibility support.

I think "switch_branches()" that updates HEAD to point at a local branch
is one good place to lock the branch, but I do not know if it is a good
idea to hook the check into the codepaths for deletion of the branch using
"branch -[dD]" and check-out of the branch using "checkout $branch". I
wonder if it makes sense to add the "checking" hook into much lower level
in the callchain, perhaps delete_ref(), rename_ref() and update_ref() to
catch attempts to update "your" current branch by other people. For that
matter, instead of switch_branches(), would it make more sense to add this
lock/unlock logic to symbolic_ref() that repoints HEAD to other branch?

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Wed, Oct 5, 2011 at 12:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> In this case, each workdir needs its own key, so I'd have to record
>> the key somewhere, unless you meant using a key of "checkout:
>> </path/to/workdir>".
>
> That actually is how I read his message.
>
> I do not think "we cannot off the top of our heads think of the reason
> other than the branch is checked out that we might want to forbid its
> update" is a very good excuse to cast the word "checkout" in the UI; you
> would paint yourself in a difficult corner that you have to expend more
> energy to get out of by later adding backward compatibility support.

Git has survived w/o needing to lock branches till now. What are these
use cases we cannot already think of today?

> I think "switch_branches()" that updates HEAD to point at a local branch
> is one good place to lock the branch, but I do not know if it is a good
> idea to hook the check into the codepaths for deletion of the branch using
> "branch -[dD]" and check-out of the branch using "checkout $branch". I
> wonder if it makes sense to add the "checking" hook into much lower level
> in the callchain, perhaps delete_ref(), rename_ref() and update_ref() to
> catch attempts to update "your" current branch by other people.

I don't think so. There are lots of ways to shoot yourself in the foot
at the plumbing level. Besides, this is not about all refs, just local
branches.

Aside, there's nothing wrong with renaming a checked out branch.

> For that
> matter, instead of switch_branches(), would it make more sense to add this
> lock/unlock logic to symbolic_ref() that repoints HEAD to other branch?

I think you mean create_symref()? Looking at it's callers that seems
too low-level.

Maybe you could sketch out how you think this should work, I'm not seeing it.

- Where/how should the lock be recorded?
- Which function(s) should record/release the lock?

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> Git has survived w/o needing to lock branches till now.

Careful. Git has survived without your patch series till now, as people
learned to be careful when they use separate workdirs and avoid certain
things, to the point that they are not necessarily aware that they are
avoiding them (one good practice is to keep the HEADs of non-primary
workdirs detached).

Does that mean what your patch aims to do is unnecessary? I think not.

> What are these
> use cases we cannot already think of today?

What is important is that we should have learned by now that the "gotchas"
live where we do not immediately see. "Can you tell me what you are missing?"
is a senseless thing to ask.

>> I think "switch_branches()" that updates HEAD to point at a local branch
>> is one good place to lock the branch, but I do not know if it is a good
>> idea to hook the check into the codepaths for deletion of the branch using
>> "branch -[dD]" and check-out of the branch using "checkout $branch". I
>> wonder if it makes sense to add the "checking" hook into much lower level
>> in the callchain, perhaps delete_ref(), rename_ref() and update_ref() to
>> catch attempts to update "your" current branch by other people.
>
> I don't think so. There are lots of ways to shoot yourself in the foot
> at the plumbing level. Besides, this is not about all refs, just local
> branches.
>
> Aside, there's nothing wrong with renaming a checked out branch.

There are pros and cons between hooking at lower level vs higher
level. The advantage of hooking at higher level is you do not risk
breaking low-level operations, but that directly results in allowing the
same low level operations that are unaware of the new requirement higher
level added break it. It also allows other high level operations you
forgot to teach the new requirement break it.

For example, you checkout branch frotz in a workdir, and then in the
primary repository that has nitfol branch checked out, you rename the
frotz branch to xyzzy. The HEAD of workdir still says refs/heads/frotz
that no longer exist. Of course you can break the same way by doing a
"update-ref -d refs/heads/frotz" from the primary repository.

Because you forgot that the high level operation "branch renaming" needs
to be aware of that "this branch is checked out elsewhere" information,
you allowed it to break the workdir. If you hooked into lower level
machinery that is shared, you wouldn't have caused this breakage.
Similarly, if delete_ref() were taught about the new requirement, you
would have covered both "branch -d" and "update-ref -d".

I do not necessarily think that it is a good approach to forbid the same
branch to be checked out in two different places, by the way. One reason
people would want to keep multiple workdirs is so that while they are
still working on a branch and are not yet at a good "stop point" to even
make a temporary commit to get interrupted, they find it sometimes
necessary to be able to build the tip of that same branch and even make a
small in-working-tree fixes (which later will be carried back to the
primary branch). The problem arises only when one of the repositories try
to update or delete the branch while it is checked out in another working
tree.

Can this series be extended/reworked so that:

 - Each branch has multi-value configuration record to note the workdirs
   that it is checked out;

 - Error out (or warn if forced) upon any attempt to update the tip of a
   branch that is checked out in more than one place; and

 - Similarly for renaming or deleting a branch that is checked out in more
   than one place.
   

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Wed, Oct 5, 2011 at 2:19 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Careful. Git has survived without your patch series till now, as people
> learned to be careful when they use separate workdirs and avoid certain
> things, to the point that they are not necessarily aware that they are
> avoiding them (one good practice is to keep the HEADs of non-primary
> workdirs detached).

I think it's more likely the case that most people have avoided
new-workdir entirely.

Also, while I might recommend new-workdir to my coworkers with the
advice "don't checkout the same branch in multiple workdirs", never in
a million years would I say "use new-workdir, but make sure to only
use a detached HEAD in the workdirs." The latter would make their
actual HEADs explode. :-)

> For example, you checkout branch frotz in a workdir, and then in the
> primary repository that has nitfol branch checked out, you rename the
> frotz branch to xyzzy. The HEAD of workdir still says refs/heads/frotz
> that no longer exist. Of course you can break the same way by doing a
> "update-ref -d refs/heads/frotz" from the primary repository.
>
> Because you forgot that the high level operation "branch renaming" needs
> to be aware of that "this branch is checked out elsewhere" information,
> you allowed it to break the workdir. If you hooked into lower level
> machinery that is shared, you wouldn't have caused this breakage.
> Similarly, if delete_ref() were taught about the new requirement, you
> would have covered both "branch -d" and "update-ref -d".

I did not forget, I just hadn't gotten there yet while this was still
an RFC/PATCH.

Another issue to resolve is what happens when the workdir or repo are
moved in the filesystem. And making prune aware of HEAD reflogs in the
alternate workdirs.

> I do not necessarily think that it is a good approach to forbid the same
> branch to be checked out in two different places, by the way. One reason
> people would want to keep multiple workdirs is so that while they are
> still working on a branch and are not yet at a good "stop point" to even
> make a temporary commit to get interrupted, they find it sometimes
> necessary to be able to build the tip of that same branch and even make a
> small in-working-tree fixes (which later will be carried back to the
> primary branch). The problem arises only when one of the repositories try
> to update or delete the branch while it is checked out in another working
> tree.

That is not at all my experience of how workdirs are used.

> Can this series be extended/reworked so that:
>
>  - Each branch has multi-value configuration record to note the workdirs
>   that it is checked out;

This is a good idea in any case for when "checkout --force" is used
(see below), so that we can find all the workdirs for other operations
that may need to.

>  - Error out (or warn if forced) upon any attempt to update the tip of a
>   branch that is checked out in more than one place; and

I think that's a worse user experience. "Sorry, can't commit your
changes because you've checked out this branch elsewhere." Now the
user's choices are:

1. commit --force (and thus confusing the other workdirs)
2. checkout -b new_branch && commit

Both of which I think are worse than preventing the checkout in the first place.

>  - Similarly for renaming or deleting a branch that is checked out in more
>   than one place.

Yep.

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Andreas Krey]

On Wed, 05 Oct 2011 15:11:30 +0000, Jay Soffian wrote:
...
> >  - Error out (or warn if forced) upon any attempt to update the tip of a
> >   branch that is checked out in more than one place; and
> 
> I think that's a worse user experience. "Sorry, can't commit your
> changes because you've checked out this branch elsewhere."

This is actually pretty much the same as "you can't push into the
currently checked-out branch".

I do come from CVS where multiple checkouts of the same branch are obviously
common, but the semantics are different. git would need to allow to be in
a detached state but still have a notion of a 'current' branch to mimic that;
this tentative 'current' branch being what we need to merge or rebase onto later.
Just thinking.

It may actually be logical to put the other workdirs into detached state when
the branch they are on is committed into; however, this is seriously confusing.

> Now the
> user's choices are:
> 
> 1. commit --force (and thus confusing the other workdirs)
> 2. checkout -b new_branch && commit
> 
> Both of which I think are worse than preventing the checkout in the first place.

Hmm. You mean forcing the user to make a new branch *earlier* than at
commit time is better?

Andreas

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Wed, Oct 5, 2011 at 4:00 PM, Andreas Krey <a.krey@gmx.de> wrote:
> Hmm. You mean forcing the user to make a new branch *earlier* than at
> commit time is better?

In my mind, we're trying to make new-workdir usable for non-advanced
users. I think it's conceptually simplest to allow a branch to be
checked out only once.

FWIW, I use a modified copy of new-workdir w/this usage:

  git new-workdir <repo> <workdir> <ref> [<start>]

Which allows me to create a new branch and workdir checked out to the
new branch in one shot. It refuses to create the <workdir> if <ref>
resolves to a checked-out branch. (If I want to start detached I can
do so with <ref>^0, but I rarely if ever do that.)

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jonathan Nieder]

Jay Soffian wrote:

> In my mind, we're trying to make new-workdir usable for non-advanced
> users.

I'd be happy already with making it comfortable for the advanced
users. :)

I think your patch goes in a right direction (using the shared
.git/config file as a way to negotiate ownership of branches).
Junio’s comments about it seeming sensible to

 - make this apply to other operations that clobber a branch
 - make the “[branch "master"] checkedout” configuration multi-valued
   if there is to be support for "git checkout -f" overriding this at
   all

ring true to me.  Making the value of this variable the path to the
.git dir or worktree (rather than an opaque string) seems like a very
good thing: it means that a future git could check if the directory
still exists and break the lock if someone has used “rm -fr”.

As for moving “git new-workdir” out of contrib, I believe another
prerequisite is sharing the HEAD reflog.

Just my two cents,
Jonathan

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Wed, Oct 5, 2011 at 5:30 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> As for moving “git new-workdir” out of contrib, I believe another
> prerequisite is sharing the HEAD reflog.

I don't understand this. Is it about not gc'ing commits that other
workdirs are detached on, or something more?

I like that each of my workdirs have their own HEAD reflog.

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jonathan Nieder]

Jay Soffian wrote:

> I don't understand this. Is it about not gc'ing commits that other
> workdirs are detached on, or something more?
>
> I like that each of my workdirs have their own HEAD reflog.

Yes, sorry for the lack of clarity.  I only meant that "git gc" needs
to be aware of the HEAD reflog for other workdirs (e.g., as described
in the thread following madcoder's proposal), not that it would be a
good idea for the reflogs to actually be symlinked.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Wed, Oct 5, 2011 at 2:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Also, while I might recommend new-workdir to my coworkers with the
> advice "don't checkout the same branch in multiple workdirs", never in
> a million years would I say "use new-workdir, but make sure to only
> use a detached HEAD in the workdirs." The latter would make their
> actual HEADs explode. :-)
>
>> ...
>> Because you forgot that the high level operation "branch renaming" needs
>> to be aware of that "this branch is checked out elsewhere" information,
>> you allowed it to break the workdir. If you hooked into lower level
>> machinery that is shared, you wouldn't have caused this breakage.
>> Similarly, if delete_ref() were taught about the new requirement, you
>> would have covered both "branch -d" and "update-ref -d".
>
> I did not forget, I just hadn't gotten there yet while this was still
> an RFC/PATCH.

You conveniently also forgot that you also said:

> Aside, there's nothing wrong with renaming a checked out branch.

With that lack of understanding, it wasn't "hadn't gotten there", but was
actually "didn't even know it was needed".

But that is OK. We have discussions to find out what we missed by learning
from others' insights.

> Another issue to resolve is what happens when the workdir or repo are
> moved in the filesystem. And making prune aware of HEAD reflogs in the
> alternate workdirs.
>
>> I do not necessarily think that it is a good approach to forbid the same
>> branch to be checked out in two different places, by the way. One reason
>> people would want to keep multiple workdirs is so that while they are
>> still working on a branch and are not yet at a good "stop point" to even
>> make a temporary commit to get interrupted, they find it sometimes
>> necessary to be able to build the tip of that same branch and even make a
>> small in-working-tree fixes (which later will be carried back to the
>> primary branch). The problem arises only when one of the repositories try
>> to update or delete the branch while it is checked out in another working
>> tree.
>
> That is not at all my experience of how workdirs are used.

I am afraid to say, with that statement, that your knowledge about the way
the tool can be used is not wide enough to judge if one policy restriction
(e.g. "never check out the same branch in multiple places") is general
enough to add to the tool. I do not claim mine is good enough, but I at
least know better than proposing a rule that may be too restictive to
negatively affect other people's workflows.

I always maintain four workdirs that I can use to build the tip of four
integration branches while I work on other things in the primary branch,
plus another that has a detached HEAD so that I can "reset --hard" around
without having to worry about what I do there would negatively affect what
I do elsewhere or vice versa. Of course, updating 'master' in my primary
repository will require the "build master" workdir to be "reset --hard"
before it is used, and that is part of my workflow already. I consider it
is one of "people learned to work around the restriction of the tool so
well already that they may not realize it" we discussed earlier.

Also, if your goal is to give a different semantics, like:

> In my mind, we're trying to make new-workdir usable for non-advanced
> users. I think it's conceptually simplest to allow a branch to be
> checked out only once.

you would really need to make sure that your changes would not harm other
users of the same tool that you are not targetting for, and also the
changes to the core part of the system that needs your specialized policy
makes sense in the wider context. The former you can claim "the policy
does not kick in when configuration is not set", but that is weak if the
policy is too ad-hoc and not well thought out. I actually care about the
latter more, as it is not worth having to spend maintenance effort to
carry changes that only stop some but not other kind of mistakes in the
core part to be more widely applicable.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Wed, Oct 5, 2011 at 5:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I am afraid to say, with that statement, that your knowledge about the way
> the tool can be used is not wide enough to judge if one policy restriction
> (e.g. "never check out the same branch in multiple places") is general
> enough to add to the tool. I do not claim mine is good enough, but I at
> least know better than proposing a rule that may be too restictive to
> negatively affect other people's workflows.
>
> I always maintain four workdirs that I can use to build the tip of four
> integration branches while I work on other things in the primary branch,
> plus another that has a detached HEAD so that I can "reset --hard" around
> without having to worry about what I do there would negatively affect what
> I do elsewhere or vice versa. Of course, updating 'master' in my primary
> repository will require the "build master" workdir to be "reset --hard"
> before it is used, and that is part of my workflow already. I consider it
> is one of "people learned to work around the restriction of the tool so
> well already that they may not realize it" we discussed earlier.

Is it a regression to your workflow that you have to now use "checkout
-f" instead of "checkout" to checkout the same branch in more than one
location?

> Also, if your goal is to give a different semantics, like:
>
>> In my mind, we're trying to make new-workdir usable for non-advanced
>> users. I think it's conceptually simplest to allow a branch to be
>> checked out only once.
>
> you would really need to make sure that your changes would not harm other
> users of the same tool that you are not targetting for, and also the
> changes to the core part of the system that needs your specialized policy
> makes sense in the wider context. The former you can claim "the policy
> does not kick in when configuration is not set", but that is weak if the
> policy is too ad-hoc and not well thought out. I actually care about the
> latter more, as it is not worth having to spend maintenance effort to
> carry changes that only stop some but not other kind of mistakes in the
> core part to be more widely applicable.

Perhaps:

  core.multipleCheckouts = {true,false}
    - false prevents multiple checkouts without -f
  advice.multipleCheckouts = {true,false}
    - false disables multiple-checkout notice

And when there are multiple checkouts, warn on committing about other
workdirs that now need reset --hard.

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

Aside, previous discussion -
http://thread.gmane.org/gmane.comp.version-control.git/150559

Sadly, it seems to have petered out, it looks to me like a case of
perfect being the enemy of the good. I'm really just trying to make it
good enough that we can move new-workdir out of contrib. It's a
valuable tool, we just need to remove some of its sharper edges.

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Nguyen Thai Ngoc Duy]

On Thu, Oct 6, 2011 at 5:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I do not necessarily think that it is a good approach to forbid the same
> branch to be checked out in two different places, by the way. One reason
> people would want to keep multiple workdirs is so that while they are
> still working on a branch and are not yet at a good "stop point" to even
> make a temporary commit to get interrupted, they find it sometimes
> necessary to be able to build the tip of that same branch and even make a
> small in-working-tree fixes (which later will be carried back to the
> primary branch). The problem arises only when one of the repositories try
> to update or delete the branch while it is checked out in another working
> tree.

I think of two options:

 - detach from the already locked branch (pretty much like what we do
with tags now)

 - refuse normally but let "checkout -f" do it anyway. However the
checkout lock will remain at the original worktree. If you want to
update branch from the second checkout, do "commit -f" and take
responsibility for your action.
-- 
Duy

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Thu, Oct 6, 2011 at 5:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> I do not necessarily think that it is a good approach to forbid the same
>> branch to be checked out in two different places, by the way. One reason
>> people would want to keep multiple workdirs is so that while they are
>> still working on a branch and are not yet at a good "stop point" to even
>> make a temporary commit to get interrupted, they find it sometimes
>> necessary to be able to build the tip of that same branch and even make a
>> small in-working-tree fixes (which later will be carried back to the
>> primary branch). The problem arises only when one of the repositories try
>> to update or delete the branch while it is checked out in another working
>> tree.
>
> I think of two options:
>
>  - detach from the already locked branch (pretty much like what we do
> with tags now)
>
>  - refuse normally but let "checkout -f" do it anyway. However the
> checkout lock will remain at the original worktree. If you want to
> update branch from the second checkout, do "commit -f" and take
> responsibility for your action.

Sorry, what problem are you trying to solve? Does that "checkout -f" meant
to nuke the local changes that are not yet at a good "stop point"?

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Nguyen Thai Ngoc Duy]

On Thu, Oct 6, 2011 at 9:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On Thu, Oct 6, 2011 at 5:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> I do not necessarily think that it is a good approach to forbid the same
>>> branch to be checked out in two different places, by the way. One reason
>>> people would want to keep multiple workdirs is so that while they are
>>> still working on a branch and are not yet at a good "stop point" to even
>>> make a temporary commit to get interrupted, they find it sometimes
>>> necessary to be able to build the tip of that same branch and even make a
>>> small in-working-tree fixes (which later will be carried back to the
>>> primary branch). The problem arises only when one of the repositories try
>>> to update or delete the branch while it is checked out in another working
>>> tree.
>>
>> I think of two options:
>>
>>  - detach from the already locked branch (pretty much like what we do
>> with tags now)
>>
>>  - refuse normally but let "checkout -f" do it anyway. However the
>> checkout lock will remain at the original worktree. If you want to
>> update branch from the second checkout, do "commit -f" and take
>> responsibility for your action.
>
> Sorry, what problem are you trying to solve? Does that "checkout -f" meant
> to nuke the local changes that are not yet at a good "stop point"?

I meant "git checkout" on the already locked branch is refused, but
"git checkout -f" in that case will act just like "git checkout"
ignoring all locks. But I forgot that "git checkout -f" also discards
worktree changes. Maybe "git checkout --ignore-locks" instead of "git
checkout -f".
-- 
Duy

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Thu, Oct 6, 2011 at 9:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> I think of two options:
>>> ...
>> Sorry, what problem are you trying to solve? Does that "checkout -f" meant
>> to nuke the local changes that are not yet at a good "stop point"?
>
> I meant "git checkout" on the already locked branch is refused, but
> "git checkout -f" in that case will act just like "git checkout"
> ignoring all locks. But I forgot that "git checkout -f" also discards
> worktree changes. Maybe "git checkout --ignore-locks" instead of "git
> checkout -f".

I see what you mean, but doesn't it feel as if it is working around a
problem that is introduced only because of a wrong policy (i.e. "you
cannot check out the same branch at two places", as opposed to "viewing
them in multiple places is perfectly fine, but no touching")?

This reminds me of how we ended up handling the "scary warning" around
detached HEAD. It is not wrong nor even dangerous to detach. It is not
wrong nor even dangerous to make commits on detached HEAD. It is however
dangerous to switch away from that state without saving it to a ref, and
that is where we give warnings.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Wed, Oct 5, 2011 at 7:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This reminds me of how we ended up handling the "scary warning" around
> detached HEAD. It is not wrong nor even dangerous to detach. It is not
> wrong nor even dangerous to make commits on detached HEAD. It is however
> dangerous to switch away from that state without saving it to a ref, and
> that is where we give warnings.

If you have the same branch in two workdirs, then if you commit to
that branch in one workdir, you have to reset --hard in the other. In
that case, wouldn't it make more sense to just use a detached head in
the second workdir?

  $ git checkout topic
  fatal: branch 'topic' is currently checked out in '...'
  $ git checkout topic^0
  ... topic is updated elsewhere ...
  $ git reset --hard topic

Either way you need to use reset --hard if topic is updated outside of
the current workdir, but at least if git encourages you to detach
first, you don't accidentally undo a commit.

Also, if we wait till commit time to tell the user "sorry, topic's
been updated elsewhere", now the user is in a perilous state. They
have uncommitted work which they clearly want on topic. And they have
to think about what steps are needed to get it there.

So, I really don't think this is quite analogous to detached HEAD, nor
pushing into a repo's checked out branch. In both those cases, at
least the user's work is already committed.

Better to prevent checking out the same branch in multiple workdirs
with an override for users that want risk shooting their foot off.

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Wed, Oct 5, 2011 at 7:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> This reminds me of how we ended up handling the "scary warning" around
>> detached HEAD. It is not wrong nor even dangerous to detach. It is not
>> wrong nor even dangerous to make commits on detached HEAD. It is however
>> dangerous to switch away from that state without saving it to a ref, and
>> that is where we give warnings.
>
> If you have the same branch in two workdirs, then if you commit to
> that branch in one workdir, you have to reset --hard in the other. In
> that case, wouldn't it make more sense to just use a detached head in
> the second workdir?

Not at all. My build infrastructure determines where to install the built
binary based on what branch is checked out. Having a head detached at a
commit that is at the tip of one branch is not necessarily the same as
having the branch actually checked out.

> Also, if we wait till commit time to tell the user "sorry, topic's
> been updated elsewhere", now the user is in a perilous state.

Wouldn't the "elsewhere" user would be warned before being able to update
the branch? I thought the whole point of your adding "this branch is
checked out over there" is exactly so that the "elsewhere" user can come
talk to you before that happens. These two people might be yourself, of
course.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Wed, Oct 5, 2011 at 8:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Not at all. My build infrastructure determines where to install the built
> binary based on what branch is checked out. Having a head detached at a
> commit that is at the tip of one branch is not necessarily the same as
> having the branch actually checked out.

That's fair, but I'm willing to wager that's a minority use-case. Not
that it shouldn't be possible, but perhaps it should require telling
git that's really what you want to do with checkout --force.

>> Also, if we wait till commit time to tell the user "sorry, topic's
>> been updated elsewhere", now the user is in a perilous state.
>
> Wouldn't the "elsewhere" user would be warned before being able to update
> the branch? I thought the whole point of your adding "this branch is
> checked out over there" is exactly so that the "elsewhere" user can come
> talk to you before that happens. These two people might be yourself, of
> course.

So you're envisioning this?

  $ git commit foo.c
  Warning, master is also checked out in workdir2

How does that help the user? Now they have to go to workdir2 and reset
--hard. Is that really something we want to encourage?

And what if they do this:

  $ cd workdir1
  $ edit foo.c
  ... time passes...
  $ cd workdir2
  $ edit foo.c
  $ git commit foo.c
  Warning, master is also checked out in workdir1

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> So you're envisioning this?
>
>   $ git commit foo.c
>   Warning, master is also checked out in workdir2

No. I would rather think it needs to be forced.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Wed, Oct 5, 2011 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> So you're envisioning this?
>>
>>   $ git commit foo.c
>>   Warning, master is also checked out in workdir2
>
> No. I would rather think it needs to be forced.

Now they do what? Either commit --force or create a new branch?
Wouldn't it have been better to create the new branch before they
started editing?

Here's what I'm trying to avoid:

  $ cd workdir2
  $ git checkout master
  $ edit foo.c
  $ git commit foo.c
  By default, committing to a branch that is checked out in more than
  one location is denied, because it will make the index and work tree
  inconsistent in the other locations and will require 'git reset --hard'
  to match the work tree to HEAD in each of those other locations.

  Either switch to a new branch first with 'git checkout -b <new_branch>'
  or use 'git commit --force' to override this message.

User: "crap, I wanted that on master". Now they do what. Something like:

  $ git checkout -b foo
  $ git commit foo.c
  $ cd workdir1
  $ git cherry-pick foo
  $ git branch -d foo

Or maybe they use stash instead. In either case, I just think that's a
terrible user experience compared to:

 $ cd workdir2
 $ git checkout master
 error: master already checked out in workdir1
 $ cd workdir1
 $ edit foo.c
 $ git commit foo.c

I guess it depends what you mostly use your workdirs for. For me, it's
to have different branches checked out, not to have the same branch
checked out in multiple locations. I want git to help me up front, not
when I go to commit.

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Wed, Oct 5, 2011 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jay Soffian <jaysoffian@gmail.com> writes:
>>
>>> So you're envisioning this?
>>>
>>>   $ git commit foo.c
>>>   Warning, master is also checked out in workdir2
>>
>> No. I would rather think it needs to be forced.
>
> Now they do what? Either commit --force or create a new branch?
> Wouldn't it have been better to create the new branch before they
> started editing?

If they are going to commit, and if they knew that they are going to
commit, yes.

But why do you want to forbid people from just checking things out if they
are not interested in committing? That is where I think you are going
backwards.

> I guess it depends what you mostly use your workdirs for. For me, it's
> to have different branches checked out, not to have the same branch
> checked out in multiple locations.

Then you wouldn't have any problem if commit refused to make commit on the
branch that is checked out elsewhere, no?

I am not saying we should never have an option to _warn_ checking out the
same branch in multiple places. I am saying it is wrong to forbid doing so
by default.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Wed, Oct 5, 2011 at 9:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>> Now they do what? Either commit --force or create a new branch?
>> Wouldn't it have been better to create the new branch before they
>> started editing?
>
> If they are going to commit, and if they knew that they are going to
> commit, yes.

Committing is obviously the common case for a checked-out branch.

> But why do you want to forbid people from just checking things out if they
> are not interested in committing? That is where I think you are going
> backwards.

Because if they do decide to commit, it's now harder for them to do so.

It would be great if git could intervene after the checkout, but
before they edit any files, so that they don't have uncommitted work.
Obviously that's not possible, so git should prevent them from getting
to that point.

Let's consider the various situations:

1. master is checked out w/edits in workdir1, user wants to work on
topic in workdir2.

There's nothing to warn about in workdir2 neither at checkout nor commit time.

2. master is checked out w/edits in workdir1, user wants examine
unedited master in workdir2

At checkout time in workdir2:

My preference: checkout advices user to use --detach or --force.
Your preference: checkout is silent.

Now user decides they want to commit to master in workdir2 (which is
insane, they've got uncommitted changes to it in workdir1). What
happens?

In my scenario, the commit happens on a detached HEAD. When they
eventually switch back to a branch, git tells them how to move their
commit to a branch.

In your scenario, commit complains. User now has to --force, stash, or
create a new branch.

It's just seems insane to me putting in obstacles to the user
committing their work. That's where I think you are going backwards.

You have a use case where using a detached HEAD doesn't work because
you've scripted around the same branch multiply checked out. I think
that's probably an exceedingly rare use case, and justifies "checkout
--force".

>> I guess it depends what you mostly use your workdirs for. For me, it's
>> to have different branches checked out, not to have the same branch
>> checked out in multiple locations.
>
> Then you wouldn't have any problem if commit refused to make commit on the
> branch that is checked out elsewhere, no?

Yes, I would, because by that point, I've already made the mistake of
checking out the same branch twice. I want git to prevent me from
doing that by accident. Because I don't want to ever be in the
situation which comes next, which is that I've got uncommitted work
for the same branch in two places!

> I am not saying we should never have an option to _warn_ checking out the
> same branch in multiple places. I am saying it is wrong to forbid doing so
> by default.

I am not saying we should never have an option to allow checking out
the same branch in multiple places. I am saying it is wrong to allow
doing so by default.

j.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Nguyen Thai Ngoc Duy]

On Thu, Oct 6, 2011 at 10:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On Thu, Oct 6, 2011 at 9:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> I think of two options:
>>>> ...
>>> Sorry, what problem are you trying to solve? Does that "checkout -f" meant
>>> to nuke the local changes that are not yet at a good "stop point"?
>>
>> I meant "git checkout" on the already locked branch is refused, but
>> "git checkout -f" in that case will act just like "git checkout"
>> ignoring all locks. But I forgot that "git checkout -f" also discards
>> worktree changes. Maybe "git checkout --ignore-locks" instead of "git
>> checkout -f".
>
> I see what you mean, but doesn't it feel as if it is working around a
> problem that is introduced only because of a wrong policy (i.e. "you
> cannot check out the same branch at two places", as opposed to "viewing
> them in multiple places is perfectly fine, but no touching")?

Well, we could do change the default so "git checkout" == "git
checkout --ignore-locks".

"git commit --ignore-locks" would commit without checking locks. "git
commit" could either:

 - reject because it does not hold the lock (to hostile?)

 - detach automatically then commit

The latter has a benefit that we can now checkout tags without
detaching from the beginning. "git branch" would show tag name until
you commit.
-- 
Duy

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Bernhard R. Link]

* Junio C Hamano <gitster@pobox.com> [111005 20:19]:
> I do not necessarily think that it is a good approach to forbid the same
> branch to be checked out in two different places, by the way. [...]
> [...] The problem arises only when one of the repositories try
> to update or delete the branch while it is checked out in another working
> tree.

I think this is mostly the same problem that also make pushing to a
checked out branch a problem.

Isn't the real problem that a checked out branch / a branch having a
workdir only has information what branch it belongs to?

Wouldn't both problems (multiple workdirs of the same branch, pushing
to a checked out branch) solved if each working directory (including
the default one in a non-bare repository) also stored the commit id
last checked out? (And then giving a warning, error or automatically
creating a detached head setting whenever the branch it followed is
moved behind it's back?)

	Bernhard R. Link

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jeff King]

On Wed, Oct 05, 2011 at 11:19:17AM -0700, Junio C Hamano wrote:

> Jay Soffian <jaysoffian@gmail.com> writes:
> 
> > Git has survived w/o needing to lock branches till now.
> 
> Careful. Git has survived without your patch series till now, as people
> learned to be careful when they use separate workdirs and avoid certain
> things, to the point that they are not necessarily aware that they are
> avoiding them (one good practice is to keep the HEADs of non-primary
> workdirs detached).
> 
> Does that mean what your patch aims to do is unnecessary? I think not.

It seems to me that things like receive.denyCurrentBranch and
receive.denyDeleteCurrent are just special hand-rolled versions of the
same concept.

Could they be implemented using this kind of branch locking? Moreover, I
think they would need to be to cope with new-workdir, as the definition
of "current" stops being "referenced by HEAD", but becomes much larger.

-Peff

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Nguyen Thai Ngoc Duy]

On Thu, Oct 6, 2011 at 3:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> On Wed, Oct 5, 2011 at 12:02 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
>>> Could you please consider a more generic approach? What I have in mind
>>> is a mechanism to "lock" a branch, so that only commands that have the
>>> key can update it.
>>>
>>> So instead of branch.<name>.checkout, I would have something like
>>> branch.<name>.locked = <key>, where <key> is just a string. Only
>>> commands that provide the matching <key> are allowed to update the
>>> branch. In checkout case, <key> could be "checkout: worktree".
>>
>> In this case, each workdir needs its own key, so I'd have to record
>> the key somewhere, unless you meant using a key of "checkout:
>> </path/to/workdir>".
>
> That actually is how I read his message.

That's what I meant.

> I think "switch_branches()" that updates HEAD to point at a local branch
> is one good place to lock the branch, but I do not know if it is a good
> idea to hook the check into the codepaths for deletion of the branch using
> "branch -[dD]" and check-out of the branch using "checkout $branch". I
> wonder if it makes sense to add the "checking" hook into much lower level
> in the callchain, perhaps delete_ref(), rename_ref() and update_ref() to
> catch attempts to update "your" current branch by other people.

I'd aim at low-level ref manipulation because too me it affects more
than just "git checkout".

> For that
> matter, instead of switch_branches(), would it make more sense to add this
> lock/unlock logic to symbolic_ref() that repoints HEAD to other branch?

Couldn't find symbolic_ref() in current code. If you meant
create_symref(), yes that would make sense.
-- 
Duy

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 5e356a6c61..26259a41a7 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -709,12 +710,35 @@ static void orphaned_commit_warning(struct commit *commit)
>  	for_each_ref(clear_commit_marks_from_one_ref, NULL);
>  }
>  
> +static void record_checkout(const char *name, const char *work_tree)
> +{
> +	struct strbuf key = STRBUF_INIT;
> +	strbuf_addf(&key, "branch.%s.checkout", name);
> +	git_config_set(key.buf, work_tree);
> +	strbuf_release(&key);
> +}
> +
> +static void check_if_checked_out(struct checkout_opts *opts, const char *name)
> +{
> +	struct branch *branch = branch_get(name);
> +	if (branch->work_tree && strlen(branch->work_tree) &&
> +	    strcmp(branch->work_tree, get_git_work_tree())) {
> +		if (opts->force)
> +			warning(_("branch '%s' is currently checked out"
> +				  " in '%s'"), name, branch->work_tree);
> +		else
> +			die(_("branch '%s' is currently checked out"
> +			      " in '%s'"), name, branch->work_tree);
> +	}
> +}
> +
>  static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>  {
>  	int ret = 0;
>  	struct branch_info old;
>  	unsigned char rev[20];
>  	int flag;
> +
>  	memset(&old, 0, sizeof(old));
>  	old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag));
>  	old.commit = lookup_commit_reference_gently(rev, 1);
> @@ -734,6 +758,9 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>  		parse_commit(new->commit);
>  	}
>  
> +	if (opts->record_checkouts)
> +		check_if_checked_out(opts, new->name);

The close brace we can see in the context closes "if (!new->name) {", so
this codepath is very well prepared to be called with new->name == NULL.

Is check_if_checked_out() prepared to be called with name == NULL and do
the right thing?

> @@ -743,6 +770,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>  
>  	update_refs_for_switch(opts, &old, new);
>  
> +	if (opts->record_checkouts) {
> +		const char *work_tree = get_git_work_tree();
> +		struct branch *branch = branch_get(old.name);
> +		if (branch->work_tree && !strcmp(branch->work_tree, work_tree))
> +			record_checkout(old.name, "");
> +		record_checkout(new->name, work_tree);
> +	}
> +

Likewise for new->name, but also old.name which is only set when old.path
is set and begins with "refs/heads/" and otherwise NULL.

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

On Wed, Oct 5, 2011 at 12:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>> @@ -734,6 +758,9 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>>               parse_commit(new->commit);
>>       }
>>
>> +     if (opts->record_checkouts)
>> +             check_if_checked_out(opts, new->name);
>
> The close brace we can see in the context closes "if (!new->name) {", so
> this codepath is very well prepared to be called with new->name == NULL.
>
> Is check_if_checked_out() prepared to be called with name == NULL and do
> the right thing?
>
>> @@ -743,6 +770,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>>
>>       update_refs_for_switch(opts, &old, new);
>>
>> +     if (opts->record_checkouts) {
>> +             const char *work_tree = get_git_work_tree();
>> +             struct branch *branch = branch_get(old.name);
>> +             if (branch->work_tree && !strcmp(branch->work_tree, work_tree))
>> +                     record_checkout(old.name, "");
>> +             record_checkout(new->name, work_tree);
>> +     }
>> +
>
> Likewise for new->name, but also old.name which is only set when old.path
> is set and begins with "refs/heads/" and otherwise NULL.

I was more looking for feedback on the idea than the implementation, but
here's a better implementation. Still an RFC so no tests yet.

-- >8 --
Subject: [RFC/PATCH] Teach branch/checkout about workdirs

When using 'git new-workdir', there is no safety mechanism to prevent the
same branch from being checked out twice, nor to prevent a checked out
branch from being deleted.

Teach 'checkout' to record the workdir path using 'branch.<name>.checkout'
when switching branches. We can then easily check if a branch is already
checked out in another workdir before switching to that branch. Add a
similar check before deleting a branch.

Allow 'checkout -f' to force the checkout and issue a warning instead of
an error.

Guard this behavior behind 'core.recordCheckouts', which we will teach
'git new-workdir' to set in a followup commit.

Note: when switching away from a branch, we set 'branch.<name>.checkout'
to the empty string, instead of deleting it entirely, since git_config()
otherwise leaves behind an empty section which it does not re-use.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin/branch.c   |   10 ++++++++++
 builtin/checkout.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 remote.c           |    4 ++++
 remote.h           |    1 +
 4 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index f49596f826..6ce1a5b133 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -182,6 +182,16 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 			ret = 1;
 			continue;
 		}
+		if (kinds == REF_LOCAL_BRANCH) {
+			struct branch *branch = branch_get(bname.buf);
+			if (branch->work_tree && strlen(branch->work_tree)) {
+				error(_("Cannot delete the branch '%s' "
+					"which is currently checked out in '%s'"),
+				      bname.buf, branch->work_tree);
+				ret = 1;
+				continue;
+			}
+		}
 
 		free(name);
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5e356a6c61..b3c658ffd4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -33,6 +33,7 @@ struct checkout_opts {
 	int force_detach;
 	int writeout_stage;
 	int writeout_error;
+	int record_checkouts;
 
 	/* not set by parse_options */
 	int branch_exists;
@@ -508,6 +509,20 @@ static void detach_advice(const char *old_path, const char *new_name)
 	fprintf(stderr, fmt, new_name);
 }
 
+static void record_checkout(const char *name, const char *new_work_tree)
+{
+	struct strbuf key = STRBUF_INIT;
+	strbuf_addf(&key, "branch.%s.checkout", name);
+	if (new_work_tree) { /* reserve name */
+		git_config_set(key.buf, new_work_tree);
+	} else { /* release name if we reserved it */
+		struct branch *branch = branch_get(name);
+		if (!strcmp(branch->work_tree, get_git_work_tree()))
+			git_config_set(key.buf, "");
+	}
+	strbuf_release(&key);
+}
+
 static void update_refs_for_switch(struct checkout_opts *opts,
 				   struct branch_info *old,
 				   struct branch_info *new)
@@ -556,6 +571,8 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 				detach_advice(old->path, new->name);
 			describe_detached_head(_("HEAD is now at"), new->commit);
 		}
+		if (opts->record_checkouts && old->name)
+			record_checkout(old->name, NULL);
 	} else if (new->path) {	/* Switch branches. */
 		create_symref("HEAD", new->path, msg.buf);
 		if (!opts->quiet) {
@@ -580,6 +597,11 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 			if (!file_exists(ref_file) && file_exists(log_file))
 				remove_path(log_file);
 		}
+		if (opts->record_checkouts) {
+			if (old->name)
+				record_checkout(old->name, NULL);
+			record_checkout(new->name, get_git_work_tree());
+		}
 	}
 	remove_branch_state();
 	strbuf_release(&msg);
@@ -709,6 +731,23 @@ static void orphaned_commit_warning(struct commit *commit)
 	for_each_ref(clear_commit_marks_from_one_ref, NULL);
 }
 
+static void check_if_checked_out(struct checkout_opts *opts, const char *name)
+{
+	struct branch *branch;
+	if (!opts->record_checkouts)
+		return;
+	branch = branch_get(name);
+	if (branch->work_tree && strlen(branch->work_tree) &&
+	    strcmp(branch->work_tree, get_git_work_tree())) {
+		if (opts->force)
+			warning(_("branch '%s' is currently checked out"
+				  " in '%s'"), name, branch->work_tree);
+		else
+			die(_("branch '%s' is currently checked out"
+			      " in '%s'"), name, branch->work_tree);
+	}
+}
+
 static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 {
 	int ret = 0;
@@ -732,6 +771,8 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 		if (!new->commit)
 			die(_("You are on a branch yet to be born"));
 		parse_commit(new->commit);
+	} else if (opts->record_checkouts) {
+		check_if_checked_out(opts, new->name);
 	}
 
 	ret = merge_working_tree(opts, &old, new);
@@ -756,6 +797,10 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.recordcheckouts")) {
+		struct checkout_opts *opts = cb;
+		opts->record_checkouts = git_config_bool(var, value);
+	}
 	if (!prefixcmp(var, "submodule."))
 		return parse_submodule_config_option(var, value);
 
diff --git a/remote.c b/remote.c
index b8ecfa5d95..2bc063dae8 100644
--- a/remote.c
+++ b/remote.c
@@ -364,6 +364,10 @@ static int handle_config(const char *key, const char *value, void *cb)
 			if (!value)
 				return config_error_nonbool(key);
 			add_merge(branch, xstrdup(value));
+		} else if (!strcmp(subkey, ".checkout")) {
+			if (!value)
+				return config_error_nonbool(key);
+			branch->work_tree = xstrdup(value);
 		}
 		return 0;
 	}
diff --git a/remote.h b/remote.h
index 9a30a9dba6..4103ec7e31 100644
--- a/remote.h
+++ b/remote.h
@@ -126,6 +126,7 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec);
 struct branch {
 	const char *name;
 	const char *refname;
+	const char *work_tree;
 
 	const char *remote_name;
 	struct remote *remote;
-- 
1.7.7.4.g39e02c

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Jay Soffian]

> I was more looking for feedback on the idea than the implementation, but
> here's a better implementation. Still an RFC so no tests yet.

Oops. Let's try that again. Sent the wrong thing.

-- >8 --
Subject: [RFC/PATCH] Teach branch/checkout about workdirs

When using 'git new-workdir', there is no safety mechanism to prevent the
same branch from being checked out twice, nor to prevent a checked out
branch from being deleted.

Teach 'checkout' to record the workdir path using 'branch.<name>.checkout'
when switching branches. We can then easily check if a branch is already
checked out in another workdir before switching to that branch. Add a
similar check before deleting a branch.

Allow 'checkout -f' to force the checkout and issue a warning instead of
an error.

Guard this behavior behind 'core.recordCheckouts', which we will teach
'git new-workdir' to set in a followup commit.

Note: when switching away from a branch, we set 'branch.<name>.checkout'
to the empty string, instead of deleting it entirely, since git_config()
otherwise leaves behind an empty section which it does not re-use.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin/branch.c   |   10 ++++++++++
 builtin/checkout.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 remote.c           |    4 ++++
 remote.h           |    1 +
 4 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index f49596f826..6ce1a5b133 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -182,6 +182,16 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
 			ret = 1;
 			continue;
 		}
+		if (kinds == REF_LOCAL_BRANCH) {
+			struct branch *branch = branch_get(bname.buf);
+			if (branch->work_tree && strlen(branch->work_tree)) {
+				error(_("Cannot delete the branch '%s' "
+					"which is currently checked out in '%s'"),
+				      bname.buf, branch->work_tree);
+				ret = 1;
+				continue;
+			}
+		}
 
 		free(name);
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5e356a6c61..75510befde 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -33,6 +33,7 @@ struct checkout_opts {
 	int force_detach;
 	int writeout_stage;
 	int writeout_error;
+	int record_checkouts;
 
 	/* not set by parse_options */
 	int branch_exists;
@@ -508,6 +509,21 @@ static void detach_advice(const char *old_path, const char *new_name)
 	fprintf(stderr, fmt, new_name);
 }
 
+static void record_checkout(const char *name, const char *new_work_tree)
+{
+	struct strbuf key = STRBUF_INIT;
+	strbuf_addf(&key, "branch.%s.checkout", name);
+	if (new_work_tree) { /* reserve name */
+		git_config_set(key.buf, new_work_tree);
+	} else { /* release name if we reserved it */
+		struct branch *branch = branch_get(name);
+		if (branch->work_tree &&
+		    !strcmp(branch->work_tree, get_git_work_tree()))
+			git_config_set(key.buf, "");
+	}
+	strbuf_release(&key);
+}
+
 static void update_refs_for_switch(struct checkout_opts *opts,
 				   struct branch_info *old,
 				   struct branch_info *new)
@@ -556,6 +572,8 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 				detach_advice(old->path, new->name);
 			describe_detached_head(_("HEAD is now at"), new->commit);
 		}
+		if (opts->record_checkouts && old->name)
+			record_checkout(old->name, NULL);
 	} else if (new->path) {	/* Switch branches. */
 		create_symref("HEAD", new->path, msg.buf);
 		if (!opts->quiet) {
@@ -580,6 +598,11 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 			if (!file_exists(ref_file) && file_exists(log_file))
 				remove_path(log_file);
 		}
+		if (opts->record_checkouts) {
+			if (old->name)
+				record_checkout(old->name, NULL);
+			record_checkout(new->name, get_git_work_tree());
+		}
 	}
 	remove_branch_state();
 	strbuf_release(&msg);
@@ -709,6 +732,20 @@ static void orphaned_commit_warning(struct commit *commit)
 	for_each_ref(clear_commit_marks_from_one_ref, NULL);
 }
 
+static void check_if_checked_out(struct checkout_opts *opts, const char *name)
+{
+	struct branch *branch = branch_get(name);
+	if (branch->work_tree && strlen(branch->work_tree) &&
+	    strcmp(branch->work_tree, get_git_work_tree())) {
+		if (opts->force)
+			warning(_("branch '%s' is currently checked out"
+				  " in '%s'"), name, branch->work_tree);
+		else
+			die(_("branch '%s' is currently checked out"
+			      " in '%s'"), name, branch->work_tree);
+	}
+}
+
 static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 {
 	int ret = 0;
@@ -732,6 +769,8 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 		if (!new->commit)
 			die(_("You are on a branch yet to be born"));
 		parse_commit(new->commit);
+	} else if (opts->record_checkouts) {
+		check_if_checked_out(opts, new->name);
 	}
 
 	ret = merge_working_tree(opts, &old, new);
@@ -756,6 +795,10 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.recordcheckouts")) {
+		struct checkout_opts *opts = cb;
+		opts->record_checkouts = git_config_bool(var, value);
+	}
 	if (!prefixcmp(var, "submodule."))
 		return parse_submodule_config_option(var, value);
 
diff --git a/remote.c b/remote.c
index b8ecfa5d95..2bc063dae8 100644
--- a/remote.c
+++ b/remote.c
@@ -364,6 +364,10 @@ static int handle_config(const char *key, const char *value, void *cb)
 			if (!value)
 				return config_error_nonbool(key);
 			add_merge(branch, xstrdup(value));
+		} else if (!strcmp(subkey, ".checkout")) {
+			if (!value)
+				return config_error_nonbool(key);
+			branch->work_tree = xstrdup(value);
 		}
 		return 0;
 	}
diff --git a/remote.h b/remote.h
index 9a30a9dba6..4103ec7e31 100644
--- a/remote.h
+++ b/remote.h
@@ -126,6 +126,7 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec);
 struct branch {
 	const char *name;
 	const char *refname;
+	const char *work_tree;
 
 	const char *remote_name;
 	struct remote *remote;
-- 
1.7.7.5.gd207e.dirty

Re: [RFC/PATCH] Add multiple workdir support to branch/checkout

[Julián Landerreche]

Jay wrote:

> I guess it depends what you mostly use your workdirs for. For me, it's 
> to have different branches checked out, not to have the same branch
> checked out in multiple locations. 

I find those both use cases for workdirs to fit perfectly in my usual workflow 
(web development).

- Different branches checked out
I've a cloned repo of a CMS and I use git-new-workdir to checkout different 
branches and tags, so to have available a few workdirs of recent versions, which 
I "attach" by symlinks to my web development projects.

- Same branch checked out in multiple locations
This use case just came up recently, when I find out that I prefer to have two 
websites "attached" (via symlink) to two different workdirs of the same branch.
I could have "attached" both websites to the same workdir, but my idea of having 
the websites "attached" to different workdirs was to be able to do some 
development (i.e: to commit stuff) on one workdir, while keeping the other one 
"fixed" at some particular commit. 

Juno wrote:

> Careful. Git has survived without your patch series till now, as people
> learned to be careful when they use separate workdirs and avoid certain
> things, to the point that they are not necessarily aware that they are
> avoiding them (one good practice is to keep the HEADs of non-primary
> workdirs detached).

Jay wrote:

> Also, while I might recommend new-workdir to my coworkers with the
> advice "don't checkout the same branch in multiple workdirs", never in
> a million years would I say "use new-workdir, but make sure to only
> use a detached HEAD in the workdirs." The latter would make their
> actual HEADs explode. 

After reading this, I noticed that using git-new-workdir with detached HEAD in 
each workdir could fit my workflow very well. In some cases (the ones mentioned 
above), I find that I may not need to have a workdir for a branch (where I won't 
do work, so won't be committing there), but rather to have that workdir "fixed" 
at a particular commit. 
That being said, I also see that I would find useful to be able to 
update/advance this workdir (in a detached HEAD state, that is, "fixed" at an 
specific commit) to a newer commit or to a particular branch.

Bottom line: making git-new-workdir a more reliable & friendly tool that could 
fit in the workflows of both advanced and non-advanced users.

-----
Quick note about me: I am an "advanced n00b" on git usage (using it since one 
year ago), and a general non-advanced user (of git and git-new-workdir). In 
other words, a git user that could easily shoot itself in the foot.

Arrived here while looking for some info about git-new-workdir, and if it was a 
Bad Idea to have different workdirs of the same branch (ie. checkout the same 
branch on different folders), as the idea of recklessly committing on different 
workdirs for the same branch sounded like a recipe for disasters to me.

I find it git-new-workdir a really useful tool in my workflow, and prefer it 
over having many clones of the same repo, which will imply having to do 
configuration for remotes and push/pull operations, which are also mind-boggling 
tasks for non-advanced users.
-----

Thanks for reading.