Blame it on the Rain by ben · Pull Request #552 · libgit2/libgit2sharp

ben · 2013-11-06T20:39:16Z

Bind the git_blame_* APIs.

marshalling
file blame
XML doc-comments
~~buffer blame - what should the signature be?~~ Remove buffer-blame binding until needed
~~Should this and Diff go into a Changes namespace?~~ (separate PR)
Demonstrate how to blame a file that has been removed in a previous commit
Until/Since naming
Squash
1-based or 0-based line numbers?

Closes #492.

dahlbyk · 2013-11-06T22:15:18Z

/cc #492

nulltoken · 2013-11-08T09:48:21Z

LibGit2Sharp.Tests/BlameFixture.cs

Missing trailing newline hunt mode on 😛

nulltoken · 2013-11-08T11:03:10Z

@ben you rock!

Final nitpick (at least for now 😉), could you please add braces to if statements?

nulltoken · 2013-11-08T11:07:50Z

nulltoken · 2013-11-08T12:42:45Z

git-blame: Show what revision and author last modified each line of a file.
git-diff: Show changes between commits, commit and working tree, etc.

Currently, we're exposing a Compare<> method under the Diff namespace.

I was wondering if that would make sense to introduce a Changes namespace (or another one conveying the same concept) under with we could expose Blame() and Diff<> methods.

/cc @dahlbyk @carlosmn @yorah

ben · 2013-11-09T16:58:17Z

LibGit2Sharp/Blame.cs

Should this be called BlameHunkCollection?

Why not? How does it feel from the code perspective?

👍 for change - it better communicates how to use the type.

Should this also implement IEnumerable<BlameHunk>?

ben · 2013-11-09T18:16:33Z

Added newlines to the end of all the files, and put braces on all the if-statements. If anyone wants to try this merge and track down the crash that's happening on the 64-bit test run, I'd happily buy them 🍻.

ethomson · 2013-11-10T02:57:19Z

Mmm, tasty 🍺.

BTW, was crashing for me on x86 and amd64 both, freeing some memory in a guard page. The CLR stomped all over that pointer you were returning.

(At least I think ben#1 is what's needed. Watching the memory window shows that the marshaller takes a crap on that memory in the marshalling but pinvoke is mostly magic to me, so I'm not 100%.)

nulltoken · 2013-11-13T15:07:50Z

LibGit2Sharp/Blame.cs

I'd rather not use the [Obsolete] argument for this kind of usage.

How about just dropping the entries?

It's done. 😄

ben · 2013-11-14T18:25:25Z

Rebased so I can make sure Travis is happy. I'll have to rebase again once #565 is merged, but I figure this should be squashed a bit anyway.

I've updated the description with the tasks that are remaining.

nulltoken · 2013-11-15T09:09:35Z

LibGit2Sharp/Blame.cs

I was under this impression that you were willing to name this BlameHunkCollection

Could you please make this IEnumerable<BlameHunk>?

👍 on both counts.

ben · 2013-11-21T23:03:20Z

Regarding the naming of the latest and earliest committishes to consider: what about StartingAt for the newest one, and StoppingAt for the oldest? @dahlbyk, what do you think?

dahlbyk · 2013-11-22T02:20:45Z

what about StartingAt for the newest one, and StoppingAt for the oldest?

👍

ben · 2013-11-22T03:30:31Z

Done! 🌞

@nulltoken, we should probably squash this down. Do you want anything separated out, or just one big commit?

dahlbyk · 2013-11-22T03:45:28Z

LibGit2Sharp/BlameOptions.cs

This can be sealed with non-virtual members.

dahlbyk · 2013-11-22T03:47:56Z

Do you want anything separated out, or just one big commit?

I'd put the Committishes() refactoring in a commit by itself.

nulltoken · 2013-11-22T06:43:37Z

LibGit2Sharp/BlameHunk.cs

How about moving to zero based?

ben · 2013-11-22T20:58:44Z

Switched to 0-based numbering, and sealed BlameOptions. Rebasing now…

ben · 2013-11-22T21:54:13Z

There, down to two commits. And Travis is happy. Is anyone else happy? I'm happy.

dahlbyk · 2013-11-22T22:05:46Z

LibGit2Sharp/BlameHunk.cs

I know you just changed this, but this still feels like a really good reason to use 1 ourselves...

Not to mention that most UIs will likely want to render the full blame, with line numbers that the user will expect...starting at 1.

@dahlbyk 👊 @nulltoken Who will win?

I originally made the numbering 1-based for exactly those reasons, but it does feel a little weird.

I haven't used this, but think I like using 0 - based numbering... (and then letting the UI's on top of us handle the conversion to 1 based).

@dahlbyk 👊@nulltoken Who will win?

@nulltoken, of course. I'm a pacifist. Emphasis on fist.

dahlbyk · 2013-11-22T22:11:09Z

Should there be a test exercising StoppingAt?

