<feed xmlns='http://www.w3.org/2005/Atom'>
<title>delta/libgit2.git/src/diff_driver.c, branch ethomson/https_proxy</title>
<subtitle>github.com: libgit2/libgit2.git
</subtitle>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/'/>
<entry>
<title>diff_driver: detect memory allocation errors when loading diff driver</title>
<updated>2019-05-19T10:12:13+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2018-03-02T12:18:59+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=31f8f82a65c11dfe9e57e618446aaff8cc21d84a'/>
<id>31f8f82a65c11dfe9e57e618446aaff8cc21d84a</id>
<content type='text'>
When searching for a configuration key for the diff driver, we construct
the config key by modifying a buffer and then passing it to
`git_config_get_multivar_foreach`. We do not check though whether the
modification of the buffer actually succeded, so we could in theory end
up passing the OOM buffer to the config function.

Fix that by checking return codes. While at it, switch to use
`git_buf_PUTS` to avoid repetition of the appended string to calculate
its length.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
When searching for a configuration key for the diff driver, we construct
the config key by modifying a buffer and then passing it to
`git_config_get_multivar_foreach`. We do not check though whether the
modification of the buffer actually succeded, so we could in theory end
up passing the OOM buffer to the config function.

Fix that by checking return codes. While at it, switch to use
`git_buf_PUTS` to avoid repetition of the appended string to calculate
its length.
</pre>
</div>
</content>
</entry>
<entry>
<title>regexec: prefix all regexec function calls with p_</title>
<updated>2019-05-19T10:10:08+00:00</updated>
<author>
<name>Edward Thomson</name>
<email>ethomson@edwardthomson.com</email>
</author>
<published>2019-01-12T23:06:39+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=02683b20c0775dab28d72727cc97a96ec77cb20c'/>
<id>02683b20c0775dab28d72727cc97a96ec77cb20c</id>
<content type='text'>
Prefix all the calls to the the regexec family of functions with `p_`.
This allows us to swap out all the regular expression functions with our
own implementation.  Move the declarations to `posix_regex.h` for
simpler inclusion.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Prefix all the calls to the the regexec family of functions with `p_`.
This allows us to swap out all the regular expression functions with our
own implementation.  Move the declarations to `posix_regex.h` for
simpler inclusion.
</pre>
</div>
</content>
</entry>
<entry>
<title>strmap: introduce high-level setter for key/value pairs</title>
<updated>2019-02-15T12:16:48+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2019-01-23T09:44:33+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=03555830784a2856e0c9651d2643b3ee5ce2084d'/>
<id>03555830784a2856e0c9651d2643b3ee5ce2084d</id>
<content type='text'>
Currently, one would use the function `git_strmap_insert` to insert key/value
pairs into a map. This function has historically been a macro, which is why its
syntax is kind of weird: instead of returning an error code directly, it instead
has to be passed a pointer to where the return value shall be stored. This does
not match libgit2's common idiom of directly returning error codes.

Introduce a new function `git_strmap_set`, which takes as parameters the map,
key and value and directly returns an error code. Convert all callers of
`git_strmap_insert` to make use of it.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Currently, one would use the function `git_strmap_insert` to insert key/value
pairs into a map. This function has historically been a macro, which is why its
syntax is kind of weird: instead of returning an error code directly, it instead
has to be passed a pointer to where the return value shall be stored. This does
not match libgit2's common idiom of directly returning error codes.

Introduce a new function `git_strmap_set`, which takes as parameters the map,
key and value and directly returns an error code. Convert all callers of
`git_strmap_insert` to make use of it.
</pre>
</div>
</content>
</entry>
<entry>
<title>strmap: introduce `git_strmap_get` and use it throughout the tree</title>
<updated>2019-02-15T12:16:48+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2019-01-23T09:44:02+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=ef507bc7bdd736d2379a0d0614b3db1341d77187'/>
<id>ef507bc7bdd736d2379a0d0614b3db1341d77187</id>
<content type='text'>
The current way of looking up an entry from a map is tightly coupled with the
map implementation, as one first has to look up the index of the key and then
retrieve the associated value by using the index. As a caller, you usually do
not care about any indices at all, though, so this is more complicated than
really necessary. Furthermore, it invites for errors to happen if the correct
error checking sequence is not being followed.

Introduce a new high-level function `git_strmap_get` that takes a map and a key
and returns a pointer to the associated value if such a key exists. Otherwise,
a `NULL` pointer is returned. Adjust all callers that can trivially be
converted.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
The current way of looking up an entry from a map is tightly coupled with the
map implementation, as one first has to look up the index of the key and then
retrieve the associated value by using the index. As a caller, you usually do
not care about any indices at all, though, so this is more complicated than
really necessary. Furthermore, it invites for errors to happen if the correct
error checking sequence is not being followed.

