API should behave consistently for staging, unstaging, and removing files #274

Closed
jamill opened this Issue Dec 13, 2012 · 7 comments

Comments

Projects
None yet
5 participants
@jamill
Member

jamill commented Dec 13, 2012

Issue to track API consistency as discussed in #265.

Below is @nulltoken's original writeup / chart

Below a proposal for an exhaustive ruleset regarding this. Please review it carefully as I may have had made mistakes:

Staging deals with moving the state of a file from the Working Directory toward the Index

Stage File exists in Index File exists in WD Ignore unmatched pathspec Result
01 Y Y Y Version in Index is replaced with version in Workdir
02 Y Y Version in Index is created from version in Workdir
03 Y Y Version in Index is removed
04 Y Does nothing
05 Y Y Version in Index is replaced with version in Workdir
06 Y Version in Index is created from version in Workdir
07 Y Version in Index is removed
08 Throws

Unstaging deals with moving the state of a file from the Head toward the Index. If the Head is orphaned (ie. the current Head "contains" no commit), then the file will be considered as "non existing is the Head"

Unstage File exists in Index File exists in Head Ignore unmatched pathspec Result
01 Y Y Y Version in Index is replaced with version in Head
02 Y Y Version in Index is removed
03 Y Y Version in Index is replaced with version in Head
04 Y Does nothing
05 Y Y Version in Index is replaced with version in Head
06 Y Version in Index is removed
07 Y Version in Index is replaced with version in Head
08 Throws

Removing deals with clearing an entry from the Index and, optionally, from the Working Directory as well

Remove File exists in Index File exists in WD Ignore unmatched pathspec Remove from WD as well Result
01 Y Y Y Y Both entries are removed from Index and Workdir
02 Y Y Y Entry in Index is Removed
03 Y Y Y ???
04 Y Y Does nothing
05 Y Y Y Entry in Index is removed
06 Y Y Entry in Index is removed
07 Y Y ???
08 Y Does Nothing
09 Y Y Y Both entries are removed from Index and Workdir
10 Y Y ???
11 Y Y Throws
12 Y Throws
13 Y Y Entry in Index is removed
14 Y Entry in Index is removed
15 Y Throws
16 Throws

Note: I haven't dealt with Move which is slightly more complicated. I'd rather make sure the behavior Remove is settled first.

/cc @nulltoken, @dahlbyk, @yishaigalatzer

@nulltoken

This comment has been minimized.

Show comment
Hide comment
@nulltoken

nulltoken Dec 14, 2012

Member

@jamill I've started to work on simplifying the Stage() implementation. You can peek at it here.

It should now be possible to pass directories to both Stage() and Unstage() ;-)

Some tests do not pass and I'm starting to wonder if I should make them pass... Going further, I'm not even sure the Ignore unmatched pathspec is required.

How about changing the signature of Stage(), Unstage() and Remove() those methods to make them return an IEnumerable<string> containing the list of paths that have been successfully operated on?

For instance, given the following structure

  • 1/a -> Untracked
  • 1/b -> Staged

Invoking Stage("1") would return { "1/a" }
Invoking Stage("1/meh") would return { }

Thoughts?

Member

nulltoken commented Dec 14, 2012

@jamill I've started to work on simplifying the Stage() implementation. You can peek at it here.

It should now be possible to pass directories to both Stage() and Unstage() ;-)

Some tests do not pass and I'm starting to wonder if I should make them pass... Going further, I'm not even sure the Ignore unmatched pathspec is required.

How about changing the signature of Stage(), Unstage() and Remove() those methods to make them return an IEnumerable<string> containing the list of paths that have been successfully operated on?

For instance, given the following structure

  • 1/a -> Untracked
  • 1/b -> Staged

Invoking Stage("1") would return { "1/a" }
Invoking Stage("1/meh") would return { }

Thoughts?

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Dec 14, 2012

Contributor

Maybe this is unrelated to this specific discussion, but how does staging a change to a submodule fit into this? As far as I can tell, there's not an actual file in the parent repo that's "staged", but the change to the submodule is definitely changed. I haven't dug into it, but I assume it's some metadata being staged.

I definitely would want Stage to include such things. The question is, how should the status for staging a submodule change be reported?

Contributor

Haacked commented Dec 14, 2012

Maybe this is unrelated to this specific discussion, but how does staging a change to a submodule fit into this? As far as I can tell, there's not an actual file in the parent repo that's "staged", but the change to the submodule is definitely changed. I haven't dug into it, but I assume it's some metadata being staged.

I definitely would want Stage to include such things. The question is, how should the status for staging a submodule change be reported?

@nulltoken

This comment has been minimized.

Show comment
Hide comment
@nulltoken

nulltoken Dec 14, 2012

Member

@Haacked This thread indeed only relates to files "directly belonging" to the repository.

