Re: [PATCH 2/2] config: use chmod() instead of fchmod()
Am 17.07.2014 00:16, schrieb Junio C Hamano:
> Karsten Blees writes:
>
>> There is no fchmod() on native Windows platforms (MinGW and MSVC), and the
>> equivalent Win32 API (SetFileInformationByHandle) requires Windows Vista.
>>
>> Use chmod() instead.
>>
>> Signed-off-by: Karsten Blees
>> ---
>
> I am wondering if it is saner to just revert the fchmod() patch and
> replace it with something along the lines of
>
> http://thread.gmane.org/gmane.comp.version-control.git/251682/focus=253219
>
I also think it makes a lot of sense to handle permissions centrally.
However, with this patch, the permissions of the target file will
additionally be limited by umask (by passing them to open()), and then
overridden completely if core.sharedRepository is set.
Perhaps the lockfile API should respect the location of the lock files
(i.e. use core.sharedRepository in .git, 0666 in the work-tree, and
copy permissions anywhere else).
Another thing I find strange is that, by doing copy/replace, git silently
overwrites readonly files. If we grab the permissions from the source
file anyway, we should perhaps add 'if (!(perms & 0222)) error("file
is readonly");', or even 'access(filename, W_OK)'?
> Having said that, these are the only two callers of fchmod()
> currently in our code base, so I'll queue this patch to allow us to
> kick the problem-can down the road ;-)
>
Thanks.
--
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: use chmod() instead of fchmod()
Karsten Blees writes:
> There is no fchmod() on native Windows platforms (MinGW and MSVC), and the
> equivalent Win32 API (SetFileInformationByHandle) requires Windows Vista.
>
> Use chmod() instead.
>
> Signed-off-by: Karsten Blees
> ---
I am wondering if it is saner to just revert the fchmod() patch and
replace it with something along the lines of
http://thread.gmane.org/gmane.comp.version-control.git/251682/focus=253219
Having said that, these are the only two callers of fchmod()
currently in our code base, so I'll queue this patch to allow us to
kick the problem-can down the road ;-)
Thanks.
> config.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/config.c b/config.c
> index ba882a1..9767c4b 100644
> --- a/config.c
> +++ b/config.c
> @@ -1636,8 +1636,8 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
> MAP_PRIVATE, in_fd, 0);
> close(in_fd);
>
> - if (fchmod(fd, st.st_mode & 0) < 0) {
> - error("fchmod on %s failed: %s",
> + if (chmod(lock->filename, st.st_mode & 0) < 0) {
> + error("chmod on %s failed: %s",
> lock->filename, strerror(errno));
> ret = CONFIG_NO_WRITE;
> goto out_free;
> @@ -1815,8 +1815,8 @@ int git_config_rename_section_in_file(const char
> *config_filename,
>
> fstat(fileno(config_file), &st);
>
> - if (fchmod(out_fd, st.st_mode & 0) < 0) {
> - ret = error("fchmod on %s failed: %s",
> + if (chmod(lock->filename, st.st_mode & 0) < 0) {
> + ret = error("chmod on %s failed: %s",
> lock->filename, strerror(errno));
> goto out;
> }
> --
> 2.0.1.779.g26aeac4.dirty
>
> --
--
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: use chmod() instead of fchmod()
Am 16.07.2014 07:33, schrieb Johannes Sixt:
> Am 16.07.2014 00:54, schrieb Karsten Blees:
>> There is no fchmod() on native Windows platforms (MinGW and MSVC), and the
>> equivalent Win32 API (SetFileInformationByHandle) requires Windows Vista.
>>
>> Use chmod() instead.
>>
>> Signed-off-by: Karsten Blees
>> ---
>> config.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/config.c b/config.c
>> index ba882a1..9767c4b 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1636,8 +1636,8 @@ int git_config_set_multivar_in_file(const char
>> *config_filename,
>> MAP_PRIVATE, in_fd, 0);
>> close(in_fd);
>>
>> -if (fchmod(fd, st.st_mode & 0) < 0) {
>> -error("fchmod on %s failed: %s",
>> +if (chmod(lock->filename, st.st_mode & 0) < 0) {
>> +error("chmod on %s failed: %s",
>> lock->filename, strerror(errno));
>> ret = CONFIG_NO_WRITE;
>> goto out_free;
>> @@ -1815,8 +1815,8 @@ int git_config_rename_section_in_file(const char
>> *config_filename,
>>
>> fstat(fileno(config_file), &st);
>>
>> -if (fchmod(out_fd, st.st_mode & 0) < 0) {
>> -ret = error("fchmod on %s failed: %s",
>> +if (chmod(lock->filename, st.st_mode & 0) < 0) {
>> +ret = error("chmod on %s failed: %s",
>> lock->filename, strerror(errno));
>> goto out;
>> }
>>
>
> I assume you tested this patch on Windows. I am mildly surprised that
> (on Windows) chmod() works on a file that is still open.
>
> -- Hannes
>
Yes, file attributes can be set independently of open files. In fact, existing
code in git already does that in many places (via adjust_shared_perm(), which
is typically called while the file is open).
--
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: use chmod() instead of fchmod()
Am 16.07.2014 00:54, schrieb Karsten Blees:
> There is no fchmod() on native Windows platforms (MinGW and MSVC), and the
> equivalent Win32 API (SetFileInformationByHandle) requires Windows Vista.
>
> Use chmod() instead.
>
> Signed-off-by: Karsten Blees
> ---
> config.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/config.c b/config.c
> index ba882a1..9767c4b 100644
> --- a/config.c
> +++ b/config.c
> @@ -1636,8 +1636,8 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
> MAP_PRIVATE, in_fd, 0);
> close(in_fd);
>
> - if (fchmod(fd, st.st_mode & 0) < 0) {
> - error("fchmod on %s failed: %s",
> + if (chmod(lock->filename, st.st_mode & 0) < 0) {
> + error("chmod on %s failed: %s",
> lock->filename, strerror(errno));
> ret = CONFIG_NO_WRITE;
> goto out_free;
> @@ -1815,8 +1815,8 @@ int git_config_rename_section_in_file(const char
> *config_filename,
>
> fstat(fileno(config_file), &st);
>
> - if (fchmod(out_fd, st.st_mode & 0) < 0) {
> - ret = error("fchmod on %s failed: %s",
> + if (chmod(lock->filename, st.st_mode & 0) < 0) {
> + ret = error("chmod on %s failed: %s",
> lock->filename, strerror(errno));
> goto out;
> }
>
I assume you tested this patch on Windows. I am mildly surprised that
(on Windows) chmod() works on a file that is still open.
-- Hannes
--
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: use chmod() instead of fchmod()
There is no fchmod() on native Windows platforms (MinGW and MSVC), and the
equivalent Win32 API (SetFileInformationByHandle) requires Windows Vista.
Use chmod() instead.
Signed-off-by: Karsten Blees
---
config.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/config.c b/config.c
index ba882a1..9767c4b 100644
--- a/config.c
+++ b/config.c
@@ -1636,8 +1636,8 @@ int git_config_set_multivar_in_file(const char
*config_filename,
MAP_PRIVATE, in_fd, 0);
close(in_fd);
- if (fchmod(fd, st.st_mode & 0) < 0) {
- error("fchmod on %s failed: %s",
+ if (chmod(lock->filename, st.st_mode & 0) < 0) {
+ error("chmod on %s failed: %s",
lock->filename, strerror(errno));
ret = CONFIG_NO_WRITE;
goto out_free;
@@ -1815,8 +1815,8 @@ int git_config_rename_section_in_file(const char
*config_filename,
fstat(fileno(config_file), &st);
- if (fchmod(out_fd, st.st_mode & 0) < 0) {
- ret = error("fchmod on %s failed: %s",
+ if (chmod(lock->filename, st.st_mode & 0) < 0) {
+ ret = error("chmod on %s failed: %s",
lock->filename, strerror(errno));
goto out;
}
--
2.0.1.779.g26aeac4.dirty
--
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
