Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add doc hint that default is different than tar #366

Merged
merged 1 commit into from
Jul 8, 2024
Merged

Add doc hint that default is different than tar #366

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ impl<W: Write> Builder<W> {

/// Follow symlinks, archiving the contents of the file they point to rather
/// than adding a symlink to the archive. Defaults to true.
///
/// When true, it exhibits the same behavior as GNU `tar` command's
/// `--dereference` or `-h` options <https://man7.org/linux/man-pages/man1/tar.1.html>.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commenting on my own PR to list some alternatives. Feel free to dismiss this comment (if you prefer the current PR), or comment below if you want to explore an alternative.

Alternatives

Copy external symlinks, preserve internal symlinks

Instead of attempting to fix this issue via documentation, the project could add an additional mode that preserves symlinks within the project but copies externally symlinked files. If that were on by default, it would help preserve the existing behavior while minimizing archive size.

Consequences:

  • If this were on by default, I never would have noticed a difference between the defaults.
  • I'm unsure if there are edge cases (such as systems that don't support symlinks) where this could break existing workflows. To find out: We could add the option before defaulting it to be on.

Different doc changes

We could change the existing documentation and memorialize the existing behavior:

/// Copy the resolved contents of symlinks.
///
/// When enabled, any symlinks will be replaced by the contents of the file
/// that they point to. If the archive has relative symlinks to other files 
/// within the same archive, those files will be duplicated. Defaults to true.

Consequences:

  • This setting could result in a larger file size. The words "copy" and "duplicated" emphasize the result of this setting.
  • The problem I ran into is spelled out (versus adding the tar options link, which is only hinted at).
  • The wording around symlinks is very hard to follow in general. I wonder if the user would get the message warning; this may cause your archive size to be quite large. It balances saying enough to get the point across without overwhelming the reader.
  • This documentation could be seen as stabilizing behavior and might increase resistance to implementing "Copy external symlinks, preserve internal" in the future.

Add a doc test demonstrating current behavior

Show the same scenario with and without the option on. That would highlight what it's doing without getting bogged down in too much prose.

Consequences:

  • Would make the docs longer.
  • Code is less ambiguous than prose.

pub fn follow_symlinks(&mut self, follow: bool) {
self.follow = follow;
}
Expand Down