stgit: bunch of bugreports/wishes

stgit: bunch of bugreports/wishes

[Yann Dirson]

Here are a number of problems I encountered while playing with
uncommit with 0.10:

- uncommit ignores grafts.  This causes "uncommit -n" to through
"graft merges" without asking, and surely gives unexpected result
when a graft is used to change an ancestor rather than adding one.

- the previous behaviour causes strange interactions with merge
commits (eg. "stg show" cannot show the diff)

- uncommit could allow to go through a merge (indeed ignoring grafts
as it did was a gain of time for me, since it was in a couple of cases
precisely what I wanted to do ;).  One possible interface to such a
feature would be to allow this if HEAD is the merge, and the branch to
follow is explicitely specified.  The issue mentionned above seems to
indicate that when uncommitting a merge, a new commit for the
newly-uncommitted patch has to be created right away, to ensure it has
a single parent.

- uncommit could be more flexible to help with mass-uncommitting,
eg. with something like "--to <commit>" (to avoid counting manually),
or "--to-merge" to cleanly stop on first merge instead of failing
there.  This may have an impact on how uncommits are numbered.

- uncommit synopsis is incomplete (lacks " | -n <n> <basename>")

- after mass-uncommitting, more help to look at the stack would be
needed.  Eg. a "stg series" flag to print more commit info (author,
files), or to limit the listing to a given author (like "stg patches"
limits for a file).


Additionally, a couple of non-uncommit-related thing:

- when a push is not committed because of a conflict, looking at the
previous diff for the patch would help.  Maybe something like "stg
show --old" ?

- the help string for push should say "patches", and possibly document
more precisely the syntax, something like:

--- a/stgit/commands/push.py
+++ b/stgit/commands/push.py
@@ -24,8 +24,8 @@ from stgit.utils import *
 from stgit import stack, git
 
 
-help = 'push a patch on top of the series'
-usage = """%prog [options] [<patch1> [<patch2>...]]
+help = 'push patches on top of the series'
+usage = """%prog [options] [<patch1> [<patch2>...] | -n <n> <patchroot>]
 
 Push a patch (defaulting to the first unapplied one) or range of
 patches to the stack. The 'push' operation allows patch reordering by


- "push --undo" is not robust.  On the occasion reproduced below, I
had to rollback the push myself by hand-modifying the stgit data,
which took me some effort.  I'll have to gather precise info, but the
issue occurs on patch reordering, on a genuine conflict, and seems to
be involve a change to a non-existent file, when that file would have
been added by a non-applied patch originally below the one I attempted
to apply.

I do agree there is a conflict, but "push --undo" should definitely be
able to rollback in any case.

Here is the log:

========
Now at patch "patch9"
Pushing patch "patch10"...The merge failed during "push". Use "refresh"
after fixing the conflicts
stg push: GIT index merging failed (possible conflicts)

stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them
stg push: local changes in the tree. Use "refresh" to commit them

ydirson$ stg push --undo
Undoing the "patch10" push...stg push: ['git-diff-index', 'HEAD', 'path/to/the/file.java'] failed (fatal: ambiguous argument
'path/to/the/file.java': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions)
========

Best regards,
-- 
Yann Dirson    <ydirson@altern.org> |
Debian-related: <dirson@debian.org> |   Support Debian GNU/Linux:
                                    |  Freedom, Power, Stability, Gratis
     http://ydirson.free.fr/        | Check <http://www.debian.org/>

Re: stgit: bunch of bugreports/wishes

[Catalin Marinas]

On 22/06/06, Yann Dirson <ydirson@altern.org> wrote:
> Here are a number of problems I encountered while playing with
> uncommit with 0.10:

"uncommit" was really intended as generating some simple patches from
a linear list of commits (maybe for undoing a "stg commit" or after a
git-am to modify some patches before pushint upstream). History
re-writing is somehow outside StGIT's goals.

> - uncommit ignores grafts.  This causes "uncommit -n" to through
> "graft merges" without asking, and surely gives unexpected result
> when a graft is used to change an ancestor rather than adding one.
[...]

I could fix "uncommit" to fail at this point but, as I said above, I
wouldn't add extra features to this command.

Maybe you can explain your workflow a bit as I don't see the need for
mass uncommitting.

> - uncommit could be more flexible to help with mass-uncommitting,
> eg. with something like "--to <commit>" (to avoid counting manually),
> or "--to-merge" to cleanly stop on first merge instead of failing
> there.  This may have an impact on how uncommits are numbered.
>
> - uncommit synopsis is incomplete (lacks " | -n <n> <basename>")
>
> - after mass-uncommitting, more help to look at the stack would be
> needed.  Eg. a "stg series" flag to print more commit info (author,
> files), or to limit the listing to a given author (like "stg patches"
> limits for a file).