Introduce a new high-level function `git_strmap_get` that takes a map and a key
and returns a pointer to the associated value if such a key exists. Otherwise,
a `NULL` pointer is returned. Adjust all callers that can trivially be
converted.
</pre>
</div>
</content>
</entry>
<entry>
<title>maps: use uniform lifecycle management functions</title>
<updated>2019-02-15T12:16:48+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2019-01-23T09:42:46+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=351eeff3b2a666b8ead5302c1130629597438df6'/>
<id>351eeff3b2a666b8ead5302c1130629597438df6</id>
<content type='text'>
Currently, the lifecycle functions for maps (allocation, deallocation, resize)
are not named in a uniform way and do not have a uniform function signature.
Rename the functions to fix that, and stick to libgit2's naming scheme of saying
`git_foo_new`. This results in the following new interface for allocation:

- `int git_&lt;t&gt;map_new(git_&lt;t&gt;map **out)` to allocate a new map, returning an
  error code if we ran out of memory

- `void git_&lt;t&gt;map_free(git_&lt;t&gt;map *map)` to free a map

- `void git_&lt;t&gt;map_clear(git&lt;t&gt;map *map)` to remove all entries from a map

This commit also fixes all existing callers.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Currently, the lifecycle functions for maps (allocation, deallocation, resize)
are not named in a uniform way and do not have a uniform function signature.
Rename the functions to fix that, and stick to libgit2's naming scheme of saying
`git_foo_new`. This results in the following new interface for allocation:

- `int git_&lt;t&gt;map_new(git_&lt;t&gt;map **out)` to allocate a new map, returning an
  error code if we ran out of memory

- `void git_&lt;t&gt;map_free(git_&lt;t&gt;map *map)` to free a map

- `void git_&lt;t&gt;map_clear(git&lt;t&gt;map *map)` to remove all entries from a map

This commit also fixes all existing callers.
</pre>
</div>
</content>
</entry>
<entry>
<title>git_error: use new names in internal APIs and usage</title>
<updated>2019-01-22T22:30:35+00:00</updated>
<author>
<name>Edward Thomson</name>
<email>ethomson@edwardthomson.com</email>
</author>
<published>2018-12-27T19:47:34+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=f673e232afe22eb865cdc915e55a2df6493f0fbb'/>
<id>f673e232afe22eb865cdc915e55a2df6493f0fbb</id>
<content type='text'>
Move to the `git_error` name in the internal API for error-related
functions.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Move to the `git_error` name in the internal API for error-related
functions.
</pre>
</div>
</content>
</entry>
<entry>
<title>khash: remove intricate knowledge of khash types</title>
<updated>2018-11-28T14:22:27+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2018-11-23T18:26:24+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=852bc9f4967d3bd70a284794ff486253e37c6980'/>
<id>852bc9f4967d3bd70a284794ff486253e37c6980</id>
<content type='text'>
Instead of using the `khiter_t`, `git_strmap_iter` and `khint_t` types,
simply use `size_t` instead. This decouples code from the khash stuff
and makes it possible to move the khash includes into the implementation
files.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Instead of using the `khiter_t`, `git_strmap_iter` and `khint_t` types,
simply use `size_t` instead. This decouples code from the khash stuff
and makes it possible to move the khash includes into the implementation
files.
</pre>
</div>
</content>
</entry>
<entry>
<title>Convert usage of `git_buf_free` to new `git_buf_dispose`</title>
<updated>2018-06-10T17:34:37+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2018-02-08T11:14:48+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=ecf4f33a4e327a91496f72816f9f02d923e5af05'/>
<id>ecf4f33a4e327a91496f72816f9f02d923e5af05</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>diff_generate: avoid excessive stats of .gitattribute files</title>
<updated>2018-01-03T16:28:24+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2018-01-03T16:07:36+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=d8896bda5c43616f3c755242703fce7c2a97ad67'/>
<id>d8896bda5c43616f3c755242703fce7c2a97ad67</id>
<content type='text'>
When generating a diff between two trees, for each file that is to be
diffed we have to determine whether it shall be treated as text or as
binary files. While git has heuristics to determine which kind of diff
to generate, users can also that default behaviour by setting or
unsetting the 'diff' attribute for specific files.

Because of that, we have to query gitattributes in order to determine
how to diff the current files. Instead of hitting the '.gitattributes'
file every time we need to query an attribute, which can get expensive
especially on networked file systems, we try to cache them instead. This
works perfectly fine for every '.gitattributes' file that is found, but
we hit cache invalidation problems when we determine that an attribuse
file is _not_ existing. We do create an entry in the cache for missing
'.gitattributes' files, but as soon as we hit that file again we
invalidate it and stat it again to see if it has now appeared.

In the case of diffing large trees with each other, this behaviour is
very suboptimal. For each pair of files that is to be diffed, we will
repeatedly query every directory component leading towards their
respective location for an attributes file. This leads to thousands or
even hundreds of thousands of wasted syscalls.

