Re: [PATCH v5 00/44] Make git-am a builtin

Hi,

(Sorry for the late reply. Caught a nasty stomach bug that kept me in
bed for a while ><)

On Thu, Jul 9, 2015 at 2:00 PM, Junio C Hamano [off-list ref] wrote:

What I pushed out tonight should have SQUASH??? (or fixup!) that
splits this into appropriate steps in your series.  Please check.

Yeah they look good.

Note that you do not have to say "if the variable h2as something,
then free it".  free(NULL) is perfectly fine and we can read

        free(var);
        var = compute_new_value();

just fine.

OK. There are other places where I used "if (x) free(x)" though (such
as in am_state_release()), so I will fix them in a re-roll as well.

However, I am reluctant to blindly replace assert(!state->field)
with free(state->field).  Are there cases where we _must_ call a
function that sets these fields at most once?

I wouldn't say we are "blindly" replacing them. The purpose of the
assert(!state->field) is to ensure that we are not overwriting any
commit metadata already stored in the state directory. And in almost
all cases, this holds true. However, as you have just shown, running
"git am" again while there is a session in progress will override the
commit metadata stored in the state directory, so I believe the
replacement with free() is reasonable.

On the other hand, assert() like this is more or less useless.

        assert(state->field);
        ...
        printf("%s", state->field); /* or other uses */

"The caller must have filled the field" can be seen by unconditional
use of "state->field" without such an assert().

OK. Will remove them as well.

Thanks,
Paul