[PATCH] refs.c: add a function to append a reflog entry to a fd

[PATCH] refs.c: add a function to append a reflog entry to a fd

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

Break out the code to create the string and writing it to the file
descriptor from log_ref_write and add it into a dedicated function
log_ref_write_fd. For now this is only used from log_ref_write,
but later on we will call this function from reflog transactions too,
which means that we will end up with only a single place,
where we write a reflog entry to a file instead of the current two
places (log_ref_write and builtin/reflog.c).

[sb: replaced sprintf by snprintf for security reasons, 
     which was there in the beginning (6de08ae688b9f2, 2006-06-17, 
     Log ref updates to logs/refs/<ref>), but got lost in (8ac65937d032ad, 
     2007-01-26, Make sure we do not write bogus reflog entries.)
]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)

This is also part of the ref-transaction-reflog series, but is 
sufficiently independant, that I'd argue it's a general code hygiene thing,
we can use in any case.

Applies on master.

Compared to the last send of this patch[1], there was one change in the print
function. Replaced sprintf by snprintf for security reasons. 

[1] part of the ref-transaction-reflog series 
    http://permalink.gmane.org/gmane.comp.version-control.git/259714
 

diff --git a/refs.c b/refs.c
index 5ff457e..f9b42e5 100644
--- a/refs.c
+++ b/refs.c
@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
 	return 0;
 }
 
+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const char *committer, const char *msg)
+{
+	int msglen, written;
+	unsigned maxlen, len;
+	char *logrec;
+
+	msglen = msg ? strlen(msg) : 0;
+	maxlen = strlen(committer) + msglen + 100;
+	logrec = xmalloc(maxlen);
+	len = snprintf(logrec, maxlen, "%s %s %s\n",
+		       sha1_to_hex(old_sha1),
+		       sha1_to_hex(new_sha1),
+		       committer);
+	if (msglen)
+		len += copy_msg(logrec + len - 1, msg) - 1;
+
+	written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
+	free(logrec);
+	if (written != len)
+		return -1;
+
+	return 0;
+}
+
 static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg)
 {
-	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
-	unsigned maxlen, len;
-	int msglen;
+	int logfd, result, oflags = O_APPEND | O_WRONLY;
 	char log_file[PATH_MAX];
-	char *logrec;
-	const char *committer;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 	logfd = open(log_file, oflags);
 	if (logfd < 0)
 		return 0;
-	msglen = msg ? strlen(msg) : 0;
-	committer = git_committer_info(0);
-	maxlen = strlen(committer) + msglen + 100;
-	logrec = xmalloc(maxlen);
-	len = sprintf(logrec, "%s %s %s\n",
-		      sha1_to_hex(old_sha1),
-		      sha1_to_hex(new_sha1),
-		      committer);
-	if (msglen)
-		len += copy_msg(logrec + len - 1, msg) - 1;
-	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
-	free(logrec);
-	if (written != len) {
+	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
+				  git_committer_info(0), msg);
+	if (result) {
 		int save_errno = errno;
 		close(logfd);
 		error("Unable to append to %s", log_file);
-- 
2.2.0.rc2.13.g0786cdb

Re: [PATCH] refs.c: add a function to append a reflog entry to a fd

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Compared to the last send of this patch[1], there was one change in the print
> function. Replaced sprintf by snprintf for security reasons. 

Careful.  I despise people who blindly think strlcpy() and
snprintf() are good solutions for for security.  They are by
themselves not.

Surely, the use of them lets you avoid stomping on pieces of memory
that you did not intend to write to.  I'd call that "protecting
other people's data".

But what about your own data?  If you are thinking that you are
trying to write "this and that", but by mistake (either by your or
by a careless future change) if you did not allocate enough, you
would stop at "this and th".  You may have protected other people's
data, but your result is still broken.  If you gave that to others
without checking that you produced an incomplete piece of data, what
guarantee are you getting that you are not creating a security hole
there (imagine you intended to copy "rm -fr ./tmpdir" and by mistake
you stopped at "rm -fr ./" @~@).

> +	msglen = msg ? strlen(msg) : 0;
> +	maxlen = strlen(committer) + msglen + 100;
> +	logrec = xmalloc(maxlen);
> +	len = snprintf(logrec, maxlen, "%s %s %s\n",
> +		       sha1_to_hex(old_sha1),
> +		       sha1_to_hex(new_sha1),
> +		       committer);
> +	if (msglen)
> +		len += copy_msg(logrec + len - 1, msg) - 1;

In this codepath, you are allocating enough buffer to hold the whole
message; there is no difference between sprintf() and snprintf().
If the difference mattered, you would have chopped the reflog entry
too short, and produced a wrong result, but you then discard the
whole record (the code that follows the above), losing data.

So use of snprintf() is not really buying you much here, not in the
current code certainly, but not as a future-proofing measure,
either.

Re: [PATCH] refs.c: add a function to append a reflog entry to a fd

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> Compared to the last send of this patch[1], there was one change in the print
>> function. Replaced sprintf by snprintf for security reasons. 
>
> Careful.  I despise people who blindly think strlcpy() and
> snprintf() are good solutions for for security.  They are by
> themselves not.
> ...
> So use of snprintf() is not really buying you much here, not in the
> current code certainly, but not as a future-proofing measure,
> either.

Don't get me wrong.  I am not saying that using snprintf() here is
bad per-se.  There should be no difference.

But I do not want to see people getting in the habit of thinking "I
now use snprintf/strlcpy instead of sprintf/strcpy, and made my code
more secure."  Often they are not doing that.

The only case snprintf/strlcpy is useful is when your data does not
matter in its detail.  E.g. when you are preparing human-readble
data whose first part is sufficient to convey the information you
want to convey, you would be perfectly happy if the result is
truncated. In such a case, counting to allocate enough to hold
everything and running sprintf() only to chop the result later is
not necessary --- it still is not wrong, though --- and allocating
enough to satisify the eventual chop length and using snprintf()
is easier way to achieve the same result.

But I do not think this codepath falls into such an "I am willing to
lose data" case.

[PATCH] refs.c: add a function to append a reflog entry to a fd

[Stefan Beller]

Break out the code to create the string and writing it to the file
descriptor from log_ref_write and add it into a dedicated function
log_ref_write_fd. For now this is only used from log_ref_write,
but later on we will call this function from reflog transactions too,
which means that we will end up with only a single place,
where we write a reflog entry to a file instead of the current two
places (log_ref_write and builtin/reflog.c).

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 refs.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)
 
Sorry for the small change w.r.t. sprintf/snprintf
Here comes a resend without changing code, but just making it a new function,
so we come forwards with the patch.


Let's discuss the change and decide if I shouldsend a follow up patch to change 
it into snprintf.

On Wed, Nov 19, 2014 at 5:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> +     logrec = xmalloc(maxlen);
>> +     len = snprintf(logrec, maxlen, "%s %s %s\n",
>> +                    sha1_to_hex(old_sha1),
>> +                    sha1_to_hex(new_sha1),
>> +                    committer);
>> +     if (msglen)
>> +             len += copy_msg(logrec + len - 1, msg) - 1;
>
> In this codepath, you are allocating enough buffer to hold the whole
> message; there is no difference between sprintf() and snprintf().
> If the difference mattered, you would have chopped the reflog entry
> too short, and produced a wrong result, but you then discard the
> whole record (the code that follows the above), losing data.

Hypothetically speaking:

There should be no difference between sprintf() and snprintf(). As soon as you
would have a difference, you'd risk stomping on other peoples data, which could lead
to code insertion, iff the format of the inserted data is long enough overwriting
parts of your stack? Or you could change data to alter the program flow to direct
the program to another easier abusable bug.
Does this make sense? This is about protecting a user from a malicious attacker.

> imagine you intended to copy "rm -fr ./tmpdir" and by mistake
> you stopped at "rm -fr ./" @~@).

Yes, that's when you should check the return code of snprintf and compare to 
strlen("rm -fr ./tmpdir"). That kind of bug also has severe consequences, but
it's not yet involving a malicious attacker, so I'd file this as a safety instead  
of a security flaw.

Safety related bugs are easier to find usually as you don't need to have to
think about what weird things an attacker might do. This kind of bug can be found
directly from reading the code and its surroundings 
("there it should print 'rm -fr ./tmpdir'... Gah! I did not check the return value")


diff --git a/refs.c b/refs.c
index 5ff457e..f9b42e5 100644
--- a/refs.c
+++ b/refs.c
@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
 	return 0;
 }
 
+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const char *committer, const char *msg)
+{
+	int msglen, written;
+	unsigned maxlen, len;
+	char *logrec;
+
+	msglen = msg ? strlen(msg) : 0;
+	maxlen = strlen(committer) + msglen + 100;
+	logrec = xmalloc(maxlen);
+	len = sprintf(logrec, maxlen, "%s %s %s\n",
+		      sha1_to_hex(old_sha1),
+		      sha1_to_hex(new_sha1),
+		      committer);
+	if (msglen)
+		len += copy_msg(logrec + len - 1, msg) - 1;
+
+	written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
+	free(logrec);
+	if (written != len)
+		return -1;
+
+	return 0;
+}
+
 static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg)
 {
-	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
-	unsigned maxlen, len;
-	int msglen;
+	int logfd, result, oflags = O_APPEND | O_WRONLY;
 	char log_file[PATH_MAX];
-	char *logrec;
-	const char *committer;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 	logfd = open(log_file, oflags);
 	if (logfd < 0)
 		return 0;
-	msglen = msg ? strlen(msg) : 0;
-	committer = git_committer_info(0);
-	maxlen = strlen(committer) + msglen + 100;
-	logrec = xmalloc(maxlen);
-	len = sprintf(logrec, "%s %s %s\n",
-		      sha1_to_hex(old_sha1),
-		      sha1_to_hex(new_sha1),
-		      committer);
-	if (msglen)
-		len += copy_msg(logrec + len - 1, msg) - 1;
-	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
-	free(logrec);
-	if (written != len) {
+	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
+				  git_committer_info(0), msg);
+	if (result) {
 		int save_errno = errno;
 		close(logfd);
 		error("Unable to append to %s", log_file);
-- 
2.2.0.rc2.13.g0786cdb

Re: [PATCH] refs.c: add a function to append a reflog entry to a fd

[Jonathan Nieder]

Stefan Beller wrote:

> [Subject: [PATCH] refs.c: add a function to append a reflog entry to a fd]

Does this supersede the other patch with the same subject?

Please keep adding v<num> in the subject --- when it's there, it makes
reading much easier.

>

Missing 'From:' line naming the original patch author.

> Break out the code to create the string and writing it to the file
> descriptor from log_ref_write and add it into a dedicated function
> log_ref_write_fd.

(grammar) I'm having trouble parsing the above.

Is the idea something like "Split out a function that [explanation
of when a caller would use this function goes here]?"

>                   For now this is only used from log_ref_write,
> but later on we will call this function from reflog transactions too,

Useful to know.  I'd end the sentence here, since it seems to run
on with a different thought.

> which means that we will end up with only a single place,
> where we write a reflog entry to a file instead of the current two
> places (log_ref_write and builtin/reflog.c).

Ah, so builtin/reflog.c is doing something similar and will later
be changed to use this code, too?

Overall it sounds like a very good change.

[...]
> Here comes a resend without changing code, but just making it a new function,
> so we come forwards with the patch.

\o/

> Let's discuss the change and decide if I shouldsend a follow up patch to change
> it into snprintf.

Both sprintf and snprintf are error-prone functions.  It would be
lovely in a followup to use strbuf_addf or xstrfmt in this code path.
strbufs are how git deals with bookkeeping for string sizes --- they
are very pleasant.

[...]
> +++ b/refs.c
[...]
> @@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
[...]
> +	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
> +				  git_committer_info(0), msg);
> +	if (result) {
>  		int save_errno = errno;
>  		close(logfd);
>  		error("Unable to append to %s", log_file);

Since 'result' isn't used here, this could be simplified to

	if (log_ref_write_fd(...)) {
		...
	}

Thanks and hope that helps,
Jonathan

Re: [PATCH] refs.c: add a function to append a reflog entry to a fd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> Break out the code to create the string and writing it to the file
>> descriptor from log_ref_write and add it into a dedicated function
>> log_ref_write_fd.
>
> (grammar) I'm having trouble parsing the above.

Yeah, I can see what it wants to say, but still...

>> Let's discuss the change and decide if I shouldsend a follow up patch to change
>> it into snprintf.
>
> Both sprintf and snprintf are error-prone functions.  It would be
> lovely in a followup to use strbuf_addf or xstrfmt in this code path.
> strbufs are how git deals with bookkeeping for string sizes --- they
> are very pleasant.

Yeah, that solves both sides of not stomping on other peoples' data
(which may lead to replaced return address and such) and not giving
broken result (which may cause trouble to the callers) equally well
without raising a stink about "security!!!", which is good ;-).

