git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] add: respect --ignore-errors when lstat() reports errors
@ 2019-10-26 21:59 Qusielle via GitGitGadget
  2019-10-26 21:59 ` [PATCH 1/1] add: respect `--ignore-errors` when `lstat()` " qusielle via GitGitGadget
  2019-11-03 20:26 ` [PATCH v2 0/1] add: respect --ignore-errors when lstat() " Qusielle via GitGitGadget
  0 siblings, 2 replies; 6+ messages in thread
From: Qusielle via GitGitGadget @ 2019-10-26 21:59 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

"git add --ignore-errors" command fails immediately when lstat returns an
error, despite the ignore errors' flag is enabled.

There could be files that triggers an error on stat(), when other files
proceed correctly. Issue can be reproduced when running git under Cygwin and
some target files have utf-8 long names (200+ utf chars). Windows can handle
them, but all operations on them failed under Cygwin. Issue can not be
reproduced with usual latin/numeric only names. For example, create a file
with 220 'й' letters by Windows Explorer, then in Cygwin:

Here and below "ййй..." means the line of й copied 220 times.
=============================================================

$ echo -n 'ййй...' | wc -c # check the real size 440

$ ls -la ls: cannot access 'ййй...'$'\320': No such file or directory
-????????? ? ? ? ? ? 'ййй...'$'\320'

$ ls й* ls: cannot access 'ййй...'$'\320': No such file or directory

$ stat й* stat: cannot stat 'ййй...'$'\320': No such file or directory

In my perspective, it's okay to skip these problematic files when ignore
error flag is specified, but official Git terminates entire git add command
when such files come up. But with proposed patch it is the desired behavior:

$ git add --ignore-errors . error: ййй... can only add regular files,
symbolic links or git-directories

All other files have been added correctly.
==========================================

Signed-off-by: Qusielle <qusielle@gmail.com>

qusielle (1):
  add: respect `--ignore-errors` when `lstat()` reports errors

 read-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-432%2Fqusielle%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-432/qusielle/master-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/432
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/1] add: respect `--ignore-errors` when `lstat()` reports errors
  2019-10-26 21:59 [PATCH 0/1] add: respect --ignore-errors when lstat() reports errors Qusielle via GitGitGadget
@ 2019-10-26 21:59 ` qusielle via GitGitGadget
  2019-10-28  2:03   ` Junio C Hamano
  2019-11-03 20:26 ` [PATCH v2 0/1] add: respect --ignore-errors when lstat() " Qusielle via GitGitGadget
  1 sibling, 1 reply; 6+ messages in thread
From: qusielle via GitGitGadget @ 2019-10-26 21:59 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, qusielle

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.

There could be files that triggers an error on stat(), when other files
proceed correctly.
Issue can be reproduced when running git under Cygwin and some target files
have utf-8 long names (200+ utf chars). Windows can handle them, but all
operations on them failed under Cygwin.
Issue can not be reproduced with usual latin/numeric only names.
For example, create a file with 220 'й' letters by Windows Explorer,
then in Cygwin:

 # Here and below "ййй..." means the line of й copied 220 times.
$ echo -n 'ййй...' | wc -c  # check the real size
440

$ ls -la
ls: cannot access 'ййй...'$'\320': No such file or directory
-????????? ? ?              ?        ?            ? 'ййй...'$'\320'

$ ls й*
ls: cannot access 'ййй...'$'\320': No such file or directory

$ stat й*
stat: cannot stat 'ййй...'$'\320': No such file or directory

In my perspective, it's okay to skip these problematic files when ignore
error flag is specified, but official Git terminates entire git add command
when such files come up. But with proposed patch it is the desired behavior:

$ git add --ignore-errors .
error: ййй... can only add regular files, symbolic links or git-directories
 # All other files have been added correctly.

Signed-off-by: Qusielle <qusielle@gmail.com>
---
 read-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
 }
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] add: respect `--ignore-errors` when `lstat()` reports errors
  2019-10-26 21:59 ` [PATCH 1/1] add: respect `--ignore-errors` when `lstat()` " qusielle via GitGitGadget
@ 2019-10-28  2:03   ` Junio C Hamano
  2019-11-03 20:17     ` qusielle
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2019-10-28  2:03 UTC (permalink / raw)
  To: qusielle via GitGitGadget; +Cc: git

"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(...);


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Re: [PATCH 1/1] add: respect `--ignore-errors` when `lstat()` reports errors
  2019-10-28  2:03   ` Junio C Hamano
@ 2019-11-03 20:17     ` qusielle
  0 siblings, 0 replies; 6+ messages in thread
