threads / patch / 28455

patchWindows: define S_ISUID properly

Subject: [PATCH di/fast-import-deltified-tree] Windows: define S_ISUID properly

## tl;dr

6 messages between Sep 21, 2011 and Sep 21, 2011. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Johannes Sixt· Sep 21, 2011, 06:33 UTC · lore

From: Johannes Sixt <j6t@kdbg.org>

8fb3ad76 (fast-import: prevent producing bad delta) introduced the first use of S_ISUID. Since before this commit the value was irrelevant, we had only a dummy definition in mingw.h. But beginning with this commit the macro must expand to a reasonable value. Make it so.

We do not change S_ISGID from the value 0 because it is used in path.c (via FORCE_DIR_SET_GID) to set the mode on directories in a manner that is not supported on Windows, and 0 is the right value in this case.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 compat/mingw.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Show changes to compat/mingw.h +1 −1

diff --git a/compat/mingw.h b/compat/mingw.h
index 547568b..e2c89d6 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -22,7 +22,7 @@ typedef int socklen_t;
 #define S_IWOTH 0
 #define S_IXOTH 0
 #define S_IRWXO (S_IROTH | S_IWOTH | S_IXOTH)
-#define S_ISUID 0
+#define S_ISUID 04000
 #define S_ISGID 0
 #define S_ISVTX 0

-- 
1.7.7.rc2.1135.gf321f

Dmitry Ivankov· Sep 21, 2011, 07:38 UTC · re: Johannes Sixt · lore

Re: [PATCH di/fast-import-deltified-tree] Windows: define S_ISUID properly

Johannes Sixt <j.sixt <at> viscovery.net> writes:

Show 30 quoted lines

> 
> From: Johannes Sixt <j6t <at> kdbg.org>
> 
> 8fb3ad76 (fast-import: prevent producing bad delta) introduced the first
> use of S_ISUID. Since before this commit the value was irrelevant, we had
> only a dummy definition in mingw.h. But beginning with this commit the
> macro must expand to a reasonable value. Make it so.
> 
> We do not change S_ISGID from the value 0 because it is used in path.c
> (via FORCE_DIR_SET_GID) to set the mode on directories in a manner that
> is not supported on Windows, and 0 is the right value in this case.
> 
> Signed-off-by: Johannes Sixt <j6t <at> kdbg.org>
> ---
>  compat/mingw.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 547568b..e2c89d6 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -22,7 +22,7 @@ typedef int socklen_t;
>  #define S_IWOTH 0
>  #define S_IXOTH 0
>  #define S_IRWXO (S_IROTH | S_IWOTH | S_IXOTH)
> -#define S_ISUID 0
> +#define S_ISUID 04000
>  #define S_ISGID 0
>  #define S_ISVTX 0
>

Ow, it's awkward that the issue was discussed in [1] but slipped and nobody noticed, especially me being a patch sender.

If we choose patch from [1] I'd also change a comment to smth like
/* 
 * We abuse the 04000 bit on directories to mean "do not delta".
 * It is a S_ISUID bit on setuid platforms and an unused bit on
 * non-setuid platforms supported in git. In either case git ignores
 * the bit, so it's safe to abuse it locally.
 */

[1] http://thread.gmane.org/gmane.comp.version-control.git/179223/focus=179762

Junio C Hamano· Sep 21, 2011, 12:07 UTC · re: Dmitry Ivankov · lore

Re: [PATCH di/fast-import-deltified-tree] Windows: define S_ISUID properly

Dmitry Ivankov <divanorama@gmail.com> writes:

Show 22 quoted lines

> Johannes Sixt <j.sixt <at> viscovery.net> writes:
>> 
>> From: Johannes Sixt <j6t <at> kdbg.org>
>> 
>> 8fb3ad76 (fast-import: prevent producing bad delta) introduced the first
>> use of S_ISUID. Since before this commit the value was irrelevant, we had
>> only a dummy definition in mingw.h. But beginning with this commit the
>> macro must expand to a reasonable value. Make it so.
>>  #define S_ISVTX 0
>> ...
> Ow, it's awkward that the issue was discussed in [1] but slipped and nobody 
> noticed, especially me being a patch sender.
>
> If we choose patch from [1] I'd also change a comment to smth like
> /* 
>  * We abuse the 04000 bit on directories to mean "do not delta".
>  * It is a S_ISUID bit on setuid platforms and an unused bit on
>  * non-setuid platforms supported in git. In either case git ignores
>  * the bit, so it's safe to abuse it locally.
>  */
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/179223/focus=179762

I think that the fix from Jonathan to stop abusing S_ISUID is much more preferrable; the Windows platform shouldn't have to worry about this.

