Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filename or path using in comment is not validated #41

Open
metafeather opened this issue Feb 19, 2016 · 4 comments
Open

Filename or path using in comment is not validated #41

metafeather opened this issue Feb 19, 2016 · 4 comments
Assignees
Labels

Comments

@metafeather
Copy link

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
Copy link
Author

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
Copy link
Collaborator

ojarjur commented Feb 19, 2016

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

I'll put together some fixes on Monday.

@ojarjur ojarjur added the bug label Feb 22, 2016
@ojarjur
Copy link
Collaborator

ojarjur commented Feb 23, 2016

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 ojarjur self-assigned this Feb 24, 2016
@ojarjur
Copy link
Collaborator

ojarjur commented Feb 24, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants