Re: xcopy try 2

[Detlef Riekenberg]

On Fr, 2007-02-23 at 16:44 +0000, Ann & Jason Edmeades wrote:

Looks nice, but a view over the code result in some more comments:

--- a/configure.ac
+++ b/configure.ac
Not needed, when sending the Patch
(and that line causes already an offset, when patching wine)
"tools/make_makefiles" takes care of all required changes and
when Alexandre commit your Patch, the changes from
make_makefiles and autoconf are included in the commit.

+EXTRADEFS = -DUNICODE
There is no need for that define. 
Remove that line to get a warning for all ANSI-Functions, that
missed the "W"

+C_SRCS = \
+        xcopy.c

Spaces are wrong before the filename. You must use TAB in makefiles

+/* Prototypes */
This can be avoided completly, when you rearange your functions
(so main() is then the last function in the file)

+   printf("Invalid number of parameters - Use xcopy /? for help\n");

The text should be read from the resources, so it can be localized .
You should convert the UNICODE-Message with WideCharToMultiByte.
When the output-handle is the console, printing UNICODE-messages
avoid converting the message twice (xcopy and kernel32)
> -
>    if (*argvW[0] != '/') {
> -
>        if (suppliedsource[0] == 0x00) {
> -
>            lstrcpyW(suppliedsource, *argvW);
> -
>        } else {
> -
>            lstrcpyW(supplieddestination, *argvW);
> -
>        }

This is wrong. tested with: "xcopy file1 file2 file3"
Your code would copy file1 to file3, while w2k fails with
"Unzulässige Parameteranzahl" (Invalid number of parameters)

I suggest to initialize supplieddestination as empty string
and copy the arg to supplieddestination only, when it is emtpy.
When supplieddestination was not empty, quit with the error.

You need to fill supplieddestination with the default "."
before     /* Trace out the supplied information */,
when it is still empty at that location.

+    if (flags & OPT_SIMULATE) {
+        printf("%d file(s) would be copied\n", filesCopied);
+    } else {
+        printf("%d file(s) copied\n", filesCopied);
+    }

As Above: Format-Pattern from the resource and WriteFile()

+    if (GetFullPathName(suppliedsource, MAX_PATH,

"W" missing: => GetFullPathNameW
(More Functions in various locations and 
WIN32_FIND_DATA => WIN32_FIND_DATAW)

I did not test the code very much, but I think, you did.

(ToDo-Reminder:
  "xcopy file1 name_not_found" => 
  is "name_not_found" a file or a directory)

Thanks for your work. It's pretty good as start!

xcopy try 2

[David Laight]

On Sat, Feb 24, 2007 at 01:00:06AM +0100, Detlef Riekenberg wrote:
> +C_SRCS = \
> -
>    xcopy.c
>
> Spaces are wrong before the filename. You must use TAB in makefiles

That is a contiuation line, IIRC the whitespace at the start of the
continued line should be discarded (and can be space/tab anyway).

David

xcopy try 2

[Ann & Jason Edmeades]

Hi Detlef,

Firstly thanks for taking the time to comment on this...

Configure.ac - I included that because another 'regular' developer who added
a dll recently added it (See:)
   http://www.winehq.org/pipermail/wine-patches/2007-February/036243.html)
I'll leave it in for that reason unless there's a big problem with it (I
always thought you were supposed to send in configure.ac but not configure)
> +EXTRADEFS = -DUNICODE
> There is no need for that define. 
> Remove that line to get a warning for all ANSI-Functions, that
> missed the "W"

and
> - if (GetFullPathName(suppliedsource, MAX_PATH,
>
> "W" missing: => GetFullPathNameW
> (More Functions in various locations and 
> WIN32_FIND_DATA => WIN32_FIND_DATAW)

I disagree here - I don't think we should be coding with explicit W and A
method names, that's the whole point of the define, and when you look
through the MSDN it's the main symbol name which is used all the time, the W
and A ones are side effects of the implementation of the definitions. It
also mirrors what is used by e.g. oleview, wineconsole and winefile.
> +C_SRCS = \

Yes - my mistake, I'll fix that and resubmit it in try 3
> +/* Prototypes */
> This can be avoided completly, when you rearange your functions
> (so main() is then the last function in the file)

It can, but I also believed that putting prototypes was good programming
practice. Why would we not want them as they are harmless? They are easy
enough to remove, I just wanted justification.
> - printf("Invalid number of parameters - Use xcopy /? for help\n");
>
> The text should be read from the resources, so it can be localized .
> You should convert the UNICODE-Message with WideCharToMultiByte.
> When the output-handle is the console, printing UNICODE-messages
> avoid converting the message twice (xcopy and kernel32)

You wanted the patch small, remember :-) Lets leave that as something for a
later patch - I just want to get the infrastructure in first, then fix up
some of the other parameters, localization, help etc - Its something which
is trivial to do.
> This is wrong. tested with: "xcopy file1 file2 file3"

Yes, I'll also fix that in the try 3 resubmit (and your suggested solution
is fine). The code as it stands certainly isn't perfect especially as I am
just trying to put the minimum in to be functional, and some other error
checking may also be missing.
> (ToDo-Reminder: "xcopy file1 name_not_found" => is "name_not_found" a file

or a directory)

Its already in there (another string to be localized though!) - See middle
of XCOPY_ProcessDestParm.
> Thanks for your work. It's pretty good as start!

Thanks - its all pretty simple, and I am surprised how little code it is!

Jason