Please keep the mailing list on Cc, so all developers can participate.
On 2009/01/12 13:46, Gunnar Roth <gunnar.r...@gmx.de> wrote:
> Hi Max,
> first of all, thank you for taking the time to discuss the patch.
> I am very sorry for all that whitespace noise. i used araxis merge
> but had white changes on ignore.
>>
>> I saw you uploaded your win32 patch to the git repository
>> http://git.musicpd.org/cgit/rodion/mpd.git/, but I cannot merge this
>> patch. You did not obey the most basic rules I explained to you on
>> IRC.
>
> I am rather unsure and didnt take all into account, because i am new to
> this kind of procedure,
> but i try to learn.
>
>>
>>> commit 6c1d9df29ad22c6ef13528718a3d91fcff34819f
>>> Author: Gunnar Roth <gun...@garmedia.(none)>
>>
>> That email address is invalid.
> The comment of the editor told me that # lines will not be in the comit,
> but that menat probably only # i added myself. i did not edit the mail
> adress because i thought this was just informational for me.
The author field is public, and should contain useful information.
>>> Date: Sun Jan 11 21:43:40 2009 +0100
>>>
>>> changes to make mpd work on win32 using gcc.
>>>
>>> this patches misses all changes for visal c compiler, this will
>>> be on a different patch.
>>> it just should compile with mingw but not tested. i have tested,
>>> that it compiles on linux.
>>> so the changes should not break linux.
>>
>> You are mixing a huge amounts of different changes in one single
>> patch. Don't do this, please. Your patch description doesn't
>> describe the patch at all, it's generic blah.
> Ok
>
>>
>> And yes, you *are* breaking the Linux build:
>>
>> event_pipe.c: In function 'main_notify_event':
>> event_pipe.c:65: error: comparison of unsigned expression < 0 is
>> always false
> you seem to have a private setting making some warnings to error.
> there are some warnings in the windows build , sorry to have overlooked
> that.
It's "-Werror" (or "--enable-werror" on configure), and enabling it is
a good thing. Especially because your code was wrong. A "gsize"
cannot be negative, and thus your check could never succeed. That's a
good example which proves why "-Werror" is good - if you had enabled
it, you would have noticed your mistake.
>>> diff --git a/src/client.c b/src/client.c
>>> old mode 100644
>>> new mode 100755
>>> index e059f64..ac10811
>>> --- a/src/client.c
>>> +++ b/src/client.c
>>> @@ -34,9 +34,8 @@
>>> #include <stdio.h>
>>> #include <errno.h>
>>>
>>> -#ifdef WIN32
>>> -#include <ws2tcpip.h>
>>> -#include <winsock.h>
>>> +#ifdef _WIN32
>>> +#include <WinSock2.h>
>>
>> No uppercase letters in file names.
>>
> Ok its no problem on windows, but i accept this as a policy.
It's not just a "policy", it's to make sure that cross builds work.
>>> /** the uid of the client process, or -1 if unknown */
>>> @@ -114,6 +113,9 @@ static void client_write_deferred(struct client
>>> *client);
>>>
>>> static void client_write_output(struct client *client);
>>>
>>> +static void client_write_direct(struct client *client,
>>> + const char *data, size_t length);
>>> +
>>> bool client_is_expired(const struct client *client)
>>> {
>>> return client->fd < 0;
>>> @@ -166,7 +168,11 @@ static inline void client_set_expired(struct
>>> client *client)
>>> }
>
> i added this prototype because i replace a write by a client_write_direct()
>
>
>>> if (client->fd >= 0) {
>>> +#ifdef _WIN32
>>> + closesocket(client->fd);
>>> +#else
>>> close(client->fd);
>>> +#endif
>>> client->fd = -1;
>>> }
>>> }
>>
>> Instead of doing an #ifdef whenever a socket is closed, add a wrapper
>> for that. Too many #ifdefs make the code unreadable.
>>
>
> True, but you removed xClose() in git, and i dint want to add it again.
> So how should this wrapper be called? or just a #define closesocket(x)
> close(x)
> for non win32?
xclose() does not have anything to do with this.
Also: don't code in CPP when you can code in C. There are many
advantages in inline functions compared to macros.
>>> @@ -186,8 +192,12 @@ static void client_init(struct client *client,
>>> int fd)
>>> client->bufferPos = 0;
>>> client->fd = fd;
>>>
>>> +#ifdef _WIN32
>>> + client->channel = g_io_channel_win32_new_socket(client->fd);
>>> +#else
>>> client->channel = g_io_channel_unix_new(client->fd);
>>> - client->source_id = g_io_add_watch(client->channel, G_IO_IN,
>>> +#endif
>>> + client->source_id = g_io_add_watch(client->channel,
>>> G_IO_IN|G_IO_NVAL | G_IO_HUP | G_IO_ERR,
>>> client_in_event, client);
>
> add awrapper for g_io_channel_win32_new_socket?
> This is needed because fd and socket are not the same on windows.
Yes, you could wrap g_io_channel_win32_new_socket() /
g_io_channel_unix_new() in some OS dependent inline function, if you
want.
>
>>> client->lastTime = time(NULL);
>>> @@ -199,7 +209,7 @@ static void client_init(struct client *client,
>>> int fd)
>>>
>>> client->permission = getDefaultPermissions();
>>>
>>> - write(fd, GREETING, sizeof(GREETING) - 1);
>>> + client_write_direct(client, GREETING, sizeof(GREETING) - 1);
>>> }
>
>
>
>
>>> @@ -462,13 +476,22 @@ static int client_read(struct client *client)
>>> assert(client->bufferPos <= client->bufferLength);
>>> assert(client->bufferLength < sizeof(client->buffer));
>>>
>>> - bytesRead = read(client->fd,
>>> +#ifdef _WIN32
>>> + bytesRead = recv(client->fd,
>>> client->buffer + client->bufferLength,
>>> - sizeof(client->buffer) - client->bufferLength);
>>> -
>>> + sizeof(client->buffer) - client->bufferLength,0);
>>> +#else
>>> + bytesRead = read(client->fd,
>>> + client->buffer + client->bufferLength,
>>> + sizeof(client->buffer) - client->bufferLength);
>>> +#endif
>>
>> Why yet another #ifdef hack, when you could just replace read() with
>> recv()? On POSIX compliant systems, both is allowed, while Windows
>> only allows recv().
>
> recv() has one more parameter than read(). (note the ,0)
I know that. So?
>>> if (bytesRead > 0)
>>> return client_input_received(client, bytesRead);
>>> +#ifdef _WIN32
>>> + else if (bytesRead < 0 && WSAGetLastError() == WSAEINTR)
>>> +#else
>>> else if (bytesRead < 0 && errno == EINTR)
>>> +#endif
>>> /* try again later, after select() */
>>> return 0;
>>> else
>>> @@ -483,13 +506,18 @@ client_out_event(G_GNUC_UNUSED GIOChannel *source,
>>>
>>> static gboolean
>>> client_in_event(G_GNUC_UNUSED GIOChannel *source,
>>> - G_GNUC_UNUSED GIOCondition condition,
>>> + GIOCondition condition,
>>> gpointer data)
>>> {
>>> struct client *client = data;
>>> int ret;
>>>
>>> assert(!client_is_expired(client));
>>> + if (condition != G_IO_IN) {
>>> + client_set_expired(client);
>>> + return false;
>>> + }
>>> +
>>>
>>> client->lastTime = time(NULL);
>>>
>>> @@ -542,7 +570,7 @@ client_out_event(G_GNUC_UNUSED GIOChannel *source,
>>> if (g_queue_is_empty(client->deferred_send)) {
>>> /* done sending deferred buffers exist: schedule
>>> read */
>>> - client->source_id = g_io_add_watch(client->channel, G_IO_IN,
>>> + client->source_id = g_io_add_watch(client->channel,
>>> G_IO_IN|G_IO_NVAL | G_IO_HUP | G_IO_ERR,
>>> client_in_event, client);
>>> return false;
>>> }
>>
>> Care to explain this one? On Unix, it's enough to listen for G_IO_IN,
>> since the socket becomes readable when the peer closes the connection.
>
> I dont know what happen on unix.
> on windows the connect list run full ,when i was in the debugger and
> stepped.
Trial'n'error is the wrong way to develop software. Do some research
instead of trying until it succeeds. This leads to fragile code.
> I cannot see code in client_in_event which handles connection close when
> no close command is issued before by the client.
See client.c:476:
else
/* peer disconnected or I/O error */
return COMMAND_RETURN_CLOSE;
This is executed when read() returns a non-positive value (except for
EINTR). This is known to work well on POSIX compliant systems.
>>> @@ -728,8 +764,13 @@ static void client_write_direct(struct client
>>> *client,
>>> assert(length > 0);
>>> assert(g_queue_is_empty(client->deferred_send));
>>>
>>> +#ifdef _WIN32
>>> + if ((ret = send(client->fd, data, length,0)) < 0) {
>>> + if (WSAGetLastError() == WSAEWOULDBLOCK || WSAGetLastError() ==
>>> WSAEINTR) {
>>> +#else
>>> if ((ret = write(client->fd, data, length)) < 0) {
>>> if (errno == EAGAIN || errno == EINTR) {
>>> +#endif
>>> client_defer_output(client, data, length);
>>> } else {
>>> g_debug("client %i: problems writing", client->num);
>>
>> See above.
>
>
> send also has one more parameter than write()
I know that, too. So?
>>> diff --git a/src/cmdline.c b/src/cmdline.c
>>> old mode 100644
>>> new mode 100755
>>> index 48c1e87..ef7dd2c
>>> --- a/src/cmdline.c
>>> +++ b/src/cmdline.c
>>> @@ -32,7 +32,11 @@
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>>
>>> +#ifdef _WIN32
>>> +#define SYSTEM_CONFIG_FILE_LOCATION "c:/.mpd/mpd.conf"
>>> +#else
>>> #define SYSTEM_CONFIG_FILE_LOCATION "/etc/mpd.conf"
>>> +#endif
>>> #define USER_CONFIG_FILE_LOCATION ".mpdconf"
>>
>> Is that actually a good default location for win32? This looks like
>> it violates Microsoft's policies.
>
> the patch uses HOMEDIR\.mpdconf as default.
> SYSTEM_CONFIG_FILE_LOCATION is just for testing.
> but better is to use ALLUSERSPROFILE env var
Don't submit "just for testing" code, it won't be merged.
Even better would be to follow the Microsoft policy. I'm not sure if
ALLUSERSPROFILE is compliant. Find out, and add references to the
documentation to the patch description and to source code comments.
>>> G_GNUC_NORETURN
>>> diff --git a/src/command.c b/src/command.c
>>> old mode 100644
>>> new mode 100755
>>> index 1fac4fd..341a1e8
>>> --- a/src/command.c
>>> +++ b/src/command.c
>>> @@ -525,12 +525,8 @@ handle_add(struct client *client, G_GNUC_UNUSED
>>> int argc, char *argv[])
>>> enum playlist_result result;
>>>
>>> if (strncmp(uri, "file:///", 8) == 0) {
>>> -#ifdef WIN32
>>> - result = PLAYLIST_RESULT_DENIED;
>>> -#else
>>> result = playlist_append_file(uri + 7, client_get_uid(client),
>>> NULL);
>>> -#endif
>>> return print_playlist_result(client, result);
>>> }
>>>
>>
>> I disabled this code on Win32 for a good reason, which can be read in
>> the according patch. Please explain this hunk.
>
> well ok, i thought this is disabled because no uid checks on windows,
> but i wanted to be able to test this feature.
Again, don't submit "just for testing" code.
> maybe it should be configurable, if the files must belong to the mpd user.
> as this is a rather hard restrinction, because no files of me would
> belong to this user.
Why not use the same semantics as MPD on Unix?
>>> --- a/src/event_pipe.c
>>> +++ b/src/event_pipe.c
>>> @@ -53,15 +53,16 @@ event_pipe_invoke(enum pipe_event event)
>>> }
>>>
>>> static gboolean
>>> -main_notify_event(G_GNUC_UNUSED GIOChannel *source,
>>> +main_notify_event(GIOChannel *source,
>>> G_GNUC_UNUSED GIOCondition condition,
>>> G_GNUC_UNUSED gpointer data)
>>> {
>>> char buffer[256];
>>> - ssize_t r = read(event_pipe[0], buffer, sizeof(buffer));
>>> + gsize bytes_read = 0;
>>> + GIOStatus status =
>>> g_io_channel_read_chars(source,buffer,1,&bytes_read,NULL);
>>> bool events[PIPE_EVENT_MAX];
>>>
>>> - if (r < 0 && errno != EAGAIN && errno != EINTR)
>>> + if (bytes_read < 0 && status != G_IO_STATUS_NORMAL && status !=
>>> G_IO_STATUS_AGAIN)
>>> g_error("error reading from pipe: %s", strerror(errno));
>>>
>>> g_mutex_lock(event_pipe_mutex);
>>
>> This hunk is wrong and breaks the Linux build, and should also break
>> the Windows build.
>
> you are right, but as mentioned did not break here.
> i replacethe line by
> if(status == G_IO_STATUS_AGAIN)
> return true;
> else if (status != G_IO_STATUS_NORMAL )
> g_error("error reading from pipe: %s", strerror(errno));
It looked like it didn't break, because you didn't trigger the error
condition which is checked here, did you?
>>> @@ -76,10 +77,9 @@ main_notify_event(G_GNUC_UNUSED GIOChannel *source,
>>>
>>> return true;
>>> }
>>> -
>>> +static GIOChannel *channel;
>>> void event_pipe_init(void)
>>> {
>>> - GIOChannel *channel;
>>> int ret;
>>>
>>> #ifdef WIN32
>>> @@ -89,14 +89,16 @@ void event_pipe_init(void)
>>> #endif
>>> if (ret < 0)
>>> g_error("Couldn't open pipe: %s", strerror(errno));
>>> +#ifdef _WIN32
>>> + channel = g_io_channel_win32_new_fd(event_pipe[0]);
>>> +#else
>>> if (set_nonblocking(event_pipe[1]) < 0)
>>> g_error("Couldn't set non-blocking I/O: %s", strerror(errno));
>>> -
>>> channel = g_io_channel_unix_new(event_pipe[0]);
>>> +#endif
>>> event_pipe_source_id = g_io_add_watch(channel, G_IO_IN,
>>> main_notify_event, NULL);
>>> - g_io_channel_unref(channel);
>>> -
>>> +
>>> event_pipe_mutex = g_mutex_new();
>>> }
>>>
>>> @@ -105,8 +107,11 @@ void event_pipe_deinit(void)
>>> g_mutex_free(event_pipe_mutex);
>>>
>>> g_source_remove(event_pipe_source_id);
>>> -
>>> - close(event_pipe[0]);
>>> +
>>> + g_io_channel_shutdown(channel,false,NULL);
>>> + g_io_channel_unref(channel);
>>> + channel = NULL;
>>> +
>>> close(event_pipe[1]);
>>> }
>>
>> The whole event_pipe.c patch requires explanation
>
> ok first this from g lib header
> /* Create an IO channel for C runtime (emulated Unix-like) file
> * descriptors. After calling g_io_add_watch() on a IO channel
> * returned by this function, you shouldn't call read() on the file
> * descriptor. This is because adding polling for a file descriptor is
> * implemented on Win32 by starting a thread that sits blocked in a
> * read() from the file descriptor most of the time. All reads from
> * the file descriptor should be done by this internal GLib
> * thread. Your code should call only g_io_channel_read_chars().
> */
> GIOChannel* g_io_channel_win32_new_fd (gint fd);
>
> so i have to use g_io_channel_read_chars()
> as i/o would block, i only read 1 byte on incoming event. that will not
> block,
> because on event at least one byte is there.
I believe you misunderstood non-blocking I/O here. A read does not
block if at least 1 byte is *available*, not if exactly one byte is
*requested*. You can request any number of bytes, but read() will
not block and return what's available.
> i also need to use g_io_channel_shutdown()
> because else the internal glib thread would not be stopped.
>
> i also think that imho if giochnnel is used for a fd ( socket)
> giochannel functions should be used for read/write/close.
> ( would also remove closescoket/close recv/read send/write problems) but
> i thought that could be done later.
Proper patch welcome. Don't wait until "later", don't write
half-finished code with improper solutions.
>>> @@ -228,23 +239,6 @@ parseListenConfigParam(G_GNUC_UNUSED unsigned
>>> int port, ConfigParam * param)
>>> BINDERROR();
>>>
>>> freeaddrinfo(ai);
>>> -#else /* WIN32 */
>>> - const struct hostent *he;
>>> -
>>> - g_debug("binding to address for %s", param->value);
>>> -
>>> - he = gethostbyname(param->value);
>>> - if (he == NULL)
>>> - g_error("can't lookup host \"%s\" at line %i",
>>> - param->value, param->line);
>>> -
>>> - if (he->h_addrtype != AF_INET)
>>> - g_error("IPv4 address expected for host \"%s\" at line %i",
>>> - param->value, param->line);
>>> -
>>> - if (establishListen(AF_INET, he->h_addr, he->h_length) < 0)
>>> - BINDERROR();
>>> -#endif /* !WIN32 */
>>
>> Please explain.
>>
>
> Well the i removed that because the other code does also compile on windows.
What other code?
You removed the whole host lookup code! That breaks the
bind_to_address setting.
>>> -#ifndef WIN32
>>> enum playlist_result
>>> playlist_append_file(const char *path, int uid, unsigned *added_id)
>>> {
>>> int ret;
>>> struct stat st;
>>> struct song *song;
>>> -
>>> +#ifndef _WIN32 /* TODO: do user checks for WIN32 */
>>> if (uid <= 0)
>>> /* unauthenticated client */
>>> return PLAYLIST_RESULT_DENIED;
>>> @@ -546,14 +545,13 @@ playlist_append_file(const char *path, int uid,
>>> unsigned *added_id)
>>> if (st.st_uid != (uid_t)uid && (st.st_mode & 0444) != 0444)
>>> /* client is not owner */
>>> return PLAYLIST_RESULT_DENIED;
>>> -
>>> +#endif
>>> song = song_file_load(path, NULL);
>>> if (song == NULL)
>>> return PLAYLIST_RESULT_NO_SUCH_SONG;
>>>
>>> return addSongToPlaylist(song, added_id);
>>> }
>>> -#endif
>>>
>>> static struct song *
>>> song_by_url(const char *url)
>>
>> See above - needs explanation why this fixes the win32 build.
>
>
> wel this just enables the feature. further more the prototype and the
> functions should b always there.
> if disabled it should just return false ( means less #ifdefs)
Yes, that's a good idea to optimize existing code. Send patch!
>>> diff --git a/src/song.h b/src/song.h
>>> old mode 100644
>>> new mode 100755
>>> index a4c72cf..e717c07
>>> --- a/src/song.h
>>> +++ b/src/song.h
>>> @@ -21,7 +21,7 @@
>>>
>>> #include <stddef.h>
>>> #include <stdbool.h>
>>> -#include <sys/time.h>
>>> +#include <time.h>
>>>
>>> #define SONG_BEGIN "songList begin"
>>> #define SONG_END "songList end"
>>> diff --git a/src/tag.c b/src/tag.c
>>> old mode 100644
>>> new mode 100755
>>> index e7ce51c..d2f02cb
>>> --- a/src/tag.c
>>> +++ b/src/tag.c
>>> @@ -152,8 +152,11 @@ struct tag *tag_ape_load(const char *file)
>>> TAG_ITEM_TRACK,
>>> TAG_ITEM_DATE,
>>> };
>>> -
>>> +#ifdef _Win32
>>> + fp = fopen(file, "rb");
>>> +#else
>>> fp = fopen(file, "r");
>>> +#endif
>>> if (!fp)
>>> return NULL;
>>>
>>
>> The "b" flag is portable, and the #ifdef can be removed
>
> ok, i just wasnt sure, because i thought O_BINARY were also portable,
> but was not. so i chaged al "rb" to be in #ifdef _WIN32
> but will change it back
Another incidence of trial'n'error engineering: instead of looking up
the fopen() documentation, you made wrong assumptions, and made the
code more a mess than it could be.
>>> #include <pwd.h>
>>> #endif
>>> -
>>> #ifdef HAVE_IPV6
>>> +#ifndef _WIN32
>>> #include <sys/socket.h>
>>> #endif
>>> +#endif
>>>
>>> #ifdef WIN32
>>> #include <windows.h>
>>> @@ -56,7 +59,14 @@ void my_usleep(long usec)
>>>
>>> char *parsePath(char *path)
>>> {
>>> -#ifndef WIN32
>>> +
>>> +#ifdef _WIN32
>>> + if (path[1] != ':' && path[0] != '/') {
>>> + g_warning("\"%s\" is not an absolute path", path);
>>> + return NULL;
>>> + }
>>> +
>>> +#else
>>
>> This causes a crash if path is an empty string.
>
> replaced it by if (path && strlen(path) > 0 && path[1] != ':' && path[0]
> != '/') {
Why the "path != NULL" check? It's illegal to pass NULL here, and it
should crash then. Maybe add an assertion.
strlen() can be a huge overhead, because strlen() reads the whole
string. Why not just check "*path != 0", which is essentially the
same as "strlen(path) > 0"?
>>> int set_nonblocking(int fd)
>>> {
>>> -#ifdef WIN32
>>> - u_long val = 0;
>>> -
>>> +#ifdef _WIN32
>>> + u_long val = 1;
>>> +
>>> return ioctlsocket(fd, FIONBIO, &val) == 0 ? 0 : -1;
>>> #else
>>> int ret, flags;
>>
>> Please explain.
>>
>
> from msdn example code
> / Set the socket I/O mode: In this case FIONBIO
> // enables or disables the blocking mode for the
> // socket based on the numerical value of iMode.
> // If iMode = 0, blocking is enabled;
> // If iMode != 0, non-blocking mode is enabled.
> u_long iMode = 0;
> ioctlsocket(m_socket, FIONBIO, &iMode);
>
> so to enable non-blocking you set param to be NOT 0
That description belongs to the patch documentation (commit message).
It's not obvious what you were trying to achieve, and it's not obvious
why the old code was wrong.
>>> diff --git a/src/volume.c b/src/volume.c
>>> old mode 100644
>>> new mode 100755
>>> index 5eb0bd6..ac2add0
>>> --- a/src/volume.c
>>> +++ b/src/volume.c
>>> @@ -40,7 +40,11 @@
>>> #define SW_VOLUME_STATE "sw_volume: "
>>>
>>> const struct audio_output_plugin *default_mixer;
>>> +#ifdef _WIN32
>>> +static int volume_mixer_type = VOLUME_MIXER_TYPE_SOFTWARE; /*
>>> hardware mixer on windows not implemented yet!*/
>>> +#else
>>> static int volume_mixer_type = VOLUME_MIXER_TYPE_HARDWARE;
>>> +#endif
>>> static int volume_software_set = 100;
>>>
>>> void volume_finish(void)
>>
>> This is wrong. MPD should always attempt to find a hardware mixer.
>> If none is implemented, MPD should automatically fall back to
>> software. This is a non-solution.
> Ok i didnt know, but sounds right. fallback just does not seem work.
> so to make it work for windows it did that can remove it of course.
Another "trial'n'error" fix: instead of fixing the root cause of the
problem (fallback not working), you changed a global constant.
>>> diff --git a/src/zeroconf.c b/src/zeroconf.c
>>> old mode 100644
>>> new mode 100755
>>> index dd9790a..4f40419
>>> --- a/src/zeroconf.c
>>> +++ b/src/zeroconf.c
>>> @@ -22,7 +22,7 @@
>>> #include "config.h"
>>>
>>> #include <glib.h>
>>> -
>>> +#ifdef HAVE_ZEROCONF
>>> /* The default service name to publish
>>> * (overridden by 'zeroconf_name' config parameter)
>>> */
>>> @@ -71,3 +71,4 @@ void finishZeroconf(void)
>>> bonjour_finish();
>>> #endif
>>> }
>>> +#endif /*HAVE_ZEROCONF*/
>>> \ No newline at end of file
>>
>> This hunk is wrong, since zeroconf.c shouldn't be compiled at all if
>> HAVE_ZEROCONF is not defined.
>
> ok didnt know that it is not compiled at all on unix.
> as i use vcproj it was easier for me this way,
> but i remove this change, no problem.
You didn't know, maybe because you didn't look at Makefile.am:
if HAVE_ZEROCONF
mpd_SOURCES += zeroconf.c
[...]
endif
I suggest you just use autoconf/automake for building, instead of
proprietary Visual Studio files. This way, you don't have to
duplicate all the code in Makefile.am.
>> Sorry Gunnary, your patch is completely unacceptable. If there's one
>> thing I begged you then it's to *please* create single tiny patches
>> for separate changes, but it looks like you refused to obey that.
> Because it was a hard task for me to create one patch, i made so much
> mistakes concerning white spaces.
> I am used not used yet to this kind of working. i dont know how to have
> all changes somewhere an dthen create patches with only some of the
> changes.
That's difficult. You should have created separate patches while you
were working on the code. Now it's already a big patch, and untying
this big knot will take a lot of time.
>> Most hunks have no effect except breaking MPD's code style by
>> inserting or deleting whitespaces. There may be a few useful hunks
>> within that patch jungle, but it'll cost me less time to rewrite that
>> instead of trying to pick them from your patch.
>
> i have fixed all this whitepsace noise,
> header uppercase and some other errors.
> i can make the wrapper function you want, if you tell me where and how
> they should be named.
> i also can remove the append_playlist stuff inclusion.
> then i need to know how to do another patch based on master. or can the
> patch based on the submitted patch and a correct patch is created as
> result?
No, that would mean that I have to merge your first huge patch, and
then more patches which undo lots of its changes. Won't happen, I
will only merge individual clean patches.
I recommend that you use "stgit" for creating smaller patches. The
MPD wiki has some information on it:
http://mpd.wikia.com/wiki/git
In your case, I'd "uncommit" (stg uncommit) the huge patch, and pop it
off the stack (stg pop), and re-create lots of smaller patches from it
("stg-fold-files-from" might be helpful). Either throw away the big
patch at the end, or "push" it on top after each new patch, and fix
all the merge conflicts manually.
Of course, there are more ways to do it.
Max
------------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It is the best place to buy or sell services for
just about anything Open Source.
http://p.sf.net/sfu/Xq1LFB
_______________________________________________
Musicpd-dev-team mailing list
Musicpd-dev-team@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
