git: blame, Fix panic when contents and commits have different length #751
Conversation
|
This bug is really annoying, it caused an BridgeCrew Yor's issue, could we merge this patch asap? |
pjbgf
changed the title
|
@williamfzc please squash the commits and reword the commit message to align with the contributing guidelines (e.g. PR's new title). |
done |
references.go
Outdated
| @@ -104,6 +104,16 @@ func walkGraph(result *[]*object.Commit, seen *map[plumbing.Hash]struct{}, curre | |||
| default: // more than one parent contains the path | |||
| // TODO: detect merges that had a conflict, because they must be | |||
| // included in the result here. | |||
|
|
|||
There was a problem hiding this comment.
Most of case 1 and default are the same. This switch could be simplified with:
case 0:
*result = append(*result, current)
return nil
default: // or or more than one parent contains the path
different, err := differentContents(path, current, parents)
if err != nil {
return err
}
if len(different) > 0 {
*result = append(*result, current)
}
for _, p := range parents {
err := walkGraph(result, seen, p, path)
if err != nil {
return err
}
}
}
|
@williamfzc @lonegunmanb the tests are failing as now we are returning more references than the tests expect. Please amend Then you can use that script to update the test cases here. |
@pjbgf ok I have updated all the references. But I am not sure that if they are correct :( . |
|
When the references generated by Apologies but at the moment I don't have the free time to dig in and help more. Feel free to continue trying and ping me if you need another review (or a tests kick off). |
Fix `references` to simulate `rev-list` command's behavior
|
Hi @pjbgf, we‘ve tried to fix the broken tests and updated the pr, would you please kick off the tests? If there're still test errors I'd like to submit my own pr based on @williamfzc's branch so we can fix this panic bug asap, thanks! |
| } | ||
| if !equals { | ||
| h1, _ := blobHash(path, cs[i]) | ||
| h2, _ := blobHash(path, cs[i-1]) |
There was a problem hiding this comment.
Why recalculate the hash for every entry twice? Keep the hash from the last iteration?
| h2, _ := blobHash(path, cs[i-1]) | ||
| equal := h1 == h2 | ||
|
|
||
| if !equal { |
There was a problem hiding this comment.
nit: no need for equal, just do if h1 != h2
| // sorted commit list. Duplication is defined according to "comp". It | ||
| // will always keep the first commit of a series of duplicated commits. | ||
| // sorted commit list. Duplication is defined according to the blob hash. | ||
| // It will always keep the first commit of a series of duplicated commits. | ||
| func removeComp(path string, cs []*object.Commit, comp contentsComparatorFn) ([]*object.Commit, error) { |
There was a problem hiding this comment.
comp is no longer used, if it really isn't needed then remove the param and rename the function
| @@ -90,20 +89,15 @@ func walkGraph(result *[]*object.Commit, seen *map[plumbing.Hash]struct{}, curre | |||
| case 0: | |||
There was a problem hiding this comment.
No longer need the switch here, just change to if len(parents) == 0 {?
| @@ -114,6 +108,19 @@ func walkGraph(result *[]*object.Commit, seen *map[plumbing.Hash]struct{}, curre | |||
| return nil | |||
| } | |||
|
|
|||
| func derivedFromOneParent(path string, current *object.Commit, parents []*object.Commit) (bool, error) { | |||
There was a problem hiding this comment.
What does derivedFromOneParent mean? You are going through all of the parents?
There was a problem hiding this comment.
Not all parents, maybe I should rename the function to derivedFromAnyParent.
| @@ -114,6 +108,19 @@ func walkGraph(result *[]*object.Commit, seen *map[plumbing.Hash]struct{}, curre | |||
| return nil | |||
| } | |||
|
|
|||
| func derivedFromOneParent(path string, current *object.Commit, parents []*object.Commit) (bool, error) { | |||
| for _, parent := range parents { | |||
| different, err := differentContents(path, current, []*object.Commit{parent}) | |||
There was a problem hiding this comment.
differentContents already iterates the list? Why do you need to pass each of the parents individually? Can't you just pass parents?
|
Hi @pjbgf this pr is no longer updated, I've opened a new one #780 based on @williamfzc's work. |
|
Thanks @lonegunmanb . |
Fixes: #603