Move `git_off_t` to `git_object_size_t` by ethomson · Pull Request #5123 · libgit2/libgit2

ethomson · 2019-06-16T18:46:02Z

git_off_t is problematic: first, it's used to represent the size of
a blob, but that's represented in git as an unsigned long, meaning
that we've lost a bit (on 64-bit machines). Second, git_off_t is a
poor name, suggesting that it's an offset, not a size (which makes
sense, given that it's a signed type).

Migrate git_off_t to git_object_size_t, an unsigned 64-bit type.

ethomson · 2019-06-17T08:53:32Z

Aha, we do actually treat git_off_t as an offset in the packfile handling (and as a length, of course). I'll have to step through here and 👀 how we're dealing with this and tighten it up.

ethomson · 2019-07-20T21:18:04Z

Rebased to fix conflicts.

ethomson · 2019-08-11T20:57:29Z

Rebased.

ethomson · 2019-08-16T09:41:44Z

@pks-t any thoughts here? I'd love to think about a release 🔜 and include this.

pks-t · 2019-08-21T05:35:14Z

On Fri, Aug 16, 2019 at 02:41:53AM -0700, Edward Thomson wrote: @pks-t any thoughts here? I'd love to think about a release 🔜 and include this.

I hope to be able to find enought time to pick up some loose ends in libgit2 this Friday. And I fully agree that we should start thinking about a release soon, it's been quite some time.

pks-t

This looks mostly good to me. There's two APIs where I'm sceptical about using git_object_size_t with the diff API and the futils interfaces, though

pks-t · 2019-08-23T11:11:01Z

 	int    num_lines;    /**< Number of newline characters in content */
 	size_t content_len;  /**< Number of bytes of data */
-	git_off_t content_offset; /**< Offset in the original file to the content */
+	git_object_size_t content_offset; /**< Offset in the original file to the content */


Does it really make sense to use git_object_size_t here? Diffs aren't necessary referring to any objects at all but, so it seems a bit dirty to me to mix these up.

Hmm, perhaps not. The rare good use of git_off_t?

pks-t · 2019-08-23T11:14:41Z

 	fc->opts_flags = opts ? opts->flags : GIT_DIFF_NORMAL;

-	if (opts && opts->max_size >= 0)
+	if (opts && (int64_t)opts->max_size >= 0)


Mh. This comparison doesn't make sense anymore, as max_size is now unsigned. If it becomes negative due to casting then I'd consider this a bug.

Yeah, ignore my last comment. You're right.

pks-t · 2019-08-23T11:15:14Z

 	 * Defaults to 512MB.
 	 */
-	git_off_t   max_size;
+	git_object_size_t max_size;


Hm, this changes our API, doesn't it?

In practice, if callers ever provided a negative value, it was caught in diff_file_content_init_common, and ignored.
#5123 (diff) was doing the same. So as-is we are API compatible, but if we remove that odd-looking check, then we are in fact changing the API.

Moved this back to a signed int for sanity.

pks-t · 2019-08-23T11:20:10Z

 			line.content_offset = bufs[1].ptr - info->xd_old_data.ptr;
 		else
-			line.content_offset = -1;
+			line.content_offset = GIT_OBJECT_SIZE_MAX;


This changes semantics too, as we pass these to the callers

Sorry, can you give me more information about what you mean?

We're populating the git_diff_line struct here, which we directly pass to the user via the data_cb callback. So previously callers would've seen content_offset = -1, now he sees content_offset = UINT64_MAX if a line was unchanged. I could imagine people having checks for that

Moved this back to a signed int to support this.

pks-t · 2019-08-23T11:22:10Z

 }

-git_off_t git_futils_filesize(git_file fd)
+int git_futils_filesize(git_object_size_t *out, git_file fd)


Not sure how I feel about these. off_t directly maps to the mmap functions of the OS, using git_object_size_t feels weird to me

Well, mmap does take an off_t but only for the offset into the file. That doesn't correspond to the length of the file or memory-mapped segment.

I definitely don't think that we should use a signed type for file length, it simply can't be negative. We wouldn't want to use size_t here, since on a 32-bit system, we want to be able to get the size of a file larger than 4 GB.

We could call this the more direct uint64_t here.

Hum. Some platforms handle negative offsets in mmap just fine and treat it as valid. It's questionable though whether we're going to ever need that, especially considering that we'd have to fight with cross-platform behaviour here. So 👍 for uint64_t.

Updated for uint64_t

tiennou

This LGTM. I think I'd like it better as a single global: deprecate git_off_t commit, since it's mostly about typechanging things around, but that's minor. Thanks for fixing this @ethomson !

pks-t · 2019-11-05T12:51:42Z

 typedef struct {
 	struct timespec mtime;
-	git_off_t  size;
+	git_object_size_t size;


Some file APIs now use uint64_t, some use git_object_size_t. Shouldn't this one here use uint64_t, as well?

Yes, this probably makes sense.

pks-t · 2019-11-05T12:55:03Z



-int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offset)
+int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, off64_t offset)


Quick question, no need to change: why did you opt to convert to off64_t instead of just typedeffing git_off_t to off64_t?

I thought that it was clearer for internal uses to use something Very Obviously 64 bit, and something that is a Standard Type. git_off_t feels like the sort of thing that is perhaps useful for our external API, but less useful for our internal.

