From: Jeff King <peff@peff.net>
Date: Thu, 01 Dec 2016 04:29:27 GMT
Subject: Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
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:

> +/*
> + * 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