On Sat, Mar 12, 2022 at 01:19:32PM +0100, Omar Polo wrote:
> Stefan Sperling <stsp@stsp.name> wrote:
> > On Sat, Mar 12, 2022 at 10:56:32AM +0100, Stefan Sperling wrote:
> > > You do not yet have to parse any of these headers. You could just treat
> > > them as noise, for now.
> > 
> > I think my suggestion boils down to this diff.
> > The patch tests keep passing.
> > 
> > What do you think could go wrong if we did this?
> > I don't see a problem.
> 
> well, this is the first step yes, but it doesn't really work because it
> doesn't properly register the rename.  Also, apply_patch set `path' to
> p->new (if not NULL) so then it'll fail when trying to open the "orig"
> file.

OK, so you do want to support renames in 'got patch' already,
instead of assuming you will only edit, add new files, or delete
existing files?

Because we allow patch to run on a work tree that contains local changes,
the actions of adding, deleting and renaming files create a bit of a can
of worms. It is a probably a good moment to talk about this now, so you
can take such problems into consideration going forward. It can take some
time to work through this because there can be many cases to consider.
Ideally, the test suite would provide complete coverage of possible
cases, though this may take some time to achieve. Test coverage could
be grown systematically, step by step.

People can^Wwill have arbitrary changes in their work trees and 'got patch'
must be prepared for them:

 - status of each of the old and new paths:
   - does it exist in the file index (i.e. is it a versioned path)?
   - does it exist on disk?
   - should existing unversioned files be modified by 'got patch'?
   - if versioned, is it deleted / added / modified / obstructed / ... ?
     ("obstructed" means that the on-disk file is a non-regular file,
      such as a directory, fifo, device node, ...)
   - if versioned, do already staged adds/deletes/edits matter?
 - if renaming: 
   - if the file is deleted, perhaps the rename the patch wants to
     perform has already been done (look for paths in D and A status)?
 - if an action the patch wants to perform is blocked by an incompatible
   state of a path, in addition to raising an error, maybe prepare for
   a rollback of changes? Or do not apply any changes at all until you
   are sure the entire patch can be applied without issues?

If you don't want to deal with a particular case, maybe you can impose
pre-conditions that make the case impossible. While local modifications
are allowed, you could disallow paths to be in a status other than M,
for example, and avoid some of the more complicated cases. Such
pre-conditions could be lifted once support for more cases gets added.

> The tests are still passing because none of them has an old path
> different from the new path.  (/dev/null is an exception, p->old or
> p->new are set to NULL if they were /dev/null in the patch.)
> 
> Here's another attempt.  There are two small commits initially that are
> just small issue I found while working on the rename.
> 
> I took this as a chance to refactor apply_patch, it was trying to do too
> much stuff and I started to loosing keeping track of things.  The
> proposed diff below moves the guts of apply_patch into a new function
> patch_file and adds two small helpers (schedule_add, schedule_del) so
> that apply_patch remains manegeable.  It also adds a test for the rename
> case that you can try against your original diff if you want.

Looks good to me!