| Message ID | pull.539.git.1579808479.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Reftable support git-core | expand |
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: > This adds the reftable library, and hooks it up as a ref backend. Just a quick impression before getting into details of individual steps. * With this series, the reftable backend seems to take over as the default and only backend. We would need to design and decide how repositories would specify which backend it uses (I personally do not think we need to allow more than one backend to be active at the same time) before we take this series out of RFC status. * What's reftable/VERSION file? Does it really have what you intended to add? * Mixed indentation and many whitespace breakages make the code distracting to review. * Comparison with 0 is written as "if (!strcmp(a, b))" in this codebase, and never "if (0 != strcmp(a, b))". * We unfortunately do not use var defn "for (int i = 0; ..." yet. * We do not use // comments. Thanks. > Han-Wen Nienhuys (5): > setup.c: enable repo detection for reftable > create .git/refs in files-backend.c > Document how ref iterators and symrefs interact > Add reftable library > Reftable support for git-core > > Makefile | 23 +- > builtin/init-db.c | 2 - > refs.c | 18 +- > refs.h | 2 + > refs/files-backend.c | 4 + > refs/refs-internal.h | 4 + > refs/reftable-backend.c | 780 ++++++++++++++++++++++++++++ > reftable/README.md | 12 + > reftable/VERSION | 293 +++++++++++ > reftable/basics.c | 180 +++++++ > reftable/basics.h | 38 ++ > reftable/block.c | 382 ++++++++++++++ > reftable/block.h | 71 +++ > reftable/block_test.c | 145 ++++++ > reftable/blocksource.h | 20 + > reftable/bytes.c | 0 > reftable/constants.h | 27 + > reftable/dump.c | 102 ++++ > reftable/file.c | 98 ++++ > reftable/iter.c | 211 ++++++++ > reftable/iter.h | 56 ++ > reftable/merged.c | 262 ++++++++++ > reftable/merged.h | 34 ++ > reftable/merged_test.c | 247 +++++++++ > reftable/pq.c | 116 +++++ > reftable/pq.h | 34 ++ > reftable/reader.c | 672 ++++++++++++++++++++++++ > reftable/reader.h | 52 ++ > reftable/record.c | 1030 +++++++++++++++++++++++++++++++++++++ > reftable/record.h | 79 +++ > reftable/record_test.c | 313 +++++++++++ > reftable/reftable.h | 396 ++++++++++++++ > reftable/reftable_test.c | 434 ++++++++++++++++ > reftable/slice.c | 179 +++++++ > reftable/slice.h | 39 ++ > reftable/slice_test.c | 38 ++ > reftable/stack.c | 931 +++++++++++++++++++++++++++++++++ > reftable/stack.h | 40 ++ > reftable/stack_test.c | 265 ++++++++++ > reftable/test_framework.c | 60 +++ > reftable/test_framework.h | 63 +++ > reftable/tree.c | 60 +++ > reftable/tree.h | 24 + > reftable/tree_test.c | 54 ++ > reftable/writer.c | 586 +++++++++++++++++++++ > reftable/writer.h | 46 ++ > setup.c | 20 +- > 47 files changed, 8530 insertions(+), 12 deletions(-) > create mode 100644 refs/reftable-backend.c > create mode 100644 reftable/README.md > create mode 100644 reftable/VERSION > create mode 100644 reftable/basics.c > create mode 100644 reftable/basics.h > create mode 100644 reftable/block.c > create mode 100644 reftable/block.h > create mode 100644 reftable/block_test.c > create mode 100644 reftable/blocksource.h > create mode 100644 reftable/bytes.c > create mode 100644 reftable/constants.h > create mode 100644 reftable/dump.c > create mode 100644 reftable/file.c > create mode 100644 reftable/iter.c > create mode 100644 reftable/iter.h > create mode 100644 reftable/merged.c > create mode 100644 reftable/merged.h > create mode 100644 reftable/merged_test.c > create mode 100644 reftable/pq.c > create mode 100644 reftable/pq.h > create mode 100644 reftable/reader.c > create mode 100644 reftable/reader.h > create mode 100644 reftable/record.c > create mode 100644 reftable/record.h > create mode 100644 reftable/record_test.c > create mode 100644 reftable/reftable.h > create mode 100644 reftable/reftable_test.c > create mode 100644 reftable/slice.c > create mode 100644 reftable/slice.h > create mode 100644 reftable/slice_test.c > create mode 100644 reftable/stack.c > create mode 100644 reftable/stack.h > create mode 100644 reftable/stack_test.c > create mode 100644 reftable/test_framework.c > create mode 100644 reftable/test_framework.h > create mode 100644 reftable/tree.c > create mode 100644 reftable/tree.h > create mode 100644 reftable/tree_test.c > create mode 100644 reftable/writer.c > create mode 100644 reftable/writer.h > > > base-commit: 232378479ee6c66206d47a9be175e3a39682aea6 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-539%2Fhanwen%2Freftable-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-539/hanwen/reftable-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/539
Hi Han-Wen, On 1/23/20 8:41 PM, Han-Wen Nienhuys via GitGitGadget wrote: > This adds the reftable library, and hooks it up as a ref backend. > > Han-Wen Nienhuys (5): > setup.c: enable repo detection for reftable > create .git/refs in files-backend.c > Document how ref iterators and symrefs interact > Add reftable library > Reftable support for git-core I am most of the time just a curious reader on this list but as someone who has no idea what the reftable library does (except that it can serve as a "ref backend"), I would expect much more elaborate commit messages. In particular, patch 4/5 (i.e. the commit message of the "add reftable library" commit) should describe the purpose of the reftable library at least briefly; and patch 5/5 should not contain shell commands and output without context but a short description why the patch is doing what it is doing. For example, if the use of the reftable library makes certain git commands a lot faster, it should state that. Thanks. Stephan
On Thu, Jan 23, 2020 at 10:44 PM Junio C Hamano <gitster@pobox.com> wrote: > > This adds the reftable library, and hooks it up as a ref backend. > > Just a quick impression before getting into details of individual > steps. > > * With this series, the reftable backend seems to take over as the > default and only backend. We would need to design and decide how > repositories would specify which backend it uses (I personally do > not think we need to allow more than one backend to be active at > the same time) before we take this series out of RFC status. Yes, obviously. Would you have concrete ideas on how this should work? > * What's reftable/VERSION file? Does it really have what you > intended to add? Fixed, and explained in reftable/VERSION. > * Mixed indentation and many whitespace breakages make the code > distracting to review. Do you mean in the changes to refs/reftable-backend.c? Or the imported code? The imported code is uniformly formatted with clang-format. Is there a clang-format setting for uniformly formatting to Linux/Git style? I really love automated formatting, so we don't have to spend time debating irrelevant details. > > * Comparison with 0 is written as "if (!strcmp(a, b))" in this > codebase, and never "if (0 != strcmp(a, b))". > > * We unfortunately do not use var defn "for (int i = 0; ..." yet. > > * We do not use // comments. > fixed.
I added some more background in one of the commit messages. You can find extensive discussion here: https://github.com/google/reftable#background-reading On Thu, Jan 23, 2020 at 11:45 PM Stephan Beyer <s-beyer@gmx.net> wrote: > > Hi Han-Wen, > > On 1/23/20 8:41 PM, Han-Wen Nienhuys via GitGitGadget wrote: > > This adds the reftable library, and hooks it up as a ref backend. > > > > Han-Wen Nienhuys (5): > > setup.c: enable repo detection for reftable > > create .git/refs in files-backend.c > > Document how ref iterators and symrefs interact > > Add reftable library > > Reftable support for git-core > > I am most of the time just a curious reader on this list but as someone > who has no idea what the reftable library does (except that it can serve > as a "ref backend"), I would expect much more elaborate commit messages. > In particular, patch 4/5 (i.e. the commit message of the "add reftable > library" commit) should describe the purpose of the reftable library at > least briefly; and patch 5/5 should not contain shell commands and > output without context but a short description why the patch is doing > what it is doing. For example, if the use of the reftable library makes > certain git commands a lot faster, it should state that. > > Thanks. > > Stephan
On Mon, Jan 27, 2020 at 2:52 PM Han-Wen Nienhuys <hanwen@xs4all.nl> wrote: > > * Mixed indentation and many whitespace breakages make the code > > distracting to review. > > Do you mean in the changes to refs/reftable-backend.c? Or the imported code? > > The imported code is uniformly formatted with clang-format. > > Is there a clang-format setting for uniformly formatting to Linux/Git > style? I really love automated formatting, so we don't have to spend > time debating irrelevant details. Ah, there is a .clang-format entry in the git repository already. Thanks!