fix: Change init to default to --config if --config-path is absent

[paltherr]

Here is an updated version of my previous PR. I have checked that it does what's expected and that it doesn't break any existing tests.

It should still be considered as a proof of concept rather than something ready to be submitted. My claim is that it contains the required logic to achieve the desired goal without breaking any existing behavior but that's about it. I haven't tried to add tests, yet. More importantly some changes, like in particular the replacement of configFileAbsPath with configFileAbsPathDefault and configFileAbsPathExplicit, look ugly. Note also that I have no experience programming in Go.

The main benefit of this change is that it becomes possible to use chezmoi with an alternate config simply by defining an alias like the following one:

alias chezmoi2="chezmoi --config=<path-to-alternate-config>"

This works for all commands, including the initial init command.

Fixes #3127.

[paltherr]

Arguably an error check similar to the newly introduced one could also be done in case there is no --config option. This could help in cases where someone switches from one format of .chezmoi.<format>.tmpl to another one. Currently, if you do that, chezmoi status signals that the config changed and that init should be rerun. Rerunning chezmoi init, signals NO error and generates a new config file with the new format but without deleting the old one. Thus, the next time you run chezmoi, it fails with an error stating that there are multiple config files. With the new error check, chezmoi init could signal that there is a conflict.

Btw, another benefit of this PR would be that the init option --config-path could be deprecated (I think). I remember that when I started using chezmoi I found it very confusing that there are two options, --config and --config-path, seemingly for the same thing. It took me some time to figure out when and how to use each one. I think that it would be a great plus if future users would only have to learn about the former.

[paltherr]

The PR still needs work (see points below) but I need your input. First of all to know whether you think this can be brought into a state where the added value is worth the extra complexity/ugliness. And, if yes, what your recommendations are to get there. I'm not very familiar with github and pull requests but I guess that I have to mark the PR as "Ready for review" to get your attention. I will do that after this comment.

Here are points that still need work/answers:

• The new error check shouldn't compare file base names but only file extensions.
• Should the new error check also be done when no --config option was given (see previous comment)? If yes,
  • In case of a conflict, should an error be reported? An alternative would be to delete the conflicting config file, i.e., changing the format of .chemoi.<format>.tmpl would just work.
  • Should this be done in a separate PR?
• Better names are needed for the Config fields.
  • The fields configFileAbsPathDefault and configFileAbsPathErr could be renamed to defaultConfigFileAbsPath and defaultConfigFileAbsPath to match the function name defaultConfigFile that computes them.
  • The field configFileAbsPathExplicit could be renamed to one of optionConfigFileAbsPath, flagConfigFileAbsPath, or explicitConfigFileAbsPath.
• Should the new function configFileAbsPath() be renamed and/or moved?
• A number of scenarios/journeys should be tested. Should I try to squeeze everything into an issue3127.txtar or could the tests be split over multiple files?

[twpayne]

Firstly, thank you for your work and this excellent PR! To answer some of your questions:

First of all to know whether you think this can be brought into a state where the added value is worth the extra complexity/ugliness.

Yes, absolutely! Users care about the interface, not the implementation, and this is a key usability improvement for people using multiple chezmoi configs. It's definitely worth the extra complexity, the ugliness is not visible, and as long as we have tests to check the user-facing behavior for common cases we can freely change the implementation to be more beautiful in the future.

Should the new error check also be done when no --config option was given (see previous comment)?

Yes, but this falls foul of the complexities I described. Specifically, how do distinguish between "no --config option was given" and "a --config option was given but was set to the default value". Elsewhere in the codebase I've used a sentinel value to handle this, but I don't know if a similar approach could be used here.

In case of a conflict, should an error be reported?

Yes. chezmoi's users are highly technical so by default report meaningful, actionable errors when anything is inconsistent.

Better names are needed for the Config fields.

Yup, the names you propose are better.

Should the new function configFileAbsPath() be renamed and/or moved?

Yes, but this is an internal detail. I would suggest getConfigFileAbsPath to make the distinction between a value (e.g. configFileAbsPath) and a potentially-expensive function that returns a value (getConfigFileAbsPath()).

chezmoi does try to keep functions in some kind of mix of hierarchical and alphabetical order, so no need to move the function right now. At some point, I'll write a linter to check function order, but, for this PR, meaningful diffs are more important than function order so there is no need to move functions.

