* [PATCH 1/4] completion: complete missing rev-list options
From: Philippe Blain via GitGitGadget @ 2024-01-21 4:07 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1650.git.git.1705810071.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
Some options listed in rev-list-options.txt, and thus accepted by 'git
log' and friends, are missing from the Bash completion script.
Add them to __git_log_common_options.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8c40ade4941..6108d523a11 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2071,6 +2071,16 @@ __git_log_common_options="
--min-age= --until= --before=
--min-parents= --max-parents=
--no-min-parents --no-max-parents
+ --alternate-refs --ancestry-path
+ --author-date-order --basic-regexp
+ --bisect --boundary --exclude-first-parent-only
+ --exclude-hidden --extended-regexp
+ --fixed-strings --grep-reflog
+ --ignore-missing --left-only --perl-regexp
+ --reflog --regexp-ignore-case --remove-empty
+ --right-only --show-linear-break
+ --show-notes-by-default --show-pulls
+ --since-as-filter --single-worktree
"
# Options that go well for log and gitk (not shortlog)
__git_log_gitk_options="
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/4] completion: add missing 'git log' options
From: Philippe Blain via GitGitGadget @ 2024-01-21 4:07 UTC (permalink / raw)
To: git; +Cc: Philippe Blain
I noticed a few 'git log' options (old and newish) were not suggested by the
completion script, so I went through the whole list and added those that
were missing.
Philippe Blain (4):
completion: complete missing rev-list options
completion: complete --patch-with-raw
completion: complete --encoding
completion: complete missing 'git log' options
contrib/completion/git-completion.bash | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1650%2Fphil-blain%2Fcompletion-log-options-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1650/phil-blain/completion-log-options-v1
Pull-Request: https://github.com/git/git/pull/1650
--
gitgitgadget
^ permalink raw reply
* [PATCH] ci(github): also skip logs of broken test cases
From: Philippe Blain via GitGitGadget @ 2024-01-21 3:38 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Victoria Dye, Philippe Blain, Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
When a test fails in the GitHub Actions CI pipeline, we mark it up using
special GitHub syntax so it stands out when looking at the run log. We
also mark up "fixed" test cases, and skip passing tests since we want to
concentrate on the failures.
The finalize_test_case_output function in
test-lib-github-workflow-markup.sh which performs this markup is however
missing a fourth case: "broken" tests, i.e. tests using
'test_expect_failure' to document a known bug. This leads to these
"broken" tests appearing along with any failed tests, potentially
confusing the reader who might not be aware that "broken" is the status
for 'test_expect_failure' tests that indeed failed, and wondering what
their commits "broke".
Also skip these "broken" tests so that only failures and fixed tests
stand out.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
ci(github): also skip logs of broken test cases
* An example of a run with failed tests appearing along with several
"broken" tests:
https://github.com/phil-blain/git/actions/runs/7589303055/job/20673657755
* An example of a run with the same failures, but with this patch on
top (no "broken" tests listed):
* https://github.com/phil-blain/git/actions/runs/7598605434/job/20694762480
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1649%2Fphil-blain%2Fgithub-ci-skip-broken-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1649/phil-blain/github-ci-skip-broken-v1
Pull-Request: https://github.com/git/git/pull/1649
t/test-lib-github-workflow-markup.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/test-lib-github-workflow-markup.sh b/t/test-lib-github-workflow-markup.sh
index 970c6538cba..33405c90d74 100644
--- a/t/test-lib-github-workflow-markup.sh
+++ b/t/test-lib-github-workflow-markup.sh
@@ -42,8 +42,8 @@ finalize_test_case_output () {
fixed)
echo >>$github_markup_output "::notice::fixed: $this_test.$test_count $1"
;;
- ok)
- # Exit without printing the "ok" tests
+ ok|broken)
+ # Exit without printing the "ok" or ""broken" tests
return
;;
esac
base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
--
gitgitgadget
^ permalink raw reply related
* Git pre-received hook not failing with exit code 1 correcly
From: Marc C @ 2024-01-21 1:57 UTC (permalink / raw)
To: git
Heya!
I am trying to understand why my pre-publish hook does not exit with the
correct status code.
I have a pre-publish script
```
#!/bin/sh
set -euo pipefail
echo "Testing nixos config"
nixos-rebuild dry-build
echo "Success"
```
Running it directly in my CLI, it acts like I would expect and return
exit code 1.
```
$ ./.git/hooks/pre-receive
Testing nixos config
building the system configuration...
error:
… while calling the 'seq' builtin
....
...
$ echo "$?"
1
```
However, when running the script as a pre-receive hook, it is not
running the commands correctly and returns the wrong exit code. I get
the following:
```
remote: Testing nixos config
remote: building the system configuration...
remote: Success <-- ????
remote: error:
remote: … while calling the 'seq' builtin
...
To myserver:/myrepo
bffa94e..a14b3f6 main -> main
```
Any clue what I am missing? When running it as a pre-receive hook, the
failing command returns exit code 0. Running it in the CLI, it returns
exit code 1. It is Schrodinger's exit code.
Thank you so much for your help.
Sincerely,
Marc
^ permalink raw reply
* Re: Full disclosure
From: Ruben Safir @ 2024-01-21 0:41 UTC (permalink / raw)
To: Michael Lohmann, christian.couder
Cc: git, gitster, j6t, newren, phillip.wood123
In-Reply-To: <20240117174125.22915-1-mi.al.lohmann@gmail.com>
https://www.zazzle.com/idit_aharons_tzfat_original_backpack_design-256246398431855710
On 1/17/24 12:41, Michael Lohmann wrote:
> Hi Christian!
>
> Yes: I agree that the current state of the last submitted patch in that
> discussion is far from optimal.
> After Jeff Kings explanation I had a much better understanding for the
> situation and the reasoning (and his suggestion was definitely better than
> mine).
>
> I had already prepared a new version which tackled (I think) pretty much all of
> the criticisms. But then the above mentioned message came in and when I read
> this:
>
>> [...] they are trying to be different for the sake of being different, which
>> is not a good sign. I'd want our contributors to be original where being
>> original matters more.
>
> I am reading:
>
> 1) I am "trying to be different for the sake of being different"
> 2) Contributors like this are not wanted
>
> So yes, I do understand this as a general statement on me as a contributor
> without any proposal for me at least to explain the situation from my side.
>
> I teach my colleges not to name variables with how they are initialized, but
> with what information they actually convey and I found the "_NONE" one at least
> misleading in its name.
>
> So in the initial discussion I was a bit stubborn, because Philip wrote "I
> don't have a strong opinion" and from my perspective the only argument was
> "over there we also do it that way" (which _can_ 100% be a valid reason), but
> Junio C Hamano did not even acknowledge my criticisms of the other the variable
> name. I am totally fine with a decision like this if done consciously, but if I
> don't even get an acknowledgement, let alone an explanation, my demands I place
> on my code quality are that I write the best code I can. And with all the info
> I had (prior to Jeffs message) I did still favour my first suggestion.
>
> In my eyes it would be helpful to at least tell me what your (in my eyes not
> obvious) preferences are on naming things, because otherwise I will rather
> stick to my standards than blindly follow a single instance of other code where
> I don't even know if that was a concious decision or it just happened by
> accident.
>
> So no, I don't agree with the assessment of point 1), but I still read the
> message like that. I will accept it and instead use my skills in different
> projects where they are indeed valued.
>
> Cheers
> Michael
--
So many immigrant groups have swept through our town
that Brooklyn, like Atlantis, reaches mythological
proportions in the mind of the world - RI Safir 1998
http://www.mrbrklyn.com
DRM is THEFT - We are the STAKEHOLDERS - RI Safir 2002
http://www.nylxs.com - Leadership Development in Free Software
http://www.brooklyn-living.com
Being so tracked is for FARM ANIMALS and extermination camps,
but incompatible with living as a free human being. -RI Safir 2013
^ permalink raw reply
* Re: [PATCH v3] merge-ll: expose revision names to custom drivers
From: Junio C Hamano @ 2024-01-20 22:49 UTC (permalink / raw)
To: Phillip Wood; +Cc: Antonin Delpeuch via GitGitGadget, git, Antonin Delpeuch
In-Reply-To: <82624802-aa7f-4856-b819-9a2990b25a69@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Thanks for working on this, I think it is a useful improvement. I
> guess '%X' and '%Y' are no worse than the existing '%A' and '%B' but I
> do wonder if we want to take the opportunity to switch to more
> descriptive names for the various parameters passed to the custom
> merge strategy. We do do this by supporting %(label:ours) modeled
> after the format specifiers used by other commands such as "git log"
> and "git for-each-ref".
Perhaps. Unlike the --format option these commands take, the
placeholders are never typed from the command line (they always are
taken from the configuration file), so mnemonic value longer version
gives over the current single-letter ones is not as valuable, while
making the total line length longer. So I dunno.
>> [...]
>> +will be stored via placeholder `%P`. Additionally, the names of the
>> +common ancestor revision (`%S`), of the current revision (`%X`) and
>> +of the other branch (`%Y`) can also be supplied. Those are short > +revision names, optionally joined with the paths of the file in each
>> +revision. Those paths are only present if they differ and are separated
>> +from the revision by a colon.
>
> It might be simpler to just call these the "conflict marker labels"
> without tying ourselves to a particular format. Something like
>
> The conflict labels to be used for the common ancestor, local head
> and other head can be passed by using '%(label:base)',
> '%(label:ours)' and '%(label:theirs) respectively.
Yeah, that sounds like a good improvement, even if we did not use
the longhand placeholders and replaced %(label:{base,ours,theirs})
with %S, %X, and %Y.
>> @@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
>
> Not part of this patch but I noticed that we're passing the filenames
> for '%A' etc. unquoted which is a bit scary.
>
>> strbuf_addf(&cmd, "%d", marker_size);
>> else if (skip_prefix(format, "P", &format))
>> sq_quote_buf(&cmd, path);
>> + else if (skip_prefix(format, "S", &format))
>> + sq_quote_buf(&cmd, orig_name);
>
> I think you can avoid the SIGSEV problem you mentioned in your other
> email by changing this to
>
> sq_quote_buf(&cmd, orig_name ? orig_name, "");
>
> That would make sure the labels we pass match the ones used by the
> internal merge.
Makes sense. That would be much better than using hardcoded string
"ours", "theirs", etc.
^ permalink raw reply
* Re: [PATCH] setup: allow cwd=.git w/ bareRepository=explicit
From: Junio C Hamano @ 2024-01-20 22:26 UTC (permalink / raw)
To: Kyle Lippincott via GitGitGadget; +Cc: git, Kyle Lippincott
In-Reply-To: <pull.1645.git.1705709303098.gitgitgadget@gmail.com>
"Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Kyle Lippincott <spectral@google.com>
>
> The safe.bareRepository setting can be set to 'explicit' to disallow
> implicit uses of bare repositories, preventing an attack [1] where an
> artificial and malicious bare repository is embedded in another git
> repository. Unfortunately, some tooling uses myrepo/.git/ as the cwd
> when executing commands, and this is blocked when
> safe.bareRepository=explicit. Blocking is unnecessary, as git already
> prevents nested .git directories.
In other words, if the directory $D that is the top level of the
working tree of a non-bare repository, you should be able to chdir
to "$D/.git" and run your git command without explicitly saying that
you are inside $GIT_DIR (e.g. "git --git-dir=$(pwd) cmd")?
It makes very good sense.
I briefly wondered if this would give us a great usability
improvement especially for hook scripts, but they are given GIT_DIR
when called already so that is not a big upside, I guess.
> Teach git to not reject uses of git inside of the .git directory: check
> if cwd is .git (or a subdirectory of it) and allow it even if
> safe.bareRepository=explicit.
> My primary concern with this patch is that I'm unsure if we need to
> worry about case-insensitive filesystems (ex: cwd=my_repo/.GIT instead
> of my_repo/.git, it might not trigger this logic and end up allowed).
You are additionally allowing ".git" so even if somebody has ".GIT"
that won't be allowed by this change, no?
> I'm assuming this isn't a significant concern, for two reasons:
>
> * most filesystems/OSes in use today (by number of users) are at least
> case-preserving, so users/tools will have had to type out .GIT
> instead of getting it from readdir/wherever.
> * this is primarily a "quality of life" change to the feature, and if
> we get it wrong we still fail closed.
Yup.
If we really cared (which I doubt), we could resort to checking with
is_ntfs_dotgit() and is_hfs_dotgit(), but that would work in the
direction of loosening the check even further, which I do not know
is needed.
> - if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)
> + if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&
> + !ends_with_path_components(dir->buf, ".git"))
> return GIT_DIR_DISALLOWED_BARE;
Thanks.
^ permalink raw reply
* Re: [PATCH] git-p4: use raw string literals for regular expressions
From: Junio C Hamano @ 2024-01-20 21:54 UTC (permalink / raw)
To: James Touton; +Cc: James Touton via GitGitGadget, git
In-Reply-To: <CAA3pzWRRrwLN16RsNEJjdQg7vnSM_p_MtLqCU2ZXg5L=WPc-RA@mail.gmail.com>
James Touton <bekenn@gmail.com> writes:
> ... is it enough to mention that they're
> already in use?
Sure. Thanks.
^ permalink raw reply
* Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: Kristoffer Haugsbakk @ 2024-01-20 21:38 UTC (permalink / raw)
To: brianmlyles; +Cc: Taylor Blau, Elijah Newren, git
In-Reply-To: <20240119060721.3734775-2-brianmlyles@gmail.com>
Initial discussion (proposal): https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=Cd8Q@mail.gmail.com/
On Fri, Jan 19, 2024, at 06:59, brianmlyles@gmail.com wrote:
> From: Brian Lyles <brianmlyles@gmail.com>
>
> Previously, a consumer of the sequencer that wishes to take advantage of
> either the `keep_redundant_commits` or `drop_redundant_commits` feature
> must also specify `allow_empty`.
Previously to this change? It is preferred to describe what the code
currently does without this change in the present tense.[1] The change
itself uses the imperative mood.[2]
† 1: SubmittingPatches, “The problem statement that describes the status
quo …”
† 2: SubmittingPatches, “Describe your changes in imperative mood […] as
if you are giving orders to the codebase to change its behavior.”
> The only consumer of `drop_redundant_commits` is `git-rebase`, which
> already allows empty commits by default and simply always enables
> `allow_empty`. `keep_redundant_commits` was also consumed by
> `git-cherry-pick`, which had to specify `allow-empty` when
> `keep_redundant_commits` was specified in order for the sequencer's
> `allow_empty()` to actually respect `keep_redundant_commits`.
>
> The latter is an interesting case: As noted in the docs, this means that
> `--keep-redundant-commits` implies `--allow-empty`, despite the two
> having distinct, non-overlapping meanings:
Huh. I’m used to the git-rebase(1) behavior and I definitely would have
just assumed that git-cherry-pick(1) behaves the same. :) Nice catch.
> This implication of `--allow-empty` therefore seems incorrect: One
> should be able to keep a commit that becomes empty without also being
> forced to pick commits that start as empty. However, today, the
> following series of commands would result in both the commit that became
> empty and the commit that started empty being picked despite only
> `--keep-redundant-commits` being specified:
Nice description of the current problem. All of it.
> In a future commit, an `--empty` option will be added to
> `git-cherry-pick`, meaning that `drop_redundant_commits` will be
> available in that command. For that to be possible with the current
> implementation of the sequencer's `allow_empty()`, `git-cherry-pick`
> would need to specify `allow_empty` with `drop_redundant_commits` as
> well, which is an even less intuitive implication of `--allow-empty`: in
> order to prevent redundant commits automatically, initially-empty
> commits would need to be kept automatically.
>
> Instead, this commit rewrites the `allow_empty()` logic to remove the
> over-arching requirement that `allow_empty` be specified in order to
> reach any of the keep/drop behaviors. Only if the commit was originally
> empty will `allow_empty` have an effect.
In general, phrases like “this commit <verb>” or “this patch <verb>” can
be rewritten to the “commanding” style (see [2]).[3] But here you’re
starting a new paragraph after having talked about a future commit, so
using the commanding style might be stylistically difficult to pull off
without breaking the flow of the text.
And “this [commit][patch] <verb>” seems to be used with some regularity in
any case.
🔗 3: https://lore.kernel.org/git/xmqqedeqienh.fsf@gitster.g/
> Disclaimer: This is my first contribution to the git project, and thus
> my first attempt at submitting a patch via `git-send-email`. It is also
> the first time I've touched worked in C in over a decade, and I really
> didn't work with it much before that either. I welcome any and all
> feedback on what I may have gotten wrong regarding the patch submission
> process, the code changes, or my commit messages.
This part (after the commit message) looks like the cover letter for the
series (the four patches). `SubmittingPatches` recommends submitting that
in a dedicated email message (for series that have more than one
patch). Maybe this cover letter style is just an alternative that is
equally accepted. But most series use a separate cover letter message for
what it’s worth.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v5 0/6] support remote archive via stateless transport
From: Linus Arver @ 2024-01-20 20:43 UTC (permalink / raw)
To: Jiang Xin, Git List, Junio C Hamano; +Cc: Jiang Xin
In-Reply-To: <cover.1705411391.git.zhiyou.jx@alibaba-inc.com>
Jiang Xin <worldhello.net@gmail.com> writes:
This v5 looks good code-wise. I've made small suggestions to make the
commit messages better, but they are just nits and you are free to
ignore them.
If you choose to reroll one more time, one additional thing you could do
is to use the word "protocol_v2" in all commit messages because that's
how that term looks like in the code (unless the "protocol-v2" string is
already the standard term used elsewhere).
Thanks.
^ permalink raw reply
* Re: [PATCH v5 5/6] transport-helper: call do_take_over() in connect_helper
From: Linus Arver @ 2024-01-20 20:37 UTC (permalink / raw)
To: Jiang Xin, Git List, Junio C Hamano; +Cc: Jiang Xin
In-Reply-To: <6ac0c8e105febe526dc64182845832297656a8a5.1705411391.git.zhiyou.jx@alibaba-inc.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> After successfully connecting to the smart transport by calling
> process_connect_service() in connect_helper(), run do_take_over() to
> replace the old vtable with a new one which has methods ready for the
> smart transport connection. This will fix the exit code of git-archive
s/will fix/fixes
> in test case "archive remote http repository" of t5003.
>
> The connect_helper() function is used as the connect method of the
> vtable in "transport-helper.c", and it is called by transport_connect()
> in "transport.c" to setup a connection. The only place that we call
> transport_connect() so far is in "builtin/archive.c". Without running
> do_take_over(), it may fail to call transport_disconnect() in
> run_remote_archiver() of "builtin/archive.c". This is because for a
> stateless connection and a service like "git-upload-archive", the
> remote helper may receive a SIGPIPE signal and exit early.
OK.
> To have a
> graceful disconnect method by calling do_take_over() will solve this
> issue.
How about rewording to
Call do_take_over() to have a graceful disconnect method, so that we
still call transport_disconnect() even if the remote helper exits
early.
to make "this issue" more explicit?
> Helped-by: Linus Arver <linusa@google.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> t/t5003-archive-zip.sh | 2 +-
> transport-helper.c | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
> index 6f85bd3463..961c6aac25 100755
> --- a/t/t5003-archive-zip.sh
> +++ b/t/t5003-archive-zip.sh
> @@ -268,7 +268,7 @@ test_expect_success 'remote archive does not work with protocol v1' '
> '
>
> test_expect_success 'archive remote http repository' '
> - test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
> + git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
> --output=remote-http.zip HEAD &&
> test_cmp_bin d.zip remote-http.zip
> '
> diff --git a/transport-helper.c b/transport-helper.c
> index 6fe9f4f208..91381be622 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -669,6 +669,8 @@ static int connect_helper(struct transport *transport, const char *name,
>
> fd[0] = data->helper->out;
> fd[1] = data->helper->in;
> +
> + do_take_over(transport);
> return 0;
> }
>
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH v5 2/6] remote-curl: supports git-upload-archive service
From: Linus Arver @ 2024-01-20 20:30 UTC (permalink / raw)
To: Jiang Xin, Git List, Junio C Hamano; +Cc: Jiang Xin
In-Reply-To: <6be331b22d51e1f6f96cb0035d99db5b8cede676.1705411391.git.zhiyou.jx@alibaba-inc.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> Add new service (git-upload-archive) support in remote-curl, so we can
> support remote archive over HTTP/HTTPS protocols. Differences between
> git-upload-archive and other serices:
s/serices/services
> 1. The git-archive command does not expect to see protocol version and
> capabilities when connecting to remote-helper, so do not send them
> in remote-curl for the git-upload-archive service.
>
> 2. We need to detect protocol version by calling discover_refs(),
s/,/.
> Fallback to use the git-upload-pack service (which, like
> git-upload-archive, is a read-only operation) to discover protocol
> version.
>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> remote-curl.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index ef05752ca5..ce6cb8ac05 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)
> * establish a stateless connection, otherwise we need to tell the
> * client to fallback to using other transport helper functions to
> * complete their request.
> + *
> + * The "git-upload-archive" service is a read-only operation. Fallback
> + * to use "git-upload-pack" service to discover protocol version.
> */
> - discover = discover_refs(service_name, 0);
> + if (!strcmp(service_name, "git-upload-archive"))
> + discover = discover_refs("git-upload-pack", 0);
> + else
> + discover = discover_refs(service_name, 0);
> if (discover->version != protocol_v2) {
> printf("fallback\n");
> fflush(stdout);
> @@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)
>
> /*
> * Dump the capability listing that we got from the server earlier
> - * during the info/refs request.
> + * during the info/refs request. This does not work with the
> + * "git-upload-archive" service.
> */
> - write_or_die(rpc.in, discover->buf, discover->len);
> + if (strcmp(service_name, "git-upload-archive"))
> + write_or_die(rpc.in, discover->buf, discover->len);
>
> /* Until we see EOF keep sending POSTs */
> while (1) {
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH v5 1/6] transport-helper: no connection restriction in connect_helper
From: Linus Arver @ 2024-01-20 20:28 UTC (permalink / raw)
To: Jiang Xin, Git List, Junio C Hamano; +Cc: Jiang Xin
In-Reply-To: <f3fef46c058968f6d0ad5a48776bd2f59ab45868.1705411391.git.zhiyou.jx@alibaba-inc.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Remove the restriction in the "connect_helper()" function and give the
> function "process_connect_service()" the opportunity to establish a
> connection using ".connect" or ".stateless_connect" for protocol v2. So
> we can connect with a stateless-rpc and do something useful. E.g., in a
> later commit, implements remote archive for a repository over HTTP
> protocol.
Nit: Perhaps add something like the following for the commit message?
Removing the restriction does not change behavior, because
process_connect_service() will return 0 if both data->connect and
data->stateless_connect are false, and we'll still die() early.
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Linus Arver <linusa@google.com>
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> transport-helper.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 49811ef176..2e127d24a5 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>
> /* Get_helper so connect is inited. */
> get_helper(transport);
> - if (!data->connect)
> - die(_("operation not supported by protocol"));
>
> if (!process_connect_service(transport, name, exec))
> die(_("can't connect to subservice %s"), name);
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper
From: Linus Arver @ 2024-01-20 20:25 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <CANYiYbGy-APMD7Cw=m-=8dkMcXCp9c+x_6OCoBhWBfvUUWm2ow@mail.gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> If both data->connect and data->stateless_connect are false,
> process_connect_service() will return 0 instead of making a connection
> and returning 1. The return value will be checked in the function
> connect_helper() as follows:
>
> if (!process_connect_service(transport, name, exec))
> die(_("can't connect to subservice %s"), name);
>
> So I think it's not necessary to make double check in connect_helper().
Ah, thank you for the clarification.
> The best position to address the bug that both data->connect and
> data->stateless_connect are enabled is in the function get_helper() as
> below:
>
> } else if (!strcmp(capname, "connect")) {
> data->connect = 1;
> } else if (!strcmp(capname, "stateless-connect")) {
> data->stateless_connect = 1;
> }
> ... ...
> if (data->connect && data->stateless_connect)
> die("cannot have both connect and stateless_connect enabled");
>
> I consider this change to be off-topic and it will not be introduced
> in this series.
SG.
^ permalink raw reply
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Kristoffer Haugsbakk @ 2024-01-20 20:24 UTC (permalink / raw)
To: brianmlyles; +Cc: Taylor Blau, Elijah Newren, git
In-Reply-To: <20240119060721.3734775-5-brianmlyles@gmail.com>
Hi
On Fri, Jan 19, 2024, at 06:59, brianmlyles@gmail.com wrote:
> From: Brian Lyles <brianmlyles@gmail.com>
> ---keep-redundant-commits::
> - If a commit being cherry picked duplicates a commit already in the
> - current history, it will become empty. By default these
> - redundant commits cause `cherry-pick` to stop so the user can
> - examine the commit. This option overrides that behavior and
> - creates an empty commit object. Note that use of this option only
> +--empty=(stop|drop|keep)::
> + How to handle commits being cherry-picked that are redundant with
> + changes already in the current history.
> ++
> +--
Trailing whitespace on this line.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH 09/10] trailer: move arg handling to interpret-trailers.c
From: Linus Arver @ 2024-01-20 20:14 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder
In-Reply-To: <xmqqo7dim8dz.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/trailer.c b/trailer.c
>> index e2d541372a3..0a86e0d5afa 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -66,6 +66,11 @@ static LIST_HEAD(conf_head);
>>
>> static char *separators = ":";
>>
>> +const char *default_separators(void)
>> +{
>> + return separators;
>> +}
>
> This allows API users to peek into the current set of separator
> bytes (either the default ":" or specified by the configuration
> varaible "trailer.separators"), which is an improvement over
> directly exposing the "separators" variable, but in a longer term,
> do we need to have some "trailer context" object that holds this
> and possibly other global variables like this?
Yes, and I've implemented a "trailer_subsystem_conf" struct to hold all
configuration-related bits (and hardcoded defaults) relating to trailers
in one place, in the larger series.
> I do not demand such further abstraction in this series, but I'd
> prefer to see if we all have shared vision into the future.
Makes sense. Thanks.
^ permalink raw reply
* Re: [PATCH 07/10] trailer: spread usage of "trailer_block" language
From: Linus Arver @ 2024-01-20 20:09 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder
In-Reply-To: <xmqqttnam8wd.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "info" is a word with so low information contents. At least "block"
> would hint that we are talking about a block of text that makes up
> the trailer attached to a single commit.
There is also "conf_info" and the whole "key"/"key alias" vs "token"
lingo which (if we are going to rename things for consistency) needs to
be updated to match documentation. In the larger series I've cleaned
this up as well.
^ permalink raw reply
* Re: [PATCH 05/10] sequencer: use the trailer iterator
From: Linus Arver @ 2024-01-20 20:04 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Christian Couder
In-Reply-To: <xmqq34uunoag.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> From: Linus Arver <linusa@google.com>
>> Also, teach the iterator about non-trailer lines, by adding a new field
>> called "raw" to hold both trailer and non-trailer lines. This is
>> necessary because a "trailer block" is a list of trailer lines of at
>> least 25% trailers (see 146245063e (trailer: allow non-trailers in
>> trailer block, 2016-10-21)), such that it may hold non-trailer lines.
>
> OK. This would change behaviour, wouldn't it, in the sense that we
> used to yield a non-trailer line from the old iterator but the new
> one skips them?
I think it's the other way; the old iterator only iterated over trailer
lines, skipping over non-trailer lines (see the "not a real trailer"
deleted bit for trailer_iterator_advance()). The new one iterates over
all lines found in the trailer block, whether they are trailer or
non-trailer lines.
The function insert_records_from_trailers() from shortlog.c uses the new
iterator, but has to be careful because the new iterator goes over
non-trailer lines too. That's why it now does
if (!iter.is_trailer)
continue;
to do the skipping itself.
> Is that something we can demonstrate and protect in
> tests (presumably once we conclude these refactoring, we would be
> able to call into this machinery from unit-testing framework)?
Yup, that is exactly what I want to do once the dust around the trailer
API settles down after this series (and also the larger series). :)
^ permalink raw reply
* Re: [PATCH] git-p4: use raw string literals for regular expressions
From: James Touton @ 2024-01-20 19:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: James Touton via GitGitGadget, git
In-Reply-To: <xmqqr0ibetap.fsf@gitster.g>
On Sat, Jan 20, 2024 at 10:47 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "James Touton via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: James Touton <bekenn@gmail.com>
> >
> > Fixes several Python diagnostics about invalid escape sequences.
>
> Thanks for noticing, but we want a bit more backstory explained here
> in the proposed commit log message, outlining:
>
> 1. With what version of Python the deprecation warning started.
>
> This will help us judge the urgency of the fix. If I am reading the
> docs.python.org/$version/howto/regex.html right, we do not see this
>
> In addition, special escape sequences that are valid in regular
> expressions, but not valid as Python string literals, now result
> in a DeprecationWarning and will eventually become a
> SyntaxError, which means the sequences will be invalid if raw
> string notation or escaping the backslashes isn’t used.
>
> in Python 3.5's document, but Python 3.6's document starts talking
> about the warning. Python 3.6 was released at the end of 2016 so it
> is 7 years old---users have lived with the warning for this many
> years, so if the above reasoning is correct, this is not all that
> urgent to require a maintenance release.
>
> 2. How well the new construct, used by the code after applying this
> patch, is supported by older version of Python.
>
> This will assure us that the change will not be robbing from users
> of older versions of Python to pay users of newer versions of
> Python. Again, if I am reading the documentation right, feeding r''
> raw strings to regexp engine was supported even by Python 2.7, which
> is what git-p4.py already requires, so we should be OK.
>
> But we want the developers who propose a change to explain why it is
> a good idea, and why it is a safe change to make, in their proposed
> commit log message, instead of forcing the reviewers to do that for
> them.
>
> For other syntactic and linguistic hints on writing a proposed log
> message, please check Documentation/SubmittingPatches document.
>
> Thanks, again.
Thanks for the notes, I will update this when I have the opportunity.
Raw strings were already present in the file, just not in these
particular locations. Given that, I wouldn't expect them to need any
particular explanation; do you still want some mention of
compatibility constraints, or is it enough to mention that they're
already in use?
^ permalink raw reply
* Re: [PATCH] git-p4: use raw string literals for regular expressions
From: Junio C Hamano @ 2024-01-20 18:47 UTC (permalink / raw)
To: James Touton via GitGitGadget; +Cc: git, James Touton
In-Reply-To: <pull.1639.git.1705775149642.gitgitgadget@gmail.com>
"James Touton via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: James Touton <bekenn@gmail.com>
>
> Fixes several Python diagnostics about invalid escape sequences.
Thanks for noticing, but we want a bit more backstory explained here
in the proposed commit log message, outlining:
1. With what version of Python the deprecation warning started.
This will help us judge the urgency of the fix. If I am reading the
docs.python.org/$version/howto/regex.html right, we do not see this
In addition, special escape sequences that are valid in regular
expressions, but not valid as Python string literals, now result
in a DeprecationWarning and will eventually become a
SyntaxError, which means the sequences will be invalid if raw
string notation or escaping the backslashes isn’t used.
in Python 3.5's document, but Python 3.6's document starts talking
about the warning. Python 3.6 was released at the end of 2016 so it
is 7 years old---users have lived with the warning for this many
years, so if the above reasoning is correct, this is not all that
urgent to require a maintenance release.
2. How well the new construct, used by the code after applying this
patch, is supported by older version of Python.
This will assure us that the change will not be robbing from users
of older versions of Python to pay users of newer versions of
Python. Again, if I am reading the documentation right, feeding r''
raw strings to regexp engine was supported even by Python 2.7, which
is what git-p4.py already requires, so we should be OK.
But we want the developers who propose a change to explain why it is
a good idea, and why it is a safe change to make, in their proposed
commit log message, instead of forcing the reviewers to do that for
them.
For other syntactic and linguistic hints on writing a proposed log
message, please check Documentation/SubmittingPatches document.
Thanks, again.
> Signed-off-by: James Touton <bekenn@gmail.com>
> ---
> git-p4: use raw string literals for regular expressions
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1639%2FBekenn%2Fp4-raw-strings-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1639/Bekenn/p4-raw-strings-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1639
>
> git-p4.py | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 0eb3bb4c47d..156597adb59 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -689,8 +689,8 @@ def setP4ExecBit(file, mode):
>
> if not isModeExec(mode):
> p4Type = getP4OpenedType(file)
> - p4Type = re.sub('^([cku]?)x(.*)', '\\1\\2', p4Type)
> - p4Type = re.sub('(.*?\+.*?)x(.*?)', '\\1\\2', p4Type)
> + p4Type = re.sub(r'^([cku]?)x(.*)', r'\1\2', p4Type)
> + p4Type = re.sub(r'(.*?\+.*?)x(.*?)', r'\1\2', p4Type)
> if p4Type[-1] == "+":
> p4Type = p4Type[0:-1]
>
> @@ -701,7 +701,7 @@ def getP4OpenedType(file):
> """Returns the perforce file type for the given file."""
>
> result = p4_read_pipe(["opened", wildcard_encode(file)])
> - match = re.match(".*\((.+)\)( \*exclusive\*)?\r?$", result)
> + match = re.match(r".*\((.+)\)( \*exclusive\*)?\r?$", result)
> if match:
> return match.group(1)
> else:
> @@ -757,7 +757,7 @@ def parseDiffTreeEntry(entry):
>
> global _diff_tree_pattern
> if not _diff_tree_pattern:
> - _diff_tree_pattern = re.compile(':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
> + _diff_tree_pattern = re.compile(r':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
>
> match = _diff_tree_pattern.match(entry)
> if match:
> @@ -918,9 +918,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
> if len(result) > 0:
> data = result[0].get('data')
> if data:
> - m = re.search('Too many rows scanned \(over (\d+)\)', data)
> + m = re.search(r'Too many rows scanned \(over (\d+)\)', data)
> if not m:
> - m = re.search('Request too large \(over (\d+)\)', data)
> + m = re.search(r'Request too large \(over (\d+)\)', data)
>
> if m:
> limit = int(m.group(1))
> @@ -1452,7 +1452,7 @@ def wildcard_encode(path):
>
>
> def wildcard_present(path):
> - m = re.search("[*#@%]", path)
> + m = re.search(r"[*#@%]", path)
> return m is not None
>
>
> @@ -3048,7 +3048,7 @@ def stripRepoPath(self, path, prefixes):
> # Preserve everything in relative path name except leading
> # //depot/; just look at first prefix as they all should
> # be in the same depot.
> - depot = re.sub("^(//[^/]+/).*", r'\1', prefixes[0])
> + depot = re.sub(r"^(//[^/]+/).*", r'\1', prefixes[0])
> if p4PathStartsWith(path, depot):
> path = path[len(depot):]
>
> @@ -3603,7 +3603,7 @@ def importP4Labels(self, stream, p4Labels):
> commitFound = True
> else:
> gitCommit = read_pipe(["git", "rev-list", "--max-count=1",
> - "--reverse", ":/\[git-p4:.*change = %d\]" % changelist], ignore_error=True)
> + "--reverse", r":/\[git-p4:.*change = %d\]" % changelist], ignore_error=True)
> if len(gitCommit) == 0:
> print("importing label %s: could not find git commit for changelist %d" % (name, changelist))
> else:
> @@ -4182,7 +4182,7 @@ def run(self, args):
> if len(self.changesFile) == 0:
> revision = "#head"
>
> - p = re.sub("\.\.\.$", "", p)
> + p = re.sub(r"\.\.\.$", "", p)
> if not p.endswith("/"):
> p += "/"
>
> @@ -4291,7 +4291,7 @@ def rebase(self):
> die("Cannot find upstream branchpoint for rebase")
>
> # the branchpoint may be p4/foo~3, so strip off the parent
> - upstream = re.sub("~[0-9]+$", "", upstream)
> + upstream = re.sub(r"~[0-9]+$", "", upstream)
>
> print("Rebasing the current branch onto %s" % upstream)
> oldHead = read_pipe(["git", "rev-parse", "HEAD"]).strip()
> @@ -4320,8 +4320,8 @@ def __init__(self):
> def defaultDestination(self, args):
> # TODO: use common prefix of args?
> depotPath = args[0]
> - depotDir = re.sub("(@[^@]*)$", "", depotPath)
> - depotDir = re.sub("(#[^#]*)$", "", depotDir)
> + depotDir = re.sub(r"(@[^@]*)$", "", depotPath)
> + depotDir = re.sub(r"(#[^#]*)$", "", depotDir)
> depotDir = re.sub(r"\.\.\.$", "", depotDir)
> depotDir = re.sub(r"/$", "", depotDir)
> return os.path.split(depotDir)[1]
>
> base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
^ permalink raw reply
* Re: How OBJ_REF_DELTA pack file size calculated
From: Junio C Hamano @ 2024-01-20 18:26 UTC (permalink / raw)
To: Farhan Khan; +Cc: git
In-Reply-To: <afea6dc9-e557-4730-abe6-00947f77be06@app.fastmail.com>
"Farhan Khan" <farhan@farhan.codes> writes:
> 82daab01f43e34b9f7c8e0db81a9951933b04f1b commit 94 101 82749 1 ecd0e8c88ed8891da372f5630d542150b0a0531e
>
> The size of the object is 94 bytes
> The size of the entry is 101 bytes.
> My patching/reconstruction of the object works, the compressed
> size is 97 bytes.
What do you mean by this?
The dense object header expresses the inflated size of the object
(which should be 94 in your case). By expressing it as a delta
against some other object in the pack and then deflating the delta,
we should get the data that is much smaller than 94+20 if we choose
to express it in the OBJ_REF_DELTA representation, as with such a
suboptimal delta base, we would be better off expressing it as a
base object that is merely deflated. We do not need 20-byte offset
overhead, and when reconstructing the object, they do not need to
deflate the base object and apply the delta.
So I am not sure what you mean by "the compressed size is 97 bytes".
> However, I cannot figure out where the 101 comes
> from. The size of the object header is 2 bytes, the OBJ_REF_DELTA
> is 20 bytes (the SHA1), but that does not add up to 101 bytes.
$ git help format-pack
- The header is followed by a number of object entries, each of
which looks like this:
(undeltified representation)
n-byte type and length (3-bit type, (n-1)*7+4-bit length)
compressed data
(deltified representation)
n-byte type and length (3-bit type, (n-1)*7+4-bit length)
base object name if OBJ_REF_DELTA or a negative relative
offset from the delta object's position in the pack if this
is an OBJ_OFS_DELTA object
compressed delta data
Observation: the length of each object is encoded in a variable
length format and is not constrained to 32-bit or anything.
So, if the object header for this object in this pack is 2 bytes
long as you observed above, then 101 bytes should be 2 bytes of
header, 20 bytes of base object name, and the remainder would be a
deflated delta data that is 101 - 22 = 79 bytes. Reading the base
object and applying that delta (which deflates to 79 bytes) would
reconstruct the original 94 bytes of the object.
^ permalink raw reply
* [PATCH] git-p4: use raw string literals for regular expressions
From: James Touton via GitGitGadget @ 2024-01-20 18:25 UTC (permalink / raw)
To: git; +Cc: James Touton, James Touton
From: James Touton <bekenn@gmail.com>
Fixes several Python diagnostics about invalid escape sequences.
Signed-off-by: James Touton <bekenn@gmail.com>
---
git-p4: use raw string literals for regular expressions
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1639%2FBekenn%2Fp4-raw-strings-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1639/Bekenn/p4-raw-strings-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1639
git-p4.py | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index 0eb3bb4c47d..156597adb59 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -689,8 +689,8 @@ def setP4ExecBit(file, mode):
if not isModeExec(mode):
p4Type = getP4OpenedType(file)
- p4Type = re.sub('^([cku]?)x(.*)', '\\1\\2', p4Type)
- p4Type = re.sub('(.*?\+.*?)x(.*?)', '\\1\\2', p4Type)
+ p4Type = re.sub(r'^([cku]?)x(.*)', r'\1\2', p4Type)
+ p4Type = re.sub(r'(.*?\+.*?)x(.*?)', r'\1\2', p4Type)
if p4Type[-1] == "+":
p4Type = p4Type[0:-1]
@@ -701,7 +701,7 @@ def getP4OpenedType(file):
"""Returns the perforce file type for the given file."""
result = p4_read_pipe(["opened", wildcard_encode(file)])
- match = re.match(".*\((.+)\)( \*exclusive\*)?\r?$", result)
+ match = re.match(r".*\((.+)\)( \*exclusive\*)?\r?$", result)
if match:
return match.group(1)
else:
@@ -757,7 +757,7 @@ def parseDiffTreeEntry(entry):
global _diff_tree_pattern
if not _diff_tree_pattern:
- _diff_tree_pattern = re.compile(':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
+ _diff_tree_pattern = re.compile(r':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
match = _diff_tree_pattern.match(entry)
if match:
@@ -918,9 +918,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
if len(result) > 0:
data = result[0].get('data')
if data:
- m = re.search('Too many rows scanned \(over (\d+)\)', data)
+ m = re.search(r'Too many rows scanned \(over (\d+)\)', data)
if not m:
- m = re.search('Request too large \(over (\d+)\)', data)
+ m = re.search(r'Request too large \(over (\d+)\)', data)
if m:
limit = int(m.group(1))
@@ -1452,7 +1452,7 @@ def wildcard_encode(path):
def wildcard_present(path):
- m = re.search("[*#@%]", path)
+ m = re.search(r"[*#@%]", path)
return m is not None
@@ -3048,7 +3048,7 @@ def stripRepoPath(self, path, prefixes):
# Preserve everything in relative path name except leading
# //depot/; just look at first prefix as they all should
# be in the same depot.
- depot = re.sub("^(//[^/]+/).*", r'\1', prefixes[0])
+ depot = re.sub(r"^(//[^/]+/).*", r'\1', prefixes[0])
if p4PathStartsWith(path, depot):
path = path[len(depot):]
@@ -3603,7 +3603,7 @@ def importP4Labels(self, stream, p4Labels):
commitFound = True
else:
gitCommit = read_pipe(["git", "rev-list", "--max-count=1",
- "--reverse", ":/\[git-p4:.*change = %d\]" % changelist], ignore_error=True)
+ "--reverse", r":/\[git-p4:.*change = %d\]" % changelist], ignore_error=True)
if len(gitCommit) == 0:
print("importing label %s: could not find git commit for changelist %d" % (name, changelist))
else:
@@ -4182,7 +4182,7 @@ def run(self, args):
if len(self.changesFile) == 0:
revision = "#head"
- p = re.sub("\.\.\.$", "", p)
+ p = re.sub(r"\.\.\.$", "", p)
if not p.endswith("/"):
p += "/"
@@ -4291,7 +4291,7 @@ def rebase(self):
die("Cannot find upstream branchpoint for rebase")
# the branchpoint may be p4/foo~3, so strip off the parent
- upstream = re.sub("~[0-9]+$", "", upstream)
+ upstream = re.sub(r"~[0-9]+$", "", upstream)
print("Rebasing the current branch onto %s" % upstream)
oldHead = read_pipe(["git", "rev-parse", "HEAD"]).strip()
@@ -4320,8 +4320,8 @@ def __init__(self):
def defaultDestination(self, args):
# TODO: use common prefix of args?
depotPath = args[0]
- depotDir = re.sub("(@[^@]*)$", "", depotPath)
- depotDir = re.sub("(#[^#]*)$", "", depotDir)
+ depotDir = re.sub(r"(@[^@]*)$", "", depotPath)
+ depotDir = re.sub(r"(#[^#]*)$", "", depotDir)
depotDir = re.sub(r"\.\.\.$", "", depotDir)
depotDir = re.sub(r"/$", "", depotDir)
return os.path.split(depotDir)[1]
base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v3] merge-ll: expose revision names to custom drivers
From: Phillip Wood @ 2024-01-20 18:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Antonin Delpeuch via GitGitGadget, git, Antonin Delpeuch
In-Reply-To: <xmqqsf2rgb39.fsf@gitster.g>
Hi Junio
On 20/01/2024 17:37, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> Not part of this patch but I noticed that we're passing the filenames
>> for '%A' etc. unquoted which is a bit scary.
>
> May be scary but safe, as long as create_temp() gives a reasonable
> temporary filename. We pass ".merge_file_XXXXXX" to xmkstemp(),
> which calls into mkstemp(), which should give us a shell safe name?
Yes. I'd mis-read create_temp() and thought we were appending
".merge_file_XXXXX" to the path being merged but looking at it again it
is safe.
> It also should be a safe conversion to change strbuf_addstr() used
> for these three to sq_quote_buf(), as the string with these %[OAB]
> placeholders are passed to the shell that eats the quoting before
> invoking the end-user supplied external merge driver, which means
> the merge driver would not notice any difference.
I agree that would be a safe conversion , but I'm not sure it is worth it.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] MyFirstContribution: update mailing list sub steps
From: Junio C Hamano @ 2024-01-20 18:08 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Josh Soref, git, spectral, Konstantin Ryabitsev
In-Reply-To: <527ae359-32e7-4c49-b733-614f17876f14@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> Just curious. What’s the difference between `Reported-by` and
> `Noticed-by`? `Reported-by` is documented in `SubmittingPatches` but not
> the other one. Is perhaps `Reported-by` specifically meant for bug
> reports?
It should be reported-by as the recent trend is to dedup similar
ones without much meaningful distinction and stick to those that are
listed in the document. It was just me being careless and picked
one that used to be relatively common without even thinking:
$ git rev-list --since 1.year --grep=Noticed-by: --count master
2
$ git rev-list --since 1.year --grep=Reported-by: --count master
54
$ git rev-list --since 5.year --until 4.year --grep=Noticed-by: --count master
4
$ git rev-list --since 5.year --until 4.year --grep=Reported-by: --count master
102
$ git rev-list --since 10.year --until 9.year --grep=Noticed-by: --count master
12
$ git rev-list --since 10.year --until 9.year --grep=Reported-by: --count master
54
There probably is a subtle difference perceived between the two by
those who used noticed-by over the years and I think you guessed
their intention right.
^ permalink raw reply
* Re: [Bug?] "git diff --no-rename A B"
From: Junio C Hamano @ 2024-01-20 17:55 UTC (permalink / raw)
To: René Scharfe; +Cc: Jeff King, git
In-Reply-To: <579fd5bc-3bfd-427f-b22d-dab5e7e56eb2@web.de>
René Scharfe <l.s.r@web.de> writes:
> Because diff_opt_parse() passes PARSE_OPT_KEEP_UNKNOWN_OPT, which makes
> parse_long_opt() skip abbreviation detection. Which it does since
> baa4adc66a (parse-options: disable option abbreviation with
> PARSE_OPT_KEEP_UNKNOWN, 2019-01-27).
>
> It added a condition only to the if. Its body can be reached via goto
> from two other places, though. So abbreviated options are effectively
> allowed through the back door.
Wow, that is nasty. Thanks for spotting.
I agree with you that the structure of that loop and the processing
in it does look confusing.
> --- >8 ---
> Subject: [PATCH] parse-options: fully disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN
>
> baa4adc66a (parse-options: disable option abbreviation with
> PARSE_OPT_KEEP_UNKNOWN, 2019-01-27) turned off support for abbreviated
> options when the flag PARSE_OPT_KEEP_UNKNOWN is given, as any shortened
> option could also be an abbreviation for one of the unknown options.
>
> The code for handling abbreviated options is guarded by an if, but it
> can also be reached via goto. baa4adc66a only blocked the first way.
> Add the condition to the other ones as well.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Formatted with --function-context for easier review.
> The code is a quite tangled, any ideas how to structure it better?
>
> parse-options.c | 8 +++++---
> t/t4013-diff-various.sh | 6 ++++++
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 4ce2b7ca16..92e96ca6cd 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -353,93 +353,95 @@ static int is_alias(struct parse_opt_ctx_t *ctx,
> static enum parse_opt_result parse_long_opt(
> struct parse_opt_ctx_t *p, const char *arg,
> const struct option *options)
> {
> const char *arg_end = strchrnul(arg, '=');
> const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
> enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
> + int allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);
>
> for (; options->type != OPTION_END; options++) {
> const char *rest, *long_name = options->long_name;
> enum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;
>
> if (options->type == OPTION_SUBCOMMAND)
> continue;
> if (!long_name)
> continue;
>
> again:
> if (!skip_prefix(arg, long_name, &rest))
> rest = NULL;
> if (!rest) {
> /* abbreviated? */
> - if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT) &&
> + if (allow_abbrev &&
> !strncmp(long_name, arg, arg_end - arg)) {
> is_abbreviated:
> if (abbrev_option &&
> !is_alias(p, abbrev_option, options)) {
> /*
> * If this is abbreviated, it is
> * ambiguous. So when there is no
> * exact match later, we need to
> * error out.
> */
> ambiguous_option = abbrev_option;
> ambiguous_flags = abbrev_flags;
> }
> if (!(flags & OPT_UNSET) && *arg_end)
> p->opt = arg_end + 1;
> abbrev_option = options;
> abbrev_flags = flags ^ opt_flags;
> continue;
> }
> /* negation allowed? */
> if (options->flags & PARSE_OPT_NONEG)
> continue;
> /* negated and abbreviated very much? */
> - if (starts_with("no-", arg)) {
> + if (allow_abbrev && starts_with("no-", arg)) {
> flags |= OPT_UNSET;
> goto is_abbreviated;
> }
> /* negated? */
> if (!starts_with(arg, "no-")) {
> if (skip_prefix(long_name, "no-", &long_name)) {
> opt_flags |= OPT_UNSET;
> goto again;
> }
> continue;
> }
> flags |= OPT_UNSET;
> if (!skip_prefix(arg + 3, long_name, &rest)) {
> /* abbreviated and negated? */
> - if (starts_with(long_name, arg + 3))
> + if (allow_abbrev &&
> + starts_with(long_name, arg + 3))
> goto is_abbreviated;
> else
> continue;
> }
> }
> if (*rest) {
> if (*rest != '=')
> continue;
> p->opt = rest + 1;
> }
> return get_value(p, options, flags ^ opt_flags);
> }
>
> if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
> die("disallowed abbreviated or ambiguous option '%.*s'",
> (int)(arg_end - arg), arg);
>
> if (ambiguous_option) {
> error(_("ambiguous option: %s "
> "(could be --%s%s or --%s%s)"),
> arg,
> (ambiguous_flags & OPT_UNSET) ? "no-" : "",
> ambiguous_option->long_name,
> (abbrev_flags & OPT_UNSET) ? "no-" : "",
> abbrev_option->long_name);
> return PARSE_OPT_HELP;
> }
> if (abbrev_option)
> return get_value(p, abbrev_option, abbrev_flags);
> return PARSE_OPT_UNKNOWN;
> }
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index cb094241ec..1e3b2dbea4 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -663,4 +663,10 @@ test_expect_success 'diff --default-prefix overrides diff.mnemonicprefix' '
> check_prefix actual a/file0 b/file0
> '
>
> +test_expect_success 'diff --no-renames cannot be abbreviated' '
> + test_expect_code 129 git diff --no-rename >actual 2>error &&
> + test_must_be_empty actual &&
> + grep "invalid option: --no-rename" error
> +'
> +
> test_done
> --
> 2.43.0
^ 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