* Re: Add support for `git rebase -no-edit`
From: Junio C Hamano @ 2024-01-11 21:30 UTC (permalink / raw)
To: Chaitanya Tata; +Cc: git
In-Reply-To: <CADA7-FOE_81ze8hdaRGLPbipihnvFcEYfp9uwnPxPVxDepG4nA@mail.gmail.com>
Chaitanya Tata <chaitanya.tk17@gmail.com> writes:
> Hi,
>
> I have a feature request to add `--no-edit` option to `git rebase`
> like we do for `git commit`.
> The workflow I typically follow is:
>
> * `git commit -a --fixup=XXX`
> * `git rebase -i HEAD~15 --autosquash`
>
> But it requires closing the editor without any changes. I can
> workaround this using the `GIT_EDITOR` option, see [1]. But it would
> be good to have this built-in.
>
> Thoughts?
With what is in the 'master' branch, you do not have to say
interactive when you do not want to go interactive. I.e.
$ git rebase --autosquash HEAD~15
should work fine. Building Git from the source should not be too
hard.
By the way, make it a habit to pass non-option argument after dashed
options. It is easier for your readers to understand your command
line that way.
Thanks.
^ permalink raw reply
* RE: [DISCUSS] Introducing Rust into the Git project
From: rsbecker @ 2024-01-11 21:28 UTC (permalink / raw)
To: 'Trevor Gross'
Cc: 'brian m. carlson', 'Taylor Blau',
'Junio C Hamano', 'Dragan Simic', git
In-Reply-To: <CALNs47vfBH9u9B5B3tWRoEkJJiqne5067A4CFnZ3OaMVvz_gSg@mail.gmail.com>
On Thursday, January 11, 2024 3:08 PM, Trevor Gross wrote:
>On Wed, Jan 10, 2024 at 10:24 PM <rsbecker@nexbridge.com> wrote:
>>
>> There are a number of issues for porting gcc (and Go). The list is fairly long, but the
>summary of what I encountered directly (on the last funded effort of 3) is:
>> 1. There are C syntax constructs required to do anything useful (required for
>access to the OS API) on NonStop that are not in gcc. I can hand code the parser for
>that, but it would take time.
>> 2. The Big Endian x86 architecture is weird to gcc and making that work is not
>easy.
>> 3. There is no assembler on NonStop.
>> 4. The ELF header is very different from standard.
>> 5. The symbol table structure is radically different, so debugging would be (nearly)
>impossible or impractical. gdb was ported to account for the platform differences.
>> 6. The linkage structure is similar but different from standard.
>> 7. The external fixup structure is radically different.
>> 8. The loader does not work the same way, so there are required sections of the
>ELF files on NonStop that are not generated by gcc.
>>
>> There are more, but I just did not get to the point if hitting them. Part of my own
>issue is that I have expertise in parsing and semantic passes of compilers, but my
>code generation skills are not where I want them to be for taking on this effort. Our
>last funded attempt had a code generation expert and he gave up in frustration.
>>
>> If I was hired on to do this, it might have a chance, but at an estimate (not mine)
>of 4-5 person years for a gcc port, best case, my $DAYJOB will not permit it.
>>
>> If gcc could be ported to NonStop, it would solve so many problems. I have heard
>of numerous failed efforts beyond what was officially funded by various companies,
>so this is considered a high-risk project.
>
>Out of curiosity - does the Tandem compiler (assuming that is the correct name)
>have a backend that is usable as a library or via an IR?
>
>If so, maybe it would be possible to write a rustc_codegen_tandem backend like the
>three that exist (rustc_codegen_{llvm,gcc,cranelift}
>at [1]. GCC and cranelift are still under development). This way you sidestep a lot of
>the codegen-specific problems listed above.
>
>I am, of course, not suggesting this as a solution for git and am sure you would
>rather have GCC support. But I wonder how feasible this would be if Rust on
>NonStop is desired at some point.
The usable compilers and interpreters on NonStop are c89, c99 (what we use for git), c11, perl, and python3 (for the x86 only). The perl and python do not have sufficient modules to do what would be needed by git. The compilers are invoked using a CLI and are not callable using a library. gcc is, for all intents and purposes, not possible - so anything requiring gcc (for example, Rust), cannot be built. There is no back-end pluggable component for any of the compilers.
^ permalink raw reply
* [PATCH] push: improve consistency of output when "up to date"
From: Benji Kay via GitGitGadget @ 2024-01-11 21:27 UTC (permalink / raw)
To: git; +Cc: Benji Kay, okaybenji
From: okaybenji <okaybenji@gmail.com>
When one issues the pull command, one may see "Already up to date."
When issuing the push command, one may have seen "Everything up-to-date".
To improve consistency, "Everything up to date." is printed instead.
(The hyphens have been removed, and a period has been added.)
Signed-off-by: okaybenji <okaybenji@gmail.com>
---
push: improve consistency of output when "up to date"
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1638%2Fokaybenji%2Fup-to-date-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1638/okaybenji/up-to-date-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1638
transport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/transport.c b/transport.c
index bd7899e9bf5..c42cb4e58b4 100644
--- a/transport.c
+++ b/transport.c
@@ -1467,7 +1467,7 @@ int transport_push(struct repository *r,
if (porcelain && !push_ret)
puts("Done");
else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
- fprintf(stderr, "Everything up-to-date\n");
+ fprintf(stderr, "Everything up to date.\n");
done:
free_refs(local_refs);
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] git-p4: stop reaching into the refdb
From: Junio C Hamano @ 2024-01-11 21:20 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <33d6a062ec56be33ed50a42a420be0b023f6f4cf.1704980814.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The git-p4 tool creates a bunch of temporary branches that share a
> common prefix "refs/git-p4-tmp/". These branches get cleaned up via
> git-update-ref(1) after the import has finished. Once done, we try to
> manually remove the now supposedly-empty ".git/refs/git-p4-tmp/"
> directory.
>
> This last step can fail in case there still are any temporary branches
> around that we failed to delete because `os.rmdir()` refuses to delete a
> non-empty directory. It can thus be seen as kind of a sanity check to
> verify that we really did delete all temporary branches.
Wow, thanks for being very careful. I would just have said "there
is no need for such rmdir---what's a single empty unused directory
between friends, which will be reused later when you run 'git p4'
again?" without thinking things through.
> This is a modification in behaviour for the "files" backend because the
> empty directory does not get deleted anymore. But arguably we should not
> care about such implementation details of the ref backend anyway, and
> this should not cause any user-visible change in behaviour.
Independently, it would be sensible to improve the files backend so
that it removes a subdirectory outside the hierarchies that are
created by refs/files-backend.c:files_init_db() when it becomes
empty (#leftoverbits).
And such a change would also have triggered the error from
os.rmdir(), so this patch is doubly welcomed.
Thanks.
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> git-p4.py | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 0eb3bb4c47..3ea1c405e5 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -4251,7 +4251,8 @@ def run(self, args):
> if self.tempBranches != []:
> for branch in self.tempBranches:
> read_pipe(["git", "update-ref", "-d", branch])
> - os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"), self.tempBranchLocation))
> + if len(read_pipe(["git", "for-each-ref", self.tempBranchLocation])) > 0:
> + die("There are unexpected temporary branches")
>
> # Create a symbolic ref p4/HEAD pointing to p4/<branch> to allow
> # a convenient shortcut refname "p4".
^ permalink raw reply
* Re: [PATCH 2/2] completion: silence pseudoref existence check
From: Junio C Hamano @ 2024-01-11 21:03 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Stan Hu
In-Reply-To: <24563975fca8df6ae73917e9ee3534933d47c429.1704969119.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> In 44dbb3bf29 (completion: support pseudoref existence checks for
> reftables, 2023-12-19), we have extended the Bash completion script to
> support future ref backends better by using git-rev-parse(1) to check
> for pseudo-ref existence. This conversion has introduced a bug, because
> even though we pass `--quiet` to git-rev-parse(1) it would still output
> the resolved object ID of the ref in question if it exists.
>
> Fix this by redirecting its stdout to `/dev/null` and add a test that
> catches this behaviour. Note that the test passes even without the fix
> for the "files" backend because we parse pseudo refs via the filesystem
> directly in that case. But the test will fail with the "reftable"
> backend.
Thanks. Will queue.
^ permalink raw reply
* [PATCH] diffcore-delta: avoid ignoring final 'line' of file
From: Elijah Newren via GitGitGadget @ 2024-01-11 20:47 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Elijah Newren
From: Elijah Newren <newren@gmail.com>
hash_chars() would hash lines to integers, and store them in a spanhash,
but cut lines at 64 characters. Thus, whenever it reached 64 characters
or a newline, it would create a new spanhash. The problem is, the final
part of the file might not end 64 characters after the previous 'line'
and might not end with a newline. This could, for example, cause an
85-byte file with 12 lines and only the first character in the file
differing to appear merely 23% similar rather than the expected 97%.
Ensure the last line is included, and add a testcase that would have
caught this problem.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
diffcore-delta: avoid ignoring final 'line' of file
Found while experimenting with converting portions of diffcore-delta to
Rust.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1637%2Fnewren%2Ffix-diffcore-final-line-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1637/newren/fix-diffcore-final-line-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1637
diffcore-delta.c | 4 ++++
t/t4001-diff-rename.sh | 19 +++++++++++++++++++
2 files changed, 23 insertions(+)
diff --git a/diffcore-delta.c b/diffcore-delta.c
index c30b56e983b..7136c3dd203 100644
--- a/diffcore-delta.c
+++ b/diffcore-delta.c
@@ -159,6 +159,10 @@ static struct spanhash_top *hash_chars(struct repository *r,
n = 0;
accum1 = accum2 = 0;
}
+ if (n > 0) {
+ hashval = (accum1 + accum2 * 0x61) % HASHBASE;
+ hash = add_spanhash(hash, hashval, n);
+ }
QSORT(hash->data, (size_t)1ul << hash->alloc_log2, spanhash_cmp);
return hash;
}
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index 85be1367de6..29299acbce7 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -286,4 +286,23 @@ test_expect_success 'basename similarity vs best similarity' '
test_cmp expected actual
'
+test_expect_success 'last line matters too' '
+ test_write_lines a 0 1 2 3 4 5 6 7 8 9 >nonewline &&
+ printf "git ignores final up to 63 characters if not newline terminated" >>nonewline &&
+ git add nonewline &&
+ git commit -m "original version of file with no final newline" &&
+
+ # Change ONLY the first character of the whole file
+ test_write_lines b 0 1 2 3 4 5 6 7 8 9 >nonewline &&
+ printf "git ignores final up to 63 characters if not newline terminated" >>nonewline &&
+ git add nonewline &&
+ git mv nonewline still-no-newline &&
+ git commit -a -m "rename nonewline -> still-no-newline" &&
+ git diff-tree -r -M01 --name-status HEAD^ HEAD >actual &&
+ cat >expected <<-\EOF &&
+ R097 nonewline still-no-newline
+ EOF
+ test_cmp expected actual
+'
+
test_done
base-commit: 055bb6e9969085777b7fab83e3fee0017654f134
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Junio C Hamano @ 2024-01-11 20:29 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Stan Hu
In-Reply-To: <73406ca9c8f38ac2ad8f0e32d6d81f1566a6b4d1.1704969119.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The Bash completion script must not print anything to either stdout or
> stderr. Instead, it is only expected to populate certain variables.
> Tighten our `test_completion ()` test helper to verify this requirement.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/t9902-completion.sh | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index aa9a614de3..78cb93bea7 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -87,9 +87,11 @@ test_completion ()
> else
> sed -e 's/Z$//' |sort >expected
> fi &&
> - run_completion "$1" &&
> + run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
OK, there is a call to test_completion after cd and this is a
reasonable way to make sure we write to a known location.
I would have done the "run_completion should be totally silent"
check immediately after this line, as they logically are more
related to each other than to the two lines that implement "the
'out' file when sorted must match the expectation we just prepared",
but that is not a huge deal.
The filename "output" is something that may tempt folks who add more
tests to this file to use for their own purpose; because we use it
only three times locally here, it might be safer to give it a bit
more specific name, e.g., "run-completion-output" or something.
> sort out >out_sorted &&
> - test_cmp expected out_sorted
> + test_cmp expected out_sorted &&
> + test_must_be_empty "$TRASH_DIRECTORY"/output &&
> + rm "$TRASH_DIRECTORY"/output
> }
>
> # Test __gitcomp.
^ permalink raw reply
* [PATCH v2 2/2] t5541: remove lockfile creation
From: Justin Tobler via GitGitGadget @ 2024-01-11 20:24 UTC (permalink / raw)
To: git; +Cc: Jeff King, Patrick Steinhardt, Justin Tobler, Justin Tobler
In-Reply-To: <pull.1634.v2.git.1705004670.gitgitgadget@gmail.com>
From: Justin Tobler <jltobler@gmail.com>
To create error conditions, some tests set up reference locks by
directly creating its lockfile. While this works for the files reference
backend, this approach is incompatible with the reftable backend.
Refactor the test to create a d/f conflict via git-update-ref(1) instead
so that the test is reference backend agnostic.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
t/t5541-http-push-smart.sh | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index df758e187df..9a8bed6c32b 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -232,8 +232,9 @@ test_expect_success 'push --atomic fails on server-side errors' '
test_config -C "$d" http.receivepack true &&
up="$HTTPD_URL"/smart/atomic-branches.git &&
- # break ref updates for other on the remote site
- mkdir "$d/refs/heads/other.lock" &&
+ # Create d/f conflict to break ref updates for other on the remote site.
+ git -C "$d" update-ref -d refs/heads/other &&
+ git -C "$d" update-ref refs/heads/other/conflict HEAD &&
# add the new commit to other
git branch -f other collateral &&
@@ -241,18 +242,9 @@ test_expect_success 'push --atomic fails on server-side errors' '
# --atomic should cause entire push to be rejected
test_must_fail git push --atomic "$up" atomic other 2>output &&
- # the new branch should not have been created upstream
- test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
-
- # upstream should still reflect atomic2, the last thing we pushed
- # successfully
- git rev-parse atomic2 >expected &&
- # ...to other.
- git -C "$d" rev-parse refs/heads/other >actual &&
- test_cmp expected actual &&
-
- # the new branch should not have been created upstream
+ # The atomic and other branches should be created upstream.
test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
+ test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
# the failed refs should be indicated to the user
grep "^ ! .*rejected.* other -> other .*atomic transaction failed" output &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 1/2] t1401: remove lockfile creation
From: Justin Tobler via GitGitGadget @ 2024-01-11 20:24 UTC (permalink / raw)
To: git; +Cc: Jeff King, Patrick Steinhardt, Justin Tobler, Justin Tobler
In-Reply-To: <pull.1634.v2.git.1705004670.gitgitgadget@gmail.com>
From: Justin Tobler <jltobler@gmail.com>
To create error conditions, some tests set up reference locks by
directly creating its lockfile. While this works for the files reference
backend, this approach is incompatible with the reftable backend.
Refactor the test to create a d/f conflict via git-symbolic-ref(1)
instead so that the test is reference backend agnostic.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
t/t1401-symbolic-ref.sh | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index c7745e1bf69..c67e5c134d9 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -106,9 +106,8 @@ test_expect_success LONG_REF 'we can parse long symbolic ref' '
'
test_expect_success 'symbolic-ref reports failure in exit code' '
- test_when_finished "rm -f .git/HEAD.lock" &&
- >.git/HEAD.lock &&
- test_must_fail git symbolic-ref HEAD refs/heads/whatever
+ # Create d/f conflict to simulate failure.
+ test_must_fail git symbolic-ref refs/heads refs/heads/foo
'
test_expect_success 'symbolic-ref writes reflog entry' '
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/2] Generalize reference locking in tests
From: Justin Tobler via GitGitGadget @ 2024-01-11 20:24 UTC (permalink / raw)
To: git; +Cc: Jeff King, Patrick Steinhardt, Justin Tobler
In-Reply-To: <pull.1634.git.1704912750.gitgitgadget@gmail.com>
Hello again,
This is the second version my patch series that refactors two tests to no
longer be dependent on the files reference backend. Changes compared to v1:
* Removed reliance on fifo magic to trigger error conditions.
* d/f conflicts now used to create errors instead.
Thanks for taking a look!
Justin
Justin Tobler (2):
t1401: remove lockfile creation
t5541: remove lockfile creation
t/t1401-symbolic-ref.sh | 5 ++---
t/t5541-http-push-smart.sh | 18 +++++-------------
2 files changed, 7 insertions(+), 16 deletions(-)
base-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1634
Range-diff vs v1:
1: cb78b549e5e ! 1: 714f713ccec t1401: generalize reference locking
@@ Metadata
Author: Justin Tobler <jltobler@gmail.com>
## Commit message ##
- t1401: generalize reference locking
+ t1401: remove lockfile creation
- Some tests set up reference locks by directly creating the lockfile.
- While this works for the files reference backend, reftable reference
- locks operate differently and are incompatible with this approach.
- Refactor the test to use git-update-ref(1) to lock refs instead so that
- the test does not need to be aware of how the ref backend locks refs.
+ To create error conditions, some tests set up reference locks by
+ directly creating its lockfile. While this works for the files reference
+ backend, this approach is incompatible with the reftable backend.
+ Refactor the test to create a d/f conflict via git-symbolic-ref(1)
+ instead so that the test is reference backend agnostic.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
## t/t1401-symbolic-ref.sh ##
@@ t/t1401-symbolic-ref.sh: test_expect_success LONG_REF 'we can parse long symbolic ref' '
- test_cmp expect actual
'
--test_expect_success 'symbolic-ref reports failure in exit code' '
+ test_expect_success 'symbolic-ref reports failure in exit code' '
- test_when_finished "rm -f .git/HEAD.lock" &&
- >.git/HEAD.lock &&
- test_must_fail git symbolic-ref HEAD refs/heads/whatever
-+test_expect_success PIPE 'symbolic-ref reports failure in exit code' '
-+ mkfifo in out &&
-+ (git update-ref --stdin <in >out &) &&
-+
-+ exec 9>in &&
-+ exec 8<out &&
-+ test_when_finished "exec 9>&-" &&
-+ test_when_finished "exec 8<&-" &&
-+
-+ echo "start" >&9 &&
-+ echo "start: ok" >expected &&
-+ read line <&8 &&
-+ echo "$line" >actual &&
-+ test_cmp expected actual &&
-+
-+ echo "update HEAD refs/heads/foo" >&9 &&
-+
-+ echo "prepare" >&9 &&
-+ echo "prepare: ok" >expected &&
-+ read line <&8 &&
-+ echo "$line" >actual &&
-+ test_cmp expected actual &&
-+
-+ test_must_fail git symbolic-ref HEAD refs/heads/bar
++ # Create d/f conflict to simulate failure.
++ test_must_fail git symbolic-ref refs/heads refs/heads/foo
'
test_expect_success 'symbolic-ref writes reflog entry' '
2: 11fd5091d61 ! 2: f953a668c6a t5541: generalize reference locking
@@ Metadata
Author: Justin Tobler <jltobler@gmail.com>
## Commit message ##
- t5541: generalize reference locking
+ t5541: remove lockfile creation
- Some tests set up reference locks by directly creating the lockfile.
- While this works for the files reference backend, reftable reference
- locks operate differently and are incompatible with this approach.
- Generalize reference locking by preparing a reference transaction.
+ To create error conditions, some tests set up reference locks by
+ directly creating its lockfile. While this works for the files reference
+ backend, this approach is incompatible with the reftable backend.
+ Refactor the test to create a d/f conflict via git-update-ref(1) instead
+ so that the test is reference backend agnostic.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
@@ t/t5541-http-push-smart.sh: test_expect_success 'push --atomic fails on server-s
- # break ref updates for other on the remote site
- mkdir "$d/refs/heads/other.lock" &&
-+ mkfifo in out &&
-+ (git -C "$d" update-ref --stdin <in >out &) &&
-+
-+ exec 9>in &&
-+ exec 8<out &&
-+ test_when_finished "exec 9>&-" &&
-+ test_when_finished "exec 8<&-" &&
-+
-+ echo "start" >&9 &&
-+ echo "start: ok" >expected &&
-+ read line <&8 &&
-+ echo "$line" >actual &&
-+ test_cmp expected actual &&
-+
-+ echo "update refs/heads/other refs/heads/other" >&9 &&
-+
-+ # Prepare reference transaction on `other` reference to lock it and thus
-+ # break updates on the remote.
-+ echo "prepare" >&9 &&
-+ echo "prepare: ok" >expected &&
-+ read line <&8 &&
-+ echo "$line" >actual &&
-+ test_cmp expected actual &&
++ # Create d/f conflict to break ref updates for other on the remote site.
++ git -C "$d" update-ref -d refs/heads/other &&
++ git -C "$d" update-ref refs/heads/other/conflict HEAD &&
# add the new commit to other
git branch -f other collateral &&
+@@ t/t5541-http-push-smart.sh: test_expect_success 'push --atomic fails on server-side errors' '
+ # --atomic should cause entire push to be rejected
+ test_must_fail git push --atomic "$up" atomic other 2>output &&
+
+- # the new branch should not have been created upstream
+- test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
+-
+- # upstream should still reflect atomic2, the last thing we pushed
+- # successfully
+- git rev-parse atomic2 >expected &&
+- # ...to other.
+- git -C "$d" rev-parse refs/heads/other >actual &&
+- test_cmp expected actual &&
+-
+- # the new branch should not have been created upstream
++ # The atomic and other branches should be created upstream.
+ test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
++ test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
+
+ # the failed refs should be indicated to the user
+ grep "^ ! .*rejected.* other -> other .*atomic transaction failed" output &&
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 0/2] Generalize reference locking in tests
From: Justin Tobler @ 2024-01-11 20:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Justin Tobler via GitGitGadget, git
In-Reply-To: <xmqqv8807ll1.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> The use of two fifos and avoiding deadlocking parent and child is
> tricky enough that it does feel that it warrants a helper function
> to do the common part of what these two patches add.
To avoid the trickiness that comes with introducing fifos to these
tests, in the next patch version I've followed the suggestion of Peff to
instead create d/f conflicts to create an error condition. It appears
that these tests are not particular to the exact error condition being
triggered and therefore d/f conflicts are much simpler to produce.
Thanks,
Justin
On Wed, Jan 10, 2024 at 6:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Justin Tobler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > This approach is more verbose and may warrant consideration of implementing
> > an abstraction to further simplify this operation. There appears to be one
> > other existing test in t1400 that also follows this pattern. Being that
> > there is only a handful of affected tests the implemented approach may be
> > sufficient as is.
>
> The use of two fifos and avoiding deadlocking parent and child is
> tricky enough that it does feel that it warrants a helper function
> to do the common part of what these two patches add.
>
> I think I read t1401 patch carefully enough to understand what is
> going on, but I cannot yet claim the same for the other one.
>
> Thanks.
>
> > Justin Tobler (2):
> > t1401: generalize reference locking
> > t5541: generalize reference locking
> >
> > t/t1401-symbolic-ref.sh | 28 ++++++++++++++++++++++++----
> > t/t5541-http-push-smart.sh | 25 +++++++++++++++++++++++--
> > 2 files changed, 47 insertions(+), 6 deletions(-)
> >
> >
> > base-commit: 624eb90fa8f65a79396615f3c2842ac5a3743350
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1634%2Fjltobler%2Fjt%2Fbackend-generic-ref-lock-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1634/jltobler/jt/backend-generic-ref-lock-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1634
^ permalink raw reply
* Re: [PATCH 2/2] t5541: generalize reference locking
From: Justin Tobler @ 2024-01-11 20:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Justin Tobler via GitGitGadget, git
In-Reply-To: <xmqqa5pbhfkw.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
>>
>>> From: Justin Tobler <jltobler@gmail.com>
>>>
>>> Some tests set up reference locks by directly creating the lockfile.
>>> While this works for the files reference backend, reftable reference
>>> locks operate differently and are incompatible with this approach.
>>> Generalize reference locking by preparing a reference transaction.
>>
>> As with the first patch, I think we could use d/f conflicts to get the
>> same effect. Perhaps something like this:
>
> Thanks for a great alternative. I agree that avoiding fifo indeed
> is a better way to go.
For this patch, in the next version, I have also followed Peff's
suggestion to create d/f conflicts to trigger an error condition instead
of using fifos.
Thanks to everyone for the feedback!
Justin
On Thu, Jan 11, 2024 at 12:48 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
> >
> >> From: Justin Tobler <jltobler@gmail.com>
> >>
> >> Some tests set up reference locks by directly creating the lockfile.
> >> While this works for the files reference backend, reftable reference
> >> locks operate differently and are incompatible with this approach.
> >> Generalize reference locking by preparing a reference transaction.
> >
> > As with the first patch, I think we could use d/f conflicts to get the
> > same effect. Perhaps something like this:
>
> Thanks for a great alternative. I agree that avoiding fifo indeed
> is a better way to go.
^ permalink raw reply
* Re: [PATCH 1/2] t1401: generalize reference locking
From: Justin Tobler @ 2024-01-11 20:19 UTC (permalink / raw)
To: Jeff King; +Cc: Justin Tobler via GitGitGadget, git
In-Reply-To: <20240111071329.GC48154@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> It looks like you re-create this situation in a backend-agnostic way by
> having two simultaneous updates that conflict on the lock (but don't
> care how that lock is implemented).
>
> That works, but I think we could keep it simple. This test doesn't care
> about the exact error condition we create. The point was just to die in
> create_symref() and make sure the exit code was propagated.
As you mentioned, the original intent was to recreate the same error
condition in a reference backend agnostic manner. Since the test doesn't
care about the exact error condition being used, I agree that creating a
d/f conflict instead is a much simpler and better approach. In the next
patch version I've updated the test in t1401 to use git-symbolic-ref(1)
to produce a d/f error.
Thanks,
Justin
On Thu, Jan 11, 2024 at 1:13 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Jan 10, 2024 at 06:52:29PM +0000, Justin Tobler via GitGitGadget wrote:
>
> > From: Justin Tobler <jltobler@gmail.com>
> >
> > Some tests set up reference locks by directly creating the lockfile.
> > While this works for the files reference backend, reftable reference
> > locks operate differently and are incompatible with this approach.
> > Refactor the test to use git-update-ref(1) to lock refs instead so that
> > the test does not need to be aware of how the ref backend locks refs.
>
> It looks like you re-create this situation in a backend-agnostic way by
> having two simultaneous updates that conflict on the lock (but don't
> care how that lock is implemented).
>
> That works, but I think we could keep it simple. This test doesn't care
> about the exact error condition we create. The point was just to die in
> create_symref() and make sure the exit code was propagated. So something
> like this would work:
>
> $ git symbolic-ref refs/heads refs/heads/foo
> error: unable to write symref for refs/heads: Is a directory
>
> (note that you get a different error message if the refs are packed,
> since there we can notice the d/f conflict manually).
>
> There may be other ways to stimulate a failure. I thought "symbolic-ref
> HEAD refs/heads/.invalid" might work, but sadly the refname format check
> happens earlier.
>
> I think it is worth avoiding the fifo magic if we can. It's complicated,
> and it means that not all platforms run the test.
>
> -Peff
^ permalink raw reply
* Re: [DISCUSS] Introducing Rust into the Git project
From: Trevor Gross @ 2024-01-11 20:07 UTC (permalink / raw)
To: rsbecker; +Cc: brian m. carlson, Taylor Blau, Junio C Hamano, Dragan Simic, git
In-Reply-To: <00a801da443d$b1539670$13fac350$@nexbridge.com>
On Wed, Jan 10, 2024 at 10:24 PM <rsbecker@nexbridge.com> wrote:
>
> There are a number of issues for porting gcc (and Go). The list is fairly long, but the summary of what I encountered directly (on the last funded effort of 3) is:
> 1. There are C syntax constructs required to do anything useful (required for access to the OS API) on NonStop that are not in gcc. I can hand code the parser for that, but it would take time.
> 2. The Big Endian x86 architecture is weird to gcc and making that work is not easy.
> 3. There is no assembler on NonStop.
> 4. The ELF header is very different from standard.
> 5. The symbol table structure is radically different, so debugging would be (nearly) impossible or impractical. gdb was ported to account for the platform differences.
> 6. The linkage structure is similar but different from standard.
> 7. The external fixup structure is radically different.
> 8. The loader does not work the same way, so there are required sections of the ELF files on NonStop that are not generated by gcc.
>
> There are more, but I just did not get to the point if hitting them. Part of my own issue is that I have expertise in parsing and semantic passes of compilers, but my code generation skills are not where I want them to be for taking on this effort. Our last funded attempt had a code generation expert and he gave up in frustration.
>
> If I was hired on to do this, it might have a chance, but at an estimate (not mine) of 4-5 person years for a gcc port, best case, my $DAYJOB will not permit it.
>
> If gcc could be ported to NonStop, it would solve so many problems. I have heard of numerous failed efforts beyond what was officially funded by various companies, so this is considered a high-risk project.
Out of curiosity - does the Tandem compiler (assuming that is the
correct name) have a backend that is usable as a library or via an IR?
If so, maybe it would be possible to write a rustc_codegen_tandem
backend like the three that exist (rustc_codegen_{llvm,gcc,cranelift}
at [1]. GCC and cranelift are still under development). This way you
sidestep a lot of the codegen-specific problems listed above.
I am, of course, not suggesting this as a solution for git and am sure
you would rather have GCC support. But I wonder how feasible this
would be if Rust on NonStop is desired at some point.
[1]: https://github.com/rust-lang/rust/tree/062e7c6a951c1e4f33c0a6f6761755949cde15ec/compiler
^ permalink raw reply
* Re: Add support for `git rebase -no-edit`
From: Chaitanya Tata @ 2024-01-11 20:07 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZaBHooq6ORZ3TTkL@nand.local>
Cheers,
Chaitanya.
On Fri, Jan 12, 2024 at 1:25 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> Hi Chaitanya,
>
> On Thu, Jan 11, 2024 at 10:55:47PM +0530, Chaitanya Tata wrote:
> > Hi,
> >
> > I have a feature request to add `--no-edit` option to `git rebase`
> > like we do for `git commit`.
> > The workflow I typically follow is:
> >
> > * `git commit -a --fixup=XXX`
> > * `git rebase -i HEAD~15 --autosquash`
> >
> > But it requires closing the editor without any changes. I can
> > workaround this using the `GIT_EDITOR` option, see [1]. But it would
> > be good to have this built-in.
>
> The easiest workaround would be setting GIT_EDITOR=true, which matches
> the recommendation in [1].
Thanks for the quick response.
>
> Short of that, you can't do a non-interactive rebase, since we rely on
> the todo list generated by interactive rebases in order for
> `--autosquash` to work.
IIUC, creating a todo list needs access to the file used in interactive
rebase?
> Presumably plumbing in a new `--[no-]edit` option would be fairly
> straightforward, and once that is done, the change boils down to just:
I am bit confused by this as this seems to contradict above statement,
I guess the below patch will only work without `--autosquash`? Sorry, not
familiar with git internals.
>
> index 3cc88d8a80..5235a003f2 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6169,7 +6169,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> struct todo_list new_todo = TODO_LIST_INIT;
> struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
> struct object_id oid = onto->object.oid;
> - int res;
> + int res = 0;
>
> repo_find_unique_abbrev_r(r, shortonto, &oid,
> DEFAULT_ABBREV);
> @@ -6197,8 +6197,9 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> return error(_("nothing to do"));
> }
>
> - res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
> - shortonto, flags);
> + if (!opts->edit)
> + res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
> + shortonto, flags);
> if (res == -1)
> return -1;
> else if (res == -2) {
>
> > [1] - https://stackoverflow.com/a/45783848
>
> Thanks,
> Taylor
^ permalink raw reply
* [PATCH v3] builtin/revert.c: refactor using an enum for cmd-action
From: Michael Lohmann @ 2024-01-11 20:06 UTC (permalink / raw)
To: gitster; +Cc: git, mi.al.lohmann, phillip.wood123, phillip.wood, wanja.hentze
In-Reply-To: <xmqqsf33fy3t.fsf@gitster.g>
This is done to avoid having to keep the char values in sync in
different places and also to get compiler warnings on non-exhaustive
switches.
In the rebase `action` enum there is the enumeration constant
`ACTION_NONE` which is not particularly descriptive, since it seems to
imply that no action should be taken. Instead it signals a start of a
revert/cherry-pick process, so here `ACTION_START` was chosen.
Co-authored-by: Wanja Henze <wanja.hentze@bevuta.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
On 11. Jan 2024, at 20:37, Junio C Hamano <gitster@pobox.com> wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> > I think ACTION_NONE was intended to covey that the user did not pass
> > one of the OPT_CMDMODE() options like "--continue" as there isn't a
> > "--start" option. I don't have a strong opinion between "_NONE" and
> > "_START".
>
> I agree with you why NONE is called as such. If "revert" does not
> take "--start" (I do not remember offhand), I would think it would
> be better to follow suit.
My point was that yes, it might be in sync with what the user passes in
as arguments, but when I followed the code and saw lots of references to
ACTION_NONE I was puzzled, since my intuition of that name was that
_no action_ should be taken (which did not make sense to me).
So the (provocative) question is: Do we want to keep the variable name
in sync with some input parameters, or rather with the real action that
should be taken?
(Depending on the outcome of this discussion I would also prepare a
patch renaming it in builtin/rebase.c)
What do you think about this version which keeps the
`if (cmd != ACTION_START)` in favour of the `goto` and instead of the
constant if/else checks for the `verify_opt_compatible` (with the
`assert` at the last one) here is one version with a
`get_cmd_flag`-function (I am not that happy with the name...) that has
a `switch` and it has a runtime error handling with `BUG`.
I think it is the most concise of the options so far.
Ciao
Michael
builtin/revert.c | 65 +++++++++++++++++++++++++++---------------------
1 file changed, 37 insertions(+), 28 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 89821bab95..891aa1d720 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -20,6 +20,14 @@
* Copyright (c) 2005 Junio C Hamano
*/
+enum action {
+ ACTION_START = 0,
+ ACTION_CONTINUE,
+ ACTION_SKIP,
+ ACTION_ABORT,
+ ACTION_QUIT,
+};
+
static const char * const revert_usage[] = {
N_("git revert [--[no-]edit] [-n] [-m <parent-number>] [-s] [-S[<keyid>]] <commit>..."),
N_("git revert (--continue | --skip | --abort | --quit)"),
@@ -33,6 +41,17 @@ static const char * const cherry_pick_usage[] = {
NULL
};
+static char* get_cmd_optionname(enum action cmd)
+{
+ switch (cmd) {
+ case ACTION_CONTINUE: return "--continue";
+ case ACTION_SKIP: return "--skip";
+ case ACTION_ABORT: return "--abort";
+ case ACTION_QUIT: return "--quit";
+ case ACTION_START: BUG("no commandline flag for ACTION_START");
+ }
+}
+
static const char *action_name(const struct replay_opts *opts)
{
return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
@@ -85,12 +104,12 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
const char *cleanup_arg = NULL;
- int cmd = 0;
+ enum action cmd = ACTION_START;
struct option base_options[] = {
- OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
- OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
- OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
- OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), 's'),
+ OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), ACTION_QUIT),
+ OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), ACTION_CONTINUE),
+ OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), ACTION_ABORT),
+ OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), ACTION_SKIP),
OPT_CLEANUP(&cleanup_arg),
OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
@@ -144,20 +163,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
}
/* Check for incompatible command line arguments */
- if (cmd) {
- char *this_operation;
- if (cmd == 'q')
- this_operation = "--quit";
- else if (cmd == 'c')
- this_operation = "--continue";
- else if (cmd == 's')
- this_operation = "--skip";
- else {
- assert(cmd == 'a');
- this_operation = "--abort";
- }
-
- verify_opt_compatible(me, this_operation,
+ if (cmd != ACTION_START)
+ verify_opt_compatible(me, get_cmd_optionname(cmd),
"--no-commit", opts->no_commit,
"--signoff", opts->signoff,
"--mainline", opts->mainline,
@@ -168,7 +175,6 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
"--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
"--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
NULL);
- }
if (!opts->strategy && opts->default_strategy) {
opts->strategy = opts->default_strategy;
@@ -183,9 +189,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
"--edit", opts->edit > 0,
NULL);
- if (cmd) {
- opts->revs = NULL;
- } else {
+ if (cmd == ACTION_START) {
struct setup_revision_opt s_r_opt;
opts->revs = xmalloc(sizeof(*opts->revs));
repo_init_revisions(the_repository, opts->revs, NULL);
@@ -198,6 +202,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
memset(&s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.assume_dashdash = 1;
argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
+ } else {
+ opts->revs = NULL;
}
if (argc > 1)
@@ -210,19 +216,22 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
free(options);
- if (cmd == 'q') {
+ switch (cmd) {
+ case ACTION_QUIT: {
int ret = sequencer_remove_state(opts);
if (!ret)
remove_branch_state(the_repository, 0);
return ret;
}
- if (cmd == 'c')
+ case ACTION_CONTINUE:
return sequencer_continue(the_repository, opts);
- if (cmd == 'a')
+ case ACTION_ABORT:
return sequencer_rollback(the_repository, opts);
- if (cmd == 's')
+ case ACTION_SKIP:
return sequencer_skip(the_repository, opts);
- return sequencer_pick_revisions(the_repository, opts);
+ case ACTION_START:
+ return sequencer_pick_revisions(the_repository, opts);
+ }
}
int cmd_revert(int argc, const char **argv, const char *prefix)
--
2.39.3 (Apple Git-145)
^ permalink raw reply related
* Re: Add support for `git rebase -no-edit`
From: Taylor Blau @ 2024-01-11 19:55 UTC (permalink / raw)
To: Chaitanya Tata; +Cc: git
In-Reply-To: <CADA7-FOE_81ze8hdaRGLPbipihnvFcEYfp9uwnPxPVxDepG4nA@mail.gmail.com>
Hi Chaitanya,
On Thu, Jan 11, 2024 at 10:55:47PM +0530, Chaitanya Tata wrote:
> Hi,
>
> I have a feature request to add `--no-edit` option to `git rebase`
> like we do for `git commit`.
> The workflow I typically follow is:
>
> * `git commit -a --fixup=XXX`
> * `git rebase -i HEAD~15 --autosquash`
>
> But it requires closing the editor without any changes. I can
> workaround this using the `GIT_EDITOR` option, see [1]. But it would
> be good to have this built-in.
The easiest workaround would be setting GIT_EDITOR=true, which matches
the recommendation in [1].
Short of that, you can't do a non-interactive rebase, since we rely on
the todo list generated by interactive rebases in order for
`--autosquash` to work.
Presumably plumbing in a new `--[no-]edit` option would be fairly
straightforward, and once that is done, the change boils down to just:
index 3cc88d8a80..5235a003f2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6169,7 +6169,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
struct todo_list new_todo = TODO_LIST_INIT;
struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
struct object_id oid = onto->object.oid;
- int res;
+ int res = 0;
repo_find_unique_abbrev_r(r, shortonto, &oid,
DEFAULT_ABBREV);
@@ -6197,8 +6197,9 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
return error(_("nothing to do"));
}
- res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
- shortonto, flags);
+ if (!opts->edit)
+ res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
+ shortonto, flags);
if (res == -1)
return -1;
else if (res == -2) {
> [1] - https://stackoverflow.com/a/45783848
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH v2] builtin/revert.c: refactor using an enum for cmd-action
From: Junio C Hamano @ 2024-01-11 19:50 UTC (permalink / raw)
To: Michael Lohmann; +Cc: phillip.wood123, git, phillip.wood, wanja.hentze
In-Reply-To: <20240111174727.55189-1-mi.al.lohmann@gmail.com>
Michael Lohmann <mi.al.lohmann@gmail.com> writes:
> struct option base_options[] = {
> - OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
> - OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
> - OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
> - OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), 's'),
> + OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), ACTION_QUIT),
> + OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), ACTION_CONTINUE),
> + OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), ACTION_ABORT),
> + OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), ACTION_SKIP),
> OPT_CLEANUP(&cleanup_arg),
> OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
> OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
I actually do not terribly mind reusing the single letter option
(e.g. 'x' in "-x") and the command mode, as it is descriptive
enough. The argument that an enum allows compilers warn about
non-exhausitve switches is valid if we have such a switch.
> + switch (cmd) {
> + case ACTION_START:
> + goto skip_opt_compatibility_verification;
> + case ACTION_QUIT:
> + this_operation = "--quit";
> + break;
> + case ACTION_CONTINUE:
> + this_operation = "--continue";
> + break;
> + case ACTION_SKIP:
> + this_operation = "--skip";
> + break;
> + case ACTION_ABORT:
> + this_operation = "--abort";
> + break;
> }
>
> + verify_opt_compatible(me, this_operation,
And indeed the if/elseif cascade in the original is easier to ensure
its exhaustiveness by turning it into a switch.
HOWEVER.
If I were writing this patch, I would rather do it like so:
this_operation = NULL;
switch (cmd) {
case 'q':
this_operation = '--quit";
break;
...
case 'a':
this_operation = '--abort";
break;
default: /* everything else is compatible with others */
break;
}
if (this_operation)
verify_opt_compatible(me, this_operation, ...);
Use of enum is optional; I just didn't like too much churning to
illustrate a single idea (here, the single idea being "switch is
more appropriate then if/else cascade in this case"), and I think
this is easier to read than with enums [*].
Side note: After all the single letter option names are
meant to be mnemonic. "case 'q'" is just as descriptive as
"case ACTION_QUIT" in the context of this switch statement.
HTH.
^ permalink raw reply
* Re: [PATCH] builtin/revert.c: refactor using an enum for cmd-action
From: Junio C Hamano @ 2024-01-11 19:37 UTC (permalink / raw)
To: Phillip Wood; +Cc: Michael Lohmann, git, Wanja Henze
In-Reply-To: <1ed0e249-dab9-4cf3-9b76-c8797b97c9c5@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> I think ACTION_NONE was intended to covey that the user did not pass
> one of the OPT_CMDMODE() options like "--continue" as there isn't a
> "--start" option. I don't have a strong opinion between "_NONE" and
> "_START".
I agree with you why NONE is called as such. If "revert" does not
take "--start" (I do not remember offhand), I would think it would
be better to follow suit.
^ permalink raw reply
* Re: [PATCH 2/2] t5541: generalize reference locking
From: Junio C Hamano @ 2024-01-11 18:47 UTC (permalink / raw)
To: Jeff King; +Cc: Justin Tobler via GitGitGadget, git, Justin Tobler
In-Reply-To: <20240111072828.GD48154@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Jan 10, 2024 at 06:52:30PM +0000, Justin Tobler via GitGitGadget wrote:
>
>> From: Justin Tobler <jltobler@gmail.com>
>>
>> Some tests set up reference locks by directly creating the lockfile.
>> While this works for the files reference backend, reftable reference
>> locks operate differently and are incompatible with this approach.
>> Generalize reference locking by preparing a reference transaction.
>
> As with the first patch, I think we could use d/f conflicts to get the
> same effect. Perhaps something like this:
Thanks for a great alternative. I agree that avoiding fifo indeed
is a better way to go.
^ permalink raw reply
* Re: [PATCH] commit-graph: retain commit slab when closing NULL commit_graph
From: Junio C Hamano @ 2024-01-11 18:35 UTC (permalink / raw)
To: Jeff King; +Cc: Taylor Blau, git, Scott Leggett
In-Reply-To: <20240111075313.GF48154@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Jan 10, 2024 at 08:38:18AM -0800, Junio C Hamano wrote:
>
>> > It should be easy-ish to iterate through the slab and look at the
>> > commits that are mentioned in it. Though maybe not? Each commit knows
>> > its slab-id, but I'm not sure if we have a master list of commits to go
>> > the other way.
>>
>> We have table of all in-core objects, don't we?
>
> Oh, duh. Yes, we could iterate over obj_hash. I do think the "on demand"
> version I showed later in the message is better, though, as the work
> both scales with the number of affected commits (rather than the total
> number of objects) and can be done lazily (so callers that are not buggy
> pay no price at all).
Yeah, it is far more desirable than scanning obj_hash if we can do
the right thing on lazily.
> So what if we just tried harder to look it up in the graph file (rather
> than the slab) when we see COMMIT_NOT_FROM_GRAPH? And indeed, we even
> have a function to do this already!
;-)
> but:
> ...
> I somehow sniped myself into thinking about it more, but that has only
> reinforced my feeling that I'm afraid to touch it. ;)
Thanks for a nice summary.
^ permalink raw reply
* [PATCH v2] builtin/revert.c: refactor using an enum for cmd-action
From: Michael Lohmann @ 2024-01-11 17:47 UTC (permalink / raw)
To: phillip.wood123; +Cc: git, mi.al.lohmann, phillip.wood, wanja.hentze
In-Reply-To: <1ed0e249-dab9-4cf3-9b76-c8797b97c9c5@gmail.com>
This is done to avoid having to keep the char values in sync in
different places and also to get compiler warnings on non-exhaustive
switches.
In the rebase `action` enum there is the enumeration constant
`ACTION_NONE` which is not particularly descriptive, since it seems to
imply that no action should be taken. Instead it signals a start of a
revert/cherry-pick process, so here `ACTION_START` was chosen.
Co-authored-by: Wanja Henze <wanja.hentze@bevuta.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
Hi Phillip!
Thanks for the feedback!
On 11/01/2024 16:57, Phillip Wood wrote:
> I can see the attraction of using an exhaustive switch() here but as
> we don't want to do anything in the _START case it gets a bit messy
> with the extra conditional below. I wonder if we'd be better to
> replace "if (cmd) {" with "if (cmd != REVERT_ACTION_START) {".
> Alternatively if you want to stick with the switch then declaring
> "this_operation" at the beginning of the function would mean you can
> get rid of the new "{}" block.
> The extra indentation here is unfortunate as some of the lines are
> rather long already. In the current code it is clear that we only call
> verify_opt_compatible() when cmd is non-nul, I think it would be
> clearer to use "if (cmd != REVERT_ACTION_START)" here.
Totally agreed - an alternative to the `if` would be a `goto` (see this
version of the patch). This would keep the benefit of the exhaustive
switch, but I don't know if that would fit the style used in this
project... (at least it is a jump forward...)
Changes compared to the previous patch:
- initialize `this_operation` pointer with NULL instead of 0
- drop "REVERT_" prefix of enum and its members
- declare `this_operation` at the toplevel to get rid of codeblock
- skip the verify_opt_compatible in case of ACTION_START with a `goto`
Best wishes
Michael
builtin/revert.c | 90 ++++++++++++++++++++++++++++--------------------
1 file changed, 53 insertions(+), 37 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 89821bab95..19e6653f99 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -20,6 +20,14 @@
* Copyright (c) 2005 Junio C Hamano
*/
+enum action {
+ ACTION_START = 0,
+ ACTION_CONTINUE,
+ ACTION_SKIP,
+ ACTION_ABORT,
+ ACTION_QUIT,
+};
+
static const char * const revert_usage[] = {
N_("git revert [--[no-]edit] [-n] [-m <parent-number>] [-s] [-S[<keyid>]] <commit>..."),
N_("git revert (--continue | --skip | --abort | --quit)"),
@@ -85,12 +93,13 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
const char *cleanup_arg = NULL;
- int cmd = 0;
+ char *this_operation = NULL;
+ enum action cmd = ACTION_START;
struct option base_options[] = {
- OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
- OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
- OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
- OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), 's'),
+ OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), ACTION_QUIT),
+ OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), ACTION_CONTINUE),
+ OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), ACTION_ABORT),
+ OPT_CMDMODE(0, "skip", &cmd, N_("skip current commit and continue"), ACTION_SKIP),
OPT_CLEANUP(&cleanup_arg),
OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
@@ -144,32 +153,36 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
}
/* Check for incompatible command line arguments */
- if (cmd) {
- char *this_operation;
- if (cmd == 'q')
- this_operation = "--quit";
- else if (cmd == 'c')
- this_operation = "--continue";
- else if (cmd == 's')
- this_operation = "--skip";
- else {
- assert(cmd == 'a');
- this_operation = "--abort";
- }
-
- verify_opt_compatible(me, this_operation,
- "--no-commit", opts->no_commit,
- "--signoff", opts->signoff,
- "--mainline", opts->mainline,
- "--strategy", opts->strategy ? 1 : 0,
- "--strategy-option", opts->xopts.nr ? 1 : 0,
- "-x", opts->record_origin,
- "--ff", opts->allow_ff,
- "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
- "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
- NULL);
+ switch (cmd) {
+ case ACTION_START:
+ goto skip_opt_compatibility_verification;
+ case ACTION_QUIT:
+ this_operation = "--quit";
+ break;
+ case ACTION_CONTINUE:
+ this_operation = "--continue";
+ break;
+ case ACTION_SKIP:
+ this_operation = "--skip";
+ break;
+ case ACTION_ABORT:
+ this_operation = "--abort";
+ break;
}
+ verify_opt_compatible(me, this_operation,
+ "--no-commit", opts->no_commit,
+ "--signoff", opts->signoff,
+ "--mainline", opts->mainline,
+ "--strategy", opts->strategy ? 1 : 0,
+ "--strategy-option", opts->xopts.nr ? 1 : 0,
+ "-x", opts->record_origin,
+ "--ff", opts->allow_ff,
+ "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
+ "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
+ NULL);
+
+skip_opt_compatibility_verification:
if (!opts->strategy && opts->default_strategy) {
opts->strategy = opts->default_strategy;
opts->default_strategy = NULL;
@@ -183,9 +196,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
"--edit", opts->edit > 0,
NULL);
- if (cmd) {
- opts->revs = NULL;
- } else {
+ if (cmd == ACTION_START) {
struct setup_revision_opt s_r_opt;
opts->revs = xmalloc(sizeof(*opts->revs));
repo_init_revisions(the_repository, opts->revs, NULL);
@@ -198,6 +209,8 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
memset(&s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.assume_dashdash = 1;
argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
+ } else {
+ opts->revs = NULL;
}
if (argc > 1)
@@ -210,19 +223,22 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
free(options);
- if (cmd == 'q') {
+ switch (cmd) {
+ case ACTION_QUIT: {
int ret = sequencer_remove_state(opts);
if (!ret)
remove_branch_state(the_repository, 0);
return ret;
}
- if (cmd == 'c')
+ case ACTION_CONTINUE:
return sequencer_continue(the_repository, opts);
- if (cmd == 'a')
+ case ACTION_ABORT:
return sequencer_rollback(the_repository, opts);
- if (cmd == 's')
+ case ACTION_SKIP:
return sequencer_skip(the_repository, opts);
- return sequencer_pick_revisions(the_repository, opts);
+ case ACTION_START:
+ return sequencer_pick_revisions(the_repository, opts);
+ }
}
int cmd_revert(int argc, const char **argv, const char *prefix)
--
2.39.3 (Apple Git-145)
^ permalink raw reply related
* Add support for `git rebase -no-edit`
From: Chaitanya Tata @ 2024-01-11 17:25 UTC (permalink / raw)
To: git
Hi,
I have a feature request to add `--no-edit` option to `git rebase`
like we do for `git commit`.
The workflow I typically follow is:
* `git commit -a --fixup=XXX`
* `git rebase -i HEAD~15 --autosquash`
But it requires closing the editor without any changes. I can
workaround this using the `GIT_EDITOR` option, see [1]. But it would
be good to have this built-in.
Thoughts?
[1] - https://stackoverflow.com/a/45783848
Cheers,
Chaitanya.
^ permalink raw reply
* Re: [PATCH 2/3] t7450: test submodule urls
From: Victoria Dye @ 2024-01-11 17:23 UTC (permalink / raw)
To: Junio C Hamano, Victoria Dye via GitGitGadget; +Cc: git
In-Reply-To: <xmqqttnmfarm.fsf@gitster.g>
Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +#define TEST_TOOL_CHECK_URL_USAGE \
>> + "test-tool submodule check-url <url>"
>> +static const char *submodule_check_url_usage[] = {
>> + TEST_TOOL_CHECK_URL_USAGE,
>> + NULL
>> +};
>
> Granted, the entry that follows this new one already uses the same
> pattern, but TEST_TOOL_CHECK_URL_USAGE being used only once here and
> nowhere else, with its name almost as long as the value it expands to,
> I found it unnecessarily verbose and confusing.
This is only used once because I missed the second place it should be used
(in 'submodule_usage[]'). It's still somewhat verbose, but once I fix that
it'll at least have the benefit of avoiding some duplication.
^ permalink raw reply
* Re: [DISCUSS] Introducing Rust into the Git project
From: Elijah Newren @ 2024-01-11 16:57 UTC (permalink / raw)
To: Dragan Simic; +Cc: Taylor Blau, git
In-Reply-To: <f5b9a57b6e2b513f1d79a93c6f0ccf45@manjaro.org>
Hi Dragan,
On Wed, Jan 10, 2024 at 9:39 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> On 2024-01-11 01:33, Elijah Newren wrote:
> > On Wed, Jan 10, 2024 at 1:57 PM Dragan Simic <dsimic@manjaro.org>
> > wrote:
> >>
> >> Thus, Git should probably follow the same approach of not converting
> >> the
> >> already existing code
> >
> > I disagree with this. I saw significant performance improvements
> > through converting some existing Git code to Rust. Granted, it was
> > only a small amount of code, but the performance benefits I saw
> > suggested we'd see more by also doing similar conversions elsewhere.
> > (Note that I kept the old C code and then conditionally compiled
> > either Rust or C versions of what I was converting.)
>
> Well, it's also possible that improving the old C code could also result
> in some performance improvements. Thus, quite frankly, I don't see that
> as a valid argument to rewrite some existing C code in Rust.
Yes, and I've made many performance improvements in the C code in git.
Sometimes I make some of the code 5% or 20% faster. Sometimes 1-3
orders of magnitude faster. Once over 60 orders of magnitude
faster.[1] Look around in git's history; I've done a fair amount of
performance stuff.
And I'm specifically arguing that I feel limited in some of the
performance work that can be done by remaining in C. Part of my
reason for interest in Rust is exactly because I think it can help us
improve performance in ways that are far more difficult to achieve in
C. And this isn't just guesswork, I've done some trials with it.
Further, I even took the time to document some of these reasons
elsewhere in this thread[2]. Arguing that some performance
improvements can be done in C is thus entirely missing the point.
If you want to dismiss the performance angle of argument for Rust, you
should take the time to address the actual reasons raised for why it
could make it easier to improve performance relative to continuing in
C.
Also, as a heads up since you seem to be relatively new to the list:
your position will probably carry more weight with others if you take
the time to understand, acknowledge, and/or address counterpoints of
the other party. It is certainly fine to simply express some concerns
without doing so (Randall and Patrick did a good job of this in this
thread), but when you simply assert that the benefits others point out
simply don't exist (e.g. your "Quite frankly, that would _only_
complicate things and cause fragmentation." (emphasis added) from your
first email in this thread[3], and which this latest email of yours
somewhat looks like as well), others may well start applying a
discount to any positions you state. Granted, it's totally up to you,
but I'm just giving a hint about how I think you might be able to be
more persuasive.
Hope that helps,
Elijah
[1] A couple examples: 6a5fb966720 ("Change default merge backend from
recursive to ort", 2021-08-04) and 8d92fb29270 ("dir: replace
exponential algorithm with a linear one", 2020-04-01)
[2] Footnote 6 of
https://lore.kernel.org/git/CABPp-BFOmwV-xBtjvtenb6RFz9wx2VWVpTeho0k=D8wsCCVwqQ@mail.gmail.com/
[3] https://lore.kernel.org/git/b2651b38a4f7edaf1c5ffee72af00e46@manjaro.org/
^ 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