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