* Re: [Bug?] "git diff --no-rename A B"
From: Junio C Hamano @ 2024-01-20 17:55 UTC (permalink / raw)
To: René Scharfe; +Cc: Jeff King, git
In-Reply-To: <579fd5bc-3bfd-427f-b22d-dab5e7e56eb2@web.de>
René Scharfe <l.s.r@web.de> writes:
> Because diff_opt_parse() passes PARSE_OPT_KEEP_UNKNOWN_OPT, which makes
> parse_long_opt() skip abbreviation detection. Which it does since
> baa4adc66a (parse-options: disable option abbreviation with
> PARSE_OPT_KEEP_UNKNOWN, 2019-01-27).
>
> It added a condition only to the if. Its body can be reached via goto
> from two other places, though. So abbreviated options are effectively
> allowed through the back door.
Wow, that is nasty. Thanks for spotting.
I agree with you that the structure of that loop and the processing
in it does look confusing.
> --- >8 ---
> Subject: [PATCH] parse-options: fully disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN
>
> baa4adc66a (parse-options: disable option abbreviation with
> PARSE_OPT_KEEP_UNKNOWN, 2019-01-27) turned off support for abbreviated
> options when the flag PARSE_OPT_KEEP_UNKNOWN is given, as any shortened
> option could also be an abbreviation for one of the unknown options.
>
> The code for handling abbreviated options is guarded by an if, but it
> can also be reached via goto. baa4adc66a only blocked the first way.
> Add the condition to the other ones as well.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Formatted with --function-context for easier review.
> The code is a quite tangled, any ideas how to structure it better?
>
> parse-options.c | 8 +++++---
> t/t4013-diff-various.sh | 6 ++++++
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 4ce2b7ca16..92e96ca6cd 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -353,93 +353,95 @@ static int is_alias(struct parse_opt_ctx_t *ctx,
> static enum parse_opt_result parse_long_opt(
> struct parse_opt_ctx_t *p, const char *arg,
> const struct option *options)
> {
> const char *arg_end = strchrnul(arg, '=');
> const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
> enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
> + int allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);
>
> for (; options->type != OPTION_END; options++) {
> const char *rest, *long_name = options->long_name;
> enum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;
>
> if (options->type == OPTION_SUBCOMMAND)
> continue;
> if (!long_name)
> continue;
>
> again:
> if (!skip_prefix(arg, long_name, &rest))
> rest = NULL;
> if (!rest) {
> /* abbreviated? */
> - if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT) &&
> + if (allow_abbrev &&
> !strncmp(long_name, arg, arg_end - arg)) {
> is_abbreviated:
> if (abbrev_option &&
> !is_alias(p, abbrev_option, options)) {
> /*
> * If this is abbreviated, it is
> * ambiguous. So when there is no
> * exact match later, we need to
> * error out.
> */
> ambiguous_option = abbrev_option;
> ambiguous_flags = abbrev_flags;
> }
> if (!(flags & OPT_UNSET) && *arg_end)
> p->opt = arg_end + 1;
> abbrev_option = options;
> abbrev_flags = flags ^ opt_flags;
> continue;
> }
> /* negation allowed? */
> if (options->flags & PARSE_OPT_NONEG)
> continue;
> /* negated and abbreviated very much? */
> - if (starts_with("no-", arg)) {
> + if (allow_abbrev && starts_with("no-", arg)) {
> flags |= OPT_UNSET;
> goto is_abbreviated;
> }
> /* negated? */
> if (!starts_with(arg, "no-")) {
> if (skip_prefix(long_name, "no-", &long_name)) {
> opt_flags |= OPT_UNSET;
> goto again;
> }
> continue;
> }
> flags |= OPT_UNSET;
> if (!skip_prefix(arg + 3, long_name, &rest)) {
> /* abbreviated and negated? */
> - if (starts_with(long_name, arg + 3))
> + if (allow_abbrev &&
> + starts_with(long_name, arg + 3))
> goto is_abbreviated;
> else
> continue;
> }
> }
> if (*rest) {
> if (*rest != '=')
> continue;
> p->opt = rest + 1;
> }
> return get_value(p, options, flags ^ opt_flags);
> }
>
> if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
> die("disallowed abbreviated or ambiguous option '%.*s'",
> (int)(arg_end - arg), arg);
>
> if (ambiguous_option) {
> error(_("ambiguous option: %s "
> "(could be --%s%s or --%s%s)"),
> arg,
> (ambiguous_flags & OPT_UNSET) ? "no-" : "",
> ambiguous_option->long_name,
> (abbrev_flags & OPT_UNSET) ? "no-" : "",
> abbrev_option->long_name);
> return PARSE_OPT_HELP;
> }
> if (abbrev_option)
> return get_value(p, abbrev_option, abbrev_flags);
> return PARSE_OPT_UNKNOWN;
> }
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index cb094241ec..1e3b2dbea4 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -663,4 +663,10 @@ test_expect_success 'diff --default-prefix overrides diff.mnemonicprefix' '
> check_prefix actual a/file0 b/file0
> '
>
> +test_expect_success 'diff --no-renames cannot be abbreviated' '
> + test_expect_code 129 git diff --no-rename >actual 2>error &&
> + test_must_be_empty actual &&
> + grep "invalid option: --no-rename" error
> +'
> +
> test_done
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH v3] merge-ll: expose revision names to custom drivers
From: Junio C Hamano @ 2024-01-20 17:37 UTC (permalink / raw)
To: Phillip Wood; +Cc: Antonin Delpeuch via GitGitGadget, git, Antonin Delpeuch
In-Reply-To: <82624802-aa7f-4856-b819-9a2990b25a69@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Not part of this patch but I noticed that we're passing the filenames
> for '%A' etc. unquoted which is a bit scary.
May be scary but safe, as long as create_temp() gives a reasonable
temporary filename. We pass ".merge_file_XXXXXX" to xmkstemp(),
which calls into mkstemp(), which should give us a shell safe name?
It also should be a safe conversion to change strbuf_addstr() used
for these three to sq_quote_buf(), as the string with these %[OAB]
placeholders are passed to the shell that eats the quoting before
invoking the end-user supplied external merge driver, which means
the merge driver would not notice any difference.
Thanks for being careful ;-)
^ permalink raw reply
* Re: [PATCH v3] merge-ll: expose revision names to custom drivers
From: Junio C Hamano @ 2024-01-20 17:25 UTC (permalink / raw)
To: Antonin Delpeuch; +Cc: Antonin Delpeuch via GitGitGadget, git
In-Reply-To: <386c0318-0138-47c9-9e7f-d1004277226c@delpeuch.eu>
Antonin Delpeuch <antonin@delpeuch.eu> writes:
> After more testing (combining custom merge drivers with rerere) I
> realized that my patch can lead to a segmentation error. Many
> apologies for not having caught that earlier!
Ah, understandable. The 3-way merge machinery may not even have to
work on commit objects (it can merge two trees, using another tree
as the "common ancestor" tree, just fine).
And in such a case, it is perfectly possible there is no "human
readable name"; all there is may be a tree object name.
> On 18/01/2024 23:09, Antonin Delpeuch via GitGitGadget wrote:
>> @@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
>> strbuf_addf(&cmd, "%d", marker_size);
>> else if (skip_prefix(format, "P", &format))
>> sq_quote_buf(&cmd, path);
>> + else if (skip_prefix(format, "S", &format))
>> + sq_quote_buf(&cmd, orig_name);
>> + else if (skip_prefix(format, "X", &format))
>> + sq_quote_buf(&cmd, name1);
>> + else if (skip_prefix(format, "Y", &format))
>> + sq_quote_buf(&cmd, name2);
>
> The "orig_name", "name1" and "name2" pointers can be NULL at this
> stage. This can happen when the merge is invoked from rerere, to
> resolve a conflict using a previous resolution.
sq_quote_buf(&cmd, name1 ? name1 : "(ours)");
or something like that, perhaps.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2024, #06; Fri, 19)
From: Junio C Hamano @ 2024-01-20 17:19 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git
In-Reply-To: <CYJ62QPHPHCA.2WLKYX1AIZ50T@gmail.com>
"Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:
> On Sat Jan 20, 2024 at 7:26 AM IST, Junio C Hamano wrote:
>> * gt/t0024-style-fixes (2024-01-18) 2 commits
>> - t0024: style fix
>> - t0024: avoid losing exit status to pipes
>>
>> source: <20240118215407.8609-1-shyamthakkar001@gmail.com>
>
> I have rerolled this series, cause it was breaking CI on alpine due to
> a syntax issue which is described here:
> https://lore.kernel.org/git/CYIDCZPQN2H1.1ET0CTP07NMYR@gmail.com/
>
> Thanks.
Thanks for pinging. Yeah, there was that buggy implementation of
tar that requires dash in "-C". Will replace.
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Dragan Simic @ 2024-01-20 15:31 UTC (permalink / raw)
To: Rubén Justo; +Cc: Junio C Hamano, Jeff King, Git List
In-Reply-To: <6a27be56-9d52-46bf-8136-5f1c42379fd1@gmail.com>
On 2024-01-20 12:31, Rubén Justo wrote:
> On 18-ene-2024 21:50:23, Dragan Simic wrote:
>> On 2024-01-18 21:19, Junio C Hamano wrote:
>> > Dragan Simic <dsimic@manjaro.org> writes:
>> > > On 2024-01-18 19:26, Junio C Hamano wrote:
>> > > > Dragan Simic <dsimic@manjaro.org> writes:
>> > > >
>> > > > > Perhaps having both options available could be a good thing.
>> > > > > Though,
>> > > > > adding quite a few knobs may end up confusing the users, so
>> > > > > we should
>> > > > > make sure to document it well.
>> > > >
>> > > > I agree there is a need for documentation, but we are not increasing
>> > > > the number of knobs, are we?
>> > >
>> > > Perhaps there would be one more knob if we end up deciding to
>> > > implement
>> > > support for both approaches, as previously discussed.
>> >
>> > But that would be just one knob (which I personally do not quite see
>> > the need for), not "quite a few knobs" as you said, no?
>>
>> I'm sorry for being imprecise.
>
> Hi Dragan
Hello Rubén!
> My original motivation for starting this series has been fulfilled:
> Give the user an option to tell us not to include the disabling
> instructions alongside the advice.
I like the whole idea, because if someone finds some hint usable and
decides to keep it displayed, displaying the additional hint about
disabling the primary hint (i.e. the advice) simply becomes redundant.
> The current consensus is: if the user explicitly enables visibility for
> an advice, we can stop including the instructions on how to disable its
> visibility.
Totally agreed, simple and effective.
> As reference, in this thread two "global" options have been reviewed:
>
> - To take the disabling instructions as an advice in itself and as
> such, as with the rest, we can have a knob to disable it. Affecting
> all advice messages.
>
> - A new knob to allow the user to set the default visibility for those
> advice messages without an explicit visibility set. And so we can
> take on globally what we now take on locally; if there is not an
> explicit visibility value but there is an explicit "default" value,
> we can stop showing the instruction on how to disable it.
>
>> Regarding the need for that additional "global" knob, I think it can't
>> hurt. Some people might even find it quite useful
>
> I don't quite understand what "global" knob you are referring to. But
> I
> share with you the feeling that a "global" knob might be useful.
The additional "global knob", at least the way I see it, would enable
all
advice messages, overriding all other configuration options that may be
present. It would be like a "big switch" that makes all advices
displayed,
for the case in which someone decides to get rid of the hint that used
to
be useful to them so the advice was disabled, but the hint is no longer
found to be useful to them. In such cases, having no advice displayed
would mean that the user is unable to know easily what knob disables the
no-longer-favored hint.
The reason for "forcing" all advices to be displayed would be to allow
the advices to be displayed without the need to "fish" for the right
knob
to be turned in the configuration file. Though, it wouldn't be perfect
from the usability standpoint, because recreating the right conditions
for displaying some hint and the associated advice may rather easily be
practically infeasible, as already discussed in this thread.
Of course, going through the man pages to find the right knob is always
the best option for those who have the time, but having a "global knob",
to help the users a bit, if possible, in general shouldn't hurt.
I hope it all makes more sense now. Please, let me know if further
clarification is required.
Additionally, the way I envisioned it could also be combined with the
first option for the "global knob" that you listed above, as an
additional
option for it to "force" the advices to be displayed, in addition to the
ability to disable all advices.
> The current iteration for this series looks like it will be merged to
> 'next' soon. In my opinion, it is a good stop for this series, and
> maybe we can explore that 'global' knob in a new one.
I agree, the "global knob" can be added later, if we decide so.
> Thank you for your interest in making this series better.
Thank you for your work on the patches!
^ permalink raw reply
* Re: [Bug?] "git diff --no-rename A B"
From: René Scharfe @ 2024-01-20 14:39 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <20240120011800.GF117170@coredump.intra.peff.net>
Am 20.01.24 um 02:18 schrieb Jeff King:
> On Wed, Jan 17, 2024 at 05:07:16PM -0800, Junio C Hamano wrote:
>
>> When the user misspells "--no-renames" as "--no-rename", it seems
>> that the rename detection (which is ont by default these days) still
>> kicks in, which means the misspelt option is silently ignored.
>> Should we show an error message instead?
>
> I wondered if "--no-foo" complained, and it does. I think this is a
> subtle corner case in parse-options.
>
> The issue is that we have "--rename-empty", which of course also
> provides "--no-rename-empty". And parse-options is happy to let you
> abbreviate names as long as they are unambiguous. But "--no-rename" _is_
> ambiguous with "--no-renames". Why don't we catch it?
Because diff_opt_parse() passes PARSE_OPT_KEEP_UNKNOWN_OPT, which makes
parse_long_opt() skip abbreviation detection. Which it does since
baa4adc66a (parse-options: disable option abbreviation with
PARSE_OPT_KEEP_UNKNOWN, 2019-01-27).
It added a condition only to the if. Its body can be reached via goto
from two other places, though. So abbreviated options are effectively
allowed through the back door.
--- >8 ---
Subject: [PATCH] parse-options: fully disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN
baa4adc66a (parse-options: disable option abbreviation with
PARSE_OPT_KEEP_UNKNOWN, 2019-01-27) turned off support for abbreviated
options when the flag PARSE_OPT_KEEP_UNKNOWN is given, as any shortened
option could also be an abbreviation for one of the unknown options.
The code for handling abbreviated options is guarded by an if, but it
can also be reached via goto. baa4adc66a only blocked the first way.
Add the condition to the other ones as well.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Formatted with --function-context for easier review.
The code is a quite tangled, any ideas how to structure it better?
parse-options.c | 8 +++++---
t/t4013-diff-various.sh | 6 ++++++
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 4ce2b7ca16..92e96ca6cd 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -353,93 +353,95 @@ static int is_alias(struct parse_opt_ctx_t *ctx,
static enum parse_opt_result parse_long_opt(
struct parse_opt_ctx_t *p, const char *arg,
const struct option *options)
{
const char *arg_end = strchrnul(arg, '=');
const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
+ int allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);
for (; options->type != OPTION_END; options++) {
const char *rest, *long_name = options->long_name;
enum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;
if (options->type == OPTION_SUBCOMMAND)
continue;
if (!long_name)
continue;
again:
if (!skip_prefix(arg, long_name, &rest))
rest = NULL;
if (!rest) {
/* abbreviated? */
- if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT) &&
+ if (allow_abbrev &&
!strncmp(long_name, arg, arg_end - arg)) {
is_abbreviated:
if (abbrev_option &&
!is_alias(p, abbrev_option, options)) {
/*
* If this is abbreviated, it is
* ambiguous. So when there is no
* exact match later, we need to
* error out.
*/
ambiguous_option = abbrev_option;
ambiguous_flags = abbrev_flags;
}
if (!(flags & OPT_UNSET) && *arg_end)
p->opt = arg_end + 1;
abbrev_option = options;
abbrev_flags = flags ^ opt_flags;
continue;
}
/* negation allowed? */
if (options->flags & PARSE_OPT_NONEG)
continue;
/* negated and abbreviated very much? */
- if (starts_with("no-", arg)) {
+ if (allow_abbrev && starts_with("no-", arg)) {
flags |= OPT_UNSET;
goto is_abbreviated;
}
/* negated? */
if (!starts_with(arg, "no-")) {
if (skip_prefix(long_name, "no-", &long_name)) {
opt_flags |= OPT_UNSET;
goto again;
}
continue;
}
flags |= OPT_UNSET;
if (!skip_prefix(arg + 3, long_name, &rest)) {
/* abbreviated and negated? */
- if (starts_with(long_name, arg + 3))
+ if (allow_abbrev &&
+ starts_with(long_name, arg + 3))
goto is_abbreviated;
else
continue;
}
}
if (*rest) {
if (*rest != '=')
continue;
p->opt = rest + 1;
}
return get_value(p, options, flags ^ opt_flags);
}
if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
die("disallowed abbreviated or ambiguous option '%.*s'",
(int)(arg_end - arg), arg);
if (ambiguous_option) {
error(_("ambiguous option: %s "
"(could be --%s%s or --%s%s)"),
arg,
(ambiguous_flags & OPT_UNSET) ? "no-" : "",
ambiguous_option->long_name,
(abbrev_flags & OPT_UNSET) ? "no-" : "",
abbrev_option->long_name);
return PARSE_OPT_HELP;
}
if (abbrev_option)
return get_value(p, abbrev_option, abbrev_flags);
return PARSE_OPT_UNKNOWN;
}
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index cb094241ec..1e3b2dbea4 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -663,4 +663,10 @@ test_expect_success 'diff --default-prefix overrides diff.mnemonicprefix' '
check_prefix actual a/file0 b/file0
'
+test_expect_success 'diff --no-renames cannot be abbreviated' '
+ test_expect_code 129 git diff --no-rename >actual 2>error &&
+ test_must_be_empty actual &&
+ grep "invalid option: --no-rename" error
+'
+
test_done
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v3] tests: move t0009-prio-queue.sh to the new unit testing framework
From: Phillip Wood @ 2024-01-20 14:23 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget, git
Cc: Chandra Pratap, Chandra Pratap, Junio C Hamano, Jeff King
In-Reply-To: <pull.1642.v3.git.1705502304219.gitgitgadget@gmail.com>
On 17/01/2024 14:38, Chandra Pratap via GitGitGadget wrote:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> t/t0009-prio-queue.sh along with t/helper/test-prio-queue.c unit
> tests Git's implementation of a priority queue. Migrate the
> test over to the new unit testing framework to simplify debugging
> and reduce test run-time. Refactor the required logic and add
> a new test case in addition to porting over the original ones in
> shell.
Thanks for working on this, it is good to see people exploring the unit
test framework. I agree with the points that Junio and Peff have already
made, I've got a couple of additional comments below.
> [...]
> +static int test_prio_queue(int *input, int *result)
This function does not need to return a value.
> +{
> + struct prio_queue pq = { intcmp };
> + int i = 0;
> +
> + while (*input) {
I agree with Junio that a for() loop would be better here, but I think
that rather than testing for '0' we should just pass the length of the
input array to this function.
> + int *val = input++;
> + void *peek, *get;
> + switch(*val) {
> + case GET:
> + peek = prio_queue_peek(&pq);
> + get = prio_queue_get(&pq);
> + if (peek != get)
> + BUG("peek and get results don't match");
If possible avoid calling BUG() in unit test code as it aborts the whole
test program, not just the current test. You can use check() and return
early instead
if (!check(peek, !=, get))
return;
> + result[i++] = show(get);
I agree with Peff that it would be safer just to use check_int() here.
If you switch to a for loop with an index you can also print the current
index with
test_msg("index %d", i);
Which will help diagnose which item in the queue is failing.
Alternatively we could grow result dynamically using ALLOC_GROW() and
add a function to compare two arrays of integers. Such a function should
take the two lengths, one for each array.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v3] merge-ll: expose revision names to custom drivers
From: Phillip Wood @ 2024-01-20 14:13 UTC (permalink / raw)
To: Antonin Delpeuch via GitGitGadget, git; +Cc: Junio C Hamano, Antonin Delpeuch
In-Reply-To: <pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com>
Hi Antonin
On 18/01/2024 22:09, Antonin Delpeuch via GitGitGadget wrote:
> From: Antonin Delpeuch <antonin@delpeuch.eu>
>
> Custom merge drivers need access to the names of the revisions they
> are working on, so that the merge conflict markers they introduce
> can refer to those revisions. The placeholders '%S', '%X' and '%Y'
> are introduced to this end.
Thanks for working on this, I think it is a useful improvement. I guess
'%X' and '%Y' are no worse than the existing '%A' and '%B' but I do
wonder if we want to take the opportunity to switch to more descriptive
names for the various parameters passed to the custom merge strategy. We
do do this by supporting %(label:ours) modeled after the format
specifiers used by other commands such as "git log" and "git for-each-ref".
> [...]
> +will be stored via placeholder `%P`. Additionally, the names of the
> +common ancestor revision (`%S`), of the current revision (`%X`) and
> +of the other branch (`%Y`) can also be supplied. Those are short > +revision names, optionally joined with the paths of the file in each
> +revision. Those paths are only present if they differ and are separated
> +from the revision by a colon.
It might be simpler to just call these the "conflict marker labels"
without tying ourselves to a particular format. Something like
The conflict labels to be used for the common ancestor, local head
and other head can be passed by using '%(label:base)',
'%(label:ours)' and '%(label:theirs) respectively.
> @@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
Not part of this patch but I noticed that we're passing the filenames
for '%A' etc. unquoted which is a bit scary.
> strbuf_addf(&cmd, "%d", marker_size);
> else if (skip_prefix(format, "P", &format))
> sq_quote_buf(&cmd, path);
> + else if (skip_prefix(format, "S", &format))
> + sq_quote_buf(&cmd, orig_name);
I think you can avoid the SIGSEV problem you mentioned in your other
email by changing this to
sq_quote_buf(&cmd, orig_name ? orig_name, "");
That would make sure the labels we pass match the ones used by the
internal merge.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 3/3] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-20 11:31 UTC (permalink / raw)
To: Dragan Simic, Junio C Hamano; +Cc: Jeff King, Git List
In-Reply-To: <dd82bef87fd82f7c4bf7cbff5f873141@manjaro.org>
On 18-ene-2024 21:50:23, Dragan Simic wrote:
> On 2024-01-18 21:19, Junio C Hamano wrote:
> > Dragan Simic <dsimic@manjaro.org> writes:
> > > On 2024-01-18 19:26, Junio C Hamano wrote:
> > > > Dragan Simic <dsimic@manjaro.org> writes:
> > > >
> > > > > Perhaps having both options available could be a good thing.
> > > > > Though,
> > > > > adding quite a few knobs may end up confusing the users, so
> > > > > we should
> > > > > make sure to document it well.
> > > >
> > > > I agree there is a need for documentation, but we are not increasing
> > > > the number of knobs, are we?
> > >
> > > Perhaps there would be one more knob if we end up deciding to
> > > implement
> > > support for both approaches, as previously discussed.
> >
> > But that would be just one knob (which I personally do not quite see
> > the need for), not "quite a few knobs" as you said, no?
>
> I'm sorry for being imprecise.
Hi Dragan
My original motivation for starting this series has been fulfilled:
Give the user an option to tell us not to include the disabling
instructions alongside the advice.
The current consensus is: if the user explicitly enables visibility for
an advice, we can stop including the instructions on how to disable its
visibility.
As reference, in this thread two "global" options have been reviewed:
- To take the disabling instructions as an advice in itself and as
such, as with the rest, we can have a knob to disable it. Affecting
all advice messages.
- A new knob to allow the user to set the default visibility for those
advice messages without an explicit visibility set. And so we can
take on globally what we now take on locally; if there is not an
explicit visibility value but there is an explicit "default" value,
we can stop showing the instruction on how to disable it.
> Regarding the need for that additional
> "global" knob, I think it can't hurt. Some people might even find it
> quite useful
I don't quite understand what "global" knob you are referring to. But I
share with you the feeling that a "global" knob might be useful.
The current iteration for this series looks like it will be merged to
'next' soon. In my opinion, it is a good stop for this series, and
maybe we can explore that 'global' knob in a new one.
Thank you for your interest in making this series better.
^ permalink raw reply
* Re: [PATCH] MyFirstContribution: update mailing list sub steps
From: Kristoffer Haugsbakk @ 2024-01-20 10:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Soref, git, spectral, Konstantin Ryabitsev
In-Reply-To: <xmqqmst1hsd6.fsf@gitster.g>
On Fri, Jan 19, 2024, at 23:26, Junio C Hamano wrote:
> Noticed-by: Kyle Lippincott <spectral@google.com>
> Helped-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Just curious. What’s the difference between `Reported-by` and
`Noticed-by`? `Reported-by` is documented in `SubmittingPatches` but not
the other one. Is perhaps `Reported-by` specifically meant for bug
reports?
Thanks
--
Kristoffer Haugsbakk
^ permalink raw reply
* How OBJ_REF_DELTA pack file size calculated
From: Farhan Khan @ 2024-01-20 4:28 UTC (permalink / raw)
To: git
Hi,
I am trying to implement REF_OBJ_DELTA, but having some trouble with offsets (sizes in the pack file) that do not align, specifically the size of the whole object does not seem to add up.
For example, consider this run:
git verify-pack -v .git/objects/pack/pack-48269bdfe1d28d20f603c6b23eed5717b7474e76.pack | grep 82daab01f43e34b9f7c8e0db81a9951933b04f1b
82daab01f43e34b9f7c8e0db81a9951933b04f1b commit 94 101 82749 1 ecd0e8c88ed8891da372f5630d542150b0a0531e
The size of the object is 94 bytes
The size of the entry is 101 bytes.
My patching/reconstruction of the object works, the compressed size is 97 bytes. However, I cannot figure out where the 101 comes from. The size of the object header is 2 bytes, the OBJ_REF_DELTA is 20 bytes (the SHA1), but that does not add up to 101 bytes.
I am trying to understand where the 101 bytes comes from.
If not, can you please point me to where in the code the offset size for OBJ_REF_DELTA is calculated. I tried myself from buildin/verify-pack.c, but there seems to be some multi-threating/processing going on and I was not able to determine where the calculation happens.
Thanks!
--
Farhan Khan
PGP Fingerprint: 1312 89CE 663E 1EB2 179C 1C83 C41D 2281 F8DA C0DE
^ permalink raw reply
* Re: [PATCH v3] tests: move t0009-prio-queue.sh to the new unit testing framework
From: Jeff King @ 2024-01-20 2:31 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1642.v3.git.1705502304219.gitgitgadget@gmail.com>
On Wed, Jan 17, 2024 at 02:38:23PM +0000, Chandra Pratap via GitGitGadget wrote:
> +#define TEST_INPUT(INPUT, EXPECTED, name) \
> + static void test_##name(void) \
> +{ \
> + int input[] = {INPUT}; \
> + int expected[] = {EXPECTED}; \
> + int result[ARRAY_SIZE(expected)]; \
> + test_prio_queue(input, result); \
> + check(!memcmp(expected, result, sizeof(expected))); \
> +}
It is somewhat unfortunate that a failing test here gives a fairly
uninformative message like:
# check "!memcmp(expected, result, sizeof(expected))" failed at t/unit-tests/t-prio-queue.c:89
not ok 5 - prio-queue works when LIFO stack is reversed
whereas the original t0009 you get a nice diff between expected and
actual output. I guess this is a place where the test framework could
help us more with a specialized check_memequal() or something that
showed the differing bytes on failure (of course being C, it has no type
info so it wouldn't even know these are ints!).
Another solution would be to have the test function check the result as
it is computed rather than storing it in an array. That would also solve
another potential problem: undefined behavior if the result is not the
expected size. If there is a bug that causes too much output we'd
overflow our buffer. If too few, we'll end up comparing uninitialized
memory (which could even accidentally have the correct values,
especially if it's recycled from a previous test!). Neither of those
should happen in practice, but then the whole point of this exercise is
to test the code.
I admit I find myself a little skeptical in general of this whole "let's
do tests in C" framework.
-Peff
^ permalink raw reply
* [Outreachy][PATCH v4] t2400: avoid losing exit status to pipes
From: Achu Luma @ 2024-01-20 2:15 UTC (permalink / raw)
To: git; +Cc: christian.couder, gitster, Achu Luma
In-Reply-To: <20231204153740.2992-1-ach.lumap@gmail.com>
The exit code of the preceding command in a pipe is disregarded. So
if that preceding command is a Git command that fails, the test would
not fail. Instead, by saving the output of that Git command to a file,
and removing the pipe, we make sure the test will fail if that Git
command fails.
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
The difference between v3 and v4 is:
- Changed subject to better reflect what the patch is doing.
- Updated the commit message.
t/t2400-worktree-add.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 3742971105..b597004adb 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -490,7 +490,8 @@ test_expect_success 'put a worktree under rebase' '
cd under-rebase &&
set_fake_editor &&
FAKE_LINES="edit 1" git rebase -i HEAD^ &&
- git worktree list | grep "under-rebase.*detached HEAD"
+ git worktree list >actual &&
+ grep "under-rebase.*detached HEAD" actual
)
'
@@ -531,7 +532,8 @@ test_expect_success 'checkout a branch under bisect' '
git bisect start &&
git bisect bad &&
git bisect good HEAD~2 &&
- git worktree list | grep "under-bisect.*detached HEAD" &&
+ git worktree list >actual &&
+ grep "under-bisect.*detached HEAD" actual &&
test_must_fail git worktree add new-bisect under-bisect &&
! test -d new-bisect
)
--
2.43.0.windows.1
^ permalink raw reply related
* Re: What's cooking in git.git (Jan 2024, #06; Fri, 19)
From: Ghanshyam Thakkar @ 2024-01-20 2:10 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqbk9ghio5.fsf@gitster.g>
On Sat Jan 20, 2024 at 7:26 AM IST, Junio C Hamano wrote:
> * gt/t0024-style-fixes (2024-01-18) 2 commits
> - t0024: style fix
> - t0024: avoid losing exit status to pipes
>
> source: <20240118215407.8609-1-shyamthakkar001@gmail.com>
I have rerolled this series, cause it was breaking CI on alpine due to
a syntax issue which is described here:
https://lore.kernel.org/git/CYIDCZPQN2H1.1ET0CTP07NMYR@gmail.com/
Thanks.
^ permalink raw reply
* What's cooking in git.git (Jan 2024, #06; Fri, 19)
From: Junio C Hamano @ 2024-01-20 1:56 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a
future release). Commits prefixed with '-' are only in 'seen', and
aren't considered "accepted" at all and may be annotated with an URL
to a message that raises issues but they are no means exhaustive. A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[Graduated to 'master']
* bk/bisect-doc-fix (2024-01-10) 2 commits
(merged to 'next' on 2024-01-12 at bdb3609554)
+ doc: refer to pathspec instead of path
+ doc: use singular form of repeatable path arg
Synopsis fix.
source: <20240103040207.661413-1-britton.kerin@gmail.com>
* cp/t4129-pipefix (2024-01-10) 1 commit
(merged to 'next' on 2024-01-12 at fd9b72b71a)
+ t4129: prevent loss of exit code due to the use of pipes
Test update.
source: <pull.1636.git.1704891257544.gitgitgadget@gmail.com>
* ps/gitlab-ci-static-analysis (2024-01-08) 1 commit
(merged to 'next' on 2024-01-10 at 71af34de07)
+ ci: add job performing static analysis on GitLab CI
(this branch is used by ps/gitlab-ci-macos.)
GitLab CI update.
source: <1536a5ef07ad24dafb5d685b40099882f89e6cc5.1703761005.git.ps@pks.im>
* ps/p4-use-ref-api (2024-01-11) 1 commit
(merged to 'next' on 2024-01-12 at 3f89cf25f6)
+ git-p4: stop reaching into the refdb
"git p4" update to prepare for reftable
source: <33d6a062ec56be33ed50a42a420be0b023f6f4cf.1704980814.git.ps@pks.im>
* ps/prompt-parse-HEAD-futureproof (2024-01-08) 2 commits
(merged to 'next' on 2024-01-10 at f9515b9d89)
+ git-prompt: stop manually parsing HEAD with unknown ref formats
+ Merge branch 'ps/refstorage-extension' into ps/prompt-parse-HEAD-futureproof
Futureproof command line prompt support (in contrib/).
source: <ef4e36a5a40c369da138242a8fdc9e12a846613b.1704356313.git.ps@pks.im>
* rj/clarify-branch-doc-m (2024-01-08) 1 commit
(merged to 'next' on 2024-01-10 at 432eaa2c6b)
+ branch: clarify <oldbranch> term
Doc update.
source: <d38e5a18-4d85-48f3-bc8b-8ca02ea683a4@gmail.com>
* sk/mingw-owner-check-error-message-improvement (2024-01-10) 1 commit
(merged to 'next' on 2024-01-12 at 05c56e151b)
+ mingw: give more details about unsafe directory's ownership
In addition to (rather cryptic) Security Identifiers, show username
and domain in the error message when we barf on mismatch between
the Git directory and the current user on Windows.
source: <20240108173837.20480-2-soekkle@freenet.de>
* tb/fetch-all-configuration (2024-01-08) 1 commit
(merged to 'next' on 2024-01-12 at 6a05050382)
+ fetch: add new config option fetch.all
"git fetch" learned to pay attention to "fetch.all" configuration
variable, which pretends as if "--all" was passed from the command
line when no remote parameter was given.
source: <20240108211832.47362-1-dev@tb6.eu>
--------------------------------------------------
[New Topics]
* kh/maintenance-use-xdg-when-it-should (2024-01-18) 4 commits
(merged to 'next' on 2024-01-19 at 9c8c7b2e9d)
+ maintenance: use XDG config if it exists
+ config: factor out global config file retrieval
+ config: rename global config function
+ config: format newlines
When $HOME/.gitignore is missing but XDG config file available, we
should write into the latter, not former. "git gc" and "git
maintenance" wrote into a wrong "global config" file, which have
been corrected.
Will merge to 'master'.
source: <cover.1705593810.git.code@khaugsbakk.name>
* ps/gitlab-ci-macos (2024-01-18) 6 commits
(merged to 'next' on 2024-01-19 at a239dc8140)
+ ci: add macOS jobs to GitLab CI
+ ci: make p4 setup on macOS more robust
+ ci: handle TEST_OUTPUT_DIRECTORY when printing test failures
+ Makefile: detect new Homebrew location for ARM-based Macs
+ t7527: decrease likelihood of racing with fsmonitor daemon
+ Merge branch 'ps/gitlab-ci-static-analysis' into ps/gitlab-ci-macos
CI for GitLab learned to drive macOS jobs.
Will merge to 'master'.
source: <cover.1705573336.git.ps@pks.im>
* ad/custom-merge-placeholder-for-symbolic-pathnames (2024-01-18) 1 commit
- merge-ll: expose revision names to custom drivers
source: <pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com>
* cp/unit-test-prio-queue (2024-01-17) 2 commits
- SQUASH???
- tests: move t0009-prio-queue.sh to the new unit testing framework
source: <pull.1642.v3.git.1705502304219.gitgitgadget@gmail.com>
* jc/reffiles-tests (2024-01-19) 12 commits
- t5312: move reffiles specific tests to t0601
- t4202: move reffiles specific tests to t0600
- t3903: make drop stash test ref backend agnostic
- t1503: move reffiles specific tests to t0600
- t1415: move reffiles specific tests to t0601
- t1410: move reffiles specific tests to t0600
- t1406: move reffiles specific tests to t0600
- t1405: move reffiles specific tests to t0601
- t1404: move reffiles specific tests to t0600
- t1414: convert test to use Git commands instead of writing refs manually
- remove REFFILES prerequisite for some tests in t1405 and t2017
- t3210: move to t0601
source: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>
* ml/log-merge-with-cherry-pick-and-other-pseudo-heads (2024-01-17) 2 commits
- revision: implement `git log --merge` also for rebase/cherry_pick/revert
- revision: ensure MERGE_HEAD is a ref in prepare_show_merge
source: <xmqqzfxa9usx.fsf@gitster.g>
* nb/rebase-x-shell-docfix (2024-01-17) 1 commit
- rebase: fix documentation about used shell in -x
source: <20240117085347.948960-1-nik.borisov@suse.com>
* tc/show-ref-exists-fix (2024-01-18) 1 commit
- builtin/show-ref: treat directory as non-existing in --exists
source: <20240110141559.387815-2-toon@iotcl.com>
* gt/t0024-style-fixes (2024-01-18) 2 commits
- t0024: style fix
- t0024: avoid losing exit status to pipes
source: <20240118215407.8609-1-shyamthakkar001@gmail.com>
* jc/majordomo-to-subspace (2024-01-19) 1 commit
- Docs: majordomo@vger.kernel.org has been decomissioned
source: <xmqqmst1hsd6.fsf@gitster.g>
* js/oss-fuzz-build-in-ci (2024-01-19) 2 commits
- ci: build and run minimal fuzzers in GitHub CI
- fuzz: fix fuzz test build rules
source: <cover.1705700054.git.steadmon@google.com>
* kn/for-all-refs (2024-01-19) 5 commits
- for-each-ref: avoid filtering on empty pattern
- refs: introduce `refs_for_each_all_refs()`
- refs: extract out `loose_fill_ref_dir_regular_file()`
- refs: make `is_pseudoref_syntax()` stricter
- refs: expose `is_pseudoref_syntax()`
source: <20240119142705.139374-1-karthik.188@gmail.com>
* ps/not-so-many-refs-are-special (2024-01-19) 7 commits
- Documentation: add "special refs" to the glossary
- refs: redefine special refs
- refs: convert MERGE_AUTOSTASH to become a normal pseudo-ref
- sequencer: introduce functions to handle autostashes via refs
- refs: convert AUTO_MERGE to become a normal pseudo-ref
- sequencer: delete REBASE_HEAD in correct repo when picking commits
- sequencer: clean up pseudo refs with REF_NO_DEREF
source: <cover.1705659748.git.ps@pks.im>
--------------------------------------------------
[Cooking]
* es/some-up-to-date-messages-must-stay (2024-01-12) 1 commit
(merged to 'next' on 2024-01-16 at 2b598f7de2)
+ messages: mark some strings with "up-to-date" not to touch
Comment updates to help developers not to attempt to modify
messages from plumbing commands that must stay constant.
It might make sense to reassess the plumbing needs every few years,
but that should be done as a separate effort.
Will merge to 'master'.
source: <20240112171910.11131-1-ericsunshine@charter.net>
* bk/complete-bisect (2024-01-18) 5 commits
- completion: git-bisect view recognized but not completed
- completion: custom git-bisect terms
- completion: move to maintain define-before-use
- completion: git-log opts to bisect visualize
- completion: complete new old actions, start opts
Command line completion support (in contrib/) has been
updated for "git bisect".
Expecting a reroll.
cf. <ZaofJhHsFjRxx7a3@tanuki>
source: <20240118204323.1113859-1-britton.kerin@gmail.com>
* bk/complete-dirname-for-am-and-format-patch (2024-01-12) 1 commit
- completion: dir-type optargs for am, format-patch
Command line completion support (in contrib/) has been
updated for a few commands to complete directory names where a
directory name is expected.
Needs review.
source: <d37781c3-6af2-409b-95a8-660a9b92d20b@smtp-relay.sendinblue.com>
* bk/complete-send-email (2024-01-12) 1 commit
- completion: don't complete revs when --no-format-patch
Command line completion support (in contrib/) has been taught to
avoid offering revision names as candidates to "git send-email" when
the command is used to send pre-generated files.
Needs review.
source: <a718b5ee-afb0-44bd-a299-3208fac43506@smtp-relay.sendinblue.com>
* gt/test-commit-o-i-options (2024-01-17) 2 commits
(merged to 'next' on 2024-01-19 at 0377e2c148)
+ t7501: add tests for --amend --signoff
+ t7501: add tests for --include and --only
A few tests to "git commit -o <pathspec>" and "git commit -i
<pathspec>" has been added.
Will merge to 'master'.
source: <20240117161421.17333-1-shyamthakkar001@gmail.com>
* jt/tests-with-reftable (2024-01-12) 2 commits
(merged to 'next' on 2024-01-19 at 498d203a57)
+ t5541: remove lockfile creation
+ t1401: remove lockfile creation
Tweak a few tests not to manually modify the reference database
(hence easier to work with other backends like reftable).
Will merge to 'master'.
source: <pull.1634.v2.git.1705004670.gitgitgadget@gmail.com>
* la/strvec-comment-fix (2024-01-12) 1 commit
(merged to 'next' on 2024-01-19 at 120ef16493)
+ strvec: use correct member name in comments
Comment fix.
Will merge to 'master'.
source: <pull.1640.git.1705043195997.gitgitgadget@gmail.com>
* la/trailer-api (2024-01-12) 10 commits
- trailer: delete obsolete argument handling code from API
- trailer: move arg handling to interpret-trailers.c
- trailer: prepare to move parse_trailers_from_command_line_args() to builtin
- trailer: spread usage of "trailer_block" language
- trailer: make trailer_info struct private
- sequencer: use the trailer iterator
- trailer: delete obsolete formatting functions
- trailer: unify trailer formatting machinery
- trailer: include "trailer" term in API functions
- trailer: move process_trailers() to interpret-trailers.c
Code clean-up.
Needs review.
source: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
* ps/tests-with-ref-files-backend (2024-01-12) 6 commits
- t: mark tests regarding git-pack-refs(1) to be backend specific
- t5526: break test submodule differently
- t1419: mark test suite as files-backend specific
- t1302: make tests more robust with new extensions
- t1301: mark test for `core.sharedRepository` as reffiles specific
- t1300: make tests more robust with non-default ref backends
Prepare existing tests on refs to work better with non-default
backends.
Needs review.
source: <cover.1704877233.git.ps@pks.im>
* ne/doc-filter-blob-limit-fix (2024-01-16) 1 commit
(merged to 'next' on 2024-01-19 at 4f78975728)
+ rev-list-options: fix off-by-one in '--filter=blob:limit=<n>' explainer
Docfix.
Will merge to 'master'.
source: <pull.1645.git.git.1705261850650.gitgitgadget@gmail.com>
* ps/commit-graph-write-leakfix (2024-01-15) 1 commit
(merged to 'next' on 2024-01-19 at df537fac39)
+ commit-graph: fix memory leak when not writing graph
Leakfix.
Will merge to 'master'.
source: <0feab5e7d5bc6275e2c7671cd8f6786ea86fd610.1702891190.git.ps@pks.im>
* rj/advice-delete-branch-not-fully-merged (2024-01-11) 3 commits
(merged to 'next' on 2024-01-19 at 7102eb6b79)
+ branch: make the advice to force-deleting a conditional one
+ advice: fix an unexpected leading space
+ advice: sort the advice related lists
(this branch is used by rj/advice-disable-how-to-disable.)
The error message given when "git branch -d branch" fails due to
commits unique to the branch has been split into an error and a new
conditional advice message.
Will merge to 'master'.
source: <4aedc15c-4b3f-4f5e-abea-581b501600f8@gmail.com>
* en/diffcore-delta-final-line-fix (2024-01-18) 1 commit
- diffcore-delta: avoid ignoring final 'line' of file
Rename detection logic ignored the final line of a file if it is an
incomplete line.
Will merge to 'next'.
source: <pull.1637.v2.git.1705119973690.gitgitgadget@gmail.com>
* mj/gitweb-unreadable-config-error (2024-01-10) 1 commit
(merged to 'next' on 2024-01-19 at 790b7a7855)
+ gitweb: die when a configuration file cannot be read
When given an existing but unreadable file as a configuration file,
gitweb behaved as if the file did not exist at all, but now it
errors out. This is a change that may break backward compatibility.
Will merge to 'master'.
source: <20240110225709.30168-1-marcelo.jimenez@gmail.com>
* ps/completion-with-reftable-fix (2024-01-16) 5 commits
(merged to 'next' on 2024-01-19 at 8854a7d903)
+ completion: treat dangling symrefs as existing pseudorefs
+ completion: silence pseudoref existence check
+ completion: improve existence check for pseudo-refs
+ t9902: verify that completion does not print anything
+ completion: discover repo path in `__git_pseudoref_exists ()`
Completion update to prepare for reftable
Will merge to 'master'.
source: <cover.1705314554.git.ps@pks.im>
* ps/reftable-optimize-io (2024-01-18) 7 commits
- reftable/stack: fix race in up-to-date check
- reftable/stack: unconditionally reload stack after commit
(merged to 'next' on 2024-01-12 at 4096e880e0)
+ reftable/blocksource: use mmap to read tables
+ reftable/blocksource: refactor code to match our coding style
+ reftable/stack: use stat info to avoid re-reading stack list
+ reftable/stack: refactor reloading to use file descriptor
+ reftable/stack: refactor stack reloading to have common exit path
Low-level I/O optimization for reftable.
The two commits at the tip are new.
Will merge to 'next' and then to 'master'?
source: <cover.1704966670.git.ps@pks.im>
source: <cover.1705585037.git.ps@pks.im>
* rj/advice-disable-how-to-disable (2024-01-16) 2 commits
- advice: allow disabling the automatic hint in advise_if_enabled()
- Merge branch 'rj/advice-delete-branch-not-fully-merged' into rj/advice-disable-how-to-disable
(this branch uses rj/advice-delete-branch-not-fully-merged.)
All conditional "advice" messages show how to turn them off, which
becomes repetitive. Add a configuration variable to omit the
instruction.
Will merge to 'next'?
source: <6a842ef8-b390-4739-9eef-e867d55ed5ea@gmail.com>
* vd/fsck-submodule-url-test (2024-01-19) 4 commits
(merged to 'next' on 2024-01-19 at dad35ae82c)
+ submodule-config.c: strengthen URL fsck check
+ t7450: test submodule urls
+ test-submodule: remove command line handling for check-name
+ submodule-config.h: move check_submodule_url
Tighten URL checks fsck makes in a URL recorded for submodules.
Will merge to 'master'.
source: <pull.1635.v2.git.1705542918.gitgitgadget@gmail.com>
* sd/negotiate-trace-fix (2024-01-03) 1 commit
- push: region_leave trace for negotiate_using_fetch
Tracing fix.
Waiting for a review response.
cf. <xmqqbka27zu9.fsf@gitster.g>
source: <20240103224054.1940209-1-delmerico@google.com>
* ps/worktree-refdb-initialization (2024-01-08) 7 commits
(merged to 'next' on 2024-01-19 at e8c649a3ac)
+ builtin/worktree: create refdb via ref backend
+ worktree: expose interface to look up worktree by name
+ builtin/worktree: move setup of commondir file earlier
+ refs/files: skip creation of "refs/{heads,tags}" for worktrees
+ setup: move creation of "refs/" into the files backend
+ refs: prepare `refs_init_db()` for initializing worktree refs
+ Merge branch 'ps/refstorage-extension' into ps/worktree-refdb-initialization
Instead of manually creating refs/ hierarchy on disk upon a
creation of a secondary worktree, which is only usable via the
files backend, use the refs API to populate it.
Will merge to 'master'.
source: <cover.1704705733.git.ps@pks.im>
* cp/apply-core-filemode (2023-12-26) 3 commits
- apply: code simplification
- apply: correctly reverse patch's pre- and post-image mode bits
- apply: ignore working tree filemode when !core.filemode
"git apply" on a filesystem without filemode support have learned
to take a hint from what is in the index for the path, even when
not working with the "--index" or "--cached" option, when checking
the executable bit match what is required by the preimage in the
patch.
Needs review.
source: <20231226233218.472054-1-gitster@pobox.com>
* al/unit-test-ctype (2024-01-16) 1 commit
(merged to 'next' on 2024-01-19 at fcdad0d06c)
+ unit-tests: rewrite t/helper/test-ctype.c as a unit test
Move test-ctype helper to the unit-test framework.
Will merge to 'master'.
source: <20240112102743.1440-1-ach.lumap@gmail.com>
* ja/doc-placeholders-fix (2023-12-26) 2 commits
- doc: enforce placeholders in documentation
- doc: enforce dashes in placeholders
Docfix.
Needs review.
source: <pull.1626.git.1703539287.gitgitgadget@gmail.com>
* jc/bisect-doc (2023-12-09) 1 commit
- bisect: document "terms" subcommand more fully
Doc update.
Needs review.
source: <xmqqzfyjmk02.fsf@gitster.g>
* tb/pair-chunk-expect (2023-11-10) 8 commits
- midx: read `OOFF` chunk with `pair_chunk_expect()`
- midx: read `OIDL` chunk with `pair_chunk_expect()`
- commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
- commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
- commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
- commit-graph: read `OIDL` chunk with `pair_chunk_expect()`
- chunk-format: introduce `pair_chunk_expect()` helper
- Merge branch 'jk/chunk-bounds-more' into HEAD
Further code clean-up.
Needs review.
source: <cover.1699569246.git.me@ttaylorr.com>
* tb/path-filter-fix (2024-01-16) 17 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: drop unnecessary `graph_read_bloom_data_context`
- commit-graph.c: unconditionally load Bloom filters
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- commit-graph: new Bloom filter version that fixes murmur3
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- t/helper/test-read-graph: implement `bloom-filters` mode
- bloom.h: make `load_bloom_filter_from_graph()` public
- t/helper/test-read-graph.c: extract `dump_graph_info()`
- gitformat-commit-graph: describe version 2 of BDAT
- commit-graph: ensure Bloom filters are read with consistent settings
- revision.c: consult Bloom filters for root commits
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
The Bloom filter used for path limited history traversal was broken
on systems whose "char" is unsigned; update the implementation and
bump the format version to 2.
Will merge to 'next'?
source: <cover.1705442923.git.me@ttaylorr.com>
* ak/color-decorate-symbols (2023-10-23) 7 commits
- log: add color.decorate.pseudoref config variable
- refs: exempt pseudorefs from pattern prefixing
- refs: add pseudorefs array and iteration functions
- log: add color.decorate.ref config variable
- log: add color.decorate.symbol config variable
- log: use designated inits for decoration_colors
- config: restructure color.decorate documentation
A new config for coloring.
Needs review.
source: <20231023221143.72489-1-andy.koppe@gmail.com>
* eb/hash-transition (2023-10-02) 30 commits
- t1016-compatObjectFormat: add tests to verify the conversion between objects
- t1006: test oid compatibility with cat-file
- t1006: rename sha1 to oid
- test-lib: compute the compatibility hash so tests may use it
- builtin/ls-tree: let the oid determine the output algorithm
- object-file: handle compat objects in check_object_signature
- tree-walk: init_tree_desc take an oid to get the hash algorithm
- builtin/cat-file: let the oid determine the output algorithm
- rev-parse: add an --output-object-format parameter
- repository: implement extensions.compatObjectFormat
- object-file: update object_info_extended to reencode objects
- object-file-convert: convert commits that embed signed tags
- object-file-convert: convert commit objects when writing
- object-file-convert: don't leak when converting tag objects
- object-file-convert: convert tag objects when writing
- object-file-convert: add a function to convert trees between algorithms
- object: factor out parse_mode out of fast-import and tree-walk into in object.h
- cache: add a function to read an OID of a specific algorithm
- tag: sign both hashes
- commit: export add_header_signature to support handling signatures on tags
- commit: convert mergetag before computing the signature of a commit
- commit: write commits for both hashes
- object-file: add a compat_oid_in parameter to write_object_file_flags
- object-file: update the loose object map when writing loose objects
- loose: compatibilty short name support
- loose: add a mapping between SHA-1 and SHA-256 for loose objects
- repository: add a compatibility hash algorithm
- object-names: support input of oids in any supported hash
- oid-array: teach oid-array to handle multiple kinds of oids
- object-file-convert: stubs for converting from one object format to another
Teach a repository to work with both SHA-1 and SHA-256 hash algorithms.
Needs review.
source: <878r8l929e.fsf@gmail.froward.int.ebiederm.org>
* jx/remote-archive-over-smart-http (2024-01-16) 6 commits
- transport-helper: call do_take_over() in process_connect
- transport-helper: call do_take_over() in connect_helper
- http-backend: new rpc-service for git-upload-archive
- transport-helper: protocol-v2 supports upload-archive
- remote-curl: supports git-upload-archive service
- transport-helper: no connection restriction in connect_helper
"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.
Will merge to 'next'?
source: <cover.1705411391.git.zhiyou.jx@alibaba-inc.com>
* jc/rerere-cleanup (2023-08-25) 4 commits
- rerere: modernize use of empty strbuf
- rerere: try_merge() should use LL_MERGE_ERROR when it means an error
- rerere: fix comment on handle_file() helper
- rerere: simplify check_one_conflict() helper function
Code clean-up.
Not ready to be reviewed yet.
source: <20230824205456.1231371-1-gitster@pobox.com>
^ permalink raw reply
* Re: partial commit of file-to-directory change, was Re: Bugreport
From: Junio C Hamano @ 2024-01-20 1:22 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, Frank Schwidom, git
In-Reply-To: <20240120004628.GA117170@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> It might also be that the bug is earlier in list_paths(), where we
> should notice that "d" is gone and now we have entries under "d/".
We had a related discussion on "commit -i/-o tests" review thread,
where we noticed that "git commit -i foobar", when "foobar" does not
match any path in the index, silently ignore that unmatching
pathspec (but "commit -o foobar" does error out, saying "did not
match any file(s) known to git"). The important design decision we
made long time ago when we introduced partial commit ("commit -o
pathspec") is that we do not _add_ paths (that match the pathspec)
that are not tracked. We require explicit "git add" for them before
you run "git commit".
So, I suspect that list_paths() that gives "d" as a thing to add is
broken. "commit -m + ." at that point is saying "we should get the
latest contents from the working tree for all the paths in the real
index, add[*] them to the temporary index freshly read from HEAD,
and the resulting temporary index should be written out as a tree"
to create a commit, and then "the same set of contents for the paths
then should be added to the real index, so that the differences
between the tree we wrote out of the temporary index to update the
HEAD and the real index would still be the changes already in the
index before this partial commit".
Side note: here, "add" would include removing (if the working
tree file for the path is gone) or killing 'd' while adding
'd/b.txt'.
So I would understand if 'd/b.txt' is listed (because in the real
index there already is d/b.txt known), but if the directory 'd'
itself is listed, that does sound like a bug.
^ permalink raw reply
* Re: [Bug?] "git diff --no-rename A B"
From: Jeff King @ 2024-01-20 1:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34uvtpob.fsf@gitster.g>
On Wed, Jan 17, 2024 at 05:07:16PM -0800, Junio C Hamano wrote:
> When the user misspells "--no-renames" as "--no-rename", it seems
> that the rename detection (which is ont by default these days) still
> kicks in, which means the misspelt option is silently ignored.
> Should we show an error message instead?
I wondered if "--no-foo" complained, and it does. I think this is a
subtle corner case in parse-options.
The issue is that we have "--rename-empty", which of course also
provides "--no-rename-empty". And parse-options is happy to let you
abbreviate names as long as they are unambiguous. But "--no-rename" _is_
ambiguous with "--no-renames". Why don't we catch it?
I'd guess it is because we do not have "--renames" as an option, but
explicitly generate an entry for "--no-renames" (since the non-negative
version is actually "--find-renames"). I know there is some special code
to handle these pre-negated cases, but I would not be surprised if the
ambiguity checker does not.
So I think it's likely just a bug in parse-options which should be
fixed.
We could also work around it by providing --renames. ;) E.g., if we let
the find-renames callback handle negation, then --renames becomes a
synonym, like so:
diff --git a/diff.c b/diff.c
index a89a6a6128..cdec9bfbd9 100644
--- a/diff.c
+++ b/diff.c
@@ -5292,7 +5292,11 @@ static int diff_opt_find_renames(const struct option *opt,
{
struct diff_options *options = opt->value;
- BUG_ON_OPT_NEG(unset);
+ if (unset) {
+ options->detect_rename = 0;
+ return 0;
+ }
+
if (!arg)
arg = "";
options->rename_score = parse_rename_score(&arg);
@@ -5686,7 +5690,7 @@ struct option *add_diff_options(const struct option *opts,
diff_opt_break_rewrites),
OPT_CALLBACK_F('M', "find-renames", options, N_("<n>"),
N_("detect renames"),
- PARSE_OPT_NONEG | PARSE_OPT_OPTARG,
+ PARSE_OPT_OPTARG,
diff_opt_find_renames),
OPT_SET_INT_F('D', "irreversible-delete", &options->irreversible_delete,
N_("omit the preimage for deletes"),
@@ -5697,9 +5701,10 @@ struct option *add_diff_options(const struct option *opts,
diff_opt_find_copies),
OPT_BOOL(0, "find-copies-harder", &options->flags.find_copies_harder,
N_("use unmodified files as source to find copies")),
- OPT_SET_INT_F(0, "no-renames", &options->detect_rename,
- N_("disable rename detection"),
- 0, PARSE_OPT_NONEG),
+ OPT_CALLBACK_F(0, "renames", options, N_("<n>"),
+ N_("synonym for --find-renames"),
+ PARSE_OPT_OPTARG,
+ diff_opt_find_renames),
OPT_BOOL(0, "rename-empty", &options->flags.rename_empty,
N_("use empty blobs as rename source")),
OPT_CALLBACK_F(0, "follow", options, NULL,
And you get the expected output:
$ git show f920b0289b --oneline --raw --no-rename
error: ambiguous option: no-rename (could be --no-renames or --no-rename-empty)
And as a bonus, now "--renames" works. :) It might pollute the output of
"-h" more, but I am not sure if we ever actually show these diff options
via "-h" (they are parsed quite indirectly, and "-h" is handled by the
main command's parse-options list).
Still, it seems like it's worth fixing the parse-options bug.
-Peff
^ permalink raw reply related
* Re: [PATCH 2/2] reftable/stack: fix race in up-to-date check
From: Jeff King @ 2024-01-20 1:05 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <713e51a25c1c4cfa830db97f71cd7c39e85864d4.1705585037.git.ps@pks.im>
On Thu, Jan 18, 2024 at 02:41:56PM +0100, Patrick Steinhardt wrote:
> Refactor the code to stop using `stat_validity_check()`. Instead, we
> manually stat(3P) the file descriptors to make relevant information
> available. On Windows and MSYS2 the result will have both `st_dev` and
> `st_ino` set to 0, which allows us to address the first issue by not
> using the stat-based cache in that case. It also allows us to make sure
> that we always compare `st_dev` and `st_ino`, addressing the second
> issue.
I didn't think too hard about the details, but does this mean that
every user of stat_validity_check() has the same issue? The other big
one is packed-refs (for which the code was originally written). Should
this fix go into that API?
-Peff
^ permalink raw reply
* Re: Suggestion for Git docummntation Book, Chapter "7.7 Git Tools - Reset Demystified"
From: Jeff King @ 2024-01-20 1:03 UTC (permalink / raw)
To: Peter Hunkeler; +Cc: git
In-Reply-To: <54482cec-88de-4e88-a153-f699c2c087d9@gmx.net>
On Thu, Jan 18, 2024 at 07:42:52PM +0100, Peter Hunkeler wrote:
> I appreciate all the work that has been done to give us Git users good
> documentation.
>
> As a rather-newbie I have to lookup the commands I need for rarely used,
> specific tasks. <git reset ...> being one of them. If find chapter "7.7
> Git Tools - Reset Demystified" very helpful. There is however one point
> that newbies might not (yet) clearly understand: The git reset does
> *not* do anything to new, untracked files that might exist in the
> working tree. To cleanup, the advice from various places on the internet
> is to issue a "git clean -f -d" after the reset.
>
> Maybe it would be helpful if said chapter had a short discussion about
> non tracked files, how to clean with "git clean -f -d"
I don't think the folks who work on the book content tend to hang out on
the development mailing list. You might get more response opening an
issue in their GitHub repo:
https://github.com/progit/progit2
-Peff
^ permalink raw reply
* Re: git fsck does not check the packed-refs file
From: Jeff King @ 2024-01-20 1:00 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: R. Diez, git
In-Reply-To: <ZakIPEytlxHGCB9Y@tanuki>
On Thu, Jan 18, 2024 at 12:15:08PM +0100, Patrick Steinhardt wrote:
> > I am guessing that "git fsck" does not check file packed-refs at all.
> > I mean, it does not even attempt to parse it, in order to check
> > whether at least the format makes any sense. Only "git push" does it.
>
> Indeed it doesn't. While the issue is comparatively easy to spot by
> manually inspecting the `packed-refs` file, I agree that it would be
> great if git-fsck(1) knew how to check the refdb for consistency. This
> problem is only going to get worse once the upcoming reftable backend
> lands -- it is a binary format, and just opening it with a text editor
> to check whether it looks sane-ish stops being a viable option here.
We don't check the packed-refs file explicitly, but we do open and parse
it to iterate over the refs it contains. E.g.:
$ git init
$ echo foo >.git/packed-refs
$ git fsck
Checking object directories: 100% (256/256), done.
fatal: unexpected line in .git/packed-refs: foo
It's quite possible that the reading code could be more careful. I'd
have to see the exact corruption that "git fsck" didn't complain about
to say more. If there's a page full of NUL bytes at the end of the
file, I wouldn't be surprised if the reading code gently ignores that,
which obviously is not ideal.
Fundamentally we cannot catch all cases here; a simple truncation, for
example, might yield a valid file that is simply missing some entries.
Unlike objects (which make promises about reachability and so on), there
is no real "consistency" for the state of the refs. But probably warning
if saw a bunch of garbage in the file is a good thing.
I also agree that a specific refdb consistency check would be valuable.
There are some things that the regular reading code will not check, but
which an fsck should (e.g., if the packed-refs file claims to have the
"sorted" trait, we should confirm that).
-Peff
^ permalink raw reply
* Re: partial commit of file-to-directory change, was Re: Bugreport
From: Junio C Hamano @ 2024-01-20 0:55 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, Frank Schwidom, git
In-Reply-To: <20240120004628.GA117170@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I dunno. I probably won't dig much further on this myself, but it's
> possible Junio (cc'd) might have an idea right away.
Sorry, but I do not have any idea "right away". I even needed to
see the tree of 2888605c64 and check if we had submodules back then
(it turns out that we did).
^ permalink raw reply
* Re: [PATCH v2 0/4] Strengthen fsck checks for submodule URLs
From: Jeff King @ 2024-01-20 0:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Victoria Dye via GitGitGadget, git, Patrick Steinhardt,
Victoria Dye
In-Reply-To: <xmqqmst2sdn0.fsf@gitster.g>
On Thu, Jan 18, 2024 at 10:24:51AM -0800, Junio C Hamano wrote:
> > * Patch 1 moves 'check_submodule_url()' to a public location so that it can
> > be used outside of 'fsck.c'.
> > * Patch 2 removes the obsolete/never-used code in 'test-tool submodule
> > check-name' handling names provided on the command line.
> > * Patch 3 adds a 'check-url' mode to 'test-tool submodule', calling the
> > now-public 'check_submodule_url()' method on a given URL, and adds new
> > tests checking valid and invalid submodule URLs.
> > * Patch 4 replaces the 'credential_from_url_gently()' check with
> > 'url_normalize()' followed by 'url_decode()' and an explicit check for
> > newlines (to preserve the newline handling added in 07259e74ec1 (fsck:
> > detect gitmodules URLs with embedded newlines, 2020-03-11)).
>
> Nicely done. I'll wait for a few days to see if anybody else has
> reaction but after reading the patches myself, my inclination is to
> suggest merging it to 'next'.
It all looks good to me to go to 'next'.
After simplifying the input handling in patch 2, I probably would not
have bothered with the abstracted interface in patch 3 (and instead just
repeated the few lines of boilerplate, since there's so much already).
Mostly just because function pointers in C often make reading and
debugging more annoying. But I don't think it's a very big deal either
way in this instance.
-Peff
^ permalink raw reply
* partial commit of file-to-directory change, was Re: Bugreport
From: Jeff King @ 2024-01-20 0:46 UTC (permalink / raw)
To: brian m. carlson; +Cc: Junio C Hamano, Frank Schwidom, git
In-Reply-To: <ZasCZ0YetzmlBFvw@tapette.crustytoothpaste.net>
On Fri, Jan 19, 2024 at 11:14:47PM +0000, brian m. carlson wrote:
> > $ git commit . -m +
> > error: 'd' does not have a commit checked out
> > fatal: updating files failed
>
> I can confirm this behaviour[0], and it's definitely wrong that it
> thinks `d` is a submodule. It does, however, work if you do `git commit
> -m +` (that is, without the .), which makes sense since the relevant
> change is already staged in the index.
>
> I'm not going to get to a patch or more thorough investigation, but
> perhaps someone else will.
I peeked at this a bit earlier; I didn't come up with a solution, but
here are a few more details.
As you noted, the problem is only with giving a pathspec to "git
commit". The bug is in the custom code git-commit uses to set up the
index for a partial commit. It generates a list of entries for the
partial commit via list_path(), and then tries to add them to the index.
But of course "d", being a directory, does not make any sense as an
index entry itself (unless as a submodule).
I'd have thought that we should just be using the same code that "git
add" does here (which obviously works). But all of this comes from
2888605c64 (builtin-commit: fix partial-commit support, 2007-11-18),
which explicitly says "you can't just run add_files_to_cache()", which
is what git-add does under the hood.
I don't know if we could revisit those assumptions and reuse some of the
existing code, or if the custom partial-commit code could be fixed. I
guess the root of the issue is that in the call to list_paths(), we call
overlay_tree_on_index(). And that's how we end up thinking that "d" is a
useful path to talk about, even though it has already been removed from
the index (we have "d/b.txt" instead).
So I'm not sure if we need a solution here-ish:
diff --git a/builtin/commit.c b/builtin/commit.c
index 65196a2827..25e65e986d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -302,7 +302,9 @@ static void add_remove_files(struct string_list *list)
continue;
if (!lstat(p->string, &st)) {
- if (add_to_index(&the_index, p->string, &st, 0))
+ if (S_ISDIR(st.st_mode))
+ warning("woah, %s is a dir", p->string);
+ else if (add_to_index(&the_index, p->string, &st, 0))
die(_("updating files failed"));
} else
remove_file_from_index(&the_index, p->string);
but I'm not quite sure what we should be doing instead of that
warning(). Should we updating and including everything in "d/"? What
about if there were a "d/c.txt" that was not a tracked file at all?
It might also be that the bug is earlier in list_paths(), where we
should notice that "d" is gone and now we have entries under "d/".
I dunno. I probably won't dig much further on this myself, but it's
possible Junio (cc'd) might have an idea right away.
-Peff
^ permalink raw reply related
* [PATCH] setup: allow cwd=.git w/ bareRepository=explicit
From: Kyle Lippincott via GitGitGadget @ 2024-01-20 0:08 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott, Kyle Lippincott
From: Kyle Lippincott <spectral@google.com>
The safe.bareRepository setting can be set to 'explicit' to disallow
implicit uses of bare repositories, preventing an attack [1] where an
artificial and malicious bare repository is embedded in another git
repository. Unfortunately, some tooling uses myrepo/.git/ as the cwd
when executing commands, and this is blocked when
safe.bareRepository=explicit. Blocking is unnecessary, as git already
prevents nested .git directories.
Teach git to not reject uses of git inside of the .git directory: check
if cwd is .git (or a subdirectory of it) and allow it even if
safe.bareRepository=explicit.
[1] https://github.com/justinsteven/advisories/blob/main/2022_git_buried_bare_repos_and_fsmonitor_various_abuses.md
Signed-off-by: Kyle Lippincott <spectral@google.com>
---
setup: allow cwd=.git w/ bareRepository=explicit
Please be aware that I'm a new contributor (this is my first patch to
git's code), so any style nits, suggestions about how to make this more
idiomatic, or any other suggestions are strongly encouraged.
My primary concern with this patch is that I'm unsure if we need to
worry about case-insensitive filesystems (ex: cwd=my_repo/.GIT instead
of my_repo/.git, it might not trigger this logic and end up allowed).
I'm assuming this isn't a significant concern, for two reasons:
* most filesystems/OSes in use today (by number of users) are at least
case-preserving, so users/tools will have had to type out .GIT
instead of getting it from readdir/wherever.
* this is primarily a "quality of life" change to the feature, and if
we get it wrong we still fail closed.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1645%2Fspectral54%2Fbare-repo-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1645/spectral54/bare-repo-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1645
setup.c | 3 ++-
t/t0035-safe-bare-repository.sh | 8 ++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/setup.c b/setup.c
index b38702718fb..b095e284979 100644
--- a/setup.c
+++ b/setup.c
@@ -1371,7 +1371,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
if (is_git_directory(dir->buf)) {
trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
- if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)
+ if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&
+ !ends_with_path_components(dir->buf, ".git"))
return GIT_DIR_DISALLOWED_BARE;
if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
return GIT_DIR_INVALID_OWNERSHIP;
diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
index 038b8b788d7..80488563795 100755
--- a/t/t0035-safe-bare-repository.sh
+++ b/t/t0035-safe-bare-repository.sh
@@ -78,4 +78,12 @@ test_expect_success 'no trace when GIT_DIR is explicitly provided' '
expect_accepted_explicit "$pwd/outer-repo/bare-repo"
'
+test_expect_success 'no trace when "bare repository" is .git' '
+ expect_accepted_implicit -C outer-repo/.git
+'
+
+test_expect_success 'no trace when "bare repository" is a subdir of .git' '
+ expect_accepted_implicit -C outer-repo/.git/objects
+'
+
test_done
base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5
--
gitgitgadget
^ permalink raw reply related
* Re: Bugreport
From: brian m. carlson @ 2024-01-19 23:14 UTC (permalink / raw)
To: Frank Schwidom; +Cc: git
In-Reply-To: <20240119132551.GA31532@debian64>
[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]
On 2024-01-19 at 13:25:51, Frank Schwidom wrote:
>
> This bug exists in possibly all git versions.
>
> $ git init
> $ touch a.txt
> $ ln -s a.txt d
> $ git add .
> $ git commit -m + .
> [master (root-commit) f6b4468] +
> 2 files changed, 1 insertion(+)
> create mode 100644 a.txt
> create mode 120000 d
> $ ls -la
> total 12
> drwxr-xr-x 3 ox ox 4096 Jan 19 14:10 .
> drwxr-xr-x 4 ox ox 4096 Jan 19 14:04 ..
> drwxr-xr-x 8 ox ox 4096 Jan 19 14:10 .git
> -rw-r--r-- 1 ox ox 0 Jan 19 14:10 a.txt
> lrwxrwxrwx 1 ox ox 5 Jan 19 14:10 d -> a.txt
> $ rm d
> $ mkdir d
> $ touch d/b.txt
> $ git add .
> $ git commit . -m +
> error: 'd' does not have a commit checked out
> fatal: updating files failed
I can confirm this behaviour[0], and it's definitely wrong that it
thinks `d` is a submodule. It does, however, work if you do `git commit
-m +` (that is, without the .), which makes sense since the relevant
change is already staged in the index.
I'm not going to get to a patch or more thorough investigation, but
perhaps someone else will.
[0] git version 2.43.0.381.gb435a96ce8
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ 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