.gitconfig parser #2395

dominikschulz · 2022-10-24T19:30:49Z

This PR adds a new config format - again. This time it's based on git's very own "gitconfig" format. Unfortunately it's not entirely trivial and to date there seem to be no git config handlers in pure Go. All I've found were shelling out to the git binary (which does make sense, as most of the options do have default values that only the binary would know about).

But I still think this format is best suited for our use case since it does combine human and machine accessibility with the right amount of flexibility.

WARNING: This will certainly break some things! The current config handling is so deeply wired in and mixed with our (ab-)use of the context package that I'm unlikely to catch every possible breakage on my own. Our users will have to help testing this. Either before it's released or afterwards. But the upshot is that this has the potential to allow us to resolve at least half a dozen outstanding issues and remove quite a bit of legacy from the codebase (more so once we drop the config backwards compatability layer). We could even consider adopting the parser for the secrets themselves in the future.

Fixes #1567
Fixes #1764
Fixes #1819
Fixes #1878
Fixes #2387
Fixes #2408
Fixes #2418

Signed-off-by: Dominik Schulz dominik.schulz@gauner.org

AnomalRoil

Okay, I haven't yet started to dig into the big changes yet. A few comments. I'll keep going later.

Makefile

docs/config.md

fish.completion

internal/action/commands.go

internal/action/config.go

internal/action/commands.go

gopasspw/gopass#2395

AnomalRoil

70/114 reviewed, but I'm getting to the big chunks. Here's a few initial comments and I'll go through the rest tomorrow hopefully.

helpers/changelog/main.go

internal/action/generate.go

internal/action/show.go

internal/action/show_test.go

internal/action/sync.go

internal/action/sync_test.go

pkg/ctxutil/ctxutil.go

internal/config/config.go

internal/action/sync.go

internal/config/legacy.go

internal/config/legacy/legacy.go

AnomalRoil

Okay, 105/114 I just haven't looked at the gitconfig package itself right now.

internal/config/legacy/location.go

internal/store/leaf/read.go

internal/config/config.go

tests/gptest/gunit.go

tests/config_test.go

docs/config.md

internal/action/sync.go

internal/config/docs_test.go

pkg/gitconfig/gitconfig.go

pkg/gitconfig/gitconfig_test.go

AnomalRoil · 2022-11-22T19:03:33Z

pkg/gitconfig/gitconfig.go

+	localConfig = "config"
+	// WorktreeConfig is the name of the local worktree configuration. Can be used to override
+	// a committed local config.
+	worktreeConfig = "config.worktree"


I think that worktree configs are even worse, since their config is stored in the original repo's .git folder under .git/worktrees/nameofwt/config.worktree

e.g. if I have a worktree named "myown" located at /home/anomalroil/code/myown for gopass it would look like:

file:/home/anomalroil/.gitconfig commit.gpgsign=true file:/home/anomalroil/code/gopass/.git/config user.email=anomalroil@users.noreply.github.com file:/home/anomalroil/code/gopass/.git/worktrees/myown/config.worktree user.name=TEST

because in a worktree, the folder .git is replaced with a file .git specifying the location of its original gitdir:

$ cat .git gitdir: /home/anomalroil/code/drand/.git/worktrees/myown

Is this currently taken into account?

No, it isn't. And I wasn't aware of that. Thank you. I don't necessarily plan to support this as part of this PR, but it's definitely something that needs to be documented and should have a TODO. Will add that and then resolve this comment.

Reading https://git-scm.com/docs/git-config#SCOPES I'm not sure if that's correct - but I haven't used this feature before so I might be wrong.

main.go

AnomalRoil

Last one for today, I hope to wrap it up tomorrow!

pkg/gitconfig/config.go

internal/config/docs_test.go

pkg/gitconfig/config.go

dominikschulz · 2022-11-23T08:45:30Z

@AnomalRoil Thanks a lot. I'll probably not get to address the open comments until tomorrow anyway. If you could leave any remaining comments today that'd be fantastic.

Maybe we can wrap this up until the end of the week 😃

AnomalRoil · 2022-11-24T21:09:24Z

docs/config.md

+| `mounts.path`          | `string` | Path to the root store. | `$XDG_DATA_HOME/gopass/stores/root` |
+| `core.showsafecontent` | `bool`   | Only output *safe content* (i.e. everything but the first line of a secret) to the terminal. Use *copy* (`-c`) to retrieve the password in the clipboard, or *force* (`-f`) to still print it. | `false` |
+| `age.usekeychain`      | `bool`   | Use the OS keychain to cache age passphrases. | `false` |
+| `domain-alias.<from>`   | `string` | Alias from domain to the string value of this entry. | `` |


