[virt-tools-list] [virt-viewer][PATCH 2/6] events: remove timeout and handle from arrays

[Christophe Fergeau]

On Fri, Jul 17, 2015 at 06:39:13PM +0200, Fabiano Fidêncio wrote:
> On Fri, Jul 17, 2015 at 6:15 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > Hey,
> >
> > On Fri, Jul 17, 2015 at 04:01:19PM +0200, Fabiano Fidêncio wrote:
> >> Based on
> >> http://libvirt.org/git/?p=libvirt-glib.git;a=commit;h=cff5f1c46f4b9661e112b85159fb58ae473a9a89
> >
> > I'd tend to c&p the initial log too
> 
> Not sure if the initial log would make sense here. But I can c&p it, no problem.
> 
> >
> >>
> >> Related to: rhbz#1243228
> >> ---
> >>  src/virt-viewer-events.c | 97 +++++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 63 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/src/virt-viewer-events.c b/src/virt-viewer-events.c
> >> index 6154353..b92506c 100644
> >> --- a/src/virt-viewer-events.c
> >> +++ b/src/virt-viewer-events.c
> >> @@ -39,7 +39,7 @@ struct virt_viewer_events_handle
> >>      int watch;
> >>      int fd;
> >>      int events;
> >> -    int enabled;
> >> +    int removed;
> >
> > This (and other parts of this commit) are
> > https://libvirt.org/git/?p=libvirt-glib.git;a=commitdiff;h=3e73e0cee977fb20dd29db3ccfe85b00cc386c43
> > they should go in a separate commit
> 
> As it was not exactly cherry-picked from the libvirt-glib commits, I
> don't see a good reason for not having all the fixes squashed when
> they make sense to be squashed.

It makes reviewing much more mechanical if each of your patch corresponds
to exactly one libvirt-glib commit. At the very least, mention all
commits that were squashed together in the commit log, it's very
misleading as you did it currently, I did not initially realize several
commits were squashed and I was wondering why you needed to do these
changes compared to the initial commit.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20150720/25c76fff/attachment.sig>