* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Karthik Nayak @ 2024-01-04 11:31 UTC (permalink / raw)
To: Patrick Steinhardt, Junio C Hamano; +Cc: Taylor Blau, git, christian.couder
In-Reply-To: <ZZWg5JvjQymy2wcn@tanuki>
[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Wed, Jan 03, 2024 at 09:59:13AM -0800, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> >> I think we should tighten things up over time. First by teaching
>> >> the ref backend that anything that is not a pseudoref, HEAD or a
>> >> proper ref (one item of whose definition is "lives under refs/
>> >> hierarchy) should not resolve_ref() successfully. That should
>> >> correctly fail things like
>> >>
>> >> $ git rev-parse worktrees/$name/bisect/bad
>> >> $ git update-ref foo/bar HEAD
>> > ...
>> > Yeah, agreed, that's something we should do. I do wonder whether this
>> > will break existing usecases, but in any case I'd rather consider it an
>> > accident that it is possible to write (and read) such refs in the first
>> > place.
>>
>> Unfortunately, the worktrees/$name/refs/bisect/bad and its friends
>> are documented in "git worktree" and the refs.c layer is aware of
>> the "main-worktree/" and "worktrees/" hierarchy, so while I still
>> think it is a good long-term direction to make it impossible to
>> create random refs like "foo/bar" and "resf/heads/master" via the
>> commands like "git update-ref", we cannot limit ourselves only to
>> "refs/" hierarchy.
>
> Ah, I first wanted to point this out, but then noticed that you didn't
> include the "refs/" prefix in "worktrees/$name/bisect/bad" and thought
> this was intentional. But yes, per-worktree refs need to stay supported,
> weird as they may be.
>
> Patrick
Thanks all for the discussion, I'll try to summarize the path forward
as per my understanding.
1. We want to introduce a way to output all refs. This includes refs
under "refs/", pseudo refs, HEAD, and any other ref like objects under
$GIT_DIR. The reasoning being that users are allowed currently to create
refs without any directory restrictions. So with the upcoming reftable
backend, it becomes important to provide a utility to print all the refs
held in the reftable. Ideally we want to restrict such ref's from being
created but for the time being, since thats allowed, we should also
provide the utility to print such refs.
2. In the files backend, this would involve iterating through the
$GIT_DIR and finding all ref-like objects and printing them.
3. To expose this to the user, we could do something like
$ git for-each-ref ""
Which is a natural extension of the current syntax, where the empty
string would imply that we do not filter to the "refs/" directory.
It is still not clear whether we should support "worktrees".
Any corrections here?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.
From: Ghanshyam Thakkar @ 2024-01-04 10:39 UTC (permalink / raw)
To: Christian Couder; +Cc: git, newren, gitster, johannes.schindelin
In-Reply-To: <CAP8UFD1wMJMY6G4SaPTPwq6b9HbeXG1kB97-RRrL-KGN1wE0rg@mail.gmail.com>
On Thu Jan 4, 2024 at 3:54 PM IST, Christian Couder wrote:
> On Tue, Jan 2, 2024 at 11:17 PM Ghanshyam Thakkar
> <shyamthakkar001@gmail.com> wrote:
> >
> > Hello,
> >
> > I'm currently an undergrad beginning my journey of contributing to the
> > Git project. I am seeking feedback on doing "Heed core.bare from
> > template config file when no command line override given" described
> > here
> > https://lore.kernel.org/git/5b39c530f2a0edf3b1492fa13a1132d622a0678e.1684218850.git.gitgitgadget@gmail.com/
> > by Elijah Newren, as a microproject. I would like to know from the
> > community, if the complexity and scope of the project is appropriate
> > for a microproject.
>
> Thanks for your interest in the next GSoC!
>
> My opinion is that it's too complex for a micro-project. Now maybe if
> Elijah or others are willing to help you on it, perhaps it will work
> out. I think it's safer to look at simpler micro-projects though.
>
Thank you for your feedback. I will wait for Elijah's response on this and
perhaps look at other microprojects in the meantime. Although, I think I will
be able to do this with others help.
> > e.g. in builtin/init-db.c :
> >
> > static int template_bare_config(const char *var, const char *value,
> > const struct config_context *ctx, void *cb)
> > {
> > if(!strcmp(var,"core.bare")) {
>
> We like to have a space character between "if" and "(" as well as after a ","
>
> > is_bare_repository_cfg = git_config_bool(var, value);
> > }
> > return 0;
> > }
> >
> > int cmd_init_db(int argc, const char **argv, const char *prefix)
> > {
> > ...
> > ...
> > if(is_bare_repository_cfg==-1) {
>
> We like to have a space character both before and after "==" as well
> as between "if" and "(".
>
> > if(!template_dir)
> > git_config_get_pathname("init.templateDir",
> > &template_dir);
> >
> > if(template_dir) {
> > const char* template_config_path
> > = xstrfmt("%s/config",
> > struct stat st;
> >
> > if(!stat(template_config_path, &st) &&
> > !S_ISDIR(st.st_mode)) {
> > git_config_from_file(template_bare_cfg,
> > template_config_path, NULL);
> > }
> > }
> > ...
> > ...
> > return init_db(git_dir, real_git_dir, template_dir, hash_algo,
> > initial_branch, init_shared_repository, flags);
> > }
> >
> > I also wanted to know if the global config files should have an effect
> > in deciding if the repo is bare or not.
> >
> > Curious to know your thoughts on, if this is the right approach or
> > does it require doing refactoring to bring all the logic in setup.c.
> > Based on your feedback, I can quickly send a patch.
>
> I don't know this area of the code well, so I don't think I can help
> you much on this.
Appreciate your suggestions above. Will keep them in mind while sending the
patch.
> Best,
> Christian.
Thanks,
Ghanshyam
^ permalink raw reply
* Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.
From: Christian Couder @ 2024-01-04 10:24 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git, newren, gitster, johannes.schindelin
In-Reply-To: <85d4e83c-b6c4-4308-ac8c-a65c911c8a95@gmail.com>
On Tue, Jan 2, 2024 at 11:17 PM Ghanshyam Thakkar
<shyamthakkar001@gmail.com> wrote:
>
> Hello,
>
> I'm currently an undergrad beginning my journey of contributing to the
> Git project. I am seeking feedback on doing "Heed core.bare from
> template config file when no command line override given" described
> here
> https://lore.kernel.org/git/5b39c530f2a0edf3b1492fa13a1132d622a0678e.1684218850.git.gitgitgadget@gmail.com/
> by Elijah Newren, as a microproject. I would like to know from the
> community, if the complexity and scope of the project is appropriate
> for a microproject.
Thanks for your interest in the next GSoC!
My opinion is that it's too complex for a micro-project. Now maybe if
Elijah or others are willing to help you on it, perhaps it will work
out. I think it's safer to look at simpler micro-projects though.
> e.g. in builtin/init-db.c :
>
> static int template_bare_config(const char *var, const char *value,
> const struct config_context *ctx, void *cb)
> {
> if(!strcmp(var,"core.bare")) {
We like to have a space character between "if" and "(" as well as after a ","
> is_bare_repository_cfg = git_config_bool(var, value);
> }
> return 0;
> }
>
> int cmd_init_db(int argc, const char **argv, const char *prefix)
> {
> ...
> ...
> if(is_bare_repository_cfg==-1) {
We like to have a space character both before and after "==" as well
as between "if" and "(".
> if(!template_dir)
> git_config_get_pathname("init.templateDir",
> &template_dir);
>
> if(template_dir) {
> const char* template_config_path
> = xstrfmt("%s/config",
> struct stat st;
>
> if(!stat(template_config_path, &st) &&
> !S_ISDIR(st.st_mode)) {
> git_config_from_file(template_bare_cfg,
> template_config_path, NULL);
> }
> }
> ...
> ...
> return init_db(git_dir, real_git_dir, template_dir, hash_algo,
> initial_branch, init_shared_repository, flags);
> }
>
> I also wanted to know if the global config files should have an effect
> in deciding if the repo is bare or not.
>
> Curious to know your thoughts on, if this is the right approach or
> does it require doing refactoring to bring all the logic in setup.c.
> Based on your feedback, I can quickly send a patch.
I don't know this area of the code well, so I don't think I can help
you much on this.
Best,
Christian.
^ permalink raw reply
* [PATCH v3] write-or-die: make GIT_FLUSH a Boolean environment variable
From: Chandra Pratap via GitGitGadget @ 2024-01-04 10:20 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1628.v2.git.1704268708720.gitgitgadget@gmail.com>
From: Chandra Pratap <chandrapratap3519@gmail.com>
Among Git's environment variable, the ones marked as "Boolean"
accept values in a way similar to Boolean configuration variables,
i.e. values like 'yes', 'on', 'true' and positive numbers are
taken as "on" and values like 'no', 'off', 'false' are taken as
"off".
GIT_FLUSH can be used to force Git to use non-buffered I/O when
writing to stdout. It can only accept two values, '1' which causes
Git to flush more often and '0' which makes all output buffered.
Make GIT_FLUSH accept more values besides '0' and '1' by turning it
into a Boolean environment variable, modifying the required logic.
Update the related documentation.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
write-or-die: make GIT_FLUSH a Boolean environment variable
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1628%2FChand-ra%2Fgit_flush-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1628/Chand-ra/git_flush-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1628
Range-diff vs v2:
1: 585c76fff68 ! 1: c98885c0ede write-or-die: make GIT_FLUSH a Boolean environment variable
@@ Documentation/git.txt: for further details.
- If this environment variable is set to "1", then commands such
+ If this Boolean environment variable is set to true, then commands such
as 'git blame' (in incremental mode), 'git rev-list', 'git log',
-- 'git check-attr' and 'git check-ignore' will
-- force a flush of the output stream after each record have been
-- flushed. If this
+ 'git check-attr' and 'git check-ignore' will
+ force a flush of the output stream after each record have been
+ flushed. If this
- variable is set to "0", the output of these commands will be done
-- using completely buffered I/O. If this environment variable is
-- not set, Git will choose buffered or record-oriented flushing
-- based on whether stdout appears to be redirected to a file or not.
-+ 'git check-attr' and 'git check-ignore' will force a flush of the output
-+ stream after each record have been flushed. If this variable is set to
-+ false, the output of these commands will be done using completely buffered
-+ I/O. If this environment variable is not set, Git will choose buffered or
-+ record-oriented flushing based on whether stdout appears to be redirected
-+ to a file or not.
-
- `GIT_TRACE`::
- Enables general trace messages, e.g. alias expansion, built-in
++ variable is set to false, the output of these commands will be done
+ using completely buffered I/O. If this environment variable is
+ not set, Git will choose buffered or record-oriented flushing
+ based on whether stdout appears to be redirected to a file or not.
## write-or-die.c ##
-@@ write-or-die.c: void maybe_flush_or_die(FILE *f, const char *desc)
+@@
+ void maybe_flush_or_die(FILE *f, const char *desc)
{
static int skip_stdout_flush = -1;
- struct stat st;
+- struct stat st;
- char *cp;
if (f == stdout) {
@@ write-or-die.c: void maybe_flush_or_die(FILE *f, const char *desc)
- if (cp)
- skip_stdout_flush = (atoi(cp) == 0);
- else if ((fstat(fileno(stdout), &st) == 0) &&
-+ if (!git_env_bool("GIT_FLUSH", -1))
-+ skip_stdout_flush = 1;
-+ else if (!fstat(fileno(stdout), &st) &&
- S_ISREG(st.st_mode))
- skip_stdout_flush = 1;
- else
+- S_ISREG(st.st_mode))
+- skip_stdout_flush = 1;
+- else
+- skip_stdout_flush = 0;
++ skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
++ if (skip_stdout_flush < 0) {
++ struct stat st;
++ if (fstat(fileno(stdout), &st))
++ skip_stdout_flush = 0;
++ else
++ skip_stdout_flush = S_ISREG(st.st_mode);
++ }
+ }
+ if (skip_stdout_flush && !ferror(f))
+ return;
Documentation/git.txt | 5 ++---
write-or-die.c | 19 ++++++++-----------
2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2535a30194f..d06eea024f7 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -724,13 +724,12 @@ for further details.
waiting for someone with sufficient permissions to fix it.
`GIT_FLUSH`::
-// NEEDSWORK: make it into a usual Boolean environment variable
- If this environment variable is set to "1", then commands such
+ If this Boolean environment variable is set to true, then commands such
as 'git blame' (in incremental mode), 'git rev-list', 'git log',
'git check-attr' and 'git check-ignore' will
force a flush of the output stream after each record have been
flushed. If this
- variable is set to "0", the output of these commands will be done
+ variable is set to false, the output of these commands will be done
using completely buffered I/O. If this environment variable is
not set, Git will choose buffered or record-oriented flushing
based on whether stdout appears to be redirected to a file or not.
diff --git a/write-or-die.c b/write-or-die.c
index 42a2dc73cd3..39421528653 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -19,20 +19,17 @@
void maybe_flush_or_die(FILE *f, const char *desc)
{
static int skip_stdout_flush = -1;
- struct stat st;
- char *cp;
if (f == stdout) {
if (skip_stdout_flush < 0) {
- /* NEEDSWORK: make this a normal Boolean */
- cp = getenv("GIT_FLUSH");
- if (cp)
- skip_stdout_flush = (atoi(cp) == 0);
- else if ((fstat(fileno(stdout), &st) == 0) &&
- S_ISREG(st.st_mode))
- skip_stdout_flush = 1;
- else
- skip_stdout_flush = 0;
+ skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
+ if (skip_stdout_flush < 0) {
+ struct stat st;
+ if (fstat(fileno(stdout), &st))
+ skip_stdout_flush = 0;
+ else
+ skip_stdout_flush = S_ISREG(st.st_mode);
+ }
}
if (skip_stdout_flush && !ferror(f))
return;
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] ci: add job performing static analysis on GitLab CI
From: Toon Claes @ 2024-01-04 9:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
In-Reply-To: <1536a5ef07ad24dafb5d685b40099882f89e6cc5.1703761005.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> --- a/ci/install-docker-dependencies.sh
> +++ b/ci/install-docker-dependencies.sh
> @@ -21,7 +21,7 @@ linux-musl)
> apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
> bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
> ;;
> -linux-*)
> +linux-*|StaticAnalysis)
> # Required so that apt doesn't wait for user input on certain packages.
> export DEBIAN_FRONTEND=noninteractive
>
> @@ -31,6 +31,11 @@ linux-*)
> perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl \
> libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
> apache2 cvs cvsps gnupg libcgi-pm-perl subversion
> +
> + if test "$jobname" = StaticAnalysis
> + then
> + apt install -q -y coccinelle
> + fi
I was wondering why this was added, because I would assume the GitHub
Workflow needed this too. Well, it seems the "StaticAnalysis" job for
the Workflow runs ci/install-dependencies.sh instead of
ci/install-docker-dependencies.sh. The ci/install-docker-dependencies.sh
script is only used in the GitHub Workflow for the "dockerized" jobs.
They set $jobname to "linux-musl", "linux32", and "pedantic", so this
change is not affected by that.
Bottom line, changes all look good to me.
--
Toon
^ permalink raw reply
* [PATCH v2] git-prompt: stop manually parsing HEAD with unknown ref formats
From: Patrick Steinhardt @ 2024-01-04 8:21 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, SZEDER Gábor, Karthik Nayak
In-Reply-To: <cc902954f30c2faa92d1c5a4469f0dcc23e4acfe.1700825779.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3043 bytes --]
We're manually parsing the HEAD reference in git-prompt to figure out
whether it is a symbolic or direct reference. This makes it intimately
tied to the on-disk format we use to store references and will stop
working once we gain additional reference backends in the Git project.
Ideally, we would refactor the code to exclusively use plumbing tools to
read refs such that we do not have to care about the on-disk format at
all. Unfortunately though, spawning processes can be quite expensive on
some systems like Windows. As the Git prompt logic may be executed quite
frequently we try very hard to spawn as few processes as possible. This
refactoring is thus out of question for now.
Instead, condition the logic on the repository's ref format: if the repo
uses the the "files" backend we can continue to use the old logic and
read the respective files from disk directly. If it's anything else,
then we use git-symbolic-ref(1) to read the value of HEAD.
This change makes the Git prompt compatible with the upcoming "reftable"
format.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
This patch depends on "ps/refstorage-extension", which is currently at
1b2234079b (t9500: write "extensions.refstorage" into config,
2023-12-29).
contrib/completion/git-prompt.sh | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 2c030050ae..71f179cba3 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -408,7 +408,7 @@ __git_ps1 ()
local repo_info rev_parse_exit_code
repo_info="$(git rev-parse --git-dir --is-inside-git-dir \
- --is-bare-repository --is-inside-work-tree \
+ --is-bare-repository --is-inside-work-tree --show-ref-format \
--short HEAD 2>/dev/null)"
rev_parse_exit_code="$?"
@@ -421,6 +421,8 @@ __git_ps1 ()
short_sha="${repo_info##*$'\n'}"
repo_info="${repo_info%$'\n'*}"
fi
+ local ref_format="${repo_info##*$'\n'}"
+ repo_info="${repo_info%$'\n'*}"
local inside_worktree="${repo_info##*$'\n'}"
repo_info="${repo_info%$'\n'*}"
local bare_repo="${repo_info##*$'\n'}"
@@ -479,12 +481,25 @@ __git_ps1 ()
b="$(git symbolic-ref HEAD 2>/dev/null)"
else
local head=""
- if ! __git_eread "$g/HEAD" head; then
- return $exit
- fi
- # is it a symbolic ref?
- b="${head#ref: }"
- if [ "$head" = "$b" ]; then
+
+ case "$ref_format" in
+ files)
+ if ! __git_eread "$g/HEAD" head; then
+ return $exit
+ fi
+
+ if [[ $head == "ref: "* ]]; then
+ head="${head#ref: }"
+ else
+ head=""
+ fi
+ ;;
+ *)
+ head="$(git symbolic-ref HEAD 2>/dev/null)"
+ ;;
+ esac
+
+ if test -z "$head"; then
detached=yes
b="$(
case "${GIT_PS1_DESCRIBE_STYLE-}" in
@@ -502,6 +517,8 @@ __git_ps1 ()
b="$short_sha..."
b="($b)"
+ else
+ b="$head"
fi
fi
fi
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH] rebase: clarify --reschedule-failed-exec default
From: Illia Bobyr @ 2024-01-04 8:06 UTC (permalink / raw)
To: git; +Cc: Ævar Arnfjörð Bjarmason, Illia Bobyr
Documentation should mention the default behavior.
It is better to explain the persistent nature of the
--reschedule-failed-exec flag from the user standpoint, rather than from
the implementation standpoint.
Signed-off-by: Illia Bobyr <illia.bobyr@gmail.com>
---
Documentation/git-rebase.txt | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git Documentation/git-rebase.txt Documentation/git-rebase.txt
index 1dd65..45d3c 100644
--- Documentation/git-rebase.txt
+++ Documentation/git-rebase.txt
@@ -626,13 +626,16 @@ See also INCOMPATIBLE OPTIONS below.
Automatically reschedule `exec` commands that failed. This only makes
sense in interactive mode (or when an `--exec` option was provided).
+
-Even though this option applies once a rebase is started, it's set for
-the whole rebase at the start based on either the
-`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1]
-or "CONFIGURATION" below) or whether this option is
-provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
-start would be overridden by the presence of
-`rebase.rescheduleFailedExec=true` configuration.
+This option applies once a rebase is started. It is preserved for the whole
+rebase based on, in order, the command line option provided to the initial `git
+rebase`, the `rebase.rescheduleFailedExec` configuration (see
+linkgit:git-config[1] or "CONFIGURATION" below), or it defaults to false.
++
+Recording this option for the whole rebase is a convenience feature. Otherwise
+an explicit `--no-reschedule-failed-exec` at the start would be overridden by
+the presence of a `rebase.rescheduleFailedExec=true` configuration when `git
+rebase --continue` is invoked. Currently, you can not, pass
+`--[no-]reschedule-failed-exec` to `git rebase --continue`.
--update-refs::
--no-update-refs::
--
2.40.1
^ permalink raw reply
* Does extending `--empty` to git-cherry-pick make sense?
From: Brian Lyles @ 2024-01-04 6:57 UTC (permalink / raw)
To: git; +Cc: Brian Lyles
The `--empty=(keep|drop|ask)` option added to git-rebase in
e98c4269c86019bfe057a91b4305f784365b6f0b seems like it would be
applicable to git-cherry-pick (and maybe git-revert for completeness?).
While the shared sequencer code likely would already handle this fairly
well (at a cursory glance from someone with very little knowledge of C
or git's codebase, admittedly), it's only exposed via git-rebase.
The use case in which this came up involves a semi-automated workflow
for moving commits between branches. At a high level, a tool is:
- Identifying commits to be picked based on a specific trailer value
- Using git-cherry-pick with `-x` to pick those commits
- Relying on the presence of the `(cherry picked from commit ...)`
trailer to avoid re-picking commits that have already been picked
If the picked commits are then rewritten in the upstream such that there
are squashes or fixups, that trailer can end up missing in the upstream.
The next time the tool runs, it will end up trying to re-pick commits
that are already represented there. As a result, those commits become
empty upon being picked a second time and the cherry-pick ends up
breaking for the user to resolve:
$ git cherry-pick main
On branch feature
You are currently cherry-picking commit cfd7cd9.
(all conflicts fixed: run "git cherry-pick --continue")
(use "git cherry-pick --skip" to skip this patch)
(use "git cherry-pick --abort" to cancel the cherry-pick operation)
nothing to commit, working tree clean
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:
git commit --allow-empty
Otherwise, please use 'git cherry-pick --skip'
I'll spare the details, but we do expect this to happen with enough
frequency that we'd really like to be able to prevent it. The
`--empty=drop` option sounds like exactly what we want here:
--empty=(drop|keep|ask)
How to handle commits that are not empty to start and are not
clean cherry-picks of any upstream commit, but which become
empty after rebasing (because they contain a subset of already
upstream changes). With drop (the default), commits that become
empty are dropped.
Is there any real barrier to exposing that option to git-cherry-pick as
well? Was this an oversight, or intentionally left out? The
corresponding commit message doesn't seem to indicate any specific
reason for limiting it to git-rebase.
Thanks,
-Brian Lyles
^ permalink raw reply
* Re: [PATCH] push: region_leave trace for negotiate_using_fetch
From: Junio C Hamano @ 2024-01-03 23:37 UTC (permalink / raw)
To: Sam Delmerico; +Cc: git, steadmon
In-Reply-To: <20240103224054.1940209-1-delmerico@google.com>
Sam Delmerico <delmerico@google.com> writes:
> There were two region_enter events for negotiate_using_fetch instead of
> one enter and one leave. This commit replaces the second region_enter
> event with a region_leave.
>
> Signed-off-by: Sam Delmerico <delmerico@google.com>
> ---
> fetch-pack.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Looks right, after skimming a29263cf (fetch-pack: add tracing for
negotiation rounds, 2022-08-02). Two questions and a half.
* How did you find it? Code inspection? While writing a script to
parse the output from around this area, your script noticed the
ever-increasing nesting level? Something else?
* Would it be feasible to write some tests or tools that find
similar problems (semi-)automatically?
* Is the breakage (before this patch) something easily demonstrated
in a new test in t/ somewhere? And if so, would it be worth
doing?
Thanks. Will queue.
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 31a72d43de..dba6d79944 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -2218,7 +2218,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
> the_repository, "%d",
> negotiation_round);
> }
> - trace2_region_enter("fetch-pack", "negotiate_using_fetch", the_repository);
> + trace2_region_leave("fetch-pack", "negotiate_using_fetch", the_repository);
> trace2_data_intmax("negotiate_using_fetch", the_repository,
> "total_rounds", negotiation_round);
> clear_common_flag(acked_commits);
>
> base-commit: a26002b62827b89a19b1084bd75d9371d565d03c
^ permalink raw reply
* [PATCH] push: region_leave trace for negotiate_using_fetch
From: Sam Delmerico @ 2024-01-03 22:40 UTC (permalink / raw)
To: git; +Cc: steadmon, Sam Delmerico
There were two region_enter events for negotiate_using_fetch instead of
one enter and one leave. This commit replaces the second region_enter
event with a region_leave.
Signed-off-by: Sam Delmerico <delmerico@google.com>
---
fetch-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 31a72d43de..dba6d79944 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -2218,7 +2218,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
the_repository, "%d",
negotiation_round);
}
- trace2_region_enter("fetch-pack", "negotiate_using_fetch", the_repository);
+ trace2_region_leave("fetch-pack", "negotiate_using_fetch", the_repository);
trace2_data_intmax("negotiate_using_fetch", the_repository,
"total_rounds", negotiation_round);
clear_common_flag(acked_commits);
base-commit: a26002b62827b89a19b1084bd75d9371d565d03c
--
2.43.0.472.g3155946c3a-goog
^ permalink raw reply related
* Re: [PATCH V4 1/1] Replace SID with domain/username
From: Junio C Hamano @ 2024-01-03 22:22 UTC (permalink / raw)
To: Matthias Aßhauer
Cc: Sören Krecker, Johannes Schindelin, git, sunshine
In-Reply-To: <DB9P250MB0692C8B4D93ED92FEE680AA9A560A@DB9P250MB0692.EURP250.PROD.OUTLOOK.COM>
Matthias Aßhauer <mha1993@live.de> writes:
> This patch only changes the output of our error message, though.
> It does not change what ownership information we actually compare.
> So if we had a hypothetical user Bob that was part of the domain
> example.com (SID S-1-5-21-100000001-1000000001-10000001-1001) and
> had been moved over from the example.org domain (old SID S-1-5-21-
> 2000000002-2000000002-20000002-2002) and we would detect a repository
> owned by bobs old SID, we would now lookup the old SID, find it
> attached to a user named example.com\Bob, look up Bobs current SID,
> find it belongs to a user named example.com\Bob and print a confusing
> error message.
Yup, that is exactly the kind of breakage I was worried about.
Perhaps we should do something along the lines of ...
- The erroring out should be done purely by SID comparison, as that
is what we have been doing to protect the users.
- When creating a message, use LookupAccountSidA() to come up with
a pair of domain\user strings for the directory and the process
to be used in the error message:
- If they are different (which is expected to be the normal
case), we just use the pair of strings.
- If they are the same, show old and new SID in stringified form
(hopefully different SIDs would strigify to different
strings?), and optionally we give the domain\user string next
to it.
... then? Then we would emit an error message (in the best case)
'directory' is owned by:
'bob@example.org'
but the current user is:
'charlie@example.com'
and in a bad case we would instead see something like:
'directory' is owned by:
SID S-1-5-21-100000001-1000000001-10000001-1001 ('bob@example.org')
but the current user is:
SID S-1-5-21-200000002-2000000002-20000002-2002 ('bob@example.org')
which may still be serviceable. I dunno.
^ permalink raw reply
* Re: Concurrent fetch commands
From: Junio C Hamano @ 2024-01-03 22:10 UTC (permalink / raw)
To: Taylor Blau; +Cc: Patrick Steinhardt, Oswald Buddenhagen, Stefan Haller, git
In-Reply-To: <ZZWOBObBmLW9Nid6@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> ... I suppose the answer is that they expect
> concurrent fetches to be tolerated, but that the contents of FETCH_HEAD
> (and of course the remote references) are consistent at the end of all
> of the fetches.
What does it mean to be "consistent" in this case, though? For the
controlled form of multiple fetches performed by "git fetch --all",
the answer is probably "as if we fetched sequentially from these
remotes, one by one, and concatenated what these individual fetch
invocations left in FETCH_HEAD". But for an uncontrolled background
fetch IDE and others perform behind user's back, it is unclear what
it means, or for that matter, it is dubious if there is a reasonable
definition for the word.
Folks who invented "git maintenance" designed their "prefetch" task
to perform the best practice, without interfering any foreground
fetches by not touching FETCH_HEAD and the remote-tracking branches.
Nobody brought up the latter so far on this discussion thread, but
mucking with the remote-tracking branches behind user's back means
completely breaking the end-user expectation that --force-with-lease
would do something useful even when it is not given the commit the
user expects to see at the remote. Perhaps those third-party tools
that want to run "git fetch" in the background can learn from how
"prefetch" task works to avoid the breakage they are inflicting on
their users?
^ permalink raw reply
* Re: [PATCH] Documentation: fix statement about rebase.instructionFormat
From: Junio C Hamano @ 2024-01-03 19:22 UTC (permalink / raw)
To: Maarten van der Schrieck via GitGitGadget; +Cc: git, Maarten van der Schrieck
In-Reply-To: <pull.1629.git.git.1704305663254.gitgitgadget@gmail.com>
"Maarten van der Schrieck via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Maarten van der Schrieck <maarten@thingsconnected.nl>
>
> Since commit 62db5247790f2612c0b407a15d1901d88789d35a "rebase -i: generate
> the script via rebase--helper" (Jul 14 2017), the short hash is given in
> rebase-todo. Specifying rebase.instructionFormat does not alter this
> behavior, contrary to what the documentation implies.
>
> Signed-off-by: Maarten van der Schrieck <maarten@thingsconnected.nl>
> ---
Looks good. Will queue. Thanks.
> Documentation: fix statement about rebase.instructionFormat
>
> Since commit 62db5247790f2612c0b407a15d1901d88789d35a "rebase -i:
> generate the script via rebase--helper" (Jul 14 2017), the short hash is
> given in rebase-todo. Specifying rebase.instructionFormat does not alter
> this behavior, contrary to what the documentation implies.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1629%2Fthingsconnected%2Fpullreq1-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1629/thingsconnected/pullreq1-v1
> Pull-Request: https://github.com/git/git/pull/1629
>
> Documentation/config/rebase.txt | 2 +-
> Documentation/git-rebase.txt | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index d59576dbb23..c6187ab28b2 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -40,7 +40,7 @@ rebase.missingCommitsCheck::
> rebase.instructionFormat::
> A format string, as specified in linkgit:git-log[1], to be used for the
> todo list during an interactive rebase. The format will
> - automatically have the long commit hash prepended to the format.
> + automatically have the commit hash prepended to the format.
>
> rebase.abbreviateCommands::
> If set to true, `git rebase` will use abbreviated command names in the
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 1dd6555f66b..25516c45d8b 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -523,7 +523,7 @@ See also INCOMPATIBLE OPTIONS below.
> +
> The commit list format can be changed by setting the configuration option
> rebase.instructionFormat. A customized instruction format will automatically
> -have the long commit hash prepended to the format.
> +have the commit hash prepended to the format.
> +
> See also INCOMPATIBLE OPTIONS below.
>
>
> base-commit: 055bb6e9969085777b7fab83e3fee0017654f134
^ permalink raw reply
* Re: [PATCH v2] write-or-die: make GIT_FLUSH a Boolean environment variable
From: Junio C Hamano @ 2024-01-03 19:18 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Taylor Blau, Patrick Steinhardt, Chandra Pratap via GitGitGadget,
git, Chandra Pratap, Chandra Pratap
In-Reply-To: <20240103184203.GA4334@tb-raspi4>
Torsten Bögershausen <tboegi@web.de> writes:
>> - cp = getenv("GIT_FLUSH");
>> - if (cp)
>> - skip_stdout_flush = (atoi(cp) == 0);
>> - else if ((fstat(fileno(stdout), &st) == 0) &&
>> - S_ISREG(st.st_mode))
>> - skip_stdout_flush = 1;
>> - else
>> - skip_stdout_flush = 0;
>> + skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
>> + if (skip_stdout_flush < 0) {
>> + struct stat st;
>> + if (fstat(fileno(f), &st))
>> + skip_stdout_flush = 0;
>> + else
>> + skip_stdout_flush = S_ISREG(st.st_mode);
>> + }
>> }
>> if (skip_stdout_flush && !ferror(f))
>> return;
>> --- >8 ---
>
> Thanks for a nice reading - I can not imagine a better version.
Yup, the flow of the logic feels very natural with this version by
making it clear that the case that the default "-1" is returned to
us is where we need to figure out an appropriate value ourselves.
An added bonus is that the scope "struct stat" has is limited to the
absolute minimum. I like it, too.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] write-or-die: make GIT_FLUSH a Boolean environment variable
From: Torsten Bögershausen @ 2024-01-03 18:42 UTC (permalink / raw)
To: Taylor Blau
Cc: Patrick Steinhardt, Chandra Pratap via GitGitGadget, git,
Chandra Pratap, Chandra Pratap
In-Reply-To: <ZZWWLkY+ixg+OMM4@nand.local>
On Wed, Jan 03, 2024 at 12:15:26PM -0500, Taylor Blau wrote:
> On Wed, Jan 03, 2024 at 09:22:13AM +0100, Patrick Steinhardt wrote:
> > On Wed, Jan 03, 2024 at 07:58:28AM +0000, Chandra Pratap via GitGitGadget wrote:
> > [snip]
> > > diff --git a/write-or-die.c b/write-or-die.c
> > > index 42a2dc73cd3..a6acabd329f 100644
> > > --- a/write-or-die.c
> > > +++ b/write-or-die.c
> > > @@ -20,15 +20,12 @@ void maybe_flush_or_die(FILE *f, const char *desc)
> > > {
> > > static int skip_stdout_flush = -1;
> > > struct stat st;
> > > - char *cp;
> > >
> > > if (f == stdout) {
> > > if (skip_stdout_flush < 0) {
> > > - /* NEEDSWORK: make this a normal Boolean */
> > > - cp = getenv("GIT_FLUSH");
> > > - if (cp)
> > > - skip_stdout_flush = (atoi(cp) == 0);
> > > - else if ((fstat(fileno(stdout), &st) == 0) &&
> > > + if (!git_env_bool("GIT_FLUSH", -1))
> > > + skip_stdout_flush = 1;
> >
> > It's a bit surprising to pass `-1` as default value to `git_env_bool()`
> > here, as this value would hint that the caller wants to explicitly
> > handle the case where the "GIT_FLUSH" envvar is not set at all. We don't
> > though, and essentially fall back to "GIT_FLUSH=1", so passing `1` as
> > the fallback value would be less confusing.
> >
> > Anyway, the resulting behaviour is the same regardless of whether we
> > pass `1` or `-1`, so I'm not sure whether this is worth a reroll.
>
> Hmm. If we pass -1 as the default value in the call to git_env_bool(),
> the only time we'll end up in the else branch is if the environment is
> set to some false-y value.
>
> I don't think that matches the existing behavior, since right now we'll
> infer skip_stdout_flush based on whether or not stdout is a regular file
> or something else.
>
> I think you'd probably want something closer to:
>
> --- 8< ---
> diff --git a/write-or-die.c b/write-or-die.c
> index 42a2dc73cd..f12e111688 100644
> --- a/write-or-die.c
> +++ b/write-or-die.c
> @@ -19,20 +19,17 @@
> void maybe_flush_or_die(FILE *f, const char *desc)
> {
> static int skip_stdout_flush = -1;
> - struct stat st;
> - char *cp;
>
> if (f == stdout) {
> if (skip_stdout_flush < 0) {
> - /* NEEDSWORK: make this a normal Boolean */
> - cp = getenv("GIT_FLUSH");
> - if (cp)
> - skip_stdout_flush = (atoi(cp) == 0);
> - else if ((fstat(fileno(stdout), &st) == 0) &&
> - S_ISREG(st.st_mode))
> - skip_stdout_flush = 1;
> - else
> - skip_stdout_flush = 0;
> + skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
> + if (skip_stdout_flush < 0) {
> + struct stat st;
> + if (fstat(fileno(f), &st))
> + skip_stdout_flush = 0;
> + else
> + skip_stdout_flush = S_ISREG(st.st_mode);
> + }
> }
> if (skip_stdout_flush && !ferror(f))
> return;
> --- >8 ---
Thanks for a nice reading - I can not imagine a better version.
^ permalink raw reply
* [PATCH] Documentation: fix statement about rebase.instructionFormat
From: Maarten van der Schrieck via GitGitGadget @ 2024-01-03 18:14 UTC (permalink / raw)
To: git; +Cc: Maarten van der Schrieck, Maarten van der Schrieck
From: Maarten van der Schrieck <maarten@thingsconnected.nl>
Since commit 62db5247790f2612c0b407a15d1901d88789d35a "rebase -i: generate
the script via rebase--helper" (Jul 14 2017), the short hash is given in
rebase-todo. Specifying rebase.instructionFormat does not alter this
behavior, contrary to what the documentation implies.
Signed-off-by: Maarten van der Schrieck <maarten@thingsconnected.nl>
---
Documentation: fix statement about rebase.instructionFormat
Since commit 62db5247790f2612c0b407a15d1901d88789d35a "rebase -i:
generate the script via rebase--helper" (Jul 14 2017), the short hash is
given in rebase-todo. Specifying rebase.instructionFormat does not alter
this behavior, contrary to what the documentation implies.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1629%2Fthingsconnected%2Fpullreq1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1629/thingsconnected/pullreq1-v1
Pull-Request: https://github.com/git/git/pull/1629
Documentation/config/rebase.txt | 2 +-
Documentation/git-rebase.txt | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index d59576dbb23..c6187ab28b2 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -40,7 +40,7 @@ rebase.missingCommitsCheck::
rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for the
todo list during an interactive rebase. The format will
- automatically have the long commit hash prepended to the format.
+ automatically have the commit hash prepended to the format.
rebase.abbreviateCommands::
If set to true, `git rebase` will use abbreviated command names in the
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1dd6555f66b..25516c45d8b 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -523,7 +523,7 @@ See also INCOMPATIBLE OPTIONS below.
+
The commit list format can be changed by setting the configuration option
rebase.instructionFormat. A customized instruction format will automatically
-have the long commit hash prepended to the format.
+have the commit hash prepended to the format.
+
See also INCOMPATIBLE OPTIONS below.
base-commit: 055bb6e9969085777b7fab83e3fee0017654f134
--
gitgitgadget
^ permalink raw reply related
* Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)
From: Junio C Hamano @ 2024-01-03 18:08 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Taylor Blau, git
In-Reply-To: <ZZWOtnP2IHNldcy6@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
>> * tb/path-filter-fix (2023-10-18) 17 commits
>> - bloom: introduce `deinit_bloom_filters()`
>> ...
>> - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
>>
>> The Bloom filter used for path limited history traversal was broken
>> on systems whose "char" is unsigned; update the implementation and
>> bump the format version to 2.
>>
>> Expecting a reroll.
>> cf. <20231023202212.GA5470@szeder.dev>
>> source: <cover.1697653929.git.me@ttaylorr.com>
>
> I was confused by this one, since I couldn't figure out which tests
> Gábor was referring to here. I responded in [1], but haven't heard back
> since the end of October.
> ...
> [1]: https://lore.kernel.org/git/ZUARCJ1MmqgXfS4i@nand.local/
OK, let's ping just once then.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Patrick Steinhardt @ 2024-01-03 18:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, Taylor Blau, git, christian.couder
In-Reply-To: <xmqqedey9u32.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]
On Wed, Jan 03, 2024 at 09:59:13AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> I think we should tighten things up over time. First by teaching
> >> the ref backend that anything that is not a pseudoref, HEAD or a
> >> proper ref (one item of whose definition is "lives under refs/
> >> hierarchy) should not resolve_ref() successfully. That should
> >> correctly fail things like
> >>
> >> $ git rev-parse worktrees/$name/bisect/bad
> >> $ git update-ref foo/bar HEAD
> > ...
> > Yeah, agreed, that's something we should do. I do wonder whether this
> > will break existing usecases, but in any case I'd rather consider it an
> > accident that it is possible to write (and read) such refs in the first
> > place.
>
> Unfortunately, the worktrees/$name/refs/bisect/bad and its friends
> are documented in "git worktree" and the refs.c layer is aware of
> the "main-worktree/" and "worktrees/" hierarchy, so while I still
> think it is a good long-term direction to make it impossible to
> create random refs like "foo/bar" and "resf/heads/master" via the
> commands like "git update-ref", we cannot limit ourselves only to
> "refs/" hierarchy.
Ah, I first wanted to point this out, but then noticed that you didn't
include the "refs/" prefix in "worktrees/$name/bisect/bad" and thought
this was intentional. But yes, per-worktree refs need to stay supported,
weird as they may be.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Junio C Hamano @ 2024-01-03 17:59 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, Taylor Blau, git, christian.couder
In-Reply-To: <ZZWbMekL2URby0qV@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> I think we should tighten things up over time. First by teaching
>> the ref backend that anything that is not a pseudoref, HEAD or a
>> proper ref (one item of whose definition is "lives under refs/
>> hierarchy) should not resolve_ref() successfully. That should
>> correctly fail things like
>>
>> $ git rev-parse worktrees/$name/bisect/bad
>> $ git update-ref foo/bar HEAD
> ...
> Yeah, agreed, that's something we should do. I do wonder whether this
> will break existing usecases, but in any case I'd rather consider it an
> accident that it is possible to write (and read) such refs in the first
> place.
Unfortunately, the worktrees/$name/refs/bisect/bad and its friends
are documented in "git worktree" and the refs.c layer is aware of
the "main-worktree/" and "worktrees/" hierarchy, so while I still
think it is a good long-term direction to make it impossible to
create random refs like "foo/bar" and "resf/heads/master" via the
commands like "git update-ref", we cannot limit ourselves only to
"refs/" hierarchy.
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Patrick Steinhardt @ 2024-01-03 17:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, Taylor Blau, git, christian.couder
In-Reply-To: <xmqqil4a9vue.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]
On Wed, Jan 03, 2024 at 09:21:13AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > ... But the problem is that tools like git-update-ref(1) don't
> > enforce this, so something like `git update-ref foo/bar HEAD` happily
> > creates "$GIT_DIR/foo/bar". And I bet there are other ways to write refs
> > at arbitrary paths.
>
> I think we should tighten things up over time. First by teaching
> the ref backend that anything that is not a pseudoref, HEAD or a
> proper ref (one item of whose definition is "lives under refs/
> hierarchy) should not resolve_ref() successfully. That should
> correctly fail things like
>
> $ git rev-parse worktrees/$name/bisect/bad
> $ git update-ref foo/bar HEAD
>
> I'd hope.
>
> Thanks.
Yeah, agreed, that's something we should do. I do wonder whether this
will break existing usecases, but in any case I'd rather consider it an
accident that it is possible to write (and read) such refs in the first
place.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Junio C Hamano @ 2024-01-03 17:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, Taylor Blau, git, christian.couder
In-Reply-To: <ZZWIlx-9D2r9AfDW@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> ... But the problem is that tools like git-update-ref(1) don't
> enforce this, so something like `git update-ref foo/bar HEAD` happily
> creates "$GIT_DIR/foo/bar". And I bet there are other ways to write refs
> at arbitrary paths.
I think we should tighten things up over time. First by teaching
the ref backend that anything that is not a pseudoref, HEAD or a
proper ref (one item of whose definition is "lives under refs/
hierarchy) should not resolve_ref() successfully. That should
correctly fail things like
$ git rev-parse worktrees/$name/bisect/bad
$ git update-ref foo/bar HEAD
I'd hope.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/2] doc: bisect: change plural paths to singular pathspec
From: Taylor Blau @ 2024-01-03 17:17 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git
In-Reply-To: <6bcbd017-968e-4ac8-a56b-164b163c76d4@smtp-relay.sendinblue.com>
On Tue, Jan 02, 2024 at 07:02:05PM -0900, Britton Leo Kerin wrote:
> Britton Leo Kerin (2):
> doc: use singular form of repeatable path arg
> doc: refer to pathspec instead of path
>
> Documentation/git-bisect.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> Range-diff against v1:
> 1: 90c081dcab ! 1: da40e4736b doc: use singular form of repeatable path arg
> @@ Commit message
> later document text mentions 'path' arguments, while it doesn't mention
> 'paths'.
>
> - Signed-off-by: Britton L Kerin <britton.kergin@gmail.com>
> + Signed-off-by: Britton Leo Kerin <britton.kergin@gmail.com>
>
> ## Documentation/git-bisect.txt ##
> @@ Documentation/git-bisect.txt: The command takes various subcommands, and different options depending
> -: ---------- > 2: d932b6d501 doc: refer to pathspec instead of path
> --
> 2.43.0
Hmm. The end-state of these two patches looks good to me, but I probably
would have written this change as a single change from "paths" ->
"pathspec", not "paths" -> "path" -> "pathspec".
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2] write-or-die: make GIT_FLUSH a Boolean environment variable
From: Taylor Blau @ 2024-01-03 17:15 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Chandra Pratap via GitGitGadget, git, Chandra Pratap,
Chandra Pratap
In-Reply-To: <ZZUZNQqDTx3bnveJ@tanuki>
On Wed, Jan 03, 2024 at 09:22:13AM +0100, Patrick Steinhardt wrote:
> On Wed, Jan 03, 2024 at 07:58:28AM +0000, Chandra Pratap via GitGitGadget wrote:
> [snip]
> > diff --git a/write-or-die.c b/write-or-die.c
> > index 42a2dc73cd3..a6acabd329f 100644
> > --- a/write-or-die.c
> > +++ b/write-or-die.c
> > @@ -20,15 +20,12 @@ void maybe_flush_or_die(FILE *f, const char *desc)
> > {
> > static int skip_stdout_flush = -1;
> > struct stat st;
> > - char *cp;
> >
> > if (f == stdout) {
> > if (skip_stdout_flush < 0) {
> > - /* NEEDSWORK: make this a normal Boolean */
> > - cp = getenv("GIT_FLUSH");
> > - if (cp)
> > - skip_stdout_flush = (atoi(cp) == 0);
> > - else if ((fstat(fileno(stdout), &st) == 0) &&
> > + if (!git_env_bool("GIT_FLUSH", -1))
> > + skip_stdout_flush = 1;
>
> It's a bit surprising to pass `-1` as default value to `git_env_bool()`
> here, as this value would hint that the caller wants to explicitly
> handle the case where the "GIT_FLUSH" envvar is not set at all. We don't
> though, and essentially fall back to "GIT_FLUSH=1", so passing `1` as
> the fallback value would be less confusing.
>
> Anyway, the resulting behaviour is the same regardless of whether we
> pass `1` or `-1`, so I'm not sure whether this is worth a reroll.
Hmm. If we pass -1 as the default value in the call to git_env_bool(),
the only time we'll end up in the else branch is if the environment is
set to some false-y value.
I don't think that matches the existing behavior, since right now we'll
infer skip_stdout_flush based on whether or not stdout is a regular file
or something else.
I think you'd probably want something closer to:
--- 8< ---
diff --git a/write-or-die.c b/write-or-die.c
index 42a2dc73cd..f12e111688 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -19,20 +19,17 @@
void maybe_flush_or_die(FILE *f, const char *desc)
{
static int skip_stdout_flush = -1;
- struct stat st;
- char *cp;
if (f == stdout) {
if (skip_stdout_flush < 0) {
- /* NEEDSWORK: make this a normal Boolean */
- cp = getenv("GIT_FLUSH");
- if (cp)
- skip_stdout_flush = (atoi(cp) == 0);
- else if ((fstat(fileno(stdout), &st) == 0) &&
- S_ISREG(st.st_mode))
- skip_stdout_flush = 1;
- else
- skip_stdout_flush = 0;
+ skip_stdout_flush = git_env_bool("GIT_FLUSH", -1);
+ if (skip_stdout_flush < 0) {
+ struct stat st;
+ if (fstat(fileno(f), &st))
+ skip_stdout_flush = 0;
+ else
+ skip_stdout_flush = S_ISREG(st.st_mode);
+ }
}
if (skip_stdout_flush && !ferror(f))
return;
--- >8 ---
You could lose one level of indentation, but it costs an extra fstat()
call in the case where GIT_FLUSH is set to some explicit value. Doing
that would look like this ugly thing instead ;-):
--- 8< ---
diff --git a/write-or-die.c b/write-or-die.c
index 42a2dc73cd..b3275d7577 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -19,20 +19,11 @@
void maybe_flush_or_die(FILE *f, const char *desc)
{
static int skip_stdout_flush = -1;
- struct stat st;
- char *cp;
if (f == stdout) {
if (skip_stdout_flush < 0) {
- /* NEEDSWORK: make this a normal Boolean */
- cp = getenv("GIT_FLUSH");
- if (cp)
- skip_stdout_flush = (atoi(cp) == 0);
- else if ((fstat(fileno(stdout), &st) == 0) &&
- S_ISREG(st.st_mode))
- skip_stdout_flush = 1;
- else
- skip_stdout_flush = 0;
+ struct stat st;
+ skip_stdout_flush = git_env_bool("GIT_FLUSH", !fstat(fileno(f), &st) && S_ISREG(st.st_mode));
}
if (skip_stdout_flush && !ferror(f))
return;
--- >8 ---
Thanks,
Taylor
^ permalink raw reply related
* Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)
From: René Scharfe @ 2024-01-03 17:14 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <20240103090152.GB1866508@coredump.intra.peff.net>
Am 03.01.24 um 10:01 schrieb Jeff King:
> On Tue, Jan 02, 2024 at 05:02:35PM -0800, Junio C Hamano wrote:
>
>> * jk/t1006-cat-file-objectsize-disk (2023-12-21) 1 commit
>> (merged to 'next' on 2023-12-28 at d82812e636)
>> + t1006: add tests for %(objectsize:disk)
>>
>> Test update.
>>
>> Will merge to 'master'.
>> source: <20231221094722.GA570888@coredump.intra.peff.net>
>
> It looks like this is the original version. I posted a v2 that took
> René's suggestion to swap out the awk for shell, but it got overlooked.
> I'm happy enough either way, but if we want to salvage that effort,
> here's a patch which could go on top:
>
> -- >8 --
> From: René Scharfe <l.s.r@web.de>
> Subject: [PATCH] t1006: prefer shell loop to awk for packed object sizes
>
> To compute the expected on-disk size of packed objects, we sort the
> output of show-index by pack offset and then compute the difference
> between adjacent entries using awk. This works but has a few readability
> problems:
>
> 1. Reading the index in pack order means don't find out the size of an
> oid's entry until we see the _next_ entry. So we have to save it to
> print later.
>
> We can instead iterate in reverse order, so we compute each oid's
> size as we see it.
>
> 2. Since the awk invocation is inside a text_expect block, we can't
> easily use single-quotes to hold the script. So we use
> double-quotes, but then have to escape the dollar signs in the awk
> script.
>
> We can swap this out for a shell loop instead (which is made much
> easier by the first change).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I gave René authorship since this was his cleverness, but obviously I
> wrote the commit message. Giving an explicit signoff would be nice,
> though.
Alright, thank you!
Signed-off-by: René Scharfe <l.s.r@web.de>
>
> t/t1006-cat-file.sh | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index 0c2eafae65..5ea3326128 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -1117,14 +1117,16 @@ test_expect_success 'cat-file %(objectsize:disk) with --batch-all-objects' '
> while read idx
> do
> git show-index <"$idx" >idx.raw &&
> - sort -n <idx.raw >idx.sorted &&
> + sort -nr <idx.raw >idx.sorted &&
> packsz=$(test_file_size "${idx%.idx}.pack") &&
> end=$((packsz - rawsz)) &&
> - awk -v end="$end" "
> - NR > 1 { print oid, \$1 - start }
> - { start = \$1; oid = \$2 }
> - END { print oid, end - start }
> - " idx.sorted ||
> + while read start oid rest
> + do
> + size=$((end - start)) &&
> + end=$start &&
> + echo "$oid $size" ||
> + return 1
> + done <idx.sorted ||
> return 1
> done
> } >expect.raw &&
^ permalink raw reply
* Re: [PATCH v2] write-or-die: make GIT_FLUSH a Boolean environment variable
From: Junio C Hamano @ 2024-01-03 17:13 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1628.v2.git.1704268708720.gitgitgadget@gmail.com>
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Documentation/git.txt | 16 +++++++---------
> write-or-die.c | 9 +++------
> 2 files changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 2535a30194f..83fd60f2d11 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -724,16 +724,14 @@ for further details.
> waiting for someone with sufficient permissions to fix it.
>
> `GIT_FLUSH`::
> -// NEEDSWORK: make it into a usual Boolean environment variable
> - If this environment variable is set to "1", then commands such
> + If this Boolean environment variable is set to true, then commands such
> as 'git blame' (in incremental mode), 'git rev-list', 'git log',
> - 'git check-attr' and 'git check-ignore' will
> - force a flush of the output stream after each record have been
> - flushed. If this
> - variable is set to "0", the output of these commands will be done
> - using completely buffered I/O. If this environment variable is
> - not set, Git will choose buffered or record-oriented flushing
> - based on whether stdout appears to be redirected to a file or not.
> + 'git check-attr' and 'git check-ignore' will force a flush of the output
> + stream after each record have been flushed. If this variable is set to
> + false, the output of these commands will be done using completely buffered
> + I/O. If this environment variable is not set, Git will choose buffered or
> + record-oriented flushing based on whether stdout appears to be redirected
> + to a file or not.
It is somewhat irritating to see that we need to change this many
lines to just change "0" to "false" and "1" to "true". I wonder if
it becomes easier to grok if we changed the description into a sub
enumeration of three possibilities, but that would be outside the
scope of this change [*].
> diff --git a/write-or-die.c b/write-or-die.c
> index 42a2dc73cd3..a6acabd329f 100644
> --- a/write-or-die.c
> +++ b/write-or-die.c
> @@ -20,15 +20,12 @@ void maybe_flush_or_die(FILE *f, const char *desc)
> {
> static int skip_stdout_flush = -1;
> struct stat st;
> - char *cp;
>
> if (f == stdout) {
> if (skip_stdout_flush < 0) {
> - /* NEEDSWORK: make this a normal Boolean */
> - cp = getenv("GIT_FLUSH");
> - if (cp)
> - skip_stdout_flush = (atoi(cp) == 0);
> - else if ((fstat(fileno(stdout), &st) == 0) &&
> + if (!git_env_bool("GIT_FLUSH", -1))
> + skip_stdout_flush = 1;
> + else if (!fstat(fileno(stdout), &st) &&
> S_ISREG(st.st_mode))
> skip_stdout_flush = 1;
> else
The above logic does not look correct to me, primarily because the
return value of git_env_bool() is inspected only once to see if it
is zero, and does not differentiate the "unset" case from other
cases.
Since git_env_bool(k, def) returns
- "def" (-1 in this case) when k is not exported (in which case
you need to do the "fstat" dance).
- 0 when k is exported and has a string that is "false" (in
which case you would want to set skip_stdout_flush to true).
- 1 when k is exported and has a string that is "true" (in which
case you would want to set skip_stdout_flush to false).
- or dies if the string exported in k is bogus.
shouldn't it be more like
skip_stdout_flush = 0; /* assume flushing */
switch (git_env_bool("GIT_FLUSH", -1)) {
case 0: /* told not to flush */
skip_stdout_flush = 1;
;;
case -1: /* unspecified */
if (!fstat(...) && S_ISREG())
skip_stdout_flush = 1;
;;
default: /* told to flush */
;;
}
perhaps?
[Footnote]
* If I were to do this change, and if I were to also improve the
style of the documentation before I forget, the way I would do so
probably is with a two-patch series:
(1) update "0" and "1" in the documentation with "false" and
"true", without reflowing the text at all, and update the
code.
(2) rewrite the documentation to use 3-possibility
sub-enumeration for values (imitate the way how other
variables, like `diff.algorithm`, that can choose from a
set of handful possible values are described).
These two changes can be done in either order, but perhaps (1) is
much less controversial change than the other, so I'd probably do
so first.
^ 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