* Re: Race condition on `git checkout -c`
From: Chris Torek @ 2023-01-19 22:55 UTC (permalink / raw)
To: Arthur Milchior; +Cc: git
In-Reply-To: <CAEcbrFdE6X6=ppBWmFZrm0Z2RqGqFatjNHdZbGb_RMteCk6P6g@mail.gmail.com>
(Top note: you mean `git checkout -b` or `git switch -c`, not `git
checkout -c`.)
On Thu, Jan 19, 2023 at 1:24 PM Arthur Milchior
<arthur.milchior@gmail.com> wrote:
>
> I expect either:
> * to see an error message stating that `b` already exists
> * to see `b` and `B` in the list of branch.
[snip]
> uname: Darwin 22.2.0 Darwin Kernel Version 22.2.0: Fri Nov 11 02:03:51
Darwin (macOS) is your problem here. The same problem
occurs on Windows, but not on Linux, by default.
Technically the problem is in the file system itself, combined with
the ways (plural) that Git stores branch names.
As far as Git itself is concerned, branch names are *always* case
sensitive, so branches named `b` and `B` are different. But Git
stores branch names in two different formats, at the moment:
* Some are kept in a plain-text file `.git/packed-refs`.
* Some are stored as directory-and-file-names in `.git/refs/`.
If the OS's file system is case sensitive, as most standard Linux
file systems are, there's no problem with the latter. But if the OS's
file system is case-INsensitive, as the default file systems on
Windows and MacOS are, this becomes a problem: the attempt
to create both `refs/heads/b` and a different file, `refs/head/B`,
fails, with one of the two names "winning" and the other attempt
to create a new name simply re-using the existing name.
If you create a case-sensitive disk volume on your Mac, which
can be a simple click-to-mount virtual drive within your regular
case-insensitive file system, you can happily use Git without this
complication. Note that the same complication applies to file
names: Linux users can create two different, separate files
named `README.TXT` and `ReadMe.txt` in a GIt project, and
if you use the default case-insensitive macOS file system, you
won't be able to check out both files. Using your case sensitive
volume will allow you to work with the Linux group.
If and when a future version of Git starts using reftables instead
of the file system to store branch and tag names, this particular
issue will go away, but until then, I recommend keeping a case
sensitive volume handy on your mac, and more generally,
avoiding mixing upper and lower case branch and/or file names
(at all, ever) whenever possible. This avoids a lot of problems,
though nothing can get you past the Windows `aux.h` problem. :-)
Chris
^ permalink raw reply
* [PATCH 5/5] format-patch: Add support for --{to,cc}-cmd flags
From: Zev Weiss @ 2023-01-19 22:38 UTC (permalink / raw)
To: git; +Cc: Zev Weiss, Denton Liu, Eric Sunshine, Junio C Hamano
In-Reply-To: <20230119223858.29262-1-zev@bewilderbeest.net>
Having these flags available for format-patch instead of only on
send-email makes it much easier to use an automated command to do the
bulk of the recipient-selection work but still manually adjust it if
desired.
Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
builtin/log.c | 10 +++
log-tree.c | 179 +++++++++++++++++++++++++++++++++++++++-
revision.h | 2 +
t/t4014-format-patch.sh | 19 +++++
4 files changed, 208 insertions(+), 2 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index c0c7b8544d73..da3edb2a8299 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1877,6 +1877,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
struct strbuf rdiff2 = STRBUF_INIT;
struct strbuf rdiff_title = STRBUF_INIT;
int creation_factor = -1;
+ char *to_cmd_arg = NULL;
+ char *cc_cmd_arg = NULL;
const struct option builtin_format_patch_options[] = {
OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
@@ -1929,6 +1931,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
N_("add email header"), header_callback),
OPT_CALLBACK(0, "to", NULL, N_("email"), N_("add To: header"), to_callback),
OPT_CALLBACK(0, "cc", NULL, N_("email"), N_("add Cc: header"), cc_callback),
+ OPT_STRING(0, "to-cmd", &to_cmd_arg, N_("command"),
+ N_("command to generate To: addresses for a patch")),
+ OPT_STRING(0, "cc-cmd", &cc_cmd_arg, N_("command"),
+ N_("command to generate Cc: addresses for a patch")),
OPT_CALLBACK_F(0, "from", &from, N_("ident"),
N_("set From address to <ident> (or committer ident if absent)"),
PARSE_OPT_OPTARG, from_callback),
@@ -2031,6 +2037,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
rev.to_recipients = &extra_to;
rev.cc_recipients = &extra_cc;
+
+ rev.to_cmd = to_cmd_arg;
+ rev.cc_cmd = cc_cmd_arg;
+
rev.extra_headers = to_free = strbuf_detach(&buf, NULL);
if (from) {
diff --git a/log-tree.c b/log-tree.c
index 7aa2841dd803..6fdc3a3ffb7f 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -20,6 +20,7 @@
#include "help.h"
#include "range-diff.h"
#include "strmap.h"
+#include "run-command.h"
static struct decoration name_decoration = { "object names" };
static int decoration_loaded;
@@ -652,11 +653,175 @@ static void next_commentary_block(struct rev_info *opt, struct strbuf *sb)
opt->shown_dashes = 1;
}
+/*
+ * Aims to mirror git-send-email.perl's function of the same name, returning a
+ * malloc()ed string that the caller must free().
+ */
+static char *sanitize_address(const struct rev_info *opt, const char *entry)
+{
+ int escaped = 0;
+ const char *inp;
+ char *tmp, *outp;
+ struct strbuf addr = STRBUF_INIT;
+ struct pretty_print_context ctx = {0};
+
+ /* Skip over any leading whitespace */
+ while (isspace(*entry))
+ entry++;
+
+ outp = tmp = xmalloc(strlen(entry) + 1);
+
+ /* Remove non-escaped quotes */
+ for (inp = entry; *inp; inp++) {
+ if (escaped) {
+ escaped = 0;
+ } else {
+ switch (*inp) {
+ case '\\':
+ escaped = 1;
+ /* fallthrough */
+ case '"':
+ continue;
+ }
+ }
+ *outp++ = *inp;
+ }
+ *outp = '\0';
+
+ ctx.fmt = opt->commit_format;
+ ctx.mailmap = opt->mailmap;
+ ctx.encode_email_headers = opt->encode_email_headers;
+ ctx.output_encoding = get_log_output_encoding();
+ ctx.name_and_address_only = 1;
+
+ pp_user_info(&ctx, NULL, &addr, tmp, get_log_output_encoding());
+
+ free(tmp);
+
+ return strbuf_detach(&addr, NULL);
+}
+
+/*
+ * Given 'text' as the output of a --to-cmd or --cc-cmd command, add each
+ * entry to 'list'.
+ */
+static void ingest_recipients_to_list(const char *text, const struct rev_info *opt,
+ struct string_list *list)
+{
+ struct string_list_item *item;
+ struct string_list lines = STRING_LIST_INIT_DUP;
+
+ string_list_split(&lines, text, '\n', -1);
+
+ for_each_string_list_item(item, &lines) {
+ char *addr = sanitize_address(opt, item->string);
+ if (*addr)
+ string_list_append_nodup(list, addr);
+ else
+ free(addr);
+ }
+
+ string_list_clear(&lines, 0);
+}
+
+/*
+ * Generate a temporary patch file for the given commit, returning its path as
+ * a malloc()ed string the caller must free (and should unlink when finished
+ * with it).
+ */
+static char *generate_temp_patch(struct commit *commit)
+{
+ char path[PATH_MAX];
+ char *diff_output_arg;
+ struct strbuf diff_output_arg_buf = STRBUF_INIT;
+ struct child_process diffproc = CHILD_PROCESS_INIT;
+
+ xsnprintf(path, sizeof(path), ".git-temp-diff.XXXXXX");
+ close(xmkstemp(path));
+
+ strbuf_addf(&diff_output_arg_buf, "--output=%s", path);
+ diff_output_arg = strbuf_detach(&diff_output_arg_buf, NULL);
+
+ diffproc.git_cmd = 1;
+ strvec_push(&diffproc.args, "format-patch");
+ strvec_push(&diffproc.args, "-1");
+ strvec_push(&diffproc.args, diff_output_arg);
+ strvec_push(&diffproc.args, oid_to_hex(&commit->object.oid));
+
+ if (run_command(&diffproc))
+ die(_("Error generating temporary diff"));
+
+ free(diff_output_arg);
+
+ return xstrdup(path);
+}
+
+/*
+ * Execute a --to-cmd or --cc-cmd command on a temporary patch file, adding
+ * each recipient produced to 'list'.
+ */
+static void run_recipients_command(const char *cmd, const char *tmpdiff,
+ const struct rev_info *opt,
+ struct string_list *list)
+{
+ char *full_cmd;
+ char *cmd_output;
+ struct strbuf cmd_output_buf = STRBUF_INIT;
+ struct strbuf cmd_buf = STRBUF_INIT;
+ struct child_process cmdproc = CHILD_PROCESS_INIT;
+
+ strbuf_addf(&cmd_buf, "%s %s", cmd, tmpdiff);
+ full_cmd = strbuf_detach(&cmd_buf, NULL);
+
+ cmdproc.use_shell = 1;
+ strvec_push(&cmdproc.args, full_cmd);
+ if (capture_command(&cmdproc, &cmd_output_buf, 1024))
+ die(_("Error generating recipients list: command failed"));
+
+ cmd_output = strbuf_detach(&cmd_output_buf, NULL);
+ ingest_recipients_to_list(cmd_output, opt, list);
+
+ free(cmd_output);
+ free(full_cmd);
+}
+
+/*
+ * Generate a To or Cc header into 'buf', where 'header' is "To" or "Cc",
+ * 'fixed' is the list of fixed recipients (e.g. those specified by --to or
+ * --cc, optional), 'cmd' is the (optional) --to-cmd or --cc-cmd argument to
+ * generate recipients, and 'tmpdiff' is the path of a temp file containing a
+ * patch file to run 'cmd' on (mandatory if 'cmd' is non-NULL).
+ */
+static void headerize_recipients(const char *header, const char *tmpdiff,
+ const struct rev_info *opt,
+ const struct string_list *fixed,
+ const char *cmd, struct strbuf *buf)
+{
+ struct string_list_item *item;
+ struct string_list recipients = STRING_LIST_INIT_DUP;
+
+ if (fixed) {
+ for_each_string_list_item(item, fixed)
+ string_list_append(&recipients, item->string);
+ }
+
+ if (cmd)
+ run_recipients_command(cmd, tmpdiff, opt, &recipients);
+
+ string_list_sort(&recipients);
+ string_list_remove_duplicates(&recipients, 0);
+
+ recipients_to_header_buf(header, buf, &recipients);
+
+ string_list_clear(&recipients, 0);
+}
+
void show_log(struct rev_info *opt)
{
struct strbuf msgbuf = STRBUF_INIT;
struct strbuf hdrbuf = STRBUF_INIT;
struct log_info *log = opt->loginfo;
+ char *tmpdiff = NULL;
struct commit *commit = log->commit, *parent = log->parent;
int abbrev_commit = opt->abbrev_commit ? opt->abbrev : the_hash_algo->hexsz;
const char *extra_headers = opt->extra_headers;
@@ -715,6 +880,18 @@ void show_log(struct rev_info *opt)
*/
graph_show_commit(opt->graph);
+ /* Generate dynamic To/Cc lists as needed */
+ if (opt->to_cmd || opt->cc_cmd)
+ tmpdiff = generate_temp_patch(commit);
+
+ headerize_recipients("To", tmpdiff, opt, opt->to_recipients, opt->to_cmd, &hdrbuf);
+ headerize_recipients("Cc", tmpdiff, opt, opt->cc_recipients, opt->cc_cmd, &hdrbuf);
+
+ if (tmpdiff) {
+ unlink_or_warn(tmpdiff);
+ free(tmpdiff);
+ }
+
/*
* Print header line of header..
*/
@@ -780,8 +957,6 @@ void show_log(struct rev_info *opt)
ctx.notes_message = strbuf_detach(¬ebuf, NULL);
}
- format_recipients(opt, &hdrbuf);
-
if (extra_headers)
strbuf_addstr(&hdrbuf, extra_headers);
diff --git a/revision.h b/revision.h
index 330d351b2e4c..9611309ae496 100644
--- a/revision.h
+++ b/revision.h
@@ -285,6 +285,8 @@ struct rev_info {
int add_signoff;
struct string_list *to_recipients;
struct string_list *cc_recipients;
+ const char *to_cmd;
+ const char *cc_cmd;
const char *extra_headers;
const char *log_reencode;
const char *subject_prefix;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ba5fd0efe2ae..2387bda4f272 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -228,6 +228,25 @@ test_expect_failure 'configuration To: header (rfc2047)' '
grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" hdrs9
'
+test_expect_success 'dynamic To: header (ascii)' '
+ git config --unset-all format.to &&
+ git format-patch --to-cmd="echo \"R E Cipient <rcipient@example.com>\" #" --stdout main..side >patch10 &&
+ sed -e "/^\$/q" patch10 >hdrs10 &&
+ grep "^To: R E Cipient <rcipient@example.com>\$" hdrs10
+'
+
+test_expect_success 'dynamic To: header (rfc822)' '
+ git format-patch --to-cmd="echo \"R. E. Cipient <rcipient@example.com>\" #" --stdout main..side >patch10 &&
+ sed -e "/^\$/q" patch10 >hdrs10 &&
+ grep "^To: \"R. E. Cipient\" <rcipient@example.com>\$" hdrs10
+'
+
+test_expect_success 'dynamic To: header (rfc2047)' '
+ git format-patch --to-cmd="echo \"R Ä Cipient <rcipient@example.com>\" #" --stdout main..side >patch10 &&
+ sed -e "/^\$/q" patch10 >hdrs10 &&
+ grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" hdrs10
+'
+
# check_patch <patch>: Verify that <patch> looks like a half-sane
# patch email to avoid a false positive with !grep
check_patch () {
--
2.39.1.236.ga8a28b9eace8
^ permalink raw reply related
* [PATCH 1/5] t4014: Add test checking cover-letter To header
From: Zev Weiss @ 2023-01-19 22:38 UTC (permalink / raw)
To: git; +Cc: Zev Weiss, Eric Sunshine, Junio C Hamano
In-Reply-To: <20230119223858.29262-1-zev@bewilderbeest.net>
I at first inadvertently broke this in the process of adding
--{to,cc}-cmd support to format-patch and didn't immediately notice
because there wasn't a test for it, so let's add one now.
Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
t/t4014-format-patch.sh | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 012f155e10ae..ba5fd0efe2ae 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -2368,4 +2368,11 @@ test_expect_success 'interdiff: solo-patch' '
test_cmp expect actual
'
+test_expect_success 'cover-letter gets To: header' '
+ rm -fr patches &&
+ git config --unset-all format.to &&
+ git format-patch -o patches --cover-letter --to "R E Cipient <rcipient@example.com>" main..side >list &&
+ grep "^To: R E Cipient <rcipient@example.com>\$" patches/0000-cover-letter.patch
+'
+
test_done
--
2.39.1.236.ga8a28b9eace8
^ permalink raw reply related
* [PATCH 4/5] pretty: Add name_and_address_only parameter
From: Zev Weiss @ 2023-01-19 22:38 UTC (permalink / raw)
To: git; +Cc: Zev Weiss, Emma Brooks, Hamza Mahfooz, Junio C Hamano
In-Reply-To: <20230119223858.29262-1-zev@bewilderbeest.net>
This is meant to be used with pp_user_info() when using it to format
email recipients generated by --to-cmd/--cc-cmd. When set it omits
the leading 'From: ', trailing linefeed, and the date suffix, and
additionally will return the input string unmodified if
split_ident_line() can't parse it (e.g. for a bare email address).
Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
pretty.c | 15 ++++++++++++---
pretty.h | 1 +
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/pretty.c b/pretty.c
index 1e1e21878c83..e6798fadc107 100644
--- a/pretty.c
+++ b/pretty.c
@@ -509,8 +509,11 @@ void pp_user_info(struct pretty_print_context *pp,
return;
line_end = strchrnul(line, '\n');
- if (split_ident_line(&ident, line, line_end - line))
+ if (split_ident_line(&ident, line, line_end - line)) {
+ if (pp->name_and_address_only)
+ strbuf_addstr(sb, line);
return;
+ }
mailbuf = ident.mail_begin;
maillen = ident.mail_end - ident.mail_begin;
@@ -538,7 +541,8 @@ void pp_user_info(struct pretty_print_context *pp,
namelen = pp->from_ident->name_end - namebuf;
}
- strbuf_addstr(sb, "From: ");
+ if (!pp->name_and_address_only)
+ strbuf_addstr(sb, "From: ");
if (pp->encode_email_headers &&
needs_rfc2047_encoding(namebuf, namelen)) {
add_rfc2047(sb, namebuf, namelen,
@@ -558,7 +562,9 @@ void pp_user_info(struct pretty_print_context *pp,
if (max_length <
last_line_length(sb) + strlen(" <") + maillen + strlen(">"))
strbuf_addch(sb, '\n');
- strbuf_addf(sb, " <%.*s>\n", (int)maillen, mailbuf);
+ strbuf_addf(sb, " <%.*s>", (int)maillen, mailbuf);
+ if (!pp->name_and_address_only)
+ strbuf_addch(sb, '\n');
} else {
struct strbuf id = STRBUF_INIT;
enum grep_header_field field = GREP_HEADER_FIELD_MAX;
@@ -582,6 +588,9 @@ void pp_user_info(struct pretty_print_context *pp,
strbuf_release(&id);
}
+ if (pp->name_and_address_only)
+ return;
+
switch (pp->fmt) {
case CMIT_FMT_MEDIUM:
strbuf_addf(sb, "Date: %s\n",
diff --git a/pretty.h b/pretty.h
index f34e24c53a49..306666e99294 100644
--- a/pretty.h
+++ b/pretty.h
@@ -39,6 +39,7 @@ struct pretty_print_context {
int preserve_subject;
struct date_mode date_mode;
unsigned date_mode_explicit:1;
+ unsigned name_and_address_only:1;
int print_email_subject;
int expand_tabs_in_log;
int need_8bit_cte;
--
2.39.1.236.ga8a28b9eace8
^ permalink raw reply related
* [PATCH 2/5] log: Refactor duplicated code to headerize recipient lists
From: Zev Weiss @ 2023-01-19 22:38 UTC (permalink / raw)
To: git
Cc: Zev Weiss, brian m. carlson,
Ævar Arnfjörð Bjarmason, Junio C Hamano,
Patrick Hemmer
In-Reply-To: <20230119223858.29262-1-zev@bewilderbeest.net>
The To and Cc headers are handled identically (the only difference is
the header name tag), so we might as well reuse the same code for both
instead of duplicating it.
Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
builtin/log.c | 23 ++---------------------
log-tree.c | 15 +++++++++++++++
log-tree.h | 2 ++
3 files changed, 19 insertions(+), 21 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 057e299c245c..ad9d7ebc6d73 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2028,27 +2028,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
strbuf_addch(&buf, '\n');
}
- if (extra_to.nr)
- strbuf_addstr(&buf, "To: ");
- for (i = 0; i < extra_to.nr; i++) {
- if (i)
- strbuf_addstr(&buf, " ");
- strbuf_addstr(&buf, extra_to.items[i].string);
- if (i + 1 < extra_to.nr)
- strbuf_addch(&buf, ',');
- strbuf_addch(&buf, '\n');
- }
-
- if (extra_cc.nr)
- strbuf_addstr(&buf, "Cc: ");
- for (i = 0; i < extra_cc.nr; i++) {
- if (i)
- strbuf_addstr(&buf, " ");
- strbuf_addstr(&buf, extra_cc.items[i].string);
- if (i + 1 < extra_cc.nr)
- strbuf_addch(&buf, ',');
- strbuf_addch(&buf, '\n');
- }
+ recipients_to_header_buf("To", &buf, &extra_to);
+ recipients_to_header_buf("Cc", &buf, &extra_cc);
rev.extra_headers = to_free = strbuf_detach(&buf, NULL);
diff --git a/log-tree.c b/log-tree.c
index 1dd5fcbf7be4..0e8863fe545a 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -426,6 +426,21 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
}
}
+void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
+ const struct string_list *recipients)
+{
+ for (int i = 0; i < recipients->nr; i++) {
+ if (!i)
+ strbuf_addf(buf, "%s: ", hdr);
+ else
+ strbuf_addstr(buf, " ");
+ strbuf_addstr(buf, recipients->items[i].string);
+ if (i + 1 < recipients->nr)
+ strbuf_addch(buf, ',');
+ strbuf_addch(buf, '\n');
+ }
+}
+
void log_write_email_headers(struct rev_info *opt, struct commit *commit,
const char **extra_headers_p,
int *need_8bit_cte_p,
diff --git a/log-tree.h b/log-tree.h
index e7e4641cf83c..227edc564121 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -25,6 +25,8 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit,
#define format_decorations(strbuf, commit, color) \
format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")")
void show_decorations(struct rev_info *opt, struct commit *commit);
+void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
+ const struct string_list *recipients);
void log_write_email_headers(struct rev_info *opt, struct commit *commit,
const char **extra_headers_p,
int *need_8bit_cte_p,
--
2.39.1.236.ga8a28b9eace8
^ permalink raw reply related
* [PATCH 3/5] log: Push to/cc handling down into show_log()
From: Zev Weiss @ 2023-01-19 22:38 UTC (permalink / raw)
To: git
Cc: Zev Weiss, brian m. carlson,
Ævar Arnfjörð Bjarmason, René Scharfe,
Denton Liu, Emma Brooks, Eric Sunshine, Junio C Hamano,
Patrick Hemmer
In-Reply-To: <20230119223858.29262-1-zev@bewilderbeest.net>
This rearrangement is a measure to facilitate adding --to-cmd/--cc-cmd
support to format-patch. The command will need to be run separately
for each commit, so the lists of individual recipients specified via
--to and --cc (and potentially --add-header) need to be available at
the per-commit level instead of just the combined headers in a single
string as has been the case until now.
Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
builtin/log.c | 6 +++---
log-tree.c | 22 +++++++++++++++++++---
log-tree.h | 3 +--
revision.h | 2 ++
4 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index ad9d7ebc6d73..c0c7b8544d73 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1326,6 +1326,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
pp.rev = rev;
pp.print_email_subject = 1;
pp_user_info(&pp, NULL, &sb, committer, encoding);
+ format_recipients(rev, &sb);
prepare_cover_text(&pp, branch_name, &sb, encoding, need_8bit_cte);
fprintf(rev->diffopt.file, "%s\n", sb.buf);
@@ -2028,9 +2029,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
strbuf_addch(&buf, '\n');
}
- recipients_to_header_buf("To", &buf, &extra_to);
- recipients_to_header_buf("Cc", &buf, &extra_cc);
-
+ rev.to_recipients = &extra_to;
+ rev.cc_recipients = &extra_cc;
rev.extra_headers = to_free = strbuf_detach(&buf, NULL);
if (from) {
diff --git a/log-tree.c b/log-tree.c
index 0e8863fe545a..7aa2841dd803 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -426,8 +426,8 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
}
}
-void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
- const struct string_list *recipients)
+static void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
+ const struct string_list *recipients)
{
for (int i = 0; i < recipients->nr; i++) {
if (!i)
@@ -441,6 +441,14 @@ void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
}
}
+void format_recipients(struct rev_info *rev, struct strbuf *sb)
+{
+ if (rev->to_recipients)
+ recipients_to_header_buf("To", sb, rev->to_recipients);
+ if (rev->cc_recipients)
+ recipients_to_header_buf("Cc", sb, rev->cc_recipients);
+}
+
void log_write_email_headers(struct rev_info *opt, struct commit *commit,
const char **extra_headers_p,
int *need_8bit_cte_p,
@@ -647,10 +655,12 @@ static void next_commentary_block(struct rev_info *opt, struct strbuf *sb)
void show_log(struct rev_info *opt)
{
struct strbuf msgbuf = STRBUF_INIT;
+ struct strbuf hdrbuf = STRBUF_INIT;
struct log_info *log = opt->loginfo;
struct commit *commit = log->commit, *parent = log->parent;
int abbrev_commit = opt->abbrev_commit ? opt->abbrev : the_hash_algo->hexsz;
const char *extra_headers = opt->extra_headers;
+ char *to_free;
struct pretty_print_context ctx = {0};
opt->loginfo = NULL;
@@ -770,6 +780,11 @@ void show_log(struct rev_info *opt)
ctx.notes_message = strbuf_detach(¬ebuf, NULL);
}
+ format_recipients(opt, &hdrbuf);
+
+ if (extra_headers)
+ strbuf_addstr(&hdrbuf, extra_headers);
+
/*
* And then the pretty-printed message itself
*/
@@ -779,7 +794,7 @@ void show_log(struct rev_info *opt)
ctx.date_mode = opt->date_mode;
ctx.date_mode_explicit = opt->date_mode_explicit;
ctx.abbrev = opt->diffopt.abbrev;
- ctx.after_subject = extra_headers;
+ ctx.after_subject = to_free = strbuf_detach(&hdrbuf, NULL);
ctx.preserve_subject = opt->preserve_subject;
ctx.encode_email_headers = opt->encode_email_headers;
ctx.reflog_info = opt->reflog_info;
@@ -828,6 +843,7 @@ void show_log(struct rev_info *opt)
strbuf_release(&msgbuf);
free(ctx.notes_message);
+ free(to_free);
if (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {
struct diff_queue_struct dq;
diff --git a/log-tree.h b/log-tree.h
index 227edc564121..ace50dad6c83 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -25,8 +25,7 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit,
#define format_decorations(strbuf, commit, color) \
format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")")
void show_decorations(struct rev_info *opt, struct commit *commit);
-void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
- const struct string_list *recipients);
+void format_recipients(struct rev_info *rev, struct strbuf *sb);
void log_write_email_headers(struct rev_info *opt, struct commit *commit,
const char **extra_headers_p,
int *need_8bit_cte_p,
diff --git a/revision.h b/revision.h
index 30febad09a1e..330d351b2e4c 100644
--- a/revision.h
+++ b/revision.h
@@ -283,6 +283,8 @@ struct rev_info {
struct ident_split from_ident;
struct string_list *ref_message_ids;
int add_signoff;
+ struct string_list *to_recipients;
+ struct string_list *cc_recipients;
const char *extra_headers;
const char *log_reencode;
const char *subject_prefix;
--
2.39.1.236.ga8a28b9eace8
^ permalink raw reply related
* [PATCH 0/5] format-patch: Add --{to,cc}-cmd support
From: Zev Weiss @ 2023-01-19 22:38 UTC (permalink / raw)
To: git
Cc: Zev Weiss, Junio C Hamano, Eric Sunshine, brian m. carlson,
Ævar Arnfjörð Bjarmason, Patrick Hemmer,
René Scharfe, Denton Liu, Emma Brooks, Hamza Mahfooz
Hello,
On more than one occasion I've found myself wishing that git's
format-patch command supported the --to-cmd/--cc-cmd flags that
send-email does; this patch series is my attempt at implementing it.
This is my first foray into the git codebase, so odds are good I've
bungled something or missed the existence of some useful API, but it
passes the existing test suite (and the tests added for the new
feature), and I'm giving it its inaugural dogfooding on this very
series with `contrib/contacts/git-contacts`.
The first patch adds a test I found myself wanting when I realized I
had accidentally broken something that wasn't covered by the existing
tests. The next two do a bit of refactoring and rearrangement, the
fourth adds a bit of library infrastructure to pretty.c, and the final
patch implements the feature itself.
There are a few points where I wasn't sure if the approach I chose was
really the best:
- The 'name_and_address_only' pretty_print_context flag added in the
fourth patch is kind of clunky, but I didn't see any other obvious
great way to reuse the bits of pp_user_info() that I wanted.
- Fork/exec-ing another internal format-patch command to generate the
temporary patch file to run the commands on -- my attempts to
recurse back into show_log() to do it more directly didn't go well.
- As currently written any commands specified are only run on the
patches themselves, not on the cover letter. The slight
inconsistency with git-send-email is a bit unfortunate, but I
figure it's probably not often all that useful in that context
anyway (and the implementation looked like it would be just
non-trivial enough that laziness may have played a role as well).
Suggestions on better alternatives there (or elsewhere) are of course
welcome.
Thanks,
Zev Weiss
Zev Weiss (5):
t4014: Add test checking cover-letter To header
log: Refactor duplicated code to headerize recipient lists
log: Push to/cc handling down into show_log()
pretty: Add name_and_address_only parameter
format-patch: Add support for --{to,cc}-cmd flags
builtin/log.c | 31 +++---
log-tree.c | 208 +++++++++++++++++++++++++++++++++++++++-
log-tree.h | 1 +
pretty.c | 15 ++-
pretty.h | 1 +
revision.h | 4 +
t/t4014-format-patch.sh | 26 +++++
7 files changed, 262 insertions(+), 24 deletions(-)
--
2.39.1.236.ga8a28b9eace8
^ permalink raw reply
* Re: [PATCH] fetch: fix duplicate remote parallel fetch bug
From: Junio C Hamano @ 2023-01-19 22:40 UTC (permalink / raw)
To: Calvin Wan; +Cc: git
In-Reply-To: <20230119220538.1522464-1-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
> Fetching in parallel from a remote group with a duplicated remote results
> in the following:
>
> error: cannot lock ref '<ref>': is at <oid> but expected <oid>
>
> This doesn't happen in serial since fetching from the same remote that
> has already been fetched from is a noop. Therefore, remove any duplicated
> remotes after remote groups are parsed.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
> builtin/fetch.c | 1 +
> t/t5506-remote-groups.sh | 9 +++++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b06e454cbd..508ab2670c 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -2225,6 +2225,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> argv++;
> }
> }
> + string_list_remove_duplicates(&list, 0);
As it always is possible to edit .git/config manually, it is
necessary to perform deduplication like this patch does on the
consumer side of the list, but do you know if our tool create
duplication, or is it entirely something the end-user does manually?
If it is the former, I am wondering if we should also fix such a
code path that does so in the first place.
Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH 8/8] bundle-uri: store fetch.bundleCreationToken
From: Victoria Dye @ 2023-01-19 22:24 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <51f210ddeb46fb06e885dc384a486c4bb16ad8cd.1673037405.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> When a bundle list specifies the "creationToken" heuristic, the Git
> client downloads the list and then starts downloading bundles in
> descending creationToken order. This process stops as soon as all
> downloaded bundles can be applied to the repository (because all
> required commits are present in the repository or in the downloaded
> bundles).
>
> When checking the same bundle list twice, this strategy requires
> downloading the bundle with the maximum creationToken again, which is
> wasteful. The creationToken heuristic promises that the client will not
> have a use for that bundle if its creationToken value is the at most the
s/is the at most/is at most(?)
> previous creationToken value.
>
> To prevent these wasteful downloads, create a fetch.bundleCreationToken
> config setting that the Git client sets after downloading bundles. This
> value allows skipping that maximum bundle download when this config
> value is the same value (or larger).
>
> To test that this works correctly, we can insert some "duplicate"
> fetches into existing tests and demonstrate that only the bundle list is
> downloaded.
>
> The previous logic for downloading bundles by creationToken worked even
> if the bundle list was empty, but now we have logic that depends on the
> first entry of the list. Terminate early in the (non-sensical) case of
> an empty bundle list.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> Documentation/config/fetch.txt | 8 ++++++++
> bundle-uri.c | 35 ++++++++++++++++++++++++++++++++--
> t/t5558-clone-bundle-uri.sh | 25 +++++++++++++++++++++++-
> 3 files changed, 65 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
> index 4f796218aab..96755ba148b 100644
> --- a/Documentation/config/fetch.txt
> +++ b/Documentation/config/fetch.txt
> @@ -104,3 +104,11 @@ fetch.bundleURI::
> first running `git fetch --bundle-uri=<uri>` immediately before
> `git fetch <args>`. See details of the `--bundle-uri` option in
> linkgit:git-fetch[1].
> +
> +fetch.bundleCreationToken::
> + When using `fetch.bundleURI` to fetch incrementally from a bundle
> + list that uses the "creationToken" heuristic, this config value
> + stores the maximum `creationToken` value of the downloaded bundles.
> + This value is used to prevent downloading bundles in the future
> + if the advertised `creationToken` is not strictly larger than this
> + value.
> diff --git a/bundle-uri.c b/bundle-uri.c
> index 1dbbbb980eb..98655bd6721 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -464,6 +464,8 @@ static int fetch_bundles_by_token(struct repository *r,
> {
> int cur;
> int pop_or_push = 0;
> + const char *creationTokenStr;
> + uint64_t maxCreationToken;
> struct bundle_list_context ctx = {
> .r = r,
> .list = list,
> @@ -477,8 +479,27 @@ static int fetch_bundles_by_token(struct repository *r,
>
> for_all_bundles_in_list(list, insert_bundle, &sorted);
>
> + if (!sorted.nr) {
> + free(sorted.items);
> + return 0;
> + }
This check is added here because we're only now at risk for an invalid
access to 'sorted' (checking 'sorted.items[0]' below).
> +
> QSORT(sorted.items, sorted.nr, compare_creation_token);
>
> + /*
> + * If fetch.bundleCreationToken exists, parses to a uint64t, and
> + * is not strictly smaller than the maximum creation token in the
> + * bundle list, then do not download any bundles.
> + */
> + if (!repo_config_get_value(r,
> + "fetch.bundlecreationtoken",
> + &creationTokenStr) &&
> + sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) == 1 &&
> + sorted.items[0]->creationToken <= maxCreationToken) {
> + free(sorted.items);
> + return 0;
> + }
And here, we exit if the cached creation token is greater than or equal to
the highest advertised token. Overall, this seems pretty safe:
- If the value is (somehow) manually updated to something larger than it
should be, objects will be fetched from the server that could have
otherwise come from a bundle. Not ideal, but no worse than a regular
fetch.
- If the value is too small or invalid (i.e., not an unsigned integer),
we'll download the first bundle, unbundle it, then overwrite the invalid
'fetch.bundlecreationtoken' with a new valid one.
The latter is self-correcting, but should the former be documented
somewhere? For example, if someone changes which bundle server they're using
with a repo, the creation token numbering scheme might be completely
different. In that case, a user would probably want to "reset" the
'fetch.bundlecreationtoken' and start fresh with a new bundle list (even if
the recommended method is "manually delete the config key from your repo").
> +
> /*
> * Use a stack-based approach to download the bundles and attempt
> * to unbundle them in decreasing order by creation token. If we
> @@ -541,14 +562,24 @@ stack_operation:
> cur += pop_or_push;
> }
>
> - free(sorted.items);
> -
> /*
> * We succeed if the loop terminates because 'cur' drops below
> * zero. The other case is that we terminate because 'cur'
> * reaches the end of the list, so we have a failure no matter
> * which bundles we apply from the list.
> */
> + if (cur < 0) {
> + struct strbuf value = STRBUF_INIT;
> + strbuf_addf(&value, "%"PRIu64"", sorted.items[0]->creationToken);
> + if (repo_config_set_multivar_gently(ctx.r,
> + "fetch.bundleCreationToken",
> + value.buf, NULL, 0))
Set the new max bundle creation token if the value has been updated (if the
'fetch.bundleCreationToken' was >= the first bundle's token, 'cur' is 0) in
the repository scope (like the cached bundle URI in patch 5 [1]). Looks
good.
[1] https://lore.kernel.org/git/d9c6f50e4f218267c1e8da060ce5b190dc8a709c.1673037405.git.gitgitgadget@gmail.com/
> + warning(_("failed to store maximum creation token"));
> +
> + strbuf_release(&value);
> + }
> +
> + free(sorted.items);
> return cur >= 0;
> }
>
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
> index 3f4d61a915c..0604d721f1b 100755
> --- a/t/t5558-clone-bundle-uri.sh
> +++ b/t/t5558-clone-bundle-uri.sh
It isn't exactly related to this patch, but it looks like the second "clone
bundle list (http, creationToken)" never received any updates to
differentiate it from the first test with that name (I noted the duplicate
name in patch 4 [2]). Is it just a leftover duplicate?
[1] https://lore.kernel.org/git/ede340d1-bce4-0c1d-7afb-4874a67d1803@github.com/
> @@ -455,6 +455,7 @@ test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> "$HTTPD_URL/smart/fetch.git" fetch-http-4 &&
>
> test_cmp_config -C fetch-http-4 "$HTTPD_URL/bundle-list" fetch.bundleuri &&
> + test_cmp_config -C fetch-http-4 1 fetch.bundlecreationtoken &&
>
> # The clone should copy two files: the list and bundle-1.
> test_bundle_downloaded bundle-list trace-clone.txt &&
> @@ -479,6 +480,8 @@ test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> refs/heads/left:refs/heads/left \
> refs/heads/right:refs/heads/right &&
>
> + test_cmp_config -C fetch-http-4 2 fetch.bundlecreationtoken &&
> +
> # This fetch should copy two files: the list and bundle-2.
> test_bundle_downloaded bundle-list trace1.txt &&
> test_bundle_downloaded bundle-2.bundle trace1.txt &&
Verifying the max creation token that's saved - nice!
> @@ -492,6 +495,15 @@ test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> EOF
> test_cmp expect refs &&
>
> + # No-op fetch
> + GIT_TRACE2_EVENT="$(pwd)/trace1b.txt" \
> + git -C fetch-http-4 fetch origin --no-tags \
> + refs/heads/left:refs/heads/left \
> + refs/heads/right:refs/heads/right &&
> + test_bundle_downloaded bundle-list trace1b.txt &&
> + ! test_bundle_downloaded bundle-1.bundle trace1b.txt &&
> + ! test_bundle_downloaded bundle-2.bundle trace1b.txt &&
Now we make sure we're not downloading that first bundle if it's already
been unbundled.
> +
> cat >>"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> [bundle "bundle-3"]
> uri = bundle-3.bundle
> @@ -508,6 +520,8 @@ test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> git -C fetch-http-4 fetch origin --no-tags \
> refs/heads/merge:refs/heads/merge &&
>
> + test_cmp_config -C fetch-http-4 4 fetch.bundlecreationtoken &&
> +
> # This fetch should copy three files: the list, bundle-3, and bundle-4.
> test_bundle_downloaded bundle-list trace2.txt &&
> test_bundle_downloaded bundle-4.bundle trace2.txt &&
> @@ -524,7 +538,16 @@ test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> refs/bundles/left
> refs/bundles/merge
> EOF
> - test_cmp expect refs
> + test_cmp expect refs &&
> +
> + # No-op fetch
> + GIT_TRACE2_EVENT="$(pwd)/trace2b.txt" \
> + git -C fetch-http-4 fetch origin &&
> + test_bundle_downloaded bundle-list trace2b.txt &&
> + ! test_bundle_downloaded bundle-1.bundle trace2b.txt &&
> + ! test_bundle_downloaded bundle-2.bundle trace2b.txt &&
> + ! test_bundle_downloaded bundle-3.bundle trace2b.txt &&
> + ! test_bundle_downloaded bundle-4.bundle trace2b.txt
And add another no-op fetch - this time, specifically making sure the
downloaded bundles have covered all objects fetched from 'origin'. As with
previous patches, these test updates are quite thorough; nice work!
> '
>
> # Do not add tests here unless they use the HTTP server, as they will
^ permalink raw reply
* Re: [PATCH v8 3/4] worktree add: add --orphan flag
From: Jacob Abel @ 2023-01-19 22:20 UTC (permalink / raw)
To: phillip.wood
Cc: git, Ævar Arnfjörð Bjarmason, Eric Sunshine,
Junio C Hamano, Phillip Wood, Rubén Justo, Taylor Blau,
rsbecker
In-Reply-To: <df2c0ae0-0654-a3e1-cc02-be2d970ea287@dunelm.org.uk>
On 23/01/19 04:18PM, Phillip Wood wrote:
> On 18/01/2023 22:40, Jacob Abel wrote:
> > [...]
> > Understood.
> >
> > I'm not entirely opposed to making this change to OPT_BOOL but I have to wonder
> > how often `--orphan` will actually be used by a given user and whether the
> > slightly shorter invocation will be used regularly.
> >
> > With the base `git worktree add $path`, the shorthand/DWYM makes sense as it's
> > used regularly but I don't see users working with `--orphan` outside of trying
> > to create the first branch in a repository.
>
> Your example use in the commit message shows the user using the same
> name for the branch and worktree. If that really is the likely use than
> I think we should make --orphan OPT_BOOL. If it is not the likely use
> perhaps you could update the commit message to show how you think it
> will be used.
The example in the commit message is mostly a trivial example to show the
change. I think whether someone uses the same name for the branch and worktree
depends on how they use the feature. At least personally, generally I name my
worktree based on either the relevant project/"hat" or using an "a/, b/, c/" or
"alpha/, beta/, gamma/" style.
So in my personal use, it'd look something like:
git worktree add --orphan main alpha/
---
Or to occassionally break out a scratchpad that I may want to keep around for a
while but not actually integrate into the feature branch in it's current form.
This isn't really for code but rather so I can scratch out and document my logic
prior to doing a formal write up at the end of a given feature. Of course since
it doesn't make it into the main tree, when everything is written up, I can just
toss the branch and let gc take care of it.
This ends up looking like so:
git worktree add --orphan scratch-1234-foobar-feature scratchpad/
And since `worktree add` only needs to be done once, I only do this the first
time I set up my dev environment on a machine. After that I can just use
`git switch --orphan` to create new scratchpad branches and `git switch` to
swap between existing scratchpads.
---
The first example I see as being the main use case (which could hopefully be
DWYMed eventually) and the latter example is a quirk of my admittedly niche
personal usecase for worktrees.
> > And I'd like that operation of creating the first branch in a repo to eventually
> > "just work" with the base command, i.e. `git worktree add main/`. The reason I
> > hadn't yet added that is because I've yet to figure out how to get it to work
> > without accidentally introducing potentially confusing situations and I didn't
> > want to hold up introducing the core functionality itself.
> >
> > Once that main use-case "just works", I don't see users utilising `--orphan`
> > except in very rare circumstances. Doubly so since the average user likely
> > shouldn't be using `--orphan` in most cases.
>
> This brings us back to the question that we discussed earlier of whether
> we need --orphan at all. If we can get the main use-case working without
> it we'd perhaps be better doing that rather than adding an option no one
> ends up using.
At least personally, I'd rather expose the option for users who may potentially
want it. I think it'd be useful regardless but in the same way `--orphan` is
currently useful in `git switch`, which is for very specific niche cases and
really only for power users rather than as a common tool everyday users are
expected to know.
And for the main use case, my concerns were:
1. If we DWYM and create a branch when there are no existing branches, what
about the case where a user sets up the repo but forgot to fetch. i.e. if a user
does:
# Or instead of init --template, use some language's project init tool to
# setup git hooks.
% git init --bare --template <tmpldir> .git
% git remote add origin <remote>
% git worktree add main/
Should we just warn the user that they haven't fetched their remote yet while we
prepare the worktree?
2. Suppose we also check whether the remote has been fetched and the remote has
a branch matching the current branch name.
Should we fail (as it currently does on main) with an advise to try the command
`git worktree add main main` instead? Or should that command also "just work"
3. If we want to do the above, should it do this for all commands trying to
create a worktree until at least one real branch (with commits) exists in the
repo or should we only do this when the branch name matches the one defined in
`init.defaultBranch`?
> Best Wishes
>
> Phillip
>
> > [...]
^ permalink raw reply
* [PATCH] fetch: fix duplicate remote parallel fetch bug
From: Calvin Wan @ 2023-01-19 22:05 UTC (permalink / raw)
To: git; +Cc: Calvin Wan
Fetching in parallel from a remote group with a duplicated remote results
in the following:
error: cannot lock ref '<ref>': is at <oid> but expected <oid>
This doesn't happen in serial since fetching from the same remote that
has already been fetched from is a noop. Therefore, remove any duplicated
remotes after remote groups are parsed.
Signed-off-by: Calvin Wan <calvinwan@google.com>
---
builtin/fetch.c | 1 +
t/t5506-remote-groups.sh | 9 +++++++++
2 files changed, 10 insertions(+)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b06e454cbd..508ab2670c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -2225,6 +2225,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
argv++;
}
}
+ string_list_remove_duplicates(&list, 0);
if (negotiate_only) {
struct oidset acked_commits = OIDSET_INIT;
diff --git a/t/t5506-remote-groups.sh b/t/t5506-remote-groups.sh
index 5bac03ede8..0e176175a3 100755
--- a/t/t5506-remote-groups.sh
+++ b/t/t5506-remote-groups.sh
@@ -99,4 +99,13 @@ test_expect_success 'updating remote name updates that remote' '
! repo_fetched two
'
+test_expect_success 'updating group in parallel with a duplicate remote does not fail (fetch)' '
+ mark fetch-group-duplicate &&
+ update_repo one &&
+ git config --add remotes.duplicate one &&
+ git config --add remotes.duplicate one &&
+ git -c fetch.parallel=2 remote update duplicate &&
+ repo_fetched one
+'
+
test_done
--
2.39.0.246.g2a6d74b583-goog
^ permalink raw reply related
* Re: [PATCH] rebase: mark --update-refs as requiring the merge backend
From: Derrick Stolee @ 2023-01-19 21:47 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git; +Cc: Elijah Newren
In-Reply-To: <pull.1466.git.1674106587550.gitgitgadget@gmail.com>
On 1/19/2023 12:36 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> --update-refs is built in terms of the sequencer, which requires the
> merge backend. It was already marked as incompatible with the apply
> backend in the git-rebase manual, but the code didn't check for this
> incompatibility and warn the user. Check and warn now.
Thank you for submitting this version.
> @@ -1514,6 +1514,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> }
> }
>
> + if (options.update_refs)
> + imply_merge(&options, "--update-refs");
> +
This solution is very elegant. The only downside is the lack of warning
if --update-refs was implied by rebase.updateRefs=true, but I'm happy to
delay implementing that warning in favor of your complete solution here.
Thinking ahead to that option, are there other options that are implied
by config that are required in imply_merge()? Is --autosquash in that
camp? If so, then maybe it would make sense to expand imply_merge() to
include a boolean config key and supply that warning within its
implementation. (This consideration should not block this patch, as it
is complete as-is.)
> While at it, fix a typo in t3422...and fix some misleading wording (all
> useful options other than --whitespace=fix have long since been
> implemented in the merge backend).
> #
> -# Rebase has lots of useful options like --whitepsace=fix, which are
> -# actually all built in terms of flags to git-am. Since neither
> -# --merge nor --interactive (nor any options that imply those two) use
> -# git-am, using them together will result in flags like --whitespace=fix
> -# being ignored. Make sure rebase warns the user and aborts instead.
> +# Rebase has a useful option, --whitespace=fix, which is actually
> +# built in terms of flags to git-am. Since neither --merge nor
> +# --interactive (nor any options that imply those two) use git-am,
> +# using them together will result in --whitespace=fix being ignored.
> +# Make sure rebase warns the user and aborts instead.
> #
Thanks for the update here. The -C option is also used in this test,
so --whitespace=fix isn't the only option, right? At least, -C doesn't
make sense to port over to the merge backend, so maybe that's what
you mean by --whitespace=fix being the last one?
The user could also explicitly request the apply backend with --apply,
but this test script doesn't check it, strangely. That seems like an
oversight, but not a drawback to your patch.
> test_rebase_am_only () {
> @@ -60,6 +60,11 @@ test_rebase_am_only () {
> test_must_fail git rebase $opt --exec 'true' A
> "
>
> + test_expect_success "$opt incompatible with --update-refs" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --update-refs A
> + "
> +
Thanks for adding this test. I would delay the rebase.updateRefs
version until there is extra behavior to check, such as the
warning message.
Thanks,
-Stolee
^ permalink raw reply
* Re: Parallelism for submodule update
From: Calvin Wan @ 2023-01-19 21:39 UTC (permalink / raw)
To: Christian.Zitzmann; +Cc: Calvin Wan, git@vger.kernel.org
In-Reply-To: <DB5PR02MB100691E6422F5E94228F0E0EC8AF79@DB5PR02MB10069.eurprd02.prod.outlook.com>
Hi Christian,
I investigated this as well about 2 months ago and am happy to share my
findings with you :)
> When updating the submodules, only the fetching part is done in parallel (with config submodule.fetchjobs or --jobs) but the checkout is done sequentially
Correct.
> What I’ve recognized when cloning with
> - scalar clone --full-clone --recurse-submodules <URL>
> or
> - git clone --filter=blob:none --also-filter-submodules --recurse-submodules <URL>
>
> We loose performance, as the fetch of the blobs is done in the sequential checkout part, instead of in the parallel part.
>
> Furthermore, the utilization - without partial clone - of network and harddisk is not always good, as first the network is utilized (fetch) and then the harddisk (checkout)
Also an astute observation that separating out the parallelization of
fetch and checkout doesn't allow us to fully use our resources.
> As the checkout part is local to the submodule (no shared resources to block), it would be great if we could move the checkout into the parallelized part.
> E.g. by doing fetch and checkout (with blob fetching) in one step with e.g. run_processes_parallel_tr2
>
> I expect that this significantly improves the performance, especially when using partial clones.
>
> Do you think this is possible? Do I miss anything in my thoughts?
Sort of. The issue with run_processes_parallel_tr2 is that it creates a
subprocess with a git command. There is no git command that we can call
that lets us do both the correct fetch and checkout command, so first
you would have to create a new option/command for that (and what happens
if we want to add to that parallelization in the future? Create another
option/command?). I think we can do better than that!
`git submodule update`, when called from clone, essentially does 4
things to the submodule: init, clone, checkout, and recursively calls
itself for child submodules. One idea I had was to separate out the
individual tasks that `git submodule update` does and create a new
submodule--helper command (eg. git submodule--helper update-helper) that
calls those individual tasks. Then, clone would directly call
run_processes_parallel_tr2 with the new submodule--helper command and
each process separated by submodule.
This is what I imagine the general idea of what
`git clone --recurse-submodules` would look like:
superproject cloning
run_processes_parallel_tr2(git submodule--helper update-helper)
Init
Clone
Checkout
Recursive git submdodule update-helper
I'll discuss what I think are the benefits of this approach:
- The entirety of submodule update would be parallelized so network and
hard disk resources can be used together
- There only needs to be one config option that controls how many
parallel processes to spawn
- Any new features to submodule update are automatically parallelized
The drawback is that any new feature that would cause a race condition
if run in parallel would have to have additional locking code written
for it since separating it out would be difficult. In this case, only
adding lines to .gitmodules in init is at risk of a race condition, but
fortunately that can be handled first in series before running
everything else in parallel.
I haven't started implementing this and am not planning to fix this in
the near future. This is because we are planning a more long-term
solution (2y+) to solve problems like this (notice how much simpler it
would've been to add parallelization if we didn't have to create
subprocesses for every separate git command and instead could call from
a variety of library functions). So if you need the parallelizations
sooner or want to scratch your itch, you're more than welcome to
implement it. Happy to bounce ideas off of and review any patches for
this!
Thanks,
Calvin
^ permalink raw reply
* Re: [PATCH 3/3] branch: rename orphan branches in any worktree
From: Junio C Hamano @ 2023-01-19 21:33 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <a47ff192-db67-dc4c-ded3-cd1e7c197726@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> + if (!copy && !(ishead > 1) &&
Logically it might be necessary to be able to extend "is that branch
what we have checked out, yes or no?" bool into something else that
can be something other than 0 or 1, but as soon as you did so,
"is_head" is no longer a Boolean "is it a HEAD, yes or no?".
Now what does that value really _mean_? Please rename the variable
and helper function appropriately to make it clear what is going on.
^ permalink raw reply
* Re: [PATCH v2 1/3] avoid unnecessary worktrees traversing
From: Junio C Hamano @ 2023-01-19 21:24 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <bc82dd95-c968-146d-0dea-f82631b74765@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> -static void reject_rebase_or_bisect_branch(const char *target)
> +static int ishead_and_reject_rebase_or_bisect_branch(const char *target)
The original name was already horrible but it became much worse by
acquiring a non-word "ishead" as part of it X-<.
At least let's replace "rebase or bisect" with something a bit more
generic, extensible, and shorter phrase. For example, isn't the
point of having the function was to give us a mechansim to see if
the branch with the given name is not to be modified because it is
being worked on elsewhere? "The branch is in use" would be a good
phrase to express such a concept, so die_if_branch_is_in_use() or
something along that line may be easier to grok.
^ permalink raw reply
* Race condition on `git checkout -c`
From: Arthur Milchior @ 2023-01-19 21:13 UTC (permalink / raw)
To: git
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
I was working on code as usual. I should note that the repo is big, so
it’s not unusual for some git operations to take half a minute.
Here is a simpler reproduction that worked on my machine.
```
mkdir g
cd g
git init
git touch a
git add a
git commit -am "a"
git checkout -b b
```
While the last command is being run, in a different terminal, run `git
checkout -b B`
I realized that I can do it manually, by just using cmd-tab, then enter.
Finally, type `git branch`
What did you expect to happen? (Expected behavior)
I expect either:
* to see an error message stating that `b` already exists
* to see `b` and `B` in the list of branch.
In any case, when both branches exists, which will be the case on
system which were started with this "bug", I expect to see both
branches listed.
What happened instead? (Actual behavior)
I was able to successively create both branches.
The branch `b` is not listed. If I check it out, then `git branch`
does not list a single branch as the current branch.
What's different between what you expected and what actually happened?
I expect that `git branch` always list me on a branch, and let me know
of all existing branch.
Anything else you want to add:
First: I can’t reproduce with `git branch b` and `git branch B`, so I
guess either `branch` is faster than `checkout -c`, or something else
is done right there.
I know that this race condition seems very contrived. In real life,
the names were `bookmarkShome` and `bookmarksHome`. Obviously, both
were supposed to be `bookmarksHome`, in camlCase. The first one was a
typo. But that was the one git show, which was confusing.
However, even if the repo is big and checking out a branch can easily
take 30 seconds, I’m still not sure when I’d have two terminals
creating branchs at the same time.
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.39.0.246.g2a6d74b583-goog
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Darwin 22.2.0 Darwin Kernel Version 22.2.0: Fri Nov 11 02:03:51
PST 2022; root:xnu-8792.61.2~4/RELEASE_ARM64_T6000 x86_64
compiler info: clang: 14.0.0 (clang-1400.0.29.202)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
^ permalink raw reply
* Re: [PATCH 7/8] fetch: fetch from an external bundle URI
From: Victoria Dye @ 2023-01-19 20:34 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <1627fc158b1e301a1663e24f9f21268b4f1caa55.1673037405.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> When a user specifies a URI via 'git clone --bundle-uri', that URI may
> be a bundle list that advertises a 'bundle.heuristic' value. In that
> case, the Git client stores a 'fetch.bundleURI' config value storing
> that URI.
>
> Teach 'git fetch' to check for this config value and download bundles
> from that URI before fetching from the Git remote(s). Likely, the bundle
> provider has configured a heuristic (such as "creationToken") that will
> allow the Git client to download only a portion of the bundles before
> continuing the fetch.
>
> Since this URI is completely independent of the remote server, we want
> to be sure that we connect to the bundle URI before creating a
> connection to the Git remote. We do not want to hold a stateful
> connection for too long if we can avoid it.
>
> To test that this works correctly, extend the previous tests that set
> 'fetch.bundleURI' to do follow-up fetches. The bundle list is updated
> incrementally at each phase to demonstrate that the heuristic avoids
> downloading older bundles. This includes the middle fetch downloading
> the objects in bundle-3.bundle from the Git remote, and therefore not
> needing that bundle in the third fetch.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> builtin/fetch.c | 8 +++++
> t/t5558-clone-bundle-uri.sh | 59 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 67 insertions(+)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 7378cafeec9..fbb1d470c38 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -29,6 +29,7 @@
> #include "commit-graph.h"
> #include "shallow.h"
> #include "worktree.h"
> +#include "bundle-uri.h"
>
> #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
>
> @@ -2109,6 +2110,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
> int cmd_fetch(int argc, const char **argv, const char *prefix)
> {
> int i;
> + const char *bundle_uri;
> struct string_list list = STRING_LIST_INIT_DUP;
> struct remote *remote = NULL;
> int result = 0;
> @@ -2194,6 +2196,12 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> if (dry_run)
> write_fetch_head = 0;
>
> + if (!git_config_get_string_tmp("fetch.bundleuri", &bundle_uri) &&
> + !starts_with(bundle_uri, "remote:")) {
Maybe a silly question, by why would the bundle URI start with 'remote:'
(and why do we silently skip fetching from the URI in that case)?
> + if (fetch_bundle_uri(the_repository, bundle_uri, NULL))
> + warning(_("failed to fetch bundles from '%s'"), bundle_uri);
> + }
> +
> if (all) {
> if (argc == 1)
> die(_("fetch --all does not take a repository argument"));
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
> index 8ff560425ee..3f4d61a915c 100755
> --- a/t/t5558-clone-bundle-uri.sh
> +++ b/t/t5558-clone-bundle-uri.sh
> @@ -465,6 +465,65 @@ test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> cat >expect <<-\EOF &&
> refs/bundles/base
> EOF
> + test_cmp expect refs &&
At this point in the test, only 'base' is in the cloned repo (equivalent to
the contents of 'bundle-1').
> +
> + cat >>"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> + [bundle "bundle-2"]
> + uri = bundle-2.bundle
> + creationToken = 2
> + EOF
> +
> + # Fetch the objects for bundle-2 _and_ bundle-3.
> + GIT_TRACE2_EVENT="$(pwd)/trace1.txt" \
> + git -C fetch-http-4 fetch origin --no-tags \
> + refs/heads/left:refs/heads/left \
> + refs/heads/right:refs/heads/right &&
> +
> + # This fetch should copy two files: the list and bundle-2.
> + test_bundle_downloaded bundle-list trace1.txt &&
> + test_bundle_downloaded bundle-2.bundle trace1.txt &&
> + ! test_bundle_downloaded bundle-1.bundle trace1.txt &&
Now, with 'bundle-2' in the list, fetch 'left' and 'right'. 'bundle-1' is
not fetched because we already have its contents (in 'base'), 'bundle-2' is
downloaded to get 'left', and 'right' is fetched directly from the repo.
> +
> + # received left from bundle-2
> + git -C fetch-http-4 for-each-ref --format="%(refname)" "refs/bundles/*" >refs &&
> + cat >expect <<-\EOF &&
> + refs/bundles/base
> + refs/bundles/left
> + EOF
> + test_cmp expect refs &&
> +
> + cat >>"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> + [bundle "bundle-3"]
> + uri = bundle-3.bundle
> + creationToken = 3
> +
> + [bundle "bundle-4"]
> + uri = bundle-4.bundle
> + creationToken = 4
> + EOF
> +
> + # This fetch should skip bundle-3.bundle, since its objets are
s/objets/objects
> + # already local (we have the requisite commits for bundle-4.bundle).
> + GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
> + git -C fetch-http-4 fetch origin --no-tags \
> + refs/heads/merge:refs/heads/merge &&
> +
> + # This fetch should copy three files: the list, bundle-3, and bundle-4.
> + test_bundle_downloaded bundle-list trace2.txt &&
> + test_bundle_downloaded bundle-4.bundle trace2.txt &&
> + ! test_bundle_downloaded bundle-1.bundle trace2.txt &&
> + ! test_bundle_downloaded bundle-2.bundle trace2.txt &&
> + ! test_bundle_downloaded bundle-3.bundle trace2.txt &&
'bundle-3' and 'bundle-4' are then added to the list and we fetch 'merge'.
The repository already has 'base', 'left', and 'right' so we don't need to
download 'bundle-1', 'bundle-2', and 'bundle-3' respectively; 'bundle-4' is
downloaded to get 'merge'.
> +
> + # received merge ref from bundle-4, but right is missing
> + # because we did not download bundle-3.
> + git -C fetch-http-4 for-each-ref --format="%(refname)" "refs/bundles/*" >refs &&
> +
> + cat >expect <<-\EOF &&
> + refs/bundles/base
> + refs/bundles/left
> + refs/bundles/merge
And this confirms that 'base', 'left', and 'merge' all came from bundles
(and 'right' did not), and everything is working as expected. This is test
is both easy to understand (the comments clearly explain each step without
being overly verbose) and thorough. Looks good!
> + EOF
> test_cmp expect refs
> '
>
^ permalink raw reply
* [PATCH v2] die: fix inconsistencies with usage header
From: Rose via GitGitGadget @ 2023-01-19 20:28 UTC (permalink / raw)
To: git; +Cc: Rose, Seija Kijin
In-Reply-To: <pull.1439.git.git.1674156875354.gitgitgadget@gmail.com>
From: Seija Kijin <doremylover123@gmail.com>
The headers for the die and usage functions
have different parameter names or are missing
the "NORETURN" attribute
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
die: fix inconsistencies with header
The headers for the die and usage functions have different parameter
names or are missing the "NORETURN" attribute
Signed-off-by: Seija Kijin doremylover123@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1439%2FAtariDreams%2Fperror-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1439/AtariDreams/perror-v2
Pull-Request: https://github.com/git/git/pull/1439
Range-diff vs v1:
1: 74e90f0fa5c ! 1: c3b436cea9d die: fix inconsistencies with header
@@ Metadata
Author: Seija Kijin <doremylover123@gmail.com>
## Commit message ##
- die: fix inconsistencies with header
+ die: fix inconsistencies with usage header
The headers for the die and usage functions
have different parameter names or are missing
contrib/credential/osxkeychain/git-credential-osxkeychain.c | 2 +-
contrib/credential/wincred/git-credential-wincred.c | 2 +-
usage.c | 6 +++---
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index e29cc28779d..fa88d621865 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -11,7 +11,7 @@ static char *password;
static UInt16 port;
__attribute__((format (printf, 1, 2)))
-static void die(const char *err, ...)
+static void NORETURN die(const char *err, ...)
{
char msg[4096];
va_list params;
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index ead6e267c78..c0610d7648c 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -12,7 +12,7 @@
#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
__attribute__((format (printf, 1, 2)))
-static void die(const char *err, ...)
+static void NORETURN die(const char *err, ...)
{
char msg[4096];
va_list params;
diff --git a/usage.c b/usage.c
index 5a7c6c346c1..5f5510ceeeb 100644
--- a/usage.c
+++ b/usage.c
@@ -206,7 +206,7 @@ static const char *fmt_with_err(char *buf, int n, const char *fmt)
return buf;
}
-void NORETURN die_errno(const char *fmt, ...)
+void NORETURN die_errno(const char *err, ...)
{
char buf[1024];
va_list params;
@@ -217,8 +217,8 @@ void NORETURN die_errno(const char *fmt, ...)
exit(128);
}
- va_start(params, fmt);
- die_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
+ va_start(params, err);
+ die_routine(fmt_with_err(buf, sizeof(buf), err), params);
va_end(params);
}
base-commit: a7caae2729742fc80147bca1c02ae848cb55921a
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 6/8] bundle-uri: drop bundle.flag from design doc
From: Victoria Dye @ 2023-01-19 19:44 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <afcfd27a883d16009a2c55c3dcfb5ade07132b50.1673037405.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
>
> The Implementation Plan section lists a 'bundle.flag' option that is not
> documented anywhere else. What is documented elsewhere in the document
> and implemented by previous changes is the 'bundle.heuristic' config
> key. For now, a heuristic is required to indicate that a bundle list is
> organized for use during 'git fetch', and it is also sufficient for all
> existing designs.
Good catch, thanks for keeping the documentation consistent!
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> Documentation/technical/bundle-uri.txt | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/technical/bundle-uri.txt b/Documentation/technical/bundle-uri.txt
> index b78d01d9adf..91d3a13e327 100644
> --- a/Documentation/technical/bundle-uri.txt
> +++ b/Documentation/technical/bundle-uri.txt
> @@ -479,14 +479,14 @@ outline for submitting these features:
> (This choice is an opt-in via a config option and a command-line
> option.)
>
> -4. Allow the client to understand the `bundle.flag=forFetch` configuration
> +4. Allow the client to understand the `bundle.heuristic` configuration key
> and the `bundle.<id>.creationToken` heuristic. When `git clone`
> - discovers a bundle URI with `bundle.flag=forFetch`, it configures the
> - client repository to check that bundle URI during later `git fetch <remote>`
> + discovers a bundle URI with `bundle.heuristic`, it configures the client
> + repository to check that bundle URI during later `git fetch <remote>`
> commands.
>
> 5. Allow clients to discover bundle URIs during `git fetch` and configure
> - a bundle URI for later fetches if `bundle.flag=forFetch`.
> + a bundle URI for later fetches if `bundle.heuristic` is set.
>
> 6. Implement the "inspect headers" heuristic to reduce data downloads when
> the `bundle.<id>.creationToken` heuristic is not available.
^ permalink raw reply
* Re: [PATCH 5/8] clone: set fetch.bundleURI if appropriate
From: Victoria Dye @ 2023-01-19 19:42 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <d9c6f50e4f218267c1e8da060ce5b190dc8a709c.1673037405.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
> index cd65d236b43..4f796218aab 100644
> --- a/Documentation/config/fetch.txt
> +++ b/Documentation/config/fetch.txt
> @@ -96,3 +96,11 @@ fetch.writeCommitGraph::
> merge and the write may take longer. Having an updated commit-graph
> file helps performance of many Git commands, including `git merge-base`,
> `git push -f`, and `git log --graph`. Defaults to false.
> +
> +fetch.bundleURI::
> + This value stores a URI for fetching Git object data from a bundle URI
> + before performing an incremental fetch from the origin Git server. If
> + the value is `<uri>` then running `git fetch <args>` is equivalent to
> + first running `git fetch --bundle-uri=<uri>` immediately before
> + `git fetch <args>`. See details of the `--bundle-uri` option in
> + linkgit:git-fetch[1].
Since it's not mentioned from this or any other user-facing documentation
(AFAICT), could you note that this value is set automatically by 'git clone'
iff '--bundle-uri' is specified *and* 'bundle.heuristic' is set for the
initially downloaded bundle list?
It would also be nice to make note of that behavior in the documentation of
the '--bundle-uri' option in 'Documentation/git-clone.txt', since command
documentation in general seems to be more popular/visible to users than
config docs.
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 5453ba5277f..5370617664d 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1248,12 +1248,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> * data from the --bundle-uri option.
> */
> if (bundle_uri) {
> + int has_heuristic = 0;
> +
> /* At this point, we need the_repository to match the cloned repo. */
> if (repo_init(the_repository, git_dir, work_tree))
> warning(_("failed to initialize the repo, skipping bundle URI"));
> - else if (fetch_bundle_uri(the_repository, bundle_uri))
> + else if (fetch_bundle_uri(the_repository, bundle_uri, &has_heuristic))
> warning(_("failed to fetch objects from bundle URI '%s'"),
> bundle_uri);
> + else if (has_heuristic)
> + git_config_set_gently("fetch.bundleuri", bundle_uri);
If the heuristic is anything other than "none", this config value is set in
the repository-scoped config file. Makes sense!
> }
>
> strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD");
> diff --git a/bundle-uri.c b/bundle-uri.c
> index b30c85ba6f2..1dbbbb980eb 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -594,9 +594,10 @@ static int fetch_bundle_list_in_config_format(struct repository *r,
> * it advertises are expected to be bundles, not nested lists.
> * We can drop 'global_list' and 'depth'.
> */
> - if (list_from_bundle.heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN)
> + if (list_from_bundle.heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN) {
> result = fetch_bundles_by_token(r, &list_from_bundle);
> - else if ((result = download_bundle_list(r, &list_from_bundle,
> + global_list->heuristic = BUNDLE_HEURISTIC_CREATIONTOKEN;
If the 'heuristic' field already existed and was being used to apply
bundles, why wasn't 'global_list->heuristic' already being set? Before this
patch, was the 'global_list->heuristic' field not accurately reflecting the
heuristic type of a given bundle list?
If so, I think it'd make sense to move this section to patch 4 [1], since
that's when the heuristic is first applied to the bundle list.
[1] https://lore.kernel.org/git/57c0174d3752fb61a05e0653de9d3057616ed16a.1673037405.git.gitgitgadget@gmail.com/
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
> index d7461ec907e..8ff560425ee 100755
> --- a/t/t5558-clone-bundle-uri.sh
> +++ b/t/t5558-clone-bundle-uri.sh
> @@ -435,6 +435,39 @@ test_expect_success 'clone bundle list (http, creationToken)' '
> test_bundle_downloaded bundle-2.bundle trace-clone.txt
> '
>
> +test_expect_success 'http clone with bundle.heuristic creates fetch.bundleURI' '
> + test_when_finished rm -rf fetch-http-4 trace*.txt &&
> +
> + cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> + [bundle]
> + version = 1
> + mode = all
> + heuristic = creationToken
> +
> + [bundle "bundle-1"]
> + uri = bundle-1.bundle
> + creationToken = 1
> + EOF
> +
> + GIT_TRACE2_EVENT="$(pwd)/trace-clone.txt" \
> + git clone --single-branch --branch=base \
> + --bundle-uri="$HTTPD_URL/bundle-list" \
> + "$HTTPD_URL/smart/fetch.git" fetch-http-4 &&
> +
> + test_cmp_config -C fetch-http-4 "$HTTPD_URL/bundle-list" fetch.bundleuri &&
> +
> + # The clone should copy two files: the list and bundle-1.
> + test_bundle_downloaded bundle-list trace-clone.txt &&
> + test_bundle_downloaded bundle-1.bundle trace-clone.txt &&
> +
> + # only received base ref from bundle-1
> + git -C fetch-http-4 for-each-ref --format="%(refname)" "refs/bundles/*" >refs &&
> + cat >expect <<-\EOF &&
> + refs/bundles/base
> + EOF
> + test_cmp expect refs
> +'
This test looks good - it verifies the config update, bundle download, and
unbundle all work as intended.
> +
> # Do not add tests here unless they use the HTTP server, as they will
> # not run unless the HTTP dependencies exist.
>
^ permalink raw reply
* [PATCH] die: fix inconsistencies with header
From: Rose via GitGitGadget @ 2023-01-19 19:34 UTC (permalink / raw)
To: git; +Cc: Rose, Seija Kijin
From: Seija Kijin <doremylover123@gmail.com>
The headers for the die and usage functions
have different parameter names or are missing
the "NORETURN" attribute
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
die: fix inconsistencies with header
The headers for the die and usage functions have different parameter
names or are missing the "NORETURN" attribute
Signed-off-by: Seija Kijin doremylover123@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1439%2FAtariDreams%2Fperror-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1439/AtariDreams/perror-v1
Pull-Request: https://github.com/git/git/pull/1439
contrib/credential/osxkeychain/git-credential-osxkeychain.c | 2 +-
contrib/credential/wincred/git-credential-wincred.c | 2 +-
usage.c | 6 +++---
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index e29cc28779d..fa88d621865 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -11,7 +11,7 @@ static char *password;
static UInt16 port;
__attribute__((format (printf, 1, 2)))
-static void die(const char *err, ...)
+static void NORETURN die(const char *err, ...)
{
char msg[4096];
va_list params;
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index ead6e267c78..c0610d7648c 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -12,7 +12,7 @@
#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
__attribute__((format (printf, 1, 2)))
-static void die(const char *err, ...)
+static void NORETURN die(const char *err, ...)
{
char msg[4096];
va_list params;
diff --git a/usage.c b/usage.c
index 5a7c6c346c1..5f5510ceeeb 100644
--- a/usage.c
+++ b/usage.c
@@ -206,7 +206,7 @@ static const char *fmt_with_err(char *buf, int n, const char *fmt)
return buf;
}
-void NORETURN die_errno(const char *fmt, ...)
+void NORETURN die_errno(const char *err, ...)
{
char buf[1024];
va_list params;
@@ -217,8 +217,8 @@ void NORETURN die_errno(const char *fmt, ...)
exit(128);
}
- va_start(params, fmt);
- die_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
+ va_start(params, err);
+ die_routine(fmt_with_err(buf, sizeof(buf), err), params);
va_end(params);
}
base-commit: a7caae2729742fc80147bca1c02ae848cb55921a
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 4/8] bundle-uri: download in creationToken order
From: Victoria Dye @ 2023-01-19 18:32 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen, Derrick Stolee
In-Reply-To: <57c0174d3752fb61a05e0653de9d3057616ed16a.1673037405.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
> +static int fetch_bundles_by_token(struct repository *r,
> + struct bundle_list *list)
> +{
> + int cur;
> + int pop_or_push = 0;
> + struct bundle_list_context ctx = {
> + .r = r,
> + .list = list,
> + .mode = list->mode,
> + };
> + struct sorted_bundle_list sorted = {
> + .alloc = hashmap_get_size(&list->bundles),
> + };
> +
> + ALLOC_ARRAY(sorted.items, sorted.alloc);
> +
> + for_all_bundles_in_list(list, insert_bundle, &sorted);
> +
> + QSORT(sorted.items, sorted.nr, compare_creation_token);
So, at this point, 'sorted' is ordered by *decreasing* creation token? With
the loop below being somewhat complex, it would be nice to have a comment
mention that explicitly so readers have a clear understanding of the
"initial state" before entering the loop.
> +
> + /*
> + * Use a stack-based approach to download the bundles and attempt
> + * to unbundle them in decreasing order by creation token. If we
> + * fail to unbundle (after a successful download) then move to the
> + * next non-downloaded bundle (push to the stack) and attempt
> + * downloading. Once we succeed in applying a bundle, move to the
> + * previous unapplied bundle (pop the stack) and attempt to unbundle
> + * it again.
> + *
> + * In the case of a fresh clone, we will likely download all of the
> + * bundles before successfully unbundling the oldest one, then the
> + * rest of the bundles unbundle successfully in increasing order
> + * of creationToken.
> + *
> + * If there are existing objects, then this process may terminate
> + * early when all required commits from "new" bundles exist in the
> + * repo's object store.
> + */
> + cur = 0;
> + while (cur >= 0 && cur < sorted.nr) {
> + struct remote_bundle_info *bundle = sorted.items[cur];
> + if (!bundle->file) {
> + /* Not downloaded yet. Try downloading. */
> + if (download_bundle_to_file(bundle, &ctx)) {
> + /* Failure. Push to the stack. */
> + pop_or_push = 1;
> + goto stack_operation;
Personally, I find the use of "stack" terminology more confusing than not.
'sorted' isn't really a stack, it's a list with fixed contents being
traversed stepwise with 'cur'. For example, 'pop_or_push' being renamed to
'move_direction' or 'step' something along those lines might more clearly
indicate what's actually happening with 'cur' & 'sorted'.
> + }
> +
> + /* We expect bundles when using creationTokens. */
> + if (!is_bundle(bundle->file, 1)) {
> + warning(_("file downloaded from '%s' is not a bundle"),
> + bundle->uri);
> + break;
> + }
> + }
> +
> + if (bundle->file && !bundle->unbundled) {
> + /*
> + * This was downloaded, but not successfully
> + * unbundled. Try unbundling again.
> + */
> + if (unbundle_from_file(ctx.r, bundle->file)) {
> + /* Failed to unbundle. Push to stack. */
> + pop_or_push = 1;
> + } else {
> + /* Succeeded in unbundle. Pop stack. */
> + pop_or_push = -1;
> + }
> + }
> +
> + /*
> + * Else case: downloaded and unbundled successfully.
> + * Skip this by moving in the same direction as the
> + * previous step.
> + */
> +
> +stack_operation:
> + /* Move in the specified direction and repeat. */
> + cur += pop_or_push;
> + }
After reading through this loop, I generally understood *what* its doing,
but didn't really follow *why* the download & unbundling is done like this.
I needed to refer back to the design doc
('Documentation/technical/bundle-uri.txt') to understand some basic
assumptions about bundles:
- A new bundle's creation token should always be strictly greater than the
previous newest bundle's creation token. I don't see any special handling
for equal creation tokens, so my assumption is that the sorting of the
list arbitrarily assigns one to be "greater" and it's dealt with that way.
- The bundle with the lowest creation token should always be unbundleable,
since it contains all objects in an initial clone.
I do still have some questions, though:
- Why would 'unbundle_from_file()' fail? From context clues, I'm guessing it
fails if it has some unreachable objects (as in an incremental bundle), or
if it's corrupted somehow.
- Why would 'download_bundle_to_file()' to fail? Unlike
'unbundle_from_file()', it looks like that represents an unexpected error.
Also - it seems like one of the assumptions here is that, if a bundle can't
be downloaded & unbundled, no bundle with a higher creation token can be
successfully unbundled ('download_bundle_to_file()' sets 'pop_or_push' to
'1', which will cause the loop to ignore all higher-token bundles and return
a nonzero value from the function).
I don't think that assumption is necessarily true, though. Suppose you have
a "base" bundle 100 and incremental bundles 101 and 102. 101 has all objects
from a new branch A, and 102 has all objects from a newer branch B (not
based on any objects in A). In this case, 102 could be unbundled even if 101
is corrupted/can't be downloaded, but we'd run into issues if we store 102
as the "latest unbundled creation token" (because it implies that 101 was
unbundled).
Is there any benefit to trying to unbundle those higher bundles *without*
advancing the "latest creation token"? E.g. in my example, unbundle 102 but
store '100' as the latest creation token?
> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
> index 328caeeae9a..d7461ec907e 100755
> --- a/t/t5558-clone-bundle-uri.sh
> +++ b/t/t5558-clone-bundle-uri.sh
> @@ -368,6 +368,8 @@ test_expect_success 'clone bundle list (HTTP, any mode)' '
> '
>
> test_expect_success 'clone bundle list (http, creationToken)' '
> + test_when_finished rm -f trace*.txt &&
> +
> cp clone-from/bundle-*.bundle "$HTTPD_DOCUMENT_ROOT_PATH/" &&
> cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> [bundle]
> @@ -392,10 +394,45 @@ test_expect_success 'clone bundle list (http, creationToken)' '
> creationToken = 4
> EOF
>
> - git clone --bundle-uri="$HTTPD_URL/bundle-list" . clone-list-http-2 &&
> + GIT_TRACE2_EVENT=$(pwd)/trace-clone.txt \
> + git clone --bundle-uri="$HTTPD_URL/bundle-list" \
> + clone-from clone-list-http-2 &&
>
> git -C clone-from for-each-ref --format="%(objectname)" >oids &&
> - git -C clone-list-http-2 cat-file --batch-check <oids
> + git -C clone-list-http-2 cat-file --batch-check <oids &&
> +
> + for b in 1 2 3 4
> + do
> + test_bundle_downloaded bundle-$b.bundle trace-clone.txt ||
> + return 1
> + done
If I understand correctly, these added conditions would have passed even if
they were added when the test was initially created in patch 1, but they're
added here to tie them to the implementation of the creationToken heuristic?
Seems reasonable.
> +'
> +
> +test_expect_success 'clone bundle list (http, creationToken)' '
This new test has the same name as the one above it - how does it differ
from that one? Whatever the difference is, can that be noted somehow in the
title or a comment?
> + test_when_finished rm -f trace*.txt &&
> +
> + cp clone-from/bundle-*.bundle "$HTTPD_DOCUMENT_ROOT_PATH/" &&
> + cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
> + [bundle]
> + version = 1
> + mode = all
> + heuristic = creationToken
> +
> + [bundle "bundle-1"]
> + uri = bundle-1.bundle
> + creationToken = 1
> +
> + [bundle "bundle-2"]
> + uri = bundle-2.bundle
> + creationToken = 2
> + EOF
> +
> + GIT_TRACE2_EVENT=$(pwd)/trace-clone.txt \
> + git clone --bundle-uri="$HTTPD_URL/bundle-list" \
> + clone-from clone-token-http &&
> +
> + test_bundle_downloaded bundle-1.bundle trace-clone.txt &&
> + test_bundle_downloaded bundle-2.bundle trace-clone.txt
> '
>
> # Do not add tests here unless they use the HTTP server, as they will
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 1928ea1dd7c..57476b6e6d7 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -831,6 +831,56 @@ test_expect_success 'auto-discover multiple bundles from HTTP clone' '
> grep -f pattern trace.txt
> '
>
> +# Usage: test_bundle_downloaded <bundle-id> <trace-filename>
> +test_bundle_downloaded () {
> + cat >pattern <<-EOF &&
> + "event":"child_start".*"argv":\["git-remote-https","$HTTPD_URL/$1.bundle"\]
> + EOF
> + grep -f pattern "$2"
> +}
This function is the same as the one created in 't5558'. Should it be moved
to 'lib-bundle.sh' or 'test-lib.sh' to avoid duplicate code?
> +
> +test_expect_success 'auto-discover multiple bundles from HTTP clone: creationToken heuristic' '
> + test_when_finished rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/repo4.git" &&
> + test_when_finished rm -rf clone-heuristic trace*.txt &&
> +
> + test_commit -C src newest &&
> + git -C src bundle create "$HTTPD_DOCUMENT_ROOT_PATH/newest.bundle" HEAD~1..HEAD &&
> + git clone --bare --no-local src "$HTTPD_DOCUMENT_ROOT_PATH/repo4.git" &&
> +
> + cat >>"$HTTPD_DOCUMENT_ROOT_PATH/repo4.git/config" <<-EOF &&
> + [uploadPack]
> + advertiseBundleURIs = true
> +
> + [bundle]
> + version = 1
> + mode = all
> + heuristic = creationToken
> +
> + [bundle "everything"]
> + uri = $HTTPD_URL/everything.bundle
> + creationtoken = 1
> +
> + [bundle "new"]
> + uri = $HTTPD_URL/new.bundle
> + creationtoken = 2
> +
> + [bundle "newest"]
> + uri = $HTTPD_URL/newest.bundle
> + creationtoken = 3
> + EOF
> +
> + GIT_TRACE2_EVENT="$(pwd)/trace-clone.txt" \
> + git -c protocol.version=2 \
> + -c transfer.bundleURI=true clone \
> + "$HTTPD_URL/smart/repo4.git" clone-heuristic &&
> +
> + # We should fetch all bundles
> + for b in everything new newest
> + do
> + test_bundle_downloaded $b trace-clone.txt || return 1
> + done
> +'
> +
> # DO NOT add non-httpd-specific tests here, because the last part of this
> # test script is only executed when httpd is available and enabled.
>
^ permalink raw reply
* [PATCH v5 5/5] doc: pretty-formats note wide char limitations, and add tests
From: Philip Oakley @ 2023-01-19 18:18 UTC (permalink / raw)
To: GitList, Junio C Hamano; +Cc: Taylor Blau, NSENGIYUMVA WILBERFORCE, self
In-Reply-To: <20230119181827.1319-1-philipoakley@iee.email>
The previous commits added clarifications to the column alignment
placeholders, note that the spaces are optional around the parameters.
Also, a proposed extension [1] to allow hard truncation (without
ellipsis '..') highlighted that the existing code does not play well
with wide characters, such as Asian fonts and emojis.
For example, N wide characters take 2N columns so won't fit an odd number
column width, causing misalignment somewhere.
Further analysis also showed that decomposed characters, e.g. separate
`a` + `umlaut` Unicode code-points may also be mis-counted, in some cases
leaving multiple loose `umlauts` all combined together.
Add some notes about these limitations, and add basic tests to demonstrate
them.
The chosen solution for the tests is to substitute any wide character
that overlaps a splitting boundary for the unicode vertical ellipsis
code point as a rare but 'obvious' substitution.
An alternative could be the substitution with a single dot '.' which
matches regular expression usage, and our two dot ellipsis, and further
in scenarios where the bulk of the text is wide characters, would be
obvious. In mainly 'ascii' scenarios a singleton emoji being substituted
by a dot could be confusing.
It is enough that the tests fail cleanly. The final choice for the
substitute character can be deferred.
[1]
https://lore.kernel.org/git/20221030185614.3842-1-philipoakley@iee.email/
Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
Documentation/pretty-formats.txt | 5 +++++
t/t4205-log-pretty-formats.sh | 27 +++++++++++++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index e51f1e54e1..3b71334459 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -157,6 +157,11 @@ The placeholders are:
only works correctly with N >= 2.
Note 2: spaces around the N and M (see below)
values are optional.
+ Note 3: Emojis and other wide characters
+ will take two display columns, which may
+ over-run column boundaries.
+ Note 4: decomposed character combining marks
+ may be misplaced at padding boundaries.
'%<|( <M> )':: make the next placeholder take at least until Mth
display column, padding spaces on the right if necessary.
Use negative M values for column positions measured
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 0404491d6e..2cba0e0c56 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -1018,4 +1018,31 @@ test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' '
test_cmp expect actual
'
+# pretty-formats note wide char limitations, and add tests
+test_expect_failure 'wide and decomposed characters column counting' '
+
+# from t/lib-unicode-nfc-nfd.sh hex values converted to octal
+ utf8_nfc=$(printf "\303\251") && # e acute combined.
+ utf8_nfd=$(printf "\145\314\201") && # e with a combining acute (i.e. decomposed)
+ utf8_emoji=$(printf "\360\237\221\250") &&
+
+# replacement character when requesting a wide char fits in a single display colum.
+# "half wide" alternative could be a plain ASCII dot `.`
+ utf8_vert_ell=$(printf "\342\213\256") &&
+
+# use ${xxx} here!
+ nfc10="${utf8_nfc}${utf8_nfc}${utf8_nfc}${utf8_nfc}${utf8_nfc}${utf8_nfc}${utf8_nfc}${utf8_nfc}${utf8_nfc}${utf8_nfc}" &&
+ nfd10="${utf8_nfd}${utf8_nfd}${utf8_nfd}${utf8_nfd}${utf8_nfd}${utf8_nfd}${utf8_nfd}${utf8_nfd}${utf8_nfd}${utf8_nfd}" &&
+ emoji5="${utf8_emoji}${utf8_emoji}${utf8_emoji}${utf8_emoji}${utf8_emoji}" &&
+# emoji5 uses 10 display columns
+
+ test_commit "abcdefghij" &&
+ test_commit --no-tag "${nfc10}" &&
+ test_commit --no-tag "${nfd10}" &&
+ test_commit --no-tag "${emoji5}" &&
+ printf "${utf8_emoji}..${utf8_emoji}${utf8_vert_ell}\n${utf8_nfd}..${utf8_nfd}${utf8_nfd}\n${utf8_nfc}..${utf8_nfc}${utf8_nfc}\na..ij\n" >expected &&
+ git log --format="%<(5,mtrunc)%s" -4 >actual &&
+ test_cmp expected actual
+'
+
test_done
--
2.39.1.windows.1
^ permalink raw reply related
* [PATCH v5 4/5] doc: pretty-formats describe use of ellipsis in truncation
From: Philip Oakley @ 2023-01-19 18:18 UTC (permalink / raw)
To: GitList, Junio C Hamano; +Cc: Taylor Blau, NSENGIYUMVA WILBERFORCE, self
In-Reply-To: <20230119181827.1319-1-philipoakley@iee.email>
Commit a7f01c6b4d (pretty: support truncating in %>, %< and %><,
2013-04-19) added the use of ellipsis when truncating placeholder
values.
Show our 'two dot' ellipsis, and examples for the left, middle and
right truncation to avoid any confusion as to which end of the string
is adjusted. (cf justification and sub-string).
Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
Documentation/pretty-formats.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index cbca60a196..e51f1e54e1 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -149,9 +149,9 @@ The placeholders are:
'%<( <N> [,trunc|ltrunc|mtrunc])':: make the next placeholder take at
least N column widths, padding spaces on
the right if necessary. Optionally
- truncate at the beginning (ltrunc),
- the middle (mtrunc) or the end
- (trunc) if the output is longer than
+ truncate (with ellipsis '..') at the left (ltrunc) `..ft`,
+ the middle (mtrunc) `mi..le`, or the end
+ (trunc) `rig..`, if the output is longer than
N columns.
Note 1: that truncating
only works correctly with N >= 2.
--
2.39.1.windows.1
^ permalink raw reply related
* [PATCH v5 2/5] doc: pretty-formats: delineate `%<|(` parameter values
From: Philip Oakley @ 2023-01-19 18:18 UTC (permalink / raw)
To: GitList, Junio C Hamano; +Cc: Taylor Blau, NSENGIYUMVA WILBERFORCE, self
In-Reply-To: <20230119181827.1319-1-philipoakley@iee.email>
Commit a57523428b4 (pretty: support padding placeholders, %< %> and %><,
2013-04-19) introduced column width place holders. It also added
separate column position `%<|(` placeholders for display screen based
placement.
Change the display screen parameter reference from 'N' to 'M' and
corresponding descriptives to make the distinction clearer.
Signed-off-by: Philip Oakley <philipoakley@iee.email>
---
Documentation/pretty-formats.txt | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 02bec23509..8cc1072196 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -147,7 +147,7 @@ The placeholders are:
'%w([<w>[,<i1>[,<i2>]]])':: switch line wrapping, like the -w option of
linkgit:git-shortlog[1].
'%<( <N> [,trunc|ltrunc|mtrunc])':: make the next placeholder take at
- least N columns, padding spaces on
+ least N column widths, padding spaces on
the right if necessary. Optionally
truncate at the beginning (ltrunc),
the middle (mtrunc) or the end
@@ -155,18 +155,18 @@ The placeholders are:
N columns.
Note 1: that truncating
only works correctly with N >= 2.
- Note 2: spaces around the N
+ Note 2: spaces around the N and M (see below)
values are optional.
-'%<|( <N> )':: make the next placeholder take at least until Nth
- columns, padding spaces on the right if necessary
-'%>( <N> )', '%>|( <N> )':: similar to '%<( <N> )', '%<|( <N> )' respectively,
+'%<|( <M> )':: make the next placeholder take at least until Mth
+ display column, padding spaces on the right if necessary
+'%>( <N> )', '%>|( <M> )':: similar to '%<( <N> )', '%<|( <M> )' respectively,
but padding spaces on the left
-'%>>( <N> )', '%>>|( <N> )':: similar to '%>( <N> )', '%>|( <N> )'
+'%>>( <N> )', '%>>|( <M> )':: similar to '%>( <N> )', '%>|( <M> )'
respectively, except that if the next
placeholder takes more spaces than given and
there are spaces on its left, use those
spaces
-'%><( <N> )', '%><|( <N> )':: similar to '%<( <N> )', '%<|( <N> )'
+'%><( <N> )', '%><|( <M> )':: similar to '%<( <N> )', '%<|( <M> )'
respectively, but padding both sides
(i.e. the text is centered)
--
2.39.1.windows.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox