-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add function to invoke Git with disabled LFS filters #2453
Conversation
aa4c822
to
8bb8614
Compare
git/git.go
Outdated
} | ||
|
||
// Invoke Git with disabled LFS filters | ||
func Exec(args ...string) *subprocess.Cmd { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to unexport this and SimpleExec
below, so that outside callers cannot git.Exec(...)
. This type of code allows arbitrary usage of Git, which I would like to prevent. If we find ourselves needing a new git
process that isn't already covered in this package, let's add it here where we can track it using static analysis tools, etc., instead of searching through the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean these kind of calls:
https://github.com/larsxschneider/git-lfs/blob/7418ab2974609e0fd2ec0d4e131126595feb31ed/commands/command_pointer.go#L120
... should go into the git
package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ideally I'd like that to turn into something like:
h, err := git.HashObject(someReader)
Where:
- The
exec("hash-object", ...)
would live inside of thegit
package, and - It would be exposed with a function exported from package
git
calledHashObject()
.
git/git.go
Outdated
} | ||
|
||
// Invoke Git with disabled LFS filters | ||
func SimpleExec(args ...string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same note here as above.
git/git.go
Outdated
@@ -490,13 +490,45 @@ func (c *gitConfig) IsGitVersionAtLeast(ver string) bool { | |||
return IsVersionAtLeast(gitver, ver) | |||
} | |||
|
|||
// Prepend Git config instructions to disable Git LFS filter | |||
func PrependConfigToDisableLFS(args ...string) []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do outside callers need this? If not, I'd like to un-export this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No outside caller needs this. Un-export is done by a lower case function name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
git/git.go
Outdated
@@ -490,13 +490,45 @@ func (c *gitConfig) IsGitVersionAtLeast(ver string) bool { | |||
return IsVersionAtLeast(gitver, ver) | |||
} | |||
|
|||
// Prepend Git config instructions to disable Git LFS filter | |||
func PrependConfigToDisableLFS(args ...string) []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, I'm not sure that disabling LFS in all uses of Exec()
, and SimpleExec()
is safe. If you want to keep this function, I'd recommend that we instead change the calls from:
// Exec runs Git, disabling LFS, with the given arguments.
func Something() ([]byte, error) {
return Exec("something", ...)
}
to:
@@ -1,5 +1,5 @@
-// Exec runs Git, disabling LFS, with the given arguments.
+// Exec runs Git with the given arguments.
func Something() ([]byte, error) {
- return Exec("something", ...)
+ return Exec(PrependConfigToDisableLFS("something")...)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand what you're saying here.
Would you prefer it if I don't use the additional Git Exec wrapper functions?
In case of config changes disabling LFS wouldn't be safe. But everything else should be, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in favor of the wrapper functions, with two recommendations:
- Un-export
Exec()
andSimpleExec()
. - Make
exec()
andsimpleExec()
execute their arguments literally, without implicitly prepending the LFS-disabling arguments to Git. If a caller wants to run a Git command that doesn't then go back and run LFS, they should callexec
like:exec(disableLfs(args...))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re 2) My thinking was that "disablingLFS" should be the "default" case, no? Although it's a bit verbose I agree with the explicit prepending!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re 2) My thinking was that "disablingLFS" should be the "default" case, no?
I would prefer that this not be the default case. I think that this approach will make it clearer that by calling exec()
/simpleExec()
you have to explicitly opt-in to not using LFS.
I agree though that this approach makes it harder for new callers to know when to add this and when not to. What do you think about execWithLFS
and execWithoutLfs
as another alternative?
dd9ae18
to
c86cd49
Compare
@ttaylorr subprocess/cmd and lfs/gitscanner_cmd look pretty similar. Would it make sense to extend |
If |
c86cd49
to
83b5b8c
Compare
Git LFS invokes Git for various actions internally. The new Git instance is started with the default Git config and therefore might invoke another Git LFS "filter-process". That is usually not be desired. Teach Git LFS to invoke Git with or without LFS filters.
The only side effect of this refactoring should be that the Git LFS is disabled for the Git subprocess. No other functional changes are intended.
a750271
to
6c0af76
Compare
@ttaylorr I am pretty happy with the current state. If you have time, please give is thorough review 😄 |
6c0af76
to
2b92300
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, this is a great refactor. Thanks :)
I do have some minor comments though:
git/git.go
Outdated
return bufio.NewScanner(cmd.Stdout), nil | ||
} | ||
|
||
func HashObject(by []byte) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about changing this to accept a reader? This will allow it to take larger objects without being forced to read it into memory first. Then you can get rid of the bytes.NewReader()
in here.
I realize that the usage in commands/command_pointer.go
will be just fine buffering in memory. But, I think it makes sense to change this function since it's now an exported git
package function.
git/git.go
Outdated
func UpdateIndex(file string) error { | ||
_, err := subprocess.SimpleExec("git", "update-index", "-q", "--refresh", file) | ||
return err | ||
func StartUpdateIndexFromStdin(output *bytes.Buffer) (io.WriteCloser, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same vein as my git.HashObject()
comment, what do you think about changing this to accept an io.Writer
? It'll give the caller some flexibility in how the output is captured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both done! Thanks for the suggestion 👍
The only side effect of this refactoring should be that the Git LFS is disabled for the Git subprocess. No other functional changes are intended.
The only side effect of this refactoring should be that the Git LFS is disabled for the Git subprocess. No other functional changes are intended.
…output Replicate `lfs/gitscanner_cmd` in `subprocess/buffered_cmd` using subprocess/cmd. In subsequent commits we will migrate all functions users of `lfs/gitscanner_cmd` to `subprocess/buffered_cmd` and afterwards remove `lfs/gitscanner_cmd`.
Git is called with the LFS filter here. No functional changes are intended.
The only side effect of this refactoring should be that the Git LFS is disabled for the Git subprocess. No other functional changes are intended.
The only side effect of this refactoring should be that the Git LFS is disabled for the Git subprocess. No other functional changes are intended.
The only side effect of this refactoring should be that the Git LFS is disabled for the Git subprocess. No other functional changes are intended.
2b92300
to
376464d
Compare
Git LFS invokes Git for various actions internally. A new Git instance is started with the default Git config and therefore might invoke another Git LFS process via the filter machinery. I have noticed that in #2439.
Teach Git LFS to invoke Git with disabled LFS filters to prevent unnecessary Git LFS processes.
I recommend to review the changes by commit.
Attention: I haven't performed extensive testing on this, yet.