"commit" behaves counterintuitively when provided a glob, which includes an empty sub-directory (proposed patch included)

(1.2) By PChemGuy on 2024-05-04 09:59:59 edited from 1.1 [source]

Command

fossil ci -m "Message" some_dir/*.*

emits a warning (regarding not managed target) if "some_dir" contains an empty sub-directory, e.g., "some_dir/empty_subdir". For empty subdirectories, this warning should never be emitted, as Fossil does not manage directory objects. Below is a proposed patch. The patch adds a check whether the target path is a directory containing no child objects. The warning is only emitted if this check fails (the target is a file or a directory containing at least on child object).

Limitations:

The patch uses a routine provided by file.c, which may or may not ignore dot-objects. On Windows, each directory contains at least two child objects, "." and "..". I am not sure whether this convention is the same on other OSs. If it is,
```
file_directory_size(blob_str(&fname),0,0)!=2
```
would be better, as
```
file_directory_size(blob_str(&fname),0,1)!=0
```
would treat a directory containing dot-files as empty, which may or may not be the expected behavior.
This check also fails if the target subdirectory contains an empty child directory (e.g., "some_dir/empty_subdir/empty_subsubdir/"), though in such a case the warning should still not be emitted. A better way would be to traverse the subtree to see if it contains any files.

Index: src/checkin.c
==================================================================
--- src/checkin.c
+++ src/checkin.c
@@ -1648,11 +1648,11 @@
       while( db_step(&q)==SQLITE_ROW ){
         cnt++;
         bag_insert(&toCommit, db_column_int(&q, 0));
       }
       db_finalize(&q);
-      if( cnt==0 ){
+      if( cnt==0 && file_directory_size(blob_str(&fname),0,1)!=0 ){
         fossil_warning("fossil knows nothing about: %s", g.argv[ii]);
         result = 1;
       }
       blob_reset(&fname);
     }

(2.1) By Stephan Beal (stephan) on 2024-05-04 11:27:43 edited from 2.0 in reply to 1.2 [link] [source]

A better way would be to traverse the subtree to see if it contains any files.

Given the list of caveats and how rarely this non-problem comes up (it a non-fatal warning which is "not wrong"), the better way is arguably to leave it as is ;).

The patch uses a routine provided by file.c, which may or may not ignore dot-objects. On Windows, each directory contains at least two child objects, "." and "..". I am not sure whether this convention is the same on other OSs.

That's the convention on, AFAIK, all filesystems/OSes, but the fact that the routine does not skip those two special entries out of hand looks like a bug in itself. That will be fixed momentarily. (Edit: src:3cb7d39e124)

(3) By PChemGuy on 2024-05-04 11:36:58 in reply to 2.1 [link] [source]

The warning is not fatal, but Fossil interrupts execution and asks for user input, complicating non-interactive execution of the command. Perhaps Fossil should simply emit a notification and ignore any non-managed objects.

(4) By Stephan Beal (stephan) on 2024-05-04 12:27:31 in reply to 3 [link] [source]

... but Fossil interrupts execution and asks for user input, complicating non-interactive execution of the command

Non-interactive uses providing offending wildcards to the commit command sounds like a purely hypothetical case. Is this a problem which has bitten you in some way or is it just unsightly?

To be clear: my objection is solely from the perspective of long-term maintenance. Unsightly warnings for what amounts to invalid inputs¹ don't necessarily deserve extra code to handle them. If it were just that one call to file_directory_size() i'd have no objection but that approach is, as you point out, only a partial solution which does not account for subdirectories. A proper solution would require, IMO, more code than this particular warning justifies.

That said: mine is just one of many valid opinions and it's not uncommon for reasoned debate to change my opinions.

^{^} If wildcards are too greedy, don't use them - just pass abc instead of abc/*.*, which will have a similar effect without the wildcard-related quirks. (Similar, but not identical, because abc/*.* will not match abc/README or abc/Makefile on most (all?) Unix-style shells, though it will (IIRC) on Windows.)

(5) By PChemGuy on 2024-05-04 13:59:08 in reply to 4 [link] [source]

You are right, I do not need wildcards, and it solves the issue. In fact, I do want to match everything inside the directory, so I should not use them anyway, my mistake.

FYI: I placed Obsidian config directory inside repository, containing automatically generated empty subdirectory. Then I tried to wire Fossil-based scripts in Obsidian GUI. I have had some other issues, so I decided not to deal with it for now. The "interactive" warning issue, however, was due to the fact that I used wildcards in "fossil ci" unnecessarily.

(6) By Trevor (MelvaigT) on 2024-05-04 14:02:26 in reply to 3 [link] [source]

I am uncomfortable when changes are proposed based on a single case. I always try to ask some good questions before reaching for the coding pen :-)

If I explicitly specify the single argument "some_dir/empty_subdir" and nothing else, what should fossil do? Bear in mind that depending on the OS or shell Fossil may not know if the original command contained a wild card. Either silence or a warning message followed by a success result seem wrong especially in the case of an automated script.

Why is it not acceptable to say that when this kind of situation arises the user is expected to list the unmanaged objects via an ignore pattern (assuming this works as I am assuming it would - I might be wrong here).

If we treat empty directories specially, are there other objects which deserve similar treatment? Might there be such in the future?

What is the principle on which the current code is based? As a user I am grateful for Fossil pointing out objects in the checkin which may have appeared without my noticing them - including empty directories.

Aside: regarding the change concerning . and .. is this really a problem? By fixing it are we introducing an issue for somebody who might refer to "a/b/c" as "a/x/../b/c" (for their own presumably sane reasons). AFAIK . and .. are not usually listed when you ask for the contents of a directory via API, any more than PRN and NUL are on Windows file systems.

(7) By Stephan Beal (stephan) on 2024-05-04 14:11:21 in reply to 6 [link] [source]

regarding the change concerning . and .. is this really a problem? By fixing it are we introducing an issue for somebody who might refer to "a/b/c" as "a/x/../b/c" (for their own presumably sane reasons)

The way that function is used wouldn't affect such inputs. The function would, in your example, be passed "a/x/../b/c" as the dir name, which is completely legitimate. Only the "magic" entries of "." and ".." are now filtered out of that function's count, and only when they are read via readdir(), so are unaffected by inputs like your example.

AFAIK . and .. are not usually listed when you ask for the contents of a directory via API, any more than PRN and NUL are on Windows file systems.

They're always returned by readdir(), rather unfortunately (IMO). The "." entry is entirely(?) superfluous and the ".." entry is only rarely useful.

(8) By Andy Bradford (andybradford) on 2024-05-04 14:29:41 in reply to 1.2 [link] [source]

> For empty  subdirectories, this  warning should  never be  emitted, as
> Fossil does not manage directory objects.

I gave it an  explicit set of files to "manage", one  of which turns out
to be an empty directory which Fossil does not manage. Given that Fossil
does not manage directories is not the presence of an empty directory an
arguably noteworthy event?  Seems like I should be warned  of that fact,
perhaps that's not what I really intended.

Andy