darcs

Patch 122 Resolve issue121: add --ask-deps supportto amend-record

Title Resolve issue121: add --ask-deps supportto amend-record
Superseder Nosy List darcs-users, galbolle, ganesh, ganesh.sittampalam, tux_rocker
Related Issues wish: amend-record --ask-deps
View: 121
Status accepted Assigned To galbolle
Milestone

Created on 2009-12-28.22:54:13 by ganesh, last changed 2011-05-10.17:16:00 by darcswatch. Tracked on DarcsWatch.

Files
File name Status Uploaded Type Edit Remove
resolve-issue121_-add-__ask_deps-support-to-amend_record.dpatch ganesh, 2009-12-28.22:54:12 text/x-darcs-patch
unnamed ganesh, 2009-12-28.22:54:12 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg9696 (view) Author: ganesh Date: 2009-12-28.22:54:12
This only provides "additive" support for dependencies: you can't
remove existing dependencies. I'll open a new feature request for
doing better with this - additive behaviour is at least consistent
with what happens with the patch contents, although you can usually
work round that by adding in the inverse changes.

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

Mon Dec 28 22:36:36 GMT 2009  Ganesh Sittampalam <ganesh@earth.li>
  * Resolve issue121: add --ask-deps support to amend-record
Attachments
msg9722 (view) Author: galbolle Date: 2010-01-05.12:59:19
Resolve issue121: add --ask-deps support to amend-record
--------------------------------------------------------
Ganesh Sittampalam <ganesh@earth.li>**20091228223636

Codewise, this patch is good. Is it hot enough to be applied despite
the soft freeze? I'd vote yes, but leave that to the release manager.

>[…]
hunk ./src/Darcs/Commands/AmendRecord.lhs 154
>                         putStrLn "Finished amending patch:"
>                         putDocLn $ description newp
>  
> -updatePatchHeader :: forall p. (RepoPatch p) => [DarcsFlag] -> PatchInfoAnd p
-> FL Prim
> +updatePatchHeader :: forall p. (RepoPatch p) => [DarcsFlag] -> Repository p
-> PatchInfoAnd p -> FL Prim
>                    -> IO (Maybe String, PatchInfoAnd p)

updatePatchHeader now needs to know where to offer new dependencies from.

hunk ./src/Darcs/Commands/AmendRecord.lhs 156
> -updatePatchHeader opts oldp chs = do
> +updatePatchHeader opts repository oldp chs = do
> +
> +                       let newchs = canonizeFL (effect oldp +>+ chs)
> +
> +                       let old_pdeps = getdeps $ hopefully oldp
> +                       newdeps <- if AskDeps `elem` opts
> +                                  then askAboutDepends repository newchs opts
old_pdeps
> +                                  else return old_pdeps
> +

Ok, we add the new deps if necessary.

[…]
hunk ./src/Darcs/Commands/Record.lhs 398
>  depended-upon patches.
>  
>  \begin{code}
> -askAboutDepends :: RepoPatch p => Repository p -> FL Prim -> [DarcsFlag] ->
IO [PatchInfo]
> -askAboutDepends repository pa' opts = do
> +askAboutDepends :: RepoPatch p => Repository p -> FL Prim -> [DarcsFlag] ->
[PatchInfo] -> IO [PatchInfo]
> +askAboutDepends repository pa' opts olddeps = do
> +  -- ideally we'd just default the olddeps to yes but still ask about them.
> +  -- SelectChanges doesn't currently (17/12/09) offer a way to do this so would
> +  -- have to have this support added first.
>    pps <- read_repo repository
>    pa <- n2pia `fmap` anonymous (fromPrims pa')
>    let ps = (reverseRL $ headRL pps)+>+(pa:>:NilFL)

hunk ./src/Darcs/Commands/Record.lhs 407
>        (pc, tps) = patchChoicesTps ps
> -      ta = case filter ((pa `unsafeCompare`) . tpPatch) $ unsafeUnFL tps of
> -                [tp] -> tag tp
> +      tas = case filter (\tp -> pa `unsafeCompare` tpPatch tp || info
(tpPatch tp) `elem` olddeps) $ unsafeUnFL tps of
>                  [] -> error "askAboutDepends: []"

This gets the tags of the old dependencies. I think that in a followup
patch, this should be abstracted into a function for retrieving a list
of tags corresponding to patches matching some function in a
PatchChoices, which would go in Darcs.Patch.Choices.

hunk ./tests/issue121.sh 1
> +#!/usr/bin/env bash
> +## Test for issue121 - amend-record --ask-deps
> +##
> +## Copyright (C) 2009 Ganesh Sittampalam
> +##
> +## Permission is hereby granted, free of charge, to any person
> +## obtaining a copy of this software and associated documentation
> +## files (the "Software"), to deal in the Software without
> +## restriction, including without limitation the rights to use, copy,
> +## modify, merge, publish, distribute, sublicense, and/or sell copies
> +## of the Software, and to permit persons to whom the Software is
> +## furnished to do so, subject to the following conditions:
> +##
> +## The above copyright notice and this permission notice shall be
> +## included in all copies or substantial portions of the Software.
> +##
> +## THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +## EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +## MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> +## NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> +## BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> +## ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> +## CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> +## SOFTWARE.
> +
> +. ../tests/lib                  # Load some portability helpers.
> +rm -rf R
> +darcs init      --repo R        # Create our test repos.
> +
> +cd R
> +touch a
> +darcs add a
> +darcs rec --ignore-times -am 'add a'
> +(echo '1' ; echo '1' ; echo '1') > a
> +darcs rec --ignore-times -am 'patch X'
> +(echo '2' ; echo '1' ; echo '1') > a
> +darcs rec --ignore-times -am 'patch Y'
> +(echo '2' ; echo '1' ; echo '2') > a
> +darcs rec --ignore-times -am 'patch Z'
> +
> +darcs obliterate --dry-run --patch 'patch Y' | not grep 'patch Z'
> +
> +echo 'yy' | darcs amend-rec --ask-deps
> +
> +darcs obliterate --dry-run --patch 'patch Y' | grep 'patch Z'

Test for this functionality. Y and Z depend on X. Then Z is made to
depend on Y. First, unpulling Y leaves Z alone, then it unpulls it too.
msg9727 (view) Author: darcswatch Date: 2010-01-05.19:16:24
This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-6c7840515bc68312dda17d6ee3ac6ebca92b955d
msg9728 (view) Author: ganesh Date: 2010-01-05.21:57:20
Hi,

Thanks for the review!

On Tue, 5 Jan 2010, Florent Becker wrote:

> Resolve issue121: add --ask-deps support to amend-record
> --------------------------------------------------------
> Ganesh Sittampalam <ganesh@earth.li>**20091228223636
>
> Codewise, this patch is good. Is it hot enough to be applied despite
> the soft freeze? I'd vote yes, but leave that to the release manager.

As I understand it, this isn't a release goal so it doesn't make it into 
the branch. (But it's fine to apply patches to head as the branch has been 
forked, and in any case I note that someone has already done this for this 
patch.)

Personally I think it'd be good to have this in 2.4, but now that it's 
missed beta1 the argument against is stronger than before.

> hunk ./src/Darcs/Commands/Record.lhs 407
>>        (pc, tps) = patchChoicesTps ps
>> -      ta = case filter ((pa `unsafeCompare`) . tpPatch) $ unsafeUnFL tps of
>> -                [tp] -> tag tp
>> +      tas = case filter (\tp -> pa `unsafeCompare` tpPatch tp || info
> (tpPatch tp) `elem` olddeps) $ unsafeUnFL tps of
>>                  [] -> error "askAboutDepends: []"
>
> This gets the tags of the old dependencies. I think that in a followup
> patch, this should be abstracted into a function for retrieving a list
> of tags corresponding to patches matching some function in a
> PatchChoices, which would go in Darcs.Patch.Choices.

I'm not convinced by this; it'd be quite a short function and I'm not 
aware of anywhere else it'd be useful.

Ganesh
msg9730 (view) Author: tux_rocker Date: 2010-01-06.08:05:14
Op dinsdag 05 januari 2010 13:59 schreef je:
> Resolve issue121: add --ask-deps support to amend-record
> --------------------------------------------------------
> Ganesh Sittampalam <ganesh@earth.li>**20091228223636
> 
> Codewise, this patch is good. Is it hot enough to be applied despite
> the soft freeze? I'd vote yes, but leave that to the release manager.

I won't apply it to the release branch because of the soft freeze. But you are 
free to apply it to the main branch.

I can be convinced to apply this to the 2.4 branch by arguments along the line 
of "power user X has been screaming for this feature for five years, don't 
keep it from him any longer!"

Reinier
msg9731 (view) Author: ganesh Date: 2010-01-06.10:30:39
Reinier Lamers wrote:
> Reinier Lamers <tux_rocker@reinier.de> added the comment:
> 
> Op dinsdag 05 januari 2010 13:59 schreef je:
>> Resolve issue121: add --ask-deps support to amend-record
>> --------------------------------------------------------
>> Ganesh Sittampalam <ganesh@earth.li>**20091228223636
>> 
>> Codewise, this patch is good. Is it hot enough to be applied despite
>> the soft freeze? I'd vote yes, but leave that to the release manager.
> 
> I won't apply it to the release branch because of the soft freeze.
> But you are free to apply it to the main branch. 
> 
> I can be convinced to apply this to the 2.4 branch by arguments along
> the line of "power user X has been screaming for this feature for
> five years, don't keep it from him any longer!"  

http://bugs.darcs.net/issue121 has three separate requests for the
feature starting from 2006. By coincidence someone else asked about this
on #haskell just a few days ago. I personally have felt the lack
repeatedly in the last few months which was why I did it, but since I'll
just run unstable anyway I don't matter here.

So I think it's worth doing, but I don't feel that strongly.

Ganesh

=============================================================================== 
 Please access the attached hyperlink for an important electronic communications disclaimer: 
 http://www.credit-suisse.com/legal/en/disclaimer_email_ib.html 
 ===============================================================================
msg14042 (view) Author: darcswatch Date: 2011-05-10.17:16:00
This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-6c7840515bc68312dda17d6ee3ac6ebca92b955d
History
Date User Action Args
2009-12-28 22:54:13ganeshcreate
2009-12-28 22:55:55darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-6c7840515bc68312dda17d6ee3ac6ebca92b955d
2009-12-28 22:59:27ganeshsetissues: + wish: amend-record --ask-deps
2010-01-04 14:46:37galbollesetstatus: needs-review -> review-in-progress
assignedto: galbolle
nosy: + galbolle
2010-01-05 12:59:22galbollesetnosy: + tux_rocker
messages: + msg9722
assignedto: galbolle -> tux_rocker
2010-01-05 13:57:20galbollesetstatus: review-in-progress -> accepted-pending-tests
assignedto: tux_rocker -> galbolle
2010-01-05 19:16:24darcswatchsetstatus: accepted-pending-tests -> accepted
messages: + msg9727
2010-01-05 21:57:21ganeshsetmessages: + msg9728
2010-01-06 08:05:16tux_rockersetmessages: + msg9730
2010-01-06 10:30:40ganesh.sittampalamsetnosy: + ganesh.sittampalam
messages: + msg9731
title: Resolve issue121: add --ask-deps support to amend-record -> Resolve issue121: add --ask-deps supportto amend-record
2011-05-10 17:16:00darcswatchsetmessages: + msg14042