GNU bug report logs - #7350
24.0.50; make vc-deduce-backend smarter

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 7350 in the body.
You can then email your comments to 7350 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Bob Rogers <rogers <at> rgrjr.dyndns.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.0.50; make vc-deduce-backend smarter
Date: Sat, 6 Nov 2010 18:18:08 -0400

   I notice that vc-root-diff only works if the current buffer is
visiting a version-controlled file, and in certain other buffer modes.
In particular, it works in dired-mode, where it uses the
default-directory, but not in shell-mode, which is not one of the
explicit special cases.  ISTM that using default-directory would make a
better default case, since that ought to DTRT in the majority of cases.
Indeed, one might be able to get rid of some of the other
vc-deduce-backend cases, since most such modes seem to need to set up
the right default-directory anyway.

					-- Bob Rogers
					   http://www.rgrjr.com/

------------------------------------------------------------------------
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 665dafb..ddeb546 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -920,9 +920,9 @@ Within directories, only files already under version control are noticed."
   (cond ((derived-mode-p 'vc-dir-mode)   vc-dir-backend)
 	((derived-mode-p 'log-view-mode) log-view-vc-backend)
 	((derived-mode-p 'diff-mode)     diff-vc-backend)
-	((derived-mode-p 'dired-mode)
-	 (vc-responsible-backend default-directory))
-	(vc-mode (vc-backend buffer-file-name))))
+	(vc-mode (vc-backend buffer-file-name))
+	(default-directory
+	 (vc-responsible-backend default-directory))))
 
 (declare-function vc-dir-current-file "vc-dir" ())
 (declare-function vc-dir-deduce-fileset "vc-dir" (&optional state-model-only-files))

Message #8 received at 7350 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Bob Rogers <rogers <at> rgrjr.dyndns.org>
Cc: 7350 <at> debbugs.gnu.org
Subject: Re: bug#7350: 24.0.50; make vc-deduce-backend smarter
Date: Sun, 07 Nov 2010 15:03:16 -0500

>    I notice that vc-root-diff only works if the current buffer is
> visiting a version-controlled file, and in certain other buffer modes.
> In particular, it works in dired-mode, where it uses the
> default-directory, but not in shell-mode, which is not one of the
> explicit special cases.

Could you give an example use-case where you'd want vc-deduce-backend to
be run in a shell-mode buffer?


        Stefan

Message #11 received at 7350 <at> debbugs.gnu.org (full text, mbox):

From: Bob Rogers <rogers <at> rgrjr.dyndns.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 7350 <at> debbugs.gnu.org
Subject: Re: bug#7350: 24.0.50; make vc-deduce-backend smarter
Date: Sun, 7 Nov 2010 16:41:08 -0500

   From: Stefan Monnier <monnier <at> iro.umontreal.ca>
   Date: Sun, 07 Nov 2010 15:03:16 -0500

   >    I notice that vc-root-diff only works if the current buffer is
   > visiting a version-controlled file, and in certain other buffer modes.
   > In particular, it works in dired-mode, where it uses the
   > default-directory, but not in shell-mode, which is not one of the
   > explicit special cases.

   Could you give an example use-case where you'd want vc-deduce-backend to
   be run in a shell-mode buffer?

	   Stefan

I have gotten into the habit of using a shell buffer to disambiguate
which repo I want to use for general VC commands like vc-root-diff and
vc-dir.  Since I bind "shell" to f8, it is often faster to type "f8 C-x
v d RET" than to supply an explicit pathname to vc-dir.  (I'm often in
the right shell buffer already, having just typed "make test".)  Since
vc-root-diff doesn't take a pathname arg, I have to do something
explicit to get into the right tree anyway.  So it makes sense to me
that vc-root-diff should work like vc-dir in a non-VC buffer.  There is
already an exception for dired-mode; why not generalize?

   In fact, this is something of a regression from Emacs 22.x, where
"C-u C-x v = . RET RET RET" would do the equivalent of

	(vc-version-diff (expand-file-name ".") nil nil)

which is nearly vc-root-diff, regardless of buffer mode.  This no longer
works in the brave new world of filesets, so I had to revise my
shorthand command to turn "." into a fileset.  I was hoping that
vc-root-diff (which I only stumbled over recently) would serve as a
replacement, but it doesn't quite match my workflow because of this
insistence on being in a VC buffer.

   As an aside, I often think that VC doesn't have a good enough notion
of "current file set;" there are other times when it doesn't seem to
DTRT.  (Except for the change in vc-diff mentioned in the previous
paragraph, I can't recall any examples off the top of my head).  But so
far I haven't had any ideas.

   In short, I don't have a killer use case -- I could just keep on
using my own command for this -- but on the other hand, I don't see the
need for limiting commands like vc-root-diff in this way.  If you think
that this is the wrong place to make this change, then the alternative
patch below makes the same change to vc-root-diff directly.  (And I'll
let you know if I find any better use cases. ;-)

					-- Bob

------------------------------------------------------------------------
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 665dafb..5c7fa81 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1682,7 +1682,9 @@ saving the buffer."
       ;; that's not what we want here, we want the diff for the VC root dir.
       (call-interactively 'vc-version-diff)
     (when buffer-file-name (vc-buffer-sync not-urgent))
-    (let ((backend (vc-deduce-backend))
+    (let ((backend (or (vc-deduce-backend)
+		       (and default-directory
+			    (vc-responsible-backend default-directory))))
 	  rootdir working-revision)
       (unless backend
 	(error "Buffer is not version controlled"))

Message #14 received at 7350 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Bob Rogers <rogers <at> rgrjr.dyndns.org>
Cc: 7350 <at> debbugs.gnu.org
Subject: Re: bug#7350: 24.0.50; make vc-deduce-backend smarter
Date: Mon, 08 Nov 2010 12:47:49 -0500

>> I notice that vc-root-diff only works if the current buffer is
>> visiting a version-controlled file, and in certain other buffer modes.
>> In particular, it works in dired-mode, where it uses the
>> default-directory, but not in shell-mode, which is not one of the
>> explicit special cases.

>    Could you give an example use-case where you'd want vc-deduce-backend to
>    be run in a shell-mode buffer?

> I have gotten into the habit of using a shell buffer to disambiguate
> which repo I want to use for general VC commands like vc-root-diff and
> vc-dir.

Hmm... I use a VC-Dir buffer for that ;-)

> Since I bind "shell" to f8, it is often faster to type "f8 C-x
> v d RET" than to supply an explicit pathname to vc-dir.  (I'm often in
> the right shell buffer already, having just typed "make test".)

I don't follow: if you're already in the right shell buffer, then f8
won't do anything.

> Since vc-root-diff doesn't take a pathname arg, I have to do something
> explicit to get into the right tree anyway.  So it makes sense to me
> that vc-root-diff should work like vc-dir in a non-VC buffer.
> There is already an exception for dired-mode; why not generalize?

It can definitely be generalized, but I'd rather stick to buffers where
there's a clear association with a particular file or directory.
E.g. *Help* buffers aren't good candidates.

shell-mode doesn't sound like a bad candidate, actually.  The only
problem I see with it is that a shell buffer's default-directory is
easily out-of-sync with the underlying process's own notion of cwd.

Does the patch below solve your immediate problem?

>    In fact, this is something of a regression from Emacs 22.x, where
> "C-u C-x v = . RET RET RET" would do the equivalent of

> 	(vc-version-diff (expand-file-name ".") nil nil)

> which is nearly vc-root-diff, regardless of buffer mode.  This no longer
> works in the brave new world of filesets,

This was the result of a trade-off (get rid of one RET since it's
almost never used).  But I guess we could/should prompt the user for
a file/dir rather than signal "File is not under version control" or
some such error.


        Stefan

        
=== modified file 'lisp/vc/vc.el'
--- lisp/vc/vc.el	2010-10-05 18:47:39 +0000
+++ lisp/vc/vc.el	2010-11-08 16:18:04 +0000
@@ -921,7 +921,7 @@
   (cond ((derived-mode-p 'vc-dir-mode)   vc-dir-backend)
 	((derived-mode-p 'log-view-mode) log-view-vc-backend)
 	((derived-mode-p 'diff-mode)     diff-vc-backend)
-	((derived-mode-p 'dired-mode)
+	((derived-mode-p 'dired-mode 'shell-mode)
 	 (vc-responsible-backend default-directory))
 	(vc-mode (vc-backend buffer-file-name))))
 

Message #17 received at 7350 <at> debbugs.gnu.org (full text, mbox):

From: Bob Rogers <rogers <at> rgrjr.dyndns.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 7350 <at> debbugs.gnu.org
Subject: Re: bug#7350: 24.0.50; make vc-deduce-backend smarter
Date: Mon, 8 Nov 2010 16:05:43 -0500

   From: Stefan Monnier <monnier <at> iro.umontreal.ca>
   Date: Mon, 08 Nov 2010 12:47:49 -0500

   >    Could you give an example use-case where you'd want vc-deduce-backend to
   >    be run in a shell-mode buffer?

   > I have gotten into the habit of using a shell buffer to disambiguate
   > which repo I want to use for general VC commands like vc-root-diff and
   > vc-dir.

   Hmm... I use a VC-Dir buffer for that ;-)

Me too -- when I'm already there.  But if not, as I've said, it's
usually faster for me to go through the shell buffer.  Besides, it also
avoids the annoying (and often unneeded) refresh from re-invoking
vc-dir.

   > Since I bind "shell" to f8, it is often faster to type "f8 C-x
   > v d RET" than to supply an explicit pathname to vc-dir.  (I'm often in
   > the right shell buffer already, having just typed "make test".)

   I don't follow: if you're already in the right shell buffer, then f8
   won't do anything.

Yes, and that saves me a keystroke.  The point being that I frequently
find myself doing VC commands from the shell buffer anyway.

   > Since vc-root-diff doesn't take a pathname arg, I have to do something
   > explicit to get into the right tree anyway.  So it makes sense to me
   > that vc-root-diff should work like vc-dir in a non-VC buffer.
   > There is already an exception for dired-mode; why not generalize?

   It can definitely be generalized, but I'd rather stick to buffers where
   there's a clear association with a particular file or directory.
   E.g. *Help* buffers aren't good candidates.

Fair enough.  Seems odd that default-directory is not nil for these.

   shell-mode doesn't sound like a bad candidate, actually.  The only
   problem I see with it is that a shell buffer's default-directory is
   easily out-of-sync with the underlying process's own notion of cwd.

In my experience, it's not too bad.  I do find myself using
shell-resync-dirs now and then, but it's almost always after exiting a
subshell, and I've trained myself to resync when needed.  (Maybe the
shell-dirtrack thing should resync automatically when it sees "exit"?)

   Does the patch below solve your immediate problem?

Works for me.  It doesn't cover the cases where I'm looking at a
generated file, or test output, but (in my workflow anyway) those cases
are rarer.

   >    In fact, this is something of a regression from Emacs 22.x, where
   > "C-u C-x v = . RET RET RET" would do the equivalent of

   > 	(vc-version-diff (expand-file-name ".") nil nil)

   > which is nearly vc-root-diff, regardless of buffer mode.  This no longer
   > works in the brave new world of filesets,

   This was the result of a trade-off (get rid of one RET since it's
   almost never used).  But I guess we could/should prompt the user for
   a file/dir rather than signal "File is not under version control" or
   some such error.

	   Stefan

That would be much nicer, especially since the underlying functionality
(i.e. support for vc-diff of directories) still works.

					-- Bob

P.S.  I mailed the signed copyright assignment today, so FSF should have
it tomorrow.

Message #20 received at 7350 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Bob Rogers <rogers <at> rgrjr.dyndns.org>
Cc: 7350 <at> debbugs.gnu.org
Subject: Re: bug#7350: 24.0.50; make vc-deduce-backend smarter
Date: Fri, 12 Nov 2010 08:48:30 -0500

>    It can definitely be generalized, but I'd rather stick to buffers where
>    there's a clear association with a particular file or directory.
>    E.g. *Help* buffers aren't good candidates.
> Fair enough.  Seems odd that default-directory is not nil for these.

There are all kinds of reasons why we don't like default-directory to be nil.

> Works for me.  It doesn't cover the cases where I'm looking at a
> generated file, or test output, but (in my workflow anyway) those cases
> are rarer.

I've installed the patch into the trunk (i.e. not for Emacs-23.3 but
Emacs-24) and added compilation-mode to shell-mode, since it's also
clearly linked to a particular location in the file-system.

>    This was the result of a trade-off (get rid of one RET since it's
>    almost never used).  But I guess we could/should prompt the user for
>    a file/dir rather than signal "File is not under version control" or
>    some such error.

> That would be much nicer, especially since the underlying functionality
> (i.e. support for vc-diff of directories) still works.

Patch welcome.

> P.S.  I mailed the signed copyright assignment today, so FSF should have
> it tomorrow.

Great (tho, OT1H I can't remember for which patch this is, and OTOH the
FSF itself usually takes a little while to process those mail).


        Stefan

Message #23 received at 7350 <at> debbugs.gnu.org (full text, mbox):

From: Bob Rogers <rogers <at> rgrjr.dyndns.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 7350 <at> debbugs.gnu.org
Subject: Re: bug#7350: 24.0.50; make vc-deduce-backend smarter
Date: Sun, 14 Nov 2010 18:21:34 -0500

   From: Stefan Monnier <monnier <at> iro.umontreal.ca>
   Date: Fri, 12 Nov 2010 08:48:30 -0500

   >    It can definitely be generalized, but I'd rather stick to buffers where
   >    there's a clear association with a particular file or directory.
   >    E.g. *Help* buffers aren't good candidates.
   > Fair enough.  Seems odd that default-directory is not nil for these.

   There are all kinds of reasons why we don't like default-directory to
   be nil.

   > Works for me.  It doesn't cover the cases where I'm looking at a
   > generated file, or test output, but (in my workflow anyway) those cases
   > are rarer.

   I've installed the patch into the trunk (i.e. not for Emacs-23.3 but
   Emacs-24) and added compilation-mode to shell-mode, since it's also
   clearly linked to a particular location in the file-system.

Great; thank you.

   >    This was the result of a trade-off (get rid of one RET since it's
   >    almost never used).  But I guess we could/should prompt the user for
   >    a file/dir rather than signal "File is not under version control" or
   >    some such error.

   > That would be much nicer, especially since the underlying functionality
   > (i.e. support for vc-diff of directories) still works.

   Patch welcome.

Hmm.  TRT would be to figure this out in the "interactive" form, so that
repeat-complex-command works, but that would change the args to vc-diff.
How big a change are you willing to contemplate?

   > P.S.  I mailed the signed copyright assignment today, so FSF should
   > have it tomorrow.

   Great (tho, OT1H I can't remember for which patch this is, and OTOH the
   FSF itself usually takes a little while to process those mail).

	   Stefan

NP; it was on my mind, so I just thought I'd mention it.

					-- Bob

Message #26 received at 7350 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Bob Rogers <rogers <at> rgrjr.dyndns.org>
Cc: 7350 <at> debbugs.gnu.org
Subject: Re: bug#7350: 24.0.50; make vc-deduce-backend smarter
Date: Mon, 15 Nov 2010 11:05:11 -0500

> Hmm.  TRT would be to figure this out in the "interactive" form, so that
> repeat-complex-command works, but that would change the args to vc-diff.
> How big a change are you willing to contemplate?

A largish patch is not a problem in and of itself (except for copyright
reasons, but once you've signed the paperwork it's a non-issue).  So the
only reason to reject such a change would be if it made the code
conceptually more tricky/complex.  From the sound of it, it would make
it actually more regular, more in line with the usual way commands work
in Emacs, so it seems OK.
Of course, there is also the risk of introducing bugs/incompatibilities,
so try and make extra sure you take into account all callers.


        Stefan

Message #29 received at 7350 <at> debbugs.gnu.org (full text, mbox):

From: Bob Rogers <rogers <at> rgrjr.dyndns.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 7350 <at> debbugs.gnu.org
Subject: Re: bug#7350: 24.0.50; make vc-deduce-backend smarter
Date: Tue, 16 Nov 2010 23:43:48 -0500

   From: Stefan Monnier <monnier <at> iro.umontreal.ca>
   Date: Mon, 15 Nov 2010 11:05:11 -0500

   > Hmm.  TRT would be to figure this out in the "interactive" form, so that
   > repeat-complex-command works, but that would change the args to vc-diff.
   > How big a change are you willing to contemplate?

   A largish patch is not a problem in and of itself (except for copyright
   reasons, but once you've signed the paperwork it's a non-issue).  So the
   only reason to reject such a change would be if it made the code
   conceptually more tricky/complex.  From the sound of it, it would make
   it actually more regular, more in line with the usual way commands work
   in Emacs, so it seems OK.

The problem, of course, is knowing when to stop.  ;-}  vc-diff calls
vc-version-diff, which should also be changed to accept a fileset, but
that's tricky because it's called via call-interactively . . .

   Anyway, I've appended a work-in-progress patch, FYI.  It depends on
refactoring vc-deduce-fileset, which in turn depends on the assumption
that vc-parent-buffer must be registered.  Comments welcomed.

   Of course, there is also the risk of introducing bugs/incompatibilities,
   so try and make extra sure you take into account all callers.

	   Stefan

Grepping found only one caller; vc-log-edit passes it as the
log-edit-diff-function.  But this really ought to use the passed
fileset, and not go through the command again, true?

					-- Bob

------------------------------------------------------------------------
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 665dafb..0681ed3 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -927,31 +927,20 @@ Within directories, only files already under version control are noticed."
 (declare-function vc-dir-current-file "vc-dir" ())
 (declare-function vc-dir-deduce-fileset "vc-dir" (&optional state-model-only-files))
 
-(defun vc-deduce-fileset (&optional observer allow-unregistered
-				    state-model-only-files)
-  "Deduce a set of files and a backend to which to apply an operation.
+(defun vc-deduce-fileset-internal (&optional nonviolent-p state-model-only-files)
+  "Deduce a set of registered files and a backend for an operation.
 
 Return (BACKEND FILESET FILESET-ONLY-FILES STATE CHECKOUT-MODEL).
-If we're in VC-dir mode, the fileset is the list of marked files.
-Otherwise, if we're looking at a buffer visiting a version-controlled file,
-the fileset is a singleton containing this file.
-If none of these conditions is met, but ALLOW_UNREGISTERED is on and the
-visited file is not registered, return a singleton fileset containing it.
-Otherwise, throw an error.
+See vc-deduce-fileset for details; we do just the first part of the search,
+looking for registered files, returning nil if nothing found.
 
-STATE-MODEL-ONLY-FILES if non-nil, means that the caller needs
-the FILESET-ONLY-FILES STATE and MODEL info.  Otherwise, that
-part may be skipped.
-BEWARE: this function may change the
-current buffer."
-  ;; FIXME: OBSERVER is unused.  The name is not intuitive and is not
-  ;; documented.  It's set to t when called from diff and print-log.
+BEWARE: this function may change the current buffer."
   (let (backend)
     (cond
      ((derived-mode-p 'vc-dir-mode)
       (vc-dir-deduce-fileset state-model-only-files))
      ((derived-mode-p 'dired-mode)
-      (if observer
+      (if nonviolent-p
 	  (vc-dired-deduce-fileset)
 	(error "State changing VC operations not supported in `dired-mode'")))
      ((setq backend (vc-backend buffer-file-name))
@@ -964,11 +953,37 @@ current buffer."
      ((and (buffer-live-p vc-parent-buffer)
            ;; FIXME: Why this test?  --Stef
            (or (buffer-file-name vc-parent-buffer)
-				(with-current-buffer vc-parent-buffer
-				  (derived-mode-p 'vc-dir-mode))))
+	       (with-current-buffer vc-parent-buffer
+		 (derived-mode-p 'vc-dir-mode))))
+      ;; Note that vc-parent-buffer must be registered.
       (progn                  ;FIXME: Why not `with-current-buffer'? --Stef.
 	(set-buffer vc-parent-buffer)
-	(vc-deduce-fileset observer allow-unregistered state-model-only-files)))
+	(vc-deduce-fileset-internal nonviolent-p state-model-only-files))))))
+
+(defun vc-deduce-fileset (&optional nonviolent-p allow-unregistered
+				    state-model-only-files)
+  "Deduce a set of files and a backend to which to apply an operation.
+
+Return (BACKEND FILESET FILESET-ONLY-FILES STATE CHECKOUT-MODEL).
+If we're in VC-dir mode, the fileset is the list of marked files.
+Otherwise, if we're in dired-mode, use current/marked files.
+Otherwise, if we're looking at a buffer visiting a version-controlled file,
+the fileset is a singleton containing this file.
+Otherwise, if we're in a VC buffer that has a parent, try again in the parent.
+If none of these conditions is met, but ALLOW-UNREGISTERED is true and the
+visited file is not registered, return a singleton fileset containing it.
+Otherwise, throw an error.
+
+NONVIOLENT-P means that the fileset will be used for a non-state-changing
+operation, such as vc-log or vc-diff.
+
+STATE-MODEL-ONLY-FILES if non-nil, means that the caller needs
+the FILESET-ONLY-FILES STATE and MODEL info.  Otherwise, that
+part may be skipped.
+BEWARE: this function may change the
+current buffer."
+  (cond
+     ((vc-deduce-fileset-internal nonviolent-p state-model-only-files))
      ((not buffer-file-name)
        (error "Buffer %s is not associated with a file" (buffer-name)))
      ((and allow-unregistered (not (vc-registered buffer-file-name)))
@@ -980,7 +995,7 @@ current buffer."
 		nil)
 	(list (vc-backend-for-registration (buffer-file-name))
 	      (list buffer-file-name))))
-     (t (error "No fileset is available here")))))
+     (t (error "No fileset is available here"))))
 
 (defun vc-dired-deduce-fileset ()
   (let ((backend (vc-responsible-backend default-directory)))
@@ -1650,7 +1665,7 @@ returns t if the buffer had changes, nil otherwise."
 		    (called-interactively-p 'interactive)))
 
 ;;;###autoload
-(defun vc-diff (historic &optional not-urgent)
+(defun vc-diff (historic &optional fileset not-urgent)
   "Display diffs between file revisions.
 Normally this compares the currently selected fileset with their
 working revisions.  With a prefix argument HISTORIC, it reads two revision
@@ -1658,11 +1673,24 @@ designators specifying which revisions to compare.
 
 The optional argument NOT-URGENT non-nil means it is ok to say no to
 saving the buffer."
-  (interactive (list current-prefix-arg t))
+  (interactive
+    (list current-prefix-arg
+	  (if current-prefix-arg
+	      nil
+	      (or (vc-deduce-fileset-internal t)
+		  (let ((file (read-file-name "VC diff file: "
+					      default-directory
+					      default-directory)))
+		    (list (or (vc-responsible-backend file)
+			      (error "%S is not under version control." file))
+			  (list (file-truename file))))))
+	  t))
   (if historic
       (call-interactively 'vc-version-diff)
-    (when buffer-file-name (vc-buffer-sync not-urgent))
-    (vc-diff-internal t (vc-deduce-fileset t) nil nil
+    (when buffer-file-name
+      ;; [there should be a vc-fileset-sync instead.  -- rgr, 16-Nov-10.]
+      (vc-buffer-sync not-urgent))
+    (vc-diff-internal t fileset nil nil
 		      (called-interactively-p 'interactive))))
 
 ;;;###autoload

Message #32 received at 7350 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Bob Rogers <rogers <at> rgrjr.dyndns.org>
Cc: 7350 <at> debbugs.gnu.org
Subject: Re: bug#7350: 24.0.50; make vc-deduce-backend smarter
Date: Wed, 17 Nov 2010 08:31:26 -0500

>    A largish patch is not a problem in and of itself (except for copyright
>    reasons, but once you've signed the paperwork it's a non-issue).  So the
>    only reason to reject such a change would be if it made the code
>    conceptually more tricky/complex.  From the sound of it, it would make
>    it actually more regular, more in line with the usual way commands work
>    in Emacs, so it seems OK.

> The problem, of course, is knowing when to stop.  ;-}

For having looked at this particular part of the code with similar
intentions a few years ago, I know.

>    Anyway, I've appended a work-in-progress patch, FYI.  It depends on
> refactoring vc-deduce-fileset, which in turn depends on the assumption
> that vc-parent-buffer must be registered.  Comments welcomed.

Thanks for the work-in-progress.  Comments will follow.

> Grepping found only one caller; vc-log-edit passes it as the
> log-edit-diff-function.  But this really ought to use the passed
> fileset, and not go through the command again, true?

Actually, what it *should* do is select the same fileset as the one that
C-c C-c will commit.  I'm not sure which fileset is chosen by C-c C-c,
but if I were to choose, I'd follow the PCL-CVS behavior which is to
recompute the fileset rather than use whatever was the fileset when the
*VC-Log* buffer was created (but aditionally check whether the new
fileset is the same as the old one, and if not prompt the user to make
sure that's really what she intended).


        Stefan

Message #35 received at 7350 <at> debbugs.gnu.org (full text, mbox):

From: Bob Rogers <rogers <at> rgrjr.dyndns.org>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 7350 <at> debbugs.gnu.org
Subject: Re: bug#7350: 24.0.50; make vc-deduce-backend smarter
Date: Thu, 18 Nov 2010 21:27:19 -0500

   From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
   Date: Wed, 17 Nov 2010 08:31:26 -0500

   > Grepping found only one caller; vc-log-edit passes it as the
   > log-edit-diff-function.  But this really ought to use the passed
   > fileset, and not go through the command again, true?

   Actually, what it *should* do is select the same fileset as the one
   that C-c C-c will commit.

Yes, exactly.

   I'm not sure which fileset is chosen by C-c C-c, but if I were to
   choose, I'd follow the PCL-CVS behavior which is to recompute the
   fileset rather than use whatever was the fileset when the *VC-Log*
   buffer was created . . .

The fileset is not recomputed; if you change the fileset in *vc-dir*,
you must do vc-next-action explicitly to update the log buffer fileset.
That threw me for a loop when I first used it, but now that I've gotten
used to it, I can see the utility of it.

   (but aditionally check whether the new fileset is the same as the old
   one, and if not prompt the user to make sure that's really what she
   intended).

	   Stefan

Sounds like a clear win, since it removes the confusingness while
continuing to allow both behaviors.  Accordingly, that ought to get
fixed first.  If I come up with a reasonable patch, I'll file a new bug
report.

					-- Bob

Message #38 received at 7350 <at> debbugs.gnu.org (full text, mbox):

From: Bob Rogers <rogers <at> rgrjr.dyndns.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 7350 <at> debbugs.gnu.org
Subject: Re: bug#7350: 24.0.50; make vc-deduce-backend smarter
Date: Sat, 20 Nov 2010 14:54:17 -0500

   From: Stefan Monnier <monnier <at> iro.umontreal.ca>
   Date: Fri, 12 Nov 2010 08:48:30 -0500

   > Works for me.  It doesn't cover the cases where I'm looking at a
   > generated file, or test output, but (in my workflow anyway) those cases
   > are rarer.

   I've installed the patch into the trunk (i.e. not for Emacs-23.3 but
   Emacs-24) and added compilation-mode to shell-mode, since it's also
   clearly linked to a particular location in the file-system.

FWIW, the "32.1.4 Examining And Comparing Old Revisions" section of the
Emacs info documentation says the following (tenth paragraph):

       If you invoke `C-x v =' or `C-u C-x v =' from a buffer that is
    neither visiting a version-controlled file nor a VC directory buffer,
    these commands generate a diff of all registered files in the current
    directory and its subdirectories.

So it turns out that my expectation is what is actually documented after
all.  ;-}

					-- Bob

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.