This still holds I guess ^
`` isn't valid markdown on GH.

AnomalRoil · 2022-11-24T21:13:11Z

internal/action/sync.go

@@ -32,6 +33,8 @@ func init() {
 		return
 	}

+	debug.Log("GOPASS_AUTOSYNC_INTERVAL is deprecated. Please use autosync.interval")


I guess this is a silent debug.Log instead of a noisy out.Warning because of the lack of context in init?

AnomalRoil · 2022-11-24T21:14:29Z

internal/action/sync.go

@@ -181,11 +204,12 @@ func (s *Action) syncMount(ctx context.Context, mp string) error {
 	}
 	syncPrintDiff(ctxno, l, ln)

-	debug.Log("Syncing Mount %s. Exportkeys: %t", mp, ctxutil.IsExportKeys(ctx))
+	exportKeys := s.cfg.GetBool("core.exportkeys")


Shouldn't this also be a s.cfg.GetM since it could be mount-specific exportkeys setting?

pkg/gitconfig/configs.go

pkg/gitconfig/config.go

pkg/gitconfig/configs.go

pkg/gitconfig/config.go

pkg/gitconfig/configs.go

pkg/gitconfig/config.go

AnomalRoil · 2022-11-24T22:31:23Z

pkg/gitconfig/configs.go

+		if cfg == nil {
+			continue
+		}
+		if cfg.vars == nil {
+			continue
+		}


Suggested change

if cfg == nil {

continue

}

if cfg.vars == nil {

continue

}

if cfg == nil || cfg.vars == nil {

continue

}

Actually we don't even have to check for cfg.vars == nil. range handles that properly.

pkg/gitconfig/configs.go

AnomalRoil

LGTM!

Maybe just 2 last changes!

Correct the doc in:
https://github.com/gopasspw/gopass/blob/dcea35c0c97ead9529da76797eba24d3ed08bdab/docs/commands/config.md

And add the store flag to the config command:

gopass/internal/action/commands.go

Lines 157 to 168 in dcea35c

    
           { 
        
           	Name:      "config", 
        
           	Usage:     "Display and edit the configuration file", 
        
           	ArgsUsage: "[key [value]]", 
        
           	Description: "" + 
        
           		"This command allows for easy printing and editing of the configuration. " + 
        
           		"Without argument, the entire config is printed. " + 
        
           		"With a single argument, a single key can be printed. " + 
        
           		"With two arguments a setting specified by key can be set to value.", 
        
           	Action:       s.Config, 
        
           	BashComplete: s.ConfigComplete, 
        
           },

AnomalRoil · 2022-11-25T09:04:50Z

Ah, it seems a few extra documentation places needs updates too:

https://github.com/gopasspw/gopass/blob/dcea35c0c97ead9529da76797eba24d3ed08bdab/docs/usecases/readyonly-store.md (also the name of that file seem wrong, why "ready" and not "read"?)
https://github.com/gopasspw/gopass/blob/dcea35c0c97ead9529da76797eba24d3ed08bdab/docs/setup.md
https://github.com/gopasspw/gopass/blob/dcea35c0c97ead9529da76797eba24d3ed08bdab/docs/config.md (still contains mention of "autoclip")
https://github.com/gopasspw/gopass/blob/dcea35c0c97ead9529da76797eba24d3ed08bdab/docs/commands/create.md (The example uses $(gopass config path))

dominikschulz · 2022-11-25T09:12:56Z

@AnomalRoil Thanks again. Updated all of these.

AnomalRoil · 2022-11-25T09:20:47Z

The following didn't work for me:

gopass config core.safecontent true
echo """this is a test
on two line""" | gopass insert test
gopass show test

It displayed the two lines instead of hiding the first line.

internal/action/show.go

AnomalRoil · 2022-11-25T09:28:35Z

Ah, it seems the problem is actually in the migration path, since in the old config.yml file, showsafecontent was named safecontent only

dominikschulz · 2022-11-25T09:30:28Z

Ack. Fixed that.

docs/usecases/readonly-store.md

AnomalRoil · 2022-11-25T09:32:44Z

docs/config.md