And it would be even better to use a value that does not overlap with the usual bits for do-not-delta bit if possible.

Junio C Hamano· Sep 21, 2011, 12:08 UTC · re: Dmitry Ivankov · lore

Re: [PATCH di/fast-import-deltified-tree] Windows: define S_ISUID properly

Dmitry Ivankov <divanorama@gmail.com> writes:

Show 22 quoted lines

> Johannes Sixt <j.sixt <at> viscovery.net> writes:
>> 
>> From: Johannes Sixt <j6t <at> kdbg.org>
>> 
>> 8fb3ad76 (fast-import: prevent producing bad delta) introduced the first
>> use of S_ISUID. Since before this commit the value was irrelevant, we had
>> only a dummy definition in mingw.h. But beginning with this commit the
>> macro must expand to a reasonable value. Make it so.
>>  #define S_ISVTX 0
>> ...
> Ow, it's awkward that the issue was discussed in [1] but slipped and nobody 
> noticed, especially me being a patch sender.
>
> If we choose patch from [1] I'd also change a comment to smth like
> /* 
>  * We abuse the 04000 bit on directories to mean "do not delta".
>  * It is a S_ISUID bit on setuid platforms and an unused bit on
>  * non-setuid platforms supported in git. In either case git ignores
>  * the bit, so it's safe to abuse it locally.
>  */
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/179223/focus=179762

I think that the fix from Jonathan to stop abusing S_ISUID is much more preferrable; the Windows platform shouldn't have to worry about this.

And it would be even better to use a value that does not overlap with the usual bits for do-not-delta bit if possible.

Dmitry Ivankov· Sep 21, 2011, 13:44 UTC · re: Junio C Hamano · lore

Re: [PATCH di/fast-import-deltified-tree] Windows: define S_ISUID properly

On Wed, Sep 21, 2011 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 30 quoted lines

> Dmitry Ivankov <divanorama@gmail.com> writes:
>
>> Johannes Sixt <j.sixt <at> viscovery.net> writes:
>>>
>>> From: Johannes Sixt <j6t <at> kdbg.org>
>>>
>>> 8fb3ad76 (fast-import: prevent producing bad delta) introduced the first
>>> use of S_ISUID. Since before this commit the value was irrelevant, we had
>>> only a dummy definition in mingw.h. But beginning with this commit the
>>> macro must expand to a reasonable value. Make it so.
>>>  #define S_ISVTX 0
>>> ...
>> Ow, it's awkward that the issue was discussed in [1] but slipped and nobody
>> noticed, especially me being a patch sender.
>>
>> If we choose patch from [1] I'd also change a comment to smth like
>> /*
>>  * We abuse the 04000 bit on directories to mean "do not delta".
>>  * It is a S_ISUID bit on setuid platforms and an unused bit on
>>  * non-setuid platforms supported in git. In either case git ignores
>>  * the bit, so it's safe to abuse it locally.
>>  */
>>
>> [1] http://thread.gmane.org/gmane.comp.version-control.git/179223/focus=179762
>
> I think that the fix from Jonathan to stop abusing S_ISUID is much more
> preferrable; the Windows platform shouldn't have to worry about this.
>
> And it would be even better to use a value that does not overlap with the
> usual bits for do-not-delta bit if possible.

Depends on what is a usual bit. I'll use linux defines for mode bits.

There are S_ISVTX, S_ISUID completely unused in git, S_ISGID is used somehow. 9 lower rwx bits are used as well as S_IFREG and S_IFDIR. Remaining are S_IFIFO (used somehow), S_IFCHR (part of GITLNK).

S_ISUID in fast-import input stream isn't accepted, so the only danger is it may come from a tree object (but not yet, git-fsck doesn't allow it). Or if there appears a platform with different S_I{F,S}* definitions, which will break more git parts than just fast-import.

I remember there was a thread concerning platform vs git-core mode bits. I'd just use hard-coded 04000 bit in fast-import as a hot-fix and leave the rest for the bits topic. With S_ISUID in a comment near 04000 it'll be a grep-able hard-coded constant, so it should be ok.

Junio C Hamano· Sep 21, 2011, 15:13 UTC · re: Dmitry Ivankov · lore

Re: [PATCH di/fast-import-deltified-tree] Windows: define S_ISUID properly

Dmitry Ivankov <divanorama@gmail.com> writes:

>> And it would be even better to use a value that does not overlap with the
>> usual bits for do-not-delta bit if possible.
>
> Depends on what is a usual bit. I'll use linux defines for mode bits.

Failed -- was too subtle to extract a hoped-for response "come to think of it we should have done it as a separate field in the structure".