darcs

Patch 2133 use hires timestamps for the index

Title use hires timestamps for the index
Superseder Nosy List bf
Related Issues
Status accepted-pending-tests Assigned To
Milestone

Created on 2020-12-20.10:12:53 by bf, last changed 2020-12-27.08:32:37 by bf.

Files
File name Status Uploaded Type Edit Remove
make-__ignore_times-a-standard-option-supported-by-all-commands.dpatch bf, 2020-12-20.10:12:50 application/x-darcs-patch
patch-preview.txt bf, 2020-12-20.10:12:50 text/x-darcs-patch
unnamed bf, 2020-12-20.10:12:50 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg22576 (view) Author: bf Date: 2020-12-20.10:12:50
6 patches for repository http://darcs.net/screened:

patch 973faa6b3113dbcd87fbff6a43d4814a965d61a2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct 25 07:36:26 CET 2020
  * make --ignore-times a standard option supported by all commands
  
  This also fixes all places where we still passed the UseIndex constructor
  directly without considering the option value.

patch eab7eae5a7a842e554e5386dac76fb72d1c254a4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct 25 07:07:15 CET 2020
  * remove extraneous setting of --ignore-times in test scripts

patch ac7be667db0b77b564dfa6f7497f2293909711d7
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Oct 26 18:46:45 CET 2020
  * Darcs.Util.Index: fix a few minor errors in comments

patch 7bc51cb780c043862c891bac5a589892b259d911
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Oct 28 00:45:49 CET 2020
  * index: use high resolution timestamps
  
  This requires building against a patched unix-compat, since the pull request
  is still pending. Tests scripts where sleep calls were inserted or
  --ignore-times was explicitly passed in order to work around low timestamp
  resolutions have been adapted accordingly. In the network tests I have kept
  the explicit --ignore-times when executing the remote darcs, because this
  may be a version of darcs different from the one under test.

patch 2b95e7816fbccef614782b90ca27f195387d9a7c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Oct 28 10:52:30 CET 2020
  * harness: add flag to test with or without the index

patch b0ec10c78ac19423c896352ed7fac14b59f60229
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Oct 30 15:12:33 CET 2020
  * enable use of the index in the test suite by default
  
  This also adapts test scripts that assumed the former default in order to
  test the right thing. We now pass --ignore-times (or its opposite)
  explicitly instead.
Attachments
msg22607 (view) Author: ganesh Date: 2020-12-26.15:37:23
On 20/12/2020 10:12, Ben Franksen wrote:

>   * make --ignore-times a standard option supported by all commands

Looks good.

What was the reason for reordering the options in Amend and Record?

As an aside the style of using `withStdOpts` infix confused me for a bit
because I thought it was just some kind of simple combining operation,
not something that also added the standard options.

>   * remove extraneous setting of --ignore-times in test scripts

OK

>   * Darcs.Util.Index: fix a few minor errors in comments

OK

>   * index: use high resolution timestamps

Looks good.

>   This requires building against a patched unix-compat, since the pull request
>   is still pending.

This will be something of a pain even for local development, as it'll
get rebuilt whenever the local cabal config changes. So we should figure
out some long-term solution fairly quickly.

>   * harness: add flag to test with or without the index
>   * enable use of the index in the test suite by default

Nice.
msg22609 (view) Author: bf Date: 2020-12-27.08:32:36
> What was the reason for reordering the options in Amend and Record?

This is an artefact of these commands using configuration records: the
order of the fields in the record matters. This is bad and one of the
reasons I decided to revert amend and record back to using [DarcsFlag],
see patch2143.

> As an aside the style of using `withStdOpts` infix confused me for a bit
> because I thought it was just some kind of simple combining operation,
> not something that also added the standard options.

Right. This was a bad idea. In new code or when I change code containing
withStdOpts I nowadays tend to use it prefix. I would not be opposed to
changing it to prefix everywhere.

>>   This requires building against a patched unix-compat, since the pull request
>>   is still pending.
> 
> This will be something of a pain even for local development, as it'll
> get rebuilt whenever the local cabal config changes. So we should figure
> out some long-term solution fairly quickly.

IME the pain is negligable, compiling unix-compat is very fast (there is
no complicated Haskell stuff inside). Nevertheless, I fully agree that
depending on patched versions of external libraries is not a good
long-term solution!

You may remember that I had a patch that drops the dependency and
instead copy/pastes the stuff we need to our win32/ subtree. We briefly
discussed this on IRC and you expressed reservations about the idea
(IIRC). I mention it because that /would/ be a viable long-term
solution, though perhaps undesirable for other reasons.
History
Date User Action Args
2020-12-20 10:12:54bfcreate
2020-12-20 10:14:34bfsetstatus: needs-screening -> needs-review
2020-12-26 15:32:14ganeshsetstatus: needs-review -> accepted-pending-tests
2020-12-26 15:37:24ganeshsetmessages: + msg22607
2020-12-27 08:32:37bfsetmessages: + msg22609