[Updated] D9090: changing-files: rework the way we store changed files in side-data

[marmoute (Pierre-Yves David)]

marmoute added inline comments.

INLINE COMMENTS

> Alphare wrote in revlogs.txt:262
> Is there a reason to use big-endian instead of little-endian (as x86 and x86-64 are little-endian)? I had the same question asked when I proposed the first draft of the dirstate cache.

My three main reason to go for big-endian

Big endian is more standard (network encoding) and everything else in Mercurial use big endian. So sticking with the current practice is better for consistency.

x86 process are usually very good at dealing with big endian data, and I have never seen any significant slowdown for using BE data.

Using Big Endian for storage usually make sure people though about Endianness for storage before it is too late. Avoiding releasing inconsistent endianess in the wild, corrupting user data.

> Alphare wrote in revlogs.txt:267
> unsigned, I assume?

sure :-/

> Alphare wrote in revlogs.txt:308
> Please fix the typos in this sentence, it's kind of hard to read.

Will do. I think it just need a:

`If now copy` → `If no copy/` and `the value or this field` → `the value of this field`.

do you see anything else ?

> pulkit wrote in metadata.py:385
> nit: I guess we can directly use `files.*` attributes.

We could, but we do not have caching in the object yet. But it is coming right after. I'll follow up with a cleanup.

> pulkit wrote in metadata.py:389
> nit: we can preserve this line of documentation.

Indeed, we should. This probably got dropped while manipulating patches.

> Alphare wrote in metadata.py:418
> The `in` + `get` seems wasteful, but I'm guessing you don't expect people to actually use the pure version?

I don't expect this to be a major bottleneck. I can do some performance measurement once the dust settle. but I do not expect to find anything special.

> Alphare wrote in metadata.py:438
> I think we could benefit from some sort of asserts when reading sizes or in case of overflow.

Good point.

> pulkit wrote in metadata.py:480
> This diff can ideally be in a different patch as it's not related to storage rework.

Which diff are you thinking about ? the specific `filesmerged = computechangesetfilesmerged(ctx)` line ? If so the value would not be used anywhere without this change.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D9090/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D9090

To: marmoute, #hg-reviewers, Alphare
Cc: Alphare, pulkit, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20201001/91d5a3a2/attachment-0002.html>