public inbox for gnats-devel@sourceware.org
 help / color / mirror / Atom feed
* patch #1 - towards a generic backend datastore
@ 2005-02-21 12:41 Mel Hatzis
  2005-02-23  7:24 ` Mel Hatzis
  0 siblings, 1 reply; 8+ messages in thread
From: Mel Hatzis @ 2005-02-21 12:41 UTC (permalink / raw)
  To: Chad Walstrom, help-gnats

[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]

Please review the attached patch which contains a number of bug fixes
in addition to modifications intended to restructure the code to
support a generalized backend datastore.

I have developed a copy of GNATS (which I'm calling gnats-dbi)
which utilizes a backend datastore api and can be configured with
either a flat-file or Oracle backend. Anyone interested can download
a copy from http://professional.juniper.net/gnats/.

This patch is the first of many I plan to submit to merge my
changes back into the official GNATS project and thereby enhance
it with the ability to have multiple backends.

This first patch is rather large. Hopefully, most of the subsequent
ones will be easier to digest. The primary goal in this patch
was to begin isolating the index logic from the PR edit functionality.
A number of performance improvements have been made in the process.

Note that I started developing and testing primarily on my Mac and
this exposed a few memory related issues which I decided to fix
in the process.

--
Mel Hatzis

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 42498 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/ChangeLog,v
retrieving revision 1.308
diff -u -p -r1.308 ChangeLog
--- ChangeLog	30 Nov 2004 15:44:37 -0000	1.308
+++ ChangeLog	21 Feb 2005 12:19:38 -0000
@@ -1,3 +1,71 @@
+2005-02-19  Mel Hatzis  <hatzis@wattes.org>
+
+	* adm.c (copy_adm_entry): Handle blank metadata fields by correctly
+	assigning the relevant field in the adm struct to NULL - and thereby
+	avoid an attempt to xstrdup a NULL value.
+
+	* file-pr.c (createNewPRFile): Avoid a NULL pointer dereference when
+	there's a blank response time field in the submitters file.
+
+	* gnatsd.c (findUserAccessLevel): Avoid a strlen of NULL when the
+	gnatsd.user_access file contains a blank 'database-alias' field.
+
+	* gnats.h (rewrite_pr): Made it a static function in edit.c - it
+	wasn't being used outside this scope anyway.
+
+	* mail.c (get_responsible_address): Avoid a NULL pointer dereference
+	if the responsible file doesn't have an email address for the
+	gnats-admin user.
+
+	* index.c, index.h (getPrevPR, setPrevPR, setNextPR): New functions.
+	The index chain is now doubly linked allowing edited PRs to be
+	replaced without incurring the overhead of a search through the
+	chain. As well as offering significantly better performance, this
+	supports a change to the PR edit functionality whereby we now start
+	with a copy of the old PR as well as the updated PR instead of
+	retrieving the old PR (from the index) in the guts of rewrite_pr.
+	The motivation behind this change is to isolate the index
+	functionality and move toward a generic backend datastore which
+	doesn't require the index.
+	(replaceCurrentPRInIndex): Modified signature. Pass in the PR to be
+	replaced (as well as the new PR) since we can now use the doubly
+	linked index chain to replace the PR without searching for it.
+
+	* pr.h (lookup_pr_path): Removed. Replaced by a call to the new
+	get_pr_from_index () function followed by a call to (a modified)
+	get_pr_path (). This change allows a single index search to be done
+	once (early on in a given call stack) to retrieve a PR and
+	thereafter access the PR file without requiring  more index
+	searches.
+	(pr_delete): New function. Contains the flat-file specific logic
+	related to PR deletions which will be moved out into a flat-file
+	backend when the datastore backend is generalized.
+
+	* pr.c (get_pr_from_index): New function. Retrieve a PR from the
+	index.
+	(pr_file_readable): New function. Check if a file can be opened for
+	reading.
+	(pr_delete): New function. Flat-file specific logic for delete.
+	(prExists): Now utilize get_pr_from_index () and get_pr_path ()
+	instead of the deprecated lookup_pr_path (). 
+	(readPRWithNum): Ditto. Also set the PR's next/prev index pointers.
+	(get_pr_path): New signature. Take in a PR * instead of searching
+	the index.
+	(lookup_pr_path): Deprecated.
+
+	* edit.c (applyChangeActions, processFieldChange, sendAuditMail):
+	Repositioned within the file so as not to require prototypes.
+	(processPRChanges): Handle NULL values appropriately when undoing
+	changes to modified read-only fields.
+	(rewrite_pr, replace_pr, edit_field): rewrite_pr now takes in a copy
+	of the old pr instead of retrieving it from the index. This is in
+	preparation for allowing it to be used with more than just the flat
+	file backend datastore. Also moved some error checking out of
+	rewrite_pr and simplified it...no need to validate the whole PR
+	when editing a single field.
+	(deletePR): Moved the file-related logic into a new pr_delete function
+	inside pr.c thereby preparing for a general backend datastore.
+
 2004-11-30  Chad Walstrom  <chewie@wookimus.net>
 
 	* gnatsd.c, gnats-pwconv.c (HAVE_CRYPT_H): Changed HAVE_CRYPT to
Index: adm.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/adm.c,v
retrieving revision 1.20
diff -u -p -r1.20 adm.c
--- adm.c	27 Oct 2002 06:21:17 -0000	1.20
+++ adm.c	21 Feb 2005 12:19:38 -0000
@@ -47,7 +47,9 @@ copy_adm_entry (AdmEntry *ent)
   res->field = ent->field;
   for (x = 0; x < ent->fieldcount; x++)
     {
-      res->admFields[x] = xstrdup (ent->admFields[x]);
+      res->admFields[x] = (ent->admFields[x] == NULL)
+			    ? NULL
+			    : xstrdup (ent->admFields[x]);
     }
   return res;
 }
Index: edit.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/edit.c,v
retrieving revision 1.60
diff -u -p -r1.60 edit.c
--- edit.c	25 Nov 2002 13:58:33 -0000	1.60
+++ edit.c	21 Feb 2005 12:19:39 -0000
@@ -24,38 +24,113 @@ Software Foundation, 59 Temple Place - S
 #include "query.h"
 #include "mail.h"
 
-static int applyChangeActions (PR *pr, PR *oldPR, FieldIndex field, 
-			       ChangeActions actions, ErrorDesc *err,
-			       FormatNamedParameter *params);
-
-static int processFieldChange (PR *pr, PR *oldPR, FieldIndex field,
-			       ErrorDesc *err, 
-			       const char *editUserEmailAddr,
-			       const char *oldValue, const char *newValue);
-
 /* Euuugh.  There's gotta be a better way to keep this around.  */
 static char *newAuditTrailEntries = NULL;
 
-static void sendAuditMail (PR *pr, PR *oldPR, const char *editUserEmailAddr,
-			   const char *newAuditTrailEntries,
-			   ErrorDesc *err);
+static void
+sendAuditMail (PR *pr, PR *oldPR, const char *editUserEmailAddr, 
+	       const char *newAuditTrailEntries, ErrorDesc *err)
+{
+  FormatNamedParameter *parms = NULL;
+  const DatabaseInfo database = pr->database;
 
-int
-replace_pr (const DatabaseInfo database, FILE *fp,
-	    const char *editUserEmailAddr, ErrorDesc *err)
+  parms = allocateNamedParameter ("$EditUserEmailAddr", editUserEmailAddr,
+				  parms);
+  parms = allocateNamedParameter ("$NewAuditTrail", newAuditTrailEntries,
+				  parms);
+
+  if (strcmp (field_value (pr, RESPONSIBLE (database)),
+	      field_value (oldPR, RESPONSIBLE (database))) != 0)
+    {
+      parms
+	= allocateNamedParameter ("$OldResponsible",
+				  field_value (oldPR, RESPONSIBLE (database)),
+				  parms);
+    }
+
+  composeMailMessage (pr, oldPR, "audit-mail", parms, NULL, err);
+  freeFormatParameterList (parms);
+}
+
+/* Add an entry to the AUDIT_TRAIL field. PARAMS are the format
+   parameters used when composing the entry.  FMT is the format of the
+   audit-trail entry to add.  */
+
+static int
+addAuditTrailEnt (PR *pr, QueryFormat *fmt, 
+		  FormatNamedParameter *params, ErrorDesc *err)
 {
-  PR *pr = allocPR (database);
+  char *newAuditString = NULL;
+  char *finalAuditString;
+  const char *t;
+  char *currDate = get_curr_date ();
 
-  /* Read the message header in.  */
-  if (read_header (pr, fp) < 0)
+  if (fmt == NULL)
     {
-      setError (err, CODE_EOF_PR, "Couldn't read PR header");
-      return 0;
+      fmt = getAuditTrailFormat (pr->database);
     }
 
-  read_pr (pr, fp, 0);
+  /* Format the new audit entry. */
+  process_format (NULL, &newAuditString, pr, NULL, fmt, "\n", params);
 
-  return rewrite_pr (pr, editUserEmailAddr, err);
+  /* Squirrel it away, because we'll need to mail it later.  */
+  append_string (&newAuditTrailEntries, newAuditString);
+
+  /* Now append the formatted string to the audit-trail field.  */
+  t = field_value (pr, AUDIT_TRAIL (pr->database));
+  if (t == NULL)
+    {
+      t = "";
+    }
+  finalAuditString = xstrdup (t);
+  append_string (&finalAuditString, newAuditString);
+  set_field (pr, AUDIT_TRAIL (pr->database), finalAuditString, err);
+
+  free (finalAuditString);
+  free (newAuditString);
+  free (currDate);
+
+  return 0;
+}
+
+static int
+applyChangeAction (ChangeActions action, PR *pr, PR *oldPR, FieldIndex field,
+		   ErrorDesc *err, FormatNamedParameter *params)
+{
+  FieldEdit *fieldEdits = action->edits;
+  FieldList fields = action->requiredFields;
+
+  while (fields != NULL)
+    {
+      const char *fldval = get_field_value (pr, oldPR, fields->ent, NULL, NULL);
+      if (value_is_empty (fldval))
+	{
+	  setError (err, CODE_INVALID_PR_CONTENTS,
+		    "Required field %s missing from PR %s.",
+		    complexFieldIndexToString (fields->ent),
+		    field_value (pr, NUMBER (pr->database)));
+	  return 1;
+	}
+      fields = fields->next;
+    }
+
+  if (action->requireChangeReason && field_change_reason (pr, field) == NULL)
+    {
+      setError (err, CODE_INVALID_PR_CONTENTS,
+		"Edit of field %s requires a change reason.",
+		fieldDefForIndex (field)->name);
+      return 1;
+    }
+
+  while (fieldEdits != NULL)
+    {
+      if (applyFieldEdit (pr, fieldEdits, err, params) != 0)
+	{
+	  return 1;
+	}
+      fieldEdits = fieldEdits->next;
+    }
+  return 0;
 }
 
 static int
@@ -88,6 +163,110 @@ addAuditEntryP (const DatabaseInfo datab
 }
 
 static int
+applyChangeActions (PR *pr, PR *oldPR, FieldIndex field, 
+		    ChangeActions actions, ErrorDesc *err,
+		    FormatNamedParameter *params)
+{
+  {
+    ChangeActions actionList = actions;
+
+    while (actionList != NULL)
+      {
+	if (actionList->expr == NULL
+	    || pr_matches_expr (pr, oldPR, actionList->expr, params))
+	  {
+	    if (applyChangeAction (actionList, pr, oldPR, field, err, params))
+	      {
+		return 1;
+	      }
+	  }
+	actionList = actionList->next;
+      }
+  }
+
+  if (field != InvalidFieldIndex && addAuditEntryP (pr->database, 
+						    field, actions))
+    {
+      ChangeActions action = actions;
+      while (actions != NULL)
+	{
+	  if (actions->addAuditTrail)
+	    {
+	      break;
+	    }
+	  actions = actions->next;
+	}
+
+      if (action != NULL)
+	{
+	  addAuditTrailEnt (pr, action->auditTrailFormat, params, err);
+	}
+      else
+	{
+	  addAuditTrailEnt (pr, NULL, params, err);
+	}
+    }
+
+  return 0;
+}
+
+static int
+processFieldChange (PR *pr, PR *oldPR, FieldIndex field,
+		    ErrorDesc *err, const char *editUserEmailAddr,
+		    const char *oldValue, const char *newValue)
+{
+  ChangeActions actions;
+  FormatNamedParameter *params = NULL;
+  int res;
+
+  if (oldValue == NULL)
+    {
+      oldValue = "";
+    }
+  if (newValue == NULL)
+    {
+      newValue = "";
+    }
+
+  if (fieldDefForIndex (field)->readonly)
+    {
+      setError (err, CODE_READONLY_FIELD, 
+		newBadFieldEntry (field, NULL, NULL),
+		"Field %s is read-only: `%s'->`%s'",
+		fieldDefForIndex (field)->name,
+		oldValue,
+		newValue);
+      return 1;
+    }
+
+  {
+    char *currDate = get_curr_date ();
+    params = allocateNamedParameter ("$CurrentDate", currDate, params);
+    free (currDate);
+  }
+
+  params = allocateNamedParameter ("$FieldName", 
+				   fieldDefForIndex (field)->name, params);
+  params = allocateNamedParameter ("$OldValue", oldValue, params);
+  params = allocateNamedParameter ("$NewValue", newValue, params);
+  params = allocateNamedParameter ("$EditUserEmailAddr", editUserEmailAddr,
+				   params);
+  {
+    const char *reason = field_change_reason (pr, field);
+    if (reason == NULL)
+      {
+	reason = "";
+      }
+    params = allocateNamedParameter ("$ChangeReason", reason , params);
+  }
+
+  actions = fieldDefForIndex (field)->changeActions;
+  res = applyChangeActions (pr, oldPR, field, actions, err, params);
+  freeFormatParameterList (params);
+  return res;
+}
+
+static int
 processPRChanges (const char *editUserEmailAddr, PR *old_pr, PR *new_pr,
 		  ErrorDesc *err)
 {
@@ -152,7 +331,14 @@ processPRChanges (const char *editUserEm
 	  if (fieldDefForIndex (field)->readonly)
 	    {
 	      fieldsChanged[x] = 0;
-	      set_field (new_pr, field, old_value, err);
+	      if (old_value == NULL)
+	        {
+		  unsetField (new_pr, field);
+	        }
+	      else
+	        {
+		  set_field (new_pr, field, old_value, err);
+	        }
 	    }
 	  else
 	    {
@@ -211,85 +397,42 @@ processPRChanges (const char *editUserEm
 /* Replace an existing PR with the same PR number as NEW_PR.
    EDITUSEREMAILADDR is the email address of the user that is performing the
    edit.  */
-int
-rewrite_pr (PR *new_pr, const char *editUserEmailAddr, ErrorDesc *err)
+static int
+rewrite_pr (PR *pr, PR *new_pr, const char *editUserEmailAddr, ErrorDesc *err)
 {
   const DatabaseInfo database = new_pr->database;
   FILE *prfile;
 
-  char *path =      NULL;
-  char *old_path =  NULL;
+  char *new_path = NULL;
+  char *old_path = NULL;
   char *bkup_path = NULL;
 
-  PR *old_pr = NULL;
-
   int res = 1;
 
   newAuditTrailEntries = NULL;
 
-  if (field_value (new_pr, NUMBER (database)) == NULL)
-    {
-      free_pr (new_pr);
-      setError (err, CODE_INVALID_PR_CONTENTS,
-		"Missing PR number from new PR contents");
-      return 0;
-    }
-
   if (field_value (new_pr, CATEGORY (database)) == NULL)
     {
-      free_pr (new_pr);
       setError (err, CODE_INVALID_PR_CONTENTS,
 		"Missing category from new PR contents");
       return 0;
     }
 
-  if (! prExists (new_pr->database, field_value (new_pr, NUMBER (database)),
-		  err))
-    {
-      free_pr (new_pr);
-      return 0;
-    }
-
   if (! isPrLocked (new_pr->database, field_value (new_pr, NUMBER (database))))
     {
       setError (err, CODE_PR_NOT_LOCKED, "PR %s is not locked", 
 		field_value (new_pr, NUMBER (database)));
-      free_pr (new_pr);
       return 0;
     }
 
-  if (! check_pr (new_pr, err, 0))
+  if (processPRChanges (editUserEmailAddr, pr, new_pr, err) != 0)
     {
       return 0;
     }
 
-  old_pr = replaceCurrentPRInIndex (new_pr);
-
-  if (old_pr == NULL)
-    {
-      setError (err, CODE_NO_INDEX, "Unable to locate existing PR %s",
-		field_value (new_pr, NUMBER (database)));
-      free_pr (new_pr);
-      return 0;
-    }
-
-  old_path = gen_pr_path (old_pr);
-
-  /* set this to be the file to be saved. now called .old. */
-  asprintf (&bkup_path, "%s.old", old_path);
-
-  if (processPRChanges (editUserEmailAddr, old_pr, new_pr, err) != 0)
-    {
-      clearPRChain (database);
-      free_pr (old_pr);
-      free (bkup_path);
-      free (old_path);
-      return 0;
-    }
-
   /* check to see if the category changes, and if so, write the 
      new file out, and unlink the old one.  */
-  if (strcmp (field_value (old_pr, CATEGORY (database)),
+  if (strcmp (field_value (pr, CATEGORY (database)),
 	      field_value (new_pr, CATEGORY (database))) != 0)
     {
       char *dirPath;
@@ -309,10 +452,6 @@ rewrite_pr (PR *new_pr, const char *edit
 			    "Error creating directory for category %s: %s",
 			    field_value (new_pr, CATEGORY (database)),
 			    strerror (errno));
-		  clearPRChain (database);
-		  free_pr (old_pr);
-		  free (bkup_path);
-		  free (old_path);
 		  free (dirPath);
 		  return 0;
 		}
@@ -322,44 +461,32 @@ rewrite_pr (PR *new_pr, const char *edit
 	      setError (err, CODE_FILE_ERROR, 
 			"Directory for category %s does not exist",
 			field_value (new_pr, CATEGORY (database)));
-	      clearPRChain (database);
-	      free_pr (old_pr);
-	      free (bkup_path);
-	      free (old_path);
 	      free (dirPath);
 	      return 0;
 	    }
 	}
       free (dirPath);
-      path = gen_pr_path (new_pr);
-    }
-  else
-    {
-      path = gen_pr_path (old_pr);
     }
 
+  /* backup the current PR file */
+  old_path = gen_pr_path (pr);
+  asprintf (&bkup_path, "%s.old", old_path);
   if (rename (old_path, bkup_path) < 0)
     {
       if (errno != EXDEV)
 	{
 	  setError (err, CODE_FILE_ERROR, "Could not rename %s: %s",
 		    old_path, strerror (errno));
-	  clearPRChain (database);
-	  free_pr (old_pr);
 	  free (bkup_path);
 	  free (old_path);
-	  free (path);
 	  return 0;
 	}
       if (copy_file (old_path, bkup_path) != 0)
 	{
 	  setError (err, CODE_FILE_ERROR, "Could not copy %s to %s: %s",
 		    old_path, bkup_path, strerror (errno));
-	  clearPRChain (database);
-	  free_pr (old_pr);
 	  free (bkup_path);
 	  free (old_path);
-	  free (path);
 	  return 0;
 	}
 
@@ -369,16 +496,15 @@ rewrite_pr (PR *new_pr, const char *edit
     }
 
   /* Now build the file.  */
-  prfile = fopen (path, "w+");
+  new_path = gen_pr_path (new_pr);
+  prfile = fopen (new_path, "w+");
   if (prfile == (FILE *) NULL)
     {
       setError (err, CODE_FILE_ERROR, "Cannot write the PR to %s: %s",
-		path, strerror (errno));
-      clearPRChain (database);
-      free_pr (old_pr);
+		new_path, strerror (errno));
       free (bkup_path);
       free (old_path);
-      free (path);
+      free (new_path);
       return 0;
     }
 
@@ -394,25 +520,28 @@ rewrite_pr (PR *new_pr, const char *edit
   if (fclose (prfile) == EOF)
     {
       setError (err, CODE_FILE_ERROR, "Error writing out PR %s: %s",
-		path, strerror (errno));
-      clearPRChain (database);
-      free_pr (old_pr);
+		new_path, strerror (errno));
       free (bkup_path);
       free (old_path);
-      free (path);
+      free (new_path);
       return 0;
     }
 
   unlink (bkup_path);
 
   /* unlink the old file, if it is in another category dir. */
-  if (strcmp (field_value (old_pr, CATEGORY (database)),
+  if (strcmp (field_value (pr, CATEGORY (database)),
 	      field_value (new_pr, CATEGORY (database))) != 0)
     {
       unlink (old_path);
     }
 
+  free (bkup_path);
+  free (old_path);
+  free (new_path);
+
   /* write out the new index.  */
+  replaceCurrentPRInIndex (pr, new_pr, err);
   if (writeIndex (database, err) != 0)
     {
       res = 0;
@@ -421,16 +550,64 @@ rewrite_pr (PR *new_pr, const char *edit
   /* Send mail about the edit.  */
   if (newAuditTrailEntries != NULL)
     {
-      sendAuditMail (new_pr, old_pr, editUserEmailAddr, newAuditTrailEntries,
+      sendAuditMail (new_pr, pr, editUserEmailAddr, newAuditTrailEntries,
 		     err);
       free (newAuditTrailEntries);
       newAuditTrailEntries = NULL;
     }
 
-  free_pr (old_pr);
-  free (bkup_path);
-  free (old_path);
-  free (path);
+  return res;
+}
+
+
+int
+replace_pr (const DatabaseInfo database, FILE *fp,
+	    const char *editUserEmailAddr, ErrorDesc *err)
+{
+  int res;
+  const char *prnum = NULL;
+  PR *curr_pr;
+  PR *new_pr = allocPR (database);
+
+  /* Read the message header in.  */
+  if (read_header (new_pr, fp) < 0)
+    {
+      setError (err, CODE_EOF_PR, "Couldn't read PR header");
+      return 0;
+    }
+
+  read_pr (new_pr, fp, 0);
+
+  prnum = field_value (new_pr, NUMBER (database));
+  if (prnum == NULL)
+    {
+      setError (err, CODE_INVALID_PR_CONTENTS,
+	        "Missing PR number from new PR contents");
+      return 0;
+    }
+
+  if (! prExists (database, prnum, err))
+    {
+      return 0;
+    }
+
+  curr_pr = readPRWithNum (database, prnum, 0, err);
+  if (curr_pr == NULL)
+    {
+      setError (err, CODE_NO_INDEX,
+	        "Unable to locate existing PR %s", prnum);
+      return 0;
+    }
+
+  if (! check_pr (new_pr, err, 0))
+    {
+      return 0;
+    }
+
+  res = rewrite_pr (curr_pr, new_pr, editUserEmailAddr, err);
+
+  free_pr (curr_pr);
+  free_pr (new_pr);
 
   return res;
 }
@@ -815,17 +992,25 @@ edit_field (const DatabaseInfo database,
   char pid[32];
   const char *oldfield;
   int res;
-  PR *pr;
+  PR *pr, *new_pr;
 
   if (! prExists (database, prnum, err))
     {
       free (newcontents);
+      if (changeReason != NULL)
+        {
+	  free (changeReason);
+        }
       return 0;
     }
   if (fieldIndex == InvalidFieldIndex)
     {
       setError (err, CODE_INVALID_FIELD_NAME, "Invalid field name");
       free (newcontents);
+      if (changeReason != NULL)
+        {
+	  free (changeReason);
+        }
       return 0;
     }
   if (requiresChangeReason (fieldDefForIndex (fieldIndex))
@@ -837,10 +1022,15 @@ edit_field (const DatabaseInfo database,
       free (newcontents);
       return 0;
     }
+
   sprintf (pid, "%d", (int) getpid ());
   if (lock_pr (database, prnum, "edit_field", pid, err) == 0)
     {
       free (newcontents);
+      if (changeReason != NULL)
+        {
+	  free (changeReason);
+        }
       return 0;
     }
 
@@ -849,7 +1039,6 @@ edit_field (const DatabaseInfo database,
     {
       unlock_pr (database, prnum, err);
       setError (err, CODE_UNREADABLE_PR, "Error reading PR %s.", prnum);
-      free_pr (pr);
       free (newcontents);
       if (changeReason != NULL)
 	{
@@ -876,36 +1065,58 @@ edit_field (const DatabaseInfo database,
       newcontents = totalVal;
     }
 
+  if (!validateFieldValue (fieldIndex, newcontents, err, 0))
+    {
+      unlock_pr (database, prnum, err);
+      free (newcontents);
+      if (changeReason != NULL)
+        {
+	  free (changeReason);
+        }
+      return 0;
+    }
+
+  /* create a copy of the currently active pr so that we can modify it and
+     do subsequent validation allowing for us to bail out cleanly */
+  new_pr = allocPR (database);
+  set_field (new_pr, NUMBER (database), prnum, err);
+  set_field (new_pr, CATEGORY (database), field_value (pr, CATEGORY (database)),
+	     err);
+  fillInPR (new_pr, err);
+
   /* set_field () verifies that the value is valid before doing the
      set.  */
-  if (set_field (pr, fieldIndex, newcontents, err) == 0)
+  /* set_field returns a boolean!!! */
+  if (set_field (new_pr, fieldIndex, newcontents, err) == 0)
     {
       res = 0;
-      free_pr (pr);
     }
   else
     {
       if (changeReason != NULL)
 	{
-	  setFieldChangeReason (pr, fieldIndex, changeReason);
+	  setFieldChangeReason (new_pr, fieldIndex, changeReason);
 	  free (changeReason);
 	  changeReason = NULL;
 	}
 
-      res = rewrite_pr (pr, editUserEmailAddr, err);
-    }
-
-  if (unlock_pr (database, prnum, err) == 0)
-    {
-      res = 0;
+      res = rewrite_pr (pr, new_pr, editUserEmailAddr, err);
     }
 
+  free_pr (pr);
+  free_pr (new_pr);
   free (newcontents);
   if (changeReason != NULL)
-    {
+  {
       free (changeReason);
       changeReason = NULL;
+  }
+
+  if (unlock_pr (database, prnum, err) == 0)
+    {
+      res = 0;
     }
+
   return res;
 }
 
@@ -1029,262 +1240,33 @@ applyFieldEdit (PR *pr, FieldEdit *edit,
   return res;
 }
 
-/* Add an entry to the AUDIT_TRAIL field. PARAMS are the format
-   parameters used when composing the entry.  FMT is the format of the
-   audit-trail entry to add.  */
-
-static int
-addAuditTrailEnt (PR *pr, QueryFormat *fmt, 
-		  FormatNamedParameter *params, ErrorDesc *err)
-{
-  char *newAuditString = NULL;
-  char *finalAuditString;
-  const char *t;
-  char *currDate = get_curr_date ();
-
-  if (fmt == NULL)
-    {
-      fmt = getAuditTrailFormat (pr->database);
-    }
-
-  /* Format the new audit entry. */
-  process_format (NULL, &newAuditString, pr, NULL, fmt, "\n", params);
-
-  /* Squirrel it away, because we'll need to mail it later.  */
-  append_string (&newAuditTrailEntries, newAuditString);
-
-  /* Now append the formatted string to the audit-trail field.  */
-  t = field_value (pr, AUDIT_TRAIL (pr->database));
-  if (t == NULL)
-    {
-      t = "";
-    }
-  finalAuditString = xstrdup (t);
-  append_string (&finalAuditString, newAuditString);
-  set_field (pr, AUDIT_TRAIL (pr->database), finalAuditString, err);
-
-  free (finalAuditString);
-  free (newAuditString);
-  free (currDate);
-
-  return 0;
-}
-
-static void
-sendAuditMail (PR *pr, PR *oldPR, const char *editUserEmailAddr, 
-	       const char *newAuditTrailEntries, ErrorDesc *err)
-{
-  FormatNamedParameter *parms = NULL;
-  const DatabaseInfo database = pr->database;
-
-  parms = allocateNamedParameter ("$EditUserEmailAddr", editUserEmailAddr,
-				  parms);
-  parms = allocateNamedParameter ("$NewAuditTrail", newAuditTrailEntries,
-				  parms);
-
-  if (strcmp (field_value (pr, RESPONSIBLE (database)),
-	      field_value (oldPR, RESPONSIBLE (database))) != 0)
-    {
-      parms
-	= allocateNamedParameter ("$OldResponsible",
-				  field_value (oldPR, RESPONSIBLE (database)),
-				  parms);
-    }
-
-  composeMailMessage (pr, oldPR, "audit-mail", parms, NULL, err);
-  freeFormatParameterList (parms);
-}
-
-static int
-applyChangeAction (ChangeActions action, PR *pr, PR *oldPR, FieldIndex field,
-		   ErrorDesc *err, FormatNamedParameter *params)
-{
-  FieldEdit *fieldEdits = action->edits;
-  FieldList fields = action->requiredFields;
-
-  while (fields != NULL)
-    {
-      const char *fldval = get_field_value (pr, oldPR, fields->ent, NULL, NULL);
-      if (value_is_empty (fldval))
-	{
-	  setError (err, CODE_INVALID_PR_CONTENTS,
-		    "Required field %s missing from PR %s.",
-		    complexFieldIndexToString (fields->ent),
-		    field_value (pr, NUMBER (pr->database)));
-	  return 1;
-	}
-      fields = fields->next;
-    }
-
-  if (action->requireChangeReason && field_change_reason (pr, field) == NULL)
-    {
-      setError (err, CODE_INVALID_PR_CONTENTS,
-		"Edit of field %s requires a change reason.",
-		fieldDefForIndex (field)->name);
-      return 1;
-    }
-
-  while (fieldEdits != NULL)
-    {
-      if (applyFieldEdit (pr, fieldEdits, err, params) != 0)
-	{
-	  return 1;
-	}
-      fieldEdits = fieldEdits->next;
-    }
-  return 0;
-}
-
-static int
-applyChangeActions (PR *pr, PR *oldPR, FieldIndex field, 
-		    ChangeActions actions, ErrorDesc *err,
-		    FormatNamedParameter *params)
-{
-  {
-    ChangeActions actionList = actions;
-
-    while (actionList != NULL)
-      {
-	if (actionList->expr == NULL
-	    || pr_matches_expr (pr, oldPR, actionList->expr, params))
-	  {
-	    if (applyChangeAction (actionList, pr, oldPR, field, err, params))
-	      {
-		return 1;
-	      }
-	  }
-	actionList = actionList->next;
-      }
-  }
-
-  if (field != InvalidFieldIndex && addAuditEntryP (pr->database, 
-						    field, actions))
-    {
-      ChangeActions action = actions;
-      while (actions != NULL)
-	{
-	  if (actions->addAuditTrail)
-	    {
-	      break;
-	    }
-	  actions = actions->next;
-	}
-
-      if (action != NULL)
-	{
-	  addAuditTrailEnt (pr, action->auditTrailFormat, params, err);
-	}
-      else
-	{
-	  addAuditTrailEnt (pr, NULL, params, err);
-	}
-    }
-
-  return 0;
-}
-
-static int
-processFieldChange (PR *pr, PR *oldPR, FieldIndex field,
-		    ErrorDesc *err, const char *editUserEmailAddr,
-		    const char *oldValue, const char *newValue)
-{
-  ChangeActions actions;
-  FormatNamedParameter *params = NULL;
-  int res;
-
-  if (oldValue == NULL)
-    {
-      oldValue = "";
-    }
-  if (newValue == NULL)
-    {
-      newValue = "";
-    }
-
-  if (fieldDefForIndex (field)->readonly)
-    {
-      setError (err, CODE_READONLY_FIELD, 
-		newBadFieldEntry (field, NULL, NULL),
-		"Field %s is read-only: `%s'->`%s'",
-		fieldDefForIndex (field)->name,
-		oldValue,
-		newValue);
-      return 1;
-    }
-
-  {
-    char *currDate = get_curr_date ();
-    params = allocateNamedParameter ("$CurrentDate", currDate, params);
-    free (currDate);
-  }
-
-  params = allocateNamedParameter ("$FieldName", 
-				   fieldDefForIndex (field)->name, params);
-  params = allocateNamedParameter ("$OldValue", oldValue, params);
-  params = allocateNamedParameter ("$NewValue", newValue, params);
-  params = allocateNamedParameter ("$EditUserEmailAddr", editUserEmailAddr,
-				   params);
-  {
-    const char *reason = field_change_reason (pr, field);
-    if (reason == NULL)
-      {
-	reason = "";
-      }
-    params = allocateNamedParameter ("$ChangeReason", reason , params);
-  }
-
-  actions = fieldDefForIndex (field)->changeActions;
-  res = applyChangeActions (pr, oldPR, field, actions, err, params);
-  freeFormatParameterList (params);
-  return res;
-}
-
 /* Delete PR PRNUM.  */
 int
 deletePR (const DatabaseInfo database, const char *prnum, 
 	  const char *editUserEmailAddr, ErrorDesc *err)
 {
-  char *path = lookup_pr_path (database, prnum, err);
   char pid[32];
   FormatNamedParameter *parms = NULL;
-
-  if (path == NULL)
-    {
-      return -1;
-    }
+  short delete_status;
 
   if (client_lock_gnats (database, err))
     {
-      free (path);
       return -4;
     }
 
   if (lock_pr (database, prnum, GNATS_USER, pid, err) == 0)
     {
       client_unlock_gnats ();
-      free (path);
       return -4;
     }
 
-  if (removePRFromIndex (database, prnum, err))
-    {
-      unlock_pr (database, prnum, err);
-      client_unlock_gnats ();
-      free (path);
-      return -5;
-    }
-  
-  if (unlink (path))
+  if ((delete_status = pr_delete (database, prnum, err)) < 0)
     {
-      setError (err, CODE_FILE_ERROR, "Unable to unlink file %s\n", path);
       unlock_pr (database, prnum, err);
       client_unlock_gnats ();
-      free (path);
-      return -6;
+      return delete_status;
     }
 
-  free (path);
-
   if (unlock_pr (database, prnum, err) == 0)
     {
       client_unlock_gnats ();
Index: file-pr.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/file-pr.c,v
retrieving revision 1.54
diff -u -p -r1.54 file-pr.c
--- file-pr.c	26 Oct 2003 07:33:26 -0000	1.54
+++ file-pr.c	21 Feb 2005 12:19:39 -0000
@@ -118,13 +118,14 @@ createNewPRFile (PR *pr, int flag_autocr
 	}
     }
 
-  if (submitter->admFields[SubmitterAdmRtime][0] != '\0')
+  if (submitter->admFields[SubmitterAdmRtime] == NULL ||
+      submitter->admFields[SubmitterAdmRtime][0] == '\0')
     {
-      submitter_rtime = atoi (submitter->admFields[SubmitterAdmRtime]);
+      submitter_rtime = -1;
     }
   else
     {
-      submitter_rtime = -1;
+      submitter_rtime = atoi (submitter->admFields[SubmitterAdmRtime]);
     }
 
   /* If originator wasn't set, try to get the value from the From mail
Index: gnats.h
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnats.h,v
retrieving revision 1.53
diff -u -p -r1.53 gnats.h
--- gnats.h	30 Aug 2003 07:58:14 -0000	1.53
+++ gnats.h	21 Feb 2005 12:19:39 -0000
@@ -315,8 +315,6 @@ extern int check_pr (PR *pr,
 		     int initial_entry);
 extern int replace_pr (const DatabaseInfo database, FILE *infile, 
 		       const char *editUserEmailAddr, ErrorDesc *err);
-extern int rewrite_pr (PR *pr, const char *editUserEmailAddr, 
-		       ErrorDesc *err);
 extern int lock_pr (const DatabaseInfo database,
 		    const char *filename, const char *username,
 		    const char *processid, ErrorDesc *error);
Index: gnatsd.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.c,v
retrieving revision 1.49
diff -u -p -r1.49 gnatsd.c
--- gnatsd.c	30 Nov 2004 15:43:42 -0000	1.49
+++ gnatsd.c	21 Feb 2005 12:19:39 -0000
@@ -478,7 +478,7 @@ findUserAccessLevel (const char *file, c
 			 requested database. */
 		      const char *l2 = ent->admFields[3];
 		      
-		      if (! strlen(l2))
+		      if (l2 == NULL || ! strlen(l2))
 			found = 1;
 		      
 		      while (l2 != NULL && ! found)
Index: index.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/index.c,v
retrieving revision 1.40
diff -u -p -r1.40 index.c
--- index.c	1 Dec 2001 22:52:12 -0000	1.40
+++ index.c	21 Feb 2005 12:19:40 -0000
@@ -61,6 +61,7 @@ struct indexEntry
   char *buf;
   char **fields;
   PR *nextPR;
+  PR *prevPR;
 };
 
 static PR *nextIndexEntryBinary (IndexFileDesc fp);
@@ -234,6 +235,7 @@ nextIndexEntry (IndexFileDesc fp)
   p = res->index;
   seplen = strlen (fp->desc->separator);
 
+  p->prevPR = NULL;
   p->nextPR = NULL;
   p->fields = (char **) xmalloc (sizeof (char *) 
 				 * get_num_fields (fp->desc->database));
@@ -326,6 +328,7 @@ nextIndexEntryBinary (IndexFileDesc fp)
 
       res = allocPR (fp->desc->database);
       indexEnt = res->index;
+      indexEnt->prevPR = NULL;
       indexEnt->nextPR = NULL;
       indexEnt->fields 
 	= (char **) xmalloc (sizeof (char *) 
@@ -573,6 +576,7 @@ getIndex (IndexDesc indexDesc, ErrorDesc
 	    }
 	  else
 	    {
+	      i->index->prevPR = end_chain;
 	      end_chain->index->nextPR = i;
 	      end_chain = i;
 	    }
@@ -662,6 +666,7 @@ allocIndex (PR *pr)
   pr->index = (Index *) xmalloc (sizeof (Index));
   pr->index->buf = NULL;
   pr->index->fields = NULL;
+  pr->index->prevPR = NULL;
   pr->index->nextPR = NULL;
 }
  
@@ -1115,34 +1120,57 @@ clearPRChain (const DatabaseInfo databas
     }
 }
 
-/* Find the PR in the current index with the same PR number as NEW_PR,
-   and replace it with NEW_PR.  The old PR is returned, or a NULL
-   pointer if a PR with the same number is not found.  */
+/* Replace a PR in the index with a copy of a new PR.
+   By generating a copy of the new PR, we allow for the new PR
+   to be free'd without harming the index pr chain. */
 
-PR *
-replaceCurrentPRInIndex (PR *new_pr)
+int
+replaceCurrentPRInIndex (PR *curr_pr, PR *new_pr, ErrorDesc *err)
 {
-  ErrorDesc err;
-  IndexDesc indexDesc = getIndexDesc (new_pr->database);
-  PR **currptr = &(indexDesc->prChain);
-  const char *new_pr_num = field_value (new_pr, NUMBER (new_pr->database));
+  PR *pr_replace, *pr_copy;
+  const DatabaseInfo database = curr_pr->database;
+  const char *new_pr_num = field_value (new_pr, NUMBER (database));
+  const char *curr_pr_num = field_value (curr_pr, NUMBER (database));
+
+  if (strcmp (curr_pr_num, new_pr_num) != 0)
+    {
+      return 0;
+    }
+
+  /* create a copy of the new pr to be put into the index pr chain */
+  pr_copy = allocPR (database);
+  set_field (pr_copy, NUMBER (curr_pr->database), new_pr_num, err);
+  set_field (pr_copy, CATEGORY (database),
+	     field_value (new_pr, CATEGORY (database)), err);
+  setPrevPR (pr_copy, curr_pr->index->prevPR);
+  setNextPR (pr_copy, curr_pr->index->nextPR);
+  fillInPR (pr_copy, err);
+
+  /* put the copy of new_pr into place in the index pr chain... */
+
+  if (curr_pr->index->prevPR == NULL)
+    {
+      /* the pr to be replaced is at the head of the pr chain */
+      IndexDesc indexDesc = getIndexDesc (database);
+      free_pr (indexDesc->prChain);
+      indexDesc->prChain = pr_copy;
+      return 1;
+    }
 
-  getFirstPR (new_pr->database, &err);
+  /* save a pointer to the pr to be replaced so that it can later be free'd */
+  pr_replace = getNextPR (curr_pr->index->prevPR);
 
-  while ((*currptr) != NULL)
+  setNextPR (curr_pr->index->prevPR, pr_copy);
+
+  if (curr_pr->index->nextPR != NULL)
     {
-      if (strcmp (field_value (*currptr, NUMBER (new_pr->database)),
-		  new_pr_num) == 0)
-	{
-	  PR *old_pr = (*currptr);
-	  new_pr->index->nextPR = old_pr->index->nextPR;
-	  old_pr->index->nextPR = NULL;
-	  (*currptr) = new_pr;
-	  return old_pr;
-	}
-      currptr = &((*currptr)->index->nextPR);
+      /* the pr to be replaced is not at the end of the pr chain */
+      setPrevPR (curr_pr->index->nextPR, pr_copy);
     }
-  return NULL;
+
+  free_pr (pr_replace);
+
+  return 1;
 }
 
 /* Remove PR PRNUM from the index for the current database, and rewrite the
@@ -1196,6 +1224,24 @@ getNextPR (PR *pr)
   return pr->index->nextPR;
 }
 
+void
+setNextPR (PR *pr, PR *next_pr)
+{
+  pr->index->nextPR = next_pr;
+}
+
+PR *
+getPrevPR (PR *pr)
+{
+  return pr->index->prevPR;
+}
+
+void
+setPrevPR (PR *pr, PR *prev_pr)
+{
+  pr->index->prevPR = prev_pr;
+}
+
 int
 indexIsBinary (const DatabaseInfo database)
 {
Index: index.h
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/index.h,v
retrieving revision 1.17
diff -u -p -r1.17 index.h
--- index.h	15 Apr 2001 18:11:51 -0000	1.17
+++ index.h	21 Feb 2005 12:19:40 -0000
@@ -24,7 +24,10 @@ extern int checkPRChain (const DatabaseI
 
 extern PR *getFirstPR (const DatabaseInfo database, ErrorDesc *err);
 
+extern PR *getPrevPR (PR *pr);
 extern PR *getNextPR (PR *pr);
+extern void setPrevPR (PR *pr, PR *prev_pr);
+extern void setNextPR (PR *pr, PR *next_pr);
 
 extern void freePRIndex (PR *pr);
 
@@ -46,7 +49,7 @@ extern int addToIndex (PR *pr, ErrorDesc
 extern char *createIndexEntry (PR *pr, size_t *entLen);
 extern char *createIndexEntryBinary (PR *pr, size_t *entLen);
 
-extern PR *replaceCurrentPRInIndex (PR *newPR);
+extern int replaceCurrentPRInIndex (PR *curr_pr, PR *newPR, ErrorDesc *err);
 
 extern char *indexValue (PR *pr, FieldIndex field);
 
Index: mail.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/mail.c,v
retrieving revision 1.22
diff -u -p -r1.22 mail.c
--- mail.c	30 Nov 2004 15:43:50 -0000	1.22
+++ mail.c	21 Feb 2005 12:19:40 -0000
@@ -65,9 +65,13 @@ get_responsible_address (const DatabaseI
      passwd file.  */
   if (res != NULL)
     {
-      if (res->admFields[ResponsibleAdmAlias] == '\0')
+      if (res->admFields[ResponsibleAdmAlias] == NULL ||
+	  res->admFields[ResponsibleAdmAlias] == '\0')
 	{
-	  free (res->admFields[ResponsibleAdmAlias]);
+	  if (res->admFields[ResponsibleAdmAlias] != NULL)
+	    {
+	      free (res->admFields[ResponsibleAdmAlias]);
+	    }
 	  res->admFields[ResponsibleAdmAlias] 
 	    = xstrdup (res->admFields[ResponsibleAdmKey]);
 	}
Index: pr.c
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/pr.c,v
retrieving revision 1.64
diff -u -p -r1.64 pr.c
--- pr.c	25 Nov 2002 13:58:33 -0000	1.64
+++ pr.c	21 Feb 2005 12:19:40 -0000
@@ -154,7 +154,6 @@ allocPR (const DatabaseInfo database)
 }
 
 /* Get the PR at PATH.  PR is the PR entry to be filled in.
-
    If PRUNE is non-zero, don't read any multitext fields.  */
 static int
 get_pr (PR *pr, const char *path, int prune)
@@ -211,6 +210,33 @@ fillInPR (PR *pr, ErrorDesc *err)
     }
 }
 
+static PR *
+get_pr_from_index (const DatabaseInfo database, const char *prnum,
+		   ErrorDesc *err)
+{
+  PR *pr = getFirstPR (database, err);
+
+  /* If they gave it to us with the category, remove it. */
+  if (( strrchr (prnum, '/')) != NULL)
+    {
+      prnum = strrchr (prnum, '/') + 1;
+    }
+
+  while (pr != NULL && strcmp (prnum, field_value (pr, NUMBER (database))) != 0)
+    {
+      pr = getNextPR (pr);
+    }
+
+  if (pr == NULL)
+    {
+      setError (err, CODE_NONEXISTENT_PR,
+	        "No PR %s listed in the index.", prnum);
+      return NULL;
+    }
+
+  return pr;
+}
+
 /* Initializes PR for reading.  Each line of the PR should be passed
    in via addLineToPR (). */
 
@@ -1348,29 +1374,16 @@ free_pr (PR *pr)
 }
 
 static char *
-get_pr_path (const DatabaseInfo database,const char *prnum, ErrorDesc *err)
+get_pr_path (const DatabaseInfo database, PR *pr, const char *prnum,
+	    ErrorDesc *err)
 {
   char *path = NULL;
   const char *category = NULL;
-  PR *curr_pr_chain = getFirstPR (database, err);
   char *categoryFromIndex = NULL;
 
-  if (curr_pr_chain != NULL)
+  if (pr != NULL)
     {
-      PR *j;
-      for (j = curr_pr_chain ; j != NULL ; j = getNextPR (j))
-	{
-	  if (strcmp (prnum, field_value (j, NUMBER (database))) == 0)
-	    {
-	      category = field_value (j, CATEGORY (database));
-	      break;
-	    }
-	}
-      if (category == NULL)
-	{
-	  setError (err, CODE_NONEXISTENT_PR,
-		    "No PR %s listed in the index.", prnum);
-	}
+      category = field_value (pr, CATEGORY (database));
     }
   else
     {
@@ -1391,9 +1404,10 @@ get_pr_path (const DatabaseInfo database
 }
 
 int
-prExists (const DatabaseInfo database, const char *prID, ErrorDesc *err)
+prExists (const DatabaseInfo database, const char *prnum, ErrorDesc *err)
 {
-  char *path = get_pr_path (database, prID, err);
+  PR *pr = get_pr_from_index (database, prnum, err);
+  char *path = get_pr_path (database, pr, prnum, err);
   int res = 0;
   if (path != NULL)
     {
@@ -1407,48 +1421,85 @@ prExists (const DatabaseInfo database, c
   return res;
 }
 
-char *
-lookup_pr_path (const DatabaseInfo database, const char *prnum, ErrorDesc *err)
+static bool
+pr_file_readable (const char *path, ErrorDesc *err)
 {
-  char *path;
   FILE *fp;
 
-  /* If they gave it to us with the category, remove it. */
-  if (( strrchr (prnum, '/')) != NULL)
+  if (path == NULL)
     {
-      prnum = strrchr (prnum, '/') + 1;
+      return FALSE;
     }
-  path = get_pr_path (database, prnum, err);
-  if ((path == NULL) || ((fp = fopen (path, "r")) == NULL))
+
+  if ((fp = fopen (path, "r")) == NULL)
     {
-      if (path != NULL)
-	{
-	  setError (err, CODE_FILE_ERROR, "Can't open file `%s'", path);
-	}
-      return NULL;
+      setError (err, CODE_FILE_ERROR, "Can't open file `%s'", path);
+      return FALSE;
     }
 
   fclose (fp);
-  return path;
+  return TRUE;
 }
 
 PR *
-readPRWithNum (const DatabaseInfo database, const char *prID,
+readPRWithNum (const DatabaseInfo database, const char *prnum,
 	       int prune, ErrorDesc *err)
 {
   PR *pr = NULL;
-  char *path = lookup_pr_path (database, prID, err);
+  PR *index_pr = get_pr_from_index (database, prnum, err);
+  char *path = get_pr_path (database, index_pr, prnum, err);
 
   if (path != NULL)
     {
-      pr = allocPR (database);
-      if (get_pr (pr, path, prune) == 0)
+      if (pr_file_readable (path, err))
+        {
+	  pr = allocPR (database);
+	  setPrevPR (pr, getPrevPR (index_pr));
+	  setNextPR (pr, getNextPR (index_pr));
+	  if (get_pr (pr, path, prune) == 0)
+	    {
+	      free_pr (pr);
+	      pr = NULL;
+	    }
+        }
+      free (path);
+    }
+  return pr;
+}
+
+int
+pr_delete (const DatabaseInfo database, const char *prnum, ErrorDesc *err)
+{
+  PR *pr;
+  char *path = NULL;
+
+  pr = get_pr_from_index (database, prnum, err);
+  path = get_pr_path (database, pr, prnum, err);
+
+  if (path == NULL || !pr_file_readable (path, err))
+    {
+      if (path != NULL)
 	{
-	  free_pr (pr);
-	  pr = NULL;
+	  free (path);
 	}
+      return -1;
     }
-  return pr;
+
+  if (removePRFromIndex (database, prnum, err))
+    {
+      free (path);
+      return -5;
+    }
+  
+  if (unlink (path))
+    {
+      setError (err, CODE_FILE_ERROR, "Unable to unlink file %s\n", path);
+      free (path);
+      return -6;
+    }
+
+  free (path);
+  return 1;
 }
 
 void
Index: pr.h
===================================================================
RCS file: /cvsroot/gnats/gnats/gnats/pr.h,v
retrieving revision 1.38
diff -u -p -r1.38 pr.h
--- pr.h	15 Apr 2001 18:11:51 -0000	1.38
+++ pr.h	21 Feb 2005 12:19:40 -0000
@@ -131,8 +131,8 @@ extern int fconfigParse (DatabaseInfo da
 extern int createPrFile (PR *pr, const char *filename, int force,
 			 ErrorDesc *err);
 
-extern char *lookup_pr_path (const DatabaseInfo database,
-			     const char *prnum, ErrorDesc *);
+extern int pr_delete (const DatabaseInfo database, const char *prnum,
+		      ErrorDesc *err);
 
 extern int prExists (const DatabaseInfo database, const char *prID, 
 		     ErrorDesc *err);

[-- Attachment #3: Type: text/plain, Size: 140 bytes --]

_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://lists.gnu.org/mailman/listinfo/help-gnats

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: patch #1 - towards a generic backend datastore
  2005-02-21 12:41 patch #1 - towards a generic backend datastore Mel Hatzis
@ 2005-02-23  7:24 ` Mel Hatzis
  2005-02-24  0:10   ` Chad Walstrom
  0 siblings, 1 reply; 8+ messages in thread
From: Mel Hatzis @ 2005-02-23  7:24 UTC (permalink / raw)
  To: Chad Walstrom; +Cc: help-gnats

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

No one has responded to my patch thus far. I plan on committing
the changes Thursday evening (PST) provided no one speaks up in
the meantime.

- --
Mel Hatzis

Mel Hatzis wrote:
> Please review the attached patch which contains a number of bug fixes
> in addition to modifications intended to restructure the code to
> support a generalized backend datastore.
> 
> I have developed a copy of GNATS (which I'm calling gnats-dbi)
> which utilizes a backend datastore api and can be configured with
> either a flat-file or Oracle backend. Anyone interested can download
> a copy from http://professional.juniper.net/gnats/.
> 
> This patch is the first of many I plan to submit to merge my
> changes back into the official GNATS project and thereby enhance
> it with the ability to have multiple backends.
> 
> This first patch is rather large. Hopefully, most of the subsequent
> ones will be easier to digest. The primary goal in this patch
> was to begin isolating the index logic from the PR edit functionality.
> A number of performance improvements have been made in the process.
> 
> Note that I started developing and testing primarily on my Mac and
> this exposed a few memory related issues which I decided to fix
> in the process.
> 
> --
> Mel Hatzis
> 
> 
> ------------------------------------------------------------------------
> 
> Index: ChangeLog
> ===================================================================
> RCS file: /cvsroot/gnats/gnats/gnats/ChangeLog,v
> retrieving revision 1.308
> diff -u -p -r1.308 ChangeLog
> --- ChangeLog	30 Nov 2004 15:44:37 -0000	1.308
> +++ ChangeLog	21 Feb 2005 12:19:38 -0000
> @@ -1,3 +1,71 @@
> +2005-02-19  Mel Hatzis  <hatzis@wattes.org>
> +
> +	* adm.c (copy_adm_entry): Handle blank metadata fields by correctly
> +	assigning the relevant field in the adm struct to NULL - and thereby
> +	avoid an attempt to xstrdup a NULL value.
> +
> +	* file-pr.c (createNewPRFile): Avoid a NULL pointer dereference when
> +	there's a blank response time field in the submitters file.
> +
> +	* gnatsd.c (findUserAccessLevel): Avoid a strlen of NULL when the
> +	gnatsd.user_access file contains a blank 'database-alias' field.
> +
> +	* gnats.h (rewrite_pr): Made it a static function in edit.c - it
> +	wasn't being used outside this scope anyway.
> +
> +	* mail.c (get_responsible_address): Avoid a NULL pointer dereference
> +	if the responsible file doesn't have an email address for the
> +	gnats-admin user.
> +
> +	* index.c, index.h (getPrevPR, setPrevPR, setNextPR): New functions.
> +	The index chain is now doubly linked allowing edited PRs to be
> +	replaced without incurring the overhead of a search through the
> +	chain. As well as offering significantly better performance, this
> +	supports a change to the PR edit functionality whereby we now start
> +	with a copy of the old PR as well as the updated PR instead of
> +	retrieving the old PR (from the index) in the guts of rewrite_pr.
> +	The motivation behind this change is to isolate the index
> +	functionality and move toward a generic backend datastore which
> +	doesn't require the index.
> +	(replaceCurrentPRInIndex): Modified signature. Pass in the PR to be
> +	replaced (as well as the new PR) since we can now use the doubly
> +	linked index chain to replace the PR without searching for it.
> +
> +	* pr.h (lookup_pr_path): Removed. Replaced by a call to the new
> +	get_pr_from_index () function followed by a call to (a modified)
> +	get_pr_path (). This change allows a single index search to be done
> +	once (early on in a given call stack) to retrieve a PR and
> +	thereafter access the PR file without requiring  more index
> +	searches.
> +	(pr_delete): New function. Contains the flat-file specific logic
> +	related to PR deletions which will be moved out into a flat-file
> +	backend when the datastore backend is generalized.
> +
> +	* pr.c (get_pr_from_index): New function. Retrieve a PR from the
> +	index.
> +	(pr_file_readable): New function. Check if a file can be opened for
> +	reading.
> +	(pr_delete): New function. Flat-file specific logic for delete.
> +	(prExists): Now utilize get_pr_from_index () and get_pr_path ()
> +	instead of the deprecated lookup_pr_path (). 
> +	(readPRWithNum): Ditto. Also set the PR's next/prev index pointers.
> +	(get_pr_path): New signature. Take in a PR * instead of searching
> +	the index.
> +	(lookup_pr_path): Deprecated.
> +
> +	* edit.c (applyChangeActions, processFieldChange, sendAuditMail):
> +	Repositioned within the file so as not to require prototypes.
> +	(processPRChanges): Handle NULL values appropriately when undoing
> +	changes to modified read-only fields.
> +	(rewrite_pr, replace_pr, edit_field): rewrite_pr now takes in a copy
> +	of the old pr instead of retrieving it from the index. This is in
> +	preparation for allowing it to be used with more than just the flat
> +	file backend datastore. Also moved some error checking out of
> +	rewrite_pr and simplified it...no need to validate the whole PR
> +	when editing a single field.
> +	(deletePR): Moved the file-related logic into a new pr_delete function
> +	inside pr.c thereby preparing for a general backend datastore.
> +
>  2004-11-30  Chad Walstrom  <chewie@wookimus.net>
>  
>  	* gnatsd.c, gnats-pwconv.c (HAVE_CRYPT_H): Changed HAVE_CRYPT to
> Index: adm.c
> ===================================================================
> RCS file: /cvsroot/gnats/gnats/gnats/adm.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 adm.c
> --- adm.c	27 Oct 2002 06:21:17 -0000	1.20
> +++ adm.c	21 Feb 2005 12:19:38 -0000
> @@ -47,7 +47,9 @@ copy_adm_entry (AdmEntry *ent)
>    res->field = ent->field;
>    for (x = 0; x < ent->fieldcount; x++)
>      {
> -      res->admFields[x] = xstrdup (ent->admFields[x]);
> +      res->admFields[x] = (ent->admFields[x] == NULL)
> +			    ? NULL
> +			    : xstrdup (ent->admFields[x]);
>      }
>    return res;
>  }
> Index: edit.c
> ===================================================================
> RCS file: /cvsroot/gnats/gnats/gnats/edit.c,v
> retrieving revision 1.60
> diff -u -p -r1.60 edit.c
> --- edit.c	25 Nov 2002 13:58:33 -0000	1.60
> +++ edit.c	21 Feb 2005 12:19:39 -0000
> @@ -24,38 +24,113 @@ Software Foundation, 59 Temple Place - S
>  #include "query.h"
>  #include "mail.h"
>  
> -static int applyChangeActions (PR *pr, PR *oldPR, FieldIndex field, 
> -			       ChangeActions actions, ErrorDesc *err,
> -			       FormatNamedParameter *params);
> -
> -static int processFieldChange (PR *pr, PR *oldPR, FieldIndex field,
> -			       ErrorDesc *err, 
> -			       const char *editUserEmailAddr,
> -			       const char *oldValue, const char *newValue);
> -
>  /* Euuugh.  There's gotta be a better way to keep this around.  */
>  static char *newAuditTrailEntries = NULL;
>  
> -static void sendAuditMail (PR *pr, PR *oldPR, const char *editUserEmailAddr,
> -			   const char *newAuditTrailEntries,
> -			   ErrorDesc *err);
> +static void
> +sendAuditMail (PR *pr, PR *oldPR, const char *editUserEmailAddr, 
> +	       const char *newAuditTrailEntries, ErrorDesc *err)
> +{
> +  FormatNamedParameter *parms = NULL;
> +  const DatabaseInfo database = pr->database;
>  
> -int
> -replace_pr (const DatabaseInfo database, FILE *fp,
> -	    const char *editUserEmailAddr, ErrorDesc *err)
> +  parms = allocateNamedParameter ("$EditUserEmailAddr", editUserEmailAddr,
> +				  parms);
> +  parms = allocateNamedParameter ("$NewAuditTrail", newAuditTrailEntries,
> +				  parms);
> +
> +  if (strcmp (field_value (pr, RESPONSIBLE (database)),
> +	      field_value (oldPR, RESPONSIBLE (database))) != 0)
> +    {
> +      parms
> +	= allocateNamedParameter ("$OldResponsible",
> +				  field_value (oldPR, RESPONSIBLE (database)),
> +				  parms);
> +    }
> +
> +  composeMailMessage (pr, oldPR, "audit-mail", parms, NULL, err);
> +  freeFormatParameterList (parms);
> +}
> +
> +/* Add an entry to the AUDIT_TRAIL field. PARAMS are the format
> +   parameters used when composing the entry.  FMT is the format of the
> +   audit-trail entry to add.  */
> +
> +static int
> +addAuditTrailEnt (PR *pr, QueryFormat *fmt, 
> +		  FormatNamedParameter *params, ErrorDesc *err)
>  {
> -  PR *pr = allocPR (database);
> +  char *newAuditString = NULL;
> +  char *finalAuditString;
> +  const char *t;
> +  char *currDate = get_curr_date ();
>  
> -  /* Read the message header in.  */
> -  if (read_header (pr, fp) < 0)
> +  if (fmt == NULL)
>      {
> -      setError (err, CODE_EOF_PR, "Couldn't read PR header");
> -      return 0;
> +      fmt = getAuditTrailFormat (pr->database);
>      }
>  
> -  read_pr (pr, fp, 0);
> +  /* Format the new audit entry. */
> +  process_format (NULL, &newAuditString, pr, NULL, fmt, "\n", params);
>  
> -  return rewrite_pr (pr, editUserEmailAddr, err);
> +  /* Squirrel it away, because we'll need to mail it later.  */
> +  append_string (&newAuditTrailEntries, newAuditString);
> +
> +  /* Now append the formatted string to the audit-trail field.  */
> +  t = field_value (pr, AUDIT_TRAIL (pr->database));
> +  if (t == NULL)
> +    {
> +      t = "";
> +    }
> +  finalAuditString = xstrdup (t);
> +  append_string (&finalAuditString, newAuditString);
> +  set_field (pr, AUDIT_TRAIL (pr->database), finalAuditString, err);
> +
> +  free (finalAuditString);
> +  free (newAuditString);
> +  free (currDate);
> +
> +  return 0;
> +}
> +
> +static int
> +applyChangeAction (ChangeActions action, PR *pr, PR *oldPR, FieldIndex field,
> +		   ErrorDesc *err, FormatNamedParameter *params)
> +{
> +  FieldEdit *fieldEdits = action->edits;
> +  FieldList fields = action->requiredFields;
> +
> +  while (fields != NULL)
> +    {
> +      const char *fldval = get_field_value (pr, oldPR, fields->ent, NULL, NULL);
> +      if (value_is_empty (fldval))
> +	{
> +	  setError (err, CODE_INVALID_PR_CONTENTS,
> +		    "Required field %s missing from PR %s.",
> +		    complexFieldIndexToString (fields->ent),
> +		    field_value (pr, NUMBER (pr->database)));
> +	  return 1;
> +	}
> +      fields = fields->next;
> +    }
> +
> +  if (action->requireChangeReason && field_change_reason (pr, field) == NULL)
> +    {
> +      setError (err, CODE_INVALID_PR_CONTENTS,
> +		"Edit of field %s requires a change reason.",
> +		fieldDefForIndex (field)->name);
> +      return 1;
> +    }
> +
> +  while (fieldEdits != NULL)
> +    {
> +      if (applyFieldEdit (pr, fieldEdits, err, params) != 0)
> +	{
> +	  return 1;
> +	}
> +      fieldEdits = fieldEdits->next;
> +    }
> +  return 0;
>  }
>  
>  static int
> @@ -88,6 +163,110 @@ addAuditEntryP (const DatabaseInfo datab
>  }
>  
>  static int
> +applyChangeActions (PR *pr, PR *oldPR, FieldIndex field, 
> +		    ChangeActions actions, ErrorDesc *err,
> +		    FormatNamedParameter *params)
> +{
> +  {
> +    ChangeActions actionList = actions;
> +
> +    while (actionList != NULL)
> +      {
> +	if (actionList->expr == NULL
> +	    || pr_matches_expr (pr, oldPR, actionList->expr, params))
> +	  {
> +	    if (applyChangeAction (actionList, pr, oldPR, field, err, params))
> +	      {
> +		return 1;
> +	      }
> +	  }
> +	actionList = actionList->next;
> +      }
> +  }
> +
> +  if (field != InvalidFieldIndex && addAuditEntryP (pr->database, 
> +						    field, actions))
> +    {
> +      ChangeActions action = actions;
> +      while (actions != NULL)
> +	{
> +	  if (actions->addAuditTrail)
> +	    {
> +	      break;
> +	    }
> +	  actions = actions->next;
> +	}
> +
> +      if (action != NULL)
> +	{
> +	  addAuditTrailEnt (pr, action->auditTrailFormat, params, err);
> +	}
> +      else
> +	{
> +	  addAuditTrailEnt (pr, NULL, params, err);
> +	}
> +    }
> +
> +  return 0;
> +}
> +
> +static int
> +processFieldChange (PR *pr, PR *oldPR, FieldIndex field,
> +		    ErrorDesc *err, const char *editUserEmailAddr,
> +		    const char *oldValue, const char *newValue)
> +{
> +  ChangeActions actions;
> +  FormatNamedParameter *params = NULL;
> +  int res;
> +
> +  if (oldValue == NULL)
> +    {
> +      oldValue = "";
> +    }
> +  if (newValue == NULL)
> +    {
> +      newValue = "";
> +    }
> +
> +  if (fieldDefForIndex (field)->readonly)
> +    {
> +      setError (err, CODE_READONLY_FIELD, 
> +		newBadFieldEntry (field, NULL, NULL),
> +		"Field %s is read-only: `%s'->`%s'",
> +		fieldDefForIndex (field)->name,
> +		oldValue,
> +		newValue);
> +      return 1;
> +    }
> +
> +  {
> +    char *currDate = get_curr_date ();
> +    params = allocateNamedParameter ("$CurrentDate", currDate, params);
> +    free (currDate);
> +  }
> +
> +  params = allocateNamedParameter ("$FieldName", 
> +				   fieldDefForIndex (field)->name, params);
> +  params = allocateNamedParameter ("$OldValue", oldValue, params);
> +  params = allocateNamedParameter ("$NewValue", newValue, params);
> +  params = allocateNamedParameter ("$EditUserEmailAddr", editUserEmailAddr,
> +				   params);
> +  {
> +    const char *reason = field_change_reason (pr, field);
> +    if (reason == NULL)
> +      {
> +	reason = "";
> +      }
> +    params = allocateNamedParameter ("$ChangeReason", reason , params);
> +  }
> +
> +  actions = fieldDefForIndex (field)->changeActions;
> +  res = applyChangeActions (pr, oldPR, field, actions, err, params);
> +  freeFormatParameterList (params);
> +  return res;
> +}
> +
> +static int
>  processPRChanges (const char *editUserEmailAddr, PR *old_pr, PR *new_pr,
>  		  ErrorDesc *err)
>  {
> @@ -152,7 +331,14 @@ processPRChanges (const char *editUserEm
>  	  if (fieldDefForIndex (field)->readonly)
>  	    {
>  	      fieldsChanged[x] = 0;
> -	      set_field (new_pr, field, old_value, err);
> +	      if (old_value == NULL)
> +	        {
> +		  unsetField (new_pr, field);
> +	        }
> +	      else
> +	        {
> +		  set_field (new_pr, field, old_value, err);
> +	        }
>  	    }
>  	  else
>  	    {
> @@ -211,85 +397,42 @@ processPRChanges (const char *editUserEm
>  /* Replace an existing PR with the same PR number as NEW_PR.
>     EDITUSEREMAILADDR is the email address of the user that is performing the
>     edit.  */
> -int
> -rewrite_pr (PR *new_pr, const char *editUserEmailAddr, ErrorDesc *err)
> +static int
> +rewrite_pr (PR *pr, PR *new_pr, const char *editUserEmailAddr, ErrorDesc *err)
>  {
>    const DatabaseInfo database = new_pr->database;
>    FILE *prfile;
>  
> -  char *path =      NULL;
> -  char *old_path =  NULL;
> +  char *new_path = NULL;
> +  char *old_path = NULL;
>    char *bkup_path = NULL;
>  
> -  PR *old_pr = NULL;
> -
>    int res = 1;
>  
>    newAuditTrailEntries = NULL;
>  
> -  if (field_value (new_pr, NUMBER (database)) == NULL)
> -    {
> -      free_pr (new_pr);
> -      setError (err, CODE_INVALID_PR_CONTENTS,
> -		"Missing PR number from new PR contents");
> -      return 0;
> -    }
> -
>    if (field_value (new_pr, CATEGORY (database)) == NULL)
>      {
> -      free_pr (new_pr);
>        setError (err, CODE_INVALID_PR_CONTENTS,
>  		"Missing category from new PR contents");
>        return 0;
>      }
>  
> -  if (! prExists (new_pr->database, field_value (new_pr, NUMBER (database)),
> -		  err))
> -    {
> -      free_pr (new_pr);
> -      return 0;
> -    }
> -
>    if (! isPrLocked (new_pr->database, field_value (new_pr, NUMBER (database))))
>      {
>        setError (err, CODE_PR_NOT_LOCKED, "PR %s is not locked", 
>  		field_value (new_pr, NUMBER (database)));
> -      free_pr (new_pr);
>        return 0;
>      }
>  
> -  if (! check_pr (new_pr, err, 0))
> +  if (processPRChanges (editUserEmailAddr, pr, new_pr, err) != 0)
>      {
>        return 0;
>      }
>  
> -  old_pr = replaceCurrentPRInIndex (new_pr);
> -
> -  if (old_pr == NULL)
> -    {
> -      setError (err, CODE_NO_INDEX, "Unable to locate existing PR %s",
> -		field_value (new_pr, NUMBER (database)));
> -      free_pr (new_pr);
> -      return 0;
> -    }
> -
> -  old_path = gen_pr_path (old_pr);
> -
> -  /* set this to be the file to be saved. now called .old. */
> -  asprintf (&bkup_path, "%s.old", old_path);
> -
> -  if (processPRChanges (editUserEmailAddr, old_pr, new_pr, err) != 0)
> -    {
> -      clearPRChain (database);
> -      free_pr (old_pr);
> -      free (bkup_path);
> -      free (old_path);
> -      return 0;
> -    }
> -
>    /* check to see if the category changes, and if so, write the 
>       new file out, and unlink the old one.  */
> -  if (strcmp (field_value (old_pr, CATEGORY (database)),
> +  if (strcmp (field_value (pr, CATEGORY (database)),
>  	      field_value (new_pr, CATEGORY (database))) != 0)
>      {
>        char *dirPath;
> @@ -309,10 +452,6 @@ rewrite_pr (PR *new_pr, const char *edit
>  			    "Error creating directory for category %s: %s",
>  			    field_value (new_pr, CATEGORY (database)),
>  			    strerror (errno));
> -		  clearPRChain (database);
> -		  free_pr (old_pr);
> -		  free (bkup_path);
> -		  free (old_path);
>  		  free (dirPath);
>  		  return 0;
>  		}
> @@ -322,44 +461,32 @@ rewrite_pr (PR *new_pr, const char *edit
>  	      setError (err, CODE_FILE_ERROR, 
>  			"Directory for category %s does not exist",
>  			field_value (new_pr, CATEGORY (database)));
> -	      clearPRChain (database);
> -	      free_pr (old_pr);
> -	      free (bkup_path);
> -	      free (old_path);
>  	      free (dirPath);
>  	      return 0;
>  	    }
>  	}
>        free (dirPath);
> -      path = gen_pr_path (new_pr);
> -    }
> -  else
> -    {
> -      path = gen_pr_path (old_pr);
>      }
>  
> +  /* backup the current PR file */
> +  old_path = gen_pr_path (pr);
> +  asprintf (&bkup_path, "%s.old", old_path);
>    if (rename (old_path, bkup_path) < 0)
>      {
>        if (errno != EXDEV)
>  	{
>  	  setError (err, CODE_FILE_ERROR, "Could not rename %s: %s",
>  		    old_path, strerror (errno));
> -	  clearPRChain (database);
> -	  free_pr (old_pr);
>  	  free (bkup_path);
>  	  free (old_path);
> -	  free (path);
>  	  return 0;
>  	}
>        if (copy_file (old_path, bkup_path) != 0)
>  	{
>  	  setError (err, CODE_FILE_ERROR, "Could not copy %s to %s: %s",
>  		    old_path, bkup_path, strerror (errno));
> -	  clearPRChain (database);
> -	  free_pr (old_pr);
>  	  free (bkup_path);
>  	  free (old_path);
> -	  free (path);
>  	  return 0;
>  	}
>  
> @@ -369,16 +496,15 @@ rewrite_pr (PR *new_pr, const char *edit
>      }
>  
>    /* Now build the file.  */
> -  prfile = fopen (path, "w+");
> +  new_path = gen_pr_path (new_pr);
> +  prfile = fopen (new_path, "w+");
>    if (prfile == (FILE *) NULL)
>      {
>        setError (err, CODE_FILE_ERROR, "Cannot write the PR to %s: %s",
> -		path, strerror (errno));
> -      clearPRChain (database);
> -      free_pr (old_pr);
> +		new_path, strerror (errno));
>        free (bkup_path);
>        free (old_path);
> -      free (path);
> +      free (new_path);
>        return 0;
>      }
>  
> @@ -394,25 +520,28 @@ rewrite_pr (PR *new_pr, const char *edit
>    if (fclose (prfile) == EOF)
>      {
>        setError (err, CODE_FILE_ERROR, "Error writing out PR %s: %s",
> -		path, strerror (errno));
> -      clearPRChain (database);
> -      free_pr (old_pr);
> +		new_path, strerror (errno));
>        free (bkup_path);
>        free (old_path);
> -      free (path);
> +      free (new_path);
>        return 0;
>      }
>  
>    unlink (bkup_path);
>  
>    /* unlink the old file, if it is in another category dir. */
> -  if (strcmp (field_value (old_pr, CATEGORY (database)),
> +  if (strcmp (field_value (pr, CATEGORY (database)),
>  	      field_value (new_pr, CATEGORY (database))) != 0)
>      {
>        unlink (old_path);
>      }
>  
> +  free (bkup_path);
> +  free (old_path);
> +  free (new_path);
> +
>    /* write out the new index.  */
> +  replaceCurrentPRInIndex (pr, new_pr, err);
>    if (writeIndex (database, err) != 0)
>      {
>        res = 0;
> @@ -421,16 +550,64 @@ rewrite_pr (PR *new_pr, const char *edit
>    /* Send mail about the edit.  */
>    if (newAuditTrailEntries != NULL)
>      {
> -      sendAuditMail (new_pr, old_pr, editUserEmailAddr, newAuditTrailEntries,
> +      sendAuditMail (new_pr, pr, editUserEmailAddr, newAuditTrailEntries,
>  		     err);
>        free (newAuditTrailEntries);
>        newAuditTrailEntries = NULL;
>      }
>  
> -  free_pr (old_pr);
> -  free (bkup_path);
> -  free (old_path);
> -  free (path);
> +  return res;
> +}
> +
> +
> +int
> +replace_pr (const DatabaseInfo database, FILE *fp,
> +	    const char *editUserEmailAddr, ErrorDesc *err)
> +{
> +  int res;
> +  const char *prnum = NULL;
> +  PR *curr_pr;
> +  PR *new_pr = allocPR (database);
> +
> +  /* Read the message header in.  */
> +  if (read_header (new_pr, fp) < 0)
> +    {
> +      setError (err, CODE_EOF_PR, "Couldn't read PR header");
> +      return 0;
> +    }
> +
> +  read_pr (new_pr, fp, 0);
> +
> +  prnum = field_value (new_pr, NUMBER (database));
> +  if (prnum == NULL)
> +    {
> +      setError (err, CODE_INVALID_PR_CONTENTS,
> +	        "Missing PR number from new PR contents");
> +      return 0;
> +    }
> +
> +  if (! prExists (database, prnum, err))
> +    {
> +      return 0;
> +    }
> +
> +  curr_pr = readPRWithNum (database, prnum, 0, err);
> +  if (curr_pr == NULL)
> +    {
> +      setError (err, CODE_NO_INDEX,
> +	        "Unable to locate existing PR %s", prnum);
> +      return 0;
> +    }
> +
> +  if (! check_pr (new_pr, err, 0))
> +    {
> +      return 0;
> +    }
> +
> +  res = rewrite_pr (curr_pr, new_pr, editUserEmailAddr, err);
> +
> +  free_pr (curr_pr);
> +  free_pr (new_pr);
>  
>    return res;
>  }
> @@ -815,17 +992,25 @@ edit_field (const DatabaseInfo database,
>    char pid[32];
>    const char *oldfield;
>    int res;
> -  PR *pr;
> +  PR *pr, *new_pr;
>  
>    if (! prExists (database, prnum, err))
>      {
>        free (newcontents);
> +      if (changeReason != NULL)
> +        {
> +	  free (changeReason);
> +        }
>        return 0;
>      }
>    if (fieldIndex == InvalidFieldIndex)
>      {
>        setError (err, CODE_INVALID_FIELD_NAME, "Invalid field name");
>        free (newcontents);
> +      if (changeReason != NULL)
> +        {
> +	  free (changeReason);
> +        }
>        return 0;
>      }
>    if (requiresChangeReason (fieldDefForIndex (fieldIndex))
> @@ -837,10 +1022,15 @@ edit_field (const DatabaseInfo database,
>        free (newcontents);
>        return 0;
>      }
> +
>    sprintf (pid, "%d", (int) getpid ());
>    if (lock_pr (database, prnum, "edit_field", pid, err) == 0)
>      {
>        free (newcontents);
> +      if (changeReason != NULL)
> +        {
> +	  free (changeReason);
> +        }
>        return 0;
>      }
>  
> @@ -849,7 +1039,6 @@ edit_field (const DatabaseInfo database,
>      {
>        unlock_pr (database, prnum, err);
>        setError (err, CODE_UNREADABLE_PR, "Error reading PR %s.", prnum);
> -      free_pr (pr);
>        free (newcontents);
>        if (changeReason != NULL)
>  	{
> @@ -876,36 +1065,58 @@ edit_field (const DatabaseInfo database,
>        newcontents = totalVal;
>      }
>  
> +  if (!validateFieldValue (fieldIndex, newcontents, err, 0))
> +    {
> +      unlock_pr (database, prnum, err);
> +      free (newcontents);
> +      if (changeReason != NULL)
> +        {
> +	  free (changeReason);
> +        }
> +      return 0;
> +    }
> +
> +  /* create a copy of the currently active pr so that we can modify it and
> +     do subsequent validation allowing for us to bail out cleanly */
> +  new_pr = allocPR (database);
> +  set_field (new_pr, NUMBER (database), prnum, err);
> +  set_field (new_pr, CATEGORY (database), field_value (pr, CATEGORY (database)),
> +	     err);
> +  fillInPR (new_pr, err);
> +
>    /* set_field () verifies that the value is valid before doing the
>       set.  */
> -  if (set_field (pr, fieldIndex, newcontents, err) == 0)
> +  /* set_field returns a boolean!!! */
> +  if (set_field (new_pr, fieldIndex, newcontents, err) == 0)
>      {
>        res = 0;
> -      free_pr (pr);
>      }
>    else
>      {
>        if (changeReason != NULL)
>  	{
> -	  setFieldChangeReason (pr, fieldIndex, changeReason);
> +	  setFieldChangeReason (new_pr, fieldIndex, changeReason);
>  	  free (changeReason);
>  	  changeReason = NULL;
>  	}
>  
> -      res = rewrite_pr (pr, editUserEmailAddr, err);
> -    }
> -
> -  if (unlock_pr (database, prnum, err) == 0)
> -    {
> -      res = 0;
> +      res = rewrite_pr (pr, new_pr, editUserEmailAddr, err);
>      }
>  
> +  free_pr (pr);
> +  free_pr (new_pr);
>    free (newcontents);
>    if (changeReason != NULL)
> -    {
> +  {
>        free (changeReason);
>        changeReason = NULL;
> +  }
> +
> +  if (unlock_pr (database, prnum, err) == 0)
> +    {
> +      res = 0;
>      }
> +
>    return res;
>  }
>  
> @@ -1029,262 +1240,33 @@ applyFieldEdit (PR *pr, FieldEdit *edit,
>    return res;
>  }
>  
> -/* Add an entry to the AUDIT_TRAIL field. PARAMS are the format
> -   parameters used when composing the entry.  FMT is the format of the
> -   audit-trail entry to add.  */
> -
> -static int
> -addAuditTrailEnt (PR *pr, QueryFormat *fmt, 
> -		  FormatNamedParameter *params, ErrorDesc *err)
> -{
> -  char *newAuditString = NULL;
> -  char *finalAuditString;
> -  const char *t;
> -  char *currDate = get_curr_date ();
> -
> -  if (fmt == NULL)
> -    {
> -      fmt = getAuditTrailFormat (pr->database);
> -    }
> -
> -  /* Format the new audit entry. */
> -  process_format (NULL, &newAuditString, pr, NULL, fmt, "\n", params);
> -
> -  /* Squirrel it away, because we'll need to mail it later.  */
> -  append_string (&newAuditTrailEntries, newAuditString);
> -
> -  /* Now append the formatted string to the audit-trail field.  */
> -  t = field_value (pr, AUDIT_TRAIL (pr->database));
> -  if (t == NULL)
> -    {
> -      t = "";
> -    }
> -  finalAuditString = xstrdup (t);
> -  append_string (&finalAuditString, newAuditString);
> -  set_field (pr, AUDIT_TRAIL (pr->database), finalAuditString, err);
> -
> -  free (finalAuditString);
> -  free (newAuditString);
> -  free (currDate);
> -
> -  return 0;
> -}
> -
> -static void
> -sendAuditMail (PR *pr, PR *oldPR, const char *editUserEmailAddr, 
> -	       const char *newAuditTrailEntries, ErrorDesc *err)
> -{
> -  FormatNamedParameter *parms = NULL;
> -  const DatabaseInfo database = pr->database;
> -
> -  parms = allocateNamedParameter ("$EditUserEmailAddr", editUserEmailAddr,
> -				  parms);
> -  parms = allocateNamedParameter ("$NewAuditTrail", newAuditTrailEntries,
> -				  parms);
> -
> -  if (strcmp (field_value (pr, RESPONSIBLE (database)),
> -	      field_value (oldPR, RESPONSIBLE (database))) != 0)
> -    {
> -      parms
> -	= allocateNamedParameter ("$OldResponsible",
> -				  field_value (oldPR, RESPONSIBLE (database)),
> -				  parms);
> -    }
> -
> -  composeMailMessage (pr, oldPR, "audit-mail", parms, NULL, err);
> -  freeFormatParameterList (parms);
> -}
> -
> -static int
> -applyChangeAction (ChangeActions action, PR *pr, PR *oldPR, FieldIndex field,
> -		   ErrorDesc *err, FormatNamedParameter *params)
> -{
> -  FieldEdit *fieldEdits = action->edits;
> -  FieldList fields = action->requiredFields;
> -
> -  while (fields != NULL)
> -    {
> -      const char *fldval = get_field_value (pr, oldPR, fields->ent, NULL, NULL);
> -      if (value_is_empty (fldval))
> -	{
> -	  setError (err, CODE_INVALID_PR_CONTENTS,
> -		    "Required field %s missing from PR %s.",
> -		    complexFieldIndexToString (fields->ent),
> -		    field_value (pr, NUMBER (pr->database)));
> -	  return 1;
> -	}
> -      fields = fields->next;
> -    }
> -
> -  if (action->requireChangeReason && field_change_reason (pr, field) == NULL)
> -    {
> -      setError (err, CODE_INVALID_PR_CONTENTS,
> -		"Edit of field %s requires a change reason.",
> -		fieldDefForIndex (field)->name);
> -      return 1;
> -    }
> -
> -  while (fieldEdits != NULL)
> -    {
> -      if (applyFieldEdit (pr, fieldEdits, err, params) != 0)
> -	{
> -	  return 1;
> -	}
> -      fieldEdits = fieldEdits->next;
> -    }
> -  return 0;
> -}
> -
> -static int
> -applyChangeActions (PR *pr, PR *oldPR, FieldIndex field, 
> -		    ChangeActions actions, ErrorDesc *err,
> -		    FormatNamedParameter *params)
> -{
> -  {
> -    ChangeActions actionList = actions;
> -
> -    while (actionList != NULL)
> -      {
> -	if (actionList->expr == NULL
> -	    || pr_matches_expr (pr, oldPR, actionList->expr, params))
> -	  {
> -	    if (applyChangeAction (actionList, pr, oldPR, field, err, params))
> -	      {
> -		return 1;
> -	      }
> -	  }
> -	actionList = actionList->next;
> -      }
> -  }
> -
> -  if (field != InvalidFieldIndex && addAuditEntryP (pr->database, 
> -						    field, actions))
> -    {
> -      ChangeActions action = actions;
> -      while (actions != NULL)
> -	{
> -	  if (actions->addAuditTrail)
> -	    {
> -	      break;
> -	    }
> -	  actions = actions->next;
> -	}
> -
> -      if (action != NULL)
> -	{
> -	  addAuditTrailEnt (pr, action->auditTrailFormat, params, err);
> -	}
> -      else
> -	{
> -	  addAuditTrailEnt (pr, NULL, params, err);
> -	}
> -    }
> -
> -  return 0;
> -}
> -
> -static int
> -processFieldChange (PR *pr, PR *oldPR, FieldIndex field,
> -		    ErrorDesc *err, const char *editUserEmailAddr,
> -		    const char *oldValue, const char *newValue)
> -{
> -  ChangeActions actions;
> -  FormatNamedParameter *params = NULL;
> -  int res;
> -
> -  if (oldValue == NULL)
> -    {
> -      oldValue = "";
> -    }
> -  if (newValue == NULL)
> -    {
> -      newValue = "";
> -    }
> -
> -  if (fieldDefForIndex (field)->readonly)
> -    {
> -      setError (err, CODE_READONLY_FIELD, 
> -		newBadFieldEntry (field, NULL, NULL),
> -		"Field %s is read-only: `%s'->`%s'",
> -		fieldDefForIndex (field)->name,
> -		oldValue,
> -		newValue);
> -      return 1;
> -    }
> -
> -  {
> -    char *currDate = get_curr_date ();
> -    params = allocateNamedParameter ("$CurrentDate", currDate, params);
> -    free (currDate);
> -  }
> -
> -  params = allocateNamedParameter ("$FieldName", 
> -				   fieldDefForIndex (field)->name, params);
> -  params = allocateNamedParameter ("$OldValue", oldValue, params);
> -  params = allocateNamedParameter ("$NewValue", newValue, params);
> -  params = allocateNamedParameter ("$EditUserEmailAddr", editUserEmailAddr,
> -				   params);
> -  {
> -    const char *reason = field_change_reason (pr, field);
> -    if (reason == NULL)
> -      {
> -	reason = "";
> -      }
> -    params = allocateNamedParameter ("$ChangeReason", reason , params);
> -  }
> -
> -  actions = fieldDefForIndex (field)->changeActions;
> -  res = applyChangeActions (pr, oldPR, field, actions, err, params);
> -  freeFormatParameterList (params);
> -  return res;
> -}
> -
>  /* Delete PR PRNUM.  */
>  int
>  deletePR (const DatabaseInfo database, const char *prnum, 
>  	  const char *editUserEmailAddr, ErrorDesc *err)
>  {
> -  char *path = lookup_pr_path (database, prnum, err);
>    char pid[32];
>    FormatNamedParameter *parms = NULL;
> -
> -  if (path == NULL)
> -    {
> -      return -1;
> -    }
> +  short delete_status;
>  
>    if (client_lock_gnats (database, err))
>      {
> -      free (path);
>        return -4;
>      }
>  
>    if (lock_pr (database, prnum, GNATS_USER, pid, err) == 0)
>      {
>        client_unlock_gnats ();
> -      free (path);
>        return -4;
>      }
>  
> -  if (removePRFromIndex (database, prnum, err))
> -    {
> -      unlock_pr (database, prnum, err);
> -      client_unlock_gnats ();
> -      free (path);
> -      return -5;
> -    }
> -  
> -  if (unlink (path))
> +  if ((delete_status = pr_delete (database, prnum, err)) < 0)
>      {
> -      setError (err, CODE_FILE_ERROR, "Unable to unlink file %s\n", path);
>        unlock_pr (database, prnum, err);
>        client_unlock_gnats ();
> -      free (path);
> -      return -6;
> +      return delete_status;
>      }
>  
> -  free (path);
> -
>    if (unlock_pr (database, prnum, err) == 0)
>      {
>        client_unlock_gnats ();
> Index: file-pr.c
> ===================================================================
> RCS file: /cvsroot/gnats/gnats/gnats/file-pr.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 file-pr.c
> --- file-pr.c	26 Oct 2003 07:33:26 -0000	1.54
> +++ file-pr.c	21 Feb 2005 12:19:39 -0000
> @@ -118,13 +118,14 @@ createNewPRFile (PR *pr, int flag_autocr
>  	}
>      }
>  
> -  if (submitter->admFields[SubmitterAdmRtime][0] != '\0')
> +  if (submitter->admFields[SubmitterAdmRtime] == NULL ||
> +      submitter->admFields[SubmitterAdmRtime][0] == '\0')
>      {
> -      submitter_rtime = atoi (submitter->admFields[SubmitterAdmRtime]);
> +      submitter_rtime = -1;
>      }
>    else
>      {
> -      submitter_rtime = -1;
> +      submitter_rtime = atoi (submitter->admFields[SubmitterAdmRtime]);
>      }
>  
>    /* If originator wasn't set, try to get the value from the From mail
> Index: gnats.h
> ===================================================================
> RCS file: /cvsroot/gnats/gnats/gnats/gnats.h,v
> retrieving revision 1.53
> diff -u -p -r1.53 gnats.h
> --- gnats.h	30 Aug 2003 07:58:14 -0000	1.53
> +++ gnats.h	21 Feb 2005 12:19:39 -0000
> @@ -315,8 +315,6 @@ extern int check_pr (PR *pr,
>  		     int initial_entry);
>  extern int replace_pr (const DatabaseInfo database, FILE *infile, 
>  		       const char *editUserEmailAddr, ErrorDesc *err);
> -extern int rewrite_pr (PR *pr, const char *editUserEmailAddr, 
> -		       ErrorDesc *err);
>  extern int lock_pr (const DatabaseInfo database,
>  		    const char *filename, const char *username,
>  		    const char *processid, ErrorDesc *error);
> Index: gnatsd.c
> ===================================================================
> RCS file: /cvsroot/gnats/gnats/gnats/gnatsd.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 gnatsd.c
> --- gnatsd.c	30 Nov 2004 15:43:42 -0000	1.49
> +++ gnatsd.c	21 Feb 2005 12:19:39 -0000
> @@ -478,7 +478,7 @@ findUserAccessLevel (const char *file, c
>  			 requested database. */
>  		      const char *l2 = ent->admFields[3];
>  		      
> -		      if (! strlen(l2))
> +		      if (l2 == NULL || ! strlen(l2))
>  			found = 1;
>  		      
>  		      while (l2 != NULL && ! found)
> Index: index.c
> ===================================================================
> RCS file: /cvsroot/gnats/gnats/gnats/index.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 index.c
> --- index.c	1 Dec 2001 22:52:12 -0000	1.40
> +++ index.c	21 Feb 2005 12:19:40 -0000
> @@ -61,6 +61,7 @@ struct indexEntry
>    char *buf;
>    char **fields;
>    PR *nextPR;
> +  PR *prevPR;
>  };
>  
>  static PR *nextIndexEntryBinary (IndexFileDesc fp);
> @@ -234,6 +235,7 @@ nextIndexEntry (IndexFileDesc fp)
>    p = res->index;
>    seplen = strlen (fp->desc->separator);
>  
> +  p->prevPR = NULL;
>    p->nextPR = NULL;
>    p->fields = (char **) xmalloc (sizeof (char *) 
>  				 * get_num_fields (fp->desc->database));
> @@ -326,6 +328,7 @@ nextIndexEntryBinary (IndexFileDesc fp)
>  
>        res = allocPR (fp->desc->database);
>        indexEnt = res->index;
> +      indexEnt->prevPR = NULL;
>        indexEnt->nextPR = NULL;
>        indexEnt->fields 
>  	= (char **) xmalloc (sizeof (char *) 
> @@ -573,6 +576,7 @@ getIndex (IndexDesc indexDesc, ErrorDesc
>  	    }
>  	  else
>  	    {
> +	      i->index->prevPR = end_chain;
>  	      end_chain->index->nextPR = i;
>  	      end_chain = i;
>  	    }
> @@ -662,6 +666,7 @@ allocIndex (PR *pr)
>    pr->index = (Index *) xmalloc (sizeof (Index));
>    pr->index->buf = NULL;
>    pr->index->fields = NULL;
> +  pr->index->prevPR = NULL;
>    pr->index->nextPR = NULL;
>  }
>   
> @@ -1115,34 +1120,57 @@ clearPRChain (const DatabaseInfo databas
>      }
>  }
>  
> -/* Find the PR in the current index with the same PR number as NEW_PR,
> -   and replace it with NEW_PR.  The old PR is returned, or a NULL
> -   pointer if a PR with the same number is not found.  */
> +/* Replace a PR in the index with a copy of a new PR.
> +   By generating a copy of the new PR, we allow for the new PR
> +   to be free'd without harming the index pr chain. */
>  
> -PR *
> -replaceCurrentPRInIndex (PR *new_pr)
> +int
> +replaceCurrentPRInIndex (PR *curr_pr, PR *new_pr, ErrorDesc *err)
>  {
> -  ErrorDesc err;
> -  IndexDesc indexDesc = getIndexDesc (new_pr->database);
> -  PR **currptr = &(indexDesc->prChain);
> -  const char *new_pr_num = field_value (new_pr, NUMBER (new_pr->database));
> +  PR *pr_replace, *pr_copy;
> +  const DatabaseInfo database = curr_pr->database;
> +  const char *new_pr_num = field_value (new_pr, NUMBER (database));
> +  const char *curr_pr_num = field_value (curr_pr, NUMBER (database));
> +
> +  if (strcmp (curr_pr_num, new_pr_num) != 0)
> +    {
> +      return 0;
> +    }
> +
> +  /* create a copy of the new pr to be put into the index pr chain */
> +  pr_copy = allocPR (database);
> +  set_field (pr_copy, NUMBER (curr_pr->database), new_pr_num, err);
> +  set_field (pr_copy, CATEGORY (database),
> +	     field_value (new_pr, CATEGORY (database)), err);
> +  setPrevPR (pr_copy, curr_pr->index->prevPR);
> +  setNextPR (pr_copy, curr_pr->index->nextPR);
> +  fillInPR (pr_copy, err);
> +
> +  /* put the copy of new_pr into place in the index pr chain... */
> +
> +  if (curr_pr->index->prevPR == NULL)
> +    {
> +      /* the pr to be replaced is at the head of the pr chain */
> +      IndexDesc indexDesc = getIndexDesc (database);
> +      free_pr (indexDesc->prChain);
> +      indexDesc->prChain = pr_copy;
> +      return 1;
> +    }
>  
> -  getFirstPR (new_pr->database, &err);
> +  /* save a pointer to the pr to be replaced so that it can later be free'd */
> +  pr_replace = getNextPR (curr_pr->index->prevPR);
>  
> -  while ((*currptr) != NULL)
> +  setNextPR (curr_pr->index->prevPR, pr_copy);
> +
> +  if (curr_pr->index->nextPR != NULL)
>      {
> -      if (strcmp (field_value (*currptr, NUMBER (new_pr->database)),
> -		  new_pr_num) == 0)
> -	{
> -	  PR *old_pr = (*currptr);
> -	  new_pr->index->nextPR = old_pr->index->nextPR;
> -	  old_pr->index->nextPR = NULL;
> -	  (*currptr) = new_pr;
> -	  return old_pr;
> -	}
> -      currptr = &((*currptr)->index->nextPR);
> +      /* the pr to be replaced is not at the end of the pr chain */
> +      setPrevPR (curr_pr->index->nextPR, pr_copy);
>      }
> -  return NULL;
> +
> +  free_pr (pr_replace);
> +
> +  return 1;
>  }
>  
>  /* Remove PR PRNUM from the index for the current database, and rewrite the
> @@ -1196,6 +1224,24 @@ getNextPR (PR *pr)
>    return pr->index->nextPR;
>  }
>  
> +void
> +setNextPR (PR *pr, PR *next_pr)
> +{
> +  pr->index->nextPR = next_pr;
> +}
> +
> +PR *
> +getPrevPR (PR *pr)
> +{
> +  return pr->index->prevPR;
> +}
> +
> +void
> +setPrevPR (PR *pr, PR *prev_pr)
> +{
> +  pr->index->prevPR = prev_pr;
> +}
> +
>  int
>  indexIsBinary (const DatabaseInfo database)
>  {
> Index: index.h
> ===================================================================
> RCS file: /cvsroot/gnats/gnats/gnats/index.h,v
> retrieving revision 1.17
> diff -u -p -r1.17 index.h
> --- index.h	15 Apr 2001 18:11:51 -0000	1.17
> +++ index.h	21 Feb 2005 12:19:40 -0000
> @@ -24,7 +24,10 @@ extern int checkPRChain (const DatabaseI
>  
>  extern PR *getFirstPR (const DatabaseInfo database, ErrorDesc *err);
>  
> +extern PR *getPrevPR (PR *pr);
>  extern PR *getNextPR (PR *pr);
> +extern void setPrevPR (PR *pr, PR *prev_pr);
> +extern void setNextPR (PR *pr, PR *next_pr);
>  
>  extern void freePRIndex (PR *pr);
>  
> @@ -46,7 +49,7 @@ extern int addToIndex (PR *pr, ErrorDesc
>  extern char *createIndexEntry (PR *pr, size_t *entLen);
>  extern char *createIndexEntryBinary (PR *pr, size_t *entLen);
>  
> -extern PR *replaceCurrentPRInIndex (PR *newPR);
> +extern int replaceCurrentPRInIndex (PR *curr_pr, PR *newPR, ErrorDesc *err);
>  
>  extern char *indexValue (PR *pr, FieldIndex field);
>  
> Index: mail.c
> ===================================================================
> RCS file: /cvsroot/gnats/gnats/gnats/mail.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 mail.c
> --- mail.c	30 Nov 2004 15:43:50 -0000	1.22
> +++ mail.c	21 Feb 2005 12:19:40 -0000
> @@ -65,9 +65,13 @@ get_responsible_address (const DatabaseI
>       passwd file.  */
>    if (res != NULL)
>      {
> -      if (res->admFields[ResponsibleAdmAlias] == '\0')
> +      if (res->admFields[ResponsibleAdmAlias] == NULL ||
> +	  res->admFields[ResponsibleAdmAlias] == '\0')
>  	{
> -	  free (res->admFields[ResponsibleAdmAlias]);
> +	  if (res->admFields[ResponsibleAdmAlias] != NULL)
> +	    {
> +	      free (res->admFields[ResponsibleAdmAlias]);
> +	    }
>  	  res->admFields[ResponsibleAdmAlias] 
>  	    = xstrdup (res->admFields[ResponsibleAdmKey]);
>  	}
> Index: pr.c
> ===================================================================
> RCS file: /cvsroot/gnats/gnats/gnats/pr.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 pr.c
> --- pr.c	25 Nov 2002 13:58:33 -0000	1.64
> +++ pr.c	21 Feb 2005 12:19:40 -0000
> @@ -154,7 +154,6 @@ allocPR (const DatabaseInfo database)
>  }
>  
>  /* Get the PR at PATH.  PR is the PR entry to be filled in.
> -
>     If PRUNE is non-zero, don't read any multitext fields.  */
>  static int
>  get_pr (PR *pr, const char *path, int prune)
> @@ -211,6 +210,33 @@ fillInPR (PR *pr, ErrorDesc *err)
>      }
>  }
>  
> +static PR *
> +get_pr_from_index (const DatabaseInfo database, const char *prnum,
> +		   ErrorDesc *err)
> +{
> +  PR *pr = getFirstPR (database, err);
> +
> +  /* If they gave it to us with the category, remove it. */
> +  if (( strrchr (prnum, '/')) != NULL)
> +    {
> +      prnum = strrchr (prnum, '/') + 1;
> +    }
> +
> +  while (pr != NULL && strcmp (prnum, field_value (pr, NUMBER (database))) != 0)
> +    {
> +      pr = getNextPR (pr);
> +    }
> +
> +  if (pr == NULL)
> +    {
> +      setError (err, CODE_NONEXISTENT_PR,
> +	        "No PR %s listed in the index.", prnum);
> +      return NULL;
> +    }
> +
> +  return pr;
> +}
> +
>  /* Initializes PR for reading.  Each line of the PR should be passed
>     in via addLineToPR (). */
>  
> @@ -1348,29 +1374,16 @@ free_pr (PR *pr)
>  }
>  
>  static char *
> -get_pr_path (const DatabaseInfo database,const char *prnum, ErrorDesc *err)
> +get_pr_path (const DatabaseInfo database, PR *pr, const char *prnum,
> +	    ErrorDesc *err)
>  {
>    char *path = NULL;
>    const char *category = NULL;
> -  PR *curr_pr_chain = getFirstPR (database, err);
>    char *categoryFromIndex = NULL;
>  
> -  if (curr_pr_chain != NULL)
> +  if (pr != NULL)
>      {
> -      PR *j;
> -      for (j = curr_pr_chain ; j != NULL ; j = getNextPR (j))
> -	{
> -	  if (strcmp (prnum, field_value (j, NUMBER (database))) == 0)
> -	    {
> -	      category = field_value (j, CATEGORY (database));
> -	      break;
> -	    }
> -	}
> -      if (category == NULL)
> -	{
> -	  setError (err, CODE_NONEXISTENT_PR,
> -		    "No PR %s listed in the index.", prnum);
> -	}
> +      category = field_value (pr, CATEGORY (database));
>      }
>    else
>      {
> @@ -1391,9 +1404,10 @@ get_pr_path (const DatabaseInfo database
>  }
>  
>  int
> -prExists (const DatabaseInfo database, const char *prID, ErrorDesc *err)
> +prExists (const DatabaseInfo database, const char *prnum, ErrorDesc *err)
>  {
> -  char *path = get_pr_path (database, prID, err);
> +  PR *pr = get_pr_from_index (database, prnum, err);
> +  char *path = get_pr_path (database, pr, prnum, err);
>    int res = 0;
>    if (path != NULL)
>      {
> @@ -1407,48 +1421,85 @@ prExists (const DatabaseInfo database, c
>    return res;
>  }
>  
> -char *
> -lookup_pr_path (const DatabaseInfo database, const char *prnum, ErrorDesc *err)
> +static bool
> +pr_file_readable (const char *path, ErrorDesc *err)
>  {
> -  char *path;
>    FILE *fp;
>  
> -  /* If they gave it to us with the category, remove it. */
> -  if (( strrchr (prnum, '/')) != NULL)
> +  if (path == NULL)
>      {
> -      prnum = strrchr (prnum, '/') + 1;
> +      return FALSE;
>      }
> -  path = get_pr_path (database, prnum, err);
> -  if ((path == NULL) || ((fp = fopen (path, "r")) == NULL))
> +
> +  if ((fp = fopen (path, "r")) == NULL)
>      {
> -      if (path != NULL)
> -	{
> -	  setError (err, CODE_FILE_ERROR, "Can't open file `%s'", path);
> -	}
> -      return NULL;
> +      setError (err, CODE_FILE_ERROR, "Can't open file `%s'", path);
> +      return FALSE;
>      }
>  
>    fclose (fp);
> -  return path;
> +  return TRUE;
>  }
>  
>  PR *
> -readPRWithNum (const DatabaseInfo database, const char *prID,
> +readPRWithNum (const DatabaseInfo database, const char *prnum,
>  	       int prune, ErrorDesc *err)
>  {
>    PR *pr = NULL;
> -  char *path = lookup_pr_path (database, prID, err);
> +  PR *index_pr = get_pr_from_index (database, prnum, err);
> +  char *path = get_pr_path (database, index_pr, prnum, err);
>  
>    if (path != NULL)
>      {
> -      pr = allocPR (database);
> -      if (get_pr (pr, path, prune) == 0)
> +      if (pr_file_readable (path, err))
> +        {
> +	  pr = allocPR (database);
> +	  setPrevPR (pr, getPrevPR (index_pr));
> +	  setNextPR (pr, getNextPR (index_pr));
> +	  if (get_pr (pr, path, prune) == 0)
> +	    {
> +	      free_pr (pr);
> +	      pr = NULL;
> +	    }
> +        }
> +      free (path);
> +    }
> +  return pr;
> +}
> +
> +int
> +pr_delete (const DatabaseInfo database, const char *prnum, ErrorDesc *err)
> +{
> +  PR *pr;
> +  char *path = NULL;
> +
> +  pr = get_pr_from_index (database, prnum, err);
> +  path = get_pr_path (database, pr, prnum, err);
> +
> +  if (path == NULL || !pr_file_readable (path, err))
> +    {
> +      if (path != NULL)
>  	{
> -	  free_pr (pr);
> -	  pr = NULL;
> +	  free (path);
>  	}
> +      return -1;
>      }
> -  return pr;
> +
> +  if (removePRFromIndex (database, prnum, err))
> +    {
> +      free (path);
> +      return -5;
> +    }
> +  
> +  if (unlink (path))
> +    {
> +      setError (err, CODE_FILE_ERROR, "Unable to unlink file %s\n", path);
> +      free (path);
> +      return -6;
> +    }
> +
> +  free (path);
> +  return 1;
>  }
>  
>  void
> Index: pr.h
> ===================================================================
> RCS file: /cvsroot/gnats/gnats/gnats/pr.h,v
> retrieving revision 1.38
> diff -u -p -r1.38 pr.h
> --- pr.h	15 Apr 2001 18:11:51 -0000	1.38
> +++ pr.h	21 Feb 2005 12:19:40 -0000
> @@ -131,8 +131,8 @@ extern int fconfigParse (DatabaseInfo da
>  extern int createPrFile (PR *pr, const char *filename, int force,
>  			 ErrorDesc *err);
>  
> -extern char *lookup_pr_path (const DatabaseInfo database,
> -			     const char *prnum, ErrorDesc *);
> +extern int pr_delete (const DatabaseInfo database, const char *prnum,
> +		      ErrorDesc *err);
>  
>  extern int prExists (const DatabaseInfo database, const char *prID, 
>  		     ErrorDesc *err);
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Help-gnats mailing list
> Help-gnats@gnu.org
> http://lists.gnu.org/mailman/listinfo/help-gnats

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (Darwin)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD4DBQFCHCyDNF74HmYqaSERArPiAJYwb/hMKgGhjwqBZHmJ+8IRs8/iAJ48X77T
I5u/r6gE77UTGDOFsarTsg==
=6hH6
-----END PGP SIGNATURE-----


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://lists.gnu.org/mailman/listinfo/help-gnats

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: patch #1 - towards a generic backend datastore
  2005-02-23  7:24 ` Mel Hatzis
@ 2005-02-24  0:10   ` Chad Walstrom
  2005-02-24  7:06     ` Mel Hatzis
  0 siblings, 1 reply; 8+ messages in thread
From: Chad Walstrom @ 2005-02-24  0:10 UTC (permalink / raw)
  To: help-gnats


[-- Attachment #1.1: Type: text/plain, Size: 918 bytes --]

Mel Hatzis wrote:
> No one has responded to my patch thus far. I plan on committing the
> changes Thursday evening (PST) provided no one speaks up in the
> meantime.

I haven't had a chance to look it over in detail, yet.  We haven't
released 4.1, either, though I'm hoping that my patches and Mike's will
be the last.  Now is the time to decide whether or not to include your
patches in 4.1 or wait until 4.2.

If we want to wait, I'll get Mike's and my patches in tonight and branch
off the 4.1 tree from the trunk, making space for you to work on the 4.2
release.  I'll add clean-up touches to the 4.1 tree for release (if
there are any), and cherry pick any necessary changes back to trunk.

If we want to include it in 4.1, then go ahead and apply to the trunk.

-- 
Chad Walstrom <chewie@wookimus.net>           http://www.wookimus.net/
           assert(expired(knowledge)); /* core dump */

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 140 bytes --]

_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://lists.gnu.org/mailman/listinfo/help-gnats

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: patch #1 - towards a generic backend datastore
  2005-02-24  0:10   ` Chad Walstrom
@ 2005-02-24  7:06     ` Mel Hatzis
  2005-02-24  8:06       ` Mike M. Volokhov
  0 siblings, 1 reply; 8+ messages in thread
From: Mel Hatzis @ 2005-02-24  7:06 UTC (permalink / raw)
  To: Chad Walstrom; +Cc: help-gnats

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Chad Walstrom wrote:
> Mel Hatzis wrote:
> 
>>No one has responded to my patch thus far. I plan on committing the
>>changes Thursday evening (PST) provided no one speaks up in the
>>meantime.
> 
> 
> I haven't had a chance to look it over in detail, yet.  We haven't
> released 4.1, either, though I'm hoping that my patches and Mike's will
> be the last.  Now is the time to decide whether or not to include your
> patches in 4.1 or wait until 4.2.

There's no harm in applying this patch to 4.1...the changes do not
involve any functional deviations. In fact, there are a few good bug
fixes in the patch, not to mention some performance improvements.

> 
> If we want to wait, I'll get Mike's and my patches in tonight and branch
> off the 4.1 tree from the trunk, making space for you to work on the 4.2
> release.  I'll add clean-up touches to the 4.1 tree for release (if
> there are any), and cherry pick any necessary changes back to trunk.
> 
> If we want to include it in 4.1, then go ahead and apply to the trunk.
> 

OK, I'll go ahead and commit to the mainline tomorrow morning (PST).
Note that I ran the changes through a fairly comprehensive set of
regression tests (which I put together) and they appear to be sound.


I'll have the next patch out for review soon.

- -Mel
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (Darwin)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFCHXkoNF74HmYqaSERAlC+AJkBY8yUz9T6Bvxoakr9PnxLrrODdQCfb7u+
mU01ZNH4cnsUj4WQd8zu0Z8=
=fn4d
-----END PGP SIGNATURE-----


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://lists.gnu.org/mailman/listinfo/help-gnats

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: patch #1 - towards a generic backend datastore
  2005-02-24  7:06     ` Mel Hatzis
@ 2005-02-24  8:06       ` Mike M. Volokhov
  2005-02-24  8:53         ` Mel Hatzis
  0 siblings, 1 reply; 8+ messages in thread
From: Mike M. Volokhov @ 2005-02-24  8:06 UTC (permalink / raw)
  To: Mel Hatzis; +Cc: chewie, help-gnats

On Wed, 23 Feb 2005 22:50:16 -0800
Mel Hatzis <mel@wattes.org> wrote:

[snip]
> OK, I'll go ahead and commit to the mainline tomorrow morning (PST).
> Note that I ran the changes through a fairly comprehensive set of
> regression tests (which I put together) and they appear to be sound.

Wow. I'd love to see such tests in tree so as when huge changes come,
it's sometimes bit difficult to found underwater stones. Many thanks!

--
Mishka.


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://lists.gnu.org/mailman/listinfo/help-gnats

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: patch #1 - towards a generic backend datastore
  2005-02-24  8:06       ` Mike M. Volokhov
@ 2005-02-24  8:53         ` Mel Hatzis
  2005-02-24 11:15           ` Mark D. Baushke
  2005-02-24 11:35           ` Mike M. Volokhov
  0 siblings, 2 replies; 8+ messages in thread
From: Mel Hatzis @ 2005-02-24  8:53 UTC (permalink / raw)
  To: Mike M. Volokhov; +Cc: chewie, help-gnats

Mike M. Volokhov wrote:
> On Wed, 23 Feb 2005 22:50:16 -0800
> Mel Hatzis <mel@wattes.org> wrote:
> 
> [snip]
> 
>>OK, I'll go ahead and commit to the mainline tomorrow morning (PST).
>>Note that I ran the changes through a fairly comprehensive set of
>>regression tests (which I put together) and they appear to be sound.
> 
> 
> Wow. I'd love to see such tests in tree so as when huge changes come,
> it's sometimes bit difficult to found underwater stones. Many thanks!

If there's general interest in incorporating the test suite I have,
I'd be happy to try and set something up.

There's a perl component in it though...for setting up a test database
against which the regression tests are run. I'm not sure how people
feel about adding a perl dependency to the mix.

My original idea was to have the tests run via a 'make test, but this
was not possible since GNATS needs to be (partly) installed in order
to run...the binaries all have a hard-wired reference to the 'databases'
file. We could fairly easily overcome this by modifying the binaries
to allow the databases file to be supplied...either by environment
variable or command line option (or both).

The perl requirement could perhaps, be resolved, by simply committing
the generated database to the mix.

--
Mel Hatzis


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://lists.gnu.org/mailman/listinfo/help-gnats

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: patch #1 - towards a generic backend datastore
  2005-02-24  8:53         ` Mel Hatzis
@ 2005-02-24 11:15           ` Mark D. Baushke
  2005-02-24 11:35           ` Mike M. Volokhov
  1 sibling, 0 replies; 8+ messages in thread
From: Mark D. Baushke @ 2005-02-24 11:15 UTC (permalink / raw)
  To: Mel Hatzis; +Cc: chewie, help-gnats

Mel Hatzis <mel@wattes.org> writes:

> Mike M. Volokhov wrote:
> > On Wed, 23 Feb 2005 22:50:16 -0800
> > Mel Hatzis <mel@wattes.org> wrote:
> > 
> > [snip]
> > 
> >>OK, I'll go ahead and commit to the mainline tomorrow morning (PST).
> >>Note that I ran the changes through a fairly comprehensive set of
> >>regression tests (which I put together) and they appear to be sound.
> > 
> > 
> > Wow. I'd love to see such tests in tree so as when huge changes come,
> > it's sometimes bit difficult to found underwater stones. Many thanks!
> 
> If there's general interest in incorporating the test suite I have,
> I'd be happy to try and set something up.
> 
> There's a perl component in it though...for setting up a test database
> against which the regression tests are run. I'm not sure how people
> feel about adding a perl dependency to the mix.
> 
> My original idea was to have the tests run via a 'make test, but this
> was not possible since GNATS needs to be (partly) installed in order
> to run...the binaries all have a hard-wired reference to the 'databases'
> file. We could fairly easily overcome this by modifying the binaries
> to allow the databases file to be supplied...either by environment
> variable or command line option (or both).
> 
> The perl requirement could perhaps, be resolved, by simply committing
> the generated database to the mix.

Other GNU projects have been known to have dependencies on external
tools such as Tcl,Tk,expect or even some PERL code. The key is to ensure
that the test suite is separable from the software distribution. For
example, see how gcc is available as gcc-core-<rev>.tar.bz2 and
gcc-testsuite-<rev>.tar.bz2

	-- Mark



_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://lists.gnu.org/mailman/listinfo/help-gnats

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: patch #1 - towards a generic backend datastore
  2005-02-24  8:53         ` Mel Hatzis
  2005-02-24 11:15           ` Mark D. Baushke
@ 2005-02-24 11:35           ` Mike M. Volokhov
  1 sibling, 0 replies; 8+ messages in thread
From: Mike M. Volokhov @ 2005-02-24 11:35 UTC (permalink / raw)
  To: Mel Hatzis; +Cc: chewie, help-gnats

On Thu, 24 Feb 2005 00:38:03 -0800
Mel Hatzis <mel@wattes.org> wrote:

> Mike M. Volokhov wrote:
> > On Wed, 23 Feb 2005 22:50:16 -0800
> > Mel Hatzis <mel@wattes.org> wrote:
> > 
> > [snip]
> > 
> >>OK, I'll go ahead and commit to the mainline tomorrow morning (PST).
> >>Note that I ran the changes through a fairly comprehensive set of
> >>regression tests (which I put together) and they appear to be sound.
> > 
> > 
> > Wow. I'd love to see such tests in tree so as when huge changes come,
> > it's sometimes bit difficult to found underwater stones. Many thanks!
> 
> If there's general interest in incorporating the test suite I have,
> I'd be happy to try and set something up.
> 
> There's a perl component in it though...for setting up a test database
> against which the regression tests are run. I'm not sure how people
> feel about adding a perl dependency to the mix.

These checks are needed to developers or beta-users only, when other
users ususally will not ran any regression tests at all. Thus IMHO it
can be implemented in any covenient way.

> My original idea was to have the tests run via a 'make test, but this
> was not possible since GNATS needs to be (partly) installed in order
> to run...the binaries all have a hard-wired reference to the 'databases'
> file. We could fairly easily overcome this by modifying the binaries
> to allow the databases file to be supplied...either by environment
> variable or command line option (or both).

Hmm... We also may create a database named, say "testdb", and encourage
testers add the following line to databases file:

	testdb:GNATS Test Database:/path/to/testdb

But we must use a new executables, which can use any path to databases,
specified at compilation time. 

I'm pesonally have my own copy of the GNATS installation and test
database by specifying --prefix=/usr/home/mishka/gnats at configure
stage. For regression tests I believe there are no needs of running
gnatsd via network.

> The perl requirement could perhaps, be resolved, by simply committing
> the generated database to the mix.

Well, why not commit both (if database is not huge)? The database
creation may be another important part of test itself. Moreover, testers
can download reference database as separate tarball.

--
Mishka.


_______________________________________________
Help-gnats mailing list
Help-gnats@gnu.org
http://lists.gnu.org/mailman/listinfo/help-gnats

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2005-02-24 11:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-02-21 12:41 patch #1 - towards a generic backend datastore Mel Hatzis
2005-02-23  7:24 ` Mel Hatzis
2005-02-24  0:10   ` Chad Walstrom
2005-02-24  7:06     ` Mel Hatzis
2005-02-24  8:06       ` Mike M. Volokhov
2005-02-24  8:53         ` Mel Hatzis
2005-02-24 11:15           ` Mark D. Baushke
2005-02-24 11:35           ` Mike M. Volokhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).