* Re: [PATCH v2] Teach git apply to respect core.fileMode settings
From: Junio C Hamano @ 2023-12-19 22:21 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1620.v2.git.1703010646036.gitgitgadget@gmail.com>
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> When applying a patch that adds an executable file, git apply
> ignores the core.fileMode setting (core.fileMode in git config
> specifies whether the executable bit on files in the working tree
> should be honored or not) resulting in warnings like:
>
> warning: script.sh has type 100644, expected 100755
>
> even when core.fileMode is set to false, which is undesired. This
> is extra true for systems like Windows, which don't rely on "lsat()".
>
> Fix this by inferring the correct file mode from the existing
> index entry when core.filemode is set to false. The added
> test case helps verify the change and prevents future regression.
Thanks. Has _this_ particular iteration of the patch been reviewed
by Dscho? Otherwise ...
> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmail.com>
... this line is a bit problematic. Just double-checking.
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
> index e7a7295f1b6..73fc472b246 100755
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -101,4 +101,29 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
> )
> '
>
> +test_expect_success 'ensure git apply respects core.fileMode' '
> + test_config core.fileMode false &&
> + echo true >script.sh &&
> + git add --chmod=+x script.sh &&
> + git ls-files -s script.sh | grep "^100755" &&
> + test_tick && git commit -m "Add script" &&
> + git ls-tree -r HEAD script.sh | grep "^100755" &&
I am wondering if we want to be more strict about hiding error
return code from "git ls-files" and "git ls-tree" behind pipes
like these. Usually we encourage using a temporary file, e.g.,
...
git ls-files -s script.sh >ls-files-output &&
test_grep "^100755" ls-files-output &&
...
> + echo true >>script.sh &&
> + test_tick && git commit -m "Modify script" script.sh &&
> + git format-patch -1 --stdout >patch &&
> + grep "index.*100755" patch &&
Anchor the pattern when you know where it has to appear and what the
surrounding letters must be, e.g.,
test_grep "^index .* 100755$" patch &&
A test that expects a match with "grep" is silent when it fails, but
if you use test_grep, the output from "sh t4129-*.sh -i -v" gets
easier to view when the test fails, as it is more verbose and say
"we wanted to see X in the output but your output does not have it".
> + git switch -c branch HEAD^ &&
> + git apply --index patch 2>err &&
> + ! grep "has type 100644, expected 100755" err &&
If you wanted to use test_grep here, the way to negate it is a bit
peculiar, i.e.
test_grep ! "has type ..." err &&
It does not have as much value as the positive case, as "! grep"
that expects to fail would show the unexpected match. In any case,
making sure this particular error message does not appear in the
output is a good way to test it, instead of insisting that the
output is empty, since we may add output to the standard error
stream for unrelated reasons to issue warnings, etc.
> + git restore -S script.sh && git restore script.sh &&
Why not "git reset --hard" here? Just being curious why we want to
waste two invocations of "git restore".
> + git apply patch 2>err &&
> + ! grep "has type 100644, expected 100755" err &&
> +
> + git apply --cached patch 2>err &&
> + ! grep "has type 100644, expected 100755" err
> +'
> +
> test_done
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
^ permalink raw reply
* [PATCH v3 2/2] completion: support pseudoref existence checks for reftables
From: Stan Hu @ 2023-12-19 22:14 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Christian Couder, Stan Hu
In-Reply-To: <cover.1703022850.git.stanhu@gmail.com>
In contrib/completion/git-completion.bash, there are a bunch of
instances where we read pseudorefs, such as HEAD, MERGE_HEAD,
REVERT_HEAD, and others via the filesystem. However, the upcoming
reftable refs backend won't use '.git/HEAD' at all but instead will
write an invalid refname as placeholder for backwards compatibility,
which will break the git-completion script.
Update the '__git_pseudoref_exists' function to:
1. Recognize the placeholder '.git/HEAD' written by the reftable
backend (its content is specified in the reftable specs).
2. If reftable is in use, use 'git rev-parse' to determine whether the
given ref exists.
3. Otherwise, continue to use 'test -f' to check for the ref's filename.
Signed-off-by: Stan Hu <stanhu@gmail.com>
---
contrib/completion/git-completion.bash | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8edd002eed..e21a39b406 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -122,12 +122,35 @@ __git ()
${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null
}
+# Helper function to read the first line of a file into a variable.
+# __git_eread requires 2 arguments, the file path and the name of the
+# variable, in that order.
+#
+# This is taken from git-prompt.sh.
+__git_eread ()
+{
+ test -r "$1" && IFS=$'\r\n' read -r "$2" <"$1"
+}
+
# Runs git in $__git_repo_path to determine whether a pseudoref exists.
# 1: The pseudo-ref to search
__git_pseudoref_exists ()
{
local ref=$1
+ # If the reftable is in use, we have to shell out to 'git rev-parse'
+ # to determine whether the ref exists instead of looking directly in
+ # the filesystem to determine whether the ref exists. Otherwise, use
+ # Bash builtins since executing Git commands are expensive on some
+ # platforms.
+ if __git_eread "$__git_repo_path/HEAD" head; then
+ b="${head#ref: }"
+ if [ "$b" == "refs/heads/.invalid" ]; then
+ __git -C "$__git_repo_path" rev-parse --verify --quiet "$ref" 2>/dev/null
+ return $?
+ fi
+ fi
+
[ -f "$__git_repo_path/$ref" ]
}
--
2.42.0
^ permalink raw reply related
* [PATCH v3 1/2] completion: refactor existence checks for pseudorefs
From: Stan Hu @ 2023-12-19 22:14 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Christian Couder, Stan Hu
In-Reply-To: <cover.1703022850.git.stanhu@gmail.com>
In preparation for the reftable backend, this commit introduces a
'__git_pseudoref_exists' function that continues to use 'test -f' to
determine whether a given pseudoref exists in the local filesystem.
Signed-off-by: Stan Hu <stanhu@gmail.com>
---
contrib/completion/git-completion.bash | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 13a39ebd2e..8edd002eed 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -122,6 +122,15 @@ __git ()
${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null
}
+# Runs git in $__git_repo_path to determine whether a pseudoref exists.
+# 1: The pseudo-ref to search
+__git_pseudoref_exists ()
+{
+ local ref=$1
+
+ [ -f "$__git_repo_path/$ref" ]
+}
+
# Removes backslash escaping, single quotes and double quotes from a word,
# stores the result in the variable $dequoted_word.
# 1: The word to dequote.
@@ -1625,7 +1634,7 @@ __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options
_git_cherry_pick ()
{
__git_find_repo_path
- if [ -f "$__git_repo_path"/CHERRY_PICK_HEAD ]; then
+ if __git_pseudoref_exists CHERRY_PICK_HEAD; then
__gitcomp "$__git_cherry_pick_inprogress_options"
return
fi
@@ -2067,7 +2076,7 @@ _git_log ()
__git_find_repo_path
local merge=""
- if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
+ if __git_pseudoref_exists MERGE_HEAD; then
merge="--merge"
fi
case "$prev,$cur" in
@@ -2934,6 +2943,7 @@ _git_reset ()
_git_restore ()
{
+ __git_find_repo_path
case "$prev" in
-s)
__git_complete_refs
@@ -2952,7 +2962,7 @@ _git_restore ()
__gitcomp_builtin restore
;;
*)
- if __git rev-parse --verify --quiet HEAD >/dev/null; then
+ if __git_pseudoref_exists HEAD; then
__git_complete_index_file "--modified"
fi
esac
@@ -2963,7 +2973,7 @@ __git_revert_inprogress_options=$__git_sequencer_inprogress_options
_git_revert ()
{
__git_find_repo_path
- if [ -f "$__git_repo_path"/REVERT_HEAD ]; then
+ if __git_pseudoref_exists REVERT_HEAD; then
__gitcomp "$__git_revert_inprogress_options"
return
fi
@@ -3592,7 +3602,7 @@ __gitk_main ()
__git_find_repo_path
local merge=""
- if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
+ if __git_pseudoref_exists MERGE_HEAD; then
merge="--merge"
fi
case "$cur" in
--
2.42.0
^ permalink raw reply related
* [PATCH v3 0/2] completion: refactor and support reftables backend
From: Stan Hu @ 2023-12-19 22:14 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Christian Couder, Stan Hu
In-Reply-To: <20231130202404.89791-1-stanhu@gmail.com>
This patch series addresses the review feedback:
1. Renames the '__git_ref_exists' helper to '__git_pseudoref_exists' in
the first refactor patch.
Stan Hu (2):
completion: refactor existence checks for pseudorefs
completion: support pseudoref existence checks for reftables
contrib/completion/git-completion.bash | 43 +++++++++++++++++++++++---
1 file changed, 38 insertions(+), 5 deletions(-)
--
2.42.0
^ permalink raw reply
* Re: [PATCH v2] Teach git apply to respect core.fileMode settings
From: Torsten Bögershausen @ 2023-12-19 20:46 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1620.v2.git.1703010646036.gitgitgadget@gmail.com>
On Tue, Dec 19, 2023 at 06:30:45PM +0000, Chandra Pratap via GitGitGadget wrote:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
Thanks for working on this, some small nit-picks inline.
> When applying a patch that adds an executable file, git apply
> ignores the core.fileMode setting (core.fileMode in git config
> specifies whether the executable bit on files in the working tree
> should be honored or not) resulting in warnings like:
>
> warning: script.sh has type 100644, expected 100755
>
> even when core.fileMode is set to false, which is undesired. This
> is extra true for systems like Windows, which don't rely on "lsat()".
Small typo here: lsat() should be lstat(). But being nit-picking (and simplifying):
Windows does not provide an implementation, so Git for Windows does it's own,
which currently does not implement the x-bit(s).
In short: The ', which don't rely on "lsat()' could probably just removed.
>
> Fix this by inferring the correct file mode from the existing
> index entry when core.filemode is set to false. The added
> test case helps verify the change and prevents future regression.
Another nit-pick, sorry for that, I try to convince everybody to not
use "added".
So may be
Add a test case that verifies the change and prevents future regression.
>
> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmail.com>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> apply: make git apply respect core.fileMode settings
>
> Closes issue #1555 on GitHub.
>
[]
>
>
> apply.c | 8 ++++++--
> t/t4129-apply-samemode.sh | 25 +++++++++++++++++++++++++
> 2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 3d69fec836d..58f26c40413 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3778,8 +3778,12 @@ static int check_preimage(struct apply_state *state,
> return error_errno("%s", old_name);
> }
>
> - if (!state->cached && !previous)
> - st_mode = ce_mode_from_stat(*ce, st->st_mode);
> + if (!state->cached && !previous) {
> + if (!trust_executable_bit)
> + st_mode = *ce ? (*ce)->ce_mode : patch->old_mode;
> + else
> + st_mode = ce_mode_from_stat(*ce, st->st_mode);
> + }
>
> if (patch->is_new < 0)
> patch->is_new = 0;
> diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
> index e7a7295f1b6..73fc472b246 100755
> --- a/t/t4129-apply-samemode.sh
> +++ b/t/t4129-apply-samemode.sh
> @@ -101,4 +101,29 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
> )
> '
>
> +test_expect_success 'ensure git apply respects core.fileMode' '
Small nit-pick:
The "ensure" is probably not needed, all tests ensure something.
> + test_config core.fileMode false &&
> + echo true >script.sh &&
> + git add --chmod=+x script.sh &&
> + git ls-files -s script.sh | grep "^100755" &&
> + test_tick && git commit -m "Add script" &&
> + git ls-tree -r HEAD script.sh | grep "^100755" &&
> +
> + echo true >>script.sh &&
> + test_tick && git commit -m "Modify script" script.sh &&
> + git format-patch -1 --stdout >patch &&
> + grep "index.*100755" patch &&
> +
> + git switch -c branch HEAD^ &&
> + git apply --index patch 2>err &&
> + ! grep "has type 100644, expected 100755" err &&
This feels somewhat volatile against future changes.
grep-ing for things that are not there, without verifying
that they are there, without this patch.
On my test system, there is no message at all, so a
test_must_be_empty err &&
feels more "stable".
> + git restore -S script.sh && git restore script.sh &&
> +
> + git apply patch 2>err &&
> + ! grep "has type 100644, expected 100755" err &&
> +
> + git apply --cached patch 2>err &&
> + ! grep "has type 100644, expected 100755" err
> +'
> +
> test_done
^ permalink raw reply
* [PATCH] remote.h: retire CAS_OPT_NAME
From: Junio C Hamano @ 2023-12-19 19:26 UTC (permalink / raw)
To: git
When the "--force-with-lease" option was introduced in 28f5d176
(remote.c: add command line option parser for "--force-with-lease",
2013-07-08), the design discussion revolved around the concept of
"compare-and-swap", and it can still be seen in the name used for
variables and helper functions. The end-user facing option name
ended up to be a bit different, so during the development iteration
of the feature, we used this C preprocessor macro to make it easier
to rename it later.
All of that happened more than 10 years ago, and the flexibility
afforded by the CAS_OPT_NAME macro outlived its usefulness. Inline
the constant string for the option name, like all other option names
in the code.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/push.c | 4 ++--
builtin/send-pack.c | 2 +-
remote.h | 2 --
3 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index dbdf609daf..e740dd93e3 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -393,7 +393,7 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
if (!is_empty_cas(&cas)) {
if (!transport->smart_options)
die("underlying transport does not support --%s option",
- CAS_OPT_NAME);
+ "force-with-lease");
transport->smart_options->cas = &cas;
}
@@ -604,7 +604,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_BIT('n' , "dry-run", &flags, N_("dry run"), TRANSPORT_PUSH_DRY_RUN),
OPT_BIT( 0, "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
- OPT_CALLBACK_F(0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
+ OPT_CALLBACK_F(0, "force-with-lease", &cas, N_("<refname>:<expect>"),
N_("require old value of ref to be at this value"),
PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, parseopt_push_cas_option),
OPT_BIT(0, TRANS_OPT_FORCE_IF_INCLUDES, &flags,
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 4784143004..87e5269e9f 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -207,7 +207,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "stateless-rpc", &stateless_rpc, N_("use stateless RPC protocol")),
OPT_BOOL(0, "stdin", &from_stdin, N_("read refs from stdin")),
OPT_BOOL(0, "helper-status", &helper_status, N_("print status from remote helper")),
- OPT_CALLBACK_F(0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
+ OPT_CALLBACK_F(0, "force-with-lease", &cas, N_("<refname>:<expect>"),
N_("require old value of ref to be at this value"),
PARSE_OPT_OPTARG, parseopt_push_cas_option),
OPT_BOOL(0, TRANS_OPT_FORCE_IF_INCLUDES, &force_if_includes,
diff --git a/remote.h b/remote.h
index 73638cefeb..9ba7f7d3e2 100644
--- a/remote.h
+++ b/remote.h
@@ -398,8 +398,6 @@ struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map);
/*
* Compare-and-swap
*/
-#define CAS_OPT_NAME "force-with-lease"
-
struct push_cas_option {
unsigned use_tracking_for_rest:1;
unsigned use_force_if_includes:1;
--
2.43.0-121-g624eb90fa8
^ permalink raw reply related
* [PATCH v2] Teach git apply to respect core.fileMode settings
From: Chandra Pratap via GitGitGadget @ 2023-12-19 18:30 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1620.git.1702908568890.gitgitgadget@gmail.com>
From: Chandra Pratap <chandrapratap3519@gmail.com>
When applying a patch that adds an executable file, git apply
ignores the core.fileMode setting (core.fileMode in git config
specifies whether the executable bit on files in the working tree
should be honored or not) resulting in warnings like:
warning: script.sh has type 100644, expected 100755
even when core.fileMode is set to false, which is undesired. This
is extra true for systems like Windows, which don't rely on "lsat()".
Fix this by inferring the correct file mode from the existing
index entry when core.filemode is set to false. The added
test case helps verify the change and prevents future regression.
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmail.com>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
apply: make git apply respect core.fileMode settings
Closes issue #1555 on GitHub.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1620%2FChand-ra%2Fdevel-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1620/Chand-ra/devel-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1620
Range-diff vs v1:
1: d896351158c ! 1: 29c8c6d120e Teach git apply to respect core.fileMode settings
@@ Metadata
## Commit message ##
Teach git apply to respect core.fileMode settings
- CC: Johannes Schindelin <johannes.schindelin@gmail.com>
+ When applying a patch that adds an executable file, git apply
+ ignores the core.fileMode setting (core.fileMode in git config
+ specifies whether the executable bit on files in the working tree
+ should be honored or not) resulting in warnings like:
+
+ warning: script.sh has type 100644, expected 100755
+
+ even when core.fileMode is set to false, which is undesired. This
+ is extra true for systems like Windows, which don't rely on "lsat()".
+
+ Fix this by inferring the correct file mode from the existing
+ index entry when core.filemode is set to false. The added
+ test case helps verify the change and prevents future regression.
+
+ Reviewed-by: Johannes Schindelin <johannes.schindelin@gmail.com>
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
## apply.c ##
@@ apply.c: static int check_preimage(struct apply_state *state,
- if (!state->cached && !previous)
- st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ if (!state->cached && !previous) {
-+ if (!trust_executable_bit && patch->old_mode)
-+ st_mode = patch->old_mode;
-+ else st_mode = ce_mode_from_stat(*ce, st->st_mode);
++ if (!trust_executable_bit)
++ st_mode = *ce ? (*ce)->ce_mode : patch->old_mode;
++ else
++ st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ }
if (patch->is_new < 0)
@@ t/t4129-apply-samemode.sh: test_expect_success POSIXPERM 'do not use core.shared
)
'
-+test_expect_success FILEMODE 'ensure git apply respects core.fileMode' '
++test_expect_success 'ensure git apply respects core.fileMode' '
+ test_config core.fileMode false &&
+ echo true >script.sh &&
+ git add --chmod=+x script.sh &&
++ git ls-files -s script.sh | grep "^100755" &&
+ test_tick && git commit -m "Add script" &&
++ git ls-tree -r HEAD script.sh | grep "^100755" &&
+
+ echo true >>script.sh &&
+ test_tick && git commit -m "Modify script" script.sh &&
+ git format-patch -1 --stdout >patch &&
++ grep "index.*100755" patch &&
+
+ git switch -c branch HEAD^ &&
++ git apply --index patch 2>err &&
++ ! grep "has type 100644, expected 100755" err &&
++ git restore -S script.sh && git restore script.sh &&
++
+ git apply patch 2>err &&
-+ ! test_grep "has type 100644, expected 100755" err
++ ! grep "has type 100644, expected 100755" err &&
++
++ git apply --cached patch 2>err &&
++ ! grep "has type 100644, expected 100755" err
+'
+
test_done
apply.c | 8 ++++++--
t/t4129-apply-samemode.sh | 25 +++++++++++++++++++++++++
2 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/apply.c b/apply.c
index 3d69fec836d..58f26c40413 100644
--- a/apply.c
+++ b/apply.c
@@ -3778,8 +3778,12 @@ static int check_preimage(struct apply_state *state,
return error_errno("%s", old_name);
}
- if (!state->cached && !previous)
- st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ if (!state->cached && !previous) {
+ if (!trust_executable_bit)
+ st_mode = *ce ? (*ce)->ce_mode : patch->old_mode;
+ else
+ st_mode = ce_mode_from_stat(*ce, st->st_mode);
+ }
if (patch->is_new < 0)
patch->is_new = 0;
diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh
index e7a7295f1b6..73fc472b246 100755
--- a/t/t4129-apply-samemode.sh
+++ b/t/t4129-apply-samemode.sh
@@ -101,4 +101,29 @@ test_expect_success POSIXPERM 'do not use core.sharedRepository for working tree
)
'
+test_expect_success 'ensure git apply respects core.fileMode' '
+ test_config core.fileMode false &&
+ echo true >script.sh &&
+ git add --chmod=+x script.sh &&
+ git ls-files -s script.sh | grep "^100755" &&
+ test_tick && git commit -m "Add script" &&
+ git ls-tree -r HEAD script.sh | grep "^100755" &&
+
+ echo true >>script.sh &&
+ test_tick && git commit -m "Modify script" script.sh &&
+ git format-patch -1 --stdout >patch &&
+ grep "index.*100755" patch &&
+
+ git switch -c branch HEAD^ &&
+ git apply --index patch 2>err &&
+ ! grep "has type 100644, expected 100755" err &&
+ git restore -S script.sh && git restore script.sh &&
+
+ git apply patch 2>err &&
+ ! grep "has type 100644, expected 100755" err &&
+
+ git apply --cached patch 2>err &&
+ ! grep "has type 100644, expected 100755" err
+'
+
test_done
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
gitgitgadget
^ permalink raw reply related
* Re: Is --minimal ever not the right thing?
From: Konstantin Tokarev @ 2023-12-19 18:09 UTC (permalink / raw)
To: Elijah Newren; +Cc: Mike Castle, Tao Klerks, git
In-Reply-To: <CABPp-BEmgOAj17DozyXNaf-9CawDic4uTpMbckef3+zHf7URqQ@mail.gmail.com>
On Tue, 19 Dec 2023 09:55:34 -0800
Elijah Newren <newren@gmail.com> wrote:
> minimal is guaranteed to produce a minimal diff, i.e. fewest total
> subtractions and additions. That is sometimes "best" quality, but
> definitely not always.
I second this. Recently I had a case when I had to use --anchored
option of git diff to produce more informative diff instead of minimal
one.
^ permalink raw reply
* Re: Is --minimal ever not the right thing?
From: Elijah Newren @ 2023-12-19 17:55 UTC (permalink / raw)
To: Mike Castle; +Cc: Tao Klerks, git
In-Reply-To: <CA+t9iMyrLAekwQHNky4w9nWD6WwxidxwfSmbqCpSRnkJgoQ0LA@mail.gmail.com>
To add to what Mike said...
On Tue, Dec 19, 2023 at 9:25 AM Mike Castle <dalgoda@gmail.com> wrote:
>
> I believe that the diff algorithms available are the same one's in GNU
> diff. From https://www.gnu.org/software/diffutils/manual/html_node/diff-Performance.html:
> """
> The way that GNU diff determines which lines have changed always comes
> up with a near-minimal set of differences. Usually it is good enough
> for practical purposes. If the diff output is large, you might want
> diff to use a modified algorithm that sometimes produces a smaller set
> of differences. The --minimal (-d) option does this; however, it can
> also cause diff to run more slowly than usual, so it is not the
> default behavior.
> """
>
> Since it has been that way decades before git even existed, I suspect
> (but do not know) that, yes, analysis has been performed, and it makes
> sense to keep the current default.
>
> Then again, in the decades sense, the entire stack from hardware to
> compilers has improved, and maybe it does deserve a revisit. You
> could check whatever email archives is used for diffutils and see if
> there has been any discussion on it recently (say, last 5 years?).
>
> As you pointed out, you can set it yourself and see what happens over time.
There have been various discussions of diff performance, quality of
results, what the default should be, etc. Including within the last
year.
minimal is guaranteed to produce a minimal diff, i.e. fewest total
subtractions and additions. That is sometimes "best" quality, but
definitely not always. On the performance axis, in special cases
minimal can be nearly as fast as myers and the other diff algorithms,
but only in special cases.
I think patience or histogram would make better defaults, at least
with some tweaks. I had some patches to improve some worst case
performance and quality results coming from histogram that I was
working on in early 2023, but those got put on the backburner when
$DAYJOB pulled support for my Git work. And I'm not aware of anyone
else currently working in the area.
Hope that helps,
Elijah
^ permalink raw reply
* Re: git diff-files bug
From: Elijah Newren @ 2023-12-19 17:47 UTC (permalink / raw)
To: Josh Reed; +Cc: git
In-Reply-To: <CAELOy+5AsRyLEs-WdYw1spqkmMDKjKSQzbogAoRBFe-zGLjvXg@mail.gmail.com>
Hi,
On Tue, Dec 19, 2023 at 8:56 AM Josh Reed <jriddy@gmail.com> wrote:
>
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
Let's extend your example a bit to point out when/if the index is updated...
>
> > bash
> ⬢[jreed@toolbox example-project]$ git diff-files --exit-code --patch; echo $?
> 0
$ stat --format=%y .git/index
2023-12-19 09:30:55.867055501 -0800
This is the modification time of the index at the very beginning.
> ⬢[jreed@toolbox example-project]$ chmod g+w README.md
$ stat --format=%y .git/index
2023-12-19 09:30:55.867055501 -0800
As expected, the index has not changed just because some file changed.
> ⬢[jreed@toolbox example-project]$ git diff-files --exit-code --patch; echo $?
> 1
$ stat --format=%y .git/index
2023-12-19 09:30:55.867055501 -0800
Note that the index still has not changed; diff-files will not update
it when it notices differences. But diff-files shows us differences
between the working tree and index, and the working tree has changed.
Those changes mean that the stat information in the index no longer
matches the working tree. That alone is enough for diff-files to give
a non-zero exit status. This does tend to trip up folks, though.
> ⬢[jreed@toolbox example-project]$ git diff --exit-code --patch; echo $?
> 0
$ stat --format=%y .git/index
2023-12-19 09:33:46.773896615 -0800
Now the index _was_ updated. So, not only are there no content
differences, and no relevant filemode differences, but the stat
information in the index now matches what is in the working tree. So,
diff returns an exit code of 0. (Note that this means "git diff" is
not a read-only operation, which sometimes trips up people in the
other direction.)
> ⬢[jreed@toolbox example-project]$ git diff-files --exit-code --patch; echo $?
> 0
Right, contents match, relelvant filemodes match, and the stat
information in the index now matches what is in the working tree.
And:
$ stat --format=%y .git/index
2023-12-19 09:33:46.773896615 -0800
...no further changes to the index from this command, of course.
> What did you expect to happen? (Expected behavior)
>
> Git diff-files should likely ignore group permissions changes, or at least
> its output should be stable across the same worktree/index state.
It does ignore group permission changes; the problem was that stat
information in the index did not match what was in the working tree.
If you run
$ git update-index --refresh
before calling diff-files, that'll make sure the stat information is up-to-date.
> What happened instead? (Actual behavior)
>
> The command `git diff-files --exit-code --patch` fails with no exit code when
> a file mode has changed in a way that the rest of git commands ignored.
> Furthermore, subsequent git commands seem to change the behavior of `git
> diff-files` in this regard, so it's hard to tell what is the expected behavior.
>
> What's different between what you expected and what actually happened?
>
> The `git diff-files --exit-code` command is inconsistent in how it behaves.
> I suspect it should ignore irrelavant mode changes like `g+w`, but even if
> it should report them, they should produce a patch or at least have stable
> results when we re-run the command.
This has bit a few folks before; I wonder if there's a reasonable
update we can make to the documentation to address this.
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2023, #03; Mon, 18)
From: Junio C Hamano @ 2023-12-19 17:30 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <3e6c7e70-33c2-4607-b9a3-8d70d2a83ff5@web.de>
René Scharfe <l.s.r@web.de> writes:
> Am 19.12.23 um 02:06 schrieb Junio C Hamano:
>> * rs/t6300-compressed-size-fix (2023-12-13) 2 commits
>> - test-lib-functions: add object size functions
>> - t6300: avoid hard-coding object sizes
>>
>> Test fix.
>>
>> Will merge to 'next'?
>> source: <9feeb6cf-aabf-4002-917f-3f6c27547bc8@web.de>
>> source: <ff735aac-b60b-4d52-a6dc-180ab504fc8d@web.de>
>
> The first patch is good to go. The seconds one isn't; please drop it.
Alright. Thanks. Will do.
^ permalink raw reply
* Re: Is --minimal ever not the right thing?
From: Mike Castle @ 2023-12-19 17:25 UTC (permalink / raw)
To: Tao Klerks; +Cc: git
In-Reply-To: <CAPMMpohbQK+3o46iiY+0o=vS+UC_HBB=CxsNT_hAb5dDz+514Q@mail.gmail.com>
I believe that the diff algorithms available are the same one's in GNU
diff. From https://www.gnu.org/software/diffutils/manual/html_node/diff-Performance.html:
"""
The way that GNU diff determines which lines have changed always comes
up with a near-minimal set of differences. Usually it is good enough
for practical purposes. If the diff output is large, you might want
diff to use a modified algorithm that sometimes produces a smaller set
of differences. The --minimal (-d) option does this; however, it can
also cause diff to run more slowly than usual, so it is not the
default behavior.
"""
Since it has been that way decades before git even existed, I suspect
(but do not know) that, yes, analysis has been performed, and it makes
sense to keep the current default.
Then again, in the decades sense, the entire stack from hardware to
compilers has improved, and maybe it does deserve a revisit. You
could check whatever email archives is used for diffutils and see if
there has been any discussion on it recently (say, last 5 years?).
As you pointed out, you can set it yourself and see what happens over time.
Cheers,
mrc
^ permalink raw reply
* Re: [PATCH] rebase: use strvec_pushf() for format-patch revisions
From: Junio C Hamano @ 2023-12-19 17:12 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <4ab7431c-6c1b-448c-b4d2-e8b9be0e4eef@web.de>
René Scharfe <l.s.r@web.de> writes:
> In run_am(), a strbuf is used to create a revision argument that is then
> added to the argument list for git format-patch using strvec_push().
> Use strvec_pushf() to add it directly instead, simplifying the code.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
Makes sense. Between the location of the original strbuf_addf()
call and the new strvec_pushf() call, nobody mucks with *opts so
this change won't affect the correctness. We no longer use the
extra strbuf, and upon failing to open the rebased-patches file,
we no longer leak the contents of it. Good.
> @@ -615,34 +614,32 @@ static int run_am(struct rebase_options *opts)
> return run_command(&am);
> }
>
> - strbuf_addf(&revisions, "%s...%s",
> - oid_to_hex(opts->root ?
> - /* this is now equivalent to !opts->upstream */
> - &opts->onto->object.oid :
> - &opts->upstream->object.oid),
> - oid_to_hex(&opts->orig_head->object.oid));
> -
> rebased_patches = xstrdup(git_path("rebased-patches"));
> format_patch.out = open(rebased_patches,
> O_WRONLY | O_CREAT | O_TRUNC, 0666);
> if (format_patch.out < 0) {
> status = error_errno(_("could not open '%s' for writing"),
> rebased_patches);
> free(rebased_patches);
> strvec_clear(&am.args);
> return status;
> }
>
> format_patch.git_cmd = 1;
> strvec_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
> "--full-index", "--cherry-pick", "--right-only",
> "--default-prefix", "--no-renames",
> "--no-cover-letter", "--pretty=mboxrd", "--topo-order",
> "--no-base", NULL);
> if (opts->git_format_patch_opt.len)
> strvec_split(&format_patch.args,
> opts->git_format_patch_opt.buf);
> - strvec_push(&format_patch.args, revisions.buf);
> + strvec_pushf(&format_patch.args, "%s...%s",
> + oid_to_hex(opts->root ?
> + /* this is now equivalent to !opts->upstream */
> + &opts->onto->object.oid :
> + &opts->upstream->object.oid),
> + oid_to_hex(&opts->orig_head->object.oid));
> if (opts->restrict_revision)
> strvec_pushf(&format_patch.args, "^%s",
> oid_to_hex(&opts->restrict_revision->object.oid));
> @@ -665,10 +662,8 @@ static int run_am(struct rebase_options *opts)
> "As a result, git cannot rebase them."),
> opts->revisions);
>
> - strbuf_release(&revisions);
> return status;
> }
> - strbuf_release(&revisions);
>
> am.in = open(rebased_patches, O_RDONLY);
> if (am.in < 0) {
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH] Teach git apply to respect core.fileMode settings
From: Chandra Pratap @ 2023-12-19 17:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Chandra Pratap via GitGitGadget, git
In-Reply-To: <xmqqbkanfkjy.fsf@gitster.g>
Thanks for the review, Junio. I have considered your feedback and
will adjust the patch as such. The mail formatting issues seem to
have arisen from my invigilant use of GitGitGadget.
> Junio C Hamano <gitster@pobox.com> writes:
>
> IOW, I am wondering if the above should look more like
>
> if (!state->cached && !previous) {
> if (!trust_executable_bit) {
> if (*ce)
> st_mode = (*ce)->ce_mode;
> else
> st_mode = patch->old_mode;
> } else {
> st_mode = ce_mode_from_stat(*ce, st->st_mode);
> }
> }
You're right, we should prioritise the file mode info from the
existing cache entry (if one exists) instead of blindly assigning
the one from the incoming patch. It's more robust that way.
> Perhaps we would want to check with "git ls-files -s script.sh" what
> its mode bits are (hopefully it would be executable).
>
> Similarly, check with "git ls-tree -r HEAD script.sh" what its mode>
> bits are?
>
> Check that the patch expects script.sh to have its executable bit
> here, too?
I assume we're doing all this filemode checking to ensure that the
executable bit doesn't get lost due to any other git command?
> The code change in this patch is primarily about making the code
> work better for folks without trustworthy filemode support.
> Emulating what happens by setting core.fileMode to false on a
> platform with capable filesystems may be a way to test the code, but
> we should have a test specific to folks without FILEMODE
> prerequisites and make sure it works well, no?
>
> IOW, shouldn't we drom FILEMODE prerequisite from this test?
I will keep that in mind.
^ permalink raw reply
* git diff-files bug
From: Josh Reed @ 2023-12-19 16:53 UTC (permalink / raw)
To: git
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
> bash
⬢[jreed@toolbox example-project]$ git diff-files --exit-code --patch; echo $?
0
⬢[jreed@toolbox example-project]$ chmod g+w README.md
⬢[jreed@toolbox example-project]$ git diff-files --exit-code --patch; echo $?
1
⬢[jreed@toolbox example-project]$ git diff --exit-code --patch; echo $?
0
⬢[jreed@toolbox example-project]$ git diff-files --exit-code --patch; echo $?
0
What did you expect to happen? (Expected behavior)
Git diff-files should likely ignore group permissions changes, or at least
its output should be stable across the same worktree/index state.
What happened instead? (Actual behavior)
The command `git diff-files --exit-code --patch` fails with no exit code when
a file mode has changed in a way that the rest of git commands ignored.
Furthermore, subsequent git commands seem to change the behavior of `git
diff-files` in this regard, so it's hard to tell what is the expected behavior.
What's different between what you expected and what actually happened?
The `git diff-files --exit-code` command is inconsistent in how it behaves.
I suspect it should ignore irrelavant mode changes like `g+w`, but even if
it should report them, they should produce a patch or at least have stable
results when we re-run the command.
Anything else you want to add:
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.41.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.10-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Nov 2
19:59:55 UTC 2023 x86_64
compiler info: gnuc: 13.1
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /usr/bin/fish
[Enabled Hooks]
pre-commit
pre-push
^ permalink raw reply
* Re: [PATCH 2/1] test-lib-functions: add object size functions
From: René Scharfe @ 2023-12-19 16:42 UTC (permalink / raw)
To: Jeff King; +Cc: git, Ondrej Pohorelsky, brian m . carlson, Junio C Hamano
In-Reply-To: <20231214205936.GA2272813@coredump.intra.peff.net>
Am 14.12.23 um 21:59 schrieb Jeff King:
> On Wed, Dec 13, 2023 at 01:28:56PM +0100, René Scharfe wrote:
>
>> Add test_object_size and its helpers test_loose_object_size and
>> test_packed_object_size, which allow determining the size of a Git
>> object using only the low-level Git commands rev-parse and show-index.
>>
>> Use it in t6300 to replace the bare-bones function test_object_file_size
>> as a motivating example. There it provides the expected output of the
>> high-level Git command for-each-ref.
>
> This adds a packed-object function, but I doubt anybody actually calls
> it. If we're going to do that, it's probably worth adding some tests for
> "cat-file --batch-check" or similar.
Yes, and I was assuming that someone else would be eager to add such
tests. *ahem*
> At which point I wonder if rather than having a function for a single
> object, we are better off just testing the result of:
>
> git cat-file --batch-all-objects --unordered --batch-check='%(objectsize:disk)'
>
> against a single post-processed "show-index" invocation.
Sure, we might want to optimize for bulk-processing and possibly end up
only checking the size of single objects in t6300, making new library
functions unnecessary.
When dumping size information of multiple objects, it's probably a good
idea to include "%(objectname)" as well in the format.
You'd need one show-index call for each .idx file. A simple test would
only have a single one; a library function might need to handle multiple
packs.
>> So how about this? I'm a bit nervous about all the rules about output
>> descriptors and error propagation and whatnot in the test library, but
>> this implementation seems simple enough and might be useful in more than
>> one test. No idea how to add support for alternate object directories,
>> but I doubt we'll ever need it.
>
> I'm not sure that we need to do anything special with output
> redirection. Shouldn't these functions just send errors to stderr as
> usual? If they are run inside a test_expect block, that goes to
> descriptor 4 (which is either /dev/null or the original stderr,
> depending on whether "-v" was used).
Good point. My bad excuse is that I copied the redirection to fd 4 from
test_grep.
>> + git show-index <"$idx" |
>> + awk -v oid="$oid" -v end="$end" '
>> + $2 == oid {start = $1}
>> + {offsets[$1] = 1}
>> + END {
>> + if (!start || start >= end)
>> + exit 1
>> + for (o in offsets)
>> + if (start < o && o < end)
>> + end = o
>> + print end - start
>> + }
>> + ' && return 0
>
> I was confused at first, because I didn't see any sorting happening. But
> if I understand correctly, you're just looking for the smallest "end"
> that comes after the start of the object we're looking for. Which I
> think works.
Yes, calculating the minimum offset suffices when handling a single
object -- no sorting required. For bulk mode we'd better sort, of
course:
git show-index <"$idx" |
sort -n |
awk -v end="$end" '
NR > 1 {print oid, $1 - start}
{start = $1; oid = $2}
END {print oid, end - start}
'
No idea how to make such a thing robust against malformed or truncated
output from show-index, but perhaps that's not necessary, depending on
how the result is used.
René
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2023, #03; Mon, 18)
From: René Scharfe @ 2023-12-19 16:42 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqr0jjc86e.fsf@gitster.g>
Am 19.12.23 um 02:06 schrieb Junio C Hamano:
> * rs/t6300-compressed-size-fix (2023-12-13) 2 commits
> - test-lib-functions: add object size functions
> - t6300: avoid hard-coding object sizes
>
> Test fix.
>
> Will merge to 'next'?
> source: <9feeb6cf-aabf-4002-917f-3f6c27547bc8@web.de>
> source: <ff735aac-b60b-4d52-a6dc-180ab504fc8d@web.de>
The first patch is good to go. The seconds one isn't; please drop it.
René
^ permalink raw reply
* Is --minimal ever not the right thing?
From: Tao Klerks @ 2023-12-19 16:10 UTC (permalink / raw)
To: git
Hi folks,
A user today showed me a situation where `git diff` (and `git blame`)
seemed to be doing the wrong thing: where two big blocks of text were
removed from a file, leaving 4 lines untouched in the middle, the
default diff was noting all three regions as lines removed, with those
4 "untouched" lines as *added* in the same place.
We compared to another diffing tool, p4merge, and that was showing
"the right thing" - two deleted regions with untouched lines in the
middle.
We realized that `--minimal` does "the right thing" in git, and you
can set up `diff.algorithm` config to use it by default in `git diff`
(although `git blame` doesn't currently/yet support it... a small
enhancement opportunity there :) ), but that raises two questions:
1. Is there any practical reason for any user *not* to set
`diff.algorithm` to `minimal`? Has anyone ever done an analysis of the
performance cost (or "diff readability cost", if that is a thing) of
"minimal" vs "default"?
2. If "minimal" is just better, and its higher computational cost is
effectively trivial, then why wouldn't we change the default?
I suspect this comes down to situations where git does big diffs
behind the scenes...? But I don't know offhand.
Any feedback would be most appreciated!
Thanks,
Tao
^ permalink raw reply
* Re: [PATCH] doc: format.notes specify a ref under refs/notes/ hierarchy
From: Jiang Xin @ 2023-12-19 15:33 UTC (permalink / raw)
To: Junio C Hamano, Teng Long, Jean-Noël Avila, Peter Pan
Cc: Patrick Steinhardt, git, Ramsay Jones
In-Reply-To: <xmqq1qbjij0f.fsf@gitster.g>
On Tue, Dec 19, 2023 at 12:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
> > the translation changes with a big grain of salt though,
>
> Hopefully pinging Jiang would be sufficient to ask help from the
> French, Chinese, and Taiwaneese translation teams.
The l10n team has a command line tool called git-po-helper that can
check for spelling errors in translations, such as mismatched command
names, configuration variables. A new pattern will be added to find
mismatched reference prefixes, and the following typos will be fixed
during the next localization window.
> > diff --git a/po/fr.po b/po/fr.po
> > index ee2e610ef1..744550b056 100644
> > --- a/po/fr.po
> > +++ b/po/fr.po
> > @@ -19773,7 +19773,7 @@ msgid ""
> > "Neither worked, so we gave up. You must fully qualify the ref."
> > msgstr ""
> > "La destination que vous avez fournie n'est pas un nom de référence complète\n"
> > -"(c'est-à-dire commençant par \"ref/\"). Essai d'approximation par :\n"
> > +"(c'est-à-dire commençant par \"refs/\"). Essai d'approximation par :\n"
> > "\n"
> > "- Recherche d'une référence qui correspond à '%s' sur le serveur distant.\n"
> > "- Vérification si la <source> en cours de poussée ('%s')\n"
> > diff --git a/po/zh_CN.po b/po/zh_CN.po
> > index 86402725b2..eb47e8f9b7 100644
> > --- a/po/zh_CN.po
> > +++ b/po/zh_CN.po
> > @@ -13224,8 +13224,8 @@ msgid ""
> > msgid_plural ""
> > "Note: Some branches outside the refs/remotes/ hierarchy were not removed;\n"
> > "to delete them, use:"
> > -msgstr[0] "注意：ref/remotes 层级之外的一个分支未被移除。要删除它，使用："
> > -msgstr[1] "注意：ref/remotes 层级之外的一些分支未被移除。要删除它们，使用："
> > +msgstr[0] "注意：refs/remotes 层级之外的一个分支未被移除。要删除它，使用："
> > +msgstr[1] "注意：refs/remotes 层级之外的一些分支未被移除。要删除它们，使用："
> >
> > #: builtin/remote.c
> > #, c-format
> > diff --git a/po/zh_TW.po b/po/zh_TW.po
> > index f777a0596f..b2a79cdd93 100644
> > --- a/po/zh_TW.po
> > +++ b/po/zh_TW.po
> > @@ -13109,7 +13109,7 @@ msgid ""
> > msgid_plural ""
> > "Note: Some branches outside the refs/remotes/ hierarchy were not removed;\n"
> > "to delete them, use:"
> > -msgstr[0] "注意：ref/remotes 層級之外的一個分支未被移除。要刪除它，使用："
> > +msgstr[0] "注意：refs/remotes 層級之外的一個分支未被移除。要刪除它，使用："
> >
> > #: builtin/remote.c
> > #, c-format
^ permalink raw reply
* Re: [PATCH 6/8] SubmittingPatches: clarify GitHub visual
From: René Scharfe @ 2023-12-19 14:44 UTC (permalink / raw)
To: Josh Soref via GitGitGadget, git; +Cc: Elijah Newren, Josh Soref
In-Reply-To: <043d2a24202d39c5564e4a4369c86ae4648dd721.1702975320.git.gitgitgadget@gmail.com>
Am 19.12.23 um 09:41 schrieb Josh Soref via GitGitGadget:
> From: Josh Soref <jsoref@gmail.com>
>
> Some people would expect a cross to be upright, and potentially have
> unequal lengths...
There are lots of types of crosses. And while looking them up on
Wikipedia I learned today that an x-cross is called "saltire" in
English. I only knew it as St. Andrew's cross before.
> GitHub uses a white x overlaying a solid red circle to indicate failure.
They call it "x-circle-fill"
(https://primer.github.io/octicons/x-circle-fill-16).
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> Documentation/SubmittingPatches | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index d7a84f59478..8e19c7f82e4 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -604,7 +604,7 @@ to your fork of Git on GitHub. You can monitor the test state of all your
> branches here: `https://github.com/<Your GitHub handle>/git/actions/workflows/main.yml`
>
> If a branch did not pass all test cases then it is marked with a red
> -cross. In that case you can click on the failing job and navigate to
> ++x+. In that case you can click on the failing job and navigate to
In the commit message you say the x is white, here it's red, so what is
it? IIUC the circle is red and the x-cross inside is the same color as
the background, i.e. white in light mode and black in dark mode. No
idea how to express that in one word. Perhaps "red circle containing
and x-cross"?
René
^ permalink raw reply
* Problem with commit-graph verify
From: Anton Sergeev @ 2023-12-19 13:39 UTC (permalink / raw)
To: git
Hi folks,
I have a problem with 'commit-graph verify' in poco repository ([1]).
A commit appeared there with an odd timestamp and time zone ([2]):
git show --no-patch --pretty=%ai
381ac1d9a82c9682a5046dd51802a687a81ace91
# 2106-02-07 06:28:18 -11309508
The main problem is that the 'commit-graph verify' return error:
git commit-graph write
git commit-graph verify
# commit-graph generation for commit
1763a5017d8c0a9af6094fde91c43a5722bbde4c is 1699836629 < 4702109779
# Verifying commits in commit graph: 100% (9489/9489), done.
echo $?
# 1
And this results in an error on fsck:
git fsck
# ...
# error in commit 381ac1d9a82c9682a5046dd51802a687a81ace91:
badTimezone: invalid author/committer line - bad time zone
# ...
# commit-graph generation for commit
1763a5017d8c0a9af6094fde91c43a5722bbde4c is 1699836629 < 4702109779
# ...
echo $?
# 20
I found that first error can be masked using 'fsck.skiplist' file. But
can't find how to mask the second.
Is there a workaround for this case?
System info:
* git version: 2.43.0
* OS: Debian GNU/Linux 11 (bullseye), x86_64
Notes:
* This error originally occurred on a local GitLab installation, that
periodically run fsck on all repos. And the poco repo mirror in our
GitLab instance is now marked as failed.
* Another strange thing about this commit is that git can't find any
belonging branch for it, but parent and child commits are has ones:
git log --pretty=format:"%h %ad | %s%d [%an]" --graph --date=short
-n10 4261-move-autocommit-abstractsession
# ac7e39ff8 2023-11-14 | Fixed indentation in ci.yml
(4261-move-autocommit-abstractsession) [Friedrich Wilckens]
# 543ea150a 2023-11-14 | Github workflow: re-activated
linux-gcc-make-postgres [Friedrich Wilckens]
# a2d10dffe 2023-11-13 | PostgreSQL SessionImpl: reuse autocommit
flag of AbstractSessionImpl. [Friedrich Wilckens]
# d32f62031 2023-11-13 | MySQL SessionImpl: make sure autocommit
mode is on when session is openend or reset. [Friedrich Wilckens]
# c919b7f79 2023-11-13 | chore(CI): re-enable mysql [Alex Fabijanic]
# ffd0007f2 2023-11-13 | fix(Data::AbstracSessionImpl): protect
autocommit feature handlers #4261 [Alex Fabijanic]
# 1763a5017 2023-11-12 | Brought MySQL backend in line with
_autoCommit flag of AbstractSessionImpl. [Friedrich Wilckens]
# 381ac1d9a 2106-02-07 | feat(Data::AbstractSessionImpl): add
autoCommit property and tests #4261 [Alex Fabijanic] <---
# 18eea1bb7 2023-11-11 | temporarily comment failing mysql ci until
fixed [Aleksandar Fabijanic]
# 6a5387ec2 2023-11-11 | add visitor pattern implementation for
Poco::Dynamic::Var (#4144) [Alexander B]
for _c in 1763a5017 381ac1d9a 18eea1bb7; do
echo "* $_c:";
git branch --contains=$_c | sed 's/^/ /';
done
# * 1763a5017:
# 4261-move-autocommit-abstractsession
# * 381ac1d9a:
# * 18eea1bb7:
#
2366-pocoprocesslaunch-unix-possible-memory-leak-when-launching-invalid-command
# 4261-move-autocommit-abstractsession
# 569-DateTimeParser-cherry-pick
# devel
Links:
[1]: https://github.com/pocoproject/poco
[2]:
https://github.com/pocoproject/poco/commit/381ac1d9a82c9682a5046dd51802a687a81ace91
^ permalink raw reply
* Re: [PATCH] git-compat-util: convert skip_{prefix,suffix}{,_mem} to bool
From: René Scharfe @ 2023-12-19 13:36 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood
Cc: git, AtariDreams via GitGitGadget, Seija Kijin, Jeff King,
Phillip Wood
In-Reply-To: <xmqqa5q7e00q.fsf@gitster.g>
Am 18.12.23 um 21:19 schrieb Junio C Hamano:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> Thanks for the comprehensive commit message, I agree that we'd be
>> better off avoiding adding a fallback. The patch looks good, I did
>> wonder if we really need to covert all of these functions for a
>> test-balloon but the patch is still pretty small overall.
>
> I do have to wonder, though, if we want to be a bit more careful
> than just blindly trusting the platform (i.e. <stdbool.h> might
> exist and __STDC_VERSION__ may say C99, but under the hood their
> implementation may be buggy and coerce the result of an assignment
> of 2 to be different from assigning true).
We could add a compile-time check like below. I can't decide if this
would be prudent or paranoid. It's cheap, though, so perhaps just add
this tripwire for non-conforming compilers without making a judgement?
René
diff --git a/git-compat-util.h b/git-compat-util.h
index 603c97e3b3..8212feaa37 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -705,7 +705,7 @@ static inline bool skip_prefix(const char *str, const char *prefix,
{
do {
if (!*prefix) {
- *out = str;
+ *out = str + BUILD_ASSERT_OR_ZERO((bool)1 == (bool)2);
return true;
}
} while (*str++ == *prefix++);
^ permalink raw reply related
* Re: [PATCH] rebase: use strvec_pushf() for format-patch revisions
From: Patrick Steinhardt @ 2023-12-19 12:25 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <4ab7431c-6c1b-448c-b4d2-e8b9be0e4eef@web.de>
[-- Attachment #1: Type: text/plain, Size: 2991 bytes --]
On Tue, Dec 19, 2023 at 08:42:18AM +0100, René Scharfe wrote:
> In run_am(), a strbuf is used to create a revision argument that is then
> added to the argument list for git format-patch using strvec_push().
> Use strvec_pushf() to add it directly instead, simplifying the code.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
Thanks, this simplification looks good to me!
Patrick
> ---
> Formatted with --inter-hunk-context=14 for easier review.
>
> builtin/rebase.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 9f8192e0a5..ddde4cbb87 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -582,7 +582,6 @@ static int run_am(struct rebase_options *opts)
> {
> struct child_process am = CHILD_PROCESS_INIT;
> struct child_process format_patch = CHILD_PROCESS_INIT;
> - struct strbuf revisions = STRBUF_INIT;
> int status;
> char *rebased_patches;
>
> @@ -615,34 +614,32 @@ static int run_am(struct rebase_options *opts)
> return run_command(&am);
> }
>
> - strbuf_addf(&revisions, "%s...%s",
> - oid_to_hex(opts->root ?
> - /* this is now equivalent to !opts->upstream */
> - &opts->onto->object.oid :
> - &opts->upstream->object.oid),
> - oid_to_hex(&opts->orig_head->object.oid));
> -
> rebased_patches = xstrdup(git_path("rebased-patches"));
> format_patch.out = open(rebased_patches,
> O_WRONLY | O_CREAT | O_TRUNC, 0666);
> if (format_patch.out < 0) {
> status = error_errno(_("could not open '%s' for writing"),
> rebased_patches);
> free(rebased_patches);
> strvec_clear(&am.args);
> return status;
> }
>
> format_patch.git_cmd = 1;
> strvec_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
> "--full-index", "--cherry-pick", "--right-only",
> "--default-prefix", "--no-renames",
> "--no-cover-letter", "--pretty=mboxrd", "--topo-order",
> "--no-base", NULL);
> if (opts->git_format_patch_opt.len)
> strvec_split(&format_patch.args,
> opts->git_format_patch_opt.buf);
> - strvec_push(&format_patch.args, revisions.buf);
> + strvec_pushf(&format_patch.args, "%s...%s",
> + oid_to_hex(opts->root ?
> + /* this is now equivalent to !opts->upstream */
> + &opts->onto->object.oid :
> + &opts->upstream->object.oid),
> + oid_to_hex(&opts->orig_head->object.oid));
> if (opts->restrict_revision)
> strvec_pushf(&format_patch.args, "^%s",
> oid_to_hex(&opts->restrict_revision->object.oid));
> @@ -665,10 +662,8 @@ static int run_am(struct rebase_options *opts)
> "As a result, git cannot rebase them."),
> opts->revisions);
>
> - strbuf_release(&revisions);
> return status;
> }
> - strbuf_release(&revisions);
>
> am.in = open(rebased_patches, O_RDONLY);
> if (am.in < 0) {
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [BUG] `git add -p` seems to corrupt a sparse index
From: Sean Allred @ 2023-12-19 11:20 UTC (permalink / raw)
To: git
What did you do before the bug happened? (Steps to reproduce your issue)
git init
for dir in a b c; do mkdir $dir && seq 1 20 > $dir/file; done
git add -A && git commit -m'Some content'
git sparse-checkout set --sparse-index b
seq 1 20 > b/file-2
git add -N b/file-2
git add -p b/file-2
git status
What did you expect to happen? (Expected behavior)
I expected to be dropped into the interactive-add workflow / see my
changes in git-status.
What happened instead? (Actual behavior)
git-add reports 'No changes' and git-status reports nothing at all
(empty output).
The original internal report also was able reproduce messaging like
fatal: cache entry out of order
warning: die() called many times. Recursion error or racy threaded death!
fatal: cache entry out of order
fatal: cache entry out of order
fatal: cache entry out of order
though I've not been able to reproduce that myself. It seems relevant
/ worth noting that core.fsmonitor would be set to 'true' in that
repository.
What's different between what you expected and what actually happened?
git-status appears to silently crash in the error case.
Compare the broken workflow with any of the following variants (all of
which work as expected):
- don't use `--intent-to-add`:
git init
for dir in a b c; do mkdir $dir && seq 1 20 > $dir/file; done
git add -A && git commit -m'Some content'
git sparse-checkout set --sparse-index b
seq 1 20 > b/file-2
git add b/file-2
git status
- don't use `sparse-checkout` at all:
git init
for dir in a b c; do mkdir $dir && seq 1 20 > $dir/file; done
git add -A && git commit -m'Some content'
seq 1 20 > b/file-2
git add -N b/file-2
git add -p b/file-2
git status
- don't use `--sparse-index` specifically:
git init
for dir in a b c; do mkdir $dir && seq 1 20 > $dir/file; done
git add -A && git commit -m'Some content'
git sparse-checkout set b
seq 1 20 > b/file-2
git add -N b/file-2
git add -p b/file-2
git status
Anything else you want to add:
Seems very related to the use of sparse-index.
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.43.0.windows.1
cpu: x86_64
built from commit: 4b968f3ea3b32a7bc50846bab49f3f381841d297
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19044
compiler info: gnuc: 13.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe
[Enabled Hooks]
--
Sean Allred
^ permalink raw reply
* Re: [PATCH] rebase: use strvec_pushf() for format-patch revisions
From: Phillip Wood @ 2023-12-19 11:07 UTC (permalink / raw)
To: René Scharfe, Git List
In-Reply-To: <4ab7431c-6c1b-448c-b4d2-e8b9be0e4eef@web.de>
Hi René
On 19/12/2023 07:42, René Scharfe wrote:
> In run_am(), a strbuf is used to create a revision argument that is then
> added to the argument list for git format-patch using strvec_push().
> Use strvec_pushf() to add it directly instead, simplifying the code.
This looks like a nice simplification and the extra context lines in the
patch are much appreciated
Thanks
Phillip
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Formatted with --inter-hunk-context=14 for easier review.
>
> builtin/rebase.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 9f8192e0a5..ddde4cbb87 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -582,7 +582,6 @@ static int run_am(struct rebase_options *opts)
> {
> struct child_process am = CHILD_PROCESS_INIT;
> struct child_process format_patch = CHILD_PROCESS_INIT;
> - struct strbuf revisions = STRBUF_INIT;
> int status;
> char *rebased_patches;
>
> @@ -615,34 +614,32 @@ static int run_am(struct rebase_options *opts)
> return run_command(&am);
> }
>
> - strbuf_addf(&revisions, "%s...%s",
> - oid_to_hex(opts->root ?
> - /* this is now equivalent to !opts->upstream */
> - &opts->onto->object.oid :
> - &opts->upstream->object.oid),
> - oid_to_hex(&opts->orig_head->object.oid));
> -
> rebased_patches = xstrdup(git_path("rebased-patches"));
> format_patch.out = open(rebased_patches,
> O_WRONLY | O_CREAT | O_TRUNC, 0666);
> if (format_patch.out < 0) {
> status = error_errno(_("could not open '%s' for writing"),
> rebased_patches);
> free(rebased_patches);
> strvec_clear(&am.args);
> return status;
> }
>
> format_patch.git_cmd = 1;
> strvec_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
> "--full-index", "--cherry-pick", "--right-only",
> "--default-prefix", "--no-renames",
> "--no-cover-letter", "--pretty=mboxrd", "--topo-order",
> "--no-base", NULL);
> if (opts->git_format_patch_opt.len)
> strvec_split(&format_patch.args,
> opts->git_format_patch_opt.buf);
> - strvec_push(&format_patch.args, revisions.buf);
> + strvec_pushf(&format_patch.args, "%s...%s",
> + oid_to_hex(opts->root ?
> + /* this is now equivalent to !opts->upstream */
> + &opts->onto->object.oid :
> + &opts->upstream->object.oid),
> + oid_to_hex(&opts->orig_head->object.oid));
> if (opts->restrict_revision)
> strvec_pushf(&format_patch.args, "^%s",
> oid_to_hex(&opts->restrict_revision->object.oid));
> @@ -665,10 +662,8 @@ static int run_am(struct rebase_options *opts)
> "As a result, git cannot rebase them."),
> opts->revisions);
>
> - strbuf_release(&revisions);
> return status;
> }
> - strbuf_release(&revisions);
>
> am.in = open(rebased_patches, O_RDONLY);
> if (am.in < 0) {
> --
> 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