Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

Hi Ævar,

On Fri, 30 Mar 2018, Ævar Arnfjörð Bjarmason wrote:

> 
> On Thu, Mar 29 2018, Johannes Schindelin wrote:
> 
> > Nonetheless, I would be confortable with this patch going into
> > v2.17.0, even at this late stage. The final verdict is Junio's, of
> > course.
> 
> Thanks a lot for working on this. I'm keen to stress test this, but
> won't have time in the next few days, and in any case think that the
> parts that change functionality should wait until after 2.17 (but e.g.
> the test renaming would be fine for a cherry-pick).

Obviously this was never meant to get into v2.17.0 (apart maybe from 1/9,
which however is so contested over that addition of the test case under
the assumption that anybody but me would dare to touch those parts of the
code).

Ciao,
Dscho

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 29 2018, Johannes Schindelin wrote:

> Nonetheless, I would be confortable with this patch going into v2.17.0, even 
> at
> this late stage. The final verdict is Junio's, of course.

Thanks a lot for working on this. I'm keen to stress test this, but
won't have time in the next few days, and in any case think that the
parts that change functionality should wait until after 2.17 (but
e.g. the test renaming would be fine for a cherry-pick).

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

Hi Peff,

On Thu, 29 Mar 2018, Jeff King wrote:

> On Thu, Mar 29, 2018 at 05:18:30PM +0200, Johannes Schindelin wrote:
> 
> > The first patch is somewhat of a "while at it" bug fix that I first
> > thought would be a lot more critical than it actually is: It really
> > only affects config files that start with a section followed
> > immediately (i.e. without a newline) by a one-letter boolean setting
> > (i.e. without a `= ` part). So while it is a real bug fix, I
> > doubt anybody ever got bitten by it.
> 
> That makes me wonder if somebody could craft a malicious config to do
> something bad.

I thought about that, and could not think of anything other than social
engineering vectors. Even in that case, the error message is instructive
enough that the user should be able to fix the config without consulting
StackOverflow.

> > Now, to the really important part: why does this patch series not
> > conflict with my very early statements that we cannot simply remove
> > empty sections because we may end up with stale comments?
> > 
> > Well, the patch in question takes pains to determine *iff* there are
> > any comments surrounding, or included in, the section. If any are
> > found: previous behavior. Under the assumption that the user edited
> > the file, we keep it as intact as possible (see below for some
> > argument against this). If no comments are found, and let's face it,
> > this is probably *the* common case, as few people edit their config
> > files by hand these days (neither should they because it is too easy
> > to end up with an unparseable one), the now-empty section *is*
> > removed.
> 
> I'm not against people editing their config files by hand. But I think
> what you propose here makes a lot of sense, because it works as long as
> you don't intermingle hand- and auto-editing in the same section (and it
> even works if you do intermingle, as long as you don't use comments,
> which are probably even more rare).
> 
> So it seems like quite a sensible compromise, and I think should make
> most people happy.

Thanks for confirming my line of thinking,
Dscho

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

Hi Stefan,

On Thu, 29 Mar 2018, Stefan Beller wrote:

> On Thu, Mar 29, 2018 at 8:18 AM, Johannes Schindelin
>  wrote:
> 
> > So what is the argument against this extra care to detect comments? Well, if
> > you have something like this:
> >
> > [section]
> > ; Here we comment about the variable called snarf
> > snarf = froop
> >
> > and we run `git config --unset section.snarf`, we end up with this config:
> >
> > [section]
> > ; Here we comment about the variable called snarf
> >
> > which obviously does not make sense. However, that is already established
> > behavior for quite a few years, and I do not even try to think of a way how
> > this could be solved.
> 
> By commenting out the key/value pair instead of deleting it.
> It's called --unset, not --delete ;)

That would open the door to new bug reports when a user starts with this
concocted config:

[section]
# This is a comment about the `key` setting
key = value

and then does this:

git config --unset section.key
git config section.key value
git config --unset section.key
git config section.key value
git config --unset section.key
git config section.key value

and then ends up with a config like this:

[section]
# This is a comment about the `key` setting
;key = value
;key = value
;key = value
key = value

And note that the comment might be about `value` instead, so reusing a
commented-out `key` setting won't fly, either.

I *did* give this problem a couple of minutes of thought before writing my
assessment that is quoted above ;-)

Ciao,
Dscho

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Jeff King]

On Thu, Mar 29, 2018 at 05:18:30PM +0200, Johannes Schindelin wrote:

> Little did I know that this would turn not only into a full patch to fix this
> issue, but into a full-blown series of nine patches.

It's amazing how often that happens. :)

> The first patch is somewhat of a "while at it" bug fix that I first thought
> would be a lot more critical than it actually is: It really only affects 
> config
> files that start with a section followed immediately (i.e. without a newline)
> by a one-letter boolean setting (i.e. without a `= ` part). So while it
> is a real bug fix, I doubt anybody ever got bitten by it.

That makes me wonder if somebody could craft a malicious config to do
something bad. But I don't think so. Config is trusted already, and it
looks like this bug is both hard to trigger and doesn't result in any
kind of memory funniness, just a bogus output.

