No fchmd. was: Re: [PATCH 00/14] Add submodule test harness
On 07/09/2014 11:57 PM, Junio C Hamano wrote:
Eric Wong normalper...@yhbt.net writes:
Junio C Hamano gits...@pobox.com wrote:
Johannes Sixt j...@kdbg.org writes:
Am 08.07.2014 21:34, schrieb Jens Lehmann:
And Msysgit complains
error: fchmod on c:/xxxt/trash
directory.t7613-merge-submodule/submodule_update_repo/.git/modules/sub1/config.lock
failed: Function not implemented
I'm not sure what this is about, seems to happen during the cp -R of
the repo under .git/modules into the submodule.
No. It happens because fchmod() is not implemented in our Windows port.
Please see my band-aid patch at
http://thread.gmane.org/gmane.comp.version-control.git/248154/focus=20266
The sub-thread ended inconclusive.
We need to start somewhere, and a no-op fchmod() in your patch may
be as a good place to start as anything. At least we would then
keep the old behaviour without introducing any new failure.
Right, this likely makes the most sense for single-user systems or
systesm without a *nix-like permission system.
An alternative might be to use chmod() after we are done writing to
the config.lock in order to avoid the use of fchmod() altogether,
which I think can replace the existing two callsites of fchmod().
That approach might be a more expedient, but may turn out to be
undesirable in the longer term.
In that case, we would need to open with mode=0600 to avoid a window
where the file may be world-readable with any data in it.
Yes, of course.
To elaborate what I was alluding to at the end of the message you
are responding to a bit more, if we were to move this grab perms
from existing file (if there is any) and propagate to the new one
into the lockfile API,
- in hold_lock_file_for_update(), we would record the permission of
the original file, if any, to a new field in struct lock_file;
- open with 0600 or tighter in lock_file(), and
- either before closing the file use fchmod() or after closing and
moving the file use chmod() to propagate the permission.
If the original did not exist, we would pass 0666 to open as before
in lock_file() and do not bother chmod/fchmod at the end.
Or something like that, perhaps.
Isn't the whole problem starting here:
in config.c:
fd = hold_lock_file_for_update(lock, config_filename, 0);
In lockfile.c:
/* This should return a meaningful errno on failure */
int hold_lock_file_for_update(struct lock_file *lk, const char *path,
int flags)
{
int fd = lock_file(lk, path, flags);
which leads to
static int lock_file(struct lock_file *lk, const char *path, int flags)
[]
lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
There is no way to tell which permissions the new lockfile should have.
That is somewhat unlucky.
On the other hand, shouldn't we call
adjust_shared_perm(const char *path) from path.c on the config file?
And to all files which are fiddled through the lock_file API?
In other words, the lockfile could be created with the restrictive
permissions
600, and once the lockfile had been closed and renamed into the final name
we apply adjust_shared_perm() on it ?
Or probably directly after close() ?
I think there are 2 different things missing here:
- Be able to specify permissions to hold_lock_file_for_update(),
especially restrictive ones, like 600 and not 666.
- Adjust the permissions for shared files in a shared repo.
This is probably needed for a shared repo, when the user itself
has a umask which is too restrictive and adjust_shared_perm()
must be run to widen the permissions.
Do I miss something ?
--
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: No fchmd. was: Re: [PATCH 00/14] Add submodule test harness
Torsten Bögershausen tbo...@web.de writes:
Isn't the whole problem starting here:
in config.c:
fd = hold_lock_file_for_update(lock, config_filename, 0);
In lockfile.c:
/* This should return a meaningful errno on failure */
int hold_lock_file_for_update(struct lock_file *lk, const char
*path, int flags)
{
int fd = lock_file(lk, path, flags);
which leads to
static int lock_file(struct lock_file *lk, const char *path, int flags)
[]
lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
There is no way to tell which permissions the new lockfile should have.
We follow whategver user's umask says with this code.
That is somewhat unlucky.
On the other hand, shouldn't we call
adjust_shared_perm(const char *path) from path.c on the config file?
Good question, but I am not sure. For $GIT_DIR/config, I tend to
agree we should, but git config --global foo bar would not be a
shared file anyway, and my understanding of Eric's original
motivation is to keep $HOME/.gitconfig to be tighter than the user's
umask normally would indicate.
And to all files which are fiddled through the lock_file API?
In other words, the lockfile could be created with the restrictive
permissions
600, and once the lockfile had been closed and renamed into the final name
we apply adjust_shared_perm() on it ?
For all files that adjust-shared-perm should apply, yes, but I do
not think it is relevant to the codepath in question.
I think there are 2 different things missing here:
- Be able to specify permissions to hold_lock_file_for_update(),
especially restrictive ones, like 600 and not 666.
Yes (in the sense that yes we can add an extra parameter) and no
(in the sense that where would we get the value to pass to the
extra parameter from? would it be worth to add configurations
variables for different kinds of files?).
If we limit the case to Inherit permissions from the file we are
replacing by taking a lock on it, which is the topic of discussion
in this thread, we do not have to worry about how to configure the
value (we do not have to) and adding a new parameter to tell the
mode to hold-lock-file-for-update is unneeded (the function will
have a pathname of the original and can learn the current permission
bits itself).
- Adjust the permissions for shared files in a shared repo.
This is probably needed for a shared repo, when the user itself
has a umask which is too restrictive and adjust_shared_perm()
must be run to widen the permissions.
Don't we already do that for $GIT_DIR/config? In any case that will
not help $HOME/.gitconfig and other files that are not shared.
--
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: No fchmd. was: Re: [PATCH 00/14] Add submodule test harness
On 2014-07-10 21.49, Junio C Hamano wrote:
[]
If we limit the case to Inherit permissions from the file we are
replacing by taking a lock on it, which is the topic of discussion
in this thread, we do not have to worry about how to configure the
value (we do not have to) and adding a new parameter to tell the
mode to hold-lock-file-for-update is unneeded (the function will
have a pathname of the original and can learn the current permission
bits itself).
So something like this:
(I will probably not have the time to make a proper patch :-(
diff --git a/lockfile.c b/lockfile.c
index 4899270..134d5c8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -156,6 +156,11 @@ static void resolve_symlink(struct strbuf *path)
/* Make sure errno contains a meaningful value on error */
static int lock_file(struct lock_file *lk, const char *path, int flags)
{
+ int perms = 0666;
+ struct stat st;
+ if (!lstat(path, st))
+ perms = st.st_mode 0777;
+
if (!lock_file_list) {
/* One-time initialization */
sigchain_push_common(remove_lock_file_on_signal);
@@ -179,7 +184,7 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
if (!(flags LOCK_NODEREF))
resolve_symlink(lk-filename);
strbuf_addstr(lk-filename, LOCK_SUFFIX);
- lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
+ lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, perms);
if (lk-fd 0) {
strbuf_reset(lk-filename);
return -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: No fchmd. was: Re: [PATCH 00/14] Add submodule test harness
Torsten Bögershausen tbo...@web.de writes:
On 2014-07-10 21.49, Junio C Hamano wrote:
[]
If we limit the case to Inherit permissions from the file we are
replacing by taking a lock on it, which is the topic of discussion
in this thread, we do not have to worry about how to configure the
value (we do not have to) and adding a new parameter to tell the
mode to hold-lock-file-for-update is unneeded (the function will
have a pathname of the original and can learn the current permission
bits itself).
So something like this:
Yeah, I think something along those lines may be sufficient and we
do not have to do anything when closing/committing, at least POSIX
systems. I do not know if other filesystems we may care about let
you open with 0400 and still write into it, though.
(I will probably not have the time to make a proper patch :-(
That's OK. I see many names on Cc: who are all capable of helping
us ;-)
diff --git a/lockfile.c b/lockfile.c
index 4899270..134d5c8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -156,6 +156,11 @@ static void resolve_symlink(struct strbuf *path)
/* Make sure errno contains a meaningful value on error */
static int lock_file(struct lock_file *lk, const char *path, int flags)
{
+ int perms = 0666;
+ struct stat st;
+ if (!lstat(path, st))
+ perms = st.st_mode 0777;
+
if (!lock_file_list) {
/* One-time initialization */
sigchain_push_common(remove_lock_file_on_signal);
@@ -179,7 +184,7 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
if (!(flags LOCK_NODEREF))
resolve_symlink(lk-filename);
strbuf_addstr(lk-filename, LOCK_SUFFIX);
- lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);
+ lk-fd = open(lk-filename.buf, O_RDWR | O_CREAT | O_EXCL, perms);
if (lk-fd 0) {
strbuf_reset(lk-filename);
return -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
