kha/safe and kha/experimental updated

kha/safe and kha/experimental updated

[Karl Hasselström]

On 2007-12-17 23:48:12 +0100, Karl Hasselström wrote:

> If you like, I can advance "safe" to include as many patches as I
> think you should merge right now.

And here it is:

The following changes since commit 5be69cd8d89bd89be31364e9c0fd9e947b6ef644:
  Karl Hasselström (1):
        Name the exit codes to improve legibility

are available in the git repository at:

  git://repo.or.cz/stgit/kha.git safe

David Kågedal (17):
      Add an StGit mode for emacs
      Emacs mode: Improve the output buffer state
      Emacs mode: Bind n and p
      Emacs mode: add stgit-repair
      Emacs mode: added stgit-commit and stgit-uncommit
      Emacs mode: Add stgit-edit command
      Emacs mode: added fontification
      Emacs mode: Added stgit-new
      Check bottom and invariants
      Remove the 'bottom' field
      Remove the 'top' field
      Split git.merge into two functions
      Leave working dir and index alone after failed (conflicting) push
      Added a test case to check what happens when push finds a conflict
      Simplify merge_recursive
      Use the output from merge-recursive to list conflicts
      Ask git about unmerged files

Karl Hasselström (24):
      Emacs mode: Show keybindings when user presses "h" or "?"
      Emacs mode: Add an explanatory header
      Emacs mode: Makefile for building stgit.el
      Emacs mode: push/pop next patch, not patch at point
      Emacs mode: Let "P" push or pop patch at point
      Emacs mode: Bind "G" to "stg goto"
      Emacs mode: show patches' short description
      Write removed fields for backwards compatibility
      Nicer conflict markers
      Better error message if merge fails
      Fix "stg resolved" to work with new conflict representation
      Refactoring: pass more than one file to resolved()
      We keep the different stages of a conflict in the index now
      "stg status --reset" is not needed anymore
      Remove "stg add"
      Remove "stg rm"
      Remove "stg cp"
      Remove "stg resolved"
      New StGit core infrastructure: repository operations
      Write metadata files used by the old infrastructure
      Upgrade older stacks to newest version
      Let "stg clean" use the new infrastructure
      Add "stg coalesce"
      Let "stg applied" and "stg unapplied" use the new infrastructure

 Documentation/stg-cp.txt      |   63 --------
 Documentation/stg.txt         |    2 -
 Documentation/tutorial.txt    |   31 +++--
 contrib/Makefile              |   19 +++
 contrib/stgit-completion.bash |    4 +-
 contrib/stgit.el              |  318 +++++++++++++++++++++++++++++++++++++++++
 examples/gitconfig            |   19 +---
 setup.py                      |    2 +-
 stgit/commands/add.py         |   44 ------
 stgit/commands/applied.py     |   27 ++--
 stgit/commands/clean.py       |   68 +++++-----
 stgit/commands/coalesce.py    |   84 +++++++++++
 stgit/commands/common.py      |   39 +++---
 stgit/commands/copy.py        |   45 ------
 stgit/commands/pick.py        |    2 +-
 stgit/commands/resolved.py    |   94 ------------
 stgit/commands/rm.py          |   48 ------
 stgit/commands/status.py      |   31 ++---
 stgit/commands/sync.py        |    1 -
 stgit/commands/unapplied.py   |   23 ++--
 stgit/config.py               |    1 -
 stgit/git.py                  |   75 ++++++----
 stgit/gitmergeonefile.py      |   97 +------------
 stgit/lib/__init__.py         |   18 +++
 stgit/lib/git.py              |  260 +++++++++++++++++++++++++++++++++
 stgit/lib/stack.py            |  172 ++++++++++++++++++++++
 stgit/lib/stackupgrade.py     |   96 ++++++++++++
 stgit/lib/transaction.py      |   79 ++++++++++
 stgit/main.py                 |   10 +-
 stgit/run.py                  |    3 +
 stgit/stack.py                |  156 ++++----------------
 stgit/utils.py                |   24 +++
 t/t0002-status.sh             |   15 +-
 t/t1200-push-modified.sh      |    2 +-
 t/t1202-push-undo.sh          |    6 +-
 t/t1203-push-conflict.sh      |   70 +++++++++
 t/t1204-pop-keep.sh           |    2 +-
 t/t1205-push-subdir.sh        |    8 +-
 t/t1300-uncommit.sh           |    4 +-
 t/t1301-repair.sh             |    2 +-
 t/t1400-patch-history.sh      |    4 +-
 t/t1500-float.sh              |   14 +-
 t/t1600-delete-one.sh         |   12 +-
 t/t1601-delete-many.sh        |    2 +-
 t/t1700-goto-top.sh           |    2 +-
 t/t2000-sync.sh               |   12 +-
 t/t2100-pull-policy-fetch.sh  |    4 +-
 t/t2101-pull-policy-pull.sh   |    4 +-
 t/t2102-pull-policy-rebase.sh |    4 +-
 t/t2300-refresh-subdir.sh     |    2 +-
 t/t2600-coalesce.sh           |   31 ++++
 51 files changed, 1420 insertions(+), 735 deletions(-)
 delete mode 100644 Documentation/stg-cp.txt
 create mode 100644 contrib/Makefile
 create mode 100644 contrib/stgit.el
 delete mode 100644 stgit/commands/add.py
 create mode 100644 stgit/commands/coalesce.py
 delete mode 100644 stgit/commands/copy.py
 delete mode 100644 stgit/commands/resolved.py
 delete mode 100644 stgit/commands/rm.py
 create mode 100644 stgit/lib/__init__.py
 create mode 100644 stgit/lib/git.py
 create mode 100644 stgit/lib/stack.py
 create mode 100644 stgit/lib/stackupgrade.py
 create mode 100644 stgit/lib/transaction.py
 create mode 100755 t/t1203-push-conflict.sh
 create mode 100755 t/t2600-coalesce.sh

                                 -+-

The following changes since commit 1189271615e7157ca3da3288a9ce3c9bcaeb2d5f:
  Karl Hasselström (1):
        Let "stg applied" and "stg unapplied" use the new infrastructure

are available in the git repository at:

  git://repo.or.cz/stgit/kha.git experimental

David Kågedal (2):
      Emacs mode: Add mark command
      Emacs mode: coalesce command

Karl Hasselström (10):
      Teach the new infrastructure about the index and worktree
      Let "stg clean" use the new transaction primitives
      Let "stg goto" use the new infrastructure
      Convert "stg uncommit" to the new infrastructure
      New infrastructure: Make sure that the branch is initialized
      Expose transaction abort function
      stg coalesce: Support --file and --save-template
      Set exit code to 3 on merge conflict
      Convert "stg commit" to new infrastructure
      Make "stg commit" fancier

 contrib/stgit.el           |   97 +++++++++++++++++++-----
 stgit/commands/clean.py    |   35 ++-------
 stgit/commands/coalesce.py |  118 +++++++++++++++++++----------
 stgit/commands/commit.py   |  111 +++++++++++++++++----------
 stgit/commands/goto.py     |   52 +++++---------
 stgit/commands/uncommit.py |   81 +++++++++-----------
 stgit/lib/git.py           |  127 +++++++++++++++++++++++++++++++
 stgit/lib/stack.py         |   10 ++-
 stgit/lib/stackupgrade.py  |    6 +-
 stgit/lib/transaction.py   |  178 ++++++++++++++++++++++++++++++++++++++------
 stgit/main.py              |    4 +-
 stgit/utils.py             |    1 +
 t/t1300-uncommit.sh        |   12 ++--
 13 files changed, 588 insertions(+), 244 deletions(-)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: kha/safe and kha/experimental updated

[Catalin Marinas]

Thanks again for maintaining these branches.

On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
>   git://repo.or.cz/stgit/kha.git safe
>
> David Kågedal (17):
[...]
>       Simplify merge_recursive
>       Use the output from merge-recursive to list conflicts

I'll drop git.merge() entirely since it is only used by the 'sync'
command due to the performance issues I had in the past with rename
detection in git-merge-recursive. Hopefully, these are gone now.

I'll try to fix the automatic invocation of the interactive merger in
case of conflicts (it is only present in git.merge()).

> Karl Hasselström (24):
>       Fix "stg resolved" to work with new conflict representation

For some reason, the interactive resolving keeps invoking the merger.
I'll have a look.

>       "stg status --reset" is not needed anymore

I would keep this as an alias for 'git reset --hard' (see below as well).

>       Remove "stg add"
>       Remove "stg rm"
>       Remove "stg cp"

I plan to add a generic command for these kind of aliases. The reason
is that I don't really like mixing GIT and StGIT commands (I think at
some point I'll get confused and try to run stg commands with git).

>       Remove "stg resolved"

I'd like to keep this command. git-mergetool doesn't support the tool
I use (emacs + ediff and more stgit-specific file extensions like
current, patch etc.). I also don't find 'git add' to be meaningful for
marking a conflict as solved.

-- 
Catalin

Re: kha/safe and kha/experimental updated

[Jakub Narebski]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> Thanks again for maintaining these branches.
> 
> On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
> >   git://repo.or.cz/stgit/kha.git safe

> >       Remove "stg resolved"
> 
> I'd like to keep this command. git-mergetool doesn't support the tool
> I use (emacs + ediff and more stgit-specific file extensions like
> current, patch etc.). I also don't find 'git add' to be meaningful for
> marking a conflict as solved.

I also would like to have this command kept (and shown in 'stg help'!).
Contrary to 'git add' it can check and add to index / update index
only for files with conflict; we have -r (ancestor|current|patched)
to choose one side, and we could add --check to check if there are
no conflict markers with files (useful with -a/--all).

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: kha/safe and kha/experimental updated

[Catalin Marinas]

On 18/12/2007, Jakub Narebski <jnareb@gmail.com> wrote:
> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
>
> > Thanks again for maintaining these branches.
> >
> > On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
> > >   git://repo.or.cz/stgit/kha.git safe
>
> > >       Remove "stg resolved"
> >
> > I'd like to keep this command. git-mergetool doesn't support the tool
> > I use (emacs + ediff and more stgit-specific file extensions like
> > current, patch etc.). I also don't find 'git add' to be meaningful for
> > marking a conflict as solved.
>
> I also would like to have this command kept (and shown in 'stg help'!).
> Contrary to 'git add' it can check and add to index / update index
> only for files with conflict; we have -r (ancestor|current|patched)
> to choose one side, and we could add --check to check if there are
> no conflict markers with files (useful with -a/--all).

I'd also like to re-add the stgit.keeporig option and additional
functionality so that the *.{ancestor,current,patched} can be left in
the working tree. Some people might use them when manually fixing
conflicts (I have a look at them from time to time when the emacs +
ediff shows a hard to understand conflict).

-- 
Catalin

Re: kha/safe and kha/experimental updated

[Karl Hasselström]

On 2007-12-18 16:09:24 +0000, Catalin Marinas wrote:

> On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
>
> >       "stg status --reset" is not needed anymore
>
> I would keep this as an alias for 'git reset --hard' (see below as
> well).
>
> >       Remove "stg add"
> >       Remove "stg rm"
> >       Remove "stg cp"
>
> I plan to add a generic command for these kind of aliases. The
> reason is that I don't really like mixing GIT and StGIT commands (I
> think at some point I'll get confused and try to run stg commands
> with git).

How should these aliases be presented in the documentation etc.? I
suggest making it very clear that they are only aliases.

> >       Remove "stg resolved"
>
> I'd like to keep this command. git-mergetool doesn't support the
> tool I use (emacs + ediff and more stgit-specific file extensions
> like current, patch etc.).

So why have a separate command instead of fixing git-mergetool?

> I also don't find 'git add' to be meaningful for marking a conflict
> as solved.

So maybe let "stg resolved" be an alias for "git add"?

This is all our usual disagreement: You want stg to be a fairly
standalone tool, and I want it to be a tool to use side by side with
git. The problem I see with your approach is that stg risks ending up
like Cogito: it'll provide inspiration for improving git, but will
itself become obsolete because of the simple fact that git has so much
more development manpower. I think it'd be more productive to let stg
do one thing -- patch stacks -- and do it well, and rely on git for
everything else.

Of course, if stuff like "stg add" and "stg resolved" are really
implemented as three-line wrappers around git commands, we don't have
that problem.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: kha/safe and kha/experimental updated

[Karl Hasselström]

On 2007-12-18 08:39:52 -0800, Jakub Narebski wrote:

> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
>
> > On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
> >
> > >       Remove "stg resolved"
> >
> > I'd like to keep this command. git-mergetool doesn't support the
> > tool I use (emacs + ediff and more stgit-specific file extensions
> > like current, patch etc.). I also don't find 'git add' to be
> > meaningful for marking a conflict as solved.
>
> I also would like to have this command kept (and shown in 'stg
> help'!). Contrary to 'git add' it can check and add to index /
> update index only for files with conflict; we have -r
> (ancestor|current|patched) to choose one side, and we could add
> --check to check if there are no conflict markers with files (useful
> with -a/--all).

This too sounds like stuff that could profitably be added to "git
add". Except for the ancestor/current/patched words, it is not
specific to patch stacks, so the implementation should be in git and
not in stg.

IMHO, of course. :-)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: kha/safe and kha/experimental updated

[David Kågedal]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> On 18/12/2007, Jakub Narebski <jnareb@gmail.com> wrote:
>> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
>>
>> > Thanks again for maintaining these branches.
>> >
>> > On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
>> > >   git://repo.or.cz/stgit/kha.git safe
>>
>> > >       Remove "stg resolved"
>> >
>> > I'd like to keep this command. git-mergetool doesn't support the tool
>> > I use (emacs + ediff and more stgit-specific file extensions like
>> > current, patch etc.). I also don't find 'git add' to be meaningful for
>> > marking a conflict as solved.
>>
>> I also would like to have this command kept (and shown in 'stg help'!).
>> Contrary to 'git add' it can check and add to index / update index
>> only for files with conflict; we have -r (ancestor|current|patched)
>> to choose one side, and we could add --check to check if there are
>> no conflict markers with files (useful with -a/--all).
>
> I'd also like to re-add the stgit.keeporig option and additional
> functionality so that the *.{ancestor,current,patched} can be left in
> the working tree. Some people might use them when manually fixing
> conflicts (I have a look at them from time to time when the emacs +
> ediff shows a hard to understand conflict).

Since all the information is in git, it is of course easy to recreate
it. But the important question to ask is: how do you use these extra
files? git.el provides a way to diff against both parent versions, and
maybe that is actually what you need.

I don't mind that you want these files, but they are mostly clutter to
me.

-- 
David Kågedal

Re: kha/safe and kha/experimental updated

[David Kågedal]

David Kågedal <davidk@lysator.liu.se> writes:

> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
>
>> On 18/12/2007, Jakub Narebski <jnareb@gmail.com> wrote:
>>> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
>>>
>>> > Thanks again for maintaining these branches.
>>> >
>>> > On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
>>> > >   git://repo.or.cz/stgit/kha.git safe
>>>
>>> > >       Remove "stg resolved"
>>> >
>>> > I'd like to keep this command. git-mergetool doesn't support the tool
>>> > I use (emacs + ediff and more stgit-specific file extensions like
>>> > current, patch etc.). I also don't find 'git add' to be meaningful for
>>> > marking a conflict as solved.
>>>
>>> I also would like to have this command kept (and shown in 'stg help'!).
>>> Contrary to 'git add' it can check and add to index / update index
>>> only for files with conflict; we have -r (ancestor|current|patched)
>>> to choose one side, and we could add --check to check if there are
>>> no conflict markers with files (useful with -a/--all).
>>
>> I'd also like to re-add the stgit.keeporig option and additional
>> functionality so that the *.{ancestor,current,patched} can be left in
>> the working tree. Some people might use them when manually fixing
>> conflicts (I have a look at them from time to time when the emacs +
>> ediff shows a hard to understand conflict).
>
> Since all the information is in git, it is of course easy to recreate
> it. But the important question to ask is: how do you use these extra
> files? git.el provides a way to diff against both parent versions, and
> maybe that is actually what you need.

Note that I'm talking about the kha/experimental branch here. The
point of my patches there are to make sure that the git index is left
in the unmerged state so that normal git tools can be used.  This
means that git diff-files -2 and -3 does what you want.

-- 
David Kågedal

Re: kha/safe and kha/experimental updated

[Catalin Marinas]

On 19/12/2007, Karl Hasselström <kha@treskal.com> wrote:
> On 2007-12-18 16:09:24 +0000, Catalin Marinas wrote:
>
> > On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
> >
> > >       "stg status --reset" is not needed anymore
> >
> > I would keep this as an alias for 'git reset --hard' (see below as
> > well).
> >
> > >       Remove "stg add"
> > >       Remove "stg rm"
> > >       Remove "stg cp"
> >
> > I plan to add a generic command for these kind of aliases. The
> > reason is that I don't really like mixing GIT and StGIT commands (I
> > think at some point I'll get confused and try to run stg commands
> > with git).
>
> How should these aliases be presented in the documentation etc.? I
> suggest making it very clear that they are only aliases.

Yes, 'stg help' should clearly mark them as aliases of git commands.
I'm not sure how I'll do this yet. For the 'status --reset', I'll keep
it if we have a separate 'resolved' command.

> > >       Remove "stg resolved"
> >
> > I'd like to keep this command. git-mergetool doesn't support the
> > tool I use (emacs + ediff and more stgit-specific file extensions
> > like current, patch etc.).
>
> So why have a separate command instead of fixing git-mergetool?

In the past, StGIT only used the core git functionality and stayed
away from the porcelain scripts. We now started to use some of the
built-in commands more and more and I think it is fine but the git
scripts look more volatile to me than the C code (look at the many
variants the git merging went through, from shell scripts to python
and only now seems to be stabilised with the recursive merge as a
built-in command).

Adding the functionality I need to git-mergetool would also mean
always using the latest GIT with StGIT. Anyway, if you like
git-mergetool, I think you can easily change the StGIT configuration
to

> > I also don't find 'git add' to be meaningful for marking a conflict
> > as solved.
>
> So maybe let "stg resolved" be an alias for "git add"?

It's not a simple alias, if you use 'stg resolved -a'. Now, if we
allow stgit.keeporig, it needs even more functionality.

> This is all our usual disagreement: You want stg to be a fairly
> standalone tool, and I want it to be a tool to use side by side with
> git. The problem I see with your approach is that stg risks ending up
> like Cogito: it'll provide inspiration for improving git, but will
> itself become obsolete because of the simple fact that git has so much
> more development manpower.

If GIT would have all the functionality I need for my workflow, I
wouldn't mind giving up StGIT (I started StGIT because I needed easier
patch management on top of GIT and modifying Quilt would've been too
intrusive).

> I think it'd be more productive to let stg
> do one thing -- patch stacks -- and do it well, and rely on git for
> everything else.

In principle, yes, but I have some minor problems with the UI of GIT
(like resolved == add). I think there is also a fundamental difference
between 'git commit' and 'stg refresh'. The former requires explicit
marking of the files to commit (or use the -a option) while the latter
always adds all the local changes to the current patch. Maybe GIT
users are more used to running 'git add' after fixing a conflict but I
use StGIT almost exclusively.

> Of course, if stuff like "stg add" and "stg resolved" are really
> implemented as three-line wrappers around git commands, we don't have
> that problem.

For add/rm/cp, they should just be a generic wrapper which doesn't
even parse the command line options. That's not the case with
'resolved'.

-- 
Catalin

Re: kha/safe and kha/experimental updated

[Catalin Marinas]

On 19/12/2007, David Kågedal <davidk@lysator.liu.se> wrote:
> "Catalin Marinas" <catalin.marinas@gmail.com> writes:
> > I'd also like to re-add the stgit.keeporig option and additional
> > functionality so that the *.{ancestor,current,patched} can be left in
> > the working tree. Some people might use them when manually fixing
> > conflicts (I have a look at them from time to time when the emacs +
> > ediff shows a hard to understand conflict).
>
> Since all the information is in git, it is of course easy to recreate
> it. But the important question to ask is: how do you use these extra
> files? git.el provides a way to diff against both parent versions, and
> maybe that is actually what you need.
>
> I don't mind that you want these files, but they are mostly clutter to
> me.

You can set stgit.keeporig to 'no' if you don't want these files.

For people not using emacs + git.el, the files might be useful. As you
said, you could use 'git diff -2/-3' but, in various occasions I just
wanted to copy a block of code from one of the checked-out stages.
Without this feature, I would have to use plain git and remember what
stage corresponds to my patch.

Note that there are other users (apart from me) that use StGIT almost
exclusively. I really don't like forcing them to use more and more
plain git commands (at some point, they might even discover 'git
rebase -i' to be good enough and give up on StGIT :-)).

-- 
Catalin

Re: kha/safe and kha/experimental updated

[Jakub Narebski]

On Wed, 19 Dec 2007, Karl Hasselström wrote:
> On 2007-12-18 08:39:52 -0800, Jakub Narebski wrote:
> > "Catalin Marinas" <catalin.marinas@gmail.com> writes:
> >
> > > On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
> > >
> > > >       Remove "stg resolved"
> > >
> > > I'd like to keep this command. git-mergetool doesn't support the
> > > tool I use (emacs + ediff and more stgit-specific file extensions
> > > like current, patch etc.). I also don't find 'git add' to be
> > > meaningful for marking a conflict as solved.
> >
> > I also would like to have this command kept (and shown in 'stg
> > help'!). Contrary to 'git add' it can check and add to index /
> > update index only for files with conflict; we have -r
> > (ancestor|current|patched) to choose one side, and we could add
> > --check to check if there are no conflict markers with files (useful
> > with -a/--all).
> 
> This too sounds like stuff that could profitably be added to "git
> add". Except for the ancestor/current/patched words, it is not
> specific to patch stacks, so the implementation should be in git and
> not in stg.

No it cannot, at least the '-r (ancestor|current|patched)' part for
resetting file to given version, even if we change the wording to
ancestor, ours and theirs. The git-add command is about adding contents, 
which updates index, which almost as a intended side-effect clears 
merge state, i.e. stages; and not about resetting to stage.

Besides with "stg resolved" you can update index _only_ for files
which were in the conflict, also for -a/--all, and not all the files
not only those which were in the conflict like "git add -u" does.

"stg resolved --check" could happily ignore things that only look
like conflict markers, but must have been intended, because they are
in files not in conflict.


Unless you are talking about adding "resolve"/"resolved" command
to git-core, as a porcelain wrapper around git-update-index, like
"git add"...


P.S. I have just noticed that 'git-checkout' [<tree-ish>] <paths>...
form of operation is not documented; you can derive what it do
only from examples.

-- 
Jakub Narebski
Poland

Re: kha/safe and kha/experimental updated

[Karl Hasselström]

On 2007-12-19 10:09:40 +0000, Catalin Marinas wrote:

> For the 'status --reset', I'll keep it if we have a separate
> 'resolved' command.

?

> In the past, StGIT only used the core git functionality and stayed
> away from the porcelain scripts. We now started to use some of the
> built-in commands more and more and I think it is fine but the git
> scripts look more volatile to me than the C code (look at the many
> variants the git merging went through, from shell scripts to python
> and only now seems to be stabilised with the recursive merge as a
> built-in command).

Hmm? Have we started to use more porcelain lately?

> Adding the functionality I need to git-mergetool would also mean
> always using the latest GIT with StGIT.

It wouldn't keep being the latest git version, though.

> Anyway, if you like git-mergetool, I think you can easily change the
> StGIT configuration to

I don't use interactive merging at all. What I wanted was to get rid
of StGit's own interactive merging.

> On 19/12/2007, Karl Hasselström <kha@treskal.com> wrote:
>
> > On 2007-12-18 16:09:24 +0000, Catalin Marinas wrote:
> >
> > > I also don't find 'git add' to be meaningful for marking a
> > > conflict as solved.
> >
> > So maybe let "stg resolved" be an alias for "git add"?
>
> It's not a simple alias, if you use 'stg resolved -a'. Now, if we
> allow stgit.keeporig, it needs even more functionality.

Hmm, yes.

(I still don't quite like it, because I think that if there's anything
that "stg resolved" does that "git add" can't do, then "git add"
should learn to do it. But until it has learned it, it's a
hypothetical argument.)

> > I think it'd be more productive to let stg do one thing -- patch
> > stacks -- and do it well, and rely on git for everything else.
>
> In principle, yes, but I have some minor problems with the UI of GIT
> (like resolved == add). I think there is also a fundamental
> difference between 'git commit' and 'stg refresh'. The former
> requires explicit marking of the files to commit (or use the -a
> option) while the latter always adds all the local changes to the
> current patch. Maybe GIT users are more used to running 'git add'
> after fixing a conflict but I use StGIT almost exclusively.

So you want StGit to do two things: patch stacks, and fix some git UI
warts. Hey, I can live with that. :-) But I firmly believe that the
wart fixing parts of StGit should be (1) optional, so that the same
job can easily be done with just git; and (2) as thin as possible, to
make them easy to explain in terms of git, and cheap to maintain.

> > Of course, if stuff like "stg add" and "stg resolved" are really
> > implemented as three-line wrappers around git commands, we don't
> > have that problem.
>
> For add/rm/cp, they should just be a generic wrapper which doesn't
> even parse the command line options.

OK.

> That's not the case with 'resolved'.

No, I see.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: kha/safe and kha/experimental updated

[Karl Hasselström]

On 2007-12-19 11:44:57 +0100, Jakub Narebski wrote:

> On Wed, 19 Dec 2007, Karl Hasselström wrote:
>
> > On 2007-12-18 08:39:52 -0800, Jakub Narebski wrote:
> >
> > > I also would like to have this command kept (and shown in 'stg
> > > help'!). Contrary to 'git add' it can check and add to index /
> > > update index only for files with conflict; we have -r
> > > (ancestor|current|patched) to choose one side, and we could add
> > > --check to check if there are no conflict markers with files
> > > (useful with -a/--all).
> >
> > This too sounds like stuff that could profitably be added to "git
> > add". Except for the ancestor/current/patched words, it is not
> > specific to patch stacks, so the implementation should be in git
> > and not in stg.
>
> No it cannot, at least the '-r (ancestor|current|patched)' part for
> resetting file to given version, even if we change the wording to
> ancestor, ours and theirs. The git-add command is about adding
> contents, which updates index, which almost as a intended
> side-effect clears merge state, i.e. stages; and not about resetting
> to stage.

  git checkout-index --stage=1|2|3 <filename>

does what you want. But "git cat-file" knows this handy syntax for
getting at particular index stages:

  :stage:<filename>

It would be very convenient to be able to do

  git checkout :stage:<filename>

but it doesn't seem to work currently. Does anyone know the "best" way
of manually checking out a particular index stage in git?

> Besides with "stg resolved" you can update index _only_ for files
> which were in the conflict, also for -a/--all, and not all the files
> not only those which were in the conflict like "git add -u" does.

This sounds like a straightforward addition to "git add".

> "stg resolved --check" could happily ignore things that only look
> like conflict markers, but must have been intended, because they are
> in files not in conflict.

git knows about conflicting files too.

> Unless you are talking about adding "resolve"/"resolved" command to
> git-core, as a porcelain wrapper around git-update-index, like "git
> add"...

Yes, that's what I want. You and others like what "stg resolved" does,
but I don't want it in StGit because it's a generic git enhancement
that has nothing to with patch stacks. People who don't use StGit
would presumably like it as well.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: kha/safe and kha/experimental updated

[Catalin Marinas]

On 19/12/2007, Karl Hasselström <kha@treskal.com> wrote:
> On 2007-12-19 10:09:40 +0000, Catalin Marinas wrote:
>
> > For the 'status --reset', I'll keep it if we have a separate
> > 'resolved' command.
>
> ?

It needs to call the resolved_all to remove checked-out stages if
stgit.keeporig == 'yes'. Maybe it could also do some sanity check if
HEAD != top. With 'git reset --hard', people might easily add an
argument and break the whole stack.

> > In the past, StGIT only used the core git functionality and stayed
> > away from the porcelain scripts. We now started to use some of the
> > built-in commands more and more and I think it is fine but the git
> > scripts look more volatile to me than the C code (look at the many
> > variants the git merging went through, from shell scripts to python
> > and only now seems to be stabilised with the recursive merge as a
> > built-in command).
>
> Hmm? Have we started to use more porcelain lately?

I think Yann was complaining about using git-show since it looks more
like a porcelain command.

> > Adding the functionality I need to git-mergetool would also mean
> > always using the latest GIT with StGIT.
>
> It wouldn't keep being the latest git version, though.

Yes, but at least initially it should be pretty recent.

> > Anyway, if you like git-mergetool, I think you can easily change the
> > StGIT configuration to
>
> I don't use interactive merging at all. What I wanted was to get rid
> of StGit's own interactive merging.

I use it quite often and I even invoke it automatically
(stgit.autoimerge). I'll push some patches tonight together with most
of your safe branch and we can alter them afterwards.

> So you want StGit to do two things: patch stacks, and fix some git UI
> warts. Hey, I can live with that. :-) But I firmly believe that the
> wart fixing parts of StGit should be (1) optional, so that the same
> job can easily be done with just git; and (2) as thin as possible, to
> make them easy to explain in terms of git, and cheap to maintain.

Unless you need the keeporig functionality, you can now always use
plain git for solving merges, add/rm/cp, 'reset --hard' etc. At some
point, we could make it safe for 'git rebase' but I think we need the
DAG patches.

> > That's not the case with 'resolved'.
>
> No, I see.

As I said, if you don't need keeporig, just use git-add (or stg-add
when I'll add the alias).

-- 
Catalin

Re: kha/safe and kha/experimental updated

[Catalin Marinas]

On 19/12/2007, Karl Hasselström <kha@treskal.com> wrote:
> You and others like what "stg resolved" does,
> but I don't want it in StGit because it's a generic git enhancement
> that has nothing to with patch stacks. People who don't use StGit
> would presumably like it as well.

The --reset option would never be added to git, at least not with the
same name for arguments since git doesn't manage patches. As I said, I
would have to remember what stage my 'patched' file is in.

-- 
Catalin

Re: kha/safe and kha/experimental updated

[Karl Hasselström]

On 2007-12-19 11:40:41 +0000, Catalin Marinas wrote:

> On 19/12/2007, Karl Hasselström <kha@treskal.com> wrote:
>
> > On 2007-12-19 10:09:40 +0000, Catalin Marinas wrote:
> >
> > > For the 'status --reset', I'll keep it if we have a separate
> > > 'resolved' command.
> >
> > ?
>
> It needs to call the resolved_all to remove checked-out stages if
> stgit.keeporig == 'yes'.

Ah, right.

> Maybe it could also do some sanity check if HEAD != top. With 'git
> reset --hard', people might easily add an argument and break the
> whole stack.

True.

> > Hmm? Have we started to use more porcelain lately?
>
> I think Yann was complaining about using git-show since it looks
> more like a porcelain command.

We should probably use cat-file or something instead.

> > It wouldn't keep being the latest git version, though.
>
> Yes, but at least initially it should be pretty recent.

:-)

