Re: [PATCH 00/14] Add submodule test harness
Jens Lehmann writes: > So what about adding "Currently only submodules living in the > root directory of the superproject with the default name (same > as the path) are supported." to the comment above the function? OK, done, and merged to 'next'. 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: No fchmod() under msygit - Was: Re: [PATCH 00/14] Add submodule test harness
Karsten Blees writes: > 1.) Permissions of files in .git are controlled by the core.sharedRepository > setting, and your patch seems to break that (i.e. if someone accidentally > has made .git/config world readable, git-config no longer fixes that, even > if core.sharedRepository=0600). For any repository, when you further run "git config" to update it, .git/config always exists with the permission bits initialized when "git init" was run there. And Eric's change is to propagate that same bits to a new file, so there wouldn't be any difference, no? And $HOME/.gitconfig should not be governed by "sharedRepository", and that was the primary focus of Eric's change, I would think. -- 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 fchmod() under msygit - Was: Re: [PATCH 00/14] Add submodule test harness
Am 09.07.2014 22:00, schrieb Eric Wong: > Torsten Bögershausen wrote: >> (And why is it "& 0" and not "& 0777") > > This is to preserve the uncommon sticky/sgid/suid bits. Probably not > needed, but better to keep as much intact as possible. > >> Can we avoid the fchmod() all together ? > > For single-user systems, sure. > > For multi-user systems with git-imap-send users and passwords in > $GIT_CONFIG, I suggest not. > -- > 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 > The Windows fchmod() problem is easily solved by using chmod() instead of fchmod(). The file name is known in both cases (and even used in the error message). However, IMO there are more fundamental problems with your patch (not related to Windows): 1.) Permissions of files in .git are controlled by the core.sharedRepository setting, and your patch seems to break that (i.e. if someone accidentally has made .git/config world readable, git-config no longer fixes that, even if core.sharedRepository=0600). 2.) Sensitive data such as passwords doesn't belong in config files in the first place, that's what git-credentials is good for. -- 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 00/14] Add submodule test harness
Am 14.07.2014 03:01, schrieb Junio C Hamano:
> Jens Lehmann writes:
>
>>> Perhaps squashing this to 7e8e5af9 instead?
>>
>> Yes please, this is much better than my first attempt.
>
> One thing that I found troubling is the ../../../ "three levels up"
> is hardcoded. Would it be always true for any value of "$1"? If
> the submodule is bound to the superproject at sub/dir/, not at dir/,
> for example, would it have to change?
Yes. And the code currently also doesn't handle submodules whose
name and path differ.
> I am not saying that we must support artibrary cases, but if there
> is such a limitation in the implementation, people who will use the
> helper in their new tests want it at least documented, I think.
Ah, I didn't think about other tests reusing that and only wrote
this as a local helper. But you're right, it would make sense to
reuse this function instead of coding that again (even though I'd
prefer to extract the generic helpers to t/lib-submodule.sh for
that purpose).
So what about adding "Currently only submodules living in the
root directory of the superproject with the default name (same
as the path) are supported." to the comment above the function?
>>> t/lib-submodule-update.sh | 19 ---
>>> 1 file changed, 12 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
>>> index e441b98..fc1da84 100755
>>> --- a/t/lib-submodule-update.sh
>>> +++ b/t/lib-submodule-update.sh
>>> @@ -110,18 +110,23 @@ replace_gitfile_with_git_dir () {
>>> }
>>>
>>> # Test that the .git directory in the submodule is unchanged (except for
>>> the
>>> -# core.worktree setting, which we temporarily restore). Call this function
>>> -# before test_submodule_content as the latter might write the index file
>>> -# leading to false positive index differences.
>>> +# core.worktree setting, which appears only in $GIT_DIR/modules/$1/config).
>>> +# Call this function before test_submodule_content as the latter might
>>> +# write the index file leading to false positive index differences.
>>> test_git_directory_is_unchanged () {
>>> (
>>> - cd "$1" &&
>>> - git config core.worktree "../../../$1"
>>> + cd ".git/modules/$1" &&
>>> + # does core.worktree point at the right place?
>>> + test "$(git config core.worktree)" = "../../../$1" &&
>>> + # remove it temporarily before comparing, as
>>> + # "$1/.git/config" lacks it...
>>> + git config --unset core.worktree
>>> ) &&
>>> diff -r ".git/modules/$1" "$1/.git" &&
>>> (
>>> - cd "$1" &&
>>> - GIT_WORK_TREE=. git config --unset core.worktree
>>> + # ... and then restore.
>>> + cd ".git/modules/$1" &&
>>> + git config core.worktree "../../../$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 fchmod() under msygit - Was: Re: [PATCH 00/14] Add submodule test harness
> Still, git might like to know what ACLs to apply to files at checkout > time. That would be a vast new feature, I think, and probably not > worth it, particularly since that would require dealing with the > different types of ACLs: NTFS/NFSv4/ZFS on the one hand, POSIX Draft > on the other, plus AFS and who knows what else -- ETOOMUCH IMO. To complete that thought... Inheritable ACLs + chmod should suffice. The user should setup inheritable ACLs for the directory where a repo is to be cloned, then on checkout git should just apply mode changes in such a way as to leave ACL "shapes" unchanged, only adding or removing bits from non-DENY ACEs according to the saved file modes. Inheritable ACEs should be left as-is, since git doesn't track directory permissions (right?), or if it did, then those should be edited just like normal ACEs. Nico -- -- 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 fchmod() under msygit - Was: Re: [PATCH 00/14] Add submodule test harness
On Mon, Jul 14, 2014 at 6:31 AM, Erik Faye-Lund wrote: > On Wed, Jul 9, 2014 at 10:00 PM, Eric Wong wrote: >> Torsten Bögershausen wrote: >> > > You're saying this as if Windows is a single-user system. It's not, > but it uses ACLs rather than POSIX permissions to manage file-system > permissions. So far we've opted to ignore ACLs in Git for Windows, > though. The clever thing to do (that some versions of ZFS do nowadays) is to use the new mask to "edit" the ACL as follows: - leave any DENY ACEs as-is; for all others continue - remove (reset) from any ACEs for Everyone (and/or Authenticated Users) any bits corresponding to zero'ed bits in the new mask's other bits - remove from any other ACEs that are not for the owner (@OWNER in NFSv4 speak) any bits corresponding to zero'ed bits in the new mask's group bits - remove from the owner's ACEs any bits corresponding to zero'ed bits in the new mask's owner bits, but with some exceptions, in particular the owner must retain the right to edit the ACL - add (set) to the Everyone (and/or Authenticated Users) ACEs a set of bits corresponding to the set bits in the new mask's other bits - add (set) either only to the ACE for the file's group (alternatively, to all non-owner, non-Everyone/Authenticated Users ACEs) a set of bits corresponding to the set bits in the new mask's group bits ... I.e., use the chmod mask to decrease/increase access without changing the "shape" of the ACL. Determining a file's mode_t from an ACL is similar, though it must take DENY entries into account: make a set of users/groups referred to by any ACEs in the ACL, divide them into owner, other (Everyone and/or Authenticated Users), and group (all others), find the maximal access granted. Still, git might like to know what ACLs to apply to files at checkout time. That would be a vast new feature, I think, and probably not worth it, particularly since that would require dealing with the different types of ACLs: NTFS/NFSv4/ZFS on the one hand, POSIX Draft on the other, plus AFS and who knows what else -- ETOOMUCH IMO. Nico -- -- 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 fchmod() under msygit - Was: Re: [PATCH 00/14] Add submodule test harness
On Wed, Jul 9, 2014 at 10:00 PM, Eric Wong wrote: > Torsten Bögershausen wrote: >> (And why is it "& 0" and not "& 0777") > > This is to preserve the uncommon sticky/sgid/suid bits. Probably not > needed, but better to keep as much intact as possible. > >> Can we avoid the fchmod() all together ? > > For single-user systems, sure. > > For multi-user systems with git-imap-send users and passwords in > $GIT_CONFIG, I suggest not. You're saying this as if Windows is a single-user system. It's not, but it uses ACLs rather than POSIX permissions to manage file-system permissions. So far we've opted to ignore ACLs in Git for Windows, though. -- 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 00/14] Add submodule test harness
Jens Lehmann writes:
>> Perhaps squashing this to 7e8e5af9 instead?
>
> Yes please, this is much better than my first attempt.
One thing that I found troubling is the ../../../ "three levels up"
is hardcoded. Would it be always true for any value of "$1"? If
the submodule is bound to the superproject at sub/dir/, not at dir/,
for example, would it have to change?
I am not saying that we must support artibrary cases, but if there
is such a limitation in the implementation, people who will use the
helper in their new tests want it at least documented, I think.
>> t/lib-submodule-update.sh | 19 ---
>> 1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
>> index e441b98..fc1da84 100755
>> --- a/t/lib-submodule-update.sh
>> +++ b/t/lib-submodule-update.sh
>> @@ -110,18 +110,23 @@ replace_gitfile_with_git_dir () {
>> }
>>
>> # Test that the .git directory in the submodule is unchanged (except for the
>> -# core.worktree setting, which we temporarily restore). Call this function
>> -# before test_submodule_content as the latter might write the index file
>> -# leading to false positive index differences.
>> +# core.worktree setting, which appears only in $GIT_DIR/modules/$1/config).
>> +# Call this function before test_submodule_content as the latter might
>> +# write the index file leading to false positive index differences.
>> test_git_directory_is_unchanged () {
>> (
>> -cd "$1" &&
>> -git config core.worktree "../../../$1"
>> +cd ".git/modules/$1" &&
>> +# does core.worktree point at the right place?
>> +test "$(git config core.worktree)" = "../../../$1" &&
>> +# remove it temporarily before comparing, as
>> +# "$1/.git/config" lacks it...
>> +git config --unset core.worktree
>> ) &&
>> diff -r ".git/modules/$1" "$1/.git" &&
>> (
>> -cd "$1" &&
>> -GIT_WORK_TREE=. git config --unset core.worktree
>> +# ... and then restore.
>> +cd ".git/modules/$1" &&
>> +git config core.worktree "../../../$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 00/14] Add submodule test harness
Am 10.07.2014 22:52, schrieb Junio C Hamano:
> Junio C Hamano writes:
>
>> Jens Lehmann writes:
>>
>>> I agree, but this case is special. The test asserts that nobody
>>> added, modified or removed *anything* inside the .git directory.
>>> The reason for problem we are seeing here is that I have to
>>> remove the core.worktree setting when moving the git directory
>>> from .git/modules into the submodule work tree.
>>
>> Hmph. Comparing the files with core.worktree removed sounds like a
>> workaround that knows too much about the implementation detail of
>> what is being tested. I am just wondering if core.worktree will
>> stay forever be the only thing that is special, or there may come
>> other things (perhaps as a fallout of integrating things like Duy's
>> multiple-worktree stuff).
>>
>> But perhaps we cannot do better than this.
>
> One thing we should be able to do (and must do) better is to
> validate that core.worktree in the relocated config file actually
> points at the right place. Unsetting before comparing may let us
> compare the relocated one in .git/modules/$1/config with the one
> that is embedded in the working tree (hence no .git/config), but the
> way your "how about this?" patch does, we wouldn't catch a possible
> breakage to the relocation code to point core.worktree to a bogus
> location, I'm afraid.
Indeed.
> Perhaps squashing this to 7e8e5af9 instead?
Yes please, this is much better than my first attempt.
> t/lib-submodule-update.sh | 19 ---
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index e441b98..fc1da84 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -110,18 +110,23 @@ replace_gitfile_with_git_dir () {
> }
>
> # Test that the .git directory in the submodule is unchanged (except for the
> -# core.worktree setting, which we temporarily restore). Call this function
> -# before test_submodule_content as the latter might write the index file
> -# leading to false positive index differences.
> +# core.worktree setting, which appears only in $GIT_DIR/modules/$1/config).
> +# Call this function before test_submodule_content as the latter might
> +# write the index file leading to false positive index differences.
> test_git_directory_is_unchanged () {
> (
> - cd "$1" &&
> - git config core.worktree "../../../$1"
> + cd ".git/modules/$1" &&
> + # does core.worktree point at the right place?
> + test "$(git config core.worktree)" = "../../../$1" &&
> + # remove it temporarily before comparing, as
> + # "$1/.git/config" lacks it...
> + git config --unset core.worktree
> ) &&
> diff -r ".git/modules/$1" "$1/.git" &&
> (
> - cd "$1" &&
> - GIT_WORK_TREE=. git config --unset core.worktree
> + # ... and then restore.
> + cd ".git/modules/$1" &&
> + git config core.worktree "../../../$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 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
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: [PATCH 00/14] Add submodule test harness
Junio C Hamano writes:
> Jens Lehmann writes:
>
>> I agree, but this case is special. The test asserts that nobody
>> added, modified or removed *anything* inside the .git directory.
>> The reason for problem we are seeing here is that I have to
>> remove the core.worktree setting when moving the git directory
>> from .git/modules into the submodule work tree.
>
> Hmph. Comparing the files with core.worktree removed sounds like a
> workaround that knows too much about the implementation detail of
> what is being tested. I am just wondering if core.worktree will
> stay forever be the only thing that is special, or there may come
> other things (perhaps as a fallout of integrating things like Duy's
> multiple-worktree stuff).
>
> But perhaps we cannot do better than this.
One thing we should be able to do (and must do) better is to
validate that core.worktree in the relocated config file actually
points at the right place. Unsetting before comparing may let us
compare the relocated one in .git/modules/$1/config with the one
that is embedded in the working tree (hence no .git/config), but the
way your "how about this?" patch does, we wouldn't catch a possible
breakage to the relocation code to point core.worktree to a bogus
location, I'm afraid.
Perhaps squashing this to 7e8e5af9 instead?
t/lib-submodule-update.sh | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index e441b98..fc1da84 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -110,18 +110,23 @@ replace_gitfile_with_git_dir () {
}
# Test that the .git directory in the submodule is unchanged (except for the
-# core.worktree setting, which we temporarily restore). Call this function
-# before test_submodule_content as the latter might write the index file
-# leading to false positive index differences.
+# core.worktree setting, which appears only in $GIT_DIR/modules/$1/config).
+# Call this function before test_submodule_content as the latter might
+# write the index file leading to false positive index differences.
test_git_directory_is_unchanged () {
(
- cd "$1" &&
- git config core.worktree "../../../$1"
+ cd ".git/modules/$1" &&
+ # does core.worktree point at the right place?
+ test "$(git config core.worktree)" = "../../../$1" &&
+ # remove it temporarily before comparing, as
+ # "$1/.git/config" lacks it...
+ git config --unset core.worktree
) &&
diff -r ".git/modules/$1" "$1/.git" &&
(
- cd "$1" &&
- GIT_WORK_TREE=. git config --unset core.worktree
+ # ... and then restore.
+ cd ".git/modules/$1" &&
+ git config core.worktree "../../../$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 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
No fchmd. was: Re: [PATCH 00/14] Add submodule test harness
On 07/09/2014 11:57 PM, Junio C Hamano wrote:
Eric Wong writes:
Junio C Hamano wrote:
Johannes Sixt 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: [PATCH 00/14] Add submodule test harness
Eric Wong writes: > Junio C Hamano wrote: >> Johannes Sixt 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. -- 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 fchmod() under msygit - Was: Re: [PATCH 00/14] Add submodule test harness
Torsten Bögershausen wrote: > (And why is it "& 0" and not "& 0777") This is to preserve the uncommon sticky/sgid/suid bits. Probably not needed, but better to keep as much intact as possible. > Can we avoid the fchmod() all together ? For single-user systems, sure. For multi-user systems with git-imap-send users and passwords in $GIT_CONFIG, I suggest not. -- 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 00/14] Add submodule test harness
Junio C Hamano wrote: > Johannes Sixt 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. -- 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 00/14] Add submodule test harness
Jens Lehmann writes:
> I agree, but this case is special. The test asserts that nobody
> added, modified or removed *anything* inside the .git directory.
> The reason for problem we are seeing here is that I have to
> remove the core.worktree setting when moving the git directory
> from .git/modules into the submodule work tree.
Hmph. Comparing the files with core.worktree removed sounds like a
workaround that knows too much about the implementation detail of
what is being tested. I am just wondering if core.worktree will
stay forever be the only thing that is special, or there may come
other things (perhaps as a fallout of integrating things like Duy's
multiple-worktree stuff).
But perhaps we cannot do better than this.
> Could you please test the following? It should avoid this kind
> of problem by removing the core.worktree setting temporarily
> from the original config in .git/modules instead of adding it
> temporarily to .git/config:
> ---8<---
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index 3584755..98c86e3 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -143,18 +143,18 @@ replace_gitfile_with_git_dir () {
> }
>
> # Test that the .git directory in the submodule is unchanged (except for the
> -# core.worktree setting, which we temporarily restore). Call this function
> +# core.worktree setting, which we temporarily remove). Call this function
> # before test_submodule_content as the latter might write the index file
> # leading to false positive index differences.
> test_git_directory_is_unchanged () {
> (
> - cd "$1" &&
> - git config core.worktree "../../../$1"
> + cd ".git/modules/$1" &&
> + git config --unset core.worktree
> ) &&
> diff -r ".git/modules/$1" "$1/.git" &&
> (
> - cd "$1" &&
> - GIT_WORK_TREE=. git config --unset core.worktree
> + cd ".git/modules/$1" &&
> + git config core.worktree "../../../$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 00/14] Add submodule test harness
Johannes Sixt 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. 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. I also wonder if this "carry forward the original permission bits when updating an existing file by first writing the updated contents into a lockfile and then renaming it after we are done" pattern ought to be done in the lockfile API at commit_lock_file() time (Duy and Michael Cc'ed for their input, as they have recently touched lockfile API implementation in their series somewhat), not at the level of the user of the lockfile API like Eric's patch daa22c6f (config: preserve config file permissions on edits, 2014-05-06) did only for the config file. -- 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 00/14] Add submodule test harness
Am 09.07.2014 08:14, schrieb Torsten Bögershausen:
>
>>> There seems to be some other trouble under Mac OS, not yet fully tracked
>>> down,
>>> (may be related to the "diff -r")
>> Torsten sees failures of this kind under Mac OS:
>>
>> diff -r .git/modules/sub1/config sub1/.git/config
>> 6d5
>> < worktree = ../../../sub1
>> 8a8
>>> worktree = ../../../sub1
>> So the config contains the same content, but the worktree setting moved
>> to a different line. This seems to be the result of setting core.worktree
>> in the test_git_directory_is_unchanged function just before the "diff -r",
>> but only under Mac OS.
>>
> So I was suspecting diff -r beinng non-portable, but that doesn't seem to be
> the problem here.
> (But I wouldn't be surprised if there where problems with diff -r on some
> Unix systems)
> Anyway, checking all the files in the working tree seems to be a good thing
> to do,
> but that does not necessarily work for .git/config.
I agree, but this case is special. The test asserts that nobody
added, modified or removed *anything* inside the .git directory.
The reason for problem we are seeing here is that I have to
remove the core.worktree setting when moving the git directory
from .git/modules into the submodule work tree. So the test adds
it again to be able to diff it, and this happens in a different
line only on Mac OS as comparing the two core sections shows:
> -
> [core]
> repositoryformatversion = 0
> filemode = true
> bare = false
> logallrefupdates = true
> worktree = ../../../sub1
> ignorecase = true
> precomposeunicode = true
> [remote "origin"]
vs.
>
> [core]
> repositoryformatversion = 0
> filemode = true
> bare = false
> logallrefupdates = true
> ignorecase = true
> precomposeunicode = true
> worktree = ../../../sub1
> [remote "origin"]
And now it's clear what happens here: On Mac OS the ignorecase
and precomposeunicode settings are added behind the worktree
line, then re-adding worktree later for the comparison adds it
after these two.
Could you please test the following? It should avoid this kind
of problem by removing the core.worktree setting temporarily
from the original config in .git/modules instead of adding it
temporarily to .git/config:
---8<---
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 3584755..98c86e3 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -143,18 +143,18 @@ replace_gitfile_with_git_dir () {
}
# Test that the .git directory in the submodule is unchanged (except for the
-# core.worktree setting, which we temporarily restore). Call this function
+# core.worktree setting, which we temporarily remove). Call this function
# before test_submodule_content as the latter might write the index file
# leading to false positive index differences.
test_git_directory_is_unchanged () {
(
- cd "$1" &&
- git config core.worktree "../../../$1"
+ cd ".git/modules/$1" &&
+ git config --unset core.worktree
) &&
diff -r ".git/modules/$1" "$1/.git" &&
(
- cd "$1" &&
- GIT_WORK_TREE=. git config --unset core.worktree
+ cd ".git/modules/$1" &&
+ git config core.worktree "../../../$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 00/14] Add submodule test harness
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. -- 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
Re: [PATCH 00/14] Add submodule test harness
Torsten Bögershausen writes: > A "brute force" approach could be to simply run the config file(s) > through sort and compare them: > > sort <.git/modules/sub1/config >expect && > sort actual && > test_cmp expect actual && Or "git config --list" from these files, perhaps? -- 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
No fchmod() under msygit - Was: Re: [PATCH 00/14] Add submodule test harness
On 07/08/2014 10:25 PM, Ramsay Jones wrote:
On 08/07/14 20:34, Jens Lehmann wrote:
Am 07.07.2014 21:40, schrieb Torsten Bögershausen:
On 2014-07-07 19.05, Junio C Hamano wrote:
Jens Lehmann writes:
Junio, do you want me to resend 02/14 without the non-portable "echo -n"
or could you just squash the following diff in?
Amended locally here already; thanks, both.
There seems to be some other trouble under Mac OS, not yet fully tracked down,
(may be related to the "diff -r")
Torsten sees failures of this kind under Mac OS:
diff -r .git/modules/sub1/config sub1/.git/config
6d5
< worktree = ../../../sub1
8a8
worktree = ../../../sub1
So the config contains the same content, but the worktree setting moved
to a different line. This seems to be the result of setting core.worktree
in the test_git_directory_is_unchanged function just before the "diff -r",
but only under Mac OS.
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.
I haven't looked into this at all, but from the above message, and
noting that fchmod() is not implemented in mingw (see compat/mingw.h
line 91), and the following:
$ git grep -n fchmodcommit daa22c6f8da466bd7a438f1bc27375fd737ffcf3
Author: Eric Wong
Date: Tue May 6 00:17:14 2014 +
config: preserve config file permissions on edits
compat/mingw.h:91:static inline int fchmod(int fildes, mode_t mode)
config.c:1639: if (fchmod(fd, st.st_mode & 0) < 0) {
config.c:1640: error("fchmod on %s failed: %s",
config.c:1818: if (fchmod(out_fd, st.st_mode & 0) < 0) {
config.c:1819: ret = error("fchmod on %s failed: %s",
$
[I happen to be on the pu branch at the moment, so YMMV!]
Both calls to fchmod() above are on config lock files, one
in git_config_set_multivar_in_file() and the other in
git_config_rename_section_in_file().
commit daa22c6f8da466bd7a438f1bc27375fd737ffcf3
Author: Eric Wong
Date: Tue May 6 00:17:14 2014 +
config: preserve config file permissions on edits
(And why is it "& 0" and not "& 0777")
Can we avoid the fchmod() all together ?
--
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 00/14] Add submodule test harness
There seems to be some other trouble under Mac OS, not yet fully tracked down, (may be related to the "diff -r") Torsten sees failures of this kind under Mac OS: diff -r .git/modules/sub1/config sub1/.git/config 6d5 < worktree = ../../../sub1 8a8 worktree = ../../../sub1 So the config contains the same content, but the worktree setting moved to a different line. This seems to be the result of setting core.worktree in the test_git_directory_is_unchanged function just before the "diff -r", but only under Mac OS. So I was suspecting diff -r beinng non-portable, but that doesn't seem to be the problem here. (But I wouldn't be surprised if there where problems with diff -r on some Unix systems) Anyway, checking all the files in the working tree seems to be a good thing to do, but that does not necessarily work for .git/config. A "brute force" approach could be to simply run the config file(s) through sort and compare them: sort <.git/modules/sub1/config >expect && sort actual && test_cmp expect actual && rm expect actual && cp git/modules/sub1/config sub1/.git/config [end of scriptlet] And here the "dumps" of the 2 config files: ... Branch remove_sub1 set up to track remote branch remove_sub1 from origin. warning: unable to rmdir sub1: Directory not empty Updating 68c8810..81b9f6a Fast-forward .gitmodules | 4 1 file changed, 4 deletions(-) delete mode 100644 .gitmodules - [core] repositoryformatversion = 0 filemode = true bare = false logallrefupdates = true worktree = ../../../sub1 ignorecase = true precomposeunicode = true [remote "origin"] url = /Users/tb/projects/git/tb.140704_JensLehman/t/trash directory.t7613-merge-submodule/submodule_update_repo/. fetch = +refs/heads/*:refs/remotes/origin/* [branch "master"] remote = origin merge = refs/heads/master [core] repositoryformatversion = 0 filemode = true bare = false logallrefupdates = true ignorecase = true precomposeunicode = true worktree = ../../../sub1 [remote "origin"] url = /Users/tb/projects/git/tb.140704_JensLehman/t/trash directory.t7613-merge-submodule/submodule_update_repo/. fetch = +refs/heads/*:refs/remotes/origin/* [branch "master"] remote = origin merge = refs/heads/master = diff -r .git/modules/sub1/config sub1/.git/config 6d5 < worktree = ../../../sub1 8a8 > worktree = ../../../sub1 not ok 7 - git merge: removed submodule leaves submodule containing a .git directory alone -- 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 00/14] Add submodule test harness
On 08/07/14 21:25, Ramsay Jones wrote:
> On 08/07/14 20:34, Jens Lehmann wrote:
>> Am 07.07.2014 21:40, schrieb Torsten Bögershausen:
>>> On 2014-07-07 19.05, Junio C Hamano wrote:
Jens Lehmann writes:
> Junio, do you want me to resend 02/14 without the non-portable "echo -n"
> or could you just squash the following diff in?
Amended locally here already; thanks, both.
>>>
>>> There seems to be some other trouble under Mac OS, not yet fully tracked
>>> down,
>>> (may be related to the "diff -r")
>>
>> Torsten sees failures of this kind under Mac OS:
>>
>> diff -r .git/modules/sub1/config sub1/.git/config
>> 6d5
>> < worktree = ../../../sub1
>> 8a8
>>> worktree = ../../../sub1
>>
>> So the config contains the same content, but the worktree setting moved
>> to a different line. This seems to be the result of setting core.worktree
>> in the test_git_directory_is_unchanged function just before the "diff -r",
>> but only under Mac OS.
>>
>>> 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.
>
> I haven't looked into this at all, but from the above message, and
> noting that fchmod() is not implemented in mingw (see compat/mingw.h
> line 91), and the following:
>
> $ git grep -n fchmod
> compat/mingw.h:91:static inline int fchmod(int fildes, mode_t mode)
> config.c:1639: if (fchmod(fd, st.st_mode & 0) < 0) {
> config.c:1640: error("fchmod on %s failed: %s",
> config.c:1818: if (fchmod(out_fd, st.st_mode & 0) < 0) {
> config.c:1819: ret = error("fchmod on %s failed: %s",
> $
>
> [I happen to be on the pu branch at the moment, so YMMV!]
>
> Both calls to fchmod() above are on config lock files, one
> in git_config_set_multivar_in_file() and the other in
> git_config_rename_section_in_file().
>
See commit daa22c6f8 ("config: preserve config file permissions
on edits", 06-05-2014).
ATB,
Ramsay Jones
--
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 00/14] Add submodule test harness
On 08/07/14 20:34, Jens Lehmann wrote:
> Am 07.07.2014 21:40, schrieb Torsten Bögershausen:
>> On 2014-07-07 19.05, Junio C Hamano wrote:
>>> Jens Lehmann writes:
>>>
Junio, do you want me to resend 02/14 without the non-portable "echo -n"
or could you just squash the following diff in?
>>>
>>> Amended locally here already; thanks, both.
>>
>> There seems to be some other trouble under Mac OS, not yet fully tracked
>> down,
>> (may be related to the "diff -r")
>
> Torsten sees failures of this kind under Mac OS:
>
> diff -r .git/modules/sub1/config sub1/.git/config
> 6d5
> < worktree = ../../../sub1
> 8a8
>> worktree = ../../../sub1
>
> So the config contains the same content, but the worktree setting moved
> to a different line. This seems to be the result of setting core.worktree
> in the test_git_directory_is_unchanged function just before the "diff -r",
> but only under Mac OS.
>
>> 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.
I haven't looked into this at all, but from the above message, and
noting that fchmod() is not implemented in mingw (see compat/mingw.h
line 91), and the following:
$ git grep -n fchmod
compat/mingw.h:91:static inline int fchmod(int fildes, mode_t mode)
config.c:1639: if (fchmod(fd, st.st_mode & 0) < 0) {
config.c:1640: error("fchmod on %s failed: %s",
config.c:1818: if (fchmod(out_fd, st.st_mode & 0) < 0) {
config.c:1819: ret = error("fchmod on %s failed: %s",
$
[I happen to be on the pu branch at the moment, so YMMV!]
Both calls to fchmod() above are on config lock files, one
in git_config_set_multivar_in_file() and the other in
git_config_rename_section_in_file().
ATB,
Ramsay Jones
--
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 00/14] Add submodule test harness
Am 07.07.2014 21:40, schrieb Torsten Bögershausen: > On 2014-07-07 19.05, Junio C Hamano wrote: >> Jens Lehmann writes: >> >>> Junio, do you want me to resend 02/14 without the non-portable "echo -n" >>> or could you just squash the following diff in? >> >> Amended locally here already; thanks, both. > > There seems to be some other trouble under Mac OS, not yet fully tracked down, > (may be related to the "diff -r") Torsten sees failures of this kind under Mac OS: diff -r .git/modules/sub1/config sub1/.git/config 6d5 < worktree = ../../../sub1 8a8 > worktree = ../../../sub1 So the config contains the same content, but the worktree setting moved to a different line. This seems to be the result of setting core.worktree in the test_git_directory_is_unchanged function just before the "diff -r", but only under Mac OS. > 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. I'm currently investigating both issues (the next steps probably being to install msysgit and to do some Git hacking on a Mac in the family). -- 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 00/14] Add submodule test harness
On 2014-07-07 19.05, Junio C Hamano wrote: > Jens Lehmann writes: > >> Junio, do you want me to resend 02/14 without the non-portable "echo -n" >> or could you just squash the following diff in? > > Amended locally here already; thanks, both. There seems to be some other trouble under Mac OS, not yet fully tracked down, (may be related to the "diff -r") 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 -- 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 00/14] Add submodule test harness
Jens Lehmann writes:
> Junio, do you want me to resend 02/14 without the non-portable "echo -n"
> or could you just squash the following diff in?
Amended locally here already; thanks, both.
>
> -8<
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index 24c9fd7..3584755 100755
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -304,7 +304,7 @@ test_submodule_switch () {
> (
> cd submodule_update &&
> git branch -t add_sub1 origin/add_sub1 &&
> - echo -n >sub1 &&
> + >sub1 &&
> test_must_fail $command add_sub1 &&
> test_superproject_content origin/no_submodule &&
> test_must_be_empty sub1
> @@ -547,7 +547,7 @@ test_submodule_forced_switch () {
> (
> cd submodule_update &&
> git branch -t add_sub1 origin/add_sub1 &&
> - echo -n >sub1 &&
> + >sub1 &&
> $command add_sub1 &&
> test_superproject_content origin/add_sub1 &&
> test_dir_is_empty sub1
> -- 2.0.1.458.gf680257.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 00/14] Add submodule test harness
Am 03.07.2014 07:56, schrieb Torsten Bögershausen:
> On 07/02/2014 09:57 PM, Jens Lehmann wrote:
>> Am 02.07.2014 16:54, schrieb Torsten Bögershausen:
>>> (Not sure if this is the right thread)
>>> (I haven't checked if this is fixed in your latest version)
>> Yes, this is the right thread and no, it isn't fixed yet.
>>
>>> On what I have on pu 7a0da7902cbbc9a876b90c9, Tue Jul 1 14:51:53 2014 -0700
>>>
>>> Many submodule tests are broken.
>>> One problem is here:
>>>
>>> lib-submodule-update.sh:264: possible problem: echo -n is not portable
>>> (please use printf): echo -n >sub1 &&
>>> lib-submodule-update.sh:507: possible problem: echo -n is not portable
>>> (please use printf): echo -n >sub1 &&
>>>
>>> You can remove the "empty" "echo -n" to create an empty file:
sub1 &&
>> Thanks for spotting and diagnosing this. Running "make lint" in the
>> test directory only feeds the tests to check-non-portable-shell.pl,
>> but not the *lib*.sh helper scripts, which made me miss this one.
>>
>> The following diff should fix it for you. Am I understanding you
>> correctly that you are experiencing other failures too? I see no
>> other incompatibilities when running ./check-non-portable-shell.pl
>> on all the shell scripts in the repo.
> The longer story is that I run the test suite once a week or so.
> Most often under Mac OS, sometimes cygwin or Linux.
> Whenever there is a breakage under Mac OS which I can not
> debug within some minutes, I run it under Linux to see if there
> is the same breakage.
Thanks, that really helps a lot and is more than I could ask for.
> The ./check-non-portable-shell.pl can sometimes give an indication
> why some test fail.
> You can run it from command line:
> ./check-non-portable-shell.pl *.sh
> and it will find the "echo -n" which I reported.
> On the longer run it could probably check all *.sh files,
> not only the ones under t/
That is exactly what I thought when I tried to find out why I missed
this. Will cook up a patch to at least check the test helpers too.
> I do not have the time to test the snipped patch below, but I can check pu
> when the next round of your patch is in and give you some more info.
That'd be great!
Junio, do you want me to resend 02/14 without the non-portable "echo -n"
or could you just squash the following diff in?
-8<
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 24c9fd7..3584755 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -304,7 +304,7 @@ test_submodule_switch () {
(
cd submodule_update &&
git branch -t add_sub1 origin/add_sub1 &&
- echo -n >sub1 &&
+ >sub1 &&
test_must_fail $command add_sub1 &&
test_superproject_content origin/no_submodule &&
test_must_be_empty sub1
@@ -547,7 +547,7 @@ test_submodule_forced_switch () {
(
cd submodule_update &&
git branch -t add_sub1 origin/add_sub1 &&
- echo -n >sub1 &&
+ >sub1 &&
$command add_sub1 &&
test_superproject_content origin/add_sub1 &&
test_dir_is_empty sub1
-- 2.0.1.458.gf680257.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 00/14] Add submodule test harness
On 07/02/2014 09:57 PM, Jens Lehmann wrote: Am 02.07.2014 16:54, schrieb Torsten Bögershausen: (Not sure if this is the right thread) (I haven't checked if this is fixed in your latest version) Yes, this is the right thread and no, it isn't fixed yet. On what I have on pu 7a0da7902cbbc9a876b90c9, Tue Jul 1 14:51:53 2014 -0700 Many submodule tests are broken. One problem is here: lib-submodule-update.sh:264: possible problem: echo -n is not portable (please use printf): echo -n >sub1 && lib-submodule-update.sh:507: possible problem: echo -n is not portable (please use printf): echo -n >sub1 && You can remove the "empty" "echo -n" to create an empty file: sub1 && Thanks for spotting and diagnosing this. Running "make lint" in the test directory only feeds the tests to check-non-portable-shell.pl, but not the *lib*.sh helper scripts, which made me miss this one. The following diff should fix it for you. Am I understanding you correctly that you are experiencing other failures too? I see no other incompatibilities when running ./check-non-portable-shell.pl on all the shell scripts in the repo. The longer story is that I run the test suite once a week or so. Most often under Mac OS, sometimes cygwin or Linux. Whenever there is a breakage under Mac OS which I can not debug within some minutes, I run it under Linux to see if there is the same breakage. The ./check-non-portable-shell.pl can sometimes give an indication why some test fail. You can run it from command line: ./check-non-portable-shell.pl *.sh and it will find the "echo -n" which I reported. On the longer run it could probably check all *.sh files, not only the ones under t/ I do not have the time to test the snipped patch below, but I can check pu when the next round of your patch is in and give you some more info. -- 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 00/14] Add submodule test harness
Am 02.07.2014 16:54, schrieb Torsten Bögershausen:
> (Not sure if this is the right thread)
> (I haven't checked if this is fixed in your latest version)
Yes, this is the right thread and no, it isn't fixed yet.
> On what I have on pu 7a0da7902cbbc9a876b90c9, Tue Jul 1 14:51:53 2014 -0700
>
> Many submodule tests are broken.
> One problem is here:
>
> lib-submodule-update.sh:264: possible problem: echo -n is not portable
> (please use printf): echo -n >sub1 &&
> lib-submodule-update.sh:507: possible problem: echo -n is not portable
> (please use printf): echo -n >sub1 &&
>
> You can remove the "empty" "echo -n" to create an empty file:
>> sub1 &&
Thanks for spotting and diagnosing this. Running "make lint" in the
test directory only feeds the tests to check-non-portable-shell.pl,
but not the *lib*.sh helper scripts, which made me miss this one.
The following diff should fix it for you. Am I understanding you
correctly that you are experiencing other failures too? I see no
other incompatibilities when running ./check-non-portable-shell.pl
on all the shell scripts in the repo.
-8<
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 24c9fd7..3584755 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -304,7 +304,7 @@ test_submodule_switch () {
(
cd submodule_update &&
git branch -t add_sub1 origin/add_sub1 &&
- echo -n >sub1 &&
+ >sub1 &&
test_must_fail $command add_sub1 &&
test_superproject_content origin/no_submodule &&
test_must_be_empty sub1
@@ -547,7 +547,7 @@ test_submodule_forced_switch () {
(
cd submodule_update &&
git branch -t add_sub1 origin/add_sub1 &&
- echo -n >sub1 &&
+ >sub1 &&
$command add_sub1 &&
test_superproject_content origin/add_sub1 &&
test_dir_is_empty sub1
--
2.0.1.458.gf680257.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 00/14] Add submodule test harness
(Not sure if this is the right thread) (I haven't checked if this is fixed in your latest version) On what I have on pu 7a0da7902cbbc9a876b90c9, Tue Jul 1 14:51:53 2014 -0700 Many submodule tests are broken. One problem is here: lib-submodule-update.sh:264: possible problem: echo -n is not portable (please use printf): echo -n >sub1 && lib-submodule-update.sh:507: possible problem: echo -n is not portable (please use printf): echo -n >sub1 && You can remove the "empty" "echo -n" to create an empty file: >sub1 && -- 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
