* Re: git grep --no-index fails when given an absolute path
From: Kristoffer Haugsbakk @ 2023-10-26 12:28 UTC (permalink / raw)
To: triallax; +Cc: Git
In-Reply-To: <Nhfa-Qv--F-9@tutanota.com>
Hi
On Thu, Oct 26, 2023, at 12:46, triallax@tutanota.com wrote:
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
> - Run `git grep --no-index -r -- -- /usr/share/fish/completions/`
See this thread: https://lore.kernel.org/git/CAKFQ_Q_P4HvCMHsg4=6ycb8r44qprhRCGSmLQf7B3_-zy28_oQ@mail.gmail.com/
> What did you expect to happen? (Expected behavior)
>
> The command executes the search successfully.
>
> What happened instead? (Actual behavior)
>
> This happens:
>
> triallax@satoru ~> git grep --no-index -r -- --
> /usr/share/fish/completions/
> BUG: environment.c:213: git environment hasn't been setup
> fish: Job 1, 'git grep --no-index -r -- -- /u…' terminated by signal
> SIGABRT (Abort)
>
> What's different between what you expected and what actually happened?
>
> One of them is an error, and the other is a successful execution.
>
> Anything else you want to add:
>
> Interestingly enough, this seems to be happening only with absolute paths from
> my testing, and it happens even when the directory that's passed is a Git repo.
What is the working directory that you are in? I’m guessing somewhere in
your home directory?
I’ll try to reproduce your command later with the proposed fix.
Kristoffer
^ permalink raw reply
* Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option
From: Junio C Hamano @ 2023-10-26 12:37 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git
In-Reply-To: <ZTi4Zd1by53q5gtM@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> + if (revs->do_not_die_on_missing_objects)
>> + oidset_init(&revs->missing_objects, 0);
>> +
>
> While we're initializing the new oidset, we never clear it. We should
> probably call `oidset_clear()` in `release_revisions()`. And if we
> unconditionally initialized the oidset here then we can also call
> `oiadset_clear()` unconditionally there, which should be preferable
> given that `oidset_init()` does not allocate memory when no initial size
> was given.
Yup, I used the conditional one to match the above, but initializing
unused oidset is cheap and frees us from having to worry about
mistakes. I like your idea much better.
>> +
>> + /* Missing objects to be tracked without failing traversal. */
>> + struct oidset missing_objects;
>
> As far as I can see we only use this set to track missing commits, but
> none of the other objects. The name thus feels a bit misleading to me,
> as a reader might rightfully assume that it contains _all_ missing
> objects after the revwalk. Should we rename it to `missing_commits` to
> clarify?
Again, very good suggestion.
Thanks.
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Junio C Hamano @ 2023-10-26 12:41 UTC (permalink / raw)
To: Jeff King
Cc: Uwe Kleine-König, Michael Strawbridge, Luben Tuikov, git,
entwicklung
In-Reply-To: <20231025072104.GA2145145@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Note that the bug will only trigger if Email::Valid is installed.
I recall we chased a different bug that depends on the use/non-use
of this package a few years ago. Is the difference significant
enough that we may want to install on one but not in another CI
environment, like we have a separate CI jobs with exotic settings, I
wonder.
^ permalink raw reply
* Re: [PATCH v2] send-email: move validation code below process_address_list
From: Junio C Hamano @ 2023-10-26 12:44 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <ddd4bfdd-ed14-44f4-89d3-192332bbc1c4@amd.com>
Michael Strawbridge <michael.strawbridge@amd.com> writes:
> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
> From: Michael Strawbridge <michael.strawbridge@amd.com>
> Date: Wed, 11 Oct 2023 16:13:13 -0400
> Subject: [PATCH v2] send-email: move validation code below
> process_address_list
Why do these in-body headers to lie about the author date?
By the way, the in-body header does seem to support the header line
folding (see the "subject" one here).
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Michael Strawbridge @ 2023-10-26 13:07 UTC (permalink / raw)
To: Junio C Hamano, Jeff King
Cc: Uwe Kleine-König, Luben Tuikov, git, entwicklung
In-Reply-To: <xmqqsf5xr1xk.fsf@gitster.g>
On 10/26/23 08:41, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> Note that the bug will only trigger if Email::Valid is installed.
>
> I recall we chased a different bug that depends on the use/non-use
> of this package a few years ago. Is the difference significant
> enough that we may want to install on one but not in another CI
> environment, like we have a separate CI jobs with exotic settings, I
> wonder.
That would make sense to me. We have had 3 regressions threads
recently for git send email where Email::Valid was important.
- [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas - (this thread)
- [REGRESSION] uninitialized value $address in git send-email - https://public-inbox.org/git/20230918212004.GC2163162@coredump.intra.peff.net/T/#m9e0211a8ad387adbbadf31dcfcd7982d4046633d
- Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address" - https://public-inbox.org/git/68d7e5c3-6b4a-4d0d-9885-f3d4e2199f26@amd.com/T/#m1411c155e11ad9c5d913d22d1d11180ed56eabc7
^ permalink raw reply
* Re: [PATCH v2] send-email: move validation code below process_address_list
From: Michael Strawbridge @ 2023-10-26 13:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <xmqqlebpr1se.fsf@gitster.g>
On 10/26/23 08:44, Junio C Hamano wrote:
> Michael Strawbridge <michael.strawbridge@amd.com> writes:
>
>> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
>> From: Michael Strawbridge <michael.strawbridge@amd.com>
>> Date: Wed, 11 Oct 2023 16:13:13 -0400
>> Subject: [PATCH v2] send-email: move validation code below
>> process_address_list
>
> Why do these in-body headers to lie about the author date?
Sorry. They weren't meant to. I have been amending my local commit
rather than making new ones for every version. It seems that when
I do that, the author date stays at the first time the commit was
created. I wasn't aware of that unintended side effect.
>
> By the way, the in-body header does seem to support the header line
> folding (see the "subject" one here).
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Ruslan Yakauleu @ 2023-10-26 13:40 UTC (permalink / raw)
To: git
In-Reply-To: <c37ba153-7239-49ff-b40f-370bc695986e@gmail.com>
> Squash and rebase are functionally identical in this case.
Sorry, but for me `git merge --squash` doesn't work.
Currently, I have a global option --no-ff for master
$ git config branch.master.mergeoptions --no-ff
In this way `git merge --squash` crashes with message
> fatal: options '--squash' and '--no-ff.' cannot be used together
In other way it writes something like
> Fast-forward
> Squash commit -- not updating HEAD
and... not updates parent branch.
For example, GitHub propose for PR's:
- Create a merge commit - the same as `git merge --no-ff`
- Rebase and merge - the same as `git rebase ...; git merge --ff-only`
- Squash AND commit - like two different operations. So after squash we
still have to merge our commit properly into parent branch.
The new option just dynamically selects between --ff and --no-ff for
`git merge`. Nothing else.
--
Ruslan
^ permalink raw reply
* Re: [PATCH v5 00/14] Introduce new `git replay` command
From: Johannes Schindelin @ 2023-10-26 13:44 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Dragan Simic, Linus Arver
In-Reply-To: <20231010123847.2777056-1-christian.couder@gmail.com>
Hi Christian,
On Tue, 10 Oct 2023, Christian Couder wrote:
> 6: ec51351889 ! 6: 37d545d5d6 replay: don't simplify history
> @@ Metadata
> Author: Elijah Newren <newren@gmail.com>
>
> ## Commit message ##
> - replay: don't simplify history
> + replay: change rev walking options
>
> Let's set the rev walking options we need after calling
> - setup_revisions() instead of before. This makes it clearer which options
> - we need.
> + setup_revisions() instead of before. This enforces options we always
> + want.
> +
> + We want the command to work from older commits to newer ones by default,
> + but we are Ok with letting users reverse that, using --reverse, if that's
> + what they really want.
>
> Also we don't want history simplification, as we want to deal with all
> the commits in the affected range.
>
> + Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix
> }
>
> + /* requirements/overrides for revs */
> -+ revs.reverse = 1;
> ++ revs.reverse = !revs.reverse;
> + revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> + revs.topo_order = 1;
> + revs.simplify_history = 0;
This still overrides a couple of command-line options, _silently_. I would
prefer those three assignments to be moved just before the
`setup_revisions()` call.
Letting users override these settings may not make much sense, but it
makes even less sense to pretend to let them override the settings and
then just ignore them without warning. (See also
https://en.wikipedia.org/wiki/Principle_of_least_astonishment.)
Moving these three assignments before the `setup_revisions()` call would
neatly remedy that.
> 7: cd4ed07d2d = 7: 2943f08926 replay: add an important FIXME comment about gpg signing
> 8: e45a55917c = 8: f81962ba41 replay: remove progress and info output
> 9: 0587a76cbb = 9: 236747497e replay: remove HEAD related sanity check
> 10: d10368e87a = 10: 3374d5be23 replay: make it a minimal server side command
> 11: 4e09572c43 ! 11: 197d076a93 replay: use standard revision ranges
> @@ Commit message
> way as many other Git commands. This makes its interface more
> standard and more flexible.
>
> + This also enables many revision related options accepted and
> + eaten by setup_revisions(). If the replay command was a high level
> + one or had a high level mode, it would make sense to restrict some
> + of the possible options, like those generating non-contiguous
> + history, as they could be confusing for most users.
> +
> Also as the interface of the command is now mostly finalized,
> we can add some documentation as well as testcases to make sure
> the command will continue to work as designed in the future.
>
> + We only document the rev-list related options among all the
> + revision related options that are now accepted, as the rev-list
> + related ones are probably the most useful for now.
> +
> + Helped-by: Dragan Simic <dsimic@manjaro.org>
> + Helped-by: Linus Arver <linusa@google.com>
> Co-authored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> @@ Documentation/git-replay.txt (new)
> +
> +NAME
> +----
> -+git-replay - Replay commits on a different base, without touching working tree
> ++git-replay - Replay commits on a new base, works on bare repos too
> +
> +
> +SYNOPSIS
As mentioned in
https://lore.kernel.org/git/03460733-0219-c648-5757-db1958f8042e@gmx.de/,
I would like the `EXPERIMENTAL` label to be shown prominently here.
Probably not only the `SYNOPSIS` as I had originally suggested but also in
the `NAME`.
Otherwise we may end up with the same situation as with the (from my
perspective, failed) `git switch`/`git restore` experiment, where we
wanted to explore a better user experience than the overloaded `git
checkout` command, only to now be stuck with having to maintain
backward-compatibility for `git switch`/`git restore` command-line options
that were not meant to be set in stone but to be iterated on, instead. A
real-life demonstration of [Hyrum's Law](hyrumslaw.com/), if you like. Or,
from a different angle, we re-enacted https://xkcd.com/927/ in a way.
I'd like to suggest to learn from history and avoid this by tacking on a
warning label right at the top of the documentation. We may eventually
manage to iterate `git replay` to a point where it is totally capable to
supersede `git rebase`, by doing everything the latter does, except
better, who knows? But we _do_ need the liberty to make sweeping changes
to this new builtin if we want to have a prayer of doing that. And I fear
that not even mentioning the EXPERIMENTAL nature right at the top of the
manual page would just render us into that undesirable corner.
In addition, I am still a bit uneasy with introducing both the manual page
and the test script in this commit (see my comments in
https://lore.kernel.org/git/03460733-0219-c648-5757-db1958f8042e@gmx.de/).
It would be better to uphold our high standard and introduce scaffolds for
both files in the first commit, then populate the file contents
incrementally in the same the patches that introduce the corresponding
options/features/changes.
The rest of the interdiff consists mostly of context changes intermixed
with a couple of changes that I like.
Ciao,
Johannes
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Todd Zullinger @ 2023-10-26 14:46 UTC (permalink / raw)
To: Michael Strawbridge
Cc: Junio C Hamano, Jeff King, Uwe Kleine-König, Luben Tuikov,
git, entwicklung
In-Reply-To: <a71f2f1f-b5f0-4628-a4f3-6fd1319062a3@amd.com>
Michael Strawbridge wrote:
> On 10/26/23 08:41, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>> Note that the bug will only trigger if Email::Valid is installed.
>>
>> I recall we chased a different bug that depends on the use/non-use
>> of this package a few years ago. Is the difference significant
>> enough that we may want to install on one but not in another CI
>> environment, like we have a separate CI jobs with exotic settings, I
>> wonder.
>
> That would make sense to me. We have had 3 regressions threads
> recently for git send email where Email::Valid was important.
>
> - [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas - (this thread)
> - [REGRESSION] uninitialized value $address in git send-email - https://public-inbox.org/git/20230918212004.GC2163162@coredump.intra.peff.net/T/#m9e0211a8ad387adbbadf31dcfcd7982d4046633d
> - Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address" - https://public-inbox.org/git/68d7e5c3-6b4a-4d0d-9885-f3d4e2199f26@amd.com/T/#m1411c155e11ad9c5d913d22d1d11180ed56eabc7
Alternately, perhaps having Email::Valid as an optional
dependency is worth reconsidering. If it's truly important
to validation, make it a requirement. If it's not, then
drop it to simplify the code and avoid these sort of issues.
As a (former) distribution packager, having these optional
dependencies which change the behavior is always a tough
position to be in.
If I make the git package require it to ensure consistent
behavior then some folks will -quite rightly- complain that
it should not be a requirement. If I keep it an optional
dependency, then debugging becomes more difficult for the
reasons we've seen in these recent (and not-so-recent)
threads.
I'd lean toward dropping the dependency entirely and leave
the more basic validation of git-send-email in place. That
may not catch every type of address error, but I would argue
that what we do without Email::Valid is perfectly reasonable
for checking basic email address syntax sanity.
Further validation will happen along the path of mail
transfer agents and failures should be reported to the
sender in the same way as any other invalid email address.
On a related note, one issue¹ we had reported in Fedora
after making Email::Valid a requirement was that it rejected
messages where the local part was too long, per the relevant
RFC's. But these were generated addresses from GitLab. The
addresses worked in practice. While Email::Valid was
technically correct in rejecting such addresses, it didn't
improve the experience of git send-email users.
¹ https://bugzilla.redhat.com/2046203
--
Todd
^ permalink raw reply
* [PATCH v3] bugreport: reject positional arguments
From: emilyshaffer @ 2023-10-26 15:54 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer, Eric Sunshine, Sheik, Dragan Simic
In-Reply-To: <20231026005542.872301-1-nasamuffin@google.com>
From: Emily Shaffer <nasamuffin@google.com>
git-bugreport already rejected unrecognized flag arguments, like
`--diaggnose`, but this doesn't help if the user's mistake was to forget
the `--` in front of the argument. This can result in a user's intended
argument not being parsed with no indication to the user that something
went wrong. Since git-bugreport presently doesn't take any positionals
at all, let's reject all positionals and give the user a usage hint.
Signed-off-by: Emily Shaffer <nasamuffin@google.com>
---
Per Eric's and Dragan's comments, dropped the null checking for argv[0].
No point in being too paranoid, I suppose :)
Note that after this morning it's not likely that I'll be able to find
time to update this again so quickly, so if there are other nits,
reviewers can feel free to send their own rerolls rather than waiting
for me to see it and turn the patch around.
- Emily
builtin/bugreport.c | 5 +++++
t/t0091-bugreport.sh | 7 +++++++
2 files changed, 12 insertions(+)
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index d2ae5c305d..3106e56a13 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -126,6 +126,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, bugreport_options,
bugreport_usage, 0);
+ if (argc) {
+ error(_("unknown argument `%s'"), argv[0]);
+ usage(bugreport_usage[0]);
+ }
+
/* Prepare the path to put the result */
prefixed_filename = prefix_filename(prefix,
option_output ? option_output : "");
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index f6998269be..5b1b3e8d07 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -69,6 +69,13 @@ test_expect_success 'incorrect arguments abort with usage' '
test_path_is_missing git-bugreport-*
'
+test_expect_success 'incorrect positional arguments abort with usage and hint' '
+ test_must_fail git bugreport false 2>output &&
+ test_i18ngrep usage output &&
+ test_i18ngrep false output &&
+ test_path_is_missing git-bugreport-*
+'
+
test_expect_success 'runs outside of a git dir' '
test_when_finished rm non-repo/git-bugreport-* &&
nongit git bugreport
--
2.42.0.758.gaed0368e0e-goog
^ permalink raw reply related
* Re: [PATCH v3] bugreport: reject positional arguments
From: Eric Sunshine @ 2023-10-26 17:18 UTC (permalink / raw)
To: emilyshaffer; +Cc: git, Emily Shaffer, Sheik, Dragan Simic
In-Reply-To: <20231026155459.2234929-1-nasamuffin@google.com>
On Thu, Oct 26, 2023 at 11:55 AM <emilyshaffer@google.com> wrote:
> git-bugreport already rejected unrecognized flag arguments, like
> `--diaggnose`, but this doesn't help if the user's mistake was to forget
> the `--` in front of the argument. This can result in a user's intended
> argument not being parsed with no indication to the user that something
> went wrong. Since git-bugreport presently doesn't take any positionals
> at all, let's reject all positionals and give the user a usage hint.
>
> Signed-off-by: Emily Shaffer <nasamuffin@google.com>
> ---
> Per Eric's and Dragan's comments, dropped the null checking for argv[0].
> No point in being too paranoid, I suppose :)
>
> Note that after this morning it's not likely that I'll be able to find
> time to update this again so quickly, so if there are other nits,
> reviewers can feel free to send their own rerolls rather than waiting
> for me to see it and turn the patch around.
Thanks. This version looks good enough to me. Just one minor comment below...
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> @@ -69,6 +69,13 @@ test_expect_success 'incorrect arguments abort with usage' '
> +test_expect_success 'incorrect positional arguments abort with usage and hint' '
> + test_must_fail git bugreport false 2>output &&
> + test_i18ngrep usage output &&
> + test_i18ngrep false output &&
> + test_path_is_missing git-bugreport-*
> +'
I didn't really pay attention to the test in earlier rounds so didn't
notice this, but these days we just use 'grep' rather than
'test_i18ngrep'. (Indeed, the existing tests in this script use
'grep'.)
^ permalink raw reply
* [PATCH v4 0/2] bugreport: reject positional arguments
From: emilyshaffer @ 2023-10-26 18:22 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer, Eric Sunshine, Sheik, Dragan Simic
In-Reply-To: <20231026155459.2234929-1-nasamuffin@google.com>
From: Emily Shaffer <nasamuffin@google.com>
The test I cribbed from for the newly added one in patch 2 was still
using test_i18ngrep, and Eric mentioned us not wanting i18ngrep at all
anymore, so I went ahead and cleaned that up as well.
- Emily
Emily Shaffer (2):
t0091-bugreport: stop using i18ngrep
bugreport: reject positional arguments
builtin/bugreport.c | 5 +++++
t/t0091-bugreport.sh | 9 ++++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply
* [PATCH v4 1/2] t0091-bugreport: stop using i18ngrep
From: emilyshaffer @ 2023-10-26 18:22 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
In-Reply-To: <20231026155459.2234929-1-nasamuffin@google.com>
From: Emily Shaffer <nasamuffin@google.com>
Since e6545201ad (Merge branch 'ab/detox-config-gettext', 2021-04-13),
test_i18ngrep is no longer required. Quit using it in the bugreport
tests, since it's setting a bad example for tests added later.
Signed-off-by: Emily Shaffer <nasamuffin@google.com>
---
t/t0091-bugreport.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index f6998269be..e1588f71b7 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -65,7 +65,7 @@ test_expect_success '--output-directory puts the report in the provided dir' '
test_expect_success 'incorrect arguments abort with usage' '
test_must_fail git bugreport --false 2>output &&
- test_i18ngrep usage output &&
+ grep usage output &&
test_path_is_missing git-bugreport-*
'
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply related
* [PATCH v4 2/2] bugreport: reject positional arguments
From: emilyshaffer @ 2023-10-26 18:22 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer
In-Reply-To: <20231026155459.2234929-1-nasamuffin@google.com>
From: Emily Shaffer <nasamuffin@google.com>
git-bugreport already rejected unrecognized flag arguments, like
`--diaggnose`, but this doesn't help if the user's mistake was to forget
the `--` in front of the argument. This can result in a user's intended
argument not being parsed with no indication to the user that something
went wrong. Since git-bugreport presently doesn't take any positionals
at all, let's reject all positionals and give the user a usage hint.
Signed-off-by: Emily Shaffer <nasamuffin@google.com>
---
builtin/bugreport.c | 5 +++++
t/t0091-bugreport.sh | 7 +++++++
2 files changed, 12 insertions(+)
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index d2ae5c305d..3106e56a13 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -126,6 +126,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, bugreport_options,
bugreport_usage, 0);
+ if (argc) {
+ error(_("unknown argument `%s'"), argv[0]);
+ usage(bugreport_usage[0]);
+ }
+
/* Prepare the path to put the result */
prefixed_filename = prefix_filename(prefix,
option_output ? option_output : "");
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index e1588f71b7..ae5b7dc31f 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -69,6 +69,13 @@ test_expect_success 'incorrect arguments abort with usage' '
test_path_is_missing git-bugreport-*
'
+test_expect_success 'incorrect positional arguments abort with usage and hint' '
+ test_must_fail git bugreport false 2>output &&
+ grep usage output &&
+ grep false output &&
+ test_path_is_missing git-bugreport-*
+'
+
test_expect_success 'runs outside of a git dir' '
test_when_finished rm non-repo/git-bugreport-* &&
nongit git bugreport
--
2.42.0.820.g83a721a137-goog
^ permalink raw reply related
* [PATCH v4] subtree: fix split processing with multiple subtrees present
From: Zach FettersMoore via GitGitGadget @ 2023-10-26 19:17 UTC (permalink / raw)
To: git; +Cc: Zach FettersMoore, Zach FettersMoore
In-Reply-To: <pull.1587.v3.git.1696019580.gitgitgadget@gmail.com>
From: Zach FettersMoore <zach.fetters@apollographql.com>
When there are multiple subtrees present in a repository and they are
all using 'git subtree split', the 'split' command can take a
significant (and constantly growing) amount of time to run even when
using the '--rejoin' flag. This is due to the fact that when processing
commits to determine the last known split to start from when looking
for changes, if there has been a split/merge done from another subtree
there will be 2 split commits, one mainline and one subtree, for the
second subtree that are part of the processing. The non-mainline
subtree split commit will cause the processing to always need to search
the entire history of the given subtree as part of its processing even
though those commits are totally irrelevant to the current subtree
split being run.
In the diagram below, 'M' represents the mainline repo branch, 'A'
represents one subtree, and 'B' represents another. M3 and B1 represent
a split commit for subtree B that was created from commit M4. M2 and A1
represent a split commit made from subtree A that was also created
based on changes back to and including M4. M1 represents new changes to
the repo, in this scenario if you try to run a 'git subtree split
--rejoin' for subtree B, commits M1, M2, and A1, will be included in
the processing of changes for the new split commit since the last
split/rejoin for subtree B was at M3. The issue is that by having A1
included in this processing the command ends up needing to processing
every commit down tree A even though none of that is needed or relevant
to the current command and result.
M1
| \ \
M2 | |
| A1 |
M3 | |
| | B1
M4 | |
So this commit makes a change to the processing of commits for the split
command in order to ignore non-mainline commits from other subtrees such
as A1 in the diagram by adding a new function
'should_ignore_subtree_commit' which is called during
'process_split_commit'. This allows the split/rejoin processing to still
function as expected but removes all of the unnecessary processing that
takes place currently which greatly inflates the processing time.
Added a test to validate that the proposed fix
solves the issue.
The test accomplishes this by checking the output
of the split command to ensure the output from
the progress of 'process_split_commit' function
that represents the 'extracount' of commits
processed does not increment.
This was tested against the original functionality
to show the test failed, and then with this fix
to show the test passes.
This illustrated that when using multiple subtrees,
A and B, when doing a split on subtree B, the
processing does not traverse the entire history
of subtree A which is unnecessary and would cause
the 'extracount' of processed commits to climb
based on the number of commits in the history of
subtree A.
Signed-off-by: Zach FettersMoore <zach.fetters@apollographql.com>
---
subtree: fix split processing with multiple subtrees present
When there are multiple subtrees in a repo and git subtree split
--rejoin is being used for the subtrees, the processing of commits for a
new split can take a significant (and constantly growing) amount of time
because the split commits from other subtrees cause the processing to
have to scan the entire history of the other subtree(s). This patch
filters out the other subtree split commits that are unnecessary for the
split commit processing.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1587%2FBobaFetters%2Fzf%2Fmulti-subtree-processing-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1587/BobaFetters/zf/multi-subtree-processing-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1587
Range-diff vs v3:
1: 43175154a82 < -: ----------- subtree: fix split processing with multiple subtrees present
2: d6811daf7cf < -: ----------- subtree: changing location of commit ignore processing
3: eff8bfcc042 ! 1: 353152910eb subtree: adding test to validate fix
@@ Metadata
Author: Zach FettersMoore <zach.fetters@apollographql.com>
## Commit message ##
- subtree: adding test to validate fix
+ subtree: fix split processing with multiple subtrees present
- Adding a test to validate that the proposed fix
+ When there are multiple subtrees present in a repository and they are
+ all using 'git subtree split', the 'split' command can take a
+ significant (and constantly growing) amount of time to run even when
+ using the '--rejoin' flag. This is due to the fact that when processing
+ commits to determine the last known split to start from when looking
+ for changes, if there has been a split/merge done from another subtree
+ there will be 2 split commits, one mainline and one subtree, for the
+ second subtree that are part of the processing. The non-mainline
+ subtree split commit will cause the processing to always need to search
+ the entire history of the given subtree as part of its processing even
+ though those commits are totally irrelevant to the current subtree
+ split being run.
+
+ In the diagram below, 'M' represents the mainline repo branch, 'A'
+ represents one subtree, and 'B' represents another. M3 and B1 represent
+ a split commit for subtree B that was created from commit M4. M2 and A1
+ represent a split commit made from subtree A that was also created
+ based on changes back to and including M4. M1 represents new changes to
+ the repo, in this scenario if you try to run a 'git subtree split
+ --rejoin' for subtree B, commits M1, M2, and A1, will be included in
+ the processing of changes for the new split commit since the last
+ split/rejoin for subtree B was at M3. The issue is that by having A1
+ included in this processing the command ends up needing to processing
+ every commit down tree A even though none of that is needed or relevant
+ to the current command and result.
+
+ M1
+ | \ \
+ M2 | |
+ | A1 |
+ M3 | |
+ | | B1
+ M4 | |
+
+ So this commit makes a change to the processing of commits for the split
+ command in order to ignore non-mainline commits from other subtrees such
+ as A1 in the diagram by adding a new function
+ 'should_ignore_subtree_commit' which is called during
+ 'process_split_commit'. This allows the split/rejoin processing to still
+ function as expected but removes all of the unnecessary processing that
+ takes place currently which greatly inflates the processing time.
+
+ Added a test to validate that the proposed fix
solves the issue.
The test accomplishes this by checking the output
@@ Commit message
Signed-off-by: Zach FettersMoore <zach.fetters@apollographql.com>
+ ## contrib/subtree/git-subtree.sh ##
+@@ contrib/subtree/git-subtree.sh: ensure_valid_ref_format () {
+ die "fatal: '$1' does not look like a ref"
+ }
+
++# Usage: check if a commit from another subtree should be
++# ignored from processing for splits
++should_ignore_subtree_split_commit () {
++ if test -n "$(git log -1 --grep="git-subtree-dir:" $1)"
++ then
++ if test -z "$(git log -1 --grep="git-subtree-mainline:" $1)" &&
++ test -z "$(git log -1 --grep="git-subtree-dir: $arg_prefix$" $1)"
++ then
++ return 0
++ fi
++ fi
++ return 1
++}
++
+ # Usage: process_split_commit REV PARENTS
+ process_split_commit () {
+ assert test $# = 2
+@@ contrib/subtree/git-subtree.sh: cmd_split () {
+ eval "$grl" |
+ while read rev parents
+ do
+- process_split_commit "$rev" "$parents"
++ if should_ignore_subtree_split_commit "$rev"
++ then
++ continue
++ fi
++ parsedParents=''
++ for parent in $parents
++ do
++ should_ignore_subtree_split_commit "$parent"
++ if test $? -eq 1
++ then
++ parsedParents+="$parent "
++ fi
++ done
++ process_split_commit "$rev" "$parsedParents"
+ done || exit $?
+
+ latest_new=$(cache_get latest_new) || exit $?
+
## contrib/subtree/t/t7900-subtree.sh ##
@@ contrib/subtree/t/t7900-subtree.sh: test_expect_success 'split sub dir/ with --rejoin' '
)
@@ contrib/subtree/t/t7900-subtree.sh: test_expect_success 'split sub dir/ with --r
+ ) &&
+ (
+ cd "$test_count" &&
-+ test "$(git subtree split --prefix=subBDir --squash --rejoin -d -m "Sub B Split 1" 2>&1 | grep -w "\[1\]")" = ""
++ test "$(git subtree split --prefix=subBDir --squash --rejoin \
++ -d -m "Sub B Split 1" 2>&1 | grep -w "\[1\]")" = ""
+ )
+'
+
contrib/subtree/git-subtree.sh | 29 ++++++++++++++++++++-
contrib/subtree/t/t7900-subtree.sh | 42 ++++++++++++++++++++++++++++++
2 files changed, 70 insertions(+), 1 deletion(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index e0c5d3b0de6..e69991a9d80 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -778,6 +778,20 @@ ensure_valid_ref_format () {
die "fatal: '$1' does not look like a ref"
}
+# Usage: check if a commit from another subtree should be
+# ignored from processing for splits
+should_ignore_subtree_split_commit () {
+ if test -n "$(git log -1 --grep="git-subtree-dir:" $1)"
+ then
+ if test -z "$(git log -1 --grep="git-subtree-mainline:" $1)" &&
+ test -z "$(git log -1 --grep="git-subtree-dir: $arg_prefix$" $1)"
+ then
+ return 0
+ fi
+ fi
+ return 1
+}
+
# Usage: process_split_commit REV PARENTS
process_split_commit () {
assert test $# = 2
@@ -963,7 +977,20 @@ cmd_split () {
eval "$grl" |
while read rev parents
do
- process_split_commit "$rev" "$parents"
+ if should_ignore_subtree_split_commit "$rev"
+ then
+ continue
+ fi
+ parsedParents=''
+ for parent in $parents
+ do
+ should_ignore_subtree_split_commit "$parent"
+ if test $? -eq 1
+ then
+ parsedParents+="$parent "
+ fi
+ done
+ process_split_commit "$rev" "$parsedParents"
done || exit $?
latest_new=$(cache_get latest_new) || exit $?
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 49a21dd7c9c..87d59afd761 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -385,6 +385,48 @@ test_expect_success 'split sub dir/ with --rejoin' '
)
'
+test_expect_success 'split with multiple subtrees' '
+ subtree_test_create_repo "$test_count" &&
+ subtree_test_create_repo "$test_count/subA" &&
+ subtree_test_create_repo "$test_count/subB" &&
+ test_create_commit "$test_count" main1 &&
+ test_create_commit "$test_count/subA" subA1 &&
+ test_create_commit "$test_count/subA" subA2 &&
+ test_create_commit "$test_count/subA" subA3 &&
+ test_create_commit "$test_count/subB" subB1 &&
+ (
+ cd "$test_count" &&
+ git fetch ./subA HEAD &&
+ git subtree add --prefix=subADir FETCH_HEAD
+ ) &&
+ (
+ cd "$test_count" &&
+ git fetch ./subB HEAD &&
+ git subtree add --prefix=subBDir FETCH_HEAD
+ ) &&
+ test_create_commit "$test_count" subADir/main-subA1 &&
+ test_create_commit "$test_count" subBDir/main-subB1 &&
+ (
+ cd "$test_count" &&
+ git subtree split --prefix=subADir --squash --rejoin -m "Sub A Split 1"
+ ) &&
+ (
+ cd "$test_count" &&
+ git subtree split --prefix=subBDir --squash --rejoin -m "Sub B Split 1"
+ ) &&
+ test_create_commit "$test_count" subADir/main-subA2 &&
+ test_create_commit "$test_count" subBDir/main-subB2 &&
+ (
+ cd "$test_count" &&
+ git subtree split --prefix=subADir --squash --rejoin -m "Sub A Split 2"
+ ) &&
+ (
+ cd "$test_count" &&
+ test "$(git subtree split --prefix=subBDir --squash --rejoin \
+ -d -m "Sub B Split 1" 2>&1 | grep -w "\[1\]")" = ""
+ )
+'
+
test_expect_success 'split sub dir/ with --rejoin from scratch' '
subtree_test_create_repo "$test_count" &&
test_create_commit "$test_count" main1 &&
base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] subtree: fix split processing with multiple subtrees present
From: Zach FettersMoore @ 2023-10-26 19:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Zach FettersMoore via GitGitGadget, git
In-Reply-To: <xmqqpm2fht2x.fsf@gitster.g>
> Please do not violate Documentation/CodingGuidelines for our shell
> scripted Porcelain, even if it is a script in contrib/ and also
>please avoid bash-isms.
I believe I have resolved the CodingGuidelines issues.
> Also doesn't "subtree" have its own test? If this change is a fix
> for some problem(s), can we have a test or two that demonstrate how
> the current code without the patch is broken?
I was able to add a test that validates against some of the metrics that
are tracked when running a split for processing commits. Validated that
before my fix the test fails, and after my fix the test passes.
>> In the diagram below, 'M' represents the mainline repo branch, 'A'
>> represents one subtree, and 'B' represents another. M3 and B1 represent
>> a split commit for subtree B that was created from commit M4. M2 and A1
>> represent a split commit made from subtree A that was also created
>> based on changes back to and including M4. M1 represents new changes to
>> the repo, in this scenario if you try to run a 'git subtree split
>> --rejoin' for subtree B, commits M1, M2, and A1, will be included in
>> the processing of changes for the new split commit since the last
>> split/rejoin for subtree B was at M3. The issue is that by having A1
>> included in this processing the command ends up needing to processing
>> every commit down tree A even though none of that is needed or relevant
>> to the current command and result.
>>
>> M1
>> | \ \
>> M2 | |
>> | A1 |
>> M3 | |
>> | | B1
>> M4 | |
> The above paragraph explains which different things you drew in the
> diagram are representing, but it is not clear how they relate to
> each other. Do they for example depict parent-child commit
> relationship? What are the wide gaps between these three tracks and
> what are the short angled lines leaning to the left near the tip?
> Is the time/topology flowing from bottom to top?
I am realizing I made a few mistakes with trying to illustrate the diagram
which I will attempt to make more clear below. As for the 3 columns in the
diagram, 'M' represents the mainline branch of the repo being developed in,
while column 'A' represents the history of a subtree 'A' included in the
repo, and column 'B' also represents the history of a subtree 'B' in the
repo. The diagram attempts to illustrate when a 'git subtree split --rejoin'
is used, that there is a commit made in the subtrees history, and that is
then merged into the mainline repo branch.
M1
|
|
M2 --- |
| A1
| |
M3 ---------- |
| | B1
M4 | |
Hopefully that helps better illustrate the state of the repo before the new
'git subtree split --rejoin' attempt and why it results in the described issue.
>> +should_ignore_subtree_commit () {
>> + if [ "$(git log -1 --grep="git-subtree-dir:" $1)" ]
>> + then
>> + if [[ -z "$(git log -1 --grep="git-subtree-mainline:" $1)" && -z "$(git log -1 --grep="git-subtree-dir: $dir$" $1)" ]]
>
> Here $dir is a free variable that comes from outside. The caller
> does not supply it as a parameter to this function (and the caller
> does not receive it as its parameter from its caller). Yet the file
> as a whole seems to liberally make assignments to it ("git grep dir="
> on the file counts 7 assignments). Are we sure we are looking for
> the right $dir in this particular grep?
>
> Side note: I am not familiar with this part of the code at
> all, so do not take it as "here is a bug", but more as "this
> smells error prone."
From my testing and what I see for '$dir' usage in the 'cmd_split'
function which leads to this code it is the correct '$dir', although
I see your point about it being reassigned in different places which
makes it error prone. I switched this to use the command
line argument '$arg_prefix' since the subtree prefix passed into
the command is what we actually want in this case so we can filter
out commits from other subtrees.
> Also can $dir have regular expressions special characters? "The
> existing code and new code alike, git-subtree is not prepared to
> handle directory names with RE special characters well at all, so
> do not use them if you do not want your history broken" is an
> acceptable answer.
As far as I can tell from looking at the code (which I only recently
started using) the '$dir' which is based on the subtree prefix is
not setup to handle this.
> The caller of this function process_split_commit is cmd_split and
> process_split_commit (hence this function) is called repeatedly
> inside a loop. This function makes a traversal over the entire
> history for each and every iteration in "good" cases where there is
> no 'mainline' or 'subtree-dir' commits for the given $dir.
>
> I wonder if it is more efficient to enumerate all commits that hits
> these grep criteria in the cmd_split before it starts to call
> process_split_commit repeatedly. If it knows which commit can be
> ignored beforehand, it can skip and not call process_split_commit,
> no?
Moved this functionality into the 'cmd_split' function as suggested.
>> + then
>> + return 0
>> + fi
>> + fi
>> + return 1
>> +}
>> +
>> # Usage: process_split_commit REV PARENTS
>> process_split_commit () {
>> assert test $# = 2
>> local rev="$1"
>> local parents="$2"
>
> These seem to assume that $1 and $2 can have $IFS in them, so
> shouldn't ...
>
>> + if should_ignore_subtree_commit $rev
>
> ... this call too enclose $rev inside a pair of double-quotes for
> consistency? We know the loop in the cmd_split that calls this
> function is reading from "rev-list --parents" and $rev is a 40-hex
> commit object name (and $parents can have more than one 40-hex
> commit object names separated with SP), so it is safe to leave $rev
> unquoted, but it pays to be consistent to help make the code more
> readable.
Updated this for consistency
On Mon, Sep 18, 2023 at 9:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Zach FettersMoore via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > In the diagram below, 'M' represents the mainline repo branch, 'A'
> > represents one subtree, and 'B' represents another. M3 and B1 represent
> > a split commit for subtree B that was created from commit M4. M2 and A1
> > represent a split commit made from subtree A that was also created
> > based on changes back to and including M4. M1 represents new changes to
> > the repo, in this scenario if you try to run a 'git subtree split
> > --rejoin' for subtree B, commits M1, M2, and A1, will be included in
> > the processing of changes for the new split commit since the last
> > split/rejoin for subtree B was at M3. The issue is that by having A1
> > included in this processing the command ends up needing to processing
> > every commit down tree A even though none of that is needed or relevant
> > to the current command and result.
> >
> > M1
> > | \ \
> > M2 | |
> > | A1 |
> > M3 | |
> > | | B1
> > M4 | |
>
> The above paragraph explains which different things you drew in the
> diagram are representing, but it is not clear how they relate to
> each other. Do they for example depict parent-child commit
> relationship? What are the wide gaps between these three tracks and
> what are the short angled lines leaning to the left near the tip?
> Is the time/topology flowing from bottom to top?
>
> > diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> > index e0c5d3b0de6..e9250dfb019 100755
> > --- a/contrib/subtree/git-subtree.sh
> > +++ b/contrib/subtree/git-subtree.sh
> > @@ -778,12 +778,29 @@ ensure_valid_ref_format () {
> > die "fatal: '$1' does not look like a ref"
> > }
> >
> > +# Usage: check if a commit from another subtree should be ignored from processing for splits
>
> Way overlong line. Please split them accordingly. I won't comment
> on what CodingGuidelines tells us already, in this review, but have
> a few comments here:
>
> > +should_ignore_subtree_commit () {
> > + if [ "$(git log -1 --grep="git-subtree-dir:" $1)" ]
> > + then
> > + if [[ -z "$(git log -1 --grep="git-subtree-mainline:" $1)" && -z "$(git log -1 --grep="git-subtree-dir: $dir$" $1)" ]]
>
> Here $dir is a free variable that comes from outside. The caller
> does not supply it as a parameter to this function (and the caller
> does not receive it as its parameter from its caller). Yet the file
> as a whole seems to liberally make assignments to it ("git grep dir="
> on the file counts 7 assignments). Are we sure we are looking for
> the right $dir in this particular grep?
>
> Side note: I am not familiar with this part of the code at
> all, so do not take it as "here is a bug", but more as "this
> smells error prone."
>
> Also can $dir have regular expressions special characters? "The
> existing code and new code alike, git-subtree is not prepared to
> handle directory names with RE special characters well at all, so
> do not use them if you do not want your history broken" is an
> acceptable answer.
>
> The caller of this function process_split_commit is cmd_split and
> process_split_commit (hence this function) is called repeatedly
> inside a loop. This function makes a traversal over the entire
> history for each and every iteration in "good" cases where there is
> no 'mainline' or 'subtree-dir' commits for the given $dir.
>
> I wonder if it is more efficient to enumerate all commits that hits
> these grep criteria in the cmd_split before it starts to call
> process_split_commit repeatedly. If it knows which commit can be
> ignored beforehand, it can skip and not call process_split_commit,
> no?
>
> > + then
> > + return 0
> > + fi
> > + fi
> > + return 1
> > +}
> > +
> > # Usage: process_split_commit REV PARENTS
> > process_split_commit () {
> > assert test $# = 2
> > local rev="$1"
> > local parents="$2"
>
> These seem to assume that $1 and $2 can have $IFS in them, so
> shouldn't ...
>
> > + if should_ignore_subtree_commit $rev
>
> ... this call too enclose $rev inside a pair of double-quotes for
> consistency? We know the loop in the cmd_split that calls this
> function is reading from "rev-list --parents" and $rev is a 40-hex
> commit object name (and $parents can have more than one 40-hex
> commit object names separated with SP), so it is safe to leave $rev
> unquoted, but it pays to be consistent to help make the code more
> readable.
>
> > + then
> > + return
> > + fi
> > +
> > if test $indent -eq 0
> > then
> > revcount=$(($revcount + 1))
> >
> > base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518
^ permalink raw reply
* Re: [PATCH v4 0/2] bugreport: reject positional arguments
From: Eric Sunshine @ 2023-10-26 20:13 UTC (permalink / raw)
To: emilyshaffer; +Cc: git, Emily Shaffer, Sheik, Dragan Simic
In-Reply-To: <20231026182231.3369370-1-nasamuffin@google.com>
On Thu, Oct 26, 2023 at 2:22 PM <emilyshaffer@google.com> wrote:
> The test I cribbed from for the newly added one in patch 2 was still
> using test_i18ngrep, and Eric mentioned us not wanting i18ngrep at all
> anymore, so I went ahead and cleaned that up as well.
Thanks for making the various tweaks in response to my review comments.
^ permalink raw reply
* Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed
From: Emily Shaffer @ 2023-10-26 21:19 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git
In-Reply-To: <20231016214045.146862-2-jacob@initialcommit.io>
Thanks Jack for drawing my attention to the summoning, and sorry for
delay.
On Mon, Oct 16, 2023 at 02:40:45PM -0700, Jacob Stopak wrote:
>
> When the -s flag is absent, git bugreport includes the current hour and
> minute values in the default bugreport filename (and diagnostics zip
> filename if --diagnose is supplied).
>
> If a user runs the bugreport command more than once within a minute, a
> filename conflict with an existing file occurs and the program errors,
> since the new output filename was already used for the previous file. If
> the user waits anywhere from 1 to 60 seconds (depending on when during
> the minute the first command was run) the command works again with no
> error since the default filename is now unique, and multiple bug reports
> are able to be created with default settings.
>
> This is a minor thing but can cause confusion for first time users of
> the bugreport command, who are likely to run it multiple times in quick
> succession to learn how it works, (like I did). Or users who quickly
> fill in a few details before closing and creating a new one.
Sure, agreed - I guess I got in the habit of testing by running `rm
git-bugreport*.txt && git bugreport --foo` and never thought about this
being annoying for an actual reporter :)
>
> Add a '+i' into the bugreport filename suffix where 'i' is an integer
> starting at 1 and growing as needed until a unique filename is obtained.
>
> This leads to default output filenames like:
>
> git-bugreport-%Y-%m-%d-%H%M+1.txt
> git-bugreport-%Y-%m-%d-%H%M+2.txt
> ...
> git-bugreport-%Y-%m-%d-%H%M+i.txt
>
> This means the user will end up with multiple bugreport files being
> created if they run the command multiple times quickly, but that feels
> more intuitive and consistent than an error arbitrarily occuring within
> a minute, especially given that the time window in which the error
> currently occurs is variable as described above.
Sure, and with the monotonic increases it's quite easy to locate the
bugreport that's the final one the user generated. This scheme seems
fine to me.
>
> If --diagnose is supplied, match the incremented suffix of the
> diagnostics zip file to the bugreport.
Nice.
>
> Signed-off-by: Jacob Stopak <jacob@initialcommit.io>
> ---
> builtin/bugreport.c | 83 +++++++++++++++++++++++++++++++--------------
> 1 file changed, 57 insertions(+), 26 deletions(-)
>
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index d2ae5c305d..ed65735873 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -11,6 +11,7 @@
> #include "diagnose.h"
> #include "object-file.h"
> #include "setup.h"
> +#include "dir.h"
>
> static void get_system_info(struct strbuf *sys_info)
> {
> @@ -97,20 +98,41 @@ static void get_header(struct strbuf *buf, const char *title)
> strbuf_addf(buf, "\n\n[%s]\n", title);
> }
>
> +static void build_path(struct strbuf *buf, const char *dir_path,
> + const char *prefix, const char *suffix,
> + time_t t, int *i, const char *ext)
> +{
> + struct tm tm;
> +
> + strbuf_reset(buf);
> + strbuf_addstr(buf, dir_path);
> + strbuf_complete(buf, '/');
> +
> + strbuf_addstr(buf, prefix);
> + strbuf_addftime(buf, suffix, localtime_r(&t, &tm), 0, 0);
> +
> + if (*i > 0)
> + strbuf_addf(buf, "+%d", *i);
> +
> + strbuf_addstr(buf, ext);
> +
> + (*i)++;
> +}
I commented on the weirdness of having to decrement i for --diagnose
below, but I think I generally just wish that instead of build_path()
this function did create_file_with_optional_suffix() and returned the
final modified option_suffix(). Better still would be if this function
created (or at least tested) all the necessary output paths so you don't
end up succeeding in creating a bugreport.txt but failing in creating
the diagnostics.zip in some edge case, something like....
build_suffix(..., &option_suffix) {
... build timestamp ...
while (...)
for (final_path in eventual_paths) {
err = select(final_path);
if (err)
final_path = strcat(most_of_path, i)
else
break
(Yeah, that's very handwavey, but I hope you see what I'm getting at.)
Really, though, I mostly don't think I like leaving the control variable i raw
to the calling scope and making it be manipulated later. Fancy
pre-guessing-path-availability aside, I think you could achieve a more
pleasant solution even by letting build_path() become
create_file_with_optional_suffix() (that reports the optional suffix
eventually settled on).
> +
> int cmd_bugreport(int argc, const char **argv, const char *prefix)
> {
> struct strbuf buffer = STRBUF_INIT;
> struct strbuf report_path = STRBUF_INIT;
> int report = -1;
> time_t now = time(NULL);
> - struct tm tm;
> enum diagnose_mode diagnose = DIAGNOSE_NONE;
> char *option_output = NULL;
> - char *option_suffix = "%Y-%m-%d-%H%M";
> + char *option_suffix = "";
> + int option_suffix_is_from_user = 0;
> const char *user_relative_path = NULL;
> char *prefixed_filename;
> - size_t output_path_len;
> int ret;
> + int i = 0;
>
> const struct option bugreport_options[] = {
> OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"),
> @@ -126,16 +148,16 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> argc = parse_options(argc, argv, prefix, bugreport_options,
> bugreport_usage, 0);
>
> + if (!strlen(option_suffix))
> + option_suffix = "%Y-%m-%d-%H%M";
> + else
> + option_suffix_is_from_user = 1;
Looking at where this is used, it looks like you're saying "if the user
specified the suffix manually and has this problem, then that sucks for
them, they put their own foot in it". But I don't know if I necessarily
follow that logic - I'd just as soon drop the exception, append an int
to the user-provided suffix, and document that we'll do that in the
manpage.
(This isn't something I feel strongly about, except that I think it
makes the code harder to follow for not very notable user benefit. I
also didn't look through the reviews up until now, so if this was
already hashed back and forth, just go ahead and ignore me.)
> +
> /* Prepare the path to put the result */
> prefixed_filename = prefix_filename(prefix,
> option_output ? option_output : "");
> - strbuf_addstr(&report_path, prefixed_filename);
> - strbuf_complete(&report_path, '/');
> - output_path_len = report_path.len;
> -
> - strbuf_addstr(&report_path, "git-bugreport-");
> - strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
> - strbuf_addstr(&report_path, ".txt");
> + build_path(&report_path, prefixed_filename, "git-bugreport-",
> + option_suffix, now, &i, ".txt");
>
> switch (safe_create_leading_directories(report_path.buf)) {
> case SCLD_OK:
> @@ -146,20 +168,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> report_path.buf);
> }
>
> - /* Prepare diagnostics, if requested */
> - if (diagnose != DIAGNOSE_NONE) {
> - struct strbuf zip_path = STRBUF_INIT;
> - strbuf_add(&zip_path, report_path.buf, output_path_len);
> - strbuf_addstr(&zip_path, "git-diagnostics-");
> - strbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0);
> - strbuf_addstr(&zip_path, ".zip");
> -
> - if (create_diagnostics_archive(&zip_path, diagnose))
> - die_errno(_("unable to create diagnostics archive %s"), zip_path.buf);
> -
> - strbuf_release(&zip_path);
> - }
> -
> /* Prepare the report contents */
> get_bug_template(&buffer);
>
> @@ -169,14 +177,37 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> get_header(&buffer, _("Enabled Hooks"));
> get_populated_hooks(&buffer, !startup_info->have_repository);
>
> - /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */
> - report = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);
> + again:
> + /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */
> + report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);
> + if (report < 0 && errno == EEXIST && !option_suffix_is_from_user) {
> + build_path(&report_path, prefixed_filename,
> + "git-bugreport-", option_suffix, now, &i,
> + ".txt");
> + goto again;
> + } else if (report < 0) {
Nit, but the double-checking of (report < 0) bothers me a little. Is it
nicer if it's nested?
if (report < 0) {
if (errno == EEXIST) {
build_path(...);
goto again;
}
die_errno(_(...));
}
I like it a little more, but that's up to taste, I suppose.
> + die_errno(_("unable to open '%s'"), report_path.buf);
> + }
>
> if (write_in_full(report, buffer.buf, buffer.len) < 0)
> die_errno(_("unable to write to %s"), report_path.buf);
>
> close(report);
>
> + /* Prepare diagnostics, if requested */
> + if (diagnose != DIAGNOSE_NONE) {
> + struct strbuf zip_path = STRBUF_INIT;
> + i--; /* Undo last increment to match zipfile suffix to bugreport */
I understand why you're doing this, but I'd rather see it decremented
(or more care taken in the increment logic elsewhere) closer to where it
is being increment-and-checked. If someone wants to add another
associated file besides the report and the diagnostics, then the logic
for the decrement becomes complicated (what happens if I run `git
bugreport --diagnostics --desktop_screencap`? what if I run only `git
bugreport --desktop_screencap`?). Even without that potential pain,
reading this comment here means I have to say "oh wait, what did I read
above? hold on, let me page it back in".
> + build_path(&zip_path, prefixed_filename, "git-diagnostics-",
> + option_suffix, now, &i, ".zip");
> +
> + if (create_diagnostics_archive(&zip_path, diagnose))
> + die_errno(_("unable to create diagnostics archive %s"),
> + zip_path.buf);
> +
> + strbuf_release(&zip_path);
> + }
> +
> /*
> * We want to print the path relative to the user, but we still need the
> * path relative to us to give to the editor.
> --
> 2.42.0.297.g36452639b8
Last thing: it probably makes sense to mention this new behavior in the
manpage, especially if you'll apply that behavior to user-provided
suffixes too.
Thanks for your effort on the patch so far and again, sorry for the late
reply.
- Emily
^ permalink raw reply
* Re: [PATCH] Include gettext.h in MyFirstContribution tutorial
From: Emily Shaffer @ 2023-10-26 21:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jacob Stopak
In-Reply-To: <xmqqzg0fpqay.fsf@gitster.g>
On Wed, Oct 18, 2023 at 02:35:17PM -0700, Junio C Hamano wrote:
>
> Jacob Stopak <jacob@initialcommit.io> writes:
>
> > The tutorial in Documentation/MyFirstContribution.txt has steps to print
> > some text using the "_" function. However, this leads to compiler errors
> > when running "make" since "gettext.h" is not #included.
> >
> > Update docs with a note to #include "gettext.h" in "builtin/psuh.c".
> >
> > Signed-off-by: Jacob Stopak <jacob@initialcommit.io>
> > ---
> > Documentation/MyFirstContribution.txt | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
>
> Who's the first responder on this document these days? I think the
> "psuh" was Emily's invention, so sending it in her direction.
>
> Thanks.
Thanks for the nudge and sorry for the slow response (and thanks to Jack
for pointing out to me that I was delinquent).
I like this change. Nice touch disambiguating "function" in the
following paragraph.
Reviewed-by: Emily Shaffer <nasamuffin@google.com>
>
> > diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt
> > index 62d11a5cd7..7cfed60c2e 100644
> > --- a/Documentation/MyFirstContribution.txt
> > +++ b/Documentation/MyFirstContribution.txt
> > @@ -160,10 +160,11 @@ in order to keep the declarations alphabetically sorted:
> > int cmd_psuh(int argc, const char **argv, const char *prefix);
> > ----
> >
> > -Be sure to `#include "builtin.h"` in your `psuh.c`.
> > +Be sure to `#include "builtin.h"` in your `psuh.c`. You'll also need to
> > +`#include "gettext.h"` to use functions related to printing output text.
> >
> > -Go ahead and add some throwaway printf to that function. This is a decent
> > -starting point as we can now add build rules and register the command.
> > +Go ahead and add some throwaway printf to the `cmd_psuh` function. This is a
> > +decent starting point as we can now add build rules and register the command.
> >
> > NOTE: Your throwaway text, as well as much of the text you will be adding over
> > the course of this tutorial, is user-facing. That means it needs to be
^ permalink raw reply
* [PATCH] git-gui: sv.po: Update Swedish translation (576t0f0u)
From: Peter Krefting @ 2023-10-26 21:40 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 244 bytes --]
Please find the updated patch Gzip'ed attached to avoid character
encoding issues. Patch is also available as a pull request on GitHub
against the https://github.com/prati0100/git-gui repository.
--
\\// Peter - http://www.softwolves.pp.se/
[-- Attachment #2: Type: application/gzip, Size: 29757 bytes --]
^ permalink raw reply
* Re: git diagnose with invalid CLI argument does not report error
From: Eric Sunshine @ 2023-10-26 22:11 UTC (permalink / raw)
To: Bagas Sanjaya
Cc: Sheik, Git Mailing List, Elijah Newren, Calvin Wan,
Junio C Hamano, Ævar Arnfjörð Bjarmason,
Alex Henrie, Derrick Stolee, Victoria Dye, Kristoffer Haugsbakk
In-Reply-To: <ZTpJFUdE2U6pbV--@debian.me>
On Thu, Oct 26, 2023 at 7:10 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> On Thu, Oct 26, 2023 at 07:49:58AM +1100, Sheik wrote:
> > Hi Maintainers,
> >
> > Running git diagnose with an invalid CLI argument in a valid Git directory
> > does not report error. Expected behaviour would be that it reports an error.
> >
> > #Example shell commands which should have reported an error but continues to
> > succeed
> >
> > cd $ToAGitDirectory
> > git diagnose mod
> > git diagnose mode
> > git diagnose mode=all
>
> I can reproduce this only when the invalid parameter is a normal word:
> ```
> $ git diagnose huh
> ```
> But the command errors out on invalid flag:
> ```
> $ git diagnose -m
> ```
> Cc:'ing people who recently worked on builtin/diagnose.c for help.
A patch by Emily to fix this has been submitted[v4].
[v4]: https://lore.kernel.org/git/20231026182231.3369370-3-nasamuffin@google.com/
^ permalink raw reply
* Re: git diagnose with invalid CLI argument does not report error
From: Emily Shaffer @ 2023-10-26 22:41 UTC (permalink / raw)
To: Eric Sunshine
Cc: Bagas Sanjaya, Sheik, Git Mailing List, Elijah Newren, Calvin Wan,
Junio C Hamano, Ævar Arnfjörð Bjarmason,
Alex Henrie, Derrick Stolee, Victoria Dye, Kristoffer Haugsbakk
In-Reply-To: <CAPig+cSsB-2xxF7uQRU2h219+0-9++M_woLX3vNwiq1Uj1SiQQ@mail.gmail.com>
To be clear, that patch was for git-bugreport and doesn't cover
git-diagnose. I assume the fix ends up being pretty similar though.
On Thu, Oct 26, 2023 at 3:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Thu, Oct 26, 2023 at 7:10 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> > On Thu, Oct 26, 2023 at 07:49:58AM +1100, Sheik wrote:
> > > Hi Maintainers,
> > >
> > > Running git diagnose with an invalid CLI argument in a valid Git directory
> > > does not report error. Expected behaviour would be that it reports an error.
> > >
> > > #Example shell commands which should have reported an error but continues to
> > > succeed
> > >
> > > cd $ToAGitDirectory
> > > git diagnose mod
> > > git diagnose mode
> > > git diagnose mode=all
> >
> > I can reproduce this only when the invalid parameter is a normal word:
> > ```
> > $ git diagnose huh
> > ```
> > But the command errors out on invalid flag:
> > ```
> > $ git diagnose -m
> > ```
> > Cc:'ing people who recently worked on builtin/diagnose.c for help.
>
> A patch by Emily to fix this has been submitted[v4].
>
> [v4]: https://lore.kernel.org/git/20231026182231.3369370-3-nasamuffin@google.com/
>
^ permalink raw reply
* Re: git diagnose with invalid CLI argument does not report error
From: Eric Sunshine @ 2023-10-26 22:43 UTC (permalink / raw)
To: Emily Shaffer
Cc: Bagas Sanjaya, Sheik, Git Mailing List, Elijah Newren, Calvin Wan,
Junio C Hamano, Ævar Arnfjörð Bjarmason,
Alex Henrie, Derrick Stolee, Victoria Dye, Kristoffer Haugsbakk
In-Reply-To: <CAJoAoZ=a30QsMXHz+47haZ=QGkY8-QYccSuY_94mi9h9RMiBFA@mail.gmail.com>
On Thu, Oct 26, 2023 at 6:41 PM Emily Shaffer <nasamuffin@google.com> wrote:
> On Thu, Oct 26, 2023 at 3:12 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Thu, Oct 26, 2023 at 7:10 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> > > On Thu, Oct 26, 2023 at 07:49:58AM +1100, Sheik wrote:
> > > > Running git diagnose with an invalid CLI argument in a valid Git directory
> > > > does not report error. Expected behaviour would be that it reports an error.
> > >
> > > I can reproduce this only when the invalid parameter is a normal word:
> > > Cc:'ing people who recently worked on builtin/diagnose.c for help.
> >
> > A patch by Emily to fix this has been submitted[v4].
>
> To be clear, that patch was for git-bugreport and doesn't cover
> git-diagnose. I assume the fix ends up being pretty similar though.
My bad.
^ permalink raw reply
* [RFC PATCH v2 0/6] Noobify format for status, add, restore
From: Jacob Stopak @ 2023-10-26 22:46 UTC (permalink / raw)
To: git; +Cc: Jacob Stopak, Junio C Hamano, Dragan Simic, Oswald Buddenhagen
In-Reply-To: <20231020183947.463882-1-jacob@initialcommit.io>
Take into account reviewer feedback by doing several things differently:
* Rename this feature (for now) as "noob format mode" (or just "noob
mode") instead of the original "--table" verbiage. As pointed out,
this no longer ties the name of the setting to it's proposed
implementation detail as a table. Noob mode is not necessarily the
right name, just a placeholder for now. Unless people like it :D
* Instead of manually having to invoke the -t, --table every time this
format is to be used, set the config option "status.noob" to true.
Although this is logically tied to the status command, there are many
commands that produce status output, (and this series adds more), so
assume that if the user wants to see the status this way, that it
should be enabled whenever the status info is displayed.
* When running "git add" and "git restore" while noob mode is enabled,
perform the add/restore function as usual, but display the table
formatted output with arrows showing how file changes moved around.
Displaying the output in this understandable format after each
command execution allows the noob to immediately see what they did.
Although this series only implements for status, add, and restore,
this output format would make sense in other commands like rm, mv,
commit, clean, and stash.
* Works consistently with commands that already have a --dry-run
(-n) option. The dry run shows the exact same output, but
doesn't actually do the thing.
* If `advice.statusHints` is true, add a table footer with status hints.
Shorten these hints so that they are still clear but better fit into a
table. Make the hint text yellow to distinguish them. The hints only
appear when explicitly running "git status", which helps the user
answer the question "what can I do next?". Hints are omitted in
"impact" commands like add and restore. Having hints here distracts
from the file change moves being showed in the table by arrows.
TODO:
* "git status" outputs myriad other information depending on the state
of the repo, like branch info, merge conflicts, rebase info, bisect,
etc. Need to think about how to convey that info with the new setting.
* Some commands (like stash) might need more than 3 table columns to
display everything clearly.
* For destructive commands, think about adding a prompt describing the
effect, so the user can confirm before the action is taken.
* Fix horrible things in the patch series code.
* Probably other things.
Play around with it! It's fun!
Jacob Stopak (6):
status: add noob format from status.noob config
status: handle long paths in noob format
add: implement noob mode
add: set unique color for noob mode arrows
restore: implement noob mode
status: add advice status hints as table footer
Makefile | 2 +
builtin/add.c | 47 +++++--
builtin/checkout.c | 46 +++++--
builtin/commit.c | 157 +----------------------
commit.c | 2 +
noob.c | 198 +++++++++++++++++++++++++++++
noob.h | 21 ++++
read-cache-ll.h | 10 +-
read-cache.c | 41 +++++-
table.c | 301 +++++++++++++++++++++++++++++++++++++++++++++
table.h | 6 +
wt-status.c | 75 +++++++----
wt-status.h | 6 +
13 files changed, 708 insertions(+), 204 deletions(-)
create mode 100644 noob.c
create mode 100644 noob.h
create mode 100644 table.c
create mode 100644 table.h
--
2.42.0.404.g2bcc23f3db
^ permalink raw reply
* [RFC PATCH v2 1/6] status: add noob format from status.noob config
From: Jacob Stopak @ 2023-10-26 22:46 UTC (permalink / raw)
To: git; +Cc: Jacob Stopak
In-Reply-To: <20231026224615.675172-1-jacob@initialcommit.io>
Signed-off-by: Jacob Stopak <jacob@initialcommit.io>
---
Makefile | 1 +
builtin/commit.c | 7 +++
table.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++
table.h | 6 +++
wt-status.c | 72 +++++++++++++++++++----------
wt-status.h | 1 +
6 files changed, 179 insertions(+), 25 deletions(-)
create mode 100644 table.c
create mode 100644 table.h
diff --git a/Makefile b/Makefile
index 9c6a2f125f..a7399ca8f0 100644
--- a/Makefile
+++ b/Makefile
@@ -1155,6 +1155,7 @@ LIB_OBJS += submodule-config.o
LIB_OBJS += submodule.o
LIB_OBJS += symlinks.o
LIB_OBJS += tag.o
+LIB_OBJS += table.o
LIB_OBJS += tempfile.o
LIB_OBJS += thread-utils.o
LIB_OBJS += tmp-objdir.o
diff --git a/builtin/commit.c b/builtin/commit.c
index 7da5f92448..880c42f5b7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1430,6 +1430,13 @@ static int git_status_config(const char *k, const char *v,
status_deferred_config.status_format = STATUS_FORMAT_NONE;
return 0;
}
+ if (!strcmp(k, "status.noob")) {
+ if (git_config_bool(k, v))
+ status_deferred_config.status_format = STATUS_FORMAT_NOOB;
+ else
+ status_deferred_config.status_format = STATUS_FORMAT_NONE;
+ return 0;
+ }
if (!strcmp(k, "status.branch")) {
status_deferred_config.show_branch = git_config_bool(k, v);
return 0;
diff --git a/table.c b/table.c
new file mode 100644
index 0000000000..15600e117f
--- /dev/null
+++ b/table.c
@@ -0,0 +1,117 @@
+#define USE_THE_INDEX_VARIABLE
+#include "builtin.h"
+#include "gettext.h"
+#include "strbuf.h"
+#include "wt-status.h"
+#include "config.h"
+#include "string-list.h"
+#include "sys/ioctl.h"
+
+static const char *color(int slot, struct wt_status *s)
+{
+ const char *c = "";
+ if (want_color(s->use_color))
+ c = s->color_palette[slot];
+ if (slot == WT_STATUS_ONBRANCH && color_is_nil(c))
+ c = s->color_palette[WT_STATUS_HEADER];
+ return c;
+}
+
+static void build_table_border(struct strbuf *buf, int cols)
+{
+ strbuf_reset(buf);
+ strbuf_addchars(buf, '-', cols);
+}
+
+static void build_table_entry(struct strbuf *buf, char *entry, int cols)
+{
+ strbuf_reset(buf);
+ strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
+ strbuf_addstr(buf, entry);
+
+ /* Bump right padding if entry length is odd */
+ if (!(strlen(entry) % 2))
+ strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2 + 1);
+ else
+ strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
+}
+
+static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s)
+{
+ printf(_("|"));
+ color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", buf1->buf);
+ printf(_("|"));
+ color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%s", buf2->buf);
+ printf(_("|"));
+ color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%s", buf3->buf);
+ printf(_("|\n"));
+}
+
+void print_noob_status(struct wt_status *s)
+{
+ struct winsize w;
+ int cols;
+ struct strbuf table_border = STRBUF_INIT;
+ struct strbuf table_col_entry_1 = STRBUF_INIT;
+ struct strbuf table_col_entry_2 = STRBUF_INIT;
+ struct strbuf table_col_entry_3 = STRBUF_INIT;
+ struct string_list_item *item;
+
+ /* Get terminal width */
+ ioctl(STDOUT_FILENO, TIOCGWINSZ, &w);
+ cols = w.ws_col;
+
+ /* Ensure table is divisible into 3 even columns */
+ while (((cols - 1) % 3) > 0 || !(cols % 2)) {
+ cols -= 1;
+ }
+
+ build_table_border(&table_border, cols);
+ build_table_entry(&table_col_entry_1, "Untracked files", cols);
+ build_table_entry(&table_col_entry_2, "Unstaged changes", cols);
+ build_table_entry(&table_col_entry_3, "Staging area", cols);
+
+ /* Draw table header */
+ printf(_("%s\n"), table_border.buf);
+ printf(_("|%s|%s|%s|\n"), table_col_entry_1.buf, table_col_entry_2.buf, table_col_entry_3.buf);
+ printf(_("%s\n"), table_border.buf);
+
+ /* Draw table body */
+ for_each_string_list_item(item, &s->untracked) {
+ build_table_entry(&table_col_entry_1, item->string, cols);
+ build_table_entry(&table_col_entry_2, "", cols);
+ build_table_entry(&table_col_entry_3, "", cols);
+ print_table_body_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s);
+ }
+
+ for_each_string_list_item(item, &s->change) {
+ struct wt_status_change_data *d = item->util;
+ if (d->worktree_status && d->index_status) {
+ build_table_entry(&table_col_entry_1, "", cols);
+ build_table_entry(&table_col_entry_2, item->string, cols);
+ build_table_entry(&table_col_entry_3, item->string, cols);
+ } else if (d->worktree_status) {
+ build_table_entry(&table_col_entry_1, "", cols);
+ build_table_entry(&table_col_entry_2, item->string, cols);
+ build_table_entry(&table_col_entry_3, "", cols);
+ } else if (d->index_status) {
+ build_table_entry(&table_col_entry_1, "", cols);
+ build_table_entry(&table_col_entry_2, "", cols);
+ build_table_entry(&table_col_entry_3, item->string, cols);
+ }
+ print_table_body_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s);
+ }
+
+ if (!s->untracked.nr && !s->change.nr) {
+ build_table_entry(&table_col_entry_1, "-", cols);
+ build_table_entry(&table_col_entry_2, "-", cols);
+ build_table_entry(&table_col_entry_3, "-", cols);
+ printf(_("|%s|%s|%s|\n"), table_col_entry_1.buf, table_col_entry_2.buf, table_col_entry_3.buf);
+ }
+
+ printf(_("%s\n"), table_border.buf);
+ strbuf_release(&table_border);
+ strbuf_release(&table_col_entry_1);
+ strbuf_release(&table_col_entry_2);
+ strbuf_release(&table_col_entry_3);
+}
diff --git a/table.h b/table.h
new file mode 100644
index 0000000000..c9e8c386de
--- /dev/null
+++ b/table.h
@@ -0,0 +1,6 @@
+#ifndef TABLE_H
+#define TABLE_H
+
+void print_noob_status(struct wt_status *s);
+
+#endif /* TABLE_H */
diff --git a/wt-status.c b/wt-status.c
index 9f45bf6949..712807aa8f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -31,6 +31,7 @@
#include "lockfile.h"
#include "sequencer.h"
#include "fsmonitor-settings.h"
+#include "table.h"
#define AB_DELAY_WARNING_IN_MS (2 * 1000)
#define UF_DELAY_WARNING_IN_MS (2 * 1000)
@@ -1833,39 +1834,46 @@ static void wt_longstatus_print_state(struct wt_status *s)
show_sparse_checkout_in_use(s, state_color);
}
-static void wt_longstatus_print(struct wt_status *s)
+static void wt_longstatus_print_onwhat(struct wt_status *s, const char *branch_name)
{
+ const char *on_what = _("On branch ");
const char *branch_color = color(WT_STATUS_ONBRANCH, s);
const char *branch_status_color = color(WT_STATUS_HEADER, s);
+
+ if (!strcmp(branch_name, "HEAD")) {
+ branch_status_color = color(WT_STATUS_NOBRANCH, s);
+ if (s->state.rebase_in_progress ||
+ s->state.rebase_interactive_in_progress) {
+ if (s->state.rebase_interactive_in_progress)
+ on_what = _("interactive rebase in progress; onto ");
+ else
+ on_what = _("rebase in progress; onto ");
+ branch_name = s->state.onto;
+ } else if (s->state.detached_from) {
+ branch_name = s->state.detached_from;
+ if (s->state.detached_at)
+ on_what = _("HEAD detached at ");
+ else
+ on_what = _("HEAD detached from ");
+ } else {
+ branch_name = "";
+ on_what = _("Not currently on any branch.");
+ }
+ } else
+ skip_prefix(branch_name, "refs/heads/", &branch_name);
+
+ status_printf_more(s, branch_status_color, "%s", on_what);
+ status_printf_more(s, branch_color, "%s\n", branch_name);
+}
+
+static void wt_longstatus_print(struct wt_status *s)
+{
enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(s->repo);
if (s->branch) {
- const char *on_what = _("On branch ");
const char *branch_name = s->branch;
- if (!strcmp(branch_name, "HEAD")) {
- branch_status_color = color(WT_STATUS_NOBRANCH, s);
- if (s->state.rebase_in_progress ||
- s->state.rebase_interactive_in_progress) {
- if (s->state.rebase_interactive_in_progress)
- on_what = _("interactive rebase in progress; onto ");
- else
- on_what = _("rebase in progress; onto ");
- branch_name = s->state.onto;
- } else if (s->state.detached_from) {
- branch_name = s->state.detached_from;
- if (s->state.detached_at)
- on_what = _("HEAD detached at ");
- else
- on_what = _("HEAD detached from ");
- } else {
- branch_name = "";
- on_what = _("Not currently on any branch.");
- }
- } else
- skip_prefix(branch_name, "refs/heads/", &branch_name);
status_printf(s, color(WT_STATUS_HEADER, s), "%s", "");
- status_printf_more(s, branch_status_color, "%s", on_what);
- status_printf_more(s, branch_color, "%s\n", branch_name);
+ wt_longstatus_print_onwhat(s, branch_name);
if (!s->is_initial)
wt_longstatus_print_tracking(s);
}
@@ -2133,6 +2141,17 @@ static void wt_shortstatus_print(struct wt_status *s)
wt_shortstatus_other(it, s, "!!");
}
+static void wt_noobstatus_print(struct wt_status *s)
+{
+ if (s->show_branch) {
+ const char *branch_name = s->branch;
+ wt_longstatus_print_onwhat(s, branch_name);
+ wt_longstatus_print_tracking(s);
+ }
+
+ print_noob_status(s);
+}
+
static void wt_porcelain_print(struct wt_status *s)
{
s->use_color = 0;
@@ -2560,6 +2579,9 @@ void wt_status_print(struct wt_status *s)
case STATUS_FORMAT_LONG:
wt_longstatus_print(s);
break;
+ case STATUS_FORMAT_NOOB:
+ wt_noobstatus_print(s);
+ break;
}
trace2_region_leave("status", "print", s->repo);
diff --git a/wt-status.h b/wt-status.h
index ab9cc9d8f0..3f08f0d72b 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -73,6 +73,7 @@ enum wt_status_format {
STATUS_FORMAT_SHORT,
STATUS_FORMAT_PORCELAIN,
STATUS_FORMAT_PORCELAIN_V2,
+ STATUS_FORMAT_NOOB,
STATUS_FORMAT_UNSPECIFIED
};
--
2.42.0.404.g2bcc23f3db
^ 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