Re: [PATCH v4 04/23] sequencer: reuse strbuf_trim() in read_oneliner()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Denton

On 06/04/2020 15:03, Phillip Wood wrote:
Hi Denton

On 04/04/2020 02:11, Denton Liu wrote:
In the original read_oneliner() logic, we duplicated the logic for
strbuf_trim_trailing_newline() with one exception: instead of preventing
buffer accesses below index 0, it would prevent buffer accesses below
index `orig_len`. Although this is correct, it isn't worth having the
duplicated logic around.

Reset `buf` before using it then replace the trimming logic with
strbuf_trim().

I should have picked up on this before but this changes the semantics of the function as it strips all whitespace from the start and end of the strbuf. Above you talked about using strbuf_trim_trailing_newline() instead which would not change the semantics of this function. You could test to see if we've read anything and only call strbuf_trim_trailing_newline() in that case without messing with strbuf_reset(). (there is a corner case where if the buffer ends with '\r' when the function is called and it reads a single '\n' then the '\r' would be stripped as well but I think that is unlikely to happen in the wild)

I'd also be fine with dropping this patch and leaving the trimming code as is

Best Wishes

Phillip

Best Wishes

Phillip

As a cleanup, remove all reset_strbuf()s that happen before
read_oneliner() is called.

Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
---
  sequencer.c | 18 +++++-------------
  1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index faab0b13e8..09ca68f540 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -420,8 +420,8 @@ static int write_message(const void *buf, size_t len, const char *filename,
  }
  /*
- * Reads a file that was presumably written by a shell script, i.e. with an
- * end-of-line marker that needs to be stripped.
+ * Resets a strbuf then reads a file that was presumably written by a shell
+ * script, i.e. with an end-of-line marker that needs to be stripped.
   *
   * Note that only the last end-of-line marker is stripped, consistent with the
   * behavior of "$(cat path)" in a shell script.
@@ -431,23 +431,19 @@ static int write_message(const void *buf, size_t len, const char *filename,
  static int read_oneliner(struct strbuf *buf,
      const char *path, int skip_if_empty)
  {
-    int orig_len = buf->len;
      if (!file_exists(path))
          return 0;
+    strbuf_reset(buf);
      if (strbuf_read_file(buf, path, 0) < 0) {
          warning_errno(_("could not read '%s'"), path);
          return 0;
      }
-    if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') {
-        if (--buf->len > orig_len && buf->buf[buf->len - 1] == '\r')
-            --buf->len;
-        buf->buf[buf->len] = '\0';
-    }
+    strbuf_trim(buf);
-    if (skip_if_empty && buf->len == orig_len)
+    if (skip_if_empty && !buf->len)
          return 0;
      return 1;
@@ -2471,7 +2467,6 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)   static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
  {
-    strbuf_reset(buf);
      if (!read_oneliner(buf, rebase_path_strategy(), 0))
          return;
      opts->strategy = strbuf_detach(buf, NULL);
@@ -2494,7 +2489,6 @@ static int read_populate_opts(struct replay_opts *opts)
                  free(opts->gpg_sign);
                  opts->gpg_sign = xstrdup(buf.buf + 2);
              }
-            strbuf_reset(&buf);
          }
          if (read_oneliner(&buf, rebase_path_allow_rerere_autoupdate(), 1)) { @@ -2526,7 +2520,6 @@ static int read_populate_opts(struct replay_opts *opts)
              opts->keep_redundant_commits = 1;
          read_strategy_opts(opts, &buf);
-        strbuf_reset(&buf);
          if (read_oneliner(&opts->current_fixups,
                    rebase_path_current_fixups(), 1)) {
@@ -4006,7 +3999,6 @@ static int pick_commits(struct repository *r,
                  res = error(_("could not read orig-head"));
                  goto cleanup_head_ref;
              }
-            strbuf_reset(&buf);
              if (!read_oneliner(&buf, rebase_path_onto(), 0)) {
                  res = error(_("could not read 'onto'"));
                  goto cleanup_head_ref;




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux