git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] init: don't reset core.filemode on git-new-workdirs.
@ 2021-03-21 12:28 Madhu
  2021-03-21 21:58 ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Madhu @ 2021-03-21 12:28 UTC (permalink / raw)
  To: git

From: Madhu <enometh@net.meer>

If the .git/config file is a symlink (as is the case of a .git created
by the contrib/workdir/git-new-workdir script) then the filemode tests
fail, and the filemode is reset to be false.  To avoid this only munge
core.filemode if .git/config is a regular file.
---
 builtin/init-db.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index dcc45bef51..b053107336 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -285,7 +285,8 @@ static int create_default_files(const char *template_path,
 	/* Check filemode trustability */
 	path = git_path_buf(&buf, "config");
 	filemode = TEST_FILEMODE;
-	if (TEST_FILEMODE && !lstat(path, &st1)) {
+	if (TEST_FILEMODE && !lstat(path, &st1)
+	    && (st1.st_mode & S_IFMT) == S_IFREG) {
 		struct stat st2;
 		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
 				!lstat(path, &st2) &&
-- 
2.31.0.dirty

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-21 12:28 [PATCH] init: don't reset core.filemode on git-new-workdirs Madhu
@ 2021-03-21 21:58 ` Junio C Hamano
  2021-03-22  2:40   ` Madhu
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2021-03-21 21:58 UTC (permalink / raw)
  To: Madhu; +Cc: git

Madhu <enometh@meer.net> writes:

> From: Madhu <enometh@net.meer>
>
> If the .git/config file is a symlink (as is the case of a .git created
> by the contrib/workdir/git-new-workdir script) then the filemode tests
> fail, and the filemode is reset to be false.  To avoid this only munge
> core.filemode if .git/config is a regular file.

Hmph, what's the sequence of events?  You let "git new-workdir" to
create a cheap copy of a working tree and then?  When new-workdir
returns, you already have a functional working tree with .git/
directory (in which there are many symbolic links).  So who wants or
needs to run "git init" there in the directory in the first place?

Is the problem being solved that running an unnecessary "git init"
in an already initialized repository does an unnecessary filemode
check?

If that is the case, I am not sure if asking "is it a symlink?" to
avoid the filemode trustability check is a good approach.  At that
point in the code you are patching, we have already determined if we
are running the "git init" in an already initialized repository
(i.e. "reinit"), so shouldn't we be basing the decision on it
instead?

I see that in a later part of the same function, we test if the
filesystem supports symbolic links but do so only when we are
running "git init" afresh.  Perhaps the filemode trustability check
and the config-set to record core.filemode should all be moved there
inside the "if (!reinit)" block.

All of the above assumes that the problem being solved is about what
happens when "git init" is run in an already functioning working
tree.  If I misread what problem you are trying to solve, then none
of what I suggested in the above may apply.

> ---
>  builtin/init-db.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Missing sign-off; please review Documentation/SubmittingPatches


> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index dcc45bef51..b053107336 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -285,7 +285,8 @@ static int create_default_files(const char *template_path,
>  	/* Check filemode trustability */
>  	path = git_path_buf(&buf, "config");
>  	filemode = TEST_FILEMODE;
> -	if (TEST_FILEMODE && !lstat(path, &st1)) {
> +	if (TEST_FILEMODE && !lstat(path, &st1)
> +	    && (st1.st_mode & S_IFMT) == S_IFREG) {
>  		struct stat st2;
>  		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
>  				!lstat(path, &st2) &&

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-21 21:58 ` Junio C Hamano
@ 2021-03-22  2:40   ` Madhu
  2021-03-22  4:53     ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Madhu @ 2021-03-22  2:40 UTC (permalink / raw)
  To: gitster; +Cc: git

*  Junio C Hamano <gitster@pobox.com> <xmqq1rc89nk7.fsf@gitster.g>
Wrote on Sun, 21 Mar 2021 14:58:16 -0700
> Madhu <enometh@meer.net> writes:
>> If the .git/config file is a symlink (as is the case of a .git created
>> by the contrib/workdir/git-new-workdir script) then the filemode tests
>> fail, and the filemode is reset to be false.  To avoid this only munge
>> core.filemode if .git/config is a regular file.
>
> Hmph, what's the sequence of events?  You let "git new-workdir" to
> create a cheap copy of a working tree and then?  When new-workdir
> returns, you already have a functional working tree with .git/
> directory (in which there are many symbolic links).  So who wants or
> needs to run "git init" there in the directory in the first place?

In this case it was part of a script which created commits from
tarballs.  If there was no .git git-init would create it. If there
was, and it should ov harmlessly "re-initialized" it (whatever that
means)

> Is the problem being solved that running an unnecessary "git init"
> in an already initialized repository does an unnecessary filemode
> check?

Yes the problem could have be avoided by not calling git-init in an
existing repository.  But the docs say that if there is a .git
directory, it would "reinitialize it".  Re-initialization was not
expected to change the filemode incorrectly.

git-new-workdir is extremly useful in this case. With no overhead I
can create a .git without doing a checkout. Then i can move the .git
into a directory where the tarball has been unpacked - do a "git add
-A -f", "git commit" I expect the commit to have the exact contents of
the tarball. and by paying attention to commit info and dates i have a
commit-sha1 reproducible history which can be created anywhere.

If the filemode is being changed in .git/config without my knowledge
all further commits in the repo are affected future tarball imports
are corrupted.

I was using this extensively to track changes to point releases and
the impact is serious (personally) and I have a lot of useless
repositories which I have been tracking for over a year whose
histories are not commit-sha reproducible.

> If that is the case, I am not sure if asking "is it a symlink?" to
> avoid the filemode trustability check is a good approach.  At that
> point in the code you are patching, we have already determined if we
> are running the "git init" in an already initialized repository
> (i.e. "reinit"), so shouldn't we be basing the decision on it
> instead?

> I see that in a later part of the same function, we test if the
> filesystem supports symbolic links but do so only when we are
> running "git init" afresh.  Perhaps the filemode trustability check
> and the config-set to record core.filemode should all be moved there
> inside the "if (!reinit)" block.
>
> All of the above assumes that the problem being solved is about what
> happens when "git init" is run in an already functioning working
> tree.  If I misread what problem you are trying to solve, then none
> of what I suggested in the above may apply.

I think you have understood the problem.  At present But doing the
filemode trustability check on .git/config assumes it is a regular
file anyway if it is to work at all.  My suggestion in the patch only
enforces that assumption explicitly.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-22  2:40   ` Madhu
@ 2021-03-22  4:53     ` Junio C Hamano
  2021-03-22  9:04       ` Madhu
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2021-03-22  4:53 UTC (permalink / raw)
  To: Madhu; +Cc: git

Madhu <enometh@meer.net> writes:

>> I see that in a later part of the same function, we test if the
>> filesystem supports symbolic links but do so only when we are
>> running "git init" afresh.  Perhaps the filemode trustability check
>> and the config-set to record core.filemode should all be moved there
>> inside the "if (!reinit)" block.
>>
>> All of the above assumes that the problem being solved is about what
>> happens when "git init" is run in an already functioning working
>> tree.  If I misread what problem you are trying to solve, then none
>> of what I suggested in the above may apply.
>
> I think you have understood the problem.  At present But doing the
> filemode trustability check on .git/config assumes it is a regular
> file anyway if it is to work at all.  My suggestion in the patch only
> enforces that assumption explicitly.

There are two points we should consider.

 * Historically, we've used .git/config as the sample file to check,
   but that was not because we wanted to make sure we can chmod the
   config file, but because we knew the file has to be there.  If
   .git/config is sometimes a symbolic link, and if chmod test
   requires a regular file, we do not have to use .git/config as the
   sample file.  We could instead switch to use a different,
   temporary, file.  After all, the symlink check I pointed out in
   the message you are responding to uses a brand new temporary
   filename for that, and there is no reason why we shouldn't do the
   same with a regular file for the filemode test.

 * If running "git init" in an already functioning repository can be
   a useful way to "re-initialize" and/or "correct" various aspect
   of the repository (e.g. perhaps core.filemode is incorrectly set
   originally and running "git init" again corrects it), we would
   want to allow that in a normal repository as well as in a
   repository that is created by new-workdir the same way.  Or if it
   is not useful and we want "re-initialize" not to touch the
   filemode, we would want to skip the check in a normal repository
   as well as in a new-workdir repository the same way.  That is why
   "if symlink, then skip" is wrong---it targets the new-workdir
   case specifically.

I personally do not have a strong opinion either way, but to me, it
seems that "does the filesystem support filemode?" and "does the
filesystem support symbolic link?" are at about the same level and
should be treated similarly unless there is a good reason not to.
And the symlink check is never done in "reinit" case, so perhaps
when "git init" is run again in an already functioning repository,
we should not muck with the filemode, either.

A natural conclusion of the line of thought is that we can move the
"check filemode trustability" block (from the comment to concluding
git_config_set()) inside the "if (!reinit)" that happens a bit later
and we'd be fine---as an existing normal repository, as well as what
new-workdir creates, won't have to do the "let's chmod +x/-x the
config file and see what happens" code at all (perhaps the attached
patch, which hasn't even been compile tested).

On the other hand, if it is worth "fixing" the filemode setting
while re-initializing, we probably should switch to use a temporary
file instead of 'config'.  And we may want to reconsider the placement
of the "is symlink supported?" check---which may also have to be
redone to "fix" its existing value.


 builtin/init-db.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git c/builtin/init-db.c w/builtin/init-db.c
index dcc45bef51..61817a02a8 100644
--- c/builtin/init-db.c
+++ w/builtin/init-db.c
@@ -282,20 +282,6 @@ static int create_default_files(const char *template_path,
 
 	initialize_repository_version(fmt->hash_algo, 0);
 
-	/* Check filemode trustability */
-	path = git_path_buf(&buf, "config");
-	filemode = TEST_FILEMODE;
-	if (TEST_FILEMODE && !lstat(path, &st1)) {
-		struct stat st2;
-		filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
-				!lstat(path, &st2) &&
-				st1.st_mode != st2.st_mode &&
-				!chmod(path, st1.st_mode));
-		if (filemode && !reinit && (st1.st_mode & S_IXUSR))
-			filemode = 0;
-	}
-	git_config_set("core.filemode", filemode ? "true" : "false");
-
 	if (is_bare_repository())
 		git_config_set("core.bare", "true");
 	else {
@@ -309,6 +295,20 @@ static int create_default_files(const char *template_path,
 	}
 
 	if (!reinit) {
+		/* Check filemode trustability */
+		path = git_path_buf(&buf, "config");
+		filemode = TEST_FILEMODE;
+		if (TEST_FILEMODE && !lstat(path, &st1)) {
+			struct stat st2;
+			filemode = (!chmod(path, st1.st_mode ^ S_IXUSR) &&
+					!lstat(path, &st2) &&
+					st1.st_mode != st2.st_mode &&
+					!chmod(path, st1.st_mode));
+			if (filemode && !reinit && (st1.st_mode & S_IXUSR))
+				filemode = 0;
+		}
+		git_config_set("core.filemode", filemode ? "true" : "false");
+
 		/* Check if symlink is supported in the work tree */
 		path = git_path_buf(&buf, "tXXXXXX");
 		if (!close(xmkstemp(path)) &&

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-22  4:53     ` Junio C Hamano
@ 2021-03-22  9:04       ` Madhu
  2021-03-22 18:02         ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Madhu @ 2021-03-22  9:04 UTC (permalink / raw)
  To: gitster; +Cc: git

*  Junio C Hamano <gitster@pobox.com> <xmqq7dlz94by.fsf@gitster.g>
Wrote on Sun, 21 Mar 2021 21:53:37 -0700
> There are two points we should consider.
>
>  * Historically, we've used .git/config as the sample file to check,
>    but that was not because we wanted to make sure we can chmod the
>    config file, but because we knew the file has to be there.  If
>    .git/config is sometimes a symbolic link, and if chmod test
>    requires a regular file, we do not have to use .git/config as the
>    sample file.  We could instead switch to use a different,
>    temporary, file.  After all, the symlink check I pointed out in
>    the message you are responding to uses a brand new temporary
>    filename for that, and there is no reason why we shouldn't do the
>    same with a regular file for the filemode test.
>
>  * If running "git init" in an already functioning repository can be
>    a useful way to "re-initialize" and/or "correct" various aspect
>    of the repository (e.g. perhaps core.filemode is incorrectly set
>    originally and running "git init" again corrects it), we would
>    want to allow that in a normal repository as well as in a
>    repository that is created by new-workdir the same way.  Or if it
>    is not useful and we want "re-initialize" not to touch the
>    filemode, we would want to skip the check in a normal repository
>    as well as in a new-workdir repository the same way.  That is why
>    "if symlink, then skip" is wrong---it targets the new-workdir
>    case specifically.

I'd say it doesn't (target the new-workdir case specifically).  The
lstat test on `config' is incorrect if `config' (or a new file) is not
a regular file, so I think it should be fixed anyway.  The new-workdir
case happens to be handled correctly once this is fixed.

> I personally do not have a strong opinion either way, but to me, it
> seems that "does the filesystem support filemode?" and "does the
> filesystem support symbolic link?" are at about the same level and
> should be treated similarly unless there is a good reason not to.
> And the symlink check is never done in "reinit" case, so perhaps
> when "git init" is run again in an already functioning repository,
> we should not muck with the filemode, either.

I'd think so (on the last point).  While it is understandable to
expect consistent re-initing behaviour (which is why the spurious
git-init was there) the expectations may not hold if the git directory
and work tree are on different filesystems - there is more scope for
making wrong inferences.

Typically checkout worktrees with new-workdir in /dev/shm for
advantages in the low overhead.

I do have repositories where .git/config is a symlink to a config.A
and I have other config.B files - all for the same repo but with
different upstreams. (I know better ways to do it but why should I be
prevented from doing this)

> A natural conclusion of the line of thought is that we can move the
> "check filemode trustability" block (from the comment to concluding
> git_config_set()) inside the "if (!reinit)" that happens a bit later
> and we'd be fine---as an existing normal repository, as well as what
> new-workdir creates, won't have to do the "let's chmod +x/-x the
> config file and see what happens" code at all (perhaps the attached
> patch, which hasn't even been compile tested).
>
> On the other hand, if it is worth "fixing" the filemode setting
> while re-initializing, we probably should switch to use a temporary
> file instead of 'config'.  And we may want to reconsider the placement
> of the "is symlink supported?" check---which may also have to be
> redone to "fix" its existing value.

I don't think the posted patch (snipped) would work as reinit is
always 1 and we are always a candidate for reiniting - I may be
missing something.

Using a new file for the filemode test would be a natural
improvement. But I dont see the added complexity as a win.  I can
imagine other scenarios that need to be fixed: calling git-init on a
worktree prepared by git-new-worktree (not git-new-workdir) on a FAT
fs with .git on ext2.  Calling git-init on a worktree prepared by
calling git-new-worktree with a parent which is checked out by
git-new-workdir. (The parent has a symlinked config). The
possibilities for fun endless :)



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-22  9:04       ` Madhu
@ 2021-03-22 18:02         ` Junio C Hamano
  2021-03-23  3:57           ` Madhu
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2021-03-22 18:02 UTC (permalink / raw)
  To: Madhu; +Cc: git

Madhu <enometh@meer.net> writes:

> *  Junio C Hamano <gitster@pobox.com> <xmqq7dlz94by.fsf@gitster.g>
>> ...
>> And the symlink check is never done in "reinit" case, so perhaps
>> when "git init" is run again in an already functioning repository,
>> we should not muck with the filemode, either.
>
> I'd think so (on the last point)....

So, assuming that you are with me to think that reinit should not
touch the filemode thing, ...

>> A natural conclusion of the line of thought is that we can move the
>> "check filemode trustability" block (from the comment to concluding
>> git_config_set()) inside the "if (!reinit)" that happens a bit later
>> and we'd be fine---as an existing normal repository, as well as what
>> new-workdir creates, won't have to do the "let's chmod +x/-x the
>> config file and see what happens" code at all (perhaps the attached
>> patch, which hasn't even been compile tested).
>> ...

... wouldn't the illustration patch I gave, which removed the "check
filemode" bit from the main codepath and moved it to inside an if
block that is executed only when "if (!reinit)" is true, "skip" the
problematic "check if config is a regular file whose executable bit
can be flipped and flopped" code in your use case, i.e. in an existing
repository?

> I don't think the posted patch (snipped) would work as reinit is
> always 1 and we are always a candidate for reiniting - I may be
> missing something.

In other words, yes, the illustration patch you are responding to
assumes that the "reinit" variable is set correctly (i.e. the HEAD
exists and sensibly readable if you run "git init" in an already
functioning working tree) and we can use it to avoid the filemode
check.

> Using a new file for the filemode test would be a natural
> improvement. 

That becomes necessary only if we want to futz with core.filemode
while doing "reinit", as .git/config can be a symlink.  When we are
creating a repository from scratch, we always create a regular file
to prepare .git/config, and there is no need to do that, if we are
happy to set core.filemode the same way as core.symlinks, i.e. only
check once when the repository is created.  No?

Thanks.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-22 18:02         ` Junio C Hamano
@ 2021-03-23  3:57           ` Madhu
  2021-03-23  6:39             ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Madhu @ 2021-03-23  3:57 UTC (permalink / raw)
  To: gitster; +Cc: git

*  Junio C Hamano <gitster@pobox.com> <xmqqr1k76p8d.fsf@gitster.g>
Wrote on Mon, 22 Mar 2021 11:02:42 -0700
>> I don't think the posted patch (snipped) would work as reinit is
>> always 1 and we are always a candidate for reiniting - I may be
>> missing something.
>
> In other words, yes, the illustration patch you are responding to
> assumes that the "reinit" variable is set correctly (i.e. the HEAD
> exists and sensibly readable if you run "git init" in an already
> functioning working tree) and we can use it to avoid the filemode
> check.

Ah yes indeed.

>> Using a new file for the filemode test would be a natural
>> improvement.
>
> That becomes necessary only if we want to futz with core.filemode
> while doing "reinit", as .git/config can be a symlink.  When we are
> creating a repository from scratch, we always create a regular file
> to prepare .git/config, and there is no need to do that, if we are
> happy to set core.filemode the same way as core.symlinks, i.e. only
> check once when the repository is created.  No?

Avoiding the filemode check completely during reinit is ok with me
because it gave me wrong results.  I can't speak for the original
author of the code - if his intention was to do it explicitly as part
of "reinitialization".

[In the latter case personally I'd want to edit the config file by hand
in response to a message from git - and I certainly do not want git to
change the config file behind my back without informing me.]

Thanks again.




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-23  3:57           ` Madhu
@ 2021-03-23  6:39             ` Junio C Hamano
  2021-03-23 16:53               ` Torsten Bögershausen
  2021-06-18  4:18               ` Madhu
  0 siblings, 2 replies; 13+ messages in thread
From: Junio C Hamano @ 2021-03-23  6:39 UTC (permalink / raw)
  To: Madhu; +Cc: git

Madhu <enometh@meer.net> writes:

> Avoiding the filemode check completely during reinit is ok with me
> because it gave me wrong results.  I can't speak for the original
> author of the code - if his intention was to do it explicitly as part
> of "reinitialization".

As the original author of the code, I know I meant filemode check to
be done and redone upon reinitialization in 4f629539 (init-db: check
template and repository format., 2005-11-25).

But then when 75d24499 (git-init: autodetect core.symlinks,
2007-08-31) started to autodetect symbolic link support, I somehow
ended up doing it only upon the repository creation.  Later,
2455406a (git-init: autodetect core.ignorecase, 2008-05-11) imitated
to check case sensitivity in the same block, doing it only once.

Either of these two commits would have been a good chance for us to
realize that filemode check should be done the same way, but somehow
nobody noticed X-<.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-23  6:39             ` Junio C Hamano
@ 2021-03-23 16:53               ` Torsten Bögershausen
  2021-03-23 17:45                 ` Junio C Hamano
  2021-06-18  4:18               ` Madhu
  1 sibling, 1 reply; 13+ messages in thread
From: Torsten Bögershausen @ 2021-03-23 16:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Madhu, git

On Mon, Mar 22, 2021 at 11:39:31PM -0700, Junio C Hamano wrote:
> Madhu <enometh@meer.net> writes:
>
> > Avoiding the filemode check completely during reinit is ok with me
> > because it gave me wrong results.  I can't speak for the original
> > author of the code - if his intention was to do it explicitly as part
> > of "reinitialization".
>
> As the original author of the code, I know I meant filemode check to
> be done and redone upon reinitialization in 4f629539 (init-db: check
> template and repository format., 2005-11-25).
>
> But then when 75d24499 (git-init: autodetect core.symlinks,
> 2007-08-31) started to autodetect symbolic link support, I somehow
> ended up doing it only upon the repository creation.  Later,
> 2455406a (git-init: autodetect core.ignorecase, 2008-05-11) imitated
> to check case sensitivity in the same block, doing it only once.
>
> Either of these two commits would have been a good chance for us to
> realize that filemode check should be done the same way, but somehow
> nobody noticed X-<.
>

In the very long run, there may be room for improvements:
While core.filemode works for a loal repo on a local disk,
there are lots of cases where I whish a better handling.

Exporting a git repo from e.g.
Linux/ext4 to MacOs : Linux sees the execute-bit as is, MacOs has it always on
Linux/ext4 to Windows : Linux sees the execute-bit as is, MacOs has it always off

Visiting the same repo under Git-for-Windows and cygwin:
cygwin supports the executable bit, Git-for-Windows does not.

And now we have the worktree (which may cross filesytem borders)

Today there are many use cases, where a single config variable is not ideal.

If there is a chance to have a "core.filemode=auto", which does probe the
filemode for this very OS/filesytem/worktree combination:
I would be happy to test/review/mentor such a code change.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-23 16:53               ` Torsten Bögershausen
@ 2021-03-23 17:45                 ` Junio C Hamano
  2021-03-23 20:31                   ` Torsten Bögershausen
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2021-03-23 17:45 UTC (permalink / raw)
  To: Torsten Bögershausen; +Cc: Madhu, git

Torsten Bögershausen <tboegi@web.de> writes:

> In the very long run, there may be room for improvements:
> While core.filemode works for a loal repo on a local disk,
> there are lots of cases where I whish a better handling.

But that is why we have multiple worktrees and per worktree
configuration, no?

Fundamentally, you cannot share a worktree from two systems and let
one work with filemode on while the other with filemode off, as you
summarized here:

> Exporting a git repo from e.g.
> Linux/ext4 to MacOs : Linux sees the execute-bit as is, MacOs has it always on
> Linux/ext4 to Windows : Linux sees the execute-bit as is, MacOs has it always off
>
> Visiting the same repo under Git-for-Windows and cygwin:
> cygwin supports the executable bit, Git-for-Windows does not.

The "new-workdirs" was a cute hack that was quite useful before the
multiple worktree support materialized, and it and (proper) worktree
share the quirk that by default the per-repo configuration file is
shared.

> And now we have the worktree (which may cross filesytem borders)
>
> Today there are many use cases, where a single config variable is not ideal.
>
> If there is a chance to have a "core.filemode=auto", which does probe the
> filemode for this very OS/filesytem/worktree combination:
> I would be happy to test/review/mentor such a code change.

I do not think we want to go there.  filemode is not the only thing
that would be shared.  What do you want to do with core.eol=native,
for example?  Paths touched while switching branches get the 'native'
line endings on the system that the user happened to be on when the
"switch" command was run, and working tree files end up with mixture?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-23 17:45                 ` Junio C Hamano
@ 2021-03-23 20:31                   ` Torsten Bögershausen
  2021-03-23 20:50                     ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Torsten Bögershausen @ 2021-03-23 20:31 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Madhu, git

On Tue, Mar 23, 2021 at 10:45:38AM -0700, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
> > In the very long run, there may be room for improvements:
> > While core.filemode works for a loal repo on a local disk,
> > there are lots of cases where I whish a better handling.
>
> But that is why we have multiple worktrees and per worktree
> configuration, no?
>
> Fundamentally, you cannot share a worktree from two systems and let
> one work with filemode on while the other with filemode off, as you
> summarized here:
>
> > Exporting a git repo from e.g.
> > Linux/ext4 to MacOs : Linux sees the execute-bit as is, MacOs has it always on
> > Linux/ext4 to Windows : Linux sees the execute-bit as is, MacOs has it always off
> >
> > Visiting the same repo under Git-for-Windows and cygwin:
> > cygwin supports the executable bit, Git-for-Windows does not.
>
> The "new-workdirs" was a cute hack that was quite useful before the
> multiple worktree support materialized, and it and (proper) worktree
> share the quirk that by default the per-repo configuration file is
> shared.
>
> > And now we have the worktree (which may cross filesytem borders)
> >
> > Today there are many use cases, where a single config variable is not ideal.
> >
> > If there is a chance to have a "core.filemode=auto", which does probe the
> > filemode for this very OS/filesytem/worktree combination:
> > I would be happy to test/review/mentor such a code change.
>
> I do not think we want to go there.  filemode is not the only thing
> that would be shared.  What do you want to do with core.eol=native,
> for example?  Paths touched while switching branches get the 'native'
> line endings on the system that the user happened to be on when the
> "switch" command was run, and working tree files end up with mixture?

I don't intend to solve all possible confusions caused by sharing all
config variables - just this very one.

After some thinking, the following may illustrate a brute-force method,
not nice, neither optimized, nor perfect or bug free:

$ alias
alias git='~/bin2/git.sh'

cat ~/bin2/git.sh

#!/bin/sh

probe_executable()
{
  touch $$.xxx
  if test -x $$.xxx; then
    rm $$.xxx
    echo "-c core.filemode=false"
    return
  fi
  chmod +x $$.xxx
  if ! test -x $$.xxx; then
    rm $$.xxx
    echo "-c core.filemode=false"
    return
  fi
  rm $$.xxx
  return
}
git $(probe_executable) "$@"

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-23 20:31                   ` Torsten Bögershausen
@ 2021-03-23 20:50                     ` Junio C Hamano
  0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2021-03-23 20:50 UTC (permalink / raw)
  To: Torsten Bögershausen; +Cc: Madhu, git

Torsten Bögershausen <tboegi@web.de> writes:

>> I do not think we want to go there.  filemode is not the only thing
>> that would be shared.  What do you want to do with core.eol=native,
>> for example?  Paths touched while switching branches get the 'native'
>> line endings on the system that the user happened to be on when the
>> "switch" command was run, and working tree files end up with mixture?
>
> I don't intend to solve all possible confusions caused by sharing all
> config variables - just this very one.

I intend to help users by drawing a clear red line and telling them
that crossing that line leads them to danger.

Sharing a working tree across systems that require different core.*
settings (filemode included) is on the other side of that line, and
that is the reason why I said I do not think we want to go there.
By saying "this is only about core.filemode but you can share a
working tree between incompatible systems", I am afraid that we end
up training users to go where they should not get nearby.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] init: don't reset core.filemode on git-new-workdirs.
  2021-03-23  6:39             ` Junio C Hamano
  2021-03-23 16:53               ` Torsten Bögershausen
@ 2021-06-18  4:18               ` Madhu
  1 sibling, 0 replies; 13+ messages in thread
From: Madhu @ 2021-06-18  4:18 UTC (permalink / raw)
  To: gitster; +Cc: git

I don't think this got any further attention?  I notice git-2.32 still
has the problem - in which case I re-commend my original patch.
Regards ---Madhu

*  Junio C Hamano <gitster@pobox.com> <xmqqr1k64bmk.fsf@gitster.g>
Wrote on Mon, 22 Mar 2021 23:39:31 -0700

> Madhu <enometh@meer.net> writes:
>> Avoiding the filemode check completely during reinit is ok with me
>> because it gave me wrong results.  I can't speak for the original
>> author of the code - if his intention was to do it explicitly as part
>> of "reinitialization".
>
> As the original author of the code, I know I meant filemode check to
> be done and redone upon reinitialization in 4f629539 (init-db: check
> template and repository format., 2005-11-25).
>
> But then when 75d24499 (git-init: autodetect core.symlinks,
> 2007-08-31) started to autodetect symbolic link support, I somehow
> ended up doing it only upon the repository creation.  Later,
> 2455406a (git-init: autodetect core.ignorecase, 2008-05-11) imitated
> to check case sensitivity in the same block, doing it only once.
>
> Either of these two commits would have been a good chance for us to
> realize that filemode check should be done the same way, but somehow
> nobody noticed X-<.
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-06-18  4:25 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-21 12:28 [PATCH] init: don't reset core.filemode on git-new-workdirs Madhu
2021-03-21 21:58 ` Junio C Hamano
2021-03-22  2:40   ` Madhu
2021-03-22  4:53     ` Junio C Hamano
2021-03-22  9:04       ` Madhu
2021-03-22 18:02         ` Junio C Hamano
2021-03-23  3:57           ` Madhu
2021-03-23  6:39             ` Junio C Hamano
2021-03-23 16:53               ` Torsten Bögershausen
2021-03-23 17:45                 ` Junio C Hamano
2021-03-23 20:31                   ` Torsten Bögershausen
2021-03-23 20:50                     ` Junio C Hamano
2021-06-18  4:18               ` Madhu

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).