Re: new platform & S_IFGITLINK problem

Alan Hourihane [off-list ref] writes:

Just joined as I'm compiling git on a new platform and wondered if
someone can help solve a problem with the definition of S_IFGITLINK.

Yikes.  The current codebase pretty much assumes that IFREG is 0100000,
IFDIR 0040000, and IFLNK 0120000, and the bits read from "struct stat" and
the mode bits we store in the index and tree objects are interchangeable.

Yes, that assumption is a gross POSIX violation but we so far haven't seen
any platform that matters in real life that assigns the IFMT bits
differently.

We would need to:

 (1) rename S_IFGITLINK to GIT_S_IFMODULE;

 (2) define GIT_S_IF{REG,DIR,LNK} as 0100000, 0040000, and 0120000;

 (3) write MODE_SYSTEM_TO_GIT() macro to convert from S_IF{REG,DIR,LNK} we
     read from struct stat to the "canonical" GIT_S_IF{REG,DIR,LNK}
     values; and

 (4) change all the code that read mode from struct stat and use it to
     first use MODE_SYSTEM_TO_GIT().

Currently 'git grep -e "S_IF[A-Z]" -e "struct stat"' reports around 250
hits, so it is not infeasible amount of work, but it is not a trivial and
mechanical replacement, either.  I or somebody need to set aside a block
of time to do this clean-up and audit the result.

Thanks for a report.

On Sat, 2010-05-01 at 19:33 -0700, Junio C Hamano wrote:

Alan Hourihane [off-list ref] writes:

quoted

Just joined as I'm compiling git on a new platform and wondered if
someone can help solve a problem with the definition of S_IFGITLINK.

Yikes.  The current codebase pretty much assumes that IFREG is 0100000,
IFDIR 0040000, and IFLNK 0120000, and the bits read from "struct stat" and
the mode bits we store in the index and tree objects are interchangeable.

Yes, that assumption is a gross POSIX violation but we so far haven't seen
any platform that matters in real life that assigns the IFMT bits
differently.

We would need to:

 (1) rename S_IFGITLINK to GIT_S_IFMODULE;

 (2) define GIT_S_IF{REG,DIR,LNK} as 0100000, 0040000, and 0120000;

 (3) write MODE_SYSTEM_TO_GIT() macro to convert from S_IF{REG,DIR,LNK} we
     read from struct stat to the "canonical" GIT_S_IF{REG,DIR,LNK}
     values; and

 (4) change all the code that read mode from struct stat and use it to
     first use MODE_SYSTEM_TO_GIT().

Currently 'git grep -e "S_IF[A-Z]" -e "struct stat"' reports around 250
hits, so it is not infeasible amount of work, but it is not a trivial and
mechanical replacement, either.  I or somebody need to set aside a block
of time to do this clean-up and audit the result.

Thanks for a report.

No problem. Thanks for taking the challenge and sorting it out.

Let me know if there's anywhere I can help, test, or debug.

Thanks again!

Alan.

On Sat, 1 May 2010, Junio C Hamano wrote:

Yikes.  The current codebase pretty much assumes that IFREG is 0100000,
IFDIR 0040000, and IFLNK 0120000, and the bits read from "struct stat" and
the mode bits we store in the index and tree objects are interchangeable.

Yeah. This is painful. I knew it was wrong to just assume everything was 
sane and used the same traditional values, but I optimistically thought 
that the st_mode bits were the _one_ thing that everybody agrees on.

I was wrong. 

Yes, that assumption is a gross POSIX violation but we so far haven't seen
any platform that matters in real life that assigns the IFMT bits
differently.

Indeed. Just out of interest - Alan, what _is_ the crazy platform that 
doesn't match what absolutely everybody else has always done?

We would need to:

 (1) rename S_IFGITLINK to GIT_S_IFMODULE;

