Re: Patches for mc-4.5.51

[Pavel Roskin <proski gnu org>]

On Sat, 20 Sep 2003, Jindrich Makovicka wrote:

> Fr?d?ric L. W. Meunier wrote:
> > Anyway, if I was a programmer I'd contribute to HEAD, not fork
> > and have my ideas used by a few people.
>
> Here is my favourite one, resume support for ftp (& others), I am using
> it for about a year, and don't think anymore that this feature will ever
> get into "official" mc :)

I don't see any comments about this code.  I don't understand the changes
in vfs/local.c.  Why do we seek the file only if it's in the linear mode?
I think there are no limitations for seek on local files.  I understand
setting O_LINEAR by the caller of mc_open means certain limitations of
what the caller can do.  I don't think it has means that VFS may not seek
the file.  I would expect you to change to comment in vfs.h if you change
the semantic of that flag.

Oh, I see it now.  You arranged the code so that offset will be 0 if
O_LINEAR is not set.  Looks like a trap for those who will work on this
code after you.

Also, there is a warning in vfs.c:

vfs.c: In function `mc_open':
vfs.c:388: warning: `mode' might be used uninitialized in this function

As far as I can see, the warning is genuine.  "mode" and "offset" are used
in the same call.  Can it be happen that the file you are going to reget
disappears at this point, and VFS creates a world writable file?

mc_open() is supposed to be a VFS equivalent of open().  I don't feel good
about adding the fourth argument to it.

What could you cay about following comment from vfs.h?

 *      c) lseek() immediately after open(), giving ftpfs chance to
 *         reget. Be warned that this lseek() can fail, and you _have_
 *         to handle that gratefully.

If that doesn't work, let's create new function mc_open_linear() with 4
arguments and get rid of O_LINEAR.

I'll appreciate comments from others who understand VFS.

> I appreciate the efforts regarding the code cleanups, but maybe the mc
> development should be a _bit_ more liberal. It's nice that the current
> mc has a much cleaner codebase, but the things which pissed me off (ftp
> resume, operations which sometimes have to be cancelled by ctrl-z kill
> -9 mc - mainly Viewer and Find File) three years ago are still there and
> there's no sign they'll go away anytime soon.

The problem is, if the patch is not explained, there is a good chance that
it's not worth looking at, at least not as the first priority.  Often it
turns out that those who don't comment their changes don't understand
them.

There may be great patches that get lost because there is no manpower to
review them.  That's too bad, but there is nothing to be done about it
unless more people start reviewing code.  It's not the same as testing in
some obvious situations.  A bad patch can withstand testing but make the
code break later.

Liberal development doesn't mean that patches that make the code more
fragile are welcome.

> And yes, I sent patches addressing these issues, with no reply. Maybe
> they are crappy, because I don't know the mc code much, but at least
> "your patch sucks because of this and that" would be appreciated.

I agree.  Everybody who understands the code is welcome to comment on
patches.  There is no way I can do it alone.

-- 
Regards,
Pavel Roskin