| Message ID | pull.949.v5.git.1620747320947.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,GSOC] ref-filter: fix read invalid union member bug | expand |
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: ZheNing Hu <adlternative@gmail.com> > > used_atom.u is an union, and it has different members depending on > what atom the auxiliary data the union part of the "struct > used_atom" wants to record. At most only one of the members can be > valid at any one time. Since the code checks u.remote_ref without > even making sure if the atom is "push" or "push:" (which are only > two cases that u.remote_ref.push becomes valid), but u.remote_ref > shares the same storage for other members of the union, the check > was reading from an invalid member, which was the bug. > > Modify the condition here to check whether the atom name > equals to "push" or starts with "push:", to avoid reading the > value of invalid member of the union. > > Helped-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > --- Thanks. > diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh > index 9866b1b57368..309cf699506f 100755 > --- a/t/t6302-for-each-ref-filter.sh > +++ b/t/t6302-for-each-ref-filter.sh > @@ -117,6 +117,24 @@ test_expect_success '%(color) must fail' ' > test_must_fail git for-each-ref --format="%(color)%(refname)" > ' > > +test_expect_success '%(color:#aa22ac) must successed' ' "succeed". > + test_when_finished "cd .. && rm -rf ./test" && Not a very good practice to chdir around, even if you have when-finished clean-up. Not worth risking "rm -rf" at random places when for example somebody breaks when-finished. Instead... > + mkdir test && Place everything below ... > + cd test && > + git init && > + cat >expect <<-\EOF && > + refs/heads/main > + EOF > + git add . && > + git branch -M main && This is in a freshly created repository without commit. Does "branch -M" work for such an unborn branch? Perhaps start this block like so: git init test && ( cd test && test_commit initial && git branch -M main && cat >expect <<-\EOF && refs/heads/main refs/tags/initial EOF git remote add origin nowhere && ... > + 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)%(refname)" >actual && > + test_cmp expect actual ... up to here inside a (subshell). By the way, your use of "git branch -M" makes the test work whether the default initial branch name is still 'master' or already 'main' by forcing the branch used for testing to be 'main'. Clever ;-). Will queue with all of the above suggestions squashed in; please see if the result is good when it is pushed out later today. Thanks. > +' > + > test_expect_success 'left alignment is default' ' > cat >expect <<-\EOF && > refname is refs/heads/main |refs/heads/main > > base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a
> > + test_when_finished "cd .. && rm -rf ./test" && > > Not a very good practice to chdir around, even if you have > when-finished clean-up. Not worth risking "rm -rf" at random > places when for example somebody breaks when-finished. > OK. > Instead... > > > + mkdir test && > > Place everything below ... > > > + cd test && > > + git init && > > + cat >expect <<-\EOF && > > + refs/heads/main > > + EOF > > + git add . && > > + git branch -M main && > > This is in a freshly created repository without commit. Does > "branch -M" work for such an unborn branch? Perhaps start this > block like so: > I think "git branch -M" before "git commit" also will be fine. Of course it's okay to put it after commit and before "test_cmp". > git init test && > ( > cd test && > test_commit initial && > git branch -M main && > cat >expect <<-\EOF && > refs/heads/main > refs/tags/initial > EOF > git remote add origin nowhere && > ... > > > + 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)%(refname)" >actual && > > + test_cmp expect actual > > ... up to here inside a (subshell). > I get it now. By executing cd in the subshell, the "cd .." step can be omitted. > By the way, your use of "git branch -M" makes the test work whether > the default initial branch name is still 'master' or already 'main' > by forcing the branch used for testing to be 'main'. Clever ;-). > Hh, real knowledge comes from practice. Yesterday, I tried it on the old version of git on some classmates’ computers, and there was an error, so using "git branch -M main" can avoid this error. > Will queue with all of the above suggestions squashed in; please see > if the result is good when it is pushed out later today. > > Thanks. > Thanks! -- ZheNing Hu
diff --git a/ref-filter.c b/ref-filter.c index a0adb4551d87..213d3773ada3 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) else v->s = xstrdup(""); continue; - } else if (atom->u.remote_ref.push) { + } else if (!strcmp(atom->name, "push") || starts_with(atom->name, "push:")) { const char *branch_name; v->s = xstrdup(""); if (!skip_prefix(ref->refname, "refs/heads/", diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh index 9866b1b57368..309cf699506f 100755 --- a/t/t6302-for-each-ref-filter.sh +++ b/t/t6302-for-each-ref-filter.sh @@ -117,6 +117,24 @@ test_expect_success '%(color) must fail' ' test_must_fail git for-each-ref --format="%(color)%(refname)" ' +test_expect_success '%(color:#aa22ac) must successed' ' + test_when_finished "cd .. && rm -rf ./test" && + mkdir test && + cd test && + git init && + cat >expect <<-\EOF && + refs/heads/main + EOF + 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)%(refname)" >actual && + test_cmp expect actual +' + test_expect_success 'left alignment is default' ' cat >expect <<-\EOF && refname is refs/heads/main |refs/heads/main