* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Jeff King @ 2023-11-10 21:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
In-Reply-To: <xmqqedgyw6jv.fsf@gitster.g>
On Fri, Nov 10, 2023 at 01:55:48PM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > +static int pair_chunk_expect_fn(const unsigned char *chunk_start,
> > + size_t chunk_size,
> > + void *data)
> > +{
> > + struct pair_chunk_data *pcd = data;
> > + if (chunk_size / pcd->record_size != pcd->record_nr)
> > + return -1;
> > + *pcd->p = chunk_start;
> > + return 0;
> > +}
>
> I know one of the original places did the "divide the whole by
> per-record size and see if it matches the number of records", the
> same as we see above, but the check in the above could also be
>
> if (chunk_size != st_mult(pcd->record_size, pcd->record_nr))
> return -1;
>
> which would also catch the case where chunk_size is not a multiple
> of the record size. Your conversion of OOFF in midx.c loses this
> protection as the original uses the multiplication-and-compare, but
> the rewrite to call pair_chunk_expect would call the above and
> checks with the truncating-divide-and-compare.
>
> Does the distinction matter? I dunno. If the record/chunk
> alignment is asserted elsewhere, then the distinction should not
> matter, but even if it were, seeing a truncating division used in
> any validation makes my skin tingle.
Yes, the distinction does matter. If pair_chunk_expect_fn() used
st_mult(), then it would die on overflow, rather than returning an
error. For commit-graph files this is propagated up, and we continue the
operation by falling back to the non-graph code. There's a test in t5318
that will fail in this case. See patches 1 and 6 in my series for more
discussion.
One of those patches calls out the truncating division issue, but to
summarize: IMHO this is OK, as what we really want to know is "is it big
enough that we can always ask for NR records of size ELEM", which
division gives us. If we do want to be more precise, but also avoid
die(), we'd need a variant of st_mult() that returns a boolean. I didn't
think it was worth it for this case (but arguably it is something that
would be useful to have in general).
-Peff
^ permalink raw reply
* Re: [PATCH 6/9] commit-graph: use fanout value for graph size
From: Jeff King @ 2023-11-10 21:52 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZU1Z6WUrEhcxLBTO@nand.local>
On Thu, Nov 09, 2023 at 05:15:05PM -0500, Taylor Blau wrote:
> > This is one of the ways that pair_chunk_expect() could do better without
> > adding a lot of code, because it can check the return value from
> > read_chunk(). It doesn't help the other cases (like OIDF) that still
> > have to call read_chunk() themselves, though. Possibly read_chunk()
> > should just take a flag to indicate that it should complain when the
> > chunk is missing. And then callers could just do:
> >
> > if (read_chunk(cf, id, our_callback, CHUNK_REQUIRED)
> > return -1; /* no need to complain; either our_callback() did, or
> > read_chunk() itself */
>
> We do return CHUNK_NOT_FOUND when we have a missing chunk, which we
> could check for individually. But TBH, I don't find the first error all
> that useful. In this instance, there's really only one way for the OIDL
> chunk to be corrupt, which is that it has the wrong size.
But aren't there two ways it can go wrong? It can be the wrong size, or
it can be missing. In the first we say:
error: wrong size
error: missing or corrupted
and in the second we say:
error: missing or corrupted
Which is why I think issuing a message from the callback has value. Of
course the ideal would be:
error: wrong size
and:
error: missing
but I didn't think it was worth making the code much longer for an error
condition we don't really expect anybody to see in practice.
And also...
> And short of making the error much more robust, e.g.:
>
> error: commit-graph OID lookup chunk is the wrong size (got: $X, wanted: $Y)
...yes, I think that would be worth doing, especially if you are
centralizing the error messages in pair_chunk_expect(). But your series
goes the opposite direction.
> and dropping the generic "missing or corrupt" error, I think that just
> the generic error is fine here.
If you drop that error, then who will warn about the missing-chunk case?
The user would see nothing at all.
-Peff
^ permalink raw reply
* Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter
From: Jeff King @ 2023-11-10 21:48 UTC (permalink / raw)
To: Simon Ser; +Cc: René Scharfe, git, Junio C Hamano
In-Reply-To: <VTz8XT3MCqWUh1HFQon62NxmGJiYFfNmBeWTtR8MwmeuaSkovfBJ02P-S79GsD94XwlxlrL6W80YZ8OwfpDd1BqA0E4GwFQlDKN5DWq0Qtg=@emersion.fr>
On Fri, Nov 10, 2023 at 10:36:22AM +0000, Simon Ser wrote:
> > I don't think that answering those questions needs to hold up your
> > patch. We can take it as a quick fix for a real bug, and then anybody
> > interested can dig further as a separate topic on top.
>
> These are good questions indeed. Unfortunately I don't hink I'll have time to
> work on this though.
That's OK. I think it's fine to stop here for now.
> > Some of these long lines (and the in-string newlines!) make this ugly
> > and hard to read. But it is also just copying the already-ugly style of
> > nearby tests. So I'm OK with that. But a prettier version might be:
> >
> > test_expect_success 'cover letter respects --encode-email-headers' '
> > test_config branch.rebuild-1.description "Café?" &&
> > git checkout rebuild-1 &&
> > git format-patch --stdout --encode-email-headers \
> > --cover-letter --cover-from-description=subject \
> > main >actual &&
> > ...
> > '
>
> Yeah, that sounds better indeed. Let me know if you want me to resend a cleaner
> version of the test.
I don't have a strong opinion, so I'd leave it up to you.
> > I also wondered if we could be just be testing this much more easily
> > with another header like "--to". But I guess that would be found in both
> > the cover letter and the actual patches (we also don't seem to encode
> > it even in the regular patches; is that a bug?).
>
> That sounds like another bug indeed… But maybe that'll be harder to fix. To
> Q-encode this field one needs to split off the full name and actual mail
> address ("André <andre@example.org>" would be split into "André" and
> "andre@example.org"), then Q-encode the full name, then join the strings
> together again. In particular, it's incorrect to Q-encode the full string.
Yeah, without even looking at the code, I had a suspicion that this
would be an issue. I doubt that format-patch is doing much parsing at
all of what we feed it via --to.
-Peff
^ permalink raw reply
* Re: [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>`
From: Jeff King @ 2023-11-10 21:46 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1699609940.git.ps@pks.im>
On Fri, Nov 10, 2023 at 11:01:11AM +0100, Patrick Steinhardt wrote:
> this is the second version of my patch series that replaces all uses of
> `test <expr> -o/a <expr`.
>
> Changes compared to v1:
>
> - I've expanded a bit on why we want to do these conversions in the
> first place in the first commit message.
>
> - Dropped a needless subshell and added missing quoting while at it.
>
> - Explained why we need to decompose the asserts in the second patch
> into two asserts.
These look OK to me. I mentioned a small nit on the first patch, but I
am OK with ignoring it (and we are reaching diminishing returns
polishing an otherwise trivial series).
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Jeff King @ 2023-11-10 21:44 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <2967c8ebb460934eb4aaaaebe5941bff643d4a94.1699609940.git.ps@pks.im>
On Fri, Nov 10, 2023 at 11:01:15AM +0100, Patrick Steinhardt wrote:
> diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
> index 669ebaf68be..9fbf90cee7c 100755
> --- a/t/valgrind/valgrind.sh
> +++ b/t/valgrind/valgrind.sh
> @@ -23,7 +23,7 @@ memcheck)
> VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
> VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
> test 3 -gt "$VALGRIND_MAJOR" ||
> - test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
> + ( test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR" ) ||
> TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
I was surprised to see this one as a subshell after you adjusted the
other. It probably isn't that big a deal either way, though (as a style
thing I generally try to use braces unless I am relying on the separate
environment provided by the subshell, but it's certainly not wrong in
this case).
-Peff
^ permalink raw reply
* Re: first-class conflicts?
From: Elijah Newren @ 2023-11-10 21:41 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: phillip.wood, Sandra Snan, git, Randall S. Becker
In-Reply-To: <CAESOdVBjEYAp+P_mYdByYrPmbiu9DWL=Z_r19H8D9bxkJrquFA@mail.gmail.com>
Hi Martin,
On Wed, Nov 8, 2023 at 10:23 AM Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Tue, Nov 7, 2023 at 11:31 PM Elijah Newren <newren@gmail.com> wrote:
> > On Tue, Nov 7, 2023 at 9:38 AM Martin von Zweigbergk
> > <martinvonz@google.com> wrote:
> > >
[...]
> > I am curious more about the data you do store. My fuzzy memory is
> > that you store a commit header involving something of the form "A + B
> > - C", where those are all commit IDs. Is that correct?
>
> We actually store it outside the Git repo (together with the "change
> id"). We have avoided using commit headers because I wasn't sure how
> well different tools deal with unexpected commit headers, and because
> I wanted commits to be indistinguishable from commits created by a
> regular Git binary. The latter argument doesn't apply to commits with
> conflicts since those are clearly not from a regular Git binary
> anyway, and we don't allow pushing them to a remote.
>
> > Is this in
> > addition to a normal "tree" header as in Git, or are one of A or B
> > found in the tree header?
>
> It's in addition. For the tree, we actually write a tree object with
> three subtrees:
>
> .jjconflict-base-0: C
> .jjconflict-side-0: A
> .jjconflict-side-1: B
>
> The tree is not authoritative - we use the Git-external storage for
> that. The reason we write the trees is mostly to prevent them from
> getting GC'd.
Oh, that seems like a clever way to handle reachability and make sure
the relevant trees are automatically included in any pushes or pulls.
> Also, if a user does `git checkout <conflicted commit>`,
> they'll see those subdirectories and will hopefully be reminded that
> they did something odd (perhaps we should drop the leading `.` so `ls`
> will show them...). They can also diff the directories in a diff tool
> if they like.
Oh, so they don't get a regular top-level looking tree with
possibly-conflicted-files present? Or is this in addition to the
regular repository contents? If in addition, are you worried about
users ever creating real entries named ".jjconflict-base-<N>" in their
repository?
> > I think you said there was also the
> > possibility for more than three terms. Are those for when a
> > conflicted commit is merged with another branch that adds more
> > conflicts, or are there other cases too? (Octopus merges?)
>
> Yes, they can happen in both of those cases you mention. More
> generally, whenever you apply a diff between two trees onto another
> tree, you might end up with a higher-arity conflict. So merging in
> another branch can do that, or doing an octopus merge (which is the
> same thing at the tree level, just different at the commit level), or
> rebasing or reverting a commit.
>
> We simplify conflicts algebraically, so rebasing a commit multiple
> times does not increase the arity - the intermediate parents were both
> added and removed and thus cancel out. These simple algorithms for
> simplifying conflicts are encapsulated in
> https://github.com/martinvonz/jj/blob/main/lib/src/merge.rs. Most of
> them are independent of the type of values being merged; they can be
> used for doing algebra on tree ids, content hunks, refs, etc. (in the
> test cases, we mostly merge integers because integer literals are
> compact).
It's done on content hunks as well? That's interesting.
When exactly would it be done on refs, though? I'm not following that one.
And what else is in that "etc."?
> > What about recursive merges, i.e. merges where the two sides do not
> > have a unique merge base. What is the form of those? (Would "- C" be
> > replaced by "- C1 - C2 - ... - Cn"? Or would we create the virtual
> > merge base V and then do a " - V"? Or do we only have "A + B"?)
>
> We do that by recursively creating a virtual tree just like Git does,
> I think (https://github.com/martinvonz/jj/blob/084b99e1e2c42c40f2d52038cdc97687b76fed89/lib/src/rewrite.rs#L56-L71).
> I think the main difference is that by modeling conflicts, we can
> avoid recursive conflict markers (if that's what Git does), and we can
> even automatically resolve some cases where the virtual tree has a
> conflict.
Okay, but that talks about the mechanics of creating a recursive
merge, omitting all the details about how the conflict header is
written when you record the merge. Is the virtual merge base
represented in the algebraic "A + B - C" expressions, or is the "- C"
part omitted? If it is represented, and the virtual merge base had
conflicts which you could not automatically resolve, what exactly does
the conflicted header for the outer merge get populated with?
[...]
> Great questions! We don't have support for renames, so we haven't had
> to worry about these things. We have talked a little about divergent
> renames and the need for recording that in the commit so we can tell
> the user about it and maybe ask them which name they want to keep. I
> had not considered the interaction with partial conflict resolution,
> so thanks for bringing that up. I don't have any answers now, but
> we'll probably need to start thinking about this soon.
I was wondering if that might be the answer. When you do tackle this,
I'd be interested to hear your thoughts. I'm wondering if we just
need to augment the data in the conflict header to handle such cases
(though I guess this could risk having commit objects that are
significantly bigger than normal in theoretical cases where many such
paths are involved?)
> > I'm curious to hear what happens when you do start dogfooding, on
> > projects with many developers and which are jj-only. Do commits with
> > conflicts accidentally end up in mainline branches, or are there good
> > ways to make sure they don't hit anything considered stable?
>
> That won't happen at Google because our source of truth for "merged
> PRs" (in GitHub-speak) is in our existing VCS. We will necessarily
> have to translate from jj's data model to its data model before a
> commit can even be sent for review.
That makes sense, but I was just hoping we'd have an example to look
to for how to keep things safe if we were to implement this. Sadly, I
don't think we have the benefit of relying on folks to first push
their commits into some other VCS which lacks this feature. ;-)
^ permalink raw reply
* Re: [PATCH v4 0/3] t: improve compatibility with NixOS
From: Jeff King @ 2023-11-10 21:41 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1699596457.git.ps@pks.im>
On Fri, Nov 10, 2023 at 09:16:56AM +0100, Patrick Steinhardt wrote:
> this is the fourth version of my patch series to improve compatibility
> of our test suite with NixOS.
>
> Three changes compared to v3:
>
> - Switched from `test <expr> -a <expr>` to `test <expr> && test
> <expr>`.
>
> - Improved the commit message to explain why the new
> runtime-detected paths are only used as a fallback.
>
> - Rebased on top of 0e3b67e2aa (ci: add support for GitLab CI,
> 2023-11-09), which has been merged to `next` and caused conflicts.
Thanks, these all look good to me.
-Peff
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Jeff King @ 2023-11-10 21:41 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: git
In-Reply-To: <ZU4widBlHljjg9lL@debian>
On Fri, Nov 10, 2023 at 02:30:49PM +0100, Alejandro Colomar wrote:
> > Having it directly in sendmailcmd causes some glitch: It repeats all CCs
> > in TO. See a log:
Ah, right. That makes sense. send-email has to pass all of the envelope
recipients on the command-line, which mutt then interprets as
destinations to add to "to". Mutt is smart enough to de-duplicate the
"to", but it (correctly) allows duplicate to/cc. (My basic test didn't
have any cc's).
> > So maybe we need the wrapper script to ignore the arguments.
>
> Heh! The following trick works as well, without needing a script:
>
> [sendemail]
> sendmailcmd = mutt -H - && true
>
> It probably relies too much on git-send-email(1)'s current
> implementation, but it works. :)
I was going to suggest the same thing. And no, I don't think it is
relying on any send-email implementation details. Git always tries to
feed command-invoking config like this to the shell for consistency (and
to allow tricks like this).
-Peff
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Alejandro Colomar @ 2023-11-10 21:06 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: Jeff King, git
In-Reply-To: <5yn2w52iymgnobesoi2jdwpyzaf5foc4sytxfdzl4vavlox62j@7att57ya6scz>
[-- Attachment #1: Type: text/plain, Size: 4055 bytes --]
Hi Konstantin,
On Thu, Nov 09, 2023 at 12:59:23PM -0500, Konstantin Ryabitsev wrote:
> On Thu, Nov 09, 2023 at 06:42:19PM +0100, Alejandro Colomar wrote:
> > I haven't yet tried b4(1), and considered trying it some time ago, but
> > really didn't find git-send-email(1) and mutt(1) so difficult to use
> > that b4(1) would simplify much.
>
> Well, sending is only a small part of what b4 will do for you -- the core
> benefits are really cover letter management, automatic versioning and
> simplified list trailer collection. It's all tailored to kernel needs, but it
> will work for any project that depends on mailed patches.
>
> > But I have tried patatt(1) before, which is what I think b4(1) uses for
> > signing. Here are some of my concerns about patatt(4):
> >
> > It doesn't sign the mail, but just the patch.
>
> Well, no, it signs the entire thing, not just the patch, but it's true that
> it's specifically targeted at patches (hence the name).
>
> > There's not much
> > difference, if any, in authenticability terms, but there's a big
> > difference in usability terms:
> >
> > To authenticate a given patch submitted to a mailing list, the receiver
> > needs to also have patatt(1) configured. Otherwise, it looks like a
> > random message.
>
> Yes, but that's a feature.
>
> > MUAs normally don't show random headers (patatt(1)
> > signs by adding the signature header), so unless one is searching for
> > that header, it will be ignored. This means, if I contribute to other
> > projects, I need to tell them my patch is signed via patatt(1) in
> > order for them to verify. If instead, I sign the email as usual with my
> > MUA, every MUA will recognize the signature by default and show it to
> > the reader.
>
> I go into this in the FAQ for patatt:
> https://github.com/mricon/patatt#why-not-simply-pgp-sign-all-patches
>
> Basically, developers really hated getting patches signed with PGP, either
> inline or MIME, which is why it never took off. Putting it into the header
> where it's not seen except by specialized tooling was a design choice.
It would be interesting if MUAs would support PGP signatures in a
header. Did you consider that option back then? Maybe patching a
mutt(1) or neomutt(1) to do that would have been simpler than developing
patatt(1).
>
> > It also doesn't allow encrypting mail, so let's say I send some patch
> > fixing a security vulnerability, I'll need a custom tool to send it. If
> > instead, I use mutt(1) to send it signed+encrypted to a mailing list
> > that provides a PGP public key, I can reuse my usual tools.
>
> Right, the goal was really *just* attestation. For encrypted patch exchange we
> have remail (https://korg.docs.kernel.org/remail.html), which worked
> significantly better than any other alternative we've considered.
>
> > Also, and I don't know if b4(1) handles this automagically, but AFAIR,
> > patatt(1) didn't: fo signing a patch, I had to configure per-directory
> > with `patatt install-hook`. I have more than a hundred git worktrees
> > (think of dozens of git repositories, and half a dozen worktrees --see
> > git-worktree(1)-- per repository). If I need to configure every one of
> > those worktrees to sign all of my patches, that's going to be
> > cumbersome. Also, I scrape and re-create worktrees for new branches
> > all the time, so I'd need to be installing hooks for patatt(1) all the
> > time. Compare that to having mutt(1) configured once. It doesn't
> > scale that well.
>
> Also true -- patatt was really envisioned as a library for b4, where you can
> configure patch signing in your ~/.gitconfig for all projects.
Overall, I think mutt(1) works better for me than patatt(1) via b4(1)
for crypto operations. I don't use software that doesn't work with
PGP/MIME, and it's more versatile. I'm still curious about the other
features of b4(1), so I'll try it for those.
Thanks!
Alex
>
> -K
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: Error when "git mv" file in a sparsed checkout
From: Elijah Newren @ 2023-11-10 20:11 UTC (permalink / raw)
To: Josef Wolf, git
In-Reply-To: <20231108113636.GT7041@raven.inka.de>
Hi,
On Wed, Nov 8, 2023 at 3:38 AM Josef Wolf <jw@raven.inka.de> wrote:
>
> Thanks for the reply, Elijah!
>
[...]
> > > 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.
> >
> > You are correct; the wording of the error message here is suboptimal
> > and seems to have been focused more on the git-add case (the error
> > message is shared by git-add, git-mv, and git-rm). Thanks for
> > pointing it out! We could improve that wording, perhaps with
> > something like:
> >
> > The following paths and/or pathspecs match paths that are
> > outside of your sparse-checkout definition, so will not be
> > updated:
> >
> > Which is still slightly slanted towards git-add and git-rm cases, but
> > I hope it works better than the current message. Thoughts?
>
> Yes, the wording was pretty much confusing me, since i could not find a file
> named "new-file" anywhere in the repo.
>
>
> There are more things confusing concerning sparse mode:
Sweet, thanks for taking the time to write these up. It certainly
helps confirm some of the directions we picked and changes we made to
make things a little clearer, and helps us continue working in that
direction. Some comments below on individual points...
> - It is not clear from git-sparse-checkout(1) when changes to
> $GIT_DIR/info/sparse-checkout are catched up. In my case: would it be enough
> to add the new pathname just before git-mv or would a fresh git-checkout be
> needed after modifying $GIT_DIR/info/sparse-checkout? You have clarified
> this in your response, but shouldn't this be clear from the manpage?
I believe at least part of this confusion is due to using the old
style of handling sparse checkouts; namely, by actually editing
$GIT_DIR/info/sparse-checkout. We have taken pains to guide people
away from that workflow, because it is both more work, and leads to
more confusion. If you instead do a
git sparse-checkout set --no-cone <pattern1> <pattern2> ... <patternN>
or a
git sparse-checkout add <another-pattern>
then the sparse-checkout command handles populating the
$GIT_DIR/info/sparse-checkout for you as well as any needed checkout,
meaning that there isn't a "catch up" step as there traditionally was.
And it makes it a bit clearer that if you add some path to your
sparse-checkout, then your sparse-checkout is ready to handle the
additional path right away.
> - git-sparse-checkout(1) refers to "skip-worktree bit". This concept is
> potentially not very familiar to the average git user which uses mostly
> porcelain. Thus, edge cases remain to be unclear.
I totally agree that the "skip-worktree bit" is something we should
avoid exposing to the user. I called it out previously:
"""
Most sparse checkout users are unaware of this implementation
detail, and the term should generally be avoided in user-facing
descriptions and command flags. Unfortunately, prior to the
`sparse-checkout` subcommand this low-level detail was exposed,
and as of time of writing, is still exposed in various places.
"""
However, we should also note that you reported using v2.34.1, which is
really quite old. The current git-sparse-checkout(1) has been almost
completely overhauled in the meantime:
$ git show v2.34.1:Documentation/git-sparse-checkout.txt | wc -l
263
$ git diff --stat v2.34.1 v2.43.0-rc1 -- Documentation/git-sparse-checkout.txt
Documentation/git-sparse-checkout.txt | 446 +++++++++++++++++++++++++---------
1 file changed, 333 insertions(+), 113 deletions(-)
And, in particular, "skip-worktree" doesn't appear until quite a bit
later in the file, and then only appears in a section labelled
"INTERNALS -- SPARSE CHECKOUT".
> - The pathspecs refers to .gitignore (which by itself is not very clear). But
> there are differences:
> 1. giignore is relative to containing directoy, which don't seem to make
> much sense for sparse mode
> 2. sparse specs are the opposite of gitignore, which seems to have different
> meaning in some edge-cases.
Yeah, copying .gitignore syntax and merely referring to the gitignore
manual for specification of the patterns was a huge design mistake.
I've hated it since the beginning. The internals actually managed to
make it *even more* confusing for quite some time as it referred to
everything as "excludes", regardless of whether used for gitignore or
sparse-checkout. But yeah, these and other inherent problems with
non-cone mode are called out in the "INTERNALS -- NON-CONE PROBLEMS"
section of the manual, and is a big piece of why we recommend users
migrate away from it if possible.
> - For cone, it is not clear how the two "accepted patterns" look like what the
> semantics are.
Yeah, it is a bit complex, and I advocated for just using a different
control file for cone mode to help us step away from the blight of
$GIT_DIR/info/sparse-checkout and its inherent tie to gitignore. I
didn't win that one.
However, why does it actually matter? You shouldn't be bothering with
the patterns or editing the $GIT_DIR/info/sparse-checkout file for
cone mode. You just
git sparse-checkout set <directory1> <directory2> ... <directoryN>
and then the file will be set up for you.
> I understand that specifying a directory adds siblings
> recursively. But what does the "Parent" mode mean exactly and when/how is
> this recognized? I guess, this is just a mis-namer? IMHO, parent of /a/b/c would
> be /a/b and not /a/b/c/* (as git-sparse-chekout(1) suggests).
No, /a/b/c/* is not the parent, that's the portion that says that all
things below a/b/c (i.e. all descendant files) should be included.
The parent parts would be where it adds /a/b/ and /a/ as patterns to
ensure things directly under a/ and directly under a/b/ are included.
I think the newer version of the manual might explain this a little
better, but honestly, attempting to explain it is a losing battle.
Users shouldn't read or edit the sparse-checkout file in cone mode.
We let them, but we strongly recommend against it. Just pass the
actual directory names to `git sparse-checkout {set,add}` and let it
take care of the patterns for you.
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Taylor Blau @ 2023-11-10 16:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <xmqqedgyw6jv.fsf@gitster.g>
On Fri, Nov 10, 2023 at 01:55:48PM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > +static int pair_chunk_expect_fn(const unsigned char *chunk_start,
> > + size_t chunk_size,
> > + void *data)
> > +{
> > + struct pair_chunk_data *pcd = data;
> > + if (chunk_size / pcd->record_size != pcd->record_nr)
> > + return -1;
> > + *pcd->p = chunk_start;
> > + return 0;
> > +}
>
> I know one of the original places did the "divide the whole by
> per-record size and see if it matches the number of records", the
> same as we see above, but the check in the above could also be
>
> if (chunk_size != st_mult(pcd->record_size, pcd->record_nr))
> return -1;
>
> which would also catch the case where chunk_size is not a multiple
> of the record size. Your conversion of OOFF in midx.c loses this
> protection as the original uses the multiplication-and-compare, but
> the rewrite to call pair_chunk_expect would call the above and
> checks with the truncating-divide-and-compare.
Hmm. I was thinking of Peff's "commit-graph: handle overflow in
chunk_size checks", but I think that I was overly eager in applying the
same reasoning to the MIDX code.
The important piece of the rationale in that patch is as follows:
In the current code this is only possible for the CDAT chunk, but
the reasons are quite subtle. We compute g->num_commits by dividing
the size of the OIDL chunk by the hash length (since it consists of
a bunch of hashes). So we know that any size_t multiplication that
uses a value smaller than the hash length cannot overflow. And the
CDAT records are the only ones that are larger (the others are just
4-byte records). So it's worth fixing all of these, to make it clear
that they're not subject to overflow (without having to reason about
seemingly unrelated code).
In particular, that g->num_commits is computed by dividing the length of
the OIDL chunk by the hash length, thus any size_t multiplication of
g->num_commits with a value smaller than that hash length cannot
overflow.
But I don't think we enjoy the same benefits in the MIDX scenario. In
this case, the num_objects field is just:
m->num_objects = ntohl(m->chunk_oid_fanout[255])
so I don't think we can make the same guarantees about what is and isn't
save to compute under size_t arithmetic.
I'd be curious what Peff has to say, but if he agrees with me, I'd
recommend taking the first five patches, and dropping the two
MIDX-related ones.
Thanks,
Taylor
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Alejandro Colomar @ 2023-11-10 13:30 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <ZU1-l4PwMU5H4_VN@debian>
[-- Attachment #1: Type: text/plain, Size: 2964 bytes --]
On Fri, Nov 10, 2023 at 01:51:34AM +0100, Alejandro Colomar wrote:
> Hi Jeff,
>
> On Thu, Nov 09, 2023 at 01:03:08PM -0500, Jeff King wrote:
> > On Thu, Nov 09, 2023 at 04:26:23PM +0100, Alejandro Colomar wrote:
> >
> > > I've tried something even simpler:
> > >
> > > ---8<---
> > > #!/bin/sh
> > >
> > > mutt -H -;
> > > --->8---
> > >
> > > I used it for sending a couple of patches to linux-man@, and it seems to
> > > work. I don't have much experience with mutt, so maybe I'm missing some
> > > corner cases. Do you expect it to not work for some case? Otherwise,
> > > we might have a winner. :)
> >
> > Wow, I don't know how I missed that when I read the manual. That was
> > exactly the feature I was thinking that mutt would need. ;)
> >
> > So yeah, that is obviously better than the "postponed" hackery I showed.
> > I notice that "-H" even causes mutt to ignore "-i" (a sendmail flag that
> > Git adds to sendemail.sendmailcmd). So you can just invoke it directly
> > from your config like:
> >
> > git config sendemail.sendmailcmd "mutt -H -"
>
> Having it directly in sendmailcmd causes some glitch: It repeats all CCs
> in TO. See a log:
>
> Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
> OK. Log says:
> Sendmail: mutt -H - -i kevin@8t8.us mutt-dev@mutt.org alx@kernel.org e.sovetkin@gmail.com neomutt-devel@neomutt.org
> From: Alejandro Colomar <alx@kernel.org>
> To: Kevin McCarthy <kevin@8t8.us>,
> mutt-dev@mutt.org
> Cc: Alejandro Colomar <alx@kernel.org>,
> Jenya Sovetkin <e.sovetkin@gmail.com>,
> neomutt-devel@neomutt.org
> Subject: [PATCH] send.c: Allow crypto operations in batch and mailx modes.
> Date: Fri, 10 Nov 2023 01:41:24 +0100
> Message-ID: <20231110004128.5972-2-alx@kernel.org>
> X-Mailer: git-send-email 2.42.0
> MIME-Version: 1.0
> Content-Transfer-Encoding: 8bit
>
> Result: OK
>
> The sent mail ended up being
>
> From: Alejandro Colomar <alx@kernel.org>
> To: Kevin McCarthy <kevin@8t8.us>, mutt-dev@mutt.org, alx@kernel.org,
> e.sovetkin@gmail.com, neomutt-devel@neomutt.org
> Cc: Alejandro Colomar <alx@kernel.org>,
> Jenya Sovetkin <e.sovetkin@gmail.com>, neomutt-devel@neomutt.org
>
> So maybe we need the wrapper script to ignore the arguments.
Heh! The following trick works as well, without needing a script:
[sendemail]
sendmailcmd = mutt -H - && true
It probably relies too much on git-send-email(1)'s current
implementation, but it works. :)
Cheers,
Alex
>
> Cheers,
> Alex
>
> >
> > Annoyingly, "-E" doesn't work when reading over stdin (I guess mutt
> > isn't willing to re-open the tty itself). But if you're happy with not
> > editing as they go through, then "-H" is then that's enough (in my
> > workflow, I do the final proofread via mutt).
> >
> > -Peff
>
> --
> <https://www.alejandro-colomar.es/>
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: Git Rename Detection Bug
From: Jeremy Pridmore @ 2023-11-10 11:28 UTC (permalink / raw)
To: Elijah Newren; +Cc: git@vger.kernel.org, Paul Baumgartner
In-Reply-To: <CABPp-BHYaxa7QoXabM=7hW-93hQLK-=KayGtDHtWxxdAnJCcJw@mail.gmail.com>
Hi Elijah,
Many thanks for your reply, the detail is much appreciated. I was aware, from recently read articles, that git doesn't record renames as such, hence my investigations into the rename detection, but I also found some interesting points in your email, such as the "git status --no-renames" flag.
I think the issue I'm encountering is described by what you say here:
"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)"
That is close to the behaviour I'm seeing. As I mentioned, git seems to think a file has been deleted and then as it continues to detect renames, it's as if it is going through lists of "Local-Base" and "Base-Remote" changes trying to match them up, but the directories of the files being matched are "offset", as highlighted by this list of mismatches:
(I'd put the paths in a table for easier analysis, but for some reason the emails need to be plain text)
> 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
Given git compares both the content and the directory\filenames, and as the repositories have unrelated histories, the "Base" file is going to be empty, therefore, even if Local and Remote are identical, they are both 100% different to Base. That given, I'm not sure why git would state that Landscape/Documentation/Rdt.Documentation.UI/Properties/licenses.licx and Landscape/src/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1 are a "both added" conflict given their file names and paths are completely different. Any ideas?
I wrote a script to resolve the conflicts best I can which categorises the files into sets according to the file status (i.e. "added by them", "added by us" etc), and then either does a "git checkout head -- <file>" or a "git rm <file>" based upon which set the file is in and whether it is in another set or not. This has worked really well and helped me through the large changeset with 3k conflicts.
As git only needs to try and match files in the "deleted by us" and "deleted by them" sets (although including the "deleted in both" set would allow matching renames/moves on both sides), an idea for a potential improvement to the matching algorithm (where you say there's a comment "too many alternatives, pick one") could be to compute a "difference value" for the path\filename of those files in one of the other sets (i.e. "added by us", "added by them" or "added in both"), and chose a potential rename based upon the smallest calculated difference. The difference value would be the number of differences in folder names, e.g.
deleted in both: Landscape/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1
added in both: Landscape/src/Deployment/PowershellScripts/pre-req/Landscape.Net/pre-req.ps1
(path\name difference = 2)
added in both: Landscape/src/Deployment/PowershellScripts/pre-req/Rdt.BatchProcessingService.Setup/pre-req.ps1
(path\name difference = 1)
added in both: Landscape/src/Deployment/PowershellScripts/pre-req/Workflow/pre-req.ps1
(path\name difference = 2)
So, given the above, git would chose the second "added in both" entry.
Food for thought? Happy to discuss the idea further.
Regards,
Jeremy Pridmore
Lead Solution Architect
From: Elijah Newren <newren@gmail.com>
Sent: 07 November 2023 08:05
To: Jeremy Pridmore <jpridmore@rdt.co.uk>
Cc: git@vger.kernel.org <git@vger.kernel.org>; Paul Baumgartner <pbaumgartner@rdt.co.uk>
Subject: Re: Git Rename Detection Bug
[You don't often get email from newren@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
Caution: This email originated from outside of the organisation. Please treat any attachments or links with caution. If in doubt please contact IT
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
________________________________
DISCLAIMER This email is confidential. It should only be read by those persons to whom it is addressed. RDT Ltd accept no liability for the consequences of any person acting, or refraining from acting, on any information contained within this e-mail or any attached documents prior to the receipt by those persons of subsequent written confirmation of that information. If you think this e-mail may not be intended for you, do not use, pass on or copy the transmission in any way. While all reasonable precautions are taken to minimise the risk of transmitting software viruses we advise you to carry out your own virus checks on any attachment to this message. We cannot accept liability for any loss or damage caused by software viruses.
^ permalink raw reply
* Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter
From: Simon Ser @ 2023-11-10 10:36 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, git, Junio C Hamano
In-Reply-To: <20231109183506.GB2711684@coredump.intra.peff.net>
On Thursday, November 9th, 2023 at 19:35, Jeff King <peff@peff.net> wrote:
> That makes sense, and your patch looks the right thing to do as an
> immediate fix. But I have to wonder:
>
> 1. Are there other bits that need to be copied? Grepping for other
> code that does the same thing, I see that show_log() and
> cmd_format_patch() copy a lot more. (For that matter, why doesn't
> make_cover_letter() just use the context set up by
> cmd_format_patch()?).
>
> 2. Why are we copying this stuff at all? When we introduced the
> pretty-print context back in 6bf139440c (clean up calling
> conventions for pretty.c functions, 2011-05-26), the idea was just
> to keep all of the format options together. But later, 6d167fd7cc
> (pretty: use fmt_output_email_subject(), 2017-03-01) added a
> pointer to the rev_info directly. So could/should we just be using
> pp->rev->encode_email_headers here?
>
> Or if that field is not always set (or we want to override some
> elements), should there be a single helper function to initialize
> the pretty_print_context from a rev_info, that could be shared
> between spots like show_log() and make_cover_letter()?
>
> I don't think that answering those questions needs to hold up your
> patch. We can take it as a quick fix for a real bug, and then anybody
> interested can dig further as a separate topic on top.
These are good questions indeed. Unfortunately I don't hink I'll have time to
work on this though.
> > +test_expect_success 'cover letter with --cover-from-description subject (UTF-8 subject line)' '
> > + test_config branch.rebuild-1.description "Café?
> > +
> > +body" &&
> > + git checkout rebuild-1 &&
> > + git format-patch --stdout --cover-letter --cover-from-description subject --encode-email-headers main >actual &&
> > + grep "^Subject: \[PATCH 0/2\] =?UTF-8?q?Caf=C3=A9=3F?=$" actual &&
> > + ! grep "Café" actual
> > +'
>
> The test looks correct to me.
>
> Some of these long lines (and the in-string newlines!) make this ugly
> and hard to read. But it is also just copying the already-ugly style of
> nearby tests. So I'm OK with that. But a prettier version might be:
>
> test_expect_success 'cover letter respects --encode-email-headers' '
> test_config branch.rebuild-1.description "Café?" &&
> git checkout rebuild-1 &&
> git format-patch --stdout --encode-email-headers \
> --cover-letter --cover-from-description=subject \
> main >actual &&
> ...
> '
Yeah, that sounds better indeed. Let me know if you want me to resend a cleaner
version of the test.
> I also wondered if we could be just be testing this much more easily
> with another header like "--to". But I guess that would be found in both
> the cover letter and the actual patches (we also don't seem to encode
> it even in the regular patches; is that a bug?).
That sounds like another bug indeed… But maybe that'll be harder to fix. To
Q-encode this field one needs to split off the full name and actual mail
address ("André <andre@example.org>" would be split into "André" and
"andre@example.org"), then Q-encode the full name, then join the strings
together again. In particular, it's incorrect to Q-encode the full string.
^ permalink raw reply
* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Patrick Steinhardt @ 2023-11-10 10:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqq8r76zg1j.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]
On Fri, Nov 10, 2023 at 08:02:32AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
> >> process_subtree_split_trailer () {
> >> - assert test $# = 2 -o $# = 3
> >> + assert test $# -ge 2
> >> + assert test $# -le 3
> >
> > It took me a minute to figure out why we were swapping "=" for "-ge". It
> > is because we want to logical-OR the two conditions, but "assert"
> > requires that we test one at a time. I think that is probably worth
> > explaining in the commit message.
>
> I wish we could write something like
>
> assert test $# -ge 2 && test $# -le 3
>
> (and I'd allow double quoting the whole thing after assert if
> needed) but we cannot do so without tweaking the implementation of
> assert.
>
> >
> >> @@ -916,7 +919,7 @@ cmd_split () {
> >> if test $# -eq 0
> >> then
> >> rev=$(git rev-parse HEAD)
> >> - elif test $# -eq 1 -o $# -eq 2
> >> + elif test $# -eq 1 || test $# -eq 2
> >
> > OK, this one is a straight-forward use of "||".
>
> Yes, but why not consistently use the range notation like the
> earlier one here, or below?
I opted to go for the "obvious" conversion, if there was one easily
available, to make the diff easier to read. We could of course use a
range notation like this:
rev=$(git rev-parse HEAD)
- elif test $# -eq 1 || test $# -eq 2
+ elif test $# -ge 1 && test $# -le 2
then
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
Or :
rev=$(git rev-parse HEAD)
- elif test $# -eq 1 || test $# -eq 2
+ elif ! { test $# -lt 1 || test $# -gt 2; }
then
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
But both of these are not consistent with the other cases due to the
negation here, and both of them are harder to read in my opinion. So
I'm not sure whether we gain anything by trying to make this a bit more
consistent with the other conversions.
Patrick
> elif test $# -ge 1 && test $# -le 2
>
> >> cmd_merge () {
> >> - test $# -eq 1 -o $# -eq 2 ||
> >> + if test $# -lt 1 || test $# -gt 2
> >> ...
> > (I am OK with either, it just took me a minute to verify that your
> > conversion was correct. But that is a one-time issue now while
> > reviewing, and I think the code is readable going forward).
>
> Yeah, the end result looks good.
>
> Thanks, both.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Patrick Steinhardt @ 2023-11-10 10:18 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20231109185515.GD2711684@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2282 bytes --]
On Thu, Nov 09, 2023 at 01:55:15PM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 11:53:35AM +0100, Patrick Steinhardt wrote:
>
> > Functions in git-subtree.sh all assert that they are being passed the
> > correct number of arguments. In cases where we accept a variable number
> > of arguments we assert this via a single call to `test` with `-o`, which
> > is discouraged by our coding guidelines.
> >
> > Convert these cases to stop doing so.
>
> OK. I think these ones really are safe, because they're only expanding
> $#, but I agree with the principle to follow the guidelines.
>
> > # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
> > process_subtree_split_trailer () {
> > - assert test $# = 2 -o $# = 3
> > + assert test $# -ge 2
> > + assert test $# -le 3
>
> It took me a minute to figure out why we were swapping "=" for "-ge". It
> is because we want to logical-OR the two conditions, but "assert"
> requires that we test one at a time. I think that is probably worth
> explaining in the commit message.
I really hate to admit how long I've pondered over this patch series in
total, up to the point where I did a `git rebase --reset-author-date` at
the end just so that it's not obvious. So I totally get everyone who
needs to stop and think for a bit here.
Will adapt the commit message.
Patrick
> > @@ -916,7 +919,7 @@ cmd_split () {
> > if test $# -eq 0
> > then
> > rev=$(git rev-parse HEAD)
> > - elif test $# -eq 1 -o $# -eq 2
> > + elif test $# -eq 1 || test $# -eq 2
>
> OK, this one is a straight-forward use of "||".
>
> > cmd_merge () {
> > - test $# -eq 1 -o $# -eq 2 ||
> > + if test $# -lt 1 || test $# -gt 2
> > + then
> > die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
> > + fi
> > +
>
> But here we swap "-eq" for other operators. We have to because we went
> from "||" to an "if". I think what you have here is correct, but you
> could also write:
>
> if ! { test $# -eq 1 || test $# -eq 2; }
>
> (I am OK with either, it just took me a minute to verify that your
> conversion was correct. But that is a one-time issue now while
> reviewing, and I think the code is readable going forward).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Patrick Steinhardt @ 2023-11-10 10:18 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20231109184843.GC2711684@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]
On Thu, Nov 09, 2023 at 01:48:43PM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 08:41:33PM +0900, Junio C Hamano wrote:
>
> > > -elif test -d ${GIT_DIR:-.git} -o -f .git &&
> > > +elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&
> >
> > I do not think this is strictly necessary.
> >
> > Because the command line parser of "test" comes from left, notices
> > "-d" and takes the next one to check if it is a directory. There is
> > no value in ${GIT_DIR} can make "test -d ${GIT_DIR} -o ..." fail the
> > same way as the problem Peff pointed out during the discussion.
>
> I think this is one of the ambiguous cases. If $GIT_DIR is "=", then
> "test" cannot tell if you meant:
>
> var1=-d
> var2=-o
> test "$var1" = "$var2" ...
>
> or:
>
> var1="="
> test -d "$var1" -o ...
>
> With bash, for example:
>
> $ test -d /tmp -o -f .git; echo $?
> 0
> $ test -d = -o -f .git; echo $?
> bash: test: syntax error: `-f' unexpected
> 2
>
> Without "-o", it uses the number of arguments to disambiguate (though of
> course the lack of quotes around $GIT_DIR is another potential problem
> here).
Right, let me fix the missing quoting while at it.
> And I think the same is true of the other cases below using "-z", "-n",
> and so on.
>
> But IMHO it is worth getting rid of all -o/-a regardless. Even
> non-ambiguous cases make reasoning about the code harder, and we don't
> want to encourage people to think they're OK to use.
Agreed. I'll amend the commit message to say so.
> > I do not need a subshell for grouping, either. Plain {} should do
> > (but you may need a LF or semicolon after the statement)..
>
> This I definitely agree with. :)
Will fix.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 4/4] Makefile: stop using `test -o` when unlinking duplicate executables
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699609940.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]
When building executables we may end up with both `foo` and `foo.exe` in
the project's root directory. This can cause issues on Cygwin, which is
why we unlink the `foo` binary (see 6fc301bbf68 (Makefile: remove $foo
when $foo.exe is built/installed., 2007-01-10)). This step is skipped if
either:
- `foo` is a directory, which can happen when building Git on
Windows via MSVC (see ade2ca0ca9f (Do not try to remove
directories when removing old links, 2009-10-27)).
- `foo` is a hardlink to `foo.exe`, which can happen on Cygwin (see
0d768f7c8f1 (Makefile: building git in cygwin 1.7.0, 2008-08-15)).
These two conditions are currently chained together via `test -o`, which
is discouraged by our code style guide. Convert the recipe to instead
use an `if` statement with `&&`'d conditions, which both matches our
style guide and is easier to ready.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 03adcb5a480..1094a557711 100644
--- a/Makefile
+++ b/Makefile
@@ -2342,7 +2342,7 @@ profile-fast: profile-clean
all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
ifneq (,$X)
- $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
+ $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), if test ! -d '$p' && test ! '$p' -ef '$p$X'; then $(RM) '$p'; fi;)
endif
all::
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 3/4] contrib/subtree: convert subtree type check to use case statement
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699609940.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]
The `subtree_for_commit ()` helper function asserts that the subtree
identified by its parameters are either a commit or tree. This is done
via the `-o` parameter of test, which is discouraged.
Refactor the code to instead use a switch statement over the type.
Despite being aligned with our coding guidelines, the resulting code is
arguably also easier to read.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/subtree/git-subtree.sh | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8af0a81ba3f..3028029ac2d 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -641,10 +641,16 @@ subtree_for_commit () {
while read mode type tree name
do
assert test "$name" = "$dir"
- assert test "$type" = "tree" -o "$type" = "commit"
- test "$type" = "commit" && continue # ignore submodules
- echo $tree
- break
+
+ case "$type" in
+ commit)
+ continue;; # ignore submodules
+ tree)
+ echo $tree
+ break;;
+ *)
+ die "fatal: tree entry is of type ${type}, expected tree or commit";;
+ esac
done || exit $?
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699609940.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]
Functions in git-subtree.sh all assert that they are being passed the
correct number of arguments. In cases where we accept a variable number
of arguments we assert this via a single call to `test` with `-o`, which
is discouraged by our coding guidelines.
Convert these cases to stop doing so. This requires us to decompose
assertions of the style `assert test $# = 2 -o $# = 3` into two calls
because we have no easy way to logically chain statements passed to the
assert function.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/subtree/git-subtree.sh | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 43b5fec7320..8af0a81ba3f 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -373,7 +373,8 @@ try_remove_previous () {
# Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
process_subtree_split_trailer () {
- assert test $# = 2 -o $# = 3
+ assert test $# -ge 2
+ assert test $# -le 3
b="$1"
sq="$2"
repository=""
@@ -402,7 +403,8 @@ process_subtree_split_trailer () {
# Usage: find_latest_squash DIR [REPOSITORY]
find_latest_squash () {
- assert test $# = 1 -o $# = 2
+ assert test $# -ge 1
+ assert test $# -le 2
dir="$1"
repository=""
if test "$#" = 2
@@ -455,7 +457,8 @@ find_latest_squash () {
# Usage: find_existing_splits DIR REV [REPOSITORY]
find_existing_splits () {
- assert test $# = 2 -o $# = 3
+ assert test $# -ge 2
+ assert test $# -le 3
debug "Looking for prior splits..."
local indent=$(($indent + 1))
@@ -916,7 +919,7 @@ cmd_split () {
if test $# -eq 0
then
rev=$(git rev-parse HEAD)
- elif test $# -eq 1 -o $# -eq 2
+ elif test $# -eq 1 || test $# -eq 2
then
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
@@ -1006,8 +1009,11 @@ cmd_split () {
# Usage: cmd_merge REV [REPOSITORY]
cmd_merge () {
- test $# -eq 1 -o $# -eq 2 ||
+ if test $# -lt 1 || test $# -gt 2
+ then
die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
+ fi
+
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
repository=""
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699609940.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4513 bytes --]
Our coding guidelines say to not use `test` with `-a` and `-o` because
it can easily lead to bugs. Convert trivial cases where we still use
these to instead instead concatenate multiple invocations of `test` via
`&&` and `||`, respectively.
While not all of the converted instances can cause ambiguity, it is
worth getting rid of all of them regardless:
- It becomes easier to reason about the code as we do not have to
argue why one use of `-a`/`-o` is okay while another one isn't.
- We don't encourage people to use these expressions.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
GIT-VERSION-GEN | 2 +-
configure.ac | 2 +-
contrib/subtree/git-subtree.sh | 4 ++--
t/perf/perf-lib.sh | 2 +-
t/perf/run | 9 +++++----
t/valgrind/valgrind.sh | 2 +-
6 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index e54492f8271..7246ab7c78c 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -11,7 +11,7 @@ LF='
if test -f version
then
VN=$(cat version) || VN="$DEF_VER"
-elif test -d ${GIT_DIR:-.git} -o -f .git &&
+elif { test -d "${GIT_DIR:-.git}" || test -f .git; } &&
VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
case "$VN" in
*$LF*) (exit 1) ;;
diff --git a/configure.ac b/configure.ac
index 276593cd9dd..d1a96da14eb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -94,7 +94,7 @@ AC_DEFUN([GIT_PARSE_WITH_SET_MAKE_VAR],
[AC_ARG_WITH([$1],
[AS_HELP_STRING([--with-$1=VALUE], $3)],
if test -n "$withval"; then
- if test "$withval" = "yes" -o "$withval" = "no"; then
+ if test "$withval" = "yes" || test "$withval" = "no"; then
AC_MSG_WARN([You likely do not want either 'yes' or 'no' as]
[a value for $1 ($2). Maybe you do...?])
fi
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index e0c5d3b0de6..43b5fec7320 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -489,13 +489,13 @@ find_existing_splits () {
;;
END)
debug "Main is: '$main'"
- if test -z "$main" -a -n "$sub"
+ if test -z "$main" && test -n "$sub"
then
# squash commits refer to a subtree
debug " Squash: $sq from $sub"
cache_set "$sq" "$sub"
fi
- if test -n "$main" -a -n "$sub"
+ if test -n "$main" && test -n "$sub"
then
debug " Prior: $main -> $sub"
cache_set $main $sub
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index e7786775a90..b952e5024b4 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -31,7 +31,7 @@ unset GIT_CONFIG_NOSYSTEM
GIT_CONFIG_SYSTEM="$TEST_DIRECTORY/perf/config"
export GIT_CONFIG_SYSTEM
-if test -n "$GIT_TEST_INSTALLED" -a -z "$PERF_SET_GIT_TEST_INSTALLED"
+if test -n "$GIT_TEST_INSTALLED" && test -z "$PERF_SET_GIT_TEST_INSTALLED"
then
error "Do not use GIT_TEST_INSTALLED with the perf tests.
diff --git a/t/perf/run b/t/perf/run
index 34115edec35..486ead21980 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -91,10 +91,10 @@ set_git_test_installed () {
run_dirs_helper () {
mydir=${1%/}
shift
- while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
+ while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
shift
done
- if test $# -gt 0 -a "$1" = --; then
+ if test $# -gt 0 && test "$1" = --; then
shift
fi
@@ -124,7 +124,7 @@ run_dirs_helper () {
}
run_dirs () {
- while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
+ while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
run_dirs_helper "$@"
shift
done
@@ -180,7 +180,8 @@ run_subsection () {
GIT_PERF_AGGREGATING_LATER=t
export GIT_PERF_AGGREGATING_LATER
- if test $# = 0 -o "$1" = -- -o -f "$1"; then
+ if test $# = 0 || test "$1" = -- || test -f "$1"
+ then
set -- . "$@"
fi
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 669ebaf68be..9fbf90cee7c 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -23,7 +23,7 @@ memcheck)
VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
test 3 -gt "$VALGRIND_MAJOR" ||
- test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
+ ( test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR" ) ||
TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
;;
*)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>`
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699526999.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3171 bytes --]
Hi,
this is the second version of my patch series that replaces all uses of
`test <expr> -o/a <expr`.
Changes compared to v1:
- I've expanded a bit on why we want to do these conversions in the
first place in the first commit message.
- Dropped a needless subshell and added missing quoting while at it.
- Explained why we need to decompose the asserts in the second patch
into two asserts.
Thanks!
Patrick
Patrick Steinhardt (4):
global: convert trivial usages of `test <expr> -a/-o <expr>`
contrib/subtree: stop using `-o` to test for number of args
contrib/subtree: convert subtree type check to use case statement
Makefile: stop using `test -o` when unlinking duplicate executables
GIT-VERSION-GEN | 2 +-
Makefile | 2 +-
configure.ac | 2 +-
contrib/subtree/git-subtree.sh | 34 +++++++++++++++++++++++-----------
t/perf/perf-lib.sh | 2 +-
t/perf/run | 9 +++++----
t/valgrind/valgrind.sh | 2 +-
7 files changed, 33 insertions(+), 20 deletions(-)
Range-diff against v1:
1: c5e627eb3fe ! 1: 2967c8ebb46 global: convert trivial usages of `test <expr> -a/-o <expr>`
@@ Commit message
these to instead instead concatenate multiple invocations of `test` via
`&&` and `||`, respectively.
+ While not all of the converted instances can cause ambiguity, it is
+ worth getting rid of all of them regardless:
+
+ - It becomes easier to reason about the code as we do not have to
+ argue why one use of `-a`/`-o` is okay while another one isn't.
+
+ - We don't encourage people to use these expressions.
+
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## GIT-VERSION-GEN ##
@@ GIT-VERSION-GEN: LF='
then
VN=$(cat version) || VN="$DEF_VER"
-elif test -d ${GIT_DIR:-.git} -o -f .git &&
-+elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&
++elif { test -d "${GIT_DIR:-.git}" || test -f .git; } &&
VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
case "$VN" in
*$LF*) (exit 1) ;;
2: b1ea45b8a88 ! 2: 977132d2236 contrib/subtree: stop using `-o` to test for number of args
@@ Commit message
of arguments we assert this via a single call to `test` with `-o`, which
is discouraged by our coding guidelines.
- Convert these cases to stop doing so.
+ Convert these cases to stop doing so. This requires us to decompose
+ assertions of the style `assert test $# = 2 -o $# = 3` into two calls
+ because we have no easy way to logically chain statements passed to the
+ assert function.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
3: 7c54d9070fa = 3: 761cde1b341 contrib/subtree: convert subtree type check to use case statement
4: bc9489ca5b8 = 4: 5326d86888a Makefile: stop using `test -o` when unlinking duplicate executables
base-commit: dadef801b365989099a9929e995589e455c51fed
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] glossary: add definitions for dereference & peel
From: Kristoffer Haugsbakk @ 2023-11-10 8:28 UTC (permalink / raw)
To: Josh Soref; +Cc: Patrick Steinhardt, Victoria Dye, git
In-Reply-To: <pull.1610.git.1699574277143.gitgitgadget@gmail.com>
On Fri, Nov 10, 2023, at 00:57, Victoria Dye via GitGitGadget wrote:
> +[[def_peel]]peel::
> + Synonym for object <<def_dereference,dereference>>. Most commonly used
> + in the context of tags, where it refers to the process of recursively
> + dereferencing a <<def_tag_object,tag object>> until the result object's
> + <<def_object_type,type>> is something other than "tag".
As a user I like that this is classified as a synonym. Because if I wanted
to ask StackOverflow about how to get to the commit that a tag points to
then I would use the term “dereference a tag”.
--
Kristoffer Haugsbakk
^ permalink raw reply
* [PATCH v4 3/3] t9164: fix inability to find basename(1) in Subversion hooks
From: Patrick Steinhardt @ 2023-11-10 8:17 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699596457.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]
Hooks executed by Subversion are spawned with an empty environment. By
default, not even variables like PATH will be propagated to them. In
order to ensure that we're still able to find required executables, we
thus write the current PATH variable into the hook script itself and
then re-export it in t9164.
This happens too late in the script though, as we already tried to
execute the basename(1) utility before exporting the PATH variable. This
tends to work on most platforms as the fallback value of PATH for Bash
(see `getconf PATH`) is likely to contain this binary. But on more
exotic platforms like NixOS this is not the case, and thus the test
fails.
While we could work around this issue by simply setting PATH earlier, it
feels fragile to inject a user-controlled value into the script and have
the shell interpret it. Instead, we can refactor the hook setup to write
a `hooks-env` file that configures PATH for us. Like this, Subversion
will know to set up the environment as expected for all hooks.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t9164-git-svn-dcommit-concurrent.sh | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index c8e6c0733f4..d1dec89c3b7 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -46,6 +46,14 @@ setup_hook()
"passed to setup_hook" >&2 ; return 1; }
echo "cnt=$skip_revs" > "$hook_type-counter"
rm -f "$rawsvnrepo/hooks/"*-commit # drop previous hooks
+
+ # Subversion hooks run with an empty environment by default. We thus
+ # need to propagate PATH so that we can find executables.
+ cat >"$rawsvnrepo/conf/hooks-env" <<-EOF
+ [default]
+ PATH = ${PATH}
+ EOF
+
hook="$rawsvnrepo/hooks/$hook_type"
cat > "$hook" <<- 'EOF1'
#!/bin/sh
@@ -63,7 +71,6 @@ EOF1
if [ "$hook_type" = "pre-commit" ]; then
echo "echo 'commit disallowed' >&2; exit 1" >>"$hook"
else
- echo "PATH=\"$PATH\"; export PATH" >>"$hook"
echo "svnconf=\"$svnconf\"" >>"$hook"
cat >>"$hook" <<- 'EOF2'
cd work-auto-commits.svn
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v4 2/3] t/lib-httpd: stop using legacy crypt(3) for authentication
From: Patrick Steinhardt @ 2023-11-10 8:17 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699596457.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]
When setting up httpd for our tests, we also install a passwd and
proxy-passwd file that contain the test user's credentials. These
credentials currently use crypt(3) as the password encryption schema.
This schema can be considered deprecated nowadays as it is not safe
anymore. Quoting Apache httpd's documentation [1]:
> Unix only. Uses the traditional Unix crypt(3) function with a
> randomly-generated 32-bit salt (only 12 bits used) and the first 8
> characters of the password. Insecure.
This is starting to cause issues in modern Linux distributions. glibc
has deprecated its libcrypt library that used to provide crypt(3) in
favor of the libxcrypt library. This newer replacement provides a
compile time switch to disable insecure password encryption schemata,
which causes crypt(3) to always return `EINVAL`. The end result is that
httpd tests that exercise authentication will fail on distros that use
libxcrypt without these insecure encryption schematas.
Regenerate the passwd files to instead use the default password
encryption schema, which is md5. While it feels kind of funny that an
MD5-based encryption schema should be more secure than anything else, it
is the current default and supported by all platforms. Furthermore, it
really doesn't matter all that much given that these files are only used
for testing purposes anyway.
[1]: https://httpd.apache.org/docs/2.4/misc/password_encryptions.html
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/lib-httpd/passwd | 2 +-
t/lib-httpd/proxy-passwd | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index 99a34d64874..d9c122f3482 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:xb4E8pqD81KQs
+user@host:$apr1$LGPmCZWj$9vxEwj5Z5GzQLBMxp3mCx1
diff --git a/t/lib-httpd/proxy-passwd b/t/lib-httpd/proxy-passwd
index 77c25138e07..2ad7705d9a3 100644
--- a/t/lib-httpd/proxy-passwd
+++ b/t/lib-httpd/proxy-passwd
@@ -1 +1 @@
-proxuser:2x7tAukjAED5M
+proxuser:$apr1$RxS6MLkD$DYsqQdflheq4GPNxzJpx5.
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox