Tree with leading '0' modes in 1.7.0.3

> Shawn O. Pearce wrote:
> 
> > Any ideas?  Why is Git 1.7.0.3 jamming a leading '0' on a file mode?
> 
> See http://thread.gmane.org/gmane.comp.version-control.git/141028
> and commit c88f0cc (notes: fix malformed tree entry, 2010-02-24).
> 
> The regression that that fixes appeared in 61a7cca0 (Notes API:
> write_notes_tree(): Store the notes tree in the database, 2010-02-13),
> which is not part of 1.7.0.3.

>>> Any ideas?  Why is Git 1.7.0.3 jamming a leading '0' on a file mode?
>> 
>> See http://thread.gmane.org/gmane.comp.version-control.git/141028
>> and commit c88f0cc (notes: fix malformed tree entry, 2010-02-24).
>> 
>> The regression that that fixes appeared in 61a7cca0 (Notes API:
>> write_notes_tree(): Store the notes tree in the database, 2010-02-13),
>> which is not part of 1.7.0.3.
>
> That may be true... but I doubt the tree in question was a notes
> tree.  The path entries were names like 'README', 'modules' and
> 'stewardbot'.  Something I would assume was the project's source
> tree, not its notes tree.

> Shawn O. Pearce wrote:
>> Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> Shawn O. Pearce wrote:
>
>>>> Any ideas?  Why is Git 1.7.0.3 jamming a leading '0' on a file mode?
>>> 
>>> See http://thread.gmane.org/gmane.comp.version-control.git/141028
>>> and commit c88f0cc (notes: fix malformed tree entry, 2010-02-24).
>>> 
>>> The regression that that fixes appeared in 61a7cca0 (Notes API:
>>> write_notes_tree(): Store the notes tree in the database, 2010-02-13),
>>> which is not part of 1.7.0.3.
>>
>> That may be true... but I doubt the tree in question was a notes
>> tree.  The path entries were names like 'README', 'modules' and
>> 'stewardbot'.  Something I would assume was the project's source
>> tree, not its notes tree.
>
> Yes, true.  The problem is probably elsewhere, especially because
> 1.7.0.3 doesn’t even have that commit.  Still, I find this a bit
> strange because such breakage should have been noticeable if it
> happens often.
>
> What has changed recently that involves writing trees?

> Apparently Scott Chacon has some clue about this error:
> http://support.github.com/discussions/repos/2566-strange-warning-from-fsck-and-github-repo-using-too-much-diskspace
> so I've added him to CC. (Note that changing all SHA1s is not really a
> problem for us, there are only 3 copies of the repo, and the project has
> only been using version control for 2 days)

> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
>> Scott, please fix that library on GitHub.  JGit's fsck has a hard
>> failure on these malformed trees, because the leading '0' mode
>> causes the tree to come up with the wrong SHA-1 hash given its
>> logical content.  They shouldn't be created like this.
>
> What is curious is that even though 6407180 (git-fsck-cache: be stricter
> about "tree" objects, 2005-07-27) does talk about zero-padding, it appears
> that we never had a version of git that padded mode in '0' in the entire
> history of write-tree (except that "notes tree" one, but even that didn't
> escape the laboratory).

> git fast-export --all |
> (
> 	cd /empty/repository &&
> 	git init &&
> 	git fast-import
> )

> On 10-03-26 08:56 PM, Avery Pennarun wrote:
> > The problem is that everything in git works perfectly with these
> > invalid file modes *except* fsck, and there's rarely a need to run
> > fsck, so this problem can hide for a long time.
> 
> So, does the error matter or not? If it doesn't matter, then shouldn't
> Jgit stop whining? If it does, then whatever-it-is needs to be fixed.

> "Mike.lifeguard" <mike.lifeguard@gmail.com> wrote:
> > On 10-03-26 08:56 PM, Avery Pennarun wrote:
> > > The problem is that everything in git works perfectly with these
> > > invalid file modes *except* fsck, and there's rarely a need to run
> > > fsck, so this problem can hide for a long time.
> > 
> > So, does the error matter or not? If it doesn't matter, then shouldn't
> > Jgit stop whining? If it does, then whatever-it-is needs to be fixed.
> 
> Its less harmful than other types of corruption.  But its quite
> wrong from a format perspective. The hash of the tree differs even
> though there is no semantic difference in the tree content.
> 
> Given that GitHub has blessed the world with this corruption,
> we may need to modify JGit to accept it.

