Synchronize Dirs - file properties not synchronized properly

[MarcinW]

Steps to reproduce (TC 9.12 RC4):

1a) In left panel, create empty file: "c:\test1\file.txt"
1b) Set read-only attribute of this file and clear all other attributes
1c) Set date and time of this file to, let's say, 2017-01-01 1:00:00

2a) In right panel, create empty file: "c:\test2\file.txt"
2b) Set archive attribute of this file and clear all other attributes
2c) Set date and time of this file to the current date and time

3) Launch "Commands" -> "Synchronize Dirs..."
4) Ensure that "ignore date" option is unchecked and press "Compare" button - a line with information about both "file.txt" files will be displayed
5) Right-click this line and choose option "Copy file properties <- (right to left)..."
6) A "Copy file properties <-" dialog appears: check "Timestamp (date+time)" option and "File attributes" option and press OK

In left panel, attributes of the "c:\test1\file.txt" file were properly changed from read-only to archive, but date and time were NOT changed and are still 2017-01-01 1:00:00

Regards

[ghisler(Author)]

This happens because Windows doesn't allow to modify files with read only attribute - not even the time stamp. I will try to fix it.

[MarcinW]

Original problem has been fixed, but there is still a problem with the current implementation (TC 9.20 beta 1). Try this:

1a) In left panel, create empty file: "c:\test1\file.txt"
1b) Clear all the attributes of the file (i.e. RAHS)

2a) In right panel, create empty file: "c:\test2\file.txt"
2b) Change access rights of the file - for example grant all access rights to everyone
2c) Clear all the attributes of the file (i.e. RAHS)

3) Launch "Commands" -> "Synchronize Dirs..."
4) Press "Compare" button - a line with information about both "file.txt" files will be displayed
5) Right-click this line and choose option "Copy file properties <- (right to left)..."
6) A "Copy file properties <-" dialog appears: check "NTFS permissions" option and press OK

In left panel, an ARCHIVE attribute of the "c:\test1\file.txt" file has been set.

It seems that modifying NTFS permissions makes an ARCHIVE attribute set.

Regards

[Dalai]

It seems that modifying NTFS permissions makes an ARCHIVE attribute set.

Yes, it does, and it makes sense because the file needs to be archived (backed up) again, at least in case all permissions are part of the backup.

Regards
Dalai

[MarcinW]

You are right - so TC should remove an ARCHIVE attribute when it's not desired.

[ghisler(Author)]

This should be fixed in TC 9.20 beta 2, please test it!

[MarcinW]

I need a bit more time to test this thoroughly.

[MarcinW]

Still not good.

Try this (TC 9.20 beta 3):

1a) In left panel, create empty file: "c:\test1\file.txt"
1b) Set Read Only, Hidden and System attributes
1c) Clear all access rights for the file - remove all access rights for everyone (this will also set Archive attribute)

2) In right panel, create empty file: "c:\test2\file.txt"

Now synchronize attributes and NTFS access rights from left to right.


Instead of having the right file with Read Only, Hidden, System and Archive attributes set, we get a file with only Archive attribute set. Not good. It's worse than in TC 9.12.


General algorithm idea should be:
1) set date/time and attributes (may fail if old NTFS permissions deny this operation)
2) change NTFS permissions (this will set an Archive attribute)
3) set date/time and attributes again, if failed in step 1 or if the Archive attribute needs to be cleared (may fail if new NTFS permissions deny this operation)

