Re: [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/
Jeff King wrote: > On Mon, Feb 18, 2013 at 02:19:15AM -0800, Jonathan Nieder wrote: >> In combination with patch 3, this changes the meaning of packet_read() >> without changing its signature, which could make other patches >> cherry-picked on top change behavior in unpredictable ways. :( >> >> So I'd be all for this if the signature changes (for example to put >> the fd at the end or something), but not so if not. > > True. Though packet_read has only existed since last June, only had one > callsite (which would now conflict, since I'm touching it in this > series), and has no new calls in origin..origin/pu. So it's relatively > low risk for such a problem. I don't know how careful we want to be. I was unclear. What I am worried about is that someone using a version of git without this patch will try some yet-to-be-written patch using packet_read from the mailing list and not notice that they are using the wrong function. For example, if someone is using 1.7.12.y or 1.8.1.y and wants to try a patch from after the above, they would get subtly different and wrong results. The rule "change the name or signature when breaking the ABI of a global function" is easy to remember and follow. I think we want not to have to be careful at all, and such rules can help with that. :) Thanks, Jonathan -- 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: [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/
On Mon, Feb 18, 2013 at 02:19:15AM -0800, Jonathan Nieder wrote: > Jeff King wrote: > > > Originally packets were used just for the line-oriented ref > > advertisement and negotiation. These days, we also stuff > > packfiles and sidebands into them, and they do not > > necessarily represent a line. Drop the "_line" suffix, as it > > is not informative and makes the function names quite long > > (especially as we add "_gently" and other variants). > > > > Signed-off-by: Jeff King > > --- > > Again, this is a taste issue. Can be optional. > > In combination with patch 3, this changes the meaning of packet_read() > without changing its signature, which could make other patches > cherry-picked on top change behavior in unpredictable ways. :( > > So I'd be all for this if the signature changes (for example to put > the fd at the end or something), but not so if not. True. Though packet_read has only existed since last June, only had one callsite (which would now conflict, since I'm touching it in this series), and has no new calls in origin..origin/pu. So it's relatively low risk for such a problem. I don't know how careful we want to be. -Peff -- 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: [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/
Jeff King wrote: > Originally packets were used just for the line-oriented ref > advertisement and negotiation. These days, we also stuff > packfiles and sidebands into them, and they do not > necessarily represent a line. Drop the "_line" suffix, as it > is not informative and makes the function names quite long > (especially as we add "_gently" and other variants). > > Signed-off-by: Jeff King > --- > Again, this is a taste issue. Can be optional. In combination with patch 3, this changes the meaning of packet_read() without changing its signature, which could make other patches cherry-picked on top change behavior in unpredictable ways. :( So I'd be all for this if the signature changes (for example to put the fd at the end or something), but not so if not. Thanks, Jonathan -- 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
[PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/
Originally packets were used just for the line-oriented ref
advertisement and negotiation. These days, we also stuff
packfiles and sidebands into them, and they do not
necessarily represent a line. Drop the "_line" suffix, as it
is not informative and makes the function names quite long
(especially as we add "_gently" and other variants).
Signed-off-by: Jeff King
---
Again, this is a taste issue. Can be optional.
builtin/archive.c| 4 ++--
builtin/fetch-pack.c | 2 +-
builtin/receive-pack.c | 2 +-
builtin/upload-archive.c | 2 +-
connect.c| 2 +-
daemon.c | 2 +-
fetch-pack.c | 6 +++---
pkt-line.c | 4 ++--
pkt-line.h | 6 +++---
remote-curl.c| 6 +++---
send-pack.c | 4 ++--
sideband.c | 2 +-
upload-pack.c| 4 ++--
13 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/builtin/archive.c b/builtin/archive.c
index 9a1cfd3..bf600f5 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -53,7 +53,7 @@ static int run_remote_archiver(int argc, const char **argv,
packet_write(fd[1], "argument %s\n", argv[i]);
packet_flush(fd[1]);
- len = packet_read_line(fd[0], buf, sizeof(buf));
+ len = packet_read(fd[0], buf, sizeof(buf));
if (!len)
die(_("git archive: expected ACK/NAK, got EOF"));
if (buf[len-1] == '\n')
@@ -66,7 +66,7 @@ static int run_remote_archiver(int argc, const char **argv,
die(_("git archive: protocol error"));
}
- len = packet_read_line(fd[0], buf, sizeof(buf));
+ len = packet_read(fd[0], buf, sizeof(buf));
if (len)
die(_("git archive: expected a flush"));
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 940ae35..b59e60e 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -102,7 +102,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char
*prefix)
*/
static char line[1000];
for (;;) {
- int n = packet_read_line(0, line, sizeof(line));
+ int n = packet_read(0, line, sizeof(line));
if (!n)
break;
if (line[n-1] == '\n')
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 48cd5dc..3f58ce8 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -736,7 +736,7 @@ static struct command *read_head_info(void)
char *refname;
int len, reflen;
- len = packet_read_line(0, line, sizeof(line));
+ len = packet_read(0, line, sizeof(line));
if (!len)
break;
if (line[len-1] == '\n')
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index b928beb..2ecf461 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -40,7 +40,7 @@ int cmd_upload_archive_writer(int argc, const char **argv,
const char *prefix)
sent_argv[0] = "git-upload-archive";
for (p = buf;;) {
/* This will die if not enough free space in buf */
- len = packet_read_line(0, p, (buf + sizeof buf) - p);
+ len = packet_read(0, p, (buf + sizeof buf) - p);
if (len == 0)
break; /* got a flush */
if (sent_argc > MAX_ARGS - 2)
diff --git a/connect.c b/connect.c
index 7e0920d..59266b1 100644
--- a/connect.c
+++ b/connect.c
@@ -76,7 +76,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
char *name;
int len, name_len;
- len = packet_read_line_gently(in, buffer, sizeof(buffer));
+ len = packet_read_gently(in, buffer, sizeof(buffer));
if (len < 0)
die_initial_contact(got_at_least_one_head);
diff --git a/daemon.c b/daemon.c
index 4602b46..ac26b93 100644
--- a/daemon.c
+++ b/daemon.c
@@ -612,7 +612,7 @@ static int execute(void)
loginfo("Connection from %s:%s", addr, port);
alarm(init_timeout ? init_timeout : timeout);
- pktlen = packet_read_line(0, line, sizeof(line));
+ pktlen = packet_read(0, line, sizeof(line));
alarm(0);
len = strlen(line);
diff --git a/fetch-pack.c b/fetch-pack.c
index b90dadf..d89c9d0 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -173,7 +173,7 @@ static void consume_shallow_list(struct fetch_pack_args
*args, int fd)
* is a block of have lines exchanged.
*/
char line[1000];
- while (packet_read_line(fd, line, sizeof(line))) {
+ while (packet_read(fd, line, sizeof(line))) {
if (!prefixcmp(line, "shallow "))
con
