[darcs-users] darcs patch: add several more darcs-hs review points

[Petr Rockai]

Since darcs.net just went bye bye, I'll reply here...

> +11. Hashed filename format
> +
> +   Ganesh (14/09/09): We've discussed the removal of sizes from the filename
> +   format before, but I'm still not entirely clear on why they were
> +   introduced in the first place. Do you know why, and could you write a
> +   brief justification for their removal? My general impression is that
> +   size+hash is generally considered more robust than hash alone, but that
> +   might be wrong/outdated. The ten digit limit is certainly a bit silly.
The reason for sizes was that the mtime comparison code also compared file
sizes. Since the hashed pristine is compressed, there's no way to know the size
of the pristine files in their unpacked form, without actually unpacking
them. Therefore, the size was tacked into the hash, so the existing mtime-based
code could continue to work. It's simply a hack, nothing to do with safety or
robustness.

> +12. Unsorted variants of ``get_unrecorded`` gone
> +
> +   Ganesh (14/09/09): The _unsorted variants of get_unrecorded_* (now
> +   unrecordedChanges) have gone. They used to omit the sort_coalesceFL call
> +   where it wasn't needed. Have you checked on the performance implications?
No, I just intuitively opted for simple. I doubt this has been properly
profiled before. At least now if sort_coalesceFL shows up high on profiles we
know where to look and can also quantify the saving.

> +13. How do you call ``unrecordedChanges`` to read all changes?
> +
> +   Ganesh (14/09/09): This is something that the old get_unrecorded_*
> +   interface used to do if you passed in an empty list, but now empty list
> +   means "just read pending".
No, unrecordedChanges [] means all changes.

> +14. Please add some haddock to unrecordedChanges
> +
> +   Ganesh (14/09/09): This is a pretty crucial function in the new world.
I was working on it.

> +15. ``invalidateIndex`` safety
> +
> +   Ganesh (14/09/09): It's quite dangerous to have to call this from each
> +   relevant command. Can't the repository code track whether it's needed?
Maybe it could, if the commands used the library in a consistent fashion. I
think a full-scale review (and likely a refactor) of the commands is needed
first. If all pristine manipulation goes through tentative, we can tack this
into the commit phase. But we need to audit the code first. (The pending
manipulation is already covered in the library and I checked all the paths.)

> +16. ``treeDiff`` should be called ``unsafeTreeDiff``
> +
> +   Ganesh (14/09/09): It's just as unsafe with respect to witnesses as the
> +   old one was - though perhaps we should use another prefix to indicate
> +   witness unsafety as opposed to segfault unsafety.
I think using unsafe in this context is pretty stupid. Maybe half of darcs is
unsafe... Patch/* is full of unsafeFoos without a line of explanation. What
does unsafeTreeDiff buy as in terms of clarity? Other than littering the code
with yet more unexplained unsafes...

Yours,
   Petr.

PS: darcs.net seems to be back, but I'll paste this into the wiki later... I
guess sleeptime is very much overdue.