On Wed, May 3, 2023 at 11:48 AM Calvin Wan [off-list ref] wrote:

Strbuf is a widely used basic structure that should only interact with
other primitives in strbuf.[ch].

"should"?  I agree with moving things in this direction, but the
wording here suggests there's a pre-existing directive or guideline;
but if so, I'm unfamiliar with it.  Maybe this should be reworded to
"This series modifies strbuf to focus on string manipulation with
minimal other dependencies"?

Over time certain functions inside of
strbuf.[ch] have been added to interact with higher level objects and
functions. This series cleans up some of those higher level interactions
by moving the offending functions to the files they interact with and
adding documentation to strbuf.h. With the goal of eventually being able
to stand up strbuf as a libary, this series also removes the use of
environment variables from strbuf.

As Junio pointed out, "environment variables" is misleading; see below.

Range-diff against v1:
-:  ---------- > 1:  e0dd3f5295 strbuf: clarify API boundary

I read this separately; this patch looks good to me.

1:  283771c088 ! 2:  ec1ea6ae4f abspath: move related functions to abspath
    @@ Commit message
         abspath: move related functions to abspath

         Move abspath-related functions from strbuf.[ch] to abspath.[ch] since
    -    they do not belong in a low-level library.
    +    paths are not primitive objects and therefore strbuf should not interact
    +    with them.

This applies to patches 4 & 5 as well:

Would this perhaps be better worded as:

    Move abspath-related functions from strbuf.[ch] to abspath.[ch] so that
    strbuf is focused on string manipulation routines with minimal dependencies.

?

      ## abspath.c ##
     @@ abspath.c: char *prefix_filename_except_for_dash(const char *pfx, const char *arg)
2:  58f78b8ae0 = 3:  2d74561b91 credential-store: move related functions to credential-store file
3:  88ab90c079 ! 4:  30b5e635cb object-name: move related functions to object-name
    @@ Commit message
         object-name: move related functions to object-name

         Move object-name-related functions from strbuf.[ch] to object-name.[ch]
    -    since they do not belong in a low-level library.
    +    since paths are not a primitive object that strbuf should directly
    +    interact with.

      ## object-name.c ##
     @@ object-name.c: static void find_abbrev_len_packed(struct min_abbrev_data *mad)
4:  30b7de5a81 ! 5:  6905618470 path: move related function to path
    @@ Metadata
      ## Commit message ##
         path: move related function to path

    -    Move path-related function from strbuf.[ch] to path.[ch] since it does
    -    not belong in a low-level library.
    +    Move path-related function from strbuf.[ch] to path.[ch] since path is
    +    not a primitive object and therefore strbuf should not directly interact
    +    with it.

      ## path.c ##
     @@ path.c: int normalize_path_copy(char *dst, const char *src)
5:  7b6d6353de ! 6:  caf3482bf7 strbuf: clarify dependency
    @@ Metadata
      ## Commit message ##
         strbuf: clarify dependency

    +    refs.h was once needed but is no longer so as of 6bab74e7fb8 ("strbuf:
    +    move strbuf_branchname to sha1_name.c", 2010-11-06). strbuf.h was
    +    included thru refs.h, so removing refs.h requires strbuf.h to be added
    +    back.
    +

Thanks.

6:  ffacd1cbe5 ! 7:  a7f23488f8 strbuf: remove enviroment variables
    @@ Metadata
     Author: Calvin Wan [off-list ref]

      ## Commit message ##
    -    strbuf: remove enviroment variables
    +    strbuf: remove environment variables

    -    As a lower level library, strbuf should not directly access environment
    -    variables within its functions. Therefore, add an additional variable to
    -    function signatures for functions that use an environment variable and
    -    refactor callers to pass in the environment variable.
    +    As a library that only interacts with other primitives, strbuf should
    +    not directly access environment variables within its
    +    functions. Therefore, add an additional variable to function signatures
    +    for functions that use an environment variable and refactor callers to
    +    pass in the environment variable.

"environment variable" makes one think not of "variable from
environment.c [that happens to be a global]" but of
https://en.wikipedia.org/wiki/Environment_variable.  The fact that the
variable is defined in environment.c isn't even relevant here, though,
while the fact that it is a global is relevant.  I'd just
s/environment variable/global variable/ in all 4 places (er, 5 if you
include the cover letter).

Your v3 7/7 has an important correction, but you didn't send out a
range diff for v3 so I'm putting my comments on your v2 range-diff.
:-)

    + /**
       * 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.
    ++ * 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.
       */

Thanks.


This series fixes two of my comments on v1, and the other was just
"here's another cleanup that could be added" -- it's fine to punt on.

I only had a couple minor comments on this v2/v3 that should be easy
to fix up, all included in this response to the cover letter.