* Re: [PATCH v2] setup: remove unnecessary variable
From: Junio C Hamano @ 2024-03-04 21:53 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: christian.couder, git, johannes.schindelin, newren
In-Reply-To: <CZLA8TF4XG5S.KU06P62V03TV@gmail.com>
"Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:
> Yes, in the previous testcase we determined that template cannot set
> core.bare. Therefore, this testcase would be like testing
> --bare/--no-bare option, which is already done in 0001-init.sh and
> t5601-clone.sh. However, I don't have strong opinion on this. I can add
> it back if you think it is worth it.
I was merely trying to make sure that I understood your motivation
behind the change, which was described at the end of the commit log
message, i.e. "... and remove one redundant testcase.".
Thanks.
^ permalink raw reply
* Re: [GSOC][PATCH v3 1/1] t7301: use test_path_is_(missing|file)
From: Junio C Hamano @ 2024-03-04 21:50 UTC (permalink / raw)
To: Vincenzo Mezzela; +Cc: git
In-Reply-To: <20240304171732.64457-2-vincenzo.mezzela@gmail.com>
Vincenzo Mezzela <vincenzo.mezzela@gmail.com> writes:
> Refactor test -f to utilize the corresponding helper functions from
As Eric pointed out to another GSoC applicant in a different thread,
this is not refactoring any code, so saying "refactor" is a bit
misleading.
Replace use of "test -f" with the "test_path_is_file" helper
function ...
> test-lib-functions.sh. These functions perform identical operations
> while enhancing debugging capabilities in case of test failures.
>
> In the context of this file, 'test ! -f' is meant to check if the file
> has been correctly cleaned, thus its usage is replaced with
> 'test_path_is_missing' instead of '! test_path_is_file'.
In other words, the original used "test ! -f" when it meant to say
"test ! -e", and test_path_is_missing would be a correct replacement?
If so that makes sense.
^ permalink raw reply
* Re: [PATCH v2] setup: remove unnecessary variable
From: Ghanshyam Thakkar @ 2024-03-04 21:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: christian.couder, git, johannes.schindelin, newren
In-Reply-To: <xmqqjzmhq2vb.fsf@gitster.g>
On Mon Mar 4, 2024 at 11:46 PM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> > -
> > -test_expect_success 'template can set core.bare but overridden by command line' '
> > - test_when_finished "rm -rf subdir" &&
> > - test_when_finished "rm -rf templates" &&
> > - test_config core.bare true &&
> > - umask 0022 &&
> > - mkdir -p templates/ &&
> > - cp .git/config templates/config &&
> > - git init --no-bare --template=templates subdir &&
> > - test_path_exists subdir/.git/HEAD
> > -'
>
> This removal is a bit unexpected. Is it because we established with
> the previous test that core.bare in the template should not affect
> the outcome, so this is not worth testing?
Yes, in the previous testcase we determined that template cannot set
core.bare. Therefore, this testcase would be like testing
--bare/--no-bare option, which is already done in 0001-init.sh and
t5601-clone.sh. However, I don't have strong opinion on this. I can add
it back if you think it is worth it.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] clean: improve -n and -f implementation and documentation
From: Sergey Organov @ 2024-03-04 20:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jean-Noël AVILA, Kristoffer Haugsbakk
In-Reply-To: <xmqqo7btom4u.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>>> The reason for the behaviour can be explained this way:
>>>
>>> * "git clean" (with neither -i nor -n. The user wants the default
>>> mode that has no built-in protection will be stopped without -f.
>>>
>>> * "git clean -n". The user wants the dry-run mode that has its own
>>> protection, i.e. being always no-op to the files, so there is no
>>> need to fail here for the lack of "-f".
>>>
>>> * "git clean --interactive". The user wants the interactive mode
>>> that has its own protection, i.e. giving the end-user a chance to
>>> say "oh, I didn't mean to remove these files, 'q'uit from this
>>> mistake", so there is no need to fail here for the lack of "-f".
>>
>> Well, if we remove -i from error message as well, then yes, this makes
>> sense.
>> ...
>> I then suggest to consider to remove mention of -i from
>> clean.requireForce description as well.
>
> The follow-up patch you just reviewed in the other thread does
> exactly that.
Yeah, the follow-up patch somehow didn't thread correctly with original
discussion, so I've noticed it only after I wrote the above, and the
patch is fine indeed.
>
> This is a tangent, but before finalizing the version that complains
> "clean.requireForce is in effect and you did not give me -f" without
> mentioning "-i" or "-n", I asked gemini.google.com to proofread the
> patch and and one of its suggestion was to use this:
>
> "clean.requireForce is true. Use -f to override, or consider
> using -n (dry-run) or -i (interactive) for a safer workflow."
>
> as a possibly cleaner message. It is the opposite of what both of
> us concluded to be good in this exchange, but in some sense, it does
> sound more helpful to end users, which I somehow found amusing.
The added advice looks fine to me, as it explicitly separates -f from
the other ways of using "git clean". However, starting phrase with
"clean.requireForce is true" sounds strange. I'd rather say:
"Refusing to remove files: use -f to force removal. Alternatively,
consider using -n (dry-run) or -i (interactive) for a safer workflow.
Set clean.requireForce to false to get rid of this message"
Here we first state what has happened, and then mention solutions in
most-probable-first order.
However, if I were gemini, I'd probably start from noticing that no
error message is required at all unless there is something to delete in
the first place. I.e., the error should probably occur not here, but
rather at every attempt to delete, and then explanation should be given
later, e.g.:
Refusing to remove FILE1
Refusing to remove FILE2
No files were removed: use -f to force removal. Alternatively,
consider using -n (dry-run) or -i (interactive) for a safer workflow.
Set clean.requireForce to false to disable this protection.
With this, user effectively gets functionality similar to "git clean -n"
by default.
Just saying.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH v2] clean: improve -n and -f implementation and documentation
From: Junio C Hamano @ 2024-03-04 19:03 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, Jean-Noël AVILA, Kristoffer Haugsbakk
In-Reply-To: <87h6hl96z7.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
>> The reason for the behaviour can be explained this way:
>>
>> * "git clean" (with neither -i nor -n. The user wants the default
>> mode that has no built-in protection will be stopped without -f.
>>
>> * "git clean -n". The user wants the dry-run mode that has its own
>> protection, i.e. being always no-op to the files, so there is no
>> need to fail here for the lack of "-f".
>>
>> * "git clean --interactive". The user wants the interactive mode
>> that has its own protection, i.e. giving the end-user a chance to
>> say "oh, I didn't mean to remove these files, 'q'uit from this
>> mistake", so there is no need to fail here for the lack of "-f".
>
> Well, if we remove -i from error message as well, then yes, this makes
> sense.
> ...
> I then suggest to consider to remove mention of -i from
> clean.requireForce description as well.
The follow-up patch you just reviewed in the other thread does
exactly that.
This is a tangent, but before finalizing the version that complains
"clean.requireForce is in effect and you did not give me -f" without
mentioning "-i" or "-n", I asked gemini.google.com to proofread the
patch and and one of its suggestion was to use this:
"clean.requireForce is true. Use -f to override, or consider
using -n (dry-run) or -i (interactive) for a safer workflow."
as a possibly cleaner message. It is the opposite of what both of
us concluded to be good in this exchange, but in some sense, it does
sound more helpful to end users, which I somehow found amusing.
^ permalink raw reply
* Re: [PATCH 1/2] fuzz: fix fuzz test build rules
From: Josh Steadmon @ 2024-03-04 18:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqcytxhrgs.fsf@gitster.g>
On 2024.01.19 14:46, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > @@ -762,7 +763,7 @@ fuzz-objs: $(FUZZ_OBJS)
> > # Always build fuzz objects even if not testing, to prevent bit-rot.
> > all:: $(FUZZ_OBJS)
>
> So, this is what you referred to in your proposed log message. We
> do build objects to prevent bit-rot, but we do not link, so it is
> merely half a protection.
>
> > ...
> > fuzz-all: $(FUZZ_PROGRAMS)
>
> But there is this target. I wonder if it makes it even better to
> update the "always build fuzz objects" one? Given that some folks
> may not have the necessary clang toochain for linking, it may
> probably be a bit too much, perhaps?
Indeed, this would have caught the previous common-main issue. I'll send
a followup to fix this, thanks for the suggestion!
^ permalink raw reply
* Re: [PATCH v2] clean: improve -n and -f implementation and documentation
From: Sergey Organov @ 2024-03-04 18:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jean-Noël AVILA, Kristoffer Haugsbakk
In-Reply-To: <xmqqa5ndq1op.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> I wonder do you mean:
>>
>> /* Dry run won't remove anything, so requiring force makes no
>> * sense. Interactive has its own means of protection, so don't
>> * require force as well */
>> if (dry_run || interactive)
>> require_force = 0;
>>
>> if (require_force != 0 && !force)
>> die_();
>> ...
>
> That is explained in a few messages after this one, so I'll wait
> until you read them all before responding ;-).
Ah, yeah, got it now! So no further response is needed.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH 3/3] t-ctype: do one test per class and char
From: Junio C Hamano @ 2024-03-04 18:46 UTC (permalink / raw)
To: Josh Steadmon
Cc: René Scharfe, Christian Couder, git, Phillip Wood, Achu Luma
In-Reply-To: <ZeYUZZ6Z8VtYnBn7@google.com>
Josh Steadmon <steadmon@google.com> writes:
> On 2024.03.02 23:00, René Scharfe wrote:
> ...
>> I was starting the programs from t/unit-tests/bin/ individually because
>> I didn't know 'make unit-tests' exists. This is much nicer, thank you!
>> Especially after adding 'DEFAULT_UNIT_TEST_TARGET = unit-tests-prove' to
>> config.mak to complement the 'DEFAULT_TEST_TARGET = prove' I added long
>> ago. It would be even nicer if the former was the default when the
>> latter is set.
>
> After js/unit-test-suite-runner [1] is merged, then using
> 'DEFAULT_TEST_TARGET = prove' will also run the unit tests alongside the
> shell test suite.
>
> [1] https://lore.kernel.org/git/cover.1708728717.git.steadmon@google.com/
Nice ;-).
^ permalink raw reply
* Re: [PATCH 1/1] clean: further clean-up of implementation around "--force"
From: Sergey Organov @ 2024-03-04 18:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20240303220600.2491792-2-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> We clarified how clean.requireForce interacts with the --dry-run
> option in the previous commit, both in the implementation and in the
> documentation. Even when "git clean" (without other options) is
> required to be used with "--force" (i.e. either clean.requireForce
> is unset, or explicitly set to true) to protect end-users from
> casual invocation of the command by mistake, "--dry-run" does not
> require "--force" to be used, because it is already its own
> protection mechanism by being a no-op to the working tree files.
>
> The previous commit, however, missed another clean-up opportunity
> around the same area. Just like in the "--dry-run" mode, the
> command in the "--interactive" mode does not require "--force",
> either. This is because by going interactive and giving the end
> user one more step to confirm, the mode itself is serving as its own
> protection mechanism.
>
> Let's take things one step further, unify the code that defines
> interaction between `--force` and these two other options. Just
> like we added explanation for the reason why "--dry-run" does not
> honor `clean.requireForce`, add the same explanation for
> "--interactive". Finally, add some tests to show the interaction
> between "--force" and "--interactive" (we already have tests that
> show interaction between "--force" and "--dry-run").
Looks fine to me, including the patch itself.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH v2] clean: improve -n and -f implementation and documentation
From: Junio C Hamano @ 2024-03-04 18:41 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, Jean-Noël AVILA, Kristoffer Haugsbakk
In-Reply-To: <87h6hl96z7.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> I wonder do you mean:
>
> /* Dry run won't remove anything, so requiring force makes no
> * sense. Interactive has its own means of protection, so don't
> * require force as well */
> if (dry_run || interactive)
> require_force = 0;
>
> if (require_force != 0 && !force)
> die_();
> ...
That is explained in a few messages after this one, so I'll wait
until you read them all before responding ;-).
Thanks.
^ permalink raw reply
* Re: [PATCH v2] clean: improve -n and -f implementation and documentation
From: Sergey Organov @ 2024-03-04 18:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jean-Noël AVILA, Kristoffer Haugsbakk
In-Reply-To: <20240303220600.2491792-1-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> Changes since v1:
>>
>> * Fixed style of the if() statement
>>
>> * Merged two error messages into one
>>
>> * clean.requireForce description changed accordingly
>
> Excellent.
>
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index d90766cad3a0..41502dcb0dde 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -25,7 +25,7 @@
>> #include "help.h"
>> #include "prompt.h"
>>
>> -static int force = -1; /* unset */
>> +static int require_force = -1; /* unset */
>> static int interactive;
>> static struct string_list del_list = STRING_LIST_INIT_DUP;
>> static unsigned int colopts;
>> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const char *value,
>> }
>>
>> if (!strcmp(var, "clean.requireforce")) {
>> - force = !git_config_bool(var, value);
>> + require_force = git_config_bool(var, value);
>> return 0;
>> }
>>
>> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>> {
>> int i, res;
>> int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
>> - int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
>> + int ignored_only = 0, force = 0, errors = 0, gone = 1;
>> int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
>> struct strbuf abs_path = STRBUF_INIT;
>> struct dir_struct dir = DIR_INIT;
>> @@ -946,22 +946,17 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>> };
>>
>> git_config(git_clean_config, NULL);
>> - if (force < 0)
>> - force = 0;
>> - else
>> - config_set = 1;
>
> The above changes are a significant improvement. Instead of a
> single "force" variable whose meaning is fuzzy, we now have
> "require_force" to track the config setting, and "force" to indicate
> the "--force" option. THis makes the code's intent much clearer.
>
>> argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
>> 0);
>>
>> - if (!interactive && !dry_run && !force) {
>> - if (config_set)
>> - die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
>> - "refusing to clean"));
>> - else
>> - die(_("clean.requireForce defaults to true and neither -i, -n, nor -f given;"
>
> And thanks to that, the above trick with an extra variable "config_set",
> which smells highly a round-about way, can be simplified.
>
>> + /* Dry run won't remove anything, so requiring force makes no sense */
>> + if (dry_run)
>> + require_force = 0;
>> + if (require_force != 0 && !force && !interactive)
>
> However, the above logic could be improved. The behaviour we have,
> for a user who does *not* explicitly disable config.requireForce,
> is, that when clean.requireForce is not set to 0, we would fail
> unless one of these is in effect: -f, -n, -i. Even though using
> either -n or -i makes it unnecessary to use -f *exactly* the same
> way, the above treats dry_run and interactive separately with two if
> statements, which is suboptimal as a "code/logic clean-up".
I wonder do you mean:
/* Dry run won't remove anything, so requiring force makes no
* sense. Interactive has its own means of protection, so don't
* require force as well */
if (dry_run || interactive)
require_force = 0;
if (require_force != 0 && !force)
die_();
that looks fine to me, as it puts 'force' flag and corresponding
configuration into one if(), whereas both exceptions are put into
another. OTOH, having:
if (require_force != 0 && !force && !interactive && !dry_run)
die_();
mixture looks less appealing to me, though I won't fight against it
either.
>
> The reason for the behaviour can be explained this way:
>
> * "git clean" (with neither -i nor -n. The user wants the default
> mode that has no built-in protection will be stopped without -f.
>
> * "git clean -n". The user wants the dry-run mode that has its own
> protection, i.e. being always no-op to the files, so there is no
> need to fail here for the lack of "-f".
>
> * "git clean --interactive". The user wants the interactive mode
> that has its own protection, i.e. giving the end-user a chance to
> say "oh, I didn't mean to remove these files, 'q'uit from this
> mistake", so there is no need to fail here for the lack of "-f".
Well, if we remove -i from error message as well, then yes, this makes
sense.
>
>> + die(_("clean.requireForce is true and neither -f nor -i given:"
>> " refusing to clean"));
>
> The message is certainly cleaner compared to the previous round, but
> this also can be improved. Stepping back a bit and thinking who are
> the target audience of this message. The only users who see this
> message are running "git clean" in its default (unprotected) mode,
> and they wanted to "clean" for real. If they wanted to do dry-run,
> they would have said "-n" themselves, and that is why we can safely
> omit mention of "-n" we had in the original message.
>
> These users did not want to run the interractive clean, either---if
> they wanted to go interractive, they would have said "-i"
> themselves. So we do not need to mention "-i" either for exactly
> the same logic.
I then suggest to consider to remove mention of -i from
clean.requireForce description as well.
>
> Based on the above observation,
>
> I'll send a follow-up patch to clean up the code around here (both
> implementation and documentation), taking '-i' into account as well.
Fine, thanks!
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH 3/3] t-ctype: do one test per class and char
From: Josh Steadmon @ 2024-03-04 18:35 UTC (permalink / raw)
To: René Scharfe; +Cc: Christian Couder, git, Phillip Wood, Achu Luma
In-Reply-To: <bd48f19b-0600-4e64-835b-98d3a97bb7f2@web.de>
On 2024.03.02 23:00, René Scharfe wrote:
> Am 27.02.24 um 11:04 schrieb Christian Couder:
> > On Mon, Feb 26, 2024 at 7:58 PM Josh Steadmon <steadmon@google.com> wrote:
> >>
> >> On 2024.02.26 18:26, René Scharfe wrote:
> >
> >>> The output is clean as well, but there's a lot of it. Perhaps too much.
> >>> The success messages are boring, though, and if all checks pass then the
> >>> only useful information is the status code. A TAP harness like prove
> >>> summarizes that nicely:
> >>>
> >>> $ prove t/unit-tests/bin/t-ctype
> >>> t/unit-tests/bin/t-ctype .. ok
> >>> All tests successful.
> >>> Files=1, Tests=3598, 0 wallclock secs ( 0.08 usr + 0.00 sys = 0.08 CPU)
> >>> Result: PASS
> >>>
> >>> Filtering out passing checks e.g. with "| grep -v ^ok" would help when
> >>> debugging a test failure. I vaguely miss the --immediate switch from the
> >>> regular test library, however.
> >>
> >> Yeah, I agree here. It's a lot of output but it's almost always going to
> >> be consumed by a test harness rather than a human, and it's easy to
> >> filter out the noise if someone does need to do some manual debugging.
> >
> > Yeah, I know about TAP harnesses like prove, but the most
> > straightforward way to run the unit tests is still `make unit-tests`
> > in the t/ directory. Also when you add or change some tests, it's a
> > good idea to run `make unit-tests` to see what the output is, so you
> > still have to see that output quite often when you work on tests and
> > going through 3598 of mostly useless output instead of just 14 isn't
> > nice.
>
> I was starting the programs from t/unit-tests/bin/ individually because
> I didn't know 'make unit-tests' exists. This is much nicer, thank you!
> Especially after adding 'DEFAULT_UNIT_TEST_TARGET = unit-tests-prove' to
> config.mak to complement the 'DEFAULT_TEST_TARGET = prove' I added long
> ago. It would be even nicer if the former was the default when the
> latter is set.
After js/unit-test-suite-runner [1] is merged, then using
'DEFAULT_TEST_TARGET = prove' will also run the unit tests alongside the
shell test suite.
[1] https://lore.kernel.org/git/cover.1708728717.git.steadmon@google.com/
^ permalink raw reply
* Re: [PATCH v2] setup: remove unnecessary variable
From: Junio C Hamano @ 2024-03-04 18:16 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: christian.couder, git, johannes.schindelin, newren
In-Reply-To: <20240304151811.511780-1-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> The TODO comment suggested to heed core.bare from template config file
> if no command line override given. And the prev_bare_repository
> variable seems to have been placed for this sole purpose as it is not
> used anywhere else.
OK.
> However, it was clarified by Junio [1] that such values (including
> core.bare) are ignored intentionally and does not make sense to
> propagate them from template config to repository config. Also, the
> directories for the worktree and repository are already created, and
> therefore the bare/non-bare decision has already been made, by the
> point we reach the codepath where the TODO comment is placed.
Correct. Who said it is much less interesting than what was said,
so I would have written the first part of the paragraph more like
Values including core.bare from the template file are
ignored on purpose because they may not make sense for the
repository being created [1]. Also, the directories for ...
but I'll let it pass.
> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
> index b1eb5c01b8..29cf8a9661 100755
> --- a/t/t1301-shared-repo.sh
> +++ b/t/t1301-shared-repo.sh
> @@ -52,7 +52,7 @@ test_expect_success 'shared=all' '
> test 2 = $(git config core.sharedrepository)
> '
>
> -test_expect_failure 'template can set core.bare' '
> +test_expect_success 'template cannot set core.bare' '
> test_when_finished "rm -rf subdir" &&
> test_when_finished "rm -rf templates" &&
> test_config core.bare true &&
> @@ -60,18 +60,7 @@ test_expect_failure 'template can set core.bare' '
> mkdir -p templates/ &&
> cp .git/config templates/config &&
> git init --template=templates subdir &&
> - test_path_exists subdir/HEAD
> + test_path_is_missing subdir/HEAD
> '
So we used to say "subdir should be created as a bare repository but
we fail to do so", but now "subdir should become a non-bare repository
because 'git init' is run without the --bare option". OK.
> -
> -test_expect_success 'template can set core.bare but overridden by command line' '
> - test_when_finished "rm -rf subdir" &&
> - test_when_finished "rm -rf templates" &&
> - test_config core.bare true &&
> - umask 0022 &&
> - mkdir -p templates/ &&
> - cp .git/config templates/config &&
> - git init --no-bare --template=templates subdir &&
> - test_path_exists subdir/.git/HEAD
> -'
This removal is a bit unexpected. Is it because we established with
the previous test that core.bare in the template should not affect
the outcome, so this is not worth testing?
> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index a400bcca62..e93e0d0cc3 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -120,14 +120,14 @@ test_expect_success 'prefers -c config over --template config' '
>
> '
>
> -test_expect_failure 'prefers --template config even for core.bare' '
> +test_expect_success 'ignore --template config for core.bare' '
>
> template="$TRASH_DIRECTORY/template-with-bare-config" &&
> mkdir "$template" &&
> git config --file "$template/config" core.bare true &&
> git clone "--template=$template" parent clone-bare-config &&
> - test "$(git -C clone-bare-config config --local core.bare)" = "true" &&
> - test_path_is_file clone-bare-config/HEAD
> + test "$(git -C clone-bare-config config --local core.bare)" = "false" &&
> + test_path_is_missing clone-bare-config/HEAD
> '
This is in the same spirit as the first change in t1301, which seems
OK.
Thanks.
^ permalink raw reply
* Re: [PATCH] sequencer: allow disabling conflict advice
From: Junio C Hamano @ 2024-03-04 17:56 UTC (permalink / raw)
To: Phillip Wood; +Cc: Philippe Blain via GitGitGadget, git, Philippe Blain
In-Reply-To: <6a31efcc-c6c2-4729-80b9-eecff4ec9e31@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> ... So we probably do need a new config variable but
> I think it should have a generic name - not be sequencer specific so
> we can extend its scope in the future to "git merge", "git am -3",
> "git stash" etc.
A very good point. Thanks for your careful thinking.
^ permalink raw reply
* Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions
From: Junio C Hamano @ 2024-03-04 17:50 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: shejialuo, git, Eric Sunshine
In-Reply-To: <ZeWbdvFmhUYN9ekE@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> This version looks good to me, thanks!
>
> One suggestion for potential future contributions by you: it's always
> helpful to create a "range-diff" of what has changed between the
> previous version of your patch series and the next one. Like this,
> reviewers can immediately see what the difference is between the two
> versions, which helps them to get the review done faster.
>
> Assuming you use git-format-patch(1) you can generate such a range diff
> with the `--range-diff=` parameter.
Thanks for a review.
^ permalink raw reply
* Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions
From: Junio C Hamano @ 2024-03-04 17:22 UTC (permalink / raw)
To: shejialuo; +Cc: git, Eric Sunshine, Patrick Steinhardt
In-Reply-To: <20240304095436.56399-2-shejialuo@gmail.com>
shejialuo <shejialuo@gmail.com> writes:
> test -(e|d) does not provide a nice error message when we hit test
> failures, so use test_path_exists, test_path_is_dir instead.
OK.
>
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
Just for the next single-patch topic you'd work on, here below the
three-dash line is where you may mention what's different between
the previous iteration and this one, if you wanted to, instead of
having a separate cover-letter message.
> t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
The patch looks good to me. Thanks (and thanks for all the
reviewers of the previous rounds).
^ permalink raw reply
* Re: [PATCH v4 0/1] Change commit message
From: Junio C Hamano @ 2024-03-04 17:19 UTC (permalink / raw)
To: shejialuo; +Cc: git, Eric Sunshine, Patrick Steinhardt
In-Reply-To: <20240304095436.56399-1-shejialuo@gmail.com>
shejialuo <shejialuo@gmail.com> writes:
> This version changes the last version's error message.
A cover letter is way overkill to tell the above to those who have
read the previous iteration (which is minority of the reviewer
population). A comment after the three-dash line in the main patch
would be more appropriate.
If you need to have a cover letter, its title shouldn't be about the
differences between the previous round and this round. It should be
about the topic of the "series".
Thanks.
^ permalink raw reply
* [GSOC][PATCH v3 1/1] t7301: use test_path_is_(missing|file)
From: Vincenzo Mezzela @ 2024-03-04 17:17 UTC (permalink / raw)
To: git; +Cc: Vincenzo Mezzela
In-Reply-To: <20240304171732.64457-1-vincenzo.mezzela@gmail.com>
Refactor test -f to utilize the corresponding helper functions from
test-lib-functions.sh. These functions perform identical operations
while enhancing debugging capabilities in case of test failures.
In the context of this file, 'test ! -f' is meant to check if the file
has been correctly cleaned, thus its usage is replaced with
'test_path_is_missing' instead of '! test_path_is_file'.
Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
---
t/t7301-clean-interactive.sh | 490 +++++++++++++++++------------------
1 file changed, 245 insertions(+), 245 deletions(-)
diff --git a/t/t7301-clean-interactive.sh b/t/t7301-clean-interactive.sh
index d82a3210a1..4afe53c66a 100755
--- a/t/t7301-clean-interactive.sh
+++ b/t/t7301-clean-interactive.sh
@@ -25,18 +25,18 @@ test_expect_success 'git clean -i (c: clean hotkey)' '
touch a.out src/part3.c src/part3.h src/part4.c src/part4.h \
docs/manual.txt obj.o build/lib.so &&
echo c | git clean -i &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test ! -f a.out &&
- test -f docs/manual.txt &&
- test ! -f src/part3.c &&
- test ! -f src/part3.h &&
- test ! -f src/part4.c &&
- test ! -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_missing a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_missing src/part3.c &&
+ test_path_is_missing src/part3.h &&
+ test_path_is_missing src/part4.c &&
+ test_path_is_missing src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -46,18 +46,18 @@ test_expect_success 'git clean -i (cl: clean prefix)' '
touch a.out src/part3.c src/part3.h src/part4.c src/part4.h \
docs/manual.txt obj.o build/lib.so &&
echo cl | git clean -i &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test ! -f a.out &&
- test -f docs/manual.txt &&
- test ! -f src/part3.c &&
- test ! -f src/part3.h &&
- test ! -f src/part4.c &&
- test ! -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_missing a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_missing src/part3.c &&
+ test_path_is_missing src/part3.h &&
+ test_path_is_missing src/part4.c &&
+ test_path_is_missing src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -67,18 +67,18 @@ test_expect_success 'git clean -i (quit)' '
touch a.out src/part3.c src/part3.h src/part4.c src/part4.h \
docs/manual.txt obj.o build/lib.so &&
echo quit | git clean -i &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test -f a.out &&
- test -f docs/manual.txt &&
- test -f src/part3.c &&
- test -f src/part3.h &&
- test -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_file a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_file src/part3.c &&
+ test_path_is_file src/part3.h &&
+ test_path_is_file src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -88,18 +88,18 @@ test_expect_success 'git clean -i (Ctrl+D)' '
touch a.out src/part3.c src/part3.h src/part4.c src/part4.h \
docs/manual.txt obj.o build/lib.so &&
echo "\04" | git clean -i &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test -f a.out &&
- test -f docs/manual.txt &&
- test -f src/part3.c &&
- test -f src/part3.h &&
- test -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_file a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_file src/part3.c &&
+ test_path_is_file src/part3.h &&
+ test_path_is_file src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -110,18 +110,18 @@ test_expect_success 'git clean -id (filter all)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines f "*" "" c |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test -f a.out &&
- test -f docs/manual.txt &&
- test -f src/part3.c &&
- test -f src/part3.h &&
- test -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_file a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_file src/part3.c &&
+ test_path_is_file src/part3.h &&
+ test_path_is_file src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -132,18 +132,18 @@ test_expect_success 'git clean -id (filter patterns)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines f "part3.* *.out" "" c |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test -f a.out &&
- test ! -f docs/manual.txt &&
- test -f src/part3.c &&
- test -f src/part3.h &&
- test ! -f src/part4.c &&
- test ! -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_file a.out &&
+ test_path_is_missing docs/manual.txt &&
+ test_path_is_file src/part3.c &&
+ test_path_is_file src/part3.h &&
+ test_path_is_missing src/part4.c &&
+ test_path_is_missing src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -154,18 +154,18 @@ test_expect_success 'git clean -id (filter patterns 2)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines f "* !*.out" "" c |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test ! -f a.out &&
- test -f docs/manual.txt &&
- test -f src/part3.c &&
- test -f src/part3.h &&
- test -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_missing a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_file src/part3.c &&
+ test_path_is_file src/part3.h &&
+ test_path_is_file src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -176,18 +176,18 @@ test_expect_success 'git clean -id (select - all)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines s "*" "" c |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test ! -f a.out &&
- test ! -f docs/manual.txt &&
- test ! -f src/part3.c &&
- test ! -f src/part3.h &&
- test ! -f src/part4.c &&
- test ! -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_missing a.out &&
+ test_path_is_missing docs/manual.txt &&
+ test_path_is_missing src/part3.c &&
+ test_path_is_missing src/part3.h &&
+ test_path_is_missing src/part4.c &&
+ test_path_is_missing src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -198,18 +198,18 @@ test_expect_success 'git clean -id (select - none)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines s "" c |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test -f a.out &&
- test -f docs/manual.txt &&
- test -f src/part3.c &&
- test -f src/part3.h &&
- test -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_file a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_file src/part3.c &&
+ test_path_is_file src/part3.h &&
+ test_path_is_file src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -220,18 +220,18 @@ test_expect_success 'git clean -id (select - number)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines s 3 "" c |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test -f a.out &&
- test -f docs/manual.txt &&
- test ! -f src/part3.c &&
- test -f src/part3.h &&
- test -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_file a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_missing src/part3.c &&
+ test_path_is_file src/part3.h &&
+ test_path_is_file src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -242,18 +242,18 @@ test_expect_success 'git clean -id (select - number 2)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines s "2 3" 5 "" c |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test -f a.out &&
- test ! -f docs/manual.txt &&
- test ! -f src/part3.c &&
- test -f src/part3.h &&
- test ! -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_file a.out &&
+ test_path_is_missing docs/manual.txt &&
+ test_path_is_missing src/part3.c &&
+ test_path_is_file src/part3.h &&
+ test_path_is_missing src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -264,18 +264,18 @@ test_expect_success 'git clean -id (select - number 3)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines s "3,4 5" "" c |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test -f a.out &&
- test -f docs/manual.txt &&
- test ! -f src/part3.c &&
- test ! -f src/part3.h &&
- test ! -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_file a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_missing src/part3.c &&
+ test_path_is_missing src/part3.h &&
+ test_path_is_missing src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -285,11 +285,11 @@ test_expect_success 'git clean -id (select - filenames)' '
touch a.out foo.txt bar.txt baz.txt &&
test_write_lines s "a.out fo ba bar" "" c |
git clean -id &&
- test -f Makefile &&
- test ! -f a.out &&
- test ! -f foo.txt &&
- test ! -f bar.txt &&
- test -f baz.txt &&
+ test_path_is_file Makefile &&
+ test_path_is_missing a.out &&
+ test_path_is_missing foo.txt &&
+ test_path_is_missing bar.txt &&
+ test_path_is_file baz.txt &&
rm baz.txt
'
@@ -301,18 +301,18 @@ test_expect_success 'git clean -id (select - range)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines s "1,3-4" 2 "" c |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test ! -f a.out &&
- test ! -f src/part3.c &&
- test ! -f src/part3.h &&
- test -f src/part4.c &&
- test -f src/part4.h &&
- test ! -f docs/manual.txt &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_missing a.out &&
+ test_path_is_missing src/part3.c &&
+ test_path_is_missing src/part3.h &&
+ test_path_is_file src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_missing docs/manual.txt &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -323,18 +323,18 @@ test_expect_success 'git clean -id (select - range 2)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines s "4- 1" "" c |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test ! -f a.out &&
- test -f docs/manual.txt &&
- test -f src/part3.c &&
- test ! -f src/part3.h &&
- test ! -f src/part4.c &&
- test ! -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_missing a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_file src/part3.c &&
+ test_path_is_missing src/part3.h &&
+ test_path_is_missing src/part4.c &&
+ test_path_is_missing src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -345,18 +345,18 @@ test_expect_success 'git clean -id (inverse select)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines s "*" "-5- 1 -2" "" c |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test ! -f a.out &&
- test -f docs/manual.txt &&
- test ! -f src/part3.c &&
- test ! -f src/part3.h &&
- test -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_missing a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_missing src/part3.c &&
+ test_path_is_missing src/part3.h &&
+ test_path_is_file src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -367,18 +367,18 @@ test_expect_success 'git clean -id (ask)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines a Y y no yes bad "" |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test ! -f a.out &&
- test ! -f docs/manual.txt &&
- test -f src/part3.c &&
- test ! -f src/part3.h &&
- test -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_missing a.out &&
+ test_path_is_missing docs/manual.txt &&
+ test_path_is_file src/part3.c &&
+ test_path_is_missing src/part3.h &&
+ test_path_is_file src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -389,18 +389,18 @@ test_expect_success 'git clean -id (ask - Ctrl+D)' '
docs/manual.txt obj.o build/lib.so &&
test_write_lines a Y no yes "\04" |
git clean -id &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test ! -f a.out &&
- test -f docs/manual.txt &&
- test ! -f src/part3.c &&
- test -f src/part3.h &&
- test -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_missing a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_missing src/part3.c &&
+ test_path_is_file src/part3.h &&
+ test_path_is_file src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -412,18 +412,18 @@ test_expect_success 'git clean -id with prefix and path (filter)' '
(cd build/ &&
test_write_lines f docs "*.h" "" c |
git clean -id ..) &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test ! -f a.out &&
- test -f docs/manual.txt &&
- test ! -f src/part3.c &&
- test -f src/part3.h &&
- test ! -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_missing a.out &&
+ test_path_is_file docs/manual.txt &&
+ test_path_is_missing src/part3.c &&
+ test_path_is_file src/part3.h &&
+ test_path_is_missing src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -435,18 +435,18 @@ test_expect_success 'git clean -id with prefix and path (select by name)' '
(cd build/ &&
test_write_lines s ../docs/ ../src/part3.c ../src/part4.c "" c |
git clean -id ..) &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test -f a.out &&
- test ! -f docs/manual.txt &&
- test ! -f src/part3.c &&
- test -f src/part3.h &&
- test ! -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_file a.out &&
+ test_path_is_missing docs/manual.txt &&
+ test_path_is_missing src/part3.c &&
+ test_path_is_file src/part3.h &&
+ test_path_is_missing src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
@@ -458,18 +458,18 @@ test_expect_success 'git clean -id with prefix and path (ask)' '
(cd build/ &&
test_write_lines a Y y no yes bad "" |
git clean -id ..) &&
- test -f Makefile &&
- test -f README &&
- test -f src/part1.c &&
- test -f src/part2.c &&
- test ! -f a.out &&
- test ! -f docs/manual.txt &&
- test -f src/part3.c &&
- test ! -f src/part3.h &&
- test -f src/part4.c &&
- test -f src/part4.h &&
- test -f obj.o &&
- test -f build/lib.so
+ test_path_is_file Makefile &&
+ test_path_is_file README &&
+ test_path_is_file src/part1.c &&
+ test_path_is_file src/part2.c &&
+ test_path_is_missing a.out &&
+ test_path_is_missing docs/manual.txt &&
+ test_path_is_file src/part3.c &&
+ test_path_is_missing src/part3.h &&
+ test_path_is_file src/part4.c &&
+ test_path_is_file src/part4.h &&
+ test_path_is_file obj.o &&
+ test_path_is_file build/lib.so
'
--
2.34.1
^ permalink raw reply related
* [GSOC][PATCH v3 0/1] microproject: use test_path_is_* functions in test scripts
From: Vincenzo Mezzela @ 2024-03-04 17:17 UTC (permalink / raw)
To: git; +Cc: Vincenzo Mezzela
In-Reply-To: <20240227161734.52830-1-vincenzo.mezzela@gmail.com>
Hi,
Following previous discussions[1][2][3], this patch is submitted as a microproject
for the application to the GSOC.
Thanks,
Vincenzo
Changes in V2:
* Fixed commit message[2].
Changes in V3:
* Fixed commit message[3].
[1] https://lore.kernel.org/git/xmqqy1bo5k5h.fsf@gitster.g/
[2] https://lore.kernel.org/git/20240219172214.7644-1-vincenzo.mezzela@gmail.com/
[1] https://lore.kernel.org/git/ZeWVB5uKLONfp6cO@tanuki/
Vincenzo Mezzela (1):
t7301: use test_path_is_(missing|file)
t/t7301-clean-interactive.sh | 490 +++++++++++++++++------------------
1 file changed, 245 insertions(+), 245 deletions(-)
--
2.34.1
^ permalink raw reply
* Re: [PATCH] SoC 2024: clarify `test_path_is_*` conversion microproject
From: Junio C Hamano @ 2024-03-04 17:02 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder, Kaartic Sivaraam
In-Reply-To: <84995a068640c72c8f17406ffa0441c7fdba4bdc.1709543804.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> One of our proposed microprojects is to convert instances of `test -e`
> and related functions to instead use `test_path_exists` or similar. This
> conversion is only feasible when `test -e` is not used as part of a
> control statement, as the replacement is used to _assert_ a condition
> instead of merely testing for it.
>
> Clarify the microproject's description accordingly.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> SoC-2024-Microprojects.md | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/SoC-2024-Microprojects.md b/SoC-2024-Microprojects.md
> index 644c0a6..782441f 100644
> --- a/SoC-2024-Microprojects.md
> +++ b/SoC-2024-Microprojects.md
> @@ -41,7 +41,10 @@ to search, so that we can remove this microproject idea.
> Find one test script that verifies the presence/absence of
> files/directories with 'test -(e|f|d|...)' and replace them with the
> appropriate `test_path_is_file`, `test_path_is_dir`, etc. helper
> -functions.
> +functions. Note that this conversion does not directly apply to control
> +flow constructs like `if test -e ./path; then ...; fi` because the
> +replacements are intended to assert the condition instead of merely
> +testing for it.
Thanks for picking it up. Of course there is one case in which we
should use test_path_* helpers to replace such an if...then...fi
construct; e.g., c431a235 (t9146: replace test -d/-e/-f with
appropriate test_path_is_* function, 2024-02-14) did exactly that.
I am not sure how best to express that in the already crowded
description above, though. Rewriting the existing test this way
Find one test script that uses 'test [!] -(e|f|d|...)' to
assert the presence/absense of files/directories to make the
test fail directly with the exit status of such "test"
commands, and replace them with the appropriate helper
functions like `test_path_is_file`, that give more
informative error messages when they fail.
would exclude use of "test -e" as a conditional in control statements,
so we could mention what c431a235 did as an exception to the rule,
perhaps like
Note that the above excludes "test -f" and friends used as a
condition in control statements such as "if test -e path
...", but as an exception, if such a "if" statement just
open-codes what these helpers do, replacing it is warranted.
But that does not read very well, even to myself. Sigh....
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3] unpack: replace xwrite() loop with write_in_full()
From: Junio C Hamano @ 2024-03-04 16:43 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <xmqqfrx6sbdf.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Patrick Steinhardt <ps@pks.im> writes:
>
>>> - while (input_len) {
>>> - err = xwrite(1, input_buffer + input_offset, input_len);
>>> - if (err <= 0)
>>> - break;
>>> - input_len -= err;
>>> - input_offset += err;
>>> - }
>>> + /* Write the last part of the buffer to stdout */
>>> + write_in_full(1, input_buffer + input_offset, input_len);
>>
>> With this change we stop updating `input_len` and `input_offset`, both
>> of which are global variables. Assuming that tests pass this must be
>> okay right now given that this is the final part of what we are writing.
>> But I wonder whether we shouldn't update those regardless just so that
>> these remain consistent?
>
> It is probably a good hygiene, even though it may not matter at all
> for the correctness in the current code.
>
> Thanks for your sharp eyes.
Actually, I changed my mind. As you said, this is flushing the very
end of the data in the input_buffer[] and nobody will fill() the
input_buffer[] after the call to this function happens.
>>> - while (len) {
>>> ...
>>> - len -= ret;
>>> - offset += ret;
>>> - }
>>> + write_in_full(1, buffer + offset, len);
>>
>> Same here.
Ditto. We are about to pass the control back to the caller that
will exit using the "has_errors" we return from here.
>>
>> Patrick
>>
>>> /* All done */
>>> return has_errors;
>>> --
>>> 2.44.0-84-gb387623c12
>>>
>>>
^ permalink raw reply
* Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD
From: Junio C Hamano @ 2024-03-04 16:28 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git, Mike Hommey
In-Reply-To: <ZeVtuqEAelfiA2J9@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> > Now there is a very particular edge case in this situation: when
>> > preparing an empty ref transacton, we end up returning whatever value
>> > `read_ref_without_reload()` returned to the caller. Under normal
>> > conditions this would be fine: "HEAD" should usually exist, and thus the
>> > function would return `0`. But if "HEAD" doesn't exist, the function
>> > returns a positive value which we end up returning to the caller.
>> >
>> > Fix this bug by resetting the return code to `0` and add a test.
So this _will_ surface as a bug when the other change in the series
is applied, but it nevertheless is worth fixing independently of the
other one, because ...
>> > @@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>> > &head_referent, &head_type);
>> > if (ret < 0)
>> > goto done;
>> > + ret = 0;
... after "if the refs->err records an error already, skip
everything we do and return to the caller", we should take the
ownership of what we return (which will be in "ret") from now on.
So the current code uses "ret" as an uninitialized variable, even
not technically so because it is "initialized" to "refs->err"
upfront, and this is like a fix of uninitialized variable use.
>> So this is not really a problem in this function, it's more of that
>> `refs.c:ref_transaction_prepare` checks if `ret` is non-zero.
>
> Well, yes. I'd claim that it is a problem in this function because it
> returns positive even though the transaction was prepared successfully.
>
>> Nit: would be nice to have a comment about why overriding this value is
>> ok.
>
> True.
Yup. It seems we will see a v2 for updating the test code as well,
so I'll assume that you'd explain this as an independent fix (as
well as a required preliminary fix for the other one).
Thanks, both.
^ permalink raw reply
* Re: [PATCH 2/2] builtin/clone: allow remote helpers to detect repo
From: Junio C Hamano @ 2024-03-04 16:17 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Mike Hommey
In-Reply-To: <ZeVt0CAfpYFZqT2i@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> > + * Git commands. To do so, we:
>> > + *
>> > + * - Create an invalid HEAD ref pointing at "refs/heads/.invalid".
>> > + *
>> > + * - Create the "refs/" directory.
>> > + *
>> > + * - Set up the ref storage format and repository version as
>> > + * required.
>>
>> "as required" by whom and in what way?
>>
>> "The code to recognize a repository requires them to be set already,
>> but they do not have to be the real value---we just assign random
>> valid values for now, let remote helper do its work and then set the
>> real values after they are done" would be a plausible interpretation
>> of the above. Is that what is going on?
>
> Partially. While we cannot yet determine the object format, we do know
> the ref storage format as it is specified by the caller when invoking
> git-clone(1) with the `--ref-format=` switch, not by the remote repo.
> In that case, we'd have to set up the ref format accordingly and also
> set the repository version to "1".
Ah, of course. Unlike the object format, we do not say "the other
end uses reftable, so we should do the same" and we do not have to,
thanks to some artificial limitations added to the reftable backend
not to exceed the expressiveness of the files backend.
Thanks.
^ permalink raw reply
* [PATCH 4/4] trace2: remove unneeded calls to generate 'def_param' set
From: Jeff Hostetler via GitGitGadget @ 2024-03-04 15:40 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1679.git.1709566808.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Now that "trace2_cmd_name()" implicitly calls "trace2_cmd_list_config()"
and "trace2_cmd_list_env_vars()", we don't need to explicitly call them.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
git.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/git.c b/git.c
index 7068a184b0a..a769d72ab8f 100644
--- a/git.c
+++ b/git.c
@@ -373,8 +373,6 @@ static int handle_alias(int *argcp, const char ***argv)
strvec_pushv(&child.args, (*argv) + 1);
trace2_cmd_alias(alias_command, child.args.v);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
trace2_cmd_name("_run_shell_alias_");
ret = run_command(&child);
@@ -411,8 +409,6 @@ static int handle_alias(int *argcp, const char ***argv)
COPY_ARRAY(new_argv + count, *argv + 1, *argcp);
trace2_cmd_alias(alias_command, new_argv);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
*argv = new_argv;
*argcp += count - 1;
@@ -462,8 +458,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
trace_argv_printf(argv, "trace: built-in: git");
trace2_cmd_name(p->cmd);
- trace2_cmd_list_config();
- trace2_cmd_list_env_vars();
validate_cache_entries(the_repository->index);
status = p->fn(argc, argv, prefix);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/4] trace2: emit 'def_param' set with 'cmd_name' event
From: Jeff Hostetler via GitGitGadget @ 2024-03-04 15:40 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Jeff Hostetler
In-Reply-To: <pull.1679.git.1709566808.gitgitgadget@gmail.com>
From: Jeff Hostetler <jeffhostetler@github.com>
Some commands do not cause a set of 'def_param' events to be emitted.
This includes "git-remote-https", "git-http-fetch", and various
"query" commands, like "git --man-path".
Since all of these commands do emit a 'cmd_name' event, add code to
the "trace2_cmd_name()" function to generate the set of 'def_param'
events.
We can later remove explicit calls to "trace2_cmd_list_config()" and
"trace2_cmd_list_env_vars()" in git.c.
Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
---
t/t0211-trace2-perf.sh | 6 +++---
trace2.c | 3 +++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 7b353195396..13ef69b92f8 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -320,7 +320,7 @@ test_expect_success 'expect def_params for normal builtin command' '
# Representative query command dispatched in handle_options()
# in git.c
#
-test_expect_failure 'expect def_params for query command' '
+test_expect_success 'expect def_params for query command' '
try_simple "git --man-path" "_query_"
'
@@ -337,7 +337,7 @@ test_expect_failure 'expect def_params for query command' '
# remote-curl.c rather than git.c. Confirm that we get def_param
# events from both layers.
#
-test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '
+test_expect_success 'expect def_params for remote-curl and _run_dashed_' '
test_when_finished "rm prop.perf actual" &&
test_config_global "trace2.configParams" "cfg.prop.*" &&
@@ -366,7 +366,7 @@ test_expect_failure 'expect def_params for remote-curl and _run_dashed_' '
# an executable built from http-fetch.c. Confirm that we get
# def_param events from both layers.
#
-test_expect_failure 'expect def_params for http-fetch and _run_dashed_' '
+test_expect_success 'expect def_params for http-fetch and _run_dashed_' '
test_when_finished "rm prop.perf actual" &&
test_config_global "trace2.configParams" "cfg.prop.*" &&
diff --git a/trace2.c b/trace2.c
index facce641ef3..f894532d053 100644
--- a/trace2.c
+++ b/trace2.c
@@ -433,6 +433,9 @@ void trace2_cmd_name_fl(const char *file, int line, const char *name)
for_each_wanted_builtin (j, tgt_j)
if (tgt_j->pfn_command_name_fl)
tgt_j->pfn_command_name_fl(file, line, name, hierarchy);
+
+ trace2_cmd_list_config();
+ trace2_cmd_list_env_vars();
}
void trace2_cmd_mode_fl(const char *file, int line, const char *mode)
--
gitgitgadget
^ permalink raw reply related
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