* 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
* 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: 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
* 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
* 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: [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
* 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 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 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 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 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 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/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: 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: 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: Elijah Newren @ 2023-11-07 8:16 UTC (permalink / raw)
To: Sandra Snan; +Cc: git
In-Reply-To: <87cywmintp.fsf@ellen.idiomdrottning.org>
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.
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.
I think it'd be really interesting to at least investigate, but it'd
also be a lot of work, and I already have several other things I've
been wanting to get back to for over a year and haven't succeeded in
generating more time for Git.
Anyway, just my $0.02.
Elijah
^ permalink raw reply
* Re: Git Rename Detection Bug
From: Elijah Newren @ 2023-11-07 8:05 UTC (permalink / raw)
To: Jeremy Pridmore; +Cc: git@vger.kernel.org, Paul Baumgartner
In-Reply-To: <LO6P265MB6736043BE8FB607DB671D21EFAAAA@LO6P265MB6736.GBRP265.PROD.OUTLOOK.COM>
Hi,
On Mon, Nov 6, 2023 at 4:01 AM Jeremy Pridmore <jpridmore@rdt.co.uk> wrote:
>
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
I think it might be worthwhile to point out a few facts about rename
handling in Git, as background information that might clarify a few
things about how Git's mental model seems to differ from yours:
* In git, renames are not tracked; they are detected (based on file
similarity of the commits being compared).
* So, when you run "git mv A B", there is no rename recorded. It's
basically the same as "git rm A", followed by creating B with the same
contents, followed by "git add B".
* The detection happens whenever an operation (diff, log -p, merge,
status, etc.) needs or wants to know about renames.
* In git, directory renames are detected _after_ normal renames, and
via amalgamation of the individual renames.
* As a corollary of the last item, the only way individual renames
can be affected by directory renames, is if the individual rename on
one side of history was into a directory that the other side of
history renamed away; in such a case, we apply an _extra_ rename to
move it into the new directory. But we don't "undo" individual
renames to make them fit the majority-determined directory rename or
anything like that.
(Also, if it matters, all of this is true of both `recursive` and
`ort` merge strategies, i.e. the old default merge backend and the new
one.)
> What did you do before the bug happened? (Steps to reproduce your issue)
> I have two GIT repositories (A and B). Both migrated from the same TFS server using git-tfs tool. I migrated code into A and made lots of changes, including moving 50,000+ files from folder "/Landscape" to "/Landscape/src". B contains the same code but with various other changes made since my original migration from TFS to A. All the files in B are still in the "/Landscape" folder. I recently needed to merge my changes from A to B, so I added A as a remote to B and then performed a number of cherry-picks from A to B, but got stuck when trying to cherry-pick the commit containing the results of moving all files into "/Landscape/src".
In case anyone else wants to dig into this, note that this problem
setup precludes directory rename detection being involved. Directory
rename detection has a rule where if the source directory wasn't
entirely removed on one side, then that directory was not renamed on
that side. Seems obvious, but the upshot of that rule is that a
directory cannot be renamed into a subdirectory of itself, because by
virtue of being a subdirectory that means its parent directory still
exists.
So, this is a problem where only regular rename detection (i.e. rename
detection of individual files) is going to be at play.
> What did you expect to happen? (Expected behavior)
> I expected the git rename detection to match all files in A "/Landscape" to files in B "/Landscape/src".
Are all files under "/Landscape" from the merge base commit
individually more similar to the counterpart under "/Landscape/src"
than to files under any other directory? If not, the expectation goes
against how rename detection has worked in git from the beginning.
> What happened instead? (Actual behavior)
> Although many files were matched successfully, git mismatched over two dozen similarly named files, e.g.
>
> Incorrect path match: Landscape/Services/uiServices/Complaints/Interfaces/IAccountsIntegration.vb -> Landscape/src/Complaints/Rdt.Complaints.UI/Interfaces/IAccountsIntegration.vb
> Incorrect path match: Landscape/Services/uiServices/Complaints/Interfaces/IDocumentIntegration.vb -> Landscape/src/Complaints/Rdt.Complaints.UI/Interfaces/IDocumentIntegration.vb
> Incorrect path match: Landscape/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1 -> Landscape/src/Deployment/PowershellScripts/pre-req/Landscape.Net/pre-req.ps1
> Incorrect path match: Landscape/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1 -> Landscape/src/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1
> Incorrect name match: Landscape/Documentation/Rdt.Documentation.UI/Properties/licenses.licx -> Landscape/src/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1
> Incorrect path match: Landscape/Documentation/uiDocumentation/licenses.licx -> Landscape/src/Documentation/Rdt.Documentation.UI/Properties/licenses.licx
> Incorrect path match: Landscape/Import/uiImport/My Project/licenses.licx -> Landscape/src/Documentation/uiDocumentation/licenses.licx
> Incorrect path match: Landscape/Main/uiMain.Workflow/My Project/licenses.licx -> Landscape/src/Import/uiImport/My Project/licenses.licx
> Incorrect path match: Landscape/Main/uiMain/My Project/licenses.licx -> Landscape/src/Main/uiMain.Workflow/My Project/licenses.licx
> Incorrect path match: Landscape/LandscapeApiService.Setup/Setup/UIContent/RDT_Logo.ico -> Landscape/src/Main/uiMain.Workflow/Resources/RDT_Logo.ico
> Incorrect path match: Landscape/Policy/Rdt.Policy.UI.Templates/Properties/licenses.licx -> Landscape/src/Main/uiMain/My Project/licenses.licx
> Incorrect path match: Landscape/Main/uiMain.Workflow/Resources/RDT_Logo.ico -> Landscape/src/Main/uiMain/Resources/RDT_Logo.ico
> Incorrect path match: Landscape/Policy/Rdt.Policy.UI/Properties/licenses.licx -> Landscape/src/Policy/Rdt.Policy.UI.Templates/Properties/licenses.licx
> Incorrect path match: Landscape/Rates/uiRates/My Project/licenses.licx -> Landscape/src/Policy/Rdt.Policy.UI/Properties/licenses.licx
> Incorrect path match: Landscape/Rdt.Claim.UI/Properties/licenses.licx -> Landscape/src/Rates/uiRates/My Project/licenses.licx
> Incorrect path match: Landscape/Rdt.Landscape.UI.Templates.Workflow/Properties/licenses.licx -> Landscape/src/Rdt.Claim.UI/Properties/licenses.licx
> Incorrect path match: Landscape/Rdt.Landscape.UI.Templates/Properties/licenses.licx -> Landscape/src/Rdt.Landscape.UI.Templates.Workflow/Properties/licenses.licx
> Incorrect path match: Landscape/Rdt.Landscape.UI.Workflow/Properties/licenses.licx -> Landscape/src/Rdt.Landscape.UI.Templates/Properties/licenses.licx
> Incorrect path match: Landscape/Rdt.Landscape.UI/Properties/licenses.licx -> Landscape/src/Rdt.Landscape.UI.Workflow/Properties/licenses.licx
> Incorrect path match: Landscape/StandardLetters/uiStandardLetters/My Project/licenses.licx -> Landscape/src/Rdt.Landscape.UI/Properties/licenses.licx
> Incorrect path match: Landscape/Complaints/Rdt.Complaints.UI/Interfaces/IDocumentIntegration.vb -> Landscape/src/Services/uiServices/Complaints/Interfaces/IDocumentIntegration.vb
> Incorrect path match: Landscape/SystemEvents/uiSystemEvents/My Project/licenses.licx -> Landscape/src/StandardLetters/uiStandardLetters/My Project/licenses.licx
> Incorrect path match: Landscape/Services/busServices/RDT_Logo.ico -> Landscape/src/Startup/uiStartup.Workflow/Resources/RDT_Logo.ico
> Incorrect path match: Landscape/Startup/uiStartup.Workflow/Resources/RDT_Logo.ico -> Landscape/src/Startup/uiStartup/Resources/RDT_Logo.ico
> Incorrect path match: Landscape/Startup/uiStartup/Resources/RDT_Logo.ico -> Landscape/src/Startup/uiStartup32/RDT_Logo.ico
> Incorrect path match: Landscape/Startup/uiStartup/Resources/newrdlogogradiant48shad.ico -> Landscape/src/Startup/uiStartup32/newrdlogogradiant48shad.ico
> Incorrect path match: Landscape/Templates/uiTemplates.Workflow/My Project/licenses.licx -> Landscape/src/SystemEvents/uiSystemEvents/My Project/licenses.licx
> Incorrect path match: Landscape/Utils/Rdt.Utils.UI/Properties/licenses.licx -> Landscape/src/Templates/uiTemplates.Workflow/My Project/licenses.licx
> Incorrect path match: Landscape/Utils/uiUtils/My Project/licenses.licx -> Landscape/src/Utils/Rdt.Utils.UI/Properties/licenses.licx
> Incorrect name match: Landscape/WebServices/ServiceFabric/Policy/Rdt.Policy.Repository.Service.Fabric.Host/PackageRoot/Data/Swagger/Examples/POST_UKSTasks_Response.json -> Landscape/src/Utils/uiUtils/My Project/licenses.licx
>
>
> What's different between what you expected and what actually happened?
>
> As you can see, although the filenames (and content) are the same,
The content is the same as well? So, these renames that you label as
incorrect are actually _exact_ renames -- and further, in most cases
they also have an identical basename for the file as well.
Exact renames are detected first, before any other method of rename
detection is employed, and other than giving a preference to files
with the same basename, if there are multiple choices it just picks
one (what I'd call at random, though technically based on what the
internal processing order happens to be; see the "Too many identical
alternatives? Pick one" code comment).
And this, too, is true of both the `recursve` and `ort` backends; no
change has been made to how exact renames are handled.
> In some cases, it seems that the catalyst has been git thinking that a file from B has been deleted from A, when in fact it has not actually been deleted at all.
>
> For example, the file Landscape/Deployment/PowershellScripts/pre-req/Landscape.Net/pre-req.ps1 has not been deleted in A or B, therefore git should not have attempted to rename Landscape/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1 to Landscape/Deployment/PowershellScripts/pre-req/Landscape.Net/pre-req.ps1, especially as it then attempts to rename Landscape/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1 to Landscape/src/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1 and so on.
Renamed files, from Git's perspective, always involve files that have
been deleted.
> Git status contains, for example:
> deleted by them: Landscape/Deployment/PowershellScripts/pre-req/Landscape.Net/pre-req.ps1
This means that it wasn't sufficiently similar to any of the new
files...or that _other_ deleted files were more similar to the new
files and thus that they were paired up instead of this file, leaving
this file to simply be marked as deleted. (Or that other deleted
files were just as similar; tie-breakers are kinda random in such a
case.)
[...]
> Anything else you want to add:
> I can't help but think that this is related to changes made by Palantir:
> https://blog.palantir.com/optimizing-gits-merge-machinery-1-127ceb0ef2a1
Curious. What makes you think it's related?
If there is some reason you think it's related, there's an easy way to
check -- just repeat the cherry-pick with the "-s recursive" flag to
use the old merge backend and compare the results.
I'll be somewhat surprised if it's related, though.
> I have tried to unstage these renames using "git restore --staged <file_name>" so I can then apply the correct "git mv" commands
Why? Just modify all the files to have the correct end results and then commit.
>, but bizzarely, this then results in "git status" reporting a different, smaller set of mismatched names:
As mentioned earlier, git does _not_ record renames. So, running the
correct "git mv" command doesn't really mean much. If you use
completely "incorrect" git-mv commands, but then manually tweak files
until they have the correct results, then what's recorded is exactly
the same as if you had used the "correct" git-mv commands.
Further, when you run "git status", it can't access any renames you
did because that information isn't recorded anywhere. It instead
recomputes renames on the fly. And it does so each and every time you
run "git status", even if you make no changes between two invocations.
In fact, from this you can probably also deduce that there are other
ways to affect what will be shown as renames, when you have multiple
files similar to any given source file. In particular, you can cause
a different pairing modifying one of the similar files enough that it
becomes the most similar to the source file, or so that it becomes no
longer the most similar to the source file. However, what "git
status" reports for renames is irrelevant, since that info won't be
recorded in the commit. Renames are never recorded. Anywhere.
In fact, you can even run "git status --no-renames" to just see the
old filenames that were removed and the new ones that were added
without having all the files be paired up as renames.
Hope that helps,
Elijah
^ permalink raw reply
* Re: Regression: git send-email Message-Id: numbering doesn't start at 1 any more
From: Uwe Kleine-König @ 2023-11-07 7:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Strawbridge, Douglas Anderson, git, entwicklung
In-Reply-To: <xmqqwmuucwi9.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 4696 bytes --]
Hello Junio,
On Tue, Nov 07, 2023 at 08:06:22AM +0900, Junio C Hamano wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>
> > Hello,
> >
> > Since commit 3ece9bf0f9e24909b090cf348d89e8920bd4f82f I experience that
> > the generated Message-Ids don't start at ....-1-... any more. I have:
> >
> > $ git send-email w/*
> > ...
> > Subject: [PATCH 0/5] watchdog: Drop platform_driver_probe() and convert to platform remove callback returning void (part II)
> > Date: Mon, 6 Nov 2023 16:10:04 +0100
> > Message-ID: <20231106151003.3844134-7-u.kleine-koenig@pengutronix.de>
> > ...
> >
> > So the cover letter is sent with Message-Id: ...-7-...
>
> The above is consistent with the fact that a 5-patch series with a
> cover letter consists of 6 messages. Dry-run uses message numbers
> 1-6 and forgets to reset the counter, so the next message becomes 7.
> As you identified, the fix in 3ece9bf0 (send-email: clear the
> $message_id after validation, 2023-05-17) for the fallout from an
> even earlier change to process each message twice still had left an
> observable side effect subject to the Hyrum's law, it seems.
>
> > +my ($message_id_stamp, $message_id_serial);
> > if ($validate) {
> > # FIFOs can only be read once, exclude them from validation.
> > my @real_files = ();
> > @@ -821,6 +822,7 @@ sub is_format_patch_arg {
> > }
> > delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
> > delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
> > + $message_id_serial = 0;
> > }
>
> This fix looks quite logical to me, but even with this, the side
> effects of the earlier "read message twice" persists in end-user
> observable form, don't they? IIRC, when sending out an N message
> series, we start from the timestamp as of N seconds ago and give
> each message the Date: header that increments by 1 second, which
> would mean the validator will see Date: that is different from what
> will actually be sent out, and more importantly, the messages sent
> out for real will have timestamps from the future, negating the
> point of starting from N seconds ago in the first place.
The series I used as an example here was finally sent out with
git-send-email patched with my suggested change.
The Message-Ids involved are:
20231106154807.3866712-1-u.kleine-koenig@pengutronix.de
20231106154807.3866712-2-u.kleine-koenig@pengutronix.de
20231106154807.3866712-3-u.kleine-koenig@pengutronix.de
20231106154807.3866712-4-u.kleine-koenig@pengutronix.de
20231106154807.3866712-5-u.kleine-koenig@pengutronix.de
20231106154807.3866712-6-u.kleine-koenig@pengutronix.de
So the Ids are are identical apart from the number this report is about.
> Your script may not have been paying attention to it and only noticed
> the difference in id_serial, but somebody else would complain the
> difference coming from calling gen_header more than once for each
> messages since a8022c5f (send-email: expose header information to
> git-send-email's sendemail-validate hook, 2023-04-19).
>
> So, I dunno. Michael, what do you think? It appears to me that a
> more fundamental fix to the fallout from a8022c5f might be needed
> (e.g., we still let gen_header run while validating, but once
> validation is done, save the headers that validator saw and use them
> without calling gen_header again when we send the messages out, or
> something), if we truly want to be regression free.
That sounds sane.
> By the way, out of curiosity, earlier you said your script looks at
> the Message-IDs and counts the number of messages. How does it do
> that? Does it read the output of send-email and pass the messages
> to MTA for sending out for real?
The output of git send-email dumps the messages it sends out and then I
pick the message-id of the last mail by cut-n-paste and call my script
with that as a parameter. It then adds notes to the $commitcount topmost
commits such that I have something like this on the sent out commits:
Notes:
Forwarded: id:20231106154807.3866712-2-u.kleine-koenig@pengutronix.de (v1)
This is very convenient to find the thread to ping if a patch doesn't
make it into the next release.
(By the way, one difficulty in my script is that depending on the series
having a cover letter or not I have to apply the id:....-1-... marker or
not. Would be great if git send-email started with ...-0-... for a
series with a cover letter. Detecting that reliably isn't trivial I
guess.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 0/2] revision: exclude all packed objects with `--unpacked`
From: Jeff King @ 2023-11-07 4:02 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1699311386.git.me@ttaylorr.com>
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
^ permalink raw reply
* Re: [PATCH 1/2] list-objects: drop --unpacked non-commit objects from results
From: Jeff King @ 2023-11-07 4:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
In-Reply-To: <xmqq7cmub8wm.fsf@gitster.g>
On Tue, Nov 07, 2023 at 11:21:29AM +0900, Junio C Hamano wrote:
> > In git-rev-list(1), we describe the `--unpacked` option as:
> >
> > Only useful with `--objects`; print the object IDs that are not in
> > packs.
> >
> > This is true of commits, which we discard via get_commit_action(), but
> > not of the objects they reach. So if we ask for an --objects traversal
> > with --unpacked, we may get arbitrarily many objects which are indeed
> > packed.
>
> Strictly speaking, as long as all the objects that are not in packs
> are shown, "print the object IDs that are not in packs" is satisfied.
> With this fix, perhaps we would want to tighten the explanation a
> little bit while we are at it. Perhaps
>
> print the object names but exclude those that are in packs
>
> or something along that line?
I think using the word "exclude" is a good idea, as it makes it clear
that we are omitting objects that otherwise would be traversed (as
opposed to just showing unpacked objects, reachable or not).
But I wanted to point out one other subtlety here. The existing code
(before this patch) checks for already-packed commits, and avoids adding
them to the traversal. The problem this patch is fixing is that we may
see objects they point to via other non-packed commits. But the opposite
problem exists, too: we have unpacked objects that are reachable from
those packed commits.
It's probably reasonably rare, since we _tend_ to make packs by rolling
up reachable chunks of history. But that's not a guarantee. One way I
can think of for it to happen in practice is that somebody pushes (or
fetches) a thin pack with commit C as a delta against an unpacked C'. In
that case "index-pack --fix-thin" will create a duplicate of C' in the
new pack, but its trees and blobs may remain unpacked.
I think with the patch in this series we could actually drop that "do
not traverse commits that are unpacked" line of code, and end up "more
correct". But I suspect performance of an incremental "git repack -d"
would suffer. This is kind of analagous to the "we do not traverse every
UNINTERESTING commit just to mark its trees/blobs as UNINTERESTING"
optimization. We know that it is not a true set difference, but it is OK
in practice and it buys us a lot of performance. And just like that
case, bitmaps do let us cheaply compute the true set difference. ;)
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] pack-bitmap: drop --unpacked non-commit objects from results
From: Jeff King @ 2023-11-07 3:52 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <7492dc699052a392d2fb394e1dcfabebac82ded0.1699311386.git.me@ttaylorr.com>
On Mon, Nov 06, 2023 at 05:56:33PM -0500, Taylor Blau wrote:
> Before returning a bitmap containing the result of the traversal back to
> the caller, drop any bits from the extended index which appear in one or
> more packs. This implements the correct behavior for rev-list operations
> which use the bitmap index to compute their result.
Nice. I was very happy to see the extra test for "rev-list --disk-usage"
to demonstrate this. The same should apply for "pack-objects --unpacked
--use-bitmap-index" (though in practice I don't think we'd do that, as
"--unpacked" implies on-disk repacking, which we do not generally use
with bitmaps).
-Peff
^ permalink raw reply
* Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together
From: Jeff King @ 2023-11-07 3:42 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Taylor Blau, git, Elijah Newren,
Eric W. Biederman, 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:
> 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)?
>
> What I have in mind is more along Elijah's suggestion at the Contributor
> Summit to use the `tmp_objdir*()` machinery. But instead of discarding the
> temporary object database, the contained objects would be repacked and the
> `.pack`, (maybe `.rev`) and the `.idx` file would then be moved (in that
> order) before discarding the temporary object database.
>
> This would probably need to be implemented as a new
> `tmp_objdir_pack_and_migrate()` function that basically spawns
> `pack-objects` and feeds it the list of generated objects, writing
> directly into the non-temporary object directory, then discarding the
> `tmp_objdir`.
Perhaps I'm missing some piece of the puzzle, but I'm not sure what
you're trying to accomplish with that approach.
If the goal is to increase performance by avoiding the loose object
writes, then we haven't really helped much. We're still writing them,
and then writing them again for the repack.
If the goal is just to end up with a single nice pack for the long term,
then why do we need to use tmp_objdir at all? That point of that API is
to avoid letting other simultaneous processes see the intermediate state
before we're committed to keeping the objects around. That makes sense
for receiving a fetch or push, since we want to do some quality checks
on the objects before agreeing to keep them. But does it make sense for
a merge? Sure, in some workflows (like GitHub's test merges) we might
end up throwing away the merge result if it's not clean. But there is no
real downside to other processes seeing those objects. They can be
cleaned up at the next pruning repack.
I guess if your scenario requirements include "and we are never allowed
to run a pruning repack", then that could make sense. And I know that
has been a historical issue for GitHub. But I'm not sure it's
necessarily a good driver for an upstream feature.
As an alternative, though, I wonder if you need to have access to the
objects outside of the merge process. If not, then rather than an
alternate object store, what if that single process wrote to a streaming
pack _and_ used its running in-core index of the objects to allow access
via the usual object-retrieval. Then you'd get a single, clean pack as
the outcome _and_ you'd get the performance boost over just "write loose
objects, repack, and prune".
-Peff
^ permalink raw reply
* Re: Bug: magic-less pathspecs that start with ":" not processed as expected.
From: Jeff King @ 2023-11-07 3:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Joanna Wang, git
In-Reply-To: <xmqqfs1icvfl.fsf@gitster.g>
On Tue, Nov 07, 2023 at 08:29:34AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > PS It took me a while to figure out where we document pathspec syntax. I
> > wonder if a "gitpathspecs" manpage would make sense, like we have
> > "gitrevisions".
>
> Yeah, I came to the same conclusion (should have saved time by
> scanning the mailing list before I started writing my response) and
> wondered where we wrote it down. The description you found in the
> glossary, as far as I recall, is the authoritative one and looks
> readable, but I agree it is not as discoverable as it should be.
>
> A simpler and more readable workaround than ":::file" is "./:file"
> by the way ;-)
Oh, indeed. That is much less horrible than ":(literal)". :)
-Peff
^ permalink raw reply
* Re: [PATCH] diff: implement config.diff.renames=copies-harder
From: Junio C Hamano @ 2023-11-07 3:10 UTC (permalink / raw)
To: Elijah Newren; +Cc: Sam James via GitGitGadget, git, Sam James
In-Reply-To: <CABPp-BEuvjduS4JiORJybKtoPWvJd+BbbR_JAvZdj4Px_v8H4A@mail.gmail.com>
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?
>> 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?
^ permalink raw reply
* Re: [PATCH 0/9] for-each-ref optimizations & usability improvements
From: Junio C Hamano @ 2023-11-07 3:04 UTC (permalink / raw)
To: Victoria Dye; +Cc: Victoria Dye via GitGitGadget, git
In-Reply-To: <dbcbcf0e-aeee-4bb9-9e39-e2e85194d083@github.com>
Victoria Dye <vdye@github.com> writes:
> Ah sorry, I over-edited my cover letter and accidentally removed the
> explanation of what this patch does! Yes - the new behavior is that
> '--no-sort' (assuming there are no subsequent --sort=<something> options)
> will completely skip sorting the filtered refs.
Makes sense.
And the way to countermand "--no-sort" that appears earlier on the
command line to revert to the default sort order is "--sort" that
uses "refname" as the sort key, which is also nice.
^ 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