search

google / git-appraise

Distributed code review system for Git repos
Apache License 2.0
5.13k stars 146 forks source link

Filename or path using in comment is not validated #41

Open metafeather opened 8 years ago

metafeather commented 8 years ago

The command below falsely succeeds, even though the filename or path is invalid: git appraise comment -f missspelled.go -m 'some message'

These comments seem to end up in the object referenced by .git/refs/notes/devtools/discuss and cause an error when listing with:

git appraise show <sha>
> comments (2 threads):
> ... comment 1 ...
> fatal: Path 'missspelled.go' does not exist in '<sha>'

All comments, including the bad one, can be seen in the --json output.

It would also be nice to have guidance on editing/amending comments before pushing, since naively using git notes remove <sha> does not appear to work(?)

In the meantime I discovered that it was possible to checkout the comments into their own branch and remove the problem comment usinggit rebase --interactive:

git checkout -B comments refs/notes/devtools/discuss
git rebase -i HEAD~2
drop the commit, and manually fix the merge after git rebase --continue
git update-ref refs/notes/devtools/discuss comments
git checkout master
git branch -D comments
metafeather commented 8 years ago

Similarly there is no validation that the line number is within the length of the referenced file (should you accidentally add a line comment on the wrong file).

ojarjur commented 8 years ago

Thanks for reporting this, and I'm sorry it hit you.

I'll put together some fixes on Monday.

ojarjur commented 8 years ago

There are two different checks we need to put in place:

  1. Make sure that the specified file and line exist at the time the comment is created.
  2. Gracefully handle the file and/or line not existing when displaying a comment.

The second is required because a file and/or line might stop existing after a comment is written (if history is changed during the review), but we still want to be able to display those comments after such a thing happens.

ojarjur commented 8 years ago

The first issue was fixed in 6bbed27428fd, but I'm still leaving this bug open until I get the second issue addressed.