darcs

Patch 307 Resolve issue1551: only use unchanged fi...

Title Resolve issue1551: only use unchanged fi...
Superseder Nosy List kowey, mornfall
Related Issues
Status accepted Assigned To
Milestone

Created on 2010-07-18.09:28:17 by kowey, last changed 2011-05-10.20:36:45 by darcswatch. Tracked on DarcsWatch.

Files
File name Status Uploaded Type Edit Remove
resolve-issue1551_-only-use-unchanged-file-content-prompt-in-darcs-send_.dpatch kowey, 2010-07-18.09:28:16 text/x-darcs-patch
resolve-issue1551_-only-use-unchanged-file-content-prompt-in-darcs-send_.dpatch kowey, 2010-07-18.09:33:00 text/x-darcs-patch
resolve-issue1551_-only-use-unchanged-file-content-prompt-in-darcs-send_.dpatch kowey, 2010-10-20.17:39:37 text/x-darcs-patch
unnamed kowey, 2010-07-18.09:28:16
unnamed kowey, 2010-07-18.09:33:00
unnamed kowey, 2010-10-20.17:39:37
See mailing list archives for discussion on individual patches.
Messages
msg11786 (view) Author: kowey Date: 2010-07-18.09:28:16
Am I really the only one who was annoyed by this behaviour?  Reviewer: you may
want to note that nobody else has commented on the ticket I filed for
issue1551, so it's not like there is some sort of consensus on this. I think it's
quite clear, but maybe somebody would disagree?  Anyway, if you think it's also
obviously the right way to behave, then I bet you can just push it in.

This behaviour has annoyed me for a while because I sometimes to use the long
comment editor to look over my patch names and (for example) make sure they fit
under 72 characters.  What tends to happen is that I accidentally hit 'n' and
then have to go record my patch again, redoing my cherry-picking (hmm, there's
another point to be made there)

The way I see it, for darcs send this confirmation makes sense because there is
no way back.  For darcs record and amend-record, however, it does not seem very
useful.

2 patches for repository http://darcs.net/releases/branch-2.5:

Sun Jul 18 10:23:27 BST 2010  Eric Kow <kowey@darcs.net>
  * Resolve issue1551: only use unchanged file content prompt in darcs send.

Sun Jul 18 10:23:34 BST 2010  Eric Kow <kowey@darcs.net>
  * Haddock promptYorn.
Attachments
msg11787 (view) Author: kowey Date: 2010-07-18.09:33:00
2 patches for repository http://darcs.net:

Minor hlint-esque tweak of the issue1551 patch

Sun Jul 18 10:34:48 BST 2010  Eric Kow <kowey@darcs.net>
  * Resolve issue1551: only use unchanged file content prompt in darcs send.

Sun Jul 18 10:23:34 BST 2010  Eric Kow <kowey@darcs.net>
  * Haddock promptYorn.
Attachments
msg11901 (view) Author: mornfall Date: 2010-07-29.19:21:19
Hi Eric,

Eric Kow <bugs@darcs.net> writes:
> Sun Jul 18 10:23:27 BST 2010  Eric Kow <kowey@darcs.net>
>   * Resolve issue1551: only use unchanged file content prompt in darcs send.
this patch is failing the issue1465_ortryrunning.sh test. I don't have
time to investigate why, so please look into that when you have a bit of
time.

> Sun Jul 18 10:23:34 BST 2010  Eric Kow <kowey@darcs.net>
>   * Haddock promptYorn.
I am pushing this one.

Yours,
   Petr.
msg12778 (view) Author: kowey Date: 2010-10-19.23:27:33
I can confirm this really makes 1465 fail on Linux (but not on MacOS X)
Problem seems to be darcs record succeeding where the test wants it to
fail, so I wonder if on Linux it's been "failing" for the wrong reason
(the confirmation prompt) the whole time.
msg12785 (view) Author: kowey Date: 2010-10-20.17:39:37
3 patches for repository http://darcs.net:

Phew, this test failure confused me!  My patch removes the confirmation prompt
from darcs record if you don't make any modifications (*).

The result was that the issue1465 test would fail. Basically issue1465 seems
to be about checking Darcs' fall-through behaviour on editors.

Darcs tries exactly 5 editors in order
1. first of DARCS_EDITOR, DARCSEDITOR, VISUAL, EDITOR that is defined; or vi
2. emacs
3. emacs -nw
4. nano
5. edit

I think what Trent wants in that test is basically to make sure that we only
fall through from one editor (eg. from 1. to 2.) for legimitate reasons. A
legitimate reason would be the editor not existing. An illegitimate reason
would be the user hitting control-C or something (false in his test).

Two things about the test.  First, it tests for darcs record failing, but the
failure seems to come from Darcs's issue1551 follow-up confirmation prompt.
Take the prompt away and the expected failure goes away (hence the test
complaining).  Second, on MacOS X it was passing for the wrong reason (the
env -u error was confused for darcs legimitately failing).

My modifications remove the test for darcs failure while trying to preserve the
core idea of testing the fall-through behaviour.  One glaring omission in this
modification: what's Darcs supposed to do if the user hits ^-C or something?
Right now, it just goes ahead and records the patch. Shouldn't it fail?

