"stgit clean" has problems with removed generated files

"stgit clean" has problems with removed generated files

[Yann Dirson]

I recently encountered a problem with "stgit clean", but it really is a
generic issue, which can also impact other subcommands.

In a kernel tree, the precise problem I had is due to generated files
committed by error in an upstream branch (a BSP from a well-known
vendor, indeed ;).  The first patch in my stgir stack does some cleanup
by removing them from git control (so that "make dep" does not cause
them to change every so often).

Now when I want to run "stg clean" for applied patches, stgit first
wants to pop the whole stack, including that patch, which triggers the
following error:

fatal: Untracked working tree file 'include/asm-arm/constants.h' would be                                                                                                              overwritten by merge.                                                                                                                                                                  stg clean: git-read-tree failed (local changes maybe?)                                                                                                                                 


Obviously, removing all such files by hand allows to run "stg clean", as
does (floating and) popping that patch and deleting it, or running "stg clean
--unappplied".  The 1st approach is intrusive, and forces the user to
rerun "make dep", which is not very friendy; the 2nd one is too error-prone for
an operation that ought to be risk-less (delete has no --undo).  The 3rd option
is probably the best, but is surely not so intuitive.

The root issue seems to be that stgit has problem with such generated
files, ie., files that were removed from version-control, but can still
legitimately exist in the tree.  Dealing with them could possibly be
done (eg. allowing to back them up, and restore them when pushing the
annoying patch), but is not a trivial issue (eg. we still need to guard
the user against real conflicts).

At least we could put a notice about this case in the doc, so users
don't get too surprised.


I could think of several (non-exclusive) possibilities to deal with the
precise problem I got, which should allow.

First, when cleaning patches, we could first look up which patches are
to be removed, and only pop the necessary ones.

Second, we could generalize the "clean" subcommand to accept arbitrary
ranges, not only the "applied" and "unapplied" ones.  A special case
would be "stg clean that-patch", which would be a secure version of "stg
delete".

BTW, maybe it would those make sense to allowthose special ranges in
most places a range is valid.


Best regards,
-- 

Re: "stgit clean" has problems with removed generated files

[Catalin Marinas]

On 23/11/06, Yann Dirson <ydirson@altern.org> wrote:
> In a kernel tree, the precise problem I had is due to generated files
> committed by error in an upstream branch (a BSP from a well-known
> vendor, indeed ;).  The first patch in my stgir stack does some cleanup
> by removing them from git control (so that "make dep" does not cause
> them to change every so often).
>
> Now when I want to run "stg clean" for applied patches, stgit first
> wants to pop the whole stack, including that patch, which triggers the
> following error:
>
> fatal: Untracked working tree file 'include/asm-arm/constants.h' would be                                                                                                              overwritten by merge.

That's a git error and I think it is the correct behaviour. It is
safer to notify that a local file is overridden by a merge/switch
operation rather than just losing its content.

> Obviously, removing all such files by hand allows to run "stg clean", as
> does (floating and) popping that patch and deleting it, or running "stg clean
> --unappplied".

Maybe 'stg clean' should only pop to the first empty applied patch
rather than popping all of them as it is also more efficient.

> The root issue seems to be that stgit has problem with such generated
> files, ie., files that were removed from version-control, but can still
> legitimately exist in the tree.  Dealing with them could possibly be
> done (eg. allowing to back them up, and restore them when pushing the
> annoying patch), but is not a trivial issue (eg. we still need to guard
> the user against real conflicts).

That's a GIT issue more than an StGIT one, unless GIT already has a
way to deal with this and StGIT doesn't pass the right options.

> First, when cleaning patches, we could first look up which patches are
> to be removed, and only pop the necessary ones.

OK, I mentioned it above as well. This should really be done for
efficiency but it might not solve the problem if the empty patch is
deeper into the stack.

> Second, we could generalize the "clean" subcommand to accept arbitrary
> ranges, not only the "applied" and "unapplied" ones.  A special case
> would be "stg clean that-patch", which would be a secure version of "stg
> delete".

Easy to fix as well.

> BTW, maybe it would those make sense to allowthose special ranges in
> most places a range is valid.

Is there any other place where ranges could be used but aren't?

-- 

Re: "stgit clean" has problems with removed generated files

[Yann Dirson]

On Thu, Nov 23, 2006 at 04:33:42PM +0000, Catalin Marinas wrote:
> >fatal: Untracked working tree file 'include/asm-arm/constants.h' would be  
> >overwritten by merge.
> 
> That's a git error and I think it is the correct behaviour. It is
> safer to notify that a local file is overridden by a merge/switch
> operation rather than just losing its content.

Right, I do not discuss the behaviour of git here.  But when I first
encountered this issue, I was really wondering about what was
happenning.  It would be really helpful in such a case, if stgit was
able to pinpoint the precise patch which could not be popped.  It could
also be helpful to tell when popping patches - currently it's done
"behind my back", and I could only understand what was happenning by
reading the code.


> >The root issue seems to be that stgit has problem with such generated
> >files, ie., files that were removed from version-control, but can still
> >legitimately exist in the tree.  Dealing with them could possibly be
> >done (eg. allowing to back them up, and restore them when pushing the
> >annoying patch), but is not a trivial issue (eg. we still need to guard
> >the user against real conflicts).
> 
> That's a GIT issue more than an StGIT one, unless GIT already has a
> way to deal with this and StGIT doesn't pass the right options.

I'm not sure if git itself should add support for this.  IMHO, such an issue
is more related to the nature of patch management - git itself does not
have a need to repetedly pop/push paches, with related merge
hassles like this one.


> >First, when cleaning patches, we could first look up which patches are
> >to be removed, and only pop the necessary ones.
> 
> OK, I mentioned it above as well. This should really be done for
> efficiency but it might not solve the problem if the empty patch is
> deeper into the stack.

Right, it could be advertised as good practice to burry those near the
bottom of the stack, and anyway to keep actively-changing ones near the
top.


> >BTW, maybe it would those make sense to allowthose special ranges in
> >most places a range is valid.
> 
> Is there any other place where ranges could be used but aren't?

Hm, let's see...  I'd say "export" (I have missed it already), "files"
and maybe "commit" and "pick", although the latter would require a syntax
for ranges in other branch.

While reviewing the various commands for this, I realized that "stg pop
<patch>" semantics is significantly different from "stg push <patch>" -
ie. it is an equivalent of "goto".  What about turning it into a
float+pop, to better match the "push" behaviour ?

I also realized that "stg help <command>" output does not go through to
the pager, while eg. the help for "mail" is quite long.

Best regards,
-- 

Re: "stgit clean" has problems with removed generated files

[Jakub Narebski]

Yann Dirson wrote:

> On Thu, Nov 23, 2006 at 04:33:42PM +0000, Catalin Marinas wrote:
>> >fatal: Untracked working tree file 'include/asm-arm/constants.h' would be  
>> >overwritten by merge.
>> 
>> That's a git error and I think it is the correct behaviour. It is
>> safer to notify that a local file is overridden by a merge/switch
>> operation rather than just losing its content.
> 
> Right, I do not discuss the behaviour of git here.  But when I first
> encountered this issue, I was really wondering about what was
> happenning.  It would be really helpful in such a case, if stgit was
> able to pinpoint the precise patch which could not be popped.  It could
> also be helpful to tell when popping patches - currently it's done
> "behind my back", and I could only understand what was happenning by
> reading the code.

I think it was corrected in git, or is being corrected (meaning it is
in 'next' or in 'pu'); it being relaxing "would be overwritten by merge"
check for files which were, but are not, under version control.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: "stgit clean" has problems with removed generated files

[Catalin Marinas]

On 23/11/06, Yann Dirson <ydirson@altern.org> wrote:
> On Thu, Nov 23, 2006 at 04:33:42PM +0000, Catalin Marinas wrote:
> > >First, when cleaning patches, we could first look up which patches are
> > >to be removed, and only pop the necessary ones.

I fixed this.

> > Is there any other place where ranges could be used but aren't?
>
> Hm, let's see...  I'd say "export" (I have missed it already), "files"
> and maybe "commit" and "pick", although the latter would require a syntax
> for ranges in other branch.

I changed the export command to take a range of patches as arguments
(and a --dir option for the export directory).

> While reviewing the various commands for this, I realized that "stg pop
> <patch>" semantics is significantly different from "stg push <patch>" -
> ie. it is an equivalent of "goto".  What about turning it into a
> float+pop, to better match the "push" behaviour ?

Thanks for the suggestion, I fixed pop to take patch ranges now.

> I also realized that "stg help <command>" output does not go through to
> the pager, while eg. the help for "mail" is quite long.

Added this feature as well. I'll try to push the changes tonight.

--