* [PATCH] advice: Add advice.scissors to suppress "do not modify or remove this line"
From: Josh Triplett @ 2024-02-26 4:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
The scissors line before the diff in a verbose commit, or above all the
comments when using --cleanup=scissors, has the following two lines of
explanation after it:
Do not modify or remove the line above.
Everything below it will be ignored.
This is useful advice for new users, but potentially redundant for
experienced users, who might instead appreciate seeing two more lines of
information in their editor.
Add advice.scissors to suppress that explanation.
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
Documentation/config/advice.txt | 5 +++++
advice.c | 1 +
advice.h | 1 +
wt-status.c | 3 ++-
4 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2..33ab688b6c 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -104,6 +104,11 @@ advice.*::
rmHints::
In case of failure in the output of linkgit:git-rm[1],
show directions on how to proceed from the current state.
+ scissors::
+ Advice shown by linkgit:git-commit[1] in the commit message
+ opened in an editor, after a scissors line (containing >8),
+ saying not to remove the line and that everything after the line
+ will be ignored.
sequencerInUse::
Advice shown when a sequencer command is already in progress.
skippedCherryPicks::
diff --git a/advice.c b/advice.c
index 6e9098ff08..0588012562 100644
--- a/advice.c
+++ b/advice.c
@@ -71,6 +71,7 @@ static struct {
[ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" },
[ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" },
[ADVICE_RM_HINTS] = { "rmHints" },
+ [ADVICE_SCISSORS] = { "scissors" },
[ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse" },
[ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" },
[ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38..9725aa4199 100644
--- a/advice.h
+++ b/advice.h
@@ -39,6 +39,7 @@ enum advice_type {
ADVICE_RESET_NO_REFRESH_WARNING,
ADVICE_RESOLVE_CONFLICT,
ADVICE_RM_HINTS,
+ ADVICE_SCISSORS,
ADVICE_SEQUENCER_IN_USE,
ADVICE_SET_UPSTREAM_FAILURE,
ADVICE_SKIPPED_CHERRY_PICKS,
diff --git a/wt-status.c b/wt-status.c
index 459d399baa..19d4986351 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1104,7 +1104,8 @@ void wt_status_append_cut_line(struct strbuf *buf)
const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
strbuf_commented_addf(buf, comment_line_char, "%s", cut_line);
- strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
+ if (advice_enabled(ADVICE_SCISSORS))
+ strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
}
void wt_status_add_cut_line(FILE *fp)
--
2.43.0
^ permalink raw reply related
* Recall: CR position changed in exported patch file subject section
From: Chen, Boyang @ 2024-02-26 3:45 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Wilson, Mark
Chen, Boyang would like to recall the message, "CR position changed in exported patch file subject section ".
^ permalink raw reply
* Re: [PATCH v2 8/8] cherry-pick: add `--empty` for more robust redundant commit handling
From: Brian Lyles @ 2024-02-26 3:32 UTC (permalink / raw)
To: phillip.wood, gitster; +Cc: git, newren, me
In-Reply-To: <83070e02-8e6b-43d2-819d-2272fe895c75@gmail.com>
Hi Phillip and Junio
On Sun, Feb 25, 2024 at 10:57 AM <phillip.wood123@gmail.com> wrote:
> On 23/02/2024 06:58, Brian Lyles wrote:
>> I think I'm on board with leaving `--keep-redundant-commits` alone. I'm
>> on the fence about having `--empty=keep` imply `--allow-empty` after
>> seeing Junio's concerns. I laid out the options that I see in a reply to
>> patch 6/8[1] and would appreciate input there. I'll adjust the details
>> of this commit in v3 based on what we decide there.
>
> I'll take a look at that in the next couple of days
>
>> [1]: https://lore.kernel.org/git/17b666ca6c4b7561.70b1dd9aae081c6e.203dcd72f6563036@zivdesk/
I'm not quite sure what happened here, but it seems that:
- The above link is to the wrong email, and
- The email I meant to link to isn't showing up in the archive for some
reason, despite clearly showing as sent in my mailbox
Apologies for the confusion -- I'm not sure what happened.
In an attempt to keep this conversation on track, I've copied my
original attempted reply to Phillip's[2] and Junio's[3] replies below.
[2]: https://lore.kernel.org/git/3f276e10-7b03-4480-a157-47a7648e7f19@gmail.com/
[3]: https://lore.kernel.org/git/xmqqwmqwcpf7.fsf@gitster.g/
On Fri, Feb 23, 2024 at 12:08 AM Brian Lyles <brianmlyles@gmail.com> wrote:
>
> On Thu, Feb 22, 2024 at 10:35 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> > I agree that if we were starting from scratch there would be no reason
> > to tie --apply-empty and --keep-redundant-commits together but I'm not
> > sure it is worth the disruption of changing it now. We're about to add
> > empty=keep which won't imply --allow-empty for anyone who wants that
> > behavior and I still tend to think the practical effect of implying
> > --allow-empty with --keep-redundant-commits is largely beneficial as I'm
> > skeptical that users want to keep commits that become empty but not the
> > ones that started empty.
>
> I think that's fair. I am okay dropping this potentially disruptive
> change.
>
> It sounds like you are on board with `--empty=keep` not having the same
> implication?
>
> That said...
>
> On Thu, Feb 22, 2024 at 12:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> > I do not quite see a good reason to do the opposite, dropping
> > commits that started out as empty but keeping the ones that have
> > become empty. Such a behaviour has additional downside that after
> > such a cherry-pick, when you cherry-pick the resulting history onto
> > yet another base, your precious "were not empty but have become so
> > during the initial cherry-pick" commits will appear as commits that
> > were empty from the start. So I do not see much reason to allow the
> > decoupling, even with the new "empty=keep" thing that does not imply
> > "allow-empty".
>
> Junio -- can you clarify this part?
>
> > So I do not see much reason to allow the decoupling, even with the new
> > "empty=keep" thing that does not imply "allow-empty"
>
> I'm not 100% sure if you are saying that you want `--empty=keep` to
> *also* imply `--allow-empty`, or that you simply want
> `--keep-redundant-commits` to continue implying `--allow-empty`
> *despite* the new `--empty=keep` no implying the same.
>
> On the one hand, I agree with Phillip's sentiment of "if we were
> starting from scratch there would be no reason to tie --apply-empty and
> --keep-redundant-commits together" (though your points perhaps provide
> such a reason). On the other, if both `--keep-redundant-commits` and
> `--empty=keep` behave the same way, it makes sense to soft-deprecate
> `--keep-redundant-commits` as I have currently done later in this
> series. If they do not behave the same way, that deprecation makes less
> sense and we have two very similar (but not quite identical) options.
>
> Just to make sure we're on the same page, the options I see are:
>
> - (A): Neither `--keep-redundant-commits` nor `--empty=keep` imply
> `--allow-empty`, `--keep-redundant-commits` is soft-deprecated
> - (B): Both `--keep-redundant-commits` and `--empty=keep` imply
> `--allow-empty`, `--keep-redundant-commits` is soft-deprecated
> - (C): Both `--keep-redundant-commits` and `--empty=keep` imply
> `--allow-empty`, `--keep-redundant-commits` is *not* soft-deprecated
> as it is more descriptive as noted by Junio here[1]
> - (D): `--keep-redundant-commits` continues to imply `--allow-empty` but
> `--empty=keep` does not. `--keep-redundant-commits` is *not*
> soft-deprecated as it behaves differently.
>
> (A) represents this v2 of the patch.
>
> I'm coming around to (B) based on Junio's workflow concerns, but to be
> honest I am fine with any of these options. Junio, I *think* you're
> saying you'd prefer (B) or (C)? Phillip, it sounds like you are okay
> with (D) based on your response -- how do you feel about (B)?
>
> [1]: https://lore.kernel.org/git/xmqq8r4gnd3c.fsf@gitster.g/
Thank you both for your review and insight!
--
Thank you,
Brian Lyles
^ permalink raw reply
* Re: [PATCH v2 7/8] cherry-pick: enforce `--keep-redundant-commits` incompatibility
From: Brian Lyles @ 2024-02-26 3:04 UTC (permalink / raw)
To: phillip.wood; +Cc: git, newren, me, gitster
In-Reply-To: <f10cc07a-0e77-462c-bc86-ca0452c20a1c@gmail.com>
Hi Phillip
On Sun, Feb 25, 2024 at 10:58 AM <phillip.wood123@gmail.com> wrote:
> Hi Brian
>
> On 23/02/2024 06:23, Brian Lyles wrote:
>> On Thu, Feb 22, 2024 at 10:35 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>>> Well spotted, do we really need a new test file just for this though? I
>>> wonder if the new test would be better off living in
>>> t3505-cherry-pick-empty.sh or t3507-cherry-pick-conflict.sh
>>
>> I was modelling this off of 't3422-rebase-incompatible-options.sh'.
>
> The rebase case is more complicated due to different options being
> supported by the two different backends. Thankfully here we only have to
> worry about options that are incompatible with "--continue/--abort" and
> so adding "--continue rejects --foo" into the file that tests option
> "--foo" keeps everything together.
>
>> Additionally, I do feel like these tests are only tangentially related
>> to the tests that actually exercise the features themselves. Notably,
>> the setup requirements are drastically different (simpler) since the
>> test should fail long before any setup actually matters. For that
>> reason, I think a separate file where other future tests for
>> incompatible options can also live does make sense.
>>
>> Is there any particular downside to the new file that I am unaware of?
>
> The main downside is that it spreads the tests for a particular option
> over several test files. There is also an overhead in setting up the
> repository at the start of each test file.
That makes sense. I'll move these tests into
`t3505-cherry-pick-empty.sh` for v3, along with the corresponding
incompatibility tests for `--empty` introduced in the ultimate commit
for the series.
--
Thank you,
Brian Lyles
^ permalink raw reply
* Re: [PATCH v2 8/8] cherry-pick: add `--empty` for more robust redundant commit handling
From: Brian Lyles @ 2024-02-26 2:21 UTC (permalink / raw)
To: phillip.wood; +Cc: git, newren, me, gitster
In-Reply-To: <83070e02-8e6b-43d2-819d-2272fe895c75@gmail.com>
Hi Phillip
On Sun, Feb 25, 2024 at 10:57 AM <phillip.wood123@gmail.com> wrote:
>> How concerned are you about the current implementation? Does it make
>> sense to you to defer this suggestion to a future series that cleans up
>> the tests to do more user-oriented checks?
>
> I think adding tests that follow a pattern we want to change is just
> storing up work for the future and makes it less likely we'll improve
> things because it will be more work to do so.
That's fair. I'll rework these in v3. Thanks!
--
Thank you,
Brian Lyles
^ permalink raw reply
* Re: [PATCH 2/2] name-rev: use mem_pool_strfmt()
From: Junio C Hamano @ 2024-02-26 2:05 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <20240225113947.89357-3-l.s.r@web.de>
René Scharfe <l.s.r@web.de> writes:
> 1c56fc2084 (name-rev: pre-size buffer in get_parent_name(), 2020-02-04)
> got a big performance boost in an unusual repository by calculating the
> name length in advance. This is a bit awkward, as it references the
> name components twice.
>
> Use a memory pool to store the strings for the struct rev_name member
> tip_name. Using mem_pool_strfmt() allows efficient allocation without
> explicit size calculation. This simplifies the formatting part of the
> code without giving up performance:
>
> Benchmark 1: ./git_2.44.0 -C ../chromium/src name-rev --all
> Time (mean ± σ): 1.231 s ± 0.013 s [User: 1.082 s, System: 0.136 s]
> Range (min … max): 1.214 s … 1.252 s 10 runs
>
> Benchmark 2: ./git -C ../chromium/src name-rev --all
> Time (mean ± σ): 1.220 s ± 0.020 s [User: 1.083 s, System: 0.130 s]
> Range (min … max): 1.197 s … 1.254 s 10 runs
>
> Don't bother discarding the memory pool just before exiting. The effort
> for that would be very low, but actually measurable in the above
> example, with no benefit to users. At least UNLEAK it to calm down leak
> checkers. This addresses the leaks that 45a14f578e (Revert "name-rev:
> release unused name strings", 2022-04-22) brought back.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> This doesn't make any test script leak-free, though.
Hmph, is the root cause of the leak because no sensible ownership
rules are applied to .tip_name? Sometimes it is allocated for the
paritcular rev_name, but some other times the pointer is copied from
another rev_name.tip_name? As the way currently the code uses the
.tip_name member seems to be "allocate and use without any freeing",
I tend to agree that throwing them into mem-pool does make sense.
> builtin/name-rev.c | 39 ++++++++++++++++++++-------------------
> 1 file changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 2dd1807c4e..ad9930c831 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -15,6 +15,7 @@
> #include "commit-slab.h"
> #include "commit-graph.h"
> #include "wildmatch.h"
> +#include "mem-pool.h"
>
> /*
> * One day. See the 'name a rev shortly after epoch' test in t6120 when
> @@ -155,30 +156,25 @@ static struct rev_name *create_or_update_name(struct commit *commit,
> return name;
> }
>
> -static char *get_parent_name(const struct rev_name *name, int parent_number)
> +static char *get_parent_name(const struct rev_name *name, int parent_number,
> + struct mem_pool *string_pool)
> {
> - struct strbuf sb = STRBUF_INIT;
> size_t len;
>
> strip_suffix(name->tip_name, "^0", &len);
> if (name->generation > 0) {
> - strbuf_grow(&sb, len +
> - 1 + decimal_width(name->generation) +
> - 1 + decimal_width(parent_number));
> - strbuf_addf(&sb, "%.*s~%d^%d", (int)len, name->tip_name,
> - name->generation, parent_number);
> + return mem_pool_strfmt(string_pool, "%.*s~%d^%d",
> + (int)len, name->tip_name,
> + name->generation, parent_number);
> } else {
> - strbuf_grow(&sb, len +
> - 1 + decimal_width(parent_number));
> - strbuf_addf(&sb, "%.*s^%d", (int)len, name->tip_name,
> - parent_number);
> + return mem_pool_strfmt(string_pool, "%.*s^%d",
> + (int)len, name->tip_name, parent_number);
> }
> - return strbuf_detach(&sb, NULL);
> }
>
> static void name_rev(struct commit *start_commit,
> const char *tip_name, timestamp_t taggerdate,
> - int from_tag, int deref)
> + int from_tag, int deref, struct mem_pool *string_pool)
> {
> struct prio_queue queue;
> struct commit *commit;
> @@ -195,9 +191,10 @@ static void name_rev(struct commit *start_commit,
> if (!start_name)
> return;
> if (deref)
> - start_name->tip_name = xstrfmt("%s^0", tip_name);
> + start_name->tip_name = mem_pool_strfmt(string_pool, "%s^0",
> + tip_name);
> else
> - start_name->tip_name = xstrdup(tip_name);
> + start_name->tip_name = mem_pool_strdup(string_pool, tip_name);
>
> memset(&queue, 0, sizeof(queue)); /* Use the prio_queue as LIFO */
> prio_queue_put(&queue, start_commit);
> @@ -235,7 +232,8 @@ static void name_rev(struct commit *start_commit,
> if (parent_number > 1)
> parent_name->tip_name =
> get_parent_name(name,
> - parent_number);
> + parent_number,
> + string_pool);
> else
> parent_name->tip_name = name->tip_name;
> ALLOC_GROW(parents_to_queue,
> @@ -415,7 +413,7 @@ static int name_ref(const char *path, const struct object_id *oid,
> return 0;
> }
>
> -static void name_tips(void)
> +static void name_tips(struct mem_pool *string_pool)
> {
> int i;
>
> @@ -428,7 +426,7 @@ static void name_tips(void)
> struct tip_table_entry *e = &tip_table.table[i];
> if (e->commit) {
> name_rev(e->commit, e->refname, e->taggerdate,
> - e->from_tag, e->deref);
> + e->from_tag, e->deref, string_pool);
> }
> }
> }
> @@ -561,6 +559,7 @@ static void name_rev_line(char *p, struct name_ref_data *data)
>
> int cmd_name_rev(int argc, const char **argv, const char *prefix)
> {
> + struct mem_pool string_pool;
> struct object_array revs = OBJECT_ARRAY_INIT;
> int all = 0, annotate_stdin = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
> struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
> @@ -587,6 +586,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
> OPT_END(),
> };
>
> + mem_pool_init(&string_pool, 0);
> init_commit_rev_name(&rev_names);
> git_config(git_default_config, NULL);
> argc = parse_options(argc, argv, prefix, opts, name_rev_usage, 0);
> @@ -648,7 +648,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
> adjust_cutoff_timestamp_for_slop();
>
> for_each_ref(name_ref, &data);
> - name_tips();
> + name_tips(&string_pool);
>
> if (annotate_stdin) {
> struct strbuf sb = STRBUF_INIT;
> @@ -676,6 +676,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
> always, allow_undefined, data.name_only);
> }
>
> + UNLEAK(string_pool);
> UNLEAK(revs);
> return 0;
> }
> --
> 2.44.0
^ permalink raw reply
* K
From: gooplestore11 @ 2024-02-26 0:55 UTC (permalink / raw)
To: git
Sent from my iPad
^ permalink raw reply
* [PATCH v2] completion: fix __git_complete_worktree_paths
From: Rubén Justo @ 2024-02-25 23:53 UTC (permalink / raw)
To: Git List
In-Reply-To: <a1561e02-40dc-4ac3-ae7a-666db0f810ed@gmail.com>
Use __git to invoke "worktree list" in __git_complete_worktree_paths, to
respect any "-C" and "--git-dir" options present on the command line.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
I stumbled upon this in a situation like:
$ git init /tmp/foo && cd /tmp/foo
$ git worktree add --orphan foo_wt
$ git init /tmp/bar && cd /tmp/bar
$ git worktree add --orphan bar_wt
$ cd /tmp/foo
$ git -C /tmp/bar worktree remove <TAB>
... expecting /tmp/bar/wt, but ...
$ git -C /tmp/bar worktree remove /tmp/foo_wt
In this iteration, v2, some tests are included.
The function __git was introduced in 1cd23e9e05 (completion: don't use
__gitdir() for git commands, 2017-02-03). It is a small function, so
I'll include here to ease the review of this patch:
# Runs git with all the options given as argument, respecting any
# '--git-dir=<path>' and '-C <path>' options present on the command line
__git ()
{
git ${__git_C_args:+"${__git_C_args[@]}"} \
${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null
}
contrib/completion/git-completion.bash | 2 +-
t/t9902-completion.sh | 24 ++++++++++++++++++++++++
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 444b3efa63..86e55dc67f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3571,7 +3571,7 @@ __git_complete_worktree_paths ()
# Generate completion reply from worktree list skipping the first
# entry: it's the path of the main worktree, which can't be moved,
# removed, locked, etc.
- __gitcomp_nl "$(git worktree list --porcelain |
+ __gitcomp_nl "$(__git worktree list --porcelain |
sed -n -e '2,$ s/^worktree //p')"
}
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index b16c284181..7c0f82f31a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1263,6 +1263,30 @@ test_expect_success '__git_complete_fetch_refspecs - fully qualified & prefix' '
test_cmp expected out
'
+test_expect_success '__git_complete_worktree_paths' '
+ test_when_finished "git worktree remove other_wt" &&
+ git worktree add --orphan other_wt &&
+ run_completion "git worktree remove " &&
+ grep other_wt out
+'
+
+test_expect_success '__git_complete_worktree_paths - not a git repository' '
+ (
+ cd non-repo &&
+ GIT_CEILING_DIRECTORIES="$ROOT" &&
+ export GIT_CEILING_DIRECTORIES &&
+ test_completion "git worktree remove " "" 2>err &&
+ test_must_be_empty err
+ )
+'
+
+test_expect_success '__git_complete_worktree_paths with -C' '
+ test_when_finished "rm -rf to_delete" &&
+ git -C otherrepo worktree add --orphan otherrepo_wt &&
+ run_completion "git -C otherrepo worktree remove " &&
+ grep otherrepo_wt out
+'
+
test_expect_success 'git switch - with no options, complete local branches and unique remote branch names for DWIM logic' '
test_completion "git switch " <<-\EOF
branch-in-other Z
--
2.44.0.1.g0da3aa8f7f
^ permalink raw reply related
* Interest in Future Collaboration for GSoC 2024
From: Akhilesh Kumar Yadav @ 2024-02-25 23:10 UTC (permalink / raw)
To: git
Greetings!
I hope this message finds you well. I am writing to express my admiration for the innovative technology that your organization is working on for GSoC 2024. I have been following your work and find it truly amazing.
Currently, I am committed to another organisation and may not be able to contribute to your projects at this time. However, I noticed that your organisation sometimes faces a shortage of proposals. If such a situation arises, please consider this email as an expression of my interest.
I hold a strong interest in your organisation and would be more than willing to submit a comprehensive proposal should the need arise. I have also been an active contribution at open source projects and have decent programming experience.
Thank you. I look forward to the possibility of future interactions.
Best regards,
Akhilesh.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)
From: SZEDER Gábor @ 2024-02-25 22:59 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, git
In-Reply-To: <ZabpBHBB1TXIXJLr@nand.local>
On Tue, Jan 16, 2024 at 03:37:24PM -0500, Taylor Blau wrote:
> Hi Gábor,
>
> On Sun, Jan 14, 2024 at 12:41:34AM +0100, SZEDER Gábor wrote:
> > > In any case, here's the patch on top (with a lightly modified version of
> > > the test you wrote in <20230830200218.GA5147@szeder.dev>:
> >
> > I certainly hope that I'm just misunderstanding, and you don't
> > actually imply that this one test in its current form would qualify as
> > thorough testing... :)
>
> I hear what you're saying, though I think that the interesting behavior
> that would be most likely to regress is the transition between different
> Bloom filter settings/hash-version across split commit-graph layers.
>
> We have extensive tests on either "side" of this transition for both v1
> and v2 Bloom filters, so I'm not sure what we'd want to add there. Like
> I said, the transition is the primary (previously-)untested area of this
> code that I would want to ensure is covered to protect against
> regressions.
>
> I think that the most recent round of this series accomplishes that
> goal.
It's great that we finally have test cases for different Bloom filter
settings in different commit-graph layers, including a test case that
merges those layers, but that test case doesn't check that the
resulting merged commit-graph file contains the right settings. And
there is still no test case that merges layers with different Bloom
filter versions.
I think adding these would be the bare minimum... and would need more
for due diligence.
^ permalink raw reply
* Re: [PATCH v6 13/16] commit-graph: new Bloom filter version that fixes murmur3
From: SZEDER Gábor @ 2024-02-25 22:15 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Junio C Hamano, Jonathan Tan
In-Reply-To: <d2f11c082d3bf10d9127c330a7d59b7e47ac4f21.1706741516.git.me@ttaylorr.com>
On Wed, Jan 31, 2024 at 05:52:54PM -0500, Taylor Blau wrote:
> diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
> index 93b8d096cf..a7bf3a7dca 100755
> --- a/t/t4216-log-bloom.sh
> +++ b/t/t4216-log-bloom.sh
> @@ -485,6 +485,33 @@ test_expect_success 'merge graph layers with incompatible Bloom settings' '
> test_must_be_empty err
> '
>
> +test_expect_success 'ensure Bloom filter with incompatible versions are ignored' '
> + rm "$repo/$graph" &&
> +
> + git -C $repo log --oneline --no-decorate -- $CENT >expect &&
The $CENT variable is not set at this point in the test script.
However, even if it were, the repository used in this test case
doesn't contain any file with that name.
> +
> + # Compute v1 Bloom filters for commits at the bottom.
> + git -C $repo rev-parse HEAD^ >in &&
> + git -C $repo commit-graph write --stdin-commits --changed-paths \
> + --split <in &&
> +
> + # Compute v2 Bloomfilters for the rest of the commits at the top.
> + git -C $repo rev-parse HEAD >in &&
> + git -C $repo -c commitGraph.changedPathsVersion=2 commit-graph write \
> + --stdin-commits --changed-paths --split=no-merge <in &&
> +
> + test_line_count = 2 $repo/$chain &&
> +
> + git -C $repo log --oneline --no-decorate -- $CENT >actual 2>err &&
> + test_cmp expect actual &&
> +
> + layer="$(head -n 1 $repo/$chain)" &&
> + cat >expect.err <<-EOF &&
> + warning: disabling Bloom filters for commit-graph layer $SQ$layer$SQ due to incompatible settings
> + EOF
> + test_cmp expect.err err
The variable $repo is used 9 times in this test case. I think it
would be simpler and easier follow if it used a ( cd $repo && ... )
subshell, like many of the previous tests in this test script,
> +'
> +
> get_first_changed_path_filter () {
> test-tool read-graph bloom-filters >filters.dat &&
> head -n 1 filters.dat
> @@ -536,6 +563,120 @@ test_expect_success 'version 1 changed-path used when version 1 requested' '
> )
> '
>
> +test_expect_success 'version 1 changed-path not used when version 2 requested' '
> + (
> + cd highbit1 &&
> + git config --add commitgraph.changedPathsVersion 2 &&
> + test_bloom_filters_not_used "-- another$CENT"
> + )
> +'
> +
> +test_expect_success 'version 1 changed-path used when autodetect requested' '
> + (
> + cd highbit1 &&
> + git config --add commitgraph.changedPathsVersion -1 &&
> + test_bloom_filters_used "-- another$CENT"
> + )
> +'
> +
> +test_expect_success 'when writing another commit graph, preserve existing version 1 of changed-path' '
> + test_commit -C highbit1 c1double "$CENT$CENT" &&
> + git -C highbit1 commit-graph write --reachable --changed-paths &&
> + (
> + cd highbit1 &&
> + git config --add commitgraph.changedPathsVersion -1 &&
> + echo "options: bloom(1,10,7) read_generation_data" >expect &&
> + test-tool read-graph >full &&
> + grep options full >actual &&
> + test_cmp expect actual
> + )
> +'
> +
> +test_expect_success 'set up repo with high bit path, version 2 changed-path' '
> + git init highbit2 &&
> + git -C highbit2 config --add commitgraph.changedPathsVersion 2 &&
> + test_commit -C highbit2 c2 "$CENT" &&
> + git -C highbit2 commit-graph write --reachable --changed-paths
> +'
> +
> +test_expect_success 'check value of version 2 changed-path' '
> + (
> + cd highbit2 &&
> + echo "c01f" >expect &&
> + get_first_changed_path_filter >actual &&
> + test_cmp expect actual
> + )
> +'
> +
> +test_expect_success 'setup make another commit' '
> + # "git log" does not use Bloom filters for root commits - see how, in
> + # revision.c, rev_compare_tree() (the only code path that eventually calls
> + # get_bloom_filter()) is only called by try_to_simplify_commit() when the commit
> + # has one parent. Therefore, make another commit so that we perform the tests on
> + # a non-root commit.
> + test_commit -C highbit2 anotherc2 "another$CENT"
> +'
> +
> +test_expect_success 'version 2 changed-path used when version 2 requested' '
> + (
> + cd highbit2 &&
> + test_bloom_filters_used "-- another$CENT"
> + )
> +'
> +
> +test_expect_success 'version 2 changed-path not used when version 1 requested' '
> + (
> + cd highbit2 &&
> + git config --add commitgraph.changedPathsVersion 1 &&
> + test_bloom_filters_not_used "-- another$CENT"
> + )
> +'
> +
> +test_expect_success 'version 2 changed-path used when autodetect requested' '
> + (
> + cd highbit2 &&
> + git config --add commitgraph.changedPathsVersion -1 &&
> + test_bloom_filters_used "-- another$CENT"
> + )
> +'
> +
> +test_expect_success 'when writing another commit graph, preserve existing version 2 of changed-path' '
> + test_commit -C highbit2 c2double "$CENT$CENT" &&
> + git -C highbit2 commit-graph write --reachable --changed-paths &&
> + (
> + cd highbit2 &&
> + git config --add commitgraph.changedPathsVersion -1 &&
> + echo "options: bloom(2,10,7) read_generation_data" >expect &&
> + test-tool read-graph >full &&
> + grep options full >actual &&
> + test_cmp expect actual
> + )
> +'
> +
> +test_expect_success 'when writing commit graph, do not reuse changed-path of another version' '
> + git init doublewrite &&
> + test_commit -C doublewrite c "$CENT" &&
> + git -C doublewrite config --add commitgraph.changedPathsVersion 1 &&
> + git -C doublewrite commit-graph write --reachable --changed-paths &&
> + for v in -2 3
> + do
> + git -C doublewrite config --add commitgraph.changedPathsVersion $v &&
> + git -C doublewrite commit-graph write --reachable --changed-paths 2>err &&
> + cat >expect <<-EOF &&
> + warning: attempting to write a commit-graph, but ${SQ}commitgraph.changedPathsVersion${SQ} ($v) is not supported
> + EOF
> + test_cmp expect err || return 1
> + done &&
> + git -C doublewrite config --add commitgraph.changedPathsVersion 2 &&
> + git -C doublewrite commit-graph write --reachable --changed-paths &&
The path 'doublewrite' is used 8 times in this test case before
finally cd-ing into that directory in a subshell...
> + (
> + cd doublewrite &&
> + echo "c01f" >expect &&
> + get_first_changed_path_filter >actual &&
> + test_cmp expect actual
> + )
> +'
> +
> corrupt_graph () {
> test_when_finished "rm -rf $graph" &&
> git commit-graph write --reachable --changed-paths &&
> --
> 2.43.0.509.g253f65a7fc
>
^ permalink raw reply
* [PATCH v5 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert
From: Philippe Blain @ 2024-02-25 21:56 UTC (permalink / raw)
To: git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Junio C Hamano, Michael Lohmann,
Philippe Blain
In-Reply-To: <20240225-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v5-0-af1ef2d9e44d@gmail.com>
From: Michael Lohmann <mi.al.lohmann@gmail.com>
'git log' learned in ae3e5e1ef2 (git log -p --merge [[--] paths...],
2006-07-03) to show commits touching conflicted files in the range
HEAD...MERGE_HEAD, an addition documented in d249b45547 (Document
rev-list's option --merge, 2006-08-04).
It can be useful to look at the commit history to understand what lead
to merge conflicts also for other mergy operations besides merges, like
cherry-pick, revert and rebase.
For rebases and cherry-picks, an interesting range to look at is
HEAD...{REBASE_HEAD,CHERRY_PICK_HEAD}, since even if all the commits
included in that range are not directly part of the 3-way merge,
conflicts encountered during these operations can indeed be caused by
changes introduced in preceding commits on both sides of the history.
For revert, as we are (most likely) reversing changes from a previous
commit, an appropriate range is REVERT_HEAD..HEAD, which is equivalent
to REVERT_HEAD...HEAD and to HEAD...REVERT_HEAD, if we keep HEAD and its
parents on the left side of the range.
As such, adjust the code in prepare_show_merge so it constructs the
range HEAD...$OTHER for OTHER={MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD
or REBASE_HEAD}. Note that we try these pseudorefs in order, so keep
REBASE_HEAD last since the three other operations can be performed
during a rebase. Note also that in the uncommon case where $OTHER and
HEAD do not share a common ancestor, this will show the complete
histories of both sides since their root commits, which is the same
behaviour as currently happens in that case for HEAD and MERGE_HEAD.
Adjust the documentation of this option accordingly.
Co-authored-by: Johannes Sixt <j6t@kdbg.org>
Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
[jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
Documentation/rev-list-options.txt | 7 +++++--
revision.c | 31 +++++++++++++++++++++++--------
2 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 2bf239ff03..9ce7a5eedc 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -341,8 +341,11 @@ See also linkgit:git-reflog[1].
Under `--pretty=reference`, this information will not be shown at all.
--merge::
- After a failed merge, show refs that touch files having a
- conflict and don't exist on all heads to merge.
+ Show commits touching conflicted paths in the range `HEAD...<other>`,
+ where `<other>` is the first existing pseudoref in `MERGE_HEAD`,
+ `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works
+ when the index has unmerged entries. This option can be used to show
+ relevant commits when resolving conflicts from a 3-way merge.
--boundary::
Output excluded boundary commits. Boundary commits are
diff --git a/revision.c b/revision.c
index ee26988cc6..a90a6f861b 100644
--- a/revision.c
+++ b/revision.c
@@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs,
}
}
+static const char *lookup_other_head(struct object_id *oid)
+{
+ int i;
+ static const char *const other_head[] = {
+ "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
+ };
+
+ for (i = 0; i < ARRAY_SIZE(other_head); i++)
+ if (!read_ref_full(other_head[i],
+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ oid, NULL)) {
+ if (is_null_oid(oid))
+ die(_("%s is a symbolic ref?"), other_head[i]);
+ return other_head[i];
+ }
+
+ die(_("--merge requires one of the pseudorefs MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD"));
+}
+
static void prepare_show_merge(struct rev_info *revs)
{
struct commit_list *bases;
struct commit *head, *other;
struct object_id oid;
+ const char *other_name;
const char **prune = NULL;
int i, prune_num = 1; /* counting terminating NULL */
struct index_state *istate = revs->repo->index;
@@ -1973,15 +1993,10 @@ static void prepare_show_merge(struct rev_info *revs)
if (repo_get_oid(the_repository, "HEAD", &oid))
die("--merge without HEAD?");
head = lookup_commit_or_die(&oid, "HEAD");
- if (read_ref_full("MERGE_HEAD",
- RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
- &oid, NULL))
- die("--merge without MERGE_HEAD?");
- if (is_null_oid(&oid))
- die(_("MERGE_HEAD is a symbolic ref?"));
- other = lookup_commit_or_die(&oid, "MERGE_HEAD");
+ other_name = lookup_other_head(&oid);
+ other = lookup_commit_or_die(&oid, other_name);
add_pending_object(revs, &head->object, "HEAD");
- add_pending_object(revs, &other->object, "MERGE_HEAD");
+ add_pending_object(revs, &other->object, other_name);
bases = repo_get_merge_bases(the_repository, head, other);
add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
--
2.39.1
^ permalink raw reply related
* [PATCH v5 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Philippe Blain @ 2024-02-25 21:56 UTC (permalink / raw)
To: git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Junio C Hamano, Michael Lohmann,
Philippe Blain
In-Reply-To: <20240225-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v5-0-af1ef2d9e44d@gmail.com>
From: Michael Lohmann <mi.al.lohmann@gmail.com>
This is done to
(1) ensure MERGE_HEAD is a ref,
(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
(3) error out when MERGE_HEAD is a symref.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
revision.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/revision.c b/revision.c
index 2424c9bd67..ee26988cc6 100644
--- a/revision.c
+++ b/revision.c
@@ -1973,8 +1973,12 @@ static void prepare_show_merge(struct rev_info *revs)
if (repo_get_oid(the_repository, "HEAD", &oid))
die("--merge without HEAD?");
head = lookup_commit_or_die(&oid, "HEAD");
- if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
+ if (read_ref_full("MERGE_HEAD",
+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ &oid, NULL))
die("--merge without MERGE_HEAD?");
+ if (is_null_oid(&oid))
+ die(_("MERGE_HEAD is a symbolic ref?"));
other = lookup_commit_or_die(&oid, "MERGE_HEAD");
add_pending_object(revs, &head->object, "HEAD");
add_pending_object(revs, &other->object, "MERGE_HEAD");
--
2.39.1
^ permalink raw reply related
* [PATCH v5 0/2] Implement `git log --merge` also for rebase/cherry-pick/revert
From: Philippe Blain @ 2024-02-25 21:56 UTC (permalink / raw)
To: git
Cc: Johannes Sixt, Elijah Newren, Michael Lohmann, Phillip Wood,
Patrick Steinhardt, Junio C Hamano, Michael Lohmann,
Philippe Blain
In-Reply-To: <20240210-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v4-0-3bc9e62808f4@gmail.com>
Changes in v5:
- Marked error messages for translation and tweaked them as suggested by Phillip
- Reworded the message of 2/2 as suggested by Phillip
- Removed the change to gitk's doc in 2/2 as pointed out by Johannes
- Fixed the trailers in 2/2
- Improved the doc in 2/2 as suggested by Phillip and Jean-Noël
Changes in v4:
- Added a commit message for 2/2 detailing the use case and summarizing the discussion in the thread
- Adjusted the documentation of the option
---
Michael Lohmann (2):
revision: ensure MERGE_HEAD is a ref in prepare_show_merge
revision: implement `git log --merge` also for rebase/cherry-pick/revert
Documentation/rev-list-options.txt | 7 +++++--
revision.c | 27 +++++++++++++++++++++++----
2 files changed, 28 insertions(+), 6 deletions(-)
---
base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5
change-id: 20240210-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-05bd8e8797db
Range-diff versus v4:
1: 37405be1a3 ! 1: c9536431d1 revision: ensure MERGE_HEAD is a ref in prepare_show_merge
@@ Commit message
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
+ Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
## revision.c ##
@@ revision.c: static void prepare_show_merge(struct rev_info *revs)
@@ revision.c: static void prepare_show_merge(struct rev_info *revs)
+ &oid, NULL))
die("--merge without MERGE_HEAD?");
+ if (is_null_oid(&oid))
-+ die("MERGE_HEAD is a symbolic ref???");
++ die(_("MERGE_HEAD is a symbolic ref?"));
other = lookup_commit_or_die(&oid, "MERGE_HEAD");
add_pending_object(revs, &head->object, "HEAD");
add_pending_object(revs, &other->object, "MERGE_HEAD");
2: 6ac1608809 ! 2: 1641c4be81 revision: implement `git log --merge` also for rebase/cherry-pick/revert
@@ Commit message
to merge conflicts also for other mergy operations besides merges, like
cherry-pick, revert and rebase.
- For rebases, an interesting range to look at is HEAD...REBASE_HEAD,
- since the conflicts are usually caused by how the code changed
- differently on HEAD since REBASE_HEAD forked from it.
+ For rebases and cherry-picks, an interesting range to look at is
+ HEAD...{REBASE_HEAD,CHERRY_PICK_HEAD}, since even if all the commits
+ included in that range are not directly part of the 3-way merge,
+ conflicts encountered during these operations can indeed be caused by
+ changes introduced in preceding commits on both sides of the history.
- For cherry-picks and revert, it is less clear that
- HEAD...CHERRY_PICK_HEAD and HEAD...REVERT_HEAD are indeed interesting
- ranges, since these commands are about applying or unapplying a single
- (or a few, for cherry-pick) commit(s) on top of HEAD. However, conflicts
- encountered during these operations can indeed be caused by changes
- introduced in preceding commits on both sides of the history.
+ For revert, as we are (most likely) reversing changes from a previous
+ commit, an appropriate range is REVERT_HEAD..HEAD, which is equivalent
+ to REVERT_HEAD...HEAD and to HEAD...REVERT_HEAD, if we keep HEAD and its
+ parents on the left side of the range.
- Adjust the code in prepare_show_merge so it constructs the range
- HEAD...$OTHER for each of OTHER={MERGE_HEAD, CHERRY_PICK_HEAD,
- REVERT_HEAD or REBASE_HEAD}. Note that we try these pseudorefs in order,
- so keep REBASE_HEAD last since the three other operations can be
- performed during a rebase. Note also that in the uncommon case where
- $OTHER and HEAD do not share a common ancestor, this will show the
- complete histories of both sides since their root commits, which is the
- same behaviour as currently happens in that case for HEAD and
- MERGE_HEAD.
+ As such, adjust the code in prepare_show_merge so it constructs the
+ range HEAD...$OTHER for OTHER={MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD
+ or REBASE_HEAD}. Note that we try these pseudorefs in order, so keep
+ REBASE_HEAD last since the three other operations can be performed
+ during a rebase. Note also that in the uncommon case where $OTHER and
+ HEAD do not share a common ancestor, this will show the complete
+ histories of both sides since their root commits, which is the same
+ behaviour as currently happens in that case for HEAD and MERGE_HEAD.
Adjust the documentation of this option accordingly.
- Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
Co-authored-by: Johannes Sixt <j6t@kdbg.org>
- Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
+ Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
[jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
-
- ## Documentation/gitk.txt ##
-@@ Documentation/gitk.txt: linkgit:git-rev-list[1] for a complete list.
-
- --merge::
-
-- After an attempt to merge stops with conflicts, show the commits on
-- the history between two branches (i.e. the HEAD and the MERGE_HEAD)
-- that modify the conflicted files and do not exist on all the heads
-- being merged.
-+ Show commits touching conflicted paths in the range `HEAD...$OTHER`,
-+ where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`,
-+ `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works
-+ when the index has unmerged entries.
-
- --left-right::
-
+ Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
## Documentation/rev-list-options.txt ##
@@ Documentation/rev-list-options.txt: See also linkgit:git-reflog[1].
@@ Documentation/rev-list-options.txt: See also linkgit:git-reflog[1].
--merge::
- After a failed merge, show refs that touch files having a
- conflict and don't exist on all heads to merge.
-+ Show commits touching conflicted paths in the range `HEAD...$OTHER`,
-+ where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`,
++ Show commits touching conflicted paths in the range `HEAD...<other>`,
++ where `<other>` is the first existing pseudoref in `MERGE_HEAD`,
+ `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works
-+ when the index has unmerged entries.
++ when the index has unmerged entries. This option can be used to show
++ relevant commits when resolving conflicts from a 3-way merge.
--boundary::
Output excluded boundary commits. Boundary commits are
@@ revision.c: static void add_pending_commit_list(struct rev_info *revs,
+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ oid, NULL)) {
+ if (is_null_oid(oid))
-+ die("%s is a symbolic ref???", other_head[i]);
++ die(_("%s is a symbolic ref?"), other_head[i]);
+ return other_head[i];
+ }
+
-+ die("--merge without MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD?");
++ die(_("--merge requires one of the pseudorefs MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD"));
+}
+
static void prepare_show_merge(struct rev_info *revs)
@@ revision.c: static void prepare_show_merge(struct rev_info *revs)
- &oid, NULL))
- die("--merge without MERGE_HEAD?");
- if (is_null_oid(&oid))
-- die("MERGE_HEAD is a symbolic ref???");
+- die(_("MERGE_HEAD is a symbolic ref?"));
- other = lookup_commit_or_die(&oid, "MERGE_HEAD");
+ other_name = lookup_other_head(&oid);
+ other = lookup_commit_or_die(&oid, other_name);
^ permalink raw reply
* Software
From: Karl Rickabus @ 2024-02-25 21:34 UTC (permalink / raw)
To: git
With a very satisfactory space invader, may prove controversial pretend for remarkable accuracy and to the facts involved in my trapezoid.
Sent from my iPhone
^ permalink raw reply
* Re: [PATCH 1/3] t-ctype: allow NUL anywhere in the specification string
From: Eric Sunshine @ 2024-02-25 21:02 UTC (permalink / raw)
To: Jeff King
Cc: René Scharfe, git, Phillip Wood, Josh Steadmon, Achu Luma,
Christian Couder
In-Reply-To: <20240225210038.GA3368483@coredump.intra.peff.net>
On Sun, Feb 25, 2024 at 4:00 PM Jeff King <peff@peff.net> wrote:
> On Sun, Feb 25, 2024 at 01:41:30PM -0500, Eric Sunshine wrote:
> > On Sun, Feb 25, 2024 at 1:28 PM René Scharfe <l.s.r@web.de> wrote:
> > > Am 25.02.24 um 19:05 schrieb Eric Sunshine:
> > > > Taking into consideration the commit message warning about string
> > > > constants, would it make sense to update the comment to mention that
> > > > limitation?
> > >
> > > I think the temptation to pass a string pointer is low -- if only
> > > because there aren't any in this file. But adding such a warning
> > > can't hurt, so yeah, why not?
> >
> > The patch just posted[1] by SZEDER reminded me that, on this project,
> > we assume that the compiler is smart enough to replace
> > `strlen("string-literal")` with the constant `15`, so rather than
> > worrying about updating comment to mention the sizeof() limitation,
> > you could perhaps just use `strlen(string)` instead of
> > `sizeof(string)-1`?
>
> That would defeat the advertised purpose that we can handle embedded
> NULs, though. Whereas with sizeof(), I think a literal like "foo\0bar"
> would still have length 8.
True. Sorry for the noise.
^ permalink raw reply
* Re: [PATCH 1/3] t-ctype: allow NUL anywhere in the specification string
From: Jeff King @ 2024-02-25 21:00 UTC (permalink / raw)
To: Eric Sunshine
Cc: René Scharfe, git, Phillip Wood, Josh Steadmon, Achu Luma,
Christian Couder
In-Reply-To: <CAPig+cThAYjf4DU40dY9jCnjL0FsVpYFhcsj9YdvW2Q=c=ZDBg@mail.gmail.com>
On Sun, Feb 25, 2024 at 01:41:30PM -0500, Eric Sunshine wrote:
> On Sun, Feb 25, 2024 at 1:28 PM René Scharfe <l.s.r@web.de> wrote:
> > Am 25.02.24 um 19:05 schrieb Eric Sunshine:
> > > On Sun, Feb 25, 2024 at 6:27 AM René Scharfe <l.s.r@web.de> wrote:
> > >> Getting the string size using sizeof only works in a macro and with a
> > >> string constant, but that's exactly what we have and I don't see it
> > >> changing anytime soon.
> > >>
> > >> /* Macro to test a character type */
> > >> #define TEST_CTYPE_FUNC(func, string) \
> > >
> > > Taking into consideration the commit message warning about string
> > > constants, would it make sense to update the comment to mention that
> > > limitation?
> > >
> > > /* Test a character type. (Only use with string constants.) */
> > > #define TEST_CTYPE_FUNC(func, string) \
> > >> static void test_ctype_##func(void) { \
> > >> for (int i = 0; i < 256; i++) { \
> > >> + int expect = !!memchr(string, i, sizeof(string) - 1); \
> >
> > I think the temptation to pass a string pointer is low -- if only
> > because there aren't any in this file. But adding such a warning
> > can't hurt, so yeah, why not?
>
> The patch just posted[1] by SZEDER reminded me that, on this project,
> we assume that the compiler is smart enough to replace
> `strlen("string-literal")` with the constant `15`, so rather than
> worrying about updating comment to mention the sizeof() limitation,
> you could perhaps just use `strlen(string)` instead of
> `sizeof(string)-1`?
That would defeat the advertised purpose that we can handle embedded
NULs, though. Whereas with sizeof(), I think a literal like "foo\0bar"
would still have length 8.
-Peff
^ permalink raw reply
* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: rsbecker @ 2024-02-25 20:36 UTC (permalink / raw)
To: 'Torsten Bögershausen'; +Cc: git
In-Reply-To: <20240225191954.GA28646@tb-raspi4>
On Sunday, February 25, 2024 2:20 PM, Torsten Bögershausen wrote:
>On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote:
>> On Sunday, February 25, 2024 1:45 PM, I wrote:
>> >To: git@vger.kernel.org
>> >Subject: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
>> >
>> >This appears to be a new issue introduced at 2.44.0. It only occurs
>> >on
>> NonStop ia64
>> >1..9
>[snip]
>>
>> I did find the following calls to write(), one of which might be
involved.
>> write() should not be used directly unless the count is clearly very
small.
>> Xwrite() should be used instead. There are other calls but those are
>> either small or not on platform.
>
>(Probably a typ0: Xwrite() -> xwrite()
>
>But I think that this should be used:
>write_in_full()
My mailer autocorrected, yes, xwrite. write_in_full() would be safe,
although a bit redundant since xwrite() does similar things and is used by
write_in_full(). The question is which call is bad? The cruft stuff is
relatively new and I don't know the code.
>> reftable/writer.c: int n = w->write(w->write_arg, zeroed,
>> w->pending_padding);
>> reftable/writer.c: n = w->write(w->write_arg, data, len);
>> run-command.c: len = write(io->fd, io->u.out.buf,
>> t/helper/test-path-utils.c: if (write(1, buffer,
count)
>> < 0)
>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
>> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
>> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, buf_line->len);
^ permalink raw reply
* Re: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: Torsten Bögershausen @ 2024-02-25 19:19 UTC (permalink / raw)
To: rsbecker; +Cc: git
In-Reply-To: <01be01da681e$0c349090$249db1b0$@nexbridge.com>
On Sun, Feb 25, 2024 at 02:08:35PM -0500, rsbecker@nexbridge.com wrote:
> On Sunday, February 25, 2024 1:45 PM, I wrote:
> >To: git@vger.kernel.org
> >Subject: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
> >
> >This appears to be a new issue introduced at 2.44.0. It only occurs on
> NonStop ia64
> >1..9
[snip]
>
> I did find the following calls to write(), one of which might be involved.
> write() should not be used directly unless the count is clearly very small.
> Xwrite() should be used instead. There are other calls but those are either
> small or not on platform.
(Probably a typ0: Xwrite() -> xwrite()
But I think that this should be used:
write_in_full()
>
> reftable/writer.c: int n = w->write(w->write_arg, zeroed,
> w->pending_padding);
> reftable/writer.c: n = w->write(w->write_arg, data, len);
> run-command.c: len = write(io->fd, io->u.out.buf,
> t/helper/test-path-utils.c: if (write(1, buffer, count)
> < 0)
> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
> t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
> trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, buf_line->len);
>
>
>
>
^ permalink raw reply
* RE: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: rsbecker @ 2024-02-25 19:08 UTC (permalink / raw)
To: rsbecker, git
In-Reply-To: <01bd01da681a$b8d70a70$2a851f50$@nexbridge.com>
On Sunday, February 25, 2024 1:45 PM, I wrote:
>To: git@vger.kernel.org
>Subject: [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
>
>This appears to be a new issue introduced at 2.44.0. It only occurs on
NonStop ia64
>but not on x86. I am not sure why this is happening although 1Mb exceeds
the
>single I/O size on this machine.
>
>expecting success of 7704.9 '--max-cruft-size with pruning':
> git init max-cruft-size-prune &&
> (
> cd max-cruft-size-prune &&
>
> test_commit base &&
> foo="$(generate_random_blob foo $((1024*1024)))" &&
> bar="$(generate_random_blob bar $((1024*1024)))" &&
> baz="$(generate_random_blob baz $((1024*1024)))" &&
>
> test-tool chmtime -10000 "$objdir/$(test_oid_to_path
"$foo")" &&
>
> git repack -d --cruft --max-cruft-size=1M &&
>
> # backdate the mtimes of all cruft packs to validate
> # that they were rewritten as a result of pruning
> ls $packdir/pack-*.mtimes | sort >cruft.before &&
> for cruft in $(cat cruft.before)
> do
> mtime="$(test-tool chmtime --get -10000 "$cruft")"
>&&
> echo $cruft $mtime >>mtimes || return 1
> done &&
>
> # repack (and prune) with a --max-cruft-size to ensure
> # that we appropriately split the resulting set of packs
> git repack -d --cruft --max-cruft-size=1M \
> --cruft-expiration=10.seconds.ago &&
> ls $packdir/pack-*.mtimes | sort >cruft.after &&
>
> for cruft in $(cat cruft.after)
> do
> old_mtime="$(grep $cruft mtimes | cut -d" " -f2)"
&&
> new_mtime="$(test-tool chmtime --get $cruft)" &&
> test $old_mtime -lt $new_mtime || return 1
> done &&
>
> test_line_count = 3 cruft.before &&
> test_line_count = 2 cruft.after &&
> test_must_fail git cat-file -e $foo &&
> git cat-file -e $bar &&
> git cat-file -e $baz
> )
>
>Initialized empty Git repository in
>/home/ituglib/randall/jenkins/.jenkins/workspace/Git_Pipeline/t/trash
>directory.t7704-repack-cruft/max-cruft-size-prune/.git/
>[master (root-commit) d1ff1c9] base
> Author: A U Thor mailto:author@example.com
> 1 file changed, 1 insertion(+)
> create mode 100644 base.t
>Enumerating objects: 3, done.
>Counting objects: 100% (3/3), done.
>Writing objects: 100% (3/3), done.
>Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) Enumerating
cruft
>objects: 6, done.
>Counting objects: 100% (3/3), done.
>Compressing objects: 100% (3/3), done.
>Writing objects: 100% (3/3), done.
>Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) Enumerating
objects: 3,
>done.
>Counting objects: 100% (3/3), done.
>Writing objects: 100% (3/3), done.
>Total 3 (delta 0), reused 3 (delta 0), pack-reused 0 (from 0)
>ls: cannot access '.git/objects/pack/pack-*.mtimes': No such file or
directory
>test_line_count: line count for cruft.after != 2 not ok 9 -
--max-cruft-size with
>pruning #
># git init max-cruft-size-prune &&
># (
># cd max-cruft-size-prune &&
>#
># test_commit base &&
># foo="$(generate_random_blob foo $((1024*1024)))" &&
># bar="$(generate_random_blob bar $((1024*1024)))" &&
># baz="$(generate_random_blob baz $((1024*1024)))" &&
>#
># test-tool chmtime -10000
"$objdir/$(test_oid_to_path
>"$foo")" &&
>#
># git repack -d --cruft --max-cruft-size=1M &&
>#
># # backdate the mtimes of all cruft packs to
validate
># # that they were rewritten as a result of pruning
># ls $packdir/pack-*.mtimes | sort >cruft.before &&
># for cruft in $(cat cruft.before)
># do
># mtime="$(test-tool chmtime --get -10000
>"$cruft")" &&
># echo $cruft $mtime >>mtimes || return 1
># done &&
>#
># # repack (and prune) with a --max-cruft-size to
>ensure
># # that we appropriately split the resulting set of
>packs
># git repack -d --cruft --max-cruft-size=1M \
># --cruft-expiration=10.seconds.ago &&
># ls $packdir/pack-*.mtimes | sort >cruft.after &&
>#
># for cruft in $(cat cruft.after)
># do
># old_mtime="$(grep $cruft mtimes | cut -d" "
>-f2)" &&
># new_mtime="$(test-tool chmtime --get
>$cruft)" &&
># test $old_mtime -lt $new_mtime || return 1
># done &&
>#
># test_line_count = 3 cruft.before &&
># test_line_count = 2 cruft.after &&
># test_must_fail git cat-file -e $foo &&
># git cat-file -e $bar &&
># git cat-file -e $baz
># )
>#
>1..9
I did find the following calls to write(), one of which might be involved.
write() should not be used directly unless the count is clearly very small.
Xwrite() should be used instead. There are other calls but those are either
small or not on platform.
reftable/writer.c: int n = w->write(w->write_arg, zeroed,
w->pending_padding);
reftable/writer.c: n = w->write(w->write_arg, data, len);
run-command.c: len = write(io->fd, io->u.out.buf,
t/helper/test-path-utils.c: if (write(1, buffer, count)
< 0)
t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
t/helper/test-windows-named-pipe.c: write(1, buf, nbr);
trace2/tr2_dst.c: bytes = write(fd, buf_line->buf, buf_line->len);
^ permalink raw reply
* [BUG] 2.44.0 t7704.9 Fails on NonStop ia64
From: rsbecker @ 2024-02-25 18:44 UTC (permalink / raw)
To: git
This appears to be a new issue introduced at 2.44.0. It only occurs on
NonStop ia64 but not on x86. I am not sure why this is happening although
1Mb exceeds the single I/O size on this machine.
expecting success of 7704.9 '--max-cruft-size with pruning':
git init max-cruft-size-prune &&
(
cd max-cruft-size-prune &&
test_commit base &&
foo="$(generate_random_blob foo $((1024*1024)))" &&
bar="$(generate_random_blob bar $((1024*1024)))" &&
baz="$(generate_random_blob baz $((1024*1024)))" &&
test-tool chmtime -10000 "$objdir/$(test_oid_to_path
"$foo")" &&
git repack -d --cruft --max-cruft-size=1M &&
# backdate the mtimes of all cruft packs to validate
# that they were rewritten as a result of pruning
ls $packdir/pack-*.mtimes | sort >cruft.before &&
for cruft in $(cat cruft.before)
do
mtime="$(test-tool chmtime --get -10000 "$cruft")"
&&
echo $cruft $mtime >>mtimes || return 1
done &&
# repack (and prune) with a --max-cruft-size to ensure
# that we appropriately split the resulting set of packs
git repack -d --cruft --max-cruft-size=1M \
--cruft-expiration=10.seconds.ago &&
ls $packdir/pack-*.mtimes | sort >cruft.after &&
for cruft in $(cat cruft.after)
do
old_mtime="$(grep $cruft mtimes | cut -d" " -f2)" &&
new_mtime="$(test-tool chmtime --get $cruft)" &&
test $old_mtime -lt $new_mtime || return 1
done &&
test_line_count = 3 cruft.before &&
test_line_count = 2 cruft.after &&
test_must_fail git cat-file -e $foo &&
git cat-file -e $bar &&
git cat-file -e $baz
)
Initialized empty Git repository in
/home/ituglib/randall/jenkins/.jenkins/workspace/Git_Pipeline/t/trash
directory.t7704-repack-cruft/max-cruft-size-prune/.git/
[master (root-commit) d1ff1c9] base
Author: A U Thor mailto:author@example.com
1 file changed, 1 insertion(+)
create mode 100644 base.t
Enumerating objects: 3, done.
Counting objects: 100% (3/3), done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
Enumerating cruft objects: 6, done.
Counting objects: 100% (3/3), done.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
Enumerating objects: 3, done.
Counting objects: 100% (3/3), done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 3 (delta 0), pack-reused 0 (from 0)
ls: cannot access '.git/objects/pack/pack-*.mtimes': No such file or
directory
test_line_count: line count for cruft.after != 2
not ok 9 - --max-cruft-size with pruning
#
# git init max-cruft-size-prune &&
# (
# cd max-cruft-size-prune &&
#
# test_commit base &&
# foo="$(generate_random_blob foo $((1024*1024)))" &&
# bar="$(generate_random_blob bar $((1024*1024)))" &&
# baz="$(generate_random_blob baz $((1024*1024)))" &&
#
# test-tool chmtime -10000 "$objdir/$(test_oid_to_path
"$foo")" &&
#
# git repack -d --cruft --max-cruft-size=1M &&
#
# # backdate the mtimes of all cruft packs to validate
# # that they were rewritten as a result of pruning
# ls $packdir/pack-*.mtimes | sort >cruft.before &&
# for cruft in $(cat cruft.before)
# do
# mtime="$(test-tool chmtime --get -10000
"$cruft")" &&
# echo $cruft $mtime >>mtimes || return 1
# done &&
#
# # repack (and prune) with a --max-cruft-size to
ensure
# # that we appropriately split the resulting set of
packs
# git repack -d --cruft --max-cruft-size=1M \
# --cruft-expiration=10.seconds.ago &&
# ls $packdir/pack-*.mtimes | sort >cruft.after &&
#
# for cruft in $(cat cruft.after)
# do
# old_mtime="$(grep $cruft mtimes | cut -d" "
-f2)" &&
# new_mtime="$(test-tool chmtime --get
$cruft)" &&
# test $old_mtime -lt $new_mtime || return 1
# done &&
#
# test_line_count = 3 cruft.before &&
# test_line_count = 2 cruft.after &&
# test_must_fail git cat-file -e $foo &&
# git cat-file -e $bar &&
# git cat-file -e $baz
# )
#
1..9
--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.
^ permalink raw reply
* Re: [PATCH 1/3] t-ctype: allow NUL anywhere in the specification string
From: Eric Sunshine @ 2024-02-25 18:41 UTC (permalink / raw)
To: René Scharfe
Cc: git, Phillip Wood, Josh Steadmon, Achu Luma, Christian Couder
In-Reply-To: <5dea0535-4b21-483a-a10a-bfa8e9e85650@web.de>
On Sun, Feb 25, 2024 at 1:28 PM René Scharfe <l.s.r@web.de> wrote:
> Am 25.02.24 um 19:05 schrieb Eric Sunshine:
> > On Sun, Feb 25, 2024 at 6:27 AM René Scharfe <l.s.r@web.de> wrote:
> >> Getting the string size using sizeof only works in a macro and with a
> >> string constant, but that's exactly what we have and I don't see it
> >> changing anytime soon.
> >>
> >> /* Macro to test a character type */
> >> #define TEST_CTYPE_FUNC(func, string) \
> >
> > Taking into consideration the commit message warning about string
> > constants, would it make sense to update the comment to mention that
> > limitation?
> >
> > /* Test a character type. (Only use with string constants.) */
> > #define TEST_CTYPE_FUNC(func, string) \
> >> static void test_ctype_##func(void) { \
> >> for (int i = 0; i < 256; i++) { \
> >> + int expect = !!memchr(string, i, sizeof(string) - 1); \
>
> I think the temptation to pass a string pointer is low -- if only
> because there aren't any in this file. But adding such a warning
> can't hurt, so yeah, why not?
The patch just posted[1] by SZEDER reminded me that, on this project,
we assume that the compiler is smart enough to replace
`strlen("string-literal")` with the constant `15`, so rather than
worrying about updating comment to mention the sizeof() limitation,
you could perhaps just use `strlen(string)` instead of
`sizeof(string)-1`?
[1]: https://lore.kernel.org/git/20240225183452.1939334-1-szeder.dev@gmail.com/
^ permalink raw reply
* [PATCH] upload-pack: don't send null character in abort message to the client
From: SZEDER Gábor @ 2024-02-25 18:34 UTC (permalink / raw)
To: git; +Cc: SZEDER Gábor
Since 583b7ea31b (upload-pack/fetch-pack: support side-band
communication, 2006-06-21) the abort message sent by upload-pack in
case of possible repository corruption ends with a null character.
This can be seen in several test cases in 't5530-upload-pack-error.sh'
where 'grep <pattern> output.err' often reports "Binary file
output.err matches" because of that null character.
The reason for this is that the abort message is defined as a string
literal, and we pass its size to the send function as
sizeof(abort_msg), which also counts the terminating null character.
Use strlen() instead to avoid sending that terminating null character.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
upload-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/upload-pack.c b/upload-pack.c
index 2537affa90..6e0d441ef5 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -463,7 +463,7 @@ static void create_pack_file(struct upload_pack_data *pack_data,
fail:
free(output_state);
- send_client_data(3, abort_msg, sizeof(abort_msg),
+ send_client_data(3, abort_msg, strlen(abort_msg),
pack_data->use_sideband);
die("git upload-pack: %s", abort_msg);
}
--
2.44.0.rc1.366.g26e5fbbdb0
^ permalink raw reply related
* git mv fails to deduplicate blob objects on transfer
From: Milan Hauth @ 2024-02-25 18:34 UTC (permalink / raw)
To: git
"git mv" followed by "git push" or "git pull"
can produce wasteful transfers of blob objects
these transfers are wasteful
because the blob object already exists in the destination repo
but "git push" or "git pull" fail to see that
this affects only some cases of "git mv"
in some cases, the deduplication works as expected
in other cases, dedup fails
this is neglegible for small files, but noticable with large files
in my case, i tried to move 5GB of files (250 x 20MB)
and i was surprised as "git push" wanted to transfer 5GB
instead of a few bytes for the tree and commit objects
to reproduce: see repro.sh in
https://github.com/milahu/git-bug-git-mv-wasteful-transfer
output of repro.sh
the first size is the transfer size before "git mv"
the second size is the transfer size after "git mv"
pass: 1.00 MiB != 288 bytes # path_a=file_a; path_b=file_b
pass: 1.00 MiB != 331 bytes # path_a=dir/file_a; path_b=dir/file_b
pass: 1.00 MiB != 286 bytes # path_a=dir_a/file; path_b=dir_b/file
pass: 1.00 MiB != 284 bytes # path_a=file; path_b=dir/file
pass: 1.00 MiB != 329 bytes # path_a=file; path_b=dir1/dir2/file
pass: 1.00 MiB != 373 bytes # path_a=file; path_b=dir1/dir2/dir3/file
pass: 1.00 MiB != 331 bytes # path_a=file_a; path_b=dir/file_b
pass: 1.00 MiB != 376 bytes # path_a=file_a; path_b=dir1/dir2/file_b
pass: 1.00 MiB != 420 bytes # path_a=file_a; path_b=dir1/dir2/dir3/file_b
pass: 1.00 MiB != 241 bytes # path_a=dir/file; path_b=file
FAIL: 1.00 MiB == 1.00 MiB # path_a=dir1/dir2/file; path_b=file
FAIL: 1.00 MiB == 1.00 MiB # path_a=dir1/dir2/dir3/file; path_b=file
FAIL: 1.00 MiB == 1.00 MiB # path_a=dir/file_a; path_b=file_b
FAIL: 1.00 MiB == 1.00 MiB # path_a=dir1/dir2/file_a; path_b=file_b
FAIL: 1.00 MiB == 1.00 MiB # path_a=dir1/dir2/dir3/file_a; path_b=file_b
FAIL: 1.00 MiB == 1.00 MiB # path_a=dir1a/dir2a/file; path_b=dir1b/dir2b/file
FAIL: 1.00 MiB == 1.00 MiB # path_a=dir1a/file_a; path_b=dir1b/file_b
see also
https://colabti.org/ircloggy/git/2024-02-24#l704
https://colabti.org/ircloggy/git/2024-02-25#l218
https://colabti.org/ircloggy/git/2024-02-25#l269
> I have a strong déjà vu about this also; I think we talked about this exact thing a while ago
> this, same: https://colabti.org/ircloggy/git/2023-09-13#l912
https://colabti.org/ircloggy/git/2024-02-25#l404
https://colabti.org/ircloggy/git/2024-02-25#l433
> reading that SO answer by jthill but can't quite get the whole picture from it -- is it saying that it's a trade-off in sending all the objects vs. spending resources trying to figure out what to send?
> I bet there's an opportunity for optimization here; Git could probably figure out a good balance based on how much data it is about to send
https://stackoverflow.com/questions/48228425/git-push-new-branch-with-same-files-uploads-all-files-again
^ permalink raw reply
* Re: [PATCH 1/3] t-ctype: allow NUL anywhere in the specification string
From: René Scharfe @ 2024-02-25 18:28 UTC (permalink / raw)
To: Eric Sunshine
Cc: git, Phillip Wood, Josh Steadmon, Achu Luma, Christian Couder
In-Reply-To: <CAPig+cTvs7Ckg=k1T+N3N=yT840xM2DYf3LV9NZhii8+c1nkbw@mail.gmail.com>
Am 25.02.24 um 19:05 schrieb Eric Sunshine:
> On Sun, Feb 25, 2024 at 6:27 AM René Scharfe <l.s.r@web.de> wrote:
>> Replace the custom function is_in() for looking up a character in the
>> specification string with memchr(3) and sizeof. This is shorter,
>> simpler and allows NUL anywhere in the string, which may come in handy
>> if we ever want to support more character classes that contain it.
>>
>> Getting the string size using sizeof only works in a macro and with a
>> string constant, but that's exactly what we have and I don't see it
>> changing anytime soon.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
>> @@ -1,23 +1,11 @@
>> /* Macro to test a character type */
>> #define TEST_CTYPE_FUNC(func, string) \
>
> Taking into consideration the commit message warning about string
> constants, would it make sense to update the comment to mention that
> limitation?
I think the temptation to pass a string pointer is low -- if only
because there aren't any in this file. But adding such a warning
can't hurt, so yeah, why not?
>
> /* Test a character type. (Only use with string constants.) */
> #define TEST_CTYPE_FUNC(func, string) \
>
>> static void test_ctype_##func(void) { \
>> for (int i = 0; i < 256; i++) { \
>> - if (!check_int(func(i), ==, is_in(string, i))) \
>> + int expect = !!memchr(string, i, sizeof(string) - 1); \
>> + if (!check_int(func(i), ==, expect)) \
>> test_msg(" i: 0x%02x", i); \
>> } \
>> if (!check(!func(EOF))) \
^ 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