[RFC PATCH 0/3] Avoid passing global comment_line_char repeatedly

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 30, 2023, 20:22 UTC
Message-ID: <cover.1698696798.git.jonathantanmy@google.com>
In-Reply-To: <db6702ba-11a7-44c1-af2a-95b080aaeb77@gmail.com>

> While I agree with your reasoning here, I think that parameter was 
> recently added as part of the libification effort - I can't remember 
> exactly why and am too lazy to look it up so I've cc'd Calvin and 
> Johathan instead.

Thanks Phillip for noticing this. Putting on my libification hat, this was probably because we wanted to remove strbuf's dependency on environment, so that we wouldn't need to include it in git-std-lib. If we were to merge these patches, libification would probably still be doable if we stubbed the global comment_line_char.

Removing my libification hat, I think it's better to solve this issue by moving the functions into environment.{c,h} instead, following the example of functions like strbuf_worktree_ref() in worktree.h and strbuf_utf8_align() in utf8.h that, when operating on both strbuf and a specific domain, are placed in the domain's header file, not in strbuf.h. This avoids a situation in which strbuf.h contains everything string-related.

The main issue with this is that by not centralizing all strbuf-related functionality, some strbuf-related helper functions that could have been private now need to be made public, but I think that a similar issue would be faced if we don't centralize, say, all environment-related functionality (some environment-related helper functions would have to be made public, although I didn't encounter this problem with this patch set).

I've attached some patches to illustrate what I've described above.

Jonathan Tan (1):
  strbuf: make add_lines() public

Junio C Hamano (2):
  strbuf_commented_addf(): drop the comment_line_char parameter
  strbuf_add_commented_lines(): drop the comment_line_char parameter

 add-patch.c          |  8 ++---
 branch.c             |  3 +-
 builtin/branch.c     |  2 +-
 builtin/merge.c      |  8 ++---
 builtin/notes.c      |  9 +++---
 builtin/stripspace.c |  2 +-
 builtin/tag.c        |  4 +--
 commit.c             |  2 +-
 environment.c        | 31 ++++++++++++++++++++
 environment.h        | 14 +++++++++
 fmt-merge-msg.c      |  9 ++----
 rebase-interactive.c |  8 ++---
 sequencer.c          | 14 ++++-----
 strbuf.c             | 69 ++++++++++----------------------------------
 strbuf.h             | 19 ++----------
 wt-status.c          |  6 ++--
 16 files changed, 98 insertions(+), 110 deletions(-)

-- 
2.42.0.820.g83a721a137-goog