Re: Resumable git clone?
On Wed, Mar 02, 2016 at 02:37:53PM +0700, Duy Nguyen wrote:
> On Wed, Mar 2, 2016 at 1:31 PM, Junio C Hamano wrote:
> > Al Viro writes:
> >
> >> FWIW, I wasn't proposing to recreate the remaining bits of that _pack_;
> >> just do the normal pull with one addition: start with sending the list
> >> of sha1 of objects you are about to send and let the recepient reply
> >> with "I already have , don't bother with those". And exclude
> >> those from the transfer.
> >
> > I did a quick-and-dirty unscientific experiment.
> >
> > I had a clone of Linus's repository that was about a week old, whose
> > tip was at 4de8ebef (Merge tag 'trace-fixes-v4.5-rc5' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace,
> > 2016-02-22). To bring it up to date (i.e. a pull about a week's
> > worth of progress) to f691b77b (Merge branch 'for-linus' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs, 2016-03-01):
> >
> > $ git rev-list --objects 4de8ebef..f691b77b1fc | wc -l
> > 1396
> > $ git rev-parse 4de8ebef..f691b77b1fc |
> > git pack-objects --revs --delta-base-offset --stdout |
> > wc -c
> > 2444127
> >
> > So in order to salvage some transfer out of 2.4MB, the hypothetical
> > Al protocol would first have the upload-pack give 20*1396 = 28kB
>
> It could be 10*1396 or less. If the server calculates the shortest
> unambiguous SHA-1 length (quite cheap on fully packed repo) and sends
> it to the client, the client can just sends short SHA-1 instead. It's
> racy though because objects are being added to the server and abbrev
> length may go up. But we can check ambiguity for all SHA-1 sent by
> client and ask for resend for ambiguous ones.
>
> On my linux-2.6.git, 10 letters (so 5 bytes) are needed for
> unambiguous short SHA-1. But we can even go optimistic and ask the
> client for shorter SHA-1 with hope that resend won't be many.
I don't think it's worth the trouble and ambiguity to send abbreviated
object names over the wire. I think several simpler optimizations seem
preferable, such as binary object names, and abbreviating complete
object sets ("I have these commits/trees and everything they need
recursively; I also have this stack of random objects.").
That would work especially well for resumable pull, or for the case of
optimizing pull during the merge window.
- Josh Triplett
--
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: Resumable git clone?
On Wed, Mar 2, 2016 at 2:37 PM, Duy Nguyen wrote: >> So in order to salvage some transfer out of 2.4MB, the hypothetical >> Al protocol would first have the upload-pack give 20*1396 = 28kB > > It could be 10*1396 or less Oops somehow I read previous mails as client sends SHA-1 to server, not the other way around that you and Al were talking about. But the same principle applies to the other direction, I think. -- 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: Resumable git clone?
On Wed, Mar 2, 2016 at 1:31 PM, Junio C Hamano wrote: > Al Viro writes: > >> FWIW, I wasn't proposing to recreate the remaining bits of that _pack_; >> just do the normal pull with one addition: start with sending the list >> of sha1 of objects you are about to send and let the recepient reply >> with "I already have , don't bother with those". And exclude >> those from the transfer. > > I did a quick-and-dirty unscientific experiment. > > I had a clone of Linus's repository that was about a week old, whose > tip was at 4de8ebef (Merge tag 'trace-fixes-v4.5-rc5' of > git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace, > 2016-02-22). To bring it up to date (i.e. a pull about a week's > worth of progress) to f691b77b (Merge branch 'for-linus' of > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs, 2016-03-01): > > $ git rev-list --objects 4de8ebef..f691b77b1fc | wc -l > 1396 > $ git rev-parse 4de8ebef..f691b77b1fc | > git pack-objects --revs --delta-base-offset --stdout | > wc -c > 2444127 > > So in order to salvage some transfer out of 2.4MB, the hypothetical > Al protocol would first have the upload-pack give 20*1396 = 28kB It could be 10*1396 or less. If the server calculates the shortest unambiguous SHA-1 length (quite cheap on fully packed repo) and sends it to the client, the client can just sends short SHA-1 instead. It's racy though because objects are being added to the server and abbrev length may go up. But we can check ambiguity for all SHA-1 sent by client and ask for resend for ambiguous ones. On my linux-2.6.git, 10 letters (so 5 bytes) are needed for unambiguous short SHA-1. But we can even go optimistic and ask the client for shorter SHA-1 with hope that resend won't be many. > object names to fetch-pack; no matter how fetch-pack encodes its > preference, its answer would be less than 28kB. We would likely to > design this part of the new protocol in line with the existing part > and use textual object names, so let's round them up to 100kB. -- 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
CISCO AND AVAYA IP Phones
Hi, Clean tested working pulls CPUs and QTYs in stock. 115 X X5650 65 X E5410 75 X X5660 145 X E5530 100 X E5645 40 X X5680 75 X X5690 Brand new sealed IP phones and QTYs in stock. 55 x CP-7937G 77 x CP-7942G 54 x CP-7945G 75 x CP-7962G .. 45 x Avaya 9630 65 x Avaya 9641 55 x Avaya 9640 USED IT HARDWARE FROM: FUJITSU IBM SUNHP QUANTUM DELL HDSSTK NETAPP SGI Oracle EMC² 3Com, ADVA, Alcatel, Brocade, Cisco, Cabletron, Enterasys, Extreme Networks, Huawei, Marconi, Nortel, Qlogic, Avaya Let me know if you're interested. We are very open to offers and willing to work with you to make sure that we have a deal. Sincerely Barbara Johnson Laison Computech Inc Tel: +1-657-205-7860 Fax: +1-347-214-0478 Email: sa...@laiisoncomputech.com Web: www.laisoncomputech.com -- 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: Resumable git clone?
Al Viro writes: > FWIW, I wasn't proposing to recreate the remaining bits of that _pack_; > just do the normal pull with one addition: start with sending the list > of sha1 of objects you are about to send and let the recepient reply > with "I already have , don't bother with those". And exclude > those from the transfer. I did a quick-and-dirty unscientific experiment. I had a clone of Linus's repository that was about a week old, whose tip was at 4de8ebef (Merge tag 'trace-fixes-v4.5-rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace, 2016-02-22). To bring it up to date (i.e. a pull about a week's worth of progress) to f691b77b (Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs, 2016-03-01): $ git rev-list --objects 4de8ebef..f691b77b1fc | wc -l 1396 $ git rev-parse 4de8ebef..f691b77b1fc | git pack-objects --revs --delta-base-offset --stdout | wc -c 2444127 So in order to salvage some transfer out of 2.4MB, the hypothetical Al protocol would first have the upload-pack give 20*1396 = 28kB object names to fetch-pack; no matter how fetch-pack encodes its preference, its answer would be less than 28kB. We would likely to design this part of the new protocol in line with the existing part and use textual object names, so let's round them up to 100kB. That is quite small, even if you are on a crappy connection that you need to retry 5 times, the additional overhead to negotiate the list of objects alone would be 0.5MB (or less than 20% of the real transfer). That is quite interesting [*1*]. For the approach to be practical, you would have to write a program that reads from a truncated packfile and writes a new packfile, excising deltas that lack their bases, to salvage objects from a half-transferred packfile; it is however unclear how involved the code would get. It is probably OK for a tiny pack that has only 1400 objects--we could just pass the early part through unpack-objects and let it die when it hits EOF, but for a "resumable clone", I do not think you can afford to unpack 4.6M objects in the kernel repository into loose objects. The approach of course requires the server end to spend 5 times as many cycles as usual in order to help a client that retries 5 times. On the other hand, the resumable "clone" we were discussing by allowing the server to respond with a slightly older bundle or a pack and then asking the client to fill the latest bits by a follow-up fetch targets to reduce the load of the server side (the "slightly older" part can be offloaded to CDN). It is a happy side effect that material offloaded to CDN can more easily obtained via HTTPS that is trivially resumable ;-) I think your "I've got these already" extention may be worth trying, and it is definitely better than the "let's make sure the server end creates byte-for-byte identical pack stream, and discard the early part without sending it to the network", and it may help resuming a small incremental fetch, but I do not think it is advisable to use it for a full clone, given that it is very likely that we would be adding the "offload 'clone' to CDN" kind. Even though I can foresee both kinds to co-exist, I do not think it is practical to offer it for resuming multi-hour cloning of the kernel repository (or worse, Android repositories) over a trans-Pacific link, for example. [Footnote] *1* To update v4.5-rc1 to today's HEAD involves 10809 objects, and the pack data takes 14955728 bytes. That translates to ~440kB needed to advertise a list of textual object names to salvage object transfer of 15MB. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] builtin/receive-pack.c: use parse_options API
> Matthieu Moy writes: > >> "Sidhant Sharma [:tk]" writes: >> >>> Make receive-pack use the parse_options API, >>> bringing it more in line with send-pack and push. >> Thanks. This version looks good to me. > I'll queue this with your "Reviewed-by:" to 'pu', just as a > Microproject reward ;-). Given that the program will never see an > interactive use from a command line, however, I am not sure if it is > worth actually merging it down thru 'next' to 'master'. > > Thanks. Thanks for accepting my patch :) Now that this one is complete, I was wondering what should I do next. Is there a list of more such microproject-like projects? I'd be very excited to contribute more patches. Thanks and regards, Sidhant Sharma [:tk] -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-p4: map a P4 user to Git author name and email address
On 1 March 2016 at 19:15, Eric Sunshine wrote: > On Tue, Mar 1, 2016 at 5:49 AM, wrote: >> Map a P4 user to a specific name and email address in Git with the >> "git-p4.mapUser" config. The config value must be a string adhering >> to the format "p4user = First Lastname ". >> >> Signed-off-by: Lars Schneider >> --- >> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt >> +git-p4.mapUser:: >> + Map a P4 user to a name and email address in Git. Use a string >> + with the following format to create a mapping: >> ++ >> +- >> +git config --add git-p4.mapUser "p4user = First Last " >> +- >> ++ >> +A mapping will override any user information from P4. Mappings for >> +multiple P4 user can be defined. > > Sorry for not paying closer attention the first time, but this needs > to be repeated for each P4 user you want to map, right? One can > imagine this quickly becoming painful if you have a lot of users to > map. Have you considered modeling this after git-svn where you can set > an "authors" file (and name the corresponding option --authors-file)? For most authors it should just use the existing Perforce user information. This is (I assume) just for the occasional exception where Perforce has the wrong email address. Luke -- 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 06/10] setup: refactor repo format reading and verification
On Tue, Mar 01, 2016 at 04:20:06PM -0500, David Turner wrote:
> On Tue, 2016-03-01 at 09:42 -0500, Jeff King wrote:
> > +/*
> > + * Read the repository format characteristics from the config file
> > "path". If
> > + * the version cannot be extracted from the file for any reason, the
> > version
> > + * field will be set to -1, and all other fields are undefined.
> > + */
> > +void read_repository_format(struct repository_format *format, const
> > char *path);
>
> Generally speaking, I don't care for this interface; I would prefer to
> return -1 and not change the struct. But I see why it's maybe simpler
> in this one case.
Yeah, I waffled on it. The main reason was simply that the existing code
it's replacing generally wanted to have a stateful return, do some
cleanup, and then check "did we get a version?".
It would be easy to provide both (which matches the redundancy of
nongit_ok).
> > + * Internally, we need to swap out "fn" here, but we don't want to
> > expose
> > + * that to the world. Hence a wrapper around this internal function.
> > + */
>
> I don't understand this comment. fn is not being swapped out -- it's
> being passed on directly.
I meant that internally to setup.c, we need to call with different
variants of "fn", but that is an ugly interface we don't need to expose
outside the file. And further on in the series, we clean that up, and
can drop this internal interface. Maybe it's not even worth commenting
on (or commenting on in the commit message only).
> > +static void read_repository_format_1(struct repository_format
> > *format,
> > +config_fn_t fn, const char
> > *path)
>
> The argument order here is different from git_config_from_file -- is
> that for a reason?
Only that I consider this to be a more object-oriented interface (akin
to strbuf, argv_array, etc), with "struct repository_format" as the
"this" object. We usually put that argument first.
> > + if (format->version >= 1 && format->unknown_extensions.nr) {
> > + int i;
> > +
> > + for (i = 0; i < format->unknown_extensions.nr; i++)
> > + strbuf_addf(err, "unknown repository
> > extension: %s",
> > + format
> > ->unknown_extensions.items[i].string);
>
> newline here or something? Otherwise multiple unknown extensions will
> get jammed together.
Thanks, I blindly replaced warning(), which handles the newline. I'll
double-check the other conversions to sure we handle newlines there,
too.
-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 10/10] setup: drop GIT_REPO_VERSION constants
On Tue, Mar 01, 2016 at 07:13:02PM -0500, David Turner wrote:
> On Tue, 2016-03-01 at 09:45 -0500, Jeff King wrote:
> > - char repo_version_string[10];
> >
> > /* This forces creation of new config file */
> > - xsnprintf(repo_version_string, sizeof(repo_version_string),
> > - "%d", GIT_REPO_VERSION);
> > - git_config_set("core.repositoryformatversion",
> > repo_version_string);
> > + git_config_set("core.repositoryformatversion", "0");
>
> I'm about to use that in my series, so let's not get rid of it here.
Thanks. I floated this one up to the end exactly to make it easy to drop
in case there were objections. :)
-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 v7 01/33] setup: call setup_git_directory_gently before accessing refs
On Tue, Mar 01, 2016 at 06:47:52PM -0500, David Turner wrote:
> > My fix for this was to teach read_mailmap to avoid looking for
> > HEAD:.mailmap if we are not in a repository, but to continue with the
> > others (.mailmap in the cwd, and the mailmap.file config variable).
> > ...
> > But I do think your patch is a potential regression there, if anybody
> > does do that.
>
> Your version sounds better. But I don't see it in the patch set you
> sent earlier?
It's not. Sorry to be unclear. There were _two_ cleanups I was talking
about (cases where we don't check whether we're in a repo, and fact that
the repo startup code is unreliable), and I got sucked into the second
one. I'll try to work up and share my startup_info one today.
> When writing my patch, I had assumed that the issue was the resolve_ref
> on the HEAD that's an argument -- but it's not. The actual traceback
> is:
> [...]
> #2 resolve_ref_unsafe (refname=refname@entry=0x550b3b "HEAD",
> resolve_flags=resolve_flags@entry=0,
> sha1=sha1@entry=0x7fffd100 "\b\326\377\377\377\177",
> flags=flags@entry=0x7fffd0fc) at refs/files-backend.c:1600
> #3 0x004ffe69 in read_config () at remote.c:471
Oh, right. I did see problems here but missed them when comparing my
patch to yours. I ended up in remote.c:read_config, having it check
whether startup_info->have_repository is set; if it isn't, there is no
point in looking at HEAD.
That covers this case, and several others I happened across. Thanks for
clarifying.
> I'm not sure what the right way to fix this is -- in read_config, we're
> about to access some stuff in a repo (config, HEAD). It's OK to skip
> that stuff if we're not in a repo, but we don't want to run
> setup_git_directory twice (that breaks some stuff), and some of the
> other callers have already called it. On top of your earlier
> repo_initialized patch, we could add the following to read_config:
>
> + if (!repo_initialized) {
> + int nongit = 0;
> + setup_git_directory_gently(&nongit);
> + if (nongit)
> + return;
> + }
>
> But that patch I think was not intended to be permanent. Still, it
> does seem odd that there's no straightforward way to know if the repo
> is initialized. Am I missing something?
No, there isn't a straightforward way; I think we'll have to add one.
I'll polish up my series which does this.
-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: Resumable git clone?
On Tue, Mar 01, 2016 at 05:40:28PM -0800, Stefan Beller wrote:
> So throwing away half finished stuff while keeping the front load?
Throw away the object that got truncated and ones for which delta chain
doesn't resolve entirely in the transferred part.
> > indexing the objects it
> > contains, and then re-running clone and not having to fetch those
> > objects.
>
> The pack is not deterministic for a given repository. When creating
> the pack, you may encounter races between threads, such that the order
> in a pack differs.
FWIW, I wasn't proposing to recreate the remaining bits of that _pack_;
just do the normal pull with one addition: start with sending the list
of sha1 of objects you are about to send and let the recepient reply
with "I already have , don't bother with those". And exclude
those from the transfer. Encoding for the set being available is an
interesting variable here - might be plain list of sha1, might be its
complement ("I want the following subset"), might be "145th to 1029th,
1517th and 1890th to 1920th of the list you've sent"; which form ends
up more efficient needs to be found experimentally...
IIRC, the objection had been that the organisation of the pack will lead
to many cases when deltas are transferred *first*, with base object not
getting there prior to disconnect. I suspect that fraction of the objects
getting through would still be worth it, but I hadn't experimented enough
to be able to tell...
I was more interested in resumable _pull_, with restarted clone treated as
special case of that.
--
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
CISCO AND AVAYA IP Phones
Hi, Clean tested working pulls CPUs and QTYs in stock. 115 X X5650 65 X E5410 75 X X5660 145 X E5530 100 X E5645 40 X X5680 75 X X5690 Brand new sealed IP phones and QTYs in stock. 55 x CP-7937G 77 x CP-7942G 54 x CP-7945G 75 x CP-7962G .. 45 x Avaya 9630 65 x Avaya 9641 55 x Avaya 9640 USED IT HARDWARE FROM: FUJITSU IBM SUNHP QUANTUM DELL HDSSTK NETAPP SGI Oracle EMC² 3Com, ADVA, Alcatel, Brocade, Cisco, Cabletron, Enterasys, Extreme Networks, Huawei, Marconi, Nortel, Qlogic, Avaya Let me know if you're interested. We are very open to offers and willing to work with you to make sure that we have a deal. Sincerely Barbara Johnson Laison Computech Inc Tel: +1-657-205-7860 Fax: +1-347-214-0478 Email: sa...@laiisoncomputech.com Web: www.laisoncomputech.com -- 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: Resumable git clone?
On Wed, Mar 2, 2016 at 8:30 AM, Josh Triplett wrote: > If you clone a repository, and the connection drops, the next attempt > will have to start from scratch. This can add significant time and > expense if you're on a low-bandwidth or metered connection trying to > clone something like Linux. > > Would it be possible to make git clone resumable after a partial clone? > (And, ideally, to make that the default?) > > In a discussion elsewhere, Al Viro suggested taking the partial pack > received so far, repairing any truncation, indexing the objects it > contains, and then re-running clone and not having to fetch those > objects. This may also require extending receive-pack's protocol for > determining objects the recipient already has, as the partial pack may > not have a consistent set of reachable objects. > > Before starting down the path of developing patches for this, does the > approach seem potentially reasonable? This topic came up recently (thanks Sarah!) and Shawn proposed a different approach that (I think) is simpler and more effective for resume _clone_ case. I'm not sure if anybody is implementing it though. [1] http://thread.gmane.org/gmane.comp.version-control.git/285921 -- 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: Resumable git clone?
+ Duy, who tried resumable clone a few days/weeks ago On Tue, Mar 1, 2016 at 5:30 PM, Josh Triplett wrote: > If you clone a repository, and the connection drops, the next attempt > will have to start from scratch. This can add significant time and > expense if you're on a low-bandwidth or metered connection trying to > clone something like Linux. > > Would it be possible to make git clone resumable after a partial clone? > (And, ideally, to make that the default?) > > In a discussion elsewhere, Al Viro suggested taking the partial pack > received so far, ok, > repairing any truncation, So throwing away half finished stuff while keeping the front load? > indexing the objects it > contains, and then re-running clone and not having to fetch those > objects. The pack is not deterministic for a given repository. When creating the pack, you may encounter races between threads, such that the order in a pack differs. > This may also require extending receive-pack's protocol for > determining objects the recipient already has, as the partial pack may > not have a consistent set of reachable objects. > > Before starting down the path of developing patches for this, does the > approach seem potentially reasonable? I think that sounds reasonable on a high level, but I'd expect it blows up in complexity as in the receive-pack's protocol or in the code for having to handle partial stuff. Thanks, Stefan > > - Josh Triplett > -- > 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
Resumable git clone?
If you clone a repository, and the connection drops, the next attempt will have to start from scratch. This can add significant time and expense if you're on a low-bandwidth or metered connection trying to clone something like Linux. Would it be possible to make git clone resumable after a partial clone? (And, ideally, to make that the default?) In a discussion elsewhere, Al Viro suggested taking the partial pack received so far, repairing any truncation, indexing the objects it contains, and then re-running clone and not having to fetch those objects. This may also require extending receive-pack's protocol for determining objects the recipient already has, as the partial pack may not have a consistent set of reachable objects. Before starting down the path of developing patches for this, does the approach seem potentially reasonable? - Josh Triplett -- 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 0/10] cleaning up check_repository_format_gently
On Tue, 2016-03-01 at 09:35 -0500, Jeff King wrote: > After the discussion in: > > http://thread.gmane.org/gmane.comp.version-control.git/287949/focus > =288002 > > I came up with this series to try to address some of the warts in > check_repository_format_gently(). > > I had hoped to come up with something very small that could go at the > front of the refs-backend series, but as with any time I look at the > setup code, it managed to snowball into a potpourri of hidden > assumptions. > > So I hope David isn't _too_ irritated to see this in his inbox. I commented on two of the patches; the rest look good to me. I rebased on these (minus the repo_version_string change from 10/10). Wasn't too bad. And it did clean up the submodule check. I will wait to resend my series until I hear back about your shortlog/mailmap fix and your suggestion on a better archive --remote fix. -- 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 v7 01/33] setup: call setup_git_directory_gently before accessing refs
On Tue, 2016-03-01 at 18:47 -0500, David Turner wrote:
> On Tue, 2016-03-01 at 03:35 -0500, Jeff King wrote:
> > On Mon, Feb 29, 2016 at 07:52:34PM -0500, David Turner wrote:
> >
> > > Usually, git calls some form of setup_git_directory at startup.
> > > But
> > > sometimes, it doesn't. Usually, that's OK because it's not
> > > really
> > > using the repository. But in some cases, it is using the repo.
> > > In
> > > those cases, either setup_git_directory_gently must be called, or
> > > the
> > > repository (e.g. the refs) must not be accessed.
> >
> > It's actually not just setup_git_directory(). We can also use
> > check_repository_format(), which is used by enter_repo() (and hence
> > by
> > things like upload-pack). I think the rule really ought to be: if
> > we
> > didn't have check_repository_format_gently() tell us we have a
> > valid
> > repo, we should not access any repo elements (refs, objects, etc).
>
> I'll change that commit message to say
> "check_repository_format_gently".
>
> > > diff --git a/builtin/grep.c b/builtin/grep.
> > [snip: this is a probably-good behavior change]
>
> Agreed.
>
> > My fix for this was to teach read_mailmap to avoid looking for
> > HEAD:.mailmap if we are not in a repository, but to continue with
> > the
> > others (.mailmap in the cwd, and the mailmap.file config variable).
> > ...
> > But I do think your patch is a potential regression there, if
> > anybody
> > does do that.
>
> Your version sounds better. But I don't see it in the patch set you
> sent earlier?
>
> > > diff --git a/git.c b/git.c
> > > index 6cc0c07..51e0508 100644
> > > --- a/git.c
> > > +++ b/git.c
> > > @@ -376,7 +376,7 @@ static struct cmd_struct commands[] = {
> > > { "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
> > > { "annotate", cmd_annotate, RUN_SETUP },
> > > { "apply", cmd_apply, RUN_SETUP_GENTLY },
> > > - { "archive", cmd_archive },
> > > + { "archive", cmd_archive, RUN_SETUP_GENTLY },
> > > { "bisect--helper", cmd_bisect__helper, RUN_SETUP },
> > > { "blame", cmd_blame, RUN_SETUP },
> > > { "branch", cmd_branch, RUN_SETUP },
> >
> > I didn't have to touch this case in my experimenting. I wonder if
> > it's
> > because I resolved the "grep" case a little differently.
> >
> > I taught get_ref_cache() to only assert() that we have a repository
> > when
> > we are looking at the main ref-cache, not a submodule. In theory,
> > we
> > can
> > look at a submodule from inside an outer non-repo (it's not really
> > a
> > submodule then, but just a plain git dir). I don't think there's
> > anything in git right now that says you can't do so, though I think
> > your
> > refs-backend work does introduce that restriction (because it
> > actually
> > requires the submodules to use the same backend).
> >
> > So with that requirement, I think we do need to require a repo even
> > to
> > access submodule refs. Is that what triggered this change?
>
> No. What triggered this change was a test failure with your earlier
> patch on master -- none of my stuff at all. The failing command was:
>
> git archive --remote=. HEAD
>
> When writing my patch, I had assumed that the issue was the
> resolve_ref
> on the HEAD that's an argument -- but it's not. The actual traceback
> is:
>
> #0 die (
> err=err@entry=0x57ddb0 "BUG: resolve_ref called without
> initializing repo") at usage.c:99
> #1 0x004f7ed9 in resolve_ref_1 (sb_refname=0x7c4a50
> ,
> sb_contents=0x7fffcfc0, sb_path=0x7fffcfe0,
> flags=0x7fffdaaa,
> sha1=0x7fffd100 "\b\326\377\377\377\177",
> resolve_flags=5572384,
> refname=0x2 )
> at refs/files-backend.c:1429
> #2 resolve_ref_unsafe (refname=refname@entry=0x550b3b "HEAD",
> resolve_flags=resolve_flags@entry=0,
> sha1=sha1@entry=0x7fffd100 "\b\326\377\377\377\177",
> flags=flags@entry=0x7fffd0fc) at refs/files-backend.c:1600
> #3 0x004ffe69 in read_config () at remote.c:471
> #4 0x00500235 in read_config () at remote.c:705
> #5 remote_get_1 (name=0x7fffdaaa ".",
> get_default=get_default@entry=0x4fe230 )
> at remote.c:688
> #6 0x005004ca in remote_get (name=) at
> remote.c:713
> #7 0x004159d8 in run_remote_archiver (name_hint=0x0,
> exec=0x550720 "git-upload-archive", remote=,
> argv=0x7fffd608, argc=2) at builtin/archive.c:35
> #8 cmd_archive (argc=2, argv=0x7fffd608, prefix=0x0)
> at builtin/archive.c:104
> #9 0x00406051 in run_builtin (argv=0x7fffd608, argc=3,
> p=0x7bd7a0 ) at git.c:357
> #10 handle_builtin (argc=3, argv=0x7fffd608) at git.c:540
> #11 0x0040519a in main (argc=3, av=) at
> git.c:671
>
> > I'd think you would need a matching line inside cmd_archive, too.
> > It
> > should allow "--remote" without a repo, but generating a local
> > archive
> > does need one. And indeed, I see in write_archive() that we run
> > setup_git_repository ourselves, and die if we're not in a git
Re: [PATCH] cherry-pick: add --no-verify option
On Tue, Mar 1, 2016 at 3:40 PM, Kevin Daudt wrote: > git commit has a --no-verify option to prevent the pre-commit hook from > running. When continuing a conflicted cherry-pick, git commit gets > executed which also causes the pre-commit hook to be run. > > Add --no-verify and pass that through to the git commit command so that > the can prevent that from happening > > Signed-off-by: Kevin Daudt > --- > diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh > @@ -340,6 +340,18 @@ test_expect_success '--continue after resolving > conflicts and committing' ' > +test_expect_success '--continue --no-verify does not run pre-commit hook ' ' > + pristine_detach initial && > + mkdir -p .git/hooks && > + echo -e "#!/bin/sh\nexit 1" >.git/hooks/pre-commit && Non-portable 'echo'. You could use printf instead, however, even better would be to use write_script() along with a here-doc (then you could drop the 'chmod' also). > + chmod u+x .git/hooks/pre-commit && > + test_when_finished "rm -r .git/hooks/" && > + > + test_must_fail git cherry-pick picked && > + git add foo && > + git cherry-pick --continue --no-verify > +' -- 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 10/10] setup: drop GIT_REPO_VERSION constants
On Tue, 2016-03-01 at 09:45 -0500, Jeff King wrote:
> - char repo_version_string[10];
>
> /* This forces creation of new config file */
> - xsnprintf(repo_version_string, sizeof(repo_version_string),
> - "%d", GIT_REPO_VERSION);
> - git_config_set("core.repositoryformatversion",
> repo_version_string);
> + git_config_set("core.repositoryformatversion", "0");
I'm about to use that in my series, so let's not get rid of it 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 v7 01/33] setup: call setup_git_directory_gently before accessing refs
On Tue, 2016-03-01 at 03:35 -0500, Jeff King wrote:
> On Mon, Feb 29, 2016 at 07:52:34PM -0500, David Turner wrote:
>
> > Usually, git calls some form of setup_git_directory at startup.
> > But
> > sometimes, it doesn't. Usually, that's OK because it's not really
> > using the repository. But in some cases, it is using the repo. In
> > those cases, either setup_git_directory_gently must be called, or
> > the
> > repository (e.g. the refs) must not be accessed.
>
> It's actually not just setup_git_directory(). We can also use
> check_repository_format(), which is used by enter_repo() (and hence
> by
> things like upload-pack). I think the rule really ought to be: if we
> didn't have check_repository_format_gently() tell us we have a valid
> repo, we should not access any repo elements (refs, objects, etc).
I'll change that commit message to say
"check_repository_format_gently".
> > diff --git a/builtin/grep.c b/builtin/grep.
> [snip: this is a probably-good behavior change]
Agreed.
> My fix for this was to teach read_mailmap to avoid looking for
> HEAD:.mailmap if we are not in a repository, but to continue with the
> others (.mailmap in the cwd, and the mailmap.file config variable).
> ...
> But I do think your patch is a potential regression there, if anybody
> does do that.
Your version sounds better. But I don't see it in the patch set you
sent earlier?
> > diff --git a/git.c b/git.c
> > index 6cc0c07..51e0508 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -376,7 +376,7 @@ static struct cmd_struct commands[] = {
> > { "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
> > { "annotate", cmd_annotate, RUN_SETUP },
> > { "apply", cmd_apply, RUN_SETUP_GENTLY },
> > - { "archive", cmd_archive },
> > + { "archive", cmd_archive, RUN_SETUP_GENTLY },
> > { "bisect--helper", cmd_bisect__helper, RUN_SETUP },
> > { "blame", cmd_blame, RUN_SETUP },
> > { "branch", cmd_branch, RUN_SETUP },
>
> I didn't have to touch this case in my experimenting. I wonder if
> it's
> because I resolved the "grep" case a little differently.
>
> I taught get_ref_cache() to only assert() that we have a repository
> when
> we are looking at the main ref-cache, not a submodule. In theory, we
> can
> look at a submodule from inside an outer non-repo (it's not really a
> submodule then, but just a plain git dir). I don't think there's
> anything in git right now that says you can't do so, though I think
> your
> refs-backend work does introduce that restriction (because it
> actually
> requires the submodules to use the same backend).
>
> So with that requirement, I think we do need to require a repo even
> to
> access submodule refs. Is that what triggered this change?
No. What triggered this change was a test failure with your earlier
patch on master -- none of my stuff at all. The failing command was:
git archive --remote=. HEAD
When writing my patch, I had assumed that the issue was the resolve_ref
on the HEAD that's an argument -- but it's not. The actual traceback
is:
#0 die (
err=err@entry=0x57ddb0 "BUG: resolve_ref called without
initializing repo") at usage.c:99
#1 0x004f7ed9 in resolve_ref_1 (sb_refname=0x7c4a50
,
sb_contents=0x7fffcfc0, sb_path=0x7fffcfe0,
flags=0x7fffdaaa,
sha1=0x7fffd100 "\b\326\377\377\377\177",
resolve_flags=5572384,
refname=0x2 )
at refs/files-backend.c:1429
#2 resolve_ref_unsafe (refname=refname@entry=0x550b3b "HEAD",
resolve_flags=resolve_flags@entry=0,
sha1=sha1@entry=0x7fffd100 "\b\326\377\377\377\177",
flags=flags@entry=0x7fffd0fc) at refs/files-backend.c:1600
#3 0x004ffe69 in read_config () at remote.c:471
#4 0x00500235 in read_config () at remote.c:705
#5 remote_get_1 (name=0x7fffdaaa ".",
get_default=get_default@entry=0x4fe230 )
at remote.c:688
#6 0x005004ca in remote_get (name=) at
remote.c:713
#7 0x004159d8 in run_remote_archiver (name_hint=0x0,
exec=0x550720 "git-upload-archive", remote=,
argv=0x7fffd608, argc=2) at builtin/archive.c:35
#8 cmd_archive (argc=2, argv=0x7fffd608, prefix=0x0)
at builtin/archive.c:104
#9 0x00406051 in run_builtin (argv=0x7fffd608, argc=3,
p=0x7bd7a0 ) at git.c:357
#10 handle_builtin (argc=3, argv=0x7fffd608) at git.c:540
#11 0x0040519a in main (argc=3, av=) at
git.c:671
> I'd think you would need a matching line inside cmd_archive, too. It
> should allow "--remote" without a repo, but generating a local
> archive
> does need one. And indeed, I see in write_archive() that we run
> setup_git_repository ourselves, and die if we're not in a git repo.
> So
> I'm puzzled about which code path accesses the refs.
I agree that --remote should work without a repo, It seems that we do
n't test this and we probably should.
I'm not sure what the right way to fix this is -- in read_config, we're
about to access some stuff in a repo (config, HEAD). It's OK to
[PATCH 2/2] bundle: keep a copy of bundle file name in the in-core bundle header
This will be necessary when we start reading from a split bundle
where the header and the thin-pack data live in different files.
The in-core bundle header will read from a file that has the header,
and will record the path to that file. We would find the name of
the file that hosts the thin-pack data from the header, and we would
take that name as relative to the file we read the header from.
Signed-off-by: Junio C Hamano
---
builtin/bundle.c | 5 +++--
bundle.c | 21 +++--
bundle.h | 3 ++-
transport.c | 3 ++-
4 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/builtin/bundle.c b/builtin/bundle.c
index 4883a43..e63388d 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -36,8 +36,9 @@ int cmd_bundle(int argc, const char **argv, const char
*prefix)
}
memset(&header, 0, sizeof(header));
- if (strcmp(cmd, "create") && (bundle_fd =
- read_bundle_header(bundle_file, &header)) < 0)
+ header.bundle_file = bundle_file;
+ if (strcmp(cmd, "create") &&
+ (bundle_fd = read_bundle_header(&header)) < 0)
return 1;
if (!strcmp(cmd, "verify")) {
diff --git a/bundle.c b/bundle.c
index 9c5a6f0..efe19e0 100644
--- a/bundle.c
+++ b/bundle.c
@@ -21,8 +21,7 @@ static void add_to_ref_list(const unsigned char *sha1, const
char *name,
list->nr++;
}
-static int parse_bundle_header(int fd, struct bundle_header *header,
- const char *report_path)
+static int parse_bundle_header(int fd, struct bundle_header *header, int quiet)
{
struct strbuf buf = STRBUF_INIT;
int status = 0;
@@ -30,9 +29,9 @@ static int parse_bundle_header(int fd, struct bundle_header
*header,
/* The bundle header begins with the signature */
if (strbuf_getwholeline_fd(&buf, fd, '\n') ||
strcmp(buf.buf, bundle_signature)) {
- if (report_path)
+ if (!quiet)
error(_("'%s' does not look like a v2 bundle file"),
- report_path);
+ header->bundle_file);
status = -1;
goto abort;
}
@@ -57,7 +56,7 @@ static int parse_bundle_header(int fd, struct bundle_header
*header,
if (get_sha1_hex(buf.buf, sha1) ||
(buf.len > 40 && !isspace(buf.buf[40])) ||
(!is_prereq && buf.len <= 40)) {
- if (report_path)
+ if (!quiet)
error(_("unrecognized header: %s%s (%d)"),
(is_prereq ? "-" : ""), buf.buf,
(int)buf.len);
status = -1;
@@ -79,13 +78,13 @@ static int parse_bundle_header(int fd, struct bundle_header
*header,
return fd;
}
-int read_bundle_header(const char *path, struct bundle_header *header)
+int read_bundle_header(struct bundle_header *header)
{
- int fd = open(path, O_RDONLY);
+ int fd = open(header->bundle_file, O_RDONLY);
if (fd < 0)
- return error(_("could not open '%s'"), path);
- return parse_bundle_header(fd, header, path);
+ return error(_("could not open '%s'"), header->bundle_file);
+ return parse_bundle_header(fd, header, 0);
}
int is_bundle(const char *path, int quiet)
@@ -96,7 +95,7 @@ int is_bundle(const char *path, int quiet)
if (fd < 0)
return 0;
memset(&header, 0, sizeof(header));
- fd = parse_bundle_header(fd, &header, quiet ? NULL : path);
+ fd = parse_bundle_header(fd, &header, quiet);
if (fd >= 0)
close(fd);
return (fd >= 0);
@@ -112,6 +111,8 @@ void release_bundle_header(struct bundle_header *header)
for (i = 0; i < header->references.nr; i++)
free(header->references.list[i].name);
free(header->references.list);
+
+ free((void *)header->bundle_file);
}
static int list_refs(struct ref_list *r, int argc, const char **argv)
diff --git a/bundle.h b/bundle.h
index f7ce23b..cf771df 100644
--- a/bundle.h
+++ b/bundle.h
@@ -10,12 +10,13 @@ struct ref_list {
};
struct bundle_header {
+ const char *bundle_file;
struct ref_list prerequisites;
struct ref_list references;
};
int is_bundle(const char *path, int quiet);
-int read_bundle_header(const char *path, struct bundle_header *header);
+int read_bundle_header(struct bundle_header *header);
int create_bundle(struct bundle_header *header, const char *path,
int argc, const char **argv);
int verify_bundle(struct bundle_header *header, int verbose);
diff --git a/transport.c b/transport.c
index 08e15c5..03ce149 100644
--- a/transport.c
+++ b/transport.c
@@ -81,7 +81,8 @@ static struct ref *get_refs_from_bundle(struct transport
*transport, int for_pus
if (data->fd > 0)
close
[PATCH 1/2] bundle: plug resource leak
The bundle header structure holds two lists of refs and object
names, which should be released when the user is done with it.
Signed-off-by: Junio C Hamano
---
bundle.c| 12
bundle.h| 1 +
transport.c | 1 +
3 files changed, 14 insertions(+)
diff --git a/bundle.c b/bundle.c
index 506ac49..9c5a6f0 100644
--- a/bundle.c
+++ b/bundle.c
@@ -102,6 +102,18 @@ int is_bundle(const char *path, int quiet)
return (fd >= 0);
}
+void release_bundle_header(struct bundle_header *header)
+{
+ int i;
+
+ for (i = 0; i < header->prerequisites.nr; i++)
+ free(header->prerequisites.list[i].name);
+ free(header->prerequisites.list);
+ for (i = 0; i < header->references.nr; i++)
+ free(header->references.list[i].name);
+ free(header->references.list);
+}
+
static int list_refs(struct ref_list *r, int argc, const char **argv)
{
int i;
diff --git a/bundle.h b/bundle.h
index 1584e4d..f7ce23b 100644
--- a/bundle.h
+++ b/bundle.h
@@ -23,5 +23,6 @@ int verify_bundle(struct bundle_header *header, int verbose);
int unbundle(struct bundle_header *header, int bundle_fd, int flags);
int list_bundle_refs(struct bundle_header *header,
int argc, const char **argv);
+void release_bundle_header(struct bundle_header *);
#endif
diff --git a/transport.c b/transport.c
index ca3cfa4..08e15c5 100644
--- a/transport.c
+++ b/transport.c
@@ -107,6 +107,7 @@ static int close_bundle(struct transport *transport)
struct bundle_transport_data *data = transport->data;
if (data->fd > 0)
close(data->fd);
+ release_bundle_header(&data->header);
free(data);
return 0;
}
--
2.8.0-rc0-114-g0b3e5e5
--
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: git submodule add fails when .git is a symlink
Junio C Hamano wrote: > A more pertinent question may be which version of Git did the above > ever work, I guess. We fairly liberally chdir around and I do not > think we deliberately avoid assuming that "cd .git && cd .." might > not come back to the original directory, for example, so I wouldn't > be surprised if it never worked. IIRC git used symlinks for .git in submodules before version 1.7.8, so I guess that older versions supported that pretty well. This one case is the only time I've seen a symlink for .git present a problem so far. -- see shy jo signature.asc Description: PGP signature
Re: [PATCH] Documentation: reword rebase summary
On Tue, Mar 01, 2016 at 02:49:58PM -0800, Stefan Beller wrote: > The wording is introduced in c3f0baaca (Documentation: sync git.txt > command list and manual page title, 2007-01-18), but rebase has evolved > since then, capture the modern usage by being more generic about the > rebase command in the summary. > > Signed-off-by: Stefan Beller > --- > > Inspired by > > https://medium.freecodecamp.com/git-rebase-and-the-golden-rule-explained-70715eccc372 > (I tried to cc the author, but I am not sure if I got the right email > address) > > Thanks, > Stefan > > Documentation/git-rebase.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 6cca8bb..6ed610a 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -3,7 +3,7 @@ git-rebase(1) > > NAME > > -git-rebase - Forward-port local commits to the updated upstream head > +git-rebase - Reapply commits on top of another base tip > > SYNOPSIS > > -- > 2.8.0.rc0 > I was thinking about doing the same. Thanks for improving this. -- 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: git submodule add fails when .git is a symlink
Stefan Beller wrote: > To elaborate on that: Starting in 2.7 parts of the submodule stuff > has been rewritten in C, in 2.8 even more and there is more in flight for > > 2.8. > > However your bug is also to be found in 2.6, which doesn't contain any > recent rewrites, so it is a rather long standing bug, I would presume. Yes, I saw it with 2.7.0, but I think the user who reported it was on 2.6. > As a workaround for now: > > echo "gitdir: ../gitdir/.git" > .git Not an option in our particular situation, unfortunately. I wonder if the miscalculated ../../../somedir could cause git to access files outside the git repos? -- see shy jo signature.asc Description: PGP signature
Re: [PATCH] Documentation: reword rebase summary
On Tue, Mar 1, 2016 at 2:49 PM, Stefan Beller wrote: > The wording is introduced in c3f0baaca (Documentation: sync git.txt > command list and manual page title, 2007-01-18), but rebase has evolved > since then, capture the modern usage by being more generic about the > rebase command in the summary. > > Signed-off-by: Stefan Beller > --- > > Inspired by > > https://medium.freecodecamp.com/git-rebase-and-the-golden-rule-explained-70715eccc372 > (I tried to cc the author, but I am not sure if I got the right email > address) > > Thanks, > Stefan > > Documentation/git-rebase.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 6cca8bb..6ed610a 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -3,7 +3,7 @@ git-rebase(1) > > NAME > > -git-rebase - Forward-port local commits to the updated upstream head > +git-rebase - Reapply commits on top of another base tip > Seems like a reasonable summary to me, and definitely more fitting of what rebase does today. Thanks, Jake -- 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] bundle doc: 'verify' is not about verifying the bundle
From: "Junio C Hamano" Even though the command does read the bundle header and checks to see if it looks reasonable, the thin-pack data stream that follows the header in the bundle file is not checked. More importantly, because the thin-pack data does not have a trailing checksum like on-disk packfiles do, there isn't much "verification" the command can do without unpacking the objects from the stream even if it wanted to. The documentation gives an incorrect impression that the thin-pack data contained in the bundle is validated, but the command is to validate that the receiving repository is ready to accept the bundle, not to check the validity of a bundle file. Rephrase the paragraph to clarify this. This looks good to me. I was actually looking at this over the weekend with respect to a back- burner issue about indicating which ref is HEAD within the bundle. (thread variously $gmane/265001 and it's prior links, though now I'm minded to add \0HEAD after the correct ref; e.g. ' refs/heads/\0HEAD') Philip Signed-off-by: Junio C Hamano --- Documentation/git-bundle.txt | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt index 3a8120c..c0113a8 100644 --- a/Documentation/git-bundle.txt +++ b/Documentation/git-bundle.txt @@ -38,11 +38,10 @@ create :: 'git-rev-list-args' arguments to define the bundle contents. verify :: - Used to check that a bundle file is valid and will apply - cleanly to the current repository. This includes checks on the - bundle format itself as well as checking that the prerequisite - commits exist and are fully linked in the current repository. - 'git bundle' prints a list of missing commits, if any, and exits + Verifies that the given 'file' has a valid-looking bundle + header, and that your repository has all prerequisite + objects necessary to unpack the file as a bundle. The + command prints a list of missing commits, if any, and exits with a non-zero status. list-heads :: -- 2.8.0-rc0-114-g0b3e5e5 -- -- 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] Documentation: reword rebase summary
The wording is introduced in c3f0baaca (Documentation: sync git.txt command list and manual page title, 2007-01-18), but rebase has evolved since then, capture the modern usage by being more generic about the rebase command in the summary. Signed-off-by: Stefan Beller --- Inspired by https://medium.freecodecamp.com/git-rebase-and-the-golden-rule-explained-70715eccc372 (I tried to cc the author, but I am not sure if I got the right email address) Thanks, Stefan Documentation/git-rebase.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 6cca8bb..6ed610a 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -3,7 +3,7 @@ git-rebase(1) NAME -git-rebase - Forward-port local commits to the updated upstream head +git-rebase - Reapply commits on top of another base tip SYNOPSIS -- 2.8.0.rc0 -- 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-svn ignore-paths and huge files
Hi. Recently I sent Eric (who is CC-ed and these days sporadic available for deeper analyses) email titled as "git-svn cloning get stuck during processing huge file". It was about "git svn clone ..." against repository which had big third-party files (1,3 GB, 1,1 GB, 80 MB, 55 MB ), temporary stored in that repository by mistake (deleted in next revisions). So, because I don' t want them, naturally, neither in WC, nor in .git/object files as a part of history, I used "--ignore-paths" to avoid them. Until today I though that process git-svn hangs, because I did not see any progress indicator for a long time, more than couple of hours. (Thankful to "--ignore-paths", there were no temporary file(s) under ./git which rapidly increase on second level, but still there were files such as (MAC's git-svn): -rw--- 1 vladimirdosen staff 0 Feb 26 17:51 Git_git_blob_1101_0_gyOjAm -rw--- 1 vladimirdosen staff 0 Feb 26 17:51 Git_svn_delta_1101_0_kG6AtD -rw--- 1 vladimirdosen staff 0 Feb 26 17:51 Git_svn_hash_OkjgGH , as well as -rw-r--r-- 1 vladimirdosen staff 0 Feb 26 17:51 index.lock under /.git/svn/refs/remotes/git-svn. ... and I could note that process "get stuck" somewhere between Ra.gs_do_update's call for "$reporter->finish_report($pool);" and return from Fetcher.apply_textdelta. ) Fortunately, today, probably thankful to much better network condition, I succeed with "latency" 60-80 minutes, where handling of mentioned files has taken 98% of total cloning. So now, my question(s) could be rephrased: Is there any way that ignoring with "ignore-paths" can be complete ignoring of marked files, or why beside many "return undef if $self->is_path_ignored" in git-svn this cloning(fetching) takes significant time anyhow? It is not only about waiting, but I was misled that process is not possible at all if I want to migrate SVN with full history :) Thanks, Vladimir -- 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: [PATCHv21 00/10] Expose submodule parallelism to the user
Stefan Beller writes:
> * evicted patches:
> run-command: expose default_{start_failure, task_finished}
> run_processes_parallel: correctly terminate callbacks with an LF
> * rebased on top of
> origin/sb/no-child-process-access-in-run-parallel-callbacks
> * followed Jonathans advice on improving the situation for translators.
Queued on top of the fix to parallel-fetch topic, and the
two patches for submodule-init rebased on top of it.
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 v2] builtin/receive-pack.c: use parse_options API
Matthieu Moy writes: > "Sidhant Sharma [:tk]" writes: > >> Make receive-pack use the parse_options API, >> bringing it more in line with send-pack and push. > > Thanks. This version looks good to me. I'll queue this with your "Reviewed-by:" to 'pu', just as a Microproject reward ;-). Given that the program will never see an interactive use from a command line, however, I am not sure if it is worth actually merging it down thru 'next' to 'master'. 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 v6 7/7] git: submodule honor -c credential.* from command line
On Tue, Mar 1, 2016 at 11:07 AM, Junio C Hamano wrote: >> I could call expect_askpass here at each time but I don't think it >> would be meaningful after a test_must_fail. > > Even if you call expect_askpass to check, another set_askpass is > expected to start the next cycle anyway (unless we restructure the > helpers around clear_askpass_query I alluded to, which I am not > convinced is a good idea yet), so you'll still be asked "why another > set_askpass to set the same 'wrong pass@host'?". > > So, I dunno. Correct. I think it's not worth changing anything here now. The bigger solution to change to clear_askpass_query might be the right answer but it's a lot more involved. Thanks, Jake -- 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: git submodule add fails when .git is a symlink
Stefan Beller writes: > On Tue, Mar 1, 2016 at 12:42 PM, Joey Hess wrote: >> git init gitdir >> mkdir worktree >> cd worktree >> ln -s ../gitdir/.git .git >> git submodule add /any/git/repo sub >> >> fatal: Could not chdir to '../../../sub': No such file or directory >> >> Fairly sure this is a bug.. > > Which version(s) of Git do you use? A more pertinent question may be which version of Git did the above ever work, I guess. We fairly liberally chdir around and I do not think we deliberately avoid assuming that "cd .git && cd .." might not come back to the original directory, for example, so I wouldn't be surprised if it never worked. -- 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: git submodule add fails when .git is a symlink
On Tue, Mar 1, 2016 at 1:39 PM, Stefan Beller wrote: > On Tue, Mar 1, 2016 at 12:42 PM, Joey Hess wrote: >> git init gitdir >> mkdir worktree >> cd worktree >> ln -s ../gitdir/.git .git >> git submodule add /any/git/repo sub >> >> fatal: Could not chdir to '../../../sub': No such file or directory >> >> Fairly sure this is a bug.. > > Which version(s) of Git do you use? To elaborate on that: Starting in 2.7 parts of the submodule stuff has been rewritten in C, in 2.8 even more and there is more in flight for > 2.8. However your bug is also to be found in 2.6, which doesn't contain any recent rewrites, so it is a rather long standing bug, I would presume. As a workaround for now: echo "gitdir: ../gitdir/.git" > .git instead of the symbolic link in your example (works in 2.6 and also in 2.8.0-rc0) Thanks, Stefan -- 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: git submodule add fails when .git is a symlink
On Tue, Mar 1, 2016 at 12:42 PM, Joey Hess wrote: > git init gitdir > mkdir worktree > cd worktree > ln -s ../gitdir/.git .git > git submodule add /any/git/repo sub > > fatal: Could not chdir to '../../../sub': No such file or directory > > Fairly sure this is a bug.. Which version(s) of Git do you use? -- 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] bundle doc: 'verify' is not about verifying the bundle
Even though the command does read the bundle header and checks to see if it looks reasonable, the thin-pack data stream that follows the header in the bundle file is not checked. More importantly, because the thin-pack data does not have a trailing checksum like on-disk packfiles do, there isn't much "verification" the command can do without unpacking the objects from the stream even if it wanted to. The documentation gives an incorrect impression that the thin-pack data contained in the bundle is validated, but the command is to validate that the receiving repository is ready to accept the bundle, not to check the validity of a bundle file. Rephrase the paragraph to clarify this. Signed-off-by: Junio C Hamano --- Documentation/git-bundle.txt | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt index 3a8120c..c0113a8 100644 --- a/Documentation/git-bundle.txt +++ b/Documentation/git-bundle.txt @@ -38,11 +38,10 @@ create :: 'git-rev-list-args' arguments to define the bundle contents. verify :: - Used to check that a bundle file is valid and will apply - cleanly to the current repository. This includes checks on the - bundle format itself as well as checking that the prerequisite - commits exist and are fully linked in the current repository. - 'git bundle' prints a list of missing commits, if any, and exits + Verifies that the given 'file' has a valid-looking bundle + header, and that your repository has all prerequisite + objects necessary to unpack the file as a bundle. The + command prints a list of missing commits, if any, and exits with a non-zero status. list-heads :: -- 2.8.0-rc0-114-g0b3e5e5 -- 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 06/10] setup: refactor repo format reading and verification
On Tue, 2016-03-01 at 09:42 -0500, Jeff King wrote:
> +/*
> + * Read the repository format characteristics from the config file
> "path". If
> + * the version cannot be extracted from the file for any reason, the
> version
> + * field will be set to -1, and all other fields are undefined.
> + */
> +void read_repository_format(struct repository_format *format, const
> char *path);
Generally speaking, I don't care for this interface; I would prefer to
return -1 and not change the struct. But I see why it's maybe simpler
in this one case.
+/*
> + * Internally, we need to swap out "fn" here, but we don't want to
> expose
> + * that to the world. Hence a wrapper around this internal function.
> + */
I don't understand this comment. fn is not being swapped out -- it's
being passed on directly.
> +static void read_repository_format_1(struct repository_format
> *format,
> + config_fn_t fn, const char
> *path)
The argument order here is different from git_config_from_file -- is
that for a reason?
> + if (format->version >= 1 && format->unknown_extensions.nr) {
> + int i;
> +
> + for (i = 0; i < format->unknown_extensions.nr; i++)
> + strbuf_addf(err, "unknown repository
> extension: %s",
> + format
> ->unknown_extensions.items[i].string);
newline here or something? Otherwise multiple unknown extensions will
get jammed together.
--
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
bug: git submodule add fails when .git is a symlink
git init gitdir mkdir worktree cd worktree ln -s ../gitdir/.git .git git submodule add /any/git/repo sub fatal: Could not chdir to '../../../sub': No such file or directory Fairly sure this is a bug.. -- see shy jo signature.asc Description: PGP signature
[PATCH] cherry-pick: add --no-verify option
git commit has a --no-verify option to prevent the pre-commit hook from
running. When continuing a conflicted cherry-pick, git commit gets
executed which also causes the pre-commit hook to be run.
Add --no-verify and pass that through to the git commit command so that
the can prevent that from happening
Signed-off-by: Kevin Daudt
---
builtin/revert.c| 2 ++
sequencer.c | 21 -
sequencer.h | 1 +
t/t3510-cherry-pick-sequence.sh | 12
4 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 56a2c36..81d9c85 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -97,6 +97,7 @@ static void parse_args(int argc, const char **argv, struct
replay_opts *opts)
OPT_END(),
OPT_END(),
OPT_END(),
+ OPT_END(),
};
if (opts->action == REPLAY_PICK) {
@@ -106,6 +107,7 @@ static void parse_args(int argc, const char **argv, struct
replay_opts *opts)
OPT_BOOL(0, "allow-empty", &opts->allow_empty,
N_("preserve initially empty commits")),
OPT_BOOL(0, "allow-empty-message",
&opts->allow_empty_message, N_("allow commits with empty messages")),
OPT_BOOL(0, "keep-redundant-commits",
&opts->keep_redundant_commits, N_("keep redundant, empty commits")),
+ OPT_BOOL(0, "no-verify", &opts->no_verify, N_("don't
run pre-commit hook when continuing cherry-pick")),
OPT_END(),
};
if (parse_options_concat(options, ARRAY_SIZE(options),
cp_extra))
diff --git a/sequencer.c b/sequencer.c
index e66f2fe..657a381 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -978,14 +978,25 @@ static int pick_commits(struct commit_list *todo_list,
struct replay_opts *opts)
return 0;
}
-static int continue_single_pick(void)
+static int continue_single_pick(struct replay_opts *opts)
{
- const char *argv[] = { "commit", NULL };
+ struct argv_array array;
+ int rc;
+
+ argv_array_init(&array);
+ argv_array_push(&array, "commit");
if (!file_exists(git_path_cherry_pick_head()) &&
!file_exists(git_path_revert_head()))
return error(_("no cherry-pick or revert in progress"));
- return run_command_v_opt(argv, RUN_GIT_CMD);
+
+ if (opts->no_verify)
+ argv_array_push(&array, "--no-verify");
+
+ rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
+ argv_array_clear(&array);
+
+ return rc;
}
static int sequencer_continue(struct replay_opts *opts)
@@ -993,14 +1004,14 @@ static int sequencer_continue(struct replay_opts *opts)
struct commit_list *todo_list = NULL;
if (!file_exists(git_path_todo_file()))
- return continue_single_pick();
+ return continue_single_pick(opts);
read_populate_opts(&opts);
read_populate_todo(&todo_list, opts);
/* Verify that the conflict has been resolved */
if (file_exists(git_path_cherry_pick_head()) ||
file_exists(git_path_revert_head())) {
- int ret = continue_single_pick();
+ int ret = continue_single_pick(opts);
if (ret)
return ret;
}
diff --git a/sequencer.h b/sequencer.h
index 5ed5cb1..d868a50 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -34,6 +34,7 @@ struct replay_opts {
int allow_empty;
int allow_empty_message;
int keep_redundant_commits;
+ int no_verify;
int mainline;
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7b7a89d..29a06f8 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -340,6 +340,18 @@ test_expect_success '--continue after resolving conflicts
and committing' '
test_cmp expect actual
'
+test_expect_success '--continue --no-verify does not run pre-commit hook ' '
+ pristine_detach initial &&
+ mkdir -p .git/hooks &&
+ echo -e "#!/bin/sh\nexit 1" >.git/hooks/pre-commit &&
+ chmod u+x .git/hooks/pre-commit &&
+ test_when_finished "rm -r .git/hooks/" &&
+
+ test_must_fail git cherry-pick picked &&
+ git add foo &&
+ git cherry-pick --continue --no-verify
+'
+
test_expect_success '--continue asks for help after resolving patch to nil' '
pristine_detach conflicting &&
test_must_fail git cherry-pick initial..picked &&
--
2.7.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 v2] builtin/receive-pack.c: use parse_options API
"Sidhant Sharma [:tk]" writes: > Make receive-pack use the parse_options API, > bringing it more in line with send-pack and push. Thanks. This version looks good to me. -- 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: [PATCH v2] builtin/receive-pack.c: use parse_options API
> Starting from here, the patch is a bit painful to read because the diff > heuristics grouped hunks in a strange way. You may try "git format-patch > --patience" or --minimal or --histogram to see if it gives a better > result. The final commit would be the same, but it may make review > easier. I tried using the other algorithms, but results were same for all. Regards, Sidhant Sharma [:tk] -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] builtin/receive-pack.c: use parse_options API
Make receive-pack use the parse_options API,
bringing it more in line with send-pack and push.
Helped-by: Matthieu Moy
Signed-off-by: Sidhant Sharma [:tk]
---
Link to previous version: $gmane/288035
builtin/receive-pack.c | 53 +++---
1 file changed, 20 insertions(+), 33 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c8e32b2..220a899 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -21,7 +21,10 @@
#include "sigchain.h"
#include "fsck.h"
-static const char receive_pack_usage[] = "git receive-pack ";
+static const char * const receive_pack_usage[] = {
+ N_("git receive-pack "),
+ NULL
+};
enum deny_action {
DENY_UNCONFIGURED,
@@ -49,7 +52,7 @@ static int quiet;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
static int auto_gc = 1;
-static int fix_thin = 1;
+static int reject_thin;
static int stateless_rpc;
static const char *service_dir;
static const char *head_name;
@@ -1548,7 +1551,7 @@ static const char *unpack(int err_fd, struct shallow_info
*si)
if (fsck_objects)
argv_array_pushf(&child.args, "--strict%s",
fsck_msg_types.buf);
- if (fix_thin)
+ if (!reject_thin)
argv_array_push(&child.args, "--fix-thin");
child.out = -1;
child.err = err_fd;
@@ -1707,45 +1710,29 @@ static int delete_only(struct command *commands)
int cmd_receive_pack(int argc, const char **argv, const char *prefix)
{
int advertise_refs = 0;
- int i;
struct command *commands;
struct sha1_array shallow = SHA1_ARRAY_INIT;
struct sha1_array ref = SHA1_ARRAY_INIT;
struct shallow_info si;
+ struct option options[] = {
+ OPT__QUIET(&quiet, N_("quiet")),
+ OPT_HIDDEN_BOOL(0, "stateless-rpc", &stateless_rpc, NULL),
+ OPT_HIDDEN_BOOL(0, "advertise-refs", &advertise_refs, NULL),
+ OPT_HIDDEN_BOOL(0, "reject-thin-pack-for-testing",
&reject_thin, NULL),
+ OPT_END()
+ };
+
packet_trace_identity("receive-pack");
- argv++;
- for (i = 1; i < argc; i++) {
- const char *arg = *argv++;
+ argc = parse_options(argc, argv, prefix, options, receive_pack_usage,
0);
- if (*arg == '-') {
- if (!strcmp(arg, "--quiet")) {
- quiet = 1;
- continue;
- }
+ if (argc > 1)
+ usage_msg_opt(_("Too many arguments."), receive_pack_usage,
options);
+ if (argc == 0)
+ usage_msg_opt(_("You must specify a directory."),
receive_pack_usage, options);
- if (!strcmp(arg, "--advertise-refs")) {
- advertise_refs = 1;
- continue;
- }
- if (!strcmp(arg, "--stateless-rpc")) {
- stateless_rpc = 1;
- continue;
- }
- if (!strcmp(arg, "--reject-thin-pack-for-testing")) {
- fix_thin = 0;
- continue;
- }
-
- usage(receive_pack_usage);
- }
- if (service_dir)
- usage(receive_pack_usage);
- service_dir = arg;
- }
- if (!service_dir)
- usage(receive_pack_usage);
+ service_dir = argv[0];
setup_path();
--
2.7.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
git add -p refuses to apply an edited patch that otherwise applies
Hi,
As subject, the problem I'm facing is that, while doing an interactive
add, an edited patch fails to apply. The same patch content
successfully applies otherwise, with git apply.
To reproduce the problem:
- Add the attached site.css to an empty git repository, and make an
initial commit
- Apply the attached full.patch using 'patch -p1'
- Do 'git add -p', and choose to edit the second hunk (i.e., s, n, e)
- Edit the presented patch look like the content of attached
no_problem.patch; write and quit
The patch fails to apply.
To see git apply the patch normally, do 'git reset --hard'. Now apply
the no_problem.patch as
git apply --cached --recount < no_problem.patch
AFAIU, the command above is what the interactive script uses to apply
the patch to the index, but I'm confused as to why an edited patch
fails.
Any thoughts? FWIW, I'm using version 2.7.2.
--
Jeenu
diff --git a/site.css b/site.css
index 68a88ae..143838c 100644
--- a/site.css
+++ b/site.css
@@ -53,14 +57,23 @@ h4 {
overflow-y: auto;
}
-p {
- margin: 15px 0px;
+/* Table of contents */
+#toc {
+ margin: 20px 0px 20px 0px;
+ padding-left: 5px;
}
-
-h1, h2, h3, h4 {
+#toc li {
+ display: block;
+}
+#toc ul {
+ padding-left: 10px;
margin: 5px 0px;
}
+p {
+ margin: 15px 0px;
+}
+
/*
* For screen wider than 480px, fix the left bar, and set 25/75 split.
* Also set a max width of 250px and 750px max-width respectively
diff --git a/site.css b/site.css
index 68a88ae..143838c 100644
--- a/site.css
+++ b/site.css
@@ -58,6 +60,9 @@
}
-
-h1, h2, h3, h4 {
- margin: 5px 0px;
-}
/* Styles to be applied for every one */
body {
position: relative;
font-family: "PT Sans", sans-serif;
font-size: 16px;
text-rendering: optimizeLegibility;
line-height: 1.4em;
word-spacing: 0.05em;
box-sizing: border-box;
margin: 0px;
padding: 0px;
color: #333;
}
/* Use progressively smaller headings */
h1 {
font-size: 2em;
}
h2 {
font-size: 1.7em;
}
h3 {
font-size: 1.5em;
}
h4 {
font-size: 1.3em;
}
#left-bar, #main {
/* Use border box */
box-sizing: border-box;
/* Use 5px padding overall; a little more on the left */
padding: 5px;
padding-left: 10px;
}
#left-bar {
/* Use a smaller font */
font-size: 0.9em;
/* Extra padding to right */
padding-right: 10px;
/* Fix height at 100% so that we get a scrollbar */
max-height: 100%;
overflow-y: auto;
}
p {
margin: 15px 0px;
}
h1, h2, h3, h4 {
margin: 5px 0px;
}
/*
* For screen wider than 480px, fix the left bar, and set 25/75 split.
* Also set a max width of 250px and 750px max-width respectively
*/
Re: [PATCH v2] Mark win32's pthread_exit() as NORETURN
Johannes Sixt writes: > Am 01.03.2016 um 15:13 schrieb Johannes Schindelin: >> The pthread_exit() function is not expected to return. Ever. On Windows, >> we call ExitThread() whose documentation claims: "This function does not >> return a value.": >> >> https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659 > > This is misleading: MSDN marks all functions declared void as "does > not return a value," for example, look at EnterCriticalSection: > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms682608 > > For this reason, I actually prefer your version 1 patch without the > explanation. ;-) >> -static inline int pthread_exit(void *ret) >> +static inline int NORETURN pthread_exit(void *ret) > > I would have written it as > > #ifdef __GNUC__ > __attribute__((__noreturn__)) > #endif > static inline int pthread_exit(void *ret) ... > > but I can live with your version as long as it compiles. Either way, let's make sure that the final version returns "void", cf. http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_exit.html 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 v2] Mark win32's pthread_exit() as NORETURN
Am 01.03.2016 um 15:13 schrieb Johannes Schindelin:
The pthread_exit() function is not expected to return. Ever. On Windows,
we call ExitThread() whose documentation claims: "This function does not
return a value.":
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659
This is misleading: MSDN marks all functions declared void as "does not
return a value," for example, look at EnterCriticalSection:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682608
For this reason, I actually prefer your version 1 patch without the
explanation.
Pointed out by Jeff King.
Signed-off-by: Johannes Schindelin
---
Relative to v1, only the commit message changed (to clarify that
ExitThread() indeed never returns).
compat/win32/pthread.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 20b35a2..148db60 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void
**value_ptr);
#define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
extern pthread_t pthread_self(void);
-static inline int pthread_exit(void *ret)
+static inline int NORETURN pthread_exit(void *ret)
I would have written it as
#ifdef __GNUC__
__attribute__((__noreturn__))
#endif
static inline int pthread_exit(void *ret) ...
but I can live with your version as long as it compiles.
Your solution is pragmatic: NORETURN is defined in git-compat-util.h,
and by using it here, we depend on that pthread.h is included
sufficiently late that the macro is available at this point. The
instance in compat/nedmalloc/malloc.c.h is bracketed with #ifndef WIN32
so that it is not compiled on Windows, all other instances are after
git-compat-util.h or cache.h or in headers that are to be included only
after git-compat-util.h or cache.h per convention. Looks like we are safe.
{
ExitThread((DWORD)(intptr_t)ret);
}
--
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 v7 29/33] setup: configure ref storage on setup
On Tue, 2016-03-01 at 17:18 +, Ramsay Jones wrote: > > On 01/03/16 00:53, David Turner wrote: > > This sets up the existing backend early, so that other code which > > reads refs is reading from the right place. > > > > Signed-off-by: David Turner > > Signed-off-by: Junio C Hamano > > --- > > config.c | 1 + > > setup.c | 4 > > 2 files changed, 5 insertions(+) > > > > diff --git a/config.c b/config.c > > index 9ba40bc..cca7e28 100644 > > --- a/config.c > > +++ b/config.c > > @@ -11,6 +11,7 @@ > > #include "strbuf.h" > > #include "quote.h" > > #include "hashmap.h" > > +#include "refs.h" > > #include "string-list.h" > > #include "utf8.h" > > > > I was just skimming these patches as they passed by, and this > caught my eye. If this include is required (eg. to fix a compiler > warning), then it should probably be in an earlier patch. > Otherwise, it should be in a later patch, no? Actually, it's cruft from the previous version of this series :(. I looked at the patch and didn't notice that it was in config.c instead of setup.c. Oops. Will remove. -- 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 v7 30/33] refs: break out resolve_ref_unsafe_submodule
On Tue, 2016-03-01 at 17:21 +, Ramsay Jones wrote: > > On 01/03/16 00:53, David Turner wrote: > > It will soon be useful for resolve_ref_unsafe to support > > submodules. > > But since it is called from so many places, changing it would have > > been painful. Fortunately, it's just a thin wrapper around (the > > former) resolve_ref_1. So now resolve_ref_1 becomes > > resolve_ref_unsafe_submodule, and it passes its submodule argument > > through to read_raw_ref. > > > > The files backend doesn't need this functionality, but it doesn't > > hurt. > > > > Signed-off-by: David Turner > > Signed-off-by: Junio C Hamano > > --- > > refs.c | 41 +--- > > - > > refs/files-backend.c | 8 ++-- > > refs/refs-internal.h | 19 --- > > 3 files changed, 47 insertions(+), 21 deletions(-) > > > > diff --git a/refs.c b/refs.c > > index 5fe0bac..d1cf707 100644 > > --- a/refs.c > > +++ b/refs.c > > @@ -60,6 +60,9 @@ void register_ref_storage_backends(void) > > * entries below when you add a new backend. > > */ > > register_ref_storage_backend(&refs_be_files); > > +#ifdef USE_LIBLMDB > > + register_ref_storage_backend(&refs_be_lmdb); > > +#endif > > Again, just skimming patches, ... > > The lmdb refs backend (hence refs_be_lmdb) is not introduced until > the next patch [31/33], right? Yep. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-p4: map a P4 user to Git author name and email address
On Tue, Mar 1, 2016 at 5:49 AM, wrote: > Map a P4 user to a specific name and email address in Git with the > "git-p4.mapUser" config. The config value must be a string adhering > to the format "p4user = First Lastname ". > > Signed-off-by: Lars Schneider > --- > diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt > +git-p4.mapUser:: > + Map a P4 user to a name and email address in Git. Use a string > + with the following format to create a mapping: > ++ > +- > +git config --add git-p4.mapUser "p4user = First Last " > +- > ++ > +A mapping will override any user information from P4. Mappings for > +multiple P4 user can be defined. Sorry for not paying closer attention the first time, but this needs to be repeated for each P4 user you want to map, right? One can imagine this quickly becoming painful if you have a lot of users to map. Have you considered modeling this after git-svn where you can set an "authors" file (and name the corresponding option --authors-file)? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line
Jacob Keller writes:
> On Tue, Mar 1, 2016 at 9:26 AM, Junio C Hamano wrote:
>> I find this in t/lib-httpd.sh:
>>
>> set_askpass() {
>> >"$TRASH_DIRECTORY/askpass-query" &&
>> echo "$1" >"$TRASH_DIRECTORY/askpass-user" &&
>> echo "$2" >"$TRASH_DIRECTORY/askpass-pass"
>> }
>>
>> and expect_askpass peeks at askpass-query to see if Git asked the
>> right questions.
>>
>> I think askpass-query is cleared here because the author of the test
>> suite expected that the helpers are used in such a way that you
>> always (1) set-askpass once, (2) run a Git command that asks
>> credentials, (3) use expect_askpass to validate and do these three
>> steps as a logical unit?
>>
>> That "clone" the test expects to fail does ask the credential, so
>> even though the test does not check if the "clone" asked the right
>> question, it finishes the three-step logical unit, and then you need
>> to clear askpass-query.
>>
>> It may have been cleaner if you had clear_askpass_query helper that
>> is called (1) at the beginning of set_askpass instead of this manual
>> clearing, (2) at the end of expect_askpass, as the exchange has been
>> tested already at that point, and (3) in place of expect_askpass if
>> you choose not to call it (e.g. this place, instead of the second
>> set_askpass, you would say clear_askpass_query), perhaps, but I do
>> know if that is worth it.
>
> Probably something worth looking at doing in the future.
>
> I could call expect_askpass here at each time but I don't think it
> would be meaningful after a test_must_fail.
Even if you call expect_askpass to check, another set_askpass is
expected to start the next cycle anyway (unless we restructure the
helpers around clear_askpass_query I alluded to, which I am not
convinced is a good idea yet), so you'll still be asked "why another
set_askpass to set the same 'wrong pass@host'?".
So, I dunno.
--
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] builtin/receive-pack.c: use parse_options API
On Tue, Mar 1, 2016 at 12:57 PM, Matthieu Moy wrote: > Sidhant Sharma writes: >> Another thing I'd like to ask is when I prepare the next patch, should >> it be sent as reply in this thread, or as a new thread? > > No strict rule on that, but I usually use --in-reply-to on the root of > the thread for previous iteration. If you don't, include a link (e.g. > gmane) to the previous iteration in the cover-letter. It's good manners to include a link to the previous version even if you do use --in-reply-to since not all reviewers will have the previous thread in their mailbox. -- 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] Store EXC_FLAG_* values in unsigned integers
Saurav Sachidanand writes: > The values defined by the macro EXC_FLAG_* (1, 4, 8, 16) are > stored in fields of the structs "pattern" and “exclude”, some > functions arguments and a local variable. It's a minor point, but it is somewhat irritating that "pattern" is enclosed in a regular dq pair while "exclude" is in a fancy dq pair. I think our log messages tend to prefer the regular ones. No need to resend; 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] environment.c: introduce DECLARE_GIT_GETTER helper macro
Jeff King writes: > On Sun, Feb 28, 2016 at 01:35:44AM +0600, Alexander Kuleshov wrote: > >> +DECLARE_GIT_GETTER(const char *, get_git_dir, git_dir) >> +DECLARE_GIT_GETTER(const char *, get_git_namespace, namespace) >> +DECLARE_GIT_GETTER(char *, get_object_directory, git_object_dir) >> +DECLARE_GIT_GETTER(char *, get_index_file, git_index_file) >> +DECLARE_GIT_GETTER(char *, get_graft_file, git_graft_file) > > Hmm. I'm somewhat lukewarm on this patch. It's fewer lines and less > duplication, which is nice, but this kind of code generation often makes > things annoying (to step into with the debugger, to find with ctags, > etc). I dunno. For this particular set of functions, single-step-ability would not be a huge issue, but I am not enthused, either, even though these are vastly more palatable than what was originally proposed. Another minor annoyance is that I expect to see a semicolon after a pair of parentheses that follows a token, but adding one of course would break the compilation. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line
On Tue, Mar 1, 2016 at 9:26 AM, Junio C Hamano wrote:
> I find this in t/lib-httpd.sh:
>
> set_askpass() {
> >"$TRASH_DIRECTORY/askpass-query" &&
> echo "$1" >"$TRASH_DIRECTORY/askpass-user" &&
> echo "$2" >"$TRASH_DIRECTORY/askpass-pass"
> }
>
> and expect_askpass peeks at askpass-query to see if Git asked the
> right questions.
>
> I think askpass-query is cleared here because the author of the test
> suite expected that the helpers are used in such a way that you
> always (1) set-askpass once, (2) run a Git command that asks
> credentials, (3) use expect_askpass to validate and do these three
> steps as a logical unit?
>
> That "clone" the test expects to fail does ask the credential, so
> even though the test does not check if the "clone" asked the right
> question, it finishes the three-step logical unit, and then you need
> to clear askpass-query.
>
> It may have been cleaner if you had clear_askpass_query helper that
> is called (1) at the beginning of set_askpass instead of this manual
> clearing, (2) at the end of expect_askpass, as the exchange has been
> tested already at that point, and (3) in place of expect_askpass if
> you choose not to call it (e.g. this place, instead of the second
> set_askpass, you would say clear_askpass_query), perhaps, but I do
> know if that is worth it.
>
Probably something worth looking at doing in the future.
I could call expect_askpass here at each time but I don't think it
would be meaningful after a test_must_fail.
Thanks,
Jake
--
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] builtin/receive-pack.c: use parse_options API
Sidhant Sharma writes:
>>> + if (argc > 1)
>>> + usage_msg_opt(_("Too many arguments."), receive_pack_usage,
>>> options);
>>> + if (argc == 0)
>>> + usage_msg_opt(_("You must specify a directory."),
>>> receive_pack_usage, options);
>> Before that, the loop was ensuring that service_dir was assigned once
>> and only once, and now you check that you have one non-option arg and
>> assign it unconditionally:
>>
>>> + service_dir = argv[0];
>> ... so isn't this "if" dead code:
>>
>>> if (!service_dir)
>>> - usage(receive_pack_usage);
>>> + usage_with_options(receive_pack_usage, options);
>> ?
>>
>>
> Yes, I just realized that is dead code (sorry). Removing the 'if'
> statement would correct that?
Yes.
> Also, is the unconditional assignment to service_dir correct in this
> case, or should some other test condition be added?
Since usage_msg_opt is NORETURN, it's OK: if you reach this point, you
know that argv[0] contains something.
> Another thing I'd like to ask is when I prepare the next patch, should
> it be sent as reply in this thread, or as a new thread?
No strict rule on that, but I usually use --in-reply-to on the root of
the thread for previous iteration. If you don't, include a link (e.g.
gmane) to the previous iteration in the cover-letter.
format-patch has a -v2 option to let you get [PATCH v2 ...]
automatically.
--
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: [PATCH v2] Mark win32's pthread_exit() as NORETURN
writes:
> Am 01.03.2016 um 15:13 schrieb Johannes Schindelin:
>> The pthread_exit() function is not expected to return. Ever. On Windows,
>> we call ExitThread() whose documentation claims: "This function does not
>> return a value.":
>
> Does this really mean that ExitThread() does not return ?
>
> Just wondering...
;-).
That made me re-read the patch and notice another thing... Dscho, I
think you would need s/int/void/, too, no?
>
>> https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659
>>
>> Pointed out by Jeff King.
>>
>> Signed-off-by: Johannes Schindelin
>> ---
>>
>> Relative to v1, only the commit message changed (to clarify that
>> ExitThread() indeed never returns).
>>
>> compat/win32/pthread.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
>> index 20b35a2..148db60 100644
>> --- a/compat/win32/pthread.h
>> +++ b/compat/win32/pthread.h
>> @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void
>> **value_ptr);
>> #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
>> extern pthread_t pthread_self(void);
>>
>> -static inline int pthread_exit(void *ret)
>> +static inline int NORETURN pthread_exit(void *ret)
>> {
>> ExitThread((DWORD)(intptr_t)ret);
>> }
>> --
>
>
> Stefan
--
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
[PATCHv2] run-command: do not pass child process data into callbacks
The expected way to pass data into the callback is to pass them via
the customizable callback pointer. The error reporting in
default_{start_failure, task_finished} is not user friendly enough, that
we want to encourage using the child data for such purposes.
Furthermore the struct child data is cleaned by the run-command API,
before we access them in the callbacks, leading to use-after-free
situations.
Signed-off-by: Stefan Beller
---
Thanks Johannes for reviewing and double checking. I squashed in your
proposed documentation change.
This applies on 2.8.0-rc0.
Thanks,
Stefan
run-command.c | 24 +++-
run-command.h | 9 +++--
submodule.c| 7 +++
test-run-command.c | 1 -
4 files changed, 9 insertions(+), 32 deletions(-)
diff --git a/run-command.c b/run-command.c
index 51fd72c..8e3ad07 100644
--- a/run-command.c
+++ b/run-command.c
@@ -902,35 +902,18 @@ struct parallel_processes {
struct strbuf buffered_output; /* of finished children */
};
-static int default_start_failure(struct child_process *cp,
-struct strbuf *err,
+static int default_start_failure(struct strbuf *err,
void *pp_cb,
void *pp_task_cb)
{
- int i;
-
- strbuf_addstr(err, "Starting a child failed:");
- for (i = 0; cp->argv[i]; i++)
- strbuf_addf(err, " %s", cp->argv[i]);
-
return 0;
}
static int default_task_finished(int result,
-struct child_process *cp,
struct strbuf *err,
void *pp_cb,
void *pp_task_cb)
{
- int i;
-
- if (!result)
- return 0;
-
- strbuf_addf(err, "A child failed with return code %d:", result);
- for (i = 0; cp->argv[i]; i++)
- strbuf_addf(err, " %s", cp->argv[i]);
-
return 0;
}
@@ -1048,8 +1031,7 @@ static int pp_start_one(struct parallel_processes *pp)
pp->children[i].process.no_stdin = 1;
if (start_command(&pp->children[i].process)) {
- code = pp->start_failure(&pp->children[i].process,
-&pp->children[i].err,
+ code = pp->start_failure(&pp->children[i].err,
pp->data,
&pp->children[i].data);
strbuf_addbuf(&pp->buffered_output, &pp->children[i].err);
@@ -1117,7 +1099,7 @@ static int pp_collect_finished(struct parallel_processes
*pp)
code = finish_command(&pp->children[i].process);
- code = pp->task_finished(code, &pp->children[i].process,
+ code = pp->task_finished(code,
&pp->children[i].err, pp->data,
&pp->children[i].data);
diff --git a/run-command.h b/run-command.h
index d5a57f9..6e17894 100644
--- a/run-command.h
+++ b/run-command.h
@@ -158,8 +158,7 @@ typedef int (*get_next_task_fn)(struct child_process *cp,
* To send a signal to other child processes for abortion, return
* the negative signal number.
*/
-typedef int (*start_failure_fn)(struct child_process *cp,
- struct strbuf *err,
+typedef int (*start_failure_fn)(struct strbuf *err,
void *pp_cb,
void *pp_task_cb);
@@ -178,7 +177,6 @@ typedef int (*start_failure_fn)(struct child_process *cp,
* the negative signal number.
*/
typedef int (*task_finished_fn)(int result,
- struct child_process *cp,
struct strbuf *err,
void *pp_cb,
void *pp_task_cb);
@@ -192,9 +190,8 @@ typedef int (*task_finished_fn)(int result,
* (both stdout and stderr) is routed to stderr in a manner that output
* from different tasks does not interleave.
*
- * If start_failure_fn or task_finished_fn are NULL, default handlers
- * will be used. The default handlers will print an error message on
- * error without issuing an emergency stop.
+ * start_failure_fn and task_finished_fn can be NULL to omit any
+ * special handling.
*/
int run_processes_parallel(int n,
get_next_task_fn,
diff --git a/submodule.c b/submodule.c
index b83939c..916fc84 100644
--- a/submodule.c
+++ b/submodule.c
@@ -705,8 +705,7 @@ static int get_next_submodule(struct child_process *cp,
return 0;
}
-static int fetch_start_failure(struct child_process *cp,
- struct strbuf *err,
+static int fetch_start_failure(struct strbuf *err,
void *cb, void *task_cb)
{
struct submodule_parallel_fetch *spf = cb;
@@ -716,8 +715,8 @@ static int fetch_start_failure(struct child_process *cp,
Re: [PATCH] builtin/receive-pack.c: use parse_options API
> Hi,
>
> Thanks for your patch.
>
> "Sidhant Sharma [:tk]" writes:
>
>> This patch makes receive-pack use the parse_options API,
> We usually avoid saying "this patch" and use imperative tone: talk to
> your patch and give it orders like "Make receive-pack use the
> parse_options API ...". Or just skip that part which is already in the
> title.
>
>> @@ -45,12 +48,12 @@ static int unpack_limit = 100;
>> static int report_status;
>> static int use_sideband;
>> static int use_atomic;
>> -static int quiet;
>> +static int quiet = 0;
> static int are already initialized to 0, you don't need this explicit "=
> 0". In the codebase of Git, we prever omiting the initialization.
>
>> +struct option options[] = {
>> +OPT__QUIET(&quiet, N_("quiet")),
>> +OPT_HIDDEN_BOOL(0, "stateless-rpc", &stateless_rpc, NULL),
>> +OPT_HIDDEN_BOOL(0, "advertise-refs", &advertise_refs, NULL),
>> +/* Hidden OPT_BOOL option */
>> +{
>> +OPTION_SET_INT, 0, "reject-thin-pack-for-testing",
>> &fix_thin, NULL,
>> +NULL, PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0,
>> +},
> After seeing the patch, I think the code would be clearer by using
> something like
>
> OPT_HIDDEN_BOOL(0, "reject-thin-pack-for-testing", &reject_thin, NULL)
>
> and then use !reject_thin where the patch was using fix_thin. Turns 5
> lines into one here, and you just pay a ! later in terms of readability.
OK, will correct the above points.
>> packet_trace_identity("receive-pack");
>>
>> -argv++;
>> -for (i = 1; i < argc; i++) {
>> -const char *arg = *argv++;
>> +argc = parse_options(argc, argv, prefix, options, receive_pack_usage,
>> 0);
>>
>> -if (*arg == '-') {
>> -if (!strcmp(arg, "--quiet")) {
>> -quiet = 1;
>> -continue;
>> -}
>> +if (argc > 1)
>> +usage_msg_opt(_("Too many arguments."), receive_pack_usage,
>> options);
>> +if (argc == 0)
>> +usage_msg_opt(_("You must specify a directory."),
>> receive_pack_usage, options);
> Before that, the loop was ensuring that service_dir was assigned once
> and only once, and now you check that you have one non-option arg and
> assign it unconditionally:
>
>> +service_dir = argv[0];
> ... so isn't this "if" dead code:
>
>> if (!service_dir)
>> -usage(receive_pack_usage);
>> +usage_with_options(receive_pack_usage, options);
> ?
>
>
Yes, I just realized that is dead code (sorry). Removing the 'if' statement
would correct that? Also, is the unconditional assignment to service_dir
correct in this case, or should some other test condition be added?
Another thing I'd like to ask is when I prepare the next patch, should it be
sent as reply in this thread, or as a new thread?
Thanks and regards,
Sidhant Sharma [:tk]
--
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] run-command: do not pass child process data into callbacks
Johannes Sixt writes:
> Am 29.02.2016 um 22:57 schrieb Stefan Beller:
>> The expected way to pass data into the callback is to pass them via
>> the customizable callback pointer. The error reporting in
>> default_{start_failure, task_finished} is not user friendly enough, that
>> we want to encourage using the child data for such purposes.
>>
>> Furthermore the struct child data is cleaned by the run-command API,
>> before we access them in the callbacks, leading to use-after-free
>> situations.
>
> Thanks. The code changes match what I had prototyped. But please squash
> in this documentation change:
>
> diff --git a/run-command.h b/run-command.h
> index c6a3e42..3d1e59e 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -191,9 +191,8 @@ typedef int (*task_finished_fn)(int result,
> * (both stdout and stderr) is routed to stderr in a manner that output
> * from different tasks does not interleave.
> *
> - * If start_failure_fn or task_finished_fn are NULL, default handlers
> - * will be used. The default handlers will print an error message on
> - * error without issuing an emergency stop.
> + * start_failure_fn and task_finished_fn can be NULL to omit any
> + * special handling.
> */
> int run_processes_parallel(int n,
> get_next_task_fn,
Thanks for careful reading.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 7/7] git: submodule honor -c credential.* from command line
Stefan Beller writes:
> On Mon, Feb 29, 2016 at 2:58 PM, Jacob Keller
> wrote:
>>
>> +test_expect_success 'cmdline credential config passes into submodules' '
>> + git init super &&
>> + set_askpass user@host pass@host &&
>
> I wonder why we use pass@host as a password, it seems confusing to me.
> p@ssword would have worked if we wanted to test a password containing an @,
> pass@host doesn't quite fit my mental model of how passwords work.
> No need to change anything, better be consistent with the rest of the tests.
>
>
>> + (
>> + cd super &&
>> + git submodule add "$HTTPD_URL/auth/dumb/repo.git" sub &&
>> + git commit -m "add submodule"
>> + ) &&
>> + set_askpass wrong pass@host &&
>> + test_must_fail git clone --recursive super super-clone &&
>> + rm -rf super-clone &&
>> + set_askpass wrong pass@host &&
>
> Why set set_askpass a second time here?
I find this in t/lib-httpd.sh:
set_askpass() {
>"$TRASH_DIRECTORY/askpass-query" &&
echo "$1" >"$TRASH_DIRECTORY/askpass-user" &&
echo "$2" >"$TRASH_DIRECTORY/askpass-pass"
}
and expect_askpass peeks at askpass-query to see if Git asked the
right questions.
I think askpass-query is cleared here because the author of the test
suite expected that the helpers are used in such a way that you
always (1) set-askpass once, (2) run a Git command that asks
credentials, (3) use expect_askpass to validate and do these three
steps as a logical unit?
That "clone" the test expects to fail does ask the credential, so
even though the test does not check if the "clone" asked the right
question, it finishes the three-step logical unit, and then you need
to clear askpass-query.
It may have been cleaner if you had clear_askpass_query helper that
is called (1) at the beginning of set_askpass instead of this manual
clearing, (2) at the end of expect_askpass, as the exchange has been
tested already at that point, and (3) in place of expect_askpass if
you choose not to call it (e.g. this place, instead of the second
set_askpass, you would say clear_askpass_query), perhaps, but I do
know if that is worth it.
--
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] builtin/receive-pack.c: use parse_options API
Hi,
Thanks for your patch.
"Sidhant Sharma [:tk]" writes:
> This patch makes receive-pack use the parse_options API,
We usually avoid saying "this patch" and use imperative tone: talk to
your patch and give it orders like "Make receive-pack use the
parse_options API ...". Or just skip that part which is already in the
title.
> @@ -45,12 +48,12 @@ static int unpack_limit = 100;
> static int report_status;
> static int use_sideband;
> static int use_atomic;
> -static int quiet;
> +static int quiet = 0;
static int are already initialized to 0, you don't need this explicit "=
0". In the codebase of Git, we prever omiting the initialization.
> + struct option options[] = {
> + OPT__QUIET(&quiet, N_("quiet")),
> + OPT_HIDDEN_BOOL(0, "stateless-rpc", &stateless_rpc, NULL),
> + OPT_HIDDEN_BOOL(0, "advertise-refs", &advertise_refs, NULL),
> + /* Hidden OPT_BOOL option */
> + {
> + OPTION_SET_INT, 0, "reject-thin-pack-for-testing",
> &fix_thin, NULL,
> + NULL, PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0,
> + },
After seeing the patch, I think the code would be clearer by using
something like
OPT_HIDDEN_BOOL(0, "reject-thin-pack-for-testing", &reject_thin, NULL)
and then use !reject_thin where the patch was using fix_thin. Turns 5
lines into one here, and you just pay a ! later in terms of readability.
Starting from here, the patch is a bit painful to read because the diff
heuristics grouped hunks in a strange way. You may try "git format-patch
--patience" or --minimal or --histogram to see if it gives a better
result. The final commit would be the same, but it may make review
easier.
(Not blaming you, just pointing a potentially useful hint, don't worry)
> packet_trace_identity("receive-pack");
>
> - argv++;
> - for (i = 1; i < argc; i++) {
> - const char *arg = *argv++;
> + argc = parse_options(argc, argv, prefix, options, receive_pack_usage,
> 0);
>
> - if (*arg == '-') {
> - if (!strcmp(arg, "--quiet")) {
> - quiet = 1;
> - continue;
> - }
> + if (argc > 1)
> + usage_msg_opt(_("Too many arguments."), receive_pack_usage,
> options);
> + if (argc == 0)
> + usage_msg_opt(_("You must specify a directory."),
> receive_pack_usage, options);
Before that, the loop was ensuring that service_dir was assigned once
and only once, and now you check that you have one non-option arg and
assign it unconditionally:
> + service_dir = argv[0];
... so isn't this "if" dead code:
> if (!service_dir)
> - usage(receive_pack_usage);
> + usage_with_options(receive_pack_usage, options);
?
--
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: [PATCH v7 30/33] refs: break out resolve_ref_unsafe_submodule
On 01/03/16 00:53, David Turner wrote: > It will soon be useful for resolve_ref_unsafe to support submodules. > But since it is called from so many places, changing it would have > been painful. Fortunately, it's just a thin wrapper around (the > former) resolve_ref_1. So now resolve_ref_1 becomes > resolve_ref_unsafe_submodule, and it passes its submodule argument > through to read_raw_ref. > > The files backend doesn't need this functionality, but it doesn't > hurt. > > Signed-off-by: David Turner > Signed-off-by: Junio C Hamano > --- > refs.c | 41 + > refs/files-backend.c | 8 ++-- > refs/refs-internal.h | 19 --- > 3 files changed, 47 insertions(+), 21 deletions(-) > > diff --git a/refs.c b/refs.c > index 5fe0bac..d1cf707 100644 > --- a/refs.c > +++ b/refs.c > @@ -60,6 +60,9 @@ void register_ref_storage_backends(void) >* entries below when you add a new backend. >*/ > register_ref_storage_backend(&refs_be_files); > +#ifdef USE_LIBLMDB > + register_ref_storage_backend(&refs_be_lmdb); > +#endif Again, just skimming patches, ... The lmdb refs backend (hence refs_be_lmdb) is not introduced until the next patch [31/33], right? ATB, Ramsay Jones -- 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 v7 29/33] setup: configure ref storage on setup
On 01/03/16 00:53, David Turner wrote:
> This sets up the existing backend early, so that other code which
> reads refs is reading from the right place.
>
> Signed-off-by: David Turner
> Signed-off-by: Junio C Hamano
> ---
> config.c | 1 +
> setup.c | 4
> 2 files changed, 5 insertions(+)
>
> diff --git a/config.c b/config.c
> index 9ba40bc..cca7e28 100644
> --- a/config.c
> +++ b/config.c
> @@ -11,6 +11,7 @@
> #include "strbuf.h"
> #include "quote.h"
> #include "hashmap.h"
> +#include "refs.h"
> #include "string-list.h"
> #include "utf8.h"
>
I was just skimming these patches as they passed by, and this
caught my eye. If this include is required (eg. to fix a compiler
warning), then it should probably be in an earlier patch.
Otherwise, it should be in a later patch, no?
ATB,
Ramsay Jones
> diff --git a/setup.c b/setup.c
> index bd3a2cf..e2e1220 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -457,6 +457,10 @@ static int check_repository_format_gently(const char
> *gitdir, int *nongit_ok)
> ret = -1;
> }
>
> + register_ref_storage_backends();
> + if (set_ref_storage_backend(ref_storage_backend))
> + die(_("Unknown ref storage backend %s"), ref_storage_backend);
> +
> strbuf_release(&sb);
> return ret;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] lockfile: improve error message when lockfile exists
A common mistake leading a user to see this message is to launch "git
commit", let the editor open (and forget about it), and try again to
commit.
The previous message was going too quickly to "a git process crashed"
and to the advice "remove the file manually".
This patch modifies the message in two ways: first, it considers that
"another process is running" is the norm, not the exception, and it
explicitly hints the user to look at text editors.
The message is 2 lines longer, but this is not a problem since
experienced users do not see the message often.
Helped-by: Moritz Neeb
Signed-off-by: Matthieu Moy
---
lockfile.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 62583d1..9268cdf 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -150,9 +150,11 @@ void unable_to_lock_message(const char *path, int err,
struct strbuf *buf)
{
if (err == EEXIST) {
strbuf_addf(buf, _("Unable to create '%s.lock': %s.\n\n"
- "If no other git process is currently running, this
probably means a\n"
- "git process crashed in this repository earlier. Make sure
no other git\n"
- "process is running and remove the file manually to
continue."),
+ "Another git process seems to be running in this
repository, e.g.\n"
+ "an editor opened by 'git commit'. Please make sure all
processes\n"
+ "are terminated then try again. If it still fails, a git
process\n"
+ "may have crashed in this repository earlier:\n"
+ "remove the file manually to continue."),
absolute_path(path), strerror(err));
} else
strbuf_addf(buf, _("Unable to create '%s.lock': %s"),
--
2.7.2.334.g35ed2ae.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] lockfile: mark strings for translation
Signed-off-by: Matthieu Moy
---
lockfile.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 80d056d..62583d1 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -149,13 +149,13 @@ static int lock_file_timeout(struct lock_file *lk, const
char *path,
void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
{
if (err == EEXIST) {
- strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
+ strbuf_addf(buf, _("Unable to create '%s.lock': %s.\n\n"
"If no other git process is currently running, this
probably means a\n"
"git process crashed in this repository earlier. Make sure
no other git\n"
- "process is running and remove the file manually to
continue.",
+ "process is running and remove the file manually to
continue."),
absolute_path(path), strerror(err));
} else
- strbuf_addf(buf, "Unable to create '%s.lock': %s",
+ strbuf_addf(buf, _("Unable to create '%s.lock': %s"),
absolute_path(path), strerror(err));
}
--
2.7.2.334.g35ed2ae.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Store EXC_FLAG_* values in unsigned integers
The values defined by the macro EXC_FLAG_* (1, 4, 8, 16) are
stored in fields of the structs "pattern" and “exclude”, some
functions arguments and a local variable.
No variable that holds these values uses its most significant
bit in any special way, as it’s value is either checked for a
variant of EXC_FLAG_* using the & operator
(flags & EXC_FLAG_NODIR), or assigned a value of 0 first
and then any one of {1, 4, 8, 16} using the | operator
(flags |= EXC_FLAG_NODIR). Hence, change the types of such
variables and fields to unsigned.
And while we’re at it, document "flags" of "exclude" to explicitly
state the values it’s supposed to take on.
Signed-off-by: Saurav Sachidanand
---
This is a patch for the suggested microproject for GSoC 2016, titled
"Use unsigned integral type for collection of bits." It’s the fourth
iteration of this patch that incorporates changes to the commit
message suggested by Moritz Neeb, Eric Sunshine and Junio C Hamano,
and to some function signatures suggested by Duy Nguyen. Thanks to
them for their feedback.
Previous versions of this patch:
1) http://thread.gmane.org/gmane.comp.version-control.git/286821
2) http://thread.gmane.org/gmane.comp.version-control.git/287387
3) http://thread.gmane.org/gmane.comp.version-control.git/287838
attr.c | 2 +-
dir.c | 8
dir.h | 8
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/attr.c b/attr.c
index 086c08d..679e13c 100644
--- a/attr.c
+++ b/attr.c
@@ -124,7 +124,7 @@ struct pattern {
const char *pattern;
int patternlen;
int nowildcardlen;
- int flags; /* EXC_FLAG_* */
+ unsigned flags; /* EXC_FLAG_* */
};
/*
diff --git a/dir.c b/dir.c
index 552af23..82cec7d 100644
--- a/dir.c
+++ b/dir.c
@@ -459,7 +459,7 @@ int no_wildcard(const char *string)
void parse_exclude_pattern(const char **pattern,
int *patternlen,
- int *flags,
+ unsigned *flags,
int *nowildcardlen)
{
const char *p = *pattern;
@@ -500,7 +500,7 @@ void add_exclude(const char *string, const char *base,
{
struct exclude *x;
int patternlen;
- int flags;
+ unsigned flags;
int nowildcardlen;
parse_exclude_pattern(&string, &patternlen, &flags, &nowildcardlen);
@@ -811,7 +811,7 @@ void add_excludes_from_file(struct dir_struct *dir, const
char *fname)
int match_basename(const char *basename, int basenamelen,
const char *pattern, int prefix, int patternlen,
- int flags)
+ unsigned flags)
{
if (prefix == patternlen) {
if (patternlen == basenamelen &&
@@ -836,7 +836,7 @@ int match_basename(const char *basename, int basenamelen,
int match_pathname(const char *pathname, int pathlen,
const char *base, int baselen,
const char *pattern, int prefix, int patternlen,
- int flags)
+ unsigned flags)
{
const char *name;
int namelen;
diff --git a/dir.h b/dir.h
index 3ec3fb0..e942b50 100644
--- a/dir.h
+++ b/dir.h
@@ -28,7 +28,7 @@ struct exclude {
int nowildcardlen;
const char *base;
int baselen;
- int flags;
+ unsigned flags; /* EXC_FLAG_* */
/*
* Counting starts from 1 for line numbers in ignore files,
@@ -229,10 +229,10 @@ struct dir_entry *dir_add_ignored(struct dir_struct *dir,
const char *pathname,
* attr.c:path_matches()
*/
extern int match_basename(const char *, int,
- const char *, int, int, int);
+ const char *, int, int, unsigned);
extern int match_pathname(const char *, int,
const char *, int,
- const char *, int, int, int);
+ const char *, int, int, unsigned);
extern struct exclude *last_exclude_matching(struct dir_struct *dir,
const char *name, int *dtype);
@@ -244,7 +244,7 @@ extern struct exclude_list *add_exclude_list(struct
dir_struct *dir,
extern int add_excludes_from_file_to_list(const char *fname, const char *base,
int baselen,
struct exclude_list *el, int
check_index);
extern void add_excludes_from_file(struct dir_struct *, const char *fname);
-extern void parse_exclude_pattern(const char **string, int *patternlen, int
*flags, int *nowildcardlen);
+extern void parse_exclude_pattern(const char **string, int *patternlen,
unsigned *flags, int *nowildcardlen);
extern void add_exclude(const char *string, const char *base,
int baselen, struct exclude_list *el, int srcpos);
extern void clear_exclude_list(struct exclude_list *el);
--
2.7.1.339.g0233b80
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.ke
Re: git-rebase + git-mergetool results in broken state
On Tue, Feb 23, 2016 at 04:44:49PM -0600, Joe Einertson wrote: > I'm experiencing an annoying issue which leaves the repository in a > weird, broken state. I am attempting a rather vanilla rebase, rebasing > the commits from a feature branch on top of the newest commits on > master. Can you tell us a little more about what's in the branch being rebased? Is it perhaps a public project that you can share so that we can reproduce the issue? Here are a few more questions that can help narrow down the issue: * What Git vesion are you using? * What mergetool are you using? - See the output "git config merge.tool" * What platform are you on? Are you on Windows? * Does the conflicting commit contain renames? I'm trying to figure out whether we are missing a `mkdir -p` somewhere, and whether we hadn't run into this in the past because the merge needs to involve renames. > So, I run a typical series of commands: > 1. git checkout feature-branch > 2. git rebase master (conflicts ensue) > 3. git mergetool > > The conflicts are expected, but when using mergetool to resolve them, > I encounter many "no such file or directory" errors. > > mv: cannot stat > ‘app/components/mediaManager/kbImageEditor.directive.coffee’: No such > file or directory > cp: cannot stat > ‘./app/components/mediaManager/kbImageEditor.directive_BACKUP_13615.coffee’: > No such file or directory > mv: cannot move ‘.merge_file_ogGjXX’ to > ‘./app/components/mediaManager/kbImageEditor.directive_BASE_13615.coffee’: > No such file or directory > /usr/lib/git-core/git-mergetool: 229: /usr/lib/git-core/git-mergetool: > cannot create > ./app/components/mediaManager/kbImageEditor.directive_LOCAL_13615.coffee: > Directory nonexistent > /usr/lib/git-core/git-mergetool: 229: /usr/lib/git-core/git-mergetool: > cannot create > ./app/components/mediaManager/kbImageEditor.directive_REMOTE_13615.coffee: > Directory nonexistent * Does the directory ./app/components/medaiManager/ exist in master? * Did a commit on master perhaps move its content somewhere else? * Does that directory have some chmod permissions, or is it owned by a different user? * Are you able to create new files in that directory? > This leaves weird dangling files like '.merge_file_ogGjXX' in the > repo, and I assume I should not proceed with the merge since it > couldn't even create the files to compare. If you got a failure at this step you can safely delete those temporary dangling files and then follow the advice given by `git status`. Typically it'll list files with conflicts. Open them with your $EDITOR, resolve conflicts like normal, and add the result using `git add`. Nonetheless, we'd like to get to the bottom of this issue. > Is this a known issue? Is there any workaround? Is it safe to proceed > with the merge? I've never ran into this myself, and it's never been reported here so this is not a known issue. It's still safe to proceed with the merge and resolve files the normal way. If you would rather undo the rebase and go back to your original state (before the rebase) then you can do `git rebase --abort` anytime. I'm not sure about a workaround, but.. it might possibly work if you were to `mkdir -p` the directory mentioned above, but that's a guess. If that does workaround the issue then please let us know since that would be an interesting data point. -- David -- 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] builtin/receive-pack.c: use parse_options API
This patch makes receive-pack use the parse_options API,
bringing it more in line with send-pack and push.
Helped-by: Matthieu Moy
Signed-off-by: Sidhant Sharma [:tk]
---
builtin/receive-pack.c | 55 ++
1 file changed, 24 insertions(+), 31 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c8e32b2..fe9a594 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -21,7 +21,10 @@
#include "sigchain.h"
#include "fsck.h"
-static const char receive_pack_usage[] = "git receive-pack ";
+static const char * const receive_pack_usage[] = {
+ N_("git receive-pack "),
+ NULL
+};
enum deny_action {
DENY_UNCONFIGURED,
@@ -45,12 +48,12 @@ static int unpack_limit = 100;
static int report_status;
static int use_sideband;
static int use_atomic;
-static int quiet;
+static int quiet = 0;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
static int auto_gc = 1;
static int fix_thin = 1;
-static int stateless_rpc;
+static int stateless_rpc = 0;
static const char *service_dir;
static const char *head_name;
static void *head_name_to_free;
@@ -1707,45 +1710,35 @@ static int delete_only(struct command *commands)
int cmd_receive_pack(int argc, const char **argv, const char *prefix)
{
int advertise_refs = 0;
- int i;
struct command *commands;
struct sha1_array shallow = SHA1_ARRAY_INIT;
struct sha1_array ref = SHA1_ARRAY_INIT;
struct shallow_info si;
+ struct option options[] = {
+ OPT__QUIET(&quiet, N_("quiet")),
+ OPT_HIDDEN_BOOL(0, "stateless-rpc", &stateless_rpc, NULL),
+ OPT_HIDDEN_BOOL(0, "advertise-refs", &advertise_refs, NULL),
+ /* Hidden OPT_BOOL option */
+ {
+ OPTION_SET_INT, 0, "reject-thin-pack-for-testing",
&fix_thin, NULL,
+ NULL, PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0,
+ },
+ OPT_END()
+ };
packet_trace_identity("receive-pack");
- argv++;
- for (i = 1; i < argc; i++) {
- const char *arg = *argv++;
+ argc = parse_options(argc, argv, prefix, options, receive_pack_usage,
0);
- if (*arg == '-') {
- if (!strcmp(arg, "--quiet")) {
- quiet = 1;
- continue;
- }
+ if (argc > 1)
+ usage_msg_opt(_("Too many arguments."), receive_pack_usage,
options);
+ if (argc == 0)
+ usage_msg_opt(_("You must specify a directory."),
receive_pack_usage, options);
- if (!strcmp(arg, "--advertise-refs")) {
- advertise_refs = 1;
- continue;
- }
- if (!strcmp(arg, "--stateless-rpc")) {
- stateless_rpc = 1;
- continue;
- }
- if (!strcmp(arg, "--reject-thin-pack-for-testing")) {
- fix_thin = 0;
- continue;
- }
+ service_dir = argv[0];
- usage(receive_pack_usage);
- }
- if (service_dir)
- usage(receive_pack_usage);
- service_dir = arg;
- }
if (!service_dir)
- usage(receive_pack_usage);
+ usage_with_options(receive_pack_usage, options);
setup_path();
--
2.6.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: Trouble Cloning Git remote repository
On Tue, Mar 01, 2016 at 10:04:49AM -0500, Fred's Personal wrote:
> Thanks for the insight, it's been a while since I debugged a Windows call
> stack. Can you give me commands to view what gets passed to
> CreateProcessW().
Sorry, my Windows knowledge is several years old. Maybe procmon[1] will
show them?
[1] https://technet.microsoft.com/en-us/sysinternals/processmonitor.aspx
> -Original Message-
> From: John Keeping [mailto:j...@keeping.me.uk]
> Sent: Monday, February 29, 2016 4:35 AM
> To: Duy Nguyen
> Cc: Fred's Personal; Git Mailing List
> Subject: Re: Trouble Cloning Git remote repository
>
> On Mon, Feb 29, 2016 at 08:20:46AM +0700, Duy Nguyen wrote:
> > On Mon, Feb 29, 2016 at 12:48 AM, Fred's Personal
> > wrote:
> > > Duy,
> > >
> > > Thanks for advice, here is the result of executing the lines you
> suggested:
> > >
> > > user1@Host1 MINGW64 ~/gitrepository (master) $ export GIT_TRACE=1
> > >
> > > user1@Host1 MINGW64 ~/gitrepository (master) $ git clone -v
> > > ssh://user1@Host2/srv/centralrepo
> > > 12:33:47.928365 git.c:348 trace: built-in: git 'clone'
> '-v' 'ssh://user1@Host2/srv/centralrepo'
> > > Cloning into 'centralrepo'...
> > > 12:33:48.022110 run-command.c:343 trace: run_command: 'C:\Program
> Files (x86)\PuTTY\plink.exe' 'user1@Host2' 'git-upload-pack
> '\''/srv/centralrepo'\'''
> >
> > This command looks good to me. So I have no clue what goes wrong :-)
> > Maybe you can execute this command directly, with more plink debugging
> > options or something. You don't have to run git-upload-pack. Just
> > spawn a shell. Another option is try something other than plink (does
> > git-for-windows come with ssh.exe?)
>
> On Windows it's probably going through mingw_spawnve_fd() which reassembles
> a quoted command line from the individual arguments. It would be
> interesting to see what gets passed to CreateProcessW().
>
> > > ##>>>Lines from $HOME/.bashrc (See below, removed here for clarity)
> > >
> > > + user1@Host2 git-upload-pack /srv/centralrepo
> > > bash: user1@Host2: command not found
> > > fatal: Could not read from remote repository.
> > >
> > > Please make sure you have the correct access rights and the
> > > repository exists.
> > >
> > >
> > > Regards,
> > > Fred
> > >
> > > freddie...@optonline.net
> > >
> > >
> > > -Original Message-
> > > From: Duy Nguyen [mailto:pclo...@gmail.com]
> > > Sent: Saturday, February 27, 2016 4:36 AM
> > > To: Fred's Personal
> > > Cc: Git Mailing List
> > > Subject: Re: Trouble Cloning Git remote repository
> > >
> > > On Sat, Feb 27, 2016 at 6:03 AM, Fred's Personal
> wrote:
> > >> $ git clone -v ssh://user1@Host2/srv/centralrepo Cloning into
> > >> 'centralrepo'...
> > >Lines from $HOME/.bashrc
> > >> + export
> > >> PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/
> > >> usr
> > >> /games
> > >> :/usr/local/games
> > >> +
> > >> PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/
> > >> usr
> > >> /games
> > >> :/usr/local/games
> > >> + PROMPT_COMMAND=
> > >> + CDPATH=
> > >> + '[' '' = yes ']'
> > >> + PS1='${debian_chroot:+($debian_chroot)}\u:\W\$ '
> > >> + export GIT_TRACE_PACKET=1
> > >> + GIT_TRACE_PACKET=1
> > >> + export GIT_TRACE=1
> > >> + GIT_TRACE=1
> > >End of Lines from $HOME/.bashrc
> > >> ## WHERE DOES The following line COME FROMWhat Script spits out
> > >> this line
> > >> + user1@Host2 git-upload-pack /srv/centralrepo
> > >
> > > Try set GIT_TRACE=1 at the clone line, I have a feeling that this line
> should be "ssh user@Host2..." but "ssh" is missing.
> > >
> > > $ export GIT_TRACE=1
> > > $ git clone -v ssh://user1@Host2/srv/centralrepo
> > > --
> > > Duy
> > >
> > >
> > > -
> > > No virus found in this message.
> > > Checked by AVG - www.avg.com
> > > Version: 2016.0.7442 / Virus Database: 4537/11702 - Release Date:
> > > 02/26/16
> > >
> > >
> >
> >
> >
> > --
> > 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
>
>
> -
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2016.0.7442 / Virus Database: 4537/11716 - Release Date: 02/28/16
>
>
> --
> 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: Trouble Cloning Git remote repository
John,
Thanks for the insight, it's been a while since I debugged a Windows call
stack. Can you give me commands to view what gets passed to
CreateProcessW().
Regards,
Fred
freddie...@optonline.net
-Original Message-
From: John Keeping [mailto:j...@keeping.me.uk]
Sent: Monday, February 29, 2016 4:35 AM
To: Duy Nguyen
Cc: Fred's Personal; Git Mailing List
Subject: Re: Trouble Cloning Git remote repository
On Mon, Feb 29, 2016 at 08:20:46AM +0700, Duy Nguyen wrote:
> On Mon, Feb 29, 2016 at 12:48 AM, Fred's Personal
> wrote:
> > Duy,
> >
> > Thanks for advice, here is the result of executing the lines you
suggested:
> >
> > user1@Host1 MINGW64 ~/gitrepository (master) $ export GIT_TRACE=1
> >
> > user1@Host1 MINGW64 ~/gitrepository (master) $ git clone -v
> > ssh://user1@Host2/srv/centralrepo
> > 12:33:47.928365 git.c:348 trace: built-in: git 'clone'
'-v' 'ssh://user1@Host2/srv/centralrepo'
> > Cloning into 'centralrepo'...
> > 12:33:48.022110 run-command.c:343 trace: run_command: 'C:\Program
Files (x86)\PuTTY\plink.exe' 'user1@Host2' 'git-upload-pack
'\''/srv/centralrepo'\'''
>
> This command looks good to me. So I have no clue what goes wrong :-)
> Maybe you can execute this command directly, with more plink debugging
> options or something. You don't have to run git-upload-pack. Just
> spawn a shell. Another option is try something other than plink (does
> git-for-windows come with ssh.exe?)
On Windows it's probably going through mingw_spawnve_fd() which reassembles
a quoted command line from the individual arguments. It would be
interesting to see what gets passed to CreateProcessW().
> > ##>>>Lines from $HOME/.bashrc (See below, removed here for clarity)
> >
> > + user1@Host2 git-upload-pack /srv/centralrepo
> > bash: user1@Host2: command not found
> > fatal: Could not read from remote repository.
> >
> > Please make sure you have the correct access rights and the
> > repository exists.
> >
> >
> > Regards,
> > Fred
> >
> > freddie...@optonline.net
> >
> >
> > -Original Message-
> > From: Duy Nguyen [mailto:pclo...@gmail.com]
> > Sent: Saturday, February 27, 2016 4:36 AM
> > To: Fred's Personal
> > Cc: Git Mailing List
> > Subject: Re: Trouble Cloning Git remote repository
> >
> > On Sat, Feb 27, 2016 at 6:03 AM, Fred's Personal
wrote:
> >> $ git clone -v ssh://user1@Host2/srv/centralrepo Cloning into
> >> 'centralrepo'...
> >Lines from $HOME/.bashrc
> >> + export
> >> PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/
> >> usr
> >> /games
> >> :/usr/local/games
> >> +
> >> PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/
> >> usr
> >> /games
> >> :/usr/local/games
> >> + PROMPT_COMMAND=
> >> + CDPATH=
> >> + '[' '' = yes ']'
> >> + PS1='${debian_chroot:+($debian_chroot)}\u:\W\$ '
> >> + export GIT_TRACE_PACKET=1
> >> + GIT_TRACE_PACKET=1
> >> + export GIT_TRACE=1
> >> + GIT_TRACE=1
> >End of Lines from $HOME/.bashrc
> >> ## WHERE DOES The following line COME FROMWhat Script spits out
> >> this line
> >> + user1@Host2 git-upload-pack /srv/centralrepo
> >
> > Try set GIT_TRACE=1 at the clone line, I have a feeling that this line
should be "ssh user@Host2..." but "ssh" is missing.
> >
> > $ export GIT_TRACE=1
> > $ git clone -v ssh://user1@Host2/srv/centralrepo
> > --
> > Duy
> >
> >
> > -
> > No virus found in this message.
> > Checked by AVG - www.avg.com
> > Version: 2016.0.7442 / Virus Database: 4537/11702 - Release Date:
> > 02/26/16
> >
> >
>
>
>
> --
> 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
-
No virus found in this message.
Checked by AVG - www.avg.com
Version: 2016.0.7442 / Virus Database: 4537/11716 - Release Date: 02/28/16
--
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] environment.c: introduce DECLARE_GIT_GETTER helper macro
On Sun, Feb 28, 2016 at 01:35:44AM +0600, Alexander Kuleshov wrote: > +DECLARE_GIT_GETTER(const char *, get_git_dir, git_dir) > +DECLARE_GIT_GETTER(const char *, get_git_namespace, namespace) > +DECLARE_GIT_GETTER(char *, get_object_directory, git_object_dir) > +DECLARE_GIT_GETTER(char *, get_index_file, git_index_file) > +DECLARE_GIT_GETTER(char *, get_graft_file, git_graft_file) Hmm. I'm somewhat lukewarm on this patch. It's fewer lines and less duplication, which is nice, but this kind of code generation often makes things annoying (to step into with the debugger, to find with ctags, etc). I dunno. -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 11/22] ident.c: mark strings for translation
On Mon, Feb 29, 2016 at 10:34:59AM -0800, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > > > Signed-off-by: Nguyễn Thái Ngọc Duy > > --- > > All (or at least most of) these look old ones; even the ones blamed > to 59f92959 (fmt_ident: refactor strictness checks, 2016-02-04) had > original in the same file without _(). > > I'm inclined to say we should do the whole thing post 2.8.0 release > for this file. I guess I'm cc'd as the author of some of these. These all look fine to me (and the other one for credential-*). I'm happy to have them down now or post-2.8.0 (it is really not any work for me, but for the translators). -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: git config --get-urlmatch does not set exit code 1 when no match is found
On Mon, Feb 29, 2016 at 06:38:28PM +0530, Guilherme wrote: > @Peff Thank you for the heads up. > > I'm trying to find out if there are any credential helpers configured > in the system that will be running tests. On the dedicated test > machines that is not a problem but the developer machines are. Do you need to do it in an automated way, or just once? If manual is OK, I suspect running: GIT_TRACE=1 git credential Should I already post a pre-emptive email asking about the corner cases? > > More importantly for me is if there is a case where get-url would not > show a match where git clone would. If git clone skips a configuration > that config url-match doesn't then it's not so bad. Sorry, I don't recall the details. I feel like we discussed it a little on the list, but I can't find it now. The closest I could find is: http://article.gmane.org/gmane.comp.version-control.git/267895 I think the main differences would be in ordering, not in what is selected. -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 v7 29/33] setup: configure ref storage on setup
On Tue, Mar 01, 2016 at 03:48:30AM -0500, Jeff King wrote:
> On Mon, Feb 29, 2016 at 07:53:02PM -0500, David Turner wrote:
>
> > diff --git a/setup.c b/setup.c
> > index bd3a2cf..e2e1220 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -457,6 +457,10 @@ static int check_repository_format_gently(const char
> > *gitdir, int *nongit_ok)
> > ret = -1;
> > }
> >
> > + register_ref_storage_backends();
> > + if (set_ref_storage_backend(ref_storage_backend))
> > + die(_("Unknown ref storage backend %s"), ref_storage_backend);
> > +
> > strbuf_release(&sb);
> > return ret;
> > }
>
> Much nicer than the one it replaces, I think.
>
> This whole block should probably go inside
>
> if (ret == 0) {
> ...
> }
>
> If we are doing setup_git_repository_gently() and we do _not_ find a
> valid repository, we would not want to enable the ref storage.
So in the new world order of the patch series I just posted, this would
probably look like:
1. Add a ref_backend string to "struct repository_format", and parse it
in the callback.
2. The bottom of check_repository_format_gently() is only reached when
we have a workable repo. So from there, you can:
set_ref_storage_backend(candidate.ref_backend);
I think you'd probably want to check nongit_ok before dying on
failure (even without my patches).
3. Elsewhere, you can use read_repository_format() to get the backend
speculatively (e.g., for submodules), rather than doing a custom
git_config_from_file invocation.
None of which is to say that building on my series is a foregone
conclusion; I just wanted to point you in the right direction if you do
want to.
-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 09/10] setup: drop repository_format_version global
Nobody reads this anymore, and they're not likely to; the
interesting thing is whether or not we passed
check_repository_format(), and possibly the individual
"extension" variables.
Signed-off-by: Jeff King
---
cache.h | 1 -
environment.c | 1 -
setup.c | 1 -
3 files changed, 3 deletions(-)
diff --git a/cache.h b/cache.h
index 1795807..ecefa00 100644
--- a/cache.h
+++ b/cache.h
@@ -747,7 +747,6 @@ extern int grafts_replace_parents;
*/
#define GIT_REPO_VERSION 0
#define GIT_REPO_VERSION_READ 1
-extern int repository_format_version;
extern int repository_format_precious_objects;
struct repository_format {
diff --git a/environment.c b/environment.c
index 8b8c8e8..d9e3861 100644
--- a/environment.c
+++ b/environment.c
@@ -25,7 +25,6 @@ int log_all_ref_updates = -1; /* unspecified */
int warn_ambiguous_refs = 1;
int warn_on_object_refname_ambiguity = 1;
int ref_paranoia = -1;
-int repository_format_version;
int repository_format_precious_objects;
const char *git_commit_encoding;
const char *git_log_output_encoding;
diff --git a/setup.c b/setup.c
index a04d7dd..f52011e 100644
--- a/setup.c
+++ b/setup.c
@@ -428,7 +428,6 @@ static int check_repository_format_gently(const char
*gitdir, int *nongit_ok)
die("%s", err.buf);
}
- repository_format_version = candidate.version;
repository_format_precious_objects = candidate.precious_objects;
string_list_clear(&candidate.unknown_extensions, 0);
if (!has_common) {
--
2.8.0.rc0.278.gfeb5644
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Mark win32's pthread_exit() as NORETURN
Am 01.03.2016 um 15:13 schrieb Johannes Schindelin:
> The pthread_exit() function is not expected to return. Ever. On Windows,
> we call ExitThread() whose documentation claims: "This function does not
> return a value.":
Does this really mean that ExitThread() does not return ?
Just wondering...
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659
>
> Pointed out by Jeff King.
>
> Signed-off-by: Johannes Schindelin
> ---
>
> Relative to v1, only the commit message changed (to clarify that
> ExitThread() indeed never returns).
>
> compat/win32/pthread.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> index 20b35a2..148db60 100644
> --- a/compat/win32/pthread.h
> +++ b/compat/win32/pthread.h
> @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void
> **value_ptr);
> #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
> extern pthread_t pthread_self(void);
>
> -static inline int pthread_exit(void *ret)
> +static inline int NORETURN pthread_exit(void *ret)
> {
> ExitThread((DWORD)(intptr_t)ret);
> }
> --
Stefan
--
/dev/random says: We're lost, but we're making good time.
python -c "print
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
[PATCH 10/10] setup: drop GIT_REPO_VERSION constants
As each constant is used in only one place, they are not
helping us avoid duplication. And they may be actively
misleading, as a version check is now much more complicated
than a simple integer comparison. The logic is in
verify_repository_format, and if you are thinking about
bumping the version, you _should_ have to go look at that
function.
Signed-off-by: Jeff King
---
builtin/init-db.c | 5 +
cache.h | 7 ---
setup.c | 6 +++---
3 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index d9934f3..ee2156e 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -176,7 +176,6 @@ static int create_default_files(const char *template_path)
struct stat st1;
struct strbuf buf = STRBUF_INIT;
char *path;
- char repo_version_string[10];
char junk[2];
int reinit;
int filemode;
@@ -228,9 +227,7 @@ static int create_default_files(const char *template_path)
}
/* This forces creation of new config file */
- xsnprintf(repo_version_string, sizeof(repo_version_string),
- "%d", GIT_REPO_VERSION);
- git_config_set("core.repositoryformatversion", repo_version_string);
+ git_config_set("core.repositoryformatversion", "0");
/* Check filemode trustability */
path = git_path_buf(&buf, "config");
diff --git a/cache.h b/cache.h
index ecefa00..d9c23d5 100644
--- a/cache.h
+++ b/cache.h
@@ -740,13 +740,6 @@ extern char *notes_ref_name;
extern int grafts_replace_parents;
-/*
- * GIT_REPO_VERSION is the version we write by default. The
- * _READ variant is the highest number we know how to
- * handle.
- */
-#define GIT_REPO_VERSION 0
-#define GIT_REPO_VERSION_READ 1
extern int repository_format_precious_objects;
struct repository_format {
diff --git a/setup.c b/setup.c
index f52011e..75d5939 100644
--- a/setup.c
+++ b/setup.c
@@ -460,9 +460,9 @@ void read_repository_format(struct repository_format
*format, const char *path)
int verify_repository_format(const struct repository_format *format,
struct strbuf *err)
{
- if (GIT_REPO_VERSION_READ < format->version) {
- strbuf_addf(err, "Expected git repo version <= %d, found %d",
- GIT_REPO_VERSION_READ, format->version);
+ if (format->version > 1) {
+ strbuf_addf(err, "Expected git repo version <= 1, found %d",
+ format->version);
return -1;
}
--
2.8.0.rc0.278.gfeb5644
--
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 08/10] setup: unify repository version callbacks
Once upon a time, check_repository_format_gently would parse
the config with a single callback, and that callback would
set up a bunch of global variables. But now that we have
separate workdirs, we have to be more careful. Commit
31e26eb (setup.c: support multi-checkout repo setup,
2014-11-30) introduced a reduced callback which omits some
values like core.worktree. In the "main" callback we call
the reduced one, and then add back in the missing variables.
Now that we have split the config-parsing from the munging
of the global variables, we can handle all the config with a
single callback, and keep all of the "are we in a separate
workdir" logic together in the caller.
Signed-off-by: Jeff King
---
cache.h | 1 -
setup.c | 69 ++---
2 files changed, 23 insertions(+), 47 deletions(-)
diff --git a/cache.h b/cache.h
index d03f5d6..1795807 100644
--- a/cache.h
+++ b/cache.h
@@ -1571,7 +1571,6 @@ extern void git_config_set_multivar_in_file(const char *,
const char *, const ch
extern int git_config_rename_section(const char *, const char *);
extern int git_config_rename_section_in_file(const char *, const char *, const
char *);
extern const char *git_etc_gitconfig(void);
-extern int check_repository_format_version(const char *var, const char *value,
void *cb);
extern int git_env_bool(const char *, int);
extern unsigned long git_env_ulong(const char *, unsigned long);
extern int git_config_system(void);
diff --git a/setup.c b/setup.c
index 3d498af..a04d7dd 100644
--- a/setup.c
+++ b/setup.c
@@ -388,27 +388,26 @@ static int check_repo_format(const char *var, const char
*value, void *vdata)
data->precious_objects = git_config_bool(var, value);
else
string_list_append(&data->unknown_extensions, ext);
+ } else if (strcmp(var, "core.bare") == 0) {
+ data->is_bare = git_config_bool(var, value);
+ } else if (strcmp(var, "core.worktree") == 0) {
+ if (!value)
+ return config_error_nonbool(var);
+ data->work_tree = xstrdup(value);
}
return 0;
}
-static void read_repository_format_1(struct repository_format *, config_fn_t,
-const char *);
-
static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
{
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
struct repository_format candidate;
- config_fn_t fn;
-
- if (get_common_dir(&sb, gitdir))
- fn = check_repo_format;
- else
- fn = check_repository_format_version;
+ int has_common;
+ has_common = get_common_dir(&sb, gitdir);
strbuf_addstr(&sb, "/config");
- read_repository_format_1(&candidate, fn, sb.buf);
+ read_repository_format(&candidate, sb.buf);
strbuf_release(&sb);
/*
@@ -432,37 +431,31 @@ static int check_repository_format_gently(const char
*gitdir, int *nongit_ok)
repository_format_version = candidate.version;
repository_format_precious_objects = candidate.precious_objects;
string_list_clear(&candidate.unknown_extensions, 0);
- if (candidate.is_bare != -1) {
- is_bare_repository_cfg = candidate.is_bare;
- if (is_bare_repository_cfg == 1)
+ if (!has_common) {
+ if (candidate.is_bare != -1) {
+ is_bare_repository_cfg = candidate.is_bare;
+ if (is_bare_repository_cfg == 1)
+ inside_work_tree = -1;
+ }
+ if (candidate.work_tree) {
+ free(git_work_tree_cfg);
+ git_work_tree_cfg = candidate.work_tree;
inside_work_tree = -1;
- }
- if (candidate.work_tree) {
- free(git_work_tree_cfg);
- git_work_tree_cfg = candidate.work_tree;
- inside_work_tree = -1;
+ }
+ } else {
+ free(candidate.work_tree);
}
return 0;
}
-/*
- * Internally, we need to swap out "fn" here, but we don't want to expose
- * that to the world. Hence a wrapper around this internal function.
- */
-static void read_repository_format_1(struct repository_format *format,
-config_fn_t fn, const char *path)
+void read_repository_format(struct repository_format *format, const char *path)
{
memset(format, 0, sizeof(*format));
format->version = -1;
format->is_bare = -1;
string_list_init(&format->unknown_extensions, 1);
- git_config_from_file(fn, path, format);
-}
-
-void read_repository_format(struct repository_format *format, const char *path)
-{
- read_repository_format_1(format, check_repository_format_version, path);
+ git_config_from_file(check_repo_format, p
[PATCH 07/10] init: use setup.c's repo version verification
We check our templates to make sure they are from a
version of git we understand (otherwise we would init a
repository we cannot ourselves run in!). But our simple
integer check has fallen behind the times. Let's use the
helpers that setup.c provides to do it right.
Signed-off-by: Jeff King
---
I actually wonder if this code has been triggered ever, in the history
of git. We do not ship a "config" file in our templates, and I doubt
that anybody would ship one that sets core.repositoryformatversion.
builtin/init-db.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index e9b2256..d9934f3 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -95,6 +95,8 @@ static void copy_templates(const char *template_dir)
struct strbuf path = STRBUF_INIT;
struct strbuf template_path = STRBUF_INIT;
size_t template_len;
+ struct repository_format template_format;
+ struct strbuf err = STRBUF_INIT;
DIR *dir;
char *to_free = NULL;
@@ -121,17 +123,18 @@ static void copy_templates(const char *template_dir)
/* Make sure that template is from the correct vintage */
strbuf_addstr(&template_path, "config");
- repository_format_version = 0;
- git_config_from_file(check_repository_format_version,
-template_path.buf, NULL);
+ read_repository_format(&template_format, template_path.buf);
strbuf_setlen(&template_path, template_len);
- if (repository_format_version &&
- repository_format_version != GIT_REPO_VERSION) {
- warning(_("not copying templates of "
- "a wrong format version %d from '%s'"),
- repository_format_version,
- template_dir);
+ /*
+* No mention of version at all is OK, but anything else should be
+* verified.
+*/
+ if (template_format.version >= 0 &&
+ verify_repository_format(&template_format, &err) < 0) {
+ warning(_("not copying templates from '%s': %s"),
+ template_dir, err.buf);
+ strbuf_release(&err);
goto close_free_return;
}
--
2.8.0.rc0.278.gfeb5644
--
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 06/10] setup: refactor repo format reading and verification
When we want to know if we're in a git repository of
reasonable vintage, we can call check_repository_format_gently(),
which does three things:
1. Reads the config from the .git/config file.
2. Verifies that the version info we read is sane.
3. Writes some global variables based on this.
There are a few things we could improve here.
One is that steps 1 and 3 happen together. So if the
verification in step 2 fails, we still clobber the global
variables. This is especially bad if we go on to try another
repository directory; we may end up with a state of mixed
config variables.
The second is there's no way to ask about the repository
version for anything besides the main repository we're in.
git-init wants to do this, and it's possible that we would
want to start doing so for submodules (e.g., to find out
which ref backend they're using).
We can improve both by splitting the first two steps into
separate functions. Now check_repository_format_gently()
calls out to steps 1 and 2, and does 3 only if step 2
succeeds.
Signed-off-by: Jeff King
---
cache.h | 23
setup.c | 121 +++-
2 files changed, 104 insertions(+), 40 deletions(-)
diff --git a/cache.h b/cache.h
index 1825851..d03f5d6 100644
--- a/cache.h
+++ b/cache.h
@@ -750,6 +750,29 @@ extern int grafts_replace_parents;
extern int repository_format_version;
extern int repository_format_precious_objects;
+struct repository_format {
+ int version;
+ int precious_objects;
+ int is_bare;
+ char *work_tree;
+ struct string_list unknown_extensions;
+};
+
+/*
+ * Read the repository format characteristics from the config file "path". If
+ * the version cannot be extracted from the file for any reason, the version
+ * field will be set to -1, and all other fields are undefined.
+ */
+void read_repository_format(struct repository_format *format, const char
*path);
+
+/*
+ * Verify that the repository described by repository_format is something we
+ * can read. If it is, return 0. Otherwise, return -1, and "err" will describe
+ * any errors encountered.
+ */
+int verify_repository_format(const struct repository_format *format,
+struct strbuf *err);
+
/*
* Check the repository format version in the path found in get_git_dir(),
* and die if it is a version we don't understand. Generally one would
diff --git a/setup.c b/setup.c
index a6013e6..3d498af 100644
--- a/setup.c
+++ b/setup.c
@@ -5,7 +5,6 @@
static int inside_git_dir = -1;
static int inside_work_tree = -1;
static int work_tree_config_is_bogus;
-static struct string_list unknown_extensions = STRING_LIST_INIT_DUP;
/*
* The input parameter must contain an absolute path, and it must already be
@@ -370,12 +369,13 @@ void setup_work_tree(void)
initialized = 1;
}
-static int check_repo_format(const char *var, const char *value, void *cb)
+static int check_repo_format(const char *var, const char *value, void *vdata)
{
+ struct repository_format *data = vdata;
const char *ext;
if (strcmp(var, "core.repositoryformatversion") == 0)
- repository_format_version = git_config_int(var, value);
+ data->version = git_config_int(var, value);
else if (skip_prefix(var, "extensions.", &ext)) {
/*
* record any known extensions here; otherwise,
@@ -385,61 +385,105 @@ static int check_repo_format(const char *var, const char
*value, void *cb)
if (!strcmp(ext, "noop"))
;
else if (!strcmp(ext, "preciousobjects"))
- repository_format_precious_objects =
git_config_bool(var, value);
+ data->precious_objects = git_config_bool(var, value);
else
- string_list_append(&unknown_extensions, ext);
+ string_list_append(&data->unknown_extensions, ext);
}
return 0;
}
+static void read_repository_format_1(struct repository_format *, config_fn_t,
+const char *);
+
static int check_repository_format_gently(const char *gitdir, int *nongit_ok)
{
struct strbuf sb = STRBUF_INIT;
- const char *repo_config;
+ struct strbuf err = STRBUF_INIT;
+ struct repository_format candidate;
config_fn_t fn;
- int ret = 0;
-
- string_list_clear(&unknown_extensions, 0);
if (get_common_dir(&sb, gitdir))
fn = check_repo_format;
else
fn = check_repository_format_version;
+
strbuf_addstr(&sb, "/config");
- repo_config = sb.buf;
+ read_repository_format_1(&candidate, fn, sb.buf);
+ strbuf_release(&sb);
/*
-* Ignore return value; for historical reasons, we must treat a missing
-* config file as a noop (git-init relies on this).
+* For historical use
[PATCH 04/10] check_repository_format_gently: stop using git_config_early
There's a chicken-and-egg problem with using the regular
git_config during the repository setup process. We get
around it here by using a special interface that lets us
specify the per-repo config, and avoid calling
git_pathdup().
But this interface doesn't actually make sense. It will look
in the system and per-user config, too; we definitely would
not want to accept a core.repositoryformatversion from
there.
The git_config_from_file interface is a better match, as it
lets us look at a single file.
Signed-off-by: Jeff King
---
This has literally been bugging me for 8 years.
setup.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/setup.c b/setup.c
index a02932b..a6013e6 100644
--- a/setup.c
+++ b/setup.c
@@ -409,15 +409,10 @@ static int check_repository_format_gently(const char
*gitdir, int *nongit_ok)
repo_config = sb.buf;
/*
-* git_config() can't be used here because it calls git_pathdup()
-* to get $GIT_CONFIG/config. That call will make setup_git_env()
-* set git_dir to ".git".
-*
-* We are in gitdir setup, no git dir has been found useable yet.
-* Use a gentler version of git_config() to check if this repo
-* is a good one.
+* Ignore return value; for historical reasons, we must treat a missing
+* config file as a noop (git-init relies on this).
*/
- git_config_early(fn, NULL, repo_config);
+ git_config_from_file(fn, repo_config, NULL);
if (GIT_REPO_VERSION_READ < repository_format_version) {
if (!nongit_ok)
die ("Expected git repo version <= %d, found %d",
--
2.8.0.rc0.278.gfeb5644
--
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 05/10] config: drop git_config_early
There are no more callers, and it's a rather confusing
interface. This could just be folded into
git_config_with_options(), but for the sake of readability,
we'll leave it as a separate (static) helper function.
Signed-off-by: Jeff King
---
Documentation/technical/api-config.txt | 7 ---
cache.h| 1 -
config.c | 12
3 files changed, 4 insertions(+), 16 deletions(-)
diff --git a/Documentation/technical/api-config.txt
b/Documentation/technical/api-config.txt
index 0d8b99b..20741f3 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -63,13 +63,6 @@ parse for configuration, rather than looking in the usual
files. Regular
Specify whether include directives should be followed in parsed files.
Regular `git_config` defaults to `1`.
-There is a special version of `git_config` called `git_config_early`.
-This version takes an additional parameter to specify the repository
-config, instead of having it looked up via `git_path`. This is useful
-early in a Git program before the repository has been found. Unless
-you're working with early setup code, you probably don't want to use
-this.
-
Reading Specific Files
--
diff --git a/cache.h b/cache.h
index 43c6a44..1825851 100644
--- a/cache.h
+++ b/cache.h
@@ -1525,7 +1525,6 @@ extern void git_config(config_fn_t fn, void *);
extern int git_config_with_options(config_fn_t fn, void *,
struct git_config_source *config_source,
int respect_includes);
-extern int git_config_early(config_fn_t fn, void *, const char *repo_config);
extern int git_parse_ulong(const char *, unsigned long *);
extern int git_parse_maybe_bool(const char *);
extern int git_config_int(const char *, const char *);
diff --git a/config.c b/config.c
index 9ba40bc..7ddb287 100644
--- a/config.c
+++ b/config.c
@@ -1188,11 +1188,12 @@ int git_config_system(void)
return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
}
-int git_config_early(config_fn_t fn, void *data, const char *repo_config)
+static int do_git_config_sequence(config_fn_t fn, void *data)
{
int ret = 0, found = 0;
char *xdg_config = xdg_config_home("config");
char *user_config = expand_user_path("~/.gitconfig");
+ char *repo_config = git_pathdup("config");
if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK,
0)) {
ret += git_config_from_file(fn, git_etc_gitconfig(),
@@ -1228,6 +1229,7 @@ int git_config_early(config_fn_t fn, void *data, const
char *repo_config)
free(xdg_config);
free(user_config);
+ free(repo_config);
return ret == 0 ? found : ret;
}
@@ -1235,8 +1237,6 @@ int git_config_with_options(config_fn_t fn, void *data,
struct git_config_source *config_source,
int respect_includes)
{
- char *repo_config = NULL;
- int ret;
struct config_include_data inc = CONFIG_INCLUDE_INIT;
if (respect_includes) {
@@ -1257,11 +1257,7 @@ int git_config_with_options(config_fn_t fn, void *data,
else if (config_source && config_source->blob)
return git_config_from_blob_ref(fn, config_source->blob, data);
- repo_config = git_pathdup("config");
- ret = git_config_early(fn, data, repo_config);
- if (repo_config)
- free(repo_config);
- return ret;
+ return do_git_config_sequence(fn, data);
}
static void git_config_raw(config_fn_t fn, void *data)
--
2.8.0.rc0.278.gfeb5644
--
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 03/10] lazily load core.sharedrepository
The "shared_repository" config is loaded as part of
check_repository_format_version, but it's not quite like the
other values we check there. Something like
core.repositoryformatversion only makes sense in per-repo
config, but core.sharedrepository can be set in a per-user
config (e.g., to make all "git init" invocations shared by
default).
So it would make more sense as part of git_default_config.
Commit 457f06d (Introduce core.sharedrepository, 2005-12-22)
says:
[...]the config variable is set in the function which
checks the repository format. If this were done in
git_default_config instead, a lot of programs would need
to be modified to call git_config(git_default_config)
first.
This is still the case today, but we have one extra trick up
our sleeve. Now that we have the git_configset
infrastructure, it's not so expensive for us to ask for a
single value. So we can simply lazy-load it on demand.
This should be OK to do in general. There are some problems
with loading config before setup_git_directory() is called,
but we shouldn't be accessing the value before then (if we
were, then it would already be broken, as the variable would
not have been set by check_repository_format_version!). The
trickiest caller is git-init, but it handles the values
manually itself.
Signed-off-by: Jeff King
---
Of the whole series, this is the one I most fear causing problems. It
passes the test suite, but that isn't to say there isn't a weird corner
case lurking.
environment.c | 9 +
setup.c | 2 --
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/environment.c b/environment.c
index b42e238..8b8c8e8 100644
--- a/environment.c
+++ b/environment.c
@@ -326,13 +326,22 @@ const char *get_commit_output_encoding(void)
}
static int the_shared_repository = PERM_UMASK;
+static int need_shared_repository_from_config = 1;
void set_shared_repository(int value)
{
the_shared_repository = value;
+ need_shared_repository_from_config = 0;
}
int get_shared_repository(void)
{
+ if (need_shared_repository_from_config) {
+ const char *var = "core.sharedrepository";
+ const char *value;
+ if (!git_config_get_value(var, &value))
+ the_shared_repository = git_config_perm(var, value);
+ need_shared_repository_from_config = 0;
+ }
return the_shared_repository;
}
diff --git a/setup.c b/setup.c
index ac777c5..a02932b 100644
--- a/setup.c
+++ b/setup.c
@@ -376,8 +376,6 @@ static int check_repo_format(const char *var, const char
*value, void *cb)
if (strcmp(var, "core.repositoryformatversion") == 0)
repository_format_version = git_config_int(var, value);
- else if (strcmp(var, "core.sharedrepository") == 0)
- set_shared_repository(git_config_perm(var, value));
else if (skip_prefix(var, "extensions.", &ext)) {
/*
* record any known extensions here; otherwise,
--
2.8.0.rc0.278.gfeb5644
--
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 02/10] wrap shared_repository global in get/set accessors
It would be useful to control access to the global
shared_repository, so that we can lazily load its config.
The first step to doing so is to make sure all access
goes through a set of functions.
This step is purely mechanical, and should result in no
change of behavior.
Signed-off-by: Jeff King
---
builtin/init-db.c | 24
cache.h | 4 +++-
environment.c | 13 -
path.c| 10 +-
setup.c | 2 +-
5 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 6223b7d..e9b2256 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -199,13 +199,13 @@ static int create_default_files(const char *template_path)
/* reading existing config may have overwrote it */
if (init_shared_repository != -1)
- shared_repository = init_shared_repository;
+ set_shared_repository(init_shared_repository);
/*
* We would have created the above under user's umask -- under
* shared-repository settings, we would need to fix them up.
*/
- if (shared_repository) {
+ if (get_shared_repository()) {
adjust_shared_perm(get_git_dir());
adjust_shared_perm(git_path_buf(&buf, "refs"));
adjust_shared_perm(git_path_buf(&buf, "refs/heads"));
@@ -369,7 +369,7 @@ int init_db(const char *template_dir, unsigned int flags)
create_object_directory();
- if (shared_repository) {
+ if (get_shared_repository()) {
char buf[10];
/* We do not spell "group" and such, so that
* the configuration can be read by older version
@@ -377,12 +377,12 @@ int init_db(const char *template_dir, unsigned int flags)
* and compatibility values for PERM_GROUP and
* PERM_EVERYBODY.
*/
- if (shared_repository < 0)
+ if (get_shared_repository() < 0)
/* force to the mode value */
- xsnprintf(buf, sizeof(buf), "0%o", -shared_repository);
- else if (shared_repository == PERM_GROUP)
+ xsnprintf(buf, sizeof(buf), "0%o",
-get_shared_repository());
+ else if (get_shared_repository() == PERM_GROUP)
xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_GROUP);
- else if (shared_repository == PERM_EVERYBODY)
+ else if (get_shared_repository() == PERM_EVERYBODY)
xsnprintf(buf, sizeof(buf), "%d", OLD_PERM_EVERYBODY);
else
die("BUG: invalid value for shared_repository");
@@ -398,7 +398,7 @@ int init_db(const char *template_dir, unsigned int flags)
"", and the last '%s%s' is the verbatim directory name. */
printf(_("%s%s Git repository in %s%s\n"),
reinit ? _("Reinitialized existing") : _("Initialized
empty"),
- shared_repository ? _(" shared") : "",
+ get_shared_repository() ? _(" shared") : "",
git_dir, len && git_dir[len-1] != '/' ? "/" : "");
}
@@ -493,8 +493,8 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
* and we know shared_repository should always
be 0;
* but just in case we play safe.
*/
- saved = shared_repository;
- shared_repository = 0;
+ saved = get_shared_repository();
+ set_shared_repository(0);
switch
(safe_create_leading_directories_const(argv[0])) {
case SCLD_OK:
case SCLD_PERMS:
@@ -506,7 +506,7 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
die_errno(_("cannot mkdir %s"),
argv[0]);
break;
}
- shared_repository = saved;
+ set_shared_repository(saved);
if (mkdir(argv[0], 0777) < 0)
die_errno(_("cannot mkdir %s"),
argv[0]);
mkdir_tried = 1;
@@ -524,7 +524,7 @@ int cmd_init_db(int argc, const char **argv, const char
*prefix)
}
if (init_shared_repository != -1)
- shared_repository = init_shared_repository;
+ set_shared_repository(init_shared_repository);
/*
* GIT_WORK_TREE makes sense only in conjunction with GIT_DIR
diff --git a/cache.h b/cache.h
index a3b6b0f..43c6a44 100644
--- a/cache.h
+++ b/cache.h
@@ -651,7 +651,6 @@ extern int pr
[PATCH 01/10] setup: document check_repository_format()
This function's interface is rather enigmatic, so let's
document it further.
While we're here, let's also drop the return value. It will
always either be "0" or the function will die (consequently,
neither of its two callers bothered to check the return).
Signed-off-by: Jeff King
---
cache.h | 9 -
setup.c | 4 ++--
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/cache.h b/cache.h
index d7ff46e..a3b6b0f 100644
--- a/cache.h
+++ b/cache.h
@@ -747,7 +747,14 @@ extern int grafts_replace_parents;
#define GIT_REPO_VERSION_READ 1
extern int repository_format_version;
extern int repository_format_precious_objects;
-extern int check_repository_format(void);
+
+/*
+ * Check the repository format version in the path found in get_git_dir(),
+ * and die if it is a version we don't understand. Generally one would
+ * set_git_dir() before calling this, and use it only for "are we in a valid
+ * repo?".
+ */
+extern void check_repository_format(void);
#define MTIME_CHANGED 0x0001
#define CTIME_CHANGED 0x0002
diff --git a/setup.c b/setup.c
index de1a2a7..b2f2e69 100644
--- a/setup.c
+++ b/setup.c
@@ -982,9 +982,9 @@ int check_repository_format_version(const char *var, const
char *value, void *cb
return 0;
}
-int check_repository_format(void)
+void check_repository_format(void)
{
- return check_repository_format_gently(get_git_dir(), NULL);
+ check_repository_format_gently(get_git_dir(), NULL);
}
/*
--
2.8.0.rc0.278.gfeb5644
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/10] cleaning up check_repository_format_gently
After the discussion in: http://thread.gmane.org/gmane.comp.version-control.git/287949/focus=288002 I came up with this series to try to address some of the warts in check_repository_format_gently(). I had hoped to come up with something very small that could go at the front of the refs-backend series, but as with any time I look at the setup code, it managed to snowball into a potpourri of hidden assumptions. So I hope David isn't _too_ irritated to see this in his inbox. Rebasing the refs-backend on top shouldn't be too bad, and the end result would be cleaner and more correct. My bigger worry is just that changing the setup code is inherently flaky, and I don't want to hold David's series hostage. [01/10]: setup: document check_repository_format() [02/10]: wrap shared_repository global in get/set accessors [03/10]: lazily load core.sharedrepository [04/10]: check_repository_format_gently: stop using git_config_early [05/10]: config: drop git_config_early [06/10]: setup: refactor repo format reading and verification [07/10]: init: use setup.c's repo version verification [08/10]: setup: unify repository version callbacks [09/10]: setup: drop repository_format_version global [10/10]: setup: drop GIT_REPO_VERSION constants -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 v2] Mark win32's pthread_exit() as NORETURN
The pthread_exit() function is not expected to return. Ever. On Windows,
we call ExitThread() whose documentation claims: "This function does not
return a value.":
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659
Pointed out by Jeff King.
Signed-off-by: Johannes Schindelin
---
Relative to v1, only the commit message changed (to clarify that
ExitThread() indeed never returns).
compat/win32/pthread.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 20b35a2..148db60 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void
**value_ptr);
#define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
extern pthread_t pthread_self(void);
-static inline int pthread_exit(void *ret)
+static inline int NORETURN pthread_exit(void *ret)
{
ExitThread((DWORD)(intptr_t)ret);
}
--
2.7.2.windows.1.5.g64acc33
--
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] Mark win32's pthread_exit() as NORETURN
Hi Peff,
On Tue, 1 Mar 2016, Jeff King wrote:
> On Tue, Mar 01, 2016 at 02:53:04PM +0100, Johannes Schindelin wrote:
>
> > The pthread_exit() function is not expected to return. Ever.
> >
> > Pointed out by Jeff King.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > compat/win32/pthread.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> > index 20b35a2..148db60 100644
> > --- a/compat/win32/pthread.h
> > +++ b/compat/win32/pthread.h
> > @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void
> > **value_ptr);
> > #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
> > extern pthread_t pthread_self(void);
> >
> > -static inline int pthread_exit(void *ret)
> > +static inline int NORETURN pthread_exit(void *ret)
> > {
> > ExitThread((DWORD)(intptr_t)ret);
> > }
>
> Looks obviously correct to me (I'll assume Windows isn't so crazy as to
> let ExitThread ever return :) ).
Indeed, you are correct in your implicit assumption that I should have
clarified that in my commit message. I will amend the commit message.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Mark win32's pthread_exit() as NORETURN
On Tue, Mar 01, 2016 at 02:53:04PM +0100, Johannes Schindelin wrote:
> The pthread_exit() function is not expected to return. Ever.
>
> Pointed out by Jeff King.
>
> Signed-off-by: Johannes Schindelin
> ---
> compat/win32/pthread.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> index 20b35a2..148db60 100644
> --- a/compat/win32/pthread.h
> +++ b/compat/win32/pthread.h
> @@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void
> **value_ptr);
> #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
> extern pthread_t pthread_self(void);
>
> -static inline int pthread_exit(void *ret)
> +static inline int NORETURN pthread_exit(void *ret)
> {
> ExitThread((DWORD)(intptr_t)ret);
> }
Looks obviously correct to me (I'll assume Windows isn't so crazy as to
let ExitThread ever return :) ).
-Peff
> --
> 2.7.2.windows.1.5.g64acc33
--
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] Mark win32's pthread_exit() as NORETURN
The pthread_exit() function is not expected to return. Ever.
Pointed out by Jeff King.
Signed-off-by: Johannes Schindelin
---
compat/win32/pthread.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 20b35a2..148db60 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void
**value_ptr);
#define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
extern pthread_t pthread_self(void);
-static inline int pthread_exit(void *ret)
+static inline int NORETURN pthread_exit(void *ret)
{
ExitThread((DWORD)(intptr_t)ret);
}
--
2.7.2.windows.1.5.g64acc33
--
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] compat/mingw: brown paper bag fix for 50a6c8e
Hi Peff, On Tue, 1 Mar 2016, Jeff King wrote: > On Tue, Mar 01, 2016 at 06:49:43AM +0100, Torsten Bögershausen wrote: > > > However, suspecting jk/epipe-in-async, I don't know if we can do > > something against this warning: > > > > CC run-command.o run-command.c: In function 'async_exit': > > run-command.c:631:1: warning: 'noreturn' function does return } ^ > > The only thing that function does is call pthread_exit(), which should > also be marked NORETURN. Looks like the one in compat/win32/pthread.h > isn't? Correct. Expect a patch momentarily. Ciao, Dscho
Re: GSoC 2016 Microproject
On 03/01, Sidhant Sharma wrote: > > > If you use PARSE_OPT_HIDDEN, I think you don't need to specify a message. > > Otherwise, the documentation only has value if it contains more than just > > the option name, but that is the hard part if you're not familiar with the > > code. The best place to find documentation is in the history (git blame the > > file and see if the commit message introducing the option enlightens you). > > But that's why I said this didn't have to be part of the microproject: > > writting good doc requires a good understanding of the whole thing ... > I used OPT_HIDDEN_BOOL for all except for reject-thin-pack-for-testing, where > I used PARSE_OPT_HIDDEN. I ran the test locally and also on Travis, and the > all tests passed. How do I proceed now? Now you can send a patch to the mailing list. See Documentation/SubmittingPatches for more details. It usually is helpful to send the patches to yourself first as well, and try to apply them using git am, to avoid mistakes in the patch submission. > > Thanks and regards, > Sidhant Sharma [:tk] > -- > 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 -- Thomas -- 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 2016 Microproject
> If you use PARSE_OPT_HIDDEN, I think you don't need to specify a message. > Otherwise, the documentation only has value if it contains more than just the > option name, but that is the hard part if you're not familiar with the code. > The best place to find documentation is in the history (git blame the file > and see if the commit message introducing the option enlightens you). But > that's why I said this didn't have to be part of the microproject: writting > good doc requires a good understanding of the whole thing ... I used OPT_HIDDEN_BOOL for all except for reject-thin-pack-for-testing, where I used PARSE_OPT_HIDDEN. I ran the test locally and also on Travis, and the all tests passed. How do I proceed now? Thanks and regards, Sidhant Sharma [:tk] -- 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
Bypassing hooks while cherry-picking
Hello, using git 2.1.4 here, and it seems there is no option to bypass pre-commit hooks while cherry-picking, while git commit provides a --no-verify option. I ended up doing this to disable hooks while cherry picking : test -f "$GIT_DIR"/CHERRY_PICK_HEAD && exit 0 Wouldn't it be best to add the --no-verify option to cherry-pick too? I had a conflict when cherry-picking the commit, maybe this does not happen otherwise? Steps to reproduce : 1. create a pre-commit hook 2. create a commit that fails the hook, and bypass the hook 3. checkout another branch 4. might be optional : create a conflicting change with the previously created commit 5. cherry-pick the commit 6. might be optional : solve the conflick and use git cherry-pick --continue Regards, -- greg0ire -- 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 v1] git-p4: fix AsciiDoc formatting
From: Lars Schneider Noticed-by: Eric Sunshine Signed-off-by: Lars Schneider --- Documentation/git-p4.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt index 738cfde..140fc12 100644 --- a/Documentation/git-p4.txt +++ b/Documentation/git-p4.txt @@ -528,7 +528,7 @@ git-p4.largeFileSystem:: git config git-p4.largeFileSystem GitLFS - + - [1] https://git-lfs.github.com/ +[1] https://git-lfs.github.com/ git-p4.largeFileExtensions:: All files matching a file extension in the list will be processed -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-p4: map a P4 user to Git author name and email address
On 01 Mar 2016, at 11:49, larsxschnei...@gmail.com wrote:
> From: Lars Schneider
oops ... this should be "larsxschnei...@gmail.com"... sorry
- Lars
>
> Map a P4 user to a specific name and email address in Git with the
> "git-p4.mapUser" config. The config value must be a string adhering
> to the format "p4user = First Lastname ".
>
> Signed-off-by: Lars Schneider
> ---
>
> diff to v1:
>
> * use '=' instead of '->' to mimic SVN user mapping format (thanks Eric)
> * fix ASCII doc formatting (thanks Eric)
> * fix broken && chain in test (thanks Eric)
> * use more innocuous names for test users (thanks Luke and Torsten)
> * make regex accept more blank characters
>
> Cheers,
> Lars
>
>
> Documentation/git-p4.txt | 11 +
> git-p4.py | 9 +++
> t/t9828-git-p4-map-user.sh | 61 ++
> 3 files changed, 81 insertions(+)
> create mode 100755 t/t9828-git-p4-map-user.sh
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index 738cfde..9f077fd 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -553,6 +553,17 @@ git-p4.keepEmptyCommits::
> A changelist that contains only excluded files will be imported
> as an empty commit if this boolean option is set to true.
>
> +git-p4.mapUser::
> + Map a P4 user to a name and email address in Git. Use a string
> + with the following format to create a mapping:
> ++
> +-
> +git config --add git-p4.mapUser "p4user = First Last "
> +-
> ++
> +A mapping will override any user information from P4. Mappings for
> +multiple P4 user can be defined.
> +
> Submit variables
>
> git-p4.detectRenames::
> diff --git a/git-p4.py b/git-p4.py
> index c33dece..bac341d 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1160,6 +1160,15 @@ class P4UserMap:
> self.users[output["User"]] = output["FullName"] + " <" +
> output["Email"] + ">"
> self.emails[output["Email"]] = output["User"]
>
> +mapUserConfigRegex =
> re.compile(r"^\s*(\S+)\s*=\s*(.+)\s*<(\S+)>\s*$", re.VERBOSE)
> +for mapUserConfig in gitConfigList("git-p4.mapUser"):
> +mapUser = mapUserConfigRegex.findall(mapUserConfig)
> +if mapUser and len(mapUser[0]) == 3:
> +user = mapUser[0][0]
> +fullname = mapUser[0][1]
> +email = mapUser[0][2]
> +self.users[user] = fullname + " <" + email + ">"
> +self.emails[email] = user
>
> s = ''
> for (key, val) in self.users.items():
> diff --git a/t/t9828-git-p4-map-user.sh b/t/t9828-git-p4-map-user.sh
> new file mode 100755
> index 000..e20395c
> --- /dev/null
> +++ b/t/t9828-git-p4-map-user.sh
> @@ -0,0 +1,61 @@
> +#!/bin/sh
> +
> +test_description='Clone repositories and map users'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> + start_p4d
> +'
> +
> +test_expect_success 'Create a repo with different users' '
> + client_view "//depot/... //client/..." &&
> + (
> + cd "$cli" &&
> +
> + >author.txt &&
> + p4 add author.txt &&
> + p4 submit -d "Add file author\\n" &&
> +
> + P4USER=mmax &&
> + >max.txt &&
> + p4 add max.txt &&
> + p4 submit -d "Add file max" &&
> +
> + P4USER=eri &&
> + >moritz.txt &&
> + p4 add moritz.txt &&
> + p4 submit -d "Add file moritz" &&
> +
> + P4USER=no &&
> + >nobody.txt &&
> + p4 add nobody.txt &&
> + p4 submit -d "Add file nobody"
> + )
> +'
> +
> +test_expect_success 'Clone repo root path with all history' '
> + client_view "//depot/... //client/..." &&
> + test_when_finished cleanup_git &&
> + (
> + cd "$git" &&
> + git init . &&
> + git config --add git-p4.mapUser "mmax = Max Musterman
> " &&
> + git config --add git-p4.mapUser " eri=Erika Musterman
> " &&
> + git p4 clone --use-client-spec --destination="$git" //depot@all
> &&
> + cat >expect <<-\EOF &&
> + no
> + Erika Musterman
> + Max Musterman
> + Dr. author
> + EOF
> + git log --format="%an <%ae>" >actual &&
> + test_cmp expect actual
> + )
> +'
> +
> +test_expect_success 'kill p4d' '
> + kill_p4d
> +'
> +
> +test_done
> --
> 2.5.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
[PATCH v2] git-p4: map a P4 user to Git author name and email address
From: Lars Schneider
Map a P4 user to a specific name and email address in Git with the
"git-p4.mapUser" config. The config value must be a string adhering
to the format "p4user = First Lastname ".
Signed-off-by: Lars Schneider
---
diff to v1:
* use '=' instead of '->' to mimic SVN user mapping format (thanks Eric)
* fix ASCII doc formatting (thanks Eric)
* fix broken && chain in test (thanks Eric)
* use more innocuous names for test users (thanks Luke and Torsten)
* make regex accept more blank characters
Cheers,
Lars
Documentation/git-p4.txt | 11 +
git-p4.py | 9 +++
t/t9828-git-p4-map-user.sh | 61 ++
3 files changed, 81 insertions(+)
create mode 100755 t/t9828-git-p4-map-user.sh
diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 738cfde..9f077fd 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -553,6 +553,17 @@ git-p4.keepEmptyCommits::
A changelist that contains only excluded files will be imported
as an empty commit if this boolean option is set to true.
+git-p4.mapUser::
+ Map a P4 user to a name and email address in Git. Use a string
+ with the following format to create a mapping:
++
+-
+git config --add git-p4.mapUser "p4user = First Last "
+-
++
+A mapping will override any user information from P4. Mappings for
+multiple P4 user can be defined.
+
Submit variables
git-p4.detectRenames::
diff --git a/git-p4.py b/git-p4.py
index c33dece..bac341d 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1160,6 +1160,15 @@ class P4UserMap:
self.users[output["User"]] = output["FullName"] + " <" +
output["Email"] + ">"
self.emails[output["Email"]] = output["User"]
+mapUserConfigRegex = re.compile(r"^\s*(\S+)\s*=\s*(.+)\s*<(\S+)>\s*$",
re.VERBOSE)
+for mapUserConfig in gitConfigList("git-p4.mapUser"):
+mapUser = mapUserConfigRegex.findall(mapUserConfig)
+if mapUser and len(mapUser[0]) == 3:
+user = mapUser[0][0]
+fullname = mapUser[0][1]
+email = mapUser[0][2]
+self.users[user] = fullname + " <" + email + ">"
+self.emails[email] = user
s = ''
for (key, val) in self.users.items():
diff --git a/t/t9828-git-p4-map-user.sh b/t/t9828-git-p4-map-user.sh
new file mode 100755
index 000..e20395c
--- /dev/null
+++ b/t/t9828-git-p4-map-user.sh
@@ -0,0 +1,61 @@
+#!/bin/sh
+
+test_description='Clone repositories and map users'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+test_expect_success 'Create a repo with different users' '
+ client_view "//depot/... //client/..." &&
+ (
+ cd "$cli" &&
+
+ >author.txt &&
+ p4 add author.txt &&
+ p4 submit -d "Add file author\\n" &&
+
+ P4USER=mmax &&
+ >max.txt &&
+ p4 add max.txt &&
+ p4 submit -d "Add file max" &&
+
+ P4USER=eri &&
+ >moritz.txt &&
+ p4 add moritz.txt &&
+ p4 submit -d "Add file moritz" &&
+
+ P4USER=no &&
+ >nobody.txt &&
+ p4 add nobody.txt &&
+ p4 submit -d "Add file nobody"
+ )
+'
+
+test_expect_success 'Clone repo root path with all history' '
+ client_view "//depot/... //client/..." &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git init . &&
+ git config --add git-p4.mapUser "mmax = Max Musterman
" &&
+ git config --add git-p4.mapUser " eri=Erika Musterman
" &&
+ git p4 clone --use-client-spec --destination="$git" //depot@all
&&
+ cat >expect <<-\EOF &&
+ no
+ Erika Musterman
+ Max Musterman
+ Dr. author
+ EOF
+ git log --format="%an <%ae>" >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/22] refs/files-backend.c: mark strings for translation
On Tue, Mar 1, 2016 at 1:43 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- > > I'd really prefer to avoid any code churn on this file before the > dust settles for David's and Michael's series (the former is in > 'pu', the latter is not but was already rerolled once) both of which > touch this file heavily. Yeah. I actually dropped a patch on fetch-pack.c because my series shallow-deepen also changes a lot there, then somehow I forgot that David's series moves a bunch of refs code around. I'll keep an eye of those series and resend once they enter 'next'. -- 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: [PATCH v7 01/33] setup: call setup_git_directory_gently before accessing refs
On Tue, Mar 01, 2016 at 04:53:30PM +0700, Duy Nguyen wrote: > > The basic strategy was to adapt the > > existing "struct startup_info" to be available everywhere, and have > > relevant bits of code assert() on it, or even behave differently (e.g., > > if some library code should do different things in a repo versus not). > > startup_info is NULL for external programs if I remember correctly, or > do you make it available to all of them too? Yes, that was what I meant by "available everywhere". Library code cannot rely on it right now, as only builtins set it up (even though external programs may call setup_git_directory()). -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 v7 01/33] setup: call setup_git_directory_gently before accessing refs
On Tue, Mar 1, 2016 at 3:35 PM, Jeff King wrote: > On Mon, Feb 29, 2016 at 07:52:34PM -0500, David Turner wrote: > >> Usually, git calls some form of setup_git_directory at startup. But >> sometimes, it doesn't. Usually, that's OK because it's not really >> using the repository. But in some cases, it is using the repo. In >> those cases, either setup_git_directory_gently must be called, or the >> repository (e.g. the refs) must not be accessed. > > It's actually not just setup_git_directory(). We can also use > check_repository_format(), which is used by enter_repo() (and hence by > things like upload-pack). I think the rule really ought to be: if we > didn't have check_repository_format_gently() tell us we have a valid > repo, we should not access any repo elements (refs, objects, etc). Agreed. There's also a lighter version of check_repo.. which is is_git_directory(). Most of the time we just want to answer the question "is it a valid repository? support or not does not matter". We probably need more eyes on submodule case when this functino is used. For example in 25/33 [1] we check if a repo is non-bare (a variant of is_git_directory) then we peek the config file inside. Should check_repository_format() be done in this case? You know what, forget my question. The answer is yes. After writing all that, I remember that part of the config file may be moved away in the next version of multiple worktrees [2]. We need proper repo validation before reading anything inside. [1] http://article.gmane.org/gmane.comp.version-control.git/287959 [2] http://article.gmane.org/gmane.comp.version-control.git/284803 > I started earlier today on a patch series to identify and fix these > cases independent of your series. Yes this sounds like a separate problem, even though it's raised by lmdb topic. > The basic strategy was to adapt the > existing "struct startup_info" to be available everywhere, and have > relevant bits of code assert() on it, or even behave differently (e.g., > if some library code should do different things in a repo versus not). startup_info is NULL for external programs if I remember correctly, or do you make it available to all of them too? -- 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: [PATCH v7 29/33] setup: configure ref storage on setup
On Mon, Feb 29, 2016 at 07:53:02PM -0500, David Turner wrote:
> diff --git a/setup.c b/setup.c
> index bd3a2cf..e2e1220 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -457,6 +457,10 @@ static int check_repository_format_gently(const char
> *gitdir, int *nongit_ok)
> ret = -1;
> }
>
> + register_ref_storage_backends();
> + if (set_ref_storage_backend(ref_storage_backend))
> + die(_("Unknown ref storage backend %s"), ref_storage_backend);
> +
> strbuf_release(&sb);
> return ret;
> }
Much nicer than the one it replaces, I think.
This whole block should probably go inside
if (ret == 0) {
...
}
If we are doing setup_git_repository_gently() and we do _not_ find a
valid repository, we would not want to enable the ref storage.
I also wondered what happens to ref_storage_backend in a case where we
call check_repository_format_gently() multiple times. I think we don't
actually call it at all for submodules, so we at least we know we are
always dealing with the main repository.
But what if we call check_repository_format_gently(), find an
extensions.refstorage config key, set the global, but then _don't_
actually accept the repo (e.g., because its version is too high). Then
we keep looking and find another repo, which does not have
extensions.refstorage set. But we use the stale value from the first
directory.
I admit this is a pretty unlikely scenario. But I think it does point to
a mis-design in the way we read extensions in the config callback. They
should not go into globals until we're sure we're accepting this config
as the actual repository. So the existing "preciousobjects" extension
has this problem, too.
-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
