Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
Wed, Oct 14, 2015 at 12:24 AM, Matthieu Moy wrote: > Yes, but I still think that this was a bad idea. If you want nobracket > to apply to "track", then the syntax should be > %(upstream:track=nobracket). I think the "nobracket" should apply to > "upstream" (i.e. be global to the atom), hence > %(upstream:nobracket,track) and %(upstream:track,nobracket) should both > be accepted. Possibly %(upstream:,nobracket) could complain, > but just ignoring "nobracket" in this case would make sense to me. > Oh okay, was thinking only WRT the "track" option. > Special-casing the implementation of "nobracket" also means you're > special-casing its user-visible behavior. And as a user, I hate it when > the behavior is subtely different for things that look like each other > (in this case, %(align:...) and %(upstream:...) ). > Makes sense, was just looking for opinions. >> %(upstream:nobracket,track) to be supported then, I think we'll have >> to change this whole layout and have the detection done up where we >> locat "upstream" / "push", what would be a nice way to go around this? > > You mean, below > > else if (starts_with(name, "upstream")) { > > within populate_value()? > > I think it would, yes. > Yes, that's what I meant. >> What I could think of: >> 1. Do the cleanup that Junio and Matthieu suggested, where we >> basically parse the >> atoms and store them into a used_atom struct. I could either work on >> those patches >> before this and then rebase this on top. >> 2. Let this be and come back on it when implementing the above series. >> After reading Matthieu's and Junio's discussion, I lean towards the latter. > > Leaving it as-is does not fit in my arguments to do the refactoring > later. It's not introducing "another instance of an existing pattern", > but actually a new pattern. > I meant after changing whatever we discussed above. -- Regards, Karthik Nayak -- 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 01/13] refs: convert some internal functions to use object_id
On Tue, Oct 13, 2015 at 01:43:30PM +0200, Michael Haggerty wrote: > On 10/09/2015 03:43 AM, brian m. carlson wrote: > > Convert several internal functions in refs.c to use struct object_id, > > and use the GIT_SHA1_HEXSZ constants in parse_ref_line. > > > > Signed-off-by: brian m. carlson > > [...] > > I looked over this patch at the diff level and didn't find any problems. > > This patch conflicts heavily with [1]. But I noticed that it is > independent of the rest of your series. I don't know when either patch > series will be ready, but see [2] for my take on the other one. > > Assuming neither series is rejected, I think it would be much easier to > redo this patch on top of the first part of [1] than vice versa, so that > would be my suggestion. If it comes down to that, I am willing to help > redo this patch if you like. I'll take a look at it over the next couple of days. I'll probably drop it from this series and either see if it can be put on top of the other one, or defer it to a future series. That way we can minimize conflicts. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: PGP signature
[PATCH 28/26] am: make direct call to mailinfo
And finally the endgame. Instead of spawning "git mailinfo" via the run_command() API the same number of times as there are incoming patches, make direct internal call to the libified mailinfo() from "git am" to reduce the spawning overhead, which would matter on some platforms. Signed-off-by: Junio C Hamano --- This is to be applied on top of a merge across - jc/am-3-fallback-regression-fix - jc/mailinfo-lib (i.e. the 27-patch series) As I do not have ready access to such a platform with slow run_command(), it would be nice to see some numbers produced on such platform by other people. Thanks. builtin/am.c | 38 +++--- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index c869796..0471355 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -27,6 +27,7 @@ #include "notes-utils.h" #include "rerere.h" #include "prompt.h" +#include "mailinfo.h" /** * Returns 1 if the file is empty or does not exist, 0 otherwise. @@ -1258,58 +1259,57 @@ static void am_append_signoff(struct am_state *state) static int parse_mail(struct am_state *state, const char *mail) { FILE *fp; - struct child_process cp = CHILD_PROCESS_INIT; struct strbuf sb = STRBUF_INIT; struct strbuf msg = STRBUF_INIT; struct strbuf author_name = STRBUF_INIT; struct strbuf author_date = STRBUF_INIT; struct strbuf author_email = STRBUF_INIT; int ret = 0; + struct mailinfo mi; - cp.git_cmd = 1; - cp.in = xopen(mail, O_RDONLY, 0); - cp.out = xopen(am_path(state, "info"), O_WRONLY | O_CREAT, 0777); + setup_mailinfo(&mi); - argv_array_push(&cp.args, "mailinfo"); - argv_array_push(&cp.args, state->utf8 ? "-u" : "-n"); + if (state->utf8) + mi.metainfo_charset = get_commit_output_encoding(); + else + mi.metainfo_charset = NULL; switch (state->keep) { case KEEP_FALSE: break; case KEEP_TRUE: - argv_array_push(&cp.args, "-k"); + mi.keep_subject = 1; break; case KEEP_NON_PATCH: - argv_array_push(&cp.args, "-b"); + mi.keep_non_patch_brackets_in_subject = 1; break; default: die("BUG: invalid value for state->keep"); } - if (state->message_id) - argv_array_push(&cp.args, "-m"); + mi.add_message_id = 1; switch (state->scissors) { case SCISSORS_UNSET: break; case SCISSORS_FALSE: - argv_array_push(&cp.args, "--no-scissors"); + mi.use_scissors = 0; break; case SCISSORS_TRUE: - argv_array_push(&cp.args, "--scissors"); + mi.use_scissors = 1; break; default: die("BUG: invalid value for state->scissors"); } - argv_array_push(&cp.args, am_path(state, "msg")); - argv_array_push(&cp.args, am_path(state, "patch")); + mi.input = fopen(mail, "r"); + mi.output = fopen(am_path(state, "info"), "w"); - if (run_command(&cp) < 0) + if (mailinfo(&mi, am_path(state, "msg"), am_path(state, "patch"))) die("could not parse patch"); - close(cp.in); - close(cp.out); + fclose(mi.input); + fclose(mi.output); /* Extract message and author information */ fp = xfopen(am_path(state, "info"), "r"); @@ -1341,8 +1341,7 @@ static int parse_mail(struct am_state *state, const char *mail) } strbuf_addstr(&msg, "\n\n"); - if (strbuf_read_file(&msg, am_path(state, "msg"), 0) < 0) - die_errno(_("could not read '%s'"), am_path(state, "msg")); + strbuf_addbuf(&msg, &mi.log_message); stripspace(&msg, 0); if (state->signoff) @@ -1366,6 +1365,7 @@ finish: strbuf_release(&author_email); strbuf_release(&author_name); strbuf_release(&sb); + clear_mailinfo(&mi); return ret; } -- 2.6.1-320-g86a1181 -- 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
[PATCH 27/26] mailinfo: close patchfile
"git mailinfo" itself did not need this, as open file handles are flushed and closed upon process exit, but the libified version needs to clean up after itself when it is done. Signed-off-by: Junio C Hamano --- mailinfo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mailinfo.c b/mailinfo.c index ca40030..00e2ea4 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -1013,7 +1013,7 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) fwrite(mi->log_message.buf, 1, mi->log_message.len, cmitmsg); } fclose(cmitmsg); - + fclose(mi->patchfile); return mi->input_error; } -- 2.6.1-320-g86a1181 -- 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 05/44] refs.c: move update_ref to refs.c
On Tue, 2015-10-13 at 05:41 +0200, Michael Haggerty wrote: > The original read > > if (read_ref(pseudoref, actual_old_sha1)) > die("could not read ref '%s'", pseudoref); > > This seems like an important test. What happened to it? > > If its removal was intentional, it deserves a careful explanation (and > should probably be done as a separate commit). If it was an accident, > please consider how this accident arose and try to think about whether > similar accidents might have happened elsewhere in this series. I went ahead and manually rechecked all of the patches to ensure that nothing else was screwed up. While doing so, I found two or three spurious whitespace changes (moving newlines around), which I fixed (in refs-backend-pre-vtable, which I'll send to the list tomorrow-ish). -- 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
[BUG] Broken links in Git Documentation/user-manual.txt
Hi list, In https://git-scm.com/docs/user-manual.html , all links to the glossary[1] are broken. I'm aware of a recent bug report about broken links ($gmane/279048/), but that one seems unrelated to what I'm reporting. [1] For example: https://git-scm.com/docs/user-manual.html#def_head -- 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
[PATCH 08/26] mailinfo: introduce "struct mailinfo" to hold globals
Initially have only 'email' and 'name' fields in there and remove the corresponding globals. In subsequent patches, more globals will be moved to this and the structure will be passed around as a new parameter to more functions. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 61 +- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 6c671fb..7b61187 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -12,8 +12,11 @@ static FILE *cmitmsg, *patchfile, *fin, *fout; static int keep_subject; static int keep_non_patch_brackets_in_subject; static const char *metainfo_charset; -static struct strbuf name = STRBUF_INIT; -static struct strbuf email = STRBUF_INIT; + +struct mailinfo { + struct strbuf name; + struct strbuf email; +}; static char *message_id; static enum { @@ -45,7 +48,7 @@ static void get_sane_name(struct strbuf *out, struct strbuf *name, struct strbuf strbuf_addbuf(out, src); } -static void parse_bogus_from(const struct strbuf *line) +static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line) { /* John Doe */ @@ -53,7 +56,7 @@ static void parse_bogus_from(const struct strbuf *line) /* This is fallback, so do not bother if we already have an * e-mail address. */ - if (email.len) + if (mi->email.len) return; bra = strchr(line->buf, '<'); @@ -63,16 +66,16 @@ static void parse_bogus_from(const struct strbuf *line) if (!ket) return; - strbuf_reset(&email); - strbuf_add(&email, bra + 1, ket - bra - 1); + strbuf_reset(&mi->email); + strbuf_add(&mi->email, bra + 1, ket - bra - 1); - strbuf_reset(&name); - strbuf_add(&name, line->buf, bra - line->buf); - strbuf_trim(&name); - get_sane_name(&name, &name, &email); + strbuf_reset(&mi->name); + strbuf_add(&mi->name, line->buf, bra - line->buf); + strbuf_trim(&mi->name); + get_sane_name(&mi->name, &mi->name, &mi->email); } -static void handle_from(const struct strbuf *from) +static void handle_from(struct mailinfo *mi, const struct strbuf *from) { char *at; size_t el; @@ -83,14 +86,14 @@ static void handle_from(const struct strbuf *from) at = strchr(f.buf, '@'); if (!at) { - parse_bogus_from(from); + parse_bogus_from(mi, from); return; } /* * If we already have one email, don't take any confusing lines */ - if (email.len && strchr(at + 1, '@')) { + if (mi->email.len && strchr(at + 1, '@')) { strbuf_release(&f); return; } @@ -109,8 +112,8 @@ static void handle_from(const struct strbuf *from) at--; } el = strcspn(at, " \n\t\r\v\f>"); - strbuf_reset(&email); - strbuf_add(&email, at, el); + strbuf_reset(&mi->email); + strbuf_add(&mi->email, at, el); strbuf_remove(&f, at - f.buf, el + (at[el] ? 1 : 0)); /* The remainder is name. It could be @@ -132,7 +135,7 @@ static void handle_from(const struct strbuf *from) strbuf_setlen(&f, f.len - 1); } - get_sane_name(&name, &f, &email); + get_sane_name(&mi->name, &f, &mi->email); strbuf_release(&f); } @@ -958,7 +961,7 @@ static void output_header_lines(FILE *fout, const char *hdr, const struct strbuf } } -static void handle_info(void) +static void handle_info(struct mailinfo *mi) { struct strbuf *hdr; int i; @@ -980,9 +983,9 @@ static void handle_info(void) output_header_lines(fout, "Subject", hdr); } else if (!strcmp(header[i], "From")) { cleanup_space(hdr); - handle_from(hdr); - fprintf(fout, "Author: %s\n", name.buf); - fprintf(fout, "Email: %s\n", email.buf); + handle_from(mi, hdr); + fprintf(fout, "Author: %s\n", mi->name.buf); + fprintf(fout, "Email: %s\n", mi->email.buf); } else { cleanup_space(hdr); fprintf(fout, "%s: %s\n", header[i], hdr->buf); @@ -991,7 +994,8 @@ static void handle_info(void) fprintf(fout, "\n"); } -static int mailinfo(FILE *in, FILE *out, const char *msg, const char *patch) +static int mailinfo(struct mailinfo *mi, + FILE *in, FILE *out, const char *msg, const char *patch) { int peek; struct strbuf line = STRBUF_INIT; @@ -1024,7 +1028,7 @@ static int mailinfo(FILE *in, FILE *out, const char *msg, const char *patch) check_header(&line, p_hdr_data, 1); handle_body(&line); - handle_info(); + handle_i
[PATCH 05/26] mailinfo: get rid of function-local static states
Two helper functions use "static int" in their scope to keep track of the state while repeatedly getting called once for each input line. Move these state variables their ultimate caller and pass down pointers to them, as a small step in preparation for making this entire callchain more reentrant. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 48 +++- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 175c6ae..5a74811 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -713,21 +713,20 @@ static int is_scissors_line(const struct strbuf *line) gap * 2 < perforation); } -static int handle_commit_msg(struct strbuf *line) +static int handle_commit_msg(struct strbuf *line, int *still_looking) { /* * Are we still scanning and discarding in-body headers? * It is initially set to 1, set to 2 when we do see a * valid in-body header. */ - static int still_looking = 1; int is_empty_line; if (!cmitmsg) return 0; is_empty_line = (!line->len || (line->len == 1 && line->buf[0] == '\n')); - if (still_looking == 1) { + if (*still_looking == 1) { /* * Haven't seen a known in-body header; discard an empty line. */ @@ -735,33 +734,33 @@ static int handle_commit_msg(struct strbuf *line) return 0; } - if (use_inbody_headers && still_looking) { + if (use_inbody_headers && *still_looking) { int is_known_header = check_header(line, s_hdr_data, 0); - if (still_looking == 2) { + if (*still_looking == 2) { /* * an empty line after the in-body header block, * or a line obviously not an attempt to invent * an unsupported in-body header. */ if (is_empty_line || !is_rfc2822_header(line)) - still_looking = 0; + *still_looking = 0; if (is_empty_line) return 0; /* otherwise do not discard the line, but keep going */ } else if (is_known_header) { - still_looking = 2; - } else if (still_looking != 2) { - still_looking = 0; + *still_looking = 2; + } else if (*still_looking != 2) { + *still_looking = 0; } - if (still_looking) + if (*still_looking) return 0; } else /* Only trim the first (blank) line of the commit message * when ignoring in-body headers. */ - still_looking = 0; + *still_looking = 0; /* normalize the log message to UTF-8. */ if (metainfo_charset) @@ -773,7 +772,7 @@ static int handle_commit_msg(struct strbuf *line) die_errno("Could not rewind output message file"); if (ftruncate(fileno(cmitmsg), 0)) die_errno("Could not truncate output message file at scissors"); - still_looking = 1; + *still_looking = 1; /* * We may have already read "secondary headers"; purge @@ -805,16 +804,13 @@ static void handle_patch(const struct strbuf *line) patch_lines++; } -static void handle_filter(struct strbuf *line) +static void handle_filter(struct strbuf *line, int *filter_stage, int *header_stage) { - static int filter = 0; - - /* filter tells us which part we left off on */ - switch (filter) { + switch (*filter_stage) { case 0: - if (!handle_commit_msg(line)) + if (!handle_commit_msg(line, header_stage)) break; - filter++; + (*filter_stage)++; case 1: handle_patch(line); break; @@ -830,7 +826,7 @@ static int find_boundary(void) return 0; } -static int handle_boundary(void) +static int handle_boundary(int *filter_stage, int *header_stage) { struct strbuf newline = STRBUF_INIT; @@ -852,7 +848,7 @@ again: "can't recover\n"); exit(1); } - handle_filter(&newline); + handle_filter(&newline, filter_stage, header_stage); strbuf_release(&newline); /* skip to the next boundary */ @@ -880,6 +876,8 @@ again: static void handle_body(void) { struct strbuf prev = STRBUF_INIT; + int filter_stage = 0; + int header_stage = 1;
[PATCH 20/26] mailinfo: move [ps]_hdr_data to struct mailinfo
Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 28 ++-- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index b591a2f..c9629c8 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -30,10 +30,10 @@ struct mailinfo { int patch_lines; int filter_stage; /* still reading log or are we copying patch? */ int header_stage; /* still checking in-body headers? */ + struct strbuf **p_hdr_data; + struct strbuf **s_hdr_data; }; -static struct strbuf **p_hdr_data, **s_hdr_data; - #define MAX_HDR_PARSED 10 #define MAX_BOUNDARIES 5 @@ -743,7 +743,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) } if (mi->use_inbody_headers && mi->header_stage) { - int is_known_header = check_header(mi, line, s_hdr_data, 0); + int is_known_header = check_header(mi, line, mi->s_hdr_data, 0); if (mi->header_stage == 2) { /* @@ -786,9 +786,9 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) * them to give ourselves a clean restart. */ for (i = 0; header[i]; i++) { - if (s_hdr_data[i]) - strbuf_release(s_hdr_data[i]); - s_hdr_data[i] = NULL; + if (mi->s_hdr_data[i]) + strbuf_release(mi->s_hdr_data[i]); + mi->s_hdr_data[i] = NULL; } return 0; } @@ -870,7 +870,7 @@ again: /* slurp in this section's info */ while (read_one_header_line(line, mi->input)) - check_header(mi, line, p_hdr_data, 0); + check_header(mi, line, mi->p_hdr_data, 0); strbuf_release(&newline); /* replenish line */ @@ -971,10 +971,10 @@ static void handle_info(struct mailinfo *mi) for (i = 0; header[i]; i++) { /* only print inbody headers if we output a patch file */ - if (mi->patch_lines && s_hdr_data[i]) - hdr = s_hdr_data[i]; - else if (p_hdr_data[i]) - hdr = p_hdr_data[i]; + if (mi->patch_lines && mi->s_hdr_data[i]) + hdr = mi->s_hdr_data[i]; + else if (mi->p_hdr_data[i]) + hdr = mi->p_hdr_data[i]; else continue; @@ -1014,8 +1014,8 @@ static int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) return -1; } - p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*p_hdr_data)); - s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*s_hdr_data)); + mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data))); + mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data))); do { peek = fgetc(mi->input); @@ -1024,7 +1024,7 @@ static int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) /* process the email header */ while (read_one_header_line(&line, mi->input)) - check_header(mi, &line, p_hdr_data, 1); + check_header(mi, &line, mi->p_hdr_data, 1); handle_body(mi, &line); handle_info(mi); -- 2.6.1-320-g86a1181 -- 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
[PATCH 23/26] mailinfo: handle errors found in decode_header() better
The decode_header() function tries to unwrap RFC2047-style quoted strings but punts when it finds anything it cannot parse. Allow the function to report if it punted to the caller, and use the resulting string in the caller only when the function says it parsed the input successfully. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 27 --- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 4b9b1cc..6452a4c 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -293,7 +293,7 @@ static void cleanup_space(struct strbuf *sb) } } -static void decode_header(struct mailinfo *mi, struct strbuf *line); +static int decode_header(struct mailinfo *mi, struct strbuf *line); static const char *header[MAX_HDR_PARSED] = { "From","Subject","Date", }; @@ -336,8 +336,8 @@ static int check_header(struct mailinfo *mi, * normalize the meta information to utf8. */ strbuf_add(&sb, line->buf + len + 2, line->len - len - 2); - decode_header(mi, &sb); - handle_header(&hdr_data[i], &sb); + if (!decode_header(mi, &sb)) + handle_header(&hdr_data[i], &sb); ret = 1; goto check_header_out; } @@ -347,25 +347,26 @@ static int check_header(struct mailinfo *mi, if (cmp_header(line, "Content-Type")) { len = strlen("Content-Type: "); strbuf_add(&sb, line->buf + len, line->len - len); - decode_header(mi, &sb); - strbuf_insert(&sb, 0, "Content-Type: ", len); - handle_content_type(mi, &sb); + if (!decode_header(mi, &sb)) { + strbuf_insert(&sb, 0, "Content-Type: ", len); + handle_content_type(mi, &sb); + } ret = 1; goto check_header_out; } if (cmp_header(line, "Content-Transfer-Encoding")) { len = strlen("Content-Transfer-Encoding: "); strbuf_add(&sb, line->buf + len, line->len - len); - decode_header(mi, &sb); - handle_content_transfer_encoding(mi, &sb); + if (!decode_header(mi, &sb)) + handle_content_transfer_encoding(mi, &sb); ret = 1; goto check_header_out; } if (cmp_header(line, "Message-Id")) { len = strlen("Message-Id: "); strbuf_add(&sb, line->buf + len, line->len - len); - decode_header(mi, &sb); - handle_message_id(mi, &sb); + if (!decode_header(mi, &sb)) + handle_message_id(mi, &sb); ret = 1; goto check_header_out; } @@ -537,11 +538,12 @@ static void convert_to_utf8(struct mailinfo *mi, strbuf_attach(line, out, strlen(out), strlen(out)); } -static void decode_header(struct mailinfo *mi, struct strbuf *it) +static int decode_header(struct mailinfo *mi, struct strbuf *it) { char *in, *ep, *cp; struct strbuf outbuf = STRBUF_INIT, *dec; struct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT; + int found_error = -1; /* pessimism */ in = it->buf; while (in - it->buf <= it->len && (ep = strstr(in, "=?")) != NULL) { @@ -610,10 +612,13 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it) strbuf_addstr(&outbuf, in); strbuf_reset(it); strbuf_addbuf(it, &outbuf); + found_error = 0; release_return: strbuf_release(&outbuf); strbuf_release(&charset_q); strbuf_release(&piecebuf); + + return found_error; } static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line) -- 2.6.1-320-g86a1181 -- 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
[PATCH 09/26] mailinfo: move keep_subject & keep_non_patch_bracket to struct mailinfo
These two are the only easy ones that do not require passing the structure around to deep corners of the callchain. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 7b61187..d642b0d 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -9,13 +9,13 @@ static FILE *cmitmsg, *patchfile, *fin, *fout; -static int keep_subject; -static int keep_non_patch_brackets_in_subject; static const char *metainfo_charset; struct mailinfo { struct strbuf name; struct strbuf email; + int keep_subject; + int keep_non_patch_brackets_in_subject; }; static char *message_id; @@ -224,7 +224,7 @@ static int is_multipart_boundary(const struct strbuf *line) !memcmp(line->buf, (*content_top)->buf, (*content_top)->len)); } -static void cleanup_subject(struct strbuf *subject) +static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject) { size_t at = 0; @@ -252,7 +252,7 @@ static void cleanup_subject(struct strbuf *subject) if (!pos) break; remove = pos - subject->buf + at + 1; - if (!keep_non_patch_brackets_in_subject || + if (!mi->keep_non_patch_brackets_in_subject || (7 <= remove && memmem(subject->buf + at, remove, "PATCH", 5))) strbuf_remove(subject, at, remove); @@ -976,8 +976,8 @@ static void handle_info(struct mailinfo *mi) continue; if (!strcmp(header[i], "Subject")) { - if (!keep_subject) { - cleanup_subject(hdr); + if (!mi->keep_subject) { + cleanup_subject(mi, hdr); cleanup_space(hdr); } output_header_lines(fout, "Subject", hdr); @@ -1071,9 +1071,9 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix) while (1 < argc && argv[1][0] == '-') { if (!strcmp(argv[1], "-k")) - keep_subject = 1; + mi.keep_subject = 1; else if (!strcmp(argv[1], "-b")) - keep_non_patch_brackets_in_subject = 1; + mi.keep_non_patch_brackets_in_subject = 1; else if (!strcmp(argv[1], "-m") || !strcmp(argv[1], "--message-id")) add_message_id = 1; else if (!strcmp(argv[1], "-u")) -- 2.6.1-320-g86a1181 -- 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
[PATCH 04/26] mailinfo: move handle_boundary() lower
This function wants to call find_boundary() and is called only from one place without any recursing, so it becomes easier to read if it appears after the called function. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 114 ++--- 1 file changed, 56 insertions(+), 58 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 3b015a5..175c6ae 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -626,64 +626,6 @@ static void decode_transfer_encoding(struct strbuf *line) free(ret); } -static void handle_filter(struct strbuf *line); - -static int find_boundary(void) -{ - while (!strbuf_getline(&line, fin, '\n')) { - if (*content_top && is_multipart_boundary(&line)) - return 1; - } - return 0; -} - -static int handle_boundary(void) -{ - struct strbuf newline = STRBUF_INIT; - - strbuf_addch(&newline, '\n'); -again: - if (line.len >= (*content_top)->len + 2 && - !memcmp(line.buf + (*content_top)->len, "--", 2)) { - /* we hit an end boundary */ - /* pop the current boundary off the stack */ - strbuf_release(*content_top); - free(*content_top); - *content_top = NULL; - - /* technically won't happen as is_multipart_boundary() - will fail first. But just in case.. -*/ - if (--content_top < content) { - fprintf(stderr, "Detected mismatched boundaries, " - "can't recover\n"); - exit(1); - } - handle_filter(&newline); - strbuf_release(&newline); - - /* skip to the next boundary */ - if (!find_boundary()) - return 0; - goto again; - } - - /* set some defaults */ - transfer_encoding = TE_DONTCARE; - strbuf_reset(&charset); - - /* slurp in this section's info */ - while (read_one_header_line(&line, fin)) - check_header(&line, p_hdr_data, 0); - - strbuf_release(&newline); - /* replenish line */ - if (strbuf_getline(&line, fin, '\n')) - return 0; - strbuf_addch(&line, '\n'); - return 1; -} - static inline int patchbreak(const struct strbuf *line) { size_t i; @@ -879,6 +821,62 @@ static void handle_filter(struct strbuf *line) } } +static int find_boundary(void) +{ + while (!strbuf_getline(&line, fin, '\n')) { + if (*content_top && is_multipart_boundary(&line)) + return 1; + } + return 0; +} + +static int handle_boundary(void) +{ + struct strbuf newline = STRBUF_INIT; + + strbuf_addch(&newline, '\n'); +again: + if (line.len >= (*content_top)->len + 2 && + !memcmp(line.buf + (*content_top)->len, "--", 2)) { + /* we hit an end boundary */ + /* pop the current boundary off the stack */ + strbuf_release(*content_top); + free(*content_top); + *content_top = NULL; + + /* technically won't happen as is_multipart_boundary() + will fail first. But just in case.. +*/ + if (--content_top < content) { + fprintf(stderr, "Detected mismatched boundaries, " + "can't recover\n"); + exit(1); + } + handle_filter(&newline); + strbuf_release(&newline); + + /* skip to the next boundary */ + if (!find_boundary()) + return 0; + goto again; + } + + /* set some defaults */ + transfer_encoding = TE_DONTCARE; + strbuf_reset(&charset); + + /* slurp in this section's info */ + while (read_one_header_line(&line, fin)) + check_header(&line, p_hdr_data, 0); + + strbuf_release(&newline); + /* replenish line */ + if (strbuf_getline(&line, fin, '\n')) + return 0; + strbuf_addch(&line, '\n'); + return 1; +} + static void handle_body(void) { struct strbuf prev = STRBUF_INIT; -- 2.6.1-320-g86a1181 -- 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
[PATCH 26/26] mailinfo: libify the whole thing
With this, the builtin "git am" can hopefully make an internal call to the mailinfo() function without going through the run_command() interface, whose overhead is heavyweight compared to what mailinfo() itself does on some platforms. Signed-off-by: Junio C Hamano --- Makefile |1 + builtin/mailinfo.c | 1091 +--- mailinfo.c | 1058 ++ mailinfo.h | 41 ++ 4 files changed, 1101 insertions(+), 1090 deletions(-) create mode 100644 mailinfo.c create mode 100644 mailinfo.h diff --git a/Makefile b/Makefile index 8d5df7e..7dd3bff 100644 --- a/Makefile +++ b/Makefile @@ -726,6 +726,7 @@ LIB_OBJS += list-objects.o LIB_OBJS += ll-merge.o LIB_OBJS += lockfile.o LIB_OBJS += log-tree.o +LIB_OBJS += mailinfo.o LIB_OBJS += mailmap.o LIB_OBJS += match-trees.o LIB_OBJS += merge.o diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index eb9ff96..f6df274 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -6,1096 +6,7 @@ #include "builtin.h" #include "utf8.h" #include "strbuf.h" - -#define MAX_BOUNDARIES 5 - -struct mailinfo { - FILE *input; - FILE *output; - FILE *patchfile; - - struct strbuf name; - struct strbuf email; - int keep_subject; - int keep_non_patch_brackets_in_subject; - int add_message_id; - int use_scissors; - int use_inbody_headers; /* defaults to 1 */ - const char *metainfo_charset; - - struct strbuf *content[MAX_BOUNDARIES]; - struct strbuf **content_top; - struct strbuf charset; - char *message_id; - enum { - TE_DONTCARE, TE_QP, TE_BASE64 - } transfer_encoding; - int patch_lines; - int filter_stage; /* still reading log or are we copying patch? */ - int header_stage; /* still checking in-body headers? */ - struct strbuf **p_hdr_data; - struct strbuf **s_hdr_data; - - struct strbuf log_message; - int input_error; -}; - -#define MAX_HDR_PARSED 10 - -static void cleanup_space(struct strbuf *sb); - - -static void get_sane_name(struct strbuf *out, struct strbuf *name, struct strbuf *email) -{ - struct strbuf *src = name; - if (name->len < 3 || 60 < name->len || strchr(name->buf, '@') || - strchr(name->buf, '<') || strchr(name->buf, '>')) - src = email; - else if (name == out) - return; - strbuf_reset(out); - strbuf_addbuf(out, src); -} - -static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line) -{ - /* John Doe */ - - char *bra, *ket; - /* This is fallback, so do not bother if we already have an -* e-mail address. -*/ - if (mi->email.len) - return; - - bra = strchr(line->buf, '<'); - if (!bra) - return; - ket = strchr(bra, '>'); - if (!ket) - return; - - strbuf_reset(&mi->email); - strbuf_add(&mi->email, bra + 1, ket - bra - 1); - - strbuf_reset(&mi->name); - strbuf_add(&mi->name, line->buf, bra - line->buf); - strbuf_trim(&mi->name); - get_sane_name(&mi->name, &mi->name, &mi->email); -} - -static void handle_from(struct mailinfo *mi, const struct strbuf *from) -{ - char *at; - size_t el; - struct strbuf f; - - strbuf_init(&f, from->len); - strbuf_addbuf(&f, from); - - at = strchr(f.buf, '@'); - if (!at) { - parse_bogus_from(mi, from); - return; - } - - /* -* If we already have one email, don't take any confusing lines -*/ - if (mi->email.len && strchr(at + 1, '@')) { - strbuf_release(&f); - return; - } - - /* Pick up the string around '@', possibly delimited with <> -* pair; that is the email part. -*/ - while (at > f.buf) { - char c = at[-1]; - if (isspace(c)) - break; - if (c == '<') { - at[-1] = ' '; - break; - } - at--; - } - el = strcspn(at, " \n\t\r\v\f>"); - strbuf_reset(&mi->email); - strbuf_add(&mi->email, at, el); - strbuf_remove(&f, at - f.buf, el + (at[el] ? 1 : 0)); - - /* The remainder is name. It could be -* -* - "John Doe " (a), or -* - "john.doe@xz (John Doe)" (b), or -* - "John (zzz) Doe (Comment)" (c) -* -* but we have removed the email part, so -* -* - remove extra spaces which could stay after email (case 'c'), and -* - trim from both ends, possibly removing the () pair at the end -* (cases 'a' and 'b'). -*/ - cleanup_space(&f); - strbuf_trim(&f); - if (f.buf[0] == '(' && f.
[PATCH 07/26] mailinfo: move global "line" into mailinfo() function
The mailinfo() function is the only one that wants the "line_global" to be directly touchable. Note that handle_body() has to be passed this strbuf so that it sees the "first line of the input" after the loop in this function processes the headers. It feels a bit dirty that handle_body() then keeps reusing this strbuf to read more lines and does its processing, but that is how the code is structured now. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index c3c7d67..6c671fb 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -12,7 +12,6 @@ static FILE *cmitmsg, *patchfile, *fin, *fout; static int keep_subject; static int keep_non_patch_brackets_in_subject; static const char *metainfo_charset; -static struct strbuf line_global = STRBUF_INIT; static struct strbuf name = STRBUF_INIT; static struct strbuf email = STRBUF_INIT; static char *message_id; @@ -995,6 +994,8 @@ static void handle_info(void) static int mailinfo(FILE *in, FILE *out, const char *msg, const char *patch) { int peek; + struct strbuf line = STRBUF_INIT; + fin = in; fout = out; @@ -1019,10 +1020,10 @@ static int mailinfo(FILE *in, FILE *out, const char *msg, const char *patch) ungetc(peek, in); /* process the email header */ - while (read_one_header_line(&line_global, fin)) - check_header(&line_global, p_hdr_data, 1); + while (read_one_header_line(&line, fin)) + check_header(&line, p_hdr_data, 1); - handle_body(&line_global); + handle_body(&line); handle_info(); return 0; -- 2.6.1-320-g86a1181 -- 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
[PATCH 22/26] mailinfo: move content/content_top to struct mailinfo
Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 45 ++--- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index d38d716..4b9b1cc 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -7,6 +7,8 @@ #include "utf8.h" #include "strbuf.h" +#define MAX_BOUNDARIES 5 + struct mailinfo { FILE *input; FILE *output; @@ -21,6 +23,8 @@ struct mailinfo { int use_inbody_headers; /* defaults to 1 */ const char *metainfo_charset; + struct strbuf *content[MAX_BOUNDARIES]; + struct strbuf **content_top; struct strbuf charset; char *message_id; enum { @@ -36,7 +40,6 @@ struct mailinfo { }; #define MAX_HDR_PARSED 10 -#define MAX_BOUNDARIES 5 static void cleanup_space(struct strbuf *sb); @@ -181,10 +184,6 @@ static int slurp_attr(const char *line, const char *name, struct strbuf *attr) return 1; } -static struct strbuf *content[MAX_BOUNDARIES]; - -static struct strbuf **content_top = content; - static void handle_content_type(struct mailinfo *mi, struct strbuf *line) { struct strbuf *boundary = xmalloc(sizeof(struct strbuf)); @@ -192,11 +191,11 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line) if (slurp_attr(line->buf, "boundary=", boundary)) { strbuf_insert(boundary, 0, "--", 2); - if (++content_top >= &content[MAX_BOUNDARIES]) { + if (++mi->content_top >= &mi->content[MAX_BOUNDARIES]) { fprintf(stderr, "Too many boundaries to handle\n"); exit(1); } - *content_top = boundary; + *(mi->content_top) = boundary; boundary = NULL; } slurp_attr(line->buf, "charset=", &mi->charset); @@ -224,10 +223,12 @@ static void handle_content_transfer_encoding(struct mailinfo *mi, mi->transfer_encoding = TE_DONTCARE; } -static int is_multipart_boundary(const struct strbuf *line) +static int is_multipart_boundary(struct mailinfo *mi, const struct strbuf *line) { - return (((*content_top)->len <= line->len) && - !memcmp(line->buf, (*content_top)->buf, (*content_top)->len)); + struct strbuf *content_top = *(mi->content_top); + + return ((content_top->len <= line->len) && + !memcmp(line->buf, content_top->buf, content_top->len)); } static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject) @@ -825,7 +826,7 @@ static void handle_filter(struct mailinfo *mi, struct strbuf *line) static int find_boundary(struct mailinfo *mi, struct strbuf *line) { while (!strbuf_getline(line, mi->input, '\n')) { - if (*content_top && is_multipart_boundary(line)) + if (*(mi->content_top) && is_multipart_boundary(mi, line)) return 1; } return 0; @@ -837,18 +838,18 @@ static int handle_boundary(struct mailinfo *mi, struct strbuf *line) strbuf_addch(&newline, '\n'); again: - if (line->len >= (*content_top)->len + 2 && - !memcmp(line->buf + (*content_top)->len, "--", 2)) { + if (line->len >= (*(mi->content_top))->len + 2 && + !memcmp(line->buf + (*(mi->content_top))->len, "--", 2)) { /* we hit an end boundary */ /* pop the current boundary off the stack */ - strbuf_release(*content_top); - free(*content_top); - *content_top = NULL; + strbuf_release(*(mi->content_top)); + free(*(mi->content_top)); + *(mi->content_top) = NULL; /* technically won't happen as is_multipart_boundary() will fail first. But just in case.. */ - if (--content_top < content) { + if (--mi->content_top < mi->content) { fprintf(stderr, "Detected mismatched boundaries, " "can't recover\n"); exit(1); @@ -883,14 +884,14 @@ static void handle_body(struct mailinfo *mi, struct strbuf *line) struct strbuf prev = STRBUF_INIT; /* Skip up to the first boundary */ - if (*content_top) { + if (*(mi->content_top)) { if (!find_boundary(mi, line)) goto handle_body_out; } do { /* process any boundary lines */ - if (*content_top && is_multipart_boundary(line)) { + if (*(mi->content_top) && is_multipart_boundary(mi, line)) { /* flush any leftover */ if (prev.len) { handle_filter(mi, &prev); @@ -1057,6 +1058,7 @@ static void setup_mailinfo(struct mailinfo *mi) strbuf_init(&mi->log_message, 0); mi->h
[PATCH 19/26] mailinfo: move cmitmsg and patchfile to struct mailinfo
Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 28 ++-- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 256d04a..b591a2f 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -7,11 +7,11 @@ #include "utf8.h" #include "strbuf.h" -static FILE *cmitmsg, *patchfile; - struct mailinfo { FILE *input; FILE *output; + FILE *cmitmsg; + FILE *patchfile; struct strbuf name; struct strbuf email; @@ -775,9 +775,9 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) if (mi->use_scissors && is_scissors_line(line)) { int i; - if (fseek(cmitmsg, 0L, SEEK_SET)) + if (fseek(mi->cmitmsg, 0L, SEEK_SET)) die_errno("Could not rewind output message file"); - if (ftruncate(fileno(cmitmsg), 0)) + if (ftruncate(fileno(mi->cmitmsg), 0)) die_errno("Could not truncate output message file at scissors"); mi->header_stage = 1; @@ -795,19 +795,19 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) if (patchbreak(line)) { if (mi->message_id) - fprintf(cmitmsg, "Message-Id: %s\n", mi->message_id); - fclose(cmitmsg); - cmitmsg = NULL; + fprintf(mi->cmitmsg, "Message-Id: %s\n", mi->message_id); + fclose(mi->cmitmsg); + mi->cmitmsg = NULL; return 1; } - fputs(line->buf, cmitmsg); + fputs(line->buf, mi->cmitmsg); return 0; } static void handle_patch(struct mailinfo *mi, const struct strbuf *line) { - fwrite(line->buf, 1, line->len, patchfile); + fwrite(line->buf, 1, line->len, mi->patchfile); mi->patch_lines++; } @@ -1002,15 +1002,15 @@ static int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) int peek; struct strbuf line = STRBUF_INIT; - cmitmsg = fopen(msg, "w"); - if (!cmitmsg) { + mi->cmitmsg = fopen(msg, "w"); + if (!mi->cmitmsg) { perror(msg); return -1; } - patchfile = fopen(patch, "w"); - if (!patchfile) { + mi->patchfile = fopen(patch, "w"); + if (!mi->patchfile) { perror(patch); - fclose(cmitmsg); + fclose(mi->cmitmsg); return -1; } -- 2.6.1-320-g86a1181 -- 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
[PATCH 14/26] mailinfo: move use_scissors and use_inbody_headers to struct mailinfo
Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 23 +-- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 35e1ab9..5302c03 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -20,6 +20,8 @@ struct mailinfo { int keep_subject; int keep_non_patch_brackets_in_subject; int add_message_id; + int use_scissors; + int use_inbody_headers; /* defaults to 1 */ char *message_id; int patch_lines; @@ -34,8 +36,6 @@ static enum { static struct strbuf charset = STRBUF_INIT; static struct strbuf **p_hdr_data, **s_hdr_data; -static int use_scissors; -static int use_inbody_headers = 1; #define MAX_HDR_PARSED 10 #define MAX_BOUNDARIES 5 @@ -745,7 +745,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) return 0; } - if (use_inbody_headers && mi->header_stage) { + if (mi->use_inbody_headers && mi->header_stage) { int is_known_header = check_header(mi, line, s_hdr_data, 0); if (mi->header_stage == 2) { @@ -777,7 +777,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) if (metainfo_charset) convert_to_utf8(line, charset.buf); - if (use_scissors && is_scissors_line(line)) { + if (mi->use_scissors && is_scissors_line(line)) { int i; if (fseek(cmitmsg, 0L, SEEK_SET)) die_errno("Could not rewind output message file"); @@ -1036,12 +1036,14 @@ static int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) return 0; } -static int git_mailinfo_config(const char *var, const char *value, void *unused) +static int git_mailinfo_config(const char *var, const char *value, void *mi_) { + struct mailinfo *mi = mi_; + if (!starts_with(var, "mailinfo.")) - return git_default_config(var, value, unused); + return git_default_config(var, value, NULL); if (!strcmp(var, "mailinfo.scissors")) { - use_scissors = git_config_bool(var, value); + mi->use_scissors = git_config_bool(var, value); return 0; } /* perhaps others here */ @@ -1054,6 +1056,7 @@ static void setup_mailinfo(struct mailinfo *mi) strbuf_init(&mi->name, 0); strbuf_init(&mi->email, 0); mi->header_stage = 1; + mi->use_inbody_headers = 1; git_config(git_mailinfo_config, &mi); } @@ -1087,11 +1090,11 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix) else if (starts_with(argv[1], "--encoding=")) metainfo_charset = argv[1] + 11; else if (!strcmp(argv[1], "--scissors")) - use_scissors = 1; + mi.use_scissors = 1; else if (!strcmp(argv[1], "--no-scissors")) - use_scissors = 0; + mi.use_scissors = 0; else if (!strcmp(argv[1], "--no-inbody-headers")) - use_inbody_headers = 0; + mi.use_inbody_headers = 0; else usage(mailinfo_usage); argc--; argv++; -- 2.6.1-320-g86a1181 -- 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
[PATCH 13/26] mailinfo: move add_message_id and message_id to struct mailinfo
This requires us to pass the structure into check_header() codepath. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 30 -- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 163032e..35e1ab9 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -19,12 +19,13 @@ struct mailinfo { struct strbuf email; int keep_subject; int keep_non_patch_brackets_in_subject; + int add_message_id; + char *message_id; int patch_lines; int filter_stage; /* still reading log or are we copying patch? */ int header_stage; /* still checking in-body headers? */ }; -static char *message_id; static enum { TE_DONTCARE, TE_QP, TE_BASE64 @@ -34,7 +35,6 @@ static struct strbuf charset = STRBUF_INIT; static struct strbuf **p_hdr_data, **s_hdr_data; static int use_scissors; -static int add_message_id; static int use_inbody_headers = 1; #define MAX_HDR_PARSED 10 @@ -209,10 +209,10 @@ static void handle_content_type(struct strbuf *line) } } -static void handle_message_id(const struct strbuf *line) +static void handle_message_id(struct mailinfo *mi, const struct strbuf *line) { - if (add_message_id) - message_id = strdup(line->buf); + if (mi->add_message_id) + mi->message_id = strdup(line->buf); } static void handle_content_transfer_encoding(const struct strbuf *line) @@ -321,11 +321,13 @@ static int is_format_patch_separator(const char *line, int len) return !memcmp(SAMPLE + (cp - line), cp, strlen(SAMPLE) - (cp - line)); } -static int check_header(const struct strbuf *line, - struct strbuf *hdr_data[], int overwrite) +static int check_header(struct mailinfo *mi, + const struct strbuf *line, + struct strbuf *hdr_data[], int overwrite) { int i, ret = 0, len; struct strbuf sb = STRBUF_INIT; + /* search for the interesting parts */ for (i = 0; header[i]; i++) { int len = strlen(header[i]); @@ -363,7 +365,7 @@ static int check_header(const struct strbuf *line, len = strlen("Message-Id: "); strbuf_add(&sb, line->buf + len, line->len - len); decode_header(&sb); - handle_message_id(&sb); + handle_message_id(mi, &sb); ret = 1; goto check_header_out; } @@ -744,7 +746,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) } if (use_inbody_headers && mi->header_stage) { - int is_known_header = check_header(line, s_hdr_data, 0); + int is_known_header = check_header(mi, line, s_hdr_data, 0); if (mi->header_stage == 2) { /* @@ -796,8 +798,8 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) } if (patchbreak(line)) { - if (message_id) - fprintf(cmitmsg, "Message-Id: %s\n", message_id); + if (mi->message_id) + fprintf(cmitmsg, "Message-Id: %s\n", mi->message_id); fclose(cmitmsg); cmitmsg = NULL; return 1; @@ -872,7 +874,7 @@ again: /* slurp in this section's info */ while (read_one_header_line(line, mi->input)) - check_header(line, p_hdr_data, 0); + check_header(mi, line, p_hdr_data, 0); strbuf_release(&newline); /* replenish line */ @@ -1026,7 +1028,7 @@ static int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) /* process the email header */ while (read_one_header_line(&line, mi->input)) - check_header(&line, p_hdr_data, 1); + check_header(mi, &line, p_hdr_data, 1); handle_body(mi, &line); handle_info(mi); @@ -1077,7 +1079,7 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix) else if (!strcmp(argv[1], "-b")) mi.keep_non_patch_brackets_in_subject = 1; else if (!strcmp(argv[1], "-m") || !strcmp(argv[1], "--message-id")) - add_message_id = 1; + mi.add_message_id = 1; else if (!strcmp(argv[1], "-u")) metainfo_charset = def_charset; else if (!strcmp(argv[1], "-n")) -- 2.6.1-320-g86a1181 -- 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
[PATCH 11/26] mailinfo: move filter/header stage to struct mailinfo
Earlier we got rid of two function-scope static variables that kept track of the states of helper functions by making them extra arguments that are passed throughout the callchain. Now we have a convenient place to store and pass them around in the form of "struct mailinfo", change them into two fields in the struct. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 50 ++ 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index a9da338..7e39769 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -19,6 +19,9 @@ struct mailinfo { struct strbuf email; int keep_subject; int keep_non_patch_brackets_in_subject; + + int filter_stage; /* still reading log or are we copying patch? */ + int header_stage; /* still checking in-body headers? */ }; static char *message_id; @@ -28,6 +31,7 @@ static enum { static struct strbuf charset = STRBUF_INIT; static int patch_lines; + static struct strbuf **p_hdr_data, **s_hdr_data; static int use_scissors; static int add_message_id; @@ -718,7 +722,7 @@ static int is_scissors_line(const struct strbuf *line) gap * 2 < perforation); } -static int handle_commit_msg(struct strbuf *line, int *still_looking) +static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) { /* * Are we still scanning and discarding in-body headers? @@ -731,7 +735,7 @@ static int handle_commit_msg(struct strbuf *line, int *still_looking) return 0; is_empty_line = (!line->len || (line->len == 1 && line->buf[0] == '\n')); - if (*still_looking == 1) { + if (mi->header_stage == 1) { /* * Haven't seen a known in-body header; discard an empty line. */ @@ -739,33 +743,33 @@ static int handle_commit_msg(struct strbuf *line, int *still_looking) return 0; } - if (use_inbody_headers && *still_looking) { + if (use_inbody_headers && mi->header_stage) { int is_known_header = check_header(line, s_hdr_data, 0); - if (*still_looking == 2) { + if (mi->header_stage == 2) { /* * an empty line after the in-body header block, * or a line obviously not an attempt to invent * an unsupported in-body header. */ if (is_empty_line || !is_rfc2822_header(line)) - *still_looking = 0; + mi->header_stage = 0; if (is_empty_line) return 0; /* otherwise do not discard the line, but keep going */ } else if (is_known_header) { - *still_looking = 2; - } else if (*still_looking != 2) { - *still_looking = 0; + mi->header_stage = 2; + } else if (mi->header_stage != 2) { + mi->header_stage = 0; } - if (*still_looking) + if (mi->header_stage) return 0; } else /* Only trim the first (blank) line of the commit message * when ignoring in-body headers. */ - *still_looking = 0; + mi->header_stage = 0; /* normalize the log message to UTF-8. */ if (metainfo_charset) @@ -777,7 +781,7 @@ static int handle_commit_msg(struct strbuf *line, int *still_looking) die_errno("Could not rewind output message file"); if (ftruncate(fileno(cmitmsg), 0)) die_errno("Could not truncate output message file at scissors"); - *still_looking = 1; + mi->header_stage = 1; /* * We may have already read "secondary headers"; purge @@ -809,13 +813,13 @@ static void handle_patch(const struct strbuf *line) patch_lines++; } -static void handle_filter(struct strbuf *line, int *filter_stage, int *header_stage) +static void handle_filter(struct mailinfo *mi, struct strbuf *line) { - switch (*filter_stage) { + switch (mi->filter_stage) { case 0: - if (!handle_commit_msg(line, header_stage)) + if (!handle_commit_msg(mi, line)) break; - (*filter_stage)++; + mi->filter_stage++; case 1: handle_patch(line); break; @@ -831,8 +835,7 @@ static int find_boundary(struct mailinfo *mi, struct strbuf *line) return 0; } -static int handle_boundary(struct mailinfo *mi, struct strbuf *line, - int *filter_stage, int *header_s
[PATCH 24/26] mailinfo: handle charset conversion errors in the caller
Instead of dying in convert_to_utf8(), just report an error and let the callers handle it. Between the two callers: - decode_header() knows how to handle malformed line by reporting the breakage to the caller, so let it follow the pattern it already knows about; - handle_commit_msg() doesn't cope with a malformed line well, so die there for now. We'll lift this even higher in later changes in this series. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 19 +++ 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 6452a4c..f14c504 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -521,21 +521,22 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg) return out; } -static void convert_to_utf8(struct mailinfo *mi, - struct strbuf *line, const char *charset) +static int convert_to_utf8(struct mailinfo *mi, + struct strbuf *line, const char *charset) { char *out; if (!mi->metainfo_charset || !charset || !*charset) - return; + return 0; if (same_encoding(mi->metainfo_charset, charset)) - return; + return 0; out = reencode_string(line->buf, mi->metainfo_charset, charset); if (!out) - die("cannot convert from %s to %s", - charset, mi->metainfo_charset); + return error("cannot convert from %s to %s", +charset, mi->metainfo_charset); strbuf_attach(line, out, strlen(out), strlen(out)); + return 0; } static int decode_header(struct mailinfo *mi, struct strbuf *it) @@ -602,7 +603,8 @@ static int decode_header(struct mailinfo *mi, struct strbuf *it) dec = decode_q_segment(&piecebuf, 1); break; } - convert_to_utf8(mi, dec, charset_q.buf); + if (convert_to_utf8(mi, dec, charset_q.buf)) + goto release_return; strbuf_addbuf(&outbuf, dec); strbuf_release(dec); @@ -778,7 +780,8 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) mi->header_stage = 0; /* normalize the log message to UTF-8. */ - convert_to_utf8(mi, line, mi->charset.buf); + if (convert_to_utf8(mi, line, mi->charset.buf)) + exit(128); if (mi->use_scissors && is_scissors_line(line)) { int i; -- 2.6.1-320-g86a1181 -- 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
[PATCH 12/26] mailinfo: move patch_lines to struct mailinfo
This one is trivial thanks to previous steps that started passing the structure throughout the input codepaths. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 7e39769..163032e 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -20,6 +20,7 @@ struct mailinfo { int keep_subject; int keep_non_patch_brackets_in_subject; + int patch_lines; int filter_stage; /* still reading log or are we copying patch? */ int header_stage; /* still checking in-body headers? */ }; @@ -30,7 +31,6 @@ static enum { } transfer_encoding; static struct strbuf charset = STRBUF_INIT; -static int patch_lines; static struct strbuf **p_hdr_data, **s_hdr_data; static int use_scissors; @@ -807,10 +807,10 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) return 0; } -static void handle_patch(const struct strbuf *line) +static void handle_patch(struct mailinfo *mi, const struct strbuf *line) { fwrite(line->buf, 1, line->len, patchfile); - patch_lines++; + mi->patch_lines++; } static void handle_filter(struct mailinfo *mi, struct strbuf *line) @@ -821,7 +821,7 @@ static void handle_filter(struct mailinfo *mi, struct strbuf *line) break; mi->filter_stage++; case 1: - handle_patch(line); + handle_patch(mi, line); break; } } @@ -973,7 +973,7 @@ static void handle_info(struct mailinfo *mi) for (i = 0; header[i]; i++) { /* only print inbody headers if we output a patch file */ - if (patch_lines && s_hdr_data[i]) + if (mi->patch_lines && s_hdr_data[i]) hdr = s_hdr_data[i]; else if (p_hdr_data[i]) hdr = p_hdr_data[i]; -- 2.6.1-320-g86a1181 -- 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
[PATCH 15/26] mailinfo: move metainfo_charset to struct mailinfo
This requires us to pass the struct down to decode_header() and convert_to_utf8() callchain. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 40 +++- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 5302c03..7e01efa 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -9,8 +9,6 @@ static FILE *cmitmsg, *patchfile; -static const char *metainfo_charset; - struct mailinfo { FILE *input; FILE *output; @@ -22,6 +20,7 @@ struct mailinfo { int add_message_id; int use_scissors; int use_inbody_headers; /* defaults to 1 */ + const char *metainfo_charset; char *message_id; int patch_lines; @@ -293,7 +292,7 @@ static void cleanup_space(struct strbuf *sb) } } -static void decode_header(struct strbuf *line); +static void decode_header(struct mailinfo *mi, struct strbuf *line); static const char *header[MAX_HDR_PARSED] = { "From","Subject","Date", }; @@ -336,7 +335,7 @@ static int check_header(struct mailinfo *mi, * normalize the meta information to utf8. */ strbuf_add(&sb, line->buf + len + 2, line->len - len - 2); - decode_header(&sb); + decode_header(mi, &sb); handle_header(&hdr_data[i], &sb); ret = 1; goto check_header_out; @@ -347,7 +346,7 @@ static int check_header(struct mailinfo *mi, if (cmp_header(line, "Content-Type")) { len = strlen("Content-Type: "); strbuf_add(&sb, line->buf + len, line->len - len); - decode_header(&sb); + decode_header(mi, &sb); strbuf_insert(&sb, 0, "Content-Type: ", len); handle_content_type(&sb); ret = 1; @@ -356,7 +355,7 @@ static int check_header(struct mailinfo *mi, if (cmp_header(line, "Content-Transfer-Encoding")) { len = strlen("Content-Transfer-Encoding: "); strbuf_add(&sb, line->buf + len, line->len - len); - decode_header(&sb); + decode_header(mi, &sb); handle_content_transfer_encoding(&sb); ret = 1; goto check_header_out; @@ -364,7 +363,7 @@ static int check_header(struct mailinfo *mi, if (cmp_header(line, "Message-Id")) { len = strlen("Message-Id: "); strbuf_add(&sb, line->buf + len, line->len - len); - decode_header(&sb); + decode_header(mi, &sb); handle_message_id(mi, &sb); ret = 1; goto check_header_out; @@ -520,23 +519,24 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg) return out; } -static void convert_to_utf8(struct strbuf *line, const char *charset) +static void convert_to_utf8(struct mailinfo *mi, + struct strbuf *line, const char *charset) { char *out; - if (!charset || !*charset) + if (!mi->metainfo_charset || !charset || !*charset) return; - if (same_encoding(metainfo_charset, charset)) + if (same_encoding(mi->metainfo_charset, charset)) return; - out = reencode_string(line->buf, metainfo_charset, charset); + out = reencode_string(line->buf, mi->metainfo_charset, charset); if (!out) die("cannot convert from %s to %s", - charset, metainfo_charset); + charset, mi->metainfo_charset); strbuf_attach(line, out, strlen(out), strlen(out)); } -static void decode_header(struct strbuf *it) +static void decode_header(struct mailinfo *mi, struct strbuf *it) { char *in, *ep, *cp; struct strbuf outbuf = STRBUF_INIT, *dec; @@ -599,8 +599,7 @@ static void decode_header(struct strbuf *it) dec = decode_q_segment(&piecebuf, 1); break; } - if (metainfo_charset) - convert_to_utf8(dec, charset_q.buf); + convert_to_utf8(mi, dec, charset_q.buf); strbuf_addbuf(&outbuf, dec); strbuf_release(dec); @@ -774,8 +773,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) mi->header_stage = 0; /* normalize the log message to UTF-8. */ - if (metainfo_charset) - convert_to_utf8(line, charset.buf); + convert_to_utf8(mi, line, charset.buf); if (mi->use_scissors && is_scissors_line(line)) { int i; @@ -1074,7 +1072,7 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix) setup_mailinfo(&mi); def_charset = get_commit_output_encoding(); - metainfo_charset = def_charset; +
[PATCH 25/26] mailinfo: remove calls to exit() and die() deep in the callchain
The top-level mailinfo() would instead punt when the code in the deeper part of the callchain detects an unrecoverable error in the input. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 36 +--- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index f14c504..eb9ff96 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -37,6 +37,7 @@ struct mailinfo { struct strbuf **s_hdr_data; struct strbuf log_message; + int input_error; }; #define MAX_HDR_PARSED 10 @@ -192,8 +193,10 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line) if (slurp_attr(line->buf, "boundary=", boundary)) { strbuf_insert(boundary, 0, "--", 2); if (++mi->content_top >= &mi->content[MAX_BOUNDARIES]) { - fprintf(stderr, "Too many boundaries to handle\n"); - exit(1); + error("Too many boundaries to handle"); + mi->input_error = -1; + mi->content_top = mi->content + MAX_BOUNDARIES - 1; + return; } *(mi->content_top) = boundary; boundary = NULL; @@ -532,9 +535,11 @@ static int convert_to_utf8(struct mailinfo *mi, if (same_encoding(mi->metainfo_charset, charset)) return 0; out = reencode_string(line->buf, mi->metainfo_charset, charset); - if (!out) + if (!out) { + mi->input_error = -1; return error("cannot convert from %s to %s", charset, mi->metainfo_charset); + } strbuf_attach(line, out, strlen(out), strlen(out)); return 0; } @@ -781,7 +786,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) /* normalize the log message to UTF-8. */ if (convert_to_utf8(mi, line, mi->charset.buf)) - exit(128); + return 0; /* the caller will reject this input */ if (mi->use_scissors && is_scissors_line(line)) { int i; @@ -820,6 +825,9 @@ static void handle_patch(struct mailinfo *mi, const struct strbuf *line) static void handle_filter(struct mailinfo *mi, struct strbuf *line) { + if (mi->input_error) + return; + switch (mi->filter_stage) { case 0: if (!handle_commit_msg(mi, line)) @@ -858,12 +866,15 @@ again: will fail first. But just in case.. */ if (--mi->content_top < mi->content) { - fprintf(stderr, "Detected mismatched boundaries, " - "can't recover\n"); - exit(1); + error("Detected mismatched boundaries, can't recover"); + mi->input_error = -1; + mi->content_top = mi->content; + return 0; } handle_filter(mi, &newline); strbuf_release(&newline); + if (mi->input_error) + return 0; /* punt to the end */ /* skip to the next boundary */ if (!find_boundary(mi, line)) @@ -948,6 +959,8 @@ static void handle_body(struct mailinfo *mi, struct strbuf *line) handle_filter(mi, line); } + if (mi->input_error) + break; } while (!strbuf_getwholeline(line, mi->input, '\n')); handle_body_out: @@ -1035,12 +1048,13 @@ static int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) check_header(mi, &line, mi->p_hdr_data, 1); handle_body(mi, &line); - handle_info(mi); - - fwrite(mi->log_message.buf, 1, mi->log_message.len, cmitmsg); + if (!mi->input_error) { + handle_info(mi); + fwrite(mi->log_message.buf, 1, mi->log_message.len, cmitmsg); + } fclose(cmitmsg); - return 0; + return mi->input_error; } static int git_mailinfo_config(const char *var, const char *value, void *mi_) -- 2.6.1-320-g86a1181 -- 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
[PATCH 18/26] mailinfo: handle_commit_msg() shouldn't be called after finding patchbreak
There is a strange "if (!cmitmsg) return 0" at the very beginning of handle_commit_msg(), but the condition should never trigger, because: * The only place cmitmsg is set to NULL is after this function sees a patch break, closes the FILE * to write the commit log message and returns 1. This function returns non-zero only from that codepath. * The caller of this function, upon seeing a non-zero return, increments filter_stage, starts treating the input as patch text and will never call handle_commit_msg() again. Replace it with an assert(!mi->filter_stage). Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index a51b2c5..256d04a 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -731,8 +731,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) */ int is_empty_line; - if (!cmitmsg) - return 0; + assert(!mi->filter_stage); is_empty_line = (!line->len || (line->len == 1 && line->buf[0] == '\n')); if (mi->header_stage == 1) { -- 2.6.1-320-g86a1181 -- 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
[PATCH 21/26] mailinfo: keep the parsed log message in a strbuf
When mailinfo() is eventually libified, the calling "git am" still will have to write out the log message in the "msg" file for hooks and other users of the information, but at least it does not have to reopen and reread what was written back if the function kept it in a strbuf so that the caller can read it from there. This also removes the need for seeking and truncating the output file when we see a scissors mark in the input, which in turn reduces two callsites of die_errno(). Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 41 - 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index c9629c8..d38d716 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -10,7 +10,6 @@ struct mailinfo { FILE *input; FILE *output; - FILE *cmitmsg; FILE *patchfile; struct strbuf name; @@ -32,6 +31,8 @@ struct mailinfo { int header_stage; /* still checking in-body headers? */ struct strbuf **p_hdr_data; struct strbuf **s_hdr_data; + + struct strbuf log_message; }; #define MAX_HDR_PARSED 10 @@ -775,10 +776,8 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) if (mi->use_scissors && is_scissors_line(line)) { int i; - if (fseek(mi->cmitmsg, 0L, SEEK_SET)) - die_errno("Could not rewind output message file"); - if (ftruncate(fileno(mi->cmitmsg), 0)) - die_errno("Could not truncate output message file at scissors"); + + strbuf_setlen(&mi->log_message, 0); mi->header_stage = 1; /* @@ -795,13 +794,12 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) if (patchbreak(line)) { if (mi->message_id) - fprintf(mi->cmitmsg, "Message-Id: %s\n", mi->message_id); - fclose(mi->cmitmsg); - mi->cmitmsg = NULL; + strbuf_addf(&mi->log_message, + "Message-Id: %s\n", mi->message_id); return 1; } - fputs(line->buf, mi->cmitmsg); + strbuf_addbuf(&mi->log_message, line); return 0; } @@ -999,18 +997,19 @@ static void handle_info(struct mailinfo *mi) static int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) { + FILE *cmitmsg; int peek; struct strbuf line = STRBUF_INIT; - mi->cmitmsg = fopen(msg, "w"); - if (!mi->cmitmsg) { + cmitmsg = fopen(msg, "w"); + if (!cmitmsg) { perror(msg); return -1; } mi->patchfile = fopen(patch, "w"); if (!mi->patchfile) { perror(patch); - fclose(mi->cmitmsg); + fclose(cmitmsg); return -1; } @@ -1029,6 +1028,9 @@ static int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) handle_body(mi, &line); handle_info(mi); + fwrite(mi->log_message.buf, 1, mi->log_message.len, cmitmsg); + fclose(cmitmsg); + return 0; } @@ -1052,11 +1054,20 @@ static void setup_mailinfo(struct mailinfo *mi) strbuf_init(&mi->name, 0); strbuf_init(&mi->email, 0); strbuf_init(&mi->charset, 0); + strbuf_init(&mi->log_message, 0); mi->header_stage = 1; mi->use_inbody_headers = 1; git_config(git_mailinfo_config, &mi); } +static void clear_mailinfo(struct mailinfo *mi) +{ + strbuf_release(&mi->name); + strbuf_release(&mi->email); + strbuf_release(&mi->charset); + strbuf_release(&mi->log_message); +} + static const char mailinfo_usage[] = "git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding= | -n] [--scissors | --no-scissors] < mail >info"; @@ -1064,6 +1075,7 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix) { const char *def_charset; struct mailinfo mi; + int status; /* NEEDSWORK: might want to do the optional .git/ directory * discovery @@ -1102,5 +1114,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix) mi.input = stdin; mi.output = stdout; - return !!mailinfo(&mi, argv[1], argv[2]); + status = !!mailinfo(&mi, argv[1], argv[2]); + clear_mailinfo(&mi); + + return status; } -- 2.6.1-320-g86a1181 -- 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
[PATCH 16/26] mailinfo: move transfer_encoding to struct mailinfo
Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 27 ++- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 7e01efa..fbfa27e 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -23,14 +23,14 @@ struct mailinfo { const char *metainfo_charset; char *message_id; + enum { + TE_DONTCARE, TE_QP, TE_BASE64 + } transfer_encoding; int patch_lines; int filter_stage; /* still reading log or are we copying patch? */ int header_stage; /* still checking in-body headers? */ }; -static enum { - TE_DONTCARE, TE_QP, TE_BASE64 -} transfer_encoding; static struct strbuf charset = STRBUF_INIT; @@ -214,14 +214,15 @@ static void handle_message_id(struct mailinfo *mi, const struct strbuf *line) mi->message_id = strdup(line->buf); } -static void handle_content_transfer_encoding(const struct strbuf *line) +static void handle_content_transfer_encoding(struct mailinfo *mi, +const struct strbuf *line) { if (strcasestr(line->buf, "base64")) - transfer_encoding = TE_BASE64; + mi->transfer_encoding = TE_BASE64; else if (strcasestr(line->buf, "quoted-printable")) - transfer_encoding = TE_QP; + mi->transfer_encoding = TE_QP; else - transfer_encoding = TE_DONTCARE; + mi->transfer_encoding = TE_DONTCARE; } static int is_multipart_boundary(const struct strbuf *line) @@ -356,7 +357,7 @@ static int check_header(struct mailinfo *mi, len = strlen("Content-Transfer-Encoding: "); strbuf_add(&sb, line->buf + len, line->len - len); decode_header(mi, &sb); - handle_content_transfer_encoding(&sb); + handle_content_transfer_encoding(mi, &sb); ret = 1; goto check_header_out; } @@ -615,11 +616,11 @@ release_return: strbuf_release(&piecebuf); } -static void decode_transfer_encoding(struct strbuf *line) +static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line) { struct strbuf *ret; - switch (transfer_encoding) { + switch (mi->transfer_encoding) { case TE_QP: ret = decode_q_segment(line, 0); break; @@ -867,7 +868,7 @@ again: } /* set some defaults */ - transfer_encoding = TE_DONTCARE; + mi->transfer_encoding = TE_DONTCARE; strbuf_reset(&charset); /* slurp in this section's info */ @@ -905,9 +906,9 @@ static void handle_body(struct mailinfo *mi, struct strbuf *line) } /* Unwrap transfer encoding */ - decode_transfer_encoding(line); + decode_transfer_encoding(mi, line); - switch (transfer_encoding) { + switch (mi->transfer_encoding) { case TE_BASE64: case TE_QP: { -- 2.6.1-320-g86a1181 -- 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
[PATCH 17/26] mailinfo: move charset to struct mailinfo
Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 15 +++ 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index fbfa27e..a51b2c5 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -22,6 +22,7 @@ struct mailinfo { int use_inbody_headers; /* defaults to 1 */ const char *metainfo_charset; + struct strbuf charset; char *message_id; enum { TE_DONTCARE, TE_QP, TE_BASE64 @@ -31,9 +32,6 @@ struct mailinfo { int header_stage; /* still checking in-body headers? */ }; - -static struct strbuf charset = STRBUF_INIT; - static struct strbuf **p_hdr_data, **s_hdr_data; #define MAX_HDR_PARSED 10 @@ -186,7 +184,7 @@ static struct strbuf *content[MAX_BOUNDARIES]; static struct strbuf **content_top = content; -static void handle_content_type(struct strbuf *line) +static void handle_content_type(struct mailinfo *mi, struct strbuf *line) { struct strbuf *boundary = xmalloc(sizeof(struct strbuf)); strbuf_init(boundary, line->len); @@ -200,7 +198,7 @@ static void handle_content_type(struct strbuf *line) *content_top = boundary; boundary = NULL; } - slurp_attr(line->buf, "charset=", &charset); + slurp_attr(line->buf, "charset=", &mi->charset); if (boundary) { strbuf_release(boundary); @@ -349,7 +347,7 @@ static int check_header(struct mailinfo *mi, strbuf_add(&sb, line->buf + len, line->len - len); decode_header(mi, &sb); strbuf_insert(&sb, 0, "Content-Type: ", len); - handle_content_type(&sb); + handle_content_type(mi, &sb); ret = 1; goto check_header_out; } @@ -774,7 +772,7 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line) mi->header_stage = 0; /* normalize the log message to UTF-8. */ - convert_to_utf8(mi, line, charset.buf); + convert_to_utf8(mi, line, mi->charset.buf); if (mi->use_scissors && is_scissors_line(line)) { int i; @@ -869,7 +867,7 @@ again: /* set some defaults */ mi->transfer_encoding = TE_DONTCARE; - strbuf_reset(&charset); + strbuf_reset(&mi->charset); /* slurp in this section's info */ while (read_one_header_line(line, mi->input)) @@ -1054,6 +1052,7 @@ static void setup_mailinfo(struct mailinfo *mi) memset(mi, 0, sizeof(*mi)); strbuf_init(&mi->name, 0); strbuf_init(&mi->email, 0); + strbuf_init(&mi->charset, 0); mi->header_stage = 1; mi->use_inbody_headers = 1; git_config(git_mailinfo_config, &mi); -- 2.6.1-320-g86a1181 -- 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
[PATCH 06/26] mailinfo: always pass "line" as an argument
Some functions in this module accessed the global "struct strbuf line" while many others used a strbuf passed as an argument. Convert the former to ensure that nobody deeper in the callchains relies on the global one. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 48 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 5a74811..c3c7d67 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -12,7 +12,7 @@ static FILE *cmitmsg, *patchfile, *fin, *fout; static int keep_subject; static int keep_non_patch_brackets_in_subject; static const char *metainfo_charset; -static struct strbuf line = STRBUF_INIT; +static struct strbuf line_global = STRBUF_INIT; static struct strbuf name = STRBUF_INIT; static struct strbuf email = STRBUF_INIT; static char *message_id; @@ -817,23 +817,23 @@ static void handle_filter(struct strbuf *line, int *filter_stage, int *header_st } } -static int find_boundary(void) +static int find_boundary(struct strbuf *line) { - while (!strbuf_getline(&line, fin, '\n')) { - if (*content_top && is_multipart_boundary(&line)) + while (!strbuf_getline(line, fin, '\n')) { + if (*content_top && is_multipart_boundary(line)) return 1; } return 0; } -static int handle_boundary(int *filter_stage, int *header_stage) +static int handle_boundary(struct strbuf *line, int *filter_stage, int *header_stage) { struct strbuf newline = STRBUF_INIT; strbuf_addch(&newline, '\n'); again: - if (line.len >= (*content_top)->len + 2 && - !memcmp(line.buf + (*content_top)->len, "--", 2)) { + if (line->len >= (*content_top)->len + 2 && + !memcmp(line->buf + (*content_top)->len, "--", 2)) { /* we hit an end boundary */ /* pop the current boundary off the stack */ strbuf_release(*content_top); @@ -852,7 +852,7 @@ again: strbuf_release(&newline); /* skip to the next boundary */ - if (!find_boundary()) + if (!find_boundary(line)) return 0; goto again; } @@ -862,18 +862,18 @@ again: strbuf_reset(&charset); /* slurp in this section's info */ - while (read_one_header_line(&line, fin)) - check_header(&line, p_hdr_data, 0); + while (read_one_header_line(line, fin)) + check_header(line, p_hdr_data, 0); strbuf_release(&newline); /* replenish line */ - if (strbuf_getline(&line, fin, '\n')) + if (strbuf_getline(line, fin, '\n')) return 0; - strbuf_addch(&line, '\n'); + strbuf_addch(line, '\n'); return 1; } -static void handle_body(void) +static void handle_body(struct strbuf *line) { struct strbuf prev = STRBUF_INIT; int filter_stage = 0; @@ -881,24 +881,24 @@ static void handle_body(void) /* Skip up to the first boundary */ if (*content_top) { - if (!find_boundary()) + if (!find_boundary(line)) goto handle_body_out; } do { /* process any boundary lines */ - if (*content_top && is_multipart_boundary(&line)) { + if (*content_top && is_multipart_boundary(line)) { /* flush any leftover */ if (prev.len) { handle_filter(&prev, &filter_stage, &header_stage); strbuf_reset(&prev); } - if (!handle_boundary(&filter_stage, &header_stage)) + if (!handle_boundary(line, &filter_stage, &header_stage)) goto handle_body_out; } /* Unwrap transfer encoding */ - decode_transfer_encoding(&line); + decode_transfer_encoding(line); switch (transfer_encoding) { case TE_BASE64: @@ -907,7 +907,7 @@ static void handle_body(void) struct strbuf **lines, **it, *sb; /* Prepend any previous partial lines */ - strbuf_insert(&line, 0, prev.buf, prev.len); + strbuf_insert(line, 0, prev.buf, prev.len); strbuf_reset(&prev); /* @@ -915,7 +915,7 @@ static void handle_body(void) * multiple new lines. Pass only one chunk * at a time to handle_filter() */ - lines = strbuf_split(&line, '\n'); + lines = strbuf_split(line, '\n'); for (it = lines; (sb = *it); it++) {
[PATCH 02/26] mailinfo: fix for off-by-one error in boundary stack
We pre-increment the pointer that we will use to store something at, so the pointer is already beyond the end of the array if it points at content[MAX_BOUNDARIES]. Signed-off-by: Junio C Hamano --- As always, I am very bad at checking and fixing off-by-one errors. A few extra sets of eyeballs are very much appreciated. --- builtin/mailinfo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 330bef4..2742d0d 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -185,7 +185,7 @@ static void handle_content_type(struct strbuf *line) if (slurp_attr(line->buf, "boundary=", boundary)) { strbuf_insert(boundary, 0, "--", 2); - if (++content_top > &content[MAX_BOUNDARIES]) { + if (++content_top >= &content[MAX_BOUNDARIES]) { fprintf(stderr, "Too many boundaries to handle\n"); exit(1); } -- 2.6.1-320-g86a1181 -- 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
[PATCH 03/26] mailinfo: fold decode_header_bq() into decode_header()
In olden days we might have wanted to behave differently in decode_header() if the header line was encoded with RFC2047, but we apparently do not do so, hence this helper function can go, together with its return value. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 23 +++ 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 2742d0d..3b015a5 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -525,19 +525,17 @@ static void convert_to_utf8(struct strbuf *line, const char *charset) strbuf_attach(line, out, strlen(out), strlen(out)); } -static int decode_header_bq(struct strbuf *it) +static void decode_header(struct strbuf *it) { char *in, *ep, *cp; struct strbuf outbuf = STRBUF_INIT, *dec; struct strbuf charset_q = STRBUF_INIT, piecebuf = STRBUF_INIT; - int rfc2047 = 0; in = it->buf; while (in - it->buf <= it->len && (ep = strstr(in, "=?")) != NULL) { int encoding; strbuf_reset(&charset_q); strbuf_reset(&piecebuf); - rfc2047 = 1; if (in != ep) { /* @@ -567,22 +565,22 @@ static int decode_header_bq(struct strbuf *it) ep += 2; if (ep - it->buf >= it->len || !(cp = strchr(ep, '?'))) - goto decode_header_bq_out; + goto release_return; if (cp + 3 - it->buf > it->len) - goto decode_header_bq_out; + goto release_return; strbuf_add(&charset_q, ep, cp - ep); encoding = cp[1]; if (!encoding || cp[2] != '?') - goto decode_header_bq_out; + goto release_return; ep = strstr(cp + 3, "?="); if (!ep) - goto decode_header_bq_out; + goto release_return; strbuf_add(&piecebuf, cp + 3, ep - cp - 3); switch (tolower(encoding)) { default: - goto decode_header_bq_out; + goto release_return; case 'b': dec = decode_b_segment(&piecebuf); break; @@ -601,17 +599,10 @@ static int decode_header_bq(struct strbuf *it) strbuf_addstr(&outbuf, in); strbuf_reset(it); strbuf_addbuf(it, &outbuf); -decode_header_bq_out: +release_return: strbuf_release(&outbuf); strbuf_release(&charset_q); strbuf_release(&piecebuf); - return rfc2047; -} - -static void decode_header(struct strbuf *it) -{ - if (decode_header_bq(it)) - return; } static void decode_transfer_encoding(struct strbuf *line) -- 2.6.1-320-g86a1181 -- 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
[PATCH 01/26] mailinfo: remove a no-op call convert_to_utf8(it, "")
The called function checks if the second parameter is either a NULL or an empty string at the very beginning and returns without doing anything. Remove the useless call. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 5 - 1 file changed, 5 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 169ee54..330bef4 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -612,11 +612,6 @@ static void decode_header(struct strbuf *it) { if (decode_header_bq(it)) return; - /* otherwise "it" is a straight copy of the input. -* This can be binary guck but there is no charset specified. -*/ - if (metainfo_charset) - convert_to_utf8(it, ""); } static void decode_transfer_encoding(struct strbuf *line) -- 2.6.1-320-g86a1181 -- 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
[PATCH 00/26] mailinfo libification
So here is an attempt to libify "git mailinfo" so that the built-in version of "git am" does not have to run it via run_command() interface. "git am", when fed an N-patch series, runs one "mailsplit", N "mailinfo" and N "apply" all via run_command() interface (plus 2 more "apply" and 1 "merge-recursive" per a patch that does not apply cleanly, when run with the "-3" option), and among the various programs spawned from "git am", "mailinfo" is the most straight-forward, stupid and light-weight program, so it is a no-brainer to pick it as the candidate for libification. This goes on top of c5920b21 (mailinfo: ignore in-body header that we do not care about, 2015-10-08) that was posted earlier as a weatherbaloon patch. Junio C Hamano (26): mailinfo: remove a no-op call convert_to_utf8(it, "") mailinfo: fix for off-by-one error in boundary stack mailinfo: fold decode_header_bq() into decode_header() mailinfo: move handle_boundary() lower mailinfo: get rid of function-local static states mailinfo: always pass "line" as an argument mailinfo: move global "line" into mailinfo() function mailinfo: introduce "struct mailinfo" to hold globals mailinfo: move keep_subject & keep_non_patch_bracket to struct mailinfo mailinfo: move global "FILE *fin, *fout" to struct mailinfo mailinfo: move filter/header stage to struct mailinfo mailinfo: move patch_lines to struct mailinfo mailinfo: move add_message_id and message_id to struct mailinfo mailinfo: move use_scissors and use_inbody_headers to struct mailinfo mailinfo: move metainfo_charset to struct mailinfo mailinfo: move transfer_encoding to struct mailinfo mailinfo: move charset to struct mailinfo mailinfo: handle_commit_msg() shouldn't be called after finding patchbreak mailinfo: move cmitmsg and patchfile to struct mailinfo mailinfo: move [ps]_hdr_data to struct mailinfo mailinfo: keep the parsed log message in a strbuf mailinfo: move content/content_top to struct mailinfo mailinfo: handle errors found in decode_header() better mailinfo: handle charset conversion errors in the caller mailinfo: remove calls to exit() and die() deep in the callchain mailinfo: libify the whole thing Makefile |1 + builtin/mailinfo.c | 1083 +--- mailinfo.c | 1058 ++ mailinfo.h | 41 ++ 4 files changed, 1120 insertions(+), 1063 deletions(-) create mode 100644 mailinfo.c create mode 100644 mailinfo.h -- 2.6.1-320-g86a1181 -- 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
[PATCH 10/26] mailinfo: move global "FILE *fin, *fout" to struct mailinfo
This requires us to pass "struct mailinfo" to more functions throughout the codepath that read input lines, which makes later steps easier. Signed-off-by: Junio C Hamano --- builtin/mailinfo.c | 54 -- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index d642b0d..a9da338 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c @@ -7,11 +7,14 @@ #include "utf8.h" #include "strbuf.h" -static FILE *cmitmsg, *patchfile, *fin, *fout; +static FILE *cmitmsg, *patchfile; static const char *metainfo_charset; struct mailinfo { + FILE *input; + FILE *output; + struct strbuf name; struct strbuf email; int keep_subject; @@ -819,16 +822,17 @@ static void handle_filter(struct strbuf *line, int *filter_stage, int *header_st } } -static int find_boundary(struct strbuf *line) +static int find_boundary(struct mailinfo *mi, struct strbuf *line) { - while (!strbuf_getline(line, fin, '\n')) { + while (!strbuf_getline(line, mi->input, '\n')) { if (*content_top && is_multipart_boundary(line)) return 1; } return 0; } -static int handle_boundary(struct strbuf *line, int *filter_stage, int *header_stage) +static int handle_boundary(struct mailinfo *mi, struct strbuf *line, + int *filter_stage, int *header_stage) { struct strbuf newline = STRBUF_INIT; @@ -854,7 +858,7 @@ again: strbuf_release(&newline); /* skip to the next boundary */ - if (!find_boundary(line)) + if (!find_boundary(mi, line)) return 0; goto again; } @@ -864,18 +868,18 @@ again: strbuf_reset(&charset); /* slurp in this section's info */ - while (read_one_header_line(line, fin)) + while (read_one_header_line(line, mi->input)) check_header(line, p_hdr_data, 0); strbuf_release(&newline); /* replenish line */ - if (strbuf_getline(line, fin, '\n')) + if (strbuf_getline(line, mi->input, '\n')) return 0; strbuf_addch(line, '\n'); return 1; } -static void handle_body(struct strbuf *line) +static void handle_body(struct mailinfo *mi, struct strbuf *line) { struct strbuf prev = STRBUF_INIT; int filter_stage = 0; @@ -883,7 +887,7 @@ static void handle_body(struct strbuf *line) /* Skip up to the first boundary */ if (*content_top) { - if (!find_boundary(line)) + if (!find_boundary(mi, line)) goto handle_body_out; } @@ -895,7 +899,7 @@ static void handle_body(struct strbuf *line) handle_filter(&prev, &filter_stage, &header_stage); strbuf_reset(&prev); } - if (!handle_boundary(line, &filter_stage, &header_stage)) + if (!handle_boundary(mi, line, &filter_stage, &header_stage)) goto handle_body_out; } @@ -938,7 +942,7 @@ static void handle_body(struct strbuf *line) handle_filter(line, &filter_stage, &header_stage); } - } while (!strbuf_getwholeline(line, fin, '\n')); + } while (!strbuf_getwholeline(line, mi->input, '\n')); handle_body_out: strbuf_release(&prev); @@ -980,29 +984,25 @@ static void handle_info(struct mailinfo *mi) cleanup_subject(mi, hdr); cleanup_space(hdr); } - output_header_lines(fout, "Subject", hdr); + output_header_lines(mi->output, "Subject", hdr); } else if (!strcmp(header[i], "From")) { cleanup_space(hdr); handle_from(mi, hdr); - fprintf(fout, "Author: %s\n", mi->name.buf); - fprintf(fout, "Email: %s\n", mi->email.buf); + fprintf(mi->output, "Author: %s\n", mi->name.buf); + fprintf(mi->output, "Email: %s\n", mi->email.buf); } else { cleanup_space(hdr); - fprintf(fout, "%s: %s\n", header[i], hdr->buf); + fprintf(mi->output, "%s: %s\n", header[i], hdr->buf); } } - fprintf(fout, "\n"); + fprintf(mi->output, "\n"); } -static int mailinfo(struct mailinfo *mi, - FILE *in, FILE *out, const char *msg, const char *patch) +static int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) { int peek; struct strbuf line = STRBUF_INIT; - fin = in; - fout = out; - cmitmsg = fop
Re: How to rebase when some commit hashes are in some commit messages
From: "Jacob Keller" On Tue, Oct 13, 2015 at 12:24 PM, Philip Oakley wrote: IIUC (as an alternate example), in G4W one can submit a (long) pull request with internal back references that would be merged directly, so the sha1's could be updated as Francois-Xavier originally asked. I have a series that's been bumping along for a long while that needs regular rebasing, though doesn't have sha1 back references, so I can see that the need does happen. I can see that others may have a workflow that would work well with the sha1 auto-update. -- Philip I still don't see how this is useful, because the part that *can* be implemented is not valuable and the part that is valuable can't be implemented. So, what we can implement easily enough: you rebase a series and any time the message contains sha1 of a commit we're modifying in this rebase, we update the sha1 to match again. This seems reasonable, but not useful. Why would you reference a commit that is *ITSELF* being rebased. No one has explained a reasonable use for this... I'm sure there exists one, but I would want an explanation of one first. This particular case is about self-references within a long series. At the moment, on the Git list there is general comments about say [PATCH v3 18/44], whichs is great for for the list ($gmane) but not for a `git log`. In flows where PRs are valid, one can have what was [34/44] refering to prior patch [26/44] as `deadbeaf` or whatever. It won't be suitable for most flows but will be useful for a proportion (as already evidenced by the request). The "useful" case is if you rebase "onto" a tree that has a previous history that has been changed. In this case, how do you propose we find it. This use case (where upstream also rebases) hasn't been considered. It would be a tricky one. As long as the possibility (of such an A depends on B re-write) isn't closed off then the smaller requested case could still go ahead. Doing as suggested above, ie: only changing sha1s that we are already rebasing works, but why are you backreferencing it if you are re-writing the commit? Essentially one wants to say `$CURR_COMMIT~nn` (i.e. "see nn commits earlier in my series") and have that replaced with its cannonical sha1, and updated when rebased. It sort of begs the question whether there should be a ref shorthand for "the (this) current commit" to allow THIS~ as an interpretable [valid?] format. That doesn't make sense to me at all. Yes, you can do it, but I don't get why this is valuable. If you're backref is "fixes xyz" why not just fix xyz instead of have two commits. If the back ref has some other value... what is that value? I don't understand it I guess. For the 'fixes' (of a bug report) case we are already talking about an immutable so it would not be part of this. Its use may be more of the type "Using helper function xyz introduced earlier in patch abcde", which would change after each rebase. It just seems pretty narrow focus. I mean if someone wants to implement it, that is fine. Agreed -- Philip -- 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 18/44] refs: move transaction functions into common code
On Tue, 2015-10-13 at 18:21 -0400, David Turner wrote: > On Tue, 2015-10-13 at 13:29 +0200, Michael Haggerty wrote: > > I reviewed the patches up to here pretty carefully, and aside from the > > comments I already sent, they look good. > > > > I like the new approach where the ref_transaction-building code is > > shared across backends. > > > > It seems to me that a good breaking point for the first batch of patches > > would be here, just before you start creating the VTABLE in commit > > [19/44]. The patches before this point all have to do with moving code > > around and a little bit of light refactoring. They cause a lot of code > > churn that will soon conflict with other people's work (e.g., [1]), but > > I think they are pretty uncontroversial. > > > > After this you start making a few important design decisions that > > *could* be controversial. Therefore, by making a cut, you can maximize > > the chance that the earlier patches can be merged to master relatively > > quickly, after which the cross section for future conflicts will be much > > smaller. > > > > Ideally, you would include a few later patches in the "pre-VTABLE" patch > > series. It looks like the following patches also mostly have to do with > > moving code around, and would fit logically with the "pre-VTABLE" changes: > > > > [24] refs.c: move refname_is_safe to the common code > > [25] refs.h: document make refname_is_safe and add it to header > > [26] refs.c: move copy_msg to the common code > > [27] refs.c: move peel_object to the common code > > [28] refs.c: move should_autocreate_reflog to common code > > [32] initdb: move safe_create_dir into common code > > [36] refs: make files_log_ref_write functions public > > [37] refs: break out a ref conflict check > > > > I tried rebasing those commits on top of your patch 18 and it wasn't too > > bad. The result is on branch "refs-backend-pre-vtable" on my GitHub repo > > [2], including my suggested changes to those eight patches (which > > therefore became seven because I squashed the first two together). > > Thanks. I started from that, and made the changes that you suggested > in reviews that were not yet in there. > > I also added Jeff's extension patch, since it seems uncontroversial to > me, and since we'll need it sooner or later anyway. > > I put the result on my github at: > https://github.com/dturner-tw/git > on the refs-backend-pre-vtable branch While rebasing the rest of the series on this, I noticed an issue in one of the patches, which I have now fixed. I re-pushed. -- 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
bug report: `git stash save -u` deletes directory whose contents are .gitignored
hi, - `git stash save -u` deletes a directory, even though the _contents_ of that directory are .gitignored (e.g. "foo/*" in .gitignore). Detailed reproduction below. - The behaviour is not present when instead .gitignoring the directory itself (e.g. "foo"). This does imo not excuse the behaviour of the described case. - The behaviour is not present with just `git stash save`. - The expected behaviour is: Only files listed in `git status` are stashed away; other things are left untouched. Potential exception: empty directories. - I am aware that `git stash save -u` is supposed to have semantics involving the equivalent of `reset --hard` and `clean -f`. Neither of those expose the directory-deletion behaviour. additional comments, not directly relevant to this bug: - both `git stash save; git stash pop` and `git stash save -u; git stash pop` should ideally behave as an "identity effect". The latter might affect the index (i.e. have an effect equivalent to `git reset` or something in that direction). Any other effect / special cases are bad interface design imo. If a differing design was chosen on purpose, i am thankful for pointers on the reasoning. - no, i luckily did not truely lose files due to the current behaviour. lennart reproduction: > git init Initialized empty Git repository in $SOMEDIR/.git/ > mkdir ignore-contents > echo "ignore-contents/*" > .gitignore > git add .gitignore > git commit -m "initial" > /dev/null > touch ignore-contents/blah.txt > touch abc.txt > git status On branch master Untracked files: (use "git add ..." to include in what will be committed) abc.txt nothing added to commit but untracked files present (use "git add" to track) > ls -1a . .. abc.txt .git .gitignore ignore-contents > git stash save -u Saved working directory and index state WIP on master: adc468b initial HEAD is now at adc468b initial > ls -1a . .. .git .gitignore > git stash pop &> /dev/null > ls -1a . .. abc.txt .git .gitignore > git --version git version 2.6.1 -- 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 18/44] refs: move transaction functions into common code
On Tue, 2015-10-13 at 13:29 +0200, Michael Haggerty wrote: > I reviewed the patches up to here pretty carefully, and aside from the > comments I already sent, they look good. > > I like the new approach where the ref_transaction-building code is > shared across backends. > > It seems to me that a good breaking point for the first batch of patches > would be here, just before you start creating the VTABLE in commit > [19/44]. The patches before this point all have to do with moving code > around and a little bit of light refactoring. They cause a lot of code > churn that will soon conflict with other people's work (e.g., [1]), but > I think they are pretty uncontroversial. > > After this you start making a few important design decisions that > *could* be controversial. Therefore, by making a cut, you can maximize > the chance that the earlier patches can be merged to master relatively > quickly, after which the cross section for future conflicts will be much > smaller. > > Ideally, you would include a few later patches in the "pre-VTABLE" patch > series. It looks like the following patches also mostly have to do with > moving code around, and would fit logically with the "pre-VTABLE" changes: > > [24] refs.c: move refname_is_safe to the common code > [25] refs.h: document make refname_is_safe and add it to header > [26] refs.c: move copy_msg to the common code > [27] refs.c: move peel_object to the common code > [28] refs.c: move should_autocreate_reflog to common code > [32] initdb: move safe_create_dir into common code > [36] refs: make files_log_ref_write functions public > [37] refs: break out a ref conflict check > > I tried rebasing those commits on top of your patch 18 and it wasn't too > bad. The result is on branch "refs-backend-pre-vtable" on my GitHub repo > [2], including my suggested changes to those eight patches (which > therefore became seven because I squashed the first two together). Thanks. I started from that, and made the changes that you suggested in reviews that were not yet in there. I also added Jeff's extension patch, since it seems uncontroversial to me, and since we'll need it sooner or later anyway. I put the result on my github at: https://github.com/dturner-tw/git on the refs-backend-pre-vtable branch -- 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: How to rebase when some commit hashes are in some commit messages
On Tue, Oct 13, 2015 at 12:24 PM, Philip Oakley wrote: > IIUC (as an alternate example), in G4W one can submit a (long) pull request > with internal back references that would be merged directly, so the sha1's > could be updated as Francois-Xavier originally asked. I have a series that's > been bumping along for a long while that needs regular rebasing, though > doesn't have sha1 back references, so I can see that the need does happen. I > can see that others may have a workflow that would work well with the sha1 > auto-update. > > -- > Philip > I still don't see how this is useful, because the part that *can* be implemented is not valuable and the part that is valuable can't be implemented. So, what we can implement easily enough: you rebase a series and any time the message contains sha1 of a commit we're modifying in this rebase, we update the sha1 to match again. This seems reasonable, but not useful. Why would you reference a commit that is *ITSELF* being rebased. No one has explained a reasonable use for this... I'm sure there exists one, but I would want an explanation of one first. The "useful" case is if you rebase "onto" a tree that has a previous history that has been changed. In this case, how do you propose we find it. Doing as suggested above, ie: only changing sha1s that we are already rebasing works, but why are you backreferencing it if you are re-writing the commit? That doesn't make sense to me at all. Yes, you can do it, but I don't get why this is valuable. If you're backref is "fixes xyz" why not just fix xyz instead of have two commits. If the back ref has some other value... what is that value? I don't understand it I guess. It just seems pretty narrow focus. I mean if someone wants to implement it, that is fine. Regards, Jake -- 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 02/44] refs: make repack_without_refs and is_branch public
On Tue, 2015-10-13 at 09:23 +0200, Michael Haggerty wrote: > On 10/13/2015 04:39 AM, Michael Haggerty wrote: > > On 10/12/2015 11:51 PM, David Turner wrote: > >> is_branch was already non-static, but this patch declares it in the > >> header. > >> > >> Signed-off-by: Ronnie Sahlberg > >> Signed-off-by: David Turner > >> --- > >> [...] > > > > It seems odd that repack_without_refs() should be made public (and > > ultimately end up in refs.h) given that it intrinsically only has to do > > with file-based references. But I will read on... > > I think the reason you needed to do this was because you wanted to move > delete_refs() to the common code. It is true that delete_ref() can be > moved to the common code. And most of the code in delete_refs() is just > a loop calling delete_ref(). But delete_refs() also does the very > files-specific optimization of calling repack_without_refs() before the > loop. *That* call shouldn't be in the common code. > > So my suggestion is that you write a common_delete_refs() function that > only includes the loop over delete_ref(), and a files_delete_refs() > function that is basically > > { > result = repack_without_refs(refnames, &err); > if (result) { > ...report error... > return result; > } > return common_delete_refs(...); > } OK, I can do that. That will have to be part of the backends work, so I'll exclude it from the refs-backend-pre-vtable patch set. -- 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 19/44] refs-be-files.c: add a backend method structure with transaction functions
On 10/13/2015 08:28 PM, David Turner wrote: > On Tue, 2015-10-13 at 09:56 +0200, Michael Haggerty wrote: >> On 10/12/2015 11:51 PM, David Turner wrote: >>> [...] >>> +extern struct ref_be refs_be_files; >> >> I don't think that refs_be_files is needed in the public interface. > > We use refs_be_lmdb in a few other places to set up the lmdb backend, so > I thought I would put refs_be_files in refs.h too. But I can remove > refs_be_files and just stick it in the places it's needed. It's OK then. It doesn't hurt to leave it for consistency's sake. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 1/1] send-email: allow to compose only prepared cover letter
Andy Shevchenko writes: > My often use case is to do: > % git format-patch --cover-letter --subject-prefix="PATCH vN" > rev1^..revXYZ > % $GIT_EDITOR -* > % git send-email 00* # assumes series less than 100 patches > % rm -f 00* I guess this patch would not hurt too much, but the above would vastly be improved if you used "-vN" option, instead of the hand-rolled subject prefix, and dropped the last "rm -f" (which in turn would mean you would want to use -o option to specify where to keep the older iterations of the topic). Then you can easily refer to cover letters and patches from previous rounds while preparing the latest to send out. -- 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] Add fetch.recurseSubmoduleParallelism config option
Stefan Beller writes: > Assuming we go with your second school of thought (N are the real > running processes, M including the finished but still pending output tasks), That's neither of my two, I would think. Anyway, I think it is now clear that it not very easy to say "I know what to fill in M and N" there ;-). -- 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 v2] merge: fix cache_entry use-after-free
David Turner writes: >> This one smelled iffy. I think it is safe because the caller does >> not look at src[] other than src[0] after this function returns, and >> this setting to NULL happens only when o->merge is set to 1, so I do >> not think this is buggy, but at the same time I do not think setting >> to NULL is necessary. >> >> Other than that, looks nice. Thanks. > > I like to set a pointer to NULL after I free the thing pointed to by it, > because it helps make use-after-free bugs easier to detect. Yes, but it also helps unintended bugs to creep in if done blindly, and that is why I vetted what happens in the codepath of the caller of this function after src[] entries are NULLed (the caller could be expecting to do things only to NULLed fields, for example, in which case clearing them like this patch would have changed the behaviour of the caller after this function returns). Thanks. -- 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 05/44] refs.c: move update_ref to refs.c
On Tue, 2015-10-13 at 05:41 +0200, Michael Haggerty wrote: > If its removal was intentional, it deserves a careful explanation (and > should probably be done as a separate commit). If it was an accident, > please consider how this accident arose and try to think about whether > similar accidents might have happened elsewhere in this series. This was an accident. I think it must have happened when I forward-ported Ronnie's changes over my change that introduced that check. Usually, when there were conflicts during this process (indicating that the moved code had changed in the meantime), I did the move by copy-pasting the code (rather than by choosing the old version). Apparently, I missed this one. Will fix. > > [...] > > Michael > -- 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: How to rebase when some commit hashes are in some commit messages
From: "Mike Rappazzo" On Tue, Oct 13, 2015 at 1:07 PM, Jacob Keller wrote: On Tue, Oct 13, 2015 at 6:29 AM, Philip Oakley wrote: My tuppence is that the only sha1's that could/would be rewritten would be those for the commits within the rebase. During rebasing it is expected that the user is re-adjusting things for later upstream consumption, with social controls and understandings with colleagues. Agreed here. There would be no need to change any sha1s that didn't change during the rebase. This limits the scope. Alright. Thus the only sha1 numbers that could be used are those that are within the (possibly implied) instruction sheet (which will list the current sha1s that will be converted by rebase to new sha1's). Correct, you would be able to limit the number of sha1s to search for. However, (see below), any reasonable reference to a sha1 should be relatively stable. It should be clear that the sha1's are always backward references (because of the impossibility of including a forward reference to an as yet un-created future commit's sha1). The key question (for me) is whether short sha1s are accepted, or if they must be full 40 char sha1's (perhaps an option). There are already options for making sure that short refs are not ambiguous. It sound to me like a sensible small project for those that have such a workflow. I'm not sure if it should work with a patch based flow when submitting upstream - I'm a little fuzzy on how would the upstream maintainer know which sha1 referred to which patch. My issue: the only sha1s in commit messages are *generally* things which will NOT be changed in general. Supporting a work flow that wants to change these is definitely crazy. Essentially: I don't see a reason that you would be rebasing a commit and needing to change any references in it. You can reference a commit which isn't changing, but here's the possible situations I see: a) you are rebasing a commit which references in the message a commit that is not being changed (it is ancient) In this case, nothing needs to be done. b) you are rebasing a commit which references another commit in the same rebase I see no valid reason to reference a sha1 in this case. If you're referencing as a "fixes", then you are being silly since you can just squash the fix into the original commit and thus prevent introduction of bug at all. What other reason? If you are referencing such as "thix extends implementation from sha1" then your commit message is probably poorly formatted. I don't see a reason to support this flow. c) you are rebasing a commit which is referencing a commit that has already been changed. (?) I think (maybe) this is your interesting case, but here are some caveats. Let's say you are fixing some old commit such as "Fixes: " or something. If you do a "git pull --rebase", your commit might be updated to play ontop of more new work, but the sha1 should still be valid, *unless* the remote history did some rewind, at which point I don't think any algorithm will work, see the issues above. It may be something worth doing in git-filter-branch, but then you're looking at losing the two assumptions above making it really hard to get right. Regards, Jake It seems reasonable that this could be added as a feature of interactive rebase. The todo list could be automatically adjusted to "reword" for those commits which are referring to other commits within the same rebase. As each commit is re-written, a mapping could be kept of old sha1 -> new sha1. Then when one of the reworded commits is being applied, the old sha1 -> new sha1 mapping could be used to add a line to the $COMMIT_MSG. -- The extra fun begins if the commit message is of a one-line pretty quoted style, where more of the quote needs changing... e.g. [alias] quote = log -1 --pretty='tformat:%h (%s, %ad)' --date=short log1 = log -1 --pretty=\"format:%ad %h (%an): %s\" --date=short Jake was concerned about the 'crazy' workflow, however almost all workflows are crazy at a distance. The rebase is required if the workflow's allowed base point moves forward faster than one can complete the (likely long) patch series, so the series is rebased and then an acceptable series can be merged without modifications. Git has the former issue i.e. master and next can move forward faster than a long series takes to be reviewed, but does not have the latter because Junio adds his signature to each commit, and uses the patch submission flow. IIUC (as an alternate example), in G4W one can submit a (long) pull request with internal back references that would be merged directly, so the sha1's could be updated as Francois-Xavier originally asked. I have a series that's been bumping along for a long while that needs regular rebasing, though doesn't have sha1 back references, so I can see that the need does happen. I can see that others may have a workflow that would work well with the sha1 auto-update. -- Philip -- To
Re: [PATCH v2] merge: fix cache_entry use-after-free
On Mon, 2015-10-12 at 15:28 -0700, Junio C Hamano wrote: > David Turner writes: > > > From: Keith McGuigan > > > > During merges, we would previously free entries that we no longer need > > in the destination index. But those entries might also be stored in > > the dir_entry cache, and when a later call to add_to_index found them, > > they would be used after being freed. > > > > To prevent this, add a ref count for struct cache_entry. Whenever > > a cache entry is added to a data structure, the ref count is incremented; > > when it is removed from the data structure, it is decremented. When > > it hits zero, the cache_entry is freed. > > > > Signed-off-by: Keith McGuigan > > --- > > I'll forge your "messenger's sign-off" here ;-) Thanks. > > diff --git a/unpack-trees.c b/unpack-trees.c > > index f932e80..1a0a637 100644 > > --- a/unpack-trees.c > > +++ b/unpack-trees.c > > @@ -606,8 +606,10 @@ static int unpack_nondirectories(int n, unsigned long > > mask, > > o); > > for (i = 0; i < n; i++) { > > struct cache_entry *ce = src[i + o->merge]; > > - if (ce != o->df_conflict_entry) > > - free(ce); > > + if (ce != o->df_conflict_entry) { > > + drop_ce_ref(ce); > > + src[i + o->merge] = NULL; > > + } > > This one smelled iffy. I think it is safe because the caller does > not look at src[] other than src[0] after this function returns, and > this setting to NULL happens only when o->merge is set to 1, so I do > not think this is buggy, but at the same time I do not think setting > to NULL is necessary. > > Other than that, looks nice. Thanks. I like to set a pointer to NULL after I free the thing pointed to by it, because it helps make use-after-free bugs easier to detect. -- 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 v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
Matthieu Moy writes: >> If you see here, we detect "track" first for >> %(upstream:track,nobracket) > > Yes, but I still think that this was a bad idea. If you want > nobracket to apply to "track", then the syntax should be > %(upstream:track=nobracket). I think the "nobracket" should apply > to "upstream" (i.e. be global to the atom), hence > %(upstream:nobracket,track) and %(upstream:track,nobracket) should > both be accepted. That makes sense to me, as I think what is being discussed would be %(upstream:track=yes,bracket=no) when it is fully spelled out. -- 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: Solaris: Fail to build git with CFLAGS=-m64
Hi Junio, On 10/13/15, Junio C Hamano wrote: > evgeny litvinenko writes: > >> Solaris has the following prototype in the file /usr/include/arpa/inet.h: >> >> extern const char *inet_ntop(int, const void *_RESTRICT_KYWD, char >> *_RESTRICT_KYWD, socklen_t); >> >> Git's prototype for inet_ntop is in file git-compat-util.h: >> >> #ifdef NO_INET_NTOP >> const char *inet_ntop(int af, const void *src, char *dst, size_t size); >> #endif > > This tells me that it is designed to be used only when the platform > does not offer inet_ntop(). If your platform does have it, why is > your build define NO_INET_NTOP? I think this is because configure.ac doesn't check for inet_ntop in libnsl (in Solaris 11.2 inet_ntop and inet_pton are in libnsl) configure.ac checks for these function in libresolv only. > Perhaps configure generated by autoconf is faulty? > > In this project, use of autoconf/configure is optional---in other > words, it is expected that you can build successfully by setting and > unsetting necessary Makefile macros in your own config.mak without > using autoconf/configure at all. I'd try without configure and make > sure I do not define NO_INET_NTOP (and if you have inet_pton(), then > make sure you do not define NO_INET_PTON either) in config.mak if I > were you. > Yes, if I do not define NO_INET_NTOP/NO_INET_PTON the build is successful as libnsl is linked - in configure.ac there is 'NEEDS_NSL = YesPlease' for SunOS'es. evgeny. -- 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 v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
Karthik Nayak writes: > On Thu, Oct 8, 2015 at 2:48 PM, Karthik Nayak wrote: >> Add support for %(upstream:track,nobracket) which will print the >> tracking information without the brackets (i.e. "ahead N, behind M"). >> >> Add test and documentation for the same. >> --- >> Documentation/git-for-each-ref.txt | 6 -- >> ref-filter.c | 28 +++- >> t/t6300-for-each-ref.sh| 9 + >> 3 files changed, 36 insertions(+), 7 deletions(-) >> >> diff --git a/Documentation/git-for-each-ref.txt >> b/Documentation/git-for-each-ref.txt >> index c713ec0..a38cbf6 100644 >> --- a/Documentation/git-for-each-ref.txt >> +++ b/Documentation/git-for-each-ref.txt >> @@ -114,8 +114,10 @@ upstream:: >> `refname` above. Additionally respects `:track` to show >> "[ahead N, behind M]" and `:trackshort` to show the terse >> version: ">" (ahead), "<" (behind), "<>" (ahead and behind), >> - or "=" (in sync). Has no effect if the ref does not have >> - tracking information associated with it. >> + or "=" (in sync). Append `:track,nobracket` to show tracking >> + information without brackets (i.e "ahead N, behind M"). Has >> + no effect if the ref does not have tracking information >> + associated with it. >> >> push:: >> The name of a local ref which represents the `@{push}` location >> diff --git a/ref-filter.c b/ref-filter.c >> index 6a38089..6044eb0 100644 >> --- a/ref-filter.c >> +++ b/ref-filter.c >> @@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item >> *ref) >> if (!strcmp(formatp, "short")) >> refname = shorten_unambiguous_ref(refname, >> warn_ambiguous_refs); >> - else if (!strcmp(formatp, "track") && >> + else if (skip_prefix(formatp, "track", &valp) && >> +strcmp(formatp, "trackshort") && >> (starts_with(name, "upstream") || >> starts_with(name, "push"))) { > > If you see here, we detect "track" first for > %(upstream:track,nobracket) Yes, but I still think that this was a bad idea. If you want nobracket to apply to "track", then the syntax should be %(upstream:track=nobracket). I think the "nobracket" should apply to "upstream" (i.e. be global to the atom), hence %(upstream:nobracket,track) and %(upstream:track,nobracket) should both be accepted. Possibly %(upstream:,nobracket) could complain, but just ignoring "nobracket" in this case would make sense to me. Special-casing the implementation of "nobracket" also means you're special-casing its user-visible behavior. And as a user, I hate it when the behavior is subtely different for things that look like each other (in this case, %(align:...) and %(upstream:...) ). > %(upstream:nobracket,track) to be supported then, I think we'll have > to change this whole layout and have the detection done up where we > locat "upstream" / "push", what would be a nice way to go around this? You mean, below else if (starts_with(name, "upstream")) { within populate_value()? I think it would, yes. > What I could think of: > 1. Do the cleanup that Junio and Matthieu suggested, where we > basically parse the > atoms and store them into a used_atom struct. I could either work on > those patches > before this and then rebase this on top. > 2. Let this be and come back on it when implementing the above series. > After reading Matthieu's and Junio's discussion, I lean towards the latter. Leaving it as-is does not fit in my arguments to do the refactoring later. It's not introducing "another instance of an existing pattern", but actually a new pattern. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 37/44] refs: break out a ref conflict check
On Tue, 2015-10-13 at 12:25 +0200, Michael Haggerty wrote: > On 10/12/2015 11:51 PM, David Turner wrote: > > Create new function verify_no_descendants, to hold one of the ref > > conflict checks used in verify_refname_available. Multiple backends > > will need this function, so it goes in the common code. > > > > Signed-off-by: David Turner > > --- > > refs-be-files.c | 33 - > > refs.c | 22 ++ > > refs.h | 7 +++ > > 3 files changed, 37 insertions(+), 25 deletions(-) > > > > [...] > > diff --git a/refs.c b/refs.c > > index 5a3125d..3ae0274 100644 > > --- a/refs.c > > +++ b/refs.c > > @@ -1101,6 +1101,28 @@ enum peel_status peel_object(const unsigned char > > *name, unsigned char *sha1) > > return PEEL_PEELED; > > } > > > > +const char *find_descendant_ref(const char *refname, > > + const struct string_list *extras, > > + const struct string_list *skip) > > +{ > > + int pos; > > + if (!extras) > > + return NULL; > > + > > + /* Look for the place where "$refname/" would be inserted in extras. */ > > The comment above is misleading. See my note at the bottom of this email > for an explanation. > > > + for (pos = string_list_find_insert_index(extras, refname, 0); > > +pos < extras->nr; pos++) { > > + const char *extra_refname = extras->items[pos].string; > > + > > + if (!starts_with(extra_refname, refname)) > > + break; > > + > > + if (!skip || !string_list_has_string(skip, extra_refname)) > > + return extra_refname; > > + } > > + return NULL; > > +} > > + > > /* backend functions */ > > int refs_initdb(struct strbuf *err, int shared) > > { > > diff --git a/refs.h b/refs.h > > index 3aad3b8..f8becea 100644 > > --- a/refs.h > > +++ b/refs.h > > @@ -637,6 +637,13 @@ int files_log_ref_write(const char *refname, const > > unsigned char *old_sha1, > > const unsigned char *new_sha1, const char *msg, > > int flags, struct strbuf *err); > > > > +/* > > + * Check for entries in extras that start with "$refname/", ignoring > > + * those in skip. If there is such an entry, then we have a conflict. > > + */ > > +const char *find_descendant_ref(const char *refname, > > + const struct string_list *extras, > > + const struct string_list *skip); > > The documentation is is not correct. As written, the function needs not > the refname as argument but the refname followed by '/'. Without the > trailing slash, "refs/heads/foo" would be blocked by "refs/heads/foobar". > > From the point of view of simplicity, it would be nice if this function > could take a refname (without the trailing slash) as argument. But then > it would basically be forced to allocate a new string, copy refname to > it, append a '/', then free the string again when it's done. > Alternatively, it could accept its refname argument in a strbuf and > temporarily append the '/'. > > But neither one of these alternatives is so elegant, so maybe it's OK if > this function is documented to take a "directory name, including the > trailing '/'" as argument and rename the argument (e.g., to "dirname"). > > Please also document that "skip" and "extras" can be NULL, but if not > NULL they need to be sorted lists. I think `extras` may not be NULL. But I've otherwise fixed this. Thanks. -- 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 25/44] refs.h: document make refname_is_safe and add it to header
On Tue, 2015-10-13 at 11:18 +0200, Michael Haggerty wrote: > On 10/13/2015 09:33 AM, Michael Haggerty wrote: > > On 10/12/2015 11:51 PM, David Turner wrote: > >> This function might be used by other refs backends > >> > >> Signed-off-by: David Turner > >> --- > >> refs.h | 11 +++ > >> 1 file changed, 11 insertions(+) > >> > >> diff --git a/refs.h b/refs.h > >> index fc8a748..7a936e2 100644 > >> --- a/refs.h > >> +++ b/refs.h > >> @@ -348,6 +348,17 @@ int verify_refname_available(const char *newname, > >> struct string_list *extra, > >> struct string_list *skip, struct strbuf *err); > >> > >> /* > >> + * Check if a refname is safe. > >> + * For refs that start with "refs/" we consider it safe as long they do > >> + * not try to resolve to outside of refs/. > >> + * > >> + * For all other refs we only consider them safe iff they only contain > >> + * upper case characters and '_' (like "HEAD" AND "MERGE_HEAD", and not > >> like > >> + * "config"). > >> + */ > >> +int refname_is_safe(const char *refname); > >> + > >> +/* > >> * Flags controlling ref_transaction_update(), ref_transaction_create(), > >> etc. > >> * REF_NODEREF: act on the ref directly, instead of dereferencing > >> * symbolic references. > >> > > > > The previous commit deleted this comment from where it previously > > appeared in refs-be-files.c. It would make more sense to squash this > > commit onto that one so it's clear that you are moving the comment > > rather than creating a new comment out of thin air. > > Also, after this commit the prototype for this function appears twice in > refs.h. Will squash and fix. -- 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 19/44] refs-be-files.c: add a backend method structure with transaction functions
On Tue, 2015-10-13 at 09:56 +0200, Michael Haggerty wrote: > On 10/12/2015 11:51 PM, David Turner wrote: > > From: Ronnie Sahlberg > > > > Add a ref structure for backend methods. Start by adding a method pointer > > for the transaction commit function. > > > > Add a function set_refs_backend to switch between backends. The files > > based backend is the default. > > > > Signed-off-by: Ronnie Sahlberg > > Signed-off-by: David Turner > > --- > > refs-be-files.c | 10 -- > > refs.c | 30 ++ > > refs.h | 15 +++ > > 3 files changed, 53 insertions(+), 2 deletions(-) > > > > [...] > > diff --git a/refs.h b/refs.h > > index 4940ae9..419abf4 100644 > > --- a/refs.h > > +++ b/refs.h > > @@ -619,4 +619,19 @@ extern int reflog_expire(const char *refname, const > > unsigned char *sha1, > > reflog_expiry_cleanup_fn cleanup_fn, > > void *policy_cb_data); > > > > +/* refs backends */ > > +typedef int ref_transaction_commit_fn(struct ref_transaction *transaction, > > + struct strbuf *err); > > +typedef void ref_transaction_free_fn(struct ref_transaction *transaction); > > The ref_transaction_free_fn typedef isn't used anywhere. Will fix, thanks. > > +struct ref_be { > > + struct ref_be *next; > > + const char *name; > > + ref_transaction_commit_fn *transaction_commit; > > +}; > > + > > + > > +extern struct ref_be refs_be_files; > > I don't think that refs_be_files is needed in the public interface. We use refs_be_lmdb in a few other places to set up the lmdb backend, so I thought I would put refs_be_files in refs.h too. But I can remove refs_be_files and just stick it in the places it's needed. -- 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 v2 08/10] ref-filter: add support for %(upstream:track,nobracket)
On Thu, Oct 8, 2015 at 2:48 PM, Karthik Nayak wrote: > Add support for %(upstream:track,nobracket) which will print the > tracking information without the brackets (i.e. "ahead N, behind M"). > > Add test and documentation for the same. > --- > Documentation/git-for-each-ref.txt | 6 -- > ref-filter.c | 28 +++- > t/t6300-for-each-ref.sh| 9 + > 3 files changed, 36 insertions(+), 7 deletions(-) > > diff --git a/Documentation/git-for-each-ref.txt > b/Documentation/git-for-each-ref.txt > index c713ec0..a38cbf6 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -114,8 +114,10 @@ upstream:: > `refname` above. Additionally respects `:track` to show > "[ahead N, behind M]" and `:trackshort` to show the terse > version: ">" (ahead), "<" (behind), "<>" (ahead and behind), > - or "=" (in sync). Has no effect if the ref does not have > - tracking information associated with it. > + or "=" (in sync). Append `:track,nobracket` to show tracking > + information without brackets (i.e "ahead N, behind M"). Has > + no effect if the ref does not have tracking information > + associated with it. > > push:: > The name of a local ref which represents the `@{push}` location > diff --git a/ref-filter.c b/ref-filter.c > index 6a38089..6044eb0 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref) > if (!strcmp(formatp, "short")) > refname = shorten_unambiguous_ref(refname, > warn_ambiguous_refs); > - else if (!strcmp(formatp, "track") && > + else if (skip_prefix(formatp, "track", &valp) && > +strcmp(formatp, "trackshort") && > (starts_with(name, "upstream") || > starts_with(name, "push"))) { If you see here, we detect "track" first for %(upstream:track,nobracket) so although the idea was to use something similar to %(align:...) I don't see a good way of going about this. If we want %(upstream:nobracket,track) to be supported then, I think we'll have to change this whole layout and have the detection done up where we locat "upstream" / "push", what would be a nice way to go around this? What I could think of: 1. Do the cleanup that Junio and Matthieu suggested, where we basically parse the atoms and store them into a used_atom struct. I could either work on those patches before this and then rebase this on top. 2. Let this be and come back on it when implementing the above series. After reading Matthieu's and Junio's discussion, I lean towards the latter. > char buf[40]; > + unsigned int nobracket = 0; > + > + if (!strcmp(valp, ",nobracket")) > + nobracket = 1; > > if (stat_tracking_info(branch, &num_ours, >&num_theirs, NULL)) { > - v->s = "[gone]"; > + if (nobracket) > + v->s = "gone"; > + else > + v->s = "[gone]"; > continue; > } > > if (!num_ours && !num_theirs) > v->s = ""; > else if (!num_ours) { > - sprintf(buf, "[behind %d]", > num_theirs); > + if (nobracket) > + sprintf(buf, "behind %d", > num_theirs); > + else > + sprintf(buf, "[behind %d]", > num_theirs); > v->s = xstrdup(buf); > } else if (!num_theirs) { > - sprintf(buf, "[ahead %d]", num_ours); > + if (nobracket) > + sprintf(buf, "ahead %d", > num_ours); > + else > + sprintf(buf, "[ahead %d]", > num_ours); > v->s = xstrdup(buf); > } else { > - sprintf(buf, "[ahead %d, behind %d]", > + if (nobracket) > +
Re: How to rebase when some commit hashes are in some commit messages
On Tue, Oct 13, 2015 at 1:07 PM, Jacob Keller wrote: > On Tue, Oct 13, 2015 at 6:29 AM, Philip Oakley wrote: >> My tuppence is that the only sha1's that could/would be rewritten would be >> those for the commits within the rebase. During rebasing it is expected that >> the user is re-adjusting things for later upstream consumption, with social >> controls and understandings with colleagues. >> > > Agreed here. There would be no need to change any sha1s that didn't > change during the rebase. This limits the scope. Alright. > >> Thus the only sha1 numbers that could be used are those that are within the >> (possibly implied) instruction sheet (which will list the current sha1s that >> will be converted by rebase to new sha1's). >> > > Correct, you would be able to limit the number of sha1s to search for. > > However, (see below), any reasonable reference to a sha1 should be > relatively stable. > >> It should be clear that the sha1's are always backward references (because >> of the impossibility of including a forward reference to an as yet >> un-created future commit's sha1). >> >> The key question (for me) is whether short sha1s are accepted, or if they >> must be full 40 char sha1's (perhaps an option). There are already options >> for making sure that short refs are not ambiguous. >> >> It sound to me like a sensible small project for those that have such a >> workflow. I'm not sure if it should work with a patch based flow when >> submitting upstream - I'm a little fuzzy on how would the upstream >> maintainer know which sha1 referred to which patch. >> > > My issue: the only sha1s in commit messages are *generally* things > which will NOT be changed in general. Supporting a work flow that > wants to change these is definitely crazy. > > Essentially: I don't see a reason that you would be rebasing a commit > and needing to change any references in it. You can reference a commit > which isn't changing, but here's the possible situations I see: > > a) you are rebasing a commit which references in the message a commit > that is not being changed (it is ancient) > > In this case, nothing needs to be done. > > b) you are rebasing a commit which references another commit in the same > rebase > > I see no valid reason to reference a sha1 in this case. If you're > referencing as a "fixes", then you are being silly since you can just > squash the fix into the original commit and thus prevent introduction > of bug at all. > > What other reason? If you are referencing such as "thix extends > implementation from sha1" then your commit message is probably poorly > formatted. I don't see a reason to support this flow. > > c) you are rebasing a commit which is referencing a commit that has > already been changed. (?) > > I think (maybe) this is your interesting case, but here are some caveats. > > Let's say you are fixing some old commit such as "Fixes: summary, date>" or something. > > If you do a "git pull --rebase", your commit might be updated to play > ontop of more new work, but the sha1 should still be valid, *unless* > the remote history did some rewind, at which point I don't think any > algorithm will work, see the issues above. > > It may be something worth doing in git-filter-branch, but then you're > looking at losing the two assumptions above making it really hard to > get right. > > Regards, > Jake It seems reasonable that this could be added as a feature of interactive rebase. The todo list could be automatically adjusted to "reword" for those commits which are referring to other commits within the same rebase. As each commit is re-written, a mapping could be kept of old sha1 -> new sha1. Then when one of the reworded commits is being applied, the old sha1 -> new sha1 mapping could be used to add a line to the $COMMIT_MSG. -- 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 v2 09/10] branch: use ref-filter printing APIs
On Tue, Oct 13, 2015 at 10:01 PM, Junio C Hamano wrote: > Karthik Nayak writes: > >> Port branch.c to use ref-filter APIs for printing. This clears out >> most of the code used in branch.c for printing and replaces them with >> calls made to the ref-filter library. >> >> Introduce get_format() which gets the format required for printing of >> refs. Make amendments to print_ref_list() to reflect these changes. > > Is it get_format() still? Will change that, thanks :) -- Regards, Karthik Nayak -- 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: How to rebase when some commit hashes are in some commit messages
On Tue, Oct 13, 2015 at 6:29 AM, Philip Oakley wrote: > My tuppence is that the only sha1's that could/would be rewritten would be > those for the commits within the rebase. During rebasing it is expected that > the user is re-adjusting things for later upstream consumption, with social > controls and understandings with colleagues. > Agreed here. There would be no need to change any sha1s that didn't change during the rebase. This limits the scope. Alright. > Thus the only sha1 numbers that could be used are those that are within the > (possibly implied) instruction sheet (which will list the current sha1s that > will be converted by rebase to new sha1's). > Correct, you would be able to limit the number of sha1s to search for. However, (see below), any reasonable reference to a sha1 should be relatively stable. > It should be clear that the sha1's are always backward references (because > of the impossibility of including a forward reference to an as yet > un-created future commit's sha1). > > The key question (for me) is whether short sha1s are accepted, or if they > must be full 40 char sha1's (perhaps an option). There are already options > for making sure that short refs are not ambiguous. > > It sound to me like a sensible small project for those that have such a > workflow. I'm not sure if it should work with a patch based flow when > submitting upstream - I'm a little fuzzy on how would the upstream > maintainer know which sha1 referred to which patch. > My issue: the only sha1s in commit messages are *generally* things which will NOT be changed in general. Supporting a work flow that wants to change these is definitely crazy. Essentially: I don't see a reason that you would be rebasing a commit and needing to change any references in it. You can reference a commit which isn't changing, but here's the possible situations I see: a) you are rebasing a commit which references in the message a commit that is not being changed (it is ancient) In this case, nothing needs to be done. b) you are rebasing a commit which references another commit in the same rebase I see no valid reason to reference a sha1 in this case. If you're referencing as a "fixes", then you are being silly since you can just squash the fix into the original commit and thus prevent introduction of bug at all. What other reason? If you are referencing such as "thix extends implementation from sha1" then your commit message is probably poorly formatted. I don't see a reason to support this flow. c) you are rebasing a commit which is referencing a commit that has already been changed. (?) I think (maybe) this is your interesting case, but here are some caveats. Let's say you are fixing some old commit such as "Fixes: " or something. If you do a "git pull --rebase", your commit might be updated to play ontop of more new work, but the sha1 should still be valid, *unless* the remote history did some rewind, at which point I don't think any algorithm will work, see the issues above. It may be something worth doing in git-filter-branch, but then you're looking at losing the two assumptions above making it really hard to get right. Regards, Jake -- 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 v2 09/10] branch: use ref-filter printing APIs
Karthik Nayak writes: > Port branch.c to use ref-filter APIs for printing. This clears out > most of the code used in branch.c for printing and replaces them with > calls made to the ref-filter library. > > Introduce get_format() which gets the format required for printing of > refs. Make amendments to print_ref_list() to reflect these changes. Is it get_format() still? > > Change calc_maxwidth() to also account for the length of HEAD ref, by > calling ref-filter:get_head_discription(). > > Mentored-by: Christian Couder > Mentored-by: Matthieu Moy > Signed-off-by: Karthik Nayak > --- > builtin/branch.c | 261 > --- > t/t6040-tracking-info.sh | 2 +- > 2 files changed, 66 insertions(+), 197 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index 67ef9f1..4d8b570 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -35,12 +35,12 @@ static unsigned char head_sha1[20]; > > static int branch_use_color = -1; > static char branch_colors[][COLOR_MAXLEN] = { > - GIT_COLOR_RESET, > - GIT_COLOR_NORMAL, /* PLAIN */ > - GIT_COLOR_RED, /* REMOTE */ > - GIT_COLOR_NORMAL, /* LOCAL */ > - GIT_COLOR_GREEN,/* CURRENT */ > - GIT_COLOR_BLUE, /* UPSTREAM */ > + "%(color:reset)", > + "%(color:reset)", /* PLAIN */ > + "%(color:red)", /* REMOTE */ > + "%(color:reset)", /* LOCAL */ > + "%(color:green)", /* CURRENT */ > + "%(color:blue)",/* UPSTREAM */ > }; > enum color_branch { > BRANCH_COLOR_RESET = 0, > @@ -271,192 +271,6 @@ static int delete_branches(int argc, const char **argv, > int force, int kinds, > return(ret); > } > > -static void fill_tracking_info(struct strbuf *stat, const char *branch_name, > - int show_upstream_ref) > -{ > - int ours, theirs; > - char *ref = NULL; > - struct branch *branch = branch_get(branch_name); > - const char *upstream; > - struct strbuf fancy = STRBUF_INIT; > - int upstream_is_gone = 0; > - int added_decoration = 1; > - > - if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) { > - if (!upstream) > - return; > - upstream_is_gone = 1; > - } > - > - if (show_upstream_ref) { > - ref = shorten_unambiguous_ref(upstream, 0); > - if (want_color(branch_use_color)) > - strbuf_addf(&fancy, "%s%s%s", > - branch_get_color(BRANCH_COLOR_UPSTREAM), > - ref, > branch_get_color(BRANCH_COLOR_RESET)); > - else > - strbuf_addstr(&fancy, ref); > - } > - > - if (upstream_is_gone) { > - if (show_upstream_ref) > - strbuf_addf(stat, _("[%s: gone]"), fancy.buf); > - else > - added_decoration = 0; > - } else if (!ours && !theirs) { > - if (show_upstream_ref) > - strbuf_addf(stat, _("[%s]"), fancy.buf); > - else > - added_decoration = 0; > - } else if (!ours) { > - if (show_upstream_ref) > - strbuf_addf(stat, _("[%s: behind %d]"), fancy.buf, > theirs); > - else > - strbuf_addf(stat, _("[behind %d]"), theirs); > - > - } else if (!theirs) { > - if (show_upstream_ref) > - strbuf_addf(stat, _("[%s: ahead %d]"), fancy.buf, ours); > - else > - strbuf_addf(stat, _("[ahead %d]"), ours); > - } else { > - if (show_upstream_ref) > - strbuf_addf(stat, _("[%s: ahead %d, behind %d]"), > - fancy.buf, ours, theirs); > - else > - strbuf_addf(stat, _("[ahead %d, behind %d]"), > - ours, theirs); > - } > - strbuf_release(&fancy); > - if (added_decoration) > - strbuf_addch(stat, ' '); > - free(ref); > -} > - > -static void add_verbose_info(struct strbuf *out, struct ref_array_item *item, > - struct ref_filter *filter, const char *refname) > -{ > - struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT; > - const char *sub = _(" invalid ref "); > - struct commit *commit = item->commit; > - > - if (!parse_commit(commit)) { > - pp_commit_easy(CMIT_FMT_ONELINE, commit, &subject); > - sub = subject.buf; > - } > - > - if (item->kind == FILTER_REFS_BRANCHES) > - fill_tracking_info(&stat, refname, filter->verbose > 1); > - > - strbuf_addf(out, " %s %s%s", > - find_unique_abbrev(item->commit->object.sha1, filter->abbrev), > - stat.buf, sub); > - strbuf_release(&stat); > - strbuf_release(&subject); > -} > - > -/* > - *
[PATCH 1/1] send-email: allow to compose only prepared cover letter
Since @rev_list_opts contains everything that goes to the git format-patch including an additional option like '--cover-letter' we might be interested to compose it before send. My often use case is to do: % git format-patch --cover-letter --subject-prefix="PATCH vN" rev1^..revXYZ % $GIT_EDITOR -* % git send-email 00* # assumes series less than 100 patches % rm -f 00* Since git-send-email may send directly from repository it would be nice to reduce above to just % git send-email --compose --cover-letter --subject-prefix="PATCH vN" rev1^..revXYZ P.S. Going further we can even introduce something like --valid-cmd to send-email to run, for example, checkpatch.pl. Signed-off-by: Andy Shevchenko --- git-send-email.perl | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/git-send-email.perl b/git-send-email.perl index e3ff44b..fc62d28 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -631,7 +631,10 @@ sub get_patch_subject { die "No subject line in $fn ?"; } -if ($compose) { +if ($compose and @rev_list_opts and grep { $_ eq '--cover-letter' } @rev_list_opts) { + # Cover letter always goes first + do_edit($files[0]); +} elsif ($compose) { # Note that this does not need to be secure, but we will make a small # effort to have it be unique $compose_filename = ($repo ? -- 2.5.3 -- 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] Add fetch.recurseSubmoduleParallelism config option
On Tue, Oct 13, 2015 at 12:32 AM, Junio C Hamano wrote: > Stefan Beller writes: > >>> The parallel_process API could learn a new "verbose" feature that it >>> by itself shows some messages like >>> >>> "processing the 'frotz' job with N tasks" >>> "M tasks finished (N still running)" >> >> I know what to fill in for M and N, 'frotz' is a bit unclear to me. > > At least I don't know what M and N should be, and I'm curious how > you'll define them. See below. I presumed the second school of thought. Another alternative there would be to have 3 numbers: "M tasks finished (N still running, K pending output)" > >>> in the output stream from strategic places. For example, the first >>> message will come at the end of pp_init(), and the second message >>> will be appended at the end of buffered output of a task that has >>> just been finished. Once you have something like that, you could >>> check for them in a test in t/. >>> >>> Just a thought. >> >> I like that thought. :) > > > A few more random thoughts: > > * The only thing you could rely on if you were to use the above in >your tests is the one from pp_init() that declares how many >processes the machinery is going to use. M/N will be unstable, >depending on the scheduling order (e.g. the foreground process >may take a lot of time to finish, while many other processes >finish first). > > * Every time the foreground process (i.e. the one whose output is >tee-ed to the overall output from the machinery) finishes, you >can emit "M tasks finished (N still running)", but I am not sure >what M should be. It is debatable how to account for background >processes that have already completed but whose output haven't >been shown. Assuming we go with your second school of thought (N are the real running processes, M including the finished but still pending output tasks), we may have confusing output, as the output order may confuse the reader: N=8 M=13 (output from live task) ... N=8 M=12 (output from buffered task) ... Anyone who has no knowledge of the internals, wonders why M goes *down* ? > >One school of thought that is in line with the "pretend as if the >background tasks are started immediately after the foreground >task finishes, and they run at infinite speed and produce output >in no time" idea, on which the "queue output from the background >processes and emit at once in order to avoid intermixing" design >was based on, would be not to include them in M (i.e. finished >ones), because their output haven't been emitted and we are >pretending that they haven't even been started. If you take this >approach, you however may have to include them in N (i.e. still >running), but that would likely bump N beyond the maximum number >of simultaneous processes. > >The other school of thought would of course tell the truth and >include the number of finished background processes in M, as they >have finished already in the reality. This will not risk showing >N that is beyond the maximum, but your first "progress" output >might say "3 tasks finished", which will make it look odd in a >different way. > -- 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: [Suggestion] add alt texts to the images of the git book
Hi Lucas, On 2015-10-13 15:53, Lucas Radaelli wrote: > the book at https://git-scm.com/book/en/v2 (which is awesome btw, > thanks for that!), contains a lot of images that are inaccessible to > me because I am visually impaired. Not sure if they are really really > important, but in some cases it was a little bit complicated to > follow some examples, more specifically, in the branching chapter. Please find the "The source of this book is hosted on GitHub" link on the left side of the page you linked to. You are cordially invited to contribute patches, suggestions and comments in that project's bug tracker. Ciao, Johannes -- 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
[Suggestion] add alt texts to the images of the git book
Hey there, the book at https://git-scm.com/book/en/v2 (which is awesome btw, thanks for that!), contains a lot of images that are inaccessible to me because I am visually impaired. Not sure if they are really really important, but in some cases it was a little bit complicated to follow some examples, more specifically, in the branching chapter. Some outputs were pasted as plain text, which was much easier to follow, and I wonder if would be possible in the future to have all outputs pasted as plain text for easier reading? Thanks. -- 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: How to rebase when some commit hashes are in some commit messages
From: "Konstantin Khomoutov" On Tue, 13 Oct 2015 10:50:40 +0200 Francois-Xavier Le Bail wrote: >> For example, if I rebase the following commits, I would want that >> if the commit hash 222... become 777..., >> the message >> "Update test output for " >> become >> "Update test output for 777..." >> >> Is it possible currently? And if yes how? > > AFAIK, it's not possible other than by editing the message by hand. It seems to me useful to be able to do it. Can we hope a new option? How do you think this could be practically implemented? A couple of things which immediately spring to my mind: To begin with, you are free to specify just a few first characters of the commit name you're referring to. So the alogrythm which finds the relevant commits them has to be smart to somehow avoid misfires. Or have knobs to tune it (like -M of `git log`). OK, suppose that this is solved through the usage of some agreed-upon keywords in the commit message. Say, you adopt a policy to put something like X-Refers-To: 2dd8a9d9bb33ebffccb2ff516497adc8535bcab4 in your commit message to make the finder tool happy. Now think how exactly it should work. First, any commit at all might mention the name of the target commit in its commit message. Okay, let's suppose there will be some way to somehow prune the possible DAG down. Then what happens if the commit to change is a part of the chain of commits reachable from some branch other than that you're rebasing? Automatically rebasing it would rewrite that commits and all commits "after" it -- possibly resulting in what the "Recovering from upstream rebase" part of the git-rebase(1) manual page deals with. Having said that, the feature you're after appears to me to be a sensible thing to have but the possibility of its generic implementation appears to be moot. Note that to deal with narrow simple cases (all possibly affected commits leave on the same branch you're rebasing, and come later than the rebase's anchor) you could write a script which uses `git log` to find those commits which need special care. My tuppence is that the only sha1's that could/would be rewritten would be those for the commits within the rebase. During rebasing it is expected that the user is re-adjusting things for later upstream consumption, with social controls and understandings with colleagues. Thus the only sha1 numbers that could be used are those that are within the (possibly implied) instruction sheet (which will list the current sha1s that will be converted by rebase to new sha1's). It should be clear that the sha1's are always backward references (because of the impossibility of including a forward reference to an as yet un-created future commit's sha1). The key question (for me) is whether short sha1s are accepted, or if they must be full 40 char sha1's (perhaps an option). There are already options for making sure that short refs are not ambiguous. It sound to me like a sensible small project for those that have such a workflow. I'm not sure if it should work with a patch based flow when submitting upstream - I'm a little fuzzy on how would the upstream maintainer know which sha1 referred to which patch. Philip -- 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: How to rebase when some commit hashes are in some commit messages
On Tue, 13 Oct 2015 10:50:40 +0200 Francois-Xavier Le Bail wrote: > >> For example, if I rebase the following commits, I would want that > >> if the commit hash 222... become 777..., > >> the message > >> "Update test output for " > >> become > >> "Update test output for 777..." > >> > >> Is it possible currently? And if yes how? > > > > AFAIK, it's not possible other than by editing the message by hand. > > It seems to me useful to be able to do it. Can we hope a new option? How do you think this could be practically implemented? A couple of things which immediately spring to my mind: To begin with, you are free to specify just a few first characters of the commit name you're referring to. So the alogrythm which finds the relevant commits them has to be smart to somehow avoid misfires. Or have knobs to tune it (like -M of `git log`). OK, suppose that this is solved through the usage of some agreed-upon keywords in the commit message. Say, you adopt a policy to put something like X-Refers-To: 2dd8a9d9bb33ebffccb2ff516497adc8535bcab4 in your commit message to make the finder tool happy. Now think how exactly it should work. First, any commit at all might mention the name of the target commit in its commit message. Okay, let's suppose there will be some way to somehow prune the possible DAG down. Then what happens if the commit to change is a part of the chain of commits reachable from some branch other than that you're rebasing? Automatically rebasing it would rewrite that commits and all commits "after" it -- possibly resulting in what the "Recovering from upstream rebase" part of the git-rebase(1) manual page deals with. Having said that, the feature you're after appears to me to be a sensible thing to have but the possibility of its generic implementation appears to be moot. Note that to deal with narrow simple cases (all possibly affected commits leave on the same branch you're rebasing, and come later than the rebase's anchor) you could write a script which uses `git log` to find those commits which need special care. -- 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 0/2] Reinstate the helpful message when `git pull --rebase` fails
Hi Junio, On 2015-10-12 22:28, Junio C Hamano wrote: > Johannes Schindelin writes: > >>> I think the most sensible regression fix as the first step at this >>> point is to call it as a separate process, just like the code calls >>> "apply" as a separate process for each patch. Optimization can come >>> later when it is shown that it matters---we need to regain >>> correctness first. >> >> I fear that you might underestimate the finality of this "first >> step". If you reintroduce that separate process, not only is it a >> performance regression, but could we really realistically expect any >> further steps to happen after that? I do not think so. >> ... >> For the above reasons, I respectfully remain convinced that >> reintroducing the separate process would be a mistake. > > I am not saying we should forever do run_command() going forward. Fine, I will stop arguing about this and go back grumble in my corner. Ciao, Dscho -- 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 01/13] refs: convert some internal functions to use object_id
On 10/09/2015 03:43 AM, brian m. carlson wrote: > Convert several internal functions in refs.c to use struct object_id, > and use the GIT_SHA1_HEXSZ constants in parse_ref_line. > > Signed-off-by: brian m. carlson > [...] I looked over this patch at the diff level and didn't find any problems. This patch conflicts heavily with [1]. But I noticed that it is independent of the rest of your series. I don't know when either patch series will be ready, but see [2] for my take on the other one. Assuming neither series is rejected, I think it would be much easier to redo this patch on top of the first part of [1] than vice versa, so that would be my suggestion. If it comes down to that, I am willing to help redo this patch if you like. Michael [1] http://thread.gmane.org/gmane.comp.version-control.git/279423 [2] http://article.gmane.org/gmane.comp.version-control.git/279496 -- Michael Haggerty mhag...@alum.mit.edu -- 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 18/44] refs: move transaction functions into common code
I reviewed the patches up to here pretty carefully, and aside from the comments I already sent, they look good. I like the new approach where the ref_transaction-building code is shared across backends. It seems to me that a good breaking point for the first batch of patches would be here, just before you start creating the VTABLE in commit [19/44]. The patches before this point all have to do with moving code around and a little bit of light refactoring. They cause a lot of code churn that will soon conflict with other people's work (e.g., [1]), but I think they are pretty uncontroversial. After this you start making a few important design decisions that *could* be controversial. Therefore, by making a cut, you can maximize the chance that the earlier patches can be merged to master relatively quickly, after which the cross section for future conflicts will be much smaller. Ideally, you would include a few later patches in the "pre-VTABLE" patch series. It looks like the following patches also mostly have to do with moving code around, and would fit logically with the "pre-VTABLE" changes: [24] refs.c: move refname_is_safe to the common code [25] refs.h: document make refname_is_safe and add it to header [26] refs.c: move copy_msg to the common code [27] refs.c: move peel_object to the common code [28] refs.c: move should_autocreate_reflog to common code [32] initdb: move safe_create_dir into common code [36] refs: make files_log_ref_write functions public [37] refs: break out a ref conflict check I tried rebasing those commits on top of your patch 18 and it wasn't too bad. The result is on branch "refs-backend-pre-vtable" on my GitHub repo [2], including my suggested changes to those eight patches (which therefore became seven because I squashed the first two together). Michael [1] http://article.gmane.org/gmane.comp.version-control.git/279286 [2] https://github.com/mhagger/git -- Michael Haggerty mhag...@alum.mit.edu -- 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 1/3] Add Travis CI support
Le 11/10/2015 19:55, larsxschnei...@gmail.com a écrit : > + > +before_script: make > + > +script: make --quiet test Travis can be used in container mode but that would need getting rid of "sudo" command and only installing from white-listed sources (https://github.com/travis-ci/apt-source-whitelist/blob/master/ubuntu.json) Anyway, even within the present VM mode, 1.5 cores are available, so it makes sense to add "-j2" to every make commands. -- 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 37/44] refs: break out a ref conflict check
On 10/12/2015 11:51 PM, David Turner wrote: > Create new function verify_no_descendants, to hold one of the ref > conflict checks used in verify_refname_available. Multiple backends > will need this function, so it goes in the common code. > > Signed-off-by: David Turner > --- > refs-be-files.c | 33 - > refs.c | 22 ++ > refs.h | 7 +++ > 3 files changed, 37 insertions(+), 25 deletions(-) > > [...] > diff --git a/refs.c b/refs.c > index 5a3125d..3ae0274 100644 > --- a/refs.c > +++ b/refs.c > @@ -1101,6 +1101,28 @@ enum peel_status peel_object(const unsigned char > *name, unsigned char *sha1) > return PEEL_PEELED; > } > > +const char *find_descendant_ref(const char *refname, > + const struct string_list *extras, > + const struct string_list *skip) > +{ > + int pos; > + if (!extras) > + return NULL; > + > + /* Look for the place where "$refname/" would be inserted in extras. */ The comment above is misleading. See my note at the bottom of this email for an explanation. > + for (pos = string_list_find_insert_index(extras, refname, 0); > + pos < extras->nr; pos++) { > + const char *extra_refname = extras->items[pos].string; > + > + if (!starts_with(extra_refname, refname)) > + break; > + > + if (!skip || !string_list_has_string(skip, extra_refname)) > + return extra_refname; > + } > + return NULL; > +} > + > /* backend functions */ > int refs_initdb(struct strbuf *err, int shared) > { > diff --git a/refs.h b/refs.h > index 3aad3b8..f8becea 100644 > --- a/refs.h > +++ b/refs.h > @@ -637,6 +637,13 @@ int files_log_ref_write(const char *refname, const > unsigned char *old_sha1, > const unsigned char *new_sha1, const char *msg, > int flags, struct strbuf *err); > > +/* > + * Check for entries in extras that start with "$refname/", ignoring > + * those in skip. If there is such an entry, then we have a conflict. > + */ > +const char *find_descendant_ref(const char *refname, > + const struct string_list *extras, > + const struct string_list *skip); The documentation is is not correct. As written, the function needs not the refname as argument but the refname followed by '/'. Without the trailing slash, "refs/heads/foo" would be blocked by "refs/heads/foobar". >From the point of view of simplicity, it would be nice if this function could take a refname (without the trailing slash) as argument. But then it would basically be forced to allocate a new string, copy refname to it, append a '/', then free the string again when it's done. Alternatively, it could accept its refname argument in a strbuf and temporarily append the '/'. But neither one of these alternatives is so elegant, so maybe it's OK if this function is documented to take a "directory name, including the trailing '/'" as argument and rename the argument (e.g., to "dirname"). Please also document that "skip" and "extras" can be NULL, but if not NULL they need to be sorted lists. > [...] Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 27/44] refs.c: move peel_object to the common code
On 10/12/2015 11:51 PM, David Turner wrote: > This function does not contain any backend specific code so we > move it to the common code. > > Signed-off-by: David Turner > --- > refs-be-files.c | 53 - > refs.c | 31 +++ > refs.h | 27 +++ > 3 files changed, 58 insertions(+), 53 deletions(-) > > [...] > diff --git a/refs.c b/refs.c > index bd8c71b..99b31f6 100644 > --- a/refs.c > +++ b/refs.c > @@ -4,6 +4,9 @@ > #include "cache.h" > #include "refs.h" > #include "lockfile.h" > +#include "object.h" > +#include "tag.h" > + > /* > * We always have a files backend and it is the default. > */ > @@ -1059,6 +1062,34 @@ int refname_is_safe(const char *refname) > return 1; > } > > +/* > + * Peel the named object; i.e., if the object is a tag, resolve the > + * tag recursively until a non-tag is found. If successful, store the > + * result to sha1 and return PEEL_PEELED. If the object is not a tag > + * or is not valid, return PEEL_NON_TAG or PEEL_INVALID, respectively, > + * and leave sha1 unchanged. > + */ Please move the docstring to the header file. > [...] > diff --git a/refs.h b/refs.h > index 3da5d09..636f959 100644 > --- a/refs.h > +++ b/refs.h > @@ -76,6 +76,33 @@ extern int is_branch(const char *refname); > */ > extern int peel_ref(const char *refname, unsigned char *sha1); > > +enum peel_status { > + /* object was peeled successfully: */ > + PEEL_PEELED = 0, > + > + /* > + * object cannot be peeled because the named object (or an > + * object referred to by a tag in the peel chain), does not > + * exist. > + */ > + PEEL_INVALID = -1, > + > + /* object cannot be peeled because it is not a tag: */ > + PEEL_NON_TAG = -2, > + > + /* ref_entry contains no peeled value because it is a symref: */ > + PEEL_IS_SYMREF = -3, > + > + /* > + * ref_entry cannot be peeled because it is broken (i.e., the > + * symbolic reference cannot even be resolved to an object > + * name): > + */ > + PEEL_BROKEN = -4 > +}; > + > +enum peel_status peel_object(const unsigned char *name, unsigned char *sha1); > + > /** > * Resolve refname in the nested "gitlink" repository that is located > * at path. If the resolution is successful, return 0 and set sha1 to > Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 26/44] refs.c: move copy_msg to the common code
On 10/12/2015 11:51 PM, David Turner wrote: > Rename copy_msg to copy_reflog_msg and make it public. > > Signed-off-by: David Turner > --- > refs-be-files.c | 28 +--- > refs.c | 26 ++ > refs.h | 2 ++ > 3 files changed, 29 insertions(+), 27 deletions(-) > > [...] > diff --git a/refs.c b/refs.c > index 2515f6e..bd8c71b 100644 > --- a/refs.c > +++ b/refs.c > @@ -886,6 +886,32 @@ int for_each_remote_ref_submodule(const char *submodule, > each_ref_fn fn, void *c > return for_each_ref_in_submodule(submodule, "refs/remotes/", fn, > cb_data); > } > > +/* > + * copy the reflog message msg to buf, which has been allocated sufficiently > + * large, while cleaning up the whitespaces. Especially, convert LF to > space, > + * because reflog file is one line per entry. > + */ Please put the docstring by the declaration in the header file. > [...] > diff --git a/refs.h b/refs.h > index 7a936e2..3da5d09 100644 > --- a/refs.h > +++ b/refs.h > @@ -597,6 +597,8 @@ enum ref_type { > > enum ref_type ref_type(const char *refname); > > +int copy_reflog_msg(char *buf, const char *msg); > + > enum expire_reflog_flags { > EXPIRE_REFLOGS_DRY_RUN = 1 << 0, > EXPIRE_REFLOGS_UPDATE_REF = 1 << 1, > Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 25/44] refs.h: document make refname_is_safe and add it to header
On 10/13/2015 09:33 AM, Michael Haggerty wrote: > On 10/12/2015 11:51 PM, David Turner wrote: >> This function might be used by other refs backends >> >> Signed-off-by: David Turner >> --- >> refs.h | 11 +++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/refs.h b/refs.h >> index fc8a748..7a936e2 100644 >> --- a/refs.h >> +++ b/refs.h >> @@ -348,6 +348,17 @@ int verify_refname_available(const char *newname, >> struct string_list *extra, >> struct string_list *skip, struct strbuf *err); >> >> /* >> + * Check if a refname is safe. >> + * For refs that start with "refs/" we consider it safe as long they do >> + * not try to resolve to outside of refs/. >> + * >> + * For all other refs we only consider them safe iff they only contain >> + * upper case characters and '_' (like "HEAD" AND "MERGE_HEAD", and not like >> + * "config"). >> + */ >> +int refname_is_safe(const char *refname); >> + >> +/* >> * Flags controlling ref_transaction_update(), ref_transaction_create(), >> etc. >> * REF_NODEREF: act on the ref directly, instead of dereferencing >> * symbolic references. >> > > The previous commit deleted this comment from where it previously > appeared in refs-be-files.c. It would make more sense to squash this > commit onto that one so it's clear that you are moving the comment > rather than creating a new comment out of thin air. Also, after this commit the prototype for this function appears twice in refs.h. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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: How to rebase when some commit hashes are in some commit messages
On 12/10/2015 22:21, Matthieu Moy wrote: > Francois-Xavier Le Bail writes: > >> Hello, >> >> [I try some search engines without success, perhaps I have missed something]. >> >> For example, if I rebase the following commits, I would want that if >> the commit hash 222... become 777..., >> the message >> "Update test output for " >> become >> "Update test output for 777..." >> >> Is it possible currently? And if yes how? > > AFAIK, it's not possible other than by editing the message by hand. It seems to me useful to be able to do it. Can we hope a new option? -- 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 19/44] refs-be-files.c: add a backend method structure with transaction functions
On 10/12/2015 11:51 PM, David Turner wrote: > From: Ronnie Sahlberg > > Add a ref structure for backend methods. Start by adding a method pointer > for the transaction commit function. > > Add a function set_refs_backend to switch between backends. The files > based backend is the default. > > Signed-off-by: Ronnie Sahlberg > Signed-off-by: David Turner > --- > refs-be-files.c | 10 -- > refs.c | 30 ++ > refs.h | 15 +++ > 3 files changed, 53 insertions(+), 2 deletions(-) > > [...] > diff --git a/refs.h b/refs.h > index 4940ae9..419abf4 100644 > --- a/refs.h > +++ b/refs.h > @@ -619,4 +619,19 @@ extern int reflog_expire(const char *refname, const > unsigned char *sha1, >reflog_expiry_cleanup_fn cleanup_fn, >void *policy_cb_data); > > +/* refs backends */ > +typedef int ref_transaction_commit_fn(struct ref_transaction *transaction, > + struct strbuf *err); > +typedef void ref_transaction_free_fn(struct ref_transaction *transaction); The ref_transaction_free_fn typedef isn't used anywhere. > +struct ref_be { > + struct ref_be *next; > + const char *name; > + ref_transaction_commit_fn *transaction_commit; > +}; > + > + > +extern struct ref_be refs_be_files; I don't think that refs_be_files is needed in the public interface. > +int set_refs_backend(const char *name); > + > #endif /* REFS_H */ Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 20/44] refs-be-files.c: add methods for misc ref operations
On 10/12/2015 11:51 PM, David Turner wrote: > From: Ronnie Sahlberg > > Add ref backend methods for: > resolve_ref_unsafe, verify_refname_available, pack_refs, peel_ref, > create_symref, resolve_gitlink_ref. > > Signed-off-by: Ronnie Sahlberg > Signed-off-by: David Turner > --- > builtin/init-db.c | 1 + > cache.h | 7 +++ > refs-be-files.c | 36 +++- > refs.c| 47 +++ > refs.h| 18 ++ > 5 files changed, 100 insertions(+), 9 deletions(-) This commit doesn't build: refs-be-files.c:3636:2: error: initialization from incompatible pointer type [-Werror] files_create_symref, ^ refs-be-files.c:3636:2: error: (near initialization for ‘refs_be_files.create_symref’) [-Werror] Apparently one of the hunks from the subsequent commit is needed in this commit. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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: Solaris: Fail to build git with CFLAGS=-m64
evgeny litvinenko writes: > Solaris has the following prototype in the file /usr/include/arpa/inet.h: > > extern const char *inet_ntop(int, const void *_RESTRICT_KYWD, char > *_RESTRICT_KYWD, socklen_t); > > Git's prototype for inet_ntop is in file git-compat-util.h: > > #ifdef NO_INET_NTOP > const char *inet_ntop(int af, const void *src, char *dst, size_t size); > #endif This tells me that it is designed to be used only when the platform does not offer inet_ntop(). If your platform does have it, why is your build define NO_INET_NTOP? Perhaps configure generated by autoconf is faulty? In this project, use of autoconf/configure is optional---in other words, it is expected that you can build successfully by setting and unsetting necessary Makefile macros in your own config.mak without using autoconf/configure at all. I'd try without configure and make sure I do not define NO_INET_NTOP (and if you have inet_pton(), then make sure you do not define NO_INET_PTON either) in config.mak if I were you. -- 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 25/44] refs.h: document make refname_is_safe and add it to header
On 10/12/2015 11:51 PM, David Turner wrote: > This function might be used by other refs backends > > Signed-off-by: David Turner > --- > refs.h | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/refs.h b/refs.h > index fc8a748..7a936e2 100644 > --- a/refs.h > +++ b/refs.h > @@ -348,6 +348,17 @@ int verify_refname_available(const char *newname, struct > string_list *extra, >struct string_list *skip, struct strbuf *err); > > /* > + * Check if a refname is safe. > + * For refs that start with "refs/" we consider it safe as long they do > + * not try to resolve to outside of refs/. > + * > + * For all other refs we only consider them safe iff they only contain > + * upper case characters and '_' (like "HEAD" AND "MERGE_HEAD", and not like > + * "config"). > + */ > +int refname_is_safe(const char *refname); > + > +/* > * Flags controlling ref_transaction_update(), ref_transaction_create(), etc. > * REF_NODEREF: act on the ref directly, instead of dereferencing > * symbolic references. > The previous commit deleted this comment from where it previously appeared in refs-be-files.c. It would make more sense to squash this commit onto that one so it's clear that you are moving the comment rather than creating a new comment out of thin air. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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] Add fetch.recurseSubmoduleParallelism config option
Stefan Beller writes: >> The parallel_process API could learn a new "verbose" feature that it >> by itself shows some messages like >> >> "processing the 'frotz' job with N tasks" >> "M tasks finished (N still running)" > > I know what to fill in for M and N, 'frotz' is a bit unclear to me. At least I don't know what M and N should be, and I'm curious how you'll define them. See below. >> in the output stream from strategic places. For example, the first >> message will come at the end of pp_init(), and the second message >> will be appended at the end of buffered output of a task that has >> just been finished. Once you have something like that, you could >> check for them in a test in t/. >> >> Just a thought. > > I like that thought. :) A few more random thoughts: * The only thing you could rely on if you were to use the above in your tests is the one from pp_init() that declares how many processes the machinery is going to use. M/N will be unstable, depending on the scheduling order (e.g. the foreground process may take a lot of time to finish, while many other processes finish first). * Every time the foreground process (i.e. the one whose output is tee-ed to the overall output from the machinery) finishes, you can emit "M tasks finished (N still running)", but I am not sure what M should be. It is debatable how to account for background processes that have already completed but whose output haven't been shown. One school of thought that is in line with the "pretend as if the background tasks are started immediately after the foreground task finishes, and they run at infinite speed and produce output in no time" idea, on which the "queue output from the background processes and emit at once in order to avoid intermixing" design was based on, would be not to include them in M (i.e. finished ones), because their output haven't been emitted and we are pretending that they haven't even been started. If you take this approach, you however may have to include them in N (i.e. still running), but that would likely bump N beyond the maximum number of simultaneous processes. The other school of thought would of course tell the truth and include the number of finished background processes in M, as they have finished already in the reality. This will not risk showing N that is beyond the maximum, but your first "progress" output might say "3 tasks finished", which will make it look odd in a different way. -- 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 02/44] refs: make repack_without_refs and is_branch public
On 10/13/2015 04:39 AM, Michael Haggerty wrote: > On 10/12/2015 11:51 PM, David Turner wrote: >> is_branch was already non-static, but this patch declares it in the >> header. >> >> Signed-off-by: Ronnie Sahlberg >> Signed-off-by: David Turner >> --- >> [...] > > It seems odd that repack_without_refs() should be made public (and > ultimately end up in refs.h) given that it intrinsically only has to do > with file-based references. But I will read on... I think the reason you needed to do this was because you wanted to move delete_refs() to the common code. It is true that delete_ref() can be moved to the common code. And most of the code in delete_refs() is just a loop calling delete_ref(). But delete_refs() also does the very files-specific optimization of calling repack_without_refs() before the loop. *That* call shouldn't be in the common code. So my suggestion is that you write a common_delete_refs() function that only includes the loop over delete_ref(), and a files_delete_refs() function that is basically { result = repack_without_refs(refnames, &err); if (result) { ...report error... return result; } return common_delete_refs(...); } Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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