Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update SQLite support #344

Merged
merged 5 commits into from
Dec 8, 2023
Merged

Update SQLite support #344

merged 5 commits into from
Dec 8, 2023

Conversation

tuffnatty
Copy link
Member

@tuffnatty tuffnatty commented Nov 15, 2023

This is a series of patches that update SQLite support and make the strict CI builds green on most non-Windows platforms. It is meant to be applied on top of #343 .

It includes several patches from @vszakats 's fork. At the moment the original Viktor's ChangeLog entries are included at the top of Changelog.txt, with their original dates. I am not sure if it's good to leave it this way. Should I make my own entries dated as of today and just include the original entry headings as comments? @harbour/dev opinions?

@alcz
Copy link
Contributor

alcz commented Nov 15, 2023

It includes several patches from @vszakats 's fork. At the moment the original Viktor's ChangeLog entries are included at the top of Changelog.txt, with their original dates. I am not sure if it's good to leave it this way. Should I make my own entries dated as of today and just include the original entry headings as comments? @harbour/dev opinions?

IMHO include headings as comments, something like

* list of changes
; changes were borrowed/merged from these Harbour 3.4 commits
  2015-04-09 11:34 UTC+0200 Viktor Szakats
  2015-09-02 19:11 UTC+0200 Viktor Szakats
  2017-08-01 20:23 UTC Viktor Szakats

@vszakats
Copy link
Member

Good question. When I'm merging commits from here, I always keep the original ChangeLog entries (manually sorted by entry timestamp and merged back into my entries), sometimes with clearly labeled comments, e.g. in case it has already been implemented, or rejected with an explanation. This assumes that the patches are merged in their original form, with conflict resolutions only. I tend to apply post-merge fixes in separate commits.

When merging in-mass, this might not be practical. Either way a reference to the original ChangeLog entry headers or Git commit hashes (or both) are very useful to add to keep track of what's committed.

