I'll add a fifth case, the copy

5) copy of a file, (from any partition to ,of course, a monitored dir)

- IN_CREATE (-> EVENT_CREATED)
- file open() for writing (IN_OPEN is ignored by GIO).
- multiple write() -> IN_MODIFY -> EVENT_CHANGED
- close() -> IN_CLOSE_WRITE -> EVENT_CHANGES_DONE_HINT

A zero-length file (touch foo) is a subcase of 5, in which just a EVENT_CHANGED is emitted, not real a special case.

This is the same of case 1 and does not need changes.

Created attachment 179523 [details] [review]
patch from Cosimo: Send CHANGES_DONE_HINT on file moves if no IN_CLOSE_WRITE is emitted

Attaching a patch on Cosimo's behalf, so I can review it :-)

Review of attachment 179523 [details] [review]:

Not marking this as reviewed because I'm not a GLib committer, but I think this looks fine apart from some naming/typo nitpicking, and the semantics are sensible.

::: gio/inotify/inotify-helper.c
@@ +199,3 @@
+   * a not monitored directory.
+   *
+   * It's not needed in cases like moves across mounted voulmes as the IN_CREATE

"mounted volumes as"

@@ +203,3 @@
+   * Also not needed if sub->pair_moves is set as EVENT_MOVED will be emitted
+   * instead of EVENT_CREATED which implies no further modification will be
+   * applied to the file */

This big comment should contain the URL to this bug, for future maintainers' benefit :-)

@@ +206,3 @@
+  if ((event->other_pair && !sub->pair_moves && (event->mask & IN_MOVED_TO)))
+    g_file_monitor_emit_event (G_FILE_MONITOR (sub->user_data),
+        child, NULL, G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT);

For those following along at home, this is case 3a: the event is the second in a pair (MOVED_TO), and we're not coalescing the pair into a single EVENT_MOVED, so we must have represented the first in the pair as IN_MOVED_FROM which becomes EVENT_CREATED. We need to emit EVENT_CHANGES_DONE_HINT to distinguish this from a "case 1" that just hasn't finished writing yet.

Since only moves are paired (nothing else would make sense), and MOVED_TO is always the second of a pair, we could probably even get rid of the "event->other_pair" condition?

@@ +211,3 @@
+      (event->original_mask & IN_MOVED_TO) && (event->mask & IN_CREATE))
+    g_file_monitor_emit_event (G_FILE_MONITOR (sub->user_data),
+        child, NULL, G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT);

This is case 2, the other problematic one: it was IN_MOVED_TO at the kernel level, but is turned into IN_CREATE in inotify-kernel.c, because inotify-kernel.c guarantees that it never emits IN_MOVED_{TO,FROM} except in pairs.

Again, we need to emit EVENT_CHANGES_DONE_HINT to distinguish this from a "case 1" that just hasn't finished yet.

::: gio/inotify/inotify-kernel.h
@@ +32,3 @@
   char *  name;
+  gboolean other_pair; /* TRUE when the second element of a paired event */
+  struct ik_event_s *pair; /* the second element of a paired event */

I think other_pair would be clearer named is_paired_move_to, or is_second_event, or something, if we need it at all (but see review of inotify-helper.c for why we might not).

Regardless, I think this'd be a better comment:

/* TRUE if this event is the MOVE_TO in a pair of
 * MOVE_FROM, MOVE_TO events */

and this'd be a better comment for @pair:

/* if move_from and move_to are paired MOVE_FROM and MOVE_TO
 * events, then move_from->pair == move_to and
 * move_to->pair == NULL */

I'd personally prefer to rename @pair to "move_to" or "second_event" or something to indicate the direction of linking, but "pair" is the existing naming, so, whatever.

Lets wait until 2.29 opens, at this point.

(In reply to comment #3)

> Since only moves are paired (nothing else would make sense), and MOVED_TO is
> always the second of a pair, we could probably even get rid of the
> "event->other_pair" condition?

We need a way to distinguish case 3a - where a pairing is done - from other cases in which MOVE_TO can appear also.
In case 3a, we need to identify the last event in the pair, after which we can emit the new GIO's event.

Unfortunately we do not have a way to identify that an event is part of a pair, if we are looking at the second element. Using a list terminology, there is no ->prev.

There's a way to write the two guards without using event->other_pair, or even joining it in one guard:

  if (!ih_event_is_paired_move (event) &&
      (event->original_mask & IN_MOVED_TO) && (event->mask & IN_CREATE))
    g_file_monitor_emit_event (G_FILE_MONITOR (sub->user_data),
        child, NULL, G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT);
  else if (event->pair == NULL && !sub->pair_moves &&
      (event->mask & IN_MOVED_TO))
    g_file_monitor_emit_event (G_FILE_MONITOR (sub->user_data),
        child, NULL, G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT);

which allows to not introduce event->other_pair.

I didn't test extensively, but it should to the work, it's just much obscure to read, as ih_event_is_paired_move() does not actually do what you'd think it does: it detects the *first* element of a paired move. I'd think, by its name, that it'd detect also the second, being it actually part or a "paired_move".

That's why in the second guard I used directly event->pair, to avoid let people
thinkng I don't wont a paired event. I just want to filter out the first
element of the pair (still, event->pair does not help much with the name :)

Were you thinking about it?
I don't like this solution very much, too cryptic and based on internal behaviours, but completely suitable.
It has the advantage it does not introduce event->other_pair, though its introduction does not change anything and makes things just clearer. In both cases there would need some renaming to make clear the involved semantic.


> @@ +211,3 @@
> +      (event->original_mask & IN_MOVED_TO) && (event->mask & IN_CREATE))
> +    g_file_monitor_emit_event (G_FILE_MONITOR (sub->user_data),
> +        child, NULL, G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT);
> 
> This is case 2, the other problematic one: it was IN_MOVED_TO at the kernel
> level, but is turned into IN_CREATE in inotify-kernel.c, because
> inotify-kernel.c guarantees that it never emits IN_MOVED_{TO,FROM} except in
> pairs.
> 
> Again, we need to emit EVENT_CHANGES_DONE_HINT to distinguish this from a "case
> 1" that just hasn't finished yet.
> 
> ::: gio/inotify/inotify-kernel.h
> @@ +32,3 @@
>    char *  name;
> +  gboolean other_pair; /* TRUE when the second element of a paired event */
> +  struct ik_event_s *pair; /* the second element of a paired event */
> 
> I think other_pair would be clearer named is_paired_move_to, or
> is_second_event, or something, if we need it at all (but see review of
> inotify-helper.c for why we might not).

