From: Calvin Wan <calvinwan@google.com>
To: git@vger.kernel.org
Cc: Calvin Wan <calvinwan@google.com>,
newren@gmail.com, peff@peff.net, phillip.wood123@gmail.com,
sunshine@sunshineco.com
Subject: [PATCH v5 0/7] strbuf cleanups
Date: Thu, 11 May 2023 19:44:46 +0000 [thread overview]
Message-ID: <20230511194446.1492907-1-calvinwan@google.com> (raw)
In-Reply-To: <20230508165728.525603-1-calvinwan@google.com>
This reroll includes suggestions made by Eric and Phillip for better
documentation on patch 1 and removal of an extraneous parameter in patch 7.
Calvin Wan (7):
strbuf: clarify API boundary
abspath: move related functions to abspath
credential-store: move related functions to credential-store file
object-name: move related functions to object-name
path: move related function to path
strbuf: clarify dependency
strbuf: remove global variable
abspath.c | 36 ++++++++++++
abspath.h | 21 +++++++
add-patch.c | 12 ++--
builtin/am.c | 2 +-
builtin/branch.c | 4 +-
builtin/commit.c | 2 +-
builtin/credential-store.c | 19 +++++++
builtin/merge.c | 10 ++--
builtin/notes.c | 16 +++---
builtin/rebase.c | 2 +-
builtin/stripspace.c | 6 +-
builtin/tag.c | 9 ++-
fmt-merge-msg.c | 9 ++-
gpg-interface.c | 5 +-
hook.c | 1 +
object-name.c | 15 +++++
object-name.h | 9 +++
path.c | 20 +++++++
path.h | 5 ++
pretty.c | 1 +
rebase-interactive.c | 15 ++---
sequencer.c | 24 +++++---
strbuf.c | 112 ++++---------------------------------
strbuf.h | 59 ++++++-------------
tempfile.c | 1 +
wt-status.c | 6 +-
26 files changed, 229 insertions(+), 192 deletions(-)
Range-diff against v4:
1: e0dd3f5295 ! 1: 287e787c9c strbuf: clarify API boundary
@@ Commit message
strbuf: clarify API boundary
strbuf, as a generic and widely used structure across the codebase,
- should be limited as a libary to only interact with primitives. Add
- documentation so future functions can be appropriately be placed. Older
+ should be limited as a library to only interact with primitives. Add
+ documentation so future functions can appropriately be placed. Older
functions that do not follow this boundary should eventually be moved or
refactored.
## strbuf.h ##
-@@ strbuf.h: struct string_list;
+@@
+ #ifndef STRBUF_H
+ #define STRBUF_H
+
++/*
++ * NOTE FOR STRBUF DEVELOPERS
++ *
++ * The objects that this API interacts with should be limited to other
++ * primitives, however, there are older functions in here that interact
++ * with non-primitive objects which should eventually be moved out or
++ * refactored.
++ */
++
+ struct string_list;
/**
- * strbuf's are meant to be used with all the usual C string and memory
-- * APIs. Given that the length of the buffer is known, it's often better to
-+ * APIs. The objects that this API interacts with in this file should be
-+ * limited to other primitives, however, there are older functions in here
-+ * that should eventually be moved out or refactored.
-+ *
-+ * Given that the length of the buffer is known, it's often better to
- * use the mem* functions than a str* one (memchr vs. strchr e.g.).
- * Though, one has to be careful about the fact that str* functions often
- * stop on NULs and that strbufs may have embedded NULs.
2: 48fb5db28b = 2: 5bd27cad7a abspath: move related functions to abspath
3: a663f91819 = 3: 08e3e40de6 credential-store: move related functions to credential-store file
4: ccef9dd5f2 = 4: 1a21d4fdd1 object-name: move related functions to object-name
5: 0d6b9cf0f7 = 5: 03b20f3384 path: move related function to path
6: 5655c56a6d = 6: b55f4a39e1 strbuf: clarify dependency
7: 874d0efac3 ! 7: 6ea36d7730 strbuf: remove global variable
@@ Commit message
not utilize the comment_line_char global variable within its
functions. Therefore, add an additional parameter for functions that use
comment_line_char and refactor callers to pass it in instead.
+ strbuf_stripspace() removes the skip_comments boolean and checks if
+ comment_line_char is a non-NULL character to determine whether to skip
+ comments or not.
## add-patch.c ##
@@ add-patch.c: static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
@@ builtin/am.c: static int parse_mail(struct am_state *state, const char *mail)
strbuf_addstr(&msg, "\n\n");
strbuf_addbuf(&msg, &mi.log_message);
- strbuf_stripspace(&msg, 0);
-+ strbuf_stripspace(&msg, 0, comment_line_char);
++ strbuf_stripspace(&msg, '\0');
assert(!state->author_name);
state->author_name = strbuf_detach(&author_name, NULL);
@@ builtin/branch.c: static int edit_branch_description(const char *branch_name)
return -1;
}
- strbuf_stripspace(&buf, 1);
-+ strbuf_stripspace(&buf, 1, comment_line_char);
++ strbuf_stripspace(&buf, comment_line_char);
strbuf_addf(&name, "branch.%s.description", branch_name);
if (buf.len || exists)
@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const cha
if (clean_message_contents)
- strbuf_stripspace(&sb, 0);
-+ strbuf_stripspace(&sb, 0, comment_line_char);
++ strbuf_stripspace(&sb, '\0');
if (signoff)
append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
@@ builtin/notes.c: static void prepare_note_data(const struct object_id *object, s
die(_("please supply the note contents using either -m or -F option"));
}
- strbuf_stripspace(&d->buf, 1);
-+ strbuf_stripspace(&d->buf, 1, comment_line_char);
++ strbuf_stripspace(&d->buf, comment_line_char);
}
}
@@ builtin/notes.c: static int parse_msg_arg(const struct option *opt, const char *
strbuf_addch(&d->buf, '\n');
strbuf_addstr(&d->buf, arg);
- strbuf_stripspace(&d->buf, 0);
-+ strbuf_stripspace(&d->buf, 0, comment_line_char);
++ strbuf_stripspace(&d->buf, '\0');
d->given = 1;
return 0;
@@ builtin/notes.c: static int parse_file_arg(const struct option *opt, const char
} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)
die_errno(_("could not open or read '%s'"), arg);
- strbuf_stripspace(&d->buf, 0);
-+ strbuf_stripspace(&d->buf, 0, comment_line_char);
++ strbuf_stripspace(&d->buf, '\0');
d->given = 1;
return 0;
@@ builtin/rebase.c: static int edit_todo_file(unsigned flags)
return error_errno(_("could not read '%s'."), todo_file);
- strbuf_stripspace(&todo_list.buf, 1);
-+ strbuf_stripspace(&todo_list.buf, 1, comment_line_char);
++ strbuf_stripspace(&todo_list.buf, comment_line_char);
res = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);
if (!res && todo_list_write_to_file(the_repository, &new_todo, todo_file,
NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS)))
@@ builtin/stripspace.c: int cmd_stripspace(int argc, const char **argv, const char
if (mode == STRIP_DEFAULT || mode == STRIP_COMMENTS)
- strbuf_stripspace(&buf, mode == STRIP_COMMENTS);
-+ strbuf_stripspace(&buf, mode == STRIP_COMMENTS,
-+ comment_line_char);
++ strbuf_stripspace(&buf,
++ mode == STRIP_COMMENTS ? comment_line_char : '\0');
else
comment_lines(&buf);
@@ builtin/tag.c: static void create_tag(const struct object_id *object, const char
if (opt->cleanup_mode != CLEANUP_NONE)
- strbuf_stripspace(buf, opt->cleanup_mode == CLEANUP_ALL);
-+ strbuf_stripspace(buf, opt->cleanup_mode == CLEANUP_ALL,
-+ comment_line_char);
++ strbuf_stripspace(buf,
++ opt->cleanup_mode == CLEANUP_ALL ? comment_line_char : '\0');
if (!opt->message_given && !buf->len)
die(_("no tag message?"));
@@ gpg-interface.c: static int verify_ssh_signed_buffer(struct signature_check *sig
- strbuf_stripspace(&ssh_keygen_out, 0);
- strbuf_stripspace(&ssh_keygen_err, 0);
-+ strbuf_stripspace(&ssh_keygen_out, 0, comment_line_char);
-+ strbuf_stripspace(&ssh_keygen_err, 0, comment_line_char);
++ strbuf_stripspace(&ssh_keygen_out, '\0');
++ strbuf_stripspace(&ssh_keygen_err, '\0');
/* Add stderr outputs to show the user actual ssh-keygen errors */
strbuf_add(&ssh_keygen_out, ssh_principals_err.buf, ssh_principals_err.len);
strbuf_add(&ssh_keygen_out, ssh_keygen_err.buf, ssh_keygen_err.len);
@@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list
return -2;
- strbuf_stripspace(&new_todo->buf, 1);
-+ strbuf_stripspace(&new_todo->buf, 1, comment_line_char);
++ strbuf_stripspace(&new_todo->buf, comment_line_char);
if (initial && new_todo->buf.len == 0)
return -3;
@@ sequencer.c: void cleanup_message(struct strbuf *msgbuf,
strbuf_setlen(msgbuf, wt_status_locate_end(msgbuf->buf, msgbuf->len));
if (cleanup_mode != COMMIT_MSG_CLEANUP_NONE)
- strbuf_stripspace(msgbuf, cleanup_mode == COMMIT_MSG_CLEANUP_ALL);
-+ strbuf_stripspace(msgbuf, cleanup_mode == COMMIT_MSG_CLEANUP_ALL,
-+ comment_line_char);
++ strbuf_stripspace(msgbuf,
++ cleanup_mode == COMMIT_MSG_CLEANUP_ALL ? comment_line_char : '\0');
}
/*
@@ sequencer.c: int template_untouched(const struct strbuf *sb, const char *templat
return 0;
- strbuf_stripspace(&tmpl, cleanup_mode == COMMIT_MSG_CLEANUP_ALL);
-+ strbuf_stripspace(&tmpl, cleanup_mode == COMMIT_MSG_CLEANUP_ALL,
-+ comment_line_char);
++ strbuf_stripspace(&tmpl,
++ cleanup_mode == COMMIT_MSG_CLEANUP_ALL ? comment_line_char : '\0');
if (!skip_prefix(sb->buf, tmpl.buf, &start))
start = sb->buf;
strbuf_release(&tmpl);
@@ sequencer.c: static int try_to_commit(struct repository *r,
if (cleanup != COMMIT_MSG_CLEANUP_NONE)
- strbuf_stripspace(msg, cleanup == COMMIT_MSG_CLEANUP_ALL);
-+ strbuf_stripspace(msg, cleanup == COMMIT_MSG_CLEANUP_ALL,
-+ comment_line_char);
++ strbuf_stripspace(msg,
++ cleanup == COMMIT_MSG_CLEANUP_ALL ? comment_line_char : '\0');
if ((flags & EDIT_MSG) && message_is_empty(msg, cleanup)) {
res = 1; /* run 'git commit' to display error message */
goto out;
@@ strbuf.c: void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...)
sb->buf[--sb->len] = '\0';
@@ strbuf.c: static size_t cleanup(char *line, size_t len)
- * Enable skip_comments to skip every line starting with comment
- * character.
+ *
+ * If last line does not have a newline at the end, one is added.
+ *
+- * Enable skip_comments to skip every line starting with comment
+- * character.
++ * Pass a non-NULL comment_line_char to skip every line starting
++ * with it
*/
-void strbuf_stripspace(struct strbuf *sb, int skip_comments)
-+void strbuf_stripspace(struct strbuf *sb, int skip_comments,
-+ char comment_line_char)
++void strbuf_stripspace(struct strbuf *sb, char comment_line_char)
{
size_t empties = 0;
size_t i, j, len, newlen;
+@@ strbuf.c: void strbuf_stripspace(struct strbuf *sb, int skip_comments)
+ eol = memchr(sb->buf + i, '\n', sb->len - i);
+ len = eol ? eol - (sb->buf + i) + 1 : sb->len - i;
+
+- if (skip_comments && len && sb->buf[i] == comment_line_char) {
++ if (comment_line_char != '\0' && len &&
++ sb->buf[i] == comment_line_char) {
+ newlen = 0;
+ continue;
+ }
## strbuf.h ##
@@ strbuf.h: void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
@@ strbuf.h: void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
__attribute__((format (printf,2,0)))
void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
-@@ strbuf.h: int strbuf_normalize_path(struct strbuf *sb);
+@@ strbuf.h: int strbuf_getcwd(struct strbuf *sb);
+ int strbuf_normalize_path(struct strbuf *sb);
/**
- * Strip whitespace from a buffer. The second parameter controls if
+- * Strip whitespace from a buffer. The second parameter controls if
- * comments are considered contents to be removed or not.
-+ * comments are considered contents to be removed or not. The third parameter
-+ * is the comment character that determines whether a line is a comment or not.
++ * Strip whitespace from a buffer. The second parameter is the comment
++ * character that determines whether a line is a comment or not. If the
++ * second parameter is a non-NULL character, comments are considered
++ * contents to be removed.
*/
-void strbuf_stripspace(struct strbuf *buf, int skip_comments);
-+void strbuf_stripspace(struct strbuf *buf, int skip_comments, char comment_line_char);
++void strbuf_stripspace(struct strbuf *buf, char comment_line_char);
static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
{
--
2.40.1.606.ga4b1b128d6-goog
next prev parent reply other threads:[~2023-05-11 19:47 UTC|newest]
Thread overview: 85+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-02 21:14 [PATCH 0/6] strbuf cleanups Calvin Wan
2023-05-02 21:14 ` [PATCH 1/6] abspath: move related functions to abspath Calvin Wan
2023-05-02 21:42 ` Junio C Hamano
2023-05-02 21:14 ` [PATCH 2/6] credential-store: move related functions to credential-store file Calvin Wan
2023-05-02 21:52 ` Junio C Hamano
2023-05-03 16:28 ` Jeff King
2023-05-03 16:34 ` Jeff King
2023-05-03 18:38 ` Calvin Wan
2023-05-02 21:14 ` [PATCH 3/6] object-name: move related functions to object-name Calvin Wan
2023-05-02 21:14 ` [PATCH 4/6] path: move related function to path Calvin Wan
2023-05-02 21:14 ` [PATCH 5/6] strbuf: clarify dependency Calvin Wan
2023-05-03 1:56 ` Elijah Newren
2023-05-02 21:14 ` [PATCH 6/6] strbuf: remove environment variables Calvin Wan
2023-05-03 2:15 ` Elijah Newren
2023-05-02 22:20 ` [PATCH 0/6] strbuf cleanups Junio C Hamano
2023-05-02 22:31 ` Junio C Hamano
2023-05-02 23:51 ` Felipe Contreras
2023-05-02 22:36 ` Calvin Wan
2023-05-03 2:37 ` Elijah Newren
2023-05-03 18:00 ` Calvin Wan
2023-05-07 0:14 ` Elijah Newren
2023-05-07 13:14 ` Jeff King
2023-05-03 18:48 ` [PATCH v2 0/7] " Calvin Wan
2023-05-03 18:50 ` [PATCH v2 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-03 18:50 ` [PATCH v2 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-03 18:50 ` [PATCH v2 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-03 18:50 ` [PATCH v2 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-03 18:50 ` [PATCH v2 5/7] path: move related function to path Calvin Wan
2023-05-03 18:50 ` [PATCH v2 6/7] strbuf: clarify dependency Calvin Wan
2023-05-03 19:26 ` Junio C Hamano
2023-05-03 18:50 ` [PATCH v2 7/7] strbuf: remove environment variables Calvin Wan
2023-05-03 19:24 ` Junio C Hamano
2023-05-03 19:41 ` Calvin Wan
2023-05-03 19:45 ` Junio C Hamano
2023-05-03 19:42 ` [PATCH v3 7/7] strbuf: remove environment variable Calvin Wan
2023-05-05 22:33 ` [PATCH v2 0/7] strbuf cleanups Junio C Hamano
2023-05-08 16:38 ` Calvin Wan
2023-05-07 0:40 ` Elijah Newren
2023-05-07 21:47 ` Felipe Contreras
2023-05-08 16:57 ` [PATCH v4 " Calvin Wan
2023-05-08 16:59 ` [PATCH v4 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-08 17:22 ` Eric Sunshine
2023-05-10 22:51 ` Junio C Hamano
2023-05-08 16:59 ` [PATCH v4 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-08 16:59 ` [PATCH v4 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-08 16:59 ` [PATCH v4 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-08 16:59 ` [PATCH v4 5/7] path: move related function to path Calvin Wan
2023-05-08 16:59 ` [PATCH v4 6/7] strbuf: clarify dependency Calvin Wan
2023-05-08 16:59 ` [PATCH v4 7/7] strbuf: remove global variable Calvin Wan
2023-05-10 8:12 ` Phillip Wood
2023-05-09 1:57 ` [PATCH v4 0/7] strbuf cleanups Elijah Newren
2023-05-09 2:13 ` Felipe Contreras
2023-05-11 19:44 ` Calvin Wan [this message]
2023-05-11 19:48 ` [PATCH v5 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-11 19:57 ` Eric Sunshine
2023-05-11 20:03 ` Calvin Wan
2023-05-11 19:48 ` [PATCH v5 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-11 19:48 ` [PATCH v5 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-11 19:48 ` [PATCH v5 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-11 19:48 ` [PATCH v5 5/7] path: move related function to path Calvin Wan
2023-05-11 19:48 ` [PATCH v5 6/7] strbuf: clarify dependency Calvin Wan
2023-05-11 19:48 ` [PATCH v5 7/7] strbuf: remove global variable Calvin Wan
2023-05-11 20:24 ` Eric Sunshine
2023-05-11 21:42 ` Junio C Hamano
2023-05-12 14:54 ` Phillip Wood
2023-05-12 14:53 ` Phillip Wood
2023-05-12 19:31 ` Junio C Hamano
2023-05-12 17:14 ` [PATCH v6 0/7] strbuf cleanups Calvin Wan
2023-05-12 17:15 ` [PATCH v6 1/7] strbuf: clarify API boundary Calvin Wan
2023-05-12 17:15 ` [PATCH v6 2/7] abspath: move related functions to abspath Calvin Wan
2023-05-12 17:15 ` [PATCH v6 3/7] credential-store: move related functions to credential-store file Calvin Wan
2023-05-12 17:15 ` [PATCH v6 4/7] object-name: move related functions to object-name Calvin Wan
2023-05-12 17:15 ` [PATCH v6 5/7] path: move related function to path Calvin Wan
2023-05-12 17:15 ` [PATCH v6 6/7] strbuf: clarify dependency Calvin Wan
2023-05-12 17:15 ` [PATCH v6 7/7] strbuf: remove global variable Calvin Wan
2023-05-12 20:24 ` [PATCH v6 0/7] strbuf cleanups Junio C Hamano
2023-05-13 5:54 ` Eric Sunshine
2023-06-06 19:47 ` [PATCH v7 " Calvin Wan
2023-06-06 19:48 ` [PATCH v7 1/7] strbuf: clarify API boundary Calvin Wan
2023-06-06 19:48 ` [PATCH v7 2/7] strbuf: clarify dependency Calvin Wan
2023-06-06 19:48 ` [PATCH v7 3/7] abspath: move related functions to abspath Calvin Wan
2023-06-06 19:48 ` [PATCH v7 4/7] credential-store: move related functions to credential-store file Calvin Wan
2023-06-06 19:48 ` [PATCH v7 5/7] object-name: move related functions to object-name Calvin Wan
2023-06-06 19:48 ` [PATCH v7 6/7] path: move related function to path Calvin Wan
2023-06-06 19:48 ` [PATCH v7 7/7] strbuf: remove global variable Calvin Wan
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=20230511194446.1492907-1-calvinwan@google.com \
--to=calvinwan@google.com \
--cc=git@vger.kernel.org \
--cc=newren@gmail.com \
--cc=peff@peff.net \
--cc=phillip.wood123@gmail.com \
--cc=sunshine@sunshineco.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).