> > I don't use interactive merging at all. What I wanted was to get
> > rid of StGit's own interactive merging.
>
> I use it quite often and I even invoke it automatically
> (stgit.autoimerge). I'll push some patches tonight together with
> most of your safe branch and we can alter them afterwards.

Jolly good! My stack was getting unwieldy ...

> > So you want StGit to do two things: patch stacks, and fix some git
> > UI warts. Hey, I can live with that. :-) But I firmly believe that
> > the wart fixing parts of StGit should be (1) optional, so that the
> > same job can easily be done with just git; and (2) as thin as
> > possible, to make them easy to explain in terms of git, and cheap
> > to maintain.
>
> Unless you need the keeporig functionality, you can now always use
> plain git for solving merges, add/rm/cp, 'reset --hard' etc.

Yes, with David's conflict series.

> At some point, we could make it safe for 'git rebase' but I think we
> need the DAG patches.

I wouldn't resurrect the DAG patches for this; I'd just invoke "stg
repair" automatically when we detect that top != HEAD.

But I think that for "git rebase", we'd want to teach "repair" to
detect that the patches' commits have been changed, rather than just
marking them unapplied.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: kha/safe and kha/experimental updated

[Catalin Marinas]

On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
>   git://repo.or.cz/stgit/kha.git safe
[...]
>       Ask git about unmerged files

This patch wrongly assumes that there is a stage 2 entry in the index.
Test t1202-push-undo.sh fails because a file is added, differently, in
both master and patch but it doesn't exist in ancestor (no stage 2).
Using stage 3 doesn't fix it either because a patch might remove a
file.

I fixed it by using a set to get the unique names.

-- 
Catalin

Re: kha/safe and kha/experimental updated

[Catalin Marinas]

On 18/12/2007, Catalin Marinas <catalin.marinas@gmail.com> wrote:
> On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
> >       Fix "stg resolved" to work with new conflict representation
>
> For some reason, the interactive resolving keeps invoking the merger.
> I'll have a look.

I found the problem. git.ls_files() now returns all three stages if
there was a conflict and the file list got longer. I added a set to
remove the duplicates.

-- 
Catalin

Re: kha/safe and kha/experimental updated

[David Kågedal]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

> On 18/12/2007, Karl Hasselström <kha@treskal.com> wrote:
>>   git://repo.or.cz/stgit/kha.git safe
> [...]
>>       Ask git about unmerged files
>
> This patch wrongly assumes that there is a stage 2 entry in the index.
> Test t1202-push-undo.sh fails because a file is added, differently, in
> both master and patch but it doesn't exist in ancestor (no stage 2).
> Using stage 3 doesn't fix it either because a patch might remove a
> file.
>
> I fixed it by using a set to get the unique names.

That sounds reasonable.

-- 
David Kågedal

Re: kha/safe and kha/experimental updated

[Jakub Narebski]

Karl Hasselström wrote:
> On 2007-12-19 11:44:57 +0100, Jakub Narebski wrote:
>> On Wed, 19 Dec 2007, Karl Hasselström wrote:
>>> On 2007-12-18 08:39:52 -0800, Jakub Narebski wrote:
>>>
>>>> I also would like to have this command kept (and shown in 'stg
>>>> help'!). Contrary to 'git add' it can check and add to index /
>>>> update index only for files with conflict; we have -r
>>>> (ancestor|current|patched) to choose one side, and we could add
>>>> --check to check if there are no conflict markers with files
>>>> (useful with -a/--all).
>>>
>>> This too sounds like stuff that could profitably be added to "git
>>> add". Except for the ancestor/current/patched words, it is not
>>> specific to patch stacks, so the implementation should be in git
>>> and not in stg.
>>
>> No it cannot, at least the '-r (ancestor|current|patched)' part for
>> resetting file to given version, even if we change the wording to
>> ancestor, ours and theirs. The git-add command is about adding
>> contents, which updates index, which almost as a intended
>> side-effect clears merge state, i.e. stages; and not about resetting
>> to stage.
> 
>   git checkout-index --stage=1|2|3 <filename>
> 
> does what you want. But "git cat-file" knows this handy syntax for
> getting at particular index stages:
> 
>   :stage:<filename>

I always forget which stage is which. It would be nice if 
git-checkout-index implemented human-friendly names, just like 
git-diff-files has --base, --ours, --theirs, i.e. if it would be 
possible to write

  git checkout-index --stage=base|ours|theirs <filename>

and perhaps even

  :base|ours|theirs:<filename>.

(but there is a problem with files containing ':'...).

> It would be very convenient to be able to do
> 
>   git checkout :stage:<filename>
> 
> but it doesn't seem to work currently. Does anyone know the "best" way
> of manually checking out a particular index stage in git?

It's a pity it doesn't work. Or if not this, then perhaps

  git checkout --stage=ours -- <filename>

>> Besides with "stg resolved" you can update index _only_ for files
>> which were in the conflict, also for -a/--all, and not all the files
>> not only those which were in the conflict like "git add -u" does.
> 
> This sounds like a straightforward addition to "git add".
> 
>> "stg resolved --check" could happily ignore things that only look
>> like conflict markers, but must have been intended, because they are
>> in files not in conflict.
> 
> git knows about conflicting files too.
> 
>> Unless you are talking about adding "resolve"/"resolved" command to
>> git-core, as a porcelain wrapper around git-update-index, like "git
>> add"...
> 
> Yes, that's what I want. You and others like what "stg resolved" does,
> but I don't want it in StGit because it's a generic git enhancement
> that has nothing to with patch stacks. People who don't use StGit
> would presumably like it as well.

You mean adding another option to git-add, similar to '-u' option, but
updating only the files which were (are) in merge conflict; 
'-c'/'-r'/'-s' perhaps? This would be replacement for 
"stg resolved --all", wouldn't it (and with filename replacement for 
"stg resolved <filename>", of course)?


P.S. Sidenote: it would be nice if git-mergetool was updated to 
understand StGIT style interactive 2-way and 3-way merge configuration,
and not offer only limited choice of pre-defined interactive merge tools 
(although pre-defined are nice, too).

-- 
Jakub Narebski
Poland

Re: kha/safe and kha/experimental updated

[Catalin Marinas]

On 19/12/2007, Jakub Narebski <jnareb@gmail.com> wrote:
> Karl Hasselström wrote:
> > On 2007-12-19 11:44:57 +0100, Jakub Narebski wrote:
> >> On Wed, 19 Dec 2007, Karl Hasselström wrote:
> >>> On 2007-12-18 08:39:52 -0800, Jakub Narebski wrote:
> >>>
> >>>> I also would like to have this command kept (and shown in 'stg
> >>>> help'!). Contrary to 'git add' it can check and add to index /
> >>>> update index only for files with conflict; we have -r
> >>>> (ancestor|current|patched) to choose one side, and we could add
> >>>> --check to check if there are no conflict markers with files
> >>>> (useful with -a/--all).
> >>>
> >>> This too sounds like stuff that could profitably be added to "git
> >>> add". Except for the ancestor/current/patched words, it is not
> >>> specific to patch stacks, so the implementation should be in git
> >>> and not in stg.
> >>
> >> No it cannot, at least the '-r (ancestor|current|patched)' part for
> >> resetting file to given version, even if we change the wording to
> >> ancestor, ours and theirs. The git-add command is about adding
> >> contents, which updates index, which almost as a intended
> >> side-effect clears merge state, i.e. stages; and not about resetting
> >> to stage.
> >
> >   git checkout-index --stage=1|2|3 <filename>
> >
> > does what you want. But "git cat-file" knows this handy syntax for
> > getting at particular index stages:
> >
> >   :stage:<filename>
>
> I always forget which stage is which. It would be nice if
> git-checkout-index implemented human-friendly names, just like
> git-diff-files has --base, --ours, --theirs, i.e. if it would be
> possible to write
>
>   git checkout-index --stage=base|ours|theirs <filename>

This gets even more confusing with StGIT. For plain git, after a git
merge or pull conflict, 'theirs' is stage 3. With StGIT, we first
advance the base of the stack and merge the patches onto it, in which
case the 'patched' (which I would normally call 'ours' rather than
'theirs') is 3.

-- 
Catalin

Re: kha/safe and kha/experimental updated

[David Kågedal]

"Catalin Marinas" <catalin.marinas@gmail.com> writes:

>> I always forget which stage is which. It would be nice if
>> git-checkout-index implemented human-friendly names, just like
>> git-diff-files has --base, --ours, --theirs, i.e. if it would be
>> possible to write
>>
>>   git checkout-index --stage=base|ours|theirs <filename>
>
> This gets even more confusing with StGIT. For plain git, after a git
> merge or pull conflict, 'theirs' is stage 3. With StGIT, we first
> advance the base of the stack and merge the patches onto it, in which
> case the 'patched' (which I would normally call 'ours' rather than
> 'theirs') is 3.

True. And the same problem obviously exists for "git rebase".

-- 
David Kågedal

Re: kha/safe and kha/experimental updated

[Karl Hasselström]

On 2007-12-19 17:23:27 +0100, Jakub Narebski wrote:

> Karl Hasselström wrote:
>
> >   git checkout-index --stage=1|2|3 <filename>
> >
> > does what you want. But "git cat-file" knows this handy syntax for
> > getting at particular index stages:
> >
> >   :stage:<filename>
>
> I always forget which stage is which. It would be nice if
> git-checkout-index implemented human-friendly names, just like
> git-diff-files has --base, --ours, --theirs, i.e. if it would be
> possible to write
>
>   git checkout-index --stage=base|ours|theirs <filename>
>
> and perhaps even
>
>   :base|ours|theirs:<filename>.
>
> (but there is a problem with files containing ':'...).

<ref>:<path> is already accepted, so I don't think this would be much
worse.

And yes, I agree that the base/ours/theirs aliases are nice, and
should be accepted by at least the porcelain commands.

> > Yes, that's what I want. You and others like what "stg resolved"
> > does, but I don't want it in StGit because it's a generic git
> > enhancement that has nothing to with patch stacks. People who
> > don't use StGit would presumably like it as well.
>
> You mean adding another option to git-add, similar to '-u' option,
> but updating only the files which were (are) in merge conflict;
> '-c'/'-r'/'-s' perhaps?

Yes. That _is_ the operation you want, isn't it?

> This would be replacement for "stg resolved --all", wouldn't it (and
> with filename replacement for "stg resolved <filename>", of course)?

Yes. Though with a filename argument, you wouldn't need the new flag.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

kha/safe and kha/experimental updated

[Karl Hasselström]

I'll follow up with the three patch series in here that are new.

The following changes since commit cd885e085a697d5377956d5beb30e6030f224ccd:
  Peter Oberndorfer (1):
        replace "git repo-config" usage by "git config"

are available in the git repository at:

  git://repo.or.cz/stgit/kha.git safe

Karl Hasselström (10):
      Don't keep old committer when rewriting a commit
      Homogenize buffer names
      Remove unused default values
      Refactor --diff-opts handling
      Create index and worktree objects just once
      Wrap excessively long line
      Eliminate temp variable that's used just once
      Simplify editor selection logic
      Let the caller supply the diff text to diffstat()
      Don't clean away patches with conflicts