The attributes cache already has a mechanism to help in that scenario in
form of the `git_attr_session`. As long as the same attributes session
is still active, we will not try to re-query the gitmodules files at all
but simply retain our currently cached results. To fix our problem, we
can create a session at the top-most level, which is the initialization
of the `git_diff` structure, and use it in order to look up the correct
diff driver. As the `git_diff` structure is used to generate patches for
multiple files at once, this neatly solves our problem by retaining the
session until patches for all files have been generated.

The fix has been tested with linux.git by calling
`git_diff_tree_to_tree` and `git_diff_to_buf` with v4.10^{tree} and
v4.14^{tree}.

                | time    | .gitattributes stats
    without fix | 33.201s | 844614
    with fix    | 30.327s | 4441

While execution only improved by roughly 10%, the stat(3) syscalls for
.gitattributes files decreased by 99.5%. The benchmarks were quite
simple with best-of-three timings on Linux ext4 systems. One can assume
that for network based file systems the performance gain will be a lot
larger due to a much higher latency.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
When generating a diff between two trees, for each file that is to be
diffed we have to determine whether it shall be treated as text or as
binary files. While git has heuristics to determine which kind of diff
to generate, users can also that default behaviour by setting or
unsetting the 'diff' attribute for specific files.

Because of that, we have to query gitattributes in order to determine
how to diff the current files. Instead of hitting the '.gitattributes'
file every time we need to query an attribute, which can get expensive
especially on networked file systems, we try to cache them instead. This
works perfectly fine for every '.gitattributes' file that is found, but
we hit cache invalidation problems when we determine that an attribuse
file is _not_ existing. We do create an entry in the cache for missing
'.gitattributes' files, but as soon as we hit that file again we
invalidate it and stat it again to see if it has now appeared.

In the case of diffing large trees with each other, this behaviour is
very suboptimal. For each pair of files that is to be diffed, we will
repeatedly query every directory component leading towards their
respective location for an attributes file. This leads to thousands or
even hundreds of thousands of wasted syscalls.

The attributes cache already has a mechanism to help in that scenario in
form of the `git_attr_session`. As long as the same attributes session
is still active, we will not try to re-query the gitmodules files at all
but simply retain our currently cached results. To fix our problem, we
can create a session at the top-most level, which is the initialization
of the `git_diff` structure, and use it in order to look up the correct
diff driver. As the `git_diff` structure is used to generate patches for
multiple files at once, this neatly solves our problem by retaining the
session until patches for all files have been generated.

The fix has been tested with linux.git by calling
`git_diff_tree_to_tree` and `git_diff_to_buf` with v4.10^{tree} and
v4.14^{tree}.

                | time    | .gitattributes stats
    without fix | 33.201s | 844614
    with fix    | 30.327s | 4441

While execution only improved by roughly 10%, the stat(3) syscalls for
.gitattributes files decreased by 99.5%. The benchmarks were quite
simple with best-of-three timings on Linux ext4 systems. One can assume
that for network based file systems the performance gain will be a lot
larger due to a much higher latency.
</pre>
</div>
</content>
</entry>
<entry>
<title>Make sure to always include "common.h" first</title>
<updated>2017-07-03T08:51:48+00:00</updated>
<author>
<name>Patrick Steinhardt</name>
<email>ps@pks.im</email>
</author>
<published>2017-06-30T11:39:01+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/libgit2.git/commit/?id=0c7f49dd4316b332f30b4ea72a657bace41e1245'/>
<id>0c7f49dd4316b332f30b4ea72a657bace41e1245</id>
<content type='text'>
Next to including several files, our "common.h" header also declares
various macros which are then used throughout the project. As such, we
have to make sure to always include this file first in all
implementation files. Otherwise, we might encounter problems or even
silent behavioural differences due to macros or defines not being
defined as they should be. So in fact, our header and implementation
files should make sure to always include "common.h" first.

This commit does so by establishing a common include pattern. Header
files inside of "src" will now always include "common.h" as its first
other file, separated by a newline from all the other includes to make
it stand out as special. There are two cases for the implementation
files. If they do have a matching header file, they will always include
this one first, leading to "common.h" being transitively included as
first file. If they do not have a matching header file, they instead
include "common.h" as first file themselves.

This fixes the outlined problems and will become our standard practice
for header and source files inside of the "src/" from now on.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Next to including several files, our "common.h" header also declares
various macros which are then used throughout the project. As such, we
have to make sure to always include this file first in all
implementation files. Otherwise, we might encounter problems or even
silent behavioural differences due to macros or defines not being
defined as they should be. So in fact, our header and implementation
files should make sure to always include "common.h" first.

This commit does so by establishing a common include pattern. Header
files inside of "src" will now always include "common.h" as its first
other file, separated by a newline from all the other includes to make
it stand out as special. There are two cases for the implementation
files. If they do have a matching header file, they will always include
this one first, leading to "common.h" being transitively included as
first file. If they do not have a matching header file, they instead
include "common.h" as first file themselves.

This fixes the outlined problems and will become our standard practice
for header and source files inside of the "src/" from now on.
</pre>
</div>
</content>
</entry>
</feed>
