Re: [PATCH v4] ref-filter: Add --no-contains option to tag/branch/for-each-ref

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> And then later in the documentation:
>
>     -l <pattern>, --list <pattern>
>
> I.e. for git-branch this type of invocation wouldn't make sense and
> would just happen to work, but for git-tag the --list option is
> explicitly documented to immediately take a <pattern> argument.

But that (i.e. "to immediately take") is not how it actually works,
as you already know after looking at how 'l' is defined as OPT_CMDMODE.

The command line is more like "-l" chooses the "list mode", and the
pattern is _NOT_ an argument to the option at all.  The command line
is more like "-l <options to affect selection criteria>..." and the
<pattern> is one of these criteria.  The command line convention being
dashed-options first then other arguments, it makes sense to do

	-l --contains HEAD v\*

because "--contains HEAD" is a dashed-option (which takes an argument)
and "v\*" is a pattern (which is "other arguments").

> I'll change it.
>
>>     git tag -l --no-contains v2.10.1-3-gcf5c7253e0 'v[0-9]*' |
>>     sort | tail -n 10
>
> Although I'll add a \ to that so you can still paste it to a terminal.

Please don't.  The shell knows, when you end a line with pipe, that
you haven't finished your sentence and keeps listening to you.

>> Reviewers would appreciate you refrain from doing that in the same
>> patch.  Do a minimum patch so that the review can concentrate on
>> what got changed (i.e. contents), followed by a mechanical reflow as
>> a follow-up, or something like that, would be much nicer to handle.
>
> Okey, so two patches, one where I potentially produce very long lines
> & then re-flow them in a subsequent commit.

Or preferrably, the last "-" line in a hunk of your first patch may
be longer than the first "+" line that replaces it that may be
overly short (i.e. chopping the end of existing paragraph and
replacing the remainder).  And then reflow comes, e.g.

    -Okey, so two patches, one where I potentially produce very long lines
    +Okey, so two patches, one where I
    +cut an existing line short if it makes the patch churn smaller
     & then re-flow them in a subsequent commit.

> If I'd like to base on top of that to make things easier for you do
> you publish  jk/ref-filter-flags-cleanup sowhere? I.e. as a git ref
> rather than me also following that topic, applying it on top of
> master, and then applying my topic on top of that.

Do you mean a repository that holds broken-out topics?  If so:

	git://github.com/gitster/git/

is what you are looking for, perhaps?

>
>> and have both default to HEAD?  I know that would not make sense as
>> a set operation, but I am curious what our command line parser
>> (which is oblivious to what the command is doing) does.  I am guessing
>> that it would barf saying "--contains" needs a commit but "--no-contains"
>> is not a commit (which is very sensible)?
>
> It'll spew out "error: malformed object name --no-contains".
>
> You can do "--contains HEAD --no-contains HEAD" to get nothing.
>
> In an earlier thread I was discussing with Jeff whether it would be
> worthwhile to error out in that case, but his opinion was
> (paraphrasing) "Nah, GIGO", which I think makes sense in this case.

I agree with Peff (I said "that would not make sense as a set
operation", didn't I? ;-); I was only curious if the notation used
in the documentation, i.e. "--opt [<object>]" made sense.  It looks
as if it would accept "--contains --no-contains" (omitting arguments
from both options), but it is not so, and I was wondering if we need
to improve the documentation, or the readers are OK with the notation.

>>> -#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, 0)
>>> +#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, PARSE_OPT_NONEG)
>>> +#define OPT_NO_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("no-contains", v, h, PARSE_OPT_NONEG)
>>>  #define OPT_WITH(v, h) _OPT_CONTAINS_OR_WITH("with", v, h, PARSE_OPT_HIDDEN)
>>> +#define OPT_WITHOUT(v, h) _OPT_CONTAINS_OR_WITH("without", v, h, PARSE_OPT_HIDDEN)
>>
>> Hmph, perhaps WITH/WITHOUT also do not take "--no-" form hence need OPT_NONEG?
>
> I may be missing some subtlety here, but I think you've misread this
> (admittedly dense) chunk. the /WITH/ options don't supply NONEG, just
> HIDDEN.

Maybe I was unclear.  As --contains should not take --no-contains
and use "unset" (because new code wants to see "no-contains" and act
on it in the new code, I was wondering if we should forbid --no-with
and --no-without in a similar way by using OPT_NONEG in addition to
OPT_HIDDEN.