Question on for-each-ref and --contains
Hi. In order to make some analyses here I had to build certain report lists of our remote refs, based on various "containing" and "merged" rules. We have more than hundred such refs at a time usually. So my poor script which tries to make decisions with the help of rev-list for each ref on each arbitrary commit is not really fast. The git-branch --contains and --merged are not very handy too, because the output is not really flexible, and the --merged works on HEAD only. Do you think is a good idea to have the option "--contains" in for-each-ref too (and/or in rev-list directly)? If yes, then probably also the --(no-)merged options, but hopefully with the arbitrary commit this time? Thank you in advance, Blind. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Saving space/network on common repos
} This seems like good motivation to try to get that series in good
shape and release it soon.
I was going to spend some time tomorrow (if I can find any :-) )
trying to fix up the contrib script to work with submodules, or at
least the kind that we use. Is that something that's worth the time
to do, or would we be better off just waiting for the work-tree stuff
to get released? If I do end up doing it, would you be interested in
a pull request (or however patches are submitted in the git world)?
craig
On Mon, Dec 22, 2014 at 7:12 PM, Jonathan Nieder wrote:
> Craig Silverstein wrote:
>
>> btw, just FYI, the scheme you lay out here doesn't actually work
>> as-is. The problem is the config file, which has an entry like:
>>worktree = ../../../mysubmodule
>> This depends on the config file living in
>> ./git/modules/mysubmodule/config. But the proposed scheme moves the
>> config file to mysubmodule/.git/config, and the relative path is
>> broken.
>
> As was pointed out to me privately, the behavior is exactly as you
> described and I had confused myself by looking at directory that
> wasn't even made with git-new-workdir. Sorry for the nonsense.
>
> Workdirs share a single config file because information associated to
> branches set by "git branch --set-upstream-to", "git branch
> --edit-description", "git remote", and so on are stored in the config
> file.
>
> The 'git checkout --to' series in "pu" avoids this problem by ignoring
> core.bare and core.worktree in worktrees created with 'git checkout --to'.
> To try it:
>
> git clone https://kernel.googlesource.com/pub/scm/git/git
> cd git
> git merge 'origin/pu^{/nd/multiple-work-trees}^2'
> make
> PATH=$(pwd)/bin-wrappers:$PATH
>
> git checkout --to=../experiment next
>
> This seems like good motivation to try to get that series in good
> shape and release it soon.
>
> Thanks again,
> Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git update-ref --stdin : too many open files
Hi Stefan, Stefan Beller wrote: > On 22.12.2014 13:22, Junio C Hamano wrote: >> Loic Dachary writes: >>> fatal: Unable to create >>> /home/gitmirror/repositories/Ceph/ceph/refs/heads/pull/1917.lock': Too many >>> open files [...] >> Stefan, want to take a look? I think we do need to keep the .lock >> files without renaming while in transaction, but we do not have to >> keep them open, so I suspect that a fix may be to split the commit >> function into two (one to close but not rename, the other to >> finalize by renaming) or something. Makes sense. > Sounds reasonable. Though by closing the file we're giving up again a > bit of safety. If we close the file everyone could tamper with the lock > file. (Sure they are not supposed to touch it, but they could) At least on Linux, keeping a file open doesn't offer any protection against someone else deleting it. (It also doesn't offer any protection against someone updating the ref directly. ;-) Opening the corresponding .lock file with O_EXCL is part of the contract for updating refs.) Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Saving space/network on common repos
Craig Silverstein wrote:
> btw, just FYI, the scheme you lay out here doesn't actually work
> as-is. The problem is the config file, which has an entry like:
>worktree = ../../../mysubmodule
> This depends on the config file living in
> ./git/modules/mysubmodule/config. But the proposed scheme moves the
> config file to mysubmodule/.git/config, and the relative path is
> broken.
As was pointed out to me privately, the behavior is exactly as you
described and I had confused myself by looking at directory that
wasn't even made with git-new-workdir. Sorry for the nonsense.
Workdirs share a single config file because information associated to
branches set by "git branch --set-upstream-to", "git branch
--edit-description", "git remote", and so on are stored in the config
file.
The 'git checkout --to' series in "pu" avoids this problem by ignoring
core.bare and core.worktree in worktrees created with 'git checkout --to'.
To try it:
git clone https://kernel.googlesource.com/pub/scm/git/git
cd git
git merge 'origin/pu^{/nd/multiple-work-trees}^2'
make
PATH=$(pwd)/bin-wrappers:$PATH
git checkout --to=../experiment next
This seems like good motivation to try to get that series in good
shape and release it soon.
Thanks again,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: XDL_FAST_HASH can be very slow
(sorry for the repost, I use gmail and it send html mails by default). On 22 December 2014 at 11:48, Thomas Rast wrote: > > 1. hash function throughput > 2. quality of the hash values > 3. avoiding collision attacks > > XDL_FAST_HASH was strictly an attempt to improve throughput, and fairly > successful at that (6942efc (xdiff: load full words in the inner loop of > xdl_hash_record, 2012-04-06) quotes an 8% improvement on 'git log -p'). > > You are now addressing quality. > > I have no idea how you ran into this, but if you are reworking things > already, I think it would be good to also randomize whatever hash you > put in so as to give some measure of protection against collision > attacks. I assume you mean DJB2 when you say DJB, and if so I will just note that it is a pretty terrible hash function for arbitrary data. (I understand it does better with raw text.) It does not pass either strict-avalanche-criteria[1], nor does it pass the bit-independence-criteria[2]. I have images which show how badly DJB2 fails these tests if anyone is interested. Murmur3 is better, in that it does pass SAC and BIC, but before you decide to use Murmur3 you should review https://131002.net/siphash/and related resources which demonstrate multi-collision attacks on Murmur3 which are independent of the seed chosen. The paper also introduces a new hash function with good performance properties, and claims that it has cyptographic strength. (I say claims because I am not qualified to judge if it is or not.) Eg: https://131002.net/siphash/murmur3collisions-20120827.tar.gz I think if you want performance and robustness against collision attacks Siphash is a good candidate, as is perhaps the AES derived hash used by the Go folks, but the performance of that algorithm is strongly dependent on the CPU supporting AES primitives. Anyway, the point is that simply adding a random seed to a hash function like DJB2 or Murmur3 is not sufficient to prevent collision attacks. Yves [1] A change in a single bit of the seed or the key should result in 50% of the output bits of the hash changing. [2] output bits j and k should change independently when any single input bit i is inverted, for all i, j and k. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git update-ref --stdin : too many open files
On 22.12.2014 13:22, Junio C Hamano wrote: > Loic Dachary writes: > >> Hi, >> >> Steps to reproduce: >> >> $ git --version >> git version 1.9.1 >> $ wc -l /tmp/1 >> 9090 /tmp/1 >> $ head /tmp/1 >> delete refs/pull/1/head >> create refs/heads/pull/1 86b715f346e52920ca7c9dfe65424eb9946ebd61 >> delete refs/pull/1/merge >> create refs/merges/1 c0633abdc5311354c9729374e0ba25c97a89f69e >> ... >> $ ulimit -n >> 1024 >> $ git update-ref --stdin < /tmp/1 >> fatal: Unable to create >> /home/gitmirror/repositories/Ceph/ceph/refs/heads/pull/1917.lock': Too >> many open files >> $ head -20 /tmp/1 | git update-ref --stdin >> $ echo $? >> 0 >> >> The workaround is to increase ulimit -n >> >> git update-ref --stdin should probably close some files. >> >> Cheers > > Sounds like the recent "ref update in a transaction" issue to me. > > Stefan, want to take a look? I think we do need to keep the .lock > files without renaming while in transaction, but we do not have to > keep them open, so I suspect that a fix may be to split the commit > function into two (one to close but not rename, the other to > finalize by renaming) or something. Sounds reasonable. Though by closing the file we're giving up again a bit of safety. If we close the file everyone could tamper with the lock file. (Sure they are not supposed to touch it, but they could) > > Also the version of transaction series we have queued seem to lock > these refs very late in the process, but as we discussed privately > a few weeks ago, we would want to move the lock much earlier, when > the first update is attempted. I'll look into that tomorrow. > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push
On 22.12.2014 14:52, Eric Sunshine wrote: > On Fri, Dec 19, 2014 at 2:38 PM, Stefan Beller wrote: >> From: Ronnie Sahlberg >> >> This adds support to the protocol between send-pack and receive-pack to >> * allow receive-pack to inform the client that it has atomic push capability >> * allow send-pack to request atomic push back. >> >> There is currently no setting in send-pack to actually request that atomic >> pushes are to be used yet. This only adds protocol capability not ability >> for the user to activate it. >> >> Signed-off-by: Ronnie Sahlberg >> Signed-off-by: Stefan Beller >> --- >> diff --git a/Documentation/technical/protocol-capabilities.txt >> b/Documentation/technical/protocol-capabilities.txt >> index 6d5424c..4f8a7bf 100644 >> --- a/Documentation/technical/protocol-capabilities.txt >> +++ b/Documentation/technical/protocol-capabilities.txt >> @@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress >> server-side progress >> reporting if the local progress reporting is also being suppressed >> (e.g., via `push -q`, or if stderr does not go to a tty). >> >> +atomic >> +-- >> + >> +If the server sends the 'atomic' capability it is capable of accepting >> +atomic pushes. If the pushing client requests this capability, the server >> +will update the refs in one atomic transaction. Either all refs are > > Not itself worth a re-send, but if you re-send for some other reason... > > "one atomic" still smacks of redundancy; "an atomic" sounds better. I did hear you saying 'one single atomic' being too redundant. And I agree that 'one' and 'single' makes the redundancy. However I have the impression 'an atomic' is too weak. Not everybody is a careful reader picking up the subtle notions. Not everybody is english native. Or concentrated. Look at it the other way: How could it be done? * All of the refs could be updated one at a time, not atomically, so foreach ref: open refs/heads/bla: write new sha1 * All of the refs could be updated at once, not atomically: open refs pack file: write new content * All of the refs could be updated, one at a time, atomically: foreach ref: get lock write content to lock rename into place * All of the refs at once, atomically. open packed refs file lock: write new content rename into place That said, atomicity and how many transactions there are, are orthogonal to each other. That's why I'd keep pointing out 'one atomic' transaction. Thanks for all the comments. I may be doing cleanup patches for you on top of what Junio queued. > >> +updated or none. >> + >> allow-tip-sha1-in-want >> -- > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.
Junio C Hamano writes: > Jim Hill writes: > >> I call it unwanted because the default works fine with the actual >> input and explicitly limiting whitespace this way breaks most command >> substitution. > > OK. I'd call that "unnecessary", not "unwanted", though. > > It becomes unwanted only when somebody cuts and pastes and changes > what happens inside the body of the loop without thinking what IFS > assignment is doing. > > Leaving it to the default is not wrong per-se, but I think it is > better to justify this change as protecting cut-and-paste people, > which is its primary benefit as far as I can see. > > Thanks for noticing. FYI, here is what I queued for today's integration cycle (you should be able to find it in 'pu' branch). -- >8 -- From: Jim Hill Date: Sun, 21 Dec 2014 11:26:00 -0800 Subject: [PATCH] pre-push.sample: remove unnecessary and misleading IFS=' ' The sample hook explicitly sets IFS to SP and nothing else so that the "read" used in the per-ref while loop that iterates over " SP SP SP " records, where we know refs and sha1s will not have SPs, would split them correctly. While this is not wrong per-se, it is not necessary; because we know these fields do not contain HT or LF, either, we can simply leave IFS the default. This will also prevent those who cut and paste from this sample from getting bitten when they write things in the per-ref loop that need splitting with the default $IFS (e.g. use $(git rev-list ...) to produce one-record-per-line output). Signed-off-by: Jim Hill Signed-off-by: Junio C Hamano --- templates/hooks--pre-push.sample | 1 - 1 file changed, 1 deletion(-) diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample index 69e3c67..6187dbf 100755 --- a/templates/hooks--pre-push.sample +++ b/templates/hooks--pre-push.sample @@ -24,7 +24,6 @@ url="$2" z40= -IFS=' ' while read local_ref local_sha remote_ref remote_sha do if [ "$local_sha" = $z40 ] -- 2.2.1-321-gd161b79 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Saving space/network on common repos
Craig Silverstein wrote: > btw, just FYI, the scheme you lay out here doesn't actually work > as-is. The problem is the config file, which has an entry like: >worktree = ../../../mysubmodule > This depends on the config file living in > ./git/modules/mysubmodule/config. But the proposed scheme moves the > config file to mysubmodule/.git/config, and the relative path is > broken. *puzzled* Can you give a transcript illustrating this happening? Submodules with .git directory within their worktree or under .git/modules/ are both supposed to work. And in either case, having refs/ and objects/ as symlinks should work fine. When git new-workdir creates a new workdir, it has its own new and separate config file, so I don't think that is the source of trouble. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes
On Fri, Dec 19, 2014 at 2:38 PM, Stefan Beller wrote:
> Update receive-pack to use an atomic transaction iff the client negotiated
> that it wanted atomic-push. This leaves the default behavior to be the old
> non-atomic one ref at a time update. This is to cause as little disruption
> as possible to existing clients. It is unknown if there are client scripts
> that depend on the old non-atomic behavior so we make it opt-in for now.
>
> If it turns out over time that there are no client scripts that depend on the
> old behavior we can change git to default to use atomic pushes and instead
> offer an opt-out argument for people that do not want atomic pushes.
Notes and observations below. None of them are particularly
actionable. If Junio is happy with the current round, and if you don't
have some other reason to re-roll, then consider them food for thought
for future patches.
> Inspired-by: Ronnie Sahlberg
> Helped-by: Eric Sunshine
> Signed-off-by: Stefan Beller
> ---
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index e76e5d5..710cd7f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1044,11 +1047,72 @@ static void reject_updates_to_hidden(struct command
> *commands)
> }
> }
>
> +static void execute_commands_non_atomic(struct command *commands,
> + struct shallow_info *si);
> +
> +
> +static void execute_commands_atomic(struct command *commands,
> + struct shallow_info *si)
> +{
> + struct command *cmd;
> + struct strbuf err = STRBUF_INIT;
> + const char *reported_error = "atomic push failure";
> + int checked_connectivity = 1;
> + transaction = ref_transaction_begin(&err);
> + if (!transaction) {
> + rp_error("%s", err.buf);
> + reported_error = "transaction failed to start";
> + goto failure;
> + }
> +
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (cmd->error_string)
> + goto failure;
> +
> + if (cmd->skip_update)
> + goto failure;
These checks are common to the atomic and non-atomic cases. To reduce
code duplication between the two cases, you could factor them out to a
cmd_okay() helper function (or some such name) which checks both
conditions.
> + cmd->error_string = update(cmd, si);
> +
> + if (cmd->error_string)
> + goto failure;
> +
> + if (shallow_update && si->shallow_ref[cmd->index]) {
> + error("BUG: connectivity check has not been run on
> ref %s",
> + cmd->ref_name);
> + checked_connectivity = 0;
> + reported_error = "transaction failed due to internal
> bug";
> + goto failure;
This code is also common to atomic and non-atomic cases and could be
factored out to a helper function, thus further reducing duplication.
The less code duplication between cases, the lower the cognitive load,
making the code easier to understand.
> + }
> + }
> +
> + if (ref_transaction_commit(transaction, &err)) {
> + rp_error("%s", err.buf);
> + reported_error = "atomic transaction failed";
> + goto failure;
> + }
> +
> + ref_transaction_free(transaction);
> + strbuf_release(&err);
> + return;
> +
> +failure:
> + for (cmd = commands; cmd; cmd = cmd->next)
> + if (!cmd->error_string)
> + cmd->error_string = reported_error;
> + ref_transaction_free(transaction);
> + strbuf_release(&err);
> +
> + if (shallow_update && !checked_connectivity)
> + error("BUG: run 'git fsck' for safety.\n"
> + "If there are errors, try to remove "
> + "the reported refs above");
This final conditional is common to both cases but does not even need
to be factored out to a helper function. It could/should have remained
in execute_commands() at its original position, following the call to
execute_commands_atomic() or execute_commands_non_atomic().
> +}
> +
> static void execute_commands(struct command *commands,
> const char *unpacker_error,
> struct shallow_info *si)
> {
> - int checked_connectivity;
> struct command *cmd;
> unsigned char sha1[20];
> struct iterate_data data;
> @@ -1079,7 +1143,20 @@ static void execute_commands(struct command *commands,
> free(head_name_to_free);
> head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
>
> - checked_connectivity = 1;
> + if (use_atomic) {
> + execute_commands_atomic(commands, si);
> + } else {
> + execute_commands_non_atomic(commands, si);
> +
Re: Saving space/network on common repos
btw, just FYI, the scheme you lay out here doesn't actually work as-is. The problem is the config file, which has an entry like: worktree = ../../../mysubmodule This depends on the config file living in ./git/modules/mysubmodule/config. But the proposed scheme moves the config file to mysubmodule/.git/config, and the relative path is broken. I'm not sure what the best solution is; the cleanest one requires a pretty substantial rewrite of git-new-workdir (not that it's such a giant piece of code), and separating out new_workdir from new_gitdir. The smallest one involves having some way to suppress the final 'git checkout -f' (which is the only thing in this script that needs the worktree entry to resolve somewhere) to allow for post-script cleanup. craig On Wed, Dec 17, 2014 at 4:07 PM, Jonathan Nieder wrote: > Craig Silverstein wrote: >> On Wed, Dec 17, 2014 at 2:32 PM, Jonathan Nieder wrote: >>> Craig Silverstein wrote: > Question 4) Is there a practical way to set up submodules so they can use the same object-sharing framework that the main repo does? >>> >>> It's possible to do, but we haven't written a nice UI for it yet. >>> (In other words, you can do this by cloning with --no-recurse-submodules >>> and manually creating the submodule workdir in the appropriate place. >> >> Hmm, let me see if I understand you right -- you're suggesting that >> when cloning my reference repo, I do >> git clone --no-recurse-submodules >> for (path, url) in `parse-.gitmodules`: git clone url path >> # this is psuedocode, obviously :-) >> >> and then when I want to create a new workdir, I do something like: >> cd reference_repo >> git new-workdir /var/workspace1 >> for (path, url) in `parse-.gitmodules`: cd path && git new-workdir >> /var/workspace1/path >> >> ? Basically, I'm going back to the old git way of having each >> submodule have its own .git directory, rather than having it have a >> .git file with a 'gitdir' entry. Am I understanding this right? > > Basically. The initial clone can still use --recurse-submodules. > When you create a new workdir you'd create new workdirs for the > submodules by hand. > > A 'git submodule foreach' command in the initial repo can take > care of the `parse-.gitmodules` part. > > [...] >> Also, it seems to me there's the possibility, with git-newdir, that if >> several of the workspaces try to fetch at the same time they could >> step on each others' toes. Is that a problem? I know there's a push >> lock but I don't believe there's a fetch lock, and I could imagine git >> getting unhappy if two fetches happened in the same repo at the same >> time. > > That's a good question. If concurrent fetches cause trouble then I'd > consider it a bug (it's not too different from multiple concurrent > pushes to the same repository, which is a very common thing to do), > but I haven't looked carefully into whether such bugs exist. > > Hopefully someone else can chime in. > > Thanks, > Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/5] update_unicode.sh: set UNICODE_DIR only once
On 22.12.14 19:02, Junio C Hamano wrote:
> dev+...@drbeat.li writes:
>
>> From: Beat Bolli
>>
>> The value is the same on both uniset invocations, so "Don't Repeat
>> Yourself" applies.
>>
>> Since we're in a subshell already, there's no need to unset UNICODE_DIR
>> at the end.
>
> Strictly speaking, you are not introducing your own subshell to
> prevent the environment from leaking (i.e. you used "{...}" not
> "(...)" in the previous step). The reason you can do this is
> because the generation of UNICODEWIDTH_H file is the last thing in
> the subshell.
I don't introduce a new one, but we're still in the outer subshell that
starts on line 12 "( cd unicode &&".
>
> I'll reword it to "Since this is done as the last command, ..."
>
> Thanks.
>
>>
>> Signed-off-by: Beat Bolli
>> ---
>> update_unicode.sh | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/update_unicode.sh b/update_unicode.sh
>> index c1c876c..bed8916 100755
>> --- a/update_unicode.sh
>> +++ b/update_unicode.sh
>> @@ -27,12 +27,13 @@ fi &&
>> fi &&
>> make
>> ) && {
>> +UNICODE_DIR=. && export UNICODE_DIR &&
>> echo "static const struct interval zero_width[] = {" &&
>> -UNICODE_DIR=. ./uniset/uniset --32 cat:Me,Mn,Cf +
>> U+1160..U+11FF - U+00AD |
>> +./uniset/uniset --32 cat:Me,Mn,Cf + U+1160..U+11FF - U+00AD |
>> grep -v plane &&
>> echo "};" &&
>> echo "static const struct interval double_width[] = {" &&
>> -UNICODE_DIR=. ./uniset/uniset --32 eaw:F,W &&
>> +./uniset/uniset --32 eaw:F,W &&
>> echo "};"
>> } >$UNICODEWIDTH_H
>> )
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
fast-import's notemodify doesn't work the same as git notes
Hi,
There are two major differences between adding notes with fast-import
and git notes, one of which is a serious problem:
- fast-import doesn't want to add notes for non commits, while git notes
does.
- fast-import and git notes have different, conflicting fanouts:
- take e.g. the git repo (there needs to be a lot of commits to start
to see the problem)
- run the following to create notes for every commit:
(echo 'blob';
echo 'mark :1';
echo 'data 0';
echo 'commit refs/notes/foo';
echo 'committer 0 +';
echo 'data 0';
git rev-list --all | awk '{print "N :1", $1}';
echo) | git fast-import
- pick a random commit. I'll pick
bbcefffcea9789e4a1a2023a1c778e2c07db77a7 as it is current master as
of writing. Take the first two characters of that sha1, and look at
the ls-tree:
git ls-tree refs/notes/foo bb/
You'll see a number of blobs.
- Now, remove the note for that commit with git notes:
git notes --ref foo remove bbcefffcea9789e4a1a2023a1c778e2c07db77a7
- ls-tree again, you'll now see a number of trees instead of blobs,
because git notes will have done a fanout. -> git notes does fanouts
for much less items than fast-import does.
- Re-add a note for that commit with fast-import:
git fast-import < 0 +
data 0
from refs/notes/foo^0
N :1 bbcefffcea9789e4a1a2023a1c778e2c07db77a7
EOF
- ls-tree again, and you'll see a number of trees and *one* blob, for
bb/cefffcea9789e4a1a2023a1c778e2c07db77a7
- See the thread starting with 20141126004242.ga13...@glandium.org,
this type of notes branch make things very slow.
- Now, if you take an even bigger repository (as long as there are more
than 65536 commits, that's good ; I guess the linux kernel
qualifies, I've been checking with a mozilla-central clone), and
create exactly 65535 notes with git fast-import, you'll end up with
a 1-level tree (2/38). Add one more note, and the entire tree turns
into a 2-level tree (2/2/36). git notes would only add a level to
the tree containing the added note. git notes's behavior scales
better, because think about what happens on the next fanout with
fast-import... adding one note would need to create millions of trees.
Cheers,
Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use wc instead of awk to count subtrees in t0090-cache-tree
Junio C Hamano wrote:
> Junio C Hamano writes:
>> From: Ben Walton
>>
>> The awk statements previously used in this test weren't compatible
>> with the native versions of awk on Solaris:
>>
>> echo "dir" | /bin/awk -v c=0 '$1 {++c} END {print c}'
>> awk: syntax error near line 1
>> awk: bailing out near line 1
If I were doing it, I'd leave the above four lines out --- they are
describing an unrelated problem. I wonder if we should make the test
harness respect SANE_TOOL_PATH to avoid that kind of problem in the
future.
[...]
> heh, not like that without updating the subject, perhaps like this:
>
> Subject: t0090: tweak awk statement for Solaris /usr/xpg4/bin/awk
With the updated subject,
Reviewed-by: Jonathan Nieder
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use wc instead of awk to count subtrees in t0090-cache-tree
Junio C Hamano writes:
> From: Ben Walton
>
> The awk statements previously used in this test weren't compatible
> with the native versions of awk on Solaris:
>
> echo "dir" | /bin/awk -v c=0 '$1 {++c} END {print c}'
> awk: syntax error near line 1
> awk: bailing out near line 1
>
> echo "dir" | /usr/xpg4/bin/awk -v c=0 '$1 {++c} END {print c}'
> 0
>
> And with GNU awk for comparison:
>
> echo "dir" | /opt/csw/gnu/awk -v c=0 '$1 {++c} END {print c}'
> 1
>
> Work it around by using $1 != "" to state more explicitly that we
> are skipping empty lines.
By the way, I was hoping (eh, what kind of hope is that???) that $1
alone is not a kosher POSIX way but a GNUism, but that does not seem
to be the case. POSIX has this [*1*]
When an expression is used in a Boolean context, if it has a
numeric value, a value of zero shall be treated as false and any
other value shall be treated as true. Otherwise, a string value
of the null string shall be treated as false and any other value
shall be treated as true. A Boolean context shall be one of the
following:
and among the "Boolean context" listed is:
* An expression used as a pattern (as in Overall Program Structure)
So the example with /usr/xpg4/bin/awk does not seem to be a
behaviour from a conformant implementationd, and it seems to be
correct to label this as "work it around by ..." (not "avoid using
GNUism").
We learn new things every day (not that I really wanted to learn
glitches in various implementations of awk) ;-).
Thanks.
[Reference]
*1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/awk.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use wc instead of awk to count subtrees in t0090-cache-tree
Junio C Hamano writes:
> From: Ben Walton
>
> The awk statements previously used in this test weren't compatible
> with the native versions of awk on Solaris:
>
> echo "dir" | /bin/awk -v c=0 '$1 {++c} END {print c}'
> awk: syntax error near line 1
> awk: bailing out near line 1
>
> echo "dir" | /usr/xpg4/bin/awk -v c=0 '$1 {++c} END {print c}'
> 0
>
> And with GNU awk for comparison:
>
> echo "dir" | /opt/csw/gnu/awk -v c=0 '$1 {++c} END {print c}'
> 1
>
> Work it around by using $1 != "" to state more explicitly that we
> are skipping empty lines.
>
> Helped-by: Jonathan Nieder
> Signed-off-by: Ben Walton
> Signed-off-by: Junio C Hamano
> ---
>
> * Then let's queue this, perhaps?
heh, not like that without updating the subject, perhaps like this:
Subject: t0090: tweak awk statement for Solaris /usr/xpg4/bin/awk
Sorry for the noise.
> t/t0090-cache-tree.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 067f4c6..601d02d 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -22,7 +22,7 @@ generate_expected_cache_tree_rec () {
> # ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
> # We want to count only foo because it's the only direct child
> subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq) &&
> - subtree_count=$(echo "$subtrees"|awk -v c=0 '$1 {++c} END {print c}') &&
> + subtree_count=$(echo "$subtrees"|awk -v c=0 '$1 != "" {++c} END {print
> c}') &&
> entries=$(git ls-files|wc -l) &&
> printf "SHA $dir (%d entries, %d subtrees)\n" "$entries"
> "$subtree_count" &&
> for subtree in $subtrees
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use wc instead of awk to count subtrees in t0090-cache-tree
From: Ben Walton
The awk statements previously used in this test weren't compatible
with the native versions of awk on Solaris:
echo "dir" | /bin/awk -v c=0 '$1 {++c} END {print c}'
awk: syntax error near line 1
awk: bailing out near line 1
echo "dir" | /usr/xpg4/bin/awk -v c=0 '$1 {++c} END {print c}'
0
And with GNU awk for comparison:
echo "dir" | /opt/csw/gnu/awk -v c=0 '$1 {++c} END {print c}'
1
Work it around by using $1 != "" to state more explicitly that we
are skipping empty lines.
Helped-by: Jonathan Nieder
Signed-off-by: Ben Walton
Signed-off-by: Junio C Hamano
---
* Then let's queue this, perhaps?
t/t0090-cache-tree.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 067f4c6..601d02d 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -22,7 +22,7 @@ generate_expected_cache_tree_rec () {
# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
# We want to count only foo because it's the only direct child
subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq) &&
- subtree_count=$(echo "$subtrees"|awk -v c=0 '$1 {++c} END {print c}') &&
+ subtree_count=$(echo "$subtrees"|awk -v c=0 '$1 != "" {++c} END {print
c}') &&
entries=$(git ls-files|wc -l) &&
printf "SHA $dir (%d entries, %d subtrees)\n" "$entries"
"$subtree_count" &&
for subtree in $subtrees
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Dec 2014, #04; Mon, 22)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. Quite a few topics have been merged to 'master' as the third batch for this cycle, on top of the recent "case sensitive .Git" fix that has been publicized very widely. The next release which is expected to be a small one is taking shape. 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"] * cc/interpret-trailers-more (2014-11-10) 4 commits (merged to 'next' on 2014-12-15 at 77f6c6a) + trailer: add test with an old style conflict block + trailer: reuse ignore_non_trailer() to ignore conflict lines + commit: make ignore_non_trailer() non static + Merge branch 'jc/conflict-hint' into cc/interpret-trailers-more (this branch uses jc/conflict-hint.) "git interpret-trailers" learned to properly handle the "Conflicts:" block at the end. * ch/new-gpg-drops-rfc-1991 (2014-12-12) 4 commits (merged to 'next' on 2014-12-15 at 32d7d50) + tests: squelch noise from GPG machinery set-up + tests: replace binary GPG keyrings with ASCII-armored keys + tests: skip RFC1991 tests for gnupg 2.1 + tests: create gpg homedir on the fly Recent GPG changes the keyring format and drops support for RFC1991 formatted signatures, breaking our existing tests. * dm/compat-s-ifmt-for-zos (2014-12-04) 1 commit (merged to 'next' on 2014-12-15 at 0eb2fe6) + compat: convert modes to use portable file type values Long overdue departure from the assumption that S_IFMT is shared by everybody made in 2005. * dw/shell-basename-dashdash-before-stripping-leading-dash-from-login (2014-11-25) 1 commit (merged to 'next' on 2014-12-15 at 42937b7) + git-sh-setup.sh: use dashdash with basename call * jc/conflict-hint (2014-10-28) 4 commits (merged to 'next' on 2014-12-15 at b72475f) + merge & sequencer: turn "Conflicts:" hint into a comment + builtin/commit.c: extract ignore_non_trailer() helper function + merge & sequencer: unify codepaths that write "Conflicts:" hint + builtin/merge.c: drop a parameter that is never used (this branch is used by cc/interpret-trailers-more.) Unlike all the other hints given in the commit log editor, the list of conflicted paths were appended at the end without commented out. * jc/exec-cmd-system-path-leak-fix (2014-11-30) 1 commit (merged to 'next' on 2014-12-15 at f926ee5) + system_path(): always return free'able memory to the caller The function sometimes returned a non-freeable memory and some other times returned a piece of memory that must be freed. * jc/hook-cleanup (2014-12-01) 1 commit (merged to 'next' on 2014-12-15 at f5759d0) + run-command.c: retire unused run_hook_with_custom_index() Remove unused code. * jc/refer-to-t-readme-from-submitting-patches (2014-11-24) 2 commits (merged to 'next' on 2014-12-15 at 0e88699) + t/README: justify why "! grep foo" is sufficient + SubmittingPatches: refer to t/README for tests * jg/prompt-localize-temporary (2014-12-12) 1 commit (merged to 'next' on 2014-12-15 at bb9cac9) + git-prompt.sh: make $f local to __git_eread() "git-prompt" (in contrib/) used a variable from the global scope, possibly contaminating end-user's namespace. * jk/always-allow-large-packets (2014-12-10) 1 commit (merged to 'next' on 2014-12-15 at c3fb2c8) + pkt-line: allow writing of LARGE_PACKET_MAX buffers "git push" and "git fetch" did not communicate an overlong refname correctly. * jk/colors (2014-12-09) 6 commits (merged to 'next' on 2014-12-15 at 20b045f) + parse_color: drop COLOR_BACKGROUND macro + diff-highlight: allow configurable colors + parse_color: recognize "no$foo" to clear the $foo attribute + parse_color: support 24-bit RGB values + parse_color: refactor color storage + Merge branch 'jn/parse-config-slot' into jk/colors "diff-highlight" filter (in contrib/) allows its color output to be customized via configuration variables. * jk/commit-date-approxidate (2014-12-11) 2 commits (merged to 'next' on 2014-12-15 at 047530e) + commit: always populate GIT_AUTHOR_* variables + commit: loosen ident checks when generating template Recent update to "git commit" broke amending an existing commit with bogus author/committer lines without a valid e-mail address. * jk/credential-quit (2014-12-04) 2 commits (merged to 'next' on 2014-12-15 at 4cfd999) + prompt: respect GIT_TERMINAL_PROMPT to disable terminal prompts + credential: let helpers tell us to quit Credential helpers are asked in turn until one of them give positive response, which is cumbersome to turn off when you need to run Git in an automated setting. The credential helper interface learned to allow a helper to say "stop, don't ask other helpers." Also GIT_TERMINAL_PROMPT environment ca
Re: [PATCH 04/18] Offer a function to demote fsck errors to warnings
Johannes Schindelin writes: > For example, try to spot the error here: > > ... > F(ALMOST_HAPPY, INFO) \ > F(CANNOT_RECOVER, ERROR) \ > F(COFFEE_IS_EMPTY, WARN) \ > F(JUST_BEING_CHATTY, INFO) \ > F(LIFE_IS_GOOD, INFO) \ > F(MISSING_SOMETHING_VITAL, FATAL_ERROR) \ > F(NEED_TO_SLEEP, WARN) \ > F(SOMETHING_WENT_WRONG, ERROR) \ > ... But that is not what is being suggested at all. I already said that FIRST_SOMETHING is fine as a measure to initialize, didn't I? I am only saying that if you have a place to store customized level, you should initialize that part with default levels and always look it up from that place at runtime. It is perfectly fine for the initialization step to take advantage of the ordering and FIRST_SOMETHING constants. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/18] fsck: support demoting errors to warnings
Johannes Schindelin writes: > Hi Junio, > > On Mon, 22 Dec 2014, Junio C Hamano wrote: > >> Johannes Schindelin writes: >> >> > Of course you can say that! ;-) The problem these ugly messages try to >> > solve is to give the user a hint which setting to change if they want to >> > override the default behavior, though... >> >> Ahh, OK, then dashed form would not work as a configuration variable >> names, so missingTaggerEntry would be the only usable option. > > Except that cunning me has made it so that both missing-tagger-entry *and* > missingTaggerEntry work... Then the missing-tagger-entry side needs to be dropped. The naming does not conform to the way how we name our officially supported configuration variables. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] send-pack.c: add --atomic command line argument
On Fri, Dec 19, 2014 at 2:38 PM, Stefan Beller wrote:
> From: Ronnie Sahlberg
>
> This adds support to send-pack to negotiate and use atomic pushes
> iff the server supports it. Atomic pushes are activated by a new command
> line flag --atomic.
>
> Signed-off-by: Ronnie Sahlberg
> Signed-off-by: Stefan Beller
> ---
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index b564a77..b961e5a 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -282,6 +282,30 @@ free_return:
> return update_seen;
> }
>
> +
> +static int atomic_push_failure(struct send_pack_args *args,
> + struct ref *remote_refs,
> + struct ref *failing_ref)
> +{
> + struct ref *ref;
> + /* Mark other refs as failed */
> + for (ref = remote_refs; ref; ref = ref->next) {
> + if (!ref->peer_ref && !args->send_mirror)
> + continue;
> +
> + switch (ref->status) {
> + case REF_STATUS_EXPECTING_REPORT:
> + ref->status = REF_STATUS_ATOMIC_PUSH_FAILED;
> + continue;
> + default:
> + ; /* do nothing */
> + }
> + }
> + error("atomic push failed for ref %s. status: %d\n",
> + failing_ref->name, failing_ref->status);
> + return -1;
Not itself worth a re-send, but if you do re-send for some other reason...
return error(...);
would be more idiomatic (as mentioned in the previous review).
> +}
> +
> int send_pack(struct send_pack_args *args,
> int fd[], struct child_process *conn,
> struct ref *remote_refs,
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/18] fsck: support demoting errors to warnings
Hi Junio, On Mon, 22 Dec 2014, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Of course you can say that! ;-) The problem these ugly messages try to > > solve is to give the user a hint which setting to change if they want to > > override the default behavior, though... > > Ahh, OK, then dashed form would not work as a configuration variable > names, so missingTaggerEntry would be the only usable option. Except that cunning me has made it so that both missing-tagger-entry *and* missingTaggerEntry work... Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/18] Offer a function to demote fsck errors to warnings
Hi Junio,
On Mon, 22 Dec 2014, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > On Mon, 22 Dec 2014, Junio C Hamano wrote:
> >
> >> Johannes Schindelin writes:
> >>
> >> >> In other words, at some point wouldn't we be better off with
> >> >> something like this
> >> >>
> >> >> struct {
> >> >> enum id;
> >> >> const char *id_string;
> >> >> enum error_level { FSCK_PASS, FSCK_WARN, FSCK_ERROR };
> >> >> } possible_fsck_errors[];
> >> >
> >> > I considered that, and Michael Haggerty also suggested that in a private
> >> > mail. However, I find that there is a clear hierarchy in the default
> >> > messages: fatal errors, errors, warnings and infos.
> >>
> >> I am glad I am not alone ;-)
> >> ...
> > Oh, but please understand that this hierarchy only applies to the default
> > settings. All of these settings can be overridden individually – and the
> > first override will initialize a full array with the default settings.
>
> But that means that the runtime needs to switch between two code
> with and without override, no?
>
> > if (options->strict_mode)
> > return options->strict_mode[msg_id];
>
> In other words, I think this is misleading and unnecessary
> optimization for the "full array" allocation. A code that uses an
> array of a struct like the above that Michael and I independently
> suggested would initialize once with or without an override and then
> at the runtime there is no "if the array is there use it"
> conditional.
>
> I do not know why Michael suggested the same thing, but the reason
> why I prefer that arrangement is because I think it would be easier
> to read and maintain.
Well, I disagree that it would be easier to maintain, because it appears
to me that the clear hierarchy keeps things simple. For example if some
clearly fatal error is clustered with non-fatal ones due to alphabetical
ordering, it is much harder to spot when it is marked as a demoteable
error by mistake.
For example, try to spot the error here:
...
F(ALMOST_HAPPY, INFO) \
F(CANNOT_RECOVER, ERROR) \
F(COFFEE_IS_EMPTY, WARN) \
F(JUST_BEING_CHATTY, INFO) \
F(LIFE_IS_GOOD, INFO) \
F(MISSING_SOMETHING_VITAL, FATAL_ERROR) \
F(NEED_TO_SLEEP, WARN) \
F(SOMETHING_WENT_WRONG, ERROR) \
...
Personally, I find it very, very hard to spot that CANNOT_RECOVER is
marked as a mere ERROR instead of a FATAL_ERROR. Even if it is nicely
alphabetically ordered.
I will sleep over this, though. Maybe I can come up with a solution that
makes all three of us happy.
Ciao,
Dscho
Re: [PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push
On Fri, Dec 19, 2014 at 2:38 PM, Stefan Beller wrote: > From: Ronnie Sahlberg > > This adds support to the protocol between send-pack and receive-pack to > * allow receive-pack to inform the client that it has atomic push capability > * allow send-pack to request atomic push back. > > There is currently no setting in send-pack to actually request that atomic > pushes are to be used yet. This only adds protocol capability not ability > for the user to activate it. > > Signed-off-by: Ronnie Sahlberg > Signed-off-by: Stefan Beller > --- > diff --git a/Documentation/technical/protocol-capabilities.txt > b/Documentation/technical/protocol-capabilities.txt > index 6d5424c..4f8a7bf 100644 > --- a/Documentation/technical/protocol-capabilities.txt > +++ b/Documentation/technical/protocol-capabilities.txt > @@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress > server-side progress > reporting if the local progress reporting is also being suppressed > (e.g., via `push -q`, or if stderr does not go to a tty). > > +atomic > +-- > + > +If the server sends the 'atomic' capability it is capable of accepting > +atomic pushes. If the pushing client requests this capability, the server > +will update the refs in one atomic transaction. Either all refs are Not itself worth a re-send, but if you re-send for some other reason... "one atomic" still smacks of redundancy; "an atomic" sounds better. > +updated or none. > + > allow-tip-sha1-in-want > -- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/18] fsck: support demoting errors to warnings
Johannes Schindelin writes: > Of course you can say that! ;-) The problem these ugly messages try to > solve is to give the user a hint which setting to change if they want to > override the default behavior, though... Ahh, OK, then dashed form would not work as a configuration variable names, so missingTaggerEntry would be the only usable option. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/18] fsck: support demoting errors to warnings
Hi Junio, On Mon, 22 Dec 2014, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Or do you want the error messages to also use camelCased IDs, i.e. > > > > warning in tag $tag: missingTaggerEntry: invalid format ... > > > > instead of > > > > warning in tag $tag: missing-tagger-entry: invalid format ... > > > > ? It is easy to do, but looks slightly uglier to this developer's eyes... > > Do you really need to know what I think? Well, but yes ;-) > Can I say "The latter is probably slightly better, but both look ugly to > me"? Of course you can say that! ;-) The problem these ugly messages try to solve is to give the user a hint which setting to change if they want to override the default behavior, though... > If we want a human readable message > > "warning: tag object lacks tagger field '$tag'" > > would be my preference. > > But I personally think it may not be necessary to give a pretty > message that later can go through l10n here. If we take that > position, it is just a machine-readble error token, so I'd say > whichever way you find more readable is OK. Okay, I will leave it as-is, then. > >> Should these be tied to receive-pack ones in any way? E.g. if you > >> set fsck.missingEmail to ignore, you do not have to do the same for > >> receive and accept a push with the specific error turned off? > >> > >> Not a rhetorical question. I can see it argued both ways. The > >> justification to defend the position of not tying these two I would > >> have is so that I can be more strict to newer breakages (i.e. not > >> accepting a push that introduce a new breakage by not ignoring with > >> receive.fsck.*) while allowing breakages that are already present. > >> The justification for the opposite position is to make it more > >> convenient to write a consistent policy. Whichever way is chosen, > >> we would want to see the reason left in the log message so that > >> people do not have to wonder what the original motivation was when > >> they decide if it is a good idea to change this part of the code. > > > > Hmm. I really tried very hard to separate the fsck.* from the receive.* > > settings because the two code paths already behave differently:... > > > > If you agree, I would rephrase this line of argument and add it to the > > commit message. Do you agree? > > Yeah, that reasoning sounds sensible. I added this paragraph: In the same spirit that `git receive-pack`'s usage of the fsck machinery differs from `git fsck`'s – some of the non-fatal warnings in `git fsck` are fatal with `git receive-pack` when receive.fsckObjects = true, for example – we strictly separate the fsck.* from the receive.fsck.* settings. Ciao, Dscho
Re: [PATCH 04/18] Offer a function to demote fsck errors to warnings
Johannes Schindelin writes:
> Hi Junio,
>
> On Mon, 22 Dec 2014, Junio C Hamano wrote:
>
>> Johannes Schindelin writes:
>>
>> >> In other words, at some point wouldn't we be better off with
>> >> something like this
>> >>
>> >> struct {
>> >> enum id;
>> >> const char *id_string;
>> >> enum error_level { FSCK_PASS, FSCK_WARN, FSCK_ERROR };
>> >> } possible_fsck_errors[];
>> >
>> > I considered that, and Michael Haggerty also suggested that in a private
>> > mail. However, I find that there is a clear hierarchy in the default
>> > messages: fatal errors, errors, warnings and infos.
>>
>> I am glad I am not alone ;-)
>> ...
> Oh, but please understand that this hierarchy only applies to the default
> settings. All of these settings can be overridden individually – and the
> first override will initialize a full array with the default settings.
But that means that the runtime needs to switch between two code
with and without override, no?
> if (options->strict_mode)
> return options->strict_mode[msg_id];
In other words, I think this is misleading and unnecessary
optimization for the "full array" allocation. A code that uses an
array of a struct like the above that Michael and I independently
suggested would initialize once with or without an override and then
at the runtime there is no "if the array is there use it"
conditional.
I do not know why Michael suggested the same thing, but the reason
why I prefer that arrangement is because I think it would be easier
to read and maintain.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/18] fsck: allow upgrading fsck warnings to errors
Hi Junio,
On Mon, 22 Dec 2014, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > On Wed, 10 Dec 2014, Junio C Hamano wrote:
> >
> >> Johannes Schindelin writes:
> >>
> >> > The 'invalid tag name' and 'missing tagger entry' warnings can now be
> >> > upgraded to errors by setting receive.fsck.invalid-tag-name and
> >> > receive.fsck.missing-tagger-entry to 'error'.
> >>
> >> Hmm, why can't all warnings promotable to errors, or are the above
> >> two mentioned only as examples?
> >
> > Those were the only ones that were always shown as warnings but never
> > treated as errors.
>
> Sorry but I don't quite understand this comment; I suspect the root
> cause might be that we have different mental models on these
> tweakable error severities.
>
> Because I come from the school "To these N kinds of events you can
> independently assign different (i.e. info, warn, error) outcomes",
> moving the FIRST_{INFO,WARNING,...} position in the array would only
> affect what happens by default, never hindering the user's ability
> to tweak (in other words, there is no linkage between "now you can
> tweak" and the order of events in the list, the latter of which only
> would affect what the default severity of each event is).
We agree on this mental model.
The only problem this patch tries to fix is that the warnings about a
missing tagger and about invalid tag names were never leading to an error.
They were purely printed, but then ignored. So what this patch does is to
add "if (err) return err;" handling for those two warnings.
As a consequence, the ordering of message IDs needs to be fixed because
the non-fatal warnings were ordered alphabetically before, but now the
non-fatal warnings are extracted so that we can give them the appropriate
FSCK_WARN by defauly – even in the git-receive-pack case.
In other words, the value assigned to those two warnings was completely
ignored before, which was the reason why it did not matter that we
assigned them to report FSCK_ERRORs in the git-receive-pack case before:
they were still only printed out and never stopped any tag from entering
the host's repository.
I could change the ordering in the patch that introduces the message IDs,
of course, but it would be even more puzzling if those two messages, of
all, were not ordered alphabetically with the others.
Ciao,
Dscho
Re: [PATCH 16/18] fsck: support demoting errors to warnings
Johannes Schindelin writes: > Or do you want the error messages to also use camelCased IDs, i.e. > > warning in tag $tag: missingTaggerEntry: invalid format ... > > instead of > > warning in tag $tag: missing-tagger-entry: invalid format ... > > ? It is easy to do, but looks slightly uglier to this developer's eyes... Do you really need to know what I think? Can I say "The latter is probably slightly better, but both look ugly to me"? If we want a human readable message "warning: tag object lacks tagger field '$tag'" would be my preference. But I personally think it may not be necessary to give a pretty message that later can go through l10n here. If we take that position, it is just a machine-readble error token, so I'd say whichever way you find more readable is OK. >> Should these be tied to receive-pack ones in any way? E.g. if you >> set fsck.missingEmail to ignore, you do not have to do the same for >> receive and accept a push with the specific error turned off? >> >> Not a rhetorical question. I can see it argued both ways. The >> justification to defend the position of not tying these two I would >> have is so that I can be more strict to newer breakages (i.e. not >> accepting a push that introduce a new breakage by not ignoring with >> receive.fsck.*) while allowing breakages that are already present. >> The justification for the opposite position is to make it more >> convenient to write a consistent policy. Whichever way is chosen, >> we would want to see the reason left in the log message so that >> people do not have to wonder what the original motivation was when >> they decide if it is a good idea to change this part of the code. > > Hmm. I really tried very hard to separate the fsck.* from the receive.* > settings because the two code paths already behave differently:... > > If you agree, I would rephrase this line of argument and add it to the > commit message. Do you agree? Yeah, that reasoning sounds sensible. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/18] Offer a function to demote fsck errors to warnings
Hi Junio,
On Mon, 22 Dec 2014, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> >> In other words, at some point wouldn't we be better off with
> >> something like this
> >>
> >>struct {
> >>enum id;
> >> const char *id_string;
> >> enum error_level { FSCK_PASS, FSCK_WARN, FSCK_ERROR };
> >>} possible_fsck_errors[];
> >
> > I considered that, and Michael Haggerty also suggested that in a private
> > mail. However, I find that there is a clear hierarchy in the default
> > messages: fatal errors, errors, warnings and infos.
>
> I am glad I am not alone ;-)
>
> These classes are ordered from more severe to less, but I do not
> think it makes much sense to force the default view of "if you
> customize to demote a questionable Q that is classified as an error
> by default as an warning, you must demote all the other ones that we
> deem less serious than Q, which come earlier (or later---I do not
> remember which) in our predefined list". So in that sense, I do not
> consider that various kinds of questionables fsck can detect are
> hierarchical at all.
Oh, but please understand that this hierarchy only applies to the default
settings. All of these settings can be overridden individually – and the
first override will initialize a full array with the default settings.
So the order really only plays a role for the defaults, no more.
> I do agree that it makes it easier to code the initialization of
> such an array to have "up to this point we assign the level 'fatal
> error' by default" constants. Then the initialization can become
>
> for (i = 0; i < FIRST_WARN; i++)
> possible_fsck_errors[i].error_level = FSCK_INFO;
> while (i < FIRST_ERROR)
> possible_fsck_errors[i++].error_level = FSCK_WARN;
> while (i < ARRAY_SIZE(possible_fsck_errors))
> possible_fsck_errors[i++].error_level = FSCK_ERROR;
>
> or something. So I am not against the FIRST_WARNING constant at
> all, but I find it very questionable in a fully customizable system
> to use such a constant anywhere other than the initialization time.
This is indeed the case. The code we are discussing comes after the
if (options->strict_mode)
return options->strict_mode[msg_id];
In other words, once the overrides are in place, the default settings are
skipped entirely.
Ciao,
Dscho
Re: [PATCH] Use wc instead of awk to count subtrees in t0090-cache-tree
Jonathan Nieder writes:
> Junio C Hamano wrote:
>> Ben Walton writes:
>
>>> echo "dir" | /usr/xpg4/bin/awk -v c=0 '$1 {++c} END {print c}'
>>> 0
>>>
>>> And with GNU awk for comparison:
>>> echo "dir" | /opt/csw/gnu/awk -v c=0 '$1 {++c} END {print c}'
>>> 1
>>>
>>> Instead of modifying the awk code to work, use wc -w instead as that
>>> is both adequate and simpler.
>>
>> Hmm, why "wc -w" not "wc -l", though? Is somebody squashing a
>> one-elem-per-line output from ls-files onto a single line?
>
> The old code was trying to skip empty lines.
Ahh, I misread the original.
Your suggestion to explicitly check $1 != "" makes sense to me now.
To be blunt, I do not have much sympathy to those who insist using
/usr/bin versions of various tools on Solaris that are overriden by
xpg variants, but it is somewhat disturbing that the one from xpg4
does not work.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/18] fsck: support demoting errors to warnings
Hi Junio, On Wed, 10 Dec 2014, Junio C Hamano wrote: > Johannes Schindelin writes: > > > We already have support in `git receive-pack` to deal with some legacy > > repositories which have non-fatal issues. > > > > Let's make `git fsck` itself useful with such repositories, too, by > > allowing users to ignore known issues, or at least demote those issues > > to mere warnings. > > > > Example: `git -c fsck.missing-email=ignore fsck` would hide problems with > > missing emails in author, committer and tagger lines. > > Hopefully I do not have to repeat myself, but please do not have > punctuations in the first and the last level of configuration variable > names, i.e. fsck.missingEmail, not mising-email. I vetted the complete patch series and think I caught them all. Or do you want the error messages to also use camelCased IDs, i.e. warning in tag $tag: missingTaggerEntry: invalid format ... instead of warning in tag $tag: missing-tagger-entry: invalid format ... ? It is easy to do, but looks slightly uglier to this developer's eyes... > Should these be tied to receive-pack ones in any way? E.g. if you > set fsck.missingEmail to ignore, you do not have to do the same for > receive and accept a push with the specific error turned off? > > Not a rhetorical question. I can see it argued both ways. The > justification to defend the position of not tying these two I would > have is so that I can be more strict to newer breakages (i.e. not > accepting a push that introduce a new breakage by not ignoring with > receive.fsck.*) while allowing breakages that are already present. > The justification for the opposite position is to make it more > convenient to write a consistent policy. Whichever way is chosen, > we would want to see the reason left in the log message so that > people do not have to wonder what the original motivation was when > they decide if it is a good idea to change this part of the code. Hmm. I really tried very hard to separate the fsck.* from the receive.* settings because the two code paths already behave differently: many warnings reported (and ignored) by fsck are fatal errors when pushing with receive.fsckObjects=true. My understanding was that these differences are deliberate, and my interpretation was that the fsck and the receive settings were considered to be fundamentally different, even if the same fsck machinery performs the validation. If you agree, I would rephrase this line of argument and add it to the commit message. Do you agree? Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 18/18] git receive-pack: support excluding objects from fsck'ing
Hi Junio,
On Wed, 10 Dec 2014, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > The optional new config option `receive.fsck.skip-list` specifies the path
> > to a file listing the names, i.e. SHA-1s, one per line, of objects that
> > are to be ignored by `git receive-pack` when `receive.fsckObjects = true`.
> >
> > This is extremely handy in case of legacy repositories where it would
> > cause more pain to change incorrect objects than to live with them
> > (e.g. a duplicate 'author' line in an early commit object).
> >
> > The intended use case is for server administrators to inspect objects
> > that are reported by `git push` as being too problematic to enter the
> > repository, and to add the objects' SHA-1 to a (preferably sorted) file
> > when the objects are legitimate, i.e. when it is determined that those
> > problematic objects should be allowed to enter the server.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > builtin/receive-pack.c | 9 +++
> > fsck.c | 59
> > +++--
> > fsck.h | 2 ++
> > t/t5504-fetch-receive-strict.sh | 12 +
> > 4 files changed, 80 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index 111e514..5169f1f 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -110,6 +110,15 @@ static int receive_pack_config(const char *var, const
> > char *value, void *cb)
> > return 0;
> > }
> >
> > + if (starts_with(var, "receive.fsck.skip-list")) {
>
> s/skip-list/skiplist/;
>
> > + const char *path = is_absolute_path(value) ?
> > + value : git_path("%s", value);
> > + if (fsck_strict_mode.len)
> > + strbuf_addch(&fsck_strict_mode, ',');
> > + strbuf_addf(&fsck_strict_mode, "skip-list=%s", path);
> > + return 0;
> > + }
> > +
> > if (starts_with(var, "receive.fsck.")) {
> > if (fsck_strict_mode.len)
> > strbuf_addch(&fsck_strict_mode, ',');
> > diff --git a/fsck.c b/fsck.c
> > index 154f361..00693f2 100644
> > --- a/fsck.c
> > +++ b/fsck.c
> > @@ -7,6 +7,7 @@
> > #include "tag.h"
> > #include "fsck.h"
> > #include "refs.h"
> > +#include "sha1-array.h"
> >
> > #define FOREACH_MSG_ID(FUNC) \
> > /* fatal errors */ \
> > @@ -56,7 +57,9 @@
> > FUNC(ZERO_PADDED_FILEMODE) \
> > /* infos (reported as warnings, but ignored by default) */ \
> > FUNC(INVALID_TAG_NAME) \
> > - FUNC(MISSING_TAGGER_ENTRY)
> > + FUNC(MISSING_TAGGER_ENTRY) \
> > + /* special value */ \
> > + FUNC(SKIP_LIST)
>
> This feels like a kludge to me without comment on what "special
> value" means. Does it mean "this object has an error (which by
> default is ignored) of being on the skip list?" Should we be able
> to optionally warn an object on the skip-list exists with the same
> mechansim the rest of the series uses to tweak the error level?
I addressed both concerns – I hope... ;-)
Ciao,
Dscho
Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt
Junio C Hamano wrote: > > Yes. I wouldn't have spent 20 minutes experimenting with various > hypothetical use cases if the above were there in the first place. Sorry for wasting your time, and thanks for reviewing the patch. (I am so used to having $? in my prompt it took me ages to find and explain the problem too... Sigh!) Tony. -- f.anthony.n.finchhttp://dotat.at/ Trafalgar: Easterly 6 to gale 8 far southeast, otherwise northeasterly veering southeasterly 4 or 5. Slight or moderate, occasionally rough at first in south. Occasional drizzle. Good, occasionally moderate. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/18] fsck: allow upgrading fsck warnings to errors
Johannes Schindelin writes:
> Hi Junio,
>
> On Wed, 10 Dec 2014, Junio C Hamano wrote:
>
>> Johannes Schindelin writes:
>>
>> > The 'invalid tag name' and 'missing tagger entry' warnings can now be
>> > upgraded to errors by setting receive.fsck.invalid-tag-name and
>> > receive.fsck.missing-tagger-entry to 'error'.
>>
>> Hmm, why can't all warnings promotable to errors, or are the above
>> two mentioned only as examples?
>
> Those were the only ones that were always shown as warnings but never
> treated as errors.
Sorry but I don't quite understand this comment; I suspect the root
cause might be that we have different mental models on these
tweakable error severities.
Because I come from the school "To these N kinds of events you can
independently assign different (i.e. info, warn, error) outcomes",
moving the FIRST_{INFO,WARNING,...} position in the array would only
affect what happens by default, never hindering the user's ability
to tweak (in other words, there is no linkage between "now you can
tweak" and the order of events in the list, the latter of which only
would affect what the default severity of each event is).
It appears that your design is from a different mental model and the
order and position in that list has more significance than what the
default severity of each event is but how much the severity can be
tweaked, or something, which I somehow find incomprehensible.
Puzzled...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use wc instead of awk to count subtrees in t0090-cache-tree
Junio C Hamano wrote:
> Ben Walton writes:
>> echo "dir" | /usr/xpg4/bin/awk -v c=0 '$1 {++c} END {print c}'
>> 0
>>
>> And with GNU awk for comparison:
>> echo "dir" | /opt/csw/gnu/awk -v c=0 '$1 {++c} END {print c}'
>> 1
>>
>> Instead of modifying the awk code to work, use wc -w instead as that
>> is both adequate and simpler.
>
> Hmm, why "wc -w" not "wc -l", though? Is somebody squashing a
> one-elem-per-line output from ls-files onto a single line?
The old code was trying to skip empty lines.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/18] fsck: allow upgrading fsck warnings to errors
Hi Junio, On Wed, 10 Dec 2014, Junio C Hamano wrote: > Johannes Schindelin writes: > > > The 'invalid tag name' and 'missing tagger entry' warnings can now be > > upgraded to errors by setting receive.fsck.invalid-tag-name and > > receive.fsck.missing-tagger-entry to 'error'. > > Hmm, why can't all warnings promotable to errors, or are the above > two mentioned only as examples? Those were the only ones that were always shown as warnings but never treated as errors. There is a third one coming, as part of the patches that will let fsck warn about NTFS-incompatible file names, but I want to get this patch series integrated into git.git first. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use wc instead of awk to count subtrees in t0090-cache-tree
Ben Walton wrote:
> echo "dir" | /usr/xpg4/bin/awk -v c=0 '$1 {++c} END {print c}'
> 0
Thanks. Weird. Does
awk -v c=0 '$1 != "" {++c} END {print c}'
work better?
[...]
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -22,7 +22,7 @@ generate_expected_cache_tree_rec () {
> # ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
> # We want to count only foo because it's the only direct child
> subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq) &&
> - subtree_count=$(echo "$subtrees"|awk -v c=0 '$1 {++c} END {print c}') &&
> + subtree_count=$(echo "$subtrees"|wc -w) &&
> entries=$(git ls-files|wc -l) &&
> printf "SHA $dir (%d entries, %d subtrees)\n" "$entries"
> "$subtree_count" &&
Some implementations of wc add a trailing space, causing
printf: 1 : invalid number
Using
printf "SHA $dir (%d entries, %d subtrees)\n" "$entries" $subtree_count
&&
(with no quotes around $subtree_count) would avoid trouble, though
that's a little subtle.
Hope that helps,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/18] Offer a function to demote fsck errors to warnings
Johannes Schindelin writes:
>> In other words, at some point wouldn't we be better off with
>> something like this
>>
>> struct {
>> enum id;
>> const char *id_string;
>> enum error_level { FSCK_PASS, FSCK_WARN, FSCK_ERROR };
>> } possible_fsck_errors[];
>
> I considered that, and Michael Haggerty also suggested that in a private
> mail. However, I find that there is a clear hierarchy in the default
> messages: fatal errors, errors, warnings and infos.
I am glad I am not alone ;-)
These classes are ordered from more severe to less, but I do not
think it makes much sense to force the default view of "if you
customize to demote a questionable Q that is classified as an error
by default as an warning, you must demote all the other ones that we
deem less serious than Q, which come earlier (or later---I do not
remember which) in our predefined list". So in that sense, I do not
consider that various kinds of questionables fsck can detect are
hierarchical at all.
I do agree that it makes it easier to code the initialization of
such an array to have "up to this point we assign the level 'fatal
error' by default" constants. Then the initialization can become
for (i = 0; i < FIRST_WARN; i++)
possible_fsck_errors[i].error_level = FSCK_INFO;
while (i < FIRST_ERROR)
possible_fsck_errors[i++].error_level = FSCK_WARN;
while (i < ARRAY_SIZE(possible_fsck_errors))
possible_fsck_errors[i++].error_level = FSCK_ERROR;
or something. So I am not against the FIRST_WARNING constant at
all, but I find it very questionable in a fully customizable system
to use such a constant anywhere other than the initialization time.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/18] Disallow demoting grave fsck errors to warnings
Hi Junio, On Wed, 10 Dec 2014, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Some kinds of errors are intrinsically unrecoverable (e.g. errors while > > uncompressing objects). It does not make sense to allow demoting them to > > mere warnings. > > Makes sense, but the patch makes it look as if this is an "oops, we > should have done the list in patch 02/18 in this order from the > beginning". Can we reorder the patches? I considered this already, but it would more be like a squash than a reordering. And when I squashed the patches, the story did not read as clearly to me as it does now. However, if you think this argument is too weak, I will squash them. Is it too weak? Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/18] fsck: handle multiple authors in commits specially
Hi Junio,
On Wed, 10 Dec 2014, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > This problem has been detected in the wild, and is the primary reason
> > to introduce an option to demote certain fsck errors to warnings. Let's
> > offer to ignore this particular problem specifically.
> > ...
> > + while (skip_prefix(buffer, "author ", &buffer)) {
> > + err = report(options, &commit->object,
> > FSCK_MSG_MULTIPLE_AUTHORS, "invalid format - multiple 'author' lines");
> > + if (err)
> > + return err;
>
> If we have an option to demote this to a warning, wouldn't we want
> to do the same fsck_ident() on that secondary author line?
Good point! I changed the following to use fsck_ident() instead:
> > + /* require_end_of_header() ensured that there is a newline */
> > + buffer = strchr(buffer, '\n') + 1;
> > + }
> > if (!skip_prefix(buffer, "committer ", &buffer))
> > return report(options, &commit->object,
> > FSCK_MSG_MISSING_COMMITTER, "invalid format - expected 'committer' line");
> > err = fsck_ident(&buffer, &commit->object, options);
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use wc instead of awk to count subtrees in t0090-cache-tree
Ben Walton writes:
> The awk statements previously used in this test weren't compatible
> with the native versions of awk on Solaris:
>
> echo "dir" | /bin/awk -v c=0 '$1 {++c} END {print c}'
> awk: syntax error near line 1
> awk: bailing out near line 1
>
> echo "dir" | /usr/xpg4/bin/awk -v c=0 '$1 {++c} END {print c}'
> 0
>
> And with GNU awk for comparison:
> echo "dir" | /opt/csw/gnu/awk -v c=0 '$1 {++c} END {print c}'
> 1
>
> Instead of modifying the awk code to work, use wc -w instead as that
> is both adequate and simpler.
Hmm, why "wc -w" not "wc -l", though? Is somebody squashing a
one-elem-per-line output from ls-files onto a single line?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/18] Allow demoting errors to warnings via receive.fsck. = warn
Hi Junio, On Wed, 10 Dec 2014, Junio C Hamano wrote: > Johannes Schindelin writes: > > > For example, missing emails in commit and tag objects can be demoted to > > mere warnings with > > > > git config receive.fsck.missing-email warn > > No punctuations in the first and the last level of configuration > variable names, please. I.e. s/missing-email/missingEmail/ or > something. Fixed. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/18] Offer a function to demote fsck errors to warnings
Hi Junio,
On Wed, 10 Dec 2014, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > There are legacy repositories out there whose older commits and tags
> > have issues that prevent pushing them when 'receive.fsckObjects' is set.
> > One real-life example is a commit object that has been hand-crafted to
> > list two authors.
> >
> > Often, it is not possible to fix those issues without disrupting the
> > work with said repositories, yet it is still desirable to perform checks
> > by setting `receive.fsckObjects = true`. This commit is the first step
> > to allow demoting specific fsck issues to mere warnings.
> >
> > The function added by this commit parses a list of settings in the form:
> >
> > missing-email=warn,bad-name=warn,...
> >
> > Unfortunately, the FSCK_WARN/FSCK_ERROR flag is only really heeded by
> > git fsck so far, but other call paths (e.g. git index-pack --strict)
> > error out *always* no matter what type was specified. Therefore, we
> > need to take extra care to default to all FSCK_ERROR in those cases.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > fsck.c | 58 ++
> > fsck.h | 7 +--
> > 2 files changed, 63 insertions(+), 2 deletions(-)
> >
> > diff --git a/fsck.c b/fsck.c
> > index 05b146c..9e6d70f 100644
> > --- a/fsck.c
> > +++ b/fsck.c
> > @@ -97,9 +97,63 @@ static int parse_msg_id(const char *text, int len)
> >
> > int fsck_msg_type(enum fsck_msg_id msg_id, struct fsck_options *options)
> > {
> > + if (options->strict_mode && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
> > + return options->strict_mode[msg_id];
> > + if (options->strict)
> > + return FSCK_ERROR;
> > return msg_id < FIRST_WARNING ? FSCK_ERROR : FSCK_WARN;
> > }
>
> Hmm, if you are later going to allow demoting (hopefully also promoting)
> error to warn, etc., would the comparison between msg_id and FIRST_WARNING
> make much sense?
A later patch indeed adds that option. The reason the comparison still
makes sense is that the pure infos do not return at all so far, but all of
the reported warnings are fatal in strict mode (i.e. when
receive.fsckObjects = true). In another later patch it is made possible to
promote even infos (such as 'missing tagger') to warnings or even errors,
and that is when the "return FSCK_ERROR" is changed to "return msg_id <
FIRST_INFO ? FSCK_ERROR : FSCK_WARN".
> In other words, at some point wouldn't we be better off with
> something like this
>
> struct {
> enum id;
> const char *id_string;
> enum error_level { FSCK_PASS, FSCK_WARN, FSCK_ERROR };
> } possible_fsck_errors[];
I considered that, and Michael Haggerty also suggested that in a private
mail. However, I find that there is a clear hierarchy in the default
messages: fatal errors, errors, warnings and infos. This should be
reflected by the order IMHO.
But I guess it would make a lot of sense to insert those levels as special
enum values to make it harder to forget to adjust, say, "#define
FIRST_WARNING FSCK_MSG_BAD_FILEMODE" when introducing another warning that
sorts before said ID alphabetically. In other words, I think that we can
really afford to put something like
...
FUNC(UNKNOWN_TYPE) \
FUNC(ZERO_PADDED_DATE) \
FUNC(___WARNINGS) \
FUNC(BAD_FILEMODE) \
FUNC(EMPTY_NAME) \
...
at the price of making the parsing a little more complicated and wasting a
slight bit of more space for the msg_id_str array.
What do you think?
Ciao,
Dscho
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/18] Provide a function to parse fsck message IDs
Hi Junio,
On Wed, 10 Dec 2014, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > This function will be used in the next commits to allow the user to
> > ask fsck to handle specific problems differently, e.g. demoting certain
> > errors to warnings. It has to handle partial strings because we would
> > like to be able to parse, say, '--strict=missing-email=warn' command
> > lines.
> >
> > To make the parsing robust, we generate strings from the enum keys, and we
> > will match both lower-case, dash-separated values as well as camelCased
> > ones (e.g. both "missing-email" and "missingEmail" will match the
> > "MISSING_EMAIL" key).
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > fsck.c | 32
> > 1 file changed, 32 insertions(+)
> >
> > diff --git a/fsck.c b/fsck.c
> > index 3cea034..05b146c 100644
> > --- a/fsck.c
> > +++ b/fsck.c
> > @@ -63,6 +63,38 @@ enum fsck_msg_id {
> > FSCK_MSG_MAX
> > };
> >
> > +#define STR(x) #x
> > +#define MSG_ID_STR(x) STR(x),
> > +static const char *msg_id_str[FSCK_MSG_MAX + 1] = {
> > + FOREACH_MSG_ID(MSG_ID_STR)
> > + NULL
> > +};
>
> I wondered what the ugly macro was in the previous step, but as a
> way to keep these two lists in sync it makes sense.
I added a comment to the commit message.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git update-ref --stdin : too many open files
Loic Dachary writes: > Hi, > > Steps to reproduce: > > $ git --version > git version 1.9.1 > $ wc -l /tmp/1 > 9090 /tmp/1 > $ head /tmp/1 > delete refs/pull/1/head > create refs/heads/pull/1 86b715f346e52920ca7c9dfe65424eb9946ebd61 > delete refs/pull/1/merge > create refs/merges/1 c0633abdc5311354c9729374e0ba25c97a89f69e > ... > $ ulimit -n > 1024 > $ git update-ref --stdin < /tmp/1 > fatal: Unable to create > /home/gitmirror/repositories/Ceph/ceph/refs/heads/pull/1917.lock': Too > many open files > $ head -20 /tmp/1 | git update-ref --stdin > $ echo $? > 0 > > The workaround is to increase ulimit -n > > git update-ref --stdin should probably close some files. > > Cheers Sounds like the recent "ref update in a transaction" issue to me. Stefan, want to take a look? I think we do need to keep the .lock files without renaming while in transaction, but we do not have to keep them open, so I suspect that a fix may be to split the commit function into two (one to close but not rename, the other to finalize by renaming) or something. Also the version of transaction series we have queued seem to lock these refs very late in the process, but as we discussed privately a few weeks ago, we would want to move the lock much earlier, when the first update is attempted. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-prompt: preserve value of $? inside shell prompt
Tony Finch writes:
> If you have a prompt which displays the command exit status,
> __git_ps1 without this change corrupts it, although it has
> the correct value in the parent shell:
>
> ~/src/git (master) 0 $ set | grep ^PS1
> PS1='\w$(__git_ps1) $? \$ '
> ~/src/git (master) 0 $ false
> ~/src/git (master) 0 $ echo $?
> 1
> ~/src/git (master) 0 $
>
> There is a slightly ugly workaround:
>
> ~/src/git (master) 0 $ set | grep ^PS1
> PS1='\w$(x=$?; __git_ps1; exit $x) $? \$ '
> ~/src/git (master) 0 $ false
> ~/src/git (master) 1 $
>
> This change makes the workaround unnecessary.
>
> Signed-off-by: Tony Finch
> ---
> contrib/completion/git-prompt.sh | 4
> 1 file changed, 4 insertions(+)
>
> I hope that explains it properly :-)
Yes. I wouldn't have spent 20 minutes experimenting with various
hypothetical use cases if the above were there in the first place.
Thanks. Will queue.
> diff --git a/contrib/completion/git-prompt.sh
> b/contrib/completion/git-prompt.sh
> index c5473dc..5fe69d0 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -288,6 +288,7 @@ __git_eread ()
> # In this mode you can request colored hints using
> GIT_PS1_SHOWCOLORHINTS=true
> __git_ps1 ()
> {
> + local exit=$?
> local pcmode=no
> local detached=no
> local ps1pc_start='\u@\h:\w '
> @@ -511,4 +512,7 @@ __git_ps1 ()
> else
> printf -- "$printf_format" "$gitstring"
> fi
> +
> + # preserve exit status
> + return $exit
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/1] http: Add Accept-Language header if possible
Yi EungJun writes:
> From: Yi EungJun
>
> Add an Accept-Language header which indicates the user's preferred
> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.
>
> Examples:
> LANGUAGE= -> ""
> LANGUAGE=ko:en -> "Accept-Language: ko, en;q=0.9, *;q=0.1"
> LANGUAGE=ko LANG=en_US.UTF-8 -> "Accept-Language: ko, *;q=0.1"
> LANGUAGE= LANG=en_US.UTF-8 -> "Accept-Language: en-US, *;q=0.1"
>
> This gives git servers a chance to display remote error messages in
> the user's preferred language.
>
> Limit the number of languages to 1,000 because q-value must not be
> smaller than 0.001, and limit the length of Accept-Language header to
> 4,000 bytes for some HTTP servers which cannot accept such long header.
>
> Signed-off-by: Yi EungJun
Overall, this one is a much more pleasant read than the previous
rounds.
> @@ -986,6 +993,166 @@ static void extract_content_type(struct strbuf *raw,
> struct strbuf *type,
> strbuf_addstr(charset, "ISO-8859-1");
> }
>
> +/*
> + * Guess the user's preferred languages from the value in LANGUAGE
> environment
> + * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.
> + *
> + * The result can be a colon-separated list like "ko:ja:en".
> + */
> +static const char *get_preferred_languages(void)
> +{
> + const char *retval;
> +
> + retval = getenv("LANGUAGE");
> + if (retval && *retval)
> + return retval;
> +
> +#ifndef NO_GETTEXT
> + retval = setlocale(LC_MESSAGES, NULL);
> + if (retval && *retval &&
> + strcmp(retval, "C") &&
> + strcmp(retval, "POSIX"))
> + return retval;
> +#endif
A tangent.
I wonder if we want to have something silly like this:
#ifndef NO_GETTEXT
#define setlocale(x, y) NULL /* or "C"??? */
#endif
in a common header (e.g. gettext.h) to avoid sprinkling #ifdefs in
our code. While I do not think we call setlocale() that often to
warrant such a trick, we already do something very similar to make
git_setup_gettext() a no-op in NO_GETTEXT builds in that header
file, and the change in this patch to remote-curl.c does take
advantage of it already, so...
> +static void write_accept_language(struct strbuf *buf)
> +{
> + const char *lang_begin, *pos;
> + int q, max_q;
> + int num_langs;
> + int decimal_places;
> + const int CODESET_OR_MODIFIER = 1;
> + const int LANGUAGE_TAG = 2;
> + const int SEPARATOR = 3;
Another tangent, but I think we tend to use either #define or enum
for constants, not "const int", in our codebase, for symbolic
constants. In order to define a set of symbolic constants limited
to a function scope, the "const int" way may be nicer than the other
two methods we have traditionally used. Perhaps we should promote
such use more widely, write our new code following this example, and
migrate existing ones over time? I dunno.
> ...
> + /* Decide the precision for q-factor on number of preferred lang_begin.
> */
> + num_langs += 1; /* for '*' */
> +
> + if (MAX_LANGS < num_langs)
> + num_langs = MAX_LANGS;
> +
> + for (max_q = 1, decimal_places = 0;
> + max_q < num_langs;
> + decimal_places++, max_q *= 10)
> + ;
So, if we have 10 languages (num_langs == 10), decimal_places
becomes 1, max_q becomes 10 ...
> + sprintf(q_format, ";q=0.%%0%dd", decimal_places);
... and we will use "q=0.%01d" as the format. This is OK because
the first one is given without q= so we use the format only nine
times, counting down from 0.9 to 0.1 in 0.1 increments.
Sounds OK to me (I always miscount this kind of loop and need to
think aloud while doing a sanity check).
> + for (pos = lang_begin; ; pos++) {
> + if (!*pos || *pos == ':') {
> + if (is_q_factor_required) {
> + /* Put a q-factor only if it is less than 1.0.
> */
> + if (q < max_q)
> + strbuf_addf(buf, q_format, q);
> +
> + if (q > 1)
> + q--;
When does this "if" statement not trigger? It seems to me that it
will stop decrementing only if you have very many languages (e.g.
num_langs was clipped to MAX_LANGS), and at that point you would not
want to scan and add more languages---is there a reason why you keep
going in such a case and not break out of the loop, i.e.
if (q-- < 1)
break;
or something like that?
> + ...
> + if (buf->len > MAX_SIZE_OF_HEADER -
> MAX_SIZE_OF_ASTERISK_ELEMENT) {
> + strbuf_remove(buf, last_size, buf->len - last_size);
> + break;
> + }
> +
> + if (!*pos)
> + break;
Alternatively use one of these breaks when q goes below 1, perhaps?
> +/*
> + * Get an Accept-Language header which indicates user's preferred languages.
> +
Re: [PATCH] completion: add --irreversible-delete for format-patch
Junio C Hamano wrote: > Eric Wong writes: > > > Normally I would use "-D", but send-email (which normally passes options > > to format-patch) interprets the "-D" as a case-insensitive abbreviation > > for "--dry-run", preventing format-patch from seeing "-D". > > Is this nonstandard option that is designed to produce an > inapplicable patch so widely used to warrant a completion? > > I'd actually understand it if this were to complete "git show" and > friends, but not for format-patch. I'd actually think we might be > better off forbidding its use for the format-patch command (but not > for other commands in the "git log" family), not just at the > completion level but at the command line argument parser level. > > Hmph... I was actually hoping Paul would resurrect his work on getting apply to understand --irreversible-delete: http://mid.gmane.org/1343939748-12256-1-git-send-email-paul.gortma...@windriver.com ($gmane/202789) I find this option useful to reduce mail traffic for others to review changes which are already pushed to a public repo. > > Signed-off-by: Eric Wong > > --- > > Case-insensitivity strikes again! :< > > What a wacky default for Getopt::Long... And it's probably too late > > for us to disable case-insensitivity in CLI parsing for send-email. > > > > contrib/completion/git-completion.bash | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/contrib/completion/git-completion.bash > > b/contrib/completion/git-completion.bash > > index 2fece98..41d8ff8 100644 > > --- a/contrib/completion/git-completion.bash > > +++ b/contrib/completion/git-completion.bash > > @@ -1257,6 +1257,7 @@ __git_format_patch_options=" > > --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix= > > --inline --suffix= --ignore-if-in-upstream --subject-prefix= > > --output-directory --reroll-count --to= --quiet --notes > > + --irreversible-delete > > " > > > > _git_format_patch () -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Use wc instead of awk to count subtrees in t0090-cache-tree
The awk statements previously used in this test weren't compatible
with the native versions of awk on Solaris:
echo "dir" | /bin/awk -v c=0 '$1 {++c} END {print c}'
awk: syntax error near line 1
awk: bailing out near line 1
echo "dir" | /usr/xpg4/bin/awk -v c=0 '$1 {++c} END {print c}'
0
And with GNU awk for comparison:
echo "dir" | /opt/csw/gnu/awk -v c=0 '$1 {++c} END {print c}'
1
Instead of modifying the awk code to work, use wc -w instead as that
is both adequate and simpler.
Signed-off-by: Ben Walton
---
t/t0090-cache-tree.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 067f4c6..f2b1c9c 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -22,7 +22,7 @@ generate_expected_cache_tree_rec () {
# ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
# We want to count only foo because it's the only direct child
subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq) &&
- subtree_count=$(echo "$subtrees"|awk -v c=0 '$1 {++c} END {print c}') &&
+ subtree_count=$(echo "$subtrees"|wc -w) &&
entries=$(git ls-files|wc -l) &&
printf "SHA $dir (%d entries, %d subtrees)\n" "$entries"
"$subtree_count" &&
for subtree in $subtrees
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] completion: add --irreversible-delete for format-patch
Eric Wong writes: > Normally I would use "-D", but send-email (which normally passes options > to format-patch) interprets the "-D" as a case-insensitive abbreviation > for "--dry-run", preventing format-patch from seeing "-D". Is this nonstandard option that is designed to produce an inapplicable patch so widely used to warrant a completion? I'd actually understand it if this were to complete "git show" and friends, but not for format-patch. I'd actually think we might be better off forbidding its use for the format-patch command (but not for other commands in the "git log" family), not just at the completion level but at the command line argument parser level. Hmph... > Signed-off-by: Eric Wong > --- > Case-insensitivity strikes again! :< > What a wacky default for Getopt::Long... And it's probably too late > for us to disable case-insensitivity in CLI parsing for send-email. > > contrib/completion/git-completion.bash | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/contrib/completion/git-completion.bash > b/contrib/completion/git-completion.bash > index 2fece98..41d8ff8 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -1257,6 +1257,7 @@ __git_format_patch_options=" > --not --all --cover-letter --no-prefix --src-prefix= --dst-prefix= > --inline --suffix= --ignore-if-in-upstream --subject-prefix= > --output-directory --reroll-count --to= --quiet --notes > + --irreversible-delete > " > > _git_format_patch () -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Bad file descriptor on filtering empty files
William Throwe writes:
> In git 2.2.0 (also tested on 2.2.0.65.g9abc44b), if an external
> smudge/clean filter is called on an empty file git reports something
> like:
> error: copy-fd: read returned Bad file descriptor
> error: cannot feed the input to external filter cat
> error: external filter cat failed
>
> Test case:
>
> mkdir bug
> cd bug
> git init
> git config filter.cat.clean cat
> git config filter.cat.smudge cat
> echo '* filter=cat' >.gitattributes
> touch a
> git add a
>
>
> This started in 9035d75a2be9d80d82676504d69553245017f6d4, which
> introduced the possible call to copy_fd in code called from
> apply_filter. It appears that NULL as the src argument to apply_filter
> is being used both as a sentinel value to indicate that the fd should be
> used instead and also as a representation of the contents of an empty
> file. I suggest switching to using fd == -1 as the sentinel as shown in
> the patch below.
>
> Thanks,
> Will
>
William, thanks for raising this issue.
Steffen, comments?
> diff --git a/convert.c b/convert.c
> index 9a5612e..0509ac1 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -355,7 +355,7 @@ static int filter_buffer_or_fd(int in, int out, void
> *data)
>
> sigchain_push(SIGPIPE, SIG_IGN);
>
> - if (params->src) {
> + if (params->fd == -1) {
> write_err = (write_in_full(child_process.in, params->src,
> params->size) < 0);
> } else {
> write_err = copy_fd(params->fd, child_process.in);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 0/7] atomic pushes
Will queue; thanks for a pleasant read. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] receive-pack: move execute_commands_non_atomic before execute_commands
Stefan Beller writes: > Notes: > This patch is new with v6 of the series > > As execute_commands_non_atomic is larger than execute_commands, the diff > is not moving around execute_commands_non_atomic, but execute_commands. ;-) Next time perhaps try "--patience" to decide between with and without which one reads better? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] git-prompt: preserve value of $? inside shell prompt
If you have a prompt which displays the command exit status,
__git_ps1 without this change corrupts it, although it has
the correct value in the parent shell:
~/src/git (master) 0 $ set | grep ^PS1
PS1='\w$(__git_ps1) $? \$ '
~/src/git (master) 0 $ false
~/src/git (master) 0 $ echo $?
1
~/src/git (master) 0 $
There is a slightly ugly workaround:
~/src/git (master) 0 $ set | grep ^PS1
PS1='\w$(x=$?; __git_ps1; exit $x) $? \$ '
~/src/git (master) 0 $ false
~/src/git (master) 1 $
This change makes the workaround unnecessary.
Signed-off-by: Tony Finch
---
contrib/completion/git-prompt.sh | 4
1 file changed, 4 insertions(+)
I hope that explains it properly :-)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index c5473dc..5fe69d0 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -288,6 +288,7 @@ __git_eread ()
# In this mode you can request colored hints using GIT_PS1_SHOWCOLORHINTS=true
__git_ps1 ()
{
+ local exit=$?
local pcmode=no
local detached=no
local ps1pc_start='\u@\h:\w '
@@ -511,4 +512,7 @@ __git_ps1 ()
else
printf -- "$printf_format" "$gitstring"
fi
+
+ # preserve exit status
+ return $exit
}
--
2.2.1.68.g56d9796
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/5] update_unicode.sh: set UNICODE_DIR only once
dev+...@drbeat.li writes:
> From: Beat Bolli
>
> The value is the same on both uniset invocations, so "Don't Repeat
> Yourself" applies.
>
> Since we're in a subshell already, there's no need to unset UNICODE_DIR
> at the end.
Strictly speaking, you are not introducing your own subshell to
prevent the environment from leaking (i.e. you used "{...}" not
"(...)" in the previous step). The reason you can do this is
because the generation of UNICODEWIDTH_H file is the last thing in
the subshell.
I'll reword it to "Since this is done as the last command, ..."
Thanks.
>
> Signed-off-by: Beat Bolli
> ---
> update_unicode.sh | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/update_unicode.sh b/update_unicode.sh
> index c1c876c..bed8916 100755
> --- a/update_unicode.sh
> +++ b/update_unicode.sh
> @@ -27,12 +27,13 @@ fi &&
> fi &&
> make
> ) && {
> + UNICODE_DIR=. && export UNICODE_DIR &&
> echo "static const struct interval zero_width[] = {" &&
> - UNICODE_DIR=. ./uniset/uniset --32 cat:Me,Mn,Cf +
> U+1160..U+11FF - U+00AD |
> + ./uniset/uniset --32 cat:Me,Mn,Cf + U+1160..U+11FF - U+00AD |
> grep -v plane &&
> echo "};" &&
> echo "static const struct interval double_width[] = {" &&
> - UNICODE_DIR=. ./uniset/uniset --32 eaw:F,W &&
> + ./uniset/uniset --32 eaw:F,W &&
> echo "};"
> } >$UNICODEWIDTH_H
> )
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] clean: typo fixed
Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Documentation/SubmittingPatches: unify whitespace/tabs for the DCO
Stefan Beller writes: > The Developers Certificate of Origin has a mixture of tabs and white > spaces which is annoying to view if your editor explicitly views white > space characters. > > Signed-off-by: Stefan Beller > --- > > Notes: > I'll also try to send it upstream to linux. Thanks. > > Documentation/SubmittingPatches | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches > index 16b5d65..6124f34 100644 > --- a/Documentation/SubmittingPatches > +++ b/Documentation/SubmittingPatches > @@ -258,15 +258,15 @@ pretty simple: if you can certify the below: > person who certified (a), (b) or (c) and I have not modified > it. > > - (d) I understand and agree that this project and the contribution > - are public and that a record of the contribution (including all > - personal information I submit with it, including my sign-off) is > - maintained indefinitely and may be redistributed consistent with > - this project or the open source license(s) involved. > +(d) I understand and agree that this project and the contribution > +are public and that a record of the contribution (including all > +personal information I submit with it, including my sign-off) is > +maintained indefinitely and may be redistributed consistent with > +this project or the open source license(s) involved. > > then you just add a line saying > > - Signed-off-by: Random J Developer > +Signed-off-by: Random J Developer > > This line can be automatically added by Git if you run the git-commit > command with the -s option. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Documentation/SubmittingPatches: Explain the rationale of git notes
Stefan Beller writes: > This adds an explanation of why you want to have the --notes option > given to git format-patch. > > Signed-off-by: Stefan Beller > --- > > Notes: > > with optionally update Documentation/SubmittingPatches > > to point at the file. > > That file actually talks about notes already. I am sending > two patches touching that file, one of them explaining > the --notes workflow rationale and one of them just changing > white spaces. > > A few weeks ago I wanted to patch format-patch to remove > change ids. This is not needed any more for the git workflow > as I disabled them and do not upload any patches to an optional > Gerrit code review server anymore. > > I do like the workflow using --notes as well from a developers > perspective as I take literally notes for my own sanity. > I wonder if I should add a config format.notes = [default-off, > always, on-if-non-empty] so I don't need always add --notes > manually to the command line. > > Thanks, > Stefan > > Documentation/SubmittingPatches | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches > index fa71b5f..16b5d65 100644 > --- a/Documentation/SubmittingPatches > +++ b/Documentation/SubmittingPatches > @@ -176,7 +176,11 @@ message starts, you can put a "From: " line to name that > person. > You often want to add additional explanation about the patch, > other than the commit message itself. Place such "cover letter" > material between the three dash lines and the diffstat. Git-notes > -can also be inserted using the `--notes` option. > +can also be inserted using the `--notes` option. If you are one Because the previous sentence is talking about the cover letter to describe the overall series, it probably is a good idea to add "after the three-dashes of each patch" after "... using the notes option" for clarity, perhaps? > +of those developers who cannot write perfect code the first time > +and need multiple iterations of review and discussion, you are > +encouraged to use the notes to describe the changes between the > +different versions of a patch. Perhaps s/you are encouraged to/you may want to/? It might be better to be even more explicit, e.g. "you may want to keep track of the changes between the versions using the notes and then use `--notes` when preparing the series for submission"? > Do not attach the patch as a MIME attachment, compressed or not. > Do not let your e-mail client send quoted-printable. Do not let Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] git remote add: allow re-adding remotes with the same URL
Johannes Schindelin writes:
> When adding a remote, we make sure that the remote does not exist
> already.
>
> For convenience, we allow re-adding remotes with the same URLs.
> This also handles the case that there is an "[url ...] insteadOf"
> setting in the config.
>
> It might seem like a mistake to compare against remote->url[0] without
> verifying that remote->url_nr >=1, but at this point a missing URL has
> been filled by the name already, therefore url_nr cannot be zero.
>
> Noticed by Anastas Dancha.
>
> Signed-off-by: Johannes Schindelin
> ---
> builtin/remote.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 46ecfd9..9168c83 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -180,7 +180,8 @@ static int add(int argc, const char **argv)
> url = argv[1];
>
> remote = remote_get(name);
> - if (remote && (remote->url_nr > 1 || strcmp(name, remote->url[0]) ||
> + if (remote && (remote->url_nr > 1 || (strcmp(name, remote->url[0]) &&
> + strcmp(url, remote->url[0])) ||
> remote->fetch_refspec_nr))
> die(_("remote %s already exists."), name);
When we need to fold an overlong line, it is easier to read if the
line is folded at an operator with higher precedence, i.e. this line
if (A && (B || (C && D) || E))
folded like this
if (A && (B || (C &&
D) ||
E))
is harder to read than when folded like this
if (A && (B ||
(C && D) ||
E))
So, it is an error if we have "remote" and if
(1) URL for the remote is defined already twice or more; or
(2) we are adding a nickname (i.e. not a URL) and it is different
from what we already have; or
(3) we already have fetch_refspec
The way I read the log message's rationale was that this is to allow
replacing an existing remote's URL; wouldn't checking the existence
of fetch_refspec go against that goal?
Puzzled. Either the code is wrong or I am mislead by the
explanation in the log.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/18] Introduce fsck options
Johannes Schindelin writes: >> Is it a good idea to allow walker to be strict but obj verifier to >> be not (or vice versa)? I am wondering why this is not a single >> struct with two callback function pointers. > > Unfortunately not. There are two different walkers used, and in fact, > fsck_walk_options() is only used to walk the objects, not to fsck them. > > Now, I could use only one struct and set the walker, but that is not > thread-safe, and while code is not threaded yet AFAICT, it might be in the > future. That is why I decided to be rather safe than sorry. If you want it > differently, please just say the word, I will make it so. Thanks for explaining; I just found that the reason behind the design choice was unclear and wanted to know. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/18] Introduce fsck options
Hi Junio,
On Wed, 10 Dec 2014, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > Subject: Re: [PATCH 01/18] Introduce fsck options
>
> please make it easier to cluster and spot the series in the eventual
> shortlog by giving a common prefix to the patches, e.g.
>
> fsck: introduce fsck_options struct
I use the fsck: prefix consistently now.
> > +static struct fsck_options fsck_walk_options = FSCK_OPTIONS_DEFAULT;
> > +static struct fsck_options fsck_obj_options = FSCK_OPTIONS_DEFAULT;
>
> Is it a good idea to allow walker to be strict but obj verifier to
> be not (or vice versa)? I am wondering why this is not a single
> struct with two callback function pointers.
Unfortunately not. There are two different walkers used, and in fact,
fsck_walk_options() is only used to walk the objects, not to fsck them.
Now, I could use only one struct and set the walker, but that is not
thread-safe, and while code is not threaded yet AFAICT, it might be in the
future. That is why I decided to be rather safe than sorry. If you want it
differently, please just say the word, I will make it so.
> > +struct fsck_options {
> > + fsck_walk_func walk;
> > + fsck_error error_func;
> > + int strict:1;
>
> A signed 1-bit-wide bitfield can hold its sign-bit and nothing else,
> no?
>
> unsigned strict:1;
Oops. Right. For some reason, it worked here, though... Fixed!
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-prompt: preserve command exit status
Tony Finch writes:
> Signed-off-by: Tony Finch
> ---
> contrib/completion/git-prompt.sh | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/contrib/completion/git-prompt.sh
> b/contrib/completion/git-prompt.sh
> index c5473dc..5fe69d0 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -288,6 +288,7 @@ __git_eread ()
> # In this mode you can request colored hints using
> GIT_PS1_SHOWCOLORHINTS=true
> __git_ps1 ()
> {
> + local exit=$?
> local pcmode=no
> local detached=no
> local ps1pc_start='\u@\h:\w '
> @@ -511,4 +512,7 @@ __git_ps1 ()
> else
> printf -- "$printf_format" "$gitstring"
> fi
> +
> + # preserve exit status
> + return $exit
> }
H. I thought "The patch trivially makes sense! Why didn't
anybody notice this before?!?", but then noticed that I never
suffered from an obvious consequence from the current lack of the
exit-code-preserving:
: gitster git.git/master; echo "<$PS1>"
<: \h \W$(__git_ps1 "/%s"); >
: gitster git.git/master; false
: gitster git.git/master; echo $?
1
And it does not seem that it is needed, at least for my use
pattern:
: gitster git.git/master; ps1func () { echo "What Now: "; exit 8; }
: gitster git.git/master; PS1='$(ps1func)'
What Now: echo $?
0
What Now: (exit 6)
What Now: echo $?
6
Also it does not seem that it is needed for the other style to use
PROMPT_COMMAND:
: gitster git.git/master; sh
$ . contrib/completion/git-prompt.sh
$ PROMPT_COMMAND='__git_ps1 "\h \W" "; "'
gitster git.git (master); (exit 13)
gitster git.git (master); echo $?
13
gitster git.git (master); true
gitster git.git (master); echo $?
0
So, what are you fixing? In other words, please describe how it
fails in the log message.
Puzzled...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] git remote add: allow re-adding remotes with the same URL
When adding a remote, we make sure that the remote does not exist
already.
For convenience, we allow re-adding remotes with the same URLs.
This also handles the case that there is an "[url ...] insteadOf"
setting in the config.
It might seem like a mistake to compare against remote->url[0] without
verifying that remote->url_nr >=1, but at this point a missing URL has
been filled by the name already, therefore url_nr cannot be zero.
Noticed by Anastas Dancha.
Signed-off-by: Johannes Schindelin
---
builtin/remote.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 46ecfd9..9168c83 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -180,7 +180,8 @@ static int add(int argc, const char **argv)
url = argv[1];
remote = remote_get(name);
- if (remote && (remote->url_nr > 1 || strcmp(name, remote->url[0]) ||
+ if (remote && (remote->url_nr > 1 || (strcmp(name, remote->url[0]) &&
+ strcmp(url, remote->url[0])) ||
remote->fetch_refspec_nr))
die(_("remote %s already exists."), name);
--
2.0.0.rc3.9669.g840d1f9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] Add a regression test for 'git remote add '
Signed-off-by: Johannes Schindelin --- t/t5505-remote.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index ac79dd9..17c6330 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1113,4 +1113,9 @@ test_extra_arg set-url origin newurl oldurl # prune takes any number of args # update takes any number of args +test_expect_success 'add remote matching the "insteadOf" URL' ' + git config url@example.com.insteadOf backup && + git remote add backup x...@example.com +' + test_done -- 2.0.0.rc3.9669.g840d1f9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Let `git remote add` play nicer with url..insteadOf
Anastas Dancha reported that it is not possible to add a remote when there is already a url..insteadOf = setting in $HOME/.gitconfig. While it makes sense to prevent surprises when a user adds a remote and it fetches from somewhere completely different, it makes less sense to prevent adding a remote when it is actually the same that was specified in the config. Therefore we add just another check that let's `git remote add` continue when the "existing" remote's URL is identical to the specified one. Signed-off-by: Johannes Schindelin Johannes Schindelin (2): git remote add: allow re-adding remotes with the same URL Add a regression test for 'git remote add ' builtin/remote.c | 3 ++- t/t5505-remote.sh | 5 + 2 files changed, 7 insertions(+), 1 deletion(-) -- 2.0.0.rc3.9669.g840d1f9 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 1/1] http: Add Accept-Language header if possible
From: Yi EungJun
Add an Accept-Language header which indicates the user's preferred
languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.
Examples:
LANGUAGE= -> ""
LANGUAGE=ko:en -> "Accept-Language: ko, en;q=0.9, *;q=0.1"
LANGUAGE=ko LANG=en_US.UTF-8 -> "Accept-Language: ko, *;q=0.1"
LANGUAGE= LANG=en_US.UTF-8 -> "Accept-Language: en-US, *;q=0.1"
This gives git servers a chance to display remote error messages in
the user's preferred language.
Limit the number of languages to 1,000 because q-value must not be
smaller than 0.001, and limit the length of Accept-Language header to
4,000 bytes for some HTTP servers which cannot accept such long header.
Signed-off-by: Yi EungJun
---
http.c | 173 +
remote-curl.c | 2 +
t/t5550-http-fetch-dumb.sh | 32 +
3 files changed, 207 insertions(+)
diff --git a/http.c b/http.c
index 040f362..7a77708 100644
--- a/http.c
+++ b/http.c
@@ -68,6 +68,8 @@ static struct curl_slist *no_pragma_header;
static struct active_request_slot *active_queue_head;
+static char *cached_accept_language;
+
size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
@@ -515,6 +517,11 @@ void http_cleanup(void)
cert_auth.password = NULL;
}
ssl_cert_password_required = 0;
+
+ if (cached_accept_language) {
+ free(cached_accept_language);
+ cached_accept_language = NULL;
+ }
}
struct active_request_slot *get_active_slot(void)
@@ -986,6 +993,166 @@ static void extract_content_type(struct strbuf *raw,
struct strbuf *type,
strbuf_addstr(charset, "ISO-8859-1");
}
+/*
+ * Guess the user's preferred languages from the value in LANGUAGE environment
+ * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.
+ *
+ * The result can be a colon-separated list like "ko:ja:en".
+ */
+static const char *get_preferred_languages(void)
+{
+ const char *retval;
+
+ retval = getenv("LANGUAGE");
+ if (retval && *retval)
+ return retval;
+
+#ifndef NO_GETTEXT
+ retval = setlocale(LC_MESSAGES, NULL);
+ if (retval && *retval &&
+ strcmp(retval, "C") &&
+ strcmp(retval, "POSIX"))
+ return retval;
+#endif
+
+ return NULL;
+}
+
+static void write_accept_language(struct strbuf *buf)
+{
+ const char *lang_begin, *pos;
+ int q, max_q;
+ int num_langs;
+ int decimal_places;
+ const int CODESET_OR_MODIFIER = 1;
+ const int LANGUAGE_TAG = 2;
+ const int SEPARATOR = 3;
+ int is_q_factor_required = 0;
+ int parse_state = 0;
+ char q_format[32];
+ /*
+* MAX_LANGS must not be larger than 1,000. If it is larger than that,
+* q-value will be smaller than 0.001, the minimum q-value the HTTP
+* specification allows [1].
+*
+* [1]: http://tools.ietf.org/html/rfc7231#section-5.3.1
+*/
+ const int MAX_LANGS = 1000;
+ const int MAX_SIZE_OF_HEADER = 4000;
+ const int MAX_SIZE_OF_ASTERISK_ELEMENT = 11; /* for ", *;q=0.001" */
+ int last_size = 0;
+
+ lang_begin = get_preferred_languages();
+
+ /* Don't add Accept-Language header if no language is preferred. */
+ if (!lang_begin)
+ return;
+
+ /* Count number of preferred lang_begin to decide precision of
q-factor. */
+ for (num_langs = 1, pos = lang_begin; *pos; pos++)
+ if (*pos == ':')
+ num_langs++;
+
+ /* Decide the precision for q-factor on number of preferred lang_begin.
*/
+ num_langs += 1; /* for '*' */
+
+ if (MAX_LANGS < num_langs)
+ num_langs = MAX_LANGS;
+
+ for (max_q = 1, decimal_places = 0;
+ max_q < num_langs;
+ decimal_places++, max_q *= 10)
+ ;
+
+ sprintf(q_format, ";q=0.%%0%dd", decimal_places);
+
+ q = max_q;
+
+ strbuf_addstr(buf, "Accept-Language: ");
+
+ /*
+* Convert a list of colon-separated locale values [1][2] to a list of
+* comma-separated language tags [3] which can be used as a value of
+* Accept-Language header.
+*
+* [1]: http://pubs.opengroup.org/onlinepubs/007908799/xbd/envvar.html
+* [2]:
http://www.gnu.org/software/libc/manual/html_node/Using-gettextized-software.html
+* [3]: http://tools.ietf.org/html/rfc7231#section-5.3.5
+*/
+ for (pos = lang_begin; ; pos++) {
+ if (!*pos || *pos == ':') {
+ if (is_q_factor_required) {
+ /* Put a q-factor only if it is less than 1.0.
*/
+ if (q < max_q)
+ strbuf_addf(buf, q_format, q);
+
+ if (
[PATCH v6 0/1] http: Add Accept-Language header if possible
Changes since v5 >From Junio C Hamano's review: * The tests use `ls-remote` instead of `clone` for tests; I copied the test code from ba8e63dc30a80656fddc616f714fb217ad220c04. * Set cached_accept_langauge to NULL after free it. >From Eric Sunshine's review: * get_accept_language() returns a pointer to const char instead of strbuf; the type of cached_accept_language also has been changed to char* from strbuf* * write_accept_language(), which is extracted from get_accept_language(), respects MAX_SIZE_OF_HEADER. * The for-loop in write_accept_language() works correctly if lang_begin points an empty string. >From Jeff King's advice: * get_preferred_languages() considers LC_MESSAGES only if NO_GETTEXT is not defined. * Remove the tests for LC_MESSAGES, LANG and LC_ALL. Yi EungJun (1): http: Add Accept-Language header if possible http.c | 173 + remote-curl.c | 2 + t/t5550-http-fetch-dumb.sh | 32 + 3 files changed, 207 insertions(+) -- 2.2.0.375.gcd18ce6.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Supporting a few more usecases with remote helpers
Mike Hommey writes: > 1. I think it should, as long as the given sha1 is reachable from the > public heads, but that's offtopic here. Sounds vaguely familiar: http://thread.gmane.org/gmane.comp.version-control.git/178814/focus=179007 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Misbehaving git bisect bad HEAD
On Mon, Dec 22, 2014 at 3:04 PM, Andreas Schwab wrote:
> Running "git bisect bad" should be the same as "git bisect bad HEAD",
> shouldn't it?
Yeah, it should.
> When replaying this bisect log on the Linux kernel tree:
>
> git bisect start
> # bad: [97bf6af1f928216fd6c5a66e8a57bfa95a659672] Linux 3.19-rc1
> git bisect bad 97bf6af1f928216fd6c5a66e8a57bfa95a659672
> # good: [b2776bf7149bddd1f4161f14f79520f17fc1d71d] Linux 3.18
> git bisect good b2776bf7149bddd1f4161f14f79520f17fc1d71d
> # good: [70e71ca0af244f48a5dcf56dc435243792e3a495] Merge
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next
> git bisect good 70e71ca0af244f48a5dcf56dc435243792e3a495
> # good: [988adfdffdd43cfd841df734664727993076d7cb] Merge branch 'drm-next' of
> git://people.freedesktop.org/~airlied/linux
> git bisect good 988adfdffdd43cfd841df734664727993076d7cb
> # good: [b024793188002b9eed452b5f6a04d45003ed5772] staging: rtl8723au:
> phy_SsPwrSwitch92CU() was never called with bRegSSPwrLvl != 1
> git bisect good b024793188002b9eed452b5f6a04d45003ed5772
> # good: [66dcff86ba40eebb5133cccf450878f2bba102ef] Merge tag 'for-linus' of
> git://git.kernel.org/pub/scm/virt/kvm/kvm
> git bisect good 66dcff86ba40eebb5133cccf450878f2bba102ef
> # bad: [88a57667f2990f00b019d46c8426441c9e516d51] Merge branch
> 'perf-urgent-for-linus' of
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
> git bisect bad 88a57667f2990f00b019d46c8426441c9e516d51
> # good: [0ec28c37c21a2b4393692e832e11a7573ac545e2] Merge tag 'media/v3.19-2'
> of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
> git bisect good 0ec28c37c21a2b4393692e832e11a7573ac545e2
> # good: [c0f486fde3f353232c1cc2fd4d62783ac782a467] Merge tag
> 'pm+acpi-3.19-rc1-2' of
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
> git bisect good c0f486fde3f353232c1cc2fd4d62783ac782a467
> # bad: [34b85e3574424beb30e4cd163e6da2e2282d2683] Merge tag 'powerpc-3.19-2'
> of git://git.kernel.org/pub/scm/linux/kernel/git/mpe/linux
> git bisect bad 34b85e3574424beb30e4cd163e6da2e2282d2683
> # good: [64ec45bff6b3dade2643ed4c0f688a15ecf46ea2] Merge tag 'for_linus' of
> git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost
> git bisect good 64ec45bff6b3dade2643ed4c0f688a15ecf46ea2
>
> Running "git bisect bad" gives this:
>
> $ git bisect bad
> Bisecting: 6 revisions left to test after this (roughly 3 steps)
> [ec2aef5a8d3c14272f7a2d29b34f1f8e71f2be5b] power/perf/hv-24x7: Use
> kmem_cache_free() instead of kfree
>
> Running "git bisect bad HEAD" instead gives this:
>
> $ git bisect bad HEAD
> Bisecting: a merge base must be tested
> [56548fc0e86cb9156af7a7e1f15ba78f251dafaf] powerpc/powernv: Return to cpu
> offline loop when finished in KVM guest
>
> This is git 2.2.1.
I think it is a very old bug.
The following patch should fix it:
diff --git a/git-bisect.sh b/git-bisect.sh
index 6cda2b5..26a336a 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -200,7 +200,8 @@ is_expected_rev() {
check_expected_revs() {
for _rev in "$@"; do
- if ! is_expected_rev "$_rev"
+ _parsed=$(git rev-parse --verify "$_rev")
+ if ! is_expected_rev "$_parsed"
then
rm -f "$GIT_DIR/BISECT_ANCESTORS_OK"
rm -f "$GIT_DIR/BISECT_EXPECTED_REV"
I will send a proper patch later.
Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-prompt: preserve command exit status
Signed-off-by: Tony Finch
---
contrib/completion/git-prompt.sh | 4
1 file changed, 4 insertions(+)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index c5473dc..5fe69d0 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -288,6 +288,7 @@ __git_eread ()
# In this mode you can request colored hints using GIT_PS1_SHOWCOLORHINTS=true
__git_ps1 ()
{
+ local exit=$?
local pcmode=no
local detached=no
local ps1pc_start='\u@\h:\w '
@@ -511,4 +512,7 @@ __git_ps1 ()
else
printf -- "$printf_format" "$gitstring"
fi
+
+ # preserve exit status
+ return $exit
}
--
2.1.0.rc1.12.g1e9b79d
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Misbehaving git bisect bad HEAD
Running "git bisect bad" should be the same as "git bisect bad HEAD", shouldn't it? When replaying this bisect log on the Linux kernel tree: git bisect start # bad: [97bf6af1f928216fd6c5a66e8a57bfa95a659672] Linux 3.19-rc1 git bisect bad 97bf6af1f928216fd6c5a66e8a57bfa95a659672 # good: [b2776bf7149bddd1f4161f14f79520f17fc1d71d] Linux 3.18 git bisect good b2776bf7149bddd1f4161f14f79520f17fc1d71d # good: [70e71ca0af244f48a5dcf56dc435243792e3a495] Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next git bisect good 70e71ca0af244f48a5dcf56dc435243792e3a495 # good: [988adfdffdd43cfd841df734664727993076d7cb] Merge branch 'drm-next' of git://people.freedesktop.org/~airlied/linux git bisect good 988adfdffdd43cfd841df734664727993076d7cb # good: [b024793188002b9eed452b5f6a04d45003ed5772] staging: rtl8723au: phy_SsPwrSwitch92CU() was never called with bRegSSPwrLvl != 1 git bisect good b024793188002b9eed452b5f6a04d45003ed5772 # good: [66dcff86ba40eebb5133cccf450878f2bba102ef] Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm git bisect good 66dcff86ba40eebb5133cccf450878f2bba102ef # bad: [88a57667f2990f00b019d46c8426441c9e516d51] Merge branch 'perf-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip git bisect bad 88a57667f2990f00b019d46c8426441c9e516d51 # good: [0ec28c37c21a2b4393692e832e11a7573ac545e2] Merge tag 'media/v3.19-2' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media git bisect good 0ec28c37c21a2b4393692e832e11a7573ac545e2 # good: [c0f486fde3f353232c1cc2fd4d62783ac782a467] Merge tag 'pm+acpi-3.19-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm git bisect good c0f486fde3f353232c1cc2fd4d62783ac782a467 # bad: [34b85e3574424beb30e4cd163e6da2e2282d2683] Merge tag 'powerpc-3.19-2' of git://git.kernel.org/pub/scm/linux/kernel/git/mpe/linux git bisect bad 34b85e3574424beb30e4cd163e6da2e2282d2683 # good: [64ec45bff6b3dade2643ed4c0f688a15ecf46ea2] Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost git bisect good 64ec45bff6b3dade2643ed4c0f688a15ecf46ea2 Running "git bisect bad" gives this: $ git bisect bad Bisecting: 6 revisions left to test after this (roughly 3 steps) [ec2aef5a8d3c14272f7a2d29b34f1f8e71f2be5b] power/perf/hv-24x7: Use kmem_cache_free() instead of kfree Running "git bisect bad HEAD" instead gives this: $ git bisect bad HEAD Bisecting: a merge base must be tested [56548fc0e86cb9156af7a7e1f15ba78f251dafaf] powerpc/powernv: Return to cpu offline loop when finished in KVM guest This is git 2.2.1. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] configure.ac,trace.c: check for CLOCK_MONOTONIC
On Sun, Dec 21, 2014 at 8:12 PM, brian m. carlson wrote: > On Sun, Dec 21, 2014 at 10:53:35AM -0800, Reuben Hawkins wrote: >> CLOCK_MONOTONIC isn't available on RHEL3, but there are still RHEL3 >> systems being used in production. This change makes compiling git >> less tedious on older platforms. > > While I'm not opposed to this change, I expect that you'll find there's > lots of subtle breakage when trying to compile (and run the testsuite > for) git on RHEL 3. I've spoken up before to prevent breakage on > RHEL/CentOS 5 (since $DAYJOB still supports it), but I'm not sure > anyone's looking out for something as old as RHEL 3. I expect you'll > probably have some pain points with perl and curl, among others. Yes, there are pain points with perl and curl. Those I've disable with other compile options when building on RHEL3, but reducing the number of options I have to set manually and increasing the number of automatic checks with configure is helpful. Sometime over the next few days I'll submit a v2 of the patches with Eric's comments taken into account. > -- > brian m. carlson / brian with sandals: Houston, Texas, US > +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only > OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: XDL_FAST_HASH can be very slow
Patrick Reynolds writes: > The original xdl_hash_record is essentially DJB hash, which does a > multiplication, load, and xor for each byte of the input. Commit > 6942efc introduces an "XDL_FAST_HASH" version of the same function > that is clearly inspired by the DJB hash, but it does only one > multiplication, load, and xor for each 8 bytes of input -- i.e., fewer > loads, but also a lot less bit mixing. Less mixing means far more > collisions, leading to the performance problems with evil-icons. It's > not clear to me if the XDL_FAST_HASH version intended to match the > output of the DJB hash function, but it doesn't at all. Note that XDL_FAST_HASH is just a ripoff of the hashing scheme that Linus socially engineered on G+ around that time. I didn't do any of the hash genealogy that you did here, and it now shows. The orginal patches are linked from 6942efc (xdiff: load full words in the inner loop of xdl_hash_record, 2012-04-06): https://lkml.org/lkml/2012/3/2/452 https://lkml.org/lkml/2012/3/5/6 The code still exists: https://github.com/torvalds/linux/blob/master/fs/namei.c#L1678 > I have implemented two simpler possibilities, both of which fix the > problems diffing the evil-icons repository: I think it would be best to separate three goals here: 1. hash function throughput 2. quality of the hash values 3. avoiding collision attacks XDL_FAST_HASH was strictly an attempt to improve throughput, and fairly successful at that (6942efc (xdiff: load full words in the inner loop of xdl_hash_record, 2012-04-06) quotes an 8% improvement on 'git log -p'). You are now addressing quality. I have no idea how you ran into this, but if you are reworking things already, I think it would be good to also randomize whatever hash you put in so as to give some measure of protection against collision attacks. > 1. An XDL_FAST_HASH implementation that matches the output of the DJB > hash exactly. Its performance is basically the same as DJB, because > the only thing is does differently is load 8 bytes at a time instead > of 1. It does all the same ALU operations as DJB. I don't think there's a point in having such a function, since it would mean a lot of code for no throughput gain. Let's just remove XDL_FAST_HASH and the original hashing scheme in favor of a better hash function. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: XDL_FAST_HASH can be very slow
I have been working with Peff on this and have more results to share. For background, xdl_hash_record is a hashing function, producing an unsigned long from an input string terminated by either a newline or the end of the mmap'd file. The original xdl_hash_record is essentially DJB hash, which does a multiplication, load, and xor for each byte of the input. Commit 6942efc introduces an "XDL_FAST_HASH" version of the same function that is clearly inspired by the DJB hash, but it does only one multiplication, load, and xor for each 8 bytes of input -- i.e., fewer loads, but also a lot less bit mixing. Less mixing means far more collisions, leading to the performance problems with evil-icons. It's not clear to me if the XDL_FAST_HASH version intended to match the output of the DJB hash function, but it doesn't at all. Peff has been experimenting with using two modern hash functions, FNV and Murmur3. In theory, these should produce fewer collisions than DJB, but in his measurements, they didn't run diff any faster than plain DJB. They do fix the evil-icons problem. I have implemented two simpler possibilities, both of which fix the problems diffing the evil-icons repository: 1. An XDL_FAST_HASH implementation that matches the output of the DJB hash exactly. Its performance is basically the same as DJB, because the only thing is does differently is load 8 bytes at a time instead of 1. It does all the same ALU operations as DJB. 2. Using (hash % prime_number) instead of (hash & ((1
make always rebuilds some targets
[redirected from off-list conversation] On Sun, Dec 21, 2014 at 01:57:55PM -0700, Tim Harper wrote: > I've noticed an issue in the builds script for 2.2.1 in which `make > all` seems to invalidate itself. The first and second invocation of > make all will compile all sources, but the 3rd finally recognizes the > cache and uses it. This negatively impacts my ability to use `make > strip`. Anyone else notice this? Can you be more specific about "invalidate" here? I'd guess from your mention of "make strip" that it's rebuilding the C code repeatedly. I don't see that, but I do notice two other problems running "make" repeatedly: 1. We also rebuild git-instaweb. It depends on gitweb, but that's a subdirectory target. I'm not sure if we should just drop this dependency completely, or if the intent is "make sure gitweb is built". I do not see anything in the build rule that actually depends on gitweb, though. I think it could have been dropped in ff2e2cd (git-instaweb: Simplify build dependency on gitweb, 2011-05-07). +cc Jakub for that. 2. The perl scripts all depend on GIT-BUILD-OPTIONS as of e204b00 (Makefile: have perl scripts depend on NO_PERL setting, 2014-11-18). This is a good thing in general, but we build GIT-BUILD-OPTIONS each time we run, which results in all of the perl scripts being rebuilt. It really needs the usual "see what it would look like, and do not update the file if it didn't change" behavior that we do for things like GIT-SCRIPT-DEFINES. The patch below fixes it for me, but we might be able to do something less messy. diff --git a/Makefile b/Makefile index 7482a4d..54f1026 100644 --- a/Makefile +++ b/Makefile @@ -2041,45 +2041,46 @@ GIT-LDFLAGS: FORCE # that runs GIT-BUILD-OPTIONS, and then again to protect it # and the first level quoting from the shell that runs "echo". GIT-BUILD-OPTIONS: FORCE - @echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@ - @echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@ - @echo DIFF=\''$(subst ','\'',$(subst ','\'',$(DIFF)))'\' >>$@ - @echo PYTHON_PATH=\''$(subst ','\'',$(PYTHON_PATH_SQ))'\' >>$@ - @echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@ - @echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@ - @echo USE_LIBPCRE=\''$(subst ','\'',$(subst ','\'',$(USE_LIBPCRE)))'\' >>$@ - @echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@ - @echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@ - @echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@ + @echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@+ + @echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@+ + @echo DIFF=\''$(subst ','\'',$(subst ','\'',$(DIFF)))'\' >>$@+ + @echo PYTHON_PATH=\''$(subst ','\'',$(PYTHON_PATH_SQ))'\' >>$@+ + @echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@+ + @echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@+ + @echo USE_LIBPCRE=\''$(subst ','\'',$(subst ','\'',$(USE_LIBPCRE)))'\' >>$@+ + @echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@+ + @echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+ + @echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst ','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+ ifdef TEST_OUTPUT_DIRECTORY - @echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@ + @echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+ endif ifdef GIT_TEST_OPTS - @echo GIT_TEST_OPTS=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_OPTS)))'\' >>$@ + @echo GIT_TEST_OPTS=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_OPTS)))'\' >>$@+ endif ifdef GIT_TEST_CMP - @echo GIT_TEST_CMP=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_CMP)))'\' >>$@ + @echo GIT_TEST_CMP=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_CMP)))'\' >>$@+ endif ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT - @echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@ + @echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+ endif - @echo NO_GETTEXT=\''$(subst ','\'',$(subst ','\'',$(NO_GETTEXT)))'\' >>$@ - @echo GETTEXT_POISON=\''$(subst ','\'',$(subst ','\'',$(GETTEXT_POISON)))'\' >>$@ + @echo NO_GETTEXT=\''$(subst ','\'',$(subst ','\'',$(NO_GETTEXT)))'\' >>$@+ + @echo GETTEXT_POISON=\''$(subst ','\'',$(subst ','\'',$(GETTEXT_POISON)))'\' >>$@+ ifdef GIT_PERF_REPEAT_COUNT - @echo GIT_PERF_REPEAT_COUNT=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_REPEAT_COUNT)))'\' >>$@ + @echo GIT_PERF_REPEAT_COUNT=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_REPEAT_COUNT)))'\' >>$@+ endif ifdef GIT_PERF_REPO - @echo GIT_PERF_REPO=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_REPO)))'\' >>$@ + @echo GIT
