| Message ID | 20180922180500.4689-3-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix per-worktree ref iteration in fsck/reflog expire | expand |
On Sat, Sep 22, 2018 at 2:05 PM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > When multiple worktrees are used, we need rules to determine if > something belongs to one worktree or all of them. Instead of keeping > adding rules when new stuff comes, have a generic rule: > [...] > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > diff --git a/t/t1415-worktree-refs.sh b/t/t1415-worktree-refs.sh > @@ -0,0 +1,36 @@ > +test_expect_success 'setup' ' > + test_commit initial && > + test_commit wt1 && > + test_commit wt2 && > + git worktree add wt1 wt1 && > + git worktree add wt2 wt2 && > + git checkout initial > +' > + > +test_expect_success 'add refs/local' ' > + git update-ref refs/local/foo HEAD && > + git -C wt1 update-ref refs/local/foo HEAD && > + git -C wt2 update-ref refs/local/foo HEAD > +' Not at all worth a re-roll, but the "add refs/local" test seems like just more setup, thus could be rolled into the "setup" test (unless it will be growing in some non-setup way in later patches).
On Sat, Sep 22, 2018 at 11:05 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > > When multiple worktrees are used, we need rules to determine if > something belongs to one worktree or all of them. Instead of keeping > adding rules when new stuff comes, have a generic rule: > > - Inside $GIT_DIR, which is per-worktree by default, add > $GIT_DIR/common which is always shared. New features that want to > share stuff should put stuff under this directory. So that /common is a directory and you have to use it specifically in new code? That would be easy to overlook when coming up with $GIT_DIR/foo for implementing the git-foo. > > - Inside refs/, which is shared by default except refs/bisect, add > refs/local/ which is per-worktree. We may eventually move > refs/bisect to this new location and remove the exception in refs > code. That sounds dangerous to me. There is already a concept of local and remote-tracking branches. So I would think that local may soon become an overused word, (just like "index" today or "recursive" to a lesser extend). Could this special area be more explicit? (refs/worktree-local/ ? or after peeking at the docs below refs/un-common/ ?)
On Tue, Sep 25, 2018 at 4:35 AM Stefan Beller <sbeller@google.com> wrote: > > On Sat, Sep 22, 2018 at 11:05 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > > > > When multiple worktrees are used, we need rules to determine if > > something belongs to one worktree or all of them. Instead of keeping > > adding rules when new stuff comes, have a generic rule: > > > > - Inside $GIT_DIR, which is per-worktree by default, add > > $GIT_DIR/common which is always shared. New features that want to > > share stuff should put stuff under this directory. > > So that /common is a directory and you have to use it specifically > in new code? That would be easy to overlook when coming up > with $GIT_DIR/foo for implementing the git-foo. There's no easy way out. I have to do _something_ if you want to share $GIT_DIR/foo to all worktrees. Either we have to update path.c and add "foo" which is not even an option for external commands, or we put "foo" in a common place, e.g. $GIT_DIR/common/foo. > > - Inside refs/, which is shared by default except refs/bisect, add > > refs/local/ which is per-worktree. We may eventually move > > refs/bisect to this new location and remove the exception in refs > > code. > > That sounds dangerous to me. There is already a concept of > local and remote-tracking branches. So I would think that local > may soon become an overused word, (just like "index" today or > "recursive" to a lesser extend). > > Could this special area be more explicit? > (refs/worktree-local/ ? or after peeking at the docs below > refs/un-common/ ?) refs/un-common sounds really "uncommon" :D. If refs/local is bad, I guess we could go with either refs/worktree-local, refs/worktree, refs/private, refs/per-worktree... My vote is on refs/worktree. I think as long as the word "worktree" is in there, people would notice the difference.
On Tue, Sep 25, 2018 at 8:36 AM Duy Nguyen <pclouds@gmail.com> wrote: > > On Tue, Sep 25, 2018 at 4:35 AM Stefan Beller <sbeller@google.com> wrote: > > > > On Sat, Sep 22, 2018 at 11:05 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > > > > > > When multiple worktrees are used, we need rules to determine if > > > something belongs to one worktree or all of them. Instead of keeping > > > adding rules when new stuff comes, have a generic rule: > > > > > > - Inside $GIT_DIR, which is per-worktree by default, add > > > $GIT_DIR/common which is always shared. New features that want to > > > share stuff should put stuff under this directory. > > > > So that /common is a directory and you have to use it specifically > > in new code? That would be easy to overlook when coming up > > with $GIT_DIR/foo for implementing the git-foo. > > There's no easy way out. I have to do _something_ if you want to share > $GIT_DIR/foo to all worktrees. Either we have to update path.c and add > "foo" which is not even an option for external commands, or we put > "foo" in a common place, e.g. $GIT_DIR/common/foo. > > > > - Inside refs/, which is shared by default except refs/bisect, add > > > refs/local/ which is per-worktree. We may eventually move > > > refs/bisect to this new location and remove the exception in refs > > > code. > > > > That sounds dangerous to me. There is already a concept of > > local and remote-tracking branches. So I would think that local > > may soon become an overused word, (just like "index" today or > > "recursive" to a lesser extend). > > > > Could this special area be more explicit? > > (refs/worktree-local/ ? or after peeking at the docs below > > refs/un-common/ ?) > > refs/un-common sounds really "uncommon" :D. If refs/local is bad, I > guess we could go with either refs/worktree-local, refs/worktree, > refs/private, refs/per-worktree... My vote is on refs/worktree. I refs/worktree sounds good to me (I do not object), but I am not overly enthused either, as when I think further worktrees and submodules are both features with a very similar nature in that they touch a lot of core concepts in Git, but seem to be a niche feature for the masses for now. For example I could think of submodules following this addressing mode as well: submodule/<path>/master sounds similar to the originally proposed worktree/<name>/<branch> convention. For now it is not quite clear to me why you would want to have access to the submodule refs in the superproject, but maybe the use case will come later. And with that said, I wonder if the "local" part should be feature agnostic, or if we want to be "local" for worktrees, "local" for remotes, "local" for submodules (i.e. our own refs vs submodule refs). > think as long as the word "worktree" is in there, people would notice > the difference. That makes sense. But is refs/worktree shared or local? It's not quite obvious to me, as I could have refs/worktree/<worktree-name>/master instead when it is shared, so I tend to favor refs/local-worktree/ a bit more, but that is more typing. :/ == As we grow the worktree feature, do we ever expect the need to reference the current worktree? For example when there is a ref "test" that could be unique per repo and in the common area, so refs/heads/test would describe it and "test" would get there in DWIM mode. But then I could also delete the common ref and recreate a "test" ref in worktree A, in worktree B however DWIMming "test" could still refer to A's "test" as it is unique (so far) in the repository. And maybe I would want to check if test exists locally, so I'd want to ask for "self/test" (with "self" == "B" as that is my cwd). Stefan
On Tue, Sep 25, 2018 at 6:24 PM Stefan Beller <sbeller@google.com> wrote: > > > That sounds dangerous to me. There is already a concept of > > > local and remote-tracking branches. So I would think that local > > > may soon become an overused word, (just like "index" today or > > > "recursive" to a lesser extend). > > > > > > Could this special area be more explicit? > > > (refs/worktree-local/ ? or after peeking at the docs below > > > refs/un-common/ ?) > > > > refs/un-common sounds really "uncommon" :D. If refs/local is bad, I > > guess we could go with either refs/worktree-local, refs/worktree, > > refs/private, refs/per-worktree... My vote is on refs/worktree. I > > refs/worktree sounds good to me (I do not object), but I am not > overly enthused either, as when I think further worktrees and > submodules are both features with a very similar nature in that > they touch a lot of core concepts in Git, but seem to be a niche > feature for the masses for now. I think the similarity is partly because submodule feature also has to manage worktrees. My view is at some point, this "git worktree" would be good enough that it can handle submodules as well (for the worktree part only of course) > For example I could think of submodules following this addressing > mode as well: submodule/<path>/master sounds similar to the > originally proposed worktree/<name>/<branch> convention. > For now it is not quite clear to me why you would want to have > access to the submodule refs in the superproject, but maybe > the use case will come later. Yeah. In theory we could "mount" the submodule ref store to a superproject's ref store. I think it may be needed just for the same reason it's needed for worktree: error reporting. If you peek into a submodule and say "HEAD has an error", the user will get confused whether it's superproject's HEAD or a submodule's HEAD. > And with that said, I wonder if the "local" part should be feature agnostic, > or if we want to be "local" for worktrees, "local" for remotes, "local" > for submodules (i.e. our own refs vs submodule refs). You lost me here. > > > think as long as the word "worktree" is in there, people would notice > > the difference. > > That makes sense. But is refs/worktree shared or local? It's not quite > obvious to me, as I could have refs/worktree/<worktree-name>/master > instead when it is shared, so I tend to favor refs/local-worktree/ a bit > more, but that is more typing. :/ OK I think mixing the two patches will different purposes messes you (or me) up ;-) refs/worktrees/xxx (and refs/main/xxx) are about visibility from other worktrees. Or like Eric put it, they are simply aliases. These refs are not shared because if they are, you can already see them without new "ref mount points" like this. refs/worktree (previously refs/local) is also per-worktree but it's specifically because you can't have per-worktree inside "refs/" (the only exception so far is refs/bisect which is hard coded). You can have refs outside "refs/" (like HEAD or FETCH_HEAD) and they will not be shared, but they cannot be iterated while those inside refs/ can be. This is more about deciding what to share and I believe is really worktree-specific and only matters to _current_ worktree. Since refs/worktree is per-worktree, you can also view them from a different worktree via refs/worktrees/. E.g. if you have refs/worktree/foo then another worktree can see it via refs/worktrees/xxx/refs/worktree/foo (besides pseudo refs like refs/worktrees/xxx/HEAD) > As we grow the worktree feature, do we ever expect the need to > reference the current worktree? > > For example when there is a ref "test" that could be unique per > repo and in the common area, so refs/heads/test would describe > it and "test" would get there in DWIM mode. > > But then I could also delete the common ref and recreate a "test" > ref in worktree A, in worktree B however DWIMming "test" could still > refer to A's "test" as it is unique (so far) in the repository. > And maybe I would want to check if test exists locally, so I'd > want to ask for "self/test" (with "self" == "B" as that is my cwd). You probably lost me again. In theory we must be able to detect ambiguity and stop DWIMing. If you want to be ambiguity-free, you specify full ref name, starting with "refs/" which should function like "self/" because worktree design so far is always about the current worktree's view.
On Tue, Sep 25, 2018 at 9:55 AM Duy Nguyen <pclouds@gmail.com> wrote: > > And with that said, I wonder if the "local" part should be feature agnostic, > > or if we want to be "local" for worktrees, "local" for remotes, "local" > > for submodules (i.e. our own refs vs submodule refs). > > You lost me here. Yeah, me too after rereading. :P I think the "local" part always implies that there is a part that is not local and depending on the feature you call it remote or other worktree. When writing this comment I briefly wondered if we want to combine the local aspects of the various features. However the "local" part really depends on the feature (e.g. a ref on a different worktree is still local from the here/remote perspective or from the superproject/submodule perspective), so I think I was misguided. > > > think as long as the word "worktree" is in there, people would notice > > > the difference. > > > > That makes sense. But is refs/worktree shared or local? It's not quite > > obvious to me, as I could have refs/worktree/<worktree-name>/master > > instead when it is shared, so I tend to favor refs/local-worktree/ a bit > > more, but that is more typing. :/ > > OK I think mixing the two patches will different purposes messes you > (or me) up ;-) possible. > > refs/worktrees/xxx (and refs/main/xxx) are about visibility from other > worktrees. Or like Eric put it, they are simply aliases. These refs > are not shared because if they are, you can already see them without > new "ref mount points" like this. > > refs/worktree (previously refs/local) is also per-worktree but it's > specifically because you can't have per-worktree inside "refs/" (the > only exception so far is refs/bisect which is hard coded). You can > have refs outside "refs/" (like HEAD or FETCH_HEAD) and they will not > be shared, but they cannot be iterated while those inside refs/ can > be. This is more about deciding what to share and I believe is really > worktree-specific and only matters to _current_ worktree. > > Since refs/worktree is per-worktree, you can also view them from a > different worktree via refs/worktrees/. E.g. if you have > refs/worktree/foo then another worktree can see it via > refs/worktrees/xxx/refs/worktree/foo (besides pseudo refs like > refs/worktrees/xxx/HEAD) Ah. now I seem to understand, thanks for explaining.
diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt index e85148f05e..fad404ed7c 100644 --- a/Documentation/gitrepository-layout.txt +++ b/Documentation/gitrepository-layout.txt @@ -95,8 +95,10 @@ refs:: References are stored in subdirectories of this directory. The 'git prune' command knows to preserve objects reachable from refs found in this directory and - its subdirectories. This directory is ignored if $GIT_COMMON_DIR - is set and "$GIT_COMMON_DIR/refs" will be used instead. + its subdirectories. + This directory is ignored (except refs/bisect and refs/local) + if $GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/refs" will be + used instead. refs/heads/`name`:: records tip-of-the-tree commit objects of branch `name` @@ -165,6 +167,11 @@ hooks:: each hook. This directory is ignored if $GIT_COMMON_DIR is set and "$GIT_COMMON_DIR/hooks" will be used instead. +common:: + When multiple working trees are used, most of files in + $GIT_DIR are per-worktree with a few known exceptions. All + files under 'common' however will be shared between all + working trees. index:: The current index file for the repository. It is diff --git a/path.c b/path.c index 34f0f98349..7eb61bf31b 100644 --- a/path.c +++ b/path.c @@ -108,6 +108,7 @@ struct common_dir { static struct common_dir common_list[] = { { 0, 1, 0, "branches" }, + { 0, 1, 0, "common" }, { 0, 1, 0, "hooks" }, { 0, 1, 0, "info" }, { 0, 0, 1, "info/sparse-checkout" }, diff --git a/refs.c b/refs.c index 9f7268d5fe..a851ef085b 100644 --- a/refs.c +++ b/refs.c @@ -624,6 +624,7 @@ int dwim_log(const char *str, int len, struct object_id *oid, char **log) static int is_per_worktree_ref(const char *refname) { return !strcmp(refname, "HEAD") || + starts_with(refname, "refs/local/") || starts_with(refname, "refs/bisect/") || starts_with(refname, "refs/rewritten/"); } diff --git a/refs/files-backend.c b/refs/files-backend.c index 16ef9325e0..416eafa453 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -269,9 +269,9 @@ static void loose_fill_ref_dir(struct ref_store *ref_store, closedir(d); /* - * Manually add refs/bisect, which, being per-worktree, might - * not appear in the directory listing for refs/ in the main - * repo. + * Manually add refs/bisect and refs/local, which, being + * per-worktree, might not appear in the directory listing for + * refs/ in the main repo. */ if (!strcmp(dirname, "refs/")) { int pos = search_ref_dir(dir, "refs/bisect/", 12); @@ -281,6 +281,14 @@ static void loose_fill_ref_dir(struct ref_store *ref_store, dir->cache, "refs/bisect/", 12, 1); add_entry_to_dir(dir, child_entry); } + + pos = search_ref_dir(dir, "refs/local/", 11); + + if (pos < 0) { + struct ref_entry *child_entry = create_dir_entry( + dir->cache, "refs/local/", 11, 1); + add_entry_to_dir(dir, child_entry); + } } } diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index cd74c0a471..c7b53e494b 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -306,6 +306,8 @@ test_git_path GIT_COMMON_DIR=bar hooks/me bar/hooks/me test_git_path GIT_COMMON_DIR=bar config bar/config test_git_path GIT_COMMON_DIR=bar packed-refs bar/packed-refs test_git_path GIT_COMMON_DIR=bar shallow bar/shallow +test_git_path GIT_COMMON_DIR=bar common bar/common +test_git_path GIT_COMMON_DIR=bar common/file bar/common/file # In the tests below, $(pwd) must be used because it is a native path on # Windows and avoids MSYS's path mangling (which simplifies "foo/../bar" and diff --git a/t/t1415-worktree-refs.sh b/t/t1415-worktree-refs.sh new file mode 100755 index 0000000000..0c2d5f89a9 --- /dev/null +++ b/t/t1415-worktree-refs.sh @@ -0,0 +1,36 @@ +#!/bin/sh + +test_description='per-worktree refs' + +. ./test-lib.sh + +test_expect_success 'setup' ' + test_commit initial && + test_commit wt1 && + test_commit wt2 && + git worktree add wt1 wt1 && + git worktree add wt2 wt2 && + git checkout initial +' + +test_expect_success 'add refs/local' ' + git update-ref refs/local/foo HEAD && + git -C wt1 update-ref refs/local/foo HEAD && + git -C wt2 update-ref refs/local/foo HEAD +' + +test_expect_success 'refs/local must not be packed' ' + git pack-refs --all && + test_path_is_missing .git/refs/tags/wt1 && + test_path_is_file .git/refs/local/foo && + test_path_is_file .git/worktrees/wt1/refs/local/foo && + test_path_is_file .git/worktrees/wt2/refs/local/foo +' + +test_expect_success 'refs/local are per-worktree' ' + test_cmp_rev local/foo initial && + ( cd wt1 && test_cmp_rev local/foo wt1 ) && + ( cd wt2 && test_cmp_rev local/foo wt2 ) +' + +test_done
When multiple worktrees are used, we need rules to determine if something belongs to one worktree or all of them. Instead of keeping adding rules when new stuff comes, have a generic rule: - Inside $GIT_DIR, which is per-worktree by default, add $GIT_DIR/common which is always shared. New features that want to share stuff should put stuff under this directory. - Inside refs/, which is shared by default except refs/bisect, add refs/local/ which is per-worktree. We may eventually move refs/bisect to this new location and remove the exception in refs code. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- Documentation/gitrepository-layout.txt | 11 ++++++-- path.c | 1 + refs.c | 1 + refs/files-backend.c | 14 +++++++--- t/t0060-path-utils.sh | 2 ++ t/t1415-worktree-refs.sh | 36 ++++++++++++++++++++++++++ 6 files changed, 60 insertions(+), 5 deletions(-) create mode 100755 t/t1415-worktree-refs.sh