bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Jambunathan K]

I have a tendency to accumulate unregistered files in my vc-dir work
area before I do away with all of them with "git clean -d -f".

It will be convenient if the existing `vc-dir-hide-up-to-date' (and the
"x" binding) is extended so that files in arbitrary state - including
unregistered - could be hidden.

I have posted my patch to emacs-devel.  See following thread:
    http://lists.gnu.org/archive/html/emacs-devel/2012-08/msg00277.html

With my patch, I can accomplish what I want with

    (custom-set-variables
     '(vc-dir-hide-these-states (quote ("unregistered" "up-to-date"))))

Feel free to close this patch once the above patch is suitably dealt
with.

In GNU Emacs 24.1.50.22 (i686-pc-linux-gnu, GTK+ Version 2.20.1)
 of 2012-08-08 on debian-6.05
Bzr revision: 109513 kjambunathan@gmail.com-20120808150232-cvekujrpotm9qlwa
Windowing system distributor `The X.Org Foundation', version 11.0.10707000
Important settings:
  value of $LANG: en_IN
  locale-coding-system: iso-latin-1-unix
  default enable-multibyte-characters: t

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Glenn Morris]

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=6148

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Jambunathan K]

Glenn Morris <rgm@gnu.org> writes:

> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=6148

Quick comments:

1. My patch could make use of completing-read-multiple.  Currently it
   uses completing-read.  So one has to do multiple "C-u x"s.

2. Magnus' changes assumes that states are just elisp symbols.  This may
   not be true.  In case of locked states - designated as USER in
   `vc-state' - the state could actually be a string.

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Stefan Monnier]

> 1. My patch could make use of completing-read-multiple.  Currently it
>    uses completing-read.  So one has to do multiple "C-u x"s.

Maybe a better option is to just have "C-u x" hide all files in the same
state as the file-at-point.  So you still need several "C-u x"s but you
don't need to enter the states, you can just point to them.


        Stefan

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Jambunathan K]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> 1. My patch could make use of completing-read-multiple.  Currently it
>>    uses completing-read.  So one has to do multiple "C-u x"s.
>
> Maybe a better option is to just have "C-u x" hide all files in the same
> state as the file-at-point.  So you still need several "C-u x"s but you
> don't need to enter the states, you can just point to them.

Currently "x" acts buffer-wide.

Your suggestion is to hide the files that are in the same state as the
file at point.  

If there are marked files, then potentially a super-set of marked files
could get hidden.  This "superset" behaviour will surprise the user -
"Why does even un-marked files get hidden?"

I am little bit hesitant to re-work the patch.

>         Stefan

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Stefan Monnier]

> If there are marked files, then potentially a super-set of marked files
> could get hidden.  This "superset" behaviour will surprise the user -
> "Why does even un-marked files get hidden?"

I don't see why it should pay attention to the marks.


        Stefan

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Jambunathan K]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> If there are marked files, then potentially a super-set of marked files
>> could get hidden.  This "superset" behaviour will surprise the user -
>> "Why does even un-marked files get hidden?"
>
> I don't see why it should pay attention to the marks.

I am attaching the modified patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-dir-hide-some-states.patch --]
[-- Type: text/x-diff, Size: 5042 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-08-11 04:46:38 +0000
+++ lisp/ChangeLog	2012-08-12 01:44:26 +0000
@@ -1,3 +1,11 @@
+2012-08-12  Jambunathan K  <kjambunathan@gmail.com>
+
+	* vc/vc-dir.el (vc-dir-hide-these-states): New custom variable.
+	(vc-dir-hide-some-states): New command.
+	(vc-dir-hide-up-to-date): Use `vc-dir-hide-some-states'.
+	(vc-dir-mode-map): Map "x" to `vc-dir-hide-some-states' instead of
+	`vc-dir-hide-up-to-date'.
+
 2012-08-11  Glenn Morris  <rgm@gnu.org>
 
 	* emacs-lisp/copyright.el (copyright-update-directory): Logic fix.

=== modified file 'lisp/vc/vc-dir.el'
--- lisp/vc/vc-dir.el	2012-07-11 23:13:41 +0000
+++ lisp/vc/vc-dir.el	2012-08-12 01:47:36 +0000
@@ -51,6 +51,25 @@
   :type 'hook
   :group 'vc)
 
