Re: [QGit PATCH] Remove most ASSERT warnings in Git::setStatus

Re: [QGit PATCH] Remove most ASSERT warnings in Git::setStatus

[Marco Costalba]

On 5/20/07, Michael <barra_cuda@katamail.com> wrote:
> Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
> ---
>
> ...is this correct?
>
>  src/git_startup.cpp |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/src/git_startup.cpp b/src/git_startup.cpp
> index a99edba..17312f9 100644
> --- a/src/git_startup.cpp
> +++ b/src/git_startup.cpp
> @@ -329,7 +329,7 @@ void Git::parseDiffFormatLine(RevFile& rf, SCRef line, int parNum) {
>
>                 // TODO rename/copy is not supported for combined merges
>                 appendFileName(rf, line.section('\t', -1));
> -               setStatus(rf, line.section(' ', 6, 6).left(1));
> +               setStatus(rf, line.section('\t', -2, -2).right(1));
>                 rf.mergeParent.append(parNum);
>         } else { // faster parsing in normal case
>
> --
> 1.5.1.2
>

Where do you see the ASSERT? could  you link me to the test repository
when you see that warnings?

I would like to understand better what causes the warnings.

Thanks
Marco

Re: [QGit PATCH] Remove most ASSERT warnings in Git::setStatus

[Michael]

(sorry, dropped git ml)

"Marco Costalba" <mcostalba@gmail.com>:
> On 5/20/07, Michael <barra_cuda@katamail.com> wrote:
> > Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
> > ---
> >
> > ...is this correct?
> >
> >  src/git_startup.cpp |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/src/git_startup.cpp b/src/git_startup.cpp
> > index a99edba..17312f9 100644
> > --- a/src/git_startup.cpp
> > +++ b/src/git_startup.cpp
> > @@ -329,7 +329,7 @@ void Git::parseDiffFormatLine(RevFile& rf, SCRef line, int parNum) {
> >
> >                 // TODO rename/copy is not supported for combined merges
> >                 appendFileName(rf, line.section('\t', -1));
> > -               setStatus(rf, line.section(' ', 6, 6).left(1));
> > +               setStatus(rf, line.section('\t', -2, -2).right(1));
> >                 rf.mergeParent.append(parNum);
> >         } else { // faster parsing in normal case
> >
> > --
> > 1.5.1.2
> >
> 
> Where do you see the ASSERT? could  you link me to the test repository
> when you see that warnings?
> 
> I would like to understand better what causes the warnings.

On the git repo:

rm .git/qgit_cache.dat
qgit origin/pu
then use arrows to go down to the first octopus merge, ie:

    Merge branches 'jc/blame' and 'jc/diff' into pu

qgit prints:

ASSERT in Git::setStatus, unknown status <1>. 'MODIFIED' will be used instead.
ASSERT in Git::setStatus, unknown status <c>. 'MODIFIED' will be used instead.

on stderr.

That's because the lines considered in Git::setStatus are:

:::100644 100644 100644 100644 35864ed3c4afe01680bd5123fc28c35f5cf328e6 29243c6e8b49958ddcb08df0eb4223b14fd3e19f 16a5b9ac49c756492a7fd91fa49b84a3aee1f6b2 77ca8dcdfbcd6667b7f511306347c0d245ee4e2b MMM  Makefile
:::100644 100644 100644 100644 0e6439b0ddaf317a6288ab4dd40ae8b9a41e9884 4204bc168c11fc7f8764e7d92e5935d2dc30c3bd cbab8ebecb4f3f13856b3be21409074dbcd3edda d1ff7f50a31d9647c26c1e2293957a1d719c9373 MMM  cache.h

...which obviously do not have the right information ("M") in

        line.section(' ', 6, 6).left(1)

but in

        line.section('\t', -2, -2).right(1)

I thought this would trigger even with .git/qgit_cache.dat,
but I was wrong. (Anyway, I have a patch that adds an option
to make .git/qgit_cache.dat optional, but I don't think it
could be that useful. What do you think?)

If it still doesn't trigger for you, maybe I should post you
my qgitrc...

Re: [QGit PATCH] Remove most ASSERT warnings in Git::setStatus

[Marco Costalba]

On 5/20/07, Michael <barra_cuda@katamail.com> wrote:
> "Marco Costalba" <mcostalba@gmail.com>:
> > On 5/20/07, Michael <barra_cuda@katamail.com> wrote:
> > > Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
> > > ---
> > >
> > > ...is this correct?
> > >
> > >  src/git_startup.cpp |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/src/git_startup.cpp b/src/git_startup.cpp
> > > index a99edba..17312f9 100644
> > > --- a/src/git_startup.cpp
> > > +++ b/src/git_startup.cpp
> > > @@ -329,7 +329,7 @@ void Git::parseDiffFormatLine(RevFile& rf, SCRef line, int parNum) {
> > >
> > >                 // TODO rename/copy is not supported for combined merges
> > >                 appendFileName(rf, line.section('\t', -1));
> > > -               setStatus(rf, line.section(' ', 6, 6).left(1));
> > > +               setStatus(rf, line.section('\t', -2, -2).right(1));
> > >                 rf.mergeParent.append(parNum);
> > >         } else { // faster parsing in normal case
> > >

>From diff-format.txt we have that an output line is formatted this way:

------------------------------------------------
in-place edit  :100644 100644 bcd1234... 0123456... M file0
copy-edit      :100644 100644 abcd123... 1234567... C68 file1 file2
rename-edit    :100644 100644 abcd123... 1234567... R86 file1 file3
create         :000000 100644 0000000... 1234567... A file4
delete         :100644 000000 1234567... 0000000... D file5
unmerged       :000000 000000 0000000... 0000000... U file6
------------------------------------------------

That is, from the left to the right:

. a colon.
. mode for "src"; 000000 if creation or unmerged.
. a space.
. mode for "dst"; 000000 if deletion or unmerged.
. a space.
. sha1 for "src"; 0\{40\} if creation or unmerged.
. a space.
. sha1 for "dst"; 0\{40\} if creation, unmerged or "look at work tree".
. a space.
. status, followed by optional "score" number.
. a tab or a NUL when '-z' option is used.
. path for "src"
. a tab or a NUL when '-z' option is used; only exists for C or R.
. path for "dst"; only exists for C or R.
. an LF or a NUL when '-z' option is used, to terminate the record.

So I rather would prefer something like

  line.section('\t', 0, 0).section(' ', -1, -1).left(1)

because we could have more then one file separated by a tab, so

 line.section('\t', -2, -2).right(1)

it seems to me a little bit fragile. What do you think?

Also I don't understand why you consider the right most (right(1)),
instead of the left most character as the status.


Thanks
Marco

Re: [QGit PATCH] Remove most ASSERT warnings in Git::setStatus

[Michael]

"Marco Costalba" <mcostalba@gmail.com>:
> So I rather would prefer something like
> 
>   line.section('\t', 0, 0).section(' ', -1, -1).left(1)
> 
> because we could have more then one file separated by a tab, so
> 
>  line.section('\t', -2, -2).right(1)
> 
> it seems to me a little bit fragile. What do you think?

You're right.

> Also I don't understand why you consider the right most (right(1)),
> instead of the left most character as the status.

Only because it was simpler AND because I didn't know it was wrong.

Re: [QGit PATCH] Remove most ASSERT warnings in Git::setStatus

[Marco Costalba]

On 5/20/07, Michael <barra_cuda@katamail.com> wrote:
> "Marco Costalba" <mcostalba@gmail.com>:
> > So I rather would prefer something like
> >
> > line.section('\t', 0, 0).section(' ', -1, -1).left(1)
> >
> > because we could have more then one file separated by a tab, so
> >
> > line.section('\t', -2, -2).right(1)
> >
> > it seems to me a little bit fragile. What do you think?
>
> You're right.
>
> > Also I don't understand why you consider the right most (right(1)),
> > instead of the left most character as the status.
>
> Only because it was simpler AND because I didn't know it was wrong.
>

Patch pushed to public repo both for qgit and qgit4.


Grazie per il report
Marco