* Re: [PATCH 2/4] completion: introduce __git_find_subcommand
From: Rubén Justo @ 2024-01-27 13:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqh6j0ngsb.fsf@gitster.g>
On 26-ene-2024 09:30:44, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> > Let's have a function to get the current subcommand when completing
> > commands that follow the syntax:
> >
> > git <command> <subcommand>
> >
> > As a convenience, let's allow an optional "default subcommand" to be
> > returned if none is found.
> >
> > Signed-off-by: Rubén Justo <rjusto@gmail.com>
> > ---
> > contrib/completion/git-completion.bash | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 916e137021..5f2e904b56 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -575,6 +575,26 @@ __gitcomp_subcommand ()
> > fi
> > }
> >
> > +# Find the current subcommand for commands that follow the syntax:
> > +#
> > +# git <command> <subcommand>
> > +#
> > +# 1: List of possible subcommands.
> > +# 2: Optional subcommand to return when none is found.
> > +__git_find_subcommand ()
> > +{
> > + local subcommand subcommands="$1" default_subcommand="$2"
>
> Are the callers expected to form "$1" by concatenating known tokens
> with a space?
>
> I am just wondering if we can avoid looping, e.g.
>
> local nextword=${words[__git_cmd_idx+1]}
> case " $subcommands " in
> *" $nextword "*)
> echo "$nextword"
> return
> ;;
> esac
>
I like the idea; and it works:
$ words=("a" "b"); __git_cmd_idx=0; __git_find_subcommand "a b" "test"
b
$ : simulate that the user moves the cursor backwards
$ words=("a" "" "b"); __git_cmd_idx=0; __git_find_subcommand "a b" "test"
test
$ : simulate that the user moves the cursor backwards
$ words=("a" " " "b"); __git_cmd_idx=0; __git_find_subcommand "a b" "test"
test
But functions like __git_find_on_cmdline or __git-find_last_on_cmdline
are already iterating. I feel we should keep the loop.
Thank you.
^ permalink raw reply
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Sergey Organov @ 2024-01-27 13:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren, git
In-Reply-To: <xmqqzfwrjdul.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> I'm still arguing in favor of fixing "-n", and I believe a fix is needed
>> independently from decision about "-f -f".
>
> Even though I do not personally like it, I do not think "which
> between do-it (f) and do-not-do-it (n) do you want to use?" is
> broken.
Well, you are right, but "-n" is not documented as "do-not-do-it" in the
sense you use it here.
> It sometimes irritates me to find "git clean" (without "-f"
> or "-n", and with clean.requireForce not disabled) complain, and I
> personally think "git clean" when clean.requireForce is in effect
> and no "-n" or "-f" were given should pretend as if "-n" were given.
> I wish if it were "without -n or -f, we pretend as if -n were given,
> possibly with a warning that says 'you need -f if you actually want
> to carry out these operations'".
Yep, then we'd not need "-n" that much, only if to cancel explicit "-f"
(provided "-f -f" feature is removed.)
>
> But that is a separate usability issue.
Yep, and that'd be very different design.
>
> What I find broken is that giving one 'f' and one 'n' in different
> order, i.e. "-f -n" and "-n -f", does not do what I expect. If you
> are choosing between do-it (f) and do-not-do-it (n), you ought to be
> able to rely on the usual last-one-wins rule. That I find broken.
I fail to see where this expectation comes from, provided "-n" is not
documented as anything opposed to "-f":
-n, --dry-run
Don’t actually remove anything, just show what would be done.
This is typical convenient description of "dry run", and current "-n"
implementation is rather close to the description, that I'd still
rewrite to emphasize the primary goal of the --dry-run:
-n, --dry-run
Show what would be done, and don’t actually remove anything.
With these descriptions, the last thing that I'd expect is "-n -f"
removing my files.
Overall, as I see it, we have buggy implementation of suitably
documented "--dry-run" option, and the best course is to fix the
bug, with no semantic changes to the option itself.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: add two steps
From: Johannes @ 2024-01-27 15:27 UTC (permalink / raw)
To: Beat Bolli, git
In-Reply-To: <a420cdc5-8294-4ffb-9f6c-06e1416ada43@drbeat.li>
I'm talking about installing git from git-scm.org. That _is_ a software
that is installed.
On 27/01/2024 13:23, Beat Bolli wrote:
> On 27.01.24 10:43, Johannes Kingma wrote:
>> For git to work at least a user.name and user.email globals are
>> needed. would it make sense to include this in the installation process?
>>
>
> The Git projects only distributes source code, so there's no
> installation process as such that could be improved.
^ permalink raw reply
* Re: add two steps
From: Beat Bolli @ 2024-01-27 16:31 UTC (permalink / raw)
To: Johannes, git
In-Reply-To: <e64ea18e-a733-4a01-af1e-22a7f9d974dd@king.ma>
On 27.01.24 16:27, Johannes wrote:
Hi Johannes,
> On 27/01/2024 13:23, Beat Bolli wrote:
>> On 27.01.24 10:43, Johannes Kingma wrote:
>>> For git to work at least a user.name and user.email globals are
>>> needed. would it make sense to include this in the installation process?
>>>
>>
>> The Git projects only distributes source code, so there's no
>> installation process as such that could be improved. > I'm talking about installing git from git-scm.org. That _is_ a
> software that is installed.
None of these installers are created by the Git project itself. What
operating system are you installing on?
--
Cheers, Beat
^ permalink raw reply
* Re: [PATCH 2/4] docs: Clean up `--empty` formatting in `git-rebase` and `git-am`
From: Brian Lyles @ 2024-01-27 21:22 UTC (permalink / raw)
To: phillip.wood; +Cc: git, me, newren
In-Reply-To: <192892be-262c-487e-bb1d-6e50c01e2d66@gmail.com>
Hi Phillip
On Tue, Jan 23, 2024 at 8:24 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 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?
Hm -- as you mention, this does appear to have a different meaning for
git-am(1) than it does for git-rebase(1). Regardless of the `--empty`
value passed to git-am(1), a non-empty patch that is already present
appears to error and stop.
That is an unfortunate difference. I think that my updated version of
the git-am(1) docs is still easier to read, and preserves the original
meaning. So I'm inclined to say that it's still an improvement worth
making, and perhaps my commit message should just clarify that.
Thoughts?
> If you're aiming for consistency then it would be worth listing the
> possible values in the same order for each command.
That makes sense. I had initially maintained the existing order in which
these were documented, keeping the default option first. I think that
the updated layout makes the order less relevant by making it easier to
read and identify the default anyway.
I could see alphabetical being better, though with the changes later in
this series we'd end up with the deprecated `ask` being first or
out-or-order at the end. What are your thoughts on the ideal order for
these?
> > 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.
I could see that -- I'll adjust this for v2 of the patch.
> > +`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.
The two lines indicating this behavior are actually pre-existing -- I
did not change them in this patch and thus didn't even think to fact
check them.
Upon testing this, I've confirmed that you are correct about the actual
behavior. I will address this in a separate commit in v2.
Thanks,
Brian Lyles
^ permalink raw reply
* Re: [PATCH 3/4] rebase: Update `--empty=ask` to `--empty=drop`
From: Brian Lyles @ 2024-01-27 21:49 UTC (permalink / raw)
To: phillip.wood; +Cc: git, me, newren
In-Reply-To: <7032ae96-602f-499d-8430-a5dc3864d1bb@gmail.com>
Hi Phillip
On Tue, Jan 23, 2024 at 8:24 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 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.
The suggestion to use 'stop' instead of 'ask' for rebase was initially
Elijah's[1], which I agreed with. I am certainly open to others'
opinions here though, and am content with whatever is decided. I am
mostly aiming for consistency between git-rebase(1), git-am(1), and
ultimately git-cherry-pick(1).
[1]: https://lore.kernel.org/git/CABPp-BGJfvBhO_zEX8nLoa8WNsjmwvtZ2qOjmYm9iPoZg4SwPw@mail.gmail.com/
> 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
You are correct -- the intent being to ensure that `--ask` continues
working for as long as it is supported. I'll add this to the message in
v2.
> > 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?
That makes sense -- I will include a warning for this in v2.
Thanks,
Brian Lyles
^ permalink raw reply
* Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: Brian Lyles @ 2024-01-27 23:30 UTC (permalink / raw)
To: phillip.wood; +Cc: git, me, newren, gitster
In-Reply-To: <7229fe62-cf9b-4829-80ec-c6b44c52163e@gmail.com>
[+cc Junio]
Hi Phillip
On Tue, Jan 23, 2024 at 8:23 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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.
Thank you for the kind words!
> 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.
I struggled a bit with this initially because the motivation behind the
change in this particular commit was driven by a technical issue in my
mind. The side-effect with git-cherry-pick(s) `--allow-empty` and
`--keep-redundant-commits` was mildly problematic, but less concerning
that the future problem that we'd have once git-cherry-pick(1) got the
more robust `--empty` option in a later commit in this series.
I think my problem came down to this commit trying to solve two problems
at once -- the underlying technical concern _and_ the git-cherry-pick(1)
behavior.
In v2, I intend to break this commit into two:
- Update `allow_empty()` to not require `allow_empty`, but without
actually changing any consumers (and thus without making any
functional change)
- Update git-cherry-pick(1) such that `--keep-redundant-commits` no
longer implies `--allow-empty`.
This allows me to better justify the technical change technically and
the functional change functionally, while also making it easier to drop
the functional change if we decide that a breaking change is not
warranted to address this.
> 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.
That practical example is documented in the initial discussion[1], which
I should have ought to have linked in a cover letter for this series
(and will do so in v2). I'll avoid copying the details here, but we'd
very much like to be able to programmatically drop the commits that
become empty when doing the automated cherry-pick described there.
[1]: https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=Cd8Q@mail.gmail.com/
> 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().
Just noting here for future readers here that you sent a followup email
indicating this was not viable:
> On Wed, Jan 24, 2024 at 5:01 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Having thought about this again I don't think we can reuse the same
> approach as rebase because cherry-pick and rebase have different
> behaviors. "git rebase --no-keep-empty" drops empty commits whereas "git
> cherry-pick" wants to error out if it sees an empty commit. So I think
> your approach of reworking allow_empty() in the sequencer is the right
> approach.
>
> Sorry for the confusion
No worries. If you have any suggestions for how I might better explain
these changes in the commit message, please let me know.
> > 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.
I think you're right -- Previously the error could not have been hit,
but now it can. An error is still an error, and we should handle it
regardless of how `allow_empty` was set. I'll address this in v2 by
simply returning the error.
> > 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.
I totally understand being wary here.
I've certainly convinced myself that having the future `--empty=drop`
behavior introduced later in this patch should not imply
`--allow-empty`.
I also _think_ that the existing behavior of `--keep-redundant-commits`
is probably technically not ideal or correct, but could be convinced
that changing it now is not worthwhile. I will defer to group consensus
here.
> > 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.
Absolutely, will do in v2.
> > [...]> +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).
Thanks for the tip, I'll add this in v2.
On Wed, Jan 24, 2024 at 5:01 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> Here are some code comments now I've realized why we need to change it
>
> On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
> > From: Brian Lyles <brianmlyles@gmail.com>
> > Documentation/git-cherry-pick.txt | 10 +++++++---
> > builtin/revert.c | 4 ----
> > sequencer.c | 18 ++++++++++--------
> > t/t3505-cherry-pick-empty.sh | 5 +++++
> > 4 files changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/Documentation/git-cherry-pick.txt
> b/Documentation/git-cherry-pick.txt
> > index fdcad3d200..806295a730 100644
> > --- a/Documentation/git-cherry-pick.txt
> > +++ b/Documentation/git-cherry-pick.txt
> > @@ -131,8 +131,8 @@ effect to your index in a row.
> > even without this option. 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`.
> > + previous commit will cause the cherry-pick to fail. To force the
> > + inclusion of those commits use `--keep-redundant-commits`.
> >
> > --allow-empty-message::
> > By default, cherry-picking a commit with an empty message will
> fail.
> > @@ -144,7 +144,11 @@ effect to your index in a row.
> > 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`.
> > + creates an empty commit object. 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`.
> >
> > --strategy=<strategy>::
> > Use the given merge strategy. Should only be used once.
> > diff --git a/builtin/revert.c b/builtin/revert.c
> > index e6f9a1ad26..b2cfde7a87 100644
> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -136,10 +136,6 @@ 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;
> >
> > - /* implies allow_empty */
> > - if (opts->keep_redundant_commits)
> > - opts->allow_empty = 1;
>
> I'm wary of this, especially after Juino's comments in
> https://lore.kernel.org/git/xmqqy1cfnca7.fsf@gitster.g/
As noted above, I've split this commit into two, and am open to
discussing dropping the functional change to `--keep-redundant-commits`
> > 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 d584cac8ed..582bde8d46 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -1739,22 +1739,24 @@ static int allow_empty(struct repository *r,
> > *
> > * (4) we allow both.
> > */
>
> The comment above should be updated if we change the behavior of this
> function.
Of course, good catch.
> I don't think we want to hide the error here or below from
> originally_empty()
>
> > if (!index_unchanged)
> > return 0; /* we do not have to say --allow-empty */
> >
> > - if (opts->keep_redundant_commits)
> > - return 1;
> > -
Agreed, will address in v2 as mentioned above.
Thanks,
Brian Lyles
^ permalink raw reply
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Brian Lyles @ 2024-01-27 23:56 UTC (permalink / raw)
To: phillip.wood; +Cc: git, me, newren, gitster
In-Reply-To: <b897771e-c60e-4e41-bfae-3bcfdd832be1@gmail.com>
[+cc Junio]
On Tue, Jan 23, 2024 at 8:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Brian
>
>
> On 19/01/2024 05:59, brianmlyles@gmail.com wrote:
>
> > 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
This is also related to Junio's comment:
> On Tue, Jan 23, 2024 at 12:01 PM Junio C Hamano <gitster@pobox.com>
wrote:
>
> True, especially since --empty=keep is much less descriptive and the
> part after "note that ..." below will take a long time before
> sticking in readers' brain.
My primary motivation here was simply for consistency with `--empty` for
both git-rebase(1) and git-am(1). In theory, I am not opposed to
updating this patch to instead simply add a `--drop-redundant-commits`
option if we feel that provides better usability. However, I think that
the consistency would be better.
I will happily defer to the group consensus here, with the options I see
being:
1. No deprecation: just make `--keep-redundant-commits` a synonym of
`--empty=keep`
2. Soft deprecation: Give a warning when `--keep-redundant-commits` is
used
3. Add `--drop-redundant-commits` instead of `--empty`
My preference would be 2, followed by 1 and then 3.
> 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".
Oh, thank you for catching that. The cause here was actually because I
had initially written these tests prior to adding the "ask -> stop"
change rather than familiarity. I simply failed to update the tests
after moving things around.
> 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.
It looks like `--keep-redundant-commits` was not being included in these
checks previously. I suspect that to be an oversight though.
I can add this for v2.
>
> > 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.
I can add a test for this in v2
> > 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.
An excellent catch, thank you. Will be addressed in v2
> > +'
> > +
> > +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
Thanks for pointing that function out -- you're absolutely right. Will
be addressed in v2.
Thanks for the review,
Brian Lyles
^ permalink raw reply
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Brian Lyles @ 2024-01-28 0:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, git, me, newren
In-Reply-To: <xmqq8r4gnd3c.fsf@gitster.g>
Hi Junio
On Tue, Jan 23, 2024 at 12:01 PM Junio C Hamano <gitster@pobox.com> wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > 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
>
> True, especially since --empty=keep is much less descriptive and the
> part after "note that ..." below will take a long time before
> sticking in readers' brain.
I responded to this in my reply to Phillip, and CC'd you there.
> >> +--empty=(stop|drop|keep)::
> >> + How to handle commits being cherry-picked that are redundant with
> >> + changes already in the current history.
>
> It might make it easier to understand if we moved the description in
> 'keep' that says something about --allow-empty here, as it should
> apply to other two choices if I understand correctly. Let me try:
>
> This option specifies how a commit that is not originally empty
> but becomes a no-op when cherry-picked due to earlier changes
> already applied or already in the current history. Regardless
> of this this option, `cherry-pick` will fail on a commit that is
> empty in the original history---see `--allow-empty` to pass them
> intact.
>
> or something. Then the description of `keep` can become just as
> short as other two, e.g. a single sentence "The commit that becomes
> empty will be kept".
Thank you for this suggestion. You are correct that the difference
between `--empty` and `allow-empty` is relevant regardless of the option
selected by the user.
In fact, this entire tidbit is somewhat duplicative with the note I
already added after the options:
> Note that commits which start empty will cause the cherry-pick to fail
> (unless `--allow-empty` is specified).
I'll clean this up in v2. Here's what I am thinking currently:
> --empty=(stop|drop|keep)::
> How to handle commits being cherry-picked that are redundant with
> changes already in the current history.
> +
> --
> `stop`;;
> The cherry-pick will stop when the commit is applied, allowing
> you to examine the commit. This is the default behavior.
> `drop`;;
> The commit will be dropped.
> `keep`;;
> The commit will be kept.
> --
> +
> Note that this option species how to handle a commit that 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`.
> +
Thank you,
Brian Lyles
^ permalink raw reply
* Re: [PATCH v4 2/2] patch-id: replace `atoi()` with `strtol_i_updated()`
From: Mohit Marathe @ 2024-01-28 4:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Mohit Marathe via GitGitGadget, git, Mohit Marathe
In-Reply-To: <xmqqttn2bg2o.fsf@gitster.g>
On Thursday, January 25th, 2024 at 2:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Mohit Marathe via GitGitGadget" gitgitgadget@gmail.com writes:
>
> > q = p + 4;
> > n = strspn(q, digits);
> > if (q[n] == ',') {
> > q += n + 1;
>
>
> So, we saw "@@ -" and skipped over these four bytes, skipped the
> digits from there, and found a comma.
>
> For "@@ -29,14 +30,18 @@", for example, our q is now "14 +30,18 @@"
> as we have skipped over that comma after 29.
>
> > - *p_before = atoi(q);
> > + if (strtol_i_updated(q, 10, p_before, &endp) != 0)
> > + return 0;
>
>
> We parse out 14 and store it to *p_before. endp points at " +30..."
> now.
>
> > n = strspn(q, digits);
> > + if (endp != q + n)
> > + return 0;
>
>
> Is this necessary? By asking strtol_i_updated() where the number ended,
> we already know endp without skipping the digits in q with strspn().
> Shouldn't these three lines become more like
>
> n = endp - q;
>
> instead?
>
> After all, we are not trying to find a bug in strtol_i_updated(),
> which would be the only reason how this "return 0" would trigger.
>
I was confused about how an invalid hunk header of a corrupted would
look like. This was just an attempt of making a sanity check. But after
taking another look, I agree that its unnecessary.
> > } else {
> > *p_before = 1;
> > }
> > @@ -48,8 +53,11 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
> > n = strspn(r, digits);
> > if (r[n] == ',') {
> > r += n + 1;
> > - *p_after = atoi(r);
> > + if (strtol_i_updated(r, 10, p_after, &endp) != 0)
> > + return 0;
> > n = strspn(r, digits);
> > + if (endp != r + n)
> > + return 0;
>
>
> Likewise.
>
> > } else {
> > *p_after = 1;
> > }
^ permalink raw reply
* [PATCH v5 0/2] Replace atoi() with strtoi_with_tail()
From: Mohit Marathe via GitGitGadget @ 2024-01-28 4:42 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe
In-Reply-To: <pull.1646.v4.git.1706079304.gitgitgadget@gmail.com>
Hello,
This patch series replaces atoi() with an updated version of strtol_i()
called strtoi_with_tail (Credits: Junio C Hamano). The reasoning behind this
is to improve error handling by not allowing non-numerical characters in the
hunk header (which might happen in case of a corrupt patch, although
rarely).
There is still a change to be made, as Junio says: "A corrupt patch may be
getting a nonsense patch-ID with the current code and hopefully is not
matching other patches that are not corrupt, but with such a change, a
corrupt patch may not be getting any patch-ID and a loop that computes
patch-ID for many files and try to match them up might need to be rewritten
to take the new failure case into account." I'm not sure where this change
needs to me made (maybe get_one_patchid()?). It would be great if anyone
could point me to the correct place.
Thanks, Mohit Marathe
Mohit Marathe (2):
git-compat-util: add strtoi_with_tail()
patch-id: replace `atoi()` with `strtoi_with_tail`
builtin/patch-id.c | 12 ++++++++----
git-compat-util.h | 23 +++++++++++++++++++++++
2 files changed, 31 insertions(+), 4 deletions(-)
base-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1646%2Fmohit-marathe%2Fupdate-strtol_i-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1646/mohit-marathe/update-strtol_i-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1646
Range-diff vs v4:
1: 60ea85a701a ! 1: f09b0838f04 git-compat-util: add strtol_i_updated()
@@ Metadata
Author: Mohit Marathe <mohitmarathe23@gmail.com>
## Commit message ##
- git-compat-util: add strtol_i_updated()
+ git-compat-util: add strtoi_with_tail()
This function is an updated version of strtol_i() function. It will
give more control to handle parsing of the characters after the
- integer and better error handling while parsing numbers.
+ numbers and better error handling while parsing numbers.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
@@ git-compat-util.h: static inline int strtol_i(char const *s, int base, int *resu
return 0;
}
-+#define strtol_i(s,b,r) strtol_i_updated((s), (b), (r), NULL)
-+static inline int strtol_i_updated(char const *s, int base, int *result, char **endp)
++#define strtol_i(s,b,r) strtoi_with_tail((s), (b), (r), NULL)
++static inline int strtoi_with_tail(char const *s, int base, int *result, char **endp)
+{
+ long ul;
+ char *dummy = NULL;
2: 17f2dda4907 ! 2: ee8f4ae991d patch-id: replace `atoi()` with `strtol_i_updated()`
@@ Metadata
Author: Mohit Marathe <mohitmarathe23@gmail.com>
## Commit message ##
- patch-id: replace `atoi()` with `strtol_i_updated()`
+ patch-id: replace `atoi()` with `strtoi_with_tail`
The change is made to improve the error-handling capabilities
- during the conversion of string representations to integers.
- The `strtol_i_updated(` function offers a more robust mechanism for
+ during the conversion of string to integers. The
+ `strtoi_with_tail` function offers a more robust mechanism for
converting strings to integers by providing enhanced error
- detection. Unlike `atoi(`, `strtol_i_updated(` allows the code to
+ detection. Unlike `atoi`, `strtoi_with_tail` allows the code to
differentiate between a valid conversion and an invalid one,
offering better resilience against potential issues such as
reading hunk header of a corrupted patch.
@@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, in
if (q[n] == ',') {
q += n + 1;
- *p_before = atoi(q);
-+ if (strtol_i_updated(q, 10, p_before, &endp) != 0)
-+ return 0;
- n = strspn(q, digits);
-+ if (endp != q + n)
+- n = strspn(q, digits);
++ if (strtoi_with_tail(q, 10, p_before, &endp) != 0)
+ return 0;
++ n = endp - q;
} else {
*p_before = 1;
}
@@ builtin/patch-id.c: static int scan_hunk_header(const char *p, int *p_before, in
if (r[n] == ',') {
r += n + 1;
- *p_after = atoi(r);
-+ if (strtol_i_updated(r, 10, p_after, &endp) != 0)
-+ return 0;
- n = strspn(r, digits);
-+ if (endp != r + n)
+- n = strspn(r, digits);
++ if (strtoi_with_tail(r, 10, p_after, &endp) != 0)
+ return 0;
++ n = endp - r;
} else {
*p_after = 1;
}
--
gitgitgadget
^ permalink raw reply
* [PATCH v5 1/2] git-compat-util: add strtoi_with_tail()
From: Mohit Marathe via GitGitGadget @ 2024-01-28 4:42 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe, Mohit Marathe
In-Reply-To: <pull.1646.v5.git.1706416952.gitgitgadget@gmail.com>
From: Mohit Marathe <mohitmarathe23@gmail.com>
This function is an updated version of strtol_i() function. It will
give more control to handle parsing of the characters after the
numbers and better error handling while parsing numbers.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
---
git-compat-util.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 7c2a6538e5a..c576b1b104f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1309,6 +1309,29 @@ static inline int strtol_i(char const *s, int base, int *result)
return 0;
}
+#define strtol_i(s,b,r) strtoi_with_tail((s), (b), (r), NULL)
+static inline int strtoi_with_tail(char const *s, int base, int *result, char **endp)
+{
+ long ul;
+ char *dummy = NULL;
+
+ if (!endp)
+ endp = &dummy;
+ errno = 0;
+ ul = strtol(s, endp, base);
+ if (errno ||
+ /*
+ * if we are told to parse to the end of the string by
+ * passing NULL to endp, it is an error to have any
+ * remaining character after the digits.
+ */
+ (dummy && *dummy) ||
+ *endp == s || (int) ul != ul)
+ return -1;
+ *result = ul;
+ return 0;
+}
+
void git_stable_qsort(void *base, size_t nmemb, size_t size,
int(*compar)(const void *, const void *));
#ifdef INTERNAL_QSORT
--
gitgitgadget
^ permalink raw reply related
* [PATCH v5 2/2] patch-id: replace `atoi()` with `strtoi_with_tail`
From: Mohit Marathe via GitGitGadget @ 2024-01-28 4:42 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe, Mohit Marathe
In-Reply-To: <pull.1646.v5.git.1706416952.gitgitgadget@gmail.com>
From: Mohit Marathe <mohitmarathe23@gmail.com>
The change is made to improve the error-handling capabilities
during the conversion of string to integers. The
`strtoi_with_tail` function offers a more robust mechanism for
converting strings to integers by providing enhanced error
detection. Unlike `atoi`, `strtoi_with_tail` allows the code to
differentiate between a valid conversion and an invalid one,
offering better resilience against potential issues such as
reading hunk header of a corrupted patch.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
---
builtin/patch-id.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3894d2b9706..4e9a301e9fb 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,3 +1,4 @@
+#include "git-compat-util.h"
#include "builtin.h"
#include "config.h"
#include "diff.h"
@@ -29,14 +30,16 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
{
static const char digits[] = "0123456789";
const char *q, *r;
+ char *endp;
int n;
q = p + 4;
n = strspn(q, digits);
if (q[n] == ',') {
q += n + 1;
- *p_before = atoi(q);
- n = strspn(q, digits);
+ if (strtoi_with_tail(q, 10, p_before, &endp) != 0)
+ return 0;
+ n = endp - q;
} else {
*p_before = 1;
}
@@ -48,8 +51,9 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
n = strspn(r, digits);
if (r[n] == ',') {
r += n + 1;
- *p_after = atoi(r);
- n = strspn(r, digits);
+ if (strtoi_with_tail(r, 10, p_after, &endp) != 0)
+ return 0;
+ n = endp - r;
} else {
*p_after = 1;
}
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2 09/10] trailer: move arg handling to interpret-trailers.c
From: Linus Arver @ 2024-01-28 5:01 UTC (permalink / raw)
To: Linus Arver via GitGitGadget, git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker
In-Reply-To: <1b4bdde65bcb375ce9ef2345814330df92e6d932.1706308737.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/trailer.h b/trailer.h
> index c6aa96dd6be..e01437160cf 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -46,9 +46,20 @@ struct new_trailer_item {
> enum trailer_if_missing if_missing;
> };
>
> +void trailer_conf_set(enum trailer_where where,
> + enum trailer_if_exists if_exists,
> + enum trailer_if_missing if_missing,
> + struct trailer_conf *conf);
> +
> +struct trailer_conf *new_trailer_conf(void);
> void duplicate_trailer_conf(struct trailer_conf *dst,
> const struct trailer_conf *src);
>
> +const char *default_separators(void);
> +
> +void add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
> + struct list_head *arg_head);
Looks like I forgot to add a "trailer" keyword for these API functions.
Will rename to "trailer_default_separators()" and
"trailer_add_arg_item()" in next reroll.
^ permalink raw reply
* Re: [PATCH v2 09/10] trailer: move arg handling to interpret-trailers.c
From: Linus Arver @ 2024-01-28 6:39 UTC (permalink / raw)
To: Linus Arver via GitGitGadget, git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker
In-Reply-To: <1b4bdde65bcb375ce9ef2345814330df92e6d932.1706308737.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> We don't move the "arg_item" struct to interpret-trailers.c, because it
> is now a struct that contains information about trailers that should be
> injected into the input text's own trailers. We will rename this
> language as such in a follow-up patch to keep the diff here small.
I forgot to add this follow-up patch in this series. That will be patch
11 in v3, which renames "arg_item" to "trailer_template" (the idea being
that trailer templates are used to create new trailers, using the
template as a guide).
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 9e41fa20b5f..ad9b9a9f9ef 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -45,23 +45,17 @@ static int option_parse_if_missing(const struct option *opt,
> return trailer_set_if_missing(opt->value, arg);
> }
>
> -static void new_trailers_clear(struct list_head *trailers)
> -{
> - struct list_head *pos, *tmp;
> - struct new_trailer_item *item;
> -
> - list_for_each_safe(pos, tmp, trailers) {
> - item = list_entry(pos, struct new_trailer_item, list);
> - list_del(pos);
> - free(item);
> - }
> -}
This function will also get promoted to the API, and will be renamed to
"free_new_trailers()" initially in patch 9, but renamed to
"free_trailer_templates()" in patch 11 (we wait until patch 11 to
introduce the term "template").
^ permalink raw reply
* Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: Brian Lyles @ 2024-01-28 16:36 UTC (permalink / raw)
To: phillip.wood; +Cc: git, me, newren, gitster
In-Reply-To: <CAHPHrSeKY2Ou9VCq6rtADTOwycc0KCTPaCCwtqf94yLi0wj0OQ@mail.gmail.com>
[+cc Junio]
On Sat, Jan 27, 2024 at 5:30 PM Brian Lyles <brianmlyles@gmail.com> wrote:
> > > 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.
>
> I think you're right -- Previously the error could not have been hit,
> but now it can. An error is still an error, and we should handle it
> regardless of how `allow_empty` was set. I'll address this in v2 by
> simply returning the error.
As I dig into this more, I'm noticing that this may have unintended side
effects that I'm unsure of. After making this change, I noticed a couple
of failures in the cherry-pick test suite. The others may be a knock-on
of this initial failure:
expecting success of 3501.8 'cherry-pick on unborn branch':
git checkout --orphan unborn &&
git rm --cached -r . &&
rm -rf * &&
git cherry-pick initial &&
git diff --quiet initial &&
test_cmp_rev ! initial HEAD
A extra_file
Switched to a new branch 'unborn'
rm 'extra_file'
rm 'spoo'
error: could not resolve HEAD commit
fatal: cherry-pick failed
not ok 8 - cherry-pick on unborn branch
#
# git checkout --orphan unborn &&
# git rm --cached -r . &&
# rm -rf * &&
# git cherry-pick initial &&
# git diff --quiet initial &&
# test_cmp_rev ! initial HEAD
#
It looks like this is caused specifically by not hiding the error from
`index_unchanged`
index_unchanged = is_index_unchanged(r);
if (index_unchanged < 0) {
return index_unchanged;
}
At this point, my inexperience with the git codebase and these more edge
case scenarios starts to show. I'm unsure how to best approach this. It
seems that exposing these errors when `allow_empty` is not set causes
the entire cherry-pick to fail in situations where it would not
previously. Here is what that same test looks like prior to any of my
changes from this series:
expecting success of 3501.8 'cherry-pick on unborn branch':
git checkout --orphan unborn &&
git rm --cached -r . &&
rm -rf * &&
git cherry-pick initial &&
git diff --quiet initial &&
test_cmp_rev ! initial HEAD
A extra_file
Switched to a new branch 'unborn'
rm 'extra_file'
rm 'spoo'
[unborn 38e6d75] initial
Author: A U Thor <author@example.com>
Date: Thu Apr 7 15:13:13 2005 -0700
1 file changed, 15 insertions(+)
create mode 100644 oops
ok 8 - cherry-pick on unborn branch
Conceptually I definitely agree that it seems odd to hide these errors
just because `allow_empty` was not set, but I fear this to be a breaking
change for which I don't have a good understanding of the impact.
Any guidance here would be appreciated.
Thank you,
Brian Lyles
^ permalink raw reply
* [GSOC][RFC PATCH 0/2] add-patch: compare object id
From: Ghanshyam Thakkar @ 2024-01-28 18:11 UTC (permalink / raw)
To: git; +Cc: Ghanshyam Thakkar
This patch series is RFC as this contains a partial behavior change.
The first patch introduces a new function to compare object id instead
of revision strings. The second patch removes the special case for HEAD
in builtin/checkout.c.
This patch series enables for HEAD to be specified in any form and
removes need to be kept a literal string 'HEAD'. Consequently, this
removes the need to keep a special case for HEAD in callers of
run_add_p() (as in builtin/checkout.c).
Additional context:
https://lore.kernel.org/git/xmqqsgat7ttf.fsf@gitster.c.googlers.com/
Ghanshyam Thakkar (2):
add-patch: compare object id instead of literal string
checkout: remove HEAD special case
add-patch.c | 28 +++++++++++++++-------
builtin/checkout.c | 9 ++++---
t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------
t/t2071-restore-patch.sh | 21 ++++++++++------
t/t7105-reset-patch.sh | 14 +++++++++++
5 files changed, 81 insertions(+), 41 deletions(-)
--
2.43.0
^ permalink raw reply
* [RFC PATCH 1/2] add-patch: compare object id instead of literal string
From: Ghanshyam Thakkar @ 2024-01-28 18:11 UTC (permalink / raw)
To: git; +Cc: Ghanshyam Thakkar
In-Reply-To: <20240128181202.986753-2-shyamthakkar001@gmail.com>
Add a new function reveq(), which takes repository struct and two revision
strings as arguments and returns 0 if the revisions point to the same
object. Passing a rev which does not point to an object is considered
undefined behavior as the underlying function memcmp() will be called
with NULL hash strings.
Subsequently, replace literal string comparison to HEAD in run_add_p()
with reveq() to handle more ways of saying HEAD (such as '@' or '$branch'
where $branch points to same commit as HEAD). This addresses the
NEEDSWORK comment in run_add_p().
However, in ADD_P_RESET mode keep string comparison in logical OR with
reveq() to handle unborn HEAD.
As for the behavior change, with this patch applied if the given
revision points to the same object as HEAD, the patch mode will be set to
patch_mode_(reset,checkout,worktree)_head instead of
patch_mode_(...)_nothead. That is equivalent of not setting -R flag in
diff-index, which would have been otherwise set before this patch.
However, when given same set of inputs, the actual outcome is same as
before this patch. Therefore, this does not affect any automated scripts.
Also, add testcases to check the similarity of result between different
ways of saying HEAD.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
Should the return values of repo_get_oid() be checked in reveq()? As
reveq() is not a global function and is only used in run_add_p(), the
validity of revisions is already checked beforehand by builtin/checkout.c
and builtin/reset.c before the call to run_add_p().
add-patch.c | 28 +++++++++++++++-------
t/t2016-checkout-patch.sh | 50 +++++++++++++++++++++++----------------
t/t2071-restore-patch.sh | 21 ++++++++++------
t/t7105-reset-patch.sh | 14 +++++++++++
4 files changed, 77 insertions(+), 36 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index 79eda168eb..01eb71d90e 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -14,6 +14,7 @@
#include "color.h"
#include "compat/terminal.h"
#include "prompt.h"
+#include "hash.h"
enum prompt_mode_type {
PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,
@@ -316,6 +317,18 @@ static void setup_child_process(struct add_p_state *s,
INDEX_ENVIRONMENT "=%s", s->s.r->index_file);
}
+// Check if two revisions point to the same object. Passing a rev which does not
+// point to an object is undefined behavior.
+static inline int reveq(struct repository *r, const char *rev1,
+ const char *rev2)
+{
+ struct object_id oid_rev1, oid_rev2;
+ repo_get_oid(r, rev1, &oid_rev1);
+ repo_get_oid(r, rev2, &oid_rev2);
+
+ return !oideq(&oid_rev1, &oid_rev2);
+}
+
static int parse_range(const char **p,
unsigned long *offset, unsigned long *count)
{
@@ -1730,28 +1743,25 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
s.mode = &patch_mode_stash;
else if (mode == ADD_P_RESET) {
/*
- * NEEDSWORK: Instead of comparing to the literal "HEAD",
- * compare the commit objects instead so that other ways of
- * saying the same thing (such as "@") are also handled
- * appropriately.
- *
- * This applies to the cases below too.
+ * The literal string comparison to HEAD below is kept
+ * to handle unborn HEAD.
*/
- if (!revision || !strcmp(revision, "HEAD"))
+ if (!revision || !strcmp(revision, "HEAD") ||
+ !reveq(r, revision, "HEAD"))
s.mode = &patch_mode_reset_head;
else
s.mode = &patch_mode_reset_nothead;
} else if (mode == ADD_P_CHECKOUT) {
if (!revision)
s.mode = &patch_mode_checkout_index;
- else if (!strcmp(revision, "HEAD"))
+ else if (!reveq(r, revision, "HEAD"))
s.mode = &patch_mode_checkout_head;
else
s.mode = &patch_mode_checkout_nothead;
} else if (mode == ADD_P_WORKTREE) {
if (!revision)
s.mode = &patch_mode_checkout_index;
- else if (!strcmp(revision, "HEAD"))
+ else if (!reveq(r, revision, "HEAD"))
s.mode = &patch_mode_worktree_head;
else
s.mode = &patch_mode_worktree_nothead;
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 747eb5563e..431f34fa9c 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -12,6 +12,7 @@ test_expect_success 'setup' '
git commit -m initial &&
test_tick &&
test_commit second dir/foo head &&
+ git branch newbranch &&
set_and_save_state bar bar_work bar_index &&
save_head
'
@@ -38,26 +39,35 @@ test_expect_success 'git checkout -p with staged changes' '
verify_state dir/foo index index
'
-test_expect_success 'git checkout -p HEAD with NO staged changes: abort' '
- set_and_save_state dir/foo work head &&
- test_write_lines n y n | git checkout -p HEAD &&
- verify_saved_state bar &&
- verify_saved_state dir/foo
-'
-
-test_expect_success 'git checkout -p HEAD with NO staged changes: apply' '
- test_write_lines n y y | git checkout -p HEAD &&
- verify_saved_state bar &&
- verify_state dir/foo head head
-'
-
-test_expect_success 'git checkout -p HEAD with change already staged' '
- set_state dir/foo index index &&
- # the third n is to get out in case it mistakenly does not apply
- test_write_lines n y n | git checkout -p HEAD &&
- verify_saved_state bar &&
- verify_state dir/foo head head
-'
+# Note: 'newbranch' points to the same commit as HEAD. And it is technically
+# allowed to name a branch '@' as of now, however in below test '@'
+# represents the shortcut for HEAD.
+for opt in "HEAD" "@" "newbranch"
+do
+ test_expect_success "git checkout -p $opt with NO staged changes: abort" '
+ set_and_save_state dir/foo work head &&
+ test_write_lines n y n | git checkout -p $opt >output &&
+ verify_saved_state bar &&
+ verify_saved_state dir/foo &&
+ test_grep "Discard" output
+ '
+
+ test_expect_success "git checkout -p $opt with NO staged changes: apply" '
+ test_write_lines n y y | git checkout -p $opt >output &&
+ verify_saved_state bar &&
+ verify_state dir/foo head head &&
+ test_grep "Discard" output
+ '
+
+ test_expect_success "git checkout -p $opt with change already staged" '
+ set_state dir/foo index index &&
+ # the third n is to get out in case it mistakenly does not apply
+ test_write_lines n y n | git checkout -p $opt >output &&
+ verify_saved_state bar &&
+ verify_state dir/foo head head &&
+ test_grep "Discard" output
+ '
+done
test_expect_success 'git checkout -p HEAD^...' '
# the third n is to get out in case it mistakenly does not apply
diff --git a/t/t2071-restore-patch.sh b/t/t2071-restore-patch.sh
index b5c5c0ff7e..305b4a0c4f 100755
--- a/t/t2071-restore-patch.sh
+++ b/t/t2071-restore-patch.sh
@@ -12,6 +12,7 @@ test_expect_success PERL 'setup' '
git commit -m initial &&
test_tick &&
test_commit second dir/foo head &&
+ git branch newbranch &&
set_and_save_state bar bar_work bar_index &&
save_head
'
@@ -44,13 +45,19 @@ test_expect_success PERL 'git restore -p with staged changes' '
verify_state dir/foo index index
'
-test_expect_success PERL 'git restore -p --source=HEAD' '
- set_state dir/foo work index &&
- # the third n is to get out in case it mistakenly does not apply
- test_write_lines n y n | git restore -p --source=HEAD &&
- verify_saved_state bar &&
- verify_state dir/foo head index
-'
+# Note: 'newbranch' points to the same commit as HEAD. And '@' is a
+# shortcut for HEAD.
+for opt in "HEAD" "@" "newbranch"
+do
+ test_expect_success PERL "git restore -p --source=$opt" '
+ set_state dir/foo work index &&
+ # the third n is to get out in case it mistakenly does not apply
+ test_write_lines n y n | git restore -p --source=$opt >output &&
+ verify_saved_state bar &&
+ verify_state dir/foo head index &&
+ test_grep "Discard" output
+ '
+done
test_expect_success PERL 'git restore -p --source=HEAD^' '
set_state dir/foo work index &&
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 05079c7246..65a8802b29 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -13,6 +13,7 @@ test_expect_success PERL 'setup' '
git commit -m initial &&
test_tick &&
test_commit second dir/foo head &&
+ git branch newbranch &&
set_and_save_state bar bar_work bar_index &&
save_head
'
@@ -33,6 +34,19 @@ test_expect_success PERL 'git reset -p' '
test_grep "Unstage" output
'
+# Note: '@' can technically also be used as a branch name, but in below test
+# it represents the shortcut for HEAD. And 'newbranch' points to the same
+# commit as HEAD.
+for opt in "HEAD" "@" "newbranch"
+do
+ test_expect_success PERL "git reset -p $opt" '
+ test_write_lines n y | git reset -p $opt >output &&
+ verify_state dir/foo work head &&
+ verify_saved_state bar &&
+ test_grep "Unstage" output
+ '
+done
+
test_expect_success PERL 'git reset -p HEAD^' '
test_write_lines n y | git reset -p HEAD^ >output &&
verify_state dir/foo work parent &&
--
2.43.0
^ permalink raw reply related
* [RFC PATCH 2/2] checkout: remove HEAD special case
From: Ghanshyam Thakkar @ 2024-01-28 18:11 UTC (permalink / raw)
To: git; +Cc: Ghanshyam Thakkar
In-Reply-To: <20240128181202.986753-2-shyamthakkar001@gmail.com>
run_add_p() is capable of handling HEAD in any form (e.g. hex, 'HEAD',
'@' etc.), not just string 'HEAD'. Therefore, special casing of HEAD
does not have any effect.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
builtin/checkout.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a6e30931b5..6b74e5fa4e 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -539,12 +539,11 @@ static int checkout_paths(const struct checkout_opts *opts,
* recognized by diff-index), we will always replace the name
* with the hex of the commit (whether it's in `...` form or
* not) for the run_add_interactive() machinery to work
- * properly. However, there is special logic for the HEAD case
- * so we mustn't replace that. Also, when we were given a
- * tree-object, new_branch_info->commit would be NULL, but we
- * do not have to do any replacement, either.
+ * properly. Also, when we were given a tree-object,
+ * new_branch_info->commit would be NULL, but we do not
+ * have to do any replacement.
*/
- if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
+ if (rev && new_branch_info->commit)
rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);
if (opts->checkout_index && opts->checkout_worktree)
--
2.43.0
^ permalink raw reply related
* [PATCH 0/1] config: add back code comment
From: Kristoffer Haugsbakk @ 2024-01-28 18:31 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, ps
This is a follow-up to the kh/maintenance-use-xdg-when-it-should
[series] which was merged in 12ee4ed506 (Merge branch
'kh/maintenance-use-xdg-when-it-sho.., 2024-01-26).
I dropped a code comment while iterating on a refactor. It still makes
as much sense in this context as before the refactor (it’s a _refactor_
in the sense of “don’t change code behavior”).
The code comment was moved to `config.c` in patch v1 3/4.[1] But review
feedback said that this comment didn’t fit in this new place and that we
shouldn’t `die()` in `git_global_config`. So in v2 3/4[2] I removed the
comment in `git_global_config`. But I forgot to put the comment back to
its original place, where it still makes as much sense as before my
series.
[Here] is the diff when I squash this patch into c15129b699 (config:
factor out global config file retrieval, 2024-01-18).
Sorry about the churn.
Cc: ps@pks.im
🔗 series: https://lore.kernel.org/git/cover.1697660181.git.code@khaugsbakk.name/
🔗 1: https://lore.kernel.org/git/147c767443c35b3b4a5516bf40557f41bb201078.1697660181.git.code@khaugsbakk.name/
🔗 2: https://lore.kernel.org/git/32e5ec7d866ff8fd26554b325812c6e19cb65126.1705267839.git.code@khaugsbakk.name/
† Here:
diff --git a/builtin/config.c b/builtin/config.c
index 6fff265581..b55bfae7d6 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -708,10 +708,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
}
if (use_global_config) {
- char *user_config, *xdg_config;
-
- git_global_config_paths(&user_config, &xdg_config);
- if (!user_config)
+ given_config_source.file = git_global_config();
+ if (!given_config_source.file)
/*
* It is unknown if HOME/.gitconfig exists, so
* we do not know if we should write to XDG
@@ -719,19 +717,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
* is set and points at a sane location.
*/
die(_("$HOME not set"));
-
given_config_source.scope = CONFIG_SCOPE_GLOBAL;
-
- if (access_or_warn(user_config, R_OK, 0) &&
- xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {
- given_config_source.file = xdg_config;
- free(user_config);
- } else {
- given_config_source.file = user_config;
- free(xdg_config);
- }
- }
- else if (use_system_config) {
+ } else if (use_system_config) {
given_config_source.file = git_system_config();
given_config_source.scope = CONFIG_SCOPE_SYSTEM;
} else if (use_local_config) {
diff --git a/config.c b/config.c
index ebc6a57e1c..3cfeb3d8bd 100644
--- a/config.c
+++ b/config.c
@@ -1987,6 +1987,26 @@ char *git_system_config(void)
return system_config;
}
+char *git_global_config(void)
+{
+ char *user_config, *xdg_config;
+
+ git_global_config_paths(&user_config, &xdg_config);
+ if (!user_config) {
+ free(xdg_config);
+ return NULL;
+ }
+
+ if (access_or_warn(user_config, R_OK, 0) && xdg_config &&
+ !access_or_warn(xdg_config, R_OK, 0)) {
+ free(user_config);
+ return xdg_config;
+ } else {
+ free(xdg_config);
+ return user_config;
+ }
+}
+
void git_global_config_paths(char **user_out, char **xdg_out)
{
char *user_config = xstrdup_or_null(getenv("GIT_CONFIG_GLOBAL"));
diff --git a/config.h b/config.h
index e5e523553c..5dba984f77 100644
--- a/config.h
+++ b/config.h
@@ -382,6 +382,7 @@ int config_error_nonbool(const char *);
#endif
char *git_system_config(void);
+char *git_global_config(void);
void git_global_config_paths(char **user, char **xdg);
int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
Kristoffer Haugsbakk (1):
config: add back code comment
builtin/config.c | 6 ++++++
1 file changed, 6 insertions(+)
--
2.43.0
^ permalink raw reply
* [PATCH 1/1] config: add back code comment
From: Kristoffer Haugsbakk @ 2024-01-28 18:31 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1706466321.git.code@khaugsbakk.name>
c15129b699 (config: factor out global config file retrieval, 2024-01-18)
was a refactor that moved some of the code in this function to
`config.c`. However, in the process I managed to drop this code comment
which explains `$HOME not set`.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
builtin/config.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/builtin/config.c b/builtin/config.c
index 08fe36d499..b55bfae7d6 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -710,6 +710,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)
if (use_global_config) {
given_config_source.file = git_global_config();
if (!given_config_source.file)
+ /*
+ * It is unknown if HOME/.gitconfig exists, so
+ * we do not know if we should write to XDG
+ * location; error out even if XDG_CONFIG_HOME
+ * is set and points at a sane location.
+ */
die(_("$HOME not set"));
given_config_source.scope = CONFIG_SCOPE_GLOBAL;
} else if (use_system_config) {
--
2.43.0
^ permalink raw reply related
* [PATCH 0/5] completion: remove hardcoded config variable names
From: Philippe Blain via GitGitGadget @ 2024-01-28 20:02 UTC (permalink / raw)
To: git; +Cc: Philippe Blain
This series removes hardcoded config variable names in the
__git_complete_config_variable_name function, partly by adding a new mode to
'git help'. It also adds completion for 'submodule.*' config variables,
which were previously missing.
I think it makes sense to do that in the same series since it's closely
related, and splitting it would result in textual conflicts between both
series if one does not build on top of the other, but I'm open to other
suggestions.
Thanks,
Philippe.
Philippe Blain (5):
completion: add space after config variable names also in Bash 3
completion: complete 'submodule.*' config variables
completion: add and use
__git_compute_first_level_config_vars_for_section
builtin/help: add --config-all-for-completion
completion: add an use
__git_compute_second_level_config_vars_for_section
builtin/help.c | 7 ++
contrib/completion/git-completion.bash | 90 +++++++++++++-------------
t/t9902-completion.sh | 21 ++++++
3 files changed, 74 insertions(+), 44 deletions(-)
base-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1660%2Fphil-blain%2Fcompletion-submodule-config-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1660/phil-blain/completion-submodule-config-v1
Pull-Request: https://github.com/git/git/pull/1660
--
gitgitgadget
^ permalink raw reply
* [PATCH 1/5] completion: add space after config variable names also in Bash 3
From: Philippe Blain via GitGitGadget @ 2024-01-28 20:02 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1660.git.git.1706472173.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
In be6444d1ca (completion: bash: add correct suffix in variables,
2021-08-16), __git_complete_config_variable_name was changed to use
"${sfx- }" instead of "$sfx" as the fourth argument of _gitcomp_nl and
_gitcomp_nl_append, such that this argument evaluates to a space if sfx
is unset. This was to ensure that e.g.
git config branch.autoSetupMe[TAB]
correctly completes to 'branch.autoSetupMerge ' with the trailing space.
This commits notes that the fix only works in Bash 4 because in Bash 3
the 'local sfx' construct at the beginning of
__git_complete_config_variable_name creates an empty string.
Make the fix also work for Bash 3 by using the "unset or null' parameter
expansion syntax ("${sfx:- }"), such that the parameter is also expanded
to a space if it is set but null, as is the behaviour of 'local sfx' in
Bash 3.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6662db221df..159a4fd8add 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2750,7 +2750,7 @@ __git_complete_config_variable_name ()
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
- __gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx- }"
+ __gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx:- }"
return
;;
guitool.*.*)
@@ -2784,7 +2784,7 @@ __git_complete_config_variable_name ()
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
__git_compute_all_commands
- __gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx- }"
+ __gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx:- }"
return
;;
remote.*.*)
@@ -2800,7 +2800,7 @@ __git_complete_config_variable_name ()
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
- __gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx- }"
+ __gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
return
;;
url.*.*)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/5] completion: complete 'submodule.*' config variables
From: Philippe Blain via GitGitGadget @ 2024-01-28 20:02 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1660.git.git.1706472173.gitgitgadget@gmail.com>
From: Philippe Blain <philippe.blain@canada.ca>
In the Bash completion script, function
__git_complete_config_variable_name completes config variables and has
special logic to deal with config variables involving user-defined
names, like branch.<name>.* and remote.<name>.*.
This special logic is missing for submodule-related config variables.
Add the appropriate branches to the case statement, making use of the
in-tree '.gitmodules' to list relevant submodules.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 159a4fd8add..8af9bc3f4e1 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2803,6 +2803,19 @@ __git_complete_config_variable_name ()
__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
return
;;
+ submodule.*.*)
+ local pfx="${cur_%.*}."
+ cur_="${cur_##*.}"
+ __gitcomp "url update branch fetchRecurseSubmodules ignore active" "$pfx" "$cur_" "$sfx"
+ return
+ ;;
+ submodule.*)
+ local pfx="${cur_%.*}."
+ cur_="${cur_#*.}"
+ __gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
+ __gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
+ return
+ ;;
url.*.*)
local pfx="${cur_%.*}."
cur_="${cur_##*.}"
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/5] completion: add and use __git_compute_first_level_config_vars_for_section
From: Philippe Blain via GitGitGadget @ 2024-01-28 20:02 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1660.git.git.1706472173.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
The function __git_complete_config_variable_name in the Bash completion
script hardcodes several config variable names. These variables are
those in config section where user-defined names can appear, such as
"branch.<name>". These sections are treated first by the case statement,
and the two last "catch all" cases are used for other sections, making
use of the __git_compute_config_vars and __git_compute_config_sections
function, which omit listing any variables containing wildcards or
placeholders. Having hardcoded config variables introduces the risk of
the completion code becoming out of sync with the actual config
variables accepted by Git.
To avoid these hardcoded config variables, introduce a new function,
__git_compute_first_level_config_vars_for_section, making use of the
existing __git_config_vars variable. This function takes as argument a
config section name and computes the matching "first level" config
variables for that section, i.e. those _not_ containing any placeholder,
like 'branch.autoSetupMerge, 'remote.pushDefault', etc. Use this
function and the variables it defines in the 'branch.*', 'remote.*' and
'submodule.*' switches of the case statement instead of hardcoding the
corresponding config variables. Note that we use indirect expansion
instead of associative arrays because those are not supported in Bash 3,
on which macOS is stuck for licensing reasons.
Add a test to make sure the new function works correctly by verfying it
lists all 'submodule' config variables. This has the downside that this
test must be updated when new 'submodule' configuration are added, but
this should be a small burden since it happens infrequently.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 24 +++++++++++++++++++++---
t/t9902-completion.sh | 11 +++++++++++
2 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8af9bc3f4e1..2934ceb7637 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2596,6 +2596,15 @@ __git_compute_config_vars ()
__git_config_vars="$(git help --config-for-completion)"
}
+__git_compute_first_level_config_vars_for_section ()
+{
+ section="$1"
+ __git_compute_config_vars
+ local this_section="__git_first_level_config_vars_for_section_${section}"
+ test -n "${!this_section}" ||
+ printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
+}
+
__git_config_sections=
__git_compute_config_sections ()
{
@@ -2749,8 +2758,11 @@ __git_complete_config_variable_name ()
branch.*)
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
+ local section="${pfx%.}"
__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
- __gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx:- }"
+ __git_compute_first_level_config_vars_for_section "${section}"
+ local this_section="__git_first_level_config_vars_for_section_${section}"
+ __gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
return
;;
guitool.*.*)
@@ -2799,8 +2811,11 @@ __git_complete_config_variable_name ()
remote.*)
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
+ local section="${pfx%.}"
__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
- __gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
+ __git_compute_first_level_config_vars_for_section "${section}"
+ local this_section="__git_first_level_config_vars_for_section_${section}"
+ __gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
return
;;
submodule.*.*)
@@ -2812,8 +2827,11 @@ __git_complete_config_variable_name ()
submodule.*)
local pfx="${cur_%.*}."
cur_="${cur_#*.}"
+ local section="${pfx%.}"
__gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
- __gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
+ __git_compute_first_level_config_vars_for_section "${section}"
+ local this_section="__git_first_level_config_vars_for_section_${section}"
+ __gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
return
;;
url.*.*)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 35eb534fdda..f28d8f531b7 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2583,6 +2583,17 @@ test_expect_success 'git config - variable name include' '
EOF
'
+test_expect_success 'git config - variable name - __git_compute_first_level_config_vars_for_section' '
+ test_completion "git config submodule." <<-\EOF
+ submodule.active Z
+ submodule.alternateErrorStrategy Z
+ submodule.alternateLocation Z
+ submodule.fetchJobs Z
+ submodule.propagateBranches Z
+ submodule.recurse Z
+ EOF
+'
+
test_expect_success 'git config - value' '
test_completion "git config color.pager " <<-\EOF
false Z
--
gitgitgadget
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox