[Gnu-arch-users] Re: Error handling

[Tom Lord]

    > From: Aaron Bentley <address@hidden>
    > > If patch-2 does not exist:
    > > address@hidden archeg]$ tla get engine--release7-6--3.0--patch-2
    > > corrupt archive
    > >   name: address@hidden
    > >   location: /mnt/extra/abentley/archives/development
    > >   revision: engine--release7-6--3.0--patch-2

    > This error was still bugging me, so I had a look at the code.
    > AFAICT, fs_revision_type is jumping to conclusions when it reports
    > "corrupt archive".  When the revision spec has been automatically
    > determined from the archive, inability to access the revision *does*
    > indicate a corrupt archive.

That's right.


    > But in the "get" case, the revision spec is user-entered, and inability
    > to access the revision indicates "corrupt user", not corrupt archive.

That's right.

    > Would you accept a patch that

    > 1. causes fs_revision_type() to return 0 on success, -1 on failure, and
    > removes error message (and the same for pfs_revision_type).

    > 2. moves the error message and exit(2) to arch_revision_type()

    > 3. adds arch_revision_exists(), which takes the same parameters as
    > arch_revision_type, but returns 0 if the archive is accessible and -1 if
    > it is not

    > 4. changes the arch_revision_type() call in build_revision() into
    > arch_revision_exists(), and reports "no such revision" if it returns -1

That sounds fine with two small qualifications.

* don't forget archive-pfs.c and, in fact, archive-fs.c will be retired

  The file "archive-fs.c", and thus the function fs_revision_type, is
  going away.
  
  Meanwhile, there is nearly isomorophic code in
  archive-pfs.c(pfs_revision_type).   
  
  Currently, archive-fs.c is used for local archives, and archive-pfs.c
  is used for everything else.    Shortly, archive-pfs.c will be used
  for everything, including local archives.
  
  So where does that leave the prospective patch?
  
  (a) if you want to do it right away, you need to make isomorphic
      changes to both fs_revision_type and pfs_revision_type.
  
      (You'd have noticed that anyway as if you only change
      fs_revision_type you'll get some type-checking compilation
      errors from archive-pfs.c).
  
  
  (b) if you want to wait a week or so, you can make the change just
      once to archive-pfs.c only.


* arch_build_revision is the wrong place for this test

  arch_build_revision is recursive.   In all but the outermost call of a 
  chain of recursive invocations, the conclusion "corrupt archive" is
  correct.  In the outermost call, the conclusion "corrupt archive" is
  only _sometimes_ incorrect.

  arch_build_revision is often called indirectly with arguments called
  from the command line.  There is no simple way to distinguish, by
  the time arch_build_revision is called, whether the arguments
  indicate a revision that should be presumed to exist (hence,
  "corrupt archive" is the right error) or one that we only know the
  user as asserted exists (hence, "no such revision" is the right
  error).

  Therefore, the better fix here is to call arch_revision_exists from
  various cmd-*.c files and give the "no such revision" message from
  there.

  I would suggest: 

        use step 1 of your plan, modulo the issue with
        archive-fs.c vs archive-pfs.c

        use steps 2 and 3 of your plan as is.

        add a convenience function, arch_check_for_revision (archive.c
        is an ok place to add it), which takes as arguments a "struct
        arch_archive *" and a revision name -- and either returns 
        normally or exits with a "no such revision" error.   It should
        call arch_revision_exists.

        Modify, at least, cmd-get.c.   Just before the call to 
        arch_build_revision, put a call to arch_check_for_revision.

        Feel free to similarly modify other cmd-*.c files or to leave
        those for the next person with a similar itch.   In making 
        additional modifications there is a slight "art" to it:  to
        avoid having to connect to an archive just to call
        arch_check_for_revision -- to connect only in those places
        which already connect.


Thank you, by the way, for a lucid description of the change you are
thinking of.

-t