Skip to content

Conversation

@derrickstolee
Copy link
Contributor

@derrickstolee derrickstolee commented Mar 9, 2020

See microsoft/git#251 for details.

A few reaction items needed to happen:

  1. We need to ignore whitespace lines when verifying Git commands. Some added an extra whitespace line.
  2. The rebase backend changed, and it causes git rebase --continue to open an editor if the commit had conflicts. By changing core.editor=true, we can no-op the editor portion instead of running forever with an open vi editor.
  3. With the updated rebase backend, the commit OID is written to output. This causes our output between the control repo and the Scalar repo to be different. Set GIT_COMMITTER_DATE to be constant to avoid this problem.
  4. The fsmonitor hook version was updated, so we need to update our hook along with Git. By copying from the templates directory instead of the samples initialized in .git/hooks, we are future-proof and will always get the latest copy recommended by Git. Resolves Install hooks from Scalar installed components #301.

Copy link
Member

@kewillford kewillford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. A couple of questions/suggestions. Nothing big.

@derrickstolee derrickstolee force-pushed the vfs-2.26.0 branch 2 times, most recently from 2ee5918 to 75b632d Compare March 18, 2020 15:05
derrickstolee and others added 5 commits March 23, 2020 16:18
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
It appears that a behavior change in Git is causing the editor to be
opened after a rebase conflict.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Before, we would copy samples from the .git/hooks/ directory, but that
relies on the timing of when a user initialized their repository.
Further, the .git directory is less safe than the place where Git is
installed. This new mechanism allows us to upgrade the fsmonitor hook
automatically with new Git updates.

Also update core.fsmonitorHookVersion to 2 explicitly.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee derrickstolee changed the title [PR Build] Update Git to v2.26.0 Update Git to v2.26.0 Mar 24, 2020
@derrickstolee derrickstolee marked this pull request as ready for review March 24, 2020 00:22
@derrickstolee derrickstolee merged commit 9428eef into microsoft:master Mar 24, 2020
derrickstolee added a commit to derrickstolee/scalar that referenced this pull request Mar 27, 2020
In microsoft#349, we updated where the hooks were being copied from. Instead of
the local .git/hooks directory, we copy from the installed templates
directory. This allowws us to grab the latest hook associated with our
Git version.

However, I neglected testing thoroughly on Mac, and the path was not
rooted. This causes the config step to log an error because the path
starts at the current working directory, not from root.

Adjust the ConfigStep to return a 'false' result when this step fails,
which will cause the RunVerbTests to fail if we cannot set the hook as
expected.

The test does not verify the hook is placed because we only place it if
Watchman is installed.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
derrickstolee added a commit to derrickstolee/scalar that referenced this pull request Mar 27, 2020
In microsoft#349, we updated where the hooks were being copied from. Instead of
the local .git/hooks directory, we copy from the installed templates
directory. This allowws us to grab the latest hook associated with our
Git version.

However, I neglected testing thoroughly on Mac, and the path was not
rooted. This causes the config step to log an error because the path
starts at the current working directory, not from root.

Adjust the ConfigStep to return a 'false' result when this step fails,
which will cause the RunVerbTests to fail if we cannot set the hook as
expected.

The test does not verify the hook is placed because we only place it if
Watchman is installed.

However, we still have an issue with this approach, because our Git
installer does not update the templates directory correctly! We do not
include the v2 fsmonitor hook. Instead, use the raw hook data.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
derrickstolee added a commit to derrickstolee/scalar that referenced this pull request Mar 27, 2020
In microsoft#349, we updated where the hooks were being copied from. Instead of
the local .git/hooks directory, we copy from the installed templates
directory. This allowws us to grab the latest hook associated with our
Git version.

However, I neglected testing thoroughly on Mac, and the path was not
rooted. This causes the config step to log an error because the path
starts at the current working directory, not from root.

Adjust the ConfigStep to return a 'false' result when this step fails,
which will cause the RunVerbTests to fail if we cannot set the hook as
expected.

The test does not verify the hook is placed because we only place it if
Watchman is installed.

Also, I noticed that the templates directory was wrong! Our installer
puts the templates in a different place than another version of Git.
Perhaps that version was installed by XCode or something, but that
caused the confusion.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
derrickstolee added a commit to derrickstolee/scalar that referenced this pull request Mar 28, 2020
In microsoft#349, we updated where the hooks were being copied from. Instead of
the local .git/hooks directory, we copy from the installed templates
directory. This allowws us to grab the latest hook associated with our
Git version.

However, I neglected testing thoroughly on Mac, and the path was not
rooted. This causes the config step to log an error because the path
starts at the current working directory, not from root.

Adjust the ConfigStep to return a 'false' result when this step fails,
which will cause the RunVerbTests to fail if we cannot set the hook as
expected.

The test does not verify the hook is placed because we only place it if
Watchman is installed.

Also, I noticed that the templates directory was wrong! Our installer
puts the templates in a different place than another version of Git.
Perhaps that version was installed by XCode or something, but that
caused the confusion.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
derrickstolee added a commit to derrickstolee/scalar that referenced this pull request Mar 28, 2020
In microsoft#349, we updated where the hooks were being copied from. Instead of
the local .git/hooks directory, we copy from the installed templates
directory. This allowws us to grab the latest hook associated with our
Git version.

However, I neglected testing thoroughly on Mac, and the path was not
rooted. This causes the config step to log an error because the path
starts at the current working directory, not from root.

Adjust the ConfigStep to return a 'false' result when this step fails,
which will cause the RunVerbTests to fail if we cannot set the hook as
expected.

The test does not verify the hook is placed because we only place it if
Watchman is installed.

Also, I noticed that the templates directory was wrong! Our installer
puts the templates in a different place than another version of Git.
Perhaps that version was installed by XCode or something, but that
caused the confusion.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
derrickstolee added a commit that referenced this pull request Mar 28, 2020
…k location

In #349, we updated where the hooks were being copied from. Instead of
the local .git/hooks directory, we copy from the installed templates
directory. This allowws us to grab the latest hook associated with our
Git version.

However, I neglected testing thoroughly on Mac, and the path was not
rooted. This causes the config step to log an error because the path
starts at the current working directory, not from root.

Adjust the ConfigStep to return a 'false' result when this step fails,
which will cause the RunVerbTests to fail if we cannot set the hook as
expected.

The test does not verify the hook is placed because we only place it if
Watchman is installed.

Also, I noticed that the templates directory was wrong! Our installer
puts the templates in a different place than another version of Git.
Perhaps that version was installed by XCode or something, but that
caused the confusion.

(Replaced #359)
chrisd8088 added a commit to chrisd8088/scalar that referenced this pull request Sep 20, 2020
In commit cbf6f82 in PR microsoft#349
the ValidateGitCommand() helper function was refactored slightly
to use a new LinesShouldMatch() function; however, the inputs
were accidentally reversed, so here we correct the order of the
expected and actual string argments to LinesShouldMatch().
chrisd8088 added a commit to chrisd8088/scalar that referenced this pull request Sep 21, 2020
In commit cbf6f82 in PR microsoft#349 the
ValidateGitCommand() helper function was refactored slightly to use
a new LinesShouldMatch() function; however, the inputs were
accidentally reversed, so here we correct the order of the "expected"
and "actual" string arguments passed to LinesShouldMatch().
chrisd8088 added a commit to chrisd8088/scalar that referenced this pull request Sep 21, 2020
In commit cbf6f82 in PR microsoft#349 the
ValidateGitCommand() helper function was refactored slightly to use
a new LinesShouldMatch() function; however, the inputs were
accidentally reversed, so here we correct the order of the "expected"
and "actual" string arguments passed to LinesShouldMatch().
chrisd8088 added a commit to chrisd8088/scalar that referenced this pull request Sep 22, 2020
In commit cbf6f82 in PR microsoft#349 the
ValidateGitCommand() helper function was refactored slightly to use
a new LinesShouldMatch() function; however, the inputs were
accidentally reversed, so here we correct the order of the "expected"
and "actual" string arguments passed to LinesShouldMatch().
chrisd8088 added a commit to chrisd8088/scalar that referenced this pull request Sep 27, 2020
In commit cbf6f82 in PR microsoft#349 the
ValidateGitCommand() helper function was refactored slightly to use
a new LinesShouldMatch() function; however, the inputs were
accidentally reversed, so here we correct the order of the "expected"
and "actual" string arguments passed to LinesShouldMatch().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Install hooks from Scalar installed components

2 participants