* [RFC PATCH 6/6] completion: add comment
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240102195744.478503-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3bb790220a..7f9a626e1b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1589,8 +1589,14 @@ _git_bisect ()
term_good=`__git bisect terms --term-good`
fi
+ # We will complete any custom terms, but still always complete the
+ # more usual bad/new/good/old because git bisect gives a good error
+ # message if these are given when not in use and that's better than
+ # silent refusal to complete if the user is confused.
+ #
# We want to recognize 'view' but not complete it, because it overlaps
# with 'visualize' too much and is just an alias for it.
+ #
local completable_subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
local all_subcommands="$completable_subcommands view"
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 3/6] completion: move to maintain define-before-use
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240102195744.478503-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 265 ++++++++++++-------------
1 file changed, 132 insertions(+), 133 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 3472fab514..4940ad3e24 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1445,6 +1445,138 @@ _git_archive ()
__git_complete_file
}
+# Options that go well for log, shortlog and gitk
+__git_log_common_options="
+ --not --all
+ --branches --tags --remotes
+ --first-parent --merges --no-merges
+ --max-count=
+ --max-age= --since= --after=
+ --min-age= --until= --before=
+ --min-parents= --max-parents=
+ --no-min-parents --no-max-parents
+"
+# Options that go well for log and gitk (not shortlog)
+__git_log_gitk_options="
+ --dense --sparse --full-history
+ --simplify-merges --simplify-by-decoration
+ --left-right --notes --no-notes
+"
+# Options that go well for log and shortlog (not gitk)
+__git_log_shortlog_options="
+ --author= --committer= --grep=
+ --all-match --invert-grep
+"
+# Options accepted by log and show
+__git_log_show_options="
+ --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
+"
+
+__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
+
+__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
+__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
+
+# Find only porcelain (i.e. not git-rev-list) option (not argument) and
+# selected option argument completions for git-log options and put them in
+# COMPREPLY.
+__git_complete_log_opts ()
+{
+ local merge=""
+ if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
+ merge="--merge"
+ fi
+ case "$prev,$cur" in
+ -L,:*:*)
+ return # fall back to Bash filename completion
+ ;;
+ -L,:*)
+ __git_complete_symbol --cur="${cur#:}" --sfx=":"
+ return
+ ;;
+ -G,*|-S,*)
+ __git_complete_symbol
+ return
+ ;;
+ esac
+ case "$cur" in
+ --pretty=*|--format=*)
+ __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
+ " "" "${cur#*=}"
+ return
+ ;;
+ --date=*)
+ __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
+ return
+ ;;
+ --decorate=*)
+ __gitcomp "full short no" "" "${cur##--decorate=}"
+ return
+ ;;
+ --diff-algorithm=*)
+ __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
+ return
+ ;;
+ --submodule=*)
+ __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
+ return
+ ;;
+ --ws-error-highlight=*)
+ __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
+ return
+ ;;
+ --no-walk=*)
+ __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
+ return
+ ;;
+ --diff-merges=*)
+ __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
+ return
+ ;;
+ --*)
+ __gitcomp "
+ $__git_log_common_options
+ $__git_log_shortlog_options
+ $__git_log_gitk_options
+ $__git_log_show_options
+ --root --topo-order --date-order --reverse
+ --follow --full-diff
+ --abbrev-commit --no-abbrev-commit --abbrev=
+ --relative-date --date=
+ --pretty= --format= --oneline
+ --show-signature
+ --cherry-mark
+ --cherry-pick
+ --graph
+ --decorate --decorate= --no-decorate
+ --walk-reflogs
+ --no-walk --no-walk= --do-walk
+ --parents --children
+ --expand-tabs --expand-tabs= --no-expand-tabs
+ $merge
+ $__git_diff_common_options
+ "
+ return
+ ;;
+ -L:*:*)
+ return # fall back to Bash filename completion
+ ;;
+ -L:*)
+ __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
+ return
+ ;;
+ -G*)
+ __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
+ return
+ ;;
+ -S*)
+ __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
+ return
+ ;;
+ esac
+
+}
+
_git_bisect ()
{
__git_has_doubledash && return
@@ -2052,139 +2184,6 @@ _git_ls_tree ()
__git_complete_file
}
-# Options that go well for log, shortlog and gitk
-__git_log_common_options="
- --not --all
- --branches --tags --remotes
- --first-parent --merges --no-merges
- --max-count=
- --max-age= --since= --after=
- --min-age= --until= --before=
- --min-parents= --max-parents=
- --no-min-parents --no-max-parents
-"
-# Options that go well for log and gitk (not shortlog)
-__git_log_gitk_options="
- --dense --sparse --full-history
- --simplify-merges --simplify-by-decoration
- --left-right --notes --no-notes
-"
-# Options that go well for log and shortlog (not gitk)
-__git_log_shortlog_options="
- --author= --committer= --grep=
- --all-match --invert-grep
-"
-# Options accepted by log and show
-__git_log_show_options="
- --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
-"
-
-__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
-
-__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
-__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-
-
-# Find only porcelain (i.e. not git-rev-list) option (not argument) and
-# selected option argument completions for git-log options and put them in
-# COMPREPLY.
-__git_complete_log_opts ()
-{
- local merge=""
- if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
- merge="--merge"
- fi
- case "$prev,$cur" in
- -L,:*:*)
- return # fall back to Bash filename completion
- ;;
- -L,:*)
- __git_complete_symbol --cur="${cur#:}" --sfx=":"
- return
- ;;
- -G,*|-S,*)
- __git_complete_symbol
- return
- ;;
- esac
- case "$cur" in
- --pretty=*|--format=*)
- __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
- " "" "${cur#*=}"
- return
- ;;
- --date=*)
- __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
- return
- ;;
- --decorate=*)
- __gitcomp "full short no" "" "${cur##--decorate=}"
- return
- ;;
- --diff-algorithm=*)
- __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
- return
- ;;
- --submodule=*)
- __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
- return
- ;;
- --ws-error-highlight=*)
- __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
- return
- ;;
- --no-walk=*)
- __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
- return
- ;;
- --diff-merges=*)
- __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
- return
- ;;
- --*)
- __gitcomp "
- $__git_log_common_options
- $__git_log_shortlog_options
- $__git_log_gitk_options
- $__git_log_show_options
- --root --topo-order --date-order --reverse
- --follow --full-diff
- --abbrev-commit --no-abbrev-commit --abbrev=
- --relative-date --date=
- --pretty= --format= --oneline
- --show-signature
- --cherry-mark
- --cherry-pick
- --graph
- --decorate --decorate= --no-decorate
- --walk-reflogs
- --no-walk --no-walk= --do-walk
- --parents --children
- --expand-tabs --expand-tabs= --no-expand-tabs
- $merge
- $__git_diff_common_options
- "
- return
- ;;
- -L:*:*)
- return # fall back to Bash filename completion
- ;;
- -L:*)
- __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
- return
- ;;
- -G*)
- __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
- return
- ;;
- -S*)
- __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
- return
- ;;
- esac
-
-}
-
_git_log ()
{
__git_has_doubledash && return
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 4/6] completion: custom git-bisect terms
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240102195744.478503-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4940ad3e24..a09598c5c1 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1581,10 +1581,19 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad new good old terms skip reset visualize replay log run help"
+ __git_find_repo_path
+
+ local term_bad term_good
+ if [ -f "$__git_repo_path"/BISECT_START ]; then
+ term_bad=`__git bisect terms --term-bad`
+ term_good=`__git bisect terms --term-good`
+ fi
+
+ local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+
local subcommand="$(__git_find_on_cmdline "$subcommands")"
+
if [ -z "$subcommand" ]; then
- __git_find_repo_path
if [ -f "$__git_repo_path"/BISECT_START ]; then
__gitcomp "$subcommands"
else
@@ -1617,7 +1626,7 @@ _git_bisect ()
esac
case "$subcommand" in
- bad|new|good|old|reset|skip|start)
+ bad|new|"$term_bad"|good|old|"$term_good"|reset|skip|start)
__git_complete_refs
;;
*)
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 2/6] completion: git-log opts to bisect visualize
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240102195744.478503-1-britton.kerin@gmail.com>
To do this the majority of _git_log has been factored out into the new
__git_complete_log_opts. This is needed because the visualize command
accepts git-log options but not rev arguments (they are fixed to the
commits under bisection).
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 30 ++++++++++++++++++++++----
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 15d22ff7d9..3472fab514 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1472,6 +1472,16 @@ _git_bisect ()
;;
esac
;;
+ visualize)
+ case "$cur" in
+ -*)
+ __git_complete_log_opts
+ return
+ ;;
+ *)
+ ;;
+ esac
+ ;;
esac
case "$subcommand" in
@@ -2074,11 +2084,12 @@ __git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-c
__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-_git_log ()
-{
- __git_has_doubledash && return
- __git_find_repo_path
+# Find only porcelain (i.e. not git-rev-list) option (not argument) and
+# selected option argument completions for git-log options and put them in
+# COMPREPLY.
+__git_complete_log_opts ()
+{
local merge=""
if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
merge="--merge"
@@ -2171,6 +2182,17 @@ _git_log ()
return
;;
esac
+
+}
+
+_git_log ()
+{
+ __git_has_doubledash && return
+ __git_find_repo_path
+
+ __git_complete_log_opts
+ [ -z "$COMPREPLY" ] || return
+
__git_complete_revlist
}
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 1/6] completion: complete new old actions, start opts
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240102195744.478503-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 185b47d802..15d22ff7d9 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1449,7 +1449,7 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad good skip reset visualize replay log run"
+ local subcommands="start bad new good old terms skip reset visualize replay log run help"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__git_find_repo_path
@@ -1462,7 +1462,20 @@ _git_bisect ()
fi
case "$subcommand" in
- bad|good|reset|skip|start)
+ start)
+ case "$cur" in
+ --*)
+ __gitcomp "--term-new --term-bad --term-old --term-good --first-parent --no-checkout"
+ return
+ ;;
+ *)
+ ;;
+ esac
+ ;;
+ esac
+
+ case "$subcommand" in
+ bad|new|good|old|reset|skip|start)
__git_complete_refs
;;
*)
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 0/6] completion: improvements for git-bisect
From: Britton Leo Kerin @ 2024-01-02 19:57 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
Bring completion for bisect up to date with current features.
This is RFC mainly because of an implementation issue in patch 2: I think
the mechanism of requiring COMPREPLY to be empty, possibly writing into it,
then checking it post-call is too fragile, but I'm not sure how people would
like to fix this. I'd prefer to just add an assert() of some sort to enforce
the empty-COMPREPLY precondition, rather than rewrite the guts of the former
_git_log() function to use some other intermediate return mechanism (this
would be overkill in this simple context IMO). But I'm not sure how to
do that in a completion context: for sure nothing that is done here should
ever have a chance of showing garbage on a user's screen, but of course for
assert() to be useful a dev would need to see it somehow. Suggestions welcome.
Other than that issue I think it's all reasonable and ready, though there
are a couple other decisions that I guess people might disagree with:
* good/old/new/bad terms are always completed (even if they're
invalild because --term-(good|bad) have been used). The idea here
is that if the user has become confused about their own terms it's
better to let git give them an error message than to have
normally-working completion not happen.
* 'view' is recognized as a subcommand, but not as a completion
candidate. This lets complete of git bisect v<TAB> keep working which
seems convenient and some poor person somewhere probably really wants :)
view is just an alias so loss of interface recall is minimal.
Britton Leo Kerin (6):
completion: complete new old actions, start opts
completion: git-log opts to bisect visualize
completion: move to maintain define-before-use
completion: custom git-bisect terms
completion: recognize but do not complete 'view'
completion: add comment
contrib/completion/git-completion.bash | 310 +++++++++++++++----------
1 file changed, 181 insertions(+), 129 deletions(-)
base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
--
2.43.0
^ permalink raw reply
* Re: [PATCH] git-clone.txt: document -4 and -6
From: Taylor Blau @ 2024-01-02 19:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Wilk, git, Eric Wong
In-Reply-To: <xmqq1qivd8d0.fsf@gitster.g>
On Thu, Jun 01, 2023 at 03:06:35PM +0900, Junio C Hamano wrote:
> Jakub Wilk <jwilk@jwilk.net> writes:
>
> > These options were added in c915f11eb4 ("connect & http: support -4 and
> > -6 switches for remote operations").
> >
> > Signed-off-by: Jakub Wilk <jwilk@jwilk.net>
> > ---
> > Documentation/git-clone.txt | 8 ++++++++
> > 1 file changed, 8 insertions(+)
>
> The patch is not _wrong_ per-se, but there are other options that
> are common among the "fetch" family of commands. I counted at least
> these should be shared between "fetch" and "clone", by splitting
> them out of "fetch-options.txt" into a new file, and including that
> new file from "fetch-options.txt" and "git-clone.txt". Those marked
> with (?) are described in different phrasing between "clone" and
> "fetch", and may fall into the "let's keep them separate, because
> they mean different things" category (later):
>
> * --jobs
> * --upload-pack
> * --quiet (?)
> * --verbose (?)
> * --progress
> * --server-option
> * --ipv[46]
>
> Note that these happen to share the same name, but to "clone" and
> "fetch" they different things, so leaving them separate is the right
> thing to do.
>
> * --no-tags
> * --recurse-submodules
I wrote this ugly shell incantation to find an exhaustive list of
potentially shareable options:
$ grep '^-.*::$' <Documentation/fetch-options.txt |
tr -d ':' | sed -e 's/\[=/=[/' -e 's/<[^>]*>//' |
grep -Eo '^[^=]+' | awk -F] '
/\[no-\]/ { print "--" $2; print "--no-" $2; next }
{ print $0 }
' |
while read arg
do
if grep -Fwq -- $arg Documentation/fetch-options.txt &&
grep -Fwq -- $arg Documentation/git-clone.txt
then
echo $arg;
fi
done
It turned up the following results:
-a
--depth
--shallow-since
--shallow-exclude
--no-tags
--recurse-submodules
-j, --jobs
-u, --upload-pack
-q, --quiet
-v, --verbose
--progress
-o, --server-option
-a is a false-positive (it comes from "you can simply run `git repack
-a`", which is in the clone documentation).
Even though depth, and the shallow options are shared by both fetch and
clone, they have different wording in each context, so they should be
kept separate.
I agree with your thinking that `--no-tags` and `--recurse-submodules`
should be kept separate.
That makes our two lists equal (modulo the --ipv[46] options, which were
previously not documented in git-clone(1) until this patch).
Thanks,
Taylor
^ permalink raw reply
* [PATCH V4 0/1] Replace SID with domain/username on Windows
From: Sören Krecker @ 2024-01-02 19:15 UTC (permalink / raw)
To: git; +Cc: sunshine, Sören Krecker
In-Reply-To: <xmqqsf3feilq.fsf@gitster.g>
Hi everone,
I improve the commit message with information from Microsoft, fixes a memmory access error and the message in case of an error.
Vielen dank für die Hinweise von Junio C Hamano.
Sören Krecker (1):
Replace SID with domain/username
compat/mingw.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
--
2.39.2
^ permalink raw reply
* [PATCH V4 1/1] Replace SID with domain/username
From: Sören Krecker @ 2024-01-02 19:15 UTC (permalink / raw)
To: git; +Cc: sunshine, Sören Krecker
In-Reply-To: <20240102191514.2583-1-soekkle@freenet.de>
Replace SID with domain/username in error message, if owner of repository
and user are not equal on windows systems. Each user should have a unique
SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers).
This means that domain/username is not a loss of information. If the translation
fails the message contains the SID as string.
Old Prompted error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
'S-1-5-21-571067702-4104414259-3379520149-500'
but the current user is:
'S-1-5-21-571067702-4104414259-3379520149-1001'
To add an exception for this directory, call:
git config --global --add safe.directory C:/Users/test/source/repos/git
'''
New prompted error massage:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
'DESKTOP-L78JVA6/Administrator'
but the current user is:
'DESKTOP-L78JVA6/test'
To add an exception for this directory, call:
git config --global --add safe.directory C:/Users/test/source/repos/git
'''
Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
compat/mingw.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..bfd9573a29 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,27 @@ static PSID get_current_user_sid(void)
return result;
}
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+ SID_NAME_USE pe_use;
+ DWORD len_user = 0, len_domain = 0;
+ BOOL translate_sid_to_user;
+
+ /* returns only FALSE, because the string pointers are NULL*/
+ LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+ &pe_use);
+ /*Alloc needed space of the strings*/
+ ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
+ translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
+ *str, &len_domain, &pe_use);
+ if (translate_sid_to_user == FALSE) {
+ FREE_AND_NULL(*str);
+ }
+ else
+ (*str)[len_domain] = '/';
+ return translate_sid_to_user;
+}
+
static int acls_supported(const char *path)
{
size_t offset = offset_1st_component(path);
@@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
} else if (report) {
LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
- if (ConvertSidToStringSidA(sid, &str1))
+ if (user_sid_to_user_name(sid, &str1))
+ to_free1 = str1;
+ else if (ConvertSidToStringSidA(sid, &str1))
to_free1 = str1;
else
str1 = "(inconvertible)";
@@ -2776,7 +2799,10 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
str2 = "(none)";
else if (!IsValidSid(current_user_sid))
str2 = "(invalid)";
- else if (ConvertSidToStringSidA(current_user_sid, &str2))
+ else if (user_sid_to_user_name(current_user_sid, &str2))
+ to_free2 = str2;
+ else if (ConvertSidToStringSidA(current_user_sid,
+ &str2))
to_free2 = str2;
else
str2 = "(inconvertible)";
@@ -2784,8 +2810,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
"'%s' is owned by:\n"
"\t'%s'\nbut the current user is:\n"
"\t'%s'\n", path, str1, str2);
- LocalFree(to_free1);
- LocalFree(to_free2);
+ free(to_free1);
+ free(to_free2);
}
}
--
2.39.2
^ permalink raw reply related
* Re: [RFC PATCH] grep: default to posix digits with -P
From: Carlo Arenas @ 2024-01-02 19:02 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <55309061-5996-4f70-8e6b-b341fc632ddf@web.de>
On Mon, Jan 1, 2024 at 9:18 AM René Scharfe <l.s.r@web.de> wrote:
>
> Am 01.01.24 um 16:03 schrieb Carlo Marcelo Arenas Belón:
> > @@ -321,17 +327,22 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
> > }
> > options |= PCRE2_CASELESS;
> > }
> > - if (!opt->ignore_locale && is_utf8_locale() && !literal)
> > - options |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);
> > + if (!opt->ignore_locale && is_utf8_locale() && !literal) {
> > + options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);
> >
> > -#ifndef GIT_PCRE2_VERSION_10_35_OR_HIGHER
> > - /*
> > - * Work around a JIT bug related to invalid Unicode character handling
> > - * fixed in 10.35:
> > - * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d
> > - */
> > - options &= ~PCRE2_UCP;
> > +#ifdef GIT_PCRE2_VERSION_10_35_OR_HIGHER
> > + /*
> > + * Work around a JIT bug related to invalid Unicode character handling
> > + * fixed in 10.35:
> > + * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d
> > + */
> > + options |= PCRE2_UCP;
> > +#ifdef GIT_PCRE2_VERSION_10_43_OR_HIGHER
> > + if (!opt->perl_digit)
> > + xoptions |= (PCRE2_EXTRA_ASCII_BSD | PCRE2_EXTRA_ASCII_DIGIT);
> > #endif
> > +#endif
>
> Why do we need that extra level of indentation?
I was just trying to simplify the code by including all the logic in
one single set.
The original lack of indentation that was introduced by later fixes to
the code was IMHO also misguided since the obvious "objective" as set
in the original code that added PCRE2_UCP was that it should be used
whenever UTF was also in use as shown by
acabd2048ee0ee53728100408970ab45a6dab65e.
Of course, we soon found out that the original implementation of
PCRE2_MATCH_INVALID_UTF that came with PCRE2 10.34 was buggy and so an
exception was introduced in 14b9a044798ebb3858a1f1a1377309a3d6054ac8.
Note that the problematic code is only relevant when JIT is also
enabled, but JIT is almost always enabled.
> The old code turned PCRE2_UCP on by default and turned it off for older
> versions. The new code enables PCRE2_UCP only for newer versions. The
> result should be the same, no? So why change that part at all?
Because it gets us a little closer to the real reason why we need to
disable UCP for anything older than 10.35, and that is that there is a
bug there that is ONLY relevant if we are using JIT.
My hope though is that with the release of 10.43 (currently in RC1),
10.34 will become irrelevant soon enough and this whole code could be
cleaned up further.
> But the comment is now off, isn't it? The workaround was turning
> PCRE2_UCP off for older versions (because those were broken), not
> turning it on for newer versions (because it would be required by some
> unfixed regression).
The comment was never correct, because it was turning it off, because
the combination of JIT + MATCH_INVALID_UTF (which was released in
10.34) + UCP is broken.
> Do we need to nest the checks for GIT_PCRE2_VERSION_10_35_OR_HIGHER and
> GIT_PCRE2_VERSION_10_43_OR_HIGHER? Keeping them separate would help
> keep the #ifdef parts as short as possible and maintainable
> independently.
I thought that nesting them would make it simpler to maintain, since
there are somehow connected anyway.
The additional flags that are added in PCRE 10.43 are ONLY needed and
useful on top of PCRE2_UCP, which itself only makes sense on top of
UTF.
> > @@ -27,13 +34,13 @@ do
> > if ! test_have_prereq PERF_GREP_ENGINES_THREADS
> > then
> > test_perf "grep -P '$pattern'" --prereq PCRE "
> > - git -P grep -f pat || :
> > + git grep -P -f pat || :
> > "
> > else
> > for threads in $GIT_PERF_GREP_THREADS
> > do
> > test_perf "grep -P '$pattern' with $threads threads" --prereq PTHREADS,PCRE "
> > - git -c grep.threads=$threads -P grep -f pat || :
> > + git -c grep.threads=$threads grep -P -f pat || :
>
> What's going on here? You remove the -P option of git (--no-pager) and
> add the -P option of git grep (--perl-regexp). So this perf test never
> tested PCRE despite its name? Seems worth a separate patch.
My original code was a dud. This would make that at least more obvious,
> Do the performance numbers in acabd2048e (grep: correctly identify utf-8
> characters with \{b,w} in -P, 2023-01-08) still hold up in that light?
FWIW the performance numbers still hold up, but just because I did the
tests independently using hyperfine, and indeed when I did the first
version of this patch, I had a nice reproducible description that
showed how to get 20% better performance while searching the git
repository itself for something like 4 digits (as used in Copyright
dates).
My idea was to reply to my own post with instructions on how to test
this, which basically require to also compile the recently released
10.43RC1 and build against that.
Indeed, AFAIK the test would fail if built with an older version of
PCRE, but wasn't sure if making a prerequisite that hardcoded the
version in test-tool might be the best approach to prevent that,
especially with all the libification work.
Carlo
PS. your reply got lost somehow in my SPAM folder, so I apologize for
the late reply
^ permalink raw reply
* Re: [PATCH] git-clone.txt: document -4 and -6
From: Jakub Wilk @ 2024-01-02 18:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Wong
In-Reply-To: <xmqq1qivd8d0.fsf@gitster.g>
* Junio C Hamano <gitster@pobox.com>, 2023-06-01 15:06:
>The patch is not _wrong_ per-se, but there are other options that are
>common among the "fetch" family of commands. I counted at least these
>should be shared between "fetch" and "clone", by splitting them out of
>"fetch-options.txt" into a new file, and including that new file from
>"fetch-options.txt" and "git-clone.txt".
Agreed such a split would be better, but I don't have energy to
implement it.
--
Jakub Wilk
^ permalink raw reply
* Re: [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Taylor Blau @ 2024-01-02 18:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Achu Luma, christian.couder, git, Christian Couder
In-Reply-To: <xmqqsf3ohkew.fsf@gitster.g>
On Tue, Dec 26, 2023 at 10:45:59AM -0800, Junio C Hamano wrote:
> Achu Luma <ach.lumap@gmail.com> writes:
>
> > diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
> > deleted file mode 100644
> > index e5659df40b..0000000000
> > --- a/t/helper/test-ctype.c
> > +++ /dev/null
> > @@ -1,70 +0,0 @@
> > -#include "test-tool.h"
> > -
> > -static int rc;
> > -
> > -static void report_error(const char *class, int ch)
> > -{
> > - printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
> > - rc = 1;
> > -}
>
> So, if we have a is_foo() that characterises a byte that ought to
> be "foo" but gets miscategorised not to be "foo", we used to
> pinpoint exactly the byte value that was an issue. We did not do
> any early return ...
>
> > ...
> > -#define TEST_CLASS(t,s) { \
> > - int i; \
> > - for (i = 0; i < 256; i++) { \
> > - if (is_in(s, i) != t(i)) \
> > - report_error(#t, i); \
> > - } \
> > - if (t(EOF)) \
> > - report_error(#t, EOF); \
> > -}
>
> ... and reported for all errors in the "class".
>
> > diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
> > new file mode 100644
> > index 0000000000..41189ba9f9
> > --- /dev/null
> > +++ b/t/unit-tests/t-ctype.c
> > @@ -0,0 +1,76 @@
> > +#include "test-lib.h"
> > +
> > +static int is_in(const char *s, int ch)
> > +{
> > + /*
> > + * We can't find NUL using strchr. Accept it as the first
> > + * character in the spec -- there are no empty classes.
> > + */
> > + if (ch == '\0')
> > + return ch == *s;
> > + if (*s == '\0')
> > + s++;
> > + return !!strchr(s, ch);
> > +}
> > +
> > +/* Macro to test a character type */
> > +#define TEST_CTYPE_FUNC(func, string) \
> > +static void test_ctype_##func(void) \
> > +{ \
> > + int i; \
> > + for (i = 0; i < 256; i++) \
> > + check_int(func(i), ==, is_in(string, i)); \
> > +}
>
> Now, we let check_int() to do the checking for each and every byte
> value for the class. check_int() uses different reporting and shows
> the problematic value in a way that is more verbose and at the same
> time is a less specific and harder to understand:
>
> test_msg(" left: %"PRIdMAX, a);
> test_msg(" right: %"PRIdMAX, b);
>
> But that is probably the price to pay to use a more generic
> framework, I guess.
Perhaps I'm missing something here, since I haven't followed the
unit-test effort very closely, but this check_int() macro feels like it
might be overkill for what we're trying to do.
We know that the expected value is the result of is_in(string, i), so I
wonder if we might benefit from having an "assert_equals()" that looks
like:
assert_equals(is_in(string, i), func(i));
Where we follow the usual convention of treating the first argument as
the expected value, and the second as the actual value. Then we could
format our error message to be more specific, like:
test_msg("expected %d, got %d", expected, actual);
I think that this would be a little more readable, and still seems
flexible enough to support the kind of thing that check_int(..., ==,
...) is after.
> > +int cmd_main(int argc, const char **argv) {
> > + /* Run all character type tests */
> > + TEST(test_ctype_isspace(), "isspace() works as we expect");
> > + TEST(test_ctype_isdigit(), "isdigit() works as we expect");
> > + TEST(test_ctype_isalpha(), "isalpha() works as we expect");
> > + TEST(test_ctype_isalnum(), "isalnum() works as we expect");
> > + TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect");
> > + TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect");
> > + TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect");
> > + TEST(test_ctype_isascii(), "isascii() works as we expect");
> > + TEST(test_ctype_islower(), "islower() works as we expect");
> > + TEST(test_ctype_isupper(), "isupper() works as we expect");
> > + TEST(test_ctype_iscntrl(), "iscntrl() works as we expect");
> > + TEST(test_ctype_ispunct(), "ispunct() works as we expect");
> > + TEST(test_ctype_isxdigit(), "isxdigit() works as we expect");
> > + TEST(test_ctype_isprint(), "isprint() works as we expect");
> > +
> > + return test_done();
> > +}
>
> As a practice to use the unit-tests framework, the patch looks OK.
> helper/test-ctype.c indeed is an oddball that runs once and checks
> everything it wants to check, for which the unit tests framework is
> much more suited.
As an aside, I don't think we need the "works as we expect" suffix in
each test description. I personally would be fine with something like:
TEST(test_ctype_isspace(), "isspace()");
TEST(test_ctype_isdigit(), "isdigit()");
...
But don't feel strongly about it.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Taylor Blau @ 2024-01-02 18:49 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Junio C Hamano, Karthik Nayak, git, christian.couder
In-Reply-To: <ZY1PLtPue4PgbhwU@tanuki>
On Thu, Dec 28, 2023 at 11:34:22AM +0100, Patrick Steinhardt wrote:
> One interesting question is how we should treat files that look like a
> pseudoref, but which really aren't. I'm not aware of any such files
> written by Git itself, but it could certainly be that a user wrote such
> a file into the repository manually. But given that we're adding new
> behaviour that will be opt-in (e.g. via a new switch) I'd rather err on
> the side of caution and mark any such file as broken instead of silently
> ignoring them.
I probably wouldn't spend a ton of time worrying about this personally.
Without additional information, I think it's impossible for us to
determine a-priori whether or not a file underneath $GIT_DIR should be
interpreted as a pseudo-ref or not.
I agree with your reasoning that since this is opt-in via a new
command-line flag, that we're probably OK here enumerating the files in
$GIT_DIR, and printing out the ones that look like pseudo-refs.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Taylor Blau @ 2024-01-02 18:47 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Junio C Hamano, git, ps, christian.couder
In-Reply-To: <CAOLa=ZTPxWXnZ8kpBB7=cybNfdEv6d6O37Em7Vpmcw=enpY1_w@mail.gmail.com>
On Tue, Jan 02, 2024 at 07:18:48AM -0800, Karthik Nayak wrote:
> > As "git for-each-ref" takes pattern that is prefix match, e.g.,
> >
> > $ git for-each-ref refs/remotes/
> >
> > shows everything like refs/remotes/origin/main that begins with
> > refs/remotes/, I wonder if
> >
> > $ git for-each-ref ""
> >
> > should mean what you are asking for. After all, "git for-each-ref"
> > does *not* take "--branches" and others like "git log" family to
> > limit its operation to subhierarchy of "refs/" to begin with.
>
> But I don't think using an empty pattern is the best way to go forward.
> This would break the pattern matching feature. For instance, what if the
> user wanted to print all refs, but pattern match "*_HEAD"?
>
> Would that be
>
> $ git for-each-ref "" "*_HEAD"
>
> I think this would be confusing, since the first pattern is now acting
> as an option, since its not really filtering rather its changing the
> search space.
>
> Maybe "--all-refs" or "--all-ref-types" instead?
I tend to agree that the special empty pattern would be a good shorthand
for listing all references underneath refs/, including any top-level
psuedo-refs.
But I don't think that I quite follow what Karthik is saying here.
for-each-ref returns the union of references that match the given
pattern(s), not their intersection. So if you wanted to list just the
psudo-refs ending in '_HEAD', you'd do:
$ git for-each-ref "*_HEAD"
I think if you wanted to list all pseudo-refs, calling the option
`--pseudo-refs` seems reasonable. But if you want to list some subset of
psueod-refs matching a given pattern, you should specify that pattern
directly.
Thanks,
Taylor
^ permalink raw reply
* Re: subtree split after deleting and re-running git-subtree add, fails with "fatal: cache for XXX already exists!"
From: Christian Couder @ 2024-01-02 18:23 UTC (permalink / raw)
To: Eli Schwartz; +Cc: git
In-Reply-To: <6de00946-9c5a-4854-9e49-069a22f8a782@gmail.com>
On Tue, Dec 26, 2023 at 1:58 AM Eli Schwartz <eschwartz93@gmail.com> wrote:
>
> Originally reported in https://github.com/eli-schwartz/aurpublish/issues/30
>
>
> Given a subtree that gets messed up, some users might naturally
> gravitate towards deleting the subtree, and recreating it again via
> `git subtree add`. This can result in a difficult to solve situation.
> Any attempt to split it seems to produce failure.
>
> Reproducer:
>
> git init testme && cd testme
> mkdir foo
> touch foo/bar
> git add foo/bar
> git commit -m ...
> split_commit=$(git subtree split -P foo --rejoin)
> # Added dir 'foo'
> echo "${split_commit}"
> # 42517e4b9fe310a64be2a777ef08c91bd582b385
>
> git rm -r foo
> git commit -m deleted
> git subtree add --prefix foo "${split_commit}"
> # Added dir 'foo'
> git subtree split -P foo --rejoin
> # fatal: cache for 42517e4b9fe310a64be2a777ef08c91bd582b385 already exists!
>
>
>
> The interesting thing here is that in git.git commit
> d2f0f819547de35ffc923fc963f806f1656eb2ca:
> "subtree: more consistent error propagation"
> the git-subtree program got a bit of a facelift w.r.t. proper error
> checking.
>
> In particular, in find_existing_splits, `cache_set $sub $sub` will fail
> here. But before that commit, the die did not propagate. It turns out
> that actually ignoring this was "fine" and resulted in successfully
> splitting (while also printing a "warning": back then, the word "fatal"
> did not appear anywhere in the message; now it does).
Thanks for reporting this issue! Unfortunately it looks like git
subtree is not very well maintained these days.
There is another thread where Zach FettersMoore proposed other fixes
in what look like a similar area:
https://lore.kernel.org/git/pull.1587.v6.git.1701442494319.gitgitgadget@gmail.com/
Maybe you could team up with Zach to review each other's fixes?
^ permalink raw reply
* Re: [PATCH v3 1/1] Replace SID with domain/username
From: Junio C Hamano @ 2024-01-02 17:43 UTC (permalink / raw)
To: Sören Krecker; +Cc: git, sunshine
In-Reply-To: <20231231091245.2853-2-soekkle@freenet.de>
Sören Krecker <soekkle@freenet.de> writes:
> Replace SID with domain/username in error message, if owner of repository
> and user are not equal on windows systems.
>
> Old message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 'S-1-5-21-571067702-4104414259-3379520149-500'
> but the current user is:
> 'S-1-5-21-571067702-4104414259-3379520149-1001'
> To add an exception for this directory, call:
>
> git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> New massage:
"massage"???
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 'DESKTOP-L78JVA6/Administrator'
> but the current user is:
> 'DESKTOP-L78JVA6/test'
> To add an exception for this directory, call:
>
> git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
The same "does this lose information?" comment applies to this one
as well, as the fundamental approach is unchanged.
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> + SID_NAME_USE pe_use;
> + DWORD len_user = 0, len_domain = 0;
> + BOOL translate_sid_to_user;
> +
> + /* returns only FALSE, because the string pointers are NULL*/
> + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> + &pe_use);
> + /*Alloc needed space of the strings*/
> + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
> + translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> + *str, &len_domain, &pe_use);
> + *(*str + len_domain) = '/';
> + if (translate_sid_to_user == FALSE) {
> + FREE_AND_NULL(*str);
> + }
Is this "FREE_AND_NULL" about clearing after you see an error? If
so, shouldn't "overwrite the byte after the domain part with a slash"
be done only when you have no error, i.e., perhaps
translate_sid_to_user = LookupAccountSidA(...);
if (!translate_sid_to_user)
FREE_AND_NULL(*str);
else
(*str)[len_domain] = '/';
or something along the line?
> + return translate_sid_to_user;
> +}
^ permalink raw reply
* Re: [PATCH v2 1/1] Replace SID with domain/username
From: Junio C Hamano @ 2024-01-02 17:33 UTC (permalink / raw)
To: Sören Krecker; +Cc: git, sunshine
In-Reply-To: <xmqqplyjg10l.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sören Krecker <soekkle@freenet.de> writes:
>
>> Replace SID with domain/username in erromessage, if owner of repository
>> and user are not equal on windows systems.
>
> "erromessage" -> "error messages" or something?
>
> This may not be a question raised by anybody who know Windows, but
> because I do not do Windows, it makes me wonder if this is losing
> information. Can two SID for the same user be active at the same
> time, which would cause user_sid_to_user_name() potentially yield
> the same string for two different SID?
>
> In any case, I am reasonably sure that Dscho will say yes or no to
> this patch (the above "makes me wonder" does not need to be
> resolved) and I can wait until then.
>
> Thanks.
Another thing I forgot to mention (but did wonder). The new helper
function does allow LookupAccountSidA() to fail. Should it fall
back to ConvertSidToStringSidA() that the original has been using?
In any case, I do not think a failure to convert will result in an
attempt to format ("%s", NULL) thanks to the existing code that uses
the stringified SID, which is good.
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Junio C Hamano @ 2024-01-02 16:49 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps, christian.couder
In-Reply-To: <CAOLa=ZTPxWXnZ8kpBB7=cybNfdEv6d6O37Em7Vpmcw=enpY1_w@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
>> As "git for-each-ref" takes pattern that is prefix match, e.g.,
>>
>> $ git for-each-ref refs/remotes/
>>
>> shows everything like refs/remotes/origin/main that begins with
>> refs/remotes/, I wonder if
>>
>> $ git for-each-ref ""
>>
>> should mean what you are asking for. After all, "git for-each-ref"
>> does *not* take "--branches" and others like "git log" family to
>> limit its operation to subhierarchy of "refs/" to begin with.
>
> But I don't think using an empty pattern is the best way to go forward.
> This would break the pattern matching feature. For instance, what if the
> user wanted to print all refs, but pattern match "*_HEAD"?
>
> Would that be
>
> $ git for-each-ref "" "*_HEAD"
Because you do not omit the leading hierarchy when using globs:
$ git for-each-ref v2.4\?.\* ;# nothing
$ git for-each-ref tags/v2.4\?.\* ;# nothing
$ git for-each-ref refs/tags/v2.4\?.\* ;# gives tags in v2.40 and onwards
I would expect that it would be more like
$ git for-each-ref "*_HEAD"
And because you can have two or more patterns, e.g.,
$ git for-each-ref refs/tags/v2.4\?.\* refs/heads/m\*
to get those recent tags and branches whose name begins with 'm', I
would expect your
$ git for-each-ref "" "*_HEAD"
would probably be equivalent to
$ git for-each-ref ""
^ permalink raw reply
* Re: [Outreachy][PATCH v3] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Junio C Hamano @ 2024-01-02 16:35 UTC (permalink / raw)
To: René Scharfe
Cc: Achu Luma, git, chriscool, christian.couder, phillip.wood,
steadmon
In-Reply-To: <435a03c7-dbf3-4ddb-b183-cac86ed0467d@web.de>
René Scharfe <l.s.r@web.de> writes:
>> ...
>> + for (int i = 0; i < 256; i++) { \
>> + if (!check_int(func(i), ==, is_in(string, i))) \
>> + test_msg(" i: %02x", i); \
>> + } \
>
> This loop is indented with spaces followed by tabs. The Git project
> prefers indenting by tabs and in some cases tabs followed by spaces, but
> not the other way array. git am warns about such whitespace errors and
> can actually fix them automatically, so I imagine this wouldn't be a
> deal breaker. But even if it seems picky, respecting the project's
> preferences from the start reduces unnecessary friction.
>
> The original test reported the number of a misclassified character
> (basically its ASCII code) in both decimal and hexadecimal form. This
> code prints it only in hexadecimal, but without the prefix "0x". A
> casual reader could mistake hexadecimal numbers for decimal ones as a
> result. Printing only one suffices, but I think it's better to either
> use decimal notation without any prefix or hexadecimal with the "0x"
> prefix to avoid confusion. There's no reason to be stingy with the
> screen space here.
> ...
> Each class (e.g. space or digit) is mentioned thrice here: When
> declaring its function with TEST_CTYPE_FUNC, when calling said function
> and again in the test description. Adding a new class requires adding
> two lines of code. That's not too bad, but the original implementation
> didn't require that repetition and adding a new class only required
> adding a single line.
Thanks for an excellent review.
>
> I mentioned this briefly in my review of v1 in
> https://lore.kernel.org/git/f743b473-40f8-423d-bf5b-d42b92e5aa1b@web.de/
> and showed a way to define character classes without repeating their
> names. You don't have to follow that suggestion, but it would be nice
> if you could give some feedback about it.
>
>> +
>> + return test_done();
>> +}
>
> René
^ permalink raw reply
* Re: [PATCH] write-or-die: make GIT_FLUSH a Boolean environment variable
From: Junio C Hamano @ 2024-01-02 16:29 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Chandra Pratap via GitGitGadget, git, Chandra Pratap,
Chandra Pratap
In-Reply-To: <20240101082446.GA21905@tb-raspi4>
Torsten Bögershausen <tboegi@web.de> writes:
>> - char *cp;
>> + int 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);
>> + cp = git_env_bool("GIT_FLUSH", -1);
>> + if (cp >= 0)
>> + skip_stdout_flush = (cp == 0);
>> else if ((fstat(fileno(stdout), &st) == 0) &&
>> S_ISREG(st.st_mode))
>> skip_stdout_flush = 1;
>
> I think that the "cp" variable could be renamed,
> cp is not a "char pointer" any more.
Absolutely. Very good point.
> However, using the logic from git_env_bool(), it can go away anyway,
> wouldn't it ?
True.
If we are doing clean-ups in this area, we may want to also stop
doing "== 0" comparisons on lines the patch touches while at it.
> diff --git a/write-or-die.c b/write-or-die.c
> index 42a2dc73cd..01b042bf12 100644
> --- a/write-or-die.c
> +++ b/write-or-die.c
> @@ -13,21 +13,21 @@
> * more. So we just ignore that case instead (and hope we get
> * the right error code on the flush).
> *
> + * The flushing can be skipped like this:
> + * GIT_FLUSH=0 git-rev-list HEAD
> + *
> * If the file handle is stdout, and stdout is a file, then skip the
> - * flush entirely since it's not needed.
> + * flush as well since it's not needed.
> */
> 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);
> + if (git_env_bool("GIT_FLUSH", -1) == 0)
> + skip_stdout_flush = 1;
> else if ((fstat(fileno(stdout), &st) == 0) &&
> S_ISREG(st.st_mode))
> skip_stdout_flush = 1;
^ permalink raw reply
* Re: [PATCH v2 1/1] Replace SID with domain/username
From: Junio C Hamano @ 2024-01-02 16:20 UTC (permalink / raw)
To: Sören Krecker; +Cc: git, sunshine
In-Reply-To: <20231229120319.3797-2-soekkle@freenet.de>
Sören Krecker <soekkle@freenet.de> writes:
> Replace SID with domain/username in erromessage, if owner of repository
> and user are not equal on windows systems.
"erromessage" -> "error messages" or something?
This may not be a question raised by anybody who know Windows, but
because I do not do Windows, it makes me wonder if this is losing
information. Can two SID for the same user be active at the same
time, which would cause user_sid_to_user_name() potentially yield
the same string for two different SID?
In any case, I am reasonably sure that Dscho will say yes or no to
this patch (the above "makes me wonder" does not need to be
resolved) and I can wait until then.
Thanks.
> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
> compat/mingw.c | 28 ++++++++++++++++++++++++----
> 1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 42053c1f65..05aeaaa9ad 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
> return result;
> }
>
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> + SID_NAME_USE pe_use;
> + DWORD len_user = 0, len_domain = 0;
> + BOOL translate_sid_to_user;
> +
> + /* returns only FALSE, because the string pointers are NULL*/
> + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> + &pe_use);
> + /*Alloc needed space of the strings*/
> + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
> + translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> + *str, &len_domain, &pe_use);
> + *(*str + len_domain) = '/';
> + if (translate_sid_to_user == FALSE) {
> + FREE_AND_NULL(*str);
> + }
> + return translate_sid_to_user;
> +}
> +
> static int acls_supported(const char *path)
> {
> size_t offset = offset_1st_component(path);
> @@ -2767,7 +2787,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
> } else if (report) {
> LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
>
> - if (ConvertSidToStringSidA(sid, &str1))
> + if (user_sid_to_user_name(sid, &str1))
> to_free1 = str1;
> else
> str1 = "(inconvertible)";
> @@ -2776,7 +2796,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
> str2 = "(none)";
> else if (!IsValidSid(current_user_sid))
> str2 = "(invalid)";
> - else if (ConvertSidToStringSidA(current_user_sid, &str2))
> + else if (user_sid_to_user_name(current_user_sid, &str2))
> to_free2 = str2;
> else
> str2 = "(inconvertible)";
> @@ -2784,8 +2804,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
> "'%s' is owned by:\n"
> "\t'%s'\nbut the current user is:\n"
> "\t'%s'\n", path, str1, str2);
> - LocalFree(to_free1);
> - LocalFree(to_free2);
> + free(to_free1);
> + free(to_free2);
> }
> }
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Karthik Nayak @ 2024-01-02 15:23 UTC (permalink / raw)
To: Patrick Steinhardt, Junio C Hamano; +Cc: git, christian.couder
In-Reply-To: <ZY1PLtPue4PgbhwU@tanuki>
[-- Attachment #1: Type: text/plain, Size: 1992 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> One interesting question is how we should treat files that look like a
> pseudoref, but which really aren't. I'm not aware of any such files
> written by Git itself, but it could certainly be that a user wrote such
> a file into the repository manually. But given that we're adding new
> behaviour that will be opt-in (e.g. via a new switch) I'd rather err on
> the side of caution and mark any such file as broken instead of silently
> ignoring them.
>
This is definitely tricky, especially since something like
`COMMIT_EDITMSG` passes the `is_pseudoref_syntax()` check and could
simply contain a commit-ish object ID. Here identifying that this is not
a pseudoref is hard when it satisfies both:
1. The general pseudoref syntax
2. Contains the expected file content type
>
> Yup, for the reftable we don't have the issue of "How do we detect refs
> dynamically" at all. So I would love for there to be a way to print all
> refs in the refdb, regardless of whether they start with `refs/` or look
> like a pseudoref or whatever else. Otherwise it wouldn't be possible for
> a user to delete anything stored in the refdb that may have a funny
> name, be it intentionally, by accident or due to a bug.
>
> In the reftable backend, the ref iterator's `_advance()` function has a
> hardcoded `starts_with(refname, "refs/")` check. If false, then we'd
> skip the ref in order to retain the same behaviour that the files
> backend has. So maybe what we should be doing is to introduce a new flag
> `DO_FOR_EACH_ALL_REFS` and expose it via git-for-each-ref(1) or
> git-show-ref(1). So:
>
> - For the reftable backend we'd skip the `starts_with()` check and
> simply return all refs.
>
> - For the files backend we'd also iterate through all files in
> $GIT_DIR and check whether they are pseudoref-like.
>
This makes sense to me and is along what I was considering for the
dynamic approach, thanks for writing it down, clarifies my thoughts.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] ref-filter: support filtering of operational refs
From: Karthik Nayak @ 2024-01-02 15:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ps, christian.couder
In-Reply-To: <xmqqsf3oj3u8.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2634 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
>
> It would not begin with 40-hex, though. If I were doing this,
> perhaps I'd say we should first split is_pseudoref_syntax() that is
> overly loose into to classes (e.g. "caps with underscores that ends
> with HEAD" and everything else), silently reject false positives
> among the latter class. Then we rename those that are misnamed
> (there should be only few, like AUTO_MERGE that should be a
> pseudoref but named without _HEAD; I do not think of anything that
> ends with _HEAD that is not a ref) over time and drop the latter
> class.
>
I agree about checking the contents of the files to filter out false
positives around the filenames. Alright, this seems like a good way to
go.
Summarizing, we'll change `is_pseudoref_syntax()` to
1. Check for filename to be UPPERCASE including '-', '_'.
a. If the filename ends with _HEAD, we return as positive
b. Else check the file content for SHA1/SHA256 hex
This still leaves out HEAD ref, which is not a pseudo ref (since it can
be a symref at times). I'll figure out something there.
>
>> While you're here, I wonder if you have any thoughts on the last block
>> of my first mail.
>>
>>> Over this, I'm also curious on what the mailing list thinks about
>>> exposing this to the client side. Would an `--all` option for
>>> git-for-each-ref(1) be sufficient?
>
> "list pseudorefs in addition to things below refs/"? Sounds OK to
> me as a feature.
>
> However, "--all" does not mean that in the context of "git log"
> family of commands. Over there, it means "not just --branches,
> --tags, and --remotes, but everything" which is still limited below
> "refs/".
>
Good point, I agree, usage of "--all" would clash here.
> As "git for-each-ref" takes pattern that is prefix match, e.g.,
>
> $ git for-each-ref refs/remotes/
>
> shows everything like refs/remotes/origin/main that begins with
> refs/remotes/, I wonder if
>
> $ git for-each-ref ""
>
> should mean what you are asking for. After all, "git for-each-ref"
> does *not* take "--branches" and others like "git log" family to
> limit its operation to subhierarchy of "refs/" to begin with.
But I don't think using an empty pattern is the best way to go forward.
This would break the pattern matching feature. For instance, what if the
user wanted to print all refs, but pattern match "*_HEAD"?
Would that be
$ git for-each-ref "" "*_HEAD"
I think this would be confusing, since the first pattern is now acting
as an option, since its not really filtering rather its changing the
search space.
Maybe "--all-refs" or "--all-ref-types" instead?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 2/6] setup: move creation of "refs/" into the files backend
From: Karthik Nayak @ 2024-01-02 13:23 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <ae013eaa4aba0d68172ff03dbe9f2c2bca596285.1703754513.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Move the code to create the directory into the files backend itself to
> make it so. This means that future ref backends will also need to have
> equivalent logic around to ensure that the directory exists, but it
> seems a lot more sensible to have it this way round than to require
> callers to create the directory themselves.
>
Why not move it to refs.c:refs_init_db() instead? this way each
implementation doesn't have to do it?
@@ -2020,14 +2024,30 @@ const char *refs_resolve_ref_unsafe(struct
ref_store *refs,
/* backend functions */
int refs_init_db(struct ref_store *refs, int flags, struct strbuf *err)
{
+ /*
+ * We need to create a "refs" dir in any case so that older versions of
+ * Git can tell that this is a repository. This serves two main
+ * purposes:
+ *
+ * - Clients will know to stop walking the parent-directory chain when
+ * detecting the Git repository. Otherwise they may end up detecting
+ * a Git repository in a parent directory instead.
+ *
+ * - Instead of failing to detect a repository with unknown reference
+ * format altogether, old clients will print an error saying that
+ * they do not understand the reference format extension.
+ */
+ safe_create_dir(git_path("refs"), 1);
+ adjust_shared_perm(git_path("refs"));
+
return refs->be->init_db(refs, flags, err);
}
^ permalink raw reply
* [ANNOUNCE] Git Rev News edition 106
From: Christian Couder @ 2024-01-01 20:53 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jakub Narebski, Markus Jansen,
Štěpán Němec, Kaartic Sivaraam, Taylor Blau,
Johannes Schindelin, Ævar Arnfjörð Bjarmason, lwn,
Josh Soref, Eric Sunshine, Elijah Newren, VonC, Scott Chacon
Hi everyone,
The 106th edition of Git Rev News is now published:
https://git.github.io/rev_news/2023/12/31/edition-106/
Thanks a lot to VonC, Josh Soref and Štěpán Němec who helped this month!
Enjoy,
Christian, Jakub, Markus and Kaartic.
PS: An issue for the next edition is already opened and contributions
are welcome:
https://github.com/git/git.github.io/issues/679
^ 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