* [PATCH 4/7] refs: separate decoration type from default filter
From: Andy Koppe @ 2023-10-19 19:39 UTC (permalink / raw)
To: git; +Cc: Andy Koppe
In-Reply-To: <20231003205442.22963-1-andy.koppe@gmail.com>
Add 'include' bit to struct ref_namespace_info to determine whether a
ref namespace is to be included in the default decoration filters,
instead of using the decoration type for the purpose.
This is to allow adding ref namespaces that do have a decoration type
but that are not shown by default.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
builtin/log.c | 6 ++----
refs.c | 6 ++++++
refs.h | 4 ++++
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index ba775d7b5cf..25d73c25697 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -227,10 +227,8 @@ static void set_default_decoration_filter(struct decoration_filter *decoration_f
* populate with sensible defaults.
*/
for (i = 0; i < ARRAY_SIZE(ref_namespace); i++) {
- if (!ref_namespace[i].decoration)
- continue;
-
- string_list_append(include, ref_namespace[i].ref);
+ if (ref_namespace[i].include)
+ string_list_append(include, ref_namespace[i].ref);
}
}
diff --git a/refs.c b/refs.c
index fcae5dddc60..416c35f0c2f 100644
--- a/refs.c
+++ b/refs.c
@@ -70,14 +70,17 @@ struct ref_namespace_info ref_namespace[] = {
.ref = "HEAD",
.decoration = DECORATION_REF_HEAD,
.exact = 1,
+ .include = 1,
},
[NAMESPACE_BRANCHES] = {
.ref = "refs/heads/",
.decoration = DECORATION_REF_LOCAL,
+ .include = 1,
},
[NAMESPACE_TAGS] = {
.ref = "refs/tags/",
.decoration = DECORATION_REF_TAG,
+ .include = 1,
},
[NAMESPACE_REMOTE_REFS] = {
/*
@@ -87,6 +90,7 @@ struct ref_namespace_info ref_namespace[] = {
*/
.ref = "refs/remotes/",
.decoration = DECORATION_REF_REMOTE,
+ .include = 1,
},
[NAMESPACE_STASH] = {
/*
@@ -96,6 +100,7 @@ struct ref_namespace_info ref_namespace[] = {
.ref = "refs/stash",
.exact = 1,
.decoration = DECORATION_REF_STASH,
+ .include = 1,
},
[NAMESPACE_REPLACE] = {
/*
@@ -107,6 +112,7 @@ struct ref_namespace_info ref_namespace[] = {
*/
.ref = "refs/replace/",
.decoration = DECORATION_GRAFTED,
+ .include = 1,
},
[NAMESPACE_NOTES] = {
/*
diff --git a/refs.h b/refs.h
index 23211a5ea1c..4b054d30fe5 100644
--- a/refs.h
+++ b/refs.h
@@ -987,10 +987,14 @@ struct ref_namespace_info {
* If 'exact' is true, then we must match the 'ref' exactly.
* Otherwise, use a prefix match.
*
+ * If 'include' is true, the namespace is included in the
+ * default decoration filters.
+ *
* 'ref_updated' is for internal use. It represents whether the
* 'ref' value was replaced from its original literal version.
*/
unsigned exact:1,
+ include:1,
ref_updated:1;
};
--
2.42.GIT
^ permalink raw reply related
* [PATCH 3/7] log: add color.decorate.symbol config option
From: Andy Koppe @ 2023-10-19 19:39 UTC (permalink / raw)
To: git; +Cc: Andy Koppe
In-Reply-To: <20231003205442.22963-1-andy.koppe@gmail.com>
Add new 'color.decorate.symbol' config option for determining the color
of the prefix, suffix, separator and arrow symbols used in --decorate
output and related log format placeholders, to allow them to be colored
differently from commit hashes.
For backward compatibility, fall back to the commit hash color that can
be specified with the 'color.diff.commit' option if the new option is
not provided.
Add the setting to the color.decorate.<slot> documentation.
Amend t4207-log-decoration-colors.sh to test it. Put ${c_reset} elements
in the expected output at the end of lines for consistency.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/config/color.txt | 2 ++
commit.h | 1 +
log-tree.c | 15 ++++++---
t/t4207-log-decoration-colors.sh | 58 +++++++++++++++++---------------
4 files changed, 43 insertions(+), 33 deletions(-)
diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
index b0e2eccad95..ba9f56885e3 100644
--- a/Documentation/config/color.txt
+++ b/Documentation/config/color.txt
@@ -92,6 +92,8 @@ color.decorate.<slot>::
the stash ref
`grafted`;;
grafted commits (used to implement shallow clones)
+`symbol`;;
+ punctuation surrounding the other elements
--
color.grep::
diff --git a/commit.h b/commit.h
index 28928833c54..cb13e4d5baa 100644
--- a/commit.h
+++ b/commit.h
@@ -56,6 +56,7 @@ enum decoration_type {
DECORATION_REF_STASH,
DECORATION_REF_HEAD,
DECORATION_GRAFTED,
+ DECORATION_SYMBOL,
};
void add_name_decoration(enum decoration_type type, const char *name, struct object *obj);
diff --git a/log-tree.c b/log-tree.c
index 8bdf889f022..890024f205b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -41,6 +41,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {
[DECORATION_REF_STASH] = GIT_COLOR_BOLD_MAGENTA,
[DECORATION_REF_HEAD] = GIT_COLOR_BOLD_CYAN,
[DECORATION_GRAFTED] = GIT_COLOR_BOLD_BLUE,
+ [DECORATION_SYMBOL] = GIT_COLOR_NIL,
};
static const char *color_decorate_slots[] = {
@@ -50,6 +51,7 @@ static const char *color_decorate_slots[] = {
[DECORATION_REF_STASH] = "stash",
[DECORATION_REF_HEAD] = "HEAD",
[DECORATION_GRAFTED] = "grafted",
+ [DECORATION_SYMBOL] = "symbol",
};
static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)
@@ -312,7 +314,7 @@ void format_decorations(struct strbuf *sb,
{
const struct name_decoration *decoration;
const struct name_decoration *current_and_HEAD;
- const char *color_commit, *color_reset;
+ const char *color_symbol, *color_reset;
const char *prefix = " (";
const char *suffix = ")";
@@ -337,7 +339,10 @@ void format_decorations(struct strbuf *sb,
tag = opts->tag;
}
- color_commit = diff_get_color(use_color, DIFF_COMMIT);
+ color_symbol = decorate_get_color(use_color, DECORATION_SYMBOL);
+ if (color_is_nil(color_symbol))
+ color_symbol = diff_get_color(use_color, DIFF_COMMIT);
+
color_reset = decorate_get_color(use_color, DECORATION_NONE);
current_and_HEAD = current_pointed_by_HEAD(decoration);
@@ -352,7 +357,7 @@ void format_decorations(struct strbuf *sb,
decorate_get_color(use_color, decoration->type);
if (*prefix) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbol);
strbuf_addstr(sb, prefix);
strbuf_addstr(sb, color_reset);
}
@@ -369,7 +374,7 @@ void format_decorations(struct strbuf *sb,
if (current_and_HEAD &&
decoration->type == DECORATION_REF_HEAD) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbol);
strbuf_addstr(sb, pointer);
strbuf_addstr(sb, color_reset);
strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
@@ -382,7 +387,7 @@ void format_decorations(struct strbuf *sb,
decoration = decoration->next;
}
if (*suffix) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbol);
strbuf_addstr(sb, suffix);
strbuf_addstr(sb, color_reset);
}
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index 21986a866df..f4173b61141 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -18,6 +18,7 @@ test_expect_success setup '
git config color.decorate.tag "reverse bold yellow" &&
git config color.decorate.stash magenta &&
git config color.decorate.grafted black &&
+ git config color.decorate.symbol white &&
git config color.decorate.HEAD cyan &&
c_reset="<RESET>" &&
@@ -29,6 +30,7 @@ test_expect_success setup '
c_stash="<MAGENTA>" &&
c_HEAD="<CYAN>" &&
c_grafted="<BLACK>" &&
+ c_symbol="<WHITE>" &&
test_commit A &&
git clone . other &&
@@ -53,17 +55,17 @@ cmp_filtered_decorations () {
# to this test since it does not contain any decoration, hence --first-parent
test_expect_success 'commit decorations colored correctly' '
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B
-${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A1${c_reset}${c_commit}, \
-${c_reset}${c_remoteBranch}other/main${c_reset}${c_commit})${c_reset} A1
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_stash}refs/stash${c_reset}${c_commit})${c_reset} On main: Changes to A.t
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbol} -> ${c_reset}${c_branch}main${c_reset}${c_symbol}, ${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_symbol}, ${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_symbol})${c_reset} B
+${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A1${c_reset}${c_symbol}, ${c_reset}\
+${c_remoteBranch}other/main${c_reset}${c_symbol})${c_reset} A1
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_stash}refs/stash${c_reset}${c_symbol})${c_reset} On main: Changes to A.t
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbol})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always --all >actual &&
@@ -78,14 +80,14 @@ test_expect_success 'test coloring with replace-objects' '
git replace HEAD~1 HEAD~2 &&
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit})${c_reset} D
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}C${c_reset}${c_commit}, \
-${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} B
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbol} -> ${c_reset}${c_branch}main${c_reset}${c_symbol}, ${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_symbol})${c_reset} D
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}C${c_reset}${c_symbol}, ${c_reset}\
+${c_grafted}replaced${c_reset}${c_symbol})${c_reset} B
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbol})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual &&
@@ -104,15 +106,15 @@ test_expect_success 'test coloring with grafted commit' '
git replace --graft HEAD HEAD~2 &&
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit}, \
-${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} D
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbol} -> ${c_reset}${c_branch}main${c_reset}${c_symbol}, ${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_symbol}, ${c_reset}\
+${c_grafted}replaced${c_reset}${c_symbol})${c_reset} D
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_symbol}, ${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_symbol})${c_reset} B
+ ${c_commit}COMMIT_ID${c_reset}${c_symbol} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbol})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual &&
--
2.42.GIT
^ permalink raw reply related
* [PATCH 2/7] log: use designated inits for decoration_colors
From: Andy Koppe @ 2023-10-19 19:39 UTC (permalink / raw)
To: git; +Cc: Andy Koppe
In-Reply-To: <20231003205442.22963-1-andy.koppe@gmail.com>
Use designated initializers instead of comments to denote the slots in
the decoration_colors array for holding color settings, to reduce the
likelihood of mistakes when extending the array.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
log-tree.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index 504da6b519e..8bdf889f022 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -34,13 +34,13 @@ static int decoration_loaded;
static int decoration_flags;
static char decoration_colors[][COLOR_MAXLEN] = {
- GIT_COLOR_RESET,
- GIT_COLOR_BOLD_GREEN, /* REF_LOCAL */
- GIT_COLOR_BOLD_RED, /* REF_REMOTE */
- GIT_COLOR_BOLD_YELLOW, /* REF_TAG */
- GIT_COLOR_BOLD_MAGENTA, /* REF_STASH */
- GIT_COLOR_BOLD_CYAN, /* REF_HEAD */
- GIT_COLOR_BOLD_BLUE, /* GRAFTED */
+ [DECORATION_NONE] = GIT_COLOR_RESET,
+ [DECORATION_REF_LOCAL] = GIT_COLOR_BOLD_GREEN,
+ [DECORATION_REF_REMOTE] = GIT_COLOR_BOLD_RED,
+ [DECORATION_REF_TAG] = GIT_COLOR_BOLD_YELLOW,
+ [DECORATION_REF_STASH] = GIT_COLOR_BOLD_MAGENTA,
+ [DECORATION_REF_HEAD] = GIT_COLOR_BOLD_CYAN,
+ [DECORATION_GRAFTED] = GIT_COLOR_BOLD_BLUE,
};
static const char *color_decorate_slots[] = {
--
2.42.GIT
^ permalink raw reply related
* [PATCH 1/7] config: restructure color.decorate documentation
From: Andy Koppe @ 2023-10-19 19:39 UTC (permalink / raw)
To: git; +Cc: Andy Koppe
In-Reply-To: <20231003205442.22963-1-andy.koppe@gmail.com>
List color.decorate slots in git-config documentation one-by-one in the
same way as color.grep slots, to aid readability and make it easier to
add slots.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/config/color.txt | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
index 1795b2d16be..b0e2eccad95 100644
--- a/Documentation/config/color.txt
+++ b/Documentation/config/color.txt
@@ -74,10 +74,25 @@ color.diff.<slot>::
`oldBold`, and `newBold` (see linkgit:git-range-diff[1] for details).
color.decorate.<slot>::
- Use customized color for 'git log --decorate' output. `<slot>` is one
- of `branch`, `remoteBranch`, `tag`, `stash` or `HEAD` for local
- branches, remote-tracking branches, tags, stash and HEAD, respectively
- and `grafted` for grafted commits.
+ Use customized color for the output of `git log --decorate` as well as
+ the `%d`, `%D` and `%(decorate)` placeholders in custom log formats,
+ whereby `<slot>` specifies which decoration elements the color applies
+ to:
++
+--
+`HEAD`;;
+ the current HEAD
+`branch`;;
+ local branches
+`remoteBranch`;;
+ remote-tracking branches
+`tag`;;
+ lightweight and annotated tags
+`stash`;;
+ the stash ref
+`grafted`;;
+ grafted commits (used to implement shallow clones)
+--
color.grep::
When set to `always`, always highlight matches. When `false` (or
--
2.42.GIT
^ permalink raw reply related
* [PATCH 0/7] log: decorate pseudorefs and other refs
From: Andy Koppe @ 2023-10-19 19:39 UTC (permalink / raw)
To: git; +Cc: Andy Koppe
In-Reply-To: <20231003205442.22963-1-andy.koppe@gmail.com>
This patch series adds three slots to the color.decorate.<slot> config
option:
- 'symbol' for coloring the punctuation symbols used around the refs in
decorations, which currently use the same color as the commit hash.
- 'ref' for coloring refs other than branches, remote-tracking branches,
tags and the stash, which currently are not colored when included in
decorations through custom decoration filter options.
- 'pseudoref' for coloring pseudorefs such as ORIG_HEAD or MERGE_HEAD.
Include them in decorations by default.
This series is to replace the 'decorate: add color.decorate.symbols
config option' patch proposed at:
https://lore.kernel.org/git/20231003205442.22963-1-andy.koppe@gmail.com
Andy Koppe (7):
config: restructure color.decorate documentation
log: use designated inits for decoration_colors
log: add color.decorate.symbol config option
refs: separate decoration type from default filter
log: add color.decorate.ref option for other refs
refs: exempt pseudoref patterns from prefixing
log: show pseudorefs in decorations
Documentation/config/color.txt | 30 +++++++-
Documentation/git-log.txt | 7 +-
builtin/log.c | 6 +-
commit.h | 3 +
log-tree.c | 60 ++++++++++++---
refs.c | 62 ++++++++++++++--
refs.h | 14 ++++
t/t4013/diff.log_--decorate=full_--all | 2 +-
..._--decorate=full_--clear-decorations_--all | 4 +-
t/t4013/diff.log_--decorate_--all | 2 +-
...f.log_--decorate_--clear-decorations_--all | 4 +-
t/t4202-log.sh | 23 +++---
t/t4207-log-decoration-colors.sh | 74 +++++++++++--------
13 files changed, 216 insertions(+), 75 deletions(-)
--
2.42.GIT
^ permalink raw reply
* Re: [PATCH v4 11/15] replay: use standard revision ranges
From: Linus Arver @ 2023-10-19 19:26 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood, Calvin Wan,
Toon Claes, Christian Couder
In-Reply-To: <CAP8UFD3f94NnBgkzkezcALJxamsz+-oPfqKe8XGNMuJ+g--z6g@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Sat, Sep 9, 2023 at 12:55 AM Linus Arver <linusa@google.com> wrote:
>>
>> Hi Christian,
>>
>> I am only reviewing the docs. To assume the mindset of a Git user
>> unfamiliar with this command, I purposely did not read the cover letter
>> until after this review was done.
>
> Ok, thanks!
>
> [...]
>> > + Starting point at which to create the new commits. May be any
>> > + valid commit, and not just an existing branch name.
>>
>> Add "See linkgit:gitrevisions[7]." at the end?
>
> I don't think it's worth it to mention "linkgit:gitrevisions[7]"
> everywhere we can pass a revision. I think it makes the doc heavier
> than it should be, especially here where the command is a plumbing one
> for now and users should be quite experienced with Git already.
I agree that plumbing commands assume that users are more
experienced with Git already, so SGTM.
>> I ask because I'm interested in informing
>> the readers of our docs about any potential pitfalls from abusing this
>> command by mistake.
>
> I appreciate your desire to give high quality docs to our users, but I
> don't think it's a big pitfall and I think that this command is still
> very much "in the works" and is also designed for experienced users
> for now, so I am not sure it's the right time to spend too much time
> on this.
Also sounds reasonable to me. Thank you for considering my suggestions,
much appreciated!
^ permalink raw reply
* Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines
From: Rubén Justo @ 2023-10-19 19:20 UTC (permalink / raw)
To: Isoken June Ibizugbe, git; +Cc: christian.couder, gitster
In-Reply-To: <20231019084052.567922-1-isokenjune@gmail.com>
On 19-oct-2023 09:40:51, Isoken June Ibizugbe wrote:
> As per the CodingGuidelines document, it is recommended that a single-line
> message provided to error messages such as die(), error() and warning(),
This is confusing; some multi-line messages are fixed in this series.
> should start with a lowercase letter and should not end with a period.
> Also this patch fixes the tests broken by the changes.
Well done, describing why the tests are touched.
>
> Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> ---
> builtin/branch.c | 66 +++++++++++++++++++--------------------
> t/t2407-worktree-heads.sh | 2 +-
> t/t3200-branch.sh | 16 +++++-----
> t/t3202-show-branch.sh | 10 +++---
> 4 files changed, 47 insertions(+), 47 deletions(-)
Looking good.
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 2ec190b14a..e7ee9bd0f1 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -173,11 +173,11 @@ static int branch_merged(int kind, const char *name,
> (head_rev ? repo_in_merge_bases(the_repository, rev, head_rev) : 0) != merged) {
> if (merged)
> warning(_("deleting branch '%s' that has been merged to\n"
> - " '%s', but not yet merged to HEAD."),
> + " '%s', but not yet merged to HEAD"),
> name, reference_name);
> else
> warning(_("not deleting branch '%s' that is not yet merged to\n"
> - " '%s', even though it is merged to HEAD."),
> + " '%s', even though it is merged to HEAD"),
> name, reference_name);
> }
> free(reference_name_to_free);
> @@ -190,13 +190,13 @@ static int check_branch_commit(const char *branchname, const char *refname,
> {
> struct commit *rev = lookup_commit_reference(the_repository, oid);
> if (!force && !rev) {
> - error(_("Couldn't look up commit object for '%s'"), refname);
> + error(_("couldn't look up commit object for '%s'"), refname);
> return -1;
> }
> if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
> - error(_("The branch '%s' is not fully merged.\n"
> + error(_("the branch '%s' is not fully merged.\n"
> "If you are sure you want to delete it, "
> - "run 'git branch -D %s'."), branchname, branchname);
> + "run 'git branch -D %s'"), branchname, branchname);
> return -1;
> }
> return 0;
> @@ -207,7 +207,7 @@ static void delete_branch_config(const char *branchname)
> struct strbuf buf = STRBUF_INIT;
> strbuf_addf(&buf, "branch.%s", branchname);
> if (git_config_rename_section(buf.buf, NULL) < 0)
> - warning(_("Update of config-file failed"));
> + warning(_("update of config-file failed"));
> strbuf_release(&buf);
> }
>
> @@ -260,7 +260,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> if (kinds == FILTER_REFS_BRANCHES) {
> const char *path;
> if ((path = branch_checked_out(name))) {
> - error(_("Cannot delete branch '%s' "
> + error(_("cannot delete branch '%s' "
> "used by worktree at '%s'"),
> bname.buf, path);
> ret = 1;
> @@ -275,7 +275,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> &oid, &flags);
> if (!target) {
> if (remote_branch) {
> - error(_("remote-tracking branch '%s' not found."), bname.buf);
> + error(_("remote-tracking branch '%s' not found"), bname.buf);
> } else {
> char *virtual_name = mkpathdup(fmt_remotes, bname.buf);
> char *virtual_target = resolve_refdup(virtual_name,
> @@ -290,7 +290,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
> "Did you forget --remote?"),
> bname.buf);
> else
> - error(_("branch '%s' not found."), bname.buf);
> + error(_("branch '%s' not found"), bname.buf);
> FREE_AND_NULL(virtual_target);
> }
> ret = 1;
> @@ -518,11 +518,11 @@ static void reject_rebase_or_bisect_branch(struct worktree **worktrees,
> continue;
>
> if (is_worktree_being_rebased(wt, target))
> - die(_("Branch %s is being rebased at %s"),
> + die(_("branch %s is being rebased at %s"),
> target, wt->path);
>
> if (is_worktree_being_bisected(wt, target))
> - die(_("Branch %s is being bisected at %s"),
> + die(_("branch %s is being bisected at %s"),
> target, wt->path);
> }
> }
> @@ -578,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> if (ref_exists(oldref.buf))
> recovery = 1;
> else
> - die(_("Invalid branch name: '%s'"), oldname);
> + die(_("invalid branch name: '%s'"), oldname);
> }
>
> for (int i = 0; worktrees[i]; i++) {
> @@ -594,9 +594,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>
> if ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {
> if (oldref_usage & IS_HEAD)
> - die(_("No commit on branch '%s' yet."), oldname);
> + die(_("no commit on branch '%s' yet"), oldname);
> else
> - die(_("No branch named '%s'."), oldname);
> + die(_("no branch named '%s'"), oldname);
> }
>
> /*
> @@ -624,32 +624,32 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>
> if (!copy && !(oldref_usage & IS_ORPHAN) &&
> rename_ref(oldref.buf, newref.buf, logmsg.buf))
> - die(_("Branch rename failed"));
> + die(_("branch rename failed"));
> if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
> - die(_("Branch copy failed"));
> + die(_("branch copy failed"));
>
> if (recovery) {
> if (copy)
> - warning(_("Created a copy of a misnamed branch '%s'"),
> + warning(_("created a copy of a misnamed branch '%s'"),
> interpreted_oldname);
> else
> - warning(_("Renamed a misnamed branch '%s' away"),
> + warning(_("renamed a misnamed branch '%s' away"),
> interpreted_oldname);
> }
>
> if (!copy && (oldref_usage & IS_HEAD) &&
> replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
> logmsg.buf))
> - die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
> + die(_("branch renamed to %s, but HEAD is not updated"), newname);
>
> strbuf_release(&logmsg);
>
> strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> - die(_("Branch is renamed, but update of config-file failed"));
> + die(_("branch is renamed, but update of config-file failed"));
> if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> - die(_("Branch is copied, but update of config-file failed"));
> + die(_("branch is copied, but update of config-file failed"));
> strbuf_release(&oldref);
> strbuf_release(&newref);
> strbuf_release(&oldsection);
> @@ -773,7 +773,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>
> head = resolve_refdup("HEAD", 0, &head_oid, NULL);
> if (!head)
> - die(_("Failed to resolve HEAD as a valid ref."));
> + die(_("failed to resolve HEAD as a valid ref"));
> if (!strcmp(head, "HEAD"))
> filter.detached = 1;
> else if (!skip_prefix(head, "refs/heads/", &head))
> @@ -866,7 +866,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>
> if (!argc) {
> if (filter.detached)
> - die(_("Cannot give description to detached HEAD"));
> + die(_("cannot give description to detached HEAD"));
> branch_name = head;
> } else if (argc == 1) {
> strbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);
> @@ -878,8 +878,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
> if (!ref_exists(branch_ref.buf))
> error((!argc || branch_checked_out(branch_ref.buf))
> - ? _("No commit on branch '%s' yet.")
> - : _("No branch named '%s'."),
> + ? _("no commit on branch '%s' yet")
> + : _("no branch named '%s'"),
> branch_name);
> else if (!edit_branch_description(branch_name))
> ret = 0; /* happy */
> @@ -892,8 +892,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> if (!argc)
> die(_("branch name required"));
> else if ((argc == 1) && filter.detached)
> - die(copy? _("cannot copy the current branch while not on any.")
> - : _("cannot rename the current branch while not on any."));
> + die(copy? _("cannot copy the current branch while not on any")
> + : _("cannot rename the current branch while not on any"));
> else if (argc == 1)
> copy_or_rename_branch(head, argv[0], copy, copy + rename > 1);
> else if (argc == 2)
> @@ -916,14 +916,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> if (!branch) {
> if (!argc || !strcmp(argv[0], "HEAD"))
> die(_("could not set upstream of HEAD to %s when "
> - "it does not point to any branch."),
> + "it does not point to any branch"),
> new_upstream);
> die(_("no such branch '%s'"), argv[0]);
> }
>
> if (!ref_exists(branch->refname)) {
> if (!argc || branch_checked_out(branch->refname))
> - die(_("No commit on branch '%s' yet."), branch->name);
> + die(_("no commit on branch '%s' yet"), branch->name);
> die(_("branch '%s' does not exist"), branch->name);
> }
>
> @@ -946,12 +946,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> if (!branch) {
> if (!argc || !strcmp(argv[0], "HEAD"))
> die(_("could not unset upstream of HEAD when "
> - "it does not point to any branch."));
> + "it does not point to any branch"));
> die(_("no such branch '%s'"), argv[0]);
> }
>
> if (!branch_has_merge_config(branch))
> - die(_("Branch '%s' has no upstream information"), branch->name);
> + die(_("branch '%s' has no upstream information"), branch->name);
>
> strbuf_reset(&buf);
> strbuf_addf(&buf, "branch.%s.remote", branch->name);
> @@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> const char *start_name = argc == 2 ? argv[1] : head;
>
> if (filter.kind != FILTER_REFS_BRANCHES)
> - die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
> + die(_("the -a, and -r, options to 'git branch' do not take a branch name.\n"
OK.
> "Did you mean to use: -a|-r --list <pattern>?"));
>
> if (track == BRANCH_TRACK_OVERRIDE)
> - die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
> + die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead"));
>
> if (recurse_submodules) {
> create_branches_recursively(the_repository, branch_name,
> diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
> index 469443d8ae..f6835c91dc 100755
> --- a/t/t2407-worktree-heads.sh
> +++ b/t/t2407-worktree-heads.sh
> @@ -45,7 +45,7 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '
> grep "cannot force update the branch" err &&
>
> test_must_fail git branch -D wt-$i 2>err &&
> - grep "Cannot delete branch" err || return 1
> + grep "cannot delete branch" err || return 1
> done
> '
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 080e4f24a6..3182abde27 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -291,10 +291,10 @@ test_expect_success 'git branch -M topic topic should work when main is checked
> test_expect_success 'git branch -M and -C fail on detached HEAD' '
> git checkout HEAD^{} &&
> test_when_finished git checkout - &&
> - echo "fatal: cannot rename the current branch while not on any." >expect &&
> + echo "fatal: cannot rename the current branch while not on any" >expect &&
> test_must_fail git branch -M must-fail 2>err &&
> test_cmp expect err &&
> - echo "fatal: cannot copy the current branch while not on any." >expect &&
> + echo "fatal: cannot copy the current branch while not on any" >expect &&
> test_must_fail git branch -C must-fail 2>err &&
> test_cmp expect err
> '
> @@ -943,7 +943,7 @@ test_expect_success 'deleting currently checked out branch fails' '
> git worktree add -b my7 my7 &&
> test_must_fail git -C my7 branch -d my7 &&
> test_must_fail git branch -d my7 2>actual &&
> - grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
> + grep "^error: cannot delete branch .my7. used by worktree at " actual &&
> rm -r my7 &&
> git worktree prune
> '
> @@ -954,7 +954,7 @@ test_expect_success 'deleting in-use branch fails' '
> git -C my7 bisect start HEAD HEAD~2 &&
> test_must_fail git -C my7 branch -d my7 &&
> test_must_fail git branch -d my7 2>actual &&
> - grep "^error: Cannot delete branch .my7. used by worktree at " actual &&
> + grep "^error: cannot delete branch .my7. used by worktree at " actual &&
> rm -r my7 &&
> git worktree prune
> '
> @@ -1024,7 +1024,7 @@ test_expect_success '--set-upstream-to fails on multiple branches' '
> test_expect_success '--set-upstream-to fails on detached HEAD' '
> git checkout HEAD^{} &&
> test_when_finished git checkout - &&
> - echo "fatal: could not set upstream of HEAD to main when it does not point to any branch." >expect &&
> + echo "fatal: could not set upstream of HEAD to main when it does not point to any branch" >expect &&
> test_must_fail git branch --set-upstream-to main 2>err &&
> test_cmp expect err
> '
> @@ -1072,7 +1072,7 @@ test_expect_success 'use --set-upstream-to modify a particular branch' '
> '
>
> test_expect_success '--unset-upstream should fail if given a non-existent branch' '
> - echo "fatal: Branch '"'"'i-dont-exist'"'"' has no upstream information" >expect &&
> + echo "fatal: branch '"'"'i-dont-exist'"'"' has no upstream information" >expect &&
> test_must_fail git branch --unset-upstream i-dont-exist 2>err &&
> test_cmp expect err
> '
> @@ -1094,7 +1094,7 @@ test_expect_success 'test --unset-upstream on HEAD' '
> test_must_fail git config branch.main.remote &&
> test_must_fail git config branch.main.merge &&
> # fail for a branch without upstream set
> - echo "fatal: Branch '"'"'main'"'"' has no upstream information" >expect &&
> + echo "fatal: branch '"'"'main'"'"' has no upstream information" >expect &&
> test_must_fail git branch --unset-upstream 2>err &&
> test_cmp expect err
> '
> @@ -1108,7 +1108,7 @@ test_expect_success '--unset-upstream should fail on multiple branches' '
> test_expect_success '--unset-upstream should fail on detached HEAD' '
> git checkout HEAD^{} &&
> test_when_finished git checkout - &&
> - echo "fatal: could not unset upstream of HEAD when it does not point to any branch." >expect &&
> + echo "fatal: could not unset upstream of HEAD when it does not point to any branch" >expect &&
> test_must_fail git branch --unset-upstream 2>err &&
> test_cmp expect err
> '
> diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
> index b17f388f56..2cdb834b37 100755
> --- a/t/t3202-show-branch.sh
> +++ b/t/t3202-show-branch.sh
> @@ -10,7 +10,7 @@ GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
> test_expect_success 'error descriptions on empty repository' '
> current=$(git branch --show-current) &&
> cat >expect <<-EOF &&
> - error: No commit on branch '\''$current'\'' yet.
> + error: no commit on branch '\''$current'\'' yet
> EOF
> test_must_fail git branch --edit-description 2>actual &&
> test_cmp expect actual &&
> @@ -21,7 +21,7 @@ test_expect_success 'error descriptions on empty repository' '
> test_expect_success 'fatal descriptions on empty repository' '
> current=$(git branch --show-current) &&
> cat >expect <<-EOF &&
> - fatal: No commit on branch '\''$current'\'' yet.
> + fatal: no commit on branch '\''$current'\'' yet
> EOF
> test_must_fail git branch --set-upstream-to=non-existent 2>actual &&
> test_cmp expect actual &&
> @@ -224,7 +224,7 @@ done
>
> test_expect_success 'error descriptions on non-existent branch' '
> cat >expect <<-EOF &&
> - error: No branch named '\''non-existent'\'.'
> + error: no branch named '\''non-existent'\''
> EOF
> test_must_fail git branch --edit-description non-existent 2>actual &&
> test_cmp expect actual
> @@ -238,7 +238,7 @@ test_expect_success 'fatal descriptions on non-existent branch' '
> test_cmp expect actual &&
>
> cat >expect <<-EOF &&
> - fatal: No branch named '\''non-existent'\''.
> + fatal: no branch named '\''non-existent'\''
> EOF
> test_must_fail git branch -c non-existent new-branch 2>actual &&
> test_cmp expect actual &&
> @@ -253,7 +253,7 @@ test_expect_success 'error descriptions on orphan branch' '
> test_branch_op_in_wt() {
> test_orphan_error() {
> test_must_fail git $* 2>actual &&
> - test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
> + test_i18ngrep "no commit on branch .orphan-branch. yet$" actual
> } &&
> test_orphan_error -C wt branch $1 $2 && # implicit branch
> test_orphan_error -C wt branch $1 orphan-branch $2 && # explicit branch
> --
> 2.42.0.346.g24618a8a3e.dirty
>
All error messages in builtin/branch.c are fixed and I've locally run
the tests with your changes, and all passes.
So, aside from the confusing message, this iteration looks good to me.
Thank you.
^ permalink raw reply
* Re: [PATCH 0/3] CMake unit test fixups
From: Junio C Hamano @ 2023-10-19 19:19 UTC (permalink / raw)
To: Phillip Wood
Cc: calvinwan, git, johannes.schindelin, linusa, rsbecker, steadmon
In-Reply-To: <20231019152726.14624-1-phillip.wood123@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> I need these fixups to get our CI to successfully build an run the
> unit tests using CMake & MSVC. They are all adjusting paths now that
> the unit test programs are built in t/unit-tests/bin
Thanks! Very much appreciated.
^ permalink raw reply
* Re: Is there any interest in localizing term delimiters in git messages?
From: Junio C Hamano @ 2023-10-19 19:18 UTC (permalink / raw)
To: Jeff Hostetler
Cc: Jiang Xin, Torsten Bögershausen, Jeff Hostetler,
Alexander Shopov, Git List, jmas, alexhenrie24, ralf.thielow,
matthias.ruester, phillip.szelat, vyruss, christopher.diaz.riv,
jn.avila, flashcode, bagasdotme,
Ævar Arnfjörð Bjarmason, alessandro.menti,
elongbug, cwryu, uneedsihyeon, arek_koz, dacs.git,
insolor@gmail.com, peter, bitigchi, ark, kate,
vnwildman@gmail.com, pclouds, dyroneteng@gmail.com,
oldsharp@gmail.com, lilydjwg@gmail.com, me, pan93412@gmail.com,
franklin@goodhorse.idv.tw
In-Reply-To: <573f1142-d1de-b379-2f8b-07396c1249ec@jeffhostetler.com>
Jeff Hostetler <git@jeffhostetler.com> writes:
> Yeah, I think it should be an untranslated trace2 message rather
> than an error. You're right, the user cannot do anything with
> that information -- and by emitting a "trivial" result, we fall
> back to the normal behavior and cause the client to a regular
> scan. So there is no reason to scare the user.
Thanks for a quick response. Note that this was something we
discovered while talking about i18n and no immediate action is
required---it is not like we saw a report that tells us that end
users are actively getting confused.
THanks.
^ permalink raw reply
* Re: [PATCH] diagnose: require repository
From: Victoria Dye @ 2023-10-19 18:16 UTC (permalink / raw)
To: Martin Ågren, Junio C Hamano; +Cc: ks1322 ks1322, git
In-Reply-To: <CAN0heSqmZ7QXJbet2Tp=YYCjBLToOHtNy+n=zcf29XYaukYN0w@mail.gmail.com>
Martin Ågren wrote:
>>> behavior, it seems more helpful to bail out clearly and early with a
>>> succinct error message.
>>
>> Without having thought things through, offhand I agree with your "no
>> repository? there is nothing worth tarring up then" assessment.
>>
>> Because "git bugreport --diag" unconditionally spawns "git
>> diagnose", the former may also want to be extra careful, perhaps
>> like the attached patch.
>
> Good point. TBH, I had no idea about `git bugreport --diagnose`.
>
>> + if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) {
>> + warning(_("no repository--diagnostic output disabled"));
>> + diagnose = DIAGNOSE_NONE;
>> + }
>> +
>
> When the user explicitly provides that option, it seems unfortunate to
> me to drop it. Yes, we'd warn, but `git bugreport` then pops a text
> editor, so you would only see the warning after finishing up the report.
> (Maybe. By the time you quit your editor, you might not consider
> checking the terminal for warnings and such.)
>
> So I'm inclined to instead just die if we see the option outside a repo.
> If `diagnose` the command fundamentally requires a repo (as with my
> patch) it seems surprising to me to not have `--diagnose` the option
> behave the same.
I agree - it was an oversight on my part to not firmly require the existence
of a repository with 'git diagnose', and the same applies to 'bugreport
--diagnose'.
For reference, there is one other usage of 'git diagnose' (in 'scalar
diagnose'). However, it's already guarded by 'setup_git_directory()' so it
shouldn't need to be updated.
>
> Martin
^ permalink raw reply
* Re: [PATCH] diagnose: require repository
From: Junio C Hamano @ 2023-10-19 18:09 UTC (permalink / raw)
To: Martin Ågren; +Cc: ks1322 ks1322, git, Victoria Dye
In-Reply-To: <CAN0heSqmZ7QXJbet2Tp=YYCjBLToOHtNy+n=zcf29XYaukYN0w@mail.gmail.com>
Martin Ågren <martin.agren@gmail.com> writes:
> Correcting myself: The zip archive would actually contain
> `diagnostics.log` with some general info about the machine and Git
> build.
So it could contain some useful information without a specific
repository, perhaps.
> Good point. TBH, I had no idea about `git bugreport --diagnose`.
You are not alone ;-) I didn't, either. Before responding to your
patch, that is.
>> + if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) {
>> + warning(_("no repository--diagnostic output disabled"));
>> + diagnose = DIAGNOSE_NONE;
>> + }
>> +
>
> When the user explicitly provides that option, it seems unfortunate to
> me to drop it. Yes, we'd warn, but `git bugreport` then pops a text
> editor, so you would only see the warning after finishing up the report.
> (Maybe. By the time you quit your editor, you might not consider
> checking the terminal for warnings and such.)
>
> So I'm inclined to instead just die if we see the option outside a repo.
> If `diagnose` the command fundamentally requires a repo (as with my
> patch) it seems surprising to me to not have `--diagnose` the option
> behave the same.
I have no strong opinion. Victoria is on Cc: already, whose name
appears a lot more often than mine in the shortlog for "diagnose"
stuff, so I'll defer to her area expertise.
Thanks.
^ permalink raw reply
* Re: Is there any interest in localizing term delimiters in git messages?
From: Jeff Hostetler @ 2023-10-19 18:07 UTC (permalink / raw)
To: Junio C Hamano, Jiang Xin, Torsten Bögershausen,
Jeff Hostetler
Cc: Alexander Shopov, Git List, jmas, alexhenrie24, ralf.thielow,
matthias.ruester, phillip.szelat, vyruss, christopher.diaz.riv,
jn.avila, flashcode, bagasdotme,
Ævar Arnfjörð Bjarmason, alessandro.menti,
elongbug, cwryu, uneedsihyeon, arek_koz, dacs.git,
insolor@gmail.com, peter, bitigchi, ark, kate,
vnwildman@gmail.com, pclouds, dyroneteng@gmail.com,
oldsharp@gmail.com, lilydjwg@gmail.com, me, pan93412@gmail.com,
franklin@goodhorse.idv.tw
In-Reply-To: <xmqqcyxaxzxw.fsf@gitster.g>
On 10/19/23 1:52 PM, Junio C Hamano wrote:
> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> I tried to find similar patterns in `po/bg.po` using:
>>
>> $ git grep -h -B5 '([a-zA-Z_\.]*_[a-zA-Z_\.]\+)' po/bg.po
>>
>> And find other translated variable names in Bulgarian as follows:
>> ...
>> I suppose it would be better to keep those variable names
>> unchanged.
>
> To me, all of them refer to names given to variables, functions, and
> mechanisms used internally as implementation details, and they are
> meant to help developers diagnose when end-users hit these errors.
>
> I agree with you that translating these would be counter-productive
> for that purpose.
>
> Having said that, I have to wonder if in an ideal world these should
> be written in terms that are more end-user facing.
>
>> * cookie_result in builtin/fsmonitor--daemon.c:
>>
>> error(_("fsmonitor: cookie_result '%d' != SEEN"),
>
> [jch: cc'ed JeffH for area expertise]
>
> For example, what does it mean to the end user when the
> cookie->result we retrieve is different from FCIR_SEEN? We lost
> sync with the fsmonitor daemon backend and to avoid yielding
> incorrect data we will be giving the "trivial" response only? It is
> not obvious from the code and b05880d3 (fsmonitor--daemon: use a
> cookie file to sync with file system, 2022-03-25) that added it why
> the end-user might even want to be shown this message [*]. I wonder
> if this should be an untranslated trace2_* message that are meant
> for debugging.
>
> Side note: and isn't the significance of the event
> "warning", not "error"? As far as the end-user is
> concerned, after emitting this message
>
> Also some of them might better be a BUG(), instead of die(_()).
...
Yeah, I think it should be an untranslated trace2 message rather
than an error. You're right, the user cannot do anything with
that information -- and by emitting a "trivial" result, we fall
back to the normal behavior and cause the client to a regular
scan. So there is no reason to scare the user.
Jeff
^ permalink raw reply
* Re: [PATCH] doc: update list archive reference to use lore.kernel.org
From: Junio C Hamano @ 2023-10-19 18:01 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <199df43378febcc746c78029c5f90d04@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> You asked for this patch to be reviewed, so I did it. It looks
> perfectly fine to me.
>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
Thanks.
^ permalink raw reply
* Re: [PATCH v2] git-p4 shouldn't attempt to store symlinks in LFS
From: Junio C Hamano @ 2023-10-19 17:59 UTC (permalink / raw)
To: Matthew McClain; +Cc: git, sandals
In-Reply-To: <20231019002558.867830-1-mmcclain@noprivs.com>
Matthew McClain <mmcclain@noprivs.com> writes:
> git-p4.py would attempt to put a symlink in LFS if its file extension
> matched git-p4.largeFileExtensions.
>
> Git LFS doesn't store symlinks because smudge/clean filters don't handle
> symlinks. They never get passed to the filter process nor the
> smudge/clean filters, nor could that occur without a change to the
> protocol or command-line interface. Unless Git learned how to send them
> to the filters, Git LFS would have a hard time using them in any useful
> way.
>
> Git LFS's goal is to move large files out of the repository history, and
> symlinks are functionally limited to 4 KiB or a similar size on most
> systems.
Reads much better and easier to reason about.
Will queue and see if "git p4" stakeholders object for a few days;
if nothing happens, let's merge it down to 'next' and then to
'master' as usual.
Thanks.
> Signed-off-by: Matthew McClain <mmcclain@noprivs.com>
> ---
> git-p4.py | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index d26a980e5a..0eb3bb4c47 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1522,6 +1522,10 @@ def processContent(self, git_mode, relPath, contents):
> file is stored in the large file system and handles all necessary
> steps.
> """
> + # symlinks aren't processed by smudge/clean filters
> + if git_mode == "120000":
> + return (git_mode, contents)
> +
> if self.exceedsLargeFileThreshold(relPath, contents) or self.hasLargeFileExtension(relPath):
> contentTempFile = self.generateTempFile(contents)
> pointer_git_mode, contents, localLargeFile = self.generatePointer(contentTempFile)
^ permalink raw reply
* Re: Is there any interest in localizing term delimiters in git messages?
From: Junio C Hamano @ 2023-10-19 17:52 UTC (permalink / raw)
To: Jiang Xin, Torsten Bögershausen, Jeff Hostetler
Cc: Alexander Shopov, Git List, jmas, alexhenrie24, ralf.thielow,
matthias.ruester, phillip.szelat, vyruss, christopher.diaz.riv,
jn.avila, flashcode, bagasdotme,
Ævar Arnfjörð Bjarmason, alessandro.menti,
elongbug, cwryu, uneedsihyeon, arek_koz, dacs.git,
insolor@gmail.com, peter, bitigchi, ark, kate,
vnwildman@gmail.com, pclouds, dyroneteng@gmail.com,
oldsharp@gmail.com, lilydjwg@gmail.com, me, pan93412@gmail.com,
franklin@goodhorse.idv.tw
In-Reply-To: <CANYiYbEqTH975j9E0GTbSbexrw3MLhKwBCw7mibfnWbxZ+-_yw@mail.gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> I tried to find similar patterns in `po/bg.po` using:
>
> $ git grep -h -B5 '([a-zA-Z_\.]*_[a-zA-Z_\.]\+)' po/bg.po
>
> And find other translated variable names in Bulgarian as follows:
> ...
> I suppose it would be better to keep those variable names
> unchanged.
To me, all of them refer to names given to variables, functions, and
mechanisms used internally as implementation details, and they are
meant to help developers diagnose when end-users hit these errors.
I agree with you that translating these would be counter-productive
for that purpose.
Having said that, I have to wonder if in an ideal world these should
be written in terms that are more end-user facing.
> * cookie_result in builtin/fsmonitor--daemon.c:
>
> error(_("fsmonitor: cookie_result '%d' != SEEN"),
[jch: cc'ed JeffH for area expertise]
For example, what does it mean to the end user when the
cookie->result we retrieve is different from FCIR_SEEN? We lost
sync with the fsmonitor daemon backend and to avoid yielding
incorrect data we will be giving the "trivial" response only? It is
not obvious from the code and b05880d3 (fsmonitor--daemon: use a
cookie file to sync with file system, 2022-03-25) that added it why
the end-user might even want to be shown this message [*]. I wonder
if this should be an untranslated trace2_* message that are meant
for debugging.
Side note: and isn't the significance of the event
"warning", not "error"? As far as the end-user is
concerned, after emitting this message
Also some of them might better be a BUG(), instead of die(_()).
> * crlf_action in convert.c:
>
> warning(_("illegal crlf_action %d"), (int)crlf_action);
[jch: cc'ed Torsten for area expertise].
For example, can convert.c::output_eol() be called with an illegal
crlf_action that is not covered by the switch() statement due to
data error, not a programming error? From my quick scan, it looks
like that the error should never happen no matter what end-user
mistakes (e.g., misspelt attribute and configuration variable names
in their files) are fed to convert_attrs(), and can come only from a
bug in that function (e.g., long and convoluted if/else cascade fails
to assign any value to ca->crlf_action and leaves an undefined and
"illegal" value there).
Thanks.
^ permalink raw reply
* Re: [PATCH v3 05/10] bulk-checkin: extract abstract `bulk_checkin_source`
From: Junio C Hamano @ 2023-10-19 17:55 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King,
Patrick Steinhardt
In-Reply-To: <ZTFI++b51Cj+Sto9@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> I want to be cautious of going too far in this direction.
That's fine. Thanks.
^ permalink raw reply
* Re: [PATCH 00/11] t: reduce direct disk access to data structures
From: Junio C Hamano @ 2023-10-19 17:55 UTC (permalink / raw)
To: Han-Wen Nienhuys; +Cc: Patrick Steinhardt, git
In-Reply-To: <CAFQ2z_Om724+o+EG1FAhC9VrvJECnQ5UA+Z04Rzycpi_mXvMHg@mail.gmail.com>
Han-Wen Nienhuys <hanwen@google.com> writes:
> I think it would be really great if there were separate unittests for
> the ref backend API. Some of the reftable work was needlessly
> difficult because the contract of the API was underspecified. The API
> is well compartmentalized in refs-internal.h, and a lot of the API
> behavior can be tested as a black box, eg.
>
> * setup symref HEAD pointing to R1
> * setup transaction updating ref R1 from C1 to C2
> * commit transaction, check that it succeeds
> * read ref R1, check if it is C2
> * read reflog for R1, see that it has a C1 => C2 update
> * read reflog for HEAD, see that it has a C1 => C2 update
>
> Tests for the loose/packed backend could directly mess with the
> on-disk files to test failure scenarios.
>
> With unittests like that, the tests can zoom in on the functionality
> of the ref backend, and provide more convenient coverage for
> dynamic/static analysis.
Yeah, I agree that something like that would really be great.
^ permalink raw reply
* [PATCH v4 7/7] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-10-19 17:29 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697736516.git.me@ttaylorr.com>
When using merge-tree often within a repository[^1], it is possible to
generate a relatively large number of loose objects, which can result in
degraded performance, and inode exhaustion in extreme cases.
Building on the functionality introduced in previous commits, the
bulk-checkin machinery now has support to write arbitrary blob and tree
objects which are small enough to be held in-core. We can use this to
write any blob/tree objects generated by ORT into a separate pack
instead of writing them out individually as loose.
This functionality is gated behind a new `--write-pack` option to
`merge-tree` that works with the (non-deprecated) `--write-tree` mode.
The implementation is relatively straightforward. There are two spots
within the ORT mechanism where we call `write_object_file()`, one for
content differences within blobs, and another to assemble any new trees
necessary to construct the merge. In each of those locations,
conditionally replace calls to `write_object_file()` with
`index_blob_bulk_checkin_incore()` or `index_tree_bulk_checkin_incore()`
depending on which kind of object we are writing.
The only remaining task is to begin and end the transaction necessary to
initialize the bulk-checkin machinery, and move any new pack(s) it
created into the main object store.
[^1]: Such is the case at GitHub, where we run presumptive "test merges"
on open pull requests to see whether or not we can light up the merge
button green depending on whether or not the presumptive merge was
conflicted.
This is done in response to a number of user-initiated events,
including viewing an open pull request whose last test merge is stale
with respect to the current base and tip of the pull request. As a
result, merge-tree can be run very frequently on large, active
repositories.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Documentation/git-merge-tree.txt | 4 ++
builtin/merge-tree.c | 5 ++
merge-ort.c | 42 +++++++++++----
merge-recursive.h | 1 +
t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++++++++++++++++
5 files changed, 136 insertions(+), 9 deletions(-)
diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index ffc4fbf7e8..9d37609ef1 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -69,6 +69,10 @@ OPTIONS
specify a merge-base for the merge, and specifying multiple bases is
currently not supported. This option is incompatible with `--stdin`.
+--write-pack::
+ Write any new objects into a separate packfile instead of as
+ individual loose objects.
+
[[OUTPUT]]
OUTPUT
------
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 0de42aecf4..672ebd4c54 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -18,6 +18,7 @@
#include "quote.h"
#include "tree.h"
#include "config.h"
+#include "bulk-checkin.h"
static int line_termination = '\n';
@@ -414,6 +415,7 @@ struct merge_tree_options {
int show_messages;
int name_only;
int use_stdin;
+ int write_pack;
};
static int real_merge(struct merge_tree_options *o,
@@ -440,6 +442,7 @@ static int real_merge(struct merge_tree_options *o,
init_merge_options(&opt, the_repository);
opt.show_rename_progress = 0;
+ opt.write_pack = o->write_pack;
opt.branch1 = branch1;
opt.branch2 = branch2;
@@ -548,6 +551,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
&merge_base,
N_("commit"),
N_("specify a merge-base for the merge")),
+ OPT_BOOL(0, "write-pack", &o.write_pack,
+ N_("write new objects to a pack instead of as loose")),
OPT_END()
};
diff --git a/merge-ort.c b/merge-ort.c
index 3653725661..523577d71e 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -48,6 +48,7 @@
#include "tree.h"
#include "unpack-trees.h"
#include "xdiff-interface.h"
+#include "bulk-checkin.h"
/*
* We have many arrays of size 3. Whenever we have such an array, the
@@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,
if ((merge_status < 0) || !result_buf.ptr)
ret = error(_("failed to execute internal merge"));
- if (!ret &&
- write_object_file(result_buf.ptr, result_buf.size,
- OBJ_BLOB, &result->oid))
- ret = error(_("unable to add %s to database"), path);
+ if (!ret) {
+ ret = opt->write_pack
+ ? index_blob_bulk_checkin_incore(&result->oid,
+ result_buf.ptr,
+ result_buf.size,
+ path, 1)
+ : write_object_file(result_buf.ptr,
+ result_buf.size,
+ OBJ_BLOB, &result->oid);
+ if (ret)
+ ret = error(_("unable to add %s to database"),
+ path);
+ }
free(result_buf.ptr);
if (ret)
@@ -3597,7 +3607,8 @@ static int tree_entry_order(const void *a_, const void *b_)
b->string, strlen(b->string), bmi->result.mode);
}
-static int write_tree(struct object_id *result_oid,
+static int write_tree(struct merge_options *opt,
+ struct object_id *result_oid,
struct string_list *versions,
unsigned int offset,
size_t hash_size)
@@ -3631,8 +3642,14 @@ static int write_tree(struct object_id *result_oid,
}
/* Write this object file out, and record in result_oid */
- if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))
+ ret = opt->write_pack
+ ? index_tree_bulk_checkin_incore(result_oid,
+ buf.buf, buf.len, "", 1)
+ : write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);
+
+ if (ret)
ret = -1;
+
strbuf_release(&buf);
return ret;
}
@@ -3797,8 +3814,8 @@ static int write_completed_directory(struct merge_options *opt,
*/
dir_info->is_null = 0;
dir_info->result.mode = S_IFDIR;
- if (write_tree(&dir_info->result.oid, &info->versions, offset,
- opt->repo->hash_algo->rawsz) < 0)
+ if (write_tree(opt, &dir_info->result.oid, &info->versions,
+ offset, opt->repo->hash_algo->rawsz) < 0)
ret = -1;
}
@@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,
fflush(stdout);
BUG("dir_metadata accounting completely off; shouldn't happen");
}
- if (write_tree(result_oid, &dir_metadata.versions, 0,
+ if (write_tree(opt, result_oid, &dir_metadata.versions, 0,
opt->repo->hash_algo->rawsz) < 0)
ret = -1;
+
+ if (opt->write_pack)
+ end_odb_transaction();
+
cleanup:
string_list_clear(&plist, 0);
string_list_clear(&dir_metadata.versions, 0);
@@ -4878,6 +4899,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
*/
strmap_init(&opt->priv->conflicts);
+ if (opt->write_pack)
+ begin_odb_transaction();
+
trace2_region_leave("merge", "allocate/init", opt->repo);
}
diff --git a/merge-recursive.h b/merge-recursive.h
index b88000e3c2..156e160876 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -48,6 +48,7 @@ struct merge_options {
unsigned renormalize : 1;
unsigned record_conflict_msgs_as_headers : 1;
const char *msg_header_prefix;
+ unsigned write_pack : 1;
/* internal fields used by the implementation */
struct merge_options_internal *priv;
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 250f721795..2d81ff4de5 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -922,4 +922,97 @@ test_expect_success 'check the input format when --stdin is passed' '
test_cmp expect actual
'
+packdir=".git/objects/pack"
+
+test_expect_success 'merge-tree can pack its result with --write-pack' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+
+ # base has lines [3, 4, 5]
+ # - side adds to the beginning, resulting in [1, 2, 3, 4, 5]
+ # - other adds to the end, resulting in [3, 4, 5, 6, 7]
+ #
+ # merging the two should result in a new blob object containing
+ # [1, 2, 3, 4, 5, 6, 7], along with a new tree.
+ test_commit -C repo base file "$(test_seq 3 5)" &&
+ git -C repo branch -M main &&
+ git -C repo checkout -b side main &&
+ test_commit -C repo side file "$(test_seq 1 5)" &&
+ git -C repo checkout -b other main &&
+ test_commit -C repo other file "$(test_seq 3 7)" &&
+
+ find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
+ tree="$(git -C repo merge-tree --write-pack \
+ refs/tags/side refs/tags/other)" &&
+ blob="$(git -C repo rev-parse $tree:file)" &&
+ find repo/$packdir -type f -name "pack-*.idx" >packs.after &&
+
+ test_must_be_empty packs.before &&
+ test_line_count = 1 packs.after &&
+
+ git show-index <$(cat packs.after) >objects &&
+ test_line_count = 2 objects &&
+ grep "^[1-9][0-9]* $tree" objects &&
+ grep "^[1-9][0-9]* $blob" objects
+'
+
+test_expect_success 'merge-tree can write multiple packs with --write-pack' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+
+ git config pack.packSizeLimit 512 &&
+
+ test_seq 512 >f &&
+
+ # "f" contains roughly ~2,000 bytes.
+ #
+ # Each side ("foo" and "bar") adds a small amount of data at the
+ # beginning and end of "base", respectively.
+ git add f &&
+ test_tick &&
+ git commit -m base &&
+ git branch -M main &&
+
+ git checkout -b foo main &&
+ {
+ echo foo && cat f
+ } >f.tmp &&
+ mv f.tmp f &&
+ git add f &&
+ test_tick &&
+ git commit -m foo &&
+
+ git checkout -b bar main &&
+ echo bar >>f &&
+ git add f &&
+ test_tick &&
+ git commit -m bar &&
+
+ find $packdir -type f -name "pack-*.idx" >packs.before &&
+ # Merging either side should result in a new object which is
+ # larger than 1M, thus the result should be split into two
+ # separate packs.
+ tree="$(git merge-tree --write-pack \
+ refs/heads/foo refs/heads/bar)" &&
+ blob="$(git rev-parse $tree:f)" &&
+ find $packdir -type f -name "pack-*.idx" >packs.after &&
+
+ test_must_be_empty packs.before &&
+ test_line_count = 2 packs.after &&
+ for idx in $(cat packs.after)
+ do
+ git show-index <$idx || return 1
+ done >objects &&
+
+ # The resulting set of packs should contain one copy of both
+ # objects, each in a separate pack.
+ test_line_count = 2 objects &&
+ grep "^[1-9][0-9]* $tree" objects &&
+ grep "^[1-9][0-9]* $blob" objects
+
+ )
+'
+
test_done
--
2.42.0.405.g86fe3250c2
^ permalink raw reply related
* [PATCH v4 6/7] bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-19 17:28 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697736516.git.me@ttaylorr.com>
The remaining missing piece in order to teach the `merge-tree` builtin
how to write the contents of a merge into a pack is a function to index
tree objects into a bulk-checkin pack.
This patch implements that missing piece, which is a thin wrapper around
all of the functionality introduced in previous commits.
If and when Git gains support for a "compatibility" hash algorithm, the
changes to support that here will be minimal. The bulk-checkin machinery
will need to convert the incoming tree to compute its length under the
compatibility hash, necessary to reconstruct its header. With that
information (and the converted contents of the tree), the bulk-checkin
machinery will have enough to keep track of the converted object's hash
in order to update the compatibility mapping.
Within some thin wrapper around `deflate_obj_to_pack_incore()` (perhaps
`deflate_tree_to_pack_incore()`), the changes should be limited to
something like:
struct strbuf converted = STRBUF_INIT;
if (the_repository->compat_hash_algo) {
if (convert_object_file(&compat_obj,
the_repository->hash_algo,
the_repository->compat_hash_algo, ...) < 0)
die(...);
format_object_header_hash(the_repository->compat_hash_algo,
OBJ_TREE, size);
}
/* compute the converted tree's hash using the compat algorithm */
strbuf_release(&converted);
, assuming related changes throughout the rest of the bulk-checkin
machinery necessary to update the hash of the converted object, which
are likewise minimal in size.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 12 ++++++++++++
bulk-checkin.h | 4 ++++
2 files changed, 16 insertions(+)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 655a583b06..c1faf75f5f 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -460,6 +460,18 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,
return status;
}
+int index_tree_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
+ int status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,
+ buf, size, path, OBJ_TREE,
+ flags);
+ if (!odb_transaction_nesting)
+ flush_bulk_checkin_packfile(&bulk_checkin_packfile);
+ return status;
+}
+
void begin_odb_transaction(void)
{
odb_transaction_nesting += 1;
diff --git a/bulk-checkin.h b/bulk-checkin.h
index 1b91daeaee..89786b3954 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -17,6 +17,10 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,
const void *buf, size_t size,
const char *path, unsigned flags);
+int index_tree_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags);
+
/*
* Tell the object database to optimize for adding
* multiple objects. end_odb_transaction must be called
--
2.42.0.405.g86fe3250c2
^ permalink raw reply related
* [PATCH v4 5/7] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-19 17:28 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697736516.git.me@ttaylorr.com>
Now that we have factored out many of the common routines necessary to
index a new object into a pack created by the bulk-checkin machinery, we
can introduce a variant of `index_blob_bulk_checkin()` that acts on
blobs whose contents we can fit in memory.
This will be useful in a couple of more commits in order to provide the
`merge-tree` builtin with a mechanism to create a new pack containing
any objects it created during the merge, instead of storing those
objects individually as loose.
Similar to the existing `index_blob_bulk_checkin()` function, the
entrypoint delegates to `deflate_obj_to_pack_incore()`. That function in
turn delegates to deflate_obj_to_pack(), which is responsible for
formatting the pack header and then deflating the contents into the
pack.
Consistent with the rest of the bulk-checkin mechanism, there are no
direct tests here. In future commits when we expose this new
functionality via the `merge-tree` builtin, we will test it indirectly
there.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 29 +++++++++++++++++++++++++++++
bulk-checkin.h | 4 ++++
2 files changed, 33 insertions(+)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 60361b3e2e..655a583b06 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -368,6 +368,23 @@ static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,
return 0;
}
+static int deflate_obj_to_pack_incore(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ const void *buf, size_t size,
+ const char *path, enum object_type type,
+ unsigned flags)
+{
+ struct bulk_checkin_source source = {
+ .type = SOURCE_INCORE,
+ .buf = buf,
+ .size = size,
+ .read = 0,
+ .path = path,
+ };
+
+ return deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -431,6 +448,18 @@ int index_blob_bulk_checkin(struct object_id *oid,
return status;
}
+int index_blob_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
+ int status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,
+ buf, size, path, OBJ_BLOB,
+ flags);
+ if (!odb_transaction_nesting)
+ flush_bulk_checkin_packfile(&bulk_checkin_packfile);
+ return status;
+}
+
void begin_odb_transaction(void)
{
odb_transaction_nesting += 1;
diff --git a/bulk-checkin.h b/bulk-checkin.h
index aa7286a7b3..1b91daeaee 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -13,6 +13,10 @@ int index_blob_bulk_checkin(struct object_id *oid,
int fd, size_t size,
const char *path, unsigned flags);
+int index_blob_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags);
+
/*
* Tell the object database to optimize for adding
* multiple objects. end_odb_transaction must be called
--
2.42.0.405.g86fe3250c2
^ permalink raw reply related
* [PATCH v4 4/7] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
From: Taylor Blau @ 2023-10-19 17:28 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697736516.git.me@ttaylorr.com>
Continue to prepare for streaming an object's contents directly from
memory by teaching `bulk_checkin_source` how to perform reads and seeks
based on an address in memory.
Unlike file descriptors, which manage their own offset internally, we
have to keep track of how many bytes we've read out of the buffer, and
make sure we don't read past the end of the buffer.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 28bc8d5ab4..60361b3e2e 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -141,11 +141,15 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
}
struct bulk_checkin_source {
- enum { SOURCE_FILE } type;
+ enum { SOURCE_FILE, SOURCE_INCORE } type;
/* SOURCE_FILE fields */
int fd;
+ /* SOURCE_INCORE fields */
+ const void *buf;
+ size_t read;
+
/* common fields */
size_t size;
const char *path;
@@ -157,6 +161,11 @@ static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,
switch (source->type) {
case SOURCE_FILE:
return lseek(source->fd, offset, SEEK_SET);
+ case SOURCE_INCORE:
+ if (!(0 <= offset && offset < source->size))
+ return (off_t)-1;
+ source->read = offset;
+ return source->read;
default:
BUG("unknown bulk-checkin source: %d", source->type);
}
@@ -168,6 +177,13 @@ static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,
switch (source->type) {
case SOURCE_FILE:
return read_in_full(source->fd, buf, nr);
+ case SOURCE_INCORE:
+ assert(source->read <= source->size);
+ if (nr > source->size - source->read)
+ nr = source->size - source->read;
+ memcpy(buf, (unsigned char *)source->buf + source->read, nr);
+ source->read += nr;
+ return nr;
default:
BUG("unknown bulk-checkin source: %d", source->type);
}
--
2.42.0.405.g86fe3250c2
^ permalink raw reply related
* [PATCH v4 3/7] bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`
From: Taylor Blau @ 2023-10-19 17:28 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697736516.git.me@ttaylorr.com>
Prepare for a future change where we will want to use a routine very
similar to the existing `deflate_blob_to_pack()` but over arbitrary
sources (i.e. either open file-descriptors, or a location in memory).
Extract out a common "deflate_obj_to_pack()" routine that acts on a
bulk_checkin_source, instead of a (int, size_t) pair. Then rewrite
`deflate_blob_to_pack()` in terms of it.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 52 ++++++++++++++++++++++++++++++--------------------
1 file changed, 31 insertions(+), 21 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 7e6b52112e..28bc8d5ab4 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -285,30 +285,23 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,
die_errno("unable to write pack header");
}
-static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
- struct object_id *result_oid,
- int fd, size_t size,
- const char *path, unsigned flags)
+
+static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ struct bulk_checkin_source *source,
+ enum object_type type,
+ off_t seekback,
+ unsigned flags)
{
- off_t seekback, already_hashed_to;
+ off_t already_hashed_to = 0;
git_hash_ctx ctx;
unsigned char obuf[16384];
unsigned header_len;
struct hashfile_checkpoint checkpoint = {0};
struct pack_idx_entry *idx = NULL;
- struct bulk_checkin_source source = {
- .type = SOURCE_FILE,
- .fd = fd,
- .size = size,
- .path = path,
- };
- seekback = lseek(fd, 0, SEEK_CUR);
- if (seekback == (off_t) -1)
- return error("cannot find the current offset");
-
- header_len = format_object_header((char *)obuf, sizeof(obuf),
- OBJ_BLOB, size);
+ header_len = format_object_header((char *)obuf, sizeof(obuf), type,
+ source->size);
the_hash_algo->init_fn(&ctx);
the_hash_algo->update_fn(&ctx, obuf, header_len);
the_hash_algo->init_fn(&checkpoint.ctx);
@@ -317,8 +310,6 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
if ((flags & HASH_WRITE_OBJECT) != 0)
CALLOC_ARRAY(idx, 1);
- already_hashed_to = 0;
-
while (1) {
prepare_to_stream(state, flags);
if (idx) {
@@ -327,7 +318,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
crc32_begin(state->f);
}
if (!stream_obj_to_pack(state, &ctx, &already_hashed_to,
- &source, OBJ_BLOB, flags))
+ source, type, flags))
break;
/*
* Writing this object to the current pack will make
@@ -339,7 +330,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
hashfile_truncate(state->f, &checkpoint);
state->offset = checkpoint.offset;
flush_bulk_checkin_packfile(state);
- if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
+ if (bulk_checkin_source_seek_to(source, seekback) == (off_t)-1)
return error("cannot seek back");
}
the_hash_algo->final_oid_fn(result_oid, &ctx);
@@ -361,6 +352,25 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
return 0;
}
+static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ int fd, size_t size,
+ const char *path, unsigned flags)
+{
+ struct bulk_checkin_source source = {
+ .type = SOURCE_FILE,
+ .fd = fd,
+ .size = size,
+ .path = path,
+ };
+ off_t seekback = lseek(fd, 0, SEEK_CUR);
+ if (seekback == (off_t) -1)
+ return error("cannot find the current offset");
+
+ return deflate_obj_to_pack(state, result_oid, &source, OBJ_BLOB,
+ seekback, flags);
+}
+
void prepare_loose_object_bulk_checkin(void)
{
/*
--
2.42.0.405.g86fe3250c2
^ permalink raw reply related
* [PATCH v4 2/7] bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
From: Taylor Blau @ 2023-10-19 17:28 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697736516.git.me@ttaylorr.com>
The existing `stream_blob_to_pack()` function is named based on the fact
that it knows only how to stream blobs into a bulk-checkin pack.
But there is no longer anything in this function which prevents us from
writing objects of arbitrary types to the bulk-checkin pack. Prepare to
write OBJ_TREEs by removing this assumption, adding an `enum
object_type` parameter to this function's argument list, and renaming it
to `stream_obj_to_pack()` accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index c05d06e1e1..7e6b52112e 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -188,10 +188,10 @@ static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,
* status before calling us just in case we ask it to call us again
* with a new pack.
*/
-static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
- git_hash_ctx *ctx, off_t *already_hashed_to,
- struct bulk_checkin_source *source,
- unsigned flags)
+static int stream_obj_to_pack(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx, off_t *already_hashed_to,
+ struct bulk_checkin_source *source,
+ enum object_type type, unsigned flags)
{
git_zstream s;
unsigned char ibuf[16384];
@@ -204,8 +204,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
git_deflate_init(&s, pack_compression_level);
- hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,
- size);
+ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
s.next_out = obuf + hdrlen;
s.avail_out = sizeof(obuf) - hdrlen;
@@ -327,8 +326,8 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
idx->offset = state->offset;
crc32_begin(state->f);
}
- if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
- &source, flags))
+ if (!stream_obj_to_pack(state, &ctx, &already_hashed_to,
+ &source, OBJ_BLOB, flags))
break;
/*
* Writing this object to the current pack will make
--
2.42.0.405.g86fe3250c2
^ permalink raw reply related
* [PATCH v4 1/7] bulk-checkin: extract abstract `bulk_checkin_source`
From: Taylor Blau @ 2023-10-19 17:28 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <cover.1697736516.git.me@ttaylorr.com>
A future commit will want to implement a very similar routine as in
`stream_blob_to_pack()` with two notable changes:
- Instead of streaming just OBJ_BLOBs, this new function may want to
stream objects of arbitrary type.
- Instead of streaming the object's contents from an open
file-descriptor, this new function may want to "stream" its contents
from memory.
To avoid duplicating a significant chunk of code between the existing
`stream_blob_to_pack()`, extract an abstract `bulk_checkin_source`. This
concept currently is a thin layer of `lseek()` and `read_in_full()`, but
will grow to understand how to perform analogous operations when writing
out an object's contents from memory.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 61 +++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 53 insertions(+), 8 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 6ce62999e5..c05d06e1e1 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -140,8 +140,41 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
return 0;
}
+struct bulk_checkin_source {
+ enum { SOURCE_FILE } type;
+
+ /* SOURCE_FILE fields */
+ int fd;
+
+ /* common fields */
+ size_t size;
+ const char *path;
+};
+
+static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,
+ off_t offset)
+{
+ switch (source->type) {
+ case SOURCE_FILE:
+ return lseek(source->fd, offset, SEEK_SET);
+ default:
+ BUG("unknown bulk-checkin source: %d", source->type);
+ }
+}
+
+static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,
+ void *buf, size_t nr)
+{
+ switch (source->type) {
+ case SOURCE_FILE:
+ return read_in_full(source->fd, buf, nr);
+ default:
+ BUG("unknown bulk-checkin source: %d", source->type);
+ }
+}
+
/*
- * Read the contents from fd for size bytes, streaming it to the
+ * Read the contents from 'source' for 'size' bytes, streaming it to the
* packfile in state while updating the hash in ctx. Signal a failure
* by returning a negative value when the resulting pack would exceed
* the pack size limit and this is not the first object in the pack,
@@ -157,7 +190,7 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
*/
static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
git_hash_ctx *ctx, off_t *already_hashed_to,
- int fd, size_t size, const char *path,
+ struct bulk_checkin_source *source,
unsigned flags)
{
git_zstream s;
@@ -167,22 +200,28 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
int status = Z_OK;
int write_object = (flags & HASH_WRITE_OBJECT);
off_t offset = 0;
+ size_t size = source->size;
git_deflate_init(&s, pack_compression_level);
- hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size);
+ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,
+ size);
s.next_out = obuf + hdrlen;
s.avail_out = sizeof(obuf) - hdrlen;
while (status != Z_STREAM_END) {
if (size && !s.avail_in) {
ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
- ssize_t read_result = read_in_full(fd, ibuf, rsize);
+ ssize_t read_result;
+
+ read_result = bulk_checkin_source_read(source, ibuf,
+ rsize);
if (read_result < 0)
- die_errno("failed to read from '%s'", path);
+ die_errno("failed to read from '%s'",
+ source->path);
if (read_result != rsize)
die("failed to read %d bytes from '%s'",
- (int)rsize, path);
+ (int)rsize, source->path);
offset += rsize;
if (*already_hashed_to < offset) {
size_t hsize = offset - *already_hashed_to;
@@ -258,6 +297,12 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
unsigned header_len;
struct hashfile_checkpoint checkpoint = {0};
struct pack_idx_entry *idx = NULL;
+ struct bulk_checkin_source source = {
+ .type = SOURCE_FILE,
+ .fd = fd,
+ .size = size,
+ .path = path,
+ };
seekback = lseek(fd, 0, SEEK_CUR);
if (seekback == (off_t) -1)
@@ -283,7 +328,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
crc32_begin(state->f);
}
if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
- fd, size, path, flags))
+ &source, flags))
break;
/*
* Writing this object to the current pack will make
@@ -295,7 +340,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
hashfile_truncate(state->f, &checkpoint);
state->offset = checkpoint.offset;
flush_bulk_checkin_packfile(state);
- if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
+ if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
return error("cannot seek back");
}
the_hash_algo->final_oid_fn(result_oid, &ctx);
--
2.42.0.405.g86fe3250c2
^ permalink raw reply related
* [PATCH v4 0/7] merge-ort: implement support for packing objects together
From: Taylor Blau @ 2023-10-19 17:28 UTC (permalink / raw)
To: git
Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
(Rebased onto the current tip of 'master', which is 813d9a9188 (The
nineteenth batch, 2023-10-18) at the time of writing).
This series implements support for a new merge-tree option,
`--write-pack`, which causes any newly-written objects to be packed
together instead of being stored individually as loose.
I intentionally broke this off from the existing thread, since I
accidentally rerolled mine and Jonathan Tan's Bloom v2 series into it,
causing some confusion.
This is a new round that is significantly simplified thanks to
another very helpful suggestion[1] from Junio. By factoring out a common
"deflate object to pack" that takes an abstract bulk_checkin_source as a
parameter, all of the earlier refactorings can be dropped since we
retain only a single caller instead of multiple.
This resulted in a rather satisfying range-diff (included below, as
usual), and a similarly satisfying inter-diff:
$ git diff --stat tb/ort-bulk-checkin.v3..
bulk-checkin.c | 203 ++++++++++++++++---------------------------------
1 file changed, 64 insertions(+), 139 deletions(-)
Beyond that, the changes since last time can be viewed in the range-diff
below. Thanks in advance for any review!
[1]: https://lore.kernel.org/git/xmqq34y7plj4.fsf@gitster.g/
Taylor Blau (7):
bulk-checkin: extract abstract `bulk_checkin_source`
bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
bulk-checkin: refactor deflate routine to accept a
`bulk_checkin_source`
bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
builtin/merge-tree.c: implement support for `--write-pack`
Documentation/git-merge-tree.txt | 4 +
builtin/merge-tree.c | 5 +
bulk-checkin.c | 161 ++++++++++++++++++++++++++-----
bulk-checkin.h | 8 ++
merge-ort.c | 42 ++++++--
merge-recursive.h | 1 +
t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++
7 files changed, 280 insertions(+), 34 deletions(-)
Range-diff against v3:
1: 2dffa45183 < -: ---------- bulk-checkin: factor out `format_object_header_hash()`
2: 7a10dc794a < -: ---------- bulk-checkin: factor out `prepare_checkpoint()`
3: 20c32d2178 < -: ---------- bulk-checkin: factor out `truncate_checkpoint()`
4: 893051d0b7 < -: ---------- bulk-checkin: factor out `finalize_checkpoint()`
5: da52ec8380 ! 1: 97bb6e9f59 bulk-checkin: extract abstract `bulk_checkin_source`
@@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta
if (*already_hashed_to < offset) {
size_t hsize = offset - *already_hashed_to;
@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
- git_hash_ctx ctx;
+ unsigned header_len;
struct hashfile_checkpoint checkpoint = {0};
struct pack_idx_entry *idx = NULL;
+ struct bulk_checkin_source source = {
@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
seekback = lseek(fd, 0, SEEK_CUR);
if (seekback == (off_t) -1)
@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
- while (1) {
- prepare_checkpoint(state, &checkpoint, idx, flags);
+ crc32_begin(state->f);
+ }
if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
- fd, size, path, flags))
+ &source, flags))
break;
- truncate_checkpoint(state, &checkpoint, idx);
+ /*
+ * Writing this object to the current pack will make
+@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
+ hashfile_truncate(state->f, &checkpoint);
+ state->offset = checkpoint.offset;
+ flush_bulk_checkin_packfile(state);
- if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
+ if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
return error("cannot seek back");
}
- finalize_checkpoint(state, &ctx, &checkpoint, idx, result_oid);
+ the_hash_algo->final_oid_fn(result_oid, &ctx);
7: 04ec74e357 ! 2: 9d633df339 bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
@@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta
s.avail_out = sizeof(obuf) - hdrlen;
@@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
-
- while (1) {
- prepare_checkpoint(state, &checkpoint, idx, flags);
+ idx->offset = state->offset;
+ crc32_begin(state->f);
+ }
- if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
- &source, flags))
+ if (!stream_obj_to_pack(state, &ctx, &already_hashed_to,
+ &source, OBJ_BLOB, flags))
break;
- truncate_checkpoint(state, &checkpoint, idx);
- if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
+ /*
+ * Writing this object to the current pack will make
-: ---------- > 3: d5bbd7810e bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`
6: 4e9bac5bc1 = 4: e427fe6ad3 bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
8: 8667b76365 ! 5: 48095afe80 bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
@@ Commit message
objects individually as loose.
Similar to the existing `index_blob_bulk_checkin()` function, the
- entrypoint delegates to `deflate_blob_to_pack_incore()`, which is
- responsible for formatting the pack header and then deflating the
- contents into the pack. The latter is accomplished by calling
- deflate_obj_contents_to_pack_incore(), which takes advantage of the
- earlier refactorings and is responsible for writing the object to the
- pack and handling any overage from pack.packSizeLimit.
-
- The bulk of the new functionality is implemented in the function
- `stream_obj_to_pack()`, which can handle streaming objects from memory
- to the bulk-checkin pack as a result of the earlier refactoring.
+ entrypoint delegates to `deflate_obj_to_pack_incore()`. That function in
+ turn delegates to deflate_obj_to_pack(), which is responsible for
+ formatting the pack header and then deflating the contents into the
+ pack.
Consistent with the rest of the bulk-checkin mechanism, there are no
direct tests here. In future commits when we expose this new
@@ Commit message
Signed-off-by: Taylor Blau <me@ttaylorr.com>
## bulk-checkin.c ##
-@@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *state,
- }
+@@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,
+ return 0;
}
-+static int deflate_obj_contents_to_pack_incore(struct bulk_checkin_packfile *state,
-+ git_hash_ctx *ctx,
-+ struct hashfile_checkpoint *checkpoint,
-+ struct object_id *result_oid,
-+ const void *buf, size_t size,
-+ enum object_type type,
-+ const char *path, unsigned flags)
++static int deflate_obj_to_pack_incore(struct bulk_checkin_packfile *state,
++ struct object_id *result_oid,
++ const void *buf, size_t size,
++ const char *path, enum object_type type,
++ unsigned flags)
+{
-+ struct pack_idx_entry *idx = NULL;
-+ off_t already_hashed_to = 0;
+ struct bulk_checkin_source source = {
+ .type = SOURCE_INCORE,
+ .buf = buf,
@@ bulk-checkin.c: static void finalize_checkpoint(struct bulk_checkin_packfile *st
+ .path = path,
+ };
+
-+ /* Note: idx is non-NULL when we are writing */
-+ if (flags & HASH_WRITE_OBJECT)
-+ CALLOC_ARRAY(idx, 1);
-+
-+ while (1) {
-+ prepare_checkpoint(state, checkpoint, idx, flags);
-+
-+ if (!stream_obj_to_pack(state, ctx, &already_hashed_to, &source,
-+ type, flags))
-+ break;
-+ truncate_checkpoint(state, checkpoint, idx);
-+ bulk_checkin_source_seek_to(&source, 0);
-+ }
-+
-+ finalize_checkpoint(state, ctx, checkpoint, idx, result_oid);
-+
-+ return 0;
-+}
-+
-+static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,
-+ struct object_id *result_oid,
-+ const void *buf, size_t size,
-+ const char *path, unsigned flags)
-+{
-+ git_hash_ctx ctx;
-+ struct hashfile_checkpoint checkpoint = {0};
-+
-+ format_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_BLOB,
-+ size);
-+
-+ return deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,
-+ result_oid, buf, size,
-+ OBJ_BLOB, path, flags);
++ return deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
@@ bulk-checkin.c: int index_blob_bulk_checkin(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
-+ int status = deflate_blob_to_pack_incore(&bulk_checkin_packfile, oid,
-+ buf, size, path, flags);
++ int status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,
++ buf, size, path, OBJ_BLOB,
++ flags);
+ if (!odb_transaction_nesting)
+ flush_bulk_checkin_packfile(&bulk_checkin_packfile);
+ return status;
9: cba043ef14 ! 6: 60568f9281 bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
@@ Commit message
machinery will have enough to keep track of the converted object's hash
in order to update the compatibility mapping.
- Within `deflate_tree_to_pack_incore()`, the changes should be limited
- to something like:
+ Within some thin wrapper around `deflate_obj_to_pack_incore()` (perhaps
+ `deflate_tree_to_pack_incore()`), the changes should be limited to
+ something like:
struct strbuf converted = STRBUF_INIT;
if (the_repository->compat_hash_algo) {
@@ Commit message
Signed-off-by: Taylor Blau <me@ttaylorr.com>
## bulk-checkin.c ##
-@@ bulk-checkin.c: static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,
- OBJ_BLOB, path, flags);
- }
-
-+static int deflate_tree_to_pack_incore(struct bulk_checkin_packfile *state,
-+ struct object_id *result_oid,
-+ const void *buf, size_t size,
-+ const char *path, unsigned flags)
-+{
-+ git_hash_ctx ctx;
-+ struct hashfile_checkpoint checkpoint = {0};
-+
-+ format_object_header_hash(the_hash_algo, &ctx, &checkpoint, OBJ_TREE,
-+ size);
-+
-+ return deflate_obj_contents_to_pack_incore(state, &ctx, &checkpoint,
-+ result_oid, buf, size,
-+ OBJ_TREE, path, flags);
-+}
-+
- static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
- struct object_id *result_oid,
- int fd, size_t size,
@@ bulk-checkin.c: int index_blob_bulk_checkin_incore(struct object_id *oid,
return status;
}
@@ bulk-checkin.c: int index_blob_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
-+ int status = deflate_tree_to_pack_incore(&bulk_checkin_packfile, oid,
-+ buf, size, path, flags);
++ int status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,
++ buf, size, path, OBJ_TREE,
++ flags);
+ if (!odb_transaction_nesting)
+ flush_bulk_checkin_packfile(&bulk_checkin_packfile);
+ return status;
10: ae70508037 = 7: b9be9df122 builtin/merge-tree.c: implement support for `--write-pack`
--
2.42.0.405.g86fe3250c2
^ permalink raw reply
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