diff --git a/src/htsback.c b/src/htsback.c index c00bd8ef..646082a8 100644 --- a/src/htsback.c +++ b/src/htsback.c @@ -585,14 +585,6 @@ static int create_back_tmpfile(httrackp *opt, lien_back *const back, return 0; } -/* Move src onto dst; RENAME does not clobber an existing target on Windows. */ -static hts_boolean replace_file(const char *src, const char *dst) { - if (RENAME(src, dst) == 0) - return HTS_TRUE; - (void) UNLINK(dst); - return RENAME(src, dst) == 0 ? HTS_TRUE : HTS_FALSE; -} - /* Commit or restore a re-fetch backup (#77 follow-up): a re-fetch over an existing file moved the good copy to back->tmpfile before truncating url_sav. commit keeps the new file and drops the backup; else restore it so an aborted @@ -610,7 +602,7 @@ static void back_finalize_backup(httrackp *opt, lien_back *const back, } /* On failure keep the backup: an orphaned temp beats losing the good copy. */ - if (!replace_file(back->tmpfile, back->url_sav)) + if (!hts_rename_over(back->tmpfile, back->url_sav)) hts_log_print(opt, LOG_WARNING | LOG_ERRNO, "could not restore %s; previous copy kept as %s", back->url_sav, back->tmpfile); @@ -741,7 +733,7 @@ int back_finalize(httrackp * opt, cache_back * cache, struct_back * sback, "Read error when decompressing"); } UNLINK(unpacked); - } else if (replace_file(unpacked, back[p].url_sav)) { + } else if (hts_rename_over(unpacked, back[p].url_sav)) { /* The temp bypassed filecreate(), which is what chmods. */ #ifndef _WIN32 chmod(back[p].url_sav, HTS_ACCESS_FILE); diff --git a/src/htssinglefile.c b/src/htssinglefile.c index 31a2f6ab..a65b9de2 100644 --- a/src/htssinglefile.c +++ b/src/htssinglefile.c @@ -1108,13 +1108,8 @@ hts_boolean singlefile_rewrite_file(httrackp *opt, const char *root, (void) chmod(fconv(catbuff, sizeof(catbuff), StringBuff(tmp)), HTS_ACCESS_FILE); #endif - if (ok) { - /* RENAME does not clobber an existing target on Windows. */ - if (RENAME(StringBuff(tmp), page_path) != 0) { - (void) UNLINK(page_path); - ok = RENAME(StringBuff(tmp), page_path) == 0 ? HTS_TRUE : HTS_FALSE; - } - } + if (ok) + ok = hts_rename_over(StringBuff(tmp), page_path); if (!ok) { hts_log_print(opt, LOG_ERROR, "single-file: could not rewrite %s", page_path); diff --git a/src/htstools.c b/src/htstools.c index b4ea3859..8cb159e2 100644 --- a/src/htstools.c +++ b/src/htstools.c @@ -1443,3 +1443,15 @@ HTSEXT_API hts_boolean hts_findissystem(find_handle find) { } return 0; } + +hts_boolean hts_rename_over(const char *src, const char *dst) { + char csrc[CATBUFF_SIZE], cdst[CATBUFF_SIZE]; + + fconv(csrc, sizeof(csrc), src); + fconv(cdst, sizeof(cdst), dst); + if (RENAME(csrc, cdst) == 0) + return HTS_TRUE; + /* RENAME does not clobber an existing target on Windows. */ + (void) UNLINK(cdst); + return RENAME(csrc, cdst) == 0 ? HTS_TRUE : HTS_FALSE; +} diff --git a/src/htstools.h b/src/htstools.h index 7a6e1849..0548bfbc 100644 --- a/src/htstools.h +++ b/src/htstools.h @@ -137,6 +137,10 @@ HTSEXT_API hts_boolean hts_findisdir(find_handle find); HTSEXT_API hts_boolean hts_findisfile(find_handle find); HTSEXT_API hts_boolean hts_findissystem(find_handle find); +/* Move src onto dst, replacing an existing dst; HTS_TRUE on success. Both + paths are fconv()'d. */ +hts_boolean hts_rename_over(const char *src, const char *dst); + #endif #endif diff --git a/src/htswarc.c b/src/htswarc.c index 9f23b816..2b799dcc 100644 --- a/src/htswarc.c +++ b/src/htswarc.c @@ -964,16 +964,6 @@ static zipFile wacz_zip_open(const char *path) { return zipOpen2_64(path, 0 /*create*/, NULL, &ff); } -/* Move src onto dst (UTF-8/Windows-safe); RENAME won't clobber on Windows, so - fall back to unlink+rename. Returns 0 on success. */ -static int wacz_rename_over(const char *src, const char *dst) { - char cs[CATBUFF_SIZE], cd[CATBUFF_SIZE]; - if (RENAME(fconv(cs, sizeof(cs), src), fconv(cd, sizeof(cd), dst)) == 0) - return 0; - (void) UNLINK(fconv(cd, sizeof(cd), dst)); - return RENAME(fconv(cs, sizeof(cs), src), fconv(cd, sizeof(cd), dst)); -} - /* Package the segment(s) + .cdx + a generated pages.jsonl into .wacz at crawl end (the archive file(s) and .cdx are already closed on disk). */ static void warc_wacz_package(warc_writer *w) { @@ -1092,7 +1082,7 @@ static void warc_wacz_package(warc_writer *w) { hts_log_print(w->opt, LOG_WARNING, "WACZ: packaging failed, kept existing %s untouched", waczpath); - } else if (wacz_rename_over(tmppath, waczpath) != 0) { + } else if (!hts_rename_over(tmppath, waczpath)) { (void) UNLINK(fconv(catbuff, sizeof(catbuff), tmppath)); hts_log_print(w->opt, LOG_WARNING | LOG_ERRNO, "WACZ: could not finalize %s", waczpath); diff --git a/tests/43_local-update-truncate.test b/tests/43_local-update-truncate.test index c3d6fe9a..4671688d 100644 --- a/tests/43_local-update-truncate.test +++ b/tests/43_local-update-truncate.test @@ -18,6 +18,7 @@ set -euo pipefail bash "$top_srcdir/tests/local-crawl.sh" \ --rerun-args '--update -M400000' \ --log-found 'More than 400000 bytes have been transferred.. giving up' \ + --log-not-found 'could not restore' \ --found bigtrunc/slow.bin \ --file-min-bytes bigtrunc/slow.bin 655360 \ --file-min-bytes bigtrunc/fast.bin 655360 \ diff --git a/tests/74_local-warc-wacz.test b/tests/74_local-warc-wacz.test index 16c617a1..2e081799 100755 --- a/tests/74_local-warc-wacz.test +++ b/tests/74_local-warc-wacz.test @@ -5,11 +5,16 @@ # real gate: STORE-mode entries, the fixed layout, recomputed sha256 digests and # the datapackage-digest chain (plus py-wacz/pywb when importable). The crawl # skips cleanly on a build without OpenSSL (no conformant SHA-256 -> no package). +# +# The cacheless second pass repackages over the .wacz the first one left: the +# clobber, not the happy rename, is what breaks when a platform refuses to +# rename onto an existing file (#726). set -eu : "${top_srcdir:=..}" bash "$top_srcdir/tests/local-crawl.sh" --errors 0 --wacz-validate \ + --rerun-args '-C0' --log-not-found 'could not finalize' \ --found 'mini304/index.html' --found 'mini304/page.html' \ httrack 'BASEURL/mini304/index.html' --warc-file warc-out --wacz diff --git a/tests/local-crawl.sh b/tests/local-crawl.sh index ce3777a9..5ab3f41c 100755 --- a/tests/local-crawl.sh +++ b/tests/local-crawl.sh @@ -57,6 +57,8 @@ rerun_dead= tmpdir= serverpid= crawlpid= +wacz_poisoned= +wacz_poison="stale-wacz-that-a-second-pass-must-replace" function warning { echo "** $*" >&2 @@ -272,6 +274,14 @@ if test -n "$warc_validate"; then test -z "$w1" || cp "$w1" "${tmpdir}/warc-pass1.gz" fi +# Poison the first-pass .wacz when a second pass follows: repackaging moves the +# new archive over it, so the marker must be gone afterwards (#726). Poisoning +# beats comparing the two packages, which can come out byte-identical. +if test -n "$wacz_validate" && test -n "${rerun}${rerun_args}"; then + wacz_poisoned=$(find "$mirrorroot" -maxdepth 2 -name '*.wacz' 2>/dev/null | sort | tail -n1) + test -z "$wacz_poisoned" || echo "$wacz_poison" >"$wacz_poisoned" +fi + # --- optional second pass: re-mirror into the same dir (cache/update path) ---- if test -n "$rerun"; then info "re-running httrack (update pass)" @@ -416,6 +426,12 @@ if test -n "$wacz_validate"; then fi die "no .wacz file produced under $mirrorroot" fi + if test -n "$wacz_poisoned"; then + info "checking the second pass replaced the .wacz" + grep -q "$wacz_poison" "$wacz_poisoned" 2>/dev/null && + die "stale .wacz kept: $wacz_poisoned" + result "OK" + fi validator=$(nativepath "${testdir}/wacz-validate.py") info "validating WACZ package" "$python" "$validator" "$(nativepath "$wacz")" >&2 || die "WACZ validation failed"