FYI: I wrote the git lfs prune command originally so this is my bad, hence why I'm fixing it 😃

Hey, thanks for both the bug report and the proposed patch! I was intending to look at the bug report today, and am delighted to have a potential solution in hand before I even do so! 😄

On a first glance this seems reasonable, but to get the CI jobs to pass I think we'll need to fix the trailing whitespace issue in t-prune.sh (I believe there's a missing final newline?)

Thanks again for the patch; that's always appreciated!

Ah yes, sorry I ran the tests individually locally rather than the full CI build, I forgot to check the trailing new line. Due to time zone I won’t be able to fix this until tomorrow but feel free to patch it up yourself if you want / have time. :)

Might need some help with this, I've tried to please the cibuild god but it keeps complaining about whitespace; I've added a couple of newlines but no luck. Its been a few years since I've had to bash my head against it so maybe there's a trick I've forgotten :)

What was pointed out to me today is that the stash uses multiple commits to store the state of the working tree and the index separately (thanks, @bk2204!), and our various experiments with git reflog show and git log -p both only give the SHAs of the working tree stash commits. That got me thinking that if someone used git stash pop --index and then tried to restore the working tree to the index, it might still cause a problem, even with this PR's changes. And indeed that seems to be the case.

I wrote up a test which demonstrates the outstanding issue here, I believe; at least for me, using this PR as it stands, it failed on the second assert_local_object "$oid_indexstashed" "${#content_indexstashed}" (and in a simpler manual test, without the assert_local_object test helpers, I get a failure on git checkout . when the smudge filter can't download the missing LFS data):

begin_test "prune keep stashed changes in index"
(
  set -e

  reponame="prune_keep_stashed_index"
  setup_remote_repo "remote_$reponame"

  clone_repo "remote_$reponame" "clone_$reponame"

  git lfs track "*.dat" 2>&1 | tee track.log
  grep "Tracking \"\*.dat\"" track.log

  # generate content we'll use
  content_inrepo="This is the original committed data"
  oid_inrepo=$(calc_oid "$content_inrepo")
  content_indexstashed="This data will be stashed from the index and should not be deleted"
  oid_indexstashed=$(calc_oid "$content_indexstashed")
  content_stashed="This data will be stashed and should not be deleted"
  oid_stashed=$(calc_oid "$content_stashed")

  # We just need one commit of base data, makes it easier to test stash
  echo "[
  {
    \"CommitDate\":\"$(get_date -1d)\",
    \"Files\":[
      {\"Filename\":\"stashedfile.dat\",\"Size\":${#content_inrepo}, \"Data\":\"$content_inrepo\"}]
  }
  ]" | lfstest-testutils addcommits

  # now modify the file, and add it to the index
  echo -n "$content_indexstashed" > stashedfile.dat
  git add stashedfile.dat

  # now modify the file again, and stash it
  echo -n "$content_stashed" > stashedfile.dat

  git stash

  # Prove that the stashed data was stored in LFS (should call clean filter)
  assert_local_object "$oid_stashed" "${#content_stashed}"
  assert_local_object "$oid_indexstashed" "${#content_indexstashed}"

  # Prune data, should NOT delete stashed file or stashed changes to index
  git lfs prune

  assert_local_object "$oid_stashed" "${#content_stashed}"
  assert_local_object "$oid_indexstashed" "${#content_indexstashed}"

  # Restore working tree from stash
  git stash pop --index

  # Reset working tree to index from stash
  git checkout .
)
end_test

So I think we should add a test like this, first of all, and also consider how best to walk back through all the SHAs associated with the stash.

Nice catch, I never realised stash could create >1 commit but thinking about it I guess it makes sense. I guess it can only ever create 2. I'll have another crack at it.

Here's one failed attempt that I thought would work:

func scanStashed(cb GitScannerFoundPointer, s *GitScanner) error {
	// First get the SHAs of all stashes
	logArgs := []string{"-g", "--format=%h", "refs/stash", "--"}

	cmd, err := git.Log(logArgs...)
	if err != nil {
		return err
	}

	cmd.Start()
	defer cmd.Wait()

	scanner := bufio.NewScanner(cmd.Stdout)

	var stashShas []string
	for scanner.Scan() {
		stashShas = append(stashShas, strings.TrimSpace(scanner.Text()))
	}

	// Stashes are 2 commits, one for index and one for working copy
	// Preserve changes in both separately, in case same file modified twice
	logArgs = stashShas // include all stashes
	logArgs = append(logArgs, "--not", "--branches", "--tags") // exclude those commits reachable locally otherwise
	// Add standard search args to find lfs references
	logArgs = append(logArgs, logLfsSearchArgs...)

	cmd, err = git.Log(logArgs...)
	if err != nil {
		return err
	}

	parseScannerLogOutput(cb, LogDiffAdditions, cmd)
	return nil
}

While this does list the correct commits, the diff generated by the -p option only includes the OID for the combined change for the entire stash, i.e. the working copy version and not the one that was in the index. I can't seem to get the diff to output both and I'm not sure why, it's not how it behaves for other commits.

Looking at the result of git show -p <stash_sha> it seems to be that it knows the changes are different to a normal log entry:

- oid sha256:67ae8ef97d615140fcb42b3fa7e300a5ce5960cd8ff9807eef3dcb85aec3cecd
- size 25
 -oid sha256:32609ffbe33e263e807f7e3b8ee6ae7d990da65de5c15a51f1b62782a8cc7276
 -size 43
++oid sha256:c9d22606d40fec4e99cc7d09fbef109f5301a30e35c177e0d14bafb36894cdc5
++size 64

This looks more like a git status --porcelain style output where positioning of the -/+ tells you whether it's the index or working copy version. In the example above OID 67ae8ef9 (- in col 0) was the previous commit, OID 32609ffbe (- in col 1) was the stashed index, and OID c9d22606d (++) was the working copy version. git log only shows the first and last OIDs even if you're definitely getting the commit inbetween in the output, the diff for it just isn't there.

FYI for stash commits that only had the working copy, git show -p <stash_sha> displays:

--oid sha256:67ae8ef97d615140fcb42b3fa7e300a5ce5960cd8ff9807eef3dcb85aec3cecd
--size 25
++oid sha256:c7fe2c1adc4c9fbe8df7d1bd56b85d98e52288ffc6547fcb7a15fab39a4942ed
++size 15

So I think the best approach is going to be to use git show -p <stash_sha>... and parse out any OIDs which have either "+" or "-" in column 1 of the diff output. That should pick up the index and working copy state of stashes. Thoughts? I'd love to figure out a way to get git log to output that full detail like git show but so far I've had no luck.

Sorry I pasted the wrong "failed attempt" function in the previous comment at first, fixed now.

One other thing we should consider (again, based on the tips I got about how git stash push operates) is that git stash push -u makes yet a third commit, to capture any untracked files.

Doing something like:

echo 123 > foo.dat; git add foo.dat
git commit -m initial
echo 456 > foo.dat
git add foo.dat
echo 789 > foo.dat
echo abc > bar.dat
git stash push -u
git log -g refs/stash

shows something like the following, where the three SHAs in the Merge: line represent the tip of HEAD at the time of the stash (the first parent), the stashed state of the index, and the stashed untracked files from the working tree:

Reflog message: WIP on master: 2678f27 initial
Merge: 2678f27 2762e90 a71ca8c

and the merge commit itself has the stashed changes to the tracked files from the working tree.

OK that suggests that we need both commands. To summarise:

1. git log -g --format=%h refs/stash -- to get the 'leaf' stash SHAs
2. git log -p <stashSHA>... --not --branches --tags (+logSearchArgs) to pick up changes from untracked files and the final diff (but it will miss the diff between working copy and index)
3. git show -p <stashSHA>... to pick up the diff between working copy & index on same file, if it exists

The set of OIDs from all of those should be the complete set.

Alternatively if we used the "Merge" line:

Reflog message: WIP on master: a70e14d First
Merge: a70e14d 4cf30f4 1bad043

In my case a70e14d is the parent commit of the stash (which should be ignored), 4cf30f4 is the index contents and 1bad043 is the untracked files. The leaf SHA is then the working copy. If that's a consistent we could ignore the first SHA on the merge line and use the rest plus the leaf as arguments to git show. It's a little more parsing though (becomes 2 stages); in the previous suggestion you only need the leaf SHAs and everything else is discoverable via OID lines (albeit with a slightly different regex to handle index/working copy).

Yes, exactly -- I realized I needed to edit my description of which commits were which in #4209 (comment) and then noticed you'd already explained the relevant commits more correctly. :-)

Bad news, I'm not sure exactly what version of git started reporting merge parents for stashes, but it was somewhere later than git 2.7 because during my testing on an older linux setup with older git, I couldn't get git log -g refs/stash to report the merge line, or the parent data when using --format=%p. It's just blank. So I don't think we want to rely on that behaviour.

For reference in this git 2.7 the parentage of the commits are still the same, it's just that git log -g --format=%p doesn't report them (and neither does git reflog). So at some point the reflog must have been enhanced to support this.

git show --format=%p does show the merge parents of a stash leaf commit, however. So that's probably the way to go.

OK this took more a lot more work than I expected, but it's finally working. I had to add a "Show" command and I had to also change the OID pointer diff parsing because it couldn't handle merge commits. I added a test which includes all 3 kinds of stash change at once because there was a bug that only happened when you had all 3 together.

Hoping this is all OK now!

OK, wow, that's a lot of work indeed, thank you! I plan to take a look tomorrow but from the tests it looks like this covers a lot of territory and all the use cases. ❤️

Thanks so much for sticking with this! I just wanted to say I'm still mid-review and have not forgotten this, and will get you more feedback as soon as I can. Thanks so much again!

Looks good to me! 👍