• A number of scenarios/journeys should be tested. Should I try to squeeze everything into an issue3127.txtar or could the tests be split over multiple files?

Please split the tests over multiple files. This both separates intent and allows the tests to run in less wallclock time (go test can run them in parallel). Specifically:

• For changes to behavior that should be obvious defaults - like this, where removing the --config-path hack is a clear improvement, please update internal/cmd/testdata/scripts/init.txtar with the new behavior.
• For exhaustive testing of the changes, please create a new initconfig.txtar script with as many tests as you can think of, especially edge cases.
• For specific issues, like the original report in The init command should default to the global --config when no --config-path is provided #3127, please do create an issue3127.txtar that tests the reported case directly. These are extremely helpful as high-level behavioral checks.
• Finally, if there are edge cases that you can think of that the current code doesn't pass, please include them anyway, but commented out or prefixed with [skip] so they don't run. Such cases are extremely helpful to future developers, and it's OK that the code doesn't pass them now.

Thanks again for this excellent PR!

[paltherr]

Yes, absolutely! Users care about the interface, not the implementation, and this is a key usability improvement for people using multiple chezmoi configs. It's definitely worth the extra complexity, the ugliness is not visible, and as long as we have tests to check the user-facing behavior for common cases we can freely change the implementation to be more beautiful in the future.

Great to hear that :-) I'll first clean up the code and then write the tests. The former should be relatively quick, the latter probably less so as I'll first have to understand how txtar files work. I found testscript <https://pkg.go.dev/github.com/rogpeppe/go-internal/testscript>, which describes them. Is there anything else I should know about tests? *Before I proceed, I have one question though: how am I supposed to contribute the updates?* This is my first PR on github and it's not entirely clear to me how I should go forward. Here is my current setup: - I cloned your repo on github. - I cloned my clone on my local machine. - I created a commit on my local machine and pushed it to my clone on github. - On github I used the commit to create the PR. My current understanding of how I should go forward is the following: - Create one or more new commits on my local machine. - Push the commits to my clone on github. They will auto-magically be added to the PR. - Rinse and repeat until we agree on a final state. - You integrate the PR into your repo. If this is indeed the correct way, how should I name and describe my commits? Should I use "chore:", "fix:", or some other prefix? How important are the descriptions? Are they just for you to understand what I'm changing/adding to the PR or will they survive the PR and should therefore also make sense for future readers of your repo? As you can see, one thing that isn't clear to me is whether the extra commits will survive the PR or whether they will all be merged into a single commit on your repo. Yes, but this falls foul of the complexities I described

<#3127 (comment)>. Specifically, how do distinguish between "no --config option was given" and "a --config option was given but was set to the default value".

That should now be possible. That's why I split the old configFileAbsPath field into two fields, configFileAbsPathDefault and configFileAbsPathExplicit, and one function configFileAbsPath(). The former field only ever contains the default value computed by defaultConfigFile(), the latter field only ever contains the value of the --config option, or Empty, if none was given. The function combines the two fields and returns the value that was previously stored in the configFileAbsPath field. This assumes that the --config option is the only way to explicitly specify the config file (to load). My understanding is that the XDG specification only allows to specify where (in which directories) to look for a config file but not the config file itself. Did I overlook something?

…

On Thu, Jan 18, 2024 at 1:19 AM Tom Payne ***@***.***> wrote: Firstly, thank you for your work and this excellent PR! To answer some of your questions: First of all to know whether you think this can be brought into a state where the added value is worth the extra complexity/ugliness. Yes, absolutely! Users care about the interface, not the implementation, and this is a key usability improvement for people using multiple chezmoi configs. It's definitely worth the extra complexity, the ugliness is not visible, and as long as we have tests to check the user-facing behavior for common cases we can freely change the implementation to be more beautiful in the future. Should the new error check also be done when no --config option was given (see previous comment)? Yes, but this falls foul of the complexities I described <#3127 (comment)>. Specifically, how do distinguish between "no --config option was given" and "a --config option was given but was set to the default value". Elsewhere in the codebase I've used a sentinel value to handle this <https://github.com/twpayne/chezmoi/blob/80717f4f1c35862d512e8b5415058cdc5cdcdc97/internal/cmd/config.go#L61>, but I don't know if a similar approach could be used here. In case of a conflict, should an error be reported? Yes. chezmoi's users are highly technical so by default report meaningful, actionable errors when anything is inconsistent. Better names are needed for the Config fields. Yup, the names you propose are better. Should the new function configFileAbsPath() be renamed and/or moved? Yes, but this is an internal detail. I would suggest getConfigFileAbsPath to make the distinction between a value (e.g. configFileAbsPath) and a potentially-expensive function that returns a value ( getConfigFileAbsPath()). chezmoi does try to keep functions in some kind of mix of hierarchical and alphabetical order, so no need to move the function right now. At some point, I'll write a linter to check function order, but, for this PR, meaningful diffs are more important than function order so there is no need to move functions. - A number of scenarios/journeys should be tested. Should I try to squeeze everything into an issue3127.txtar or could the tests be split over multiple files? Please split the tests over multiple files. This both separates intent and allows the tests to run in less wallclock time (go test can run them in parallel). Specifically: - For changes to behavior that should be obvious defaults - like this, where removing the --config-path hack is a clear improvement, please update internal/cmd/testdata/scripts/init.txtar with the new behavior. - For exhaustive testing of the changes, please create a new initconfig.txtar script with as many tests as you can think of to this script, especially edge cases. - For specific issues, like the original report in #3127 <#3127>, please do create an issue3127.txtar that tests the reported case directly. These are extremely helpful as high-level behavioral checks. - Finally, if there are edge cases that you can think of that the current code doesn't pass, please include them anyway, but commented out or prefixed with [skip] so they don't run. Such cases are extremely helpful to future developers, and it's OK that the code doesn't pass them now. Thanks again for this excellent PR! — Reply to this email directly, view it on GitHub <#3489 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AXTPHFFG3JQABUBNMBUJCE3YPBTAJAVCNFSM6AAAAABB5REJ7CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJXGU2DEMBZGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[twpayne]

Firstly, there are answers to most of your questions in the contributing docs. You also see this link when you open a PR.

Is there anything else I should know about tests?

Please follow the style of the existing tests. Note that you can't easily use a debugger to step through testscript tests, so you might have to do some printf-style debugging. Useful commands for this include:

exec find $WORK # find everything in the filesystem

exec chezmoi ...
! stdout . # print whatever the previous command wrote stdout and stop
! stderr . # print whatever the previous command wrote stderr and stop

This assumes that the --config option is the only way to explicitly specify the config file (to load). My understanding is that the XDG specification only allows to specify where (in which directories) to look for a config file but not the config file itself. Did I overlook something?

I think you are correct. It will be great to verify edge cases with tests.

[paltherr]

tl;dr: Rather than extend the new error check to more cases I will drop it. The case that it signals is a corner case that isn't much different from similar already existing corner cases that aren't signaled either. I still think that it would make sense to signal these cases but doing so turns out to be complex enough to warrant its own issue and pull request.

Long Version

The new error check that I added in my first version of the code tries to ensure that the updated config is written into a file of the right format. More precisely, it tries to enforce that if the config template is of format FORMAT, then the config is written into a file with a .FORMAT extension. The check tests the whole base name of the target file and thus enforces that it's equal to chezmoi.FORMAT. That's too strict because the --config option allows to use config files with arbitrary names. When I looked into restricting the check to the extension of the file, I figured that even the extension could be arbitrarily chosen, since the format of the config file can be independently specified with the --config-format option. Thus, just writing a check that is neither too strict nor too lax is already non-trivial.

The check tries to prevent chezmoi from corrupting its own config. The init command unfortunately has that potential. Running it can prevent further chezmoi commands of working, even with a perfectly valid config template. One scenario where this can happen is if the user replaces their config template with a new one, possibly an equivalent one, but in a different format. After such a change, chezmoi prompts the user to rerun the init command. Assuming that the config file is in the default location, running chezmoi init creates a new config file in the new format in the default location. From then on, all chezmoi commands, including the init command, exit with an error stating that there are multiple config files. The fix is easy, delete the config file in the old format, but it may not be so obvious for users who didn't realize that the change of format of the config template wasn't completely innocuous. Therefore detecting and signaling this case may be worthwhile.

For users that rely on a config file in a custom location, things are a bit different. They have to run all their chezmoi commands with the --config PATH option and whenever they run the init command they have to run it with the --config-path PATH option. If they do that after replacing their config template with a new one in a different format, then all following chezmoi commands fail because they can't parse the updated config file. Here again the fix is easy, rename the updated file. Alternatively the init command could have been run with a --config-path option with the new file name. In any case, the user has to update their commands/aliases/scripts to use a --config option with the new file name. All this may well be obvious to users advanced enough to use the --config option but a little warning would certainly not hurt either.

With the change proposed in this PR and assuming that the init option --config-path is eventually deprecated, users using a custom config location would be compelled to run chezmoi -config PATH init and then rename the updated config file. This illustrates that while detecting and reporting the case might be desirable, turning it into an inescapable error is probably not advisable, as it may make it much more difficult to update the config file in that scenario.

Fortunately the case looks very much like a corner case. The scenario described above leading to it, while possible, is certainly not one happening very often. I'm not aware of other scenarios leading to the same case. I can't exclude that others exist but they too most likely occur only rarely. Thus, there is no urgency in detecting and signaling this case. Furthermore, the problem exists already and the change in the PR doesn't make it significantly worse, the logic to detect the case isn't completely trivial, and finally even how to signal it isn't necessarily completely obvious. For all these reasons, it looks more appropriate to leave the detection and signaling of this case for a separate PR with its own issue.

[twpayne]

This PR is looking great. Please can you add some tests. The right place to add them is probably internal/cmd/testdata/scripts/init.txtar, although you can also create a new separate testscript for this functionality.

Thanks for the detailed explanations. It's certainly a very messy area with multiple interacting, possibly conflicting, sources of truth (e.g. multiple XDG config directories, --config flag, --config--format flag, chezmoi's internal defaults).

On the up side, people changing config file formats is a relatively rare thing (although it does happen, see #1430), so I'm OK if chezmoi does not handle all edge cases correctly.

[paltherr]

I'm working on the tests. I was pondering on what exactly to include and also how to express it with testscript. They should soon be ready.

[twpayne]

Please don't hesitate to ask questions or brainstorm about tests. Generally speaking, the more the better, and it doesn't matter if they're ugly. We can always tidy them up later.

[paltherr]

I have now cleaned up the code and added tests. In initconfig.txtar I test the various combinations of --config and --config-format option. I didn't test the case were the latter is present but not the former. That case looks non-sensical to me.

I published here a proposal on how format conflicts could be detected and signaled. It's much more involved than the error check that I initially had in this PR. The code path for cases where --config is present looks robust to me. I'm less confident about the code path for case where it's absent. In particular, I'm not 100% certain that it will warn when and only when writing the config file would break future chezmoi commands run with the same options. I'm also not sure whether that code path should take --config-format into account even though running chezmoi with that option but without --config looks non-sensical to me. Another question is whether the two code paths (for with or without --config) should have dedicated warnings. These could be a bit more precise and could also hint at what the user will have to do: rename the config file in one case or delete the old config file in the other case. Let me know what you think.

[twpayne]

Looks excellent.

Please follow the existing style of testscripts, specifically with respect to removing empty lines between comments and the commands they refer to as they have a meaning to testscript.

[twpayne]

Use

! stdout .
! stderr .

to check that there was no output on stdout or stderr. Comparing with /dev/null doesn't work on Windows. This should allow the tests to pass on Windows.

[paltherr]

The latest error is a bit weird. First I thought that it was because I should have used $/ instead of / but after looking more closely, it looks like that chezmoi prints UNIX-like paths (with /) even when it runs on Windows and the problem is that $CHEZMOICONFIGDIR, $HOME, and $WORK contain Windows-like paths (with ). Is there a way to rewrite variables when they are expanded by testscript?

[twpayne]

Sadly it's really tricky to get tests that test paths to pass on Windows. chezmoi uses forward slashes internally on all platforms, but testscript uses the native path separator, i.e. backward slashes on Windows. When you mix chezmoi paths with testscript paths then you get a mix of the two.

In the short term, please add

[windows] skip 'test requires path separator to be forward slash'

as the first line of the failing testscript tests. I hope to clean this up sometime in the future.

Once that's done, please also squash all commits into a single one using this PR's title as the commit message, and then I think this PR is good to merge :) Thank you for your patience on this one.

[twpayne]

This is a fantastic PR that fixes a long-standing bug in chezmoi. Thank you so much!