Hi Eric,

if have now implemented an easy version. Could you please have a look at 
it and point out to me what to improve.

Thanks for your help,
  Johannes

Attachments

• resolve-issue1470_-allow-to-amend_record-patch-by-removing-some-hunks.dpatch

Hi Johannes,

Just dashing off a quick reply,

On Sun, Aug 14, 2011 at 16:26:37 +0000, Johannes Weiß wrote:
> if have now implemented an easy version. Could you please have a look at 
> it and point out to me what to improve.

Hey! Working at the Hackathon was fun.

Overall, the patch looks good although I'd be more comfortable perhaps
if another hacker could look at the prims we pass through. 

I'm not sure I understand the changes to the type of addChangesToPatch.
Is that just a cleanup or is it actually important?

Superficial stuff: thanks for fixing that typo! Normally we like these
in separate patches, but it's no big deal :-)

Hope to see you at another hackathon, or even a Darcs sprint!

-- 
Eric Kow <http://erickow.com>

I find this feature useful, unfortunately I can't really review the
patch since I'm not familiar with prims/witnesses and all that stuff.

One remark: we can amend --rollback a patch until it becomes empty.
Also, when trying to amend --rollback an empty patch, we are immediately
presented with the message "You don't want to record anything!" which is
a little weird. Perhaps this should be fixed in another patch.

The implementation looks fine, and I think this is a great feature to 
have. Thanks, and sorry for the delay in fully reviewing it.

Two minor things:

 - I think the feature should be called unrecord, not rollback/amend-
rollback. What it does is closer to unrecord than rollback.

 - It needs a test.

I'll make both these changes (rename and write a test) shortly unless 
there are objections.

BTW the UI glitch when you empty out a recorded patch is also 
reproducible by making the same changes manually, so I don't see it as 
serious.

A general comment about witnesses to other reviewers: in general, if 
something doesn't require an unsafeCoerceP it's probably ok in that 
regard. That's in part the point of that stuff. There are cases where 
you can still screwup, but the main goal of having witnesses is to make 
it harder to do so.

A few more thoughts:

 - The changes get offered in reverse order. I'll switch that to 
forwards order.

 - Should we offer a splitter (allowing for interactive edit) like we do 
for rollback?

 - What should we do with --all?

 - What should we do if a list of files is passed on the command-line?

Some unthought-out reactions:

 - The changes get offered in reverse order. I'll switch that to 
forwards order.

Don't we want it to behave like unrecord would?

 - Should we offer a splitter (allowing for interactive edit) like we do 
for rollback?

I *think* so, on the basis that we (at least I) think that part of what 
makes darcs friendly is few-concepts-broadly-applicable.  So I think as 
a general principle we want to have mechanisms that exist in one context 
exist in all the contexts that they make sense in.

Hope that's reasonable to want (be careful what you wish for?)

 - What should we do with --all?

Ah, so the question here is what happens when somebody types in darcs 
amend-record --unrecord --all?

One option is to treat it as synonymous for darcs unrecord, end of 
story.

Another option is to reject on the grounds that the user surely must not 
have meant --all

A third option more generally is to catch the fact that we would create 
an empty patch and then say something like "Hey, removing this change 
will create an empty patch.  Do you mean to do darcs unrecord instead?"

Am I missing options?

 - What should we do if a list of files is passed on the command-line?

What about it?  Is this a clash between how darcs amend-record and how 
darcs unrecord would behave?

On Fri, Jan 06, 2012 at 08:11:07PM +0000, Eric Kow wrote:
>  - The changes get offered in reverse order. I'll switch that to 
> forwards order.
> 
> Don't we want it to behave like unrecord would?

I don't think so, at least in a given file it would be very confusing to review
chunks in reverse order.

>  - Should we offer a splitter (allowing for interactive edit) like we do 
> for rollback?
> 
> I *think* so, on the basis that we (at least I) think that part of what 
> makes darcs friendly is few-concepts-broadly-applicable.  So I think as 
> a general principle we want to have mechanisms that exist in one context 
> exist in all the contexts that they make sense in.

I agree with the principle, no idea about the implementation.

>  - What should we do with --all?
> 
> Ah, so the question here is what happens when somebody types in darcs 
> amend-record --unrecord --all?
> 
> One option is to treat it as synonymous for darcs unrecord, end of 
> story.

I would vote for this one with a warning, but then emitting a confirmation
might conflict with the idea of --all?  In that case, refusing with an error
message and suggesting to unrecord instead would be sensible.

-- 
Gabriel

Sorry, should have been clearer: the questions all relate to what should 
happen during the prim selection (i.e. hunks etc), not during the 
initial patch selection.

