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

Already on GitHub? Sign in to your account

Add unmatched pathspecs options #343

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

yorah commented Feb 19, 2013

This is related to libgit2/libgit2sharp#274

  • 💤 Implementation in progress
  • 🔦 Ready for review
  • 🎱 more work
  • ⭐️ Really ready for review this time

Points of attention:

  • Remove() is not included in this PR.
  • This PR includes some bug fixing related to libgit2/libgit2sharp#330 (the bugs were there before, they were just put into light by the aforementionned PR)
Contributor

yorah commented Feb 19, 2013

/cc @jamill

Contributor

yorah commented Feb 19, 2013

As a bonus, the build is now 💚

For Stage(), I had to include Unmodified records, to differentiate between:

  • a non-existent file
  • a removed file
  • an unaltered file (which acts as an unmatched pathspec when we don't include Unmodified files)
  • a file that was already added to the index (same thing)

I have the feeling I'm missing some other edge cases though, but can't figure out which ones...

@dahlbyk dahlbyk commented on an outdated diff Feb 19, 2013

LibGit2Sharp/Core/Proxy.cs
@@ -253,7 +253,7 @@ public static Signature git_commit_committer(GitObjectSafeHandle obj)
{
using (ThreadAffinity())
using (var treePtr = new ObjectSafeWrapper(tree.Id, repo))
- using (var parentObjectPtrs = new DisposableEnumerable<ObjectSafeWrapper>(parentIds.Select(id => new ObjectSafeWrapper(id, repo))))
+ using (var parentObjectPtrs = new DisposableEnumerable<ObjectSafeWrapper>(parentIds.Select(id => new ObjectSafeWrapper(id, repo)).ToArray()))
@dahlbyk

dahlbyk Feb 19, 2013

Member

Using ToList() here would avoid an extra array allocation for the final result.

Based on usage of parentObjectPtrs.Count() below, should DisposableEnumerable be converted to DisposableCollection to avoid an extra enumeration?

Contributor

yorah commented Feb 21, 2013

Latest changes:

  • implemented @dahlbyk proposal regarding DisposableEnumerable (actually, I just exposed a Count property to avoid the extra enumeration => I did not feel like implementing the whole ICollection<T> interface as we don't need it).
  • by default, pathspec validation is now strict. This means that unless you specify otherwise, an exception will be thrown if one of the passed path/pathspec doesn't match.
Member

dahlbyk commented Feb 21, 2013

actually, I just exposed a Count property to avoid the extra enumeration => I did not feel like implementing the whole ICollection interface as we don't need it

👍

Member

nulltoken commented Feb 21, 2013

@yorah I cherry picked the first five commits and pushed them into vNext. Could you please rebase this PR to drop them?

by default, pathspec validation is now strict. This means that unless you specify otherwise, an exception will be thrown if one of the passed path/pathspec doesn't match.

I'm in favor of this approach. And the code looks pretty clean.

However, as this is quite a breaking change, I'd like to wait for some days to see if the other contributors have anything to say about this.

@nulltoken nulltoken referenced this pull request Feb 22, 2013

Closed

Ignore Case via Repository.PathComparer #344

2 of 3 tasks complete
Contributor

yorah commented Feb 22, 2013

Rebased and running.

As a bonus, I will also add some test coverage related to accepting globs as pathspecs for staging/unstaging. In the next few days.

Contributor

yorah commented Feb 26, 2013

Added some tests related to accepting globs as pathspecs. Passing "*" is currently broken, waiting for feedback on libgit2/libgit2#1367.

Does anyone have an opinion related to the change of behaviour (strict vs lax)? Please? 🐱

Contributor

yorah commented Mar 11, 2013

Proposal as of now (current code)

Only throw/notify for explicit paths (renamed the PathspecsOptions class to ExplicitPathsOptions to match what it does).

Advantages I see:

  • simple and predictable behavior
  • no breaking changes with what was previously there (if you don't pass an ExplicitPathsOptions instance, no notify/throw is done, and you get standard pathspec matching)

@dahlbyk regarding the proposal you made in libgit2/libgit2#1367, I think checking if offending pathspecs match any of the already found files in libgit2sharp is not easily feasible. This would mean either exposing the _fnmatch functionality implemented in libgit2 (which doesn't feel right), or re-implementing our own fnmatch. We would also have to take the ignore_case flag into account.

RFC

Before going further, I would like to make a request for comments.

Basically, the initial question this PR tries to address boils down to:
Is it ok to stage/unstage (and remove/move) a file which doesn't exist?

I think there are two main cases:

  • the client passes a list of pathspecs
  • the client passes a list of explicit paths
  • the client passes a mix of both (unlikely corner case, excluded on purpose => this looks like a "slippery slope"™, as the result will depend on the way the list is being sorted)

So the question can also be reformulated as:
What should be the expected behavior when a path/pathspec is unmatched?
Silently ignore? Notify? Throw?

@Haacked @dahlbyk @jamill @ethomson

Member

dahlbyk commented Mar 14, 2013

@nulltoken would it be possible to cherry-pick 144ac99 and 5675ea0 so it's easier to read the core PR?

What should be the expected behavior when a path/pathspec is unmatched?

I'll defer to people that are actually building clients that need to Stage/Unstage.

Member

nulltoken commented Mar 14, 2013

@nulltoken would it be possible to cherry-pick 144ac99 and 5675ea0 so it's easier to read the core PR?

@dahlbyk Done!

Member

nulltoken commented Mar 14, 2013

@yorah Could you please rebase this PR on top of vNext?

Contributor

yorah commented Mar 18, 2013

Could you please rebase this PR on top of vNext?

Done!

@Haacked @jamill whenever you have time, I would really appreciate your feedback.

@nulltoken nulltoken and 1 other commented on an outdated diff Apr 7, 2013

LibGit2Sharp.Tests/StageFixture.cs
+ * D deleted_staged_file.txt
+ * D deleted_unstaged_file.txt
+ * M modified_staged_file.txt
+ * M modified_unstaged_file.txt
+ * M new.txt
+ * A new_tracked_file.txt
+ * ?? new_untracked_file.txt
+ *
+ * By passing "*" to Stage, the following files will be added/removed/updated from the index:
+ * - deleted_unstaged_file.txt : removed
+ * - modified_unstaged_file.txt : updated
+ * - new_untracked_file.txt : added
+ */
+ [Theory]
+ [InlineData("*u*", 0)]
+ //[InlineData("*", 0)] TODO: waiting for https://github.com/libgit2/libgit2/pull/1367
@nulltoken

nulltoken Apr 7, 2013

Member

What's the current status of this?

@yorah

yorah Apr 9, 2013

Contributor

Working with my latest proposal, uncommenting.

@nulltoken nulltoken and 1 other commented on an outdated diff Apr 7, 2013

LibGit2Sharp.Tests/StageFixture.cs
+ [InlineData("*modified_unstaged*", 0)]
+ [InlineData("new_*file.txt", 1)]
+ public void CanStageWithPathspec(string relativePath, int expectedIndexCountVariation)
+ {
+ TemporaryCloneOfTestRepo path = BuildTemporaryCloneOfTestRepo(StandardTestRepoWorkingDirPath);
+ using (var repo = new Repository(path.RepositoryPath))
+ {
+ int count = repo.Index.Count;
+
+ repo.Index.Stage(relativePath);
+
+ Assert.Equal(count + expectedIndexCountVariation, repo.Index.Count);
+ }
+ }
+
+ [Fact(Skip = "libgit2 doesn't notify of all matching pathspecs yet")]
@nulltoken

nulltoken Apr 7, 2013

Member

What's the current status of this?

@yorah

yorah Apr 9, 2013

Contributor

Working with my latest proposal, uncommenting.

yorah added some commits Mar 11, 2013

Add ExplicitPathsOptions to Repository.Diff.Compare()
All the overloads can now report and/or fail upon unmatched explicit paths.
By default, the passed list of paths will be treated as a list of pathspecs.

When an ExplicitPathsOptions is passed to the overloads, this list of paths
will be treated as explicit paths. In that case, the default behavior is to
throw when one of this explicit path is unmatched.
Member

nulltoken commented Apr 9, 2013

👍 Merged

@nulltoken nulltoken closed this Apr 9, 2013

Contributor

yorah commented Apr 9, 2013

Cool!

@dahlbyk I think this impacts your submodule PR (for the Stage() method, where we now rely on diff). Unless you beat me to it, I will have a look tomorrow to see if there is an easy way to make it work nicely with your PR!

@yorah yorah deleted the yorah:topic/unmatched-pathspecs branch Apr 9, 2013

@dahlbyk dahlbyk referenced this pull request Apr 10, 2013

Merged

Submodules! #312

10 of 13 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment