Message ID | fd022f88f54f6cf0feb61965b2dc47bca64c0937.1572127149.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | add: respect --ignore-errors when lstat() reports errors | expand |
"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(...);
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: > "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(...); > >
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); }