I think that it might make sense to bring in a few of the flags from import, namely: --verbose and --object-map. I think that we can leave --no-rewrite off for now, but it might make sense to bring that in later.

Nit; let's put exportCmd before the other two arguments, to keep this list alphabetized.

Could you clarify this line? I think the last sentence in this block makes the intended purpose of the function clear, but the term .gitattributes I am unsure of in this context alone.

Maybe something like:

"... where each entry is a line we intend to place in the repository's .gitattributes file."

I think this and the ObjectMapFilePath are good to have, but presently don't have any effect, since the flags are not added to the export command.

I think we may run into a problem where the size of the contents that we're smudging are larger than what we would like to hold in memory. Since buf is local to the stack frame, we'll have to worry about that here.

Since we already do a pass over history below with ScanForPointers, I think we can avoid smudging here, since the large object that we're looking for is present in the .git/lfs/objects cache.

So, we can use the MediaPath function on gitfilter to find the location of this file on disk. We'll have to open that file and then pass the *os.File (which implements io.Reader) to the *odb.Blob constructor's Contents field.

We'll need to close the file later, but something like this could work:

diff --git a/git/odb/blob.go b/git/odb/blob.go
index 7f9d66ae..e9cc52f8 100644
--- a/git/odb/blob.go
+++ b/git/odb/blob.go
@@ -3,6 +3,9 @@ package odb
 import (
 	"bytes"
 	"io"
+	"os"
+
+	"github.com/git-lfs/git-lfs/errors"
 )
 
 // Blob represents a Git object of type "blob".
@@ -19,6 +22,42 @@ type Blob struct {
 	closeFn func() error
 }
 
