Re: [PATCH v2 05/25] sequencer: allow the sequencer to take custody of malloc()ed data

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> If you prefer to accept such sloppy work, I will change it of course,
> feeling dirty that it has my name on it.

I do want neither leaks nor sloppyness, and I thought that was
understood by everybody, hence I thought the last round made it
clear that _entrust() must not exist.

Let's try again.

We manage lifetime of a field in a structure in one of three ways in
our codebase [*1*].

 * A field can always point at a borrowed piece of memory that the
   destruction of the containing structure or re-assignment to the
   field do not have to free the current value of the field.  This
   is a minority, simply because it is rare for a field to be always
   able to borrow from others.  The destruction of the containing
   structure or re-assignment to the field needs to do nothing
   special.

 * A field can own the piece of memory that it points at.  The
   destruction of the containing structure or re-assignment to the
   field needs to free the current value of the field [*2*].  A
   field that can be assigned a value from the configuration (which
   is typically xstrdup()'ed) is an example of such a field, and if
   a command line option also can assign to the field, we'd need to
   xstrdup() it, even though we know we could borrow from argv[],
   because cleaning-up needs to be able to free(3) it.

 * A field can sometimes own and sometimes borrow the memory, and it
   is accompanied by another field to tell which case it is, so that
   cleaning-up can tell when it needs to be free(3)d.  This is a
   minority case, and we generally avoid it especially in modern
   code for small allocation, as it makes the lifetime rule more
   complex than it is worth.

The _entrust() thing may have been an excellent fourth option if it
were cost-free.  xstrdup() that the second approach necessitates for
literal strings (like part of argv[]) would become unnecessary and
we can treat as if all the fields in a structure were all borrowing
the memory from elsewhere (in fact, _entrust() creates the missing
owners of pieces of memory that need to be freed later); essentially
it turns a "mixed ownership" case into the first approach.

But it is not cost-free.  The mechanism needs to allocate memory to
become the missing owner and keep track of which pieces of memory
need to be freed later, and the resulting code does not become any
easier to follow.  The programmer still needs to know which ones to
_entrust() just like the programmer in the model #2 needs to make
sure xstrdup() is done for appropriate pieces of memory--the only
difference is that an _entrust() programmer needs to mark the pieces
of memory to be freed, while a #2 programmer needs to xstrdup() the
pieces of memory that are being "borrowed".

So let's not add the fourth way to our codebase.  "mixed ownership"
case should be turned into "field owns the memory", i.e. approach #2.


[Footnotes]

*1* It is normal for different fields in the same structure follow
    different lifetime rules.

*2* Unless leaks are allowed, that is.  I think we have instances
    where "git cmd --opt=A --opt=B" allocates and stores a new piece
    of memory that is computed based on A and store it to a field,
    and then overwrites the field with another allocation of a value
    based on B without freeing old value, saying "the caller can
    avoid passing the same thing twice, and such a leak is miniscule
    anyway".  That is not nice, and we've been reducing them over
    time.