Re: [PATCH v4] [GSOC] ref-filter: fix read invalid union member bug

ZheNing Hu [off-list ref] writes:

quoted

Well, the thing is,

    $ git checkout 49f38e2d ;# (The fifteenth batch, 2021-05-10)
    $ git am -s mbox
    $ git show --stat --oneline
    39509d100a (HEAD) ref-filter: fix read invalid union member bug
     ref-filter.c                   |  2 +-
     t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++
     2 files changed, 19 insertions(+), 1 deletion(-)
    $ git show ref-filter.c | git apply -R ;# revert only the fix
    $ make -j32 && make -C t T=t6302-*.sh

does not seem to break anything.  Perhaps there is something more
than the "17th byte" thing (like structure padding that may vary
depending on the compiler and architecture)?

Fine, I guess the reason for this mystery is I "push" this branch to github
and you haven't done it. That may not be due to the platform. Because I
can see no this bug happening when I use a new git repo without "git push",
and I test in archlinux or deepin, this bug will happen in these environments.

Sorry, you lost me.  I was talking about what happens in the new
test you added to t6302 not failing as designed, and there shouldn't
be "I've pushed but you haven't pushed to GitHub" distinction.  The
test is running in a brand-new repository just created for the sole
purpose of running the test after all.

#!/bin/sh
mkdir test
cd test
git init
echo 1>1
git add .
git branch -M main
git commit -m "test"
git remote add origin nowhere
git config branch.main.remote origin
git config branch.main.merge refs/heads/main
git for-each-ref --format="%(color:#aa22ac)%(objectname)"

These two "git config" is for simulating a push environment.

So in short, the test script added to t6302 in the v4 patch was not
testing what it was supposed to be testing, as it didn't have the
configuration items related to %(push) atom necessary to trigger the
error?  

That I can believe.  I was starting to worry if there was something
more subtle going on, but I am glad that it was only an uncooked
patch submitted without checking.

I guess you also saw this bug:

BUG: ref-filter.c:1544: unhandled RR_* enum

No, I didn't.  I just tried to make sure the new test was truly
checking the existing breakage by partially reverting the code fix,
and saw that the new test did not fail.

Thanks.

quoted

Fine, I guess the reason for this mystery is I "push" this branch to github
and you haven't done it. That may not be due to the platform. Because I
can see no this bug happening when I use a new git repo without "git push",
and I test in archlinux or deepin, this bug will happen in these environments.

Sorry, you lost me.  I was talking about what happens in the new
test you added to t6302 not failing as designed, and there shouldn't
be "I've pushed but you haven't pushed to GitHub" distinction.  The
test is running in a brand-new repository just created for the sole
purpose of running the test after all.

Well, there are some expression problems here, I just want to express:
This bug is only triggered after I push and Git adds some config during
the push process. And then those config effort %(push) behavior.

So in short, the test script added to t6302 in the v4 patch was not
testing what it was supposed to be testing, as it didn't have the
configuration items related to %(push) atom necessary to trigger the
error?

Truly.

That I can believe.  I was starting to worry if there was something
more subtle going on, but I am glad that it was only an uncooked
patch submitted without checking.

Me too, I want to complain a little bit: this bug is too difficult to locate.

quoted

I guess you also saw this bug:

BUG: ref-filter.c:1544: unhandled RR_* enum

No, I didn't.  I just tried to make sure the new test was truly
checking the existing breakage by partially reverting the code fix,
and saw that the new test did not fail.

I understand.

Thanks.

Thanks.
--
ZheNing Hu