-
Notifications
You must be signed in to change notification settings - Fork 910
Remote tracking branches pruning #925
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.
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
Conversation
#695 was about "on demand" pruning. |
LibGit2Sharp/Remote.cs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively return shouldPruneOnFetch ?? false;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldPruneOnFetch
holds a ConfigurationEntry
, not a bool, so I'm afraid this shortcut wouldn't work here.
Is the purpose of |
@jamill I've thought about this as well, but was unsure about the outcome. Are we trying to determine if this remote is configured to prune on fetch, or we we trying to determine if the next fetch against this remote is going to prune? Thoughts? Whatever the option we will choose, the xml doc should be updated to reflect the underlying logic. /cc @carlosmn |
LibGit2Sharp/Remote.cs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Add some description about the logic when we reach an agreement (also consider fetch.prune
or not?)
I would expect this to tell me if fetching from this remote would prune. I would find it unexpected if If all this is doing is providing a convenience to read off the |
@jamill PR has been updated. Better? |
LibGit2Sharp/RemoteUpdater.cs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we also need a way to clear this value? maybe this is just a Getter property? And possibly expose a method that can set true, false, or clear it (or just force setting to go through the repo.Config.Set
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the getter is a combined value, this setter is now confusing. Let's drop the RemoteUpdater
implementation for now.
70eebbf
to
7fff0df
Compare
Determining that would require a I would go with looking at the configuration (as you have now) since I plan to remove the setter from |
Yeah - I meant what the default behavior would be without passing in any options to override the default behavior (I tried to capture this with
Not sure I follow - which setter you are removing? The ability to override this as part of a fetch? The ability to set this config value through |
I mean the |
7fff0df
to
6b63f6e
Compare
6b63f6e
to
2d5cdda
Compare
Changes Unknown when pulling 2d5cdda on ntk/remote_prune into * on vNext*. |
libgit2/libgit2#2761 has been merged and brings a lot of niceties!
We may now expose the following
remote.ShouldPruneOnFetch { get; }
remoteUpdater.ShouldPruneOnFetch { get; set; }
Along with some basic test coverage to ensure everything works as expected