[RFD PATCH] git-fetch--tool and "insanely" long actions

This fixes a problem my repository mirroring script has been having since
the git-fetch--tool was added to master in the middle of March. However,
it is not a proper fix since it causes actual errors from snprintf() to be
ignored. A proper fix is complicated by the lack of a consistent indicator
that the buffer is too small across snprintf() implementations.


 builtin-fetch--tool.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index e9d6764..173dd4f 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c

@@ -44,7 +44,7 @@ static int update_ref(const char *action,
 		rla = "(reflog update)";
 	len = snprintf(msg, sizeof(msg), "%s: %s", rla, action);
 	if (sizeof(msg) <= len)
-		die("insanely long action");
+		msg[sizeof(msg)-1] = '\0';
 	lock = lock_any_ref_for_update(refname, oldval);
 	if (!lock)
 		return 1;

On Thu, 19 Apr 2007, A Large Angry SCM wrote:

This fixes a problem my repository mirroring script has been having since
the git-fetch--tool was added to master in the middle of March. However,
it is not a proper fix since it causes actual errors from snprintf() to be
ignored. A proper fix is complicated by the lack of a consistent indicator
that the buffer is too small across snprintf() implementations.

.
.
.

      if (sizeof(msg) <= len)
-             die("insanely long action");
+             msg[sizeof(msg)-1] = '\0';

Or you could just let the whole thing through?

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index e9d6764..9b5ae9f 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c

@@ -36,21 +36,26 @@ static int update_ref(const char *action,
 		      unsigned char *oldval)
 {
 	int len;
-	char msg[1024];
+	char buffer[1024];
+	int ret = 0;
+	char *msg = buffer;
 	char *rla = getenv("GIT_REFLOG_ACTION");
 	static struct ref_lock *lock;
 
 	if (!rla)
 		rla = "(reflog update)";
-	len = snprintf(msg, sizeof(msg), "%s: %s", rla, action);
-	if (sizeof(msg) <= len)
-		die("insanely long action");
+	len = strlen(rla) + strlen(action) + 3;
+	if (len > sizeof(buffer))
+		msg = xmalloc(len);
+	snprintf(msg, len, "%s: %s", rla, action);
 	lock = lock_any_ref_for_update(refname, oldval);
 	if (!lock)
-		return 1;
+		ret = 1;
 	if (write_ref_sha1(lock, sha1, msg) < 0)
-		return 1;
-	return 0;
+		ret = 1;
+	if (msg != buffer)
+		free(msg);
+	return ret;
 }
 
 static int update_local_ref(const char *name,

-- 
1.5.1.1

Julian Phillips wrote:

See the last sentence in my original message. Yours also ignores errors 
from snprintf().

On Thu, 19 Apr 2007, A Large Angry SCM wrote:

Julian Phillips wrote:

quoted

 On Thu, 19 Apr 2007, A Large Angry SCM wrote:

quoted

 This fixes a problem my repository mirroring script has been having 
 since
 the git-fetch--tool was added to master in the middle of March. However,
 it is not a proper fix since it causes actual errors from snprintf() to 
 be
 ignored. A proper fix is complicated by the lack of a consistent 
 indicator
 that the buffer is too small across snprintf() implementations.

See the last sentence in my original message. Yours also ignores errors from 
snprintf().

Well your last sentence was about the buffer being too small.  The change 
I made means it won't ever be too small.  True that it doesn't check for 
other errors.

-- 
Julian

  ---
You will be successful in your work.