+// NewBlobFromFile returns a new *Blob that contains the contents of the file
+// at location "path" on disk. NewBlobFromFile does not read the file ahead of
+// time, and instead defers this task until encoding the blob to the object
+// database.
+//
+// If the file cannot be opened or stat(1)-ed, an error will be returned.
+//
+// When the blob receives a function call Close(), the file will also be closed,
+// and any error encountered in doing so will be returned from Close().
+func NewBlobFromFile(path string) (*Blob, error) {
+	f, err := os.Open(path)
+	if err != nil {
+		return nil, errors.Wrapf(err, "git/odb: could not open: %s",
+			path)
+	}
+
+	stat, err := f.Stat()
+	if err != nil {
+		return nil, errors.Wrapf(err, "git/odb: could not stat %s",
+			path)
+	}
+
+	return &Blob{
+		Contents: f,
+		Size:     stat.Size(),
+
+		closeFn: func() error {
+			if err := f.Close(); err != nil {
+				return errors.Wrapf(err,
+					"git/odb: could not close %s", path)
+			}
+			return nil
+		},
+	}, nil
+}
+
 // NewBlobFromBytes returns a new *Blob that yields the data given.
 func NewBlobFromBytes(contents []byte) *Blob {
 	return &Blob{

Nice. What do you think about allowing [--remote=<remote>] as an optional argument in this command, to specify where clients would like to get their objects from?

We could even take it a step further and allow them to specify multiple remotes. Package tq today doesn't know how to interact with multiple remotes at once (and I'm inclined not to teach it how to do so), but we could work around this by looping through each given remote. Maybe something like:

var wg sync.WaitGroup

for _, remote := range remotes {
        wg.Add(1)
        go downloadObjects(remotes, wg)
}

wg.Wait()

I think that's a good idea! We can just default to cfg.Remote() like we do here if the argument isn't provided.

Do we want to support multiple remotes for this? What would the use cases for that look like? I'm concerned about potential conflicts in object downloads, for example if one provided remote points to a fork of another repo, which is also provided as a remote.

I think that supporting just the one remote is fine for now; I can see this becoming somewhat complicated because the transfer queue really isn't designed for multi-remote operations.

I think that there are a few approaches we can take in the longer term, but for now, I think that supporting one remote via --remote is totally good. If a user is (1) the bleeding-edge version of Git LFS, and (2) wants to do a multi-remote export, they can always use git lfs fetch && git lfs migrate export instead.

Longer-term, I think we could do one of the following:

1. Teach package tq how to handle multiple remotes. This would provide a consistent interface for using the transfer queue, but will be only used at this singular call-site, and seems too complicated for too little benefit.

2. Keep many transfer queues alive, and round-robin the objects through them. This has the benefit of simplicity and no requirements for change inside of package tq, and the external call-sites should be fairly straightforward, too. If an earlier transfer queue has the same object as a later one, it doesn't really matter which one answers first. This has the drawback of having to wait for one transfer queue to cycle before the next one in line can determine which objects they need to get.

I think that we could theoretically get more efficient than that, but I bet that that's an OK starting point for now.

Nit; I think that we can re-wrap this line here and in git lfs migrate import (though perhaps the later should go in a preparatory commit).

Nice. I think that we are supposed to spell the -filter as !filter, though.

According to the gitattributes documentation, it looks like - sets the value of an attribute to false, whereas ! sets the value of an attribute to unspecified. Since we're doing an export here, it seems like it might be better to use -

That's what it looks like to me, too:

https://github.com/git/git/blob/53f9a3e157dbbc901a02ac2c73346d375e24978c/attr.c#L334-L335

But, I think that ATTR__UNSET is more appropriate; according to the merge documentation from that same page:

Unset

Take the version from the current branch as the tentative merge result, and
declare that the merge has conflicts. This is suitable for binary files that
do not have a well-defined merge semantics.

Unspecified

By default, this uses the same built-in 3-way merge driver as is the case when
the merge attribute is set. However, the merge.default configuration
variable can name different merge driver to be used with paths for which the
merge attribute is unspecified.

I think that given the choice between the two, we'd prefer unspecified, which defaults back to the configuration.

Ah, that makes sense! In that case I'll go ahead and switch this over

Nice. Historically, we've used package lfs's GitScanner type to do such a thing. It's backed by the now-outdated git/git.go, which shells out to git(1), but it does roughly what you're doing here. I'd like someday to replace it with a git/odb-backed implementation, but for now that doesn't exist. Instead of growing two separate implementations, how about we lean on what's already provided and replace it over time in a later PR?

diff --git a/commands/command_migrate_export.go b/commands/command_migrate_export.go
index b234eac9..53e64f49 100644
--- a/commands/command_migrate_export.go
+++ b/commands/command_migrate_export.go
@@ -104,10 +104,15 @@ func migrateExportCommand(cmd *cobra.Command, args []string) {
 	// If we have a valid remote, pre-download all objects using the Transfer Queue
 	if remoteURL := getAPIClient().Endpoints.RemoteEndpoint("download", cfg.Remote()).Url; remoteURL != "" {
 		q := newDownloadQueue(getTransferManifestOperationRemote("Download", cfg.Remote()), cfg.Remote())
-		if err := rewriter.ScanForPointers(q, opts, gitfilter); err != nil {
+		scan := lfs.NewGitScanner(func(p *lfs.WrappedPointer, err error) {
+			if err != nil {
+				return
+			}
+			q.Add(p.Name, p.Name, p.Oid, p.Size)
+		})
+		if err := scan.ScanRefComplex(opts.Include, opts.Exclude, nil); err != nil {
 			ExitWithError(err)
 		}
-
 		q.Wait()
 
 		for _, err := range q.Errors() {
diff --git a/lfs/gitscanner.go b/lfs/gitscanner.go
index 15a6d58c..20d6e49d 100644
--- a/lfs/gitscanner.go
+++ b/lfs/gitscanner.go
@@ -92,6 +92,17 @@ func (s *GitScanner) ScanLeftToRemote(left string, cb GitScannerFoundPointer) er
 	return scanRefsToChan(s, callback, left, "", s.opts(ScanLeftToRemoteMode))
 }
 
+func (s *GitScanner) ScanRefComplex(include, exclude []string, cb GitScannerFoundPointer) error {
+	callback, err := firstGitScannerCallback(cb, s.FoundPointer)
+	if err != nil {
+		return err
+	}
+
+	opts := s.opts(ScanRefsMode)
+	opts.SkipDeletedBlobs = false
+	return scanRefsToChan(s, callback, include, exclude, opts)
+}
+
 // ScanRefRange scans through all commits from the given left and right refs,
 // including git objects that have been modified or deleted.
 func (s *GitScanner) ScanRefRange(left, right string, cb GitScannerFoundPointer) error {
@@ -102,7 +113,7 @@ func (s *GitScanner) ScanRefRange(left, right string, cb GitScannerFoundPointer)
 
 	opts := s.opts(ScanRefsMode)
 	opts.SkipDeletedBlobs = false
-	return scanRefsToChan(s, callback, left, right, opts)
+	return scanLeftRightToChan(s, callback, left, right, opts)
 }
 
 // ScanRefWithDeleted scans through all objects in the given ref, including
diff --git a/lfs/gitscanner_refs.go b/lfs/gitscanner_refs.go
index e66daee2..024ff117 100644
--- a/lfs/gitscanner_refs.go
+++ b/lfs/gitscanner_refs.go
@@ -33,10 +33,7 @@ func (s *lockableNameSet) Check(blobSha string) (string, bool) {
 
 func noopFoundLockable(name string) {}
 
-// scanRefsToChan takes a ref and returns a channel of WrappedPointer objects
-// for all Git LFS pointers it finds for that ref.
-// Reports unique oids once only, not multiple times if >1 file uses the same content
-func scanRefsToChan(scanner *GitScanner, pointerCb GitScannerFoundPointer, refLeft, refRight string, opt *ScanRefsOptions) error {
+func scanRefsToChan(scanner *GitScanner, pointerCb GitScannerFoundPointer, include, exclude []string, opt *ScanRefsOptions) error {
 	if opt == nil {
 		panic("no scan ref options")
 	}
@@ -134,3 +131,10 @@ func revListShas(include, exclude []string, opt *ScanRefsOptions) (*StringChanne
 
 	return NewStringChannelWrapper(revs, errs), nil
 }
+
+// scanLeftRightToChan takes a ref and returns a channel of WrappedPointer objects
+// for all Git LFS pointers it finds for that ref.
+// Reports unique oids once only, not multiple times if >1 file uses the same content
+func scanLeftRightToChan(scanner *GitScanner, pointerCb GitScannerFoundPointer, refLeft, refRight string, opt *ScanRefsOptions) error {
+	return scanRefsToChan(scanner, pointerCb, []string{refLeft}, []string{refRight}, opt)
+}

Thanks! I agree with you, we should be using the GitScanner for this code - I'll go ahead and replace what I've done with the ScanForPointers function

Nit; let's push this down into the below conditional, since an error will never be nil and pass errors.IsNotAPointerError(...).

I think that supporting just the one remote is fine for now; I can see this becoming somewhat complicated because the transfer queue really isn't designed for multi-remote operations.

I think that there are a few approaches we can take in the longer term, but for now, I think that supporting one remote via --remote is totally good. If a user is (1) the bleeding-edge version of Git LFS, and (2) wants to do a multi-remote export, they can always use git lfs fetch && git lfs migrate export instead.

Longer-term, I think we could do one of the following:

1. Teach package tq how to handle multiple remotes. This would provide a consistent interface for using the transfer queue, but will be only used at this singular call-site, and seems too complicated for too little benefit.

2. Keep many transfer queues alive, and round-robin the objects through them. This has the benefit of simplicity and no requirements for change inside of package tq, and the external call-sites should be fairly straightforward, too. If an earlier transfer queue has the same object as a later one, it doesn't really matter which one answers first. This has the drawback of having to wait for one transfer queue to cycle before the next one in line can determine which objects they need to get.

I think that we could theoretically get more efficient than that, but I bet that that's an OK starting point for now.

Nice. I wonder if we could add this to the Add() function on the transfer queue itself. I can imagine that other callers would find this useful.

I just tried this, but unfortunately it led to a bunch of inexplicable failures about objects not being found by the transfer queue when running the export tests. I verified that the conditional I added to the function is indeed correct so I'm not sure what's going wrong - it's definitely trying to download the files since they don't exist on disk. This problem bled over into other tests that leverage the transfer queue as well, such as fetch.

I can try to do some additional debugging but I'm inclined to leave this as-is for now. This is here to prevent download of an object that exists on disk but isn't present on the remote (say, if a recently-migrated pointer hasn't been pushed up to the remote repository yet). The transfer queue is leveraged by lots of code in Git LFS, and it seems like re-downloading an object file won't necessarily be wrong in all cases

I can try to do some additional debugging but I'm inclined to leave this as-is for now.

That's fine, let's mark this as #leftoverbits, and touch this in a future pull request.

Will exported ever contain an object evicted by the migration, but used elsewhere on disk? An example demonstrating this is a repository with two files, both tracked in LFS, sharing the same contents at different paths. If only one of those paths is migrated out, the other will still wish to retain the object.

Perhaps we could invoke the git lfs prune command here, instead?

✨ Nice.

Hm, should this be:

return scanRefsToChan(..., []string{refLeft}, []string{refRight}, nil, opt)

instead? I think that they should be split, because the right is a boundary of everything we don't want in the range.

I think this is correct as-is. My gut reaction was the same, but

[]string{refLeft, refRight}, nil

was being used internally by scanRefsToChan() before I changed its functionality here. Additionally, changing it to

[]string{refLeft}, []string{refRight},

causes tests that leverage this function to fail. I'm still doing some investigation as to why and will come back with an update when I better understand the rev scanner.

Ah, you're totally right. I just checked the code and the way that it is in your diff is 👍, my comment is incorrect.

I left a comment above but this was required to ensure certain objects were properly pruned. prune preserves any cached objects that exist on the remote from commits within a time window of PruneOffsetDays + FetchRecentRefsDays. Since migrate export rewrites commits but does not push them, exported objects can still exist on the remote, and can be retained in the cache if this time window is not set to 0.

A negative side effect of this is that all objects that exist on the remote will be pruned, regardless of whether or not the associated object was exported. It seems like this should be okay though, as if necessary Git LFS can just re-download the pruned files.

With respect to my comment below, what happens if we remove these assignments iff we perform the prune after we do the checkout?

These values only affect whether to preserve pointers referenced in the remote git history, so they're still required if the prune is moved after the checkout

It looks like in general though we don't actually need to set PruneOffsetDays, just FetchRecentRefsDays, so I'll remove it

Can this be removed? I see that we append() into it, but I don't think the contents are used anywhere.

Will this create an empty .gitattributes blob if no patterns are given?

It might if there was also no pre-existing .gitattributes file, but right now we require a pattern being given to migrate export through the --include argument, so it shouldn't be an issue.

Thanks for the clarification, that sounds good to me.

Hm; do we need to do the checkout first to make sure that we don't have reference-able LFS pointers in the current checkout?

It doesn't seem to make a difference in regards to what gets pruned (all tests still pass if it's moved below the checkout), but I agree that after the checkout is the correct place for it! I'll go ahead and move it

Could you add a clarifying sentence in the middle? Perhaps something like:

In other words, --include-ed files will be removed from Git LFS, while --exclude-ed files will retain their Git LFS status.

Are these subshells needed? If they are, perhaps we could wrap this in a new function in test/testhelpers.sh called refute_pointer?

I like this error message, but I'm wondering what it is users would expect iff they didn't provide --include or --exclude. My intuition tells me that a user invoking git lfs migrate export --everything would want everything to be removed from Git LFS, so perhaps a default of --include=* (under the semantics that are implemented below) would make sense.

What do you think?

Hmm, I don't think we should use --everything for this, as it's already being used to specify that all local references should be migrated.

Maybe we should add a separate flag, --export-all?

I agree, though I wasn't suggesting that --everything be used for that purpose. Rather, that git lfs migrate export (with --everything) would be a common invocation, and that because it is a common invocation, we should also assume --include="*" if no --include or --exclude flag was given

Ah okay! I see what you mean.

I mentioned this below but I'm a bit wary of this. I think it's easy to mistype git lfs migrate export when you mean to do something more specific, thus migrating everything in your repository. If your repository has lots of large files, this could also lead to a very large number of downloads.

Saying that, this is the way that git lfs migrate import works so it may be fine. If we were to implement it I'd like to do it the way import does it, i.e., maintaining a list of file extensions that it's done an operation on and adding attributes for each one.

Hmm. Good point, I agree that it could be upsetting for users to download many GiB of LFS object data.

That said, I think that we should leave it as-is, since I don't want to mirror the behavior of migrate import here, which I think should have generated an error as you have implemented it here.

With respect to my comment below, what happens if we remove these assignments iff we perform the prune after we do the checkout?

I'm not sure if this would be the place to put it, but I think that the --include="*" flag should only be the default if no --include or --exclude argument is given, since I think writing patterns against an implicit --include="*" would be confusing for users.

Hmm, I'm not sure I understand what you mean here. Do you mean that if we run git lfs migrate export, it should export all pointers? I'm a little worried about how easy that could make it for a user to accidentally do that operation, especially given the potentially large download that could occur.

Regarding my comment above, I was suggesting that if we decide on a default value of "*" for --include, it should only be the default when neither --include= nor --exclude= is given.

Should there be any assertions following this? I think that the corresponding migrate import test is blank because it was simply fixing a crash, but we should check that the export works correctly here.

Nit; I think that this is missing a newline here.