Re: [PATCH 1/1] add: respect `--ignore-errors` when `lstat()` reports errors

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 28, 2019, 02:03 UTC
Message-ID: <xmqq36fdbp8v.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <fd022f88f54f6cf0feb61965b2dc47bca64c0937.1572127149.git.gitgitgadget@gmail.com>

"qusielle via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 18 quoted lines

> From: qusielle <31454380+qusielle@users.noreply.github.com>
>
> "git add --ignore-errors" command fails immediately when lstat returns
> an error, despite the ignore errors' flag is enabled.
> ...
> diff --git a/read-cache.c b/read-cache.c
> index 133f790fa4..67237ecd29 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -801,7 +801,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>  int add_file_to_index(struct index_state *istate, const char *path, int flags)
>  {
>  	struct stat st;
> -	if (lstat(path, &st))
> +	if (lstat(path, &st) && !(flags & ADD_CACHE_IGNORE_ERRORS))
>  		die_errno(_("unable to stat '%s'"), path);
>  	return add_to_index(istate, path, &st, flags);
>  }

The only callers of this function that matter calls it and then responds to an error return like so:

(in builtin/add.c::update_callback())

	if (add_file_to_index(&the_index, path,	data->flags)) {
		if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
			die(_("updating files failed"));

(in builtin/add.c::add_files(), where ignore_add_errors was used to set the ADD_CACHE_IGNORE_ERRORS to flags in its caller)

	if (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {
		if (!ignore_add_errors)
			die(_("adding files failed"));

So you correctly identified what is the right place to fix. We should not "die_errno()"; we should give the control back to the caller instead.

But after a failed stat, the code with your patch still calls add_to_index() using the now undefined stat data, which would contaminate the in-core index with wrong data.

I think we should instead return without touching the index for the path we had trouble lstat()ing.

IOW

	if (lstat(path, &st)) {
		if (flags & ADD_CACHE_IGNORE_ERRORS)
			return -1;
		else
			die_errno(_("unable to ..."));
	}
	return add_to_index(...);