Hi Vivek, On 03/24/2012 10:03 PM, Vivek Prakash wrote: > Line 357 > The static declarations IMHO should not go to the .h file, but rather to > the .c file. > > > I have moved them to ls.c file. But, please have a look at rm/rm.h file, > where static declarations are done.
It might be the bdsh cmd skeleton generator which prepares a new command this way. Anyway, I believe private declarations should go to the implementation, not the header file. > I have set up a bazaar branch. The diff can be seen at > http://bazaar.launchpad.net/~vivek-cs-iitr/vivekp/mainline/revision/1441. The > corresponding branch can be cloned as: > bzr branch lp:~vivek-cs-iitr/vivekp/mainline HelenOS > > I am new to bazaar, but i have prior experience with other VCS. Please > tell me if it's not the right way of using bazaar. >From definition, Bazaar will be very similar to most other distributed VCSs, such as Mercurial or Git. Of course, there will be also differences. One such difference is that in Bazaar you have one directory per branch. I don't know if Bazaar can also cram more branches into one directory / repository, but I don't really care. There is one thing about your branch though. You named it mainline, which suggest you will only use it to track mainline changes. For branches that aim to provide a fix or a new feature, we would normally pick some descriptive name of that feature or a bugfix, such as 'ls' or 'bdsh' in your case. I know Launchpad is not very intuitive in this regard as it allows several approaches (branches vs. projects), but I usually have branches in the following form: lp:~jakub/helenos/<branch_name>. It is even possible to create a branch like that on Launchpad from command line once you have your SSH keys uploaded to it. > I have also attached the patch file in this mail. Any suggestions are > very welcome. If the patch is not acceptable at all, i can start all > over again. The patch looks visually good now. I also tried running your changes and found several small issues. There seems to be a file descriptor leak in ls_recursive() after it calls a nested ls_recursive(): ret = ls_recursive(subdir_path, subdirp); + closedir(subdirp); Without the closedir(), you will not be able to successfully run ls -R / more than once or twice because the whole bdsh task will run out of descriptors. I also suspect ls_recursive() leaks the entire dir_list list and also for each dir_elem_t on it, it leaks the name member, can you confirm? I will be happy to merge your changes once these issues are resolved. Regards, Jakub _______________________________________________ HelenOS-devel mailing list [email protected] http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
