bug#11328: 24.1.50; Comment in `dired-copy-file-recursive' code

[Drew Adams]

Hi Thierry,

> about fluid variables in dired code, particularly in
> `dired-create-files', I would say instead of fluid "obscure", as they
> are variables that must be in any calls of `dired-create-files'
> within a lambda used as argument of this function.
> I think it would be better to have explicit arg in the caller and have
> also the lambda's there.  The code would be easier to read and
> understand and no need to have obscure comments such as "fluid
> variable...etc".

That the use of those vars can make the code obscure is one thing.  But what
makes the vars potentially problematic is that they are free.  If there is to be
a comment in the code to draw attention to this then the comment should say that
the vars are _free_ here or there.  A comment should not just say that they are
"obscure" (I know you did not suggest that).

What you suggest is one approach to eliminating the free occurrences.  I'm not
sure that's really needed or is the best approach.  I have no opinion about that
- I don't really care much one way or the other.  To know what the best approach
is someone would need to spend a bit of time with the code.

There are also some other approaches, if we did want to eliminate those free
occurrences:

The code (e.g. callers) could just use either (a) lexical scoping (`lexical-let'
or file-level) to capture the variable plus its value within the lambda closure,
or (b) backquote with quote+comma to capture only the value (i.e., a
pseudo-closure: no var at all, just the value).

E.g., in the NAME-CONSTRUCTOR arg that is passed by `dired-do-create-files' to
`dired-create-files', the code could use this, substituting TARGET's value
instead of leaving TARGET as a free var in the lambda:

`(lambda (from)
  (expand-file-name (file-name-nondirectory from) ',target))

instead of:

(lambda (from)
  (expand-file-name (file-name-nondirectory from) target))

Or it could just use the latter if TARGET were lexically bound with the right
value.  In that case the lambda would form a closure.

That's an easy one.  There is also the more convoluted case of `d-do-copy',
which calls `d-create-files', which binds `d-overwrite-confirmed' around its
funcall of the FILE-CREATOR arg, which is `d-copy-file' in this case, which
calls `d-handle-overwrite' (without passing `d-overwrite-confirmed'), which uses
`d-overwrite-confirmed'.  Maybe that's what you had in mind.

First thing about that one is that the funcall actually passes
`d-overwrite-confirmed' as an arg to `d-copy-file', in addition to binding it
for use by `d-handle-overwrite'.  It would be simpler to just add it as a
parameter for `d-handle-overwrite' and then let `d-copy-file' and others pass it
along explicitly to that function.

Second thing is that the value of `overwrite-backup-query', which var is free in
`d-handle-overwrite', is never even used anywhere.  That var is bound in
`d-create-files' presumably only because `d-query', to which it is passed,
expects a variable (which it sets - in this case uselessly).

There is plenty of such convoluted stuff in the Dired code.  No doubt some of it
could be simplified, but the cleanup would have to be careful and be sure not to
change any behavior.  And some changes will likely affect 3rd-party code (e.g.
Dired+).

One reason for such complicated code (a guess) is that someone at one point
tried to define some generic code (macros, functions), and other code was
written to use that code, and then later someone needed slightly different
generic functions (e.g. additional parameters), but instead of modifying the
generic functions (because there was already consuming code that expected the
existing interfaces) they fudged other ways of getting the additional info to
the recipients.  Such is life.

Other than that, the same ideas above apply.  Suppose that there is some good
reason not to add a parameter to `d-handle-overwrite' in order to pass the
`d-overwrite-confirmed' value.  Or suppose that `d-copy-file' did not accept a
third arg, and that it were only `d-handle-overwrite' that needed the
`d-overwrite-confirmed' value.

In such cases, to get the `d-overwrite-confirmed' value to the innards of such
functions that do not accept it as an arg, instead of funcalling FILE-CREATOR
directly, `d-create-files' could funcall a lambda that encapsulates the value of
`d-overwrite-confirmed' and calls FILE-CREATOR.  That too would explicitly
reflect the fact that the file creator function depends on that value.

Or if `d-copy-file' & compagnie are used only for `d-create-files', then put
their defuns inside it lexically (nested defuns) and use lexical scoping for the
file.  And so on.

There are different ways to eliminate the free vars or wrap them together with
their values in a closure.  And perhaps the code could anyway be simplified in
other ways, which might obviate any such need.  Dunno.  I haven't bothered to
look closely at it (I don't care enough).  Again, if someone does that, I really
hope they are careful.

Or we can just live with the free vars, in which case a comment doesn't hurt.
But it should say "free", not "fluid", IMO. ;-)

 - Drew