#348 mount/umount: Make MOUNTPOINT a positional aurgument instead of using --mount-point

Open
opened 2 months ago by hanschen · 5 comments

What I’m trying to do and why

I just started using borgmatic (it’s fantastic!) so I may be missing something here, but isn’t a mount point always required in borgmatic mount/umount? If that’s the case, I would suggest dropping the --mount-point flag to simplify the syntax and make it more similar to borg mount/umount (and also the traditional mount/umount commands).

For example, instead of

borgmatic mount --archive some-archive --mount-point /mnt/backups

it would become

borgmatic mount --archive some-archive /mnt/backups

Other notes / implementation ideas

I would also suggest dropping the --archive flag, e.g.

borgmatic mount some-archive /mnt/backups

To mount all archives or the latest archive, there could be e.g. a --all and a --latest flag. Mounting all archives if --archive is omitted doesn’t sound like a very good default behavior to me.

If this sounds good, I could post it under a separate issue. Just let me know.

Environment

borgmatic version: 1.5.9

#### What I'm trying to do and why I just started using borgmatic (it's fantastic!) so I may be missing something here, but isn't a mount point always required in `borgmatic mount/umount`? If that's the case, I would suggest dropping the `--mount-point` flag to simplify the syntax and make it more similar to `borg mount/umount` (and also the traditional `mount/umount` commands). For example, instead of `borgmatic mount --archive some-archive --mount-point /mnt/backups` it would become `borgmatic mount --archive some-archive /mnt/backups` #### Other notes / implementation ideas I would also suggest dropping the `--archive` flag, e.g. `borgmatic mount some-archive /mnt/backups` To mount all archives or the latest archive, there could be e.g. a `--all` and a `--latest` flag. Mounting all archives if `--archive` is omitted doesn't sound like a very good default behavior to me. If this sounds good, I could post it under a separate issue. Just let me know. #### Environment **borgmatic version:** 1.5.9
witten commented 2 months ago
Owner

The current set of borgmatic mount flags is actually a deliberate design decision. When I’m using commands like borg directly where required arguments are just positional, I often have a hard time remembering the order of the arguments.

But with commands like borgmatic mount as currently implemented, I don’t have to remember anything other than the names of flags, and I can specify them in any order I like. And required arguments don’t have to come before optional arguments; they can be in the order that makes sense as I’m typing them.

I realize this flies a little in the face the usual Unix convention. I also realize that this often requires more typing! But I feel like the trade-offs are worth it with a program like borgmatic. You’ll notice similiar convention in all the other borgmatic sub-commands as well.

I will say that if you know of a way in Python (argparse) to make a sub-command accept a flag either as positional or as a --long-flag, I’d be willing to make that a supported thing.

The current set of `borgmatic mount` flags is actually a deliberate design decision. When I'm using commands like `borg` directly where required arguments are just positional, I often have a hard time remembering the order of the arguments. But with commands like `borgmatic mount` as currently implemented, I don't have to remember anything other than the names of flags, and I can specify them in any order I like. And required arguments don't have to come before optional arguments; they can be in the order that makes sense as I'm typing them. I realize this flies a little in the face the usual Unix convention. I also realize that this often requires more typing! But I feel like the trade-offs are worth it with a program like borgmatic. You'll notice similiar convention in all the other borgmatic sub-commands as well. I will say that if you know of a way in Python (`argparse`) to make a sub-command accept a flag *either* as positional *or* as a `--long-flag`, I'd be willing to make that a supported thing.
hanschen commented 2 months ago
Poster

Thanks for the explanation, that makes sense. I usually also prefer keyword arguments when in comes to e.g. Python functions. In this case, however, I guess I was expecting the borgmatic mount/umount commands to behave similarly to their borg/Unix mount/umount counterparts.

Here’s a quick proof-of-concept I cooked up:

#!/usr/bin/env python
"""Example of borgmatic mount with optional --archive and --mount-point flags.
"""
import argparse

parser = argparse.ArgumentParser(description='Borgmatic example')
parser.add_argument("archive", nargs='?', type=str,
                    help='path to archive')
parser.add_argument("mountpoint", metavar='mount-point', nargs='?', type=str,
                    help='path to mount point')
parser.add_argument("--archive", dest='archive_flag', metavar="PATH", type=str,
                    help='path to archive')
parser.add_argument("--mount-point", dest='mountpoint_flag', metavar="PATH",
                    type=str,
                    help='path to mountpoint')

args = parser.parse_args()

if args.archive is not None and args.archive_flag is not None:
    parser.error("multiple values for 'archive' given.")

if args.mountpoint is not None and args.mountpoint_flag is not None:
    parser.error("multiple values for 'mountpoint' given.")

archive = args.archive or args.archive_flag
mountpoint = args.mountpoint or args.mountpoint_flag

if archive is None or mountpoint is None:
    print(parser.format_help())

print(f"Archive: {archive}")
print(f"Mount point: {mountpoint}")

Some examples:

$ ./borgmatic-mount.py archivename /mnt/backup                                                               
Archive: archivename
Mount point: /mnt/backup

$ ./borgmatic-mount.py --archive archivename --mount-point /mnt/backup                                       
Archive: archivename
Mount point: /mnt/backup

