Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd
Jonathan Nieder 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd
>From 4bec12b878ca02a1e80af3c265e7e7ab52ba17ce Mon Sep 17 00:00:00 2001
From: Ronnie Sahlberg
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
Signed-off-by: Stefan Beller
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd
Stefan Beller writes:
> From: Ronnie Sahlberg
>
> 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
> Signed-off-by: Stefan Beller
> ---
>
> 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);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd
Stefan Beller wrote: > On Thu, Nov 20, 2014 at 1:20 PM, Jonathan Nieder 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd
On Thu, Nov 20, 2014 at 1:20 PM, Jonathan Nieder 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd
Stefan Beller wrote:
> From: Ronnie Sahlberg
>
> 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
> Signed-off-by: Stefan Beller
[...]
> +++ 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
