Add `--no-rewrite` flag to `migrate import` command #3029

PastelMobileSuit · 2018-05-26T00:18:02Z

This PR adds a new --no-rewrite flag to the migrate import command. When this flag is given, migrate import takes a list of files as arguments and imports these files into Git LFS. It does this by rewriting the git tree in a new commit, without modifying git history.

Some of the code in this PR was based off of code previously written by @ttaylorr. Also, a big thank you to @ttaylorr for pairing with me on parts of this PR!

Allow `migrate import` command to create a new commit without modifying git history when using the --no-rewrite flag

Allow the `migrate import` command to convert blobs to git-lfs pointers by rewriting the tree in a new commit without touching git history

Change the `migrate import` command to take a list of files to be imported when the `--no-rewrite` flag is used.

Add a new test script, test-migrate-import-no-rewrite.sh, that runs a series of integration tests using the `migrate import` command and the `--no-rewrite` flag

ttaylorr

Welcome to Git LFS! This is a stellar first contribution and I am so excited to land these changes in v2.5.0. I left a handful of notes on various things, some little and some not.

Let me know if you have any thoughts/questions:

ttaylorr · 2018-05-28T23:34:09Z

commands/command_migrate.go

@@ -287,6 +294,8 @@ func init() {
 	importCmd := NewCommand("import", migrateImportCommand)
 	importCmd.Flags().BoolVar(&migrateVerbose, "verbose", false, "Verbose logging")
 	importCmd.Flags().StringVar(&objectMapFilePath, "object-map", "", "Object map file")
+	importCmd.Flags().BoolVar(&migrateNoRewrite, "no-rewrite", false, "Add new history without rewriting previous")


I think that these are great short descriptions of what the --no-rewrite and --message flags do, which is exactly what is appropriate for these.

Could you also add a longer explanation of what these flags are in git-lfs-migrate(1)? The docs/man tree is used to automatically generate commands/mancontent.go, and this in turn is what is shown when you run git lfs migrate --help, or git lfs help migrate, for instance.

The existing prose should be a good place to start in terms of what level of detail is appropriate.

ttaylorr · 2018-05-28T23:35:41Z

commands/command_migrate_import.go

@@ -29,6 +30,67 @@ func migrateImportCommand(cmd *cobra.Command, args []string) {
 	}
 	defer db.Close()

+	if migrateNoRewrite {
+		if len(args) == 0 {
+			ExitWithError(errors.Errorf("fatal: the migrate import command requires a list of files to import when using the --no-rewrite flag"))


Great; I think dying here is the right thing to do. Let's shorten this message a little bit while we're here. For instance, the phrase "the migrate import command" might be unnecessary, since this will be the first line of output after a user runs git lfs migrate import.

What about: "fatal: expected one or more file(s) with --no-rewrite"?

ttaylorr · 2018-05-28T23:36:24Z

commands/command_migrate_import.go

+			ExitWithError(errors.Wrap(err, "fatal: unable to find current reference"))
+		}
+
+		sha, _ := hex.DecodeString(ref.Sha)


Great; throwing this error away is reasonable since we trust git.CurrentRef() to always give back a valid SHA-256 ref.Sha.

ttaylorr · 2018-05-28T23:40:57Z

commands/command_migrate_import.go

+
+		filter := git.GetAttributeFilter(cfg.LocalWorkingDir(), cfg.LocalGitDir())
+		if len(filter.Include()) == 0 && len(filter.Exclude()) == 0 {
+			ExitWithError(errors.Errorf("fatal: no git lfs filters setup in .gitattributes"))


Let's use "found" instead of "setup".

ttaylorr · 2018-05-28T23:46:16Z

commands/command_migrate_import.go

+			ExitWithError(errors.Errorf("fatal: no git lfs filters setup in .gitattributes"))
+		}
+
+		for _, file := range args {


I think one interesting consequence of rewriteTree() processing one file at a time is that there are some penalties that we'll pay if we get part of the way through this loop. In particular, if there are m arguments where m is large, and the m-1th of them doesn't match a pattern in .gitattributes, we'll have created a lot of tree objects that are dangling in the object database.

So, I think that we could run this loop twice, once to check that all given filenames match at least one pattern, and then once more to call rewriteTree(), which would mean we spend more time here. I think that's an OK tradeoff for not creating more objects than we need.

Of course, we could also look into ways to make rewriteTree() take more than one file and therefore create less dangling tree objects, but I'll leave it up to you if you want to go down that path.

Ah okay, thanks! I didn't think about the possibility of dangling objects.

I agree with you on running the loop twice and think that's the best option, since we won't be able to avoid checking files against the .gitattributes filters.

ttaylorr · 2018-05-28T23:52:11Z

commands/command_migrate_import.go

+	}
+}
+
+// findEntry searches a tree for the desired entry, and returns the


Nit; This line is wrapped at fewer than 80 characters.

ttaylorr · 2018-05-28T23:53:33Z

git/attribs.go

+	patterns := make([]filepathfilter.Pattern, len(paths))
+
+	for i, path := range paths {
+		patterns[i] = filepathfilter.NewPattern(path.Path)


In case we want to change this later in a way that takes away the 1-1 correspondence between paths and patterns, we can minimize that diff by calling append() instead of using the slicing operator []. In order to make that work as-expected, you'll want to change L108 to initialize patterns with a capacity of len(paths), instead of a len of len(paths).

ttaylorr · 2018-05-28T23:53:58Z

test/test-migrate-fixtures.sh

+# - Commit 'A' has 120, in a.txt, and a corresponding entry in .gitattributes. There is also
+#   140 in a.md, with no corresponding entry in .gitattributes.
+#   It also has 140 in subtree/a.md, and a corresponding entry in subtree/.gitattributes
+setup_local_branch_with_nested_gitattrs() {


✨ Awesome!

ttaylorr · 2018-05-28T23:55:43Z

test/test-migrate-import-no-rewrite.sh

+  assert_local_object "$txt_oid" "120"
+
+  # Ensure the git history remained the same
+  new_commit_oid="$(git rev-parse HEAD~1)"


ttaylorr · 2018-05-28T23:57:19Z

test/test-migrate-import-no-rewrite.sh

+  setup_multiple_local_branches
+
+  # Ensure command fails if no .gitattributes files are present
+  [ !"$(git lfs migrate import --no-rewrite *.txt *.md)" ]


Do you think we could check that the error message this invocation gives is what we expect it to be? I think one way to do this would be to write something of the ilk:

git lfs migrate import --no-rewrite *.txt *.md 2>&1 | tee migrate.log if [ ${PIPESTATUS[0]} -eq 0 ]; then echo >&2 "fatal: expected git lfs migrate import --no-rewrite to fail, didn't" fi grep "..." migrate.log

Add documentation about the `--no-rewrite` flag to the man page for the `migrate` command

* Update error message text to be more concise/descriptive * Fix casing/formatting issues * Inline variables to reduce scope * Cache and reuse GitFilter instead of creating one every time a clean is performed

With the `--no-rewrite` flag to the `migrate import`, check all provided files match `.gitattributes` filters before any tree rewrite operations.

Append elements to the array rather than assigning them to indices in GetAttributeFilter

Update `--no-rewrite` error messages to correctly case Git LFS and use the term `filters`

In the `--no-rewrite` tests that cover error cases, test that the provided error message is as expected

PastelMobileSuit · 2018-05-30T18:02:42Z

@ttaylorr Thank you for the review! I've gone ahead and pushed up some new commits addressing your feedback

ttaylorr

Great work, @PastelMobileSuit! I left a few minor nit-picks, but besides that I think we are ready to go.

ttaylorr · 2018-05-30T18:40:13Z

commands/command_migrate_import.go

+		root := commit.TreeID
+
+		filter := git.GetAttributeFilter(cfg.LocalWorkingDir(), cfg.LocalGitDir())
+		if len(filter.Include()) == 0 && len(filter.Exclude()) == 0 {


Can we avoid checking len(filter.Exclude())? My thought is that if filter.Include() is empty, but filter.Exclude() is not, we'll get past this check, but not pick up any files.

ttaylorr · 2018-05-30T18:41:49Z

test/test-migrate-import-no-rewrite.sh

+
+  # Ensure command fails if no .gitattributes files are present
+  git lfs migrate import --no-rewrite *.txt *.md 2>&1 | tee migrate.log
+  if [ ${PIPESTATUS[0]} -eq 0 ]; then


Should we exit 1 here?

ttaylorr · 2018-05-30T18:41:58Z

test/test-migrate-import-no-rewrite.sh

+  # Failure should occur when trying to import a.md as no entry exists in
+  # top-level .gitattributes file
+  git lfs migrate import --no-rewrite a.md 2>&1 | tee migrate.log
+  if [ ${PIPESTATUS[0]} -eq 0 ]; then


Should we exit 1 here as well?

ttaylorr · 2018-05-30T18:42:08Z

test/test-migrate-import-no-rewrite.sh

+    exit 1
+  fi
+)
+end_test


Nit: missing trailing LF.

ttaylorr · 2018-05-30T18:42:55Z

docs/man/git-lfs-migrate.1.ronn

+    a different argument list, specialized options will become available, and the core 
+    `migrate` options will be ignored. See [IMPORT (NO REWRITE)].
+
+If `--no-rewrite` is not provided and `--include` or `--exclude` (`-I`, `-X`, respectively) are given, the


Nit: it looks like a few lines in this file are longer than 80 columns. Can you make sure that the wrapping is set properly for .ronn files?

ttaylorr · 2018-05-30T18:43:44Z

docs/man/git-lfs-migrate.1.ronn

+
+The `--no-rewrite` sub-mode supports the following options and arguments:
+
+* `-m` <message> `--message`=<message>


I think the backticks should wrap from the first - to the last > in each example.

ttaylorr · 2018-05-30T18:44:52Z

docs/man/git-lfs-migrate.1.ronn

+The `import` mode has a special sub-mode enabled by the `--no-rewrite` flag. This sub-mode
+will migrate large objects to pointers as in the base `import` mode, but will do so in a new commit
+without rewriting Git history. When using this sub-mode, the base `migrate` options, 
+such as `--include-ref`, will be ignored, as will those for the base `import` mode. 


I think that ignoring --include-ref is totally OK here, but we should perhaps add a note in the documentation that all interested branches will have to merge new commits generated by git lfs migrate import --no-rewrite if they also want to take new commits in.

ttaylorr · 2018-05-30T18:45:34Z

docs/man/git-lfs-migrate.1.ronn

+You can also migrate files without modifying the existing history of your respoitory:
+
+```
+# Without a specified commit message


Let's move these comments out of the code block, and replace the shell with 4 spaces, and then git lfs migrate ... (and without the $).

Ensure `/` is used as the path separator when parsing Git LFS filters in nested directories.

PastelMobileSuit · 2018-05-31T17:28:35Z

git/attribs.go

@@ -73,7 +73,7 @@ func GetAttributePaths(workingDir, gitDir string) []AttributePath {
 				fields := strings.Fields(line)
 				pattern := fields[0]
 				if len(reldir) > 0 {
-					pattern = filepath.Join(reldir, pattern)
+					pattern = filepath.ToSlash(filepath.Join(reldir, pattern))


The necessity of this change was prompted by a consistent Windows test failure I was seeing on a VM. Every time I ran the nested .gitattributes test, I saw a failure.

It turns out the problem is that filepath.Join() uses \ as the separator character on Windows, rather than /. As a result, a *.txt pattern in a folder subtree would be parsed as subtree\*.txt, and instead of acting as a path separator, the \ would escape the * character, so the pattern would then only match a file with the name subtree*.txt.

Great fix! One nit is that filepath.Join calls filepath.Clean (see [1]), which can have unintended consequences with certain pattern types (i.e., escape sequences are removed, etc.)

How about we shorten this to:

pattern = fmt.Sprintf("%s/%s", reldir, pattern)

or:

pattern = strings.Join([]string{reldir, pattern}, "/")

@ttaylorr Hey! I believe this code was already being used as-is with the track command, so I think that was already removing escape characters from patterns. I did some investigation to confirm this, and git lfs track "*\*.mov" gets converted to */*.mov in the .gitattributes file, and if you manually change it to *\*.mov, that's not matched by track on a file with the name a*.mov.

@PastelMobileSuit Oh! That makes sense; it looks like as a historical artifact we don't disambiguate between *\*.dat meaning "any directory name and any filename, ending in '.dat'" (on Unix) and "any filename, ending with '*.dat' (literal '*')" (on Windows).

I think it's safer to leave this behavior as-is, in which case it makes sense to me to keep using filepath.ToSlash(filepath.Join(...)). Merge away!

* Fix various formatting issues * Add note about `--no-rewrite` commits needing to be merged into other branches

* Exit `migrate import --no-rewrite` if attributes filter has no elements in its Include() list * In `--no-rewrite` tests checking errors, fail out of test if the command doesn't fail

Shorten the repo/folder names in the nested .gitattributes test to prevent AppVeyor failure

Convert path separators to `/` when creating a pattern from a path to a .gitattributes filter instead of on creating the path to the filter

PastelMobileSuit · 2018-05-31T21:44:32Z

Okay I've pushed up some additional commits to address feedback and have finally fixed all the AppVeyor test failures, so this should be ready to go!

ttaylorr

One tiny change requested, but everything looks great! Well done.

ghost · 2018-09-17T10:01:34Z

@PastelMobileSuit @ttaylorr
Hi!
I am not sure I grasp the use-case for the new "--no-rewrite" feature of git lfs import:
I was looking for a way to migrate our huge repository to lfs withouth destroying all of our (numerous) references to commit SHAs. I came to the conclusion that this is not possible and we will need to do a clean start with a rewritten history.
Then I found the announcement of the "--no-rewrite" feature and thought it may provide a way of moving all required files to lfs without changing the history (maybe by leaving the SHA intact although the content of the commits has changed). But this does not seem to be the use-case for this feature.

Can you please elaborate on when to use this feature? Sorry if I am missing something obvious, but the manual for migrate did not help me understand it and there is no further documentation that I can find.

Thanks a lot!

ttaylorr · 2018-09-17T13:31:47Z

Hi, @hoeler. Thanks for writing in, and I'd be more than happy to answer your question. It seems to me that the central point is:

Then I found the announcement of the "--no-rewrite" feature and thought it may provide a way of moving all required files to lfs without changing the history (maybe by leaving the SHA intact although the content of the commits has changed).

Which isn't quite the case. The '--no-rewrite' flag works by _adding_ new commits, instead of _rewriting_ existing ones. The SHA-1 of a commit is dependent upon the contents of the files in that commit's root tree, the parents, message, and etc., of the commit itself. So, there is no way to "rewrite" the contents of a commit without also changing its SHA-1. '--no-rewrite' is used in a case where (1) you have files in Git that should have been committed with Git LFS, but weren't, and (2) those files are under 100 MiB (thus, don't prevent pushing to github.com), and/or (3) history rewriting is not an option. Per (3), when you run 'git lfs migrate <import | export>', you have to _also_ ensure that each clone of the repository has (1) fetched and (2) reset their local references to match the remote's. When running 'git lfs migrate import' with '--no-rewrite', the repository's cloners need only run 'git pull' to receive the new changes. The central difference between the two from the standpoint of "what are my references?" is: 'git lfs migrate import' (with or without '--no-rewrite') changes your references, the difference is whether or not the old values are still valid locations in your repository's line of history or not.

ghost · 2018-09-17T14:12:07Z

Hi @ttaylorr, thank you for the explanation.
I am still not sure if i got it right though:

So basically "--no-rewrite" only works with the tip of a branch, by replacing existing files passed as arguments with LFS pointers and commiting that as a new change? Isn't this the same as manually removing the files from git and simply re-adding them under LFS control?
As I understand it now the "--no-rewrite" option does not help with getting rid of large files referenced somewhere in the git history, because this will always require a rewrite of the history, correct?

ttaylorr · 2018-09-17T14:30:29Z

1) So basically "--no-rewrite" only works with the tip of a branch, by replacing existing files passed as arguments with LFS pointers and commiting that as a new change? Isn't this the same as manually removing the files from git and simply re-adding them under LFS control?

I think that the gist of that is correct, but 'git lfs migrate import --no-rewrite' has two attributes that make it different than 'git rm --cached', which are: 1. Change each file in a separate commit, and 2. Perform the change in Git LFS, so that it will work even if 'git lfs install' hasn't been run.

2) As I understand it now the "--no-rewrite" option does not help with getting rid of large files referenced somewhere in the git history, because this will _always_ require a rewrite of the history, correct?

Exactly.

ghost · 2018-09-18T11:22:13Z

Thanks @ttaylorr for clearing it up for me!

PastelMobileSuit added 5 commits May 22, 2018 11:12

commands: teach --no-rewrite flag to 'migrate-import'

9820f20

commands: create new commit with --no-rewrite

bbcea3d

Allow `migrate import` command to create a new commit without modifying git history when using the --no-rewrite flag

commands: rewrite tree with --no-rewrite

9632eb9

Allow the `migrate import` command to convert blobs to git-lfs pointers by rewriting the tree in a new commit without touching git history

commands: take files as args with --no-rewrite

8f6c9e9

Change the `migrate import` command to take a list of files to be imported when the `--no-rewrite` flag is used.

command: add integration tests for --no-rewrite

133da52

Add a new test script, test-migrate-import-no-rewrite.sh, that runs a series of integration tests using the `migrate import` command and the `--no-rewrite` flag

ttaylorr reviewed May 28, 2018

View reviewed changes

PastelMobileSuit added 6 commits May 29, 2018 16:56

commands: add man page docs for --no-rewrite

dfcbd79

Add documentation about the `--no-rewrite` flag to the man page for the `migrate` command

commands: address --no-rewrite feedback

9774ddc

* Update error message text to be more concise/descriptive * Fix casing/formatting issues * Inline variables to reduce scope * Cache and reuse GitFilter instead of creating one every time a clean is performed

commands: pre-check files against lfs filters

62b1142

With the `--no-rewrite` flag to the `migrate import`, check all provided files match `.gitattributes` filters before any tree rewrite operations.

git: use append in GetAttributeFilter()

afffdd0

Append elements to the array rather than assigning them to indices in GetAttributeFilter

commands: use consistent casing/terms in errors

5fec165

Update `--no-rewrite` error messages to correctly case Git LFS and use the term `filters`

commands: test error messages are as expected

188a4c6

In the `--no-rewrite` tests that cover error cases, test that the provided error message is as expected

ttaylorr approved these changes May 30, 2018

View reviewed changes

git: use '/' when parsing paths to lfs filters

abbd514

Ensure `/` is used as the path separator when parsing Git LFS filters in nested directories.

PastelMobileSuit commented May 31, 2018

View reviewed changes

PastelMobileSuit added 4 commits May 31, 2018 11:18

docs: update migrate docs based on feedback

77ebf9b

* Fix various formatting issues * Add note about `--no-rewrite` commits needing to be merged into other branches

commands: fix issues based on feedback

30cdf66

* Exit `migrate import --no-rewrite` if attributes filter has no elements in its Include() list * In `--no-rewrite` tests checking errors, fail out of test if the command doesn't fail

test: shorten repo/folder names

6e40c67

Shorten the repo/folder names in the nested .gitattributes test to prevent AppVeyor failure

git: convert separators to / on pattern creation

7caa122

Convert path separators to `/` when creating a pattern from a path to a .gitattributes filter instead of on creating the path to the filter

ttaylorr approved these changes Jun 8, 2018

View reviewed changes

PastelMobileSuit force-pushed the no-rewrite-flag branch from 6cb4b36 to 7caa122 Compare June 8, 2018 21:28

ttaylorr merged commit caf1e9a into git-lfs:master Jun 11, 2018

ttaylorr added this to the v2.5.0 milestone Jun 11, 2018

ttaylorr mentioned this pull request Jun 13, 2018

Implement migrate export command #3057

Closed

bitinn mentioned this pull request Dec 10, 2018

git lfs migrate --no-rewrite couldn't see files in commit? #3421

Closed

ttaylorr mentioned this pull request Jan 27, 2019

Make it easier to recover from "file(s) that should have been pointers, but weren't" #2910

Closed

This was referenced Jan 15, 2020

migrate woff file from binary to LFS pointer learningequality/kolibri#6467

Merged

Encountered 1 file(s) that should have been pointers, but weren't #1939

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--no-rewrite` flag to `migrate import` command #3029

Add `--no-rewrite` flag to `migrate import` command #3029

PastelMobileSuit commented May 26, 2018

ttaylorr left a comment

ttaylorr May 28, 2018

ttaylorr May 28, 2018

ttaylorr May 28, 2018

ttaylorr May 28, 2018

ttaylorr May 28, 2018

PastelMobileSuit May 30, 2018

ttaylorr May 28, 2018

ttaylorr May 28, 2018

ttaylorr May 28, 2018

ttaylorr May 28, 2018

ttaylorr May 28, 2018

PastelMobileSuit commented May 30, 2018

ttaylorr left a comment

ttaylorr May 30, 2018

ttaylorr May 30, 2018

ttaylorr May 30, 2018

ttaylorr May 30, 2018

ttaylorr May 30, 2018

ttaylorr May 30, 2018

ttaylorr May 30, 2018

ttaylorr May 30, 2018

PastelMobileSuit May 31, 2018

ttaylorr Jun 8, 2018

PastelMobileSuit Jun 8, 2018

ttaylorr Jun 8, 2018 •

edited

Loading

PastelMobileSuit commented May 31, 2018

ttaylorr left a comment

ghost commented Sep 17, 2018

ttaylorr commented Sep 17, 2018 via email

ghost commented Sep 17, 2018

ttaylorr commented Sep 17, 2018 via email

ghost commented Sep 18, 2018


		The `--no-rewrite` sub-mode supports the following options and arguments:

		* `-m` <message> `--message`=<message>

Add --no-rewrite flag to migrate import command #3029

Add --no-rewrite flag to migrate import command #3029

Conversation

PastelMobileSuit commented May 26, 2018

ttaylorr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PastelMobileSuit commented May 30, 2018

ttaylorr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttaylorr Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

PastelMobileSuit commented May 31, 2018

ttaylorr left a comment

Choose a reason for hiding this comment

ghost commented Sep 17, 2018

ttaylorr commented Sep 17, 2018 via email

ghost commented Sep 17, 2018

ttaylorr commented Sep 17, 2018 via email

ghost commented Sep 18, 2018

Add `--no-rewrite` flag to `migrate import` command #3029

Add `--no-rewrite` flag to `migrate import` command #3029

ttaylorr Jun 8, 2018 •

edited

Loading