darcs

Patch 2212 lift writeInventoryPrivate to the top le... (and 2 more)

Title lift writeInventoryPrivate to the top le... (and 2 more)
Superseder Nosy List bfrk
Related Issues
Status accepted Assigned To
Milestone

Created on 2021-11-03.23:06:29 by bfrk, last changed 2023-03-24.23:21:50 by ganesh.

Files
File name Status Uploaded Type Edit Remove
add-test-case-for-empty-inventories.dpatch bfrk, 2023-03-24.02:12:14 application/x-darcs-patch
lift-writeinventoryprivate-to-the-top-level-and-rename-to-writeinventory.dpatch bfrk, 2021-11-03.23:06:28 application/x-darcs-patch
patch-preview.txt bfrk, 2021-11-03.23:06:27 text/x-darcs-patch
patch-preview.txt bfrk, 2023-03-24.02:12:14 text/x-darcs-patch
See mailing list archives for discussion on individual patches.
Messages
msg22915 (view) Author: bfrk Date: 2021-11-03.23:06:28
3 patches for repository http://darcs.net/screened:

patch 36028bf066c9754dcccbd300a8889399c1b5a82b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Nov  3 07:34:16 CET 2020
  * lift writeInventoryPrivate to the top level and rename to writeInventory

patch f6b14b28d588cefdf6679a0a197dbafbbc707afa
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Mar 24 06:37:23 CET 2021
  * simplify writeInventory and writeTentativeInventory

  The simplification consists of no longer treating empty inventories
  specially. It involves is a minor change in behavior in that an empty
  inventory is now written to disk as a hashed file; which is the reason we
  have to adapt tests/issue1987.sh. Indeed, my original motivation for the
  refactor was so to make writeInventory return a hash unconditionally.

patch ab64067074f7843d61d3ca2859effa28afb2dc0f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Mar 24 18:26:07 CET 2021
  * return hash from writePristine
Attachments
msg23150 (view) Author: ganesh Date: 2023-03-19.00:32:45
>   It involves is a minor change in behavior in that an empty inventory 
is now written to disk as a hashed file

This seems like a good simplification.

Do we need to do anything to check that older darcs will read the new 
empty inventories ok?
msg23166 (view) Author: bfrk Date: 2023-03-22.22:26:04
> Do we need to do anything to check that older darcs will read the new
> empty inventories ok?

No. Like git the darcs hashed store is content addressed, so you can 
litter your repo with hashed files of all sorts (inventories, patches, 
pristine) related or unrelated to the repo with no semantic effect 
whatsoever. (This is why the global cache works as it does, BTW.)
msg23169 (view) Author: ganesh Date: 2023-03-22.23:26:26
> Like git the darcs hashed store is content addressed, so
> you can litter your repo with hashed files of all sorts
> related or unrelated to the repo with no semantic effect
> whatsoever.

Oh, so we don't refer to the hashed file, just literally
dump it to disk? I hadn't realised.
msg23171 (view) Author: bfrk Date: 2023-03-23.00:07:32
> Oh, so we don't refer to the hashed file, just literally
> dump it to disk? I hadn't realised.

It took me a while to understand this remark (again the case of 
something being "obvious" to me), but yes, this is precisely what 
happens. (This is true for all hashed files but not for e.g. 
_darcs/hashed_inventory.) Hashed files are "write-once" i.e. never 
modified, which is the reason we can safely use lazy IO when reading 
them. In git they even go so far as to remove write permission for 
hashed files ("objects" in git jargon) as soon as they are written.
msg23172 (view) Author: ganesh Date: 2023-03-23.00:19:47
> Hashed files are "write-once" i.e. never modified,
> which is the reason we can safely use lazy IO when
> reading them.

We may be talking at cross-purposes, so just to check.

Under normal circumstances, every hashed file we write is intended
to be reachable from `hashed_inventory` at the end of the current
operation. Of course they might become garbage later and that's fine.

But here you seem to be saying that we will write a hashed file that
will immediately and unconditionally become garbage? That's what I
meant by "refer to the hashed file" - I had assumed that we would
have a reference to it from some other inventory.
msg23174 (view) Author: bfrk Date: 2023-03-23.00:27:00
Side note: the hashed store was designed by mornfall (Petr Ročkai) 
who took many inspirations from how git works internally. As I 
realized just recently, the "index" in Darcs (also work by mornfall) 
is yet another concept inspired by git, in fact our index looks 
quite similar to that of git, right down to the binary format 
(though we don't use it as a "staging area", only as a cache to 
improve performance of working tree lookups).
msg23175 (view) Author: bfrk Date: 2023-03-23.00:48:28
> But here you seem to be saying that we will write a hashed file that
> will immediately and unconditionally become garbage? That's what I
> meant by "refer to the hashed file" - I had assumed that we would
> have a reference to it from some other inventory.

Ah, I see what you mean. Indeed, when you recorded a tag in an empty repo, 
darcs would previously NOT refer to any empty inventory.

In contrast, now we DO create an empty inventory and also refer to it. In 
the above scenario our _darcs/hashed_inventory now contains an additional

Starting with inventory:
0000000000-e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855

But the functions that /read/ inventories never had a special case for this 
situation and are therefore able to read both variants just fine.
msg23176 (view) Author: bfrk Date: 2023-03-23.00:59:10
Note that the patch does not touch the inventory reading functions 
at all.

I guess it wouldn't hurt to add a script that tests this situation 
explicitly i.e. recording a tag in an empty repo and check that we 
can read that. And similarly, test that we can still read a repo 
with just a tag in it that was created using darcs-2.16.
msg23177 (view) Author: bfrk Date: 2023-03-24.02:12:14
So here is my attempt to test backward and (in a limited way) forward
compatibility for writing empty inventories.

1 patch for repository http://darcs.net/screened:

patch 6735bbefb8618e572ec17a17bdd170ba6ebb4221
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Mar 23 02:19:18 CET 2023
  * add test case for empty inventories
Attachments
History
Date User Action Args
2021-11-03 23:06:29bfrkcreate
2021-11-04 06:38:43bfrksetstatus: needs-screening -> needs-review
2023-03-19 00:32:45ganeshsetstatus: needs-review -> review-in-progress
messages: + msg23150
2023-03-22 22:26:04bfrksetmessages: + msg23166
2023-03-22 23:26:26ganeshsetmessages: + msg23169
2023-03-22 23:26:31ganeshsetstatus: review-in-progress -> accepted-pending-tests
2023-03-23 00:07:32bfrksetmessages: + msg23171
2023-03-23 00:19:47ganeshsetmessages: + msg23172
2023-03-23 00:27:00bfrksetmessages: + msg23174
2023-03-23 00:48:28bfrksetmessages: + msg23175
2023-03-23 00:59:10bfrksetmessages: + msg23176
2023-03-24 02:12:14bfrksetfiles: + patch-preview.txt, add-test-case-for-empty-inventories.dpatch
messages: + msg23177
2023-03-24 23:21:50ganeshsetstatus: accepted-pending-tests -> accepted