* Re: [PATCH v2 1/2] rebase: remove completely useless -C option
From: Elijah Newren @ 2023-01-20 6:42 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Elijah Newren via GitGitGadget, git, Derrick Stolee
In-Reply-To: <CAPig+cQj4Gi+ikkPz3wffqGZVz5uALGzYV25nio3k4+cthP9Uw@mail.gmail.com>
On Thu, Jan 19, 2023 at 9:40 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Jan 20, 2023 at 12:14 AM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > The `-C` option to rebase was introduced with 67dad687ad ("add -C[NUM]
> > to git-am", 2007-02-08). Based on feedback on the patch, the author
> > added the -C option not just to git-am but also to git-rebase. But did
> > it such that the option was just passed through to git-am (which passes
> > it along to git-apply), with no corresponding option to format-patch.
> >
> > As per the git-apply documentation for the `-C` option:
> > Ensure at least <n> lines of surrounding context match...When fewer
> > lines of surrounding context exist they all must match.
> >
> > The fact that format-patch was not passed a -U option to increase the
> > number of context lines meant that there would still only be 3 lines of
> > context to match on. So, anyone attempting to use this option in
> > git-rebase would just be spinning their wheels and wasting time. I was
> > one such user a number of years ago.
> >
> > Since this option can at best waste users time and is nothing more than
> > a confusing no-op, and has never been anything other than a confusing
> > no-op, and no one has ever bothered to create a testcase for it that
> > goes beyond option parsing, simply excise the option.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
>
> Is there a chance that this patch could break some existing tooling or
> someone's workflow or alias? If so, perhaps it would be better to
> continue recognizing it, but as a proper no-op? That is:
>
> (1) continue accepting the option but mark it PARSE_OPT_HIDDEN or some such
>
> (2) completely ignore the option
>
> (3) in case someone runs across it in some existing script or example
> and wants to know what it does, leave one mention of it in the
> documentation, such as:
>
> -C<n>
> Does nothing. This option is accepted for backward compatibility
> only. Do not use.
If an option becomes a no-op over time due to other changes, this
would likely be the route I'd lean towards. I'm just not sure it's
merited in this case.
We already turned numerous no-op choices in rebase into errors with
commit 8295ed690b ("rebase: make the backend configurable via config
setting", 2020-02-15), where we started pointing out that apply-based
and merge-based options were incompatible (as opposed to silently
accepting competing options and ignoring ones not supported by
whichever backend happened to trump at the time based on the options.)
So, there's recent precedent to jump directly to errors with no-ops
in rebase. Those cases are slightly different in that options were
only conditionally no-ops, whereas in this case we have an option that
is unconditionally a no-op, suggesting we might be able to do
something stronger.
On top of that, I just can't find any evidence of anyone ever using
this option: (a) it was only put in as an afterthought by the original
author (who was only really interested in git-am -C), (b) there are
absolutely no testcases in the testsuite covering its behavior, (c) my
searches of the mailing list and google don't turn up any hits though
admittedly it's hard to figure out what to search on, and (d) while I
tried to use the option at times, it was only because I was doing work
to make backends consistent and switch the default one and trying to
understand all the inner workings, and even then I gave up on it and
punted it down the road.
And, of course, the option never worked as advertised anyway.
That combination makes me think nuking it would go completely
unnoticed outside this mailing list. If others would rather see the
more cautious route despite all the above, I can implement it, but
likely alter your step (2) from ignoring into throwing an error
message (or at the very least a warning).
^ permalink raw reply
* Re: Race condition on `git checkout -c`
From: Arthur Milchior @ 2023-01-20 9:01 UTC (permalink / raw)
To: Chris Torek; +Cc: git
In-Reply-To: <CAPx1GvcBDcZ1K_YJKm3+fUBNYQWKE2FBz-qS6JrV2TJCTc5k1w@mail.gmail.com>
Thank you very much for the explanation. I’m ever so happy not to be
working at the file system level nor on cross platform app.
Alas, it’s a computer configured by work, so not only do I have no
control over the disk configuration, but I’m not even allowed to plus
external storage. So I guess I’ll just remain with the case
almost-insensitive branch. Not like I expect this to occur again in
the future.
Good luck with whatever next change git plans.
Regards,
Arthur
On Thu, Jan 19, 2023 at 11:56 PM Chris Torek <chris.torek@gmail.com> wrote:
>
> (Top note: you mean `git checkout -b` or `git switch -c`, not `git
> checkout -c`.)
>
> On Thu, Jan 19, 2023 at 1:24 PM Arthur Milchior
> <arthur.milchior@gmail.com> wrote:
> >
> > I expect either:
> > * to see an error message stating that `b` already exists
> > * to see `b` and `B` in the list of branch.
>
> [snip]
>
> > uname: Darwin 22.2.0 Darwin Kernel Version 22.2.0: Fri Nov 11 02:03:51
>
> Darwin (macOS) is your problem here. The same problem
> occurs on Windows, but not on Linux, by default.
>
> Technically the problem is in the file system itself, combined with
> the ways (plural) that Git stores branch names.
>
> As far as Git itself is concerned, branch names are *always* case
> sensitive, so branches named `b` and `B` are different. But Git
> stores branch names in two different formats, at the moment:
>
> * Some are kept in a plain-text file `.git/packed-refs`.
> * Some are stored as directory-and-file-names in `.git/refs/`.
>
> If the OS's file system is case sensitive, as most standard Linux
> file systems are, there's no problem with the latter. But if the OS's
> file system is case-INsensitive, as the default file systems on
> Windows and MacOS are, this becomes a problem: the attempt
> to create both `refs/heads/b` and a different file, `refs/head/B`,
> fails, with one of the two names "winning" and the other attempt
> to create a new name simply re-using the existing name.
>
> If you create a case-sensitive disk volume on your Mac, which
> can be a simple click-to-mount virtual drive within your regular
> case-insensitive file system, you can happily use Git without this
> complication. Note that the same complication applies to file
> names: Linux users can create two different, separate files
> named `README.TXT` and `ReadMe.txt` in a GIt project, and
> if you use the default case-insensitive macOS file system, you
> won't be able to check out both files. Using your case sensitive
> volume will allow you to work with the Linux group.
>
> If and when a future version of Git starts using reftables instead
> of the file system to store branch and tag names, this particular
> issue will go away, but until then, I recommend keeping a case
> sensitive volume handy on your mac, and more generally,
> avoiding mixing upper and lower case branch and/or file names
> (at all, ever) whenever possible. This avoids a lot of problems,
> though nothing can get you past the Windows `aux.h` problem. :-)
>
> Chris
^ permalink raw reply
* Re: [PATCH] ssh signing: better error message when key not in agent
From: Fabian Stelzer @ 2023-01-20 9:03 UTC (permalink / raw)
To: phillip.wood; +Cc: Adam Szkoda, Adam Szkoda via GitGitGadget, git
In-Reply-To: <55282dec-825f-8c4b-1fb0-6e26ec326db1@dunelm.org.uk>
On 18.01.2023 16:29, Phillip Wood wrote:
>Hi Adam
>
>I've cc'd Fabian who knows more about the ssh signing code that I do.
>
>On 18/01/2023 15:28, Adam Szkoda wrote:
>>Hi Phillip,
>>
>>Good point! My first thought is to try doing a stat() syscall on the
>>path from 'user.signingKey' to see if it exists and if not, treat it
>>as a public key (and pass the -U option). If that sounds reasonable,
>>I can update the patch.
>
>My reading of the documentation is that user.signingKey may point to a
>public or private key so I'm not sure how stat()ing would help.
>Looking at the code in sign_buffer_ssh() we have a function
>is_literal_ssh_key() that checks if the config value is a public key.
>When the user passes the path to a key we could read the file check
>use is_literal_ssh_key() to check if it is a public key (or possibly
>just check if the file begins with "ssh-"). Fabian - does that sound
>reasonable?
Hi,
I have encountered the mentioned problem before as well and tried to fix it
but did not find a good / reasonable way to do so. Git just passes the
user.signingKey to ssh-keygen which states:
`The key used for signing is specified using the -f option and may refer to
either a private key, or a public key with the private half available via
ssh-agent(1)`
I don't think it's a good idea for git to parse the key and try to determine
if it's public or private. The fix should probably be in openssh (different
error message) but when looking into it last time i remember that the logic
for using the key is quite deeply embedded into the ssh code and not easily
adjusted for the signing use case. At the moment I don't have the time to
look into it but the openssh code for signing is quite readable so feel free
to give it a try. Maybe you find a good way.
Best regards,
Fabian
>
>Best Wishes
>
>Phillip
>
>>Best
>>— Adam
>>
>>
>>On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>
>>>On 18/01/2023 11:10, Phillip Wood wrote:
>>>>>the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All
>>>>>that
>>>>>needs to be done is to pass an additional backward-compatible option
>>>>>-U to
>>>>>'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets
>>>>>the file
>>>>>as public key and expects to find the private key in the agent.
>>>>
>>>>The documentation for user.signingKey says
>>>>
>>>> If gpg.format is set to ssh this can contain the path to either your
>>>>private ssh key or the public key when ssh-agent is used.
>>>>
>>>>If I've understood correctly passing -U will prevent users from setting
>>>>this to a private key.
>>>
>>>If there is an easy way to tell if the user has given us a public key
>>>then we could pass "-U" in that case.
>>>
>>>Best Wishes
>>>
>>>Phillip
^ permalink raw reply
* Re: [PATCH v2 1/2] docs: fix sparse-checkout docs link
From: Martin Ågren @ 2023-01-20 9:35 UTC (permalink / raw)
To: Elijah Newren
Cc: ZheNing Hu via GitGitGadget, git,
Ævar Arnfjörð Bjarmason, Junio C Hamano,
Derrick Stolee, Victoria Dye,
Nguyễn Thái Ngọc Duy, ZheNing Hu
In-Reply-To: <CABPp-BEC4PmQfYT=UhtbJ5QcXXMqwF1e-KvSVVDNjDy69bGH5w@mail.gmail.com>
On Fri, 20 Jan 2023 at 06:29, Elijah Newren <newren@gmail.com> wrote:
>
> On Thu, Jan 19, 2023 at 9:34 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: ZheNing Hu <adlternative@gmail.com>
> > So fix the format of sparse-checkout.txt, and link it in the
> > Makefile to correct that.
> > -0. Behavior A is not well supported in Git. (Behavior B didn't used to
> > - be either, but was the easier of the two to implement.)
> > +Behavior A is not well supported in Git. (Behavior B didn't used to
> > +be either, but was the easier of the two to implement.)
>
> Why did you remove the numbering on this, though? If asciidoc doesn't
> like starting with 0 (the only guess I can think of for why you'd
> change this), shouldn't the series be renumbered starting at 1 rather
> than removing this from the list?
It looks like the zero causes both asciidoc and Asciidoctor to emit
warnings (one per item, since each item's number is off by one). They
also helpfully relabel everything starting at 1.
I agree that there's a better fix here than dropping the 0. Either
renumber everything or, probably better, just use "." for each item
rather than "1.", "2." and so on. The right numbers will be inserted
automatically. This also means that if an item is ever added earlier in
the list, we won't need to update all the numbers below that point.
(The numbers being generated automatically means we can't refer to them
("see item 2") without potentially getting out of sync, but that is true
regardless if the numbers are corrected for us, as now, or if we just
use ".".)
The contents of these list items could be prettified in various ways,
but I'm not sure what the status and goal is for these technical/
documents. Avoiding warnings in the build process, as ZheNing aimed for,
seems like a good idea regardless.
Martin
^ permalink raw reply
* Re: [PATCH v2 1/2] rebase: remove completely useless -C option
From: Martin Ågren @ 2023-01-20 9:55 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Derrick Stolee, Elijah Newren
In-Reply-To: <a0f8f5fac1c3f79cd46b943e95636728677dffef.1674190573.git.gitgitgadget@gmail.com>
On Fri, 20 Jan 2023 at 06:27, Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> As per the git-apply documentation for the `-C` option:
> Ensure at least <n> lines of surrounding context match...When fewer
> lines of surrounding context exist they all must match.
>
> The fact that format-patch was not passed a -U option to increase the
> number of context lines meant that there would still only be 3 lines of
> context to match on. So, anyone attempting to use this option in
> git-rebase would just be spinning their wheels and wasting time. I was
> one such user a number of years ago.
I suppose someone might have something like GIT_DIFF_OPTS="--unified=20"
meaning they would actually have more context for their `-C` to act on.
So I guess there is a chance that someone somewhere has actually been
able to make use of `git rebase -C` after all? I'm not really arguing
either way -- just noting the possibility, as remote as it may be.
Martin
^ permalink raw reply
* [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees
From: Carlo Marcelo Arenas Belón @ 2023-01-20 11:35 UTC (permalink / raw)
To: git
Cc: pclouds, gitster, Carlo Marcelo Arenas Belón, Jinwook Jeong,
Eric Sunshine, Rubén Justo, Phillip Wood
In-Reply-To: <20230119055325.1013-1-carenas@gmail.com>
Commands `git switch -C` and `git checkout -B` neglect to check whether
the provided branch is already checked out in some other worktree, as
shown by the following:
$ git worktree list
.../foo beefb00f [main]
$ git worktree add ../other
Preparing worktree (new branch 'other')
HEAD is now at beefb00f first
$ cd ../other
$ git switch -C main
Switched to and reset branch 'main'
$ git worktree list
.../foo beefb00f [main]
.../other beefb00f [main]
Fix this problem by teaching `git switch -C` and `git checkout -B` to
check whether the branch in question is already checked out elsewhere.
Unlike what it is done for `git switch` and `git checkout`, that have
an historical exception to ignore other worktrees if the branch to
check is the current one (as required when called as part of other
tools), the logic implemented is more strict and will require the user
to invoke the command with `--ignore-other-worktrees` to explicitly
indicate they want the risky behaviour.
This matches the current behaviour of `git branch -f` and is safer; for
more details see the tests in t2400.
Reported-by: Jinwook Jeong <vustthat@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Rubén Justo <rjusto@gmail.com>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Changes since v3
* Code and Tests improvements as suggested by Phillip
* Disable unreliable test that triggers a known bug
Changes since v2
* A leak free implementation
* More details in commit as suggested by Junio
Changes since v1
* A much better commit message
* Changes to the tests as suggested by Eric
* Changes to the logic as suggested by Rubén
builtin/checkout.c | 32 ++++++++++++++++++++++++--------
t/t2400-worktree-add.sh | 34 +++++++++++++++++++++++++++-------
2 files changed, 51 insertions(+), 15 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3fa29a08ee..0688652f99 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)
}
static int checkout_branch(struct checkout_opts *opts,
- struct branch_info *new_branch_info)
+ struct branch_info *new_branch_info,
+ char *check_branch_path)
{
if (opts->pathspec.nr)
die(_("paths cannot be used with switching branches"));
@@ -1533,13 +1534,13 @@ static int checkout_branch(struct checkout_opts *opts,
if (!opts->can_switch_when_in_progress)
die_if_some_operation_in_progress();
- if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
- !opts->ignore_other_worktrees) {
+ if (!opts->ignore_other_worktrees && !opts->force_detach &&
+ check_branch_path && ref_exists(check_branch_path)) {
int flag;
char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag);
- if (head_ref &&
- (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))
- die_if_checked_out(new_branch_info->path, 1);
+ if (opts->new_branch_force || (head_ref &&
+ (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_path))))
+ die_if_checked_out(check_branch_path, 1);
free(head_ref);
}
@@ -1627,7 +1628,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
const char * const usagestr[],
struct branch_info *new_branch_info)
{
+ int ret;
int parseopt_flags = 0;
+ char *check_branch_path = NULL;
opts->overwrite_ignore = 1;
opts->prefix = prefix;
@@ -1717,6 +1720,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
opts->new_branch = argv0 + 1;
}
+ if (opts->new_branch && !opts->ignore_other_worktrees) {
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_branchname(&buf, opts->new_branch, INTERPRET_BRANCH_LOCAL);
+ strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
+ check_branch_path = strbuf_detach(&buf, NULL);
+ }
/*
* Extract branch name from command line arguments, so
* all that is left is pathspecs.
@@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
new_branch_info, opts, &rev);
argv += n;
argc -= n;
+
+ if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
+ check_branch_path = xstrdup(new_branch_info->path);
} else if (!opts->accept_ref && opts->from_treeish) {
struct object_id rev;
@@ -1817,9 +1830,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
}
if (opts->patch_mode || opts->pathspec.nr)
- return checkout_paths(opts, new_branch_info);
+ ret = checkout_paths(opts, new_branch_info);
else
- return checkout_branch(opts, new_branch_info);
+ ret = checkout_branch(opts, new_branch_info, check_branch_path);
+
+ free(check_branch_path);
+ return ret;
}
int cmd_checkout(int argc, const char **argv, const char *prefix)
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index d587e0b20d..7ab7e87440 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -118,14 +118,17 @@ test_expect_success '"add" worktree creating new branch' '
)
'
-test_expect_success 'die the same branch is already checked out' '
+test_expect_success 'die if the same branch is already checked out' '
(
cd here &&
- test_must_fail git checkout newmain
+ test_must_fail git checkout newmain &&
+ test_must_fail git checkout -B newmain &&
+ test_must_fail git switch newmain &&
+ test_must_fail git switch -C newmain
)
'
-test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '
+test_expect_success SYMLINKS 'die if the same branch is already checked out (symlink)' '
head=$(git -C there rev-parse --git-path HEAD) &&
ref=$(git -C there symbolic-ref HEAD) &&
rm "$head" &&
@@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin
test_must_fail git -C here checkout newmain
'
-test_expect_success 'not die the same branch is already checked out' '
+test_expect_success 'allow creating multiple worktrees for same branch with force' '
+ git worktree add --force anothernewmain newmain
+'
+
+test_expect_success 'allow checkout/reset from the conflicted branch' '
(
cd here &&
- git worktree add --force anothernewmain newmain
+ git checkout -b conflictedmain newmain &&
+ git checkout -B conflictedmain newmain &&
+ git switch -C conflictedmain newmain
+ )
+'
+
+test_expect_success 'and not die on re-checking out current branch even if conflicted' '
+ (
+ cd there &&
+ git checkout newmain &&
+ git switch newmain
)
'
-test_expect_success 'not die on re-checking out current branch' '
+test_expect_failure 'unless using force without --ignore-other-worktrees' '
(
cd there &&
- git checkout newmain
+ test_must_fail git checkout -B newmain &&
+ test_must_fail git switch -C newmain &&
+ git checkout --ignore-other-worktrees -B newmain &&
+ git switch --ignore-other-worktrees -C newmain
)
'
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply related
* Re: [PATCH v2 1/2] rebase: remove completely useless -C option
From: Junio C Hamano @ 2023-01-20 12:05 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Derrick Stolee, Elijah Newren
In-Reply-To: <a0f8f5fac1c3f79cd46b943e95636728677dffef.1674190573.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> The `-C` option to rebase was introduced with 67dad687ad ("add -C[NUM]
> to git-am", 2007-02-08).
> ...
> As per the git-apply documentation for the `-C` option:
> Ensure at least <n> lines of surrounding context match...When fewer
> lines of surrounding context exist they all must match.
>
> The fact that format-patch was not passed a -U option to increase the
> number of context lines meant that there would still only be 3 lines of
> context to match on.
I am afraid that this is only less than half true. Isn't the
intended use of -C<num> similar to how "patch --fuzz" is used?
That is, even when a patch does not cleanly apply with full context
in the incoming diff, by requiring *smaller* number of lines to
match, the diff *could* be forced to apply with reduced precision?
My read of "even if context has changed a bit" in the log of that
commit is exactly that. And for such a use case (which I think is
the primary use case for the feature), you do not need to futz with
the patch generation side at all.
commit 67dad687ad15d26d8e26f4d27874af0bc0965ce2
Author: Michael S. Tsirkin <mst@kernel.org>
Date: Thu Feb 8 15:57:08 2007 +0200
add -C[NUM] to git-am
Add -C[NUM] to git-am and git-rebase so that patches can be applied even
if context has changed a bit.
Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>
Signed-off-by: Junio C Hamano <junkio@cox.net>
^ permalink raw reply
* Re: [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Luben Tuikov @ 2023-01-20 13:07 UTC (permalink / raw)
To: Michael Strawbridge, git
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason
In-Reply-To: <20230120012459.920932-3-michael.strawbridge@amd.com>
On 2023-01-19 20:24, Michael Strawbridge wrote:
> To allow further flexibility in the Git hook, the SMTP header
> information of the email which git-send-email intends to send, is now
> passed as the 2nd argument to the sendemail-validate hook.
>
> As an example, this can be useful for acting upon keywords in the
> subject or specific email addresses.
>
> Cc: Luben Tuikov <luben.tuikov@amd.com>
> Cc: Junio C Hamano <gitster@pobox.com>
> Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Acked-by: Luben Tuikov <luben.tuikov@amd.com>
> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
> ---
> Documentation/githooks.txt | 27 ++++++++++++++++++----
> git-send-email.perl | 46 ++++++++++++++++++++++----------------
> t/t9001-send-email.sh | 27 ++++++++++++++++++++--
> 3 files changed, 75 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index a16e62bc8c..0decbfc92d 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -583,10 +583,29 @@ processed by rebase.
> sendemail-validate
> ~~~~~~~~~~~~~~~~~~
>
> -This hook is invoked by linkgit:git-send-email[1]. It takes a single parameter,
> -the name of the file that holds the e-mail to be sent. Exiting with a
> -non-zero status causes `git send-email` to abort before sending any
> -e-mails.
> +This hook is invoked by linkgit:git-send-email[1].
> +
> +It takes these command line arguments. They are,
> +1. the name of the file which holds the contents of the email to be sent.
> +2. The name of the file which holds the SMTP headers of the email.
> +
> +The SMTP headers are passed in the exact same way as they are passed to the
> +user's Mail Transport Agent (MTA). In effect, the email given to the user's
> +MTA, is the contents of $2 followed by the contents of $1.
> +
> +Below is an example for a few common headers. Take notice of the
"example of" not "for".
This maybe clearer:
"An example of a few common headers is shown below. Take notice ..."
> +capitalization and multi-line tab structure.
> +
> + From: Example <from@example.com>
> + To: to@example.com
> + Cc: cc@example.com,
> + A <author@example.com>,
> + One <one@example.com>,
> + two@example.com
> + Subject: PATCH-STRING
> +
> +Exiting with a non-zero status causes `git send-email` to abort
> +before sending any e-mails.
>
> fsmonitor-watchman
> ~~~~~~~~~~~~~~~~~~
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 42f135a266..0e595d6ac5 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -785,16 +785,31 @@ sub is_format_patch_arg {
> push @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1), @rev_list_opts);
> }
>
> -@files = handle_backup_files(@files);
> +if (defined $sender) {
> + $sender =~ s/^\s+|\s+$//g;
> + ($sender) = expand_aliases($sender);
> +} else {
> + $sender = $repoauthor->() || $repocommitter->() || '';
> +}
> +
> +# $sender could be an already sanitized address
> +# (e.g. sendemail.from could be manually sanitized by user).
> +# But it's a no-op to run sanitize_address on an already sanitized address.
> +$sender = sanitize_address($sender);
> +
> +$time = time - scalar $#files;
>
> if ($validate) {
> foreach my $f (@files) {
> unless (-p $f) {
> + pre_process_file($f, 1);
> validate_patch($f, $target_xfer_encoding);
> }
> }
> }
>
> +@files = handle_backup_files(@files);
> +
> if (@files) {
> unless ($quiet) {
> print $_,"\n" for (@files);
> @@ -1043,18 +1058,6 @@ sub file_declares_8bit_cte {
> }
> }
>
> -if (defined $sender) {
> - $sender =~ s/^\s+|\s+$//g;
> - ($sender) = expand_aliases($sender);
> -} else {
> - $sender = $repoauthor->() || $repocommitter->() || '';
> -}
> -
> -# $sender could be an already sanitized address
> -# (e.g. sendemail.from could be manually sanitized by user).
> -# But it's a no-op to run sanitize_address on an already sanitized address.
> -$sender = sanitize_address($sender);
> -
> my $to_whom = __("To whom should the emails be sent (if anyone)?");
> my $prompting = 0;
> if (!@initial_to && !defined $to_cmd) {
> @@ -1214,10 +1217,6 @@ sub make_message_id {
> #print "new message id = $message_id\n"; # Was useful for debugging
> }
>
> -
> -
> -$time = time - scalar $#files;
> -
> sub unquote_rfc2047 {
> local ($_) = @_;
> my $charset;
> @@ -2101,11 +2100,20 @@ sub validate_patch {
> chdir($repo->wc_path() or $repo->repo_path())
> or die("chdir: $!");
> local $ENV{"GIT_DIR"} = $repo->repo_path();
> +
> + my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
> +
> + require File::Temp;
> + my ($header_filehandle, $header_filename) = File::Temp::tempfile(
> + ".gitsendemail.header.XXXXXX", DIR => $repo->repo_path());
> + print $header_filehandle $header;
> +
> my @cmd = ("git", "hook", "run", "--ignore-missing",
> $hook_name, "--");
> - my @cmd_msg = (@cmd, "<patch>");
> - my @cmd_run = (@cmd, $target);
> + my @cmd_msg = (@cmd, "<patch>", "<header>");
> + my @cmd_run = (@cmd, $target, $header_filename);
> $hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
> + unlink($header_filehandle);
> chdir($cwd_save) or die("chdir: $!");
> }
> if ($hook_error) {
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 1130ef21b3..8a5c111a24 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -540,7 +540,7 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" '
> test_path_is_file my-hooks.ran &&
> cat >expect <<-EOF &&
> fatal: longline.patch: rejected by sendemail-validate hook
> - fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
> + fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
> warning: no patches were sent
> EOF
> test_cmp expect actual
> @@ -559,12 +559,35 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
> test_path_is_file my-hooks.ran &&
> cat >expect <<-EOF &&
> fatal: longline.patch: rejected by sendemail-validate hook
> - fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
> + fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
> warning: no patches were sent
> EOF
> test_cmp expect actual
> '
>
> +test_expect_success $PREREQ "--validate hook supports header argument" '
> + write_script my-hooks/sendemail-validate <<-\EOF &&
> + if test "$#" -ge 2
> + then
There appears to be an extra indentation of the "if" statement.
--
Regards,
Luben
^ permalink raw reply
* Re: [PATCH v9 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Michael Strawbridge @ 2023-01-20 14:25 UTC (permalink / raw)
To: Luben Tuikov, git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason
In-Reply-To: <e353df62-c189-755f-5536-5ea91177c55c@amd.com>
On 2023-01-20 08:07, Luben Tuikov wrote:
> On 2023-01-19 20:24, Michael Strawbridge wrote:
>> To allow further flexibility in the Git hook, the SMTP header
>> information of the email which git-send-email intends to send, is now
>> passed as the 2nd argument to the sendemail-validate hook.
>>
>> As an example, this can be useful for acting upon keywords in the
>> subject or specific email addresses.
>>
>> Cc: Luben Tuikov <luben.tuikov@amd.com>
>> Cc: Junio C Hamano <gitster@pobox.com>
>> Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Acked-by: Luben Tuikov <luben.tuikov@amd.com>
>> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
>> ---
>> Documentation/githooks.txt | 27 ++++++++++++++++++----
>> git-send-email.perl | 46 ++++++++++++++++++++++----------------
>> t/t9001-send-email.sh | 27 ++++++++++++++++++++--
>> 3 files changed, 75 insertions(+), 25 deletions(-)
>>
>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
>> index a16e62bc8c..0decbfc92d 100644
>> --- a/Documentation/githooks.txt
>> +++ b/Documentation/githooks.txt
>> @@ -583,10 +583,29 @@ processed by rebase.
>> sendemail-validate
>> ~~~~~~~~~~~~~~~~~~
>>
>> -This hook is invoked by linkgit:git-send-email[1]. It takes a single parameter,
>> -the name of the file that holds the e-mail to be sent. Exiting with a
>> -non-zero status causes `git send-email` to abort before sending any
>> -e-mails.
>> +This hook is invoked by linkgit:git-send-email[1].
>> +
>> +It takes these command line arguments. They are,
>> +1. the name of the file which holds the contents of the email to be sent.
>> +2. The name of the file which holds the SMTP headers of the email.
>> +
>> +The SMTP headers are passed in the exact same way as they are passed to the
>> +user's Mail Transport Agent (MTA). In effect, the email given to the user's
>> +MTA, is the contents of $2 followed by the contents of $1.
>> +
>> +Below is an example for a few common headers. Take notice of the
> "example of" not "for".
>
> This maybe clearer:
> "An example of a few common headers is shown below. Take notice ..."
Good idea - I've fixed it locally.
>> +capitalization and multi-line tab structure.
>> +
>> + From: Example <from@example.com>
>> + To: to@example.com
>> + Cc: cc@example.com,
>> + A <author@example.com>,
>> + One <one@example.com>,
>> + two@example.com
>> + Subject: PATCH-STRING
>> +
>> +Exiting with a non-zero status causes `git send-email` to abort
>> +before sending any e-mails.
>>
>> fsmonitor-watchman
>> ~~~~~~~~~~~~~~~~~~
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 42f135a266..0e595d6ac5 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -785,16 +785,31 @@ sub is_format_patch_arg {
>> push @files, $repo->command('format-patch', '-o', File::Temp::tempdir(CLEANUP => 1), @rev_list_opts);
>> }
>>
>> -@files = handle_backup_files(@files);
>> +if (defined $sender) {
>> + $sender =~ s/^\s+|\s+$//g;
>> + ($sender) = expand_aliases($sender);
>> +} else {
>> + $sender = $repoauthor->() || $repocommitter->() || '';
>> +}
>> +
>> +# $sender could be an already sanitized address
>> +# (e.g. sendemail.from could be manually sanitized by user).
>> +# But it's a no-op to run sanitize_address on an already sanitized address.
>> +$sender = sanitize_address($sender);
>> +
>> +$time = time - scalar $#files;
>>
>> if ($validate) {
>> foreach my $f (@files) {
>> unless (-p $f) {
>> + pre_process_file($f, 1);
>> validate_patch($f, $target_xfer_encoding);
>> }
>> }
>> }
>>
>> +@files = handle_backup_files(@files);
>> +
>> if (@files) {
>> unless ($quiet) {
>> print $_,"\n" for (@files);
>> @@ -1043,18 +1058,6 @@ sub file_declares_8bit_cte {
>> }
>> }
>>
>> -if (defined $sender) {
>> - $sender =~ s/^\s+|\s+$//g;
>> - ($sender) = expand_aliases($sender);
>> -} else {
>> - $sender = $repoauthor->() || $repocommitter->() || '';
>> -}
>> -
>> -# $sender could be an already sanitized address
>> -# (e.g. sendemail.from could be manually sanitized by user).
>> -# But it's a no-op to run sanitize_address on an already sanitized address.
>> -$sender = sanitize_address($sender);
>> -
>> my $to_whom = __("To whom should the emails be sent (if anyone)?");
>> my $prompting = 0;
>> if (!@initial_to && !defined $to_cmd) {
>> @@ -1214,10 +1217,6 @@ sub make_message_id {
>> #print "new message id = $message_id\n"; # Was useful for debugging
>> }
>>
>> -
>> -
>> -$time = time - scalar $#files;
>> -
>> sub unquote_rfc2047 {
>> local ($_) = @_;
>> my $charset;
>> @@ -2101,11 +2100,20 @@ sub validate_patch {
>> chdir($repo->wc_path() or $repo->repo_path())
>> or die("chdir: $!");
>> local $ENV{"GIT_DIR"} = $repo->repo_path();
>> +
>> + my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
>> +
>> + require File::Temp;
>> + my ($header_filehandle, $header_filename) = File::Temp::tempfile(
>> + ".gitsendemail.header.XXXXXX", DIR => $repo->repo_path());
>> + print $header_filehandle $header;
>> +
>> my @cmd = ("git", "hook", "run", "--ignore-missing",
>> $hook_name, "--");
>> - my @cmd_msg = (@cmd, "<patch>");
>> - my @cmd_run = (@cmd, $target);
>> + my @cmd_msg = (@cmd, "<patch>", "<header>");
>> + my @cmd_run = (@cmd, $target, $header_filename);
>> $hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
>> + unlink($header_filehandle);
>> chdir($cwd_save) or die("chdir: $!");
>> }
>> if ($hook_error) {
>> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
>> index 1130ef21b3..8a5c111a24 100755
>> --- a/t/t9001-send-email.sh
>> +++ b/t/t9001-send-email.sh
>> @@ -540,7 +540,7 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" '
>> test_path_is_file my-hooks.ran &&
>> cat >expect <<-EOF &&
>> fatal: longline.patch: rejected by sendemail-validate hook
>> - fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
>> + fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
>> warning: no patches were sent
>> EOF
>> test_cmp expect actual
>> @@ -559,12 +559,35 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
>> test_path_is_file my-hooks.ran &&
>> cat >expect <<-EOF &&
>> fatal: longline.patch: rejected by sendemail-validate hook
>> - fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
>> + fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
>> warning: no patches were sent
>> EOF
>> test_cmp expect actual
>> '
>>
>> +test_expect_success $PREREQ "--validate hook supports header argument" '
>> + write_script my-hooks/sendemail-validate <<-\EOF &&
>> + if test "$#" -ge 2
>> + then
> There appears to be an extra indentation of the "if" statement.
Good catch. It was a matter of spaces and tabs combining that wasn't
easy to see.
^ permalink raw reply
* Re: [PATCH 4/8] bundle-uri: download in creationToken order
From: Derrick Stolee @ 2023-01-20 14:56 UTC (permalink / raw)
To: Victoria Dye, Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen
In-Reply-To: <ede340d1-bce4-0c1d-7afb-4874a67d1803@github.com>
On 1/19/2023 1:32 PM, Victoria Dye wrote:> Derrick Stolee via GitGitGadget wrote:
>> +static int fetch_bundles_by_token(struct repository *r,
>> + struct bundle_list *list)
>> +{
>> + int cur;
>> + int pop_or_push = 0;
>> + struct bundle_list_context ctx = {
>> + .r = r,
>> + .list = list,
>> + .mode = list->mode,
>> + };
>> + struct sorted_bundle_list sorted = {
>> + .alloc = hashmap_get_size(&list->bundles),
>> + };
>> +
>> + ALLOC_ARRAY(sorted.items, sorted.alloc);
>> +
>> + for_all_bundles_in_list(list, insert_bundle, &sorted);
>> +
>> + QSORT(sorted.items, sorted.nr, compare_creation_token);
>
> So, at this point, 'sorted' is ordered by *decreasing* creation token? With
> the loop below being somewhat complex, it would be nice to have a comment
> mention that explicitly so readers have a clear understanding of the
> "initial state" before entering the loop.
That's a good point, but also in my local version I have the following line:
QSORT(bundles.items, bundles.nr, compare_creation_token_decreasing);
The comparison function was renamed based on Junio's feedback. After making
that change, this line is more self-documenting. Do you still think that it
needs a clarification comment if this rename occurs?
>> + /*
>> + * Use a stack-based approach to download the bundles and attempt
>> + * to unbundle them in decreasing order by creation token. If we
>> + * fail to unbundle (after a successful download) then move to the
>> + * next non-downloaded bundle (push to the stack) and attempt
>> + * downloading. Once we succeed in applying a bundle, move to the
>> + * previous unapplied bundle (pop the stack) and attempt to unbundle
>> + * it again.
>> + *
>> + * In the case of a fresh clone, we will likely download all of the
>> + * bundles before successfully unbundling the oldest one, then the
>> + * rest of the bundles unbundle successfully in increasing order
>> + * of creationToken.
>> + *
>> + * If there are existing objects, then this process may terminate
>> + * early when all required commits from "new" bundles exist in the
>> + * repo's object store.
>> + */
>> + cur = 0;
>> + while (cur >= 0 && cur < sorted.nr) {
>> + struct remote_bundle_info *bundle = sorted.items[cur];
>> + if (!bundle->file) {
>> + /* Not downloaded yet. Try downloading. */
>> + if (download_bundle_to_file(bundle, &ctx)) {
>> + /* Failure. Push to the stack. */
>> + pop_or_push = 1;
>> + goto stack_operation;
>
> Personally, I find the use of "stack" terminology more confusing than not.
> 'sorted' isn't really a stack, it's a list with fixed contents being
> traversed stepwise with 'cur'. For example, 'pop_or_push' being renamed to
> 'move_direction' or 'step' something along those lines might more clearly
> indicate what's actually happening with 'cur' & 'sorted'.
s/pop_or_push/move_direction/ makes a lot of sense.
I'll think about describing the strategy differently to avoid the "stack"
language. Mentally, I'm constructing a stack of "downloaded but unable to
unbundle bundles", but they aren't actually arranged that way in any
explicit structure. Instead, they are just the bundles in the list that
have a file but haven't been unbundled.
>> + }
>> +
>> + /* We expect bundles when using creationTokens. */
>> + if (!is_bundle(bundle->file, 1)) {
>> + warning(_("file downloaded from '%s' is not a bundle"),
>> + bundle->uri);
>> + break;
>> + }
>> + }
>> +
>> + if (bundle->file && !bundle->unbundled) {
>> + /*
>> + * This was downloaded, but not successfully
>> + * unbundled. Try unbundling again.
>> + */
>> + if (unbundle_from_file(ctx.r, bundle->file)) {
>> + /* Failed to unbundle. Push to stack. */
>> + pop_or_push = 1;
>> + } else {
>> + /* Succeeded in unbundle. Pop stack. */
>> + pop_or_push = -1;
>> + }
>> + }
>> +
>> + /*
>> + * Else case: downloaded and unbundled successfully.
>> + * Skip this by moving in the same direction as the
>> + * previous step.
>> + */
>> +
>> +stack_operation:
>> + /* Move in the specified direction and repeat. */
>> + cur += pop_or_push;
>> + }
>
> After reading through this loop, I generally understood *what* its doing,
> but didn't really follow *why* the download & unbundling is done like this.
The commit message should be updated to point to refer to the previously-
added test setup in t5558:
# To get interesting tests for bundle lists, we need to construct a
# somewhat-interesting commit history.
#
# ---------------- bundle-4
#
# 4
# / \
# ----|---|------- bundle-3
# | |
# | 3
# | |
# ----|---|------- bundle-2
# | |
# 2 |
# | |
# ----|---|------- bundle-1
# \ /
# 1
# |
# (previous commits)
And then this can be used to motivate the algorithm. Suppose we have
already downloaded commit 1 through a previous fetch. We try to download
bundle-4 first, but it can't apply because it requires commits that are
in bundle-3 _and_ bundle-2, but the client doesn't know which bundles
contain those commits. Downloading bundle-3 successfully unbundles, so a
naive algorithm would think we are "done" and expect to unbundle bundle-4.
However, that unbundling fails, so we go deeper into the list to download
bundle-2. That succeeds, and then retrying bundle-4 succeeds.
> I needed to refer back to the design doc
> ('Documentation/technical/bundle-uri.txt') to understand some basic
> assumptions about bundles:
>
> - A new bundle's creation token should always be strictly greater than the
> previous newest bundle's creation token. I don't see any special handling
> for equal creation tokens, so my assumption is that the sorting of the
> list arbitrarily assigns one to be "greater" and it's dealt with that way.
Yes, the bundle provider should not have equal values unless the bundles are
truly independent. That could be clarified in that doc.
> - The bundle with the lowest creation token should always be unbundleable,
> since it contains all objects in an initial clone.
Yes, at least it should not have any required commits.
> I do still have some questions, though:
>
> - Why would 'unbundle_from_file()' fail? From context clues, I'm guessing it
> fails if it has some unreachable objects (as in an incremental bundle), or
> if it's corrupted somehow.
You are correct. We assume that the data is well-formed and so the problem
must be due to required commits not already present in the local object store.
> - Why would 'download_bundle_to_file()' to fail? Unlike
> 'unbundle_from_file()', it looks like that represents an unexpected error.
Yes, that could fail for network issues such as a server error or other
network failure. In such cases, the client should expect that we will not
be able to download that bundle for the process's lifetime. We may be able
to opportunistically download other bundles, but we will rely on the Git
protocol to get the objects if the bundles fail.
These failure conditions are not tested deeply (there are some tests from
earlier series that test the behavior, but there is room for improvement).
> Also - it seems like one of the assumptions here is that, if a bundle can't
> be downloaded & unbundled, no bundle with a higher creation token can be
> successfully unbundled ('download_bundle_to_file()' sets 'pop_or_push' to
> '1', which will cause the loop to ignore all higher-token bundles and return
> a nonzero value from the function).
>
> I don't think that assumption is necessarily true, though. Suppose you have
> a "base" bundle 100 and incremental bundles 101 and 102. 101 has all objects
> from a new branch A, and 102 has all objects from a newer branch B (not
> based on any objects in A). In this case, 102 could be unbundled even if 101
> is corrupted/can't be downloaded, but we'd run into issues if we store 102
> as the "latest unbundled creation token" (because it implies that 101 was
> unbundled).
You are correct. bundle-3 can be unbundled even if bundle-2 fails in the
test example above.
> Is there any benefit to trying to unbundle those higher bundles *without*
> advancing the "latest creation token"? E.g. in my example, unbundle 102 but
> store '100' as the latest creation token?
I will need to think more about this.
Generally, most repositories that care about this will not have independent
bundles because between every bundle creation step the default branch will
advance. (Of course, exceptions can still occur, such as over weekends.)
Thus, the latest bundle will have a required commit that only exists in the
previous bundle. This algorithm and its error conditions are then looking
for ways to recover when that is not the case.
When a bundle fails to download, my gut feeling is that it is unlikely that
it was completely independent of a bundle with higher creationToken. However,
we have already downloaded that bundle and it is a very low cost to attempt
an unbundling of it.
The tricky part is that we want to avoid downloading _all_ the bundles just
because one is failing to unbundle. If a failed download would cause the top
bundle from unbundling, we don't want to go through the whole list of bundles
even though they unbundle without issue. I'm thinking specifically about the
incremental fetch case, where we don't want to blow up to a full clone worth
of downloads.
This deserves a little more attention, so I'll think more on it and get
back to you.
>> git -C clone-from for-each-ref --format="%(objectname)" >oids &&
>> - git -C clone-list-http-2 cat-file --batch-check <oids
>> + git -C clone-list-http-2 cat-file --batch-check <oids &&
>> +
>> + for b in 1 2 3 4
>> + do
>> + test_bundle_downloaded bundle-$b.bundle trace-clone.txt ||
>> + return 1
>> + done
>
> If I understand correctly, these added conditions would have passed even if
> they were added when the test was initially created in patch 1, but they're
> added here to tie them to the implementation of the creationToken heuristic?
> Seems reasonable.
They probably should have been added in patch 1 to be clear that behavior
is not changing here.
>> +'
>> +
>> +test_expect_success 'clone bundle list (http, creationToken)' '
>
> This new test has the same name as the one above it - how does it differ
> from that one? Whatever the difference is, can that be noted somehow in the
> title or a comment?
The title should change, pointing out that the bundle list is truncated
and the rest of the clone is being fetched over the Git protocol. It will
be expanded with fetches later, I think, but it should be better motivated
in this patch, even if that is so.
>> +# Usage: test_bundle_downloaded <bundle-id> <trace-filename>
>> +test_bundle_downloaded () {
>> + cat >pattern <<-EOF &&
>> + "event":"child_start".*"argv":\["git-remote-https","$HTTPD_URL/$1.bundle"\]
>> + EOF
>> + grep -f pattern "$2"
>> +}
>
> This function is the same as the one created in 't5558'. Should it be moved
> to 'lib-bundle.sh' or 'test-lib.sh' to avoid duplicate code?
It's slightly different, but that is just because we are using the advertisement
and thus we never download a bundle-list and always download .bundle files. That
is not an important distinction and I expect to replace it with the
test_remote_https_urls() helper discussed in an earlier response.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees
From: Phillip Wood @ 2023-01-20 15:08 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón, git
Cc: pclouds, gitster, Jinwook Jeong, Eric Sunshine, Rubén Justo,
Phillip Wood
In-Reply-To: <20230120113553.24655-1-carenas@gmail.com>
Hi Carlo
On 20/01/2023 11:35, Carlo Marcelo Arenas Belón wrote:
> Commands `git switch -C` and `git checkout -B` neglect to check whether
> the provided branch is already checked out in some other worktree, as
> shown by the following:
>
> $ git worktree list
> .../foo beefb00f [main]
> $ git worktree add ../other
> Preparing worktree (new branch 'other')
> HEAD is now at beefb00f first
> $ cd ../other
> $ git switch -C main
> Switched to and reset branch 'main'
> $ git worktree list
> .../foo beefb00f [main]
> .../other beefb00f [main]
>
> Fix this problem by teaching `git switch -C` and `git checkout -B` to
> check whether the branch in question is already checked out elsewhere.
>
> Unlike what it is done for `git switch` and `git checkout`, that have
> an historical exception to ignore other worktrees if the branch to
> check is the current one (as required when called as part of other
> tools), the logic implemented is more strict and will require the user
> to invoke the command with `--ignore-other-worktrees` to explicitly
> indicate they want the risky behaviour.
>
> This matches the current behaviour of `git branch -f` and is safer; for
> more details see the tests in t2400.
As I said before, it would be much easier for everyone else to
understand the changes if you wrote out what they were rather than
saying "look at the tests". I do appreciate the improved test
descriptions though - thanks for that.
> Reported-by: Jinwook Jeong <vustthat@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Rubén Justo <rjusto@gmail.com>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> Changes since v3
> * Code and Tests improvements as suggested by Phillip
> * Disable unreliable test that triggers a known bug
>
> Changes since v2
> * A leak free implementation
> * More details in commit as suggested by Junio
>
> Changes since v1
> * A much better commit message
> * Changes to the tests as suggested by Eric
> * Changes to the logic as suggested by Rubén
>
>
> builtin/checkout.c | 32 ++++++++++++++++++++++++--------
> t/t2400-worktree-add.sh | 34 +++++++++++++++++++++++++++-------
> 2 files changed, 51 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 3fa29a08ee..0688652f99 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)
> }
>
> static int checkout_branch(struct checkout_opts *opts,
> - struct branch_info *new_branch_info)
> + struct branch_info *new_branch_info,
> + char *check_branch_path)
> {
> if (opts->pathspec.nr)
> die(_("paths cannot be used with switching branches"));
> @@ -1533,13 +1534,13 @@ static int checkout_branch(struct checkout_opts *opts,
> if (!opts->can_switch_when_in_progress)
> die_if_some_operation_in_progress();
>
> - if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
> - !opts->ignore_other_worktrees) {
> + if (!opts->ignore_other_worktrees && !opts->force_detach &&
> + check_branch_path && ref_exists(check_branch_path)) {
I think check_branch_path is NULL if opts->ignore_other_worktrees is set
so we could maybe lose "!opts->ignore_other_worktrees" here (or possibly
below where you set check_branch_path).
> int flag;
> char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag);
> - if (head_ref &&
> - (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))
> - die_if_checked_out(new_branch_info->path, 1);
> + if (opts->new_branch_force || (head_ref &&
> + (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_path))))
> + die_if_checked_out(check_branch_path, 1);
I don't think it is worth a re-roll but the reformatting here is
unfortunate. If you add the new condition at the end it is clearer what
is being changed.
if ((head_ref &&
(!(flag & REF_IS_YMREF) || strcmp(head_ref, check_branch_path))) ||
opts->new_branch_force)
preserves the original code structure so one can see we've added a new
condition and done s/new_branch_info->path/check_branch_path/
> free(head_ref);
> }
>
> @@ -1627,7 +1628,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
> const char * const usagestr[],
> struct branch_info *new_branch_info)
> {
> + int ret;
> int parseopt_flags = 0;
> + char *check_branch_path = NULL;
>
> opts->overwrite_ignore = 1;
> opts->prefix = prefix;
> @@ -1717,6 +1720,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
> opts->new_branch = argv0 + 1;
> }
>
> + if (opts->new_branch && !opts->ignore_other_worktrees) {
> + struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_branchname(&buf, opts->new_branch, INTERPRET_BRANCH_LOCAL);
> + strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
> + check_branch_path = strbuf_detach(&buf, NULL);
> + }
This block will run whenever -b/-B is given which is good
> /*
> * Extract branch name from command line arguments, so
> * all that is left is pathspecs.
> @@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
> new_branch_info, opts, &rev);
> argv += n;
> argc -= n;
> +
> + if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
> + check_branch_path = xstrdup(new_branch_info->path);
I'm a bit confused what this is doing.
> } else if (!opts->accept_ref && opts->from_treeish) {
> struct object_id rev;
>
> @@ -1817,9 +1830,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
> }
>
> if (opts->patch_mode || opts->pathspec.nr)
> - return checkout_paths(opts, new_branch_info);
> + ret = checkout_paths(opts, new_branch_info);
> else
> - return checkout_branch(opts, new_branch_info);
> + ret = checkout_branch(opts, new_branch_info, check_branch_path);
> +
> + free(check_branch_path);
> + return ret;
This is clearer now - thanks
> }
>
> int cmd_checkout(int argc, const char **argv, const char *prefix)
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index d587e0b20d..7ab7e87440 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -118,14 +118,17 @@ test_expect_success '"add" worktree creating new branch' '
> )
> '
>
> -test_expect_success 'die the same branch is already checked out' '
> +test_expect_success 'die if the same branch is already checked out' '
> (
> cd here &&
> - test_must_fail git checkout newmain
> + test_must_fail git checkout newmain &&
> + test_must_fail git checkout -B newmain &&
> + test_must_fail git switch newmain &&
> + test_must_fail git switch -C newmain
> )
> '
>
> -test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '
> +test_expect_success SYMLINKS 'die if the same branch is already checked out (symlink)' '
> head=$(git -C there rev-parse --git-path HEAD) &&
> ref=$(git -C there symbolic-ref HEAD) &&
> rm "$head" &&
> @@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin
> test_must_fail git -C here checkout newmain
> '
>
> -test_expect_success 'not die the same branch is already checked out' '
> +test_expect_success 'allow creating multiple worktrees for same branch with force' '
> + git worktree add --force anothernewmain newmain
> +'
> +
> +test_expect_success 'allow checkout/reset from the conflicted branch' '
I'm not sure what "the conflicted branch" means (it reminds we of merge
conflicts). Is this just testing that "checkout -b/B <branch>
<start-point>" works?
> (
> cd here &&
> - git worktree add --force anothernewmain newmain
> + git checkout -b conflictedmain newmain &&
> + git checkout -B conflictedmain newmain &&
> + git switch -C conflictedmain newmain
> + )
> +'
> +
> +test_expect_success 'and not die on re-checking out current branch even if conflicted' '
I think 'allow re-checking out ...' would be clearer, again I'm not sure
what's conflicted here.
> + (
> + cd there &&
> + git checkout newmain &&
> + git switch newmain
> )
> '
>
> -test_expect_success 'not die on re-checking out current branch' '
> +test_expect_failure 'unless using force without --ignore-other-worktrees' '
This test passes for me - what's the reason for changing from
test_expect_success to test_expect_failure?
Thanks for working on this
Phillip
> (
> cd there &&
> - git checkout newmain
> + test_must_fail git checkout -B newmain &&
> + test_must_fail git switch -C newmain &&
> + git checkout --ignore-other-worktrees -B newmain &&
> + git switch --ignore-other-worktrees -C newmain
> )
> '
>
^ permalink raw reply
* Re: [CI]: Is t7527 known to be flakey?
From: Jeff Hostetler @ 2023-01-20 15:23 UTC (permalink / raw)
To: Junio C Hamano, Jeff Hostetler, edecosta; +Cc: git
In-Reply-To: <xmqqtu0lzzn2.fsf@gitster.g>
On 1/19/23 9:52 PM, Junio C Hamano wrote:
> The said test failed its linux-musl job in its first attempt, but
> re-running the failed job passed.
>
> https://github.com/git/git/actions/runs/3963948890/jobs/6792356234
> (seen@e096683 attempt #1 linux-musl)
>
> https://github.com/git/git/actions/runs/3963948890/jobs/6792850313
> (seen@e096683 attempt #2 linux-musl)
>
This is on Linux, so it would be using the linux inotify backend.
Let me add Eric to the "To:" line for visibility. And see if he
has experienced this during his development of it.
I've not looked at the inotify code so I can't say if there are
races there or not. Tests that move directories feel like good
candidates for race conditions -- since the daemon doesn't get a
recursive view of the tree with inotify() and must simulate that
and manage the individual directories, but again I don't want to
assume that.
Jeff
^ permalink raw reply
* Re: [PATCH] rebase: mark --update-refs as requiring the merge backend
From: Junio C Hamano @ 2023-01-20 15:27 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Elijah Newren via GitGitGadget, git, Elijah Newren
In-Reply-To: <f480813c-7583-179f-0149-d970d3f2519f@github.com>
Derrick Stolee <derrickstolee@github.com> writes:
>> + if (options.update_refs)
>> + imply_merge(&options, "--update-refs");
>> +
>
> This solution is very elegant. The only downside is the lack of warning
> if --update-refs was implied by rebase.updateRefs=true, but I'm happy to
> delay implementing that warning in favor of your complete solution here.
If features A and B are incompatible and both can be specified from
both command line and configuration, ideally I would expect the
system to operate in one of two ways. I haven't thought it through
to decide which one I prefer between the two.
* Take "command line trumps configuration" one step further, so
that A that is configured but not asked for from the command
line is defeated by B that is asked for from the command line.
This way, only when A and B are both requested via the
configuration, of via the command line, we'd fail the operation
by saying A and B are incompatible. Otherwise, the one that is
configured but overridden is turned off (either silently or with
a warning).
* Declare "command line trumps configuration" is only among the
same feature. Regardless of how features A and B that are
incompatible are requested, the command will error out, citing
incompatibility. It would be very nice if the warning mentioned
where the requests for features A and B came from (e.g. "You
asked for -B from the command line, but you have A configured,
and both cannot be active at the same time---disable A from the
command line, or do not ask for B")
When A is configured and B is requested from the command line,
the command will error out, and the user must defeat A from the
command line before the user can use B, e.g. "git cmd --no-A -B".
A knee-jerk reaction to the situation is that the latter feels
somewhat safer than the former, but when I imagine the actual end
user who saw the error message, especially the suggested solution
"disable A from the command line or do not ask for B from the
command line", may say "well, I asked for B for this invocation
explicitly with -B from the command line, and you(Git) should be
able to make it imply --no-A", which amounts to the same thing as
the former choice.
^ permalink raw reply
* Re: [PATCH v2 1/2] rebase: remove completely useless -C option
From: Elijah Newren @ 2023-01-20 15:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren via GitGitGadget, git, Derrick Stolee
In-Reply-To: <xmqq5yd1za0t.fsf@gitster.g>
On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > The `-C` option to rebase was introduced with 67dad687ad ("add -C[NUM]
> > to git-am", 2007-02-08).
> > ...
> > As per the git-apply documentation for the `-C` option:
> > Ensure at least <n> lines of surrounding context match...When fewer
> > lines of surrounding context exist they all must match.
> >
> > The fact that format-patch was not passed a -U option to increase the
> > number of context lines meant that there would still only be 3 lines of
> > context to match on.
>
> I am afraid that this is only less than half true. Isn't the
> intended use of -C<num> similar to how "patch --fuzz" is used?
>
> That is, even when a patch does not cleanly apply with full context
> in the incoming diff, by requiring *smaller* number of lines to
> match, the diff *could* be forced to apply with reduced precision?
Oh! Reducing the number of lines of context to pay attention to never
even occurred to me for whatever reason. I'll drop the patch.
^ permalink raw reply
* Re: [PATCH v2 1/2] rebase: remove completely useless -C option
From: Elijah Newren @ 2023-01-20 15:32 UTC (permalink / raw)
To: Martin Ågren; +Cc: Elijah Newren via GitGitGadget, git, Derrick Stolee
In-Reply-To: <CAN0heSo8cqNqhx9+AJZJS2rwJxBG_HZjhgxQZf8nw5t82NDXBg@mail.gmail.com>
On Fri, Jan 20, 2023 at 1:55 AM Martin Ågren <martin.agren@gmail.com> wrote:
>
> On Fri, 20 Jan 2023 at 06:27, Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > As per the git-apply documentation for the `-C` option:
> > Ensure at least <n> lines of surrounding context match...When fewer
> > lines of surrounding context exist they all must match.
> >
> > The fact that format-patch was not passed a -U option to increase the
> > number of context lines meant that there would still only be 3 lines of
> > context to match on. So, anyone attempting to use this option in
> > git-rebase would just be spinning their wheels and wasting time. I was
> > one such user a number of years ago.
>
> I suppose someone might have something like GIT_DIFF_OPTS="--unified=20"
> meaning they would actually have more context for their `-C` to act on.
> So I guess there is a chance that someone somewhere has actually been
> able to make use of `git rebase -C` after all? I'm not really arguing
> either way -- just noting the possibility, as remote as it may be.
>
> Martin
Ah, good point. And combined with Junio's point that -C is apparently
about reducing rather than increasing context, I should just drop the
patch.
^ permalink raw reply
* Re: [CI]: Is t7527 known to be flakey?
From: Junio C Hamano @ 2023-01-20 15:40 UTC (permalink / raw)
To: Jeff Hostetler; +Cc: Jeff Hostetler, edecosta, git
In-Reply-To: <d5e2cd60-49f7-91bf-0678-70e3671b1cad@jeffhostetler.com>
Jeff Hostetler <git@jeffhostetler.com> writes:
> This is on Linux, so it would be using the linux inotify backend.
> Let me add Eric to the "To:" line for visibility.
Thanks for redirection.
^ permalink raw reply
* Re: [PATCH 5/8] clone: set fetch.bundleURI if appropriate
From: Derrick Stolee @ 2023-01-20 15:42 UTC (permalink / raw)
To: Victoria Dye, Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen
In-Reply-To: <61a830f0-a8d8-5a8a-e952-db213d571352@github.com>
On 1/19/2023 2:42 PM, Victoria Dye wrote:
> Derrick Stolee via GitGitGadget wrote:
>> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
>> index cd65d236b43..4f796218aab 100644
>> --- a/Documentation/config/fetch.txt
>> +++ b/Documentation/config/fetch.txt
>> @@ -96,3 +96,11 @@ fetch.writeCommitGraph::
>> merge and the write may take longer. Having an updated commit-graph
>> file helps performance of many Git commands, including `git merge-base`,
>> `git push -f`, and `git log --graph`. Defaults to false.
>> +
>> +fetch.bundleURI::
>> + This value stores a URI for fetching Git object data from a bundle URI
>> + before performing an incremental fetch from the origin Git server. If
>> + the value is `<uri>` then running `git fetch <args>` is equivalent to
>> + first running `git fetch --bundle-uri=<uri>` immediately before
>> + `git fetch <args>`. See details of the `--bundle-uri` option in
>> + linkgit:git-fetch[1].
>
> Since it's not mentioned from this or any other user-facing documentation
> (AFAICT), could you note that this value is set automatically by 'git clone'
> iff '--bundle-uri' is specified *and* 'bundle.heuristic' is set for the
> initially downloaded bundle list?
Can do.
> It would also be nice to make note of that behavior in the documentation of
> the '--bundle-uri' option in 'Documentation/git-clone.txt', since command
> documentation in general seems to be more popular/visible to users than
> config docs.
Yes. I also thought that I had updated this documentation to not refer
to 'git fetch --bundle-uri', which doesn't exist anymore since an earlier
RFC version. I'll be sure to update that, too.
>> strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD");
>> diff --git a/bundle-uri.c b/bundle-uri.c
>> index b30c85ba6f2..1dbbbb980eb 100644
>> --- a/bundle-uri.c
>> +++ b/bundle-uri.c
>> @@ -594,9 +594,10 @@ static int fetch_bundle_list_in_config_format(struct repository *r,
>> * it advertises are expected to be bundles, not nested lists.
>> * We can drop 'global_list' and 'depth'.
>> */
>> - if (list_from_bundle.heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN)
>> + if (list_from_bundle.heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN) {
>> result = fetch_bundles_by_token(r, &list_from_bundle);
>> - else if ((result = download_bundle_list(r, &list_from_bundle,
>> + global_list->heuristic = BUNDLE_HEURISTIC_CREATIONTOKEN;
>
> If the 'heuristic' field already existed and was being used to apply
> bundles, why wasn't 'global_list->heuristic' already being set? Before this
> patch, was the 'global_list->heuristic' field not accurately reflecting the
> heuristic type of a given bundle list?
>
> If so, I think it'd make sense to move this section to patch 4 [1], since
> that's when the heuristic is first applied to the bundle list.
>
> [1] https://lore.kernel.org/git/57c0174d3752fb61a05e0653de9d3057616ed16a.1673037405.git.gitgitgadget@gmail.com/
Can do.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH 7/8] fetch: fetch from an external bundle URI
From: Derrick Stolee @ 2023-01-20 15:47 UTC (permalink / raw)
To: Victoria Dye, Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen
In-Reply-To: <58269fe6-ebe1-7b12-4cd9-2110a94543e5@github.com>
On 1/19/2023 3:34 PM, Victoria Dye wrote:
> Derrick Stolee via GitGitGadget wrote:
>> + if (!git_config_get_string_tmp("fetch.bundleuri", &bundle_uri) &&
>> + !starts_with(bundle_uri, "remote:")) {
>
> Maybe a silly question, by why would the bundle URI start with 'remote:'
> (and why do we silently skip fetching from the URI in that case)?
Thanks for catching this. I originally was going to include fetching from
lists advertised by a Git remote, and use the same `fetch.bundleURI` config.
However, it makes more sense to make a `remote.<name>.bundles` config
instead, so I dropped that functionality from this series. I forgot to remove
this `remote:` case, but will do so in v2.
I've locally fixed the "objects" typo you pointed out, too.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2] fsm-listen-darwin: combine bit operations
From: Jeff Hostetler @ 2023-01-20 15:48 UTC (permalink / raw)
To: Rose via GitGitGadget, git; +Cc: Rose, Seija Kijin
In-Reply-To: <pull.1437.v2.git.git.1673992448371.gitgitgadget@gmail.com>
On 1/17/23 4:54 PM, Rose via GitGitGadget wrote:
> From: Seija Kijin <doremylover123@gmail.com>
>
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
> fsm-listen-darwin: combine bit operations
>
> Signed-off-by: Seija Kijin doremylover123@gmail.com
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1437%2FAtariDreams%2Fdarwin-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1437/AtariDreams/darwin-v2
> Pull-Request: https://github.com/git/git/pull/1437
>
> Range-diff vs v1:
>
> 1: a98654c7507 ! 1: 9943d52654f fsm-listen-daarwin: combine bit operations
> @@ Metadata
> Author: Seija Kijin <doremylover123@gmail.com>
>
> ## Commit message ##
> - fsm-listen-daarwin: combine bit operations
> + fsm-listen-darwin: combine bit operations
>
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
>
>
>
> compat/fsmonitor/fsm-listen-darwin.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c
> index 97a55a6f0a4..fccdd21d858 100644
> --- a/compat/fsmonitor/fsm-listen-darwin.c
> +++ b/compat/fsmonitor/fsm-listen-darwin.c
> @@ -129,9 +129,9 @@ static int ef_is_root_renamed(const FSEventStreamEventFlags ef)
>
> static int ef_is_dropped(const FSEventStreamEventFlags ef)
> {
> - return (ef & kFSEventStreamEventFlagMustScanSubDirs ||
> - ef & kFSEventStreamEventFlagKernelDropped ||
> - ef & kFSEventStreamEventFlagUserDropped);
> + return (ef & (kFSEventStreamEventFlagMustScanSubDirs |
> + kFSEventStreamEventFlagKernelDropped |
> + kFSEventStreamEventFlagUserDropped));
> }
Technically, the returned value is slightly different, but
the only caller is just checking for non-zero, so it doesn't
matter.
So this is fine.
Thanks,
Jeff
^ permalink raw reply
* Re: [PATCH 8/8] bundle-uri: store fetch.bundleCreationToken
From: Derrick Stolee @ 2023-01-20 15:53 UTC (permalink / raw)
To: Victoria Dye, Derrick Stolee via GitGitGadget, git
Cc: gitster, me, avarab, steadmon, chooglen
In-Reply-To: <f7a777f0-3f37-86a1-f520-1ab6853b9ed1@github.com>
On 1/19/2023 5:24 PM, Victoria Dye wrote:
> Derrick Stolee via GitGitGadget wrote:
>> + /*
>> + * If fetch.bundleCreationToken exists, parses to a uint64t, and
>> + * is not strictly smaller than the maximum creation token in the
>> + * bundle list, then do not download any bundles.
>> + */
>> + if (!repo_config_get_value(r,
>> + "fetch.bundlecreationtoken",
>> + &creationTokenStr) &&
>> + sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) == 1 &&
>> + sorted.items[0]->creationToken <= maxCreationToken) {
>> + free(sorted.items);
>> + return 0;
>> + }
>
> And here, we exit if the cached creation token is greater than or equal to
> the highest advertised token. Overall, this seems pretty safe:
>
> - If the value is (somehow) manually updated to something larger than it
> should be, objects will be fetched from the server that could have
> otherwise come from a bundle. Not ideal, but no worse than a regular
> fetch.
> - If the value is too small or invalid (i.e., not an unsigned integer),
> we'll download the first bundle, unbundle it, then overwrite the invalid
> 'fetch.bundlecreationtoken' with a new valid one.
>
> The latter is self-correcting, but should the former be documented
> somewhere? For example, if someone changes which bundle server they're using
> with a repo, the creation token numbering scheme might be completely
> different. In that case, a user would probably want to "reset" the
> 'fetch.bundlecreationtoken' and start fresh with a new bundle list (even if
> the recommended method is "manually delete the config key from your repo").
I can update the config documentation to include this.
>> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
>> index 3f4d61a915c..0604d721f1b 100755
>> --- a/t/t5558-clone-bundle-uri.sh
>> +++ b/t/t5558-clone-bundle-uri.sh
>
> It isn't exactly related to this patch, but it looks like the second "clone
> bundle list (http, creationToken)" never received any updates to
> differentiate it from the first test with that name (I noted the duplicate
> name in patch 4 [2]). Is it just a leftover duplicate?
>
> [1] https://lore.kernel.org/git/ede340d1-bce4-0c1d-7afb-4874a67d1803@github.com/
I'll be sure to follow up and make these test changes be of higher value,
avoiding these confusions.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2 1/2] rebase: remove completely useless -C option
From: Junio C Hamano @ 2023-01-20 16:15 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, git, Derrick Stolee
In-Reply-To: <CABPp-BFakaEqnpW4Xn1rzcOC6oVmcEz+OxBV4dKA_TJZ-xbTvg@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>> That is, even when a patch does not cleanly apply with full context
>> in the incoming diff, by requiring *smaller* number of lines to
>> match, the diff *could* be forced to apply with reduced precision?
>
> Oh! Reducing the number of lines of context to pay attention to never
> even occurred to me for whatever reason. I'll drop the patch.
Yup. "completely useless" on the title is less than half correct,
but still correct for a minor use case where -C is used to use
*more* context lines than the default.
We could update "rebase --apply" codepath to increase the context
lines generated by format-patch. That would make the "completely
useless" totally incorrect ;-)
^ permalink raw reply
* Re: [PATCH v2 2/2] ls-files: add %(skipworktree) atom to format option
From: Junio C Hamano @ 2023-01-20 16:34 UTC (permalink / raw)
To: Elijah Newren
Cc: ZheNing Hu via GitGitGadget, git,
Ævar Arnfjörð Bjarmason, Derrick Stolee,
Victoria Dye, Nguyễn Thái Ngọc Duy, ZheNing Hu
In-Reply-To: <CABPp-BGLmhoHAcuLoz_yQ4TmNBvDU6Ehymy_3rh0wguSw0hjGw@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> On Thu, Jan 19, 2023 at 9:34 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: ZheNing Hu <adlternative@gmail.com>
>>
>> Because sometimes we want to check if the files in the
>> index match the sparse specification, so introduce
>> "%(skipworktree)" atom to git ls-files `--format` option.
>> When we use this option, if the file match the sparse
>> specification, it will output "1", otherwise, output
>> empty string "".
>
> Why is that output format useful? It seems like it'll just lead to
> bugs, or someone re-implementing the same field with a different name
> to make it useful in the future. In particular, if there are multiple
> boolean fields and someone specifies e.g.
> git ls-files --format="%(path) %(skipworktree) %(intentToAdd)"
> and both boolean fields are displayed the same way (either a "1" or a
> blank string), and we see something like:
> foo.c 1
> bar.c 1
> Then how do we know if foo.c and bar.c are SKIP_WORKTREE or
> INTENT_TO_ADD? The "1" could have come from either field.
Perhaps it becomes useful in conjunction with %(if) and friends,
when they become avaiable?
Until then, I agree that the output format looks pretty klunky.
The calling scripts still can do
--format='%(path) A=%(A) B=%(B) C=%(C)'
and take an empty value as false, though.
^ permalink raw reply
* Re: [PATCH] hooks: add sendemail-validate-series
From: Robin Jarry @ 2023-01-20 16:34 UTC (permalink / raw)
To: git
In-Reply-To: <20230103231133.64050-1-robin@jarry.cc>
Hi all,
This is a gentle bump to see if that feature could be of interest for
anyone.
Thanks in advance.
^ permalink raw reply
* Re: [PATCH v2 2/2] rebase: mark --update-refs as requiring the merge backend
From: Phillip Wood @ 2023-01-20 16:46 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget, git
Cc: Derrick Stolee, Elijah Newren, Junio C Hamano, Philippe Blain
In-Reply-To: <2e44d0b7e571cfac2a25d00f3fe3d143c895793b.1674190573.git.gitgitgadget@gmail.com>
Hi Elijah
Thanks for working on this
On 20/01/2023 04:56, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> --update-refs is built in terms of the sequencer, which requires the
> merge backend. It was already marked as incompatible with the apply
> backend in the git-rebase manual, but the code didn't check for this
> incompatibility and warn the user. Check and warn now.
Strictly speaking we die rather than warn but I don't think that
warrants a re-roll. I just had a quick look to see how easy it would be
to add the advice Stolee's patch had if the user has set
rebase.updaterefs but does not pass "--no-update-refs" when using the
apply backend but it looks a bit fiddly unfortunately as we could die in
imply_merge() or later on.
Thinking more generally, imply_merge() does a good job of telling the
user which option is incompatible with "--apply" but if the user passes
a merge option with "--whitespace=fix" and omits "--apply" then we just
print a generic message saying "apply options and merge options cannot
be used together" which isn't terribly helpful to the user (doubly so
when the merge option come from a config setting).
I've also noticed that "--autosquash" is ignored if we end up using the
apply backend. That's a separate issue but shares the "this may have
come from a config setting rather than a command line argument" problem.
All in all I'm not sure if it is friendlier to die when the user has
rebsase.updaterefs set and they try to rebase with "--whitespace=fix" or
if it is better just to ignore the config in that case. If we can find a
way to print some help when we die in that case it would be nicer for
the user.
Best Wishes
Phillip
> While at it, fix a typo in t3422...and fix some misleading wording (all
> useful options other than --whitespace=fix have long since been
> implemented in the merge backend).
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> builtin/rebase.c | 3 +++
> t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index ace8bd4a41c..e8bcdbf9fcd 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1507,6 +1507,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> }
> }
>
> + if (options.update_refs)
> + imply_merge(&options, "--update-refs");
> +
> if (options.type == REBASE_UNSPECIFIED) {
> if (!strcmp(options.default_backend, "merge"))
> imply_merge(&options, "--merge");
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index ebcbd79ab8a..d72c863b21b 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -25,11 +25,11 @@ test_expect_success 'setup' '
> '
>
> #
> -# Rebase has lots of useful options like --whitepsace=fix, which are
> -# actually all built in terms of flags to git-am. Since neither
> -# --merge nor --interactive (nor any options that imply those two) use
> -# git-am, using them together will result in flags like --whitespace=fix
> -# being ignored. Make sure rebase warns the user and aborts instead.
> +# Rebase has a useful option, --whitespace=fix, which is actually
> +# built in terms of flags to git-am. Since neither --merge nor
> +# --interactive (nor any options that imply those two) use git-am,
> +# using them together will result in --whitespace=fix being ignored.
> +# Make sure rebase warns the user and aborts instead.
> #
>
> test_rebase_am_only () {
> @@ -60,6 +60,11 @@ test_rebase_am_only () {
> test_must_fail git rebase $opt --exec 'true' A
> "
>
> + test_expect_success "$opt incompatible with --update-refs" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --update-refs A
> + "
> +
> }
>
> test_rebase_am_only --whitespace=fix
^ permalink raw reply
* Re: [PATCH] rebase: mark --update-refs as requiring the merge backend
From: Elijah Newren @ 2023-01-20 16:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Derrick Stolee, Elijah Newren via GitGitGadget, git
In-Reply-To: <xmqqr0vpxm3d.fsf@gitster.g>
On Fri, Jan 20, 2023 at 7:27 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Derrick Stolee <derrickstolee@github.com> writes:
>
> >> + if (options.update_refs)
> >> + imply_merge(&options, "--update-refs");
> >> +
> >
> > This solution is very elegant. The only downside is the lack of warning
> > if --update-refs was implied by rebase.updateRefs=true, but I'm happy to
> > delay implementing that warning in favor of your complete solution here.
>
> If features A and B are incompatible and both can be specified from
> both command line and configuration, ideally I would expect the
> system to operate in one of two ways.
I agree that one of the two ways you highlight below would be ideal.
Should this series be held up on extending the checks to implement
this ideal, or are you just commenting for whoever later circles back
to implement this?
> I haven't thought it through
> to decide which one I prefer between the two.
>
> * Take "command line trumps configuration" one step further, so
> that A that is configured but not asked for from the command
> line is defeated by B that is asked for from the command line.
>
> This way, only when A and B are both requested via the
> configuration, of via the command line, we'd fail the operation
> by saying A and B are incompatible. Otherwise, the one that is
> configured but overridden is turned off (either silently or with
> a warning).
>
> * Declare "command line trumps configuration" is only among the
> same feature. Regardless of how features A and B that are
> incompatible are requested, the command will error out, citing
> incompatibility. It would be very nice if the warning mentioned
> where the requests for features A and B came from (e.g. "You
> asked for -B from the command line, but you have A configured,
> and both cannot be active at the same time---disable A from the
> command line, or do not ask for B")
>
> When A is configured and B is requested from the command line,
> the command will error out, and the user must defeat A from the
> command line before the user can use B, e.g. "git cmd --no-A -B".
>
> A knee-jerk reaction to the situation is that the latter feels
> somewhat safer than the former, but when I imagine the actual end
> user who saw the error message, especially the suggested solution
> "disable A from the command line or do not ask for B from the
> command line", may say "well, I asked for B for this invocation
> explicitly with -B from the command line, and you(Git) should be
> able to make it imply --no-A", which amounts to the same thing as
> the former choice.
If it is clear to the user that A and B preclude each other, then I
agree with this sentiment that the former choice (silently ignoring
the config) would avoid a minor frustration for some users and thus
would be better. But I don't think that's applicable here. There is
no reason that --whitespace=fix shouldn't be available from the merge
backend other than that we haven't implemented it yet, and it's likely
feasible to implement --update-refs for the apply backend with enough
effort if we thought it was worth it. So, if a user sets
rebase.updateRefs=true in their config because they always want
included branches updated, but one time they run `git rebase
--whitespace=fix`, they will likely have a negative experience like
the one that inspired this patch. Perhaps we're forced to choose
between possible frustration by different end users, but if so, I
think trying to debug and figure out "Wait, I switched to this branch
and started tweaking it but it appears to not have some relevant
changes I'm sure I made to it yesterday. What happened?" is a much
worse frustration than "I have to manually specify --no-A in this
special case". So, when it's not at all obvious that A and B preclude
each other, I think we're better off giving the error.
^ 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