git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Philip Oakley <philipoakley@iee.email>
Cc: Taylor Blau <me@ttaylorr.com>, GitList <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	NSENGIYUMVA WILBERFORCE <nsengiyumvawilberforce@gmail.com>
Subject: Re: [PATCH 1/1] pretty-formats: add hard truncation, without ellipsis, options
Date: Sun, 30 Oct 2022 19:42:46 -0400	[thread overview]
Message-ID: <Y18L9k0vAw1CArjw@nand.local> (raw)
In-Reply-To: <85f56b56-74c5-ffe5-703f-ef0c1e6d3b45@iee.email>

On Sun, Oct 30, 2022 at 10:01:47PM +0000, Philip Oakley wrote:
> > If I remember correctly, strbuf_utf8_replace() supports taking NULL as
> > its last argument, though upon searching I couldn't find any callers
> > that behave that way. Can we use that instead of supplying the empty
> > string? If not, should we drop support for the NULL-as-last-argument?
>
> I wasalso  concerned about zero length strings but my brief look at the
> code indicated it could cope with a zero length string.
> The last param is `const char *subst`.
>
> I've just relooked at the code and it does have a
>
>      if (subst)
>         subst_len = strlen(subst);
>
> so it does look safe to pass NULL, though it would probably cause a
> pause for thought for readers, and isn't likely to be that much faster
> in these format scenarios.

I'm not worried at all about speed, I was just wondering if there was a
more designated helper for "truncate these many UTF-8 characters from
the end of a string".

It appears that passing NULL to that argument behaves the same as
passing "", so I was wondering if it would be clearer to use that. But
I'm fine either way. If there are no callers that pass NULL, then we
should consider something like the following:

--- 8< ---
diff --git a/utf8.c b/utf8.c
index de4ce5c0e6..e8813f64fe 100644
--- a/utf8.c
+++ b/utf8.c
@@ -361,10 +361,9 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width,
 	char *src = sb_src->buf;
 	char *end = src + sb_src->len;
 	char *dst;
-	int w = 0, subst_len = 0;
+	int w = 0, subst_len;

-	if (subst)
-		subst_len = strlen(subst);
+	subst_len = strlen(subst);
 	strbuf_grow(&sb_dst, sb_src->len + subst_len);
 	dst = sb_dst.buf;
--- >8 ---

Below the context there is another "if (subst)", but that one needs to
stay since we only perform the replacement once.

Thanks,
Taylor

  reply	other threads:[~2022-10-30 23:42 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-30 18:56 [PATCH 0/1] extend the truncating pretty formats Philip Oakley
2022-10-30 18:56 ` [PATCH 1/1] pretty-formats: add hard truncation, without ellipsis, options Philip Oakley
2022-10-30 19:23   ` Taylor Blau
2022-10-30 22:01     ` Philip Oakley
2022-10-30 23:42       ` Taylor Blau [this message]
2022-10-30 21:41 ` [PATCH 0/1] extend the truncating pretty formats Philip Oakley
2022-11-01 22:57 ` [PATCH v2 " Philip Oakley
2022-11-01 22:57   ` [PATCH v2 1/1] pretty-formats: add hard truncation, without ellipsis, options Philip Oakley
2022-11-01 23:05     ` Philip Oakley
2022-11-02  0:45       ` Taylor Blau
2022-11-02 12:08         ` [PATCH v3] " Philip Oakley
2022-11-12 14:36           ` [PATCH v4] " Philip Oakley
2022-11-21  0:34             ` Junio C Hamano
2022-11-21 18:10               ` Philip Oakley
2022-11-22  0:57                 ` Junio C Hamano
2022-11-23 14:26                   ` Philip Oakley
2022-11-25  7:11                     ` Junio C Hamano
2022-11-26 14:32                       ` Philip Oakley
2022-11-26 22:44                         ` Philip Oakley
2022-11-26 23:19                         ` Junio C Hamano
2022-11-28 13:39                           ` Philip Oakley
2022-11-29  0:18                             ` Junio C Hamano
2022-12-07  0:24                           ` Philip Oakley
2022-12-07  0:54                             ` Junio C Hamano
2023-01-19 18:18             ` [PATCH v5 0/5] Pretty formats: Clarify column alignment Philip Oakley
2023-01-19 18:18               ` [PATCH v5 1/5] doc: pretty-formats: separate parameters from placeholders Philip Oakley
2023-01-19 18:18               ` [PATCH v5 2/5] doc: pretty-formats: delineate `%<|(` parameter values Philip Oakley
2023-01-19 18:18               ` [PATCH v5 3/5] doc: pretty-formats document negative column alignments Philip Oakley
2023-01-19 18:18               ` [PATCH v5 4/5] doc: pretty-formats describe use of ellipsis in truncation Philip Oakley
2023-01-19 18:18               ` [PATCH v5 5/5] doc: pretty-formats note wide char limitations, and add tests Philip Oakley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y18L9k0vAw1CArjw@nand.local \
    --to=me@ttaylorr.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=nsengiyumvawilberforce@gmail.com \
    --cc=philipoakley@iee.email \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).