Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust
Hi, > This subject line will appear out of context in "git shortlog" output, > so it's useful to pack in enough information to briefly summarize what > the change does. I'm happy to do so. I think that "simplify" is misleading, because this is a bug fix, not a refactoring. I like your first suggestion, though it exceeds the 50-character soft limit. What do you think of: test_dir_is_empty: find files with newline in name ? > I don't think "xargs -0" is present on all supported platforms Wow---I'm shocked that it's not POSIX, but you're right. (That makes `xargs` so much less useful!) t/t3600-rm.sh seems to just literally embed the newline into the argument to `touch`. I can do that. (I intentionally avoided $'' for the reason that you mention.) > Not all filesystems support newlines in filenames. I think we'd want > to factor out the FUNNYNAMES prereq from there into a test_lazy_prereq > so this test can be skipped on such filenames. This makes sense. Would you like me to send in a separate patch to add this `test_lazy_prereq` to `t/test-lib.sh`, fixing up existing uses (of which there are several), and then rebase this patch on top of it? > Another portability gotcha: wc output includes a space on Mac so this > test would always return true there. That's good to know. I can use `test -n "$(ls -A1 "$1")"` as you suggest, although... > "ls -A" was added in POSIX.1-2017. [...] > That's very recent, but the widespread implementation it mentions is > less so. This would be our first use of "ls -A", so I'd be interested > to hear from people on more obscure platforms. It does seem to be > widespread. ...if you prefer, a variant of `test "$(ls -a1 | wc -l)" -eq 2` should satisfy all the crtieria that you mention above (POSIX since forever, should work on Mac). The assumption is that a file with a newline character may take up more than one line, but every file must take up _at least_ one line, and we get two lines from `.` and `..`. If this assumption is false, then I will have learned yet another new thing! > Can you say a little more about where you ran into this? Did you > discover it by code inspection? Sure. Yes. I have a build script that accepts a target output directory, and rejects if the directory is nonempty. I used `ls -A | wc -l` to implement this check. When testing the script with Sharness, I ran across `test_dir_is_empty`. I was curious about the implementation, having recently implemented the same test myself. The `egrep` in the implementation stood out to me as suspicious, and so it was easy to come up with an explicit counterexample. > I do think the resulting implementation using -A is simpler. Thanks > for writing it. You're welcome. Thank you for the detailed and helpful review. Best, WC
Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust
On Sun, Aug 5, 2018 at 12:20 AM Jonathan Nieder wrote:
> William Chargin wrote:
> > test_dir_is_empty () {
> > test_path_is_dir "$1" &&
> > - if test -n "$(ls -a1 "$1" | egrep -v '^\.\.?$')"
> > + if test "$(ls -A1 "$1" | wc -c)" != 0
>
> Another portability gotcha: wc output includes a space on Mac so this
> test would always return true there. How about
>
> if test -n "$(ls -A1 "$1")"
>
> "ls -A" was added in POSIX.1-2017. [...]
> That's very recent, but the widespread implementation it mentions is
> less so. This would be our first use of "ls -A", so I'd be interested
> to hear from people on more obscure platforms. It does seem to be
> widespread.
A simpler approach, without the portability concerns of -A, would be
to remove the "." and ".." lines from the top of the listing:
ls -f1 "$1" | sed '1,2d'
If we're worried about -f not being sufficiently portable, then an
even simpler approach would be to check whether the output of 'ls -a1'
has more lines than the two expected ("." and ".."):
test $(ls -a1 "$1" | wc -l) -gt 2
I think I favor this final implementation over the others.
Re: [PATCH] Makefile: enable DEVELOPER by default
On Sat, Aug 4, 2018 at 11:33 PM Eric Sunshine wrote: > On Sat, Aug 4, 2018 at 11:17 PM Jonathan Nieder wrote: > > So it looks like FreeBSD has modernized and we need to make that > > conditional in config.mak.uname on $(uname_R). Do you know which > > version of FreeBSD changed the signature? Care to write a patch? > > I'm not very familiar with FreeBSD-land, but I would > hope there would be an easier way to determine when it changed than by > installing old versions. Does FreeBSD have historic package > repositories (containing headers, for instance) which one could > consult instead? I thought perhaps we could figure out the timeframe by looking at the Git package[1] in the FreeBSD ports tree to see when they added, removed, or changed a patch file for config.mak.uname, but that didn't pan out since they invoke the Git 'configure' script which determines OLD_ICONV automatically. [1]: https://github.com/freebsd/freebsd-ports/tree/master/devel/git
[PATCH v2] t/test-lib: make `test_dir_is_empty` more robust
While the `test_dir_is_empty` function appears correct in most normal
use cases, it can fail when filenames contain newlines. This patch
changes the implementation to use `ls -A`, which is specified by POSIX.
The output should be empty exactly if the directory is empty.
The newly added unit test fails before this change and passes after it.
Signed-off-by: William Chargin
---
I originally wrote this patch for the standalone Sharness library, but
that library advises that such patches be sent to the Git mailing list
first.
t/t-basic.sh| 29 +
t/test-lib-functions.sh | 2 +-
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/t/t-basic.sh b/t/t-basic.sh
index 34859fe4a..3885b26f9 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -821,6 +821,35 @@ test_expect_success 'tests clean up even on failures' "
EOF
"
+test_expect_success 'test_dir_is_empty behaves even in pathological cases' "
+ run_sub_test_lib_test \
+ dir-empty 'behavior of test_dir_is_empty' <<-\\EOF &&
+ test_expect_success 'should pass with actually empty directory' '
+ mkdir empty_dir &&
+ test_dir_is_empty empty_dir
+ '
+ test_expect_success 'should fail with a normal filename' '
+ mkdir nonempty_dir &&
+ touch nonempty_dir/some_file &&
+ test_must_fail test_dir_is_empty nonempty_dir
+ '
+ test_expect_success 'should fail with dot-newline-dot filename' '
+ mkdir pathological_dir &&
+ printf \"pathological_dir/.n.0\" | xargs -0 touch &&
+ test_must_fail test_dir_is_empty pathological_dir
+ '
+ test_done
+ EOF
+ check_sub_test_lib_test dir-empty <<-\\EOF
+ > ok 1 - should pass with actually empty directory
+ > ok 2 - should fail with a normal filename
+ > ok 3 - should fail with dot-newline-dot filename
+ > # passed all 3 test(s)
+ > 1..3
+ EOF
+"
+
+
# Basics of the basics
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2b2181dca..f7ff28ef6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -568,7 +568,7 @@ test_path_is_dir () {
# Check if the directory exists and is empty as expected, barf otherwise.
test_dir_is_empty () {
test_path_is_dir "$1" &&
- if test -n "$(ls -a1 "$1" | egrep -v '^\.\.?$')"
+ if test "$(ls -A1 "$1" | wc -c)" != 0
then
echo "Directory '$1' is not empty, it contains:"
ls -la "$1"
--
2.18.0.548.geb6c14151
Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust
Hi,
William Chargin wrote:
> Subject: t/test-lib: make `test_dir_is_empty` more robust
This subject line will appear out of context in "git shortlog" output,
so it's useful to pack in enough information to briefly summarize what
the change does. How about something like
test_dir_is_empty: avoid being confused by $'.\n.' file
or
test_dir_is_empty: simplify by using "ls --almost-all"
?
[...]
> + test_expect_success 'should fail with dot-newline-dot filename' '
> + mkdir pathological_dir &&
> + printf \"pathological_dir/.n.0\" | xargs -0 touch &&
I don't think "xargs -0" is present on all supported platforms. I'd
be tempted to use
>pathological_dir/$'.\n.'
but $'' is too recent of a shell feature to count on (e.g., dash doesn't
support it). See t/t3600-rm.sh for an example of a portable way to do
this.
Not all filesystems support newlines in filenames. I think we'd want
to factor out the FUNNYNAMES prereq from there into a test_lazy_prereq
so this test can be skipped on such filenames.
[...]
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -568,7 +568,7 @@ test_path_is_dir () {
> # Check if the directory exists and is empty as expected, barf otherwise.
> test_dir_is_empty () {
> test_path_is_dir "$1" &&
> - if test -n "$(ls -a1 "$1" | egrep -v '^\.\.?$')"
> + if test "$(ls -A1 "$1" | wc -c)" != 0
Another portability gotcha: wc output includes a space on Mac so this
test would always return true there. How about
if test -n "$(ls -A1 "$1")"
?
"ls -A" was added in POSIX.1-2017. Its rationale section explains
Earlier versions of this standard did not describe the BSD -A
option (like -a, but dot and dot-dot are not written out). It
has been added due to widespread implementation.
That's very recent, but the widespread implementation it mentions is
less so. This would be our first use of "ls -A", so I'd be interested
to hear from people on more obscure platforms. It does seem to be
widespread.
Can you say a little more about where you ran into this? Did you
discover it by code inspection?
I do think the resulting implementation using -A is simpler. Thanks
for writing it.
Thanks and hope that helps,
Jonathan
Re: [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust
Hi Jonathan, > This information belongs in the commit message Agreed: I included it at the start of the commit message, though I suppose that the wording in the cover letter is clearer, so I've amended the commit to use that wording instead. > Continuing the note about administrivia, this > kind of cover letter material that you want to not be part of the > commit message can go below the three-dashes delimiter when you send a > patch. Perfect; this is what I was missing. Thanks. Let me try again. :-)
Re: [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust
Hi, William Chargin wrote: > While the `test_dir_is_empty` function appears correct in most normal > use cases, it can fail when filenames contain newlines. This information belongs in the commit message, since it's useful context for understanding the motivation behind the patch when encountering it with e.g. "git log". That's part of why I recommend never sending a separate cover-letter email for a single-patch series. See [1] from Documentation/SubmittingPatches for more on this subject. > I originally > wrote this patch for the standalone Sharness library, but that library > advises that such patches be sent to the Git mailing list first. Thanks for writing it! Continuing the note about administrivia, this kind of cover letter material that you want to not be part of the commit message can go below the three-dashes delimiter when you send a patch. There's more about this at [2]. Thanks again, Jonathan [1] https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#describe-changes [2] https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#send-patches
Re: [PATCH] Makefile: enable DEVELOPER by default
On Sat, Aug 4, 2018 at 11:17 PM Jonathan Nieder wrote: > > utf8.c:486:28: warning: passing 'iconv_ibp *' (aka 'const char **') to > > parameter > > of type 'char **' discards qualifiers in nested pointer types > > [-Wincompatible-pointer-types-discards-qualifiers] > > Oh, good catch! POSIX documents iconv has having signature > > size_t iconv(iconv_t cd, char **restrict inbuf, >size_t *restrict inbytesleft, char **restrict outbuf, >size_t *restrict outbytesleft); > > config.mak.uname contains > > ifeq ($(uname_S),FreeBSD) > NEEDS_LIBICONV = YesPlease > OLD_ICONV = YesPlease > > So it looks like FreeBSD has modernized and we need to make that > conditional in config.mak.uname on $(uname_R). Do you know which > version of FreeBSD changed the signature? Care to write a patch? Unfortunately, I don't know in which version of FreeBSD that changed. I rarely fire up that virtual machine (only in rare cases when I want to verify some change to Git also builds/runs/whatever on FreeBSD), so I haven't really been paying attention to it. I know that this warning was present in 11.1 (and I'm guessing all of 11.x), but I don't recall if it manifested in 10.x. I guess it shouldn't be too hard to install various versions of FreeBSD to determine this, but it would be quite time-consuming. I'm not very familiar with FreeBSD-land, but I would hope there would be an easier way to determine when it changed than by installing old versions. Does FreeBSD have historic package repositories (containing headers, for instance) which one could consult instead?
Re: How to push using SSH and pull using HTTPS for all repos on GitHub?
Jeffrey Walton wrote: > On Sat, Aug 4, 2018 at 9:26 PM, Jonathan Nieder wrote: >> Jeffrey Walton wrote: >>> I'm having trouble setting up my ~/.gitconfig to push using SSH and >>> pull using HTTPS for all repos on GitHub. The idea is, no passwords on >>> pulls and only use the password for push. [...] >>>[url "ssh://g...@github.com/"] >>>insteadOf = https://github.com/ >> >> Does putting pushInsteadOf here work? > > Yes, that was the trick. > > Thank you very much. You're welcome. I should have asked: do you remember where you first looked for this answer in documentation? Maybe we can improve it. A few thoughts: 1. git-push(1) could get a CONFIGURATION section. 2. the description of url.*.insteadOf in git-config(1) could mention pushInsteadOf. The description of url.*.pushInstead is right after it today, but there is nothing guaranteeing that that will continue to be true. 3. Likewise, the description of remote..push could include a pointer. 4. Maybe there's a place to put some thoughts on this in the user-manual, too. It already mentions remote..push. What do you think? Thanks, Jonathan
Re: [PATCH] Makefile: enable DEVELOPER by default
Eric Sunshine wrote: > And, compilation warnings are not limited to old compilers. Even my > fully up-to-date FreeBSD 11.2 installation is not warning-free[1]. > > [1]: For instance: > utf8.c:486:28: warning: passing 'iconv_ibp *' (aka 'const char **') to > parameter > of type 'char **' discards qualifiers in nested pointer types > [-Wincompatible-pointer-types-discards-qualifiers] Oh, good catch! POSIX documents iconv has having signature size_t iconv(iconv_t cd, char **restrict inbuf, size_t *restrict inbytesleft, char **restrict outbuf, size_t *restrict outbytesleft); The Makefile explains # Define OLD_ICONV if your library has an old iconv(), where the second # (input buffer pointer) parameter is declared with type (const char **). which is implemented as #if defined(OLD_ICONV) || (defined(__sun__) && !defined(_XPG6)) typedef const char * iconv_ibp; #else typedef char * iconv_ibp; #endif config.mak.uname contains ifeq ($(uname_S),FreeBSD) NEEDS_LIBICONV = YesPlease OLD_ICONV = YesPlease So it looks like FreeBSD has modernized and we need to make that conditional in config.mak.uname on $(uname_R). Do you know which version of FreeBSD changed the signature? Care to write a patch? Thanks, Jonathan
Re: [PATCH] Makefile: enable DEVELOPER by default
On Sat, Aug 4, 2018 at 2:38 AM Duy Nguyen wrote: > On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder wrote: > > My main concern is not about them but about other > > people building from source in order to run (instead of to develop) > > Git, and by extension, the people they go to for help when it doesn't > > work. I have lots of bitter experience of -Werror being a support > > headache and leading to bad workarounds when someone upgrades their > > compiler and the build starts failing due to a new warning it has > > introduced. > > Even old compilers can also throw some silly, false positive warnings > (which now turn into errors) because they are not as smart as new > ones. And, compilation warnings are not limited to old compilers. Even my fully up-to-date FreeBSD 11.2 installation is not warning-free[1]. [1]: For instance: utf8.c:486:28: warning: passing 'iconv_ibp *' (aka 'const char **') to parameter of type 'char **' discards qualifiers in nested pointer types [-Wincompatible-pointer-types-discards-qualifiers]
[PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust
The previous behavior could incorrectly pass given a directory with a
filename containing a newline. This patch changes the implementation to
use `ls -A`, which is specified by POSIX. The output should be empty
exactly if the directory is empty.
The newly added unit test fails before this change and passes after it.
Signed-off-by: William Chargin
---
t/t-basic.sh| 29 +
t/test-lib-functions.sh | 2 +-
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/t/t-basic.sh b/t/t-basic.sh
index 34859fe4a..3885b26f9 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -821,6 +821,35 @@ test_expect_success 'tests clean up even on failures' "
EOF
"
+test_expect_success 'test_dir_is_empty behaves even in pathological cases' "
+ run_sub_test_lib_test \
+ dir-empty 'behavior of test_dir_is_empty' <<-\\EOF &&
+ test_expect_success 'should pass with actually empty directory' '
+ mkdir empty_dir &&
+ test_dir_is_empty empty_dir
+ '
+ test_expect_success 'should fail with a normal filename' '
+ mkdir nonempty_dir &&
+ touch nonempty_dir/some_file &&
+ test_must_fail test_dir_is_empty nonempty_dir
+ '
+ test_expect_success 'should fail with dot-newline-dot filename' '
+ mkdir pathological_dir &&
+ printf \"pathological_dir/.n.0\" | xargs -0 touch &&
+ test_must_fail test_dir_is_empty pathological_dir
+ '
+ test_done
+ EOF
+ check_sub_test_lib_test dir-empty <<-\\EOF
+ > ok 1 - should pass with actually empty directory
+ > ok 2 - should fail with a normal filename
+ > ok 3 - should fail with dot-newline-dot filename
+ > # passed all 3 test(s)
+ > 1..3
+ EOF
+"
+
+
# Basics of the basics
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2b2181dca..f7ff28ef6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -568,7 +568,7 @@ test_path_is_dir () {
# Check if the directory exists and is empty as expected, barf otherwise.
test_dir_is_empty () {
test_path_is_dir "$1" &&
- if test -n "$(ls -a1 "$1" | egrep -v '^\.\.?$')"
+ if test "$(ls -A1 "$1" | wc -c)" != 0
then
echo "Directory '$1' is not empty, it contains:"
ls -la "$1"
--
2.18.0.548.g79b975644
[PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust
While the `test_dir_is_empty` function appears correct in most normal use cases, it can fail when filenames contain newlines. I originally wrote this patch for the standalone Sharness library, but that library advises that such patches be sent to the Git mailing list first. William Chargin (1): t/test-lib: make `test_dir_is_empty` more robust t/t-basic.sh| 29 + t/test-lib-functions.sh | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) -- 2.18.0.548.g79b975644
Re: How to push using SSH and pull using HTTPS for all repos on GitHub?
On Sat, Aug 4, 2018 at 9:26 PM, Jonathan Nieder wrote: > Hi, > > Jeffrey Walton wrote: > >> I'm having trouble setting up my ~/.gitconfig to push using SSH and >> pull using HTTPS for all repos on GitHub. The idea is, no passwords on >> pulls and only use the password for push. >> >> I've got the first part of the equation using the following in my >> ~/.gitconfig (the ellipses are user info): >> >>$ cat ~/.gitconfig >>... >># Enforce SSH >>[url "ssh://g...@github.com/"] >>insteadOf = https://github.com/ > > Does putting pushInsteadOf here work? Yes, that was the trick. Thank you very much.
Re: [PATCH 1/4] line-log: demonstrate a bug with nearly-overlapping ranges
Hi,
Johannes Schindelin wrote:
> Currently, this test case throws an assertion:
>
> Assertion failed!
>
> Program: git.exe
> File: line-log.c, Line 71
>
> Signed-off-by: Johannes Schindelin
> ---
> t/t4211-line-log.sh | 17 +
> 1 file changed, 17 insertions(+)
Thanks for finding and demonstrating it. Can you say more about what
is going on in the test case? Alternatively, could it be squashed in
with the patch that fixes it?
The below will be more nitpicky:
[...]
> --- a/t/t4211-line-log.sh
> +++ b/t/t4211-line-log.sh
> @@ -115,4 +115,21 @@ test_expect_success 'range_set_union' '
> git log $(for x in $(test_seq 200); do echo -L $((2*x)),+1:c.c; done)
> '
>
> +q_to_lf () {
> + tr Q '\012'
> +}
> +
> +test_expect_failure 'close to overlapping ranges' '
> + test_seq 5 >a1.c &&
> + git add a1.c &&
> + git commit -m "5 lines" a1.c &&
It would be nice to use test_tick or test_commit for a more realistic
history (with time marching forward).
> + sed s/3/3QaQb/ tmp &&
> + mv tmp a1.c &&
> + git commit -m "2 more lines" a1.c &&
It's probably just me, but the bit with Q makes it hard for me to
follow. Maybe there's a simpler way?
"sed -e '3aa' -e '3ab'" works here, but I don't know how portable it
is. I'd be more tempted to do
test_write_lines 1 2 3 4 5 >a1.c &&
...
test_write_lines 1 2 3 a b 4 5 >a1.c &&
...
test_write_lines 1 2 3 a b c 4 5 >a1.c &&
...
which is concise and has obvious behavior.
Thanks and hope that helps,
Jonathan
Re: concurrent access to multiple local git repos is error prone
Hi Alexander, Alexander Mills wrote: > On Sat, Aug 4, 2018 at 2:47 PM, Alexander Mills > wrote: >> I assume that access more than 1 git repo concurrently on a local >> machine is not without errors. However this seems like a huge >> limitation or design flaw. >> >> Is my observation correct? Are there any plans to remove this limitation? [...] > Note, in my first paragraph, I should have said "If I have multiple > local git repos, and I run `git status -v` on them *concurrently*"... There's no known limitation of this kind, no. It sounds quite strange, and I've never heard of anything like that before. Do you have more details? Thanks and hope that helps, Jonathan
Re: [RFC PATCH 0/1] Introduce git-recover
Hi, Edward Thomson wrote: > I created a simple shell script a while back to help people recover > files that they deleted from their working directory (but had been added > to the repository), which looks for unreachable blobs in the object > database and places them in the working directory (either en masse, > interactively, or via command-line arguments). Cool! Most of this belongs in the commit message, which is part of why I always discourage having a separate cover letter in single-patch series. > This has been available at https://github.com/ethomson/git-recover for > about a year, and in that time, someone has suggested that I propose > this as part of git itself. So I thought I'd see if there's any > interest in this. > > If there is, I'd like to get a sense of the amount of work required to > make this suitable for inclusion. There are some larger pieces of work > required -- at a minimum, I think this requires: > > - Tests -- there are none, which is fine with me but probably less fine > for inclusion here. > - Documentation -- the current README is below but it will need proper > documentation that can be rendered into manpages, etc, by the tools. > - Remove bashisms -- there are many. One possible path in that direction would be to "stage" the code in contrib/ first, while documenting the intention of graduating to a command in git itself. Then the list can pitch in with those tasks. There are good reasons for a tool to exist outside of Git, so I wouldn't recommend this unless we have a clear plan for its graduation that we've agreed upon as a project, but thought I should mention it as a mechanism in case we decide to do that. The trend these days for Git commands has been to prefer to have them in C. Portable shell is a perfectly fine stopping point on the way there, though. My more fundamental main thought is separate from those logistics: how does this relate to "git fsck --lost-found"? What would your ideal interface to solve this problem look like? Can we make Git's commands complement each other in a good way to solve it well? Thanks, Jonathan
Re: How to push using SSH and pull using HTTPS for all repos on GitHub?
Hi, Jeffrey Walton wrote: > I'm having trouble setting up my ~/.gitconfig to push using SSH and > pull using HTTPS for all repos on GitHub. The idea is, no passwords on > pulls and only use the password for push. > > I've got the first part of the equation using the following in my > ~/.gitconfig (the ellipses are user info): > >$ cat ~/.gitconfig >... ># Enforce SSH >[url "ssh://g...@github.com/"] >insteadOf = https://github.com/ Does putting pushInsteadOf here work? Thanks and hope that helps, Jonathan
pk/rebase-in-c, was Re: What's cooking in git.git (Aug 2018, #01; Thu, 2)
Hi Junio, On Thu, 2 Aug 2018, Junio C Hamano wrote: > * pk/rebase-in-c (2018-07-30) 3 commits > - builtin/rebase: support running "git rebase " > - rebase: refactor common shell functions into their own file > - rebase: start implementing it as a builtin > > Rewrite of the "rebase" machinery in C. > > Will merge to 'next'. Please hold. I found several bugs in the third patch, and it will need to be fixed before sending another iteration. Ciao, Dscho
How to push using SSH and pull using HTTPS for all repos on GitHub?
I'm having trouble setting up my ~/.gitconfig to push using SSH and pull using HTTPS for all repos on GitHub. The idea is, no passwords on pulls and only use the password for push. I've got the first part of the equation using the following in my ~/.gitconfig (the ellipses are user info): $ cat ~/.gitconfig ... # Enforce SSH [url "ssh://g...@github.com/"] insteadOf = https://github.com/ [push] default = current The above pushes and pulls using SSH. Pulls only need HTTPS so I tried adding the following which does not work as expected: [pull "https://github.com/;] I've found several ways to break Git when trying to setup the HTTPS pull. My question is, how do I setup the HTTPS pull in my ~/.gitconfig? Thanks in advance.
[PATCH 1/4] line-log: demonstrate a bug with nearly-overlapping ranges
From: Johannes Schindelin
Currently, this test case throws an assertion:
Assertion failed!
Program: git.exe
File: line-log.c, Line 71
Signed-off-by: Johannes Schindelin
---
t/t4211-line-log.sh | 17 +
1 file changed, 17 insertions(+)
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 436b13ad2..61ff37430 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -115,4 +115,21 @@ test_expect_success 'range_set_union' '
git log $(for x in $(test_seq 200); do echo -L $((2*x)),+1:c.c; done)
'
+q_to_lf () {
+ tr Q '\012'
+}
+
+test_expect_failure 'close to overlapping ranges' '
+ test_seq 5 >a1.c &&
+ git add a1.c &&
+ git commit -m "5 lines" a1.c &&
+ sed s/3/3QaQb/ tmp &&
+ mv tmp a1.c &&
+ git commit -m "2 more lines" a1.c &&
+ sed s/4/cQ4/ tmp &&
+ mv tmp a1.c &&
+ git commit -m "1 more line" a1.c &&
+ git --no-pager log -L 1,3:a1.c -L 5,8:a1.c
+'
+
test_done
--
gitgitgadget
[PATCH 0/4] line-log: be more careful when adjusting multiple line ranges
I am a heavy user of git log -L In fact, I use the feature where
multiple ranges can be specified extensively, via a not-exactly-trivial
shell script function that takes the currently staged changes (or if none
are staged, the current unstanged changes) and turns them into the
corresponding commit history:
staged_log () { #
diff="$(git diff --cached -U1)"
test -n "$diff" ||
diff="$(git diff -U1)"
test -n "$diff" ||
die "No changes"
eval "git log $(echo "$diff" |
sed -ne '/^--- a\//{s/^-* a\/\(.*\)/'\''\1'\''/;x}' -e \
'/^@@ -/{s/^@@ -\([^, ]*\),\([^ ]*\).*/-L
\1,+\2/;G;s/\n/:/g;p}' |
tr '\n' ' ')"
}
This is an extremely useful way to look at the history, especially when
trying to fix up a commit deep in a long branch (or a thicket of branches).
Today, however, this method failed me, by greeting me with an assertion.
When I tried to paper over that assertion by joining line ranges that became
adjacent (or overlapping), it still produced a segmentation fault when the
line-log tried to print lines past the file contents.
So I had no choice but to fix this properly.
I still wanted to keep the optimization where multiple line ranges are
joined into a single one (I am convinced that this also affects the output,
where previously multiple hunks would have been displayed, but I ran out of
time to investigate this). This is the 3rd patch. It is not purely an
optimization, as the assertion would still trigger when line ranges could be
joined.
Now, I am fairly certain that the changes are correct, but given my track
record with off-by-one bugs (and once even an off-by-two bug), I would
really appreciate some thorough review of this code, in particular the
second one that is the actual bug fix. I am specifically interested in
reviews from people who know line-log.c pretty well and can tell me whether
the src[i].end > target[j].end is correct, or whether it should actually
have been a >= (I tried to wrap my head around this, but I would feel more
comfortable if a domain expert would analyze this, whistling, and looking
Eric's way).
Cc: Eric Sunshine sunsh...@sunshineco.com [sunsh...@sunshineco.com]
Johannes Schindelin (4):
line-log: demonstrate a bug with nearly-overlapping ranges
line-log: adjust start/end of ranges individually
line-log: optimize ranges by joining them when possible
line-log: convert an assertion to a full BUG() call
line-log.c | 18 +++---
t/t4211-line-log.sh | 17 +
2 files changed, 32 insertions(+), 3 deletions(-)
base-commit: 1d89318c48d233d52f1db230cf622935ac3c69fa
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-15%2Fdscho%2Fline-log-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-15/dscho/line-log-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/15
--
gitgitgadget
[PATCH 4/4] line-log: convert an assertion to a full BUG() call
From: Johannes Schindelin
The assertion in question really indicates a bug, when triggered, so we
might just as well use the sanctioned method to report it.
Signed-off-by: Johannes Schindelin
---
line-log.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/line-log.c b/line-log.c
index bc7ef69d6..0e09df9db 100644
--- a/line-log.c
+++ b/line-log.c
@@ -72,7 +72,9 @@ void range_set_append(struct range_set *rs, long a, long b)
rs->ranges[rs->nr-1].end = b;
return;
}
- assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a);
+ if (rs->nr > 0 && rs->ranges[rs->nr-1].end > a)
+ BUG("append %ld-%ld, after %ld-%ld?!?", a, b,
+ rs->ranges[rs->nr-1].start, rs->ranges[rs->nr-1].end);
range_set_append_unsafe(rs, a, b);
}
--
gitgitgadget
[PATCH 3/4] line-log: optimize ranges by joining them when possible
From: Johannes Schindelin
Technically, it is okay to have line ranges that touch (i.e. the end of
the first range ends just before the next range begins). However, it is
inefficient, and when the user provides such touching ranges via
multiple `-L` options, we already join them.
When we traverse the history, though, we never join ranges, even they
become "touchy-feely" due to added lines (which are "removed" from
line-log's point of view because it traverses the commit history into
the past).
Let's optimize also this case.
Signed-off-by: Johannes Schindelin
---
line-log.c | 4
t/t4211-line-log.sh | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/line-log.c b/line-log.c
index d8d09b5ee..bc7ef69d6 100644
--- a/line-log.c
+++ b/line-log.c
@@ -68,6 +68,10 @@ void range_set_append_unsafe(struct range_set *rs, long a,
long b)
void range_set_append(struct range_set *rs, long a, long b)
{
+ if (rs->nr > 0 && rs->ranges[rs->nr-1].end + 1 == a) {
+ rs->ranges[rs->nr-1].end = b;
+ return;
+ }
assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a);
range_set_append_unsafe(rs, a, b);
}
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 61ff37430..ebaf5ea86 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -119,7 +119,7 @@ q_to_lf () {
tr Q '\012'
}
-test_expect_failure 'close to overlapping ranges' '
+test_expect_success 'close to overlapping ranges' '
test_seq 5 >a1.c &&
git add a1.c &&
git commit -m "5 lines" a1.c &&
--
gitgitgadget
[PATCH 2/4] line-log: adjust start/end of ranges individually
From: Johannes Schindelin
When traversing commits and adjusting the ranges, things can get really
tricky. For example, when the line range of interest encloses several
hunks of a commit, the line range can actually shrink.
Currently, range_set_shift_diff() does not anticipate that scenario and
blindly adjusts start and end by the same offset ("shift" the range).
This can lead to a couple of surprising issues, such as assertions in
range_set_append() (when the end of a given range is not adjusted
properly, it can point after the start of the next range) or even
segmentation faults (when t_end in the loop of dump_diff_hacky_one()
points outside the valid line range).
Let's fix this by adjusting the start and the end offsets individually.
Signed-off-by: Johannes Schindelin
---
line-log.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/line-log.c b/line-log.c
index 72a5fed66..d8d09b5ee 100644
--- a/line-log.c
+++ b/line-log.c
@@ -427,7 +427,7 @@ static void range_set_shift_diff(struct range_set *out,
struct diff_ranges *diff)
{
unsigned int i, j = 0;
- long offset = 0;
+ long offset = 0, start_offset;
struct range *src = rs->ranges;
struct range *target = diff->target.ranges;
struct range *parent = diff->parent.ranges;
@@ -438,7 +438,13 @@ static void range_set_shift_diff(struct range_set *out,
- (target[j].end-target[j].start);
j++;
}
- range_set_append(out, src[i].start+offset, src[i].end+offset);
+ start_offset = offset;
+ while (j < diff->target.nr && src[i].end > target[j].end) {
+ offset += (parent[j].end-parent[j].start)
+ - (target[j].end-target[j].start);
+ j++;
+ }
+ range_set_append(out, src[i].start+start_offset,
src[i].end+offset);
}
}
--
gitgitgadget
Re: concurrent access to multiple local git repos is error prone
Note, in my first paragraph, I should have said "If I have multiple local git repos, and I run `git status -v` on them *concurrently*"... -alex On Sat, Aug 4, 2018 at 2:47 PM, Alexander Mills wrote: > If I have multiple local git repos, and I run `git status -v` on them, > sometimes I don't get any stdout for the command. This is highly > reproducible. > > I assume that access more than 1 git repo concurrently on a local > machine is not without errors. However this seems like a huge > limitation or design flaw. > > Is my observation correct? Are there any plans to remove this limitation? > > My use case - I create a lot of developer tools and more than once I > have hit this limitation...I have to create a queue with concurrency > of 1 to run commands on git repos. It's very strange and > counterintuitive to have to do this. > > > -- > Alexander D. Mills > ¡¡¡ New cell phone number: (415)730-1805 !!! > alexander.d.mi...@gmail.com > > www.linkedin.com/pub/alexander-mills/b/7a5/418/ -- Alexander D. Mills ¡¡¡ New cell phone number: (415)730-1805 !!! alexander.d.mi...@gmail.com www.linkedin.com/pub/alexander-mills/b/7a5/418/
concurrent access to multiple local git repos is error prone
If I have multiple local git repos, and I run `git status -v` on them, sometimes I don't get any stdout for the command. This is highly reproducible. I assume that access more than 1 git repo concurrently on a local machine is not without errors. However this seems like a huge limitation or design flaw. Is my observation correct? Are there any plans to remove this limitation? My use case - I create a lot of developer tools and more than once I have hit this limitation...I have to create a queue with concurrency of 1 to run commands on git repos. It's very strange and counterintuitive to have to do this. -- Alexander D. Mills ¡¡¡ New cell phone number: (415)730-1805 !!! alexander.d.mi...@gmail.com www.linkedin.com/pub/alexander-mills/b/7a5/418/
[PATCH 0/1] Support git pull --rebase=i
The patch [https://github.com/git-for-windows/git/commit/4aa8b8c82] that introduced support for pull --rebase= into the Git for Windows project still allowed the very convenient abbreviation git pull --rebase=i which was later lost when it was ported to the builtin git pull, and it was not introduced before the patch eventually made it into Git as f5eb87b98dd (pull: allow interactive rebase with --rebase=interactive, 2016-01-13). However, it is really a useful short hand for the occasional rebasing pull on branches that do not usually want to be rebased. So let's reintroduce this convenience, at long last. Johannes Schindelin (1): pull --rebase=: allow single-letter abbreviations for the type builtin/pull.c | 6 +++--- t/t5520-pull.sh | 12 2 files changed, 15 insertions(+), 3 deletions(-) base-commit: 1d89318c48d233d52f1db230cf622935ac3c69fa Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-14%2Fdscho%2Fpull-rebase-i-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-14/dscho/pull-rebase-i-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/14 -- gitgitgadget
[PATCH 1/1] pull --rebase=: allow single-letter abbreviations for the type
From: Johannes Schindelin Git for Windows' original 4aa8b8c8283 (Teach 'git pull' to handle --rebase=interactive, 2011-10-21) had support for the very convenient abbreviation git pull --rebase=i which was later lost when it was ported to the builtin `git pull`, and it was not introduced before the patch eventually made it into Git as f5eb87b98dd (pull: allow interactive rebase with --rebase=interactive, 2016-01-13). However, it is *really* a useful short hand for the occasional rebasing pull on branches that do not usually want to be rebased. So let's reintroduce this convenience, at long last. Signed-off-by: Johannes Schindelin --- builtin/pull.c | 6 +++--- t/t5520-pull.sh | 12 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 4e7893539..53bc5facf 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -48,11 +48,11 @@ static enum rebase_type parse_config_rebase(const char *key, const char *value, return REBASE_FALSE; else if (v > 0) return REBASE_TRUE; - else if (!strcmp(value, "preserve")) + else if (!strcmp(value, "preserve") || !strcmp(value, "p")) return REBASE_PRESERVE; - else if (!strcmp(value, "merges")) + else if (!strcmp(value, "merges") || !strcmp(value, "m")) return REBASE_MERGES; - else if (!strcmp(value, "interactive")) + else if (!strcmp(value, "interactive") || !strcmp(value, "i")) return REBASE_INTERACTIVE; if (fatal) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 68aa5f034..5e501c8b0 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -475,10 +475,22 @@ test_expect_success 'pull.rebase=interactive' ' false EOF test_set_editor "$TRASH_DIRECTORY/fake-editor" && + test_when_finished "test_might_fail git rebase --abort" && test_must_fail git pull --rebase=interactive . copy && test "I was here" = "$(cat fake.out)" ' +test_expect_success 'pull --rebase=i' ' + write_script "$TRASH_DIRECTORY/fake-editor" <<-\EOF && + echo I was here, too >fake.out && + false + EOF + test_set_editor "$TRASH_DIRECTORY/fake-editor" && + test_when_finished "test_might_fail git rebase --abort" && + test_must_fail git pull --rebase=i . copy && + test "I was here, too" = "$(cat fake.out)" +' + test_expect_success 'pull.rebase=invalid fails' ' git reset --hard before-preserve-rebase && test_config pull.rebase invalid && -- gitgitgadget
[PATCH v2] t4150: fix broken test for am --scissors
Tests for "git am --[no-]scissors" [1] work in the following way: 1. Create files with commit messages 2. Use these files to create expected commits 3. Generate eml file with patch from expected commits 4. Create commits using git am with these eml files 5. Compare these commits with expected The test for "git am --scissors" is supposed to take a message with a scissors line and demonstrate that the subject line from the e-mail itself is overridden by the in-body "Subject:" header and that only text below the scissors line is included in the commit message of the commit created by the invocation of "git am --scissors". However, the setup of the test incorrectly uses a commit without the scissors line and in-body "Subject:" header in the commit message, and thus, creates eml file not suitable for testing of "git am --scissors". This can be checked by intentionally breaking is_scissors_line function in mailinfo.c, for example, by changing string ">8", which is used by the test. With such change the test should fail, but does not. Fix broken test by generating eml file with scissors line and in-body header "Subject:". Since the two tests for --scissors and --no-scissors options are there to test cutting or keeping the commit message, update both tests to change the test file in the same way, which allows us to generate only one eml file to be passed to git am. To clarify the intention of the test, give files and tags more explicit names. [1]: introduced in bf72ac17d (t4150: tests for am --[no-]scissors, 2015-07-19) Signed-off-by: Andrei Rybak --- Applies on top of 980a3d3dd (Merge branch 'pt/am-tests', 2015-08-03). This patch is also available at https://github.com/rybak/git fix-am-scissors-test-v2 Changes since v1: - Reword commit message after feedback from Junio - Keep the empty line under scissors in the test e-mail, as it does not affect the test t/t4150-am.sh | 39 --- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/t/t4150-am.sh b/t/t4150-am.sh index e9b6f8158..bb2d951a7 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -67,13 +67,15 @@ test_expect_success 'setup: messages' ' EOF - cat >scissors-msg <<-\EOF && - Test git-am with scissors line + cat >msg-without-scissors-line <<-\EOF && + Test that git-am --scissors cuts at the scissors line This line should be included in the commit message. EOF - cat - scissors-msg >no-scissors-msg <<-\EOF && + printf "Subject: " >subject-prefix && + + cat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\EOF && This line should not be included in the commit message with --scissors enabled. - - >8 - - remove everything above this line - - >8 - - @@ -150,18 +152,17 @@ test_expect_success setup ' } >patch1-hg.eml && - echo scissors-file >scissors-file && - git add scissors-file && - git commit -F scissors-msg && - git tag scissors && - git format-patch --stdout scissors^ >scissors-patch.eml && + echo file >file && + git add file && + git commit -F msg-without-scissors-line && + git tag scissors-used && git reset --hard HEAD^ && - echo no-scissors-file >no-scissors-file && - git add no-scissors-file && - git commit -F no-scissors-msg && - git tag no-scissors && - git format-patch --stdout no-scissors^ >no-scissors-patch.eml && + echo file >file && + git add file && + git commit -F msg-with-scissors-line && + git tag scissors-not-used && + git format-patch --stdout scissors-not-used^ >patch-with-scissors-line.eml && git reset --hard HEAD^ && sed -n -e "3,\$p" msg >file && @@ -418,10 +419,10 @@ test_expect_success 'am --scissors cuts the message at the scissors line' ' rm -fr .git/rebase-apply && git reset --hard && git checkout second && - git am --scissors scissors-patch.eml && + git am --scissors patch-with-scissors-line.eml && test_path_is_missing .git/rebase-apply && - git diff --exit-code scissors && - test_cmp_rev scissors HEAD + git diff --exit-code scissors-used && + test_cmp_rev scissors-used HEAD ' test_expect_success 'am --no-scissors overrides mailinfo.scissors' ' @@ -429,10 +430,10 @@ test_expect_success 'am --no-scissors overrides mailinfo.scissors' ' git reset --hard && git checkout second && test_config mailinfo.scissors true && - git am --no-scissors no-scissors-patch.eml && + git am --no-scissors patch-with-scissors-line.eml && test_path_is_missing .git/rebase-apply && - git diff --exit-code no-scissors && - test_cmp_rev no-scissors HEAD + git diff --exit-code scissors-not-used && + test_cmp_rev scissors-not-used HEAD ' test_expect_success 'setup: new author and
Re: [RFC PATCH 1/1] recover: restoration of deleted worktree files
Hi, Robert P. J. Day wrote: > On Sat, 4 Aug 2018, Junio C Hamano wrote: >> In other words, I think this patch can be a fine addition to >> somebody else's project (i.e. random collection of scripts that may >> help Git users), so let's see how I can offer comments/inputs to >> help you improve it. So I won't comment on lang, log message, or >> shell scripting style---these are project convention and the >> git-core convention won't be relevant to this patch. > > not sure how relevant this is, but fedora bundles a bunch of neat > utilities into two packages: git-tools and git-extras. i have no idea > what relationship those packages have to official git, or who decides > what goes into them. For anyone curious, those packages (git-extras and git-tools) are both entirely separate projects upstream and in the fedora packaging. A git-recover script may well be a good fit in one of those upstream projects. The git-(extras|tools) package names are a bit confusing IMO. But it's probably more confusing that they each add a number of git-* commands in the default PATH the way they're packaged. We do package some bits from contrib/ (e.g. completion, subtree, etc.) in the fedora git packages. We don't add scripts and commands from outside of the git tarballs as part of the fedora git package, though. So far, I don't recall anyone filing a bug report about commands from git-extras or git-tools against git. So it seems that users of those additional packages aren't being confused, thankfully. -- Todd ~~ Between two evils, I always pick the one I never tried before. -- Mae West
Re: [PATCH v9 2/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875
Max Kirillov writes:
> On Sat, Aug 04, 2018 at 08:34:08AM +0200, Duy Nguyen wrote:
>> On Fri, Jul 27, 2018 at 5:50 AM Max Kirillov wrote:
>>> + if (max_request_buffer < req_len) {
>>> + die("request was larger than our maximum size (%lu): "
>>> + "%" PRIuMAX "; try setting GIT_HTTP_MAX_REQUEST_BUFFER",
>>> + max_request_buffer, (uintmax_t)req_len);
>>
>> Please mark these strings for translation with _().
>
> It has been discussed in [1]. Since it is not a local user
> facing part, probably should not be translated.
>
> [1] https://public-inbox.org/git/20180610150727.GE27650@jessie.local/
I'd support that design decision, FWIW.
Re: [PATCH] Makefile: enable DEVELOPER by default
Duy Nguyen writes: > On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder wrote: >> My main concern is not about them but about other >> people building from source in order to run (instead of to develop) >> Git, and by extension, the people they go to for help when it doesn't >> work. I have lots of bitter experience of -Werror being a support >> headache and leading to bad workarounds when someone upgrades their >> compiler and the build starts failing due to a new warning it has >> introduced. > > Even old compilers can also throw some silly, false positive warnings > (which now turn into errors) because they are not as smart as new > ones. I agree with both of the above. I do not think the pros-and-cons are in favor of forcing the developer bit to everybody, even though I am sympathetic to the desire to see people throw fewer bad changes that waste review bandwidth by not compiling or passing its own tests at us.
Re: [RFC PATCH 7/7] diff/am: enhance diff format to use */~ for moved lines
Stefan Beller writes: > Try it out via > ./git-format-patch --mark-moved 15ef69314d^..15ef69314d > to see if you like it. > > This separates the coloring decision from the detection of moved lines. > When giving --mark-moved, move detection is still performed and the output > markers are adjusted to */~ for new and old code. > > git-apply and git-am will also accept these patches by rewriting those > signs back to +/-. > > Signed-off-by: Stefan Beller > --- This does not have anything to do with the range-diff topic, but would stand on its own merit. I have a mixed feeling about this. If you need to convince "GNU patch" maintainers to accept these two signs, then probably it is not worth the battle of incompatiblity. If it is truly a worthy innovation, they would follow suit, which is how they learned to take our renaming diffs without us prodding them. I just do not get the gut feeling that it would happen for this particular thing, and I am not convinced myself enough to sell this to "patch" maintainers and expect to be taken seriously. When reviewing anything complex that would be helped by moved code highlighting, I do not think a normal person would choose to review such a change only inside MUA. I certainly won't. I'd rather apply the patch and view it within a larger context than the piece of e-mail that was originally sent offers, with better tools like -W and --color-moved applied locally. So in that sense, I do not think I'd appreciate lines that begin with '~'/'*' as different kind of '-'/'+', as helpful hints; at least until my eyes get used to them, they would only appear as distraction. In other words, I have this nagging suspicion that people who suggested to you that this would help in e-mail workflow are misguided and they do not understand e-mail workflow in the first place, but perhaps it is just me. Thanks.
Re: [PATCH 0/7] improve range-diffs coloring and [RFC] move detection
Stefan Beller writes: > This builds on top of sb/range-diff-colors, which builds on js/range-diff. As another round of js/range-diff is expected, according to I will refrain from queuing this right now. Possible conflict resolution that won't be reusable when the base one is rerolled and this and another topic that depend on the current round of js/range-diff are rebased on top is not something I can spend my time on this week.
Re: [RFC PATCH 1/1] recover: restoration of deleted worktree files
Edward Thomson writes: > In any case, it sounds like you're not particularly interested in > this, although I certainly appreciate you taking the time to suggest > improvements despite that. There's some good feedback there. Not in its current shape. But do not take this in a wrong way. It may be useful in a third-party script collection in its current shape already. More importantly, I am not opposed to have a "resurrect" utility in the core distribution. It just has to be a lot better than what "grep -e 'I think I wrote this string' .git/lost-found/other/*" gives us. Filename discovery (perhaps from lost trees, which was the idea I wrote in the message I am responding to, but others may come up with better alternatibve approaches) is a must, but not primarily because such a grep won't find the path to which the contents should go. When a user says "I think I wrote this string in the file I am looking for", s/he already knows what s/he wants to recover (i.e. it was a README file at the top-level). Filename discovery is a must because grepping in the raw blob contents without smudge filter chain applied may not find what we want in the first place, and for that to happen, we need to have a filename. Side note. That may mean that even working in the do-recover mode, the script may want to take a filename, letting the user to say "pretend all lost blobs are of this type, as that is the type of the blob I just lost and am interested in, and a filename will help you find an appropriate smudge and/or textconv filter to help me" That makes me realize that I did not mention one more thing, other than the "interactibve loop", I did like in the script over what lost-found gives us: smudge filter support. I do not very often work with contents that needs clean/smudge other than in one project (obviously not "git.git"), and I can see how it is essential in helping the user to find the contents the user is looking for. Thanks.
Re: [RFC PATCH 1/1] recover: restoration of deleted worktree files
On Sat, 4 Aug 2018, Junio C Hamano wrote: > Edward Thomson writes: > > > Introduce git-recover, a simple script to aide in restoration of > > deleted worktree files. This will look for unreachable blobs in > > the object database and prompt users to restore them to disk, > > either interactively or on the command-line. > > git-recover.sh | 311 > > + > > 1 file changed, 311 insertions(+) > > create mode 100755 git-recover.sh > > My first reaction was to say that I am not going to take a new > command written only for bash with full bashism, even if it came > with docs, tests nor Makefile integration, for Git itself. Then I > reconsidered, as not everything related to Git is git-core, and all > of the above traits are sign of this patch _not_ meant for git-core. > > In other words, I think this patch can be a fine addition to > somebody else's project (i.e. random collection of scripts that may > help Git users), so let's see how I can offer comments/inputs to > help you improve it. So I won't comment on lang, log message, or > shell scripting style---these are project convention and the > git-core convention won't be relevant to this patch. not sure how relevant this is, but fedora bundles a bunch of neat utilities into two packages: git-tools and git-extras. i have no idea what relationship those packages have to official git, or who decides what goes into them. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca/dokuwiki Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: [RFC PATCH 1/1] recover: restoration of deleted worktree files
On Sat, Aug 04, 2018 at 08:54:49AM -0700, Junio C Hamano wrote: > > My first reaction was to say that I am not going to take a new > command written only for bash with full bashism, even if it came > with docs, tests nor Makefile integration, for Git itself. Then I > reconsidered, as not everything related to Git is git-core, and all > of the above traits are sign of this patch _not_ meant for git-core. Yes, obviously I was not suggesting that this would be mergeable with the bashims, as I mentioned in my cover letter. In any case, it sounds like you're not particularly interested in this, although I certainly appreciate you taking the time to suggest improvements despite that. There's some good feedback there. Cheers- -ed
Re: [RFC PATCH 1/1] recover: restoration of deleted worktree files
Edward Thomson writes:
> Introduce git-recover, a simple script to aide in restoration of deleted
> worktree files. This will look for unreachable blobs in the object
> database and prompt users to restore them to disk, either interactively
> or on the command-line.
> ---
> git-recover.sh | 311
> +
> 1 file changed, 311 insertions(+)
> create mode 100755 git-recover.sh
My first reaction was to say that I am not going to take a new
command written only for bash with full bashism, even if it came
with docs, tests nor Makefile integration, for Git itself. Then I
reconsidered, as not everything related to Git is git-core, and all
of the above traits are sign of this patch _not_ meant for git-core.
In other words, I think this patch can be a fine addition to
somebody else's project (i.e. random collection of scripts that may
help Git users), so let's see how I can offer comments/inputs to
help you improve it. So I won't comment on lang, log message, or
shell scripting style---these are project convention and the git-core
convention won't be relevant to this patch.
> diff --git a/git-recover.sh b/git-recover.sh
> new file mode 100755
> index 0..651d4116f
> --- /dev/null
> +++ b/git-recover.sh
> @@ -0,0 +1,311 @@
> +#!/usr/bin/env bash
> +#
> +# This program helps recover files in your repository that were deleted
> +# from the working tree.
> +#
> +# Copyright (c) 2017-2018 Edward Thomson.
> +
> +set -e
> +
> +IFS=$'\n'
> +
> +PROGNAME=$(echo "$0" | sed -e 's/.*\///')
> +GIT_DIR=$(git rev-parse --git-dir)
> +
> +DO_RECOVER=0
> +DO_FULL=0
> +DO_INTERACTIVE=0
> +BLOBS=()
> +FILENAMES=()
> +
> +function die_usage {
> + echo "usage: $PROGNAME [-a] [-i] [--full] [ [-f ] ...]"
> >&2
> + exit 1
> +}
> +
> +while [[ $# -gt 0 ]]; do
> + case "$1" in
> + -a|--all)
> + DO_RECOVER=1
> + ;;
> + -i|--interactive)
> + DO_INTERACTIVE=1
> + ;;
> + --full)
> + DO_FULL=1
> + ;;
> + *)
> + if [ "${1:0:1}" == "-" ]; then
> + echo "$PROGNAME: unknown argument: $1" >&2
> + die_usage
> + fi
> + BLOBS+=("$1")
> +
> + shift
> + if [ "$1" == "-f" ] || [ "$1" == "--filename" ]; then
> + shift
> + if [ $# == 0 ]; then
> + die_usage
> + fi
> + FILENAMES+=("$1")
> + shift
> + else
> + FILENAMES+=("")
> + fi
You do not want to take "--file=Makefile" (i.e. abbreviated option
name, and value as part of the option arg after '=')?
> + continue
> + ;;
> + esac
> + shift
> +done
So, as a user, I can run this with "-a" but no blob object names to
run it in DO_RECOVER mode, or I can give one or more "blob spec"
where I say object id, optionally followed by one "-f filename"; in
the latter mode, BLOBS[] and FILENAMES[] array would have the same
number of elements, corresponding to each other.
> +if [ ${#BLOBS[@]} != 0 ] && [ $DO_RECOVER == 1 ]; then
> + die_usage
> +elif [ ${#BLOBS[@]} != 0 ]; then
> + DO_RECOVER=1
> +fi
If I did not say "-a" but did not give "blob spec", then I am
implicitly asking for "-a" to work in DO_RECOVER mode.
I think I understood what the program wants to do so far.
> +case "$OSTYPE" in
> + darwin*|freebsd*) IS_BSD=1 ;;
> + *) IS_BSD=0 ;;
> +esac
> +
> +function expand_given_blobs() {
> + for i in "${!BLOBS[@]}"; do
> + ID=$(git rev-parse --verify "${BLOBS[$i]}" 2>/dev/null || true)
> +
> + if [ -z "$ID" ]; then
> + echo "$PROGNAME: ${BLOBS[$i]} is not a valid object."
> 1>&2
> + exit 1
> + fi
> +
> + TYPE=$(git cat-file -t "${ID}" 2>/dev/null || true)
An earlier "set -e" makes "|| true" ugliness required. I suspect
use of "set -e" overall is a loss (vs explicit error checking).
> + if [ "$TYPE" != "blob" ]; then
> + echo "$PROGNAME: ${BLOBS[$i]} is not a blob." 1>&2
> + exit
> + fi
A user may have given us 11f5bcd9 and this function makes sure such
an object exists in the object store *and* is a blob. Otherwise
it dies. The main objective of this function is to turn that user
supplied object name to a full hex that is known to refer to an
existing blob.
> + BLOBS[$i]=$ID
> + done
I find a disconnect between this being a loop and the attiude "we
won't tolerate any erroneous input". If a user is feeding dozens of
blob object names, wouldn't it be more helpful to give a warning, go
on and help the user with the rest?
> +}
> +
> +# find all the unreachable blobs
> +function find_unreachable() {
> + FULLNESS="--no-full"
> +
> + if [ $DO_FULL == 1 ]; then
[RFC PATCH 1/1] recover: restoration of deleted worktree files
Introduce git-recover, a simple script to aide in restoration of deleted
worktree files. This will look for unreachable blobs in the object
database and prompt users to restore them to disk, either interactively
or on the command-line.
---
git-recover.sh | 311 +
1 file changed, 311 insertions(+)
create mode 100755 git-recover.sh
diff --git a/git-recover.sh b/git-recover.sh
new file mode 100755
index 0..651d4116f
--- /dev/null
+++ b/git-recover.sh
@@ -0,0 +1,311 @@
+#!/usr/bin/env bash
+#
+# This program helps recover files in your repository that were deleted
+# from the working tree.
+#
+# Copyright (c) 2017-2018 Edward Thomson.
+
+set -e
+
+IFS=$'\n'
+
+PROGNAME=$(echo "$0" | sed -e 's/.*\///')
+GIT_DIR=$(git rev-parse --git-dir)
+
+DO_RECOVER=0
+DO_FULL=0
+DO_INTERACTIVE=0
+BLOBS=()
+FILENAMES=()
+
+function die_usage {
+ echo "usage: $PROGNAME [-a] [-i] [--full] [ [-f ] ...]"
>&2
+ exit 1
+}
+
+while [[ $# -gt 0 ]]; do
+ case "$1" in
+ -a|--all)
+ DO_RECOVER=1
+ ;;
+ -i|--interactive)
+ DO_INTERACTIVE=1
+ ;;
+ --full)
+ DO_FULL=1
+ ;;
+ *)
+ if [ "${1:0:1}" == "-" ]; then
+ echo "$PROGNAME: unknown argument: $1" >&2
+ die_usage
+ fi
+ BLOBS+=("$1")
+
+ shift
+ if [ "$1" == "-f" ] || [ "$1" == "--filename" ]; then
+ shift
+ if [ $# == 0 ]; then
+ die_usage
+ fi
+ FILENAMES+=("$1")
+ shift
+ else
+ FILENAMES+=("")
+ fi
+ continue
+ ;;
+ esac
+ shift
+done
+
+if [ ${#BLOBS[@]} != 0 ] && [ $DO_RECOVER == 1 ]; then
+ die_usage
+elif [ ${#BLOBS[@]} != 0 ]; then
+ DO_RECOVER=1
+fi
+
+case "$OSTYPE" in
+ darwin*|freebsd*) IS_BSD=1 ;;
+ *) IS_BSD=0 ;;
+esac
+
+function expand_given_blobs() {
+ for i in "${!BLOBS[@]}"; do
+ ID=$(git rev-parse --verify "${BLOBS[$i]}" 2>/dev/null || true)
+
+ if [ -z "$ID" ]; then
+ echo "$PROGNAME: ${BLOBS[$i]} is not a valid object."
1>&2
+ exit 1
+ fi
+
+ TYPE=$(git cat-file -t "${ID}" 2>/dev/null || true)
+
+ if [ "$TYPE" != "blob" ]; then
+ echo "$PROGNAME: ${BLOBS[$i]} is not a blob." 1>&2
+ exit
+ fi
+
+ BLOBS[$i]=$ID
+ done
+}
+
+# find all the unreachable blobs
+function find_unreachable() {
+ FULLNESS="--no-full"
+
+ if [ $DO_FULL == 1 ]; then FULLNESS="--full"; fi
+
+ BLOBS=($(git fsck --unreachable --no-reflogs \
+ "${FULLNESS}" --no-progress | sed -ne 's/^unreachable blob
//p'))
+}
+
+function read_one_file {
+ BLOB=$1
+ FILTER_NAME=$2
+ ARGS=()
+
+ if [ -z "$FILTER_NAME" ]; then
+ ARGS+=("blob")
+ else
+ ARGS+=("--filters" "--path=$FILTER_NAME")
+ fi
+
+ git cat-file "${ARGS[@]}" "$BLOB"
+}
+
+function write_one_file {
+ BLOB=$1
+ FILTER_NAME=$2
+ OUTPUT_NAME=$3
+
+ ABBREV=$(git rev-parse --short "${BLOB}")
+
+ echo -n "Writing $ABBREV: "
+ read_one_file "$BLOB" "$FILTER_NAME" > "$OUTPUT_NAME"
+ echo "$OUTPUT_NAME."
+}
+
+function unique_filename {
+ if [ ! -f "${BLOB}" ]; then
+ echo "$BLOB"
+ else
+ cnt=1
+ while true
+ do
+ fn="${BLOB}~${cnt}"
+ if [ ! -f "${fn}" ]; then
+ echo "${fn}"
+ break
+ fi
+ cnt=$((cnt+1))
+ done
+ fi
+}
+
+function write_recoverable {
+ for i in "${!BLOBS[@]}"; do
+ BLOB=${BLOBS[$i]}
+ FILTER_NAME=${FILENAMES[$i]}
+ OUTPUT_NAME=${FILENAMES[$i]:-$(unique_filename)}
+
+ write_one_file "$BLOB" "$FILTER_NAME" "$OUTPUT_NAME"
+ done
+}
+
+function file_time {
+ if [ $IS_BSD == 1 ]; then
+ stat -f %c "$1"
+ else
+ stat -c %Y "$1"
+ fi
+}
+
+function timestamp_to_s {
+ if [ $IS_BSD == 1 ]; then
+ date -r "$1"
+ else
+ date -d @"$1"
+ fi
+}
+
+function sort_by_timestamp {
+ # sort blobs in loose objects by their timestamp (packed blobs last)
+ BLOB_AND_TIMESTAMPS=($(for BLOB in "${BLOBS[@]}"; do
+ LOOSE="${BLOB::2}/${BLOB:2}"
+ TIME=$(file_time "$GIT_DIR/objects/$LOOSE" 2>/dev/null || true)
+ echo "$BLOB $TIME"
+
[RFC PATCH 0/1] Introduce git-recover
Hello- I created a simple shell script a while back to help people recover files that they deleted from their working directory (but had been added to the repository), which looks for unreachable blobs in the object database and places them in the working directory (either en masse, interactively, or via command-line arguments). This has been available at https://github.com/ethomson/git-recover for about a year, and in that time, someone has suggested that I propose this as part of git itself. So I thought I'd see if there's any interest in this. If there is, I'd like to get a sense of the amount of work required to make this suitable for inclusion. There are some larger pieces of work required -- at a minimum, I think this requires: - Tests -- there are none, which is fine with me but probably less fine for inclusion here. - Documentation -- the current README is below but it will need proper documentation that can be rendered into manpages, etc, by the tools. - Remove bashisms -- there are many. Again, this may not be particularly interesting, but I thought I'd send it along in case it is. Cheers- -ed --- git-recover allows you to recover some files that you've accidentally deleted from your working directory. It helps you find files that exist in the repository's object database - because you ran git add - but were never committed. Getting Started --- The simplest way to use git-recover is in interactive mode - simply run `git-recover -i` and it will show you all the files that you can recover and prompt you to act. Using git-recover - Running git-recover without any arguments will list all the files (git "blobs") that were recently orphaned, by their ID. (Their filename is not known.) You can examine these blobs by running `git show `. If you find one that you want to recover, you can provide the ID as the argument to git-recover. You can specify the `--filename` option to write the file out and apply any filters that are set up in the repository. For example: git-recover 38762cf7f55934b34d179ae6a4c80cadccbb7f0a \ --filename shattered.pdf You can also specify multiple files to recover, each with an optional output filename: git-recover 38762c --filename one.txt cafebae --filename bae.txt If you want to recover _all_ the orphaned blobs in your repository, run `git-recover --all`. This will write all the orphaned files to the current working directory, so it's best to run this inside a temporary directory beneath your working directory. For example: mkdir _tmp && cd _tmp && git-recover --all By default, git-recover limits itself to recently created orphaned blobs. If you want to see _all_ orphaned files that have been created in your repository (but haven't yet been garbage collected), you can run: git-recover --full Options --- git-recover [-a] [-i] [--full] [ [-f ] ...] -a, --all Write all orphaned blobs to the current working directory. Each file will be named using its 40 character object ID. -i, --interactive Display information about each orphaned blob and prompt to recover it. --full List or recover all orphaned blobs, even those that are in packfiles. By default, `git-recover` will only look at loose object files, which limits it to the most recently created files. Examining packfiles may be slow, especially in large repositories. The object ID (or its abbreviation) to recover. The file will be written to the current working directory and named using its 40 character object ID, unless the `-f` option is specified. -f , --filename When specified after an object ID, the file written will use this filename. In addition, any filters (for example: CRLF conversion or Git-LFS) will be run according to the `gitattributes` configuration. Edward Thomson (1): recover: restoration of deleted worktree files git-recover.sh | 311 + 1 file changed, 311 insertions(+) create mode 100755 git-recover.sh -- 2.0.0 (libgit2)
Re: [PATCH v9 2/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875
On Sat, Aug 04, 2018 at 08:34:08AM +0200, Duy Nguyen wrote:
> On Fri, Jul 27, 2018 at 5:50 AM Max Kirillov wrote:
>> + if (max_request_buffer < req_len) {
>> + die("request was larger than our maximum size (%lu): "
>> + "%" PRIuMAX "; try setting GIT_HTTP_MAX_REQUEST_BUFFER",
>> + max_request_buffer, (uintmax_t)req_len);
>
> Please mark these strings for translation with _().
It has been discussed in [1]. Since it is not a local user
facing part, probably should not be translated.
[1] https://public-inbox.org/git/20180610150727.GE27650@jessie.local/
--
Max
Re: [PATCH v2 2/2] doc hash-function-transition: pick SHA-256 as NewHash
Subject: doc hash-function-transition: pick SHA-256 as NewHash >From a security perspective, it seems that SHA-256, BLAKE2, SHA3-256, K12, and so on are all believed to have similar security properties. All are good options from a security point of view. SHA-256 has a number of advantages: * It has been around for a while, is widely used, and is supported by just about every single crypto library (OpenSSL, mbedTLS, CryptoNG, SecureTransport, etc). * When you compare against SHA1DC, most vectorized SHA-256 implementations are indeed faster, even without acceleration. * If we're doing signatures with OpenPGP (or even, I suppose, CMS), we're going to be using SHA-2, so it doesn't make sense to have our security depend on two separate algorithms when either one of them alone could break the security when we could just depend on one. So SHA-256 it is. Update the hash-function-transition design doc to say so. After this patch, there are no remaining instances of the string "NewHash", except for an unrelated use from 2008 as a variable name in t/t9700/test.pl. Signed-off-by: Ævar Arnfjörð Bjarmason Acked-by: Linus Torvalds Acked-by: brian m. carlson Acked-by: Johannes Schindelin Acked-by: Dan Shumow Signed-off-by: Jonathan Nieder --- Hi, Ævar Arnfjörð Bjarmason wrote: > I think it makes if you just take over 2/2 of this series (or even the > whole thing), since the meat of it is already something I copy/pasted > from you, and you've got more of an opinion / idea about how to proceed > (which is good!); it's more efficient than me trying to fix various > stuff you're pointing out at this point, I also think it makes sense > that you change the "Author" line for that, since the rest of the > changes will mainly be search-replace by me. Fair enough. Here's that updated patch 2/2. I'll try to make a more comprehensive set of proposed edits tomorrow, in a fresh thread (dealing with the cksum-trailer, etc). Brian, is your latest work in progress available somewhere (e.g. a branch on https://git.crustytoothpaste.net/git/bmc/git) so I can make sure any edits I make match well with it? Thanks, Jonathan .../technical/hash-function-transition.txt| 196 +- 1 file changed, 98 insertions(+), 98 deletions(-) diff --git a/Documentation/technical/hash-function-transition.txt b/Documentation/technical/hash-function-transition.txt index 5ee4754adb..bc2ace2a6e 100644 --- a/Documentation/technical/hash-function-transition.txt +++ b/Documentation/technical/hash-function-transition.txt @@ -59,14 +59,11 @@ that are believed to be cryptographically secure. Goals - -Where NewHash is a strong 256-bit hash function to replace SHA-1 (see -"Selection of a New Hash", below): - -1. The transition to NewHash can be done one local repository at a time. +1. The transition to SHA-256 can be done one local repository at a time. a. Requiring no action by any other party. - b. A NewHash repository can communicate with SHA-1 Git servers + b. A SHA-256 repository can communicate with SHA-1 Git servers (push/fetch). - c. Users can use SHA-1 and NewHash identifiers for objects + c. Users can use SHA-1 and SHA-256 identifiers for objects interchangeably (see "Object names on the command line", below). d. New signed objects make use of a stronger hash function than SHA-1 for their security guarantees. @@ -79,7 +76,7 @@ Where NewHash is a strong 256-bit hash function to replace SHA-1 (see Non-Goals - -1. Add NewHash support to Git protocol. This is valuable and the +1. Add SHA-256 support to Git protocol. This is valuable and the logical next step but it is out of scope for this initial design. 2. Transparently improving the security of existing SHA-1 signed objects. @@ -87,26 +84,26 @@ Non-Goals repository. 4. Taking the opportunity to fix other bugs in Git's formats and protocols. -5. Shallow clones and fetches into a NewHash repository. (This will - change when we add NewHash support to Git protocol.) -6. Skip fetching some submodules of a project into a NewHash - repository. (This also depends on NewHash support in Git +5. Shallow clones and fetches into a SHA-256 repository. (This will + change when we add SHA-256 support to Git protocol.) +6. Skip fetching some submodules of a project into a SHA-256 + repository. (This also depends on SHA-256 support in Git protocol.) Overview We introduce a new repository format extension. Repositories with this -extension enabled use NewHash instead of SHA-1 to name their objects. +extension enabled use SHA-256 instead of SHA-1 to name their objects. This affects both object names and object content --- both the names of objects and all references to other objects within an object are switched to the new hash function. -NewHash repositories cannot be read by older versions of Git. +SHA-256 repositories cannot be read by older versions of Git.
Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted
On Fri, Aug 03, 2018 at 12:06:34PM -0400, Jeff King wrote: > On Fri, Aug 03, 2018 at 11:43:44AM -0400, Santiago Torres wrote: > > > > This is not a deviation. GPG correctly recognizes difference between > > > trusted, > > > untrusted and unknown levels. git on the other hand does not. Well it did > > > until > > > the commit a4cc18f29. That one removed GPG exit code propagation. > > > > Oh wow. Sorry my assumption parted from looking at the code back in the > > day where this happens. I assumed git was quietly propagating the gpg > > error code and took it from there. > > > > Now that I think about it though, verify tag can verify more than one > > tag. I assume that this would make it difficult to propagate individual > > errors in trusting. I honestly don't know what's the best way to modify > > this behavior then. > > I think the only sensible thing is to err on the conservative side, and > return non-zero if we saw _any_ invalid signature. > > I will note, though, that just checking the exit code of `verify-tag` > isn't really that thorough. It shows that there was _a_ signature, but > we don't know: > > - if it was an identity the user would expect to be signing tags > > - if it even matches the refname we used to find the tag > > So I'd argue that any real verification needs to either have a human in > the loop, or implement a custom policy based on reading the full output. > > I know we (and you specifically Santiago) talked about this a while ago, > and we ended up providing ways to get more information out of > verify-tag, so that a tool could sit on top of that and implement more > project-specific policy. I don't know offhand of any reusable tools that > do so, though. I think that it would be even legit to exit on first tag verification failure. If someone wants to really verify all tags then it can be done with simple for loop. git that way does not have to solve problem of error combination. > - if it was an identity the user would expect to be signing tags That can be done just by using trust levels. > - if it even matches the refname we used to find the tag Can you explain this more? You mean that string (such as v1.1) used to lookup tag object is not verified as part of that object? OK I thing that it was enough of abstract concepts from me. Let me explain you what am I trying to achieve. I am implementing feeds (in other words git repositories with packages) and package sources verification for OpenWRT. We (project Turris by CZ.NIC) are signing all our commits and all our tags. Now we are using small script that is verifying our repositories just before we run build. That is against keyring maintained on our server. I am trying to extend that to whole OpenWRT tree. That introduces problem of having a lot of keys and a lot of packages sharing same allowed keys. Fetching all allowed keys for every package from key servers is just slow because of that I have to share those between packages. In general there are two options. First one is to have cache of already fetched keys in armor format. Second one is to have one keyring and by setting all keys explicitly as never trusted with package given exception. Unfortunately first option can't be used because of one other request that is from our team. We don't want to be forced to update list of allowed contributors to our projects every time we have new colleague. Solution we come up with is to have central PGP key that signs our whole team and then verification is done by allowing GPG to fetch additional keys with max-cert-depth 1. That brings me to git verify-commit/tag that won't exit with zero code when signature is not trusted. I have a solution for my problem (calling git verify-* twice and grep). That is not the point of this email nor this contribution. The point is that although GPG's behavior of exiting with 0 code when trust level is unknown is unexpected but in the end understandable, git's behavior of exiting with 0 code even if key is explicitly untrusted is just counterintuitive. I think that few people are still going to get nasty surprise when I consider that this change was introduced mid 2014 just after v2.4.0 and Ubuntu 14.04 lts (running even on part of our infrastructure) still contains version 1.9.1 and in that release it was acknowledging GPG exit code. K.K. signature.asc Description: PGP signature
Re: [PATCH] pack-objects: document about thread synchronization
Jeff King wrote: > On Sun, Jul 29, 2018 at 05:36:05PM +0200, Nguyễn Thái Ngọc Duy wrote: >> These extra comments should be make it easier to understand how to use >> locks in pack-objects delta search code. For reference, see >> >> 8ecce684a3 (basic threaded delta search - 2007-09-06) >> 384b32c09b (pack-objects: fix threaded load balancing - 2007-12-08) >> 50f22ada52 (threaded pack-objects: Use condition... - 2007-12-16) > > Thanks, I think this is an improvement. Yes, Reviewed-by: Jonathan Nieder as well. Usually I would prefer to see comments about what lock protects some state where the state is defined, but here the state is normally not protected by any lock, since Git is single-threaded except in limited places (like pack-objects). So the documentation you added is in the right place today, even though it's an unusual place. Longer term, if we start using more multithreading in Git, we'll have to reconsider how to structure the locking anyway. Thanks, Jonathan
Re: [PATCH] Makefile: enable DEVELOPER by default
On Sat, Aug 4, 2018 at 8:11 AM Jonathan Nieder wrote: > My main concern is not about them but about other > people building from source in order to run (instead of to develop) > Git, and by extension, the people they go to for help when it doesn't > work. I have lots of bitter experience of -Werror being a support > headache and leading to bad workarounds when someone upgrades their > compiler and the build starts failing due to a new warning it has > introduced. Even old compilers can also throw some silly, false positive warnings (which now turn into errors) because they are not as smart as new ones. -- Duy
Re: [PATCH v9 2/3] http-backend: respect CONTENT_LENGTH as specified by rfc3875
On Fri, Jul 27, 2018 at 5:50 AM Max Kirillov wrote:
> -static void inflate_request(const char *prog_name, int out, int buffer_input)
> +static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char
> **out)
> +{
> + unsigned char *buf = NULL;
> + ssize_t cnt = 0;
> +
> + if (max_request_buffer < req_len) {
> + die("request was larger than our maximum size (%lu): "
> + "%" PRIuMAX "; try setting GIT_HTTP_MAX_REQUEST_BUFFER",
> + max_request_buffer, (uintmax_t)req_len);
Please mark these strings for translation with _().
--
Duy
[PATCH] config.txt: reorder blame stuff to keep config keys sorted
The color group in config.txt is actually sorted but changes in sb/blame-color broke this. Reorder color.blame.* and move blame.coloring back to the rest of blame.* (and reorder that group too while we're there) Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/config.txt | 68 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index fd8d27e761..d97455057c 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -995,23 +995,28 @@ apply.whitespace:: Tells 'git apply' how to handle whitespaces, in the same way as the `--whitespace` option. See linkgit:git-apply[1]. -blame.showRoot:: - Do not treat root commits as boundaries in linkgit:git-blame[1]. - This option defaults to false. - blame.blankBoundary:: Show blank commit object name for boundary commits in linkgit:git-blame[1]. This option defaults to false. -blame.showEmail:: - Show the author email instead of author name in linkgit:git-blame[1]. - This option defaults to false. +blame.coloring:: + This determines the coloring scheme to be applied to blame + output. It can be 'repeatedLines', 'highlightRecent', + or 'none' which is the default. blame.date:: Specifies the format used to output dates in linkgit:git-blame[1]. If unset the iso format is used. For supported values, see the discussion of the `--date` option at linkgit:git-log[1]. +blame.showEmail:: + Show the author email instead of author name in linkgit:git-blame[1]. + This option defaults to false. + +blame.showRoot:: + Do not treat root commits as boundaries in linkgit:git-blame[1]. + This option defaults to false. + branch.autoSetupMerge:: Tells 'git branch' and 'git checkout' to set up new branches so that linkgit:git-pull[1] will appropriately merge from the @@ -1149,6 +1154,28 @@ color.advice:: color.advice.hint:: Use customized color for hints. +color.blame.highlightRecent:: + This can be used to color the metadata of a blame line depending + on age of the line. ++ +This setting should be set to a comma-separated list of color and date settings, +starting and ending with a color, the dates should be set from oldest to newest. +The metadata will be colored given the colors if the the line was introduced +before the given timestamp, overwriting older timestamped colors. ++ +Instead of an absolute timestamp relative timestamps work as well, e.g. +2.weeks.ago is valid to address anything older than 2 weeks. ++ +It defaults to 'blue,12 month ago,white,1 month ago,red', which colors +everything older than one year blue, recent changes between one month and +one year old are kept white, and lines introduced within the last month are +colored red. + +color.blame.repeatedLines:: + Use the customized color for the part of git-blame output that + is repeated meta information per line (such as commit id, + author name, date and timezone). Defaults to cyan. + color.branch:: A boolean to enable/disable color in the output of linkgit:git-branch[1]. May be set to `always`, @@ -1296,33 +1323,6 @@ color.status.:: status short-format), or `unmerged` (files which have unmerged changes). -color.blame.repeatedLines:: - Use the customized color for the part of git-blame output that - is repeated meta information per line (such as commit id, - author name, date and timezone). Defaults to cyan. - -color.blame.highlightRecent:: - This can be used to color the metadata of a blame line depending - on age of the line. -+ -This setting should be set to a comma-separated list of color and date settings, -starting and ending with a color, the dates should be set from oldest to newest. -The metadata will be colored given the colors if the the line was introduced -before the given timestamp, overwriting older timestamped colors. -+ -Instead of an absolute timestamp relative timestamps work as well, e.g. -2.weeks.ago is valid to address anything older than 2 weeks. -+ -It defaults to 'blue,12 month ago,white,1 month ago,red', which colors -everything older than one year blue, recent changes between one month and -one year old are kept white, and lines introduced within the last month are -colored red. - -blame.coloring:: - This determines the coloring scheme to be applied to blame - output. It can be 'repeatedLines', 'highlightRecent', - or 'none' which is the default. - color.transport:: A boolean to enable/disable color when pushes are rejected. May be set to `always`, `false` (or `never`) or `auto` (or `true`), in which -- 2.18.0.759.gbd3bccdecd
Re: [PATCH] Makefile: enable DEVELOPER by default
Hi, Stefan Beller wrote: > Reviewer bandwidth is limited, so let's have the machine of the > (potentially new) contributor warn about issues with the code by default. > > As setting DEVELOPER, the compiler is stricter and we may run into problems > on some architectures. But packagers of said platforms are knowledgeable > enough to turn off this flag. (Also they are fewer than the number of new > contributors) Which architectures would we run into problems on? Aren't those bugs that should themselves be fixed? I think you are right that the packagers will cope with whatever setting we choose. My main concern is not about them but about other people building from source in order to run (instead of to develop) Git, and by extension, the people they go to for help when it doesn't work. I have lots of bitter experience of -Werror being a support headache and leading to bad workarounds when someone upgrades their compiler and the build starts failing due to a new warning it has introduced. > Signed-off-by: Stefan Beller > --- > Makefile | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Makefile b/Makefile > index 41b93689add..95aa3ff3185 100644 > --- a/Makefile > +++ b/Makefile > @@ -497,6 +497,8 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) > ALL_LDFLAGS = $(LDFLAGS) > STRIP ?= strip > > +DEVELOPER=1 I like the idea of making this more discoverable to new contributors. It seems that Documentation/SubmittingPatches doesn't mention this setting. Should it? Should a non-DEVELOPER build print a note encouraging enabling this setting in case you're developing patches meant for submission to the project? Should we have a CONTRIBUTING.md file suggesting this setting? Other ideas for ensuring it's enabled for those who need it? Thanks and hope that helps, Jonathan
