Re: [PATCH] Teach cvsexportcommit to add new files

From: Junio C Hamano <junkio@cox.net>
Date: Jan 6, 2006, 22:37 UTC
Message-ID: <7virsxrnik.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20060106205441.GA32585@nowhere.earth>

Yann Dirson <ydirson@altern.org> writes:

> Teach cvsexportcommit to add new files
> "cvs add" support was already there, but the "unknown" status
> returned when querying a file not yet known to cvs caused the
> script to abort prematurely.

Thanks. I'll take the patch as is, but I noticed that the cvsexportcommit still has some room for polishing.

 * The command is to be run from CVS working copy (as desribed
   in the documentation) and seems to assume that the parent
   commit is checked out at least for the paths involved between
   the commit and the parent (not clearly documented).

 * I wonder if it is safe with filenames with embedded SP?

 * This undef @files seem premature; later you will say
   cleanupcvs(@files) won't you?

Show 6 quoted lines

> $opt_v && print "The commit affects:\n ";
> $opt_v && print join ("\n ", @afiles,@mfiles,@dfiles) . "\n\n";
> undef @files; # don't need it anymore
> ...
> ### NOTE: if you are planning to die() past this point
> ###       you MUST call cleanupcvs(@files) before die()

 * I think this grep '^Binary' is unsafe; different versions of
   gnu diff say just "Files differ".

> print "'Patching' binary files\n";
> 
> my @bfiles = `git-diff-tree -p $parent $commit | grep '^Binary'`;

 * ls-tree can take $parent (a commit which is by definition a
   treeish) happily here, so rev-parse is not needed.

>     my $tree = `git-rev-parse $parent^{tree} `;
>     chomp $tree;
>     my $blob = `git-ls-tree $tree "$f" | cut -f 1 | cut -d ' ' -f 3`;
>     chomp $blob;

 * another way would be git-hash-object (without -w) the working
   tree file (which you already know matches the $parent
   version) and compare the hash with $blob without extracting
   it with cat-file.

>     `git-cat-file blob $blob > $tmpdir/blob`;
>     `cmp -q $f $tmpdir/blob`;

I am getting an impression that _if_ we care about applying fuzzy patch like this, giving fuzz feature to git-apply would help this program somewhat, but at the same time I do not think of a good reason to even accept nonexact patch when synching two SCM histories.