* Re: first-class conflicts?
From: Dragan Simic @ 2023-11-07 8:21 UTC (permalink / raw)
To: Elijah Newren; +Cc: Sandra Snan, git
In-Reply-To: <CABPp-BH7WBm1j-Ue9oZFjoy6sTcw5B0hz_ndDEtJqvpZF4YF=w@mail.gmail.com>
On 2023-11-07 09:16, Elijah Newren wrote:
> But we'd also have to be careful and think through usecases, including
> in the surrounding community. People would probably want to ensure
> that e.g. "Protected" or "Integration" branches don't get accept
> fetches or pushes of conflicted commits, git status would probably
> need some special warnings or notices, git checkout would probably
> benefit from additional warnings/notices checks for those cases, git
> log should probably display conflicted commits differently, we'd need
> to add special handling for higher order conflicts (e.g. a merge with
> conflicts is itself involved in a merge) probably similar to what jj
> has done, and audit a lot of other code paths to see what would be
> needed.
That would be a truly _massive_ project.
^ permalink raw reply
* Re: first-class conflicts?
From: Sandra Snan @ 2023-11-07 9:16 UTC (permalink / raw)
To: git
In-Reply-To: <CABPp-BH7WBm1j-Ue9oZFjoy6sTcw5B0hz_ndDEtJqvpZF4YF=w@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> Martin talked about this and other features at Git Merge 2022, a
> little over a year ago.
That is something I should've checked or searched for before
starting this thread, in hindsight. Thank you, Elijah, for letting
me know that.
> And if we did, it'd solve various issues such as people wanting
> to be able to stash conflicts, or wanting to be able to
> partially resolve conflicts and fix it up later, or be able to
> collaboratively resolve conflicts without having everyone have
> access to the same checkout.
One feature I would really like and maybe vanilla git can already
do this today and I just don't know how, but just becoming more
aware of conflicts, of when there's a conflict in the commit.
> git status would probably need some special warnings or notices,
> git checkout would probably benefit from additional
> warnings/notices checks for those cases, git log should probably
> display conflicted commits differently
That's exactly what I dream of! I wouldn't wanna commit conflicts
deliberately, just that I'm paranoid that I might have some failed
merges and three way diffs in code that I missed when they flashed
by on the screen.
> it'd also be a lot of work
That is for sure. And don't get me wrong, it's not a feature I
personally really need or am clamoring for. Thank you so much for the
thoughtful explanation.
^ permalink raw reply
* Re: [PATCH 0/2] revision: exclude all packed objects with `--unpacked`
From: Patrick Steinhardt @ 2023-11-07 9:43 UTC (permalink / raw)
To: Jeff King; +Cc: Taylor Blau, git, Junio C Hamano
In-Reply-To: <20231107040235.GD873619@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]
On Mon, Nov 06, 2023 at 11:02:35PM -0500, Jeff King wrote:
> On Mon, Nov 06, 2023 at 05:56:27PM -0500, Taylor Blau wrote:
>
> > While working on my longer series to enable verbatim pack reuse across
> > multiple packs[^1], I noticed a couple of oddities with the `--unpacked`
> > rev-walk flag.
> >
> > While it does exclude packed commits, it does not exclude (all) packed
> > trees/blobs/annotated tags. This problem exists in the pack-bitmap
> > machinery, too, which will over-count queries like:
> >
> > $ git rev-list --use-bitmap-index --all --unpacked --objects
> >
> > , etc.
> >
> > The fix is relatively straightforward, split across two patches that
> > Peff and I worked on together earlier today.
>
> I'm not sure my review is worth anything, but this looks good to me. ;)
> I do think it might be worth tightening up the docs as Junio suggested,
> but I would be fine to see that as a patch on top.
>
> -Peff
I also read through the patches and agree, this looks good to me.
Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 00/14] Introduce new `git replay` command
From: Christian Couder @ 2023-11-07 9:43 UTC (permalink / raw)
To: Elijah Newren
Cc: git, Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
John Cai, Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <CABPp-BE6G2qaF50bhz-CwxSsvxGDHzwvsWtfQO4zVcX6ERppLw@mail.gmail.com>
Hi,
On Tue, Nov 7, 2023 at 3:44 AM Elijah Newren <newren@gmail.com> wrote:
> Looking good, just one comment on one small hunk...
>
> On Thu, Nov 2, 2023 at 6:52 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> [...]
>
> > @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> > -
> > strvec_pushl(&rev_walk_args, "", argv[2], "--not", argv[1], NULL);
> >
> > ++ /*
> > ++ * TODO: For now, let's warn when we see an option that we are
> > ++ * going to override after setup_revisions() below. In the
> > ++ * future we might want to either die() or allow them if we
> > ++ * think they could be useful though.
> > ++ */
> > ++ for (i = 0; i < argc; i++) {
> > ++ if (!strcmp(argv[i], "--reverse") || !strcmp(argv[i], "--date-order") ||
> > ++ !strcmp(argv[i], "--topo-order") || !strcmp(argv[i], "--author-date-order") ||
> > ++ !strcmp(argv[i], "--full-history"))
> > ++ warning(_("option '%s' will be overridden"), argv[i]);
> > ++ }
> > ++
>
> Two things:
>
> 1) Not sure it makes sense to throw a warning with --topo-order or
> --full-history, since they would result in a value matching what we
> would be setting anyway.
Yeah, I am not sure about this either. About "--reverse" I think it
makes sense because even if the command is setting the reverse bit, it
would be possible to reverse the reverse like Dscho wanted. But I
agree "--topo-order" and "--full-history" will very unlikely be reused
for anything else in the future.
> 2) This seems like an inefficient way to provide this warning; could
> we avoid parsing the arguments for an extra time? Perhaps instead
> a) set the desired values here, before setup_revisions()
> b) after setup_revisions, check whether these values differ from the
> desired values, if so throw a warning.
> c) set the desired values, again
Yeah, that would work. The downside is that it would be more difficult
in the warning to tell the user which command line option was
overridden as there are some values changed by different options.
Maybe we can come up with a warning message that is still useful and
enough for now, as the command is supposed to be used by experienced
users. Perhaps something like:
warning(_("some rev walking options will be overridden as '%s' bit in
'struct rev_info' will be forced"), "sort_order");
Thanks!
^ permalink raw reply
* Re: [PATCH 0/9] for-each-ref optimizations & usability improvements
From: Patrick Steinhardt @ 2023-11-07 10:49 UTC (permalink / raw)
To: Victoria Dye; +Cc: Junio C Hamano, Victoria Dye via GitGitGadget, git
In-Reply-To: <dbcbcf0e-aeee-4bb9-9e39-e2e85194d083@github.com>
[-- Attachment #1: Type: text/plain, Size: 1790 bytes --]
On Mon, Nov 06, 2023 at 06:48:29PM -0800, Victoria Dye wrote:
> Junio C Hamano wrote:
> > "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
[snip]
> >> * I'm not attached to '--full-deref' as a name - if someone has an idea for
> >> a more descriptive name, please suggest it!
> >
> > Another candidate verb may be "to peel", and I have no strong
> > opinion between it and "to dereference". But I have a mild aversion
> > to an abbreviation that is not strongly established.
> >
>
> Makes sense. I got the "deref" abbreviation for 'update-ref --no-deref', but
> 'show-ref' has a "--dereference" option and protocol v2's "ls-refs" includes
> a "peel" arg. "Dereference" is the term already used in the 'for-each-ref'
> documentation, though, so if no one comes in with an especially strong
> opinion on this I'll change the option to '--full-dereference'. Thanks!
But doesn't dereferencing in the context of git-update-ref(1) refer to
something different? It's not about tags, but it is about symbolic
references and whether we want to update the symref or the pointee. But
true enough, in git-show-ref(1) "dereference" actually means that we
should peel the tag.
To me it feels like preexisting commands are confused already. In my
mind model:
- "peel" means that an object gets resolved to one of its pointees.
This also includes the case here, where a tag gets peeled to its
pointee.
- "dereference" means that a symbolic reference gets resolved to its
pointee. This matches what we do in `git update-ref --no-deref`.
But after reading through the code I don't think we distinguish those
terms cleanly throughout our codebase. Still, "peeling" feels like a
better match in my opinion.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/9] ref-filter.c: really don't sort when using --no-sort
From: Patrick Steinhardt @ 2023-11-07 10:49 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <dea8d7d1e866d9784320051b372ff729fca855d7.1699320362.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 13178 bytes --]
On Tue, Nov 07, 2023 at 01:25:53AM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
>
> Update 'ref_sorting_options()' to return a NULL 'struct ref_sorting *' if
> the string list provided to it is empty, rather than returning the default
> refname sort structure. Also update 'ref_array_sort()' to explicitly skip
> sorting if its 'struct ref_sorting *' arg is NULL. Other functions using
> 'struct ref_sorting *' do not need any changes because they already properly
> ignore NULL values.
>
> The goal of this change is to have the '--no-sort' option truly disable
> sorting in commands like 'for-each-ref, 'tag', and 'branch'. Right now,
> '--no-sort' will still trigger refname sorting by default in 'for-each-ref',
> 'tag', and 'branch'.
>
> To match existing behavior as closely as possible, explicitly add "refname"
> to the list of sort keys in 'for-each-ref', 'tag', and 'branch' before
> parsing options (if no config-based sort keys are set). This ensures that
> sorting will only be fully disabled if '--no-sort' is provided as an option;
> otherwise, "refname" sorting will remain the default. Note: this also means
> that even when sort keys are provided on the command line, "refname" will be
> the final sort key in the sorting structure. This doesn't actually change
> any behavior, since 'compare_refs()' already falls back on comparing
> refnames if two refs are equal w.r.t all other sort keys.
>
> Finally, remove the condition around sorting in 'ls-remote', since it's no
> longer necessary. Unlike 'for-each-ref' et. al., it does *not* set any sort
> keys by default. The default empty list of sort keys will produce a NULL
> 'struct ref_sorting *', which causes the sorting to be skipped in
> 'ref_array_sort()'.
I found the order in this commit message a bit funny because you first
explain what you're doing, then explain the goal, and then jump into the
changes again. The message might be a bit easier to read if the goal was
stated up front.
I was also briefly wondering whether it would make sense to split up
this commit, as you're doing two different things:
- Refactor how git-for-each-ref(1), git-tag(1) and git-branch(1) set
up their default sorting.
- Change `ref_array_sort()` to not sort when its sorting option is
`NULL`.
If this was split up into two commits, then the result might be a bit
easier to reason about. But I don't feel strongly about this.
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> builtin/branch.c | 6 ++++
> builtin/for-each-ref.c | 3 ++
> builtin/ls-remote.c | 10 ++----
> builtin/tag.c | 6 ++++
> ref-filter.c | 19 ++----------
> t/t3200-branch.sh | 68 +++++++++++++++++++++++++++++++++++++++--
> t/t6300-for-each-ref.sh | 21 +++++++++++++
> t/t7004-tag.sh | 45 +++++++++++++++++++++++++++
> 8 files changed, 152 insertions(+), 26 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index e7ee9bd0f15..d67738bbcaa 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -767,7 +767,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage_with_options(builtin_branch_usage, options);
>
> + /*
> + * Try to set sort keys from config. If config does not set any,
> + * fall back on default (refname) sorting.
> + */
> git_config(git_branch_config, &sorting_options);
> + if (!sorting_options.nr)
> + string_list_append(&sorting_options, "refname");
>
> track = git_branch_track;
>
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 350bfa6e811..93b370f550b 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -67,6 +67,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>
> git_config(git_default_config, NULL);
>
> + /* Set default (refname) sorting */
> + string_list_append(&sorting_options, "refname");
> +
> parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);
> if (maxcount < 0) {
> error("invalid --count argument: `%d'", maxcount);
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index fc765754305..436249b720c 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -58,6 +58,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
> struct transport *transport;
> const struct ref *ref;
> struct ref_array ref_array;
> + struct ref_sorting *sorting;
> struct string_list sorting_options = STRING_LIST_INIT_DUP;
>
> struct option options[] = {
> @@ -141,13 +142,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
> item->symref = xstrdup_or_null(ref->symref);
> }
>
> - if (sorting_options.nr) {
> - struct ref_sorting *sorting;
> -
> - sorting = ref_sorting_options(&sorting_options);
> - ref_array_sort(sorting, &ref_array);
> - ref_sorting_release(sorting);
> - }
> + sorting = ref_sorting_options(&sorting_options);
> + ref_array_sort(sorting, &ref_array);
We stopped calling `ref_sorting_release()`. Doesn't that cause us to
leak memory?
> for (i = 0; i < ref_array.nr; i++) {
> const struct ref_array_item *ref = ref_array.items[i];
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 3918eacbb57..64f3196cd4c 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -501,7 +501,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>
> setup_ref_filter_porcelain_msg();
>
> + /*
> + * Try to set sort keys from config. If config does not set any,
> + * fall back on default (refname) sorting.
> + */
> git_config(git_tag_config, &sorting_options);
> + if (!sorting_options.nr)
> + string_list_append(&sorting_options, "refname");
>
> memset(&opt, 0, sizeof(opt));
> filter.lines = -1;
> diff --git a/ref-filter.c b/ref-filter.c
> index e4d3510e28e..7250089b7c6 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -3142,7 +3142,8 @@ void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting,
>
> void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
> {
> - QSORT_S(array->items, array->nr, compare_refs, sorting);
> + if (sorting)
> + QSORT_S(array->items, array->nr, compare_refs, sorting);
> }
>
> static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)
> @@ -3248,18 +3249,6 @@ static int parse_sorting_atom(const char *atom)
> return res;
> }
>
> -/* If no sorting option is given, use refname to sort as default */
> -static struct ref_sorting *ref_default_sorting(void)
> -{
> - static const char cstr_name[] = "refname";
> -
> - struct ref_sorting *sorting = xcalloc(1, sizeof(*sorting));
> -
> - sorting->next = NULL;
> - sorting->atom = parse_sorting_atom(cstr_name);
> - return sorting;
> -}
> -
> static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
> {
> struct ref_sorting *s;
> @@ -3283,9 +3272,7 @@ struct ref_sorting *ref_sorting_options(struct string_list *options)
> struct string_list_item *item;
> struct ref_sorting *sorting = NULL, **tail = &sorting;
>
> - if (!options->nr) {
> - sorting = ref_default_sorting();
> - } else {
> + if (options->nr) {
> for_each_string_list_item(item, options)
> parse_ref_sorting(tail, item->string);
> }
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 3182abde27f..9918ba05dec 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -1570,9 +1570,10 @@ test_expect_success 'tracking with unexpected .fetch refspec' '
>
> test_expect_success 'configured committerdate sort' '
> git init -b main sort &&
> + test_config -C sort branch.sort "committerdate" &&
> +
> (
> cd sort &&
> - git config branch.sort committerdate &&
> test_commit initial &&
> git checkout -b a &&
> test_commit a &&
> @@ -1592,9 +1593,10 @@ test_expect_success 'configured committerdate sort' '
> '
>
> test_expect_success 'option override configured sort' '
> + test_config -C sort branch.sort "committerdate" &&
> +
> (
> cd sort &&
> - git config branch.sort committerdate &&
> git branch --sort=refname >actual &&
> cat >expect <<-\EOF &&
> a
> @@ -1606,10 +1608,70 @@ test_expect_success 'option override configured sort' '
> )
> '
>
> +test_expect_success '--no-sort cancels config sort keys' '
> + test_config -C sort branch.sort "-refname" &&
> +
> + (
> + cd sort &&
> +
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
> + git branch \
> + --no-sort \
> + --sort="objecttype" >actual &&
This test is a bit confusing to me. Shouldn't we in fact ignore the
configured sorting order as soon as we pass `--sort=` anyway? In other
words, I would expect the `--no-sort` option to not make a difference
here. What should make a difference is if you _only_ passed `--no-sort`.
> + cat >expect <<-\EOF &&
> + a
> + * b
> + c
> + main
> + EOF
> + test_cmp expect actual
> + )
> +
> +'
> +
> +test_expect_success '--no-sort cancels command line sort keys' '
> + (
> + cd sort &&
> +
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
> + git branch \
> + --sort="-refname" \
> + --no-sort \
> + --sort="objecttype" >actual &&
> + cat >expect <<-\EOF &&
> + a
> + * b
> + c
> + main
> + EOF
> + test_cmp expect actual
> + )
> +'
> +
> +test_expect_success '--no-sort without subsequent --sort prints expected branches' '
> + (
> + cd sort &&
> +
> + # Sort the results with `sort` for a consistent comparison
> + # against expected
> + git branch --no-sort | sort >actual &&
> + cat >expect <<-\EOF &&
> + a
> + c
> + main
> + * b
> + EOF
> + test_cmp expect actual
> + )
> +'
> +
> test_expect_success 'invalid sort parameter in configuration' '
> + test_config -C sort branch.sort "v:notvalid" &&
> +
> (
> cd sort &&
> - git config branch.sort "v:notvalid" &&
>
> # this works in the "listing" mode, so bad sort key
> # is a dying offence.
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 00a060df0b5..0613e5e3623 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -1335,6 +1335,27 @@ test_expect_success '--no-sort cancels the previous sort keys' '
> test_cmp expected actual
> '
>
> +test_expect_success '--no-sort without subsequent --sort prints expected refs' '
> + cat >expected <<-\EOF &&
> + refs/tags/multi-ref1-100000-user1
> + refs/tags/multi-ref1-100000-user2
> + refs/tags/multi-ref1-200000-user1
> + refs/tags/multi-ref1-200000-user2
> + refs/tags/multi-ref2-100000-user1
> + refs/tags/multi-ref2-100000-user2
> + refs/tags/multi-ref2-200000-user1
> + refs/tags/multi-ref2-200000-user2
> + EOF
> +
> + # Sort the results with `sort` for a consistent comparison against
> + # expected
> + git for-each-ref \
> + --format="%(refname)" \
> + --no-sort \
> + "refs/tags/multi-*" | sort >actual &&
> + test_cmp expected actual
> +'
> +
> test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
> test_when_finished "git checkout main" &&
> git for-each-ref --format="%(HEAD) %(refname:short)" refs/heads/ >actual &&
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index e689db42929..b41a47eb943 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1862,6 +1862,51 @@ test_expect_success 'option override configured sort' '
> test_cmp expect actual
> '
>
> +test_expect_success '--no-sort cancels config sort keys' '
> + test_config tag.sort "-refname" &&
> +
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
> + git tag -l \
> + --no-sort \
> + --sort="objecttype" \
> + "foo*" >actual &&
> + cat >expect <<-\EOF &&
> + foo1.10
> + foo1.3
> + foo1.6
> + EOF
> + test_cmp expect actual
> +'
Same question here.
Patrick
> +test_expect_success '--no-sort cancels command line sort keys' '
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
> + git tag -l \
> + --sort="-refname" \
> + --no-sort \
> + --sort="objecttype" \
> + "foo*" >actual &&
> + cat >expect <<-\EOF &&
> + foo1.10
> + foo1.3
> + foo1.6
> + EOF
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--no-sort without subsequent --sort prints expected tags' '
> + # Sort the results with `sort` for a consistent comparison against
> + # expected
> + git tag -l --no-sort "foo*" | sort >actual &&
> + cat >expect <<-\EOF &&
> + foo1.10
> + foo1.3
> + foo1.6
> + EOF
> + test_cmp expect actual
> +'
> +
> test_expect_success 'invalid sort parameter on command line' '
> test_must_fail git tag -l --sort=notvalid "foo*" >actual
> '
> --
> gitgitgadget
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/9] ref-filter.h: move contains caches into filter
From: Patrick Steinhardt @ 2023-11-07 10:49 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <6c66445ee31dd4117e1384d8da7be81f401317b3.1699320362.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3709 bytes --]
On Tue, Nov 07, 2023 at 01:25:56AM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
>
> Move the 'contains_cache' and 'no_contains_cache' used in filter_refs into
> an 'internal' struct of the 'struct ref_filter'. In later patches, the
> 'struct ref_filter *' will be a common data structure across multiple
> filtering functions. These caches are part of the common functionality the
> filter struct will support, so they are updated to be internally accessible
> wherever the filter is used.
>
> The design used here is mirrors what was introduced in 576de3d956
Nit: s/is //
Patrick
> (unpack_trees: start splitting internal fields from public API, 2023-02-27)
> for 'unpack_trees_options'.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> ref-filter.c | 14 ++++++--------
> ref-filter.h | 6 ++++++
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 7250089b7c6..5129b6986c9 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2764,8 +2764,6 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
> struct ref_filter_cbdata {
> struct ref_array *array;
> struct ref_filter *filter;
> - struct contains_cache contains_cache;
> - struct contains_cache no_contains_cache;
> };
>
> /*
> @@ -2816,11 +2814,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
> return 0;
> /* We perform the filtering for the '--contains' option... */
> if (filter->with_commit &&
> - !commit_contains(filter, commit, filter->with_commit, &ref_cbdata->contains_cache))
> + !commit_contains(filter, commit, filter->with_commit, &filter->internal.contains_cache))
> return 0;
> /* ...or for the `--no-contains' option */
> if (filter->no_commit &&
> - commit_contains(filter, commit, filter->no_commit, &ref_cbdata->no_contains_cache))
> + commit_contains(filter, commit, filter->no_commit, &filter->internal.no_contains_cache))
> return 0;
> }
>
> @@ -2989,8 +2987,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> save_commit_buffer_orig = save_commit_buffer;
> save_commit_buffer = 0;
>
> - init_contains_cache(&ref_cbdata.contains_cache);
> - init_contains_cache(&ref_cbdata.no_contains_cache);
> + init_contains_cache(&filter->internal.contains_cache);
> + init_contains_cache(&filter->internal.no_contains_cache);
>
> /* Simple per-ref filtering */
> if (!filter->kind)
> @@ -3014,8 +3012,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> head_ref(ref_filter_handler, &ref_cbdata);
> }
>
> - clear_contains_cache(&ref_cbdata.contains_cache);
> - clear_contains_cache(&ref_cbdata.no_contains_cache);
> + clear_contains_cache(&filter->internal.contains_cache);
> + clear_contains_cache(&filter->internal.no_contains_cache);
>
> /* Filters that need revision walking */
> reach_filter(array, &filter->reachable_from, INCLUDE_REACHED);
> diff --git a/ref-filter.h b/ref-filter.h
> index d87d61238b7..0db3ff52889 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -7,6 +7,7 @@
> #include "commit.h"
> #include "string-list.h"
> #include "strvec.h"
> +#include "commit-reach.h"
>
> /* Quoting styles */
> #define QUOTE_NONE 0
> @@ -75,6 +76,11 @@ struct ref_filter {
> lines;
> int abbrev,
> verbose;
> +
> + struct {
> + struct contains_cache contains_cache;
> + struct contains_cache no_contains_cache;
> + } internal;
> };
>
> struct ref_format {
> --
> gitgitgadget
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 6/9] ref-filter.c: refactor to create common helper functions
From: Patrick Steinhardt @ 2023-11-07 10:49 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <8c77452e5dd8d5cafd95c68480bf5675d51b4736.1699320362.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9785 bytes --]
On Tue, Nov 07, 2023 at 01:25:58AM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
>
> Factor out parts of 'ref_array_push()', 'ref_filter_handler()', and
> 'filter_refs()' into new helper functions ('ref_array_append()',
> 'apply_ref_filter()', and 'do_filter_refs()' respectively), as well as
> rename 'ref_filter_handler()' to 'filter_one()'. In this and later
> patches, these helpers will be used by new ref-filter API functions. This
> patch does not result in any user-facing behavior changes or changes to
> callers outside of 'ref-filter.c'.
>
> The changes are as follows:
>
> * The logic to grow a 'struct ref_array' and append a given 'struct
> ref_array_item *' to it is extracted from 'ref_array_push()' into
> 'ref_array_append()'.
> * 'ref_filter_handler()' is renamed to 'filter_one()' to more clearly
> distinguish it from other ref filtering callbacks that will be added in
> later patches. The "*_one()" naming convention is common throughout the
> codebase for iteration callbacks.
> * The code to filter a given ref by refname & object ID then create a new
> 'struct ref_array_item' is moved out of 'filter_one()' and into
> 'apply_ref_filter()'. 'apply_ref_filter()' returns either NULL (if the ref
> does not match the given filter) or a 'struct ref_array_item *' created
> with 'new_ref_array_item()'; 'filter_one()' appends that item to
> its ref array with 'ref_array_append()'.
> * The filter pre-processing, contains cache creation, and ref iteration of
> 'filter_refs()' is extracted into 'do_filter_refs()'. 'do_filter_refs()'
> takes its ref iterator function & callback data as an input from the
> caller, setting it up to be used with additional filtering callbacks in
> later patches.
To me, a bulleted list spelling out the different changes I'm doing
often indicates that I might want to split up the commit into one for
each of the items. I don't feel strongly about this, but think that it
might help the reviewer in this case.
Patrick
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> ref-filter.c | 115 ++++++++++++++++++++++++++++++---------------------
> 1 file changed, 69 insertions(+), 46 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 8992fbf45b1..ff00ab4b8d8 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2716,15 +2716,18 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
> return ref;
> }
>
> +static void ref_array_append(struct ref_array *array, struct ref_array_item *ref)
> +{
> + ALLOC_GROW(array->items, array->nr + 1, array->alloc);
> + array->items[array->nr++] = ref;
> +}
> +
> struct ref_array_item *ref_array_push(struct ref_array *array,
> const char *refname,
> const struct object_id *oid)
> {
> struct ref_array_item *ref = new_ref_array_item(refname, oid);
> -
> - ALLOC_GROW(array->items, array->nr + 1, array->alloc);
> - array->items[array->nr++] = ref;
> -
> + ref_array_append(array, ref);
> return ref;
> }
>
> @@ -2761,46 +2764,36 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
> return ref_kind_from_refname(refname);
> }
>
> -struct ref_filter_cbdata {
> - struct ref_array *array;
> - struct ref_filter *filter;
> -};
> -
> -/*
> - * A call-back given to for_each_ref(). Filter refs and keep them for
> - * later object processing.
> - */
> -static int ref_filter_handler(const char *refname, const struct object_id *oid, int flag, void *cb_data)
> +static struct ref_array_item *apply_ref_filter(const char *refname, const struct object_id *oid,
> + int flag, struct ref_filter *filter)
> {
> - struct ref_filter_cbdata *ref_cbdata = cb_data;
> - struct ref_filter *filter = ref_cbdata->filter;
> struct ref_array_item *ref;
> struct commit *commit = NULL;
> unsigned int kind;
>
> if (flag & REF_BAD_NAME) {
> warning(_("ignoring ref with broken name %s"), refname);
> - return 0;
> + return NULL;
> }
>
> if (flag & REF_ISBROKEN) {
> warning(_("ignoring broken ref %s"), refname);
> - return 0;
> + return NULL;
> }
>
> /* Obtain the current ref kind from filter_ref_kind() and ignore unwanted refs. */
> kind = filter_ref_kind(filter, refname);
> if (!(kind & filter->kind))
> - return 0;
> + return NULL;
>
> if (!filter_pattern_match(filter, refname))
> - return 0;
> + return NULL;
>
> if (filter_exclude_match(filter, refname))
> - return 0;
> + return NULL;
>
> if (filter->points_at.nr && !match_points_at(&filter->points_at, oid, refname))
> - return 0;
> + return NULL;
>
> /*
> * A merge filter is applied on refs pointing to commits. Hence
> @@ -2811,15 +2804,15 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
> filter->with_commit || filter->no_commit || filter->verbose) {
> commit = lookup_commit_reference_gently(the_repository, oid, 1);
> if (!commit)
> - return 0;
> + return NULL;
> /* We perform the filtering for the '--contains' option... */
> if (filter->with_commit &&
> !commit_contains(filter, commit, filter->with_commit, &filter->internal.contains_cache))
> - return 0;
> + return NULL;
> /* ...or for the `--no-contains' option */
> if (filter->no_commit &&
> commit_contains(filter, commit, filter->no_commit, &filter->internal.no_contains_cache))
> - return 0;
> + return NULL;
> }
>
> /*
> @@ -2827,11 +2820,32 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
> * to do its job and the resulting list may yet to be pruned
> * by maxcount logic.
> */
> - ref = ref_array_push(ref_cbdata->array, refname, oid);
> + ref = new_ref_array_item(refname, oid);
> ref->commit = commit;
> ref->flag = flag;
> ref->kind = kind;
>
> + return ref;
> +}
> +
> +struct ref_filter_cbdata {
> + struct ref_array *array;
> + struct ref_filter *filter;
> +};
> +
> +/*
> + * A call-back given to for_each_ref(). Filter refs and keep them for
> + * later object processing.
> + */
> +static int filter_one(const char *refname, const struct object_id *oid, int flag, void *cb_data)
> +{
> + struct ref_filter_cbdata *ref_cbdata = cb_data;
> + struct ref_array_item *ref;
> +
> + ref = apply_ref_filter(refname, oid, flag, ref_cbdata->filter);
> + if (ref)
> + ref_array_append(ref_cbdata->array, ref);
> +
> return 0;
> }
>
> @@ -2967,26 +2981,12 @@ void filter_ahead_behind(struct repository *r,
> free(commits);
> }
>
> -/*
> - * API for filtering a set of refs. Based on the type of refs the user
> - * has requested, we iterate through those refs and apply filters
> - * as per the given ref_filter structure and finally store the
> - * filtered refs in the ref_array structure.
> - */
> -int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int type)
> +static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref_fn fn, void *cb_data)
> {
> - struct ref_filter_cbdata ref_cbdata;
> - int save_commit_buffer_orig;
> int ret = 0;
>
> - ref_cbdata.array = array;
> - ref_cbdata.filter = filter;
> -
> filter->kind = type & FILTER_REFS_KIND_MASK;
>
> - save_commit_buffer_orig = save_commit_buffer;
> - save_commit_buffer = 0;
> -
> init_contains_cache(&filter->internal.contains_cache);
> init_contains_cache(&filter->internal.no_contains_cache);
>
> @@ -3001,20 +3001,43 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> * of filter_ref_kind().
> */
> if (filter->kind == FILTER_REFS_BRANCHES)
> - ret = for_each_fullref_in("refs/heads/", ref_filter_handler, &ref_cbdata);
> + ret = for_each_fullref_in("refs/heads/", fn, cb_data);
> else if (filter->kind == FILTER_REFS_REMOTES)
> - ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata);
> + ret = for_each_fullref_in("refs/remotes/", fn, cb_data);
> else if (filter->kind == FILTER_REFS_TAGS)
> - ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata);
> + ret = for_each_fullref_in("refs/tags/", fn, cb_data);
> else if (filter->kind & FILTER_REFS_ALL)
> - ret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata);
> + ret = for_each_fullref_in_pattern(filter, fn, cb_data);
> if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
> - head_ref(ref_filter_handler, &ref_cbdata);
> + head_ref(fn, cb_data);
> }
>
> clear_contains_cache(&filter->internal.contains_cache);
> clear_contains_cache(&filter->internal.no_contains_cache);
>
> + return ret;
> +}
> +
> +/*
> + * API for filtering a set of refs. Based on the type of refs the user
> + * has requested, we iterate through those refs and apply filters
> + * as per the given ref_filter structure and finally store the
> + * filtered refs in the ref_array structure.
> + */
> +int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int type)
> +{
> + struct ref_filter_cbdata ref_cbdata;
> + int save_commit_buffer_orig;
> + int ret = 0;
> +
> + ref_cbdata.array = array;
> + ref_cbdata.filter = filter;
> +
> + save_commit_buffer_orig = save_commit_buffer;
> + save_commit_buffer = 0;
> +
> + ret = do_filter_refs(filter, type, filter_one, &ref_cbdata);
> +
> /* Filters that need revision walking */
> reach_filter(array, &filter->reachable_from, INCLUDE_REACHED);
> reach_filter(array, &filter->unreachable_from, EXCLUDE_REACHED);
> --
> gitgitgadget
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 7/9] ref-filter.c: filter & format refs in the same callback
From: Patrick Steinhardt @ 2023-11-07 10:49 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <84db440896c162bcbeeaaf00d528839056aefaa5.1699320362.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5290 bytes --]
On Tue, Nov 07, 2023 at 01:25:59AM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
>
> Update 'filter_and_format_refs()' to try to perform ref filtering &
> formatting in a single ref iteration, without an intermediate 'struct
> ref_array'. This can only be done if no operations need to be performed on a
> pre-filtered array; specifically, if the refs are
>
> - filtered on reachability,
> - sorted, or
> - formatted with ahead-behind information
>
> they cannot be filtered & formatted in the same iteration. In that case,
> fall back on the current filter-then-sort-then-format flow.
>
> This optimization substantially improves memory usage due to no longer
> storing a ref array in memory. In some cases, it also dramatically reduces
> runtime (e.g. 'git for-each-ref --no-sort --count=1', which no longer loads
> all refs into a 'struct ref_array' to printing only the first ref).
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> ref-filter.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 74 insertions(+), 6 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index ff00ab4b8d8..384cf1595ff 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2863,6 +2863,44 @@ static void free_array_item(struct ref_array_item *item)
> free(item);
> }
>
> +struct ref_filter_and_format_cbdata {
> + struct ref_filter *filter;
> + struct ref_format *format;
> +
> + struct ref_filter_and_format_internal {
> + int count;
> + } internal;
> +};
> +
> +static int filter_and_format_one(const char *refname, const struct object_id *oid, int flag, void *cb_data)
> +{
> + struct ref_filter_and_format_cbdata *ref_cbdata = cb_data;
> + struct ref_array_item *ref;
> + struct strbuf output = STRBUF_INIT, err = STRBUF_INIT;
> +
> + ref = apply_ref_filter(refname, oid, flag, ref_cbdata->filter);
> + if (!ref)
> + return 0;
> +
> + if (format_ref_array_item(ref, ref_cbdata->format, &output, &err))
> + die("%s", err.buf);
> +
> + if (output.len || !ref_cbdata->format->array_opts.omit_empty) {
> + fwrite(output.buf, 1, output.len, stdout);
> + putchar('\n');
> + }
> +
> + strbuf_release(&output);
> + strbuf_release(&err);
> + free_array_item(ref);
> +
> + if (ref_cbdata->format->array_opts.max_count &&
> + ++ref_cbdata->internal.count >= ref_cbdata->format->array_opts.max_count)
> + return -1;
It feels a bit weird to return a negative value here, which usually
indicates that an error has happened whereas we only use it here to
abort the iteration. But we ignore the return value of
`do_iterate_refs()` anyway, so it doesn't make much of a difference.
> + return 0;
> +}
> +
> /* Free all memory allocated for ref_array */
> void ref_array_clear(struct ref_array *array)
> {
> @@ -3046,16 +3084,46 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> return ret;
> }
>
> +static inline int can_do_iterative_format(struct ref_filter *filter,
> + struct ref_sorting *sorting,
> + struct ref_format *format)
> +{
> + /*
> + * Refs can be filtered and formatted in the same iteration as long
> + * as we aren't filtering on reachability, sorting the results, or
> + * including ahead-behind information in the formatted output.
> + */
Do we want to format this as a bulleted list so that it's more readily
extensible if we ever need to pay attention to new options here? Also, I
noted that this commit doesn't add any new tests -- do we already
exercise all of these conditions?
More generally, I worry a bit about maintainability of this code snippet
as we need to remember to always update this condition whenever we add a
new option, and this can be quite easy to miss. The performance benefit
might be worth the effort though.
Patrick
> + return !(filter->reachable_from ||
> + filter->unreachable_from ||
> + sorting ||
> + format->bases.nr);
> +}
> +
> void filter_and_format_refs(struct ref_filter *filter, unsigned int type,
> struct ref_sorting *sorting,
> struct ref_format *format)
> {
> - struct ref_array array = { 0 };
> - filter_refs(&array, filter, type);
> - filter_ahead_behind(the_repository, format, &array);
> - ref_array_sort(sorting, &array);
> - print_formatted_ref_array(&array, format);
> - ref_array_clear(&array);
> + if (can_do_iterative_format(filter, sorting, format)) {
> + int save_commit_buffer_orig;
> + struct ref_filter_and_format_cbdata ref_cbdata = {
> + .filter = filter,
> + .format = format,
> + };
> +
> + save_commit_buffer_orig = save_commit_buffer;
> + save_commit_buffer = 0;
> +
> + do_filter_refs(filter, type, filter_and_format_one, &ref_cbdata);
> +
> + save_commit_buffer = save_commit_buffer_orig;
> + } else {
> + struct ref_array array = { 0 };
> + filter_refs(&array, filter, type);
> + filter_ahead_behind(the_repository, format, &array);
> + ref_array_sort(sorting, &array);
> + print_formatted_ref_array(&array, format);
> + ref_array_clear(&array);
> + }
> }
>
> static int compare_detached_head(struct ref_array_item *a, struct ref_array_item *b)
> --
> gitgitgadget
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 8/9] for-each-ref: add option to fully dereference tags
From: Patrick Steinhardt @ 2023-11-07 10:50 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <352b5c42ac39d5d2646a1b6d47d6d707637db539.1699320362.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8763 bytes --]
On Tue, Nov 07, 2023 at 01:26:00AM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
>
> Add a boolean flag '--full-deref' that, when enabled, fills '%(*fieldname)'
> format fields using the fully peeled target of tag objects, rather than the
> immediate target.
>
> In other builtins ('rev-parse', 'show-ref'), "dereferencing" tags typically
> means peeling them down to their non-tag target. Unlike these commands,
> 'for-each-ref' dereferences only one "level" of tags in '*' format fields
> (like "%(*objectname)"). For most annotated tags, one level of dereferencing
> is enough, since most tags point to commits or trees. However, nested tags
> (annotated tags whose target is another annotated tag) dereferenced once
> will point to their target tag, different a full peel to e.g. a commit.
>
> Currently, if a user wants to filter & format refs and include information
> about the fully dereferenced tag, they can do so with something like
> 'cat-file --batch-check':
>
> git for-each-ref --format="%(objectname)^{} %(refname)" <pattern> |
> git cat-file --batch-check="%(objectname) %(rest)"
>
> But the combination of commands is inefficient. So, to improve the
> efficiency of this use case, add a '--full-deref' option that causes
> 'for-each-ref' to fully dereference tags when formatting with '*' fields.
I do wonder whether it would make sense to introduce this feature in the
form of a separate field prefix, as you also mentioned in your cover
letter. It would buy the user more flexibility, but the question is
whether such flexibility would really ever be needed.
The only thing I could really think of where it might make sense is to
distinguish tags that peel to a commit immediately from ones that don't.
That feels rather esoteric to me and doesn't seem to be of much use. But
regardless of whether or not we can see the usefulness now, if this
wouldn't be significantly more complex I wonder whether it would make
more sense to use a new field prefix instead anyway.
In any case, I think it would be helpful if this was discussed in the
commit message.
Patrick
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> Documentation/git-for-each-ref.txt | 9 ++++++++
> builtin/for-each-ref.c | 2 ++
> ref-filter.c | 26 ++++++++++++++---------
> ref-filter.h | 1 +
> t/t6300-for-each-ref.sh | 34 ++++++++++++++++++++++++++++++
> 5 files changed, 62 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 407f624fbaa..2714a87088e 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -11,6 +11,7 @@ SYNOPSIS
> 'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
> [(--sort=<key>)...] [--format=<format>]
> [ --stdin | <pattern>... ]
> + [--full-deref]
> [--points-at=<object>]
> [--merged[=<object>]] [--no-merged[=<object>]]
> [--contains[=<object>]] [--no-contains[=<object>]]
> @@ -77,6 +78,14 @@ OPTIONS
> the specified host language. This is meant to produce
> a scriptlet that can directly be `eval`ed.
>
> +--full-deref::
> + Populate dereferenced format fields (indicated with an asterisk (`*`)
> + prefix before the fieldname) with information about the fully-peeled
> + target object of a tag ref, rather than its immediate target object.
> + This only affects the output for nested annotated tags, where the tag's
> + immediate target is another tag but its fully-peeled target is another
> + object type (e.g. a commit).
> +
> --points-at=<object>::
> Only list refs which points at the given object.
>
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 1c19cd5bd34..7a2127a3bc4 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -43,6 +43,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
> OPT_INTEGER( 0 , "count", &format.array_opts.max_count, N_("show only <n> matched refs")),
> OPT_STRING( 0 , "format", &format.format, N_("format"), N_("format to use for the output")),
> OPT__COLOR(&format.use_color, N_("respect format colors")),
> + OPT_BOOL(0, "full-deref", &format.full_deref,
> + N_("fully dereference tags to populate '*' format fields")),
> OPT_REF_FILTER_EXCLUDE(&filter),
> OPT_REF_SORT(&sorting_options),
> OPT_CALLBACK(0, "points-at", &filter.points_at,
> diff --git a/ref-filter.c b/ref-filter.c
> index 384cf1595ff..a66ac7921b1 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -237,7 +237,14 @@ static struct used_atom {
> char *head;
> } u;
> } *used_atom;
> -static int used_atom_cnt, need_tagged, need_symref;
> +static int used_atom_cnt, need_symref;
> +
> +enum tag_dereference_mode {
> + NO_DEREF = 0,
> + DEREF_ONE,
> + DEREF_ALL
> +};
> +static enum tag_dereference_mode need_tagged;
>
> /*
> * Expand string, append it to strbuf *sb, then return error code ret.
> @@ -1066,8 +1073,8 @@ static int parse_ref_filter_atom(struct ref_format *format,
> memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
> if (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))
> return -1;
> - if (*atom == '*')
> - need_tagged = 1;
> + if (*atom == '*' && !need_tagged)
> + need_tagged = format->full_deref ? DEREF_ALL : DEREF_ONE;
> if (i == ATOM_SYMREF)
> need_symref = 1;
> return at;
> @@ -2511,14 +2518,13 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
> * If it is a tag object, see if we use a value that derefs
> * the object, and if we do grab the object it refers to.
> */
> - oi_deref.oid = *get_tagged_oid((struct tag *)obj);
> + if (need_tagged == DEREF_ALL) {
> + if (peel_iterated_oid(&obj->oid, &oi_deref.oid))
> + die("bad tag");
> + } else {
> + oi_deref.oid = *get_tagged_oid((struct tag *)obj);
> + }
>
> - /*
> - * NEEDSWORK: This derefs tag only once, which
> - * is good to deal with chains of trust, but
> - * is not consistent with what deref_tag() does
> - * which peels the onion to the core.
> - */
> return get_object(ref, 1, &obj, &oi_deref, err);
> }
>
> diff --git a/ref-filter.h b/ref-filter.h
> index 0ce5af58ab3..0caa39ecee5 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -92,6 +92,7 @@ struct ref_format {
> const char *rest;
> int quote_style;
> int use_color;
> + int full_deref;
>
> /* Internal state to ref-filter */
> int need_color_reset_at_eol;
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 0613e5e3623..3c2af785cdb 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -1839,6 +1839,40 @@ test_expect_success 'git for-each-ref with non-existing refs' '
> test_must_be_empty actual
> '
>
> +test_expect_success 'git for-each-ref with nested tags' '
> + git tag -am "Normal tag" nested/base HEAD &&
> + git tag -am "Nested tag" nested/nest1 refs/tags/nested/base &&
> + git tag -am "Double nested tag" nested/nest2 refs/tags/nested/nest1 &&
> +
> + head_oid="$(git rev-parse HEAD)" &&
> + base_tag_oid="$(git rev-parse refs/tags/nested/base)" &&
> + nest1_tag_oid="$(git rev-parse refs/tags/nested/nest1)" &&
> + nest2_tag_oid="$(git rev-parse refs/tags/nested/nest2)" &&
> +
> + # Without full dereference
> + cat >expect <<-EOF &&
> + refs/tags/nested/base $base_tag_oid tag $head_oid commit
> + refs/tags/nested/nest1 $nest1_tag_oid tag $base_tag_oid tag
> + refs/tags/nested/nest2 $nest2_tag_oid tag $nest1_tag_oid tag
> + EOF
> +
> + git for-each-ref --format="%(refname) %(objectname) %(objecttype) %(*objectname) %(*objecttype)" \
> + refs/tags/nested/ >actual &&
> + test_cmp expect actual &&
> +
> + # With full dereference
> + cat >expect <<-EOF &&
> + refs/tags/nested/base $base_tag_oid tag $head_oid commit
> + refs/tags/nested/nest1 $nest1_tag_oid tag $head_oid commit
> + refs/tags/nested/nest2 $nest2_tag_oid tag $head_oid commit
> + EOF
> +
> + git for-each-ref --full-deref \
> + --format="%(refname) %(objectname) %(objecttype) %(*objectname) %(*objecttype)" \
> + refs/tags/nested/ >actual &&
> + test_cmp expect actual
> +'
> +
> GRADE_FORMAT="%(signature:grade)%0a%(signature:key)%0a%(signature:signer)%0a%(signature:fingerprint)%0a%(signature:primarykeyfingerprint)"
> TRUSTLEVEL_FORMAT="%(signature:trustlevel)%0a%(signature:key)%0a%(signature:signer)%0a%(signature:fingerprint)%0a%(signature:primarykeyfingerprint)"
>
> --
> gitgitgadget
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* git-send-email: Send with mutt(1)
From: Alejandro Colomar @ 2023-11-07 11:14 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 330 bytes --]
Hi!
I'm trying to use mutt(1) with git-send-email(1). Is that possible?
I tried --sendmail-cmd=mutt, but am getting strange errors.
The reason I want to send with mutt(1) is because it can encrypt and
sign mail (with some tweaks), which git-send-mail(1) doesn't.
Thanks,
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: first-class conflicts?
From: Phillip Wood @ 2023-11-07 11:23 UTC (permalink / raw)
To: Sandra Snan, git, Dragan Simic, rsbecker
In-Reply-To: <Gr..Y5kkszDx87g@idiomdrottning.org>
Hi Sandra
On 06/11/2023 22:45, Sandra Snan wrote:
> Randall, thank you for that.
>
> I did mean of the first type, pure content conflicts (just like the
> examples on that jj page).
>
> I just have sometimes wish git could be a little more aware of them
> beyond just storing them with ASCII art in the files themselves (and
> alerting / warning when they happen but I often can't properly see those
> warnings flash by so I end up having to search for the conflict markers
> manually). So if conflicts are a thing that *can* happen, it'd be better
> if vc could know about them which would make some of the rebases simpler
> as in jj. That doesn't mean we wanna adopt the jj workflow of
> deliberately checking in conflicts (not even locally), just be able to
> deal with them better if it does happen.
>
> I dunno… and I've really appreciated the naysayers so far, helps me sort
> out my thoughts in this. I personally really prefer the vanilla
> "explicit staging" workflow (with magit) over jj, got, gitless etc. I'm
> more scared of overcommitting by mistake than undercommitting. But this
> one feature seemed to me that it might be really good: just having the
> vc be aware of the conflicts it has created.
If you run "git status" it will list the files that have conflicts as
"unmerged". To prevent "git commit" from creating a commit that contains
conflict markers you can use a pre-commit hook that runs "git diff
--cached--check". The sample hook that is created by default does this,
to activate it run
mv .git/hooks/pre-commit.sample .git/hooks/pre-commit
in the main worktree. You can also run "git config commit.verbose true"
to make "git commit" show the diff of the changes that will be committed
below the commit message when you're editing the message.
Best Wishes
Phillip
^ permalink raw reply
* Re: RE: first-class conflicts?
From: Sandra Snan @ 2023-11-07 11:24 UTC (permalink / raw)
To: git, Dragan Simic, rsbecker
In-Reply-To: <86e6b392-5a61-4864-89b0-42023e1804a6@gmail.com>
That is wonderful! Thank you so much, Phillip! 👍🏻
^ permalink raw reply
* issue unable to commit file and folder name to upper lower case
From: chengpu lee @ 2023-11-07 11:28 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 1972 bytes --]
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
$ mkdir abc
$ echo "">abc/.keep
$ git status
On branch master
Untracked files:
(use "git add <file>..." to include in what will be committed)
abc/
$ mv abc Abc
$ git status
On branch master
Untracked files:
(use "git add <file>..." to include in what will be committed)
Abc/ # rename
to Abc/ successfully
$ git commit -m "commit Abc/" # commit
$ mv Abc abc
$ git status
On branch master
nothing to commit, working tree clean # cannot
rename again
$ git diff
>""
$ git add .
$ git commit -m "Cannot rename from Abc/ to abc/"
On branch master
nothing to commit, working tree clean # cannot
commit to right, keep old file name / folder name
What did you expect to happen? (Expected behavior)
It should be correct to new name although just change upper/lower case,
because other commiter can pull it from repo or improve reading clarity.
What happened instead? (Actual behavior)
can commit file/folder but cannot fix upper/lower case.
What's different between what you expected and what actually happened?
git unable to faithfully represent file/folder upper/lower case changes.
Anything else you want to add:
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.40.0.windows.1
cpu: x86_64
built from commit: 1d90ca2906dd4b7ddaf0669a13c173ec579d794a
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19045
compiler info: gnuc: 12.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe
[Enabled Hooks]
[-- Attachment #1.2: Type: text/html, Size: 2378 bytes --]
[-- Attachment #2: git-bugreport-2023-11-07-1855.txt --]
[-- Type: text/plain, Size: 1973 bytes --]
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
$ mkdir abc
$ echo "">abc/.keep
$ git status
On branch master
Untracked files:
(use "git add <file>..." to include in what will be committed)
abc/
$ mv abc Abc
$ git status
On branch master
Untracked files:
(use "git add <file>..." to include in what will be committed)
Abc/ # rename to Abc/ successfully
$ git commit -m "commit Abc/" # commit
$ mv Abc abc
$ git status
On branch master
nothing to commit, working tree clean # cannot rename again
$ git diff
>""
$ git add .
$ git commit -m "Cannot rename from Abc/ to abc/"
On branch master
nothing to commit, working tree clean # cannot commit to right, keep old file name / folder name
What did you expect to happen? (Expected behavior)
It should be correct to new name although just change upper/lower case, because other commiter can pull it from repo or improve reading clarity.
What happened instead? (Actual behavior)
can commit file/folder but cannot fix upper/lower case.
What's different between what you expected and what actually happened?
git unable to faithfully represent file/folder upper/lower case changes.
Anything else you want to add:
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.40.0.windows.1
cpu: x86_64
built from commit: 1d90ca2906dd4b7ddaf0669a13c173ec579d794a
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19045
compiler info: gnuc: 12.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe
[Enabled Hooks]
^ permalink raw reply
* Re: first-class conflicts?
From: Phillip Wood @ 2023-11-07 11:49 UTC (permalink / raw)
To: Elijah Newren, Sandra Snan; +Cc: git, Martin von Zweigbergk, Randall S. Becker
In-Reply-To: <CABPp-BH7WBm1j-Ue9oZFjoy6sTcw5B0hz_ndDEtJqvpZF4YF=w@mail.gmail.com>
Hi Elijah
[I've cc'd Martin to see if he has anything to add about how "jj"
manages the issues around storing conflicts.]
On 07/11/2023 08:16, Elijah Newren wrote:
> On Mon, Nov 6, 2023 at 1:26 PM Sandra Snan
> <sandra.snan@idiomdrottning.org> wrote:
>>
>> Is this feature from jj also a good idea for git?
>> https://martinvonz.github.io/jj/v0.11.0/conflicts/
>
> Martin talked about this and other features at Git Merge 2022, a
> little over a year ago. I talked to him in more depth about these
> while there. I personally think he has some really interesting
> features here, though at the time, I thought that the additional
> object type might be too much to ask for in a Git change, and it was
> an intrinsic part of the implementation back then.
>
> Martin also gave us an update at the 2023 Git Contributors summit, and
> in particular noted a significant implementation change to not have
> per-file storage of conflicts, but rather storing at the commit level
> the multiple conflicting trees involved. That model might be
> something we could implement in Git. And if we did, it'd solve
> various issues such as people wanting to be able to stash conflicts,
> or wanting to be able to partially resolve conflicts and fix it up
> later, or be able to collaboratively resolve conflicts without having
> everyone have access to the same checkout.
One thing to think about if we ever want to implement this is what other
data we need to store along with the conflict trees to preserve the
context in which the conflict was created. For example the files that
are read by "git commit" when it commits a conflict resolution. For a
single cherry-pick/revert it would probably be fairly straight forward
to store CHERRY_PICK_HEAD/REVERT_HEAD and add it as a parent so it gets
transferred along with the conflicts. For a sequence of cherry-picks or
a rebase it is more complicated to preserve the context of the conflict.
Even "git merge" can create several files in addition to MERGE_HEAD
which are read when the conflict resolution is committed.
> But we'd also have to be careful and think through usecases, including
> in the surrounding community. People would probably want to ensure
> that e.g. "Protected" or "Integration" branches don't get accept
> fetches or pushes of conflicted commits,
I think this is a really important point, while it can be useful to
share conflicts so they can be collaboratively resolved we don't want to
propagate them into "stable" or production branches. I wonder how 'jj'
handles this.
> git status would probably
> need some special warnings or notices, git checkout would probably
> benefit from additional warnings/notices checks for those cases, git
> log should probably display conflicted commits differently, we'd need
> to add special handling for higher order conflicts (e.g. a merge with
> conflicts is itself involved in a merge) probably similar to what jj
> has done, and audit a lot of other code paths to see what would be
> needed.
As you point out there is a lot more to this than just being able to
store the conflict data in a commit - in many ways I think that is the
easiest part of the solution to sharing conflicts.
Best Wishes
Phillip
^ permalink raw reply
* Error when "git mv" file in a sparsed checkout
From: Josef Wolf @ 2023-11-07 13:03 UTC (permalink / raw)
To: git
Hello,
I have used the procedure described below for many years. In fact,
this procedure is part of a script which I am using for about 10 years.
This procedure was definitely working with git-2-25-1 and git-2.26.2.
Now, with git-2.34.1 (on a freshly installed ubuntu-22.04), this
procedure fails.
Here is what I do:
I want to rename a file on a branch which is not currently checked out
without messing/touching my current working directory.
For this, I first create a clone of the repo with shared git-directory:
$ SANDBOX=/var/tmp/manage-scans-X1pKZQiey
$ WT=$SANDBOX/wt
$ GIT=$SANDBOX/git
$ mkdir -p $SANDBOX
$ git --work-tree $WT --git-dir $GIT clone -qns -n ~/upstream-repo $GIT
Then, I do a sparse checkout in this clone, containing only the file
that is to be renamed:
$ cd $WT
$ echo 'path/to/old-filename' >>$GIT/info/sparse-checkout
$ git --work-tree $WT --git-dir $GIT config core.sparsecheckout true
$ git --work-tree $WT --git-dir $GIT checkout -b the-branch remotes/origin/the-branch
Switched to a new branch 'the-branch'
Next step would be to "git mv" the file:
$ mkdir -p /path/to # already exists, but should do no harm
$ git --work-tree $WT --git-dir $GIT mv path/to/old-filename path/to/new-filename
The following paths and/or pathspecs matched paths that exist
outside of your sparse-checkout definition, so will not be
updated in the index:
path/to/new-filename
hint: If you intend to update such entries, try one of the following:
hint: * Use the --sparse option.
hint: * Disable or modify the sparsity rules.
hint: Disable this message with "git config advice.updateSparsePath false"
This error is something I have not expected.
Error message suggests, there already exists a file named "new-filename". This
is not true at all. There is no file named "new-filename" in the entire
repository. Not in any directory of any branch.
--
Josef Wolf
jw@raven.inka.de
^ permalink raw reply
* Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together
From: Taylor Blau @ 2023-11-07 15:58 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, git, Elijah Newren, Eric W. Biederman, Jeff King,
Patrick Steinhardt
In-Reply-To: <0ac32374-7d52-8f0c-8583-110de678291e@gmx.de>
On Mon, Nov 06, 2023 at 04:46:32PM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Mon, 23 Oct 2023, Junio C Hamano wrote:
>
> > Taylor Blau <me@ttaylorr.com> writes:
> >
> > > But I think that this approach ended up being less heavy-weight than I
> > > had originally imagined, so I think that this version is a worthwhile
> > > improvement over v4.
> >
> > ;-).
> >
> > This version is a good place to stop, a bit short of going full OO.
> > Nicely done.
>
> I wonder whether a more generic approach would be more desirable, an
> approach that would work for `git replay`, too, for example (where
> streaming objects does not work because they need to be made available
> immediately because subsequent `merge_incore_nonrecursive()` might expect
> the created objects to be present)?
The goal of this series is to bound the number of inodes consumed by a
single merge-tree invocation down from arbitrarily many (in the case of
storing each new object loose) to a constant (by storing everything in a
single pack).
I'd think that we would want a similar approach for 'replay', but as
you note we have some additional requirements, too:
- each replayed commit is computed in a single step, which will result
in a new pack
- we must be able to see objects from previous steps
I think one feasible approach here for replay is to combine the two
ideas and have a separate objdir that stores N packs (one for each step
of the replay), but then repacks them down into a single pack before
migrating back to the main object store.
That would ensure that we have some isolation between replay-created
objects and the rest of the repository in the intermediate state. Even
though we'd have as many packs as there are commits, we'd consume far
fewer inodes in the process, since each commit can introduce arbitrarily
many new objects, each requiring at least a single inode (potentially
more with sharding).
We'd have to be mindful of having a large number of packs, but I think
that this should mostly be a non-issue, since we'd only be living with N
packs for the lifetime of the replay command (before repacking them down
to a single pack and migrating them back to the main object store).
Thanks,
Taylor
^ permalink raw reply
* Re: Explanation of `eol` attribute doesn't seem correct for Windows machines
From: Torsten Bögershausen @ 2023-11-07 17:12 UTC (permalink / raw)
To: Hans Meiser; +Cc: git@vger.kernel.org
In-Reply-To: <AS4P195MB1527BC2DF67412D4FF340989E2A9A@AS4P195MB1527.EURP195.PROD.OUTLOOK.COM>
On Tue, Nov 07, 2023 at 12:23:29AM +0000, Hans Meiser wrote:
> > > And what happens when a file is added to the index with `core.autocrlf=input` on Windows machines?
> > Do you have a .gitattributes file ? Or not ?
> > Is the file a new one, or does it exist ?
>
> Exactly all these questions/cases should be explained by the documentation, I suppose.
In general, the CRLF - LF ("line endings") conversion is defined by
different settings.
The .gitattributes file (which is typically added to the repo and commited,
so that it travels trough into all repos by using `git clone`, `git pull`
or `git fetch` followed by a `git checkout`
This link
https://git-scm.com/docs/gitattributes#_text
should describe everything you may want to know about
.gitattributes
to handle line endings.
If I say "should" then this is from a perspective of someone who has read and used this
too often to see with fresh eyes what may be missing.
^ permalink raw reply
* Re: [PATCH] diff: implement config.diff.renames=copies-harder
From: Elijah Newren @ 2023-11-07 17:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sam James via GitGitGadget, git, Sam James
In-Reply-To: <xmqq7cmu9s29.fsf@gitster.g>
On Mon, Nov 6, 2023 at 7:10 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Fri, Nov 3, 2023 at 4:25 AM Sam James via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Sam James <sam@gentoo.org>
> >>
> >> This patch adds a config value for 'diff.renames' called 'copies-harder'
> >> which make it so '-C -C' is in effect always passed for 'git log -p',
> >> 'git diff', etc.
> >>
> >> This allows specifying that 'git log -p', 'git diff', etc should always act
> >> as if '-C --find-copies-harder' was passed.
> >>
> >> I've found this especially useful for certain types of repository (like
> >> Gentoo's ebuild repositories) because files are often copies of a previous
> >> version.
> >
> > These must be very small repositories? --find-copies-harder is really
> > expensive...
>
> True. "often copies of a previous version" means that it is a
> directory that has a collection of subdirectories, one for each
> version? In a source tree managed in a version control system,
> files are often rewritten in place from the previous version,
> so I am puzzled by that justification.
>
> It is, in the proposed log message of our commits, a bit unusual to
> see "This patch does X" and "I do Y", by the way, which made my
> reading hiccup a bit, but perhaps it is just me?
I think I read Sam's description a bit differently than you. My
assumption was they'd have files with names like the following in the
same directory:
gcc-13.x.build.recipe
gcc-12.x.build.recipe
gcc-11.x.build.recipe
gcc-10.x.build.recipe
And that gcc-13.x.build.recipe was started as a copy of
gcc-12.x.build.recipe (which was started as a copy of
gcc-11.x.build.recipe, etc.). They keep all versions because they
want users to be able to build and install multiple gcc versions.
I could be completely off, but that's what I was imagining from the description.
> >> diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt
> >> index bd5ae0c3378..d2ff3c62d41 100644
> >> --- a/Documentation/config/diff.txt
> >> +++ b/Documentation/config/diff.txt
> >> @@ -131,7 +131,8 @@ diff.renames::
> >> Whether and how Git detects renames. If set to "false",
> >> rename detection is disabled. If set to "true", basic rename
> >> detection is enabled. If set to "copies" or "copy", Git will
> >> - detect copies, as well. Defaults to true. Note that this
> >> + detect copies, as well. If set to "copies-harder", Git will try harder
> >> + to detect copies. Defaults to true. Note that this
> >
> > "try harder to detect copies" feels like an unhelpful explanation.
>
> Yup. "will spend extra cycles to find more copies", perhaps?
I find that marginally better; but I still don't think it answers the
user's question of why they should pick one option or the other. The
wording for the `--find-copies-harder` does explain when it's useful:
For performance reasons, by default, `-C` option finds copies only
if the original file of the copy was modified in the same
changeset. This flag makes the command
inspect unmodified files as candidates for the source of
copy. This is a very expensive operation for large
projects, so use it with caution.
We probably don't want to copy all three of those sentences here, but
I think we need to make sure users can find them, thus my suggestion
to reference the `--find-copies-harder` option to git-diff so that
affected users can get the info they need to choose.
^ permalink raw reply
* Re: issue unable to commit file and folder name to upper lower case
From: Torsten Bögershausen @ 2023-11-07 17:35 UTC (permalink / raw)
To: chengpu lee; +Cc: git
In-Reply-To: <CAHv3AeCOoEXxpNh=gzjNcKbVyZFaYZ5BzSf3FGL1=pdheNebZw@mail.gmail.com>
On Tue, Nov 07, 2023 at 07:28:14PM +0800, chengpu lee wrote:
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
> $ mkdir abc
>
> $ echo "">abc/.keep
>
> $ git status
> On branch master
> Untracked files:
> (use "git add <file>..." to include in what will be committed)
> abc/
>
> $ mv abc Abc
>
> $ git status
> On branch master
> Untracked files:
> (use "git add <file>..." to include in what will be committed)
> Abc/ # rename
> to Abc/ successfully
>
> $ git commit -m "commit Abc/" # commit
>
> $ mv Abc abc
>
> $ git status
> On branch master
> nothing to commit, working tree clean # cannot
> rename again
>
> $ git diff
> >""
>
> $ git add .
>
> $ git commit -m "Cannot rename from Abc/ to abc/"
> On branch master
> nothing to commit, working tree clean # cannot
> commit to right, keep old file name / folder name
>
>
> What did you expect to happen? (Expected behavior)
> It should be correct to new name although just change upper/lower case,
> because other commiter can pull it from repo or improve reading clarity.
>
> What happened instead? (Actual behavior)
> can commit file/folder but cannot fix upper/lower case.
>
> What's different between what you expected and what actually happened?
> git unable to faithfully represent file/folder upper/lower case changes.
>
Yes, that is a restriction in Git, call it a bug, call it a missing feature.
Unless someone fixes it, the recommendation is still to use a workaround:
tb@pc:/tmp/ttt> git mv Abc tmp
tb@pc:/tmp/ttt> git mv tmp abc
tb@pc:/tmp/ttt> git status
On branch master
Changes to be committed:
(use "git restore --staged <file>..." to unstage)
renamed: Abc/.keep -> abc/.keep
tb@pc:/tmp/ttt>
^ permalink raw reply
* Re: first-class conflicts?
From: Martin von Zweigbergk @ 2023-11-07 17:38 UTC (permalink / raw)
To: phillip.wood; +Cc: Elijah Newren, Sandra Snan, git, Randall S. Becker
In-Reply-To: <ba047d38-4ad1-4440-8342-3379404f430b@gmail.com>
(new attempt in plain text)
On Tue, Nov 7, 2023 at 3:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Elijah
>
> [I've cc'd Martin to see if he has anything to add about how "jj"
> manages the issues around storing conflicts.]
>
> On 07/11/2023 08:16, Elijah Newren wrote:
> > On Mon, Nov 6, 2023 at 1:26 PM Sandra Snan
> > <sandra.snan@idiomdrottning.org> wrote:
> >>
> >> Is this feature from jj also a good idea for git?
> >> https://martinvonz.github.io/jj/v0.11.0/conflicts/
> >
> > Martin talked about this and other features at Git Merge 2022, a
> > little over a year ago. I talked to him in more depth about these
> > while there. I personally think he has some really interesting
> > features here, though at the time, I thought that the additional
> > object type might be too much to ask for in a Git change, and it was
> > an intrinsic part of the implementation back then.
> >
> > Martin also gave us an update at the 2023 Git Contributors summit, and
> > in particular noted a significant implementation change to not have
> > per-file storage of conflicts, but rather storing at the commit level
> > the multiple conflicting trees involved. That model might be
> > something we could implement in Git. And if we did, it'd solve
> > various issues such as people wanting to be able to stash conflicts,
> > or wanting to be able to partially resolve conflicts and fix it up
> > later, or be able to collaboratively resolve conflicts without having
> > everyone have access to the same checkout.
>
> One thing to think about if we ever want to implement this is what other
> data we need to store along with the conflict trees to preserve the
> context in which the conflict was created. For example the files that
> are read by "git commit" when it commits a conflict resolution. For a
> single cherry-pick/revert it would probably be fairly straight forward
> to store CHERRY_PICK_HEAD/REVERT_HEAD and add it as a parent so it gets
> transferred along with the conflicts. For a sequence of cherry-picks or
> a rebase it is more complicated to preserve the context of the conflict.
> Even "git merge" can create several files in addition to MERGE_HEAD
> which are read when the conflict resolution is committed.
Good point. We actually don't store any extra data in jj. The old
per-path conflict model was prepared for having some label associated
with each term of the conflict but we never actually used it.
If we add such metadata, it would probably have to be something that
makes sense even after pushing the conflict to another repo, so it
probably shouldn't be commit ids, unless we made sure to also push
those commits. Also note that if you `jj restore --from <commit with
conflict>`, you can get a conflict into a commit that didn't have
conflicts previously. Or if you already had conflicts in the
destination commit, your root trees (the multiple root trees
constituting the conflict) will now have conflicts that potentially
were created by two completely unrelated operations, so you would kind
of need different labels for different paths.
https://github.com/martinvonz/jj/issues/1176 has some more discussion
about this.
> > But we'd also have to be careful and think through usecases, including
> > in the surrounding community. People would probably want to ensure
> > that e.g. "Protected" or "Integration" branches don't get accept
> > fetches or pushes of conflicted commits,
>
> I think this is a really important point, while it can be useful to
> share conflicts so they can be collaboratively resolved we don't want to
> propagate them into "stable" or production branches. I wonder how 'jj'
> handles this.
Agreed. `jj git push` refuses to push commits with conflicts, because
it's very unlikely that the remote will be able to make any sense of
it. Our commit backend at Google does support conflicts, so users can
check out each other's conflicted commits there (except that we
haven't even started dogfooding yet).
> > git status would probably
> > need some special warnings or notices, git checkout would probably
> > benefit from additional warnings/notices checks for those cases, git
> > log should probably display conflicted commits differently, we'd need
> > to add special handling for higher order conflicts (e.g. a merge with
> > conflicts is itself involved in a merge) probably similar to what jj
> > has done, and audit a lot of other code paths to see what would be
> > needed.
>
> As you point out there is a lot more to this than just being able to
> store the conflict data in a commit - in many ways I think that is the
> easiest part of the solution to sharing conflicts.
Yes, I think it would be a very large project. Unlike jj, Git of
course has to worry about backwards compatibility. For example, you
would have to decide if your goal - even in the long term - is to make
`git rebase` etc. not get interrupted due to conflicts.
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Jeff King @ 2023-11-07 17:48 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: git
In-Reply-To: <ZUocFhmPHstwKCkZ@devuan>
On Tue, Nov 07, 2023 at 12:14:21PM +0100, Alejandro Colomar wrote:
> I'm trying to use mutt(1) with git-send-email(1). Is that possible?
> I tried --sendmail-cmd=mutt, but am getting strange errors.
> The reason I want to send with mutt(1) is because it can encrypt and
> sign mail (with some tweaks), which git-send-mail(1) doesn't.
I think there's a lot of overlap between what git-send-email does and
what mutt does, to the point that you probably don't need to use
send-email at all.
I assume what you want out of send-email here is the actual generation
of patch emails. But under the hood that is all done by git-format-patch
anyway. So for example if you do:
git format-patch --stdout origin..HEAD >patches
mutt -f patches
And then you can use mutt's "resend-message" function to send each one.
I use config like this:
macro index,pager b ":set edit_headers=yes<enter><resend-message>:set edit_headers=no<enter>"
If you're sending a long series, it's helpful to pre-populate various
headers in the format-patch command with "--to", etc. I usually do so by
sending the cover letter directly via mutt, and then using some perl
hackery to convert those headers into format-patch args. The script I
use is below (it will also, when run without a terminal, generate the
patch summary for the cover letter; I use it with "r!my-script" while
writing the cover letter in vim).
(This script is what I use every day, so it should be fairly robust. But
it is also over 15 years old, so I don't promise there isn't a simpler
way to do some of what it does ;) ).
-- >8 --
#!/bin/sh
upstream_branch() {
current=`git symbolic-ref HEAD`
upstream=`git for-each-ref --format='%(upstream)' "$current"`
if test -n "$upstream"; then
echo $upstream
else
echo origin
fi
}
get_reply_headers() {
perl -ne '
if (defined $opt) {
if (/^\s+(.*)/) {
$val .= " $1";
next;
}
print "--$opt=", quotemeta($val), " ";
$opt = $val = undef;
}
if (/^(cc|to):\s*(.*)/i) {
$opt = lc($1);
$val = $2;
}
elsif (/^message-id:\s*(.*)/i) {
$opt = "in-reply-to";
$val = $1;
}
elsif (/^subject:\s*\[PATCH v(\d+)/i) {
print "-v$1 ";
}
elsif (/^$/) {
last;
}
'
}
format_patch() {
git format-patch -s --stdout --from "$@"
}
has_nonoption=
for i in "$@"; do
case "$i" in
-[0-9]*) has_nonoption=yes ;;
-*) ;;
*) has_nonoption=yes
esac
done
: ${REPLY:=$HOME/patch}
test -e "$REPLY" && eval "set -- `get_reply_headers <\"$REPLY\"` \"\$@\""
test "$has_nonoption" = "yes" || set -- "$@" `upstream_branch`
if test -t 1; then
format_patch "$@" >.mbox
mutt -e 'set sort=mailbox-order' -f .mbox
rm -f .mbox
else
format_patch "$@" |
perl -lne '
if (/^Subject: (.*)/) {
$subject = $1;
}
elsif ($subject && /^\s+(.*)/) {
$subject .= " $1";
}
elsif ($subject) {
print $subject;
$subject = undef;
}
' |
sed -e 's/\[PATCH /[/' \
-e 's/]/]:/' \
-e 's/^/ /'
echo
format_patch --cover-letter "$@" |
sed -ne '/|/,/^$/p; /^-- /q'
fi
^ permalink raw reply
* Re: [PATCH 1/9] ref-filter.c: really don't sort when using --no-sort
From: Victoria Dye @ 2023-11-07 18:13 UTC (permalink / raw)
To: Patrick Steinhardt, Victoria Dye via GitGitGadget; +Cc: git
In-Reply-To: <ZUoWRZcD0xyfgVnc@tanuki>
Patrick Steinhardt wrote:
> On Tue, Nov 07, 2023 at 01:25:53AM +0000, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>
>>
>> Update 'ref_sorting_options()' to return a NULL 'struct ref_sorting *' if
>> the string list provided to it is empty, rather than returning the default
>> refname sort structure. Also update 'ref_array_sort()' to explicitly skip
>> sorting if its 'struct ref_sorting *' arg is NULL. Other functions using
>> 'struct ref_sorting *' do not need any changes because they already properly
>> ignore NULL values.
>>
>> The goal of this change is to have the '--no-sort' option truly disable
>> sorting in commands like 'for-each-ref, 'tag', and 'branch'. Right now,
>> '--no-sort' will still trigger refname sorting by default in 'for-each-ref',
>> 'tag', and 'branch'.
>>
>> To match existing behavior as closely as possible, explicitly add "refname"
>> to the list of sort keys in 'for-each-ref', 'tag', and 'branch' before
>> parsing options (if no config-based sort keys are set). This ensures that
>> sorting will only be fully disabled if '--no-sort' is provided as an option;
>> otherwise, "refname" sorting will remain the default. Note: this also means
>> that even when sort keys are provided on the command line, "refname" will be
>> the final sort key in the sorting structure. This doesn't actually change
>> any behavior, since 'compare_refs()' already falls back on comparing
>> refnames if two refs are equal w.r.t all other sort keys.
>>
>> Finally, remove the condition around sorting in 'ls-remote', since it's no
>> longer necessary. Unlike 'for-each-ref' et. al., it does *not* set any sort
>> keys by default. The default empty list of sort keys will produce a NULL
>> 'struct ref_sorting *', which causes the sorting to be skipped in
>> 'ref_array_sort()'.
>
> I found the order in this commit message a bit funny because you first
> explain what you're doing, then explain the goal, and then jump into the
> changes again. The message might be a bit easier to read if the goal was
> stated up front.
I'll try to restructure it.
>
> I was also briefly wondering whether it would make sense to split up
> this commit, as you're doing two different things:
>
> - Refactor how git-for-each-ref(1), git-tag(1) and git-branch(1) set
> up their default sorting.
>
> - Change `ref_array_sort()` to not sort when its sorting option is
> `NULL`.
>
> If this was split up into two commits, then the result might be a bit
> easier to reason about. But I don't feel strongly about this.
The addition of "refname" to the sorting defaults really only makes sense in
the context of needing it to update 'ref_array_sort()', though. While you
can convey some of that in a commit message, when reading through commits
(mine and others') I find it much easier to contextualize small refactors
with their associated behavior change if they're done in a single patch.
There's a limit to that, of course; even within this series I have a lot of
"this will make sense later" commit messages (more than I'd like really)
because the refactors are large & varied enough that they'd be overwhelming
if squashed into a single patch.
So, while I definitely see where you're coming from, I think this patch is
better off not being split.
>> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
>> index fc765754305..436249b720c 100644
>> --- a/builtin/ls-remote.c
>> +++ b/builtin/ls-remote.c
>> @@ -58,6 +58,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>> struct transport *transport;
>> const struct ref *ref;
>> struct ref_array ref_array;
>> + struct ref_sorting *sorting;
>> struct string_list sorting_options = STRING_LIST_INIT_DUP;
>>
>> struct option options[] = {
>> @@ -141,13 +142,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>> item->symref = xstrdup_or_null(ref->symref);
>> }
>>
>> - if (sorting_options.nr) {
>> - struct ref_sorting *sorting;
>> -
>> - sorting = ref_sorting_options(&sorting_options);
>> - ref_array_sort(sorting, &ref_array);
>> - ref_sorting_release(sorting);
>> - }
>> + sorting = ref_sorting_options(&sorting_options);
>> + ref_array_sort(sorting, &ref_array);
>
> We stopped calling `ref_sorting_release()`. Doesn't that cause us to
> leak memory?
Nice catch, thanks! It should have been moved to the end of this function
(right before the 'ref_array_clear()').
>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index 3182abde27f..9918ba05dec 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -1606,10 +1608,70 @@ test_expect_success 'option override configured sort' '
>> )
>> '
>>
>> +test_expect_success '--no-sort cancels config sort keys' '
>> + test_config -C sort branch.sort "-refname" &&
>> +
>> + (
>> + cd sort &&
>> +
>> + # objecttype is identical for all of them, so sort falls back on
>> + # default (ascending refname)
>> + git branch \
>> + --no-sort \
>> + --sort="objecttype" >actual &&
>
> This test is a bit confusing to me. Shouldn't we in fact ignore the
> configured sorting order as soon as we pass `--sort=` anyway? In other
> words, I would expect the `--no-sort` option to not make a difference
> here. What should make a difference is if you _only_ passed `--no-sort`.
The existing behavior (as demonstrated by this test) is that the command
line sort keys append to, rather than replace, the config-based sort keys. I
don't see any evidence in the commit history to indicate that this was an
intentional design decision, but it's not necessarily incorrect either.
For one, it's not universal in string list options that the command line
replaces the config. There are examples of both approaches to string list
options in other commands:
- in 'git push', specifying '--push-option' on the command line even once
will remove any values set by 'push.pushoption'
- in 'git blame', any values specified with '--ignore-revs-file' are
appended to those set by 'blame.ignorerevsfile'
In the case of 'git (tag|branch)', I can see why users might not want
command line sort keys to completely remove config-based ones. The only time
the config-based keys will come into play is when two entries are identical
w.r.t _all_ of the command line sort keys. In that scenario, I'd expect a
user would want to use their configured defaults to "break the tie" instead
of the hardcoded ascending refname sort. If they do actually want to remove
the config keys, they can set '--no-sort' before their other sort keys.
^ permalink raw reply
* [RFC PATCH 0/3] replay: implement support for writing new objects to a pack
From: Taylor Blau @ 2023-11-07 18:22 UTC (permalink / raw)
To: git
Cc: Jeff King, Patrick Steinhardt, Elijah Newren, Junio C Hamano,
Johannes Schindelin
In-Reply-To: <ZUpepnSCSxL8i96b@nand.local>
(Based on a combination of Christian's cc/git-replay and my
tb/merge-tree-write-pack branches).
This RFC demonstrates extending the new `--write-pack` option that
`merge-tree` recently learned to the `replay` builtin as well.
The approach is as follows:
- write a pack out after each step in the replay operation, so that
subsequent steps may see any new object(s) created during previous
steps
- combine those packs into one before migrating them back into the
main object store
This is accomplished with a combination of the bulk-checkin and
tmp-objdir APIs, with some minor modifications made to when we flush out
and finalize bulk-checkin transactions.
The benefit to this approach is that we bound the number of inodes
required per replayed commit to a constant (by default, 3: one for the
.pack, one for the .idx, and another for the .rev file), instead of
having each operation take an unbounded number of inodes proportional to
the number of new objects created during that step. We also only migrate
a single pack back to the main object store.
In other words, this makes the maximum number of inodes required by
'replay' grow proportional to the number of commits being replayed,
instead of the number of new *objects* created as a result of the replay
operation.
Taylor Blau (3):
merge-ort.c: finalize ODB transactions after each step
tmp-objdir: introduce `tmp_objdir_repack()`
builtin/replay.c: introduce `--write-pack`
Documentation/git-replay.txt | 4 ++++
builtin/replay.c | 18 ++++++++++++++++++
merge-ort.c | 5 ++++-
t/t3650-replay-basics.sh | 37 ++++++++++++++++++++++++++++++++++++
tmp-objdir.c | 13 +++++++++++++
tmp-objdir.h | 6 ++++++
6 files changed, 82 insertions(+), 1 deletion(-)
--
2.42.0.446.g0b9ef90488
^ permalink raw reply
* [RFC PATCH 1/3] merge-ort.c: finalize ODB transactions after each step
From: Taylor Blau @ 2023-11-07 18:22 UTC (permalink / raw)
To: git
Cc: Jeff King, Patrick Steinhardt, Elijah Newren, Junio C Hamano,
Johannes Schindelin
In-Reply-To: <cover.1699381371.git.me@ttaylorr.com>
In a previous commit, the ORT merge backend learned how to use the
bulk-checkin mechanism to emit a single pack containing any new objects
created during the merge. This functionality was implemented by setting
up a new ODB transaction, and finalizing it at the end of the merge via
`process_entries()`.
In a future commit, we will extend this functionality to the new `git
replay` command, which needs to see objects from previous steps in order
to replay each commit.
As a step towards implementing this, teach the ORT backend to flush the
ODB transaction at the end of each step in `process_entries()`, and then
finalize the result with `end_odb_transaction()` when calling
`merge_finalize()`.
For normal `merge-tree --write-pack` invocations, this produces no
functional change: the pack is written out at the end of
`process_entries()`, and then the `end_odb_transaction()` call is a
noop.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
merge-ort.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/merge-ort.c b/merge-ort.c
index 523577d71e..7b352451cc 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4354,7 +4354,7 @@ static int process_entries(struct merge_options *opt,
ret = -1;
if (opt->write_pack)
- end_odb_transaction();
+ flush_odb_transaction();
cleanup:
string_list_clear(&plist, 0);
@@ -4726,6 +4726,9 @@ void merge_switch_to_result(struct merge_options *opt,
void merge_finalize(struct merge_options *opt,
struct merge_result *result)
{
+ if (opt->write_pack)
+ end_odb_transaction();
+
if (opt->renormalize)
git_attr_set_direction(GIT_ATTR_CHECKIN);
assert(opt->priv == NULL);
--
2.42.0.446.g0b9ef90488
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox