Skip to content

Add support for directories when using fileHistoryWalk#1583

Merged
implausible merged 2 commits into
nodegit:masterfrom
elastic:yl/dirhistory
Dec 13, 2018
Merged

Add support for directories when using fileHistoryWalk#1583
implausible merged 2 commits into
nodegit:masterfrom
elastic:yl/dirhistory

Conversation

@spacedragon
Copy link
Copy Markdown
Contributor

No description provided.

@spacedragon spacedragon force-pushed the yl/dirhistory branch 2 times, most recently from 0b9f656 to c38e88c Compare November 8, 2018 07:01
@emilsedgh
Copy link
Copy Markdown

I was just looking at the documentation to see how this could be done. Any updates on merging this?

if (isEqualNewFile) {
std::pair<git_commit *, std::pair<char *, git_delta_t> > *historyEntry;
if (!isEqualOldFile) {
if (!isEqualOldFile || delta ->status == GIT_DELTA_RENAMED) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you fix the spacing on the arrow between delta and status?

Also why was adding git_delta_renamed here necessary?

Copy link
Copy Markdown
Contributor Author

@spacedragon spacedragon Dec 10, 2018

Choose a reason for hiding this comment

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

Before this pr, when:

  1. isEqualNewFile == true && isEqualOldFile == false
    the file is created or a file is renamed
  2. isEqualNewFile == true && isEqualOldFile == true
    the file content changed
    ...

After this pr, the meaning of isEqualNewFile and isEqualOldFile is changed because of strncmp ,

  1. isEqualNewFile == true && isEqualOldFile == false
    the file is created in this folder or a file is moved to this folder
  2. isEqualNewFile == true && isEqualOldFile == true
    the file content changed or a file is renamed but stays in the same folder

Without delta ->status == GIT_DELTA_RENAMED, the 'a file is renamed but stays in the same folder' will be handled line 194, which will cause panic when we collect the results here at line 266

Copy link
Copy Markdown
Member

@implausible implausible left a comment

Choose a reason for hiding this comment

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

Thanks for your work here! Nicely done 😄 💯

@implausible implausible merged commit 4a66926 into nodegit:master Dec 13, 2018
@spacedragon spacedragon deleted the yl/dirhistory branch December 13, 2018 03:51
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 this pull request may close these issues.

3 participants