Re: [PATCH] diff -- file1 file2: do not default to --no-index inside a git repository

[Junio C Hamano <gitster@pobox.com>]

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

> 	On Fri, 23 May 2008, Junio C Hamano wrote:
>
> 	> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 	> 
> 	> > ... So I would suggest that we make it *much* harder to 
> 	> > trigger the "make it act like a traditional 'diff'" thing.
> 	> >
> 	> > I would suggest that we *not* invoce the traditional 'diff' 
> 	> > behaviour when:
> 	> >
> 	> >  - we're called as "git-diff-files". That's clearly a git 
> 	> >    thing. Don't try to make it act like an external non-git diff. 
> 	> >    Only do the special case for plain "git diff" itself.
>
> 	I am not so sure about that.

Unsure that "git-diff-files" is clearly a git thing?

> 	> >  - even for plain "git diff", make it much harder to trigger 
> 	> >    non-git behaviour. Don't do it if the files don't exist.
> 	> >    Don't do it if there is '--' there.
> 	> >
> 	> > In fact, maybe we should remove that thing entirely, or 
> 	> > *require* a flag to enable it (at least if we're in a git
> 	> > directory).  It's a cute hack, but when the cute hack actually 
> 	> > makes it impossible to do certain real git operations, it's a 
> 	> > cute hack that is detrimental.
> 	> 
> 	> Very well said.  I've always wanted to rip that hack out of the 
> 	> normal "git diff with two pathspec parameters" codepath.
>
> 	Yes, I see how that cute hack does the wrong thing here.
>
> 	However, it is pretty convenient to be able to say "git diff a b" 
> 	and have it fall back to --no-index if either a or b is not in the 
> 	index.  Most of the time, I would actually expect it to be the 
> 	right thing, too.

"Most-of-the-time" may be a good thing in Porcelains, but is clearly a
wrong thing for plumbing.

I think ripping out --no-index from git-diff-files is probably the first
step.  "git diff" also needs to fixed as Linus outlined, but that is a
separate topic.

How about doing something like this as a start?  At least that would get
well behaved Porcelains like stg unstuck?

---

 builtin-diff-files.c |   38 +++++++++++++++++++++++++++++++-------
 git.c                |    2 +-
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index e2306c1..6898e3e 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -10,26 +10,50 @@
 #include "builtin.h"
 
 static const char diff_files_usage[] =
-"git-diff-files [-q] [-0/-1/2/3 |-c|--cc|--no-index] [<common diff options>] [<path>...]"
+"git-diff-files [-q] [-0/-1/2/3 |-c|--cc] [<common diff options>] [<path>...]"
 COMMON_DIFF_OPTIONS_HELP;
 
 int cmd_diff_files(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info rev;
-	int nongit;
 	int result;
+	unsigned options;
 
-	prefix = setup_git_directory_gently(&nongit);
 	init_revisions(&rev, prefix);
 	git_config(git_diff_basic_config); /* no "diff" UI options */
 	rev.abbrev = 0;
 
-	if (!setup_diff_no_index(&rev, argc, argv, nongit, prefix))
-		argc = 0;
-	else
-		argc = setup_revisions(argc, argv, &rev, NULL);
+	argc = setup_revisions(argc, argv, &rev, NULL);
+	while (1 < argc && argv[1][0] == '-') {
+		if (!strcmp(argv[1], "--base"))
+			rev.max_count = 1;
+		else if (!strcmp(argv[1], "--ours"))
+			rev.max_count = 2;
+		else if (!strcmp(argv[1], "--theirs"))
+			rev.max_count = 3;
+		else if (!strcmp(argv[1], "-q"))
+			options |= DIFF_SILENT_ON_REMOVED;
+		else
+			usage(diff_files_usage);
+		argv++; argc--;
+	}
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
+
+	/*
+	 * Make sure there are NO revision (i.e. pending object) parameter,
+	 * rev.max_count is reasonable (0 <= n <= 3), and
+	 * there is no other revision filtering parameters.
+	 */
+	if (rev.pending.nr ||
+	    rev.min_age != -1 || rev.max_age != -1 ||
+	    3 < rev.max_count)
+		usage(diff_files_usage);
+
+	if (rev.max_count == -1 &&
+	    (rev.diffopt.output_format & DIFF_FORMAT_PATCH))
+		rev.combine_merges = rev.dense_combined_merges = 1;
+
 	result = run_diff_files_cmd(&rev, argc, argv);
 	return diff_result_code(&rev.diffopt, result);
 }
diff --git a/git.c b/git.c
index 89b431f..4b79380 100644
--- a/git.c
+++ b/git.c
@@ -293,7 +293,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
 		{ "describe", cmd_describe, RUN_SETUP },
 		{ "diff", cmd_diff },
-		{ "diff-files", cmd_diff_files },
+		{ "diff-files", cmd_diff_files, RUN_SETUP },
 		{ "diff-index", cmd_diff_index, RUN_SETUP },
 		{ "diff-tree", cmd_diff_tree, RUN_SETUP },
 		{ "fast-export", cmd_fast_export, RUN_SETUP },