Greetings!!!!!
Greetings! I have a proposal for you, this however is not mandatory nor will I in any manner compel you to honor against your will. I am Dr.Paul Benk a former executive director with Arab Tunisian Bank here in Tunis; I retired 1 year 7 months ago after putting in 28 years of meticulous service. During my days with Arab Tunisian Bank, I was the personal account officer and one of the financial advisers to Zine Al-Abidine Ben Ali the past Tunisian President in self exile at Saudi Arabia. During his trying period, he instructed me to move all his investment in my care which consists of US$115M and 767KG of gold out of the Gulf States for safe keeping; and that I successfully did by moving US$50 Million to Madrid Spain, US$50 Million to Dubai United Arab Emirate, US$15 Million to Burkina Faso and the 767KG of gold to Burkina Faso in West Africa as an anonymous deposits, so that the funds will in no way be traced to him. He has instructed me to find an investor who would stand as the anonymous depositor of the fund and gold; and claim it for further investment. Consequent upon the above, my proposal is that I would like you as a foreigner to stand in as the anonymous depositor of this fund and gold that I have successfully moved outside the country and provide an account overseas where this said fund will be transferred into. It is a careful network and my voluntary retirement from the Arab Tunisian Bank is to ensure a hitch-free operation as all modalities for you to stand as beneficiary and owner of the deposits has been perfected by me. Mr. Zine al-Abidine Ben Ali will be offering you 20% of the total investment if you can be the investor and claim this deposits in Spain and Burkina Faso as the anonymous depositor. Now my questions are:- 1. Can you handle this transaction? 2. Can I give you this trust? Consider this and get back to me as soon as possible on my private email Only here: dr.paulb...@gmail.com, so that I can give you more details regarding this transaction. Finally, it is my humble request that the information as contained herein be accorded the necessary attention, urgency as well as the secrecy it deserves. I expect your urgent response if you can handle this project. Respectfully yours, From:Dr.Paul Benk. Email: dr.paulb...@gmail.com
Re: [PATCH v2 1/3] read-cache: use shared perms when writing shared index
On Sat, Jun 24, 2017 at 12:02 AM, Junio C Hamano wrote: > Christian Couder writes: >> Because of that, on repositories created with `git init --shared=all` >> and using the split index feature, one gets an error like: >> >> fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file >> open failed: Permission denied >> >> when another user performs any operation that reads the shared index. >> >> We could use create_tempfile() that calls adjust_shared_perm(), but >> unfortunately create_tempfile() doesn't replace the XX at the end >> of the path it is passed. So let's just call adjust_shared_perm() by >> ourselves. > > Because create_tempfile() is not even a viable alternative, the > above sounds just as silly as saying "We could use X, but > unfortunately that X doesn't create a temporary file and return its > file descriptor" with X replaced with any one of about a dozen > functions that happen to call adjust_shared_perm(). > > Call adjust_shared_perm() on the temporary file created by > mks_tempfile() ourselves to adjust the permission bits. > > should be sufficient. Ok, the v3 has the above in the commit message and also uses get_tempfile_path().
Re: [PATCH v2 3/3] t1700: make sure split-index respects core.sharedrepository
On Sat, Jun 24, 2017 at 12:20 AM, Junio C Hamano wrote: > Christian Couder writes: > >> Add a few tests to check that both the split-index file and the >> shared-index file are created using the right permissions when >> core.sharedrepository is set. >> >> Signed-off-by: Christian Couder >> --- >> t/t1700-split-index.sh | 17 + >> 1 file changed, 17 insertions(+) >> >> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh >> index af3ec0da5a..2c5be732e4 100755 >> --- a/t/t1700-split-index.sh >> +++ b/t/t1700-split-index.sh >> @@ -370,4 +370,21 @@ test_expect_success 'check splitIndex.sharedIndexExpire >> set to "never" and "now" >> test $(ls .git/sharedindex.* | wc -l) -le 2 >> ' >> >> +while read -r mode modebits filename; do > > Style. Fixed in the version (v3) I just sent. > Running this twice in a loop would create two .git/sharedindex.* > files in quick succession. I do not think we want to assume that > the filesystem timestamp can keep up with us to allow "ls -t" to > work reliably in the second round (if there is a leftover shared > index from previous test, even the first round may not catch the > latest one). Yeah, it might be a problem on some systems. > How about doing each iteration this way instead? Which might be a > better solution to work around that. > > - with core.sharedrepository set to false, force the index to be > unsplit; "index" will have the default unshared permission > bits (but we do not care what it is and no need to check it). > > - remove any leftover sharedindex.*, if any. > > - with core.sharedrepository set to whatever mode being tested, > do the adding to force split. > > - test the permission of index file. > > - test the permission of sharedindex.* file; there should be > only one instance, so erroring out when we see two or more is > also a good test. > > The last two steps may look like: > > test_modebits .git/index >actual && test_cmp expect actual && > shared=$(ls .git/sharedindex.*) && > case "$shared" in > *" "*) > # we have more than one??? > false ;; > *) > test_modebits "shared" >actual && > test_cmp expect actual ;; > esac Ok, it does what you suggest in v3. Thanks.
[PATCH v3 2/3] t1301: move modebits() to test-lib-functions.sh
As the modebits() function can be useful outside t1301,
let's move it into test-lib-functions.sh, and while at
it let's rename it test_modebits().
Signed-off-by: Christian Couder
---
t/t1301-shared-repo.sh | 18 +++---
t/test-lib-functions.sh | 5 +
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 1312004f8c..dfece751b5 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -19,10 +19,6 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
)
'
-modebits () {
- ls -l "$1" | sed -e 's|^\(..\).*|\1|'
-}
-
for u in 002 022
do
test_expect_success POSIXPERM "shared=1 does not clear bits preset by
umask $u" '
@@ -88,7 +84,7 @@ do
rm -f .git/info/refs &&
git update-server-info &&
- actual="$(modebits .git/info/refs)" &&
+ actual="$(test_modebits .git/info/refs)" &&
verbose test "x$actual" = "x-$y"
'
@@ -98,7 +94,7 @@ do
rm -f .git/info/refs &&
git update-server-info &&
- actual="$(modebits .git/info/refs)" &&
+ actual="$(test_modebits .git/info/refs)" &&
verbose test "x$actual" = "x-$x"
'
@@ -111,7 +107,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in
unshared repo' '
umask 002 &&
git update-server-info &&
echo "-rw-rw-r--" >expect &&
- modebits .git/info/refs >actual &&
+ test_modebits .git/info/refs >actual &&
test_cmp expect actual
'
@@ -177,7 +173,7 @@ test_expect_success POSIXPERM 'remote init does not use
config from cwd' '
umask 0022 &&
git init --bare child.git &&
echo "-rw-r--r--" >expect &&
- modebits child.git/config >actual &&
+ test_modebits child.git/config >actual &&
test_cmp expect actual
'
@@ -187,7 +183,7 @@ test_expect_success POSIXPERM 're-init respects
core.sharedrepository (local)' '
echo whatever >templates/foo &&
git init --template=templates &&
echo "-rw-rw-rw-" >expect &&
- modebits .git/foo >actual &&
+ test_modebits .git/foo >actual &&
test_cmp expect actual
'
@@ -198,7 +194,7 @@ test_expect_success POSIXPERM 're-init respects
core.sharedrepository (remote)'
test_path_is_missing child.git/foo &&
git init --bare --template=../templates child.git &&
echo "-rw-rw-rw-" >expect &&
- modebits child.git/foo >actual &&
+ test_modebits child.git/foo >actual &&
test_cmp expect actual
'
@@ -209,7 +205,7 @@ test_expect_success POSIXPERM 'template can set
core.sharedrepository' '
cp .git/config templates/config &&
git init --bare --template=../templates child.git &&
echo "-rw-rw-rw-" >expect &&
- modebits child.git/HEAD >actual &&
+ test_modebits child.git/HEAD >actual &&
test_cmp expect actual
'
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5ee124332a..db622c3555 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -216,6 +216,11 @@ test_chmod () {
git update-index --add "--chmod=$@"
}
+# Get the modebits from a file.
+test_modebits () {
+ ls -l "$1" | sed -e 's|^\(..\).*|\1|'
+}
+
# Unset a configuration variable, but don't fail if it doesn't exist.
test_unconfig () {
config_dir=
--
2.13.1.517.gf6399a5ea5
[PATCH v3 3/3] t1700: make sure split-index respects core.sharedrepository
Add a few tests to check that both the split-index file and the shared-index file are created using the right permissions when core.sharedrepository is set. Signed-off-by: Christian Couder --- t/t1700-split-index.sh | 30 ++ 1 file changed, 30 insertions(+) diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh index af3ec0da5a..22f69a410b 100755 --- a/t/t1700-split-index.sh +++ b/t/t1700-split-index.sh @@ -370,4 +370,34 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now" test $(ls .git/sharedindex.* | wc -l) -le 2 ' +while read -r mode modebits +do + test_expect_success POSIXPERM "split index respects core.sharedrepository $mode" ' + # Remove existing shared index files + git config core.splitIndex false && + git update-index --force-remove one && + rm -f .git/sharedindex.* && + # Create one new shared index file + git config core.sharedrepository "$mode" && + git config core.splitIndex true && + : >one && + git update-index --add one && + echo "$modebits" >expect && + test_modebits .git/index >actual && + test_cmp expect actual && + shared=$(ls .git/sharedindex.*) && + case "$shared" in + *" "*) + # we have more than one??? + false ;; + *) + test_modebits "$shared" >actual && + test_cmp expect actual ;; + esac + ' +done <<\EOF +0666 -rw-rw-rw- +0642 -rw-r---w- +EOF + test_done -- 2.13.1.517.gf6399a5ea5
[PATCH v3 1/3] read-cache: use shared perms when writing shared index
Since f6ecc62dbf (write_shared_index(): use tempfile module, 2015-08-10)
write_shared_index() has been using mks_tempfile() to create the
temporary file that will become the shared index.
But even before that, it looks like the functions used to create this
file didn't call adjust_shared_perm(), which means that the shared
index file has always been created with 600 permissions regardless
of the shared permission settings.
Because of that, on repositories created with `git init --shared=all`
and using the split index feature, one gets an error like:
fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file
open failed: Permission denied
when another user performs any operation that reads the shared index.
Call adjust_shared_perm() on the temporary file created by
mks_tempfile() ourselves to adjust the permission bits.
Signed-off-by: Christian Couder
---
read-cache.c | 8
1 file changed, 8 insertions(+)
diff --git a/read-cache.c b/read-cache.c
index bc156a133e..1f4ec1b022 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2425,6 +2425,14 @@ static int write_shared_index(struct index_state *istate,
delete_tempfile(&temporary_sharedindex);
return ret;
}
+ ret = adjust_shared_perm(get_tempfile_path(&temporary_sharedindex));
+ if (ret) {
+ int save_errno = errno;
+ error("cannot fix permission bits on %s",
get_tempfile_path(&temporary_sharedindex));
+ delete_tempfile(&temporary_sharedindex);
+ errno = save_errno;
+ return ret;
+ }
ret = rename_tempfile(&temporary_sharedindex,
git_path("sharedindex.%s",
sha1_to_hex(si->base->sha1)));
if (!ret) {
--
2.13.1.517.gf6399a5ea5
What's cooking in git.git (Jun 2017, #07; Sat, 24)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. A new maintenance release v2.13.2 has been tagged with various fixes that have been already in the 'master' branch. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * ab/free-and-null (2017-06-16) 6 commits (merged to 'next' on 2017-06-21 at a1825fabd8) + *.[ch] refactoring: make use of the FREE_AND_NULL() macro + coccinelle: make use of the "expression" FREE_AND_NULL() rule + coccinelle: add a rule to make "expression" code use FREE_AND_NULL() + coccinelle: make use of the "type" FREE_AND_NULL() rule + coccinelle: add a rule to make "type" code use FREE_AND_NULL() + git-compat-util: add a FREE_AND_NULL() wrapper around free(ptr); ptr = NULL A common pattern to free a piece of memory and assign NULL to the pointer that used to point at it has been replaced with a new FREE_AND_NULL() macro. * ab/pcre-v2 (2017-06-21) 1 commit (merged to 'next' on 2017-06-21 at fb6320213c) + grep: fix erroneously copy/pasted variable in check/assert pattern Hotfix for a topic already in 'master'. * ab/wildmatch-glob-slash-test (2017-06-15) 1 commit (merged to 'next' on 2017-06-21 at 8f4a056f5f) + wildmatch test: cover a blind spot in "/" matching A new test to show the interaction between the pattern [^a-z] (which matches '/') and a slash in a path has been added. The pattern should not match the slash with "pathmatch", but should with "wildmatch". * ah/doc-gitattributes-empty-index (2017-06-15) 1 commit (merged to 'next' on 2017-06-21 at f1dc92557b) + doc: do not use `rm .git/index` when normalizing line endings An example in documentation that does not work in multi worktree configuration has been corrected. * bw/config-h (2017-06-15) 6 commits (merged to 'next' on 2017-06-21 at 15c5f34034) + config: don't implicitly use gitdir or commondir + config: respect commondir + setup: teach discover_git_directory to respect the commondir + config: don't include config.h by default + config: remove git_config_iter + config: create config.h (this branch is used by bw/repo-object; uses js/alias-early-config.) Fix configuration codepath to pay proper attention to commondir that is used in multi-worktree situation, and isolate config API into its own header file. * bw/ls-files-sans-the-index (2017-06-13) 17 commits (merged to 'next' on 2017-06-21 at 39ce64f6c7) + ls-files: factor out tag calculation + ls-files: factor out debug info into a function + ls-files: convert show_files to take an index + ls-files: convert show_ce_entry to take an index + ls-files: convert prune_cache to take an index + ls-files: convert ce_excluded to take an index + ls-files: convert show_ru_info to take an index + ls-files: convert show_other_files to take an index + ls-files: convert show_killed_files to take an index + ls-files: convert write_eolinfo to take an index + ls-files: convert overlay_tree_on_cache to take an index + tree: convert read_tree to take an index parameter + convert: convert renormalize_buffer to take an index + convert: convert convert_to_git to take an index + convert: convert convert_to_git_filter_fd to take an index + convert: convert crlf_to_git to take an index + convert: convert get_cached_convert_stats_ascii to take an index (this branch is used by bw/repo-object.) Code clean-up. * da/mergetools-meld-output-opt-on-macos (2017-06-18) 1 commit (merged to 'next' on 2017-06-21 at de00cce3c0) + mergetools/meld: improve compatibiilty with Meld on macOS X "git mergetool" learned to work around a wrapper MacOS X adds around underlying meld. * jk/diff-highlight-module (2017-06-15) 1 commit (merged to 'next' on 2017-06-21 at e418062ad2) + diff-highlight: split code into module The 'diff-highlight' program (in contrib/) has been restructured for easier reuse by an external project 'diff-so-fancy'. * jk/warn-add-gitlink (2017-06-15) 2 commits (merged to 'next' on 2017-06-21 at 7210ddbb2e) + t: move "git add submodule" into test blocks + add: warn when adding an embedded repository Using "git add d/i/r" when d/i/r is the top of the working tree of a separate repository would create a gitlink in the index, which would appear as a not-quite-initialized submodule to others. We learned to give warnings when this happens. * js/alias-early-config (2017-06-15) 6 commits (merged to 'next' on 2017-06-21 at ca4995aac2) + alias: use the early config machinery to expand aliases + t7006: demonstrate a problem with aliases in subdirectories + t1308: relax the test verifying that empty alias valu
[ANNOUNCE] Git v2.13.2
The latest maintenance release Git v2.13.2 is now available at
the usual places.
The tarballs are found at:
https://www.kernel.org/pub/software/scm/git/
The following public repositories all have a copy of the 'v2.13.2'
tag and the 'maint' branch that the tag points at:
url = https://kernel.googlesource.com/pub/scm/git/git
url = git://repo.or.cz/alt-git.git
url = https://github.com/gitster/git
Git v2.13.2 Release Notes
=
Fixes since v2.13.1
---
* The "collision detecting" SHA-1 implementation shipped with 2.13.1
was still broken on some platforms. Update to the upstream code
again to take their fix.
* "git checkout --recurse-submodules" did not quite work with a
submodule that itself has submodules.
* Introduce the BUG() macro to improve die("BUG: ...").
* The "run-command" API implementation has been made more robust
against dead-locking in a threaded environment.
* A recent update to t5545-push-options.sh started skipping all the
tests in the script when a web server testing is disabled or
unavailable, not just the ones that require a web server. Non HTTP
tests have been salvaged to always run in this script.
* "git clean -d" used to clean directories that has ignored files,
even though the command should not lose ignored ones without "-x".
"git status --ignored" did not list ignored and untracked files
without "-uall". These have been corrected.
* The timestamp of the index file is now taken after the file is
closed, to help Windows, on which a stale timestamp is reported by
fstat() on a file that is opened for writing and data was written
but not yet closed.
* "git pull --rebase --autostash" didn't auto-stash when the local history
fast-forwards to the upstream.
* "git describe --contains" penalized light-weight tags so much that
they were almost never considered. Instead, give them about the
same chance to be considered as an annotated tag that is the same
age as the underlying commit would.
* The result from "git diff" that compares two blobs, e.g. "git diff
$commit1:$path $commit2:$path", used to be shown with the full
object name as given on the command line, but it is more natural to
use the $path in the output and use it to look up .gitattributes.
* A flaky test has been corrected.
* Help contributors that visit us at GitHub.
* "git stash push " did not work from a subdirectory at all.
Bugfix for a topic in v2.13
Also contains various documentation updates and code clean-ups.
Changes since v2.13.1 are as follows:
Adam Dinwoodie (1):
docs: fix formatting and grammar
Brandon Williams (12):
t5550: use write_script to generate post-update hook
t0061: run_command executes scripts without a #! line
run-command: prepare command before forking
run-command: use the async-signal-safe execv instead of execvp
string-list: add string_list_remove function
run-command: prepare child environment before forking
run-command: don't die in child when duping /dev/null
run-command: eliminate calls to error handling functions in child
run-command: handle dup2 and close errors in child
run-command: add note about forking and threading
run-command: expose is_executable function
run-command: restrict PATH search to executable files
Dennis Kaarsemaker (1):
send-email: Net::SMTP::SSL is obsolete, use only when necessary
Eric Wong (1):
run-command: block signals between fork and execve
Jeff Hostetler (1):
read-cache: close index.lock in do_write_index
Jeff King (23):
usage.c: add BUG() function
setup_git_env: convert die("BUG") to BUG()
config: complain about --local outside of a git repo
usage.c: drop set_error_handle()
handle_revision_arg: reset "dotdot" consistently
handle_revision_arg: simplify commit reference lookups
handle_revision_arg: stop using "dotdot" as a generic pointer
handle_revision_arg: hoist ".." check out of range parsing
handle_revision_arg: add handle_dotdot() helper
sha1_name: consistently refer to object_context as "oc"
get_sha1_with_context: always initialize oc->symlink_path
get_sha1_with_context: dynamically allocate oc->path
t4063: add tests of direct blob diffs
handle_revision_arg: record modes for "a..b" endpoints
handle_revision_arg: record paths for pending objects
diff: pass whole pending entry in blobinfo
diff: use the word "path" instead of "name" for blobs
diff: use pending "path" if it is available
diff: use blob path for blob/file diffs
connect.c: fix leak in parse_one_symref_info()
remote: drop free_refspecs() function
t5313: make extended-table test more deterministic
sha
A note from the maintainer
Welcome to the Git development community.
This message is written by the maintainer and talks about how Git
project is managed, and how you can work with it.
* Mailing list and the community
The development is primarily done on the Git mailing list. Help
requests, feature proposals, bug reports and patches should be sent to
the list address . You don't have to be
subscribed to send messages. The convention on the list is to keep
everybody involved on Cc:, so it is unnecessary to say "Please Cc: me,
I am not subscribed".
Before sending patches, please read Documentation/SubmittingPatches
and Documentation/CodingGuidelines to familiarize yourself with the
project convention.
If you sent a patch and you did not hear any response from anybody for
several days, it could be that your patch was totally uninteresting,
but it also is possible that it was simply lost in the noise. Please
do not hesitate to send a reminder message in such a case. Messages
getting lost in the noise may be a sign that those who can evaluate
your patch don't have enough mental/time bandwidth to process them
right at the moment, and it often helps to wait until the list traffic
becomes calmer before sending such a reminder.
The list archive is available at a few public sites:
http://public-inbox.org/git/
http://marc.info/?l=git
http://www.spinics.net/lists/git/
For those who prefer to read it over NNTP:
nntp://news.public-inbox.org/inbox.comp.version-control.git
nntp://news.gmane.org/gmane.comp.version-control.git
are available.
When you point at a message in a mailing list archive, using its
message ID is often the most robust (if not very friendly) way to do
so, like this:
http://public-inbox.org/git/pine.lnx.4.58.0504150753440.7...@ppc970.osdl.org
Often these web interfaces accept the message ID with enclosing <>
stripped (like the above example to point at one of the most important
message in the Git list).
Some members of the development community can sometimes be found on
the #git and #git-devel IRC channels on Freenode. Their logs are
available at:
http://colabti.org/irclogger/irclogger_log/git
http://colabti.org/irclogger/irclogger_log/git-devel
There is a volunteer-run newsletter to serve our community ("Git Rev
News" http://git.github.io/rev_news/rev_news.html).
Git is a member project of software freedom conservancy, a non-profit
organization (https://sfconservancy.org/). To reach a committee of
liaisons to the conservancy, contact them at .
* Reporting bugs
When you think git does not behave as you expect, please do not stop
your bug report with just "git does not work". "I used git in this
way, but it did not work" is not much better, neither is "I used git
in this way, and X happend, which is broken". It often is that git is
correct to cause X happen in such a case, and it is your expectation
that is broken. People would not know what other result Y you expected
to see instead of X, if you left it unsaid.
Please remember to always state
- what you wanted to achieve;
- what you did (the version of git and the command sequence to reproduce
the behavior);
- what you saw happen (X above);
- what you expected to see (Y above); and
- how the last two are different.
See http://www.chiark.greenend.org.uk/~sgtatham/bugs.html for further
hints.
If you think you found a security-sensitive issue and want to disclose
it to us without announcing it to wider public, please contact us at
our security mailing list . This is
a closed list that is limited to people who need to know early about
vulnerabilities, including:
- people triaging and fixing reported vulnerabilities
- people operating major git hosting sites with many users
- people packaging and distributing git to large numbers of people
where these issues are discussed without risk of the information
leaking out before we're ready to make public announcements.
* Repositories and documentation.
My public git.git repositories are at:
git://git.kernel.org/pub/scm/git/git.git/
https://kernel.googlesource.com/pub/scm/git/git
git://repo.or.cz/alt-git.git/
https://github.com/git/git/
git://git.sourceforge.jp/gitroot/git-core/git.git/
git://git-core.git.sourceforge.net/gitroot/git-core/git-core/
This one shows not just the main integration branches, but also
individual topics broken out:
git://github.com/gitster/git/
A few web interfaces are found at:
http://git.kernel.org/cgit/git/git.git
https://kernel.googlesource.com/pub/scm/git/git
http://repo.or.cz/w/alt-git.git
Preformatted documentation from the tip of the "master" branch can be
found in:
git://git.kernel.org/pub/scm/git/git-{htmldocs,manpages}.git/
git://repo.or.cz/git-{htmldocs,manpages}.git/
https://github.com/gitster/git-{htmldocs,manpages}.git/
The manual pages formatted in HTML for the tip of 'master' can be
viewed online at:
https://git.github.io/htmldocs/git.html
* H
Re: EOL LF Windows auto.crlf issue
On 24/06/17 18:56, Filip Kucharczyk wrote: I'm on Windows 10. auto.crlf in .gitconfig is set to [core] autocrlf = true I've got a git (git version 2.13.1.windows.2) repo. A linux guy emails me a text with with line endings LF. I paste this file into my repo. Now every time I introduce changes to this file and stage it, git tell me: "warning: LF will be replaced by CRLF in a.txt. This conversion will happen if you do rm a.txt git checkout a.txt After these 2 steps, the file willhave CRLF in the working tree, see below. But if you don't, the file stays as it is in the working tree. The file will have its original line endings in your working directory." But when I commit the file git does not replace anyline endings - they stay LF in the commited file and in my working direcotry. Yes, core.autocrlf=true tells Git that they should have CRLF in the working tree, at least after a clean checkout. I'm sort of misleaded by this message. Suggestions how to improve things are of course welcome - for your case it may help to set git config core.autocrlf input for this very repo. Filip
Re: [PATCH v4 7/8] sha1_file: do not access pack if unneeded
On Sat, Jun 24, 2017 at 11:41:39AM -0700, Junio C Hamano wrote: > > The other nice thing about whence_only is that it flips the logic. So > > any existing callers which depend on filling the union automatically > > will not be affected (though I would be surprised if there are any such > > callers; most of that information isn't actually that interesting). > > Hmph, but the solution does not scale. When a caller wants whence > and something else that cannot be asked for or ignored by being a > "pointer to a result" field, such a request cannot be expressed. We > either need to make all fields in oi request to "pointer to a > result, if the result is needed, or NULL when the result is not of > interest", or give a bit for each non-pointer field to allow the > caller to express "I am not interested in the value of this field". True. > In the usecase Jonathan has, the caller's wish is a very narrow "I > am interested in nothing; just checking if the object is there", and > passing NULL for oi works fine. So I'm inclined to suggest that we > take that approach now and worry about a more generic and scalable > "how would one tell the machinery that the value for a field is > uninteresting when the field is not a pointer to result?" mechanism > until a real need arises. If we are open to writing anything, then I think it should follow the same pointer-to-data pattern that the rest of the struct does. I.e., declare the extra pack-data struct as a pointer, and let callers fill it in or not. It's excessive in the sense that it's not a variable-sized answer, but it at least makes the interface consistent. And no callers who read it would be silently broken; the actual data type in "struct object_info" would change, so they'd get a noisy compile error. -Peff
Re: [PATCH v5 5/5] convert: add "status=delayed" to filter process protocol
On Sat, Jun 24, 2017 at 11:51:49AM -0700, Junio C Hamano wrote: > >> if ((CAP_DELAY & entry->supported_capabilities) && > >> dco && dco->state == CE_CAN_DELAY)) > > > > Agreed! > > Why wasn't this caught earlier? I thought this is something gcc warns about. I thought so, too. If it warned about: if (A & B) that would probably be too annoying. But: if (A & B && C) is much more questionable. I wonder if it used to exist and gcc dropped it (or dropped it from -Wall). -Wlogical-op seems like the most likely candidate, but it does not catch it (and it has a false positive in handle_nonblock, or perhaps I just can't see the problem). > >> The operator precedence is such that it works without them, so this is > >> just a style question (I'd also usually put the flags field before the > >> flag itself, but that's really getting into aesthetics). > > > > You mean (entry & CAP_DELAY) instead of (CAP_DELAY & entry)? > > Peff is continuing his explanation why (A & B && C) is technically > correct and preferring ((A & B) && C) is purely stylistic. "A & B" > binds tighter than "something && C" which means that (A & B && C) > cannot be misinterpreted as (A & (B && C)). I actually meant both. The bitwise operator binds tighter so it's OK either way. But I would write "flags & MY_FLAG" and never "MY_FLAG & flags". -Peff
Re: [PATCH v5 5/5] convert: add "status=delayed" to filter process protocol
On Sat, Jun 24, 2017 at 07:22:40PM +0200, Lars Schneider wrote:
> > It might be worth giving a reason in this last paragraph. I think the
> > reason is "because it's more complicated for the caller, as they have to
> > be OK with out-of-order processing and remembering to go back and handle
> > the delayed cases".
>
> Correct! However, my real reason was that these code paths process all
> files of the tree. Therefore the "out-of-order" processing can be
> effective.
>
> How about this:
>
> Git has a multiple code paths that checkout a blob. Support delayed
> checkouts only in `clone` (in unpack-trees.c) and `checkout` operations
> for now. The optimization is most effective in these code paths as all
> files of the tree are processed.
Sounds good.
> > Why do we need to tell the filter we know about delay? Shouldn't it just
> > need to tell us that it knows about delay, and then we choose whether to
> > ask for can-delay for particular entries?
>
> Because in the protocol I defined that the filter needs to answer with
> a strict subset of this list [1]. I thought that this would make the protocol
> more future proof/backward compatible. Because the filter is not allowed to
> answer with something that Git does not understand.
>
> [1]
> https://github.com/git/git/blob/5402b1352f5181247405fbff1887008a0cb3b04a/Documentation/gitattributes.txt#L408-L411
OK. That makes sense, then.
> > The operator precedence is such that it works without them, so this is
> > just a style question (I'd also usually put the flags field before the
> > flag itself, but that's really getting into aesthetics).
>
> You mean (entry & CAP_DELAY) instead of (CAP_DELAY & entry)?
Yes, exactly.
> How about this?
>
> errs |= dco->paths.nr;
> for_each_string_list_item(path, &dco->paths) {
> warning("%s was not processed properly.", path->string);
> }
> string_list_clear(&dco->paths, 0);
>
> The output would be:
>
> warning: test-delay10.a was not processed properly.
> warning: test-delay10.b was not processed properly.
> warning: test-delay11.a was not processed properly.
> warning: test-delay20.a was not processed properly.
> fatal: unable to checkout working tree
> warning: Clone succeeded, but checkout failed.
> You can inspect what was checked out with 'git status'
> and retry the checkout with 'git checkout -f HEAD'
I think it may make sense to use something more specific than
"processed". The user might not even be thinking about filters during
their operation. It would be really nice if we could mention the name of
the filter. As you noted, we don't have it here but I wonder how hard it
would be. Anyway, I'm OK with leaving it more vague for now.
> I contemplated about the warning text.
> "$FILE was not filtered properly." is technical more
> correct but maybe it would confuse the user?
I like it better because "filter" is a word the user might associate
with the filter feature. Whereas "processed" is vague and could mean
many things.
> > Hmm. This "reset the state" bit at the end surprised me. I guess it's
> > not wrong, but it goes against the mental model I had formed above. ;)
> >
> > We really are using dco->state as a per-entry state flag. It just
> > happens to be in a persistent shared struct. I don't think it's wrong,
> > it was mostly just surprising. I don't know if it's worth trying to
> > simplify, but I think you could do it by:
> >
> > 1. Passing back the "was delayed" state from async_convert... in the
> > return value or via a separate out-parameter.
>
> In the beginning I had it implemented that way. But that meant that I
> had to pass two variables through the entire convert stack:
>
> async_convert_to_working_tree
> -> convert_to_working_tree_internal
> --> apply_filter
> ---> apply_multi_file_filter
Right, I see. I wonder if just a comment in the definition of the
delayed_checkout struct would make it more clear exactly how we expect
the member to be used.
> > 2. Setting dco->state to CE_RETRY at the top of finish_delayed... so
> > that it's clear that it's about what phase of the conversation
> > we're in.
>
> I could do that. However, I thought it is safer to set the state *before*
> every checkout operation in case convert.c messes with this field (it
> should not in this phase).
>
> > But I'm OK with it as-is, too.
>
> I'll try 2.
I think you'd have to do (1) and (2) together. But if it causes pain, I
think the comment I suggested above may be the simplest way to go.
> Thanks a lot for the review,
You're welcome. The bits of your response I didn't quote all made sense
to me.
-Peff
Re: Crash in t3507-cherry-pick-conflict.sh with GIT_TEST_SPLIT_INDEX set
Christian Couder writes: > It bisects to f9d7abec2a (split-index: add and use > unshare_split_index(), 2017-05-05) that is fixing memory leaks when > discarding the index. > It looks like we are freeing some cache entries that we shouldn't free. Ouch. Let's revert the merge for now so that we can figure out what is exactly broken in 'pu'.
Re: [PATCH v5 5/5] convert: add "status=delayed" to filter process protocol
Lars Schneider writes:
>> On 24 Jun 2017, at 16:19, Jeff King wrote:
>>
>> Speaking of which, _are_ we OK with out-of-order processing in things
>> like checkout? Certainly we care about deleting items before checking
>> out new ones (so getting rid of "foo/bar" to make room for "foo" and
>> vice versa). I guess it's OK as long as the delayed items are always
>> filters that check out new items.
>
> Junio noticed that too but thinks we should be OK:
> "[...] We checkout removals first to make room so
> that creation of a path X can succeed if an existing path X/Y
> that used to want to see X as a directory can succeed [...]"
>
> http://public-inbox.org/git/xmqqvavotych@gitster.mtv.corp.google.com/
In which I said
... I
think "remove all and then create" you do not specifically have
to worry about with the proposed change, but you may need to
inspect and verify there aren't other kind of order dependency.
Yes, I think having two separate loops in the caller can help
guaranteeing this, as long as the delayed items are always filters
that check out new things. It will break once you have delayed
removals but I do not see how such a thing would be necessary ;-)
But there may be other kinds of order dependency--I didn't look for
them.
>>> @@ -647,6 +653,14 @@ static int apply_multi_file_filter(const char *path,
>>> const char *src, size_t len
>>> if (err)
>>> goto done;
>>>
>>> + if (CAP_DELAY & entry->supported_capabilities &&
>>> + dco && dco->state == CE_CAN_DELAY) {
>>
>> In a complicated conditional with bit operations, we usually put the bit
>> operation in its own parentheses so it's more obvious that it wasn't
>> supposed to be "&&". Like:
>>
>> if ((CAP_DELAY & entry->supported_capabilities) &&
>> dco && dco->state == CE_CAN_DELAY))
>
> Agreed!
Why wasn't this caught earlier? I thought this is something gcc warns about.
>> The operator precedence is such that it works without them, so this is
>> just a style question (I'd also usually put the flags field before the
>> flag itself, but that's really getting into aesthetics).
>
> You mean (entry & CAP_DELAY) instead of (CAP_DELAY & entry)?
Peff is continuing his explanation why (A & B && C) is technically
correct and preferring ((A & B) && C) is purely stylistic. "A & B"
binds tighter than "something && C" which means that (A & B && C)
cannot be misinterpreted as (A & (B && C)).
Crash in t3507-cherry-pick-conflict.sh with GIT_TEST_SPLIT_INDEX set
Git from the master branch currently segfaults when running t3507-cherry-pick-conflict.sh with GIT_TEST_SPLIT_INDEX set: expecting success: pristine_detach initial && test_must_fail git cherry-pick -s picked-signed && git commit -a -s && test $(git show -s |grep -c "Signed-off-by") = 1 HEAD is now at df2a63d... initial Auto-merging foo CONFLICT (content): Merge conflict in foo error: could not apply e4ca149... picked-signed hint: after resolving the conflicts, mark the corrected paths hint: with 'git add ' or 'git rm ' hint: and commit the result with 'git commit' Segmentation fault not ok 29 - commit after failed cherry-pick does not add duplicated -s # # pristine_detach initial && # test_must_fail git cherry-pick -s picked-signed && # git commit -a -s && # test $(git show -s |grep -c "Signed-off-by") = 1 # The crash happens during the `git commit -a -s` with a backtrace like this: Program received signal SIGSEGV, Segmentation fault. 0x0050bcf1 in entry_equals (map=0x8cef80 , e1=0xa2d2d2d2d2d2d00, e2=0x7fffce10, keydata=0x0) at hashmap.c:98 98 return (e1 == e2) || (e1->hash == e2->hash && !map->cmpfn(e1, e2, keydata)); (gdb) bt #0 0x0050bcf1 in entry_equals (map=0x8cef80 , e1=0xa2d2d2d2d2d2d00, e2=0x7fffce10, keydata=0x0) at hashmap.c:98 #1 0x0050bec6 in find_entry_ptr (map=0x8cef80 , key=0x7fffce10, keydata=0x0) at hashmap.c:138 #2 0x0050c044 in hashmap_get (map=0x8cef80 , key=0x7fffce10, keydata=0x0) at hashmap.c:182 #3 0x00525a1d in hashmap_get_from_hash (map=0x8cef80 , hash=1625022057, keydata=0x0) at hashmap.h:78 #4 0x00526edd in index_file_exists (istate=0x8cef40 , name=0x8f19d0 "unrelated", namelen=9, icase=0) at name-hash.c:701 #5 0x004f55ba in treat_one_path (dir=0x7fffd0b0, untracked=0x0, istate=0x8cef40 , path=0x7fffcf80, baselen=0, pathspec=0x88c2b8 , dtype=8, de=0x8f8a00) at dir.c:1550 #6 0x004f5914 in treat_path (dir=0x7fffd0b0, untracked=0x0, cdir=0x7fffcfa0, istate=0x8cef40 , path=0x7fffcf80, baselen=0, pathspec=0x88c2b8 ) at dir.c:1660 #7 0x004f6006 in read_directory_recursive (dir=0x7fffd0b0, istate=0x8cef40 , base=0x61561b "", baselen=0, untracked=0x0, check_only=0, pathspec=0x88c2b8 ) at dir.c:1809 It bisects to f9d7abec2a (split-index: add and use unshare_split_index(), 2017-05-05) that is fixing memory leaks when discarding the index. It looks like we are freeing some cache entries that we shouldn't free.
Re: [PATCH v4 7/8] sha1_file: do not access pack if unneeded
Jeff King writes:
> On Wed, Jun 21, 2017 at 11:15:01AM -0700, Junio C Hamano wrote:
>
>> > + if (!oi->typep && !oi->sizep && !oi->disk_sizep &&
>> > + !oi->delta_base_sha1 && !oi->typename && !oi->contentp &&
>> > + !oi->populate_u) {
>> > + oi->whence = OI_PACKED;
>> > + return 0;
>> > + }
>> > +
>>
>> ... this "if" statement feels like a maintenance nightmare. The
>> intent of the guard, I think, is "when the call wants absolutely
>> nothing but whence", but the implementation of the guard will not
>> stay true to the intent whenever somebody adds a new field to oi.
>>
>> I wonder if it makes more sense to have a new field "whence_only",
>> which is set only by such a specialized caller, which this guard
>> checks (and no other fields).
>
> The other nice thing about whence_only is that it flips the logic. So
> any existing callers which depend on filling the union automatically
> will not be affected (though I would be surprised if there are any such
> callers; most of that information isn't actually that interesting).
Hmph, but the solution does not scale. When a caller wants whence
and something else that cannot be asked for or ignored by being a
"pointer to a result" field, such a request cannot be expressed. We
either need to make all fields in oi request to "pointer to a
result, if the result is needed, or NULL when the result is not of
interest", or give a bit for each non-pointer field to allow the
caller to express "I am not interested in the value of this field".
In the usecase Jonathan has, the caller's wish is a very narrow "I
am interested in nothing; just checking if the object is there", and
passing NULL for oi works fine. So I'm inclined to suggest that we
take that approach now and worry about a more generic and scalable
"how would one tell the machinery that the value for a field is
uninteresting when the field is not a pointer to result?" mechanism
until a real need arises.
Thanks.
Re: [PATCH v2] die(): stop hiding errors due to overzealous recursion guard
Jeff King writes: >> One case I'd be worried about would be that the race is so bad that >> die-is-recursing-builtin never returns 0 even once. Everybody will >> just say "recursing" and die, without giving any useful information. > > I was trying to think how that would happen. If nobody's actually > recursing indefinitely, then the value in theory peaks at the number of > threads (modulo the fact that we're modifying a variable from multiple > threads without any locking; I'm not sure how reasonable it is to assume > in practice that sheared writes may cause us to lose an increment but > not to put nonsense in to the variable). If they are, then one thread > may increment it to 1024 before another thread gets a chance to say > anything. But in that case, the recursion-die is our expected outcome. > > Anyway, it might be reasonable to protect the counter with a mutex. > Like: > ... > To be honest, I'm not sure if it's worth giving it much more time, > though. I'd be fine with Ævar's patch as-is. The scenario I had in mind was three or more threads simultaneously dying, each incrementing dying counter by one and before any of them have a chance to say "called many times, error or racy threaded death!", because they all observe three (or more). But I was incorrectly reading the code---in that case, as long as dying is small enough, we'll return 0 and let at least one of the caller give a chance to give a message that came in "err" from their invocations of die()'s. So I do not think it is worth worrying about too deeply. Thanks.
Re: [PATCH] doc: clarify syntax for %C(auto,...) in pretty formats
Andreas Heiduk writes: > The change actually adds only > > (e.g. `%C(auto,red)`) > > but reflowing the paragraph blows it up a little. In such a case, you can avoid re-flowing and make the resulting lines of a-bit uneven lengths. The end result can be checked with "git diff --word-diff", so do not worry too much about this either way, as long as the real change is small. Thanks. > > 8< > The manual correctly describes the syntax with `auto,` but the > trailing `,` is hard to spot in a terminal. The HTML format does not > have this problem. Adding an example helps both worlds. > > Signed-off-by: Andreas Heiduk > --- > Documentation/pretty-formats.txt | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/Documentation/pretty-formats.txt > b/Documentation/pretty-formats.txt > index 38040e95b..b03985101 100644 > --- a/Documentation/pretty-formats.txt > +++ b/Documentation/pretty-formats.txt > @@ -174,11 +174,12 @@ endif::git-rev-list[] > - '%Creset': reset color > - '%C(...)': color specification, as described under Values in the >"CONFIGURATION FILE" section of linkgit:git-config[1]; > - adding `auto,` at the beginning will emit color only when colors are > - enabled for log output (by `color.diff`, `color.ui`, or `--color`, and > - respecting the `auto` settings of the former if we are going to a > - terminal). `auto` alone (i.e. `%C(auto)`) will turn on auto coloring > - on the next placeholders until the color is switched again. > + adding `auto,` at the beginning (e.g. `%C(auto,red)`) will emit > + color only when colors are enabled for log output (by `color.diff`, > + `color.ui`, or `--color`, and respecting the `auto` settings of the > + former if we are going to a terminal). `auto` alone (i.e. > + `%C(auto)`) will turn on auto coloring on the next placeholders > + until the color is switched again. > - '%m': left (`<`), right (`>`) or boundary (`-`) mark > - '%n': newline > - '%%': a raw '%'
Re: [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
René Scharfe writes:
> Am 24.06.2017 um 14:14 schrieb Ævar Arnfjörð Bjarmason:
>> Change the code for deciding what's to be done about %Z to stop
>> passing always either a NULL or "" char * to
>> strbuf_addftime(). Instead pass a boolean int to indicate whether the
>> strftime() %Z format should be suppressed by converting it to an empty
>> string, which is what this code is actually doing.
>>
>> This code grew organically between the changes in 9eafe86d58 ("Merge
>> branch 'rs/strbuf-addftime-zZ'", 2017-06-22). The intent was to use
>> this API in the future to pass a custom leave the door open to pass a
>> custom timezone name to the function (see my [1] and related
>> messages).
>
> "leave the door open to pass a" seems redundant.
The intent was to use this API in the future to leave the door open
to pass a custom timezone name to the function (see my [1] and
related messages).
perhaps?
>> But that's not what this code does now, and this strbuf_addstr() call
>> always being redundant makes it hard to understand the current
>> functionality. So simplify this internal API to match its use, we can
>> always change it in the future if it gets a different use-case.
>
> I don't understand the confusion, but of course I'm biased. And I don't
> like binary parameters in general and would use named flags or two
> function names in most cases. But that aside I find the description
> hard to follow (perhaps I should do something about my attention span).
I share this feeling.
> Here's an attempt at a commit message that would have be easier to
> understand for me:
>
> strbuf_addstr() allows callers to pass a time zone name for expanding
> %Z. The only current caller either passes the empty string or NULL,
> in which case %Z is handed over verbatim to strftime(3). Replace that
> string parameter with a flag controlling whether to remove %Z from the
> format specification. This simplifies the code.
I think the first one is strbuf_addftime(); other than that, I think
this version explains what is going on in this patch than the
original.
I'll wait for Ævar to respond, but my inclination is to take the
patch with the above tweaks to the log message, as the change is
easy to revert if we find it necessary.
Re: [RFC/PATCH 2/3] wildmatch: add interface for precompiling wildmatch() patterns
Ævar Arnfjörð Bjarmason writes: > On Sat, Jun 24 2017, Junio C. Hamano jotted: > >> Is the far-in-the-future vision to make this the other way around? >> That is, this being scaffolding, wildmatch_match() which is supposed >> to be precompiled match actually uses wildmatch() as its underlying >> engine, but when a viable compilation machinery is plugged in, the >> wildmatch_match() that takes a precompiled pattern will call into >> the machinery to execute the compiled pattern, and wildmatch() will >> be reimplemented as "compile, call wildmatch_match() once and >> discard" sequence? > > Exactly there would be no functional difference in the results, only > fixed overhead. OK, good! Thanks.
Re: [PATCH] git-p4: changelist template with p4 -G change -o
> On 24 Jun 2017, at 13:49, Luke Diamand wrote:
>
> On 22 June 2017 at 18:32, Junio C Hamano wrote:
>> Miguel Torroja writes:
>>
>>> The option -G of p4 (python marshal output) gives more context about the
>>> data being output. That's useful when using the command "change -o" as
>>> we can distinguish between warning/error line and real change description.
>>>
>>> Some p4 plugin/hooks in the server side generates some warnings when
>>> executed. Unfortunately those messages are mixed with the output of
>>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>>> in python marshal output (-G). The real change output is reported as
>>> {'code':'stat'}
>
> I think this seems like a reasonable thing to do if "p4 change -o" is
> jumbling up output.
>
> One thing I notice trying it out by hand is that we seem to have lost
> the annotation of the Perforce per-file modification type (is there a
> proper name for this?).
>
> For example, if I add a file called "baz", then the original version
> creates a template which looks like this:
>
> //depot/baz# add
>
> But the new one creates a template which looks like:
>
> //depot/baz
@Miguel: You wrote that p4 plugins/hooks generate these warnings.
I wonder if you see a way to replicate that in a test case. Either
in t9800 or a new t98XX test case file?
- Lars
Re: [PATCH v5 5/5] convert: add "status=delayed" to filter process protocol
> On 24 Jun 2017, at 16:19, Jeff King wrote:
>
> On Thu, Jun 01, 2017 at 10:22:03AM +0200, Lars Schneider wrote:
>
>> Some `clean` / `smudge` filters might require a significant amount of
>> time to process a single blob (e.g. the Git LFS smudge filter might
>> perform network requests). During this process the Git checkout
>> operation is blocked and Git needs to wait until the filter is done to
>> continue with the checkout.
>>
>> Teach the filter process protocol (introduced in edcc858) to accept the
>> status "delayed" as response to a filter request. Upon this response Git
>> continues with the checkout operation. After the checkout operation Git
>> calls "finish_delayed_checkout" which queries the filter for remaining
>> blobs. If the filter is still working on the completion, then the filter
>> is expected to block. If the filter has completed all remaining blobs
>> then an empty response is expected.
>>
>> Git has a multiple code paths that checkout a blob. Support delayed
>> checkouts only in `clone` (in unpack-trees.c) and `checkout` operations.
>
> It might be worth giving a reason in this last paragraph. I think the
> reason is "because it's more complicated for the caller, as they have to
> be OK with out-of-order processing and remembering to go back and handle
> the delayed cases".
Correct! However, my real reason was that these code paths process all
files of the tree. Therefore the "out-of-order" processing can be
effective.
How about this:
Git has a multiple code paths that checkout a blob. Support delayed
checkouts only in `clone` (in unpack-trees.c) and `checkout` operations
for now. The optimization is most effective in these code paths as all
files of the tree are processed.
> Speaking of which, _are_ we OK with out-of-order processing in things
> like checkout? Certainly we care about deleting items before checking
> out new ones (so getting rid of "foo/bar" to make room for "foo" and
> vice versa). I guess it's OK as long as the delayed items are always
> filters that check out new items.
Junio noticed that too but thinks we should be OK:
"[...] We checkout removals first to make room so
that creation of a path X can succeed if an existing path X/Y
that used to want to see X as a directory can succeed [...]"
http://public-inbox.org/git/xmqqvavotych@gitster.mtv.corp.google.com/
>> ...
>
>> +After Git received the pathnames, it will request the corresponding
>> +blobs again. These requests contain a pathname and an empty content
>> +section. The filter is expected to respond with the smudged content
>> +in the usual way as explained above.
>> +
>> +packet: git> command=smudge
>> +packet: git> pathname=path/testfile.dat
>> +packet: git>
>> +packet: git> # empty content!
>> +packet: git< status=success
>> +packet: git<
>> +packet: git< SMUDGED_CONTENT
>> +packet: git<
>> +packet: git< # empty list, keep "status=success" unchanged!
>> +
>
> Out of curiosity, what should happen if we re-ask for a pathname that
> wasn't mentioned in list_available_blobs? I guess it would depend on the
> filter implementation, but probably most would just block (since we
> wouldn't have asked for can-delay). The other option is returning an
> error, I suppose, but blocking and filling the request sounds
> friendlier.
I agree that is up to the filter. I would expect the blocking strategy.
The filter cannot delay it anymore as we have not send the "can-delay"
flag.
> ...
>> diff --git a/cache.h b/cache.h
>> index ae4c45d379..523ead1d95 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1543,16 +1543,20 @@ extern int ident_cmp(const struct ident_split *,
>> const struct ident_split *);
>> struct checkout {
>> struct index_state *istate;
>> const char *base_dir;
>> +struct delayed_checkout *delayed_checkout;
>> int base_dir_len;
>
> Should base_dir_len and base_dir be kept together?
>
> I suspect you ordered it this way because...
>
>> unsigned force:1,
>> quiet:1,
>> not_new:1,
>> refresh_cache:1;
>> };
>> -#define CHECKOUT_INIT { NULL, "" }
>> +#define CHECKOUT_INIT { NULL, "", NULL }
>> +
>
> ...you wanted to add the NULL to the initializer here. But it's not
> necessary. Once one element of a struct is initialized, all the
> remaining members are initialized according to the usual static rules
> (so NULL for pointers). We're already using that to zero out
> base_dir_len and the flags (the "" is necessary because we want a real
> empty string, not NULL).
>
> Since you want NULL for your new member, you can just leave the
> initializer as-is and it will do the right thing.
That was indeed my reasoning. I am no C expert and I wasn't sure about
the initialization. The "Once one element of a struct is initialized"
thing is news to me :-) Thanks!
I'll keep the ba
Re: Using '--help' for aliases
On Thu, Jun 22, 2017 at 12:11:29AM +0530, Kaartic Sivaraam wrote:
> I am yet another user of 'git alias' (who wouldn't ?). It has become so
> natural to me to use the aliased version that at some point of time I
> tried the following,
>
> > $ git co --help
> > `git co' is aliased to `checkout'
>
> That made me wonder. Git is able to inform the user that 'co' is
> aliased to 'checkout' but isn't it possible for it to take one step
> more to display help ? Just wondering if there were any reason for not
> doing it.
It's possible to do if "co" is an alias for "checkout", but it works
less well when co is the following[0]:
co = "!f() { if git checkout -h | grep -qs recurse-submodules; \
then git checkout --recurse-submodules \"$@\"; \
else git checkout \"$@\" && git sui; \
fi; };f"
If this alias were to run on a Git with that option but without checkout
--recurse-submodules, then it would print help and then update
submodles, which isn't what I want.
[0] sui is an alias for submodule update --init.
--
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
EOL LF Windows auto.crlf issue
I'm on Windows 10. auto.crlf in .gitconfig is set to [core] autocrlf = true I've got a git (git version 2.13.1.windows.2) repo. A linux guy emails me a text with with line endings LF. I paste this file into my repo. Now every time I introduce changes to this file and stage it, git tell me: "warning: LF will be replaced by CRLF in a.txt. The file will have its original line endings in your working directory." But when I commit the file git does not replace anyline endings - they stay LF in the commited file and in my working direcotry. I'm sort of misleaded by this message. Filip
Re: [PATCH v5 0/5] convert: add "status=delayed" to filter process protocol
On Thu, Jun 01, 2017 at 10:21:58AM +0200, Lars Schneider wrote: > here is the 5th iteration of my "status delayed" topic. Patch 1 to 3 are > minor t0021 test adjustments and haven't been changed since v3. Patch 4 > is a minor "extract method" refactoring in convert. Patch 5 is the new > feature. Yeah, patches 1-4 were pretty straightforward and obvious. I just left some comments after a detailed read-through of patch 5. Overall the direction looks good. I had a few minor comments on the code, but I think with those addressed it will probably be ready for 'next'. Note that I'll be offline all next week, so don't expect a timely review on the next iteration (unlike this not-so-timely one ;) ). -Peff
Re: [PATCH v5 5/5] convert: add "status=delayed" to filter process protocol
On Thu, Jun 01, 2017 at 10:22:03AM +0200, Lars Schneider wrote: > Some `clean` / `smudge` filters might require a significant amount of > time to process a single blob (e.g. the Git LFS smudge filter might > perform network requests). During this process the Git checkout > operation is blocked and Git needs to wait until the filter is done to > continue with the checkout. > > Teach the filter process protocol (introduced in edcc858) to accept the > status "delayed" as response to a filter request. Upon this response Git > continues with the checkout operation. After the checkout operation Git > calls "finish_delayed_checkout" which queries the filter for remaining > blobs. If the filter is still working on the completion, then the filter > is expected to block. If the filter has completed all remaining blobs > then an empty response is expected. > > Git has a multiple code paths that checkout a blob. Support delayed > checkouts only in `clone` (in unpack-trees.c) and `checkout` operations. It might be worth giving a reason in this last paragraph. I think the reason is "because it's more complicated for the caller, as they have to be OK with out-of-order processing and remembering to go back and handle the delayed cases". Speaking of which, _are_ we OK with out-of-order processing in things like checkout? Certainly we care about deleting items before checking out new ones (so getting rid of "foo/bar" to make room for "foo" and vice versa). I guess it's OK as long as the delayed items are always filters that check out new items. > +Delay > +^ > + > +If the filter supports the "delay" capability, then Git can send the > +flag "can-delay" after the filter command and pathname. This flag > +denotes that the filter can delay filtering the current blob (e.g. to > +compensate network latencies) by responding with no content but with > +the status "delayed" and a flush packet. > + > +packet: git> command=smudge > +packet: git> pathname=path/testfile.dat > +packet: git> can-delay=1 > +packet: git> > +packet: git> CONTENT > +packet: git> > +packet: git< status=delayed > +packet: git< > + OK, so the client has to opt into delay for each entry. Makes sense. > +If the filter supports the "delay" capability then it must support the > +"list_available_blobs" command. If Git sends this command, then the > +filter is expected to return a list of pathnames of blobs that are > +available. The list must be terminated with a flush packet followed > +by a "success" status that is also terminated with a flush packet. If > +no blobs for the delayed paths are available, yet, then the filter is > +expected to block the response until at least one blob becomes > +available. The filter can tell Git that it has no more delayed blobs > +by sending an empty list. > + > +packet: git> command=list_available_blobs > +packet: git> > +packet: git< pathname=path/testfile.dat > +packet: git< pathname=path/otherfile.dat > +packet: git< > +packet: git< status=success > +packet: git< > + Earlier I proposed just having the client ask "give me something that's ready". This introduces an extra round-trip to say "show me what's ready" and ask for it. But I think that's OK, as it should be a local process running over a pipe. The important thing is that the client is able to always fetch without blocking when something is ready, and to block when nothing is ready, which this does. Good. > +After Git received the pathnames, it will request the corresponding > +blobs again. These requests contain a pathname and an empty content > +section. The filter is expected to respond with the smudged content > +in the usual way as explained above. > + > +packet: git> command=smudge > +packet: git> pathname=path/testfile.dat > +packet: git> > +packet: git> # empty content! > +packet: git< status=success > +packet: git< > +packet: git< SMUDGED_CONTENT > +packet: git< > +packet: git< # empty list, keep "status=success" unchanged! > + Out of curiosity, what should happen if we re-ask for a pathname that wasn't mentioned in list_available_blobs? I guess it would depend on the filter implementation, but probably most would just block (since we wouldn't have asked for can-delay). The other option is returning an error, I suppose, but blocking and filling the request sounds friendlier. > diff --git a/builtin/checkout.c b/builtin/checkout.c > index a6b2af39d3..c1a256df8d 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -376,6 +376,8 @@ static int checkout_paths(const struct checkout_opts > *opts, > state.force = 1; > s
Re: [PATCH] sha1_name: cache readdir(3) results in find_short_object_filename()
On Sat, Jun 24, 2017 at 04:09:39PM +0200, René Scharfe wrote:
> Am 24.06.2017 um 14:20 schrieb Jeff King:
> > On Sat, Jun 24, 2017 at 02:12:30PM +0200, René Scharfe wrote:
> >
> >>> That's redundant with "subdir_nr". Should we push that logic down into
> >>> the function, and basically do:
> >>>
> >>> if (subdir_nr < 0 || subdir_nr > 256)
> >>> BUG("object subdir number out of range");
> >>
> >> Hmm. I don't expect many more callers, so do we really need this safety
> >> check? It's cheap compared to the readdir(3) call, of course.
> >
> > To me it's as much about documenting the assumptions as it is about
> > catching buggy callers. I'd be OK with a comment, too.
>
> I didn't catch the off-by-one error above in the first reading. Did you
> add it intentionally? ;-) In any case, I'm convinced now that we need
> such a check, but with hexadecimal notation to avoid such mistakes.
Err...yeah, it was totally intentional. ;)
I agree writing it in hex is better.
> -- >8 --
> Subject: [PATCH] sha1_file: guard against invalid loose subdirectory numbers
>
> Loose object subdirectories have hexadecimal names based on the first
> byte of the hash of contained objects, thus their numerical
> representation can range from 0 (0x00) to 255 (0xff). Change the type
> of the corresponding variable in for_each_file_in_obj_subdir() and
> associated callback functions to unsigned int and add a range check.
>
> Suggested-by: Jeff King
> Signed-off-by: Rene Scharfe
This looks great. Thanks.
-Peff
Re: [PATCH] sha1_name: cache readdir(3) results in find_short_object_filename()
Am 24.06.2017 um 14:20 schrieb Jeff King:
> On Sat, Jun 24, 2017 at 02:12:30PM +0200, René Scharfe wrote:
>
>>> That's redundant with "subdir_nr". Should we push that logic down into
>>> the function, and basically do:
>>>
>>> if (subdir_nr < 0 || subdir_nr > 256)
>>> BUG("object subdir number out of range");
>>
>> Hmm. I don't expect many more callers, so do we really need this safety
>> check? It's cheap compared to the readdir(3) call, of course.
>
> To me it's as much about documenting the assumptions as it is about
> catching buggy callers. I'd be OK with a comment, too.
I didn't catch the off-by-one error above in the first reading. Did you
add it intentionally? ;-) In any case, I'm convinced now that we need
such a check, but with hexadecimal notation to avoid such mistakes.
>> But wouldn't it make more sense to use an unsigned data type to avoid
>> the first half? And an unsigned char specifically to only accept valid
>> values, leaving the overflow concern to the caller? It warrants a
>> separate patch in any case IMHO.
>
> Using unsigned makes sense. Using "char" because you know it only goes
> to 256 is a bit too subtle for my taste. And yes, I'd do it in a
> preparatory patch (or follow-on if you prefer).
It's subtle on the caller's side, as big numbers would just wrap if the
programmer ignored the limits of the type. On the callee's side it's
fundamental, though.
Anyway, here's a patch on top of the others.
-- >8 --
Subject: [PATCH] sha1_file: guard against invalid loose subdirectory numbers
Loose object subdirectories have hexadecimal names based on the first
byte of the hash of contained objects, thus their numerical
representation can range from 0 (0x00) to 255 (0xff). Change the type
of the corresponding variable in for_each_file_in_obj_subdir() and
associated callback functions to unsigned int and add a range check.
Suggested-by: Jeff King
Signed-off-by: Rene Scharfe
---
builtin/fsck.c | 2 +-
builtin/prune-packed.c | 2 +-
builtin/prune.c| 2 +-
cache.h| 4 ++--
sha1_file.c| 5 -
5 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 3a2c27f241..5d113f8bc0 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -536,7 +536,7 @@ static int fsck_cruft(const char *basename, const char
*path, void *data)
return 0;
}
-static int fsck_subdir(int nr, const char *path, void *progress)
+static int fsck_subdir(unsigned int nr, const char *path, void *progress)
{
display_progress(progress, nr + 1);
return 0;
diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index c026299e78..ac978ad401 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c
@@ -10,7 +10,7 @@ static const char * const prune_packed_usage[] = {
static struct progress *progress;
-static int prune_subdir(int nr, const char *path, void *data)
+static int prune_subdir(unsigned int nr, const char *path, void *data)
{
int *opts = data;
display_progress(progress, nr + 1);
diff --git a/builtin/prune.c b/builtin/prune.c
index f0e2bff04c..c378690545 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -68,7 +68,7 @@ static int prune_cruft(const char *basename, const char
*path, void *data)
return 0;
}
-static int prune_subdir(int nr, const char *path, void *data)
+static int prune_subdir(unsigned int nr, const char *path, void *data)
{
if (!show_only)
rmdir(path);
diff --git a/cache.h b/cache.h
index 00a017dfcb..04dd2961ad 100644
--- a/cache.h
+++ b/cache.h
@@ -1819,10 +1819,10 @@ typedef int each_loose_object_fn(const struct object_id
*oid,
typedef int each_loose_cruft_fn(const char *basename,
const char *path,
void *data);
-typedef int each_loose_subdir_fn(int nr,
+typedef int each_loose_subdir_fn(unsigned int nr,
const char *path,
void *data);
-int for_each_file_in_obj_subdir(int subdir_nr,
+int for_each_file_in_obj_subdir(unsigned int subdir_nr,
struct strbuf *path,
each_loose_object_fn obj_cb,
each_loose_cruft_fn cruft_cb,
diff --git a/sha1_file.c b/sha1_file.c
index 98ce85acf9..90b9414170 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3735,7 +3735,7 @@ void assert_sha1_type(const unsigned char *sha1, enum
object_type expect)
typename(expect));
}
-int for_each_file_in_obj_subdir(int subdir_nr,
+int for_each_file_in_obj_subdir(unsigned int subdir_nr,
struct strbuf *path,
each_loose_object_fn obj_cb,
each_loose_cruft_fn cruft_cb,
@@ -3747,6 +3747,9 @@ int for_each_file_in_obj_subdir(int subdir_nr,
struct dirent *de;
int r = 0;
+ if (subdir_nr > 0xff)
+
Re: [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Am 24.06.2017 um 14:14 schrieb Ævar Arnfjörð Bjarmason:
Change the code for deciding what's to be done about %Z to stop
passing always either a NULL or "" char * to
strbuf_addftime(). Instead pass a boolean int to indicate whether the
strftime() %Z format should be suppressed by converting it to an empty
string, which is what this code is actually doing.
This code grew organically between the changes in 9eafe86d58 ("Merge
branch 'rs/strbuf-addftime-zZ'", 2017-06-22). The intent was to use
this API in the future to pass a custom leave the door open to pass a
custom timezone name to the function (see my [1] and related
messages).
"leave the door open to pass a" seems redundant.
But that's not what this code does now, and this strbuf_addstr() call
always being redundant makes it hard to understand the current
functionality. So simplify this internal API to match its use, we can
always change it in the future if it gets a different use-case.
I don't understand the confusion, but of course I'm biased. And I don't
like binary parameters in general and would use named flags or two
function names in most cases. But that aside I find the description
hard to follow (perhaps I should do something about my attention span).
Here's an attempt at a commit message that would have be easier to
understand for me:
strbuf_addstr() allows callers to pass a time zone name for expanding
%Z. The only current caller either passes the empty string or NULL,
in which case %Z is handed over verbatim to strftime(3). Replace that
string parameter with a flag controlling whether to remove %Z from the
format specification. This simplifies the code.
René
Re: [PATCH v4 0/8] Improvements to sha1_file
On Mon, Jun 19, 2017 at 06:03:07PM -0700, Jonathan Tan wrote: > > I had the same thoughts (both on the name and the "vocabularies"). IMHO > > we should consider allocating the bits from the same set. There's only > > one HAS_SHA1 flag, and it has an exact match in OBJECT_INFO_QUICK. > > Agreed - in this patch set, I have also consolidated the relevant flags, > including LOOKUP_REPLACE_OBJECT and LOOKUP_UNKNOWN_OBJECT. > > In addition, Junio has mentioned the potential confusion in behavior > between a NULL and an empty struct passed to > sha1_object_info_extended(). In this patch set, I require non-NULL, and > have added an optimization that avoids accessing the pack in certain > situations, but this optimization requires checking a lot of fields. Let > me know what you think. Yes, like that direction (and the direction of the whole series) much better. Thanks for working on it. I'm trying to clear my "to be reviewed" backlog before going offline for a week, so I gave it a fairly cursory review. I had only a few minor comments, but I agree with the points that Junio already raised. -Peff
Re: [PATCH v4 7/8] sha1_file: do not access pack if unneeded
On Wed, Jun 21, 2017 at 11:15:01AM -0700, Junio C Hamano wrote:
> > + if (!oi->typep && !oi->sizep && !oi->disk_sizep &&
> > + !oi->delta_base_sha1 && !oi->typename && !oi->contentp &&
> > + !oi->populate_u) {
> > + oi->whence = OI_PACKED;
> > + return 0;
> > + }
> > +
>
> ... this "if" statement feels like a maintenance nightmare. The
> intent of the guard, I think, is "when the call wants absolutely
> nothing but whence", but the implementation of the guard will not
> stay true to the intent whenever somebody adds a new field to oi.
>
> I wonder if it makes more sense to have a new field "whence_only",
> which is set only by such a specialized caller, which this guard
> checks (and no other fields).
The other nice thing about whence_only is that it flips the logic. So
any existing callers which depend on filling the union automatically
will not be affected (though I would be surprised if there are any such
callers; most of that information isn't actually that interesting).
-Peff
Re: [PATCH v4 6/8] sha1_file: improve sha1_object_info_extended
On Mon, Jun 19, 2017 at 06:03:13PM -0700, Jonathan Tan wrote: > Subject: [PATCH v4 6/8] sha1_file: improve sha1_object_info_extended > Improve sha1_object_info_extended() by supporting additional flags. This > allows has_sha1_file_with_flags() to be modified to use > sha1_object_info_extended() in a subsequent patch. A minor nit, but try to avoid vague words like "improve" in your subject lines. Something like: sha1_file: teach sha1_object_info_extended more flags That's not too specific either, but I think in --oneline output it gives you a much better clue about what part of the function it touches. > --- > cache.h | 4 > sha1_file.c | 43 --- > 2 files changed, 28 insertions(+), 19 deletions(-) The patch itself looks good. -Peff
Re: [PATCH] git-p4: changelist template with p4 -G change -o
You are right about the "# add"comment. I couldn't find any extra info
in the marshaled output that I can use to add the change action
comment after the path. That's one downside of that change.
On Sat, Jun 24, 2017 at 1:49 PM, Luke Diamand wrote:
> On 22 June 2017 at 18:32, Junio C Hamano wrote:
>> Miguel Torroja writes:
>>
>>> The option -G of p4 (python marshal output) gives more context about the
>>> data being output. That's useful when using the command "change -o" as
>>> we can distinguish between warning/error line and real change description.
>>>
>>> Some p4 plugin/hooks in the server side generates some warnings when
>>> executed. Unfortunately those messages are mixed with the output of
>>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>>> in python marshal output (-G). The real change output is reported as
>>> {'code':'stat'}
>
> I think this seems like a reasonable thing to do if "p4 change -o" is
> jumbling up output.
>
> One thing I notice trying it out by hand is that we seem to have lost
> the annotation of the Perforce per-file modification type (is there a
> proper name for this?).
>
> For example, if I add a file called "baz", then the original version
> creates a template which looks like this:
>
>//depot/baz# add
>
> But the new one creates a template which looks like:
>
>//depot/baz
>
> Luke
Re: [PATCH v2] die(): stop hiding errors due to overzealous recursion guard
On Wed, Jun 21, 2017 at 02:32:16PM -0700, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
> > So let's just set the recursion limit to a number higher than the
> > number of threads we're ever likely to spawn. Now we won't lose
> > errors, and if we have a recursing die handler we'll still die within
> > microseconds.
> >
> > There are race conditions in this code itself, in particular the
> > "dying" variable is not thread mutexed, so we e.g. won't be dying at
> > exactly 1024, or for that matter even be able to accurately test
> > "dying == 2", see the cases where we print out more than one "W"
> > above.
>
> One case I'd be worried about would be that the race is so bad that
> die-is-recursing-builtin never returns 0 even once. Everybody will
> just say "recursing" and die, without giving any useful information.
I was trying to think how that would happen. If nobody's actually
recursing indefinitely, then the value in theory peaks at the number of
threads (modulo the fact that we're modifying a variable from multiple
threads without any locking; I'm not sure how reasonable it is to assume
in practice that sheared writes may cause us to lose an increment but
not to put nonsense in to the variable). If they are, then one thread
may increment it to 1024 before another thread gets a chance to say
anything. But in that case, the recursion-die is our expected outcome.
Anyway, it might be reasonable to protect the counter with a mutex.
Like:
diff --git a/usage.c b/usage.c
index fc2b31c54b..34fef0f9fa 100644
--- a/usage.c
+++ b/usage.c
@@ -44,9 +44,19 @@ static void warn_builtin(const char *warn, va_list params)
vreportf("warning: ", warn, params);
}
+#ifndef NO_PTHREADS
+static pthread_mutex_t recursion_mutex = PTHREAD_MUTEX_INITIALIZER;
+#define recursion_lock() pthread_mutex_lock(&recursion_mutex)
+#define recursion_unlock() pthread_mutex_unlock(&recursion_mutex)
+#else
+#define recursion_lock()
+#define recursion_unlock()
+#endif
+static int recursion_counter;
+
static int die_is_recursing_builtin(void)
{
- static int dying;
+ int dying;
/*
* Just an arbitrary number X where "a < x < b" where "a" is
* "maximum number of pthreads we'll ever plausibly spawn" and
@@ -55,7 +65,10 @@ static int die_is_recursing_builtin(void)
*/
static const int recursion_limit = 1024;
- dying++;
+ recursion_lock();
+ dying = ++recursion_counter;
+ recursion_unlock();
+
if (dying > recursion_limit) {
return 1;
} else if (dying == 2) {
I can't remember if there are problems on Windows with using constant
mutex initializers, though. If so, I guess common-main would have to
initialize it.
I left the rest of the logic as-is, but if we switched to post-increment:
dying = recursion_counter++;
then I think the numbers around "dying" would make more sense (e.g.,
"dying == 2" would make more sense to me as "dying == 1" to check that
we were already dying).
To be honest, I'm not sure if it's worth giving it much more time,
though. I'd be fine with Ævar's patch as-is.
-Peff
Re: [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
On Sat, Jun 24, 2017 at 12:14:52PM +, Ævar Arnfjörð Bjarmason wrote: > On Sat, Jun 24, 2017 at 2:10 PM, Ævar Arnfjörð Bjarmason > wrote: > > Thanks. Docs fixed per your suggestion. I sent a v4 of 1/2 too, but > > that's unchanged, just thought it was simpler than having just one > > patch have a v4... > > Urgh, mistake on my end, sent v3 again as v4. Here's v5 with the > *actual* fixes. Sorry. Heh, I skimmed over v4 again and thought "this looks good", but the one thing I _didn't_ read was the final hunk that actually changed from v3. Yikes. So much for my code review skills. -Peff
Re: [PATCH] sha1_name: cache readdir(3) results in find_short_object_filename()
On Sat, Jun 24, 2017 at 02:12:30PM +0200, René Scharfe wrote:
> > That's redundant with "subdir_nr". Should we push that logic down into
> > the function, and basically do:
> >
> >if (subdir_nr < 0 || subdir_nr > 256)
> > BUG("object subdir number out of range");
>
> Hmm. I don't expect many more callers, so do we really need this safety
> check? It's cheap compared to the readdir(3) call, of course.
To me it's as much about documenting the assumptions as it is about
catching buggy callers. I'd be OK with a comment, too.
> But wouldn't it make more sense to use an unsigned data type to avoid
> the first half? And an unsigned char specifically to only accept valid
> values, leaving the overflow concern to the caller? It warrants a
> separate patch in any case IMHO.
Using unsigned makes sense. Using "char" because you know it only goes
to 256 is a bit too subtle for my taste. And yes, I'd do it in a
preparatory patch (or follow-on if you prefer).
> -- >8 --
> Subject: [PATCH] sha1_file: let for_each_file_in_obj_subdir() handle subdir
> names
>
> The function for_each_file_in_obj_subdir() takes a object subdirectory
> number and expects the name of the same subdirectory to be included in
> the path strbuf. Avoid this redundancy by letting the function append
> the hexadecimal subdirectory name itself. This makes it a bit easier
> and safer to use the function -- it becomes impossible to specify
> different subdirectories in subdir_nr and path.
Yeah, this is what I had in mind.
> for (i = 0; i < 256; i++) {
> - strbuf_addf(path, "/%02x", i);
> r = for_each_file_in_obj_subdir(i, path, obj_cb, cruft_cb,
> subdir_cb, data);
> - strbuf_setlen(path, baselen);
One side effect of the original code is that this trailing setlen()
would catch any early-exits from for_each_file_in_obj_subdir() which
forgot to reset the strbuf. If any exist, that's a bug that should be in
fixed in that function, though. I double-checked, and there aren't any
(your patch already handles the extra setlen required when opendir
fails).
-Peff
[PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Change the code for deciding what's to be done about %Z to stop
passing always either a NULL or "" char * to
strbuf_addftime(). Instead pass a boolean int to indicate whether the
strftime() %Z format should be suppressed by converting it to an empty
string, which is what this code is actually doing.
This code grew organically between the changes in 9eafe86d58 ("Merge
branch 'rs/strbuf-addftime-zZ'", 2017-06-22). The intent was to use
this API in the future to pass a custom leave the door open to pass a
custom timezone name to the function (see my [1] and related
messages).
But that's not what this code does now, and this strbuf_addstr() call
always being redundant makes it hard to understand the current
functionality. So simplify this internal API to match its use, we can
always change it in the future if it gets a different use-case.
1. CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com
(https://public-inbox.org/git/CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com/)
Signed-off-by: Ævar Arnfjörð Bjarmason
---
On Sat, Jun 24, 2017 at 2:10 PM, Ævar Arnfjörð Bjarmason
wrote:
> Thanks. Docs fixed per your suggestion. I sent a v4 of 1/2 too, but
> that's unchanged, just thought it was simpler than having just one
> patch have a v4...
Urgh, mistake on my end, sent v3 again as v4. Here's v5 with the
*actual* fixes. Sorry.
date.c | 2 +-
strbuf.c | 5 ++---
strbuf.h | 5 +++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/date.c b/date.c
index 1fd6d66375..c3e673fd04 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const
struct date_mode *mode)
tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
else if (mode->type == DATE_STRFTIME)
strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
- mode->local ? NULL : "");
+ !mode->local);
else
strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index be3b9e37b1..89e40bb496 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -786,7 +786,7 @@ char *xstrfmt(const char *fmt, ...)
}
void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
-int tz_offset, const char *tz_name)
+int tz_offset, int suppress_tz_name)
{
struct strbuf munged_fmt = STRBUF_INIT;
size_t hint = 128;
@@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm,
fmt++;
break;
case 'Z':
- if (tz_name) {
- strbuf_addstr(&munged_fmt, tz_name);
+ if (suppress_tz_name) {
fmt++;
break;
}
diff --git a/strbuf.h b/strbuf.h
index 6708cef0f9..9262615ca0 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -343,11 +343,12 @@ extern void strbuf_vaddf(struct strbuf *sb, const char
*fmt, va_list ap);
* `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
* of Greenwich, and it's used to expand %z internally. However, tokens
* with modifiers (e.g. %Ez) are passed to `strftime`.
- * `tz_name` is used to expand %Z internally unless it's NULL.
+ * `suppress_tz_name`, when set, expands %Z internally to the empty
+ * string rather than passing it to `strftime`.
*/
extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm, int tz_offset,
- const char *tz_name);
+ int suppress_tz_name);
/**
* Read a given size of data from a FILE* pointer to the buffer.
--
2.13.1.611.g7e3b11ae1
[PATCH v5 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order
Change the comment documenting the strbuf_addftime() function to discuss the parameters in the order in which they appear, which makes this easier to read than discussing them out of order. Signed-off-by: Ævar Arnfjörð Bjarmason --- strbuf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/strbuf.h b/strbuf.h index 4559035c47..6708cef0f9 100644 --- a/strbuf.h +++ b/strbuf.h @@ -340,10 +340,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap); /** * Add the time specified by `tm`, as formatted by `strftime`. - * `tz_name` is used to expand %Z internally unless it's NULL. * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west * of Greenwich, and it's used to expand %z internally. However, tokens * with modifiers (e.g. %Ez) are passed to `strftime`. + * `tz_name` is used to expand %Z internally unless it's NULL. */ extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm, int tz_offset, -- 2.13.1.611.g7e3b11ae1
Re: [PATCH] sha1_name: cache readdir(3) results in find_short_object_filename()
On Sat, Jun 24, 2017 at 02:12:07PM +0200, René Scharfe wrote: > Am 23.06.2017 um 01:10 schrieb Jeff King: > > On Thu, Jun 22, 2017 at 08:19:48PM +0200, René Scharfe wrote: > > > >> Read each loose object subdirectory at most once when looking for unique > >> abbreviated hashes. This speeds up commands like "git log --pretty=%h" > >> considerably, which previously caused one readdir(3) call for each > >> candidate, even for subdirectories that were visited before. > > > > Is it worth adding a perf test that's heavy on abbreviations? > > Sure. Here's a simple one. It currently reports for me: > > Testorigin/master origin/next > origin/pu > - > 4205.1: log with %H 0.44(0.41+0.02) 0.43(0.42+0.01) -2.3% > 0.43(0.43+0.00) -2.3% > 4205.2: log with %h 1.03(0.60+0.42) 1.04(0.64+0.39) +1.0% > 0.57(0.55+0.01) -44.7% > 4205.3: log with %T 0.43(0.42+0.00) 0.43(0.42+0.01) +0.0% > 0.43(0.40+0.02) +0.0% > 4205.4: log with %t 1.05(0.64+0.38) 1.05(0.61+0.42) +0.0% > 0.59(0.58+0.00) -43.8% > 4205.5: log with %P 0.45(0.42+0.02) 0.43(0.42+0.00) -4.4% > 0.43(0.42+0.01) -4.4% > 4205.6: log with %p 1.21(0.63+0.57) 1.17(0.68+0.47) -3.3% > 0.59(0.58+0.00) -51.2% > 4205.7: log with %h-%h-%h 1.05(0.64+0.39) 2.00(0.83+1.15) +90.5% > 0.69(0.66+0.02) -34.3% > > origin/next has fe9e2aefd4 (pretty: recalculate duplicate short hashes), > while origin/pu has cc817ca3ef (sha1_name: cache readdir(3) results in > find_short_object_filename()). Perfect. That last one says everything we need to know about which approach to take. :) -Peff
Re: [PATCH v4 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
On Sat, Jun 24, 2017 at 12:10:23PM +, Ævar Arnfjörð Bjarmason wrote: > > I couldn't quite parse "let suppress". I'm not sure if it was supposed > > to be "let's". Probably "means to suppress the strftime..." would be > > more clear. I'd probably have written it more like: > > > > `suppress_tz_name`, when set, expands %Z internally to the empty > > string rather than passing it to `strftime`. > > Thanks. Docs fixed per your suggestion. I sent a v4 of 1/2 too, but > that's unchanged, just thought it was simpler than having just one > patch have a v4... Thanks, both of the v4 patches look OK to me. -Peff
Re: [PATCH] sha1_name: cache readdir(3) results in find_short_object_filename()
Am 23.06.2017 um 01:10 schrieb Jeff King: > On Thu, Jun 22, 2017 at 08:19:48PM +0200, René Scharfe wrote: > >> Read each loose object subdirectory at most once when looking for unique >> abbreviated hashes. This speeds up commands like "git log --pretty=%h" >> considerably, which previously caused one readdir(3) call for each >> candidate, even for subdirectories that were visited before. > > Is it worth adding a perf test that's heavy on abbreviations? Sure. Here's a simple one. It currently reports for me: Testorigin/master origin/next origin/pu - 4205.1: log with %H 0.44(0.41+0.02) 0.43(0.42+0.01) -2.3% 0.43(0.43+0.00) -2.3% 4205.2: log with %h 1.03(0.60+0.42) 1.04(0.64+0.39) +1.0% 0.57(0.55+0.01) -44.7% 4205.3: log with %T 0.43(0.42+0.00) 0.43(0.42+0.01) +0.0% 0.43(0.40+0.02) +0.0% 4205.4: log with %t 1.05(0.64+0.38) 1.05(0.61+0.42) +0.0% 0.59(0.58+0.00) -43.8% 4205.5: log with %P 0.45(0.42+0.02) 0.43(0.42+0.00) -4.4% 0.43(0.42+0.01) -4.4% 4205.6: log with %p 1.21(0.63+0.57) 1.17(0.68+0.47) -3.3% 0.59(0.58+0.00) -51.2% 4205.7: log with %h-%h-%h 1.05(0.64+0.39) 2.00(0.83+1.15) +90.5% 0.69(0.66+0.02) -34.3% origin/next has fe9e2aefd4 (pretty: recalculate duplicate short hashes), while origin/pu has cc817ca3ef (sha1_name: cache readdir(3) results in find_short_object_filename()). -- >8 -- Subject: [PATCH] p4205: add perf test script for pretty log formats Add simple performance tests for expanded log format placeholders. Suggested-by: Jeff King Signed-off-by: Rene Scharfe --- t/perf/p4205-log-pretty-formats.sh | 16 1 file changed, 16 insertions(+) create mode 100755 t/perf/p4205-log-pretty-formats.sh diff --git a/t/perf/p4205-log-pretty-formats.sh b/t/perf/p4205-log-pretty-formats.sh new file mode 100755 index 00..7c26f4f337 --- /dev/null +++ b/t/perf/p4205-log-pretty-formats.sh @@ -0,0 +1,16 @@ +#!/bin/sh + +test_description='Tests the performance of various pretty format placeholders' + +. ./perf-lib.sh + +test_perf_default_repo + +for format in %H %h %T %t %P %p %h-%h-%h +do + test_perf "log with $format" " + git log --format=\"$format\" >/dev/null + " +done + +test_done -- 2.13.1
Re: [PATCH] sha1_name: cache readdir(3) results in find_short_object_filename()
Am 23.06.2017 um 01:10 schrieb Jeff King:
> On Thu, Jun 22, 2017 at 08:19:48PM +0200, René Scharfe wrote:
>> @@ -1811,6 +1822,12 @@ typedef int each_loose_cruft_fn(const char *basename,
>> typedef int each_loose_subdir_fn(int nr,
>> const char *path,
>> void *data);
>> +int for_each_file_in_obj_subdir(int subdir_nr,
>> +struct strbuf *path,
>> +each_loose_object_fn obj_cb,
>> +each_loose_cruft_fn cruft_cb,
>> +each_loose_subdir_fn subdir_cb,
>> +void *data);
>
> Now that this is becoming public, I think we need to document what
> should be in "path" here. It is the complete path, including the 2-hex
> subdir.
>
> That's redundant with "subdir_nr". Should we push that logic down into
> the function, and basically do:
>
>if (subdir_nr < 0 || subdir_nr > 256)
> BUG("object subdir number out of range");
Hmm. I don't expect many more callers, so do we really need this safety
check? It's cheap compared to the readdir(3) call, of course.
But wouldn't it make more sense to use an unsigned data type to avoid
the first half? And an unsigned char specifically to only accept valid
values, leaving the overflow concern to the caller? It warrants a
separate patch in any case IMHO.
>orig_len = path->len;
>strbuf_addf(path, "/%02x", subdir_nr);
>baselen = path->len;
>
>...
>
>strbuf_setlen(path, orig_len);
>
> That's one less thing for the caller to worry about, and it's easy to
> explain the argument as "the path to the object directory root".
>
>> +if (!alt->loose_objects_subdir_seen[subdir_nr]) {
>> +struct strbuf *buf = alt_scratch_buf(alt);
>> +strbuf_addf(buf, "%02x/", subdir_nr);
>> +for_each_file_in_obj_subdir(subdir_nr, buf,
>> +append_loose_object,
>> +NULL, NULL,
>> +&alt->loose_objects_cache);
>
> I think the trailing slash here is superfluous. If you take my
> suggestion above, that line goes away. But then the front slash it adds
> becomes superfluous. It should probably just do:
>
>strbuf_complete(path, '/');
>strbuf_addf(path, "%02x", subdir_nr);
So how about this then as a follow-up patch?
-- >8 --
Subject: [PATCH] sha1_file: let for_each_file_in_obj_subdir() handle subdir
names
The function for_each_file_in_obj_subdir() takes a object subdirectory
number and expects the name of the same subdirectory to be included in
the path strbuf. Avoid this redundancy by letting the function append
the hexadecimal subdirectory name itself. This makes it a bit easier
and safer to use the function -- it becomes impossible to specify
different subdirectories in subdir_nr and path.
Suggested-by: Jeff King
Signed-off-by: Rene Scharfe
---
sha1_file.c | 22 ++
sha1_name.c | 1 -
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 5e0ee2b68b..98ce85acf9 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3742,15 +3742,22 @@ int for_each_file_in_obj_subdir(int subdir_nr,
each_loose_subdir_fn subdir_cb,
void *data)
{
- size_t baselen = path->len;
- DIR *dir = opendir(path->buf);
+ size_t origlen, baselen;
+ DIR *dir;
struct dirent *de;
int r = 0;
+ origlen = path->len;
+ strbuf_complete(path, '/');
+ strbuf_addf(path, "%02x", subdir_nr);
+ baselen = path->len;
+
+ dir = opendir(path->buf);
if (!dir) {
- if (errno == ENOENT)
- return 0;
- return error_errno("unable to open %s", path->buf);
+ if (errno != ENOENT)
+ r = error_errno("unable to open %s", path->buf);
+ strbuf_setlen(path, origlen);
+ return r;
}
while ((de = readdir(dir))) {
@@ -3788,6 +3795,8 @@ int for_each_file_in_obj_subdir(int subdir_nr,
if (!r && subdir_cb)
r = subdir_cb(subdir_nr, path->buf, data);
+ strbuf_setlen(path, origlen);
+
return r;
}
@@ -3797,15 +3806,12 @@ int for_each_loose_file_in_objdir_buf(struct strbuf
*path,
each_loose_subdir_fn subdir_cb,
void *data)
{
- size_t baselen = path->len;
int r = 0;
int i;
for (i = 0; i < 256; i++) {
- strbuf_addf(path, "/%02x", i);
r = for_each_file_in_obj_subdir(i, path, obj_cb, cruft_cb,
subdir_cb, data);
- strbuf_setlen(path, baselen);
if (r)
break;
[PATCH v4 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Change the code for deciding what's to be done about %Z to stop
passing always either a NULL or "" char * to
strbuf_addftime(). Instead pass a boolean int to indicate whether the
strftime() %Z format should be suppressed by converting it to an empty
string, which is what this code is actually doing.
This code grew organically between the changes in 9eafe86d58 ("Merge
branch 'rs/strbuf-addftime-zZ'", 2017-06-22). The intent was to use
this API in the future to pass a custom leave the door open to pass a
custom timezone name to the function (see my [1] and related
messages).
But that's not what this code does now, and this strbuf_addstr() call
always being redundant makes it hard to understand the current
functionality. So simplify this internal API to match its use, we can
always change it in the future if it gets a different use-case.
1. CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com
(https://public-inbox.org/git/CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com/)
Signed-off-by: Ævar Arnfjörð Bjarmason
---
On Sat, Jun 24, 2017 at 2:02 PM, Jeff King wrote:
> On Sat, Jun 24, 2017 at 11:36:35AM +, Ævar Arnfjörð Bjarmason wrote:
>
>> >> extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
>> >> const struct tm *tm, int tz_offset,
>> >> - const char *tz_name);
>> >> + const int omit_strftime_tz_name);
>> >
>> > This would need the new name, too (whatever it is).
>>
>> *Nod*. Now the parameter is called suppress_tz_name.
>
> Thanks. That sounds good (and your initial re-ordering patch looks fine,
> too). One minor typo:
>
>> diff --git a/strbuf.h b/strbuf.h
>> index 6708cef0f9..d3e6e65123 100644
>> --- a/strbuf.h
>> +++ b/strbuf.h
>> @@ -343,11 +343,12 @@ extern void strbuf_vaddf(struct strbuf *sb, const char
>> *fmt, va_list ap);
>> * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
>> * of Greenwich, and it's used to expand %z internally. However, tokens
>> * with modifiers (e.g. %Ez) are passed to `strftime`.
>> - * `tz_name` is used to expand %Z internally unless it's NULL.
>> + * `suppress_tz_name` when set, means let suppress the `strftime` %Z
>> + * format and replace it with an empty string.
>
> I couldn't quite parse "let suppress". I'm not sure if it was supposed
> to be "let's". Probably "means to suppress the strftime..." would be
> more clear. I'd probably have written it more like:
>
> `suppress_tz_name`, when set, expands %Z internally to the empty
> string rather than passing it to `strftime`.
Thanks. Docs fixed per your suggestion. I sent a v4 of 1/2 too, but
that's unchanged, just thought it was simpler than having just one
patch have a v4...
date.c | 2 +-
strbuf.c | 5 ++---
strbuf.h | 5 +++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/date.c b/date.c
index 1fd6d66375..c3e673fd04 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const
struct date_mode *mode)
tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
else if (mode->type == DATE_STRFTIME)
strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
- mode->local ? NULL : "");
+ !mode->local);
else
strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index be3b9e37b1..89e40bb496 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -786,7 +786,7 @@ char *xstrfmt(const char *fmt, ...)
}
void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
-int tz_offset, const char *tz_name)
+int tz_offset, int suppress_tz_name)
{
struct strbuf munged_fmt = STRBUF_INIT;
size_t hint = 128;
@@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm,
fmt++;
break;
case 'Z':
- if (tz_name) {
- strbuf_addstr(&munged_fmt, tz_name);
+ if (suppress_tz_name) {
fmt++;
break;
}
diff --git a/strbuf.h b/strbuf.h
index 6708cef0f9..d3e6e65123 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -343,11 +343,12 @@ extern void strbuf_vaddf(struct strbuf *sb, const char
*fmt, va_list ap);
* `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
* of Greenwich, and it's used to expand %z internally. However, tokens
* with modifiers (e.g. %Ez) are passed to `strftime`.
- * `tz_name` is used to expand %Z internally unless it's NULL.
+ * `suppress_tz_name` when set, means let suppress the `strftime` %Z
+ * format and replace it with an empty string.
*/
extern void strbuf_addftime(st
[PATCH v4 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order
Change the comment documenting the strbuf_addftime() function to discuss the parameters in the order in which they appear, which makes this easier to read than discussing them out of order. Signed-off-by: Ævar Arnfjörð Bjarmason --- strbuf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/strbuf.h b/strbuf.h index 4559035c47..6708cef0f9 100644 --- a/strbuf.h +++ b/strbuf.h @@ -340,10 +340,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap); /** * Add the time specified by `tm`, as formatted by `strftime`. - * `tz_name` is used to expand %Z internally unless it's NULL. * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west * of Greenwich, and it's used to expand %z internally. However, tokens * with modifiers (e.g. %Ez) are passed to `strftime`. + * `tz_name` is used to expand %Z internally unless it's NULL. */ extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm, int tz_offset, -- 2.13.1.611.g7e3b11ae1
Re: [PATCH v3 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
On Sat, Jun 24, 2017 at 11:36:35AM +, Ævar Arnfjörð Bjarmason wrote: > >> extern void strbuf_addftime(struct strbuf *sb, const char *fmt, > >>const struct tm *tm, int tz_offset, > >> - const char *tz_name); > >> + const int omit_strftime_tz_name); > > > > This would need the new name, too (whatever it is). > > *Nod*. Now the parameter is called suppress_tz_name. Thanks. That sounds good (and your initial re-ordering patch looks fine, too). One minor typo: > diff --git a/strbuf.h b/strbuf.h > index 6708cef0f9..d3e6e65123 100644 > --- a/strbuf.h > +++ b/strbuf.h > @@ -343,11 +343,12 @@ extern void strbuf_vaddf(struct strbuf *sb, const char > *fmt, va_list ap); > * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west > * of Greenwich, and it's used to expand %z internally. However, tokens > * with modifiers (e.g. %Ez) are passed to `strftime`. > - * `tz_name` is used to expand %Z internally unless it's NULL. > + * `suppress_tz_name` when set, means let suppress the `strftime` %Z > + * format and replace it with an empty string. I couldn't quite parse "let suppress". I'm not sure if it was supposed to be "let's". Probably "means to suppress the strftime..." would be more clear. I'd probably have written it more like: `suppress_tz_name`, when set, expands %Z internally to the empty string rather than passing it to `strftime`. -Peff
Re: [PATCH v2 22/29] commit_packed_refs(): use a staging file separate from the lockfile
On Sat, Jun 24, 2017 at 01:43:09PM +0200, Michael Haggerty wrote: > >> out: > >> + rollback_lock_file(&refs->lock); > > > > We always rollback the lockfile regardless, because committing it would > > overwrite our new content with an empty file. There's no real safety > > against somebody calling commit_lock_file() on it, but it also seems > > like an uncommon error to make. > > If this seems too dangerous, we could add a `LOCK_NO_COMMIT` flag for > `hold_lock_file_for_update()` and `hold_lock_file_for_update_timeout()`, > which would die() if somebody tries to commit the associated lockfile. I > think we can live without it. I hope that any such bugs would be caught > immediately by CI. But I admit that the prospect of renaming an empty > file on top of a `packed-refs` file is quite sobering, so if anybody is > worried about this, let me know and I'll implement it. Yeah, that was the protection I was thinking of. Reflecting on it more, though, it's not really any different than somebody calling commit_lock_file() when we haven't correctly written out the whole content. It's probably not worth adding code to protect against this special case. -Peff
Re: [PATCH] git-p4: changelist template with p4 -G change -o
On 22 June 2017 at 18:32, Junio C Hamano wrote:
> Miguel Torroja writes:
>
>> The option -G of p4 (python marshal output) gives more context about the
>> data being output. That's useful when using the command "change -o" as
>> we can distinguish between warning/error line and real change description.
>>
>> Some p4 plugin/hooks in the server side generates some warnings when
>> executed. Unfortunately those messages are mixed with the output of
>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>> in python marshal output (-G). The real change output is reported as
>> {'code':'stat'}
I think this seems like a reasonable thing to do if "p4 change -o" is
jumbling up output.
One thing I notice trying it out by hand is that we seem to have lost
the annotation of the Perforce per-file modification type (is there a
proper name for this?).
For example, if I add a file called "baz", then the original version
creates a template which looks like this:
//depot/baz# add
But the new one creates a template which looks like:
//depot/baz
Luke
Re: [PATCH v2 22/29] commit_packed_refs(): use a staging file separate from the lockfile
On 06/23/2017 09:46 PM, Jeff King wrote:
> On Fri, Jun 23, 2017 at 09:01:40AM +0200, Michael Haggerty wrote:
>
>> @@ -522,10 +529,16 @@ int lock_packed_refs(struct ref_store *ref_store, int
>> flags)
>> timeout_configured = 1;
>> }
>>
>> +/*
>> + * Note that we close the lockfile immediately because we
>> + * don't write new content to it, but rather to a separate
>> + * tempfile.
>> + */
>> if (hold_lock_file_for_update_timeout(
>> &refs->lock,
>> refs->path,
>> -flags, timeout_value) < 0)
>> +flags, timeout_value) < 0 ||
>> +close_lock_file(&refs->lock))
>> return -1;
>
> I was wondering whether we'd catch a case which accidentally wrote to
> the lockfile (instead of the new tempfile, but this close() is a good
> safety check).
>
>> -if (commit_lock_file(&refs->lock)) {
>> -strbuf_addf(err, "error overwriting %s: %s",
>> +if (rename_tempfile(&refs->tempfile, refs->path)) {
>> +strbuf_addf(err, "error replacing %s: %s",
>> refs->path, strerror(errno));
>> goto out;
>> }
>
> So our idea of committing now is the tempfile rename, and then...
>
>> @@ -617,9 +640,10 @@ int commit_packed_refs(struct ref_store *ref_store,
>> struct strbuf *err)
>> goto out;
>>
>> error:
>> -rollback_lock_file(&refs->lock);
>> +delete_tempfile(&refs->tempfile);
>>
>> out:
>> +rollback_lock_file(&refs->lock);
>
> We always rollback the lockfile regardless, because committing it would
> overwrite our new content with an empty file. There's no real safety
> against somebody calling commit_lock_file() on it, but it also seems
> like an uncommon error to make.
If this seems too dangerous, we could add a `LOCK_NO_COMMIT` flag for
`hold_lock_file_for_update()` and `hold_lock_file_for_update_timeout()`,
which would die() if somebody tries to commit the associated lockfile. I
think we can live without it. I hope that any such bugs would be caught
immediately by CI. But I admit that the prospect of renaming an empty
file on top of a `packed-refs` file is quite sobering, so if anybody is
worried about this, let me know and I'll implement it.
Michael
[PATCH v3 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Change the code for deciding what's to be done about %Z to stop
passing always either a NULL or "" char * to
strbuf_addftime(). Instead pass a boolean int to indicate whether the
strftime() %Z format should be suppressed by converting it to an empty
string, which is what this code is actually doing.
This code grew organically between the changes in 9eafe86d58 ("Merge
branch 'rs/strbuf-addftime-zZ'", 2017-06-22). The intent was to use
this API in the future to pass a custom leave the door open to pass a
custom timezone name to the function (see my [1] and related
messages).
But that's not what this code does now, and this strbuf_addstr() call
always being redundant makes it hard to understand the current
functionality. So simplify this internal API to match its use, we can
always change it in the future if it gets a different use-case.
1. CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com
(https://public-inbox.org/git/CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com/)
Signed-off-by: Ævar Arnfjörð Bjarmason
---
On Fri, Jun 23 2017, Jeff King jotted:
> On Fri, Jun 23, 2017 at 04:36:06PM +, Ævar Arnfjörð Bjarmason wrote:
>
>> I believe this addresses the comments in the thread so far. Also Re:
>> René's "why const?" in a2673ce4-5cf8-6b40-d4db-8e2a49518...@web.de:
>> Because tzname_from_tz isn't changed in the body of the function, only
>> read.
>
> Sure, it's not wrong. But that property is also held by 99% of the
> parameters that are passed by value. It's the normal style in our code
> base (and in most C code bases I know of) to never declare pass-by-value
> as const. It pollutes the interface and isn't something the caller cares
> about.
>
> Without passing judgement on whether that style is good or not (though
> IMHO it is), making this one case different than all the others is a bad
> idea. It makes the reader wonder why it's different.
Makes sense. I wasn't trying to be snary or curt or whatever. I'd just
never noticed this pattern in the codebase.
Seems a bit odd to me to not make use of the compiler guarding against
accidental assignments and giving it a strong hint to inline the value
where possible, but whatever, makes sense to have it stylistically be
consistent. So this version does that.
>> diff --git a/date.c b/date.c
>> index 1fd6d66375..17db07d905 100644
>> --- a/date.c
>> +++ b/date.c
>> @@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const
>> struct date_mode *mode)
>> tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
>> else if (mode->type == DATE_STRFTIME)
>> strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
>> -mode->local ? NULL : "");
>> +mode->local);
>
> You flipped the boolean here. That's OK by me. But in the definition...
>
>> void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm
>> *tm,
>> - int tz_offset, const char *tz_name)
>> + int tz_offset, const int tzname_from_tz)
>
> Wouldn't tzname_from_tz only happen when we're _not_ in local mode? I
> suggested that name anticipating your second patch to actually compute
> it based on "tz". In local-mode it's not coming from tz, it's coming
> from secret unportable magic (the combination of localtime() and
> strftime()).
I misread (I think) an earlier E-Mail of yours and thought this was
what you were suggesting. This version hopefully looks OK.
>> @@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt,
>> const struct tm *tm,
>> fmt++;
>> break;
>> case 'Z':
>> -if (tz_name) {
>> -strbuf_addstr(&munged_fmt, tz_name);
>> +if (!tzname_from_tz) {
>> fmt++;
>> break;
>> }
>
> This logic matches your inversion in the caller, so it does the right
> thing. But I think the name is wrong, as above.
Fixed.
>> index 4559035c47..eba5d59a77 100644
>> --- a/strbuf.h
>> +++ b/strbuf.h
>> @@ -340,14 +340,15 @@ extern void strbuf_vaddf(struct strbuf *sb, const char
>> *fmt, va_list ap);
>>
>> /**
>> * Add the time specified by `tm`, as formatted by `strftime`.
>> - * `tz_name` is used to expand %Z internally unless it's NULL.
>> * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
>> * of Greenwich, and it's used to expand %z internally. However, tokens
>> * with modifiers (e.g. %Ez) are passed to `strftime`.
>> + * `tzname_from_tz` when set, means let `strftime` format %Z, instead
>> + * of intercepting it and doing our own formatting.
>> */
>> extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
>> const struct tm *tm, int tz_offset,
>> -const char *tz_name);
>> +const int omit_strftime_tz_name);
[PATCH v3 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order
Change the comment documenting the strbuf_addftime() function to discuss the parameters in the order in which they appear, which makes this easier to read than discussing them out of order. Signed-off-by: Ævar Arnfjörð Bjarmason --- I though it was more readable to split out this change into its own patch. strbuf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/strbuf.h b/strbuf.h index 4559035c47..6708cef0f9 100644 --- a/strbuf.h +++ b/strbuf.h @@ -340,10 +340,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap); /** * Add the time specified by `tm`, as formatted by `strftime`. - * `tz_name` is used to expand %Z internally unless it's NULL. * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west * of Greenwich, and it's used to expand %z internally. However, tokens * with modifiers (e.g. %Ez) are passed to `strftime`. + * `tz_name` is used to expand %Z internally unless it's NULL. */ extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm, int tz_offset, -- 2.13.1.611.g7e3b11ae1
Re: [PATCH v2 00/29] Create a reference backend for packed refs
On 06/24/2017 03:11 AM, Jeff King wrote: > On Fri, Jun 23, 2017 at 02:47:01PM -0700, Junio C Hamano wrote: > >>> Speculating on my own question. I guess it would prepare us for a day >>> when a possible ref store is to use a packed-refs _without_ loose refs. >>> IOW, the property is defined on packed-refs today, but a possible future >>> direction would be to use it by itself. But maybe I'm just making things >>> up. >> >> OK. In other words, it's not a packed-refs's characteristics that >> cruft are allowed. It's that a ref storage that is implemented as >> an overlay of one storage (which happens to be the loose one) on top >> of another (which happens to be the packed refs file) allows the >> latter one to have cruft if (and only if) that broken one is covered >> by the former one. > > Thanks, that's a much better way of saying what I was trying to get at. > I don't know if that's Michael's argument or not, but it's certainly one > I find reasonable. :) That was exactly my thinking. A packed-without-loose storage scheme might, for example, be interesting for people with case-insensitive or strangely-Unicode-normalized filesystems but have colleagues who like to use case or Unicode in their reference names. (Of course that would still require a way to store symbolic refs and reflogs, so I'm not saying that we're there yet.) I also think it is a good idea to keep the backends' interfaces as similar as possible to reduce the number of quirks that the reader has to keep in mind. Michael
Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
On Fri, Jun 23 2017, René Scharfe jotted:
> Am 23.06.2017 um 17:23 schrieb Jeff King:
>> On Fri, Jun 23, 2017 at 05:13:38PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
The idea was that eventually the caller might be able to come up with a
TZ that is not blank, but is also not what strftime("%Z") would produce.
Conceivably that could be done if Git commits carried the "%Z"
information (not likely), or if we used a reverse-lookup table (also not
likely).
This closes the door on that. Since we don't have immediate plans to go
that route, I'm OK with this patch. It would be easy enough to re-open
the door if we change our minds later.
>>>
>>> Closes the door on doing that via passing the char * of the prepared
>>> custom tz_name to strbuf_addftime().
>>>
>>> I have a WIP patch (which may not make it on-list, depending) playing
>>> with the idea I proposed in
>>> CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com which
>>> just inserts the custom TZ name based on the offset inside that `if
>>> (omit_strftime_tz_name)` branch.
>>
>> OK. I'd assumed that would all happen outside of strbuf_addftime(). But
>> if it happens inside, then I agree a flag is better.
>
> Oh, so the interface that was meant to allow better time zone names
> without having to make strbuf_addftime() even bigger than it already is
> turns out to be too ugly for its purpose? I'm sorry. :(
I don't think it's ugly. My motivation for sending this patch is that I
started playing with this code and was confused because I thought that
strbuf_addstr(...) actually did something to the string, but it never
did.
Since it's a purely internal API used in just one place I thought it
made sense to adjust the prototype / code to its current usage for ease
of readability, if we want to do something else with it in the future
it'll be trivial to adjust it then.
But I don't feel strongly about this patch at all, it's just a minor
fixup I submitted while reading / playing with the code.
Re: [RFC/PATCH 2/3] wildmatch: add interface for precompiling wildmatch() patterns
On Sat, Jun 24 2017, Junio C. Hamano jotted:
> Ævar Arnfjörð Bjarmason writes:
>
>> +struct wildmatch_compiled *wildmatch_compile(const char *pattern, unsigned
>> int flags)
>> +{
>> +struct wildmatch_compiled *code = xmalloc(sizeof(struct
>> wildmatch_compiled));
>> +code->pattern = xstrdup(pattern);
>> +code->flags = flags;
>> +
>> +return code;
>> +}
>> +
>> +int wildmatch_match(struct wildmatch_compiled *code, const char *text)
>> +{
>> +return wildmatch(code->pattern, text, code->flags);
>> +}
>
> Is the far-in-the-future vision to make this the other way around?
> That is, this being scaffolding, wildmatch_match() which is supposed
> to be precompiled match actually uses wildmatch() as its underlying
> engine, but when a viable compilation machinery is plugged in, the
> wildmatch_match() that takes a precompiled pattern will call into
> the machinery to execute the compiled pattern, and wildmatch() will
> be reimplemented as "compile, call wildmatch_match() once and
> discard" sequence?
Exactly there would be no functional difference in the results, only
fixed overhead.
wildmatch() would be the one-off lazy interface and
wildmatch_{compile,match,free}(), just like how you can have a wrapper
function that calls regcomp() followed by regexec() & regfree(), but
it's better to structure your code so you're not compiling & freeing the
pattern all the time.
Right now of course there's no difference in the behavior, and a
trivially more overhead with the extra xstrdup() & free(), but I wanted
to split up the discussion of the semantics of the interface from any
actual behavior change in wildmatch() which would make use of it further
down the line.
> Otherwise I'd be worried about wildmatch() vs wildmatch_match()
> introducing subtle behaviour differences that leads to hard to debug
> problems.
this is urgent reply.
Greetings My Dear Friend, Before I introduce myself, I wish to inform you that this letter is not a hoax mail and I urge you to treat it serious. This letter must come to you as a big surprise, but I believe it is only a day that people meet and become great friends and business partners. Please I want you to read this letter very carefully and I must apologize for barging this message into your mail box without any formal introduction due to the urgency and confidentiality of this business and I know that this message will come to you as a surprise. Please this is not a joke and I will not like you to joke with it ok, MY name is Mr. zaki ibrahim i am working in ADB bank I have ($17.4million Dollars) to transfer to your country and if you are interested get back to me immediately for more details.and i we give you 40% for you and 60% for me ok. Please note; reply me through this email (mrzakiibra...@gmail.com), call me +226 68 25 71 46 Mr. zaki ibrahim. Telex Manager African Development Bank (ADB)
Re: My Second Email to You, Pls Reply Me Soon
Hello Dear, How are you doing? I hope you are doing well. I am writing as I have written to you previously without any response from you. I hope all is well with you.I will appreciate if you will acknowledge your receipt of this mail. Thank you and have a good day. Mrs Fatma. Please Write Me at My Private email which i used to send you the previous email( d...@t-com.me )
[PATCH] doc: clarify syntax for %C(auto,...) in pretty formats
The change actually adds only (e.g. `%C(auto,red)`) but reflowing the paragraph blows it up a little. 8< The manual correctly describes the syntax with `auto,` but the trailing `,` is hard to spot in a terminal. The HTML format does not have this problem. Adding an example helps both worlds. Signed-off-by: Andreas Heiduk --- Documentation/pretty-formats.txt | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 38040e95b..b03985101 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -174,11 +174,12 @@ endif::git-rev-list[] - '%Creset': reset color - '%C(...)': color specification, as described under Values in the "CONFIGURATION FILE" section of linkgit:git-config[1]; - adding `auto,` at the beginning will emit color only when colors are - enabled for log output (by `color.diff`, `color.ui`, or `--color`, and - respecting the `auto` settings of the former if we are going to a - terminal). `auto` alone (i.e. `%C(auto)`) will turn on auto coloring - on the next placeholders until the color is switched again. + adding `auto,` at the beginning (e.g. `%C(auto,red)`) will emit + color only when colors are enabled for log output (by `color.diff`, + `color.ui`, or `--color`, and respecting the `auto` settings of the + former if we are going to a terminal). `auto` alone (i.e. + `%C(auto)`) will turn on auto coloring on the next placeholders + until the color is switched again. - '%m': left (`<`), right (`>`) or boundary (`-`) mark - '%n': newline - '%%': a raw '%' -- 2.13.1