+(defcustom vc-dir-hide-these-states '("up-to-date")
+  "States hidden by `vc-dir-hide-some-states'."
+  :type '(choice
+	  (const :tag "None")
+	  (set :tag "Choices"
+	       (string :tag "VC State" "added")
+	       (string :tag "VC State" "conflict")
+	       (string :tag "VC State" "edited")
+	       (string :tag "VC State" "ignored")
+	       (string :tag "VC State" "missing")
+	       (string :tag "VC State" "needs-merge")
+	       (string :tag "VC State" "needs-update")
+	       (string :tag "VC State" "removed")
+	       (string :tag "VC State" "unlocked-changes")
+	       (string :tag "VC State" "unregistered")
+	       (string :tag "VC State" "up-to-date")))
+  :group 'vc
+  :version "24.2")
+
 ;; Used to store information for the files displayed in the directory buffer.
 ;; Each item displayed corresponds to one of these defstructs.
 (cl-defstruct (vc-dir-fileinfo
@@ -271,7 +290,7 @@
     (define-key map [down-mouse-3] 'vc-dir-menu)
     (define-key map [mouse-2] 'vc-dir-toggle-mark)
     (define-key map [follow-link] 'mouse-face)
-    (define-key map "x" 'vc-dir-hide-up-to-date)
+    (define-key map "x" 'vc-dir-hide-some-states)
     (define-key map [?\C-k] 'vc-dir-kill-line)
     (define-key map "S" 'vc-dir-search) ;; FIXME: Maybe use A like dired?
     (define-key map "Q" 'vc-dir-query-replace-regexp)
@@ -1106,20 +1125,45 @@
   (interactive "fShow file: ")
   (vc-dir-update (list (list (file-relative-name file) (vc-state file))) (current-buffer)))
 
-(defun vc-dir-hide-up-to-date ()
-  "Hide up-to-date items from display."
-  (interactive)
-  (let ((crt (ewoc-nth vc-ewoc -1))
-	(first (ewoc-nth vc-ewoc 0)))
-    ;; Go over from the last item to the first and remove the
-    ;; up-to-date files and directories with no child files.
-    (while (not (eq crt first))
-      (let* ((data (ewoc-data crt))
-	     (dir (vc-dir-fileinfo->directory data))
-	     (next (ewoc-next vc-ewoc crt))
-	     (prev (ewoc-prev vc-ewoc crt))
-	     ;; ewoc-delete does not work without this...
-	     (inhibit-read-only t))
+(defun vc-dir-hide-some-states (&optional states)
+  "Hide items that are in STATES from display.
+STATES is a list of vc states (see`vc-state') formatted as a
+string.
+
+If STATES is nil, use `vc-dir-hide-these-states' as the default
+value.
+
+In interactive mode, if prefix argument is non-nil or if
+`vc-dir-hide-these-states' is nil, hide items that share the same
+state as file at point."
+  (interactive
+   ;; Interactive use.
+   (list
+    (or (and (not current-prefix-arg) vc-dir-hide-these-states)
+	;; No prefix arg or `vc-dir-hide-these-states' is nil.  State
+	;; to hide is the one at point.
+	(let ((node (ewoc-locate vc-ewoc)))
+	  (unless node (error "No file available"))
+	  (let* ((data (ewoc-data node))
+		 (state (vc-dir-fileinfo->state data)))
+	    (unless state (error "No state at point"))
+	    (list (format "%s" state)))))))
+  ;; Non-interactive use.
+  (unless (called-interactively-p 'any)
+    (setq states (or states vc-dir-hide-these-states)))
+  ;; Hide specified states.
+  (when states
+    (let ((crt (ewoc-nth vc-ewoc -1))
+	  (first (ewoc-nth vc-ewoc 0)))
+      ;; Go over from the last item to the first and remove the
+      ;; up-to-date files and directories with no child files.
+      (while (not (eq crt first))
+	(let* ((data (ewoc-data crt))
+	       (dir (vc-dir-fileinfo->directory data))
+	       (next (ewoc-next vc-ewoc crt))
+	       (prev (ewoc-prev vc-ewoc crt))
+	       ;; ewoc-delete does not work without this...
+	       (inhibit-read-only t))
 	  (when (or
 		 ;; Remove directories with no child files.
 		 (and dir
@@ -1128,10 +1172,15 @@
 		       (not next)
 		       ;; Next item is a directory.
 		       (vc-dir-fileinfo->directory (ewoc-data next))))
-		 ;; Remove files in the up-to-date state.
-		 (eq (vc-dir-fileinfo->state data) 'up-to-date))
+		 ;; Remove files that are to be hidden.
+		 (member (format "%s" (vc-dir-fileinfo->state data)) states))
 	    (ewoc-delete vc-ewoc crt))
-	  (setq crt prev)))))
+	  (setq crt prev))))))
+
+(defun vc-dir-hide-up-to-date ()
+  "Hide up-to-date items from display."
+  (interactive)
+  (vc-dir-hide-some-states '("up-to-date")))
 
 (defun vc-dir-kill-line ()
   "Remove the current line from display."

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Andreas Schwab]

Jambunathan K <kjambunathan@gmail.com> writes:

> +(defcustom vc-dir-hide-these-states '("up-to-date")
> +  "States hidden by `vc-dir-hide-some-states'."
> +  :type '(choice
> +	  (const :tag "None")
> +	  (set :tag "Choices"
> +	       (string :tag "VC State" "added")

Why a string?  Using the symbol would be more natural.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Jambunathan K]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Jambunathan K <kjambunathan@gmail.com> writes:
>
>> +(defcustom vc-dir-hide-these-states '("up-to-date")
>> +  "States hidden by `vc-dir-hide-some-states'."
>> +  :type '(choice
>> +	  (const :tag "None")
>> +	  (set :tag "Choices"
>> +	       (string :tag "VC State" "added")
>
> Why a string?  Using the symbol would be more natural.

There is a note up in the thread, which reads:

,----
| 2. Magnus' changes assumes that states are just elisp symbols.  This
| may not be true.  In case of locked states - designated as USER in
| `vc-state' - the state could actually be a string.
`----

I have gone by what I could make out from the dav backend.

Btw, does it make a big difference?

> Andreas.

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Andreas Schwab]

Jambunathan K <kjambunathan@gmail.com> writes:

> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> Jambunathan K <kjambunathan@gmail.com> writes:
>>
>>> +(defcustom vc-dir-hide-these-states '("up-to-date")
>>> +  "States hidden by `vc-dir-hide-some-states'."
>>> +  :type '(choice
>>> +	  (const :tag "None")
>>> +	  (set :tag "Choices"
>>> +	       (string :tag "VC State" "added")
>>
>> Why a string?  Using the symbol would be more natural.
>
> There is a note up in the thread, which reads:
>
> ,----
> | 2. Magnus' changes assumes that states are just elisp symbols.  This
> | may not be true.  In case of locked states - designated as USER in
> | `vc-state' - the state could actually be a string.
> `----

There is nothing wrong with using a string where the state is a string.
But if the user name happens to match the name of one of the state
symbols it becomes ambigous.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Jambunathan K]

[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Jambunathan K <kjambunathan@gmail.com> writes:
>
>> Andreas Schwab <schwab@linux-m68k.org> writes:
>>
>>> Jambunathan K <kjambunathan@gmail.com> writes:
>>>
>>>> +(defcustom vc-dir-hide-these-states '("up-to-date")
>>>> +  "States hidden by `vc-dir-hide-some-states'."
>>>> +  :type '(choice
>>>> +	  (const :tag "None")
>>>> +	  (set :tag "Choices"
>>>> +	       (string :tag "VC State" "added")
>>>
>>> Why a string?  Using the symbol would be more natural.
>>
>> There is a note up in the thread, which reads:
>>
>> ,----
>> | 2. Magnus' changes assumes that states are just elisp symbols.  This
>> | may not be true.  In case of locked states - designated as USER in
>> | `vc-state' - the state could actually be a string.
>> `----
>
> There is nothing wrong with using a string where the state is a string.
> But if the user name happens to match the name of one of the state
> symbols it becomes ambigous.

Sure, I will name my next kid `conflict' or even better `missing'.  In
the latter case, I will have hard time reporting to police if he ever
goes missing.

Here comes the modified patch.

I think I am hitting a bug with defcustom and I will open a separte
report for it.  When I do, M-x customize-variable RET
vc-hide-these-states RET, I see two "up-to-date"s and "added" goes
missing.

I will let you folks handle the defcustom issue.

,----
| Operate on all settings in this buffer:
|  Revert...   Apply   Apply and Save 
| 
| Hide Vc Dir Hide These States: Value Menu Choices:
| [X] VC State: up-to-date
| [ ] VC State: conflict
| [ ] VC State: edited
| [ ] VC State: ignored
| [ ] VC State: missing
| [ ] VC State: needs-merge
| [ ] VC State: needs-update
| [ ] VC State: removed
| [ ] VC State: unlocked-changes
| [ ] VC State: unregistered
| [ ] VC State: up-to-date
|     State : STANDARD.
|    States hidden by `vc-dir-hide-some-states'.
| Groups: Vc
`----


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: bug#12159.patch --]
[-- Type: text/x-diff, Size: 4895 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-08-11 04:46:38 +0000
+++ lisp/ChangeLog	2012-08-12 01:44:26 +0000
@@ -1,3 +1,11 @@
+2012-08-12  Jambunathan K  <kjambunathan@gmail.com>
+
+	* vc/vc-dir.el (vc-dir-hide-these-states): New custom variable.
+	(vc-dir-hide-some-states): New command.
+	(vc-dir-hide-up-to-date): Use `vc-dir-hide-some-states'.
+	(vc-dir-mode-map): Map "x" to `vc-dir-hide-some-states' instead of
+	`vc-dir-hide-up-to-date'.
+
 2012-08-11  Glenn Morris  <rgm@gnu.org>
 
 	* emacs-lisp/copyright.el (copyright-update-directory): Logic fix.

=== modified file 'lisp/vc/vc-dir.el'
--- lisp/vc/vc-dir.el	2012-07-11 23:13:41 +0000
+++ lisp/vc/vc-dir.el	2012-08-12 10:47:23 +0000
@@ -51,6 +51,25 @@
   :type 'hook
   :group 'vc)
 
+(defcustom vc-dir-hide-these-states '(up-to-date)
+  "States hidden by `vc-dir-hide-some-states'."
+  :type '(choice
+	  (const :tag "None")
+	  (set :tag "Choices"
+	       (symbol :tag "VC State" added)
+	       (symbol :tag "VC State" conflict)
+	       (symbol :tag "VC State" edited)
+	       (symbol :tag "VC State" ignored)
+	       (symbol :tag "VC State" missing)
+	       (symbol :tag "VC State" needs-merge)
+	       (symbol :tag "VC State" needs-update)
+	       (symbol :tag "VC State" removed)
+	       (symbol :tag "VC State" unlocked-changes)
+	       (symbol :tag "VC State" unregistered)
+	       (symbol :tag "VC State" up-to-date)))
+  :group 'vc
+  :version "24.2")
+
 ;; Used to store information for the files displayed in the directory buffer.
 ;; Each item displayed corresponds to one of these defstructs.
 (cl-defstruct (vc-dir-fileinfo
@@ -271,7 +290,7 @@
     (define-key map [down-mouse-3] 'vc-dir-menu)
     (define-key map [mouse-2] 'vc-dir-toggle-mark)
     (define-key map [follow-link] 'mouse-face)
-    (define-key map "x" 'vc-dir-hide-up-to-date)
+    (define-key map "x" 'vc-dir-hide-some-states)
     (define-key map [?\C-k] 'vc-dir-kill-line)
     (define-key map "S" 'vc-dir-search) ;; FIXME: Maybe use A like dired?
     (define-key map "Q" 'vc-dir-query-replace-regexp)
@@ -1106,20 +1125,43 @@
   (interactive "fShow file: ")
   (vc-dir-update (list (list (file-relative-name file) (vc-state file))) (current-buffer)))
 
-(defun vc-dir-hide-up-to-date ()
-  "Hide up-to-date items from display."
-  (interactive)
-  (let ((crt (ewoc-nth vc-ewoc -1))
-	(first (ewoc-nth vc-ewoc 0)))
-    ;; Go over from the last item to the first and remove the
-    ;; up-to-date files and directories with no child files.
-    (while (not (eq crt first))
-      (let* ((data (ewoc-data crt))
-	     (dir (vc-dir-fileinfo->directory data))
-	     (next (ewoc-next vc-ewoc crt))
-	     (prev (ewoc-prev vc-ewoc crt))
-	     ;; ewoc-delete does not work without this...
-	     (inhibit-read-only t))
+(defun vc-dir-hide-some-states (&optional states)
+  "Hide items that are in STATES from display.
+STATES is a list of vc states (see`vc-state').
+
+If STATES is nil, use `vc-dir-hide-these-states' as default.
+
+In interactive mode, if prefix argument is non-nil or if
+`vc-dir-hide-these-states' is nil, hide items that share the same
+state as file at point."
+  (interactive
+   ;; Interactive use.
+   (list
+    (or (and (not current-prefix-arg) vc-dir-hide-these-states)
+	;; Command is prefixed or `vc-dir-hide-these-states' is nil.
+	;; State to hide, is the one at point.
+	(let ((node (ewoc-locate vc-ewoc)))
+	  (unless node (error "No file available"))
+	  (let* ((data (ewoc-data node))
+		 (state (vc-dir-fileinfo->state data)))
+	    (unless state (error "No state at point"))
+	    (list state))))))
+  ;; Non-interactive use.
+  (unless (called-interactively-p 'any)
+    (setq states (or states vc-dir-hide-these-states)))
+  ;; Hide specified states.
+  (when states
+    (let ((crt (ewoc-nth vc-ewoc -1))
+	  (first (ewoc-nth vc-ewoc 0)))
+      ;; Go over from the last item to the first and remove the
+      ;; up-to-date files and directories with no child files.
+      (while (not (eq crt first))
+	(let* ((data (ewoc-data crt))
+	       (dir (vc-dir-fileinfo->directory data))
+	       (next (ewoc-next vc-ewoc crt))
+	       (prev (ewoc-prev vc-ewoc crt))
+	       ;; ewoc-delete does not work without this...
+	       (inhibit-read-only t))
 	  (when (or
 		 ;; Remove directories with no child files.
 		 (and dir
@@ -1129,9 +1171,14 @@
 		       ;; Next item is a directory.
 		       (vc-dir-fileinfo->directory (ewoc-data next))))
 		 ;; Remove files in the up-to-date state.
-		 (eq (vc-dir-fileinfo->state data) 'up-to-date))
+		 (member (vc-dir-fileinfo->state data) states))
 	    (ewoc-delete vc-ewoc crt))
-	  (setq crt prev)))))
+	  (setq crt prev))))))
+
+(defun vc-dir-hide-up-to-date ()
+  "Hide up-to-date items from display."
+  (interactive)
+  (vc-dir-hide-some-states '(up-to-date)))
 
 (defun vc-dir-kill-line ()
   "Remove the current line from display."


[-- Attachment #3: Type: text/plain, Size: 14 bytes --]




> Andreas.

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Andreas Schwab]

Jambunathan K <kjambunathan@gmail.com> writes:

> Sure, I will name my next kid `conflict' or even better `missing'.

<http://xkcd.com/327/>

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Stefan Monnier]

> +	* vc/vc-dir.el (vc-dir-hide-these-states): New custom variable.

Don't bother.  Just always default to up-to-date.

> +(defun vc-dir-hide-some-states (&optional states)

Make it `state' and not a list.

> +  (interactive
> +   ;; Interactive use.

Redundant comment.

> +  ;; Non-interactive use.
> +  (unless (called-interactively-p 'any)
> +    (setq states (or states vc-dir-hide-these-states)))

The test is wrong (it prevents non-interactive use where you specify
the state explicitly).
The above should simply be (unless state (setq state 'up-to-date)).

> +(defun vc-dir-hide-up-to-date ()
> +  "Hide up-to-date items from display."
> +  (interactive)
> +  (vc-dir-hide-some-states '("up-to-date")))
 
Why bother?


        Stefan

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Jambunathan K]

I am not working on this patch any more.

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Jambunathan K]

I wish reviewers provide feedback which is comprehensive right from the
word go.  Let me explain ...

When I submitted my patch it was complete i.e., I did not present it
hunk-by-hunk.  I re-worked the patch based on feedback and I have
demonstrated some seriousness in making the patch acceptable.

Unfortunately, the review process here seems to have gone by "hunk by
hunk" mode.  A small note here, a small note there.  For something as
simple as this patch, why should we have 100 exchanges?

I can't care less if you call my patch a crap or hold an opinion that I
should never enter a programmer's territory.  It is not what I am
talking about.

Reviewers have infinite time to review the patch.  Let them collect
their notes and give a comprehensive list of what they think is
acceptable to them.

I hope I am not placing an un-reasonable demand.  

We are talking of an implicit social contract that reviewers and patch
submitters should adhere to.  Unfortunately, it is only the patch
submitters end of the contract that gets much emphasis.

Jambunathan K.

>> +	* vc/vc-dir.el (vc-dir-hide-these-states): New custom variable.
>
> Don't bother.  Just always default to up-to-date.
>
>> +(defun vc-dir-hide-some-states (&optional states)
>
> Make it `state' and not a list.
>
>> +  (interactive
>> +   ;; Interactive use.
>
> Redundant comment.
>
>> +  ;; Non-interactive use.
>> +  (unless (called-interactively-p 'any)
>> +    (setq states (or states vc-dir-hide-these-states)))
>
> The test is wrong (it prevents non-interactive use where you specify
> the state explicitly).
> The above should simply be (unless state (setq state 'up-to-date)).
>
>> +(defun vc-dir-hide-up-to-date ()
>> +  "Hide up-to-date items from display."
>> +  (interactive)
>> +  (vc-dir-hide-some-states '("up-to-date")))
>  
> Why bother?
>
>
>         Stefan

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Eli Zaretskii]

> From: Jambunathan K <kjambunathan@gmail.com>
> Date: Mon, 13 Aug 2012 00:41:00 +0530
> Cc: 12159@debbugs.gnu.org
> 
> I wish reviewers provide feedback which is comprehensive right from the
> word go.  Let me explain ...
> 
> When I submitted my patch it was complete i.e., I did not present it
> hunk-by-hunk.  I re-worked the patch based on feedback and I have
> demonstrated some seriousness in making the patch acceptable.
> 
> Unfortunately, the review process here seems to have gone by "hunk by
> hunk" mode.  A small note here, a small note there.  For something as
> simple as this patch, why should we have 100 exchanges?
> 
> I can't care less if you call my patch a crap or hold an opinion that I
> should never enter a programmer's territory.  It is not what I am
> talking about.
> 
> Reviewers have infinite time to review the patch.  Let them collect
> their notes and give a comprehensive list of what they think is
> acceptable to them.
> 
> I hope I am not placing an un-reasonable demand.  
> 
> We are talking of an implicit social contract that reviewers and patch
> submitters should adhere to.  Unfortunately, it is only the patch
> submitters end of the contract that gets much emphasis.

I'm sorry you feel this way.  However, after reading the entire
discussion, I see nothing but a reasonable process.  Let me explain.

Your original submission first got a general comment from Stefan
suggesting a different approach.  When you reworked the patch
according to Stefan's suggestions, you got one comment from Andreas
(with whom you exchanged a couple of messages regarding his comment),
and several specific comments from Stefan.  It is entirely reasonable
that two different people comment on different portions of the patch.
Sorry, but I see no "hunk by hunk" here.

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Jambunathan K]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jambunathan K <kjambunathan@gmail.com>
>> Date: Mon, 13 Aug 2012 00:41:00 +0530
>> Cc: 12159@debbugs.gnu.org
>> 
>> I wish reviewers provide feedback which is comprehensive right from the
>> word go.  Let me explain ...
>> 
>> When I submitted my patch it was complete i.e., I did not present it
>> hunk-by-hunk.  I re-worked the patch based on feedback and I have
>> demonstrated some seriousness in making the patch acceptable.
>> 
>> Unfortunately, the review process here seems to have gone by "hunk by
>> hunk" mode.  A small note here, a small note there.  For something as
>> simple as this patch, why should we have 100 exchanges?
>> 
>> I can't care less if you call my patch a crap or hold an opinion that I
>> should never enter a programmer's territory.  It is not what I am
>> talking about.
>> 
>> Reviewers have infinite time to review the patch.  Let them collect
>> their notes and give a comprehensive list of what they think is
>> acceptable to them.
>> 
>> I hope I am not placing an un-reasonable demand.  
>> 
>> We are talking of an implicit social contract that reviewers and patch
>> submitters should adhere to.  Unfortunately, it is only the patch
>> submitters end of the contract that gets much emphasis.
>
> I'm sorry you feel this way.  However, after reading the entire
> discussion, I see nothing but a reasonable process.  Let me explain.

I really feel frustrated.  

More efforts have gone in the patch than what the patch actually shows.

I switched from Windows XP to Debian.  I have downloaded 3 ISOs.  I had
to get my 3G-Modem working.  I have to learn Bazaar and put up with the
"stream of consciousness" style that the two articles in Emacswiki
adopts.  (I remember my troubles making even limited progress with
Ulysess, Portrait of artist as an young man or the Sound and Fury.)

,----
|    http://www.emacswiki.org/emacs/BzrForEmacsDevs
|    http://www.emacswiki.org/emacs/BzrForEmacsCasualDevs
`----

More importantly, the persistent merge conflicts with the Changelog that
I created.  (I have grown wiser now.)

> Your original submission first got a general comment from Stefan
> suggesting a different approach.  

Considering that I have submitted a patch, "general comment" could have
been avoided in first place.

> When you reworked the patch according to Stefan's suggestions, you got
> one comment from Andreas (with whom you exchanged a couple of messages
> regarding his comment), and several specific comments from Stefan.  It
> is entirely reasonable that two different people comment on different
> portions of the patch.

> Sorry, but I see no "hunk by hunk" here.

Ah! programmers.  I am trying to communicate how I feel.  Don't apply
logic in human affairs.

Let me offer a branch of olive: 

Have I heard what all I have to hear on this patch.  If yes, I will make
one more attempt at re-submitting it.

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Bastien]

Jambunathan K <kjambunathan@gmail.com> writes:

> (I remember my troubles making even limited progress with
> Ulysess, Portrait of artist as an young man or the Sound and Fury.)

Parsing these great novels is like fixing dependancies in a messy
library: you constantly need to zoom in and out, switch from the local
evaluation of the meaning to the global interpretation context, try to
synchronize your flow of consciousness to the narrative pace, while
enjoying the whole experience.

Perhaps the emacswiki style is just training for more sweaty days :)

-- 
 Bastien

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Stefan Monnier]

I'm really sorry you feel so frustrated.

All the process (from my point of view) is trying to get the patch to be
as simple and clean as it can be.  First by changing the actual
behavior, then by getting the details of the code right (hence my last
hunk-by-hunk comments).

This last message should have admittedly started with encouraging
words rather than dryly jumping into the nitpicking without even warning
that these are nitpicks.

Hopefully the end result should be a patch of barely 10 lines including
context, so while you may find it frustrating to go through this ordeal
for such a tiny change, I see it as a great success to bring down the
original request to such a simple change.

> Reviewers have infinite time to review the patch.

I don't know of any reviewer with infinite time, but if you find one,
please send him up here, we'd love to have one of those.


        Stefan


>>>>> "Jambunathan" == Jambunathan K <kjambunathan@gmail.com> writes:

> I wish reviewers provide feedback which is comprehensive right from the
> word go.  Let me explain ...

> When I submitted my patch it was complete i.e., I did not present it
> hunk-by-hunk.  I re-worked the patch based on feedback and I have
> demonstrated some seriousness in making the patch acceptable.

> Unfortunately, the review process here seems to have gone by "hunk by
> hunk" mode.  A small note here, a small note there.  For something as
> simple as this patch, why should we have 100 exchanges?

> I can't care less if you call my patch a crap or hold an opinion that I
> should never enter a programmer's territory.  It is not what I am
> talking about.

> Reviewers have infinite time to review the patch.  Let them collect
> their notes and give a comprehensive list of what they think is
> acceptable to them.

> I hope I am not placing an un-reasonable demand.  

> We are talking of an implicit social contract that reviewers and patch
> submitters should adhere to.  Unfortunately, it is only the patch
> submitters end of the contract that gets much emphasis.

> Jambunathan K.

>>> +	* vc/vc-dir.el (vc-dir-hide-these-states): New custom variable.
>> 
>> Don't bother.  Just always default to up-to-date.
>> 
>>> +(defun vc-dir-hide-some-states (&optional states)
>> 
>> Make it `state' and not a list.
>> 
>>> +  (interactive
>>> +   ;; Interactive use.
>> 
>> Redundant comment.
>> 
>>> +  ;; Non-interactive use.
>>> +  (unless (called-interactively-p 'any)
>>> +    (setq states (or states vc-dir-hide-these-states)))
>> 
>> The test is wrong (it prevents non-interactive use where you specify
>> the state explicitly).
>> The above should simply be (unless state (setq state 'up-to-date)).
>> 
>>> +(defun vc-dir-hide-up-to-date ()
>>> +  "Hide up-to-date items from display."
>>> +  (interactive)
>>> +  (vc-dir-hide-some-states '("up-to-date")))
>> 
>> Why bother?
>> 
>> 
>> Stefan

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Jambunathan K]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]


FWIW, I am attaching two patches.  If you want any changes please do it
yourself.

patch-1 :: There is only one command 'x' - which hides state at point.
patch-2 :: `x' hides up-to-date and `C-u x' hides state at point.

Personally, I will go with patch 1.  It is simpler.  No prefix key is
used.

Btw, reviewer who takes infinite time to review could either be a
perfectionist or a procrastinator :-).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: approach-1.patch --]
[-- Type: text/x-diff, Size: 3968 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-08-12 22:52:33 +0000
+++ lisp/ChangeLog	2012-08-13 17:26:55 +0000
@@ -1,3 +1,12 @@
+2012-08-13  Jambunathan K  <kjambunathan@gmail.com>
+
+	* vc/vc-dir.el (vc-dir-hide-up-to-date): Remove it.
+	(vc-dir-hide-state): New command.
+	(vc-dir-mode-map): Map key "x" to `vc-dir-hide-state' instead of
+	`vc-dir-hide-up-to-date'.
+	(vc-dir-menu-map): Use `vc-dir-hide-state' instead of
+	`vc-dir-hide-up-to-date'
+
 2012-08-12  Stefan Monnier  <monnier@iro.umontreal.ca>
 
 	* subr.el (internal--before-with-seleted-window)

=== modified file 'lisp/vc/vc-dir.el'
--- lisp/vc/vc-dir.el	2012-07-11 23:13:41 +0000
+++ lisp/vc/vc-dir.el	2012-08-13 17:32:12 +0000
@@ -123,9 +123,9 @@
       '(menu-item "Refresh" revert-buffer
 		  :enable (not (vc-dir-busy))
 		  :help "Refresh the contents of the directory buffer"))
-    (define-key map [remup]
-      '(menu-item "Hide Up-to-date" vc-dir-hide-up-to-date
-		  :help "Hide up-to-date items from display"))
+    (define-key map [hide]
+      '(menu-item "Hide current state" vc-dir-hide-state
+		  :help "Hide items share current state"))
     ;; Movement.
     (define-key map [sepmv] '("--"))
     (define-key map [next-line]
@@ -271,7 +271,7 @@
     (define-key map [down-mouse-3] 'vc-dir-menu)
     (define-key map [mouse-2] 'vc-dir-toggle-mark)
     (define-key map [follow-link] 'mouse-face)
-    (define-key map "x" 'vc-dir-hide-up-to-date)
+    (define-key map "x" 'vc-dir-hide-state)
     (define-key map [?\C-k] 'vc-dir-kill-line)
     (define-key map "S" 'vc-dir-search) ;; FIXME: Maybe use A like dired?
     (define-key map "Q" 'vc-dir-query-replace-regexp)
@@ -1106,20 +1106,30 @@
   (interactive "fShow file: ")
   (vc-dir-update (list (list (file-relative-name file) (vc-state file))) (current-buffer)))
 
-(defun vc-dir-hide-up-to-date ()
-  "Hide up-to-date items from display."
-  (interactive)
-  (let ((crt (ewoc-nth vc-ewoc -1))
-	(first (ewoc-nth vc-ewoc 0)))
-    ;; Go over from the last item to the first and remove the
-    ;; up-to-date files and directories with no child files.
-    (while (not (eq crt first))
-      (let* ((data (ewoc-data crt))
-	     (dir (vc-dir-fileinfo->directory data))
-	     (next (ewoc-next vc-ewoc crt))
-	     (prev (ewoc-prev vc-ewoc crt))
-	     ;; ewoc-delete does not work without this...
-	     (inhibit-read-only t))
+(defun vc-dir-hide-state (state)
+  "Hide items that are in STATE from display.
+See `vc-state' for valid values of STATE.
+
+Interactively, set STATE to state of item at point."
+  (interactive (list
+		;; Infer STATE from point.  Complain otherwise.
+		(let ((node (ewoc-locate vc-ewoc)))
+		  (unless node (error "No file available"))
+		  (or (vc-dir-fileinfo->state (ewoc-data node))
+		      (error "No state at point")))))
+  (when state
+    (message "Hiding items in state \"%s\"" state)
+    (let ((crt (ewoc-nth vc-ewoc -1))
+	  (first (ewoc-nth vc-ewoc 0)))
+      ;; Go over from the last item to the first and remove the
+      ;; up-to-date files and directories with no child files.
+      (while (not (eq crt first))
+	(let* ((data (ewoc-data crt))
+	       (dir (vc-dir-fileinfo->directory data))
+	       (next (ewoc-next vc-ewoc crt))
+	       (prev (ewoc-prev vc-ewoc crt))
+	       ;; ewoc-delete does not work without this...
+	       (inhibit-read-only t))
 	  (when (or
 		 ;; Remove directories with no child files.
 		 (and dir
@@ -1128,10 +1138,11 @@
 		       (not next)
 		       ;; Next item is a directory.
 		       (vc-dir-fileinfo->directory (ewoc-data next))))
-		 ;; Remove files in the up-to-date state.
-		 (eq (vc-dir-fileinfo->state data) 'up-to-date))
+		 ;; Remove files in specified STATE.  STATE can be a
+		 ;; symbol or a user-name.
+		 (equal (vc-dir-fileinfo->state data) state))
 	    (ewoc-delete vc-ewoc crt))
-	  (setq crt prev)))))
+	  (setq crt prev))))))
 
 (defun vc-dir-kill-line ()
   "Remove the current line from display."


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: approach-2.patch --]
[-- Type: text/x-diff, Size: 2721 bytes --]

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-08-12 22:52:33 +0000
+++ lisp/ChangeLog	2012-08-13 17:16:15 +0000
@@ -1,3 +1,8 @@
+2012-08-13  Jambunathan K  <kjambunathan@gmail.com>
+
+	* vc/vc-dir.el (vc-dir-hide-state): New command
+	(vc-dir-hide-up-to-date): Route it to `vc-dir-hide-state'.
+
 2012-08-12  Stefan Monnier  <monnier@iro.umontreal.ca>
 
 	* subr.el (internal--before-with-seleted-window)

=== modified file 'lisp/vc/vc-dir.el'
--- lisp/vc/vc-dir.el	2012-07-11 23:13:41 +0000
+++ lisp/vc/vc-dir.el	2012-08-13 17:12:16 +0000
@@ -1106,9 +1106,22 @@
   (interactive "fShow file: ")
   (vc-dir-update (list (list (file-relative-name file) (vc-state file))) (current-buffer)))
 
-(defun vc-dir-hide-up-to-date ()
-  "Hide up-to-date items from display."
-  (interactive)
+(defun vc-dir-hide-state (&optional state)
+  "Hide items that are in STATE from display.
+See `vc-state' for valid values of STATE.
+
+If STATE is nil, default it to up-to-date.
+
+Interactively, if `current-prefix-arg' is non-nil, set STATE to
+state of item at point.  Otherwise, set STATE to up-to-date."
+  (interactive (list
+		(and current-prefix-arg
+		     ;; Command is prefixed.  Infer STATE from point.
+		     (let ((node (ewoc-locate vc-ewoc)))
+		       (and node (vc-dir-fileinfo->state (ewoc-data node)))))))
+  ;; If STATE is un-specified, use up-to-date.
+  (setq state (or state 'up-to-date))
+  (message "Hiding items in state \"%s\"" state)
   (let ((crt (ewoc-nth vc-ewoc -1))
 	(first (ewoc-nth vc-ewoc 0)))
     ;; Go over from the last item to the first and remove the
@@ -1120,18 +1133,21 @@
 	     (prev (ewoc-prev vc-ewoc crt))
 	     ;; ewoc-delete does not work without this...
 	     (inhibit-read-only t))
-	  (when (or
-		 ;; Remove directories with no child files.
-		 (and dir
-		      (or
-		       ;; Nothing follows this directory.
-		       (not next)
-		       ;; Next item is a directory.
-		       (vc-dir-fileinfo->directory (ewoc-data next))))
-		 ;; Remove files in the up-to-date state.
-		 (eq (vc-dir-fileinfo->state data) 'up-to-date))
-	    (ewoc-delete vc-ewoc crt))
-	  (setq crt prev)))))
+	(when (or
+	       ;; Remove directories with no child files.
+	       (and dir
+		    (or
+		     ;; Nothing follows this directory.
+		     (not next)
+		     ;; Next item is a directory.
+		     (vc-dir-fileinfo->directory (ewoc-data next))))
+	       ;; Remove files in specified STATE.  STATE can be a
+	       ;; symbol or a user-name.
+	       (equal (vc-dir-fileinfo->state data) state))
+	  (ewoc-delete vc-ewoc crt))
+	(setq crt prev)))))
+
+(defalias 'vc-dir-hide-up-to-date 'vc-dir-hide-state)
 
 (defun vc-dir-kill-line ()
   "Remove the current line from display."


[-- Attachment #4: Type: text/plain, Size: 782 bytes --]


>> +	* vc/vc-dir.el (vc-dir-hide-these-states): New custom variable.
>
> Don't bother.  Just always default to up-to-date.
>
>> +(defun vc-dir-hide-some-states (&optional states)
>
> Make it `state' and not a list.
>
>> +  (interactive
>> +   ;; Interactive use.
>
> Redundant comment.
>
>> +  ;; Non-interactive use.
>> +  (unless (called-interactively-p 'any)
>> +    (setq states (or states vc-dir-hide-these-states)))
>
> The test is wrong (it prevents non-interactive use where you specify
> the state explicitly).
> The above should simply be (unless state (setq state 'up-to-date)).
>
>> +(defun vc-dir-hide-up-to-date ()
>> +  "Hide up-to-date items from display."
>> +  (interactive)
>> +  (vc-dir-hide-some-states '("up-to-date")))
>  
> Why bother?
>
>
>         Stefan

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Stefan Monnier]

> patch-1 :: There is only one command 'x' - which hides state at point.
> patch-2 :: `x' hides up-to-date and `C-u x' hides state at point.

Thanks.

> Personally, I will go with patch 1.  It is simpler.  No prefix key is
> used.

I can agree that patch 1 is simpler and might be preferable, but given
the pre-existence of `x', I'd rather preserve its behavior.
Furthermore, I do use `x' fairly often and usually without paying much
attention to where I am.

So, I installed patch 2.

> Btw, reviewer who takes infinite time to review could either be a
> perfectionist or a procrastinator :-).

Indeed, from the submitter's point of view, they're indistinguishable.
Usually, if I'm the reviewer and the review isn't coming back quickly
... well let's just say it's not because I'm a perfectionist.


        Stefan

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Bastien]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> ... well let's just say it's not because I'm a perfectionist.

Hopefully enough, even perfect procrastinators can get 
some stuff done:

  http://structuredprocrastination.com

-- 
 Bastien

bug#12159: 24.1.50; vc-dir: Need a way to hide unregistered files

[Jambunathan K]

Glenn Morris <rgm@gnu.org> writes:

> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=6148

Please mark both these bugs as fixed in bzr builds.