Why my changes to code is missing?

Why my changes to code is missing?

Postby acme_pjz » 24 Oct 2011, 09:51

Hi Edward_Lii,

Last time I have fixed some bugs under Windows in FileManager and commited it, but today after I update the working copy I found my changes are lost :( Maybe you forget update before commiting changes? :?
Some of my open source games on GitHub
User avatar
acme_pjz
 
Posts: 665
Joined: 10 Dec 2009, 15:32
Location: PeeKing, China

Re: Why my changes to code is missing?

Postby Edward_Lii » 24 Oct 2011, 18:12

Hello acme_pjz,

acme_pjz {l Wrote}:Hi Edward_Lii,

Last time I have fixed some bugs under Windows in FileManager and commited it, but today after I update the working copy I found my changes are lost :( Maybe you forget update before commiting changes? :?

I think I know what the problem is, I've changed some things back to original in the removeDirectory() functions.
And with the latest revision it's again not working under Windows.
The booleans are under Linux reversed since the return values of unlink() and rmdir() are zero for succes (bool r=false).
I think it would be best if we changed it that no matter what, true=succes and false=failure.

The first part of the code is ok.
bool removeDirectory(const char *path){
#ifdef WIN32
             WIN32_FIND_DATAA f;
             HANDLE h = FindFirstFileA((string(path)+"\\*").c_str(),&f);
#else
             //Open the directory that needs to be removed.
             DIR* d=opendir(path);
#endif
             //Get the path length
             size_t path_len = strlen(path);
             //Boolean if the directory is emptied.
             //True: succes                         False: failure
             //Default is false because if the directory couldn't be opened we can return r(false).
             bool r = false;
#ifdef WIN32
             if(h!=NULL && h!=INVALID_HANDLE_VALUE) {
#else
             //Check if the directory exists.
             if(d) {
                         //Pointer to an entry of the directory.
                         struct dirent* p;
#endif


In the Linux code we enter a while loop as long as r!=true and there's an diren(try) in the directory that needs to be removed.
#ifdef WIN32
                         do{
#else
                         //Loop the entries of the directory that needs to be removed.
                         while(!r && (p=readdir(d))) {
#endif


Here we create a second boolean r2 which contains true if something went wrong.
Which I changed to true if it succeeds and false if it fails.
                                      //Boolean if the entry is deleted.
                                      //True: succes                         False: failure
                                      bool r2 = false;
                                      char* buf;
                                      size_t len;

                                      /* Skip the names "." and ".." as we don't want to recurse on them. */
#ifdef WIN32
                                      if (!strcmp(f.cFileName, ".") || !strcmp(f.cFileName, "..")) {
#else
                                      if (!strcmp(p->d_name, ".") || !strcmp(p->d_name, "..")) {
#endif
                                                  //The filename is . or .. so we continue to the next entry.
                                                  continue;
                                      } else {

#ifdef WIN32
                                                  //Get the length of the path + the directory entry name.
                                                  len = path_len + strlen(f.cFileName) + 2;
#else
                                                  //Get the length of the path + the directory entry name.
                                                  len = path_len + strlen(p->d_name) + 2;
#endif
                                                  buf = (char*) malloc(len);


Here's something wrong with the Windows code, in the else statement it should remove a file?
                                                  if(buf) {
#ifdef WIN32
                                                               _snprintf(buf, len, "%s\\%s", path, f.cFileName);

                                                               if(f.dwFileAttributes &; FILE_ATTRIBUTE_DIRECTORY){
                                                                           r2 = removeDirectory(buf);
                                                               }else{
                                                                           r2 = true; //Remove file here? <-----
                                                               }
#else


The Linux code shouldn't contain the unlink(buf)!=0;.
This way r2 will be true if the action failed.
r2 must be true when the action succeeded so, r2=unlink(buf)==0
                                                               struct stat statbuf;
                                                               snprintf(buf, len, "%s/%s", path, p->d_name);

                                                               if(!stat(buf, &statbuf)){
                                                                           //Check if the entry is a directory or a file.
                                                                           if (S_ISDIR(statbuf.st_mode)){
                                                                                        r2 = removeDirectory(buf);
                                                                           }else{
                                                                                        r2 = unlink(buf)!=0;
                                                                           }
                                                               }
#endif
                                                               //Free the buf.
                                                               free(buf);
                                                  }


There's no need to set r to r && r2, at least for the Linux code.
At the moment as long as r=false the while loop continues.
When a file fails to be deleted r2=true r will be set true, the while loop will be broken and the removeDirectory() function failed.

Even if we change r and r2 to true=success we don't need to edit this line, since the while loop will end depending on the status of the deletion of the diren(try). (r2)
                                                  //We set r to r2 since r2 contains the status of the latest deletion.
                                                  // FIXME: should be r && r2 (??)
                                                  r = r2;
                                      }
#ifdef WIN32
                         }while(FindNextFileA(h,&f));
                         FindClose(h);
#else
                         }
                         //Close the directory.
                         closedir(d);
#endif
             }
            


The while loop can end in two ways, there are no directory entries any more(normal end).
Or a failure, r=true.
That's why !r is checked, r is only true when it failed.
             //The while loop has ended, meaning we (tried) cleared the directory.
             //If r is true, meaning no errors we can delete the directory.
             // FIXME: should be 'r', not '!r' (??)
             if(/*!*/r){
                         // FIXME: the return value of rmdir is 0 means successful (??)
                         r = rmdir(path)==0;
             }
            


This piece of code is wrong either way.
It returns false on success.
             //Return the status.
             return r;
}


The fixed code where true=success and false=failure is in the svn. (here)
Sorry for the inconvenience. :(
From,
Edward_Lii
User avatar
Edward_Lii
MnMS Moderator
 
Posts: 777
Joined: 20 Dec 2010, 16:46

Re: Why my changes to code is missing?

Postby acme_pjz » 25 Oct 2011, 06:49

Hi,

1. In Linux code you don't have code for removing existing files either, if I read the code correctly. So in Windows I don't write code for removing files...

2. I haven't see the notes that the return value is "false" means successful, and "true" means failed :| How confusing :? So I'll check the Windows code soon...

3. Does the Linux code work now?
Some of my open source games on GitHub
User avatar
acme_pjz
 
Posts: 665
Joined: 10 Dec 2009, 15:32
Location: PeeKing, China

Re: Why my changes to code is missing?

Postby Edward_Lii » 25 Oct 2011, 12:59

Hello acme_pjz,

acme_pjz {l Wrote}:1. In Linux code you don't have code for removing existing files either, if I read the code correctly. So in Windows I don't write code for removing files...

This is the line that removes the files: r2 = unlink(buf)!=0; (Unlink() man page)

acme_pjz {l Wrote}:2. I haven't see the notes that the return value is "false" means successful, and "true" means failed :| How confusing :? So I'll check the Windows code soon...

I've changed it to true=success, I know it's confusing. :|
From now one true=success or at least something positive!

acme_pjz {l Wrote}:3. Does the Linux code work now?

Yes it's working fine now.

If I'm not mistaking all that needs to be done to the Windows code is:
    * Add file deletion code.
    * Change the while loop to while(r && FindNextFileA(h,&f));
From,
Edward_Lii
User avatar
Edward_Lii
MnMS Moderator
 
Posts: 777
Joined: 20 Dec 2010, 16:46

Re: Why my changes to code is missing?

Postby acme_pjz » 26 Oct 2011, 04:45

Hi Edward_Lii,

Yes you are right, and I finished checking Windows code and uploaded it to SVN :)
Some of my open source games on GitHub
User avatar
acme_pjz
 
Posts: 665
Joined: 10 Dec 2009, 15:32
Location: PeeKing, China

Who is online

Users browsing this forum: No registered users and 1 guest