Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
On Sun, Aug 9, 2015 at 1:49 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Sun, Aug 9, 2015 at 4:09 AM, Karthik Nayak karthik@gmail.com wrote:
On Sun, Aug 9, 2015 at 1:34 PM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak karthik@gmail.com wrote:
On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
That raises another question. Why are 'struct ref_formatting_state',
'struct align', 'struct atom_value', etc. defined in ref-filter.h at
all? Aren't those private implementation details of ref-filter.c, or
do you expect other code to be using them?
I guess struct ref_formatting_state and struct align could be moved to
ref-filter.c. About struct atom_value its referenced by ref_array_item()
so any reader reading about this, would find it easier if atom_value()
is at the same place.
Do you expect callers ever to be manipulating or otherwise accessing
the atom_value of ref_array_item? If callers have no business mucking
with atom_value, then one option would be to simply forward declare
atom_value in the header:
struct atom_value;
struct ref_array_item {
...
struct atom_value *value;
...
};
which makes atom_value opaque to clients of ref-filter. The actual
declaration of atom_value would then be moved to ref-filter.c, thus
kept private.
Also the code that this was done in has been excepted into `next`
so either I send a new series for the same, or write a patch just to
move this from ref-filter.h to ref-filter.c. So what would you suggest?
To my eye, atom_value seems to encapsulate a bunch of state local to
and only meaningful to ref-filter's internal workings, so it doesn't
really belong in the public header. Assuming that you don't foresee
any callers ever needing to access the properties of atom_value, then
it might indeed be reasonable to introduce a patch which moves it from
the .h file to the .c file (while leaving only a forward declaration
in the .h file).
Thanks! will add it to the series.
--
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 v9 03/11] ref-filter: implement an `align` atom
On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak karthik@gmail.com wrote:
On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com wrote:
of the padding to be performed. If the atom length is more than the
padding length then no padding is performed. e.g. to pad a succeeding
atom to the middle with a total padding size of 40 we can do a
It's odd to have alignment described in terms of padding and
padding length, especially in the case of center alignment. It
might be better to rephrase the discussion in terms of field width or
such.
--format=%(align:middle,40)..
Ok this makes sense,
I'll rephrase as :
`width` is the total length of the content with alignment.
This doesn't really make sense. width isn't the content length; it's
the size of the area into which the content will be placed.
Will change this.
If the atom length is more than the width then no alignment is performed.
What atom? I think you mean the content between %(align:) and %(end)
rather than atom. The description might be clearer if you actually
say content between %(align:) and %(end) to indicate specifically
what is being aligned.
Yes, that's what I meant.
e.g. to align a succeeding atom to the middle with a total width of 40
we can do:
--format=%(align:middle,40)..
@@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
else
v-s = ;
continue;
+ } else if (starts_with(name, align:)) {
+ const char *valp = NULL;
Unnecessary NULL assignment.
Thats required for the second skip_prefix and so on.
Else we get: warning: ‘valp’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
Okay, so that's because skip_prefix() is inline, and it doesn't touch
its out argument unless it actually skips the prefix. Ugly, but
makes sense, although I think this issue would go away if you combined
the starts_with() and skips_prefix() as suggested earlier.
Okay then I'll declare valp prehand to get rid of this and also to
remove redundant, starts_with() and skip_prefix().
+ struct align *align = xmalloc(sizeof(struct
align));
+
+ skip_prefix(name, align:, valp);
You could simplify the code by combining this skip_prefix() with the
earlier starts_with() in the conditional:
} else if (skip_prefix(name, align:, valp)) {
struct align *align = xmalloc(sizeof(struct align));
...
That would require valp to be previously defined. Hence the split.
Yes, it would require declaring 'valp' earlier, but that seems a
reasonable tradeoff for cleaner, simpler, less redundant code.
Yes. will do this.
static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
{
- /* More formatting options to be evetually added */
+ if (state-align state-end) {
+ struct strbuf *value = state-output;
+ int len = 0, buf_len = value-len;
+ struct align *align = state-align;
+
+ if (!value-buf)
+ return;
+ if (!is_utf8(value-buf)) {
+ len = value-len - utf8_strwidth(value-buf);
What is this doing, exactly? If the string is *not* utf-8, then you're
asking it for its utf-8 width. Am I reading that correctly?
Also, what is 'len' supposed to represent? I guess you want it to be
the difference between the byte length and the display length, but the
name 'len' doesn't convey that at all, nor does it help the reader
understand the code below where you do the actual formatting.
In fact, if I'm reading this correctly, then 'len' is always zero in
your tests (because the tests never trigger this conditional), so this
functionality is never being exercised.
There shouldn't be a ! there, will change.
I guess 'utf8_compensation' would be a better name.
Definitely better than 'len'.
+ else if (align-align_type == ALIGN_MIDDLE) {
+ int right = (align-align_value - buf_len)/2;
+ strbuf_addf(final, %*s%-*s, align-align_value -
right + len,
+ value-buf, right, );
An aesthetic aside: When (align_value - buf_len) is an odd number,
this implementation favors placing more whitespace to the left of the
string, and less to the right. In practice, this often tends to look a
bit more awkward than the inverse of placing more whitespace to the
right, and less to the left (but that again is subjective).
I know that, maybe we could add an additional padding to even out the value
given?
I don't understand your question. I was merely suggesting (purely
subjectively), for the odd length
Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
Le 8 août 2015 09:03:03 GMT+02:00, Karthik Nayak karthik@gmail.com a écrit : On Fri, Aug 7, 2015 at 9:34 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com wrote: Implement an `align` atom which will act as a modifier atom and align any string with or without an %(atom) appearing before a %(end) atom to the right, left or middle. It is followed by `:type,paddinglength`, where the `type` is either left, right or middle and `paddinglength` is the total length of the padding to be performed. If the atom length is more than the padding length then no padding is performed. e.g. to pad a succeeding atom to the middle with a total padding size of 40 we can do a --format=%(align:middle,40).. Add documentation and tests for the same. I forgot to mention in my earlier review of this patch that you should explain in the commit message, and probably the documentation, this this implementation (assuming I'm understanding the code) does not correctly support nested %(foo)...%(end) constructs, where %(foo) might be %(if:), %(truncate:), %(cut:), or even a nested %(align:), or some as yet unimagined modifier. Supporting nesting of these constructs will require pushing the formatting states onto a stack (or invoking the parser recursively). Signed-off-by: Karthik Nayak karthik@gmail.com Good point, I have been working on this parallely and it works for now, I'll send that with the %(if) and %(end) feature. But for now, it should be documented and added in the commit message. Using a linked list of sorts where whenever a new modifier atom is encountered a new state is created, and once %(end) is encountered we can pop that state into the previous state. Good, but keep in mind that this is not needed to port tag/branch, and your GSoC end soon. So keep your priorities in mind... IMHO, the nestable implementation can wait. Cheers, -- 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 v9 03/11] ref-filter: implement an `align` atom
On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak karthik@gmail.com wrote:
On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak karthik@gmail.com wrote:
On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com
wrote:
+ else if (align-align_type == ALIGN_MIDDLE) {
+ int right = (align-align_value - buf_len)/2;
+ strbuf_addf(final, %*s%-*s, align-align_value
- right + len,
+ value-buf, right, );
An aesthetic aside: When (align_value - buf_len) is an odd number,
this implementation favors placing more whitespace to the left of the
string, and less to the right. In practice, this often tends to look a
bit more awkward than the inverse of placing more whitespace to the
right, and less to the left (but that again is subjective).
I know that, maybe we could add an additional padding to even out the value
given?
I don't understand your question. I was merely suggesting (purely
subjectively), for the odd length case, putting the extra space
after the centered text rather than before it. For instance:
int left = (align-align_value - buf_len) / 2;
strbuf_addf(final, %*s%-*s, left, ,
align-align_value - left + len, value-buf);
or any similar variation which would give the same result.
I get this could be done, what I was asking was, Consider given a alignment
width of 25 would be better to make that 26 so that we have even padding on
both sides. But I don't like the adding of manipulating user given data.
I thought you might be asking that, but wasn't certain. I do agree
with your conclusion that second-guessing the user is a bad idea, and
that you should give the user exactly what was requested.
That raises another question. Why are 'struct ref_formatting_state',
'struct align', 'struct atom_value', etc. defined in ref-filter.h at
all? Aren't those private implementation details of ref-filter.c, or
do you expect other code to be using them?
I guess struct ref_formatting_state and struct align could be moved to
ref-filter.c. About struct atom_value its referenced by ref_array_item()
so any reader reading about this, would find it easier if atom_value()
is at the same place.
Do you expect callers ever to be manipulating or otherwise accessing
the atom_value of ref_array_item? If callers have no business mucking
with atom_value, then one option would be to simply forward declare
atom_value in the header:
struct atom_value;
struct ref_array_item {
...
struct atom_value *value;
...
};
which makes atom_value opaque to clients of ref-filter. The actual
declaration of atom_value would then be moved to ref-filter.c, thus
kept private.
--
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 v9 03/11] ref-filter: implement an `align` atom
On Sun, Aug 9, 2015 at 1:34 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak karthik@gmail.com wrote:
On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak karthik@gmail.com wrote:
On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com
wrote:
+ else if (align-align_type == ALIGN_MIDDLE) {
+ int right = (align-align_value - buf_len)/2;
+ strbuf_addf(final, %*s%-*s, align-align_value
- right + len,
+ value-buf, right, );
An aesthetic aside: When (align_value - buf_len) is an odd number,
this implementation favors placing more whitespace to the left of the
string, and less to the right. In practice, this often tends to look a
bit more awkward than the inverse of placing more whitespace to the
right, and less to the left (but that again is subjective).
I know that, maybe we could add an additional padding to even out the value
given?
I don't understand your question. I was merely suggesting (purely
subjectively), for the odd length case, putting the extra space
after the centered text rather than before it. For instance:
int left = (align-align_value - buf_len) / 2;
strbuf_addf(final, %*s%-*s, left, ,
align-align_value - left + len, value-buf);
or any similar variation which would give the same result.
I get this could be done, what I was asking was, Consider given a alignment
width of 25 would be better to make that 26 so that we have even padding on
both sides. But I don't like the adding of manipulating user given data.
I thought you might be asking that, but wasn't certain. I do agree
with your conclusion that second-guessing the user is a bad idea, and
that you should give the user exactly what was requested.
In that case I'll be doing what you suggested, thanks :)
That raises another question. Why are 'struct ref_formatting_state',
'struct align', 'struct atom_value', etc. defined in ref-filter.h at
all? Aren't those private implementation details of ref-filter.c, or
do you expect other code to be using them?
I guess struct ref_formatting_state and struct align could be moved to
ref-filter.c. About struct atom_value its referenced by ref_array_item()
so any reader reading about this, would find it easier if atom_value()
is at the same place.
Do you expect callers ever to be manipulating or otherwise accessing
the atom_value of ref_array_item? If callers have no business mucking
with atom_value, then one option would be to simply forward declare
atom_value in the header:
struct atom_value;
struct ref_array_item {
...
struct atom_value *value;
...
};
which makes atom_value opaque to clients of ref-filter. The actual
declaration of atom_value would then be moved to ref-filter.c, thus
kept private.
Also the code that this was done in has been excepted into `next`
so either I send a new series for the same, or write a patch just to
move this from ref-filter.h to ref-filter.c. So what would you suggest?
--
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 v9 03/11] ref-filter: implement an `align` atom
On Sun, Aug 9, 2015 at 1:30 PM, Matthieu Moy matthieu@imag.fr wrote: Le 8 août 2015 09:03:03 GMT+02:00, Karthik Nayak karthik@gmail.com a écrit : On Fri, Aug 7, 2015 at 9:34 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com wrote: Implement an `align` atom which will act as a modifier atom and align any string with or without an %(atom) appearing before a %(end) atom to the right, left or middle. It is followed by `:type,paddinglength`, where the `type` is either left, right or middle and `paddinglength` is the total length of the padding to be performed. If the atom length is more than the padding length then no padding is performed. e.g. to pad a succeeding atom to the middle with a total padding size of 40 we can do a --format=%(align:middle,40).. Add documentation and tests for the same. I forgot to mention in my earlier review of this patch that you should explain in the commit message, and probably the documentation, this this implementation (assuming I'm understanding the code) does not correctly support nested %(foo)...%(end) constructs, where %(foo) might be %(if:), %(truncate:), %(cut:), or even a nested %(align:), or some as yet unimagined modifier. Supporting nesting of these constructs will require pushing the formatting states onto a stack (or invoking the parser recursively). Signed-off-by: Karthik Nayak karthik@gmail.com Good point, I have been working on this parallely and it works for now, I'll send that with the %(if) and %(end) feature. But for now, it should be documented and added in the commit message. Using a linked list of sorts where whenever a new modifier atom is encountered a new state is created, and once %(end) is encountered we can pop that state into the previous state. Good, but keep in mind that this is not needed to port tag/branch, and your GSoC end soon. So keep your priorities in mind... IMHO, the nestable implementation can wait. Cheers, Agreed, but it was just something I had already worked on. Probably will push that series after GSoC :) -- 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 v9 03/11] ref-filter: implement an `align` atom
On Sun, Aug 9, 2015 at 4:09 AM, Karthik Nayak karthik@gmail.com wrote:
On Sun, Aug 9, 2015 at 1:34 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak karthik@gmail.com wrote:
On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
That raises another question. Why are 'struct ref_formatting_state',
'struct align', 'struct atom_value', etc. defined in ref-filter.h at
all? Aren't those private implementation details of ref-filter.c, or
do you expect other code to be using them?
I guess struct ref_formatting_state and struct align could be moved to
ref-filter.c. About struct atom_value its referenced by ref_array_item()
so any reader reading about this, would find it easier if atom_value()
is at the same place.
Do you expect callers ever to be manipulating or otherwise accessing
the atom_value of ref_array_item? If callers have no business mucking
with atom_value, then one option would be to simply forward declare
atom_value in the header:
struct atom_value;
struct ref_array_item {
...
struct atom_value *value;
...
};
which makes atom_value opaque to clients of ref-filter. The actual
declaration of atom_value would then be moved to ref-filter.c, thus
kept private.
Also the code that this was done in has been excepted into `next`
so either I send a new series for the same, or write a patch just to
move this from ref-filter.h to ref-filter.c. So what would you suggest?
To my eye, atom_value seems to encapsulate a bunch of state local to
and only meaningful to ref-filter's internal workings, so it doesn't
really belong in the public header. Assuming that you don't foresee
any callers ever needing to access the properties of atom_value, then
it might indeed be reasonable to introduce a patch which moves it from
the .h file to the .c file (while leaving only a forward declaration
in the .h file).
--
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 v9 03/11] ref-filter: implement an `align` atom
On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com wrote:
Implement an `align` atom which will act as a modifier atom and align
any string with or without an %(atom) appearing before a %(end) atom
to the right, left or middle.
For someone not familiar with the evolution of this patch series,
align any string with or without an %(atom) is confusing and
distracting and seems to imply that something extra mysterious is
going on behind the scenes. Keeping it simple makes it easier to
understand:
Add an `align` atom which left-, middle-, or right-aligns the
content between %(align:...) and %(end).
Ok will change this.
It is followed by `:type,paddinglength`, where the `type` is
'type' may not be the best name. Perhaps 'style' or 'position' or
something else would be better?
position is a better term.
either left, right or middle and `paddinglength` is the total length
'paddinglength' is misleading. You're really describing the container
width in which alignment takes places, so perhaps call it 'width' or
'fieldwidth' or something.
width seems good to go.
of the padding to be performed. If the atom length is more than the
padding length then no padding is performed. e.g. to pad a succeeding
atom to the middle with a total padding size of 40 we can do a
It's odd to have alignment described in terms of padding and
padding length, especially in the case of center alignment. It
might be better to rephrase the discussion in terms of field width or
such.
--format=%(align:middle,40)..
Ok this makes sense,
I'll rephrase as :
`width` is the total length of the content with alignment.
If the atom length is more than the width then no alignment is performed.
e.g. to align a succeeding atom to the middle with a total width of 40
we can do:
--format=%(align:middle,40)..
I may have missed the discussion, but why was comma chosen as a
separator here, rather than, say, colon? For instance:
%(align:middle:40)
I think it's based of this:
http://thread.gmane.org/gmane.comp.version-control.git/275119
Add documentation and tests for the same.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index e49d578..d865f98 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,14 @@ color::
+align::
+ Align any string with or without %(atom) before the %(end)
+ atom to the right, left or middle. Followed by
Ditto regarding any string with or without %(atom) being confusing
to someone not familiar with the evolution of this patch series.
Instead, perhaps:
Left-, middle-, or right-align content between %(align:...)
and %(end).
Will change as per change in the commit message.
+ `:type,paddinglength`, where the `type` is either left,
+ right or middle and `paddinglength` is the total length of
+ the padding to be performed. If the string length is more than
+ the padding length then no padding is performed.
Ditto regarding above observations about 'type', 'paddinglength', and
padding.
Noted.
diff --git a/ref-filter.c b/ref-filter.c
index 2c074a1..d123299 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)
const char *name = used_atom[i];
struct atom_value *v = ref-value[i];
int deref = 0;
- const char *refname;
+ const char *refname = NULL;
What is this change about?
Was to remove the compiler error, can be removed now.
const char *formatp;
struct branch *branch = NULL;
@@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
else
v-s = ;
continue;
+ } else if (starts_with(name, align:)) {
+ const char *valp = NULL;
Unnecessary NULL assignment.
Thats required for the second skip_prefix and so on.
Else we get: warning: ‘valp’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
+ struct align *align = xmalloc(sizeof(struct align));
+
+ skip_prefix(name, align:, valp);
You could simplify the code by combining this skip_prefix() with the
earlier starts_with() in the conditional:
} else if (skip_prefix(name, align:, valp)) {
struct align *align = xmalloc(sizeof(struct align));
...
That would require valp to be previously defined. Hence the split.
+ if (skip_prefix(valp, left,, valp))
+ align-align_type = ALIGN_LEFT;
+ else if (skip_prefix(valp, right,, valp))
+
Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
On Fri, Aug 7, 2015 at 9:34 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com wrote: Implement an `align` atom which will act as a modifier atom and align any string with or without an %(atom) appearing before a %(end) atom to the right, left or middle. It is followed by `:type,paddinglength`, where the `type` is either left, right or middle and `paddinglength` is the total length of the padding to be performed. If the atom length is more than the padding length then no padding is performed. e.g. to pad a succeeding atom to the middle with a total padding size of 40 we can do a --format=%(align:middle,40).. Add documentation and tests for the same. I forgot to mention in my earlier review of this patch that you should explain in the commit message, and probably the documentation, this this implementation (assuming I'm understanding the code) does not correctly support nested %(foo)...%(end) constructs, where %(foo) might be %(if:), %(truncate:), %(cut:), or even a nested %(align:), or some as yet unimagined modifier. Supporting nesting of these constructs will require pushing the formatting states onto a stack (or invoking the parser recursively). Signed-off-by: Karthik Nayak karthik@gmail.com Good point, I have been working on this parallely and it works for now, I'll send that with the %(if) and %(end) feature. But for now, it should be documented and added in the commit message. Using a linked list of sorts where whenever a new modifier atom is encountered a new state is created, and once %(end) is encountered we can pop that state into the previous state. -- 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 v9 03/11] ref-filter: implement an `align` atom
On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak karthik@gmail.com wrote:
On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com wrote:
of the padding to be performed. If the atom length is more than the
padding length then no padding is performed. e.g. to pad a succeeding
atom to the middle with a total padding size of 40 we can do a
It's odd to have alignment described in terms of padding and
padding length, especially in the case of center alignment. It
might be better to rephrase the discussion in terms of field width or
such.
--format=%(align:middle,40)..
Ok this makes sense,
I'll rephrase as :
`width` is the total length of the content with alignment.
This doesn't really make sense. width isn't the content length; it's
the size of the area into which the content will be placed.
If the atom length is more than the width then no alignment is performed.
What atom? I think you mean the content between %(align:) and %(end)
rather than atom. The description might be clearer if you actually
say content between %(align:) and %(end) to indicate specifically
what is being aligned.
e.g. to align a succeeding atom to the middle with a total width of 40
we can do:
--format=%(align:middle,40)..
@@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
else
v-s = ;
continue;
+ } else if (starts_with(name, align:)) {
+ const char *valp = NULL;
Unnecessary NULL assignment.
Thats required for the second skip_prefix and so on.
Else we get: warning: ‘valp’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
Okay, so that's because skip_prefix() is inline, and it doesn't touch
its out argument unless it actually skips the prefix. Ugly, but
makes sense, although I think this issue would go away if you combined
the starts_with() and skips_prefix() as suggested earlier.
+ struct align *align = xmalloc(sizeof(struct align));
+
+ skip_prefix(name, align:, valp);
You could simplify the code by combining this skip_prefix() with the
earlier starts_with() in the conditional:
} else if (skip_prefix(name, align:, valp)) {
struct align *align = xmalloc(sizeof(struct align));
...
That would require valp to be previously defined. Hence the split.
Yes, it would require declaring 'valp' earlier, but that seems a
reasonable tradeoff for cleaner, simpler, less redundant code.
static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
{
- /* More formatting options to be evetually added */
+ if (state-align state-end) {
+ struct strbuf *value = state-output;
+ int len = 0, buf_len = value-len;
+ struct align *align = state-align;
+
+ if (!value-buf)
+ return;
+ if (!is_utf8(value-buf)) {
+ len = value-len - utf8_strwidth(value-buf);
What is this doing, exactly? If the string is *not* utf-8, then you're
asking it for its utf-8 width. Am I reading that correctly?
Also, what is 'len' supposed to represent? I guess you want it to be
the difference between the byte length and the display length, but the
name 'len' doesn't convey that at all, nor does it help the reader
understand the code below where you do the actual formatting.
In fact, if I'm reading this correctly, then 'len' is always zero in
your tests (because the tests never trigger this conditional), so this
functionality is never being exercised.
There shouldn't be a ! there, will change.
I guess 'utf8_compensation' would be a better name.
Definitely better than 'len'.
+ else if (align-align_type == ALIGN_MIDDLE) {
+ int right = (align-align_value - buf_len)/2;
+ strbuf_addf(final, %*s%-*s, align-align_value -
right + len,
+ value-buf, right, );
An aesthetic aside: When (align_value - buf_len) is an odd number,
this implementation favors placing more whitespace to the left of the
string, and less to the right. In practice, this often tends to look a
bit more awkward than the inverse of placing more whitespace to the
right, and less to the left (but that again is subjective).
I know that, maybe we could add an additional padding to even out the value
given?
I don't understand your question. I was merely suggesting (purely
subjectively), for the odd length case, putting the extra space
after the centered text rather than before it. For instance:
int left = (align-align_value - buf_len) / 2;
strbuf_addf(final, %*s%-*s, left, ,
align-align_value - left + len, value-buf);
or any similar variation which would give the same
Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com wrote: Implement an `align` atom which will act as a modifier atom and align any string with or without an %(atom) appearing before a %(end) atom to the right, left or middle. It is followed by `:type,paddinglength`, where the `type` is either left, right or middle and `paddinglength` is the total length of the padding to be performed. If the atom length is more than the padding length then no padding is performed. e.g. to pad a succeeding atom to the middle with a total padding size of 40 we can do a --format=%(align:middle,40).. Add documentation and tests for the same. I forgot to mention in my earlier review of this patch that you should explain in the commit message, and probably the documentation, this this implementation (assuming I'm understanding the code) does not correctly support nested %(foo)...%(end) constructs, where %(foo) might be %(if:), %(truncate:), %(cut:), or even a nested %(align:), or some as yet unimagined modifier. Supporting nesting of these constructs will require pushing the formatting states onto a stack (or invoking the parser recursively). Signed-off-by: Karthik Nayak karthik@gmail.com -- 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 v9 03/11] ref-filter: implement an `align` atom
On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak karthik@gmail.com wrote:
Implement an `align` atom which will act as a modifier atom and align
any string with or without an %(atom) appearing before a %(end) atom
to the right, left or middle.
For someone not familiar with the evolution of this patch series,
align any string with or without an %(atom) is confusing and
distracting and seems to imply that something extra mysterious is
going on behind the scenes. Keeping it simple makes it easier to
understand:
Add an `align` atom which left-, middle-, or right-aligns the
content between %(align:...) and %(end).
It is followed by `:type,paddinglength`, where the `type` is
'type' may not be the best name. Perhaps 'style' or 'position' or
something else would be better?
either left, right or middle and `paddinglength` is the total length
'paddinglength' is misleading. You're really describing the container
width in which alignment takes places, so perhaps call it 'width' or
'fieldwidth' or something.
of the padding to be performed. If the atom length is more than the
padding length then no padding is performed. e.g. to pad a succeeding
atom to the middle with a total padding size of 40 we can do a
It's odd to have alignment described in terms of padding and
padding length, especially in the case of center alignment. It
might be better to rephrase the discussion in terms of field width or
such.
--format=%(align:middle,40)..
I may have missed the discussion, but why was comma chosen as a
separator here, rather than, say, colon? For instance:
%(align:middle:40)
Add documentation and tests for the same.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index e49d578..d865f98 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,14 @@ color::
+align::
+ Align any string with or without %(atom) before the %(end)
+ atom to the right, left or middle. Followed by
Ditto regarding any string with or without %(atom) being confusing
to someone not familiar with the evolution of this patch series.
Instead, perhaps:
Left-, middle-, or right-align content between %(align:...)
and %(end).
+ `:type,paddinglength`, where the `type` is either left,
+ right or middle and `paddinglength` is the total length of
+ the padding to be performed. If the string length is more than
+ the padding length then no padding is performed.
Ditto regarding above observations about 'type', 'paddinglength', and padding.
diff --git a/ref-filter.c b/ref-filter.c
index 2c074a1..d123299 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)
const char *name = used_atom[i];
struct atom_value *v = ref-value[i];
int deref = 0;
- const char *refname;
+ const char *refname = NULL;
What is this change about?
const char *formatp;
struct branch *branch = NULL;
@@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
else
v-s = ;
continue;
+ } else if (starts_with(name, align:)) {
+ const char *valp = NULL;
Unnecessary NULL assignment.
+ struct align *align = xmalloc(sizeof(struct align));
+
+ skip_prefix(name, align:, valp);
You could simplify the code by combining this skip_prefix() with the
earlier starts_with() in the conditional:
} else if (skip_prefix(name, align:, valp)) {
struct align *align = xmalloc(sizeof(struct align));
...
+ if (skip_prefix(valp, left,, valp))
+ align-align_type = ALIGN_LEFT;
+ else if (skip_prefix(valp, right,, valp))
+ align-align_type = ALIGN_RIGHT;
+ else if (skip_prefix(valp, middle,, valp))
+ align-align_type = ALIGN_MIDDLE;
+ else
+ die(_(align: improper format));
You could do a better job of helping the user track down the offending
improper format by actually including it in the error message.
+ if (strtoul_ui(valp, 10, align-align_value))
+ die(_(align: positive value expected));
Ditto.
+ v-align = align;
+ v-modifier_atom = 1;
+ continue;
+ } else if (starts_with(name, end)) {
+ v-end = 1;
+ v-modifier_atom = 1;
+ continue;
} else
