From: Nicolas Pitre <nico@cam.org>
Date: Thu, 28 Apr 2005 01:02:48 GMT
Subject: Re: [PATCH] add a diff-files command
Message-ID: <Pine.LNX.4.62.0504272031330.14033@localhost.localdomain>
In-Reply-To: <7vr7gvevpv.fsf@assigned-by-dhcp.cox.net>

On Wed, 27 Apr 2005, Junio C Hamano wrote:

> >>>>> "NP" == Nicolas Pitre <nico@cam.org> writes:
> 
> NP> It also has the ability to accept exclude file patterns with -x and even 
> NP> a file containing a list of patterns to exclude with -X.  This is 
> NP> especially useful to use the famous dontdiff file when looking for 
> NP> uncommitted files in a compiled kernel tree.
> 
> I think show-diff with path restriction (if restriction is
> simple), or piping its output to grep or filterdiff (otherwise),
> would be enough to do what you do here, so personally I doubt
> this new command is even useful that much.

First, show-diff doesn't handle files in the work tree which are not 
listed in the cache.

Have you ever looked at the dontdiff file?  You can get a sample of it 
from http://www.moses.uklinux.net/patches/dontdiff to give you an idea.  
Using grep or filterdiff is really backward in that case since out of 
all the junk that might appear in the output about 98% will be filtered 
away in most useful cases, which is rather inefficient.

Path restriction is inclusive, while the exclude list is, well, 
exclusive.  They serves separate purposes.  So trust me it _is_ pretty 
damn useful, unless you always run "make clean" on your kernel tree 
before checking for potentially uncommitted files then recompile 
everything afterwards which is a hassle.

> That said, I have a couple of comments.  Other than these I do
> not see anything majorly wrong (although I haven't even compiled
> it yet ;-).
> 
> NP> +static const char *diff_files_usage = "diff-files [-a] [-c] [-d] [-o] [-p | -z]"
> NP> +				      " [-x <pattern>] [-X <file>] [paths...]";
> NP> +
> 
> If you are trying to do something similar to show-files by these
> -[acdo] flags, matching these flags in both commands would be
> less confusing to the users and script writers.  Either make
> diff-files take fully spelled --others etc. that show-files
> takes, or submit a patch for show-files to match these shorter
> ones as well.  I personally prefer the latter.

Agreed.

> NP> +/*
> NP> + * Read a directory tree. We currently ignore anything but
> NP> + * directories and regular files. That's because git doesn't
> NP> + * handle them at all yet. Maybe that will change some day.
> NP> + *
> NP> + * Also, we currently ignore all names starting with a dot.
> NP> + * That likely will not change.
> NP> + */
> 
> For that logic, instead of doing de->d_name[0] == '.' and things
> yourself, I'd rather see you lift verify_path() function from
> update-cache.c into common library and call it.

Sure, but actually I'd prefer for that to be a separate patch since it's 
a tangential issue that might affect show-files as well.


Nicolas