Re: [PATCH v14 09/27] bisect--helper: `bisect_write` shell function in C

>
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
> >>> +struct bisect_terms {
> >>> +     struct strbuf term_good;
> >>> +     struct strbuf term_bad;
> >>> +};
> >>
> >> I think "struct strbuf" is overrated.  ...
> >> I think you can just say "const char *" in this case.
> >
> > Using struct strbuf is not really overrated but in fact required. But
>
> Nothing is required.
>
> I can make your life easier by requiring you to never use struct
> strbuf as a structure field type, though.  You will almost never
> need it unless you are building something exotic anyway.
>
> Step back and think what "strbuf" is good for.  It holds a pointer
> to a piece of memory the field owns, over-allocates and knows the
> size of allocation and usage.  That is good if you need to
>
>  (1) frequently find out the length of the string; without a
>      separate .len member you would have to run strlen().
>
>  (2) incrementally add to the string in-place; as it overallocates,
>      appending to the string would not have to involve realloc()
>      every time and the cost of it is amortized.
>
>  (3) make complex operations like splicing another string in,
>      trimming substring out, etc.
>
> You need to do none of the above to these fields.  term.term_good is
> either taken from an argv[] element, or you read from a line from a
> file and set it.  You may do some trivial computation and set it to
> the result, like "the other field uses 'old', so this one need to be
> set to 'new'".  The user of the field either has the string and sets
> it there, or reads the field's value as a whole string.  No string
> manipulation in the field in-place is needed.
>
> > yes, for this patch it might seem as overrated. In the shell code
> > initally TERM_GOOD is set to "good" while TERM_BAD is set to "bad".
> > Now there are a lot of instances (one of which is bisect_start()
> > function) where this can change. So if we keep it as "const char *",
> > it would be right to change the value of it after wards. And we cannot
> > keep it as "char []" because we don't know its size before hand.
>
> You are not making sense.  Nobody would suggest to use a fixed-size
> char array in this structure.  That wouldn't even work in the case
> where you are stuffing what comes in an argv[] element in there,
> e.g.
>
>     terms.term_good = argv[3];
>
> And you can of course support changing the value of the field
> without using strbuf.  Just update the pointer to point at the piece
> of memory that holds the new value.
>
> In short, I do not see any good reason why the term_good field has
> to be anything other than "char *term_good" or "const char *term_good".
>
> Now, what you need to consider choosing between the two depends on
> where these strings can come from.  If they are known to be always
> unchanging between the time you set it til the end of the program
> (e.g. using an element in argv[]), you can just assign without
> making any copy and you can use "const char *term_good".  All other
> cases, the structure needs to take the ownership, and you would need
> to make a copy if you don't have the ownership, e.g.
>
>         terms.term_good = xstrdup(argv[3]);
>
> You may be reading from a file, a line at a time and you may have a
> line's content in a strbuf.  You do not (yet) own the buffer after
> reading it, e.g.
>
>         strbuf_getline(&buf, fp);
>         terms.term_good = strbuf_detach(&buf, NULL);
>
> Of course, if you need to take ownership of the memory, you would
> need to free(3) it as needed, which means the pattern to set the
> field would become
>
>         free(terms.term_good);
>         terms.term_good = ... some new value ...;
>
> Using strbuf as a local variable is good.  It gives a higher level
> of abstraction when you are actually performing string operations.
> In most applications, however, a field in a struct is where the
> result of a step of computation is kept, not a scratch-pad to
> perform steps of computation in.  When you are ready to update the
> value of a field, you _have_ a completed string, and you can just
> use "char *" field to point at it.  There is no need for strbuf in
> the field.
>
> Don't look at the data structure used in trailer.[ch] as a model; it
> is an example of a terribly bad implementation taste, a pattern that
> should not be followed.  Print it, not read it and burn it as a good
> symbolic gesture.