Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OPT/RF: AnnexRepo.commit - remove invocation to commit --dry-run + add check for unknown files #4127

Closed
wants to merge 1 commit into from
Closed

OPT/RF: AnnexRepo.commit - remove invocation to commit --dry-run + add check for unknown files #4127

wants to merge 1 commit into from

Conversation

yarikoptic
Copy link
Member

Closes #4126

This PR is to provide OPT/RF aiming to not change behavior. Subsequent issue would be to point to desired further RF.

This was referenced Feb 10, 2020
Closed
Closed
kyleam
kyleam suggested changes Feb 10, 2020
View reviewed changes
Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

  • As indicated by the failing tests, this won't work for directories.

  • It'd be nice to have some demonstration of how much of an optimization this is.

  • I think the commit message should argue why this is worth doing at all given that this is not a heavily used part of code because the save command does not go through it.

@kyleam
Copy link
Contributor

kyleam commented Feb 10, 2020

I said:

  • [...] this is not a heavily used part of code because the save command does not go through it.

Grepping through the codebase, I'm not sure there is any command (except the obsolete interface.save) that calls AnnexRepo.commit with a values for files.

@yarikoptic
Copy link
Member Author

do you remember if there is a reason to keep obsolete interface.save around? may be we should just kill both AnnexRepo.commit and interface.save?

@kyleam
Copy link
Contributor

kyleam commented Feb 10, 2020

do you remember if there is a reason to keep obsolete interface.save around?

The obsolete add still depends on interface.save. The "remove add/interface.save" waters were tested a bit with gh-3735.

Aside from some compatibility-window for third-party code that uses add, I think the main reason is just compatibility with extensions. Now that a non-rc 0.12 is out, we could have releases in all the extensions that rework them as needed and bump their minimal datalad dependency to 0.12.

@mih
Copy link
Member

mih commented Feb 10, 2020

FTR: I would be delighted to get rid of all of this code!!! And its tests! (and yes, I did port all that still make sense)

@mih
Copy link
Member

mih commented Feb 20, 2020

Old save and add are gone now.

mih added a commit to mih/datalad that referenced this pull request Feb 20, 2020
@mih
CLN: Remove obsolete AnnexRepo.commit()
bfd9aeb 
The GitRepo implementation should be capable of doing what is needed in
the rest of the codebase.

Removing it helps with dataladgh-2970 and makes dataladgh-4127 obsolete by fixing
dataladgh-4126 too.
@mih mih mentioned this pull request Feb 20, 2020
Merged
@yarikoptic
Copy link
Member Author

superseded by #4168 which removed that .commit

@yarikoptic yarikoptic closed this Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyleam kyleam kyleam requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AnnexRepo.commit() investigate performance of test for untracked files
3 participants
@yarikoptic @kyleam @mih