* Re: [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.
From: Christian Couder @ 2024-01-09 9:20 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <20240109060417.1144647-3-shyamthakkar001@gmail.com>
First about the commit subject:
"t7501: Add tests for various index usages, -i and -o, of commit command."
it should be shorter, shouldn't end with a "." and "Add" should be "add".
On Tue, Jan 9, 2024 at 7:10 AM Ghanshyam Thakkar
<shyamthakkar001@gmail.com> wrote:
>
> This commit adds tests for -i and -o flags of the commit command. It
> includes testing -i with and without staged changes, testing -o with and
> without staged changes, and testing -i and -o together.
A few suggestions to make things a bit more clear:
- tell that -i is a synonymous for --include and -o for --only
- tell if there are already tests for these options
- tell why the tests you add are worth it if tests for an option already exist
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> ---
> t/t7501-commit-basic-functionality.sh | 90 +++++++++++++++++++++++++++
> 1 file changed, 90 insertions(+)
>
> diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
> index 3d8500a52e..71dc52ce3a 100755
> --- a/t/t7501-commit-basic-functionality.sh
> +++ b/t/t7501-commit-basic-functionality.sh
> @@ -76,6 +76,96 @@ test_expect_success 'nothing to commit' '
> test_must_fail git commit -m initial
> '
>
> +test_expect_success 'commit with -i fails with untracked files' '
> + test_when_finished "rm -rf testdir" &&
> + git init testdir &&
> + echo content >testdir/file.txt &&
> + test_must_fail git -C testdir commit -i file.txt -m initial
> +'
Existing tests didn't need a repo, so I am not sure it's worth
creating a testdir repo just for this test.
Also I am not sure this is the best place in the test script to add -i
and -o related tests. Here we are between a 'nothing to commit' test
and a '--dry-run fails with nothing to commit' and then more 'nothing
to commit' related tests. I think it would be better if all those
'nothing to commit' related tests could stay together.
> +test_expect_success 'commit with -i' '
> + echo content >bar &&
> + git add bar &&
> + git commit -m "bar" &&
Why is the above needed for this test?
> + echo content2 >bar &&
> + git commit -i bar -m "bar second"
> +'
> +
> +test_expect_success 'commit with -i multiple files' '
> + test_when_finished "git reset --hard" &&
> + echo content >bar &&
> + echo content >baz &&
> + echo content >saz &&
> + git add bar baz saz &&
> + git commit -m "bar baz saz" &&
Not sure why the above is needed here too.
> + echo content2 >bar &&
> + echo content2 >baz &&
> + echo content2 >saz &&
> + git commit -i bar saz -m "bar" &&
> + git diff --name-only >remaining &&
> + test_grep "baz" remaining
> +'
> +
> +test_expect_success 'commit with -i includes staged changes' '
> + test_when_finished "git reset --hard" &&
> + echo content >bar &&
> + git add bar &&
> + git commit -m "bar" &&
> +
> + echo content2 >bar &&
> + echo content2 >baz &&
> + git add baz &&
> + git commit -i bar -m "bar baz" &&
> + git diff --name-only >remaining &&
> + test_must_be_empty remaining &&
> + git diff --name-only --staged >remaining &&
> + test_must_be_empty remaining
> +'
> +
> +test_expect_success 'commit with -o' '
> + echo content >bar &&
> + git add bar &&
> + git commit -m "bar" &&
> + echo content2 >bar &&
> + git commit -o bar -m "bar again"
> +'
> +
> +test_expect_success 'commit with -o fails with untracked files' '
> + test_when_finished "rm -rf testdir" &&
> + git init testdir &&
> + echo content >testdir/bar &&
> + test_must_fail git -C testdir commit -o bar -m "bar"
> +'
> +
> +test_expect_success 'commit with -o exludes staged changes' '
s/exludes/excludes/
> + test_when_finished "git reset --hard" &&
> + echo content >bar &&
> + echo content >baz &&
> + git add . &&
> + git commit -m "bar baz" &&
> +
> + echo content2 >bar &&
> + echo content2 >baz &&
> + git add baz &&
> + git commit -o bar -m "bar" &&
> + git diff --name-only --staged >actual &&
> + test_grep "baz" actual
> +'
Thanks.
^ permalink raw reply
* [PATCH 2/2] t7501: Add test for amending commit to add signoff.
From: Ghanshyam Thakkar @ 2024-01-09 6:04 UTC (permalink / raw)
To: git; +Cc: Ghanshyam Thakkar
In-Reply-To: <20240109060417.1144647-2-shyamthakkar001@gmail.com>
This commit adds test for amending the latest commit to add
Signed-off-by trailer at the end of commit message.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t7501-commit-basic-functionality.sh | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 71dc52ce3a..35c69c3ddd 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -464,6 +464,24 @@ test_expect_success 'amend commit to fix author' '
'
+test_expect_success 'amend commit to add signoff' '
+
+ test_when_finished "rm -rf testdir" &&
+ git init testdir &&
+ echo content >testdir/file &&
+ git -C testdir add file &&
+ git -C testdir commit -m "file" &&
+ git -C testdir commit --amend --signoff &&
+ git -C testdir log -1 --pretty=format:%B >actual &&
+ (
+ echo file &&
+ echo &&
+ git -C testdir var GIT_COMMITTER_IDENT >ident &&
+ sed -e "s/>.*/>/" -e "s/^/Signed-off-by: /" ident
+ ) >expected &&
+ test_cmp expected actual
+'
+
test_expect_success 'amend commit to fix date' '
test_tick &&
--
2.43.0
^ permalink raw reply related
* [PATCH 1/2] t7501: Add tests for various index usages, -i and -o, of commit command.
From: Ghanshyam Thakkar @ 2024-01-09 6:04 UTC (permalink / raw)
To: git; +Cc: Ghanshyam Thakkar
In-Reply-To: <20240109060417.1144647-2-shyamthakkar001@gmail.com>
This commit adds tests for -i and -o flags of the commit command. It
includes testing -i with and without staged changes, testing -o with and
without staged changes, and testing -i and -o together.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
t/t7501-commit-basic-functionality.sh | 90 +++++++++++++++++++++++++++
1 file changed, 90 insertions(+)
diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..71dc52ce3a 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -76,6 +76,96 @@ test_expect_success 'nothing to commit' '
test_must_fail git commit -m initial
'
+test_expect_success 'commit with -i fails with untracked files' '
+ test_when_finished "rm -rf testdir" &&
+ git init testdir &&
+ echo content >testdir/file.txt &&
+ test_must_fail git -C testdir commit -i file.txt -m initial
+'
+
+test_expect_success 'commit with -i' '
+ echo content >bar &&
+ git add bar &&
+ git commit -m "bar" &&
+
+ echo content2 >bar &&
+ git commit -i bar -m "bar second"
+'
+
+test_expect_success 'commit with -i multiple files' '
+ test_when_finished "git reset --hard" &&
+ echo content >bar &&
+ echo content >baz &&
+ echo content >saz &&
+ git add bar baz saz &&
+ git commit -m "bar baz saz" &&
+
+ echo content2 >bar &&
+ echo content2 >baz &&
+ echo content2 >saz &&
+ git commit -i bar saz -m "bar" &&
+ git diff --name-only >remaining &&
+ test_grep "baz" remaining
+'
+
+test_expect_success 'commit with -i includes staged changes' '
+ test_when_finished "git reset --hard" &&
+ echo content >bar &&
+ git add bar &&
+ git commit -m "bar" &&
+
+ echo content2 >bar &&
+ echo content2 >baz &&
+ git add baz &&
+ git commit -i bar -m "bar baz" &&
+ git diff --name-only >remaining &&
+ test_must_be_empty remaining &&
+ git diff --name-only --staged >remaining &&
+ test_must_be_empty remaining
+'
+
+test_expect_success 'commit with -o' '
+ echo content >bar &&
+ git add bar &&
+ git commit -m "bar" &&
+ echo content2 >bar &&
+ git commit -o bar -m "bar again"
+'
+
+test_expect_success 'commit with -o fails with untracked files' '
+ test_when_finished "rm -rf testdir" &&
+ git init testdir &&
+ echo content >testdir/bar &&
+ test_must_fail git -C testdir commit -o bar -m "bar"
+'
+
+test_expect_success 'commit with -o exludes staged changes' '
+ test_when_finished "git reset --hard" &&
+ echo content >bar &&
+ echo content >baz &&
+ git add . &&
+ git commit -m "bar baz" &&
+
+ echo content2 >bar &&
+ echo content2 >baz &&
+ git add baz &&
+ git commit -o bar -m "bar" &&
+ git diff --name-only --staged >actual &&
+ test_grep "baz" actual
+'
+
+test_expect_success 'commit with both -i and -o fails' '
+ test_when_finished "git reset --hard" &&
+ echo content >bar &&
+ echo content >baz &&
+ git add . &&
+ git commit -m "bar baz" &&
+
+ echo content2 >bar &&
+ echo content2 >baz &&
+ test_must_fail git commit -i baz -o bar -m "bar"
+'
+
test_expect_success '--dry-run fails with nothing to commit' '
test_must_fail git commit -m initial --dry-run
'
--
2.43.0
^ permalink raw reply related
* [PATCH 0/2][GSOC] t7501: Add tests for various index usages, -i and -o, of commit command and amending commit to add signoff.
From: Ghanshyam Thakkar @ 2024-01-09 6:04 UTC (permalink / raw)
To: git; +Cc: Ghanshyam Thakkar
This patch series adds tests for various index usages, -i and -o, of commit
command and amending commit to add Signed-of-by trailer. This is in
reference to the comment added in commit 12ace0b2 which reads:
# FIXME: Test the various index usages, -i and -o, test reflog,
# signoff, hooks
Ghanshyam Thakkar (2):
t7501: Add tests for various index usages, -i and -o, of commit
command.
t7501: Add test for amending commit to add signoff.
t/t7501-commit-basic-functionality.sh | 108 ++++++++++++++++++++++++++
1 file changed, 108 insertions(+)
--
2.43.0
^ permalink raw reply
* [PATCH v2 1/1] completion: don't complete revs when --no-format-patch
From: Britton Leo Kerin @ 2024-01-09 1:08 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240109010830.458775-1-britton.kerin@gmail.com>
In this case the user has specifically said they don't want send-email
to run format-patch so revs aren't valid argument completions (and it's
likely revs and dirs do have some same names or prefixes as in
Documentation/MyFirstContribution.txt 'psuh').
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 185b47d802..c983f3b2ab 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1242,10 +1242,12 @@ __git_find_last_on_cmdline ()
while test $# -gt 1; do
case "$1" in
--show-idx) show_idx=y ;;
+ --) shift && break ;;
*) return 1 ;;
esac
shift
done
+ [ $# -eq 1 ] || return 1 # return 1 if we got wrong # of non-opts
local wordlist="$1"
while [ $c -gt "$__git_cmd_idx" ]; do
@@ -2429,7 +2431,9 @@ _git_send_email ()
return
;;
esac
- __git_complete_revlist
+ if [ "$(__git_find_last_on_cmdline -- "--format-patch --no-format-patch")" != "--no-format-patch" ]; then
+ __git_complete_revlist
+ fi
}
_git_stage ()
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/1] completion: don't complete revs when --no-format-patch
From: Britton Leo Kerin @ 2024-01-09 1:08 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <9627364b-c0c9-4b85-a81a-ba1ef0735c9a@smtp-relay.sendinblue.com>
Improve commit message
Britton Leo Kerin (1):
completion: don't complete revs when --no-format-patch
contrib/completion/git-completion.bash | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Range-diff against v1:
1: ff4d2e55e3 ! 1: e56dbbacd9 completion: don't comp revs when --no-format-patch
@@ Metadata
Author: Britton Leo Kerin <britton.kerin@gmail.com>
## Commit message ##
- completion: don't comp revs when --no-format-patch
+ completion: don't complete revs when --no-format-patch
In this case the user has specifically said they don't want send-email
to run format-patch so revs aren't valid argument completions (and it's
--
2.43.0
^ permalink raw reply
* [PATCH v2 0/1] completion: complete dir-type option args to am, format_patch
From: Britton Leo Kerin @ 2024-01-09 0:53 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin, Junio C Hamano
In-Reply-To: <4714b88d-df5b-4e37-a5d7-af5033cfb861@smtp-relay.sendinblue.com>
This revision adds missing double quotes to improve the completion
situation for paths with spaces in them, and adds some comments.
Britton Leo Kerin (1):
completion: dir-type optargs for am, format-patch
contrib/completion/git-completion.bash | 37 ++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
Range-diff against v1:
1: 2d788b0b18 ! 1: 5161304a92 completion: dir-type optargs for am, format-patch
@@ contrib/completion/git-completion.bash: __git_count_arguments ()
printf "%d" $c
}
-+
+# Complete actual dir (not pathspec), respecting any -C options.
+#
+# Usage: __git_complete_refs [<option>]...
@@ contrib/completion/git-completion.bash: __git_count_arguments ()
+ shift
+ done
+
++ # This rev-parse invocation amounts to a pwd which respects -C options
+ local context_dir=$(__git rev-parse --show-toplevel --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)
-+ [ -d "$context_dir" ] || return
++ [ -d "$context_dir" ] || return 1
+
-+ COMPREPLY=$(cd $context_dir 2>/dev/null && compgen -d -- "$cur_")
++ COMPREPLY=$(cd "$context_dir" 2>/dev/null && compgen -d -- "$cur_")
+}
-+
+
__git_whitespacelist="nowarn warn error error-all fix"
__git_patchformat="mbox stgit stgit-series hg mboxrd"
--
2.43.0
^ permalink raw reply
* [PATCH v2 1/1] completion: dir-type optargs for am, format-patch
From: Britton Leo Kerin @ 2024-01-09 0:53 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240109005303.444932-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 37 ++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 185b47d802..2b2b6c9738 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1356,6 +1356,29 @@ __git_count_arguments ()
printf "%d" $c
}
+# Complete actual dir (not pathspec), respecting any -C options.
+#
+# Usage: __git_complete_refs [<option>]...
+# --cur=<word>: The current dir to be completed. Defaults to the current word.
+__git_complete_dir ()
+{
+ local cur_="$cur"
+
+ while test $# != 0; do
+ case "$1" in
+ --cur=*) cur_="${1##--cur=}" ;;
+ *) return 1 ;;
+ esac
+ shift
+ done
+
+ # This rev-parse invocation amounts to a pwd which respects -C options
+ local context_dir=$(__git rev-parse --show-toplevel --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)
+ [ -d "$context_dir" ] || return 1
+
+ COMPREPLY=$(cd "$context_dir" 2>/dev/null && compgen -d -- "$cur_")
+}
+
__git_whitespacelist="nowarn warn error error-all fix"
__git_patchformat="mbox stgit stgit-series hg mboxrd"
__git_showcurrentpatch="diff raw"
@@ -1374,6 +1397,10 @@ _git_am ()
__gitcomp "$__git_whitespacelist" "" "${cur##--whitespace=}"
return
;;
+ --directory=*)
+ __git_complete_dir --cur="${cur##--directory=}"
+ return
+ ;;
--patch-format=*)
__gitcomp "$__git_patchformat" "" "${cur##--patch-format=}"
return
@@ -1867,7 +1894,17 @@ __git_format_patch_extra_options="
_git_format_patch ()
{
+ case "$prev,$cur" in
+ -o,*)
+ __git_complete_dir
+ return
+ ;;
+ esac
case "$cur" in
+ --output-directory=*)
+ __git_complete_dir --cur="${cur##--output-directory=}"
+ return
+ ;;
--thread=*)
__gitcomp "
deep shallow
--
2.43.0
^ permalink raw reply related
* Re: [Outreachy][PATCH v4] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Junio C Hamano @ 2024-01-08 22:32 UTC (permalink / raw)
To: René Scharfe
Cc: Achu Luma, git, chriscool, christian.couder, phillip.wood,
steadmon, me, Phillip Wood
In-Reply-To: <a087f57c-ce72-45c7-8182-f38d0aca9030@web.de>
René Scharfe <l.s.r@web.de> writes:
> Quite an improvement over v3! Now you only need to repeat the class
> names once.
>
> Can we do any better?
;-)
^ permalink raw reply
* [PATCH] fetch: add new config option fetch.all
From: Tamino Bauknecht @ 2024-01-08 21:13 UTC (permalink / raw)
To: git; +Cc: Tamino Bauknecht
In-Reply-To: <xmqqwmsjlou7.fsf@gitster.g>
Introduce a boolean configuration option fetch.all which allows to
fetch all available remotes by default. The config option can be
overridden by explicitly specifying a remote or by using --no-all.
The behavior for --all is unchanged and calling git-fetch with --all
and a remote will still result in an error.
Additionally, describe the configuration variable in the config
documentation and implement new tests to cover the expected behavior.
Also add --no-all to the command-line documentation of git-fetch.
Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
---
Thanks again for the amazing in-depth feedback, Junio and Eric.
I misread Documentation/technical/api-parse-options.txt and thought
OPT_BOOL will always change to 0 or 1, but looks like it also doesn't
touch the variable if no --[no-]all was given.
Your other suggestion also simplified the patch quite a bit, thanks.
I only added an additional line of comment to hopefully make it easier
to understand.
The unnecessary "|| return 1" was also removed.
Documentation/config/fetch.txt | 6 ++
Documentation/fetch-options.txt | 5 +-
builtin/fetch.c | 17 +++-
t/t5514-fetch-multiple.sh | 161 ++++++++++++++++++++++++++++++++
4 files changed, 186 insertions(+), 3 deletions(-)
diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index aea5b97477..d7dc461bd1 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -50,6 +50,12 @@ fetch.pruneTags::
refs. See also `remote.<name>.pruneTags` and the PRUNING
section of linkgit:git-fetch[1].
+fetch.all::
+ If true, fetch will attempt to update all available remotes.
+ This behavior can be overridden by passing `--no-all` or by
+ explicitly specifying one or more remote(s) to fetch from.
+ Defaults to false.
+
fetch.output::
Control how ref update status is printed. Valid values are
`full` and `compact`. Default value is `full`. See the
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index a1d6633a4f..54ebb4452e 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -1,5 +1,6 @@
---all::
- Fetch all remotes.
+--[no-]all::
+ Fetch all remotes. This overrides the configuration variable
+ `fetch.all`.
-a::
--append::
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a284b970ef..94bd12affd 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -102,6 +102,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
struct fetch_config {
enum display_format display_format;
+ int all;
int prune;
int prune_tags;
int show_forced_updates;
@@ -115,6 +116,11 @@ static int git_fetch_config(const char *k, const char *v,
{
struct fetch_config *fetch_config = cb;
+ if (!strcmp(k, "fetch.all")) {
+ fetch_config->all = git_config_bool(k, v);
+ return 0;
+ }
+
if (!strcmp(k, "fetch.prune")) {
fetch_config->prune = git_config_bool(k, v);
return 0;
@@ -2132,7 +2138,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
const char *bundle_uri;
struct string_list list = STRING_LIST_INIT_DUP;
struct remote *remote = NULL;
- int all = 0, multiple = 0;
+ int all = -1, multiple = 0;
int result = 0;
int prune_tags_ok = 1;
int enable_auto_gc = 1;
@@ -2337,11 +2343,20 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
fetch_bundle_uri(the_repository, bundle_uri, NULL))
warning(_("failed to fetch bundles from '%s'"), bundle_uri);
+ if (all < 0) {
+ /*
+ * no --[no-]all given;
+ * only use config option if no remote was explicitly specified
+ */
+ all = (!argc) ? config.all : 0;
+ }
+
if (all) {
if (argc == 1)
die(_("fetch --all does not take a repository argument"));
else if (argc > 1)
die(_("fetch --all does not make sense with refspecs"));
+
(void) for_each_remote(get_one_remote_for_fetch, &list);
/* do not do fetch_multiple() of one */
diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
index a95841dc36..25772c85c5 100755
--- a/t/t5514-fetch-multiple.sh
+++ b/t/t5514-fetch-multiple.sh
@@ -24,6 +24,15 @@ setup_repository () {
)
}
+setup_test_clone () {
+ test_dir="$1" &&
+ git clone one "$test_dir" &&
+ for r in one two three
+ do
+ git -C "$test_dir" remote add "$r" "../$r" || return 1
+ done
+}
+
test_expect_success setup '
setup_repository one &&
setup_repository two &&
@@ -209,4 +218,156 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
git fetch --multiple --jobs=0)
'
+create_fetch_all_expect () {
+ cat >expect <<-\EOF
+ one/main
+ one/side
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+ three/another
+ three/main
+ three/side
+ two/another
+ two/main
+ two/side
+ EOF
+}
+
+for fetch_all in true false
+do
+ test_expect_success "git fetch --all (works with fetch.all = $fetch_all)" '
+ test_dir="test_fetch_all_$fetch_all" &&
+ setup_test_clone "$test_dir" &&
+ (
+ cd "$test_dir" &&
+ git config fetch.all $fetch_all &&
+ git fetch --all &&
+ create_fetch_all_expect &&
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+ '
+done
+
+test_expect_success 'git fetch (fetch all remotes with fetch.all = true)' '
+ setup_test_clone test9 &&
+ (
+ cd test9 &&
+ git config fetch.all true &&
+ git fetch &&
+ git branch -r >actual &&
+ create_fetch_all_expect &&
+ test_cmp expect actual
+ )
+'
+
+create_fetch_one_expect () {
+ cat >expect <<-\EOF
+ one/main
+ one/side
+ origin/HEAD -> origin/main
+ origin/main
+ origin/side
+ EOF
+}
+
+test_expect_success 'git fetch one (explicit remote overrides fetch.all)' '
+ setup_test_clone test10 &&
+ (
+ cd test10 &&
+ git config fetch.all true &&
+ git fetch one &&
+ create_fetch_one_expect &&
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+'
+
+create_fetch_two_as_origin_expect () {
+ cat >expect <<-\EOF
+ origin/HEAD -> origin/main
+ origin/another
+ origin/main
+ origin/side
+ EOF
+}
+
+test_expect_success 'git config fetch.all false (fetch only default remote)' '
+ setup_test_clone test11 &&
+ (
+ cd test11 &&
+ git config fetch.all false &&
+ git remote set-url origin ../two &&
+ git fetch &&
+ create_fetch_two_as_origin_expect &&
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+'
+
+for fetch_all in true false
+do
+ test_expect_success "git fetch --no-all (fetch only default remote with fetch.all = $fetch_all)" '
+ test_dir="test_no_all_fetch_all_$fetch_all" &&
+ setup_test_clone "$test_dir" &&
+ (
+ cd "$test_dir" &&
+ git config fetch.all $fetch_all &&
+ git remote set-url origin ../two &&
+ git fetch --no-all &&
+ create_fetch_two_as_origin_expect &&
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+ '
+done
+
+test_expect_success 'git fetch --no-all (fetch only default remote without fetch.all)' '
+ setup_test_clone test12 &&
+ (
+ cd test12 &&
+ git config --unset-all fetch.all || true &&
+ git remote set-url origin ../two &&
+ git fetch --no-all &&
+ create_fetch_two_as_origin_expect &&
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'git fetch --all --no-all (fetch only default remote)' '
+ setup_test_clone test13 &&
+ (
+ cd test13 &&
+ git remote set-url origin ../two &&
+ git fetch --all --no-all &&
+ create_fetch_two_as_origin_expect &&
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'git fetch --no-all one (fetch only explicit remote)' '
+ setup_test_clone test14 &&
+ (
+ cd test14 &&
+ git fetch --no-all one &&
+ create_fetch_one_expect &&
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'git fetch --no-all --all (fetch all remotes)' '
+ setup_test_clone test15 &&
+ (
+ cd test15 &&
+ git fetch --no-all --all &&
+ create_fetch_all_expect &&
+ git branch -r >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.43.0
^ permalink raw reply related
* Re: Analyzing a corrupted index file
From: Junio C Hamano @ 2024-01-08 19:51 UTC (permalink / raw)
To: Nathan Manceaux-Panot; +Cc: git
In-Reply-To: <B38C5D82-33E3-4D10-8119-7E0D59DD3DA2@lemon.garden>
Nathan Manceaux-Panot <fresh.tree3651@lemon.garden> writes:
> I have a corrupted git index file, and am trying to read it by
> hand, to understand what's wrong with it. Are there any tools
> that'll let me parse the on-disk, binary version of the index
> file, to unpack it into a human-readable data structure?
"git ls-files" with its various options is probably the closest, but
even the command is not meant for "debugging the bits". It is more
about showing lower-level details of a working index file to help
diagnose the mismatch between end-user expectation and the reality
(e.g. the user says the conflicts were all resolved, an expert asks
to run "ls-files -u" and together they discover that there are paths
that are still unmerged).
^ permalink raw reply
* Re: Storing private config files in .git directory?
From: Marc Branchaud @ 2024-01-08 19:48 UTC (permalink / raw)
To: Stefan Haller, git
In-Reply-To: <8e344dee-f84e-4a2c-835a-406ee72d129b@haller-berlin.de>
On 2024-01-07 08:03, Stefan Haller wrote:
> Our git client (lazygit) has a need to store per-repo config files that
> override the global one, much like git itself. The easiest way to do
> that is to store those in a .git/lazygit.cfg file, and I'm wondering if
> there's any reason why this is a bad idea?
In a worktree (created by "git worktree"), .git is a file not a directory.
Worktrees are designed to each have their own .git directory, which you
can find with "git rev-parse --git-dir". If you just want a single,
repo-wide config file, not a per-worktree config, you probably want to
instead use "git rev-parse --git-common-dir" to find the "main" repo's
.git directory.
The problem of finding a worktree's .git directory goes away if you use
Git's own config system, though.
> Another alternative would be to store the config values in .git/config
> (that's the path taken by git gui, for example), but since our config
> file format is yaml, this would require translation. It would be trivial
> for scalar values such as int or string, but I'm not sure how well this
> would work for more complex settings like lists of objects.
>
> Any thoughts?
YAML is a horrid little format (hey, you asked for "thoughts"!), and
IIRC Git's config file format only supports multi-line values with
\-escaping and similar patterns, making it nearly impossible to directly
embed YAML in Git's config file. Ideally, if you do use Git's own
config then you really should just drop YAML altogether.
But you have a couple of options without going so far as translating all
the YAML constructs you use into git-config ones. For example, you
could replace all the newlines in a YAML blob with \n to make a
single-line value that you could store in Git's config file. That
complicates hand-editing the YAML though, if that's a use case you care
about.
But even if you replace all the newlines with \n, in my experience there
are always corner-case clashes when mixing file syntaxes (e.g. quoted
strings are often problematic, and maybe some of your YAML values are
themselves multi-line). If you want to use Git's own config file but
stick with YAML, and you really don't care about directly editing the
YAML, I suggest you encode the entire YAML blob in a robust single-line
format, like base64, and store/retrieve that using "git config".
You could still support hand-editing the YAML with a command like
"lazygit editconfig", too.
M.
^ permalink raw reply
* Re: [PATCH 0/1] completion: send-email: don't complete revs when --no-format-patch
From: Britton Kerin @ 2024-01-08 19:34 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <0e8a1572100faae72db54becefe19f6b@manjaro.org>
On Mon, Jan 8, 2024 at 12:40 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> On 2024-01-08 10:36, Britton Leo Kerin wrote:
> > Along the way I taught __git_find_last_on_cmdline to understand '--',
> > which
> > isn't stricly necessary but I think reads more clearly at the call
> > sites.
> > __git_find_on_cmdline could be changed to work the same, or this part
> > dropped
> > if people don't like it.
>
> If I may suggest, there's no need for a cover letter for a single patch.
> If you want to include some notes in the patch submission, which aren't
> supposed to be part of the commit summary, you can do that in the patch
> itself.
Ok thanks, I'll do it that way in future.
Britton
^ permalink raw reply
* Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership
From: Junio C Hamano @ 2024-01-08 19:18 UTC (permalink / raw)
To: Sören Krecker; +Cc: git, sunshine, j6t
In-Reply-To: <20240108173837.20480-2-soekkle@freenet.de>
Sören Krecker <soekkle@freenet.de> writes:
> Add domain/username in error message, if owner sid of repository and
> user sid are not equal on windows systems.
Will queue. "git am --whitespace=fix" noticed numerous whitespace
breakages in the patch, which has been corrected on the receiving
end. Please make sure your future patches will not have such
problems.
Thanks.
$ git am -s3c ./+sk-v7-mingw-ownership-report
Applying: mingw: give more details about unsafe directory's ownership
.git/rebase-apply/patch:23: trailing whitespace.
&pe_use);
.git/rebase-apply/patch:27: trailing whitespace.
ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
.git/rebase-apply/patch:45: trailing whitespace.
LPSTR str1, str2, str3, str4, to_free1 = NULL,
warning: 3 lines add whitespace errors.
.git/rebase-apply/patch:23: trailing whitespace.
&pe_use);
.git/rebase-apply/patch:27: trailing whitespace.
ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
.git/rebase-apply/patch:45: trailing whitespace.
LPSTR str1, str2, str3, str4, to_free1 = NULL,
warning: 3 lines applied after fixing whitespace errors.
Using index info to reconstruct a base tree...
M compat/mingw.c
Falling back to patching base and 3-way merge...
Auto-merging compat/mingw.c
^ permalink raw reply
* Re: Storing private config files in .git directory?
From: Konstantin Ryabitsev @ 2024-01-08 18:56 UTC (permalink / raw)
To: Stefan Haller; +Cc: git
In-Reply-To: <8e344dee-f84e-4a2c-835a-406ee72d129b@haller-berlin.de>
On Sun, Jan 07, 2024 at 02:03:20PM +0100, Stefan Haller wrote:
> Our git client (lazygit) has a need to store per-repo config files that
> override the global one, much like git itself. The easiest way to do
> that is to store those in a .git/lazygit.cfg file, and I'm wondering if
> there's any reason why this is a bad idea?
I have considered the same question for b4 as well, but I chose to just rely
on git's config file handling instead of any other option. There's a large
number of people who tend to deal with weird repository situations by blowing
away the entire repo and then recloning it. They may remember to back up the
.git/config file, but not really anything else.
So, that would be the only consideration against keeping anything in the .git
directory.
-K
^ permalink raw reply
* Re: [PATCH 1/1] completion: don't comp revs when --no-format-patch
From: Junio C Hamano @ 2024-01-08 18:39 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git
In-Reply-To: <ce76a31e-d435-477e-803c-92d0532174eb@smtp-relay.sendinblue.com>
>Subject: Re: [PATCH 1/1] completion: don't comp revs when --no-format-patch
I suspect "comp revs" -> "complete revs" is what you meant, but
if so, please spell it out to help folks who read "git shortlog --no-merges"
output, i.e. without help from the body of the log message.
^ permalink raw reply
* Re: [PATCH 1/1] completion: dir-type optargs for am, format-patch
From: Junio C Hamano @ 2024-01-08 18:37 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git
In-Reply-To: <8a8dfcfc-3369-42b8-8387-f1af33202b16@smtp-relay.sendinblue.com>
"Britton Leo Kerin" <britton.kerin@gmail.com> writes:
> + local context_dir=$(__git rev-parse --show-toplevel --show-prefix 2>/dev/null | paste -s -d '/' 2>/dev/null)
Is there a practical difference with the above with
context_dir=$(pwd)
other than that it will give an empty string outside a git working tree?
If not, I suspect
local inside
inside=$(__git rev-parse --is-inside-work-tree) &&
test "$inside" = true || return
local context_dir=$(pwd)
might be clearer on the intent.
> + [ -d "$context_dir" ] || return
> +
> + COMPREPLY=$(cd $context_dir 2>/dev/null && compgen -d -- "$cur_")
Can $context_dir contain $IFS whitespaces here?
> +}
^ permalink raw reply
* Re: Storing private config files in .git directory?
From: Junio C Hamano @ 2024-01-08 18:20 UTC (permalink / raw)
To: Stefan Haller; +Cc: git
In-Reply-To: <8e344dee-f84e-4a2c-835a-406ee72d129b@haller-berlin.de>
Stefan Haller <lists@haller-berlin.de> writes:
> Our git client (lazygit) has a need to store per-repo config files that
> override the global one, much like git itself. The easiest way to do
> that is to store those in a .git/lazygit.cfg file, and I'm wondering if
> there's any reason why this is a bad idea?
An obvious alternative is to have .lazygit directory next to .git directory
which would give you a bigger separation, which can cut both ways.
^ permalink raw reply
* Re: [PATCH] branch: clarify <oldbranch> term
From: Junio C Hamano @ 2024-01-08 18:04 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
In-Reply-To: <d38e5a18-4d85-48f3-bc8b-8ca02ea683a4@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> Since 52d59cc645 (branch: add a --copy (-c) option to go with --move
> (-m), 2017-06-18) <oldbranch> is used in more operations than just -m.
>
> Let's also clarify what we do if <oldbranch> is omitted.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> Documentation/git-branch.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 4395aa9354..233264549c 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -312,7 +312,8 @@ superproject's "origin/main", but tracks the submodule's "origin/main".
> option is omitted, the current HEAD will be used instead.
>
> <oldbranch>::
> - The name of an existing branch to rename.
> + The name of an existing branch. If this option is omitted,
> + the name of the current branch will be used instead.
Thanks.
Will queue this patch as is for now, but I suspect that in the
longer term it will help readers a lot to revamp the whole page.
The description for the "branch rename" operation, for example, is
split and partly repreated in three places, i.e.
* a paragraph in the DESCRIPTION that talks about <oldbranch>,
where readers will be helped by the same clarification as this
patch gives;
* -m/-M option description which is very sketchy and does not even
hint that they take <oldbranch> and <newbranch>; and
* <oldbranch> description as a separate bullet item in the same
list, which does not even hint that this is used by -m/-M.
which is unmanageable from writers' point of view, and hard to
follow from readers' point of view.
Such a rewrite may look like this:
* Trim down the DESCRIPTION explanation to enumerate "features"
offered, with pointers into the OPTIONS section using the option
names as hints, e.g.,
The command offers many features that work on branches,
depending on the options.
- The default mode of operation is to list (the --list option
explicitly calls for it, or the lack of other options to
trigger different mode).
- ...
- An existing branch can be renamed to a different name with
"-m/-M" options.
- ...
* Enhance the description in the OPTIONS section so that each
bullet item (e.g., "-m") gives everything the user wants to know
about that option, e.g.,
-m [<oldbranch>] <newbranch>::
-M [<oldbranch>] <newbranch>::
Rename <oldbranch> (defaults to the current branch) to
<newbranch>. If <newbranch> already exists, `-m` will error
out and renaming must be forced by using `-M` (in other
words, `-M` works as a short-hand for `-m --force`).
+
The contents reflog of the <oldbranch> will also be renamed to become
the reflog of the <newbranch>, and a reflog entry is added for
the renaming of the branch.
* Remove the non-option entries from the OPTIONS bullet list.
^ permalink raw reply
* Re: [PATCH v6 0/1] mingw: give more details about unsafe directory's
From: Dragan Simic @ 2024-01-08 17:51 UTC (permalink / raw)
To: Sören Krecker; +Cc: git, sunshine, j6t
In-Reply-To: <20240108173837.20480-1-soekkle@freenet.de>
On 2024-01-08 18:38, Sören Krecker wrote:
> I have processed the points raised.
If I may suggest, there's no need for a cover letter for a single patch.
If you want to include some notes in the patch submission, which aren't
supposed to be part of the commit summary, you can do that in the patch
itself.
> Sören Krecker (1):
> mingw: give more details about unsafe directory's ownership
>
> compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 57 insertions(+), 13 deletions(-)
>
>
> base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
^ permalink raw reply
* [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership
From: Sören Krecker @ 2024-01-08 17:38 UTC (permalink / raw)
To: git; +Cc: sunshine, j6t, Sören Krecker
In-Reply-To: <20240108173837.20480-1-soekkle@freenet.de>
Add domain/username in error message, if owner sid of repository and
user sid are not equal on windows systems.
Old 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 error message:
'''
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 (S-1-5-21-571067702-4104414259-3379520149-500)
but the current user is:
DESKTOP-L78JVA6/test (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
'''
Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 57 insertions(+), 13 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..d85fae3747 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,30 @@ 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)
+ 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);
@@ -2765,27 +2789,47 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
strbuf_addf(report, "'%s' is on a file system that does "
"not record ownership\n", path);
} else if (report) {
- LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
+ LPSTR str1, str2, str3, str4, to_free1 = NULL,
+ to_free3 = NULL, to_local_free2 = NULL,
+ to_local_free4 = NULL;
- if (ConvertSidToStringSidA(sid, &str1))
+ if (user_sid_to_user_name(sid, &str1))
to_free1 = str1;
else
str1 = "(inconvertible)";
-
- if (!current_user_sid)
- str2 = "(none)";
- else if (!IsValidSid(current_user_sid))
- str2 = "(invalid)";
- else if (ConvertSidToStringSidA(current_user_sid, &str2))
- to_free2 = str2;
+ if (ConvertSidToStringSidA(sid, &str2))
+ to_local_free2 = str2;
else
str2 = "(inconvertible)";
+
+ if (!current_user_sid) {
+ str3 = "(none)";
+ str4 = "(none)";
+ }
+ else if (!IsValidSid(current_user_sid)) {
+ str3 = "(invalid)";
+ str4 = "(invalid)";
+ } else {
+ if (user_sid_to_user_name(current_user_sid,
+ &str3))
+ to_free3 = str3;
+ else
+ str3 = "(inconvertible)";
+ if (ConvertSidToStringSidA(current_user_sid,
+ &str4))
+ to_local_free4 = str4;
+ else
+ str4 = "(inconvertible)";
+ }
strbuf_addf(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);
+ "\t%s (%s)\nbut the current user is:\n"
+ "\t%s (%s)\n",
+ path, str1, str2, str3, str4);
+ free(to_free1);
+ LocalFree(to_local_free2);
+ free(to_free3);
+ LocalFree(to_local_free4);
}
}
--
2.39.2
^ permalink raw reply related
* [PATCH v6 0/1] mingw: give more details about unsafe directory's
From: Sören Krecker @ 2024-01-08 17:38 UTC (permalink / raw)
To: git; +Cc: sunshine, j6t, Sören Krecker
In-Reply-To: <de9cf40a-1ad6-45fb-8b70-8b0c71a3bfbb@kdbg.org>
I have processed the points raised.
Sören Krecker (1):
mingw: give more details about unsafe directory's ownership
compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 57 insertions(+), 13 deletions(-)
base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
--
2.39.2
^ permalink raw reply
* [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject
From: mohitmarathe @ 2024-01-08 17:34 UTC (permalink / raw)
To: git@vger.kernel.org
Cc: gitster@pobox.com, britton.kerin@gmail.com, peff@peff.net
Hello,
I'm Mohit, an undergrad from India, and I want to start contributing to the Git project. I have already built Git from source and finished `git psuh` tutorial. And I must say the "Hacking Git" documentation is great (very detailed and maybe exhaustive) and easy to follow. So I also read the topic on "microprojects", and while searching for one, I came across this message: https://public-inbox.org/git/xmqqjzpjsbjl.fsf@gitster.g/.
I want to work on this task (if it is not taken up already) as a microproject for GSoC.
Approach:
From what I understood, the idea is to *not* allow non-integer characters in the arguments by printing an error message. So we have to replace `atoi` with `strtol_i`, like it is done in this patch: https://public-inbox.org/git/xmqq5y181fx0.fsf_-_@gitster.g/.
There are some places where we want to continue to parse after the end of the integer (where `strspn` is used as mentioned by Junio), and based on Junio's suggestion, a new helper can be created like this:
> static inline int strtol_i2(char const *s, int base, int *result, char **endp)
> {
> long ul;
> char *dummy = NULL;
>
> if (!endp)
> endp = &dummy;
> errno = 0;
> ul = strtol(s, &endp, base);
> if (errno ||
> /*
> * if we are told to parse to the end of the string by
> * passing NULL to endp, it is an error to have any
> * remaining character after the digits.
> */
> (dummy && *dummy) ||
> endp == s || (int) ul != ul)
> return -1;
> *result = ul;
> return 0;
> }
So I searched for all the occurrences of `atoi(` (as suggested by Junio), and I found only two instances (both in `builtin/patch-id.c`) where it is followed by `strspn`. Is it safe to assume that this is the only place where we cannot directly replace `atoi` with `strtol_i`, or should I keep digging?
Also, this seems like a large change due to the number of files involved, while the documentation about the microproject emphasizes keeping the change small. Does it mean a small change per commit or a small change per Pull Request?
Thanks!
^ permalink raw reply
* Re: [GSOC][RFC] Heed core.bare from template config file when no command line override given, as a microproject.
From: Junio C Hamano @ 2024-01-08 17:32 UTC (permalink / raw)
To: Ghanshyam Thakkar
Cc: Elijah Newren, Christian Couder, git, johannes.schindelin
In-Reply-To: <CY7M09XT547N.2OOTI5APX9RIX@gmail.com>
"Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:
>> Specifically, the commit that introduced the comment never wanted to
>> honor core.bare in the template. I do not think I has core.bare in
>> mind when I wrote the comment, but I would have described it as the
>> same category as the repository format version, i.e. something you
>> would not want to copy, if I were pressed to clarify back then.
>
> Then I suppose this warrants updating the TODO comment in
> create_default_files(), which currently can be interpreted as this
> being a unwanted behavior. And also amending the testcases which
> currently display this as knwon breakage.
I obviously agree with that, after saying that I suspect 0f7443bd
comes from a misunderstanding ;-).
>> If somebody wants to always create a bare repository by having
>> core.bare=true in their template and if we wanted to honor it (which
>> I am dubious of the value of, by the way), I would think the right
>> place to do so would be way before create_default_files() is called.
>> When running "git init [$DIR]", long before calling init_db(), we
>> decide if we are about to create a bare repository and either create
>> $DIR or $DIR/.git. What is in the template, if we really wanted to
>> do so, should be read before that happens, no?
>
> That is what I proposed in my original email, after which I had a
> working solution which passed all the tests. That solution was indeed to
> check for core.bare in the template before we set GIT_DIR_ENVIRONMENT,
> which subsequently creates either $DIR or $DIR/.git as you described
> above.
Yeah, if this were still in soon after 4f629539 was written, then
such a change might have been a useful feature enhancement, but risk
of breaking people (third-party tools) who use the same template to
initialize both bare and non-bare repositories is there, so...
Thanks.
^ permalink raw reply
* Re: [PATCH v3] fetch: add new config option fetch.all
From: Junio C Hamano @ 2024-01-08 17:25 UTC (permalink / raw)
To: Tamino Bauknecht; +Cc: git
In-Reply-To: <20240106202352.7253-2-dev@tb6.eu>
Tamino Bauknecht <dev@tb6.eu> writes:
> This commit introduces the new boolean configuration option fetch.all
"This commit introduces the new boolean" -> "Introduce a boolean"
In general, our log messages are written as if we are giving an
order to explain what should happen to the codebase.
> which allows to fetch all available remotes by default. The config
> option can be overridden by explicitly specifying a remote or by using
> --no-all.
> The behavior for --all is unchanged and calling git-fetch with --all and
> a remote will still result in an error.
Good.
> The config option was also added to the config documentation and new
"The config option was also added to the config" -> "Describe the
configuration variable in the"
> tests cover the expected behavior.
> Additionally, --no-all was added to the command-line documentation of
"--no-all was added to" -> "add --no-all to".
> git-fetch.
>
> Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
> ---
> +fetch.all::
> + If true, fetch will attempt to update all available remotes.
> + This behavior can be overridden by passing `--no-all` or by
> + explicitly specifying one or more remote(s) to fetch from.
> + Defaults to false.
Good.
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index a1d6633a4f..5e70f6d2e4 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -1,5 +1,6 @@
> ---all::
> - Fetch all remotes.
> +--[no-]all::
> + Fetch all remotes. This overrides the configuration option
> + `fetch.all`.
"configuration option" -> "configuration variable".
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index a284b970ef..5a0b249c07 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -102,6 +102,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
>
> struct fetch_config {
> enum display_format display_format;
> + int all;
> int prune;
> int prune_tags;
> int show_forced_updates;
> @@ -115,6 +116,11 @@ static int git_fetch_config(const char *k, const char *v,
> {
> struct fetch_config *fetch_config = cb;
>
> + if (!strcmp(k, "fetch.all")) {
> + fetch_config->all = git_config_bool(k, v);
> + return 0;
> + }
> +
> if (!strcmp(k, "fetch.prune")) {
> fetch_config->prune = git_config_bool(k, v);
> return 0;
> @@ -2121,6 +2127,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> {
> struct fetch_config config = {
> .display_format = DISPLAY_FORMAT_FULL,
> + .all = -1,
> .prune = -1,
> .prune_tags = -1,
> .show_forced_updates = 1,
Not incorrect per-se, but I find it misleading, as there is no
reason to initialize it to -1. If left initialized to 0 (which
would be the default), and if we do not see "fetch.all", it will be
left to 0, which is the default value, and that is what you want.
> @@ -2132,7 +2139,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> const char *bundle_uri;
> struct string_list list = STRING_LIST_INIT_DUP;
> struct remote *remote = NULL;
> - int all = 0, multiple = 0;
> + int all = -1, multiple = 0;
> int result = 0;
> int prune_tags_ok = 1;
> int enable_auto_gc = 1;
> @@ -2148,7 +2155,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>
> struct option builtin_fetch_options[] = {
> OPT__VERBOSITY(&verbosity),
> - OPT_BOOL(0, "all", &all,
> + OPT_COUNTUP(0, "all", &all,
I do not think this change from BOOL to COUNTUP is warranted. Does
it trigger an error if you feed a variable that is initialized to -1
to OPT_BOOL()? If not, "initialize to -1 and let OPT_BOOL() set
either 0 or 1 when seeing a command line option" pattern would be
preferrable.
The idea is to initialize the "all" variable to -1 (unspecified) and
then if there is a command line option set it to either 0 or 1.
After the command line option parsing returns, we can tell:
-1: there was not --all and there was not --no-all. We need to
look at the configuration variable.
0: there was --no-all. Ignore the configuration.
1: there was --all. Ignore the configuration.
Use of COUNTUP can leave the variable set to say 2 or 3. There are
legitimate uses of COUNTUP (e.g., expressing verbosity levels via
multiple use of "-v -v"), but you want "--all" and "--all --all"
behave identically, so COUNTUP is not a good fit here.
> @@ -2337,11 +2344,18 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> fetch_bundle_uri(the_repository, bundle_uri, NULL))
> warning(_("failed to fetch bundles from '%s'"), bundle_uri);
>
OK. The following has to be a bit unusual and different from the
usual "we initialize a variable to its default, read into it from
the config, and then overwrite it from the command line option"
pattern because we want to ignore configured value upon factors
other than the command line option that corresponds to the
configuration variable (namely, even if --all or --no-all does not
appear on the command line, a remote makes the fetch.all
configuration ignored, without triggering an error).
> - if (all) {
> + if (all > 0) {
Hence, we need to catch only the case where "--all" was given
explicitly (i.e., all == -1 is not handled here). Good.
> if (argc == 1)
> die(_("fetch --all does not take a repository argument"));
> else if (argc > 1)
> die(_("fetch --all does not make sense with refspecs"));
> + }
> +
> + if (all > 0 || (config.all > 0 && !argc && all < 0)) {
Here, you can say config.all (not "config.all > 0") if you
initialized the .all member to 0 (remove the ".all = -1"
initialization, in other words).
> + /*
> + * Only use fetch.all config option if no remotes were
> + * explicitly given and if no --no-all was passed
> + */
> (void) for_each_remote(get_one_remote_for_fetch, &list);
>
> /* do not do fetch_multiple() of one */
But I suspect that it is easier to understand if you added this
if (all < 0)
/* no --[no-]all given. */
all = (!argc) ? config.all : 0;
before all of the above, and leave the rest of the original
untouched. In other words, when there is no command line option
"--[no-]all", we take "fetch.all" into consideration only when there
is no explicit remote. If there is an explicit remote and there is
no "--[no-]all", we can explicitly set all to 0 here.
And the remainder of the existing logic will work fine with all == 0
or all == 1 so we do not have to touch it.
> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> index a95841dc36..63cd730718 100755
> --- a/t/t5514-fetch-multiple.sh
> +++ b/t/t5514-fetch-multiple.sh
> @@ -24,6 +24,15 @@ setup_repository () {
> )
> }
>
> +setup_test_clone () {
> + test_dir="$1" &&
> + git clone one "$test_dir" &&
> + for r in one two three
> + do
> + git -C "$test_dir" remote add "$r" "../$r" || return 1
> + done
> +}
> +
> test_expect_success setup '
> setup_repository one &&
> setup_repository two &&
> @@ -209,4 +218,156 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
> git fetch --multiple --jobs=0)
> '
>
> +create_fetch_all_expect () {
> + cat >expect <<-\EOF || return 1
I saw there already was a comment on "|| return 1", which I agree
with.
Thanks.
^ 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