[PATCH 3/4] fsck: add/use standardized EXIT_TRUE and EXIT_FALSE

[Rob Browning]

We'll add other values later.

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---

Proposed for 0.33.x and main.

lib/bup/cmd/fsck.py | 8 +++++---
lib/bup/helpers.py | 5 ++++-
2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/bup/cmd/fsck.py b/lib/bup/cmd/fsck.py
index f0aac3583..accf75f7f 100644
--- a/lib/bup/cmd/fsck.py
+++ b/lib/bup/cmd/fsck.py
@@ -8,7 +8,9 @@ import glob, os, subprocess, sys

from bup import options, git
from bup.compat import argv_bytes
-from bup.helpers import Sha1, chunkyreader, istty2, log, progress, temp_dir
+from bup.helpers \
+ import (EXIT_FALSE, EXIT_TRUE,
+ Sha1, chunkyreader, istty2, log, progress, temp_dir)
from bup.io import byte_stream


@@ -210,9 +212,9 @@ def main(argv):
par2_setup()
if opt.par2_ok:
if par2_ok:
- sys.exit(0) # 'true' in sh
+ sys.exit(EXIT_TRUE)
else:
- sys.exit(1)
+ sys.exit(EXIT_FALSE)
if opt.disable_par2:
par2_ok = 0

diff --git a/lib/bup/helpers.py b/lib/bup/helpers.py
index 8c9338432..365c246be 100644
--- a/lib/bup/helpers.py
+++ b/lib/bup/helpers.py
@@ -1,6 +1,5 @@
"""Helper functions and classes for bup."""

-from __future__ import absolute_import, division
from collections import namedtuple
from contextlib import ExitStack
from ctypes import sizeof, c_void_p
@@ -21,6 +20,10 @@ from bup.io import byte_stream, path_msg
from bup.options import _tty_width as tty_width


+EXIT_TRUE = 0
+EXIT_FALSE = 1
+
+
buglvl = int(os.environ.get('BUP_DEBUG', 0))


--
2.43.0

[Rob Browning]

We only need a repository (to search for *.pack) when given no paths.

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---

Proposed for 0.33.x and main.

lib/bup/cmd/fsck.py | 3 +--
note/0.33.x.md | 3 +++
2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/bup/cmd/fsck.py b/lib/bup/cmd/fsck.py
index 10074c8e4..16a2f2178 100644
--- a/lib/bup/cmd/fsck.py
+++ b/lib/bup/cmd/fsck.py
@@ -195,12 +195,11 @@ def main(argv):

if opt.disable_par2:
par2_ok = 0

- git.check_repo_or_die()
-
if extra:
extra = [argv_bytes(x) for x in extra]
else:
debug('fsck: No filenames given: checking all packs.\n')
+ git.check_repo_or_die()
extra = glob.glob(git.repo(b'objects/pack/*.pack'))

sys.stdout.flush()
diff --git a/note/0.33.x.md b/note/0.33.x.md
index 35342fce1..32d739697 100644
--- a/note/0.33.x.md
+++ b/note/0.33.x.md
@@ -18,6 +18,9 @@ Bugs
on REMOTE ...` incorrectly reads the `pack.packSizeLimit` from the
`REMOTE` repository.

+* `bup fsck` no longer requires a repository via `BUP_DIR`, `-d`,
+ etc. when paths are provided on the command line.
+
Thanks to (at least)
====================

--
2.43.0

[Rob Browning]

If par2 generate is interrupted, it may leave empty *.par2 files, and
not just the top-level PATH.par2 index. To avoid that possiblity, run
par2 in a temporary directory and then move the recovery files to the
right place if par2 succeeds.

fsck currently checks whether the top-level index is empty, but
doesn't check any of the other vol files.

Thanks to Johannes Berg for reporting the problem.

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---

Proposed for 0.33.x and main.

lib/bup/cmd/fsck.py | 39 ++++++++++++++++++++++++++++++---------
note/0.33.x.md | 6 ++++++
2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/lib/bup/cmd/fsck.py b/lib/bup/cmd/fsck.py
index 16a2f2178..f0aac3583 100644
--- a/lib/bup/cmd/fsck.py
+++ b/lib/bup/cmd/fsck.py
@@ -1,14 +1,14 @@

-from __future__ import absolute_import, print_function
from shutil import rmtree
from subprocess import PIPE
from tempfile import mkdtemp
from binascii import hexlify
+from os.path import join

import glob, os, subprocess, sys

from bup import options, git
from bup.compat import argv_bytes
-from bup.helpers import Sha1, chunkyreader, istty2, log, progress

+from bup.helpers import Sha1, chunkyreader, istty2, log, progress, temp_dir
from bup.io import byte_stream


@@ -20,14 +20,14 @@ def debug(s):
if opt.verbose > 1:
log(s)

-def run(argv):
+def run(argv, *, cwd=None):
# at least in python 2.5, using "stdout=2" or "stdout=sys.stderr" below
# doesn't actually work, because subprocess closes fd #2 right before
# execing for some reason. So we work around it by duplicating the fd
# first.
fd = os.dup(2) # copy stderr
try:
- p = subprocess.Popen(argv, stdout=fd, close_fds=False)
+ p = subprocess.Popen(argv, stdout=fd, close_fds=False, cwd=cwd)
return p.wait()
finally:
os.close(fd)
@@ -70,7 +70,7 @@ def is_par2_parallel():

_par2_parallel = None

-def par2(action, args, verb_floor=0):
+def par2(action, args, verb_floor=0, cwd=None):
global _par2_parallel
if _par2_parallel is None:
_par2_parallel = is_par2_parallel()
@@ -82,12 +82,33 @@ def par2(action, args, verb_floor=0):
if _par2_parallel:
cmd.append(b'-t1')
cmd.extend(args)
- return run(cmd)
+ return run(cmd, cwd=cwd)

def par2_generate(base):
- return par2(b'create',
- [b'-n1', b'-c200', b'--', base, base + b'.pack', base + b'.idx'],
- verb_floor=2)
+ parent, name = os.path.split(base)
+ # Work in a temp_dir because par2 was observed creating empty
+ # files when interrupted by C-c.
+ # cf. https://github.com/Parchive/par2cmdline/issues/84
+ with temp_dir(dir=parent, prefix=(name + b'-bup-tmp-')) as tmpdir:
+ idx_name = name + b'.idx'
+ pack_name = name + b'.pack'
+ os.symlink(join(b'../', idx_name), join(tmpdir, idx_name))
+ os.symlink(join(b'../', pack_name), join(tmpdir, pack_name))
+ rc = par2(b'create',
+ [b'-n1', b'-c200', b'--', name, pack_name, idx_name],
+ verb_floor=2, cwd=tmpdir)
+ if rc == 0:
+ p2_idx = name + b'.par2'
+ for tmp in os.listdir(tmpdir):
+ if tmp in (p2_idx, idx_name, pack_name):
+ continue
+ os.rename(join(tmpdir, tmp), join(parent, tmp))
+ # Let this indicate success
+ os.rename(join(tmpdir, p2_idx), join(parent, p2_idx))
+ expected = frozenset((idx_name, pack_name))
+ remaining = frozenset(os.listdir(tmpdir))
+ assert expected == remaining
+ return rc

def par2_verify(base):
return par2(b'verify', [b'--', base], verb_floor=3)
diff --git a/note/0.33.x.md b/note/0.33.x.md
index 32d739697..1fbd153fa 100644
--- a/note/0.33.x.md
+++ b/note/0.33.x.md
@@ -4,6 +4,12 @@ Notable changes in main since 0.33.3 (incomplete)
May require attention
---------------------

+* The `par2` command (invoked by `bup fsck -g`) may generate empty
+ recovery files if interrupted (say via C-c). To mitigate this, bup
+ now runs `par2` in a temporary directory, and only moves the
+ recovery files into place if the generation succeeds. See also
+ https://github.com/Parchive/par2cmdline/issues/84
+
* Previously, any `bup on REMOTE ...` commands that attempted to read
from standard input (for example `bup on HOST split < something` or
`bup on HOST split --git-ids ...`) would read nothing instead of the
--
2.43.0

[Rob Browning]

Make sure all the par2 files aren't empty. Previously, we only
checked whether the top-level PACK.idx index file was empty, ignoring
the vol files. That meant that we might skip generation, even if the
vol files were broken.

Use stem instead of base in some cases to refer to the
/some/where/pack-HASH packfile "stems" since base is potentially
misleading, suggesting "basename", which we also need, e.g. this also
allows us to change "last" (which is the basename) to base.

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---

Proposed for 0.33.x and main.

lib/bup/cmd/fsck.py | 68 ++++++++++++++++++++++++++++-----------------
lib/bup/helpers.py | 1 +
note/0.33.x.md | 7 ++++-
3 files changed, 50 insertions(+), 26 deletions(-)

diff --git a/lib/bup/cmd/fsck.py b/lib/bup/cmd/fsck.py
index accf75f7f..cb1cbb45d 100644
--- a/lib/bup/cmd/fsck.py
+++ b/lib/bup/cmd/fsck.py
@@ -9,9 +9,9 @@ import glob, os, subprocess, sys

from bup import options, git
from bup.compat import argv_bytes

from bup.helpers \
- import (EXIT_FALSE, EXIT_TRUE,
+ import (EXIT_ERROR, EXIT_FALSE, EXIT_TRUE,
Sha1, chunkyreader, istty2, log, progress, temp_dir)
-from bup.io import byte_stream
+from bup.io import byte_stream, path_msg


par2_ok = 0
@@ -144,39 +144,63 @@ def git_verify(base):
return run([b'git', b'verify-pack', b'--', base])


-def do_pack(base, last, par2_exists, out):
+def do_pack(stem, out):
+ pack_dir, base = os.path.split(stem)
+
+ # Check par2 status. We look for empty *.par2 files because C-c
+ # during "par2 create" may produce them. For now, we decide the
+ # existing data is OK if the pack-HASH.par2 index file exists, and
+ # no pack-HASH*.par2 files are empty.
+ par2_idx = base + b'.par2'
+ par2_idx_ent, par2_empty = None, False
+ empty_severity = 'warning:' if opt.generate else 'error:'
+ for ent in [ent for ent in os.scandir(pack_dir)
+ if ent.name.startswith(base) and ent.name.endswith(b'.par2')]:
+ if ent.name == par2_idx:
+ par2_idx_ent = ent
+ if ent.stat().st_size == 0:
+ log(f'{empty_severity} empty file - {path_msg(ent.name)}\n')
+ par2_empty = True
+ if par2_empty:
+ if not opt.generate:
+ # FIXME: could print multiple times for parallel jobs
+ log(f'{empty_severity} please remove empty (broken) *.par2 files\n')
+ return EXIT_ERROR
+
+ par2_exists = par2_idx_ent and not par2_empty
+
code = 0
if par2_ok and par2_exists and (opt.repair or not opt.generate):
- vresult = par2_verify(base)
+ vresult = par2_verify(stem)
if vresult != 0:
if opt.repair:
- rresult = par2_repair(base)
+ rresult = par2_repair(stem)
if rresult != 0:
action_result = b'failed'
- log('%s par2 repair: failed (%d)\n' % (last, rresult))
+ log('%s par2 repair: failed (%d)\n' % (base, rresult))
code = rresult
else:
action_result = b'repaired'
- log('%s par2 repair: succeeded (0)\n' % last)
+ log('%s par2 repair: succeeded (0)\n' % base)
code = 100
else:
action_result = b'failed'
- log('%s par2 verify: failed (%d)\n' % (last, vresult))
+ log('%s par2 verify: failed (%d)\n' % (base, vresult))
code = vresult
else:
action_result = b'ok'
elif not opt.generate or (par2_ok and not par2_exists):
- gresult = git_verify(base)
+ gresult = git_verify(stem)
if gresult != 0:
action_result = b'failed'
- log('%s git verify: failed (%d)\n' % (last, gresult))
+ log('%s git verify: failed (%d)\n' % (base, gresult))
code = gresult
else:
if par2_ok and opt.generate:
- presult = par2_generate(base)
+ presult = par2_generate(stem)
if presult != 0:
action_result = b'failed'
- log('%s par2 create: failed (%d)\n' % (last, presult))
+ log('%s par2 create: failed (%d)\n' % (base, presult))
code = presult
else:
action_result = b'generated'
@@ -186,7 +210,7 @@ def do_pack(base, last, par2_exists, out):
assert(opt.generate and (not par2_ok or par2_exists))
action_result = b'exists' if par2_exists else b'skipped'
if opt.verbose:
- out.write(last + b' ' + action_result + b'\n')
+ out.write(base + b' ' + action_result + b'\n')
return code


@@ -232,27 +256,21 @@ def main(argv):
outstanding = {}
for name in extra:
if name.endswith(b'.pack'):
- base = name[:-5]
+ stem = name[:-5]
elif name.endswith(b'.idx'):
- base = name[:-4]
+ stem = name[:-4]
elif name.endswith(b'.par2'):
- base = name[:-5]
+ stem = name[:-5]
elif os.path.exists(name + b'.pack'):
- base = name
+ stem = name
else:
raise Exception('%r is not a pack file!' % name)
- (dir,last) = os.path.split(base)
- par2_exists = os.path.exists(base + b'.par2')
- if par2_exists and os.stat(base + b'.par2').st_size == 0:
- par2_exists = 0
sys.stdout.flush() # Not sure we still need this, but it'll flush out too
- debug('fsck: checking %r (%s)\n'
- % (last, par2_ok and par2_exists and 'par2' or 'git'))
if not opt.verbose:
progress('fsck (%d/%d)\r' % (count, len(extra)))

if not opt.jobs:
- nc = do_pack(base, last, par2_exists, out)
+ nc = do_pack(stem, out)
code = code or nc
count += 1
else:
@@ -268,7 +286,7 @@ def main(argv):
outstanding[pid] = 1
else: # child
try:
- sys.exit(do_pack(base, last, par2_exists, out))
+ sys.exit(do_pack(stem, out))
except Exception as e:
log('exception: %r\n' % e)
sys.exit(99)
diff --git a/lib/bup/helpers.py b/lib/bup/helpers.py
index 365c246be..c609d6ed5 100644
--- a/lib/bup/helpers.py
+++ b/lib/bup/helpers.py
@@ -22,6 +22,7 @@ from bup.options import _tty_width as tty_width

EXIT_TRUE = 0
EXIT_FALSE = 1
+EXIT_ERROR = 2

buglvl = int(os.environ.get('BUP_DEBUG', 0))

diff --git a/note/0.33.x.md b/note/0.33.x.md
index 1fbd153fa..e41a1f92f 100644
--- a/note/0.33.x.md
+++ b/note/0.33.x.md
@@ -7,7 +7,12 @@ May require attention

* The `par2` command (invoked by `bup fsck -g`) may generate empty

recovery files if interrupted (say via C-c). To mitigate this, bup

now runs `par2` in a temporary directory, and only moves the

- recovery files into place if the generation succeeds. See also
+ recovery files into place if the generation succeeds. It will also
+ look for any existing, empty par2 files associated with packfiles it
+ has been asked to examine. If found, they will provoke an error
+ unless bup has been asked to `--generate` new ones, in which case
+ bup will just issue a warning, since the pending `par2 generate ...`
+ should correct the problem. See also
https://github.com/Parchive/par2cmdline/issues/84

* Previously, any `bup on REMOTE ...` commands that attempted to read

--
2.43.0

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> Proposed for 0.33.x and main.

Thinking about making a release once this set of changes is in 0.33.x.
Finding out your par2 files were empty just when you needed them would
be...bad.

Thanks
--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

[Johannes Berg]

On Sat, 2024-06-15 at 15:50 -0500, Rob Browning wrote:
> @@ -210,9 +212,9 @@ def main(argv):
> par2_setup()
> if opt.par2_ok:
> if par2_ok:
> - sys.exit(0) # 'true' in sh
> + sys.exit(EXIT_TRUE)
> else:
> - sys.exit(1)
> + sys.exit(EXIT_FALSE)

Hm. Now that I think about it again - are these really good names? I get
that in any shell these are true/false, and there literally are 'true'
and 'false' that do nothing but exit with status 0/1, but ...

What does it mean for 'bup fsck' to return 'true'? I kind of have a
vague feeling that we might be better off with something like

EXIT_SUCCESS = 0
EXIT_FAIL = 1
EXIT_FAIL_<SPECIFIC> = 2

or something, because really we don't have any 'boolean-ish' commands
like "bup is-dir-initialized" or whatever?

Also "false" is kind of hard to really pin down: anything non-zero is
false in the context that this refers to, but if we want to capture
different error states then what does that it mean if all are false, but
only one is _named_ false?

But ... bike-shedding.

johannes

[Johannes Berg]

On Sat, 2024-06-15 at 15:50 -0500, Rob Browning wrote:

> If found, they will provoke an error
> + unless bup has been asked to `--generate` new ones, in which case
> + bup will just issue a warning, since the pending `par2 generate ...`
> + should correct the problem.

Did you try this? I'm pretty sure I ran into this issue because par2
generate did in fact _not_ correct the problem, but rather errored out
on the next round generating, and I had to manually delete the 0-sized
files, so perhaps bup should do that?

johannes

[Rob Browning]

Johannes Berg <joha...@sipsolutions.net> writes:

> Did you try this? I'm pretty sure I ran into this issue because par2
> generate did in fact _not_ correct the problem, but rather errored out
> on the next round generating

I thought I did, but I'll double-check.

> and I had to manually delete the 0-sized files, so perhaps bup should
> do that?

Right -- I'd wondered if we might want to do that anyway, though decided
it'd be a bit more "do no harm" to just warn and leave it up to the user
as long as par2 would correct all the paths it cares about.

[Rob Browning]

Johannes Berg <joha...@sipsolutions.net> writes:

> Hm. Now that I think about it again - are these really good names? I get
> that in any shell these are true/false, and there literally are 'true'
> and 'false' that do nothing but exit with status 0/1, but ...
>
> What does it mean for 'bup fsck' to return 'true'?

Semantically, nothing. I actually wondered about having two 0 aliases,
i.e.

EXIT_OK = 0
EXIT_TRUE = 0
EXIT_FALSE = 1 # only for grep-like boolean false
EXIT_ERROR = 2 # when we don't want to say anything more specific
EXIT_<SOMETHING_MORE_SPECIFIC> = ...

And of course EXIT_TRUE would only be used by any current or future bup
subcommands that really did have a grep-like boolean result. (I suppose
you could argue we should use EXIT_SUCCESS as you did to match C99 and
posix...)

For now, I also wasn't bothering to try to sort out our other random
error codes. Eventually I imagine we'll want to clarify (and
*document*) some of them, but possibly not all.

> Also "false" is kind of hard to really pin down: anything non-zero is
> false in the context that this refers to, but if we want to capture
> different error states then what does that it mean if all are false, but
> only one is _named_ false?

Perhaps I've clarified sufficiently, but FALSE is intended to be special
in the same way it is for grep (or say bash test[1]), i.e. FALSE means
nothing went wrong, but the answer to the question is "no".

[1] e.g. "test -ne 1 nope" returns 2.

[Greg Troxel]

Rob Browning <r...@defaultvalue.org> writes:

> And of course EXIT_TRUE would only be used by any current or future bup
> subcommands that really did have a grep-like boolean result. (I suppose
> you could argue we should use EXIT_SUCCESS as you did to match C99 and
> posix...)

My immediate reaction is to follow posix absent a compelling reason not
to.

/usr/include/stdlib.h:#define EXIT_FAILURE 1
/usr/include/stdlib.h:#define EXIT_SUCCESS 0

>> Also "false" is kind of hard to really pin down: anything non-zero is
>> false in the context that this refers to, but if we want to capture
>> different error states then what does that it mean if all are false, but
>> only one is _named_ false?
>
> Perhaps I've clarified sufficiently, but FALSE is intended to be special
> in the same way it is for grep (or say bash test[1]), i.e. FALSE means
> nothing went wrong, but the answer to the question is "no".
>
> [1] e.g. "test -ne 1 nope" returns 2.

I guess it's interesting to contemplate that fsck can have

- all tests completed, everything is ok
- all tests completed (with no syscall errors), but the bits are not ok
- an error was encountered while testing, such as EIO from read(2)

and you are mapping the first case to EXIT_SUCCESS and the other to to
EXIT_FAILURE, which seems ok to me.

[Rob Browning]

Greg Troxel <g...@lexort.com> writes:

> I guess it's interesting to contemplate that fsck can have
>
> - all tests completed, everything is ok
> - all tests completed (with no syscall errors), but the bits are not ok
> - an error was encountered while testing, such as EIO from read(2)
>
> and you are mapping the first case to EXIT_SUCCESS and the other to to
> EXIT_FAILURE, which seems ok to me.

At the moment, I'm not really even as concerned about fsck specifically.
The main thing I'm interested in is in general, never returning 1 from
any commands when an "actual error" (whatever that means) has occurred,
i.e. always reserve 1 for "true" as a matter of policy so that either
now of later, it'll be available.

Of course, various commands (maybe fsck) are complex enough that we
might consider having multiple meaningful values (and we do, albeit with
no documentation). Though past some point, I imagine we'd be better off
with a richer response channel, even if it's just some form of well
specified output on stdout).

In any case, I'm not proposing any kind of wholesale overhaul, just (at
least) a preference when it's feasible, and/or we're already making
changes.

[Rob Browning]

Greg Troxel <g...@lexort.com> writes:

> It would be good to document a plan (in DESIGN.md), because while this
> seems reasonable, it's a departure from what people might expect.

I'd plan to document it in the manpage for each command that actually
ends up with any true/false cases. Otherwise, "not zero" is an error,
and that's all afaik that POSIX specifies. Also seems fine, because
it's opt-in, i.e., you'll know you ran a command that's running a test
where 1 is "no" (as with grep).

But right, also reasonable to mention the intent in DESIGN for anyone
hacking on bup in the future.

Really, this is just a effort/intent to leave open the well-established
precedent set by (at least) grep and test as an option.

Thanks

[Rob Browning]

Rob Browning <r...@defaultvalue.org> writes:

> Johannes Berg <joha...@sipsolutions.net> writes:
>
>> Did you try this? I'm pretty sure I ran into this issue because par2
>> generate did in fact _not_ correct the problem, but rather errored out
>> on the next round generating
>
> I thought I did, but I'll double-check.

I must have misunderstood whatever I did before because you're right, it
crashes here with

Could not create "....vol000+01.par2": File already exists.

>> and I had to manually delete the 0-sized files, so perhaps bup should
>> do that?
>
> Right -- I'd wondered if we might want to do that anyway, though decided
> it'd be a bit more "do no harm" to just warn and leave it up to the user
> as long as par2 would correct all the paths it cares about.

So what to do... Perhaps remove all pack-HASH*.par2 files if any one of
them is zero length for --generate. (Will also add some tests.)

Thanks much

[Johannes Berg]

On Tue, 2024-06-18 at 20:40 -0500, Rob Browning wrote:
>
> I must have misunderstood whatever I did before because you're right, it
> crashes here with
>
> Could not create "....vol000+01.par2": File already exists.

Right, was pretty sure that's how I'd noticed the issue in the first
place.

> So what to do... Perhaps remove all pack-HASH*.par2 files if any one of
> them is zero length for --generate. (Will also add some tests.)

I'd be really wary of removing any non-zero sized files, they _might_
contain useful data? Maybe delete zero-sized ones, and (loudly?) move
the others out of the way?

At least theoretically it seems possible that the original files are
already corrupt at this point and then maybe you'd want to have as much
data as possible?

johannes

[Rob Browning]

Hah, I'd just been about to reply to myself that after thinking about
it, I'd decided against removals too.

Here's what I was contemplating:

* for a check, exit with EXIT_FAILURE when there are 0 length
pack-HASH*.par2 files.

* for --repair, just attempt the repair and let par2 tell us
something's wrong.

* for --generate, log an error (suggesting removal) and skip packs
with zero length .par2 files. Then exit with EXIT_FAILURE if any
were found.

And of course, now that we'll only be moving the par2 files into place
if par2 succeeds, it should be unlikely we'll end up creating new empty
files. (Of course, all of this also assumes that a par2 file can never
legitimately be empty.)

Thanks

[Rob Browning]

We only need a repository (to search for *.pack) when given no paths.

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---

lib/bup/cmd/fsck.py | 3 +--
note/0.33.x.md | 3 +++

2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/bup/cmd/fsck.py b/lib/bup/cmd/fsck.py
index 10074c8e4..16a2f2178 100644
--- a/lib/bup/cmd/fsck.py
+++ b/lib/bup/cmd/fsck.py

@@ -195,12 +195,11 @@ def main(argv):
if opt.disable_par2:
par2_ok = 0

- git.check_repo_or_die()
-
if extra:
extra = [argv_bytes(x) for x in extra]
else:
debug('fsck: No filenames given: checking all packs.\n')
+ git.check_repo_or_die()
extra = glob.glob(git.repo(b'objects/pack/*.pack'))

sys.stdout.flush()

diff --git a/note/0.33.x.md b/note/0.33.x.md
index 35342fce1..32d739697 100644
--- a/note/0.33.x.md
+++ b/note/0.33.x.md

[Greg Troxel]

It would be good to document a plan (in DESIGN.md), because while this
seems reasonable, it's a departure from what people might expect.

Rob Browning <r...@defaultvalue.org> writes:

> At the moment, I'm not really even as concerned about fsck specifically.
> The main thing I'm interested in is in general, never returning 1 from
> any commands when an "actual error" (whatever that means) has occurred,
> i.e. always reserve 1 for "true" as a matter of policy so that either
> now of later, it'll be available.

Except that in exit status space, 0 is success (true) and 1 is an error.
Or maybe you mean some test result, and neither result is an error of
the test program or considered an error status. If 1 is true, then what
is false (predicate not found to be true, but no error)?

> Of course, various commands (maybe fsck) are complex enough that we
> might consider having multiple meaningful values (and we do, albeit with
> no documentation). Though past some point, I imagine we'd be better off
> with a richer response channel, even if it's just some form of well
> specified output on stdout).

This seems better to me. Something as simple as the following would be
nonconfusing to people.

TOKEN optional words for humans

or maybe just skip the rest and have it be TOKEN, in the man page. With
fsck, I could see exit 0 is "I ran fsck, I had no system call errors,
and everything was ok". With or without OK on stdout, and I lean to
with.

[Rob Browning]