From: qusielle @ 2019-11-03 20:17 UTC (permalink / raw)
  To: Junio C Hamano, qusielle via GitGitGadget; +Cc: git

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(...);
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 0/1] add: respect --ignore-errors when lstat() reports errors
  2019-10-26 21:59 [PATCH 0/1] add: respect --ignore-errors when lstat() reports errors Qusielle via GitGitGadget
  2019-10-26 21:59 ` [PATCH 1/1] add: respect `--ignore-errors` when `lstat()` " qusielle via GitGitGadget
@ 2019-11-03 20:26 ` Qusielle via GitGitGadget
  2019-11-03 20:26   ` [PATCH v2 1/1] add: respect `--ignore-errors` when `lstat()` " qusielle via GitGitGadget
  1 sibling, 1 reply; 6+ messages in thread
From: Qusielle via GitGitGadget @ 2019-11-03 20:26 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

"git add --ignore-errors" command fails immediately when lstat returns an
error, despite the ignore errors' flag is enabled.

There could be files that triggers an error on stat(), when other files
proceed correctly. Issue can be reproduced when running git under Cygwin and
some target files have utf-8 long names (200+ utf chars). Windows can handle
them, but all operations on them failed under Cygwin. Issue can not be
reproduced with usual latin/numeric only names. For example, create a file
with 220 'й' letters by Windows Explorer, then in Cygwin:

Here and below "ййй..." means the line of й copied 220 times.
=============================================================

$ echo -n 'ййй...' | wc -c # check the real size 440

$ ls -la ls: cannot access 'ййй...'$'\320': No such file or directory
-????????? ? ? ? ? ? 'ййй...'$'\320'

$ ls й* ls: cannot access 'ййй...'$'\320': No such file or directory

$ stat й* stat: cannot stat 'ййй...'$'\320': No such file or directory

In my perspective, it's okay to skip these problematic files when ignore
error flag is specified, but official Git terminates entire git add command
when such files come up. But with proposed patch it is the desired behavior:

$ git add --ignore-errors . error: ййй... can only add regular files,
symbolic links or git-directories

All other files have been added correctly.
==========================================

Signed-off-by: Qusielle <qusielle@gmail.com>

qusielle (1):
  add: respect `--ignore-errors` when `lstat()` reports errors

 read-cache.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


base-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-432%2Fqusielle%2Fmaster-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-432/qusielle/master-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/432

Range-diff vs v1:

 1:  fd022f88f5 ! 1:  d88ea544d9 add: respect `--ignore-errors` when `lstat()` reports errors
     @@ -46,7 +46,13 @@
       {
       	struct stat st;
      -	if (lstat(path, &st))
     -+	if (lstat(path, &st) && !(flags & ADD_CACHE_IGNORE_ERRORS))
     - 		die_errno(_("unable to stat '%s'"), path);
     +-		die_errno(_("unable to stat '%s'"), path);
     ++	if (lstat(path, &st)) {
     ++		if (flags & ADD_CACHE_IGNORE_ERRORS)
     ++			return -1;
     ++		else
     ++			die_errno(_("unable to stat '%s'"), path);
     ++	}
       	return add_to_index(istate, path, &st, flags);
       }
     + 

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/1] add: respect `--ignore-errors` when `lstat()` reports errors
  2019-11-03 20:26 ` [PATCH v2 0/1] add: respect --ignore-errors when lstat() " Qusielle via GitGitGadget
@ 2019-11-03 20:26   ` qusielle via GitGitGadget
  0 siblings, 0 replies; 6+ messages in thread
From: qusielle via GitGitGadget @ 2019-11-03 20:26 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, qusielle

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.

There could be files that triggers an error on stat(), when other files
proceed correctly.
Issue can be reproduced when running git under Cygwin and some target files
have utf-8 long names (200+ utf chars). Windows can handle them, but all
operations on them failed under Cygwin.
Issue can not be reproduced with usual latin/numeric only names.
For example, create a file with 220 'й' letters by Windows Explorer,
then in Cygwin:

 # Here and below "ййй..." means the line of й copied 220 times.
$ echo -n 'ййй...' | wc -c  # check the real size
440

$ ls -la
ls: cannot access 'ййй...'$'\320': No such file or directory
-????????? ? ?              ?        ?            ? 'ййй...'$'\320'

$ ls й*
ls: cannot access 'ййй...'$'\320': No such file or directory

$ stat й*
stat: cannot stat 'ййй...'$'\320': No such file or directory

In my perspective, it's okay to skip these problematic files when ignore
error flag is specified, but official Git terminates entire git add command
when such files come up. But with proposed patch it is the desired behavior:

$ git add --ignore-errors .
error: ййй... can only add regular files, symbolic links or git-directories
 # All other files have been added correctly.

Signed-off-by: Qusielle <qusielle@gmail.com>
---
 read-cache.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 133f790fa4..791ef65bac 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -801,8 +801,12 @@ 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))
-		die_errno(_("unable to stat '%s'"), path);
+	if (lstat(path, &st)) {
+		if (flags & ADD_CACHE_IGNORE_ERRORS)
+			return -1;
+		else
+			die_errno(_("unable to stat '%s'"), path);
+	}
 	return add_to_index(istate, path, &st, flags);
 }
 
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-11-03 20:27 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-26 21:59 [PATCH 0/1] add: respect --ignore-errors when lstat() reports errors Qusielle via GitGitGadget
2019-10-26 21:59 ` [PATCH 1/1] add: respect `--ignore-errors` when `lstat()` " qusielle via GitGitGadget
2019-10-28  2:03   ` Junio C Hamano
2019-11-03 20:17     ` qusielle
2019-11-03 20:26 ` [PATCH v2 0/1] add: respect --ignore-errors when lstat() " Qusielle via GitGitGadget
2019-11-03 20:26   ` [PATCH v2 1/1] add: respect `--ignore-errors` when `lstat()` " qusielle via GitGitGadget

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).