Re: [PATCH/RFC v3 0/4] Improving performance of git clean

Erik Elfström [off-list ref] writes:

Known Problems:
* Unsure about the setup.c:read_gitfile refactor, feels a bit
  messy?

The interface indeed feels somewhat messy.  I suspect that a better
interface might be more like setup_git_directory_gently() that is a
gentler version of setup_git_directory().  The function takes an
optional pointer to an int, and it behaves not-gently-at-all when
the optional argument is NULL.  The location the optional pointer
points at, when it is not NULL, can be used to return the error
state to the caller.  That function pair only uses the optional
argument to convey one bit of information (i.e. "are we in any git
repository or not?") back to the caller, but the interface could be
used to tell the caller a lot more if we wanted to.

If you model read_gitfile_gently() after that pattern, I would
expect that

 - The extra pattern would be "int *error";
 - The implementation of read_gitfile() would be

       #define read_gitfile(path) read_gitfile_gently((path), NULL)

   and the _gently() version will die when "error" parameter is set
   to NULL and finds any error.

 - The caller of the gentle variant can use the error code to
   determine what went wrong, not just the fact that it failed.  I
   do not think your caller does not have an immediate need to tell
   between "invalid gitfile format" and "No path in gitfile", but
   such an interface leaves that possibility open.

Thanks.

On Sun, Apr 19, 2015 at 3:14 AM, Junio C Hamano [off-list ref] wrote:

Erik Elfström [off-list ref] writes:

quoted

Known Problems:
* Unsure about the setup.c:read_gitfile refactor, feels a bit
  messy?

The interface indeed feels somewhat messy.  I suspect that a better
interface might be more like setup_git_directory_gently() that is a
gentler version of setup_git_directory().  The function takes an
optional pointer to an int, and it behaves not-gently-at-all when
the optional argument is NULL.  The location the optional pointer
points at, when it is not NULL, can be used to return the error
state to the caller.  That function pair only uses the optional
argument to convey one bit of information (i.e. "are we in any git
repository or not?") back to the caller, but the interface could be
used to tell the caller a lot more if we wanted to.

If you model read_gitfile_gently() after that pattern, I would
expect that

 - The extra pattern would be "int *error";
 - The implementation of read_gitfile() would be

       #define read_gitfile(path) read_gitfile_gently((path), NULL)

   and the _gently() version will die when "error" parameter is set
   to NULL and finds any error.

 - The caller of the gentle variant can use the error code to
   determine what went wrong, not just the fact that it failed.  I
   do not think your caller does not have an immediate need to tell
   between "invalid gitfile format" and "No path in gitfile", but
   such an interface leaves that possibility open.

Thanks.

Ok, I'll try that approach, thanks. (and apologies for the late reply)

/Erik