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

Already on GitHub? Sign in to your account

Introduce reflog #388

Merged
merged 3 commits into from Apr 10, 2013

Conversation

Projects
None yet
4 participants
Contributor

Saaman commented Apr 8, 2013

This PR is originated by issue #371.

I first introduced the ability to read the reflog :

ReflogCollection reflog = repo.Refs.Log(string canonicalReferenceName);
ReflogCollection reflog = repo.Refs.Log(Reference reference);

A ReflogCollectionis a IEnumerable<ReflogEntry>, where ReflogEntrycontains details about a single entry.

Then I corrected the issue by calling ReflogCollection.Append() from the Repository.Commit() method.
I implemented correct message formatting for standard commits and ammended commits. I'm not sure if there is another case to cover.

  • Travis OK
  • First review
  • First amend
  • Merged!
Member

nulltoken commented Apr 8, 2013

@Haacked How do you feel about this?

Member

nulltoken commented Apr 8, 2013

@Saaman Travis chokes because of a tiny case related issue.

Contributor

Haacked commented Apr 8, 2013

@nulltoken excited to see this! Having it automatically created on commit sounds good to me.

What about other cases when reflog entries are created such as checking out branches and merging two branches?

Member

nulltoken commented Apr 8, 2013

What about other cases when reflog entries are created such as checking out branches and merging two branches?

@Haacked Merging seems a bit far away. However, checking out sounds achievable.

Maybe could you create a separate feature request with the use cases you'd like to see implemented?

Contributor

Haacked commented Apr 8, 2013

Sure.

Contributor

Saaman commented Apr 8, 2013

@nulltoken Sorry, I missed a couple of things in my class definitions. Travis should not complain anymore. 🙏

@nulltoken nulltoken and 1 other commented on an outdated diff Apr 9, 2013

LibGit2Sharp.Tests/ReflogFixture.cs
+ {
+ [Fact]
+ public void CanReadReflog()
+ {
+ const int expectedReflogEntriesCount = 3;
+
+
+ using (var repo = new Repository(StandardTestRepoWorkingDirPath))
+ {
+ var reflog = repo.Refs.Log(repo.Refs.Head);
+
+ Assert.Equal(expectedReflogEntriesCount, reflog.Count());
+
+ // Initial commit assertions
+ Assert.Equal("timothy.clem@gmail.com", reflog.Last().Commiter.Email);
+ Assert.Contains("clone: from c:/GitHub/libgit2sharp/Resources/testrepo.git", reflog.Last().Message);
@nulltoken

nulltoken Apr 9, 2013

Member

Would StartWith() work as well?

@Saaman

Saaman Apr 9, 2013

Contributor

I used Contains() because there was a trailing "\r" in the clone message... Maybe a minor bug from libgit? This does not happen on commit related reflog entries.
Anyway, I can make it work with Assert.Equal() adding the trailing "\r" in my expectation.
Although, StartWith() does not belong to XUnit built-in assertions methods!

@nulltoken nulltoken commented on an outdated diff Apr 9, 2013

LibGit2Sharp/ReflogCollection.cs
+ {
+ get
+ {
+ return string.Format(CultureInfo.InvariantCulture,
+ "Count = {0}", this.Count());
+ }
+ }
+
+ /// <summary>
+ /// Add a new <see cref="ReflogEntry"/> to the current <see cref="ReflogCollection"/>. It will be created as first item of the collection
+ /// The native reflog object will be saved right after inserting the entry.
+ /// </summary>
+ /// <param name="objectId">the <see cref="ObjectId"/> of the new commit the <see cref="Reference"/> will point out.</param>
+ /// <param name="committer"><see cref="Signature"/> of the author of the new commit.</param>
+ /// <param name="message">the message associated with the new <see cref="ReflogEntry"/>.</param>
+ public virtual void Append(ObjectId objectId, Signature committer, string message)
@nulltoken

nulltoken Apr 9, 2013

Member

Please, make this internal

@nulltoken nulltoken commented on the diff Apr 9, 2013

LibGit2Sharp/ReflogEntry.cs
+ /// <see cref="Signature"/> of the commiter of this reference update
+ /// </summary>
+ public virtual Signature Commiter
+ {
+ get { return _commiter; }
+ }
+
+ /// <summary>
+ /// the message assiocated to this reference update
+ /// </summary>
+ public virtual string Message
+ {
+ get { return message; }
+ }
+ }
+}
@nulltoken

nulltoken Apr 9, 2013

Member

ZOMG! Someone shamelessly stole this trailing newline.... Help! Someone call the Diff Police!!!! 🚓

@Saaman

Saaman Apr 9, 2013

Contributor

Arf... I forgot about this, sorry!

@nulltoken nulltoken and 1 other commented on an outdated diff Apr 9, 2013

LibGit2Sharp/Repository.cs
@@ -680,6 +680,10 @@ public Commit Commit(string message, Signature author, Signature committer, bool
Proxy.git_repository_merge_cleanup(handle);
+ // Insert reflog entry
+ string reflogMessage = string.Format("commit{0}: {1}", (amendPreviousCommit ? " (amend)" : string.Empty), message);
+ Refs.Log("HEAD").Append(result.Id, committer, reflogMessage);
@nulltoken

nulltoken Apr 9, 2013

Member

I think that if the current HEAD is not detached (ie. points to another reference), the log of target reference should also be updated.

Cornercase: I don't know what should happen given the following configuration: HEAD -> A -> B (with HEAD and A being symrefs). Should all three reflogs be updated or only HEAD and B?

@Saaman

Saaman Apr 9, 2013

Contributor

I thought about this, even though I'm not completly at ease with these matters... Thing is, I was not able to figure out what references are impacted by the current Commit() action.

@nulltoken nulltoken and 2 others commented on an outdated diff Apr 9, 2013

LibGit2Sharp/ReflogEntry.cs
+ _commiter = Proxy.git_reflog_entry_committer(entryHandle);
+ message = Proxy.git_reflog_entry_message(entryHandle);
+ }
+
+ /// <summary>
+ /// <see cref="ObjectId"/> targeted before the reference update described by this <see cref="ReflogEntry"/>
+ /// </summary>
+ public virtual ObjectId OldTargetOid
+ {
+ get { return _oldTargetOid; }
+ }
+
+ /// <summary>
+ /// <see cref="ObjectId"/> targeted after the reference update described by this <see cref="ReflogEntry"/>
+ /// </summary>
+ public virtual ObjectId NewTargetOid
@nulltoken

nulltoken Apr 9, 2013

Member

How about shortening this to TargetOid?

@Saaman

Saaman Apr 9, 2013

Contributor

You mean to keep the OldTargetOid named this way but assume that TargetOid is the new target of the reference?

@yorah

yorah Apr 9, 2013

Contributor

How about naming the properties From and To?

@Saaman

Saaman Apr 9, 2013

Contributor

Good idea! 👍

Saaman added some commits Apr 10, 2013

@Saaman Saaman Allow update of the target of a symbolic reference with another symbo…
…lic reference
3818634
@Saaman Saaman Introduce Repository.Refs.Log()
returns the reflog of the reference given as parameter
030089b
@Saaman Saaman Insert new reflog entry on commit
Fix #371

Insert entry on HEAD and target direct reference on commit
Tag as initial the first commit
90c9b20
Contributor

Saaman commented Apr 10, 2013

Here is a first amend on the PR.
Among some rewriting, I also :

  • fixed a bug on ReferenceCollection.UpdateTarget(). It was not possible to have "HEAD" target a SymbolicReference. It worked with signature using references names, but not with the one using actual Reference objects.
  • When "HEAD" targets a SymbolicReference the "HEAD" reflog and DirectReference reflog must be updated (this is the behavior of git.exe).
  • Add (initial) in the reflog entry message when the Reference is unborn.

@nulltoken nulltoken merged commit 90c9b20 into libgit2:vNext Apr 10, 2013

1 check passed

default The Travis build passed
Details
Member

nulltoken commented Apr 10, 2013

@Saaman Travis ❤️ you. And so am I!

Merged.

@Saaman Saaman referenced this pull request Apr 11, 2013

Merged

Additional tests for Reflog #390

@Saaman Saaman deleted the Saaman:topic/reflog branch Apr 15, 2013

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