Re: [PATCH v3 0/7] use oidset for skiplist + docs + tests + comment support

On Mon, Aug 27 2018, Ævar Arnfjörð Bjarmason wrote:

On Mon, Aug 27 2018, René Scharfe wrote:

quoted

Am 27.08.2018 um 09:37 schrieb Ævar Arnfjörð Bjarmason:

quoted

On Sat, Aug 25 2018, René Scharfe wrote:
[...]
Now, I like yours much better. I'm just saying that currently the
patch/commit message combo is confusing about *what* it's
doing. I.e. let's not mix up the correction of docs that were always
wrong with a non-change in the user facing implementation, and some
internal optimization all in one patch.

Now you have me confused.  Unsorted lists were always accepted, but the
documentation asked for a sorted one anyway, probably to avoid sorting
it with every use.  Switching the underlying data structure makes that a
moot point -- sortedness is no longer a concern at all -- not in the
code, and not for users.

Inviting users to run the array implementation with unsorted lists is
not incorrect, but it also doesn't help anyone.  We could clarify that
sorted lists are preferred or recommended instead of required.  I don't
see value in perfecting the documentation of a quirk just before
removing it, though.

I think it's easier to clarify step-by-step with code, so here's an my
version of a v3 which implements what I was suggesting, but then of
course while doing it I found other stuff to fix, changes since your
v2:

Sorry for breaking threading, forgot In-Reply-To, which should be
https://public-inbox.org/git/c7cbc289-d86e-09dc-bdb3-b5496cf0b7ce@web.de/

René Scharfe (2):
  fsck: use strbuf_getline() to read skiplist file

None to the code.

I changed some docs I add in earlier patches to now describe behavior
in a past tense (and the s/sorted // change is earlier), and to change
the docs to say that sorting the list on-disk is now pointless for
optimization purposes, but did something in the past.

  fsck: use oidset for skiplist

There is now a test for the bug you were fixing in your 1/2.

Ævar Arnfjörð Bjarmason (5):
  fsck tests: setup of bogus commit object

Fixing some test redundancies while I'm at it.

  fsck tests: add a test for no skipList input

We didn't test the most basic skipList invocation, fixed while I was
at it...

  fsck: document and test sorted skipList input

Test that sorted & unsorted skipList input, and document that in the
past we said this was required, but it never was, but what (ever so
slight) optimization this gives us.

  fsck: document and test commented & empty line skipList input

We don't support comments or empty lines. Add tests for this not
working, and explicitly document that it doesn't work (I for one tried
it).

  fsck: support comments & empty lines in skipList

Ignoring comments and empty lines is very useful for a file format
that might be human-edited (I maintain one such file). Support that,
and document & test for it.

 Documentation/config.txt        | 22 ++++++++++----
 fsck.c                          | 48 ++++++++++-------------------
 fsck.h                          |  8 +++--
 t/t5504-fetch-receive-strict.sh | 53 ++++++++++++++++++++++++++++++---
 4 files changed, 86 insertions(+), 45 deletions(-)