Re: [PATCH] shell32: Fix SHFileOperation(FO_MOVE) for creating subdirectories.

[Zhenbo Li]

Excuse me,
Does anyone knows how patch 113646 can be improved?

Thank you very much

2015-08-14 21:18 GMT+08:00 Zhenbo Li :
> This patch fixes bug 25207. Seems that a dozen of apps are affected by this
> bug, so please tell me how I can improve this patch if this patch can't be
> committed directly.
>
> There will be a test error:
> shlfileop.c:818: Test failed: Expected ERROR_CANCELLED or DE_DIFFDIR
>
> It is not related to my patch.
>
> Hope you enjoy your weekend.
>
> dlls/shell32/shlfileop.c       | 11 +++++++++--
>  dlls/shell32/tests/shlfileop.c |  6 +++---
>  2 files changed, 12 insertions(+), 5 deletions(-)

[PATCH] shell32: Fix SHFileOperation(FO_MOVE) for creating subdirectories.

[Matteo Bruni]

> 2015-08-14 21:18 GMT+08:00 Zhenbo Li :
> > This patch fixes bug 25207. Seems that a dozen of apps are affected by this
> > bug, so please tell me how I can improve this patch if this patch can't be
> > committed directly.
> >
> > There will be a test error:
> > shlfileop.c:818: Test failed: Expected ERROR_CANCELLED or DE_DIFFDIR
> >
> > It is not related to my patch.
> >
> > Hope you enjoy your weekend.
> >
> > dlls/shell32/shlfileop.c       | 11 +++++++++--
> >  dlls/shell32/tests/shlfileop.c |  6 +++---
> >  2 files changed, 12 insertions(+), 5 deletions(-)

Not a real review, I have no clue about shell32. This is just what I
noticed at a quick and pretty uninformed look.

@@ -1426,7 +1426,14 @@ static DWORD move_files(LPSHFILEOPSTRUCTW
lpFileOp, const FILE_LIST *flFrom, con
     }

if (!PathFileExistsW(flTo->feFiles[0].szDirectory))
-        return ERROR_CANCELLED;
+    {
+        ret = SHCreateDirectoryExW(NULL, flTo->feFiles[0].szDirectory, NULL);
+        if(ret)
+        {
+            ERR("SHCreateDirectoryW failed: %08x\n", ret);
+            return ret;
+        }
+    }

SHCreateDirectoryExW can probably legitimately fail so I don't think
an ERR is appropriate here. A WARN might be okay. While at it, please
add a space between 'if' and '('.

Unrelated preexisting issue, those helper functions for
SHFileOperationW() like move_files() return a DWORD but the error code
is otherwise int. It shouldn't matter in practice but it would be nice
to fix that (in a separate patch).

[PATCH] shell32: Fix SHFileOperation(FO_MOVE) for creating subdirectories.

[Zhenbo Li]

Hi Matteo,
Thanks for commenting on my patch

2015-08-27 0:45 GMT+08:00 Matteo Bruni :
> SHCreateDirectoryExW can probably legitimately fail so I don't think
> an ERR is appropriate here. A WARN might be okay. While at it, please
> add a space between 'if' and '('.

I think it could be more challenging than I expected to handle the
errors. I'll look into it deeper.
> Unrelated preexisting issue, those helper functions for
> SHFileOperationW() like move_files() return a DWORD but the error code
> is otherwise int. It shouldn't matter in practice but it would be nice
> to fix that (in a separate patch).

I agree with you. DWORD is unsigned, but SHFileOperationW returns a
int value. I'll write a patch to fix it,

PS. Thanks to Sebastian Lackner's comment on my patch:
https://bugs.wine-staging.com/show_bug.cgi?id=541#c3