Re: [PATCH 2/5] refs: split log_ref_write logic into log_ref_setup

From: Erick Mattos <erick.mattos@gmail.com>
Date: May 26, 2010, 18:11 UTC
Message-ID: <AANLkTikPypcmGB6NuTl-SQZR3lnIvdmVG5E8wjVAlIej@mail.gmail.com>
In-Reply-To: <7v632bs13c.fsf@alter.siamese.dyndns.org>

Hi there,

2010/5/26 Junio C Hamano <gitster@pobox.com>:

> I have a slight suspicion that it would have made the patch smaller and
> easier to read if you kept the name of the on-stack log_file[] as-is, and
> named the retval parameter logfile_p or soemthing.

The size of the patch is indeed by the split/insertion which complicates the diff's life. If you compare both blobs you see it is not a hard change. But we can not hope for computer's intelligence during this lifetime. ;-D

>  Also you would need to
> make this buffer "static char log_file[]", no?  Otherwise you would be
> returning a pointer to a dead buffer to the caller.

Not really. git_snpath() is taking care of setting up the buffer dynamically in the heap. The calling function presents its buffer by reference thus only the pointer's address which its content is later changed to point to the dynamic one.

Show 12 quoted lines

>> +static int log_ref_write(const char *ref_name, const unsigned char *old_sha1,
>> +                      const unsigned char *new_sha1, const char *msg)
>> +{
>> + ...
>> +     result = log_ref_setup(ref_name, &log_file);
>> +     if (result)
>> +             return result;
>> +
>> +     logfd = open(log_file, oflags);
>
> Yuck, the caller needs to call "setup" which discards the file descriptor
> opened for writing and then open it again itself?

The separation of logic of setup from writing of the reflog and the consequently created log_ref_setup was meant to just prepare the reflog file. This way it can be used consistently on different functions.

At the moment It is being used on log_ref_write() and in update_refs_for_switch(). In the first case it is interesting that the reflog keeps opened to be used. On the later case it is not. So, one of the calling functions would have to do something.

We have two approaches to that:
1. keeping the reflog opened and making sure the calling function close it.
2. closing it and making the calling function open it or not as needed.

I have chosen 2 because of:
* I think it is safer to have any function closing open files,
cleaning variables or
  resources used by it whenever possible.
* It is more elegant that the function does what it is meant to do, in this case
  setting up the file only.
* It possibly keeps the code cleaner because only one 'close' for this function
  needs to be done and in the same place it happened the correspondent 'open'.
* No approach was going to cost any resources more.

Now just a question, Junio:

I forgot to sign-off those patches, should I have to send them again?

Regards