I'd suggest dropping the "_S_" part, and just calling it

	GIT_IFxyz
	GIT_ISxyz()

 (2) define GIT_S_IF{REG,DIR,LNK} as 0100000, 0040000, and 0120000;

 (3) write MODE_SYSTEM_TO_GIT() macro to convert from S_IF{REG,DIR,LNK} we
     read from struct stat to the "canonical" GIT_S_IF{REG,DIR,LNK}
     values; and

 (4) change all the code that read mode from struct stat and use it to
     first use MODE_SYSTEM_TO_GIT().

Currently 'git grep -e "S_IF[A-Z]" -e "struct stat"' reports around 250
hits, so it is not infeasible amount of work, but it is not a trivial and
mechanical replacement, either.  I or somebody need to set aside a block
of time to do this clean-up and audit the result.

Ugh. And since nobody sane has different values from the system ones, if 
we miss some case we'll never notice on any sane platform ;(

		Linus

On Mon, 3 May 2010, Linus Torvalds wrote:

On Sat, 1 May 2010, Junio C Hamano wrote:

quoted

 (3) write MODE_SYSTEM_TO_GIT() macro to convert from S_IF{REG,DIR,LNK} we
     read from struct stat to the "canonical" GIT_S_IF{REG,DIR,LNK}
     values; and

 (4) change all the code that read mode from struct stat and use it to
     first use MODE_SYSTEM_TO_GIT().

Currently 'git grep -e "S_IF[A-Z]" -e "struct stat"' reports around 250
hits, so it is not infeasible amount of work, but it is not a trivial and
mechanical replacement, either.  I or somebody need to set aside a block
of time to do this clean-up and audit the result.

Ugh. And since nobody sane has different values from the system ones, if 
we miss some case we'll never notice on any sane platform ;(

As to your (3) and (4), I actually think that the best option would be to 
stop using '[lf]stat()' directly for "working tree stat", and instead just 
introduce a 'git_[lf]stat()' function that just always does the conversion 
(when necessary - the "conversion" can be a no-op on sane architectures) 
for us.

Right now, a _lot_ of the functions work either directly on 'st->st_mode' 
_or_ on random index entries, and that would be a major pain if they might 
ever be in "different number domains". So the easiest way to make sure 
that we _always_ use the GIT_IFxyz numbers would be to never ever have 
anything that uses the native ones even by mistake.

I've actually wanted to have a 'git_lstat()' wrapper for other reasons: it 
would have made it _so_ much easier to do breakpoints etc when doing the 
whole name lookup optimizations. 

Note: there are various cases of '[lf]stat()' being used not for working 
tree entries, but for things like the pack files etc internal git files. 
Those should _not_ do the conversion - they should use the "native" stat() 
functions. It's only the working tree accesses we should need to do any 
conversion on, since those are the ones that are relevant for the index 
(and tree) comparisons.

			Linus

On 05/04/2010 05:52 AM, Linus Torvalds wrote:

On Mon, 3 May 2010, Linus Torvalds wrote:

quoted

On Sat, 1 May 2010, Junio C Hamano wrote:

quoted

  (3) write MODE_SYSTEM_TO_GIT() macro to convert from S_IF{REG,DIR,LNK} we
      read from struct stat to the "canonical" GIT_S_IF{REG,DIR,LNK}
      values; and

  (4) change all the code that read mode from struct stat and use it to
      first use MODE_SYSTEM_TO_GIT().

Currently 'git grep -e "S_IF[A-Z]" -e "struct stat"' reports around 250
hits, so it is not infeasible amount of work, but it is not a trivial and
mechanical replacement, either.  I or somebody need to set aside a block
of time to do this clean-up and audit the result.

Ugh. And since nobody sane has different values from the system ones, if
we miss some case we'll never notice on any sane platform ;(

As to your (3) and (4), I actually think that the best option would be to
stop using '[lf]stat()' directly for "working tree stat", and instead just
introduce a 'git_[lf]stat()' function that just always does the conversion
(when necessary - the "conversion" can be a no-op on sane architectures)
for us.

