[PATCH QGit RFC] Fix "Save patch..." on a commit range

[PATCH QGit RFC] Fix "Save patch..." on a commit range

[Markus Heidelberg]

Creating a patch series didn't work, because the SHA1 list was
interpreted in the wrong order.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---

To find out if this problem was caused by a regression, I tested
2e63608 (Format patch: use selected patches as a range, 2008-01-13),
which changed the behavior of generating multiple patches and introduced
the lines that are now changed in my patch.

But that commit didn't work for me either. Now I don't think that it
didn't work for you at that time. Can this be a Qt problem?

 src/git.cpp |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/git.cpp b/src/git.cpp
index a20bf0d..0f18f61 100644
--- a/src/git.cpp
+++ b/src/git.cpp
@@ -1577,9 +1577,9 @@ bool Git::formatPatch(SCList shaList, SCRef dirPath, SCRef remoteDir) {
 	if (remote)
 		workDir = remoteDir; // run() uses workDir value
 
-	// shaList is ordered by newest to oldest
-	runCmd.append(" " + shaList.last());
-	runCmd.append(QString::fromLatin1("^..") + shaList.first());
+	// shaList is ordered by oldest to newest
+	runCmd.append(" " + shaList.first());
+	runCmd.append(QString::fromLatin1("^..") + shaList.last());
 	bool ret = run(runCmd);
 	workDir = tmp;
 	return ret;
-- 
1.6.3.2.248.g8cb59

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Marco Costalba]

On Sat, Jun 13, 2009 at 00:29, Markus
Heidelberg<markus.heidelberg@web.de> wrote:
> Creating a patch series didn't work, because the SHA1 list was
> interpreted in the wrong order.
>
> Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
> ---

I think they are generated in apply order as should be.

As example, from git repo we have the following revisions:

GIT 1.6.3
t4029: use sh instead of bash
t4200: convert sed expression which operates on non-text file to perl
t4200: remove two unnecessary lines

Now if I select the 4 revisions and use "Save patch..." I have

0001-t4200-remove-two-unnecessary-lines.patch
0002-t4200-convert-sed-expression-which-operates-on-non.patch
0003-t4029-use-sh-instead-of-bash.patch
0004-GIT-1.6.3.patch

That is correct because if I go to apply the patches I have to apply
in the reverse cronological order, from the oldest to the newest.

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Markus Heidelberg]

Marco Costalba, 13.06.2009:
> On Sat, Jun 13, 2009 at 00:29, Markus
> Heidelberg<markus.heidelberg@web.de> wrote:
> > Creating a patch series didn't work, because the SHA1 list was
> > interpreted in the wrong order.
> >
> > Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
> > ---
> 
> I think they are generated in apply order as should be.
> 
> As example, from git repo we have the following revisions:
> 
> GIT 1.6.3
> t4029: use sh instead of bash
> t4200: convert sed expression which operates on non-text file to perl
> t4200: remove two unnecessary lines
> 
> Now if I select the 4 revisions and use "Save patch..." I have
> 
> 0001-t4200-remove-two-unnecessary-lines.patch
> 0002-t4200-convert-sed-expression-which-operates-on-non.patch
> 0003-t4029-use-sh-instead-of-bash.patch
> 0004-GIT-1.6.3.patch
> 
> That is correct because if I go to apply the patches I have to apply
> in the reverse cronological order, from the oldest to the newest.

Sorry, I didn't describe the problem proper. The problem was not about
the order of the created patches - it's that I can't create multiple
patches at all.

If I only select 1 revision and use "Save patch..." I get
0001-xyz.patch, which is correct.

But if I select 2 or more revisions, I don't get any patch file.

For example, with the following DAG:

    A---B---C---D---E

If I select B to D and use "Save patch...", Qgit creates the following
command:

    git format-patch -o /home/markus D^..B

but D^..B obviously is empty.

My patch now changes it, so that this command is produced:

    git format-patch -o /home/markus B^..D

which generates 3 patches.


OK, that was the state from yesterday. While testing a bit more, I can
notice some really strange behavior:

I managed to get the following shaList in Git::formatPatch(): D, B, C
which led to this revision range in the command: C^..D
which produced 2 patches.