These would be good indeed. I also had a plan to generate the patch
name from the subject line (i.e. replacing the spaces with a dash) to
be more meaningful. But got really busy with my job recently and
didn't have time.

> - when a push is not committed because of a conflict, looking at the
> previous diff for the patch would help.  Maybe something like "stg
> show --old" ?

"stg show <patch>//top.old" should show it (well, with a bit more
typing than --old).

> - the help string for push should say "patches", and possibly document
> more precisely the syntax, something like:

I plan to change the syntax of push a bit to allow things like
patch1..patch2 without the --to option (the latter would still be
there but taking a single patch).

> -help = 'push a patch on top of the series'
> -usage = """%prog [options] [<patch1> [<patch2>...]]
> +help = 'push patches on top of the series'
> +usage = """%prog [options] [<patch1> [<patch2>...] | -n <n> <patchroot>]

Does the <patchroot> syntax work?

> - "push --undo" is not robust.  On the occasion reproduced below, I
> had to rollback the push myself by hand-modifying the stgit data,
> which took me some effort.  I'll have to gather precise info, but the
> issue occurs on patch reordering, on a genuine conflict, and seems to
> be involve a change to a non-existent file, when that file would have
> been added by a non-applied patch originally below the one I attempted
> to apply.
[...]
> ydirson$ stg push --undo
> Undoing the "patch10" push...stg push: ['git-diff-index', 'HEAD', 'path/to/the/file.java'] failed (fatal: ambiguous argument
> 'path/to/the/file.java': unknown revision or path not in the working tree.
> Use '--' to separate paths from revisions)

I got this problem as well. StGIT needs fixing but I think a quick
workaround is to create an empty file (touch patch/to/the/file.java)
before the undo and git-diff-index will be happy.

Thanks for the bug reports/suggestions.

-- 
Catalin

Re: stgit: bunch of bugreports/wishes

[Catalin Marinas]

On 22/06/06, Yann Dirson <ydirson@altern.org> wrote:
> - "push --undo" is not robust.  On the occasion reproduced below, I
> had to rollback the push myself by hand-modifying the stgit data,
> which took me some effort.  I'll have to gather precise info, but the
> issue occurs on patch reordering, on a genuine conflict, and seems to
> be involve a change to a non-existent file, when that file would have
> been added by a non-applied patch originally below the one I attempted
> to apply.

I fixed this bug and I'll push the changes tonight. It was caused by a
"git-diff-index" call with files but without the "--" argument. When
the file was actually missing from the repository, git was confused
and returned an error. The 'status --reset' command was failing in
this situation as well.

-- 
Catalin

Re: stgit: bunch of bugreports/wishes

[Yann Dirson]

Hi Catalin,

I was a bit out of time lately, following the birth of my 3rd child.
Now seems like a good moment to followup on this issue.

On Mon, Jun 26, 2006 at 10:04:07PM +0100, Catalin Marinas wrote:
> On 22/06/06, Yann Dirson <ydirson@altern.org> wrote:
> >Here are a number of problems I encountered while playing with
> >uncommit with 0.10:
> 
> "uncommit" was really intended as generating some simple patches from
> a linear list of commits (maybe for undoing a "stg commit" or after a
> git-am to modify some patches before pushint upstream). History
> re-writing is somehow outside StGIT's goals.

But uncommitting commits to an stgit stack is surely rarely done without
taking advantage of stgit main features afterwards, and that is history
rewriting anyway.  It's just another way of looking at it :)


> >- uncommit ignores grafts.  This causes "uncommit -n" to through
> >"graft merges" without asking, and surely gives unexpected result
> >when a graft is used to change an ancestor rather than adding one.
> [...]
> 
> I could fix "uncommit" to fail at this point but, as I said above, I
> wouldn't add extra features to this command.

This is most probably fixed by one of the 2 patches I just posted.  The
problem is not specific to uncommit at all, it impacts all commands
needing to look at the parents of a commit.

BTW, while I'm at it - I only changed the commit constructor to stop
reading from the commit object for parent identification, there may be
similar standard ways to access the other informations.

 
> Maybe you can explain your workflow a bit as I don't see the need for
> mass uncommitting.

The team is using CVS for history, and I'm working on a git mirror of
that repository.  In this repository we're importing upstream sources on
a branch, developping our own stuff in the CVS trunk, merging new
upstream versions as they are shipped to us.