+| `mounts.path`          | `string` | Path to the root store. | `$XDG_DATA_HOME/gopass/stores/root` |
+| `core.showsafecontent` | `bool`   | Only output *safe content* (i.e. everything but the first line of a secret) to the terminal. Use *copy* (`-c`) to retrieve the password in the clipboard, or *force* (`-f`) to still print it. | `false` |
+| `age.usekeychain`      | `bool`   | Use the OS keychain to cache age passphrases. | `false` |
+| `domain-alias.<from>`   | `string` | Alias from domain to the string value of this entry. | `` |


This still holds I guess ^
`` isn't valid markdown on GH.

AnomalRoil · 2022-11-25T09:37:46Z

I wonder how the migration path would be for old config files that still contain legacy options in their YAML:

check_recipient_hash
concurrency

and others

This commit adds yet another config handler for gopass. It is based on the format used by git itself. This has the potential to address a lot of long standing issues, but it also causes a lot of changes to how we handle configuration, so bugs are inevitable. Fixes gopasspw#1567 Fixes gopasspw#1764 Fixes gopasspw#1819 Fixes gopasspw#1878 Fixes gopasspw#2387 Fixes gopasspw#2418 RELEASE_NOTES=[BREAKING] New config format based on git config. Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org> Co-authored-by: Yolan Romailler <AnomalRoil@users.noreply.github.com> address comments Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

dominikschulz · 2022-11-25T09:39:05Z

These old configs will just be caried over with the core. prefix. But they will not be used by anything.

dominikschulz self-assigned this Nov 3, 2022

dominikschulz added the feature Enhancements and new features label Nov 3, 2022

dominikschulz added this to the 1.x.x milestone Nov 3, 2022

dominikschulz requested a review from AnomalRoil November 5, 2022 19:20

dominikschulz marked this pull request as ready for review November 6, 2022 13:00

dominikschulz modified the milestones: 1.x.x, 1.15.0 Nov 6, 2022

dominikschulz force-pushed the feat/gitconfig branch from 7fe184b to e5b1560 Compare November 6, 2022 19:29

dominikschulz mentioned this pull request Nov 8, 2022

gopass sync with no_push remote and missing GPG public keys #2408

Closed

AnomalRoil reviewed Nov 8, 2022

View reviewed changes

dominikschulz requested a review from AnomalRoil November 9, 2022 08:41

dominikschulz force-pushed the feat/gitconfig branch from 0667dd2 to 20c15ba Compare November 9, 2022 18:01

christian-heusel added a commit to christian-heusel/dotfiles that referenced this pull request Nov 9, 2022

upgrade to the new gopass 1.15.0 config format

2af4993

gopasspw/gopass#2395

dominikschulz force-pushed the feat/gitconfig branch from 2781b20 to dfb8fcb Compare November 12, 2022 20:16

AnomalRoil reviewed Nov 17, 2022

View reviewed changes

dominikschulz mentioned this pull request Nov 17, 2022

gitconfig cleanup #2418

Closed

dominikschulz force-pushed the feat/gitconfig branch 2 times, most recently from cb9d479 to 0c5a9ec Compare November 18, 2022 17:33

AnomalRoil reviewed Nov 18, 2022

View reviewed changes

internal/config/config.go Show resolved Hide resolved

internal/action/sync.go Show resolved Hide resolved

internal/config/legacy.go Show resolved Hide resolved

internal/config/legacy/legacy.go Show resolved Hide resolved

AnomalRoil reviewed Nov 18, 2022

View reviewed changes

dominikschulz force-pushed the feat/gitconfig branch 6 times, most recently from 4722169 to c144d02 Compare November 20, 2022 17:56

AnomalRoil reviewed Nov 22, 2022

View reviewed changes

docs/config.md Outdated Show resolved Hide resolved

docs/config.md Outdated Show resolved Hide resolved

docs/config.md Outdated Show resolved Hide resolved

internal/action/sync.go Outdated Show resolved Hide resolved

AnomalRoil reviewed Nov 22, 2022

View reviewed changes

main.go Outdated Show resolved Hide resolved

AnomalRoil reviewed Nov 22, 2022

View reviewed changes

pkg/gitconfig/config.go Outdated Show resolved Hide resolved

pkg/gitconfig/config.go Outdated Show resolved Hide resolved

internal/config/docs_test.go Show resolved Hide resolved

pkg/gitconfig/config.go Outdated Show resolved Hide resolved

dominikschulz requested a review from AnomalRoil November 24, 2022 07:23

dominikschulz force-pushed the feat/gitconfig branch from 0fae794 to 6b2a2af Compare November 24, 2022 18:02

AnomalRoil reviewed Nov 24, 2022

View reviewed changes