> On Fri, 26 Mar 2010, Shawn O. Pearce wrote:
> > "Mike.lifeguard" <mike.lifeguard@gmail.com> wrote:
> > > On 10-03-26 08:56 PM, Avery Pennarun wrote:
> > > > The problem is that everything in git works perfectly with these
> > > > invalid file modes *except* fsck, and there's rarely a need to run
> > > > fsck, so this problem can hide for a long time.
> > > 
> > > So, does the error matter or not? If it doesn't matter, then shouldn't
> > > Jgit stop whining? If it does, then whatever-it-is needs to be fixed.
> > 
> > Its less harmful than other types of corruption.  But its quite
> > wrong from a format perspective. The hash of the tree differs even
> > though there is no semantic difference in the tree content.
> > 
> > Given that GitHub has blessed the world with this corruption,
> > we may need to modify JGit to accept it.
> 
> Should we?
> 
> This is going to screw up pack v4 (yes, someday I'll have the time to 
> make it real).

> Nicolas Pitre <nico@fluxnic.net> wrote:
> > On Fri, 26 Mar 2010, Shawn O. Pearce wrote:
> > > Given that GitHub has blessed the world with this corruption,
> > > we may need to modify JGit to accept it.
> > 
> > Should we?
> > 
> > This is going to screw up pack v4 (yes, someday I'll have the time to 
> > make it real).
> 
> Exactly.  I *really* don't want to permit this sort of corruption
> in a Git repository.
> 
> But GitHub's approach here seems to be "Meh, its fine, don't worry
> about it".

> Though, FWIW, it might not screw up pack v4.  IIRC from our
> discussions long ago on pack v4, we store "$mode $name" pairs in
> an indexed list, preciously because we needed to support odd modes
> like 10664 from ancient Git binaries.  If we continue to allow this
> corruption, it means we have to ensure $mode is the octal string
> and not the binary value.

> It's up to GitHub to fork Git then, and while at it stop calling it Git
> compatible.  Really.  If we start to get slack about the pack format
> like this then every Git reimplementation du jour will make similar
> deviations except in different directions and we'll end up with a mess
> to support.

> Hey,
> 
> Sorry it's taken me a bit - I'm traveling right now.
> 
> On Fri, Mar 26, 2010 at 6:56 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> >> > > Given that GitHub has blessed the world with this corruption,
> >> > > we may need to modify JGit to accept it.
> 
> Well, shouldn't it accept it just because CGit accepts it?  Isn't that
> an incompatibility in implementation?

> >> But GitHub's approach here seems to be "Meh, its fine, don't worry
> >> about it".
> 
> That isn't really my approach, I actually thought I had fixed this a
> while ago.  It seems to be a pretty understandable mistake, since
> ls-tree and cat-file -p both output zero padded modes and it is only
> an issue on trees with subtrees, obviously, so we don't see it all the
> time at GitHub.  I have fixed this and it's in the queue for
> deployment which should be in the next few days (I gotta get home
> first).

> > It's up to GitHub to fork Git then, and while at it stop calling it Git
> > compatible.  Really.  If we start to get slack about the pack format
> > like this then every Git reimplementation du jour will make similar
> > deviations except in different directions and we'll end up with a mess
> > to support.
> 
> Really?  It's not the pack format - we use stock Git servers and
> almost always have.  It's the tree writing when someone edits a file
> inline - I was writing out zero-padded trees. And, it _is_ Git
> compatible - CGit only issues a warning, and that only if the
> circumstances align such that we write a tree with a subtree, which
> again is pretty rare.  There are only a handful of projects like this
> and in all CGit circumstances makes no practical difference.

> > My stance has always been that the C Git is authoritative with regards to
> > formats and protocols.  It's up to Github to fix their screw-up.
> 
> It is fixed and will be deployed soon, but really, there is no reason
> to be snippy.  It is a simple and minor mistake effecting very few
> repositories (maybe 100 out of 730k), and the only reason it's an
> issue at all is that JGit is not following the authoritative CGit
> implementation of basically ignoring it.

> Also, if we're all concerned about "Git reimplementation du jour"
> deviations, then we need to focus on libifying Git so there isn't a
> need for such re-implementations.  I'm hoping to help with a possible
> GSoC project on libgit2, but the lack of a linkable library will
> ensure that re-implementations in nearly every useful language will
> continue.

