* Re: [PATCH] setup: Only allow extenions.objectFormat to be specified once
From: Eric W. Biederman @ 2023-09-27 13:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, brian m. carlson
In-Reply-To: <xmqqr0mkmx9b.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Eric W. Biederman" <ebiederm@gmail.com> writes:
>
>> Today there is no sanity checking of what happens when
>> extensions.objectFormat is specified multiple times. Catch confused git
>> configurations by only allowing this option to be specified once.
>
> Hmph. I am not sure if this is worth doing, and especially only for
> "objectformat". Do we intend to apply different rules other than
> "you can give it only once" to other extensions, and if so where
> will these rules be catalogued? I do not see particular harm to let
> them follow the usual "last one wins".
>
> If the patch were about trying to make sure that extensions, which
> are inherentaly per-repository, appear only in $GIT_DIR/config and
> complain if the code gets confused and tried to read them from the
> system or global configuration files, I would understand, and
> strongly support such an effort, ithough.
Unless I misread something, the code only checks for [extensions]
in $GIT_DIR/config.
> The real sanity we want to enforce is that what is reported by
> running "git config extensions.objectformat" must match the object
> format that is used in refs and object database.
Agreed. Allowing git config extensions.objectformat to change the
existing value is allowing the repository to be corrupted.
> Manually futzing
> the configuration file and adding an entry with a contradictory
> value certainly is one way to break that sanity, and this patch may
> catch such a breakage, but once we start worrying about manually
> futzing the configuration file, the check added here would easily
> miss if the futzing is done by replacing instead of adding, so I am
> not sure if this extra code is worth its bits.
>
> But perhaps I am missing something and not seeing why it is worth
> insisting on "last one is the first one" for this particular one.
I somewhat have blinders on. There are 3 configuration options I am
concerned with:
extensions.objectFormat
extensions.compatObjectFormat
core.historicObjectFormat (or whatever name we settle on).
One key concern I heard expressed in earlier reviews is that however we
handle these options we handle them in such a way as to give ourselves
room to rise to challenges in the future.
Whatever we do with parsing we have the following logical
constraints:
For extensions.objectFormat: There can only be a single storage hash.
For extensions.compatObjectFormat: There can be no compatibility hash,
there can be a single compatibility hash, and depending how things go
between now and the next hash function transition we might want multiple
compatibility hashes.
For core.historicObjectFormat: There can be no historic hash function, there
can be a single historic hash function, there can be multiple historic
hash functions.
For the compatibility hash I think it is unlikely we will want to
support more than one compatibility hash in practice but I can imagine
a scenario where we just get into the transition from SHA-1 to SHA-256
and a serious break is discovered that requires switching to FutureHash
ASAP.
For historic object formats like SHA-1 will become post transition there
are references embedded in commit comments, email messages, bug
trackers. All kinds of places that we can not update so there is
fundamentally a need to be able to find which current objects correspond
to the historic names. For a project each hash function transition will
create more such objects.
When I looked I saw two ways within current git to specify a list of
values for a single configuration option.
- Give that option multiple times.
- Parse the option value in such a way as to generate a list.
It is my sense just specifying the compatObjectFormat multiple times to
specify multiple compatibility object formats makes the most sense.
Especially as all is needed today is to only allow a single value.
After I had implemented the only allow once logic for compatObjectFormat
I saw that objectFormat had nothing similar, and knowing it is a bug
for multiple objectFormat wrote a patch to enforce only appear once
for objectFormat as well.
For objectFormat I don't care very much. For compatObjectFormat I truly
care, and for even more for the option that allows finding the current
object from a historic oid (even a truncated one) I care very much.
For me the fundamental question is if we allow multiples compatibility
hashes or historical hashes how do we specify them? Have the option
appear more than once? A comma separated list?
Whatever we decided I want to enforce that doesn't appear in current
configurations so we can support for multiples later.
Eric
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>> setup.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/setup.c b/setup.c
>> index 18927a847b86..ef9f79b8885e 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -580,6 +580,7 @@ static enum extension_result handle_extension(const char *var,
>> if (!strcmp(ext, "noop-v1")) {
>> return EXTENSION_OK;
>> } else if (!strcmp(ext, "objectformat")) {
>> + struct string_list_item *item;
>> int format;
>>
>> if (!value)
>> @@ -588,6 +589,13 @@ static enum extension_result handle_extension(const char *var,
>> if (format == GIT_HASH_UNKNOWN)
>> return error(_("invalid value for '%s': '%s'"),
>> "extensions.objectformat", value);
>> + /* Only support objectFormat being specified once. */
>> + for_each_string_list_item(item, &data->v1_only_extensions) {
>> + if (!strcmp(item->string, "objectformat"))
>> + return error(_("'%s' already specified as '%s'"),
>> + "extensions.objectformat",
>> + hash_algos[data->hash_algo].name);
>> + }
>> data->hash_algo = format;
>> return EXTENSION_OK;
>> }
^ permalink raw reply
* Re: git-retry tool or git.retry config (built-in implementation)?
From: Yaroslav Halchenko @ 2023-09-27 12:42 UTC (permalink / raw)
To: Bagas Sanjaya; +Cc: Git Mailing List, Isaac To, Junio C Hamano
In-Reply-To: <ZRNv-n_VlIDPX0oi@debian.me>
On Wed, 27 Sep 2023, Bagas Sanjaya wrote:
> On Tue, Sep 26, 2023 at 12:47:51PM -0400, Yaroslav Halchenko wrote:
> > Dear Git Gurus,
> > In DataLad (https://datalad.org) we are doing lots of automated cloning,
> > fetching etc as part of our CI etc jobs. Once in a while git operations
> > fail [see e.g. 1], and beg us to retry but we need to know when to
> > do so, and not do it upon every failed git invocation since some
> > failures could be legit (repository is gone). While looking how others
> > solve it we found
> > https://stackoverflow.com/questions/35014012/git-retry-if-http-request-failed
> > which pointed to tools like git-retry and later part of
> > https://chromium.googlesource.com/infra/infra/+/HEAD/go/src/infra/tools/git/retry_regexp.go
> > which serve as a collection of regexes to be on lookout for to retry.
> > Would that be the "best" strategy currently?
> Looking at the actual git_retry.py script [1], it really just wraps
> actual Git commands. IMO, git-retry(1) shell script as you mentioned
> only calls the python version, which adds another level of indirection
> (why not doing it in pure shell?).
My guess would be that it is just easier to code in Python usually for
such cases with a "registry" of hits etc. But why not just to strip .py
from python script which has shebang already and not require bash
wrapper at all? ;)
> AFAIK, to solve the retrying problem, we need to have a way to tell
> transport backend (curl/ssh) to resume transfer from the faulty point.
some times it seems not even getting connected (https) entirely and that
(?) leading to error in the caller above, e.g. from
https://github.com/datalad/datalad/issues/7485#issuecomment-1735619755
error: Failed to connect to datasets-tests.datalad.org port 443 after 8291 ms: Couldn't connect to server (curl_result = 28, http_code = 0, sha1 = 3980af8de56946a10ff5c48879e5d6025965d936)\nerror: Unable to find 3980af8de56946a10ff5c48879e5d6025965d936 under ...
> > As regex matching might eventually break whenever `git` changes
> > anything in the output messages, I wondered if there could be a more
> > robust internal implementation in git itself? Similarly git-annex has
> > annex.retry config setting which sets the count of retries for
> > "retriable" operations.
> Do you use porcelain interfaces instead of plumbing ones?
I would say -- a "mix".
--
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
WWW: http://www.linkedin.com/in/yarik
^ permalink raw reply
* Re: Projects for the next Outreachy round
From: Christian Couder @ 2023-09-27 6:33 UTC (permalink / raw)
To: Shuqi Liang; +Cc: git, Victoria Dye, Kaartic Sivaraam, Hariom verma, nasamuffin
In-Reply-To: <CAMO4yUEws6CmBH+rYynnGGsX067iJRgyQwVxa2b4mkYwnV_u+A@mail.gmail.com>
Hi Shuqi,
On Wed, Sep 27, 2023 at 5:30 AM Shuqi Liang <cheskaqiqi@gmail.com> wrote:
> Hi Christian,
>
> I'm afraid I won't have sufficient time to take on the remaining
> commands in the upcoming school year. I'd like to leave those for other
> contributors who are interested.
Sure, no worries. As per Victoria's suggestion, I will not propose
this as a project for the upcoming Outreachy round though.
> Sorry for not providing more details in my final report. In 2023 ideas[1]
> git write-tree, git diff-files, git diff-tree, git worktree have all
> been successfully
> implemented with sparse index integration.There are some remaining
> issues in git check-attr related to diffs.[2][3]
>
> Two other commands on the list are being worked on by Raghul:
> 'git describe' has been successfully integrated, 'git diff-index'
> still need more work. The remaining commands on the list have not been
> started on.
>
> [1]https://git.github.io/SoC-2023-Ideas/
> [2]https://lore.kernel.org/git/20230811142211.4547-3-cheskaqiqi@gmail.com/
> [3]https://cheskaqiqi.github.io/2023/08/20/Week9-10/
Thanks for this information! The details will be useful to
contributors interested in this.
Best,
Christian.
^ permalink raw reply
* Re: Projects for the next Outreachy round
From: Christian Couder @ 2023-09-27 6:26 UTC (permalink / raw)
To: Kousik Sanagavarapu
Cc: git, Shuqi Liang, Victoria Dye, Kaartic Sivaraam, Hariom verma
In-Reply-To: <ZRMJRVl16gBEElGZ@five231003>
Hi Kousik,
On Tue, Sep 26, 2023 at 6:39 PM Kousik Sanagavarapu
<five231003@gmail.com> wrote:
>
> Hi,
>
> On Tue, Sep 26, 2023 at 04:48:12PM +0200, Christian Couder wrote:
> > By the way, congrats to Shuqi and
> > Kousik for successfully completing their projects!
>
> Thanks.
>
> > So Shuqi and Kousik, please tell us if you would like to continue
> > working on your projects or if it's Ok if we propose them for
> > Outreachy.
>
> I would like to continue working on the project and hopefully complete
> the duplication of the remaining pretty formats into ref-fitler.
Ok, I will not propose the project for the upcoming Outreachy round then.
Thanks for your answer and for continuing to work on it,
Christian.
^ permalink raw reply
* Re: Projects for the next Outreachy round
From: Christian Couder @ 2023-09-27 6:18 UTC (permalink / raw)
To: Victoria Dye
Cc: git, Kousik Sanagavarapu, Shuqi Liang, Kaartic Sivaraam,
Hariom verma
In-Reply-To: <1c12ac0a-5e45-bb27-c452-250ffd4b9320@github.com>
Hi Victoria,
On Tue, Sep 26, 2023 at 7:18 PM Victoria Dye <vdye@github.com> wrote:
>
> Christian Couder wrote:
> > About the "More Sparse Index Integrations" Shuqi worked on, mentored
> > by Victoria, I am likely not the best person to mentor it, but I think
> > I could manage. It would be nice though if I got an idea about what
> > should be done next and how much work is left in general in this area.
> > (Shuqi's GSoC final report at
> > https://cheskaqiqi.github.io/2023/08/22/Final/ doesn't talk much about
> > this.) Perhaps even if Shuqi is continuing to work on the project,
> > there is still work that could be done in parallel on other commands
> > than the ones he is working on.
>
> To be honest, I'd recommend against using "More Sparse Index Integrations"
> as a project again - I was actually going to suggest "retiring" the project
> after this past GSoC term. The remaining commands are all fairly complex, to
> the point that they'd be challenging even for someone that's done a lot of
> sparse index work.
Ok, I will not propose that project then.
> All that said, if someone is *really* interested in this project, you might
> be able to get it to work. You'll probably want to limit the scope to one
> command and make sure there's a strong emphasis placed on testing. Sparse
> index integrations can introduce a lot of subtle bugs (e.g. the one Shuqi
> found in 'diff' [1]), and a buggy command is worse for users than lacking
> sparse index compatibility.
>
> I hope that helps!
Sure, thanks for the interesting update on this,
Christian.
^ permalink raw reply
* Re: Projects for the next Outreachy round
From: Christian Couder @ 2023-09-27 6:14 UTC (permalink / raw)
To: Emily Shaffer
Cc: git, Kousik Sanagavarapu, Shuqi Liang, Victoria Dye,
Kaartic Sivaraam, Hariom verma
In-Reply-To: <CAJoAoZmC-Ku80aSp5xkxfirJitoi_ai6ryK3cJ7Jkx_2fUjENw@mail.gmail.com>
Hi Emily,
On Wed, Sep 27, 2023 at 12:23 AM Emily Shaffer <nasamuffin@google.com> wrote:
>
> On Tue, Sep 26, 2023 at 7:48 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> > Hi everyone, especially Shuqi and Kousik,
> >
> > September 29 is the deadline for proposing Outreachy projects. As we
> > have at least one mentor for now (me), we should propose at least one
> > project though.
> >
> > All ideas for projects are welcome, don't hesitate to suggest one!
>
> It is probably premature for this cycle. But once we have unit testing
> framework[1] in, I expect that moving tests from t/helper/test-foo.c +
> a sh test to invoke test-foo over to using the unit testing framework
> could be a tidy project for GSOC/Outreachy. Especially if we can have
> an example migration to refer to, which Siddharth from Google is
> working on in his 20% time just now :)
>
> 1: https://lore.kernel.org/git/cover.1692297001.git.steadmon@google.com/
Thanks for the great idea! I am going to propose this as the coding
part of Outreachy should start only in December and I think there are
good chances that the unit test framework will be merged by then. In
case everything is not ready when Outreachy starts, I think we will
probably find other unit tests or libification related work that the
intern can do.
For example, I recently thought about converting commands that store
config info in global variables and use old config functions like
git_config_(int|bool|string|...) to instead use the git_config_get_*()
functions and no more global variables. I wanted to ask about it
today, but I think your suggestion would be better.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin
From: Junio C Hamano @ 2023-09-27 4:08 UTC (permalink / raw)
To: Taylor Blau; +Cc: Eric W. Biederman, brian m. carlson, git
In-Reply-To: <ZROHrSmmZOIE6bl9@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> Hmm. I wonder if retaining some flexibility in the bulk-checkin
> mechanism may be worthwhile. We discussed at the Contributor's
> Summit[^1] today that the bulk-checkin system may be a good fit for
> packing any blobs/trees created by `merge-tree` or `replay` instead of
> writing them out as loose objects.
But see the last paragraph of my review comments for the earlier
round upthread. This particular function implements logic that is
only applicable to blob objects, and streaming trees, commits, and
tags will need their own separate helper functions. And when they
are written, the top-level stream_to_pack() function can be
reintroduced, which will be a thin dispatcher to the four
type-specific helpers.
^ permalink raw reply
* [silly] loose, pack, and another thing?
From: Junio C Hamano @ 2023-09-27 4:15 UTC (permalink / raw)
To: git
Just wondering if it would help to have the third kind of object
representation in the object database, sitting next to loose objects
and packed objects, say .git/objects/verbatim/<hex-object-name> for
the contents and .git/objects/verbatim/<hex-object-name>.type that
records "blob", "tree", "commit", or "tag" (in practice, I would
expect huge "blob" objects would be the only ones that use this
mechanism).
The contents will be stored verbatim without compression and without
any object header (i.e., the usual "<type> <length>\0") and the file
could be "ln"ed (or "cow"ed if the underlying filesystem allows it)
to materialize it in the working tree if needed.
"fsck" needs to be told about how to verify them. Create the object
header in-core and hash that, followed by the contents of that file,
and make sure the result matches the <hex-object-name> part of the
filename, or something like that.
^ permalink raw reply
* Re: Projects for the next Outreachy round
From: Shuqi Liang @ 2023-09-27 3:30 UTC (permalink / raw)
To: Christian Couder, git, Victoria Dye, Kaartic Sivaraam,
Hariom verma, nasamuffin
In-Reply-To: <CAP8UFD1bsez-eMis5yH7Esds+LkhMnj0qTUMFPL1tRuDv2fiPw@mail.gmail.com>
On Tue, Sep 26, 2023 at 10:48 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> So Shuqi and Kousik, please tell us if you would like to continue
> working on your projects or if it's Ok if we propose them for
> Outreachy.
Hi Christian,
I'm afraid I won't have sufficient time to take on the remaining
commands in the upcoming school year. I'd like to leave those for other
contributors who are interested.
>It would be nice though if I got an idea about what
> should be done next and how much work is left in general in this area.
> (Shuqi's GSoC final report at
> https://cheskaqiqi.github.io/2023/08/22/Final/ doesn't talk much about
> this.) Perhaps even if Shuqi is continuing to work on the project,
> there is still work that could be done in parallel on other commands
> than the ones he is working on.
Sorry for not providing more details in my final report. In 2023 ideas[1]
git write-tree, git diff-files, git diff-tree, git worktree have all
been successfully
implemented with sparse index integration.There are some remaining
issues in git check-attr related to diffs.[2][3]
Two other commands on the list are being worked on by Raghul:
'git describe' has been successfully integrated, 'git diff-index'
still need more work. The remaining commands on the list have not been
started on.
[1]https://git.github.io/SoC-2023-Ideas/
[2]https://lore.kernel.org/git/20230811142211.4547-3-cheskaqiqi@gmail.com/
[3]https://cheskaqiqi.github.io/2023/08/20/Week9-10/
hope that helps!
Shuqi
^ permalink raw reply
* Re: [PATCH v2] diff --stat: set the width defaults in a helper function
From: Dragan Simic @ 2023-09-27 2:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <d45d1dac1a20699e370905b88b6fd0ec296751e7.1695441501.git.dsimic@manjaro.org>
Just a brief reminder about version 2 of this patch...
On 2023-09-23 06:01, Dragan Simic wrote:
> Extract the commonly used initialization of the --stat-width=<width>,
> --stat-name-width=<width> and --stat-graph-with=<width> parameters to
> their
> internal default values into a helper function, to avoid repeating the
> same
> initialization code in a few places.
>
> Add a couple of tests to additionally cover existing configuration
> options
> diff.statNameWidth=<width> and diff.statGraphWidth=<width> when used by
> git-merge to generate --stat outputs. This closes the gap that existed
> previously in the --stat tests, and reduces the chances for having any
> regressions introduced by this commit.
>
> While there, perform a small bunch of minor wording tweaks in the
> improved
> unit test, to improve its test-level consistency a bit.
>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
> builtin/diff.c | 4 +--
> builtin/log.c | 6 ++---
> builtin/merge.c | 4 +--
> builtin/rebase.c | 4 +--
> diff.c | 7 +++++
> diff.h | 1 +
> t/t4052-stat-output.sh | 60 ++++++++++++++++++++++++++----------------
> 7 files changed, 51 insertions(+), 35 deletions(-)
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index c0f564273a..55e7d21755 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -474,9 +474,7 @@ int cmd_diff(int argc, const char **argv, const
> char *prefix)
> repo_init_revisions(the_repository, &rev, prefix);
>
> /* Set up defaults that will apply to both no-index and regular
> diffs. */
> - rev.diffopt.stat_width = -1;
> - rev.diffopt.stat_name_width = -1;
> - rev.diffopt.stat_graph_width = -1;
> + init_diffstat_widths(&rev.diffopt);
> rev.diffopt.flags.allow_external = 1;
> rev.diffopt.flags.allow_textconv = 1;
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 80e1be1645..ba775d7b5c 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -176,17 +176,15 @@ static void cmd_log_init_defaults(struct rev_info
> *rev)
> if (default_follow)
> rev->diffopt.flags.default_follow_renames = 1;
> rev->verbose_header = 1;
> + init_diffstat_widths(&rev->diffopt);
> rev->diffopt.flags.recursive = 1;
> - rev->diffopt.stat_width = -1; /* use full terminal width */
> - rev->diffopt.stat_name_width = -1; /* respect statNameWidth config */
> - rev->diffopt.stat_graph_width = -1; /* respect statGraphWidth config
> */
> + rev->diffopt.flags.allow_textconv = 1;
> rev->abbrev_commit = default_abbrev_commit;
> rev->show_root_diff = default_show_root;
> rev->subject_prefix = fmt_patch_subject_prefix;
> rev->patch_name_max = fmt_patch_name_max;
> rev->show_signature = default_show_signature;
> rev->encode_email_headers = default_encode_email_headers;
> - rev->diffopt.flags.allow_textconv = 1;
>
> if (default_date_mode)
> parse_date_format(default_date_mode, &rev->date_mode);
> diff --git a/builtin/merge.c b/builtin/merge.c
> index fd21c0d4f4..8f397cbeff 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -466,9 +466,7 @@ static void finish(struct commit *head_commit,
> if (new_head && show_diffstat) {
> struct diff_options opts;
> repo_diff_setup(the_repository, &opts);
> - opts.stat_width = -1; /* use full terminal width */
> - opts.stat_name_width = -1; /* respect statNameWidth config */
> - opts.stat_graph_width = -1; /* respect statGraphWidth config */
> + init_diffstat_widths(&opts);
> opts.output_format |=
> DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> opts.detect_rename = DIFF_DETECT_RENAME;
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b93dca95a6..4783f90ac5 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1806,9 +1806,7 @@ int cmd_rebase(int argc, const char **argv,
> const char *prefix)
>
> /* We want color (if set), but no pager */
> repo_diff_setup(the_repository, &opts);
> - opts.stat_width = -1; /* use full terminal width */
> - opts.stat_name_width = -1; /* respect statNameWidth config */
> - opts.stat_graph_width = -1; /* respect statGraphWidth config */
> + init_diffstat_widths(&opts);
> opts.output_format |=
> DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> opts.detect_rename = DIFF_DETECT_RENAME;
> diff --git a/diff.c b/diff.c
> index 353e3b2cc9..2c602df10a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6936,6 +6936,13 @@ void diff_queued_diff_prefetch(void *repository)
> oid_array_clear(&to_fetch);
> }
>
> +void init_diffstat_widths(struct diff_options *options)
> +{
> + options->stat_width = -1; /* use full terminal width */
> + options->stat_name_width = -1; /* respect diff.statNameWidth config
> */
> + options->stat_graph_width = -1; /* respect diff.statGraphWidth
> config */
> +}
> +
> void diffcore_std(struct diff_options *options)
> {
> int output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT |
> diff --git a/diff.h b/diff.h
> index caf1528bf0..66bd8aeb29 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -573,6 +573,7 @@ int git_config_rename(const char *var, const char
> *value);
>
> #define DIFF_PICKAXE_IGNORE_CASE 32
>
> +void init_diffstat_widths(struct diff_options *);
> void diffcore_std(struct diff_options *);
> void diffcore_fix_diff_index(void);
>
> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
> index beb2ec2a55..7badd72488 100755
> --- a/t/t4052-stat-output.sh
> +++ b/t/t4052-stat-output.sh
> @@ -12,7 +12,7 @@ TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
> . "$TEST_DIRECTORY"/lib-terminal.sh
>
> -# 120 character name
> +# 120-character name
> name=aaaaaaaaaa
> name=$name$name$name$name$name$name$name$name$name$name$name$name
> test_expect_success 'preparation' '
> @@ -58,15 +58,15 @@ while read verb expect cmd args
> do
> # No width limit applied when statNameWidth is ignored
> case "$expect" in expect72|expect.6030)
> - test_expect_success "$cmd $verb statNameWidth config with long name"
> '
> + test_expect_success "$cmd $verb diff.statNameWidth with long name" '
> git -c diff.statNameWidth=30 $cmd $args >output &&
> grep " | " output >actual &&
> test_cmp $expect actual
> ';;
> esac
> # Maximum width limit still applied when statNameWidth is ignored
> case "$expect" in expect.60|expect.6030)
> - test_expect_success "$cmd --stat=width $verb statNameWidth config
> with long name" '
> + test_expect_success "$cmd --stat=width $verb diff.statNameWidth
> with long name" '
> git -c diff.statNameWidth=30 $cmd $args --stat=60 >output &&
> grep " | " output >actual &&
> test_cmp $expect actual
> @@ -111,19 +111,19 @@ do
> test_cmp $expect.6030 actual
> '
>
> - test_expect_success "$cmd --stat-name-width with long name" '
> + test_expect_success "$cmd --stat-name-width=width with long name" '
> git $cmd $args --stat-name-width=30 >output &&
> grep " | " output >actual &&
> test_cmp $expect.6030 actual
> '
> done <<\EOF
> expect2 format-patch --cover-letter -1 --stdout
> expect diff HEAD^ HEAD --stat
> expect show --stat
> expect log -1 --stat
> EOF
>
> -test_expect_success 'preparation for big change tests' '
> +test_expect_success 'preparation for big-change tests' '
> >abcd &&
> git add abcd &&
> git commit -m message &&
> @@ -139,7 +139,7 @@ cat >expect72 <<'EOF'
> abcd | 1000
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> abcd | 1000
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> EOF
> -test_expect_success "format-patch --cover-letter ignores COLUMNS (big
> change)" '
> +test_expect_success "format-patch --cover-letter ignores COLUMNS with
> big change" '
> COLUMNS=200 git format-patch -1 --stdout --cover-letter >output &&
> grep " | " output >actual &&
> test_cmp expect72 actual
> @@ -159,15 +159,15 @@ cat >expect200-graph <<'EOF'
> EOF
> while read verb expect cmd args
> do
> - test_expect_success "$cmd $verb COLUMNS (big change)" '
> + test_expect_success "$cmd $verb COLUMNS with big change" '
> COLUMNS=200 git $cmd $args >output &&
> grep " | " output >actual &&
> test_cmp "$expect" actual
> '
>
> case "$cmd" in diff|show) continue;; esac
>
> - test_expect_success "$cmd --graph $verb COLUMNS (big change)" '
> + test_expect_success "$cmd --graph $verb COLUMNS with big change" '
> COLUMNS=200 git $cmd $args --graph >output &&
> grep " | " output >actual &&
> test_cmp "$expect-graph" actual
> @@ -187,15 +187,15 @@ cat >expect40-graph <<'EOF'
> EOF
> while read verb expect cmd args
> do
> - test_expect_success "$cmd $verb not enough COLUMNS (big change)" '
> + test_expect_success "$cmd $verb not enough COLUMNS with big change" '
> COLUMNS=40 git $cmd $args >output &&
> grep " | " output >actual &&
> test_cmp "$expect" actual
> '
>
> case "$cmd" in diff|show) continue;; esac
>
> - test_expect_success "$cmd --graph $verb not enough COLUMNS (big
> change)" '
> + test_expect_success "$cmd --graph $verb not enough COLUMNS with big
> change" '
> COLUMNS=40 git $cmd $args --graph >output &&
> grep " | " output >actual &&
> test_cmp "$expect-graph" actual
> @@ -215,15 +215,15 @@ cat >expect40-graph <<'EOF'
> EOF
> while read verb expect cmd args
> do
> - test_expect_success "$cmd $verb statGraphWidth config" '
> + test_expect_success "$cmd $verb diff.statGraphWidth" '
> git -c diff.statGraphWidth=26 $cmd $args >output &&
> grep " | " output >actual &&
> test_cmp "$expect" actual
> '
>
> case "$cmd" in diff|show) continue;; esac
>
> - test_expect_success "$cmd --graph $verb statGraphWidth config" '
> + test_expect_success "$cmd --graph $verb diff.statGraphWidth" '
> git -c diff.statGraphWidth=26 $cmd $args --graph >output &&
> grep " | " output >actual &&
> test_cmp "$expect-graph" actual
> @@ -255,33 +255,33 @@ do
> test_cmp expect actual
> '
>
> - test_expect_success "$cmd --stat-graph-width with big change" '
> + test_expect_success "$cmd --stat-graph-width=width with big change" '
> git $cmd $args --stat-graph-width=26 >output &&
> grep " | " output >actual &&
> test_cmp expect actual
> '
>
> case "$cmd" in diff|show) continue;; esac
>
> test_expect_success "$cmd --stat-width=width --graph with big change"
> '
> git $cmd $args --stat-width=40 --graph >output &&
> grep " | " output >actual &&
> test_cmp expect-graph actual
> '
>
> - test_expect_success "$cmd --stat-graph-width --graph with big change"
> '
> + test_expect_success "$cmd --stat-graph-width=width --graph with big
> change" '
> git $cmd $args --stat-graph-width=26 --graph >output &&
> grep " | " output >actual &&
> test_cmp expect-graph actual
> '
> done <<\EOF
> format-patch -1 --stdout
> diff HEAD^ HEAD --stat
> show --stat
> log -1 --stat
> EOF
>
> -test_expect_success 'preparation for long filename tests' '
> +test_expect_success 'preparation for long-name tests' '
> cp abcd aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
> git add aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
> git commit -m message
> @@ -329,15 +329,15 @@ cat >expect200-graph <<'EOF'
> EOF
> while read verb expect cmd args
> do
> - test_expect_success "$cmd $verb COLUMNS (long filename)" '
> + test_expect_success "$cmd $verb COLUMNS with long name" '
> COLUMNS=200 git $cmd $args >output &&
> grep " | " output >actual &&
> test_cmp "$expect" actual
> '
>
> case "$cmd" in diff|show) continue;; esac
>
> - test_expect_success "$cmd --graph $verb COLUMNS (long filename)" '
> + test_expect_success "$cmd --graph $verb COLUMNS with long name" '
> COLUMNS=200 git $cmd $args --graph >output &&
> grep " | " output >actual &&
> test_cmp "$expect-graph" actual
> @@ -358,41 +358,57 @@ EOF
> while read verb expect cmd args
> do
> test_expect_success COLUMNS_CAN_BE_1 \
> - "$cmd $verb prefix greater than COLUMNS (big change)" '
> + "$cmd $verb prefix greater than COLUMNS with big change" '
> COLUMNS=1 git $cmd $args >output &&
> grep " | " output >actual &&
> test_cmp "$expect" actual
> '
>
> case "$cmd" in diff|show) continue;; esac
>
> test_expect_success COLUMNS_CAN_BE_1 \
> - "$cmd --graph $verb prefix greater than COLUMNS (big change)" '
> + "$cmd --graph $verb prefix greater than COLUMNS with big change" '
> COLUMNS=1 git $cmd $args --graph >output &&
> grep " | " output >actual &&
> test_cmp "$expect-graph" actual
> '
> done <<\EOF
> ignores expect72 format-patch -1 --stdout
> respects expect1 diff HEAD^ HEAD --stat
> respects expect1 show --stat
> respects expect1 log -1 --stat
> EOF
>
> cat >expect <<'EOF'
> abcd | 1000
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> EOF
> -test_expect_success 'merge --stat respects COLUMNS (big change)' '
> - git checkout -b branch HEAD^^ &&
> +test_expect_success 'merge --stat respects diff.statGraphWidth with
> big change' '
> + git checkout -b branch1 HEAD^^ &&
> + git -c diff.statGraphWidth=26 merge --stat --no-ff main^ >output &&
> + grep " | " output >actual &&
> + test_cmp expect40 actual
> +'
> +test_expect_success 'merge --stat respects COLUMNS with big change' '
> + git checkout -b branch2 HEAD^^ &&
> COLUMNS=100 git merge --stat --no-ff main^ >output &&
> grep " | " output >actual &&
> test_cmp expect actual
> '
>
> cat >expect <<'EOF'
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000
> +++++++++++++++++++++++++++++++++++++++
> EOF
> -test_expect_success 'merge --stat respects COLUMNS (long filename)' '
> +cat >expect.30 <<'EOF'
> + ...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000
> ++++++++++++++++++++++++++++++++++++++++
> +EOF
> +test_expect_success 'merge --stat respects diff.statNameWidth with
> long name' '
> + git switch branch1 &&
> + git -c diff.statNameWidth=30 merge --stat --no-ff main >output &&
> + grep " | " output >actual &&
> + test_cmp expect.30 actual
> +'
> +test_expect_success 'merge --stat respects COLUMNS with long name' '
> + git switch branch2 &&
> COLUMNS=100 git merge --stat --no-ff main >output &&
> grep " | " output >actual &&
> test_cmp expect actual
^ permalink raw reply
* Re: [PATCH] setup: Only allow extenions.objectFormat to be specified once
From: Junio C Hamano @ 2023-09-26 21:37 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git, brian m. carlson
In-Reply-To: <87h6ngapqb.fsf@gmail.froward.int.ebiederm.org>
"Eric W. Biederman" <ebiederm@gmail.com> writes:
> Today there is no sanity checking of what happens when
> extensions.objectFormat is specified multiple times. Catch confused git
> configurations by only allowing this option to be specified once.
Hmph. I am not sure if this is worth doing, and especially only for
"objectformat". Do we intend to apply different rules other than
"you can give it only once" to other extensions, and if so where
will these rules be catalogued? I do not see particular harm to let
them follow the usual "last one wins".
If the patch were about trying to make sure that extensions, which
are inherentaly per-repository, appear only in $GIT_DIR/config and
complain if the code gets confused and tried to read them from the
system or global configuration files, I would understand, and
strongly support such an effort, ithough.
The real sanity we want to enforce is that what is reported by
running "git config extensions.objectformat" must match the object
format that is used in refs and object database. Manually futzing
the configuration file and adding an entry with a contradictory
value certainly is one way to break that sanity, and this patch may
catch such a breakage, but once we start worrying about manually
futzing the configuration file, the check added here would easily
miss if the futzing is done by replacing instead of adding, so I am
not sure if this extra code is worth its bits.
But perhaps I am missing something and not seeing why it is worth
insisting on "last one is the first one" for this particular one.
Thanks.
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> setup.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/setup.c b/setup.c
> index 18927a847b86..ef9f79b8885e 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -580,6 +580,7 @@ static enum extension_result handle_extension(const char *var,
> if (!strcmp(ext, "noop-v1")) {
> return EXTENSION_OK;
> } else if (!strcmp(ext, "objectformat")) {
> + struct string_list_item *item;
> int format;
>
> if (!value)
> @@ -588,6 +589,13 @@ static enum extension_result handle_extension(const char *var,
> if (format == GIT_HASH_UNKNOWN)
> return error(_("invalid value for '%s': '%s'"),
> "extensions.objectformat", value);
> + /* Only support objectFormat being specified once. */
> + for_each_string_list_item(item, &data->v1_only_extensions) {
> + if (!strcmp(item->string, "objectformat"))
> + return error(_("'%s' already specified as '%s'"),
> + "extensions.objectformat",
> + hash_algos[data->hash_algo].name);
> + }
> data->hash_algo = format;
> return EXTENSION_OK;
> }
^ permalink raw reply
* Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin
From: Taylor Blau @ 2023-09-27 1:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric W. Biederman, brian m. carlson, git
In-Reply-To: <xmqqmsx8mwr4.fsf@gitster.g>
On Tue, Sep 26, 2023 at 02:48:31PM -0700, Junio C Hamano wrote:
> > Avoid all of those future complications by limiting index_bulk_checkin
> > to only work on blobs.
>
> Thanks. Will queue.
Hmm. I wonder if retaining some flexibility in the bulk-checkin
mechanism may be worthwhile. We discussed at the Contributor's
Summit[^1] today that the bulk-checkin system may be a good fit for
packing any blobs/trees created by `merge-tree` or `replay` instead of
writing them out as loose objects.
Being able to write trees in addition to blobs is definitely important
there, so we may want to wait on merging this down until that direction
solidifies a bit more. (FWIW, I started working on that today and hope
to have patches on the list in the next day or two).
Alternatively, if there is an urgency to merge these down, we can always
come back to it in the future and revert it if need be. Either way :-).
Thanks,
Taylor
[^1]: I'll clean up our notes in the next day or two and share them with
the list here.
^ permalink raw reply
* Re: Projects for the next Outreachy round
From: Emily Shaffer @ 2023-09-26 22:23 UTC (permalink / raw)
To: Christian Couder
Cc: git, Kousik Sanagavarapu, Shuqi Liang, Victoria Dye,
Kaartic Sivaraam, Hariom verma
In-Reply-To: <CAP8UFD1bsez-eMis5yH7Esds+LkhMnj0qTUMFPL1tRuDv2fiPw@mail.gmail.com>
On Tue, Sep 26, 2023 at 7:48 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> Hi everyone, especially Shuqi and Kousik,
>
> September 29 is the deadline for proposing Outreachy projects. As we
> have at least one mentor for now (me), we should propose at least one
> project though.
>
> All ideas for projects are welcome, don't hesitate to suggest one!
It is probably premature for this cycle. But once we have unit testing
framework[1] in, I expect that moving tests from t/helper/test-foo.c +
a sh test to invoke test-foo over to using the unit testing framework
could be a tidy project for GSOC/Outreachy. Especially if we can have
an example migration to refer to, which Siddharth from Google is
working on in his 20% time just now :)
1: https://lore.kernel.org/git/cover.1692297001.git.steadmon@google.com/
^ permalink raw reply
* Re: git-retry tool or git.retry config (built-in implementation)?
From: Bagas Sanjaya @ 2023-09-26 23:57 UTC (permalink / raw)
To: Yaroslav Halchenko, Git Mailing List; +Cc: Isaac To, Junio C Hamano
In-Reply-To: <ZRMLNyHXoWOj6K-l@bilena>
[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]
On Tue, Sep 26, 2023 at 12:47:51PM -0400, Yaroslav Halchenko wrote:
> Dear Git Gurus,
>
> In DataLad (https://datalad.org) we are doing lots of automated cloning,
> fetching etc as part of our CI etc jobs. Once in a while git operations
> fail [see e.g. 1], and beg us to retry but we need to know when to
> do so, and not do it upon every failed git invocation since some
> failures could be legit (repository is gone). While looking how others
> solve it we found
> https://stackoverflow.com/questions/35014012/git-retry-if-http-request-failed
> which pointed to tools like git-retry and later part of
> https://chromium.googlesource.com/infra/infra/+/HEAD/go/src/infra/tools/git/retry_regexp.go
> which serve as a collection of regexes to be on lookout for to retry.
>
> Would that be the "best" strategy currently?
Looking at the actual git_retry.py script [1], it really just wraps
actual Git commands. IMO, git-retry(1) shell script as you mentioned
only calls the python version, which adds another level of indirection
(why not doing it in pure shell?).
AFAIK, to solve the retrying problem, we need to have a way to tell
transport backend (curl/ssh) to resume transfer from the faulty point.
>
> As regex matching might eventually break whenever `git` changes
> anything in the output messages, I wondered if there could be a more
> robust internal implementation in git itself? Similarly git-annex has
> annex.retry config setting which sets the count of retries for
> "retriable" operations.
Do you use porcelain interfaces instead of plumbing ones?
Thanks.
[1]: https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/01d2cde990f22d409e74e239de7e4d347102d6f6/git_retry.py
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v3] bulk-checkin: only support blobs in index_bulk_checkin
From: Junio C Hamano @ 2023-09-26 21:48 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: brian m. carlson, git
In-Reply-To: <87msx99b9o.fsf_-_@gmail.froward.int.ebiederm.org>
"Eric W. Biederman" <ebiederm@gmail.com> writes:
> As the code is written today index_bulk_checkin only accepts blobs.
> Remove the enum object_type parameter and rename index_bulk_checkin to
> index_blob_bulk_checkin, index_stream to index_blob_stream,
> deflate_to_pack to deflate_blob_to_pack, stream_to_pack to
> stream_blob_to_pack, to make this explicit.
>
> Not supporting commits, tags, or trees has no downside as it is not
> currently supported now, and commits, tags, and trees being smaller by
> design do not have the problem that the problem that index_bulk_checkin
> was built to solve.
>
> Before we start adding code to support the hash function transition
> supporting additional objects types in index_bulk_checkin has no real
> additional cost, just an extra function parameter to know what the
> object type is. Once we begin the hash function transition this is not
> the case.
>
> The hash function transition document specifies that a repository with
> compatObjectFormat enabled will compute and store both the SHA-1 and
> SHA-256 hash of every object in the repository.
>
> What makes this a challenge is that it is not just an additional hash
> over the same object. Instead the hash function transition document
> specifies that the compatibility hash (specified with
> compatObjectFormat) be computed over the equivalent object that another
> git repository whose storage hash (specified with objectFormat) would
> store. When comparing equivalent repositories built with different
> storage hash functions, the oids embedded in objects used to refer to
> other objects differ and the location of signatures within objects
> differ.
>
> As blob objects have neither oids referring to other objects nor stored
> signatures their storage hash and their compatibility hash are computed
> over the same object.
>
> The other kinds of objects: trees, commits, and tags, all store oids
> referring to other objects. Signatures are stored in commit and tag
> objects. As oids and the tags to store signatures are not the same size
> in repositories built with different storage hashes the size of the
> equivalent objects are also different.
>
> A version of index_bulk_checkin that supports more than just blobs when
> computing both the SHA-1 and the SHA-256 of every object added would
> need a different, and more expensive structure. The structure is more
> expensive because it would be required to temporarily buffering the
> equivalent object the compatibility hash needs to be computed over.
>
> A temporary object is needed, because before a hash over an object can
> computed it's object header needs to be computed. One of the members of
> the object header is the entire size of the object. To know the size of
> an equivalent object an entire pass over the original object needs to be
> made, as trees, commits, and tags are composed of a variable number of
> variable sized pieces. Unfortunately there is no formula to compute the
> size of an equivalent object from just the size of the original object.
>
> Avoid all of those future complications by limiting index_bulk_checkin
> to only work on blobs.
Thanks. Will queue.
^ permalink raw reply
* Re: [PATCH] pretty-formats.txt: fix whitespace
From: Linus Arver @ 2023-09-26 21:20 UTC (permalink / raw)
To: Josh Soref; +Cc: Junio C Hamano, Josh Soref via GitGitGadget, git
In-Reply-To: <CACZqfqC3HWOuveQdbpVEEO5KOwmnugvUmm6M0WpVu2MtUopm-w@mail.gmail.com>
Josh Soref <jsoref@gmail.com> writes:
> Linus Arver wrote:
>> FWIW we already have some guidelines about what is acceptable for doc
>> patches in SubmittingPatches:
>>
>> We currently have a liberal mixture of US and UK English norms for
>> spelling and grammar, which is somewhat unfortunate. A huge patch that
>> touches the files all over the place only to correct the inconsistency
>> is not welcome, though. Potential clashes with other changes that can
>> result from such a patch are not worth it. We prefer to gradually
>> reconcile the inconsistencies in favor of US English, with small and
>> easily digestible patches, as a side effect of doing some other real
>> work in the vicinity (e.g. rewriting a paragraph for clarity, while
>> turning en_UK spelling to en_US). Obvious typographical fixes are much
>> more welcomed ("teh -> "the"), preferably submitted as independent
>> patches separate from other documentation changes.
>>
>> and both the 2-space vs 1-space and comma changes seem to fall into the
>> "not welcome" category.
>
> Ok, then, I think at this point I'll abandon this PR.
>
> I could probably find the time to justify rewriting one paragraph in
> this single file to try to abide by the requirements, but it doesn't
> seem worth it.
The change to make a paragraph read better (fixes to awkward wording,
for example) are very much appreciated and would be good as a follow-up
patch in its own right.
> I'm sorry I wasted everyone's time.
Please don't let this experience deter you from considering future
contributions to our documentation. Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] diff-merges: introduce '-d' option
From: Sergey Organov @ 2023-09-26 20:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqa5t8ooaj.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> No need to ask for a new option, as the behavior you describe is already
>> there, and is spelled "git log --diff-merges=first-parent"
>> (--diff-merges=1 for short).
>
> Ah, that changes things.
Only a tiny bit, unfortunately, as I'm still struggling to finally
convince you (((
>
> Making "--diff-merges=<how>" only about the presentation of merge
> commits, requiring a separate "-p" for single-parent commits [*],
> does make the life for those in the "merges are the only interesting
> things" camp a lot easier, exactly because the lack of "-p" can be
> used to say "I am not interested in chanages by single-parent
> commits".
>
> Side note: I personally think it is a design mistake of
> --diff-merges=<how> (e.g., --cc and --diff-merges=cc do not
> behave the same way) but that is a different story, and it
> is way too late now anyway to "fix" or change.
Side note: This has been considered and agreed upon when
--diff-merges= options were introduced, and as far as I recall,
at that time you explicitly agreed it might be useful to be able
to get output only for merge commits.
--cc is a simple alias for "--diff-merges=cc --patch" nowadays,
so yes, they do behave differently, and that's by design. Dunno
see any design mistake here, as we get all useful variations of
behavior with a straightforward design, more frequent use-cases
served by shorter options. Looks fine.
>
> So "-d" that stands for "--diff-merges=first-parent -p" makes the
> more useful (to those who think "merges are the only interesting
> things", which I do not belong to) "--diff-merges=first-parent"
> (without "-p") less useful. And the combination is not useful for
> those of us who find individual patches plus tweaks by merges
> (either --cc or --remerge-diff) are the way to look at the history.
Yes, you have your --cc, -c, and --remerge-diff (that I'd call something
like --rd probably, but anyway). Could I please have my simple,
straightforward, mnemonic, and terribly useful "-d" as well?
In other words, will I finally be faced with "if you need it, do it
yourself" argument? ;)
> I still do not think that we want to give a short-and-sweet single
> letter option for such a combination.
I have very simple desire: convenient way to tell Git to show me diff to
the first parent for merge commits, as that's the thing I need 99% of
times when I do request diff output at all. That's exactly what I'd have
seen as changes when I was about to commit the merge as well, similar to
any other commit. It's so natural that I can't figure why it looks so
damn rare or unusual to you, and that it makes you argue so hard against
-d, especially when -p, -c, --cc, or even -m, are already there?
I do sympathize your desire to be careful about short options, but what
reservation for "-d" do you still have in mind? It seems that it was
just waiting for me to come and finally bring it to life with the best
meaning possible. How long should I wait for it to remain unused to
finally be able to make use of it?
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH] pretty-formats.txt: fix whitespace
From: Josh Soref @ 2023-09-26 19:51 UTC (permalink / raw)
To: Linus Arver; +Cc: Junio C Hamano, Josh Soref via GitGitGadget, git
In-Reply-To: <owlypm24sr6n.fsf@fine.c.googlers.com>
Linus Arver wrote:
> FWIW we already have some guidelines about what is acceptable for doc
> patches in SubmittingPatches:
>
> We currently have a liberal mixture of US and UK English norms for
> spelling and grammar, which is somewhat unfortunate. A huge patch that
> touches the files all over the place only to correct the inconsistency
> is not welcome, though. Potential clashes with other changes that can
> result from such a patch are not worth it. We prefer to gradually
> reconcile the inconsistencies in favor of US English, with small and
> easily digestible patches, as a side effect of doing some other real
> work in the vicinity (e.g. rewriting a paragraph for clarity, while
> turning en_UK spelling to en_US). Obvious typographical fixes are much
> more welcomed ("teh -> "the"), preferably submitted as independent
> patches separate from other documentation changes.
>
> and both the 2-space vs 1-space and comma changes seem to fall into the
> "not welcome" category.
Ok, then, I think at this point I'll abandon this PR.
I could probably find the time to justify rewriting one paragraph in
this single file to try to abide by the requirements, but it doesn't
seem worth it. I'm sorry I wasted everyone's time.
^ permalink raw reply
* Re: [PATCH] pretty-formats.txt: fix whitespace
From: Linus Arver @ 2023-09-26 18:52 UTC (permalink / raw)
To: Josh Soref, Junio C Hamano; +Cc: Josh Soref via GitGitGadget, git
In-Reply-To: <CACZqfqCVsv-ZaSRWt_ejMn5f_U_1E2h7wsCgUg_50A+KHzOgkA@mail.gmail.com>
Josh Soref <jsoref@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> wrote:
>> > * space before `(`
>>
>> If you mean by the above that we used to say
>>
>> ... as described below(see linkgit:git-config[1]))
>>
>> and you added a SP before "(see", that is a definite improvement. I
>> however didn't find an example of a line that lacks SP before '('
>> that got corrected to have a SP there.
>
> I'm pretty sure that's what I meant, but I can't find the change in my
> git reflog, and I'm not quite sure how I could have made that mistake.
I too would support this sort of fix (fixing a missing space before
`(`).
WRT 2-space vs 1-space, I am not sure we need this because this is a
stylistic issue. Same thing for the commas. I am much more in favor of
changes that would actually help users find the information they need
(e.g., fixing spelling mistakes so that users who type in the correct
spelling can get the expected documentation hits).
FWIW we already have some guidelines about what is acceptable for doc
patches in SubmittingPatches:
We currently have a liberal mixture of US and UK English norms for
spelling and grammar, which is somewhat unfortunate. A huge patch that
touches the files all over the place only to correct the inconsistency
is not welcome, though. Potential clashes with other changes that can
result from such a patch are not worth it. We prefer to gradually
reconcile the inconsistencies in favor of US English, with small and
easily digestible patches, as a side effect of doing some other real
work in the vicinity (e.g. rewriting a paragraph for clarity, while
turning en_UK spelling to en_US). Obvious typographical fixes are much
more welcomed ("teh -> "the"), preferably submitted as independent
patches separate from other documentation changes.
and both the 2-space vs 1-space and comma changes seem to fall into the
"not welcome" category.
^ permalink raw reply
* Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision
From: John Cai @ 2023-09-26 18:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, John Cai via GitGitGadget, git
In-Reply-To: <xmqq1qer7vrv.fsf@gitster.g>
Hi Junio,
On 21 Sep 2023, at 4:52, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> In an empty repository, "git log" will die anyway. So I think the more
>> interesting case is "I have a repository with stuff in it, but HEAD
>> points to an unborn branch". So:
>>
>> git --attr-source=HEAD diff foo^ foo
>
> This still looks like a made-up example. Who in the right mind
> would specify HEAD when both of the revs involved in the operation
> are from branch 'foo'? The history of HEAD may not have anything
> common with the operand of the operation 'foo' (or its parent), or
> worse, it may not even exist.
>
> But your "in this repository we never trust attributes from working
> tree, take it instead from this file or from this blob" example does
> make a lot more sense as a use case.
>
>> And there you really are saying "if there are attributes in HEAD, use
>> them; otherwise, don't worry about it". This is exactly what we do with
>> mailmap.blob: in a bare repository it is set to HEAD by default, but if
>> HEAD does not resolve, we just ignore it (just like a HEAD that does not
>> contain a .mailmap file). And those match the non-bare cases, where we'd
>> read those files from the working tree instead.
>
> "HEAD" -> "HEAD:.mailmap" if I recall correctly.
>
> And if HEAD does not resolve, we pretend as if HEAD is an empty
> tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting
> to do the same for the attribute sources and treat unborn HEAD as if
> it specifies an empty tree-ish, without any configuration or an
> extra option.
>
> Such a change would be an end-user observable behaviour change, but
> nobody sane would be running "git --attr-source=HEAD diff HEAD^ HEAD"
> to check and detect an unborn HEAD for its error exit code, so I do
> not think it is a horribly wrong thing to do.
>
> But again, as you said, --attr-source=<tree-ish> does not sound like
> a good fit for bare-repository hosted environment and a tentative
> hack waiting for a proper attr.blob support, or something like that,
> to appear.
>
>> But what is weird about this patch is that we are using a config option
>> to change how a command-line option is interpreted. If the idea is that
>> some invocations care about the validity of the source and some do not,
>> then the config option is much too blunt. It is set once long ago, but
>> it can't distinguish between times you care about invalid sources and
>> times you don't.
>>
>> It would make much more sense to me to have another command-line option,
>> like:
>>
>> git --attr-source=HEAD --allow-invalid-attr-source
>
> Yeah, if we were to make it configurable without changing the
> default behaviour, I agree that would be more correct approach. A
> configuration does not sound like a good fit.
>
>> ... And I really think attr.blob is a better match for what GitLab
>> is trying to do here, because it is set once and applies to all
>> commands, rather than having to teach every invocation to pass it
>> (though I guess maybe they use it as an environment variable).
Between adding an --allow-invalid-attr-source, and adding attr.blob and
attr.allowInvalidSource I think I like adding the attr.blob config more.
>
> True, too.
>
> Thanks.
thanks
John
^ permalink raw reply
* Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision
From: John Cai @ 2023-09-26 18:27 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, John Cai via GitGitGadget, git
In-Reply-To: <20230921214021.GB302338@coredump.intra.peff.net>
Hi Peff,
On 21 Sep 2023, at 17:40, Jeff King wrote:
> On Thu, Sep 21, 2023 at 01:52:52AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> In an empty repository, "git log" will die anyway. So I think the more
>>> interesting case is "I have a repository with stuff in it, but HEAD
>>> points to an unborn branch". So:
>>>
>>> git --attr-source=HEAD diff foo^ foo
>>
>> This still looks like a made-up example. Who in the right mind
>> would specify HEAD when both of the revs involved in the operation
>> are from branch 'foo'? The history of HEAD may not have anything
>> common with the operand of the operation 'foo' (or its parent), or
>> worse, it may not even exist.
>
> I think it's unlikely for a user to write that. But if you are running a
> server full of bare repositories that does diffs, you might end up with
> a script that sticks "--attr-source=HEAD" at the beginning of every
> command.
>
> It is true that HEAD may not be related. But that is what we use if you
> are in a non-bare repository and run "git diff foo^ foo".
>
> Arguably:
>
> git --attr-source=$to diff $from $to
>
> is a better default for this command. But something like "git log -p" is
> trickier, as you have many commits to show. You can try to use the tip
> of the traversal, but there may be multiple. Using the attributes from
> the destination of each commit is the most likely to avoid divergence
> between the attributes and the code, but it may also not be what people
> want. Using the modern attributes from the working tree often makes
> showing historical commits much nicer.
>
> So I dunno. I could see arguments in both directions, but I think in
> general people have been OK with pulling attributes from the working
> tree. And --attr-source=HEAD is the bare equivalent.
>
>> But your "in this repository we never trust attributes from working
>> tree, take it instead from this file or from this blob" example does
>> make a lot more sense as a use case.
>
> I don't know that it was my example. :) But yes, if you do
> "--attr-source=$to", you're overriding even the non-bare case. That may
> be what you want for some cases, but as above, I think it's hard to
> apply consistently (or even what you'd want for the general case).
>
>>> And there you really are saying "if there are attributes in HEAD, use
>>> them; otherwise, don't worry about it". This is exactly what we do with
>>> mailmap.blob: in a bare repository it is set to HEAD by default, but if
>>> HEAD does not resolve, we just ignore it (just like a HEAD that does not
>>> contain a .mailmap file). And those match the non-bare cases, where we'd
>>> read those files from the working tree instead.
>>
>> "HEAD" -> "HEAD:.mailmap" if I recall correctly.
>
> True, yeah. We can't do that here because attributes are spread across
> the tree. So all my mentions of attr.blob would really be attr.tree.
>
>> And if HEAD does not resolve, we pretend as if HEAD is an empty
>> tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting
>> to do the same for the attribute sources and treat unborn HEAD as if
>> it specifies an empty tree-ish, without any configuration or an
>> extra option.
>>
>> Such a change would be an end-user observable behaviour change, but
>> nobody sane would be running "git --attr-source=HEAD diff HEAD^ HEAD"
>> to check and detect an unborn HEAD for its error exit code, so I do
>> not think it is a horribly wrong thing to do.
>
> Yeah, that is basically what I am proposing. It sounds from the
> discussion here that there are two interesting cases:
>
> 1. You want to use --attr-source=HEAD because you are trying to make a
> bare repo behave like a non-bare one. You probably want the "don't
> complain if it is missing" behavior.
Yep, this is the primary use case for us.
>
> 2. You are trying to use the attributes from one side of the diff to
> override the worktree ones (because the two trees are unrelated).
> In which case it does not really matter if --attr-source complains,
> because the diff will likewise complain if the tree cannot be
> resolved.
>
> Just trying to play devil's advocate, though, I guess you could run a
> non-diff operation like say:
>
> git --attr-source=my-branch check-attr foo
>
> and then you probably _do_ want to know if that source was ignored or
> typo'd.
>
>> But again, as you said, --attr-source=<tree-ish> does not sound like
>> a good fit for bare-repository hosted environment and a tentative
>> hack waiting for a proper attr.blob support, or something like that,
>> to appear.
>
> I think folks mentioned mailmap.blob in the original discussion for
> --attr-source. I don't remember why the patch went with --attr-source
> there. Maybe John can speak to that.
Yeah I was looking for this, and I think I found it in [1], which was part of a
separate patch having to do with gitattributes. If someone did mention it in the
patch for --attr-source, then I can't remember why we decided to go with the
comand line flag rather than the config.
1. https://lore.kernel.org/git/ZBMn5T6zfKK+PYUe@coredump.intra.peff.net/
>
> -Peff
^ permalink raw reply
* Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision
From: John Cai @ 2023-09-26 18:23 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, John Cai via GitGitGadget, git
In-Reply-To: <20230921041545.GA2338791@coredump.intra.peff.net>
Hi Peff,
On 21 Sep 2023, at 0:15, Jeff King wrote:
> On Wed, Sep 20, 2023 at 09:06:46AM -0700, Junio C Hamano wrote:
>
>>> With empty repositories however, HEAD does not point to a valid treeish,
>>> causing Git to die. This means we would need to check for a valid
>>> treeish each time.
>>
>> Naturally.
>>
>>> To avoid this, let's add a configuration that allows
>>> Git to simply ignore --attr-source if it does not resolve to a valid
>>> tree.
>>
>> Not convincing at all as to the reason why we want to do anything
>> "to avoid this". "git log" in a repository whose HEAD does not
>> point to a valid treeish. "git blame" dies with "no such ref:
>> HEAD". An empty repository (more precisely, an unborn history)
>> needs special casing if you want to present it if you do not want to
>> spew underlying error messages to the end users *anyway*. It is
>> unclear why seeing what commit the HEAD pointer points at (or which
>> branch it points at for that matter) is *an* *extra* and *otherwise*
>> *unnecessary* overhead that need to be avoided.
>
> In an empty repository, "git log" will die anyway. So I think the more
> interesting case is "I have a repository with stuff in it, but HEAD
> points to an unborn branch". So:
>
> git --attr-source=HEAD diff foo^ foo
>
> And there you really are saying "if there are attributes in HEAD, use
> them; otherwise, don't worry about it". This is exactly what we do with
> mailmap.blob: in a bare repository it is set to HEAD by default, but if
> HEAD does not resolve, we just ignore it (just like a HEAD that does not
> contain a .mailmap file). And those match the non-bare cases, where we'd
> read those files from the working tree instead.
>
> So I think the same notion applies here. You want to be able to point it
> at HEAD by default, but if there is no HEAD, that is the same as if HEAD
> simply did not contain any attributes. If we had attr.blob, that is
> exactly how I would expect it to work.
You captured this quite well!
>
> My gut feeling is that --attr-source should do the same, and just
> quietly ignore a ref that does not resolve. But I think an argument can
> be made that because the caller explicitly gave us a ref, they expect it
> to work (and that would catch misspellings, etc). Like:
>
> git --attr-source=my-barnch diff foo^ foo
>
> So I'm OK with not changing that behavior.
>
> But what is weird about this patch is that we are using a config option
> to change how a command-line option is interpreted. If the idea is that
> some invocations care about the validity of the source and some do not,
> then the config option is much too blunt. It is set once long ago, but
> it can't distinguish between times you care about invalid sources and
> times you don't.
>
> It would make much more sense to me to have another command-line option,
> like:
>
> git --attr-source=HEAD --allow-invalid-attr-source
>
> Obviously that is horrible to type, but I think the point is that you'd
> only do this from a script anyway (because it's those automated cases
> where you want to say "use HEAD only if it exists").
Yeah, I see the point about using a config to change default behavior of a
command leading to confusion.
>
> If there were an attr.blob config option and it complained about an
> invalid HEAD, _then_ I think attr.allowInvalidSource might make sense
> (though again, I would just argue for switching the behavior by
> default). And I really think attr.blob is a better match for what GitLab
> is trying to do here, because it is set once and applies to all
> commands, rather than having to teach every invocation to pass it
> (though I guess maybe they use it as an environment variable).
In retrospect perhaps a config would have been better here. I think this
patch started with improving an existing command line flag [1] by making
it global. So I think we were just thinking about command line flags and
didn't consider configs.
That being said, for GitLab at least there's not a lot of difference since
we pass in configs through the commandline anyways rather than relying on the config
state itself on disk for our bare server-side repositories.
1. https://lore.kernel.org/git/pull.1470.git.git.1679109928556.gitgitgadget@gmail.com/
>
> Of course I would think that, as the person who solved GitHub's exact
> same problem for mailmap by adding mailmap.blob. So you may ingest the
> appropriate grain of salt. :)
>
> -Peff
thanks
John
^ permalink raw reply
* Re: Projects for the next Outreachy round
From: Victoria Dye @ 2023-09-26 17:18 UTC (permalink / raw)
To: Christian Couder, git, Kousik Sanagavarapu, Shuqi Liang
Cc: Kaartic Sivaraam, Hariom verma
In-Reply-To: <CAP8UFD1bsez-eMis5yH7Esds+LkhMnj0qTUMFPL1tRuDv2fiPw@mail.gmail.com>
Christian Couder wrote:
> About the "More Sparse Index Integrations" Shuqi worked on, mentored
> by Victoria, I am likely not the best person to mentor it, but I think
> I could manage. It would be nice though if I got an idea about what
> should be done next and how much work is left in general in this area.
> (Shuqi's GSoC final report at
> https://cheskaqiqi.github.io/2023/08/22/Final/ doesn't talk much about
> this.) Perhaps even if Shuqi is continuing to work on the project,
> there is still work that could be done in parallel on other commands
> than the ones he is working on.
To be honest, I'd recommend against using "More Sparse Index Integrations"
as a project again - I was actually going to suggest "retiring" the project
after this past GSoC term. The remaining commands are all fairly complex, to
the point that they'd be challenging even for someone that's done a lot of
sparse index work.
All that said, if someone is *really* interested in this project, you might
be able to get it to work. You'll probably want to limit the scope to one
command and make sure there's a strong emphasis placed on testing. Sparse
index integrations can introduce a lot of subtle bugs (e.g. the one Shuqi
found in 'diff' [1]), and a buggy command is worse for users than lacking
sparse index compatibility.
I hope that helps!
- Victoria
[1] https://lore.kernel.org/git/20230811142211.4547-3-cheskaqiqi@gmail.com/
>
> Sorry for the late request, but please let me know soon about this.
>
> Thanks,
> Christian.
^ permalink raw reply
* Re: [PATCH 2/2] diff-merges: introduce '-d' option
From: Junio C Hamano @ 2023-09-26 17:08 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <87bkdpl2yx.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> No need to ask for a new option, as the behavior you describe is already
> there, and is spelled "git log --diff-merges=first-parent"
> (--diff-merges=1 for short).
Ah, that changes things.
Making "--diff-merges=<how>" only about the presentation of merge
commits, requiring a separate "-p" for single-parent commits [*],
does make the life for those in the "merges are the only interesting
things" camp a lot easier, exactly because the lack of "-p" can be
used to say "I am not interested in chanages by single-parent
commits".
Side note: I personally think it is a design mistake of
--diff-merges=<how> (e.g., --cc and --diff-merges=cc do not
behave the same way) but that is a different story, and it
is way too late now anyway to "fix" or change.
So "-d" that stands for "--diff-merges=first-parent -p" makes the
more useful (to those who think "merges are the only interesting
things", which I do not belong to) "--diff-merges=first-parent"
(without "-p") less useful. And the combination is not useful for
those of us who find individual patches plus tweaks by merges
(either --cc or --remerge-diff) are the way to look at the history.
I still do not think that we want to give a short-and-sweet single
letter option for such a combination.
Thanks for clarification.
^ permalink raw reply
* git-retry tool or git.retry config (built-in implementation)?
From: Yaroslav Halchenko @ 2023-09-26 16:47 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Isaac To
Dear Git Gurus,
In DataLad (https://datalad.org) we are doing lots of automated cloning,
fetching etc as part of our CI etc jobs. Once in a while git operations
fail [see e.g. 1], and beg us to retry but we need to know when to
do so, and not do it upon every failed git invocation since some
failures could be legit (repository is gone). While looking how others
solve it we found
https://stackoverflow.com/questions/35014012/git-retry-if-http-request-failed
which pointed to tools like git-retry and later part of
https://chromium.googlesource.com/infra/infra/+/HEAD/go/src/infra/tools/git/retry_regexp.go
which serve as a collection of regexes to be on lookout for to retry.
Would that be the "best" strategy currently?
As regex matching might eventually break whenever `git` changes
anything in the output messages, I wondered if there could be a more
robust internal implementation in git itself? Similarly git-annex has
annex.retry config setting which sets the count of retries for
"retriable" operations.
Do you think something like that could be implemented in git in some
foreseable future?
[1] https://github.com/datalad/datalad/issues/7485
Thank you in advance!
--
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
WWW: http://www.linkedin.com/in/yarik
^ 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