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

Support comments that are detached from any review. #97

Closed
ojarjur opened this issue Apr 27, 2020 · 0 comments · Fixed by #102
Closed

Support comments that are detached from any review. #97

ojarjur opened this issue Apr 27, 2020 · 0 comments · Fixed by #102

Comments

@ojarjur
Copy link
Collaborator

ojarjur commented Apr 27, 2020

The git-appraise tool supports commenting on arbitrary locations in the repository, but it only does so in the context of reviewing a commit.

It would be nice if we could also support commenting on arbitrary locations in the source code without having to wait for a code review.

For example, this could allow someone to pair git-appraise with the git-sync-changes tool to build real-time pair-programming tools on top of git.

My initial thoughts on how this could be done:

  1. Extend git appraise comment by adding a -d flag to indicate that the comment is attached to any review.
  2. Extend the git appraise show command by allowing you to specify a file path instead of a review hash, and then having it show you the detached comments for that path.

Since git-notes always have to be attached to a git-object, we could define a fixed, well-known object to anchor these detached comments. For example, we could pick a "zero" commit of 71ee2d2204443f001fde5aa510ec956d7f90e5e1, which can always be recreated with the following command:

echo "" | \
    GIT_AUTHOR_NAME="nobody" \
    GIT_AUTHOR_EMAIL="nobody" \
    GIT_AUTHOR_DATE="100000000 +0000" \
    GIT_COMMITTER_NAME="nobody" \
    GIT_COMMITTER_EMAIL="nobody" \
    GIT_COMMITTER_DATE="100000000 +0000" \
    git commit-tree `git hash-object -t tree /dev/null`

We could then save that commit to a ref like refs/devtools/archives/71ee2d2204443f001fde5aa510ec956d7f90e5e1 to make sure it does not get garbage collected.

When adding a comment for a path, we would create a new comment struct for that path at the specified commit (defaulting to HEAD if no commit is specified), serialize it, and add it to the git notes for the well-known commit under refs/notes/discuss.

When listing the comments for a path, we would parse the git-notes for the well-known commit under refs/notes/discuss, and then filter out any whose location does not match the provided path.

That would not preserve comments after a file rename, but a caller could account for this by computing the previous file names using the command git log --follow --name-only --pretty=format:"" ${FILENAME}, and then listing all of the detached comments for every previous path.

There is a risk that the use of a single well-known commit might pose a scaling bottleneck, so we will have to load test this with lots of comments to see how well it scales. If that is a issue, then we could use a different, reproducible commit object for each path (such as by adding a message to the otherwise empty commit specifying the matching path).

ojarjur added a commit that referenced this issue Apr 27, 2020
This change cleans up the API for low-level repository operations
in the following ways:

1. Introduce more generic operations on the underlying repository
   DAG objects (i.e. blobs, trees, commits, and refs).
2. Fix a bug in the code for merging archives where we did not
   check if the local archives ref existed before trying to
   resolve it.

Previously, the API exposed by the `repository` interface was very
tailored to the tasks that the git-appraise needed to perform for
code reviews. That meant that adding new features which used the
repository in slightly different ways would require rather large
changes to the repository interface.

Under the new API, any feature that simply relies upon the
features of git should be able to use the API without changes.

This was originally part of the pull request to add support for
forks, but is now being split off into a stand-alone pull request
so that it can be reviewed in isolation, and so that it is
available for other work like the proposed change to support
detached comments (#97).

The bug-fix for the archives issue is being included in here
because the refactoring to clean up the repository code is what
made us catch the underlying bug. The issue with that bug is that
we tried to use a single command ('git show') to both check if
the local archives ref (`refs/devtools/archives/reviews`) exists
and to resolve what commit it points to. However, that command
returns an error if the ref does not exist.

This change fixes it by including an explicit check to see if
the ref exists before trying to resolve it.
ojarjur added a commit that referenced this issue Jun 3, 2020
Cleanup and generalize the API for low-level repository operations.

This change cleans up the API for low-level repository operations
in the following ways:

1. Introduce more generic operations on the underlying repository
   DAG objects (i.e. blobs, trees, commits, and refs).
2. Fix a bug in the code for merging archives where we did not
   check if the local archives ref existed before trying to
   resolve it.

Previously, the API exposed by the `repository` interface was very
tailored to the tasks that the git-appraise needed to perform for
code reviews. That meant that adding new features which used the
repository in slightly different ways would require rather large
changes to the repository interface.

Under the new API, any feature that simply relies upon the
features of git should be able to use the API without changes.

This was originally part of the pull request to add support for
forks, but is now being split off into a stand-alone pull request
so that it can be reviewed in isolation, and so that it is
available for other work like the proposed change to support
detached comments (#97).

The bug-fix for the archives issue is being included in here
because the refactoring to clean up the repository code is what
made us catch the underlying bug. The issue with that bug is that
we tried to use a single command ('git show') to both check if
the local archives ref (`refs/devtools/archives/reviews`) exists
and to resolve what commit it points to. However, that command
returns an error if the ref does not exist.

This change fixes it by including an explicit check to see if
the ref exists before trying to resolve it.
@ojarjur ojarjur closed this as completed in d586caf Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant