Re: [PATCH] Make is_gitfile a non-static generic function

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

Phil Hord <hordp@cisco.com> writes:

> The new is_gitfile is an amalgam of similar functional checks
> from different places in the code....

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 488f48e..5110399 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -120,13 +120,7 @@ static char *get_repo_path(const char *repo, int
> *is_bundle)
>  			return xstrdup(absolute_path(path));
>  		} else if (S_ISREG(st.st_mode) && st.st_size > 8) {
>  			/* Is it a "gitfile"? */
> -			char signature[8];
> -			int len, fd = open(path, O_RDONLY);
> -			if (fd < 0)
> -				continue;
> -			len = read_in_full(fd, signature, 8);
> -			close(fd);
> -			if (len != 8 || strncmp(signature, "gitdir: ", 8))
> +			if (!is_gitfile(path))
>  				continue;
>  			path = read_gitfile(path);
>  			if (path) {
> diff --git a/cache.h b/cache.h
> index 601f6f6..7a8d9f9 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -441,6 +441,7 @@ extern const char *get_git_work_tree(void);
>  extern const char *read_gitfile(const char *path);
>  extern const char *resolve_gitdir(const char *suspect);
>  extern void set_git_work_tree(const char *tree);
> +extern int is_gitfile(const char *path);
>   #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
>  diff --git a/transport.c b/transport.c
> index f3195c0..d08a826 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -859,7 +859,11 @@ static int is_local(const char *url)
>  		has_dos_drive_prefix(url);
>  }
>  -static int is_gitfile(const char *url)
> +/*
> + * See if the referenced file looks like a 'gitfile'.
> + * Does not try to determine if the referenced gitdir is actually valid.
> + */
> +int is_gitfile(const char *url)
>  {
>  	struct stat st;
>  	char buf[9];

After looking at this patch and the way the other caller in transport.c
uses it, I am more and more convinced that "is_gitfile()" is a stupid and
horrible mistake.

The caller in transport.c says "I am about to read from a regular file,
and usually I would treat it as a bundle, but I want to avoid that
codepath if that regular file is not a bundle. So I use the codepath only
when that file is not a gitfile".

It should be saying "Is it a bundle? Then I'd use the codepath to read
from the bundle" to begin with. Otherwise the code will break when we add
yet another regular file we can fetch from that is not a bundle nor a
gitfile.

I think the hand-crafted check in builtin/clone.c you removed originated
from laziness to avoid teaching read_gitfile() to read potential gitfile
silently (and signal errors by simply returning NULL). I also suspect the
codepath may become simpler if we had a way to ask "Is this a bundle?".

I think read_bundle_header() in bundle.c can be refactored to a silent
interface that allows us to ask "Is this a bundle?" question properly.