$ ./borgmatic-mount.py --mount-point /mnt/backup --archive archivename                                       
Archive: archivename
Mount point: /mnt/backup

$ ./borgmatic-mount.py archivename --mount-point /mnt/backup                                                 
Archive: archivename
Mount point: /mnt/backup

$ ./borgmatic-mount.py --archive archivename /mnt/backup                                                     
usage: borgmatic-mount.py [-h] [--archive PATH] [--mount-point PATH] [archive] [mount-point]
borgmatic-mount.py: error: multiple values for 'archive' given.

Not sure what would make most sense in the last case, should the positional argument be interpreted as mount-point since archive has been given, or should it interpret it as archive (current situation) since it’s the first positional argument?

If you think this feature is worth implementing I could try to put together a pull request.

Thanks for the explanation, that makes sense. I usually also prefer keyword arguments when in comes to e.g. Python functions. In this case, however, I guess I was expecting the `borgmatic mount/umount` commands to behave similarly to their `borg`/Unix `mount/umount` counterparts. Here's a quick proof-of-concept I cooked up: ``` #!/usr/bin/env python """Example of borgmatic mount with optional --archive and --mount-point flags. """ import argparse parser = argparse.ArgumentParser(description='Borgmatic example') parser.add_argument("archive", nargs='?', type=str, help='path to archive') parser.add_argument("mountpoint", metavar='mount-point', nargs='?', type=str, help='path to mount point') parser.add_argument("--archive", dest='archive_flag', metavar="PATH", type=str, help='path to archive') parser.add_argument("--mount-point", dest='mountpoint_flag', metavar="PATH", type=str, help='path to mountpoint') args = parser.parse_args() if args.archive is not None and args.archive_flag is not None: parser.error("multiple values for 'archive' given.") if args.mountpoint is not None and args.mountpoint_flag is not None: parser.error("multiple values for 'mountpoint' given.") archive = args.archive or args.archive_flag mountpoint = args.mountpoint or args.mountpoint_flag if archive is None or mountpoint is None: print(parser.format_help()) print(f"Archive: {archive}") print(f"Mount point: {mountpoint}") ``` Some examples: ``` $ ./borgmatic-mount.py archivename /mnt/backup Archive: archivename Mount point: /mnt/backup $ ./borgmatic-mount.py --archive archivename --mount-point /mnt/backup Archive: archivename Mount point: /mnt/backup $ ./borgmatic-mount.py --mount-point /mnt/backup --archive archivename Archive: archivename Mount point: /mnt/backup $ ./borgmatic-mount.py archivename --mount-point /mnt/backup Archive: archivename Mount point: /mnt/backup $ ./borgmatic-mount.py --archive archivename /mnt/backup usage: borgmatic-mount.py [-h] [--archive PATH] [--mount-point PATH] [archive] [mount-point] borgmatic-mount.py: error: multiple values for 'archive' given. ``` Not sure what would make most sense in the last case, should the positional argument be interpreted as `mount-point` since `archive` has been given, or should it interpret it as `archive` (current situation) since it's the first positional argument? If you think this feature is worth implementing I could try to put together a pull request.
witten commented 2 months ago
Owner

Not sure what would make most sense in the last case, should the positional argument be interpreted as mount-point since archive has been given, or should it interpret it as archive (current situation) since it’s the first positional argument?

Hmm. Good question. I think it’s probably okay to leave the current behavior (the multiple values error). When you mix positional and non-positional arguments like that, the right way to intrepret that is kind of undefined. The user can always switch to either 100% positional or 100% flags to disambiguate.

If you think this feature is worth implementing I could try to put together a pull request.

I think it’s a question of whether you think it’s worth implementing. :) I’d certainly entertain a pull request though if you’re up to it. Based on your example, it does look like argparse could be made to support both positional and flag arguments.

One suggestion I have though is to make a canonical destination for each logical option (e.g. args.archive) and copy values from other sources (like args.archive_flag) into it. That way, any downstream code that needs to access the archive argument doesn’t need to be conditional, and can just consume from a single place (e.g. args.archive).

> Not sure what would make most sense in the last case, should the positional argument be interpreted as mount-point since archive has been given, or should it interpret it as archive (current situation) since it’s the first positional argument? Hmm. Good question. I think it's probably okay to leave the current behavior (the multiple values error). When you mix positional and non-positional arguments like that, the right way to intrepret that is kind of undefined. The user can always switch to either 100% positional or 100% flags to disambiguate. > If you think this feature is worth implementing I could try to put together a pull request. I think it's a question of whether *you* think it's worth implementing. :) I'd certainly entertain a pull request though if you're up to it. Based on your example, it does look like argparse could be made to support both positional and flag arguments. One suggestion I have though is to make a canonical destination for each logical option (e.g. `args.archive`) and copy values from other sources (like `args.archive_flag`) into it. That way, any downstream code that needs to access the archive argument doesn't need to be conditional, and can just consume from a single place (e.g. `args.archive`).
hanschen commented 2 months ago
Poster

I’ll take a look at the code and give it a try. :)

I'll take a look at the code and give it a try. :)
witten commented 2 months ago
Owner

Awesome. :)

Awesome. :)
witten added the
waiting for response
label 2 months ago
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.