Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules

From: Jeff King <peff@peff.net>
Date: Dec 1, 2016, 04:29 UTC
Message-ID: <20161201042926.mr2qdta7hviizcya@sigill.intra.peff.net>
In-Reply-To: <1480555714-186183-2-git-send-email-bmwill@google.com>

On Wed, Nov 30, 2016 at 05:28:29PM -0800, Brandon Williams wrote:

Show 15 quoted lines

> +/*
> + * Determine if a submodule has been populated at a given 'path'
> + */
> +int is_submodule_populated(const char *path)
> +{
> +	int ret = 0;
> +	struct stat st;
> +	char *gitdir = xstrfmt("%s/.git", path);
> +
> +	if (!stat(gitdir, &st))
> +		ret = 1;
> +
> +	free(gitdir);
> +	return ret;
> +}

I don't know if it's worth changing or not, but this could be a bit shorter:

  int is_submodule_populated(const char *path)
  {
	return !access(mkpath("%s/.git", path), F_OK);
  }

There is a file_exists() helper, but it uses lstat(), which I think you don't want (because you'd prefer to bail on a broken .git symlink). But access(F_OK) does what you want, I think.

mkpath() is generally an unsafe function because it uses a static buffer, but it's handy and safe for handing values to syscalls like this.

I say "I don't know if it's worth it" because what you've written is fine, and while more lines, it's fairly obvious and safe.

-Peff