From: Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>
To: Karthik Nayak <karthik.188@gmail•com>
Cc: git@vger•kernel.org, christian.couder@gmail•com, gitster@pobox•com
Subject: Re: [PATCH v5 01/11] ref-filter: introduce 'ref_formatting_state'
Date: Mon, 27 Jul 2015 14:42:15 +0200 [thread overview]
Message-ID: <vpqmvyhai1k.fsf@anie.imag.fr> (raw)
In-Reply-To: <1437982035-6658-1-git-send-email-Karthik.188@gmail.com> (Karthik Nayak's message of "Mon, 27 Jul 2015 12:57:05 +0530")
Karthik Nayak <karthik.188@gmail•com> writes:
> +static void ref_formatting(struct ref_formatting_state *state,
> + struct atom_value *v, struct strbuf *value)
> {
> - struct strbuf sb = STRBUF_INIT;
> - switch (quote_style) {
> + strbuf_addf(value, "%s", v->s);
> +}
You're taking 'state' as argument, but you're not using it in the
function for now. Perhaps add a temporary comment like:
static void ref_formatting(...)
{
/* Formatting according to 'state' will be applied here */
strbuf_addf(...)
}
Or perhaps it's OK like this.
> -static void print_value(struct atom_value *v, int quote_style)
> +static void print_value(struct ref_formatting_state *state, struct atom_value *v)
Changing the position of the v parameter makes the patch a bit harder to
read. I would have written in this order:
static void print_value(struct atom_value *v, struct ref_formatting_state *state)
So the patch reads as "encapsulate quote_style in a struct" more
straightforwardly.
> @@ -1257,6 +1269,10 @@ static void emit(const char *cp, const char *ep)
> void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
> {
> const char *cp, *sp, *ep;
> + struct ref_formatting_state state;
I still found it a bit hard to read, and I would have appreciated a
comment here, like
/*
* Some (pseudo) atom have no immediate side effect, but only
* affect the next atom. Store the relevant information from
* these atoms in the 'state' variable for use when displaying
* the next atom.
*/
With this in mind, it becomes more obvious that you also need to reset
the state after using it, which you forgot to do. See:
$ ./git for-each-ref --format '%(padright:30)|%(refname)|%(refname)|' refs/tags/v2.4.\*
|refs/tags/v2.4.0 |refs/tags/v2.4.0 |
|refs/tags/v2.4.0-rc0 |refs/tags/v2.4.0-rc0 |
|refs/tags/v2.4.0-rc1 |refs/tags/v2.4.0-rc1 |
|refs/tags/v2.4.0-rc2 |refs/tags/v2.4.0-rc2 |
|refs/tags/v2.4.0-rc3 |refs/tags/v2.4.0-rc3 |
|refs/tags/v2.4.1 |refs/tags/v2.4.1 |
|refs/tags/v2.4.2 |refs/tags/v2.4.2 |
|refs/tags/v2.4.3 |refs/tags/v2.4.3 |
|refs/tags/v2.4.4 |refs/tags/v2.4.4 |
|refs/tags/v2.4.5 |refs/tags/v2.4.5 |
|refs/tags/v2.4.6 |refs/tags/v2.4.6 |
I think only the first column should have padding, not the second. You
can fix this with a patch like this:
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1431,6 +1431,14 @@ static void apply_pseudo_state(struct ref_formatting_state *state,
state->ifexists = v->s;
}
+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;
+}
+
+
/*
* If 'lines' is greater than 0, print that many lines from the given
* object_id 'oid'.
@@ -1492,8 +1500,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format,
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
if (atomv->pseudo_atom)
apply_pseudo_state(&state, atomv);
- else
+ else {
print_value(&state, atomv);
+ reset_formatting_state(&state);
+ }
+
}
if (*cp) {
sp = cp + strlen(cp);
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
next prev parent reply other threads:[~2015-07-27 12:42 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-27 7:26 [PATCH v5 00/11] port tag.c to use ref-filter APIs Karthik Nayak
2015-07-27 7:27 ` [PATCH v5 01/11] ref-filter: introduce 'ref_formatting_state' Karthik Nayak
2015-07-27 7:27 ` [PATCH v5 02/11] ref-filter: make `color` use `ref_formatting_state` Karthik Nayak
2015-07-27 12:47 ` Matthieu Moy
2015-07-27 14:28 ` Junio C Hamano
2015-07-27 17:23 ` Karthik Nayak
2015-07-27 16:02 ` Karthik Nayak
2015-07-27 13:06 ` Matthieu Moy
2015-07-27 17:16 ` Karthik Nayak
2015-07-27 7:27 ` [PATCH v5 03/11] ref-filter: add option to pad atoms to the right Karthik Nayak
2015-07-27 12:50 ` Matthieu Moy
2015-07-27 15:45 ` Junio C Hamano
2015-07-27 15:54 ` Matthieu Moy
2015-07-27 18:42 ` Karthik Nayak
2015-07-27 18:47 ` Matthieu Moy
2015-07-27 18:54 ` Karthik Nayak
2015-07-27 18:56 ` Junio C Hamano
2015-07-27 18:43 ` Karthik Nayak
2015-07-28 21:41 ` Eric Sunshine
2015-07-29 16:16 ` Karthik Nayak
2015-07-27 7:27 ` [PATCH v5 04/11] ref-filter: add option to filter only tags Karthik Nayak
2015-07-27 7:27 ` [PATCH v5 05/11] ref-filter: support printing N lines from tag annotation Karthik Nayak
2015-07-27 7:27 ` [PATCH v5 06/11] ref-filter: add support to sort by version Karthik Nayak
2015-07-27 7:27 ` [PATCH v5 07/11] ref-filter: add option to match literal pattern Karthik Nayak
2015-07-27 12:54 ` Matthieu Moy
2015-07-27 15:57 ` Karthik Nayak
2015-07-27 15:59 ` Karthik Nayak
2015-07-27 16:06 ` Matthieu Moy
2015-07-27 16:48 ` Karthik Nayak
2015-07-28 21:49 ` Eric Sunshine
2015-07-29 16:17 ` Karthik Nayak
2015-07-30 11:21 ` Karthik Nayak
2015-07-27 7:27 ` [PATCH v5 08/11] tag.c: use 'ref-filter' data structures Karthik Nayak
2015-07-27 7:27 ` [PATCH v5 09/11] tag.c: use 'ref-filter' APIs Karthik Nayak
2015-07-27 7:27 ` [PATCH v5 10/11] tag.c: implement '--format' option Karthik Nayak
2015-07-27 7:27 ` [PATCH v5 11/11] tag.c: implement '--merged' and '--no-merged' options Karthik Nayak
2015-07-27 12:42 ` Matthieu Moy [this message]
2015-07-27 15:28 ` [PATCH v5 01/11] ref-filter: introduce 'ref_formatting_state' 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vpqmvyhai1k.fsf@anie.imag.fr \
--to=matthieu.moy@grenoble-inp$(echo .)fr \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=karthik.188@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox