find command: don't abort on tree load errors

[cdhowie]

What is the purpose of this change? What does it change?

As per #2224, this changes find not to abort when a tree can't be found. Instead, the tree ID and referent snapshot ID are displayed. This makes it easier to recover a broken repository by forgetting any snapshots that refer to missing objects.

Closes #2224

Was the change discussed in an issue or in the forum before?

A few times on the forum:

• https://forum.restic.net/t/repository-damaged-after-accidentally-deleting-data-files/1516/17
• https://forum.restic.net/t/unable-to-check-or-prune/1239/12

Checklist

• I have read the Contribution Guidelines
• I have added tests for all changes in this PR
• I have added documentation for the changes (in the manual)
• There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
• I have run gofmt on the code in all commits
• All commit messages are formatted in the same style as the other commits in the repo
• I'm done, this Pull Request is ready for review

Comments on above:

• There's a few cursory tests for find, but not ones that use many of the options. This is also a particularly hard change to construct a test repository for, at least automatically.
• It was not documented before that find stops on a tree load error, so there aren't really any sensible changes to be made.

[codecov-io]

Codecov Report

Merging #2231 into master will decrease coverage by 4.27%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master    #2231      +/-   ##
==========================================
- Coverage   50.92%   46.65%   -4.28%     
==========================================
  Files         176      176              
  Lines       14265    14269       +4     
==========================================
- Hits         7265     6657     -608     
- Misses       5942     6611     +669     
+ Partials     1058     1001      -57

Impacted Files	Coverage Δ
cmd/restic/cmd_find.go	36.15% <14.28%> (-0.48%)	⬇️
internal/backend/b2/b2.go	0% <0%> (-80.69%)	⬇️
internal/backend/swift/swift.go	0% <0%> (-78.83%)	⬇️
internal/backend/gs/gs.go	0% <0%> (-74%)	⬇️
internal/backend/azure/azure.go	0% <0%> (-69.46%)	⬇️
internal/backend/swift/config.go	34.69% <0%> (-57.15%)	⬇️
internal/checker/checker.go	64.54% <0%> (-3.87%)	⬇️
internal/archiver/blob_saver.go	95.06% <0%> (+2.46%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 870e758...36f22a0. Read the comment docs.

[fd0]

Thank you very much for your contribution! The code looks great, just a minor thing: I'd like to suggest using Warnf() instead of Printf(), so the warning is printed to stderr instead of stdout. What do you think?

[fd0]

Suggested change

	Printf("Unable to load tree %s\n ... which belongs to snapshot %s.\n", parentTreeID, sn.ID())
	Warnf("Unable to load tree %s\n ... which belongs to snapshot %s.\n", parentTreeID, sn.ID())

[fd0]

Suggested change

	Printf("Unable to load tree %s\n ... which belongs to snapshot %s.\n", parentTreeID, sn.ID())
	Warnf("Unable to load tree %s\n ... which belongs to snapshot %s.\n", parentTreeID, sn.ID())

[cdhowie]

I think this one could go either way. Here is my take on it.

The point of having stdout and stderr is so that output can be separated from diagnostic information, particularly in the case where the output needs to be read by another program.

I'm not sure how find works with --json (I admittedly did not test it before submitting the PR). If it supports that, then we have to decide if this is diagnostic information for humans (it should go to stderr) or it's relevant results from the operation (it should be added to the JSON structure).

Not running in JSON mode, the same question applies: are these errors considered important output from the requested operation, or a diagnostic that should be relegated to a side-channel?

IMO, if you're running find with --blob or --tree then your repository has problems and you're trying to figure out why. In that case, I think that these warnings are directly related to the purpose of the command: someone is asking "what is wrong with my repository?" and therefore they should be part of the output.

But I will not argue if you'd rather have them on stderr because I think a case can be made for both options.

[fd0]

You made a good point and I agree, let's leave it on stdout for now. Thanks!