When using this idea - except the case, when both old and new NTFS permissions deny any operations - the worst case failure is when we have an Archive attribute set (but it shouldn't be set) - not when we have Read Only, Hidden and System attributes cleared (when they should be set).


I created a draft of the algorithm - pay attention to "for Retry:=False to True do" loop:

Code: Select all

{Notes:
- setting date/time requires opening the file (SetFileTime),
  so Read-Only attribute must NOT be set
- setting NTFS access rights makes Archive attribute set
}
procedure SyncAlgorithmDraft(
  SyncDateTime, SyncAttr, SyncAccessRights : Boolean;
  const SrcFileName, DestFileName : string;
  const SrcDateTime : TFileTime;
  const SrcAttr : Integer; const DestAttr : Integer);
var
  SyncDateTimeERR, SyncAttrERR, SyncAccessRightsERR : Boolean;
  NeededDestDateTime : TFileTime;
  NeededDestAttr : Integer;
  NeededDestAccessRights : record end; // place some needed data format here
  Retry : Boolean;
begin
  NeededDestDateTime:=SrcDateTime;
  if SyncAttr then
    NeededDestAttr:=SrcAttr
  else
    NeededDestAttr:=DestAttr;

  for Retry:=False to True do
  begin
    SyncDateTimeERR:=False;
    SyncAttrERR:=False;
    SyncAccessRightsERR:=False;

    {==========================================================================}
    {STEP 1: Synchronize date/time}
    if SyncDateTime then
    if not SetFileTimeWrapper(DestFileName,NeededDestDateTime) then
    begin
      if DestAttr and FILE_ATTRIBUTE_READONLY <> 0 then
      if FileSetAttr(DestFileName,NeededDestAttr and not FILE_ATTRIBUTE_READONLY) = ERROR_SUCCESS then
      begin
        {If we don't need Read-Only attribute set, we are ready with
         attributes; if we need it set - we must force setting it}
        SyncAttr:=NeededDestAttr and FILE_ATTRIBUTE_READONLY <> 0;

        if not SetFileTimeWrapper(DestFileName,NeededDestDateTime) then
          SyncDateTimeERR:=True;
      end;
    end;

    {==========================================================================}
    {STEP 2: Synchronize attributes}
    if SyncAttr then
    if FileSetAttr(DestFileName,NeededDestAttr) <> ERROR_SUCCESS then
      SyncAttrERR:=True;

    if not SyncAccessRights then
      Break; {We are ready}
    if Retry then
      Break; {We already tried with new access rights}

    {==========================================================================}
    {STEP 3: Synchronize access rights}
    if not GetFileAccessRightsWrapper(SrcFileName,NeededDestAccessRights) then
    begin
      SyncAccessRightsERR:=True;
      Break; {Access rights have NOT been changed, so retrying with new access
              rights makes no sense}
    end;
    if not SetFileAccessRightsWrapper(DestFileName,NeededDestAccessRights) then
    begin
      SyncAccessRightsERR:=True;
      Break; {Access rights have NOT been changed, so retrying with new access
              rights makes no sense}
    end;

    {==========================================================================}
    {STEP 4: Handle retrying}

    {In case of earlier error, schedule setting date/time again, since the
     destination file has new access rights now}
    SyncDateTime:=SyncDateTimeERR;

    {In case of earlier error, schedule setting attributes again, since the
     destination file has new access rights now}
    {Setting access rights makes Archive attribute set, so schedule clearing
     it, if it's not desired}
    SyncAttr:=SyncAttrERR or (NeededDestAttr and FILE_ATTRIBUTE_ARCHIVE = 0);
  end;

  if SyncDateTimeERR or SyncAttrERR or SyncAccessRightsERR then
    ; // Show some message here, like "Some of the file properties could not be set properly"
end;

[ghisler(Author)]

Sorry, but this wouldn't work either - the new access rights may prevent the setting of attributes too, e.g. when copying other user's files. It may be tricky to set access rights to allow setting file attributes. Therefore I will keep it as it is now.

[MarcinW]

the new access rights may prevent the setting of attributes too, e.g. when copying other user's files

Of course you are right. Maybe I wasn't clear enough.

I was talking here only about cases when:
1) old access rights don't, but new access rights do allow setting attributes and date/time,
2) old access rights do, but new access rights don't allow setting attributes and date/time.


Current TC 9.12 behavior is:
TC sets attributes before setting new access rights, so it fails in case 1) and succeeds in case 2)

Current TC 9.20 beta 3 behavior is:
TC sets attributes after setting new access rights, so it succeeds in case 1) and fails in case 2)

What I'm trying to say, is that TC should set attributes and and date/time before and also after changing access rights, so it will handle both case 1) and case 2).


Instead of placing the same piece of code (setting date/time and attributes) twice in the SyncAlgorithmDraft above, I just used a "for Retry:=False to True do" loop. Please load this piece of code into Delphi, so it will be formatted and colored well, comments will be more visible - then everything should become clear. The code sets attributes and date/time for the second time only if necessary - to avoid useless disk operations. It also handles the fact, that setting access rights makes an Archive attribute set.

Regards

[ghisler(Author)]

Well, this would have other unwanted results, e.g. when the first change attribute call succeeds and the second fails, then the files will have that additional "archive" attribute.

[MarcinW]

when the first change attribute call succeeds and the second fails, then the files will have that additional "archive" attribute.

Unfortunately this is true. But it's a worst case failure of this algorithm. It seems to be better than the current implementation, when you can end up having a file with lacking Read Only, Hidden and/or System attributes...

After all, having an Archive attribute set (when not needed), may only lead to making a backup of the file (when not really needed).

By trying to set date/time and attributes twice, TC can do everything that is possible to do.

And, of course, TC can still inform the user, that the attributes have not been set properly...

Regards

[ghisler(Author)]

I think a good compomise is calling SetFileAttributes before and after setting the permissions. This will allow to set them if at least one of the two permissions allows to set the attributes. Nothing changes for setting the time.

[MarcinW]

Nothing changes for setting the time.

But setting time is similar to setting attributes - we must have access rights to open the file. So - if I didn't miss something - it may currently fail in a similar way as setting attributes?

[ghisler(Author)]

Sorry, for now I will not change it. This way it will at least not be worse than in TC 9.12.