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

cyr_dbtool: document flags #3647

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Conversation

robn
Copy link
Contributor

@robn robn commented Sep 5, 2021

Not complete, as I don't quite understand what -t and -T are supposed to achieve (something about transactions). Also, I haven't actually tested that -c and -M even work (-n does, which is what I wanted). ALSO, -c sounds kinda destructive enough that it should perhaps be an explicit convert action and maybe shouldn't be neatly documented at all, but anyway, here we are!

@robn robn requested a review from elliefm September 5, 2021 04:06
Copy link
Contributor

@elliefm elliefm left a comment

Choose a reason for hiding this comment

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

Looks good, couple of suggestions if that's okay?

imap/cyr_dbtool.c Outdated Show resolved Hide resolved
imap/cyr_dbtool.c Show resolved Hide resolved
@robn
Copy link
Contributor Author

robn commented Sep 6, 2021

Looks good, couple of suggestions if that's okay?

Of course! :D

@robn
Copy link
Contributor Author

robn commented Sep 6, 2021

So how do you suggest I proceed here?

Sounds like:

  • we want to remove -c
  • -M might not be clear or useful enough if you have to know how your database is setup ahead of time
  • -n could conceivably be replaced by a create action
  • no one knows what -t and -T are

I wanted to avoid a large change to cyr_dbtool because I was just passing through on my way to something else, and the if/else action tree offended me and if I get started on cleanup I'll want to do something about it. But maybe you will tell me this is a good opportunity to pay down some debt, especially since I only pop up in this repo every couple of years 😅

@elliefm
Copy link
Contributor

elliefm commented Sep 6, 2021

I think we might have vague plans to consolidate ctl_cyrusdb, cvt_cyrusdb, and cyr_dbtool into a single tool with a coherent interface someday, and I'm fine with taking an "if it ain't broke" approach and leaving them mostly alone until we can do it right, once.

For now, I think it's fine to just leave -c out of the cyr_dbtool usage message, and pretend it doesn't exist, just like the man page does.

The man page says that -T is for holding a transaction while doing whatever we're doing ("most especially for 'show'"), and it seems like that used to be the default behaviour but now you need to ask for it with -T. Looks like -t is a legacy option for "don't hold a transaction", which is obsolete now that it's the default behaviour, we just continue to accept-and-ignore it so we don't break people's scripts I guess.

I have no idea why one would choose to hold a transaction or not. It might be fine to just add something like "-T use a transaction to do the action". I tend to think of usage messages as brief reminders of the functionality you already understand (or can learn about elsewhere), so I don't think we need a detailed explanation of any of this here.

It would help if the "elsewhere" were more informative, but that's a different rabbit hole!

Signed-off-by: Rob N ★ <robn@despairlabs.com>
@elliefm elliefm self-assigned this Sep 16, 2024
@elliefm elliefm merged commit fb1d371 into cyrusimap:master Sep 16, 2024
1 check passed
This pull request was closed.
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