Another fast-import/import-tars issue

Hi, folks.

I believe I've uncovered an issue in fast-import, but I don't know the code well enough yet to debug it. So, I'll produce my evidence and let others work on finding the solution. It should be pretty easy to reproduce.

First, I'm running: 1.5.2.rc1.9.g6644

Grab the tarball of Perl 5.8.8 - http://www.perl.com/CPAN/src/perl-5.8.8.tar.bz2

Note its md5, just so you know it's not corrupted from the outset. b8c118d4360846829beb30b02a6b91a7 perl-5.8.8.tar.gz a377c0c67ab43fd96eeec29ce19e8382 perl-5.8.8.tar.bz2

Try this:

$ tar -xjf perl-5.8.8.tar.bz2 $ cd perl-5.8.8 $ git init $ git add . $ git commit -a -m "Import from working tree copy"

Now, for convenience of debugging, I have myself a script I call ~/bin/fast-import-filter.sh:

#!/bin/bash tee fast-import.log | git fast-import --quiet

Then, I have a slightly-changed ~/bin/import-tars script, like so:

20c20,21 < open(FI, '|-', 'git', 'fast-import', '--quiet') ---

Now,

$ import-tars.pl ../perl-5.8.8.tar.bz2

Okay, so the trees pointed to by the tips of the master and import-tars branches *should* be identical here, right?

$ git diff-tree master: import-tars: | wc -l 229

Not so good.

Earlier, we used the mode to determine if a name was associated with a directory. This fails, since some tar programs do not set the mode correctly. However, the link indicator _has_ to be set correctly.

Noticed by Chris Riddoch.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Fri, 11 May 2007, Chris Riddoch wrote:

	> I believe I've uncovered an issue in fast-import, but I don't 
	> know the code well enough yet to debug it.  So, I'll produce my 
	> evidence and let others work on finding the solution.  It should 
	> be pretty easy to reproduce.

	It was easy. Thanks.

	The problem is -- again -- that a directory is overwritten, since 
	it is not recognized as a directory. Earlier, I tried to use the 
	trailing "/" for that. Which fails with your example.

	I actually took the time to research in Wikipedia what should be 
	the correct way to find out if the current item is a directory...

 contrib/fast-import/import-tars.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

This matches my reading of GNU tar as well. The patch would not break a correctly made tar archive, would fix importing archives created by a broken tar that does not do mode right (but uses the correct typeflag), _and_ would _break_ archives created by tar that is broken in a different way, sets the mode right but uses a wrong typeflag.

I do not know which breakages are more common, and this one being in contrib/ I do not think it really matters in practice, but as a principle, I think we should try to adhere to the same "no regression" policy the kernel folks try to adhere to. If something used to work, even if its was by accident or a bug, we had better have a pretty good reason to break it by a change that fixes things for other people, _even_ when that other people outnumber the people who are affected by the regression.

I'd first ask GNU tar maintainer if he knows of existing implementations of tar that are broken in the latter sense (iow, sets modes correctly but typeflag incorrectly), as the tarball extraction codepath would have the exact same issue.

Junio C Hamano <junkio@cox.net> writes:

Nah, what was I smoking. Even gtar seems to give mode="0000775" (with NUL termination) for directories, so there is no way there were regressions. Patch looks good.

	Acked-by: Junio C Hamano <junkio@cox.net>