Re: [PATCH 0/2] git diff -q option removal
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 17, 2013, 17:04 UTC
- Message-ID
- <7vvc496ruf.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1373970487-32595-1-git-send-email-stefanbeller@googlemail.com>
Stefan Beller <stefanbeller@googlemail.com> writes:
Show 8 quoted lines
> The changes in the following patch are in diff_no_index.c, but the
> diff_no_index(...) is called from cmd_diff, which is in builtin/diff.c
> That cmd_diff is actually called from git.c having the
> { "diff", cmd_diff }, entry in handle_internal_command.
>
> My question now is this: Why is the builtin/diff.c relying on stuff
> outside of builtin/ ? Wouldn't it be better to move all these files
> (such as diff_no_index.c) into the builtin folder as well?Builtins link all sorts of stuff from outside, e.g. diff.c and diffcore-*.c at the toplevel. I do not see diff_no_index.c is any different, so I am probably not understanding your question.
Show 12 quoted lines
> Regarding the removal of the -q option, I tried it in the second patch. > Is it as easy as that, or am I missing the point? > > The first patch doesn't change the behavior, so I'd assume it's safe to > apply it to origin/sb/misc-fixes, whereas the second patch will make > git diff complain about the -q option, so I'd assume it would wait for the > next major release? > > Before: > touch actual_file > git diff -q actual_file no_file > error: Could not access 'no_file'
Hmm, do you really get that error message? I think you would get
fatal: ambiguous argument 'no_file': unknown revision or path not in the working tree.
> echo $? > 1
The command line parsing infrastructure has changed vastly since "show-diff" days (see below for a history lesson); I think your "Before" should read more like this
git diff -q -- actual_file no_file
and it should not show removal of no_file in its output. E.g. in git.git
$ git reset --hard
$ rm COPYING
$ git diff -q -- COPYINGshould show nothing.
I personally think "-q" no longer makes sense in today's codebase, but I am not convinced that removal of '-q' from the proper "git diff-files" and the "git diff --no-index" (aka "I am too lazy to teach our diff enhancement to other people's diff implementations, so let's throw in a "files do not have to be tracked in Git repository at all" mode") is the right direction to go.
The "-q" option is a remnant from the "show-diff" command, the precursor of today's "git diff-files" (back then, we didn't even have "git" potty. The user literally typed "show-diff", not "git show-diff").
ca2a0798 ([PATCH] Add "-q" option to show-diff.c, 2005-04-15) added that option. Back then, we did not have pathspec matching, and we iterated over command line arguments, and required all of them exist as filesystem entities. "-q" was a way to defeat that "you name a file, it must exist in the working tree" safety, and also at the same time not give output for such a file that was removed from the working tree.
These days, the former "safety" is enforced by the generalized revision parser ("is it a path or is it a rev?") code and the "--" delimiter on the command line is the way to defeat it. The latter is done by giving a filtering specification that lack D to the "--diff-filter".
If we wanted to make "-q" follow the spirit of its original addition to "show-diff" again, we could internally add a diff-filter when the "-q" option is parsed.
"git diff -q ..." is "git diff --diff-filter=ACMRTUB ...", and "git diff -q --diff-filter=AD" is "git diff --diff-filter=A". That would let us remove the special case for SILENT_ON_REMOVED, and also make "-q" work across various commands in the "diff" family. It might even make it work for "diff --no-index", but I didn't bother to check.
Show 17 quoted lines
> After: > touch actual_file > git diff -q actual_file no_file > fatal: invalid diff option/value: -q > echo $? > 128 > > Thanks, > Stefan > > Stefan Beller (2): > diff --no-index: remove nonfunctional "-q" handling > git diff: Remove -q option to stay silent on missing files. > > Documentation/git-diff-files.txt | 6 +----- > diff-no-index.c | 5 ----- > 2 files changed, 1 insertion(+), 10 deletions(-)