> On Sat, 27 Mar 2010, Scott Chacon wrote:
> > > My stance has always been that the C Git is authoritative with regards to
> > > formats and protocols. ??It's up to Github to fix their screw-up.
> > 
> > It is fixed and will be deployed soon, but really, there is no reason
> > to be snippy.  It is a simple and minor mistake effecting very few
> > repositories (maybe 100 out of 730k)

> > , and the only reason it's an
> > issue at all is that JGit is not following the authoritative CGit
> > implementation of basically ignoring it.
> 
> But again CGit's fsck is not ignoring this discrepancy.  And if the CGit 
> core is otherwise silently accepting it then it is a mistake.

> > Also, if we're all concerned about "Git reimplementation du jour"
> > deviations, then we need to focus on libifying Git so there isn't a
> > need for such re-implementations.  I'm hoping to help with a possible
> > GSoC project on libgit2, but the lack of a linkable library will
> > ensure that re-implementations in nearly every useful language will
> > continue.
> 
> Don't get me wrong.  I'm not against Git reimplementations per se, as 
> long as they rigorously implement the exact format and protocol from 
> CGit.  In that sense it is important that the CGit fsck and verify-pack 
> tools be exploited on objects/packs produced by alternate Git 
> implementation systematically to find such issues.

> Nicolas Pitre <nico@fluxnic.net> wrote:
>> On Sat, 27 Mar 2010, Scott Chacon wrote:
>>>> My stance has always been that the C Git is authoritative with regards to
>>>> formats and protocols. ??It's up to Github to fix their screw-up.
>>> It is fixed and will be deployed soon, but really, there is no reason
>>> to be snippy.  It is a simple and minor mistake effecting very few
>>> repositories (maybe 100 out of 730k)
> 
> What is the C Git stance on these 100 repositories then?  Are they
> now considered corrupt?  Or is 100 enough in the wild that we have
> to accept the problem, just like we accept the 10664 mode issue from
> "ancient" Linux?
> 
> I would love to say "those are corrupt, sorry, fix your repository".
> 
> But we have traditionally tried to help our users, and not cause
> them pain.  Forcing a rewrite on these 100 projects to fix up the
> corruption is going to be painful for them.  
> 
>>> , and the only reason it's an
>>> issue at all is that JGit is not following the authoritative CGit
>>> implementation of basically ignoring it.
>> But again CGit's fsck is not ignoring this discrepancy.  And if the CGit 
>> core is otherwise silently accepting it then it is a mistake.
> 
> Right.  I tend to agree.  CGit was too lax here, fsck shouldn't
> be issuing a warning, it should be a fatal error.  Both CGit and
> JGit are too lax by not failing when reading that tree during
> normal processing.
>  
>>> Also, if we're all concerned about "Git reimplementation du jour"
>>> deviations, then we need to focus on libifying Git so there isn't a
>>> need for such re-implementations.  I'm hoping to help with a possible
>>> GSoC project on libgit2, but the lack of a linkable library will
>>> ensure that re-implementations in nearly every useful language will
>>> continue.
>> Don't get me wrong.  I'm not against Git reimplementations per se, as 
>> long as they rigorously implement the exact format and protocol from 
>> CGit.  In that sense it is important that the CGit fsck and verify-pack 
>> tools be exploited on objects/packs produced by alternate Git 
>> implementation systematically to find such issues.
> 
> When JGit had the tree sort order wrong, JGit was in the wrong,
> and any repository which contained those corrupt trees had to be
> fixed by rewriting them.  IIRC it was only the JGit repository
> itself that had this problem in the wild.  But we fixed our code.
> 
> IMHO, this leading '0' thing is a similar breakage.  We shouldn't
> relax CGit or JGit to accept it just because the Ruby implementation
> of Git got the tree encoding wrong.  If anything, we should teach
> these implementations to catch these sorts of problems earlier.
> 

> Shawn O. Pearce wrote:
>>
>> IMHO, this leading '0' thing is a similar breakage.  We shouldn't
>> relax CGit or JGit to accept it just because the Ruby implementation
>> of Git got the tree encoding wrong.  If anything, we should teach
>> these implementations to catch these sorts of problems earlier.
>
> Just add an additional data point, it looks like up to 16 of these trees  
> with zero-padded file modes are reachable from Linus' kernel master ref.

