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

Hi there,

2010/5/26 Junio C Hamano [off-list ref]:

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.

quoted

+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