Re: [PATCH 3/4] clone: factor out dir_exists() helper

[Jeff King <peff@peff.net>]

On Thu, Jan 04, 2018 at 03:47:18PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Two parts of git-clone's setup logic check whether a
> > directory exists, and they both call stat directly with the
> > same scratch "struct stat" buffer. Let's pull that into a
> > helper, which has a few advantages:
> >
> >   - it makes the purpose of the stat calls more obvious
> >
> >   - it makes it clear that we don't care about the
> >     information in "buf" remaining valid
> >
> >   - if we later decide to make the check more robust (e.g.,
> >     complaining about non-directories), we can do it in one
> >     place
> >
> > Note that we could just use file_exists() for this, which
> > has identical code. But we specifically care about
> > directories, so this future-proofs us against that function
> > later getting more picky about seeing actual files.
> 
> It leaves funny taste in my mouth to see that dir_exists() does call
> stat() but does not check st.st_mode to see if it is a directory,
> but for this particular caller, we want dest_exists() to be true
> even when the thing is a non-directory, so that !is_empty_dir(dir)
> call is made on the next line to trigger "exists but not an empty
> dir" error.  After all, what this caller really wants to ask is "is
> something sitting there?" and the answer it expects under normal
> condition is "no, there is nothing there".

Yeah, that was part of the reason I left this as file-local instead of
adding it globally.

> If we really want to be anal, perhaps a new helper path_exists()
> that cares only about existence of paths (i.e. the implementation of
> these two helpers they currently have), together with update to
> check the st.st_mode for file_exists() and dir_exists(), may help
> making the API set more rational, but I do not think it is worth it.

Yep, I also considered that file_exists() probably wants to be
path_exists() with its current implementation. We'd probably want to
review all of the callers.

Anyway, I tried to do the minimal refactoring here, with no change in
behavior. I'm not opposed to calling this dir_exists() as path_exists()
and making it globally available (as you note, I don't think we'd want
to use a true dir_exists() here).

-Peff