* Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code
From: Jeff King @ 2023-09-12 8:35 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Ævar Arnfjörð Bjarmason
In-Reply-To: <1a60a1ca-0ef0-ecf5-d0aa-a28d7c148a82@gmail.com>
On Sun, Sep 10, 2023 at 01:08:11AM +0200, Rubén Justo wrote:
> GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true
> use internnlly the --invert-exit-code machinery. Therefore if the user
> wants to use --invert-exit-code in combination with them, the result
> will be confusing.
>
> For the same reason, we are already using BAIL_OUT if the user tries to
> combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.
>
> Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and
> GIT_TEST_SANITIZE_LEAK_LOG=true.
OK, so we are trying to find a case where the user is triggering
--invert-exit-code themselves and complaining. But in the code...
> @@ -1557,15 +1557,25 @@ then
> say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
> invert_exit_code=t
> fi
> - elif test -z "$passes_sanitize_leak" &&
> - test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> + elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> then
> - skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> - test_done
> + if test -n "$invert_exit_code"
> + then
> + BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> + elif test -z "$passes_sanitize_leak"
> + then
> + skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> + test_done
> + fi
> fi
You can see at the top of the context that we will set
invert_exit_code=t ourselves, which will then complain here:
> if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
> then
> + if test -n "$invert_exit_code"
> + then
> + BAIL_OUT "cannot use --invert-exit-code and GIT_TEST_SANITIZE_LEAK_LOG=true"
> + fi
> +
> if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
> then
> BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
That varible-set in the earlier context is from running in "check" mode.
So:
make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true
will now always fail. But this is the main way you'd want to run it
(enabling the leak log catches more stuff, and the log-check function
you touch in patch 2 already covers check mode).
So I think you'd have to hoist your check above the if/else for setting
up PASSING_SANITIZE_LEAK modes.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
From: Jeff King @ 2023-09-12 8:27 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Ævar Arnfjörð Bjarmason
In-Reply-To: <64825651-00d2-d2c5-2da2-91a9917bef21@gmail.com>
On Sun, Sep 10, 2023 at 01:09:52AM +0200, Rubén Justo wrote:
> GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will make the
> test return zero unintentionally:
>
> $ git checkout v2.40.1
> $ make SANITIZE=leak
> $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh
> ...
> With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!
> # faked up failures as TODO & now exiting with 0 due to --invert-exit-code
>
> Let's use invert_exit_code only if needed.
Hmm, OK. So we saw some actual test errors (maybe from leaks or maybe
not), but then we _also_ saw entries in the leak-log. So the inversion
cancels out, and we accidentally say everything is OK, which is wrong.
I'm not quite sure of your fix, though. In the if-else chain you're
touching, we know going in that we found a leak in the log. And then we
cover these 5 cases:
1. if the test is marked as passing-leak
a. if we saw no test failures, invert (and mention the leaking log)
b. otherwise, do not invert (and mention the log)
2. else if we are in "check" mode
a. if we saw no test failures, do not invert (we do have a leak,
which is equivalent to a test failure). Mention the log.
b. otherwise, invert (to switch back to "success", since we are
looking for leaks), but still mention the log.
3. invert to trigger failure (and mention the log)
And the problem is in (3). You switch it to trigger only if we have no
failures (fixing the inversion). But should we have the same a/b split
for this case? I.e.:
3a. if we saw no test failures, invert to cause a failure
3b. we saw other failures; do not invert, but _do_ mention that the
log found extra leaks
In 3b we are explaining to the user what happened. Though maybe it is
not super important, because I think we'd have dumped the log contents
anyway?
Other than that, I think the patch is correct. I wondered when we ran
this "check_test_results_san_file_" code, but it is only at the end of
the script. So we are OK to make a definitive call on the zero/non-zero
count of failed tests.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] diff-merges: introduce '-d' option
From: Sergey Organov @ 2023-09-12 7:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtts0tof8.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> This option provides a shortcut to request diff with respect to first
>> parent for any kind of commit, universally. It's implemented as pure
>> synonym for "--diff-merges=first-parent --patch".
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>
> Sounds very straight-forward.
>
> Given that "--first-parent" in "git log --first-parent -p" already
> defeats "-m" and shows the diff against the first parent only,
> people may find it confusing if "git log -d" does not act as a
> shorthand for that.
It doesn't, and I believe it's a good thing, as primary function of
--first-parent is to change history traversal rules, and if -d did that,
it would be extremely confusing.
Also, --first-parent is correctly documented as implying
--diff-merges=first-parent, not as defeating -m.
> From the above and also from the documentation update, it is hard to
> tell if that is what you implemented, or it only affects the
> "diff-merges" part.
If we read resulting documentation with a fresh eye, -d is similar to
--cc, and -c, just producing yet another kind of output, so I think all
this fits together quite nicely and shouldn't cause confusion.
>
> Other than that, the patch looks quite small and to the point.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH v3 02/32] doc hash-function-transition: Augment compatObjectFormat with readCompatMap
From: Oswald Buddenhagen @ 2023-09-12 7:57 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Junio C Hamano, brian m. carlson, git
In-Reply-To: <87ledcb7ec.fsf_-_@email.froward.int.ebiederm.org>
On Mon, Sep 11, 2023 at 06:46:19PM -0500, Eric W. Biederman wrote:
>+The difference between compatObjectFormat and readCompatMap would be that
>+compatObjectFormat would ask git to read existing maps, but would not ask
>+git to write or create them.
>
the argument makes sense, but the asymmetry in the naming bugs me. in
particular "[read]compatMap" seems too non-descript.
regards
^ permalink raw reply
* Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth
From: Jeff King @ 2023-09-12 7:51 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <cec47733-5b15-6ca7-adaf-7f3216ad178b@web.de>
On Sat, Sep 09, 2023 at 12:28:20AM +0200, René Scharfe wrote:
> >> OPTARG would need a new macro to allow specifying the default value. Or
> >> is there a variadic macro trick that we could use?
> >
> > Hmm, I had just assumed OPTARG was a lost cause (or we would need an
> > "OPTARG" variant of each macro; yuck).
>
> Only for OPT_INTEGER and OPT_STRING AFAICS.
True, my use of BOOL was obviously dumb, as it wouldn't have arguments.
But in theory anything that takes an argument could have an OPTARG
variant. So that would include special stuff like OPT_EXPIRY_DATE,
OPT_FILENAME, and so on. Though I would not be surprised if we currently
only use it for string/integer.
> It's true that a macro that accepts a variable number of arguments would
> accept accidental extra arguments of the right type, but I don't see how
> it would ignore excessive ones.
The macro itself wouldn't notice, but I guess the generated code would
probably complain about getting "(foo,bar)" as the initializer, if you
really sent to many.
But I was more worried about an error where you accidentally give an
extra argument. Right now that's an error, but would it be quietly
shifted into the OPTARG slot?
> > You'd want some semantic check between what's in flags (i.e., is the
> > OPTARG flag set), but I think that's beyond what the compiler itself can
> > do (you could probably write a coccinelle rule for it, though).
>
> Actually I'd want the macro to set that flag for me.
For a dedicated OPT_STRING_OPTARG(), I'd agree. For OPT_STRING() that
uses varargs, I'm more on the fence (because of the cross-checking
above; now we are getting into "accidentally adding a parameter is
quietly accepted" territory).
I dunno. Maybe saving some keystrokes is worth it, but having to say
both OPTARG _and_ provide the extra argument makes things less subtle.
> I was thinking more about something like the solutions discussed at
> https://stackoverflow.com/questions/47674663/variable-arguments-inside-a-macro.
> It allows selecting variants based on argument count.
> [...]
> So OPT_INTEGER(s, l, v, h) would be the same as before. Add an argument
> and it becomes current OPT_INTEGER_F, add another one and it acts as
> your _OPTARG_F variant.
Ah, yeah, I've seen something like this before. I do think it would
work as you're suggesting. I'm just not sure if being verbose and
explicit is better than trying to be clever here.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] diff-merges: improve --diff-merges documentation
From: Sergey Organov @ 2023-09-12 7:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqfs3ktnvo.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> ifdef::git-log[]
>> ---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r)::
>> +-m::
>> + Show diffs for merge commits in the default format. This is
>> + similar to '--diff-merges=on' (which see) except `-m` will
>> + produce no output unless `-p` is given as well.
>> ++
>> +Note: This option not implying `-p` is legacy feature that is
>> +preserved for the sake of backward compatibility.
>
> It is more like that `-p` does not imply `-m` (which used to mean
> "consider showing the comparison between parent(s) and the child,
> even for merge commits"), even though newer options like `-c`,
> `--cc` and others do imply `-m` (simply because they do not make
> much sense if they are not allowed to work on merges) that may make
> new people confused.
No, neither --cc nor -c imply -m.
-m is documented to produce very specific output that is neither -c nor
--cc, and it's indeed how it works.
-c and --cc imply -p, not -m, and it has been documented for ages
already, and it's indeed how it works, and that's what corresponding
commits that added the features claim.
Overall, --cc, -c, and --remerge-diff all imply -p, whereas -m does not.
This is simple fact.
So I feel we need to document why -m doesn't imply -p as other similar
options do.
> If `-p` implied `-m` (or if `-m` implied
> `-p`), it would also have been utterly confusing and useless for
> human consumption.
Fortunately, -p does not imply -m, but if -m implied -p, similar to --cc
and -c, it'd be rather very natural, and thus people keep asking why
it's not the case.
> In either case, unless the reason why `-p` does not imply `-m`
> unlike others is explained, I do not think the note adds that much
> value. I'd suggest dropping it.
-p does not imply others. It's others (--cc, etc.) that imply -p.
The problem being solved is that we periodically get (valid) questions
why -m does not behave similar to -c and --cc, and now --remerge-diff.
>
>> --no-diff-merges::
>> + Synonym for '--diff-merges=off'.
>> +
>> +--diff-merges=<format>::
>> Specify diff format to be used for merge commits. Default is
>> - {diff-merges-default} unless `--first-parent` is in use, in which case
>> - `first-parent` is the default.
>> + {diff-merges-default} unless `--first-parent` is in use, in
>> + which case `first-parent` is the default.
>> +
>> +The following formats are supported:
>> ++
>> +--
>> +off, none::
>> Disable output of diffs for merge commits. Useful to override
>> implied value.
>> +
>> +on, m::
>> + Make diff output for merge commits to be shown in the default
>> + format. The default format could be changed using
>> `log.diffMerges` configuration parameter, which default value
>> is `separate`.
>> +
>> +first-parent, 1::
>> + Show full diff with respect to first parent. This is the same
>> + format as `--patch` produces for non-merge commits.
>> +
>> +separate::
>> + Show full diff with respect to each of parents.
>> + Separate log entry and diff is generated for each parent.
>> +
>> +remerge, r::
>> + Remerge two-parent merge commits to create a temporary tree
>> + object--potentially containing files with conflict markers
>> + and such. A diff is then shown between that temporary tree
>> + and the actual merge commit.
>> +
>> The output emitted when this option is used is subject to change, and
>> so is its interaction with other options (unless explicitly
>> documented).
>> +
>> +combined, c::
>> + Show differences from each of the parents to the merge
>> + result simultaneously instead of showing pairwise diff between
>> + a parent and the result one at a time. Furthermore, it lists
>> + only files which were modified from all parents.
>> +
>> +dense-combined, cc::
>> + Further compress output produced by `--diff-merges=combined`
>> + by omitting uninteresting hunks whose contents in the parents
>> + have only two variants and the merge result picks one of them
>> + without modification.
>> +--
>
> Looks reasonable, even though I didn't quite see much problem with
> the original.
The original --diff-merge=... line was so long it didn't fit, especially
after "remerge" has been added, and also was hard to grok.
> If we were shuffling the sections like this patch, I
> wonder if moving combined/dense-combined a bit higher (perhaps
> before the "remerge") may make more sense, though (the ordering
> would simply become "simpler to more involved").
I kept original order, but I agree combined/dense-combined fit better
above remerge.
I'll change the order in re-roll.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH v2 3/3] test-tool: retire "index-version"
From: Linus Arver @ 2023-09-12 6:10 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <20230818233729.2766281-4-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> As "git update-index --show-index-version" can do the same thing,
> the test-tool lost its reason to exist.
I think you meant
the equivalent functionality in test-tool ...
and not that the test-tool (in its entirety) lost its reason to exist.
The rest of the patch LGTM.
^ permalink raw reply
* Re: [PATCH v2 2/3] update-index: add --show-index-version
From: Linus Arver @ 2023-09-12 5:54 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <20230818233729.2766281-3-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> "git update-index --version N" is used to set the index format
s/--version/--index-version
> version to a specific version, but there was no way to query the
> current version used in the on-disk index file.
>
> Teach the command a new "--show-index-version" option, and also
> teach the "--index-version N" option to report what the version was
> when run with the "--verbose" option.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Documentation/git-update-index.txt | 6 +++++-
> builtin/update-index.c | 19 +++++++++++++------
> t/t2107-update-index-basic.sh | 16 ++++++++++++++++
> 3 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
> index a367f8d65a..3f204891ce 100644
> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -162,7 +162,8 @@ you will need to handle the situation manually.
> Write the resulting index out in the named on-disk format version.
> Supported versions are 2, 3 and 4. The current default version is 2
> or 3, depending on whether extra features are used, such as
> - `git add -N`.
> + `git add -N`. With `--verbose` option, also reports the
How about
`git add -N`. With `--verbose`, also report the
> + version the index file uses before and after this command.
> +
> Version 4 performs a simple pathname compression that reduces index
> size by 30%-50% on large repositories, which results in faster load
> @@ -171,6 +172,9 @@ and support for it was added to libgit2 in 2016 and to JGit in 2020.
> Older editions of this manual page called it "relatively young", but
> it should be considered mature technology these days.
>
> +--show-index-version::
> + Report the index format version used by the on-disk index file.
> + See `--index-version` above.
Looks good.
> -z::
> Only meaningful with `--stdin` or `--index-info`; paths are
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 5fab9ad2ec..e713e2a04c 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1089,6 +1089,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
> resolve_undo_clear_callback),
> OPT_INTEGER(0, "index-version", &preferred_index_format,
> N_("write index in this format")),
> + OPT_SET_INT(0, "show-index-version", &preferred_index_format,
> + N_("show index format version"), -1),
How about
"show on-disk index format version"
for the help text? Also "report" may be preferable instead of "show" for
consistency with the manpage.
Also, I think it would be easier to read if we didn't overload
preferred_index_format here and instead used a new variable (perhaps named
"show_index_format").
And I just realized that we are now using not only "show" and "report"
interchangeably in this patch, but also "format" and "version" ---
it would be nice to clean up as #leftoverbits.
> OPT_BOOL(0, "split-index", &split_index,
> N_("enable or disable split index")),
> OPT_BOOL(0, "untracked-cache", &untracked_cache,
> @@ -1181,15 +1183,20 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>
> getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;
> if (preferred_index_format) {
> - if (preferred_index_format < INDEX_FORMAT_LB ||
> - INDEX_FORMAT_UB < preferred_index_format)
> + if (preferred_index_format < 0) {
> + printf(_("%d\n"), the_index.version);
> + } else if (preferred_index_format < INDEX_FORMAT_LB ||
> + INDEX_FORMAT_UB < preferred_index_format) {
> die("index-version %d not in range: %d..%d",
> preferred_index_format,
> INDEX_FORMAT_LB, INDEX_FORMAT_UB);
> -
> - if (the_index.version != preferred_index_format)
> - the_index.cache_changed |= SOMETHING_CHANGED;
> - the_index.version = preferred_index_format;
> + } else {
> + if (the_index.version != preferred_index_format)
> + the_index.cache_changed |= SOMETHING_CHANGED;
> + report(_("index-version: was %d, set to %d"),
> + the_index.version, preferred_index_format);
> + the_index.version = preferred_index_format;
> + }
> }
Looks good. But if you decide to use a new variable like
"show_index_format" as suggested above, you might want to refactor this
out to a separate function. Then you could obviously do away with the
chained if/else and replace them with early returns, which would be
easier to read.
> if (read_from_stdin) {
> diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
> index 89b285fa3a..c78d1e9396 100755
> --- a/t/t2107-update-index-basic.sh
> +++ b/t/t2107-update-index-basic.sh
> @@ -111,4 +111,20 @@ test_expect_success '--chmod=+x and chmod=-x in the same argument list' '
> test_cmp expect actual
> '
>
> +test_expect_success '--index-version' '
> + git commit --allow-empty -m snap &&
> + git reset --hard &&
Not sure why this "git reset --hard" is needed here --- is it to clear
out state from previous test cases? If so, perhaps it's better to run
this as the very first command in this test case?
But this also makes me wonder why "git commit --allow-empty -m snap" is
even necessary (if we already have a git repo)?
> + git update-index --index-version 2 >actual &&
> + test_must_be_empty actual &&
Before we do "--index-version 2", it may be desirable to run the
"--show-index-version" flag first to see what we have before modifying
it with "2".
> + git update-index --show-index-version >actual &&
> + echo 2 >expect &&
> + test_cmp expect actual &&
> +
> + git update-index --index-version 4 --verbose >actual &&
> + echo "index-version: was 2, set to 4" >expect &&
> + test_cmp expect actual
> +'
How about adding this check below to check what happens if the newly-set
version is the same as the existing version?
git update-index --index-version 4 --verbose >actual &&
echo "index-version: was 4, set to 4" >expect &&
test_cmp expect actual
And also how about a similar check, but for downgrading from a higher
version number to a lower one?
git update-index --index-version 2 --verbose >actual &&
echo "index-version: was 4, set to 2" >expect &&
test_cmp expect actual
> +
> test_done
> --
> 2.42.0-rc2-7-gf9972720e9
^ permalink raw reply
* Re: [PATCH v2 1/3] update-index doc: v4 is OK with JGit and libgit2
From: Linus Arver @ 2023-09-12 4:57 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <20230818233729.2766281-2-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> [...]
> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
> index f4bb9c5daf..a367f8d65a 100644
> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -166,9 +166,11 @@ you will need to handle the situation manually.
> +
> Version 4 performs a simple pathname compression that reduces index
> size by 30%-50% on large repositories, which results in faster load
> -time. Version 4 is relatively young (first released in 1.8.0 in
> -October 2012). Other Git implementations such as JGit and libgit2
> -may not support it yet.
> +time. Git supports it since version 1.8.0, released in October 2012,
> +and support for it was added to libgit2 in 2016 and to JGit in 2020.
> +Older editions of this manual page called it "relatively young", but
Nit: s/editions/versions because typically "edition" means a version of
physically published text (such as books). Also I don't think we use the
word "edition" anywhere else in our codebase.
^ permalink raw reply
* Re: [bug] git clone command leaves orphaned ssh process
From: Jeff King @ 2023-09-12 4:33 UTC (permalink / raw)
To: Aaron Schrab
Cc: Max Amelchenko, Taylor Blau, Bagas Sanjaya, git,
Hideaki Yoshifuji, Junio C Hamano
In-Reply-To: <20230912T004049Z.jiWw7xuK7fiT@pug.qqx.org>
On Mon, Sep 11, 2023 at 08:40:49PM -0400, Aaron Schrab wrote:
> At 13:11 +0300 11 Sep 2023, Max Amelchenko <maxamel2002@gmail.com> wrote:
> > Maybe it's connected also to the underlying infrastructure? We are
> > getting this in AWS lambda jobs and we're hitting a system limit of
> > max processes because of it.
>
> Running as a lambda, or in a container, could definitely be why you're
> seeing a difference. Normally when a process is orphaned it gets adopted by
> `init` (PID 1), and that will take care of cleaning up after orphaned zombie
> processes.
>
> But most of the time containers just run the configured process directly,
> without an init process. That leaves nothing to clean orphan processes.
Yeah, that seems like the culprit. If the clone finishes successfully,
we do end up in finish_connect(), where we wait() for the process. But
if we exit early (in this case, ssh bails and we get EOF on the pipe
reading from it), then we may call die() and exit immediately.
We _could_ take special care to add every spawned process to a global
list, set up handlers via atexit() and signal(), and then reap the
processes. But traditionally it's not a big deal to exit with un-reaped
children, and this is the responsibility of init. I'm not sure it makes
sense for Git to basically reimplement that catch-all (and of course we
cannot even do it reliably if we are killed by certain signals).
> Although for that to really be a problem, would require hitting that max
> process limit inside a single container invocation. Of course since
> containers usually aren't meant to be spawning a lot of processes, that
> limit might be a lot lower than on a normal system.
>
> I know that Docker provides a way to include an init process in the started
> container (`docker run --init`), but I don't think that AWS Lambda does.
I don't know anything about Lambda, but if you are running arbitrary
commands, then it seems like you could insert something like this:
https://github.com/krallin/tini
into the mix. I much prefer that to teaching Git to try to do the same
thing in-process.
-Peff
^ permalink raw reply
* Re: [PATCH] diff --stat: add config option to limit filename width
From: Dragan Simic @ 2023-09-12 2:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqil8gs3s0.fsf@gitster.g>
On 2023-09-12 01:12, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> Add new configuration option diff.statNameWidth=<width> that is
>> equivalent
>> to the command-line option --stat-name-width=<width>, but it is
>> ignored
>> by format-patch. This follows the logic established by the already
>> existing configuration option diff.statGraphWidth=<width>.
>>
>> Limiting the widths of names and graphs in the --stat output makes
>> sense
>> for interactive work on wide terminals with many columns, hence the
>> support
>> for these configuration options. They don't affect format-patch
>> because
>> it already adheres to the traditional 80-column standard.
>>
>> Update the documentation and add more tests to cover new configuration
>> option diff.statNameWidth=<width>. While there, perform a few minor
>> code
>> and whitespace cleanups here and there, as spotted.
>>
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>
> The stat lines have <width> (the entire display width),
> <graph-width> (what appears after '|') and <name-width> (what
> appears before '|'), so I would worry about letting users specify
> all three to contradictory values, if there weren't an existing
> command line option already. But of course there already is a
> command line option, so somebody more clever than me must have
> thought about how to deal with such an impossible settings, and
> adding a configuration variable to specify the same impossible
> settings to the system would not make things worse.
Good point, but we're actually fine with adding diff.statNameWidth as a
new configuration option, because the real troubles with contradictory
configuration values might arise if we ever add diff.statWidth later.
However, we should still be fine at that point, because the code in
diff.c, starting around the line #2730, performs a reasonable amount of
sanity checks and value adjustments.
If we ever add diff.statWidth later, a good thing to do would be to emit
warnings from the above-mentioned code in diff.c if it actually performs
the adjustments, to make the users aware of the contradictory values. I
might even have a look at that separately, if you're fine with adding
such warnings.
>> Documentation/config/diff.txt | 4 ++++
>> Documentation/diff-options.txt | 17 +++++++-------
>> builtin/diff.c | 1 +
>> builtin/log.c | 1 +
>> builtin/merge.c | 1 +
>> builtin/rebase.c | 1 +
>
> Someday, as a follow-up after the dust from this topic settles, we
> would probably want to look at how these rev.diffopt.* members are
> initialized and refactor the common code out to a helper. It would
> allow us to instead of doing this ...
Another good point. If you agree, I'd prefer to have my patch accepted
and merged as-is, after which I'll have a look into unifying the
initialization of the rev.diffopt.* members. Such an approach should,
in general, also be better in case any regressions are detected at some
point in the future.
I'll also have a look into the NEEDSWORK note in diff.c that asks for
using utf8_strnwidth() to calculate the display width of line_prefix. I
already had a brief look at that, and it seems that it leaves enough
space for some rather nice related code cleanups.
>> /* 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;
>> rev.diffopt.flags.allow_external = 1;
>> rev.diffopt.flags.allow_textconv = 1;
>
> ... in many places, do so in a single place in the helper function,
> and these places will just call the helper:
>
> std_graph_options(&rev.diffopt);
>
> I do not know offhand if "stat graph options related members" is a
> good line to draw, or there are other things that are common outside
> these .stat_foo members. If the latter and the helper function will
> initialize the members other than the stat-graph settings, its name
> obviously needs a bit more thought, but you get the idae.
Sure, I'm willing to have a detailed look into all that, as I already
described above.
^ permalink raw reply
* Re: [PATCH v3] diff-lib: Fix check_removed when fsmonitor is on
From: Josip Sokcevic @ 2023-09-12 3:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: jonathantanmy, git, git
In-Reply-To: <xmqqsf7ks4nn.fsf@gitster.g>
On Mon, Sep 11, 2023 at 3:53 PM Junio C Hamano <gitster@pobox.com> wrote:
> This certainly is more "complete" if simpler than the previous one
> ;-)
>
> In the longer term, we would probably want to enable optimization
> using what fsmonitor knows, but as we have seen in the review on
> the previous round, this code needs a bit more work than the
> original we are reverting here to get it right, and in the shorter
> term, hopefully this would do.
Yes, I agree we should optimize this in a follow up. One thing I'm not
sure about is if we should try to construct `struct stat` using
`cache_index`, or we should check for `CE_FSMONITOR_VALID` in a way
that `stat` would no longer be needed for those code paths.
--
Josip Sokcevic
^ permalink raw reply
* Re: [PATCH v3] diff-lib: Fix check_removed when fsmonitor is on
From: Junio C Hamano @ 2023-09-11 22:53 UTC (permalink / raw)
To: Josip Sokcevic; +Cc: jonathantanmy, git, git
In-Reply-To: <20230911170901.49050-2-sokcevic@google.com>
Josip Sokcevic <sokcevic@google.com> writes:
> git-diff-index may return incorrect deleted entries when fsmonitor is used in a
> repository with git submodules. This can be observed on Mac machines, but it
> can affect all other supported platforms too.
>
> If fsmonitor is used, `stat *st` is not initialized if cache_entry has
> CE_FSMONITOR_VALID set. But, there are three call sites that rely on stat
> afterwards, which can result in incorrect results.
>
> This change partially reverts commit 4f3d6d0.
>
> Signed-off-by: Josip Sokcevic <sokcevic@google.com>
> ---
> diff-lib.c | 12 ++++++------
> t/t7527-builtin-fsmonitor.sh | 5 +++++
> 2 files changed, 11 insertions(+), 6 deletions(-)
This certainly is more "complete" if simpler than the previous one
;-)
In the longer term, we would probably want to enable optimization
using what fsmonitor knows, but as we have seen in the review on
the previous round, this code needs a bit more work than the
original we are reverting here to get it right, and in the shorter
term, hopefully this would do.
Thanks.
> diff --git a/diff-lib.c b/diff-lib.c
> index d8aa777a73..5848e4f9ca 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -36,14 +36,14 @@
> * exists for ce that is a submodule -- it is a submodule that is not
> * checked out). Return negative for an error.
> */
> -static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)
> +static int check_removed(const struct cache_entry *ce, struct stat *st)
> {
> - assert(is_fsmonitor_refreshed(istate));
> - if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
> + if (lstat(ce->name, st) < 0) {
> if (!is_missing_file_error(errno))
> return -1;
> return 1;
> }
> +
> if (has_symlink_leading_path(ce->name, ce_namelen(ce)))
> return 1;
> if (S_ISDIR(st->st_mode)) {
> @@ -149,7 +149,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
> memset(&(dpath->parent[0]), 0,
> sizeof(struct combine_diff_parent)*5);
>
> - changed = check_removed(istate, ce, &st);
> + changed = check_removed(ce, &st);
> if (!changed)
> wt_mode = ce_mode_from_stat(ce, st.st_mode);
> else {
> @@ -229,7 +229,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
> } else {
> struct stat st;
>
> - changed = check_removed(istate, ce, &st);
> + changed = check_removed(ce, &st);
> if (changed) {
> if (changed < 0) {
> perror(ce->name);
> @@ -303,7 +303,7 @@ static int get_stat_data(const struct index_state *istate,
> if (!cached && !ce_uptodate(ce)) {
> int changed;
> struct stat st;
> - changed = check_removed(istate, ce, &st);
> + changed = check_removed(ce, &st);
> if (changed < 0)
> return -1;
> else if (changed) {
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
> index 0c241d6c14..78503158fd 100755
> --- a/t/t7527-builtin-fsmonitor.sh
> +++ b/t/t7527-builtin-fsmonitor.sh
> @@ -809,6 +809,11 @@ my_match_and_clean () {
> status --porcelain=v2 >actual.without &&
> test_cmp actual.with actual.without &&
>
> + git -C super --no-optional-locks diff-index --name-status HEAD >actual.with &&
> + git -C super --no-optional-locks -c core.fsmonitor=false \
> + diff-index --name-status HEAD >actual.without &&
> + test_cmp actual.with actual.without &&
> +
> git -C super/dir_1/dir_2/sub reset --hard &&
> git -C super/dir_1/dir_2/sub clean -d -f
> }
^ permalink raw reply
* Re: [PATCH] start_command: reset disposition of all signals in child
From: Junio C Hamano @ 2023-09-11 22:20 UTC (permalink / raw)
To: Phillip Wood; +Cc: Phillip Wood via GitGitGadget, git, Eric Wong, Phillip Wood
In-Reply-To: <c64cd85f-f14c-46b7-a0d3-b8e0bfc60053@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Preserve the set of ignored signals so that running git via a
> wrapper like nohup works as the user expects
OK.
> The other thing is that we have some instances where we ignore SIGPIPE
> before calling start_command() which means we're ignoring it in the
> child process as well. For example in gpg-interface.c we have
> ...
> rev-list does not check for errors when writing to stdout unless
> GIT_FLUSH is set in the environment so if parent process exits early
> rev-list will keep going until it thinks it has printed everything.
Ahh, yeah, that is bad. Thanks for pointing them out.
> I think adding a flag to struct child_process to ignore SIGPIPE in the
> parent is probably the best way to avoid problems like this.
OK. SIGPIPE is special enough that it may deserve to be singled
out. A pager reading from us quitting does not have to be "we
wanted to write more but got an error" but just "ok, if the user
does not want any more output, that is fine".
THanks.
^ permalink raw reply
* [RFC PATCH] Not computing changed path filter for root commits
From: Jonathan Tan @ 2023-09-11 22:31 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, szeder.dev, me, derrickstolee
This is following the discussion about adding a new changed path filter
version due to the current implementation of murmur3 not matching the
algorithm. [1]
SZEDER Gábor suggested [2] that we change the revision walk to read
changed path filters also for root commits, but I don't think that's
possible - we have to tie reading changed path filters to when we read
trees, and right now, we don't seem to read trees when evaluating root
commits (rev_compare_tree() in revision.c is in the only code path that
uses changed path filters, and it itself is only called per-parent and
thus not called for root commits). The alternative is to not generate
changed path filters for root commits (or what I did in this patch,
which is to generate an all-1 filter), which seems reasonable to me.
Is this a good idea? If yes, there is the follow-up question of how to
report it in traces. I don't know off-hand if it's better to reuse the
"large" statistic, since what we output is the same (all 1s), or if we
should make a new one. Presumably what we need to weigh is the clarity
versus the migration costs, but I don't know how to weigh it.
If people are generally in agreement, I can send an updated patch that
does not goto into an "else" block :) and also with updated tests (t0095
needs to check a non-root commit, and t4216 needs to be updated with
whatever statistics we decide to use).
[1] https://lore.kernel.org/git/20230826150610.GA1928@szeder.dev/
[2] https://lore.kernel.org/git/20230830200218.GA5147@szeder.dev/
---
bloom.c | 3 ++-
t/t0095-bloom.sh | 2 +-
t/t4216-log-bloom.sh | 12 +++---------
3 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/bloom.c b/bloom.c
index aef6b5fea2..b21130b236 100644
--- a/bloom.c
+++ b/bloom.c
@@ -226,7 +226,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
if (c->parents)
diff_tree_oid(&c->parents->item->object.oid, &c->object.oid, "", &diffopt);
else
- diff_tree_oid(NULL, &c->object.oid, "", &diffopt);
+ goto large;
diffcore_std(&diffopt);
if (diff_queued_diff.nr <= settings->max_changed_paths) {
@@ -292,6 +292,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
} else {
for (i = 0; i < diff_queued_diff.nr; i++)
diff_free_filepair(diff_queued_diff.queue[i]);
+large:
init_truncated_large_filter(filter);
if (computed)
diff --git a/t/t0095-bloom.sh b/t/t0095-bloom.sh
index b567383eb8..02a0b41026 100755
--- a/t/t0095-bloom.sh
+++ b/t/t0095-bloom.sh
@@ -74,7 +74,7 @@ test_expect_success !SANITIZE_LEAK 'get bloom filters for commit with no changes
git commit --allow-empty -m "c0" &&
cat >expect <<-\EOF &&
Filter_Length:1
- Filter_Data:00|
+ Filter_Data:ff|
EOF
test-tool bloom get_filter_for_commit "$(git rev-parse HEAD)" >actual &&
test_cmp expect actual
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index fa9d32facf..d14fe93fb1 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -283,7 +283,6 @@ test_expect_success 'correctly report changes over limit' '
git commit-graph write --reachable --changed-paths &&
test_max_changed_paths 11 trace-update &&
test_filter_computed 2 trace-update &&
- test_filter_trunc_large 0 trace-update &&
for path in $(git ls-tree -r --name-only HEAD)
do
@@ -306,9 +305,7 @@ test_expect_success 'correctly report commits with no changed paths' '
GIT_TRACE2_EVENT="$(pwd)/trace.event" \
git commit-graph write --reachable --changed-paths &&
test_filter_computed 1 trace.event &&
- test_filter_not_computed 0 trace.event &&
- test_filter_trunc_empty 1 trace.event &&
- test_filter_trunc_large 0 trace.event
+ test_filter_not_computed 0 trace.event
)
'
@@ -363,8 +360,7 @@ test_expect_success '--max-new-filters overrides configuration' '
--max-new-filters=1 &&
test_filter_computed 1 trace.event &&
test_filter_not_computed 1 trace.event &&
- test_filter_trunc_empty 0 trace.event &&
- test_filter_trunc_large 0 trace.event
+ test_filter_trunc_empty 0 trace.event
)
'
@@ -386,9 +382,7 @@ test_expect_success 'Bloom generation backfills empty commits' '
git commit-graph write --reachable \
--changed-paths --max-new-filters=2 &&
test_filter_computed 2 trace.event &&
- test_filter_not_computed 4 trace.event &&
- test_filter_trunc_empty 2 trace.event &&
- test_filter_trunc_large 0 trace.event || return 1
+ test_filter_not_computed 4 trace.event || return 1
done &&
# Finally, make sure that once all commits have filters, that
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* Re: [RFC] New configuration option "diff.statNameWidth"
From: Junio C Hamano @ 2023-09-11 23:02 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <261907dbfafc8d7b3b048854b663e82d@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> On 2023-09-03 05:43, Dragan Simic wrote:
>> On 2023-09-03 00:16, Junio C Hamano wrote:
>>> Having said all the above, once we start seeing the actual patches
>>> and its sales pitch (aka proposed commit log message), we do guide
>>> and help polishing the patch into a better shape, so it will not
>>> be like the contributor has to work in the dark without knowing
>>> what level of bar their contribution has to pass.
>> Thanks, everything sounds great and welcoming to the new
>> contributors,
>> who of course need to be willing to put in the required amount of
>> skill and effort.
>
> I sent a patch to the git mailing list today, about five hours ago,
> but it hasn't appeared on the list yet. Could something be wrong with
> the mail server(s), as I also received no other messages from the list
> in the last six hours or so?
It is a high-volume mailing list server and occasional hiccup is
part of regular life. Waiting a bit and then poking the postmater
at vger.kernel.org may be needed from time to time, but I am seeing
your message on the archive, so it seems "waiting" has worked fine?
^ permalink raw reply
* Re: [RFC][PATCH 0/32] SHA256 and SHA1 interoperability
From: brian m. carlson @ 2023-09-11 22:05 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Junio C Hamano, git
In-Reply-To: <87cyyoeli0.fsf@email.froward.int.ebiederm.org>
[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]
On 2023-09-11 at 16:13:27, Eric W. Biederman wrote:
> Junio, brian for the very warm reception of this, it is very
> encouraging.
>
> I am not worried about what it will take time to get the changes I
> posted into the integration. I had only envisioned them as good enough
> to get the technical ideas across, and had never envisioned them as
> being accepted as is.
>
> What I am envisioning as my future directions are:
>
> - Post non controversial cleanups, so they can be merged.
> (I can only see about 4 of them the most significant is:
> bulk-checkin: Only accept blobs)
>
> - Sort out the configuration options
>
> - Post the smallest patchset I can that will allow testing the code in
> object-file-convert.c. Unfortunately for that I need configuration
> options to enable the mapping.
>
> In starting to write the tests I have already found a bug in
> the conversion of tags (an extra newline is added), and I haven't
> even gotten to testing the tricky bits with signatures.
I wonder if unit tests are a possibility here now that we're starting to
use them. They're not obligatory, of course, but it may be more
convenient for you if they turn out to be a suitable option. If not, no
big deal.
> - Once the object file conversion is tested and is solid work on
> the more substantial pieces.
>
> Does that sound like a reasonable plan?
Yeah, that seems fine.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: Oleg Nesterov @ 2023-09-11 23:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Calvin Wan,
Carlo Marcelo Arenas Belón, Elijah Newren, Jeff King,
Linus Torvalds, Mathias Krause, René Scharfe, Taylor Blau,
git
In-Reply-To: <xmqq34zktk4h.fsf@gitster.g>
On 09/11, Junio C Hamano wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > $ git grep --untracked -pn xxx TEST.c
> >
> > before the patch:
> >
> > TEST.c=1=void func(void);
> > TEST.c:3:void func1(xxx)
> > TEST.c:5: use1(xxx);
> > TEST.c:8:void func2(xxx)
> > TEST.c:10: use2(xxx);
> >
> > after the patch:
> >
> > TEST.c=1=void func(void);
> > TEST.c:3:void func1(xxx)
> > TEST.c=3=void func1(xxx)
> > TEST.c:5: use1(xxx);
> > TEST.c:8:void func2(xxx)
> > TEST.c=8=void func2(xxx)
> > TEST.c:10: use2(xxx);
> >
> > which looks much better to me.
>
> The "better" is often subjective.
Sure. that is why I added "to me".
> The former is showing what is
> going on in the TEST.c code very clearly without wasting valuable
> vertical screen real estate, at least to me.
very clearly? As you probably understand this is subjective as well.
But yes, you too added "at least to me" ;)
However, certainly this is not true when you use git-grep in scripts,
please see 0/1.
> If we want to adopt
> the proposed behaviour, which I would recommend against, the same
> patch should update the documentation, which currently says
>
> Show the preceding line that contains the function name of the
> match, unless the matching line is a function name itself.
And I still don't think this patch changes the documented behaviour.
See my reply to Rene.
Again, if you do
./git grep -pn --untracked func1 TEST.c
with this patch applied, the output is still
TEST.c=1=void func(void);
TEST.c:3:void func1(xxx)
which iiuc matches the documentation above.
Now,
./git grep -pn --untracked xxx TEST.c
adds the additional
TEST.c=3=void func1(xxx)
...
TEST.c=8=void func2(xxx)
but how does this contradict with the documentation above?
the matching lines are use1(xxx) and use2(xxx), there are NOT
"the matching line is a function name itself".
> As René said, I think -p/--show-function is a rather less used
> option in modern Git where "--function-context", which back in
> 2944e4e6 did not exist, tend to be a much more useful option,
Well, not to me. And you know, I am a git user too ;)
> but it still is a backward
> incompatible behaviour change that needs to be handled with care.
And this is what I still don't understand.
> Thanks.
Thanks,
Oleg.
^ permalink raw reply
* Re: [PATCH] t3404-rebase-interactive.sh: fix name of a rewording test
From: Junio C Hamano @ 2023-09-11 22:36 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Phillip Wood
In-Reply-To: <20230911122108.199207-1-oswald.buddenhagen@gmx.de>
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> The given test name made no sense to me at all; it seems to be a
> concatenation of two unrelated things. This was introduced by
> commit 0c164ae7a ("rebase -i: add another reword test", 20-08-20).
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> ---
> actually, i don't understand what the test even does. shouldn't it, to
> match the description, actually dirty the tree and verify that the
> operation fails?
>
> Cc: Phillip Wood <phillip.wood@dunelm.org.uk>
As a typofix the updated text looks more correct, but the above is a
very good point, so I'll let Phillip, whose 0c164ae7 (rebase -i: add
another reword test, 2021-08-20) introduced this test, react first.
Thanks for spotting and fixing.
> ---
> t/t3404-rebase-interactive.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 96a56aafbe..31ee5bc1f6 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -758,7 +758,7 @@ test_expect_success 'reword' '
> git show HEAD~2 | grep "C changed"
> '
>
> -test_expect_success 'no uncommited changes when rewording the todo list is reloaded' '
> +test_expect_success 'no uncommitted changes when rewording' '
> git checkout E &&
> test_when_finished "git checkout @{-1}" &&
> (
^ permalink raw reply
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
From: Oswald Buddenhagen @ 2023-09-11 22:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, René Scharfe, Git List, Jeff King
In-Reply-To: <xmqq7cowv7pm.fsf@gitster.g>
On Mon, Sep 11, 2023 at 12:19:17PM -0700, Junio C Hamano wrote:
>Yup, that does cross my mind, even though I would have used
>
> union {
> void *void_ptr;
> int *int_ptr;
> } value;
>
>or something without a rather meaningless 'u'.
>
i'd go the opposite way and make it an anonymous union.
that would require c11, though. imo not exactly an outrageous
proposition in 2023, but ...
regards
^ permalink raw reply
* [PATCH v3 02/32] doc hash-function-transition: Augment compatObjectFormat with readCompatMap
From: Eric W. Biederman @ 2023-09-11 23:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: brian m. carlson, git
In-Reply-To: <87sf7kd5xg.fsf_-_@email.froward.int.ebiederm.org>
Deeply and fundamentally the plan is to only operate one one hash
function for the core of git, to use only one hash function for what
is stored in the repository.
To avoid requring a flag day to transition hash functions for naming
objects, and to support being able to access objects using legacy object
names a mapping functionality will be provided.
We want to provide user facing configuration that is robust enough
that it can accomodate multiple different scenarios on how git
evolves and how people use their repositories.
There are two different ways it is envisioned to use mapped object
ids. The first is to require every object in the repository to have a
mapping, so that pushes and pulls from repositories using a different
hash algorithm can work. The second is to have an incomplete mapping
of object ids so that old references to objects in emails, commit
messages, bug trackers and are usable in a read-only manner
with tools like "git show".
The first way fundamentally needs every object in the repository to
have a mapping, which requires the repository to be marked incompatible
for writes fron older versions of git. Thus the compatObjectFormat option
is placed in [extensions].
The ext2 family of filesystems has 3 ways of describing new features
compatible, read-only-compatible, and incompatible. The current git
configurtation has compat (any feature mentioned anywhere in the
configuration outside of [extensions] section), and incompatible (any
configuration inside of the [extensions] section. It would be nice to
have a read-only compatible section for the mandatory mapping
function. Would it be worth adding it now so that we have it for
future extensions?
Having a mapping that is just used in a read-only mode for looking up
old objects with old object ids will be needed post-transition. Such
a mode does not require computing the old hash function or even
support automatically writing any new mappings. So it is completely
safe to enable in a backwards compatible mode. Fort that let's
use core.readCompatMap to make it clear the mappings only read.
I have documented that both of the options readCompatMap and
compatObjectFormat can be specified multiple times if that is needed to
support the desired configuration of git.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
My v2 version was just silly. Changing the name of the option in
the [extensions] section made practical sense. It was just me being
contrary for no good reason. I still think we should have an additional
option for reading old hashes and to document that we expect multiple of
these.
So here is my proposal for extending the documentation along those
lines.
Additionally just accepting the existing option name means I am not
bottlenecked for writing tests convert_object_file which is the
important part right now.
My apologies for all of the noise.
.../technical/hash-function-transition.txt | 37 +++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/Documentation/technical/hash-function-transition.txt b/Documentation/technical/hash-function-transition.txt
index 4b937480848a..26dfc3138b3b 100644
--- a/Documentation/technical/hash-function-transition.txt
+++ b/Documentation/technical/hash-function-transition.txt
@@ -171,6 +171,43 @@ repository, instead producing an error message.
objectformat
compatobjectformat
+Configurate for a future hash function transition would be:
+
+ [core]
+ repositoryFormatVersion = 1
+ [extensions]
+ objectFormat = futureHash
+ compatObjectFormat = sha256
+ compatObjectFormat = sha1
+
+Or possibly:
+
+ [core]
+ repositoryFormatVersion = 1
+ readCompatMap = sha1
+ [extensions]
+ objectFormat = futureHash
+ compatObjectFormat = sha256
+
+Or post transition to futureHash:
+
+ [core]
+ repositoryFormatVersion = 1
+ readCompatMap = sha1
+ readComaptMap = sha256
+ [extensions]
+ objectFormat = futureHash
+
+The difference between compatObjectFormat and readCompatMap would be that
+compatObjectFormat would ask git to read existing maps, but would not ask
+git to write or create them. Which is enough to support looking up
+old oids post transition, when they are only needed to support
+references in commit logs, bug trackers, emails and the like.
+
+Meanwhile with compatObjectFormat set every object in the entire
+repository would be required to have a bi-directional mapping from the
+the mapped object format to the repositories storage hash function.
+
See the "Transition plan" section below for more details on these
repository extensions.
--
2.41.0
^ permalink raw reply related
* Re: [PATCH 2/2] diff-merges: introduce '-d' option
From: Junio C Hamano @ 2023-09-11 21:01 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <20230909125446.142715-3-sorganov@gmail.com>
Sergey Organov <sorganov@gmail.com> writes:
> This option provides a shortcut to request diff with respect to first
> parent for any kind of commit, universally. It's implemented as pure
> synonym for "--diff-merges=first-parent --patch".
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
Sounds very straight-forward.
Given that "--first-parent" in "git log --first-parent -p" already
defeats "-m" and shows the diff against the first parent only,
people may find it confusing if "git log -d" does not act as a
shorthand for that. From the above and also from the documentation
update, it is hard to tell if that is what you implemented, or it
only affects the "diff-merges" part.
Other than that, the patch looks quite small and to the point.
Thanks.
> Documentation/diff-options.txt | 4 ++++
> Documentation/git-log.txt | 2 +-
> diff-merges.c | 3 +++
> t/t4013-diff-various.sh | 8 ++++++++
> 4 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index f93aa3e46a52..d773dafcb10a 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -51,6 +51,10 @@ ifdef::git-log[]
> Note: This option not implying `-p` is legacy feature that is
> preserved for the sake of backward compatibility.
>
> +-d::
> + Produce diff with respect to first parent.
> + Shortcut for '--diff-merges=first-parent -p'.
> +
> -c::
> Produce combined diff output for merge commits.
> Shortcut for '--diff-merges=combined -p'.
> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
> index 9b7ec96e767a..59bd74a1a596 100644
> --- a/Documentation/git-log.txt
> +++ b/Documentation/git-log.txt
> @@ -120,7 +120,7 @@ By default, `git log` does not generate any diff output. The options
> below can be used to show the changes made by each commit.
>
> Note that unless one of `--diff-merges` variants (including short
> -`-m`, `-c`, and `--cc` options) is explicitly given, merge commits
> +`-d`, `-m`, `-c`, and `--cc` options) is explicitly given, merge commits
> will not show a diff, even if a diff format like `--patch` is
> selected, nor will they match search options like `-S`. The exception
> is when `--first-parent` is in use, in which case `first-parent` is
> diff --git a/diff-merges.c b/diff-merges.c
> index ec97616db1df..6eb72e6fc28a 100644
> --- a/diff-merges.c
> +++ b/diff-merges.c
> @@ -125,6 +125,9 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
> if (!suppress_m_parsing && !strcmp(arg, "-m")) {
> set_to_default(revs);
> revs->merges_need_diff = 0;
> + } else if (!strcmp(arg, "-d")) {
> + set_first_parent(revs);
> + revs->merges_imply_patch = 1;
> } else if (!strcmp(arg, "-c")) {
> set_combined(revs);
> revs->merges_imply_patch = 1;
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 5de1d190759f..a07d6eb6dd97 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -473,6 +473,14 @@ test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
> test_cmp expected actual
> '
>
> +test_expect_success 'log -d matches --diff-merges=1 -p' '
> + git log --diff-merges=1 -p master >result &&
> + process_diffs result >expected &&
> + git log -d master >result &&
> + process_diffs result >actual &&
> + test_cmp expected actual
> +'
> +
> test_expect_success 'deny wrong log.diffMerges config' '
> test_config log.diffMerges wrong-value &&
> test_expect_code 128 git log
^ permalink raw reply
* Re: [PATCH] diff --stat: add config option to limit filename width
From: Junio C Hamano @ 2023-09-11 23:12 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <87badb12f040d1c66cd9b89074d3de5015a45983.1694446743.git.dsimic@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> Add new configuration option diff.statNameWidth=<width> that is equivalent
> to the command-line option --stat-name-width=<width>, but it is ignored
> by format-patch. This follows the logic established by the already
> existing configuration option diff.statGraphWidth=<width>.
>
> Limiting the widths of names and graphs in the --stat output makes sense
> for interactive work on wide terminals with many columns, hence the support
> for these configuration options. They don't affect format-patch because
> it already adheres to the traditional 80-column standard.
>
> Update the documentation and add more tests to cover new configuration
> option diff.statNameWidth=<width>. While there, perform a few minor code
> and whitespace cleanups here and there, as spotted.
>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
The stat lines have <width> (the entire display width),
<graph-width> (what appears after '|') and <name-width> (what
appears before '|'), so I would worry about letting users specify
all three to contradictory values, if there weren't an existing
command line option already. But of course there already is a
command line option, so somebody more clever than me must have
thought about how to deal with such an impossible settings, and
adding a configuration variable to specify the same impossible
settings to the system would not make things worse.
> Documentation/config/diff.txt | 4 ++++
> Documentation/diff-options.txt | 17 +++++++-------
> builtin/diff.c | 1 +
> builtin/log.c | 1 +
> builtin/merge.c | 1 +
> builtin/rebase.c | 1 +
Someday, as a follow-up after the dust from this topic settles, we
would probably want to look at how these rev.diffopt.* members are
initialized and refactor the common code out to a helper. It would
allow us to instead of doing this ...
> /* 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;
> rev.diffopt.flags.allow_external = 1;
> rev.diffopt.flags.allow_textconv = 1;
... in many places, do so in a single place in the helper function,
and these places will just call the helper:
std_graph_options(&rev.diffopt);
I do not know offhand if "stat graph options related members" is a
good line to draw, or there are other things that are common outside
these .stat_foo members. If the latter and the helper function will
initialize the members other than the stat-graph settings, its name
obviously needs a bit more thought, but you get the idae.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: Junio C Hamano @ 2023-09-11 22:34 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ævar Arnfjörð Bjarmason, Calvin Wan,
Carlo Marcelo Arenas Belón, Elijah Newren, Jeff King,
Linus Torvalds, Mathias Krause, René Scharfe, Taylor Blau,
git
In-Reply-To: <20230911121211.GA17401@redhat.com>
Oleg Nesterov <oleg@redhat.com> writes:
> $ git grep --untracked -pn xxx TEST.c
>
> before the patch:
>
> TEST.c=1=void func(void);
> TEST.c:3:void func1(xxx)
> TEST.c:5: use1(xxx);
> TEST.c:8:void func2(xxx)
> TEST.c:10: use2(xxx);
>
> after the patch:
>
> TEST.c=1=void func(void);
> TEST.c:3:void func1(xxx)
> TEST.c=3=void func1(xxx)
> TEST.c:5: use1(xxx);
> TEST.c:8:void func2(xxx)
> TEST.c=8=void func2(xxx)
> TEST.c:10: use2(xxx);
>
> which looks much better to me.
The "better" is often subjective. The former is showing what is
going on in the TEST.c code very clearly without wasting valuable
vertical screen real estate, at least to me. If we want to adopt
the proposed behaviour, which I would recommend against, the same
patch should update the documentation, which currently says
Show the preceding line that contains the function name of the
match, unless the matching line is a function name itself.
which has allowed the users to depend on the current behaviour for
practically forever since the feature was introduced by 2944e4e6
(grep: add option -p/--show-function, 2009-07-02).
As René said, I think -p/--show-function is a rather less used
option in modern Git where "--function-context", which back in
2944e4e6 did not exist, tend to be a much more useful option, so the
fallout from such a change may be small, but it still is a backward
incompatible behaviour change that needs to be handled with care.
Thanks.
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> grep.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 0904d55b24..7cad8352f4 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -1350,12 +1350,11 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
> while (bol > gs->buf) {
> const char *eol = --bol;
>
> + if (--lno < opt->last_shown)
> + break;
> +
> while (bol > gs->buf && bol[-1] != '\n')
> bol--;
> - lno--;
> -
> - if (lno <= opt->last_shown)
> - break;
>
> if (match_funcname(opt, gs, bol, eol)) {
> show_line(opt, bol, eol, gs->name, lno, 0, '=');
^ permalink raw reply
* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: Oleg Nesterov @ 2023-09-11 21:54 UTC (permalink / raw)
To: René Scharfe
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Calvin Wan, Carlo Marcelo Arenas Belón, Elijah Newren,
Jeff King, Linus Torvalds, Mathias Krause, Taylor Blau, git
In-Reply-To: <e214eb9c-7576-f8f5-ef1d-3828affd47d8@web.de>
Hi René,
Thanks for feedback. I am already sleeping but let me try to reply anyway,
even if I don't really understand you.
On 09/11, René Scharfe wrote:
>
> Am 11.09.23 um 14:12 schrieb Oleg Nesterov:
> > show_funcname_line() returns when "lno <= opt->last_shown" and this
> > is not right in that the ->last_shown line (which matched the pattern)
> > can also have the actual function name we need to report.
> >
> > Change this code to check "lno < opt->last_shown". While at it, move
> > this check up to avoid the unnecessary "find the previous bol" loop.
> >
> > Note that --lno can't underflow, lno==0 is not possible in this loop.
> >
> > Simple test-case:
> >
> > $ cat TEST.c
> > void func(void);
> >
> > void func1(xxx)
> > {
> > use1(xxx);
> > }
> >
> > void func2(xxx)
> > {
> > use2(xxx);
> > }
> >
> > $ git grep --untracked -pn xxx TEST.c
> >
> > before the patch:
> >
> > TEST.c=1=void func(void);
> > TEST.c:3:void func1(xxx)
> > TEST.c:5: use1(xxx);
> > TEST.c:8:void func2(xxx)
> > TEST.c:10: use2(xxx);
> >
> > after the patch:
> >
> > TEST.c=1=void func(void);
> > TEST.c:3:void func1(xxx)
> > TEST.c=3=void func1(xxx)
> > TEST.c:5: use1(xxx);
> > TEST.c:8:void func2(xxx)
> > TEST.c=8=void func2(xxx)
> > TEST.c:10: use2(xxx);
> >
> > which looks much better to me.
>
> Interesting idea to treat function lines as annotations of matches
> instead of as special context.
Sorry, I don't understand... Let me repeat I am not familiar with this
code and terminology. Could you spell please?
> Showing lines twice feels wasteful, but
> at least for -p it might be justifiable from that angle.
Just in case... say, "func1" is reported twice only when it is really
needed. From the "after the patch" output above:
TEST.c:3:void func1(xxx)
this is what we already have without this patch
TEST.c=3=void func1(xxx)
this is what we have with this patch because the next
TEST.c:5: use1(xxx);
line needs the proper funcname line, and without this patch it would be
"void func()" which has nothing to do with use1(xxx),
If I do, say,
./git grep --untracked -pn func1 TEST.c
then (with or without this patch) the output is
TEST.c=1=void func(void);
TEST.c:3:void func1(xxx)
in this case there is no reason to report "=void func1(xxx)".
> Wouldn't you
> have to repeat function line 3 before the match in line 8, though?
Why?
> The reason for not repeating a matched function line was that it
> doesn't add much information under the assumption that it's easy to
> identify function lines visually.
But it is not. Lets look again at the "before the patch:" output above,
TEST.c=1=void func(void);
TEST.c:3:void func1(xxx)
TEST.c:5: use1(xxx);
TEST.c:8:void func2(xxx)
TEST.c:10: use2(xxx);
it looks as if every "xxx" match is inside the (unrelated) func().
OK, "visually" you can also notice the "void funcX(xxx)" declarations
and understand whats going on.
But a) I don't think this is always easy, and b) it is certainly not
easy when you use "git-grep -p" in scripts. Please see 0/1.
> The patch would need to update Documentation/git-grep.txt as well to
> reflect the changed output.
Hmm... From Documentation/git-grep.txt:
-p::
--show-function::
Show the preceding line that contains the function name of
the match, unless the matching line is a function name itself.
...
this is still true after this patch. How do you think I should update this
section?
Oleg.
^ 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