I prefer is_second_event or is_second_of_pair, according to the naming and semantic the modules are using:

It seems to me that in -helper.c and -kernel.c the tendency is to keep separated the fact that there is a paired event and the implication that it will be a file move, abstracting it.
I actually agree.

ik_pair_events() pairs events based on the presence of the kernel's cookie, whatever the actual events are and no assumption in the code is done that they need to be a _MOVE_* event (though, they actual are). Actually checks are done on paired events to be sure that they are moves.

ih_event_is_paired_move() check both the presence of pair and the fact the the two events in the pair are actual src/dst of a move.

That's why I do not assume in the if guards that the second element of the pair is MOVE_TO but I check it.

> Regardless, I think this'd be a better comment:
> 
> /* TRUE if this event is the MOVE_TO in a pair of
>  * MOVE_FROM, MOVE_TO events */

> and this'd be a better comment for @pair:
> 
> /* if move_from and move_to are paired MOVE_FROM and MOVE_TO
>  * events, then move_from->pair == move_to and
>  * move_to->pair == NULL */

I Agree with the improvements. Keeping the semantic as above:

/* TRUE if this event is the last element of a pair, i.e.,
 * MOVE_TO in a pair of MOVE_FROM, MOVE_TO events */

/* if event1 and event2 are two paired events
 * (i.e., MOVE_FROM and MOVE_TO events related to the same file move),
 * then event1->pair == event2 and event2->pair == NULL */

> I'd personally prefer to rename @pair to "move_to" or "second_event" or
> something to indicate the direction of linking, but "pair" is the existing
> naming, so, whatever.

I actually like @pair, for the reasons expressed above :-)

As said, ih_event_is_paired_move() should be renamed to reflect the fact that it catches only the case in which the event it first in a moved pair.

And probably something else.

thanks for the review :)

(In reply to comment #5)
> I didn't test extensively, but it should to the work, it's just much obscure to
> read

Yeah, you're right. Keep it like it is now, perhaps with some struct-member renaming, IMO.

> I prefer is_second_event or is_second_of_pair, according to the naming and
> semantic the modules are using:
> 
> It seems to me that in -helper.c and -kernel.c the tendency is to keep
> separated the fact that there is a paired event and the implication that it
> will be a file move, abstracting it.

Fair enough, call it something involving "second" then.

> I Agree with the improvements. Keeping the semantic as above:
> 
> /* TRUE if this event is the last element of a pair, i.e.,
>  * MOVE_TO in a pair of MOVE_FROM, MOVE_TO events */

You mean "e.g." ("for example" or "for instance"), not "i.e." ("that is").

i.e. would imply that MOVE_TO is the only possible reason, which is what you're trying to avoid; e.g. implies that MOVE_TO is one of several possible reasons.

Native English speakers get this distinction wrong too, though. It might be better to spell it out as "for instance".

> As said, ih_event_is_paired_move() should be renamed to reflect the fact that
> it catches only the case in which the event it first in a moved pair.

Makes sense; ih_event_is_first_in_pair()?

(In reply to comment #6)
> > As said, ih_event_is_paired_move() should be renamed to reflect the fact that
> > it catches only the case in which the event it first in a moved pair.
> Makes sense; ih_event_is_first_in_pair()?

better ih_event_is_first_in_paired_move(), which is quite long but explains what it does. It actually checks it's a move and a paired event.

event->is_second_in_pair, consequently, might be a reasonable name for the new member.

Created attachment 179788 [details] [review]
Patch after Simon's 1st review

Attached patch with fixes after Simon reviews,
also I joined the to "if"s in -helper.c to a single one.

No rename has been applied yet to existing symbols.

Review of attachment 179788 [details] [review]:

Again, I'm not a GLib committer, but I think this is fine now.

We'd appreciate any review you can provide on this, even if it can't be committed just yet (we're applying this in Maemo already).

Any chance someone can review this for 2.31?

Review of attachment 179788 [details] [review]:

I like the concept of pairs, well prepared sources.

Looks good to me, tested locally, working fine as expected. Please commit.

::: gio/inotify/inotify-helper.c
@@ +209,3 @@
+        event->is_second_in_pair && (event->mask & IN_MOVED_TO)) ||
+      (!ih_event_is_paired_move (event) &&
+       (event->original_mask & IN_MOVED_TO) && (event->mask & IN_CREATE)))

This test is huge but with the help of the comment above plus the indentation it's readable after a closer look.

Thanks, fixed in git for 2.31.3 (fd1e9938) using Cosimo's patch.