I forgot to add, nothing of this happens with Xwayland clients, which behave fine except for not being scaled up at all.
Xwayland gtk+ apps, which support scaling in the toolkit, have the proper full functionality.

So the issue here is that the coordinates of the "configure" event should be in logical pixels, but currently it's in device pixels.

So, if you have a monitor with 800x600 resolution at scale 2, then you have 400x300 logical pixels, and when you maximize the window, you should get an   xdg_surface.configure(400, 300, STATE_MAXIMIZED); event.

If the surface has HiDPI rendering, then it will make an 800x600 buffer, render into it, and submit it with buffer size 2, and so it will map 1:1 on the screen. All fine.

If the surface doesn't have HiDPI rendering, then it will make a 400x300 buffer, render into it, and mutter will scale it up to 800x600.

...

The issue is that we send an event in device pixels, always. The work area is in device pixels, the monitor size is in device pixels, so we just sort of have always worked in device pixels.

Cleaning this up will be super tricky, since it involves changing around the constraints and multimonitor code.

For now, I've pushed a hack where you can override the size of all monitors by setting the org.gnome.desktop.interface.scaling-factor key to something greater than one, so your windows aren't cut off.

Er, greater than zero. Like one.

(In reply to Jasper St. Pierre from comment #2)
> 
> The issue is that we send an event in device pixels, always. The work area
> is in device pixels, the monitor size is in device pixels, so we just sort
> of have always worked in device pixels.
> 
> Cleaning this up will be super tricky, since it involves changing around the
> constraints and multimonitor code.

Can't you do the conversion only at the protocol level?
Ie, keep everything in device pixels internally, and convert to logical pixels as you're about to send the configure event to the client.

That would work if we had a way to convert to the protocol level. We can't take "configure(800, 600)" and always divide it by two, because what happens if you have a HiDPI monitor and a LowDPI one? Which scale factor do we obey, and when?

There's too much context lost at that point.

Mh? You know the window you're sending the event for, so you know its monitor and therefore the scale...

I (In reply to Jasper St. Pierre from comment #5)
> That would work if we had a way to convert to the protocol level. We can't
> take "configure(800, 600)" and always divide it by two, because what happens
> if you have a HiDPI monitor and a LowDPI one? Which scale factor do we obey,
> and when?
> 
> There's too much context lost at that point

I don't think there's any context lost - Mutter knows everything relevant.

There is some definite confusion in the mutter codebase. I took a look at how mouse events are handled (meta_wayland_pointer_get_relative_coordinates) and there we divide the coordinates by the surface's scale. This is wrong, since the surface's scale is purely an *output* property. The handling of configure events and mouse events should be the same, and should report coordinates in what Mutter considers to be the logical coordinates for the window.

The reason we get into conceptual trouble is that the when Mutter reports a value for the scale of an output, it's a promise of how it's going to handle surface contents displayed on that output. If a surface has a scale factor of 1 then it should be displayed literally on an output with a scale factor of 1, and displayed scaled up by 2 on an output with a scale factor of 2. But Mutter currently draws each window only once, so we're lying to the application!

Luckily, a Wayland application has no way to know way to know we're lying :-) so what we need to do short term is to pick *one* output for each window - this is the meta_window_get_monitor()

 Coordinates sent to the window - in configure events and in pointer events should be divided by the scale factor of the output
 The surface should be displayed scaled up by scale_factor_of_output/surface_scale

The trickiest bit here is that when we change the output selected for the window to a different scale then we need to either a) resize the window b) send a configure event to the window - a) is the better thing to do.

(In reply to Owen Taylor from comment #7)
> The trickiest bit here is that when we change the output selected for the
> window to a different scale then we need to either a) resize the window b)
> send a configure event to the window - a) is the better thing to do.

No, that's incorrect. If I have an 800x600 monitor on the left and a 1600x1200/x2 monitor on the right, and I fullscreen the app, both configure events will be "xdg_surface.configure(800, 600, fullscreen)". Configure events are in *logical* pixels, and so sending it another configure event won't do anything. We let it know that it entered the x2 monitor through wl_surface.enter, and then if it decides to attach an x2 buffer in response, we lay out the scene better.


I'm confused by "scale_factor_of_output / surface_scale". "surface_scale" is already the ratio of the output to the buffer size. Did you mean "scale_factor_of_output / buffer_scale" ?

