threads / rfc / 19796

RFC patchFix "Save patch..." on a commit range

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

## tl;dr

15 messages between Jun 12, 2009 and Jun 16, 2009. Diffs are folded; open one to read it.

replies: 14people: 2as markdown or json

Markus Heidelberg· Jun 12, 2009, 23:29 UTC · lore

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(-)
Show changes to src/git.cpp +3 −3
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
Marco Costalba· Jun 13, 2009, 07:13 UTC · re: Markus Heidelberg · lore

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

On Sat, Jun 13, 2009 at 00:29, Markus Heidelberg<markus.heidelberg@web.de> wrote:

Show 5 quoted lines
> 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.

Markus Heidelberg· Jun 13, 2009, 10:11 UTC · re: Marco Costalba · lore

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

Marco Costalba, 13.06.2009:
Show 26 quoted lines
> 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
Markus Heidelberg· Jun 13, 2009, 11:02 UTC · re: Marco Costalba · lore

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

Marco Costalba, 13.06.2009:
Show 23 quoted lines
> 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
Marco Costalba· Jun 13, 2009, 11:12 UTC · re: Markus Heidelberg · lore

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

On Sat, Jun 13, 2009 at 12:02, Markus Heidelberg<markus.heidelberg@web.de> wrote:

Show 7 quoted lines
>
> 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

Markus Heidelberg· Jun 13, 2009, 11:33 UTC · re: Marco Costalba · lore

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

Marco Costalba, 13.06.2009:
Show 13 quoted lines
> 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
Marco Costalba· Jun 13, 2009, 12:23 UTC · re: Markus Heidelberg · lore

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

On Sat, Jun 13, 2009 at 12:33, Markus Heidelberg<markus.heidelberg@web.de> wrote:

Show 5 quoted lines
>
> 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

Markus Heidelberg· Jun 15, 2009, 21:13 UTC · re: Markus Heidelberg · lore

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

Markus Heidelberg, 13.06.2009:
Show 9 quoted lines
> 
> 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
Marco Costalba· Jun 15, 2009, 21:25 UTC · re: Markus Heidelberg · lore

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

On Mon, Jun 15, 2009 at 22:13, Markus Heidelberg<markus.heidelberg@web.de> wrote:

Show 34 quoted lines
> 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

Marco Costalba· Jun 15, 2009, 21:45 UTC · re: Marco Costalba · lore

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

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.
Marco Costalba· Jun 15, 2009, 22:07 UTC · re: Marco Costalba · lore

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

On Mon, Jun 15, 2009 at 22:45, Marco Costalba<mcostalba@gmail.com> wrote:
Show 5 quoted lines
> 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

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(-)
Show changes to src/git.cpp +12 −3
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
Markus Heidelberg· Jun 15, 2009, 23:53 UTC · re: Marco Costalba · lore

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

Marco Costalba, 16.06.2009:
Show 9 quoted lines
> 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
Marco Costalba· Jun 16, 2009, 06:10 UTC · re: Markus Heidelberg · lore

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

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.
Markus Heidelberg· Jun 16, 2009, 18:48 UTC · re: Marco Costalba · lore

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

Marco Costalba, 16.06.2009:
Show 8 quoted lines
> 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?

Show 17 quoted lines
> 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.
Show 9 quoted lines
> > 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
Markus Heidelberg· Jun 15, 2009, 23:44 UTC · re: Marco Costalba · lore

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

Marco Costalba, 15.06.2009:
> So you could check, in ListView::getSelectedItems() how the rows are returned.
I added this snippet:
-->8--
Show changes to src/listview.cpp +2 −1
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) )  )

← back to recent threads