After several tests it seems as if the revisions C and D are always the
first and last of the shaList variable, if they are included in the
selection. Selecting only the first and last revision with
Ctrl+LeftMouse instead of all including the ones in between with
Shift+LeftMouse works.

Hmm, I will look some more into it...

Markus

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Markus Heidelberg]

Marco Costalba, 13.06.2009:
> On Sat, Jun 13, 2009 at 00:29, Markus
> Heidelberg<markus.heidelberg@web.de> wrote:
> > Creating a patch series didn't work, because the SHA1 list was
> > interpreted in the wrong order.
> >
> > Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
> > ---
> 
> I think they are generated in apply order as should be.
> 
> As example, from git repo we have the following revisions:
> 
> GIT 1.6.3
> t4029: use sh instead of bash
> t4200: convert sed expression which operates on non-text file to perl
> t4200: remove two unnecessary lines
> 
> Now if I select the 4 revisions and use "Save patch..." I have
> 
> 0001-t4200-remove-two-unnecessary-lines.patch
> 0002-t4200-convert-sed-expression-which-operates-on-non.patch
> 0003-t4029-use-sh-instead-of-bash.patch
> 0004-GIT-1.6.3.patch

In my git repository with some local branches I get this:

0001-t4200-convert-sed-expression-which-operates-on-non-t.patch
0002-t4029-use-sh-instead-of-bash.patch

In a plain newly created git repo with this command:

git clone --reference git git://git.kernel.org/pub/scm/git/git.git git-orig

I don't get any patches. I haven't tried it explicitely now, but I guess
my patch would solve this for me.

OK, if I now disable "All branches" in "View->Select range...", I get
the 2 patches from above again.
But in my git repo with local branches, it's the other way round...

Markus

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Marco Costalba]

On Sat, Jun 13, 2009 at 12:02, Markus
Heidelberg<markus.heidelberg@web.de> wrote:
>
> OK, if I now disable "All branches" in "View->Select range...", I get
> the 2 patches from above again.
> But in my git repo with local branches, it's the other way round...
>
> Markus
>

Ok. The point is that if I select 4 _consecutive_ revisions from any
repo and do "Save as..." then I see the 4 patches created in reverse
cronoligical order as it should be.

If I select only two patches _non_ consecutives and I do "Save as..."
I get the two patches + all the pacthes in between still in reverse
cronological order.

This is with stock QGit 2.3, Windows version.

Have you some problem to reproduce this behavior (that is intended to
be the correct one BTW) ?

Thanks
Marco

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Markus Heidelberg]

Marco Costalba, 13.06.2009:
> On Sat, Jun 13, 2009 at 12:02, Markus
> Heidelberg<markus.heidelberg@web.de> wrote:
> >
> > OK, if I now disable "All branches" in "View->Select range...", I get
> > the 2 patches from above again.
> > But in my git repo with local branches, it's the other way round...
> >
> > Markus
> >
> 
> Ok. The point is that if I select 4 _consecutive_ revisions from any
> repo and do "Save as..." then I see the 4 patches created in reverse
> cronoligical order as it should be.

I did the same as you: selected the 4 consecutive revisions. But I
didn't get 4 patches. Dependent on the state of the "All branches"
checkbox of the "Range select", I get 2 or 0 patches. The QStringList
shaList is set wrong for me.

> If I select only two patches _non_ consecutives and I do "Save as..."
> I get the two patches + all the pacthes in between still in reverse
> cronological order.

Yes, I'm aware of that. It shouldn't make a difference if I only select
start end end revision or if I select all revisions including the
revisions between start and end.
And as I said earlier: the order of the patches is not my problem.

> This is with stock QGit 2.3, Windows version.

I tested with the latest qgit.git and with QGit 2.2 from my
distribution. All on Gentoo Linux.

I just built QGit 2.3, this has the same problems.

> Have you some problem to reproduce this behavior (that is intended to
> be the correct one BTW) ?

Yes, I have :)

Can you please try, if enabling/disabling the "All branches" checkbox
makes a difference for you?

I have few time this weekend to respond. Have to play a concert with my
band today and clean my bike for a triathlon tomorrow. I will try to
reproduce on Windows, probably after this weekend.

Thanks, Markus

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Marco Costalba]

