On 2011/09/29 15:03, Steven Blackburn <st...@beeka.org> wrote: > After the pointer to git interactive adding (and with additional help from > google regarding hard/soft resets and stashes), I have split the patch into > a "tidying" one and a "feature" one. I have added fuller commit messages, > corrected the indentation error (was spaces instead of tabs in one place) > and removed logging of unreachable code. > > While googleing for generally accepted approaches for git commit messages, I > realised the cause of confusion is that the web-based interface to git (at > git.musicpd.org) shows only brief messages and I thought this was the whole > commit message. I have since found that it is common for git-based systems > to show the first line/paragraph of a commit message in summary lists and > that the MPD commits are often more detailed. I have attempted to format my > commit message in a way that is compatible with this approach. So my > original (invalid) point was: MPD only has very brief messages, so why are > you giving me such a hard time. But I now realise we were talking at cross > purposes as MPD commit messages have detail where needed, just that I didn't > see it (being new to git / MPD). > > I hope the new patches address your concerns. Let me know what you think.
"Free the client input buffer if it exists" - Why this? Does it fix a memory leak, or is it just refactoring? "Tweak logic in httpd_client_read() so that peer disconnection" - is that the part that moves the "state==RESPONSE" check into the "switch"? How does it detect peer disconnection? What is the advantage here? I don't understand the intention of that patch either. "Add more logging to aid debugging communication problems" - I would not log "peer disconnected", and even less as a "warning". It's a normal event, and if you run a big radio station, your log gets filled with these boring messages. The user (non-developer) will probably not know what "fifo is not empty" means. And I would not log verbatim (unquoted) text received from the socket; I would fear it could be used, for example, to put ANSI codes on the admin's terminal, clearing log lines with warnings about break-in attempts. I do not agree with "Unhandled status from read" either, because adding a "default:" will suppress gcc warnings about missing "case" statements. Not dealing with a documented IO status is a bug, and if you want to detect such a bug, write an assertion instead of a log message. (Since you combined three different changes in one patch, I cannot pick just one patch and merge it - we have to get all of it right at the same time) I have merged your DLNA patch, but I have fixed indentation and I have removed the g_warning() calls, because I wanted to discuss how we should do that: - should not be g_warning(), because it is an informational message - is logging an event about a client useful if you cannot identify the client? It doesn't show the IP address or any other identification of the client. If we do want to log client events, we should assign each client an id/name (could be his IP address, or a serial number), and use this string in each log message. Max P.S.: try "stgit" for refining patches. It's a wonderful tool. ------------------------------------------------------------------------------ All of the data generated in your IT infrastructure is seriously valuable. Why? It contains a definitive record of application performance, security threats, fraudulent activity, and more. Splunk takes this data and makes sense of it. IT sense. And common sense. http://p.sf.net/sfu/splunk-d2dcopy2 _______________________________________________ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
