Re: mingw and same-inode

[Jim Meyering]

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 <address@hidden>
> 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)