Eric Blake wrote:
> I'm currently testing these two patches, as mingw prerequisites before I can
> get linkat() working. In particular, mingw is lousy at SAME_INODE, since all
> three of [fl]stat produce st_ino == 0 for all files (then again, mingw never
> claimed POSIX compliance!). Code was always taking the identical-file path,
> even for distinct files.
>
> I'm also preparing a followup patch for coreutils usage of SAME_INODE.
> Thoughts before I apply this?
You have pushed these changes already.
Imagine my surprise upon seeing unreviewed and unapproved API changes...
Accidentally, no doubt, since I would expect you to wait at least
for an "ok" from me before making API-changing changes to such
fundamental modules.
I haven't reviewed everything yet, but this error
jumped out at me:
bool
same_name (const char *source, const char *dest)
{
...
bool same = false;
...
same = SAME_INODE (source_dir_stats, dest_dir_stats);
if (same < 0)
same = (identical_basenames
&& strcmp (source_basename, dest_basename) == 0);
...
return same;
}
Your new test, "same < 0" will never be true when
compiled on a system with proper "bool" support.
Apparently, since your testing did not expose this flaw,
no tested use of same_name requires the new semantics of SAME_INODE.
I have profound doubts about whether this is a desirable change.
Since it is solely in support of non-POSIX platforms,
I find it debatable, to say the least. You've converted the
clearly-boolean SAME_INODE to be tri-state, with no way (other
than inspection) for callers to detect the need to update
their code.
If the tri-state test is required in only a few places,
how about using a new macro, say SAME_INODE_TRISTATE,
in just those few places?
With a name that makes it obvious the result is not boolean,
there is far less risk of misuse.
>>From 5538c910e1acd307a3d3e69f6d80044b1f3d935a Mon Sep 17 00:00:00 2001
> From: Eric Blake <e...@byu.net>
> Date: Wed, 23 Sep 2009 14:51:29 -0600
> Subject: [PATCH 2/2] same-inode: make SAME_INODE tri-state, to port to mingw
>
> Mingw has the annoying habit (already documented in
> doc/posix-functions/*stat) that st_ino is always 0.
> This means that naive uses of SAME_INODE(a,b) would
> succeed, even on distinct files.
...
> diff --git a/NEWS b/NEWS
> index 62c631f..87fc884 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -6,6 +6,9 @@ User visible incompatible changes
>
> Date Modules Changes
>
> +2009-09-23 same-inode The macro SAME_INODE is now tri-state, adding -1
> + for unknown.
> +
...
> diff --git a/lib/same.c b/lib/same.c
> index af3a95e..5251fb8 100644
> --- a/lib/same.c
> +++ b/lib/same.c
> @@ -1,7 +1,7 @@
> /* Determine whether two file names refer to the same file.
>
> - Copyright (C) 1997, 1998, 1999, 2000, 2002, 2003, 2004, 2005, 2006 Free
> - Software Foundation, Inc.
> + Copyright (C) 1997, 1998, 1999, 2000, 2002, 2003, 2004, 2005, 2006,
> + 2009 Free Software Foundation, Inc.
>
> This program is free software: you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> @@ -97,6 +97,9 @@ same_name (const char *source, const char *dest)
> }
>
> same = SAME_INODE (source_dir_stats, dest_dir_stats);
> + if (same < 0)
> + same = (identical_basenames
> + && strcmp (source_basename, dest_basename) == 0);
>
> #if ! _POSIX_NO_TRUNC && HAVE_PATHCONF && defined _PC_NAME_MAX
> if (same && ! identical_basenames)
