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

[Junio C Hamano <gitster@pobox.com>]

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.