[PATCH v5 03/11] ref-filter: add option to pad atoms to the right
Add a new atom "padright" and support %(padright:X) where X is a
number. This will align the succeeding atom value to the left
followed by spaces for a total length of X characters. If X is less
than the item size, the entire atom value is printed.
Add tests and documentation for the same.
Helped-by: Duy Nguyen
Helped-by: Junio C Hamano
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
Documentation/git-for-each-ref.txt | 6 ++
ref-filter.c | 24
ref-filter.h | 4 +++-
t/t6302-for-each-ref-filter.sh | 16
4 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index e49d578..45dd7f8 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,12 @@ color::
Change output color. Followed by `:`, where names
are described in `color.branch.*`.
+padright::
+ Pad succeeding atom to the right. Followed by `:`,
+ where `value` states the total length of atom including the
+ padding. If the `value` is greater than the atom length, then
+ no padding is performed.
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index cc25c85..7ab34be 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -55,6 +55,7 @@ static struct {
{ "flag" },
{ "HEAD" },
{ "color" },
+ { "padright" },
};
/*
@@ -691,6 +692,18 @@ static void populate_value(struct ref_array_item *ref)
else
v->s = " ";
continue;
+ } else if (starts_with(name, "padright:")) {
+ const char *valp = NULL;
+
+ skip_prefix(name, "padright:", &valp);
+ if (!valp[0])
+ die(_("no value given with 'padright:'"));
+ if (strtoul_ui(valp, 10, (unsigned int *)&v->ul))
+ die(_("positive integer expected after ':' in
padright:%u\n"),
+ (unsigned int)v->ul);
+ v->pseudo_atom = 1;
+ v->pad_to_right = 1;
+ continue;
} else
continue;
@@ -1202,6 +1215,15 @@ static void ref_formatting(struct ref_formatting_state
*state,
free(state->color);
state->color = NULL;
}
+ if (state->pad_to_right) {
+ if (!is_utf8(v->s))
+ strbuf_addf(value, "%-*s", state->pad_to_right, v->s);
+ else {
+ int utf8_compensation = strlen(v->s) -
utf8_strwidth(v->s);
+ strbuf_addf(value, "%-*s", state->pad_to_right +
utf8_compensation, v->s);
+ }
+ return;
+ }
strbuf_addf(value, "%s", v->s);
}
@@ -1278,6 +1300,8 @@ static void apply_pseudo_state(struct
ref_formatting_state *state,
{
if (v->color)
state->color = (char *)v->s;
+ if (v->pad_to_right)
+ state->pad_to_right = v->ul;
}
void show_ref_array_item(struct ref_array_item *info, const char *format, int
quote_style)
diff --git a/ref-filter.h b/ref-filter.h
index 7687879..63b8175 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -20,12 +20,14 @@ struct atom_value {
const char *s;
unsigned long ul; /* used for sorting when not FIELD_STR */
unsigned int pseudo_atom : 1, /* atoms which aren't placeholders for
ref attributes */
- color : 1;
+ color : 1,
+ pad_to_right : 1;
};
struct ref_formatting_state {
int quote_style;
char *color;
+ unsigned int pad_to_right;
};
struct ref_sorting {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 505a360..daaa27a 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -81,4 +81,20 @@ test_expect_success 'filtering with --contains' '
test_cmp expect actual
'
+test_expect_success 'padding to the right using `padright`' '
+ cat >expect <<-\EOF &&
+ refs/heads/master|
+ refs/heads/side |
+ refs/odd/spot|
+ refs/tags/double-tag |
+ refs/tags/four |
+ refs/tags/one|
+ refs/tags/signed-tag |
+ refs/tags/three |
+ refs/tags/two|
+ EOF
+ git for-each-ref --format="%(padright:25)%(refname)|" >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.4.6
--
To unsubscribe from this list: send the line "uns
Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right
Karthik Nayak writes: > --- a/t/t6302-for-each-ref-filter.sh > +++ b/t/t6302-for-each-ref-filter.sh > @@ -81,4 +81,20 @@ test_expect_success 'filtering with --contains' ' > test_cmp expect actual > ' > > +test_expect_success 'padding to the right using `padright`' ' > + cat >expect <<-\EOF && > + refs/heads/master| > + refs/heads/side | > + refs/odd/spot| > + refs/tags/double-tag | > + refs/tags/four | > + refs/tags/one| > + refs/tags/signed-tag | > + refs/tags/three | > + refs/tags/two| > + EOF > + git for-each-ref --format="%(padright:25)%(refname)|" >actual && > + test_cmp expect actual > +' > + > test_done See my remark on previous patch: this test is not sufficient. You do not only need to check that %(padright) is taken into account, but you also need to check that it is taken into account for the right atom and only this one. I'd suggest --format '%(refname)%(padright:25)|%(refname)|%(refname)|' Only the middle column should be padded. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right
Matthieu Moy writes: > See my remark on previous patch: this test is not sufficient. You do > not only need to check that %(padright) is taken into account, but you > also need to check that it is taken into account for the right atom and > only this one. I'd suggest > > --format '%(refname)%(padright:25)|%(refname)|%(refname)|' > > Only the middle column should be padded. This actually raises an interesting point. It is equally valid to view that format string as requesting to pad the first "|" with 24 spaces; in other words, %(padright:N) would apply to the next span, be it a literal string up to the next %(atom), or the %(atom) that comes immediately after it. The thing is, the above _might_ be an OK way to ask the middle refname to be padded, but I somehow find it very unnatural and unintuitive to expect that this: --format '%(padright:25)A Very Long Irrelevancy%(refname)' would do nothing to "A Very Long Irrelevancy" and affects the refname that comes much later in the format string. Or we could simply forbid certain atoms followed by an non-empty literal as an error. My preference between the three is "%(padright:4)", etc. to apply to the "next span", but I can live with "it is an error to pad-right a far-away %(atom)". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right
Junio C Hamano writes: > Matthieu Moy writes: > >> See my remark on previous patch: this test is not sufficient. You do >> not only need to check that %(padright) is taken into account, but you >> also need to check that it is taken into account for the right atom and >> only this one. I'd suggest >> >> --format '%(refname)%(padright:25)|%(refname)|%(refname)|' >> >> Only the middle column should be padded. > > This actually raises an interesting point. It is equally valid to > view that format string as requesting to pad the first "|" with 24 > spaces; in other words, %(padright:N) would apply to the next span, > be it a literal string up to the next %(atom), or the %(atom) that > comes immediately after it. For an arbitrary %(modifier), I'd agree, but in the particular case of %(padright), I think it only makes sense if applied to something of variable length. > The thing is, the above _might_ be an OK way to ask the middle > refname to be padded, but I somehow find it very unnatural and > unintuitive to expect that this: > > --format '%(padright:25)A Very Long Irrelevancy%(refname)' Yes, but on the other hand we already have: git log --format='%<|(50)A very long irrevlevancy|%an|' that pads/truncate %an. So, consistancy would dictate that Karthik's version is the right one. > My preference between the three is "%(padright:4)", etc. to apply to > the "next span", but I can live with "it is an error to pad-right > a far-away %(atom)". I think there are valid argument for the 3 versions. I'm fine with any of them, as long as there's a test that shows which one is picked. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right
On Mon, Jul 27, 2015 at 9:24 PM, Matthieu Moy wrote: > Junio C Hamano writes: > >> Matthieu Moy writes: >> >>> See my remark on previous patch: this test is not sufficient. You do >>> not only need to check that %(padright) is taken into account, but you >>> also need to check that it is taken into account for the right atom and >>> only this one. I'd suggest >>> >>> --format '%(refname)%(padright:25)|%(refname)|%(refname)|' >>> >>> Only the middle column should be padded. >> >> This actually raises an interesting point. It is equally valid to >> view that format string as requesting to pad the first "|" with 24 >> spaces; in other words, %(padright:N) would apply to the next span, >> be it a literal string up to the next %(atom), or the %(atom) that >> comes immediately after it. > > For an arbitrary %(modifier), I'd agree, but in the particular case of > %(padright), I think it only makes sense if applied to something of > variable length > I agree with Matthieu here, Other than atom values, any user defined string would be of known size, hence padding for such things would make no sense. >> The thing is, the above _might_ be an OK way to ask the middle >> refname to be padded, but I somehow find it very unnatural and >> unintuitive to expect that this: >> >> --format '%(padright:25)A Very Long Irrelevancy%(refname)' > > Yes, but on the other hand we already have: > > git log --format='%<|(50)A very long irrevlevancy|%an|' > > that pads/truncate %an. So, consistancy would dictate that Karthik's > version is the right one. Sorry but I didn't understand what you're trying to say here, Matthieu. > >> My preference between the three is "%(padright:4)", etc. to apply to >> the "next span", but I can live with "it is an error to pad-right >> a far-away %(atom)". > > I think there are valid argument for the 3 versions. I'm fine with any > of them, as long as there's a test that shows which one is picked. > Makes sense, thanks both of you :) -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right
On Mon, Jul 27, 2015 at 6:20 PM, Matthieu Moy wrote: > u > also need to check that it is taken into account for the right atom and > only this one. I'd suggest > > --format '%(refname)%(padright:25)|%(refname)|%(refname)|' I guess this is more accurate, Thanks. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right
Karthik Nayak writes: > On Mon, Jul 27, 2015 at 9:24 PM, Matthieu Moy > wrote: >> Yes, but on the other hand we already have: >> >> git log --format='%<|(50)A very long irrevlevancy|%an|' >> >> that pads/truncate %an. So, consistancy would dictate that Karthik's >> version is the right one. > > Sorry but I didn't understand what you're trying to say here, Matthieu. The "git log" equivalent of %(padright:N) is %<|(N), and it behaves the same way as your current implementation of %(padright) (except for the missing reset in your v5). So, if we want to be consistant with "git log", we should keep the "apply to next atom, even if it's far away in the format string" semantics. Note that consistancy is not the only criterion for choice, so I'm not saying we should absolutely do this, just that there's an argument in favor of it. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right
On Tue, Jul 28, 2015 at 12:17 AM, Matthieu Moy wrote: > Karthik Nayak writes: > >> On Mon, Jul 27, 2015 at 9:24 PM, Matthieu Moy >> wrote: >>> Yes, but on the other hand we already have: >>> >>> git log --format='%<|(50)A very long irrevlevancy|%an|' >>> >>> that pads/truncate %an. So, consistancy would dictate that Karthik's >>> version is the right one. >> >> Sorry but I didn't understand what you're trying to say here, Matthieu. > > The "git log" equivalent of %(padright:N) is %<|(N), and it behaves the > same way as your current implementation of %(padright) (except for the > missing reset in your v5). > > So, if we want to be consistant with "git log", we should keep the > "apply to next atom, even if it's far away in the format string" > semantics. > > Note that consistancy is not the only criterion for choice, so I'm not > saying we should absolutely do this, just that there's an argument in > favor of it. > I didn't know that, thanks, I think I'll let Junio make the call here :) -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right
Matthieu Moy writes: > Yes, but on the other hand we already have: > > git log --format='%<|(50)A very long irrevlevancy|%an|' > > that pads/truncate %an. So, consistancy would dictate that Karthik's > version is the right one. Interesting. Although that %<(50) looks simply a bug to me which we may want to fix. I wonder if there is a good reason why the above should not be spelled as --format="A very long irrelevancy%<(50)%an". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right
On Mon, Jul 27, 2015 at 3:27 AM, Karthik Nayak wrote: > Add a new atom "padright" and support %(padright:X) where X is a > number. This will align the succeeding atom value to the left > followed by spaces for a total length of X characters. If X is less > than the item size, the entire atom value is printed. > > Signed-off-by: Karthik Nayak > --- > diff --git a/Documentation/git-for-each-ref.txt > b/Documentation/git-for-each-ref.txt > index e49d578..45dd7f8 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -127,6 +127,12 @@ color:: > Change output color. Followed by `:`, where names > are described in `color.branch.*`. > > +padright:: > + Pad succeeding atom to the right. Followed by `:`, > + where `value` states the total length of atom including the > + padding. If the `value` is greater than the atom length, then > + no padding is performed. Isn't this backward? Don't you mean ... If the atom length is greater than `value`, then no padding is performed. ? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right
On Wed, Jul 29, 2015 at 3:11 AM, Eric Sunshine wrote: > On Mon, Jul 27, 2015 at 3:27 AM, Karthik Nayak wrote: >> Add a new atom "padright" and support %(padright:X) where X is a >> number. This will align the succeeding atom value to the left >> followed by spaces for a total length of X characters. If X is less >> than the item size, the entire atom value is printed. >> >> Signed-off-by: Karthik Nayak >> --- >> diff --git a/Documentation/git-for-each-ref.txt >> b/Documentation/git-for-each-ref.txt >> index e49d578..45dd7f8 100644 >> --- a/Documentation/git-for-each-ref.txt >> +++ b/Documentation/git-for-each-ref.txt >> @@ -127,6 +127,12 @@ color:: >> Change output color. Followed by `:`, where names >> are described in `color.branch.*`. >> >> +padright:: >> + Pad succeeding atom to the right. Followed by `:`, >> + where `value` states the total length of atom including the >> + padding. If the `value` is greater than the atom length, then >> + no padding is performed. > > Isn't this backward? Don't you mean > > ... If the atom length is greater than `value`, then > no padding is performed. > > ? Yes, silly mistake. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
