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?

@spacedragon spacedragon Dec 10, 2018

Copy link
Copy Markdown
Contributor Author

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

@implausible implausible left a comment

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.

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