Keep in mind that there's lots more that we should be doing. Subsurface position is also in logical pixels, for instance, while we currently treat it as device pixels (and we currently get it super wrong for nested subsurfaces, I believe).

If we decide to simply switch over literally everything when the window changes monitors (which is what I want to do, but you didn't seem to receptive to that), then we can pick a monitor and a scale and lay out the scene in device pixels according to that monitor, and then when the monitor changes again, we lay out the scene again according to the new monitor, etc.

(In reply to Jasper St. Pierre from comment #8)
> (In reply to Owen Taylor from comment #7)
> > The trickiest bit here is that when we change the output selected for the
> > window to a different scale then we need to either a) resize the window b)
> > send a configure event to the window - a) is the better thing to do.
> 
> No, that's incorrect. If I have an 800x600 monitor on the left and a
> 1600x1200/x2 monitor on the right, and I fullscreen the app, both configure
> events will be "xdg_surface.configure(800, 600, fullscreen)". Configure
> events are in *logical* pixels, and so sending it another configure event
> won't do anything. We let it know that it entered the x2 monitor through
> wl_surface.enter, and then if it decides to attach an x2 buffer in response,
> we lay out the scene better.

If a window is 100x100 in physical pixels on the 800x600 monitor, and we move it to the 1600x1200 window, I'm saying to be consistent we need to either:

 A) Change Mutter's internal accounting of the size so that it's now 100x100

OR:

 B) Send a configure event to 50x50

What you are saying is that it doesn't make sense to do is to send a configure event at 100x100 and I agree. Adel pointed out to me on IRC that if the app notices the change in output and scale factor and attaches a new buffer at scale factor 2 but the old size, then things get synced back up. But the app is *not required to do this* - and what will happen then is that we will display the old buffer for the client scaled up by two (so 200x200 physical pixels), but the metacity core code will think that the window is 100x100 physical pixels. Anything that is done in *actor* terms will work properly, but I can easily imagine bugs appearing - as one example, computing whether a new window overlaps for positioning won't work right.

> I'm confused by "scale_factor_of_output / surface_scale". "surface_scale" is
> already the ratio of the output to the buffer size. Did you mean
> "scale_factor_of_output / buffer_scale" ?

Sorry - I wasn't aware of an existing meaning for surface_scale - I just meant surface->scale, which, if I'm not mistaken is the scale of the currently attached buffer.

> Keep in mind that there's lots more that we should be doing. Subsurface
> position is also in logical pixels, for instance, while we currently treat
> it as device pixels (and we currently get it super wrong for nested
> subsurfaces, I believe).

Hopefully patches in bug 745655 help with this.
 
> If we decide to simply switch over literally everything when the window
> changes monitors (which is what I want to do, but you didn't seem to
> receptive to that), then we can pick a monitor and a scale and lay out the
> scene in device pixels according to that monitor, and then when the monitor
> changes again, we lay out the scene again according to the new monitor, etc.

There are two different things:

 A) Short term, we need to have a consistent interpretation within the Mutter code base, and not have bugs and race conditions where things go wacky. The best way to do this is to pick *an output* for each window, draw the window according to that output's scale factor, and, when sending events to the window, consider the drawing on that window to represent the "logical coordinates".

 B) Longer term, I do consider that it's important to have overlapping windows handled correctly. I don't think any of a) multiple monitors b) monitors with different resolutions c) the UI paradigm of dragging a window from one monitor to another will go away. But that requires a bigger change - it's not compatible with the idea that the entire display is just one big undifferentiated area of physical pixels which we blit the window onto as a whole.

(In reply to Owen Taylor from comment #9)
> Anything that is done in *actor* terms will work
> properly, but I can easily imagine bugs appearing - as one example,
> computing whether a new window overlaps for positioning won't work right.

For our cascading algorithm, is having it work in device pixels sane? On the HiDPI monitor, cascading windows are placed half down and to the right as the other ones. *Any* placement or constraint algorithms in terms of device pixels is going to give poor results at best, and wrong results at worst.

The only sane option in any sense is to treat the window's rectangle as logical pixels, but there's a lot of hell to get there. We already went through it with the frame rect, and I'm honestly going to admit I'm a bit burned out from that, and I'm not eager to do it again. And then we have more to fix up, since things like struts and the workarea are in device pixels, and they span multiple monitors, making it

If you want to do some temporary fixups here and there to get mostly OK results so that things sort of work if you don't stare at them too close (cascading will fail, sometimes windows will be placed in weird ways and sometimes we think they're not onscreen when they totally are, the workarea is sort of fudgy, etc.) then feel free to that.

