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...