Then we ship some of our changes upstream, and this is where I'd like to
use the power of stgit, to change the cvs history into a set of
coherent, feature-sized patches, to be fed upstream.

I manually maintain a grafts file to record the merges (and occasionally
get rid of cvs artifacts in the imported history), so my tree is full
of merges and grafts.


More recently, the work I've started today is about similarly
reorganizing the history of a linux-2.4 BSP for a custom board, to make
it more easy to maintain it.  In that case, I've taken a slightly different
approach, since I knew "uncommit" would not help: I have started a new
stgit branch, and "stgit pick"'d the individual patches from various
branches - but that was only reasonable because the number of patches
was low.


> >- when a push is not committed because of a conflict, looking at the
> >previous diff for the patch would help.  Maybe something like "stg
> >show --old" ?
> 
> "stg show <patch>//top.old" should show it (well, with a bit more
> typing than --old).

Hm, that does not seem to work with 0.10:

$ stg show pick-parent//top.old
stg show: Unknown revision: pick-parent//top.old^{commit}


> >-help = 'push a patch on top of the series'
> >-usage = """%prog [options] [<patch1> [<patch2>...]]
> >+help = 'push patches on top of the series'
> >+usage = """%prog [options] [<patch1> [<patch2>...] | -n <n> <patchroot>]
> 
> Does the <patchroot> syntax work?

I mean, that when using -n, the string is not a patchname, but a root
that gets suffixed by numbers, and in that case only one name can be
given.


While I'm at it, here is another list of problems I hit today with 0.10:

- "stg fold" short usage does not tell what <file> is used for                                                                                                                       
- I did not find a clean way to fold into current patch only the part of a patch
  impacted a given file.  Whereas I can use "stg show | filterdiff | stg
  fold" for file modifications, that fails to work for permission
  changes and the like.
  
- "stg patches" only reports about currently applied patches, it would
  be very useful to report on unapplied patches as well

- "stg patches -d" is broken, but that may be fixed in HEAD already
  (missing -- when calling git-diff-tree)

- commit.get_parent() silently returns 1st parent when there are several
  ones.  I think it should raise an exception instead, and callers must
  guard against failure.

- pick badly fails when hitting conflicts.  This is especially confusing when the
  conflicts only comes from grafts being ignored.  It is especially bad when using
  "pick" to create the 1st patch in a series : then "stg delete" on the partially
  created patch cannot recover the failure (IIRC it complains there is
  no current patch)

- Looks like trying to resolve a conflict by removing the file currently require
  a precise ordering of operations, or we need manual recovering.  In
  the following example, the mach-type.h files was removed in a patch
  below, and the conflict occurs because the current patch was modifying
  it.  This may as well be fixed by the -- in diff-index call, but I'd
  rather report it in case there is another remaining issue:

$ stg status
C include/asm-arm/mach-types.h
[...]
$ stg rm include/asm-arm/mach-types.h
$ stg resolved
include/asm-arm/mach-types.h
stg resolved: ['git-diff-index', 'HEAD', 'include/asm-arm/mach-types.h']
failed (fatal: ambiguous argument 'include/asm-arm/mach-types.h':
unknown
revision or path not in the working tree.
Use '--' to separate paths from revisions)
ydirson@unx-4791:linux-git[906]$ stg status
C include/asm-arm/mach-types.h

Emergency fixup:

$ :>include/asm-arm/mach-types.h
$ cg-add include/asm-arm/mach-types.h
Adding file include/asm-arm/mach-types.h
$ stg resolved
include/asm-arm/mach-types.h
$ cg-rm include/asm-arm/mach-types.h
Removing file include/asm-arm/mach-types.h


- "pick --fold" embeds \n's in sha1's ?  (this needs to be checked, it
  could be a side-effect of my initial implementation of using rev-list
  to identify parents)

$ ~/tools/git/stgit-0.10/stg pick --fold
8d8ecef22870e6a6b063d95d9b90d69e8391a0d0
Folding commit 8d8ecef22870e6a6b063d95d9b90d69e8391a0d0...stg pick:
['git-diff-tree', '-p', 'de47893112debe4cd3871162a0d1df0b73000e94\n',
'8d8ecef22870e6a6b063d95d9b90d69e8391a0d0'] failed (fatal: ambiguous
argument 'de47893112debe4cd3871162a0d1df0b73000e94
': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions)

whereas:

$ ~/tools/git/stgit-0.10/stg pick
8d8ecef22870e6a6b063d95d9b90d69e8391a0d0 -n import-fix
Importing commit 8d8ecef22870e6a6b063d95d9b90d69e8391a0d0... done
Now at patch "import-fix"

Best regards,
-- 
Yann.