On Sat, Sep 16, 2023 at 07:28:59PM +0200, Omar Polo wrote:
> On 2023/09/16 13:26:48 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> > On Sat, Sep 16, 2023 at 01:00:05PM +0200, Omar Polo wrote:
> > > I was overly eager to optimize when I suggested to set {c,m}time to
> > > epoch in the fileindex when checking out over existing files because
> > > the usual (?) case would be to checkout over a previous checkout (or
> > > something that resembles it.)
> > >
> > > However, it seems easier to propagate the info upward from
> > > install_blob() rather than -E down from cmd_checkout().
> > 
> > This approach is fine with me.
> > 
> > > diff just to illustrate, still lacking a regress.
> > 
> > Tweaking test_checkout_into_nonempty_dir somehow to cover this code
> > path should be sufficieut.
> >  
> > > P.S. is the manpage wrong too by the way?  It says that the statuses
> > >      for checkout are A and E, no mention of ? is done.  Should we
> > >      return E for this case or add ? to the listing?
> > 
> > It should say 'E', and test_checkout_into_nonempty_dir checks for 'E'.
> > 
> > I am not sure why '?' shows up in naddy's test case. Could you try to
> > tweak the existing test such that '?' gets triggered and then fix the
> > implementation to say 'E' instead?
> 
> It's easy to trigger.  Checkout something, delete .got and re-checkout
> with -E.  All the files in the output are unversioned.  I always seen
> this and just assumed is was 'as intended'.  I've read that part of
> the manpage only today.
> 
> Here's the updated diff with a logic error fixed (update_timestamps
> was always zero...) and a tweaked test case.  I've also changed the
> status char, now we report E when attempting to checking out a file
> that already exists but was untracked.
> 
> checkout.sh is fine, i'm running the regress but so far seems happy.

Looks good to me, thank you. Ok by me provided regress was indeed happy.