Re: [PATCH 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
On 11/16/2014 07:49 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: There is no reason for $GIT_DIR/config to be executable, plus this change will help clean up repositories affected by the bug that was fixed by the previous commit. I do not think we want to do this. It is a welcome bugfix to create $GIT_DIR/config without executable bit when and only when we create it. It is very much in line with There is no reason for $GIT_DIR/config to be executable---we do not need to make it executable ourselves, so we shouldn't, but we did which was a bug we want to fix in patch 1/2 you posted. But with the preserve existing permissions fix we did earlier, the end users are now allowed to flip the executable bit on for their own purpose, and dropping it without knowing why they are doing so is simply rude. And honestly, Git do *not* even want to know why the users want to flip the bit. So I would suggest not to spend any cycle or any code complexity to repair existing repositories. Having that bit on does not hurt anybody. Those who found it curious can flip that bit off and then Git with preserve existing permissions fix will keep that bit off from then on. I disagree. The point of preserve existing permissions was to allow people to make their config files more readable/writable than the default, with the main use case being to help users who want to hide secret information in their config files. I think it is really far-fetched to imagine that anybody made his config file executable on purpose. Whereas we are *sure* that we have created lots of repositories with config files that were set executable by accident. Let's redefine the feature that was added in 2.1 from preserve existing permissions to preserve existing read/write permissions and thereby help people clean up the mess we made. That being said, I still believe that executable config files are not a significant risk in practice, so I'm not going to lose sleep about it either way. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
Michael Haggerty mhag...@alum.mit.edu writes: On 11/16/2014 07:49 PM, Junio C Hamano wrote: ... So I would suggest not to spend any cycle or any code complexity to repair existing repositories. Having that bit on does not hurt anybody. Those who found it curious can flip that bit off and then Git with preserve existing permissions fix will keep that bit off from then on. I disagree. The point of preserve existing permissions was to allow people to make their config files more readable/writable than the default,... s/more/less/, I think, was the original motivation. Having to limit more tightly than usual was what made the config unusual among files under $GIT_DIR. If it were to loosen, Eric's change should not have been done in the first place. The sharedRepository setting to defeat the default umask is there for that kind of thing. That being said, I still believe that executable config files are not a significant risk ... It is merely an annoyance, to the same degree of annoyance we find when we see all files appear executable on a FAT floppy mounted on Linux ;-) I do not think it is a risk at all, and I do not see a point in going into people's repository and actively fixing it. People who notice can fix, and people who do not care do not care and are not harmed. -- 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 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
On 11/17/2014 04:33 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: On 11/16/2014 07:49 PM, Junio C Hamano wrote: ... So I would suggest not to spend any cycle or any code complexity to repair existing repositories. Having that bit on does not hurt anybody. Those who found it curious can flip that bit off and then Git with preserve existing permissions fix will keep that bit off from then on. I disagree. The point of preserve existing permissions was to allow people to make their config files more readable/writable than the default,... s/more/less/, I think, was the original motivation. Having to limit more tightly than usual was what made the config unusual among files under $GIT_DIR. If it were to loosen, Eric's change should not have been done in the first place. The sharedRepository setting to defeat the default umask is there for that kind of thing. Oops, you are right. I actually meant to type less or more, but I see that more would be pretty useless. That being said, I still believe that executable config files are not a significant risk ... It is merely an annoyance, to the same degree of annoyance we find when we see all files appear executable on a FAT floppy mounted on Linux ;-) I do not think it is a risk at all, and I do not see a point in going into people's repository and actively fixing it. People who notice can fix, and people who do not care do not care and are not harmed. OK, then, I'll send a new copy of patch 1/2 and drop 2/2. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
Michael Haggerty mhag...@alum.mit.edu writes: There is no reason for $GIT_DIR/config to be executable, plus this change will help clean up repositories affected by the bug that was fixed by the previous commit. I do not think we want to do this. It is a welcome bugfix to create $GIT_DIR/config without executable bit when and only when we create it. It is very much in line with There is no reason for $GIT_DIR/config to be executable---we do not need to make it executable ourselves, so we shouldn't, but we did which was a bug we want to fix in patch 1/2 you posted. But with the preserve existing permissions fix we did earlier, the end users are now allowed to flip the executable bit on for their own purpose, and dropping it without knowing why they are doing so is simply rude. And honestly, Git do *not* even want to know why the users want to flip the bit. So I would suggest not to spend any cycle or any code complexity to repair existing repositories. Having that bit on does not hurt anybody. Those who found it curious can flip that bit off and then Git with preserve existing permissions fix will keep that bit off from then on. -- 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 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
There is no reason for $GIT_DIR/config to be executable, plus this
change will help clean up repositories affected by the bug that was
fixed by the previous commit.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
config.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 9e42d38..0942e5f 100644
--- a/config.c
+++ b/config.c
@@ -1653,7 +1653,15 @@ int git_config_set_multivar_in_file(const char
*config_filename,
MAP_PRIVATE, in_fd, 0);
close(in_fd);
- if (chmod(lock-filename, st.st_mode 0) 0) {
+ /*
+* We make of the executable bits because (a) it
+* doesn't make sense to have executable bits set on
+* the config file, and (b) there was a bug in git 2.1
+* which caused the config file to be created with u+x
+* set, so this will help repair repositories created
+* with that version.
+*/
+ if (chmod(lock-filename, st.st_mode 07666) 0) {
error(chmod on %s failed: %s,
lock-filename, strerror(errno));
ret = CONFIG_NO_WRITE;
@@ -1832,7 +1840,7 @@ int git_config_rename_section_in_file(const char
*config_filename,
fstat(fileno(config_file), st);
- if (chmod(lock-filename, st.st_mode 0) 0) {
+ if (chmod(lock-filename, st.st_mode 07666) 0) {
ret = error(chmod on %s failed: %s,
lock-filename, strerror(errno));
goto out;
--
2.1.1
--
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 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
On 14.11.2014 23:26, Michael Haggerty wrote:
There is no reason for $GIT_DIR/config to be executable, plus this
change will help clean up repositories affected by the bug that was
fixed by the previous commit.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
config.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 9e42d38..0942e5f 100644
--- a/config.c
+++ b/config.c
@@ -1653,7 +1653,15 @@ int git_config_set_multivar_in_file(const char
*config_filename,
MAP_PRIVATE, in_fd, 0);
close(in_fd);
- if (chmod(lock-filename, st.st_mode 0) 0) {
+ /*
+ * We make of the executable bits because (a) it
We make *use* of
+ * doesn't make sense to have executable bits set on
+ * the config file, and (b) there was a bug in git 2.1
+ * which caused the config file to be created with u+x
+ * set, so this will help repair repositories created
+ * with that version.
+ */
+ if (chmod(lock-filename, st.st_mode 07666) 0) {
error(chmod on %s failed: %s,
lock-filename, strerror(errno));
ret = CONFIG_NO_WRITE;
@@ -1832,7 +1840,7 @@ int git_config_rename_section_in_file(const char
*config_filename,
fstat(fileno(config_file), st);
- if (chmod(lock-filename, st.st_mode 0) 0) {
+ if (chmod(lock-filename, st.st_mode 07666) 0) {
ret = error(chmod on %s failed: %s,
lock-filename, strerror(errno));
goto out;
--
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 2/2] config: clear the executable bits (if any) on $GIT_DIR/config
On 11/15/2014 08:32 AM, Stefan Beller wrote:
On 14.11.2014 23:26, Michael Haggerty wrote:
There is no reason for $GIT_DIR/config to be executable, plus this
change will help clean up repositories affected by the bug that was
fixed by the previous commit.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
config.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 9e42d38..0942e5f 100644
--- a/config.c
+++ b/config.c
@@ -1653,7 +1653,15 @@ int git_config_set_multivar_in_file(const char
*config_filename,
MAP_PRIVATE, in_fd, 0);
close(in_fd);
-if (chmod(lock-filename, st.st_mode 0) 0) {
+/*
+ * We make of the executable bits because (a) it
We make *use* of
Thanks for catching this. But it's even worse. I meant to type mask off.
Junio, would you mind fixing this if there is no other reason for a
re-roll? Thanks.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
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
