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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 27, 2016, 21:22 UTC
Message-ID: <xmqqtwe5di02.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAFZEwPMcUf-Tm_ijJbEG1Bh7F+7ML2fw6Vw2Gt94B9gyoGL1FQ@mail.gmail.com>

Pranit Bauva <pranit.bauva@gmail.com> writes:

Show 9 quoted lines

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

Show 6 quoted lines

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