Skip to contentSkip to conversation

git-fetch--tool and "insanely" long actions

PATCH In review A Large Angry SCM 2 participants 4 messages Jun 15, 2016

lore
Share & tools
lore

A Large Angry SCM

author
#0 lore raw

PATCH 1/1 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(-)
builtin-fetch--tool.c +1 -1
index e9d6764..173dd4f 100644
@@ -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;
#1 lore raw
Quoted 5 lines · A Large Angry SCM

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.

.
.
.

Review

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

Or you could just let the whole thing through?

builtin-fetch--tool.c +12 -7
index e9d6764..9b5ae9f 100644
@@ -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,
Signature · 1 line
1.5.1.1
author
#2 lore raw
Quoted 44 lines · Julian Phillips
Quoted 5 lines · A Large Angry SCM

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.

.
.
.

Review

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

Or you could just let the whole thing through?

builtin-fetch--tool.c +8 -5
index e9d6764..9b5ae9f 100644
@@ -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;

builtin-fetch--tool.c static int update_ref(const char *action, · line 61

Quoted 10 lines
builtin-fetch--tool.c +4 -2
 	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,

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

#3 lore raw
Quoted 13 lines · A Large Angry SCM
Quoted 10 lines · Julian Phillips
Quoted 8 lines · A Large Angry SCM

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.

Quoted 2 lines

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.

Signature · 4 lines
Julian

 ---
You will be successful in your work.