Pavel Roskin (1):
      Add test to ensure that "stg clean" preserves conflicting patches

 contrib/stgit.el           |    6 +++---
 stgit/commands/clean.py    |    7 +++++++
 stgit/commands/coalesce.py |    2 +-
 stgit/commands/commit.py   |    2 +-
 stgit/commands/common.py   |    3 ++-
 stgit/commands/diff.py     |   19 ++++++-------------
 stgit/commands/edit.py     |   16 ++++------------
 stgit/commands/export.py   |   20 +++++++-------------
 stgit/commands/files.py    |   15 +++++----------
 stgit/commands/goto.py     |    2 +-
 stgit/commands/mail.py     |   26 +++++++++-----------------
 stgit/commands/status.py   |   17 +++++------------
 stgit/git.py               |    9 +++------
 stgit/lib/git.py           |   42 ++++++++++++++++++++++++++++++++----------
 stgit/lib/transaction.py   |    4 ++--
 stgit/utils.py             |   20 ++++++++++++++------
 t/t2500-clean.sh           |   17 +++++++++++++++++
 17 files changed, 119 insertions(+), 108 deletions(-)

                                 -+-

The following changes since commit 149ad73c6b1639981b1064a9e8f3699b08928621:
  Karl Hasselström (1):
        Don't clean away patches with conflicts

are available in the git repository at:

  git://repo.or.cz/stgit/kha.git experimental

Karl Hasselström (6):
      Let "stg show" use the unified --diff-opts handling
      Read default diff options from the user's config
      Teach new infrastructure about the default author and committer
      Teach new infrastructure to apply patches
      Teach new infrastructure to diff two trees
      Convert "stg edit" to the new infrastructure

Peter Oberndorfer (1):
      Add an --index option to "stg refresh"

 examples/gitconfig        |    4 +
 stgit/commands/edit.py    |  309 +++++++++++++++++++++------------------------
 stgit/commands/refresh.py |   25 +++-
 stgit/commands/show.py    |   13 +--
 stgit/lib/git.py          |   54 ++++++++
 stgit/utils.py            |    3 +-
 t/t2700-refresh.sh        |   57 ++++++++-
 7 files changed, 283 insertions(+), 182 deletions(-)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

[StGit PATCH 0/5] Various cleanups

[Karl Hasselström]

Some of these are very tiny, others are a bit bigger. They're all in
kha/safe, since they should be perfectly safe.

---

Karl Hasselström (5):
      Let the caller supply the diff text to diffstat()
      Simplify editor selection logic
      Eliminate temp variable that's used just once
      Wrap excessively long line
      Create index and worktree objects just once


 stgit/commands/coalesce.py |    2 +-
 stgit/commands/commit.py   |    2 +-
 stgit/commands/common.py   |    3 ++-
 stgit/commands/diff.py     |    9 ++++-----
 stgit/commands/edit.py     |    2 +-
 stgit/commands/export.py   |   10 +++++-----
 stgit/commands/files.py    |    2 +-
 stgit/commands/goto.py     |    2 +-
 stgit/commands/mail.py     |   16 +++++++---------
 stgit/git.py               |    9 +++------
 stgit/lib/git.py           |   34 ++++++++++++++++++++++++----------
 stgit/lib/transaction.py   |    3 +--
 stgit/utils.py             |    8 ++------
 13 files changed, 53 insertions(+), 49 deletions(-)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

[StGit PATCH 1/5] Create index and worktree objects just once

[Karl Hasselström]

Create the objects for a repository's default index, worktree, and
index+worktree just once. Both for performance (though the gain is
probably negligible), and for future-proofing if we ever add mutable
state to those objects.

And make them properties while we're at it.

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/commands/coalesce.py |    2 +-
 stgit/commands/commit.py   |    2 +-
 stgit/commands/goto.py     |    2 +-
 stgit/lib/git.py           |   34 ++++++++++++++++++++++++----------
 4 files changed, 27 insertions(+), 13 deletions(-)


diff --git a/stgit/commands/coalesce.py b/stgit/commands/coalesce.py
index d2cba3e..291a537 100644
--- a/stgit/commands/coalesce.py
+++ b/stgit/commands/coalesce.py
@@ -118,5 +118,5 @@ def func(parser, options, args):
                                           + list(stack.patchorder.unapplied)))
     if len(patches) < 2:
         raise common.CmdException('Need at least two patches')
-    return _coalesce(stack, stack.repository.default_iw(), options.name,
+    return _coalesce(stack, stack.repository.default_iw, options.name,
                      options.message, options.save_template, patches)
diff --git a/stgit/commands/commit.py b/stgit/commands/commit.py
index 1d741b3..bff94ce 100644
--- a/stgit/commands/commit.py
+++ b/stgit/commands/commit.py
@@ -68,7 +68,7 @@ def func(parser, options, args):
     if not patches:
         raise common.CmdException('No patches to commit')
 
-    iw = stack.repository.default_iw()
+    iw = stack.repository.default_iw
     trans = transaction.StackTransaction(stack, 'stg commit')
     try:
         common_prefix = 0
diff --git a/stgit/commands/goto.py b/stgit/commands/goto.py
index 763a8af..fe13e49 100644
--- a/stgit/commands/goto.py
+++ b/stgit/commands/goto.py
@@ -35,7 +35,7 @@ def func(parser, options, args):
     patch = args[0]
 
     stack = directory.repository.current_stack
-    iw = stack.repository.default_iw()
+    iw = stack.repository.default_iw
     trans = transaction.StackTransaction(stack, 'stg goto')
     if patch in trans.applied:
         to_pop = set(trans.applied[trans.applied.index(patch)+1:])
diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 118c9b2..2af1844 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -211,6 +211,9 @@ class Repository(RunWithEnv):
         self.__refs = Refs(self)
         self.__trees = ObjectCache(lambda sha1: Tree(sha1))
         self.__commits = ObjectCache(lambda sha1: Commit(self, sha1))
+        self.__default_index = None
+        self.__default_worktree = None
+        self.__default_iw = None
     env = property(lambda self: { 'GIT_DIR': self.__git_dir })
     @classmethod
     def default(cls):
@@ -220,21 +223,32 @@ class Repository(RunWithEnv):
                                ).output_one_line())
         except run.RunException:
             raise RepositoryException('Cannot find git repository')
+    @property
     def default_index(self):
-        return Index(self, (os.environ.get('GIT_INDEX_FILE', None)
-                            or os.path.join(self.__git_dir, 'index')))
+        if self.__default_index == None:
+            self.__default_index = Index(
+                self, (os.environ.get('GIT_INDEX_FILE', None)
+                       or os.path.join(self.__git_dir, 'index')))
+        return self.__default_index
     def temp_index(self):
         return Index(self, self.__git_dir)
+    @property
     def default_worktree(self):
-        path = os.environ.get('GIT_WORK_TREE', None)
-        if not path:
-            o = run.Run('git', 'rev-parse', '--show-cdup').output_lines()
-            o = o or ['.']
-            assert len(o) == 1
-            path = o[0]
-        return Worktree(path)
+        if self.__default_worktree == None:
+            path = os.environ.get('GIT_WORK_TREE', None)
+            if not path:
+                o = run.Run('git', 'rev-parse', '--show-cdup').output_lines()
+                o = o or ['.']
+                assert len(o) == 1
+                path = o[0]
+            self.__default_worktree = Worktree(path)
+        return self.__default_worktree
+    @property
     def default_iw(self):
-        return IndexAndWorktree(self.default_index(), self.default_worktree())
+        if self.__default_iw == None:
+            self.__default_iw = IndexAndWorktree(self.default_index,
+                                                 self.default_worktree)
+        return self.__default_iw
     directory = property(lambda self: self.__git_dir)
     refs = property(lambda self: self.__refs)
     def cat_object(self, sha1):

[StGit PATCH 2/5] Wrap excessively long line

[Karl Hasselström]

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

I try to kill those whenever I can get away with it ...

 stgit/commands/common.py |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)


diff --git a/stgit/commands/common.py b/stgit/commands/common.py
index 7d9df02..7a7cb80 100644
--- a/stgit/commands/common.py
+++ b/stgit/commands/common.py
@@ -274,7 +274,8 @@ def name_email(address):
     if not str_list:
         str_list = re.findall('^(.*)\s*\((.*)\)\s*$', address)
         if not str_list:
-            raise CmdException, 'Incorrect "name <email>"/"email (name)" string: %s' % address
+            raise CmdException('Incorrect "name <email>"/"email (name)"'
+                               ' string: %s' % address)
         return ( str_list[0][1], str_list[0][0] )
 
     return str_list[0]

[StGit PATCH 3/5] Eliminate temp variable that's used just once

[Karl Hasselström]

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/lib/transaction.py |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)


diff --git a/stgit/lib/transaction.py b/stgit/lib/transaction.py
index 809eabf..6a2ed81 100644
--- a/stgit/lib/transaction.py
+++ b/stgit/lib/transaction.py
@@ -174,7 +174,6 @@ class StackTransaction(object):
         """Attempt to push the named patch. If this results in conflicts,
         halts the transaction. If index+worktree are given, spill any
         conflicts to them."""
-        i = self.unapplied.index(pn)
         cd = self.patches[pn].data
         cd = cd.set_committer(None)
         s = ['', ' (empty)'][cd.is_nochange()]
@@ -203,7 +202,7 @@ class StackTransaction(object):
                 s = ' (conflict)'
         cd = cd.set_tree(tree)
         self.patches[pn] = self.__stack.repository.commit(cd)
-        del self.unapplied[i]
+        del self.unapplied[self.unapplied.index(pn)]
         self.applied.append(pn)
         out.info('Pushed %s%s' % (pn, s))
         if merge_conflict:

[StGit PATCH 4/5] Simplify editor selection logic

[Karl Hasselström]

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

There are a few other env variables we might like to look at, like
VISUAL. And isn't there a git-specific variable too?

 stgit/utils.py |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)


diff --git a/stgit/utils.py b/stgit/utils.py
index 00776b0..43366c9 100644
--- a/stgit/utils.py
+++ b/stgit/utils.py
@@ -175,12 +175,8 @@ def call_editor(filename):
 
     # the editor
     editor = config.get('stgit.editor')
-    if editor:
-        pass
-    elif 'EDITOR' in os.environ:
-        editor = os.environ['EDITOR']
-    else:
-        editor = 'vi'
+    if not editor:
+        editor = os.environ.get('EDITOR', 'vi')
     editor += ' %s' % filename
 
     out.start('Invoking the editor: "%s"' % editor)

[StGit PATCH 5/5] Let the caller supply the diff text to diffstat()

[Karl Hasselström]

Almost all diffstat() callers already have the diff text, so they
might as well pass it to diffstat() instead of letting it recompute
it. In some cases this even makes for a code simplification since the
diff (and thus diffstat) parameters were nontrivial.

Also, diffstat() wasn't as versatile as diff(); for example, it didn't
accept any extra diff options. This patch solves all of those problems
as a side-effect.

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

I wanted this for the "stg edit" rewrite. It took some time before I
saw the TODO comment describing exactly the change I was in the
process of making!

 stgit/commands/diff.py   |    9 ++++-----
 stgit/commands/edit.py   |    2 +-
 stgit/commands/export.py |   10 +++++-----
 stgit/commands/files.py  |    2 +-
 stgit/commands/mail.py   |   16 +++++++---------
 stgit/git.py             |    9 +++------
 6 files changed, 21 insertions(+), 27 deletions(-)


diff --git a/stgit/commands/diff.py b/stgit/commands/diff.py
index 7c213d1..fd6be34 100644
--- a/stgit/commands/diff.py
+++ b/stgit/commands/diff.py
@@ -81,12 +81,11 @@ def func(parser, options, args):
         rev1 = 'HEAD'
         rev2 = None
 
+    diff_str = git.diff(args, git_id(crt_series, rev1),
+                        git_id(crt_series, rev2),
+                        diff_flags = options.diff_flags)
     if options.stat:
-        out.stdout_raw(git.diffstat(args, git_id(crt_series, rev1),
-                                    git_id(crt_series, rev2)) + '\n')
+        out.stdout_raw(git.diffstat(diff_str) + '\n')
     else:
-        diff_str = git.diff(args, git_id(crt_series, rev1),
-                            git_id(crt_series, rev2),
-                            diff_flags = options.diff_flags)
         if diff_str:
             pager(diff_str)
diff --git a/stgit/commands/edit.py b/stgit/commands/edit.py
index da67275..9915e49 100644
--- a/stgit/commands/edit.py
+++ b/stgit/commands/edit.py
@@ -154,9 +154,9 @@ def __generate_file(pname, write_fn, options):
                 '%(diffstat)s\n' \
                 '%(diff)s'
 
-        tmpl_dict['diffstat'] = git.diffstat(rev1 = bottom, rev2 = top)
         tmpl_dict['diff'] = git.diff(rev1 = bottom, rev2 = top,
                                      diff_flags = options.diff_flags)
+        tmpl_dict['diffstat'] = git.diffstat(tmpl_dict['diff'])
 
     for key in tmpl_dict:
         # make empty strings if key is not available
diff --git a/stgit/commands/export.py b/stgit/commands/export.py
index 16c64ba..50f6f67 100644
--- a/stgit/commands/export.py
+++ b/stgit/commands/export.py
@@ -138,11 +138,13 @@ def func(parser, options, args):
         long_descr = reduce(lambda x, y: x + '\n' + y,
                             descr_lines[1:], '').strip()
 