>> @@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname,
>> const unsigned char *old_sha1,
> [...]
>> +	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
>> +				  git_committer_info(0), msg);
>> +	if (result) {
>>  		int save_errno = errno;
>>  		close(logfd);
>>  		error("Unable to append to %s", log_file);
>
> Since 'result' isn't used here, this could be simplified to
>
> 	if (log_ref_write_fd(...)) {
> 		...
> 	}

Yeah that is a good simplification.

[PATCH v3] refs.c: add a function to append a reflog entry to a fd

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

Move code to create the string for a ref and write it to a file descriptor
from log_ref_write and add it into a new dedicated function
log_ref_write_fd.

For now the new function is only used from log_ref_write, but later
on we will call this function from reflog transactions too. That means
that we will end up with only a single place, where we write a
reflog entry to a file instead of the current two places
(log_ref_write and builtin/reflog.c).

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

Changes in version 3:
* reword the commit message to make it more understandable.
* no changes in code
* wait for the follow up to address any changes in the code.

 refs.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index 5ff457e..9948841 100644
--- a/refs.c
+++ b/refs.c
@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
 	return 0;
 }
 
+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const char *committer, const char *msg)
+{
+	int msglen, written;
+	unsigned maxlen, len;
+	char *logrec;
+
+	msglen = msg ? strlen(msg) : 0;
+	maxlen = strlen(committer) + msglen + 100;
+	logrec = xmalloc(maxlen);
+	len = sprintf(logrec, maxlen, "%s %s %s\n",
+		      sha1_to_hex(old_sha1),
+		      sha1_to_hex(new_sha1),
+		      committer);
+	if (msglen)
+		len += copy_msg(logrec + len - 1, msg) - 1;
+
+	written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
+	free(logrec);
+	if (written != len)
+		return -1;
+
+	return 0;
+}
+
 static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg)
 {
-	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
-	unsigned maxlen, len;
-	int msglen;
+	int logfd, result, oflags = O_APPEND | O_WRONLY;
 	char log_file[PATH_MAX];
-	char *logrec;
-	const char *committer;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 	logfd = open(log_file, oflags);
 	if (logfd < 0)
 		return 0;
-	msglen = msg ? strlen(msg) : 0;
-	committer = git_committer_info(0);
-	maxlen = strlen(committer) + msglen + 100;
-	logrec = xmalloc(maxlen);
-	len = sprintf(logrec, "%s %s %s\n",
-		      sha1_to_hex(old_sha1),
-		      sha1_to_hex(new_sha1),
-		      committer);
-	if (msglen)
-		len += copy_msg(logrec + len - 1, msg) - 1;
-	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
-	free(logrec);
-	if (written != len) {
+	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
+				  git_committer_info(0), msg);
+	if (result) {
 		int save_errno = errno;
 		close(logfd);
 		error("Unable to append to %s", log_file);
-- 
2.2.0.rc2.23.gca0107e

Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd

[Jonathan Nieder]

Stefan Beller wrote:

> From: Ronnie Sahlberg <sahlberg@google.com>
>
> Move code to create the string for a ref and write it to a file descriptor
> from log_ref_write and add it into a new dedicated function
> log_ref_write_fd.
>
> For now the new function is only used from log_ref_write, but later
> on we will call this function from reflog transactions too. That means
> that we will end up with only a single place, where we write a
> reflog entry to a file instead of the current two places
> (log_ref_write and builtin/reflog.c).

Line-wrapping width is still inconsistent.  I don't think it's worth
resending just for that, but something to look out for in the future.

> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
[...]
> +++ b/refs.c
[...]
> @@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
[...]
> +	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
> +				  git_committer_info(0), msg);
> +	if (result) {
>  		int save_errno = errno;

I don't understand why the above writes to a temporary variable and
checks it, never to read that temporary again.

I don't think that alone is a reason to block the patch, but it
worries me in that the review comment seems to have been just lost.

Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd

[Stefan Beller]

On Thu, Nov 20, 2014 at 1:20 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>
>> For now the new function is only used from log_ref_write, but later
>> on we will call this function from reflog transactions too. That means
>> that we will end up with only a single place, where we write a
>> reflog entry to a file instead of the current two places
>> (log_ref_write and builtin/reflog.c).
>
> Line-wrapping width is still inconsistent.  I don't think it's worth
> resending just for that, but something to look out for in the future.
>

ok, I'll care about that more in the future.


> I don't understand why the above writes to a temporary variable and
> checks it, never to read that temporary again.
>
> I don't think that alone is a reason to block the patch, but it
> worries me in that the review comment seems to have been just lost.

It wasn't lost as I think it should go in a follow up patch. Sorry for
not stating that clearly.
(This patch is about moving code around, not changing code)

I got interrupted preparing the follow up patch, which gets rid of the temporary
variable.

Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd

[Jonathan Nieder]

Stefan Beller wrote:
> On Thu, Nov 20, 2014 at 1:20 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> I don't understand why the above writes to a temporary variable and
>> checks it, never to read that temporary again.
>>
>> I don't think that alone is a reason to block the patch, but it
>> worries me in that the review comment seems to have been just lost.
>
> It wasn't lost as I think it should go in a follow up patch. Sorry for
> not stating that clearly.
> (This patch is about moving code around, not changing code)

Ah, sorry for the lack of clarity.

I agree completely with the above principle.  But the code that writes
to result and checks result is new code, not part of the code that
moved.

Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> From: Ronnie Sahlberg <sahlberg@google.com>
>
> Move code to create the string for a ref and write it to a file descriptor
> from log_ref_write and add it into a new dedicated function
> log_ref_write_fd.
>
> For now the new function is only used from log_ref_write, but later
> on we will call this function from reflog transactions too. That means
> that we will end up with only a single place, where we write a
> reflog entry to a file instead of the current two places
> (log_ref_write and builtin/reflog.c).
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Changes in version 3:
> * reword the commit message to make it more understandable.
> * no changes in code
> * wait for the follow up to address any changes in the code.
>
>  refs.c | 48 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 5ff457e..9948841 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
>  	return 0;
>  }
>  
> +static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
> +			    const unsigned char *new_sha1,
> +			    const char *committer, const char *msg)
> +{
> +	int msglen, written;
> +	unsigned maxlen, len;
> +	char *logrec;
> +
> +	msglen = msg ? strlen(msg) : 0;
> +	maxlen = strlen(committer) + msglen + 100;
> +	logrec = xmalloc(maxlen);
> +	len = sprintf(logrec, maxlen, "%s %s %s\n",

You cannot have maxlen here ;-)

> +		      sha1_to_hex(old_sha1),
> +		      sha1_to_hex(new_sha1),
> +		      committer);
> +	if (msglen)
> +		len += copy_msg(logrec + len - 1, msg) - 1;
> +
> +	written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
> +	free(logrec);
> +	if (written != len)
> +		return -1;
> +
> +	return 0;
> +}
> +
>  static int log_ref_write(const char *refname, const unsigned char *old_sha1,
>  			 const unsigned char *new_sha1, const char *msg)
>  {
> -	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
> -	unsigned maxlen, len;
> -	int msglen;
> +	int logfd, result, oflags = O_APPEND | O_WRONLY;
>  	char log_file[PATH_MAX];
> -	char *logrec;
> -	const char *committer;
>  
>  	if (log_all_ref_updates < 0)
>  		log_all_ref_updates = !is_bare_repository();
> @@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
>  	logfd = open(log_file, oflags);
>  	if (logfd < 0)
>  		return 0;
> -	msglen = msg ? strlen(msg) : 0;
> -	committer = git_committer_info(0);
> -	maxlen = strlen(committer) + msglen + 100;
> -	logrec = xmalloc(maxlen);
> -	len = sprintf(logrec, "%s %s %s\n",
> -		      sha1_to_hex(old_sha1),
> -		      sha1_to_hex(new_sha1),
> -		      committer);
> -	if (msglen)
> -		len += copy_msg(logrec + len - 1, msg) - 1;
> -	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
> -	free(logrec);
> -	if (written != len) {
> +	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
> +				  git_committer_info(0), msg);
> +	if (result) {
>  		int save_errno = errno;
>  		close(logfd);
>  		error("Unable to append to %s", log_file);

Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd

[Stefan Beller]

>From 4bec12b878ca02a1e80af3c265e7e7ab52ba17ce Mon Sep 17 00:00:00 2001
From: Ronnie Sahlberg <sahlberg@google.com>
Date: Thu, 20 Nov 2014 13:48:14 -0800
Subject: [PATCH v4] refs.c: add a function to append a reflog entry to a fd

Move code to create the string for a ref and write it to a file descriptor
from log_ref_write and add it into a new dedicated function
log_ref_write_fd.

For now the new function is only used from log_ref_write, but later
on we will call this function from reflog transactions too. That means
that we will end up with only a single place, where we write a reflog
entry to a file instead of the current two places (log_ref_write and
builtin/reflog.c).

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 
Changes in version 3:
 * reword the commit message to make it more understandable.
 * no changes in code
 * wait for the follow up to address any changes in the code.

Changes in version 4:
 * fix arguments of sprintf, (note to self: compile testing helps)
 * break lines of commit message again to appease the taste of Jonathan ;)
 * take the simplification for
   
   result = log_ref_write_fd(...);
   
   if (result) {
       ...
       
   and omit the result variable in there to just become
    
   if (log_ref_write_fd(...)) {
       ...
   
 refs.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index 5ff457e..a6088e3 100644
--- a/refs.c
+++ b/refs.c
@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
 	return 0;
 }
 
+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const char *committer, const char *msg)
+{
+	int msglen, written;
+	unsigned maxlen, len;
+	char *logrec;
+
+	msglen = msg ? strlen(msg) : 0;
+	maxlen = strlen(committer) + msglen + 100;
+	logrec = xmalloc(maxlen);
+	len = sprintf(logrec, "%s %s %s\n",
+		      sha1_to_hex(old_sha1),
+		      sha1_to_hex(new_sha1),
+		      committer);
+	if (msglen)
+		len += copy_msg(logrec + len - 1, msg) - 1;
+
+	written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
+	free(logrec);
+	if (written != len)
+		return -1;
+
+	return 0;
+}
+
 static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg)
 {
-	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
-	unsigned maxlen, len;
-	int msglen;
+	int logfd, result, oflags = O_APPEND | O_WRONLY;
 	char log_file[PATH_MAX];
-	char *logrec;
-	const char *committer;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 	logfd = open(log_file, oflags);
 	if (logfd < 0)
 		return 0;
-	msglen = msg ? strlen(msg) : 0;
-	committer = git_committer_info(0);
-	maxlen = strlen(committer) + msglen + 100;
-	logrec = xmalloc(maxlen);
-	len = sprintf(logrec, "%s %s %s\n",
-		      sha1_to_hex(old_sha1),
-		      sha1_to_hex(new_sha1),
-		      committer);
-	if (msglen)
-		len += copy_msg(logrec + len - 1, msg) - 1;
-	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
-	free(logrec);
-	if (written != len) {
+
+	if (log_ref_write_fd(logfd, old_sha1, new_sha1,
+			     git_committer_info(0), msg)) {
 		int save_errno = errno;
 		close(logfd);
 		error("Unable to append to %s", log_file);
-- 
2.2.0.rc2.23.gca0107e

Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd

[Jonathan Nieder]

Stefan Beller wrote:

> From 4bec12b878ca02a1e80af3c265e7e7ab52ba17ce Mon Sep 17 00:00:00 2001

The above line causes "git am" to be unable to parse the message
downloaded as an mbox, if I remember correctly.

[...]
>  * break lines of commit message again to appease the taste of Jonathan ;)

I hope it's not just to appease me.  If the rationale behind a comment I
make isn't clear, always feel free to ask.

In this case, it is about readability.  It's perhaps irrational, but I
find text much more readable and the intent to be clearer when
paragraphs are wrapped to a consistent width instead of lines breaking
at arbitrary points.

Perhaps a nice way to make this a non-issue in the future would be an
option to "git am" to rewrap.

Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> In this case, it is about readability.  It's perhaps irrational, but I
> find text much more readable and the intent to be clearer when
> paragraphs are wrapped to a consistent width instead of lines breaking
> at arbitrary points.

Yeah I agree with that.

Also a blank line between paragraphs, but that goes without saying
once you accept that we do not do a single line per paragraph.

> Perhaps a nice way to make this a non-issue in the future would be an
> option to "git am" to rewrap.

No.  There are cases where you do need to have a single oddball long
line that you cannot flow.  In a project that employs "am", the
users of "am" are much more likely to become bottleneck than the
senders of patches, and it does not make sense to have them spend
cycles to decide if they want to let "am" do such wrapping.

[PATCH v4] refs.c: add a function to append a reflog entry to a fd

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

Move code to create the string for a ref and write it to a file descriptor
from log_ref_write and add it into a new dedicated function
log_ref_write_fd.

For now the new function is only used from log_ref_write, but later
on we will call this function from reflog transactions too. That means
that we will end up with only a single place, where we write a reflog
entry to a file instead of the current two places (log_ref_write and
builtin/reflog.c).

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 
Changes in version 3:
 * reword the commit message to make it more understandable.
 * no changes in code
 * wait for the follow up to address any changes in the code.

Changes in version 4:
 * fix arguments of sprintf
 * break lines of commit message again to appease the taste of Jonathan ;)
 * take the simplification for
   
   result = log_ref_write_fd(...);
   
   if (result) {
       ...
       
   and omit the result variable in there to just become
    
   if (log_ref_write_fd(...)) {
       ...
   
 refs.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index 5ff457e..a6088e3 100644
--- a/refs.c
+++ b/refs.c
@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
 	return 0;
 }
 
+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const char *committer, const char *msg)
+{
+	int msglen, written;
+	unsigned maxlen, len;
+	char *logrec;
+
+	msglen = msg ? strlen(msg) : 0;
+	maxlen = strlen(committer) + msglen + 100;
+	logrec = xmalloc(maxlen);
+	len = sprintf(logrec, "%s %s %s\n",
+		      sha1_to_hex(old_sha1),
+		      sha1_to_hex(new_sha1),
+		      committer);
+	if (msglen)
+		len += copy_msg(logrec + len - 1, msg) - 1;
+
+	written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
+	free(logrec);
+	if (written != len)
+		return -1;
+
+	return 0;
+}
+
 static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg)
 {
-	int logfd, result, written, oflags = O_APPEND | O_WRONLY;
-	unsigned maxlen, len;
-	int msglen;
+	int logfd, result, oflags = O_APPEND | O_WRONLY;
 	char log_file[PATH_MAX];
-	char *logrec;
-	const char *committer;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
 	logfd = open(log_file, oflags);
 	if (logfd < 0)
 		return 0;
-	msglen = msg ? strlen(msg) : 0;
-	committer = git_committer_info(0);
-	maxlen = strlen(committer) + msglen + 100;
-	logrec = xmalloc(maxlen);
-	len = sprintf(logrec, "%s %s %s\n",
-		      sha1_to_hex(old_sha1),
-		      sha1_to_hex(new_sha1),
-		      committer);
-	if (msglen)
-		len += copy_msg(logrec + len - 1, msg) - 1;
-	written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
-	free(logrec);
-	if (written != len) {
+
+	if (log_ref_write_fd(logfd, old_sha1, new_sha1,
+			     git_committer_info(0), msg)) {
 		int save_errno = errno;
 		close(logfd);
 		error("Unable to append to %s", log_file);
-- 
2.2.0.rc2.23.gca0107e

Re: [PATCH v4] refs.c: add a function to append a reflog entry to a fd

[Jonathan Nieder]

Stefan Beller wrote:

> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
[...]
>  refs.c | 48 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 18 deletions(-)

The --patience diff makes review of this version very simple.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH v4] refs.c: add a function to append a reflog entry to a fd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Stefan Beller wrote:
>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
> [...]
>>  refs.c | 48 ++++++++++++++++++++++++++++++------------------
>>  1 file changed, 30 insertions(+), 18 deletions(-)
>
> The --patience diff makes review of this version very simple.

I guess it depends on what the reader is focusing on, but at least
to me I found the default or histogram much easier to see what is
going on.  It could be due to my use of large -U<context> value,
too.

> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH] refs.c: add a function to append a reflog entry to a fd

[Jonathan Nieder]

Stefan Beller wrote:

> Compared to the last send of this patch[1], there was one change in the print
> function. Replaced sprintf by snprintf for security reasons.

I prefer the version that didn't change the code while we are
extracting it into a new function.  A separate patch on top can switch
it to use strbuf_addf or xstrfmt if we want that.

My two cents,
Jonathan