Patch Etiquette

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Patch Etiquette

Ziemowit Laski
Hello SQLite maintainers,

I'm glad that my patch below has been incorporated (with some changes) into future releases:

[50e60cb4]<http://www.sqlite.org/src/info/50e60cb44fd3687d> Modify the ICU extension to use a static initializer, as VC++ complains about a dynamic initialization. Maybe the dynamic structure initialization is a GCC extension. (user: drh<http://www.sqlite.org/src/timeline?u=drh&c=2017-01-26+00%3A58%3A27&nd&n=200>, tags: trunk<http://www.sqlite.org/src/timeline?r=trunk&nd&c=2017-01-26+00%3A58%3A27&n=200>)

I think that SQLite is a great product, and I'm happy to be able to contribute to it.

HOWEVER, one thing bothers me:  You did not acknowledge my authorship of it.  AFAICT, you NEVER seem to acknowledge third party contributions.  Clearly, I'm not user 'drh'.  Like with other open-source projects, I would expect my name and e-mail to appear in the commit message (and the changelog, if you had one), and definitely in the release notes.  This is the polite and professional thing to do, and may even encourage others to contribute to the project.

I have another patch in the works which will make SQLITE_ENABLE_UPDATE_DELETE_LIMIT directly accessible from the amalgamation, but am reluctant to submit it.

Thank you,

--Zem


From: Ziemowit Laski
Sent: Wednesday, 25 January 2017 12:36
To: [hidden email]
Subject: BUG: Illegal initialization in icu.c : sqlite3IcuInit

Visual C++ correctly catches this.  The fragment

  struct IcuScalar {
    const char *zName;                        /* Function name */
    int nArg;                                 /* Number of arguments */
    int enc;                                  /* Optimal text encoding */
    void *pContext;                           /* sqlite3_user_data() context */
    void (*xFunc)(sqlite3_context*,int,sqlite3_value**);
  } scalars[] = {
    {"regexp", 2, SQLITE_ANY|SQLITE_DETERMINISTIC,          0, icuRegexpFunc},

    {"lower",  1, SQLITE_UTF16|SQLITE_DETERMINISTIC,        0, icuCaseFunc16},
    {"lower",  2, SQLITE_UTF16|SQLITE_DETERMINISTIC,        0, icuCaseFunc16},
    {"upper",  1, SQLITE_UTF16|SQLITE_DETERMINISTIC, (void*)1, icuCaseFunc16},
    {"upper",  2, SQLITE_UTF16|SQLITE_DETERMINISTIC, (void*)1, icuCaseFunc16},

    {"lower",  1, SQLITE_UTF8|SQLITE_DETERMINISTIC,         0, icuCaseFunc16},
    {"lower",  2, SQLITE_UTF8|SQLITE_DETERMINISTIC,         0, icuCaseFunc16},
    {"upper",  1, SQLITE_UTF8|SQLITE_DETERMINISTIC,  (void*)1, icuCaseFunc16},
    {"upper",  2, SQLITE_UTF8|SQLITE_DETERMINISTIC,  (void*)1, icuCaseFunc16},

    {"like",   2, SQLITE_UTF8|SQLITE_DETERMINISTIC,         0, icuLikeFunc},
    {"like",   3, SQLITE_UTF8|SQLITE_DETERMINISTIC,         0, icuLikeFunc},

    {"icu_load_collation",  2, SQLITE_UTF8, (void*)db, icuLoadCollation},
  };

  int rc = SQLITE_OK;
  int i;

should read

       struct IcuScalar {
              const char *zName;                        /* Function name */
              int nArg;                                 /* Number of arguments */
              int enc;                                  /* Optimal text encoding */
              void *pContext;                           /* sqlite3_user_data() context */
              void(*xFunc)(sqlite3_context*, int, sqlite3_value**);
       } scalars[] = {
              { "regexp", 2, SQLITE_ANY | SQLITE_DETERMINISTIC,          0, icuRegexpFunc },

              { "lower",  1, SQLITE_UTF16 | SQLITE_DETERMINISTIC,        0, icuCaseFunc16 },
              { "lower",  2, SQLITE_UTF16 | SQLITE_DETERMINISTIC,        0, icuCaseFunc16 },
              { "upper",  1, SQLITE_UTF16 | SQLITE_DETERMINISTIC, (void*)1, icuCaseFunc16 },
              { "upper",  2, SQLITE_UTF16 | SQLITE_DETERMINISTIC, (void*)1, icuCaseFunc16 },

              { "lower",  1, SQLITE_UTF8 | SQLITE_DETERMINISTIC,         0, icuCaseFunc16 },
              { "lower",  2, SQLITE_UTF8 | SQLITE_DETERMINISTIC,         0, icuCaseFunc16 },
              { "upper",  1, SQLITE_UTF8 | SQLITE_DETERMINISTIC,  (void*)1, icuCaseFunc16 },
              { "upper",  2, SQLITE_UTF8 | SQLITE_DETERMINISTIC,  (void*)1, icuCaseFunc16 },

              { "like",   2, SQLITE_UTF8 | SQLITE_DETERMINISTIC,         0, icuLikeFunc },
              { "like",   3, SQLITE_UTF8 | SQLITE_DETERMINISTIC,         0, icuLikeFunc },

              { "icu_load_collation",  2, SQLITE_UTF8, 0, icuLoadCollation }
       };

       int rc = SQLITE_OK;
       int i;

       scalars[11].pContext = (void*)db;

Thank you,

--Zem
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Patch Etiquette

Jan Nijtmans
2017-02-05 21:14 GMT+01:00 Ziemowit Laski:
> I have another patch in the works which will make SQLITE_ENABLE_UPDATE_DELETE_LIMIT directly accessible from the amalgamation, but am reluctant to submit it.

Well, I submitted such a patch already, some half year ago. You can
find the full thread here:
    <http://sqlite.1065341.n5.nabble.com/ENABLE-UPDATE-DELETE-LIMIT-td90381.html>

Feel free to try this patch (see my message Jul 12, 2016; 11:14am in
this thread)

Regards,
        Jan Nijtmans
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Patch Etiquette

Jens Alfke-2
In reply to this post by Ziemowit Laski

> On Feb 5, 2017, at 12:14 PM, Ziemowit Laski <[hidden email]> wrote:
>
> HOWEVER, one thing bothers me:  You did not acknowledge my authorship of it.  AFAICT, you NEVER seem to acknowledge third party contributions.  Clearly, I'm not user 'drh’. Like with other open-source projects, I would expect my name and e-mail to appear in the commit message (and the changelog, if you had one), and definitely in the release notes.

The commit description does say "This fixes a problem identified on the SQLite mailing list by Ziemowit Laski.”

Are you complaining that your email address was left out [some people strongly dislike having their email address posted online]; or that you weren’t identified as the author of the patch; or that you don’t have an account on the version-control system?

(FYI I am not associated with SQLite in any way, just curious.)

—Jens
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Patch Etiquette

Ziemowit Laski
In reply to this post by Ziemowit Laski
Hi Jens,

Yes, I did notice the commit that mentions my name, and am glad for the attribution.  What I was getting at, though, is that such attributions should be part of SQLite policy and should happen automatically and every time.

To your question, I don't particularly care if I have write privileges to the repo.  In fact, I don't think it would be a good idea because (1) I'm a relative newbie to SQLite and could easily break things and (2) I'm not and will not be a regular contributor.  I DO care about being acknowledged as the author of the patch, however.

On the e-mail address question, I tend to think that they should be required as well.  Any patch that is committed may cause integration problems down the line.  SQLite has numerous extensions which can be baked into it, and it is impossible for any single contributor to perform regression testing on all these permutations.  Ergo, it may be necessary to contact the author of the patch for them to try to rework it and/or explain how the patch's functionality is supposed to interoperate with the new feature/change which caused the breakage.

SQLite is a relatively small project, so we can probably get away with not publishing e-mail addresses, since the 3 maintainers probably know who submitted what and can contact them if needed.  For larger projects (e.g., GCC), having e-mail addresses of contributors known to everyone is absolutely essential.

Thanks,

--Zem


_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Patch Etiquette

Clemens Ladisch
Ziemowit Laski wrote:
> [...]
> I DO care about being acknowledged as the author of the patch, however.

But you are not the author.  You reported the problem, but the actual
patches that got applied (http://www.sqlite.org/cgi/src/info/50e60cb44fd3687d,
http://www.sqlite.org/cgi/src/info/d9752c8f7c55426f) were not derived
from the patch you proposed.


Regards,
Clemens
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users