git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Karthik Nayak <karthik.188@gmail.com>,
	"git\@vger.kernel.org" <git@vger.kernel.org>,
	"christian.couder\@gmail.com" <christian.couder@gmail.com>,
	"Matthieu.Moy\@grenoble-inp.fr" <Matthieu.Moy@grenoble-inp.fr>
Subject: Re: [PATCH v6 01/10] ref-filter: introduce 'ref_formatting_state'
Date: Wed, 29 Jul 2015 12:54:13 -0700	[thread overview]
Message-ID: <xmqqfv4691ui.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CAPig+cS+w8ECma--ncJDoN1fEgrFZMvBC8GBgU6+tLYm_oGkaw@mail.gmail.com> (Eric Sunshine's message of "Wed, 29 Jul 2015 15:19:36 -0400")

Eric Sunshine <sunshine@sunshineco.com> writes:

>> @@ -1254,9 +1273,26 @@ static void emit(const char *cp, const char *ep)
>> +static void reset_formatting_state(struct ref_formatting_state *state)
>> +{
>> +       int quote_style = state->quote_style;
>> +       memset(state, 0, sizeof(*state));
>> +       state->quote_style = quote_style;
>
> I wonder if this sledge-hammer approach of saving one or two values
> before clearing the entire 'ref_formatting_state' and then restoring
> the saved values will scale well. Would it be better for this to just
> individually reset the fields which need resetting and not touch those
> that don't?
>
> Also, the fact that quote_style has to be handled specially may be an
> indication that it doesn't belong in this structure grouped with the
> other modifiers or that you need better classification within the
> structure.

Actually, I think it is wrong to have this function in the first
place.  It is a sign that the caller is doing too little before
calling this function.

If the act of printing an atom uses the formatting state that says
"next one needs X", then it is responsible to clear that "next one
needs X" part of the state, as it is the one who consumed that
state.  E.g. if it used to say "next one needs to be padded to the
right" before entering print_value(), then the function did that
"padded output", then the "next one needs to be padded to the
right" should be cleared inside print_value().

And with that arrangement, together with calling emit() with
formatting state, %(color:blue) can be handled as a normal part of
the formatting state mechanism.  The pseudo/modifier atom should
update the state to "Start printing in blue", and either emit() or
print_value(), whichever is called first, would notice that state,
does what was requested, and flip that bit down (because we are
already printing in "blue" so the next output function does not have
to do the "blue" thing again).

  parent reply	other threads:[~2015-07-29 19:54 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-28  6:32 [PATCH v6 0/10] port tag.c to use ref-filter APIs Karthik Nayak
2015-07-28  6:33 ` [PATCH v6 01/10] ref-filter: introduce 'ref_formatting_state' Karthik Nayak
2015-07-28  6:33   ` [PATCH v6 02/10] ref-filter: add option to pad atoms to the right Karthik Nayak
2015-07-29 19:29     ` Eric Sunshine
2015-07-30 10:18       ` Karthik Nayak
2015-07-28  6:33   ` [PATCH v6 03/10] ref-filter: add option to filter only tags Karthik Nayak
2015-07-28  6:33   ` [PATCH v6 04/10] ref-filter: support printing N lines from tag annotation Karthik Nayak
2015-07-28  6:33   ` [PATCH v6 05/10] ref-filter: add support to sort by version Karthik Nayak
2015-07-29 19:34     ` Eric Sunshine
2015-07-30 10:23       ` Karthik Nayak
2015-07-28  6:33   ` [PATCH v6 06/10] ref-filter: add option to match literal pattern Karthik Nayak
2015-07-28  6:33   ` [PATCH v6 07/10] tag.c: use 'ref-filter' data structures Karthik Nayak
2015-07-28  6:33   ` [PATCH v6 08/10] tag.c: use 'ref-filter' APIs Karthik Nayak
2015-07-28  6:33   ` [PATCH v6 09/10] tag.c: implement '--format' option Karthik Nayak
2015-07-28  6:33   ` [PATCH v6 10/10] tag.c: implement '--merged' and '--no-merged' options Karthik Nayak
2015-07-28  7:26   ` [PATCH v6 01/10] ref-filter: introduce 'ref_formatting_state' Matthieu Moy
2015-07-29 15:56     ` Karthik Nayak
2015-07-29 16:04       ` Matthieu Moy
2015-07-29 16:10         ` Karthik Nayak
2015-07-29 16:35           ` Matthieu Moy
2015-07-29 19:19   ` Eric Sunshine
2015-07-29 19:50     ` Junio C Hamano
2015-07-29 19:54     ` Junio C Hamano [this message]
2015-07-30  9:18       ` Karthik Nayak
2015-07-30  9:25       ` Matthieu Moy
2015-07-29 21:34     ` Matthieu Moy
2015-07-30  6:53     ` Karthik Nayak
2015-07-29 19:27 ` [PATCH v6 0/10] port tag.c to use ref-filter APIs Eric Sunshine
2015-07-29 20:29   ` Junio C Hamano
2015-07-30  9:44     ` Matthieu Moy
2015-07-30 10:13       ` Karthik Nayak
2015-07-30 15:35 ` [PATCH v7 01/11] ref-filter: introduce 'ref_formatting_state' Karthik Nayak
2015-07-30 15:35   ` [PATCH v7 02/11] ref-filter: make `color` use `ref_formatting_state` Karthik Nayak
2015-07-30 15:35   ` [PATCH v7 03/11] ref-filter: add option to pad atoms to the right Karthik Nayak
2015-07-30 15:35   ` [PATCH v7 04/11] ref-filter: add option to filter only tags Karthik Nayak
2015-07-30 15:35   ` [PATCH v7 05/11] ref-filter: support printing N lines from tag annotation Karthik Nayak
2015-07-30 15:35   ` [PATCH v7 06/11] ref-filter: add support to sort by version Karthik Nayak
2015-07-30 15:35   ` [PATCH v7 07/11] ref-filter: add option to match literal pattern Karthik Nayak
2015-07-30 15:35   ` [PATCH v7 08/11] tag.c: use 'ref-filter' data structures Karthik Nayak
2015-07-30 15:35   ` [PATCH v7 09/11] tag.c: use 'ref-filter' APIs Karthik Nayak
2015-07-30 15:35   ` [PATCH v7 10/11] tag.c: implement '--format' option Karthik Nayak
2015-07-30 15:35   ` [PATCH v7 11/11] tag.c: implement '--merged' and '--no-merged' options Karthik Nayak

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqfv4691ui.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=Matthieu.Moy@grenoble-inp.fr \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=karthik.188@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).