(*) Because
    a. confirmation prompts are annoying and
    b. I claim that editing long-comment just to see it in an
        editor is a legitimate workflow
    
    However
    c. we still have to keep it in darcs send because there
    is no last resort

Sun Jul 18 10:34:48 BST 2010  Eric Kow <kowey@darcs.net>
  * Resolve issue1551: only use unchanged file content prompt in darcs send.

Wed Oct 20 15:16:27 BST 2010  Eric Kow <kowey@darcs.net>
  * Improve issue1465 test (portability, tidiness).
  
  - No env -u on MacOS X.
  - No need to clean up directories (new harness)
    Keeping them around makes debugging easier.

Wed Oct 20 18:24:22 BST 2010  Eric Kow <kowey@darcs.net>
  * Make issue1465 test a bit more straightforward.
Attachments
msg12786 (view) Author: darcswatch Date: 2010-10-20.17:48:07
This patch bundle (with 3 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-fa1b4fc56a81b19c05f1cbb3d60431475954c045
msg12881 (view) Author: ganesh Date: 2010-11-02.22:53:56
As far as the issue1551 resolution goes, I'm happy with the code change 
and I agree with the principle. However I think it's worth giving people 
a bit of time to object as it's a behaviour change.

Also, after this patch:

> Wed Oct 20 18:24:22 BST 2010  Eric Kow <kowey@darcs.net>
>   * Make issue1465 test a bit more straightforward.

the issue1465 test stops working on my machine. Let me know if you can't 
reproduce and I'll try to investigate further.
msg13004 (view) Author: galbolle Date: 2010-11-12.08:40:23
for the problem with the issue1465 test, look at patch433. I this patch 
and patch433 can/should go in at the same time.
msg13024 (view) Author: ganesh Date: 2010-11-14.15:06:54
I'll accept this assuming I can verify that patch433 does indeed fix the 
problem with the issue1465 test.
msg13034 (view) Author: darcswatch Date: 2010-11-14.16:37:17
This patch bundle (with 3 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-fa1b4fc56a81b19c05f1cbb3d60431475954c045
msg13035 (view) Author: darcswatch Date: 2010-11-14.16:37:19
This patch bundle (with 2 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-5e0f542cbb84292a79db22ebb854c256fed443b4
msg14148 (view) Author: darcswatch Date: 2011-05-10.19:06:07
This patch bundle (with 3 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-fa1b4fc56a81b19c05f1cbb3d60431475954c045
msg14292 (view) Author: darcswatch Date: 2011-05-10.20:36:45
This patch bundle (with 2 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-5e0f542cbb84292a79db22ebb854c256fed443b4
History
Date User Action Args
2010-07-18 09:28:17koweycreate
2010-07-18 09:29:12darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-0d830512ce9b1ed361b4ce163190bb6f3d970eb6
2010-07-18 09:33:00koweysetfiles: + resolve-issue1551_-only-use-unchanged-file-content-prompt-in-darcs-send_.dpatch, unnamed
messages: + msg11787
2010-07-18 09:34:56darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-0d830512ce9b1ed361b4ce163190bb6f3d970eb6 -> http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-5e0f542cbb84292a79db22ebb854c256fed443b4
2010-07-29 19:21:20mornfallsetstatus: needs-review -> followup-requested
nosy: + mornfall
messages: + msg11901
title: Resolve issue1551: only use unchanged fi... (and 1 more) -> Resolve issue1551: only use unchanged fi...
2010-08-02 15:47:22koweysetassignedto: kowey
2010-10-19 23:27:34koweysetstatus: followup-requested -> followup-in-progress
messages: + msg12778
2010-10-20 17:28:59koweysetstatus: followup-in-progress -> needs-screening
2010-10-20 17:39:38koweysetfiles: + resolve-issue1551_-only-use-unchanged-file-content-prompt-in-darcs-send_.dpatch, unnamed
messages: + msg12785
2010-10-20 17:40:22darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-5e0f542cbb84292a79db22ebb854c256fed443b4 -> http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-fa1b4fc56a81b19c05f1cbb3d60431475954c045
2010-10-20 17:45:52koweysetassignedto: kowey ->
2010-10-20 17:48:07darcswatchsetstatus: needs-screening -> needs-review
messages: + msg12786
2010-11-02 22:53:56ganeshsetstatus: needs-review -> review-in-progress
messages: + msg12881
2010-11-12 08:40:23galbollesetmessages: + msg13004
2010-11-14 15:06:54ganeshsetstatus: review-in-progress -> accepted-pending-tests
messages: + msg13024
2010-11-14 16:37:17darcswatchsetstatus: accepted-pending-tests -> accepted
messages: + msg13034
2010-11-14 16:37:19darcswatchsetmessages: + msg13035
2011-05-10 19:06:07darcswatchsetmessages: + msg14148
2011-05-10 20:06:10darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-fa1b4fc56a81b19c05f1cbb3d60431475954c045 -> http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-0d830512ce9b1ed361b4ce163190bb6f3d970eb6
2011-05-10 20:36:45darcswatchsetmessages: + msg14292