pkg/gitconfig/configs.go Show resolved Hide resolved

pkg/gitconfig/config.go Show resolved Hide resolved

pkg/gitconfig/configs.go Show resolved Hide resolved

pkg/gitconfig/config.go Show resolved Hide resolved

AnomalRoil reviewed Nov 24, 2022

View reviewed changes

pkg/gitconfig/configs.go Outdated Show resolved Hide resolved

dominikschulz force-pushed the feat/gitconfig branch 2 times, most recently from 1632f28 to 4c47698 Compare November 25, 2022 08:12

AnomalRoil previously approved these changes Nov 25, 2022

View reviewed changes

dominikschulz dismissed AnomalRoil’s stale review via 702be31 November 25, 2022 09:12

dominikschulz force-pushed the feat/gitconfig branch from 4c47698 to 702be31 Compare November 25, 2022 09:12

AnomalRoil requested changes Nov 25, 2022

View reviewed changes

internal/action/show.go Show resolved Hide resolved

internal/action/show.go Show resolved Hide resolved

internal/action/show.go Show resolved Hide resolved

dominikschulz force-pushed the feat/gitconfig branch from 702be31 to d8ab89a Compare November 25, 2022 09:30

AnomalRoil reviewed Nov 25, 2022

View reviewed changes

dominikschulz force-pushed the feat/gitconfig branch from d8ab89a to bf8bd27 Compare November 25, 2022 09:38

dominikschulz merged commit 18ffee3 into gopasspw:master Nov 25, 2022

dominikschulz deleted the feat/gitconfig branch November 25, 2022 09:50

dominikschulz mentioned this pull request Dec 4, 2022

Cleanup: Migrate from ctxutil to option structs where feasible #1798

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.gitconfig parser #2395

.gitconfig parser #2395

dominikschulz commented Oct 24, 2022 •

edited

Loading

AnomalRoil left a comment

AnomalRoil left a comment

AnomalRoil left a comment

AnomalRoil Nov 22, 2022

dominikschulz Nov 23, 2022

dominikschulz Nov 23, 2022

AnomalRoil left a comment

dominikschulz commented Nov 23, 2022

AnomalRoil Nov 24, 2022

AnomalRoil Nov 25, 2022 •

edited

Loading

AnomalRoil Nov 24, 2022

AnomalRoil Nov 24, 2022

AnomalRoil Nov 24, 2022

dominikschulz Nov 25, 2022

AnomalRoil left a comment

AnomalRoil commented Nov 25, 2022

dominikschulz commented Nov 25, 2022

AnomalRoil commented Nov 25, 2022

AnomalRoil commented Nov 25, 2022

dominikschulz commented Nov 25, 2022

AnomalRoil Nov 25, 2022 •

edited

Loading

AnomalRoil commented Nov 25, 2022

dominikschulz commented Nov 25, 2022

	\| `domain-alias.<from>` \| `string` \| Alias from domain to the string value of this entry. \| `` \|
	\| `domain-alias.<from>` \| `string` \| Alias from domain to the string value of this entry. \| `""` \|

	{
	Name: "config",
	Usage: "Display and edit the configuration file",
	ArgsUsage: "[key [value]]",
	Description: "" +
	"This command allows for easy printing and editing of the configuration. " +
	"Without argument, the entire config is printed. " +
	"With a single argument, a single key can be printed. " +
	"With two arguments a setting specified by key can be set to value.",
	Action: s.Config,
	BashComplete: s.ConfigComplete,
	},

.gitconfig parser #2395

.gitconfig parser #2395

Conversation

dominikschulz commented Oct 24, 2022 • edited Loading

AnomalRoil left a comment

Choose a reason for hiding this comment

AnomalRoil left a comment

Choose a reason for hiding this comment

AnomalRoil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnomalRoil left a comment

Choose a reason for hiding this comment

dominikschulz commented Nov 23, 2022

Choose a reason for hiding this comment

AnomalRoil Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnomalRoil left a comment

Choose a reason for hiding this comment

AnomalRoil commented Nov 25, 2022

dominikschulz commented Nov 25, 2022

AnomalRoil commented Nov 25, 2022

AnomalRoil commented Nov 25, 2022

dominikschulz commented Nov 25, 2022

AnomalRoil Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

AnomalRoil commented Nov 25, 2022

dominikschulz commented Nov 25, 2022

dominikschulz commented Oct 24, 2022 •

edited

Loading

AnomalRoil Nov 25, 2022 •

edited

Loading

AnomalRoil Nov 25, 2022 •

edited

Loading