Skip to content

Restrict potential reference targets of HistoryRewriter #942

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

Merged
merged 1 commit into from
May 27, 2015

Conversation

nulltoken
Copy link
Member

Currently, the HistoryRewriter will blindy rewrite any reference that's being passed to it.

However, this may make no sense to rewrite the remote tracking branches.
Moreover, the way git notes are structured isn't actually handled by the rewriting process.

Proposal:

  • Would a reference which canonical name starts with ref/notes/ be passed, throw a NotSupportedException with a meaningful message.
  • Silently not consider references which canonical name starts with ref/remotes/

Thoughts?

@carlosmn
Copy link
Member

It feels odd that we would have a whitelist or blacklist about rewriting, since it's inherently an operation in which you do whatever you want to the data.

@nulltoken
Copy link
Member Author

It feels odd that we would have a whitelist or blacklist about rewriting, since it's inherently an operation in which you do whatever you want to the data.

So that would be a 👎 with regards to preventing the rewrite of refs/remotes. Makes sense as git.git indeed actually tolerates this.

What about notes? We can't actually rewrite them due to their internal format IIRC.

@ethomson
Copy link
Member

ethomson commented Apr 8, 2015

What about notes? We can't actually rewrite them due to their internal format IIRC.

It makes sense to me if we can't do anything meaningful that we throw... But I agree that rewriting refs/remotes is probably silly but that doesn't necessarily mean we should block it..

@nulltoken
Copy link
Member Author

Hmmm. Actually, the HistoryRewriter only accepts a list of commits. It then infers from them the references to be rewritten.

So two options there:

  • Throw when a commit is reachable from a refs/notes namespace
  • Silently not rewrite the references living under refs/notes

Thoughts?

@nulltoken
Copy link
Member Author

Silently not rewrite the references living under refs/notes

I'm going to go with this option for now, push up something so we can discuss further about it

nulltoken added a commit that referenced this pull request May 9, 2015
@nulltoken
Copy link
Member Author

Ready for review

@nulltoken nulltoken force-pushed the ntk/history_rewriter_notes branch from f39a648 to 59eb6c8 Compare May 9, 2015 15:58
nulltoken added a commit that referenced this pull request May 27, 2015
Restrict potential reference targets of HistoryRewriter
@nulltoken nulltoken merged commit 63f8d6d into vNext May 27, 2015
@nulltoken nulltoken deleted the ntk/history_rewriter_notes branch May 27, 2015 18:02
@nulltoken nulltoken added this to the v0.22 milestone May 27, 2015
@nulltoken
Copy link
Member Author

Published as NuGet pre-release package LibGit2Sharp.0.22.0-pre20150527180327

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

Successfully merging this pull request may close these issues.

3 participants