+        diff = git.diff(rev1 = patch.get_bottom(),
+                        rev2 = patch.get_top(),
+                        diff_flags = options.diff_flags)
         tmpl_dict = {'description': patch.get_description().rstrip(),
                      'shortdescr': short_descr,
                      'longdescr': long_descr,
-                     'diffstat': git.diffstat(rev1 = patch.get_bottom(),
-                                              rev2 = patch.get_top()),
+                     'diffstat': git.diffstat(diff),
                      'authname': patch.get_authname(),
                      'authemail': patch.get_authemail(),
                      'authdate': patch.get_authdate(),
@@ -172,9 +174,7 @@ def func(parser, options, args):
             print '-'*79
 
         f.write(descr)
-        f.write(git.diff(rev1 = patch.get_bottom(),
-                         rev2 = patch.get_top(),
-                         diff_flags = options.diff_flags))
+        f.write(diff)
         if not options.stdout:
             f.close()
         patch_no += 1
diff --git a/stgit/commands/files.py b/stgit/commands/files.py
index ab1f6a3..b43b12f 100644
--- a/stgit/commands/files.py
+++ b/stgit/commands/files.py
@@ -60,7 +60,7 @@ def func(parser, options, args):
     rev2 = git_id(crt_series, '%s//top' % patch)
 
     if options.stat:
-        out.stdout_raw(git.diffstat(rev1 = rev1, rev2 = rev2) + '\n')
+        out.stdout_raw(git.diffstat(git.diff(rev1 = rev1, rev2 = rev2)) + '\n')
     elif options.bare:
         out.stdout_raw(git.barefiles(rev1, rev2) + '\n')
     else:
diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index 4aa16fb..7d19eca 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py
@@ -360,9 +360,9 @@ def __build_cover(tmpl, patches, msg_id, options):
                  'number':       number_str,
                  'shortlog':     stack.shortlog(crt_series.get_patch(p)
                                                 for p in patches),
-                 'diffstat':     git.diffstat(
+                 'diffstat':     git.diffstat(git.diff(
                      rev1 = git_id(crt_series, '%s//bottom' % patches[0]),
-                     rev2 = git_id(crt_series, '%s//top' % patches[-1]))}
+                     rev2 = git_id(crt_series, '%s//top' % patches[-1])))}
 
     try:
         msg_string = tmpl % tmpl_dict
@@ -433,6 +433,9 @@ def __build_message(tmpl, patch, patch_nr, total_nr, msg_id, ref_id, options):
     else:
         number_str = ''
 
+    diff = git.diff(rev1 = git_id(crt_series, '%s//bottom' % patch),
+                    rev2 = git_id(crt_series, '%s//top' % patch),
+                    diff_flags = options.diff_flags)
     tmpl_dict = {'patch':        patch,
                  'sender':       sender,
                  # for backward template compatibility
@@ -441,13 +444,8 @@ def __build_message(tmpl, patch, patch_nr, total_nr, msg_id, ref_id, options):
                  'longdescr':    long_descr,
                  # for backward template compatibility
                  'endofheaders': '',
-                 'diff':         git.diff(
-                     rev1 = git_id(crt_series, '%s//bottom' % patch),
-                     rev2 = git_id(crt_series, '%s//top' % patch),
-                     diff_flags = options.diff_flags),
-                 'diffstat':     git.diffstat(
-                     rev1 = git_id(crt_series, '%s//bottom'%patch),
-                     rev2 = git_id(crt_series, '%s//top' % patch)),
+                 'diff':         diff,
+                 'diffstat':     git.diffstat(diff),
                  # for backward template compatibility
                  'date':         '',
                  'version':      version_str,
diff --git a/stgit/git.py b/stgit/git.py
index 85cceb0..4dc4dcf 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -640,12 +640,9 @@ def diff(files = None, rev1 = 'HEAD', rev2 = None, diff_flags = [],
     else:
         return ''
 
-# TODO: take another parameter representing a diff string as we
-# usually invoke git.diff() form the calling functions
-def diffstat(files = None, rev1 = 'HEAD', rev2 = None):
-    """Return the diffstat between rev1 and rev2."""
-    return GRun('apply', '--stat', '--summary'
-                ).raw_input(diff(files, rev1, rev2)).raw_output()
+def diffstat(diff):
+    """Return the diffstat of the supplied diff."""
+    return GRun('apply', '--stat', '--summary').raw_input(diff).raw_output()
 
 def files(rev1, rev2, diff_flags = []):
     """Return the files modified between rev1 and rev2

[StGit PATCH 0/2] "stg clean" test+bugfix

[Karl Hasselström]

This is the test by Pavel, followed by a fix by me.

They're in kha/safe, since they should be low-risk, and because losing
a conflicting patch (even though you still have all the changes in
your worktree) is unpleasant.

---

Karl Hasselström (1):
      Don't clean away patches with conflicts

Pavel Roskin (1):
      Add test to ensure that "stg clean" preserves conflicting patches


 stgit/commands/clean.py |    7 +++++++
 stgit/lib/git.py        |    8 ++++++++
 t/t2500-clean.sh        |   17 +++++++++++++++++
 3 files changed, 32 insertions(+), 0 deletions(-)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

[StGit PATCH 1/2] Add test to ensure that "stg clean" preserves conflicting patches

[Karl Hasselström]

From: Pavel Roskin <proski@gnu.org>

If "stg push" fails, the subsequent "stg clean" will remove the patch
that could not be applied. I think it's wrong. Especially when doing
"stg pull", it can happen that I want to run "stg clean" to get rid of
the patches applied upstream so I can concentrate on the conflict.
Instead, the conflicting patch is removed too.

The test added by this patch should pass once the bug is fixed.

Signed-off-by: Pavel Roskin <proski@gnu.org>
Signed-off-by: Karl Hasselström <kha@treskal.com>

---

I doctored the commit message a bit, and made the failing test use
test_expect_failure.

 t/t2500-clean.sh |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)


diff --git a/t/t2500-clean.sh b/t/t2500-clean.sh
index 3364c18..b38d868 100755
--- a/t/t2500-clean.sh
+++ b/t/t2500-clean.sh
@@ -24,4 +24,21 @@ test_expect_success 'Clean empty patches' '
     [ "$(echo $(stg unapplied))" = "" ]
 '
 
+test_expect_success 'Create a conflict' '
+    stg new p1 -m p1 &&
+    echo bar > foo.txt &&
+    stg refresh &&
+    stg pop &&
+    stg new p2 -m p2
+    echo quux > foo.txt &&
+    stg refresh &&
+    ! stg push
+'
+
+test_expect_failure 'Make sure conflicting patches are preserved' '
+    stg clean &&
+    [ "$(echo $(stg applied))" = "p0 p2 p1" ] &&
+    [ "$(echo $(stg unapplied))" = "" ]
+'
+
 test_done

[StGit PATCH 2/2] Don't clean away patches with conflicts

[Karl Hasselström]

If we have conflicts, it means that the topmost patch is empty because
of those conflicts (since StGit explicitly makes a conflicting patch
empty), so don't let "stg clean" touch it.

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

I considered fixing this by way of a check in the shared
new-infrastructure code, but came to the conclusion that the check
really does belong in "stg clean".

 stgit/commands/clean.py |    7 +++++++
 stgit/lib/git.py        |    8 ++++++++
 t/t2500-clean.sh        |    2 +-
 3 files changed, 16 insertions(+), 1 deletions(-)


diff --git a/stgit/commands/clean.py b/stgit/commands/clean.py
index a0a0dca..889c1dc 100644
--- a/stgit/commands/clean.py
+++ b/stgit/commands/clean.py
@@ -40,6 +40,13 @@ def _clean(stack, clean_applied, clean_unapplied):
     trans = transaction.StackTransaction(stack, 'stg clean')
     def del_patch(pn):
         if pn in stack.patchorder.applied:
+            if pn == stack.patchorder.applied[-1]:
+                # We're about to clean away the topmost patch. Don't
+                # do that if we have conflicts, since that means the
+                # patch is only empty because the conflicts have made
+                # us dump its contents into the index and worktree.
+                if stack.repository.default_index.conflicts():
+                    return False
             return clean_applied and trans.patches[pn].data.is_nochange()
         elif pn in stack.patchorder.unapplied:
             return clean_unapplied and trans.patches[pn].data.is_nochange()
diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 2af1844..6cd7450 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -353,6 +353,14 @@ class Index(RunWithEnv):
     def delete(self):
         if os.path.isfile(self.__filename):
             os.remove(self.__filename)
+    def conflicts(self):
+        """The set of conflicting paths."""
+        paths = set()
+        for line in self.run(['git', 'ls-files', '-z', '--unmerged']
+                             ).raw_output().split('\0')[:-1]:
+            stat, path = line.split('\t', 1)
+            paths.add(path)
+        return paths
 
 class Worktree(object):
     def __init__(self, directory):
diff --git a/t/t2500-clean.sh b/t/t2500-clean.sh
index b38d868..ad8f892 100755
--- a/t/t2500-clean.sh
+++ b/t/t2500-clean.sh
@@ -35,7 +35,7 @@ test_expect_success 'Create a conflict' '
     ! stg push
 '
 
-test_expect_failure 'Make sure conflicting patches are preserved' '
+test_expect_success 'Make sure conflicting patches are preserved' '
     stg clean &&
     [ "$(echo $(stg applied))" = "p0 p2 p1" ] &&
     [ "$(echo $(stg unapplied))" = "" ]

[StGit PATCH 0/4] Rewrite "stg edit" to use new infrastructure

[Karl Hasselström]

This is in kha/experimental. We hardly have any tests for "stg edit",
and the control flow is kind of complicated.

---

Karl Hasselström (4):
      Convert "stg edit" to the new infrastructure
      Teach new infrastructure to diff two trees
      Teach new infrastructure to apply patches
      Teach new infrastructure about the default author and committer


 stgit/commands/edit.py |  309 ++++++++++++++++++++++--------------------------
 stgit/lib/git.py       |   54 ++++++++
 2 files changed, 196 insertions(+), 167 deletions(-)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

[StGit PATCH 1/4] Teach new infrastructure about the default author and committer

[Karl Hasselström]

As specified by the config options user.name and user.email, and the
environment variables GIT_{AUTHOR,COMMITTER}_{NAME,EMAIL,DATE} (the
latter overriding the former).

Nothing uses this yet, but "stg edit" will soon.

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/lib/git.py |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)


diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 6cd7450..8678979 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -52,6 +52,30 @@ class Person(Repr):
         email = m.group(2)
         date = m.group(3)
         return cls(name, email, date)
+    @classmethod
+    def user(cls):
+        if not hasattr(cls, '__user'):
+            cls.__user = cls(name = config.get('user.name'),
+                             email = config.get('user.email'))
+        return cls.__user
+    @classmethod
+    def author(cls):
+        if not hasattr(cls, '__author'):
+            cls.__author = cls(
+                name = os.environ.get('GIT_AUTHOR_NAME', NoValue),
+                email = os.environ.get('GIT_AUTHOR_EMAIL', NoValue),
+                date = os.environ.get('GIT_AUTHOR_DATE', NoValue),
+                defaults = cls.user())
+        return cls.__author
+    @classmethod
+    def committer(cls):
+        if not hasattr(cls, '__committer'):
+            cls.__committer = cls(
+                name = os.environ.get('GIT_COMMITTER_NAME', NoValue),
+                email = os.environ.get('GIT_COMMITTER_EMAIL', NoValue),
+                date = os.environ.get('GIT_COMMITTER_DATE', NoValue),
+                defaults = cls.user())
+        return cls.__committer
 
 class Tree(Repr):
     """Immutable."""

[StGit PATCH 2/4] Teach new infrastructure to apply patches

[Karl Hasselström]

Two new methods: one index method that applies a patch to that index
or fails without side-effects (without touching a worktree in either
case); and one repository method that uses a temp index to apply a
patch to a tree and returning the new tree (or None if the application
failed), entirely side-effect free.

Nothing uses this yet, but "stg edit" will soon.

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/lib/git.py |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)


diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 8678979..9cb2521 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -338,6 +338,23 @@ class Repository(RunWithEnv):
                 return None
         finally:
             index.delete()
+    def apply(self, tree, patch_text):
+        """Given a tree and a patch, will either return the new tree that
+        results when the patch is applied, or None if the patch
+        couldn't be applied."""
+        assert isinstance(tree, Tree)
+        if not patch_text:
+            return tree
+        index = self.temp_index()
+        try:
+            index.read_tree(tree)
+            try:
+                index.apply(patch_text)
+                return index.write_tree()
+            except MergeException:
+                return None
+        finally:
+            index.delete()
 
 class MergeException(exception.StgException):
     pass
@@ -374,6 +391,13 @@ class Index(RunWithEnv):
         """In-index merge, no worktree involved."""
         self.run(['git', 'read-tree', '-m', '-i', '--aggressive',
                   base.sha1, ours.sha1, theirs.sha1]).no_output()
+    def apply(self, patch_text):
+        """In-index patch application, no worktree involved."""
+        try:
+            self.run(['git', 'apply', '--cached']
+                     ).raw_input(patch_text).no_output()
+        except run.RunException:
+            raise MergeException('Patch does not apply cleanly')
     def delete(self):
         if os.path.isfile(self.__filename):
             os.remove(self.__filename)

[StGit PATCH 3/4] Teach new infrastructure to diff two trees

[Karl Hasselström]

Nothing uses this yet, but "stg edit" will soon.

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/lib/git.py |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)


diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 9cb2521..d75f724 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -1,5 +1,6 @@
 import os, os.path, re
 from stgit import exception, run, utils
+from stgit.config import config
 
 class RepositoryException(exception.StgException):
     pass
@@ -355,6 +356,11 @@ class Repository(RunWithEnv):
                 return None
         finally:
             index.delete()
+    def diff_tree(self, t1, t2, diff_opts):
+        assert isinstance(t1, Tree)
+        assert isinstance(t2, Tree)
+        return self.run(['git', 'diff-tree', '-p'] + list(diff_opts)
+                        + [t1.sha1, t2.sha1]).raw_output()
 
 class MergeException(exception.StgException):
     pass

[StGit PATCH 4/4] Convert "stg edit" to the new infrastructure

[Karl Hasselström]

The --annotate and --undo switches were dropped in the conversion.
--annotate could be re-added, but --undo is more problematic since the
command will now rewrite any applied patches on top of the edited
patch. It seems best to leave this job to the fabled general undo
command, expected Real Soon Now.

In addition to the usual improvements from the new infrastructure,
this patch has some additional benefits:

  * There's a new -e/--edit flag, which forces interactive editing
    even if options such as --sign or --author are given. (Normally,
    interactive editing is skipped if the patch is modified with a
    commandline option.)

  * It's now possible to edit any patch, including unapplied patches.
    Even diff editing works for all patches, including unapplied
    patches. (In fact, editing unapplied patches is slightly safer,
    since they don't mind a dirty index/worktree.)

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

Testers welcome!

 stgit/commands/edit.py |  309 ++++++++++++++++++++++--------------------------
 1 files changed, 142 insertions(+), 167 deletions(-)


diff --git a/stgit/commands/edit.py b/stgit/commands/edit.py
index 9915e49..b42728f 100644
--- a/stgit/commands/edit.py
+++ b/stgit/commands/edit.py
@@ -18,14 +18,12 @@ along with this program; if not, write to the Free Software
 Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 """
 
-from optparse import OptionParser, make_option
-from email.Utils import formatdate
+from optparse import make_option
 
-from stgit.commands.common import *
-from stgit.utils import *
+from stgit import git, utils
+from stgit.commands import common
+from stgit.lib import git as gitlib, transaction
 from stgit.out import *
-from stgit import stack, git
-
 
 help = 'edit a patch description or diff'
 usage = """%prog [options] [<patch>]
@@ -49,23 +47,19 @@ separator:
   Diff text
 
 Command-line options can be used to modify specific information
-without invoking the editor.
+without invoking the editor. (With the --edit option, the editor is
+invoked even if such command-line options are given.)
 
-If the patch diff is edited but the patch application fails, the
-rejected patch is stored in the .stgit-failed.patch file (and also in
-.stgit-edit.{diff,txt}). The edited patch can be replaced with one of
-these files using the '--file' and '--diff' options.
-"""
+If the patch diff is edited but does not apply, no changes are made to
+the patch at all. The edited patch is saved to a file which you can
+feed to "stg edit --file", once you have made sure it does apply."""
 
-directory = DirectoryGotoToplevel()
+directory = common.DirectoryHasRepositoryLib()
 options = [make_option('-d', '--diff',
                        help = 'edit the patch diff',
                        action = 'store_true'),
-           make_option('--undo',
-                       help = 'revert the commit generated by the last edit',
-                       action = 'store_true'),
-           make_option('-a', '--annotate', metavar = 'NOTE',
-                       help = 'annotate the patch log entry'),
+           make_option('-e', '--edit', action = 'store_true',
+                       help = 'invoke interactive editor'),
            make_option('--author', metavar = '"NAME <EMAIL>"',
                        help = 'replae the author details with "NAME <EMAIL>"'),
            make_option('--authname',
@@ -78,162 +72,143 @@ options = [make_option('-d', '--diff',
                        help = 'replace the committer name with COMMNAME'),
            make_option('--commemail',
                        help = 'replace the committer e-mail with COMMEMAIL')
-           ] + (make_sign_options() + make_message_options()
-                + make_diff_opts_option())
-
-def __update_patch(pname, text, options):
-    """Update the current patch from the given text.
-    """
-    patch = crt_series.get_patch(pname)
-
-    bottom = patch.get_bottom()
-    top = patch.get_top()
-
-    if text:
-        (message, author_name, author_email, author_date, diff
-         ) = parse_patch(text)
-    else:
-        message = author_name = author_email = author_date = diff = None
-
-    out.start('Updating patch "%s"' % pname)
-
-    if options.diff:
-        git.switch(bottom)
-        try:
-            git.apply_patch(diff = diff)
-        except:
-            # avoid inconsistent repository state
-            git.switch(top)
-            raise
-
-    def c(a, b):
-        if a != None:
-            return a
-        return b
-    crt_series.refresh_patch(message = message,
-                             author_name = c(options.authname, author_name),
-                             author_email = c(options.authemail, author_email),
-                             author_date = c(options.authdate, author_date),
-                             committer_name = options.commname,
-                             committer_email = options.commemail,
-                             backup = True, sign_str = options.sign_str,
-                             log = 'edit', notes = options.annotate)
-
-    if crt_series.empty_patch(pname):
-        out.done('empty patch')
-    else:
-        out.done()
-
-def __generate_file(pname, write_fn, options):
-    """Generate a file containing the description to edit
-    """
-    patch = crt_series.get_patch(pname)
-
-    # generate the file to be edited
-    descr = patch.get_description().strip()
-    authdate = patch.get_authdate()
-
-    tmpl = 'From: %(authname)s <%(authemail)s>\n'
-    if authdate:
-        tmpl += 'Date: %(authdate)s\n'
-    tmpl += '\n%(descr)s\n'
-
-    tmpl_dict = {
-        'descr': descr,
-        'authname': patch.get_authname(),
-        'authemail': patch.get_authemail(),
-        'authdate': patch.get_authdate()
-        }
-
-    if options.diff:
-        # add the patch diff to the edited file
-        bottom = patch.get_bottom()
-        top = patch.get_top()
+           ] + (utils.make_sign_options() + utils.make_message_options()
+                + utils.make_diff_opts_option())
 
-        tmpl += '---\n\n' \
-                '%(diffstat)s\n' \
-                '%(diff)s'
-
-        tmpl_dict['diff'] = git.diff(rev1 = bottom, rev2 = top,
-                                     diff_flags = options.diff_flags)
-        tmpl_dict['diffstat'] = git.diffstat(tmpl_dict['diff'])
-
-    for key in tmpl_dict:
-        # make empty strings if key is not available
-        if tmpl_dict[key] is None:
-            tmpl_dict[key] = ''
-
-    text = tmpl % tmpl_dict
-
-    # write the file to be edited
-    write_fn(text)
-
-def __edit_update_patch(pname, options):
-    """Edit the given patch interactively.
-    """
-    if options.diff:
-        fname = '.stgit-edit.diff'
+def patch_diff(repository, cd, diff, diff_flags):
+    if diff:
+        diff = repository.diff_tree(cd.parent.data.tree, cd.tree, diff_flags)
+        return '\n'.join([git.diffstat(diff), diff])
     else:
-        fname = '.stgit-edit.txt'
-    def write_fn(text):
-        f = file(fname, 'w')
-        f.write(text)
-        f.close()
-
-    __generate_file(pname, write_fn, options)
-
-    # invoke the editor
-    call_editor(fname)
-
-    __update_patch(pname, file(fname).read(), options)
+        return None
+
+def patch_description(cd, diff):
+    """Generate a string containing the description to edit."""
+
+    desc = ['From: %s <%s>' % (cd.author.name, cd.author.email),
+            'Date: %s' % cd.author.date,
+            '',
+            cd.message]
+    if diff:
+        desc += ['---',
+                 '',
+                diff]
+    return '\n'.join(desc)
+
+def patch_desc(repository, cd, failed_diff, diff, diff_flags):
+    return patch_description(cd, failed_diff or patch_diff(
+            repository, cd, diff, diff_flags))
+
+def update_patch_description(repository, cd, text):
+    message, authname, authemail, authdate, diff = common.parse_patch(text)
+    cd = (cd.set_message(message)
+            .set_author(cd.author.set_name(authname)
+                                 .set_email(authemail)
+                                 .set_date(authdate)))
+    failed_diff = None
+    if diff:
+        tree = repository.apply(cd.parent.data.tree, diff)
+        if tree == None:
+            failed_diff = diff
+        else:
+            cd = cd.set_tree(tree)
+    return cd, failed_diff
 
 def func(parser, options, args):
     """Edit the given patch or the current one.
     """
-    crt_pname = crt_series.get_current()
+    stack = directory.repository.current_stack
 
-    if not args:
-        pname = crt_pname
-        if not pname:
-            raise CmdException, 'No patches applied'
+    if len(args) == 0:
+        if not stack.patchorder.applied:
+            raise CmdException(
+                'Cannot edit top patch, because no patches are applied')
+        patchname = stack.patchorder.applied[-1]
     elif len(args) == 1:
-        pname = args[0]
-        if crt_series.patch_unapplied(pname) or crt_series.patch_hidden(pname):
-            raise CmdException, 'Cannot edit unapplied or hidden patches'
-        elif not crt_series.patch_applied(pname):
-            raise CmdException, 'Unknown patch "%s"' % pname
+        [patchname] = args
+        if not stack.patches.exists(patchname):
+            raise CmdException('%s: no such patch' % patchname)
     else:
-        parser.error('incorrect number of arguments')
-
-    check_local_changes()
-    check_conflicts()
-    check_head_top_equal(crt_series)
-
-    if pname != crt_pname:
-        # Go to the patch to be edited
-        applied = crt_series.get_applied()
-        between = applied[:applied.index(pname):-1]
-        pop_patches(crt_series, between)
-
-    if options.author:
-        options.authname, options.authemail = name_email(options.author)
-
-    if options.undo:
-        out.start('Undoing the editing of "%s"' % pname)
-        crt_series.undo_refresh()
-        out.done()
-    elif options.save_template:
-        __generate_file(pname, options.save_template, options)
-    elif any([options.message, options.authname, options.authemail,
-              options.authdate, options.commname, options.commemail,
-              options.sign_str]):
-        out.start('Updating patch "%s"' % pname)
-        __update_patch(pname, options.message, options)
-        out.done()
-    else:
-        __edit_update_patch(pname, options)
+        parser.error('Cannot edit more than one patch')
+
+    cd = orig_cd = stack.patches.get(patchname).commit.data
 
-    if pname != crt_pname:
-        # Push the patches back
-        between.reverse()
-        push_patches(crt_series, between)
+    # Read patch from user-provided description.
+    if options.message == None:
+        failed_diff = None
+    else:
+        cd, failed_diff = update_patch_description(stack.repository, cd,
+                                                   options.message)
+
+    # Modify author and committer data.
+    if options.author != None:
+        options.authname, options.authemail = common.name_email(options.author)
+    for p, f, val in [('author', 'name', options.authname),
+                      ('author', 'email', options.authemail),
+                      ('author', 'date', options.authdate),
+                      ('committer', 'name', options.commname),
+                      ('committer', 'email', options.commemail)]:
+        if val != None:
+            cd = getattr(cd, 'set_' + p)(
+                getattr(getattr(cd, p), 'set_' + f)(val))
+
+    # Add Signed-off-by: or similar.
+    if options.sign_str != None:
+        cd = cd.set_message(utils.add_sign_line(
+                cd.message, options.sign_str, gitlib.Person.committer().name,
+                gitlib.Person.committer().email))
+
+    if options.save_template:
+        options.save_template(
+            patch_desc(stack.repository, cd, failed_diff,
+                       options.diff, options.diff_flags))
+        return utils.STGIT_SUCCESS
+
+    # Let user edit the patch manually.
+    if cd == orig_cd or options.edit:
+        fn = '.stgit-edit.' + ['txt', 'patch'][bool(options.diff)]
+        cd, failed_diff = update_patch_description(
+            stack.repository, cd, utils.edit_string(
+                patch_desc(stack.repository, cd, failed_diff,
+                           options.diff, options.diff_flags),
+                fn))
+
+    def failed():
+        fn = '.stgit-failed.patch'
+        f = file(fn, 'w')
+        f.write(patch_desc(stack.repository, cd, failed_diff,
+                           options.diff, options.diff_flags))
+        f.close()
+        out.error('Edited patch did not apply.',
+                  'It has been saved to "%s".' % fn)
+        return utils.STGIT_COMMAND_ERROR
+
+    # If we couldn't apply the patch, fail without even trying to
+    # effect any of the changes.
+    if failed_diff:
+        return failed()
+
+    # The patch applied, so now we have to rewrite the StGit patch
+    # (and any patches on top of it).
+    iw = stack.repository.default_iw
+    trans = transaction.StackTransaction(stack, 'stg edit')
+    if patchname in trans.applied:
+        popped = trans.applied[trans.applied.index(patchname)+1:]
+        assert not trans.pop_patches(lambda pn: pn in popped)
+    else:
+        popped = []
+    trans.patches[patchname] = stack.repository.commit(cd)
+    try:
+        for pn in popped:
+            trans.push_patch(pn, iw)
+    except transaction.TransactionHalted:
+        pass
+    try:
+        # Either a complete success, or a conflict during push. But in
+        # either case, we've successfully effected the edits the user
+        # asked us for.
+        return trans.run(iw)
+    except transaction.TransactionException:
+        # Transaction aborted -- we couldn't check out files due to
+        # dirty index/worktree. The edits were not carried out.
+        return failed()

Re: [StGit PATCH 3/4] Teach new infrastructure to diff two trees

[David Kågedal]

Karl Hasselström <kha@treskal.com> writes:

> Nothing uses this yet, but "stg edit" will soon.
>
> Signed-off-by: Karl Hasselström <kha@treskal.com>
>
> ---
>
>  stgit/lib/git.py |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
>
> diff --git a/stgit/lib/git.py b/stgit/lib/git.py
> index 9cb2521..d75f724 100644
> --- a/stgit/lib/git.py
> +++ b/stgit/lib/git.py
> @@ -1,5 +1,6 @@
>  import os, os.path, re
>  from stgit import exception, run, utils
> +from stgit.config import config

But "config" isn't used in this patch.

>  class RepositoryException(exception.StgException):
>      pass
> @@ -355,6 +356,11 @@ class Repository(RunWithEnv):
>                  return None
>          finally:
>              index.delete()
> +    def diff_tree(self, t1, t2, diff_opts):
> +        assert isinstance(t1, Tree)
> +        assert isinstance(t2, Tree)
> +        return self.run(['git', 'diff-tree', '-p'] + list(diff_opts)
> +                        + [t1.sha1, t2.sha1]).raw_output()
>  
>  class MergeException(exception.StgException):
>      pass
>

-- 
David Kågedal

Re: [StGit PATCH 3/4] Teach new infrastructure to diff two trees

[Karl Hasselström]

On 2008-01-29 15:40:55 +0100, David Kågedal wrote:

> Karl Hasselström <kha@treskal.com> writes:

> > +from stgit.config import config
>
> But "config" isn't used in this patch.

You're right, it's not. It's actually used in patch 1/4, but because
that code is never called until patch 4/4, the interpreter didn't
catch the error.

Thanks, will fix.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 4/5] Simplify editor selection logic

[Peter Oberndorfer]

Subject: [PATCH] use the same search order for picking a editor as git

Signed-off-by: Peter Oberndorfer <kumbayo84@arcor.de>
---
On Dienstag 29 Januar 2008, Karl Hasselström wrote:
> Signed-off-by: Karl Hasselström <kha@treskal.com>
> 
> ---
> 
> There are a few other env variables we might like to look at, like
> VISUAL. And isn't there a git-specific variable too?
> 
>  stgit/utils.py |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/stgit/utils.py b/stgit/utils.py
> index 00776b0..43366c9 100644
> --- a/stgit/utils.py
> +++ b/stgit/utils.py
> @@ -175,12 +175,8 @@ def call_editor(filename):
>  
>      # the editor
>      editor = config.get('stgit.editor')
> -    if editor:
> -        pass
> -    elif 'EDITOR' in os.environ:
> -        editor = os.environ['EDITOR']
> -    else:
> -        editor = 'vi'
> +    if not editor:
> +        editor = os.environ.get('EDITOR', 'vi')
>      editor += ' %s' % filename
>  
>      out.start('Invoking the editor: "%s"' % editor)

Since i personally dislike having a separate config for the editor in git/stgit
i locally use this patch.
unfortunately it makes the whole editor searching thing more complex :-(
But i am sure it is possible to rewrite the code to something easier
with some more python knowledge :-/
So it is not meant for direct applying, just for discussion...

TODO: update sample gitconfig?
TODO: do the same for pager?

 Documentation/stg-new.txt |    6 ++++++
 stgit/utils.py            |   10 +++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/Documentation/stg-new.txt b/Documentation/stg-new.txt
index fbf2f67..a728d8e 100644
--- a/Documentation/stg-new.txt
+++ b/Documentation/stg-new.txt
@@ -31,7 +31,10 @@ the patch, unless the '--message' flag already specified one.  The
 editor.  The editor to use is taken from the first of the following
 sources of information, and defaults to 'vi':
 
+. the 'GIT_EDITOR' environment variable
 . the 'stgit.editor' GIT configuration variable
+. the 'core.editor' GIT configuration variable
+. the 'VISUAL' environment variable
 . the 'EDITOR' environment variable
 
 The message and other GIT commit attributes can be modified later
@@ -101,13 +104,16 @@ ENVIRONMENT VARIABLES
 	GIT_AUTHOR_DATE
 	GIT_COMMITTER_NAME
 	GIT_COMMITTER_EMAIL
+	GIT_EDITOR
 	EDITOR
+	VISUAL
 
 GIT CONFIGURATION VARIABLES
 ---------------------------
 
 	user.name
 	user.email
+	core.editor
 	stgit.editor
 
 StGIT
diff --git a/stgit/utils.py b/stgit/utils.py
index 2ff1d74..6b1d196 100644
--- a/stgit/utils.py
+++ b/stgit/utils.py
@@ -174,9 +174,17 @@ def call_editor(filename):
     """Run the editor on the specified filename."""
 
     # the editor
-    editor = config.get('stgit.editor')
+    editor = None
+    if 'GIT_EDITOR' in os.environ:
+        editor = os.environ['GIT_EDITOR']
+    if not editor:
+        editor = config.get('stgit.editor')
+    if not editor:
+        editor = config.get('core.editor')
     if editor:
         pass
+    elif 'VISUAL' in os.environ:
+        editor = os.environ['VISUAL']
     elif 'EDITOR' in os.environ:
         editor = os.environ['EDITOR']
     else:
-- 
1.5.4.rc3

Re: [StGit PATCH 4/5] Simplify editor selection logic

[Karl Hasselström]

On 2008-01-29 21:09:37 +0100, Peter Oberndorfer wrote:

> Since i personally dislike having a separate config for the editor
> in git/stgit i locally use this patch. unfortunately it makes the
> whole editor searching thing more complex :-( But i am sure it is
> possible to rewrite the code to something easier with some more
> python knowledge :-/ So it is not meant for direct applying, just
> for discussion...

I like the intention. What I'd like to do is to let the code _use_ the
stgit.editor variable, but not advertise it in the docs except for
noting that "this is deprecated and will go away in the future, but
for now it's still effective".

> +. the 'GIT_EDITOR' environment variable
>  . the 'stgit.editor' GIT configuration variable
> +. the 'core.editor' GIT configuration variable
> +. the 'VISUAL' environment variable
>  . the 'EDITOR' environment variable

That's a very nice order. (For completeness, the list should end with
"vi", though. And I'd like that deprecation note for stgit.editor,
unless someone has strong objections.)

>      # the editor
> -    editor = config.get('stgit.editor')
> +    editor = None
> +    if 'GIT_EDITOR' in os.environ:
> +        editor = os.environ['GIT_EDITOR']
> +    if not editor:
> +        editor = config.get('stgit.editor')
> +    if not editor:
> +        editor = config.get('core.editor')
>      if editor:
>          pass
> +    elif 'VISUAL' in os.environ:
> +        editor = os.environ['VISUAL']
>      elif 'EDITOR' in os.environ:
>          editor = os.environ['EDITOR']
>      else:

You could write it kind of like this:

  def e(key): return os.environ.get(key, None)
  def c(key): return config.get(key)
  editor = filter(None, [e('GIT_EDITOR'), c('stgit.editor'), c('core.editor'),
                         e('VISUAL'), e('EDITOR'), 'vi'])[0]

Of course, if we're going to have code like this in several places
(you already mentioned the pager), we could build a function like
this:

  editor = get_config(['GIT_EDITOR', 'stgit.editor', 'core.editor',
                       'VISUAL', 'EDITOR'], default = 'vi')

that would differentiate between env variables and conf keys by
looking for dots in the name or something.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: [StGit PATCH 4/5] Simplify editor selection logic

[Jay Soffian]

On Jan 30, 2008 2:28 AM, Karl Hasselström <kha@treskal.com> wrote:
> You could write it kind of like this:
>
>   def e(key): return os.environ.get(key, None)
>   def c(key): return config.get(key)
>   editor = filter(None, [e('GIT_EDITOR'), c('stgit.editor'), c('core.editor'),
>                          e('VISUAL'), e('EDITOR'), 'vi'])[0]

Too clever by half if you ask me. Why not just:

editor = (os.environ.get('GIT_EDITOR') or
          config.get('stgit.editor') or
          config.get('core.editor') or
          os.environ.get('VISUAL') or
          os.environ.get('EDITOR') or
          'vi')

And be done with it?

> Of course, if we're going to have code like this in several places
> (you already mentioned the pager), we could build a function like
> this:
>
>   editor = get_config(['GIT_EDITOR', 'stgit.editor', 'core.editor',
>                        'VISUAL', 'EDITOR'], default = 'vi')
>
> that would differentiate between env variables and conf keys by
> looking for dots in the name or something.

def get_config(keys, default=None):
    rv = default
    for k in keys:
        if '.' in k:
            d = config
        else:
            d = os.environ
        if k in d:
            rv = d[k]
            break
    return rv

j.

Re: [StGit PATCH 4/5] Simplify editor selection logic

[Karl Hasselström]

On 2008-01-30 09:55:18 -0500, Jay Soffian wrote:

> On Jan 30, 2008 2:28 AM, Karl Hasselström <kha@treskal.com> wrote:

> > You could write it kind of like this:
> >
> >   def e(key): return os.environ.get(key, None)
> >   def c(key): return config.get(key)
> >   editor = filter(None, [e('GIT_EDITOR'), c('stgit.editor'), c('core.editor'),
> >                          e('VISUAL'), e('EDITOR'), 'vi'])[0]
>
> Too clever by half if you ask me. Why not just:
>
> editor = (os.environ.get('GIT_EDITOR') or
>           config.get('stgit.editor') or
>           config.get('core.editor') or
>           os.environ.get('VISUAL') or
>           os.environ.get('EDITOR') or
>           'vi')
>
> And be done with it?

Yes. It's more repetitive, but not much longer. With only five options
and one default -- if there were more, my version would be nicer
(IMHO).

> > Of course, if we're going to have code like this in several places
> > (you already mentioned the pager), we could build a function like
> > this:
> >
> >   editor = get_config(['GIT_EDITOR', 'stgit.editor', 'core.editor',
> >                        'VISUAL', 'EDITOR'], default = 'vi')
> >
> > that would differentiate between env variables and conf keys by
> > looking for dots in the name or something.
> 
> def get_config(keys, default=None):
>     rv = default
>     for k in keys:
>         if '.' in k:
>             d = config
>         else:
>             d = os.environ
>         if k in d:
>             rv = d[k]
>             break
>     return rv

'config' isn't a dict, so you have to use config.get, and you can't
use 'k in config'. But otherwise yes. So something like this maybe:

def get_config(keys, default = None):
    rv = None
    for k in keys:
        if '.' in k:
            rv = config.get(k)
        else:
            rv = os.environ.get(k, None)
        if rv != None:
            return rv
    return default

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

[StGit PATCH 0/3] "stg edit" fixups

[Karl Hasselström]

I've fixed a few things with the "stg edit" series:

  * Took care of the import problem David pointed out.

  * Made the emacs mode not refuse to edit unaplied patches, now that
    "stg edit" can do it.

  * Made it so that the dates the user gets to edit are nicely
    human-readable, and not that seconds-since-1970 mess.

Also pushed out to kha/experimental.

---

Karl Hasselström (3):
      It's possible to edit unapplied patches now
      Convert "stg edit" to the new infrastructure
      Parse the date instead of treating it as an opaque string


 contrib/stgit.el       |    4 -
 stgit/commands/edit.py |  309 ++++++++++++++++++++++--------------------------
 stgit/lib/git.py       |   76 +++++++++++-
 3 files changed, 215 insertions(+), 174 deletions(-)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

[StGit PATCH 1/3] Parse the date instead of treating it as an opaque string

[Karl Hasselström]

This is needed for when we want to display it, since the
seconds-since-the-epoch format isn't that human-friendly.

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/lib/git.py |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 72 insertions(+), 4 deletions(-)


diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index d75f724..50dc4f1 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -1,10 +1,17 @@
 import os, os.path, re
+from datetime import datetime, timedelta, tzinfo
+
 from stgit import exception, run, utils
 from stgit.config import config
 
 class RepositoryException(exception.StgException):
     pass
 
+class DateException(exception.StgException):
+    def __init__(self, string, type):
+        exception.StgException.__init__(
+            self, '"%s" is not a valid %s' % (string, type))
+
 class DetachedHeadException(RepositoryException):
     def __init__(self):
         RepositoryException.__init__(self, 'Not on any branch')
@@ -26,6 +33,65 @@ def make_defaults(defaults):
             return None
     return d
 
+class TimeZone(tzinfo, Repr):
+    def __init__(self, tzstring):
+        m = re.match(r'^([+-])(\d{2}):?(\d{2})$', tzstring)
+        if not m:
+            raise DateException(tzstring, 'time zone')
+        sign = int(m.group(1) + '1')
+        try:
+            self.__offset = timedelta(hours = sign*int(m.group(2)),
+                                      minutes = sign*int(m.group(3)))
+        except OverflowError:
+            raise DateException(tzstring, 'time zone')
+        self.__name = tzstring
+    def utcoffset(self, dt):
+        return self.__offset
+    def tzname(self, dt):
+        return self.__name
+    def dst(self, dt):
+        return timedelta(0)
+    def __str__(self):
+        return self.__name
+
+class Date(Repr):
+    """Immutable."""
+    def __init__(self, datestring):
+        # Try git-formatted date.
+        m = re.match(r'^(\d+)\s+([+-]\d\d:?\d\d)$', datestring)
+        if m:
+            try:
+                self.__time = datetime.fromtimestamp(int(m.group(1)),
+                                                     TimeZone(m.group(2)))
+            except ValueError:
+                raise DateException(datestring, 'date')
+            return
+
+        # Try iso-formatted date.
+        m = re.match(r'^(\d{4})-(\d{2})-(\d{2})\s+(\d{2}):(\d{2}):(\d{2})\s+'
+                     + r'([+-]\d\d:?\d\d)$', datestring)
+        if m:
+            try:
+                self.__time = datetime(
+                    *[int(m.group(i + 1)) for i in xrange(6)],
+                    **{'tzinfo': TimeZone(m.group(7))})
+            except ValueError:
+                raise DateException(datestring, 'date')
+            return
+
+        raise DateException(datestring, 'date')
+    def __str__(self):
+        return self.isoformat()
+    def isoformat(self):
+        """Human-friendly ISO 8601 format."""
+        return '%s %s' % (self.__time.replace(tzinfo = None).isoformat(' '),
+                          self.__time.tzinfo)
+    @classmethod
+    def maybe(cls, datestring):
+        if datestring in [None, NoValue]:
+            return datestring
+        return cls(datestring)
+
 class Person(Repr):
     """Immutable."""
     def __init__(self, name = NoValue, email = NoValue,
@@ -34,6 +100,7 @@ class Person(Repr):
         self.__name = d(name, 'name')
         self.__email = d(email, 'email')
         self.__date = d(date, 'date')
+        assert isinstance(self.__date, Date) or self.__date in [None, NoValue]
     name = property(lambda self: self.__name)
     email = property(lambda self: self.__email)
     date = property(lambda self: self.__date)
@@ -51,7 +118,7 @@ class Person(Repr):
         assert m
         name = m.group(1).strip()
         email = m.group(2)
-        date = m.group(3)
+        date = Date(m.group(3))
         return cls(name, email, date)
     @classmethod
     def user(cls):
@@ -65,7 +132,7 @@ class Person(Repr):
             cls.__author = cls(
                 name = os.environ.get('GIT_AUTHOR_NAME', NoValue),
                 email = os.environ.get('GIT_AUTHOR_EMAIL', NoValue),
-                date = os.environ.get('GIT_AUTHOR_DATE', NoValue),
+                date = Date.maybe(os.environ.get('GIT_AUTHOR_DATE', NoValue)),
                 defaults = cls.user())
         return cls.__author
     @classmethod
@@ -74,7 +141,8 @@ class Person(Repr):
             cls.__committer = cls(
                 name = os.environ.get('GIT_COMMITTER_NAME', NoValue),
                 email = os.environ.get('GIT_COMMITTER_EMAIL', NoValue),
-                date = os.environ.get('GIT_COMMITTER_DATE', NoValue),
+                date = Date.maybe(
+                    os.environ.get('GIT_COMMITTER_DATE', NoValue)),
                 defaults = cls.user())
         return cls.__committer
 
@@ -301,7 +369,7 @@ class Repository(RunWithEnv):
                 for attr, v2 in (('name', 'NAME'), ('email', 'EMAIL'),
                                  ('date', 'DATE')):
                     if getattr(p, attr) != None:
-                        env['GIT_%s_%s' % (v1, v2)] = getattr(p, attr)
+                        env['GIT_%s_%s' % (v1, v2)] = str(getattr(p, attr))
         sha1 = self.run(c, env = env).raw_input(commitdata.message
                                                 ).output_one_line()
         return self.get_commit(sha1)

[StGit PATCH 2/3] Convert "stg edit" to the new infrastructure

[Karl Hasselström]

The --annotate and --undo switches were dropped in the conversion.
--annotate could be re-added, but --undo is more problematic since the
command will now rewrite any applied patches on top of the edited
patch. It seems best to leave this job to the fabled general undo
command, expected Real Soon Now.

In addition to the usual improvements from the new infrastructure,
this patch has some additional benefits:

  * There's a new -e/--edit flag, which forces interactive editing
    even if options such as --sign or --author are given. (Normally,
    interactive editing is skipped if the patch is modified with a
    commandline option.)

  * It's now possible to edit any patch, including unapplied patches.
    Even diff editing works for all patches, including unapplied
    patches. (In fact, editing unapplied patches is slightly safer,
    since they don't mind a dirty index/worktree.)

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 stgit/commands/edit.py |  309 ++++++++++++++++++++++--------------------------
 1 files changed, 142 insertions(+), 167 deletions(-)


diff --git a/stgit/commands/edit.py b/stgit/commands/edit.py
index 9915e49..7daf156 100644
--- a/stgit/commands/edit.py
+++ b/stgit/commands/edit.py
@@ -18,14 +18,12 @@ along with this program; if not, write to the Free Software
 Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 """
 
-from optparse import OptionParser, make_option
-from email.Utils import formatdate
+from optparse import make_option
 
-from stgit.commands.common import *
-from stgit.utils import *
+from stgit import git, utils
+from stgit.commands import common
+from stgit.lib import git as gitlib, transaction
 from stgit.out import *
-from stgit import stack, git
-
 
 help = 'edit a patch description or diff'
 usage = """%prog [options] [<patch>]
@@ -49,23 +47,19 @@ separator:
   Diff text
 
 Command-line options can be used to modify specific information
-without invoking the editor.
+without invoking the editor. (With the --edit option, the editor is
+invoked even if such command-line options are given.)
 
-If the patch diff is edited but the patch application fails, the
-rejected patch is stored in the .stgit-failed.patch file (and also in
-.stgit-edit.{diff,txt}). The edited patch can be replaced with one of
-these files using the '--file' and '--diff' options.
-"""
+If the patch diff is edited but does not apply, no changes are made to
+the patch at all. The edited patch is saved to a file which you can
+feed to "stg edit --file", once you have made sure it does apply."""
 
-directory = DirectoryGotoToplevel()
+directory = common.DirectoryHasRepositoryLib()
 options = [make_option('-d', '--diff',
                        help = 'edit the patch diff',
                        action = 'store_true'),
-           make_option('--undo',
-                       help = 'revert the commit generated by the last edit',
-                       action = 'store_true'),
-           make_option('-a', '--annotate', metavar = 'NOTE',
-                       help = 'annotate the patch log entry'),
+           make_option('-e', '--edit', action = 'store_true',
+                       help = 'invoke interactive editor'),
            make_option('--author', metavar = '"NAME <EMAIL>"',
                        help = 'replae the author details with "NAME <EMAIL>"'),
            make_option('--authname',
@@ -78,162 +72,143 @@ options = [make_option('-d', '--diff',
                        help = 'replace the committer name with COMMNAME'),
            make_option('--commemail',
                        help = 'replace the committer e-mail with COMMEMAIL')
-           ] + (make_sign_options() + make_message_options()
-                + make_diff_opts_option())
-
-def __update_patch(pname, text, options):
-    """Update the current patch from the given text.
-    """
-    patch = crt_series.get_patch(pname)
-
-    bottom = patch.get_bottom()
-    top = patch.get_top()
-
-    if text:
-        (message, author_name, author_email, author_date, diff
-         ) = parse_patch(text)
-    else:
-        message = author_name = author_email = author_date = diff = None
-
-    out.start('Updating patch "%s"' % pname)
-
-    if options.diff:
-        git.switch(bottom)
-        try:
-            git.apply_patch(diff = diff)
-        except:
-            # avoid inconsistent repository state
-            git.switch(top)
-            raise
-
-    def c(a, b):
-        if a != None:
-            return a
-        return b
-    crt_series.refresh_patch(message = message,
-                             author_name = c(options.authname, author_name),
-                             author_email = c(options.authemail, author_email),
-                             author_date = c(options.authdate, author_date),
-                             committer_name = options.commname,
-                             committer_email = options.commemail,
-                             backup = True, sign_str = options.sign_str,
-                             log = 'edit', notes = options.annotate)
-
-    if crt_series.empty_patch(pname):
-        out.done('empty patch')
-    else:
-        out.done()
-
-def __generate_file(pname, write_fn, options):
-    """Generate a file containing the description to edit
-    """
-    patch = crt_series.get_patch(pname)
-
-    # generate the file to be edited
-    descr = patch.get_description().strip()
-    authdate = patch.get_authdate()
-
-    tmpl = 'From: %(authname)s <%(authemail)s>\n'
-    if authdate:
-        tmpl += 'Date: %(authdate)s\n'
-    tmpl += '\n%(descr)s\n'
-
-    tmpl_dict = {
-        'descr': descr,
-        'authname': patch.get_authname(),
-        'authemail': patch.get_authemail(),
-        'authdate': patch.get_authdate()
-        }
-
-    if options.diff:
-        # add the patch diff to the edited file
-        bottom = patch.get_bottom()
-        top = patch.get_top()
+           ] + (utils.make_sign_options() + utils.make_message_options()
+                + utils.make_diff_opts_option())
 
-        tmpl += '---\n\n' \
-                '%(diffstat)s\n' \
-                '%(diff)s'
-
-        tmpl_dict['diff'] = git.diff(rev1 = bottom, rev2 = top,
-                                     diff_flags = options.diff_flags)
-        tmpl_dict['diffstat'] = git.diffstat(tmpl_dict['diff'])
-
-    for key in tmpl_dict:
-        # make empty strings if key is not available
-        if tmpl_dict[key] is None:
-            tmpl_dict[key] = ''
-
-    text = tmpl % tmpl_dict
-
-    # write the file to be edited
-    write_fn(text)
-
-def __edit_update_patch(pname, options):
-    """Edit the given patch interactively.
-    """
-    if options.diff:
-        fname = '.stgit-edit.diff'
+def patch_diff(repository, cd, diff, diff_flags):
+    if diff:
+        diff = repository.diff_tree(cd.parent.data.tree, cd.tree, diff_flags)
+        return '\n'.join([git.diffstat(diff), diff])
     else:
-        fname = '.stgit-edit.txt'
-    def write_fn(text):
-        f = file(fname, 'w')
-        f.write(text)
-        f.close()
-
-    __generate_file(pname, write_fn, options)
-
-    # invoke the editor
-    call_editor(fname)
-
-    __update_patch(pname, file(fname).read(), options)
+        return None
+
+def patch_description(cd, diff):
+    """Generate a string containing the description to edit."""
+
+    desc = ['From: %s <%s>' % (cd.author.name, cd.author.email),
+            'Date: %s' % cd.author.date.isoformat(),
+            '',
+            cd.message]
+    if diff:
+        desc += ['---',
+                 '',
+                diff]
+    return '\n'.join(desc)
+
+def patch_desc(repository, cd, failed_diff, diff, diff_flags):
+    return patch_description(cd, failed_diff or patch_diff(
+            repository, cd, diff, diff_flags))
+
+def update_patch_description(repository, cd, text):
+    message, authname, authemail, authdate, diff = common.parse_patch(text)
+    cd = (cd.set_message(message)
+            .set_author(cd.author.set_name(authname)
+                                 .set_email(authemail)
+                                 .set_date(gitlib.Date.maybe(authdate))))
+    failed_diff = None
+    if diff:
+        tree = repository.apply(cd.parent.data.tree, diff)
+        if tree == None:
+            failed_diff = diff
+        else:
+            cd = cd.set_tree(tree)
+    return cd, failed_diff
 
 def func(parser, options, args):
     """Edit the given patch or the current one.
     """
-    crt_pname = crt_series.get_current()
+    stack = directory.repository.current_stack
 
-    if not args:
-        pname = crt_pname
-        if not pname:
-            raise CmdException, 'No patches applied'
+    if len(args) == 0:
+        if not stack.patchorder.applied:
+            raise common.CmdException(
+                'Cannot edit top patch, because no patches are applied')
+        patchname = stack.patchorder.applied[-1]
     elif len(args) == 1:
-        pname = args[0]
-        if crt_series.patch_unapplied(pname) or crt_series.patch_hidden(pname):
-            raise CmdException, 'Cannot edit unapplied or hidden patches'
-        elif not crt_series.patch_applied(pname):
-            raise CmdException, 'Unknown patch "%s"' % pname
+        [patchname] = args
+        if not stack.patches.exists(patchname):
+            raise common.CmdException('%s: no such patch' % patchname)
     else:
-        parser.error('incorrect number of arguments')
-
-    check_local_changes()
-    check_conflicts()
-    check_head_top_equal(crt_series)
-
-    if pname != crt_pname:
-        # Go to the patch to be edited
-        applied = crt_series.get_applied()
-        between = applied[:applied.index(pname):-1]
-        pop_patches(crt_series, between)
-
-    if options.author:
-        options.authname, options.authemail = name_email(options.author)
-
-    if options.undo:
-        out.start('Undoing the editing of "%s"' % pname)
-        crt_series.undo_refresh()
-        out.done()
-    elif options.save_template:
-        __generate_file(pname, options.save_template, options)
-    elif any([options.message, options.authname, options.authemail,
-              options.authdate, options.commname, options.commemail,
-              options.sign_str]):
-        out.start('Updating patch "%s"' % pname)
-        __update_patch(pname, options.message, options)
-        out.done()
-    else:
-        __edit_update_patch(pname, options)
+        parser.error('Cannot edit more than one patch')
+
+    cd = orig_cd = stack.patches.get(patchname).commit.data
 
-    if pname != crt_pname:
-        # Push the patches back
-        between.reverse()
-        push_patches(crt_series, between)
+    # Read patch from user-provided description.
+    if options.message == None:
+        failed_diff = None
+    else:
+        cd, failed_diff = update_patch_description(stack.repository, cd,
+                                                   options.message)
+
+    # Modify author and committer data.
+    if options.author != None:
+        options.authname, options.authemail = common.name_email(options.author)
+    for p, f, val in [('author', 'name', options.authname),
+                      ('author', 'email', options.authemail),
+                      ('author', 'date', gitlib.Date.maybe(options.authdate)),
+                      ('committer', 'name', options.commname),
+                      ('committer', 'email', options.commemail)]:
+        if val != None:
+            cd = getattr(cd, 'set_' + p)(
+                getattr(getattr(cd, p), 'set_' + f)(val))
+
+    # Add Signed-off-by: or similar.
+    if options.sign_str != None:
+        cd = cd.set_message(utils.add_sign_line(
+                cd.message, options.sign_str, gitlib.Person.committer().name,
+                gitlib.Person.committer().email))
+
+    if options.save_template:
+        options.save_template(
+            patch_desc(stack.repository, cd, failed_diff,
+                       options.diff, options.diff_flags))
+        return utils.STGIT_SUCCESS
+
+    # Let user edit the patch manually.
+    if cd == orig_cd or options.edit:
+        fn = '.stgit-edit.' + ['txt', 'patch'][bool(options.diff)]
+        cd, failed_diff = update_patch_description(
+            stack.repository, cd, utils.edit_string(
+                patch_desc(stack.repository, cd, failed_diff,
+                           options.diff, options.diff_flags),
+                fn))
+
+    def failed():
+        fn = '.stgit-failed.patch'
+        f = file(fn, 'w')
+        f.write(patch_desc(stack.repository, cd, failed_diff,
+                           options.diff, options.diff_flags))
+        f.close()
+        out.error('Edited patch did not apply.',
+                  'It has been saved to "%s".' % fn)
+        return utils.STGIT_COMMAND_ERROR
+
+    # If we couldn't apply the patch, fail without even trying to
+    # effect any of the changes.
+    if failed_diff:
+        return failed()
+
+    # The patch applied, so now we have to rewrite the StGit patch
+    # (and any patches on top of it).
+    iw = stack.repository.default_iw
+    trans = transaction.StackTransaction(stack, 'stg edit')
+    if patchname in trans.applied:
+        popped = trans.applied[trans.applied.index(patchname)+1:]
+        assert not trans.pop_patches(lambda pn: pn in popped)
+    else:
+        popped = []
+    trans.patches[patchname] = stack.repository.commit(cd)
+    try:
+        for pn in popped:
+            trans.push_patch(pn, iw)
+    except transaction.TransactionHalted:
+        pass
+    try:
+        # Either a complete success, or a conflict during push. But in
+        # either case, we've successfully effected the edits the user
+        # asked us for.
+        return trans.run(iw)
+    except transaction.TransactionException:
+        # Transaction aborted -- we couldn't check out files due to
+        # dirty index/worktree. The edits were not carried out.
+        return failed()

[StGit PATCH 3/3] It's possible to edit unapplied patches now

[Karl Hasselström]

With the rewrite, "stg edit" gained the ability to edit unapplied
patches, so the emacs mode no longer has to check that a patch is
applied before trying to edit it.

Signed-off-by: Karl Hasselström <kha@treskal.com>

---

 contrib/stgit.el |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)


diff --git a/contrib/stgit.el b/contrib/stgit.el
index e8bbb2c..bef41c7 100644
--- a/contrib/stgit.el
+++ b/contrib/stgit.el
@@ -290,9 +290,7 @@ Commands:
 (defun stgit-edit ()
   "Edit the patch on the current line"
   (interactive)
-  (let ((patch (if (stgit-applied-at-point)
-                   (stgit-patch-at-point)
-                 (error "This patch is not applied")))
+  (let ((patch (stgit-patch-at-point))
         (edit-buf (get-buffer-create "*StGit edit*"))
         (dir default-directory))
     (log-edit 'stgit-confirm-edit t nil edit-buf)