Hi Assaf, > Attached 2 patches: > > mountlist tests: add mountlist-tests module > mountlist: add support for windows DOS-style drives
Two great contributions!! Comments about 0001-mountlist-tests-add-mountlist-tests-module.patch: It's your first gnulib test. I appreciate it! In the include sequence #include <config.h> #include <stdio.h> #include <mountlist.h> it's better to put <mountlist.h> directly after <config.h>. This adds a verification that the header file is self-contained. (Otherwise, if the header file accidentally uses e.g. the FILE type without including <stdio.h>, you wouldn't notice it.) Common indentation is by 2 spaces, not 3 spaces. In the module description, you can drop the dependency on 'mountlist', because this dependency is already implicit. This is documented in https://www.gnu.org/software/gnulib/manual/html_node/Module-description.html . These comments + Call read_file_system_list, print the list, and free the entries. + Typical output example: + $ ./gnulib-tool -create-testdir -dir mnt-list mountlist-tests + $ cd mnt-list + $ ./configure && make + $ ./gltests/test-mountlist + devname: proc + mountdir: /proc + mntdoor: / + type: proc + dev: 4 + dummy: 1 + remote: 0 + type_alc: 1 + devname: /dev/sda1 + mountdir: / + mntdoor: / + type: ext4 + dev: 2049 + dummy: 0 + remote: 0 + type_alc: 1 + [...] don't belong in the ChangeLog entry or commit log. They belong in the source code (tests/test-mountlist.c) instead. The ChangeLog should only tell what has changed. Even the "why" of a change is often better stored in the source code than in the ChangeLog. Please show the gnulib-tool options in their documentated form, with two minus signs in each option: ./gnulib-tool --create-testdir --dir=... Please consider using ASSERT in order to verify assertions about the values returned in the mount_entry struct. For example, if you expect mnt_ent->me_devname to be non-NULL, write ASSERT (mnt_ent->me_devname != NULL); It's more reliable this way, than to expect that printf will crash when you pass it a NULL argument. Some printf implementations do, some don't. The ASSERT macro is defined in "macros.h". Add tests/macros.h in the module description. Comments about 0002-mountlist-add-support-for-windows-DOS-style-drives.patch: Here too, text that explains the functioning of code or what result to expect from a test belongs in the source code, not in the ChangeLog entry. "Win32 API" is an old name; Microsoft calls it the "Windows API" for several years already. It's better to #include <windows.h> just once (lines 139 and 203). The code computes 'A'+i and stores it in a string. Probably the loop should go over 0..25, not 0..31 ? The first argument to GetVolumeInformation must be NUL-terminated. It is confusing if you construct the string through snprintf. I would write char RootPathName[3+1]; sprintf (RootPathName, "%c:\\", (char)('A'+i)); This is clearer. Why do you write '\x0' where most C programmers write '\0' ? In ls-mntd-fs.m4 you don't need the 'return 0;' to make an ANSI C compliant main() function - AC_LANG_PROGRAM takes care of it already. In ls-mntd-fs.m4 please try to use an indentation that reflects the logical nesting level: + [[ DWORD dw = GetLogicalDrives(); return 0; ]])], + [fu_cv_getlogicaldrives=yes], The second line should have less indentation that the first line. Bruno
