From: Junio C Hamano Date: Wed, 27 Apr 2005 21:49:32 GMT Subject: Re: [PATCH] add a diff-files command Message-ID: <7vr7gvevpv.fsf@assigned-by-dhcp.cox.net> In-Reply-To: >>>>> "NP" == Nicolas Pitre 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. 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 ] [-X ] [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. 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. Then if the "likely will not change" part needs to be updated, you do not have to worry about it; updates to verify_path() would take care of it for you.