Gitserver: add StatusTypeChanged to supported statuses in ChangedFiles#62548
Conversation
Graphite Automations"Auto-assign PRs to author" took an action on this PR • (05/08/24)3 assignees were added to this PR based on Geoffrey Gilmore's automation. |
|
Oops. Didn't mean to assign anyone yet. Still adding tests. |
|
sorry, rogue automation |
StatusTypeChanged to supported statuses in ChangedFiles
| expectedChanges := []gitdomain.PathStatus{ | ||
| {Path: "f1", Status: gitdomain.StatusTypeChanged}, | ||
| {Path: "f2", Status: gitdomain.StatusAdded}, | ||
| } |
There was a problem hiding this comment.
Added a test that we correctly parse StatusTypeChange
|
|
||
| // GitChangedFiles returns an iterator that yields list of changed files that have changed in between commitA and commitB in the given | ||
| // repo. | ||
| // repo. The returned statuses will be one of StatusAdded, StatusRemoved, StatusModified, or StatusTypeChanged |
There was a problem hiding this comment.
Documented which statuses can be returned by this function.
| StatusUnspecified Status = 'X' | ||
| StatusAdded Status = 'A' | ||
| StatusModified Status = 'M' | ||
| StatusDeleted Status = 'D' | ||
| StatusTypeChanged Status = 'T' | ||
| // TODO: add other possible statuses here (see man git-diff-tree) |
There was a problem hiding this comment.
Modified the type of status to be byte because 1) it's smaller, 2) it's directly comparable to what we get from Git, and 3) it's more decipherable when debugging.
| STATUS_ADDED = 1; | ||
| STATUS_MODIFIED = 2; | ||
| STATUS_DELETED = 3; | ||
| STATUS_TYPE_CHANGED = 4; |
There was a problem hiding this comment.
Added an item to the enum. The rest of this seems to be output from buf format and my editor's comment wrapping (sorry)
| {Status: gitdomain.StatusModified, Path: "changed.go"}, | ||
| {Status: gitdomain.StatusAdded, Path: "added.md"}, | ||
| {Status: gitdomain.StatusDeleted, Path: "removed.md"}, | ||
| {Status: gitdomain.StatusTypeChanged, Path: "type_changed.md"}, |
There was a problem hiding this comment.
Added an entry to the iterator to ensure that the test output doesn't change with a T status
| expectedChanges := []gitdomain.PathStatus{ | ||
| {Path: "f1", Status: gitdomain.StatusTypeChanged}, | ||
| {Path: "f2", Status: gitdomain.StatusAdded}, | ||
| } |
Co-authored-by: Erik Seliger <erikseliger@me.com>
| // doesn't exist in "indexed" | ||
| unindexedSearch = append(unindexedSearch, c.Path) | ||
| default: | ||
| // ignore any other statuses |
There was a problem hiding this comment.
I know this is preserving the previous behaviour, but I think it's clearer to say type change doesn't affect search results. Does it make sense from a style perspective to handle it but have a noop body?
There was a problem hiding this comment.
Yep, that's aligned with @ggilmore's comments too. Changed!
This adds support for
Tstatuses (type changed) toChangedFiles. This was previously hard-erroring, which was causing problems for code insights and hybrid search.Apologies for the large diff. Part of this required adding a "type changed" member to the
StatusAMDenum, which would be confusing becauseAMDstands for "added, modified, deleted". I renamedStatusAMDtoStatus, renamed the members to a pattern likeStatusAdded, and addedStatusTypeChanged. I checked every consumer of this enum to ensure that it safely handled new values.Because of the noise from the rename, I added comments to all the important bits of this PR.
Related Slack discussion
Test plan
Added test both to the git CLI backend and to hybrid search, which is a consumer of this API.