This is an archive of the discontinued Mercurial Phabricator instance.

sidedatacopies: read rename information from sidedata
ClosedPublic

Authored by marmoute on Oct 3 2019, 1:55 AM.

Details

Summary

Repository using the new format now use changeset centric algorithm and read the
copies information from the changelog sidedata.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

marmoute created this revision.Oct 3 2019, 1:55 AM
marmoute updated this revision to Diff 16954.Oct 7 2019, 8:38 PM
martinvonz added inline comments.
mercurial/changelog.py
369–371

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.

martinvonz added inline comments.Oct 9 2019, 2:10 PM
mercurial/context.py
536–550

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().

martinvonz added inline comments.Oct 9 2019, 2:13 PM
tests/test-copies.t
457–459

nit: combine these into a no-filelog case?

marmoute added inline comments.Oct 9 2019, 2:30 PM
mercurial/changelog.py
369–371

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 ?

martinvonz added inline comments.Oct 9 2019, 2:33 PM
mercurial/changelog.py
369–371

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?

marmoute added inline comments.Oct 9 2019, 2:34 PM
mercurial/context.py
536–550

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.

martinvonz added inline comments.Oct 9 2019, 2:38 PM
mercurial/context.py
536–550

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.

marmoute added inline comments.Oct 9 2019, 2:39 PM
mercurial/changelog.py
369–371

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):
martinvonz added inline comments.Oct 9 2019, 2:45 PM
mercurial/changelog.py
369–371

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

marmoute added inline comments.Oct 9 2019, 2:46 PM
mercurial/context.py
536–550

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
marmoute marked 3 inline comments as done.Oct 9 2019, 2:48 PM
marmoute added inline comments.
mercurial/changelog.py
369–371

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

marmoute marked an inline comment as done.Oct 9 2019, 5:31 PM
marmoute updated this revision to Diff 17008.
martinvonz added inline comments.Oct 9 2019, 5:49 PM
mercurial/changelog.py
369–371

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.

marmoute added inline comments.Oct 9 2019, 5:56 PM
mercurial/changelog.py
369–371

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.

martinvonz added inline comments.Oct 9 2019, 5:59 PM
mercurial/changelog.py
369–371

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.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.