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.

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

Improved log messages that contain a directory path #36

Closed
wants to merge 1 commit into from
Closed

Improved log messages that contain a directory path #36

wants to merge 1 commit into from

Conversation

xanderio
Copy link

When logging a message about archive creation it was formatted with a
static '/' in front for the directory name. This is fine relative
directory paths, but when using a absolute path this will lead to a
double '/' being printed.

This change only add a '/' in front of the path if there is non.

Without this change:

   input            output
"/usr/local" ->  "//usr/local"
"usr/local"  ->  "/usr/local"

With this change:

   input            output
"/usr/local" ->  "/usr/local"
"usr/local"  ->  "/usr/local"

When logging a message about archive creation it was formatted with a
static '/' in front for the directory name. This is fine relative
directory paths, but when using a absolute path this will lead to a
double '/' being printed.

This change only add a '/' in front of the path if there is non.

Without this change:
   input            output
"/usr/local" ->  "//usr/local"
"usr/local"  ->  "/usr/local"

With this change:
   input            output
"/usr/local" ->  "/usr/local"
"usr/local"  ->  "/usr/local"
@xanderio
Copy link
Author

I'm not sure how to make the CI happy as the tool shfmt give me some other output when run locally and i'm unable to find the configuration for the CI pipeline.

@assistcontrol
Copy link
Collaborator

Thanks for this PR, @xanderio!

I'm not clear on why you'd need absolute paths. Everything is always relative to /. Can you describe your use case?

@xanderio
Copy link
Author

xanderio commented Jun 23, 2019 via email

@assistcontrol
Copy link
Collaborator

assistcontrol commented Jun 23, 2019

That's an interesting use case; I hadn't considered that approach before.

This sounds like a pretty unique usage though, and I'm not sure this message will have a big impact on other users.

To me, this sounds like something that upstream tarsnap might address. Why should -c /usr/*/etc behave differently from -C / -c usr/*/etc?

(Edit: typo)

@xanderio
Copy link
Author

Because the * is not expanded by tarsnap. * is a shell feature.

In this case for *-expansion to work the shell need to know what to expand it to.
So when using a relative path it will check based on the current working directory. Which is probably the users $HOME, so the shell can't expand the pattern which is indent to be bases on / and will leave it as it is.

When using a absolute path the shell can expand the pattern and replace the path with a * in it with a list of all the possible options.

In case of the program path in the backuptargets variable will expanded when the shell first evaluates the variable.

For example amusing this file layout

/
   home
       user1
       user2
       user3    
backuptargets="/home/*"

will become

backuptargets="/home/user1 /home/user2 /home/user3"

@assistcontrol
Copy link
Collaborator

Ah, I hadn't thought about it being a simple shell expansion.

So, you're of course completely correct that, while technically valid, the output is clearly clunky-looking and suboptimal. I'm working a major refactoring that would clobber this patch anyway, so I won't merge the PR right now. I'll do my best to fix this output during the refactor, and I'll ping you here once the branch for it is pushed.

In the meantime, some alternatives might also accomplish what you're looking for:

  • acts.conf is just a sourced shell script:

      backuptargets="home var/backups $(cd / && find usr/jail -type d -name etc)"
    
  • Tell tarsnap itself to run from / in acts.conf:

      tarsnap="cd / && tarsnap"
    

assistcontrol added a commit that referenced this pull request Aug 17, 2019
We have a couple different ways we need to format strings. Putting
everything into one function makes it easy to format strings in
ways that look right to our users. This also simplifies the printing
of error output, which required shell-check escapes and multiple
levels of escaped quotes.

It's a bit obtuse to dump all this stuff into one function, but it
gives us the most flexibility.
@assistcontrol
Copy link
Collaborator

I added the formatter into a single function, so this should be working as you intended now. Please re-open if it doesn't do the right thing. Thanks for submitting this and for your patience, @xanderio.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants