darcs

Patch 1917 fix lazy reading of inventories for apply and obliterate

Title fix lazy reading of inventories for apply and obliterate
Superseder Nosy List bf
Related Issues
Status accepted Assigned To
Milestone

Created on 2019-09-14.18:30:53 by bf, last changed 2019-09-20.20:18:43 by bf.

Files
File name Status Uploaded Type Edit Remove
patch-preview.txt bf, 2019-09-14.18:30:52 text/x-darcs-patch
patch-preview.txt bf, 2019-09-19.15:15:47 text/x-darcs-patch
print-out-name-of-patch-bundle-for-obliterate-__output.dpatch bf, 2019-09-19.15:15:47 application/x-darcs-patch
remove-message-output-in-getuniquedpatchname.dpatch bf, 2019-09-14.18:30:52 application/x-darcs-patch
unnamed bf, 2019-09-14.18:30:52 text/plain
unnamed bf, 2019-09-19.15:15:47 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg21414 (view) Author: bf Date: 2019-09-14.18:30:52
The bundle contains a few minor cleanups that came up while I worked on
this. They are recorded in separate patches.

6 patches for repository http://darcs.net/screened:

patch 06e57cd0115de504d1b39a2f535aab85c3ed28f5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Sep 14 13:11:43 CEST 2019
  * remove message output in getUniqueDPatchName
  
  This function in used to derive an automatic name for a patch bundle, so we
  don't have to explain that there already exists a file with that name etc,
  since we display the final file name anyway.

patch 53dd8138ad1d47d048af85e777b7dfef940ef4c6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Sep 14 13:15:26 CEST 2019
  * use contextPatches when creating a patch bundle
  
  This performs an extra slightlyOptimizePatchset which can't hurt.

patch 97bd6c89444ef5d9ecee3ba685c05e785a54ffee
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Sep 14 20:05:56 CEST 2019
  * simplify an expression in applyCmdCommon

patch 963f3b20f5081d536455954064e5c81d920d05a8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Sep 14 13:37:47 CEST 2019
  * fix lazy reading of inventories for obliterate with --output
  
  This one possibly fell victim to a simplification I made once, therefore the
  somewhat longer comment that explains why we have to re-read the original
  repo contents when we generate the context for the patch bundle.

patch dd705f5f9c5aa80f68306c36aff6f31f2c389be6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Sep 14 20:02:21 CEST 2019
  * fix lazy reading of inventories for apply command
  
  This was broken by checking availability of patches to be applied in a
  complicated and inefficient way, which as a side-effect reads all local
  inventories in our history.

patch c63580de8d04b2b2205412597b359fd74a031642
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Sep 14 20:03:06 CEST 2019
  * extend tests/issue2293-laziness-amend.sh with more commands
  
  We now test that it works with log (--context and --last=1), amend, record,
  send, obliterate, and apply.
Attachments
msg21452 (view) Author: ganesh Date: 2019-09-18.20:14:01
>  * remove message output in getUniqueDPatchName

I'm not sure we do always print out the final file name (e.g.
in darcs obliterate -O) but either way I agree it doesn't make
sense to say that an automatically generated file name had to be
changed.

>   * use contextPatches when creating a patch bundle

OK

>   * simplify an expression in applyCmdCommon

OK

>  * simplify an expression in applyCmdCommon

Did you swap the arguments of saveToBundle just so you could partially
apply it? I find the new order more confusing but it's no big deal.

>   * fix lazy reading of inventories for apply command

Fine

>   * extend tests/issue2293-laziness-amend.sh with more commands

Fine
msg21460 (view) Author: bf Date: 2019-09-19.14:13:14
>>  * remove message output in getUniqueDPatchName
> 
> I'm not sure we do always print out the final file name (e.g.
> in darcs obliterate -O)

Yes. I plan to fix that, showing the actual file name is pretty
essential for usability.

>>  * simplify an expression in applyCmdCommon
> 
> Did you swap the arguments of saveToBundle just so you could partially
> apply it? I find the new order more confusing but it's no big deal.

(This is actually in patch

  * fix lazy reading of inventories for obliterate with --output)

Yes, ahem, looks like this was the reason. I tried to re-construct why I
did it like that and I think my thinking went as follows:

- okay, result of readRepo is only ever used once
- giving a name to something that will be immediately consumed is bad
- so use a combinator, hmm, >>= seems to be the one
- oh, order of arguments doesn't fit, use flip?
- no, flip is a bit obscure; where is saveToBundle used?
- this is the only call site, let's just change its argument order

I am curious: would you consider keeping the original order of arguments
and instead using flip the better solution in this case? Or would you
rather give a name to the intermediate PatchSet?
msg21461 (view) Author: bf Date: 2019-09-19.15:15:47
Following up on review. Will screen immediately.

1 patch for repository http://darcs.net/screened:

patch 3bfb31206bf6299a4a806f2d35e89e7ae5fe1f1d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Sep 19 17:17:45 CEST 2019
  * print out name of patch bundle for obliterate --output
Attachments
msg21504 (view) Author: ganesh Date: 2019-09-20.19:41:49
> I am curious: would you consider keeping the original order of arguments
> and instead using flip the better solution in this case? Or would you
> rather give a name to the intermediate PatchSet?

I think I generally find pointfree style more attractive when I'm
writing code than when I'm reading it :-) So from the perspective of a
reviewer now I'd probably say "give it a name". But I think a flip would
have been fine too. And even the actual code is not worth changing again
now.
msg21508 (view) Author: bf Date: 2019-09-20.20:18:43
>> I am curious: would you consider keeping the original order of arguments
>> and instead using flip the better solution in this case? Or would you
>> rather give a name to the intermediate PatchSet?
> 
> I think I generally find pointfree style more attractive when I'm
> writing code than when I'm reading it :-)

:-)) You have a certain point there.

> So from the perspective of a
> reviewer now I'd probably say "give it a name". But I think a flip would
> have been fine too. And even the actual code is not worth changing again
> now.

Agreed, I really asked (only) out of curiosity. I will try and be more
careful about pointlessly using pointfree style...
History
Date User Action Args
2019-09-14 18:30:53bfcreate
2019-09-14 18:37:27bfsetstatus: needs-screening -> needs-review
2019-09-18 20:14:01ganeshsetstatus: needs-review -> accepted-pending-tests
messages: + msg21452
2019-09-18 22:50:56ganeshsetstatus: accepted-pending-tests -> accepted
2019-09-19 14:13:14bfsetmessages: + msg21460
2019-09-19 15:15:47bfsetfiles: + patch-preview.txt, print-out-name-of-patch-bundle-for-obliterate-__output.dpatch, unnamed
messages: + msg21461
2019-09-20 19:41:49ganeshsetmessages: + msg21504
2019-09-20 20:18:43bfsetmessages: + msg21508