Hi again,

Here's a linkto
a commit containing a refactor of the palette loading code. I have moved
the file open/close logic to common code. This change actually removes more code than it adds. Here's a interesting diffstat (without whitespace only changes):

$ git diff --stat --color -w origin/master app/core/gimp.c | 4 +- app/core/gimppalette-import.c | 37 +++++++++-- app/core/gimppalette-load.c | 148 ++++++++++++----------------------------- app/core/gimppalette-load.h | 12 +++- 4 files changed, 85 insertions(+), 116 deletions(-)

Any chance this could be pulled into the master? Do y'all have any other thoughts on this?

Thanks,
wt

BTW, I tested this with gpl, act, aco, pal, and css palette files. I couldn't find RIFF file. If anyone has one, would you mind emailing it to me so that I can try loading it?

Thanks, wt

On Sun, Sep 8, 2013 at 5:06 AM, Warren Turkal wrote:

Hi again,

Here's a linkto a commit containing a refactor of the palette loading code. I have moved the file open/close logic to common code. This change actually removes more code than it adds. Here's a interesting diffstat (without whitespace only changes):

$ git diff --stat --color -w origin/master app/core/gimp.c | 4 +- app/core/gimppalette-import.c | 37 +++++++++-- app/core/gimppalette-load.c | 148 ++++++++++++----------------------------- app/core/gimppalette-load.h | 12 +++- 4 files changed, 85 insertions(+), 116 deletions(-)

Any chance this could be pulled into the master? Do y'all have any other thoughts on this?

Thanks,
wt

Hey,

I will get the format-patch as soon as I can. However, if you want to do it the git way and get it sooner, you can always add my remote to your git with the following command:
git remote add wt git@github.com:wt/gimp.git

Note that "wt" can be whatever alias you want, but my commands below assume you use "wt" as the remote alias.

That url for the repo is available on the main page for the repo on that website after a single click. To find it from the commit page that I sent earlier (this link
here),
look near the top left. You can see it if you look toward the top left. You'll see something like "PUBLIC wt/gimp". If you click on the "gimp" part, it will take you the repo main page (here ). Look on the right side of the page. You can get https, ssh, and a few other types of urls for the repo.

Once you have my remote, you can fetch my repo objects with this commands: git fetch wt

Then, you can do all the git operations you want. For example, here's how to get a log:
git log wt/refactor_palette_loader

If you want to merge in my commit, do the following while on your local working (maybe "master") branch:
git merge wt/refactor_palette_loader

And if you find that you don't want my remote in your repo anymore, you can remove it. Make sure you don't have any branches tracking mine. One way deal with branches tracking mine is to just delete them. Then execute this command:
git remote remove wt

After doing that, you can also do the following if you to get rid of commits that were only in my repo:
git prune

wt

On Sun, Sep 8, 2013 at 5:33 AM, Jehan Pags wrote:

Hi,

I searched a little in this web UI, but couldn't find: is there a way to generate a proper patch from this? Otherwise, could you generate one with "git format-patch origin/master", run on your locale branch? Thanks.

Jehan

On Mon, Sep 9, 2013 at 12:20 AM, Warren Turkal wrote:

BTW, I tested this with gpl, act, aco, pal, and css palette files. I couldn't find RIFF file. If anyone has one, would you mind emailing it to me so that I can try loading it?

Thanks, wt

On Sun, Sep 8, 2013 at 5:06 AM, Warren Turkal

wrote:

Hi again,

Here's a link<

https://github.com/wt/gimp/commit/d1e8c4c8543b18c6f5d95f6ab6b3bbbf8f80778b>to a commit containing a refactor of the palette loading code. I have moved

the file open/close logic to common code. This change actually removes

more

code than it adds. Here's a interesting diffstat (without whitespace

only

changes):

$ git diff --stat --color -w origin/master app/core/gimp.c | 4 +- app/core/gimppalette-import.c | 37 +++++++++-- app/core/gimppalette-load.c | 148 ++++++++++++----------------------------- app/core/gimppalette-load.h | 12 +++- 4 files changed, 85 insertions(+), 116 deletions(-)

Any chance this could be pulled into the master? Do y'all have any other thoughts on this?

Thanks,
wt

_______________________________________________ gimp-developer-list mailing list
List address: gimp-developer-list@gnome.org List membership:

https://mail.gnome.org/mailman/listinfo/gimp-developer-list

Attached is the patch.

wt

On Sun, Sep 8, 2013 at 12:13 PM, Warren Turkal wrote:

Hey,

I will get the format-patch as soon as I can. However, if you want to do it the git way and get it sooner, you can always add my remote to your git with the following command:
git remote add wt git@github.com:wt/gimp.git

Note that "wt" can be whatever alias you want, but my commands below assume you use "wt" as the remote alias.

That url for the repo is available on the main page for the repo on that website after a single click. To find it from the commit page that I sent earlier (this link here),
look near the top left. You can see it if you look toward the top left. You'll see something like "PUBLIC wt/gimp". If you click on the "gimp" part, it will take you the repo main page (here). Look on the right side of the page. You can get https, ssh, and a few other types of urls for the repo.

Once you have my remote, you can fetch my repo objects with this commands: git fetch wt

Then, you can do all the git operations you want. For example, here's how to get a log:
git log wt/refactor_palette_loader

If you want to merge in my commit, do the following while on your local working (maybe "master") branch:
git merge wt/refactor_palette_loader

And if you find that you don't want my remote in your repo anymore, you can remove it. Make sure you don't have any branches tracking mine. One way deal with branches tracking mine is to just delete them. Then execute this command:
git remote remove wt

After doing that, you can also do the following if you to get rid of commits that were only in my repo:
git prune

wt

On Sun, Sep 8, 2013 at 5:33 AM, Jehan Pags wrote:

Hi,

I searched a little in this web UI, but couldn't find: is there a way to generate a proper patch from this? Otherwise, could you generate one with "git format-patch origin/master", run on your locale branch? Thanks.

Jehan

On Mon, Sep 9, 2013 at 12:20 AM, Warren Turkal wrote:

BTW, I tested this with gpl, act, aco, pal, and css palette files. I couldn't find RIFF file. If anyone has one, would you mind emailing it

to

me so that I can try loading it?

Thanks, wt

On Sun, Sep 8, 2013 at 5:06 AM, Warren Turkal

wrote:

Hi again,

Here's a link<

https://github.com/wt/gimp/commit/d1e8c4c8543b18c6f5d95f6ab6b3bbbf8f80778b>to a commit containing a refactor of the palette loading code. I have moved

the file open/close logic to common code. This change actually removes

more

code than it adds. Here's a interesting diffstat (without whitespace

only

changes):

$ git diff --stat --color -w origin/master app/core/gimp.c | 4 +- app/core/gimppalette-import.c | 37 +++++++++-- app/core/gimppalette-load.c | 148 ++++++++++++----------------------------- app/core/gimppalette-load.h | 12 +++- 4 files changed, 85 insertions(+), 116 deletions(-)

Any chance this could be pulled into the master? Do y'all have any

other

thoughts on this?

Thanks,
wt

_______________________________________________ gimp-developer-list mailing list
List address: gimp-developer-list@gnome.org List membership:

https://mail.gnome.org/mailman/listinfo/gimp-developer-list

Sorry for taking so long to respond. I don't have a lot of time during normal work days to work on this. :)

On Tue, Sep 10, 2013 at 6:10 PM, Michael Henning wrote:

I made a few minor nitpicks on your commit on github. If you would fix those points, I'll commit your code to master.

Thanks. I think I addressed all your comments. However, I just added a rewind at the end of gimp_palette_load_detect format instead of doing what your comment suggested. PTAL and see if you approve. Here's the link.
I have also attached a patch to this mail.

As a side note for the future, the fastest way to get a patch reviewed

is normally if you post it to a pastebin and bother people on irc.

I am not in a huge hurry, and I haven't had much luck catching folks on IRC when I am on. Is there really any benefit of using pastebin over pushing my branch out so that it can be looked at and fetched? The pastebin method seems like it'd be pretty inconvenient for bigger patches, and it doesn't have any kind of commenting on the patch. Also, which pastebin do you prefer?

I will say that I was surprised there wasn't more interest in using git to pass around these patches. I would have expected you folks to acquire the patch from my repository (e.g. fetch my branch from my repo and look at the branch directly). I was somewhat surprised by the request for a patch file.

With regard to git, I don't see any merges in the history for the project. Does this project not do git merges?

-- drawoc

P.S. I don't see the patch on that last email. Are you sure you attached it?

It appears to be attached on my end. Does the ML block attachments?

FWIW, I also attached the new diff to this email. It indeed does appear to be attached at this point.

wt

On Tue, Sep 10, 2013 at 6:54 PM, Jehan Pags wrote:

On Wed, Sep 11, 2013 at 1:10 PM, Michael Henning wrote:
> As a side note for the future, the fastest way to get a patch reviewed

is normally if you post it to a pastebin and bother people on irc.

For my own, I would prefer a git format-patch like this, but on a feature request/bug report on bugzilla. That's easy to patch a branch and to remove after. And also we keep track of any discussion or updated patch about a feature/fix. For instance go find this email thread in one year in the mailing history.

Even for small refactorings like this one? I would certainly understand that for a feature add or a major refactor, but it seems like a lot of overhead for a pretty small refactor like this one. However, I am willing to do whatever you folks want since I just wanna help the project. However, please keep in mind that I have very little time to commit to this kind of work.

P.S. I don't see the patch on that last email. Are you sure you attached it?

I see it but I was also a direct recipient. I guess that the list cleans emails out from any attached file. Could we have conditional filters? Like any text file with a ".patch" or ".diff" extension should not be filtered out.

You should also allow git bundle files, which are a way to pass around git commits. I have attached one to this mail that includes the second iteration of my change. I guess only direct receivers of the email will receive it.

wt

I am willing to do whatever is needed to contribute. However, it would be nice if the mailing list wouldn't block patches.

Has anyone taken a look at maybe using gerrit? It's actually a pretty reasonable way to handle code changes when using git. It has a pretty nice code review workflow. Projects like Android and Libreoffice use it. As an example, here's a link
to the Libreoffice
instance.

wt

On Sun, Sep 15, 2013 at 2:22 AM, Jehan Pags wrote:

Hi,

On Sun, Sep 15, 2013 at 8:32 PM, Warren Turkal wrote:

On Tue, Sep 10, 2013 at 6:54 PM, Jehan Pags

wrote:

On Wed, Sep 11, 2013 at 1:10 PM, Michael Henning wrote:

As a side note for the future, the fastest way to get a patch reviewed is normally if you post it to a pastebin and bother people on irc.

For my own, I would prefer a git format-patch like this, but on a feature request/bug report on bugzilla. That's easy to patch a branch and to remove after. And also we keep track of any discussion or updated patch about a feature/fix. For instance go find this email thread in one year in the mailing history.

Even for small refactorings like this one? I would certainly understand

that

for a feature add or a major refactor, but it seems like a lot of

overhead

for a pretty small refactor like this one. However, I am willing to do whatever you folks want since I just wanna help the project. However,

please

keep in mind that I have very little time to commit to this kind of work.

Well there are no strict rules, I guess, likely because the team is small. I guess the real "rule" is to do so that others are not annoyed by the form your patch (so that they will actually check it, and not delay it to forever). Which means that if the other developers are ok with a git bundle for instance (I did not even know what it is, I had to look it up), or by adding your repo as a remote, well that's all good. :-)

I am myself flexible and adapt to various team logics. But by experience, I know some others of the core GIMP team want git format-patch. When I made my first patches (I am myself likely the most recent of the core developers), I also set up a public git repo for others to fetch. Well I was told my patches would more likely be reviewed if I provided patch files instead. And now I got used to it, so I work also easily with these. That's not more time-consuming. With a patch formatted with `git format-patch`, you can just "git apply" it, and done! So easy to review (and also to commit if the patch looks good with with git am, nothing to do).

I believe that at the opposite, for small patches, it is much easier to provide patch files than maintain a public repo. For huge features which require many commits over weeks, yeah there probably a public branch is worth it. :-)

Jehan

P.S. I don't see the patch on that last email. Are you sure you

attached

it?

I see it but I was also a direct recipient. I guess that the list cleans emails out from any attached file. Could we have conditional filters? Like any text file with a ".patch" or ".diff" extension should not be filtered out.

You should also allow git bundle files, which are a way to pass around

git

commits. I have attached one to this mail that includes the second

iteration

of my change. I guess only direct receivers of the email will receive it.

wt

Thanks for letting me know.

wt On Sep 17, 2013 4:44 PM, "Michael Henning" wrote:

wt: Your new patch looks good, so I committed it. Thanks!

As a side note, I modified the message slightly. We normally include the general area of the commit (often the top level directory) as a prefix to the commit message. Also, I removed the "signed off by" line because we don't really use that.

The short paragraph probably wasn't necessary in this case (we know what refactoring means), but I left that in anyway. :)

commit 198f2514ab03cd77c769b0cea9678fa0deba4f6e Author: Warren Turkal
Date: Sat Sep 14 23:46:28 2013 -0700

app: Refactor palette loaders.

I specifically moved the file opening/closing logic to the common code. This makes the code easier to understand for me since there is less duplication. In fact, this commit removes more lines than it adds.

After I committed it, Mitch asked me to change the function names from your patch on irc. You can see that commit here:

https://git.gnome.org/browse/gimp/commit/?id=d02dd9f0da778640a0a8a82420ee22f9a6efc943

On Mon, Sep 16, 2013 at 4:56 AM, Michael Schumacher wrote:

Gesendet: Montag, 16. September 2013 um 09:34 Uhr Von: "Warren Turkal"

I am willing to do whatever is needed to contribute. However, it would

be

nice if the mailing list wouldn't block patches.

Please keep in mind that those file would be sent out to every

subscriber of this mailing list, even those who would not want to receive them. Now imagine people being subscribed to > 10 mailing lists. Would you want to receive all patches and bundles from all the projects you're subscribed to?

The preferred way right now is to open bug reports in Bugzilla and

attach your patches there.

Has anyone taken a look at maybe using gerrit? It's actually a pretty reasonable way to handle code changes when using git. It has a pretty

nice

code review workflow. Projects like Android and Libreoffice use it. As

an

example, here's a link
to the Libreoffice
instance.

Might be worthwhile to discuss that with GNOME; it's their repository

we're using after all.

They could be interesed in this especially for their GNOME Love bugs,

see https://wiki.gnome.org/GnomeLove

--
Regards,
Michael
_______________________________________________ gimp-developer-list mailing list
List address: gimp-developer-list@gnome.org List membership:

https://mail.gnome.org/mailman/listinfo/gimp-developer-list _______________________________________________ gimp-developer-list mailing list
List address: gimp-developer-list@gnome.org List membership:
https://mail.gnome.org/mailman/listinfo/gimp-developer-list