Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimal patch series to fix the CI runs of hn/reftable #623

Closed
wants to merge 18 commits into from

Conversation

dscho
Copy link
Member

@dscho dscho commented May 4, 2020

These patches are not intended to be complete, not by any stretch of imagination. They are just enough to get the CI run to pass, even in the Windows-specific parts.

As I mentioned elsewhere, I would much prefer for hn/reftable to not re-invent get_be*(), struct strbuf, struct string_list, struct lock_file etc.

However, in the context of the test failures, I do not think that this would have made a big difference. Apart from the unportable constructs, and from the "delete/rename while there is still a handle on the file" issues, it would appear that one big reason why it was so hard to debug and fix the test is the recursive complexity of the data structures.

To elaborate on that: struct reftable_stack has an attribute called merged that has an array of struct reftable_reader * (confusingly called "stack"). Each of these readers has an attribute of type struct reftable_block_source that uses a vtable and an opaque pointer to abstract three types of block sources: file, slice (which is apparently unused, i.e. it is apparently just dead weight for now) and malloc.

I am not sure that this abstraction fest serves us well here.

Quite honestly, I would have expected the packed_git data structure to be imitated more closely, as it strikes me as similar in purpose, and it has seen a ton of testing over the past decade and a half. I could not recognize that design in the reftable, though.

It is quite obvious, of course, that the code tries to imitate the object-oriented nature of the Go code, but it seems quite obvious from my difficulties to address the problem where stack_compact_range() would try to delete stale reftable files (without even so much as a warning when the files could not be deleted!) without releasing all file handles to all reftable files, even the ones that do not need to be deleted. To be smarter about this, the code has to know more about the nature of the block source than the abstraction layer suggests. It has to know that a block source refers to a file, and that that file is marked for deletion. My heavy-handed work-around, even if it works, is not really a good solution, but it is a testament that there is a lot of opportunity to simplify the code drastically while at the same time simplifying the design, too.

I know you have been putting a lot of effort into this library, so I feel a bit bad about saying the following: The hn/reftable patches will need substantial work before we can merge it with confidence. Part of the reason why it is so hard to review the reftable patches is that they intentionally refuse to integrate well within Git's source code, such as (re-)implementing its own fundamental data structures, intentionally using a totally different coding style, and it concerns me that the stated requirement for bug fixes is to treat Git's source code as a downstream of the actual project. I am not too bad a C developer and would consider myself competent in debugging issues, even hard ones, in Git, and yet... it was really hard to wade through the layers of abstraction to figure out where the file handles should be closed that were opened and prevented deleting/renaming files.

At this point, I don't feel that it makes sense to keep insisting on having this in a separate library. The only other user of that library will be libgit2, and I really think that given libgit2's heritage, it won't be a problem to adapt the code after it stabilized in git.git (and since libgit2 treats git.git as upstream for the libxdiff code, it won't be a problem to do the same for the reftable code, too). I believe that the best course of action is to reuse the data structures libgit.a provides, and to delete the re-implementations in reftable/. Only then can we start working effectively on refactoring the code to simplify the data structures in order to clarify resource usage (which was the root cause for the bugs I fixed, although I am sure that there are way more lurking in there, hidden by the fact that the code is not covered thoroughly by our tests).

Cc: Han-Wen Nienhuys hanwen@google.com

hanwen and others added 18 commits April 27, 2020 14:43
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This happens implicitly in the files/packed ref backend; making it
explicit simplifies adding alternate ref storage backends, such as
reftable.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This prepares for supporting the reftable format, which will want
create its own file system layout in .git

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Shawn Pearce explains:

Some repositories contain a lot of references (e.g. android at 866k,
rails at 31k). The reftable format provides:

- Near constant time lookup for any single reference, even when the
  repository is cold and not in process or kernel cache.
- Near constant time verification a SHA-1 is referred to by at least
  one reference (for allow-tip-sha1-in-want).
- Efficient lookup of an entire namespace, such as `refs/tags/`.
- Support atomic push `O(size_of_update)` operations.
- Combine reflog storage with ref storage.

This file format spec was originally written in July, 2017 by Shawn
Pearce.  Some refinements since then were made by Shawn and by Han-Wen
Nienhuys based on experiences implementing and experimenting with the
format.  (All of this was in the context of our work at Google and
Google is happy to contribute the result to the Git project.)

Imported from JGit[1]'s current version (c217d33ff,
"Documentation/technical/reftable: improve repo layout", 2020-02-04)
of Documentation/technical/reftable.md and converted to asciidoc by
running

  pandoc -t asciidoc -f markdown reftable.md >reftable.txt

using pandoc 2.2.1.  The result required the following additional
minor changes:

- removed the [TOC] directive to add a table of contents, since
  asciidoc does not support it
- replaced git-scm.com/docs links with linkgit: directives that link
  to other pages within Git's documentation

[1] https://eclipse.googlesource.com/jgit/jgit

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The format allows for some ambiguity, as a lone footer also starts
with a valid file header. However, the current JGit code will barf on
this. This commit codifies this behavior into the standard.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Reftable is a format for storing the ref database. Its rationale and
specification is in the preceding commit.

This imports the upstream library as one big commit. For understanding
the code, it is suggested to read the code in the following order:

* The specification under Documentation/technical/reftable.txt

* reftable.h - the public API

* record.{c,h} - reading and writing records

* block.{c,h} - reading and writing blocks.

* writer.{c,h} - writing a complete reftable file.

* merged.{c,h} and pq.{c,h} - reading a stack of reftables

* stack.{c,h} - writing and compacting stacks of reftable on the
filesystem.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
For background, see the previous commit introducing the library.

TODO:

 * Resolve spots marked with XXX

 * Detect and prevent directory/file conflicts in naming.

 * Support worktrees (t0002-gitfile "linked repo" testcase)

Example use: see t/t0031-reftable.sh

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
* Add GIT_TEST_REFTABLE environment var to control default ref storage

* Add test_prerequisite REFTABLE. Skip t/t3210-pack-refs.sh for REFTABLE.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Reading and writing .git/refs/* assumes that refs are stored in the 'files'
ref backend.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Close files before trying to unlink them. This is actually required on
Windows.

Note: this is probably not the only part of the code that requires this
type of fix, but it seems to be enough to let the _current_ version of
t0031 pass.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Do close the file descriptors to obsolete files before trying to delete
them. This is actually required on Windows.

Note: this patch is probably a bit heavy-handed, releasing more than
necessary (which will then have to be re-read). But it seems to be
enough to let t0031 pass on Windows, so it is at least a clue as to what
the fix will look like eventually.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
A `void` function cannot return a value.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
`usleep()` is unportable. We need to find a way _not_ to use it.

For Visual C, we can use `sleep_millisec()`, but the current design of
libreftable seems to be _really_ keen _not_ to depend on anything in
libgit.a.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Yet another instance of `= {}` initialization.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This allows Git to be compiled via Visual Studio again after integrating
the `hn/reftable` branch.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho self-assigned this May 4, 2020
@dscho
Copy link
Member Author

dscho commented May 4, 2020

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented May 4, 2020

@gitgitgadget
Copy link

gitgitgadget bot commented May 4, 2020

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> These patches are not intended to be complete, not by any stretch of
> imagination. They are just enough to get the CI run to pass, even in the
> Windows-specific parts.

Thanks.  Now I know at least somebody has been paying attention to
call for help in the past few issues of the "What's cooking" report ;-) 

@gitgitgadget
Copy link

gitgitgadget bot commented May 4, 2020

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

On Mon, May 4, 2020 at 3:31 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> These patches are not intended to be complete, not by any stretch of
> imagination. They are just enough to get the CI run to pass, even in the
> Windows-specific parts.

Thanks for the investigation. I had begun to setup a Windows
installation, but you beat me to it.

Here are the two fixes as I applied them.

 https://github.com/google/reftable/commit/201f21aa39f993c970ef4d19122fedd5b91e716d
 https://github.com/google/reftable/commit/18b041472306b31813dbd6408f6a61daaba60013

> As I mentioned elsewhere, I would much prefer for hn/reftable to not
> re-invent get_be*(), struct strbuf, struct string_list, struct lock_file
> etc.
>
> However, in the context of the test failures, I do not think that this would
> have made a big difference. Apart from the unportable constructs, and from
> the "delete/rename while there is still a handle on the file" issues, it
> would appear that one big reason why it was so hard to debug and fix the
> test is the recursive complexity of the data structures.
>
> To elaborate on that: struct reftable_stack has an attribute called merged
> that has an array of struct reftable_reader * (confusingly called "stack").
> Each of these readers has an attribute of type struct reftable_block_source
> that uses a vtable and an opaque pointer to abstract three types of block
> sources: file, slice (which is apparently unused, i.e. it is apparently just
> dead weight for now) and malloc.

All of the unittests use slices to read and write single reftables in-memory.

The malloc block source is used to swap out a block coming from the
file directly, with zlib  uncompressed data (log blocks are zlib
compressed). I think there should be a mmap block source too, at some
point.

> I am not sure that this abstraction fest serves us well here.
>
> Quite honestly, I would have expected the packed_git data structure to be
> imitated more closely, as it strikes me as similar in purpose, and it has
> seen a ton of testing over the past decade and a half. I could not recognize
> that design in the reftable, though.
>
> It is quite obvious, of course, that the code tries to imitate the
> object-oriented nature of the Go code, but it seems quite obvious from my
> difficulties to address the problem where stack_compact_range() would try to
> delete stale reftable files (without even so much as a warning when the
> files could not be deleted!) without releasing all file handles to all
> reftable files, even the ones that do not need to be deleted. To be smarter
> about this, the code has to know more about the nature of the block source
> than the abstraction layer suggests. It has to know that a block source
> refers to a file, and that that file is marked for deletion. My heavy-handed
> work-around, even if it works, is not really a good solution, but it is a
> testament that there is a lot of opportunity to simplify the code
> drastically while at the same time simplifying the design, too.

If the code tries to delete a file while it is open, that is a
testament to the fact that I haven't written code for Windows for many
years now. It is not a testament to any fundamental problems with the
library design.

The library which you are complaining of weighs in at about 7500 lines
of code (excluding tests), which compares pretty well to the original
JGit code which is now ~6500 lines of code. I don't think there is
room to simplify it further, and I say this as a person who has
significant experience with this format.

If you wish to prove me wrong, you can send me patch. Until then, I
don't buy your arguments.

> I know you have been putting a lot of effort into this library, so I feel a
> bit bad about saying the following: The hn/reftable patches will need
> substantial work before we can merge it with confidence. Part of the reason
> why it is so hard to review the reftable patches is that they intentionally
> refuse to integrate well within Git's source code, such as (re-)implementing
> its own fundamental data structures, intentionally using a totally different
> coding style, and it concerns me that the stated requirement for bug fixes
> is to treat Git's source code as a downstream of the actual project. I am
> not too bad a C developer and would consider myself competent in debugging
> issues, even hard ones, in Git, and yet... it was really hard to wade
> through the layers of abstraction to figure out where the file handles
> should be closed that were opened and prevented deleting/renaming files.
>
> At this point, I don't feel that it makes sense to keep insisting on having
> this in a separate library. The only other user of that library will be
> libgit2, and I really think that given libgit2's heritage, it won't be a
> problem to adapt the code after it stabilized in git.git (and since libgit2
> treats git.git as upstream for the libxdiff code, it won't be a problem to
> do the same for the reftable code, too). I believe that the best course of
> action is to reuse the data structures libgit.a provides, and to delete the
> re-implementations in reftable/. Only then can we start working effectively
> on refactoring the code to simplify the data structures in order to clarify
> resource usage (which was the root cause for the bugs I fixed, although I am
> sure that there are way more lurking in there, hidden by the fact that the
> code is not covered thoroughly by our tests).

I'm happy to change the library to use more common primitives that are
shared between libgit2 and git. Could you point them out to me? Note
that the basics that you are complaining of (put_be*, strbuf vs slice
routines, etc.) constitute around 700 lines of code. It's not going to
make an appreciable difference in complexity.

Here is my counterpoint to your proposal:

Reftable was developed outside of git-core. If you feel this series
with its giant patch is too much to review, you can have a look at
the incremental story of how it came to be, both in the history and
code reviews of the JGit project, and in the commit history of
https://github.com/google/reftable.

The idea to put the code into git-core, and especially your proposed
(but unsubstantiated) plans to "simplify" its design, make me very
worried about interoperability with the JGit reference implementation.

Could you explain to me how you would qualify a reftable-in-git
library against the JGit implementation?

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

@@ -700,7 +700,7 @@ vcxproj:
# Make .vcxproj files and add them
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> This allows Git to be compiled via Visual Studio again after integrating
> the `hn/reftable` branch.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  config.mak.uname                           |  2 +-
>  contrib/buildsystems/Generators/Vcxproj.pm | 11 ++++++++++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index 0ab8e009383..8a01a0da3f1 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -700,7 +700,7 @@ vcxproj:
>  	# Make .vcxproj files and add them
>  	unset QUIET_GEN QUIET_BUILT_IN; \
>  	perl contrib/buildsystems/generate -g Vcxproj
> -	git add -f git.sln {*,*/lib,t/helper/*}/*.vcxproj
> +	git add -f git.sln {*,*/lib,*/libreftable,t/helper/*}/*.vcxproj
>  
>  	# Generate the LinkOrCopyBuiltins.targets and LinkOrCopyRemoteHttp.targets file
>  	(echo '<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">' && \
> diff --git a/contrib/buildsystems/Generators/Vcxproj.pm b/contrib/buildsystems/Generators/Vcxproj.pm
> index 5c666f9ac03..33a08d31652 100644
> --- a/contrib/buildsystems/Generators/Vcxproj.pm
> +++ b/contrib/buildsystems/Generators/Vcxproj.pm
> @@ -77,7 +77,7 @@ sub createProject {
>      my $libs_release = "\n    ";
>      my $libs_debug = "\n    ";
>      if (!$static_library) {
> -      $libs_release = join(";", sort(grep /^(?!libgit\.lib|xdiff\/lib\.lib|vcs-svn\/lib\.lib)/, @{$$build_structure{"$prefix${name}_LIBS"}}));
> +      $libs_release = join(";", sort(grep /^(?!libgit\.lib|xdiff\/lib\.lib|vcs-svn\/lib\.lib|reftable\/libreftable\.lib)/, @{$$build_structure{"$prefix${name}_LIBS"}}));
>        $libs_debug = $libs_release;
>        $libs_debug =~ s/zlib\.lib/zlibd\.lib/g;
>        $libs_debug =~ s/libcurl\.lib/libcurl-d\.lib/g;
> @@ -231,6 +231,7 @@ sub createProject {
>  EOM
>      if (!$static_library || $target =~ 'vcs-svn' || $target =~ 'xdiff') {
>        my $uuid_libgit = $$build_structure{"LIBS_libgit_GUID"};
> +      my $uuid_libreftable = $$build_structure{"LIBS_reftable/libreftable_GUID"};
>        my $uuid_xdiff_lib = $$build_structure{"LIBS_xdiff/lib_GUID"};
>  
>        print F << "EOM";
> @@ -240,6 +241,14 @@ sub createProject {
>        <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
>      </ProjectReference>
>  EOM
> +      if (!($name =~ /xdiff|libreftable/)) {

Wow.  That is messy.  I find it somewhat funny that this is not just
libreftable, i.e. "don't include/refer to libreftable in itself",
but as long as it works, I won't complain ;-)

Thanks for helping the tip of 'pu' moving forward.

> +        print F << "EOM";
> +    <ProjectReference Include="$cdup\\reftable\\libreftable\\libreftable.vcxproj">
> +      <Project>$uuid_libreftable</Project>
> +      <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
> +    </ProjectReference>
> +EOM
> +      }
>        if (!($name =~ 'xdiff')) {
>          print F << "EOM";
>      <ProjectReference Include="$cdup\\xdiff\\lib\\xdiff_lib.vcxproj">

@gitgitgadget
Copy link

gitgitgadget bot commented May 4, 2020

This patch series was integrated into pu via git@7a06b4b.

@gitgitgadget gitgitgadget bot added the pu label May 4, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented May 5, 2020

This patch series was integrated into pu via git@82b6f41.

@gitgitgadget
Copy link

gitgitgadget bot commented May 5, 2020

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Han-Wen,

On Mon, 4 May 2020, Han-Wen Nienhuys wrote:

> On Mon, May 4, 2020 at 3:31 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > These patches are not intended to be complete, not by any stretch of
> > imagination. They are just enough to get the CI run to pass, even in the
> > Windows-specific parts.
>
> Thanks for the investigation. I had begun to setup a Windows
> installation, but you beat me to it.
>
> Here are the two fixes as I applied them.
>
>  https://github.com/google/reftable/commit/201f21aa39f993c970ef4d19122fedd5b91e716d
>  https://github.com/google/reftable/commit/18b041472306b31813dbd6408f6a61daaba60013

If you ever needed _any_ kind of proof that the current setup with that
monster patch and a totally separate "upstream" does simply _not_ work, it
is these two commits.

_Every_ bug fix originating from git.git would have to manually port the
fixes because of the vendor copy, just like you did, losing all provenance
apart from a meager "Thanks to" (which mentions just the bug report
forgetting about the actually-tested patch).

But I don't get the impression that my objections are heard, so I might
just as well stop talking about it. Please do not misinterpret my silence
about your patches as any indication that I am happy with `hn/reftable`: I
am not. Should Junio accept them without drastic changes (which I hope he
will not), I expect this to turn into a complete maintenance nightmare.

Besides, it is a bit disappointing to see only those two patches. I made
very clear in my patch series that I got the _distinct_ impression that
those two bug fixes were just the tip of the iceberg: there are most
likely a lot more bugs waiting to be discovered where a file is not closed
when it is no longer needed (or related: memory not being released as soon
as it can be released). I expect _many_ more similar issues to lurk in the
code base.

> > As I mentioned elsewhere, I would much prefer for hn/reftable to not
> > re-invent get_be*(), struct strbuf, struct string_list, struct lock_file
> > etc.
> >
> > However, in the context of the test failures, I do not think that this would
> > have made a big difference. Apart from the unportable constructs, and from
> > the "delete/rename while there is still a handle on the file" issues, it
> > would appear that one big reason why it was so hard to debug and fix the
> > test is the recursive complexity of the data structures.
> >
> > To elaborate on that: struct reftable_stack has an attribute called merged
> > that has an array of struct reftable_reader * (confusingly called "stack").
> > Each of these readers has an attribute of type struct reftable_block_source
> > that uses a vtable and an opaque pointer to abstract three types of block
> > sources: file, slice (which is apparently unused, i.e. it is apparently just
> > dead weight for now) and malloc.
>
> All of the unittests use slices to read and write single reftables
> in-memory.
>
>
> The malloc block source is used to swap out a block coming from the
> file directly, with zlib  uncompressed data (log blocks are zlib
> compressed). I think there should be a mmap block source too, at some
> point.

If that was intended to convince me that I am wrong in my assessment of
the unnecessary complexity of libreftable, I am afraid it did not.

Only code should be included in the patch adding libreftable to git.git
which is actually needed in git.git. Everything above that is just
unnecessary complexity.

> > I am not sure that this abstraction fest serves us well here.
> >
> > Quite honestly, I would have expected the packed_git data structure to be
> > imitated more closely, as it strikes me as similar in purpose, and it has
> > seen a ton of testing over the past decade and a half. I could not recognize
> > that design in the reftable, though.
> >
> > It is quite obvious, of course, that the code tries to imitate the
> > object-oriented nature of the Go code, but it seems quite obvious from my
> > difficulties to address the problem where stack_compact_range() would try to
> > delete stale reftable files (without even so much as a warning when the
> > files could not be deleted!) without releasing all file handles to all
> > reftable files, even the ones that do not need to be deleted. To be smarter
> > about this, the code has to know more about the nature of the block source
> > than the abstraction layer suggests. It has to know that a block source
> > refers to a file, and that that file is marked for deletion. My heavy-handed
> > work-around, even if it works, is not really a good solution, but it is a
> > testament that there is a lot of opportunity to simplify the code
> > drastically while at the same time simplifying the design, too.
>
> If the code tries to delete a file while it is open, that is a
> testament to the fact that I haven't written code for Windows for many
> years now. It is not a testament to any fundamental problems with the
> library design.

To the contrary. The library design neglects taking good care of
resources (such as closing file descriptors when they are clearly no
longer needed). C code _has_ to be careful with resources.

By the way, I am not really trying to convince you of this. That would be
the wrong direction: you have to convince _me_, the reviewer, that your
patches do the right thing, in the right way. So far, I am unconvinced.

Even worse: my debugging session two days ago convinced me that there _is_
unnecessary complexity, and unclear resource management. Based on how much
experience I have with debugging and developing in unfamiliar code bases,
this means quite a bit. I am not a junior intern who gets easily
overwhelmed by code they haven't seen before.

When I point out the unnecessary complexity of having a vtable for a mere
two "subclasses", of course, as an open source contributor you are free to
dismiss this without even so much as trying to refute it. Just like the
Git project is free to reject contributions when the reviewer comments are
not taken seriously.

> The library which you are complaining of weighs in at about 7500 lines
> of code (excluding tests), which compares pretty well to the original
> JGit code which is now ~6500 lines of code. I don't think there is
> room to simplify it further, and I say this as a person who has
> significant experience with this format.
>
> If you wish to prove me wrong, you can send me patch. Until then, I
> don't buy your arguments.

Again, it is not so much a question whether _you_ buy _my_ arguments (and
at the same time, it is somewhat shocking that you dismiss that it took me
two hours to figure out a single bug, just because the code and the data
structures are unnecessarily complex/abstract).

It is more a question whether you can convince the reviewers that your
code is correct.

In short: I am not selling anything here that you need to buy. You are
trying to sell me something, and I do not like the shape, and I like even
less how easily you seem to brush aside my explanations how this would need
to be improved to make it viable in Git's source code.

> > I know you have been putting a lot of effort into this library, so I feel a
> > bit bad about saying the following: The hn/reftable patches will need
> > substantial work before we can merge it with confidence. Part of the reason
> > why it is so hard to review the reftable patches is that they intentionally
> > refuse to integrate well within Git's source code, such as (re-)implementing
> > its own fundamental data structures, intentionally using a totally different
> > coding style, and it concerns me that the stated requirement for bug fixes
> > is to treat Git's source code as a downstream of the actual project. I am
> > not too bad a C developer and would consider myself competent in debugging
> > issues, even hard ones, in Git, and yet... it was really hard to wade
> > through the layers of abstraction to figure out where the file handles
> > should be closed that were opened and prevented deleting/renaming files.
> >
> > At this point, I don't feel that it makes sense to keep insisting on having
> > this in a separate library. The only other user of that library will be
> > libgit2, and I really think that given libgit2's heritage, it won't be a
> > problem to adapt the code after it stabilized in git.git (and since libgit2
> > treats git.git as upstream for the libxdiff code, it won't be a problem to
> > do the same for the reftable code, too). I believe that the best course of
> > action is to reuse the data structures libgit.a provides, and to delete the
> > re-implementations in reftable/. Only then can we start working effectively
> > on refactoring the code to simplify the data structures in order to clarify
> > resource usage (which was the root cause for the bugs I fixed, although I am
> > sure that there are way more lurking in there, hidden by the fact that the
> > code is not covered thoroughly by our tests).
>
> I'm happy to change the library to use more common primitives that are
> shared between libgit2 and git.

I never talked about primitives that are shared between libgit2. I talked
about data structures and helpers that already exist in Git's source code.

And I said that I am fairly convinced that it won't be hard to adapt the
code to libgit2 after it enters git.git.

I am sorry if I did not make myself clear.

It is a bit unreasonable to reject a review on the basis that git.git
should accept cruft that _might_ be required to support libgit2 better. It
would be very unreasonable to accept a patch series that demands to
include code in git.git that it does not need just because a separate
project might need that code.

And if you truly care about libgit2 support, you will include the libgit2
developers in the development of the reftable support, not throw a
nominally finalized library at them that might, or might not integrate
well into libgit2.

> Could you point them out to me? Note that the basics that you are
> complaining of (put_be*, strbuf vs slice routines, etc.) constitute
> around 700 lines of code. It's not going to make an appreciable
> difference in complexity.

Was I unclear? This additional code _makes it harder to review the monster
patch_.

The complexity _will_ need to be reduced. In addition.

But that cannot be the first step: in order to integrate reasonably well
with Git's source code, it will _have_ to stop duplicating code. You
cannot possibly deny that you reimplemented your own string manipulation
library in libreftable. Such code adds to the maintenance burden, and in
this instance (and in other, similar instances) for no good reason at all
because libgit.a already has code for that.

Without integrating the code better in git.git, there is no sense to even
start reducing the complexity. Both will be necessary.

> Here is my counterpoint to your proposal:
>
> Reftable was developed outside of git-core.

That was your decision, not mine. I merely pointed out that it would have
been wiser to develop it within.

And I offered that it is undesirable to take `hn/reftable` as long as it
looks very much like it _wants_ to stay out of core Git. I mean, what's
the point of accepting code into our code base that does not really want
to live there?

> If you feel this series with its giant patch is too much to review, you
> can have a look at the incremental story of how it came to be, both in
> the history and code reviews of the JGit project, and in the commit
> history of
> https://github.com/google/reftable.

So now you want to place _even more_ of a burden on reviewers? Seriously?

> The idea to put the code into git-core, and especially your proposed
> (but unsubstantiated) plans to "simplify" its design, make me very
> worried about interoperability with the JGit reference implementation.

The JGit reference implementation is written in Java. In an
object-oriented language with its own idiosyncracies demanded by the
design of said language (the lengths to which Shawn had to go to make it
performant are recorded in the commit history).

It will be better to have a clean-room implementation in C that does not
suffer from transpiling artifacts.

About ways to simplify its design: I mentioned the layers of abstraction.
I mentioned the design of `struct packed_git`, which could (and should)
serve as an example to follow.

If those points weren't ignored, I would mention other suggestions, such
as how the code builds file contents via `slice_append_string()` only to
write them out in one go, when the contents could have been written much
more simply via `fprintf()` instead. And that is just _one_ more example
how complexity could be reduced, very, very easily. There's much more.
Would there be a point of me writing these up? So far, I don't get that
impression. All I get is "I don't buy this". That is unnecessarily
frustrating: code review is already thankless (as much as it is
necessary!) as it is.

> Could you explain to me how you would qualify a reftable-in-git
> library against the JGit implementation?

Of course: by a comprehensive set of unit/regression tests, including
pre-generated data that the implementation must read and parse correctly.

The fact that keeping the code close to its non-C origins resulted in
long-undetected segmentation faults makes me rather certain the idea of
keeping the code close to said reference implementation is overrated: it
does not build the confidence in the code that you think it does. Not
after discovering those segmentation faults, it doesn't.

At the end of the day, a comprehensive set of unit/regression tests will
be required for implementing reftable support in other projects, anyway,
in particular for projects that cannot, or do not want to, rely on your
libreftable implementation, such as isomorphic-git or Dulwich.

Ciao,
Johannes

@gitgitgadget
Copy link

gitgitgadget bot commented May 5, 2020

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

On Mon, 4 May 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > These patches are not intended to be complete, not by any stretch of
> > imagination. They are just enough to get the CI run to pass, even in
> > the Windows-specific parts.
>
> Thanks.

You're welcome!

> Now I know at least somebody has been paying attention to call for help
> in the past few issues of the "What's cooking" report ;-)

Yes, I saw those requests for assistance, and I would have gladly jumped
in earlier, if my time had allowed.

Let's see whether my efforts bear fruit...

Ciao,
Dscho

@gitgitgadget
Copy link

gitgitgadget bot commented May 5, 2020

This patch series was integrated into pu via git@3163555.

@gitgitgadget
Copy link

gitgitgadget bot commented May 6, 2020

This patch series was integrated into pu via git@1699668.

@gitgitgadget
Copy link

gitgitgadget bot commented May 7, 2020

This patch series was integrated into pu via git@06f931f.

@gitgitgadget
Copy link

gitgitgadget bot commented May 7, 2020

This patch series was integrated into pu via git@eda5e82.

@gitgitgadget
Copy link

gitgitgadget bot commented May 8, 2020

This patch series was integrated into pu via git@9f480bd.

@gitgitgadget
Copy link

gitgitgadget bot commented May 8, 2020

This patch series was integrated into pu via git@cb6edfd.

@gitgitgadget
Copy link

gitgitgadget bot commented May 9, 2020

This patch series was integrated into pu via git@a774050.

@dscho
Copy link
Member Author

dscho commented May 10, 2020

I'll close this as some of it was accepted, and I do not plan on working on this anymore.

@dscho dscho closed this May 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants