* Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory
From: Johannes Schindelin @ 2017-02-10 15:44 UTC (permalink / raw)
To: Mike Rappazzo; +Cc: Junio C Hamano, Duy Nguyen, Git Mailing List
In-Reply-To: <CANoM8SV7oJ6YmKM-n63620EkODxD562BZnLZB6OvX8O6BmDT1A@mail.gmail.com>
Hi Mike,
On Thu, 9 Feb 2017, Mike Rappazzo wrote:
> On Thu, Feb 9, 2017 at 5:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > That leaves what the right single-step behaviour change should be. As
> > I recall Duy said something about --common-dir and other things Mike's
> > earlier change also covered, I'd prefer to leave it to three of you to
> > figure out what the final patch should be.
> >
>
> The tests which I covered in my previous patch [1] addressed the places
> where we identified similar problems. We should try to include some
> form of those tests. As far as implementation goes in rev-parse, the
> version in this thread is probably better that what I had, but it would
> need to also be applied to --git-common-dir and --shared-index-path.
Thank you so much for pointing out that git-common-dir and
shared-index-path have the same problem.
I looked a little further, and it seems that the show_file() function may
have the exact same problem... but then, it only prefixes filenames if the
--prefix=<prefix> option has been passed, and it could be argued that
those prefixed filenames are *not* meant to be relative to the cwd but to
the top-level directory.
Anways, v2 was just sent out, and with Peff's acknowledgement that this
fixes a real bug and that hypothetical scripts relying on the buggy
behavior were broken beyond repair even without worktrees anyway, I am
hopeful that we'll get somewhere.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Johannes Schindelin @ 2017-02-10 15:49 UTC (permalink / raw)
To: Jeff King; +Cc: git, Jeff Hostetler, Junio C Hamano
In-Reply-To: <20170210050237.gajicliueuvk6s5d@sigill.intra.peff.net>
Hi Peff,
On Fri, 10 Feb 2017, Jeff King wrote:
> On Thu, Feb 09, 2017 at 11:27:49PM +0100, Johannes Schindelin wrote:
>
> > From: Jeff Hostetler <jeffhost@microsoft.com>
> >
> > Use OpenSSL's SHA-1 routines rather than builtin block-sha1 routines.
> > This improves performance on SHA1 operations on Intel processors.
> >
> > OpenSSL 1.0.2 has made considerable performance improvements and
> > support the Intel hardware acceleration features. See:
> > https://software.intel.com/en-us/articles/improving-openssl-performance
> > https://software.intel.com/en-us/articles/intel-sha-extensions
> >
> > To test this I added/staged a single file in a gigantic repository
> > having a 450MB index file. The code in read-cache.c verifies the
> > header SHA as it reads the index and computes a new header SHA as it
> > writes out the new index. Therefore, in this test the SHA code must
> > process 900MB of data. Testing was done on an Intel I7-4770 CPU @
> > 3.40GHz (Intel64, Family 6, Model 60) CPU.
>
> I think this is only half the story. A heavy-sha1 workload is faster,
> which is good. But one of the original reasons to prefer blk-sha1 (at
> least on Linux) is that resolving libcrypto.so symbols takes a
> non-trivial amount of time. I just timed it again, and it seems to be
> consistently 1ms slower to run "git rev-parse --git-dir" on my machine
> (from the top-level of a repo).
>
> 1ms is mostly irrelevant, but it adds up on scripted workloads that
> start a lot of git processes.
You know my answer to that. If scripting slows things down, we should
avoid it in production code. As it is, scripting slows us down. Therefore
I work slowly but steadily to get rid of scripting where it hurts most.
Ciao,
Dscho
^ permalink raw reply
* [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory
From: Johannes Schindelin @ 2017-02-10 15:33 UTC (permalink / raw)
To: git; +Cc: Michael Rappazzo, Junio C Hamano,
Nguyễn Thái Ngọc Duy
In-Reply-To: <cover.1486740772.git.johannes.schindelin@gmx.de>
From: Michael Rappazzo <rappazzo@gmail.com>
t2027-worktree-list has an incorrect expectation for --git-common-dir
which has been adjusted and marked to expect failure.
Some of the tests added have been marked to expect failure. These
demonstrate a problem with the way that some options to git rev-parse
behave when executed from a subdirectory of the main worktree.
[jes: fixed incorrect assumption that objects/ lives in the
worktree-specific git-dir (it lives in the common dir instead).]
Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++
t/t1700-split-index.sh | 17 +++++++++++++++++
t/t2027-worktree-list.sh | 12 ++++++++++--
3 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 038e24c4014..f39f783f2db 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru
test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_expect_success 'git-common-dir from worktree root' '
+ echo .git >expect &&
+ git rev-parse --git-common-dir >actual &&
+ test_cmp expect actual
+'
+
+test_expect_failure 'git-common-dir inside sub-dir' '
+ mkdir -p path/to/child &&
+ test_when_finished "rm -rf path" &&
+ echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
+ git -C path/to/child rev-parse --git-common-dir >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git-path from worktree root' '
+ echo .git/objects >expect &&
+ git rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
+test_expect_failure 'git-path inside sub-dir' '
+ mkdir -p path/to/child &&
+ test_when_finished "rm -rf path" &&
+ echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
+ git -C path/to/child rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 292a0720fcc..1d6e27a09d8 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,21 @@ EOF
test_cmp expect actual
'
+test_expect_failure 'rev-parse --shared-index-path' '
+ rm -rf .git &&
+ test_create_repo . &&
+ git update-index --split-index &&
+ ls -t .git/sharedindex* | tail -n 1 >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual &&
+ mkdir work &&
+ test_when_finished "rm -rf work" &&
+ (
+ cd work &&
+ ls -t ../.git/sharedindex* | tail -n 1 >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 465eeeacd3d..c1a072348e7 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -8,16 +8,24 @@ test_expect_success 'setup' '
test_commit init
'
-test_expect_success 'rev-parse --git-common-dir on main worktree' '
+test_expect_failure 'rev-parse --git-common-dir on main worktree' '
git rev-parse --git-common-dir >actual &&
echo .git >expected &&
test_cmp expected actual &&
mkdir sub &&
git -C sub rev-parse --git-common-dir >actual2 &&
- echo sub/.git >expected2 &&
+ echo ../.git >expected2 &&
test_cmp expected2 actual2
'
+test_expect_failure 'rev-parse --git-path objects linked worktree' '
+ echo "$(git rev-parse --show-toplevel)/.git/objects" >expect &&
+ test_when_finished "rm -rf linked-tree && git worktree prune" &&
+ git worktree add --detach linked-tree master &&
+ git -C linked-tree rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
test_expect_success '"list" all worktrees from main' '
echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
test_when_finished "rm -rf here && git worktree prune" &&
--
2.11.1.windows.1
^ permalink raw reply related
* Re: Bug with fixup and autosquash
From: Johannes Schindelin @ 2017-02-10 15:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ashutosh Bapat, git, Michael Haggerty, Michael J Gruber,
Matthieu Moy
In-Reply-To: <xmqqd1ernh7g.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 9 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Almost. While I fixed the performance issues as well as the design
> > allowed, I happened to "fix" the problem where an incomplete prefix
> > match could be favored over an exact match.
>
> Hmph. Would it require too much further work to do what you said the
> code does:
I was just being overly precise. I *did* fix the problem. But since it was
not my intention, I quoted the verb "fix".
> > The rebase--helper code (specifically, the patch moving autosquash
> > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries
> > to match exact onelines first, and falls back to prefix matching only
> > after that.
>
> If the code matches exact onlines and then falls back to prefix, I do
> not think incomplete prefix would be mistakenly chosen over an exact
> one, so perhaps your code already does the right thing?
The code does exactly that. It does even more: as `fixup! <SHA-1>` is
allowed (for SHA-1s that have been mentioned in previous `pick` lines), it
tries to match that before falling back to the incomplete prefix match.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list
From: Jeff King @ 2017-02-10 15:56 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Stefan Beller, Johannes Schindelin, David Turner, git
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>
On Fri, Feb 10, 2017 at 12:16:10PM +0100, Michael Haggerty wrote:
> This is v2 of the patch series, considerably reorganized but not that
> different codewise. Thanks to Stefan, Junio, and Peff for their
> feedback about v1 [1]. I think I have addressed all of your comments.
>
> Changes since v1:
I read through these and didn't didn't see any problems.
The reordering and new commits made sense to me.
-Peff
^ permalink raw reply
* Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C
From: Pranit Bauva @ 2017-02-10 15:30 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <a1b9143bb29a8a5979dd733ed20161e6769b2b83.1486736391.git.johannes.schindelin@gmx.de>
Hey Johannes,
On Fri, Feb 10, 2017 at 7:50 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
>
> It is curious that only MacOSX builds trigger an error about this, both
> GCC and Clang, but not Linux GCC nor Clang (see
> https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
>
> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
> uninitialized whenever 'if' condition is true
> [-Werror,-Wsometimes-uninitialized]
> if (missing_good && !missing_bad && current_term &&
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
> if (!good_syn)
> ^~~~~~~~
>
> If you "re-roll" (or, as pointed out at the Contributors' Summit, better
> put: if you send another iteration of the patch series), please squash
> this fix in.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Thanks for making this fix! :) I will squash it in.
Regards,
Pranit Bauva
^ permalink raw reply
* Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]
From: Johannes Schindelin @ 2017-02-10 16:10 UTC (permalink / raw)
To: Arif Khokar
Cc: Jakub Narębski, Jeff King, Stefan Beller,
meta@public-inbox.org, git@vger.kernel.org, Eric Wong
In-Reply-To: <alpine.DEB.2.20.1608241509200.4924@virtualbox>
[-- Attachment #1: Type: text/plain, Size: 2311 bytes --]
Hi Arif,
On Wed, 24 Aug 2016, Johannes Schindelin wrote:
> On Tue, 23 Aug 2016, Arif Khokar wrote:
>
> > On 08/20/2016 03:57 PM, Jakub Narębski wrote:
> >
> > > But perhaps the problem is current lack of tooling in the opposite
> > > direction, namely getting patches from mailing list and applying
> > > them to GitHub repo, or Bitbucket, or GitLab. Though with working
> > > Git, it is something easier than sending patches via email; it is
> > > enough that email client can save email to a file (or better, whole
> > > sub-thread to file or files).
> >
> > Given that public-inbox provides an NNTP interface, couldn't the
> > ARTICLE <message-id> NNTP command be used to easily retrieve the
> > messages in a given patch series (at least compared to POP or IMAP).
> > Perhaps git-send-email could be modified to include the message-id
> > value of each patch in the series that it sends to the mailing list
> > and include it in the cover letter.
>
> I am no expert in the NNTP protocol (I abandoned News long ago), but if
> you go from HTML, you can automate the process without requiring changes
> in format-patch.
>
> > Then a script could be written (i.e., git-download-patch) which could
> > parse the cover letter message (specified using its message-id), and
> > download all the patches in series, which can then be applied using
> > git-am. This would in fact take the email client out of the equation
> > in terms of saving patches.
>
> I recently adapted an old script I had to apply an entire patch series
> given the GMane link to its cover letter:
>
> https://github.com/git-for-windows/build-extra/blob/master/apply-from-gmane.sh
>
> Maybe you find it in you to adapt that to work with public-inbox.org?
Oh well. That would have been too easy a task, right?
As it happens, I needed this functionality myself (when reworking my
git-path-in-subdir patch to include Mike Rappazzo's previous patch series
that tried to fix the same bug).
I copy-edited the script to work with public-inbox.org, it accepts a
Message-ID or URL or GMane URL and will try to apply the patch (or patch
series) on top of the current revision:
https://github.com/git-for-windows/build-extra/blob/2268850552c7/apply-from-public-inbox.sh
Ciao,
Johannes
^ permalink raw reply
* Re: Google Doc about the Contributors' Summit
From: Johannes Schindelin @ 2017-02-10 15:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7f4zqiyj.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 9 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > I just started typing stuff up in a Google Doc, and made it editable to
> > everyone, feel free to help and add things:
> >
> > https://docs.google.com/document/d/1KDoSn4btbK5VJCVld32he29U0pUeFGhpFxyx9ZJDDB0/edit?usp=sharing
>
> Thanks for a write-up, Dscho.
Technically, it is not a write-up, and I never meant it to be that. I
intended this document to help me remember what had been discussed, and I
doubt it is useful at all to anybody who has not been there.
I abused the Git mailing list to share that link, what I really should
have done is to use an URL shortener and jot the result down on the
whiteboard.
Very sorry for that,
Johannes
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Jeff King @ 2017-02-10 16:04 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff Hostetler, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1702101647340.3496@virtualbox>
On Fri, Feb 10, 2017 at 04:49:02PM +0100, Johannes Schindelin wrote:
> > I think this is only half the story. A heavy-sha1 workload is faster,
> > which is good. But one of the original reasons to prefer blk-sha1 (at
> > least on Linux) is that resolving libcrypto.so symbols takes a
> > non-trivial amount of time. I just timed it again, and it seems to be
> > consistently 1ms slower to run "git rev-parse --git-dir" on my machine
> > (from the top-level of a repo).
> >
> > 1ms is mostly irrelevant, but it adds up on scripted workloads that
> > start a lot of git processes.
>
> You know my answer to that. If scripting slows things down, we should
> avoid it in production code. As it is, scripting slows us down. Therefore
> I work slowly but steadily to get rid of scripting where it hurts most.
Well, yes. My question is more "what does it look like on normal Git
workloads?". Are you trading off an optimization for your giant 450MB
index workload (which _also_ could be fixed by trying do the slow
operation less, rather than micro-optimizing it) in a way that hurts
people working with more normal sized repos?
For instance, "make BLK_SHA1=Yes test" is measurably faster for me than
"make BLK_SHA1= test".
I'm open to the argument that it doesn't matter in practice for normal
git users. But it's not _just_ scripting. It depends on the user's
pattern of invoking git commands (and how expensive the symbol
resolution is on their system).
-Peff
^ permalink raw reply
* Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list
From: Stefan Beller @ 2017-02-10 16:44 UTC (permalink / raw)
To: Jeff King
Cc: Michael Haggerty, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Johannes Schindelin,
David Turner, git@vger.kernel.org
In-Reply-To: <20170210155651.lecjrzmoux5mcm5d@sigill.intra.peff.net>
On Fri, Feb 10, 2017 at 7:56 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Feb 10, 2017 at 12:16:10PM +0100, Michael Haggerty wrote:
>
>> This is v2 of the patch series, considerably reorganized but not that
>> different codewise. Thanks to Stefan, Junio, and Peff for their
>> feedback about v1 [1]. I think I have addressed all of your comments.
>>
>> Changes since v1:
>
> I read through these and didn't didn't see any problems.
>
> The reordering and new commits made sense to me.
Same here.
Stefan
>
> -Peff
^ permalink raw reply
* [PATCH] fetch: print an error when declining to request an unadvertised object
From: Matt McCutchen @ 2017-02-10 17:26 UTC (permalink / raw)
To: git
Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
allow requests for unadvertised objects by sha1. Change it to print a
meaningful error message.
Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---
The fetch code looks unbelievably complicated to me and I don't understand all
the cases that can arise. Hopefully this patch is an acceptable solution to the
problem.
fetch-pack.c | 31 ++++++++++++++++---------------
t/t5516-fetch-push.sh | 3 ++-
2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 601f077..117874c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -598,23 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
}
/* Append unmatched requests to the list */
- if ((allow_unadvertised_object_request &
- (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
- for (i = 0; i < nr_sought; i++) {
- unsigned char sha1[20];
+ for (i = 0; i < nr_sought; i++) {
+ unsigned char sha1[20];
- ref = sought[i];
- if (ref->matched)
- continue;
- if (get_sha1_hex(ref->name, sha1) ||
- ref->name[40] != '\0' ||
- hashcmp(sha1, ref->old_oid.hash))
- continue;
+ ref = sought[i];
+ if (ref->matched)
+ continue;
+ if (get_sha1_hex(ref->name, sha1) ||
+ ref->name[40] != '\0' ||
+ hashcmp(sha1, ref->old_oid.hash))
+ continue;
- ref->matched = 1;
- *newtail = copy_ref(ref);
- newtail = &(*newtail)->next;
- }
+ if (!(allow_unadvertised_object_request &
+ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)))
+ die(_("Server does not allow request for unadvertised object %s"), ref->name);
+
+ ref->matched = 1;
+ *newtail = copy_ref(ref);
+ newtail = &(*newtail)->next;
}
*refs = newlist;
}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 0fc5a7c..177897e 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1098,7 +1098,8 @@ test_expect_success 'fetch exact SHA1' '
test_must_fail git cat-file -t $the_commit &&
# fetching the hidden object should fail by default
- test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy &&
+ test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
+ test_i18ngrep "Server does not allow request for unadvertised object" err &&
test_must_fail git rev-parse --verify refs/heads/copy &&
# the server side can allow it to succeed
--
2.9.3
^ permalink raw reply related
* Re: Trying to use xfuncname without success.
From: René Scharfe @ 2017-02-10 17:24 UTC (permalink / raw)
To: Jack Adrian Zappa; +Cc: git-mailing-list
In-Reply-To: <CAKepmai0NbQmZ-8KCm71qh4cBd9HOzVD1M87TAMhGPYaH1HsHA@mail.gmail.com>
Am 09.02.2017 um 01:10 schrieb Jack Adrian Zappa:
>> where it has grabbed a line at 126 and is using that for the hunk header.
>
> When I say that, I mean that it is using that line for *every* hunk
> header, for every change, regardless if it has passed a hunk head that
> it should have matched.
Strange. That should only happen if no other function lines are
recognized before the changes. You can check if that's the case using
git grep and your xfuncline regex, e.g. like this:
$ git grep -En "^[\t ]*<Type[\t ]+Name=\"([^\"]+)\".*$" *.natvis
And you can check *that* result with regular grep -E or egrep if it
looks funny.
René
^ permalink raw reply
* Re: [PATCH] fetch: print an error when declining to request an unadvertised object
From: Junio C Hamano @ 2017-02-10 18:36 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1486747828.17272.5.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
> Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
> allow requests for unadvertised objects by sha1. Change it to print a
> meaningful error message.
>
> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
> ---
>
> The fetch code looks unbelievably complicated to me and I don't understand all
> the cases that can arise. Hopefully this patch is an acceptable solution to the
> problem.
At first I thought that this should be caught on the sending end
(grep for "not our ref" in upload-pack.c), but you found a case
where we do not even ask the sender on the requesting side.
I am not convinced that modifying filter_refs() is needed or even
desirable, though.
There is this piece of code near the end of builtin/fetch-pack.c:
/*
* If the heads to pull were given, we should have consumed
* all of them by matching the remote. Otherwise, 'git fetch
* remote no-such-ref' would silently succeed without issuing
* an error.
*/
for (i = 0; i < nr_sought; i++) {
if (!sought[i] || sought[i]->matched)
continue;
error("no such remote ref %s", sought[i]->name);
ret = 1;
}
that happens before the command shows the list of fetched refs, and
this code is prepared to inspect what happend to the requests it (in
response to the user request) made to the underlying fetch
machinery, and issue the error message.
If you change your command line to "git fetch-pack REMOTE SHA1", you
do see an error from the above.
That is a good indication that the underlying fetch machinery called
by this caller is already doing diagnosis that is necessary for the
caller to issue such an error. So why do we fail to say anything in
"git fetch"?
I think the real issue is that when fetch-pack machinery is used via
the transport layer, the transport layer discards the information on
these original request (i.e. what is returned in sought[]).
Instead, the caller of the fetch-pack machinery receives the list of
filtered refs as the return value of fetch_pack() function, and only
looks at "refs" without checking what happened to the original
request to_fetch[] (which corresponds to sought[] in the fetch-pack
command). This all happens in transport.c::fetch_refs_via_pack().
I think that function is a much better place to error or die than
filter_refs().
> fetch-pack.c | 31 ++++++++++++++++---------------
> t/t5516-fetch-push.sh | 3 ++-
> 2 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 601f077..117874c 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -598,23 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
> }
>
> /* Append unmatched requests to the list */
> - if ((allow_unadvertised_object_request &
> - (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
> - for (i = 0; i < nr_sought; i++) {
> - unsigned char sha1[20];
> + for (i = 0; i < nr_sought; i++) {
> + unsigned char sha1[20];
>
> - ref = sought[i];
> - if (ref->matched)
> - continue;
> - if (get_sha1_hex(ref->name, sha1) ||
> - ref->name[40] != '\0' ||
> - hashcmp(sha1, ref->old_oid.hash))
> - continue;
> + ref = sought[i];
> + if (ref->matched)
> + continue;
> + if (get_sha1_hex(ref->name, sha1) ||
> + ref->name[40] != '\0' ||
> + hashcmp(sha1, ref->old_oid.hash))
> + continue;
>
> - ref->matched = 1;
> - *newtail = copy_ref(ref);
> - newtail = &(*newtail)->next;
> - }
> + if (!(allow_unadvertised_object_request &
> + (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)))
> + die(_("Server does not allow request for unadvertised object %s"), ref->name);
> +
> + ref->matched = 1;
> + *newtail = copy_ref(ref);
> + newtail = &(*newtail)->next;
> }
> *refs = newlist;
> }
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 0fc5a7c..177897e 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1098,7 +1098,8 @@ test_expect_success 'fetch exact SHA1' '
> test_must_fail git cat-file -t $the_commit &&
>
> # fetching the hidden object should fail by default
> - test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy &&
> + test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
> + test_i18ngrep "Server does not allow request for unadvertised object" err &&
> test_must_fail git rev-parse --verify refs/heads/copy &&
>
> # the server side can allow it to succeed
^ permalink raw reply
* Re: [PATCHv2] push options: pass push options to the transport helper
From: Stefan Beller @ 2017-02-10 18:44 UTC (permalink / raw)
To: Jonathan Nieder, git@vger.kernel.org
In-Reply-To: <CAFzf2XzLNOKbNbkCfzpVJtN-ROW8m8WVBsWM-z7Bxdnv55b45Q@mail.gmail.com>
On Thu, Feb 9, 2017 at 5:56 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Private reply because using HTML mail on phone:
So I presume putting it back on the list is fine.
> Does this need to be a remote helper capability?
No, as all other capabilities of the git-protocol are mapped 1:1 to
the transport helper protocol for support, e.g. each transport helper
has to handle this on its own, c.f. remote-curl.c:set_option
(line 39 ff and 1065),
> What happens with remote
> helpers that don't understand the option?
The helper ought to print "unsupported"
(remote-curl.c:1065 for the http helper) and then the transport helper
will take care of it (transport-helper.c: 265 and e.g. 820)
>
> How do remote helpers communicate whether the server has said it accepts
> push options?
I guess the remote helper would communicate it the same way as communicating
if the push succeeded? (i.e. reject non fast forward.)
>
> On Wed, Feb 8, 2017, 14:04 Stefan Beller <sbeller@google.com> wrote:
>>
>> When using non-builtin protocols relying on a transport helper
>> (such as http), push options are not propagated to the helper.
>>
>> The user could ask for push options and a push would seemingly succeed,
>> but the push options would never be transported to the server,
>> misleading the users expectation.
>>
>> Fix this by propagating the push options to the transport helper.
>>
>> This is only addressing the first issue of
>> (1) the helper protocol does not propagate push-option
>> (2) the http helper is not prepared to handle push-option
>>
>> Once we fix (2), the http transport helper can make use of push options
>> as well, but that happens as a follow up. (1) is a bug fix, whereas (2)
>> is a feature, which is why we only do (1) here.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> Documentation/gitremote-helpers.txt | 4 ++++
>> t/t5545-push-options.sh | 15 +++++++++++++++
>> transport-helper.c | 7 +++++++
>> 3 files changed, 26 insertions(+)
>>
>> diff --git a/Documentation/gitremote-helpers.txt
>> b/Documentation/gitremote-helpers.txt
>> index 9e8681f9e1..23474b1eab 100644
>> --- a/Documentation/gitremote-helpers.txt
>> +++ b/Documentation/gitremote-helpers.txt
>> @@ -462,6 +462,10 @@ set by Git if the remote helper has the 'option'
>> capability.
>> 'option pushcert {'true'|'false'}::
>> GPG sign pushes.
>>
>> +'option push-option <string>::
>> + Transmit <string> as a push option. As the a push option
>> + must not contain LF or NUL characters, the string is not encoded.
>> +
>> SEE ALSO
>> --------
>> linkgit:git-remote[1]
>> diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
>> index ea813b9383..9a57a7d8f2 100755
>> --- a/t/t5545-push-options.sh
>> +++ b/t/t5545-push-options.sh
>> @@ -3,6 +3,8 @@
>> test_description='pushing to a repository using push options'
>>
>> . ./test-lib.sh
>> +. "$TEST_DIRECTORY"/lib-httpd.sh
>> +start_httpd
>>
>> mk_repo_pair () {
>> rm -rf workbench upstream &&
>> @@ -100,4 +102,17 @@ test_expect_success 'two push options work' '
>> test_cmp expect upstream/.git/hooks/post-receive.push_options
>> '
>>
>> +test_expect_success 'push option denied properly by http remote helper'
>> '\
>> + mk_repo_pair &&
>> + git -C upstream config receive.advertisePushOptions false &&
>> + git -C upstream config http.receivepack true &&
>> + cp -R upstream/.git "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git &&
>> + git clone "$HTTPD_URL"/smart/upstream test_http_clone &&
>> + test_commit -C test_http_clone one &&
>> + test_must_fail git -C test_http_clone push --push-option=asdf
>> origin master &&
>> + git -C test_http_clone push origin master
>> +'
>> +
>> +stop_httpd
>> +
>> test_done
>> diff --git a/transport-helper.c b/transport-helper.c
>> index 91aed35ebb..1258d6aedd 100644
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -826,6 +826,13 @@ static void set_common_push_options(struct transport
>> *transport,
>> if (set_helper_option(transport, TRANS_OPT_PUSH_CERT,
>> "if-asked") != 0)
>> die("helper %s does not support
>> --signed=if-asked", name);
>> }
>> +
>> + if (flags & TRANSPORT_PUSH_OPTIONS) {
>> + struct string_list_item *item;
>> + for_each_string_list_item(item, transport->push_options)
>> + if (set_helper_option(transport, "push-option",
>> item->string) != 0)
>> + die("helper %s does not support
>> 'push-option'", name);
>> + }
>> }
>>
>> static int push_refs_with_push(struct transport *transport,
>> --
>> 2.12.0.rc0.1.g018cb5e6f4
>>
>
^ permalink raw reply
* Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory
From: Junio C Hamano @ 2017-02-10 18:50 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Michael Rappazzo, Nguyễn Thái Ngọc Duy
In-Reply-To: <cc23463af8096c5f8429f939ce881cf0eb5c2dcd.1486740772.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> From: Michael Rappazzo <rappazzo@gmail.com>
>
> t2027-worktree-list has an incorrect expectation for --git-common-dir
> which has been adjusted and marked to expect failure.
>
> Some of the tests added have been marked to expect failure. These
> demonstrate a problem with the way that some options to git rev-parse
> behave when executed from a subdirectory of the main worktree.
>
> [jes: fixed incorrect assumption that objects/ lives in the
> worktree-specific git-dir (it lives in the common dir instead).]
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
Just one iffy part; otherwise looks good.
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 038e24c4014..f39f783f2db 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru
>
> test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
>
> +test_expect_success 'git-common-dir from worktree root' '
> + echo .git >expect &&
> + git rev-parse --git-common-dir >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_failure 'git-common-dir inside sub-dir' '
> + mkdir -p path/to/child &&
> + test_when_finished "rm -rf path" &&
> + echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
> + git -C path/to/child rev-parse --git-common-dir >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'git-path from worktree root' '
> + echo .git/objects >expect &&
> + git rev-parse --git-path objects >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_failure 'git-path inside sub-dir' '
> + mkdir -p path/to/child &&
> + test_when_finished "rm -rf path" &&
> + echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
> + git -C path/to/child rev-parse --git-path objects >actual &&
> + test_cmp expect actual
> +'
All of these look sensible.
> test_done
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 292a0720fcc..1d6e27a09d8 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -200,4 +200,21 @@ EOF
> test_cmp expect actual
> '
>
> +test_expect_failure 'rev-parse --shared-index-path' '
> + rm -rf .git &&
> + test_create_repo . &&
> + git update-index --split-index &&
> + ls -t .git/sharedindex* | tail -n 1 >expect &&
> + git rev-parse --shared-index-path >actual &&
> + test_cmp expect actual &&
> + mkdir work &&
> + test_when_finished "rm -rf work" &&
> + (
> + cd work &&
> + ls -t ../.git/sharedindex* | tail -n 1 >expect &&
> + git rev-parse --shared-index-path >actual &&
> + test_cmp expect actual
> + )
This looks iffy. If we expect multiple sharedindex* files, the
first output from "ls -t" may or may not match the real one in use
(multiple things do happen within a single second or whatever your
filesystem's time granularity is). Two "ls -t" run in this test
would (hopefully) give stable results, but I suspect that the chance
the first line in the output matches what --shared-index-path reports
is 50% if we expect to have two sharedindex* files.
On the other hand, if we do not expect multiple sharedindex* files,
use of "ls" piped to "tail" is simply misleading.
If this test can be written in such a way that there is only one
such file that match the pattern, it would be the cleanest to
understand and explain. As there is only a single invocation of
"update-index --split-index" immediately after a new repository is
created, I suspect that the expectation to see only one sharedindex*
file already holds (because its name is unpredictable, we still need
to catch it with wildcard), and if that is the case, we can just
lose "-t" and pipe to tail, i.e. "ls .git/sharedindex* >expect".
> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 465eeeacd3d..c1a072348e7 100755
> --- a/t/t2027-worktree-list.sh
> +++ b/t/t2027-worktree-list.sh
> @@ -8,16 +8,24 @@ test_expect_success 'setup' '
> test_commit init
> '
>
> -test_expect_success 'rev-parse --git-common-dir on main worktree' '
> +test_expect_failure 'rev-parse --git-common-dir on main worktree' '
> git rev-parse --git-common-dir >actual &&
> echo .git >expected &&
> test_cmp expected actual &&
> mkdir sub &&
> git -C sub rev-parse --git-common-dir >actual2 &&
> - echo sub/.git >expected2 &&
> + echo ../.git >expected2 &&
> test_cmp expected2 actual2
> '
OK, this swaps a wrong expectation with a more usable one.
> +test_expect_failure 'rev-parse --git-path objects linked worktree' '
> + echo "$(git rev-parse --show-toplevel)/.git/objects" >expect &&
> + test_when_finished "rm -rf linked-tree && git worktree prune" &&
> + git worktree add --detach linked-tree master &&
> + git -C linked-tree rev-parse --git-path objects >actual &&
> + test_cmp expect actual
> +'
> +
Looking good.
Thanks.
^ permalink raw reply
* [PATCH 0/2 v3] WIP: allow "-" as a shorthand for "previous branch"
From: Siddharth Kannan @ 2017-02-10 18:55 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
Thanks a lot, Matthieu, for your comments on an earlier version of this
patch! [1]
After the discussion there, I have considered the changes that have been made
and I broke them into two separate commits.
I have included the list of commands that are touched by this patch series in
the second commit message. (idea from the v1 discussion [2]) Reproduced here:
* builtin/blame.c
* builtin/diff.c
* builtin/diff-files.c
* builtin/diff-index.c
* builtin/diff-tree.c
* builtin/log.c
* builtin/rev-list.c
* builtin/shortlog.c
* builtin/fast-export.c
* builtin/fmt-merge-msg.c
builtin/add.c
builtin/checkout.c
builtin/commit.c
builtin/merge.c
builtin/pack-objects.c
builtin/revert.c
* marked commands are information-only.
I have added the WIP tag because I am still unsure if the tests that I have
added (for git-log) are sufficient for this patch or more comprehensive tests
need to be added. So, please help me with some feedback on that.
I have removed the "log:" tag from the subject line because this patch now
affects commands other than log.
I have run the test suite locally and on Travis CI! [3]
[1]: https://public-inbox.org/git/vpqh944eof7.fsf@anie.imag.fr/#t
[2]: https://public-inbox.org/git/CAN-3QhoZN_wYvqbVdU_c1h4vUOaT5FOBFL7k+FemNpqkxjWDDA@mail.gmail.com/
[3]: https://travis-ci.org/icyflame/git/builds/200431159
Siddharth Kannan (2):
revision.c: args starting with "-" might be a revision
sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"
revision.c | 12 ++++++--
sha1_name.c | 5 ++++
t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 88 insertions(+), 2 deletions(-)
create mode 100755 t/t4214-log-shorthand.sh
--
2.1.4
^ permalink raw reply
* [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision
From: Siddharth Kannan @ 2017-02-10 18:55 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1486752926-12020-1-git-send-email-kannan.siddharth12@gmail.com>
setup_revisions used to consider any argument starting with "-" to be either a
valid option or nothing at all. This patch will teach it to check if the
argument is a revision before declaring that it is nothing at all.
Before this patch, handle_revision_arg was not called for arguments starting
with "-" and once for arguments that didn't start with "-". Now, it will be
called once per argument.
This patch prepares the addition of "-" as a shorthand for "previous branch".
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index b37dbec..4131ad5 100644
--- a/revision.c
+++ b/revision.c
@@ -2205,6 +2205,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
read_from_stdin = 0;
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
+ int handle_rev_arg_called = 0, args;
if (*arg == '-') {
int opts;
@@ -2234,11 +2235,18 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
- continue;
+
+ args = handle_revision_arg(arg, revs, flags, revarg_opt);
+ handle_rev_arg_called = 1;
+ if (args)
+ continue;
+ else
+ --left;
}
- if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+ if ((handle_rev_arg_called && args) ||
+ handle_revision_arg(arg, revs, flags, revarg_opt)) {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
--
2.1.4
^ permalink raw reply related
* [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Siddharth Kannan @ 2017-02-10 18:55 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1486752926-12020-2-git-send-email-kannan.siddharth12@gmail.com>
This patch introduces "-" as a method to refer to a revision, and adds tests to
test that git-log works with this shorthand.
This change will touch the following commands (through
revision.c:setup_revisions):
* builtin/blame.c
* builtin/diff.c
* builtin/diff-files.c
* builtin/diff-index.c
* builtin/diff-tree.c
* builtin/log.c
* builtin/rev-list.c
* builtin/shortlog.c
* builtin/fast-export.c
* builtin/fmt-merge-msg.c
builtin/add.c
builtin/checkout.c
builtin/commit.c
builtin/merge.c
builtin/pack-objects.c
builtin/revert.c
* marked commands are information-only.
As most commands in this list are not of the rm-variety, (i.e a command that
would delete something), this change does not make it easier for people to
delete. (eg: "git branch -d -" is *not* enabled by this patch)
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
sha1_name.c | 5 ++++
t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+)
create mode 100755 t/t4214-log-shorthand.sh
diff --git a/sha1_name.c b/sha1_name.c
index 73a915f..d774e46 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
if (!ret)
return 0;
+ if (!strcmp(name, "-")) {
+ name = "@{-1}";
+ len = 5;
+ }
+
ret = get_sha1_basic(name, len, sha1, lookup_flags);
if (!ret)
return 0;
diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
new file mode 100755
index 0000000..dec966c
--- /dev/null
+++ b/t/t4214-log-shorthand.sh
@@ -0,0 +1,73 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ echo hello >world &&
+ git add world &&
+ git commit -m initial &&
+ echo "hello second time" >>world &&
+ git add world &&
+ git commit -m second &&
+ echo "hello other file" >>planet &&
+ git add planet &&
+ git commit -m third &&
+ echo "hello yet another file" >>city &&
+ git add city &&
+ git commit -m fourth
+'
+
+test_expect_success '"log -" should not work initially' '
+ test_must_fail git log -
+'
+
+test_expect_success '"log -" should work' '
+ git checkout -b testing-1 master^ &&
+ git checkout -b testing-2 master~2 &&
+ git checkout master &&
+
+ git log testing-2 >expect &&
+ git log - >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'symmetric revision range should work when one end is left empty' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log ...@{-1} > expect.first_empty &&
+ git log @{-1}... > expect.last_empty &&
+ git log ...- > actual.first_empty &&
+ git log -... > actual.last_empty &&
+ test_cmp expect.first_empty actual.first_empty &&
+ test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'asymmetric revision range should work when one end is left empty' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log ..@{-1} > expect.first_empty &&
+ git log @{-1}.. > expect.last_empty &&
+ git log ..- > actual.first_empty &&
+ git log -.. > actual.last_empty &&
+ test_cmp expect.first_empty actual.first_empty &&
+ test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'symmetric revision range should work when both ends are given' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log -...testing-1 >expect &&
+ git log testing-2...testing-1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'asymmetric revision range should work when both ends are given' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log -..testing-1 >expect &&
+ git log testing-2..testing-1 >actual &&
+ test_cmp expect actual
+'
+test_done
--
2.1.4
^ permalink raw reply related
* Re: [PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory
From: Junio C Hamano @ 2017-02-10 18:57 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Nguyễn Thái Ngọc Duy, Michael Rappazzo
In-Reply-To: <9242ee9717dcec95351b356070102f198eeb2537.1486740772.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index ff13e59e1db..84af2802f6f 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> unsigned int flags = 0;
> const char *name = NULL;
> struct object_context unused;
> + struct strbuf buf = STRBUF_INIT;
>
> if (argc > 1 && !strcmp("--parseopt", argv[1]))
> return cmd_parseopt(argc - 1, argv + 1, prefix);
> @@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> if (!strcmp(arg, "--git-path")) {
> if (!argv[i + 1])
> die("--git-path requires an argument");
> - puts(git_path("%s", argv[i + 1]));
> + strbuf_reset(&buf);
> + puts(relative_path(git_path("%s", argv[i + 1]),
> + prefix, &buf));
> i++;
> continue;
> }
> @@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> continue;
> }
> if (!strcmp(arg, "--git-common-dir")) {
> - const char *pfx = prefix ? prefix : "";
> - puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
> + strbuf_reset(&buf);
> + puts(relative_path(get_git_common_dir(),
> + prefix, &buf));
> continue;
> }
> if (!strcmp(arg, "--is-inside-git-dir")) {
> @@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> die(_("Could not read the index"));
> if (the_index.split_index) {
> const unsigned char *sha1 = the_index.split_index->base_sha1;
> - puts(git_path("sharedindex.%s", sha1_to_hex(sha1)));
> + const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
> + strbuf_reset(&buf);
> + puts(relative_path(path, prefix, &buf));
> }
> continue;
> }
> @@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> die_no_single_rev(quiet);
> } else
> show_default();
> + strbuf_release(&buf);
This uses "reset then use" pattern for repeated use of strbuf, and
causes the string last held in the strbuf to leak on early return,
which can be mitigated by using "use then reset" pattern. I.e.
if (!strcmp(arg, "--git-common-dir")) {
puts(relative_path(get_git_common_dir(),
prefix, &buf));
strbuf_reset(&buf);
continue;
}
I'd think. You'd still want to release it at the end anyway for
good code hygiene, though.
Other than that, looks good to me.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/2] Fix bugs in rev-parse's output when run in a subdirectory
From: Junio C Hamano @ 2017-02-10 18:59 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Nguyễn Thái Ngọc Duy, Michael Rappazzo
In-Reply-To: <cover.1486740772.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Johannes Schindelin (1):
> rev-parse: fix several options when running in a subdirectory
>
> Michael Rappazzo (1):
> rev-parse tests: add tests executed from a subdirectory
>
> builtin/rev-parse.c | 15 +++++++++++----
> t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++
> t/t1700-split-index.sh | 17 +++++++++++++++++
> t/t2027-worktree-list.sh | 10 +++++++++-
> 4 files changed, 65 insertions(+), 5 deletions(-)
>
>
> base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
> Published-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v2
> Fetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v2
Will queue as js/git-path-in-subdir topic forked at 2.12-rc0.
I still think the log message in 2/2 is making a mountain out of
molehill and showing a skewed perception on pros-and-cons in a
design decision, but I won't repeat my review. I saw a few
correctness issues and pointed them out on the patches.
^ permalink raw reply
* Re: Bug with fixup and autosquash
From: Philip Oakley @ 2017-02-10 19:02 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano
Cc: Ashutosh Bapat, git, Michael Haggerty, Michael J Gruber,
Matthieu Moy
In-Reply-To: <alpine.DEB.2.20.1702101654500.3496@virtualbox>
From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> Hi Junio,
>
> On Thu, 9 Feb 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > Almost. While I fixed the performance issues as well as the design
>> > allowed, I happened to "fix" the problem where an incomplete prefix
>> > match could be favored over an exact match.
>>
>> Hmph. Would it require too much further work to do what you said the
>> code does:
>
> I was just being overly precise. I *did* fix the problem. But since it was
> not my intention, I quoted the verb "fix".
>
>> > The rebase--helper code (specifically, the patch moving autosquash
>> > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries
>> > to match exact onelines first, and falls back to prefix matching only
>> > after that.
>>
>> If the code matches exact onlines and then falls back to prefix, I do
>> not think incomplete prefix would be mistakenly chosen over an exact
>> one, so perhaps your code already does the right thing?
>
> The code does exactly that. It does even more: as `fixup! <SHA-1>` is
> allowed (for SHA-1s that have been mentioned in previous `pick` lines), it
> tries to match that before falling back to the incomplete prefix match.
>
> Ciao,
> Johannes
Now just the doc update to do.... ;-)
I definitely think the 'fix' that allows the `fixup! <SHA-1>` as the subject
line is a good way to go for those who mix in the use of the gui and gitk
into their workflow (*)
--
Philip
(*) I just don't see the point of having multiple cli tty windows, and then
not using the gui/k windows that are part of the tool set.
^ permalink raw reply
* Re: [PATCH v2 1/9] refs: reorder some function definitions
From: Junio C Hamano @ 2017-02-10 19:14 UTC (permalink / raw)
To: Michael Haggerty
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git
In-Reply-To: <661ef87844918501e84b43d254305e1997af9b57.1486724698.git.mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> This avoids the need to add forward declarations in the next step.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> refs.c | 64 ++++++++++++++++++++++++++++++++--------------------------------
> 1 file changed, 32 insertions(+), 32 deletions(-)
Makes sense, but the patch itself looks like ... unreadble ;-)
>
> diff --git a/refs.c b/refs.c
> index 9bd0bc1..707092f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1358,27 +1358,19 @@ static struct ref_store *main_ref_store;
> /* A linked list of ref_stores for submodules: */
> static struct ref_store *submodule_ref_stores;
>
> -void base_ref_store_init(struct ref_store *refs,
> - const struct ref_storage_be *be,
> - const char *submodule)
> +struct ref_store *lookup_ref_store(const char *submodule)
> {
> - refs->be = be;
> - if (!submodule) {
> - if (main_ref_store)
> - die("BUG: main_ref_store initialized twice");
> + struct ref_store *refs;
>
> - refs->submodule = "";
> - refs->next = NULL;
> - main_ref_store = refs;
> - } else {
> - if (lookup_ref_store(submodule))
> - die("BUG: ref_store for submodule '%s' initialized twice",
> - submodule);
> + if (!submodule || !*submodule)
> + return main_ref_store;
>
> - refs->submodule = xstrdup(submodule);
> - refs->next = submodule_ref_stores;
> - submodule_ref_stores = refs;
> + for (refs = submodule_ref_stores; refs; refs = refs->next) {
> + if (!strcmp(submodule, refs->submodule))
> + return refs;
> }
> +
> + return NULL;
> }
>
> struct ref_store *ref_store_init(const char *submodule)
> @@ -1395,21 +1387,6 @@ struct ref_store *ref_store_init(const char *submodule)
> return be->init(submodule);
> }
>
> -struct ref_store *lookup_ref_store(const char *submodule)
> -{
> - struct ref_store *refs;
> -
> - if (!submodule || !*submodule)
> - return main_ref_store;
> -
> - for (refs = submodule_ref_stores; refs; refs = refs->next) {
> - if (!strcmp(submodule, refs->submodule))
> - return refs;
> - }
> -
> - return NULL;
> -}
> -
> struct ref_store *get_ref_store(const char *submodule)
> {
> struct ref_store *refs;
> @@ -1435,6 +1412,29 @@ struct ref_store *get_ref_store(const char *submodule)
> return refs;
> }
>
> +void base_ref_store_init(struct ref_store *refs,
> + const struct ref_storage_be *be,
> + const char *submodule)
> +{
> + refs->be = be;
> + if (!submodule) {
> + if (main_ref_store)
> + die("BUG: main_ref_store initialized twice");
> +
> + refs->submodule = "";
> + refs->next = NULL;
> + main_ref_store = refs;
> + } else {
> + if (lookup_ref_store(submodule))
> + die("BUG: ref_store for submodule '%s' initialized twice",
> + submodule);
> +
> + refs->submodule = xstrdup(submodule);
> + refs->next = submodule_ref_stores;
> + submodule_ref_stores = refs;
> + }
> +}
> +
> void assert_main_repository(struct ref_store *refs, const char *caller)
> {
> if (*refs->submodule)
^ permalink raw reply
* [PATCH v3] gc: ignore old gc.log files
From: David Turner @ 2017-02-10 19:20 UTC (permalink / raw)
To: git; +Cc: peff, pclouds, David Turner
Git should never get itself into a state where it refuses to do any
maintenance, just because at some point some piece of the maintenance
didn't make progress.
In this commit, git learns to ignore gc.log files which are older than
(by default) one day old. It also learns about a config, gc.logExpiry
to manage this. There is also some cleanup: a successful manual gc,
or a warning-free auto gc with an old log file, will remove any old
gc.log files.
It might still happen that manual intervention is required
(e.g. because the repo is corrupt), but at the very least it won't be
because Git is too dumb to try again.
Automatic gc was intended to make client repositories be
self-maintaining. It would be good if automatic gc were also useful
to server operators. A server can end up in a state whre there are
lots of unreferenced loose objects (say, because many users are doing
a bunch of rebasing and pushing their rebased branches). Before this
patch, this state would cause a gc.log file to be created, preventing
future auto gcs. Then pack files could pile up. Since many git
operations are O(n) in the number of pack files, this would lead to
poor performance. Now, the pack files will get cleaned up, if
necessary, at least once per day. And operators who find a need for
more-frequent gcs can adjust gc.logExpiry to meet their needs.
Signed-off-by: David Turner <dturner@twosigma.com>
Helped-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 6 ++++
builtin/gc.c | 75 +++++++++++++++++++++++++++++++++++++++++++-----
t/t6500-gc.sh | 13 +++++++++
3 files changed, 87 insertions(+), 7 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index fc5a28a32..a684b7e3e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1402,6 +1402,12 @@ gc.autoDetach::
Make `git gc --auto` return immediately and run in background
if the system supports it. Default is true.
+gc.logExpiry::
+ If the file gc.log exists, then `git gc --auto` won't run
+ unless that file is more than 'gc.logExpiry' old. Default is
+ "1.day". See `gc.pruneExpire` for more ways to specify its
+ value.
+
gc.packRefs::
Running `git pack-refs` in a repository renders it
unclonable by Git versions prior to 1.5.1.2 over dumb
diff --git a/builtin/gc.c b/builtin/gc.c
index 331f21926..6f297aa91 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -33,6 +33,7 @@ static int aggressive_window = 250;
static int gc_auto_threshold = 6700;
static int gc_auto_pack_limit = 50;
static int detach_auto = 1;
+static unsigned long gc_log_expire_time;
static const char *prune_expire = "2.weeks.ago";
static const char *prune_worktrees_expire = "3.months.ago";
@@ -76,10 +77,47 @@ static void git_config_date_string(const char *key, const char **output)
static void process_log_file(void)
{
struct stat st;
- if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)
+ if (fstat(get_lock_file_fd(&log_lock), &st)) {
+ if (errno == ENOENT) {
+ /*
+ * The user has probably intentionally deleted
+ * gc.log.lock (perhaps because they're blowing
+ * away the whole repo), so thre's no need to
+ * report anything here. But we also won't
+ * delete gc.log, because we don't know what
+ * the user's intentions are.
+ */
+ } else {
+ FILE *fp;
+ int fd;
+ int saved_errno = errno;
+ /*
+ * Perhaps there was an i/o error or another
+ * unlikely situation. Try to make a note of
+ * this in gc.log. If this fails again,
+ * give up and leave gc.log as it was.
+ */
+ rollback_lock_file(&log_lock);
+ fd = hold_lock_file_for_update(&log_lock,
+ git_path("gc.log"),
+ LOCK_DIE_ON_ERROR);
+
+ fp = fdopen(fd, "w");
+ fprintf(fp, _("Failed to fstat %s: %s"),
+ get_tempfile_path(&log_lock.tempfile),
+ strerror(errno));
+ fclose(fp);
+ commit_lock_file(&log_lock);
+ }
+
+ } else if (st.st_size) {
+ /* There was some error recorded in the lock file */
commit_lock_file(&log_lock);
- else
+ } else {
+ /* No error, clean up any old gc.log */
+ unlink(git_path("gc.log"));
rollback_lock_file(&log_lock);
+ }
}
static void process_log_file_at_exit(void)
@@ -113,6 +151,9 @@ static void gc_config(void)
git_config_get_bool("gc.autodetach", &detach_auto);
git_config_date_string("gc.pruneexpire", &prune_expire);
git_config_date_string("gc.worktreepruneexpire", &prune_worktrees_expire);
+ if (!git_config_get_value("gc.logexpiry", &value))
+ parse_expiry_date(value, &gc_log_expire_time);
+
git_config(git_default_config, NULL);
}
@@ -290,19 +331,34 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
static int report_last_gc_error(void)
{
struct strbuf sb = STRBUF_INIT;
- int ret;
+ int ret = 0;
+ struct stat st;
+ char *gc_log_path = git_pathdup("gc.log");
+
+ if (stat(gc_log_path, &st)) {
+ if (errno == ENOENT)
+ goto done;
+
+ ret = error_errno(_("Can't stat %s"), gc_log_path);
+ goto done;
+ }
+
+ if (st.st_mtime < gc_log_expire_time)
+ goto done;
- ret = strbuf_read_file(&sb, git_path("gc.log"), 0);
+ ret = strbuf_read_file(&sb, gc_log_path, 0);
if (ret > 0)
- return error(_("The last gc run reported the following. "
+ ret = error(_("The last gc run reported the following. "
"Please correct the root cause\n"
"and remove %s.\n"
"Automatic cleanup will not be performed "
"until the file is removed.\n\n"
"%s"),
- git_path("gc.log"), sb.buf);
+ gc_log_path, sb.buf);
strbuf_release(&sb);
- return 0;
+done:
+ free(gc_log_path);
+ return ret;
}
static int gc_before_repack(void)
@@ -349,6 +405,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
argv_array_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL);
argv_array_pushl(&rerere, "rerere", "gc", NULL);
+ /* default expiry time, overwritten in gc_config */
+ parse_expiry_date("1.day", &gc_log_expire_time);
gc_config();
if (pack_refs < 0)
@@ -448,5 +506,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
warning(_("There are too many unreachable loose objects; "
"run 'git prune' to remove them."));
+ if (!daemonized)
+ unlink(git_path("gc.log"));
+
return 0;
}
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 1762dfa6a..84ad07eb2 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -67,5 +67,18 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre
test_line_count = 2 new # There is one new pack and its .idx
'
+test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '
+ keep=$(ls .git/objects/pack/*.pack|head -1|sed -e "s/pack$/keep/") &&
+ test_commit foo &&
+ test_commit bar &&
+ git repack &&
+ test_config gc.autopacklimit 1 &&
+ test_config gc.autodetach true &&
+ echo fleem >.git/gc.log &&
+ test_must_fail git gc --auto 2>err &&
+ test_i18ngrep "^error:" err &&
+ test-chmtime =-86401 .git/gc.log &&
+ git gc --auto
+'
test_done
--
2.11.GIT
^ permalink raw reply related
* fuzzy patch application
From: Nick Desaulniers @ 2017-02-10 19:20 UTC (permalink / raw)
To: git
I frequently need to backport patches from the Linux kernel to older
kernel versions (Android Security). My usual workflow for simple
patches is:
1. try `git am patch.txt`.
2. if that fails try `git apply -v patch.txt` then add commit message
manually.
3. if that fails try `patch -p1 < patch.txt` then add commit message
manually.
4. if that fails, manually fix bug based on patch.
My question is, why does `patch` seem to do a better job at applying
patches than `git am`? It's almost like the `git` tools don't try to fuzz
the offsets. Is there a way to tell git to try fuzzing offsets when
applying patches?
From the output of `patch` I ran recently (for a patch that
`git apply` would not apply):
patching file <filename.c>
Hunk #1 succeeded at 113 (offset -1 lines).
Hunk #2 succeeded at 435 (offset -3 lines).
Hunk #3 succeeded at 693 (offset 2 lines).
Hunk #4 succeeded at 1535 (offset -41 lines).
Hunk #5 succeeded at 1551 (offset -41 lines).
Hunk #6 succeeded at 1574 with fuzz 2 (offset -42 lines).
Hunk #7 succeeded at 1614 (offset -42 lines).
In fact, `git apply -v` errors for hunk #6.
I would guess maybe that there's an upper limit on the offset searched?
Also, I'm not sure what the `fuzz 2` part means exactly, but it seems like
`git apply` chokes when fuzzing is needed to properly apply a patch.
http://stackoverflow.com/q/6336440/1027966
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()
From: Junio C Hamano @ 2017-02-10 19:22 UTC (permalink / raw)
To: Michael Haggerty
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git
In-Reply-To: <d8e906d969700acbca8dc717673d0a9cdc910f62.1486724698.git.mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> There is no need to call read_ref_full() or resolve_gitlink_ref() from
> read_loose_refs(), because we already have a ref_store object in hand.
> So we can call resolve_ref_recursively() ourselves. Happily, this
> unifies the code for the submodule vs. non-submodule cases.
>
> This requires resolve_ref_recursively() to be exposed to the refs
> subsystem, though not to non-refs code.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> refs.c | 50 +++++++++++++++++++++++++-------------------------
> refs/files-backend.c | 18 ++++--------------
> refs/refs-internal.h | 5 +++++
> 3 files changed, 34 insertions(+), 39 deletions(-)
OK, but one thing puzzles me...
> @@ -1390,27 +1390,6 @@ static struct ref_store *main_ref_store;
> static struct hashmap submodule_ref_stores;
>
> /*
> - * Return the ref_store instance for the specified submodule (or the
> - * main repository if submodule is NULL). If that ref_store hasn't
> - * been initialized yet, return NULL.
> - */
> -static struct ref_store *lookup_ref_store(const char *submodule)
> -{
> - struct submodule_hash_entry *entry;
> -
> - if (!submodule)
> - return main_ref_store;
> -
> - if (!submodule_ref_stores.tablesize)
> - /* It's initialized on demand in register_ref_store(). */
> - return NULL;
> -
> - entry = hashmap_get_from_hash(&submodule_ref_stores,
> - strhash(submodule), submodule);
> - return entry ? entry->refs : NULL;
> -}
> -
> -/*
> * Register the specified ref_store to be the one that should be used
> * for submodule (or the main repository if submodule is NULL). It is
> * a fatal error to call this function twice for the same submodule.
> @@ -1451,6 +1430,27 @@ static struct ref_store *ref_store_init(const char *submodule)
> return refs;
> }
>
> +/*
> + * Return the ref_store instance for the specified submodule (or the
> + * main repository if submodule is NULL). If that ref_store hasn't
> + * been initialized yet, return NULL.
> + */
> +static struct ref_store *lookup_ref_store(const char *submodule)
> +{
> + struct submodule_hash_entry *entry;
> +
> + if (!submodule)
> + return main_ref_store;
> +
> + if (!submodule_ref_stores.tablesize)
> + /* It's initialized on demand in register_ref_store(). */
> + return NULL;
> +
> + entry = hashmap_get_from_hash(&submodule_ref_stores,
> + strhash(submodule), submodule);
> + return entry ? entry->refs : NULL;
> +}
> +
I somehow thought that we had an early "reorder the code" step to
avoid hunks like these? Am I missing some subtle changes made while
moving the function down?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox