Update Git to v2.27.0 #271
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 18, 2020
That looks like an inadvertent indentation change. |
And it looks that way in the over-all diff, too: https://github.com/microsoft/git/pull/271/files#diff-98e4472df34c789adc5b2d67f64e540eL1589-R1621
I don't find that commit upstream... Was it intentional to drop this from vfs-2.27.0? I see it in vfs-2.26.2...
Likewise this commit...
And these two...
Are you sure? I do not see any equivalent for this upstream. More concretely:
I see this in upstream's if (options->repo == the_repository && has_promisor_remote() &&
(options->output_format & output_formats_to_prefetch ||
options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK))I guess the int output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT |
DIFF_FORMAT_NUMSTAT |
DIFF_FORMAT_PATCH |
DIFF_FORMAT_SHORTSTAT |
DIFF_FORMAT_DIRSTAT;But your original patch did this: commit 51b804b57b04480128d91381c6976b999bbc9ce7
Author: Derrick Stolee <dstolee@microsoft.com>
Date: Mon Jan 6 15:39:50 2020 -0500
diff: skip batch object download when possible
When computing changed-path Bloom filters or performing a name-only
diff, we do not need the blob contents before completing the diff
values. Thus, we do not need to download a pack containing the blobs
we do not have on-disk before completing our diff calculation.
This prevents downloading every blob in a partial clone during
"git log --raw" and "git diff --name-only" commands.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
diff --git a/diff.c b/diff.c
index f2cfbf2214a2..7a5f8b4bbc56 100644
--- a/diff.c
+++ b/diff.c
@@ -4630,6 +4630,11 @@ void diff_setup_done(struct diff_options *options)
if (!options->use_color || external_diff())
options->color_moved = 0;
+ if (!(options->output_format & ~(DIFF_FORMAT_NAME |
+ DIFF_FORMAT_RAW)) &&
+ !options->detect_rename)
+ options->skip_batch_download_objects = 1;
+
FREE_AND_NULL(options->parseopts);
}
@@ -6504,7 +6509,9 @@ static void add_if_missing(struct repository *r,
void diffcore_std(struct diff_options *options)
{
- if (options->repo == the_repository && has_promisor_remote()) {
+ if (!options->skip_batch_download_objects &&
+ options->repo == the_repository &&
+ has_promisor_remote()) {
/*
* Prefetch the diff pairs that are about to be flushed.
*/
diff --git a/diff.h b/diff.h
index 6febe7e3656a..89131a185a45 100644
--- a/diff.h
+++ b/diff.h
@@ -281,6 +281,7 @@ struct diff_options {
int show_rename_progress;
int dirstat_permille;
int setup;
+ int skip_batch_download_objects;
/* Number of hexdigits to abbreviate raw format output to. */
int abbrev;
In other words, I do not think that upstream has this: if only (I could imagine that this logic is only sound if tree objects have been fetched already, but that is beside the point here: we do not want to regress just because we inadvertently drop (parts of) patches when rebasing.) But what I see upstream is a slightly different logic: instead of a negative logic it has a positive one. Granted, it does handle But yes, the surviving version of your patch only ever sets
|
Ah, I think I see it here:
IOW it was squashed into 1ae89a5. |
This is actually no longer necessary, as
But I am actually certain that this is still needed, after inspecting the commit and its context (the rationale in the commit message still makes sense to me, too). |
I gave this some thought, and I thought the best would be to do this: Subject: [PATCH] fixup! diff: skip batch object download when possible
---
diff.c | 11 ++++++-----
diff.h | 1 -
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index 0d5918dfc244..86a674b6b0d5 100644
--- a/diff.c
+++ b/diff.c
@@ -4666,11 +4666,6 @@ void diff_setup_done(struct diff_options *options)
if (!options->use_color || external_diff())
options->color_moved = 0;
- if (!(options->output_format & ~(DIFF_FORMAT_NAME |
- DIFF_FORMAT_RAW)) &&
- !options->detect_rename)
- options->skip_batch_download_objects = 1;
-
FREE_AND_NULL(options->parseopts);
}
@@ -6578,6 +6573,12 @@ void diffcore_std(struct diff_options *options)
DIFF_FORMAT_SHORTSTAT |
DIFF_FORMAT_DIRSTAT;
+ if (options->detect_rename)
+ output_formats_to_prefetch |=
+ DIFF_FORMAT_DIFFSTAT |
+ DIFF_FORMAT_SUMMARY |
+ DIFF_FORMAT_NAME_STATUS;
+
/*
* Check if the user requested a blob-data-requiring diff output and/or
* break-rewrite detection (which requires blob data). If yes, prefetch
diff --git a/diff.h b/diff.h
index e9f104309c45..9443dc1b0039 100644
--- a/diff.h
+++ b/diff.h
@@ -281,7 +281,6 @@ struct diff_options {
int show_rename_progress;
int dirstat_permille;
int setup;
- int skip_batch_download_objects;
/* Number of hexdigits to abbreviate raw format output to. */
int abbrev;
--
2.27.0.rc0.windows.1However, then I finally did what I should have done in the first place and looked at the history: The commit message clearly describes that In other words: I am now finally convinced that you were right all along, @derrickstolee, and this commit can be dropped now. |
|
@derrickstolee I spent a little bit of time on these patches and I think I found good spots for the two dropped commits, and I reintroduced a couple of those merge commits that structure the I hope that this lightens your burden; My hope is that you will find this good enough and just force-push 2126e09 to |
I also rebased this on top of the just-released v2.27.0-rc1 and pushed out the result as So now my hope is that you will find that branch good enough ;-) |
Thanks for this, @dscho! I will most likely take your |
|
Thank you! |
Yes. Thanks for fixing it in your
I did intentionally drop this, but forgot to comment about it. The commit message mentions how So I would call this commit "obsolete due to upstream changes." I'll drop it from your
This patch worked for the legacy stash working with post-command hooks, but the legacy stash is removed! (And you commented that you figured that out. I'll do better with justifying my dropped patches in the future so you don't need to do that.)
The
(You did the work to be absolutely sure we can drop it.) |
Thank you for the explanation!
No worries!
Yep, I saw that, and that's why I cherry-picked it back.
So you agree? Excellent! |
derrickstolee
force-pushed
the
tentative/vfs-2.27.0
branch
from
9ee9f47
to
1a0ad6f
Compare
|
I just pushed my adjustments to @dscho's The only tree-diff from his HEAD to mine is the change from dropping b2422f6 |
|
I updated the branch thicket to group common areas together better. Here is the new and the old: |
|
Nice! My only (very minor) nit:
This merge commit probably now wants an adjusted commit message, as does...
... this one and...
... this one, and ...
... this one, too.
|
I noticed that, too. I did reword the merges a few times but it was early in my multiple attempts to get the graph right. I'll reword in the next rebase, now that the structure is solidified. |
dscho
force-pushed
the
tentative/vfs-2.27.0
branch
from
1a0ad6f
to
f47a145
Compare
It made it upstream, into v2.27.0-rc2, as e68a527. I rebased and force-pushed. |
dscho
changed the title
derrickstolee
force-pushed
the
tentative/vfs-2.27.0
branch
from
f47a145
to
fdc68d1
Compare
|
Thanks, @dscho for rebasing onto I just wanted to point out that I pushed again after editing the merge commit message. The first-parent history now looks like this: |
This looks great! |
While using the reset --stdin feature on windows path added may have a \r at the end of the path that wasn't getting removed so didn't match the path in the index and wasn't reset. Signed-off-by: Kevin Willford <kewillf@microsoft.com>
Signed-off-by: Saeed Noursalehi <sanoursa@microsoft.com>
Signed-off-by: Johannes Schindelin <johasc@microsoft.com>
This header file will accumulate GVFS-specific definitions. Signed-off-by: Kevin Willford <kewillf@microsoft.com>
This does not do anything yet. The next patches will add various values for that config setting that correspond to the various features offered/required by GVFS. Signed-off-by: Kevin Willford <kewillf@microsoft.com>
This takes a substantial amount of time, and if the user is reasonably sure that the files' integrity is not compromised, that time can be saved. Git no longer verifies the SHA-1 by default, anyway. Signed-off-by: Kevin Willford <kewillf@microsoft.com>
If our POST request includes a commit ID, then the the remote will send a pack-file containing the commit and all trees reachable from its root tree. With the current implementation, this causes a failure since we call install_loose() when asking for one object. Modify the condition to check for install_pack() when the response type changes. Also, create a tempfile for the pack-file download or else we will have problems! Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
…and report_tracking() Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
The gvfs-helper allows us to download prefetch packs using a simple subprocess call. The gvfs-helper-client.h method will automatically compute the timestamp if passing 0, and passing NULL for the number of downloaded packs is valid. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Create t/helper/test-gvfs-protocol.c and t/t5799-gvfs-helper.sh to test gvfs-helper. Create t/helper/test-gvfs-protocol.c as a stand-alone web server that speaks the GVFS Protocol [1] and serves loose objects and packfiles to clients. It is borrows heavily from the code in daemon.c. It includes a "mayhem" mode to cause various network and HTTP errors to test the retry/recovery ability of gvfs-helper. Create t/t5799-gvfs-helper.sh to test gvfs-helper. [1] https://github.com/microsoft/VFSForGit/blob/master/Protocol.md Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Teach gvfs-helper to better support the concurrent fetching of the same packfile by multiple instances. If 2 instances of gvfs-helper did a POST and requested the same set of OIDs, they might receive the exact same packfile (same checksum SHA). Both processes would then race to install their copy of the .pack and .idx files into the ODB/pack directory. This is not a problem on Unix (because of filesystem semantics). On Windows, this can cause an EBUSY/EPERM problem for the loser while the winner is holding a handle to the target files. (The existing packfile code already handled simple the existence and/or replacement case.) The solution presented here is to silently let the loser claim victory IIF the .pack and .idx are already present in the ODB. (We can't check this in advance because we don't know the packfile SHA checksum until after we receive it and run index-pack.) We avoid using a per-packfile lockfile (or a single lockfile for the `vfs-` prefix) to avoid the usual issues with stale lockfiles. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
gvfs-helper prints a "loose <oid>" or "packfile <name>" messages after they are received to help invokers update their in-memory caches. Move the code to accumulate these messages in the result_list into the install_* functions rather than waiting until the end. POST requests containing 1 object may return a loose object or a packfile depending on whether the object is a commit or non-commit. Delaying the message generation just complicated the caller. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Add data for the number of files created/overwritten and deleted during the checkout. Give proper category name to all events in unpack-trees.c and eliminate "exp". This is modified slightly from the original version due to interactions with 26f924d (unpack-trees: exit check_updates() early if updates are not wanted, 2020-01-07). Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
When using the GVFS protocol, we should _never_ call "git fetch-pack" to attempt downloading a pack-file via the regular Git protocol. It appears that the mechanism that prevented this in the VFS for Git world is due to the read-object hook populating the commits at the new ref tips in a different way than the gvfs-helper does. By acting as if the fetch-pack succeeds here in remote-curl, we prevent a failed fetch. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Earlier versions of the test always returned a packfile in response to a POST. Now we look at the number of objects in the POST request. If > 1, always send a packfile. If = 1 and it is a commit, send a packfile. Otherwise, send a loose object. This is to better model the behavior of the GVFS server/protocol which treats commits differently. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Update tracing around report_tracking() to use 'tracking' category rather than 'exp' category. Add ahead/behind results from stat_tracking_info(). Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
When we create temp files for downloading packs, we use a name based on the current timestamp. There is no randomness in the name, so we can have collisions in the same second. Retry the temp pack names using a new "-<retry>" suffix to the name before the ".temp". Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Test t5516-fetch-push.sh has a test 'deny fetch unreachable SHA1, allowtipsha1inwant=true' that checks stderr for a specific error string from the remote. In some build environments the error sent over the remote connection gets mingled with the error from the die() statement. Since both signals are being output to the same file descriptor (but from parent and child processes), the output we are matching with grep gets split. To reduce the risk of this failure, follow this process instead: 1. Write an error message to stderr. 2. Write an error message across the connection. 3. exit(1). This reorders the events so the error is written entirely before the client receives a message from the remote, removing the race condition. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Fix NTLM support in gvfs-helper. NTLM is handled magically by libcurl when we CURLAUTH_ANY is enabled AND we passed empty username/password. This lets it negotiate with the server and choose the best authentication scheme.
Includes commits from these pull requests: #188 Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
derrickstolee
force-pushed
the
tentative/vfs-2.27.0
branch
from
fdc68d1
to
26f52f4
Compare
dscho
approved these changes
Jun 1, 2020
derrickstolee
changed the title
derrickstolee
added a commit
to microsoft/scalar
that referenced
this pull request
Jun 2, 2020
See microsoft/git#271. Updates necessary here: * The `multi-pack-index` builtin started honoring `repack.packKeptObjects`, which is `false` by default. We want to repack the `.keep` pack because that is just the most-recent prefetch pack-file. We don't expire it, but we want to repack it. * Upstream now has the `log.excludeDecoration` config option. This allows us to hide the `refs/scalar/hidden/*` refs from the history views, de-cluttering the history for our Scalar enlistments.
derrickstolee
added a commit
to microsoft/VFSForGit
that referenced
this pull request
Jun 2, 2020
See microsoft/git#271. Updates necessary here: * The `multi-pack-index` builtin started honoring `repack.packKeptObjects`, which is `false` by default. We want to repack the `.keep` pack because that is just the most-recent prefetch pack-file. We don't expire it, but we want to repack it. * The `sparse-checkout` code changed some error messages and behavior, so the `reset --mixed` tests no longer reflect an exact behavior match to vanilla Git (without sparse-checkout). Since VFS for Git dynamically sparsifies, we can't exactly match the behavior in the control repo.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here is the rebase on top of
v2.27.0-rc0.windows.1.I did quite a bit of squashing and rearranging commits to ensure that each one compiles and passes the test suite. (I probably won't repeat the test suite check again.)
I'll update the range-diff soon with annotations after I kick off the installer builds.
Range-diff:
These three commits got squashed together somehow, it seems:
The following commit was dropped because it is now non-functional:
This one is dropped on purpose because the legacy stash is removed:
The rest of the commits are dropped.
These were squashed into the first
gvfs-helpercommit:These were all introduced upstream, or are no longer needed because of upstream changes: