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

From: Paul Tan <pyokagan@gmail.com>
Date: Jul 12, 2015, 12:29 UTC
Message-ID: <CACRoPnQyPpK3g0G3N-Yf2Zatk5RCEUe_1mbW9LGq8-=zypYcNg@mail.gmail.com>
In-Reply-To: <xmqqio9tx42z.fsf@gitster.dls.corp.google.com>

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 <gitster@pobox.com> 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.

Show 7 quoted lines

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

Show 8 quoted lines

> 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