Re: [PATCH v2 0/7] strbuf cleanups

From: Elijah Newren <newren@gmail.com>
Date: May 7, 2023, 00:40 UTC
Message-ID: <CABPp-BGk-s+4b+OHi9FDXjwz5BeZPS8qb4XiJ8CVwkt8Siaq8A@mail.gmail.com>
In-Reply-To: <20230503184849.1809304-1-calvinwan@google.com>

On Wed, May 3, 2023 at 11:48 AM Calvin Wan <calvinwan@google.com> 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"?

Show 7 quoted lines

> 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.

Show 8 quoted lines

> 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.

Show 37 quoted lines

>       ## 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.

Show 17 quoted lines

> 6:  ffacd1cbe5 ! 7:  a7f23488f8 strbuf: remove enviroment variables
>     @@ Metadata
>      Author: Calvin Wan <calvinwan@google.com>
>
>       ## 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. :-)

Show 7 quoted lines

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