| Message ID | 1274167881-6966-2-git-send-email-corentincj@iksaif.net |
|---|---|
| State | New |
| Headers | show |
On 18.05.2010, at 09:31, Corentin Chary wrote: This is missing a patch description. When people later on either cherry-pick your commits or simply git show them, it's almost impossible to know what's going on. So please always put in a patch description. Alex > Signed-off-by: Corentin Chary <corentincj@iksaif.net> > --- > vnc.c | 19 ++++++++++--------- > 1 files changed, 10 insertions(+), 9 deletions(-)
On 18.05.2010, at 09:31, Corentin Chary wrote:
> Signed-off-by: Corentin Chary <corentincj@iksaif.net>
I'm also missing a cover letter explaining what your patch set does. As a general rule of thumb, whenever your patch set spans more than 2 patches, a cover letter is really helpful. To generate one, use git-format-patch --cover-letter.
Alex
On Tue, May 18, 2010 at 11:19 AM, Alexander Graf <agraf@suse.de> wrote: > > On 18.05.2010, at 09:31, Corentin Chary wrote: > >> Signed-off-by: Corentin Chary <corentincj@iksaif.net> > > I'm also missing a cover letter explaining what your patch set does. As a general rule of thumb, whenever your patch set spans more than 2 patches, a cover letter is really helpful. To generate one, use git-format-patch --cover-letter. > > > Alex > > > There is a cover-letter, but I accidentally put a '\n' and the beginning ( see http://thread.gmane.org/gmane.comp.emulators.qemu/70495 ).
On 18.05.2010, at 11:53, Corentin Chary wrote: > On Tue, May 18, 2010 at 11:19 AM, Alexander Graf <agraf@suse.de> wrote: >> >> On 18.05.2010, at 09:31, Corentin Chary wrote: >> >>> Signed-off-by: Corentin Chary <corentincj@iksaif.net> >> >> I'm also missing a cover letter explaining what your patch set does. As a general rule of thumb, whenever your patch set spans more than 2 patches, a cover letter is really helpful. To generate one, use git-format-patch --cover-letter. >> >> >> Alex >> >> >> > > There is a cover-letter, but I accidentally put a '\n' and the > beginning ( see > http://thread.gmane.org/gmane.comp.emulators.qemu/70495 ). I see :). Sounds like you should just rework all the patch descriptions so people actually know what's going on within the patch without reading it and then give it a go for v2. Alex
On Tue, May 18, 2010 at 11:18 AM, Alexander Graf <agraf@suse.de> wrote: > > On 18.05.2010, at 09:31, Corentin Chary wrote: > > This is missing a patch description. When people later on either cherry-pick your commits or simply git show them, it's almost impossible to know what's going on. > So please always put in a patch description. > > > Alex > In this case, the description is all in the subject and the patch is pretty obvious. Should I really add something like "Create a new set_encoding() function to remove duplicate code in set_encodings()." ?
On 18.05.2010, at 11:56, Corentin Chary wrote: > On Tue, May 18, 2010 at 11:18 AM, Alexander Graf <agraf@suse.de> wrote: >> >> On 18.05.2010, at 09:31, Corentin Chary wrote: >> >> This is missing a patch description. When people later on either cherry-pick your commits or simply git show them, it's almost impossible to know what's going on. >> So please always put in a patch description. >> >> >> Alex >> > > In this case, the description is all in the subject and the patch is > pretty obvious. > Should I really add something like "Create a new set_encoding() > function to remove duplicate code in set_encodings()." ? Yes. If possible, including the reasoning behind it. Something like: Currently the code to specify which encoding we use is written out individually every time. Since we want to be able to add code to every singe occurence, let's move it to a helper function. Alex
diff --git a/vnc.c b/vnc.c index 1f7ad73..a91c3a3 100644 --- a/vnc.c +++ b/vnc.c @@ -1587,6 +1587,13 @@ static void send_ext_audio_ack(VncState *vs) vnc_flush(vs); } +static void set_encoding(VncState *vs, int encoding) +{ + if (vs->vnc_encoding == -1) { + vs->vnc_encoding = encoding; + } +} + static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings) { int i; @@ -1603,24 +1610,18 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings) enc = encodings[i]; switch (enc) { case VNC_ENCODING_RAW: - if (vs->vnc_encoding != -1) { - vs->vnc_encoding = enc; - } + set_encoding(vs, enc); break; case VNC_ENCODING_COPYRECT: vs->features |= VNC_FEATURE_COPYRECT_MASK; break; case VNC_ENCODING_HEXTILE: vs->features |= VNC_FEATURE_HEXTILE_MASK; - if (vs->vnc_encoding != -1) { - vs->vnc_encoding = enc; - } + set_encoding(vs, enc); break; case VNC_ENCODING_ZLIB: vs->features |= VNC_FEATURE_ZLIB_MASK; - if (vs->vnc_encoding != -1) { - vs->vnc_encoding = enc; - } + set_encoding(vs, enc); break; case VNC_ENCODING_DESKTOPRESIZE: vs->features |= VNC_FEATURE_RESIZE_MASK;
Signed-off-by: Corentin Chary <corentincj@iksaif.net> --- vnc.c | 19 ++++++++++--------- 1 files changed, 10 insertions(+), 9 deletions(-)