ctubbsii commented on PR #5158: URL: https://github.com/apache/accumulo/pull/5158#issuecomment-2533008175
@ddanielr wrote: > I don't think the readability is great with these changes. Agreed, but it's nearly impossible to get readability when you want to print quotes. We can pull those out, and the readability will improve, but the printed result will (probably) not be copy-and-paste-able. I'm okay with that, if others are. > If the script was extremely static then that is less of an issue. However we've been modifying these scripts pretty frequently across the different accumulo versions. The latest iteration in the main branch (aside from a few breakages that this attempts to fix) are quite nice. It'd be easier if we could adopt the same scripts across all branches, instead of them having different versions of the script. I understand some differences are needed, but I think the actual required differences are probably pretty small, and we could use the same version of the scripts (mostly) in all branches. > Using the `&&` `||` logic is harder to parse as commends start to become nested. True, but I don't think I have any complicated nesting here. I didn't even have to use curly braces. The uses I used were very simple cases. Those can certainly be converted to if-then-else-fi blocks, though, if they are hard to read like this. I had them as if-then-else-fi blocks to begin with, but it actually looked a little more complicated. This pattern is a simple "if debug print || execute". > However I do understand the confusion about what the command actually looks like when it gets run vs being echo'd out. > > To help troubleshoot this I was using the `set -v` bash option to determine what the script was doing. `set -v` can be used for troubleshooting, sure. It can't be used to avoid execution, though... which is the purpose of the debug function, in support of the `--dry-run` flag. Similarly, you can't use `set -x` to show the expanded command to be executed. If the goal is just to debug while executing, either of those would be fine, but as originally written, it was to support a `--dry-run` flag, which means do not execute. > I created #5164 to remove the reliance on the bash version. But I tried testing those changes on `main` and it did not solely solve the issue. > > If this code is currently functional in 2.1 then I would prefer to not merge these changes and pursue #5164 if the reasoning is due to the reliance on newer bash features. No, #5164 is not sufficient. The problem isn't the reliance on the newer bash feature. The problem is the nested quoting due to indirect execution. The way it is currently implemented, it is *not* functioning correctly. Having the function do the optional printing or executing means an extra layer of quoting, and that makes it *very* difficult to reason about. Getting it to work with the extra layer of quoting for the indirect execution via the function means that the readability of the actual command being executed is poor. In my version, for this PR, only the printed message's readability is poor, but at least the correctness of the printed message is easy to verify from running with `--dry-run`. It is far less easy to verify the correctness of a poorly readable command that is actually being executed. I don't mind using the printf solution in #5164 to help print the commands without needing to make them so hard to read. However, to ensure correctness of the command actually being executed, and to ensure we have the ability to reason about those commands, I strongly thing we should avoid using a function to indirectly execute the actual commands. And that's the core part of this PR that I'd want to keep. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
