threads / patch / 8242

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

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

## tl;dr

5 messages between May 20, 2007 and May 20, 2007. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Marco Costalba· May 20, 2007, 12:53 UTC · lore
On 5/20/07, Michael <barra_cuda@katamail.com> wrote:
Show 24 quoted lines
> 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

Michael· May 20, 2007, 14:08 UTC · re: Marco Costalba · lore
(sorry, dropped git ml)
"Marco Costalba" <mcostalba@gmail.com>:
Show 30 quoted lines
> 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...

Marco Costalba· May 20, 2007, 14:23 UTC · lore
On 5/20/07, Michael <barra_cuda@katamail.com> wrote:
Show 23 quoted lines
> "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

Michael· May 20, 2007, 14:41 UTC · re: Marco Costalba · lore
"Marco Costalba" <mcostalba@gmail.com>:
Show 9 quoted lines
> 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.
Marco Costalba· May 20, 2007, 16:06 UTC · re: Michael · lore
On 5/20/07, Michael <barra_cuda@katamail.com> wrote:
Show 18 quoted lines
> "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

← back to recent threads