> Now, to the really important part: why does this patch series not conflict 
> with
> my very early statements that we cannot simply remove empty sections because 
> we
> may end up with stale comments?
> 
> Well, the patch in question takes pains to determine *iff* there are any
> comments surrounding, or included in, the section. If any are found: previous
> behavior. Under the assumption that the user edited the file, we keep it as
> intact as possible (see below for some argument against this). If no comments
> are found, and let's face it, this is probably *the* common case, as few 
> people
> edit their config files by hand these days (neither should they because it is
> too easy to end up with an unparseable one), the now-empty section *is*
> removed.

I'm not against people editing their config files by hand. But I think
what you propose here makes a lot of sense, because it works as long as
you don't intermingle hand- and auto-editing in the same section (and it
even works if you do intermingle, as long as you don't use comments,
which are probably even more rare).

So it seems like quite a sensible compromise, and I think should make
most people happy.

-Peff

Re: [PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Stefan Beller]

On Thu, Mar 29, 2018 at 8:18 AM, Johannes Schindelin
 wrote:

> So what is the argument against this extra care to detect comments? Well, if
> you have something like this:
>
> [section]
> ; Here we comment about the variable called snarf
> snarf = froop
>
> and we run `git config --unset section.snarf`, we end up with this config:
>
> [section]
> ; Here we comment about the variable called snarf
>
> which obviously does not make sense. However, that is already established
> behavior for quite a few years, and I do not even try to think of a way how
> this could be solved.

By commenting out the key/value pair instead of deleting it.
It's called --unset, not --delete ;)

Now onto reviewing the patches.

Stefan

[PATCH 0/9] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin]

This patch series started out as a single patch trying to figure out what it
takes to fix that annoying bug that has been reported several times over the
years, where `git config --unset` would leave empty sections behind, and `git
config --add` would not reuse them.

Little did I know that this would turn not only into a full patch to fix this
issue, but into a full-blown series of nine patches.

The first patch is somewhat of a "while at it" bug fix that I first thought
would be a lot more critical than it actually is: It really only affects config
files that start with a section followed immediately (i.e. without a newline)
by a one-letter boolean setting (i.e. without a `= ` part). So while it
is a real bug fix, I doubt anybody ever got bitten by it.

Nonetheless, I would be confortable with this patch going into v2.17.0, even at
this late stage. The final verdict is Junio's, of course.

The next swath of patches add some tests, and adjust one test about which I
already complained at length yesterday, so I will spare you the same ordeal
today. These fixes are pretty straight-forward, and I always try to keep my
added tests as concise as possible, so please tell me if you find a way to make
them smaller (without giving up readability and debuggability).

Finally, the interesting part, where I do two things, essentially (with
preparatory steps for each thing):

1. I add the ability for `git config --unset/--unset-all` to detect that it
   can remove a section that has just become empty (see below for some more
   discussion of what I consider "become empty"), and

2. I add the ability for `git config [--add] key value` to re-use empty
   sections.

Note that the --unset/--unset-all part is the hairy one, and I would love it if
people could concentrate on wrapping their heads around that function, and
obviously tell me how I can change it to make it more readable (or even point
out incorrect behavior).

Now, to the really important part: why does this patch series not conflict with
my very early statements that we cannot simply remove empty sections because we
may end up with stale comments?

Well, the patch in question takes pains to determine *iff* there are any
comments surrounding, or included in, the section. If any are found: previous
behavior. Under the assumption that the user edited the file, we keep it as
intact as possible (see below for some argument against this). If no comments
are found, and let's face it, this is probably *the* common case, as few people
edit their config files by hand these days (neither should they because it is
too easy to end up with an unparseable one), the now-empty section *is*
removed.

So what is the argument against this extra care to detect comments? Well, if
you have something like this:

[section]
; Here we comment about the variable called snarf
snarf = froop

and we run `git config --unset section.snarf`, we end up with this config:

[section]
; Here we comment about the variable called snarf

which obviously does not make sense. However, that is already established
behavior for quite a few years, and I do not even try to think of a way how
this could be solved.


Johannes Schindelin (9):
  git_config_set: fix off-by-two
  t1300: rename it to reflect that `repo-config` was deprecated
  t1300: avoid relying on a bug
  t1300: remove unreasonable expectation from TODO
  t1300: `--unset-all` can leave an empty section behind (bug)
  git_config_set: simplify the way the section name is remembered
  git config --unset: remove empty sections (in normal situations)
  git_config_set: use do_config_from_file() directly
  git_config_set: reuse empty sections

 config.c| 234 +---
 t/{t1300-repo-config.sh => t1300-config.sh} |  36 -
 2 files changed, 250 insertions(+), 20 deletions(-)
 rename t/{t1300-repo-config.sh => t1300-config.sh} (98%)


base-commit: 03df4959472e7d4b5117bb72ac86e1e2bcf21723
Published-As: https://github.com/dscho/git/releases/tag/empty-config-section-v1
Fetch-It-Via: git fetch https://github.com/dscho/git empty-config-section-v1
-- 
2.16.2.windows.1.26.g2cc3565eb4b