git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Duy Nguyen <pclouds@gmail.com>
To: Karthik Nayak <karthik.188@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, Git <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>,
	Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>,
	Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option
Date: Sun, 12 Jul 2015 08:47:39 +0700	[thread overview]
Message-ID: <CACsJy8BsLnkPFX04WujMdnBiRGmXacF1QSQudb3T8nxGrZ_1Fg@mail.gmail.com> (raw)
In-Reply-To: <CAOLa=ZTNGV=NAXDPB3HX=fanPMQaQQc4ZpFdDFeg+LJ8SFuVOQ@mail.gmail.com>

On Sat, Jul 11, 2015 at 7:05 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Fri, Jul 10, 2015 at 9:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> This may be enough to support the various existing formats that are
>> offered by "git branch" and/or "git tag", but I do not think if this
>> is the right approach in the longer term, or if we are painting
>> ourselves in a corner we cannot cleanly get out of later [*1*].
>> Will the "refname" stay to be the only thing that may want alignment
>> padding appended in the future?  Will it stay true that we want to
>> align only to the left?  Etc., etc.
>>
>> Cc'ed Duy as %< in the pretty-format was his invention at around
>> a5752342 (pretty: support padding placeholders, %< %> and %><,
>> 2013-04-19).
>>
>
> I kinda had the same though, my only justification was that it was only being
> internally used. I'll have another look if as to see if I can make it
> universal somehow.
> Let's see what Duy has to suggest.

I guess if you can have multiple arguments after ':' in an atom, then
you have wiggle room for future. But it looks like you only accept one
argument after ':'.. (I only checked the version on 'pu'). Having an
"alignment atom" to augment the real one (like %< changes the behavior
of the next placeholder), could also work, but it adds dependency
between atoms, something I don't think ref-filter.c is ready for.

Another thing, the atom value is also used for sorting. When used for
sorting, I think these padding spaces should not be generated or it
may confuse the sort algorithm. Left alignment may be ok, right or
center alignment (in future?), not  so much. Perhaps we should do the
padding in a separate phase, outside populate_value(). If you go this
route, having separate atoms for alignment works better: you don't
have to parse them in populate_value() which is for actual values, and
you can handle dependency easily (I think).

By the way, please consider adding _() back to translatable strings,
usually those die() or warn(), or "[ahead %s]"... In the last case,
because you don't really know how long the string is after
translation, avoid hard coding buffer size (to 40).
-- 
Duy

  reply	other threads:[~2015-07-12  1:48 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-09 10:27 [PATCH v2 00/10] Port tag.c to use ref-filter APIs Karthik Nayak
2015-07-09 10:27 ` [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option Karthik Nayak
2015-07-09 10:27   ` [PATCH v2 02/10] ref-filter: add option to filter only tags Karthik Nayak
2015-07-09 10:27   ` [PATCH v2 03/10] ref-filter: support printing N lines from tag annotation Karthik Nayak
2015-07-09 13:07     ` Matthieu Moy
2015-07-10 10:38       ` Karthik Nayak
2015-07-09 10:27   ` [PATCH v2 04/10] ref-filter: add support to sort by version Karthik Nayak
2015-07-09 13:29     ` Matthieu Moy
2015-07-10 10:52       ` Karthik Nayak
2015-07-10 11:01         ` Karthik Nayak
2015-07-10 12:18           ` Matthieu Moy
2015-07-11  5:54             ` Karthik Nayak
2015-07-09 10:27   ` [PATCH v2 05/10] ref-filter: add option to match literal pattern Karthik Nayak
2015-07-09 13:32     ` Matthieu Moy
2015-07-10 11:11       ` Karthik Nayak
2015-07-10 16:43     ` Junio C Hamano
2015-07-11  5:55       ` Karthik Nayak
2015-07-11  9:26         ` Matthieu Moy
2015-07-11 12:54           ` Karthik Nayak
2015-07-09 10:27   ` [PATCH v2 06/10] Documentation/tag: remove double occurance of "<pattern>" Karthik Nayak
2015-07-09 12:19     ` Christian Couder
2015-07-09 12:56       ` Karthik Nayak
2015-07-10 16:44       ` Junio C Hamano
2015-07-12 12:39         ` Karthik Nayak
2015-07-09 10:27   ` [PATCH v2 07/10] tag.c: use 'ref-filter' data structures Karthik Nayak
2015-07-09 10:55   ` [PATCH v2 08/10] tag.c: use 'ref-filter' APIs Karthik Nayak
2015-07-09 12:48     ` Matthieu Moy
2015-07-09 12:55       ` Karthik Nayak
2015-07-09 13:43         ` Matthieu Moy
2015-07-10  9:41           ` Karthik Nayak
2015-07-09 13:41     ` Matthieu Moy
2015-07-09 10:58   ` Karthik Nayak
2015-07-12  9:45     ` Duy Nguyen
2015-07-12 19:36       ` Karthik Nayak
2015-07-13 10:46         ` Duy Nguyen
2015-07-13 20:34           ` Karthik Nayak
2015-07-09 10:59   ` [PATCH v2 09/10] tag.c: implement '--format' option Karthik Nayak
2015-07-09 11:00   ` [PATCH v2 10/10] tag.c: implement '--merged' and '--no-merged' options Karthik Nayak
2015-07-09 12:58   ` [PATCH v2 01/10] ref-filter: add %(refname:shortalign=X) option Matthieu Moy
2015-07-11  6:07     ` Karthik Nayak
2015-07-11 10:20       ` Matthieu Moy
2015-07-10 16:20   ` Junio C Hamano
2015-07-11 12:05     ` Karthik Nayak
2015-07-12  1:47       ` Duy Nguyen [this message]
2015-07-12  8:59         ` Duy Nguyen
2015-07-12 19:56           ` Karthik Nayak
2015-07-13 10:51             ` Duy Nguyen
2015-07-13 20:36               ` 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=CACsJy8BsLnkPFX04WujMdnBiRGmXacF1QSQudb3T8nxGrZ_1Fg@mail.gmail.com \
    --to=pclouds@gmail.com \
    --cc=Matthieu.Moy@grenoble-inp.fr \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=karthik.188@gmail.com \
    --cc=mhagger@alum.mit.edu \
    /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).