On Tue, Jul 29, 2014 at 12:56:24PM -0700, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy  <pclo...@gmail.com> writes:
> > When utf8_width(&src) is called with *src == NULL (because the
> > source string ends with an ansi sequence),
> I am not sure what you mean by "because" here.  Do you mean somebody
> (who?) decides to call the utf8_width() with NULL pointer stored in
> *src because of "ansi sequence"?
> What do you mean by "ansi sequence"?  I'll assume that you mean
> those terminal control that all use bytes with hi-bit clear.

OK let's try with an example, strbuf_utf8_replace(&sb, 0, 0, "") where
"sb" contains "\033[m". The expected result is exactly the same string
with length of 3. After this block

                while ((n = display_mode_esc_sequence_len(src))) {
                        memcpy(dst, src, n);
                        src += n;
                        dst += n;

src will be right after 'm', *src is NUL (iow. src == end). After

                n = utf8_width((const char**)&src, NULL);

n is zero but 'src' is advanced by another character, so
src - old_src == 1. This NUL character is counted as part of the
string, so after the _setlen, we have the same string with length of
_4_ (instead of 3).

> At the very beginning of utf8_width(), *start can be cleared to
> point at a NULL pointer by pick_one_utf8_char() if the pointer that
> comes into utf8_width() originally points at an invalid UTF-8
> string, but as far as I can see, ESC (or any bytes that would be
> used in those terminal control sequences like colors and cursor
> control) will simply be returned as a single byte, without going
> into error path that clears *start = NULL.
> Puzzled...
> > it returns 0 and steps 'src' by one. 
> Here "it" refers to utf8_width()?  Who steps 'src' by one?

utf8_width() steps 'src'.

> Ahh, did you mean *src == NUL, i.e. "already at the end of the
> string"?

Yes.. I guess you have a better commit message prepared for me now :)

> I think utf8_width() called with an empty string should not move the
> pointer past that end-of-string NUL in the first place.  It makes me
> wonder if it would be a better fix to make it not to do that (and
> return 0), but if we declare it is the caller's fault, perhaps we
> may want to add
>       if (!**start)
>               die("BUG: empty string to utf8_width()???");
> at the very beginning of utf8_width(), even before it calls
> pick-one-utf8-char.
> Still puzzled...

I don't know. I mean, in a UTF-8 byte sequence, NUL is a valid
character (part of the ASCII subset), so advancing '*start' by one
makes sense. Whether we have any call sites crazy enough to do that on
purpose is a different matter.

> > This stepping makes strbuf_utf8_replace add NUL to the
> > destination string at the end of the loop. Check and break the loop
> > early.
> >
> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
> > ---
> >  utf8.c | 2 ++
> >  1 file changed, 2 insertions(+)
> Tests?

Ugh.. this was triggered by one of the alignment specifier in log
--pretty. Probably easier to export strbuf_utf8_replace() as a
test-command and verify the output?

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

Reply via email to