| Message ID | pull.1302.v2.git.1659132518935.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] git-prompt: show presence of unresolved conflicts at command prompt | expand |
On Fri, Jul 29, 2022 at 6:08 PM Justin Donnelly via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Justin Donnelly <justinrdonnelly@gmail.com> > > If GIT_PS1_SHOWCONFLICTSTATE is set to "yes", show the word "CONFLICT" > on the command prompt when there are unresolved conflicts. > > Example prompt: (main|CONFLICT) > > Signed-off-by: Justin Donnelly <justinrdonnelly@gmail.com> > --- > Show 'CONFLICT' indicator at command prompt > > This patch adds functionality for bash/zsh to show "CONFLICT" on the > prompt in cases where there are unresolved conflicts. The feature is > only enabled after setting an environment variable. > > The conflict state is determined by running git ls-files --unmerged. In > my testing, the performance was very good. It took around 0.01 seconds > to run git ls-files --unmerged regardless of the number of conflicts, or > their depth, even on very large projects (Linux kernel). However, if > anybody has any concerns with this, I'm open to other options. > > Any tests that were impacted (with the conflict prompt feature enabled) > were duplicated. The original test was left as-is (no changes, and > conflict prompt feature disabled). The new version of each test enables > the conflict prompt feature and confirms the prompt includes > "|CONFLICT". > > ------------------------------------------------------------------------ > > Changes since v1: > > * This feature is now disabled by default. > * Created new tests for conflict state (instead of modifying existing > tests). > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1302%2Fjustinrdonnelly%2Fconflict-indicator-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1302/justinrdonnelly/conflict-indicator-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/1302 > > Range-diff vs v1: > > 1: e380826dcaf ! 1: 7154d695426 git-prompt: show 'CONFLICT' indicator at command prompt > @@ Metadata > Author: Justin Donnelly <justinrdonnelly@gmail.com> > > ## Commit message ## > - git-prompt: show 'CONFLICT' indicator at command prompt > + git-prompt: show presence of unresolved conflicts at command prompt > > - When there are unresolved conflicts, show the word 'CONFLICT' on the > - command prompt. Similar to other state indicators, this provides > - information to the user about the current state of the repository. > + If GIT_PS1_SHOWCONFLICTSTATE is set to "yes", show the word "CONFLICT" > + on the command prompt when there are unresolved conflicts. > > Example prompt: (main|CONFLICT) > > @@ contrib/completion/git-prompt.sh > # single '?' character by setting GIT_PS1_COMPRESSSPARSESTATE, or omitted > # by setting GIT_PS1_OMITSPARSESTATE. > # > -+# When there is a conflict, the prompt will include "|CONFLICT". This can > -+# be omitted by setting GIT_PS1_OMITCONFLICTSTATE. > ++# If you would like to see a notification on the prompt when there are > ++# unresolved conflicts, set GIT_PS1_SHOWCONFLICTSTATE to "yes". The > ++# prompt will include "|CONFLICT". > +# > # If you would like to see more information about the identity of > # commits checked out as a detached HEAD, set GIT_PS1_DESCRIBE_STYLE > @@ contrib/completion/git-prompt.sh: __git_ps1 () > fi > > + local conflict="" # state indicator for unresolved conflicts > -+ if [[ -z "${GIT_PS1_OMITCONFLICTSTATE-}" ]] && > ++ if [[ "${GIT_PS1_SHOWCONFLICTSTATE}" == "yes" ]] && > + [[ $(git ls-files --unmerged 2>/dev/null) ]]; then > + conflict="|CONFLICT" > + fi > @@ contrib/completion/git-prompt.sh: __git_ps1 () > if [ "${__git_printf_supports_v-}" != yes ]; then > > ## t/t9903-bash-prompt.sh ## > -@@ t/t9903-bash-prompt.sh: test_expect_success 'prompt - interactive rebase' ' > - ' > - > - test_expect_success 'prompt - rebase merge' ' > -- printf " (b2|REBASE 1/3)" >expected && > -+ printf " (b2|REBASE 1/3|CONFLICT)" >expected && > - git checkout b2 && > +@@ t/t9903-bash-prompt.sh: test_expect_success 'prompt - rebase merge' ' > test_when_finished "git checkout main" && > test_must_fail git rebase --merge b1 b2 && > -@@ t/t9903-bash-prompt.sh: test_expect_success 'prompt - rebase merge' ' > + test_when_finished "git rebase --abort" && > +- __git_ps1 >"$actual" && > ++ ( > ++ sane_unset GIT_PS1_SHOWCONFLICTSTATE && > ++ __git_ps1 >"$actual" > ++ ) && > ++ test_cmp expected "$actual" > ++' > ++ > ++test_expect_success 'prompt - rebase merge conflict' ' > ++ printf " (b2|REBASE 1/3|CONFLICT)" >expected && > ++ git checkout b2 && > ++ test_when_finished "git checkout main" && > ++ test_must_fail git rebase --merge b1 b2 && > ++ test_when_finished "git rebase --abort" && > ++ ( > ++ GIT_PS1_SHOWCONFLICTSTATE="yes" && > ++ __git_ps1 >"$actual" > ++ ) && > + test_cmp expected "$actual" > ' > > - test_expect_success 'prompt - rebase am' ' > -- printf " (b2|REBASE 1/3)" >expected && > -+ printf " (b2|REBASE 1/3|CONFLICT)" >expected && > - git checkout b2 && > +@@ t/t9903-bash-prompt.sh: test_expect_success 'prompt - rebase am' ' > test_when_finished "git checkout main" && > test_must_fail git rebase --apply b1 b2 && > -@@ t/t9903-bash-prompt.sh: test_expect_success 'prompt - rebase am' ' > + test_when_finished "git rebase --abort" && > +- __git_ps1 >"$actual" && > ++ ( > ++ sane_unset GIT_PS1_SHOWCONFLICTSTATE && > ++ __git_ps1 >"$actual" > ++ ) && > ++ test_cmp expected "$actual" > ++' > ++ > ++test_expect_success 'prompt - rebase am conflict' ' > ++ printf " (b2|REBASE 1/3|CONFLICT)" >expected && > ++ git checkout b2 && > ++ test_when_finished "git checkout main" && > ++ test_must_fail git rebase --apply b1 b2 && > ++ test_when_finished "git rebase --abort" && > ++ ( > ++ GIT_PS1_SHOWCONFLICTSTATE="yes" && > ++ __git_ps1 >"$actual" > ++ ) && > + test_cmp expected "$actual" > ' > > - test_expect_success 'prompt - merge' ' > -- printf " (b1|MERGING)" >expected && > -+ printf " (b1|MERGING|CONFLICT)" >expected && > - git checkout b1 && > +@@ t/t9903-bash-prompt.sh: test_expect_success 'prompt - merge' ' > test_when_finished "git checkout main" && > test_must_fail git merge b2 && > -@@ t/t9903-bash-prompt.sh: test_expect_success 'prompt - merge' ' > + test_when_finished "git reset --hard" && > +- __git_ps1 >"$actual" && > ++ ( > ++ sane_unset GIT_PS1_SHOWCONFLICTSTATE && > ++ __git_ps1 >"$actual" > ++ ) && > ++ test_cmp expected "$actual" > ++' > ++ > ++test_expect_success 'prompt - merge conflict' ' > ++ printf " (b1|MERGING|CONFLICT)" >expected && > ++ git checkout b1 && > ++ test_when_finished "git checkout main" && > ++ test_must_fail git merge b2 && > ++ test_when_finished "git reset --hard" && > ++ ( > ++ GIT_PS1_SHOWCONFLICTSTATE="yes" && > ++ __git_ps1 >"$actual" > ++ ) && > + test_cmp expected "$actual" > ' > > - test_expect_success 'prompt - cherry-pick' ' > -- printf " (main|CHERRY-PICKING)" >expected && > -+ printf " (main|CHERRY-PICKING|CONFLICT)" >expected && > +@@ t/t9903-bash-prompt.sh: test_expect_success 'prompt - cherry-pick' ' > + printf " (main|CHERRY-PICKING)" >expected && > test_must_fail git cherry-pick b1 b1^ && > test_when_finished "git cherry-pick --abort" && > - __git_ps1 >"$actual" && > +- __git_ps1 >"$actual" && > ++ ( > ++ sane_unset GIT_PS1_SHOWCONFLICTSTATE && > ++ __git_ps1 >"$actual" > ++ ) && > test_cmp expected "$actual" && > -+ printf " (main|CHERRY-PICKING)" >expected && > git reset --merge && > test_must_fail git rev-parse CHERRY_PICK_HEAD && > - __git_ps1 >"$actual" && > +@@ t/t9903-bash-prompt.sh: test_expect_success 'prompt - cherry-pick' ' > + test_cmp expected "$actual" > + ' > + > ++test_expect_success 'prompt - cherry-pick conflict' ' > ++ printf " (main|CHERRY-PICKING|CONFLICT)" >expected && > ++ test_must_fail git cherry-pick b1 b1^ && > ++ test_when_finished "git cherry-pick --abort" && > ++ ( > ++ GIT_PS1_SHOWCONFLICTSTATE="yes" && > ++ __git_ps1 >"$actual" > ++ ) && > ++ test_cmp expected "$actual" > ++' > ++ > + test_expect_success 'prompt - revert' ' > + printf " (main|REVERTING)" >expected && > + test_must_fail git revert b1^ b1 && > > > contrib/completion/git-prompt.sh | 12 +++++- > t/t9903-bash-prompt.sh | 70 ++++++++++++++++++++++++++++++-- > 2 files changed, 77 insertions(+), 5 deletions(-) > > diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh > index 1435548e004..57972c2845c 100644 > --- a/contrib/completion/git-prompt.sh > +++ b/contrib/completion/git-prompt.sh > @@ -84,6 +84,10 @@ > # single '?' character by setting GIT_PS1_COMPRESSSPARSESTATE, or omitted > # by setting GIT_PS1_OMITSPARSESTATE. > # > +# If you would like to see a notification on the prompt when there are > +# unresolved conflicts, set GIT_PS1_SHOWCONFLICTSTATE to "yes". The > +# prompt will include "|CONFLICT". > +# > # If you would like to see more information about the identity of > # commits checked out as a detached HEAD, set GIT_PS1_DESCRIBE_STYLE > # to one of these values: > @@ -508,6 +512,12 @@ __git_ps1 () > r="$r $step/$total" > fi > > + local conflict="" # state indicator for unresolved conflicts > + if [[ "${GIT_PS1_SHOWCONFLICTSTATE}" == "yes" ]] && > + [[ $(git ls-files --unmerged 2>/dev/null) ]]; then > + conflict="|CONFLICT" > + fi > + > local w="" > local i="" > local s="" > @@ -572,7 +582,7 @@ __git_ps1 () > fi > > local f="$h$w$i$s$u$p" > - local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}" > + local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}${conflict}" > > if [ $pcmode = yes ]; then > if [ "${__git_printf_supports_v-}" != yes ]; then > diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh > index 6a30f5719c3..47eb98893ef 100755 > --- a/t/t9903-bash-prompt.sh > +++ b/t/t9903-bash-prompt.sh > @@ -188,7 +188,23 @@ test_expect_success 'prompt - rebase merge' ' > test_when_finished "git checkout main" && > test_must_fail git rebase --merge b1 b2 && > test_when_finished "git rebase --abort" && > - __git_ps1 >"$actual" && > + ( > + sane_unset GIT_PS1_SHOWCONFLICTSTATE && > + __git_ps1 >"$actual" > + ) && > + test_cmp expected "$actual" > +' > + > +test_expect_success 'prompt - rebase merge conflict' ' > + printf " (b2|REBASE 1/3|CONFLICT)" >expected && > + git checkout b2 && > + test_when_finished "git checkout main" && > + test_must_fail git rebase --merge b1 b2 && > + test_when_finished "git rebase --abort" && > + ( > + GIT_PS1_SHOWCONFLICTSTATE="yes" && > + __git_ps1 >"$actual" > + ) && > test_cmp expected "$actual" > ' > > @@ -198,7 +214,23 @@ test_expect_success 'prompt - rebase am' ' > test_when_finished "git checkout main" && > test_must_fail git rebase --apply b1 b2 && > test_when_finished "git rebase --abort" && > - __git_ps1 >"$actual" && > + ( > + sane_unset GIT_PS1_SHOWCONFLICTSTATE && > + __git_ps1 >"$actual" > + ) && > + test_cmp expected "$actual" > +' > + > +test_expect_success 'prompt - rebase am conflict' ' > + printf " (b2|REBASE 1/3|CONFLICT)" >expected && > + git checkout b2 && > + test_when_finished "git checkout main" && > + test_must_fail git rebase --apply b1 b2 && > + test_when_finished "git rebase --abort" && > + ( > + GIT_PS1_SHOWCONFLICTSTATE="yes" && > + __git_ps1 >"$actual" > + ) && > test_cmp expected "$actual" > ' > > @@ -208,7 +240,23 @@ test_expect_success 'prompt - merge' ' > test_when_finished "git checkout main" && > test_must_fail git merge b2 && > test_when_finished "git reset --hard" && > - __git_ps1 >"$actual" && > + ( > + sane_unset GIT_PS1_SHOWCONFLICTSTATE && > + __git_ps1 >"$actual" > + ) && > + test_cmp expected "$actual" > +' > + > +test_expect_success 'prompt - merge conflict' ' > + printf " (b1|MERGING|CONFLICT)" >expected && > + git checkout b1 && > + test_when_finished "git checkout main" && > + test_must_fail git merge b2 && > + test_when_finished "git reset --hard" && > + ( > + GIT_PS1_SHOWCONFLICTSTATE="yes" && > + __git_ps1 >"$actual" > + ) && > test_cmp expected "$actual" > ' > > @@ -216,7 +264,10 @@ test_expect_success 'prompt - cherry-pick' ' > printf " (main|CHERRY-PICKING)" >expected && > test_must_fail git cherry-pick b1 b1^ && > test_when_finished "git cherry-pick --abort" && > - __git_ps1 >"$actual" && > + ( > + sane_unset GIT_PS1_SHOWCONFLICTSTATE && > + __git_ps1 >"$actual" > + ) && > test_cmp expected "$actual" && > git reset --merge && > test_must_fail git rev-parse CHERRY_PICK_HEAD && > @@ -224,6 +275,17 @@ test_expect_success 'prompt - cherry-pick' ' > test_cmp expected "$actual" > ' > > +test_expect_success 'prompt - cherry-pick conflict' ' > + printf " (main|CHERRY-PICKING|CONFLICT)" >expected && > + test_must_fail git cherry-pick b1 b1^ && > + test_when_finished "git cherry-pick --abort" && > + ( > + GIT_PS1_SHOWCONFLICTSTATE="yes" && > + __git_ps1 >"$actual" > + ) && > + test_cmp expected "$actual" > +' > + > test_expect_success 'prompt - revert' ' > printf " (main|REVERTING)" >expected && > test_must_fail git revert b1^ b1 && > > base-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25 > -- > gitgitgadget [SubmittingPatches](https://github.com/git/git/blob/master/Documentation/SubmittingPatches) recommends using `git-contacts` to determine who to "cc" on the email. I used GitGitGadget, which doesn't cc anybody by default (I see now that others manually add cc's to the PR description). So I've added the following cc's to this email: Ævar Arnfjörð Bjarmason Junio C Hamano Elijah Newren Phillip Wood Johannes Schindelin I hope this is against protocol/etiquette, but after some initial feedback from Junio, I haven't gotten any more. I wasn't sure if nobody had seen the patch, or if there just wasn't any interest. Thanks, Justin
Justin Donnelly <justinrdonnelly@gmail.com> writes: > I hope this is against protocol/etiquette, but after some initial > feedback from Junio, I haven't gotten any more. I wasn't sure if > nobody had seen the patch, or if there just wasn't any interest. It probably is a bit of both. I personally did not see much point in adding the long "conflicts" marker to the shell prompt (I did worry about possible complaints by end users triggered by seeing them suddenly without asking, which was why I commented on the patch) and I was waiting for interested folks to speak out. I do not know about other folks if they did see and did not find it interesting, they are not looking at others' work, or your second round came on a particularly bad day (busy with other topics, or weekend just before the list was swamped with many new topics at the beginning of a new week, or something). Pinging on the thread like you did would be the right thing to do to "kick" those, who did see and who were in favor but who kept silent, into action ;-)
Hi, On Sun, 14 Aug 2022, Junio C Hamano wrote: > Justin Donnelly <justinrdonnelly@gmail.com> writes: > > > I hope this is against protocol/etiquette, but after some initial > > feedback from Junio, I haven't gotten any more. I wasn't sure if > > nobody had seen the patch, or if there just wasn't any interest. > > It probably is a bit of both. I personally did not see much point > in adding the long "conflicts" marker to the shell prompt (I did > worry about possible complaints by end users triggered by seeing > them suddenly without asking, which was why I commented on the > patch) and I was waiting for interested folks to speak out. Speaking for myself, I was too busy elsewhere. But now that I looked over the patch, I think it is fine. My only feedback is that it would be wise to only add a single test case because that is plenty enough (after all, it validates the `ls-files --unmerged` call and not the `cherry-pick` code) and it is unnecessary to waste the electricity on additional tests cases (even if somebody else foots the bill, it would do well for all of us to start being more mindful about energy consumption). Ciao, Dscho
Hi On 15/08/2022 05:22, Junio C Hamano wrote: > Justin Donnelly <justinrdonnelly@gmail.com> writes: > >> I hope this is against protocol/etiquette, but after some initial >> feedback from Junio, I haven't gotten any more. I wasn't sure if >> nobody had seen the patch, or if there just wasn't any interest. > > It probably is a bit of both. I personally did not see much point > in adding the long "conflicts" marker to the shell prompt (I did > worry about possible complaints by end users triggered by seeing > them suddenly without asking, which was why I commented on the > patch) and I was waiting for interested folks to speak out. > > I do not know about other folks if they did see and did not find it > interesting, they are not looking at others' work, or your second > round came on a particularly bad day (busy with other topics, or > weekend just before the list was swamped with many new topics at the > beginning of a new week, or something). Pinging on the thread like > you did would be the right thing to do to "kick" those, who did see > and who were in favor but who kept silent, into action ;-) I had not commented as I don't use the prompt. I have just had a quick read and I wonder if it would be more efficient to use git diff --cached --quiet --diff-filter=U rather than git ls-files --unmerged 2>/dev/null to check if there are unmerged entries, but as we don't normally expect there to be that many unmerged entries it probably does not matter. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > I had not commented as I don't use the prompt. I have just had a quick > read and I wonder if it would be more efficient to use > git diff --cached --quiet --diff-filter=U > rather than > git ls-files --unmerged 2>/dev/null > to check if there are unmerged entries, The former reads the on-disk index into in-core index, and reads tree objects (recursively for subdirectories) referenced by the HEAD, walks both in parallel to find differences and filters out the result to unmerged (I am not sure how well diff-filter works with unmerged paths, though). The latter rads the on-disk index into in-core index, scans the entries and finds unmerged entries. So if we compare the overhead to run either command standalone, I am reasonably sure that the latter would be a lot more efficient. But if the shell prompt code already needs to run the diff-index for other reasons (e.g. to show if there is any modification added to the index), that may change the equation. Instead of adding a separate and extra call to "ls-files -u", it might be more efficient if you can somehow piggy-back on an existing diff-index call. For example, you may be running "git diff --cached --quiet" for exit code to show if any change has been added, but you can instead run "git diff --no-ext-diff --no-renames --cached --name-status" and (1) if there is any output, then the index is dirty, and (2) if there is a line that begins with "U", you have an unmerged path right there.
On Mon, Aug 15, 2022 at 8:50 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi, > > On Sun, 14 Aug 2022, Junio C Hamano wrote: > > > Justin Donnelly <justinrdonnelly@gmail.com> writes: > > > > > I hope this is against protocol/etiquette, but after some initial > > > feedback from Junio, I haven't gotten any more. I wasn't sure if > > > nobody had seen the patch, or if there just wasn't any interest. > > > > It probably is a bit of both. I personally did not see much point > > in adding the long "conflicts" marker to the shell prompt (I did > > worry about possible complaints by end users triggered by seeing > > them suddenly without asking, which was why I commented on the > > patch) and I was waiting for interested folks to speak out. > > Speaking for myself, I was too busy elsewhere. But now that I looked over > the patch, I think it is fine. My only feedback is that it would be wise > to only add a single test case because that is plenty enough (after all, > it validates the `ls-files --unmerged` call and not the `cherry-pick` > code) and it is unnecessary to waste the electricity on additional tests > cases (even if somebody else foots the bill, it would do well for all of > us to start being more mindful about energy consumption). That makes sense. I'll get started on a re-roll to just have a single test that focuses specifically on the conflict indicator. > > Ciao, > Dscho Thanks, Justin
On Mon, Aug 15, 2022 at 12:00 PM Junio C Hamano <gitster@pobox.com> wrote: > > Phillip Wood <phillip.wood123@gmail.com> writes: > > > I had not commented as I don't use the prompt. I have just had a quick > > read and I wonder if it would be more efficient to use > > git diff --cached --quiet --diff-filter=U > > rather than > > git ls-files --unmerged 2>/dev/null > > to check if there are unmerged entries, > > The former reads the on-disk index into in-core index, and reads > tree objects (recursively for subdirectories) referenced by the > HEAD, walks both in parallel to find differences and filters out the > result to unmerged (I am not sure how well diff-filter works with > unmerged paths, though). > > The latter rads the on-disk index into in-core index, scans the > entries and finds unmerged entries. > > So if we compare the overhead to run either command standalone, I am > reasonably sure that the latter would be a lot more efficient. > Here's how I tested performance. The setup and test execution code are below. I tested each technique (`git ls-files --unmerged 2>/dev/null` and `git diff --cached --quiet --diff-filter=U`) 3 times and listed the results. Please let me know if you see any problems with the methodology. Setup: mkdir -p /tmp/perf/base && cd /tmp/perf/base git clone https://github.com/torvalds/linux.git Each test: cd /tmp/perf rm -rf test cp -r base test cd test/linux/drivers/watchdog git switch --detach v6.0-rc1 # pick some commit for consistency for f in *; do echo "/* a */" >> $f; done # 182 files git stash for f in *; do echo "/* b */" >> $f; done git commit -am "adding to end of files in watchdog directory" git stash pop time [[ $(git ls-files --unmerged 2>/dev/null) ]] # OR run the next one instead # time ! git diff --cached --quiet --diff-filter=U 2>/dev/null Results (hopefully this text lines up better for others than it does for me): time [[ $(git ls-files --unmerged 2>/dev/null) ]] run 1 run 2 run3 real 0m0.008s 0m0.009s 0m0.008s user 0m0.005s 0m0.001s 0m0.004s sys 0m0.004s 0m0.008s 0m0.004s time ! git diff --cached --quiet --diff-filter=U 2>/dev/null run 1 run 2 run3 real 0m0.009s 0m0.009s 0m0.007s user 0m0.004s 0m0.009s 0m0.007s sys 0m0.004s 0m0.000s 0m0.000s As you can see, the results are basically the same. I'm not sure if real world usage would yield different results. So for now, I'll defer to Junio's analysis and stick with `ls-files --unmerged`. > But if the shell prompt code already needs to run the diff-index for > other reasons (e.g. to show if there is any modification added to > the index), that may change the equation. Instead of adding a > separate and extra call to "ls-files -u", it might be more efficient > if you can somehow piggy-back on an existing diff-index call. For > example, you may be running "git diff --cached --quiet" for exit code > to show if any change has been added, but you can instead run "git > diff --no-ext-diff --no-renames --cached --name-status" and (1) if > there is any output, then the index is dirty, and (2) if there is a > line that begins with "U", you have an unmerged path right there. It had not occurred to me to consolidate and piggy-back off of other commands. I find the idea intriguing, but am not sure it makes sense to do it for only a single feature (especially this feature since the time to determine the conflict is short). I think it would make the code more complex, and it might be better to take a holistic approach to such an effort. Let me know if you strongly disagree. Thanks, Justin
On Tue, Aug 16, 2022 at 12:20 AM Justin Donnelly <justinrdonnelly@gmail.com> wrote: > > On Mon, Aug 15, 2022 at 12:00 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > Phillip Wood <phillip.wood123@gmail.com> writes: > > > > > I had not commented as I don't use the prompt. I have just had a quick > > > read and I wonder if it would be more efficient to use > > > git diff --cached --quiet --diff-filter=U > > > rather than > > > git ls-files --unmerged 2>/dev/null > > > to check if there are unmerged entries, > > > > The former reads the on-disk index into in-core index, and reads > > tree objects (recursively for subdirectories) referenced by the > > HEAD, walks both in parallel to find differences and filters out the > > result to unmerged (I am not sure how well diff-filter works with > > unmerged paths, though). > > > > The latter rads the on-disk index into in-core index, scans the > > entries and finds unmerged entries. > > > > So if we compare the overhead to run either command standalone, I am > > reasonably sure that the latter would be a lot more efficient. > > > > Here's how I tested performance. The setup and test execution code are > below. I tested each technique (`git ls-files --unmerged 2>/dev/null` > and `git diff --cached --quiet --diff-filter=U`) 3 times and listed > the results. Please let me know if you see any problems with the > methodology. > > Setup: > mkdir -p /tmp/perf/base && cd /tmp/perf/base > git clone https://github.com/torvalds/linux.git > > Each test: > cd /tmp/perf > rm -rf test > cp -r base test > cd test/linux/drivers/watchdog > git switch --detach v6.0-rc1 # pick some commit for consistency > for f in *; do echo "/* a */" >> $f; done # 182 files > git stash > for f in *; do echo "/* b */" >> $f; done > git commit -am "adding to end of files in watchdog directory" > git stash pop > time [[ $(git ls-files --unmerged 2>/dev/null) ]] > # OR run the next one instead > # time ! git diff --cached --quiet --diff-filter=U 2>/dev/null > > Results (hopefully this text lines up better for others than it does for me): > time [[ $(git ls-files --unmerged 2>/dev/null) ]] > run 1 run 2 run3 > real 0m0.008s 0m0.009s 0m0.008s > user 0m0.005s 0m0.001s 0m0.004s > sys 0m0.004s 0m0.008s 0m0.004s > > time ! git diff --cached --quiet --diff-filter=U 2>/dev/null > run 1 run 2 run3 > real 0m0.009s 0m0.009s 0m0.007s > user 0m0.004s 0m0.009s 0m0.007s > sys 0m0.004s 0m0.000s 0m0.000s > Actually, what's probably more important is how long the commands take when there is no conflict (that will be a far more common situation). I tested that today, and the numbers were about the same. > As you can see, the results are basically the same. I'm not sure if > real world usage would yield different results. So for now, I'll defer > to Junio's analysis and stick with `ls-files --unmerged`. > > > But if the shell prompt code already needs to run the diff-index for > > other reasons (e.g. to show if there is any modification added to > > the index), that may change the equation. Instead of adding a > > separate and extra call to "ls-files -u", it might be more efficient > > if you can somehow piggy-back on an existing diff-index call. For > > example, you may be running "git diff --cached --quiet" for exit code > > to show if any change has been added, but you can instead run "git > > diff --no-ext-diff --no-renames --cached --name-status" and (1) if > > there is any output, then the index is dirty, and (2) if there is a > > line that begins with "U", you have an unmerged path right there. > > It had not occurred to me to consolidate and piggy-back off of other > commands. I find the idea intriguing, but am not sure it makes sense > to do it for only a single feature (especially this feature since the > time to determine the conflict is short). I think it would make the > code more complex, and it might be better to take a holistic approach > to such an effort. Let me know if you strongly disagree. > > Thanks, > Justin
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index 1435548e004..57972c2845c 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -84,6 +84,10 @@ # single '?' character by setting GIT_PS1_COMPRESSSPARSESTATE, or omitted # by setting GIT_PS1_OMITSPARSESTATE. # +# If you would like to see a notification on the prompt when there are +# unresolved conflicts, set GIT_PS1_SHOWCONFLICTSTATE to "yes". The +# prompt will include "|CONFLICT". +# # If you would like to see more information about the identity of # commits checked out as a detached HEAD, set GIT_PS1_DESCRIBE_STYLE # to one of these values: @@ -508,6 +512,12 @@ __git_ps1 () r="$r $step/$total" fi + local conflict="" # state indicator for unresolved conflicts + if [[ "${GIT_PS1_SHOWCONFLICTSTATE}" == "yes" ]] && + [[ $(git ls-files --unmerged 2>/dev/null) ]]; then + conflict="|CONFLICT" + fi + local w="" local i="" local s="" @@ -572,7 +582,7 @@ __git_ps1 () fi local f="$h$w$i$s$u$p" - local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}" + local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}${conflict}" if [ $pcmode = yes ]; then if [ "${__git_printf_supports_v-}" != yes ]; then diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh index 6a30f5719c3..47eb98893ef 100755 --- a/t/t9903-bash-prompt.sh +++ b/t/t9903-bash-prompt.sh @@ -188,7 +188,23 @@ test_expect_success 'prompt - rebase merge' ' test_when_finished "git checkout main" && test_must_fail git rebase --merge b1 b2 && test_when_finished "git rebase --abort" && - __git_ps1 >"$actual" && + ( + sane_unset GIT_PS1_SHOWCONFLICTSTATE && + __git_ps1 >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - rebase merge conflict' ' + printf " (b2|REBASE 1/3|CONFLICT)" >expected && + git checkout b2 && + test_when_finished "git checkout main" && + test_must_fail git rebase --merge b1 b2 && + test_when_finished "git rebase --abort" && + ( + GIT_PS1_SHOWCONFLICTSTATE="yes" && + __git_ps1 >"$actual" + ) && test_cmp expected "$actual" ' @@ -198,7 +214,23 @@ test_expect_success 'prompt - rebase am' ' test_when_finished "git checkout main" && test_must_fail git rebase --apply b1 b2 && test_when_finished "git rebase --abort" && - __git_ps1 >"$actual" && + ( + sane_unset GIT_PS1_SHOWCONFLICTSTATE && + __git_ps1 >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - rebase am conflict' ' + printf " (b2|REBASE 1/3|CONFLICT)" >expected && + git checkout b2 && + test_when_finished "git checkout main" && + test_must_fail git rebase --apply b1 b2 && + test_when_finished "git rebase --abort" && + ( + GIT_PS1_SHOWCONFLICTSTATE="yes" && + __git_ps1 >"$actual" + ) && test_cmp expected "$actual" ' @@ -208,7 +240,23 @@ test_expect_success 'prompt - merge' ' test_when_finished "git checkout main" && test_must_fail git merge b2 && test_when_finished "git reset --hard" && - __git_ps1 >"$actual" && + ( + sane_unset GIT_PS1_SHOWCONFLICTSTATE && + __git_ps1 >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - merge conflict' ' + printf " (b1|MERGING|CONFLICT)" >expected && + git checkout b1 && + test_when_finished "git checkout main" && + test_must_fail git merge b2 && + test_when_finished "git reset --hard" && + ( + GIT_PS1_SHOWCONFLICTSTATE="yes" && + __git_ps1 >"$actual" + ) && test_cmp expected "$actual" ' @@ -216,7 +264,10 @@ test_expect_success 'prompt - cherry-pick' ' printf " (main|CHERRY-PICKING)" >expected && test_must_fail git cherry-pick b1 b1^ && test_when_finished "git cherry-pick --abort" && - __git_ps1 >"$actual" && + ( + sane_unset GIT_PS1_SHOWCONFLICTSTATE && + __git_ps1 >"$actual" + ) && test_cmp expected "$actual" && git reset --merge && test_must_fail git rev-parse CHERRY_PICK_HEAD && @@ -224,6 +275,17 @@ test_expect_success 'prompt - cherry-pick' ' test_cmp expected "$actual" ' +test_expect_success 'prompt - cherry-pick conflict' ' + printf " (main|CHERRY-PICKING|CONFLICT)" >expected && + test_must_fail git cherry-pick b1 b1^ && + test_when_finished "git cherry-pick --abort" && + ( + GIT_PS1_SHOWCONFLICTSTATE="yes" && + __git_ps1 >"$actual" + ) && + test_cmp expected "$actual" +' + test_expect_success 'prompt - revert' ' printf " (main|REVERTING)" >expected && test_must_fail git revert b1^ b1 &&