Re: [PATCH/RFC v4] grep: Add the option '--exclude'

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 14, 2012, 17:35 UTC
Message-ID: <7vfwed8gis.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CACsJy8DhtjG6AhPkb0SEm4g6zhtmuRb5x+4+P3A6eS0+_7OQNw@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

Show 5 quoted lines

> It makes me wonder, why not add match_pathspec_with_exclusion(const
> struct pathspec *include_ps, const struct pathspec *exclude_ps,...),
> use the new function in grep.c and revert struct pathspec back to
> original? The same can be applied to tree_entry_interesting() (i.e.
> add a new one that takes two pathspec sets, which supports exclusion)

Sorry, but I am the one to blame for this in:

 http://thread.gmane.org/gmane.comp.version-control.git/189455/focus=189486

> I think you may make less changes that way.

The arrangement to use two pathspecs certainly is simpler in the short term, but I think it is a short-sighted hack that is wrong for two reasons.

 - Doing it that way will not give you a solution where you only have to
   update the command line parsing of an existing command to stuff
   negative patterns to the pathspec structure you have been passing down
   to the existing codepath, and the command starts to understand negative
   patterns without any other change.  You introduce a new pathspec that
   holds negative patterns, and pass that along with the positive one you
   have been passing down in the existing codepath for all commands that
   need to understand pathspec (otherwise we will be back to a similar
   situation we were in before you started looking at pathspec where we
   had three different implementations giving different semantics---some
   command knows how to handle this kind of pathspec but some others do
   not).

 - Look at the fields in 'struct pathspec' and think about what those
   outside "items" list mean. The recursive and max_depth fields are about
   how the namespace is traversed [*1*], so if a codepath that used to
   know only about positive patterns learns to also care about negative
   ones, using different settings for these two fields in two separate
   'struct pathspec' does not make sense.  It is conceptually much cleaner
   to make 'struct pathspec' the data structure used by the updated logic
   to match paths to pathspec that can have positive and negative patterns.

[Footnote]

*1* The has_wildcard field is about optimizing the matching during the traversal so it is a bit different. Also it is our longer term goal to get rid of the "raw" field (which does not work well to substring match against the path when pathspec "magic" is involved), so we won't worry about it in this discussion.