bug#7675: 24.0.50; vc-checkin should prompt if the fileset changes

[Bob Rogers]

   From: Stefan Monnier <monnier@iro.umontreal.ca>
   Date: Sun, 19 Dec 2010 21:50:03 -0500

   >    Thanks for submitting a patch to try and fix this problem.
   >    A quick obvious issue, tho:

   >> --- a/lisp/vc/vc.el
   >> +++ b/lisp/vc/vc.el
   >    [...]
   >> +(defun log-edit-deduce-fileset (state-model-only-files)

   >    If it's in vc.el it can't start with the "log-edit-" prefix.

   > Yes, I was wondering about that.  I had thought about putting it in
   > log-edit.el, but it seems closer to VC internals than log-edit
   > internals.

   log-edit deals with editing the text buffer, so clearly this job is not
   about log-edit but VC.

Fair enough.

   > Which would you prefer:  Rename it, or move it?  If
   > "rename", would "vc-log-edit-deduce-fileset" suffice?

   I guess so, tho I don't understand why you need this function.  IIUC you
   should switch to vc-parent before calling vc-deduce-fileset, so that
   function never needs to handle log-edit buffers.

Ah, that's a good question.  That is what the existing vc-deduce-fileset
does (as I'm sure you know), which means that if you change the *vc-dir*
fileset after starting a commit, C-x v = in the log buffer will diff the
new fileset, but C-c C-c will still commit the old one; this is also a
bug in the pretest [1], and probably back to 23.1.

   ISTM that if we want to detect when the *VC-log* buffer and the
*vc-dir* buffer have different filesets, then it makes sense to consider
that the *VC-log* buffer implies a fileset is independent of its parent.
That is the model to which I was coding.

   Another way to fix the problem would be to define the existing
vc-deduce-fileset behavior as correct, and fix vc-commit always to
commit the most current fileset.  Then you would need a different patch
entirely.  I am not crazy about this idea, because it would make it
harder to perform multiple commits in parallel.

   A third way to fix it would be to put vc-deduce-fileset in charge of
detecting when the *vc-dir* fileset has changed.  In other words, not
only would C-c C-c ask you what was up, C-x v = would as well.  This
ISTM would make parallel commits even harder.

   >    And while I'm thinking of it, a VC fileset really ought to be
   > represented by a struct, since all this car-ing and cdr-ing is really
   > messy.  But filesets are passed to backends, so testing such a change
   > would be a pain.  What are your thoughts?  It is worth it?

   It doesn't seem complex enough to warrant such a change.
   Maybe another approach is to use pcase.
   E.g. instead of

      (let ((backend (car fs))
            (files (cdr fs)))
        ..)

   or something like that, you'd write

      (pcase-let ((`(,backend . ,files) fileset))
        ...)

   Experience with ML-style languages tends to indicate that such pattern
   matching tends to reduce the need for named fields,

I was thinking more of abstraction.  This sort of destructuring means
you need to know that a fileset is represented as a list in which the
backend comes first and the files come second.  When I was trying to
find out how the various backends used the "state" element of that list,
for example, it would have been really nice to have been able to grep
for calls to something like "vc-fileset-state".

   tho obviously type checking (and inference) is also an important part
   of it since it catches incorrect patterns.

           Stefan

Indeed.  In fact, the correct pattern for a fileset is more like
`(,backend ,files ,nondir-files ,state ,model) -- so thank you for
illustrating my point.  ;-}

   I think that is part of why I am less than keen on Haskell and
Erlang, the only two ML-style languages which which I have any
acquaintance.

                                        -- Bob

[1] I hadn't noticed this before because I have a hacked version of
    vc-deduce-fileset I wrote more than two years ago that fixes this,
    by doing almost the same thing for log buffers as the code in the
    patch.  Not sure why I didn't report this bug at the time.  In any
    case, I had forgotten about it completely.