Hi Kamil, On Mon, Aug 31, 2009 at 5:28 AM, Kamil Dworakowski<[email protected]> wrote: > Hi, > > this is my first commit to darcs. It fixes http://bugs.darcs.net/issue1406 > remove_from_tentative_inventory was changing the real inventory and pristine, > and then syncing tentative with it. I changed it operate on tentative only.
Thanks for the patch. Below is the prose of me wandering through the darcs source while I read your patch. I would label this change as 'controversial' for now (see below). I think it should certainly be possible to incorporate it into darcs once my concerns have been addressed. Thanks again! > Three things I don't fully understand there: > 1) what is progressFl for? I think it gives the user feedback about what is happening. I didn't look at the function, I'm just guessing from the name. > 2) what is inventories dir for, or why does it have to be created by > write_inventory There is more than one inventory. I forget which operations split the inventory, but I think it is the creation of a tags. So we store the older inventories in a directory instead of inside the current inventory file. I believe the directory is created during write_inventory as a simple way to always ensure it always exists. > 3) "remove_from_checkpoint_inventory to_remove" line in the > remove_from_tentative_inventory function. It's hard for me to even hazard a guess. I've never understood checkpoints. > Mon Aug 31 13:16:37 BST 2009 Kamil Dworakowski <[email protected]> > * resolve issue1406: amend-record unrecords on a test failure > > A failing test on amend no longer unrecords the original patch. The > failure manifested itself only on old-fashioned repositories. Change > DarcsRepo.remove_from_tentative_inventory not to modify the pristine > nor the inventory. Any changes to old-fashioned repositories are changes we need to be extra careful about. We don't want to break anyone's repos. I've looked at your patch and the coding style and everything seems quite nice. What I don't know is if your changes are correct. For example, this part of the diff: -------- hunk ./src/Darcs/Repository/DarcsRepo.lhs 201 remove_from_tentative_inventory :: RepoPatch p => Bool -> [DarcsFlag] -> FL (Named p) C(x y) -> IO () remove_from_tentative_inventory update_pristine opts to_remove = - do finalize_tentative_changes + do + revert_tentative_changes Sealed allpatches <- read_repo opts "." skipped :< unmodified <- return $ commute_to_end (unsafeCoerceP to_remove) allpatches sequence_ $ mapFL (write_patch opts) skipped -------- The original version of remove_from_tentative_inventory looks like this: ----------- remove_from_tentative_inventory :: RepoPatch p => Bool -> [DarcsFlag] -> FL (Named p) C(x y) -> IO () remove_from_tentative_inventory update_pristine opts to_remove = do finalize_tentative_changes Sealed allpatches <- read_repo opts "." skipped :< unmodified <- return $ commute_to_end (unsafeCoerceP to_remove) allpatches sequence_ $ mapFL (write_patch opts) skipped write_inventory "." $ deep_optimize_patchset $ mapRL_RL n2pia (reverseFL skipped) :<: unmodified remove_from_checkpoint_inventory to_remove when update_pristine $ do pris <- identifyPristine repairable $ applyPristine pris $ progressFL "Applying inverse to pristine" $ invert to_remove revert_tentative_changes ----------- You're replacing a finalize_tentative_changes with a revert_tentative_changes. Just going by the function names that seems a bit odd to me. So I will look a bit deeper. finalize_tentative_changes looks like this: finalize_tentative_changes :: IO () finalize_tentative_changes = renameFile (darcsdir++"/tentative_inventory") (darcsdir++"/inventory") Okay, so remove_from_tentative_inventory assumes there is a file _darcs/tentative_inventory that can be renamed to _darcs/inventory. And you replaced this with a call to revert_tentative_changes, let's see what that does: revert_tentative_changes :: IO () revert_tentative_changes = do cloneFile (darcsdir++"/inventory") (darcsdir++"/tentative_inventory") writeBinFile (darcsdir++"/tentative_pristine") "" So instead of moving the tentative_inventory to the inventory it creates a copy of the current inventory as the tentative_inventory and also creates a tentative_pristine for whatever reason. The next step in your version is to then write out a new tentative_inventory. Hmm...That seems a bit odd. You just cloned the inventory as the tentative_inventory and you're going to write over it. Okay, well the code does read_repo and write out some of those patches so let's see which inventory read_repo looks at. It's more code than I want to paste here, but it looks as though read_repo is going to read inventory and not tentative_inventory. To recap. 1) Clone inventory to tentative_inventory. 2) Read inventory and figure out which patches to get rid of. 3) Rewrite inventory as tentative_inventory. It seems like step 1 is not needed. Am I missing something? Now, the last bit, you removed several lines of code and replaced it with add_to_tentative_pristine: add_to_tentative_pristine :: Effect p => p C(x y) -> IO () add_to_tentative_pristine p = do -- Sealed p <- (fst . fromJust . readPatchCarefully) `fmap` gzReadFilePS fp appendDocBinFile (darcsdir++"/tentative_pristine") $ showPatch (effect p) -- FIXME: this is inefficient! appendBinFile (darcsdir++"/tentative_pristine") "\n" This function seems straightforward. It writes patches to the tentative_pristine file. Here is an important distinction: It doesn't apply patches to the pristine, it just creates a file that contains the contents of the patches. The difference at this part is that the old code would update the pristine to reflect the removal of the patch sequence to_remove. Your version removes the patch sequence to_remove from tentative_inventory and then writes the patch sequence which needs to be removed to the file tentative_pristine. Now I should go read the ticket you mention. I think I see how your change fixes the bug. Will this cause a regression anywhere else? In particular, remove_from_tentative_inventory now no longer removes the sequence to be removed from pristine. If there is a regression it seems like we should be able to come up with cases where the patches should have been unapplied to pristine but are not removed. I've spent a while making this review so hopefully someone else will be willing to construct (or demonstrate it is not possible to construct) such an example. Also, since this is my first time in this par of the code, I could be overlooking something important so please let me know if I missed something. Thanks, Jason _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