On Sat, Jun 13, 2009 at 12:33, Markus
Heidelberg<markus.heidelberg@web.de> wrote:
>
> I have few time this weekend to respond. Have to play a concert with my
> band today and clean my bike for a triathlon tomorrow. I will try to
> reproduce on Windows, probably after this weekend.
>

All your patches but the format-patch one have been applied and pushed.

I will investigate further on the format-patch one....

Thanks
Marco

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Markus Heidelberg]

Markus Heidelberg, 13.06.2009:
> 
> I tested with the latest qgit.git and with QGit 2.2 from my
> distribution. All on Gentoo Linux.
> 
> I just built QGit 2.3, this has the same problems.
> 
> [...]
> 
> I will try to reproduce on Windows, probably after this weekend.

Some news, tested with the 4 commits in git.git around v1.6.3 as before:

It works correctly with QGit-2.3 installed from the Windows installer
(http://sourceforge.net/projects/qgit).

It works reversed when using a self-compiled qgit.
gcc: mingw-gcc 3.4.5
Qt: 4.4.1

Reversed means, git-format-patch is invoked like this:
    git format-patch v1.6.3..v1.6.3~3^
and doesn't produce patch files, instead of:
    git format-patch v1.6.3~3^..v1.6.3

The strange behaviour of getting only 2 patches on Linux, I couldn't
reproduce on Windows.

You use MSVC, don't you? Can you please try with mingw? It would be
really helpful, if you could reproduce it.

Markus

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Marco Costalba]

On Mon, Jun 15, 2009 at 22:13, Markus
Heidelberg<markus.heidelberg@web.de> wrote:
> Markus Heidelberg, 13.06.2009:
>>
>> I tested with the latest qgit.git and with QGit 2.2 from my
>> distribution. All on Gentoo Linux.
>>
>> I just built QGit 2.3, this has the same problems.
>>
>> [...]
>>
>> I will try to reproduce on Windows, probably after this weekend.
>
> Some news, tested with the 4 commits in git.git around v1.6.3 as before:
>
> It works correctly with QGit-2.3 installed from the Windows installer
> (http://sourceforge.net/projects/qgit).
>
> It works reversed when using a self-compiled qgit.
> gcc: mingw-gcc 3.4.5
> Qt: 4.4.1
>
> Reversed means, git-format-patch is invoked like this:
>    git format-patch v1.6.3..v1.6.3~3^
> and doesn't produce patch files, instead of:
>    git format-patch v1.6.3~3^..v1.6.3
>
> The strange behaviour of getting only 2 patches on Linux, I couldn't
> reproduce on Windows.
>
> You use MSVC, don't you? Can you please try with mingw? It would be
> really helpful, if you could reproduce it.
>
> Markus
>
>

This is really strange !

Yes I use MSVC, I cannot try mingw-gcc because I should recompile all
Qt with gcc.

But I can test on Linux tomorrow evening and see what's happens
there...but there is _nothing_ in the code that is platform dependent,
see Git::formatPatch() in git.cpp

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Marco Costalba]

On Mon, Jun 15, 2009 at 22:25, Marco Costalba<mcostalba@gmail.com> wrote:
>
> This is really strange !
>

Ok. This is a quick test that perhaps you could do.

Git::formatPatch() get the list of selected revisions already ordered
by its caller, MainImpl::ActMailFormatPatch_activated() in
mainimpl.cpp, in this function the selected items are retrieved
calling ListView::getSelectedItems() in listview.cpp

There, finally, there is the call to the native Qt function that
collects the selected rows, QItemSelectionModel::selectedRows()

The returned list is takes as is by QGit and nevere reordered or
touched, so the order of the revisions belong directly on how
QItemSelectionModel::selectedRows() returns the rows.

Now the published windows version (and also teh Linux one) is compiled
against Qt4.3.3, while you are using Qt4.4.1


I have checked in the Qt documentation and I didn't found any point
where the order of the returned rows is specified, I know in Qt4.3.3
is from the top toward the bottom of the list, but perhaps in Qt4.4.1
it has been changed.

So you could check, in ListView::getSelectedItems() how the rows are returned.

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Marco Costalba]

