Skip specified settings in .gitconfig #340

ghost · 2020-09-28T02:21:28Z

Bool option couldn't be disabled by command.
There was three options to solve this,

Make all bool options into string <- kind of messy.
Make --no-** flag <- kind of messy. Need to add each no-flags. Even if How to define mutually competing flags TeXitoi/structopt#320 will be able to do, still need to handle in set_option.
[This PR] Change --no-gitconfig option to Vec, and it will ignore those specified keys. <- This one option can disable as much options as you want. Also, you can skip some non-bool config at same time. It will be useful.

I'm still not sure if this way is the best, so I titled 'suggestion'.
I would like to hear any ideas.

dandavison · 2020-09-28T14:49:44Z

Hi @Ryuta69, isn't this confusingly similar to the existing --no-gitconfig?

ghost · 2020-09-28T18:48:37Z

@dandavison
Hi, the biggest difference is that --no-gitconfig disables whole settings in git-config while --skip-config disables only settings you specified (eg, only line-numbers as original issue says.)

I personally didn't find it similar, but should I change the name maybe....?

ghost · 2020-10-06T04:27:00Z

@dandavison
Hi, sorry to ask you many times, but as I stated yesterday I would like to hear your opinion.

There is an another suggestion, how do you think below plans?

Make option's name be --ignore
Don't make new option. Make --no-gitconfig into string, and if value is all it will skip all gitconfig. If value is specific config, it will skip only that config.

dandavison · 2020-10-06T15:38:13Z

Hi @Ryuta69, thanks for working on this and thinking about it. I still need to think about it a bit more, but I do like your idea of changing --no-gitconfig to be a string. Would you agree that this isn't urgent, and the first priority is releasing your improvements to git add -p?

ghost · 2020-10-06T15:52:16Z

@dandavison
I agree!

As I stated here #331 (comment), this one is not urgent for release in my opinion.

I'm going to work for this, but there's no problem to release without this because the --pager is the first priotiry.

Because it is more clear for users who often used this option. If this remains, they will be surprised at this option not working withuout errors.

ghost · 2020-10-10T17:20:43Z

src/git_config.rs

        }
