( )⚙ D10142 rhg: `cat` command: print error messages for missing files

This is an archive of the discontinued Mercurial Phabricator instance.

rhg: `cat` command: print error messages for missing files
ClosedPublic

Authored by SimonSapin on Mar 9 2021, 4:41 AM.

Details

Summary

And exit with an error code if no file was matched.
This matches the behavior of Python-based hg.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

SimonSapin created this revision.Mar 9 2021, 4:41 AM
baymax updated this revision to Diff 26201.Mar 9 2021, 8:40 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

marmoute accepted this revision.Mar 9 2021, 1:26 PM
Alphare requested changes to this revision.Mar 10 2021, 4:23 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/operations/cat.rs
20

Could you add a simple one-line docstring to the fields?

87–98

This seems wasteful because it's always looping over all files, the happy path should be as fast as possible. Should we simply populate a Vec with files that don't match anything?

This revision now requires changes to proceed.Mar 10 2021, 4:23 AM
SimonSapin updated this revision to Diff 26241.Mar 11 2021, 5:05 AM
SimonSapin marked an inline comment as done.Mar 11 2021, 5:12 AM
SimonSapin added inline comments.
rust/hg-core/src/operations/cat.rs
20

Done.

87–98

This is looping over the path CLI arguments, which are typically much fewer than files in the manifest. Populating a Vec with files that don’t match anything is exactly what this loop does.

Note that the "main" loop above is over the manifest, not over CLI arguments. This is because when multiple files match they are concatenated in manifest order rather than in the order requested on CLI. At first I thought this was an rhg bug, but Python-based hg cat also does this so 🤷

Alphare accepted this revision.Mar 11 2021, 5:17 AM
Alphare added inline comments.
rust/hg-core/src/operations/cat.rs
87–98

Right, that makes sense, thanks!

marmoute accepted this revision.Mar 12 2021, 11:54 AM
SimonSapin marked an inline comment as done.Mar 12 2021, 5:07 PM
SimonSapin edited the summary of this revision. (Show Details)
SimonSapin updated this revision to Diff 26300.
baymax updated this revision to Diff 26348.Mar 15 2021, 9:10 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.