FWIW our custom of keeping a manual ChangeLog.txt is an unsustainable solution IMO. It causes permanent merge conflicts and inflates the Git database size (but that's probably the least of its problems). With Git, the effective ChangeLog is git log.

@tuffnatty
Copy link
Member Author

I have updated the ChangeLog entries and the commit messages.

@vszakats Yes, the ChangeLog can definitely freeze GitHub-based collaboration. Every rebase is a mess.

@alcz
Copy link
Contributor

alcz commented Nov 15, 2023

@vszakats Yes, the ChangeLog can definitely freeze GitHub-based collaboration. Every rebase is a mess.

I suppose redacted ChangeLog.txt could alternatively land in hbdoc repo or something like that. Would broke traditional/simple commits, so feels bit bad, but surely would relive working with pull requests, merging, etc.

@tuffnatty tuffnatty mentioned this pull request Nov 15, 2023
@vszakats
Copy link
Member

@vszakats Yes, the ChangeLog can definitely freeze GitHub-based collaboration. Every rebase is a mess.

I suppose redacted ChangeLog.txt could alternatively land in hbdoc repo or something like that. Would broke traditional/simple commits, so feels bit bad, but surely would relive working with pull requests, merging, etc.

I think it's enough to leave it in the repo (where it is now) and
stop updating it at one agreed-on point in time.

That said this also makes the current ChangeLog entry format
obsolete. Git already tracks the timestamp and committer, and
it makes little sense to repeat that manually in the commit message.
It actually makes the one-liner commit message unusable (because
it repeats the timestamp and committer name instead of saying what
the commit did.) The list of filenames is also redundant.

Also, referencing commits via their hashes would be more useful
than copying the current entry headers.

This command can generate a log from commit entries, with filenames
and descriptions, dynamically:

git log --pretty=medium --no-merges --date=iso --stat HEAD~50..HEAD

@snaiperis
Copy link
Member

Don't we need to release the new item in trace_handler case SQLITE_TRACE_CLOSE?

* contrib/hbsqlit3/hbsqlit3.ch
+ add SQLITE3_PREPARE_PERSISTENT constant
; commit is borrowed from vszakats/hb@b33de96c81ac790f390e506160a4d1d8c2b5f4fe
; 2017-08-01 20:23 UTC Viktor Szakats (vszakats users.noreply.github.com)
Copy link
Member

@vszakats vszakats Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tuffnatty:

Please keep the original header for merged commit entries
copied as-is. The commit entry itself is part of the work,
and the above reattributes that to someone else.

It's confusing and looks as if this entry was written by another
person. The "borrowed from" doesn't clarify this.

Also "origin", "merged from", "copied from" describes what is
going on. "borrowed from" doesn't., even though I know it's
customary wording in Harbour communities.

Furthermore I'd suggest using a link to the original commit. It's
more descripive and useful than "vszakas/hb@",
and also clickable.

This is what I have in mind:

2017-08-01 20:23 UTC Viktor Szakats (vszakats users.noreply.github.com)
   * contrib/hbsqlit3/core.c
   * contrib/sddsqlt3/core.c
     * use new sqlite3_prepare_v3() when built against
       sqlite 3.20.0 or upper.
   * contrib/hbsqlit3/core.c
     + SQLITE3_PREPARE() add 3rd parameter <nPrepFlags>
       Usable if build against sqlite 3.20.0 or upper, ignored otherwise
   * contrib/hbsqlit3/hbsqlit3.ch
     + add SQLITE3_PREPARE_PERSISTENT constant
   ; Origin: https://github.com/vszakats/hb/commit/b33de96c81ac790f390e506160a4d1d8c2b5f4fe

Your name will appear as the committer in the merged commit,
so that is recorded as well.

@tuffnatty
Copy link
Member Author

Don't we need to release the new item in trace_handler case SQLITE_TRACE_CLOSE?

Probably; I'll look at it but it needs to be discussed in #343, not here .

@tuffnatty
Copy link
Member Author

@vszakats As I understand it, you are objecting to the way more or less everyone (yourself as well, see, e.g. 2017-09-13 21:17 UTC Viktor Szakats (vszakats users.noreply.github.com)) did those merges before I raised up this question. I always perceived the top row as the commit date and committer in the repository being pushed to. If I change the top row to the original ChangeLog entry, should I also move them deep down to 2015 and 2017 or keep them strangely in the middle of 2023?
As for the wording of borrowed/origin, I'll change it as you wish.

@tuffnatty
Copy link
Member Author

@vszakats Of course, if you would like to merge the commits in question to harbour/core yourself, it could also solve the problem.

@alcz
Copy link
Contributor

alcz commented Nov 16, 2023

With original timestamp they have to be "buried". One of the Viktor's response, on merging the other way around into 3.4:

When I'm merging commits from here, I always keep the original ChangeLog entries (manually sorted by entry timestamp and merged back into my entries), sometimes with clearly labeled comments, e.g. in case it has already been implemented, or rejected with an explanation.

Not that i remember doing this myself for old entries ever, sorry. But sort of makes sense if the merge takes all, getting all changes available for code, so should ChangeLog entries be merged likewise.

@tuffnatty
Copy link
Member Author

@alcz @vszakats In my opinion, ChangeLog makes sense only when it's a chronological history of changes in the given repository. (well, after the switch to Git, one may argue that this is better served by git log). After I bury the changes, a person can't understand (without the help of git tools) the commit order. Makes no sense to me. In purely Git projects, when one needs to backport a commit to an older branch, one usually cherry-picks the commit but modifies the message to include the "backported from" info (as it's being lost on cherry-picked commit).

@tuffnatty
Copy link
Member Author

@vszakats I'd like to add that I am more troubled by the chronological ordering of the ChangeLog than by the issue of ChangeLog entry authorship.

@vszakats
Copy link
Member

vszakats commented Nov 24, 2023

I'm objecting to take a ChangeLog entry word-by-word and re-assign it
to the committer's name.

When copying entries around, I'm asking to copy around the whole entry,
including its header with the date and author.

In "2017-09-13 21:17 UTC" I was merging my own commits, so there was
no change in the author's name. I was cherry-picking partial commits
into a single commit. It musn't change authorship.

When merging locally, commit-by-commit, I always keep the whole entry
as-is. Either when manually or automatically merging patches from another
branch. This not only keeps the entry text (commit message) intact, but
also Git 'author' records. After recent changes I had to switch to manual
merge with Harbour (into my local repo), and in these case I'm also adding
a reference (link) to the original commit hash.

And yes, maintaining a copy of these entries in ChangeLog.txt makes the
above overly labourous and error-prone because there is almost always a
merge conflict. That's why I was suggesting to stop editing ChangeLog.txt
in future commits.

EDIT: here is an example:

commit 4ed2ac9b7f58bcac062edf8b3c58650695482951 (HEAD -> main, private/main)
Author:     Przemysław Czerpak <...>
AuthorDate: 2023-11-13 23:58:30 +0100
Commit:     Viktor Szakats <...>
CommitDate: 2023-11-16 02:11:34 +0000

    2023-11-13 23:58 UTC+0100 Przemyslaw Czerpak (druzus/at/poczta.onet.pl)
    
      * contrib/hbct/ctwin.c
        ! typo in comment
    
      * include/hbexprb.c
      * include/hbexprop.h
      * src/common/expropt2.c
      * src/harbour.def
        * moved code for reducing NOT expression to new separate function
          hb_compExprReduceNot()
        ; added comments to mark places for possible compiletime warnings when
          incompatible types are used inside in logical expressions
          (.AND. / .OR. / .NOT.)
    
    Origin: https://github.com/harbour/core/commit/6bbb31e5408929e04d54e391020dd374ec366457

@tuffnatty
Copy link
Member Author

@vszakats Please take a look if this approach is Ok. I've changed all headers with your code to your name. The header dates are new. Two of the commits are cherry-picked unchanged, the other two are the result of commit splitting and merging. Every commit is linked to all its origins in your repo.

@vszakats
Copy link
Member

@tuffnatty: Thanks, it looks much better! Though in case of 23-11-30 19:52 UTC+0100 it's your commit message text, so the header should be yours too.

For 023-11-30 19:50 UTC+0100, 2023-11-30 19:45 UTC+0100, 2023-11-30 19:40 UTC+0100, I'd prefer the original dates in the headers. These serve as a kind of 'identifier' when searching for commits. Changing the date also suggests this was committed by myself now, which isn't the case.

Perhaps a format like would fit all criteria:

2023-11-30 19:45 UTC+0100 Phil Krylov (phil a t krylov.eu)
  * merge commits:
    2015-09-02 19:11 UTC+0200 Viktor Szakats (vszakats users.noreply.github.com)
       * contrib/hbsqlit3/core.c
         % use hb_fileLoad()
       ; Commit split from:
       ; origin: https://github.com/vszakats/hb/commit/8f534aaa77ff07ad86db5ad079d376d515561c5a

    2015-09-02 19:11 UTC+0200 Viktor Szakats (vszakats users.noreply.github.com)
      * contrib/hbsqlit3/core.c
        % use hb_fileLoad()
      ; Commit split from:
      ; origin: https://github.com/vszakats/hb/commit/8f534aaa77ff07ad86db5ad079d376d515561c5a

    2015-04-09 11:34 UTC+0200 Viktor Szakats (vszakats users.noreply.github.com)
       * contrib/hbsqlit3/core.c
       * contrib/hbsqlit3/hbsqlit3.hbx
         + add sqlite3_status64()
         % adjust variable scopes
         ! fix to reset reference parameters in functions with
           functionality disabled
       ; origin: https://github.com/vszakats/hb/commit/5bcaa24c93bccdbdfe9ea52f3cbf4ebb8837dd5f

or this, with less indentation:

2023-11-30 19:45 UTC+0100 Phil Krylov (phil a t krylov.eu)
  ; merged:
  2015-09-02 19:11 UTC+0200 Viktor Szakats (vszakats users.noreply.github.com)
     * contrib/hbsqlit3/core.c
       % use hb_fileLoad()
     ; Commit split from:
     ; origin: https://github.com/vszakats/hb/commit/8f534aaa77ff07ad86db5ad079d376d515561c5a

  2015-09-02 19:11 UTC+0200 Viktor Szakats (vszakats users.noreply.github.com)
    * contrib/hbsqlit3/core.c
      % use hb_fileLoad()
    ; Commit split from:
    ; origin: https://github.com/vszakats/hb/commit/8f534aaa77ff07ad86db5ad079d376d515561c5a

  2015-04-09 11:34 UTC+0200 Viktor Szakats (vszakats users.noreply.github.com)
     * contrib/hbsqlit3/core.c
     * contrib/hbsqlit3/hbsqlit3.hbx
       + add sqlite3_status64()
       % adjust variable scopes
       ! fix to reset reference parameters in functions with
         functionality disabled
     ; origin: https://github.com/vszakats/hb/commit/5bcaa24c93bccdbdfe9ea52f3cbf4ebb8837dd5f

@tuffnatty tuffnatty force-pushed the sqlite branch 2 times, most recently from 3e1f161 to 8137d8b Compare December 1, 2023 11:42
@tuffnatty
Copy link
Member Author

Rebased against latest master.

@tuffnatty tuffnatty force-pushed the sqlite branch 2 times, most recently from 43ee189 to 8087b05 Compare December 7, 2023 20:36
@tuffnatty
Copy link
Member Author

Added patches for Borland C support, updated SQLite to 3.44.2, rebased against current master.
@vszakats Can I assume you don't have any more objections?

Viktor Szakats added 2 commits December 8, 2023 22:58
  ; merged:
  2015-04-09 11:34 UTC+0200 Viktor Szakats (vszakats users.noreply.github.com)
    * contrib/hbsqlit3/core.c
    * contrib/hbsqlit3/hbsqlit3.hbx
      + add sqlite3_status64()
      % adjust variable scopes
      ! fix to reset reference parameters in functions with
        functionality disabled
    ; origin: vszakats/hb@5bcaa24

  2015-09-02 19:11 UTC+0200 Viktor Szakats (vszakats users.noreply.github.com)
    * contrib/hbsqlit3/core.c
      % use hb_fileLoad()
    ; Commit split from:
    ; origin: vszakats/hb@8f534aa

  2017-08-01 20:23 UTC Viktor Szakats (vszakats users.noreply.github.com)
    * contrib/hbsqlit3/core.c
    * contrib/sddsqlt3/core.c
      * use new sqlite3_prepare_v3() when built against
        sqlite 3.20.0 or upper.
    * contrib/hbsqlit3/core.c
      + SQLITE3_PREPARE() add 3rd parameter <nPrepFlags>
        Usable if build against sqlite 3.20.0 or upper, ignored otherwise
    * contrib/hbsqlit3/hbsqlit3.ch
      + add SQLITE3_PREPARE_PERSISTENT constant
    ; origin: vszakats/hb@b33de96
  * contrib/hbsqlit3/core.c
  * contrib/hbsqlit3/hbsqlit3.ch
    % Minor updates to hbsqlit3, borrowed from @vszakats's fork.
  ; parts of original commits:
  ; * vszakats/hb@ca9f758
  ;   2015-05-13 21:07 UTC+0200 Viktor Szakats (vszakats users.noreply.github.com)
  ; * vszakats/hb@949e77a
  ;   2015-07-15 20:26 UTC+0200 Viktor Szakats (vszakats users.noreply.github.com)
  ; * vszakats/hb@dae1a86
  ;   2017-08-14 18:59 UTC Viktor Szakats (vszakats users.noreply.github.com)
  * contrib/hbsqlit3/hbsqlit3.ch
    * Updated SQLite constants to version 3.44.2.
    % Reordered hbsqlit3.ch to simplify diffing from sqlite.h.
  * contrib/hbsqlit3/core.c
    + SQLITE3_CREATE_FUNCTION(): Added support for function flags.
  * contrib/3rd/sqlite3/*
    * Updated local sqlite3 from 3.8.2 to 3.44.2.
  * contrib/sddsqlt3/core.c
    ! Fixed a different signedness compare warning.
@vszakats
Copy link
Member

vszakats commented Dec 8, 2023

@tuffnatty: It looks good to me, thank you!

@tuffnatty tuffnatty merged commit 8405632 into harbour:master Dec 8, 2023
19 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants