cp -r stops with an exception #2329
Labels
No Label
0.2.0
0.3.0
0.4.0
0.5.0
0.5.1
0.6.0
0.6.1
0.7.0
0.8.0
0.9.0
1.0.0
1.1.0
1.10.0
1.10.1
1.10.2
1.10a2
1.11.0
1.12.0
1.12.1
1.13.0
1.14.0
1.15.0
1.15.1
1.2.0
1.3.0
1.4.1
1.5.0
1.6.0
1.6.1
1.7.0
1.7.1
1.7β
1.8.0
1.8.1
1.8.2
1.8.3
1.8β
1.9.0
1.9.0-s3branch
1.9.0a1
1.9.0a2
1.9.0b1
1.9.1
1.9.2
1.9.2a1
LeastAuthority.com automation
blocker
cannot reproduce
cloud-branch
code
code-dirnodes
code-encoding
code-frontend
code-frontend-cli
code-frontend-ftp-sftp
code-frontend-magic-folder
code-frontend-web
code-mutable
code-network
code-nodeadmin
code-peerselection
code-storage
contrib
critical
defect
dev-infrastructure
documentation
duplicate
enhancement
fixed
invalid
major
minor
n/a
normal
operational
packaging
somebody else's problem
supercritical
task
trivial
unknown
was already fixed
website
wontfix
worksforme
No Milestone
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Reference: tahoe-lafs/trac-2024-07-25#2329
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
When I tried the same thing with the Tahoe-LAFS v1.10.0 release (from Ubuntu), it worked! So this is a regression.
Possibly caused by the fix to #712 (which was the last patch to touch the code where the error occurs: source:trunk/src/allmydata/scripts/tahoe_cp.py?annotate=blame#L767 )
Test (which fails) at https://github.com/tahoe-lafs/tahoe-lafs/commit/2329-test-0.
Review needed for the test.
warner will review the test, commit it with a TODO flag, then try to solve the original problem
So it looks like the intention of #712 was to make a command like
cp -r A LOCALorcp -r A LOCAL/createLOCAL/A. This depends upon "A" including a file or directory name (egcp -r DIRCAP/foo.txt LOCAL/). But if A is a pure cap, then it has no human-meaningful name. What should get created in this case?I think the #2329 bug is resulting from a code path that assumes it will be provided with a name, and the pure-cap source argument doesn't give it one.
Well, what did it do in Tahoe-LAFS v1.10.0? (There are two cases: $CAP is a file cap, or a directory cap.)
From a tahoe-side tree with PARENTCAP, DIRCAP=PARENTCAP/dir, and FILECAP=PARENTCAP/dir/file.txt, and a local (real) target directory "local":
cp -r PARENTCAP/dir local/->local/file.txtcp -r DIRCAP local/->local/file.txtcp -r DIRCAP_ALIAS: local/->local/file.txtcp -r DIRCAP/file.txt local/->local/file.txtcp -r FILECAP local/-> "error, you must specify a destination filename"local/andlocalbehave the same waycp -r PARENTCAP/dir local/->local/dir/file.txtcp -r DIRCAP local/-> (exception)cp -r DIRCAP_ALIAS: local/-> (exception)cp -r DIRCAP/file.txt local/->local/file.txtcp -r FILECAP local/-> "error, you must specify a destination filename"#712 fixed F to behave more like regular POSIX
/bin/cp. I can think of three ways we might go with case G and H:1:
cp -r DIRCAP local/should behave like an imaginarycp -r DIRCAP/* local/would do (this is imaginary because we have no tahoe-side globs). In this example, it'd createlocal/file.txt. This would match what it did in 1.10 (case B/C), but wouldn't match F. That'd be a shame, because we generally claim that "PARENTCAP/dir" and "DIRCAP" and "ALIAS:" are all interchangeable.2:
cp -r DIRCAP local/would pretend that the source directory was named after the actual DIRCAP string (the ugly base32 representation), and createlocal/BASE32DIRCAP/file.txt. Ick. For H we could use the alias name as a target name, so it'd createlocal/ALIASNAME/file.txt, which is still lame but at least human-readable.3:
cp -r DIRCAP local/would complain "you must specify a destination directory name", like cases E and J. This is somewhat plausible for G, and looks slightly weird for H.We don't have enough information to make case G behave like F.
Well, I guess I'm going to implement option 1: make
cp -r DIRCAP local/behave like it used to (putting the contents of DIRCAP into local/) instead of like howcp -r PARENTCAP/dir local/does now (making a new subdirectory under local/). It's not super consistent, but it's better than an exception.+1 for option 1.
When I said +1 for option 1, I was thinking that the previous behaviour of cases (B) and (C) --i.e. copying the contents of a dircap into a directory-- would become inexpressible. However I now understand the error in option 3 to be saying that "you must specify a previously nonexisting destination directory name". In that case you could still write:
cp -r DIRCAP local/missing -> local/missingwhich would be consistent with cases (F) and (I), because
local/missingbecomes a copy ofDIRCAP.If there were a tahoe-side globbing feature, the error messages in option 3 could say:
"""
To copy the contents of this dircap into an existing destination directory append /*; eg:
To create a new directory with the contents of this dircap, specify a new local directory name; eg:
A glob feature would allow the explicit indication of whether a bag, or the contents of the bag, should be copied.
Splitting by whether the destination exists or not:
cp -r PARENTCAP/dir local/->local/file.txtcp -r PARENTCAP/dir local/missing-> ?cp -r DIRCAP local/->local/file.txtcp -r DIRCAP local/missing-> ?cp -r DIRCAP_ALIAS: local/->local/file.txtcp -r DIRCAP_ALIAS: local/missing-> ?cp -r DIRCAP/file.txt local/->local/file.txtcp -r DIRCAP/file.txt local/missing->local/missingcp -r FILECAP local/-> "error, you must specify a destination filename"cp -r FILECAP local/missing->local/missinglocal/andlocalbehave the same waycp -r PARENTCAP/dir local/->local/dir/file.txtcp -r PARENTCAP/dir local/missing-> ?cp -r DIRCAP local/-> (exception)cp -r DIRCAP local/missing-> (exception?)cp -r DIRCAP_ALIAS: local/-> (exception)cp -r DIRCAP_ALIAS: local/missing-> (exception?)cp -r DIRCAP/file.txt local/->local/file.txtcp -r DIRCAP/file.txt local/missing->local/missingcp -r FILECAP local/-> "error, you must specify a destination filename"cp -r FILECAP local/missing->local/missingHere's a proposal:
We determine whether the source means the bag, or the contents of the bag, according to this rule:
Note that the question of whether the source means the bag or the contents of the bag is not influenced by whether the target exists or doesn't exist.
This is another way of expressing proposal 1 from comment:96152.
I'm convinced globbing is too complicated if it evolves towards emulating bash. The key feature I was interested in comment:96156 was an explicit consistent way to disambiguate between container and contents.
So this could also be a
--contentsflag, or another non-globby-looking symbol, such as the presence of a trailing slash (which I believe warner suggested elsewhere).Examples:
Note that this can be orthogonal to whether or not dest/ exists.
Some semiconcrete examples:
tahoe cp -r myalias: foo/ # Error: "myalias:" has no name. To copy the contents into foo/ append a /
tahoe cp -r myalias:/ foo/ # foo will contain the contents of myalias after this.
tahoe cp -r URI:DIR2... foo/ # Error: a dircap has no name, to copy the contents, append a /
tahoe cp -r URI:DIR2.../ foo # Copy the contents of the cap into foo.
tahoe cp -r URI:DIR2.../blah foo # ./foo/blah/ will contain the contents.
tahoe cp -r URI:DIR2.../blah/ foo # ./foo will contain the contents.
Here's my attempt to fill out the table from comment:96157 for a certain rule that I have in my mind right now (written below).
cp -r PARENTCAP/dir local/->local/dir/file.txtcp -r PARENTCAP/dir local/missing->local/missing/dir/file.txtcp -r DIRCAP local/->local/file.txtcp -r DIRCAP local/missing->local/missing/file.txtcp -r DIRCAP_ALIAS: local/->local/file.txtcp -r DIRCAP_ALIAS: local/missing->local/missing/file.txtcp -r DIRCAP/file.txt local/->local/file.txtcp -r DIRCAP/file.txt local/missing->local/missingcp -r FILECAP local/-> "error, you must specify a destination filename"cp -r FILECAP local/missing->local/missing"Rule comment:-1" is:
If the source is a directory:
If the target is the name of a locally existing file, then "error: there is already a local file present under the name $TARGET".
If the target is the name of something not locally existing, then mkdir it and then use it as "target directory".
If the target is the name of a locally existing directory, then proceed to use it as "target directory".
Check whether the source directory has a name (as in F1, F2) or has no name (as in G1, G2, H1, H2). If it has a name then we say that the source means the bag itself — the directory, and if it has no name then we say that the source means the contents of the bag — the contents of the directory.
Now if the source is the bag itself, then mkdir a new directory inside "target directory", named by the name of the source directory, and copy the contents of the bag into "target directory"/"source directory name"/ (which are cases F1 and F2).
Else (the source was the contents of the bag instead of the bag itself) copy the contents of the bag into "target directory"/ (which are cases G1, G2, H1, and H2).
If the source is a file, then check if the target is an existing directory.
If the source is a nameless file (as in J1, J2) and target is an existing directory (as in J1), then "error, you must specify a destination filename".
Else, if the source is a nameless file and the target is not an existing directory, then use the target as the local filename (which is case J2).
Else, if the source is a named file and the target is an existing directory, then use the source filename within the target existing directory (which is case I1).
Else, if the source is a named file and the target is not an existing directory, then use the target as the local filename (which is case I2).
"Rule comment:96160" could in the future be extended by additional syntax to indicate that the user wants to copy the contents-of-the-bag instead of the bag, in those cases where the bag had a name. comment:96159 suggests such additional syntax. (Side note: I have been confused in the past by the presence-or-absence-of-trailing-slash, such as rsync uses, to indicate this sort of thing. I'd prefer a very long and explicit thing like a command-line switch
--contents-of.)Another argument against globbing is that the argument would have to be quoted -- and if it were not, strange things would happen on Unix (I think the argument would appear to be omitted assuming there is no local file that matches the glob).
Ok, here's the current state of affairs:
cp -r X local/, wherelocalalready exists:cp -r X local/missing, wherelocalexists butmissingdoes not:build_graphs(),NoneType has no attribute startswitherror: you must specify a destination filenameput_file()line 156,nameis None but precondition requiresisinstance(unicode)I think zooko's rule in comment:96160 / comment:-1 gives us:
cp -r X local/, wherelocalalready exists:cp -r X local/missing, wherelocalexists butmissingdoes not:Although:
Feel free to edit the table in this comment if I interpreted your
proposal incorrectly.
warner: are you sure the table is right for
cp -R DIRCAP/file local/missing, columns 1.10 and trunk? I would have expected the resulting file to be atlocal/missing.My original comment:96160 was badly misformatted which hid some of the structure of my proposed rules. Here it is again, unchanged except for proper formatting.
Here's my attempt to fill out the table from comment:96157 for a certain rule that I have in my mind right now (written below).
cp -r PARENTCAP/dir local/->local/dir/file.txtcp -r PARENTCAP/dir local/missing->local/missing/dir/file.txtcp -r DIRCAP local/->local/file.txtcp -r DIRCAP local/missing->local/missing/file.txtcp -r DIRCAP_ALIAS: local/->local/file.txtcp -r DIRCAP_ALIAS: local/missing->local/missing/file.txtcp -r DIRCAP/file.txt local/->local/file.txtcp -r DIRCAP/file.txt local/missing->local/missingcp -r FILECAP local/-> "error, you must specify a destination filename"cp -r FILECAP local/missing->local/missing"Rule comment:96160" is:
.1. If the source is a directory:
.a. If the target is the name of a locally existing file, then "error: there is already a local file present under the name $TARGET".
.b. If the target is the name of something not locally existing, then mkdir it and then use it as "target directory".
.c. If the target is the name of a locally existing directory, then proceed to use it as "target directory".
.d. Check whether the source directory has a name (as in F1, F2) or has no name (as in G1, G2, H1, H2). If it has a name then we say that the source means the bag itself — the directory, and if it has no name then we say that the source means the contents of the bag — the contents of the directory.
.2. If the source is a file, then check if the target is an existing directory.
.a. If the source is a nameless file (as in J1, J2) and target is an existing directory (as in J1), then "error, you must specify a destination filename".
.b. Else, if the source is a nameless file and the target is not an existing directory, then use the target as the local filename (which is case J2).
.c. Else, if the source is a named file and the target is an existing directory, then use the source filename within the target existing directory (which is case I1).
.d. Else, if the source is a named file and the target is not an existing directory, then use the target as the local filename (which is case I2).
Replying to warner:
That sounds okay to me.
I think the table is an accurate reflection of the comment:96160 / comment:96166 proposal.
Note that this could potentially interact with #2027.
Replying to daira:
Ping in case you missed this question.
D2 and I2, right? Yeah, those give local/missing/file.txt in both cases.
Ok, I agree that that's a bit weird.
/bin/cpdoesn't do that:/bin/cp parent/dir/file.txt local/missingcreateslocal/missing, and/bin/cp -rdoes the same thing.I guess I need to add table entries for what unix does. If possible/applicable, we should match unix behavior.
I will try to implement zooko's algorithm from comment:96166 , and enhance the table to mention what happens with
/bin/cpin both no-flag,-r, and-Rcases.We discussed this during Nuts and Bolts, and I was persuaded that we should Zooko's comment:96166 algorithm despite the difference from
/bin/cp(as tested on OS X). The clinching arguments were:/bin/cpdoesn't behave consistently across platforms anyway. Its different treatment of a trailing slash on some platforms is confusing and I don't think we should emulate that.Updated table for
cp [-r] X local/, wherelocalalready exists:|
And for
cp [-r] X local/missing, wherelocalexists butmissingdoes not:|
build_graphs(),NoneType has no attribute startswithput_file()line 156,nameis None but precondition requiresisinstance(unicode)I updated comment:96174 with the proposed behaviour from comment:96166. There is one case without
-rthat I didn't know how to fill in:tahoe cp PARENTCAP/dir local/. (Eitherlocal/fileor ERROR-4b are reasonable possibilities.)In TC&C, we agreed that
tahoe cp PARENTCAP/dir local/should also give ERROR-4.I've updated comment:96174 to reflect that (the rule is that you must give -r to copy a directory, so a directory-like source without -r gives an error). I think our table is now up to date.
Looking at the table, with an eye towards writing docs that explain what has changed, the new "cp -r PARENTCAP/dir local/missing" case (F2, bottom row) stands out. It's consistent with current trunk, but not with 1.10, or the proposed behavior for other directory-like sources, or with bin/cp. It'd be easier to explain if it were
local/missing/fileinstead. I'm sure we've discussed this to death.. I've been too busy figuring out how to format that table. I'll reread the ticket and re-understand the rationale for that one.Next steps:
--helpdocstringReplying to warner:
Let's see…
That is intentional in the comment:96166 algorithm. The reasoning is twofold:
First of all, the question of whether you want the bag vs. the contents of the bag is determined by whether the directory-like source has a name (the
Fcolumn) vs. doesn't have a name (theGandHcolumns). That's a nice simple rule, and according to that, the result in the "comment:96166 -r" row has to belocal/missing/dir/fileinstead oflocal/missing/file, because the latter would be just the contents of the bag (file) instead of the bag (dir/file).So looking at the table, the cells of column
Fneed to result indir/file(if they aren't instead an error) and the cells of columnsGandHneed to result infile(if they aren't instead an error).Second, the comment:96166 algorithm behaves the same way whether the target exists or doesn't exist at the beginning of the algorithm. This is (in my intuition) a nice simple rule, and it also means there isn't a race condition in which the behavior is unpredictable because the existence of the target is unpredictable.
So looking at the table, that means the result of
F1andF2both need to result in the same behavior as each other.I'm not saying this rationale is better than other rationales that would justify other designs (such as “This is as much like
/bin/cpas we could make it.”, or “The behavior is the same for all directory-like sources.”), but that's the rationale for this design.Yay!
Yay!
Yay!
Yay!
☺
Thank you for your good work on this.
I've got a branch which adds a test that exercises the full table. While studying how to change the code to let that test pass, I came across another wrinkle:
cpaccepts multiple source arguments.In general, if you have multiple source arguments, then the target must be a directory. If the target is a directory, then you can't use unnamed files as sources (one or multiple). The only case that accepts an unnamed file as a source is when you're copying exactly one of them to a target that is (or will be) a file.
Here's a list of what I think should happen (I tried to compress some of the cases.. let me know if it doesn't seem to cover everything):
Next step is to figure out how to turn this into a flowchart for
tahoe_cp.Copier.try_copy.. I've started on the internal refactorings to make this easier (I was wrong before when I thought the basename should be tracked from outside ofTahoeFileSource/etc.. treating it as a possibly-empty property of the source instance is totally the right way to do it).You're missing a case:
cp NAMEDANDUNNAMEDFILES.. X: error: need a name (if there is >1 source of which >=1 is unnamed)Similarly for
cp -R X.. TO-DIR:Ok, updates:
(https://github.com/warner/tahoe-lafs/tree/2329) has a branch that adds the tests (and some useful refactoring). It does not yet make any behavior changes. Take a look at the new
test_cli_cp.CopyOutand see if it covers all those cases.Ok, the patch is ready for review: https://github.com/tahoe-lafs/tahoe-lafs/pull/143 . I rewrote the target-assignment code, cleaning up an awful lot in the process. I tried to make the diff as minimal as possible, but it may still look a bit ugly. The core 100-line cluster of functions was replaced by a different 100-line cluster of functions, but there's enough overlap that 'git diff' tries too hard to show you line-by-line changes, and does it badly. My best advice for reviewing it is to print out those 100 lines before, and those 100 lines after, and compare those two printouts, instead of looking at a detailed diff.
Two unexpected cases discovered during today's review:
We decided the first should signal an error: the trailing slash will only be accepted if the target is directory-like (either a pre-existing directory, or something missing that will be created as a directory, either because the source is a directory, or because there are multiple sources). If the target is directory-like, the trailing slash will not affect the copying behavior.
For the second, the realization was that this causes the contents of DIR1 and DIR2 to be merged together in the TARGETDIR, and thus the order of the source arguments matters (presumeably DIR2's contents will overwrite those of DIR1). The same thing happens if you copy multiple files of the same name into a directory (
cp foo/file.txt bar/file.txt outdir).We need to ensure the implementation preserves the order of the source arguments (I think my current patch does, but if I'd used a set instead of a dict, it would behave differently). We should also double-check that
/bin/cpdoes the same thing.I guess there are a couple of different forms we should examine.
cp -r foo/dir bar/dir TARGETDIRcp -r DIRCAP1 DIRCAP2 TARGETDIRThe first is using two named directories that both happen to have the same name. This will cause
TARGETDIR/dirto be created, and the contents of the sources merged into it.The second is using unnamed directories, triggering our "unnamed directories refer to the contents, not the bag itself" rule. Their contents will be merged into TARGETDIR.
/bin/cpdoesn't have unnamed directories, so the only way to simulate the contents-not-bag rule is to use/bin/cp -r DIR1/* DIR2/* TARGETDIR, in which case any name collisions are obvious on the post-glob-expansion argv that's passed from the shell tocp. I think (but I'm not sure) that the only way to wind up with collisions in/bin/cpis if the basename of two SOURCE arguments are the same, since/bin/cpalways uses the basename of the source to create a child of the target.(in other words,
/bin/cpmay not actually give us precedent to follow)I think I was able to compress the rules above into the following set of assertions. These are the ones implemented by the code:
Copying files into files is easy. Copying things into directories requires looking at the type of each source object:
New pull request is up: https://github.com/tahoe-lafs/tahoe-lafs/pull/144
This incorporates all the review feedback so far. It also rejects trailing slashes on file-like targets, ensures in-order copies of colliding sources (last source wins), fixes a bug I uncovered that preventing colliding sources from working anyways, and adds a bunch more test cases.
Reviewing.
Ok, that PR (plus review feedback) has been landed, in e60392a. Remaining work: docs updates, and some notes for future cleanups.
Some notes from our review discussion, which didn't suggest changes to the current behavior, but which should be recorded for future analysis:
cp SOURCE1 SOURCE2 missingis not so clear-cut. Current behavior is to mkdir, but maybe it should throw an error instead.cp SOURCE1 SOURCE2 missing/is obviously referring to a directory, so mkdir is more correct. (current behavior is to mkdir). (/bin/cpemits an error).cp -r SOURCE1 SOURCEDIR2 missingis not clear-cut.Cleanups to do:
docs changes are in https://github.com/tahoe-lafs/tahoe-lafs/pull/162
In /tahoe-lafs/trac-2024-07-25/commit/97fd19407d610de298c45500f7e1ad3e62b8a263:
ok, I think that's a wrap.
In /tahoe-lafs/trac-2024-07-25/commit/ca23c4fa23a77b1fa557c734a2ad1f2abe4e7688: