* [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
* [RFC PATCH v2 2/6] status: handle long paths in noob format
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>
---
table.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++-------
table.h | 2 +-
wt-status.c | 2 +-
3 files changed, 53 insertions(+), 9 deletions(-)
diff --git a/table.c b/table.c
index 15600e117f..d085f2a098 100644
--- a/table.c
+++ b/table.c
@@ -25,15 +25,44 @@ static void build_table_border(struct strbuf *buf, int cols)
static void build_table_entry(struct strbuf *buf, char *entry, int cols)
{
+ int len = strlen(entry);
+ size_t col_width = (cols / 3) - 5; /* subtract for padding */
+
strbuf_reset(buf);
- strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
+
+ /* Trim equally from both sides if it doesn't fit in column */
+ if (len > col_width) {
+ struct strbuf start_buf = STRBUF_INIT;
+ struct strbuf end_buf = STRBUF_INIT;
+ struct strbuf entry_buf = STRBUF_INIT;
+
+ strbuf_addstr(&start_buf, entry);
+ strbuf_addstr(&end_buf, entry);
+
+ strbuf_remove(&start_buf, col_width / 2, len - col_width / 2);
+ strbuf_remove(&end_buf, 0, len - col_width / 2);
+
+ strbuf_addstr(&entry_buf, start_buf.buf);
+ strbuf_addstr(&entry_buf, "...");
+ strbuf_addstr(&entry_buf, end_buf.buf);
+
+ entry = strbuf_detach(&entry_buf, &col_width);
+ len = strlen(entry);
+
+ strbuf_release(&start_buf);
+ strbuf_release(&end_buf);
+ strbuf_release(&entry_buf);
+ }
+
+ strbuf_addchars(buf, ' ', (cols / 3 - len - 1) / 2); /* left padding */
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);
+ /* right padding */
+ if (!(len % 2))
+ /* Bump right padding if entry length is odd */
+ strbuf_addchars(buf, ' ', (cols / 3 - len - 1) / 2 + 1);
else
- strbuf_addchars(buf, ' ', (cols / 3 - 1 - strlen(entry)) / 2);
+ strbuf_addchars(buf, ' ', (cols / 3 - len - 1) / 2);
}
static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s)
@@ -47,7 +76,7 @@ static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, stru
printf(_("|\n"));
}
-void print_noob_status(struct wt_status *s)
+void print_noob_status(struct wt_status *s, int add_advice)
{
struct winsize w;
int cols;
@@ -66,14 +95,29 @@ void print_noob_status(struct wt_status *s)
cols -= 1;
}
+ /* Draw table header */
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);
+
+ if (add_advice) {
+ build_table_entry(&table_col_entry_1, "(stage: git add <file>)", cols);
+ build_table_entry(&table_col_entry_2, "(stage: git add <file>)", cols);
+ build_table_entry(&table_col_entry_3, "(unstage: git restore --staged <file>)", cols);
+
+ printf(_("|%s|%s|%s|\n"), table_col_entry_1.buf, table_col_entry_2.buf, table_col_entry_3.buf);
+
+ build_table_entry(&table_col_entry_1, "", cols);
+ build_table_entry(&table_col_entry_2, "(discard: git restore --staged <file>)", 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);
/* Draw table body */
diff --git a/table.h b/table.h
index c9e8c386de..5dff7162a4 100644
--- a/table.h
+++ b/table.h
@@ -1,6 +1,6 @@
#ifndef TABLE_H
#define TABLE_H
-void print_noob_status(struct wt_status *s);
+void print_noob_status(struct wt_status *s, int i);
#endif /* TABLE_H */
diff --git a/wt-status.c b/wt-status.c
index 712807aa8f..b5899dcc98 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2149,7 +2149,7 @@ static void wt_noobstatus_print(struct wt_status *s)
wt_longstatus_print_tracking(s);
}
- print_noob_status(s);
+ print_noob_status(s, 0);
}
static void wt_porcelain_print(struct wt_status *s)
--
2.42.0.404.g2bcc23f3db
^ permalink raw reply related
* [RFC PATCH v2 4/6] add: set unique color for noob mode arrows
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>
---
table.c | 64 +++++++++++++++++++++++++++++++----------------------
wt-status.c | 1 +
wt-status.h | 1 +
3 files changed, 39 insertions(+), 27 deletions(-)
diff --git a/table.c b/table.c
index 527e38c07d..d29b311440 100644
--- a/table.c
+++ b/table.c
@@ -5,6 +5,7 @@
#include "wt-status.h"
#include "config.h"
#include "string-list.h"
+#include "color.h"
#include "sys/ioctl.h"
static const char *color(int slot, struct wt_status *s)
@@ -26,7 +27,7 @@ static void build_table_border(struct strbuf *buf, int cols)
static void build_table_entry(struct strbuf *buf, char *entry, int cols)
{
int len = strlen(entry);
- size_t col_width = (cols / 3) - 5; /* subtract for padding */
+ size_t col_width = (cols / 3) - 9; /* subtract for padding */
strbuf_reset(buf);
@@ -65,52 +66,51 @@ static void build_table_entry(struct strbuf *buf, char *entry, int cols)
strbuf_addchars(buf, ' ', (cols / 3 - len - 1) / 2);
}
-static void add_arrow_to_entry(struct strbuf *buf, int add_after_entry)
+static void build_arrow(struct strbuf *buf, struct strbuf* arrow, int add_after_entry)
{
struct strbuf empty = STRBUF_INIT;
struct strbuf trimmed = STRBUF_INIT;
- struct strbuf holder = STRBUF_INIT;
int len = strlen(buf->buf);
+ strbuf_reset(arrow);
strbuf_addstr(&trimmed, buf->buf);
strbuf_trim(&trimmed);
if (!strbuf_cmp(&trimmed, &empty) && !add_after_entry) {
strbuf_reset(buf);
- strbuf_addchars(buf, '-', len + 1);
+ strbuf_addchars(arrow, '-', len + 1);
} else if (add_after_entry) {
strbuf_rtrim(buf);
- strbuf_addchars(buf, ' ', 1);
- strbuf_addchars(buf, '-', len - strlen(buf->buf) + 1);
+ strbuf_addchars(arrow, ' ', 1);
+ strbuf_addchars(arrow, '-', len - strlen(buf->buf) + 1);
} else if (!add_after_entry) {
strbuf_ltrim(buf);
- strbuf_addchars(&holder, '-', len - strlen(buf->buf) - 2);
- strbuf_addchars(&holder, '>', 1);
- strbuf_addchars(&holder, ' ', 1);
- strbuf_addstr(&holder, buf->buf);
- strbuf_reset(buf);
- strbuf_addstr(buf, holder.buf);
+ strbuf_addchars(arrow, '-', len - strlen(buf->buf) - 3);
+ strbuf_addchars(arrow, '>', 1);
+ strbuf_addchars(arrow, ' ', 1);
}
}
-static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s, int hide_pipe)
+static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct strbuf *arrow1, struct strbuf *arrow2, struct strbuf *arrow3, struct wt_status *s, int hide_pipe)
{
printf(_("|"));
color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", buf1->buf);
+ if (strlen(arrow1->buf) > 0)
+ color_fprintf(s->fp, color(WT_STATUS_ARROW, s), "%s", arrow1->buf);
if (hide_pipe != 1 && hide_pipe != 3)
printf(_("|"));
color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%s", buf2->buf);
+ if (strlen(arrow2->buf) > 0)
+ color_fprintf(s->fp, color(WT_STATUS_ARROW, s), "%s", arrow2->buf);
if (hide_pipe != 2 && hide_pipe != 3)
printf(_("|"));
+ if (strlen(arrow3->buf) > 0) {
+ color_fprintf(s->fp, color(WT_STATUS_ARROW, s), "%s", arrow3->buf);
+ }
color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%s", buf3->buf);
printf(_("|\n"));
}
-static void print_table_body_line_(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s)
-{
- print_table_body_line(buf1, buf2, buf3, s, 0);
-}
-
void print_noob_status(struct wt_status *s, int advice)
{
struct winsize w;
@@ -119,6 +119,9 @@ void print_noob_status(struct wt_status *s, int advice)
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 strbuf arrow_1 = STRBUF_INIT;
+ struct strbuf arrow_2 = STRBUF_INIT;
+ struct strbuf arrow_3 = STRBUF_INIT;
struct string_list_item *item, *item2;
/* Get terminal width */
@@ -170,17 +173,21 @@ void print_noob_status(struct wt_status *s, int advice)
strbuf_addstr(&buf_2, item2->string);
if (!strbuf_cmp(&buf_1, &buf_2)) {
build_table_entry(&table_col_entry_3, buf_1.buf, cols);
- add_arrow_to_entry(&table_col_entry_1, 1);
- add_arrow_to_entry(&table_col_entry_2, 0);
- add_arrow_to_entry(&table_col_entry_3, 0);
+ build_arrow(&table_col_entry_1, &arrow_1, 1);
+ build_arrow(&table_col_entry_2, &arrow_2, 0);
+ build_arrow(&table_col_entry_3, &arrow_3, 0);
is_arrow = 1;
}
}
if (!is_arrow)
- print_table_body_line_(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s);
+ print_table_body_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, &arrow_1, &arrow_2, &arrow_3, s, 0);
else
- print_table_body_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s, 3);
+ print_table_body_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, &arrow_1, &arrow_2, &arrow_3, s, 3);
+
+ strbuf_reset(&arrow_1);
+ strbuf_reset(&arrow_2);
+ strbuf_reset(&arrow_3);
}
for_each_string_list_item(item, &s->change) {
@@ -203,8 +210,8 @@ void print_noob_status(struct wt_status *s, int advice)
strbuf_addstr(&buf_2, item2->string);
if (!strbuf_cmp(&buf_1, &buf_2)) {
build_table_entry(&table_col_entry_3, buf_1.buf, cols);
- add_arrow_to_entry(&table_col_entry_2, 1);
- add_arrow_to_entry(&table_col_entry_3, 0);
+ build_arrow(&table_col_entry_2, &arrow_2, 1);
+ build_arrow(&table_col_entry_3, &arrow_3, 0);
is_arrow = 1;
}
}
@@ -215,9 +222,12 @@ void print_noob_status(struct wt_status *s, int advice)
}
if (!is_arrow)
- print_table_body_line_(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s);
+ print_table_body_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, &arrow_1, &arrow_2, &arrow_3, s, 0);
else
- print_table_body_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s, 2);
+ print_table_body_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, &arrow_1, &arrow_2, &arrow_3, s, 2);
+ strbuf_reset(&arrow_1);
+ strbuf_reset(&arrow_2);
+ strbuf_reset(&arrow_3);
}
if (!s->untracked.nr && !s->change.nr) {
diff --git a/wt-status.c b/wt-status.c
index 969f79f441..1332d07dba 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -49,6 +49,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
GIT_COLOR_GREEN, /* WT_STATUS_LOCAL_BRANCH */
GIT_COLOR_RED, /* WT_STATUS_REMOTE_BRANCH */
GIT_COLOR_NIL, /* WT_STATUS_ONBRANCH */
+ GIT_COLOR_CYAN, /* WT_STATUS_ARROW */
};
static const char *color(int slot, struct wt_status *s)
diff --git a/wt-status.h b/wt-status.h
index 64551f3a75..7b883fd476 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -19,6 +19,7 @@ enum color_wt_status {
WT_STATUS_LOCAL_BRANCH,
WT_STATUS_REMOTE_BRANCH,
WT_STATUS_ONBRANCH,
+ WT_STATUS_ARROW,
WT_STATUS_MAXSLOT
};
--
2.42.0.404.g2bcc23f3db
^ permalink raw reply related
* [RFC PATCH v2 3/6] add: implement noob mode
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/add.c | 47 ++++++++---
builtin/commit.c | 163 +-------------------------------------
commit.c | 2 +
noob.c | 198 +++++++++++++++++++++++++++++++++++++++++++++++
noob.h | 21 +++++
read-cache-ll.h | 9 ++-
read-cache.c | 32 ++++++--
table.c | 92 +++++++++++++++++++---
wt-status.c | 1 +
wt-status.h | 1 +
11 files changed, 381 insertions(+), 186 deletions(-)
create mode 100644 noob.c
create mode 100644 noob.h
diff --git a/Makefile b/Makefile
index a7399ca8f0..78acfaf14d 100644
--- a/Makefile
+++ b/Makefile
@@ -1070,6 +1070,7 @@ LIB_OBJS += name-hash.o
LIB_OBJS += negotiator/default.o
LIB_OBJS += negotiator/noop.o
LIB_OBJS += negotiator/skipping.o
+LIB_OBJS += noob.o
LIB_OBJS += notes-cache.o
LIB_OBJS += notes-merge.o
LIB_OBJS += notes-utils.o
diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..dbb99d179e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -27,6 +27,9 @@
#include "strvec.h"
#include "submodule.h"
#include "add-interactive.h"
+#include "wt-status.h"
+#include "commit.h"
+#include "noob.h"
static const char * const builtin_add_usage[] = {
N_("git add [<options>] [--] <pathspec>..."),
@@ -322,7 +325,7 @@ static void check_embedded_repo(const char *path)
strbuf_release(&name);
}
-static int add_files(struct dir_struct *dir, int flags)
+static int add_files(struct dir_struct *dir, int flags, struct wt_status *status)
{
int i, exit_status = 0;
struct string_list matched_sparse_paths = STRING_LIST_INIT_NODUP;
@@ -345,7 +348,7 @@ static int add_files(struct dir_struct *dir, int flags)
dir->entries[i]->name);
continue;
}
- if (add_file_to_index(&the_index, dir->entries[i]->name, flags)) {
+ if (add_file_to_index_with_status(&the_index, dir->entries[i]->name, flags, status)) {
if (!ignore_add_errors)
die(_("adding files failed"));
exit_status = 1;
@@ -374,8 +377,14 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int require_pathspec;
char *seen = NULL;
struct lock_file lock_file = LOCK_INIT;
-
+ struct wt_status status;
+ unsigned int progress_flag = 0;
+
+ wt_status_prepare(the_repository, &status);
git_config(add_config, NULL);
+ git_config(git_status_config, &status);
+ finalize_deferred_config(&status);
+ status.status_format = status_format;
argc = parse_options(argc, argv, prefix, builtin_add_options,
builtin_add_usage, PARSE_OPT_KEEP_ARGV0);
@@ -459,7 +468,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
(intent_to_add ? ADD_CACHE_INTENT : 0) |
(ignore_add_errors ? ADD_CACHE_IGNORE_ERRORS : 0) |
(!(addremove || take_worktree_changes)
- ? ADD_CACHE_IGNORE_REMOVAL : 0));
+ ? ADD_CACHE_IGNORE_REMOVAL : 0) |
+ (status.status_format == STATUS_FORMAT_NOOB ? ADD_CACHE_FORMAT_NOOB : 0));
if (repo_read_index_preload(the_repository, &pathspec, 0) < 0)
die(_("index file corrupt"));
@@ -551,15 +561,32 @@ int cmd_add(int argc, const char **argv, const char *prefix)
begin_odb_transaction();
- if (add_renormalize)
+ if (status.status_format == STATUS_FORMAT_NOOB) {
+ /* Read index and populate status */
+ repo_read_index(the_repository);
+ refresh_index(&the_index,
+ REFRESH_QUIET|REFRESH_UNMERGED|progress_flag,
+ &status.pathspec, NULL, NULL);
+ status.show_branch = 0;
+ wt_status_collect(&status);
+ }
+
+ if (add_renormalize) {
exit_status |= renormalize_tracked_files(&pathspec, flags);
- else
- exit_status |= add_files_to_cache(the_repository, prefix,
+ } else {
+ exit_status |= add_files_to_cache_with_status(the_repository, prefix,
&pathspec, include_sparse,
- flags);
+ flags, &status);
+ }
- if (add_new_files)
- exit_status |= add_files(&dir, flags);
+ if (add_new_files) {
+ exit_status |= add_files(&dir, flags, &status);
+ }
+
+ if (status.status_format == STATUS_FORMAT_NOOB) {
+ wt_status_print(&status);
+ wt_status_collect_free_buffers(&status);
+ }
if (chmod_arg && pathspec.nr)
exit_status |= chmod_pathspec(&pathspec, chmod_arg[0], show_only);
diff --git a/builtin/commit.c b/builtin/commit.c
index 880c42f5b7..3f816c117d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -46,6 +46,7 @@
#include "commit-reach.h"
#include "commit-graph.h"
#include "pretty.h"
+#include "noob.h"
static const char * const builtin_commit_usage[] = {
N_("git commit [-a | --interactive | --patch] [-s] [-v] [-u<mode>] [--amend]\n"
@@ -148,8 +149,6 @@ static int use_editor = 1, include_status = 1;
static int have_option_m;
static struct strbuf message = STRBUF_INIT;
-static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
-
static int opt_pass_trailer(const struct option *opt, const char *arg, int unset)
{
BUG_ON_OPT_NEG(unset);
@@ -310,7 +309,7 @@ static void add_remove_files(struct string_list *list)
continue;
if (!lstat(p->string, &st)) {
- if (add_to_index(&the_index, p->string, &st, 0))
+ if (add_file_to_index(&the_index, p->string, 0))
die(_("updating files failed"));
} else
remove_file_from_index(&the_index, p->string);
@@ -1196,59 +1195,6 @@ static const char *read_commit_message(const char *name)
return repo_logmsg_reencode(the_repository, commit, NULL, out_enc);
}
-/*
- * Enumerate what needs to be propagated when --porcelain
- * is not in effect here.
- */
-static struct status_deferred_config {
- enum wt_status_format status_format;
- int show_branch;
- enum ahead_behind_flags ahead_behind;
-} status_deferred_config = {
- STATUS_FORMAT_UNSPECIFIED,
- -1, /* unspecified */
- AHEAD_BEHIND_UNSPECIFIED,
-};
-
-static void finalize_deferred_config(struct wt_status *s)
-{
- int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN &&
- status_format != STATUS_FORMAT_PORCELAIN_V2 &&
- !s->null_termination);
-
- if (s->null_termination) {
- if (status_format == STATUS_FORMAT_NONE ||
- status_format == STATUS_FORMAT_UNSPECIFIED)
- status_format = STATUS_FORMAT_PORCELAIN;
- else if (status_format == STATUS_FORMAT_LONG)
- die(_("options '%s' and '%s' cannot be used together"), "--long", "-z");
- }
-
- if (use_deferred_config && status_format == STATUS_FORMAT_UNSPECIFIED)
- status_format = status_deferred_config.status_format;
- if (status_format == STATUS_FORMAT_UNSPECIFIED)
- status_format = STATUS_FORMAT_NONE;
-
- if (use_deferred_config && s->show_branch < 0)
- s->show_branch = status_deferred_config.show_branch;
- if (s->show_branch < 0)
- s->show_branch = 0;
-
- /*
- * If the user did not give a "--[no]-ahead-behind" command
- * line argument *AND* we will print in a human-readable format
- * (short, long etc.) then we inherit from the status.aheadbehind
- * config setting. In all other cases (and porcelain V[12] formats
- * in particular), we inherit _FULL for backwards compatibility.
- */
- if (use_deferred_config &&
- s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
- s->ahead_behind_flags = status_deferred_config.ahead_behind;
-
- if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
- s->ahead_behind_flags = AHEAD_BEHIND_FULL;
-}
-
static void check_fixup_reword_options(int argc, const char *argv[]) {
if (whence != FROM_COMMIT) {
if (whence == FROM_MERGE)
@@ -1399,111 +1345,6 @@ static int dry_run_commit(const char **argv, const char *prefix,
define_list_config_array_extra(color_status_slots, {"added"});
-static int parse_status_slot(const char *slot)
-{
- if (!strcasecmp(slot, "added"))
- return WT_STATUS_UPDATED;
-
- return LOOKUP_CONFIG(color_status_slots, slot);
-}
-
-static int git_status_config(const char *k, const char *v,
- const struct config_context *ctx, void *cb)
-{
- struct wt_status *s = cb;
- const char *slot_name;
-
- if (starts_with(k, "column."))
- return git_column_config(k, v, "status", &s->colopts);
- if (!strcmp(k, "status.submodulesummary")) {
- int is_bool;
- s->submodule_summary = git_config_bool_or_int(k, v, ctx->kvi,
- &is_bool);
- if (is_bool && s->submodule_summary)
- s->submodule_summary = -1;
- return 0;
- }
- if (!strcmp(k, "status.short")) {
- if (git_config_bool(k, v))
- status_deferred_config.status_format = STATUS_FORMAT_SHORT;
- else
- 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;
- }
- if (!strcmp(k, "status.aheadbehind")) {
- status_deferred_config.ahead_behind = git_config_bool(k, v);
- return 0;
- }
- if (!strcmp(k, "status.showstash")) {
- s->show_stash = git_config_bool(k, v);
- return 0;
- }
- if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
- s->use_color = git_config_colorbool(k, v);
- return 0;
- }
- if (!strcmp(k, "status.displaycommentprefix")) {
- s->display_comment_prefix = git_config_bool(k, v);
- return 0;
- }
- if (skip_prefix(k, "status.color.", &slot_name) ||
- skip_prefix(k, "color.status.", &slot_name)) {
- int slot = parse_status_slot(slot_name);
- if (slot < 0)
- return 0;
- if (!v)
- return config_error_nonbool(k);
- return color_parse(v, s->color_palette[slot]);
- }
- if (!strcmp(k, "status.relativepaths")) {
- s->relative_paths = git_config_bool(k, v);
- return 0;
- }
- if (!strcmp(k, "status.showuntrackedfiles")) {
- if (!v)
- return config_error_nonbool(k);
- else if (!strcmp(v, "no"))
- s->show_untracked_files = SHOW_NO_UNTRACKED_FILES;
- else if (!strcmp(v, "normal"))
- s->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;
- else if (!strcmp(v, "all"))
- s->show_untracked_files = SHOW_ALL_UNTRACKED_FILES;
- else
- return error(_("Invalid untracked files mode '%s'"), v);
- return 0;
- }
- if (!strcmp(k, "diff.renamelimit")) {
- if (s->rename_limit == -1)
- s->rename_limit = git_config_int(k, v, ctx->kvi);
- return 0;
- }
- if (!strcmp(k, "status.renamelimit")) {
- s->rename_limit = git_config_int(k, v, ctx->kvi);
- return 0;
- }
- if (!strcmp(k, "diff.renames")) {
- if (s->detect_rename == -1)
- s->detect_rename = git_config_rename(k, v);
- return 0;
- }
- if (!strcmp(k, "status.renames")) {
- s->detect_rename = git_config_rename(k, v);
- return 0;
- }
- return git_diff_ui_config(k, v, ctx, NULL);
-}
-
int cmd_status(int argc, const char **argv, const char *prefix)
{
static int no_renames = -1;
diff --git a/commit.c b/commit.c
index b3223478bc..c08faf48fd 100644
--- a/commit.c
+++ b/commit.c
@@ -28,6 +28,8 @@
#include "shallow.h"
#include "tree.h"
#include "hook.h"
+#include "column.h"
+#include "config.h"
static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
diff --git a/noob.c b/noob.c
new file mode 100644
index 0000000000..680d461698
--- /dev/null
+++ b/noob.c
@@ -0,0 +1,198 @@
+#include "git-compat-util.h"
+#include "tag.h"
+#include "commit.h"
+#include "commit-graph.h"
+#include "environment.h"
+#include "gettext.h"
+#include "hex.h"
+#include "repository.h"
+#include "object-name.h"
+#include "object-store-ll.h"
+#include "pkt-line.h"
+#include "utf8.h"
+#include "diff.h"
+#include "revision.h"
+#include "notes.h"
+#include "alloc.h"
+#include "gpg-interface.h"
+#include "mergesort.h"
+#include "commit-slab.h"
+#include "prio-queue.h"
+#include "hash-lookup.h"
+#include "wt-status.h"
+#include "advice.h"
+#include "refs.h"
+#include "commit-reach.h"
+#include "run-command.h"
+#include "setup.h"
+#include "shallow.h"
+#include "tree.h"
+#include "hook.h"
+#include "column.h"
+#include "config.h"
+#include "noob.h"
+
+static const char *color_status_slots[] = {
+ [WT_STATUS_HEADER] = "header",
+ [WT_STATUS_UPDATED] = "updated",
+ [WT_STATUS_CHANGED] = "changed",
+ [WT_STATUS_UNTRACKED] = "untracked",
+ [WT_STATUS_NOBRANCH] = "noBranch",
+ [WT_STATUS_UNMERGED] = "unmerged",
+ [WT_STATUS_LOCAL_BRANCH] = "localBranch",
+ [WT_STATUS_REMOTE_BRANCH] = "remoteBranch",
+ [WT_STATUS_ONBRANCH] = "branch",
+};
+
+enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
+
+struct status_deferred_config status_deferred_config = {
+ STATUS_FORMAT_UNSPECIFIED,
+ -1, /* unspecified */
+ AHEAD_BEHIND_UNSPECIFIED,
+};
+
+int parse_status_slot(const char *slot)
+{
+ if (!strcasecmp(slot, "added"))
+ return WT_STATUS_UPDATED;
+
+ return LOOKUP_CONFIG(color_status_slots, slot);
+}
+
+int git_status_config(const char *k, const char *v,
+ const struct config_context *ctx, void *cb)
+{
+ struct wt_status *s = cb;
+ const char *slot_name;
+
+ if (starts_with(k, "column."))
+ return git_column_config(k, v, "status", &s->colopts);
+ if (!strcmp(k, "status.submodulesummary")) {
+ int is_bool;
+ s->submodule_summary = git_config_bool_or_int(k, v, ctx->kvi,
+ &is_bool);
+ if (is_bool && s->submodule_summary)
+ s->submodule_summary = -1;
+ return 0;
+ }
+ if (!strcmp(k, "status.short")) {
+ if (git_config_bool(k, v))
+ status_deferred_config.status_format = STATUS_FORMAT_SHORT;
+ else
+ 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;
+ }
+ if (!strcmp(k, "status.aheadbehind")) {
+ status_deferred_config.ahead_behind = git_config_bool(k, v);
+ return 0;
+ }
+ if (!strcmp(k, "status.showstash")) {
+ s->show_stash = git_config_bool(k, v);
+ return 0;
+ }
+ if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
+ s->use_color = git_config_colorbool(k, v);
+ return 0;
+ }
+ if (!strcmp(k, "status.displaycommentprefix")) {
+ s->display_comment_prefix = git_config_bool(k, v);
+ return 0;
+ }
+ if (skip_prefix(k, "status.color.", &slot_name) ||
+ skip_prefix(k, "color.status.", &slot_name)) {
+ int slot = parse_status_slot(slot_name);
+ if (slot < 0)
+ return 0;
+ if (!v)
+ return config_error_nonbool(k);
+ return color_parse(v, s->color_palette[slot]);
+ }
+ if (!strcmp(k, "status.relativepaths")) {
+ s->relative_paths = git_config_bool(k, v);
+ return 0;
+ }
+ if (!strcmp(k, "status.showuntrackedfiles")) {
+ if (!v)
+ return config_error_nonbool(k);
+ else if (!strcmp(v, "no"))
+ s->show_untracked_files = SHOW_NO_UNTRACKED_FILES;
+ else if (!strcmp(v, "normal"))
+ s->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;
+ else if (!strcmp(v, "all"))
+ s->show_untracked_files = SHOW_ALL_UNTRACKED_FILES;
+ else
+ return error(_("Invalid untracked files mode '%s'"), v);
+ return 0;
+ }
+ if (!strcmp(k, "diff.renamelimit")) {
+ if (s->rename_limit == -1)
+ s->rename_limit = git_config_int(k, v, ctx->kvi);
+ return 0;
+ }
+ if (!strcmp(k, "status.renamelimit")) {
+ s->rename_limit = git_config_int(k, v, ctx->kvi);
+ return 0;
+ }
+ if (!strcmp(k, "diff.renames")) {
+ if (s->detect_rename == -1)
+ s->detect_rename = git_config_rename(k, v);
+ return 0;
+ }
+ if (!strcmp(k, "status.renames")) {
+ s->detect_rename = git_config_rename(k, v);
+ return 0;
+ }
+ return git_diff_ui_config(k, v, ctx, NULL);
+}
+
+void finalize_deferred_config(struct wt_status *s)
+{
+ int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN &&
+ status_format != STATUS_FORMAT_PORCELAIN_V2 &&
+ !s->null_termination);
+
+ if (s->null_termination) {
+ if (status_format == STATUS_FORMAT_NONE ||
+ status_format == STATUS_FORMAT_UNSPECIFIED)
+ status_format = STATUS_FORMAT_PORCELAIN;
+ else if (status_format == STATUS_FORMAT_LONG)
+ die(_("options '%s' and '%s' cannot be used together"), "--long", "-z");
+ }
+
+ if (use_deferred_config && status_format == STATUS_FORMAT_UNSPECIFIED) {
+ status_format = status_deferred_config.status_format;
+ }
+ if (status_format == STATUS_FORMAT_UNSPECIFIED)
+ status_format = STATUS_FORMAT_NONE;
+
+ if (use_deferred_config && s->show_branch < 0)
+ s->show_branch = status_deferred_config.show_branch;
+ if (s->show_branch < 0)
+ s->show_branch = 0;
+
+ /*
+ * If the user did not give a "--[no]-ahead-behind" command
+ * line argument *AND* we will print in a human-readable format
+ * (short, long etc.) then we inherit from the status.aheadbehind
+ * config setting. In all other cases (and porcelain V[12] formats
+ * in particular), we inherit _FULL for backwards compatibility.
+ */
+ if (use_deferred_config &&
+ s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
+ s->ahead_behind_flags = status_deferred_config.ahead_behind;
+
+ if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
+ s->ahead_behind_flags = AHEAD_BEHIND_FULL;
+}
diff --git a/noob.h b/noob.h
new file mode 100644
index 0000000000..d5bc073594
--- /dev/null
+++ b/noob.h
@@ -0,0 +1,21 @@
+#ifndef NOOB_H
+#define NOOB_H
+
+struct status_deferred_config {
+ enum wt_status_format status_format;
+ int show_branch;
+ enum ahead_behind_flags ahead_behind;
+};
+
+extern enum wt_status_format status_format;
+
+extern struct status_deferred_config status_deferred_config;
+
+int git_status_config(const char *k, const char *v,
+ const struct config_context *ctx, void *cb);
+
+int parse_status_slot(const char *slot);
+
+void finalize_deferred_config(struct wt_status *s);
+
+#endif /* NOOB_H */
diff --git a/read-cache-ll.h b/read-cache-ll.h
index 9a1a7edc5a..302a075714 100644
--- a/read-cache-ll.h
+++ b/read-cache-ll.h
@@ -4,6 +4,7 @@
#include "hash-ll.h"
#include "hashmap.h"
#include "statinfo.h"
+#include "wt-status.h"
/*
* Basic data structures for the directory cache
@@ -395,6 +396,7 @@ int remove_file_from_index(struct index_state *, const char *path);
#define ADD_CACHE_IGNORE_ERRORS 4
#define ADD_CACHE_IGNORE_REMOVAL 8
#define ADD_CACHE_INTENT 16
+#define ADD_CACHE_FORMAT_NOOB 32
/*
* These two are used to add the contents of the file at path
* to the index, marking the working tree up-to-date by storing
@@ -404,7 +406,8 @@ int remove_file_from_index(struct index_state *, const char *path);
* the latter will do necessary lstat(2) internally before
* calling the former.
*/
-int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
+int add_to_index(struct index_state *, const char *path, struct stat *, int flags, struct wt_status *status);
+int add_file_to_index_with_status(struct index_state *, const char *path, int flags, struct wt_status *status);
int add_file_to_index(struct index_state *, const char *path, int flags);
int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip);
@@ -475,6 +478,10 @@ int add_files_to_cache(struct repository *repo, const char *prefix,
const struct pathspec *pathspec, int include_sparse,
int flags);
+int add_files_to_cache_with_status(struct repository *repo, const char *prefix,
+ const struct pathspec *pathspec, int include_sparse,
+ int flags, struct wt_status *status);
+
void overlay_tree_on_index(struct index_state *istate,
const char *tree_name, const char *prefix);
diff --git a/read-cache.c b/read-cache.c
index 080bd39713..319415430a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -45,6 +45,8 @@
#include "csum-file.h"
#include "promisor-remote.h"
#include "hook.h"
+#include "wt-status.h"
+#include "string-list.h"
/* Mask for the name length in ce_flags in the on-disk index */
@@ -664,7 +666,7 @@ void set_object_name_for_intent_to_add_entry(struct cache_entry *ce)
oidcpy(&ce->oid, &oid);
}
-int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags)
+int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags, struct wt_status *status)
{
int namelen, was_same;
mode_t st_mode = st->st_mode;
@@ -672,6 +674,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
unsigned ce_option = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE|CE_MATCH_RACY_IS_DIRTY;
int verbose = flags & (ADD_CACHE_VERBOSE | ADD_CACHE_PRETEND);
int pretend = flags & ADD_CACHE_PRETEND;
+ int noob = flags & ADD_CACHE_FORMAT_NOOB;
int intent_only = flags & ADD_CACHE_INTENT;
int add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|
(intent_only ? ADD_CACHE_NEW_ONLY : 0));
@@ -760,17 +763,26 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
discard_cache_entry(ce);
return error(_("unable to add '%s' to index"), path);
}
- if (verbose && !was_same)
+ if (verbose && !was_same && !noob)
printf("add '%s'\n", path);
+ if (noob && !was_same) {
+ string_list_insert(&status->added, path);
+ }
return 0;
}
-int add_file_to_index(struct index_state *istate, const char *path, int flags)
+int add_file_to_index_with_status(struct index_state *istate, const char *path, int flags, struct wt_status *status)
{
struct stat st;
if (lstat(path, &st))
die_errno(_("unable to stat '%s'"), path);
- return add_to_index(istate, path, &st, flags);
+ return add_to_index(istate, path, &st, flags, status);
+}
+
+int add_file_to_index(struct index_state *istate, const char *path, int flags)
+{
+ struct wt_status status;
+ return add_file_to_index_with_status(istate, path, flags, &status);
}
struct cache_entry *make_empty_cache_entry(struct index_state *istate, size_t len)
@@ -3872,6 +3884,7 @@ struct update_callback_data {
int include_sparse;
int flags;
int add_errors;
+ struct wt_status *status;
};
static int fix_unmerged_status(struct diff_filepair *p,
@@ -3914,7 +3927,7 @@ static void update_callback(struct diff_queue_struct *q,
die(_("unexpected diff status %c"), p->status);
case DIFF_STATUS_MODIFIED:
case DIFF_STATUS_TYPE_CHANGED:
- if (add_file_to_index(data->index, path, data->flags)) {
+ if (add_file_to_index_with_status(data->index, path, data->flags, data->status)) {
if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
die(_("updating files failed"));
data->add_errors++;
@@ -3935,6 +3948,14 @@ static void update_callback(struct diff_queue_struct *q,
int add_files_to_cache(struct repository *repo, const char *prefix,
const struct pathspec *pathspec, int include_sparse,
int flags)
+{
+ struct wt_status status;
+ return add_files_to_cache_with_status(repo, prefix, pathspec, include_sparse, flags, &status);
+}
+
+int add_files_to_cache_with_status(struct repository *repo, const char *prefix,
+ const struct pathspec *pathspec, int include_sparse,
+ int flags, struct wt_status *status)
{
struct update_callback_data data;
struct rev_info rev;
@@ -3943,6 +3964,7 @@ int add_files_to_cache(struct repository *repo, const char *prefix,
data.index = repo->index;
data.include_sparse = include_sparse;
data.flags = flags;
+ data.status = status;
repo_init_revisions(repo, &rev, prefix);
setup_revisions(0, NULL, &rev, NULL);
diff --git a/table.c b/table.c
index d085f2a098..527e38c07d 100644
--- a/table.c
+++ b/table.c
@@ -65,18 +65,53 @@ static void build_table_entry(struct strbuf *buf, char *entry, int cols)
strbuf_addchars(buf, ' ', (cols / 3 - len - 1) / 2);
}
-static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s)
+static void add_arrow_to_entry(struct strbuf *buf, int add_after_entry)
+{
+ struct strbuf empty = STRBUF_INIT;
+ struct strbuf trimmed = STRBUF_INIT;
+ struct strbuf holder = STRBUF_INIT;
+ int len = strlen(buf->buf);
+
+ strbuf_addstr(&trimmed, buf->buf);
+ strbuf_trim(&trimmed);
+
+ if (!strbuf_cmp(&trimmed, &empty) && !add_after_entry) {
+ strbuf_reset(buf);
+ strbuf_addchars(buf, '-', len + 1);
+ } else if (add_after_entry) {
+ strbuf_rtrim(buf);
+ strbuf_addchars(buf, ' ', 1);
+ strbuf_addchars(buf, '-', len - strlen(buf->buf) + 1);
+ } else if (!add_after_entry) {
+ strbuf_ltrim(buf);
+ strbuf_addchars(&holder, '-', len - strlen(buf->buf) - 2);
+ strbuf_addchars(&holder, '>', 1);
+ strbuf_addchars(&holder, ' ', 1);
+ strbuf_addstr(&holder, buf->buf);
+ strbuf_reset(buf);
+ strbuf_addstr(buf, holder.buf);
+ }
+}
+
+static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s, int hide_pipe)
{
printf(_("|"));
color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", buf1->buf);
- printf(_("|"));
+ if (hide_pipe != 1 && hide_pipe != 3)
+ printf(_("|"));
color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%s", buf2->buf);
- printf(_("|"));
+ if (hide_pipe != 2 && hide_pipe != 3)
+ printf(_("|"));
color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%s", buf3->buf);
printf(_("|\n"));
}
-void print_noob_status(struct wt_status *s, int add_advice)
+static void print_table_body_line_(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s)
+{
+ print_table_body_line(buf1, buf2, buf3, s, 0);
+}
+
+void print_noob_status(struct wt_status *s, int advice)
{
struct winsize w;
int cols;
@@ -84,7 +119,7 @@ void print_noob_status(struct wt_status *s, int add_advice)
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;
+ struct string_list_item *item, *item2;
/* Get terminal width */
ioctl(STDOUT_FILENO, TIOCGWINSZ, &w);
@@ -104,7 +139,7 @@ void print_noob_status(struct wt_status *s, int add_advice)
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);
- if (add_advice) {
+ if (advice) {
build_table_entry(&table_col_entry_1, "(stage: git add <file>)", cols);
build_table_entry(&table_col_entry_2, "(stage: git add <file>)", cols);
build_table_entry(&table_col_entry_3, "(unstage: git restore --staged <file>)", cols);
@@ -122,14 +157,38 @@ void print_noob_status(struct wt_status *s, int add_advice)
/* Draw table body */
for_each_string_list_item(item, &s->untracked) {
- build_table_entry(&table_col_entry_1, item->string, cols);
+ struct strbuf buf_1 = STRBUF_INIT;
+ struct strbuf buf_2 = STRBUF_INIT;
+ int is_arrow = 0;
+ strbuf_addstr(&buf_1, item->string);
+ build_table_entry(&table_col_entry_1, buf_1.buf, 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(item2, &s->added) {
+ strbuf_reset(&buf_2);
+ strbuf_addstr(&buf_2, item2->string);
+ if (!strbuf_cmp(&buf_1, &buf_2)) {
+ build_table_entry(&table_col_entry_3, buf_1.buf, cols);
+ add_arrow_to_entry(&table_col_entry_1, 1);
+ add_arrow_to_entry(&table_col_entry_2, 0);
+ add_arrow_to_entry(&table_col_entry_3, 0);
+ is_arrow = 1;
+ }
+ }
+
+ if (!is_arrow)
+ print_table_body_line_(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s);
+ else
+ print_table_body_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s, 3);
}
for_each_string_list_item(item, &s->change) {
struct wt_status_change_data *d = item->util;
+ struct strbuf buf_1 = STRBUF_INIT;
+ struct strbuf buf_2 = STRBUF_INIT;
+ int is_arrow = 0;
+ strbuf_addstr(&buf_1, item->string);
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);
@@ -138,12 +197,27 @@ void print_noob_status(struct wt_status *s, int add_advice)
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);
+
+ for_each_string_list_item(item2, &s->added) {
+ strbuf_reset(&buf_2);
+ strbuf_addstr(&buf_2, item2->string);
+ if (!strbuf_cmp(&buf_1, &buf_2)) {
+ build_table_entry(&table_col_entry_3, buf_1.buf, cols);
+ add_arrow_to_entry(&table_col_entry_2, 1);
+ add_arrow_to_entry(&table_col_entry_3, 0);
+ is_arrow = 1;
+ }
+ }
} 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 (!is_arrow)
+ print_table_body_line_(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s);
+ else
+ print_table_body_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s, 2);
}
if (!s->untracked.nr && !s->change.nr) {
diff --git a/wt-status.c b/wt-status.c
index b5899dcc98..969f79f441 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -153,6 +153,7 @@ void wt_status_prepare(struct repository *r, struct wt_status *s)
s->change.strdup_strings = 1;
s->untracked.strdup_strings = 1;
s->ignored.strdup_strings = 1;
+ s->added.strdup_strings = 1;
s->show_branch = -1; /* unspecified */
s->show_stash = 0;
s->ahead_behind_flags = AHEAD_BEHIND_UNSPECIFIED;
diff --git a/wt-status.h b/wt-status.h
index 3f08f0d72b..64551f3a75 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -142,6 +142,7 @@ struct wt_status {
struct string_list change;
struct string_list untracked;
struct string_list ignored;
+ struct string_list added;
uint32_t untracked_in_ms;
};
--
2.42.0.404.g2bcc23f3db
^ permalink raw reply related
* [RFC PATCH v2 5/6] restore: implement noob mode
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>
---
builtin/checkout.c | 46 ++++++++++++++++++++++++++++-------
read-cache-ll.h | 1 +
read-cache.c | 9 ++++++-
table.c | 60 +++++++++++++++++++++++++++++++++++++++-------
wt-status.h | 1 +
5 files changed, 100 insertions(+), 17 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f02434bc15..afc414b0b1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -41,6 +41,7 @@
#include "entry.h"
#include "parallel-checkout.h"
#include "add-interactive.h"
+#include "noob.h"
static const char * const checkout_usage[] = {
N_("git checkout [<options>] <branch>"),
@@ -456,7 +457,8 @@ static int checkout_worktree(const struct checkout_opts *opts,
}
static int checkout_paths(const struct checkout_opts *opts,
- const struct branch_info *new_branch_info)
+ const struct branch_info *new_branch_info,
+ struct wt_status *status)
{
int pos;
static char *ps_matched;
@@ -598,8 +600,10 @@ static int checkout_paths(const struct checkout_opts *opts,
for (pos = 0; pos < the_index.cache_nr; pos++) {
const struct cache_entry *ce = the_index.cache[pos];
if (ce->ce_flags & CE_MATCHED) {
- if (!ce_stage(ce))
+ if (!ce_stage(ce)) {
+ string_list_insert(&status->restored, ce->name);
continue;
+ }
if (opts->ignore_unmerged) {
if (!opts->quiet)
warning(_("path '%s' is unmerged"), ce->name);
@@ -621,7 +625,7 @@ static int checkout_paths(const struct checkout_opts *opts,
if (opts->checkout_worktree)
errs |= checkout_worktree(opts, new_branch_info);
else
- remove_marked_cache_entries(&the_index, 1);
+ remove_marked_cache_entries_with_status(&the_index, 1, status);
/*
* Allow updating the index when checking out from the index.
@@ -1668,7 +1672,8 @@ static char cb_option = 'b';
static int checkout_main(int argc, const char **argv, const char *prefix,
struct checkout_opts *opts, struct option *options,
const char * const usagestr[],
- struct branch_info *new_branch_info)
+ struct branch_info *new_branch_info,
+ struct wt_status *status)
{
int parseopt_flags = 0;
@@ -1865,7 +1870,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
}
if (opts->patch_mode || opts->pathspec.nr)
- return checkout_paths(opts, new_branch_info);
+ return checkout_paths(opts, new_branch_info, status);
else
return checkout_branch(opts, new_branch_info);
}
@@ -1887,6 +1892,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
};
int ret;
struct branch_info new_branch_info = { 0 };
+ struct wt_status status;
memset(&opts, 0, sizeof(opts));
opts.dwim_new_local_branch = 1;
@@ -1917,7 +1923,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
options = add_checkout_path_options(&opts, options);
ret = checkout_main(argc, argv, prefix, &opts,
- options, checkout_usage, &new_branch_info);
+ options, checkout_usage,
+ &new_branch_info, &status);
branch_info_release(&new_branch_info);
clear_pathspec(&opts.pathspec);
free(opts.pathspec_from_file);
@@ -1942,6 +1949,7 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
};
int ret;
struct branch_info new_branch_info = { 0 };
+ struct wt_status status;
memset(&opts, 0, sizeof(opts));
opts.dwim_new_local_branch = 1;
@@ -1961,7 +1969,8 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
cb_option = 'c';
ret = checkout_main(argc, argv, prefix, &opts,
- options, switch_branch_usage, &new_branch_info);
+ options, switch_branch_usage,
+ &new_branch_info, &status);
branch_info_release(&new_branch_info);
FREE_AND_NULL(options);
return ret;
@@ -1985,6 +1994,13 @@ int cmd_restore(int argc, const char **argv, const char *prefix)
};
int ret;
struct branch_info new_branch_info = { 0 };
+ struct wt_status status;
+ unsigned int progress_flag = 0;
+
+ wt_status_prepare(the_repository, &status);
+ git_config(git_status_config, &status);
+ finalize_deferred_config(&status);
+ status.status_format = status_format;
memset(&opts, 0, sizeof(opts));
opts.accept_ref = 0;
@@ -2000,7 +2016,21 @@ int cmd_restore(int argc, const char **argv, const char *prefix)
options = add_checkout_path_options(&opts, options);
ret = checkout_main(argc, argv, prefix, &opts,
- options, restore_usage, &new_branch_info);
+ options, restore_usage,
+ &new_branch_info, &status);
+
+ if (status.status_format == STATUS_FORMAT_NOOB) {
+ /* Read index and populate status */
+ repo_read_index(the_repository);
+ refresh_index(&the_index,
+ REFRESH_QUIET|REFRESH_UNMERGED|progress_flag,
+ &status.pathspec, NULL, NULL);
+ status.show_branch = 0;
+ wt_status_collect(&status);
+ wt_status_print(&status);
+ wt_status_collect_free_buffers(&status);
+ }
+
branch_info_release(&new_branch_info);
FREE_AND_NULL(options);
return ret;
diff --git a/read-cache-ll.h b/read-cache-ll.h
index 302a075714..8bdc157196 100644
--- a/read-cache-ll.h
+++ b/read-cache-ll.h
@@ -389,6 +389,7 @@ void rename_index_entry_at(struct index_state *, int pos, const char *new_name);
/* Remove entry, return true if there are more entries to go. */
int remove_index_entry_at(struct index_state *, int pos);
+void remove_marked_cache_entries_with_status(struct index_state *istate, int invalidate, struct wt_status *status);
void remove_marked_cache_entries(struct index_state *istate, int invalidate);
int remove_file_from_index(struct index_state *, const char *path);
#define ADD_CACHE_VERBOSE 1
diff --git a/read-cache.c b/read-cache.c
index 319415430a..1c1a3290c0 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -558,7 +558,7 @@ int remove_index_entry_at(struct index_state *istate, int pos)
* CE_REMOVE is set in ce_flags. This is much more effective than
* calling remove_index_entry_at() for each entry to be removed.
*/
-void remove_marked_cache_entries(struct index_state *istate, int invalidate)
+void remove_marked_cache_entries_with_status(struct index_state *istate, int invalidate, struct wt_status *status)
{
struct cache_entry **ce_array = istate->cache;
unsigned int i, j;
@@ -570,6 +570,7 @@ void remove_marked_cache_entries(struct index_state *istate, int invalidate)
ce_array[i]->name);
untracked_cache_remove_from_index(istate,
ce_array[i]->name);
+ string_list_insert(&status->restored, ce_array[i]->name);
}
remove_name_hash(istate, ce_array[i]);
save_or_free_index_entry(istate, ce_array[i]);
@@ -583,6 +584,12 @@ void remove_marked_cache_entries(struct index_state *istate, int invalidate)
istate->cache_nr = j;
}
+void remove_marked_cache_entries(struct index_state *istate, int invalidate)
+{
+ struct wt_status status;
+ remove_marked_cache_entries_with_status(istate, invalidate, &status);
+}
+
int remove_file_from_index(struct index_state *istate, const char *path)
{
int pos = index_name_pos(istate, path, strlen(path));
diff --git a/table.c b/table.c
index d29b311440..3602def17a 100644
--- a/table.c
+++ b/table.c
@@ -66,7 +66,7 @@ static void build_table_entry(struct strbuf *buf, char *entry, int cols)
strbuf_addchars(buf, ' ', (cols / 3 - len - 1) / 2);
}
-static void build_arrow(struct strbuf *buf, struct strbuf* arrow, int add_after_entry)
+static void build_arrow_(struct strbuf *buf, struct strbuf* arrow, int add_after_entry, int reversed)
{
struct strbuf empty = STRBUF_INIT;
struct strbuf trimmed = STRBUF_INIT;
@@ -80,17 +80,38 @@ static void build_arrow(struct strbuf *buf, struct strbuf* arrow, int add_after_
strbuf_reset(buf);
strbuf_addchars(arrow, '-', len + 1);
} else if (add_after_entry) {
- strbuf_rtrim(buf);
- strbuf_addchars(arrow, ' ', 1);
- strbuf_addchars(arrow, '-', len - strlen(buf->buf) + 1);
+ if (!reversed) {
+ strbuf_rtrim(buf);
+ strbuf_addchars(arrow, ' ', 1);
+ strbuf_addchars(arrow, '-', len - strlen(buf->buf) + 1);
+ } else {
+ strbuf_rtrim(buf);
+ strbuf_addchars(arrow, ' ', 1);
+ strbuf_addchars(arrow, '<', 1);
+ strbuf_addchars(arrow, '-', len - strlen(buf->buf) - 3);
+ }
} else if (!add_after_entry) {
- strbuf_ltrim(buf);
- strbuf_addchars(arrow, '-', len - strlen(buf->buf) - 3);
- strbuf_addchars(arrow, '>', 1);
- strbuf_addchars(arrow, ' ', 1);
+ if (!reversed) {
+ strbuf_ltrim(buf);
+ strbuf_addchars(arrow, '-', len - strlen(buf->buf) - 3);
+ strbuf_addchars(arrow, '>', 1);
+ strbuf_addchars(arrow, ' ', 1);
+ } else {
+ strbuf_ltrim(buf);
+ strbuf_addchars(arrow, '-', len - strlen(buf->buf) + 1);
+ strbuf_addchars(arrow, ' ', 1);
+ }
}
}
+static void build_arrow(struct strbuf *buf, struct strbuf* arrow, int add_after_entry) {
+ build_arrow_(buf, arrow, add_after_entry, 0);
+}
+
+static void build_reversed_arrow(struct strbuf *buf, struct strbuf* arrow, int add_after_entry) {
+ build_arrow_(buf, arrow, add_after_entry, 1);
+}
+
static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct strbuf *arrow1, struct strbuf *arrow2, struct strbuf *arrow3, struct wt_status *s, int hide_pipe)
{
printf(_("|"));
@@ -180,6 +201,18 @@ void print_noob_status(struct wt_status *s, int advice)
}
}
+ for_each_string_list_item(item2, &s->restored) {
+ strbuf_reset(&buf_2);
+ strbuf_addstr(&buf_2, item2->string);
+ if (!strbuf_cmp(&buf_1, &buf_2)) {
+ build_table_entry(&table_col_entry_3, buf_1.buf, cols);
+ build_reversed_arrow(&table_col_entry_1, &arrow_1, 1);
+ build_reversed_arrow(&table_col_entry_2, &arrow_2, 0);
+ build_reversed_arrow(&table_col_entry_3, &arrow_3, 0);
+ is_arrow = 1;
+ }
+ }
+
if (!is_arrow)
print_table_body_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, &arrow_1, &arrow_2, &arrow_3, s, 0);
else
@@ -215,6 +248,17 @@ void print_noob_status(struct wt_status *s, int advice)
is_arrow = 1;
}
}
+
+ for_each_string_list_item(item2, &s->restored) {
+ strbuf_reset(&buf_2);
+ strbuf_addstr(&buf_2, item2->string);
+ if (!strbuf_cmp(&buf_1, &buf_2)) {
+ build_table_entry(&table_col_entry_3, buf_1.buf, cols);
+ build_reversed_arrow(&table_col_entry_2, &arrow_2, 1);
+ build_reversed_arrow(&table_col_entry_3, &arrow_3, 0);
+ is_arrow = 1;
+ }
+ }
} else if (d->index_status) {
build_table_entry(&table_col_entry_1, "", cols);
build_table_entry(&table_col_entry_2, "", cols);
diff --git a/wt-status.h b/wt-status.h
index 7b883fd476..c6bce8f74a 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -144,6 +144,7 @@ struct wt_status {
struct string_list untracked;
struct string_list ignored;
struct string_list added;
+ struct string_list restored;
uint32_t untracked_in_ms;
};
--
2.42.0.404.g2bcc23f3db
^ permalink raw reply related
* [RFC PATCH v2 6/6] status: add advice status hints as table footer
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>
---
builtin/commit.c | 1 +
table.c | 42 +++++++++++++++++++++++++++---------------
table.h | 2 +-
wt-status.c | 3 ++-
wt-status.h | 2 ++
5 files changed, 33 insertions(+), 17 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 3f816c117d..b97943e642 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1434,6 +1434,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
s.ignore_submodule_arg = ignore_submodule_arg;
s.status_format = status_format;
s.verbose = verbose;
+ s.is_cmd_status = 1;
if (no_renames != -1)
s.detect_rename = !no_renames;
if ((intptr_t)rename_score_arg != -1) {
diff --git a/table.c b/table.c
index 3602def17a..36719e3d09 100644
--- a/table.c
+++ b/table.c
@@ -6,6 +6,7 @@
#include "config.h"
#include "string-list.h"
#include "color.h"
+#include "advice.h"
#include "sys/ioctl.h"
static const char *color(int slot, struct wt_status *s)
@@ -132,7 +133,18 @@ static void print_table_body_line(struct strbuf *buf1, struct strbuf *buf2, stru
printf(_("|\n"));
}
-void print_noob_status(struct wt_status *s, int advice)
+static void print_table_hint_line(struct strbuf *buf1, struct strbuf *buf2, struct strbuf *buf3, struct wt_status *s)
+{
+ printf(_("|"));
+ color_fprintf(s->fp, color(WT_STATUS_HINT, s), "%s", buf1->buf);
+ printf(_("|"));
+ color_fprintf(s->fp, color(WT_STATUS_HINT, s), "%s", buf2->buf);
+ printf(_("|"));
+ color_fprintf(s->fp, color(WT_STATUS_HINT, s), "%s", buf3->buf);
+ printf(_("|\n"));
+}
+
+void print_noob_status(struct wt_status *s)
{
struct winsize w;
int cols;
@@ -163,20 +175,6 @@ void print_noob_status(struct wt_status *s, int advice)
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);
- if (advice) {
- build_table_entry(&table_col_entry_1, "(stage: git add <file>)", cols);
- build_table_entry(&table_col_entry_2, "(stage: git add <file>)", cols);
- build_table_entry(&table_col_entry_3, "(unstage: git restore --staged <file>)", cols);
-
- printf(_("|%s|%s|%s|\n"), table_col_entry_1.buf, table_col_entry_2.buf, table_col_entry_3.buf);
-
- build_table_entry(&table_col_entry_1, "", cols);
- build_table_entry(&table_col_entry_2, "(discard: git restore --staged <file>)", 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);
/* Draw table body */
@@ -282,6 +280,20 @@ void print_noob_status(struct wt_status *s, int advice)
}
printf(_("%s\n"), table_border.buf);
+
+ if (s->is_cmd_status && advice_enabled(ADVICE_STATUS_HINTS)) {
+ build_table_entry(&table_col_entry_1, "stage: git add ...", cols);
+ build_table_entry(&table_col_entry_2, "stage: git add ...", cols);
+ build_table_entry(&table_col_entry_3, "unstage: git restore --staged ...", cols);
+ print_table_hint_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s);
+
+ build_table_entry(&table_col_entry_1, "", cols);
+ build_table_entry(&table_col_entry_2, "discard: git restore --staged ...", cols);
+ build_table_entry(&table_col_entry_3, "", cols);
+ print_table_hint_line(&table_col_entry_1, &table_col_entry_2, &table_col_entry_3, s);
+ printf(_("%s\n"), table_border.buf);
+ }
+
strbuf_release(&table_border);
strbuf_release(&table_col_entry_1);
strbuf_release(&table_col_entry_2);
diff --git a/table.h b/table.h
index 5dff7162a4..c9e8c386de 100644
--- a/table.h
+++ b/table.h
@@ -1,6 +1,6 @@
#ifndef TABLE_H
#define TABLE_H
-void print_noob_status(struct wt_status *s, int i);
+void print_noob_status(struct wt_status *s);
#endif /* TABLE_H */
diff --git a/wt-status.c b/wt-status.c
index 1332d07dba..288817dcf7 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -50,6 +50,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
GIT_COLOR_RED, /* WT_STATUS_REMOTE_BRANCH */
GIT_COLOR_NIL, /* WT_STATUS_ONBRANCH */
GIT_COLOR_CYAN, /* WT_STATUS_ARROW */
+ GIT_COLOR_YELLOW, /* WT_STATUS_HINT */
};
static const char *color(int slot, struct wt_status *s)
@@ -2151,7 +2152,7 @@ static void wt_noobstatus_print(struct wt_status *s)
wt_longstatus_print_tracking(s);
}
- print_noob_status(s, 0);
+ print_noob_status(s);
}
static void wt_porcelain_print(struct wt_status *s)
diff --git a/wt-status.h b/wt-status.h
index c6bce8f74a..0a14b4b064 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -20,6 +20,7 @@ enum color_wt_status {
WT_STATUS_REMOTE_BRANCH,
WT_STATUS_ONBRANCH,
WT_STATUS_ARROW,
+ WT_STATUS_HINT,
WT_STATUS_MAXSLOT
};
@@ -146,6 +147,7 @@ struct wt_status {
struct string_list added;
struct string_list restored;
uint32_t untracked_in_ms;
+ int is_cmd_status;
};
size_t wt_status_locate_end(const char *s, size_t len);
--
2.42.0.404.g2bcc23f3db
^ permalink raw reply related
* [PATCH 0/2] pretty: add %aA to show domain-part of email addresses
From: Liam Beguin @ 2023-10-26 23:16 UTC (permalink / raw)
To: git; +Cc: Liam Beguin
Many reports use the email domain to keep track of organizations
contributing to projects.
Add support for formatting the domain-part of a contributor's address so
that this can be done using git itself, with something like:
git shortlog -sn --group=format:%aA v2.41.0..v2.42.0
---
Liam Beguin (2):
doc: pretty-formats: add missing word
pretty: add '%aA' to show domain-part of email addresses
Documentation/pretty-formats.txt | 10 ++++++++--
pretty.c | 13 ++++++++++++-
t/t4203-mailmap.sh | 28 ++++++++++++++++++++++++++++
t/t6006-rev-list-format.sh | 6 ++++--
4 files changed, 52 insertions(+), 5 deletions(-)
---
base-commit: 2e8e77cbac8ac17f94eee2087187fa1718e38b14
change-id: 20231025-pretty-email-domain-2eb2ae23f416
Best regards,
--
Liam Beguin <liambeguin@gmail.com>
^ permalink raw reply
* [PATCH 1/2] doc: pretty-formats: add missing word
From: Liam Beguin @ 2023-10-26 23:16 UTC (permalink / raw)
To: git; +Cc: Liam Beguin
In-Reply-To: <20231026-pretty-email-domain-v1-0-5d6bfa6615c0@gmail.com>
Follow %al and %cl and make sure to mention it's the 'email' local-part.
Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
Documentation/pretty-formats.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index d38b4ab5666c..a22f6fceecdd 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -193,7 +193,7 @@ The placeholders are:
'%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]
or linkgit:git-blame[1])
'%al':: author email local-part (the part before the '@' sign)
-'%aL':: author local-part (see '%al') respecting .mailmap, see
+'%aL':: author email local-part (see '%al') respecting .mailmap, see
linkgit:git-shortlog[1] or linkgit:git-blame[1])
'%ad':: author date (format respects --date= option)
'%aD':: author date, RFC2822 style
@@ -211,7 +211,7 @@ The placeholders are:
'%cE':: committer email (respecting .mailmap, see
linkgit:git-shortlog[1] or linkgit:git-blame[1])
'%cl':: committer email local-part (the part before the '@' sign)
-'%cL':: committer local-part (see '%cl') respecting .mailmap, see
+'%cL':: committer email local-part (see '%cl') respecting .mailmap, see
linkgit:git-shortlog[1] or linkgit:git-blame[1])
'%cd':: committer date (format respects --date= option)
'%cD':: committer date, RFC2822 style
--
2.39.0
^ permalink raw reply related
* [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Liam Beguin @ 2023-10-26 23:16 UTC (permalink / raw)
To: git; +Cc: Liam Beguin
In-Reply-To: <20231026-pretty-email-domain-v1-0-5d6bfa6615c0@gmail.com>
Many reports use the email domain to keep track of organizations
contributing to projects.
Add support for formatting the domain-part of a contributor's address so
that this can be done using git itself, with something like:
git shortlog -sn --group=format:%aA v2.41.0..v2.42.0
Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
Documentation/pretty-formats.txt | 6 ++++++
pretty.c | 13 ++++++++++++-
t/t4203-mailmap.sh | 28 ++++++++++++++++++++++++++++
t/t6006-rev-list-format.sh | 6 ++++--
4 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index a22f6fceecdd..72102a681c3a 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -195,6 +195,9 @@ The placeholders are:
'%al':: author email local-part (the part before the '@' sign)
'%aL':: author email local-part (see '%al') respecting .mailmap, see
linkgit:git-shortlog[1] or linkgit:git-blame[1])
+'%aa':: author email domain-part (the part after the '@' sign)
+'%aA':: author email domain-part (see '%al') respecting .mailmap, see
+ linkgit:git-shortlog[1] or linkgit:git-blame[1])
'%ad':: author date (format respects --date= option)
'%aD':: author date, RFC2822 style
'%ar':: author date, relative
@@ -213,6 +216,9 @@ The placeholders are:
'%cl':: committer email local-part (the part before the '@' sign)
'%cL':: committer email local-part (see '%cl') respecting .mailmap, see
linkgit:git-shortlog[1] or linkgit:git-blame[1])
+'%ca':: committer email domain-part (the part before the '@' sign)
+'%cA':: committer email domain-part (see '%cl') respecting .mailmap, see
+ linkgit:git-shortlog[1] or linkgit:git-blame[1])
'%cd':: committer date (format respects --date= option)
'%cD':: committer date, RFC2822 style
'%cr':: committer date, relative
diff --git a/pretty.c b/pretty.c
index cf964b060cd1..4f5d081589ea 100644
--- a/pretty.c
+++ b/pretty.c
@@ -791,7 +791,7 @@ static size_t format_person_part(struct strbuf *sb, char part,
mail = s.mail_begin;
maillen = s.mail_end - s.mail_begin;
- if (part == 'N' || part == 'E' || part == 'L') /* mailmap lookup */
+ if (part == 'N' || part == 'E' || part == 'L' || part == 'A') /* mailmap lookup */
mailmap_name(&mail, &maillen, &name, &namelen);
if (part == 'n' || part == 'N') { /* name */
strbuf_add(sb, name, namelen);
@@ -808,6 +808,17 @@ static size_t format_person_part(struct strbuf *sb, char part,
strbuf_add(sb, mail, maillen);
return placeholder_len;
}
+ if (part == 'a' || part == 'A') { /* domain-part */
+ const char *at = memchr(mail, '@', maillen);
+ if (at) {
+ at += 1;
+ maillen -= at - mail;
+ strbuf_add(sb, at, maillen);
+ } else {
+ strbuf_add(sb, mail, maillen);
+ }
+ return placeholder_len;
+ }
if (!s.date_begin)
goto skip;
diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
index 2016132f5161..35bf7bb05bea 100755
--- a/t/t4203-mailmap.sh
+++ b/t/t4203-mailmap.sh
@@ -624,6 +624,34 @@ test_expect_success 'Log output (local-part email address)' '
test_cmp expect actual
'
+test_expect_success 'Log output (domain-part email address)' '
+ cat >expect <<-EOF &&
+ Author email cto@coompany.xx has domain-part coompany.xx
+ Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
+
+ Author email me@company.xx has domain-part company.xx
+ Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
+
+ Author email me@company.xx has domain-part company.xx
+ Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
+
+ Author email nick2@company.xx has domain-part company.xx
+ Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
+
+ Author email bugs@company.xx has domain-part company.xx
+ Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
+
+ Author email bugs@company.xx has domain-part company.xx
+ Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
+
+ Author email author@example.com has domain-part example.com
+ Committer email $GIT_COMMITTER_EMAIL has domain-part $TEST_COMMITTER_DOMAIN
+ EOF
+
+ git log --pretty=format:"Author email %ae has domain-part %aa%nCommitter email %ce has domain-part %ca%n" >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'Log output with --use-mailmap' '
test_config mailmap.file complex.map &&
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 573eb97a0f7f..34c686becf2d 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -163,11 +163,12 @@ commit $head1
EOF
# we don't test relative here
-test_format author %an%n%ae%n%al%n%ad%n%aD%n%at <<EOF
+test_format author %an%n%ae%n%al%aa%n%ad%n%aD%n%at <<EOF
commit $head2
$GIT_AUTHOR_NAME
$GIT_AUTHOR_EMAIL
$TEST_AUTHOR_LOCALNAME
+$TEST_AUTHOR_DOMAIN
Thu Apr 7 15:13:13 2005 -0700
Thu, 7 Apr 2005 15:13:13 -0700
1112911993
@@ -180,11 +181,12 @@ Thu, 7 Apr 2005 15:13:13 -0700
1112911993
EOF
-test_format committer %cn%n%ce%n%cl%n%cd%n%cD%n%ct <<EOF
+test_format committer %cn%n%ce%n%cl%ca%n%cd%n%cD%n%ct <<EOF
commit $head2
$GIT_COMMITTER_NAME
$GIT_COMMITTER_EMAIL
$TEST_COMMITTER_LOCALNAME
+$TEST_COMMITTER_DOMAIN
Thu Apr 7 15:13:13 2005 -0700
Thu, 7 Apr 2005 15:13:13 -0700
1112911993
--
2.39.0
^ permalink raw reply related
* Re: ls-remote bug
From: Bagas Sanjaya @ 2023-10-27 1:08 UTC (permalink / raw)
To: Lior Zeltzer, Git Mailing List
Cc: Andrzej Hunt, Ævar Arnfjörð Bjarmason,
Junio C Hamano
In-Reply-To: <BL0PR18MB2130A3CA5DEF0DD7199F2979BADFA@BL0PR18MB2130.namprd18.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 2395 bytes --]
On Tue, Oct 24, 2023 at 10:55:24AM +0000, Lior Zeltzer wrote:
>
> >uname -a
> Linux dc3lp-veld0045 3.10.0-1160.21.1.el7.x86_64 #1 SMP Tue Mar 16 18:28:22 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
>
> Gerrit version :
> 3.8.0
>
> Bug description :
> When running ls-remote : sometime data gets cut in the middle
>
> Reproducing :
> You need a few files with a few repo names (I used 4 files with 10 repos each)
> Call then l1..l4
> And the code below just cd into each of them does ls-remote twice and compares the data
> Doing it in parallel on all lists.
> Data received in both ls-remotes should be the same , if not, it prints ***
> Repos should contain a lot of tags and refs
What repo did you find this regression? Did you mean linux.git (Linux kernel)?
>
> Note :
> 1. without stderr redirection (2>&1) all works well
> 2. On local repos (not through gerrit) all works well
>
> I compared various git vers and found the bug to be between 2.31.8 and 2.32.0
> Comparing ls-remote.c file between those vers gave me :
>
> Lines :
> if (transport_disconnect(transport))
> return 1;
>
> moved to end of sub
>
> copying ls-remote.c from 2.31.8 to 2.32.0 - fixed the bug
>
>
>
> Code reproducing bug :
>
> #!/proj/mislcad/areas/DAtools/tools/perl/5.10.1/bin/perl -w
> use strict;
> use Cwd qw(cwd);
>
> my $count = 4;
> for my $f (1..$count) {
> my $child = fork();
> if (!$child) {
> my $curr = cwd();
>
> my @repos = `cat l$f`;
> foreach my $repo (@repos) {
> chomp $repo;
> print "$repo\n";
> chdir($repo);
> my $remote_tags_str = `git ls-remote 2>&1`;
> my $remote_tags_str2 = `git ls-remote 2>&1 `;
> chdir($curr);
> if ( $remote_tags_str ne $remote_tags_str2) {
> print "***\n";
> }
> }
>
> exit(0);
> }
> }
> while (wait != -1) {}
> 1;
>
I tried reproducing this regression by:
```
$ cd /path/to/git.git
$ git ls-remote 2>&1 > /tmp/root.list
$ cd builtin/
$ git ls-remote 2>&1 > /tmp/builtin.list
$ cd ../
$ git diff --no-index /tmp/root.list /tmp/builtin.list
```
And indeed, the diff was empty (which meant that both listings are same).
Confused...
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 5/9] t1450: convert tests to remove worktrees via git-worktree(1)
From: Eric Sunshine @ 2023-10-27 2:42 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys, Taylor Blau, Junio C Hamano
In-Reply-To: <089565a358eb28544f0ad6b83b8c47e1edf2db6f.1698156169.git.ps@pks.im>
On Tue, Oct 24, 2023 at 10:05 AM Patrick Steinhardt <ps@pks.im> wrote:
> Some of our tests in t1450 create worktrees and then corrupt them.
> As it is impossible to delete such worktrees via a normal call to `git
> worktree remove`, we instead opt to remove them manually by calling
> rm(1) instead.
>
> This is ultimately unnecessary though as we can use the `-f` switch to
> remove the worktree. Let's convert the tests to do so such that we don't
> have to reach into internal implementation details of worktrees.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> @@ -141,7 +141,7 @@ test_expect_success 'HEAD link pointing at a funny place' '
> test_expect_success 'HEAD link pointing at a funny object (from different wt)' '
> test_when_finished "git update-ref HEAD $orig_head" &&
> - test_when_finished "rm -rf .git/worktrees wt" &&
> + test_when_finished "git worktree remove -f wt" &&
> git worktree add wt &&
> echo $ZERO_OID >.git/HEAD &&
Technically, this is a change of behavior since the original code
removed the entire .git/worktrees directory, which deleted
administrative metainformation for _all_ worktrees, whereas the new
code only deletes administrative metadata for the mentioned worktree.
However, since there are no other existing worktrees at this point in
any of these tests, the result is functionally the same, so the change
of behavior is immaterial. Moreover, the revised code has a smaller
blast-radius, which may be a desirable property since there has been
some movement toward making tests more self-contained so that they can
be run individually more easily.
^ permalink raw reply
* Re: [PATCH v5 3/3] rev-list: add commit object support in `--missing` option
From: Patrick Steinhardt @ 2023-10-27 6:25 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster
In-Reply-To: <20231026101109.43110-4-karthik.188@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8377 bytes --]
On Thu, Oct 26, 2023 at 12:11:09PM +0200, Karthik Nayak wrote:
> The `--missing` object option in rev-list currently works only with
> missing blobs/trees. For missing commits the revision walker fails with
> a fatal error.
>
> Let's extend the functionality of `--missing` option to also support
> commit objects. This is done by adding a `missing_objects` field to
> `rev_info`. This field is an `oidset` to which we'll add the missing
> commits as we encounter them. The revision walker will now continue the
> traversal and call `show_commit()` even for missing commits. In rev-list
> we can then check if the commit is a missing commit and call the
> existing code for parsing `--missing` objects.
>
> A scenario where this option would be used is to find the boundary
> objects between different object directories. Consider a repository with
> a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate
> object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a
> repository, using the `--missing=print` option while disabling the
> alternate object directory allows us to find the boundary objects
> between the main and alternate object directory.
>
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> builtin/rev-list.c | 6 +++
> list-objects.c | 3 ++
> revision.c | 17 ++++++++-
> revision.h | 4 ++
> t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 102 insertions(+), 2 deletions(-)
> create mode 100755 t/t6022-rev-list-missing.sh
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 98542e8b3c..181353dcf5 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)
>
> display_progress(progress, ++progress_counter);
>
> + if (revs->do_not_die_on_missing_objects &&
> + oidset_contains(&revs->missing_commits, &commit->object.oid)) {
> + finish_object__ma(&commit->object);
> + return;
> + }
> +
> if (show_disk_usage)
> total_disk_usage += get_object_disk_usage(&commit->object);
>
> diff --git a/list-objects.c b/list-objects.c
> index 47296dff2f..f4e1104b56 100644
> --- a/list-objects.c
> +++ b/list-objects.c
> @@ -389,6 +389,9 @@ static void do_traverse(struct traversal_context *ctx)
> */
> if (!ctx->revs->tree_objects)
> ; /* do not bother loading tree */
> + else if (ctx->revs->do_not_die_on_missing_objects &&
> + oidset_contains(&ctx->revs->missing_commits, &commit->object.oid))
> + ;
> else if (repo_get_commit_tree(the_repository, commit)) {
> struct tree *tree = repo_get_commit_tree(the_repository,
> commit);
> diff --git a/revision.c b/revision.c
> index 219dc76716..738bacad08 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -6,6 +6,7 @@
> #include "object-name.h"
> #include "object-file.h"
> #include "object-store-ll.h"
> +#include "oidset.h"
> #include "tag.h"
> #include "blob.h"
> #include "tree.h"
> @@ -1112,6 +1113,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
>
> if (commit->object.flags & ADDED)
> return 0;
> + if (revs->do_not_die_on_missing_objects &&
> + oidset_contains(&revs->missing_commits, &commit->object.oid))
> + return 0;
> commit->object.flags |= ADDED;
>
> if (revs->include_check &&
> @@ -1168,7 +1172,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
> for (parent = commit->parents; parent; parent = parent->next) {
> struct commit *p = parent->item;
> int gently = revs->ignore_missing_links ||
> - revs->exclude_promisor_objects;
> + revs->exclude_promisor_objects ||
> + revs->do_not_die_on_missing_objects;
> if (repo_parse_commit_gently(revs->repo, p, gently) < 0) {
> if (revs->exclude_promisor_objects &&
> is_promisor_object(&p->object.oid)) {
> @@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
> break;
> continue;
> }
> - return -1;
> +
> + if (revs->do_not_die_on_missing_objects)
> + oidset_insert(&revs->missing_commits, &p->object.oid);
> + else
> + return -1; /* corrupt repository */
> }
> if (revs->sources) {
> char **slot = revision_sources_at(revs->sources, p);
> @@ -3109,6 +3118,7 @@ void release_revisions(struct rev_info *revs)
> clear_decoration(&revs->merge_simplification, free);
> clear_decoration(&revs->treesame, free);
> line_log_free(revs);
> + oidset_clear(&revs->missing_commits);
> }
>
> static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
> @@ -3800,6 +3810,9 @@ int prepare_revision_walk(struct rev_info *revs)
> FOR_EACH_OBJECT_PROMISOR_ONLY);
> }
>
> + if (revs->do_not_die_on_missing_objects)
> + oidset_init(&revs->missing_commits, 0);
> +
We unconditionally clear the oidset now, so shouldn't we also
unconditionally initialize it?
Patrick
> if (!revs->reflog_info)
> prepare_to_use_bloom_filter(revs);
> if (!revs->unsorted_input)
> diff --git a/revision.h b/revision.h
> index c73c92ef40..94c43138bc 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -4,6 +4,7 @@
> #include "commit.h"
> #include "grep.h"
> #include "notes.h"
> +#include "oidset.h"
> #include "pretty.h"
> #include "diff.h"
> #include "commit-slab-decl.h"
> @@ -373,6 +374,9 @@ struct rev_info {
>
> /* Location where temporary objects for remerge-diff are written. */
> struct tmp_objdir *remerge_objdir;
> +
> + /* Missing commits to be tracked without failing traversal. */
> + struct oidset missing_commits;
> };
>
> /**
> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> new file mode 100755
> index 0000000000..40265a4f66
> --- /dev/null
> +++ b/t/t6022-rev-list-missing.sh
> @@ -0,0 +1,74 @@
> +#!/bin/sh
> +
> +test_description='handling of missing objects in rev-list'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
> +. ./test-lib.sh
> +
> +# We setup the repository with two commits, this way HEAD is always
> +# available and we can hide commit 1.
> +test_expect_success 'create repository and alternate directory' '
> + test_commit 1 &&
> + test_commit 2 &&
> + test_commit 3
> +'
> +
> +for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> +do
> + test_expect_success "rev-list --missing=error fails with missing object $obj" '
> + oid="$(git rev-parse $obj)" &&
> + path=".git/objects/$(test_oid_to_path $oid)" &&
> +
> + mv "$path" "$path.hidden" &&
> + test_when_finished "mv $path.hidden $path" &&
> +
> + test_must_fail git rev-list --missing=error --objects \
> + --no-object-names HEAD
> + '
> +done
> +
> +for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> +do
> + for action in "allow-any" "print"
> + do
> + test_expect_success "rev-list --missing=$action with missing $obj" '
> + oid="$(git rev-parse $obj)" &&
> + path=".git/objects/$(test_oid_to_path $oid)" &&
> +
> + # Before the object is made missing, we use rev-list to
> + # get the expected oids.
> + git rev-list --objects --no-object-names \
> + HEAD ^$obj >expect.raw &&
> +
> + # Blobs are shared by all commits, so evethough a commit/tree
> + # might be skipped, its blob must be accounted for.
> + if [ $obj != "HEAD:1.t" ]; then
> + echo $(git rev-parse HEAD:1.t) >>expect.raw &&
> + echo $(git rev-parse HEAD:2.t) >>expect.raw
> + fi &&
> +
> + mv "$path" "$path.hidden" &&
> + test_when_finished "mv $path.hidden $path" &&
> +
> + git rev-list --missing=$action --objects --no-object-names \
> + HEAD >actual.raw &&
> +
> + # When the action is to print, we should also add the missing
> + # oid to the expect list.
> + case $action in
> + allow-any)
> + ;;
> + print)
> + grep ?$oid actual.raw &&
> + echo ?$oid >>expect.raw
> + ;;
> + esac &&
> +
> + sort actual.raw >actual &&
> + sort expect.raw >expect &&
> + test_cmp expect actual
> + '
> + done
> +done
> +
> +test_done
> --
> 2.42.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed
From: Jacob Stopak @ 2023-10-27 6:34 UTC (permalink / raw)
To: Emily Shaffer; +Cc: git
In-Reply-To: <ZTrX4PMYtbVT-tUu@google.com>
> > +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....
So the funny thing is that the existing behavior of the diagnostics file
just overwrites any existing diagnostics file with the same name, which
is at odds with how the bugreport throws an error in the case of a
conflict. I wasn't sure whether it made sense to touch that here since it
seemed possibly out of the scope of this change, given that there is
a separate "git diagnose" command to take into account.
But if we keep this behavior it basically means there's no need to test
the necessary diagnostics output paths when determining the path of the
bugreport itself, since whatever we pick for the bugreport will just
overwrite anything for the diagnotics zip if it already exists. The one
caveat is that any future files created should behave the same way...
But I get that this is perhaps inconsistent and it might be worth it to
make "git diagnose" work the same way as bugreport so it makes more sense
to do the kind of validations you mentioned.
> 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).
Hmm... so when you say "create_file_with_optional_suffix", do you mean
a function that tests a set of paths to find the minimum incremented
suffix that will work for all the paths? Would it use the current method
the patch uses of trying to create the files and if creation fails we
know the file exists -> increment -> try again? Repeat until all the
files are able to be created and make sure to clean up any extras?
(And maybe just clean up everything since the actual files with content
will be created later on, now that the suffix is known).
An alternative could be to wrap `build_path()` in a function that does
this work, locally scope `i` in it, call build_path on each needed path,
and test creation until all paths are valid, clean up all the paths,
then return the suffix.
> > + 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.
Haha - it's interesting how we each interpreted the user's experience here,
and I was a bit torn about this too. But here are my thoughts on it:
If the user specifies their own suffix, it seems more natural and even
expected for them to just get an error if a file with that name already
exists. To me it's not that they put their foot in anything, it's that
they asked for a specific thing that we can't deliver since it's already
taken, so just let them know so they can either delete the existing one
or alter the custom suffix. Incremeting the filename without telling them
in this case might not be what they actually want, we're kindof guessing.
However, in the default case where no suffix is specified, the user likely
has no assumption or awareness about the suffix at all, which is why it
felt odd to throw an error out of the blue if the default suffix happens
to conflict with an existing file that was also created by default. The
user didn't ask for anything specific in this case, so would likely be
confused as to why they are getting an error when it just worked a second
ago. So I was thinking we can just handle it gracefully and give them the
incremented report.
> (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.)
Setting up the default variables like this was discussed, but not the
difference in behavior based on whether the user specifies a suffix.
Altho I do agree that we sacrifice some consistency here and it does add
an extra pathway to the code, imo there is a good enough reason to do it
as described above - to me it makes things more intuitive to the user.
> > + /* 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.
I like yours better too :)
> > + 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".
>
Yeah these are great points and I completely agree! Even if we stick with
this method of doing things, the `i` decrement should be moved out of the
conditional and up closer to where the value of `i` is set.
> > + 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.
Sure I can add some docs to the manpage, altho as described above I
prefer throwing an error instead of adding the increment to the
user-provided suffixes :D
> Thanks for your effort on the patch so far and again, sorry for the late
> reply.
> - Emily
No worries at all and thanks a lot for your review! I'll start working
on a v3 - it would be great to get your thoughts on the unresolved items.
^ permalink raw reply
* GFW fails with ST3 on Windows 11
From: John @ 2023-10-27 7:31 UTC (permalink / raw)
To: git
I have been using Sublime Text 3 as the editor on Git for Windows for years, on Windows 10. I recently purchased a Windows 11 machine. On that machine, when I give GFW the following command, I get the response shown:
$ git commit -a
[…]
hint: Waiting for your editor to close the file… C:\Program Files\Sublime Text 3\sublime_text.exe: C:Program: command not found
error: There was a problem with the editor ‘C:\Program Files\Sublime Text 3\sublime_text.exe’.
Please supply the message using either -m or -F option.
Does anyone know of a fix? To repeat, it’s only a problem on Windows 11.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox