Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> On Fri, 23 May 2008, Junio C Hamano wrote:
>
> > Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> 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 },
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
