Re: [PATCH] ls-files: do not trust stat info if lstat() fails
Duy Nguyen pclo...@gmail.com writes: Or even better to show an error message when the error code is unexpected? The unkown tag '!' says there are problems but if it shows up sort of permanently, '!' won't help much, I think. I am OK with that approach, but then one question remains: should we say it is deleted, modified, both, or neither? The question is moot if the user does not ignore stderr because they should just ignore those error-reported entries. If they do 2/dev/null, I think we should err on the safe side and say modified. We only say deleted if lstat() returns ENOENT or ENOTDIR like in your patch. Doesn't the same reasoning behind when we do not know for sure that a path is not modified, it would be safe if we said the path may be modified also tell us that it is safer to say a path may be lost if we cannot tell? One likely case where we cannot tell if it is modified would be when we cannot read the path (perhaps the parent directory accidentally lost its x-bit). Saying it may be modified would be one way to have the user take notice, for an interactive user. A script that runs ls-files may be using the paths to drive git add, tar cf -, etc. and emitting such an unreadable path is one way to make these downstream commands signal that something fishy is going on by erroring out. So, I am not sure if we should be silent on an unexpected error when we are asked to report deletes when we would be vocal when we are asked to report modifications. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ls-files: do not trust stat info if lstat() fails
On Thu, Apr 3, 2014 at 11:30 PM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: On Thu, Apr 3, 2014 at 1:15 AM, Junio C Hamano gits...@pobox.com wrote: I am guessing that, even though this was discovered during the development of list-files, is a fix applicable outside the context of that series. I do think the patched result is an improvement than the status quo, but at the same time, I find it insufficient in the context of the whole codepath. What if errno were other than ENOENT and we were told to show_deleted (with or without show_modified)? We would end up saying the path was deleted and modified at the same time, when we do not know either is or is not true at all, because of the failure to lstat() the path. Wouldn't it be saner to add tag_unknown and do something like this instead, I wonder? Or even better to show an error message when the error code is unexpected? The unkown tag '!' says there are problems but if it shows up sort of permanently, '!' won't help much, I think. I am OK with that approach, but then one question remains: should we say it is deleted, modified, both, or neither? The question is moot if the user does not ignore stderr because they should just ignore those error-reported entries. If they do 2/dev/null, I think we should err on the safe side and say modified. We only say deleted if lstat() returns ENOENT or ENOTDIR like in your patch. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ls-files: do not trust stat info if lstat() fails
On Thu, Apr 3, 2014 at 1:15 AM, Junio C Hamano gits...@pobox.com wrote: I am guessing that, even though this was discovered during the development of list-files, is a fix applicable outside the context of that series. I do think the patched result is an improvement than the status quo, but at the same time, I find it insufficient in the context of the whole codepath. What if errno were other than ENOENT and we were told to show_deleted (with or without show_modified)? We would end up saying the path was deleted and modified at the same time, when we do not know either is or is not true at all, because of the failure to lstat() the path. Wouldn't it be saner to add tag_unknown and do something like this instead, I wonder? Or even better to show an error message when the error code is unexpected? The unkown tag '!' says there are problems but if it shows up sort of permanently, '!' won't help much, I think. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ls-files: do not trust stat info if lstat() fails
Duy Nguyen pclo...@gmail.com writes: On Thu, Apr 3, 2014 at 1:15 AM, Junio C Hamano gits...@pobox.com wrote: I am guessing that, even though this was discovered during the development of list-files, is a fix applicable outside the context of that series. I do think the patched result is an improvement than the status quo, but at the same time, I find it insufficient in the context of the whole codepath. What if errno were other than ENOENT and we were told to show_deleted (with or without show_modified)? We would end up saying the path was deleted and modified at the same time, when we do not know either is or is not true at all, because of the failure to lstat() the path. Wouldn't it be saner to add tag_unknown and do something like this instead, I wonder? Or even better to show an error message when the error code is unexpected? The unkown tag '!' says there are problems but if it shows up sort of permanently, '!' won't help much, I think. I am OK with that approach, but then one question remains: should we say it is deleted, modified, both, or neither? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ls-files: do not trust stat info if lstat() fails
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
If 'err' is non-zero, lstat() has failed. Consider the entry modified
without passing the (unreliable) stat info to ce_modified() in this
case.
Noticed-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
On Fri, Mar 28, 2014 at 11:04 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com
wrote:
+ err = lstat(ce-name, st);
+ if (show_deleted err) {
+ show_ce_entry(tag_removed, ce);
+ shown = 1;
+ }
+ if (show_modified ce_modified(ce, st, 0)) {
Is it possible for the lstat() to have failed for some reason when we
get here? If so, relying upon 'st' is unsafe, isn't it?
The chance of random stat making ce_modified() return false is pretty
low, but you're right. This code is a copy from the old show_files().
I'll fix it in the git-ls series. Meanwhile a patch for maint to fix
the original function.
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 47c3880..e6bd00e 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -260,7 +260,7 @@ static void show_files(struct dir_struct *dir)
err = lstat(ce-name, st);
if (show_deleted err)
show_ce_entry(tag_removed, ce);
- if (show_modified ce_modified(ce, st, 0))
+ if (show_modified (err || ce_modified(ce, st, 0)))
show_ce_entry(tag_modified, ce);
}
}
I am guessing that, even though this was discovered during the
development of list-files, is a fix applicable outside the context
of that series.
I do think the patched result is an improvement than the status quo,
but at the same time, I find it insufficient in the context of the
whole codepath. What if errno were other than ENOENT and we were
told to show_deleted (with or without show_modified)? We would end
up saying the path was deleted and modified at the same time, when
we do not know either is or is not true at all, because of the
failure to lstat() the path.
Wouldn't it be saner to add tag_unknown and do something like this
instead, I wonder?
builtin/ls-files.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 47c3880..af2ce99 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -46,6 +46,7 @@ static const char *tag_killed = ;
static const char *tag_modified = ;
static const char *tag_skip_worktree = ;
static const char *tag_resolve_undo = ;
+static const char *tag_unknown = ;
static void write_name(const char *name)
{
@@ -249,7 +250,7 @@ static void show_files(struct dir_struct *dir)
for (i = 0; i active_nr; i++) {
const struct cache_entry *ce = active_cache[i];
struct stat st;
- int err;
+
if ((dir-flags DIR_SHOW_IGNORED)
!ce_excluded(dir, ce))
continue;
@@ -257,11 +258,15 @@ static void show_files(struct dir_struct *dir)
continue;
if (ce_skip_worktree(ce))
continue;
- err = lstat(ce-name, st);
- if (show_deleted err)
- show_ce_entry(tag_removed, ce);
- if (show_modified ce_modified(ce, st, 0))
+ errno = 0;
+ if (lstat(ce-name, st)) {
+ if (errno != ENOENT errno != ENOTDIR)
+ show_ce_entry(tag_unknown, ce);
+ else if (show_deleted)
+ show_ce_entry(tag_removed, ce);
+ } else if (show_modified ce_modified(ce, st, 0)) {
show_ce_entry(tag_modified, ce);
+ }
}
}
}
@@ -533,6 +538,7 @@ int cmd_ls_files(int argc, const char **argv, const char
*cmd_prefix)
tag_killed = K ;
tag_skip_worktree = S ;
tag_resolve_undo = U ;
+ tag_unknown = ! ;
}
if (show_modified || show_others || show_deleted || (dir.flags
DIR_SHOW_IGNORED) || show_killed)
require_work_tree = 1;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ls-files: do not trust stat info if lstat() fails
If 'err' is non-zero, lstat() has failed. Consider the entry modified
without passing the (unreliable) stat info to ce_modified() in this
case.
Noticed-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
On Fri, Mar 28, 2014 at 11:04 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Wed, Mar 26, 2014 at 9:48 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com
wrote:
+ err = lstat(ce-name, st);
+ if (show_deleted err) {
+ show_ce_entry(tag_removed, ce);
+ shown = 1;
+ }
+ if (show_modified ce_modified(ce, st, 0)) {
Is it possible for the lstat() to have failed for some reason when we
get here? If so, relying upon 'st' is unsafe, isn't it?
The chance of random stat making ce_modified() return false is pretty
low, but you're right. This code is a copy from the old show_files().
I'll fix it in the git-ls series. Meanwhile a patch for maint to fix
the original function.
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 47c3880..e6bd00e 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -260,7 +260,7 @@ static void show_files(struct dir_struct *dir)
err = lstat(ce-name, st);
if (show_deleted err)
show_ce_entry(tag_removed, ce);
- if (show_modified ce_modified(ce, st, 0))
+ if (show_modified (err || ce_modified(ce, st, 0)))
show_ce_entry(tag_modified, ce);
}
}
--
1.9.1.345.ga1a145c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
