Re: [PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree
Stefan Beller writes:
> Shortly after f178c13fda (Revert "Merge branch
> 'sb/submodule-core-worktree'", 2018-09-07), we had another series
> that implemented partially the same, ensuring that core.worktree was
> set in a checked out submodule, namely 74d4731da1 (submodule--helper:
> replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)
>
> As the series 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c',
> 2018-09-17) has different goals than the reverted series 7e25437d35
> (Merge branch 'sb/submodule-core-worktree', 2018-07-18), I'd wanted to
> replay the series on top of it to reach the goal of having `core.worktree`
> correctly set when the submodules worktree is present, and unset when the
> worktree is not present.
>
> The replay resulted in a strange merge conflict highlighting that
> the BUG message was not changed in 74d4731da1 (submodule--helper:
> replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13).
>
> Fix the error message.
>
> Signed-off-by: Stefan Beller
> ---
Unlike the step 2/4 I commented on, this does explain what this
wants to do and why, at least when looked from sideways. Is the
above saying the same as the following two-liner?
An ealier mistake while rebasing to produce 74d4731da1
failed to update this BUG message. Fix this.
> builtin/submodule--helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d38113a31a..31ac30cf2f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char
> **argv, const char *prefix)
> struct repository subrepo;
>
> if (argc != 2)
> - BUG("submodule--helper connect-gitdir-workingtree
> ");
> + BUG("submodule--helper ensure-core-worktree ");
>
> path = argv[1];
Re: bug: git fetch reports too many unreachable loose objects
On Fri, Dec 7, 2018 at 6:14 PM Josh Wolfe wrote:
>
> git version 2.19.1
> steps to reproduce:
>
> # start in a brand new repo
> git init
>
> # create lots of unreachable loose objects
> for i in {1..1}; do git commit-tree -m "$(head -c 12 /dev/urandom
> | base64)" "$(git mktree <&-)" <&-; done
> # this prints a lot of output and takes a minute or so to run
>
> # trigger git gc to run in the background
> git fetch
> # Auto packing the repository in background for optimum performance.
> # See "git help gc" for manual housekeeping.
>
> # trigger it again
> git fetch
> # Auto packing the repository in background for optimum performance.
> # See "git help gc" for manual housekeeping.
> # error: The last gc run reported the following. Please correct the root cause
> # and remove .git/gc.log.
> # Automatic cleanup will not be performed until the file is removed.
> #
> # warning: There are too many unreachable loose objects; run 'git
> prune' to remove them.
>
> # to manually fix this, run git prune:
> git prune
>
> # note that `git gc` does not fix the problem, and appears to do
> nothing in this situation:
> git gc
>
>
> According to the `git fetch` output, the `git help gc` docs, and the
> `git help prune` docs, I don't think I shouldn't ever have to run `git
> prune` manually, so this behavior seems like a bug to me. Please
> correct me if this is expected behavior.
Known bug, there are a variety of other ways to trigger it too. See
the threads here for more info:
https://public-inbox.org/git/87inc89j38@evledraar.gmail.com/
https://public-inbox.org/git/20180716172717.237373-1-jonathanta...@google.com/
There are probably other threads as well.
bug: git fetch reports too many unreachable loose objects
git version 2.19.1
steps to reproduce:
# start in a brand new repo
git init
# create lots of unreachable loose objects
for i in {1..1}; do git commit-tree -m "$(head -c 12 /dev/urandom
| base64)" "$(git mktree <&-)" <&-; done
# this prints a lot of output and takes a minute or so to run
# trigger git gc to run in the background
git fetch
# Auto packing the repository in background for optimum performance.
# See "git help gc" for manual housekeeping.
# trigger it again
git fetch
# Auto packing the repository in background for optimum performance.
# See "git help gc" for manual housekeeping.
# error: The last gc run reported the following. Please correct the root cause
# and remove .git/gc.log.
# Automatic cleanup will not be performed until the file is removed.
#
# warning: There are too many unreachable loose objects; run 'git
prune' to remove them.
# to manually fix this, run git prune:
git prune
# note that `git gc` does not fix the problem, and appears to do
nothing in this situation:
git gc
According to the `git fetch` output, the `git help gc` docs, and the
`git help prune` docs, I don't think I shouldn't ever have to run `git
prune` manually, so this behavior seems like a bug to me. Please
correct me if this is expected behavior.
In case anyone's wondering why I'm creating unreachable loose objects,
here's the usecase: https://stackoverflow.com/a/50403179/367916 . I
would love a first-class solution to obviate that workaround, but that
is probably a separate issue.
Josh
[PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree
Shortly after f178c13fda (Revert "Merge branch
'sb/submodule-core-worktree'", 2018-09-07), we had another series
that implemented partially the same, ensuring that core.worktree was
set in a checked out submodule, namely 74d4731da1 (submodule--helper:
replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)
As the series 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c',
2018-09-17) has different goals than the reverted series 7e25437d35
(Merge branch 'sb/submodule-core-worktree', 2018-07-18), I'd wanted to
replay the series on top of it to reach the goal of having `core.worktree`
correctly set when the submodules worktree is present, and unset when the
worktree is not present.
The replay resulted in a strange merge conflict highlighting that
the BUG message was not changed in 74d4731da1 (submodule--helper:
replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13).
Fix the error message.
Signed-off-by: Stefan Beller
---
builtin/submodule--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d38113a31a..31ac30cf2f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char
**argv, const char *prefix)
struct repository subrepo;
if (argc != 2)
- BUG("submodule--helper connect-gitdir-workingtree
");
+ BUG("submodule--helper ensure-core-worktree ");
path = argv[1];
--
2.20.0.rc2.403.gdbc3b29805-goog
Re: [BUG REPORT] Git does not correctly replay bisect log
On Thu, Dec 6, 2018 at 6:30 PM Lukáš Krejčí wrote:
>
> I am talking about `git bisect replay`. The shell script, as far as I
> can see, only updates the references (ref/bisect/*) and never checks if
> the revisions marked as 'good' are ancestors of the 'bad' one.
> Therefore, $GIT_DIR/BISECT_ANCESTORS_OK file is never created.
Indeed `git bisect replay` first only updates the references
(ref/bisect/*) according to all the "git bisect {good,bad}"
instructions it finds in the log it is passed. After doing that
though, before exiting, it calls `bisect_auto_next` which calls
`bisect_next` which calls `git bisect--helper --next-all` which checks
the merge bases.
I think it is a bug.
`git bisect replay` is right to only update the references
(ref/bisect/*) and not to compute and checkout the best commit to test
at each step except at the end, but it should probably just create the
$GIT_DIR/BISECT_ANCESTORS_OK file if more than one bisection step has
been performed (because the merge bases are checked as part of the
first bisection step only).
> The first time the ancestors are checked is in the helper (`git-bisect-
> -help --next-all`) that has only limited information from refs/bisect*.
`git-bisect--helper --next-all` knows how to get refs/bisect*
information, otherwise it couldn't decide which is the next best
commit to test.
Thanks for your help in debugging this,
Christian.
behaviour of git-blame -M -C (maybe a bug?)
hi everybody, I am the maintainer of cregit. We are trying to improve blame traceability at the token level (see https://github.com/dmgerman/papers/blob/master/editorials/cregit/cregit.org) We use git-blame heavilty in cregit. One of the features that I would like to add to cregit is the ability track movement of code. I have been testing git-blame -M -C and I found some behaviour that seems incorrect. I have created a very simple repository that I think showcases this problem: https://github.com/dmgerman/testBlameMove this repo have 4 commits (listed below in order of execution): 1. A file is created tpm-dev.c (authored by D German), 2. a refactoring (code is moved from tpm-dev.c to tpm-dev-common.c, a new file). Author is "refactor" 3. a commit that adds some few contiguous lines (the existence of this commit seems to matter). Author is "none" 4. a commit that changes few lines and alters the result of blame for lines not modified by this commit. Author is "problem" See below. I am running blame at different commits, showing only the lines attributed to author "refactor" (author of commit #2). dmg@iodine:/tmp/testRepo|master ⇒ git log --oneline ded1aa1 (HEAD -> master, origin/master) problematic commit 3720e68 simple commit 391adba refactoring 33165cb file before refactoring if we checkout 391adba and do blame -M -C we get this: dmg@iodine:/tmp/testRepo|3720e68 ⇒ git checkout 3720e68 && git blame -M -C tpm-dev-common.c | grep refactor | head HEAD is now at 3720e68 simple commit 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 24) begin_include 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 25) include|# 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 26) directive|include 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 27) file|"tpm-dev.h" 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 28) end_include 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 147) DECL|function|tpm_common_open (struct file * file,struct tpm_chip * chip,struct file_priv * priv) 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 148) name|void 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 149) name|tpm_common_open 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 150) parameter_list|( 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 151) name|struct so far, so good. blame detects the movement. Note that the changes by refactor are adding 5 lines (24 to 28) and then adding some at 147 and beyond. now do it for the next commit: 3720e68 things continue to look good. The changes of this commit do not affect any of these lines. now... the next commit, the problematic: ded1aa1 (author is not refactor) dmg@iodine:/tmp/testRepo|3720e68 ⇒ git checkout ded1aa1 && git blame -M -C tpm-dev-common.c | grep refactor | head Previous HEAD position was 3720e68 simple commit HEAD is now at ded1aa1 problematic commit 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 24) begin_include 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 25) include|# 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 26) directive|include 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 27) file|"tpm-dev.h" 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 28) end_include 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 29) 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 30) begin_function 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 53) name|user_read_timer 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 54) argument_list|) 391adba4 tpm-dev-common.c (refactor 2018-12-06 12:41:10 -0800 150) DECL|function|tpm_common_open (struct file * file,struct tpm_chip * chip,struct file_priv * priv) now blame assigns the lines 29, 30, 53 and 54 to commit 391adba4 refactor!!! This is what I think is a bug. (by the way, the changes made in this last commit were between 28 and 150) thank you in advance for any clues on why git-blame is behaving like this. --dmg --- D M German http://turingmachine.org
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 06.12.18 um 01:58 schrieb Junio C Hamano: > Frank Schäfer writes: > >> Just to be sure that I'm not missing anything here: >> What's your definition of "LF in repository, CRLF in working tree" in >> terms of config parameters ? > :::Documentation/config/core.txt::: > > core.autocrlf:: > Setting this variable to "true" is the same as setting > the `text` attribute to "auto" on all files and core.eol to "crlf". > Set to true if you want to have `CRLF` line endings in your > working directory and the repository has LF line endings. > This variable can be set to 'input', > in which case no output conversion is performed. Argh... crap. I was missing that I have to set the "text" attribute manually... Thats why core.eol=crlf didn't make a difference in my tests. :/ Let me thoroughly re-validate all cases. I will likely not find the time for that before Monday, but I think that break could be helpful. ;) Regards, Frank
Re: [BUG REPORT] Git does not correctly replay bisect log
On Thu, 2018-12-06 at 17:31 +0100, Christian Couder wrote: > > When Git replays the bisect log, it only updates refs/bisect/bad, > > refs/bisect/good-*, refs/bisect/skip-* and reconstructs the log in > > .git/BISECT_LOG. After that check_good_are_ancestors_of_bad() verifies > > that all good commits are ancestors of the bad commit, and if not, the > > message "Bisecting: a merge base must be tested" is printed and the > > branch is switched to the merge base of the bad and all the good > > commits. > > I am not sure if you are talking about running `git bisect replay` or > sourcing the log in the above. I am talking about `git bisect replay`. The shell script, as far as I can see, only updates the references (ref/bisect/*) and never checks if the revisions marked as 'good' are ancestors of the 'bad' one. Therefore, $GIT_DIR/BISECT_ANCESTORS_OK file is never created. The first time the ancestors are checked is in the helper (`git-bisect- -help --next-all`) that has only limited information from refs/bisect*.
Re: [BUG REPORT] Git does not correctly replay bisect log
Hi, On Thu, Dec 6, 2018 at 3:43 PM Lukáš Krejčí wrote: > > Hello again, > > after looking into this today, I'm not sure if this can be considered a > bug - it's just that I expected Git to check out the exact commit to > test that was there before resetting the bisect. That made me uncertain > whether Git restored the correct state. > > When I looked at what Git actually does, it became clear that the > behavior is not incorrect but perhaps a bit surprising. Yeah, I agree. I suspect, but I am not sure, that the difference of behavior is because in one case we only check merge bases once at the beginning (maybe because the BISECT_ANCESTORS_OK file always exists) while in the other case we check them more than once during the bisection. I haven't had time to look closely at this, but I would like to. > When Git replays the bisect log, it only updates refs/bisect/bad, > refs/bisect/good-*, refs/bisect/skip-* and reconstructs the log in > .git/BISECT_LOG. After that check_good_are_ancestors_of_bad() verifies > that all good commits are ancestors of the bad commit, and if not, the > message "Bisecting: a merge base must be tested" is printed and the > branch is switched to the merge base of the bad and all the good > commits. I am not sure if you are talking about running `git bisect replay` or sourcing the log in the above. > Basically, some state is lost because Git "forgot" the first good > commit from the log already was an ancestor of the first bad one. The BISECT_ANCESTORS_OK file should be there to avoid forgetting that we already checked the merge bases. > In other words, it's as if I just started the bisect with the following > commands and just pasted the whole bisect log to .git/BISECT_LOG: > > $ git bisect start > $ git bisect good 94710cac0ef4ee177a63b5227664b38c95bbf703 > $ git bisect good 958f338e96f874a0d29442396d6adf9c1e17aa2d > $ git bisect bad 1b0d274523df5ef1caedc834da055ff721e4d4f0 > Bisecting: a merge base must be tested > [1e4b044d22517cae7047c99038abb23243ca] Linux 4.18-rc4 Yeah, when we start a new bisection the BISECT_ANCESTORS_OK file should be erased if it exists, while it shouldn't be erased when we are already in the middle of an existing bisection. [...] > And indeed, marking the merge base as good switches to the correct > commit after the bisect. Marking it as bad will fail, so at least you > can't make a mistake after replaying the bisect log: > $ git bisect bad > The merge base 1e4b044d22517cae7047c99038abb23243ca is bad. > This means the bug has been fixed between > 1e4b044d22517cae7047c99038abb23243ca and > [94710cac0ef4ee177a63b5227664b38c95bbf703 > 958f338e96f874a0d29442396d6adf9c1e17aa2d]. Yeah, I think this works as expected. > Once again, I'm sorry for the noise. I guess it wasn't clear from the > man page that something like this could happen and that made me think > that this was a bug. No reason to be sorry, there might still be a bug related to the BISECT_ANCESTORS_OK file or something. I hope I can take a look at this more closely soon. Thanks for your report and your work on this, Christian.
Re: [BUG REPORT] Git does not correctly replay bisect log
Hello again, after looking into this today, I'm not sure if this can be considered a bug - it's just that I expected Git to check out the exact commit to test that was there before resetting the bisect. That made me uncertain whether Git restored the correct state. When I looked at what Git actually does, it became clear that the behavior is not incorrect but perhaps a bit surprising. When Git replays the bisect log, it only updates refs/bisect/bad, refs/bisect/good-*, refs/bisect/skip-* and reconstructs the log in .git/BISECT_LOG. After that check_good_are_ancestors_of_bad() verifies that all good commits are ancestors of the bad commit, and if not, the message "Bisecting: a merge base must be tested" is printed and the branch is switched to the merge base of the bad and all the good commits. Basically, some state is lost because Git "forgot" the first good commit from the log already was an ancestor of the first bad one. In other words, it's as if I just started the bisect with the following commands and just pasted the whole bisect log to .git/BISECT_LOG: $ git bisect start $ git bisect good 94710cac0ef4ee177a63b5227664b38c95bbf703 $ git bisect good 958f338e96f874a0d29442396d6adf9c1e17aa2d $ git bisect bad 1b0d274523df5ef1caedc834da055ff721e4d4f0 Bisecting: a merge base must be tested [1e4b044d22517cae7047c99038abb23243ca] Linux 4.18-rc4 (here's the full bisect log again for reference) git bisect start # bad: [5b394b2ddf0347bef56e50c69a58773c94343ff3] Linux 4.19-rc1 git bisect bad 5b394b2ddf0347bef56e50c69a58773c94343ff3 # good: [94710cac0ef4ee177a63b5227664b38c95bbf703] Linux 4.18 git bisect good 94710cac0ef4ee177a63b5227664b38c95bbf703 # bad: [54dbe75bbf1e189982516de179147208e90b5e45] Merge tag 'drm-next-2018-08-15' of git://anongit.freedesktop.org/drm/drm git bisect bad 54dbe75bbf1e189982516de179147208e90b5e45 # bad: [0a957467c5fd46142bc9c52758ffc552d4c5e2f7] x86: i8259: Add missing include file git bisect bad 0a957467c5fd46142bc9c52758ffc552d4c5e2f7 # good: [958f338e96f874a0d29442396d6adf9c1e17aa2d] Merge branch 'l1tf-final' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip git bisect good 958f338e96f874a0d29442396d6adf9c1e17aa2d # bad: [2c20443ec221dcb76484b30933593e8ecd836bbd] Merge tag 'acpi-4.19-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm git bisect bad 2c20443ec221dcb76484b30933593e8ecd836bbd # bad: [c2fc71c9b74c1e87336a27dba1a5edc69d2690f1] Merge tag 'mtd/for-4.19' of git://git.infradead.org/linux-mtd git bisect bad c2fc71c9b74c1e87336a27dba1a5edc69d2690f1 # bad: [b86d865cb1cae1e61527ea0b8977078bbf694328] blkcg: Make blkg_root_lookup() work for queues in bypass mode git bisect bad b86d865cb1cae1e61527ea0b8977078bbf694328 # bad: [1b0d274523df5ef1caedc834da055ff721e4d4f0] nvmet: don't use uuid_le type git bisect bad 1b0d274523df5ef1caedc834da055ff721e4d4f0 And indeed, marking the merge base as good switches to the correct commit after the bisect. Marking it as bad will fail, so at least you can't make a mistake after replaying the bisect log: $ git bisect bad The merge base 1e4b044d22517cae7047c99038abb23243ca is bad. This means the bug has been fixed between 1e4b044d22517cae7047c99038abb23243ca and [94710cac0ef4ee177a63b5227664b38c95bbf703 958f338e96f874a0d29442396d6adf9c1e17aa2d]. Once again, I'm sorry for the noise. I guess it wasn't clear from the man page that something like this could happen and that made me think that this was a bug.
Bug: git add --patch does not honor "diff.noprefix"
Hi, When running "git add -p" on git version 2.19.2 and "diff.noprefix" set to true, it still shows the "a/" and "b/" prefixes. This issue has been reported in 2016 already[1], but is still there in 2.19.2. [1] https://public-inbox.org/git/e1d7329a-a54b-4d09-a72a-62eca8005...@gmail.com/T/ -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=-
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Frank Schäfer writes: > Just to be sure that I'm not missing anything here: > What's your definition of "LF in repository, CRLF in working tree" in > terms of config parameters ? :::Documentation/config/core.txt::: core.autocrlf:: Setting this variable to "true" is the same as setting the `text` attribute to "auto" on all files and core.eol to "crlf". Set to true if you want to have `CRLF` line endings in your working directory and the repository has LF line endings. This variable can be set to 'input', in which case no output conversion is performed.
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 05.12.18 um 20:29 schrieb Frank Schäfer: Am 02.12.18 um 22:22 schrieb Johannes Sixt: Am 02.12.18 um 20:31 schrieb Frank Schäfer: With other words: "If CR comes immediately before a LF, do the following with *all* lines: after the CR if eol=lf but do not after the CR if eol=crlf." Why? It is the pager's duty to highlight CR, IMO. If it doesn't, but the user wants to see them, then they are using the wrong pager or the wrong pager settings. AFAIU Junios explanation it's not the pagers fault. Then Junio and I are in disagreement. IMO, Git does not have to be more clever than the pager when it comes to presentation of text. As far as I am concerned, I do not have any of my files marked as eol=crlf, but I do *not* want to see ^M in the pager. I.e., having git insert between CR and LF would do the wrong thing for me. But doing the same thing in added lines is doing the right thing for you ? Yes, I think so. As long as I'm not telling Git that my files are CRLF when they actual are, then the CR before LF is a whitespace error. Nevertheless, I do *NOT* want Git to outwit my pager by inserting between CR and LF all the time so that it is forced to treat the lone CR like a control character that is to be made visible. Or are you suggesting to fix the behavior of added lines instead ? In any case, inconsistent behavior is not what we want. I'm suggesting that users who knowingly store CRLF files in the database, but do not want to see ^M in added lines have to use whitespace=cr-at-eol and that's all. I do not see inconsistency. -- Hannes
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 03.12.18 um 02:15 schrieb Junio C Hamano: > Frank Schäfer writes: > >> Hi Junio, >> >> Am 29.11.18 um 03:11 schrieb Junio C Hamano: >> [...] >>> This was misspoken a bit. Let's revise it to >>> >>> When producing a colored output (not limited to whitespace >>> error coloring of diff output) for contents that are not >>> marked as eol=crlf (and other historical means), insert >>> before a CR that comes immediately before a LF. >> You mean >> ... >> *after* a CR that comes immediately before a LF." >> >> OK, AFAICS this produces the desired output in all cases if eol=lf. > OK, yeah, I think I meant "after", i.e. ... CR LF, in order > to force CR to be separated from LF. > >> Now what about the case eol=crlf ? > I have no strong opinions, other than that "LF in repository, CRLF > in working tree" would make the issue go away (when it is solved for > EOL=LF like the above, that is). > >> Keeping the current behavior of '-' lines is correct. >> But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ? > If "LF in repository, CRLF in working tree" is done, there would not > be ^M at the end of the line, not just for removed lines, but also > for added lines, no? Just to be sure that I'm not missing anything here: What's your definition of "LF in repository, CRLF in working tree" in terms of config parameters ? Regards, Frank
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 02.12.18 um 22:22 schrieb Johannes Sixt: > Am 02.12.18 um 20:31 schrieb Frank Schäfer: >> With other words: >> "If CR comes immediately before a LF, do the following with *all* lines: >> after the CR if eol=lf but do not after the CR if >> eol=crlf." > > Why? It is the pager's duty to highlight CR, IMO. If it doesn't, but > the user wants to see them, then they are using the wrong pager or the > wrong pager settings. AFAIU Junios explanation it's not the pagers fault. > > As far as I am concerned, I do not have any of my files marked as > eol=crlf, but I do *not* want to see ^M in the pager. I.e., having git > insert between CR and LF would do the wrong thing for me. > But doing the same thing in added lines is doing the right thing for you ? Or are you suggesting to fix the behavior of added lines instead ? In any case, inconsistent behavior is not what we want. Regards, Frank
Re: [BUG REPORT] Git does not correctly replay bisect log
On Tue, 2018-12-04 at 13:01 +0100, Christian Couder wrote: > To debug I think it would be interesting to see the output of the > following commands just before we get different results: > > git for-each-ref 'refs/bisect/*' > > and > > git log -1 --format=oneline > I placed the following snippet at the end of the loop in bisect_replay(): echo "COMMAND: '$git' '$bisect' '$command' '$rev'" git for-each-ref 'refs/bisect/*' echo "current HEAD: $(git log -1 --format=oneline)" echo "---" $ env GIT_TRACE=0 git bisect replay /var/tmp/git-bisect.log We are not bisecting. COMMAND: 'git' 'bisect' 'start' '' current HEAD: 2595646791c319cadfdbf271563aac97d0843dc7 Linux 4.20-rc5 --- COMMAND: 'git' 'bisect' 'bad' '5b394b2ddf0347bef56e50c69a58773c94343ff3' 5b394b2ddf0347bef56e50c69a58773c94343ff3 commit refs/bisect/bad current HEAD: 2595646791c319cadfdbf271563aac97d0843dc7 Linux 4.20-rc5 --- COMMAND: 'git' 'bisect' 'good' '94710cac0ef4ee177a63b5227664b38c95bbf703' 5b394b2ddf0347bef56e50c69a58773c94343ff3 commit refs/bisect/bad 94710cac0ef4ee177a63b5227664b38c95bbf703 commit refs/bisect/good-94710cac0ef4ee177a63b5227664b38c95bbf703 current HEAD: 2595646791c319cadfdbf271563aac97d0843dc7 Linux 4.20-rc5 --- COMMAND: 'git' 'bisect' 'bad' '54dbe75bbf1e189982516de179147208e90b5e45' 54dbe75bbf1e189982516de179147208e90b5e45 commit refs/bisect/bad 94710cac0ef4ee177a63b5227664b38c95bbf703 commit refs/bisect/good-94710cac0ef4ee177a63b5227664b38c95bbf703 current HEAD: 2595646791c319cadfdbf271563aac97d0843dc7 Linux 4.20-rc5 --- COMMAND: 'git' 'bisect' 'bad' '0a957467c5fd46142bc9c52758ffc552d4c5e2f7' 0a957467c5fd46142bc9c52758ffc552d4c5e2f7 commit refs/bisect/bad 94710cac0ef4ee177a63b5227664b38c95bbf703 commit refs/bisect/good-94710cac0ef4ee177a63b5227664b38c95bbf703 current HEAD: 2595646791c319cadfdbf271563aac97d0843dc7 Linux 4.20-rc5 --- COMMAND: 'git' 'bisect' 'good' '958f338e96f874a0d29442396d6adf9c1e17aa2d' 0a957467c5fd46142bc9c52758ffc552d4c5e2f7 commit refs/bisect/bad 94710cac0ef4ee177a63b5227664b38c95bbf703 commit refs/bisect/good-94710cac0ef4ee177a63b5227664b38c95bbf703 958f338e96f874a0d29442396d6adf9c1e17aa2d commit refs/bisect/good-958f338e96f874a0d29442396d6adf9c1e17aa2d current HEAD: 2595646791c319cadfdbf271563aac97d0843dc7 Linux 4.20-rc5 --- COMMAND: 'git' 'bisect' 'bad' '2c20443ec221dcb76484b30933593e8ecd836bbd' 2c20443ec221dcb76484b30933593e8ecd836bbd commit refs/bisect/bad 94710cac0ef4ee177a63b5227664b38c95bbf703 commit refs/bisect/good-94710cac0ef4ee177a63b5227664b38c95bbf703 958f338e96f874a0d29442396d6adf9c1e17aa2d commit refs/bisect/good-958f338e96f874a0d29442396d6adf9c1e17aa2d current HEAD: 2595646791c319cadfdbf271563aac97d0843dc7 Linux 4.20-rc5 --- COMMAND: 'git' 'bisect' 'bad' 'c2fc71c9b74c1e87336a27dba1a5edc69d2690f1' c2fc71c9b74c1e87336a27dba1a5edc69d2690f1 commit refs/bisect/bad 94710cac0ef4ee177a63b5227664b38c95bbf703 commit refs/bisect/good-94710cac0ef4ee177a63b5227664b38c95bbf703 958f338e96f874a0d29442396d6adf9c1e17aa2d commit refs/bisect/good-958f338e96f874a0d29442396d6adf9c1e17aa2d current HEAD: 2595646791c319cadfdbf271563aac97d0843dc7 Linux 4.20-rc5 --- COMMAND: 'git' 'bisect' 'bad' 'b86d865cb1cae1e61527ea0b8977078bbf694328' b86d865cb1cae1e61527ea0b8977078bbf694328 commit refs/bisect/bad 94710cac0ef4ee177a63b5227664b38c95bbf703 commit refs/bisect/good-94710cac0ef4ee177a63b5227664b38c95bbf703 958f338e96f874a0d29442396d6adf9c1e17aa2d commit refs/bisect/good-958f338e96f874a0d29442396d6adf9c1e17aa2d current HEAD: 2595646791c319cadfdbf271563aac97d0843dc7 Linux 4.20-rc5 --- COMMAND: 'git' 'bisect' 'bad' '1b0d274523df5ef1caedc834da055ff721e4d4f0' 1b0d274523df5ef1caedc834da055ff721e4d4f0 commit refs/bisect/bad 94710cac0ef4ee177a63b5227664b38c95bbf703 commit refs/bisect/good-94710cac0ef4ee177a63b5227664b38c95bbf703 958f338e96f874a0d29442396d6adf9c1e17aa2d commit refs/bisect/good-958f338e96f874a0d29442396d6adf9c1e17aa2d current HEAD: 2595646791c319cadfdbf271563aac97d0843dc7 Linux 4.20-rc5 --- Bisecting: a merge base must be tested [1e4b044d22517cae7047c99038abb23243ca] Linux 4.18-rc4 # I placed git for-each-ref 'refs/bisect/*' after each command in the file: $ . /var/tmp/git-bisect.log 5b394b2ddf0347bef56e50c69a58773c94343ff3 commit refs/bisect/bad Bisecting: 6112 revisions left to test after this (roughly 13 steps) [54dbe75bbf1e189982516de179147208e90b5e45] Merge tag 'drm-next-2018-08-15' of git://anongit.freedesktop.org/drm/drm 5b394b2ddf0347bef56e50c69a58773c94343ff3 commit refs/bisect/bad 94710cac0ef4ee177a63b5227664b38c95bbf703 commit refs/bisect/good-94710cac0ef4ee177a63b5227664b38c95bbf703 Bisecting: 3881 revisions left to test after this (roughly 12 steps) [0a957467c5fd46142bc9c52758ffc552d4c5e2f7] x86: i8259: Add missing include file 54dbe75bbf1e189982516de179147208e90b5e45 commit refs/bisect/bad 94710cac0ef4ee177a63b5227664b38c95bbf703 commit refs/bisect/good-94710cac0ef4ee177a63b5227664b38c95bbf703 Bisecting: 1595 revisions left to test after this
Re: [BUG REPORT] Git does not correctly replay bisect log
On Tue, Dec 4, 2018 at 12:20 PM Lukáš Krejčí wrote: > > On Tue, 2018-12-04 at 12:04 +0100, Christian Couder wrote: > > > > Could you try to check that? And first could you give us the output of: > > > > git merge-base 5b394b2ddf0347bef56e50c69a58773c94343ff3 > > 94710cac0ef4ee177a63b5227664b38c95bbf703 > > $ git merge-base 5b394b2ddf0347bef56e50c69a58773c94343ff3 > 94710cac0ef4ee177a63b5227664b38c95bbf703 > 94710cac0ef4ee177a63b5227664b38c95bbf703 > $ git log -1 --format=oneline 94710cac0ef4ee177a63b5227664b38c95bbf703 > 94710cac0ef4ee177a63b5227664b38c95bbf703 (tag: v4.18) Linux 4.18 94710cac0ef4ee177a63b5227664b38c95bbf703 is the good commit that was initially given. This means that the good commit 94710cac0ef4ee177a63b5227664b38c95bbf703 is an ancestor of the bad commit 5b394b2ddf0347bef56e50c69a58773c94343ff3 i and there should be no reason to test a merge base when replaying. After testing on my machine, it seems that the problem is not happening at the beginning of the replay. To debug I think it would be interesting to see the output of the following commands just before we get different results: git for-each-ref 'refs/bisect/*' and git log -1 --format=oneline in the case we are using `git bisect replay` and in the case we are running the commands from the bisect log manually. (You might need to temporarily remove the last command from the bisect log to do that.)
Re: [BUG REPORT] Git does not correctly replay bisect log
(I'm sorry about the formatting, here's the message again.) Executing git bisect replay reaches a different commit than the one that is obtained by running the commands from the bisect log manually. Distribution: Arch Linux git: 2.19.2-1 perl: 5.28.1-1 pcre2: 10.32-1 expat: 2.2.6-1 perl-error: 0.17027-1 grep: 3.1-2 bash: 4.4.023-1 no system /etc/gitconfig is present tried with no ~/.gitconfig $ cat .git/config [core] repositoryformatversion = 0 filemode = true bare = false logallrefupdates = true [remote "origin"] url = git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git fetch = +refs/heads/*:refs/remotes/origin/* [branch "master"] remote = origin merge = refs/heads/master $ git fsck Checking object directories: 100% (256/256), done. warning in tag 5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 26791a8bcf0e6d33f43aef7682bdb555236d56de: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 9e734775f7c22d2f89943ad6c745571f1930105f: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 0397236d43e48e821cce5bbe6a80a1a56bb7cc3a: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag ebb5573ea8beaf000d4833735f3e53acb9af844c: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 06f6d9e2f140466eeb41e494e14167f90210f89d: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 701d7ecec3e0c6b4ab9bb824fd2b34be4da63b7e: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 733ad933f62e82ebc92fed988c7f0795e64dea62: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag c521cb0f10ef2bf28a18e1cc8adf378ccbbe5a19: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag a339981ec18d304f9efeb9ccf01b1f04302edf32: missingTaggerEntry: invalid format - expected 'tagger' line Checking objects: 100% (6428247/6428247), done. Checking connectivity: 6369862, done. $ cat /var/tmp/git-bisect.log git bisect start # bad: [5b394b2ddf0347bef56e50c69a58773c94343ff3] Linux 4.19-rc1 git bisect bad 5b394b2ddf0347bef56e50c69a58773c94343ff3 # good: [94710cac0ef4ee177a63b5227664b38c95bbf703] Linux 4.18 git bisect good 94710cac0ef4ee177a63b5227664b38c95bbf703 # bad: [54dbe75bbf1e189982516de179147208e90b5e45] Merge tag 'drm-next-2018-08-15' of git://anongit.freedesktop.org/drm/drm git bisect bad 54dbe75bbf1e189982516de179147208e90b5e45 # bad: [0a957467c5fd46142bc9c52758ffc552d4c5e2f7] x86: i8259: Add missing include file git bisect bad 0a957467c5fd46142bc9c52758ffc552d4c5e2f7 # good: [958f338e96f874a0d29442396d6adf9c1e17aa2d] Merge branch 'l1tf-final' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip git bisect good 958f338e96f874a0d29442396d6adf9c1e17aa2d # bad: [2c20443ec221dcb76484b30933593e8ecd836bbd] Merge tag 'acpi-4.19-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm git bisect bad 2c20443ec221dcb76484b30933593e8ecd836bbd # bad: [c2fc71c9b74c1e87336a27dba1a5edc69d2690f1] Merge tag 'mtd/for-4.19' of git://git.infradead.org/linux-mtd git bisect bad c2fc71c9b74c1e87336a27dba1a5edc69d2690f1 # bad: [b86d865cb1cae1e61527ea0b8977078bbf694328] blkcg: Make blkg_root_lookup() work for queues in bypass mode git bisect bad b86d865cb1cae1e61527ea0b8977078bbf694328 # bad: [1b0d274523df5ef1caedc834da055ff721e4d4f0] nvmet: don't use uuid_le type git bisect bad 1b0d274523df5ef1caedc834da055ff721e4d4f0 $ git status On branch master Your branch is up to date with 'origin/master'. nothing to commit, working tree clean $ git log -1 --format=oneline 2595646791c319cadfdbf271563aac97d0843dc7 (HEAD -> master, tag: v4.20-rc5, origin/master, origin/HEAD) Linux 4.20-rc5 $ git bisect replay /var/tmp/git-bisect.log We are not bisecting. Bisecting: a merge base must be tested [d72e90f33aa4709ebecc5005562f52335e106a60] Linux 4.18-rc6 $ git log -1 --format=oneline d72e90f33aa4709ebecc5005562f52335e106a60 (HEAD, tag: v4.18-rc6) Linux 4.18-rc6 # Running the commands from the bisect log manually, however: $ git bisect reset Checking out files: 100% (18326/18326), done. Previous HEAD position was d72e90f33aa4 Linux 4.18-rc6 Switched to branch 'master' Your branch is up to date with 'origin/master'. $ . /var/tmp/git-bisect.log Bisecting: 6112 revisions left to test after this (roughly 13 steps) [54dbe75bbf1e189982516de179147208e90b5e45] Merge tag 'drm-next-2018-08-15' of git://anongit.freedesktop.org/drm/drm Bisecting: 3881 revisions left to test after this (roughly 12 steps) [0a957467c5fd46142bc9c52758ffc552d4c5e2f7] x86: i8259: Add missing include file Bisecting: 1595 revisions left to test after this (roughly 11 steps) [958f338e96f874a0d29442396d6adf9c1e17aa2d] Merge branch 'l1tf-final' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip Bisecting: 854 revisions left to test after this (roughly 10
Re: [BUG REPORT] Git does not correctly replay bisect log
On Tue, 2018-12-04 at 12:04 +0100, Christian Couder wrote: > > Could you try to check that? And first could you give us the output of: > > git merge-base 5b394b2ddf0347bef56e50c69a58773c94343ff3 > 94710cac0ef4ee177a63b5227664b38c95bbf703 $ git merge-base 5b394b2ddf0347bef56e50c69a58773c94343ff3 94710cac0ef4ee177a63b5227664b38c95bbf703 94710cac0ef4ee177a63b5227664b38c95bbf703 $ git log -1 --format=oneline 94710cac0ef4ee177a63b5227664b38c95bbf703 94710cac0ef4ee177a63b5227664b38c95bbf703 (tag: v4.18) Linux 4.18
Re: [BUG REPORT] Git does not correctly replay bisect log
On Tue, Dec 4, 2018 at 10:53 AM Lukáš Krejčí wrote: > > Executing git bisect replay reaches a different commit than > the one that is obtained by running the commands from the bisect log manually. > $ git bisect replay /var/tmp/git-bisect.log > We are not bisecting. > Bisecting: a merge base must be tested > [d72e90f33aa4709ebecc5005562f52335e106a60] Linux 4.18-rc6 Merge bases are tested only when the good commit is not an ancestor of the bad commit. If this didn't happen when the log was recorded, it shouldn't happen when it is replayed. Here it seems that this is happening at the beginning of the replay. Perhaps git bisect replay is taking into account the current branch/commit though it shouldn't. I wonder if this would happen if the current branch/commit has the good commit as an ancestor. Could you try to check that? And first could you give us the output of: git merge-base 5b394b2ddf0347bef56e50c69a58773c94343ff3 94710cac0ef4ee177a63b5227664b38c95bbf703
[BUG REPORT] Git does not correctly replay bisect log
Executing git bisect replay reaches a different commit than the one that is obtained by running the commands from the bisect log manually. Distribution: Arch Linux git: 2.19.2-1 perl: 5.28.1-1 pcre2: 10.32-1 expat: 2.2.6-1 perl-error: 0.17027-1 grep: 3.1-2 bash: 4.4.023-1 no system /etc/gitconfig is present tried with no ~/.gitconfig $ cat .git/config [core] repositoryformatversion = 0 filemode = true bare = false logallrefupdates = true [remote "origin"] url = git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git fetch = +refs/heads/*:refs/remotes/origin/* [branch "master"] remote = origin merge = refs/heads/master $ git fsck Checking object directories: 100% (256/256), done. warning in tag 5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 26791a8bcf0e6d33f43aef7682bdb555236d56de: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 9e734775f7c22d2f89943ad6c745571f1930105f: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 0397236d43e48e821cce5bbe6a80a1a56bb7cc3a: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag ebb5573ea8beaf000d4833735f3e53acb9af844c: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 06f6d9e2f140466eeb41e494e14167f90210f89d: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 701d7ecec3e0c6b4ab9bb824fd2b34be4da63b7e: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag 733ad933f62e82ebc92fed988c7f0795e64dea62: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag c521cb0f10ef2bf28a18e1cc8adf378ccbbe5a19: missingTaggerEntry: invalid format - expected 'tagger' line warning in tag a339981ec18d304f9efeb9ccf01b1f04302edf32: missingTaggerEntry: invalid format - expected 'tagger' line Checking objects: 100% (6428247/6428247), done. Checking connectivity: 6369862, done. $ cat /var/tmp/git-bisect.log git bisect start # bad: [5b394b2ddf0347bef56e50c69a58773c94343ff3] Linux 4.19-rc1 git bisect bad 5b394b2ddf0347bef56e50c69a58773c94343ff3 # good: [94710cac0ef4ee177a63b5227664b38c95bbf703] Linux 4.18 git bisect good 94710cac0ef4ee177a63b5227664b38c95bbf703 # bad: [54dbe75bbf1e189982516de179147208e90b5e45] Merge tag 'drm-next-2018-08-15' of git://anongit.freedesktop.org/drm/drm git bisect bad 54dbe75bbf1e189982516de179147208e90b5e45 # bad: [0a957467c5fd46142bc9c52758ffc552d4c5e2f7] x86: i8259: Add missing include file git bisect bad 0a957467c5fd46142bc9c52758ffc552d4c5e2f7 # good: [958f338e96f874a0d29442396d6adf9c1e17aa2d] Merge branch 'l1tf-final' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip git bisect good 958f338e96f874a0d29442396d6adf9c1e17aa2d # bad: [2c20443ec221dcb76484b30933593e8ecd836bbd] Merge tag 'acpi-4.19-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm git bisect bad 2c20443ec221dcb76484b30933593e8ecd836bbd # bad: [c2fc71c9b74c1e87336a27dba1a5edc69d2690f1] Merge tag 'mtd/for-4.19' of git://git.infradead.org/linux-mtd git bisect bad c2fc71c9b74c1e87336a27dba1a5edc69d2690f1 # bad: [b86d865cb1cae1e61527ea0b8977078bbf694328] blkcg: Make blkg_root_lookup() work for queues in bypass mode git bisect bad b86d865cb1cae1e61527ea0b8977078bbf694328 # bad: [1b0d274523df5ef1caedc834da055ff721e4d4f0] nvmet: don't use uuid_le type git bisect bad 1b0d274523df5ef1caedc834da055ff721e4d4f0 $ git status On branch master Your branch is up to date with 'origin/master'. nothing to commit, working tree clean $ git log -1 --format=oneline 2595646791c319cadfdbf271563aac97d0843dc7 (HEAD -> master, tag: v4.20-rc5, origin/master, origin/HEAD) Linux 4.20-rc5 $ git bisect replay /var/tmp/git-bisect.log We are not bisecting. Bisecting: a merge base must be tested [d72e90f33aa4709ebecc5005562f52335e106a60] Linux 4.18-rc6 $ git log -1 --format=oneline d72e90f33aa4709ebecc5005562f52335e106a60 (HEAD, tag: v4.18-rc6) Linux 4.18-rc6 Running the commands from the bisect log manually, however: $ git bisect reset Checking out files: 100% (18326/18326), done. Previous HEAD position was d72e90f33aa4 Linux 4.18-rc6 Switched to branch 'master' Your branch is up to date with 'origin/master'. $ . /var/tmp/git-bisect.log Bisecting: 6112 revisions left to test after this (roughly 13 steps) [54dbe75bbf1e189982516de179147208e90b5e45] Merge tag 'drm-next-2018-08-15' of git://anongit.freedesktop.org/drm/drm Bisecting: 3881 revisions left to test after this (roughly 12 steps) [0a957467c5fd46142bc9c52758ffc552d4c5e2f7] x86: i8259: Add missing include file Bisecting: 1595 revisions left to test after this (roughly 11 steps) [958f338e96f874a0d29442396d6adf9c1e17aa2d] Merge branch 'l1tf-final' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip Bisecting: 854 revisions left to test after this (roughly 10 steps) [2c20443ec221dcb76484b30933593e8ecd836bbd] Merge tag 'acpi-4.19-rc1' of
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Frank Schäfer writes: > Hi Junio, > > Am 29.11.18 um 03:11 schrieb Junio C Hamano: > [...] >> This was misspoken a bit. Let's revise it to >> >> When producing a colored output (not limited to whitespace >> error coloring of diff output) for contents that are not >> marked as eol=crlf (and other historical means), insert >> before a CR that comes immediately before a LF. > You mean > ... > *after* a CR that comes immediately before a LF." > > OK, AFAICS this produces the desired output in all cases if eol=lf. OK, yeah, I think I meant "after", i.e. ... CR LF, in order to force CR to be separated from LF. > Now what about the case eol=crlf ? I have no strong opinions, other than that "LF in repository, CRLF in working tree" would make the issue go away (when it is solved for EOL=LF like the above, that is). > Keeping the current behavior of '-' lines is correct. > But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ? If "LF in repository, CRLF in working tree" is done, there would not be ^M at the end of the line, not just for removed lines, but also for added lines, no?
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 02.12.18 um 20:31 schrieb Frank Schäfer: Am 29.11.18 um 03:11 schrieb Junio C Hamano: [...] This was misspoken a bit. Let's revise it to When producing a colored output (not limited to whitespace error coloring of diff output) for contents that are not marked as eol=crlf (and other historical means), insert before a CR that comes immediately before a LF. You mean ... *after* a CR that comes immediately before a LF." OK, AFAICS this produces the desired output in all cases if eol=lf. Now what about the case eol=crlf ? Keeping the current behavior of '-' lines is correct. But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ? That can be achieved with whitespace=cr-at-eol. With other words: "If CR comes immediately before a LF, do the following with *all* lines: after the CR if eol=lf but do not after the CR if eol=crlf." Why? It is the pager's duty to highlight CR, IMO. If it doesn't, but the user wants to see them, then they are using the wrong pager or the wrong pager settings. As far as I am concerned, I do not have any of my files marked as eol=crlf, but I do *not* want to see ^M in the pager. I.e., having git insert between CR and LF would do the wrong thing for me. -- Hannes
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Hi Junio, Am 29.11.18 um 03:11 schrieb Junio C Hamano: [...] > This was misspoken a bit. Let's revise it to > > When producing a colored output (not limited to whitespace > error coloring of diff output) for contents that are not > marked as eol=crlf (and other historical means), insert >before a CR that comes immediately before a LF. You mean ... *after* a CR that comes immediately before a LF." OK, AFAICS this produces the desired output in all cases if eol=lf. Now what about the case eol=crlf ? Keeping the current behavior of '-' lines is correct. But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ? With other words: "If CR comes immediately before a LF, do the following with *all* lines: after the CR if eol=lf but do not after the CR if eol=crlf." Regards, Frank
Re: [bug report] git-gui child windows are blank
Just checked gitk, it seems to work fine including children windows. This problem seems to affect git-gui only. On Thu, Nov 29, 2018 at 5:16 AM Eric Sunshine wrote: > > On Wed, Nov 28, 2018 at 2:29 PM Stefan Beller wrote: > > On Wed, Nov 28, 2018 at 6:13 AM Kenn Sebesta wrote: > > > v2.19.2, installed from brew on macOS Mojave 14.2.1. > > > > > > `git-gui` is my much beloved go-to tool for everything git. > > > Unfortunately, on my new Macbook Air it seems to have a bug. When I > > > first load the program, the parent window populates normally with the > > > stage/unstaged and diff panes. However, when I click Push, the child > > > window is completely blank. The frame is there, but there is no > > > content. > > > > Looking through the mailing list archive, this > > seemed to be one of the last relevant messges > > https://public-inbox.org/git/20180724065120.7664-1-sunsh...@sunshineco.com/ > > I was hoping that Junio would patch-monkey that one since that > crash-at-launch makes gitk unusable for Mojave users, but apparently > it never got picked up. > > Anyhow, the issue fixed by that patch has to do with 'osascript' on > Apple, and it's the only such invocation in the source code of > gitk/git-gui. The 'osascript' invocation merely attempts to foreground > the application at launch, so is almost certainly unrelated to the > above reported behavior. Somebody running Mojave will likely need to > spend some time debugging it. (Unfortunately, I'm a couple major > releases behind and don't plan on upgrading.)
Re: [bug report] git-gui child windows are blank
On Wed, Nov 28, 2018 at 2:29 PM Stefan Beller wrote: > On Wed, Nov 28, 2018 at 6:13 AM Kenn Sebesta wrote: > > v2.19.2, installed from brew on macOS Mojave 14.2.1. > > > > `git-gui` is my much beloved go-to tool for everything git. > > Unfortunately, on my new Macbook Air it seems to have a bug. When I > > first load the program, the parent window populates normally with the > > stage/unstaged and diff panes. However, when I click Push, the child > > window is completely blank. The frame is there, but there is no > > content. > > Looking through the mailing list archive, this > seemed to be one of the last relevant messges > https://public-inbox.org/git/20180724065120.7664-1-sunsh...@sunshineco.com/ I was hoping that Junio would patch-monkey that one since that crash-at-launch makes gitk unusable for Mojave users, but apparently it never got picked up. Anyhow, the issue fixed by that patch has to do with 'osascript' on Apple, and it's the only such invocation in the source code of gitk/git-gui. The 'osascript' invocation merely attempts to foreground the application at launch, so is almost certainly unrelated to the above reported behavior. Somebody running Mojave will likely need to spend some time debugging it. (Unfortunately, I'm a couple major releases behind and don't plan on upgrading.)
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Johannes Sixt writes: > Am 27.11.18 um 00:31 schrieb Junio C Hamano: >> Johannes Sixt writes: >>> Am 26.11.18 um 04:04 schrieb Junio C Hamano: >>> ... this goes too far, IMO. It is the pager's task to decode control >>> characters. >> >> It was tongue-in-cheek suggestion to split a CR into caret-em on our >> end, but we'd get essentially the same visual effect if we added a >> rule: >> >> When producing a colored output (not limited to whitespace >> error coloring of diff output), insert before a CR >> that comes immediately before a LF. This was misspoken a bit. Let's revise it to When producing a colored output (not limited to whitespace error coloring of diff output) for contents that are not marked as eol=crlf (and other historical means), insert before a CR that comes immediately before a LF. to exempt CR in CRLF sequence that the contents and the user agree to be part of the end-of-line marker. > I wouldn't want that to happen for all output (context lines, - lines, > + lines): I really am not interested to see all the CRs in my CRLF > files. So your CRLF files will not give caret-em. >> And a good thing is that I do not think that new rule is doing any >> decode of control chars on our end. We are just producing colored >> output normally. > > But we already have it, as Brian pointed out: > >git diff --ws-error-highlight=old,new > > or by setting diff.wsErrorHighlight accordingly. Not really. If the context lines end with CRLF and the material is not CRLF, I do not think CR at the end of them should be highlighted as whitespace errors (as we tell to detect errors on "old" and "new" lines), but I think we still should make an effort to help them be visible to the users. Otherwise, next Frank will come and complain after seeing -something some common thing +something_new^M With the suggested change, you can distinguish the following two (and use your imagination that there are many "some common thing" lines), which would help the user guide if the file should be marked as CRLF file, or the contents mistakenly has some lines that end with CRLF by mistake. The corrective action between the two cases would vastly be different. -something^M-something some common thing^M some common thing some common thing^M some common thing some common thing^M some common thing +something_new^M+something_new^M Without, both would look like the RHS of the illustration, and with the "highlight both", you'd only get an extra caret-em on the removed line, and need to judge without the help from common context lines. Besides, --ws-error-highlight shows all whitespace errors, and showing CR as caret-em is mere side effect of the interaction between its coloring and the pager.
Re: [BUG REPORT] t5322: demonstrate a pack-objects bug
On 11/28/2018 2:45 PM, Derrick Stolee wrote: I was preparing a new "sparse" algorithm for calculating the interesting objects to send on push. The important steps happen during 'git pack-objects', so I was creating test cases to see how the behavior changes in narrow cases. Specifically, when copying a directory across sibling directories (see test case), the new logic would accidentally send that object as an extra. However, I found a bug in the existing logic. The included test demonstrates this during the final 'git index-pack' call. It fails with the message 'fatal: pack has 1 unresolved delta' I realize now that I've gone through this effort that the problem is me (of course it is). + git pack-objects --stdout --thin --revs nonsparse.pack && Since I'm packing thin packs, the index operation is complaining about deltas. So, I need to use a different mechanism to list the objects in the pack (say, 'git verify-pack -v'). Sorry for the noise! Thanks, -Stolee
[BUG REPORT] t5322: demonstrate a pack-objects bug
I was preparing a new "sparse" algorithm for calculating the
interesting objects to send on push. The important steps happen
during 'git pack-objects', so I was creating test cases to see
how the behavior changes in narrow cases. Specifically, when
copying a directory across sibling directories (see test case),
the new logic would accidentally send that object as an extra.
However, I found a bug in the existing logic. The included test
demonstrates this during the final 'git index-pack' call. It
fails with the message
'fatal: pack has 1 unresolved delta'
It is probable that this is not a minimal test case, but happens
to be the test I had created before discovering the problem.
I compiled v2.17.0 and v2.12.0 as checks to see if I could find
a "good" commit with which to start a bisect, but both failed.
This is an old bug!
Signed-off-by: Derrick Stolee
---
t/t5322-pack-objects-sparse.sh | 94 ++
1 file changed, 94 insertions(+)
create mode 100755 t/t5322-pack-objects-sparse.sh
diff --git a/t/t5322-pack-objects-sparse.sh b/t/t5322-pack-objects-sparse.sh
new file mode 100755
index 00..36faa70fe9
--- /dev/null
+++ b/t/t5322-pack-objects-sparse.sh
@@ -0,0 +1,94 @@
+#!/bin/sh
+
+test_description='pack-objects object selection using sparse algorithm'
+. ./test-lib.sh
+
+test_expect_success 'setup repo' '
+ test_commit initial &&
+ for i in $(test_seq 1 3)
+ do
+ mkdir f$i &&
+ for j in $(test_seq 1 3)
+ do
+ mkdir f$i/f$j &&
+ echo $j >f$i/f$j/data.txt
+ done
+ done &&
+ git add . &&
+ git commit -m "Initialized trees" &&
+ for i in $(test_seq 1 3)
+ do
+ git checkout -b topic$i master &&
+ echo change-$i >f$i/f$i/data.txt &&
+ git commit -a -m "Changed f$i/f$i/data.txt"
+ done &&
+ cat >packinput.txt <<-EOF &&
+ topic1
+ ^topic2
+ ^topic3
+ EOF
+ git rev-parse \
+ topic1 \
+ topic1^{tree} \
+ topic1:f1 \
+ topic1:f1/f1\
+ topic1:f1/f1/data.txt | sort >actual_objects.txt
+'
+
+test_expect_success 'non-sparse pack-objects' '
+ git pack-objects --stdout --thin --revs nonsparse.pack
&&
+ git index-pack -o nonsparse.idx nonsparse.pack &&
+ git show-index nonsparse_objects.txt &&
+ test_cmp actual_objects.txt nonsparse_objects.txt
+'
+
+# Demonstrate that both algorithms send "extra" objects because
+# they are not in the frontier.
+
+test_expect_success 'duplicate a folder from f3 and commit to topic1' '
+ git checkout topic1 &&
+ echo change-3 >f3/f3/data.txt &&
+ git commit -a -m "Changed f3/f3/data.txt" &&
+ git rev-parse \
+ topic1~1\
+ topic1~1^{tree} \
+ topic1^{tree} \
+ topic1 \
+ topic1:f1 \
+ topic1:f1/f1\
+ topic1:f1/f1/data.txt \
+ topic1:f3 \
+ topic1:f3/f3\
+ topic1:f3/f3/data.txt | sort >actual_objects.txt
+'
+
+test_expect_success 'non-sparse pack-objects' '
+ git pack-objects --stdout --thin --revs nonsparse.pack
&&
+ git index-pack -o nonsparse.idx nonsparse.pack &&
+ git show-index nonsparse_objects.txt &&
+ test_cmp actual_objects.txt nonsparse_objects.txt
+'
+
+test_expect_success 'duplicate a folder from f3 and commit to topic1' '
+ mkdir f3/f4 &&
+ cp -r f1/f1/* f3/f4 &&
+ git add f3/f4 &&
+ git commit -m "Copied f1/f1 to f3/f4" &&
+ cat >packinput.txt <<-EOF &&
+ topic1
+ ^topic1~1
+ EOF
+ git rev-parse \
+ topic1 \
+ topic1^{tree} \
+ topic1:f3 | sort >actual_objects.txt
+'
+
+test_expect_success 'non-sparse pack-objects' '
+ git pack-objects --stdout --thin --revs nonsparse.pack
&&
+ git index-pack -o nonsparse.idx nonsparse.pack &&
+ git show-index nonsparse_objects.txt &&
+ test_cmp actual_objects.txt nonsparse_objects.txt
+'
+
+test_done
--
2.20.0.rc1
Re: [bug report] git-gui child windows are blank
On Wed, Nov 28, 2018 at 6:13 AM Kenn Sebesta wrote: > > v2.19.2, installed from brew on macOS Mojave 14.2.1. > > `git-gui` is my much beloved go-to tool for everything git. > Unfortunately, on my new Macbook Air it seems to have a bug. When I > first load the program, the parent window populates normally with the > stage/unstaged and diff panes. However, when I click Push, the child > window is completely blank. The frame is there, but there is no > content. > > This same behavior is seen if I do a `git gui blame`, where the > primary blame window opens normally but all the children windows are > empty. > > I have googled but was unsuccessful in finding a solution. Is this a > known issue? Looking through the mailing list archive, this seemed to be one of the last relevant messges https://public-inbox.org/git/20180724065120.7664-1-sunsh...@sunshineco.com/ > > > --Kenn
[bug report] git-gui child windows are blank
v2.19.2, installed from brew on macOS Mojave 14.2.1. `git-gui` is my much beloved go-to tool for everything git. Unfortunately, on my new Macbook Air it seems to have a bug. When I first load the program, the parent window populates normally with the stage/unstaged and diff panes. However, when I click Push, the child window is completely blank. The frame is there, but there is no content. This same behavior is seen if I do a `git gui blame`, where the primary blame window opens normally but all the children windows are empty. I have googled but was unsuccessful in finding a solution. Is this a known issue? --Kenn
Re: [PATCH] tests: avoid syntax triggering old dash bug
On Wed, Nov 28, 2018 at 01:47:41AM +, brian m. carlson wrote:
> On Tue, Nov 27, 2018 at 05:42:53PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > Avoid a bug in dash that's been fixed ever since its
> > ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> > released with dash v0.5.7 in July 2011.
> >
> > This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> > failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> > before URL encoding", 2016-02-09).
>
> Are people still using such versions of Debian? I only see wheezy (7)
> on the mirrors, not squeeze (6) or lenny (5). It might be better for us
> to encourage users to upgrade to an OS that has security support rather
> than work around bugs in obsolete OSes.
Yes, I have an old PowerPC box to test if code handle endians right.
And to ask people to upgrade does not conflict with supporting older
versions (if that is as easy as this patch).
I think we can have both.
Re: [PATCH] tests: avoid syntax triggering old dash bug
Eric Sunshine writes:
> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
> wrote:
>> Avoid a bug in dash that's been fixed ever since its
>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>> released with dash v0.5.7 in July 2011.
>
> Perhaps enhance the commit message to explain the nature of the bug
> itself. It is not at all obvious from reading the above or from
> looking at the diff itself what the actual problem is that the patch
> is fixing. (And it wasn't even immediately obvious by looking at the
> commit message of ec2c84d in the dash repository.) To help readers of
> this patch avoid re-introducing this problem or diagnose such a
> failure, it might be a good idea to give an example of the syntax
> which trips up old dash (i.e. a here-doc followed immediately by a
> {...} expression) and the actual error message 'Syntax error: "}"
> unexpected'.
Indeed. From the patch text, I would not have even guessed. I was
wondering if there were interactions with "" and $() inside it.
If having {...} immediately after a here-doc is a problem, then the
patch should not touch existing code at all, but instead insert a
new line, perhaps like
: avoid open brace immediately after here-doc for old dash
immediately before {...}; that would have made it easier to grok.
>@@ -892,8 +892,9 @@ test_expect_success 'get --expiry-date' '
> 1510348087
> 0
> EOF
>+ date_valid1=$(git config --expiry-date date.valid1) &&
> {
>- echo "$rel_out $(git config --expiry-date date.valid1)"
>+ echo "$rel_out $date_valid1"
> git config --expiry-date date.valid2 &&
> git config --expiry-date date.valid3 &&
> git config --expiry-date date.valid4 &&
Re: [PATCH] tests: avoid syntax triggering old dash bug
On Tue, Nov 27, 2018 at 05:42:53PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011.
>
> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> before URL encoding", 2016-02-09).
Are people still using such versions of Debian? I only see wheezy (7)
on the mirrors, not squeeze (6) or lenny (5). It might be better for us
to encourage users to upgrade to an OS that has security support rather
than work around bugs in obsolete OSes.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 27.11.18 um 19:15 schrieb Johannes Sixt: > Am 27.11.18 um 00:31 schrieb Junio C Hamano: >> Johannes Sixt writes: >>> Am 26.11.18 um 04:04 schrieb Junio C Hamano: >>> ... this goes too far, IMO. It is the pager's task to decode control >>> characters. >> >> It was tongue-in-cheek suggestion to split a CR into caret-em on our >> end, but we'd get essentially the same visual effect if we added a >> rule: >> >> When producing a colored output (not limited to whitespace >> error coloring of diff output), insert before a CR >> that comes immediately before a LF. >> >> Then, what Frank saw in the troublesome output would become >> >> -something CR LF >> +something_new CR LF >> >> and we'll let the existing pager+terminal magic turn that trailing >> CR on the preimage line into caret-em, just like the trailing CR on >> the postimage line is already shown as caret-em with the current >> output. > > I wouldn't want that to happen for all output (context lines, - lines, > + lines): I really am not interested to see all the CRs in my CRLF files. > >> And a good thing is that I do not think that new rule is doing any >> decode of control chars on our end. We are just producing colored >> output normally. > > But we already have it, as Brian pointed out: > > git diff --ws-error-highlight=old,new > > or by setting diff.wsErrorHighlight accordingly. ... but that also turns on displaying normal space/tab errors in removed lines. So to make both of us happy, we would need separate switches. Any chance this can be done easily enough ? Regards, Frank
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 27.11.18 um 00:31 schrieb Junio C Hamano: > Johannes Sixt writes: > >> Am 26.11.18 um 04:04 schrieb Junio C Hamano: >>> That does not sound right. I would understand it if both lines >>> showed ^M at the end, and only the one on the postimage line had it >>> highlighted as a trailing-whitespace. >> I agree that this is a (small?) weakness. But... >> >>> When we are producing a colored output, we know we are *not* writing >>> for machines, so one way to do it would be to turn CR at the end of >>> the line into two letter "^" "M" sequence on our end, without relying >>> on the terminal and/or the pager. I dunno. >> ... this goes too far, IMO. It is the pager's task to decode control >> characters. > It was tongue-in-cheek suggestion to split a CR into caret-em on our > end, but we'd get essentially the same visual effect if we added a > rule: > > When producing a colored output (not limited to whitespace > error coloring of diff output), insert before a CR > that comes immediately before a LF. > > Then, what Frank saw in the troublesome output would become > >-something CR LF >+something_new CR LF > > and we'll let the existing pager+terminal magic turn that trailing > CR on the preimage line into caret-em, just like the trailing CR on > the postimage line is already shown as caret-em with the current > output. > > And a good thing is that I do not think that new rule is doing any > decode of control chars on our end. We are just producing colored > output normally. Hmm... I think I now understand what caused the confusion here. It was my mistake: I didn't consider that EOL characters are whitespace characters, too. :/ Nevertheless, I still think that eol (CR, LF) and "normal" whitespace (space, tab) should be distinguished and marked/displayed differently, because they are playing different roles. Your suggestion seems to be a good solution for that. Regards, Frank
Re: [PATCH] tests: avoid syntax triggering old dash bug
On Tue, Nov 27 2018, Eric Sunshine wrote:
> On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
> wrote:
>> Avoid a bug in dash that's been fixed ever since its
>> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
>> released with dash v0.5.7 in July 2011.
>
> Perhaps enhance the commit message to explain the nature of the bug
> itself. It is not at all obvious from reading the above or from
> looking at the diff itself what the actual problem is that the patch
> is fixing. (And it wasn't even immediately obvious by looking at the
> commit message of ec2c84d in the dash repository.) To help readers of
> this patch avoid re-introducing this problem or diagnose such a
> failure, it might be a good idea to give an example of the syntax
> which trips up old dash (i.e. a here-doc followed immediately by a
> {...} expression) and the actual error message 'Syntax error: "}"
> unexpected'.
I haven't taken the time to understand the bug either. Our entire test
suite had one instance of this, so I think it's obscure enough that it's
fine to just fix it as a one-off and not spend any more time on making
sure it doesn't happen again or add some lint for detecting it.
>> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
>> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
>> before URL encoding", 2016-02-09).
>>
>> This particular test has been failing since 5f9674243d ("config: add
>> --expiry-date", 2017-11-18).
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason
Re: [PATCH] tests: avoid syntax triggering old dash bug
On Tue, Nov 27, 2018 at 11:43 AM Ævar Arnfjörð Bjarmason
wrote:
> Avoid a bug in dash that's been fixed ever since its
> ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
> released with dash v0.5.7 in July 2011.
Perhaps enhance the commit message to explain the nature of the bug
itself. It is not at all obvious from reading the above or from
looking at the diff itself what the actual problem is that the patch
is fixing. (And it wasn't even immediately obvious by looking at the
commit message of ec2c84d in the dash repository.) To help readers of
this patch avoid re-introducing this problem or diagnose such a
failure, it might be a good idea to give an example of the syntax
which trips up old dash (i.e. a here-doc followed immediately by a
{...} expression) and the actual error message 'Syntax error: "}"
unexpected'.
Thanks.
> This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
> failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
> before URL encoding", 2016-02-09).
>
> This particular test has been failing since 5f9674243d ("config: add
> --expiry-date", 2017-11-18).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 27.11.18 um 00:31 schrieb Junio C Hamano: Johannes Sixt writes: Am 26.11.18 um 04:04 schrieb Junio C Hamano: ... this goes too far, IMO. It is the pager's task to decode control characters. It was tongue-in-cheek suggestion to split a CR into caret-em on our end, but we'd get essentially the same visual effect if we added a rule: When producing a colored output (not limited to whitespace error coloring of diff output), insert before a CR that comes immediately before a LF. Then, what Frank saw in the troublesome output would become -something CR LF +something_new CR LF and we'll let the existing pager+terminal magic turn that trailing CR on the preimage line into caret-em, just like the trailing CR on the postimage line is already shown as caret-em with the current output. I wouldn't want that to happen for all output (context lines, - lines, + lines): I really am not interested to see all the CRs in my CRLF files. And a good thing is that I do not think that new rule is doing any decode of control chars on our end. We are just producing colored output normally. But we already have it, as Brian pointed out: git diff --ws-error-highlight=old,new or by setting diff.wsErrorHighlight accordingly. -- Hannes
[PATCH] tests: avoid syntax triggering old dash bug
Avoid a bug in dash that's been fixed ever since its
ec2c84d ("[PARSER] Fix clobbering of checkkwd", 2011-03-15)[1] first
released with dash v0.5.7 in July 2011.
This fixes 1/2 tests failing on Debian Lenny & Squeeze. The other
failure is due to 1b42f45255 ("git-svn: apply "svn.pathnameencoding"
before URL encoding", 2016-02-09).
This particular test has been failing since 5f9674243d ("config: add
--expiry-date", 2017-11-18).
1. https://git.kernel.org/pub/scm/utils/dash/dash.git/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
t/t1300-config.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 9652b241c7..7690b518b8 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -892,8 +892,9 @@ test_expect_success 'get --expiry-date' '
1510348087
0
EOF
+ date_valid1=$(git config --expiry-date date.valid1) &&
{
- echo "$rel_out $(git config --expiry-date date.valid1)"
+ echo "$rel_out $date_valid1"
git config --expiry-date date.valid2 &&
git config --expiry-date date.valid3 &&
git config --expiry-date date.valid4 &&
--
2.20.0.rc1.379.g1dd7ef354c
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Johannes Sixt writes: > Am 26.11.18 um 04:04 schrieb Junio C Hamano: >> >> That does not sound right. I would understand it if both lines >> showed ^M at the end, and only the one on the postimage line had it >> highlighted as a trailing-whitespace. > > I agree that this is a (small?) weakness. But... > >> When we are producing a colored output, we know we are *not* writing >> for machines, so one way to do it would be to turn CR at the end of >> the line into two letter "^" "M" sequence on our end, without relying >> on the terminal and/or the pager. I dunno. > > ... this goes too far, IMO. It is the pager's task to decode control > characters. It was tongue-in-cheek suggestion to split a CR into caret-em on our end, but we'd get essentially the same visual effect if we added a rule: When producing a colored output (not limited to whitespace error coloring of diff output), insert before a CR that comes immediately before a LF. Then, what Frank saw in the troublesome output would become -something CR LF +something_new CR LF and we'll let the existing pager+terminal magic turn that trailing CR on the preimage line into caret-em, just like the trailing CR on the postimage line is already shown as caret-em with the current output. And a good thing is that I do not think that new rule is doing any decode of control chars on our end. We are just producing colored output normally.
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 26.11.18 um 04:04 schrieb Junio C Hamano: It appears to me that what Frank sees is not "^M highlighted for whitespace breakage appears only on postimage lines, while ^M is shown but not highlighted on preimage lines". My reading of the above is "Not just without highlight, ^M is just *NOT* shown on the preimage line". That does not sound right. I would understand it if both lines showed ^M at the end, and only the one on the postimage line had it highlighted as a trailing-whitespace. I agree that this is a (small?) weakness. But... When we are producing a colored output, we know we are *not* writing for machines, so one way to do it would be to turn CR at the end of the line into two letter "^" "M" sequence on our end, without relying on the terminal and/or the pager. I dunno. ... this goes too far, IMO. It is the pager's task to decode control characters. -- Hannes
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Johannes Sixt writes: > But incorrect whitespace is never highlighted in removed lines, why > should CR be an exception? > ... > Same here for other cases, for example > > -something > +something > > will not have on obvious indicator that whitespace was corrected. All correct, but misses one point in Frank's original report, which observed -something +something_new^M with ^M highlighted for whitespace error. The highlighting is correct. But notice lack of caret-em on the preimage line? It turns out that we show something like this -something CR LF for the preimage line, while showing something like this +something_new CR LF for the postimage line. Because CR on the postimage line, thanks to highlighting, appears alone separate from the LF, it is shown as two-letter caret-em sequence to the user. On the other hand, because CR and LF appear next to each other on the preimage line, the pager and/or the terminal behaves as if CR is not even there and that is where Frank's complaint comes from, I think. The code is doing the right thing by showing CR, but it is hidden by the pager and/or the terminal.
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
On Sun, Nov 25, 2018 at 03:03:11PM +0100, Frank Schäfer wrote: > Am 24.11.18 um 23:07 schrieb Johannes Sixt: > > I don't think that there is anything to fix. If you have a file with > > CRLF in it, but you did not declare to Git that CRLF is the expected > > end-of-line indicator, then the CR *is* trailing whitespace (because > > the line ends at LF), and 'git diff' highlights it. > > Sure, it's correct to highlight it. > But it doesn't highlight it in removed lines, just in added lines. > I can see no good reason why removed and added lines should be treated > differently. The default behavior is to highlight whitespace errors only in new lines, because the assumption is that while you don't want to introduce new errors, you can't do anything about old mistakes without rewriting history. I agree that in many circumstances, such as code review, this may be undesirable. In the past, I've done code reviews where I may let existing trailing whitespace go but am strict about not introducing new trailing whitespace, and being able to see both is helpful. If you want to see whitespace errors in both the old and the new, use --ws-error-highlight or set diff.wsErrorHighlight. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 25.11.18 um 15:03 schrieb Frank Schäfer: Am 24.11.18 um 23:07 schrieb Johannes Sixt: I don't think that there is anything to fix. If you have a file with CRLF in it, but you did not declare to Git that CRLF is the expected end-of-line indicator, then the CR *is* trailing whitespace (because the line ends at LF), and 'git diff' highlights it. Sure, it's correct to highlight it. But it doesn't highlight it in removed lines, just in added lines. I can see no good reason why removed and added lines should be treated differently. But incorrect whitespace is never highlighted in removed lines, why should CR be an exception? 1) If CR+LF line termination is used in a file, changing the content of a line (but not its termination) currently produces a diff like -something +something_new^M which causes the user to think he has changed the line ending (added a CR) although he didn't. But this is not limited to CR at EOL: -something +something_new will also show the incorrect whitespace highlighted only for the + line. 2) If someone/something unintentionally changes the line termination from CR+LF to LF, it doesn't show up in the diff: -something +something Same here for other cases, for example -something +something will not have on obvious indicator that whitespace was corrected. If you are worried about a change in EOL style, you should better listen to your other tools. Either it is important, or it is not. If it is, they will report it to you. If it isn't, why care? -- Hannes
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 24.11.18 um 23:07 schrieb Johannes Sixt: > I don't think that there is anything to fix. If you have a file with > CRLF in it, but you did not declare to Git that CRLF is the expected > end-of-line indicator, then the CR *is* trailing whitespace (because > the line ends at LF), and 'git diff' highlights it. Sure, it's correct to highlight it. But it doesn't highlight it in removed lines, just in added lines. I can see no good reason why removed and added lines should be treated differently. Let me give you two real-life examples: 1) If CR+LF line termination is used in a file, changing the content of a line (but not its termination) currently produces a diff like -something +something_new^M which causes the user to think he has changed the line ending (added a CR) although he didn't. 2) If someone/something unintentionally changes the line termination from CR+LF to LF, it doesn't show up in the diff: -something +something I don't think this behavior makes sense. At least it's IMHO not what users expect to see. They want to see what's really going on, not to get confused. Regards, Frank
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 24.11.18 um 15:51 schrieb Frank Schäfer: Am 23.11.18 um 22:47 schrieb Johannes Sixt: Am 23.11.18 um 19:19 schrieb Frank Schäfer: The CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF. It shows up as expected in '-' lines when the ending of the removed line is CR only. It also always shows up as expected in '+' lines. Is your repository configured to (1) highlight whitespace errors in diff output and (2) to leave CRLF alone in text files? I'm using the default configuration, so whitespace is set to trailing-space, but cr-at-eol is not set. If so, then it is just a side-effect of this combination, an illusion, so to say: The CR in the CRLF combo is trailing whitespace. The 'git diff' marks it by inserting an escape sequence to switch the color before ^M and another escape sequence to reset to color after ^M. This breaks the CRLF combination apart, so that the pager does not process it as a combined CRLF sequence; it displays the lone CR as ^M. Urghh... so that needs to be fixed. Why does it work correctly with '+' lines ? I don't think that there is anything to fix. If you have a file with CRLF in it, but you did not declare to Git that CRLF is the expected end-of-line indicator, then the CR *is* trailing whitespace (because the line ends at LF), and 'git diff' highlights it. -- Hannes
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
On Sat, Nov 24, 2018 at 03:51:26PM +0100, Frank Schäfer wrote: [] > > Hmm... is CR-only line termination supported at all ? > E.g. 'eol' can be set to 'lf' or 'crlf' but not 'cr'... > No, CR-only is not supported, because: Nobody was implementing it, and that is probably because the only question abou CR-only (at least what I remember) was a when an old Mac OS (not the Mac OS X) was used (which used to use CR instead of LF). And, such feature may be implemented by writing a filter, replace CR with LF as "clean" and "LF" with "CR" for smudge.
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 23.11.18 um 22:47 schrieb Johannes Sixt: > Am 23.11.18 um 19:19 schrieb Frank Schäfer: >> The CR marker ^M doesn't show up in '-' lines of diffs when the ending >> of the removed line is CR+LF. >> It shows up as expected in '-' lines when the ending of the removed line >> is CR only. >> It also always shows up as expected in '+' lines. > > Is your repository configured to (1) highlight whitespace errors in > diff output and (2) to leave CRLF alone in text files? I'm using the default configuration, so whitespace is set to trailing-space, but cr-at-eol is not set. > > If so, then it is just a side-effect of this combination, an illusion, > so to say: The CR in the CRLF combo is trailing whitespace. The 'git > diff' marks it by inserting an escape sequence to switch the color > before ^M and another escape sequence to reset to color after ^M. This > breaks the CRLF combination apart, so that the pager does not process > it as a combined CRLF sequence; it displays the lone CR as ^M. Urghh... so that needs to be fixed. Why does it work correctly with '+' lines ? > > It is easy to achieve the opposite effect, i.e., that ^M is not > displayed. For example with these lines in .git/info/attributes or > .gitattributes: > > *.cpp > whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab > *.h > whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab > > Note the cr-at-eol. (There may be shorter versions to achieve a > similar effect.) With cr-at-eol, ^M indeed doesn't show up anymore in '+' lines with CR+LF line endings. That's correct/expected. '-' lines with CR+LF line endings are displayed correctly in this case, too. However, ^M still shows up in '+' and '-' lines with CR line endings. Hmm... is CR-only line termination supported at all ? E.g. 'eol' can be set to 'lf' or 'crlf' but not 'cr'... Regards, Frank > > -- Hannes
Re: [PATCH v4 0/2] Fix scissors bug during merge conflict
Denton Liu writes: > I just realised that there is a slight problem with the proposed change. > When we do a merge and there are no merge conflicts, at the end of the > merge, we get dropped into an editor with this text: > > Merge branch 'master' into new > > # Please enter a commit message to explain why this merge is necessary, > # especially if it merges an updated upstream into a topic branch. > # > # Lines starting with '#' will be ignored, and an empty message aborts > # the commit. > > Note that in git-merge, the cleanup only removes commented lines and > this cannot be configured to be scissors or whatever else. I think that > the fact that it's not configurable isn't a problem; most hardcore > commit message editing happens in git-commit anyway. OK. > However, since we taught git-merge the --cleanup option, this might be > misleading for the end-user since they would expect the MERGE_MSG to be > cleaned up as specified. > > I see two resolutions for this. We can either rename --cleanup more > precisely so users won't be confused (perhaps something like > --merge-conflict-scissors but a lot more snappy) or we can actually make > git-merge respect the cleanup option and post-process the message > according to the specified cleanup rule. The former certainly would be simpler to implement, but feels more like an excuse for not doing the right thing to me, when I put myself in shoes of users who use 'scissors' clean-up option in commit. I dunno.
Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
Am 23.11.18 um 19:19 schrieb Frank Schäfer: The CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF. It shows up as expected in '-' lines when the ending of the removed line is CR only. It also always shows up as expected in '+' lines. Is your repository configured to (1) highlight whitespace errors in diff output and (2) to leave CRLF alone in text files? If so, then it is just a side-effect of this combination, an illusion, so to say: The CR in the CRLF combo is trailing whitespace. The 'git diff' marks it by inserting an escape sequence to switch the color before ^M and another escape sequence to reset to color after ^M. This breaks the CRLF combination apart, so that the pager does not process it as a combined CRLF sequence; it displays the lone CR as ^M. It is easy to achieve the opposite effect, i.e., that ^M is not displayed. For example with these lines in .git/info/attributes or .gitattributes: *.cpp whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab *.h whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab Note the cr-at-eol. (There may be shorter versions to achieve a similar effect.) -- Hannes
BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF
The CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF. It shows up as expected in '-' lines when the ending of the removed line is CR only. It also always shows up as expected in '+' lines. These are the diffs of the 6 possible line ending changes: LF to CR+LF: @@ -1,2 +1,2 @@ -aaa +aaa^M bbb CR+LF to LF: @@ -1,2 +1,2 @@ -aaa => BUG: should be -aaa^M +aaa bbb CR to CR+LF: @@ -1 +1,2 @@ -aaa^Mbbb +aaa^M +bbb CR+LF to CR: @@ -1,2 +1 @@ -aaa => BUG: should be -aaa^M -bbb +aaa^Mbbb LF to CR: @@ -1,2 +1 @@ -aaa -bbb +aaa^Mbbb CR to LF: @@ -1 +1,2 @@ -aaa^Mbbb +aaa +bbb Tested with version 2.19.1. Regards, Frank Schäfer
Re: [PATCH v4 0/2] Fix scissors bug during merge conflict
On Wed, Nov 21, 2018 at 06:38:20PM +0900, Junio C Hamano wrote: > Denton Liu writes: > > > Changes since V3: > > * Add patch to cleanup 'merge --squash c3 with c7' test in t7600 > > * Use test_i18ncmp instead of test_cmp to pass GETTEXT_POISON tests > > Queued. Thanks, both. I just realised that there is a slight problem with the proposed change. When we do a merge and there are no merge conflicts, at the end of the merge, we get dropped into an editor with this text: Merge branch 'master' into new # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit. Note that in git-merge, the cleanup only removes commented lines and this cannot be configured to be scissors or whatever else. I think that the fact that it's not configurable isn't a problem; most hardcore commit message editing happens in git-commit anyway. However, since we taught git-merge the --cleanup option, this might be misleading for the end-user since they would expect the MERGE_MSG to be cleaned up as specified. I see two resolutions for this. We can either rename --cleanup more precisely so users won't be confused (perhaps something like --merge-conflict-scissors but a lot more snappy) or we can actually make git-merge respect the cleanup option and post-process the message according to the specified cleanup rule. I would personally think the first option is better than the second but I'd like to hear your thoughts. Thanks!
Re: [PATCH v4 0/2] Fix scissors bug during merge conflict
Denton Liu writes: > Changes since V3: > * Add patch to cleanup 'merge --squash c3 with c7' test in t7600 > * Use test_i18ncmp instead of test_cmp to pass GETTEXT_POISON tests Queued. Thanks, both.
[PATCH v4 0/2] Fix scissors bug during merge conflict
Thanks for catching that, Szeder. I tested this revised patch under GETTEXT_POISON and it should be passing tests now. Changes since V1: * Only check MERGE_MSG for a scissors line instead of all prepended files * Make a variable static in merge where appropriate * Add passthrough options in pull * Add documentation for the new option * Add tests to ensure desired behaviour Changes since V2: * Merge both patches into one patch * Fix bug in help message printing logic Changes since V3: * Add patch to cleanup 'merge --squash c3 with c7' test in t7600 * Use test_i18ncmp instead of test_cmp to pass GETTEXT_POISON tests Denton Liu (2): t7600: clean up 'merge --squash c3 with c7' test merge: add scissors line on merge conflict Documentation/merge-options.txt | 6 builtin/commit.c| 20 + builtin/merge.c | 16 ++ builtin/pull.c | 6 t/t7600-merge.sh| 53 ++--- 5 files changed, 92 insertions(+), 9 deletions(-) -- 2.19.1
Re: [PATCH] tests: send "bug in the test script" errors to the script's stderr
On Tue, Nov 20, 2018 at 05:58:25AM -0500, Jeff King wrote: > On Tue, Nov 20, 2018 at 05:45:28AM -0500, Jeff King wrote: > > > On Tue, Nov 20, 2018 at 12:34:04AM +0100, SZEDER Gábor wrote: > > > > > > I do notice that many of the existing "FATAL:" errors use descriptor 5, > > > > which goes to stdout. I'm not sure if those should actually be going to > > > > stderr (or if there's some TAP significance to those lines). > > > > > > I chose to send these messages to standard error, because they are, > > > well, errors. TAP only cares about stdout, but by aborting the test > > > script in BUG(), error() or die() we are already violating TAP anyway, > > > so I don't think it matters whether we send "bug in test script" or > > > "FATAL" errors to stdout or stderr. > > > > Yeah, I agree it probably doesn't matter. I was mostly suggesting to > > make the existing ">&5" into ">&7" for consistency. But I don't think > > that needs to block your patch. > > By the way, I did check to see whether this might help the situation > where running under "prove" we see only "Dubious..." and not the actual > error() message produced by the test script. But no, it eats both stdout > and stderr. You can sneak a line through by prepending it with "#", but > only if "prove -o" is used (and even then, it's hard to associate it > with a particular test when you're running many in parallel). Just to be clear: I don't mind if in some combination of test harnesses and test options a "bug in the test script" message doesn't reach the terminal as long as I get a clearly visible error from somewhere. > So I guess the status quo is not too bad: prove says "dubious", and then > you re-run the test with "./t1234-foo.sh -v -i" and you get to see the > error. And with '--verbose-log' the "bug in the test script" message goes to the test's log as well, even when it has to go through fd 7 first, so if you use 'prove' and your GIT_TEST_OPTS includes '--verbose-log' then you can just look at the log, there's no need to re-run the test.
Re: [PATCH] tests: send "bug in the test script" errors to the script's stderr
On Tue, Nov 20, 2018 at 05:45:28AM -0500, Jeff King wrote: > On Tue, Nov 20, 2018 at 12:34:04AM +0100, SZEDER Gábor wrote: > > > > I do notice that many of the existing "FATAL:" errors use descriptor 5, > > > which goes to stdout. I'm not sure if those should actually be going to > > > stderr (or if there's some TAP significance to those lines). > > > > I chose to send these messages to standard error, because they are, > > well, errors. TAP only cares about stdout, but by aborting the test > > script in BUG(), error() or die() we are already violating TAP anyway, > > so I don't think it matters whether we send "bug in test script" or > > "FATAL" errors to stdout or stderr. > > Yeah, I agree it probably doesn't matter. I was mostly suggesting to > make the existing ">&5" into ">&7" for consistency. But I don't think > that needs to block your patch. By the way, I did check to see whether this might help the situation where running under "prove" we see only "Dubious..." and not the actual error() message produced by the test script. But no, it eats both stdout and stderr. You can sneak a line through by prepending it with "#", but only if "prove -o" is used (and even then, it's hard to associate it with a particular test when you're running many in parallel). So I guess the status quo is not too bad: prove says "dubious", and then you re-run the test with "./t1234-foo.sh -v -i" and you get to see the error. -Peff
Re: [PATCH] tests: send "bug in the test script" errors to the script's stderr
On Tue, Nov 20, 2018 at 12:34:04AM +0100, SZEDER Gábor wrote: > > I do notice that many of the existing "FATAL:" errors use descriptor 5, > > which goes to stdout. I'm not sure if those should actually be going to > > stderr (or if there's some TAP significance to those lines). > > I chose to send these messages to standard error, because they are, > well, errors. TAP only cares about stdout, but by aborting the test > script in BUG(), error() or die() we are already violating TAP anyway, > so I don't think it matters whether we send "bug in test script" or > "FATAL" errors to stdout or stderr. Yeah, I agree it probably doesn't matter. I was mostly suggesting to make the existing ">&5" into ">&7" for consistency. But I don't think that needs to block your patch. > BTW, TAP understands "Bail out!" as bail out from the _entire_ test > suite, but that's not what we want here, I'd think. > > https://testanything.org/tap-specification.html#bail-out Yes, I added the only existing "Bail out!" to test-lib.sh. :) I agree that's not what we want here. I actually think the current behavior (to exit non-zero) does what we want. A TAP harness will realize that's an error, but not block other scripts. -Peff
Re: [PATCH] tests: send "bug in the test script" errors to the script's stderr
On Mon, Nov 19, 2018 at 02:44:32PM -0500, Jeff King wrote:
> On Mon, Nov 19, 2018 at 02:13:26PM +0100, SZEDER Gábor wrote:
>
> > Send these "bug in the test script" error messages directly to the
> > test scripts standard error and thus to the terminal, so those bugs
> > will be much harder to overlook. Instead of updating all ~20 such
> > 'error' calls with a redirection, let's add a BUG() function to
> > 'test-lib.sh', wrapping an 'error' call with the proper redirection
> > and also including the common prefix of those error messages, and
> > convert all those call sites [4] to use this new BUG() function
> > instead.
>
> Yes, I think this is an improvement.
>
> > +BUG () {
> > + error >&7 "bug in the test script: $*"
> > +}
>
> I naively expected this to go to >&4, but of course that is the
> conditional "stderr or /dev/null, depending on --verbose" descriptor.
The first version of this patch did send the error message to fd 4 ;)
> I do notice that many of the existing "FATAL:" errors use descriptor 5,
> which goes to stdout. I'm not sure if those should actually be going to
> stderr (or if there's some TAP significance to those lines).
I chose to send these messages to standard error, because they are,
well, errors. TAP only cares about stdout, but by aborting the test
script in BUG(), error() or die() we are already violating TAP anyway,
so I don't think it matters whether we send "bug in test script" or
"FATAL" errors to stdout or stderr.
BTW, TAP understands "Bail out!" as bail out from the _entire_ test
suite, but that's not what we want here, I'd think.
https://testanything.org/tap-specification.html#bail-out
Re: [PATCH] tests: send "bug in the test script" errors to the script's stderr
On Mon, Nov 19, 2018 at 02:13:26PM +0100, SZEDER Gábor wrote:
> Send these "bug in the test script" error messages directly to the
> test scripts standard error and thus to the terminal, so those bugs
> will be much harder to overlook. Instead of updating all ~20 such
> 'error' calls with a redirection, let's add a BUG() function to
> 'test-lib.sh', wrapping an 'error' call with the proper redirection
> and also including the common prefix of those error messages, and
> convert all those call sites [4] to use this new BUG() function
> instead.
Yes, I think this is an improvement.
> +BUG () {
> + error >&7 "bug in the test script: $*"
> +}
I naively expected this to go to >&4, but of course that is the
conditional "stderr or /dev/null, depending on --verbose" descriptor. I
have a feeling that we could get rid of descriptors 5 and 7 in favor of
3 and 4, if we did the conditional redirection when running each test,
instead of ahead of time.
But unless we are running out of descriptors, it's not worth the effort
(it's debatable whether we are; 9be795fbce (t5615: avoid re-using
descriptor 4, 2017-12-08) made me nervous, but it's more about the
special-ness of BASHE_XTRACEFD than anything).
Anyway, that's all a tangent to your patch.
I do notice that many of the existing "FATAL:" errors use descriptor 5,
which goes to stdout. I'm not sure if those should actually be going to
stderr (or if there's some TAP significance to those lines).
-Peff
[PATCH] tests: send "bug in the test script" errors to the script's stderr
Some of the functions in our test library check that they were invoked
properly with conditions like this:
test "$#" = 2 ||
error "bug in the test script: not 2 parameters to test-expect-success"
If this particular condition is triggered, then 'error' will abort the
whole test script with a bold red error message [1] right away.
However, under certain circumstances the test script will be aborted
completely silently, namely if:
- a similar condition in a test helper function like
'test_line_count' is triggered,
- which is invoked from the test script's "main" shell [2],
- and the test script is run manually (i.e. './t1234-foo.sh' as
opposed to 'make t1234-foo.sh' or 'make test') [3]
- and without the '--verbose' option,
because the error message is printed from within 'test_eval_', where
standard output is redirected either to /dev/null or to a log file.
The only indication that something is wrong is that not all tests in
the script are executed and at the end of the test script's output
there is no "# passed all N tests" message, which are subtle and can
easily go unnoticed, as I had to experience myself.
Send these "bug in the test script" error messages directly to the
test scripts standard error and thus to the terminal, so those bugs
will be much harder to overlook. Instead of updating all ~20 such
'error' calls with a redirection, let's add a BUG() function to
'test-lib.sh', wrapping an 'error' call with the proper redirection
and also including the common prefix of those error messages, and
convert all those call sites [4] to use this new BUG() function
instead.
[1] That particular error message from 'test_expect_success' is
printed in color only when running with or without '--verbose';
with '--tee' or '--verbose-log' the error is printed without
color, but it is printed to the terminal nonetheless.
[2] If such a condition is triggered in a subshell of a test, then
'error' won't be able to abort the whole test script, but only the
subshell, which in turn causes the test to fail in the usual way,
indicating loudly and clearly that something is wrong.
[3] Well, 'error' aborts the test script the same way when run
manually or by 'make' or 'prove', but both 'make' and 'prove' pay
attention to the test script's exit status, and even a silently
aborted test script would then trigger those tools' usual
noticable error messages.
[4] Strictly speaking, not all those 'error' calls need that
redirection to send their output to the terminal, see e.g.
'test_expect_success' in the opening example, but I think it's
better to be consistent.
Signed-off-by: SZEDER Gábor
---
t/perf/perf-lib.sh | 4 ++--
t/t0001-init.sh | 4 ++--
t/t4013-diff-various.sh | 2 +-
t/t5516-fetch-push.sh | 2 +-
t/t9902-completion.sh | 2 +-
t/test-lib-functions.sh | 25 -
t/test-lib.sh | 10 +++---
7 files changed, 26 insertions(+), 23 deletions(-)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 11d1922cf5..2e33ab3ec3 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -82,7 +82,7 @@ test_perf_do_repo_symlink_config_ () {
test_perf_create_repo_from () {
test "$#" = 2 ||
- error "bug in the test script: not 2 parameters to test-create-repo"
+ BUG "not 2 parameters to test-create-repo"
repo="$1"
source="$2"
source_git="$("$MODERN_GIT" -C "$source" rev-parse --git-dir)"
@@ -184,7 +184,7 @@ test_wrapper_ () {
test_start_
test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
test "$#" = 2 ||
- error "bug in the test script: not 2 or 3 parameters to
test-expect-success"
+ BUG "not 2 or 3 parameters to test-expect-success"
export test_prereq
if ! test_skip "$@"
then
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 182da069f1..42a263cada 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -319,14 +319,14 @@ test_lazy_prereq GETCWD_IGNORES_PERMS '
base=GETCWD_TEST_BASE_DIR &&
mkdir -p $base/dir &&
chmod 100 $base ||
- error "bug in test script: cannot prepare $base"
+ BUG "cannot prepare $base"
(cd $base/dir && /bin/pwd -P)
status=$?
chmod 700 $base &&
rm -rf $base ||
- error "bug in test script: cannot clean $base"
+ BUG "cannot clean $base"
return $status
'
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 73f7038253..7d985ff6b1 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -129,7 +129,7 @@ do
case "$magic" in
noellipses) ;;
Re: [PATCH v3 0/1] Fix scissors bug during merge conflict
Denton Liu writes:
>> I wonder if this is what you really meant to have instead:
>>
>> >else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
>> > - whence == FROM_COMMIT)
>> > - wt_status_add_cut_line(s->fp);
>> > + whence == FROM_COMMIT) {
>> > + if (!merge_contains_scissors)
>> > + wt_status_add_cut_line(s->fp);
>> > + }
>> >else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
>> >status_printf(s, GIT_COLOR_NORMAL,
>>
>> That is, the only behaviour change in "merge contains scissors"
>> mode is to omit the cut line and nothing else.
>
> Thanks for catching this. I noticed that the originally behaviour is
> buggy too: in the case where we're merging a commit and scissors are
> printed from the `if (whence != FROM_COMMIT)` block, the original
> behaviour would drop us into the else (COMMIT_MSG_CLEANUP_SPACE)
> statement, which is undesired.
The original calls add-cut-line in the "whence != FROM_COMMIT" when
cleanup_mode is CLEANUP_SCISSORS (and then in that block it also adds
the message about committing a merge or cherry-pick). After that,
the original does the three-arm if/else if/else cascade and we end
up showing the "lines starting with # will be kept" message.
Yeah, you read the original correctly and I agree that in that block
the right thing to do is not to say anything in CLEANUP_SCISSORS
mode.
Thanks for carefully reading the code again.
[PATCH v3 0/1] Fix scissors bug during merge conflict
On Sat, Nov 17, 2018 at 05:06:43PM +0900, Junio C Hamano wrote:
> Are we already sometimes adding a scissors line in that file? The
> impression I was getting was that the change in the step 2/2 is the
> only reason why this becomes necessary. In which case this change
> makes no sense as a standalone patch and requires 2/2 to be a
> sensible change, no?
>
My mistake, I guess I went a little overboard trying to split my
contribution into digestable patches.
> > @@ -798,7 +807,8 @@ static int prepare_to_commit(const char *index_file,
> > const char *prefix,
> > struct ident_split ci, ai;
> >
> > if (whence != FROM_COMMIT) {
> > - if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
> > + if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> > + !merge_contains_scissors)
> > wt_status_add_cut_line(s->fp);
> > status_printf_ln(s, GIT_COLOR_NORMAL,
> > whence == FROM_MERGE
>
> This one is done before we show a block of text, which looks correct.
>
> > @@ -824,7 +834,8 @@ static int prepare_to_commit(const char *index_file,
> > const char *prefix,
> > " Lines starting\nwith '%c' will be ignored,
> > and an empty"
> > " message aborts the commit.\n"),
> > comment_line_char);
> > else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> > -whence == FROM_COMMIT)
> > +whence == FROM_COMMIT &&
> > +!merge_contains_scissors)
> > wt_status_add_cut_line(s->fp);
> > else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> > status_printf(s, GIT_COLOR_NORMAL,
>
> The correctness of this one in an if/elseif/else cascade is less
> clear. When "contains scissors" logic does not kick in, we just
> have a scissors line here (i.e. the original behaviour). Now when
> the new logic kicks in, we not just omit the (extra) scissors line,
> but also say "Please enter the commit message..." which is the
> message used with the "comment line char" mode of the clean-up.
>
> I wonder if this is what you really meant to have instead:
>
> > else if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS &&
> > -whence == FROM_COMMIT)
> > - wt_status_add_cut_line(s->fp);
> > +whence == FROM_COMMIT) {
> > +if (!merge_contains_scissors)
> > + wt_status_add_cut_line(s->fp);
> > + }
> > else /* COMMIT_MSG_CLEANUP_SPACE, that is. */
> > status_printf(s, GIT_COLOR_NORMAL,
>
> That is, the only behaviour change in "merge contains scissors"
> mode is to omit the cut line and nothing else.
Thanks for catching this. I noticed that the originally behaviour is
buggy too: in the case where we're merging a commit and scissors are
printed from the `if (whence != FROM_COMMIT)` block, the original
behaviour would drop us into the else (COMMIT_MSG_CLEANUP_SPACE)
statement, which is undesired. With this fix, the message about `#`
_not_ being removed is now silenced in both cases.
Changes since V1:
* Only check MERGE_MSG for a scissors line instead of all prepended
files
* Make a variable static in merge where appropriate
* Add passthrough options in pull
* Add documentation for the new option
* Add tests to ensure desired behaviour
Changes since V2:
* Merge both patches into one patch
* Fix bug in help message printing logic
Denton Liu (1):
merge: add scissors line on merge conflict
Documentation/merge-options.txt | 6 +
builtin/commit.c| 20 ++
builtin/merge.c | 16 +++
builtin/pull.c | 6 +
t/t7600-merge.sh| 48 +
5 files changed, 91 insertions(+), 5 deletions(-)
--
2.19.1
[PATCH v2 0/2] Fix scissors bug during merge conflict
Thanks for your feedback, Junio. I tried to reroll the patch by adding another option into the MERGE_MODE file but unfortunately, it didn't work completely because doing `merge --squash` doesn't produce a MERGE_MODE. In addition to this, because of the way merge and commit were structured, I needed to reorder a lot of calls because some variables were only being set after I needed them. Unless we want to produce a MERGE_MODE during --squash (which I don't think is worth it) I don't think that this is the way to go. Instead, I just refined my first approach and only checked the contents of MERGE_MSG for a scissors line. The MERGE_MSG is going to be machine-generated anyway so we should be safe from accidentally ignoring a human-placed scissors line. Changes since V1: - * Only check MERGE_MSG for a scissors line instead of all prepended files * Make a variable static in merge where appropriate * Add passthrough options in pull * Add documentation for the new option * Add tests to ensure desired behaviour Denton Liu (2): commit: don't add scissors line if one exists merge: add scissors line on merge conflict Documentation/merge-options.txt | 6 + builtin/commit.c| 15 +-- builtin/merge.c | 16 +++ builtin/pull.c | 6 + t/t7600-merge.sh| 48 + 5 files changed, 89 insertions(+), 2 deletions(-) -- 2.19.1
Re: [PATCH v2 1/1] config: report a bug if git_dir exists without commondir
On Wed, Nov 14, 2018 at 05:59:02AM -0800, Johannes Schindelin via GitGitGadget
wrote:
> From: Johannes Schindelin
>
> This did happen at some stage, and was fixed relatively quickly. Make
> sure that we detect very quickly, too, should that happen again.
>
> Signed-off-by: Johannes Schindelin
> ---
> config.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/config.c b/config.c
> index 4051e38823..db6b0167c6 100644
> --- a/config.c
> +++ b/config.c
> @@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct
> config_options *opts,
>
> if (opts->commondir)
> repo_config = mkpathdup("%s/config", opts->commondir);
> + else if (opts->git_dir)
> + BUG("git_dir without commondir");
Yeah, I think this is the right thing to do. Thanks!
-Peff
[PATCH v2 1/1] config: report a bug if git_dir exists without commondir
From: Johannes Schindelin
This did happen at some stage, and was fixed relatively quickly. Make
sure that we detect very quickly, too, should that happen again.
Signed-off-by: Johannes Schindelin
---
config.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/config.c b/config.c
index 4051e38823..db6b0167c6 100644
--- a/config.c
+++ b/config.c
@@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct
config_options *opts,
if (opts->commondir)
repo_config = mkpathdup("%s/config", opts->commondir);
+ else if (opts->git_dir)
+ BUG("git_dir without commondir");
else
repo_config = NULL;
--
gitgitgadget
Re: [RFC PATCH 0/2] Fix scissors bug during merge conflict
On Wed, Nov 14, 2018 at 04:52:59PM +0900, Junio C Hamano wrote: > Denton Liu writes: > > > With this fix, the message becomes the following: > > > > Merge branch 'master' into new > > > > # >8 > > # Do not modify or remove the line above. > > # Everything below it will be ignored. > > # > > # Conflicts: > > # a > > I have a mixed feeling about this change and I certainly would not > call it a "fix". > > The reason why we give the list of conflicted paths that is > commented out is to remind the user of them in order to help them > describe what area of the codebase had overlapping changes, why, and > how the overlap was resolved, which is relevant information when > somebody else later needs to dig into the history to understand how > the code came into today's shape and why. For that reason, if a > careless user left conflicts list behind without describing these > details about the merge, it might be better to have the unexplained > list in the merge than nothing. > The reason why I implemented it this way is because the default cleanup setting (strip) produces this message: Merge branch 'master' into new # Conflicts: # a # # It looks like you may be committing a merge. # If this is not correct, please remove the file # .git/MERGE_HEAD # and try again. # Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # On branch new # All conflicts fixed but you are still merging. # # Changes to be committed: # modified: a # Which makes it seem like the `Conflicts:` section should be removed by default. > In theory, the above argument applies the same way for the paths to > be committed, but the list is fairly trivial to recreate with "git > diff $it^!", unlike "which paths had conflict", which can only be > found out by recreating the auto-merge. So in practice, the paths > that had conflicts is more worth showing than the paths that were > modified. > > So, I dunno. If we value the "more expensive list to reproduce", > the fix might be not to place it (and possibly the comments and > everything under the scissors line) behind a "# " comment char on > the line, without moving its position. If I understood correctly, then I have no strong opinions between uncommenting the Conflicts section by default and this change; I just think it'd be nice to have behaviour that's consistent. > > . > >
Re: [RFC PATCH 0/2] Fix scissors bug during merge conflict
Denton Liu writes: > With this fix, the message becomes the following: > > Merge branch 'master' into new > > # >8 > # Do not modify or remove the line above. > # Everything below it will be ignored. > # > # Conflicts: > # a I have a mixed feeling about this change and I certainly would not call it a "fix". The reason why we give the list of conflicted paths that is commented out is to remind the user of them in order to help them describe what area of the codebase had overlapping changes, why, and how the overlap was resolved, which is relevant information when somebody else later needs to dig into the history to understand how the code came into today's shape and why. For that reason, if a careless user left conflicts list behind without describing these details about the merge, it might be better to have the unexplained list in the merge than nothing. In theory, the above argument applies the same way for the paths to be committed, but the list is fairly trivial to recreate with "git diff $it^!", unlike "which paths had conflict", which can only be found out by recreating the auto-merge. So in practice, the paths that had conflicts is more worth showing than the paths that were modified. So, I dunno. If we value the "more expensive list to reproduce", the fix might be not to place it (and possibly the comments and everything under the scissors line) behind a "# " comment char on the line, without moving its position. .
[RFC PATCH 0/2] Fix scissors bug during merge conflict
I discovered a bug in Git a while ago where if one is using commit.cleanup = scissors, if making a commit after a merge conflict, the scissors line will be placed after the `Conflicts:` section. As a result, a careless Git user (e.g. me) may accidentally commit the `Conflicts:` section. Here is a test case to replicate the behaviour: mkdir test cd test/ git init git config commit.cleanup scissors touch a git add a git commit -m 'test' echo a > a git commit -am 'test2' git checkout -b new HEAD^ echo b > a git commit -am 'test3' git merge master echo c > a git add a git commit # look at the commit message here And the resulting message that's sent to the text editor: Merge branch 'master' into new # Conflicts: # a # >8 # Do not modify or remove the line above. # Everything below it will be ignored. # # It looks like you may be committing a merge. # If this is not correct, please remove the file # .git/MERGE_HEAD # and try again. # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # On branch new # All conflicts fixed but you are still merging. # # Changes to be committed: # modified: a # With this fix, the message becomes the following: Merge branch 'master' into new # >8 # Do not modify or remove the line above. # Everything below it will be ignored. # # Conflicts: # a # # It looks like you may be committing a merge. # If this is not correct, please remove the file # .git/MERGE_HEAD # and try again. # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # On branch new # All conflicts fixed but you are still merging. # # Changes to be committed: # modified: a # Let me know what you think of the change. Of course, documentation and testing will come after this leaves the RFC phase. Denton Liu (2): commit: don't add scissors line if one exists merge: add scissors line on merge conflict builtin/commit.c | 11 +-- builtin/merge.c | 16 2 files changed, 25 insertions(+), 2 deletions(-) -- 2.19.1
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
Johannes Schindelin writes: > You misunderstand. In this case it is crucial to read the regression test > first. The fix does not make much sense before one understands the > condition under which the order of the code statements matters. I am not sure what you mean. It sounds as if you want to use diff-orderfile to present change for t/ before changes to other files are presented in format-patch output to help readers, and I think that may make sense for certain cases. It may even include this case. But that is not incompatible with "avoid showing the patch that updates the code to fix breakages separately, ending up with showing the changes to t/ that are mostly about s/_failure/_success/ and readers are forced to go back to the previous patch to remind themselves what the broken scenario was about; by keeping it in a single patch, the readers can get the tests in one piece".
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
Hi Junio, On Tue, 13 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> For a trivially small change/fix like this, it is OK and even > >> preferrable to make 1+2 a single step, as applying t/ part only to > >> try to see the breakage (or "am"ing everything and then "diff | > >> apply -R" the part outside t/ for the same purpose) is easy enough. > > > > I disagree. It helps both development and porting to different branches to > > be able to cherry-pick the regression test individually. Please do not ask > > me to violate this hard-learned principle. > > A trivially small change/fix like this, by definition (of "trivial" > and "small" ness), it is trivial to develop and port to different > branches a single patch, and test with just one half (either the > test part or the code-change part) of the change reversed, to ensure > that the codebase is broken without the code-change and to > demonstrate that the code-change does fix the problem revealed by > the test change. And "porting" by cherry-picking a single patch is > always easier than two patch series. > > So you may disagree all you want in your project, but do not make > reviewer's lives unnecessarily harder in this project. You misunderstand. In this case it is crucial to read the regression test first. The fix does not make much sense before one understands the condition under which the order of the code statements matters. By trying to force me to smoosh them together, you are trying to force me to combine them in one patch where you would read the (now seemingly non-sensical) fix first, and only then the test. That's just really unhelpful. If I were a reviewer, I would want it presented in the way it *was* presented. I firmly believe most reviewers would. Ciao, Dscho
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
Johannes Schindelin writes: >> For a trivially small change/fix like this, it is OK and even >> preferrable to make 1+2 a single step, as applying t/ part only to >> try to see the breakage (or "am"ing everything and then "diff | >> apply -R" the part outside t/ for the same purpose) is easy enough. > > I disagree. It helps both development and porting to different branches to > be able to cherry-pick the regression test individually. Please do not ask > me to violate this hard-learned principle. A trivially small change/fix like this, by definition (of "trivial" and "small" ness), it is trivial to develop and port to different branches a single patch, and test with just one half (either the test part or the code-change part) of the change reversed, to ensure that the codebase is broken without the code-change and to demonstrate that the code-change does fix the problem revealed by the test change. And "porting" by cherry-picking a single patch is always easier than two patch series. So you may disagree all you want in your project, but do not make reviewer's lives unnecessarily harder in this project. Thanks.
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
Hi Junio, On Tue, 13 Nov 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > From: Johannes Schindelin > > > > When calling `merge` on a branch that has already been merged, that > > `merge` is skipped quietly, but currently a MERGE_HEAD file is being > > left behind and will then be grabbed by the next `pick` (that did > > not want to create a *merge* commit). > > > > Demonstrate this. > > > > Signed-off-by: Johannes Schindelin > > --- > > t/t3430-rebase-merges.sh | 16 > > 1 file changed, 16 insertions(+) > > For a trivially small change/fix like this, it is OK and even > preferrable to make 1+2 a single step, as applying t/ part only to > try to see the breakage (or "am"ing everything and then "diff | > apply -R" the part outside t/ for the same purpose) is easy enough. I disagree. It helps both development and porting to different branches to be able to cherry-pick the regression test individually. Please do not ask me to violate this hard-learned principle. > Because the patch 2 with your method ends up showing only the test > set-up part in the context by changing _failure to _success, without > showing what end-user visible breakage the step fixed, which usually > comes near the end of the added test piece. A single patch that > gives tests that ought to succeed would not force the readers to > switch between patches 1 and 2 while reading the fix. That is why I put in a verbose commit message, so that you do not have to guess. And even the test title talks about this. Seriously, I am very much opposed to changing the patches in the direction you suggested. In my mind, they would make the story substantially worse. Thank you for your review, Dscho > > Of course, the above would not apply for a more involved case where > the actual fix to the code needs to span multiple patches. > > Thanks. > > > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > > index aa7bfc88ec..1f08a33687 100755 > > --- a/t/t3430-rebase-merges.sh > > +++ b/t/t3430-rebase-merges.sh > > @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' ' > > grep "G: +G" actual > > ' > > > > +test_expect_failure '--continue after resolving conflicts after a merge' ' > > + git checkout -b already-has-g E && > > + git cherry-pick E..G && > > + test_commit H2 && > > + > > + git checkout -b conflicts-in-merge H && > > + test_commit H2 H2.t conflicts H2-conflict && > > + test_must_fail git rebase -r already-has-g && > > + grep conflicts H2.t && > > Is this making sure that the above test_must_fail succeeded because > of a conflict and not due to any other failure? I would have used > "ls-files -u H2.t" to see if the index is unmerged, which probably > is a more direct way to test what this is trying to test, but if we > are in the conflicted state, the one side of << == >> has this > string (the other has "H2" in it, presumably?), so in practice this > should be good enough. > > > + echo resolved >H2.t && > > + git add -u && > > and we resolve to continue. > > > + git rebase --continue && > > + test_must_fail git rev-parse --verify HEAD^2 && > > Even if we made an octopus by mistake, the above will catch it, > which is good. > > > + test_path_is_missing .git/MERGE_HEAD > > +' > > + > > test_done > > And from the proposed log message, I am reading that the last two > things (i.e. resulting tip is a child with a single parent and there > is no leftover MERGE_HEAD file) fail without the fix. > > This is enough material to convince me or anybody that the bug is > worth fixing. Thanks for being careful noticing a glitch during > your real (and otherwise unrelated to the bug) work and following > through. >
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
"Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > When calling `merge` on a branch that has already been merged, that > `merge` is skipped quietly, but currently a MERGE_HEAD file is being > left behind and will then be grabbed by the next `pick` (that did > not want to create a *merge* commit). > > Demonstrate this. > > Signed-off-by: Johannes Schindelin > --- > t/t3430-rebase-merges.sh | 16 > 1 file changed, 16 insertions(+) For a trivially small change/fix like this, it is OK and even preferrable to make 1+2 a single step, as applying t/ part only to try to see the breakage (or "am"ing everything and then "diff | apply -R" the part outside t/ for the same purpose) is easy enough. Because the patch 2 with your method ends up showing only the test set-up part in the context by changing _failure to _success, without showing what end-user visible breakage the step fixed, which usually comes near the end of the added test piece. A single patch that gives tests that ought to succeed would not force the readers to switch between patches 1 and 2 while reading the fix. Of course, the above would not apply for a more involved case where the actual fix to the code needs to span multiple patches. Thanks. > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index aa7bfc88ec..1f08a33687 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' ' > grep "G: +G" actual > ' > > +test_expect_failure '--continue after resolving conflicts after a merge' ' > + git checkout -b already-has-g E && > + git cherry-pick E..G && > + test_commit H2 && > + > + git checkout -b conflicts-in-merge H && > + test_commit H2 H2.t conflicts H2-conflict && > + test_must_fail git rebase -r already-has-g && > + grep conflicts H2.t && Is this making sure that the above test_must_fail succeeded because of a conflict and not due to any other failure? I would have used "ls-files -u H2.t" to see if the index is unmerged, which probably is a more direct way to test what this is trying to test, but if we are in the conflicted state, the one side of << == >> has this string (the other has "H2" in it, presumably?), so in practice this should be good enough. > + echo resolved >H2.t && > + git add -u && and we resolve to continue. > + git rebase --continue && > + test_must_fail git rev-parse --verify HEAD^2 && Even if we made an octopus by mistake, the above will catch it, which is good. > + test_path_is_missing .git/MERGE_HEAD > +' > + > test_done And from the proposed log message, I am reading that the last two things (i.e. resulting tip is a child with a single parent and there is no leftover MERGE_HEAD file) fail without the fix. This is enough material to convince me or anybody that the bug is worth fixing. Thanks for being careful noticing a glitch during your real (and otherwise unrelated to the bug) work and following through.
[PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
From: Johannes Schindelin When calling `merge` on a branch that has already been merged, that `merge` is skipped quietly, but currently a MERGE_HEAD file is being left behind and will then be grabbed by the next `pick` (that did not want to create a *merge* commit). Demonstrate this. Signed-off-by: Johannes Schindelin --- t/t3430-rebase-merges.sh | 16 1 file changed, 16 insertions(+) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index aa7bfc88ec..1f08a33687 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' ' grep "G: +G" actual ' +test_expect_failure '--continue after resolving conflicts after a merge' ' + git checkout -b already-has-g E && + git cherry-pick E..G && + test_commit H2 && + + git checkout -b conflicts-in-merge H && + test_commit H2 H2.t conflicts H2-conflict && + test_must_fail git rebase -r already-has-g && + grep conflicts H2.t && + echo resolved >H2.t && + git add -u && + git rebase --continue && + test_must_fail git rev-parse --verify HEAD^2 && + test_path_is_missing .git/MERGE_HEAD +' + test_done -- gitgitgadget
Re: BUG REPORT: git clone of non-existent repository results in request for credentials
I was afraid that was the reason. Oh well, at least we know why :-) Thanks Ævar! Best-F > On Nov 11, 2018, at 9:00 AM, Ævar Arnfjörð Bjarmason wrote: > > >> On Sun, Nov 11 2018, Federico Lucifredi wrote: >> >> git clone of non-existent repository results in request for credentials >> >> REPRODUCING: >> sudo apt install git >> git clone https://github.com/xorbit/LiFePo4owered-Pi.git#this repo does >> not exist >> >> Git will then prompt for username and password on Github. >> >> I can see a valid data-leak concern (one could probe for private repository >> names in a brute-force fashion), but then again the UX impact is appalling. >> Chances of someone typing an invalid repo name are pretty high, and this >> error message has nothing to do with the actual error. >> >> RESOLUTION: >> The error message should indicate that the repository name does not exist. > > This is a legitimate thing to complain about, but it has nothing to do > with git itself maintained on this mailing list, but the response codes > of specific git hosting websites. E.g. here's two issues for fixing this > on GitLab: > > https://gitlab.com/gitlab-org/gitlab-ce/issues/50201 > https://gitlab.com/gitlab-org/gitlab-ce/issues/50660 > > These hosting platforms are intentionally producing bad error messages > to not leak information, as you note. > > So I doubt it's something they'll ever change, the bug I have open with > this on GitLab is to make this configurable for privately run instances. >
Re: BUG REPORT: git clone of non-existent repository results in request for credentials
On Sun, Nov 11 2018, Federico Lucifredi wrote: > git clone of non-existent repository results in request for credentials > > REPRODUCING: > sudo apt install git > git clone https://github.com/xorbit/LiFePo4owered-Pi.git#this repo does > not exist > > Git will then prompt for username and password on Github. > > I can see a valid data-leak concern (one could probe for private repository > names in a brute-force fashion), but then again the UX impact is appalling. > Chances of someone typing an invalid repo name are pretty high, and this > error message has nothing to do with the actual error. > > RESOLUTION: > The error message should indicate that the repository name does not exist. This is a legitimate thing to complain about, but it has nothing to do with git itself maintained on this mailing list, but the response codes of specific git hosting websites. E.g. here's two issues for fixing this on GitLab: https://gitlab.com/gitlab-org/gitlab-ce/issues/50201 https://gitlab.com/gitlab-org/gitlab-ce/issues/50660 These hosting platforms are intentionally producing bad error messages to not leak information, as you note. So I doubt it's something they'll ever change, the bug I have open with this on GitLab is to make this configurable for privately run instances.
BUG REPORT: git clone of non-existent repository results in request for credentials
git clone of non-existent repository results in request for credentials REPRODUCING: sudo apt install git git clone https://github.com/xorbit/LiFePo4owered-Pi.git#this repo does not exist Git will then prompt for username and password on Github. I can see a valid data-leak concern (one could probe for private repository names in a brute-force fashion), but then again the UX impact is appalling. Chances of someone typing an invalid repo name are pretty high, and this error message has nothing to do with the actual error. RESOLUTION: The error message should indicate that the repository name does not exist. Best -F _ -- "'Problem' is a bleak word for challenge" - Richard Fish (Federico L. Lucifredi) - flucifredi at acm.org - GnuPG 0x4A73884C
[PATCH v3 13/16] parse-options.c: turn some die() to BUG()
These two strings are clearly not for the user to see. Reduce the
violence in one string while at there.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
parse-options.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 0bf817193d..3f5f985c1e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -197,7 +197,7 @@ static int get_value(struct parse_opt_ctx_t *p,
return 0;
default:
- die("should not happen, someone must be hit on the forehead");
+ BUG("opt->type %d should not happen", opt->type);
}
}
@@ -424,7 +424,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
ctx->flags = flags;
if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
(flags & PARSE_OPT_STOP_AT_NON_OPTION))
- die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
+ BUG("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
parse_options_check(options);
}
--
2.19.1.1231.g84aef82467
[PATCH v3 05/16] read-cache.c: turn die("internal error") to BUG()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
read-cache.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index d57958233e..0c37f4885e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -316,7 +316,7 @@ static int ce_match_stat_basic(const struct cache_entry
*ce, struct stat *st)
changed |= DATA_CHANGED;
return changed;
default:
- die("internal error: ce_mode is %o", ce->ce_mode);
+ BUG("unsupported ce_mode: %o", ce->ce_mode);
}
changed |= match_stat_data(>ce_stat_data, st);
@@ -2356,14 +2356,14 @@ void validate_cache_entries(const struct index_state
*istate)
for (i = 0; i < istate->cache_nr; i++) {
if (!istate) {
- die("internal error: cache entry is not allocated from
expected memory pool");
+ BUG("cache entry is not allocated from expected memory
pool");
} else if (!istate->ce_mem_pool ||
!mem_pool_contains(istate->ce_mem_pool,
istate->cache[i])) {
if (!istate->split_index ||
!istate->split_index->base ||
!istate->split_index->base->ce_mem_pool ||
!mem_pool_contains(istate->split_index->base->ce_mem_pool, istate->cache[i])) {
- die("internal error: cache entry is not
allocated from expected memory pool");
+ BUG("cache entry is not allocated from expected
memory pool");
}
}
}
--
2.19.1.1231.g84aef82467
[PATCH v3 09/16] remote.c: turn some error() or die() to BUG()
The first error, "internal error", is clearly a BUG(). The second two
are meant to catch calls with invalid parameters and should never
happen outside the test suite.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
remote.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/remote.c b/remote.c
index 81f4f01b00..257630ff21 100644
--- a/remote.c
+++ b/remote.c
@@ -620,7 +620,7 @@ static void handle_duplicate(struct ref *ref1, struct ref
*ref2)
* FETCH_HEAD_IGNORE entries always appear at
* the end of the list.
*/
- die(_("Internal error"));
+ BUG("Internal error");
}
}
free(ref2->peer_ref);
@@ -707,7 +707,7 @@ static void query_refspecs_multiple(struct refspec *rs,
int find_src = !query->src;
if (find_src && !query->dst)
- error("query_refspecs_multiple: need either src or dst");
+ BUG("query_refspecs_multiple: need either src or dst");
for (i = 0; i < rs->nr; i++) {
struct refspec_item *refspec = >items[i];
@@ -735,7 +735,7 @@ int query_refspecs(struct refspec *rs, struct refspec_item
*query)
char **result = find_src ? >src : >dst;
if (find_src && !query->dst)
- return error("query_refspecs: need either src or dst");
+ BUG("query_refspecs: need either src or dst");
for (i = 0; i < rs->nr; i++) {
struct refspec_item *refspec = >items[i];
--
2.19.1.1231.g84aef82467
Bug: git-svn clone --preserve-empty-dirs fail - couldn't truncate file at /usr/share/perl5/Git.pm line 1337.
Hi, i am in the middle of a migration project. Almost over 60 projects (aprox 1/3) suffer from this behaviour. I was able to create a svn repository dump (8MB) of a small project thats provoke that failure. The failure do not occur if the option "--preserve-empty-dirs" is omitted. Also no problem if the svn dump is filtered with "svndumpfilter --drop-empty-revs" before load into a fresh repo. I'll be able to provide the dump for further investigations. I would send it on demand. I'm able to reproduce the failure with the dump. It seemed many others had suffered from this bug. Used Version: 1:2.17.1-1ubuntu0.3 OS: Ubuntu 16.04.5 LTS Commandline: /usr/bin/git svn clone --prefix= --preserve-empty-dirs --authors-file=/mnt/migration/authors.txt --stdlayout https://host/repo /mnt/migration/repos/TEST/plausiweb r37895 = bf620e85ad1ac0d42298cb01cb0dab594958d1e1 (refs/remotes/trunk) A release-pom.xml A plausiweb-report/release-pom.xml M plausiweb-report/pom.xml A plausiweb-web/release-pom.xml M plausiweb-web/pom.xml M pom.xml r37896 = 9420d7081b44797825aa7a19bb508ef2533a3356 (refs/remotes/trunk) Found possible branch point: https://scm/svn/git/java/plausiweb/trunk => https://scm/svn/git/java/plausiweb/tags/plausiweb-18.06.1, 37896 Found branch parent: (refs/remotes/tags/plausiweb-18.06.1) 9420d7081b44797825aa7a19bb508ef2533a3356 Following parent with do_switch couldn't truncate file at /usr/share/perl5/Git.pm line 1337. -- Best regards from Bremen Georg Tsakumagos
Re: [PATCH v2 13/16] parse-options.c: turn some die() to BUG()
Nguyễn Thái Ngọc Duy writes:
> diff --git a/parse-options.c b/parse-options.c
> index 0bf817193d..3f5f985c1e 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -197,7 +197,7 @@ static int get_value(struct parse_opt_ctx_t *p,
> return 0;
>
> default:
> - die("should not happen, someone must be hit on the forehead");
> + BUG("opt->type %d should not happen", opt->type);
> }
> }
OK, this should not happen.
> @@ -424,7 +424,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
> ctx->flags = flags;
> if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
> (flags & PARSE_OPT_STOP_AT_NON_OPTION))
> - die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
> + BUG("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
The correctness of this conversion was not immediately obvious, as
"git rev-parse --parse-options" could allow end-users to specify
flags, and when these two flags came together from that codepath, it
is an end-user error that should be diagnosed with die(), not BUG().
It turns out that stop-at-non-option can be passed, but keep-unknown
is not (yet) available to the codepath, so this conversion to BUG()
is correct---an end user futzing with Git correctly compiled from a
bug-free source should not be able to trigger this.
Re: [PATCH v2 09/16] remote.c: turn some error() or die() to BUG()
Nguyễn Thái Ngọc Duy writes:
> The first error, "internal error", is clearly a BUG(). The second two
> are meant to catch calls with invalid parameters and should never
> happen outside the test suite.
Sounds as if it would happen inside test suites.
I guess by "inside the test suite" you mean a test that links broken
callers to this code as if it is a piece of library function, but
"should never happen, even with invalid or malformed end-user
request" would convey the point better, as the normal part of
testing that is done by driing "git we just built from the command
line should not hit these.
The changes all look sensible. Thanks.
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> remote.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 81f4f01b00..257630ff21 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -620,7 +620,7 @@ static void handle_duplicate(struct ref *ref1, struct ref
> *ref2)
>* FETCH_HEAD_IGNORE entries always appear at
>* the end of the list.
> */
> - die(_("Internal error"));
> + BUG("Internal error");
> }
> }
> free(ref2->peer_ref);
> @@ -707,7 +707,7 @@ static void query_refspecs_multiple(struct refspec *rs,
> int find_src = !query->src;
>
> if (find_src && !query->dst)
> - error("query_refspecs_multiple: need either src or dst");
> + BUG("query_refspecs_multiple: need either src or dst");
>
> for (i = 0; i < rs->nr; i++) {
> struct refspec_item *refspec = >items[i];
> @@ -735,7 +735,7 @@ int query_refspecs(struct refspec *rs, struct
> refspec_item *query)
> char **result = find_src ? >src : >dst;
>
> if (find_src && !query->dst)
> - return error("query_refspecs: need either src or dst");
> + BUG("query_refspecs: need either src or dst");
>
> for (i = 0; i < rs->nr; i++) {
> struct refspec_item *refspec = >items[i];
Re: [PATCH v2 05/16] read-cache.c: turn die("internal error") to BUG()
Nguyễn Thái Ngọc Duy writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> read-cache.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Makes sense; thanks.
>
> diff --git a/read-cache.c b/read-cache.c
> index d57958233e..0c37f4885e 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -316,7 +316,7 @@ static int ce_match_stat_basic(const struct cache_entry
> *ce, struct stat *st)
> changed |= DATA_CHANGED;
> return changed;
> default:
> - die("internal error: ce_mode is %o", ce->ce_mode);
> + BUG("unsupported ce_mode: %o", ce->ce_mode);
> }
>
> changed |= match_stat_data(>ce_stat_data, st);
> @@ -2356,14 +2356,14 @@ void validate_cache_entries(const struct index_state
> *istate)
>
> for (i = 0; i < istate->cache_nr; i++) {
> if (!istate) {
> - die("internal error: cache entry is not allocated from
> expected memory pool");
> + BUG("cache entry is not allocated from expected memory
> pool");
> } else if (!istate->ce_mem_pool ||
> !mem_pool_contains(istate->ce_mem_pool,
> istate->cache[i])) {
> if (!istate->split_index ||
> !istate->split_index->base ||
> !istate->split_index->base->ce_mem_pool ||
>
> !mem_pool_contains(istate->split_index->base->ce_mem_pool, istate->cache[i]))
> {
> - die("internal error: cache entry is not
> allocated from expected memory pool");
> + BUG("cache entry is not allocated from expected
> memory pool");
> }
> }
> }
[PATCH v2 13/16] parse-options.c: turn some die() to BUG()
These two strings are clearly not for the user to see. Reduce the
violence in one string while at there.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
parse-options.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 0bf817193d..3f5f985c1e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -197,7 +197,7 @@ static int get_value(struct parse_opt_ctx_t *p,
return 0;
default:
- die("should not happen, someone must be hit on the forehead");
+ BUG("opt->type %d should not happen", opt->type);
}
}
@@ -424,7 +424,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
ctx->flags = flags;
if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
(flags & PARSE_OPT_STOP_AT_NON_OPTION))
- die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
+ BUG("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
parse_options_check(options);
}
--
2.19.1.1005.gac84295441
[PATCH v2 09/16] remote.c: turn some error() or die() to BUG()
The first error, "internal error", is clearly a BUG(). The second two
are meant to catch calls with invalid parameters and should never
happen outside the test suite.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
remote.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/remote.c b/remote.c
index 81f4f01b00..257630ff21 100644
--- a/remote.c
+++ b/remote.c
@@ -620,7 +620,7 @@ static void handle_duplicate(struct ref *ref1, struct ref
*ref2)
* FETCH_HEAD_IGNORE entries always appear at
* the end of the list.
*/
- die(_("Internal error"));
+ BUG("Internal error");
}
}
free(ref2->peer_ref);
@@ -707,7 +707,7 @@ static void query_refspecs_multiple(struct refspec *rs,
int find_src = !query->src;
if (find_src && !query->dst)
- error("query_refspecs_multiple: need either src or dst");
+ BUG("query_refspecs_multiple: need either src or dst");
for (i = 0; i < rs->nr; i++) {
struct refspec_item *refspec = >items[i];
@@ -735,7 +735,7 @@ int query_refspecs(struct refspec *rs, struct refspec_item
*query)
char **result = find_src ? >src : >dst;
if (find_src && !query->dst)
- return error("query_refspecs: need either src or dst");
+ BUG("query_refspecs: need either src or dst");
for (i = 0; i < rs->nr; i++) {
struct refspec_item *refspec = >items[i];
--
2.19.1.1005.gac84295441
[PATCH v2 05/16] read-cache.c: turn die("internal error") to BUG()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
read-cache.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index d57958233e..0c37f4885e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -316,7 +316,7 @@ static int ce_match_stat_basic(const struct cache_entry
*ce, struct stat *st)
changed |= DATA_CHANGED;
return changed;
default:
- die("internal error: ce_mode is %o", ce->ce_mode);
+ BUG("unsupported ce_mode: %o", ce->ce_mode);
}
changed |= match_stat_data(>ce_stat_data, st);
@@ -2356,14 +2356,14 @@ void validate_cache_entries(const struct index_state
*istate)
for (i = 0; i < istate->cache_nr; i++) {
if (!istate) {
- die("internal error: cache entry is not allocated from
expected memory pool");
+ BUG("cache entry is not allocated from expected memory
pool");
} else if (!istate->ce_mem_pool ||
!mem_pool_contains(istate->ce_mem_pool,
istate->cache[i])) {
if (!istate->split_index ||
!istate->split_index->base ||
!istate->split_index->base->ce_mem_pool ||
!mem_pool_contains(istate->split_index->base->ce_mem_pool, istate->cache[i])) {
- die("internal error: cache entry is not
allocated from expected memory pool");
+ BUG("cache entry is not allocated from expected
memory pool");
}
}
}
--
2.19.1.1005.gac84295441
Re: [BUG?] protocol.version=2 sends HTTP "Expect" headers
On Thu, Nov 01, 2018 at 12:48:04AM +, brian m. carlson wrote:
> > So a few questions:
> >
> > - is this a bug or not? I.e., do we still need to care about proxies
> > that can't handle Expect? The original commit was from 2011. Maybe
> > things are better now. Or maybe that's blind optimism.
> >
> > Nobody has complained yet, but that's probably just because v2 isn't
> > widely deployed yet.
>
> HTTP/1.1 requires support for 100 Continue on the server side and in
> proxies (it is mandatory to implement). The original commit disabling
> it (959dfcf42f ("smart-http: Really never use Expect: 100-continue",
> 2011-03-14)), describes proxies as the reason for disabling it.
>
> It's my understanding that all major proxies (including, as of version
> 3.2, Squid) support HTTP/1.1 at this point. Moreover, Kerberos is more
> likely to be used in enterprises, where proxies (especially poorly
> behaved and outright broken proxies) are more common. We haven't seen
> any complaints about Kerberos support in a long time. This leads me to
> believe that things generally work[0].
Rooting around in the archive a bit, I found this discussion:
https://public-inbox.org/git/CAJo=hjvyormjfyznvwz4izr88ewor6luqoe-mpt4lspyodd...@mail.gmail.com/
The original motivation for 959dfcf42f was apparently Google's own
servers. And they are likely the biggest users of the v2 protocol
(since my impression is that Google ships a modified client to most of
their devs that flips the v2 switch). So if they're not having problems,
maybe that's a sign that this is a non-issue.
> Finally, modern versions of libcurl also have a timeout after which they
> assume that the server is not going to respond and just send the request
> body anyways. This makes the issue mostly moot.
I think this was always the case. It's just that it introduced annoying
stalls into the protocol.
> > - alternatively, should we just leave it on for v2, and provide a
> > config switch to disable it if you have a crappy proxy? I don't know
> > how widespread the problem is, but I can imagine that the issue is
> > subtle enough that most users wouldn't even know.
>
> For the reasons I mentioned above, I'd leave it on for now. Between
> libcurl and better proxy support, I think this issue is mostly solved.
> If we need to fix it in the future, we can, and people can fall back to
> older protocols via config in the interim.
OK. If nobody is complaining, this seems like a good way to ease into
the migration. I.e., if we dropped the old "suppress Expect headers"
code, people might complain that things are now broken. But if it
gradually follows the v2 adoptions, that's a bit more of a gentle curve
(well, until we hit the cliff where we switch the default to trying v2).
-Peff
Re: [BUG?] protocol.version=2 sends HTTP "Expect" headers
On Wed, Oct 31, 2018 at 12:03:53PM -0400, Jeff King wrote:
> Since 959dfcf42f (smart-http: Really never use Expect: 100-continue,
> 2011-03-14), we try to avoid sending "Expect" headers, since some
> proxies apparently don't handle them well. There we have to explicitly
> tell curl not to use them.
>
> The exception is large requests with GSSAPI, as explained in c80d96ca0c
> (remote-curl: fix large pushes with GSSAPI, 2013-10-31).
>
> However, Jon Simons noticed that when using protocol.version=2, we've
> started sending Expect headers again. That's because rather than going
> through post_rpc(), we push the stateless data through a proxy_state
> struct. And in proxy_state_init(), when we set up the headers, we do not
> disable curl's Expect handling.
>
> So a few questions:
>
> - is this a bug or not? I.e., do we still need to care about proxies
> that can't handle Expect? The original commit was from 2011. Maybe
> things are better now. Or maybe that's blind optimism.
>
> Nobody has complained yet, but that's probably just because v2 isn't
> widely deployed yet.
HTTP/1.1 requires support for 100 Continue on the server side and in
proxies (it is mandatory to implement). The original commit disabling
it (959dfcf42f ("smart-http: Really never use Expect: 100-continue",
2011-03-14)), describes proxies as the reason for disabling it.
It's my understanding that all major proxies (including, as of version
3.2, Squid) support HTTP/1.1 at this point. Moreover, Kerberos is more
likely to be used in enterprises, where proxies (especially poorly
behaved and outright broken proxies) are more common. We haven't seen
any complaints about Kerberos support in a long time. This leads me to
believe that things generally work[0].
Finally, modern versions of libcurl also have a timeout after which they
assume that the server is not going to respond and just send the request
body anyways. This makes the issue mostly moot.
> - alternatively, should we just leave it on for v2, and provide a
> config switch to disable it if you have a crappy proxy? I don't know
> how widespread the problem is, but I can imagine that the issue is
> subtle enough that most users wouldn't even know.
For the reasons I mentioned above, I'd leave it on for now. Between
libcurl and better proxy support, I think this issue is mostly solved.
If we need to fix it in the future, we can, and people can fall back to
older protocols via config in the interim.
[0] In some environments, people use SSH because the proxy breaks
everything that looks like HTTP, but that's beyond the scope of this
discussion.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
[BUG?] protocol.version=2 sends HTTP "Expect" headers
Since 959dfcf42f (smart-http: Really never use Expect: 100-continue, 2011-03-14), we try to avoid sending "Expect" headers, since some proxies apparently don't handle them well. There we have to explicitly tell curl not to use them. The exception is large requests with GSSAPI, as explained in c80d96ca0c (remote-curl: fix large pushes with GSSAPI, 2013-10-31). However, Jon Simons noticed that when using protocol.version=2, we've started sending Expect headers again. That's because rather than going through post_rpc(), we push the stateless data through a proxy_state struct. And in proxy_state_init(), when we set up the headers, we do not disable curl's Expect handling. So a few questions: - is this a bug or not? I.e., do we still need to care about proxies that can't handle Expect? The original commit was from 2011. Maybe things are better now. Or maybe that's blind optimism. Nobody has complained yet, but that's probably just because v2 isn't widely deployed yet. - if it is a bug, how can we handle it like the v0 code? There we enable it only for GSSAPI on large requests. But I'm not sure we can know here whether the request is large, since we're inherently just streaming through chunked data. It looks like post_rpc tries to read a single packet first, and considers anything over 64k to be large. - alternatively, should we just leave it on for v2, and provide a config switch to disable it if you have a crappy proxy? I don't know how widespread the problem is, but I can imagine that the issue is subtle enough that most users wouldn't even know. I think I've convinced myself that we probably do need to do the "peek at the first packet" thing like post_rpc() does, but I think it might be tricky with the way the proxy_state code is structured. Thoughts from people with more HTTP knowledge/experience? -Peff
Re: bug?: git grep HEAD with exclude in pathspec not taken into account
On Wed, Oct 24, 2018 at 4:55 PM Christophe Bliard wrote: > > Hi, > > I observed an unexpected behavior while using git grep with both git > 2.19.1 and 2.14.3. Quick note. I confirm this is a bug in tree_entry_interesting() perhaps being over-optimistic. It'll take me more time to familiarize myself with this negative matching in the function before I come up with a fix for it. Thanks for reporting. -- Duy
Re: Possible bug in referenced configuration file loading
On Fri, Oct 26, 2018 at 09:30:51AM +0200, Raphael Stolt wrote: > Configuration for global ignore patterns in ~/.config/git/config: > > [core] > excludesfile = .gitignore > > doesn't get looked up per default in ~/.config/git/ which might be > considered a bug as the .gitignore file isn't loaded. It's only loaded > when .gitignore is located in $HOME or explicitly referenced via > ~/.config/git/.gitignore. I don't think we ever defined or documented any behavior for relative paths in core.excludesFile (nor most other path-based config options). I suspect it also changes depending on the command, as well as whether it is a bare or non-bare repository. Because I think it is probably just wherever the cwd happens to be at the time we parse the config, which is usually at the top of the working tree (non-bare) or inside the repository (bare). > Configuration for a conditional include also in ~/.config/git/config: > > [includeIf "gitdir:~/Work/git-repos/"] > path = .oss-gitconfig > > does get looked up per default in ~/.config/git/ and therefor the > .oss-gitconfig is loaded. Whereas config includes have always explicitly advertised from day one the behavior of relative paths (that they match the surrounding config file). I am not opposed to coming up with a rule for relative paths in excludesFile and other config options. The "pretend as if it is next to the enclosed config file" rule is sensible to me, but that is not surprising since I was the one who implemented it for includes. ;) We'd potentially break some people's config, but I suspect such config is flaky already (see above). It would be nice if this were implemented via git_config_pathname() for consistency, but beware that some options already do specify a behavior for relative paths, and we would have to make sure not to break that (e.g., see the documentation and implementation for core.hooksPath). -Peff
Possible bug in referenced configuration file loading
Configuration for global ignore patterns in ~/.config/git/config: [core] excludesfile = .gitignore doesn't get looked up per default in ~/.config/git/ which might be considered a bug as the .gitignore file isn't loaded. It's only loaded when .gitignore is located in $HOME or explicitly referenced via ~/.config/git/.gitignore. Configuration for a conditional include also in ~/.config/git/config: [includeIf "gitdir:~/Work/git-repos/"] path = .oss-gitconfig does get looked up per default in ~/.config/git/ and therefor the .oss-gitconfig is loaded. So there seems to be a difference in Git's configuration loading strategies. Environment details: macOS High Sierra git version 2.19.1 Windows 10 git version 2.16.1.windows.2
Bug: git log changes output depending on how the output is used
I noticed that the piped output still prints the (HEAD -> ) : git log --pretty=format:"%d*%B" --decorate-refs='refs/tags/*' *bar (tag: "123")*foo git log --pretty=format:"%d*%B" --decorate-refs='refs/tags/*' > tmp.tmp (HEAD -> branch)*bar (tag: "123")*foo I would expect the output that is printed to console, not what's present in tmp.tmp
[PATCH v4 1/3] repack: point out a bug handling stale shallow info
From: Johannes Schindelin A `git fetch --prune` can turn previously-reachable objects unreachable, even commits that are in the `shallow` list. A subsequent `git repack -ad` will then unceremoniously drop those unreachable commits, and the `shallow` list will become stale. This means that when we try to fetch with a larger `--depth` the next time, we may end up with: fatal: error in object: unshallow Reported by Alejandro Pauly. Signed-off-by: Johannes Schindelin --- t/t5537-fetch-shallow.sh | 27 +++ 1 file changed, 27 insertions(+) diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index 7045685e2..686fdc928 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -186,6 +186,33 @@ EOF test_cmp expect actual ' +test_expect_failure '.git/shallow is edited by repack' ' + git init shallow-server && + test_commit -C shallow-server A && + test_commit -C shallow-server B && + git -C shallow-server checkout -b branch && + test_commit -C shallow-server C && + test_commit -C shallow-server E && + test_commit -C shallow-server D && + d="$(git -C shallow-server rev-parse --verify D^0)" && + git -C shallow-server checkout master && + + git clone --depth=1 --no-tags --no-single-branch \ + "file://$PWD/shallow-server" shallow-client && + + : now remove the branch and fetch with prune && + git -C shallow-server branch -D branch && + git -C shallow-client fetch --prune --depth=1 \ + origin "+refs/heads/*:refs/remotes/origin/*" && + git -C shallow-client repack -adfl && + test_must_fail git -C shallow-client rev-parse --verify $d^0 && + ! grep $d shallow-client/.git/shallow && + + git -C shallow-server branch branch-orig $d && + git -C shallow-client fetch --prune --depth=2 \ + origin "+refs/heads/*:refs/remotes/origin/*" +' + . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd -- gitgitgadget
Re: bug?: git grep HEAD with exclude in pathspec not taken into account
In fact, per https://github.com/git/git/commit/859b7f1d0e742493d2a9396794cd9040213ad846, having only a negative pattern is like having a catch-all positive pattern and the negative pattern (since git 2.13.0). Thus, adding the positive pattern yields the same results: > git --no-pager grep foo HEAD -- ':!fileA' . HEAD:fileB:foo is false+ > git --no-pager grep foo HEAD -- ':!fileB' . HEAD:fileA:foo HEAD:fileB:foo is false+ Le mer. 24 oct. 2018 à 17:14, Duy Nguyen a écrit : > > On Wed, Oct 24, 2018 at 4:55 PM Christophe Bliard > wrote: > > > > Hi, > > > > I observed an unexpected behavior while using git grep with both git > > 2.19.1 and 2.14.3. Here is how to reproduce it: > > > > > git init > > Initialized empty Git repository in /private/tmp/hello/.git/ > > > echo foo > fileA > > > echo 'foo is false+' > fileB > > > git add fileA > > > git commit -m fileA > > [master (root-commit) f2c83e7] fileA > > 1 file changed, 1 insertion(+) > > create mode 100644 fileA > > > git add fileB > > > git commit -m fileB > > [master e35df26] fileB > > 1 file changed, 1 insertion(+) > > create mode 100644 fileB > > > git --no-pager grep foo HEAD -- ':!fileA' > > HEAD:fileB:foo is false+ > > > git --no-pager grep foo HEAD -- ':!fileB' > > HEAD:fileA:foo > > HEAD:fileB:foo is false+ > > > > In the last command, fileB appears in grep results but it should have > > been excluded. > > > > If the HEAD parameter is removed, it works as expected: > > It's probably a bug. We have different code paths for matching things > with or without HEAD, roughly speaking. I'll look into to this more on > the weekend, unless somebody (is welcome to) beats me first. > > Another thing that might also be a problem is, negative patterns are > supposed to exclude stuff from "something" but you don't specify that > "something" (i.e. positive patterns) in your grep commands above. If > "grep foo HEAD -- :/ ':!fileB'" works, then you probably just run into > some undefined corner case. > -- > Duy
Re: bug?: git grep HEAD with exclude in pathspec not taken into account
On Wed, Oct 24, 2018 at 4:55 PM Christophe Bliard wrote: > > Hi, > > I observed an unexpected behavior while using git grep with both git > 2.19.1 and 2.14.3. Here is how to reproduce it: > > > git init > Initialized empty Git repository in /private/tmp/hello/.git/ > > echo foo > fileA > > echo 'foo is false+' > fileB > > git add fileA > > git commit -m fileA > [master (root-commit) f2c83e7] fileA > 1 file changed, 1 insertion(+) > create mode 100644 fileA > > git add fileB > > git commit -m fileB > [master e35df26] fileB > 1 file changed, 1 insertion(+) > create mode 100644 fileB > > git --no-pager grep foo HEAD -- ':!fileA' > HEAD:fileB:foo is false+ > > git --no-pager grep foo HEAD -- ':!fileB' > HEAD:fileA:foo > HEAD:fileB:foo is false+ > > In the last command, fileB appears in grep results but it should have > been excluded. > > If the HEAD parameter is removed, it works as expected: It's probably a bug. We have different code paths for matching things with or without HEAD, roughly speaking. I'll look into to this more on the weekend, unless somebody (is welcome to) beats me first. Another thing that might also be a problem is, negative patterns are supposed to exclude stuff from "something" but you don't specify that "something" (i.e. positive patterns) in your grep commands above. If "grep foo HEAD -- :/ ':!fileB'" works, then you probably just run into some undefined corner case. -- Duy
Re: bug?: git grep HEAD with exclude in pathspec not taken into account
Hi, I observed an unexpected behavior while using git grep with both git 2.19.1 and 2.14.3. Here is how to reproduce it: > git init Initialized empty Git repository in /private/tmp/hello/.git/ > echo foo > fileA > echo 'foo is false+' > fileB > git add fileA > git commit -m fileA [master (root-commit) f2c83e7] fileA 1 file changed, 1 insertion(+) create mode 100644 fileA > git add fileB > git commit -m fileB [master e35df26] fileB 1 file changed, 1 insertion(+) create mode 100644 fileB > git --no-pager grep foo HEAD -- ':!fileA' HEAD:fileB:foo is false+ > git --no-pager grep foo HEAD -- ':!fileB' HEAD:fileA:foo HEAD:fileB:foo is false+ In the last command, fileB appears in grep results but it should have been excluded. If the HEAD parameter is removed, it works as expected: > git --no-pager grep foo -- ':!fileB' fileA:foo If giving the revision, it does not work either > git --no-pager grep foo e35df26 -- ':!fileB' e35df26:fileA:foo e35df26:fileB:foo is false+ The same behavior can be seen with git archive: > git archive --verbose master ':(top)' fileA fileB pax_global_header66600064133641017230014512gustar00rootroot0052 comment=e35df26c65f3c0b303e78743496598b8b6a566e9 fileA66441336410172300120130ustar00rootroot00foo fileB664000161336410172300120170ustar00rootroot00foo is false+ > /usr/local/bin/git archive --verbose master ':(top)' ':(exclude)fileA' fileB pax_global_header66600064133641017230014512gustar00rootroot0052 comment=e35df26c65f3c0b303e78743496598b8b6a566e9 fileB664000161336410172300120170ustar00rootroot00foo is false+ > /usr/local/bin/git archive --verbose master ':(top)' ':(exclude)fileB' fileA fileB pax_global_header66600064133641017230014512gustar00rootroot0052 comment=e35df26c65f3c0b303e78743496598b8b6a566e9 fileA66441336410172300120130ustar00rootroot00foo fileB664000161336410172300120170ustar00rootroot00foo is false+ fileA can be excluded, but not fileB. Is it a bug? Thanks -- Christophe Bliard
Re: [PATCH v3 1/3] repack: point out a bug handling stale shallow info
Hi Junio, me again. I was wrong. On Wed, 24 Oct 2018, Johannes Schindelin wrote: > On Wed, 24 Oct 2018, Junio C Hamano wrote: > > > "Johannes Schindelin via GitGitGadget" > > writes: > > > > > +... > > > + d="$(git -C shallow-server rev-parse --verify D)" && > > > + git -C shallow-server checkout master && > > > + > > > +... > > > + ! grep $d shallow-client/.git/shallow && > > > > We know D (and $d) is not a tag, > > Actually, it is... the `test_commit D` creates that tag, and that is what > I use here. > > > but perhaps the place that assigns to $d (way above) can do the > > rev-parse of D^0, not D, to make it more clear what is going on, > > especially given that... > > ... that the `grep` really wants to test for the absence of the *commit*, > not the *tag* in .git/shallow? > > Yes, you are right ;-) > > So why did my test do the right thing, if it looked at a tag, but did not > dereference it via ^0? The answer is: the `test_commit` function creates > light-weight tags, i.e. no tag objects. And therefore, the $d^0 you found > below, that's just confusing. What I was referring to was the call test_must_fail git -C shallow-client rev-parse --verify $d^0 However, here we *have* to append ^0, otherwise `rev-parse --verify` will (and quite confusingly so) *succeed*. I *repeatedly* fall into that trap that `git rev-parse --verify ` will succeed. Why? Because that is a valid 40-digit hex string. Not because the object exists. Because it does not. So I managed to confuse myself again into believing that I only need to append ^0 to the earlier rev-parse call, but can remove it from this one, when in reality, I have to append it to both. In the first case, to avoid having to think about dereferencing a tag, in the second case, to force rev-parse to look for the object. Ciao, Dscho > > I will change it so that the `rev-parse` call uses ^0 (even if it is > technically not necessary), to avoid said confusion. > > Thanks, > Dscho > > > > > > + git -C shallow-server branch branch-orig D^0 && > > > > ... this does that D^0 thing here to makes us wonder if D needs > > unwrapping before using it as a commit (not commit-ish). > > > > If we did so, we could use $d here instead of D^0. > > > > > + git -C shallow-client fetch --prune --depth=2 \ > > > + origin "+refs/heads/*:refs/remotes/origin/*" > > > +' > > > + > > > . "$TEST_DIRECTORY"/lib-httpd.sh > > > start_httpd > > > > >
