autosaved icon indicating copy to clipboard operation
autosaved copied to clipboard

Feature addition to restore: `autosaved restore --paths dir/file1 dir/file2 <commit-hash>`

Open nikochiko opened this issue 3 years ago • 14 comments

Only restore the given paths. Recently ran into this need when I made some change to a file that I later thought I wanted to revert. This feature may help in cases like these.

I am putting this on low priority. If some other people also think there is a need for this then comment or react with a thumbs up on this issue.

nikochiko avatar Jan 12 '22 14:01 nikochiko

We’d probably need different syntax for the --paths argument, since that way it wouldn’t be able to tell what is a path for it and what is another argument. There are a few ways to get around that:

  • we could have the paths comma separated (i.e. --paths dir/file1,dir/file2)
  • or we could use --path for each path, like curl does (i.e. --path dir/file1 --path dir/file2).

The first is more concise, but it wouldn’t support paths with commas in. The second one will appease the masochists, but it looks like cobra literally can’t support duplicate flags since it’s just built over the flag module which just uses a map to store flags. Obviously since maps can’t contain duplicate values it might have to be done the first way.

I can work on a PR if you want.

bolshoytoster avatar Jan 12 '22 23:01 bolshoytoster

Ah, yes. Nice catch! @bolshoytoster

What do you think about autosaved restore <commit-hash> -- dir/file1 dir/file2 in Git style? (https://stackoverflow.com/a/22750480)

This solves the path-with-commas issue, allows autocomplete with shell's default abilities, and is consistent with Git.

I can work on a PR if you want.

That would be amazing! :)

nikochiko avatar Jan 13 '22 04:01 nikochiko

@nikochiko it looks like cobra gets rid of -- from args, so we can't tell where it was.

Since restore currently doesn't take any arguments other than the commit hash, we could do it without, i.e. autosaved restore <commit-hash> dir/file1 dir/file2 or autosaved restore dir/file1 dir/file2 <commit-hash>.

bolshoytoster avatar Jan 13 '22 17:01 bolshoytoster

I played around a bit with Cobra and did some reading on this issue: https://github.com/spf13/cobra/issues/739

It turns out that cobra doesn't complete ignore --, but instead uses it to mark the "end of options" in the command. i.e. all following terms, even those starting with - or -- will be parsed as arguments. (autosaved help -- -wont-error vs autosaved help -will-error)

Since cobra gets rid of -- after parsing, I tested that behavior in Git, and it is no different. git diff HEAD^ -- dir/file1 is the same as git diff HEAD^ dir/file1 for normal filenames.

Explicitly recommending a syntax like autosaved restore <commit-hash> dir/file1 dir/file2 may not be a good idea as this won't be obvious for someone looking at it without documentation. But I feel we can allow this format as a side effect while recommending autosaved restore <commit-hash> -- dir/file1 dir/file2 in documentation and help.

What's your take?

nikochiko avatar Jan 13 '22 19:01 nikochiko

@nikochiko

But I feel we can allow this format as a side effect while recommending autosaved restore -- dir/file1 dir/file2 in documentation and help.

Yeah, that sounds like a good idea.

I've got the parsing working now I need to figure out how to get WorkTree.Checkout() to only restore certain files.

bolshoytoster avatar Jan 13 '22 19:01 bolshoytoster

Awesome! Let me know if you need any help. I have created this Telegram group for contributors and users to discuss autosaved: https://t.me/autosaved_community

If it sounds like a good idea, you can hop in there for slightly more real time communication than issues.

nikochiko avatar Jan 13 '22 19:01 nikochiko

I need to figure out how to get WorkTree.Checkout() to only restore certain files.

If you're still at this, maybe the two different types of checkouts (keep checkout and force checkout) may be of help here: https://pkg.go.dev/github.com/go-git/go-git/v5#CheckoutOptions

nikochiko avatar Jan 13 '22 19:01 nikochiko

maybe the two different types of checkouts (keep checkout and force checkout) may be of help here

I had a look but it didn't look like it would help; one way that could work would be to implement our own version of WorkTree.resetWorkTree but instead only running through the files/directories specified.

bolshoytoster avatar Jan 13 '22 21:01 bolshoytoster

That seems like a good idea :+1:

This kind of checkout is also supported in Git (https://git-scm.com/docs/git-checkout#Documentation/git-checkout.txt-emgitcheckoutem-f--ours--theirs-m--conflictltstylegtlttree-ishgt--pathspec-from-fileltfilegt--pathspec-file-null), but not in go-git from the docs that I have read so far. This may also be a good opportunity to contribute there if you like.

nikochiko avatar Jan 15 '22 05:01 nikochiko

There is basically no way to do this because go-git doesn't export most of it's methods.

I found that the go-git repository does have functionality for this, added in november. Unfortunately the latest release is from june. If you're ok with using a potentially unstable version of go-git we can use the SparseCheckoutDirectories variable in the CheckoutOptions struct which will make this possible.

bolshoytoster avatar Jan 16 '22 14:01 bolshoytoster

Can we copy-paste some of the functionality to this repo for our use case? In Go, the dependencies are bundled into the binary so it should be the same.

I had to do that for the diffing functionality: https://github.com/nikochiko/autosaved/blob/8f8e9ca319b18fa9622d1b6bfb11f18cd6b9aa37/core/diff.go

Why I'm skeptical of using an unstable version is because autosaved will do a lot of operations on important Git repositories. While the chances of a bug slipping in into the functions that we've used is slim, it may not be worth it to take that shot.

nikochiko avatar Jan 17 '22 13:01 nikochiko

@bolshoytoster you can take the call here on the unstable version.

I think it's time we start adding tests to make changes like this more reliable. I'll start working on that soon once I find some time.

nikochiko avatar Jan 19 '22 18:01 nikochiko

Adding tests is probably a good idea.

I tried just pasting the code in but I ran into a problem with unexported structs. We could see if the go-git devs want to make a new release, since it has been months since the last one.

bolshoytoster avatar Jan 19 '22 18:01 bolshoytoster

Ah, I see.

We could see if the go-git devs want to make a new release, since it has been months since the last one.

Yeah that would be a good idea.

nikochiko avatar Jan 20 '22 05:01 nikochiko