Calling the list of filenames rawindices is misleading. I see that you've changed this code to return an empty list of files when the list of indices was itself an empty list. That makes sense. Could you extract that to a separate patch ? Does it matter in practice for sidedata? Please explain in the commit message if it does. That patch would also assign the decoded list of indices to a variable called files or something like that.

What do you think about writing this as follows? That reduces some of the if filesremoved is None: filesremoved = [] duplication.

filesadded = self._changeset.filesadded
if self._repo.filecopiesmode != b'changeset-sidedata':
    source = self._repo.ui.config(b'experimental', b'copies.read-from')
    if source == b'compatibility':
        if filesadded is None:
            filesadded = copies.computechangesetfilesadded(self)
    elif source != b'changeset-only':
        filesadded = copies.computechangesetfilesadded(self)
if filesadded is None:
    filesadded = []
return filesadded

Analogous changes can be made to filesremoved() and p[12]copies().

nit: combine these into a no-filelog case?

I see that you've changed this code to return an empty list of files when the list of indices was itself an empty list.

I dont' follow this sentence. What do you mean ?

I thought the reason you explicitly did if rawindices is not None was in order to not run the next statement if rawindices was an empty list. But that should have no effect anyway, so I was clearly wrong about that. So why did you make the is not None explicit? Can you just revert it if there was no good reason for it?

I think I prefer my version for being a bit more explicite (all ifs are positive) and hence easier to read for people new to this code. However, I don't mind your version if you really want it that way. Just let me know.

Is there a way of writing self._repo.filecopiesmode != b'changeset-sidedata' in positive way? Maybe self._repo.filecopiesmode is None? The source != b'changeset-only' is because we want any invalid/unexpected config to be treated as "filelog-only".

I won't insist on changing.

I am confused, the function should return a list or None. Since rawindides are bytes, they are not eligible for returns.
I updated the code as follow:

--- a/mercurial/changelog.py
+++ b/mercurial/changelog.py
@@ -366,9 +366,9 @@ class changelogrevision(object):
             rawindices = self._sidedata.get(sidedatamod.SD_FILESADDED)
         else:
             rawindices = self.extra.get(b'filesadded')
-        if rawindices is not None:
-            rawindices = decodefileindices(self.files, rawindices)
-        return rawindices
+        if rawindices is None:
+            return None
+        return decodefileindices(self.files, rawindices)
 
     @property
     def filesremoved(self):

I think you're saying that you changed it in order to handle rawindices == b'' correctly. Makes sense.

What about something like this: We "duplicate" the intend code, and factor the actual "computation" out.

def filesadded(self):
    filesadded = self._changeset.filesadded
    compute_on_none = True
    if self._repo.filecopiesmode == b'changeset-sidedata':
        compute_on_none = False
    else:
        source = self._repo.ui.config(b'experimental', b'copies.read-from')
        if source == b'changeset-only':
            compute_on_none = False
        elif source != b'compatibility':
            # filelog mode, ignore any changelog content
            filesadded = None
    if filesadded is None:
        if compute_on_none:
            filesadded = scmutil.computechangesetfilesadded(self)
        else:
            filesadded = []
    return filesadded

Yes, this is to handle b'' in different way from None.

Actually, both decodefileindices() and decodecopies() seem to properly handle the "empty input" case. So I think you can revert this change. As I suggested earlier, it may be best as a separate patch anyway since it seems unrelated to this patch (although maybe it is required by, if I'm still misunderstanding and it really is a bug). Sorry I didn't notice this earlier.

On this side of the discussion, confusion increases I want None value to be preserved (if not relevant now, if will quickly become so). As far as I understand, decodefileindices does not do this.

I think we've talked about this long enough now that it's clear that changing this behavior is not the purpose of this patch and that it deserves its own patch where you can explain what you're changing and why.