There's no submodule support in LibGit2Sharp yet (However, @dahlbyk may have started something in this area cf. #220).

Member

nulltoken commented Dec 14, 2012

@Haacked This thread indeed only relates to files "directly belonging" to the repository.

There's no submodule support in LibGit2Sharp yet (However, @dahlbyk may have started something in this area cf. #220).

@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Dec 16, 2012

Member

I've started on the bindings, but haven't started using them.

In the context of @nulltoken's new Stage() implementation, handling submodules could look something like this:

        public virtual void Stage(IEnumerable<string> paths)
        {
            Ensure.ArgumentNotNull(paths, "paths");

            var nonSubmodulePaths = StageAndExcludeSubmodules(paths);

            TreeChanges changes = repo.Diff.Compare(
                DiffOptions.IncludeUntracked | DiffOptions.IncludeIgnored,
                nonSubmodulePaths);

            ...
        }

        private IEnumerable<string> StageAndExcludeSubmodules(IEnumerable<string> paths)
        {
            foreach (var path in paths)
            {
                var submodule = repo.Submodules[path]; // git_submodule_lookup
                if (submodule != null)
                {
                    submodule.Stage(); // git_submodule_add_to_index
                    continue;
                }

                yield return path;
            }
        }

As far as I can tell, libgit2 does not provide an unstage for submodule changes - @arrbee?

Member

dahlbyk commented Dec 16, 2012

I've started on the bindings, but haven't started using them.

In the context of @nulltoken's new Stage() implementation, handling submodules could look something like this:

        public virtual void Stage(IEnumerable<string> paths)
        {
            Ensure.ArgumentNotNull(paths, "paths");

            var nonSubmodulePaths = StageAndExcludeSubmodules(paths);

            TreeChanges changes = repo.Diff.Compare(
                DiffOptions.IncludeUntracked | DiffOptions.IncludeIgnored,
                nonSubmodulePaths);

            ...
        }

        private IEnumerable<string> StageAndExcludeSubmodules(IEnumerable<string> paths)
        {
            foreach (var path in paths)
            {
                var submodule = repo.Submodules[path]; // git_submodule_lookup
                if (submodule != null)
                {
                    submodule.Stage(); // git_submodule_add_to_index
                    continue;
                }

                yield return path;
            }
        }

As far as I can tell, libgit2 does not provide an unstage for submodule changes - @arrbee?

@jamill

This comment has been minimized.

Show comment
Hide comment
@jamill

jamill Dec 17, 2012

Member

@nulltoken I like the diff based comparison.

How about changing the signature of Stage(), Unstage() and Remove() those methods to make them return an IEnumerable containing the list of paths that have been successfully operated on?

I am not sure how I would use this information as a consumer of this API. Is it mainly for reporting purposes? If I want to check for an error condition, I would have to iterate through and mark off all of the expected paths - if that is the use case, should it indicate the paths that it failed to operate on?

Member

jamill commented Dec 17, 2012

@nulltoken I like the diff based comparison.

How about changing the signature of Stage(), Unstage() and Remove() those methods to make them return an IEnumerable containing the list of paths that have been successfully operated on?

I am not sure how I would use this information as a consumer of this API. Is it mainly for reporting purposes? If I want to check for an error condition, I would have to iterate through and mark off all of the expected paths - if that is the use case, should it indicate the paths that it failed to operate on?

@yorah

This comment has been minimized.

Show comment
Hide comment
@yorah

yorah Jan 16, 2013

Contributor

@jamill I believe that libgit2/libgit2#1249 should help you returning the paths that stage/unstage failed to operate on.

Contributor

yorah commented Jan 16, 2013

@jamill I believe that libgit2/libgit2#1249 should help you returning the paths that stage/unstage failed to operate on.

@yorah yorah referenced this issue Feb 19, 2013

Closed

Add unmatched pathspecs options #343

4 of 4 tasks complete
@nulltoken

This comment has been minimized.

Show comment
Hide comment
@nulltoken

nulltoken May 4, 2013

Member

Index.Remove(), Index.Unstage(), Index.Stage(), Diff.Compare() and Repository.Reset() now accept an ExplicitPathsOptions to control handling of unmatched pathspecs.

Credits go to @yorah for dealing with this!

Note: The only remaining part of the Index API which is yet to be worked on is Index.Move(). As it should be a slighter more complicated task, it deserves an issue of its own.

Member

nulltoken commented May 4, 2013

Index.Remove(), Index.Unstage(), Index.Stage(), Diff.Compare() and Repository.Reset() now accept an ExplicitPathsOptions to control handling of unmatched pathspecs.

Credits go to @yorah for dealing with this!

Note: The only remaining part of the Index API which is yet to be worked on is Index.Move(). As it should be a slighter more complicated task, it deserves an issue of its own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment