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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 12, 2017, 17:49 UTC
Message-ID: <xmqqk27uv100.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CACBZZX4v49zfyGVpcxGSKsxbMfVaUcGHtitpfaZMUtG82YzW-g@mail.gmail.com>

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

Show 7 quoted lines

> 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").

Show 6 quoted lines

> 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.

Show 7 quoted lines

>> 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?

Show 14 quoted lines

>
>> 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.

Show 11 quoted lines

>>> -#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.