darcs unrecord --all does actually unrecord all patches (back to the 
last clean tag, probably), but that doesn't strike me as very helpful :-
)

Oh, right, duh.  It's not that I was thinking you meant the initial 
patch selection, it's just that I got confused and forgot that 
interactiveness works over both kinds, that unrecord/record select 
different kinds of objects, and that amend-record (like rollback) does 
two phases.

What are the two-phase commands we have?
* amend-record --unrecord: named, prim
* rollback: named, prim (--all says yes to all in both)
* record --ask-deps: prim, named (--all says yes to prims, prompts for 
named)

It could make sense to start out really conservative and go the first 
option, "Use darcs unrecord if you just want to unrecord the whole 
patch."  If we treat it as unrecord, one unfortunate effect is that the 
user loses the name/log in case he didn't actually mean it. I wonder 
what would cause a user to pass in --all.  Maybe the sort of confusion I 
had?  I dunno, maybe we shouldn't go around in circles too much thinking 
about this.  I suggest just doing the conservative thing unless we think 
we have an idea what's best.

Unrecord/obliterate --all (for named patches) is one of those surprising 
things that David was talked into implementing and later found useful.  
I think it may make sense when you have matchers.

--all being useful in combination with other options is a good point. In 
fact it sort of answers the question about lists of files too; it might 
make sense to remove all changes to file X from a patch, so you'd do 
darcs amend --unrecord -a X

Another thought: should we have darcs amend-unrecord as an alias fro 
darcs amend-record --unrecord?

On 8 Jan 2012, at 17:00, Ganesh Sittampalam wrote:
> Another thought: should we have darcs amend-unrecord as an alias fro 
> darcs amend-record --unrecord?

Couldn't hurt.  Might even make sense as *the* UI

-- 
Eric Kow <http://erickow.com>

On 08/01/2012 18:25, Eric Kow wrote:
> 
> On 8 Jan 2012, at 17:00, Ganesh Sittampalam wrote:
>> Another thought: should we have darcs amend-unrecord as an alias fro 
>> darcs amend-record --unrecord?
> 
> Couldn't hurt.  Might even make sense as *the* UI

Anyone know how to actually do this cleanly? :-) We've also discussed
something similar for check --repair, but I can't find any existing
examples in of x being an alias for y --option.

Obviously I can develop such a mechanism, but I want to check I'm not
missing an obvious or already existing answer first.

Ganesh

On 13 Jan 2012, at 07:31, Ganesh Sittampalam wrote:
> 
> Anyone know how to actually do this cleanly? :-) We've also discussed
> something similar for check --repair, but I can't find any existing
> examples in of x being an alias for y --option.


One way: See darcs status in Darcs.Commands.Whatsnew

-- 
Eric Kow <http://erickow.com>

On 13/01/2012 07:41, Eric Kow wrote:
> 
> On 13 Jan 2012, at 07:31, Ganesh Sittampalam wrote:
>>
>> Anyone know how to actually do this cleanly? :-) We've also discussed
>> something similar for check --repair, but I can't find any existing
>> examples in of x being an alias for y --option.
> 
> 
> One way: See darcs status in Darcs.Commands.Whatsnew

Perfect, thanks. I should clearly become more knowledgeable about darcs :-)

Ganesh

On 08/01/2012 18:25, Eric Kow wrote:
> 
> On 8 Jan 2012, at 17:00, Ganesh Sittampalam wrote:
>> Another thought: should we have darcs amend-unrecord as an alias fro 
>> darcs amend-record --unrecord?
> 
> Couldn't hurt.  Might even make sense as *the* UI
> 

Actually, one way this could hurt: 'darcs amend' is no longer a valid
command. I'd have to retrain my fingers!

Ganesh

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/screened.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_screened.html#bundle-4b1f771881d6933de060b0ed55afb20ae50da914

A couple of follow-ups as discussed. I'll self accept fairly soon if no 
comments.

2 patches for repository /home/ganesh/darcs/darcs-temp:

Mon Jan  9 18:09:46 GMT 2012  Ganesh Sittampalam <ganesh@earth.li>
  * rename amend-record's --rollback option to --unrecord

Fri Jan 13 23:36:12 GMT 2012  Ganesh Sittampalam <ganesh@earth.li>
  * fill out options for amend-record --unrecord and add test
   - interactive hunk edit
   - selecting files

Attachments

• patch-preview.txt
• rename-amend_record_s-__rollback-option-to-__unrecord.dpatch
• unnamed

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-4b1f771881d6933de060b0ed55afb20ae50da914