Tom Lane wrote:
> Bruce Momjian <[EMAIL PROTECTED]> writes:
> >> Tom Lane wrote:
> >>> The cases that I think we most need to defend against are
> >>> (A) diff program not found
>
> > In summary, on MinGW, files differ or 'diff' not found, returns 1. If
> > one of the files to be compared does not exist, it returns 2. And of
> > course, if the files are the same, it returns zero.
>
> OK. The problem here is that pg_regress is coded to assume that
> zero-length output file represents success. Given the above Windows
> behavior that is *clearly* not good enough, because that's probably
> exactly what we will see after diff-not-found (if the Windows shell
> acts like a Unix shell does and creates the ">" target first).
>
> I'd suggest modifying the logic so that zero-length output file with a
> nonzero return from the child be treated as a fatal condition (not just
> a difference, but bail out).
I modified pg_regress.c to use just the return code to determine if the
diff worked, but I added in a WIN32-specific test for the file size. I
think that is the cleanest solution. Attached.
--
Bruce Momjian [EMAIL PROTECTED]
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: src/include/port/win32.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/port/win32.h,v
retrieving revision 1.53
diff -c -c -r1.53 win32.h
*** src/include/port/win32.h 16 Jul 2006 20:17:04 -0000 1.53
--- src/include/port/win32.h 29 Jul 2006 02:21:57 -0000
***************
*** 109,120 ****
/*
! * Signal stuff
*/
! #define WEXITSTATUS(w) (((w) >> 8) & 0xff)
! #define WIFEXITED(w) (((w) & 0xff) == 0)
! #define WIFSIGNALED(w) (((w) & 0x7f) > 0 && (((w) & 0x7f) < 0x7f))
! #define WTERMSIG(w) ((w) & 0x7f)
#define sigmask(sig) ( 1 << ((sig)-1) )
--- 109,125 ----
/*
! * Signal stuff
! * WIN32 doesn't have wait(), so the return value for children
! * is simply the return value specified by the child, without
! * any additional information on whether the child terminated
! * on its own or via a signal. These macros are also used
! * to interpret the return value of system().
*/
! #define WEXITSTATUS(w) (w)
! #define WIFEXITED(w) (true)
! #define WIFSIGNALED(w) (false)
! #define WTERMSIG(w) (0)
#define sigmask(sig) ( 1 << ((sig)-1) )
Index: src/test/regress/pg_regress.c
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/pg_regress.c,v
retrieving revision 1.16
diff -c -c -r1.16 pg_regress.c
*** src/test/regress/pg_regress.c 27 Jul 2006 15:37:19 -0000 1.16
--- src/test/regress/pg_regress.c 29 Jul 2006 02:22:23 -0000
***************
*** 799,827 ****
}
/*
! * Run a "diff" command and check that it didn't crash
*/
! static void
! run_diff(const char *cmd)
{
int r;
r = system(cmd);
- /*
- * XXX FIXME: it appears that include/port/win32.h's definitions of
- * WIFEXITED and related macros may be wrong. They certainly don't
- * work for inspecting the results of system(). For the moment,
- * hard-wire the check on Windows.
- */
- #ifndef WIN32
if (!WIFEXITED(r) || WEXITSTATUS(r) > 1)
- #else
- if (r != 0 && r != 1)
- #endif
{
fprintf(stderr, _("diff command failed with status %d: %s\n"), r, cmd);
exit_nicely(2);
}
}
/*
--- 799,826 ----
}
/*
! * Run a "diff" command and also check that it didn't crash
*/
! static int
! run_diff(const char *cmd, const char *filename)
{
int r;
r = system(cmd);
if (!WIFEXITED(r) || WEXITSTATUS(r) > 1)
{
fprintf(stderr, _("diff command failed with status %d: %s\n"), r, cmd);
exit_nicely(2);
}
+ #ifdef WIN32
+ if (WEXITSTATUS(r) == 1 && file_size(filename) == 0)
+ {
+ fprintf(stderr, _("diff command not found: %s\n"), cmd);
+ exit_nicely(2);
+ }
+ #endif
+
+ return WEXITSTATUS(r);
}
/*
***************
*** 844,850 ****
int best_line_count;
int i;
int l;
!
/* Check in resultmap if we should be looking at a different file */
expectname = testname;
for (rm = resultmap; rm != NULL; rm = rm->next)
--- 843,849 ----
int best_line_count;
int i;
int l;
!
/* Check in resultmap if we should be looking at a different file */
expectname = testname;
for (rm = resultmap; rm != NULL; rm = rm->next)
***************
*** 872,883 ****
snprintf(cmd, sizeof(cmd),
SYSTEMQUOTE "diff %s \"%s\" \"%s\" > \"%s\"" SYSTEMQUOTE,
basic_diff_opts, expectfile, resultsfile, diff);
- run_diff(cmd);
/* Is the diff file empty? */
! if (file_size(diff) == 0)
{
- /* No diff = no changes = good */
unlink(diff);
return false;
}
--- 871,880 ----
snprintf(cmd, sizeof(cmd),
SYSTEMQUOTE "diff %s \"%s\" \"%s\" > \"%s\"" SYSTEMQUOTE,
basic_diff_opts, expectfile, resultsfile, diff);
/* Is the diff file empty? */
! if (run_diff(cmd, diff) == 0)
{
unlink(diff);
return false;
}
***************
*** 896,906 ****
snprintf(cmd, sizeof(cmd),
SYSTEMQUOTE "diff %s \"%s\" \"%s\" > \"%s\"" SYSTEMQUOTE,
basic_diff_opts, expectfile, resultsfile, diff);
- run_diff(cmd);
! if (file_size(diff) == 0)
{
- /* No diff = no changes = good */
unlink(diff);
return false;
}
--- 893,901 ----
snprintf(cmd, sizeof(cmd),
SYSTEMQUOTE "diff %s \"%s\" \"%s\" > \"%s\"" SYSTEMQUOTE,
basic_diff_opts, expectfile, resultsfile, diff);
! if (run_diff(cmd, diff) == 0)
{
unlink(diff);
return false;
}
***************
*** 921,927 ****
snprintf(cmd, sizeof(cmd),
SYSTEMQUOTE "diff %s \"%s\" \"%s\" >> \"%s\"" SYSTEMQUOTE,
pretty_diff_opts, best_expect_file, resultsfile, difffilename);
! run_diff(cmd);
/* And append a separator */
difffile = fopen(difffilename, "a");
--- 916,922 ----
snprintf(cmd, sizeof(cmd),
SYSTEMQUOTE "diff %s \"%s\" \"%s\" >> \"%s\"" SYSTEMQUOTE,
pretty_diff_opts, best_expect_file, resultsfile, difffilename);
! run_diff(cmd, difffilename);
/* And append a separator */
difffile = fopen(difffilename, "a");
---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match
