[darcs-devel] [patch2135] index refactors

[Ben Franksen]

Ben Franksen <ben.franksen at online.de> added the comment:

Am 27.12.20 um 14:17 schrieb Ganesh Sittampalam:
> 
> Ganesh Sittampalam <ganesh at earth.li> added the comment:
> 
> Partial review:
> 
>>   * avoid useless CPP in D.U.Index and D.R.State
>>   
>>   We can use the same action (renameFile instead of removeFile) for both
>>   Windows and Posix. The point here is that the index may still be open via an
>>   mmapped ForeignPtr, so removing it is not portable, but renaming it should
>>   work on all systems.
> 
>> - -- TODO this conditional logic (rename or delete) is mirrored in
>> - -- Darcs.Repository.State.checkIndex and should be refactored
> 
> I think it'd still be good to have a single function, or to keep the
> TODO pointing between the two bits of code, even though the logic is
> only one line now.
> 
> That way if someone does want to change it in future they won't
> accidentally change one and not the other.

This is indeed quite tricky. In the latest version I do have an extra
comment in both places. However, I am not 100% sure the reason we need
to use renameFile and not removeFile is the same. Inside
Darcs.Util.Index, the index is still open (mmapped) at this point
(creating the new index involves reading from the old one), so it makes
sense to me that we should not remove it, at least on Windows (on Posix
systems, removing a file does not invalidate access to an open handle to
that file). But in Darcs.Repository.State.checkIndex this should not be
the case; still, using removeFile there /also/ fails (with an "access
denied") and in this case it is pretty unclear to me why.

Can we postpone this question until after you have reviewed the full
bundle and seen the related changes, including the new comment I added?

>>   * index: re-use getFileStatus for getFileID' (Posix)
> 
> Is this intended to be behaviour-preserving?> getFileStatus (defined in
> the same file) seems to behave differently on symbolic links to the old
> code.

It is well possible that the new behavior differs in some corner cases
from the old one. Messy IO code like this is always difficult to reason
about ;-) I should also mention that the file IDs in the index are there
exclusively to detect file moves. So even if I made a mistake here, it
is likely that this does not break anything essential.

Anyway, I claim that the new code does what we want it to do (apart from
the extra warning): it should treat symbolic links as non-existent,
regardless of where the link points to. Note that
Darcs.Util.Index.getFileStatus calls Darcs.Util.File.getFileStatus which
calls getSymbolicLinkStatus and then returns Nothing if this fails with
an exception. This means that the existence tests are redundant (they
will cause exceptions and thus a return Nothing). If it exists and is a
symbolic link, Darcs.Util.Index.getFileStatus issues a warning and
returns Nothing. So the new code is "correct" in that it behaves as
specified above.

The extra warning is unwanted but is fixed in a later patch "use file
IDs from unix-compat" where we completely remove getFileID' and always
take it from the file status. This patch should be seen as a preparation
for that.

>>   * index: move update of fileid out of createItem
>>   
>>   This is a preparation for a later refactor.
> 
> OK - I shall assume the later refactor is worthwhile :-)

I think you will like it: it is the one I mentioned above, where we get
rid of getFileID' (including the CPP).

> Does the doc comment for createItem need to change?

No, I just checked and it still seems to be "correct" i.e. vague enough
not to be outright wrong ;-)

>>   * show index: show everything, not just hash, id, and path
> 
> [Darcs.UI.Commands.ShowIndex]
>>  entries <- dumpIndex indexPath `catchall` return []
> 
> Should we avoid introducing new uses of catchall? Is there a specific
> kind we could catch here?

Hmm. I must admit I was lazy here. My excuse is that 'darcs show index'
is more a debugging aid than a proper user level command. I think the
right thing to do here is to simply remove the catchall, which has the
same effect as catching and printing the exception and then aborting. I
just tried that and tested it by removing _darcs/index, which now gives me:

>darcs show index
darcs: _darcs/index: opening of '_darcs/index' failed: does not exist
(No such file or directory)

Will send a follow-up patch.

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch2135>
__________________________________