ben · 2013-11-22T22:36:22Z

I suppose so.

ben · 2013-11-22T22:41:42Z

There.

jamill · 2013-11-22T22:48:21Z

LibGit2Sharp/BlameHunk.cs

I was just looking at the above comment and noticed that you use FinalStartLineNumber 2X here. Should one of these be rawhunk.OrigStartLineNumber?

WOO, good catch. The orig* fields aren't being filled in just yet, but at some point they will be. 💗

dahlbyk · 2013-11-22T23:22:44Z

There.

Be that way

ben · 2013-11-22T23:29:24Z

Fixes incorporated and squashed.

dahlbyk · 2013-11-23T03:28:53Z

I am saddened that highlighting an image and hitting r doesn't quote it.

nulltoken · 2013-11-23T19:10:32Z

LibGit2Sharp/BlameHunk.cs

iffffffff 😝

nulltoken · 2013-11-24T20:27:33Z

@ben THIS IS ALL YOUR FAULT!

...and I 💓 you for this.

Aimeast · 2013-11-25T05:56:34Z

Thanks for all!

I am try to blame

using (var repo = new Repository(@"path\to\libgit2sharp")) // updated to 4f4819e
{
    var blame = repo.Blame(".gitignore");
}

The debugger display is

blame.ToArray()
{LibGit2Sharp.BlameHunk[9]}
    [0]: 0-15 (1583de3)
    [1]: 15-16 (1498ed4)
    [2]: 16-32 (1583de3)
    [3]: 32-33 (bf9ffdc)
    [4]: 33-35 (0bee099)
    [5]: 35-36 (1583de3)
    [6]: 36-37 (96df5f8)
    [7]: 37-39 (48ea4d5)
    [8]: 39-41 (96df5f8)

And libgit2sharp is 0 based line number written in code

// Libgit2's line numbers are 1-based
FinalStartLineNumber = rawHunk.FinalStartLineNumber - 1;
InitialStartLineNumber = rawHunk.OrigStartLineNumber - 1;

So, I think the line range number which in the debugger display not quite right. The information [0]: 0-15 (1583de3) means from line 0 to line 15 which including 0 and 15. In my understanding, display 0-14 is clear than 0-15.

/cc @nulltoken @ben

ben · 2013-11-25T22:27:03Z

@Aimeast: 1517a66!

nulltoken mentioned this pull request Nov 6, 2013

Support "git blame" #492

Closed

nulltoken reviewed Nov 8, 2013
View reviewed changes

ben reviewed Nov 9, 2013
View reviewed changes

ben mentioned this pull request Nov 13, 2013

Blame fixes libgit2/libgit2#1958

Merged

nulltoken reviewed Nov 13, 2013
View reviewed changes

ben mentioned this pull request Nov 13, 2013

Update libgit2 binaries to 98eaf39 #565

Merged

nulltoken reviewed Nov 15, 2013
View reviewed changes

dahlbyk reviewed Nov 22, 2013
View reviewed changes

LibGit2Sharp/BlameOptions.cs Outdated

Copy link

Member

dahlbyk Nov 22, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be sealed with non-virtual members.

nulltoken reviewed Nov 22, 2013
View reviewed changes

LibGit2Sharp/BlameHunk.cs Outdated

Copy link

Member

nulltoken Nov 22, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about moving to zero based?

Introduce repo.Committish(es)

a3f95fc

dahlbyk reviewed Nov 22, 2013
View reviewed changes

jamill reviewed Nov 22, 2013
View reviewed changes

nulltoken reviewed Nov 23, 2013
View reviewed changes

Introduce Repository.Blame

4f4819e

nulltoken merged commit 4f4819e into libgit2:vNext Nov 24, 2013

ben deleted the blame branch November 25, 2013 16:31

ben mentioned this pull request Nov 25, 2013

Fix off-by-one error in BlameHunk debugger display #575

Merged

Conversation

ben commented Nov 6, 2013

Uh oh!

dahlbyk commented Nov 6, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nulltoken commented Nov 8, 2013

Uh oh!

nulltoken commented Nov 8, 2013

Uh oh!

nulltoken commented Nov 8, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben commented Nov 9, 2013

Uh oh!

ethomson commented Nov 10, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben commented Nov 14, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben commented Nov 21, 2013

Uh oh!

dahlbyk commented Nov 22, 2013

Uh oh!

ben commented Nov 22, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dahlbyk commented Nov 22, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben commented Nov 22, 2013

Uh oh!

ben commented Nov 22, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dahlbyk commented Nov 22, 2013

Uh oh!

ben commented Nov 22, 2013

Uh oh!

ben commented Nov 22, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dahlbyk commented Nov 22, 2013

Uh oh!

ben commented Nov 22, 2013

Uh oh!

dahlbyk commented Nov 23, 2013

Uh oh!

Choose a reason for hiding this comment