Move `git_off_t` to `git_object_size_t` #5123

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

include/git2/diff.h

@@ -603,7 +603,7 @@ typedef struct {
 	int    new_lineno;   /**< Line number in new file or -1 for deleted line */
 	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

src/diff_file.c

@@ -46,7 +46,7 @@ static int diff_file_content_init_common(
 {
 	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

include/git2/diff.h

 	 * 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

src/diff_xdiff.c

@@ -160,7 +160,7 @@ static int git_xdiff_cb(void *priv, mmbuffer_t *bufs, int len)
 		else if (line.origin == GIT_DIFF_LINE_DELETION)
 			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

src/futils.c

@@ -112,7 +112,7 @@ int git_futils_truncate(const char *path, int mode)
 	return 0;
 }

-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

src/futils.h

@@ -330,7 +330,7 @@ extern int git_futils_fake_symlink(const char *new, const char *old);
 */
 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

src/posix.c

@@ -235,7 +235,7 @@ int git__mmap_alignment(size_t *alignment)
 }


-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

src/win32/map.c

@@ -99,7 +99,7 @@ int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offs
 		return -1;
 	}

-	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

src/diff_file.c

+	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

Move git_off_t to git_object_size_t #5123

Move git_off_t to git_object_size_t #5123

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Move `git_off_t` to `git_object_size_t` #5123

Move `git_off_t` to `git_object_size_t` #5123