darcs

Patch 1743 rename test for issue1702 to mark as fai... (and 3 more)

Title rename test for issue1702 to mark as fai... (and 3 more)
Superseder Nosy List bf, ganesh
Related Issues
Status accepted Assigned To ganesh
Milestone

Created on 2018-10-14.00:00:57 by bf, last changed 2018-11-25.10:30:11 by ganesh.

Files
File name Status Uploaded Type Edit Remove
patch-preview.txt bf, 2018-10-14.00:00:56 text/x-darcs-patch
rename-test-for-issue1702-to-mark-as-failing.dpatch bf, 2018-10-14.00:00:56 application/x-darcs-patch
unnamed bf, 2018-10-14.00:00:56 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg20401 (view) Author: bf Date: 2018-10-14.00:00:56
These are cleanups and a minor fix which are all dependencies of more
important refactors to be sent separately.

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

patch 674ba6bd0e794040877a64c62c1ae09b534b768a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Oct 12 00:18:11 CEST 2018
  * rename test for issue1702 to mark as failing

patch 62524e7c12637be444ff69f1b029df1164d6c30e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Oct  7 18:53:39 CEST 2018
  * fix in output of log command
  
  I think the 'not' here got lost during a refactor.

patch 2ab10ac8cecf5e0ff1692b357ed590fc4d780410
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Oct  3 20:57:48 CEST 2018
  * eliminate hard-coded repo paths from the UI layer
  
  There is one exception in darcs convert where we create a "marks" file
  inside _darcs but I guess this is an ad-hoc addition.
  
  Note that this is the first time this operation of concentrating repo paths
  to a single module actually pays off: in the convert implementation we used
  a wrong file name _darcs/tentative_hashed_pristine (the correct name is
  _darcs/pristine.tentative). So this also fixes a bug.

patch d44114d1a0de2a740062270c6787e382f6484210
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep 30 19:22:35 CEST 2018
  * remove excessive debug output in D.R.HashedIO
Attachments
msg20471 (view) Author: bf Date: 2018-11-16.14:04:26
Note this bundle needs a number of other patches in order for it to
compile cleanly. These patches are in screened but not in reviewed yet.
msg20473 (view) Author: ganesh Date: 2018-11-16.16:07:28
Pushing the issue1702 test rename to reviewed now to help get the
tests in better shape, as it's also failing for me locally.
msg20481 (view) Author: ganesh Date: 2018-11-17.11:27:09
>   * rename test for issue1702 to mark as failing

Fine, I've lost track of what's actually going on with this test,
but it's only a test and it was failing for me so I'm happy not
to have to worry about it more :-)

>  * fix in output of log command
>    I think the 'not' here got lost during a refactor.

This also changes a variable reference from 'files' to 'paths'
and doesn't compile for me yet (as you already warned about).
I'll come back to it once I find whatever other patch makes it work.

In general patches with "extra" changes often cause disproportionate
effort on the reviewing side for this kind of reason. But it's only
an occasional issue so no big deal, and to a large extent it's my
own fault for being so far behind on reviewing.

>  * eliminate hard-coded repo paths from the UI layer

Fine

>  * remove excessive debug output in D.R.HashedIO

Fine
msg20483 (view) Author: ganesh Date: 2018-11-17.13:18:39
>  * fix in output of log command

The introduction of the 'not' looks fine.

[Turns out this starts to work after this patch:

>  * use AnchoredPath instead of FileName for internal path 
representation
]
msg20528 (view) Author: bf Date: 2018-11-19.02:12:09
>>   * rename test for issue1702 to mark as failing
> 
> Fine, I've lost track of what's actually going on with this test,

The test is fine but optimize relink doesn't (fully) work as expected
(the link count is 2, not 3) and I have no idea why.

>>  * fix in output of log command
>>    I think the 'not' here got lost during a refactor.
> 
> This also changes a variable reference from 'files' to 'paths'
> and doesn't compile for me yet (as you already warned about).
> I'll come back to it once I find whatever other patch makes it work.

I think I did some of these files->paths renames in the course of the
FileName->AnchoredPath refactor.

> In general patches with "extra" changes often cause disproportionate
> effort on the reviewing side for this kind of reason. But it's only
> an occasional issue so no big deal, and to a large extent it's my
> own fault for being so far behind on reviewing.

There are other factors that come into play here. One lesson is that
being able to commute patches is not always a win. Here I should have
put in a semantic i.e. explicit dependency, but it is hard to think of
such things at the right time. It would help if darcs could
automatically add dependencies based on the outcome of a test. I am
thinking of something similar to 'darcs test': obliterate one patch
after the other (if dependencies allow it) until the test (cabal build,
whatever) fails, in that case add the patch to the list of dependencies
and continue until we hit a clean tag or some such.

Another lesson for me is to try harder to avoid mixing "extra" changes
with a refactor or bug fix. I have become better at this but now and
again I still forget to record such changes separately. Doing such a
separation as an afterthought can be a very tedious affair up to the
point at which it is easier to throw away all changes to a file and
re-do the them manually. I also find myself using amend --unrecord a lot
lately to pull apart things I previously lumped together in a single patch.
History
Date User Action Args
2018-10-14 00:00:57bfcreate
2018-10-14 00:02:34bfsetstatus: needs-screening -> needs-review
2018-11-16 14:04:27bfsetmessages: + msg20471
2018-11-16 16:07:28ganeshsetmessages: + msg20473
2018-11-17 11:27:09ganeshsetstatus: needs-review -> review-in-progress
assignedto: ganesh
messages: + msg20481
nosy: + ganesh
2018-11-17 13:18:40ganeshsetstatus: review-in-progress -> accepted-pending-tests
messages: + msg20483
2018-11-19 02:12:11bfsetmessages: + msg20528
2018-11-25 10:30:11ganeshsetstatus: accepted-pending-tests -> accepted