Re: [PATCH] add a diff-files command

From: Nicolas Pitre <nico@cam.org>
Date: Apr 28, 2005, 01:02 UTC
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:

Show 11 quoted lines

> >>>>> "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.

Show 14 quoted lines

> 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.

Show 12 quoted lines

> 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