Re: test &&-chain lint
Jeff King writes: > [+cc Jonathan, whose patch I apparently subconsciously copied] > > On Thu, Mar 19, 2015 at 10:08:51PM -0400, Jeff King wrote: > >> diff --git a/t/test-lib.sh b/t/test-lib.sh >> index c096778..02a03d5 100644 >> --- a/t/test-lib.sh >> +++ b/t/test-lib.sh >> @@ -524,6 +524,21 @@ test_eval_ () { >> test_run_ () { >> test_cleanup=: >> expecting_failure=$2 >> + >> +if test -n "$GIT_TEST_CHAIN_LINT"; then >> +# 117 is unlikely to match the exit code of >> +# another part of the chain >> +test_eval_ "(exit 117) && $1" >> +if test "$?" != 117; then >> +# all bets are off for continuing with other tests; >> +# we expected none of the rest of the test commands to >> +# run, but at least some did. Who knows what weird >> +# state we're in? Just bail, and the user can diagnose >> +# by running in --verbose mode >> +error "bug in the test script: broken &&-chain" >> +fi >> +fi >> + >> setup_malloc_check >> test_eval_ "$1" >> eval_ret=$? >> >> This turns up an appalling number of failures, but AFAICT they are all >> "real" in the sense that the &&-chains are broken. In some cases these >> are real, but in others the tests are of an older style where they did >> not expect some early commands to fail (and we would catch their bogus >> output if they did). E.g., in the patch below, I think the first one is >> a real potential bug, and the other two are mostly noise. I do not mind >> setting a rule and fixing all of them, though. >> >> I seem to recall people looked at doing this sort of lint a while ago, >> but we never ended up committing anything. I wonder if it was because of >> all of these "false positives". > > This turns out to be rather annoying to grep for in the list archives, > but I found at least one discussion: > > http://article.gmane.org/gmane.comp.version-control.git/235913 > > I don't know why we didn't follow it up then. Perhaps because the patch > there (which is rather similar to what I have above) was not > conditional, so whole chunks of the test suite needed fixing. There are > enough problems that we would probably want to do this conditionally, > fix them over time, and then finally flip the feature on by default. Hmmm, they do look similar and unfamiliar ;-) It happened while I was offline, it seems. Thanks for working on this---I think test-chain-lint should become one of the pre-acceptance test on my end when it gets applied. -- 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: test &&-chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)
On Thu, Mar 19, 2015 at 10:25:32PM -0400, Jeff King wrote: > > diff --git a/t/test-lib.sh b/t/test-lib.sh > > index c096778..02a03d5 100644 > > --- a/t/test-lib.sh > > +++ b/t/test-lib.sh > > @@ -524,6 +524,21 @@ test_eval_ () { > > test_run_ () { > > test_cleanup=: > > expecting_failure=$2 > > + > > + if test -n "$GIT_TEST_CHAIN_LINT"; then > > + # 117 is unlikely to match the exit code of > > + # another part of the chain > > + test_eval_ "(exit 117) && $1" > > + if test "$?" != 117; then > > + # all bets are off for continuing with other tests; > > + # we expected none of the rest of the test commands to > > + # run, but at least some did. Who knows what weird > > + # state we're in? Just bail, and the user can diagnose > > + # by running in --verbose mode > > + error "bug in the test script: broken &&-chain" > > + fi > > + fi > > + > > setup_malloc_check > > test_eval_ "$1" > > eval_ret=$? > > > > This turns up an appalling number of failures, but AFAICT they are all > > "real" in the sense that the &&-chains are broken. In some cases these > > are real, but in others the tests are of an older style where they did > > not expect some early commands to fail (and we would catch their bogus > > output if they did). E.g., in the patch below, I think the first one is > > a real potential bug, and the other two are mostly noise. I do not mind > > setting a rule and fixing all of them, though. FWIW, I have spent about a few hours wading through the errors, and am about 75% done. There are definitely some broken chains that were causing test results to be ignored (as opposed to just minor setup steps that we would not expect to fail). In most cases, the tests do passed. I have a few that I still need to examine more closely, but there may be some where there are actual test failures (but it's possible that I just screwed it up while fixing the &&-chaining). I hope to post something tonight, but I wanted to drop a note on the off chance that you were actively looking at it at the same time. -Peff -- 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: Why is "git fetch --prune" so much slower than "git remote prune"?
On 03/19/2015 08:24 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >>> Now that we have ref_transaction_*, I think if git-fetch fed all of the >>> deletes (along with the updates) into a single transaction, we would get >>> the same optimization for free. Maybe that is even part of some of the >>> pending ref_transaction work from Stefan or Michael (both cc'd). I >>> haven't kept up very well with what is cooking in pu. >> >> I am looking into this now. >> >> For pruning, we can't use a ref_transaction as it is currently >> implemented because it would fail if any of the reference deletions >> failed. But in this case I think if any deletions fail, we would prefer >> to emit a warning but keep going. > > I am not quite sure what you mean here. I agree with you if you > meant "we shouldn't fail the fetch only because 'fetch --prune' > failed to remove only one of the remote-tracking refs that are no > longer there" but that can easily be solved by the pruning phase > into a separate transaction. If you meant "we would rather remove > origin/{a,b} non-atomically when we noticed that origin/{a,b,c} are > all gone than leaving all three intact only because we failed to > remove origin/c for whatever reason", my knee-jerk reaction is "does > it make practical difference to the end user between these two?" > > What are the plausible cause of failing to prune unused > remote-tracking refs? I wasn't proposing to combine the updates with the prunes in a single transaction. (Though now that I think about it, when "fetch --atomic" is specified it wouldn't be so crazy. It would also have the virtue of allowing the reference transaction code to deal with any D/F conflicts between references being added and references being deleted.) What I meant is the second thing you mentioned, namely that currently we prune each reference on a "best-effort" basis and wouldn't skip *all* prunes just because one failed. But you raise an interesting question: what could cause a failure to prune an unused remote-tracking ref (i.e. if all pruning is done in a single transaction)? * I think the most likely reason for a failure would be a lock conflict with another process. We don't retry lock attempts [1], so this would cause the whole transaction to fail. This could happen, for example, if the user launches two "fetch --prune" operations at the same time, or runs "pack-refs" while fetching [2]. * It is *not* a failure if another process deletes or updates the reference before we get to it, because we don't provide an "old" value of the reference for a pre-check. * I haven't verified this, but I can imagine it could fail if another process deletes the reference (say refs/remotes/origin/foo/bar) and adds another reference that would D/F conflict with it (say refs/remotes/origin/foo) while we are working, because our attempt to create refs/remotes/origin/foo/bar.lock would fail. * A repository problem, like for example wrong file permissions somewhere in the loose refs directory, could prevent us from being able to create the lockfile or delete the loose reference file. The lock conflict scenario is the only one that really worries me. But I don't think it is *so* hard to keep the current "best-effort" behavior. I am working on a function delete_refs() for the reference API that deletes a bunch of references on a "best effort" basis, reporting per-reference errors for any deletions that fail. Longer term, we could add something like a REFS_NON_ATOMIC flag to the refs_transaction_update() functions, which allows the rest of the transaction to proceed even if that particular update fails. Michael [1] I hope to submit a patch to make it possible to retry lock acquisition with a specified timeout. This should help in scenarios like this. [2] Maybe a careful analysis or a slight change to our locking protocol could prove that the only lock acquisition that can fail is the one on packed-refs, which would cause all of the deletes to fail anyway. In that case per-reference failures would cease to be a worry. -- Michael Haggerty mhag...@alum.mit.edu -- 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
A git hook that does git cherry-pick and push automatically
So today I had a shocking moment while I was doing my cherry-pick, after I performed all the pre-checkin duties (the usual build the code, run the test to make sure the cherry-pick infact works), I found out that my original commit was already cherry-picked, then I found out someone in engineering make the decision to do an automate cherry-pick from that branch to another so he added a git hook to run do cherry-pick and push on every commit, yes, a simple cherry-pick then push; mind you, these are not feature / dev branch, these are release branches, both of them. Then after I came back from the shock, made a big protest about how this is the worst thing I have seen and I will never live with it, and that's why "git cherry-pick" and "git push" are 2 separate commands, as any reasonable developer, you do your very best to ensure you are not pushing something that is fundamentally broken; however for the life of me and talk these few people into senses. So, I am sending this to seek for some expert advice how I can drive some sense into these people so they don't screw up my life as an developer. Regards, Desperate developer, Ray. -- 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: test &&-chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)
[+cc Jonathan, whose patch I apparently subconsciously copied] On Thu, Mar 19, 2015 at 10:08:51PM -0400, Jeff King wrote: > diff --git a/t/test-lib.sh b/t/test-lib.sh > index c096778..02a03d5 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -524,6 +524,21 @@ test_eval_ () { > test_run_ () { > test_cleanup=: > expecting_failure=$2 > + > + if test -n "$GIT_TEST_CHAIN_LINT"; then > + # 117 is unlikely to match the exit code of > + # another part of the chain > + test_eval_ "(exit 117) && $1" > + if test "$?" != 117; then > + # all bets are off for continuing with other tests; > + # we expected none of the rest of the test commands to > + # run, but at least some did. Who knows what weird > + # state we're in? Just bail, and the user can diagnose > + # by running in --verbose mode > + error "bug in the test script: broken &&-chain" > + fi > + fi > + > setup_malloc_check > test_eval_ "$1" > eval_ret=$? > > This turns up an appalling number of failures, but AFAICT they are all > "real" in the sense that the &&-chains are broken. In some cases these > are real, but in others the tests are of an older style where they did > not expect some early commands to fail (and we would catch their bogus > output if they did). E.g., in the patch below, I think the first one is > a real potential bug, and the other two are mostly noise. I do not mind > setting a rule and fixing all of them, though. > > I seem to recall people looked at doing this sort of lint a while ago, > but we never ended up committing anything. I wonder if it was because of > all of these "false positives". This turns out to be rather annoying to grep for in the list archives, but I found at least one discussion: http://article.gmane.org/gmane.comp.version-control.git/235913 I don't know why we didn't follow it up then. Perhaps because the patch there (which is rather similar to what I have above) was not conditional, so whole chunks of the test suite needed fixing. There are enough problems that we would probably want to do this conditionally, fix them over time, and then finally flip the feature on by default. -Peff -- 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
test &&-chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)
On Thu, Mar 19, 2015 at 09:37:12PM -0400, Eric Sunshine wrote: > > Thanks. I notice that a large number of broken &&-chains are on > > here-docs. I really wish you could put the && on the "EOF" line at the > > end of the here-doc. I understand _why_ that this not the case, but > > mentally it is where I want to type it, and I obviously sometimes fail > > to go back and fix it. I don't think there's a better solution in POSIX > > sh, though. > > I wonder if test-lint could be enhanced to detect this sort of problem? That would be nice, but it's complicated. A naive: diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl index b170cbc..3a6d8d8 100755 --- a/t/check-non-portable-shell.pl +++ b/t/check-non-portable-shell.pl @@ -22,6 +22,7 @@ while (<>) { /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)'; /\btest\s+[^=]*==/ and err '"test a == b" is not portable (please use =)'; /\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (please use FOO=bar && export FOO)'; + / <<-?.?EOF(.*)/ && $1 !~ /&&/ and err 'here-doc with broken &&-chain'; # this resets our $. for each file close ARGV if eof; } yields quite a few false positives, because of course we don't know which are meant to be at the end of the chain and which are not. And finding that out is tough. We'd have to actually parse to the end of the here-doc ourselves, then see if it was the end of the test_expect block. I think it would be simpler to ask the shell to check this for us, like: diff --git a/t/test-lib.sh b/t/test-lib.sh index c096778..02a03d5 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -524,6 +524,21 @@ test_eval_ () { test_run_ () { test_cleanup=: expecting_failure=$2 + + if test -n "$GIT_TEST_CHAIN_LINT"; then + # 117 is unlikely to match the exit code of + # another part of the chain + test_eval_ "(exit 117) && $1" + if test "$?" != 117; then + # all bets are off for continuing with other tests; + # we expected none of the rest of the test commands to + # run, but at least some did. Who knows what weird + # state we're in? Just bail, and the user can diagnose + # by running in --verbose mode + error "bug in the test script: broken &&-chain" + fi + fi + setup_malloc_check test_eval_ "$1" eval_ret=$? This turns up an appalling number of failures, but AFAICT they are all "real" in the sense that the &&-chains are broken. In some cases these are real, but in others the tests are of an older style where they did not expect some early commands to fail (and we would catch their bogus output if they did). E.g., in the patch below, I think the first one is a real potential bug, and the other two are mostly noise. I do not mind setting a rule and fixing all of them, though. I seem to recall people looked at doing this sort of lint a while ago, but we never ended up committing anything. I wonder if it was because of all of these "false positives". diff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh index 6d3b828..62fce10 100755 --- a/t/t3010-ls-files-killed-modified.sh +++ b/t/t3010-ls-files-killed-modified.sh @@ -62,7 +62,7 @@ test_expect_success 'git update-index --add to add various paths.' ' cd submod$i && git commit --allow-empty -m "empty $i" ) || break done && - git update-index --add submod[12] + git update-index --add submod[12] && ( cd submod1 && git commit --allow-empty -m "empty 1 (updated)" @@ -99,12 +99,12 @@ test_expect_success 'git ls-files -k to show killed files.' ' ' test_expect_success 'git ls-files -k output (w/o icase)' ' - git ls-files -k >.output + git ls-files -k >.output && test_cmp .expected .output ' test_expect_success 'git ls-files -k output (w/ icase)' ' - git -c core.ignorecase=true ls-files -k >.output + git -c core.ignorecase=true ls-files -k >.output && test_cmp .expected .output ' -Peff -- 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/5] t5312: test object deletion code paths in a corrupted repository
On Thu, Mar 19, 2015 at 9:32 PM, Jeff King wrote: > On Thu, Mar 19, 2015 at 09:16:52PM -0400, Eric Sunshine wrote: > >> > --- /dev/null >> > +++ b/t/t5312-prune-corruption.sh >> > @@ -0,0 +1,104 @@ >> > +# we do not want to count on running pack-refs to >> > +# actually pack it, as it is perfectly reasonable to >> > +# skip processing a broken ref >> > +test_expect_success 'create packed-refs file with broken ref' ' >> > + rm -f .git/refs/heads/master && >> > + cat >.git/packed-refs <<-EOF >> >> Broken &&-chain. > > Thanks. I notice that a large number of broken &&-chains are on > here-docs. I really wish you could put the && on the "EOF" line at the > end of the here-doc. I understand _why_ that this not the case, but > mentally it is where I want to type it, and I obviously sometimes fail > to go back and fix it. I don't think there's a better solution in POSIX > sh, though. I wonder if test-lint could be enhanced to detect this sort of problem? -- 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/5] t5312: test object deletion code paths in a corrupted repository
On Thu, Mar 19, 2015 at 09:16:52PM -0400, Eric Sunshine wrote: > > --- /dev/null > > +++ b/t/t5312-prune-corruption.sh > > @@ -0,0 +1,104 @@ > > +# we do not want to count on running pack-refs to > > +# actually pack it, as it is perfectly reasonable to > > +# skip processing a broken ref > > +test_expect_success 'create packed-refs file with broken ref' ' > > + rm -f .git/refs/heads/master && > > + cat >.git/packed-refs <<-EOF > > Broken &&-chain. Thanks. I notice that a large number of broken &&-chains are on here-docs. I really wish you could put the && on the "EOF" line at the end of the here-doc. I understand _why_ that this not the case, but mentally it is where I want to type it, and I obviously sometimes fail to go back and fix it. I don't think there's a better solution in POSIX sh, though. -Peff -- 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/5] refs.c: drop curate_packed_refs
On Tue, Mar 17, 2015 at 3:31 AM, Jeff King wrote: > When we delete a ref, we have to rewrite the entire > packed-refs file. We take this opportunity to "curate" the > packed-refs file and drop any entries that are crufty or > broken. > > Dropping broken entries (e.g., with bogus names, or ones > that point to missing objects) is actively a bad idea, as it > means that we lose any notion that the data was there in the > first place. Aside from the general hackiness that we might > lose any information about ref "foo" while deleting an > unrelated ref "bar", this may seriously hamper any attempts > by the user at recovering from the corruption in "foo". > > They will lose the sha1 and name of "foo"; the exact pointer > may still be useful even if they recover missing objects > from a different copy of the repository. But worse, once the > ref is gone, there is no trace of the corruption. A > follow-up "git prune" may delete objects, even though it > would otherwise bail when seeing corruption. > > We could just drop the "broken" bits from > curate_packed_refs, and continue to drop the "crufty" bits: > refs whose loose counterpart exists in the filesystem. This > is not wrong to do, and it does have the advantage that we > may write out a slightly smaller packed-refs file. But it > has two disadvantages: > > 1. It is a potential source of races or mistakes with > respect to these refs that are otherwise unrelated to > the operation. To my knowledge, there aren't any active > problems in this area, but it seems like an unnecessary > risk. > > 2. We have to spend time looking up the matching loose > refs for every item in the packed-refs file. If you > have a large number of packed refs that do not change, > that outweights the benefit from writing out a smaller s/outweights/outweighs/ > packed-refs file (it doesn't get smaller, and you do a > bunch of directory traversal to find that out). > > Signed-off-by: Jeff King -- 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/5] t5312: test object deletion code paths in a corrupted repository
On Tue, Mar 17, 2015 at 3:28 AM, Jeff King wrote: > When we are doing a destructive operation like "git prune", > we want to be extra careful that the set of reachable tips > we compute is valid. If there is any corruption or oddity, > we are better off aborting the operation and letting the > user figure things out rather than plowing ahead and > possibly deleting some data that cannot be recovered. > > Signed-off-by: Jeff King > --- > diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh > new file mode 100755 > index 000..167031e > --- /dev/null > +++ b/t/t5312-prune-corruption.sh > @@ -0,0 +1,104 @@ > +# we do not want to count on running pack-refs to > +# actually pack it, as it is perfectly reasonable to > +# skip processing a broken ref > +test_expect_success 'create packed-refs file with broken ref' ' > + rm -f .git/refs/heads/master && > + cat >.git/packed-refs <<-EOF Broken &&-chain. > + $missing refs/heads/master > + $recoverable refs/heads/other > + EOF > + echo $missing >expect && > + git rev-parse refs/heads/master >actual && > + test_cmp expect actual > +' -- 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 for Windows 1.9.5.msysgit.1
On Fri, Mar 20, 2015 at 12:03:43AM +0100, Thomas Braun wrote: > Hi, > > the Git for Windows team just released the first maintenance release of > the Windows-specific installers for git 1.9.5. is it expected that there is no corresponding release on https://github.com/git-for-windows/git/releases ? 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, RFC] checkout: Attempt to checkout submodules
On Thu, Mar 19, 2015 at 02:15:19PM -0700, Junio C Hamano wrote: > Trevor Saunders writes: > > > On one hand it seems kind of user hostile to just toss out any changes > > in the submodule that are uncommitted, on the other for any other path > > it would seem weird to have git checkout trigger rebasing or merging. > > I think that is exactly why we do not do anything in this codepath. yeah, and not only is it weird, but git diff will still report that there's a difference which I imagine people will find strange. > I have a feeling that an optional feature that allows "git submodule > update" to happen automatically from this codepath might be > acceptable by the submodule folks, and they might even say it does > not even have to be optional but should be enabled by default. ok, that seems fairly reasonable. I do kind of wonder though if it shouldn't be 'git submodule update --checkout' but that would get us kind of back to where we started. I guess since the default is checkout if you set the pref then you can be assumed to have some amount of idea what your doing. > But I do not think it would fly well to unconditionally run > "checkout -f" here. agreed Trev > -- > 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 v4] rev-list: refuse --first-parent combined with --bisect
From: "Junio C Hamano" Sent: Tuesday, March 17, 2015 6:33 PM Christian Couder writes: On Mon, Mar 16, 2015 at 10:05 PM, Junio C Hamano wrote: However, you can say "git bisect bad " (and "git bisect good " for that matter) on a rev that is unrelated to what the current bisection state is. E.g. after you mark the child of 8 as "bad", the bisected graph would become G...1---2---3---4---6---8---B and you would be offered to test somewhere in the middle, say, 4. But it is perfectly OK for you to respond with "git bisect bad 7", if you know 7 is bad. I _think_ the current code blindly overwrites the "bad" pointer, making the bisection state into this graph if you do so. G...1---2---3---4 \ 5---B Yes, we keep only one "bad" pointer. This is very suboptimal. The side branch 4-to-7 could be much longer than the original trunk 4-to-the-tip, in which case we would have made the suspect space _larger_, not smaller. Yes, but the user is supposed to not change the "bad" pointer for no good reason. That is irrelevant, no? Nobody is questioning that the user is supposed to judge if a commit is "good" or "bad" correctly. [...] and/or we could make "git bisect bad" accept any number of bad commitishs. Yes, that is exactly what I meant. The way I understand the Philip's point is that the user may have a-priori knowledge that a breakage from the same cause appears in both tips of these branches. Just to clarify; my initial query followed on from the way Junio had described it with having two tips which were known bad. I hadn't been aware of how the bisect worked on a DAG, so I wanted to fully understand Junio's comment regarding the expectation of a clean jump to commit 4 (i.e. shouldn't we test commit 4 before assuming it's actually bad). I was quite happy with a bisect of a linear list, but was unsure about how Git dissected DAGs. I can easily see cases in more complicated product branching where users report intermittent operation for various product variants (especially if modular) and one wants to seek out those commits that introduced the behavious (which is typically some racy condition - otherwise it would be deterministic). Given Junio's explantion with the two bad commits (on different legs) I'd sort of assumed it could be both user given, or algorithmically determined as part of the bisect. In such a case, we can start bisection after marking the merge-base of two 'bad' commits, e.g. 4 in the illustration in the message you are responding to, instead of including 5, 6, and 8 in the suspect set. You need to be careful, though. An obvious pitfall is what you should do when there is a criss-cross merge. You end up with possibly two (or more) merges being marked as the source of the bad behaviour, especially when racy ;-) Thanks. -- Hope that helps. Philip -- 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
Sparse checkout not working as expected (colons in filenames on Windows)
Hi, I was expecting that sparse checkout could be used to avoid the checking out, by git, of files which have colons in their name into the worktree when on Windows. Yue Lin Ho reported on the Msygit list [1] that he had a repo where there was already committed a file with a colon in it's name, which was a needed file and had been committed by a Linux user. The problem was how to work with that repo on a Windows box where such a file is prohibited to exist on the FS (hence the expectation that a sparse checkout would suffice). Yue has created a short test repo [2] Even after getting the pathspec escaping right, I still haven't been able to make this expected behaviour work [3]. Am I wrong to expect that sparse checkout (and the skip-worktree bit) can be used to avoid files with undesirable filenames hitting the OS's file system? If it should be OK, what's the correct recipe? -- Philip [1] https://groups.google.com/forum/?hl=en_US?hl%3Den#!topic/msysgit/D4HcHRpxPgU "How to play around with the filename with colon on Windows?" [2] Test repo https://github.com/t-pascal/tortoisegit-colons [3] test sequence:: $ mkdir colons && cd colons $ git clone -n https://github.com/t-pascal/tortoisegit-colons $ cd tortoisegit-colons/ $ git config core.sparseCheckout true $ cat .git/info/sparse-checkout # external editor /* !ifcfg-eth0\:0 $ git update-index --skip-worktree -- ifcfg-eth0\:0 Ignoring path ifcfg-eth0:0 $ git checkout -b test 7f35d34bc6160cc # tip commit, we are still unborn! error: Invalid path 'ifcfg-eth0:0 D ifcfg-eth0:0 Switched to a new branch 'test' -- 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
Git for Windows 1.9.5.msysgit.1
Hi, the Git for Windows team just released the first maintenance release of the Windows-specific installers for git 1.9.5. It can be downloaded from the usual place [1] and I also attached some (although non-gpg-signed) SHA sums [2]. New Features - Comes with Git 1.9.5 plus Windows-specific patches. - Make vimdiff usable with git mergetool. Security Updates - Mingw-openssl to 0.9.8zf and msys-openssl to 1.0.1m - Bash to 3.1.23(6) - Curl to 7.41.0 Bugfixes - ssh-agent: only ask for password if not already loaded - Reenable perl debugging ("perl -de 1" possible again) - Set icon background color for Windows 8 tiles - poll: honor the timeout on Win32 - For git.exe alone, use the same HOME directory fallback mechanism as /etc/profile Have phun, Thomas [1]: https://github.com/msysgit/msysgit/releases/download/Git-1.9.5-preview20150319/Git-1.9.5-preview20150319.exe [2]: SHA1(Git-1.9.5-preview20150319.exe)= a8658bae0de8c8d3e40aa97a236a4fcf81de50df SHA1(PortableGit-1.9.5-preview20150319.7z)= 44e7f249797af9a3833b88e17575bcbf22c282df SHA1(msysGit-netinstall-1.9.5-preview20150319.exe)= 60b73db7959fb841b7d29286608e2333f429ca85 -- 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 v5] rev-list: refuse --first-parent combined with --bisect
Kevin Daudt writes: > rev-list --bisect is used by git bisect, but never together with > --first-parent. Because rev-list --bisect together with --first-parent > is not handled currently, and even leads to segfaults, refuse to use > both options together. > > Because this is not supported, it makes little sense to use git log > --bisect --first parent either, because refs/heads/bad is not limited to > the first parent chain. > > Helped-by: Junio C. Hamano > Helped-by: Eric Sunshine > Signed-off-by: Kevin Daudt > --- Thanks; will queue. -- 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: [v3 PATCH 2/2] reset: add tests for git reset -
On Wed, Mar 18, 2015 at 02:05:09PM +0530, Sundararajan R wrote: > The failure case which occurs on teaching git is taught the '-' shorthand > is when there exists no branch pointed to by '@{-1}'. But, if there is a file > named - in the working tree, the user can be unambiguously assumed to be > referring to it while issuing this command. > The first line is hard to read. I think the "is taught" part is redundant. -- 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 v5] rev-list: refuse --first-parent combined with --bisect
rev-list --bisect is used by git bisect, but never together with --first-parent. Because rev-list --bisect together with --first-parent is not handled currently, and even leads to segfaults, refuse to use both options together. Because this is not supported, it makes little sense to use git log --bisect --first parent either, because refs/heads/bad is not limited to the first parent chain. Helped-by: Junio C. Hamano Helped-by: Eric Sunshine Signed-off-by: Kevin Daudt --- Updates since v4: * Not only refusing rev-list --bisect --first-parent, but also log --bisect --first-parent Documentation/rev-list-options.txt | 7 --- revision.c | 3 +++ t/t6000-rev-list-misc.sh | 4 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 4ed8587..e2de789 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit). because merges into a topic branch tend to be only about adjusting to updated upstream from time to time, and this option allows you to ignore the individual commits - brought in to your history by such a merge. + brought in to your history by such a merge. Cannot be + combined with --bisect. --not:: Reverses the meaning of the '{caret}' prefix (or lack thereof) @@ -185,7 +186,7 @@ ifndef::git-rev-list[] Pretend as if the bad bisection ref `refs/bisect/bad` was listed and as if it was followed by `--not` and the good bisection refs `refs/bisect/good-*` on the command - line. + line. Cannot be combined with --first-parent. endif::git-rev-list[] --stdin:: @@ -566,7 +567,7 @@ outputs 'midpoint', the output of the two commands would be of roughly the same length. Finding the change which introduces a regression is thus reduced to a binary search: repeatedly generate and test new 'midpoint's until the commit chain is of length -one. +one. Cannot be combined with --first-parent. --bisect-vars:: This calculates the same as `--bisect`, except that refs in diff --git a/revision.c b/revision.c index 66520c6..ed3f6e9 100644 --- a/revision.c +++ b/revision.c @@ -2342,6 +2342,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s if (!revs->reflog_info && revs->grep_filter.use_reflog_filter) die("cannot use --grep-reflog without --walk-reflogs"); + if (revs->first_parent_only && revs->bisect) + die(_("--first-parent is incompatible with --bisect")); + return left; } diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh index 2602086..1f58b46 100755 --- a/t/t6000-rev-list-misc.sh +++ b/t/t6000-rev-list-misc.sh @@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' ' test_cmp expect actual ' +test_expect_success '--bisect and --first-parent can not be combined' ' + test_must_fail git rev-list --bisect --first-parent HEAD +' + test_done -- 2.3.2 -- 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=p4.py rebase now honor's client spec
On Thu, Mar 19, 2015 at 12:28:09PM +, Sam Bishop wrote: > When using the git-p4.py script, I found that if I used a client spec when > cloning out a perforce repository, and then using a git-p4.py rebase, that > the rebase command would be using the current perforce client spec, > instead of the one used when doing the initial clone. My proposed patch > causes the rebase command to switch to the perforce client spec used when > doing the initial git-p4.py clone. > > This email and any attachments may contain confidential and > proprietary information of Blackboard that is for the sole use of the > intended recipient. If you are not the intended recipient, disclosure, > copying, re-distribution or other use of any of this information is > strictly prohibited. Please immediately notify the sender and delete > this transmission if you received this email in error. You might want to read Documentation/SubmittingPatches for information on patch submission procedures. Most notably, you'll need to sign-off your work to indicate that you can submit it under the appropriate license. The confidentiality notice above is not only inappropriate for a public mailing list, it discourages people from even looking at your patch, as you just claimed it was confidential and proprietary and nobody wants to be sued. -- 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 signature.asc Description: Digital signature
Re: Need help deciding between subtree and submodule
On Wed, Mar 18, 2015 at 6:04 PM, Doug Kelly wrote: > On Wed, Mar 18, 2015 at 3:20 AM, Chris Packham > wrote: >> My $0.02 based on $dayjob >> >> (disclaimer I've never used subtree) >> >> On Wed, Mar 18, 2015 at 11:14 AM, Robert Dailey >> wrote: >>> At my workplace, the team is using Atlassian Stash + git >>> >>> We have a "Core" library that is our common code between various >>> projects. To avoid a single monolithic repository and to allow our >>> apps and tools to be modularized into their own repos, I have >>> considered moving Core to a subtree or submodule. > > $DAYJOB has actually tried both... with varying levels of success. As > you note, subtree looks wonderful from a user perspective, but behind > the scenes, it does have issues. In our case, subtree support was > modified into Gerrit, and this became cumbersome and difficult to > maintain (which is the reason we eventually dropped support for > subtree). Submodules have more of a labor-intensive aspect, but > are far more obvious about what actions have been taken (IMHO). > Either way, both our developers' needs were satisfied: the code was > tracked cleanly, and there wasn't a configuration mismatch where > a dependency was able to change versions without implicit direction. > >> >> Our environment is slightly different. Our projects are made up >> entirely of submodules, we don't embed submodules within a repo with >> actual code (side note: I know syslog-ng does so it might be worth >> having a look around there). >> >> Day to day development is done at the submodule level. A developer >> working on a particular feature is generally only touching one repo >> notwithstanding a little bit of to-and-fro as they work on the UI >> aspects. When changes do touch multiple submodules the pushes can >> generally be ordered in a sane manner. Things get a little complicated >> when there are interdependent changes, then those pushes require >> co-operation between submodule owners. > > We've done both (all of the above? a hybrid approach?)... We've gone so > far to create 30 modules for every conceivable component, then tried to > work that way with submodule, and our developers quickly revolted as it > became too much of a maintenance burden. The other direction (with > hugely monolithic code) is also problematic since the module boundaries > become blurred. For us, usually cooperation between modules isn't so > difficult, but the problem comes about when attempting to merge the > changes. Sometimes, it can take significant effort to ensure conflict-free > merges (going so far as to require "merge lock" emails to ask other > developers to hold off on merging commits until the change lands > completely and the project is stable). > >> >> The key to making this work is our build system. It is the thing that >> updates the project repo. After a successful build for all targets (we >> hope to add unit/regression tests one day) the submodules sha1s are >> updated and a new baseline (to borrow a clearcase term) is published. >> Developers can do "git pull && git submodule update" to get the latest >> stable baseline, but they can also run git pull in a submodule if they >> want to be on the bleeding edge. >> >>> I tried subtree and this is definitely far more transparent and simple >>> to the team (simplicity is very important), however I notice it has >>> problems with unnecessary conflicts when you do not do `git subtree >>> push` for each `git subtree pull`. This is unnecessary overhead and >>> complicates the log graph which I don't like. >>> >>> Submodule functionally works but it is complicated. We make heavy use >>> of pull requests for code reviews (they are required due to company >>> policy). Instead of a pull request being atomic and containing any app >>> changes + accompanying Core changes, we now need to create two pull >>> requests and manage them in proper order. Things also become more >>> difficult when branching. All around it just feels like submodule >>> would interfere and add more administration overhead on a day to day >>> basis, affecting productivity. >> >> We do have policies around review etc. With submodules it does >> sometimes require engaging owners/reviewers from multiple >> repositories. Tools like Gerrit can help, particularly where multiple >> changes and reviewers are involved. > > Conflicts are definitely going to be a difficulty with either subtree or > submodule (if multiple users could be changing the submodule), but > if you have additional tools, such as Gerrit to look out for, submodule > is the way to go since subtrees aren't supported within Gerrit. (Other > tools may support it better: I'm honestly not sure?) That would be > my one word of caution: I don't know how well Stash supports subtree. > > You are absolutely correct about the difficulty of integrating submodule > pull requests taking two steps. This was an issue we worked hard > to mitigate here, but at the end of the day, the work is necess
Re: [PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME
On Wed, Mar 18, 2015 at 3:26 PM, Eric Sunshine wrote: > On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan wrote: >> t0302 now tests git-credential-store's support for the XDG user-specific >> configuration file $XDG_CONFIG_HOME/git/credentials. Specifically: >> --- >> >> The previous version can be found at [1]. >> >> [1] >> http://thread.gmane.org/gmane.comp.version-control.git/265305/focus=265308 >> >> * Merge related, but previously separate, tests together in order to >> make the test suite easier to understand. >> >> * Instead of setting/unsetting XDG_CONFIG_HOME in separate tests, set >> it, and unset it immediately before and after "helper_test store" is >> called in order to make it localized to only the command that it >> should affect. >> >> * Add test, previously missing, to check that only the home credentials >> file is written to if both the xdg and home files exist. >> >> * Correct mislabelling of "home-user"/"home-pass" to the proper >> "xdg-user"/"xdg-pass". >> >> * Use "rm -f" instead of "test_might_fail rm". > > This round looks much better. Thanks. > > Most of the comments below are just nit-picky, with one or two genuine > (minor) issues. I should add that the nit-picky items are not necessarily actionable. As the person doing the actual work, it's okay if you disagree and feel that they are not worth the effort of addressing. (The genuine issues, on the other hand, ought to be addressed.) -- 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/5] t5312: test object deletion code paths in a corrupted repository
On Thu, Mar 19, 2015 at 02:49:37PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > So I'm inclined to leave it (we do confirm with the rev-parse call at > > the end of the setup that our packed-refs file is working) unless you > > feel strongly. If you do, I'd rather go the route of sticking each > > corruption in its own separate sub-repo. > > No, I don't feel strongly either way---otherwise I wouldn't be > wondering if it makes a difference, but explaining why hand-crafting > is a bad idea (or the other way around). And here I thought you were just being polite. ;) -Peff -- 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/5] refs: introduce a "ref paranoia" flag
On Thu, Mar 19, 2015 at 02:31:39PM -0700, Junio C Hamano wrote: > > We do have to have this variable cross some process boundaries. Only > > "repack" knows whether to turn on paranoia, but "pack-objects" is the > > one that must act on it. > > > > Or is there something else I'm missing? > > In general, I do not like the pattern of program A setting an > environment only because it wants to tell program B it spawns > something, because we cannot tell program B that the environment > should be dropped when it calls something else (e.g. user defined > hooks, merge drivers, textconvs, etc.) to prevent end user > invocation of Git commands from honoring it. Setting GIT_DIR or > GIT_WORK_TREE and having to know when to drop them is not very > pleasant, for example. > > I think the use of this pattern is OK in this codepath in which > repack calls pack-objects, and I think I can be persuaded to buy an > argument that there is no harm, or it may even be a good thing, to > run such an end-user program under paranoia mode, if pack-objects > wants to spawn one. Ah, I see. Yeah, I consider that to be a _feature_ for REF_PARANOIA here. If you are running receive-pack under REF_PARANOIA, for example, you would want your pre-receive hooks to use the same rules as the rest of receive-pack. If there is a misfeature, it is that we turn on REF_PARANOIA automatically behind the user's back in some cases, which could surprise them if we call through to custom code. But as you note, I think this code path is OK, because we don't spawn anything else from pack-objects (and if we did, arguably it is OK because our caller told us we are doing something dangerous; but we would have to evaluate that case-by-case, I would think). -Peff -- 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/5] t5312: test object deletion code paths in a corrupted repository
Jeff King writes: > So I'm inclined to leave it (we do confirm with the rev-parse call at > the end of the setup that our packed-refs file is working) unless you > feel strongly. If you do, I'd rather go the route of sticking each > corruption in its own separate sub-repo. No, I don't feel strongly either way---otherwise I wouldn't be wondering if it makes a difference, but explaining why hand-crafting is a bad idea (or the other way around). 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/5] t5312: test object deletion code paths in a corrupted repository
On Thu, Mar 19, 2015 at 02:23:25PM -0700, Junio C Hamano wrote: > Jeff King writes: > > >> A safer check may be to pack and then make it missing, I guess, but > >> I do not know if the difference matters. > > > > Yeah, I considered that. The trouble is that we are relying on the > > earlier setup that made the object go missing. We cannot pack the refs > > in the setup step, because the earlier tests are checking the loose-ref > > behavior. So we would have to actually restore the object, pack, and > > then re-delete it. > > Yes, "restore pack redelete" was what I had in mind when I wondered > such a sequence of extra steps is worth and the difference between > such an approach and an approach to use a hand-crafted packed-refs > file matters. I took a look at this. It turns out to be rather annoying, because we can't just restore $missing. The earlier tests may have deleted other random objects (like $recoverable) depending on whether or not they actually failed. So I'm inclined to leave it (we do confirm with the rev-parse call at the end of the setup that our packed-refs file is working) unless you feel strongly. If you do, I'd rather go the route of sticking each corruption in its own separate sub-repo. -Peff -- 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/5] refs: introduce a "ref paranoia" flag
Jeff King writes: >> > + if (ref_paranoia < 0) >> > + ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); >> > + if (ref_paranoia) >> > + data.flags |= DO_FOR_EACH_INCLUDE_BROKEN; >> >> I am not a big fan of proliferation of interfaces based on >> environment variables, but hopefully this is isolated enough to >> become an issue in the future. > > I'm not sure which part you don't like. > > We do have to have this variable cross some process boundaries. Only > "repack" knows whether to turn on paranoia, but "pack-objects" is the > one that must act on it. > > Or is there something else I'm missing? In general, I do not like the pattern of program A setting an environment only because it wants to tell program B it spawns something, because we cannot tell program B that the environment should be dropped when it calls something else (e.g. user defined hooks, merge drivers, textconvs, etc.) to prevent end user invocation of Git commands from honoring it. Setting GIT_DIR or GIT_WORK_TREE and having to know when to drop them is not very pleasant, for example. I think the use of this pattern is OK in this codepath in which repack calls pack-objects, and I think I can be persuaded to buy an argument that there is no harm, or it may even be a good thing, to run such an end-user program under paranoia mode, if pack-objects wants to spawn one. -- 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: submodule.$name.url is ignored during submodule update
Sorry, my bad. I thought 'git submodule sync' changes only 'submodule.$name.url' in main repository config, but it also changes the 'remote.origin.url' in submodule's config. I indeed ran 'git submodule sync', that's why the default url was used even though 'submodule.$name.url' had a different value. On Thu, Mar 19, 2015 at 2:16 PM, Dmitry Neverov wrote: > I want to use a custom url for both initial submodule clone and > submodule update. Git submodule man page states that if I run 'git > submodule init' and then change the 'submodule.$name.url' in the main > repository config, git will use this url instead of url in > .gitmodules. Git does use the custom url for initial submodule clone, > but doesn't use it when cloned submodule needs to be updated. Is that > by design? > > On Thu, Mar 19, 2015 at 2:09 PM, Doug Kelly wrote: >> On Thu, Mar 19, 2015 at 4:27 AM, Dmitry Neverov >> wrote: >>> Hi, >>> >>> I've noticed that the 'submodule.$name.url' config parameter from the >>> main repository is ignored when a submodule needs to be updated, the >>> submodule's 'remote.origin.url' is used instead. Is there any way to >>> customize the submodule url for both the initial clone and for >>> updates? >> >> That's what "git submodule sync" is for. It will synchronize the url >> in .gitmodules with >> to the remote.origin.url for each submodule. I'm not sure about the second >> part >> of your question: are you talking about using different URLs for the >> initial clone >> and updates? -- 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: Why is "git fetch --prune" so much slower than "git remote prune"?
On Thu, Mar 19, 2015 at 12:24:21PM -0700, Junio C Hamano wrote: > > For pruning, we can't use a ref_transaction as it is currently > > implemented because it would fail if any of the reference deletions > > failed. But in this case I think if any deletions fail, we would prefer > > to emit a warning but keep going. > > I am not quite sure what you mean here. I agree with you if you > meant "we shouldn't fail the fetch only because 'fetch --prune' > failed to remove only one of the remote-tracking refs that are no > longer there" but that can easily be solved by the pruning phase > into a separate transaction. If you meant "we would rather remove > origin/{a,b} non-atomically when we noticed that origin/{a,b,c} are > all gone than leaving all three intact only because we failed to > remove origin/c for whatever reason", my knee-jerk reaction is "does > it make practical difference to the end user between these two?" > > What are the plausible cause of failing to prune unused > remote-tracking refs? I had assumed earlier that Michael meant to use a single ref_transaction for all updates. Thinking on it more, that is probably a bad idea, as it makes fetch atomic in a user-visible way, whereas currently the updates are always per-ref (i.e., some may fail, but we let others succeed). I don't know if people actually care or not (certainly the exit code of fetch represents all of the refs, so it is not like you could say "eh, git-fetch return 1, but it probably got the ref I wanted" without parsing the human-readable output). If it is just a single atomic commit for all of the deletions, I agree it is less of a big deal. They are unlikely to fail, and when they do, you are not blocking the new refs from coming in. I think the ref_transaction does have some smarts to handle a case where we are updating "refs/foo/bar" while "refs/foo" exists but is deleted in the transaction. We switched to pruning before updating in 10a6cc8 (fetch --prune: Run prune before fetching, 2014-01-02), so it is a non-issue, but what is there now is technically racy[1], and it would have been nice to let the ref-storage code handle it. I guess we still can if we introduce a "git fetch --atomic" option. -Peff [1] http://article.gmane.org/gmane.comp.version-control.git/239519 -- 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/5] t5312: test object deletion code paths in a corrupted repository
Jeff King writes: >> A safer check may be to pack and then make it missing, I guess, but >> I do not know if the difference matters. > > Yeah, I considered that. The trouble is that we are relying on the > earlier setup that made the object go missing. We cannot pack the refs > in the setup step, because the earlier tests are checking the loose-ref > behavior. So we would have to actually restore the object, pack, and > then re-delete it. Yes, "restore pack redelete" was what I had in mind when I wondered such a sequence of extra steps is worth and the difference between such an approach and an approach to use a hand-crafted packed-refs file matters. -- 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 with Lader logic
On Thu, Mar 19, 2015 at 07:14:52AM +, Bharat Suvarna wrote: > Thanks all .. I will have a look. But could I just set this up on my laptop > and checking this works on system first before installing one of Git on server > Sure, that's no problem. Git happily runs just locally on your own machine and does not depend on a server. -- 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, RFC] checkout: Attempt to checkout submodules
Trevor Saunders writes: > On one hand it seems kind of user hostile to just toss out any changes > in the submodule that are uncommitted, on the other for any other path > it would seem weird to have git checkout trigger rebasing or merging. I think that is exactly why we do not do anything in this codepath. I have a feeling that an optional feature that allows "git submodule update" to happen automatically from this codepath might be acceptable by the submodule folks, and they might even say it does not even have to be optional but should be enabled by default. But I do not think it would fly well to unconditionally run "checkout -f" here. -- 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/4] filter_ref: avoid overwriting ref->old_sha1 with garbage
Jeff King writes: > I wonder if the thinking in the original was that it was our > responsibility here to make sure that ref->old_sha1 was filled in. I am reasonably sure that is the (perhaps mistaken) reasoning behind the use of old_sha1 as the second parameter to get_sha1_hex(). > It is > always filled in by the caller who gives us "sought", which makes sense > to me (this matches the rest of the "sought" entries, which come from > reading the remote's ref list, and of course must fill in old_sha1 from > that list). I see that sought is populated by reading the command line of "git fetch-pack", and for a 40-hex request ref->old_sha1 is already filled there, so I agree that it is redundant to try filling it in filter_refs(). Thanks. > fetch-pack.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fetch-pack.c b/fetch-pack.c > index 655ee64..058c258 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -544,10 +544,14 @@ static void filter_refs(struct fetch_pack_args *args, > /* Append unmatched requests to the list */ > if (allow_tip_sha1_in_want) { > for (i = 0; i < nr_sought; i++) { > + unsigned char sha1[20]; > + > ref = sought[i]; > if (ref->matched) > continue; > - if (get_sha1_hex(ref->name, ref->old_sha1)) > + if (get_sha1_hex(ref->name, sha1) || > + ref->name[40] != '\0' || > + hashcmp(sha1, ref->old_sha1)) > continue; > > ref->matched = 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: Slow git pushes: sitting 1 minute in pack-objects
On Thu, Mar 19, 2015 at 04:31:36PM -0400, Stephen Morton wrote: > > Hmm. The "push" process must feed the set of object boundaries to > > "pack-objects" so it knows what to pack (i.e., what we want to send, and > > what the other side has). > > > > 120,000 is an awfully large number of objects to be pass there, though. > > Does the repo you are pushing to by any chance have an extremely large > > number of refs (e.g., on the order of 120K tags)? > > No. There are on the order of 9,500 refs (mostly tags) but nowhere near 120k. I think you mentioned that it uses alternates to share objects with other repos. Does the repository (or repositories) pointed to by the alternates file have a large number of refs (especially distinct refs, as I think modern git will squelch duplicate sha1s). > It did _not_ happen in a new clone --a push took just 5s -- and I > think the culprit could have been "repack.writebitmaps=true". Although > I had thought writebitmaps was not originally enabled, I now suspect > that it was. Let me follow up on that first, before I recompile git > with your changes. It's certainly possible that bitmaps have an impact here. They should not contribute to the 120K objects being passed to pack-objects, but it's possible that size is a red herring (or possibly the number of objects is choking something in the bitmap code path that does not have problems otherwise). -Peff -- 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/5] refs: introduce a "ref paranoia" flag
On Thu, Mar 19, 2015 at 01:13:13PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > diff --git a/refs.c b/refs.c > > index e23542b..7f0e7be 100644 > > --- a/refs.c > > +++ b/refs.c > > @@ -1934,6 +1934,11 @@ static int do_for_each_ref(struct ref_cache *refs, > > const char *base, > > data.fn = fn; > > data.cb_data = cb_data; > > > > + if (ref_paranoia < 0) > > + ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); > > + if (ref_paranoia) > > + data.flags |= DO_FOR_EACH_INCLUDE_BROKEN; > > I am not a big fan of proliferation of interfaces based on > environment variables, but hopefully this is isolated enough to > become an issue in the future. I'm not sure which part you don't like. We do have to have this variable cross some process boundaries. Only "repack" knows whether to turn on paranoia, but "pack-objects" is the one that must act on it. For that case, we could add a "--ref-paranoia" flag to pack-objects. But there are also other cases where the _user_ might want to tell us to be paranoid (e.g., the upload-pack example I gave earlier). Adding --paranoia options to every command that might iterate, along with support for other programs that call them to pass the option through, seems like a large amount of maintenance burden for little benefit. We could add a git-global "git --ref-paranoia " option and leave the environment variable as an implementation detail. That makes it hard to turn on for server-side operations, though. git-daemon runs git-upload-pack without room for options, though I suppose you could run "git --ref-paranoia daemon". Smart-http is harder. I'm not sure Apache lets you add random arguments to CGI programs. Or is there something else I'm missing? -Peff -- 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/5] t5312: test object deletion code paths in a corrupted repository
On Thu, Mar 19, 2015 at 01:04:16PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > +test_expect_success 'create history with missing tip commit' ' > > + test_tick && git commit --allow-empty -m one && > > + recoverable=$(git rev-parse HEAD) && > > + git cat-file commit $recoverable >saved && > > + test_tick && git commit --allow-empty -m two && > > + missing=$(git rev-parse HEAD) && > > + # point HEAD elsewhere > > + git checkout $base && > > Could you spell this as "$base^0" (or "--detach") to clarify the > intention? I have been scraching my head for a few minutes just > now, trying to figure out what you are doing here. I _think_ you > wanted master to point at the missing "two" and wanted to make sure > all other refs (including HEAD) to point away from it. Yes, exactly. I've squashed in your suggestion and added a comment explaining it: diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh index 1001a69..1cdbd9f 100755 --- a/t/t5312-prune-corruption.sh +++ b/t/t5312-prune-corruption.sh @@ -50,14 +50,24 @@ test_expect_success 'clean up bogus ref' ' rm .git/refs/heads/bogus..name ' +# We create two new objects here, "one" and "two". Our +# master branch points to "two", which is deleted, +# corrupting the repository. But we'd like to make sure +# that the otherwise unreachable "one" is not pruned +# (since it is the user's best bet for recovering +# from the corruption). +# +# Note that we also point HEAD somewhere besides "two", +# as we want to make sure we test the case where we +# pick up the reference to "two" by iterating the refs, +# not by resolving HEAD. test_expect_success 'create history with missing tip commit' ' test_tick && git commit --allow-empty -m one && recoverable=$(git rev-parse HEAD) && git cat-file commit $recoverable >saved && test_tick && git commit --allow-empty -m two && missing=$(git rev-parse HEAD) && - # point HEAD elsewhere - git checkout $base && + git checkout --detach $base && rm .git/objects/$(echo $missing | sed "s,..,&/,") && test_must_fail git cat-file -e $missing ' > > +# we do not want to count on running pack-refs to > > +# actually pack it, as it is perfectly reasonable to > > +# skip processing a broken ref > > +test_expect_success 'create packed-refs file with broken ref' ' > > + rm -f .git/refs/heads/master && > > + cat >.git/packed-refs <<-EOF > > + $missing refs/heads/master > > + $recoverable refs/heads/other > > + EOF > > I do not know offhand if the lack of the pack-refs feature header > matters here; I assume it does not? It doesn't matter. We also do similarly gross things in other corruption-related tests, but I suspect if you git-blamed them all you would find that I am responsible. :) > A safer check may be to pack and then make it missing, I guess, but > I do not know if the difference matters. Yeah, I considered that. The trouble is that we are relying on the earlier setup that made the object go missing. We cannot pack the refs in the setup step, because the earlier tests are checking the loose-ref behavior. So we would have to actually restore the object, pack, and then re-delete it. Another option would be to restructure the whole test script to perform each individual corruption in its own sub-repo. I thought that would end up making things harder to understand due to the extra setup boilerplate, but it would make the tests less fragile with respect to each other (e.g., see the "clean up bogus ref" step which exists only to clean up our earlier corruption that could influence later tests). -Peff -- 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 4/4] fetch-pack: remove dead assignment to ref->new_sha1
In everything_local(), we used to assign the current ref's value found in ref->old_sha1 to ref->new_sha1 when we already have all the necessary objects to complete the history leading to that commit. This copying was broken at 49bb805e (Do not ask for objects known to be complete., 2005-10-19) and ever since we instead stuffed a random bytes in ref->new_sha1 here. No code complained or failed due to this breakage. It turns out that no code path that comes after this assignment even looks at ref->new_sha1 at all. - The only caller of everything_local(), do_fetch_pack(), returns this list of refs, whose element has bogus new_sha1 values, to its caller. It does not look at the elements itself, but does pass them to find_common, which looks only at the name and old_sha1 fields. - The only caller of do_fetch_pack(), fetch_pack(), returns this list to its caller. It does not look at the elements nor act on them. - One of the two callers of fetch_pack() is cmd_fetch_pack(), the top-level that implements "git fetch-pack". The only thing it looks at in the elements of the returned ref list is the old_sha1 and name fields. - The other caller of fetch_pack() is fetch_refs_via_pack() in the transport layer, which is a helper that implements "git fetch". It only cares about whether the returned list is empty (i.e. failed to fetch anything). Just drop the bogus assignment, that is not even necessary. The remote-tracking refs are updated based on a different list and not using the ref list being manipulated by this code path; the caller do_fetch_pack() created a copy of that real ref list and passed the copy down to this function, and modifying the elements here does not affect anything. Noticed-by: Kyle J. McKay Signed-off-by: Jeff King --- fetch-pack.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 84d5a66..48526aa 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -628,7 +628,6 @@ static int everything_local(struct fetch_pack_args *args, for (retval = 1, ref = *refs; ref ; ref = ref->next) { const unsigned char *remote = ref->old_sha1; - unsigned char local[20]; struct object *o; o = lookup_object(remote); @@ -641,8 +640,6 @@ static int everything_local(struct fetch_pack_args *args, ref->name); continue; } - - hashcpy(ref->new_sha1, local); if (!args->verbose) continue; fprintf(stderr, -- 2.3.3.520.g3cfbb5d -- 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 3/4] fetch_refs_via_pack: free extra copy of refs
When fetch_refs_via_pack calls fetch_pack(), we pass a list of refs to fetch, and the function returns either a copy of that list, with the fetched items filled in, or NULL. We check the return value to see whether the fetch was successful, but do not otherwise look at the copy, and simply leak it at the end of the function. Signed-off-by: Jeff King --- Without patch 2, this segfaults in t5516. :) transport.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/transport.c b/transport.c index 0694a7c..b1953b2 100644 --- a/transport.c +++ b/transport.c @@ -519,7 +519,7 @@ static int fetch_refs_via_pack(struct transport *transport, int nr_heads, struct ref **to_fetch) { struct git_transport_data *data = transport->data; - const struct ref *refs; + struct ref *refs; char *dest = xstrdup(transport->url); struct fetch_pack_args args; struct ref *refs_tmp = NULL; @@ -552,15 +552,17 @@ static int fetch_refs_via_pack(struct transport *transport, &transport->pack_lockfile); close(data->fd[0]); close(data->fd[1]); - if (finish_connect(data->conn)) + if (finish_connect(data->conn)) { + free_refs(refs); refs = NULL; + } data->conn = NULL; data->got_remote_heads = 0; data->options.self_contained_and_connected = args.self_contained_and_connected; free_refs(refs_tmp); - + free_refs(refs); free(dest); return (refs ? 0 : -1); } -- 2.3.3.520.g3cfbb5d -- 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/4] filter_ref: make a copy of extra "sought" entries
If the server supports allow_tip_sha1_in_want, we add any unmatched raw-sha1 entries in our "sought" list of refs to the list of refs we will ask the other side for. We do so by inserting the original "struct ref" directly into our list, rather than making a copy. This has several problems. The most minor problem is that one cannot ever free the resulting list; it contains structs that are copies of the remote refs (made earlier by fetch_pack) along with sought refs that are referenced elsewhere. But more importantly that we set the ref->next pointer to NULL, chopping off the remainder of any existing list that the ref was a part of. We get the set of "sought" refs in an array rather than a linked list, but that array is often in turn generated from a list. The test modification in t5516 demonstrates this. Rather than fetching just an exact sha1, we fetch that sha1 plus another ref: - we build a linked list of refs to fetch when do_fetch calls get_ref_map; the exact sha1 is first, followed by the named ref ("refs/heads/extra" in this case). - we pass that linked list to transport_fetch_ref, which squashes it into an array of pointers - that array goes to fetch_pack, which calls filter_ref. There we generate the want list from a mix of what the remote side has advertised, and the "sought" entry for the exact sha1. We set the sought entry's "next" pointer to NULL. - after we return from transport_fetch_refs, we then try to update the refs by following the linked list. But our list is now truncated, and we do not update refs/heads/extra at all. We can fix this by making a copy of the ref. There's nothing that fetch_pack does to it that must be reflected in the original "sought" list (and indeed, if that were the case we would have a serious bug, because it is only exact-sha1 entries which are treated this way). Signed-off-by: Jeff King --- I always feel a little dirty modifying an existing test rather than writing a new one that more clearly demonstrates what we are testing. But the setup involved in the exact-sha1 fetch is a little complicated, so this seemed easiest (and anybody blaming the change can hopefully rely on the explanation above to understand what is going on). fetch-pack.c | 5 ++--- t/t5516-fetch-push.sh | 13 ++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 058c258..84d5a66 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -555,9 +555,8 @@ static void filter_refs(struct fetch_pack_args *args, continue; ref->matched = 1; - *newtail = ref; - ref->next = NULL; - newtail = &ref->next; + *newtail = copy_ref(ref); + newtail = &(*newtail)->next; } } *refs = newlist; diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 630885d..5e04d64 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1107,9 +1107,16 @@ test_expect_success 'fetch exact SHA1' ' git config uploadpack.allowtipsha1inwant true ) && - git fetch -v ../testrepo $the_commit:refs/heads/copy && - result=$(git rev-parse --verify refs/heads/copy) && - test "$the_commit" = "$result" + git fetch -v ../testrepo $the_commit:refs/heads/copy master:refs/heads/extra && + cat >expect <<-EOF && + $the_commit + $the_first_commit + EOF + { + git rev-parse --verify refs/heads/copy && + git rev-parse --verify refs/heads/extra + } >actual && + test_cmp expect actual ) ' -- 2.3.3.520.g3cfbb5d -- 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/4] filter_ref: avoid overwriting ref->old_sha1 with garbage
If the server supports allow_tip_sha1_in_want, then fetch-pack's filter_refs function tries to check whether a ref is a request for a straight sha1 by running: if (get_sha1_hex(ref->name, ref->old_sha1)) ... I.e., we are using get_sha1_hex to ask "is this ref name a sha1?". If it is true, then the contents of ref->old_sha1 will end up unchanged. But if it is false, then get_sha1_hex makes no guarantees about what it has written. With a ref name like "abcdefoo", we would overwrite 3 bytes of ref->old_sha1 before realizing that it was not a sha1. This is likely not a problem in practice, as anything in refs->name (besides a sha1) will start with "refs/", meaning that we would notice on the first character that there is a problem. Still, we are making assumptions about the state left in the output when get_sha1_hex returns an error (e.g., it could start from the end of the string, or error check the values only once they were placed in the output). It's better to be defensive. We could just check that we have exactly 40 characters of sha1. But let's be even more careful and make sure that we have a 40-char hex refname that matches what is in old_sha1. This is perhaps overly defensive, but spells out our assumptions clearly. Signed-off-by: Jeff King --- This one is not necessary for the others, of course, and we can drop it if you disagree with the reasoning. I wonder if the thinking in the original was that it was our responsibility here to make sure that ref->old_sha1 was filled in. It is always filled in by the caller who gives us "sought", which makes sense to me (this matches the rest of the "sought" entries, which come from reading the remote's ref list, and of course must fill in old_sha1 from that list). fetch-pack.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fetch-pack.c b/fetch-pack.c index 655ee64..058c258 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -544,10 +544,14 @@ static void filter_refs(struct fetch_pack_args *args, /* Append unmatched requests to the list */ if (allow_tip_sha1_in_want) { for (i = 0; i < nr_sought; i++) { + unsigned char sha1[20]; + ref = sought[i]; if (ref->matched) continue; - if (get_sha1_hex(ref->name, ref->old_sha1)) + if (get_sha1_hex(ref->name, sha1) || + ref->name[40] != '\0' || + hashcmp(sha1, ref->old_sha1)) continue; ref->matched = 1; -- 2.3.3.520.g3cfbb5d -- 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: Slow git pushes: sitting 1 minute in pack-objects
On Mon, Mar 16, 2015 at 6:15 PM, Jeff King wrote: > On Mon, Mar 09, 2015 at 09:37:25PM -0400, Stephen Morton wrote: > >> 3. Not sure how long this part takes. It takes 1/3 - 1/2 of the time >> when straced, but I think it's much less, as little as 10s when not >> straced. >> It then reads a bunch of what look like objects from filehandle 0 >> (presumably stdin, read from the remote end?) >> It then tries to lstat() these filenames in .git/ >> ./git/refs/, .git/heads/, etc. It always fails ENOENT. >> It fails some 120,000 times. This could be a problem. Though I haven't >> checked to see if this happens on a fast push on another machine. > > Hmm. The "push" process must feed the set of object boundaries to > "pack-objects" so it knows what to pack (i.e., what we want to send, and > what the other side has). > > 120,000 is an awfully large number of objects to be pass there, though. > Does the repo you are pushing to by any chance have an extremely large > number of refs (e.g., on the order of 120K tags)? No. There are on the order of 9,500 refs (mostly tags) but nowhere near 120k. > > Can you try building git with this patch which would tell us more about > where those objects are coming from? > Those are all rather blunt debugging methods, but hopefully it can get > us a sense of where the time is going. > > -Peff Thanks Peff, I haven't tried your patch, but I tried to backtrack a bit and double-check that the problem always happened in a fresh clone with no other modifications. It did _not_ happen in a new clone --a push took just 5s -- and I think the culprit could have been "repack.writebitmaps=true". Although I had thought writebitmaps was not originally enabled, I now suspect that it was. Let me follow up on that first, before I recompile git with your changes. Thanks again, I really appreciate the help. Steve -- 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: Bug in fetch-pack.c, please confirm
On Thu, Mar 19, 2015 at 12:01:26PM -0700, Junio C Hamano wrote: > > I'm working up a few patches in that area, which I'll send out in a few > > minutes. Once that is done, then I think the explanation you give above > > would be correct. > > If a follow-up is coming then I'd just drop this one. Thanks. OK, here it is. Took me a bit longer than I expected, as I wanted to figure out whether the second patch was actually fixing a bug (and if so, to add test coverage). Turns out that it is a real bug. The final patch is what you sent, rebased on top (though there are not any code changes; the underlying commits make the _explanation_ true, but no code change was required). I fixed up the nits I mentioned in my earlier email. [1/4]: filter_ref: avoid overwriting ref->old_sha1 with garbage [2/4]: filter_ref: make a copy of extra "sought" entries [3/4]: fetch_refs_via_pack: free extra copy of refs [4/4]: fetch-pack: remove dead assignment to ref->new_sha1 -Peff -- 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, RFC] checkout: Attempt to checkout submodules
On Thu, Mar 19, 2015 at 11:53:10AM -0700, Junio C Hamano wrote: > Trevor Saunders writes: > > > If a user does git checkout HEAD -- path/to/submodule they'd expect the > > submodule to be checked out to the commit that submodule is at in HEAD. > > Hmmm. > > Is it a good idea to do that unconditionally by hard-coding the > behaviour like this patch does? if I was sure it was a good idea it wouldn't be an RFC :-) > Is it a good idea that hard-coded behaviour is "checkout [-f]"? I suspect it depends on how you end up in checkout_entry. If you do git checkout HEAD -- /some/file then you force over writing any changes to /some/file so I think a user should probably expect when the path is to a submodule the effect is the same, the path is forced to be in the state it is in HEAD. > I think "git submodule update" is the command people use when they > want to "match" the working trees of submodules, and via the > configuration mechanism submodule.*.update, people can choose what > they mean by "match"ing. Some people want to checkout the commit > specified in the superproject tree by detaching HEAD at it. Some > people want to integrate by merging or rebasing. git submodule update is certainly the current way to deal with the situation that your checkout of the submodule is out of sync with what is in the containing repo. However I think users who aren't familiar with submodules would expect to be able to use "standard" git tools to deal with them. So if they see diff --git a/git-core b/git-core index bb85775..52cae64 16 --- a/git-core +++ b/git-core @@ -1 +1 @@ -Subproject commit bb8577532add843833ebf8b5324f94f84cb71ca0 +Subproject commit 52cae643c5d49b7fa18a7a4c60c284f9ae2e2c71 I think they'd expect they could restore git-core to the state in head the same way they could with any other file by running git checkout HEAD -- git-core, and they'd be suprised when that sighlently did nothing. I suppose its an option to print a message saying that nothing is being done with the submodule git submodule should be used, but that seems kind of unhelpful. On one hand it seems kind of user hostile to just toss out any changes in the submodule that are uncommitted, on the other for any other path it would seem weird to have git checkout trigger rebasing or merging. Trev > > > This is the most brute force possible way of try to do that, and so its > > probably broken in some cases. However I'm not terribly familiar with > > git's internals and I'm not sure if this is even wanted so I'm starting > > simple. If people want this to work I can try and do something better. > > > > Signed-off-by: Trevor Saunders > > --- > > entry.c | 22 -- > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/entry.c b/entry.c > > index 1eda8e9..2dbf5b9 100644 > > --- a/entry.c > > +++ b/entry.c > > @@ -1,6 +1,8 @@ > > #include "cache.h" > > +#include "argv-array.h" > > #include "blob.h" > > #include "dir.h" > > +#include "run-command.h" > > #include "streaming.h" > > > > static void create_directories(const char *path, int path_len, > > @@ -277,9 +279,25 @@ int checkout_entry(struct cache_entry *ce, > > * just do the right thing) > > */ > > if (S_ISDIR(st.st_mode)) { > > - /* If it is a gitlink, leave it alone! */ > > - if (S_ISGITLINK(ce->ce_mode)) > > + if (S_ISGITLINK(ce->ce_mode)) { > > + struct argv_array args = ARGV_ARRAY_INIT; > > + char sha1[41]; > > + > > + argv_array_push(&args, "checkout"); > > + > > + if (state->force) > > + argv_array_push(&args, "-f"); > > + > > + memcpy(sha1, sha1_to_hex(ce->sha1), 41); > > + argv_array_push(&args, sha1); > > + > > + run_command_v_opt_cd_env(args.argv, > > +RUN_GIT_CMD, ce->name, > > +NULL); > > + argv_array_clear(&args); > > + > > return 0; > > + } > > if (!state->force) > > return error("%s is a directory", path.buf); > > remove_subtree(&path); > -- > 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 2/5] refs: introduce a "ref paranoia" flag
Jeff King writes: > diff --git a/refs.c b/refs.c > index e23542b..7f0e7be 100644 > --- a/refs.c > +++ b/refs.c > @@ -1934,6 +1934,11 @@ static int do_for_each_ref(struct ref_cache *refs, > const char *base, > data.fn = fn; > data.cb_data = cb_data; > > + if (ref_paranoia < 0) > + ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); > + if (ref_paranoia) > + data.flags |= DO_FOR_EACH_INCLUDE_BROKEN; I am not a big fan of proliferation of interfaces based on environment variables, but hopefully this is isolated enough to become an issue in the future. > + > return do_for_each_entry(refs, base, do_one_ref, &data); > } -- 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/5] t5312: test object deletion code paths in a corrupted repository
Jeff King writes: > +test_expect_success 'create history with missing tip commit' ' > + test_tick && git commit --allow-empty -m one && > + recoverable=$(git rev-parse HEAD) && > + git cat-file commit $recoverable >saved && > + test_tick && git commit --allow-empty -m two && > + missing=$(git rev-parse HEAD) && > + # point HEAD elsewhere > + git checkout $base && Could you spell this as "$base^0" (or "--detach") to clarify the intention? I have been scraching my head for a few minutes just now, trying to figure out what you are doing here. I _think_ you wanted master to point at the missing "two" and wanted to make sure all other refs (including HEAD) to point away from it. Mental note: At this point, the history looks like base onetwo o--o--o \ o bogus and because the reference to two is still there but two itself is missing, pruning may well end up losing one, because the reference to it is only through master pointing at two. > + rm .git/objects/$(echo $missing | sed "s,..,&/,") && > + test_must_fail git cat-file -e $missing > +' > + > +test_expect_failure 'pruning with a corrupted tip does not drop history' ' > + test_when_finished "git hash-object -w -t commit saved" && > + test_might_fail git prune --expire=now && > + verbose git cat-file -e $recoverable > +' Mental note: OK, this demonstrates that the missing two makes us lose the only reference to one (aka $recoverable in saved). > +test_expect_success 'pack-refs does not silently delete broken loose ref' ' > + git pack-refs --all --prune && > + echo $missing >expect && > + git rev-parse refs/heads/master >actual && > + test_cmp expect actual > +' > + > +# we do not want to count on running pack-refs to > +# actually pack it, as it is perfectly reasonable to > +# skip processing a broken ref > +test_expect_success 'create packed-refs file with broken ref' ' > + rm -f .git/refs/heads/master && > + cat >.git/packed-refs <<-EOF > + $missing refs/heads/master > + $recoverable refs/heads/other > + EOF I do not know offhand if the lack of the pack-refs feature header matters here; I assume it does not? A safer check may be to pack and then make it missing, I guess, but I do not know if the difference matters. -- 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: Rebase pain on (the) pot
Michael J Gruber writes: > Do we have a merge driver or something for the l10n files? I haven't heard of any, but given that these can be added back by running xgettext and the result will have the up-to-date line numbers, it wouldn't be wrong to define a script that roughly does: * find a run of lines that match "^#: .*$" and replace it with a single line with "#:" in original, ours and theirs. * feed these three files to 'merge' from RCS suite. and use that as a merge driver. The merged result would lose the line number information, so if you really care, you could run "make pot" at each step to update it but I do not think it matters as long as you do so at the very end once, even as a follow-up "fix-up" patch that says "we deliberately lost the line number information during the series to avoid unnecessary merge conflicts, and this commit puts it back". -- 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: Why is "git fetch --prune" so much slower than "git remote prune"?
Michael Haggerty writes: >> Now that we have ref_transaction_*, I think if git-fetch fed all of the >> deletes (along with the updates) into a single transaction, we would get >> the same optimization for free. Maybe that is even part of some of the >> pending ref_transaction work from Stefan or Michael (both cc'd). I >> haven't kept up very well with what is cooking in pu. > > I am looking into this now. > > For pruning, we can't use a ref_transaction as it is currently > implemented because it would fail if any of the reference deletions > failed. But in this case I think if any deletions fail, we would prefer > to emit a warning but keep going. I am not quite sure what you mean here. I agree with you if you meant "we shouldn't fail the fetch only because 'fetch --prune' failed to remove only one of the remote-tracking refs that are no longer there" but that can easily be solved by the pruning phase into a separate transaction. If you meant "we would rather remove origin/{a,b} non-atomically when we noticed that origin/{a,b,c} are all gone than leaving all three intact only because we failed to remove origin/c for whatever reason", my knee-jerk reaction is "does it make practical difference to the end user between these two?" What are the plausible cause of failing to prune unused remote-tracking 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: Bug in fetch-pack.c, please confirm
Jeff King writes: >> - The only caller of everything_local(), do_fetch_pack(), returns >>this list of ref, whose element has bogus new_sha1 values, to its >>caller. It does not look at the elements and acts on them. > > I'm not sure what the final sentence means. Should it be "It does not > look at the elements nor act on them"? Yes. "It does not look at the new_sha1. It does not use the information to change its behaviour. Both of the previous two statements are true." is what I meant. > do_fetch_pack actually does pass them down to find_common. But the > latter does not look at ref->new_sha1, so we're OK. >> - The other caller of fetch_pack() is fetch_refs_via_pack() in the >>transport layer, which is a helper that implements "git fetch". >>It only cares about whether the returned list is empty (i.e. >>failed to fetch anything). > > So I thought I would follow this up by adding a free_refs() call in > fetch_refs_via_pack. After all, we just leak that list, right? Yeah, I agree. > I'm working up a few patches in that area, which I'll send out in a few > minutes. Once that is done, then I think the explanation you give above > would be correct. If a follow-up is coming then I'd just drop this one. 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, RFC] checkout: Attempt to checkout submodules
Trevor Saunders writes: > If a user does git checkout HEAD -- path/to/submodule they'd expect the > submodule to be checked out to the commit that submodule is at in HEAD. Hmmm. Is it a good idea to do that unconditionally by hard-coding the behaviour like this patch does? Is it a good idea that hard-coded behaviour is "checkout [-f]"? I think "git submodule update" is the command people use when they want to "match" the working trees of submodules, and via the configuration mechanism submodule.*.update, people can choose what they mean by "match"ing. Some people want to checkout the commit specified in the superproject tree by detaching HEAD at it. Some people want to integrate by merging or rebasing. > This is the most brute force possible way of try to do that, and so its > probably broken in some cases. However I'm not terribly familiar with > git's internals and I'm not sure if this is even wanted so I'm starting > simple. If people want this to work I can try and do something better. > > Signed-off-by: Trevor Saunders > --- > entry.c | 22 -- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/entry.c b/entry.c > index 1eda8e9..2dbf5b9 100644 > --- a/entry.c > +++ b/entry.c > @@ -1,6 +1,8 @@ > #include "cache.h" > +#include "argv-array.h" > #include "blob.h" > #include "dir.h" > +#include "run-command.h" > #include "streaming.h" > > static void create_directories(const char *path, int path_len, > @@ -277,9 +279,25 @@ int checkout_entry(struct cache_entry *ce, >* just do the right thing) >*/ > if (S_ISDIR(st.st_mode)) { > - /* If it is a gitlink, leave it alone! */ > - if (S_ISGITLINK(ce->ce_mode)) > + if (S_ISGITLINK(ce->ce_mode)) { > + struct argv_array args = ARGV_ARRAY_INIT; > + char sha1[41]; > + > + argv_array_push(&args, "checkout"); > + > + if (state->force) > + argv_array_push(&args, "-f"); > + > + memcpy(sha1, sha1_to_hex(ce->sha1), 41); > + argv_array_push(&args, sha1); > + > + run_command_v_opt_cd_env(args.argv, > + RUN_GIT_CMD, ce->name, > + NULL); > + argv_array_clear(&args); > + > return 0; > + } > if (!state->force) > return error("%s is a directory", path.buf); > remove_subtree(&path); -- 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: Bug in fetch-pack.c, please confirm
On Thu, Mar 19, 2015 at 10:41:50AM -0700, Junio C Hamano wrote: > Just to make sure we do not keep this hanging forever and eventually > forget it, I'm planning to queue this. Thanks for following up. A few minor nits (and maybe a more serious problem) on the explanation in the commit message: > be complete., 2005-10-19) and ever since we instead stuffed a random > bytes in ref->new_sha1 here. s/a random/random/ > It turns out that no codepath that comes after this assignment even > looks at ref->new_sha1 at all. > > - The only caller of everything_local(), do_fetch_pack(), returns >this list of ref, whose element has bogus new_sha1 values, to its >caller. It does not look at the elements and acts on them. s/list of ref/list of refs/ ? Or did you mean "the list we store in the 'ref' variable"? I'm not sure what the final sentence means. Should it be "It does not look at the elements nor act on them"? do_fetch_pack actually does pass them down to find_common. But the latter does not look at ref->new_sha1, so we're OK. > - The only caller of do_fetch_pack(), fetch_pack(), returns this >list to its caller. It does not look at the elements and acts on >them. Ditto on the wording in the final sentence here (but it is correct in this case that we do not touch the list at all). > - The other caller of fetch_pack() is fetch_refs_via_pack() in the >transport layer, which is a helper that implements "git fetch". >It only cares about whether the returned list is empty (i.e. >failed to fetch anything). So I thought I would follow this up by adding a free_refs() call in fetch_refs_via_pack. After all, we just leak that list, right? If only it were so simple. It turns out that the list returned from fetch_pack() is _mostly_ a copy of the incoming refs list we give it. But in filter_refs(), if allow_tip_sha1_in_want is set, we actually add the unmatched "sought" refs here, too. Which means we may be setting new_sha1 in one of these "sought" refs, and that broadens the scope of code that might be affected by the patch we have been discussing. However, I think the filter_refs code is wrong, and should be making a copy of the sought refs to put in the new list. I'm working up a few patches in that area, which I'll send out in a few minutes. Once that is done, then I think the explanation you give above would be correct. -Peff -- 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: Fwd: Seems to be pushing more than necessary
Graham Hay writes: > We have a fairly large repo (~2.4GB), mainly due to binary resources > (for an ios app). I know this can generally be a problem, but I have a > specific question. > > If I cut a branch, and edit a few (non-binary) files, and push, what > should be uploaded? I assumed it was just the diff (I know whole > compressed files are used, I mean the differences between my branch > and where I cut it from). Is that correct? If you start from this state: (the 'origin')(you) ---Z---A clone ->---Z---A and edit a few files, say, a/b, a/c and d/e/f, and committed to make the history look like this: (the 'origin')(you) ---Z---A ---Z---A---B i.e. "git diff --name-only A B" would show these three files, then the next push from you to the origin, i.e. (the 'origin')(you) ---Z---A---B<- push ---Z---A---B would involve transferring from you to the origin of the following: * The commit object that holds the message, authorship, etc. for B * The top-level tree object of commit B (as that is different from that of A) * The tree object for 'a', 'd', 'd/e' and the blob object for 'a/b', 'a/c', and 'd/e/f'. However, that assumes that nothing is happening on the 'origin' side. If the 'origin', for example, rewound its head to Z before you attempt to push your B, then you may end up sending objects that do not exist in Z that are reachable from B. Just like the above bullet points enumerated what is different between A and B, you can enumerate what is different between Z and A and add that to the above set. That would be what will be sent. If the 'origin' updated its tip to a commit you do not even know about, normally you will be prevented from pushing B because we would not want you to lose somebody else's work. If you forced such push, then you may end up sending a lot more. -- 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: Bug in git-verify-pack or in its documentation?
Duy Nguyen writes: > On Mon, Mar 16, 2015 at 8:18 PM, Mike Hommey wrote: >> On Mon, Mar 16, 2015 at 05:13:25PM +0700, Duy Nguyen wrote: >>> On Mon, Mar 16, 2015 at 3:05 PM, Mike Hommey wrote: >>> > Hi, >>> > >>> > git-verify-pack's man page says the following about --stat-only: >>> > >>> >Do not verify the pack contents; only show the histogram of delta >>> >chain length. With --verbose, list of objects is also shown. >>> > >>> > However, there is no difference of output between --verbose and >>> > --verbose --stat-only (and in fact, looking at the code, only one of >>> > them has an effect, --stat-only having precedence). >>> >>> There is. very-pack --verbose would show a lot of " >>> <"random" numbers>" lines before the histogram while --stat-only (with >>> or without --verbose) would only show the histogram. >> >> Err, I meant between --stat-only and --verbose --stat-only. > > Yeah --verbose is always ignored when --stat-only is set. This command is > funny. I would disagree. "--verbose" is "do whatever you are told to do but you can enhance the result by giving more verbose output". To understand what I mean, compare "git verify-pack" (no other options) and "git verify-pack --verbose". Now, when you are asking the command to show the statistics only and nothing else, there may be nothing useful that you can output to enhance the "stat-only" output. "git verify-pack --stat-only" (no other options) and "git verify-pack --stat-only --verbose" can produce exactly the same result in such a case. So I do not see anything funny there. -- 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/RFC] completion: filter sources of git scripts from available commands
SZEDER Gábor writes: > Possible solutions to avoid this issue are: > > - Strip the current working directory from $PATH temporarily while we run > 'git help -a' to get all the available commands. Care must be taken, > because '.' can appear at the very beginning, end, or in the middle of > $PATH, not to mention that on unixes it's unlikely but possible to have > a directory containing e.g. ':.:' in the $PATH. > > - Filter out scripts from the output of 'git help -a'. This can be done > by either > * listing all possible script extensions, but this list will most likely > never be complete, or > * filtering out all commands containing a filename extension, i.e. > anything with a '.' in it. > This will bite users whose custom git commands have filename extensions, > i.e. who put 'git-mycmd.sh' in '~/bin'. > > Since this is an RFC, it goes with the last option, because that's the > shortest and simplest. Would it be another option to re-evaluate the list upon chdir (we can trap an attempt to "cd", can't we?) _and_ if $PATH might contain ".". It is OK if the latter criterion is not precise as long as it does not say "$PATH does not have '.' in it" when it does. > > Signed-off-by: SZEDER Gábor > --- > 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 c21190d..9173c41 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -637,6 +637,7 @@ __git_list_all_commands () > do > case $i in > *--*) : helper pattern;; > + *.*) : script sources;; > *) echo $i;; > esac > done -- 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: Bug in fetch-pack.c, please confirm
Jeff King writes: > ... > And there we stop. We don't pass the "refs" list out of that function > (which, as an aside, is probably a leak). Instead, we depend on the list > of heads we already knew in the "to_fetch" array. That comes from > processing the earlier list of refs returned from get_refs_via_connect. > ... > That doesn't mean there isn't an additional bug lurking. That is, > _should_ somebody be caring about that value? I don't think so. The > old/new pairs in a "struct ref" are meaningful as "I proposed to update > to X, and we are at Y". But this list of refs does not have anything to > do with the update of local refs. It is only "what is the value of the > ref on the other side". The local refs are taken care of in a separate > list. Correct. When we want to insert some function to allow users/hooks to tweak the result of update, we might want to make sure that we are passing the final list of refs to that function, but the purpose of this function is not to make such a modification. Just to make sure we do not keep this hanging forever and eventually forget it, I'm planning to queue this. Thanks. -- >8 -- From: Jeff King Date: Sun, 15 Mar 2015 21:13:43 -0400 Subject: [PATCH] fetch-pack: remove dead assignment to ref->new_sha1 In everything_local(), we used to assign the current ref's value found in ref->old_sha1 to ref->new_sha1 when we already have all the necessary objects to complete the history leading to that commit. This copying was broken at 49bb805e (Do not ask for objects known to be complete., 2005-10-19) and ever since we instead stuffed a random bytes in ref->new_sha1 here. No code complained or failed due to this breakage. It turns out that no codepath that comes after this assignment even looks at ref->new_sha1 at all. - The only caller of everything_local(), do_fetch_pack(), returns this list of ref, whose element has bogus new_sha1 values, to its caller. It does not look at the elements and acts on them. - The only caller of do_fetch_pack(), fetch_pack(), returns this list to its caller. It does not look at the elements and acts on them. - One of the two callers of fetch_pack() is cmd_fetch_pack(), the top-level that implements "git fetch-pack". The only thing it looks at in the elements of the returned ref list is the old_sha1 and name fields. - The other caller of fetch_pack() is fetch_refs_via_pack() in the transport layer, which is a helper that implements "git fetch". It only cares about whether the returned list is empty (i.e. failed to fetch anything). Just drop the bogus assignment, that is not even necessary. The remote-tracking refs are updated based on a different list and not using the ref list being manipulated by this codepath; the caller do_fetch_pack() created a copy of that real ref list and passed the copy down to this function, and modifying the elements here does not affect anything. Noticed-by: Kyle J. McKay Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- fetch-pack.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 655ee64..6f0c0e1 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -625,7 +625,6 @@ static int everything_local(struct fetch_pack_args *args, for (retval = 1, ref = *refs; ref ; ref = ref->next) { const unsigned char *remote = ref->old_sha1; - unsigned char local[20]; struct object *o; o = lookup_object(remote); @@ -638,8 +637,6 @@ static int everything_local(struct fetch_pack_args *args, ref->name); continue; } - - hashcpy(ref->new_sha1, local); if (!args->verbose) continue; fprintf(stderr, -- 2.3.3-377-gdf43604 -- 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: Why is "git fetch --prune" so much slower than "git remote prune"?
On Thu, Mar 19, 2015 at 03:49:08PM +0100, Michael Haggerty wrote: > For pruning, we can't use a ref_transaction as it is currently > implemented because it would fail if any of the reference deletions > failed. But in this case I think if any deletions fail, we would prefer > to emit a warning but keep going. > > I'm trying to decide whether to have a separate function in the refs API > to "delete as many of the following refs as possible", or whether to add > a flag to ref_transaction_update() that says "try this update, but don't > fail the transaction if it fails". The latter would probably be more > work because we would need to invent a way to return multiple error > messages from a single transaction. Hmm, yeah. That is sort of antithetical to the original purpose of ref transactions (which was atomicity). Given the way it is implemented now, I don't think it would be too big a deal to keep a per-ref status (one per "struct ref_upate"). But I would worry in the long run that it makes things much harder on proposed ref backends, which otherwise can consider a transaction an atomic unit. OTOH, I do not think there is an abstracted way to do this _without_ the ref backend knowing about it. You cannot just do a series of transactions; that defeats the purpose. It is a bit of a layering violation that builtin/remote.c (which does this optimization already) calls repack_without_refs directly. -Peff -- 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/5] gitweb: fix typo in man page
Signed-off-by: Tony Finch --- Documentation/gitweb.conf.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt index ebe7a6c..29f1e06 100644 --- a/Documentation/gitweb.conf.txt +++ b/Documentation/gitweb.conf.txt @@ -487,7 +487,7 @@ By default it is set to (), i.e. an empty list. This means that gitweb would not try to create project URL (to fetch) from project name. $projects_list_group_categories:: - Whether to enables the grouping of projects by category on the project + Whether to enable the grouping of projects by category on the project list page. The category of a project is determined by the `$GIT_DIR/category` file or the `gitweb.category` variable in each repository's configuration. Disabled by default (set to 0). -- 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
[PATCH 5/5] gitweb: make category headings into links when they are directories
When $projects_list_category_is_directory is turned on, project categories can be useful as project filters, so with that setting gitweb now makes the category headings into project_filter links (like the breadcrumbs). Signed-off-by: Tony Finch --- gitweb/gitweb.perl | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 0aab3e0..a02f3e4 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -5838,8 +5838,18 @@ sub git_project_list_body { if ($check_forks) { print "\n"; } - print "".esc_html($cat)."\n"; - print "\n"; + print ""; + if ($projects_list_directory_is_category) { + print $cgi->a({-href => + href(project => undef, + project_filter => $cat, + action => "project_list"), + -class => "list"}, + esc_html($cat)); + } else { + print esc_html($cat); + } + print "\n\n"; } git_project_list_rows($categories{$cat}, undef, undef, $check_forks); -- 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
[PATCH 2/5] gitweb: if the PATH_INFO is incomplete, use it as a project_filter
Previously gitweb would ignore partial PATH_INFO. For example, it would produce a project list for the top URL https://www.example.org/projects/ and a project summary for https://www.example.org/projects/git/git.git but if you tried to list just the git-related projects with https://www.example.org/projects/git/ you would get a list of all projects, same as the top URL. As well as fixing that omission, this change also makes gitweb generate PATH_INFO-style URLs for project filter links, such as in the breadcrumbs. Signed-off-by: Tony Finch --- gitweb/gitweb.perl | 24 +++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 7a5b23a..073f324 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -895,7 +895,17 @@ sub evaluate_path_info { while ($project && !check_head_link("$projectroot/$project")) { $project =~ s,/*[^/]*$,,; } - return unless $project; + # If there is no project, use the PATH_INFO as a project filter if it + # is a directory in the projectroot. (It can't be a subdirectory of a + # repo because we just verified that isn't the case.) + unless ($project) { + if (-d "$projectroot/$path_info") { + $path_info =~ s,/+$,,; + $input_params{'project_filter'} = $path_info; + $path_info = ""; + } + return; + } $input_params{'project'} = $project; # do not change any parameters if an action is given using the query string @@ -1360,6 +1370,18 @@ sub href { } my $use_pathinfo = gitweb_check_feature('pathinfo'); + + # we have to check for a project_filter first because handling the full + # project-plus-parameters deletes some of the paramaters we check here + if (!defined $params{'project'} && $params{'project_filter'} && + $params{'action'} eq "project_list" && + (exists $params{-path_info} ? $params{-path_info} : $use_pathinfo)) { + $href =~ s,/$,,; + $href .= "/".esc_path_info($params{'project_filter'})."/"; + delete $params{'project_filter'}; + delete $params{'action'}; + } + if (defined $params{'project'} && (exists $params{-path_info} ? $params{-path_info} : $use_pathinfo)) { # try to put as many parameters as possible in PATH_INFO: -- 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
[PATCH 3/5] gitweb: add a link under the search box to clear a project filter
Previously when a project filter was active, the only simple way to clear it was by clicking the home link in the breadcrumbs, which is not very obvious. This change adds another home link under the search box which clears both project filter and search, next to the existing link that clears the search and keeps the project filter. Signed-off-by: Tony Finch --- gitweb/gitweb.perl | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 073f324..9abc5bc 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -5549,10 +5549,13 @@ sub git_project_search_form { "\n" . $cgi->submit(-name => 'btnS', -value => 'Search') . $cgi->end_form() . "\n" . - $cgi->a({-href => href(project => undef, searchtext => undef, -project_filter => $project_filter)}, - esc_html("List all projects$limit")) . "\n"; - print "\n"; + $cgi->a({-href => $my_uri}, esc_html("List all projects")); + print " / " . + $cgi->a({-href => href(project => undef, action => "project_list", +project_filter => $project_filter)}, + esc_html("List projects$limit")) + if $project_filter; + print "\n\n"; } # entry for given @keys needs filling if at least one of keys in list -- 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
[PATCH 4/5] gitweb: optionally set project category from its pathname
When repositories are organized in a hierarchial directory tree it is convenient if gitweb project categories can be set automatically based on their parent directory, so that users do not have to set the same information twice. Signed-off-by: Tony Finch --- Documentation/gitweb.conf.txt | 6 ++ gitweb/gitweb.perl| 13 ++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt index 29f1e06..7c0de18 100644 --- a/Documentation/gitweb.conf.txt +++ b/Documentation/gitweb.conf.txt @@ -492,6 +492,12 @@ $projects_list_group_categories:: `$GIT_DIR/category` file or the `gitweb.category` variable in each repository's configuration. Disabled by default (set to 0). +$projects_list_directory_is_category:: + Whether to set a project's category to its parent directory, i.e. its + pathname excluding the `/repo.git` leaf name. This is only used if + the repo has no explicit setting, and if the pathname has more than + one component. Disabled by default (set to 0). + $project_list_default_category:: Default category for projects for which none is specified. If this is set to the empty string, such projects will remain uncategorized and diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 9abc5bc..0aab3e0 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -133,6 +133,10 @@ our $projects_list_description_width = 25; # (enabled if this variable evaluates to true) our $projects_list_group_categories = 0; +# project's category defaults to its parent directory +# (enabled if this variable evaluates to true) +our $projects_list_directory_is_category = 0; + # default category if none specified # (leave the empty string for no category) our $project_list_default_category = ""; @@ -2908,7 +2912,11 @@ sub git_get_project_description { sub git_get_project_category { my $path = shift; - return git_get_file_or_project_config($path, 'category'); + my $cat = git_get_file_or_project_config($path, 'category'); + return $cat if $cat; + return $1 if $projects_list_directory_is_category + && $path =~ m,^(.*)/[^/]*$,; + return $project_list_default_category; } @@ -5622,8 +5630,7 @@ sub fill_project_list_info { } if ($projects_list_group_categories && project_info_needs_filling($pr, $filter_set->('category'))) { - my $cat = git_get_project_category($pr->{'path'}) || - $project_list_default_category; + my $cat = git_get_project_category($pr->{'path'}); $pr->{'category'} = to_utf8($cat); } -- 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: Why is "git fetch --prune" so much slower than "git remote prune"?
On 03/06/2015 11:59 PM, Jeff King wrote: > On Fri, Mar 06, 2015 at 05:48:39PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> The --prune option to fetch added in v1.6.5-8-gf360d84 seems to be >> around 20-30x slower than the equivalent operation with git remote >> prune. I'm wondering if I'm missing something and fetch does something >> more, but it doesn't seem so. > > [...] > We spend a lot of time checking refs here. Probably this comes from > writing the `packed-refs` file out 1000 times in your example, because > fetch handles each ref individually. Whereas since c9e768b (remote: > repack packed-refs once when deleting multiple refs, 2014-05-23), > git-remote does it in one pass. > > Now that we have ref_transaction_*, I think if git-fetch fed all of the > deletes (along with the updates) into a single transaction, we would get > the same optimization for free. Maybe that is even part of some of the > pending ref_transaction work from Stefan or Michael (both cc'd). I > haven't kept up very well with what is cooking in pu. I am looking into this now. For pruning, we can't use a ref_transaction as it is currently implemented because it would fail if any of the reference deletions failed. But in this case I think if any deletions fail, we would prefer to emit a warning but keep going. I'm trying to decide whether to have a separate function in the refs API to "delete as many of the following refs as possible", or whether to add a flag to ref_transaction_update() that says "try this update, but don't fail the transaction if it fails". The latter would probably be more work because we would need to invent a way to return multiple error messages from a single transaction. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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
Rebase pain on (the) pot
Do we have a merge driver or something for the l10n files? I'm trying to rebase an older branch on top of origin/next. My topic branch has changes to git.pot (the old glossary command idea), and rebasing produces a lot of conflicts due to simple line number changes in the comments. (The de.po in the next commit to be rebased won't fair any better, probably.) Michael -- 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 v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME
Eric Sunshine writes: > I also tend to favor adding "failure" tests which are flipped to > "success" when appropriate, however, as this is an entirely new > feature, this approach may be unsuitable (and perhaps overkill). I can buy the "overkill", but not unsuitable. Even for new features, the tests should fail before and pass after. Otherwise, the tests are not testing the feature. Actually, this is a strong principle in test-driven development: don't write code unless you have a failing test. But I was just thinking out loudly, certainly not requesting a change. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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: submodule.$name.url is ignored during submodule update
I want to use a custom url for both initial submodule clone and submodule update. Git submodule man page states that if I run 'git submodule init' and then change the 'submodule.$name.url' in the main repository config, git will use this url instead of url in .gitmodules. Git does use the custom url for initial submodule clone, but doesn't use it when cloned submodule needs to be updated. Is that by design? On Thu, Mar 19, 2015 at 2:09 PM, Doug Kelly wrote: > On Thu, Mar 19, 2015 at 4:27 AM, Dmitry Neverov > wrote: >> Hi, >> >> I've noticed that the 'submodule.$name.url' config parameter from the >> main repository is ignored when a submodule needs to be updated, the >> submodule's 'remote.origin.url' is used instead. Is there any way to >> customize the submodule url for both the initial clone and for >> updates? > > That's what "git submodule sync" is for. It will synchronize the url > in .gitmodules with > to the remote.origin.url for each submodule. I'm not sure about the second > part > of your question: are you talking about using different URLs for the > initial clone > and updates? -- 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: submodule.$name.url is ignored during submodule update
On Thu, Mar 19, 2015 at 4:27 AM, Dmitry Neverov wrote: > Hi, > > I've noticed that the 'submodule.$name.url' config parameter from the > main repository is ignored when a submodule needs to be updated, the > submodule's 'remote.origin.url' is used instead. Is there any way to > customize the submodule url for both the initial clone and for > updates? That's what "git submodule sync" is for. It will synchronize the url in .gitmodules with to the remote.origin.url for each submodule. I'm not sure about the second part of your question: are you talking about using different URLs for the initial clone and updates? -- 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: [GSoC Project Help] Unifying git branch -l, git tag -l and git for-each-ref
Sundararajan R venit, vidit, dixit 19.03.2015 12:22: > Hi all, > > I am a Computer Science sophomore at IIT Kanpur. I am interested in > contributing to git in GSoC 2015. I have been using git for the past one year > and am pretty comfortable with its commands which is what made me think about > contributing to git. I have attempted the microproject “adding ‘-’ as a > shorthand to @{-1} in the reset command” [1] [2] from which I learnt about > how code is reviewed in the community and how a seemingly small change can > end up being much more difficult. But the thing I liked the most is the warm > and welcoming attitude of everyone in the community towards a newcomer like > me. I wish to take up the project idea “Unifying git branch -l, git tag -l > and git for-each-ref”. I am in the process of reading and understanding the > codes of these three commands and figuring out similarities and differences > in them. I have gone through some of the discussions regarding this on the > archive and have also read Junio’s reply to Amate Yolande [3], but I haven’t > been able to find patches which attempt to unify the selection process as > mentioned in the description of the idea. It would be great if someone could > point me towards these patches which would help me when I start designing the > details of the unified implementation. Thanks a lot for your time. > > Regards, > R Sundararajan. > > [1] : http://marc.info/?l=git&m=142666740415816&w=2 > [2] : http://marc.info/?l=git&m=142666773315899&w=2 > [3] : http://article.gmane.org/gmane.comp.version-control.git/264966 I don't think there have been actual attempts at unifying the "display" part of the list modes. A first step would be: Check what "tag -l" and "branch -l" output you can reproduce using for-each-ref format strings. Then see whether for-each-ref needs to expose more information about the refs. I wouldn't mind unifying the actual output format, but some will disagree. As for the issue of ref selection (contains and such), the following thread may be helpful: http://thread.gmane.org/gmane.comp.version-control.git/252472 Cheers Michael -- 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=p4.py rebase now honor's client spec
When using the git-p4.py script, I found that if I used a client spec when cloning out a perforce repository, and then using a git-p4.py rebase, that the rebase command would be using the current perforce client spec, instead of the one used when doing the initial clone. My proposed patch causes the rebase command to switch to the perforce client spec used when doing the initial git-p4.py clone. This email and any attachments may contain confidential and proprietary information of Blackboard that is for the sole use of the intended recipient. If you are not the intended recipient, disclosure, copying, re-distribution or other use of any of this information is strictly prohibited. Please immediately notify the sender and delete this transmission if you received this email in error. fix_git_p4_rebase.patch Description: fix_git_p4_rebase.patch
[GSoC Project Help] Unifying git branch -l, git tag -l and git for-each-ref
Hi all, I am a Computer Science sophomore at IIT Kanpur. I am interested in contributing to git in GSoC 2015. I have been using git for the past one year and am pretty comfortable with its commands which is what made me think about contributing to git. I have attempted the microproject “adding ‘-’ as a shorthand to @{-1} in the reset command” [1] [2] from which I learnt about how code is reviewed in the community and how a seemingly small change can end up being much more difficult. But the thing I liked the most is the warm and welcoming attitude of everyone in the community towards a newcomer like me. I wish to take up the project idea “Unifying git branch -l, git tag -l and git for-each-ref”. I am in the process of reading and understanding the codes of these three commands and figuring out similarities and differences in them. I have gone through some of the discussions regarding this on the archive and have also read Junio’s reply to Amate Yolande [3], but I haven’t been able to find patches which attempt to unify the selection process as mentioned in the description of the idea. It would be great if someone could point me towards these patches which would help me when I start designing the details of the unified implementation. Thanks a lot for your time. Regards, R Sundararajan. [1] : http://marc.info/?l=git&m=142666740415816&w=2 [2] : http://marc.info/?l=git&m=142666773315899&w=2 [3] : http://article.gmane.org/gmane.comp.version-control.git/264966N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf
Re: Seems to be pushing more than necessary
On Wed, Mar 18, 2015 at 10:14 PM, Graham Hay wrote: > Got there eventually! > > $ git verify-pack --verbose bar.pack > e13e21a1f49704ed35ddc3b15b6111a5f9b34702 commit 220 152 12 > 03691863451ef9db6c69493da1fa556f9338a01d commit 334 227 164 > ... snip ... > chain length = 50: 2 objects > bar.pack: ok > > Now what do I do with it :) Try "fast-export --anonymize" as that would help us understand this. Or you can try to see if these commits exist in the remote repo. If yes, that only confirms that push sends more that it should, but it's hard to know why. Maybe if you fire up gitk and mark them commits, you'll figure out a connection. There are actually objects in this pack that are expected to exist in remote repo, but it's hard to tell.. -- Duy -- 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 pull & git gc
On Wed, Mar 18, 2015 at 9:58 PM, John Keeping wrote: > On Wed, Mar 18, 2015 at 09:41:59PM +0700, Duy Nguyen wrote: >> On Wed, Mar 18, 2015 at 9:33 PM, Duy Nguyen wrote: >> > If not, I made some mistake in analyzing this and we'll start again. >> >> I did make one mistake, the first "gc" should have reduced the number >> of loose objects to zero. Why didn't it.? I'll come back to this >> tomorrow if nobody finds out first :) > > Most likely they are not referenced by anything but are younger than 2 > weeks. > > I saw a similar issue with automatic gc triggering after every operation > when I did something equivalent to: > > git add > git commit > git reset --hard HEAD^ > > which creates a log of unreachable objects which are not old enough to > be pruned. And there's another problem caused by background gc. If it's not run in background, it should print this warning: There are too many unreachable loose objects; run 'git prune' to remove them. but because background gc does not have access to stdout/stderr anymore, this is lost. -- Duy -- 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
submodule.$name.url is ignored during submodule update
Hi, I've noticed that the 'submodule.$name.url' config parameter from the main repository is ignored when a submodule needs to be updated, the submodule's 'remote.origin.url' is used instead. Is there any way to customize the submodule url for both the initial clone and for updates? -- Dmitry -- 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 00/14] numparse module: systematically tighten up integer parsing
Jeff King writes: > I wonder how much of the boilerplate in the parse_* functions could be > factored out to use a uintmax_t, with the caller just providing the > range. That would make it easier to add new types like off_t, and > possibly even constrained types (e.g., an integer from 0 to 100). On the > other hand, you mentioned to me elsewhere that there may be some bugs in > the range-checks of config.c's integer parsing. I suspect they are > related to exactly this kind of refactoring, so perhaps writing things > out is best. I like this idea very well. I wonder if we can implement the family of parse_{type}(const char *, unsigned int flags, const char **endptr, {type} *result) functions by calls a helper that internally deals with the numbers in uintmax_t, and then checks if the value fits within the possible range of the *result before returning. int parse_i(const char *str, unsigned flags, const char **endptr, int *result) { uintmax_t val; int sign = 1, status; if (*str == '-') { sign = -1; str++; } status = parse_num(str, flags, endptr, &val, INT_MAX); if (status < 0) return status; *result = sign < 0 ? -val : val; return 0; } (assuming the last parameter to parse_num is used to check if the result fits within that range). Or that may be easier and cleaner to be done in the caller with or something like that: status = parse_num(str, flags, endptr, &val); if (status < 0) return status; if (INT_MAX <= val * sign || val * sign <= INT_MIN) { errno = ERANGE; return -1; } If we wanted to, we may even be able to avoid duplication of boilerplate by wrapping the above pattern within a macro, parameterizing the TYPE_{MIN,MAX} and using token pasting, to expand to four necessary result types. There is no reason for the implementation of the parse_num() helper to be using strtoul(3) or strtoull(3); its behaviour will be under our total control. It can become fancier by enriching the flags bits (e.g. allow scaling factor, etc.) only once and all variants for various result types will get the same feature. -- 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 with Lader logic
Thanks all .. I will have a look. But could I just set this up on my laptop and checking this works on system first before installing one of Git on server Sent from my iPhone > On 18 Mar 2015, at 22:28, Doug Kelly wrote: > > On Wed, Mar 18, 2015 at 2:53 PM, Randall S. Becker > wrote: >> On March 17, 2015 7:34 PM, Bharat Suvarna wrote: >>> I am trying to find a way of using version control on PLC programmers like >> Allen >>> Bradley PLC. I can't find a way of this. >>> >>> Could you please give me an idea if it will work with Plc programs. Which >> are >>> basically Ladder logic. >> >> Many PLC programs either store their project code in XML, L5K or L5X (for >> example), TXT, CSV, or some other text format or can import and export to >> text forms. If you have a directory structure that represents your project, >> and the file formats have reasonable line separators so that diffs can be >> done easily, git very likely would work out for you. You do not have to have >> the local .git repository in the same directory as your working area if your >> tool has issues with that or .gitignore. You may want to use a GUI client to >> manage your local repository and handle the commit/push/pull/merge/rebase >> functions as I expect whatever PLC system you are using does not have git >> built-in. >> >> To store binary PLC data natively, which some tools use, I expect that those >> who are better at git-conjuring than I, could provide guidance on how to >> automate binary diffs for your tool's particular file format. > > The one thing I find interesting about RSLogix in general (caveat: I > only have very limited experience with RSLogix 500 / 5000; if I do > anything nowadays, it's in the micro series using RSLogix Micro > Starter Lite)... they do have some limited notion of version control > inside the application itself, though it seems rudimentary to me. > This could prove to be helpful or extremely annoying, since even when > I connect to a PLC and go online, just to reset the RTC, it still > prompts me to save again (even though nothing changed, other than the > processor state). > > You may also find this link on stackexchange helpful: > http://programmers.stackexchange.com/questions/102487/are-there-realistic-useful-solutions-for-source-control-for-ladder-logic-program > > As Randall noted, L5K is just text, and RSLogix 5000 uses it, > according to this post. It may work okay. > > --Doug -- 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