* Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions
From: Patrick Steinhardt @ 2024-01-23 15:18 UTC (permalink / raw)
To: Toon Claes; +Cc: git, Taylor Blau, Eric Sunshine
In-Reply-To: <87a5owqgzi.fsf@iotcl.com>
[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]
On Tue, Jan 23, 2024 at 03:08:00PM +0100, Toon Claes wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > [[PGP Signed Part:Undecided]]
> > [1. text/plain]
> > In t1302 we exercise logic around "core.repositoryFormatVersion" and
> > extensions. These tests are not particularly robust against extensions
> > like the newly introduced "refStorage" extension.
> >
> > Refactor the tests to be more robust:
> >
> > - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
> > repository format version. This helps to ensure that we only need to
> > update the prereq in a central place when new extensions are added.
> >
> > - Use a separate repository to rewrite ".git/config" to test
> > combinations of the repository format version and extensions. This
> > ensures that we don't break the main test repository.
> >
> > - Do not rewrite ".git/config" when exercising the "preciousObjects"
> > extension.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > t/t1302-repo-version.sh | 19 +++++++++++++++----
> > 1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
> > index 179474fa65..7c680c91c4 100755
> > --- a/t/t1302-repo-version.sh
> > +++ b/t/t1302-repo-version.sh
> > @@ -79,8 +84,13 @@ mkconfig () {
> >
> > while read outcome version extensions; do
> > test_expect_success "$outcome version=$version $extensions" "
> > - mkconfig $version $extensions >.git/config &&
> > - check_${outcome}
> > + test_when_finished 'rm -rf extensions' &&
> > + git init extensions &&
> > + (
> > + cd extensions &&
> > + mkconfig $version $extensions >.git/config &&
>
> Why did you not remove the use of `mkconfig` here?
I guess you mean stop using `mkconfig` and instead use git-config(1) to
manually write only the repository format version as well as extensions?
The problem here is that the resulting configuration would be dependent
on some environment variables, like `GIT_TEST_DEFAULT_HASH` and the
refdb equivalent `GIT_TEST_DEFAULT_REF_FORMAT` which do end up in the
repo's configuration. So I think it's actually preferable to overwrite
the complete ".git/config" so that is exactly in the expected state.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Sergey Organov @ 2024-01-23 15:10 UTC (permalink / raw)
To: Elijah Newren; +Cc: Junio C Hamano, git
In-Reply-To: <CABPp-BHUVLS4vB5maZzU5gS33ve6LkKgij+rc1bBZges6Xej-g@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> On Tue, Jan 9, 2024 at 12:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> I think the current code makes "-n" take precedence, and ignores
>> "-f".
>
> :-(
>
>> Shouldn't it either
>>
>> (1) error out with "-n and -f cannot be used together", or
>> (2) let "-n" and "-f" follow the usual "last one wins" rule?
>
> I believe so.
Then how does one figure what "git clean -f -f" will do without actually
doing it?
Please notice that -f -f is special according to the manual:
"Git will refuse to modify untracked nested git repositories
(directories with a .git subdirectory) unless a second -f is given."
I looks like neither (0), nor (1) nor (2) gives us any useful behavior
in this case.
I figure the best solution is to rather make -n orthogonal to -f, that
will solve the puzzle, and that is what actually expected from a "dry
run" option: don't change any behavior, except print actions instead of
performing them.
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Phillip Wood @ 2024-01-23 14:25 UTC (permalink / raw)
To: brianmlyles, git; +Cc: me, newren
In-Reply-To: <20240119060721.3734775-5-brianmlyles@gmail.com>
Hi Brian
On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
> From: Brian Lyles <brianmlyles@gmail.com>
>
> As with `git-rebase` and `git-am`, `git-cherry-pick` can result in a
> commit being made redundant if the content from the picked commit is
> already present in the target history. However, `git-cherry-pick` does
> not have the same options available that `git-rebase` and `git-am` have.
>
> There are three things that can be done with these redundant commits:
> drop them, keep them, or have the cherry-pick stop and wait for the user
> to take an action. `git-rebase` has the `--empty` option added in commit
> e98c4269c8 (rebase (interactive-backend): fix handling of commits that
> become empty, 2020-02-15), which handles all three of these scenarios.
> Similarly, `git-am` got its own `--empty` in 7c096b8d61 (am: support
> --empty=<option> to handle empty patches, 2021-12-09).
>
> `git-cherry-pick`, on the other hand, only supports two of the three
> possiblities: Keep the redundant commits via `--keep-redundant-commits`,
> or have the cherry-pick fail by not specifying that option. There is no
> way to automatically drop redundant commits.
>
> In order to bring `git-cherry-pick` more in-line with `git-rebase` and
> `git-am`, this commit adds an `--empty` option to `git-cherry-pick`. It
> has the same three options (keep, drop, and stop), and largely behaves
> the same. The notable difference is that for `git-cherry-pick`, the
> default will be `stop`, which maintains the current behavior when the
> option is not specified.
Thanks for the well explained commit message
> The `--keep-redundant-commits` option will be documented as a deprecated
> synonym of `--empty=keep`, and will be supported for backwards
> compatibility for the time being.
I'm not sure if we need to deprecate it as in "it will be removed in the
future" or just reduce it prominence in favor of --empty
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> ---
> Documentation/git-cherry-pick.txt | 28 ++++++++++++++++++-------
> builtin/revert.c | 35 ++++++++++++++++++++++++++++++-
> sequencer.c | 6 ++++++
> t/t3505-cherry-pick-empty.sh | 26 ++++++++++++++++++++++-
> 4 files changed, 86 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index 806295a730..8c20a10d4b 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -132,23 +132,37 @@ effect to your index in a row.
> keeps commits that were initially empty (i.e. the commit recorded the
> same tree as its parent). Commits which are made empty due to a
> previous commit will cause the cherry-pick to fail. To force the
> - inclusion of those commits use `--keep-redundant-commits`.
> + inclusion of those commits use `--empty=keep`.
>
> --allow-empty-message::
> By default, cherry-picking a commit with an empty message will fail.
> This option overrides that behavior, allowing commits with empty
> messages to be cherry picked.
>
> ---keep-redundant-commits::
> - If a commit being cherry picked duplicates a commit already in the
> - current history, it will become empty. By default these
> - redundant commits cause `cherry-pick` to stop so the user can
> - examine the commit. This option overrides that behavior and
> - creates an empty commit object. Note that use of this option only
> +--empty=(stop|drop|keep)::
> + How to handle commits being cherry-picked that are redundant with
> + changes already in the current history.
> ++
> +--
> +`stop`;;
I'm still on the fence about "stop" vs "ask". I see in your tests you've
accidentally used "ask" which makes me wonder if that is the more
familiar term for users who probably use "git rebase" more often than
"git am".
> + The cherry-pick will stop when the empty commit is applied, allowing
> + you to examine the commit. This is the default behavior.
> +`drop`;;
> + The empty commit will be dropped.
> +`keep`;;
> + The empty commit will be kept. Note that use of this option only
> results in an empty commit when the commit was not initially empty,
> but rather became empty due to a previous commit. Commits that were
> initially empty will cause the cherry-pick to fail. To force the
> inclusion of those commits use `--allow-empty`.
> +--
> ++
> +Note that commits which start empty will cause the cherry-pick to fail (unless
> +`--allow-empty` is specified).
> ++
> +
> +--keep-redundant-commits::
> + Deprecated synonym for `--empty=keep`.
>
> --strategy=<strategy>::
> Use the given merge strategy. Should only be used once.
> diff --git a/builtin/revert.c b/builtin/revert.c
> index b2cfde7a87..1491c45e26 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -45,6 +45,30 @@ static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
> return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
> }
>
> +enum empty_action {
> + STOP_ON_EMPTY_COMMIT = 0, /* output errors and stop in the middle of a cherry-pick */
> + DROP_EMPTY_COMMIT, /* skip with a notice message */
> + KEEP_EMPTY_COMMIT, /* keep recording as empty commits */
> +};
> +
> +static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
> +{
> + int *opt_value = opt->value;
> +
> + BUG_ON_OPT_NEG(unset);
> +
> + if (!strcmp(arg, "stop"))
> + *opt_value = STOP_ON_EMPTY_COMMIT;
> + else if (!strcmp(arg, "drop"))
> + *opt_value = DROP_EMPTY_COMMIT;
> + else if (!strcmp(arg, "keep"))
> + *opt_value = KEEP_EMPTY_COMMIT;
> + else
> + return error(_("invalid value for '%s': '%s'"), "--empty", arg);
> +
> + return 0;
> +}
> +
> static int option_parse_m(const struct option *opt,
> const char *arg, int unset)
> {
> @@ -87,6 +111,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
> const char * const * usage_str = revert_or_cherry_pick_usage(opts);
> const char *me = action_name(opts);
> const char *cleanup_arg = NULL;
> + enum empty_action empty_opt;
> int cmd = 0;
> struct option base_options[] = {
> OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
> @@ -116,7 +141,10 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
> OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow fast-forward")),
> OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
> OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
> - OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
> + OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("deprecated: use --empty=keep instead")),
> + OPT_CALLBACK_F(0, "empty", &empty_opt, "(stop|drop|keep)",
> + N_("how to handle commits that become empty"),
> + PARSE_OPT_NONEG, parse_opt_empty),
> OPT_END(),
> };
> options = parse_options_concat(options, cp_extra);
> @@ -136,6 +164,11 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
> prepare_repo_settings(the_repository);
> the_repository->settings.command_requires_full_index = 0;
>
> + if (opts->action == REPLAY_PICK) {
> + opts->drop_redundant_commits = (empty_opt == DROP_EMPTY_COMMIT);
> + opts->keep_redundant_commits = opts->keep_redundant_commits || (empty_opt == KEEP_EMPTY_COMMIT);
> + }
The code changes look good but I think we want to update
verify_opt_compatible() to check for "--empty" being combined with
"--continue" etc. as well.
> if (cleanup_arg) {
> opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
> opts->explicit_cleanup = 1;
> diff --git a/sequencer.c b/sequencer.c
> index 582bde8d46..c49c27c795 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2934,6 +2934,9 @@ static int populate_opts_cb(const char *key, const char *value,
> else if (!strcmp(key, "options.allow-empty-message"))
> opts->allow_empty_message =
> git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
> + else if (!strcmp(key, "options.drop-redundant-commits"))
> + opts->drop_redundant_commits =
> + git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
> else if (!strcmp(key, "options.keep-redundant-commits"))
> opts->keep_redundant_commits =
> git_config_bool_or_int(key, value, ctx->kvi, &error_flag);
> @@ -3478,6 +3481,9 @@ static int save_opts(struct replay_opts *opts)
> if (opts->allow_empty_message)
> res |= git_config_set_in_file_gently(opts_file,
> "options.allow-empty-message", "true");
> + if (opts->drop_redundant_commits)
> + res |= git_config_set_in_file_gently(opts_file,
> + "options.drop-redundant-commits", "true");
It is good that we're saving the option - it would be good to add a test
to check that we remember --empty after stopping for a conflict resolution.
> if (opts->keep_redundant_commits)
> res |= git_config_set_in_file_gently(opts_file,
> "options.keep-redundant-commits", "true");
> diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
> index 6adfd25351..ae0cf7886a 100755
> --- a/t/t3505-cherry-pick-empty.sh
> +++ b/t/t3505-cherry-pick-empty.sh
> @@ -89,7 +89,7 @@ test_expect_success 'cherry-pick a commit that becomes no-op (prep)' '
> git commit -m "add file2 on the side"
> '
>
> -test_expect_success 'cherry-pick a no-op without --keep-redundant' '
> +test_expect_success 'cherry-pick a no-op with neither --keep-redundant nor --empty' '
> git reset --hard &&
> git checkout fork^0 &&
> test_must_fail git cherry-pick main
> @@ -104,4 +104,28 @@ test_expect_success 'cherry-pick a no-op with --keep-redundant' '
> test_cmp expect actual
> '
>
> +test_expect_success 'cherry-pick a no-op with --empty=ask' '
> + git reset --hard &&
> + git checkout fork^0 &&
> + test_must_fail git cherry-pick --empty=ask main
This is an example of why it is a good idea to check the error message
when using "test_must_fail" as here the test will fail due to a bad
value passed to "--empty" rather than for the reason we want the test to
check. It would be good to add a separate test to check that we reject
invalid "--empty" values.
> +'
> +
> +test_expect_success 'cherry-pick a no-op with --empty=drop' '
> + git reset --hard &&
> + git checkout fork^0 &&
> + git cherry-pick --empty=drop main &&
> + git show -s --format=%s >actual &&
> + echo "add file2 on the side" >expect &&
> + test_cmp expect actual
I think you could simplify this by using test_commit_message
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 3/4] rebase: Update `--empty=ask` to `--empty=drop`
From: Phillip Wood @ 2024-01-23 14:24 UTC (permalink / raw)
To: brianmlyles, git; +Cc: me, newren
In-Reply-To: <20240119060721.3734775-4-brianmlyles@gmail.com>
Hi Brian
On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
> From: Brian Lyles <brianmlyles@gmail.com>
>
> When `git-am` got its own `--empty` option in 7c096b8d61 (am: support
> --empty=<option> to handle empty patches, 2021-12-09), `stop` was used
> instead of `ask`. `stop` is a more accurate term for describing what
> really happens,
I can see your reasoning but I think of stopping as git's way of asking
what to do so I'm not sure if "stop" is better than "ask". I don't know
how we ended up with two different terms - the prior art is "ask" so
maybe we should change "am --empty" instead. Lets see what others think.
It would be helpful to mention the tests in the commit message - we end
up with a mixture of "--empty=ask" and "--empty=stop" I assume that is
by design
> and consistency is good. This commit updates
> `git-rebase` to also use `stop`, while keeping `ask` as a deprecated
> synonym.
If we're deprecating "ask" do we want to print a warning recommending
"stop" instead?
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 2/4] docs: Clean up `--empty` formatting in `git-rebase` and `git-am`
From: Phillip Wood @ 2024-01-23 14:24 UTC (permalink / raw)
To: brianmlyles, git; +Cc: me, newren
In-Reply-To: <20240119060721.3734775-3-brianmlyles@gmail.com>
Hi Brian
On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
> From: Brian Lyles <brianmlyles@gmail.com>
>
> Both of these pages document very similar `--empty` options, but with
> different styles. This commit aims to make them more consistent.
I think that's reasonable though the options they are worded as doing
different things. For "am" it talks about the patch being empty - i.e. a
patch of an empty commit whereas for "rebase" the option applies to
non-empty commits that become empty. What does "am" do if you try to
apply a patch whose changes are already present?
If you're aiming for consistency then it would be worth listing the
possible values in the same order for each command.
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index b4526ca246..3ee85f6d86 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -293,13 +293,20 @@ See also INCOMPATIBLE OPTIONS below.
> How to handle commits that are not empty to start and are not
> clean cherry-picks of any upstream commit, but which become
> empty after rebasing (because they contain a subset of already
> - upstream changes). With drop (the default), commits that
> - become empty are dropped. With keep, such commits are kept.
> - With ask (implied by `--interactive`), the rebase will halt when
> - an empty commit is applied allowing you to choose whether to
> - drop it, edit files more, or just commit the empty changes.
> + upstream changes):
> ++
> +--
> +`drop`;;
> + The empty commit will be dropped. This is the default behavior.
I think it would be clearer to say "The commit" - I found "The empty
commit" confusing as the commit that is being picked isn't empty.
> +`keep`;;
> + The empty commit will be kept.
> +`ask`;;
> + The rebase will halt when the empty commit is applied, allowing you to
> + choose whether to drop it, edit files more, or just commit the empty
> + changes. This option is implied when `--interactive` is specified.
> Other options, like `--exec`, will use the default of drop unless
> `-i`/`--interactive` is explicitly specified.
Thanks for adding a bit more detail about the default, however it looks
to me like we keep commits that become empty when --exec is specified
if (options.empty == EMPTY_UNSPECIFIED) {
if (options.flags & REBASE_INTERACTIVE_EXPLICIT)
options.empty = EMPTY_STOP;
else if (options.exec.nr > 0)
options.empty = EMPTY_KEEP;
else
options.empty = EMPTY_DROP;
}
Off the top of my head I'm not sure why or if that is a good idea.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: Phillip Wood @ 2024-01-23 14:23 UTC (permalink / raw)
To: brianmlyles, git; +Cc: me, newren
In-Reply-To: <20240119060721.3734775-2-brianmlyles@gmail.com>
Hi Brian
Let me start by saying that overall I'm impressed with the quality of
this submission. I've left quite a few comments but for a first patch
series it is very good.
On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
> From: Brian Lyles <brianmlyles@gmail.com>
>
> Previously, a consumer of the sequencer that wishes to take advantage of
> either the `keep_redundant_commits` or `drop_redundant_commits` feature
> must also specify `allow_empty`.
>
> The only consumer of `drop_redundant_commits` is `git-rebase`, which
> already allows empty commits by default and simply always enables
> `allow_empty`. `keep_redundant_commits` was also consumed by
> `git-cherry-pick`, which had to specify `allow-empty` when
> `keep_redundant_commits` was specified in order for the sequencer's
> `allow_empty()` to actually respect `keep_redundant_commits`.
I think it might be more persuasive to start the commit message by
explaining what user visible change you're trying to make and why rather
than concentrating on the implementation details.
> The latter is an interesting case: As noted in the docs, this means that
> `--keep-redundant-commits` implies `--allow-empty`, despite the two
> having distinct, non-overlapping meanings:
>
> - `allow_empty` refers specifically to commits which start empty, as
> indicated by the documentation for `--allow-empty` within
> `git-cherry-pick`:
>
> "Note also, that use of this option only keeps commits that were
> initially empty (i.e. the commit recorded the same tree as its
> parent). Commits which are made empty due to a previous commit are
> dropped. To force the inclusion of those commits use
> --keep-redundant-commits."
>
> - `keep_redundant_commits` refers specifically to commits that do not
> start empty, but become empty due to the content already existing in
> the target history. This is indicated by the documentation for
> `--keep-redundant-commits` within `git-cherry-pick`:
>
> "If a commit being cherry picked duplicates a commit already in the
> current history, it will become empty. By default these redundant
> commits cause cherry-pick to stop so the user can examine the commit.
> This option overrides that behavior and creates an empty commit
> object. Implies --allow-empty."
>
> This implication of `--allow-empty` therefore seems incorrect: One
> should be able to keep a commit that becomes empty without also being
> forced to pick commits that start as empty.
Do you have a practical example of where you want to keep the commits
that become empty but not the ones that start empty? I agree there is a
distinction but I think the common case is that the user wants to keep
both types of empty commit or none. I'm not against giving the user the
option to keep one or the other if it is useful but I'm wary of changing
the default.
> However, today, the
> following series of commands would result in both the commit that became
> empty and the commit that started empty being picked despite only
> `--keep-redundant-commits` being specified:
>
> git init
> echo "a" >test
> git add test
> git commit -m "Initial commit"
> echo "b" >test
> git commit -am "a -> b"
> git commit --allow-empty -m "empty"
> git cherry-pick --keep-redundant-commits HEAD^ HEAD
>
> The same cherry-pick with `--allow-empty` would fail on the redundant
> commit, and with neither option would fail on the empty commit.
>
> In a future commit, an `--empty` option will be added to
> `git-cherry-pick`, meaning that `drop_redundant_commits` will be
> available in that command. For that to be possible with the current
> implementation of the sequencer's `allow_empty()`, `git-cherry-pick`
> would need to specify `allow_empty` with `drop_redundant_commits` as
> well, which is an even less intuitive implication of `--allow-empty`: in
> order to prevent redundant commits automatically, initially-empty
> commits would need to be kept automatically.
>
> Instead, this commit rewrites the `allow_empty()` logic to remove the
> over-arching requirement that `allow_empty` be specified in order to
> reach any of the keep/drop behaviors. Only if the commit was originally
> empty will `allow_empty` have an effect.
rebase always sets "opts->allow_empty = 1" in
builtin/rebase.c:get_replay_opts() and if the user passes
--no-keep-empty drops commits that start empty from the list of commits
to be picked. This is slightly confusing but is more efficient as we
don't do waste time trying to pick a commit we're going to drop. Can we
do something similar for "git cherry-pick"? When cherry-picking a
sequence of commits I think it should just work because the code is
shared with rebase, for a single commit we'd need to add a test to see
if it is empty in single_pick() before calling pick_commits().
> For some amount of backwards compatibility with the existing code and
> tests, I have opted to preserve the behavior of returning 0 when:
>
> - `allow_empty` is specified, and
> - either `is_index_unchanged` or `is_original_commit_empty` indicates an
> error
I'm not sure that is a good idea as it is hiding an error that we didn't
hit before because we returned early.
> This is primarily out of caution -- I am not positive what downstream
> impacts this might have.
>
> Note that this commit is a breaking change: `--keep-redundant-commits`
> will no longer imply `--allow-empty`. It would be possible to maintain
> the current behavior of `--keep-redundant-commits` implying
> `--allow-empty` if it were needed to avoid a breaking change, but I
> believe that decoupling them entirely is the correct behavior.
Thank you for being clear about the change in behavior, as I said above
I'm wary of changing the default unless there is a compelling reason but
I'm happy to support
git cherry-pick --keep-redundant-commits --no-allow-empty
if it is needed.
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> ---
>
> Disclaimer: This is my first contribution to the git project, and thus
> my first attempt at submitting a patch via `git-send-email`. It is also
> the first time I've touched worked in C in over a decade, and I really
> didn't work with it much before that either. I welcome any and all
> feedback on what I may have gotten wrong regarding the patch submission
> process, the code changes, or my commit messages.
As others have mentioned I think it would be useful to have a
cover-letter where we can discuss the aim of the patch series
independently of the individual patches.
> This is the first in a series of commits that aims to introduce an
> `--empty` option to `git-cherry-pick` that provides the same flexibility
> as the `--empty` options for `git-rebase` and `git-am`, as well as
> improve the consistency in the values and documentation for this option
> across the three commands.
I think that is a good aim
> The main thing that may be controversial with this particular commit is
> that I am proposing a breaking change. As described in the above
> message, I do not think that it makes sense to tie `--allow-empty` and
> `--keep-redundant-commits` together since they appear to be intended to
> work with different types of empty commits. That being said, if it is
> deemed unacceptable to make this breaking change, we can consider an
> alternative approach where we maintain the behavior of
> `--keep-redundant-commits` implying `--allow-empty`, while preventing
> the need for the future `--empty=drop` to have that same implication.
As I said above I think it would be worth looking at what "git rebase"
does to see if we can do the same thing for "git cherry-pick".
> [...]> +test_expect_success 'cherry pick an empty non-ff commit with
--keep-redundant-commits' '
> + git checkout main &&
> + test_must_fail git cherry-pick --keep-redundant-commits empty-change-branch
When using test_must_fail it is a good idea to check the error message
to make sure that it's failing for the reason we expect (see patch 4).
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions
From: Toon Claes @ 2024-01-23 14:08 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau, Eric Sunshine
In-Reply-To: <1284b70fab6dd85114cb24fc5c7b6c49e35eb135.1704877233.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> [[PGP Signed Part:Undecided]]
> [1. text/plain]
> In t1302 we exercise logic around "core.repositoryFormatVersion" and
> extensions. These tests are not particularly robust against extensions
> like the newly introduced "refStorage" extension.
>
> Refactor the tests to be more robust:
>
> - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
> repository format version. This helps to ensure that we only need to
> update the prereq in a central place when new extensions are added.
>
> - Use a separate repository to rewrite ".git/config" to test
> combinations of the repository format version and extensions. This
> ensures that we don't break the main test repository.
>
> - Do not rewrite ".git/config" when exercising the "preciousObjects"
> extension.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/t1302-repo-version.sh | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
> index 179474fa65..7c680c91c4 100755
> --- a/t/t1302-repo-version.sh
> +++ b/t/t1302-repo-version.sh
> @@ -79,8 +84,13 @@ mkconfig () {
>
> while read outcome version extensions; do
> test_expect_success "$outcome version=$version $extensions" "
> - mkconfig $version $extensions >.git/config &&
> - check_${outcome}
> + test_when_finished 'rm -rf extensions' &&
> + git init extensions &&
> + (
> + cd extensions &&
> + mkconfig $version $extensions >.git/config &&
Why did you not remove the use of `mkconfig` here?
--
Toon
^ permalink raw reply
* Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends
From: Toon Claes @ 2024-01-23 13:41 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau, Eric Sunshine
In-Reply-To: <0552123fa30243d6d8d6b378991651dd6ade7de3.1704877233.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> [[PGP Signed Part:Undecided]]
> [1. text/plain]
> The t1300 test suite exercises the git-config(1) tool. To do so we
> overwrite ".git/config" to contain custom contents. While this is easy
> enough to do, it may create problems when using a non-default repository
> format because we also overwrite the repository format version as well
> as any potential extensions. With the upcoming "reftable" ref backend
> the result is that we may try to access refs via the "files" backend
> even though the repository has been initialized with the "reftable"
> backend.
>
> Refactor tests which access the refdb to be more robust by using their
> own separate repositories, which allows us to be more careful and not
> discard required extensions.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
> '
>
> test_expect_success 'set up custom config file' '
> - CUSTOM_CONFIG_FILE="custom.conf" &&
> - cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> + cat >"custom.conf" <<-\EOF &&
> [user]
> custom = true
> EOF
> + CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path
> custom.conf)"
> '
From the commit message it was not clear to me this change was needed.
Do you think it's worth it to add something to the commit message
explaining you now need to copy the custom.conf into each seperate
repository?
--
Toon
^ permalink raw reply
* Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
From: Karthik Nayak @ 2024-01-23 12:49 UTC (permalink / raw)
To: phillip.wood, Junio C Hamano; +Cc: git
In-Reply-To: <ba721840-7b67-4822-8046-c0da4d3b9bde@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]
Hello Phillip,
Phillip Wood <phillip.wood123@gmail.com> writes:
>
> Hopefully such a rule would stop us adding pseudorefs that are really
> private state like MERGE_AUTOSTASH. I think that is good in the long
> term but isn't it is happening now with this patch without any warning
> to users? This patch changes the behavior of parse_worktree_ref() which
> the files backend uses to figure out the path it should use when reading
> and writing a ref.
>
I do agree with the problem you're outlining here. Changing
`is_pseudoref_syntax()` does indeed break things since its also used by
`parse_worktree_ref()`.
I first thought I could get around this by adding the required missing
refs, but even that wouldn't work. Because BISECT_START has dual nature,
it act as a ref and also as file storing a branch name as Patrick
mentions in detail in his email [1]. Meaning if `is_pseudoref_syntax()`
identifies it as a pseudoref, it could be wrong and printing it as such
might not work. But we can't not match it because that is the current
expectation.
So there is no way to make `is_pseudoref_syntax()` stricter without
breaking backward compatibility. While we do want to reach that goal, we
have to go about in the other way around, that i.e.
1. Fix all pseudorefs to have the '_HEAD' suffix.
2. Move bisect files to '$GIT_DIR/bisect-state' (see [1] for more
details).
After this, we can safely make `is_pseudoref_syntax()` stricter.
Given this, I think for the next version, I'll do the following changes:
1. keep `is_pseudoref_syntax()` as is.
2. introduce `is_pseudoref()` which calls `is_pseudoref_syntax()` and
also checks the content of the file.
3. replace use of `is_pseudoref_syntax()` with `is_pseudoref()` in this
patch series.
[1]: https://public-inbox.org/git/20240119142705.139374-1-karthik.188@gmail.com/T/#m6e3790e30613fd68349708faaf5f4d9c704ba677
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
From: Patrick Steinhardt @ 2024-01-23 11:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, Karthik Nayak, git
In-Reply-To: <xmqqplxtrucm.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2500 bytes --]
On Mon, Jan 22, 2024 at 12:22:49PM -0800, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> > The list of hard coded exceptions also looks quite short, I
> > can see MERGE_AUTOSTASH and BISECT_START are missing and there are
> > probably others I've not thought of.
>
> I agree that it is something we need to fix.
I've taken a deeper look at BISECT_START because I previously missed it
in my conversion to make special refs become normal pseudo refs. But as
it turns out, BISECT_START is not a ref at all.
Depending on how you execute git-bisect(1), it will either contain the
object ID of the detached HEAD or the branch you're starting the bisect
from. This information is used to switch back to that state when you
abort the bisect. So far this sounds like a proper ref indeed. But in
case you're starting from a branch it will not be a symref that points
to this branch, but it will just contain the branch name. This is not a
valid format that could be read as a loose ref, and thus this file is
not a proper ref at all (except that sometimes it behaves like one when
starting from a detached HEAD).
My first hunch was to convert it so that it indeed always is a proper
ref. But thinking about it a bit more I'm less convinced that this is
sensible as it is deeply tied to the behaviour of git-bisect(1) and only
represents its internal state. I thus came to the conclusion that it is
more similar to the sequencer state that we have in ".git/rebase-merge"
and ".git/rebase-apply" than anything else.
So if we wanted to rectify this, I think the most sensible way to
address this would be to introduce a new ".git/bisect-state" directory
that contains all of git-bisect(1)'s state:
- BISECT_TERMS -> bisect-state/terms
- BISECT_LOG -> bisect-state/log
- BISECT_START -> bisect-state/start
- BISECT_RUN -> bisect-state/run
- BISECT_FIRST_PARENT -> bisect-state/first-parent
- BISECT_ANCESTORS_OK -> bisect-state/ancestors-ok
I think this would make for a much cleaner solution overall as things
are neatly contained. Cleaning up after a bisect would thus only require
a delete of ".git/bisect-state/" and we're done.
Of course, this would be a backwards-incompatible change. We could
transition to that newer schema by having newer Git versions recognize
both ways to store the state, but only ever write the new schema. But
I'm not sure whether it would ultimately be worth it.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
From: Phillip Wood @ 2024-01-23 11:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, git
In-Reply-To: <xmqqplxtrucm.fsf@gitster.g>
Hi Junio
On 22/01/2024 20:22, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> I'm concerned that this change is a regression. is_pseudoref_syntax()
>> is used by is_current_worktree_ref() and so scripts that create
>> pseudorefs that do not conform to your new rules will break as git
>> will no-longer consider the pseudorefs they create to be worktree
>> specific.
>
> Ideally, when the exception list in the function becomes more
> complete, those "pseudorefs" created by those scripts shouldn't
> probably be created either as common or worktree specific thing
> if they are not "pseudoref".
I not sure I quite understand what you mean here. Are you saying that
scripts should stop using "git update-ref" and "git rev-parse" for
anything that does not match the new pseudoref syntax?
>> Another approach would be to
>> read all the files whose name matches the pseudoref syntax and see if
>> its contents looks like a valid ref skipping names like COMMIT_EDITMSG
>> that we know are not pseudorefs.
>
> In the longer term, I'd prefer to see a simpler rule, like "all-caps
> or underscore string, ending with _HEAD and nothing else are the
> pseudorefs but we have these small number of exceptions that are
> grandfathered".
Hopefully such a rule would stop us adding pseudorefs that are really
private state like MERGE_AUTOSTASH. I think that is good in the long
term but isn't it is happening now with this patch without any warning
to users? This patch changes the behavior of parse_worktree_ref() which
the files backend uses to figure out the path it should use when reading
and writing a ref.
Best Wishes
Phillip
^ permalink raw reply
* Re: ps/reftable-optimize-io (was: What's cooking in git.git (Jan 2024, #06; Fri, 19))
From: Patrick Steinhardt @ 2024-01-23 8:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqbk9ghio5.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 905 bytes --]
On Fri, Jan 19, 2024 at 05:56:10PM -0800, Junio C Hamano wrote:
> * ps/reftable-optimize-io (2024-01-18) 7 commits
> - reftable/stack: fix race in up-to-date check
> - reftable/stack: unconditionally reload stack after commit
> (merged to 'next' on 2024-01-12 at 4096e880e0)
> + reftable/blocksource: use mmap to read tables
> + reftable/blocksource: refactor code to match our coding style
> + reftable/stack: use stat info to avoid re-reading stack list
> + reftable/stack: refactor reloading to use file descriptor
> + reftable/stack: refactor stack reloading to have common exit path
>
> Low-level I/O optimization for reftable.
>
> The two commits at the tip are new.
> Will merge to 'next' and then to 'master'?
> source: <cover.1704966670.git.ps@pks.im>
> source: <cover.1705585037.git.ps@pks.im>
Yeah, this topic is good to go from my point of view.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Kristoffer Haugsbakk @ 2024-01-23 7:11 UTC (permalink / raw)
To: brianmlyles; +Cc: Taylor Blau, Elijah Newren, git
In-Reply-To: <CAHPHrSd8rLj_TDE11dYQW+51--8YC4rumnfT+v2bYr+K7AMQrQ@mail.gmail.com>
On Tue, Jan 23, 2024, at 06:23, Brian Lyles wrote:
>> Oh, and this thread reminded me https://lore.kernel.org/git/xmqqle8hrtcs.fsf@gitster.g/T/#t
>>
>> that editorconfig[1] has this option:
>>
>> ```
>> trim_trailing_whitespace = true
>> ```
>> […]
>
> Is there a good reason that this should not just be added to the
> `.editorconfig` in this repository? Would a patch for this be welcome?
I was thinking the same thing when I saw that other thread. It doesn’t
hurt to try.
I was also curious about some subtleties like: does it dictate enforcing
this for all the lines of a touched files or only the modified ones?
Because the latter is much more useful.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: Fwd: Unexpected behavior of ls-files command when using --others --exclude-from, and a .gitignore file which resides in a subdirectory
From: Raúl Núñez de Arenas Coronado @ 2024-01-23 6:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, Phillip Wood, Sebastian Thiel
In-Reply-To: <xmqq5xzlrqoi.fsf@gitster.g>
Hi Junio!
El lun, 22 ene 2024 a las 22:42, Junio C Hamano (<gitster@pobox.com>) escribió:
> We have been discussing to extend the mechanism so that we can have
> "precious" files, which also will be left out of the project (e.g.,
> "git add ." will not add to the index, just like an ignored file)
> but are not considered "expendable". If file F is "precious":
>
> - "git add ." will not add F to the index
>
> - "git status" will not say F is left untracked and can be added
>
> - "git clean -f" will not remove it.
>
> - checking out a branch with a tracked file F/G will *fail*, to
> prevent the loss of file.
And that is exactly the concept I'm handling here: files that should
not be tracked BUT that are not expendable. You explained it concisely
and perfectly :)))
I'll wait until that is implemented, and in the meantime I have a
couple solutions I want to try, like using .gitprecious files and 'git
ls-files --other --ignored --exclude-from=.gitprecious'. I have more
ideas.
Thanks a lot for the help and the explanation!
--
Raúl Núñez de Arenas Coronado
.
^ permalink raw reply
* Re: Fwd: Unexpected behavior of ls-files command when using --others --exclude-from, and a .gitignore file which resides in a subdirectory
From: Raúl Núñez de Arenas Coronado @ 2024-01-23 5:40 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240122213410.GA811766@coredump.intra.peff.net>
Hi Jeff, and thanks for your reply :)
El lun, 22 ene 2024 a las 22:34, Jeff King (<peff@peff.net>) escribió:
> For example, I get:
>
> [setup]
> $ git init
> $ mkdir subdir
> $ echo '*' >subdir/.gitignore
> $ git add -f subdir/.gitignore && git commit -m "add gitignore"
> $ touch subdir/file file
>
> [no exclusions]
> $ git ls-files -o
> file
> subdir/file
>
> [use .gitignore]
> $ git ls-files --exclude-per-directory=.gitignore -o
> file
>
> [using standard excludes]
> $ git ls-files --exclude-standard -o
> file
>
> Do you get different results from that toy repo? If not, then what is
> different about your main repo? Do you perhaps have a stray "*" match
> somewhere in .git/info/exclude, etc? Or are you still providing
> --exclude-from in addition to --exclude-standard?
The difference lies in how I deal in my computer with ignored files. I
have some files in all my repos which have to be ignored always, so I
have that pattern in my core.excludes file. BUT those files have to be
backed up on my system, just in case my local copy of the repo is lost
for some reason, as they are files I need for development in my
personal machine.
If I use --exclude-standard, no matter if those files are being
ignored by core.excludes OR .git/info/exclude, they won't appear in
that 'list other files' command output.
So, my previous idea was ignore ONLY those files ignored by the
repository .gitignore file. That way, the other files that go in my
core.excludes or .git/info/exclude will be listed and backed up. The
problem here is that some of the repository gitignored files (files
related to building, testing, etc.) have their own .gitignore file
containing '*", but they are NOT present in the repo .gitignore. This
is a common practice for Python virtual environments, for example.
Summing up: or I end up backing up those directories that have their
own .gitignore file (not the end of the world) or I use a different
solution to decide which files are ignored but "precious" (I love that
concept, see Junio C Hamano message for that) OR I add those
directories to the .gitignore present in the repo. This last idea I
don't like very much, as they are particular to my system, not the
repo, for example, *I* may decide to use ESLint and node_modules for
some particular web page I'm writing, but the related configuration
should not go into the repository and should not be ignored in
.gitignore, but in .git/info/exclude. I won't backup node_modules, of
course, it is going to be recreated, but I may want to back up
packages.json and .eslintrc. And both of those files will go into
.git/info/exclude, so if I use --exclude-standard, they won't be
backed up.
Since as both Junio and you correctly pointed, this is in fact the
expected, I'll try to find a different solution while the "precious"
concept is finally implemented. I have a couple of ideas, like using a
.giprecious file which my backup script can process easily, containing
the untracked files to back-up.
Thanks for the toy repo, helped me test the differences with my setup!
--
Raúl Núñez de Arenas Coronado
.
^ permalink raw reply
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Brian Lyles @ 2024-01-23 5:23 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Taylor Blau, Elijah Newren, git
In-Reply-To: <10838549-c364-429b-a086-68a41b7369de@app.fastmail.com>
On Sun, Jan 21, 2024 at 4:05 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
> I don’t know of any global config. So a pre-commit hook is probably the
> safest bet. Personally I set all my editors to remove trailing space and
> they very seldom mess it up. :)
Fair point -- Apparently this was a gap in my nvim config.
Easily added.
On Mon, Jan 22, 2024 at 2:55 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
> Oh, and this thread reminded me https://lore.kernel.org/git/xmqqle8hrtcs.fsf@gitster.g/T/#t
>
> that editorconfig[1] has this option:
>
> ```
> trim_trailing_whitespace = true
> ```
>
> So I guess that should be enough for all editors that respect this
> config (although I haven’t tested it).
>
> 🔗 1: https://editorconfig.org/
Is there a good reason that this should not just be added to the
`.editorconfig` in this repository? Would a patch for this be welcome?
I do see that there are ~130 files with trailing whitespace in maint
today, though I suspect that most of those are not intentional.
Brian Lyles
^ permalink raw reply
* Re: [RFC PATCH 2/4] test-tool run-command testsuite: support unit tests
From: Jeff King @ 2024-01-23 0:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Steadmon, git, johannes.schindelin, phillip.wood
In-Reply-To: <xmqqil3sx2y6.fsf@gitster.g>
On Tue, Jan 16, 2024 at 03:40:01PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Josh Steadmon <steadmon@google.com> writes:
> >
> >> Teach the testsuite runner in `test-tool run-command testsuite` how to
> >> run unit tests, by adding two new flags:
> >>
> >> First, "--(no-)run-in-shell" allows the test-tool to exec the unit-test
> >> binaries directly, rather than trying to interpret them as shell
> >> scripts.
> >
> > Makes perfect sense.
>
> This may be a stupid question, but do we even need the current "push
> 'sh' to the strvec"? If our executable shell scripts run just fine
> without, then this may not have to be conditional.
It is necessary for the same reason that the indirection provided by
patch 4 is necessary: the test scripts are supposed to be run by
TEST_SHELL_PATH, even if they may say "#!/bin/sh" in their header.
But the "testsuite" helper just hard-codes "sh", which is wrong. It's
somewhat academic for Windows where "sh" is already bash, and that's the
only reasonable thing anybody would set TEST_SHELL_PATH to anyway. But
if the point is to be a drop-in replacement for the existing flow, it
should be checking getenv("TEST_SHELL_PATH"). And perhaps the Makefile
might need to correctly export the variable.
-Peff
^ permalink raw reply
* Re: Defining a platform support policy
From: Junio C Hamano @ 2024-01-23 0:57 UTC (permalink / raw)
To: rsbecker
Cc: 'Emily Shaffer', 'Taylor Blau',
'Dragan Simic', 'Git List',
'Johannes Schindelin'
In-Reply-To: <038701da4d90$b1bfb010$153f1030$@nexbridge.com>
<rsbecker@nexbridge.com> writes:
> I think we might want to add some considerations to the above list
> that go beyond what other projects use, OpenSSL as an example:
[jc: if you want to have a meaningful discussion on this list,
please stick to a reasonable line width. I'll rewrap your lines
below].
> * Can support for exotic platforms be delegated to some
> "community" support concept. In NonStop's case, I currently do 99%
> of the verification that each release runs properly. If I am able
> to provide a fix, I will. We have been fortunate that most
> problems/solutions have been of general interest and impact, with
> my platforms being more of a "Canary in the Coalmine" situation
> where we just encounter it first because of edge conditions, but
> other platforms may be impacted. The problem here is time of how
> long a designated community support person(s) can keep supporting
> git and what happens when they (me) retire or get hit by a
> bus. Like all good NonStop people, I have a backup, so git does
> not need to worry about me specifically.
There are platform packagers that deliver binary releases, and we do
not have to worry about them. We _could_ have a tier of minority
platform that we can treat pretty much the same as these packagers,
i.e. the "community supported version of Git for platform X" might
consist of many patches on top of what I release, and some patches
that are acceptable quality may be given upstream, but there may
need hacks that are too ugly to live in my tree, which the
"community edition" may have to keep outside the upstream. Even in
such a case, if they try to engage with this list, they will often
find somebody willing to help them polish such "ugly hacks" into
acceptable patches.
> * What is the broad impact of dropping support for a platform that
> has a significant investment in git, where loss of support could
> have societal impact. There are platforms with 100 million to 1000
> million lines of code managed by git today. This type of
> investment-related impact is specific to git compared to other
> Open-Source products. Leaving debit or credit card authorizers
> without a supported git would be, let's say, "bad".
Let's say we may want to start requiring new enough version of
library that is not yet ported to a minority platform. Do we deeply
care? It depends but "investment-related impact" is unlikely cause
for us to personally care. But those $CORPS who will feel the
"investment-related impact" are welcome to hire quality developers
and to these employed developers, the "impact" might become an issue
they care more deeply about.
> * Could stakeholders be consulted before changing support levels?
> Yes, I get that commercial fee-based products hit this more than
> Open-Source. Looking at other products in the Open-Source space,
> there are fee-based support models that could be developed for
> long-term support (beyond the obvious LTS-type considerations -
> see OpenSSL's model for reference).
The stakeholders are already consulted, aren't they? Every time we
make noises like "let's raise the minimum version of Perl we
require", we discuss it here. They have to monitor this list, of
course, and if they lack people to do so, then they may have to
invest in it.
> A related question is: "If there is a bug detected in git, what
> version is the oldest supported git version to which a fix can be
> made?"
This is a good question. The latest security-induced maintenance
release was Git 2.40.1 done in March 2023 and the fixes go back to
the v2.30 track, and Git 2.30.0 was done at the end of 2021. This
window was unusually generous from our usual standard, IIRC, so I
would say roughly speaking 2 years is the maximum.
> ... But this position puts pressure on the team to maintain
> platform compatibility for indefinite periods.
Sure, but I think we should just say something like "18 months to 24
months", if you want backport of a fix to older track, you can do so
yourself.
The story is probably the same if a minority platform that lacks
recent enough dependencies (e.g. libraries) and stop linking
correctly. If you care deeply enough, you should be ready to invest
yourself in porting such dependencies. We can help, but the primary
driving force for porting issues ought to be folks with stake in the
platform. We as the project won't bend over backwards and keep
everybody else to an ancient version of the dependency if some
platforms cannot catch up with the time.
^ permalink raw reply
* Re: [RFC PATCH 1/4] t0080: turn t-basic unit test into a helper
From: Jeff King @ 2024-01-23 0:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Steadmon, git, johannes.schindelin, phillip.wood
In-Reply-To: <xmqqa5p4yjmq.fsf@gitster.g>
On Tue, Jan 16, 2024 at 02:54:21PM -0800, Junio C Hamano wrote:
> > ... so let's move it to t/t0080/t-basic.c and
> > adjust Makefiles and .gitignores as necessary.
>
> ... this conclusion. I somehow thought that t-basic part would be a
> good test-tool subcommand, as it is run from the suite of shell
> scripts for end-to-end testing CLI interaction.
Heh, I was about to ask the same thing.
In particular...
> Do we have any precedent to place programs placed under t/tXXXX/ and
> get them compiled?
...no, I don't think we do. And quite often I exclude those directories
when grepping around the code base, because there is often code there
that is purely used as a data fixture. E.g., t4256 contains a copy of
mailinfo.c which it uses as input for some of the tests. That code also
happens to have out-of-bounds memory reads which we have since fixed in
the real mailinfo.c, but of course "grep" finds them both. :)
So I would prefer a rule like "no buildable code in t/t[0-9]*". Barring
that, maybe we could avoid using things that look too much like real Git
code in our tests (though we sometimes do need fake code for things like
funclines, and even that might end up creating false positives).
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] reftable/stack: fix race in up-to-date check
From: Jeff King @ 2024-01-23 0:32 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <Za5ELqZps0DLbb5q@tanuki>
On Mon, Jan 22, 2024 at 11:32:14AM +0100, Patrick Steinhardt wrote:
> > I didn't think too hard about the details, but does this mean that
> > every user of stat_validity_check() has the same issue? The other big
> > one is packed-refs (for which the code was originally written). Should
> > this fix go into that API?
>
> In theory, the issue is the same for the `packed-refs` file. But in
> practice it's much less likely to be an issue:
Thanks for laying this all out. It does concern me a little that there's
still a possible race, because they can be so hard to catch and debug in
practice. But I think you make a compelling argument that it's probably
not happening a lot in practice, and especially...
> Also, applying the same fix for the packed-refs would essentially mean
> that the caching mechanism is now ineffective on Windows systems where
> we do not have proper `st_dev` and `st_ino` values available. I think
> this is a no-go in the context of packed-refs because we don't have a
> second-level caching mechanism like we do in the reftable backend. It's
> not great that we have to reread the "tables.list" file on Windows
> systems for now, but at least it's better than having to reread the
> complete "packed-refs" file like we'd have to do with the files backend.
...here that the performance profile is so different. If the "fix"
means re-reading the whole packed-refs file constantly, that's going
to be quite noticeable.
Given that this race has been here forever-ish, I agree with you that we
should leave it be.
-Peff
^ permalink raw reply
* Re: Defining a platform support policy
From: Junio C Hamano @ 2024-01-23 0:31 UTC (permalink / raw)
To: Emily Shaffer
Cc: Randall S. Becker, Taylor Blau, Dragan Simic, Git List,
Johannes Schindelin
In-Reply-To: <CAJoAoZnHGTFhfR6e6r=GMSfVbSNgLoHF-opaWYLbHppiuzi+Rg@mail.gmail.com>
Emily Shaffer <nasamuffin@google.com> writes:
> But, Randall's remarks bring up something pretty compelling: I don't
> think Git has a clearly defined platform support policy. As far as I
> can tell, the support policy now is "if you run `make test` on it and
> breaks, and you let us know, we'll try to fix it"
I doubt this part. If there is somebody motivated enough among us
who has access to such a platform, then that person may try to fix
it and if the fix is not too ugly, I may accept such a patch as the
upstream maintainer. So your "you let us know we'll try" does not
reflect reality at all. The major platforms luckily have such
motivated somebody almost always available for them. Niche ones,
perhaps not.
> ..., but nowhere do I see "how to know if your platform is
> supported" or even "here are platforms we have heard Git works OK on".
Yup. Patches, with commitments to keep such lists up-to-date, are
very much welcome.
Thanks.
^ permalink raw reply
* Re: [Bug?] "git diff --no-rename A B"
From: Jeff King @ 2024-01-23 0:28 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, git
In-Reply-To: <20240122230051.GB814674@coredump.intra.peff.net>
On Mon, Jan 22, 2024 at 06:00:51PM -0500, Jeff King wrote:
> then running:
>
> t/helper/test-tool parse-options --do
>
> correctly complains about the ambiguity (though amusingly it mentions
> "--no-no-doubt" in the error message). And if I add KEEP_UNKNOWN_OPT,
> then it gives the wrong behavior. But curiously it does so even with
> your patch applied. So I think there may be further fixes needed.
Oh sorry, I'm stupid. Of course it does not complain with
KEEP_UNKNOWN_OPT. It treats it as unknown and keeps it!
So with the patch I showed (and adding in KEEP_UNKNOWN_OPT), even
without your patch "--do" is handled correctly (it is unknown and left
in argv[0]). It is only "--no-do" which is broken, and your patch fixes
that.
Sorry for the noise. :)
-Peff
^ permalink raw reply
* RE: Defining a platform support policy (Was: [DISCUSS] Introducing Rust into the Git project)
From: rsbecker @ 2024-01-23 0:11 UTC (permalink / raw)
To: 'Emily Shaffer'
Cc: 'Taylor Blau', 'Junio C Hamano',
'Dragan Simic', 'Git List',
'Johannes Schindelin'
In-Reply-To: <CAJoAoZnHGTFhfR6e6r=GMSfVbSNgLoHF-opaWYLbHppiuzi+Rg@mail.gmail.com>
On Monday, January 22, 2024 6:18 PM, Emily Shaffer wrote:
>To: Randall S. Becker <rsbecker@nexbridge.com>
>Cc: Taylor Blau <me@ttaylorr.com>; Junio C Hamano <gitster@pobox.com>; Dragan
>Simic <dsimic@manjaro.org>; Git List <git@vger.kernel.org>; Johannes Schindelin
><Johannes.Schindelin@gmx.de>
>Subject: Defining a platform support policy (Was: [DISCUSS] Introducing Rust into
>the Git project)
>
>On Wed, Jan 10, 2024 at 3:52 PM <rsbecker@nexbridge.com> wrote:
>>
>> On Wednesday, January 10, 2024 5:26 PM, Taylor Blau wrote:
>> >On Wed, Jan 10, 2024 at 05:15:53PM -0500, rsbecker@nexbridge.com wrote:
>> >> Just a brief concern: Rust is not broadly portable. Adding another
>> >> dependency to git will remove many existing platforms from future releases.
>> >> Please consider this carefully before going down this path.
>> >
>> >I was hoping to hear from you as one of the few (only?) folks who
>> >participate on the list and represent HPE NonStop users.
>> >
>> >I'm curious which if any of the compiler frontends that I listed in
>> >my earlier email would work for you.
>>
>> Unfortunately, none of the compiler frontends listed previously can be built for
>NonStop. These appear to all require gcc either directly or transitively, which cannot
>be ported to NonStop. I do not expect this to change any time soon - and is outside
>of my control anyway. An attempt was made to port Rust but it did not succeed
>primarily because of that dependency. Similarly, Golang is also not portable to
>NonStop because of architecture assumptions made by the Go team that cannot be
>satisfied on NonStop at this time. If some of the memory/pointer issues are the
>primary concern, c11 might be something acceptable with smart pointers. C17 will
>eventually be deployable, but is not available on most currently supported OS
>versions on the platform.
>
>I hope y'all don't mind me hijacking this part of the thread ;)
I'm happy you did this. The topic is crucial - if nowhere else but to my ability to sleep at night. Preserving Emily's comments without snipping as these are important questions and comments.
>But, Randall's remarks bring up something pretty compelling: I don't think Git has a
>clearly defined platform support policy. As far as I can tell, the support policy now is
>"if you run `make test` on it and breaks, and you let us know, we'll try to fix it" -
>without much in the way of additional caveats. If I look in CodingGuidelines I see a
>few "this doesn't work on platform X so don't do it" (like around %z in printf), but
>nowhere do I see "how to know if your platform is supported" or even "here are
>platforms we have heard Git works OK on".
>
>That causes a lot of confusion for the project - threads like this one (and presumably
>a similar one about C99 adoption) become a blend of "is this change good for the
>project or not?" and "will this change leave behind platform X?" that is difficult to
>pick apart.
>
>Does it make sense for us to formalize a support policy? For example, if we wanted
>to formalize the status quo, I could envision:
>
>"""
>Platform support: We make a best-effort attempt to solve any bugs reported to the
>list, regardless of platform. To prevent breakages in the first place, consider running
>Git's `make test` regularly on your platform and reporting the results to
>git@vger.kernel.org; or, better yet, consider adding your platform to the GitHub
>Actions CI (configured in `.github/`).
>"""
>
>Or, if we wanted to be able to move very nimbly, we could imagine something much
>more restrictive (note that I'm not endorsing it, just
>illustrating):
>
>"""
>Platform support: Git is guaranteed to work well on Linux platforms using a kernel
>version that is less than 1 year old. Support for all other platforms is best-effort;
>when reporting a bug on another platform, you may need to patch the issue and
>verify your fix yourself.
>"""
>
>I suspect there's a happy medium in here somewhere - trying to fix (or
>avoid) an issue on a platform which the average developer cannot run tests on is
>not a recipe for a happy developer, and a general policy of "patches welcome" for
>anything but latest Linux is not a recipe for happy users.
>
>I see a few axes we can play with:
> * which architectures/kernels/OS (do we care about more than the usual suspects
>of Linux/Mac/Windows // x86/amd/arm //
>POSIX-compliant?)
> * age of architectures/kernels (do we care to offer full support for a 10 or 15 year
>old OS?)
> * new feature compatibility guarantees vs. core functionality/security fix
>guarantees (which do we really define "support" as?)
> * test provisioning (do we require a VM we can run CI on, or is a report generated
>from a nightly build and mailed to the list OK?)
> * test/breakage timing (should the above tests run on every commit to 'next'?
>every merge to 'master'? every RC?)
> * who provides the support (is it the patch author's responsibility to fix late-
>breaking platform support bugs? is it the reporter's responsibility? and especially,
>how does this interplay with test provisioning and frequency above?)
>
>If we had clearer answers to these questions, it'd be much simpler to determine
>whether experimentation with Rust is possible or useful.
>Plus it would make developer lives easier, in general, to understand how much
>compatibility support work they're potentially signing up for when sending a change
>of any size.
I think we might want to add some considerations to the above list that go beyond what other projects use, OpenSSL as an example:
* Can support for exotic platforms be delegated to some "community" support concept. In NonStop's case, I currently do 99% of the verification that each release runs properly. If I am able to provide a fix, I will. We have been fortunate that most problems/solutions have been of general interest and impact, with my platforms being more of a "Canary in the Coalmine" situation where we just encounter it first because of edge conditions, but other platforms may be impacted. The problem here is time of how long a designated community support person(s) can keep supporting git and what happens when they (me) retire or get hit by a bus. Like all good NonStop people, I have a backup, so git does not need to worry about me specifically.
* What is the broad impact of dropping support for a platform that has a significant investment in git, where loss of support could have societal impact. There are platforms with 100 million to 1000 million lines of code managed by git today. This type of investment-related impact is specific to git compared to other Open-Source products. Leaving debit or credit card authorizers without a supported git would be, let's say, "bad".
* Could stakeholders be consulted before changing support levels? Yes, I get that commercial fee-based products hit this more than Open-Source. Looking at other products in the Open-Source space, there are fee-based support models that could be developed for long-term support (beyond the obvious LTS-type considerations - see OpenSSL's model for reference). A related question is: "If there is a bug detected in git, what version is the oldest supported git version to which a fix can be made?" 2.0.0? 1.8.0 (looking at some Linux distro's RPM or APT repositories as being seriously guilty here). My MacOS server (just a couple of years ago) came with 1.8.0. I can't answer that question if asked by a customer. An alternative model, which seems to be informally embraced by git is "please upgrade to the latest - or a fix on the latest few releases". But this position puts pressure on the team to maintain platform compatibility for indefinite periods.
* What level of compatibility will be more appropriate to ensure git's reputation as the gold-standard version control platform? Without a board of directors, or at least an advisory board, this might not be answerable (or even decidable). That role has been taken up, by intent and/or because it has to be done, by our quite awesome committers.
This last two point put a serious amount of pressure for compatibility on both the customer and the git dev team to keep compatibility in the latest release with all platforms, especially the exotic platforms - mine included - although dropping those is not a good approach either. As an example (not intended as guidance) If we said something like git 2.40.0 is LTS until September 2026, and security fixes (and critical functional ones) will be done going back to that version, and anything older requires an extended fee-based support contract, I think it would make some organizations more comfortable with the support model. This is not a panacea and there are some obviously difficult concerns here - causing git's support model to vary from some Linux distro models from the earliest inception of both products.
I don't have a good answer to any of this that would satisfy everyone. I'm not sure there is one.
--Randall
^ permalink raw reply
* Re: [PATCH v2 00/12] Group reffiles tests
From: Junio C Hamano @ 2024-01-23 0:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: John Cai via GitGitGadget, git, John Cai
In-Reply-To: <Za5TW-q4cKS8pNNc@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> I've got two minor nits, but other than that this looks good to me. I've
> also verified that all tests continue to pass with the current version
> of the reftable backend.
OK. I've squashed all the nits from you and Karthik into the copy
in my tree. If there is nothing else, let's declare a victory and
merge the topic down to 'next' soonish.
> There's a minor merge conflict with db4192c364 (t: mark tests regarding
> git-pack-refs(1) to be backend specific, 2024-01-10). This conflict
> comes from the fact that both patch series add the REFFILES prereq to
> t3210, semantically the changes are the same. So it doesn't quite matter
> which of both versions we retain as they both do the same.
Yup, that is what I've been resolving them.
Thanks.
^ permalink raw reply
* Re: [PATCH 05/10] sequencer: use the trailer iterator
From: Linus Arver @ 2024-01-22 23:22 UTC (permalink / raw)
To: Linus Arver via GitGitGadget, git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder
In-Reply-To: <fd4a9d54d9522973a4c22e43cb1d7964033d4837.1704869487.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/trailer.h b/trailer.h
> index 50f70556302..d50c9fd79b2 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -127,6 +127,19 @@ struct trailer_iterator {
> struct strbuf key;
> struct strbuf val;
>
> + /*
> + * Raw line (e.g., "foo: bar baz") before being parsed as a trailer
> + * key/val pair. This field can contain non-trailer lines because it's
> + * valid for a trailer block to contain such lines (i.e., we only
> + * require 25% of the lines in a trailer block to be trailer lines).
> + */
> + struct strbuf raw;
Originally I used a strbuf here for consistency with the other strbufs
used in the iterator for the key and val members. But now I've realized
that there's no need to make "raw" a strbuf at all, because iterator
users will never need to manipulate the string that this points to. Will
change to just "const char *" in the reroll.
^ 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