Your comment did make me rethink a few things, so I actually kept git_off_t in the external type places that it was used. I think this is a pretty reasonable compromise.

pks-t · 2019-11-05T12:55:59Z

 	}

-	assert(sizeof(git_off_t) == 8);
+	assert(sizeof(off64_t) == 8);


Does this make sense? I mean it's very unlikely that off64_t is ever going to be something else than 8 bytes

😬 Yes, that's very true. Removed entirely.

Introduce `git_object_size_t`, an unsigned type that we can use for the maximum size of git objects.

Instead of using a signed type (`off_t`) use a new `git_object_size_t` for the sizes of objects.

Instead of using a signed type (`off_t`) use `uint64_t` for the maximum size of files.

Instead of using a signed type (`off_t`) use a new `git_object_size_t` for the sizes of objects.

Instead of using a signed type (`off_t`) use an unsigned `uint64_t` for the size of the files.

64 bit types are always 64 bit.

Prefer `off64_t` to `git_off_t` for internal visibility.

Prefer `off64_t` to `git_off_t` internally for visibility.

Use int64_t internally for type visibility.

Prefer `off64_t` internally.

pks-t

Thanks a lot, Ed!

pks-t · 2019-11-28T11:36:06Z

+	if (!fc->file->size)
+	    error = git_futils_filesize(&fc->file->size, fd);
+
+	if (error < 0 || !fc->file->size)


This does look a bit weird. Seems like we do not allow loading empty files from disk here. Your change only makes code a lot clearer here, so I'm fine with this as is.

ethomson force-pushed the ethomson/off_t branch 6 times, most recently from 88fc191 to f22e3fc Compare June 16, 2019 21:50

ethomson force-pushed the ethomson/off_t branch 4 times, most recently from 5d18fc5 to 33ff267 Compare June 23, 2019 19:01

ethomson force-pushed the ethomson/off_t branch 4 times, most recently from a2a587d to b871167 Compare July 20, 2019 20:30

ethomson force-pushed the ethomson/off_t branch from 4f033a1 to 54c9fb4 Compare August 11, 2019 20:04

pks-t reviewed Aug 23, 2019

View reviewed changes

ethomson force-pushed the ethomson/off_t branch 2 times, most recently from c689857 to 284d1d9 Compare October 28, 2019 11:07

ethomson mentioned this pull request Nov 1, 2019

Release 0.99 #5291

Merged

2 tasks

tiennou approved these changes Nov 5, 2019

View reviewed changes

pks-t reviewed Nov 5, 2019

View reviewed changes

ethomson added 4 commits November 22, 2019 15:04

types: introduce git_object_size_t

9b04d0b

Introduce `git_object_size_t`, an unsigned type that we can use for the maximum size of git objects.

odb: use git_object_size_t for object size

bed9fc6

Instead of using a signed type (`off_t`) use a new `git_object_size_t` for the sizes of objects.

blob: use git_object_size_t for object size

4334b17

Instead of using a signed type (`off_t`) use a new `git_object_size_t` for the sizes of objects.

futils_filesize: use uint64_t for object size

fb2198d

Instead of using a signed type (`off_t`) use `uint64_t` for the maximum size of files.

ethomson added 4 commits November 22, 2019 15:14

odb: use git_object_size_t for object size

fefefd1

Instead of using a signed type (`off_t`) use a new `git_object_size_t` for the sizes of objects.

filestamp: use uint64_t for object size

6c13cf6

Instead of using a signed type (`off_t`) use an unsigned `uint64_t` for the size of the files.

blame: use a size_t for the buffer

4dffa29

example: use git_object_size_t for object size

f94c927

ethomson force-pushed the ethomson/off_t branch 7 times, most recently from 994e41e to 6c13cf6 Compare November 22, 2019 23:18

ethomson added 5 commits November 25, 2019 13:18

mmap: remove unnecessary assertion

7e1cc29

64 bit types are always 64 bit.

mmap: use a 64-bit signed type off64_t for mmap

8be1202

Prefer `off64_t` to `git_off_t` for internal visibility.

offmap: store off64_t's instead of git_off_t's

ee0c861

Prefer `off64_t` to `git_off_t` internally for visibility.

integer: use int64_t's for checks

05237ee

Use int64_t internally for type visibility.

internal: use off64_t instead of git_off_t

6460e8a

Prefer `off64_t` internally.

ethomson force-pushed the ethomson/off_t branch from f48d720 to 6460e8a Compare November 25, 2019 02:19

pks-t approved these changes Nov 28, 2019

View reviewed changes

pks-t merged commit 0e5243b into master Nov 28, 2019

ethomson deleted the ethomson/off_t branch February 2, 2020 13:07

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Mar 12, 2020

[Snyk] Upgrade nodegit from 0.26.0 to 0.26.4 thisconnect/nodegit-kit#95

Merged

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open



		int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offset)
		int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, off64_t offset)

Uh oh!

Conversation

ethomson commented Jun 16, 2019

Uh oh!

ethomson commented Jun 17, 2019

Uh oh!

ethomson commented Jul 20, 2019

Uh oh!

ethomson commented Aug 11, 2019

Uh oh!

ethomson commented Aug 16, 2019

Uh oh!

pks-t commented Aug 21, 2019 via email

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants