From: Junio C Hamano <gitster@pobox•com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] diff --no-index: allow pathspec after --
Date: Thu, 18 Sep 2014 15:41:44 -0700 [thread overview]
Message-ID: <xmqqha04o8k7.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1410256584-19562-1-git-send-email-pclouds@gmail.com> ("Nguyễn Thái Ngọc Duy"'s message of "Tue, 9 Sep 2014 16:56:24 +0700")
Nguyễn Thái Ngọc Duy <pclouds@gmail•com> writes:
> Another patch to test the water before I put more effort into it.
>
> Commit d516c2d (Teach git-diff-files the new option `--no-index` -
> 2007-02-22) brings the bells and whistles of git-diff to the world
> outside a git repository. This patch continues that direction and adds
> a new syntax
>
> git diff --no-index [--] <path> <path> -- <pathspec>
>
> It's easy to guess that the two directories will be filtered by
> pathspec,...
Ugh. Nobody's diff works that way, and nowhere in our UI we use
double-dashes that way, either.
So while I like the idea of "I want to tell Git to compare these two
directories with these pathspec", I do not think we would want to
touch such a syntax with 10 foot pole X-<.
> @@ -194,19 +207,23 @@ void diff_no_index(struct rev_info *revs,
> int j;
> if (!strcmp(argv[i], "--no-index"))
> i++;
> - else if (!strcmp(argv[i], "--"))
> + else if (!strcmp(argv[i], "--")) {
> i++;
> - else {
> + break;
> + } else {
I think this part is a good bugfix regardless of your new feature.
The general command line structure ought to be:
$ git diff [--no-index] [--<diff options>...] [--] <path> <path>
but the existing code is too loose in that "--" is just ignored.
The caller in builtin/diff.c makes sure "--no-index" comes before
"--", the latter of which stops option processing, so it is not so
grave a bug, though.
Coming back to the command line syntax for the new feature, if I had
to choose, I would say
git diff --no-index [-<options>] [--] <path> <path> <pathspec>
perhaps? As we never compare anything other than two things, we can
do the following:
* Implicit --no-index heuristics will kick in _ONLY_ when we have
two things at the end after parsing options in builtin/diff.c, as
before;
* In diff_no_index() after parsing options at its beginning,
- if we have only two things left, they may be
. two files;
. a file and "-" or "-" and a file; or
. two directories (fully traversed without pathspecs)
just as before.
- if we have more than two things left, the first two of these
_MUST_ be directories, and the remainder is pathspec to filter
the result of directory traversal.
Wluldn't that be cleaner and easier to understand?
next prev parent reply other threads:[~2014-09-18 22:41 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-09 9:56 [PATCH] diff --no-index: allow pathspec after -- Nguyễn Thái Ngọc Duy
2014-09-18 22:41 ` Junio C Hamano [this message]
2014-09-21 4:08 ` Duy Nguyen
2014-09-21 6:19 ` Junio C Hamano
2014-09-21 9:25 ` Duy Nguyen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqha04o8k7.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=pclouds@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox