Re: [PATCH/RFC v1 4/5] fast-import.c: cleanup using strbuf_set operations

2014-06-09 Thread Eric Sunshine
On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler jmmah...@gmail.com wrote:
 Subject: fast-import.c: cleanup using strbuf_set operations

This might read more clearly if written:

fast-import: simplify via strbuf_set()

 Simplified cases where a strbuf_reset was immediately followed by a
 strbuf_add using the new strbuf_set operations.

On this project, commit messages are written in imperative mood:

Simplify cases where ... is immediately ...

More below.

 Signed-off-by: Jeremiah Mahler jmmah...@gmail.com
 ---
  fast-import.c | 19 ++-
  1 file changed, 6 insertions(+), 13 deletions(-)

 diff --git a/fast-import.c b/fast-import.c
 index e8ec34d..c23935c 100644
 --- a/fast-import.c
 +++ b/fast-import.c
 @@ -2741,8 +2741,7 @@ static void parse_new_commit(void)
 hashcpy(b-branch_tree.versions[0].sha1,
 b-branch_tree.versions[1].sha1);

 -   strbuf_reset(new_data);
 -   strbuf_addf(new_data, tree %s\n,
 +   strbuf_setf(new_data, tree %s\n,
 sha1_to_hex(b-branch_tree.versions[1].sha1));
 if (!is_null_sha1(b-sha1))
 strbuf_addf(new_data, parent %s\n, sha1_to_hex(b-sha1));

Unlike the cases in patches 3/5 and 5/5 where the strbuf is used or
returned immediately following the strbuf_set() call, I am not
convinced that this change is an improvement. This code has the
general form:

strbuf_reset(...);
strbuf_add(...);
if (condition)
strbuf_add(...);
strbuf_add(...);

in which it is clear that the string is being built piecemeal, and
it's easy for a programmer to insert, remove, or re-order strbuf_add()
calls.

Replacing the first two lines with strbuf_set() somewhat obscures the
fact that the string is going to be built up piecemeal. Plus, the
change makes it more difficult to insert, remove, or re-order the
strbuf_add() invocations.

This isn't a strong objection, but the benefit of the change seems
minimal or non-existent.

Ditto for several remaining cases in this patch.

 @@ -2829,9 +2828,7 @@ static void parse_new_tag(void)
 parse_data(msg, 0, NULL);

 /* build the tag object */
 -   strbuf_reset(new_data);
 -
 -   strbuf_addf(new_data,
 +   strbuf_setf(new_data,
 object %s\n
 type %s\n
 tag %s\n,
 @@ -2898,8 +2895,7 @@ static void cat_blob(struct object_entry *oe, unsigned 
 char sha1[20])
  * Output based on batch_one_object() from cat-file.c.
  */
 if (type = 0) {
 -   strbuf_reset(line);
 -   strbuf_addf(line, %s missing\n, sha1_to_hex(sha1));
 +   strbuf_setf(line, %s missing\n, sha1_to_hex(sha1));
 cat_blob_write(line.buf, line.len);
 strbuf_release(line);
 free(buf);
 @@ -2910,8 +2906,7 @@ static void cat_blob(struct object_entry *oe, unsigned 
 char sha1[20])
 if (type != OBJ_BLOB)
 die(Object %s is a %s but a blob was expected.,
 sha1_to_hex(sha1), typename(type));
 -   strbuf_reset(line);
 -   strbuf_addf(line, %s %s %lu\n, sha1_to_hex(sha1),
 +   strbuf_setf(line, %s %s %lu\n, sha1_to_hex(sha1),
 typename(type), size);
 cat_blob_write(line.buf, line.len);
 strbuf_release(line);
 @@ -3034,14 +3029,12 @@ static void print_ls(int mode, const unsigned char 
 *sha1, const char *path)

 if (!mode) {
 /* missing SP path LF */
 -   strbuf_reset(line);
 -   strbuf_addstr(line, missing );
 +   strbuf_setstr(line, missing );
 quote_c_style(path, line, NULL, 0);
 strbuf_addch(line, '\n');
 } else {
 /* mode SP type SP object_name TAB path LF */
 -   strbuf_reset(line);
 -   strbuf_addf(line, %06o %s %s\t,
 +   strbuf_setf(line, %06o %s %s\t,
 mode  ~NO_DELTA, type, sha1_to_hex(sha1));
 quote_c_style(path, line, NULL, 0);
 strbuf_addch(line, '\n');
 --
 2.0.0.573.ged771ce.dirty
--
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


Re: [PATCH/RFC v1 4/5] fast-import.c: cleanup using strbuf_set operations

2014-06-09 Thread Jeremiah Mahler
Eric,

On Mon, Jun 09, 2014 at 06:12:12AM -0400, Eric Sunshine wrote:
 On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler jmmah...@gmail.com wrote:
  Subject: fast-import.c: cleanup using strbuf_set operations
 
...
 
  Signed-off-by: Jeremiah Mahler jmmah...@gmail.com
  ---
   fast-import.c | 19 ++-
   1 file changed, 6 insertions(+), 13 deletions(-)
 
  diff --git a/fast-import.c b/fast-import.c
  index e8ec34d..c23935c 100644
  --- a/fast-import.c
  +++ b/fast-import.c
  @@ -2741,8 +2741,7 @@ static void parse_new_commit(void)
  hashcpy(b-branch_tree.versions[0].sha1,
  b-branch_tree.versions[1].sha1);
 
  -   strbuf_reset(new_data);
  -   strbuf_addf(new_data, tree %s\n,
  +   strbuf_setf(new_data, tree %s\n,
  sha1_to_hex(b-branch_tree.versions[1].sha1));
  if (!is_null_sha1(b-sha1))
  strbuf_addf(new_data, parent %s\n, sha1_to_hex(b-sha1));
 
 Unlike the cases in patches 3/5 and 5/5 where the strbuf is used or
 returned immediately following the strbuf_set() call, I am not
 convinced that this change is an improvement. This code has the
 general form:
 
 strbuf_reset(...);
 strbuf_add(...);
 if (condition)
 strbuf_add(...);
 strbuf_add(...);
 
 in which it is clear that the string is being built piecemeal, and
 it's easy for a programmer to insert, remove, or re-order strbuf_add()
 calls.
 
 Replacing the first two lines with strbuf_set() somewhat obscures the
 fact that the string is going to be built up piecemeal. Plus, the
 change makes it more difficult to insert, remove, or re-order the
 strbuf_add() invocations.
 
 This isn't a strong objection, but the benefit of the change seems
 minimal or non-existent.
 
 Ditto for several remaining cases in this patch.
 
...

This is a great observation that I certainly did overlook.  Using
strbuf_add or strbuf_set to help make it more obvious what the code is
doing.

By the same token, strbuf_set can be used to replace strbuf_add to make
it clear that nothing important was being added to and that the entire
buffer is being replaced.

  struct strbuf mybuf = STRBUF_INIT;

  strbuf_add(mybuf, ...);  /* Was something there before? */

  strbuf_set(mybuf, ...);  /* Replace everything. */

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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