Fix git_worktree_validate failing on bare repositories by tiennou · Pull Request #4686 · libgit2/libgit2

tiennou · 2018-06-18T18:30:53Z

This fixes #4669, as well as checking that #4684 really isn't our fault (I wasn't sure, since #4630 was about that problem).

Sidenotes :

I was expecting git_worktree_validate to return a "logical C bool", but it instead return negative error codes that don't map to "the standard" ones. That was surprising 😉.

Looking at the diff, I'm also unsure what the list_bare test I moved was actually testing, as it's not run against the testsuite's repository.

ethomson · 2018-06-18T18:35:25Z

I was expecting git_worktree_validate to return a "logical C bool", but it instead return negative error codes that don't map to "the standard" ones. That was surprising 😉.

I wonder if this was an oversight from debugging? The documentation mentions that these will return 0 "or an error code". I would much rather see this returning something in the scope of git_error_code.

tiennou · 2018-06-19T10:27:33Z

Well, I'm not against changing it, but that's against the no-API-change rule, so we'd have to put up a replacement.

typedef enum {
  ok = 0,
  bad = -1,
  …
} git_worktree_status;
int git_worktree_is_valid(git_worktree_status *status, git_worktree *wt);

That would return a 1 on success, 0 on failure, and set *status to the error, which makes it usable like this :

int state;
if (!git_worktree_is_valid(&state, wt) && state == bad1) {
}

Not happy with the enum name though…

ethomson · 2018-06-19T10:37:47Z

We're not changing the API. We're fixing the function to behave as it's documented.

tiennou · 2018-06-19T15:55:55Z

I might be missing something, but I see only three ways to fix that function :

do nothing. The documentation is right, because we either return a 0, or a negative error code. That negative error code is completely out of touch with the API expectations, but can't be changed because API is frozen.
do nothing, but clarify the documentation state that it will return GIT_ERROR/-1 if the gitdir is invalid, -2 (which doesn't actually map to anything) on a missing parent workdir, GIT_ENOTFOUND/ -3 if the commondir cannot be found. Still unexpected, but at least the documentation is correct.
provide a replacement that doesn't use "untyped" negative error codes and deprecate the old one.

ethomson · 2018-06-19T16:00:16Z

The documentation is right, because we either return a 0, or a negative error code.

The documentation is right. The code is wrong. We are not returning error codes, we are returning random numbers. Nobody is relying on that code returning -2 or -3 - because it isn't documented to do that, so there's no backward compatibility to keep.

No need to overthink this, we should just fix the code.

tiennou · 2018-06-19T16:06:11Z

Okay, thanks for the clarification. I'll remap everything to GIT_ERROR then.

ethomson · 2018-06-19T16:11:05Z

Awesome, thanks!

pks-t

I like these changes, thanks for working on them! Some comments below

pks-t · 2018-06-22T07:03:07Z

+	git_worktree *wt;
+
+	cl_git_pass(git_worktree_add(&wt, g_repo, "name", WORKTREE_REPO, NULL));
+	cl_assert_equal_i(0, git_worktree_validate(wt));


I would've just added this additional call to the previous test, but yeah, doesn't really matter too much.

Also it should probably be cl_git_pass instead of cl_assert_equal_i(0, ...

pks-t · 2018-06-22T07:03:43Z


+const char *cl_git_sandbox_path(int is_dir, const char *path, ...)
+{
+	static char _temp[4096];


GIT_PATH_MAX?

True (inspiration was clar_sandbox_path(), which has no such define). Fixed.

pks-t · 2018-06-22T07:05:19Z

+	if (is_dir)
+		git_path_to_dir(&buf);
+
+	git_buf_copy_cstr(_temp, 4096, &buf);


You should check whether this truncates buf, first. Agreed, it is very unlikely, but I don't want to clean up (rm -rf) truncated paths at any point in time.

Fixed by checking the length of buf before copying.

pks-t · 2018-06-22T07:05:55Z

-	git_buf_dispose(&path);
+	cl_assert_equal_s(wt->workdir, cl_git_sandbox_path(1, wtdir, NULL));
+	cl_assert_equal_s(wt->gitlink, cl_git_sandbox_path(0, wtdir, ".git", NULL));
+	cl_assert_equal_s(wt->gitdir, cl_git_sandbox_path(1, parentdir, ".git", "worktrees", wtdir, NULL));


Very nice cleanup

Thanks 😉.

pks-t · 2018-06-22T07:08:27Z

+	va_list arg;
+
+	cl_git_pass(git_buf_sets(&buf, clar_sandbox_path()));
+	if (path) {


This check is kind of superfluous. The signature should rather be const char *cl_git_sandbox_path(int is_dir, ...). Then you could avoid this additional check and just enter right into the while (va_arg(...) != NULL) loop.

True. is_dir was added afterward, hence the path test. Fixed.

tiennou · 2018-06-23T14:05:36Z

Ok, this is now green and ready to go!

pks-t · 2018-06-29T11:45:14Z

+
+	/* make sure we won't truncate */
+	cl_assert(git_buf_len(&buf) < GIT_PATH_MAX);
+	git_buf_copy_cstr(_temp, 4096, &buf);


Sorry to be nitpicky, but this should probably use the same constant GIT_PATH_MAX or sizeof(_temp), shouldn't it?

Good catch. Fixed!

pks-t · 2018-07-06T11:25:58Z

Thanks, @tiennou!

tiennou mentioned this pull request Jun 18, 2018

git_repository_path is incorrect for worktree from bare repo #4684

Closed

pks-t reviewed Jun 22, 2018

View reviewed changes

tiennou force-pushed the fix/more-worktree-from-bare branch from 0ac8a8c to bc9bf5d Compare June 22, 2018 20:03

pks-t reviewed Jun 29, 2018

View reviewed changes

tiennou added 7 commits June 29, 2018 14:39

tests: worktree/bare: gather all tests

7e3c13e

tests: worktree/bare: check git_worktree_list

bd40f0d

tests: worktree/bare: fix git_worktree_validate

83c35f7

tests: add a helper to build sandbox subpaths quickly

d092112

tests: worktree/bare: test some paths

8356bf7

worktree: skip building a buffer when validating

292a6ec

worktree: don't return "untyped" negative numbers as error codes

1da6329

tiennou force-pushed the fix/more-worktree-from-bare branch from bc9bf5d to 1da6329 Compare June 29, 2018 12:39

pks-t merged commit f2a1cec into libgit2:master Jul 6, 2018

tiennou deleted the fix/more-worktree-from-bare branch August 14, 2018 19:03

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 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

Conversation

tiennou commented Jun 18, 2018

Uh oh!

ethomson commented Jun 18, 2018

Uh oh!

tiennou commented Jun 19, 2018

Uh oh!

ethomson commented Jun 19, 2018

Uh oh!

tiennou commented Jun 19, 2018

Uh oh!

ethomson commented Jun 19, 2018

Uh oh!

tiennou commented Jun 19, 2018

Uh oh!

ethomson commented Jun 19, 2018

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!

tiennou commented Jun 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t commented Jul 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants