Re: [PATCH v2 00/10] send-email: various optimizations to speed up by >2x

[Jeff King <peff@peff.net>]

On Fri, May 21, 2021 at 08:23:15AM +0200, Ævar Arnfjörð Bjarmason wrote:

> The "performant" really doesn't matter here, we're comparing shelling
> out to getting a small number of config keys back. So I wasn't trying to
> optimize this.
> 
> Returning a flattened list is idiomatic in Perl, it means that a caller
> can do any of:
> 
>     # I only care about the last value for a key, or only about
>     # existence checks
>     my %hash = func();
> 
> Or:
> 
>     # I want all key-values to iterate over
>     my @kv = func();
> 
> Returning touples like this makes that less convenient for both, who'll
> need to do more work to unpack them.
> 
> For what it's worth In Perl "return a list" means "flattened list", the
> term "flattened list" I think comes from other languages. You'd call
> what you're suggesting a list of arrays, or (if a top-level reference)
> an array of arrays, AoA for short, AoH for array (ref) of hashes etc.

Yeah, I think that is reasonable. But it made me wonder how we handle
value-less booleans, and I think there's indeed a bug.

Try a config like this:

  $ cat >foo <<\EOF
  [foo]
  key-with-value = string
  key-with-empty =
  just-bool
  another-key-with-value = another
  EOF

A regular config --list looks like:

  $ git config --file=foo --list
  foo.key-with-value=string
  foo.key-with-empty=
  foo.just-bool
  foo.another-key-with-value=another

Note how "just-bool" drops the "=" to distinguish it from the empty
string. With "-z", it looks like this:

  $ git config --file=foo --list -z
  foo.key-with-value
  string^@foo.key-with-empty
  ^@foo.just-bool^@foo.another-key-with-value
  another^@

The NULs separate keys, but keys are separated from their values by a
newline. And again, just-bool omits the newline.

Your parser splits on newline, so for that entry we'll get only one
string returned in the list (the key), rather than two (the key and
value). In a flattened list, that becomes ambiguous. E.g., adapting your
parser into a stand-alone script:

  $ git config --file=foo --list -z |
    perl -e '
      local $/;
      my $data = <STDIN>;
      my (@kv) = map { split /\n/, $_, 2 } split /\0/, $data;

      while (@kv) {
        my $k = shift @kv;
	print "key: $k\n";
	my $v = shift @kv;
	print "  value: ", (defined $v ? $v : "undef"), "\n";
      }
    '
  key: foo.key-with-value
    value: string
  key: foo.key-with-empty
    value: 
  key: foo.just-bool
    value: foo.another-key-with-value
  key: another
    value: undef

We end up misinterpreting a key as a value, and vice versa.

Using a non-flattened structure would have prevented this (we'd sensibly
get undef when trying to access the missing second element of the
array). But I do agree the flattened structure is more perl-ish.
Probably you'd want to insert an explicit "undef" into the list. The
most perl-ish I could come up with is:

  my (@kv) = map { my ($k, $v) = split /\n/, $_, 2;
                   ($k, $v)
                 } split /\0/, $data;

I notice that $known_keys then becomes a non-flat representation. You'd
either want to turn that back into a zero-length array there, or store
the "undef" and handle it appropriately (it can be a synonym for "true",
though that is just an optimization at this point).

-Peff