* [Fix v1] builtin/ls-files.c: add error check on lstat for modified files
@ 2019-02-17 16:34 randall.s.becker
2019-02-17 17:04 ` Ramsay Jones
2019-04-01 9:29 ` Junio C Hamano
0 siblings, 2 replies; 4+ messages in thread
From: randall.s.becker @ 2019-02-17 16:34 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker
From: "Randall S. Becker" <rsbecker@nexbridge.com>
The result from lstat, checking whether a file has been deleted, is now
included priot to calling id_modified when showing modified files. Prior
to this fix, it is possible that files that were deleted could show up
as being modified because the lstat error was unchecked.
Reported-by: Joe Ranieri <jranieri@grammatech.com>
Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
builtin/ls-files.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 29a8762d4..fc21f4795 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -348,7 +348,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
err = lstat(fullname.buf, &st);
if (show_deleted && err)
show_ce(repo, dir, ce, fullname.buf, tag_removed);
- if (show_modified && ie_modified(repo->index, ce, &st, 0))
+ if (show_modified && !err && ie_modified(repo->index, ce, &st, 0))
show_ce(repo, dir, ce, fullname.buf, tag_modified);
}
}
--
2.12.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Fix v1] builtin/ls-files.c: add error check on lstat for modified files
2019-02-17 16:34 [Fix v1] builtin/ls-files.c: add error check on lstat for modified files randall.s.becker
@ 2019-02-17 17:04 ` Ramsay Jones
2019-02-17 17:29 ` Randall S. Becker
2019-04-01 9:29 ` Junio C Hamano
1 sibling, 1 reply; 4+ messages in thread
From: Ramsay Jones @ 2019-02-17 17:04 UTC (permalink / raw)
To: randall.s.becker, git; +Cc: Randall S. Becker
On 17/02/2019 16:34, randall.s.becker@rogers.com wrote:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> The result from lstat, checking whether a file has been deleted, is now
> included priot to calling id_modified when showing modified files. Prior
s/priot/prior/; s/id_modified/ie_modified/
> to this fix, it is possible that files that were deleted could show up
> as being modified because the lstat error was unchecked.
>
> Reported-by: Joe Ranieri <jranieri@grammatech.com>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> builtin/ls-files.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 29a8762d4..fc21f4795 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -348,7 +348,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
> err = lstat(fullname.buf, &st);
> if (show_deleted && err)
To be pedantic, this should probably check for (err == ENOENT), since
lstat() can fail for several reasons which don't imply that the path
has been deleted. However, that is unlikely.
No reason to include such a check in this patch, of course.
ATB,
Ramsay Jones
> show_ce(repo, dir, ce, fullname.buf, tag_removed);
> - if (show_modified && ie_modified(repo->index, ce, &st, 0))
> + if (show_modified && !err && ie_modified(repo->index, ce, &st, 0))
> show_ce(repo, dir, ce, fullname.buf, tag_modified);
> }
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [Fix v1] builtin/ls-files.c: add error check on lstat for modified files
2019-02-17 17:04 ` Ramsay Jones
@ 2019-02-17 17:29 ` Randall S. Becker
0 siblings, 0 replies; 4+ messages in thread
From: Randall S. Becker @ 2019-02-17 17:29 UTC (permalink / raw)
To: 'Ramsay Jones', git; +Cc: 'Joe Ranieri'
On February 17, 2019 12:05, Ramsay Jones wrote:
> On 17/02/2019 16:34, randall.s.becker@rogers.com wrote:
> > From: "Randall S. Becker" <rsbecker@nexbridge.com>
> >
> > The result from lstat, checking whether a file has been deleted, is
> > now included priot to calling id_modified when showing modified files.
> > Prior
>
> s/priot/prior/; s/id_modified/ie_modified/
>
> > to this fix, it is possible that files that were deleted could show up
> > as being modified because the lstat error was unchecked.
> >
> > Reported-by: Joe Ranieri <jranieri@grammatech.com>
> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> > ---
> > builtin/ls-files.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/builtin/ls-files.c b/builtin/ls-files.c index
> > 29a8762d4..fc21f4795 100644
> > --- a/builtin/ls-files.c
> > +++ b/builtin/ls-files.c
> > @@ -348,7 +348,7 @@ static void show_files(struct repository *repo,
> struct dir_struct *dir)
> > err = lstat(fullname.buf, &st);
> > if (show_deleted && err)
>
> To be pedantic, this should probably check for (err == ENOENT), since
> lstat() can fail for several reasons which don't imply that the path has been
> deleted. However, that is unlikely.
That would be very platform specific error checking. lstat can fail for a variety of other reasons also leaving the file deleted (like a symlink issue), but you are correct. I was following the prior line's model of checking for consistency.
> No reason to include such a check in this patch, of course.
>
> ATB,
> Ramsay Jones
>
> > show_ce(repo, dir, ce, fullname.buf,
> tag_removed);
> > - if (show_modified && ie_modified(repo->index, ce,
> &st, 0))
> > + if (show_modified && !err && ie_modified(repo-
> >index, ce, &st, 0))
> > show_ce(repo, dir, ce, fullname.buf,
> tag_modified);
> > }
> > }
> >
This was just to address what Joe reported earlier. It seemed like an easy one to address.
Regards,
Randall
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Fix v1] builtin/ls-files.c: add error check on lstat for modified files
2019-02-17 16:34 [Fix v1] builtin/ls-files.c: add error check on lstat for modified files randall.s.becker
2019-02-17 17:04 ` Ramsay Jones
@ 2019-04-01 9:29 ` Junio C Hamano
1 sibling, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2019-04-01 9:29 UTC (permalink / raw)
To: randall.s.becker; +Cc: git, Randall S. Becker
randall.s.becker@rogers.com writes:
> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> The result from lstat, checking whether a file has been deleted, is now
> included priot to calling id_modified when showing modified files. Prior
s/priot/prior/
> to this fix, it is possible that files that were deleted could show up
> as being modified because the lstat error was unchecked.
>
> Reported-by: Joe Ranieri <jranieri@grammatech.com>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> builtin/ls-files.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
The justification for the change reads quite convincing. Is it
merely "it is _possible_ ... _could_ show up", though? The code is
iterating over the in-core index, so if you add a blob at path F in
the index then remove that regular file F from the working tree,
when it is the cache entry for "F"'s turn to get inspected, lstat()
would say ENOENT, (show_deleted && err) would show tag_removed, and
ie_modified() gets a garbage &st and ie_match_stat() would say
"modified", no?
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 29a8762d4..fc21f4795 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -348,7 +348,7 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
> err = lstat(fullname.buf, &st);
> if (show_deleted && err)
> show_ce(repo, dir, ce, fullname.buf, tag_removed);
> - if (show_modified && ie_modified(repo->index, ce, &st, 0))
> + if (show_modified && !err && ie_modified(repo->index, ce, &st, 0))
> show_ce(repo, dir, ce, fullname.buf, tag_modified);
> }
> }
And the implementation of the change looks OK.
I wonder if there is an easy way to cover this with a test or two.
Wouldn't it be just the matter of doing something like this
test_expect_success 'allow telling modified and deleted ones apart' '
>testfile &&
git add testfile &&
rm testfile &&
echo C testfile >expect &&
git ls-files -m -d -t testfile >actual &&
test_cmp expect actual
'
in some existing test file for ls-files, perhaps in t3004 (ls-files-basic)?
I went back to the original discussion of the "BUG" around mid Feb
2019, and didn't find anybody worried about backward compatibility.
As "ls-files -[dm...t]" is marked semi-deprecated, perhaps breaking
the current users does not matter that much ;-)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-04-01 9:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-17 16:34 [Fix v1] builtin/ls-files.c: add error check on lstat for modified files randall.s.becker
2019-02-17 17:04 ` Ramsay Jones
2019-02-17 17:29 ` Randall S. Becker
2019-04-01 9:29 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).