Right now, a _lot_ of the functions work either directly on 'st->st_mode'
_or_ on random index entries, and that would be a major pain if they might
ever be in "different number domains". So the easiest way to make sure
that we _always_ use the GIT_IFxyz numbers would be to never ever have
anything that uses the native ones even by mistake.

I've actually wanted to have a 'git_lstat()' wrapper for other reasons: it
would have made it _so_ much easier to do breakpoints etc when doing the
whole name lookup optimizations.

Note: there are various cases of '[lf]stat()' being used not for working
tree entries, but for things like the pack files etc internal git files.
Those should _not_ do the conversion - they should use the "native" stat()
functions. It's only the working tree accesses we should need to do any

3 more quoted lines

There are other benefits to the git_[fl]stat as well. Windows people would
probably be delighted if they were introduced. We have them in libgit2 for
that precise reason, since stat() sucks arse but some fileGetInfo() call is
at least reasonably fast on windows.

So why not make even pack file access work with the git macros? After all,
the conversion is a no-op on sane platforms, and it would probably be
easier to review patches if one doesn't have to remember that we have two
stat() implementations and which one to use where. Especially when we add
utility functions like read_file_chunk() (no, it doesn't exist) that could
operate on either internal files or work-tree files.

It would also make code-stealing to libgit2 a bit easier ;)

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

On Mon, 2010-05-03 at 20:39 -0700, Linus Torvalds wrote:

On Sat, 1 May 2010, Junio C Hamano wrote:

quoted

Yes, that assumption is a gross POSIX violation but we so far haven't seen
any platform that matters in real life that assigns the IFMT bits
differently.

Indeed. Just out of interest - Alan, what _is_ the crazy platform that 
doesn't match what absolutely everybody else has always done?

You don't want to know :-)

But if you really must, it's an Atari platforms running FreeMiNT, but
was also essentially Atari's own MultiTOS as well which they adopted
from the original MiNT project.

Alan.

On Tue, 4 May 2010, Andreas Ericsson wrote:

So why not make even pack file access work with the git macros?

Because I'm a sneaky bastard, and I have a long-term plan.

The whole filename character set conversion thing.

So having a git_[l]stat() macro - used only for working tree accesses - 
would be one step in that direction. Doing it for git internal files would 
totally screw that plan up.

		Linus

On Sat, 2010-05-01 at 19:33 -0700, Junio C Hamano wrote:

Alan Hourihane [off-list ref] writes:

quoted

Just joined as I'm compiling git on a new platform and wondered if
someone can help solve a problem with the definition of S_IFGITLINK.

Yikes.  The current codebase pretty much assumes that IFREG is 0100000,
IFDIR 0040000, and IFLNK 0120000, and the bits read from "struct stat" and
the mode bits we store in the index and tree objects are interchangeable.

Yes, that assumption is a gross POSIX violation but we so far haven't seen
any platform that matters in real life that assigns the IFMT bits
differently.

We would need to:

 (1) rename S_IFGITLINK to GIT_S_IFMODULE;

 (2) define GIT_S_IF{REG,DIR,LNK} as 0100000, 0040000, and 0120000;

 (3) write MODE_SYSTEM_TO_GIT() macro to convert from S_IF{REG,DIR,LNK} we
     read from struct stat to the "canonical" GIT_S_IF{REG,DIR,LNK}
     values; and

 (4) change all the code that read mode from struct stat and use it to
     first use MODE_SYSTEM_TO_GIT().

Currently 'git grep -e "S_IF[A-Z]" -e "struct stat"' reports around 250
hits, so it is not infeasible amount of work, but it is not a trivial and
mechanical replacement, either.  I or somebody need to set aside a block
of time to do this clean-up and audit the result.

Thanks for a report.

Hi Junio,

Is there any ETA on fixing this up ?

Thanks,

Alan.