Stash from StashCollection.Add is unsafe.

[Therzok]

Let's say we create two stashes.

First stash operation:
Stash1 will be stash@{0}.

Second stash operation:
Stash1 will be stash@{1}
Stash2 will be stash@{0}

The managed reference to Stash1 will still be stash@{0}. We need a way to reflect these index changes, as unsafe in practice.

[Therzok]

To see this in action, look at:
https://github.com/libgit2/libgit2sharp/blob/vNext/LibGit2Sharp.Tests/StashFixture.cs#L254

Replace repo.Stashes[1] with secondStash or repo.Stashes[2] with firstStash.

[nulltoken]

Hmmm. I get your point. However, those objects are immutables. I'd rather not see those CanonicalNames magically change. FWIW, we could see a similar behavior if we spawned an instance of Reference and then, behind the scene, change its target.

However, I agree it's unfortunate that stash rely on indices and that your use case may indeed be confusing for the end user.

I haven't got any proposal to make this better (besides maybe improving the xml documentation) for the moment.

@jamill @ethomson @dahlbyk @carlosmn Thoughts?

[dahlbyk]

This is just one of many examples where immutable instances can get quickly out of date (e.g. rename a branch and the old Branch instance is no longer correct). I'm also not sure there's much we can do other than provide hints in the documentation ("Stash instances may be invalidated by changes to their parent StashCollection.").

[Therzok]

@nulltoken I've thought this over and I propose two solutions:

• Moving more of the logic into StashCollection. Stash shouldn't exist (I know it's pretty rough), and we'd provide some functions which would grab data about the Stash (commits, message) from StashCollection. We'd obviously be against some .NET standards.
• Add an internal Stash function which would update its index on every operation. Search for the first stash that has its Commit SHAs and name?

[dahlbyk]

I think it would be a mistake to move away from a Stash as a representation of a particular stash at a moment in time. If anything, we might want to updated Stash to proactively retrieve its underlying TargetObject (it's Lazy<>) to make sure we're truly capturing its current state rather than its state when the consumer gets around to checking what's behind the curtain (at which point a stash may have been added/removed changing or invalidating the target resolved). We may want to move away from using ReferenceWrapper<> altogether, in fact.

From an API standpoint, consider how git stash drop "returns" details about the now-defunct stash:

Dropped refs/stash@{0} (49c963a9d568b455be73a45b4f15fbccf01941da)

It seems reasonable that we would want to be able to support the same.

---

I do like @nulltoken's idea that we could have overloads that accept a Stash instance rather than the stash@{n} reference. I think the correct behavior in these cases would be to apply/drop the stash, if one exists, identified by the WorkTree (49c963 above). Even if the stash collection shifts over time, Commits Are Forever™.

In fact, git.git will let me git stash show 49c963a and git stash apply 49c963a even after I've dropped the stash. If we supported StashCollection.Apply(Commit), a StashCollection.Apply(Stash) extension would be as simple as calling Apply(stash.WorkTree).

[nulltoken]

If anything, we might want to updated Stash to proactively retrieve its underlying TargetObject (it's Lazy<>) to make sure we're truly capturing its current state

@dahlbyk I've looked into this. Actually, the TargetObject is indeed lazy loaded. However, the reference (and its target ObjectId) is eagerly embedded. So I think we're safe from this perspective.

About making Remove() accept a Stash instance, this could be done. However, we would have to enumerate all the stashes to find out which one is the correct one to drop. This looks a bit clunky to me (and racy as well as libgit2 Api expects an index).

It could be easily done, but I think this would make me cry a little inside.

Thoughts?

[ethomson]

(Note, removing the stabilization label - I think that this is not a 1.0 issue, we can add the Remove overload later.)

[dahlbyk]

About making Remove() accept a Stash instance, this could be done. However, we would have to enumerate all the stashes to find out which one is the correct one to drop. This looks a bit clunky to me (and racy as well as libgit2 Api expects an index).

It could be easily done, but I think this would make me cry a little inside.

Would it be silly for libgit2 to support dropping a stash by ObjectId in addition to index, and we'd just use that? If anything that seems the safer (if not git.git-aligned) API in the first place - no race to worry about other than a lock on refs/stash.

[nulltoken]

Would it be silly for libgit2 to support dropping a stash by ObjectId

I'd like this very much! Less racy.

@carlosmn @ethomson How much would you hate this approach?

[carlosmn]

I'm not too keen on this idea, as we'd still be racing against an external process or ourselves. If a user says, "drop the top stash" and we drop the second-to-last because something else acted on the stash, it's still going to be a surprising result. Stash is inherently racy, as is the update of any other reference. But what we can do is add a version of drop/apply/whatever which takes in the index and the expected entry and can fail with EMODIFIED if the entry does not correspond to the one the caller expects, which is what we do for references.

[dahlbyk]

If a user says, "drop the top stash" and we drop the second-to-last because something else acted on the stash, it's still going to be a surprising result.

It's only surprising if the UX is framed as "second-to-last" rather than "the one I'm viewing that was recently second-to-last". For example, in Git Extensions:

1. Open stash window
2. Pick a stash to preview
3. Create a new stash
4. Click delete in the already-open stash window
5. Note the stash you were looking at is still there, and another was deleted

This UX is only "reasonable" if you know to forgive the racy implementation. A less knowledgeable user is just confused that the stash they were looking at wasn't deleted. As it stands, I'd think any user interface on top of Git Stash should warn/cancel if the user is about to act on a stash index that has changed since the UI last updated.

What if we went the other direction: add a version of drop/apply/whatever that accept a commit ID and optional index. If index is specified, failure to match commit+index fails with EMODIFIED; if index is GIT_STASH_IGNORE_INDEX or whatever, do the requested action based on the commit and ignore refs/stash mismatches (e.g. dropping a stash entry that wasn't in the list in the first place could be a no-op since the end goal - the specified stash not in the list - is already met).

This would be particularly handy for implementing auto-stash type behavior. Unless I'm missing it, we don't have an analog to git stash create (create stash commit but don't touch refs/stash) nor do we support git stash apply <stash>:

Unlike pop, <stash> may be any commit that looks like a commit created by stash save or stash create.