On Mon, Nov 10, 2014 at 9:36 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Johan Herland <johan@xxxxxxxxxxx> writes:
>
>> Signed-off-by: Johan Herland <johan@xxxxxxxxxxx>
>> ---
>> builtin/notes.c | 12 +++++-------
>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index 1017472..f1480cf 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> @@ -399,7 +399,7 @@ static int append_edit(int argc, const char **argv, const char *prefix);
>>
>> static int add(int argc, const char **argv, const char *prefix)
>> {
>> - int retval = 0, force = 0;
>> + int force = 0;
>> const char *object_ref;
>> struct notes_tree *t;
>> unsigned char object[20], new_note[20];
>> @@ -441,6 +441,8 @@ static int add(int argc, const char **argv, const char *prefix)
>>
>> if (note) {
>> if (!force) {
>> + free_note_data(&d);
>> + free_notes(t);
>> if (!d.given) {
>
> It looks a bit strange to refer to d.given after calling a function
> that sounds as if it is meant to clear what is recorded in and to
> invalidate &d; yes, I can read the implementation to see that
> d.given keeps its stale value, but that is something other people
> may want to "clean up" later and this reference to d.given will get
> in the way when that happens.
Yes, that was obviously an oversight on my part.
> At this point of the code, it makes sense to free t in preparation
> to either switching to append codepath or erroring out, but does &d
> have anything in it already to necessitate its freeing?
Actually, no, although verifying that required double-checking that
each of the -m/-F/-c/-C parsers which store data into &d, do in fact
set d.given.
I suggest to either move the free_note_data(&d) call below the "if
(!d.given)" block, or reorganize into this (which at the moment reads
better to me):
if (note) {
if (!force) {
free_notes(t);
if (d.given) {
free_note_data(&d);
return error(_("Cannot add notes. "
"Found existing notes for object %s. "
"Use '-f' to overwrite existing notes"),
sha1_to_hex(object));
}
/*
* Redirect to "edit" subcommand.
*
* We only end up here if none of -m/-F/-c/-C or -f are
* given. The original args are therefore still in
* argv[0-1].
*/
argv[0] = "edit";
return append_edit(argc, argv, prefix);
}
fprintf(stderr, _("Overwriting existing notes for object %s\n"),
sha1_to_hex(object));
}
This keeps the two free_* calls close together, only separated by if
(d.given), which nicely indicates whether we need to call
free_notes_data(&d) at all.
If that looks good to you, do you want a re-roll, or is it easier to
fix up yourself?
...Johan
--
Johan Herland, <johan@xxxxxxxxxxx>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html