Repository navigation
feat: emit a warning when implicit parent conflict found - #232
Conversation
letFunny
commented
Jul 15, 2025
letFunny
commented
- Have you signed the CLA?
|
| if err != nil { | ||
| return err | ||
| } | ||
| // Content created was not listed in a slice contents because extractInfo |
There was a problem hiding this comment.
[Comment for reviewer]: This was an optimization which was only skipping some variable assignment and an empty for loop. It is simpler if we remove it and continue processing the path normally rather than adding the logic for finding implicit conflicts twice.
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Looking good. Only minors:
| if err != nil { | ||
| return err | ||
| } | ||
| // Content created was not listed in a slice contents because extractInfo |
| if mode, ok := createdNotListed[relPath]; ok && mode != o.Mode { | ||
| implicitConflicts = append(implicitConflicts, relPath) | ||
| } | ||
| createdNotListed[relPath] = o.Mode |
There was a problem hiding this comment.
Isn't inSliceContents and createdNotListed opposites of each other? If so, let's please unify terminology.
There was a problem hiding this comment.
I have changed inSliceContents to listed which I think is also much simpler and enough given the context of the function and the terminology. It also matches the rest of the comments were we generally use "listed".
There was a problem hiding this comment.
This was the correct one apparently? That literally says "in slice contents", right? The "contents" field under the "slices" key?
There was a problem hiding this comment.
Sure, we can use that one, I just thought the other one will be too long: notInSliceContents.
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Please see replies to your comments.
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Just a trivial and LGTM, thanks for the work on this.
