After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 792733 - Diff's hunk callback always report first hunk in diff
Diff's hunk callback always report first hunk in diff
Status: RESOLVED OBSOLETE
Product: libgit2-glib
Classification: Core
Component: General
git master
Other Linux
: Normal major
: ---
Assigned To: gitg-maint
gitg-maint
Depends on:
Blocks:
 
 
Reported: 2018-01-20 17:45 UTC by Martin Blanchard
Modified: 2019-02-22 03:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Do not rely on memory locations for diff objects caching (6.83 KB, patch)
2018-01-22 16:12 UTC, Martin Blanchard
none Details | Review
Do not rely on memory locations for diff objects caching (4.01 KB, patch)
2018-02-20 21:49 UTC, Martin Blanchard
none Details | Review
Refactor wrapper data initialisation (4.66 KB, patch)
2018-02-20 21:50 UTC, Martin Blanchard
none Details | Review
Do not rely on memory locations for diff objects caching (4.00 KB, patch)
2018-03-03 10:07 UTC, Martin Blanchard
none Details | Review
Refactor wrapper data initialisation (4.10 KB, patch)
2018-03-03 10:08 UTC, Martin Blanchard
none Details | Review

Description Martin Blanchard 2018-01-20 17:45:33 UTC
Using ggit_diff_blob_to_buffer(), the hunk callback always gets passed a pointer to the very first hunk in the diff, whatever the number of hunk may be for that diff.

This seems to be because libgit2-glib uses libgit2's hunk object pointer address as a key for its hunks cache hash table while libgit2 always passes the same pointer and simply memcopy() new hunk data in place at each run...

Most probably, libgit's hunk object pointer address should not be use as a key in that hash table. I can work on a fix, but I'd like to discuss what's best to use here as a unique key first.

The key should identify a particular hunk for a particular delta (ie. file). Thus it should be build with something unique from the delta and something unique from the hunk. For the hunk, the new_start (or old_start) integer should be unique within a given delta. The content string should also be. For the delta, the only thing that I can think of right now is the new_file (or old_file) path. Would using a combination of both of these be acceptable? Any better option?
Comment 1 Martin Blanchard 2018-01-22 16:12:38 UTC
Created attachment 367228 [details] [review]
Do not rely on memory locations for diff objects caching

First attempt, using delta's old_file oid and hunk's header content.

Comments and feedbacks welcome.
Comment 2 Ignacio Casal Quinteiro (nacho) 2018-02-19 22:32:22 UTC
Review of attachment 367228 [details] [review]:

Sorry for the late response. Could you please split the patch making first the refactoring of the init method and then the real patch?
Comment 3 Martin Blanchard 2018-02-20 21:49:12 UTC
Created attachment 368680 [details] [review]
Do not rely on memory locations for diff objects caching
Comment 4 Martin Blanchard 2018-02-20 21:50:47 UTC
Created attachment 368681 [details] [review]
Refactor wrapper data initialisation
Comment 5 Martin Blanchard 2018-03-03 10:07:40 UTC
Created attachment 369215 [details] [review]
Do not rely on memory locations for diff objects caching
Comment 6 Martin Blanchard 2018-03-03 10:08:06 UTC
Created attachment 369216 [details] [review]
Refactor wrapper data initialisation
Comment 7 GNOME Infrastructure Team 2018-05-22 13:28:00 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gitg/issues/109.