Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options

[Jacob Keller <jacob.keller@gmail.com>]

On Wed, Dec 7, 2016 at 7:36 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> This is part of unification of the commands 'git tag -l, git branch -l
> and git for-each-ref'. This ports over branch.c to use ref-filter's
> printing options.
>
> Initially posted here: $(gmane/279226). It was decided that this series
> would follow up after refactoring ref-filter parsing mechanism, which
> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>
> v1 can be found here: $(gmane/288342)
> v2 can be found here: $(gmane/288863)
> v3 can be found here: $(gmane/290299)
> v4 can be found here: $(gmane/291106)
> v5b can be found here: $(gmane/292467)
> v6 can be found here: http://marc.info/?l=git&m=146330914118766&w=2
> v7 can be found here: http://marc.info/?l=git&m=147863593317362&w=2
>
> Changes in this version:
>
> 1. use an enum for holding the comparision type in
> %(if:[equals/notequals=...]) options.
> 2. rename the 'strip' option to 'lstrip' and introduce an 'rstrip'
> option. Also modify them to take negative values. This drops the
> ':dri' and ':base' options.
> 3. Drop unecessary code.
> 4. Cleanup code and fix spacing.
> 5. Add more comments wherever required.
> 6. Add quote_literal_for_format(const char *s) for safer string
> insertions in branch.c:build_format().
>
> Thanks to Jacob, Jackub, Junio and Matthieu for their inputs on the
> previous version.
>
> Interdiff below.
>
> Karthik Nayak (19):
>   ref-filter: implement %(if), %(then), and %(else) atoms
>   ref-filter: include reference to 'used_atom' within 'atom_value'
>   ref-filter: implement %(if:equals=<string>) and
>     %(if:notequals=<string>)
>   ref-filter: modify "%(objectname:short)" to take length
>   ref-filter: move get_head_description() from branch.c
>   ref-filter: introduce format_ref_array_item()
>   ref-filter: make %(upstream:track) prints "[gone]" for invalid
>     upstreams
>   ref-filter: add support for %(upstream:track,nobracket)
>   ref-filter: make "%(symref)" atom work with the ':short' modifier
>   ref-filter: introduce refname_atom_parser_internal()
>   ref-filter: introduce refname_atom_parser()
>   ref-filter: make remote_ref_atom_parser() use
>     refname_atom_parser_internal()
>   ref-filter: rename the 'strip' option to 'lstrip'
>   ref-filter: modify the 'lstrip=<N>' option to work with negative '<N>'
>   ref-filter: add an 'rstrip=<N>' option to atoms which deal with
>     refnames
>   ref-filter: allow porcelain to translate messages in the output
>   branch, tag: use porcelain output
>   branch: use ref-filter printing APIs
>   branch: implement '--format' option
>
>  Documentation/git-branch.txt       |   7 +-
>  Documentation/git-for-each-ref.txt |  86 +++++--
>  builtin/branch.c                   | 290 +++++++---------------
>  builtin/tag.c                      |   6 +-
>  ref-filter.c                       | 488 +++++++++++++++++++++++++++++++------
>  ref-filter.h                       |   7 +
>  t/t3203-branch-output.sh           |  16 +-
>  t/t6040-tracking-info.sh           |   2 +-
>  t/t6300-for-each-ref.sh            |  88 ++++++-
>  t/t6302-for-each-ref-filter.sh     |  94 +++++++
>  10 files changed, 784 insertions(+), 300 deletions(-)
>
> Interdiff:
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index f4ad297..c72baeb 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -92,13 +92,14 @@ refname::
>         The name of the ref (the part after $GIT_DIR/).
>         For a non-ambiguous short name of the ref append `:short`.
>         The option core.warnAmbiguousRefs is used to select the strict
> -       abbreviation mode. If `strip=<N>` is appended, strips `<N>`
> -       slash-separated path components from the front of the refname
> -       (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
> -       `<N>` must be a positive integer.  If a displayed ref has fewer
> -       components than `<N>`, the command aborts with an error. For the base
> -       directory of the ref (i.e. foo in refs/foo/bar/boz) append
> -       `:base`. For the entire directory path append `:dir`.
> +       abbreviation mode. If `lstrip=<N>` or `rstrip=<N>` option can

Grammar here, drop the If before `lstrip since you're referring to
multiples and you say "x can be appended to y" rather than "if x is
added, do y"

> +       be appended to strip `<N>` slash-separated path components
> +       from or end of the refname respectively (e.g.,
> +       `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo` and
> +       `%(refname:rstrip=2)` turns `refs/tags/foo` into `refs`).  if
> +       `<N>` is a negative number, then only `<N>` path components
> +       are left behind.  If a displayed ref has fewer components than
> +       `<N>`, the command aborts with an error.
>

Would it make more sense to not die and instead just return the empty
string? On the one hand, if we die() it's obvious that you tried to
strip too many components. But on the other hand, it's also somewhat
annoying to have the whole command fail because we happen upon a
single ref that has fewer components?

So, for positive numbers, we simply strip what we can, which may
result in the empty string, and for negative numbers, we keep up to
what we said, while potentially keeping the entire string. I feel
that's a better alternative than a die() in the middle of a ref
filter..

What are other people's thoughts on this?

Thanks,
Jake