> A Large Angry SCM <gitzilla@gmail.com> wrote:
>> Shawn O. Pearce wrote:
>>> IMHO, this leading '0' thing is a similar breakage.  We shouldn't
>>> relax CGit or JGit to accept it just because the Ruby implementation
>>> of Git got the tree encoding wrong.  If anything, we should teach
>>> these implementations to catch these sorts of problems earlier.
>> Just add an additional data point, it looks like up to 16 of these trees  
>> with zero-padded file modes are reachable from Linus' kernel master ref.
> 
> Frell.
> 
> We can't ask Linus to rewrite his history to repair this breakage.
> The fact that its made it into the kernel history means we have
> to accept this.  The kernel project is simply too large and move
> too fast for us to ask them to fix their repository history.
> Smaller projects of 1-2 people, we could have gotten away with
> asking them to fix their history.
> 
> I guess that answers the questions then.  CGit permits this with
> a warning, and must always continue to do that.  And JGit needs to
> fix itself to do the same.
> 

> Shawn O. Pearce wrote:
>> A Large Angry SCM <gitzilla@gmail.com> wrote:
>>> Shawn O. Pearce wrote:
>>>> IMHO, this leading '0' thing is a similar breakage.  We shouldn't
>>>> relax CGit or JGit to accept it just because the Ruby implementation
>>>> of Git got the tree encoding wrong.  If anything, we should teach
>>>> these implementations to catch these sorts of problems earlier.
>>> Just add an additional data point, it looks like up to 16 of these 
>>> trees  with zero-padded file modes are reachable from Linus' kernel 
>>> master ref.
>>
>> Frell.
>>
>> We can't ask Linus to rewrite his history to repair this breakage.
>> The fact that its made it into the kernel history means we have
>> to accept this.  The kernel project is simply too large and move
>> too fast for us to ask them to fix their repository history.
>> Smaller projects of 1-2 people, we could have gotten away with
>> asking them to fix their history.
>>
>> I guess that answers the questions then.  CGit permits this with
>> a warning, and must always continue to do that.  And JGit needs to
>> fix itself to do the same.
>>
> 
> Wait a minute, something strange is going on here.
> 
> My combined kernel repository has 16 of these things according to 
> git-fsck. And when I do 'git-fsck torvalds/linux-2.6/master' I get the 
> same 16 BUT when I 'git-rev-list --objects torvalds/linux-2.6/master' 
> they do not appear in the output.
> 

> A Large Angry SCM wrote:
>> Shawn O. Pearce wrote:
>>> A Large Angry SCM <gitzilla@gmail.com> wrote:
>>>> Shawn O. Pearce wrote:
>>>>> IMHO, this leading '0' thing is a similar breakage.  We shouldn't
>>>>> relax CGit or JGit to accept it just because the Ruby implementation
>>>>> of Git got the tree encoding wrong.  If anything, we should teach
>>>>> these implementations to catch these sorts of problems earlier.
>>>> Just add an additional data point, it looks like up to 16 of these 
>>>> trees  with zero-padded file modes are reachable from Linus' kernel 
>>>> master ref.
>>>
>>> Frell.
>>>
>>> We can't ask Linus to rewrite his history to repair this breakage.
>>> The fact that its made it into the kernel history means we have
>>> to accept this.  The kernel project is simply too large and move
>>> too fast for us to ask them to fix their repository history.
>>> Smaller projects of 1-2 people, we could have gotten away with
>>> asking them to fix their history.
>>>
>>> I guess that answers the questions then.  CGit permits this with
>>> a warning, and must always continue to do that.  And JGit needs to
>>> fix itself to do the same.
>>>
>>
>> Wait a minute, something strange is going on here.
>>
>> My combined kernel repository has 16 of these things according to 
>> git-fsck. And when I do 'git-fsck torvalds/linux-2.6/master' I get the 
>> same 16 BUT when I 'git-rev-list --objects torvalds/linux-2.6/master' 
>> they do not appear in the output.
>>
> 
> Ignore my noise until some more checking is done!
> 
> My combined repository also includes Scott's progit book and examples 
> repositories. I'm guessing that git-fsck did not limit itself to just 
> the object reachable from the torvalds/linux-2.6/master ref.
> 

> On Sun, Mar 28, 2010 at 1:14 AM, A Large Angry SCM <gitzilla@gmail.com> wrote:
> 
>> My combined repository also includes Scott's progit book and examples
>> repositories. I'm guessing that git-fsck did not limit itself to just the
>> object reachable from the torvalds/linux-2.6/master ref.
> 
> <struck speechless>
> 
> You have *one* repo containing both Scott's book and the Linux kernel
> tree?  "large angry SCM" is probably an understatement then... any SCM
> has the right to be angry mixing up things that are so unrelated!
> 
> Or did I totally, *totally* misunderstand...?
> 

