Fix null pointer dereference in status.ByIndex #628

2020-08-14T05:08:07-05:00

mbfr commented

2020-08-14 05:08:07 -05:00

(Migrated from github.com)

git_status_byindex can return a null pointer if there is no statuses.

`git_status_byindex` can return a null pointer if there is no statuses.

lhchavez (Migrated from github.com) reviewed 2020-08-14 08:36:40 -05:00

lhchavez (Migrated from github.com) left a comment

nice, thanks! just some small requests.

status.go

lhchavez (Migrated from github.com) commented

2020-08-14 08:28:41 -05:00

welp, unfortunately there's no consistency with what these functions return :(

Blame.HunkByIndex: 462ebd83e0/blame.go (L102)
Index.EntryByIndex: 462ebd83e0/index.go (L420)
Tree.EntryByIndex: Does nothing and may also need to be fixed (although since it returns a pointer, presumably it could return nil when that is fixed).

But in this case, since libgit2 does not set an error itself, using MakeGitError() will call giterr_last(), which will most likely return GIT_OK, which will cause this error to have an empty message.

so I think the more correct solution here is to do the same thing as Index.EntryByIndex:

		return StatusEntry{}, fmt.Errorf("Index out of Bounds")

welp, unfortunately there's no consistency with what these functions return :( `Blame.HunkByIndex`: https://github.com/libgit2/git2go/blob/462ebd83e0ccba9cd93c05ec12dc3d98064e3d5c/blame.go#L102 `Index.EntryByIndex`: https://github.com/libgit2/git2go/blob/462ebd83e0ccba9cd93c05ec12dc3d98064e3d5c/index.go#L420 `Tree.EntryByIndex`: Does nothing and may also need to be fixed (although since it returns a pointer, presumably it could return `nil` when that is fixed). But in this case, since libgit2 does not set an error itself, using `MakeGitError()` will call [`giterr_last()`](https://github.com/libgit2/git2go/blob/462ebd83e0ccba9cd93c05ec12dc3d98064e3d5c/git.go#L281), which will most likely return `GIT_OK`, which will cause this error to have an empty message. so I *think* the more correct solution here is to do the same thing as `Index.EntryByIndex`: ```suggestion return StatusEntry{}, fmt.Errorf("Index out of Bounds") ```

status_test.go

lhchavez (Migrated from github.com) commented

2020-08-14 08:32:06 -05:00

nit:

	opts := &StatusOptions{
		Show:  StatusShowIndexAndWorkdir,
		Flags: StatusOptIncludeUntracked | StatusOptRenamesHeadToIndex | StatusOptSortCaseSensitively,
	}

this is a bit more idiomatic.

nit: ```suggestion opts := &StatusOptions{ Show: StatusShowIndexAndWorkdir, Flags: StatusOptIncludeUntracked | StatusOptRenamesHeadToIndex | StatusOptSortCaseSensitively, } ``` this is a bit [more idiomatic](https://golang.org/doc/effective_go.html#composite_literals).

lhchavez (Migrated from github.com) commented

2020-08-14 08:36:19 -05:00

nit:

		t.Fatal("expected no statuses in empty repo")

if this fails, then the preconditions for the rest of the test case are not met, which will make the rest of the assertions fail, causing log spam.

nit: ```suggestion t.Fatal("expected no statuses in empty repo") ``` if this fails, then the preconditions for the rest of the test case are not met, which will make the rest of the assertions fail, causing log spam.