Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 28, 2012, 17:55 UTC
Message-ID: <7vr4pmm3qz.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1348815682-18696-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

Show 29 quoted lines

> Similar to --author/--committer which filters commits by author and
> committer header fields. --grep-reflog adds a fake "reflog" header to
> commit and a grep filter to search on that line.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  On Fri, Sep 28, 2012 at 12:09 AM, Junio C Hamano <gitster@pobox.com>
>  wrote:
>  > I am debating myself if it is sane for this option to have no hint
>  > that it is about "limiting" in its name.  "--author/--committer"
>  > don't and it is clear from the context of the command that they are
>  > not about setting author/committer, so "--reflog-message" may be
>  > interpreted the same, perhaps.
>  >
>  > The entry in the context above talks about multiple occurrence of
>  > that option. Shouldn't this new one also say what happens when it
>  > is
>  > given twice?
>
>  Fixed.
>
>  > I think I wrote prep_header_patterns() and compile_grep_patterns()
>  > carefully enough not to assume the headers are only the author and
>  > committer names, so the various combinations i.e. all-match,
>  > author(s), committer(s), grep(s), and reflog-message(s), should
>  > work
>  > out of the box, but have you actually tested them?
>
>  I did not. I do now, tests are also added.

You seem to have skipped a lot more important comment from the message that came immediately after the above part. Did you take a look at grep.c::match_one_pattern() that calls strip_timestamp() for any and all GREP_PATTERN_HEAD items?

This is why I think that the [1/3] in this series shows a total lack of design taste. Each element in "headers", unlike "body", is a structured field and its payload has meaning depending on what field name is associated with it. That is why we do not give a way to users to willy-nilly add ad-hoc header fields.

The other side of this coin is that Git ought to be able to take advantage of the meaning of each header. Stripping the timestamp part when doing a textual match against --author/--committer is one example. The code in match_one_pattern() to deal with the header patterns need to be _extended_ to take into account the special treatment each header, includign this new "reflog " fake header, wants when doing a textual match, and when we add any other match against header (either real or fake), we should be able to add specific matching logic tailored to handle the particular meaning of the header would have.

For that to happen, the code _must_ know what kind of headers we would support; discarding the existing enum is going in a wrong direction.

When we introduce "git log --header=frotz:xyzzy" option that looks for a generic "frotz " header and tries to see if "xyzzy" textually appears in the field, we may want to add a generic "this is not a header that we have special treatment for" enum to the mix. But for known kinds of headers, we would need a list of what each header is and what semantics it wants.

So please reconsider undoing [1/3], and rerolling [2/3] that depends on it.