> Nicolas Pitre <nico@fluxnic.net> wrote:
>> On Sat, 27 Mar 2010, Scott Chacon wrote:
>>>> My stance has always been that the C Git is authoritative with regards to
>>>> formats and protocols. ??It's up to Github to fix their screw-up.
>>> It is fixed and will be deployed soon, but really, there is no reason
>>> to be snippy.  It is a simple and minor mistake effecting very few
>>> repositories (maybe 100 out of 730k)
> 
> What is the C Git stance on these 100 repositories then?  Are they
> now considered corrupt?  Or is 100 enough in the wild that we have
> to accept the problem, just like we accept the 10664 mode issue from
> "ancient" Linux?
> 
> I would love to say "those are corrupt, sorry, fix your repository".
> 
> But we have traditionally tried to help our users, and not cause
> them pain.  Forcing a rewrite on these 100 projects to fix up the
> corruption is going to be painful for them.  
> 
>>> , and the only reason it's an
>>> issue at all is that JGit is not following the authoritative CGit
>>> implementation of basically ignoring it.
>> But again CGit's fsck is not ignoring this discrepancy.  And if the CGit 
>> core is otherwise silently accepting it then it is a mistake.
> 
> Right.  I tend to agree.  CGit was too lax here, fsck shouldn't
> be issuing a warning, it should be a fatal error.  Both CGit and
> JGit are too lax by not failing when reading that tree during
> normal processing.

>>> Also, if we're all concerned about "Git reimplementation du jour"
>>> deviations, then we need to focus on libifying Git so there isn't a
>>> need for such re-implementations.  I'm hoping to help with a possible
>>> GSoC project on libgit2, but the lack of a linkable library will
>>> ensure that re-implementations in nearly every useful language will
>>> continue.
>> Don't get me wrong.  I'm not against Git reimplementations per se, as 
>> long as they rigorously implement the exact format and protocol from 
>> CGit.  In that sense it is important that the CGit fsck and verify-pack 
>> tools be exploited on objects/packs produced by alternate Git 
>> implementation systematically to find such issues.
> 
> When JGit had the tree sort order wrong, JGit was in the wrong,
> and any repository which contained those corrupt trees had to be
> fixed by rewriting them.  IIRC it was only the JGit repository
> itself that had this problem in the wild.  But we fixed our code.
> 
> IMHO, this leading '0' thing is a similar breakage.  We shouldn't
> relax CGit or JGit to accept it just because the Ruby implementation
> of Git got the tree encoding wrong.  If anything, we should teach
> these implementations to catch these sorts of problems earlier.
> 

> Nicolas Pitre <nico@fluxnic.net> wrote:
>> On Sat, 27 Mar 2010, Scott Chacon wrote:
>> > > My stance has always been that the C Git is authoritative with regards to
>> > > formats and protocols. ??It's up to Github to fix their screw-up.
>> > 
>> > It is fixed and will be deployed soon, but really, there is no reason
>> > to be snippy.  It is a simple and minor mistake effecting very few
>> > repositories (maybe 100 out of 730k)
>
> What is the C Git stance on these 100 repositories then?  Are they
> now considered corrupt?  Or is 100 enough in the wild that we have
> to accept the problem, just like we accept the 10664 mode issue from
> "ancient" Linux?
>
> I would love to say "those are corrupt, sorry, fix your repository".

> Nicolas Pitre <nico@fluxnic.net> wrote:
>> On Sat, 27 Mar 2010, Scott Chacon wrote:
>> > , and the only reason it's an
>> > issue at all is that JGit is not following the authoritative CGit
>> > implementation of basically ignoring it.
>>
>> But again CGit's fsck is not ignoring this discrepancy.  And if the CGit
>> core is otherwise silently accepting it then it is a mistake.
>
> Right.  I tend to agree.  CGit was too lax here, fsck shouldn't
> be issuing a warning, it should be a fatal error.  Both CGit and
> JGit are too lax by not failing when reading that tree during
> normal processing.

> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > But GitHub's approach here seems to be "Meh, its fine, don't worry
> > about it".
> >
> > Its *NOT* fine.  But Avery and Junio might disagree with me.  :-)
> 
> Did I ever say it is _fine_?  I thought I said "complain loudly".