Improve git_status_list (git status) #11

julianmesa-gitkraken · 2022-07-06T15:11:47Z

There are 4 commits from romkatv
And one from me
Tested repo: https://axogitlab.axonet.local/Vicente-axosoft/knime_workflows
Perf times:
git status: ~500ms
Before PR git_status_list: ~1300ms
After PR git_status_list: ~750ms

GitHub is removing support for the unauthenticated git protocol; test with the https protocol.

Provide individual file ownership checks for both the current user and the system user, as well as a combined current user and system user check.

Ensure that the repository directory is owned by the current user; this prevents us from opening configuration files that may have been created by an attacker.

Provide a mock for file ownership for testability.

Test that we prevent opening directories that are not owned by ourselves.

Pull the global configuration loader out of the symlink check so that it can be re-used.

Obey the `safe.directory` configuration variable if it is set in the global or system configuration. (Do not try to load this from the repository configuration - to avoid malicious repositories that then mark themselves as safe.)

Introduce the `GIT_OPT_SET_OWNER_VALIDATION` option, so that users can disable repository ownership validation.

Validate repository directory ownership (v1.3)

v1.3.1 release

…library' into libgit-next

…new' into libgit-next

It is a string array of the filters you want to disable in checkout

…USE_NSEC disabled

…s-on-no-nanoseconds-repos Fix degraded performance using GIT_USE_NSEC on repos cloned with GIT_USE_NSEC disabled

There is a typo, Instead comparing nanoseconds to zero the code is comparing seconds to zero

Fix the GIT_USE_NSEC performance fix

Original commit: tiennou@5911878

Original commit: tiennou@bb2a754

Original commit: libgit2@8a565a7 iterator: sort same-dir entries by basename The current code sorts entries in a directory by their full paths. Full paths share long prefixes, which makes comparator slow, which makes sorting slow. TimSort, that libgit2 uses for sorting, performs insertion sort when there are fewer than 65 elements. Most directories have fewer than 65 entries. Insertion sort performs a lot of comparisons, so it's very important to make comparator cheap.

Original commit: romkatv@e52594f

AlexaXs

Reviewed just first commit 8254d2e2aedde8edf6641cea729626d750591205 (Do not add the .gitignore file if it not existing)

AlexaXs · 2022-07-19T08:43:57Z

src/ignore.c

+{
+	char *path_file = NULL;
+
+	path_file = git__malloc(path->size + 11);


Better replace 11 with a definition, like #define GIT_IGNORE_FILE_NAMELENGTH 11

AlexaXs · 2022-07-19T08:44:16Z

src/ignore.c

+		return NULL;
+
+	memcpy(path_file, path->ptr, path->size);
+	memcpy(path_file + path->size, GIT_IGNORE_FILE, 11);


also here, replace 11 with GIT_IGNORE_FILE_NAMELENGTH

AlexaXs · 2022-07-19T08:52:38Z

src/ignore.c

@@ -370,13 +370,40 @@ int git_ignore__for_path(
 	return error;
 }

+char * concat_path_ign_file(git_buf *path)
+{
+	char *path_file = NULL;


I guess we are not using git_buf and git_buf_joinpath (or git_str and git_str_joinpath after libgit2 1.4) for efficiency?

AlexaXs · 2022-07-19T08:55:20Z

src/ignore.c

 	if (git_buf_joinpath(&ign->dir, ign->dir.ptr, dir) < 0)
 		return -1;

 	ign->depth++;

+	// Do not add the .gitignore file if it not existing


Maybe extend a bit the explanation in this comment, like:
// Do not add the .gitignore file if it does not exist, since
// calling push_ignore_file for repos with many dirs is expensive
?

AlexaXs · 2022-07-19T09:03:27Z

src/ignore.c

+	file_ign = concat_path_ign_file(&ign->dir);
+	if (file_ign == NULL)
+		return -1;
+	if (p_stat(file_ign, &st) < 0) {


I tested checking errno after p_stat:

if (p_stat(file_ign, &st) < 0) { free(file_ign); if (errno != ENOENT && errno != ENOTDIR) return -1; else return 0; }

But the times went up a bit. At the moment caller of git_ignore__push_dir is not checking the return value.
Could that be a problem if p_stat fails with (errno != ENOENT && errno != ENOTDIR)?

AlexaXs · 2022-07-19T09:05:02Z

src/ignore.c

 int git_ignore__push_dir(git_ignores *ign, const char *dir)
 {
+	char *file_ign = NULL;
+	struct stat st;
+


missing asserts:

GIT_ASSERT_ARG(ign); GIT_ASSERT_ARG(dir);

AlexaXs

also comment for first commit

AlexaXs · 2022-07-19T14:38:08Z

src/ignore.c

+		free(file_ign);
+		return 0;
+	}
+	free(file_ign);


since we reserve with git_malloc, free with git__free

AlexaXs

Review for only second commit 45f0e262f532a1bde03c8d35d8cdc0b471974a51 (iterator: don't stat directories)

AlexaXs · 2022-07-19T14:46:13Z

src/path.h

@@ -482,6 +482,7 @@ struct git_path_diriter
 	size_t parent_len;

 	unsigned int flags;
+	unsigned char d_type;


It seems d_type is being assigned only for non-Windows platforms, but used for all platforms. Should we add this member as well to the same structure git_path_diriter above for Windows? or maybe specify the platform when used?

AlexaXs · 2022-07-20T09:17:15Z

src/iterator.c

-		if (S_ISDIR(statbuf.st_mode)) {
-			bool submodule = false;
+			/* Ignore wacky things in the filesystem */
+			if (!S_ISDIR(statbuf.st_mode) &&


Do we need to check again if it's a directory? Can a submodule not be a directory?

AlexaXs · 2022-07-20T09:17:28Z

src/iterator.c

-			if (submodule)
-				statbuf.st_mode = GIT_FILEMODE_COMMIT;
+			/* convert submodules to GITLINK and remove trailing slashes */
+			if (S_ISDIR(statbuf.st_mode)) {


Do we need to check again if it's a directory? Can a submodule not be a directory?

AlexaXs · 2022-07-20T09:20:27Z

src/iterator.c

-				statbuf.st_mode = GIT_FILEMODE_COMMIT;
+			/* convert submodules to GITLINK and remove trailing slashes */
+			if (S_ISDIR(statbuf.st_mode)) {
+				if (!submodule) {


matching end of { is confusing, maybe because it's not correctly indented?

implausible and others added 30 commits January 26, 2022 14:36

Implement custom thread local storage for user of library

b45219f

checkout: cleanup duplication in checkout_create_the_new

7a76a33

thread checkout: move checkout buffers to tls

07493cc

thread checkout: add locks around shared state

e1a1eaa

thread checkout: add locks around non thread-safe actions

dc4e588

thread checkout: stub indirection for threading

0739689

thread checkout: add threading to checkout_create_the_new

27f3c80

meta: show build status for v1.3 branch

37caa8d

online: test with https instead of git protocol

6b12762

GitHub is removing support for the unauthenticated git protocol; test with the https protocol.

clone: update bitbucket tests

670415a

path: refactor ownership checks into current user and system

973d959

Provide individual file ownership checks for both the current user and the system user, as well as a combined current user and system user check.

repo: ensure that repo dir is owned by current user

62d492d

Ensure that the repository directory is owned by the current user; this prevents us from opening configuration files that may have been created by an attacker.

fs_path: mock ownership checks

e4eabb0

Provide a mock for file ownership for testability.

repo: test configuration ownership validation

caee92e

Test that we prevent opening directories that are not owned by ourselves.

repo: refactor global config loader function

f683806

Pull the global configuration loader out of the symlink check so that it can be re-used.

repo: honor safe.directory during ownership checks

eb8c3e5

Obey the `safe.directory` configuration variable if it is set in the global or system configuration. (Do not try to load this from the repository configuration - to avoid malicious repositories that then mark themselves as safe.)

repo: make ownership checks optional

b58e905

Introduce the `GIT_OPT_SET_OWNER_VALIDATION` option, so that users can disable repository ownership validation.

Merge pull request libgit2#6268 from libgit2/ethomson/ownership_13

a9eac6a

Validate repository directory ownership (v1.3)

meta: update version numbers for v1.3.1

1f39aac

meta: changelog for v1.3.1

23c24f8

Merge pull request libgit2#6271 from libgit2/ethomson/v1.3.1

1f5e7f9

v1.3.1 release

Merge remote-tracking branch 'zawata/feature/custom-tls-for-external-…

6da6a10

…library' into libgit-next

Merge remote-tracking branch 'zawata/multithread/checkout_create_the_…

30d5c08

…new' into libgit-next

New checkout option: disabled_filters

4b193b1

It is a string array of the filters you want to disable in checkout

Merge branch 'disabled-filters-checkout' into libgit-next

fe44f25

Fix degraded performance using GIT_USE_NSEC on repos cloned with GIT_…

e78ee33

…USE_NSEC disabled

Merge pull request nodegit#7 from julianmesa-gitkraken/fix-nanosecond…

013d416

…s-on-no-nanoseconds-repos Fix degraded performance using GIT_USE_NSEC on repos cloned with GIT_USE_NSEC disabled

Fix the GIT_USE_NSEC performance fix

3ad710a

There is a typo, Instead comparing nanoseconds to zero the code is comparing seconds to zero

Merge pull request nodegit#8 from julianmesa-gitkraken/fix-nsecs-fix

4c98283

Fix the GIT_USE_NSEC performance fix

Do not add the .gitignore file if it not existing

8254d2e

julianmesa-gitkraken added 4 commits July 6, 2022 16:02

iterator: don't stat directories

45f0e26

Original commit: tiennou@5911878

path: use fstatat instead of lstat

a4c112d

Original commit: tiennou@bb2a754

push ignore frames lazily

8bbbfab

Original commit: romkatv@e52594f

AlexaXs reviewed Jul 19, 2022

View reviewed changes

AlexaXs reviewed Jul 20, 2022

View reviewed changes

ianhattendorf force-pushed the libgit-next branch from 0ac7af7 to efdaa4a Compare February 14, 2024 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve git_status_list (git status) #11

Improve git_status_list (git status) #11

julianmesa-gitkraken commented Jul 6, 2022

AlexaXs left a comment

AlexaXs Jul 19, 2022

AlexaXs Jul 19, 2022

AlexaXs Jul 19, 2022 •

edited

Loading

AlexaXs Jul 19, 2022

AlexaXs Jul 19, 2022

AlexaXs Jul 19, 2022

AlexaXs left a comment

AlexaXs Jul 19, 2022

AlexaXs left a comment

AlexaXs Jul 19, 2022

AlexaXs Jul 20, 2022

AlexaXs Jul 20, 2022

AlexaXs Jul 20, 2022

Improve git_status_list (git status) #11

Are you sure you want to change the base?

Improve git_status_list (git status) #11

Conversation

julianmesa-gitkraken commented Jul 6, 2022

AlexaXs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexaXs Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexaXs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexaXs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexaXs Jul 19, 2022 •

edited

Loading