Hi
The following thing is a cleanup-patch for issue #604, the fixes I was
referring to as a cleanup commit last week. I'm posting it here now before
commit for viewing and commenting, if anyone would feel like it.
This patch adds proper return code checks all over, it adds proper return
codes all over (using the previously discussed and shown new client error
code include file "client_errors.h" meant to live in the cmdline directory).
I think I've taken most of the comments into account. I may have forgotten
something or I may just have missed something.
As a patch, it isn't extremely readable as I moved around parts of the code
to smaller functions etc.
I've made a minor effort to make the message_from_editor() function into one
that could possibly become a more generic function for $EDITOR usage in the
future. It will still need adjusting before that, but is slightly more
prepared now.
The actual running of the editor is *still* using system() with a malloc()ed
string, but there's a huge FIXME comment saying that this is a temporary
work-around since svn_io_run_cmd() doesn't currently do the things we need it
to do in order for this to work. I've left a #if 0'ed section that shows how
I think it _should_ (almost) work.
This code does no further effort to translate newlines than before, and thus
it'll still fail on win32 as previously reported by Brane. I don't know just
yet how to deal with this matter.
Index: ./commit-cmd.c
===================================================================
--- ./commit-cmd.c
+++ ./commit-cmd.c Wed Jan 30 23:57:11 2002
@@ -35,23 +35,32 @@
#include "svn_sorts.h"
#include "cl.h"
+#include "client_errors.h"
+
+
/*** Code. ***/
/*
* Prints a single status line to a given file about the given entry.
+ * Return failure, that means TRUE if something went wrong.
*/
-static void
-print_short_format (apr_file_t *file,
- const char *path,
- svn_wc_status_t *status)
+static int
+print_single_file_status (apr_file_t *file,
+ const char *path,
+ svn_wc_status_t *status)
{
char str_status[5];
- char array[128];
+ char array[256]; /* the larger this is, the longer path names we allow in
+ the commit message editor file */
apr_size_t size;
+ apr_size_t written;
+ int failure = FALSE;
+ apr_status_t rc;
if (! status)
- return;
+ /* this shouldn't happen */
+ return TRUE;
/* Create local-mod status code block. */
if (status->text_status != svn_wc_status_unversioned)
@@ -65,26 +74,36 @@
status->locked,
status->copied);
- apr_snprintf(array, 128, "SVN: %s %s\n", str_status, path);
+ apr_snprintf (array, sizeof (array),
+ "SVN: %s %s\n", str_status, path);
+
+ size = strlen (array);
- size = strlen(array);
+ rc = apr_file_write_full (file, array, size, &written);
- apr_file_write(file, array, &size);
+ if ((size != written) || (! APR_STATUS_IS_SUCCESS (rc)))
+ {
+ /* we didn't write the complete line, return an error code */
+ failure = TRUE;
+ }
}
+
+ return failure;
}
/*
* Walks throught the 'nelts' of the given hash and calls the status-
* print function for each.
*/
-void static
-print_status (apr_file_t *file,
- apr_hash_t *statushash,
- apr_pool_t *pool)
+static int
+print_hash_status (apr_file_t *file,
+ apr_hash_t *statushash,
+ apr_pool_t *pool)
{
int i;
apr_array_header_t *statusarray;
svn_wc_status_t *status = NULL;
+ int failure=FALSE;
/* Convert the unordered hash to an ordered, sorted array */
statusarray = apr_hash_sorted_keys (statushash,
@@ -96,11 +115,17 @@
{
const svn_item_t *item;
- item = &APR_ARRAY_IDX(statusarray, i, const svn_item_t);
+ item = &APR_ARRAY_IDX (statusarray, i, const svn_item_t);
status = item->value;
- print_short_format (file, item->key, status);
+ if (print_single_file_status (file, item->key, status))
+ {
+ failure = TRUE;
+ break;
+ }
}
+
+ return failure;
}
/*
@@ -108,11 +133,11 @@
* possibly edited in your favourite $EDITOR.
*/
static svn_error_t *
-write_status_to_file(apr_pool_t *pool,
- apr_file_t *file,
- svn_client_auth_baton_t *auth_baton,
- svn_cl__opt_state_t *opt_state,
- apr_array_header_t *targets)
+write_status_to_file (apr_pool_t *pool,
+ apr_file_t *file,
+ svn_client_auth_baton_t *auth_baton,
+ svn_cl__opt_state_t *opt_state,
+ apr_array_header_t *targets)
{
apr_hash_t *statushash;
svn_revnum_t youngest = SVN_INVALID_REVNUM;
@@ -129,43 +154,138 @@
switches (-n, -u, -[vV]) : */
SVN_ERR (svn_client_status (&statushash, &youngest, target, auth_baton,
- opt_state->nonrecursive ? 0 : 1,
+ opt_state->nonrecursive ? FALSE : TRUE,
opt_state->verbose,
FALSE, /* no update */
pool));
- /* Now print the structures to the screen.
- The flag we pass indicates whether to use the 'detailed'
- output format or not. */
- print_status (file,
- statushash,
- pool);
+ /* Now print the structures to a file. */
+ if (print_hash_status (file, statushash, pool))
+ {
+ return svn_error_create
+ (SVN_ERR_CMDLINE__FAILED_WRITING_TO_TEMPORARY_FILE, 0, NULL,
+ pool,
+ "Failed to write status information to temporary file: %s");
+ }
}
return SVN_NO_ERROR;
}
+static svn_stringbuf_t *
+strip_svn_from_buffer (svn_stringbuf_t *buffer,
+ apr_pool_t *pool)
+{
+ char *ptr;
+ char *prefix;
+
+ /* cut off all the SVN: lines, but nothing else */
+ ptr = buffer->data;
+
+ do
+ {
+ prefix=strstr (ptr, "SVN:");
+
+ if (prefix)
+ {
+ /* substring found */
+
+ if ( (prefix == buffer->data) ||
+ ((prefix[-1] == '\n') || (prefix[-1] == '\r')))
+ {
+ /* it is on the start of a line */
+
+ /* Figure out the end of the line. This needs a
+ little better intelligence since a LF is not the
+ end-of-line on every imaginable system .*/
+ char *eol= strchr (prefix, '\n');
+ size_t linelen;
+
+ if(NULL == eol)
+ {
+ /* last line, we just make the buffer shorter,
+ no need to move around any data */
+ linelen = strlen (prefix);
+
+ buffer->len -= linelen;
+
+ /* set a new zero terminator */
+ buffer->data [ buffer->len] = 0;
+
+ break;
+ }
+
+ eol++; /* eol now points to the first character
+ beyond */
+
+ /* this line that is about to get cut off measures
+ from 'prefix' to 'eol' */
+ linelen = eol-prefix;
+
+ /* move the rest of the chunk over this line that
+ shouldn't be a part of the final message */
+ memmove (prefix, eol,
+ buffer->len -
+ (prefix-buffer->data) -
+ linelen);
+
+ /* decrease total message size */
+ buffer->len -= linelen;
+
+ /* set a new zero terminator */
+ buffer->data [ buffer->len] = 0;
+
+ /* continue searching from here */
+ ptr = prefix;
+ }
+ else
+ {
+ /* substring found but not on the first column of
+ a line, just continue from here */
+ ptr = prefix+1;
+ }
+ }
+ }
+ while (prefix);
+
+ return buffer;
+}
+
/*
* Invoke $EDITOR to get a commit message.
*/
static svn_error_t *
-message_from_editor(apr_pool_t *pool,
- apr_array_header_t *targets,
- svn_client_auth_baton_t *auth_baton,
- svn_stringbuf_t *path,
- svn_cl__opt_state_t *opt_state,
- svn_stringbuf_t **messagep )
+message_from_editor (apr_pool_t *pool,
+ apr_array_header_t *targets,
+ svn_client_auth_baton_t *auth_baton,
+ svn_stringbuf_t *path,
+ svn_cl__opt_state_t *opt_state,
+ svn_stringbuf_t **messagep,
+ const char *default_msg, /* text to include in editor */
+ svn_boolean_t include_status_output)
{
char const *editor;
- char *command;
- size_t editorlen;
apr_file_t *tempfile;
apr_status_t rc;
const char *fullfile;
+ apr_finfo_t finfo_before;
+ apr_finfo_t finfo_after;
+ apr_size_t size;
+ apr_size_t written;
+ int exitcode;
+ apr_exit_why_e exitwhy;
+ svn_error_t *error;
+ const char *cmdargs[3];
/* default is no returned message */
*messagep = NULL;
+ /* ### FIXME: editor-choice:
+ 1. the command line
+ 2. config file.
+ 3. environment variable
+ 4. configure-default */
+
/* try to get an editor to use */
editor = getenv ("SVN_EDITOR");
if(NULL == editor)
@@ -176,6 +296,7 @@
if(NULL == editor)
{
/* no custom editor, use built-in defaults */
+ /* ### FIXME: the path should be discovered in configure */
#ifdef SVN_WIN32
editor = "notepad.exe";
#else
@@ -190,208 +311,169 @@
pool));
/* we need to know the name of the temporary file */
- apr_file_name_get (&fullfile, tempfile);
+ rc = apr_file_name_get (&fullfile, tempfile);
+ if (APR_SUCCESS != rc)
+ {
+ /* close temp file first, but we can't remove it since we can't get
+ its name! */
+ apr_file_close (tempfile);
- editorlen = strlen (editor);
+ /* could not get name we can't continue */
+ return svn_error_create
+ (SVN_ERR_CMDLINE__FAILED_WRITING_TO_TEMPORARY_FILE, 0, NULL,
+ pool,
+ "Failed to write status information to temporary file.");
+ }
- command = (char *)malloc (editorlen + strlen(fullfile) + 2);
+ size = strlen (default_msg);
+ rc = apr_file_write_full (tempfile, default_msg, size, &written);
+
+ if ((APR_SUCCESS != rc) || written != size)
+ {
+ error = svn_error_create
+ (SVN_ERR_CMDLINE__FAILED_WRITING_TO_TEMPORARY_FILE, 0, NULL,
+ pool,
+ "Failed to write stauts information to temporary file.");
+ }
+ else if (include_status_output)
+ {
+ error = write_status_to_file (pool, tempfile, auth_baton,
+ opt_state, targets);
+ }
-#define DEFAULT_MSG \
-"\nSVN: ----------------------------------------------------------------------\n" \
-"SVN: Enter Log. Lines beginning with 'SVN:' are removed automatically\n" \
-"SVN: \n" \
-"SVN: Current status of the target files and directories:\n"\
-"SVN: \n"
+ apr_file_close (tempfile); /* we don't check return code here, since
+ we have no way to deal with errors on
+ file close */
+
+ if (error)
+ {
+ /* we didn't manage to write the complete file, we can't fulfill
+ what we're set out to do, get out */
+
+ return error;
+ }
- if (command)
+ /* Get information about the temporary file before the user has
+ been allowed to edit any message */
+ rc = apr_stat (&finfo_before, fullfile,
+ APR_FINFO_MTIME|APR_FINFO_SIZE, pool);
+
+ if (APR_SUCCESS != rc)
{
- apr_finfo_t finfo_before;
- apr_finfo_t finfo_after;
- apr_size_t size, written;
+ /* remove file */
+ apr_file_remove (fullfile, pool);
- size = strlen (DEFAULT_MSG);
+ return svn_error_create
+ (SVN_ERR_CMDLINE__FAILED_STAT_ON_TEMPORARY_FILE, 0, NULL,
+ pool,
+ "Failed getting info about temporary file.");
+ }
- rc = apr_file_write_full (tempfile, DEFAULT_MSG, size, &written);
+#if 0
+ /* This is what I think is almost proper code to run the editor command
+ line. We should somehow tell the svn_io_run_cmd() that we want it
+ to search the PATH and inherit our environment. */
+
+ /* setup the editor command line */
+ cmdargs[0] = editor;
+ cmdargs[1] = fullfile;
+ cmdargs[2] = NULL;
+
+ fprintf(stderr, "RUN: %s %s\n",
+ editor, fullfile);
+
+ /* run the editor and come back when done */
+ error = svn_io_run_cmd (".",
+ editor,
+ cmdargs,
+ &exitcode,
+ &exitwhy,
+ NULL, NULL, NULL, /* no in, out or err files */
+ pool);
+#else
+ /* ### FIXME:
+ this is just a work-around as the above doesn't seem to work!
+ Remove as soon as possible.
+ */
+
+ {
+ char *command=(char *)malloc(strlen(editor)+strlen(fullfile)+2);
+ sprintf(command, "%s %s", editor, fullfile);
+ system(command);
+ free(command);
+ }
+#endif
- write_status_to_file (pool, tempfile, auth_baton, opt_state, targets);
+ if (error)
+ {
+ /* remove file */
+ apr_file_remove (fullfile, pool);
- apr_file_close (tempfile);
+ return error;
+ }
- /* we didn't manage to write the complete file, we can't fulfill
- what we're set out to do, get out */
- /* ### FIXME: The documentation for apr_file_full_write()
- doesn't explicitly promise that if size != written, then
- there *must* be an error returned, so below we handle the two
- cases separately. But a glance at apr_file_full_write's
- implementation, on Unix at least, shows that it could
- document that promise. Maybe we should fix the doc in APR,
- and just check rc below? */
- if (! APR_STATUS_IS_SUCCESS (rc))
- {
- return svn_error_createf
- (rc, 0, NULL, pool, "Trouble writing `%s'", fullfile);
- }
- else if (written != size)
- {
- /* ### FIXME: this error code may change when there is a
- general need to revamp the client's error code system. */
- return svn_error_createf
- (SVN_ERR_INCOMPLETE_DATA,
- 0, NULL, pool, "Failed to completely write `%s'", fullfile);
- }
+ /* Get information about the message file after the assumed editing. */
+ rc = apr_stat (&finfo_after, fullfile,
+ APR_FINFO_MTIME|APR_FINFO_SIZE, pool);
+
+ if (APR_SUCCESS != rc)
+ {
+ /* remove file */
+ apr_file_remove (fullfile, pool);
- /* Get information about the temporary file before the user has
- been allowed to edit any message */
- apr_stat (&finfo_before, fullfile,
- APR_FINFO_MTIME|APR_FINFO_SIZE, pool);
+ return svn_error_create
+ (SVN_ERR_CMDLINE__FAILED_STAT_ON_TEMPORARY_FILE, 0, NULL,
+ pool,
+ "Failed getting info about temporary file.");
+ }
- /* create the command line */
- apr_snprintf (command, editorlen + strlen(fullfile) + 2,
- "%s %s", editor, fullfile);
- /* run the editor command line */
- system (command);
-
- /* Get information about the message file after the assumed editing. */
- apr_stat (&finfo_after, fullfile,
- APR_FINFO_MTIME|APR_FINFO_SIZE, pool);
-
- /* Check if there seems to be any changes in the file */
- if((finfo_before.mtime == finfo_after.mtime) &&
- (finfo_before.size == finfo_after.size))
+ /* Check if there seems to be any changes in the file */
+ if ((finfo_before.mtime != finfo_after.mtime) ||
+ (finfo_before.size != finfo_after.size))
+ {
+ /* The file seem to have been modified, load it and strip it */
+ apr_file_t *read_file;
+
+ /* we have a commit message in a temporary file, get it */
+ rc = apr_file_open (&read_file, fullfile,
+ APR_READ, APR_UREAD, pool);
+
+ if (APR_SUCCESS != rc) /* open failed */
{
- /* The file doesn't seem to have been modified, no
- need to load it and strip it and such */
+ /* This is an annoying situation, as the file seems to have
+ been edited but we can't read it! */
+
+ /* remove file */
+ apr_file_remove (fullfile, pool);
+
+ return svn_error_create
+ (SVN_ERR_CMDLINE__FAILED_OPENING_TEMPORARY_FILE, 0, NULL,
+ pool,
+ "Failed opening temporary file.");
}
- else {
- apr_file_t *read_file;
+ else
+ {
+ svn_stringbuf_t *entirefile;
- /* we have a commit message in a temporary file, get it */
- rc = apr_file_open (&read_file, fullfile,
- APR_READ, APR_UREAD, pool);
-
- if(APR_SUCCESS != rc) /* open failed */
- {
- /* This is an annoying situation, as the file seems to have
- been edited but we can't read it! */
- }
- else
- {
- /* read the entire file into one chunk of memory */
- char readbuffer[1024];
- svn_stringbuf_t *entirefile;
- char *ptr;
- char *prefix;
-
- /* create a buffer */
- entirefile = svn_stringbuf_ncreate ("", 0, pool);
+ /* read the entire file into one chunk of memory */
+ SVN_ERR (svn_string_from_aprfile (&entirefile, read_file, pool));
- do
- {
- size = sizeof (readbuffer);
- apr_file_read (read_file, readbuffer, &size);
-
- /* append chunk to the entirefile string */
- svn_stringbuf_appendbytes (entirefile, readbuffer, size);
-
- if( size != sizeof (readbuffer))
- {
- /* partly filled buffer, this is the end */
- break; /* out of loop */
- }
- }
- while(1);
-
- /* close the file */
- apr_file_close (read_file);
-
- /* a full chunk was read, now strip all the SVN: lines, but
- nothing else */
- ptr = entirefile->data;
-
- do
- {
- prefix=strstr (ptr, "SVN:");
-
- if(prefix)
- {
- /* substring found */
-
- if( (prefix == entirefile->data) ||
- ((prefix[-1] == '\n') || (prefix[-1] == '\r')))
- {
- /* it is on the start of a line */
-
- /* Figure out the end of the line. This needs a little
- better intelligence since a LF is not the
- end-of-line on every imaginable system .*/
- char *eol= strchr(prefix, '\n');
- size_t linelen;
-
- if(NULL == eol)
- {
- /* last line, we just make the buffer shorter,
- no need to move around any data */
- linelen = strlen(prefix);
-
- entirefile->len -= linelen;
-
- /* set a new zero terminator */
- entirefile->data [ entirefile->len] = 0;
-
- break;
- }
-
- eol++; /* eol now points to the first character
- beyond */
-
- /* this line that is about to get cut off measures
- from 'prefix' to 'eol' */
- linelen = eol-prefix;
-
- /* move the rest of the chunk over this line that
- shouldn't be a part of the final message */
- memmove (prefix, eol,
- entirefile->len - (prefix-entirefile->data) -
- linelen);
-
- /* decrease total message size */
- entirefile->len -= linelen;
-
- /* set a new zero terminator */
- entirefile->data [ entirefile->len] = 0;
-
- /* continue searching from here */
- ptr = prefix;
- }
- else
- {
- /* substring found but not on the first column of
- a line, just continue from here */
- ptr = prefix+1;
- }
- }
- }
- while(prefix);
-
- /* set the return-message to the entire-file buffer */
- *messagep = entirefile;
- }
-
- }
+ /* close the file */
+ apr_file_close (read_file);
- /* free the memory allocated for the command line here */
- free (command);
- }
- else
- {
- /* major memory problem */
+ /* strip SVN: lines */
+ entirefile = strip_svn_from_buffer(entirefile, pool);
+
+ /* set the return-message to the entire-file buffer */
+ *messagep = entirefile;
+ }
+
}
/* remove the temporary file */
apr_file_remove (fullfile, pool);
return SVN_NO_ERROR;
-
}
/*
@@ -399,14 +481,15 @@
* This function also outputs this fact to stdout for users to see.
*/
static svn_error_t *
-store_message(svn_stringbuf_t *message,
- svn_stringbuf_t *path,
- apr_pool_t *pool)
+store_message (svn_stringbuf_t *message,
+ svn_stringbuf_t *path,
+ apr_pool_t *pool)
{
/* Store the message in a temporary file name and display the
file name to the user */
apr_file_t *tempfile;
apr_size_t size;
+ apr_size_t written;
apr_status_t rc;
const char *fullfile;
@@ -416,24 +499,30 @@
pool));
/* we need to know the name of the temporary file */
- apr_file_name_get (&fullfile, tempfile);
+ rc = apr_file_name_get (&fullfile, tempfile);
- size = message->len;
- rc = apr_file_write (tempfile, message->data, &size);
+ if (APR_SUCCESS == rc)
+ {
+ size = message->len;
+ rc = apr_file_write_full (tempfile, message->data, size, &written);
+ }
- apr_file_close(tempfile);
+ apr_file_close (tempfile);
- if (APR_SUCCESS == rc)
+ if ((APR_SUCCESS == rc) && (size == written))
{
- printf("The commit message has been stored in this location:\n%s\n",
- fullfile);
+ printf ("The commit message has been stored in this location:\n%s\n",
+ fullfile);
}
else
{
- /* FIX! return a proper error message here */
+ return svn_error_createf
+ (SVN_ERR_CMDLINE__FAILED_WRITING_TO_TEMPORARY_FILE, 0, NULL,
+ pool,
+ "Failed writing to temporary file %s.", fullfile);
}
- return NULL;
+ return SVN_NO_ERROR;
}
svn_error_t *
@@ -506,18 +595,27 @@
/* There was no commit message given anywhere in the command line,
fire up our favourite editor to get one instead! */
+ /* this default message might need to be configurable somehow */
+ const char *default_msg=
+ "\nSVN: ----------------------------------------------------------------------\n"
+ "SVN: Enter Log. Lines beginning with 'SVN:' are removed automatically\n" \
+ "SVN: \n"
+ "SVN: Current status of the target files and directories:\n"
+ "SVN: \n";
+
/* no message given, try getting one from $EDITOR */
message_from_editor (pool, targets, auth_baton,
- base_dir, opt_state, &messagep);
+ base_dir, opt_state, &messagep,
+ default_msg, TRUE);
- if(messagep)
+ if (messagep)
{
/* We did get message, now check if it is anything more than just
white space as we will consider white space only as empty */
int len;
for (len=messagep->len; len>=0; len--)
- if(!apr_isspace (messagep->data[len]))
+ if (!apr_isspace (messagep->data[len]))
break;
if (len >= 0)
/* there was something else besides space */
@@ -526,7 +624,7 @@
/* message can still be NULL here if none was entered or if an
error occurred */
- if(NULL == message)
+ if (NULL == message)
{
char *reply;
svn_cl__prompt_user (&reply,
--
Daniel Stenberg - http://daniel.haxx.se - +46-705-44 31 77
ech`echo xiun|tr nu oc|sed 'sx\([sx]\)\([xoi]\)xo un\2\1 is xg'`ol
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:37:01 2006