[-- Attachment #1: Type: text/plain, Size: 312 bytes --]

On Mon, Jun 15, 2009 at 22:45, Marco Costalba<mcostalba@gmail.com> wrote:
> On Mon, Jun 15, 2009 at 22:25, Marco Costalba<mcostalba@gmail.com> wrote:
>>
>> This is really strange !
>>
>

I have prepared a possible patch, could you please test if fixes the
problem for you.

Patch is in attachment.

Thanks
Marco

[-- Attachment #2: 0001-Fix-format-patch-bad-selected-revision-order.patch --]
[-- Type: text/x-diff, Size: 1623 bytes --]

From 8e4ac4bf9b6c04b09e16a2a49b96f6bf4c75d2e7 Mon Sep 17 00:00:00 2001
From: Marco Costalba <mcostalba@gmail.com>
Date: Mon, 15 Jun 2009 23:01:42 +0100
Subject: [PATCH] Fix format patch bad selected revision order

We cannot trust the order of selected items returned
by QItemSelectionModel::selectedRows() that is used
to call git format-patch.

It is not documented and could change with a new version
of Qt libraries.

So manually reorder the selected revisions before to feed
git format-patch.

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 src/git.cpp |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/git.cpp b/src/git.cpp
index 8546f6f..d55d1c2 100644
--- a/src/git.cpp
+++ b/src/git.cpp
@@ -1577,9 +1577,18 @@ bool Git::formatPatch(SCList shaList, SCRef dirPath, SCRef remoteDir) {
 	if (remote)
 		workDir = remoteDir; // run() uses workDir value
 
-	// shaList is ordered by newest to oldest
-	runCmd.append(" " + shaList.last());
-	runCmd.append(QString::fromLatin1("^..") + shaList.first());
+	// Don't trust shaList order but reorder from newest to oldest
+	QStringList orderedShaList;
+	FOREACH_SL (it, shaList)
+		appendNamesWithId(orderedShaList, *it, QStringList(*it), true);
+
+	orderedShaList.sort();
+	QStringList::iterator itN(orderedShaList.begin());
+	for ( ; itN != orderedShaList.end(); ++itN) // strip 'idx'
+		(*itN) = (*itN).section(' ', -1, -1);
+
+	runCmd.append(" " + orderedShaList.last());
+	runCmd.append(QString::fromLatin1("^..") + orderedShaList.first());
 	bool ret = run(runCmd);
 	workDir = tmp;
 	return ret;
-- 
1.6.1.9.g97c34

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Markus Heidelberg]

Marco Costalba, 15.06.2009:
> So you could check, in ListView::getSelectedItems() how the rows are returned.

I added this snippet:

-->8--

diff --git a/src/listview.cpp b/src/listview.cpp
index b2f4915..5bd1ee6 100644
--- a/src/listview.cpp
+++ b/src/listview.cpp
@@ -171,10 +171,12 @@ void ListView::showIdValues() {
 	viewport()->update();
 }
 
+#include <QtDebug>
 void ListView::getSelectedItems(QStringList& selectedItems) {
 
 	selectedItems.clear();
 	QModelIndexList ml = selectionModel()->selectedRows();
+	qDebug() << ml;
 	FOREACH (QModelIndexList, it, ml)
 		selectedItems.append(sha((*it).row()));
 }

--8<--

and got the following for the 4 commits around v1.6.3 in git.git when
enabled "All branches" (output prettified, was one line):

(QModelIndex(2554,0,0x0,FileHistory(0xa06bfe8) )  ,
 QModelIndex(2553,0,0x0,FileHistory(0xa06bfe8) )  ,
 QModelIndex(2552,0,0x0,FileHistory(0xa06bfe8) )  ,
 QModelIndex(2551,0,0x0,FileHistory(0xa06bfe8) )  )

Here it is ordered from oldest to newest. The newest commit (HEAD or
working tree) has '0'.

The same with "All branches" unchecked, just a bit smaller values:

(QModelIndex(404,0,0x0,FileHistory(0xa06bfe8) )  ,
 QModelIndex(403,0,0x0,FileHistory(0xa06bfe8) )  ,
 QModelIndex(402,0,0x0,FileHistory(0xa06bfe8) )  ,
 QModelIndex(401,0,0x0,FileHistory(0xa06bfe8) )  )

But try with selecting many commits, for example v1.6.3..origin/master
(fewer are sufficient). The order has a pattern, but it's not sorted at
all.

Or selecting only two commits v1.6.3 and origin/master prints this for
me, ordered from newest to oldest this time:

(QModelIndex(16,0,0x0,FileHistory(0xa06bfe8) )  ,
 QModelIndex(401,0,0x0,FileHistory(0xa06bfe8) )  )

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Markus Heidelberg]

Marco Costalba, 16.06.2009:
> On Mon, Jun 15, 2009 at 22:45, Marco Costalba<mcostalba@gmail.com> wrote:
> > On Mon, Jun 15, 2009 at 22:25, Marco Costalba<mcostalba@gmail.com> wrote:
> >>
> >> This is really strange !
> >>
> >
> 
> I have prepared a possible patch, could you please test if fixes the
> problem for you.

Yes, it works. Thanks. Would it make sense and be possible to fix it
directly in ListView::getSelectedItems()?

BTW, a nice way to see the created git-format-patch command without
modifying the sources is to use a directory with spaces, which is not
handled correctly by qgit.

Markus

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Marco Costalba]

On Tue, Jun 16, 2009 at 00:53, Markus
Heidelberg<markus.heidelberg@web.de> wrote:
>
> Yes, it works. Thanks. Would it make sense and be possible to fix it
> directly in ListView::getSelectedItems()?
>

I am not sure it is a problem of the selected items set. Unfortunatly
it is not ordered, but actually this was not documented, so it was my
bad to assume rows are always ordered because it happens they are
sorted with Qt4.3.3

And also ordering in Git::formatPatch() is easier because the "tools"
like appendNamesWithId() are already there and only there.

Another reason is that is Git::formatPatch() that needs the rows to be
ordered, so has a sense to do it there where is needed. Of corse if
other places of QGit requires that the rows returned by
ListView::getSelectedItems() should be ordered then we can move the
ordering in ListView::getSelectedItems() as you suggest, or, even
better, move the ordering in ListView::getSelectedItems() but from
there call a new helper function Git::sortShaByIndex() that does the
work and is defined in git.cpp because should use internal information
like the index and call directly the sha database.

Yes, a possible cleanup / reformat job could be to:

- Add a new public helper function Git::sortShaByIndex() defined in git.cpp

- Call from ListView::getSelectedItems() when we get the list of sha
user as select

- Document ListView::getSelectedItems() always returns a list of ordered sha's


> BTW, a nice way to see the created git-format-patch command without
> modifying the sources is to use a directory with spaces, which is not
> handled correctly by qgit.
>


????

I cannot test now, but this seems a bug.

Re: [PATCH QGit RFC] Fix "Save patch..." on a commit range

[Markus Heidelberg]

Marco Costalba, 16.06.2009:
> On Tue, Jun 16, 2009 at 00:53, Markus
> Heidelberg<markus.heidelberg@web.de> wrote:
> >
> > Yes, it works. Thanks. Would it make sense and be possible to fix it
> > directly in ListView::getSelectedItems()?
> 
> Another reason is that is Git::formatPatch() that needs the rows to be
> ordered, so has a sense to do it there where is needed.

ListView::mouseMoveEvent() needs it as well, I just tested
cherry-picking revisions per drag-and-drop and it was applied in the
wrong order.

I have never used stgit, but isn't MainImpl::ActPush_activated() and
MainImpl::ActPop_activated() affected in the same way?

> Of corse if
> other places of QGit requires that the rows returned by
> ListView::getSelectedItems() should be ordered then we can move the
> ordering in ListView::getSelectedItems() as you suggest, or, even
> better, move the ordering in ListView::getSelectedItems() but from
> there call a new helper function Git::sortShaByIndex() that does the
> work and is defined in git.cpp because should use internal information
> like the index and call directly the sha database.
> 
> Yes, a possible cleanup / reformat job could be to:
> 
> - Add a new public helper function Git::sortShaByIndex() defined in git.cpp
> 
> - Call from ListView::getSelectedItems() when we get the list of sha
> user as select
> 
> - Document ListView::getSelectedItems() always returns a list of ordered sha's

I guess this is the right solution.

> > BTW, a nice way to see the created git-format-patch command without
> > modifying the sources is to use a directory with spaces, which is not
> > handled correctly by qgit.
> >
> 
> 
> ????
> 
> I cannot test now, but this seems a bug.

I was not clear enough, but yes, it's a bug - with a usable side effect :)

Markus