* 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
* [RFC PATCH 2/3] tmp-objdir: introduce `tmp_objdir_repack()`
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 the following commit, we will teach `git replay` how to write a pack
containing the set of new objects created as a result of the `replay`
operation.
Since `replay` needs to be able to see the object(s) written
from previous steps in order to replay each commit, the ODB transaction
may have multiple pending packs. Migrating multiple packs back into the
main object store has a couple of downsides:
- It is error-prone to do so: each pack must be migrated in the
correct order (with the ".idx" file staged last), and the set of
packs themselves must be moved over in the correct order to avoid
racy behavior.
- It is a (potentially significant) performance degradation to migrate
a large number of packs back into the main object store.
Introduce a new function that combines the set of all packs in the
temporary object store to produce a single pack which is the logical
concatenation of all packs created during that level of the ODB
transaction.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
tmp-objdir.c | 13 +++++++++++++
tmp-objdir.h | 6 ++++++
2 files changed, 19 insertions(+)
diff --git a/tmp-objdir.c b/tmp-objdir.c
index 5f9074ad1c..ef53180b47 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -12,6 +12,7 @@
#include "strvec.h"
#include "quote.h"
#include "object-store-ll.h"
+#include "run-command.h"
struct tmp_objdir {
struct strbuf path;
@@ -277,6 +278,18 @@ int tmp_objdir_migrate(struct tmp_objdir *t)
return ret;
}
+int tmp_objdir_repack(struct tmp_objdir *t)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+
+ cmd.git_cmd = 1;
+
+ strvec_pushl(&cmd.args, "repack", "-a", "-d", "-k", "-l", NULL);
+ strvec_pushv(&cmd.env, tmp_objdir_env(t));
+
+ return run_command(&cmd);
+}
+
const char **tmp_objdir_env(const struct tmp_objdir *t)
{
if (!t)
diff --git a/tmp-objdir.h b/tmp-objdir.h
index 237d96b660..d00e3b3e27 100644
--- a/tmp-objdir.h
+++ b/tmp-objdir.h
@@ -36,6 +36,12 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix);
*/
const char **tmp_objdir_env(const struct tmp_objdir *);
+/*
+ * Combines all packs in the tmp_objdir into a single pack before migrating.
+ * Removes original pack(s) after installing the combined pack into place.
+ */
+int tmp_objdir_repack(struct tmp_objdir *);
+
/*
* Finalize a temporary object directory by migrating its objects into the main
* object database, removing the temporary directory, and freeing any
--
2.42.0.446.g0b9ef90488
^ permalink raw reply related
* [RFC PATCH 3/3] builtin/replay.c: introduce `--write-pack`
From: Taylor Blau @ 2023-11-07 18:23 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>
Now that the prerequisites are in place, we can implement a
`--write-pack` option for `git replay`, corresponding to the existing
one in `git merge-tree`.
The changes are mostly limited to:
- introducing a new option in the builtin
- replacing the main object store with the temporary one
- then repacking and migrating the temporary object store back into
the main object store after the replay has completed
Along with tests and documentation to ensure that the new behavior
matches our expectations.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Documentation/git-replay.txt | 4 ++++
builtin/replay.c | 18 ++++++++++++++++++
t/t3650-replay-basics.sh | 37 ++++++++++++++++++++++++++++++++++++
3 files changed, 59 insertions(+)
diff --git a/Documentation/git-replay.txt b/Documentation/git-replay.txt
index e7551aec54..f424c1a676 100644
--- a/Documentation/git-replay.txt
+++ b/Documentation/git-replay.txt
@@ -42,6 +42,10 @@ When `--advance` is specified, the update-ref command(s) in the output
will update the branch passed as an argument to `--advance` to point at
the new commits (in other words, this mimics a cherry-pick operation).
+--write-pack::
+ Write any new objects into a separate packfile instead of as
+ individual loose objects.
+
<revision-range>::
Range of commits to replay. More than one <revision-range> can
be passed, but in `--advance <branch>` mode, they should have
diff --git a/builtin/replay.c b/builtin/replay.c
index c3d53ff0cd..72b7b7f43a 100644
--- a/builtin/replay.c
+++ b/builtin/replay.c
@@ -17,6 +17,7 @@
#include "strmap.h"
#include <oidset.h>
#include <tree.h>
+#include "tmp-objdir.h"
static const char *short_commit_name(struct commit *commit)
{
@@ -272,6 +273,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
struct commit *onto = NULL;
const char *onto_name = NULL;
int contained = 0;
+ int write_pack = 0;
struct rev_info revs;
struct commit *last_commit = NULL;
@@ -279,6 +281,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
struct merge_options merge_opt;
struct merge_result result;
struct strset *update_refs = NULL;
+ struct tmp_objdir *tmp_objdir = NULL;
kh_oid_map_t *replayed_commits;
int i, ret = 0;
@@ -296,6 +299,8 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
N_("replay onto given commit")),
OPT_BOOL(0, "contained", &contained,
N_("advance all branches contained in revision-range")),
+ OPT_BOOL(0, "write-pack", &write_pack,
+ N_("write new objects to a pack instead of as loose")),
OPT_END()
};
@@ -352,8 +357,15 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
init_merge_options(&merge_opt, the_repository);
memset(&result, 0, sizeof(result));
merge_opt.show_rename_progress = 0;
+ merge_opt.write_pack = write_pack;
last_commit = onto;
replayed_commits = kh_init_oid_map();
+
+ if (merge_opt.write_pack) {
+ tmp_objdir = tmp_objdir_create("replay");
+ tmp_objdir_replace_primary_odb(tmp_objdir, 0);
+ }
+
while ((commit = get_revision(&revs))) {
const struct name_decoration *decoration;
khint_t pos;
@@ -417,5 +429,11 @@ int cmd_replay(int argc, const char **argv, const char *prefix)
/* Return */
if (ret < 0)
exit(128);
+ if (ret && tmp_objdir) {
+ if (tmp_objdir_repack(tmp_objdir) < 0)
+ ret = 0;
+ else if (tmp_objdir_migrate(tmp_objdir) < 0)
+ ret = 0;
+ }
return ret ? 0 : 1;
}
diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh
index 389670262e..e7048748c2 100755
--- a/t/t3650-replay-basics.sh
+++ b/t/t3650-replay-basics.sh
@@ -67,6 +67,43 @@ test_expect_success 'using replay to rebase two branches, one on top of other' '
test_cmp expect result
'
+packdir=.git/objects/pack
+
+test_expect_success 'using replay to rebase two branches, with --write-pack' '
+ # Remove the results of the previous rebase, ensuring that they
+ # are pruned from the object store.
+ git gc --prune=now &&
+ test_must_fail git cat-file -t "$(cut -d " " -f 3 expect)" &&
+
+ # Create an extra packfile to ensure that the tmp-objdir repack
+ # takes place outside of the main object store.
+ git checkout --detach &&
+ test_commit unreachable &&
+ git repack -d &&
+ git checkout main &&
+
+ find $packdir -type f -name "*.idx" | sort >packs.before &&
+ git replay --write-pack --onto main topic1..topic2 >result &&
+ find $packdir -type f -name "*.idx" | sort >packs.after &&
+
+ comm -13 packs.before packs.after >packs.new &&
+
+ # Ensure that we got a single new pack.
+ test_line_count = 1 result &&
+ test_line_count = 1 packs.new &&
+
+ # ... and that the rest of the results match our expeectations.
+ git log --format=%s $(cut -f 3 -d " " result) >actual &&
+ test_write_lines E D M L B A >expect &&
+ test_cmp expect actual &&
+
+ printf "update refs/heads/topic2 " >expect &&
+ printf "%s " $(cut -f 3 -d " " result) >>expect &&
+ git rev-parse topic2 >>expect &&
+
+ test_cmp expect result
+'
+
test_expect_success 'using replay on bare repo to rebase two branches, one on top of other' '
git -C bare replay --onto main topic1..topic2 >result-bare &&
test_cmp expect result-bare
--
2.42.0.446.g0b9ef90488
^ permalink raw reply related
* Re: git-send-email: Send with mutt(1)
From: Alejandro Colomar @ 2023-11-07 18:36 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231107174803.GA507007@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 3961 bytes --]
Hi Jeff,
On Tue, Nov 07, 2023 at 12:48:03PM -0500, Jeff King wrote:
> 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:
Yeah, most of it is done by format-patch. There are few things I
actually need from send-email. One of them is generating the Cc from
the sign-offs and other tags found in the patch.
I had been thinking these days that it would be useful to have
format-patch generate those. How about adding a --signed-off-by-cc to
format-patch?
>
> 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
Indeed, that hackery is what send-email already does, so how about
moving those features a bit upstream so that format-patch can do them
too?
Although then, maybe it's simpler to teach send-email to learn to use
mutt(1) under the hood for the actual send.
> 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
Thanks! I'll try it. Although I don't know perl, so I hope I don't
need to tweak it much. :)
Cheers,
Alex
--
<https://www.alejandro-colomar.es/>
[-- 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: Victoria Dye @ 2023-11-07 18:41 UTC (permalink / raw)
To: Patrick Steinhardt, Victoria Dye via GitGitGadget; +Cc: git
In-Reply-To: <ZUoWT8GyrZlvH_Go@tanuki>
Patrick Steinhardt wrote:
> 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.
While that's a good guideline to keep in mind, it's not universally
applicable. In this case, (almost) all of the changes are done the same way,
focused on the same goal: extract bits of 'filter_refs()' into generic,
internal helpers so we can use those bits elsewhere in later patches.
Splitting those extractions into multiple patches would essentially lead to
a handful of very small patches that more-or-less have the same commit
message. As I mentioned in [1], I think there's value to having the
immediate context of related changes in a single patch (as long as that
single patch doesn't become unwieldy), so I'm not inclined to split this up.
That said, I did say "(almost) all" of the changes are conceptually similar.
Looking at this now, the rename of 'ref_filter_handler()' => 'filter_one()'
doesn't really fit the "extract into helper functions" theme of the rest of
the patch, I'll pull that out into its own.
[1] https://lore.kernel.org/git/a833b5a7-0201-4c2e-8821-f2a1930cb403@github.com/
^ permalink raw reply
* Re: [PATCH 2/9] for-each-ref: clarify interaction of --omit-empty & --count
From: Øystein Walle @ 2023-11-07 19:23 UTC (permalink / raw)
To: gitgitgadget; +Cc: git, vdye, Øystein Walle
In-Reply-To: <88eba4146cd250fcabfb9ffa9b410ce912a82ce7.1699320362.git.gitgitgadget@gmail.com>
Hi Victoria,
Victoria Dye <vdye@github.com> writes:
> Update the 'for-each-ref' builtin documentation to clarify that refs
> "omitted" by --omit-empty are still counted toward the limit specified
> by --count. The use of the term "omit" would otherwise be somewhat
> ambiguous and could incorrectly be construed as excluding empty refs
> entirely (i.e. not counting them towards the total ref count).
I implemented --omit-empty and I completely overlooked --count!
(If I were to do it all over again I probably would have implemented it
so that so-called omitted refs did not count towards the total. It makes
sense to me since e.g. `git log -3 -- git.c` prints the three most
recent commits that touch git.c regardless of how many commits were
walked in the process.)
This is a good and welcome clarification.
Acked-by: Øystein Walle <oystwa@gmail.com>
^ permalink raw reply
* Re: [PATCH 2/9] for-each-ref: clarify interaction of --omit-empty & --count
From: Victoria Dye @ 2023-11-07 19:30 UTC (permalink / raw)
To: Øystein Walle, gitgitgadget; +Cc: git
In-Reply-To: <20231107192326.48296-1-oystwa@gmail.com>
Øystein Walle wrote:
> Hi Victoria,
>
> Victoria Dye <vdye@github.com> writes:
>
>> Update the 'for-each-ref' builtin documentation to clarify that refs
>> "omitted" by --omit-empty are still counted toward the limit specified
>> by --count. The use of the term "omit" would otherwise be somewhat
>> ambiguous and could incorrectly be construed as excluding empty refs
>> entirely (i.e. not counting them towards the total ref count).
>
> I implemented --omit-empty and I completely overlooked --count!
>
> (If I were to do it all over again I probably would have implemented it
> so that so-called omitted refs did not count towards the total. It makes
> sense to me since e.g. `git log -3 -- git.c` prints the three most
> recent commits that touch git.c regardless of how many commits were
> walked in the process.)
Since the interaction isn't clearly defined at the moment, we could probably
still update it to work like you're describing here. I'm happy to drop this
patch and implement your recommendation in a follow-up series. Let me know
what you think!
>
> This is a good and welcome clarification.
>
> Acked-by: Øystein Walle <oystwa@gmail.com>
^ permalink raw reply
* Re: [PATCH 7/9] ref-filter.c: filter & format refs in the same callback
From: Victoria Dye @ 2023-11-07 19:45 UTC (permalink / raw)
To: Patrick Steinhardt, Victoria Dye via GitGitGadget; +Cc: git
In-Reply-To: <ZUoWVPSE1GcJdHFE@tanuki>
Patrick Steinhardt wrote:
>> 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.
I'll update it to 1, and also add a comment that the non-zero return value
stops iteration since it's not immediately clear from other 'each_ref_fn's
what that means. For reference, there appears to only be one other
'each_ref_fn' that even has the potential to return a nonzero return value
('ref_present()' in 'refs/files-backend.c).
>
>> + 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?
Sure, I'll convert it to a bulleted list. I don't really expect it to change
much, though; to have any effect on this condition, the new filter/format
would need to act on the pre-filtered ref_array, which isn't particularly
common.
And yes, the existing tests cover scenarios where this function returns true
(e.g. 'git for-each-ref --no-sort') & where it returns false (essentially
anything else).
>
> 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.
I'll add more detailed comments to clarify what's going on here.
In practice, though, I don't think this would be all that easy to miss. As I
noted above, the only filters/formats that affect this are ones that need to
loop over an entire filtered ref_array after the initial
'for_each_fullref_in()'. To have it actually apply to commands that use
'filter_and_format_refs()', they'll need to add that behavior here (like
'filter_ahead_behind()'), where it should be apparent that
'can_do_iterative_format()' is relevant to their change.
>
> Patrick
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Jeff King @ 2023-11-07 20:16 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: git
In-Reply-To: <ZUqDwnmu9d1dD1tb@devuan>
On Tue, Nov 07, 2023 at 07:36:44PM +0100, Alejandro Colomar wrote:
> > 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:
>
> Yeah, most of it is done by format-patch. There are few things I
> actually need from send-email. One of them is generating the Cc from
> the sign-offs and other tags found in the patch.
>
> I had been thinking these days that it would be useful to have
> format-patch generate those. How about adding a --signed-off-by-cc to
> format-patch?
That seems like a reasonable feature. Probably it should be
--cc-from-trailer=signed-off-by, and then you could do the same with
other trailers.
It feels like you could _almost_ do it with the existing
--format='%(trailers)' functionality, but there's no way to say "do the
regular --format=email output, but also stick this extra format in the
headers section". Plus there are probably some niceties you'd get from
Git knowing that you're adding headers (like de-duping addresses).
That feature might end up somewhat hairy, though, as then you get into
questions of parsing address lists, etc. We do all that now in perl with
send-email, where we can lean on some parsing libraries. So I dunno.
> > 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
>
> Indeed, that hackery is what send-email already does, so how about
> moving those features a bit upstream so that format-patch can do them
> too?
Yeah, if they existed in format-patch I might be able to reuse them. I
am hesitant, though, just because handling all the corner cases on
parsing is going to be a bit of new C code.
> Although then, maybe it's simpler to teach send-email to learn to use
> mutt(1) under the hood for the actual send.
I think you will find some corner cases in trying to make mutt act just
like an mta accepting delivery. Two I can think of:
1. It will take a body on stdin, but not a whole message. We can hack
around that with some postponed-folder magic, though.
2. Bcc headers are stripped before sendmail sees the message (but
those addresses appear on the command-line). Converting that back
to bcc so that mutt can then re-strip them would be annoying but
possible. If you don't use bcc, it probably makes sense to just
punt on this.
So maybe a script like this:
-- >8 --
#!/bin/sh
# ignore arguments; mutt will parse them itself
# from to/cc headers. Note that we'll miss bcc this
# way, but handling that would probably be kind of
# tricky; we'd need to re-add those recipients as actual
# bcc headers so that mutt knows how to handle them.
# spool the message to a fake mbox; we need to add
# a "From" line to make it look legit
trap 'rm -f to-send' 0 &&
{
echo "From whatever Mon Sep 17 00:00:00 2001" &&
cat
} >to-send &&
# and then have mutt "resume" it. We have to redirect
# stdin back from the terminal, since ours is a pipe
# with the message contents.
mutt -p \
-e 'set postponed=to-send' \
-e 'set edit_headers=yes' \
</dev/tty
-- 8< --
and then in your git config:
[sendemail]
sendmailcmd = /path/to/mutt-as-mta.sh
There are mutt-specific bits there that I don't think send-email should
have to know about. Perhaps there are generic options that send-email
could learn, but it really feels like you'd do better teaching mutt to
be more ready to handle this (like taking a whole message on stdin,
headers and all, rather than just a body).
-Peff
^ permalink raw reply
* Tender Enquiry | UAE-Oman Railway Link Project
From: P.V. Soma Sekhar @ 2023-11-07 14:32 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 935 bytes --]
Sayın kullanıcı,
Umarım bu mesaj sizi iyi bulur.
Aşağıdaki malzemeler için teklifinizi istiyoruz,
Ekte Sipariş örneğimiz ve şirket bilgilerimiz bulunmaktadır,
Lütfen bize en son kataloğunuzu rekabetçi fiyatlarla gönderin.
1'den 4'e kadar olan maddelere acilen ihtiyacımız var, lütfen müsaitlik durumunu belirtin.
Acil bir soruşturmamız olduğu için nazik ve acil geri bildiriminizi bekliyoruz.
Hızlı yanıtınız takdir edilecektir.
Best Regards,
P.V.SOMA SEKHAR
Project Manager
Telecom Infrastructure Project Business Unit
Technology & Communication Division
Infrastructure, Technology, Industrial & Consumer Solutions
MOHSIN HAIDER DARWISH LLC
P. O. Box 880, Ruwi – 112,
Muscat, Sultanate of Oman
Office: +968 24835500
Fax : +968 24833369
Mob. : + 968-98632229
soma.s.@mhd.co.om mailto:soma.s.@mhd.co.om
www.mhdoman.com http://www.mhdoman.com/
[-- Attachment #1.2: Type: text/html, Size: 11280 bytes --]
[-- Attachment #2: Tender Enquiry UAE-Oman Railway Link Project.xlam --]
[-- Type: application/octet-stream, Size: 689376 bytes --]
^ permalink raw reply
* Re: [PATCH 8/9] for-each-ref: add option to fully dereference tags
From: Victoria Dye @ 2023-11-08 1:13 UTC (permalink / raw)
To: Patrick Steinhardt, Victoria Dye via GitGitGadget; +Cc: git
In-Reply-To: <ZUoWWo7IEKsiSx-C@tanuki>
Patrick Steinhardt wrote:
> 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.
I've been going back and forth on this, but I think a field specifier might
be the way to go after all. Using a field specifier would inherently be more
complex than the command line option (since the formatting code is a bit
complicated), but that's not an insurmountable problem. The thing I kept
getting caught up on was which symbol (or symbols?) to use to indicate a full
object peel. I mentioned `**fieldname` in the cover letter, but that looks
more like a double dereference than a recursive one.
I think `^{}fieldname` would be a good candidate, but it's *extremely*
important (for the sake of avoiding user confusion/frustration) that it
produces the same object & associated info as the standard revision parsing
machinery [1]. One notable difference (it might be the only one) from
`*fieldname` would be, if a ref points to a non-tag object, then that
object's information would printed (rather than an empty string). But maybe
that difference is what we'd want anyway, since it's a better one-for-one
replacement of 'git for-each-ref | git cat-file --batch-check'.
I'll try implementing that for V2. If it doesn't work for some reason,
though, I'll explain why in the commit message.
[1] https://git-scm.com/docs/git-rev-parse#Documentation/git-rev-parse.txt-emltrevgtemegemv0998em
>
> Patrick
>
^ permalink raw reply
* Re: [PATCH] diff: implement config.diff.renames=copies-harder
From: Junio C Hamano @ 2023-11-08 1:26 UTC (permalink / raw)
To: Elijah Newren; +Cc: Sam James via GitGitGadget, git, Sam James
In-Reply-To: <CABPp-BF9iUkF+g_w7wLATFTmjfJ3f1hsBr+zXxNZEcq-XiNOWg@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
>> 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.
OK, "previous version" is within the context of "variants of gcc",
and to us, there is no distinction among them (we do not care which
ones are older than the others---we need to keep track of them all).
Which makes sense. OK.
> 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.
"in addition to paths that are different, will look for more copies
even in unmodified paths" then?
^ permalink raw reply
* Re: [PATCH 0/9] for-each-ref optimizations & usability improvements
From: Victoria Dye @ 2023-11-08 1:31 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Junio C Hamano, Victoria Dye via GitGitGadget, git
In-Reply-To: <ZUoWPpFHEi-PZjoD@tanuki>
Patrick Steinhardt wrote:
> 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.
Since both annotated tags and symbolic refs are essentially pointers, it's
not surprising that they both use the term "dereference." Even though
"deref" refers to symbolic refs in 'update-ref', its existence as an
abbreviation for "dereference" is relevant when coming up with a way to
abbreviate "dereference" when referring to tags.
>
> 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.
Hmm. I think I mostly agree on your definition of "peel". In the docs, it's
used to refer to:
- recursively resolving an OID to an object of a specified type [1]
- recursively resolving a tag OID to a non-tag object [2]
Notably, there seems to be a strong association of "peeling" to "recursive
resolution". Which means it doesn't necessarily describe what "*" currently
does.
"Dereference" generally seems like a looser term than what you've suggested.
It does refer to symbolic ref resolution as you describe [3], but "recursive
dereference" is definitely also a synonym for "peel" [4]. That, combined
with the fact that "*" is the "dereference operator", leads me to believe
that "%(*fieldname)" would accurately be described as a "tag dereference"
field in the context of 'for-each-ref'.
As I mentioned in [5], I'm going to try adding this functionality with a
field specifier rather than a command line option, so the name of the option
might be moot. But, since dereferencing/peeling will still be relevant to
the changes, I'll make sure the terminology I use in the documentation is as
precise as possible (i.e., use "peel" where I previously used "fully
dereference").
Separately, this has inspired me to revisit something I've been putting off,
which is to add a definition for "peel" (and now probably "dereference" as
well) in 'gitglossary'. I'll try to send that out in the next couple days.
Thanks!
[1] https://git-scm.com/docs/git-rev-parse#Documentation/git-rev-parse.txt---verify
[2] https://git-scm.com/docs/gitprotocol-v2#_ls_refs
[3] https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefsymrefasymref
[4] https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefcommit-ishacommit-ishalsocommittish
[5] https://lore.kernel.org/git/cf691b7c-288f-4cc9-a2ac-1a43972ae446@github.com/
>
> Patrick
^ permalink raw reply
* Re: Regression: git send-email Message-Id: numbering doesn't start at 1 any more
From: Junio C Hamano @ 2023-11-08 1:34 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Michael Strawbridge, Douglas Anderson, git, entwicklung
In-Reply-To: <20231107070632.spe3cappk5b5jg3q@pengutronix.de>
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 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.
Yikes. I was hoping that the whole "dumps the messages" output is
read by the script, so that it does not have to assume anything
about the Message-ID format (like "it has some unchanging parts,
the changing parts begin with 1, and it increments by 1 for each
message").
You could give Message-ID externally to the output of format-patch
before feeding send-email, which would just use them, and that way
you would have more control over the entire process, I guess.
^ permalink raw reply
* Re: issue unable to commit file and folder name to upper lower case
From: Junio C Hamano @ 2023-11-08 1:48 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: chengpu lee, git
In-Reply-To: <20231107173557.GA29083@tb-raspi4>
Torsten Bögershausen <tboegi@web.de> writes:
> 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>
Correct and very helpful suggestion. Or get a better filesystem ;-)
Thanks.
^ 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