+
+        if !self.ignored_keys.is_empty() {
+            let key_name = &key.rsplit('.').collect::<Vec<&str>>()[0];


keys will be

core.pager

delta.line-numbers

delta.features.line-numbers

I think users don't want to specify parent key, so I split key here.

ghost · 2020-10-10T17:31:54Z

@dandavison
Hi, I fixed this PR by changing --no-gitconfig to be a string.
I would be happy if you could review this when you have a time.

dandavison · 2020-10-10T18:30:53Z

Thanks @Ryuta69, I'll review as soon as I can.

Your other changes are all merged, right? So we could do a delta release now?

ghost · 2020-10-10T18:43:51Z

@dandavison
Absolutely!

All other PRs are merged, so there are no problems for release.

dandavison · 2020-10-11T14:39:44Z

OK, all your changes are released in delta 0.4.4, thanks very much for all the work!

ghost · 2020-12-12T21:32:18Z

@dandavison
Hi, just a reminder.
I'd be happy if you could review this when you have a time.

If this approach is preferrable, I'm going to start fix conflicts.

dandavison · 2020-12-13T15:50:35Z

Hi @ulwlu, I'm hesitant about this one. Here is what I'm asking myself:

Is the added complexity to the interface justified?
Is it clear to users, or is it confusing?
In 🐛 Can't disable line numbering from command line #307 @HaleTom said

It would be great to be able to disable any boolean with --no-*, to override something that had been previously enabled (eg by a script or bash alias)

However, the approach in this PR would work for neither scripts nor bash aliases since it would not override a command line arg.
This wouldn't be a standard pattern for a UNIX tool; it somehow feels a bit convoluted to me to have an option specifying which entries in a config file to disregard.
I wonder whether we should consider environment variables instead? For example, perhaps for every delta flag, delta could look an an environment variable with a predictable name: DELTA_NO_LINE_NUMBERS, DELTA_NO_SIDE_BY_SIDE, in general if there is a boolean setting xxx-yyy then delta would automatically look for DELTA_NO_XXX_YYY.

But I don't have a clear sense here of what is best.

ghost · 2020-12-13T15:59:18Z

@dandavison
I agree with your concern, and now thinking below is the cleanest way to solve.

I wonder whether we should consider environment variables instead? For example, perhaps for every delta flag, delta could look an an environment variable with a predictable name: DELTA_NO_LINE_NUMBERS, DELTA_NO_SIDE_BY_SIDE, in general if there is a boolean setting xxx-yyy then delta would automatically look for DELTA_NO_XXX_YYY

The purpose of creating this PR was written in description (I am trying to achieve with less maintenance. Since there is no automatic negative boolean in clap).

I'm going to close this, and will keep above plan in mind.

dandavison · 2020-12-13T18:38:06Z

Thanks @ulwlu, sounds like we agree. Let's keep thinking about the environment variable option. Above I suggested DELTA_NO_XXX_YYY, but another option would be DELTA_XXX_YYY. This way the env var could be used to override gitconfig in both a positive and negative direction. We would have to decide on the rules for which strings mean true and which mean false though, which is always a slightly difficult decision.

HaleTom · 2020-12-14T06:42:00Z

Could we have a 2nd config file, possibly using the same format as gitconfig, and overriding anything set there?

dandavison · 2020-12-14T07:19:26Z

@HaleTom can you expand on that -- what's an example of a problem that would be solved by having a second config file?

dandavison · 2020-12-14T08:06:48Z

@HaleTom I think I see what you're suggesting -- we could have a second config file containing an alternative config that we sometimes need to use as a variant, overriding values in the main config file. I think that we actually already have that: that is essentially what named custom "features" are. There are already precedence rules built into the features implementation, so they allow overriding in a predictable way. And we can define as many variant configs as we want as named features. The only question seems to be -- how do we select our features conveniently at the command line (which would also be a question for your suggestion of having a second config file, right?)

I am thinking that we want this to be possible without having to pipe git into delta to apply a command-line arg, in which case an environment variable seems like the way forwards, since that will affect e.g. git diff without needing to explicitly invoke a delta process.

So as a third variant of the above environment variable ideas, I wonder whether we only need one new environment variable: DELTA_FEATURES. This would allow us to activate a named custom feature (with higher priority than any features already named in gitconfig). Then we can do anything we want in the named feature sections, including disabling boolean flags like line-numbers. I'm thinking this might make sense because in practice, each user will only have a small number of variants that they need, and so those variants can be hard-coded in gitconfig and given a convenient name.

dandavison · 2020-12-14T10:40:03Z

Just giving that a very quick go I see that a bit of work might be required: the following would ideally have worked first time but did not:

[delta]
    line-numbers = true
    features = no-line-numbers

[delta "no-line-numbers"]
    line-numbers = false

HaleTom · 2020-12-17T00:55:21Z

You got me, @dandavison . Cheers!

ghost · 2020-12-18T06:17:18Z

@dandavison Hi shall I check that behaviour?

the following would ideally have worked first time but did not

I actually found this bug when fixing git add pager, because some styles like hunk_header_style breaks pager while hunk_header_style in features won't break it.

I totally forgot where it happens, but maybe can fix.

dandavison · 2020-12-18T11:18:14Z

@dandavison Hi shall I check that behaviour?

@ulwlu , thanks! Yes, that would be great. I opened #446 for it. In addition to #307 I think that DELTA_FEATURES might be a solution for #447 also (as you know), so it would be great to get it working exactly as expected for all options.

ghost · 2020-12-18T16:13:33Z

Okay! I'm gonna start working this in a few days because I need take a surgery today.

dandavison · 2020-12-18T16:27:57Z

Awesome, no hurry! I hope that goes smoothly and you're feeling good after.

ryuta69 added 3 commits September 28, 2020 10:55

Add skip_config cli_option

2ed8627

Add test for skip-config

8bfacf0

Add should_skip_config fn to filter when set_option

596b1f4

ghost changed the title ~~[Suggestion] Feature skip-config option to disable git-config value.~~ [Suggestion] Feature skip-config option to disable git-config value of only keys you specify (while --git-config disables whole settings.)). Sep 28, 2020

ghost mentioned this pull request Oct 4, 2020

Add identical test when color_only and line_numbers are enabled at same time #331

Merged

ryuta69 added 6 commits October 11, 2020 01:59

Revert skip-config option

eb2ceb3

Refactor --no-gitconfig to Vec<String> to ignore specified settings

0566027

Fix tests using --no-gitconfig option to specify all

946aecb

Add test for --no-gitconfig

bd70417

Update README for --no-gitconfig

a379b37

Delete default value from no-gitconfig

9161aab

Because it is more clear for users who often used this option. If this remains, they will be surprised at this option not working withuout errors.

ghost changed the title ~~[Suggestion] Feature skip-config option to disable git-config value of only keys you specify (while --git-config disables whole settings.)).~~ Skip specified settings in .gitconfig Oct 10, 2020

ghost commented Oct 10, 2020

View reviewed changes

ghost closed this Dec 13, 2020

dandavison mentioned this pull request Dec 14, 2020

🐛 Can't disable line-numbers in a custom feature #446

Open

dandavison mentioned this pull request Dec 17, 2020

🚀 Use light/dark theme base on terminal theme or system variable. #447

Closed

dandavison mentioned this pull request Dec 18, 2020

🐛 Can't disable line numbering from command line #307

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip specified settings in .gitconfig #340

Skip specified settings in .gitconfig #340

ghost commented Sep 28, 2020 •

edited by ghost

Loading

dandavison commented Sep 28, 2020

ghost commented Sep 28, 2020 •

edited by ghost

Loading

ghost commented Oct 6, 2020

dandavison commented Oct 6, 2020 •

edited

Loading

ghost commented Oct 6, 2020

ghost Oct 10, 2020

ghost commented Oct 10, 2020

dandavison commented Oct 10, 2020

ghost commented Oct 10, 2020

dandavison commented Oct 11, 2020

ghost commented Dec 12, 2020 •

edited by ghost

Loading

dandavison commented Dec 13, 2020 •

edited

Loading

ghost commented Dec 13, 2020

dandavison commented Dec 13, 2020 •

edited

Loading

HaleTom commented Dec 14, 2020

dandavison commented Dec 14, 2020

dandavison commented Dec 14, 2020

dandavison commented Dec 14, 2020

HaleTom commented Dec 17, 2020

ghost commented Dec 18, 2020

dandavison commented Dec 18, 2020

ghost commented Dec 18, 2020

dandavison commented Dec 18, 2020

Skip specified settings in .gitconfig #340

Skip specified settings in .gitconfig #340

Conversation

ghost commented Sep 28, 2020 • edited by ghost Loading

dandavison commented Sep 28, 2020

ghost commented Sep 28, 2020 • edited by ghost Loading

ghost commented Oct 6, 2020

dandavison commented Oct 6, 2020 • edited Loading

ghost commented Oct 6, 2020

ghost Oct 10, 2020

Choose a reason for hiding this comment

ghost commented Oct 10, 2020

dandavison commented Oct 10, 2020

ghost commented Oct 10, 2020

dandavison commented Oct 11, 2020

ghost commented Dec 12, 2020 • edited by ghost Loading

dandavison commented Dec 13, 2020 • edited Loading

ghost commented Dec 13, 2020

dandavison commented Dec 13, 2020 • edited Loading

HaleTom commented Dec 14, 2020

dandavison commented Dec 14, 2020

dandavison commented Dec 14, 2020

dandavison commented Dec 14, 2020

HaleTom commented Dec 17, 2020

ghost commented Dec 18, 2020

dandavison commented Dec 18, 2020

ghost commented Dec 18, 2020

dandavison commented Dec 18, 2020

ghost commented Sep 28, 2020 •

edited by ghost

Loading

ghost commented Sep 28, 2020 •

edited by ghost

Loading

dandavison commented Oct 6, 2020 •

edited

Loading

ghost commented Dec 12, 2020 •

edited by ghost

Loading

dandavison commented Dec 13, 2020 •

edited

Loading

dandavison commented Dec 13, 2020 •

edited

Loading