This whole thing is sort of amplified because I don't have any HiDPI equipment, and it's not really that important to my current employer right now. I'm not happy about patches like the ones in bug 745655, since they're more hacks on top of what we have already, and it's hard to keep the logic straight, so that's why I haven't really been excited about reviewing them.

(In reply to Jasper St. Pierre from comment #10)
> (In reply to Owen Taylor from comment #9)
> > Anything that is done in *actor* terms will work
> > properly, but I can easily imagine bugs appearing - as one example,
> > computing whether a new window overlaps for positioning won't work right.
> 
> For our cascading algorithm, is having it work in device pixels sane? On the
> HiDPI monitor, cascading windows are placed half down and to the right as
> the other ones. *Any* placement or constraint algorithms in terms of device
> pixels is going to give poor results at best, and wrong results at worst.
> 
> The only sane option in any sense is to treat the window's rectangle as
> logical pixels, but there's a lot of hell to get there. We already went
> through it with the frame rect, and I'm honestly going to admit I'm a bit
> burned out from that, and I'm not eager to do it again. And then we have
> more to fix up, since things like struts and the workarea are in device
> pixels, and they span multiple monitors, making it

After having looked at this some more, I don't think its necessarily the best idea to make the rects in MetaWindow in logical pixels while the canvas we draw to is strictly in physical pixels, because we'd have to put up magic scale conversion everywhere an actor does anything with a window. For a short term duct taped solution, it seems much simpler to make MetaWindow rects in the same coordinate space as its actors, output configuration and more or less everything else. When we have a way to make our output canvas use logical pixels we can probably fix it without the loads of hacks that both mixing coordinate spaces or scaling regions back and forth mean.

> 
> If you want to do some temporary fixups here and there to get mostly OK
> results so that things sort of work if you don't stare at them too close
> (cascading will fail, sometimes windows will be placed in weird ways and
> sometimes we think they're not onscreen when they totally are, the workarea
> is sort of fudgy, etc.) then feel free to that.

Most of the HiDPI functionality we have is more or less a temporary work arounds until we have a more sane scene graph coordinate space. If we want to have some level of HiDPI support before attacking the scene graph coordinate space problem we need these hacks.

> 
> This whole thing is sort of amplified because I don't have any HiDPI
> equipment, and it's not really that important to my current employer right
> now. I'm not happy about patches like the ones in bug 745655, since they're
> more hacks on top of what we have already, and it's hard to keep the logic
> straight, so that's why I haven't really been excited about reviewing them.

I have a git stash that makes the nested mutter have two outputs with different output scales. Should I make that a proper patch so things gets easier to test maybe?

(In reply to Jasper St. Pierre from comment #10)
> (In reply to Owen Taylor from comment #9)
> > Anything that is done in *actor* terms will work
> > properly, but I can easily imagine bugs appearing - as one example,
> > computing whether a new window overlaps for positioning won't work right.
> 
> For our cascading algorithm, is having it work in device pixels sane? On the
> HiDPI monitor, cascading windows are placed half down and to the right as
> the other ones. *Any* placement or constraint algorithms in terms of device
> pixels is going to give poor results at best, and wrong results at worst.

The cascading algorithm would actually prefer to use the titlebar height as the cascade distance - which would work fine on hidpi without regard to the coordinate system. You are probably right that with CSD we are falling back to some arbitrary value like 10 device pixels, but I don't see this as being particularly relevant to this conversation - it doesn't have anything to do with the coordinate system of a window and could be fixed separately or ignored for now.

> The only sane option in any sense is to treat the window's rectangle as
> logical pixels, but there's a lot of hell to get there. We already went
> through it with the frame rect, and I'm honestly going to admit I'm a bit
> burned out from that, and I'm not eager to do it again. And then we have
> more to fix up, since things like struts and the workarea are in device
> pixels, and they span multiple monitors, making it

From experience, multiplication and division are less problematical to get right than addition and subtraction, and I think there might not be the same sort of infinite pile of arithmetical bugs to deal with. But though I do think we need to switch to logical coordinates in Mutter eventually, I don't want to make the work sound minimal - there are definitely some big issues with trying to switch over - another thing that is going to be problematical is what to do with X windows that are a non-integer number of device pixels - and a lot of code to change.

This is one reason that spending some time on the current code base and getting it working as well as possible worthwhile. The other reason is that we have a stable release of GNOME coming out and there are some things that are pretty broken - windows jumping to half size, popups showing up in the wrong place, etc, so some incremental fixes are definitely useful!

> If you want to do some temporary fixups here and there to get mostly OK
> results so that things sort of work if you don't stare at them too close
> (cascading will fail, sometimes windows will be placed in weird ways and
> sometimes we think they're not onscreen when they totally are, the workarea
> is sort of fudgy, etc.) then feel free to that.
> 
> This whole thing is sort of amplified because I don't have any HiDPI
> equipment, and it's not really that important to my current employer right
> now. I'm not happy about patches like the ones in bug 745655, since they're
> more hacks on top of what we have already, and it's hard to keep the logic
> straight, so that's why I haven't really been excited about reviewing them.

I don't see anything that qualifies as a hack, either in a good or bad sense, in forming a consistent understanding of the coordinate systems that are involved and fixing bugs where we are handling them wrong. Sound more like basic software engineering :-)

(In reply to Jonas Ådahl from comment #11)


> 
> I have a git stash that makes the nested mutter have two outputs with
> different output scales. Should I make that a proper patch so things gets
> easier to test maybe?


I would love that, personally (No hidpi hw here either).

Created attachment 299765 [details] [review]
meta-wayland-pointer: Fix event scaling

Divide by the output's scale factor not the surface's.
The coordinates we send to the window should be unscaled.

Created attachment 300263 [details] [review]
MetaMonitorInfo: Provide scale information

https://bugzilla.gnome.org/show_bug.cgi?id=744932

Created attachment 300264 [details] [review]
wayland: Scale window geometry rects given the main output

Since we scale surface actors given what main output their toplevel
window is on, also scale the window geometry coordinates and sizes
(window->rect size and window->custom_frame_extents.top/left) in order
to make the window geometry represent what is being rendered on the
stage.

Created attachment 300265 [details] [review]
MetaWaylandSurface: Return top most toplevel window for popups

Make meta_wayland_surface_get_toplevel_window return the top most window
in case its a chain of popups. This is to make all popups in a chain
including the top most surface have the same scale.

Created attachment 300266 [details] [review]
wayland: Take scale into account when placing windows relatively

When placing a popup and the legacy transient wl_shell_surface surfaces,
take the current scale of the window into account. This commit doesn't
fix relative positioning in case a window scale would change, but since
the use case for relative positioning is mostly popups, which would be
dismissed before the parent window would be moved, it should not be that
much of a problem.

Created attachment 300267 [details] [review]
wayland: Fix calculation of window geometry when scaled

Take the surface actor scale into account when calculating the window
geometry.

Review of attachment 299765 [details] [review]:

::: src/wayland/meta-wayland-pointer.c
@@ +626,3 @@
 
+  *sx = wl_fixed_from_double (xf) / output_scale;
+  *sy = wl_fixed_from_double (yf) / output_scale;

clutter_actor_transform_stage_point transforms the stage global position to buffer coordinates, meaning dividing with surface->scale is already correct. This patch breaks pointer motion events whenever output_scale != surface->scale, for example when running weston-clickdot on a HiDPI monitor.

Review of attachment 299765 [details] [review]:

I didn't actually test this patch that was the result of an IRC conversation but yeah you right its wrong.

Review of attachment 300263 [details] [review]:

Seems OK, but needs a better commit message. Mutter/GNOME Shell style is that all commit messages should have a body - even if just one sentence. There's basically always something useful to say - here probably it be describing the motivation.

Review of attachment 300264 [details] [review]:

Mostly makes sense to me with a few comments.

::: src/wayland/window-wayland.c
@@ +303,3 @@
+   * needed to avoid jumping back and forth between the new and the old, since
+   * changing main monitor may cause the window to be resized so that it no
+   * longer have that same new main monitor. */

Hmm, I don't mind that there's hysteresis in window->monitor, but I don't like the fact that meta_screen_get_monitor_for_window() can be different than window->monitor. Suggest

 s/meta_screen_get_monitor_for_window/meta_screen_compute_monitor_for_window/

and using that *only* when updating window->monitor, but in other places like keybindings.c, stack.c, using window->monitor.

@@ +373,3 @@
+  meta_compositor_sync_window_geometry (window->display->compositor,
+                                        window,
+                                        TRUE);

Doesn't the MetaWindow::size-changed signal need to be emitted? (I think it's fine if it's get emitted twice on a window resize that causes the window to change monitors and be rescaled.)

@@ +376,3 @@
+
+  /* The surface actor need to update their scale, recursively for itself
+   * and all its subsurfaces. */

Some sort of grammar problem here. Maybe "needs to update the scale recursively for itself and ..."

@@ +556,3 @@
+  new_geom.y *= monitor_scale;
+  new_geom.width *= monitor_scale;
+  new_geom.height *= monitor_scale;

Don't dx and dy also need to be scaled?

Review of attachment 300265 [details] [review]:

Seems to do what it says, but what I don't find find explained is why popups are different from other windows with transient parents. (Also is this dependent on a patch from some other patch series? I don't see this function in the current source)

Review of attachment 300266 [details] [review]:

Looks good

Review of attachment 300267 [details] [review]:

Makes sense

::: src/compositor/meta-surface-actor-wayland.c
@@ +133,3 @@
+
+  clutter_actor_get_position (CLUTTER_ACTOR (self), &x, &y);
+  *rect = (MetaRectangle) {

OK, so compound literals are cool and all that - but I don't see an advantage here over just assigning rect.x, rect.y, ... individually. (gcc *does* generate the same code .... it wasn't obvious to me that it wouldn't do a copy, but I checked)

Review of attachment 300267 [details] [review]:

::: src/compositor/meta-surface-actor-wayland.c
@@ +133,3 @@
+
+  clutter_actor_get_position (CLUTTER_ACTOR (self), &x, &y);
+  *rect = (MetaRectangle) {

The advantage is that code can be written more declarative than the old "do this then this then this then this" way. Writing declarative code is a good enough benefit by itself IMHO. Semantically its a copy, but there really is no point in caring about such things here as we're not in a busy loop, and as you say the compiler will optimize it away anyway.

Review of attachment 300264 [details] [review]:

::: src/wayland/window-wayland.c
@@ +303,3 @@
+   * needed to avoid jumping back and forth between the new and the old, since
+   * changing main monitor may cause the window to be resized so that it no
+   * longer have that same new main monitor. */

Sure.

@@ +373,3 @@
+  meta_compositor_sync_window_geometry (window->display->compositor,
+                                        window,
+                                        TRUE);

Hmm, I suppose so yes.

@@ +556,3 @@
+  new_geom.y *= monitor_scale;
+  new_geom.width *= monitor_scale;
+  new_geom.height *= monitor_scale;

Hmm. Yes, you are right. Would be good with a test case, because I've never seen dx, dy be != 0 (*puts on todo*).

(In reply to Owen Taylor from comment #24)
> Review of attachment 300265 [details] [review] [review]:
> 
> (Also is this
> dependent on a patch from some other patch series? I don't see this function
> in the current source)

Yes, sorry, missed to mention that. It depends on (in this order) 
bug 743617, bug 744453 and bug 745655.

Created attachment 301180 [details] [review]
MetaMonitorInfo: Provide scale information

The scale information is needed to determine what scale windows should
have in order to scale windows given what monitor it is on.

Created attachment 301181 [details] [review]
Don't calculate the main window monitor every time its needed

The main monitor of a window is maintained as 'window->monitor' and is
updated when the window is resized or moved. Lets avoid calculating it
everytime its needed.

Created attachment 301182 [details] [review]
wayland: Scale window geometry rects given the main output

Since we scale surface actors given what main output their toplevel
window is on, also scale the window geometry coordinates and sizes
(window->rect size and window->custom_frame_extents.top/left) in order
to make the window geometry represent what is being rendered on the
stage.

Created attachment 301183 [details] [review]
MetaWaylandSurface: Return top most toplevel window for popups

Make meta_wayland_surface_get_toplevel_window return the top most window
in case its a chain of popups. This is to make all popups in a chain
including the top most surface have the same scale.

The reason for this is that popups are mostly integrated part of the
user interface of its parent (such as menus). Having them in a different
scale would look awkward.

Note that this doesn't affect non-popup windows with parent-child
relationship, because such windows are typically not an integral part of
the user interface (settings window, dialogs, ..) and can typically be
moved independently. It would probably make sense to make attached modal
dialogs have the same scale as their parent windows, but modal dialogs
are currently not supported for Wayland clients.

(In reply to Jonas Ådahl from comment #31)
> Created attachment 301181 [details] [review] [review]
> Don't calculate the main window monitor every time its needed
> 
> The main monitor of a window is maintained as 'window->monitor' and is
> updated when the window is resized or moved. Lets avoid calculating it
> every time its needed.

s/its/it's/ and s/everytime/every time/ typos fixed locally.

Review of attachment 301180 [details] [review]:

can you fixup or remove meta_window_wayland_get_main_monitor_scale() has part of this patch?

::: src/backends/meta-monitor-manager.c
@@ +154,3 @@
       info = output->crtc->logical_monitor;
 
+      info->scale = output->scale;

Hmm, I don't think this handles mirroring properly - maybe put this inside the:

      if (output->is_primary || info->winsys_id == 0)
        info->winsys_id = output->winsys_id;

conditional?

Review of attachment 301180 [details] [review]:

Also - thanks for the improved commit message! it's definitely better, but to give an example
of what sort of information I'd expect there:

 Tracking back from the monitor to the output every time we need to figure
 out the scale of a window on a monitor is inconvenient, so propagate the
 scale from the output to the monitor it is associated with.

Review of attachment 301181 [details] [review]:

Looks good, thanks!

Review of attachment 301182 [details] [review]:

Looks good to commit to me other than one comment where I apparently was confusing when I suggested an edit.

::: src/wayland/window-wayland.c
@@ +375,3 @@
+                                        TRUE);
+
+  /* Needs to update the scale recursively for itself and any subsurfaces. */

When I said:

	/* The surface actor need to update their scale, recursively for itself
	* and all its subsurfaces. */

Some sort of grammar problem here. Maybe "needs to update the scale recursively for itself and ..." I actually meant:

        /* The surface actor needs to update the scale recursively for itself
         * and all its subsurfaces */

Sorry for the confusion :-)

Review of attachment 301183 [details] [review]:

OK, makes sense. I can see that, in the current world where we display a window at a single scale on all monitors, we wouldn't want a popup menu for a window that extended sufficiently far onto a different monitor with a different scale to suddenly change become huge or tiny. Hopefully popups that are on a different monitor than the main window are rare!

Created attachment 301972 [details] [review]
MetaMonitorInfo: Provide scale information

The scale information is needed to determine what scale windows should
have in order to scale windows given what monitor it is on.

Created attachment 301973 [details] [review]
wayland: Scale window geometry rects given the main output

Since we scale surface actors given what main output their toplevel
window is on, also scale the window geometry coordinates and sizes
(window->rect size and window->custom_frame_extents.top/left) in order
to make the window geometry represent what is being rendered on the
stage.

Review of attachment 301973 [details] [review]:

OK, looks good

Review of attachment 301973 [details] [review]:

OK, looks good

Review of attachment 301972 [details] [review]:

Code looks good, still perhaps would like a better body to the commit message as in my last commit message, unless you think there's something wrong about my suggestion,.

(In reply to Owen Taylor from comment #44)
> Review of attachment 301972 [details] [review] [review]:
> 
> Code looks good, still perhaps would like a better body to the commit
> message as in my last commit message, unless you think there's something
> wrong about my suggestion,.

Ah, missed to add that paragraph. No, it sounds correct.

Attachment 300266 [details] pushed as db6caa2 - wayland: Take scale into account when placing windows relatively
Attachment 300267 [details] pushed as f01247d - wayland: Fix calculation of window geometry when scaled
Attachment 301183 [details] pushed as fbd237b - MetaWaylandSurface: Return top most toplevel window for popups
Attachment 301972 [details] pushed as 441efd1 - MetaMonitorInfo: Provide scale information
Attachment 301973 [details] pushed as f6c9261 - wayland: Scale window geometry rects given the main output

Review of attachment 301181 [details] [review]:

This one was pushed as well. No idea why git bz missed it.