Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
Hi Junio,
On Sun, 21 Oct 2018, Junio C Hamano wrote:
> Alban Gruin writes:
>
> > The error comes from the call to `git stash apply $stash_id' in
> > builtin/rebase.c:261. When $stash_id only contains decimals and no
> > letters, git-stash tries to apply stash@{$stash_id}[0][1]. Thas was not
> > a real problem with the shell script, because it did not abbreviate the
> > object id of the stashed commit, so it was very unlikely that the oid
> > would contain only digits. builtin/rebase.c shortens the oid[2], making
> > this problem more likely to occur.
>
> OK, so make "rebase in C" a faithful conversion of the original, the
> code that lead to builtin/rebase.c:261 must be fixed not to pass a
> shortened oid. I suspect that internally it has a full object name,
> so not shortening would be the right thing anyway, so regaredless of
> this bug, it probably makes sense to do the fix.
>
> But as you said, even the scripted version that passed the full
> object name had a (10/16^40) chance of using a 40-hex that only has
> [0-9] and caused the same breakage, so such a change to "rebase in
> C" is sweeping the age old bug under the same rug, in the context of
> discussing this particular bug.
>
> I agree with you that it is a better fix to the problem to allow the
> caller to say "I am giving an oid---it may (or may not---I do not
> even bother to check) consist of only digits, but do not treat it as
> an index to the stash reflog."
We already have a way to say "I am giving you an oid": append `^0` to the
hash.
I implemented a fix, pushed it up to GitGitGadget, and will submit it
tomorrow (after a fresh look, and after the build finished):
https://github.com/gitgitgadget/git/pull/52
Ciao,
Dscho
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
Alban Gruin writes:
> The error comes from the call to `git stash apply $stash_id' in
> builtin/rebase.c:261. When $stash_id only contains decimals and no
> letters, git-stash tries to apply stash@{$stash_id}[0][1]. Thas was not
> a real problem with the shell script, because it did not abbreviate the
> object id of the stashed commit, so it was very unlikely that the oid
> would contain only digits. builtin/rebase.c shortens the oid[2], making
> this problem more likely to occur.
OK, so make "rebase in C" a faithful conversion of the original, the
code that lead to builtin/rebase.c:261 must be fixed not to pass a
shortened oid. I suspect that internally it has a full object name,
so not shortening would be the right thing anyway, so regaredless of
this bug, it probably makes sense to do the fix.
But as you said, even the scripted version that passed the full
object name had a (10/16^40) chance of using a 40-hex that only has
[0-9] and caused the same breakage, so such a change to "rebase in
C" is sweeping the age old bug under the same rug, in the context of
discussing this particular bug.
I agree with you that it is a better fix to the problem to allow the
caller to say "I am giving an oid---it may (or may not---I do not
even bother to check) consist of only digits, but do not treat it as
an index to the stash reflog."
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
Le 19/10/2018 à 20:05, Alban Gruin a écrit :
> Le 19/10/2018 à 14:46, SZEDER Gábor a écrit :
>> On Fri, Oct 19, 2018 at 03:02:22PM +0900, Junio C Hamano wrote:
>>> Two large set of topics on "rebase in C" and "rebase -i in C" are
>>> now in 'next'.
>>
>> I see occasional failures in 't5520-pull.sh':
>>
>> […]
>>
>> When running t5520 in a loop, it tends to fail between 10-40
>> iterations, even when the machine is not under heavy load.
>>
>> It appears that these failures started with commit 5541bd5b8f (rebase:
>> default to using the builtin rebase, 2018-08-08), i.e. tip of
>> 'pk/rebase-in-c-6-final', but it's a "flip the big switch" commit, so
>> not very useful...
>>
The bug can be bisected by prepending the command chain of 5520.25 with
`git config --bool rebase.usebuiltin true &&`. Commit 7debdaa4ad
(“builtin rebase: support `--autostash` option”) is the culprit, but
some commits that cause this test to fail with a different error (ie.
“TODO”…) must be marked as good.
The error comes from the call to `git stash apply $stash_id' in
builtin/rebase.c:261. When $stash_id only contains decimals and no
letters, git-stash tries to apply stash@{$stash_id}[0][1]. Thas was not
a real problem with the shell script, because it did not abbreviate the
object id of the stashed commit, so it was very unlikely that the oid
would contain only digits. builtin/rebase.c shortens the oid[2], making
this problem more likely to occur.
We could add a switch to git-stash to tell it that the parameter is an
oid, not anything else.
[0] https://github.com/git/git/blob/master/git-stash.sh#L514-L520
[1] https://github.com/git/git/blob/pu/builtin/stash.c#L167-L168
[2] https://github.com/git/git/blob/next/builtin/rebase.c#L1373
Cheers,
Alban
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
Hi, Le 19/10/2018 à 14:46, SZEDER Gábor a écrit : > On Fri, Oct 19, 2018 at 03:02:22PM +0900, Junio C Hamano wrote: >> Two large set of topics on "rebase in C" and "rebase -i in C" are >> now in 'next'. > > I see occasional failures in 't5520-pull.sh': > > […] > > When running t5520 in a loop, it tends to fail between 10-40 > iterations, even when the machine is not under heavy load. > > It appears that these failures started with commit 5541bd5b8f (rebase: > default to using the builtin rebase, 2018-08-08), i.e. tip of > 'pk/rebase-in-c-6-final', but it's a "flip the big switch" commit, so > not very useful... > I can reproduce this. I also tried to run this specific test under valgrind, and found out that some cases I did not targeted with --valgrind-only failed. The same thing happens with t3404, which did not crash with valgrind before. Here is a log: > expecting success: > HEAD=$(git rev-parse HEAD) && > set_fake_editor && > git rebase -i -p HEAD^ && > git update-index --refresh && > git diff-files --quiet && > git diff-index --quiet --cached HEAD -- && > test $HEAD = $(git rev-parse HEAD) > > +++ git rev-parse HEAD > ++ HEAD=d2d5ba71c6d0266f26238e804f77f026984ae0d9 > ++ set_fake_editor > ++ write_script fake-editor.sh > ++ echo '#!/bin/sh' > ++ cat > ++ chmod +x fake-editor.sh > +++ pwd > ++ test_set_editor '/tmp/git-alban/trash > directory.t3404-rebase-interactive/fake-editor.sh' > ++ FAKE_EDITOR='/tmp/git-alban/trash > directory.t3404-rebase-interactive/fake-editor.sh' > ++ export FAKE_EDITOR > ++ EDITOR='"$FAKE_EDITOR"' > ++ export EDITOR > ++ git rebase -i -p 'HEAD^' > GIT_DIR='/tmp/git-alban/trash directory.t3404-rebase-interactive/.git'; > state_dir='.git/rebase-merge'; upstream_name='HEAD^'; > upstream='8f99a4f1fbbd214b25a070ad34ec5a8f833522cc'; > head_name='refs/heads/branch1'; > orig_head='d2d5ba71c6d0266f26238e804f77f026984ae0d9'; > onto='8f99a4f1fbbd214b25a070ad34ec5a8f833522cc'; onto_name='HEAD^'; unset > revisions; unset restrict_revision; GIT_QUIET=''; git_am_opt=''; verbose=''; > diffstat=''; force_rebase=''; action=''; signoff=''; > allow_rerere_autoupdate=''; keep_empty=''; autosquash=''; unset gpg_sign_opt; > unset cmd; allow_empty_message='--allow-empty-message'; rebase_merges=''; > rebase_cousins=''; unset strategy; unset strategy_opts; rebase_root=''; > squash_onto=''; git_format_patch_opt=''; . git-sh-setup && . > git-rebase--common && . git-rebase--preserve-merges && > git_rebase__preserve_merges: line 0: .: git-rebase--common: file not found > error: last command exited with $?=1 > not ok 25 - -p handles "no changes" gracefully > # > # HEAD=$(git rev-parse HEAD) && > # set_fake_editor && > # git rebase -i -p HEAD^ && > # git update-index --refresh && > # git diff-files --quiet && > # git diff-index --quiet --cached HEAD -- && > # test $HEAD = $(git rev-parse HEAD) > # This comes from 't3404-rebase-interactive.sh', with --valgrind-only set to '63'. Cheers, Alban
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
Am 19.10.18 um 08:02 schrieb Junio C Hamano: * js/diff-notice-has-drive-prefix (2018-10-19) 1 commit - diff: don't attempt to strip prefix from absolute Windows paths "git diff /a/b/c /a/d/f" noticed these are full paths with shared leading prefix "/a", but failed to notice a similar fact about "git diff D:/a/b/c D:/a/d/f", which has been corrected. This patch isn't about a misdetected leading prefix, but about incorrectly truncated absolute paths. How about: Under certain circumstances, "git diff D:/a/b/c D:/a/b/d" on Windows would strip initial parts from the paths because they were not recognized as absolute, which has been corrected. Want tests. I've sent v2 with a test. -- Hannes
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
On 10/19/2018 2:02 AM, Junio C Hamano wrote: * ds/ci-commit-graph-and-midx (2018-10-19) 1 commit - ci: add optional test variables One of our CI tests to run with "unusual/experimental/random" settings now also uses commti-graph and midx. Will merge to 'next'. s/commti-graph/commit-graph/ * ds/test-multi-pack-index (2018-10-09) 3 commits - multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX - midx: close multi-pack-index on repack - midx: fix broken free() in close_midx() Tests for the recently introduced multi-pack index machinery. Expecting a reroll. cf. <8b5dbe3d-b382-bf48-b524-d9e8a074a...@gmail.com> A reroll exists: https://public-inbox.org/git/pull.27.v2.git.gitgitgad...@gmail.com/ Thanks, -Stolee
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
On Fri, Oct 19, 2018 at 03:02:22PM +0900, Junio C Hamano wrote:
> Two large set of topics on "rebase in C" and "rebase -i in C" are
> now in 'next'.
I see occasional failures in 't5520-pull.sh':
expecting success:
test_config rebase.autostash true &&
test_pull_autostash --rebase
+ test_config rebase.autostash true
+ config_dir=
+ test rebase.autostash = -C
+ test_when_finished test_unconfig 'rebase.autostash'
+ test 0 = 0
+ test_cleanup={ test_unconfig 'rebase.autostash'
} && (exit "$eval_ret"); eval_ret=$?; :
+ git config rebase.autostash true
+ test_pull_autostash --rebase
+ git reset --hard before-rebase
HEAD is now at 12212b3 new file
+ echo dirty
+ git add new_file
+ git pull --rebase . copy
>From .
* branchcopy -> FETCH_HEAD
Created autostash: 5417697
HEAD is now at 12212b3 new file
First, rewinding head to replay your work on top of it...
Applying: new file
Applying autostash resulted in conflicts.
Your changes are safe in the stash.
You can run "git stash pop" or "git stash drop" at any time.
+ test_cmp_rev HEAD^ copy
+ git rev-parse --verify HEAD^
+ git rev-parse --verify copy
+ test_cmp expect.rev actual.rev
+ diff -u expect.rev actual.rev
+ cat new_file
cat: new_file: No such file or directory
+ test = dirty
error: last command exited with $?=1
not ok 25 - pull --rebase succeeds with dirty working directory and
rebase.autostash set
When running t5520 in a loop, it tends to fail between 10-40
iterations, even when the machine is not under heavy load.
It appears that these failures started with commit 5541bd5b8f (rebase:
default to using the builtin rebase, 2018-08-08), i.e. tip of
'pk/rebase-in-c-6-final', but it's a "flip the big switch" commit, so
not very useful...
What's cooking in git.git (Oct 2018, #04; Fri, 19)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. Two large set of topics on "rebase in C" and "rebase -i in C" are now in 'next'. A handful of improvements around "git help", "git grep", and "git status" are in 'master', as well as other internal changes. Note that "cf. " are not meant as "clear all of these and you are home free". They are primarily to prevent me from merging topics that are not ready to 'next' by mistake and remind me where to go back to read the last state of the discussion in the archive. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * bp/read-cache-parallel (2018-10-11) 7 commits (merged to 'next' on 2018-10-12 at ed6edde799) + read-cache: load cache entries on worker threads + ieot: add Index Entry Offset Table (IEOT) extension + read-cache: load cache extensions on a worker thread + config: add new index.threads config setting + eoie: add End of Index Entry (EOIE) extension + read-cache: clean up casting and byte decoding + read-cache.c: optimize reading index format v4 A new extension to the index file has been introduced, which allows the file to be read in parallel. * bp/rename-test-env-var (2018-09-28) 6 commits (merged to 'next' on 2018-10-12 at 201e451d20) + t: do not get self-test disrupted by environment warnings + preload-index: update GIT_FORCE_PRELOAD_TEST support + read-cache: update TEST_GIT_INDEX_VERSION support + fsmonitor: update GIT_TEST_FSMONITOR support + preload-index: use git_env_bool() not getenv() for customization + t/README: correct spelling of "uncommon" Some environment variables that control the runtime options of Git used during tests are getting renamed for consistency. * ds/commit-graph-leakfix (2018-10-07) 3 commits (merged to 'next' on 2018-10-12 at 8cc7f2f1e9) + commit-graph: reduce initial oid allocation + builtin/commit-graph.c: UNLEAK variables + commit-graph: clean up leaked memory during write Code clean-up. * jc/how-to-document-api (2018-09-29) 1 commit (merged to 'next' on 2018-10-12 at 7c9bd82285) + CodingGuidelines: document the API in *.h files Doc update. * jt/avoid-ls-refs (2018-10-07) 4 commits (merged to 'next' on 2018-10-12 at 5775aabbc1) + fetch: do not list refs if fetching only hashes + transport: list refs before fetch if necessary + transport: do not list refs if possible + transport: allow skipping of ref listing Over some transports, fetching objects with an exact commit object name can be done without first seeing the ref advertisements. The code has been optimized to exploit this. * jt/cache-tree-allow-missing-object-in-partial-clone (2018-10-10) 1 commit (merged to 'next' on 2018-10-12 at 152ad8e336) + cache-tree: skip some blob checks in partial clone In a partial clone that will lazily be hydrated from the originating repository, we generally want to avoid "does this object exist (locally)?" on objects that we deliberately omitted when we created the clone. The cache-tree codepath (which is used to write a tree object out of the index) however insisted that the object exists, even for paths that are outside of the partial checkout area. The code has been updated to avoid such a check. * jt/fetch-tips-in-partial-clone (2018-09-21) 2 commits (merged to 'next' on 2018-10-12 at 521b3fb44d) + fetch: in partial clone, check presence of targets + connected: document connectivity in partial clones "git fetch $repo $object" in a partial clone did not correctly fetch the asked-for object that is referenced by an object in promisor packfile, which has been fixed. * jt/non-blob-lazy-fetch (2018-10-04) 2 commits (merged to 'next' on 2018-10-12 at 7466c6bd7d) + fetch-pack: exclude blobs when lazy-fetching trees + fetch-pack: avoid object flags if no_dependents A partial clone that is configured to lazily fetch missing objects will on-demand issue a "git fetch" request to the originating repository to fill not-yet-obtained objects. The request has been optimized for requesting a tree object (and not the leaf blob objects contained in it) by telling the originating repository that no blobs are needed. * nd/complete-fetch-multiple-args (2018-09-21) 1 commit (merged to 'next' on 2018-10-10 at f78e14123c) + completion: support "git fetch --multiple" Teach bash completion that "git fetch --multiple" only takes remote names as arguments and no refspecs. * nd/help-commands-verbose-by-default (2018-10-03) 1 commit (merged to 'next' on 2018-10-12 at 32de8f53e0) + help -a: improve and make --verbose default "git help -a" and "git
