* [PATCH v2 9/9] t: mark several tests that assume the files backend with REFFILES
From: Patrick Steinhardt @ 2023-10-24 14:05 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Eric Sunshine, Taylor Blau, Junio C Hamano
In-Reply-To: <cover.1698156169.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 8927 bytes --]
Add the REFFILES prerequisite to several tests that assume we're using
the files backend. There are various reasons why we cannot easily
convert those tests to be backend-independent, where the most common
one is that we have no way to write corrupt references into the refdb
via our tooling. We may at a later point in time grow the tooling to
make this possible, but for now we just mark these tests as requiring
the files backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1400-update-ref.sh | 8 ++++----
t/t1450-fsck.sh | 6 +++---
t/t2011-checkout-invalid-head.sh | 16 ++++++++--------
t/t3200-branch.sh | 8 ++++----
t/t3400-rebase.sh | 2 +-
t/t5605-clone-local.sh | 2 +-
6 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index b7d1e5deede..70a760ba378 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -236,7 +236,7 @@ test_expect_success 'update-ref --no-deref -d can delete self-reference' '
test_must_fail git show-ref --verify -q refs/heads/self
'
-test_expect_success 'update-ref --no-deref -d can delete reference to bad ref' '
+test_expect_success REFFILES 'update-ref --no-deref -d can delete reference to bad ref' '
>.git/refs/heads/bad &&
test_when_finished "rm -f .git/refs/heads/bad" &&
git symbolic-ref refs/heads/ref-to-bad refs/heads/bad &&
@@ -288,7 +288,7 @@ test_expect_success "set $m (logged by touch)" '
test $A = $(git show-ref -s --verify $m)
'
-test_expect_success 'empty directory removal' '
+test_expect_success REFFILES 'empty directory removal' '
git branch d1/d2/r1 HEAD &&
git branch d1/r2 HEAD &&
test_path_is_file .git/refs/heads/d1/d2/r1 &&
@@ -300,7 +300,7 @@ test_expect_success 'empty directory removal' '
test_path_is_file .git/logs/refs/heads/d1/r2
'
-test_expect_success 'symref empty directory removal' '
+test_expect_success REFFILES 'symref empty directory removal' '
git branch e1/e2/r1 HEAD &&
git branch e1/r2 HEAD &&
git checkout e1/e2/r1 &&
@@ -1639,7 +1639,7 @@ test_expect_success PIPE 'transaction flushes status updates' '
test_cmp expected actual
'
-test_expect_success 'directory not created deleting packed ref' '
+test_expect_success REFFILES 'directory not created deleting packed ref' '
git branch d1/d2/r1 HEAD &&
git pack-refs --all &&
test_path_is_missing .git/refs/heads/d1/d2 &&
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index a6af550867c..50b15bd7fc0 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -123,7 +123,7 @@ test_expect_success 'branch pointing to non-commit' '
test_i18ngrep "not a commit" out
'
-test_expect_success 'HEAD link pointing at a funny object' '
+test_expect_success REFFILES 'HEAD link pointing at a funny object' '
test_when_finished "git update-ref HEAD $orig_head" &&
echo $ZERO_OID >.git/HEAD &&
# avoid corrupt/broken HEAD from interfering with repo discovery
@@ -139,7 +139,7 @@ test_expect_success 'HEAD link pointing at a funny place' '
test_i18ngrep "HEAD points to something strange" out
'
-test_expect_success 'HEAD link pointing at a funny object (from different wt)' '
+test_expect_success REFFILES 'HEAD link pointing at a funny object (from different wt)' '
test_when_finished "git update-ref HEAD $orig_head" &&
test_when_finished "git worktree remove -f wt" &&
git worktree add wt &&
@@ -149,7 +149,7 @@ test_expect_success 'HEAD link pointing at a funny object (from different wt)' '
test_i18ngrep "main-worktree/HEAD: detached HEAD points" out
'
-test_expect_success 'other worktree HEAD link pointing at a funny object' '
+test_expect_success REFFILES 'other worktree HEAD link pointing at a funny object' '
test_when_finished "git worktree remove -f other" &&
git worktree add other &&
echo $ZERO_OID >.git/worktrees/other/HEAD &&
diff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh
index d9997e7b6b4..3c8135831b8 100755
--- a/t/t2011-checkout-invalid-head.sh
+++ b/t/t2011-checkout-invalid-head.sh
@@ -18,18 +18,18 @@ test_expect_success 'checkout should not start branch from a tree' '
test_must_fail git checkout -b newbranch main^{tree}
'
-test_expect_success 'checkout main from invalid HEAD' '
+test_expect_success REFFILES 'checkout main from invalid HEAD' '
echo $ZERO_OID >.git/HEAD &&
git checkout main --
'
-test_expect_success 'checkout notices failure to lock HEAD' '
+test_expect_success REFFILES 'checkout notices failure to lock HEAD' '
test_when_finished "rm -f .git/HEAD.lock" &&
>.git/HEAD.lock &&
test_must_fail git checkout -b other
'
-test_expect_success 'create ref directory/file conflict scenario' '
+test_expect_success REFFILES 'create ref directory/file conflict scenario' '
git update-ref refs/heads/outer/inner main &&
# do not rely on symbolic-ref to get a known state,
@@ -39,26 +39,26 @@ test_expect_success 'create ref directory/file conflict scenario' '
}
'
-test_expect_success 'checkout away from d/f HEAD (unpacked, to branch)' '
+test_expect_success REFFILES 'checkout away from d/f HEAD (unpacked, to branch)' '
reset_to_df &&
git checkout main
'
-test_expect_success 'checkout away from d/f HEAD (unpacked, to detached)' '
+test_expect_success REFFILES 'checkout away from d/f HEAD (unpacked, to detached)' '
reset_to_df &&
git checkout --detach main
'
-test_expect_success 'pack refs' '
+test_expect_success REFFILES 'pack refs' '
git pack-refs --all --prune
'
-test_expect_success 'checkout away from d/f HEAD (packed, to branch)' '
+test_expect_success REFFILES 'checkout away from d/f HEAD (packed, to branch)' '
reset_to_df &&
git checkout main
'
-test_expect_success 'checkout away from d/f HEAD (packed, to detached)' '
+test_expect_success REFFILES 'checkout away from d/f HEAD (packed, to detached)' '
reset_to_df &&
git checkout --detach main
'
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a38601dbbb7..8d82f9bc52a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -28,7 +28,7 @@ test_expect_success 'git branch --help should not have created a bogus branch' '
test_path_is_missing .git/refs/heads/--help
'
-test_expect_success 'branch -h in broken repository' '
+test_expect_success REFFILES 'branch -h in broken repository' '
mkdir broken &&
(
cd broken &&
@@ -242,7 +242,7 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou
git worktree prune
'
-test_expect_success 'git branch -M fails if updating any linked working tree fails' '
+test_expect_success REFFILES 'git branch -M fails if updating any linked working tree fails' '
git worktree add -b baz bazdir1 &&
git worktree add -f bazdir2 baz &&
touch .git/worktrees/bazdir1/HEAD.lock &&
@@ -833,14 +833,14 @@ test_expect_success 'renaming a symref is not allowed' '
test_path_is_missing .git/refs/heads/new-topic
'
-test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for u is a symlink' '
+test_expect_success SYMLINKS,REFFILES 'git branch -m u v should fail when the reflog for u is a symlink' '
git branch --create-reflog u &&
mv .git/logs/refs/heads/u real-u &&
ln -s real-u .git/logs/refs/heads/u &&
test_must_fail git branch -m u v
'
-test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
+test_expect_success SYMLINKS,REFFILES 'git branch -m with symlinked .git/refs' '
test_when_finished "rm -rf subdir" &&
git init --bare subdir &&
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index d3df19a51f8..435943a0891 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -424,7 +424,7 @@ test_expect_success 'refuse to switch to branch checked out elsewhere' '
test_i18ngrep "already used by worktree at" err
'
-test_expect_success MINGW,SYMLINKS_WINDOWS 'rebase when .git/logs is a symlink' '
+test_expect_success REFFILES,MINGW,SYMLINKS_WINDOWS 'rebase when .git/logs is a symlink' '
git checkout main &&
mv .git/logs actual_logs &&
cmd //c "mklink /D .git\logs ..\actual_logs" &&
diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index a26689de379..8ad6445eca7 100755
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -157,7 +157,7 @@ test_expect_success 'cloning locally respects "-u" for fetching refs' '
test_must_fail git clone --bare -u false a should_not_work.git
'
-test_expect_success 'local clone from repo with corrupt refs fails gracefully' '
+test_expect_success REFFILES 'local clone from repo with corrupt refs fails gracefully' '
git init corrupt &&
test_commit -C corrupt one &&
echo a >corrupt/.git/refs/heads/topic &&
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [Outreachy] Move existing tests to a unit testing framework
From: Achu Luma @ 2023-10-24 14:25 UTC (permalink / raw)
To: Christian Couder, git, Junio C Hamano
In-Reply-To: <CAP8UFD0A_vWCZ5cVAZqdTBebdhZNye_FmNNJF+vA7epUx2JWHQ@mail.gmail.com>
On Mon, Oct 23, 2023 at 2:41 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Fri, Oct 20, 2023 at 3:16 PM Achu Luma <ach.lumap@gmail.com> wrote:
> >
> > Dear Git Community and Mentors,
> >
> > I hope this email finds you well. As a follow-up to my previous application, I'd like to provide additional details on the process of migrating existing unit tests from the t/helper/ directory to the new Git unit test framework.
>
> Thanks for these details!
>
> > -- Identify Target Unit Tests: Start by identifying the specific unit tests in the t/helper/ directory that we want to port to the new Git unit test framework. Ensure that the tests are suitable for migration and that the benefits of doing so outweigh the effort(By avoiding integration tests). The following points have been developed with on going work on the unit-tests framework visible here:
> >
> > 1- https://lore.kernel.org/git/0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com/
> > 2- https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc
>
> Maybe if you have time you could add some descriptions or comments
> related to the above emails and documents. For example you could tell
> what the new unit test framework will be like, how the unit tests will
> look like, etc. Maybe a short overview would be nice.
>
sure,
1- https://lore.kernel.org/git/0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com/
:
The emails highlight the significant milestones achieved in
defining and testing the custom TAP
framework for writing git unit tests. It also contains some
examples of implementation such as
that of STRBUF_INIT with output:
ok 1 - static initialization works
1..1
2- https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc:
From this technical doc, the new unit test framework in the Git
project represents a significant
enhancement, introducing a systematic and efficient approach to
unit testing. The custom git
TAP implementation was selected from several alternatives based
on strict criteria as the most
suitable test framework for porting the unit tests.
The unit tests are written in pure C, eliminating the need for
the previous shell/test-tool helper
setup, simplifying test configuration, data handling, and
reducing testing runtime.
Each unit test is encapsulated as a function and employs a range
of predefined check functions
for validation. These checks can evaluate conditions, compare
integers or characters, validate
strings, and more, providing comprehensive coverage for test scenarios.
When a test is run using the TEST() macro, it undergoes a series
of checks, and if any check fails,
a diagnostic message is printed to aid in debugging. This
diagnostic output includes information
about the specific check that failed, the file and line number
where it occurred, and a clear comparison
of the expected and actual values. Such detailed reporting
simplifies the identification and resolution
of issues, contributing to codebase stability.
Additionally, the framework supports features like skipping tests
with explanations, sending custom
diagnostic messages using test_msg(), and marking known-to-fail
checks using TEST_TODO().
This flexibility allows developers to tailor their tests to
specific scenarios while ensuring a
comprehensive testing suite.
> You could also try to apply the patches in the series that adds the
> test framework, or alternatively use the 'seen' branch where the
> series has been merged, and start playing with it by writing, or
> porting, a small example test.
>
ok, I think I can push a patch for one.
> > -- Create a New C Test File: For each unit test I plan to migrate, create a new C source file (.c) in the Git project's test suite directory(t/unit-tests). Name it appropriately to reflect the purpose of the test.
>
> Could you provide an example of what the new name would be for an
> existing test that is worth porting?
>
Sure... let's consider an existing unit test in t/helper directory
such as t/helper/test-date.c or
its shell named t0006-date.sh, which is part of the current
shell-based test suite. In the context
of the new unit testing framework, this test could be reimagined and renamed as
"t-date.c". The "t-" prefix is typically used for test program files
in Git, and "date" is retained to
reflect the nature of the tests within this suite.
> > -- Include Necessary Headers:In the new C test file, include the necessary Git unit test framework headers. Typically, this includes headers like "test-lib.h" and others relevant to the specific test.
> > #include "test-lib.h"
>
> Maybe you could continue the above example and tell which headers
> would be needed for it?
>
> > -- Convert Test Logic: Refactor the test logic from the original Shell script into the new C-based test format. Use the testing macros provided by the Git unit test framework, such as test_expect_success, test_expect_failure, etc., to define the tests.
> > test_expect_success("simple progress display", "{
> > // Test logic here...
> > }");
>
> Ok, a simple example would be nice too.
>
we can continue with the example used for naming: test-date.c. a
typical t-date.c unit test would look
like the following:
--
#include "test-lib.h"
#include "date.h"
--
date.h here is a necessary header file. Now refactoring the test logic
from the original shell script:
--
#include "test-lib.h"
#include "date.h"
static void test_parse_dates(void)
{
const char *dates[] = { "invalid_date", "2023-10-17 10:00:00 +0200", NULL };
for (const char **argv = dates; *argv; argv++) {
check_int(parse_dates((const char *[]){ *argv, NULL }), 0);
}
}
--
> > -- Add Test Descriptions: Provide clear and informative descriptions for each test using the testing macros. These descriptions will help in identifying the purpose of each test when the test suite is run.
>
> This would seem to be part of the previous step, as you would have to
> provide a description when using the testing macro. But Ok.
>
> > -- Define a Test Entry Point: Create a cmd_main function as the entry point for the C-based tests. Inside this function, include the test functions using the testing macros.
> > int cmd_main(int argc, const char **argv) {
> > // Test functions...
> > return test_done();
> > }
>
> Yeah, continuing an example would be nice.
>
Continuing, we can add a test entrance as follows:
--
#include "test-lib.h"
#include "date.h"
static void test_parse_dates(void)
{
const char *dates[] = { "invalid_date", "2023-10-17 10:00:00 +0200", NULL };
for (const char **argv = dates; *argv; argv++) {
check_int(parse_dates((const char *[]){ *argv, NULL }), 0);
}
}
int main(int argc UNUSED, const char **argv UNUSED)
{
TEST(test_parse_dates, "Test date parsing");
return test_done();
}
--
A typical unit tests with the custom TAP framework would look
something like above. This might run in theory
but I have not yet run it as I used it here just for demonstration.
The unit tests can be built using
"make unit-tests." Additionally, Makefile can be modified to add the
file to the build:
--
UNIT_TEST_PROGRAMS += t-date
--
> > -- Ensure TAP Format Output: Ensure that the C-based tests produce output in the Test Anything Protocol (TAP) format. This format includes the test name, status (ok or not ok), and any diagnostic information.
>
> That means using TEST* macros in the cmd_main() function, as they
> should do the right thing or is there more to be done here?
>
> > -- Test Interaction: Ensure that the migrated tests interact correctly with the new Git unit test framework and any other tests that may be relevant. Consider dependencies and interactions with other parts of the Git project.
>
> I am not sure what work would be needed here. Is there more to do than
> compiling the test files? Having an example would be nice.
>
> > -- Test Execution: Run the migrated tests to verify that they produce the expected results when executed as part of the Git project's test suite. Use the Git testing framework's test runners to execute the tests.
>
> Ok.
>
> > -- Documentation Update: Update the Git project's documentation to reflect the changes made during the migration. Include a reference to the original unit tests in the t/helper/ directory and indicate that these tests have been ported to the new Git unit test framework.
>
> I am not sure that we would want that. I think we might instead want
> to document things in t/helper/ that we don't want to port to the new
> unit test framework and why.
>
Ok noted.
> > By following these points, I think I can successfully port existing unit tests from the t/helper/ directory to use the new Git unit test framework. This migration helps standardize and streamline the testing process within the Git project, improving code quality and maintainability.
>
> Yeah!
>
> > Next Steps:
> >
> > I am eager to discuss these suggestions and collaborate with the Git community to ensure the success of this project. I will continue to engage with the community, seek guidance, and refine my proposal as per your suggestions.
> > I look forward to the opportunity to contribute to the Git project and help make it even more robust and reliable.
>
> Thanks for this application and sorry for the late answer!
>
> Best,
> Christian.
I look forward to feedback and better understanding of the new testing
framework.
BR,
Achu Luma.
^ permalink raw reply
* Re: [PATCH v4 4/7] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
From: Junio C Hamano @ 2023-10-24 17:08 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Jeff King, Taylor Blau, git, Elijah Newren, Eric W. Biederman
In-Reply-To: <ZTdlgEaRLpdSjB1n@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> I'm inclined to agree with your philosophy. Makes me wonder whether we
> should write a Coccinelle rule to catch this. But a quick-and-dirty grep
> in our codebase shows that such a rule would cause quite a lot of churn:
>
> $ git grep BUG\( | wc -l
> 677
> $ git grep assert\( | wc -l
> 549
>
> Probably not worth it.
Yeah, we can stick to our usual "do not add new instances, fix them
while touching near-by code" pattern for this one, I would say.
Thanks.
^ permalink raw reply
* Re: [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Junio C Hamano @ 2023-10-24 17:19 UTC (permalink / raw)
To: Phillip Wood
Cc: Oswald Buddenhagen, phillip.wood, git, Christian Couder,
Charvi Mendiratta, Marc Branchaud, Johannes Sixt
In-Reply-To: <b2b76344-11b7-4f21-8658-f18ffcca2dea@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> I agree there is an argument for improving the implementation of
> --autosquash but until we do I think it is counterproductive to change
> the documentation like this as it will cause users to wonder why
> "rebase --autosquash" generates a todo list that is incorrect
> according to the documentation.
That's a good point.
> I do think it is a good idea to document where the authorship of a
> rebased commit comes from.
Yeah, sounds like a good idea. As to the authorship information, it
might be nicer if the "rebase -i" insn language supported an option
to trigger --reset-author (or even better, --author=...) action for
a single commit, but I presume that it is rather a rare event, and
as long as people understand that they can stop the sequencing
(e.g., an "edit" of the commit would do) and run "commit --amend",
it should be OK, so it probably is OK to leave it as-is.
Thanks.
^ permalink raw reply
* Re: using oldest date when squashing commits
From: Junio C Hamano @ 2023-10-24 17:30 UTC (permalink / raw)
To: Phillip Wood; +Cc: Oswald Buddenhagen, phillip.wood, Johannes Sixt, git
In-Reply-To: <138631cd-ead3-4f22-95ce-61afccfa409f@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
>>> Unfortunately "fixup -C" only copies the commit message not the
>>> authorship
>>
>>> (that's usually a good thing
>>>
>> why? what would that be useful for?
>> it seems rather counter-intuitive.
>
> In the same way that you do not want to change the author date when
> using a fixup to move a small hunk from one commit to another most
> users do not want to update the author information when they make a
> small change to a commit message using "fixup -C"
Exactly.
It would be OK to add "fixup -c --reset-author", but the default
should stay. In addition, I wouldn't be able to use "rebase -i" to
make typofixes to commits made out of received patches if the
operation changes the authorship.
> "fixup -c/-C" were conceived as a way to reword a commit message at
> the same time as optionally fixing up the commit's content.
Yup, it still is a "fix", meaning the identity and the spirit of the
commit being fixed are unchanged. What it aims to achieve, how it
implements the behaviour it wants to give its users, who thought of
that change, all that are the same as the original. It may be a
nice addition to optionally allow users to use --reset-author (or
better yet, --author="Na Me <a@dd.re.ss>") with "fixup", but if the
"-c" variant can be concluded with "commit --amend --reset-author"
to achieve the same effect, that may be sufficient.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option
From: Junio C Hamano @ 2023-10-24 17:45 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps
In-Reply-To: <20231024122631.158415-4-karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> @@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs,...
> break;
> continue;
> }
> - return -1;
> +
> + if (!revs->do_not_die_on_missing_objects)
> + return -1;
> + else
> + oidset_insert(&revs->missing_objects, &p->object.oid);
I would suspect that swapping if/else would make it easier to
follow. Everybody else in the patch guards the use of the oidset
with "were we told not to die on missing objects?", i.e.,
if (revs->do_not_die_on_missing_objects)
oidset_insert(&revs->missing_objects, &p->object.oid);
else
return -1; /* corrupt repository */
> @@ -3800,6 +3809,9 @@ int prepare_revision_walk(struct rev_info *revs)
> FOR_EACH_OBJECT_PROMISOR_ONLY);
> }
>
> + if (revs->do_not_die_on_missing_objects)
> + oidset_init(&revs->missing_objects, 0);
I read the patch to make sure that .missing_objects oidset is used
only when .do_not_die_on_missing_objects is set and the oidset is
untouched unless it is initialized. Well done.
I know I floated "perhaps oidset can replace the object bits,
especially because the number of objects that need marking is
expected to be small", but I am curious what the performance
implication of this would be. Is this something we can create
comparison easily?
I noticed that nobody releases the resource held by this new oidset.
Shouldn't we do so in revision.c:release_revisions()?
Thanks.
^ permalink raw reply
* Re: [PATCH 02/12] builtin/show-ref: split up different subcommands
From: Eric Sunshine @ 2023-10-24 17:55 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <7e6ab5dee230dcb66cb8adfe4a8114a06c805802.1698152926.git.ps@pks.im>
On Tue, Oct 24, 2023 at 9:10 AM Patrick Steinhardt <ps@pks.im> wrote:
> While not immediately obvious, git-show-ref(1) actually implements three
> different subcommands:
>
> - `git show-ref <patterns>` can be used to list references that
> match a specific pattern.
>
> - `git show-ref --verify <refs>` can be used to list references.
> These are _not_ patterns.
>
> - `git show-ref --exclude-existing` can be used as a filter that
> reads references from standard input, performing some conversions
> on each of them.
>
> Let's make this more explicit in the code by splitting up the three
> subcommands into separate functions. This also allows us to address the
> confusingly named `patterns` variable, which may hold either patterns or
> reference names.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> @@ -142,6 +142,53 @@ static int exclude_existing(const char *match)
> +static int cmd_show_ref__verify(const char **refs)
> +{
> + if (!refs || !*refs)
> + die("--verify requires a reference");
> +
> + while (*refs) {
> + struct object_id oid;
> +
> + if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
> + !read_ref(*refs, &oid)) {
> + show_one(*refs, &oid);
> + }
> + else if (!quiet)
> + die("'%s' - not a valid ref", *refs);
> + else
> + return 1;
> + refs++;
> + }
A couple style-nits here caught my attention...
- "}" and "else" should be cuddled: `} else if`
- coding guidelines these days want braces on all branches if any
branch needs them
However, since this code is merely being relocated from elsewhere in
this file and since these style-nits were already present, moving the
code verbatim without correcting the style problems is more
reviewer-friendly. Okay.
> + return 0;
> +}
> +
> +static int cmd_show_ref__patterns(const char **patterns)
> +{
> + struct show_ref_data show_ref_data = {
> + .patterns = (patterns && *patterns) ? patterns : NULL,
> + };
Are we allowing non-constant initializers in the codebase? If not,
this should probably initialize .patterns to NULL and then
conditionally assign `patterns` separately in code below the
initializer.
> + if (show_head)
> + head_ref(show_ref, &show_ref_data);
> + if (heads_only || tags_only) {
> + if (heads_only)
> + for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
> + if (tags_only)
> + for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
> + } else {
> + for_each_ref(show_ref, &show_ref_data);
> + }
> + if (!found_match) {
> + if (verify && !quiet)
> + die("No match");
> + return 1;
> + }
> +
> + return 0;
> +}
^ permalink raw reply
* Re: [PATCH 04/12] builtin/show-ref: fix dead code when passing patterns
From: Eric Sunshine @ 2023-10-24 18:02 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <29c0c0c6c9728f286efd0b78996f9a1394957862.1698152926.git.ps@pks.im>
On Tue, Oct 24, 2023 at 9:10 AM Patrick Steinhardt <ps@pks.im> wrote:
> When passing patterns to `git show-ref` we have some code that will
> cause us to die of `verify && !quiet` is true. But because `verify`
s/of/if/
> indicates a different subcommand of git-show-ref(1) that causes us to
> execute `cmd_show_ref__verify()` and not `cmd_show_ref__patterns()`, the
> condition cannot ever be true.
>
> Let's remove this dead code.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
^ permalink raw reply
* Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch
From: Junio C Hamano @ 2023-10-24 18:16 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jiang Xin, Git List, Jiang Xin
In-Reply-To: <ZTZF3AbNNuGpy38l@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> With that reasoning we could get rid of the error handling of abort
> completely as it's known not to fail. But only because it does not fail
> right now doesn't mean that it won't in the future, as the infra for it
> to fail is all in place. And in case it ever does the current code will
> run into the bug I described.
>
> So in my opinion, we should either refactor the code to clarify that
> this cannot fail indeed. Or do the right thing and handle the error case
> correctly, which right now we don't.
Sounds reasonable. Thanks for a good review.
^ permalink raw reply
* Re: [PATCH 05/12] builtin/show-ref: refactor `--exclude-existing` options
From: Eric Sunshine @ 2023-10-24 18:48 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <8d0b0b5700c7ffed6b3a74760d0d9155b404bb4f.1698152926.git.ps@pks.im>
On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> It's not immediately obvious options which options are applicable to
> what subcommand ni git-show-ref(1) because all options exist as global
s/ni/in/
> state. This can easily cause confusion for the reader.
>
> Refactor options for the `--exclude-existing` subcommand to be contained
> in a separate structure. This structure is stored on the stack and
> passed down as required. Consequentially, it clearly delimits the scope
s/Consequentially/Consequently/
> of those options and requires the reader to worry less about global
> state.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> @@ -95,6 +94,11 @@ static int add_existing(const char *refname,
> +struct exclude_existing_options {
> + int enabled;
> + const char *pattern;
> +};
Do we need this `enabled` flag? Can't the same be achieved by checking
whether `pattern` is NULL or not (see below)?
> @@ -104,11 +108,11 @@ static int add_existing(const char *refname,
> -static int cmd_show_ref__exclude_existing(const char *match)
> +static int cmd_show_ref__exclude_existing(const struct exclude_existing_options *opts)
Since you're renaming `match` to `opts->pattern`...
> {
> - int matchlen = match ? strlen(match) : 0;
> + int matchlen = opts->pattern ? strlen(opts->pattern) : 0;
... and since you're touching this line anyway, maybe it makes sense
to rename `matchlen` to `patternlen`?
> @@ -124,11 +128,11 @@ static int cmd_show_ref__exclude_existing(const char *match)
> - if (strncmp(ref, match, matchlen))
> + if (strncmp(ref, opts->pattern, matchlen))
Especially since, as shown in this context, `matchlen` is really the
length of the _pattern_, not the length of the resulting _match_.
> @@ -200,44 +204,46 @@ static int hash_callback(const struct option *opt, const char *arg, int unset)
> int cmd_show_ref(int argc, const char **argv, const char *prefix)
> {
> [...]
> - if (exclude_arg)
> - return cmd_show_ref__exclude_existing(exclude_existing_arg);
> + if (exclude_existing_opts.enabled)
> + return cmd_show_ref__exclude_existing(&exclude_existing_opts);
(continued from above) Can't this be handled without a separate `enabled` flag?
if (exclude_existing_opts.pattern)
...
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Michael Strawbridge @ 2023-10-24 19:00 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: git, Luben Tuikov, entwicklung
In-Reply-To: <20231024130037.sbevzk2x7oclj7d7@pengutronix.de>
On 10/24/23 09:00, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Oct 20, 2023 at 05:06:36PM -0400, Michael Strawbridge wrote:
>> On 10/20/23 06:04, Uwe Kleine-König wrote:
>>> hello,
>>>
>>> On Fri, Oct 13, 2023 at 04:14:37PM +0200, Uwe Kleine-König wrote:
>>>> Hello,
>>>>
>>>> $ git send-email --to 'A B <a@b.org>, C D <c@d.org>' lala.patch
>>>> Use of uninitialized value $address in sprintf at /usr/lib/git-core/git-send-email line 1172.
>>>> error: unable to extract a valid address from:
>>>>
>>>> This happens for me with git 2.42.0 and also on master (59167d7d09fd, "The seventeenth batch").
Hm. I tried reproing with master (59167d7d09fd, "The seventeenth batch") but I don't seem to see an error:
```
$ git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>' -1 --smtp-server="$(pwd)/fake.sendmail"
/tmp/FEGzLrm_4U/0001-test-test.patch
1
(mbox) Adding cc: Your Name <you@example.com> from line 'From: Your Name <you@example.com>'
(body) Adding cc: Your Name <you@example.com> from line 'Signed-off-by: Your Name <you@example.com>'
From: Your Name <you@example.com>
To: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Cc: Your Name <you@example.com>
Subject: [PATCH] test test
Date: Tue, 24 Oct 2023 18:33:51 +0000
Message-ID: <20231024183350.10-2-you@example.com>
X-Mailer: git-send-email 2.40.0.rc1.1708.gbffa95ed23.dirty
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
The Cc list above has been expanded by additional
addresses found in the patch commit message. By default
send-email prompts before sending whenever this occurs.
This behavior is controlled by the sendemail.confirm
configuration setting.
For additional information, run 'git send-email --help'.
To retain the current behavior, but squelch this message,
run 'git config --global sendemail.confirm auto'.
Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
-i
OK. Log says:
Sendmail: /git/fake.sendmail -i u.kleine-koenig@pengutronix.de you@example.com
From: Your Name <you@example.com>
To: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Cc: Your Name <you@example.com>
Subject: [PATCH] test test
Date: Tue, 24 Oct 2023 18:33:51 +0000
Message-ID: <20231024183350.10-2-you@example.com>
X-Mailer: git-send-email 2.40.0.rc1.1708.gbffa95ed23.dirty
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
```
I think my terminal may be mangling the UTF-8 encoding but git send-email doesn't seem to complain.
Only when I go to an older git (version 2.42.0 for example) do I then get:
```
Use of uninitialized value $address in sprintf at /usr/lib/git-core/git-send-email line 1172.
error: unable to extract a valid address from:
```
This would be expected because https://public-inbox.org/git/20230918212004.GC2163162@coredump.intra.peff.net/T/#mae64003cbb72f015bf5c0c04216524fcb6bb8d09
is meant to fix it.
Just in case it was the "from" address, I tried using 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>'
as the from address. This still worked without issue for (59167d7d09fd, "The seventeenth batch").
>>>>
>>>> Bisection points at
>>>>
>>>> a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
>>>>
>>>> I didn't try to understand that change and fix the problem.
>>>
>>> Another (similar?) problem with non-ascii-chars:
>>>
>>> $ git send-email --to 'Will Deacon <will@kernel.org>' --to 'Krzysztof Wilczyński <kw@linux.com>' --to 'Lorenzo Pieralisi <lpieralisi@kernel.org>' --cc 'Rob Herring <robh@kernel.org>' --to 'Bjorn Helgaas <bhelgaas@google.com>' --cc 'linux-pci@vger.kernel.org' --cc kernel@pengutronix.de -1 --base=@~
>>> Use of uninitialized value $address in sprintf at /home/uwe/gsrc/git/git-send-email line 1162.
>>> error: unable to extract a valid address from:
>>>
>>> Bisection points to the same commit, when dropping ń in Krzysztof's
>>> name, it works fine.
I tried this as well with seventeen and it succeeds:
```
/tmp/GfTxbwKSx_/0001-test-test.patch
1
(mbox) Adding cc: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de> from line 'From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>'
(body) Adding cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> from line 'Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>'
From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
To: Will Deacon <will@kernel.org>,
=?UTF-8?q?Krzysztof=20Wilczy=C5=84ski?= <kw@linux.com>,
Lorenzo Pieralisi <lpieralisi@kernel.org>,
Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>,
linux-pci@vger.kernel.org,
kernel@pengutronix.de,
=?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Subject: [PATCH] test test
Date: Tue, 24 Oct 2023 18:57:16 +0000
Message-ID: <20231024185715.10-2-u.kleine-koenig@pengutronix.de>
X-Mailer: git-send-email 2.40.0.rc1.1708.gbffa95ed23.dirty
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The Cc list above has been expanded by additional
addresses found in the patch commit message. By default
send-email prompts before sending whenever this occurs.
This behavior is controlled by the sendemail.confirm
configuration setting.
For additional information, run 'git send-email --help'.
To retain the current behavior, but squelch this message,
run 'git config --global sendemail.confirm auto'.
Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
-i
OK. Log says:
Sendmail: /git/fake.sendmail -i will@kernel.org kw@linux.com lpieralisi@kernel.org bhelgaas@google.com robh@kernel.org linux-pci@vger.kernel.org kernel@pengutronix.de u.kleine-koenig@pengutronix.de
From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
To: Will Deacon <will@kernel.org>,
=?UTF-8?q?Krzysztof=20Wilczy=C5=84ski?= <kw@linux.com>,
Lorenzo Pieralisi <lpieralisi@kernel.org>,
Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>,
linux-pci@vger.kernel.org,
kernel@pengutronix.de,
=?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Subject: [PATCH] test test
Date: Tue, 24 Oct 2023 18:57:16 +0000
Message-ID: <20231024185715.10-2-u.kleine-koenig@pengutronix.de>
X-Mailer: git-send-email 2.40.0.rc1.1708.gbffa95ed23.dirty
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Result: OK
```
Perhaps there is another variable involved?
>>>
>> This is interesting. Thanks for reporting it. If you are able, could you please try the patches found in the below threads:
>> - https://public-inbox.org/git/20230918212004.GC2163162@coredump.intra.peff.net/T/#mae64003cbb72f015bf5c0c04216524fcb6bb8d09
>
> On main (2e8e77cbac8a) this one is already applied, with that the error
> message reduces to:
>
> $ git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>' -1
> error: unable to extract a valid address from: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
>> - https://public-inbox.org/git/f5c6a72b-f888-4d43-8be8-3ce2c878c669@gmail.com/T/#mca12dc95ccfd3ce2b94e7752ebaae9891201084f
>
> This one doesn't help either. With it applied on top of main I get the
> sams result as on vanilla main.
>
> Best regards
> Uwe
>
^ permalink raw reply
* Re: [PATCH v2 2/2] commit: detect commits that exist in commit-graph but not in the ODB
From: Junio C Hamano @ 2023-10-24 19:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak, Taylor Blau, Jeff King
In-Reply-To: <0476d4855562b677ced106a4cc7788b46434cf21.1698060036.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Commit graphs can become stale and contain references to commits that do
> not exist in the object database anymore. Theoretically, this can lead
> to a scenario where we are able to successfully look up any such commit
> via the commit graph even though such a lookup would fail if done via
> the object database directly.
>
> As the commit graph is mostly intended as a sort of cache to speed up
> parsing of commits we do not want to have diverging behaviour in a
> repository with and a repository without commit graphs, no matter
> whether they are stale or not. As commits are otherwise immutable, the
> only thing that we really need to care about is thus the presence or
> absence of a commit.
>
> To address potentially stale commit data that may exist in the graph,
> our `lookup_commit_in_graph()` function will check for the commit's
> existence in both the commit graph, but also in the object database. So
> even if we were able to look up the commit's data in the graph, we would
> still pretend as if the commit didn't exist if it is missing in the
> object database.
>
> We don't have the same safety net in `parse_commit_in_graph_one()`
> though. This function is mostly used internally in "commit-graph.c"
> itself to validate the commit graph, and this usage is fine. We do
> expose its functionality via `parse_commit_in_graph()` though, which
> gets called by `repo_parse_commit_internal()`, and that function is in
> turn used in many places in our codebase.
>
> For all I can see this function is never used to directly turn an object
> ID into a commit object without additional safety checks before or after
> this lookup. What it is being used for though is to walk history via the
> parent chain of commits. So when commits in the parent chain of a graph
> walk are missing it is possible that we wouldn't notice if that missing
> commit was part of the commit graph. Thus, a query like `git rev-parse
> HEAD~2` can succeed even if the intermittent commit is missing.
>
> It's unclear whether there are additional ways in which such stale
> commit graphs can lead to problems. In any case, it feels like this is a
> bigger bug waiting to happen when we gain additional direct or indirect
> callers of `repo_parse_commit_internal()`. So let's fix the inconsistent
> behaviour by checking for object existence via the object database, as
> well.
>
> This check of course comes with a performance penalty. The following
> benchmarks have been executed in a clone of linux.git with stable tags
> added:
>
> Benchmark 1: git -c core.commitGraph=true rev-list --topo-order --all (git = master)
> Time (mean ± σ): 2.913 s ± 0.018 s [User: 2.363 s, System: 0.548 s]
> Range (min … max): 2.894 s … 2.950 s 10 runs
>
> Benchmark 2: git -c core.commitGraph=true rev-list --topo-order --all (git = pks-commit-graph-inconsistency)
> Time (mean ± σ): 3.834 s ± 0.052 s [User: 3.276 s, System: 0.556 s]
> Range (min … max): 3.780 s … 3.961 s 10 runs
>
> Benchmark 3: git -c core.commitGraph=false rev-list --topo-order --all (git = master)
> Time (mean ± σ): 13.841 s ± 0.084 s [User: 13.152 s, System: 0.687 s]
> Range (min … max): 13.714 s … 13.995 s 10 runs
>
> Benchmark 4: git -c core.commitGraph=false rev-list --topo-order --all (git = pks-commit-graph-inconsistency)
> Time (mean ± σ): 13.762 s ± 0.116 s [User: 13.094 s, System: 0.667 s]
> Range (min … max): 13.645 s … 14.038 s 10 runs
>
> Summary
> git -c core.commitGraph=true rev-list --topo-order --all (git = master) ran
> 1.32 ± 0.02 times faster than git -c core.commitGraph=true rev-list --topo-order --all (git = pks-commit-graph-inconsistency)
> 4.72 ± 0.05 times faster than git -c core.commitGraph=false rev-list --topo-order --all (git = pks-commit-graph-inconsistency)
> 4.75 ± 0.04 times faster than git -c core.commitGraph=false rev-list --topo-order --all (git = master)
>
> We look at a ~30% regression in general, but in general we're still a
> whole lot faster than without the commit graph. To counteract this, the
> new check can be turned off with the `GIT_COMMIT_GRAPH_PARANOIA` envvar.
Very nicely described. Will queue. I'll go offline for the rest of
the week but if there are no significant issues discovered by the
time I come back, let's declare a victory and merge these two
patches down to 'next'.
Thanks.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> commit.c | 16 +++++++++++++++-
> t/t5318-commit-graph.sh | 27 +++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/commit.c b/commit.c
> index b3223478bc..7399e90212 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -28,6 +28,7 @@
> #include "shallow.h"
> #include "tree.h"
> #include "hook.h"
> +#include "parse.h"
>
> static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
>
> @@ -572,8 +573,21 @@ int repo_parse_commit_internal(struct repository *r,
> return -1;
> if (item->object.parsed)
> return 0;
> - if (use_commit_graph && parse_commit_in_graph(r, item))
> + if (use_commit_graph && parse_commit_in_graph(r, item)) {
> + static int object_paranoia = -1;
> +
> + if (object_paranoia == -1)
> + object_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
> +
> + if (object_paranoia && !has_object(r, &item->object.oid, 0)) {
> + unparse_commit(r, &item->object.oid);
> + return quiet_on_missing ? -1 :
> + error(_("commit %s exists in commit-graph but not in the object database"),
> + oid_to_hex(&item->object.oid));
> + }
> +
> return 0;
> + }
>
> if (oid_object_info_extended(r, &item->object.oid, &oi, flags) < 0)
> return quiet_on_missing ? -1 :
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index c0cc454538..79467d7926 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -842,4 +842,31 @@ test_expect_success 'stale commit cannot be parsed when given directly' '
> )
> '
>
> +test_expect_success 'stale commit cannot be parsed when traversing graph' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> +
> + test_commit A &&
> + test_commit B &&
> + test_commit C &&
> + git commit-graph write --reachable &&
> +
> + # Corrupt the repository by deleting the intermittent commit
> + # object. Commands should notice that this object is absent and
> + # thus that the repository is corrupt even if the commit graph
> + # exists.
> + oid=$(git rev-parse B) &&
> + rm .git/objects/"$(test_oid_to_path "$oid")" &&
> +
> + # Again, we should be able to parse the commit when not
> + # being paranoid about commit graph staleness...
> + GIT_COMMIT_GRAPH_PARANOIA=false git rev-parse HEAD~2 &&
> + # ... but fail when we are paranoid.
> + test_must_fail git rev-parse HEAD~2 2>error &&
> + grep "error: commit $oid exists in commit-graph but not in the object database" error
> + )
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH 00/12] show-ref: introduce mode to check for ref existence
From: Junio C Hamano @ 2023-10-24 19:17 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> this patch series introduces a new `--exists` mode to git-show-ref(1) to
> explicitly check for the existence of a reference, only.
I agree that show-ref would be the best place for this feature (not
rev-parse, which is already a kitchen sink). After all, the command
was designed for validating refs in 358ddb62 (Add "git show-ref"
builtin command, 2006-09-15).
Thanks. Hopefully I can take a look before I go offline.
^ permalink raw reply
* Re: [PATCH 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands
From: Eric Sunshine @ 2023-10-24 19:25 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <d0a991cf4f892e73e4fd62ef3fdae3fa73277321.1698152926.git.ps@pks.im>
On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> The git-show-ref(1) command has three different modes, of which one is
> implicit and the other two can be chosen explicitly by passing a flag.
> But while these modes are standalone and cause us to execute completely
> separate code paths, we gladly accept the case where a user asks for
> both `--exclude-existing` and `--verify` at the same time even though it
> is not obvious what will happen. Spoiler: we ignore `--verify` and
> execute the `--exclude-existing` mode.
>
> Let's explicitly detect this invalid usage and die in case both modes
> were requested.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> @@ -269,6 +269,9 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
> + if ((!!exclude_existing_opts.enabled + !!verify) > 1)
> + die(_("only one of --exclude-existing or --verify can be given"));
Somewhat recently, work was done to normalize this sort of message.
The result was to instead use the phrasing "options '%s' and '%s'
cannot be used together". See, for instance, 43ea635c35 (i18n:
refactor "foo and bar are mutually exclusive", 2022-01-05).
^ permalink raw reply
* Re: [PATCH 10/12] builtin/show-ref: explicitly spell out different modes in synopsis
From: Eric Sunshine @ 2023-10-24 19:39 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <adcfa7a6a9d8fb6f915faf77df52362544cd590e.1698152926.git.ps@pks.im>
On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> The synopsis treats the `--verify` and the implicit mode the same. They
> are slightly different though:
>
> - They accept different sets of flags.
>
> - The implicit mode accepts patterns while the `--verify` mode
> accepts references.
>
> Split up the synopsis for these two modes such that we can disambiguate
> those differences.
Good. When reading [2/12], my immediate thought was that such a
documentation change was needed.
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
> @@ -8,9 +8,12 @@ git-show-ref - List references in a local repository
> SYNOPSIS
> -'git show-ref' [-q | --quiet] [--verify] [--head] [-d | --dereference]
> +'git show-ref' [-q | --quiet] [--head] [-d | --dereference]
> [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]
> [--heads] [--] [<pattern>...]
> +'git show-ref' --verify [-q | --quiet] [-d | --dereference]
> + [-s | --hash[=<n>]] [--abbrev[=<n>]]
> + [--] [<ref>...]
> 'git show-ref' --exclude-existing[=<pattern>]
What does it mean to request "quiet" for the plain `git show-ref`
mode? That seems pointless and counterintuitive. Even though this mode
may accept --quiet as a quirk of implementation, we probably shouldn't
be promoting its use in the documentation. Moreover, the blurb for
--quiet later in the document:
Do not print any results to stdout. When combined with --verify,
this can be used to silently check if a reference exists.
should probably be rephrased since it currently implies that it may be
used with modes other than --verify, but that's not really the case
(implementation quirks aside).
This also raises the question as to whether an interlock should be
added to disallow --quiet with plain `git show-ref`, much like the
interlock preventing --exclude-existing and --verify from being used
together. Ideally, such an interlock ought to be added, but I wouldn't
be surprised to learn that doing so would break someone's existing
tooling which insensibly uses --quiet with plain `git show-ref`.
^ permalink raw reply
* [PATCH] SubmittingPatches: call gitk's command "Copy commit reference"
From: Andrei Rybak @ 2023-10-24 19:51 UTC (permalink / raw)
To: git
Documentation/SubmittingPatches informs the contributor that gitk's
context menu command "Copy commit summary" can be used to obtain the
conventional format of referencing existing commits. This command in
gitk was renamed to "Copy commit reference" in commit [1], following
implementation of Git's "reference" pretty format in [2].
Update mention of this gitk command in Documentation/SubmittingPatches
to its new name.
[1] b8b60957ce (gitk: rename "commit summary" to "commit reference",
2019-12-12)
[2] commit 1f0fc1d (pretty: implement 'reference' format, 2019-11-20)
Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
---
Documentation/SubmittingPatches | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 0e2d3fbb9c..653bb2ad44 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -266,7 +266,7 @@ date)", like this:
noticed that ...
....
-The "Copy commit summary" command of gitk can be used to obtain this
+The "Copy commit reference" command of gitk can be used to obtain this
format (with the subject enclosed in a pair of double-quotes), or this
invocation of `git show`:
--
2.42.0
^ permalink raw reply related
* [PATCH 1/1] merge-file: add an option to process object IDs
From: brian m. carlson @ 2023-10-24 19:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren, Phillip Wood
In-Reply-To: <20231024195655.2413191-1-sandals@crustytoothpaste.net>
From: "brian m. carlson" <bk2204@github.com>
git merge-file knows how to merge files on the file system already. It
would be helpful, however, to allow it to also merge single blobs.
Teach it an `--object-id` option which means that its arguments are
object IDs and not files to allow it to do so.
Since we obviously won't be writing the data to the first argument,
either write to the object store and print the object ID, or honor the
-p argument and print it to standard out.
We handle the empty blob specially since read_mmblob doesn't read it
directly, instead throwing an error, and otherwise users cannot specify
an empty ancestor.
Signed-off-by: brian m. carlson <bk2204@github.com>
---
Documentation/git-merge-file.txt | 20 +++++++++++
builtin/merge-file.c | 58 +++++++++++++++++++++++---------
t/t6403-merge-file.sh | 58 ++++++++++++++++++++++++++++++++
3 files changed, 120 insertions(+), 16 deletions(-)
diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
index 7e9093fab6..45460f3916 100644
--- a/Documentation/git-merge-file.txt
+++ b/Documentation/git-merge-file.txt
@@ -12,6 +12,9 @@ SYNOPSIS
'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]]
[--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
[--[no-]diff3] <current-file> <base-file> <other-file>
+'git merge-file' --object-id [-L <current-name> [-L <base-name> [-L <other-name>]]]
+ [--ours|--theirs|--union] [-q|--quiet] [--marker-size=<n>]
+ [--[no-]diff3] <current-oid> <base-oid> <other-oid>
DESCRIPTION
@@ -40,6 +43,10 @@ however, these conflicts are resolved favouring lines from `<current-file>`,
lines from `<other-file>`, or lines from both respectively. The length of the
conflict markers can be given with the `--marker-size` option.
+If `--object-id` is specified, exactly the same behavior occurs, except that
+instead of specifying what to merge as files, it is specified as a list of
+object IDs referring to blobs.
+
The exit value of this program is negative on error, and the number of
conflicts otherwise (truncated to 127 if there are more than that many
conflicts). If the merge was clean, the exit value is 0.
@@ -52,6 +59,14 @@ linkgit:git[1].
OPTIONS
-------
+--object-id::
+ Specify the contents to merge as blobs in the current repository instead of
+ files. In this case, the operation must take place within a valid repository.
++
+If the `-p` option is specified, the merged file (including conflicts, if any)
+goes to standard output as normal; otherwise, the merged file is written to the
+object store and the object ID of its blob is written to standard output.
+
-L <label>::
This option may be given up to three times, and
specifies labels to be used in place of the
@@ -93,6 +108,11 @@ EXAMPLES
merges tmp/a123 and tmp/c345 with the base tmp/b234, but uses labels
`a` and `c` instead of `tmp/a123` and `tmp/c345`.
+`git merge-file -p --object-id abc1234 def567 890abcd`::
+
+ combines the changes of the blob abc1234 and 890abcd since def567,
+ tries to merge them and writes the result to standard output
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index d7eb4c6540..d308434b8e 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -1,5 +1,8 @@
#include "builtin.h"
#include "abspath.h"
+#include "hex.h"
+#include "object-name.h"
+#include "object-store.h"
#include "config.h"
#include "gettext.h"
#include "setup.h"
@@ -31,10 +34,11 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
mmfile_t mmfs[3] = { 0 };
mmbuffer_t result = { 0 };
xmparam_t xmp = { 0 };
- int ret = 0, i = 0, to_stdout = 0;
+ int ret = 0, i = 0, to_stdout = 0, object_id = 0;
int quiet = 0;
struct option options[] = {
OPT_BOOL('p', "stdout", &to_stdout, N_("send results to standard output")),
+ OPT_BOOL(0, "object-id", &object_id, N_("use object IDs instead of filenames")),
OPT_SET_INT(0, "diff3", &xmp.style, N_("use a diff3 based merge"), XDL_MERGE_DIFF3),
OPT_SET_INT(0, "zdiff3", &xmp.style, N_("use a zealous diff3 based merge"),
XDL_MERGE_ZEALOUS_DIFF3),
@@ -71,8 +75,12 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
return error_errno("failed to redirect stderr to /dev/null");
}
+ if (object_id)
+ setup_git_directory();
+
for (i = 0; i < 3; i++) {
char *fname;
+ struct object_id oid;
mmfile_t *mmf = mmfs + i;
if (!names[i])
@@ -80,12 +88,21 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
fname = prefix_filename(prefix, argv[i]);
- if (read_mmfile(mmf, fname))
+ if (object_id) {
+ if (repo_get_oid(the_repository, argv[i], &oid))
+ ret = -1;
+ else if (!oideq(&oid, the_hash_algo->empty_blob))
+ read_mmblob(mmf, &oid);
+ else
+ read_mmfile(mmf, "/dev/null");
+ } else if (read_mmfile(mmf, fname)) {
ret = -1;
- else if (mmf->size > MAX_XDIFF_SIZE ||
- buffer_is_binary(mmf->ptr, mmf->size))
+ }
+ if (ret != -1 && (mmf->size > MAX_XDIFF_SIZE ||
+ buffer_is_binary(mmf->ptr, mmf->size))) {
ret = error("Cannot merge binary files: %s",
argv[i]);
+ }
free(fname);
if (ret)
@@ -99,20 +116,29 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
ret = xdl_merge(mmfs + 1, mmfs + 0, mmfs + 2, &xmp, &result);
if (ret >= 0) {
- const char *filename = argv[0];
- char *fpath = prefix_filename(prefix, argv[0]);
- FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
+ if (object_id && !to_stdout) {
+ struct object_id oid;
+ if (result.size)
+ write_object_file(result.ptr, result.size, OBJ_BLOB, &oid);
+ else
+ oidcpy(&oid, the_hash_algo->empty_blob);
+ printf("%s\n", oid_to_hex(&oid));
+ } else {
+ const char *filename = argv[0];
+ char *fpath = prefix_filename(prefix, argv[0]);
+ FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
- if (!f)
- ret = error_errno("Could not open %s for writing",
- filename);
- else if (result.size &&
- fwrite(result.ptr, result.size, 1, f) != 1)
- ret = error_errno("Could not write to %s", filename);
- else if (fclose(f))
- ret = error_errno("Could not close %s", filename);
+ if (!f)
+ ret = error_errno("Could not open %s for writing",
+ filename);
+ else if (result.size &&
+ fwrite(result.ptr, result.size, 1, f) != 1)
+ ret = error_errno("Could not write to %s", filename);
+ else if (fclose(f))
+ ret = error_errno("Could not close %s", filename);
+ free(fpath);
+ }
free(result.ptr);
- free(fpath);
}
if (ret > 127)
diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh
index 1a7082323d..2c92209eca 100755
--- a/t/t6403-merge-file.sh
+++ b/t/t6403-merge-file.sh
@@ -65,11 +65,30 @@ test_expect_success 'merge with no changes' '
test_cmp test.txt orig.txt
'
+test_expect_success 'merge with no changes with --object-id' '
+ git add orig.txt &&
+ git merge-file -p --object-id :orig.txt :orig.txt :orig.txt >actual &&
+ test_cmp actual orig.txt
+'
+
test_expect_success "merge without conflict" '
cp new1.txt test.txt &&
git merge-file test.txt orig.txt new2.txt
'
+test_expect_success 'merge without conflict with --object-id' '
+ git add orig.txt new2.txt &&
+ git merge-file --object-id :orig.txt :orig.txt :new2.txt >actual &&
+ git rev-parse :new2.txt >expected &&
+ test_cmp actual expected
+'
+
+test_expect_success 'can accept object ID with --object-id' '
+ git merge-file --object-id $(test_oid empty_blob) $(test_oid empty_blob) :new2.txt >actual &&
+ git rev-parse :new2.txt >expected &&
+ test_cmp actual expected
+'
+
test_expect_success 'works in subdirectory' '
mkdir dir &&
cp new1.txt dir/a.txt &&
@@ -138,6 +157,31 @@ test_expect_success "expected conflict markers" '
test_cmp expect.txt test.txt
'
+test_expect_success "merge with conflicts with --object-id" '
+ git add backup.txt orig.txt new3.txt &&
+ test_must_fail git merge-file -p --object-id :backup.txt :orig.txt :new3.txt >actual &&
+ sed -e "s/<< test.txt/<< :backup.txt/" \
+ -e "s/>> new3.txt/>> :new3.txt/" \
+ expect.txt >expect &&
+ test_cmp expect actual &&
+ test_must_fail git merge-file --object-id :backup.txt :orig.txt :new3.txt >oid &&
+ git cat-file blob "$(cat oid)" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "merge with conflicts with --object-id with labels" '
+ git add backup.txt orig.txt new3.txt &&
+ test_must_fail git merge-file -p --object-id \
+ -L test.txt -L orig.txt -L new3.txt \
+ :backup.txt :orig.txt :new3.txt >actual &&
+ test_cmp expect.txt actual &&
+ test_must_fail git merge-file --object-id \
+ -L test.txt -L orig.txt -L new3.txt \
+ :backup.txt :orig.txt :new3.txt >oid &&
+ git cat-file blob "$(cat oid)" >actual &&
+ test_cmp expect.txt actual
+'
+
test_expect_success "merge conflicting with --ours" '
cp backup.txt test.txt &&
@@ -256,6 +300,14 @@ test_expect_success 'binary files cannot be merged' '
grep "Cannot merge binary files" merge.err
'
+test_expect_success 'binary files cannot be merged with --object-id' '
+ cp "$TEST_DIRECTORY"/test-binary-1.png . &&
+ git add orig.txt new1.txt test-binary-1.png &&
+ test_must_fail git merge-file --object-id \
+ :orig.txt :test-binary-1.png :new1.txt 2> merge.err &&
+ grep "Cannot merge binary files" merge.err
+'
+
test_expect_success 'MERGE_ZEALOUS simplifies non-conflicts' '
sed -e "s/deerit.\$/deerit;/" -e "s/me;\$/me./" <new5.txt >new6.txt &&
sed -e "s/deerit.\$/deerit,/" -e "s/me;\$/me,/" <new5.txt >new7.txt &&
@@ -389,4 +441,10 @@ test_expect_success 'conflict sections match existing line endings' '
test $(tr "\015" Q <nolf.txt | grep "^[<=>].*Q$" | wc -l) = 0
'
+test_expect_success '--object-id fails without repository' '
+ empty="$(test_oid empty_blob)" &&
+ nongit test_must_fail git merge-file --object-id $empty $empty $empty 2>err &&
+ grep "not a git repository" err
+'
+
test_done
^ permalink raw reply related
* [PATCH 0/1] Object ID support for git merge-file
From: brian m. carlson @ 2023-10-24 19:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren, Phillip Wood
This series introduces an --object-id option to git merge-file such
that, instead of reading and writing from files on the system, it reads
from and writes to the object store using blobs. This is in use at
GitHub to produce conflict diffs when a merge fails, and it seems
generally useful, so I'm sending it here.
The only tricky piece is the fact that we have to special-case the empty
blob since otherwise it isn't handled correctly.
brian m. carlson (1):
merge-file: add an option to process object IDs
Documentation/git-merge-file.txt | 20 +++++++++++
builtin/merge-file.c | 58 +++++++++++++++++++++++---------
t/t6403-merge-file.sh | 58 ++++++++++++++++++++++++++++++++
3 files changed, 120 insertions(+), 16 deletions(-)
^ permalink raw reply
* Re: [PATCH 0/3] some send-email --compose fixes
From: Michael Strawbridge @ 2023-10-24 20:12 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Bagas Sanjaya, Git Mailing List
In-Reply-To: <20231023185152.GC1537181@coredump.intra.peff.net>
On 10/23/23 14:51, Jeff King wrote:
> On Fri, Oct 20, 2023 at 02:42:13PM -0700, Junio C Hamano wrote:
>
>>> So here's the fix in a cleaned up form, guided by my own comments from
>>> earlier. ;) I think this is actually all orthogonal to the patch you are
>>> working on, so yours could either go on top or just be applied
>>> separately.
>>>
>>> [1/3]: doc/send-email: mention handling of "reply-to" with --compose
>>> [2/3]: Revert "send-email: extract email-parsing code into a subroutine"
>>> [3/3]: send-email: handle to/cc/bcc from --compose message
>>
>> Nice.
>>
>> With the approach suggested to move the validation down to where the
>> necessary addresses are already all defined, Michael observed "whoa,
>> why am I getting stringified array ref?". If that is the only issue
>> in the approach, queuing these three patches first and then have
>> Michael's fix on top of them sounds like the cleanest thing to do.
Patch coming soon.
>
> I don't think it is even an issue in Michael's approach. I'd have to see
> his patch and how he tested it to be sure, but I suspect he was simply
> being extra careful to test nearby behavior and stumbled upon the
> ARRAY() bug. But the bug was there long before either of his patches.
>
Thank you for your patches Peff! I think it fixes the issue I was seeing.
I was trying to be extra careful with my testing. I had missed testing
--compose and also the multiple --to/cc/bcc examples before.
>> Will queue on top of v2.42.0 to help those who may want to backport
>> these to the maintenance track.
>
> So I think you could take my series on top of master (or 2.42.0), and
> eventually target 'master'. The bug it fixes is from 2017, so not
> urgent. The reading of "to" headers is a new feature.
>
> But the fix to move the validation around should probably go directly
> onto a8022c5f7b (send-email: expose header information to
> git-send-email's sendemail-validate hook, 2023-04-19) for use on maint.
> I guess maybe it is not that urgent anymore, as that regression is in
> v2.41, and we would not release anything along that maint track anymore,
> though.
>
> -Peff
^ permalink raw reply
* Re: [PATCH 1/1] merge-file: add an option to process object IDs
From: Eric Sunshine @ 2023-10-24 20:12 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Junio C Hamano, Elijah Newren, Phillip Wood
In-Reply-To: <20231024195655.2413191-2-sandals@crustytoothpaste.net>
On Tue, Oct 24, 2023 at 3:58 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> git merge-file knows how to merge files on the file system already. It
> would be helpful, however, to allow it to also merge single blobs.
> Teach it an `--object-id` option which means that its arguments are
> object IDs and not files to allow it to do so.
>
> Since we obviously won't be writing the data to the first argument,
> either write to the object store and print the object ID, or honor the
> -p argument and print it to standard out.
>
> We handle the empty blob specially since read_mmblob doesn't read it
> directly, instead throwing an error, and otherwise users cannot specify
> an empty ancestor.
>
> Signed-off-by: brian m. carlson <bk2204@github.com>
> ---
> diff --git a/builtin/merge-file.c b/builtin/merge-file.c
> @@ -99,20 +116,29 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
> if (ret >= 0) {
> - const char *filename = argv[0];
> - char *fpath = prefix_filename(prefix, argv[0]);
> - FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
> + if (object_id && !to_stdout) {
> + struct object_id oid;
> + if (result.size)
> + write_object_file(result.ptr, result.size, OBJ_BLOB, &oid);
Should this be caring about errors by checking the return value of
write_object_file()?
> + } else {
> + const char *filename = argv[0];
> + char *fpath = prefix_filename(prefix, argv[0]);
> + FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
> + if (!f)
> + ret = error_errno("Could not open %s for writing",
> + filename);
> + else if (result.size &&
> + fwrite(result.ptr, result.size, 1, f) != 1)
> + ret = error_errno("Could not write to %s", filename);
> + else if (fclose(f))
> + ret = error_errno("Could not close %s", filename);
> + free(fpath);
The non-"object-id" case cares about errors.
^ permalink raw reply
* Re: using oldest date when squashing commits
From: Oswald Buddenhagen @ 2023-10-24 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, phillip.wood, Johannes Sixt, git
In-Reply-To: <xmqqpm143p46.fsf@gitster.g>
On Tue, Oct 24, 2023 at 10:30:01AM -0700, Junio C Hamano wrote:
>Phillip Wood <phillip.wood123@gmail.com> writes:
>>>> Unfortunately "fixup -C" only copies the commit message not the
>>>> authorship
>>>
>>>> (that's usually a good thing
>>>>
>>> why? what would that be useful for?
>>> it seems rather counter-intuitive.
>>
>> In the same way that you do not want to change the author date when
>> using a fixup to move a small hunk from one commit to another most
>> users do not want to update the author information when they make a
>> small change to a commit message using "fixup -C"
>
>Exactly. [...]
>I wouldn't be able to use "rebase -i" to
>make typofixes to commits made out of received patches if the
>operation changes the authorship.
>
>> "fixup -c/-C" were conceived as a way to reword a commit message at
>> the same time as optionally fixing up the commit's content.
>
>Yup, it still is a "fix", meaning the identity and the spirit of the
>commit being fixed are unchanged. What it aims to achieve, how it
>implements the behaviour it wants to give its users, who thought of
>that change, all that are the same as the original.
>
ok, i think i finally got it. it would have never ocurred to me to make
a command for that - i just use "squash" and throw away the extra lines.
but i guess it sort of makes sense if you use rebase as a
non-interactive execution backend for instructions that are fully
determined long in advance by heaping commits at the end.
> It may be a nice addition to optionally allow users to use
> --reset-author (or better yet, --author="Na Me <a@dd.re.ss>") with
> "fixup"
>
that's kind of the opposite of what i'd want - the "pre-fixup" commit
already has the equivalent of that by virtue of being fresh. so it would
be more like --copy-author. but i'd go with adding -ca/-CA variants
instead, for brevity.
>but if the "-c" variant can be concluded with "commit --amend
>--reset-author" to achieve the same effect, that may be sufficient.
>
from the above follows that the equivalent of my original request would
be appending "exec git commit --amend -C <orig>" to the "pick
<pre-fixup>" + "fixup <orig>" commands. which is of course horrible, and
i'd never remember to actually do that. it will be hard enough to
retrain myself to use -CA instead of -C.
regards
^ permalink raw reply
* [PATCH] send-email: move validation code below process_address_list
From: Michael Strawbridge @ 2023-10-24 20:19 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Bagas Sanjaya, Git Mailing List
In-Reply-To: <393f598e-c7cd-4dc6-a221-9aed7ffcc2b1@amd.com>
From 09ea51d63cebdf9ff0c073ef86e21b4b09c268e5 Mon Sep 17 00:00:00 2001
From: Michael Strawbridge <michael.strawbridge@amd.com>
Date: Wed, 11 Oct 2023 16:13:13 -0400
Subject: [PATCH] send-email: move validation code below process_address_list
Move validation logic below processing of email address lists so that
email validation gets the proper email addresses.
This fixes email address validation errors when the optional
perl module Email::Valid is installed and multiple addresses are passed
in on a single to/cc argument like --to=foo@example.com,bar@example.com.
Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
git-send-email.perl | 48 ++++++++++++++++++++++-----------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..a898dbc76e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -799,30 +799,6 @@ sub is_format_patch_arg {
$time = time - scalar $#files;
-if ($validate) {
- # FIFOs can only be read once, exclude them from validation.
- my @real_files = ();
- foreach my $f (@files) {
- unless (-p $f) {
- push(@real_files, $f);
- }
- }
-
- # Run the loop once again to avoid gaps in the counter due to FIFO
- # arguments provided by the user.
- my $num = 1;
- my $num_files = scalar @real_files;
- $ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
- foreach my $r (@real_files) {
- $ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
- pre_process_file($r, 1);
- validate_patch($r, $target_xfer_encoding);
- $num += 1;
- }
- delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
- delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
-}
-
@files = handle_backup_files(@files);
if (@files) {
@@ -2023,6 +1999,30 @@ sub process_file {
return 1;
}
+if ($validate) {
+ # FIFOs can only be read once, exclude them from validation.
+ my @real_files = ();
+ foreach my $f (@files) {
+ unless (-p $f) {
+ push(@real_files, $f);
+ }
+ }
+
+ # Run the loop once again to avoid gaps in the counter due to FIFO
+ # arguments provided by the user.
+ my $num = 1;
+ my $num_files = scalar @real_files;
+ $ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
+ foreach my $r (@real_files) {
+ $ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
+ pre_process_file($r, 1);
+ validate_patch($r, $target_xfer_encoding);
+ $num += 1;
+ }
+ delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
+ delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
+}
+
foreach my $t (@files) {
while (!process_file($t)) {
# user edited the file
--
2.42.0
^ permalink raw reply related
* bugreport
From: galo joel @ 2023-10-24 20:40 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 1 bytes --]
[-- Attachment #1.2: Type: text/html, Size: 26 bytes --]
[-- Attachment #2: git-bugreport-2023-10-24-2215.txt --]
[-- Type: text/plain, Size: 1442 bytes --]
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
execute as admin git bash and,(in cmd W10, same. open git-bash.exe as system-32).
try chmod 755, 777... does not work 'cause im user ($) and not admin (#)
Wha did you expect to happen? (Expected behavior)
change .sh to chmod 755 for execute bash
What happened instead? (Actual behavior)
nothing ._.
What's different between what you expected and what actually happened?
i expected a good sript in bash. now i'm sad
Anything else you want to add:
Please tell me what happen, maybe i'm wrong but chatGPT also no have idea why
i cannot be admin in my own laptop xD, or maybe i need some libraries that i didnot
install. I sell all my information so please help me to understand why does not work :)
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.42.0.windows.2
cpu: x86_64
built from commit: 2f819d1670fff9a1818f63b6722e9959405378e3
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 19045
compiler info: gnuc: 13.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe
[Enabled Hooks]
not run from a git repository - no hooks to show
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Uwe Kleine-König @ 2023-10-24 20:43 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Luben Tuikov, git, entwicklung
In-Reply-To: <89712aea-04fc-4775-afd4-afd3ca24ad01@amd.com>
[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]
Hello Michael,
On Tue, Oct 24, 2023 at 03:00:38PM -0400, Michael Strawbridge wrote:
> On 10/24/23 09:00, Uwe Kleine-König wrote:
> > On Fri, Oct 20, 2023 at 05:06:36PM -0400, Michael Strawbridge wrote:
> >> On 10/20/23 06:04, Uwe Kleine-König wrote:
> >>> On Fri, Oct 13, 2023 at 04:14:37PM +0200, Uwe Kleine-König wrote:
> >>>> $ git send-email --to 'A B <a@b.org>, C D <c@d.org>' lala.patch
> >>>> Use of uninitialized value $address in sprintf at /usr/lib/git-core/git-send-email line 1172.
> >>>> error: unable to extract a valid address from:
> >>>>
> >>>> This happens for me with git 2.42.0 and also on master (59167d7d09fd, "The seventeenth batch").
> Hm. I tried reproing with master (59167d7d09fd, "The seventeenth batch") but I don't seem to see an error:
> ```
> $ git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>' -1 --smtp-server="$(pwd)/fake.sendmail"
> [...]
I debugged a bit and if I do
mv .git/hooks/sendemail-validate .git/hooks/sendemail-validate.bak
git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>'
starts to work for me, too.
I'd guess the content of my sendemail-validate script doesn't matter
much, but for the record, it's:
#!/bin/sh
# installed by patatt install-hook
patatt sign --hook "${1}"
Does the problem reproduce on your end with a sendemail-validate script?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 11/12] builtin/show-ref: add new mode to check for reference existence
From: Eric Sunshine @ 2023-10-24 21:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <2f876e61dd36a8887a1286bb8db9fb6577c55c9b.1698152926.git.ps@pks.im>
On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> While we have multiple ways to show the value of a given reference, we
> do not have any way to check whether a reference exists at all. While
> commands like git-rev-parse(1) or git-show-ref(1) can be used to check
> for reference existence in case the reference resolves to something
> sane, neither of them can be used to check for existence in some other
> scenarios where the reference does not resolve cleanly:
>
> - References which have an invalid name cannot be resolved.
>
> - References to nonexistent objects cannot be resolved.
>
> - Dangling symrefs can be resolved via git-symbolic-ref(1), but this
> requires the caller to special case existence checks depending on
> whteher or not a reference is symbolic or direct.
s/whteher/whether/
> Furthermore, git-rev-list(1) and other commands do not let the caller
> distinguish easily between an actually missing reference and a generic
> error.
>
> Taken together, this gseems like sufficient motivation to introduce a
s/gseems/seems/
> separate plumbing command to explicitly check for the existence of a
> reference without trying to resolve its contents.
>
> This new command comes in the form of `git show-ref --exists`. This
> new mode will exit successfully when the reference exists, with a
> specific error code of 2 when it does not exist, or with 1 when there
> has been a generic error.
>
> Note that the only way to properly implement this command is by using
> the internal `refs_read_raw_ref()` function. While the public function
> `refs_resolve_ref_unsafe()` can be made to behave in the same way by
> passing various flags, it does not provide any way to obtain the errno
> with which the reference backend failed when reading the reference. As
> such, it becomes impossible for us to distinguish generic errors from
> the explicit case where the reference wasn't found.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
> @@ -65,6 +70,12 @@ OPTIONS
> +--exists::
> +
> + Check whether the given reference exists. Returns an error code of 0 if
We probably want to call this "exit code" rather than "error code"
since the latter is unnecessarily scary sounding for the success case
(when the ref does exit).
> + it does, 2 if it is missing, and 128 in case looking up the reference
> + failed with an error other than the reference being missing.
The commit message says it returns 1 for a generic error, but this
inconsistently says it returns 128 for that case. The actual
implementation returns 1.
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> @@ -214,6 +215,41 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,
> +static int cmd_show_ref__exists(const char **refs)
> +{
> + struct strbuf unused_referent = STRBUF_INIT;
> + struct object_id unused_oid;
> + unsigned int unused_type;
> + int failure_errno = 0;
> + const char *ref;
> + int ret = 1;
> +
> + if (!refs || !*refs)
> + die("--exists requires a reference");
> + ref = *refs++;
> + if (*refs)
> + die("--exists requires exactly one reference");
> +
> + if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
> + &unused_oid, &unused_referent, &unused_type,
> + &failure_errno)) {
> + if (failure_errno == ENOENT) {
> + error(_("reference does not exist"));
The documentation doesn't mention this printing any output, and indeed
one would intuitively expect a boolean-like operation to not produce
any printed output since its exit code indicates the result (except,
of course, in the case of a real error).
> + ret = 2;
> + } else {
> + error(_("failed to look up reference: %s"), strerror(failure_errno));
Or use error_errno():
errno = failure_errno;
error_errno(_("failed to look up reference: %s"));
> + }
> +
> + goto out;
> + }
> +
> + ret = 0;
> +
> +out:
> + strbuf_release(&unused_referent);
> + return ret;
> +}
It's a bit odd having `ret` be 1 at the outset rather than 0, thus
making the logic a bit more difficult to reason about. I would have
expected it to be organized like this:
int ret = 0;
if (refs_read_raw_ref(...)) {
if (failure_errno == ENOENT) {
ret = 2;
} else {
ret = 1;
errno = failure_errno;
error_errno(_("failed to look up reference: %s"));
}
}
strbuf_release(...);
return ret;
> @@ -272,13 +309,15 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
> + if ((!!exclude_existing_opts.enabled + !!verify + !!exists) > 1)
> + die(_("only one of --exclude-existing, --exists or --verify can be given"));
When reviewing an earlier patch in this series, I forgot to mention
that we can simplify the life of translators by using placeholders:
die(_("options '%s', '%s' or '%s' cannot be used together"),
"--exclude-existing", "--exists", "--verify");
which ensures that they don't translate the literal option names, and
makes it possible to reuse the translated message in multiple
locations (since it doesn't mention hard-coded option names).
^ 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