Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
From: Jacob Keller <hidden>
Date: 2016-12-08 00:02:15
On Wed, Dec 7, 2016 at 7:36 AM, Karthik Nayak [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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