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

logs: put borg arguments within quotation marks when they contain special strings #2190

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

goebbe
Copy link
Contributor

@goebbe goebbe commented Jan 13, 2025

Description

This pull request has two purposes:

  1. logs: print the list of borg command arguments, that are used by Vorta (and executed via Popen())
  2. logs: print a shell command approximation for the borg command that respects the borg recommendations.

The pull request only concerns what is printed to the logs.

Related Issue

#2189

Current situation in Vorta (prior to 0.10.3):
When executing borg commands, an "approximation" of the borg (shell) command is printed to the logs.
This command is an approximation in the sense that it is artificially constructed from a list of command line arguments cmd, that is used internally by Vorta. In the past this lead to issues #2164, since the approximation of the borg command in the logs was correct, but internally the list of arguments cmd was broken. :-/

Background: The list of command line arguments cmd does neither require nor contain quotation marks - or escape characters (e.g. for special characters), and Vorta passes cmd "as is" to Popen() for execution.

For shell usage, borg recommends the use of quotation marks in the following cases:

  • arguments with wildcards that are used for pattern matching, as "*" and "?".
  • regular expressions.
  • item names (paths, folder or file names) that contain whitespace " ", since escaping whitespace with "\" (as currently used in Vorta) is shell specific.

This pull request does two things:

  1. Print the unmodified list of borg arguments cmd to the logs
  2. Changes to the shell command approximation:
    • change the description, and make clear that this is a shell command approximation
    • add quotation marks, where recommended (don't use escape characters)
    • the aim is to get a shell command that could be copy/pasted to any shell and be directly executed there.

Motivation and Context

  • Currently there can be differences between the borg command in the logs and the actual list of borg arguments cmd, this makes debugging cumbersome. See Extra borg arguments with white space raise an error #2164
  • The borg command (approximation) in the logs currently does not fit to the borg recommendations, i.e. use quotation marks for white space or regular expressions (this should be fixed anyway).

How Has This Been Tested?

The following borg arguments have been added to the "extra borg arguments" via the Vorta UI - to check if the resulting shell command approximation provides the intended format / quotation marks:

--remote-path=/usr/local/bin/borg             #no changes required
--pattern=-home/goebbe/test\ folder           #whitespace escaped, usind "\"
--pattern='+home/user/test folder/testfile'   #white space in folder name
--pattern='-re:home/user/test\sfolder2'       #regular expression
--pattern='-re:crazy?=cra'                    #regualr expression that contains a "="
-e 're:crazy?=cra'                            #regular expression but without pattern=
-e '/home/user/*.tmp'                         #wildcard should be quoted
-e /home/user/test/cache                      #no changes required
--patterns-from /home/user/patterns.lst       #no changes required

These are the resulting entries in the logs:

The list of borg command line arguments cmd. Note that quotation marks have been stripped away:
2025-01-13 12:35:57,302 - vorta.borg.borg_job - INFO - borg command arguments: ['/usr/bin/borg', 'create', '--remote-path=/usr/local/bin/borg', '--list', '--progress', '--info', '--log-json', '--json', '--filter=AM', '-C', 'lz4', '--pattern=+home/user/test folder/testfile', '--pattern=-home/user/test folder', '--pattern=-home/goebbe/test folder', '--pattern=-re:home/user/test\\sfolder2', '--pattern=-re:crazy?=cra', '-e', 're:crazy?=cra', '-e', '/home/user/*.tmp', '-e', '/home/user/test/cache', '--patterns-from', '/home/user/patterns.lst', 'ssh://user@192.168.0.5:22/~/backup/vorta-repo::user-2025-01-13-123557', '/home/user']

The shell command approximation:
2025-01-13 12:35:57,302 - vorta.borg.borg_job - INFO - shell command approximation: /usr/bin/borg create --remote-path=/usr/local/bin/borg --list --progress --info --log-json --json --filter=AM -C lz4 --pattern='+home/user/test folder/testfile' --pattern='-home/user/test folder' --pattern='-home/user/test folder' --pattern='-re:home/user/test\sfolder2' --pattern='-re:crazy?=cra' -e 're:crazy?=cra' -e '/home/user/*.tmp' -e /home/user/test/cache --patterns-from /home/user/patterns.lst ssh://user@192.168.0.5:22/~/backup/vorta-repo::user-2025-01-13-123557 /home/user

The shell command approximation can be copied to a shell and executed "as is".

Discussion:

Printing both, the list of command line arguments AND the shell command approximation, leads to obvious redundancies, but each of the options has its specific advantages:

  • List of borg command line arguments:

    • advantage: If something goes wrong with the execution, this is what is required for debugging.
    • disadvantage: Hard to read, cannot easily be copied/ pasted / modified / executed in a shell
  • shell command approximation:

    • advantage: Can directly be copied/ pasted to any shell, easy to read
    • disadvantage: Should not be used for debugging, not the command that is actually executed by Vorta

Personally, I would put both to the logs, since both are helpful in some cases.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING guide.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have added tests to cover my changes.

I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.

@goebbe
Copy link
Contributor Author

goebbe commented Jan 14, 2025

@m3nu This is ready for review.

@m3nu
Copy link
Contributor

m3nu commented Jan 14, 2025

Thanks for the PR, as always. So this aims to log a command for copy & pasting and reversing some of the escaping done when logging this? I wonder if there is a simpler way to achieve something similar? E.g. here some different ways to log stuff.

# This WILL escape the newline because %r uses repr(msg).
logging.debug("Repr style: %r", msg)

# This will NOT escape the newline; you'll actually see the newline rendered.
logging.debug("String style: %s", msg)

The issue could also come from our join() function.

For this PR I worry that:

  • It's pretty complex and runs for each Borg command, even though it's rarely needed.
  • If there is any issue in this function, the Borg command will fail.
  • No tests to ensure the examples work.
  • There may be more cases that need be added now or in the future. I think building a function that reverses some other escaping function is pretty complex.
  • I wouldn't be able to debug it in the future and would likely just remove it, if there is any issue.

@goebbe
Copy link
Contributor Author

goebbe commented Jan 14, 2025

@m3nu: Thanks for the feedback.

Printing the list of borg command arguments is a simple one-liner:
logger.info('borg command arguments: %s', self.cmd)
This prints the exact list of borg arguments as passed to Popen() - this is everything that is required in the logs for debugging.

So a simple (and correct) solution would be to just put this single line instead of (re-)constructing a valid shell command.
Disadvantage: This list of "borg command arguments" cannot be copied/ pasted to a shell (not sure if this is even a goal)

The list of borg command arguments does not contain any escape characters or quotation marks. These are not required if no shell is involved.
So whatever we do when building a valid shell command for the logs - we have to manually take care about how we treat whitespace and special characters:

  • Thomas Waldman pointed out that whitespace should not be escaped by "\" since this is not supported by all shells.
  • Moreover, there may be wildcards and regular expression which also should be put into quotation marks - for shell commands (this is written in the borg docs).

Computational overhead: I wondered about this, too. :-) To get a rough estimate, I run the code to format the pretty long and complicated list of borg arguments from my commit message, on my mid-range laptop inside a Jupyter notebook.
It took 0.13 milliseconds - not sure if this is acceptable, or not.

Complexity I agree that the code in the pull request is not self-explanatory, since I tried to put the first quotation mark after the first "=" if there are any.
Thinking about it - one way to reduce the complexity is to just always put the whole argument inside quotation marks, whenever it contains one of the special strings - which would result in a pretty self-explanatory four-liner, since I would get rid of the "if elseif else craziness":

special_strings = [' ','*', '?', 're:']
for i, arg in enumerate(cmd):
     if any(specialstr in arg for specialstr in special_strings):
        cmd[i]=arg.replace(arg,"\'"+arg+"\'")    #add quotation marks 

The only difference for the shell command approximation would be that with the simplified code I would get
'--pattern=-home/user/test folder'
instead of:
--pattern='-home/user/test folder'
Which both run just fine in the shell, despite the whitespace. :-)

Currently, in 0.10.3, we write a borg command (approximation) to the logs that escape whitespace using "\" and without any quotation marks (not even for regular expressions "re:") - I would like to fix this.

I believe there are several valid options, here:

  1. Just print the plain list of borg arguments
  2. Simplify the pull request and only put the shell command (approximation)
  3. Put both
  4. ?

Any preferences?

@goebbe
Copy link
Contributor Author

goebbe commented Jan 15, 2025

Thinking about this again, I believe option 2 would be a good one:

  • The only change would be that in the logs, the borg arguments are put into quotation marks if they contain
    special strings (i.e. whitespace, wildcards or regular expressions)
  • as a result the arguments are preserved (not changed), which is good for debugging
  • the resulting borg command, in the logs, could be used as a shell command

@goebbe
Copy link
Contributor Author

goebbe commented Jan 15, 2025

@m3nu I simplified the code and updated my pull request:

Changes:

  • Simplified logic: just put the whole argument within quotation marks if it contains whitespace, wildcards or regular expressions
    • good for debugging, since argument splitting and arguments themselves are not touched
    • no complicated if/else if/else construct :-)
    • code is pretty much self-explanatory (IMO) - it would be easy to e.g. add a new "quotation_string".
  • the resulting borg command from the logs can be copied/ pasted/executed in a shell as is
  • computational time is down by half, to 0,0731 milliseconds for complicated cases

This solution is more robust than what is in 0.10.3:

  • the use of escape characters "\" for whitespace in 0.10.3 is shell dependent.
  • currently, the borg command in the logs is printed without any quotation marks.

Example log output with the modified pull request - for a complicated case:
/usr/bin/borg create --remote-path=/usr/local/bin/borg --list --progress --info --log-json --json --filter=AM -C lz4 '--pattern=+home/user/test folder/testfile' '--pattern=-home/user/test folder' '--pattern=-home/user/test folder' '--pattern=-re:home/user/test\sfolder2' '--pattern=-re:crazy?=cra' -e 're:crazy?=cra' -e '/home/user/*.tmp' -e /home/user/test/cache --patterns-from /home/user/patterns.lst ssh://user@192.168.0.5:22/~/backup/vorta-repo::user-845-2025-01-15-083513 /home/user

This command can be copied pasted from the logs to the shell - and runs just fine (only archive name has to be updated, obviously)

As before: The execution path is not concerned here - this is purely about a pretty and useful (for debugging and copy) borg command in the logs.

@m3nu
Copy link
Contributor

m3nu commented Jan 16, 2025

The new version is much clearer! No objections to add this.

@goebbe goebbe changed the title logs: Add borg command arguments and a shell command approximation logs: put borg arguments within quotation marks when they contain special strings Jan 17, 2025
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.

2 participants