Re: [PATCH] setup: do not create $X/gitdir unnecessarily when accessing git file $X

[Junio C Hamano]

Duy Nguyen  writes:

> The whole prune strategy is a bit messy trying to cover all cases
> while still keeping out of the user's way. Perhaps if we implement
> "git worktree mv", or even "worktree fixup" so the user can do it
> manually (back when the prune strategy commit was implemented, there
> was no git-worktree), then we don't need this magic any more.
>
> So, which way to go?

I'd prefer to see "conservative and minimal first and carefully
build up" instead of "snapping something together quickly and having
to patch it up in rapid succession before people get hurt." and that
is not limited to the "worktree" topic.

I think "if you move, you are on your own, do not do it." would be a
good starting point.  The user education material would say: if you
create worktree B out of the primary A, and if you do not like the
location B, you "rm -fr B" and then create a new C out of the
primary A at a desired place, and do not reuse location B for any
other purpose.  The list of worktrees kept somewhere in A would then
name the old location B, and it is OK to notice the staleness and
remove it, but we do not have to.  At that point, the magic can and
should go.

After setting the user expectation that way, it is fine to design
how we would give "worktree rm" and "worktree mv".  As long as
users' initial expectation is set to "you do not mv, ever", these
can be introduced without hurting their established use pattern that
would involve no 'mv'.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] setup: do not create $X/gitdir unnecessarily when accessing git file $X

[Jeff King]

On Mon, Nov 02, 2015 at 08:08:26PM +0100, Nguyễn Thái Ngọc Duy wrote:

> $X/gitdir is created, or refreshed, in order to keep a linked worktree
> from being pruned. But while git file is used as the foundation for
> linked worktrees, it's used for other purposes as well and we should
> not create $X/gitdir in those cases.
> 
> Tighten the check. Only update an existing file, which is an
> indication this is a linked worktree.

Hrm. I think this fixes the immediate problem, but it seems odd for us
to rely on "does the file exist"[1].

We trigger this code unconditionally from read_gitfile_gently(). But
.git files are a general-purpose mechanism.  Shouldn't we be doing this
only if we suspect we are working with a linked working tree directory
in the first place?

Or we do not know at all, because we are operating in the linked dir,
and seeing the presence of the "gitdir" file is the only way we say "ah,
it turns out we are linked, so we should take the opportunity to do some
maintenance"?

If the latter, then I guess this is the only way to do it. It does seem
a bit strange to me that an otherwise read-only operation (reading the
file) might involve writing[2].

-Peff

[1] You check only !stat(), so it is not really "does it exist", but
"can we stat it". I think that is OK, because this is an
opportunistic update, and failing the stat should just mean we don't
do the update.

[2] I suspect this code should use write_file_gently(). What happens if
I have a read-only linked checkout?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] setup: do not create $X/gitdir unnecessarily when accessing git file $X

[Nguyễn Thái Ngọc Duy]

$X/gitdir is created, or refreshed, in order to keep a linked worktree
from being pruned. But while git file is used as the foundation for
linked worktrees, it's used for other purposes as well and we should
not create $X/gitdir in those cases.

Tighten the check. Only update an existing file, which is an
indication this is a linked worktree.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 setup.c| 2 +-
 t/t0002-gitfile.sh | 7 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index d343725..b30d923 100644
--- a/setup.c
+++ b/setup.c
@@ -440,7 +440,7 @@ static void update_linked_gitdir(const char *gitfile, const 
char *gitdir)
struct stat st;
 
strbuf_addf(, "%s/gitdir", gitdir);
-   if (stat(path.buf, ) || st.st_mtime + 24 * 3600 < time(NULL))
+   if (!stat(path.buf, ) && st.st_mtime + 24 * 3600 < time(NULL))
write_file(path.buf, "%s", gitfile);
strbuf_release();
 }
diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index 9670e8c..b1b59f2 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh
@@ -99,6 +99,13 @@ test_expect_success 'check rev-list' '
test "$SHA" = "$(git rev-list HEAD)"
 '
 
+test_expect_success '$REAL/gitdir is not created on ordinary git file' '
+   echo "gitdir: $REAL" >expected &&
+   test_cmp expected .git &&
+   git status &&
+   ! test -f "$REAL"/gitdir
+'
+
 test_expect_success 'setup_git_dir twice in subdir' '
git init sgd &&
(
-- 
2.2.0.513.g477eb31

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] setup: do not create $X/gitdir unnecessarily when accessing git file $X

[Eric Sunshine]

On Mon, Nov 2, 2015 at 2:08 PM, Nguyễn Thái Ngọc Duy  wrote:
> $X/gitdir is created, or refreshed, in order to keep a linked worktree
> from being pruned. But while git file is used as the foundation for
> linked worktrees, it's used for other purposes as well and we should
> not create $X/gitdir in those cases.
>
> Tighten the check. Only update an existing file, which is an
> indication this is a linked worktree.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
> @@ -99,6 +99,13 @@ test_expect_success 'check rev-list' '
> +test_expect_success '$REAL/gitdir is not created on ordinary git file' '
> +   echo "gitdir: $REAL" >expected &&
> +   test_cmp expected .git &&
> +   git status &&
> +   ! test -f "$REAL"/gitdir

Minor: test_path_is_missing() might convey the intention a bit more clearly.

> +'
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] setup: do not create $X/gitdir unnecessarily when accessing git file $X

[Duy Nguyen]

(resend)

On Mon, Nov 2, 2015 at 9:51 PM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> [2] I suspect this code should use write_file_gently(). What happens if
>> I have a read-only linked checkout?

I can't hide anything from you guys can I? :) My first attempt was
move this update logic back to setup_..._gentle where it should
belong, but it got complicated because read_file_gently was buried too
deep and there was no easy way to get the information out.

I can try again, or..

>
> Or you may not be the owner of the repository, you think you are
> doing a read-only operation, and you silently end up creating a file
> that cannot be written by the repository owner?
>
> Honestly, I think this whole "just in case the user moved without
> telling us, we sneakily fix things without telling the user" should
> just go away.  This is not the first incidence of a tool trying to
> be overly clever and pretend to know better than the end user biting
> us, is it?

The whole prune strategy is a bit messy trying to cover all cases
while still keeping out of the user's way. Perhaps if we implement
"git worktree mv", or even "worktree fixup" so the user can do it
manually (back when the prune strategy commit was implemented, there
was no git-worktree), then we don't need this magic any more.

So, which way to go?

-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] setup: do not create $X/gitdir unnecessarily when accessing git file $X

[Junio C Hamano]

Jeff King  writes:

> [2] I suspect this code should use write_file_gently(). What happens if
> I have a read-only linked checkout?

Or you may not be the owner of the repository, you think you are
doing a read-only operation, and you silently end up creating a file
that cannot be written by the repository owner?

Honestly, I think this whole "just in case the user moved without
telling us, we sneakily fix things without telling the user" should
just go away.  This is not the first incidence of a tool trying to
be overly clever and pretend to know better than the end user biting
us, is it?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] setup: do not create $X/gitdir unnecessarily when accessing git file $X

[Jeff King]

On Mon, Nov 02, 2015 at 12:51:16PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > [2] I suspect this code should use write_file_gently(). What happens if
> > I have a read-only linked checkout?
> 
> Or you may not be the owner of the repository, you think you are
> doing a read-only operation, and you silently end up creating a file
> that cannot be written by the repository owner?
> 
> Honestly, I think this whole "just in case the user moved without
> telling us, we sneakily fix things without telling the user" should
> just go away.  This is not the first incidence of a tool trying to
> be overly clever and pretend to know better than the end user biting
> us, is it?

I have to admit, that was my gut feeling, too, but I do not know enough
about the problem it is solving to say whether it is a good tradeoff.
Unfortunately 23af91d102e1efaff33b77ab7746356835a3d600 did not have much
discussion. I didn't dig into the mailing list, though. I was hoping Duy
could summarize it. :)

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html