git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Qqusielle@gmail.com <qusielle@gmail.com>
Date
Nov 3, 2019, 20:17 UTC
Message-ID
<eac0456e-0f61-9424-975d-0567bb92327d@gmail.com>
In-Reply-To
<xmqq36fdbp8v.fsf@gitster-ct.c.googlers.com>
Dear Junio,

Thank you for reviewing my patch. I completely agree with you, that add_to_index() should not be called with undefined data.

I will amend patch now with proposed changes.
Thank you!

Best regards, Qusielle

On 28.10.2019 03:03, Junio C Hamano wrote:
Show 59 quoted lines
> "qusielle via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 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(...);
>
>
Previous: Junio C HamanoNext: Qusielle via GitGitGadget
Message 4 of 6 in “add: respect --ignore-errors when lstat() reports errors”
  1. 0/1 add: respect --ignore-errors when lstat() reports errorsQusielle via GitGitGadget, Oct 26, 2019
  2. 1/1 add: respect `--ignore-errors` when `lstat()` reports errorsqusielle via GitGitGadget, Oct 26, 2019
  3. Junio C HamanoOct 28, 2019
  4. qusielle@gmail.comNov 3, 2019
  5. 0/1 add: respect --ignore-errors when lstat() reports errorsQusielle via GitGitGadget, Nov 3, 2019
  6. 1/1 add: respect `--ignore-errors` when `lstat()` reports errorsqusielle via GitGitGadget, Nov 3, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.