Re: [PATCH] clone: error specifically with --local and symlinked objects

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 6, 2023, 21:35 UTC
Message-ID: <ZC87IcLcBnxBRCdr@nand.local>
In-Reply-To: <kl6ly1n65l7t.fsf@chooglen-macbookpro.roam.corp.google.com>

On Wed, Apr 05, 2023 at 09:48:22AM -0700, Glen Choo wrote:

> Since this is an error code path, I think the extra lstat() is probably
> worth it since it lets us be really specific about the error. Maybe:

FWIW, I don't totally think it's necessary for us to report back to the user that they're trying to clone a repository with `--local` whose `$GIT_DIR/objects` either is or contains a symbolic link. Especially so since you're already updating the documentation here.

But I think it's a compelling argument in the name of improving the user experience, so I think that this is a reasonable direction. And I agree, that while the extra lstat() is unfortunate, I don't think there is a convenient way to do it otherwise.

We *could* teach the dir-iterator API to return a specialized error code either through a pointer, like:

    struct dir_iterator *dir_iterator_begin(const char *path,
                                            unsigned int flags, int *error)

and set error to something like -DIR_ITERATOR_IS_SYMLINK when error is non-NULL.

Or we could do something like this:

    int dir_iterator_begin(struct dir_iterator **it,
                           const char *path, unsigned int flags)

and have the `dir_iterator_begin()` function return its specialized error and initialize the dir iterator through a pointer. Between these two, I prefer the latter, but I think it's up to individual taste.

> 	if (!iter) {
> 		struct stat st;
>
>     if (errno == ENOTDIR && lstat(src->buf, &st) == 0 && S_ISLNK(st.st_mode))

Couple of nit-picks here. Instead of writing "lstat(src->buf, &st == 0)", we should write "!lstat(src->buf, &st)" to match Documentation/CodingGuidelines.

But note that that call to lstat() will clobber your errno value, so you'd want to save it off beforehand. If you end up going this route, I'd probably do something like:

    if (!iter) {
      int saved_errno = errno;
      if (errno == ENOTDIR) {
        struct stat st;

        if (!lstat(src->buf, &st) && S_ISLNK(st.st_mode))
          die(_("'%s' is a symlink, refusing to clone with `--local`"),
              src->buf);
      }
      errno = saved_errno;

      die_errno(_("failed to start iterator over '%s'"), src->buf);
    }

Thanks, Taylor