Skip to content

Conversation

@libin2015
Copy link
Contributor

Geting the changed objects from the patched dir, in order to support
adding new files in patch.

Signed-off-by: Li Bin huawei.libin@huawei.com

Geting the changed objects from the patched dir, in order to support
adding new files in patch.

Signed-off-by: Li Bin <huawei.libin@huawei.com>
@jpoimboe
Copy link
Member

@libin2015 FYI, next time there's no need to open a new pull request. You can just update the old pull request by rebasing and force pushing the existing branch.

@jpoimboe
Copy link
Member

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to set CHANGED=1 here? it seems that if the only thing that changed was the new file, it will bail out a few lines down with "no functional changes found".

Copy link
Member

Choose a reason for hiding this comment

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

I think the "no functional changes found" error would still be appropriate. If they're not changing any functions, why use kpatch?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, after i thought about it for a little while, i tired to think of a situation where someone would do this i.e. call new functions in a new object without changing an existing object.

only situation i could come up with is registering in a new function in a notification chain with a kpatch load hook or something.

i'm fine with it returning an error though.

Copy link
Member

Choose a reason for hiding this comment

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

only situation i could come up with is registering in a new function in a notification chain with a kpatch load hook or something.

Good point. I could go either way... the current patch is fine.

sjenning added a commit that referenced this pull request Oct 16, 2015
kpatch-build: support adding new files in patch
@sjenning sjenning merged commit d444caa into dynup:master Oct 16, 2015
@libin2015
Copy link
Contributor Author

@libin2015 FYI, next time there's no need to open a new pull request. You can just update the old pull request by rebasing and force pushing the existing branch

But if donging in this way, the previous comments based on the previous patch will be viewed odd?

@jpoimboe
Copy link
Member

But if donging in this way, the previous comments based on the previous patch will be viewed odd?

In the past we have noticed patch comments that disappear after rebasing the branch. But that problem only happened when adding a comment to a "Commit" rather than the "Files Changed" tab. I don't know if GitHub still has that quirk. But our policy to avoid this problem is to always add review comments to the "Files Changed" tab rather than the "Commits" tab.

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