[PATCH] mailmap: use Kaartic Sivaraam's new address
Map the old address to the new, hopefully more permanent one. Signed-off-by: Kaartic Sivaraam--- .mailmap | 1 + 1 file changed, 1 insertion(+) diff --git a/.mailmap b/.mailmap index ab85e0d16..2634baef2 100644 --- a/.mailmap +++ b/.mailmap @@ -113,6 +113,7 @@ Junio C Hamano Junio C Hamano Junio C Hamano Junio C Hamano +Kaartic Sivaraam Karl Wiberg Karl Hasselström Karl Wiberg Karsten Blees -- 2.15.0.461.gf957c703b.dirty
Re: [PATCHv2 6/7] builtin/describe.c: describe a blob
Jacob Kellerwrites: > I think both questions are valuable, the first is "which commit last > had this blob", the second is "which commit first introduced this > blob", neither of which can always give a definitive answer. It really > depends on what question you're asking up front. Given that "describe" is about giving an object _a_ name that is plausibly useful to refer to it, it is not a good match for the above query that wants to know where it came from, how long it remains in the tree and where it ceases to be relevant. In order to support that use case, a totally different and possibly more useful avenue would be to think how this can be hooked into "git log" machinery. A refresher for how "git log [--options] " works may be beneficial. We walk history and compare the tree of the commit we are looking at with the tree of its parent commits. If everything within is the same, we mark the transition between the parent and our commit TREESAME (other commits, i.e. the ones that have meaningful change within , are !TREESAME). Then the output routine presents the set of commits that includes commits that are !TREESAME, within the constraints of the --options given (e.g. with --full-history, sides of a merge that is TREESAME may still be shown to preserve connectedness of the resulting graph). It is easy to imagine that we can restrict "git log" traversal with a "--blobchange=" option instead of (or in addition to) the limitation gives us. Instead of treating a commit whose diff against its parent commit has any filepair that is different within as "!TREESAME", we can treat a commit whose diff against its parent commit has a filepair that has the on either side of the filepair as "!TREESAME" (in other words, we ignore a transition that is not about introducing or forgetting the we are looking for as an "interesting change"). That would give you a commit history graph in which only (and all) such commits that either adds or removes the in it. Hmm?
Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation
On Friday 03 November 2017 12:12 AM, Stefan Beller wrote: On Thu, Nov 2, 2017 at 1:39 AM, Kaartic Sivaraamwrote: On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote: Signed-off-by: Kaartic Sivaraam Signed-off-by: Kaartic Sivaraam I just now saw this small glitch as a consequence of recently changing my email address. I would prefer to keep the second one but as the other patches have the first one it's better to keep the first one for now. If you prefer one of them, you may have incentive to add an entry into .mailmap file, otherwise I'd kindly ask you to. :) (c.f. `git log --no-merges -- .mailmap`) Sure, I'll do that. My intuition says this doesn't remove the duplicated sign-off line. Anyways, there's for sure a v4 that's going to update the connector string in [4/4] and another update. I'll be careful to address these issues in v4. --- Kaartic
Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming
On Thursday 02 November 2017 07:51 PM, Eric Sunshine wrote: Nicely explained; easily understood. Good to hear that. Translators can correct me, but this smells like "sentence lego"[1], which we'd like to avoid. Translators lack full context when presented with bits and pieces of a sentence like this, thus the translation may be of poor quality; it may even be entirely incorrect since they don't necessarily know how you will be composing the various pieces. You _might_ be able to able to resolve this by dropping "and" from: "foo is moo, and bar is boo" to turn the error messages into independent clauses: "foo is moo; bar is boo" > but I'm no translator, and even that may fail the lego sniff test. Though I can't be very sure that the one you suggested will pass the "lego sniff test", its better than the "and" I used. Further, my instinct says it wouldn't qualify as sentence lego (it's just a ";"). A sure-fire way to avoid lego construction would be simply to emit each messages on its own line: fatal: branch 'tset' doesn't exist fatal: branch 'master' already exists (though, you might consider that too ugly). Though it might not look that ugly, I don't know how you could make 'git' die() twice (or am I missing something)! Of course we could use 'error()' to report the errors and then 'die()' with a message like "Branch rename failed" but I find that to be a little too verbose and further using the connector ";" instead of "and" does seem to reduce the possibilities for the above program fragment to pass the "lego sniff test". --- Kaartic
Re: [PATCHv3 0/7] git describe blob
On Thu, Nov 2, 2017 at 6:46 PM, Junio C Hamanowrote: > Jacob Keller writes: > >> I agree, "something" is better than "nothing", and we can work to >> improve "something" in the future, especially after we get more real >> use and see what people think. Only question would be how much do we >> need to document the current behavior might be open for improvement? > > If > > - it always digs to the root of the history no matter what; and/or this is fixed. > - it almost always yields correct but suboptimal result this is not, for the lack of knowing what the optimal result is. > > then that fact must be documented in BUGS section, possibly a brief > descrition of why that limitation is there, with a hint to invite > people to look into fixing it. > > We should mark it prominently as experimental and advertise it as > such. "It's too slow in real project to be usable" I found it quite fast after fixing the history walking, but still. > and "Its output > bases the answer on an irrelevant commit" are not something we want > our users to experience, except for those with inclination to (or > ability and time to) help improve the feature. I think the current documentation states exactly this. Thanks.
Re: [PATCHv2] config: document blame configuration
On Thu, Nov 2, 2017 at 6:26 PM, SZEDER Gáborwrote: > On Thu, Nov 2, 2017 at 7:10 PM, Stefan Beller wrote: > >> +blame.blankBoundary:: >> + Show blank SHA-1 for boundary commits in linkgit:git-blame[1]. > > This is still SHA-1 instead of object id (or perhaps "commit object > name" would be even better). > Not sure whether oversight or intentional. definitely oversight.
Re: [PATCH 3/3] mingw: document the experimental standard handle redirection
Johannes Schindelinwrites: >> If I was correct in assuming that "2>&1" is just as foreign as >> ">/dev/null", then we should be shunning "2>&1" just like we shun >> ">/dev/null". That was all I meant to say. > > Did you know that `2>&1` works in Powershell? No. And that makes me curious if ">&-" is also there to replace "off" ;-)
Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages
Simon Ruderichwrites: > On Thu, Nov 02, 2017 at 02:16:52PM +0900, Junio C Hamano wrote: >> Junio C Hamano writes: >>> This patch is incomplete without adjusting a handful of tests to >>> expect the updated messages, no? >> >> I'll squash these in while queuing, but there might be more that I >> didn't notice. > > Sorry, didn't think about the tests. Heh, tests are not something you need to think about, if you always run them after making changes. > I've re-checked and I think those are the only affected tests. > The test suite passes with your squashed changes. OK. Thanks.
Re: [PATCHv3 0/7] git describe blob
Jacob Kellerwrites: > I agree, "something" is better than "nothing", and we can work to > improve "something" in the future, especially after we get more real > use and see what people think. Only question would be how much do we > need to document the current behavior might be open for improvement? If - it always digs to the root of the history no matter what; and/or - it almost always yields correct but suboptimal result then that fact must be documented in BUGS section, possibly a brief descrition of why that limitation is there, with a hint to invite people to look into fixing it. We should mark it prominently as experimental and advertise it as such. "It's too slow in real project to be usable" and "Its output bases the answer on an irrelevant commit" are not something we want our users to experience, except for those with inclination to (or ability and time to) help improve the feature.
Re: [PATCHv2] config: document blame configuration
On Thu, Nov 2, 2017 at 7:10 PM, Stefan Bellerwrote: > +blame.blankBoundary:: > + Show blank SHA-1 for boundary commits in linkgit:git-blame[1]. This is still SHA-1 instead of object id (or perhaps "commit object name" would be even better). Not sure whether oversight or intentional.
[PATCH] setup.c: don't try to access '//HEAD' during repo discovery
Commit ce9b8aab5 (setup_git_directory_1(): avoid changing global state, 2017-03-13) changed how the git directory is discovered, and as a side effect when the discovery reaches the root directory Git tries to access paths like '//HEAD' and '//objects'. This interacts badly with Cygwin, which interprets it as a UNC file share, and so demand-loads a bunch of extra DLLs and attempts to resolve/contact the server named HEAD. This obviously doesn't work too well, especially over a slow network link. Special case the root directory in is_git_directory() to prevent accessing paths with leading double slashes. Reported-by: Andrew BaumannSigned-off-by: SZEDER Gábor --- I'm not quite sure whether this is the right or complete fix. I can't test it on Cygwin, and I don't know whether Git for Windows is affected with it's drive prefixes in paths. Anyway, at least strace output on Linux looks good to me. setup.c | 4 1 file changed, 4 insertions(+) diff --git a/setup.c b/setup.c index 03f51e056..0cfc5e676 100644 --- a/setup.c +++ b/setup.c @@ -311,6 +311,10 @@ int is_git_directory(const char *suspect) int ret = 0; size_t len; + /* To avoid accessing '//HEAD' & co when checking the root directory */ + if (!strcmp(suspect, "/")) + suspect = ""; + /* Check worktree-related signatures */ strbuf_addf(, "%s/HEAD", suspect); if (validate_headref(path.buf)) -- 2.15.0.67.gb67a46776
Re: git tries to stat //HEAD when searching for a repo, leading to huge delays on Cygwin
On Thu, Nov 02, 2017 at 11:45:55PM +, Andrew Baumann wrote: > I have a workaround for this, but someone on stack overflow [1] > suggested reporting it upstream, so here you go: > > I have a fancy shell prompt that executes "git rev-parse > --is-inside-work-tree" to determine whether we're currently inside a > working directory. This causes git to walk up the directory hierarchy > looking for a containing git repo. For example, when invoked from my > home directory, it stats the following paths, in order: > > /home/me/.git > /home/me/.git/HEAD > /home/me/HEAD > /home > /home/.git > /home/.git/HEAD > /home/HEAD > / > /.git > /.git/HEAD > //HEAD > > The last name (//HEAD) interacts badly with Cygwin, which interprets > it as a UNC file share, and so demand-loads a bunch of extra DLLs and > attempts to resolve/contact the server named HEAD. This obviously > doesn't work too well, especially over a slow network link. > > I've tested with the latest Cygwin git (2.15.0); this was also present > in a prior version. Interesting. I can reproduce on Linux (but of course "//HEAD" is cheap to look at there). It bisects to ce9b8aab5d (setup_git_directory_1(): avoid changing global state, 2017-03-13). Before that, the end of the strace for "git rev-parse --git-dir" looks like: chdir("..") = 0 stat(".git", 0x7fffba398e00)= -1 ENOENT (No such file or directory) lstat(".git/HEAD", 0x7fffba398dd0) = -1 ENOENT (No such file or directory) lstat("./HEAD", 0x7fffba398dd0) = -1 ENOENT (No such file or directory) write(2, "fatal: Not a git repository (or "..., 69) = 69 and after: stat("/.git", 0x7ffdb28b7eb0) = -1 ENOENT (No such file or directory) lstat("/.git/HEAD", 0x7ffdb28b7e80) = -1 ENOENT (No such file or directory) lstat("//HEAD", 0x7ffdb28b7e80) = -1 ENOENT (No such file or directory) write(2, "fatal: Not a git repository (or "..., 69) = 69 Switching to using absolute paths rather than chdir-ing around is intentional for that commit, but it looks like we just need to special-case the construction of the root path. Like this, perhaps: diff --git a/setup.c b/setup.c index 27a31de33f..5d0b6a88e3 100644 --- a/setup.c +++ b/setup.c @@ -283,7 +283,9 @@ int is_git_directory(const char *suspect) size_t len; /* Check worktree-related signatures */ - strbuf_addf(, "%s/HEAD", suspect); + strbuf_addstr(, suspect); + strbuf_complete(, '/'); + strbuf_addstr(, "HEAD"); if (validate_headref(path.buf)) goto done; -Peff
Re: [PATCHv3 0/7] git describe blob
On Thu, Nov 2, 2017 at 12:41 PM, Stefan Bellerwrote: > Thanks for the discussion on v2[1]. > > Interdiff is below, just fixing minor things. > > We'll keep the original algorithm for now, deferring an improvement on > the algorithm front towards a future developer. > I agree, "something" is better than "nothing", and we can work to improve "something" in the future, especially after we get more real use and see what people think. Only question would be how much do we need to document the current behavior might be open for improvement? > Thanks, > Stefan > > [1] https://public-inbox.org/git/20171031211852.13001-1-sbel...@google.com/ > > Stefan Beller (7): > t6120: fix typo in test name > list-objects.c: factor out traverse_trees_and_blobs > revision.h: introduce blob/tree walking in order of the commits > builtin/describe.c: rename `oid` to avoid variable shadowing > builtin/describe.c: print debug statements earlier > builtin/describe.c: factor out describe_commit > builtin/describe.c: describe a blob > > Documentation/git-describe.txt | 13 +++- > Documentation/rev-list-options.txt | 5 ++ > builtin/describe.c | 125 > - > list-objects.c | 52 +-- > revision.c | 2 + > revision.h | 3 +- > t/t6100-rev-list-in-order.sh | 47 ++ > t/t6120-describe.sh| 17 - > 8 files changed, 214 insertions(+), 50 deletions(-) > create mode 100755 t/t6100-rev-list-in-order.sh > diff --git c/Documentation/git-describe.txt w/Documentation/git-describe.txt > index 077c3c2193..79ec0be62a 100644 > --- c/Documentation/git-describe.txt > +++ w/Documentation/git-describe.txt > @@ -11,7 +11,7 @@ SYNOPSIS > [verse] > 'git describe' [--all] [--tags] [--contains] [--abbrev=] [...] > 'git describe' [--all] [--tags] [--contains] [--abbrev=] --dirty[=] > -'git describe' [] > +'git describe' [] > > DESCRIPTION > --- > diff --git c/builtin/describe.c w/builtin/describe.c > index 382cbae908..cf08bef344 100644 > --- c/builtin/describe.c > +++ w/builtin/describe.c > @@ -440,6 +440,7 @@ struct process_commit_data { > struct object_id current_commit; > struct object_id looking_for; > struct strbuf *dst; > + struct rev_info *revs; > }; > > static void process_commit(struct commit *commit, void *data) > @@ -456,6 +457,7 @@ static void process_object(struct object *obj, const char > *path, void *data) > reset_revision_walk(); > describe_commit(>current_commit, pcd->dst); > strbuf_addf(pcd->dst, ":%s", path); > + pcd->revs->max_count = 0; > } > } > > @@ -463,7 +465,7 @@ static void describe_blob(struct object_id oid, struct > strbuf *dst) > { > struct rev_info revs; > struct argv_array args = ARGV_ARRAY_INIT; > - struct process_commit_data pcd = { null_oid, oid, dst}; > + struct process_commit_data pcd = { null_oid, oid, dst, }; > > argv_array_pushl(, "internal: The first arg is not parsed", > "--all", "--reflog", /* as many starting points as possible */ > @@ -497,14 +499,12 @@ static void describe(const char *arg, int last_one) > die(_("Not a valid object name %s"), arg); > cmit = lookup_commit_reference_gently(, 1); > > - if (cmit) { > + if (cmit) > describe_commit(, ); > - } else { > - if (lookup_blob()) > - describe_blob(oid, ); > - else > - die(_("%s is neither a commit nor blob"), arg); > - } > + else if (lookup_blob()) > + describe_blob(oid, ); > + else > + die(_("%s is neither a commit nor blob"), arg); > > puts(sb.buf); > > diff --git c/list-objects.c w/list-objects.c > index 03438e5a8b..07a92f35fe 100644 > --- c/list-objects.c > +++ w/list-objects.c > @@ -184,13 +184,13 @@ static void add_pending_tree(struct rev_info *revs, > struct tree *tree) > } > > static void traverse_trees_and_blobs(struct rev_info *revs, > -struct strbuf *base_path, > +struct strbuf *base, > show_object_fn show_object, > void *data) > { > int i; > > - assert(base_path->len == 0); > + assert(base->len == 0); > > for (i = 0; i < revs->pending.nr; i++) { > struct object_array_entry *pending = revs->pending.objects + > i; > @@ -208,12 +208,12 @@ static void traverse_trees_and_blobs(struct rev_info > *revs, > path = ""; > if (obj->type == OBJ_TREE) { > process_tree(revs, (struct tree *)obj, show_object, > -
RE: NEED A LOAN?
Hello, Do you need a loan? Regards Chen Alex Prime Funds
git tries to stat //HEAD when searching for a repo, leading to huge delays on Cygwin
Hi, I have a workaround for this, but someone on stack overflow [1] suggested reporting it upstream, so here you go: I have a fancy shell prompt that executes "git rev-parse --is-inside-work-tree" to determine whether we're currently inside a working directory. This causes git to walk up the directory hierarchy looking for a containing git repo. For example, when invoked from my home directory, it stats the following paths, in order: /home/me/.git /home/me/.git/HEAD /home/me/HEAD /home /home/.git /home/.git/HEAD /home/HEAD / /.git /.git/HEAD //HEAD The last name (//HEAD) interacts badly with Cygwin, which interprets it as a UNC file share, and so demand-loads a bunch of extra DLLs and attempts to resolve/contact the server named HEAD. This obviously doesn't work too well, especially over a slow network link. I've tested with the latest Cygwin git (2.15.0); this was also present in a prior version. Cheers, Andrew [1] https://stackoverflow.com/questions/47084672
Re: [PATCH 00/14] WIP Partial clone part 3: clone, fetch, fetch-pack, upload-pack, and tests
On Thu, 2 Nov 2017 20:31:15 + Jeff Hostetlerwrote: > From: Jeff Hostetler > > This is part 3 of 3 for partial clone. > It assumes that part 1 [1] and part 2 [2] are in place. > > Part 3 is concerned with the commands: clone, fetch, upload-pack, fetch-pack, > remote-curl, index-pack, and the pack-protocol. > > Jonathan and I independently started on this task. This is a first > pass at merging those efforts. So there are several places that need > refactoring and cleanup. In particular, the test cases should be > squashed and new tests added. Thanks. What are your future plans with this patch set? In particular, the tests don't pass at HEAD^. I took a quick glance to see if there were any issues that I could immediately spot, but couldn't find any. I thought of fetch_if_missing, but it seems that it is indeed used in this patch set (as expected). I'll look at it more thorougly, and feel free to let me know if there is anything in particular you would like comments on.
Re: [PATCH v3 7/7] remote-mediawiki: show progress while fetching namespaces
On 2017-11-02 22:31:02, Thomas Adam wrote: > On Thu, Nov 02, 2017 at 06:26:43PM -0400, Antoine Beaupré wrote: >> On 2017-11-02 22:18:07, Thomas Adam wrote: >> > Hi, >> > >> > On Thu, Nov 02, 2017 at 05:25:18PM -0400, Antoine Beaupré wrote: >> >> +print {*STDERR} "$#{$mw_pages} found in namespace >> >> $local_namespace ($namespace_id)\n"; >> > >> > How is this any different to using warn()? I appreciate you're using a >> > globbed filehandle, but it seems superfluous to me. >> >> It's what is used everywhere in the module, I'm just tagging along. >> >> This was discussed before: there's an issue about cleaning up the >> messaging in that module, that can be fixed separately. > > Understood. That should happen sooner rather than later. Actually, is there a standard way to do this in git with Perl extensions? I know about "option verbosity N" but how should I translate this into Perl? Carp? Warn? Log::Any? Log4perl? Recommendations welcome... A. -- Si Dieu existe, j'espère qu'Il a une excuse valable - Daniel Pennac
Re: [PATCH v3 4/7] remote-mediawiki: skip virtual namespaces
On 2017-11-02 18:43:00, Eric Sunshine wrote: > On Thu, Nov 2, 2017 at 5:25 PM, Antoine Beaupréwrote: >> Virtual namespaces do not correspond to pages in the database and are >> automatically generated by MediaWiki. It makes little sense, >> therefore, to fetch pages from those namespaces and the MW API doesn't >> support listing those pages. >> >> According to the documentation, those virtual namespaces are currently >> "Special" (-1) and "Media" (-2) but we treat all negative namespaces >> as "virtual" as a future-proofing mechanism. >> >> Reviewed-by: Eric Sunshine > > It probably would be best to omit this Reviewed-by: since it was not > provided explicitly. More importantly, I'm neither a user of nor > familiar with MediaWiki or its API, so a Reviewed-by: from me has > little or no value. Probably best would be for someone such as > Matthieu to give his Reviewed-by: if he so desires. Alright, I was wondering what the process was for those. I didn't want to leave your contributions by the wayside... I'll wait a little while longer for more feedback and then resend without those. unless... @junio: my github repo has the branch without those Reviewed-by tags, iirc. so if you can to merge from there, that will keep me from sending yet another pile of patches for such a trivial change... a. -- Semantics is the gravity of abstraction.
Re: [PATCH v3 7/7] remote-mediawiki: show progress while fetching namespaces
On Thu, Nov 2, 2017 at 5:25 PM, Antoine Beaupréwrote: > Without this, the fetch process seems hanged while we fetch page > listings across the namespaces. Obviously, it should be possible to > silence this with -q, but that's an issue already present everywhere > in the code and should be fixed separately: > > https://github.com/Git-Mediawiki/Git-Mediawiki/issues/30 > > Reviewed-by: Eric Sunshine Ditto: It would be best to drop this Reviewed-by: since it has no value with my name attached to it and was not provided explicitly. > Signed-off-by: Antoine Beaupré
Re: [PATCH v3 6/7] remote-mediawiki: process namespaces in order
On Thu, Nov 2, 2017 at 5:25 PM, Antoine Beaupréwrote: > Ideally, we'd process them in numeric order since that is more > logical, but we can't do that yet since this is where we find the > numeric identifiers in the first place. Lexicographic order is a good > compromise. > > Reviewed-by: Eric Sunshine Ditto: It would be best to drop this Reviewed-by: since it has no value with my name attached to it and was not provided explicitly. > Signed-off-by: Antoine Beaupré
Re: [PATCH v3 5/7] remote-mediawiki: support fetching from (Main) namespace
On Thu, Nov 2, 2017 at 5:25 PM, Antoine Beaupréwrote: > When we specify a list of namespaces to fetch from, by default the MW > API will not fetch from the default namespace, refered to as "(Main)" > in the documentation: > > https://www.mediawiki.org/wiki/Manual:Namespace#Built-in_namespaces > > I haven't found a way to address that "(Main)" namespace when getting > the namespace ids: indeed, when listing namespaces, there is no > "canonical" field for the main namespace, although there is a "*" > field that is set to "" (empty). So in theory, we could specify the > empty namespace to get the main namespace, but that would make > specifying namespaces harder for the user: we would need to teach > users about the "empty" default namespace. It would also make the code > more complicated: we'd need to parse quotes in the configuration. > > So we simply override the query here and allow the user to specify > "(Main)" since that is the publicly documented name. > > Reviewed-by: Eric Sunshine As with the previous patch, it would be best to drop this Reviewed-by: since it has no value with my name attached to it and was not provided explicitly. > Signed-off-by: Antoine Beaupré
Re: [PATCH v3 4/7] remote-mediawiki: skip virtual namespaces
On Thu, Nov 2, 2017 at 5:25 PM, Antoine Beaupréwrote: > Virtual namespaces do not correspond to pages in the database and are > automatically generated by MediaWiki. It makes little sense, > therefore, to fetch pages from those namespaces and the MW API doesn't > support listing those pages. > > According to the documentation, those virtual namespaces are currently > "Special" (-1) and "Media" (-2) but we treat all negative namespaces > as "virtual" as a future-proofing mechanism. > > Reviewed-by: Eric Sunshine It probably would be best to omit this Reviewed-by: since it was not provided explicitly. More importantly, I'm neither a user of nor familiar with MediaWiki or its API, so a Reviewed-by: from me has little or no value. Probably best would be for someone such as Matthieu to give his Reviewed-by: if he so desires. > Signed-off-by: Antoine Beaupré
Re: [PATCH v3 7/7] remote-mediawiki: show progress while fetching namespaces
On Thu, Nov 02, 2017 at 06:26:43PM -0400, Antoine Beaupré wrote: > On 2017-11-02 22:18:07, Thomas Adam wrote: > > Hi, > > > > On Thu, Nov 02, 2017 at 05:25:18PM -0400, Antoine Beaupré wrote: > >> +print {*STDERR} "$#{$mw_pages} found in namespace > >> $local_namespace ($namespace_id)\n"; > > > > How is this any different to using warn()? I appreciate you're using a > > globbed filehandle, but it seems superfluous to me. > > It's what is used everywhere in the module, I'm just tagging along. > > This was discussed before: there's an issue about cleaning up the > messaging in that module, that can be fixed separately. Understood. That should happen sooner rather than later. -- Thomas Adam
Re: [PATCH v3 7/7] remote-mediawiki: show progress while fetching namespaces
On 2017-11-02 22:18:07, Thomas Adam wrote: > Hi, > > On Thu, Nov 02, 2017 at 05:25:18PM -0400, Antoine Beaupré wrote: >> +print {*STDERR} "$#{$mw_pages} found in namespace $local_namespace >> ($namespace_id)\n"; > > How is this any different to using warn()? I appreciate you're using a > globbed filehandle, but it seems superfluous to me. It's what is used everywhere in the module, I'm just tagging along. This was discussed before: there's an issue about cleaning up the messaging in that module, that can be fixed separately. A. -- N'aimer qu'un seul est barbarie, car c'est au détriment de tous les autres. Fût-ce l'amour de Dieu. - Nietzsche, "Par delà le bien et le mal"
Re: [PATCH 1/9] extension.partialclone: introduce partial clone extension
On Thu, 2 Nov 2017 20:20:44 + Jeff Hostetlerwrote: > From: Jeff Hostetler > > Introduce the ability to have missing objects in a repo. This > functionality is guarded by new repository extension options: > `extensions.partialcloneremote` and > `extensions.partialclonefilter`. With this, it is unclear what happens if extensions.partialcloneremote is not set but extensions.partialclonefilter is set. For something as significant as a repository extension (which Git uses to determine if it will even attempt to interact with a repo), I think - I would prefer just extensions.partialclone (or extensions.partialcloneremote, though I prefer the former) which determines the remote (the important part, which controls the dynamic object fetching), and have another option "core.partialclonefilter" which is only useful if "extensions.partialclone" is set. I also don't think extensions.partialclonefilter (or core.partialclonefilter, if we use my suggestion) needs to be introduced so early in the patch set when it will only be used once we start fetching/cloning. > +void partial_clone_utils_register( > + const struct list_objects_filter_options *filter_options, > + const char *remote, > + const char *cmd_name) > +{ This function is useful once we have fetch/clone, but probably not before that. Since the fetch/clone patches are several patches ahead, could this be moved there? > @@ -420,6 +420,19 @@ static int check_repo_format(const char *var, const char > *value, void *vdata) > ; > else if (!strcmp(ext, "preciousobjects")) > data->precious_objects = git_config_bool(var, value); > + > + else if (!strcmp(ext, KEY_PARTIALCLONEREMOTE)) > + if (!value) > + return config_error_nonbool(var); > + else > + data->partial_clone_remote = xstrdup(value); > + > + else if (!strcmp(ext, KEY_PARTIALCLONEFILTER)) > + if (!value) > + return config_error_nonbool(var); > + else > + data->partial_clone_filter = xstrdup(value); > + > else > string_list_append(>unknown_extensions, ext); > } else if (strcmp(var, "core.bare") == 0) { With a complicated block, probably better to use braces in these clauses.
Re: [PATCH v3 7/7] remote-mediawiki: show progress while fetching namespaces
Hi, On Thu, Nov 02, 2017 at 05:25:18PM -0400, Antoine Beaupré wrote: > +print {*STDERR} "$#{$mw_pages} found in namespace $local_namespace > ($namespace_id)\n"; How is this any different to using warn()? I appreciate you're using a globbed filehandle, but it seems superfluous to me. Kindly, Thomas
Re: Regression[2.14.3->2.15]: Interactive rebase fails if submodule is modified
On Thu, Nov 2, 2017 at 8:34 PM, Stefan Bellerwrote: > On Thu, Nov 2, 2017 at 1:30 AM, Orgad Shaneh wrote: >> I can't reproduce this with a minimal example, but it happens in my project. >> >> What I tried to do for reproducing is: >> rm -rf super sub >> mkdir sub; cd sub; git init >> git commit --allow-empty -m 'Initial commit' >> mkdir ../super; cd ../super >> git init >> git submodule add ../sub >> touch foo; git add foo sub >> git commit -m 'Initial commit' >> touch a; git add a; git commit -m 'a' >> touch b; git add b; git commit -m 'b' >> cd sub; git commit --allow-empty -m 'New commit'; cd .. >> git rebase -i HEAD^^ >> >> Then drop a. >> >> In my project I get: >> error: cannot rebase: You have unstaged changes. >> >> This works fine with 2.14.3. > > git log --oneline v2.14.3..v2.15.0 -- submodule.c > doesn't give any promising hints (i.e. I don't think one of a > submodule related series introduced this either by chance or > on purpose) > > "rebase -i" was rewritten into C in 570676e011, though > that series was extensively tested by DScho, so I wouldn't > want to point fingers here quickly. > > Would you be willing to bisect this behavior? Bisected to ff6f1f564c48def1f8e1852826bab58af5044b06: submodule-config: lazy-load a repository's .gitmodules file - Orgad
RE
my name is Mrs. Alice Walton, a business woman an America Citizen and the heiress to the fortune of Walmart stores, born October 7, 1949. I have a mission for you worth $100,000,000.00(Hundred Million United State Dollars) which I intend using for CHARITY
[PATCH v3 1/7] remote-mediawiki: add namespace support
From: KevinThis introduces a new remote.origin.namespaces argument that is a space-separated list of namespaces. The list of pages extract is then taken from all the specified namespaces. Reviewed-by: Antoine Beaupré Signed-off-by: Antoine Beaupré --- contrib/mw-to-git/git-remote-mediawiki.perl | 25 + 1 file changed, 25 insertions(+) diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl index e7f857c1a..5ffb57595 100755 --- a/contrib/mw-to-git/git-remote-mediawiki.perl +++ b/contrib/mw-to-git/git-remote-mediawiki.perl @@ -63,6 +63,10 @@ chomp(@tracked_pages); my @tracked_categories = split(/[ \n]/, run_git("config --get-all remote.${remotename}.categories")); chomp(@tracked_categories); +# Just like @tracked_categories, but for MediaWiki namespaces. +my @tracked_namespaces = split(/[ \n]/, run_git("config --get-all remote.${remotename}.namespaces")); +chomp(@tracked_namespaces); + # Import media files on pull my $import_media = run_git("config --get --bool remote.${remotename}.mediaimport"); chomp($import_media); @@ -256,6 +260,23 @@ sub get_mw_tracked_categories { return; } +sub get_mw_tracked_namespaces { +my $pages = shift; +foreach my $local_namespace (@tracked_namespaces) { +my $mw_pages = $mediawiki->list( { +action => 'query', +list => 'allpages', +apnamespace => get_mw_namespace_id($local_namespace), +aplimit => 'max' } ) +|| die $mediawiki->{error}->{code} . ': ' +. $mediawiki->{error}->{details} . "\n"; +foreach my $page (@{$mw_pages}) { +$pages->{$page->{title}} = $page; +} +} +return; +} + sub get_mw_all_pages { my $pages = shift; # No user-provided list, get the list of pages from the API. @@ -319,6 +340,10 @@ sub get_mw_pages { $user_defined = 1; get_mw_tracked_categories(\%pages); } + if (@tracked_namespaces) { + $user_defined = 1; + get_mw_tracked_namespaces(\%pages); + } if (!$user_defined) { get_mw_all_pages(\%pages); } -- 2.11.0
[PATCH v3 4/7] remote-mediawiki: skip virtual namespaces
Virtual namespaces do not correspond to pages in the database and are automatically generated by MediaWiki. It makes little sense, therefore, to fetch pages from those namespaces and the MW API doesn't support listing those pages. According to the documentation, those virtual namespaces are currently "Special" (-1) and "Media" (-2) but we treat all negative namespaces as "virtual" as a future-proofing mechanism. Reviewed-by: Eric SunshineSigned-off-by: Antoine Beaupré --- contrib/mw-to-git/git-remote-mediawiki.perl | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl index e7616e1a2..21fb2e302 100755 --- a/contrib/mw-to-git/git-remote-mediawiki.perl +++ b/contrib/mw-to-git/git-remote-mediawiki.perl @@ -264,10 +264,13 @@ sub get_mw_tracked_categories { sub get_mw_tracked_namespaces { my $pages = shift; foreach my $local_namespace (@tracked_namespaces) { +my $namespace_id = get_mw_namespace_id($local_namespace); +# virtual namespaces don't support allpages +next if !defined($namespace_id) || $namespace_id < 0; my $mw_pages = $mediawiki->list( { action => 'query', list => 'allpages', -apnamespace => get_mw_namespace_id($local_namespace), +apnamespace => $namespace_id, aplimit => 'max' } ) || die $mediawiki->{error}->{code} . ': ' . $mediawiki->{error}->{details} . "\n"; -- 2.11.0
[PATCH v3 0/7] remote-mediawiki: namespace support
This should be the final roll of patches for namespace support. I included the undef check even though that problem occurs elsewhere in the code. I also removed the needless "my" move. Hopefully that should be the last in the queue!
[PATCH v3 2/7] remote-mediawiki: allow fetching namespaces with spaces
From: Ingo Ruhnkewe still want to use spaces as separators in the config, but we should allow the user to specify namespaces with spaces, so we use underscore for this. Reviewed-by: Antoine Beaupré Signed-off-by: Antoine Beaupré --- contrib/mw-to-git/git-remote-mediawiki.perl | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl index 5ffb57595..a1d783789 100755 --- a/contrib/mw-to-git/git-remote-mediawiki.perl +++ b/contrib/mw-to-git/git-remote-mediawiki.perl @@ -65,6 +65,7 @@ chomp(@tracked_categories); # Just like @tracked_categories, but for MediaWiki namespaces. my @tracked_namespaces = split(/[ \n]/, run_git("config --get-all remote.${remotename}.namespaces")); +for (@tracked_namespaces) { s/_/ /g; } chomp(@tracked_namespaces); # Import media files on pull -- 2.11.0
[PATCH v3 3/7] remote-mediawiki: show known namespace choices on failure
If we fail to find a requested namespace, we should tell the user which ones we know about, since those were already fetched. This allows users to fetch all namespaces by specifying a dummy namespace, failing, then copying the list of namespaces in the config. Eventually, we should have a flag that allows fetching all namespaces automatically. Reviewed-by: Antoine BeaupréSigned-off-by: Antoine Beaupré --- contrib/mw-to-git/git-remote-mediawiki.perl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl index a1d783789..e7616e1a2 100755 --- a/contrib/mw-to-git/git-remote-mediawiki.perl +++ b/contrib/mw-to-git/git-remote-mediawiki.perl @@ -1334,7 +1334,9 @@ sub get_mw_namespace_id { my $id; if (!defined $ns) { - print {*STDERR} "No such namespace ${name} on MediaWiki.\n"; + my @namespaces = sort keys %namespace_id; + for (@namespaces) { s/ /_/g; } + print {*STDERR} "No such namespace ${name} on MediaWiki, known namespaces: @namespaces\n"; $ns = {is_namespace => 0}; $namespace_id{$name} = $ns; } -- 2.11.0
[PATCH v3 6/7] remote-mediawiki: process namespaces in order
Ideally, we'd process them in numeric order since that is more logical, but we can't do that yet since this is where we find the numeric identifiers in the first place. Lexicographic order is a good compromise. Reviewed-by: Eric SunshineSigned-off-by: Antoine Beaupré --- contrib/mw-to-git/git-remote-mediawiki.perl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl index 898541a9f..f53e638cf 100755 --- a/contrib/mw-to-git/git-remote-mediawiki.perl +++ b/contrib/mw-to-git/git-remote-mediawiki.perl @@ -263,7 +263,7 @@ sub get_mw_tracked_categories { sub get_mw_tracked_namespaces { my $pages = shift; -foreach my $local_namespace (@tracked_namespaces) { +foreach my $local_namespace (sort @tracked_namespaces) { my $namespace_id; if ($local_namespace eq "(Main)") { $namespace_id = 0; -- 2.11.0
[PATCH v3 7/7] remote-mediawiki: show progress while fetching namespaces
Without this, the fetch process seems hanged while we fetch page listings across the namespaces. Obviously, it should be possible to silence this with -q, but that's an issue already present everywhere in the code and should be fixed separately: https://github.com/Git-Mediawiki/Git-Mediawiki/issues/30 Reviewed-by: Eric SunshineSigned-off-by: Antoine Beaupré --- contrib/mw-to-git/git-remote-mediawiki.perl | 1 + 1 file changed, 1 insertion(+) diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl index f53e638cf..dc43a950b 100755 --- a/contrib/mw-to-git/git-remote-mediawiki.perl +++ b/contrib/mw-to-git/git-remote-mediawiki.perl @@ -279,6 +279,7 @@ sub get_mw_tracked_namespaces { aplimit => 'max' } ) || die $mediawiki->{error}->{code} . ': ' . $mediawiki->{error}->{details} . "\n"; +print {*STDERR} "$#{$mw_pages} found in namespace $local_namespace ($namespace_id)\n"; foreach my $page (@{$mw_pages}) { $pages->{$page->{title}} = $page; } -- 2.11.0
[PATCH v3 5/7] remote-mediawiki: support fetching from (Main) namespace
When we specify a list of namespaces to fetch from, by default the MW API will not fetch from the default namespace, refered to as "(Main)" in the documentation: https://www.mediawiki.org/wiki/Manual:Namespace#Built-in_namespaces I haven't found a way to address that "(Main)" namespace when getting the namespace ids: indeed, when listing namespaces, there is no "canonical" field for the main namespace, although there is a "*" field that is set to "" (empty). So in theory, we could specify the empty namespace to get the main namespace, but that would make specifying namespaces harder for the user: we would need to teach users about the "empty" default namespace. It would also make the code more complicated: we'd need to parse quotes in the configuration. So we simply override the query here and allow the user to specify "(Main)" since that is the publicly documented name. Reviewed-by: Eric SunshineSigned-off-by: Antoine Beaupré --- contrib/mw-to-git/git-remote-mediawiki.perl | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl b/contrib/mw-to-git/git-remote-mediawiki.perl index 21fb2e302..898541a9f 100755 --- a/contrib/mw-to-git/git-remote-mediawiki.perl +++ b/contrib/mw-to-git/git-remote-mediawiki.perl @@ -264,7 +264,12 @@ sub get_mw_tracked_categories { sub get_mw_tracked_namespaces { my $pages = shift; foreach my $local_namespace (@tracked_namespaces) { -my $namespace_id = get_mw_namespace_id($local_namespace); +my $namespace_id; +if ($local_namespace eq "(Main)") { +$namespace_id = 0; +} else { +$namespace_id = get_mw_namespace_id($local_namespace); +} # virtual namespaces don't support allpages next if !defined($namespace_id) || $namespace_id < 0; my $mw_pages = $mediawiki->list( { -- 2.11.0
Re: [PATCH 4/7] remote-mediawiki: skip virtual namespaces
On 2017-11-02 10:24:40, Junio C Hamano wrote: > Antoine Beaupréwrites: > >> It might still worth fixing this, but I'm not sure what the process is >> here - in the latest "what's cooking" Junio said this patchset would be >> merged in "next". Should I reroll the patchset to fix this or not? > > The process is for you (the contributor of the topic) to yell at me, > "don't merge it yet, there still are updates to come". YELL! "don't merge it yet, there still are updates to come". :) > That message _may_ come to late, in which case we may have to go > incremental, but I usually try to leave at least a few days between > the time I mark a topic as "will merge" and the time I actually do > the merge, for this exact reason. Awesome, thanks for the update. i'll roll a v4 with the last tweaks, hopefully that will be the last. a. -- How inappropriate to call this planet 'Earth' when it is quite clearly 'Ocean'. - Arthur C. Clarke
Re: No log --no-decorate completion?
On Thu, Nov 2, 2017 at 1:25 PM, Max Rothmanwrote: > No problem! Let me know if I've done something wrong with this patch, > I'm new to git's contributor process. Thanks for coming up with a patch! Yeah, the contribution process has some initial road blocks; I'll point them out below. > completion: add missing completions for log, diff, show I would think this is a good commit subject as it describes the changes made superficially > * Add bash completion for the missing --no-* options on git log > * Add bash completion for --textconv and --indent-heuristic families to > git diff and all commands that use diff's options > * Add bash completion for --no-abbrev-commit, --expand-tabs, and > --no-expand-tabs to git show This describes what happens in this patch, but not why, which helps future readers of commit message more than the "what". I'm also just guessing but maybe: A user might have configured a repo to show diffs in a certain way, add the --no-* options to the autocompletion to override it easier from the command line. New in this patch is autocompletion for --textconv as well as abbreviation options as that needs to be flipped all the time in . I wonder if we really want to expose indent-heuristic, I guess by not having it experimental any more it makes sense to do so. c.f. https://public-inbox.org/git/20171029151228.607834-1-...@dwim.me/ At the end of a commit message, the Git project requires a sign off. (See section (5) in Documentation/SubmittingPatches; tl;dr: add Signed-off-by: NAME if you can agree to https://developercertificate.org/) > --- > contrib/completion/git-completion.bash | 18 ++ > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/contrib/completion/git-completion.bash > b/contrib/completion/git-completion.bash > index 0e16f017a4144..252a6c8c0c80c 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -1412,6 +1412,8 @@ __git_diff_common_options="--stat --numstat > --shortstat --summary > --dirstat-by-file= --cumulative > --diff-algorithm= > --submodule --submodule= > + --indent-heuristic --no-indent-heuristic > + --textconv --no-textconv > " > > _git_diff () > @@ -1752,6 +1754,10 @@ _git_log () > __gitcomp "$__git_diff_submodule_formats" "" > "${cur##--submodule=}" > return > ;; > + --no-walk=*) > + __gitcomp "sorted unsorted" "" "${cur##--no-walk=}" > + return > + ;; > --*) > __gitcomp " > $__git_log_common_options > @@ -1759,16 +1765,19 @@ _git_log () > $__git_log_gitk_options > --root --topo-order --date-order --reverse > --follow --full-diff > - --abbrev-commit --abbrev= > + --abbrev-commit --no-abbrev-commit --abbrev= > --relative-date --date= > --pretty= --format= --oneline > --show-signature > --cherry-mark > --cherry-pick > --graph > - --decorate --decorate= > + --decorate --decorate= --no-decorate > --walk-reflogs > + --no-walk --no-walk= --do-walk > --parents --children > + --expand-tabs --expand-tabs= --no-expand-tabs > + --patch > $merge > $__git_diff_common_options > --pickaxe-all --pickaxe-regex > @@ -2816,8 +2825,9 @@ _git_show () > return > ;; > --*) > - __gitcomp "--pretty= --format= --abbrev-commit --oneline > - --show-signature > + __gitcomp "--pretty= --format= --abbrev-commit > --no-abbrev-commit > + --oneline --show-signature --patch > + --expand-tabs --expand-tabs= --no-expand-tabs > $__git_diff_common_options > " > return > The patch looks good, but doesn't apply because the email contains white spaces instead of tabs. Maybe try https://submitgit.herokuapp.com/ (or fix/change your email client to send a patch; the gmail web interface doesn't work. I personally got 'git send-email' up and running; The Documentation/SubmittingPatches has a section on email clients, too) Thanks, Stefan
Re: [PATCH 5/7] remote-mediawiki: support fetching from (Main) namespace
On 2017-11-01 15:56:51, Eric Sunshine wrote: >> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl >> b/contrib/mw-to-git/git-remote-mediawiki.perl >> @@ -264,9 +264,14 @@ sub get_mw_tracked_categories { >> sub get_mw_tracked_namespaces { >> my $pages = shift; >> foreach my $local_namespace (@tracked_namespaces) { >> -my $namespace_id = get_mw_namespace_id($local_namespace); >> +my ($namespace_id, $mw_pages); >> +if ($local_namespace eq "(Main)") { >> +$namespace_id = 0; >> +} else { >> +$namespace_id = get_mw_namespace_id($local_namespace); >> +} > > I meant to ask this in the previous round, but with the earlier patch > mixing several distinct changes into one, I plumb forgot: Would it > make sense to move this "(Main)" special case into > get_mw_namespace_id() itself? After all, that function is all about > determining an ID associated with a name, and "(Main)" is a name. Right. At first sight, I agree: get_mw_namespace_id should do the right thing. But then, I look at the code of that function, and it strikes me as ... well... really hard to actually do this the right way. In fact, I suspect that passing "" to get_mw_namespace_id would actually do the right thing. The problem, as I explained before, is that passing that in the configuration is pretty hard: it would needlessly complicate the configuration setting, so I think it's a fair shortcut to do it here. >> next if $namespace_id < 0; # virtual namespaces don't support >> allpages >> -my $mw_pages = $mediawiki->list( { >> +$mw_pages = $mediawiki->list( { > > Why did the "my" of $my_pages get moved up to the top of the foreach > loop? I can't seem to see any reason for it. Is this an unrelated > change accidentally included in this patch? Just a habit of declaring functions at the beginning of a block. Maybe it's because I'm old? :) I'll reroll a last patchset with those fixes. A. -- One of the strongest motives that leads men to art and science is escape from everyday life with its painful crudity and hopeless dreariness. Such men make this cosmos and its construction the pivot of their emotional life, in order to find the peace and security which they cannot find in the narrow whirlpool of personal experience. - Albert Einstein
[PATCH 01/14] upload-pack: add object filtering for partial clone
From: Jeff HostetlerTeach upload-pack to negotiate object filtering over the protocol and to send filter parameters to pack-objects. This is intended for partial clone and fetch. The idea to make upload-pack configurable using uploadpack.allowFilter comes from Jonathan Tan's work in [1]. [1] https://public-inbox.org/git/f211093280b422c32cc1b7034130072f35c5ed51.1506714999.git.jonathanta...@google.com/ Signed-off-by: Jeff Hostetler --- Documentation/config.txt | 4 Documentation/technical/pack-protocol.txt | 8 Documentation/technical/protocol-capabilities.txt | 8 upload-pack.c | 20 +++- 4 files changed, 39 insertions(+), 1 deletion(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 1ac0ae6..e528210 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -3268,6 +3268,10 @@ uploadpack.packObjectsHook:: was run. I.e., `upload-pack` will feed input intended for `pack-objects` to the hook, and expects a completed packfile on stdout. + +uploadpack.allowFilter:: + If this option is set, `upload-pack` will advertise partial + clone and partial fetch object filtering. + Note that this configuration variable is ignored if it is seen in the repository-level config (this is a safety measure against fetching from diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt index ed1eae8..a43a113 100644 --- a/Documentation/technical/pack-protocol.txt +++ b/Documentation/technical/pack-protocol.txt @@ -212,6 +212,7 @@ out of what the server said it could do with the first 'want' line. upload-request= want-list *shallow-line *1depth-request + [filter-request] flush-pkt want-list = first-want @@ -227,6 +228,8 @@ out of what the server said it could do with the first 'want' line. additional-want = PKT-LINE("want" SP obj-id) depth = 1*DIGIT + + filter-request= PKT-LINE("filter" SP filter-spec) Clients MUST send all the obj-ids it wants from the reference @@ -249,6 +252,11 @@ complete those commits. Commits whose parents are not received as a result are defined as shallow and marked as such in the server. This information is sent back to the client in the next step. +The client can optionally request that pack-objects omit various +objects from the packfile using one of several filtering techniques. +These are intended for use with partial clone and partial fetch +operations. See `rev-list` for possible "filter-spec" values. + Once all the 'want's and 'shallow's (and optional 'deepen') are transferred, clients MUST send a flush-pkt, to tell the server side that it is done sending the list. diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt index 26dcc6f..332d209 100644 --- a/Documentation/technical/protocol-capabilities.txt +++ b/Documentation/technical/protocol-capabilities.txt @@ -309,3 +309,11 @@ to accept a signed push certificate, and asks the to be included in the push certificate. A send-pack client MUST NOT send a push-cert packet unless the receive-pack server advertises this capability. + +filter +-- + +If the upload-pack server advertises the 'filter' capability, +fetch-pack may send "filter" commands to request a partial clone +or partial fetch and request that the server omit various objects +from the packfile. diff --git a/upload-pack.c b/upload-pack.c index e25f725..64a57a4 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -10,6 +10,8 @@ #include "diff.h" #include "revision.h" #include "list-objects.h" +#include "list-objects-filter.h" +#include "list-objects-filter-options.h" #include "run-command.h" #include "connect.h" #include "sigchain.h" @@ -64,6 +66,10 @@ static int advertise_refs; static int stateless_rpc; static const char *pack_objects_hook; +static int filter_capability_requested; +static int filter_advertise; +static struct list_objects_filter_options filter_options; + static void reset_timeout(void) { alarm(timeout); @@ -131,6 +137,7 @@ static void create_pack_file(void) argv_array_push(_objects.args, "--delta-base-offset"); if (use_include_tag) argv_array_push(_objects.args, "--include-tag"); + arg_format_list_objects_filter(_objects.args, _options); pack_objects.in = -1; pack_objects.out = -1; @@ -794,6 +801,12 @@ static void receive_needs(void) deepen_rev_list = 1; continue; } + if (skip_prefix(line, "filter ", )) { + if (!filter_capability_requested) + die("git
[PATCH 07/14] fetch-pack: test support excluding large blobs
From: Jonathan TanCreated tests to verify fetch-pack and upload-pack support for excluding large blobs using --filter=blobs:limit= parameter. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- t/t5500-fetch-pack.sh | 27 +++ 1 file changed, 27 insertions(+) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 80a1a32..fdb98a8 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -755,4 +755,31 @@ test_expect_success 'fetching deepen' ' ) ' +test_expect_success 'filtering by size' ' + rm -rf server client && + test_create_repo server && + test_commit -C server one && + test_config -C server uploadpack.allowfilter 1 && + + test_create_repo client && + git -C client fetch-pack --filter=blobs:limit=0 ../server HEAD && + + # Ensure that object is not inadvertently fetched + test_must_fail git -C client cat-file -e $(git hash-object server/one.t) +' + +test_expect_success 'filtering by size has no effect if support for it is not advertised' ' + rm -rf server client && + test_create_repo server && + test_commit -C server one && + + test_create_repo client && + git -C client fetch-pack --filter=blobs:limit=0 ../server HEAD 2> err && + + # Ensure that object is fetched + git -C client cat-file -e $(git hash-object server/one.t) && + + test_i18ngrep "filtering not recognized by server" err +' + test_done -- 2.9.3
[PATCH 03/14] fetch: refactor calculation of remote list
From: Jonathan TanSeparate out the calculation of remotes to be fetched from and the actual fetching. This will allow us to include an additional step before the actual fetching in a subsequent commit. Signed-off-by: Jonathan Tan --- builtin/fetch.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 225c734..1b1f039 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1322,7 +1322,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) { int i; struct string_list list = STRING_LIST_INIT_DUP; - struct remote *remote; + struct remote *remote = NULL; int result = 0; struct argv_array argv_gc_auto = ARGV_ARRAY_INIT; @@ -1367,17 +1367,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) else if (argc > 1) die(_("fetch --all does not make sense with refspecs")); (void) for_each_remote(get_one_remote_for_fetch, ); - result = fetch_multiple(); } else if (argc == 0) { /* No arguments -- use default remote */ remote = remote_get(NULL); - result = fetch_one(remote, argc, argv); } else if (multiple) { /* All arguments are assumed to be remotes or groups */ for (i = 0; i < argc; i++) if (!add_remote_or_group(argv[i], )) die(_("No such remote or remote group: %s"), argv[i]); - result = fetch_multiple(); } else { /* Single remote or group */ (void) add_remote_or_group(argv[0], ); @@ -1385,14 +1382,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) /* More than one remote */ if (argc > 1) die(_("Fetching a group and specifying refspecs does not make sense")); - result = fetch_multiple(); } else { /* Zero or one remotes */ remote = remote_get(argv[0]); - result = fetch_one(remote, argc-1, argv+1); + argc--; + argv++; } } + if (remote) + result = fetch_one(remote, argc, argv); + else + result = fetch_multiple(); + if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { struct argv_array options = ARGV_ARRAY_INIT; -- 2.9.3
[PATCH 13/14] fetch-pack: restore save_commit_buffer after use
From: Jonathan TanIn fetch-pack, the global variable save_commit_buffer is set to 0, but not restored to its original value after use. In particular, if show_log() (in log-tree.c) is invoked after fetch_pack() in the same process, show_log() will return before printing out the commit message (because the invocation to get_cached_commit_buffer() returns NULL, because the commit buffer was not saved). I discovered this when attempting to run "git log -S" in a partial clone, triggering the case where revision walking lazily loads missing objects. Therefore, restore save_commit_buffer to its original value after use. An alternative to solve the problem I had is to replace get_cached_commit_buffer() with get_commit_buffer(). That invocation was introduced in commit a97934d ("use get_cached_commit_buffer where appropriate", 2014-06-13) to replace "commit->buffer" introduced in commit 3131b71 ("Add "--show-all" revision walker flag for debugging", 2008-02-13). In the latter commit, the commit author seems to be deciding between not showing an unparsed commit at all and showing an unparsed commit without the message (which is what the commit does), and did not mention parsing the unparsed commit, so I prefer to preserve the existing behavior. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- fetch-pack.c | 4 1 file changed, 4 insertions(+) diff --git a/fetch-pack.c b/fetch-pack.c index 895e8f9..121f03e 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -717,6 +717,7 @@ static int everything_local(struct fetch_pack_args *args, { struct ref *ref; int retval; + int old_save_commit_buffer = save_commit_buffer; timestamp_t cutoff = 0; save_commit_buffer = 0; @@ -784,6 +785,9 @@ static int everything_local(struct fetch_pack_args *args, print_verbose(args, _("already have %s (%s)"), oid_to_hex(remote), ref->name); } + + save_commit_buffer = old_save_commit_buffer; + return retval; } -- 2.9.3
[PATCH 06/14] pack-objects: test support for blob filtering
From: Jonathan TanAs part of an effort to improve Git support for very large repositories in which clients typically have only a subset of all version-controlled blobs, test pack-objects support for --filter=blobs:limit=, packing only blobs not exceeding that size unless the blob corresponds to a file whose name starts with ".git". upload-pack will eventually be taught to use this new parameter if needed to exclude certain blobs during a fetch or clone, potentially drastically reducing network consumption when serving these very large repositories. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- t/t5300-pack-object.sh | 45 + t/test-lib-functions.sh | 12 2 files changed, 57 insertions(+) diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index 9c68b99..0739a07 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -457,6 +457,51 @@ test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'pack-objects --threads=N or pack. grep -F "no threads support, ignoring pack.threads" err ' +lcut () { + perl -e '$/ = undef; $_ = <>; s/^.{'$1'}//s; print $_' +} + +test_expect_success 'filtering by size works with multiple excluded' ' + rm -rf server && + git init server && + printf a > server/a && + printf b > server/b && + printf c-very-long-file > server/c && + printf d-very-long-file > server/d && + git -C server add a b c d && + git -C server commit -m x && + + git -C server rev-parse HEAD >objects && + git -C server pack-objects --revs --stdout --filter=blobs:limit=10 my.pack && + + # Ensure that only the small blobs are in the packfile + git index-pack my.pack && + git verify-pack -v my.idx >objectlist && + grep $(git hash-object server/a) objectlist && + grep $(git hash-object server/b) objectlist && + ! grep $(git hash-object server/c) objectlist && + ! grep $(git hash-object server/d) objectlist +' + +test_expect_success 'filtering by size never excludes special files' ' + rm -rf server && + git init server && + printf a-very-long-file > server/a && + printf a-very-long-file > server/.git-a && + printf b-very-long-file > server/b && + git -C server add a .git-a b && + git -C server commit -m x && + + git -C server rev-parse HEAD >objects && + git -C server pack-objects --revs --stdout --filter=blobs:limit=10 my.pack && + + # Ensure that the .git-a blob is in the packfile, despite also + # appearing as a non-.git file + git index-pack my.pack && + git verify-pack -v my.idx >objectlist && + grep $(git hash-object server/a) objectlist +' + # # WARNING! # diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 1701fe2..07b79c7 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1020,3 +1020,15 @@ nongit () { "$@" ) } + +# Converts big-endian pairs of hexadecimal digits into bytes. For example, +# "printf 61620d0a | hex_pack" results in "ab\r\n". +hex_pack () { + perl -e '$/ = undef; $input = <>; print pack("H*", $input)' +} + +# Converts bytes into big-endian pairs of hexadecimal digits. For example, +# "printf 'ab\r\n' | hex_unpack" results in "61620d0a". +hex_unpack () { + perl -e '$/ = undef; $input = <>; print unpack("H2" x length($input), $input)' +} -- 2.9.3
[PATCH 09/14] t5500: add fetch-pack tests for partial clone
From: Jonathan TanSigned-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- t/t5500-fetch-pack.sh | 36 1 file changed, 36 insertions(+) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index fdb98a8..7c8339f 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -782,4 +782,40 @@ test_expect_success 'filtering by size has no effect if support for it is not ad test_i18ngrep "filtering not recognized by server" err ' +fetch_blob_max_bytes () { + SERVER="$1" + URL="$2" + + rm -rf "$SERVER" client && + test_create_repo "$SERVER" && + test_commit -C "$SERVER" one && + test_config -C "$SERVER" uploadpack.allowfilter 1 && + + git clone "$URL" client && + test_config -C client extensions.partialcloneremote origin && + + test_commit -C "$SERVER" two && + + git -C client fetch --filter=blobs:limit=0 origin HEAD:somewhere && + + # Ensure that commit is fetched, but blob is not + test_config -C client extensions.partialcloneremote "arbitrary string" && + git -C client cat-file -e $(git -C "$SERVER" rev-parse two) && + test_must_fail git -C client cat-file -e $(git hash-object "$SERVER/two.t") +} + +test_expect_success 'fetch with filtering' ' +fetch_blob_max_bytes server server +' + +. "$TEST_DIRECTORY"/lib-httpd.sh +start_httpd + +test_expect_success 'fetch with filtering and HTTP' ' +fetch_blob_max_bytes "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server" +' + +stop_httpd + + test_done -- 2.9.3
[PATCH 12/14] unpack-trees: batch fetching of missing blobs
From: Jonathan TanWhen running checkout, first prefetch all blobs that are to be updated but are missing. This means that only one pack is downloaded during such operations, instead of one per missing blob. This operates only on the blob level - if a repository has a missing tree, they are still fetched one at a time. This does not use the delayed checkout mechanism introduced in commit 2841e8f ("convert: add "status=delayed" to filter process protocol", 2017-06-30) due to significant conceptual differences - in particular, for partial clones, we already know what needs to be fetched based on the contents of the local repo alone, whereas for status=delayed, it is the filter process that tells us what needs to be checked in the end. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- fetch-object.c | 27 +++ fetch-object.h | 5 + t/t5601-clone.sh | 52 unpack-trees.c | 22 ++ 4 files changed, 102 insertions(+), 4 deletions(-) diff --git a/fetch-object.c b/fetch-object.c index 369b61c..21b4dfa 100644 --- a/fetch-object.c +++ b/fetch-object.c @@ -3,12 +3,12 @@ #include "pkt-line.h" #include "strbuf.h" #include "transport.h" +#include "fetch-object.h" -void fetch_object(const char *remote_name, const unsigned char *sha1) +static void fetch_refs(const char *remote_name, struct ref *ref) { struct remote *remote; struct transport *transport; - struct ref *ref; int original_fetch_if_missing = fetch_if_missing; fetch_if_missing = 0; @@ -17,10 +17,29 @@ void fetch_object(const char *remote_name, const unsigned char *sha1) die(_("Remote with no URL")); transport = transport_get(remote, remote->url[0]); - ref = alloc_ref(sha1_to_hex(sha1)); - hashcpy(ref->old_oid.hash, sha1); transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1"); transport_set_option(transport, TRANS_OPT_NO_HAVES, "1"); transport_fetch_refs(transport, ref); fetch_if_missing = original_fetch_if_missing; } + +void fetch_object(const char *remote_name, const unsigned char *sha1) +{ + struct ref *ref = alloc_ref(sha1_to_hex(sha1)); + hashcpy(ref->old_oid.hash, sha1); + fetch_refs(remote_name, ref); +} + +void fetch_objects(const char *remote_name, const struct oid_array *to_fetch) +{ + struct ref *ref = NULL; + int i; + + for (i = 0; i < to_fetch->nr; i++) { + struct ref *new_ref = alloc_ref(oid_to_hex(_fetch->oid[i])); + oidcpy(_ref->old_oid, _fetch->oid[i]); + new_ref->next = ref; + ref = new_ref; + } + fetch_refs(remote_name, ref); +} diff --git a/fetch-object.h b/fetch-object.h index f371300..4b269d0 100644 --- a/fetch-object.h +++ b/fetch-object.h @@ -1,6 +1,11 @@ #ifndef FETCH_OBJECT_H #define FETCH_OBJECT_H +#include "sha1-array.h" + extern void fetch_object(const char *remote_name, const unsigned char *sha1); +extern void fetch_objects(const char *remote_name, + const struct oid_array *to_fetch); + #endif diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 567161e..3211f86 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -611,6 +611,58 @@ test_expect_success 'partial clone: warn if server does not support object filte test_i18ngrep "filtering not recognized by server" err ' +test_expect_success 'batch missing blob request during checkout' ' + rm -rf server client && + + test_create_repo server && + echo a >server/a && + echo b >server/b && + git -C server add a b && + + git -C server commit -m x && + echo aa >server/a && + echo bb >server/b && + git -C server add a b && + git -C server commit -m x && + + test_config -C server uploadpack.allowfilter 1 && + test_config -C server uploadpack.allowanysha1inwant 1 && + + git clone --filter=blobs:limit=0 "file://$(pwd)/server" client && + + # Ensure that there is only one negotiation by checking that there is + # only "done" line sent. ("done" marks the end of negotiation.) + GIT_TRACE_PACKET="$(pwd)/trace" git -C client checkout HEAD^ && + grep "git> done" trace >done_lines && + test_line_count = 1 done_lines +' + +test_expect_success 'batch missing blob request does not inadvertently try to fetch gitlinks' ' + rm -rf server client && + + test_create_repo repo_for_submodule && + test_commit -C repo_for_submodule x && + + test_create_repo server && + echo a >server/a && + echo b >server/b && + git -C server add a b && + git -C server commit -m x && + + echo aa >server/a && + echo bb >server/b && + # Also add a gitlink pointing to an arbitrary repository
[PATCH 14/14] index-pack: silently assume missing objects are promisor
From: Jeff HostetlerTeach index-pack to not complain about missing objects when the --promisor flag is given. The assumption is that index-pack is currently building the idx and promisor data and the is_promisor_object() query would fail anyway. Signed-off-by: Jeff Hostetler --- builtin/index-pack.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 31cd5ba..51693dc 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -82,6 +82,7 @@ static int verbose; static int show_resolving_progress; static int show_stat; static int check_self_contained_and_connected; +static int arg_promisor_given; static struct progress *progress; @@ -223,10 +224,11 @@ static unsigned check_object(struct object *obj) unsigned long size; int type = sha1_object_info(obj->oid.hash, ); - if (type <= 0) { + if (type <= 0 && arg_promisor_given) { /* -* TODO Use the promisor code to conditionally -* try to fetch this object -or- assume it is ok. +* Assume this missing object is promised. We can't +* confirm it because we are indexing the packfile +* that omitted it. */ obj->flags |= FLAG_CHECKED; return 0; @@ -1717,8 +1719,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) keep_msg = arg + 7; } else if (!strcmp(arg, "--promisor")) { promisor_msg = ""; + arg_promisor_given = 1; } else if (starts_with(arg, "--promisor=")) { promisor_msg = arg + strlen("--promisor="); + arg_promisor_given = 1; } else if (starts_with(arg, "--threads=")) { char *end; nr_threads = strtoul(arg+10, , 0); -- 2.9.3
[PATCH 11/14] t5500: more tests for partial clone and fetch
From: Jonathan TanSigned-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- t/t5500-fetch-pack.sh | 60 +++ 1 file changed, 56 insertions(+), 4 deletions(-) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 7c8339f..86cf653 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -782,7 +782,7 @@ test_expect_success 'filtering by size has no effect if support for it is not ad test_i18ngrep "filtering not recognized by server" err ' -fetch_blob_max_bytes () { +setup_blob_max_bytes () { SERVER="$1" URL="$2" @@ -794,7 +794,11 @@ fetch_blob_max_bytes () { git clone "$URL" client && test_config -C client extensions.partialcloneremote origin && - test_commit -C "$SERVER" two && + test_commit -C "$SERVER" two +} + +do_blob_max_bytes() { + SERVER="$1" && git -C client fetch --filter=blobs:limit=0 origin HEAD:somewhere && @@ -805,14 +809,62 @@ fetch_blob_max_bytes () { } test_expect_success 'fetch with filtering' ' -fetch_blob_max_bytes server server + setup_blob_max_bytes server server && + do_blob_max_bytes server +' + +test_expect_success 'fetch respects configured filtering' ' + setup_blob_max_bytes server server && + + test_config -C client extensions.partialclonefilter blobs:limit=0 && + + git -C client fetch origin HEAD:somewhere && + + # Ensure that commit is fetched, but blob is not + test_config -C client extensions.partialcloneremote "arbitrary string" && + git -C client cat-file -e $(git -C server rev-parse two) && + test_must_fail git -C client cat-file -e $(git hash-object server/two.t) +' + +test_expect_success 'pull respects configured filtering' ' + setup_blob_max_bytes server server && + + # Hide two.t from tip so that client does not load it upon the + # automatic checkout that pull performs + git -C server rm two.t && + test_commit -C server three && + + test_config -C server uploadpack.allowanysha1inwant 1 && + test_config -C client extensions.partialclonefilter blobs:limit=0 && + + git -C client pull origin && + + # Ensure that commit is fetched, but blob is not + test_config -C client extensions.partialcloneremote "arbitrary string" && + git -C client cat-file -e $(git -C server rev-parse two) && + test_must_fail git -C client cat-file -e $(git hash-object server/two.t) +' + +test_expect_success 'clone configures filtering' ' + rm -rf server client && + test_create_repo server && + test_commit -C server one && + test_commit -C server two && + test_config -C server uploadpack.allowanysha1inwant 1 && + + git clone --filter=blobs:limit=12345 server client && + + # Ensure that we can, for example, checkout HEAD^ + rm -rf client/.git/objects/* && + git -C client checkout HEAD^ ' . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd test_expect_success 'fetch with filtering and HTTP' ' -fetch_blob_max_bytes "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server" + setup_blob_max_bytes "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server" && + do_blob_max_bytes "$HTTPD_DOCUMENT_ROOT_PATH/server" ' stop_httpd -- 2.9.3
[PATCH 00/14] WIP Partial clone part 3: clone, fetch, fetch-pack, upload-pack, and tests
From: Jeff HostetlerThis is part 3 of 3 for partial clone. It assumes that part 1 [1] and part 2 [2] are in place. Part 3 is concerned with the commands: clone, fetch, upload-pack, fetch-pack, remote-curl, index-pack, and the pack-protocol. Jonathan and I independently started on this task. This is a first pass at merging those efforts. So there are several places that need refactoring and cleanup. In particular, the test cases should be squashed and new tests added. [1] https://public-inbox.org/git/20171102124445.fbffd43521cd35f6a71e1...@google.com/T/ [2] TODO Jeff Hostetler (5): upload-pack: add object filtering for partial clone clone, fetch-pack, index-pack, transport: partial clone fetch: add object filtering for partial fetch remote-curl: add object filtering for partial clone index-pack: silently assume missing objects are promisor Jonathan Tan (9): fetch: refactor calculation of remote list pack-objects: test support for blob filtering fetch-pack: test support excluding large blobs fetch: add from_promisor and exclude-promisor-objects parameters t5500: add fetch-pack tests for partial clone t5601: test for partial clone t5500: more tests for partial clone and fetch unpack-trees: batch fetching of missing blobs fetch-pack: restore save_commit_buffer after use Documentation/config.txt | 4 + Documentation/gitremote-helpers.txt | 4 + Documentation/technical/pack-protocol.txt | 8 ++ Documentation/technical/protocol-capabilities.txt | 8 ++ builtin/clone.c | 24 - builtin/fetch-pack.c | 4 + builtin/fetch.c | 83 ++-- builtin/index-pack.c | 14 +++ connected.c | 3 + fetch-object.c| 27 - fetch-object.h| 5 + fetch-pack.c | 17 fetch-pack.h | 2 + remote-curl.c | 10 +- t/t5300-pack-object.sh| 45 + t/t5500-fetch-pack.sh | 115 ++ t/t5601-clone.sh | 101 +++ t/test-lib-functions.sh | 12 +++ transport-helper.c| 5 + transport.c | 4 + transport.h | 5 + unpack-trees.c| 22 + upload-pack.c | 20 +++- 23 files changed, 526 insertions(+), 16 deletions(-) -- 2.9.3
[PATCH 02/14] clone, fetch-pack, index-pack, transport: partial clone
From: Jeff HostetlerSigned-off-by: Jeff Hostetler --- builtin/clone.c | 9 + builtin/fetch-pack.c | 4 builtin/index-pack.c | 10 ++ fetch-pack.c | 13 + fetch-pack.h | 2 ++ transport-helper.c | 5 + transport.c | 4 transport.h | 5 + 8 files changed, 52 insertions(+) diff --git a/builtin/clone.c b/builtin/clone.c index dbddd98..fceb9e7 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -26,6 +26,7 @@ #include "run-command.h" #include "connected.h" #include "packfile.h" +#include "list-objects-filter-options.h" /* * Overall FIXMEs: @@ -60,6 +61,7 @@ static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP; static int option_dissociate; static int max_jobs = -1; static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP; +static struct list_objects_filter_options filter_options; static int recurse_submodules_cb(const struct option *opt, const char *arg, int unset) @@ -135,6 +137,7 @@ static struct option builtin_clone_options[] = { TRANSPORT_FAMILY_IPV4), OPT_SET_INT('6', "ipv6", , N_("use IPv6 addresses only"), TRANSPORT_FAMILY_IPV6), + OPT_PARSE_LIST_OBJECTS_FILTER(_options), OPT_END() }; @@ -1073,6 +1076,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix) warning(_("--shallow-since is ignored in local clones; use file:// instead.")); if (option_not.nr) warning(_("--shallow-exclude is ignored in local clones; use file:// instead.")); + if (filter_options.choice) + warning(_("--filter is ignored in local clones; use file:// instead.")); if (!access(mkpath("%s/shallow", path), F_OK)) { if (option_local > 0) warning(_("source repository is shallow, ignoring --local")); @@ -1104,6 +1109,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix) transport_set_option(transport, TRANS_OPT_UPLOADPACK, option_upload_pack); + if (filter_options.choice) + transport_set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER, +filter_options.raw_value); + if (transport->smart_options && !deepen) transport->smart_options->check_self_contained_and_connected = 1; diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 9a7ebf6..d0fdaa8 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -153,6 +153,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) args.no_haves = 1; continue; } + if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), )) { + parse_list_objects_filter(_options, arg); + continue; + } usage(fetch_pack_usage); } if (deepen_not.nr) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index a0a35e6..31cd5ba 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -222,6 +222,16 @@ static unsigned check_object(struct object *obj) if (!(obj->flags & FLAG_CHECKED)) { unsigned long size; int type = sha1_object_info(obj->oid.hash, ); + + if (type <= 0) { + /* +* TODO Use the promisor code to conditionally +* try to fetch this object -or- assume it is ok. +*/ + obj->flags |= FLAG_CHECKED; + return 0; + } + if (type <= 0) die(_("did not receive expected object %s"), oid_to_hex(>oid)); diff --git a/fetch-pack.c b/fetch-pack.c index 4640b4e..895e8f9 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -29,6 +29,7 @@ static int deepen_not_ok; static int fetch_fsck_objects = -1; static int transfer_fsck_objects = -1; static int agent_supported; +static int server_supports_filtering; static struct lock_file shallow_lock; static const char *alternate_shallow_file; @@ -379,6 +380,8 @@ static int find_common(struct fetch_pack_args *args, if (deepen_not_ok) strbuf_addstr(, " deepen-not"); if (agent_supported)strbuf_addf(, " agent=%s", git_user_agent_sanitized()); + if (args->filter_options.choice) + strbuf_addstr(, " filter"); packet_buf_write(_buf, "want %s%s\n", remote_hex, c.buf);
[PATCH 04/14] fetch: add object filtering for partial fetch
From: Jeff HostetlerTeach fetch to use the list-objects filtering parameters to allow a "partial fetch" following a "partial clone". Signed-off-by: Jeff Hostetler --- builtin/fetch.c | 66 - 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 1b1f039..150ca0a 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -18,6 +18,7 @@ #include "argv-array.h" #include "utf8.h" #include "packfile.h" +#include "list-objects-filter-options.h" static const char * const builtin_fetch_usage[] = { N_("git fetch [] [ [...]]"), @@ -55,6 +56,7 @@ static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND; static int shown_url = 0; static int refmap_alloc, refmap_nr; static const char **refmap_array; +static struct list_objects_filter_options filter_options; static int git_fetch_config(const char *k, const char *v, void *cb) { @@ -160,6 +162,7 @@ static struct option builtin_fetch_options[] = { TRANSPORT_FAMILY_IPV4), OPT_SET_INT('6', "ipv6", , N_("use IPv6 addresses only"), TRANSPORT_FAMILY_IPV6), + OPT_PARSE_LIST_OBJECTS_FILTER(_options), OPT_END() }; @@ -754,6 +757,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, const char *filename = dry_run ? "/dev/null" : git_path_fetch_head(); int want_status; int summary_width = transport_summary_width(ref_map); + struct check_connected_options opt = CHECK_CONNECTED_INIT; fp = fopen(filename, "a"); if (!fp) @@ -765,7 +769,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, url = xstrdup("foreign"); rm = ref_map; - if (check_connected(iterate_ref_map, , NULL)) { + if (check_connected(iterate_ref_map, , )) { rc = error(_("%s did not send all necessary objects\n"), url); goto abort; } @@ -1044,6 +1048,9 @@ static struct transport *prepare_transport(struct remote *remote, int deepen) set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, "yes"); if (update_shallow) set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes"); + if (filter_options.choice) + set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER, + filter_options.raw_value); return transport; } @@ -1242,6 +1249,20 @@ static int fetch_multiple(struct string_list *list) int i, result = 0; struct argv_array argv = ARGV_ARRAY_INIT; + if (filter_options.choice) { + /* +* We currently only support partial-fetches to the remote +* used for the partial-clone because we only support 1 +* promisor remote, so we DO NOT allow explicit command +* line filter arguments. +* +* Note that the loop below will spawn background fetches +* for each remote and one of them MAY INHERIT the proper +* partial-fetch settings, so everything is consistent. +*/ + die(_("partial-fetch is not supported on multiple remotes")); + } + if (!append && !dry_run) { int errcode = truncate_fetch_head(); if (errcode) @@ -1267,6 +1288,46 @@ static int fetch_multiple(struct string_list *list) return result; } +static inline void partial_fetch_one_setup(struct remote *remote) +{ +#if 0 /* TODO */ + if (filter_options.choice) { + /* +* A partial-fetch was explicitly requested. +* +* If this is the first partial-* command on +* this repo, we must register the partial +* settings in the repository extension. +* +* If this follows a previous partial-* command +* we must ensure the args are consistent with +* the existing registration (because we don't +* currently support mixing-and-matching). +*/ + partial_clone_utils_register(_options, +remote->name, "fetch"); + return; + } + + if (is_partial_clone_registered() && + !strcmp(remote->name, repository_format_partial_clone_remote)) { + /* +* If a partial-* command has already been used on +* this repo and it was to this remote, we should +* inherit the filter settings used previously. +* That is, if clone omitted very large blobs, then +* fetch should too. +* +* Use the cached filter-spec and create the filter +* settings. +
[PATCH 08/14] fetch: add from_promisor and exclude-promisor-objects parameters
From: Jonathan TanTeach fetch to use from-promisor and exclude-promisor-objects parameters with sub-commands. Initialize fetch_if_missing global variable. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- builtin/fetch.c | 9 ++--- connected.c | 3 +++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 150ca0a..ab53df3 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -19,6 +19,7 @@ #include "utf8.h" #include "packfile.h" #include "list-objects-filter-options.h" +#include "partial-clone-utils.h" static const char * const builtin_fetch_usage[] = { N_("git fetch [] [ [...]]"), @@ -1048,9 +1049,11 @@ static struct transport *prepare_transport(struct remote *remote, int deepen) set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, "yes"); if (update_shallow) set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes"); - if (filter_options.choice) + if (filter_options.choice) { set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER, filter_options.raw_value); + set_option(transport, TRANS_OPT_FROM_PROMISOR, "1"); + } return transport; } @@ -1290,7 +1293,6 @@ static int fetch_multiple(struct string_list *list) static inline void partial_fetch_one_setup(struct remote *remote) { -#if 0 /* TODO */ if (filter_options.choice) { /* * A partial-fetch was explicitly requested. @@ -1325,7 +1327,6 @@ static inline void partial_fetch_one_setup(struct remote *remote) _options, repository_format_partial_clone_filter); } -#endif } static int fetch_one(struct remote *remote, int argc, const char **argv) @@ -1392,6 +1393,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) packet_trace_identity("fetch"); + fetch_if_missing = 0; + /* Record the command line for the reflog */ strbuf_addstr(_rla, "fetch"); for (i = 1; i < argc; i++) diff --git a/connected.c b/connected.c index f416b05..6015316 100644 --- a/connected.c +++ b/connected.c @@ -4,6 +4,7 @@ #include "connected.h" #include "transport.h" #include "packfile.h" +#include "partial-clone-utils.h" /* * If we feed all the commits we want to verify to this command @@ -56,6 +57,8 @@ int check_connected(sha1_iterate_fn fn, void *cb_data, argv_array_push(_list.args,"rev-list"); argv_array_push(_list.args, "--objects"); argv_array_push(_list.args, "--stdin"); + if (is_partial_clone_registered()) + argv_array_push(_list.args, "--exclude-promisor-objects"); argv_array_push(_list.args, "--not"); argv_array_push(_list.args, "--all"); argv_array_push(_list.args, "--quiet"); -- 2.9.3
[PATCH 10/14] t5601: test for partial clone
From: Jonathan TanSigned-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- builtin/clone.c | 17 ++--- t/t5601-clone.sh | 49 + 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index fceb9e7..08315d8 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -27,6 +27,7 @@ #include "connected.h" #include "packfile.h" #include "list-objects-filter-options.h" +#include "partial-clone-utils.h" /* * Overall FIXMEs: @@ -889,6 +890,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix) struct refspec *refspec; const char *fetch_pattern; + fetch_if_missing = 0; + packet_trace_identity("clone"); argc = parse_options(argc, argv, prefix, builtin_clone_options, builtin_clone_usage, 0); @@ -1109,11 +1112,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix) transport_set_option(transport, TRANS_OPT_UPLOADPACK, option_upload_pack); - if (filter_options.choice) + if (filter_options.choice) { transport_set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER, filter_options.raw_value); + transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1"); + } - if (transport->smart_options && !deepen) + if (transport->smart_options && !deepen && !filter_options.choice) transport->smart_options->check_self_contained_and_connected = 1; refs = transport_get_remote_refs(transport); @@ -1173,13 +1178,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix) write_refspec_config(src_ref_prefix, our_head_points_at, remote_head_points_at, _top); + if (filter_options.choice) + partial_clone_utils_register(_options, "origin", +"clone"); + if (is_local) clone_local(path, git_dir); else if (refs && complete_refs_before_fetch) transport_fetch_refs(transport, mapped_refs); update_remote_refs(refs, mapped_refs, remote_head_points_at, - branch_top.buf, reflog_msg.buf, transport, !is_local); + branch_top.buf, reflog_msg.buf, transport, + !is_local && !filter_options.choice); update_head(our_head_points_at, remote_head, reflog_msg.buf); @@ -1200,6 +1210,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) } junk_mode = JUNK_LEAVE_REPO; + fetch_if_missing = 1; err = checkout(submodule_progress); strbuf_release(_msg); diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 9c56f77..567161e 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -571,4 +571,53 @@ test_expect_success 'GIT_TRACE_PACKFILE produces a usable pack' ' git -C replay.git index-pack -v --stdin err && + + test_i18ngrep "filtering not recognized by server" err +' + +. "$TEST_DIRECTORY"/lib-httpd.sh +start_httpd + +test_expect_success 'partial clone using HTTP' ' +partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server" +' + +stop_httpd + test_done -- 2.9.3
[PATCH 05/14] remote-curl: add object filtering for partial clone
From: Jeff HostetlerSigned-off-by: Jeff Hostetler --- Documentation/gitremote-helpers.txt | 4 remote-curl.c | 10 -- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index 6da3f41..d46d561 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -468,6 +468,10 @@ set by Git if the remote helper has the 'option' capability. TODO document 'option from-promisor' and 'option no-haves' ? +'option filter ':: + An object filter specification for partial clone or fetch + as described in rev-list. + SEE ALSO linkgit:git-remote[1] diff --git a/remote-curl.c b/remote-curl.c index 41e8a42..840f3ce 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -13,6 +13,7 @@ #include "credential.h" #include "sha1-array.h" #include "send-pack.h" +#include "list-objects-filter-options.h" static struct remote *remote; /* always ends with a trailing slash */ @@ -22,6 +23,7 @@ struct options { int verbosity; unsigned long depth; char *deepen_since; + char *partial_clone_filter; struct string_list deepen_not; struct string_list push_options; unsigned progress : 1, @@ -163,11 +165,12 @@ static int set_option(const char *name, const char *value) } else if (!strcmp(name, "from-promisor")) { options.from_promisor = 1; return 0; - } else if (!strcmp(name, "no-haves")) { options.no_haves = 1; return 0; - + } else if (!strcmp(name, "filter")) { + options.partial_clone_filter = xstrdup(value); + return 0; } else { return 1 /* unsupported */; } @@ -837,6 +840,9 @@ static int fetch_git(struct discovery *heads, argv_array_push(, "--from-promisor"); if (options.no_haves) argv_array_push(, "--no-haves"); + if (options.partial_clone_filter) + argv_array_pushf(, "--%s=%s", +CL_ARG__FILTER, options.partial_clone_filter); argv_array_push(, url.buf); for (i = 0; i < nr_heads; i++) { -- 2.9.3
Re: No log --no-decorate completion?
No problem! Let me know if I've done something wrong with this patch, I'm new to git's contributor process. completion: add missing completions for log, diff, show * Add bash completion for the missing --no-* options on git log * Add bash completion for --textconv and --indent-heuristic families to git diff and all commands that use diff's options * Add bash completion for --no-abbrev-commit, --expand-tabs, and --no-expand-tabs to git show --- contrib/completion/git-completion.bash | 18 ++ 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 0e16f017a4144..252a6c8c0c80c 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1412,6 +1412,8 @@ __git_diff_common_options="--stat --numstat --shortstat --summary --dirstat-by-file= --cumulative --diff-algorithm= --submodule --submodule= + --indent-heuristic --no-indent-heuristic + --textconv --no-textconv " _git_diff () @@ -1752,6 +1754,10 @@ _git_log () __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}" return ;; + --no-walk=*) + __gitcomp "sorted unsorted" "" "${cur##--no-walk=}" + return + ;; --*) __gitcomp " $__git_log_common_options @@ -1759,16 +1765,19 @@ _git_log () $__git_log_gitk_options --root --topo-order --date-order --reverse --follow --full-diff - --abbrev-commit --abbrev= + --abbrev-commit --no-abbrev-commit --abbrev= --relative-date --date= --pretty= --format= --oneline --show-signature --cherry-mark --cherry-pick --graph - --decorate --decorate= + --decorate --decorate= --no-decorate --walk-reflogs + --no-walk --no-walk= --do-walk --parents --children + --expand-tabs --expand-tabs= --no-expand-tabs + --patch $merge $__git_diff_common_options --pickaxe-all --pickaxe-regex @@ -2816,8 +2825,9 @@ _git_show () return ;; --*) - __gitcomp "--pretty= --format= --abbrev-commit --oneline - --show-signature + __gitcomp "--pretty= --format= --abbrev-commit --no-abbrev-commit + --oneline --show-signature --patch + --expand-tabs --expand-tabs= --no-expand-tabs $__git_diff_common_options " return On Tue, Oct 24, 2017 at 11:32 AM, Stefan Bellerwrote: > On Tue, Oct 24, 2017 at 8:15 AM, Max Rothman wrote: >> Just re-discovered this in my inbox. So is this worth fixing? I could >> (probably) figure out a patch. >> >> Thanks, >> Max >> > > $ git log -- > Display all 100 possibilities? (y or n) > > I guess adding one more option is no big deal, so go for it! > We also have a couple --no-options already (as you said earlier), > which I think makes it even more viable. > > Tangent: > I wonder if we can cascade the completion by common > prefixes, e.g. --no- or --ignore- occurs a couple of times, > such that we could only show these --no- once and only > if you try autocompleting thereafter you get all --no- options. > > Thanks for reviving this thread! > Stefan
[PATCH 8/9] sha1_file: support lazily fetching missing objects
From: Jonathan TanTeach sha1_file to fetch objects from the remote configured in extensions.partialcloneremote whenever an object is requested but missing. The fetching of objects can be suppressed through a global variable. This is used by fsck and index-pack. However, by default, such fetching is not suppressed. This is meant as a temporary measure to ensure that all Git commands work in such a situation. Future patches will update some commands to either tolerate missing objects (without fetching them) or be more efficient in fetching them. In order to determine the code changes in sha1_file.c necessary, I investigated the following: (1) functions in sha1_file.c that take in a hash, without the user regarding how the object is stored (loose or packed) (2) functions in packfile.c (because I need to check callers that know about the loose/packed distinction and operate on both differently, and ensure that they can handle the concept of objects that are neither loose nor packed) (1) is handled by the modification to sha1_object_info_extended(). For (2), I looked at for_each_packed_object and others. For for_each_packed_object, the callers either already work or are fixed in this patch: - reachable - only to find recent objects - builtin/fsck - already knows about missing objects - builtin/cat-file - warning message added in this commit Callers of the other functions do not need to be changed: - parse_pack_index - http - indirectly from http_get_info_packs - find_pack_entry_one - this searches a single pack that is provided as an argument; the caller already knows (through other means) that the sought object is in a specific pack - find_sha1_pack - fast-import - appears to be an optimization to not store a file if it is already in a pack - http-walker - to search through a struct alt_base - http-push - to search through remote packs - has_sha1_pack - builtin/fsck - already knows about promisor objects - builtin/count-objects - informational purposes only (check if loose object is also packed) - builtin/prune-packed - check if object to be pruned is packed (if not, don't prune it) - revision - used to exclude packed objects if requested by user - diff - just for optimization Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- builtin/cat-file.c | 3 + builtin/fetch-pack.c | 2 + builtin/fsck.c | 3 + builtin/index-pack.c | 6 ++ builtin/rev-list.c | 35 +-- cache.h | 8 +++ fetch-object.c | 3 + list-objects.c | 8 ++- object.c | 2 +- revision.c | 32 +- revision.h | 5 +- sha1_file.c | 39 t/t0410-partial-clone.sh | 152 +++ 13 files changed, 277 insertions(+), 21 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index f5fa4fd..ba77b73 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -13,6 +13,7 @@ #include "tree-walk.h" #include "sha1-array.h" #include "packfile.h" +#include "partial-clone-utils.h" struct batch_options { int enabled; @@ -475,6 +476,8 @@ static int batch_objects(struct batch_options *opt) for_each_loose_object(batch_loose_object, , 0); for_each_packed_object(batch_packed_object, , 0); + if (is_partial_clone_registered()) + warning("This repository has partial clone enabled. Some objects may not be loaded."); cb.opt = opt; cb.expand = diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 9f303cf..9a7ebf6 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -53,6 +53,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) struct oid_array shallow = OID_ARRAY_INIT; struct string_list deepen_not = STRING_LIST_INIT_DUP; + fetch_if_missing = 0; + packet_trace_identity("fetch-pack"); memset(, 0, sizeof(args)); diff --git a/builtin/fsck.c b/builtin/fsck.c index 578a7c8..3b76c0e 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -678,6 +678,9 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) int i; struct alternate_object_database *alt; + /* fsck knows how to handle missing promisor objects */ + fetch_if_missing = 0; + errors_found = 0; check_replace_refs = 0; diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 24c2f05..a0a35e6 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1657,6 +1657,12 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) unsigned foreign_nr = 1;/* zero is a "good" value, assume bad */ int report_end_of_input = 0; +
[PATCH 1/9] extension.partialclone: introduce partial clone extension
From: Jeff HostetlerIntroduce the ability to have missing objects in a repo. This functionality is guarded by new repository extension options: `extensions.partialcloneremote` and `extensions.partialclonefilter`. See the update to Documentation/technical/repository-version.txt in this patch for more information. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- Documentation/technical/repository-version.txt | 22 Makefile | 1 + cache.h| 4 ++ config.h | 3 + environment.c | 2 + partial-clone-utils.c | 78 ++ partial-clone-utils.h | 34 +++ setup.c| 15 + 8 files changed, 159 insertions(+) create mode 100644 partial-clone-utils.c create mode 100644 partial-clone-utils.h diff --git a/Documentation/technical/repository-version.txt b/Documentation/technical/repository-version.txt index 00ad379..9d488db 100644 --- a/Documentation/technical/repository-version.txt +++ b/Documentation/technical/repository-version.txt @@ -86,3 +86,25 @@ for testing format-1 compatibility. When the config key `extensions.preciousObjects` is set to `true`, objects in the repository MUST NOT be deleted (e.g., by `git-prune` or `git repack -d`). + +`partialcloneremote` + + +When the config key `extensions.partialcloneremote` is set, it indicates +that the repo was created with a partial clone (or later performed +a partial fetch) and that the remote may have omitted sending +certain unwanted objects. Such a remote is called a "promisor remote" +and it promises that all such omitted objects can be fetched from it +in the future. + +The value of this key is the name of the promisor remote. + +`partialclonefilter` + + +When the config key `extensions.partialclonefilter` is set, it gives +the initial filter expression used to create the partial clone. +This value becomed the default filter expression for subsequent +fetches (called "partial fetches") from the promisor remote. This +value may also be set by the first explicit partial fetch following a +normal clone. diff --git a/Makefile b/Makefile index ca378a4..12d141a 100644 --- a/Makefile +++ b/Makefile @@ -838,6 +838,7 @@ LIB_OBJS += pack-write.o LIB_OBJS += pager.o LIB_OBJS += parse-options.o LIB_OBJS += parse-options-cb.o +LIB_OBJS += partial-clone-utils.o LIB_OBJS += patch-delta.o LIB_OBJS += patch-ids.o LIB_OBJS += path.o diff --git a/cache.h b/cache.h index 6440e2b..4b785c0 100644 --- a/cache.h +++ b/cache.h @@ -860,12 +860,16 @@ extern int grafts_replace_parents; #define GIT_REPO_VERSION 0 #define GIT_REPO_VERSION_READ 1 extern int repository_format_precious_objects; +extern char *repository_format_partial_clone_remote; +extern char *repository_format_partial_clone_filter; struct repository_format { int version; int precious_objects; int is_bare; char *work_tree; + char *partial_clone_remote; /* value of extensions.partialcloneremote */ + char *partial_clone_filter; /* value of extensions.partialclonefilter */ struct string_list unknown_extensions; }; diff --git a/config.h b/config.h index a49d264..90544ef 100644 --- a/config.h +++ b/config.h @@ -34,6 +34,9 @@ struct config_options { const char *git_dir; }; +#define KEY_PARTIALCLONEREMOTE "partialcloneremote" +#define KEY_PARTIALCLONEFILTER "partialclonefilter" + typedef int (*config_fn_t)(const char *, const char *, void *); extern int git_default_config(const char *, const char *, void *); extern int git_config_from_file(config_fn_t fn, const char *, void *); diff --git a/environment.c b/environment.c index 8289c25..2fcf9bb 100644 --- a/environment.c +++ b/environment.c @@ -27,6 +27,8 @@ int warn_ambiguous_refs = 1; int warn_on_object_refname_ambiguity = 1; int ref_paranoia = -1; int repository_format_precious_objects; +char *repository_format_partial_clone_remote; +char *repository_format_partial_clone_filter; const char *git_commit_encoding; const char *git_log_output_encoding; const char *apply_default_whitespace; diff --git a/partial-clone-utils.c b/partial-clone-utils.c new file mode 100644 index 000..32cc20d --- /dev/null +++ b/partial-clone-utils.c @@ -0,0 +1,78 @@ +#include "cache.h" +#include "config.h" +#include "partial-clone-utils.h" + +int is_partial_clone_registered(void) +{ + if (repository_format_partial_clone_remote || + repository_format_partial_clone_filter) + return 1; + + return 0; +} + +void partial_clone_utils_register( + const struct list_objects_filter_options *filter_options, + const char *remote, + const char *cmd_name) +{ +
[PATCH 6/9] index-pack: refactor writing of .keep files
From: Jonathan TanIn a subsequent commit, index-pack will be taught to write ".promisor" files which are similar to the ".keep" files it knows how to write. Refactor the writing of ".keep" files, so that the implementation of writing ".promisor" files becomes easier. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- builtin/index-pack.c | 99 1 file changed, 53 insertions(+), 46 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 8ec459f..4f305a7 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1389,15 +1389,58 @@ static void fix_unresolved_deltas(struct sha1file *f) free(sorted_by_pos); } +static const char *derive_filename(const char *pack_name, const char *suffix, + struct strbuf *buf) +{ + size_t len; + if (!strip_suffix(pack_name, ".pack", )) + die(_("packfile name '%s' does not end with '.pack'"), + pack_name); + strbuf_add(buf, pack_name, len); + strbuf_addch(buf, '.'); + strbuf_addstr(buf, suffix); + return buf->buf; +} + +static void write_special_file(const char *suffix, const char *msg, + const char *pack_name, const unsigned char *sha1, + const char **report) +{ + struct strbuf name_buf = STRBUF_INIT; + const char *filename; + int fd; + int msg_len = strlen(msg); + + if (pack_name) + filename = derive_filename(pack_name, suffix, _buf); + else + filename = odb_pack_name(_buf, sha1, suffix); + + fd = odb_pack_keep(filename); + if (fd < 0) { + if (errno != EEXIST) + die_errno(_("cannot write %s file '%s'"), + suffix, filename); + } else { + if (msg_len > 0) { + write_or_die(fd, msg, msg_len); + write_or_die(fd, "\n", 1); + } + if (close(fd) != 0) + die_errno(_("cannot close written %s file '%s'"), + suffix, filename); + *report = suffix; + } + strbuf_release(_buf); +} + static void final(const char *final_pack_name, const char *curr_pack_name, const char *final_index_name, const char *curr_index_name, - const char *keep_name, const char *keep_msg, - unsigned char *sha1) + const char *keep_msg, unsigned char *sha1) { const char *report = "pack"; struct strbuf pack_name = STRBUF_INIT; struct strbuf index_name = STRBUF_INIT; - struct strbuf keep_name_buf = STRBUF_INIT; int err; if (!from_stdin) { @@ -1409,28 +1452,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name, die_errno(_("error while closing pack file")); } - if (keep_msg) { - int keep_fd, keep_msg_len = strlen(keep_msg); - - if (!keep_name) - keep_name = odb_pack_name(_name_buf, sha1, "keep"); - - keep_fd = odb_pack_keep(keep_name); - if (keep_fd < 0) { - if (errno != EEXIST) - die_errno(_("cannot write keep file '%s'"), - keep_name); - } else { - if (keep_msg_len > 0) { - write_or_die(keep_fd, keep_msg, keep_msg_len); - write_or_die(keep_fd, "\n", 1); - } - if (close(keep_fd) != 0) - die_errno(_("cannot close written keep file '%s'"), - keep_name); - report = "keep"; - } - } + if (keep_msg) + write_special_file("keep", keep_msg, final_pack_name, sha1, + ); if (final_pack_name != curr_pack_name) { if (!final_pack_name) @@ -1472,7 +1496,6 @@ static void final(const char *final_pack_name, const char *curr_pack_name, strbuf_release(_name); strbuf_release(_name); - strbuf_release(_name_buf); } static int git_index_pack_config(const char *k, const char *v, void *cb) @@ -1615,26 +1638,13 @@ static void show_pack_info(int stat_only) } } -static const char *derive_filename(const char *pack_name, const char *suffix, - struct strbuf *buf) -{ - size_t len; - if (!strip_suffix(pack_name, ".pack", )) - die(_("packfile name '%s' does not end with '.pack'"), - pack_name); - strbuf_add(buf, pack_name,
[PATCH 3/9] fsck: support refs pointing to promisor objects
From: Jonathan TanTeach fsck to not treat refs referring to missing promisor objects as an error when extensions.partialclone is set. For the purposes of warning about no default refs, such refs are still treated as legitimate refs. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- builtin/fsck.c | 8 t/t0410-partial-clone.sh | 24 2 files changed, 32 insertions(+) diff --git a/builtin/fsck.c b/builtin/fsck.c index 2934299..ee937bb 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -434,6 +434,14 @@ static int fsck_handle_ref(const char *refname, const struct object_id *oid, obj = parse_object(oid); if (!obj) { + if (is_promisor_object(oid)) { + /* +* Increment default_refs anyway, because this is a +* valid ref. +*/ +default_refs++; +return 0; + } error("%s: invalid sha1 pointer %s", refname, oid_to_hex(oid)); errors_found |= ERROR_REACHABLE; /* We'll continue with the rest despite the error.. */ diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 52347fb..5a03ead 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -13,6 +13,14 @@ pack_as_from_promisor () { >repo/.git/objects/pack/pack-$HASH.promisor } +promise_and_delete () { + HASH=$(git -C repo rev-parse "$1") && + git -C repo tag -a -m message my_annotated_tag "$HASH" && + git -C repo rev-parse my_annotated_tag | pack_as_from_promisor && + git -C repo tag -d my_annotated_tag && + delete_object repo "$HASH" +} + test_expect_success 'missing reflog object, but promised by a commit, passes fsck' ' test_create_repo repo && test_commit -C repo my_commit && @@ -78,4 +86,20 @@ test_expect_success 'missing reflog object alone fails fsck, even with extension test_must_fail git -C repo fsck ' +test_expect_success 'missing ref object, but promised, passes fsck' ' + rm -rf repo && + test_create_repo repo && + test_commit -C repo my_commit && + + A=$(git -C repo commit-tree -m a HEAD^{tree}) && + + # Reference $A only from ref + git -C repo branch my_branch "$A" && + promise_and_delete "$A" && + + git -C repo config core.repositoryformatversion 1 && + git -C repo config extensions.partialcloneremote "arbitrary string" && + git -C repo fsck +' + test_done -- 2.9.3
[PATCH 2/9] fsck: introduce partialclone extension
From: Jonathan TanCurrently, Git does not support repos with very large numbers of objects or repos that wish to minimize manipulation of certain blobs (for example, because they are very large) very well, even if the user operates mostly on part of the repo, because Git is designed on the assumption that every referenced object is available somewhere in the repo storage. In such an arrangement, the full set of objects is usually available in remote storage, ready to be lazily downloaded. Introduce the ability to have missing objects in a repo. This functionality is guarded behind a new repository extension option `extensions.partialcloneremote`. See Documentation/technical/repository-version.txt for more information. Teach fsck about the new state of affairs. In this commit, teach fsck that missing promisor objects referenced from the reflog are not an error case; in future commits, fsck will be taught about other cases. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- builtin/fsck.c | 2 +- cache.h | 3 +- packfile.c | 78 -- packfile.h | 13 setup.c | 3 -- t/t0410-partial-clone.sh | 81 6 files changed, 172 insertions(+), 8 deletions(-) create mode 100755 t/t0410-partial-clone.sh diff --git a/builtin/fsck.c b/builtin/fsck.c index 56afe40..2934299 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -398,7 +398,7 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid, xstrfmt("%s@{%"PRItime"}", refname, timestamp)); obj->flags |= USED; mark_object_reachable(obj); - } else { + } else if (!is_promisor_object(oid)) { error("%s: invalid reflog entry %s", refname, oid_to_hex(oid)); errors_found |= ERROR_REACHABLE; } diff --git a/cache.h b/cache.h index 4b785c0..5f84103 100644 --- a/cache.h +++ b/cache.h @@ -1589,7 +1589,8 @@ extern struct packed_git { unsigned pack_local:1, pack_keep:1, freshened:1, -do_not_close:1; +do_not_close:1, +pack_promisor:1; unsigned char sha1[20]; struct revindex_entry *revindex; /* something like ".git/objects/pack/x.pack" */ diff --git a/packfile.c b/packfile.c index 4a5fe7a..b015a54 100644 --- a/packfile.c +++ b/packfile.c @@ -8,6 +8,12 @@ #include "list.h" #include "streaming.h" #include "sha1-lookup.h" +#include "commit.h" +#include "object.h" +#include "tag.h" +#include "tree-walk.h" +#include "tree.h" +#include "partial-clone-utils.h" char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, @@ -643,10 +649,10 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local) return NULL; /* -* ".pack" is long enough to hold any suffix we're adding (and +* ".promisor" is long enough to hold any suffix we're adding (and * the use xsnprintf double-checks that) */ - alloc = st_add3(path_len, strlen(".pack"), 1); + alloc = st_add3(path_len, strlen(".promisor"), 1); p = alloc_packed_git(alloc); memcpy(p->pack_name, path, path_len); @@ -654,6 +660,10 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local) if (!access(p->pack_name, F_OK)) p->pack_keep = 1; + xsnprintf(p->pack_name + path_len, alloc - path_len, ".promisor"); + if (!access(p->pack_name, F_OK)) + p->pack_promisor = 1; + xsnprintf(p->pack_name + path_len, alloc - path_len, ".pack"); if (stat(p->pack_name, ) || !S_ISREG(st.st_mode)) { free(p); @@ -781,7 +791,8 @@ static void prepare_packed_git_one(char *objdir, int local) if (ends_with(de->d_name, ".idx") || ends_with(de->d_name, ".pack") || ends_with(de->d_name, ".bitmap") || - ends_with(de->d_name, ".keep")) + ends_with(de->d_name, ".keep") || + ends_with(de->d_name, ".promisor")) string_list_append(, path.buf); else report_garbage(PACKDIR_FILE_GARBAGE, path.buf); @@ -1889,6 +1900,9 @@ int for_each_packed_object(each_packed_object_fn cb, void *data, unsigned flags) for (p = packed_git; p; p = p->next) { if ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local) continue; + if ((flags & FOR_EACH_OBJECT_PROMISOR_ONLY) && + !p->pack_promisor) + continue;
[PATCH 9/9] gc: do not repack promisor packfiles
From: Jonathan TanTeach gc to stop traversal at promisor objects, and to leave promisor packfiles alone. This has the effect of only repacking non-promisor packfiles, and preserves the distinction between promisor packfiles and non-promisor packfiles. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- Documentation/git-pack-objects.txt | 4 +++ builtin/gc.c | 4 +++ builtin/pack-objects.c | 14 ++ builtin/prune.c| 7 + builtin/repack.c | 12 +++-- t/t0410-partial-clone.sh | 54 -- 6 files changed, 91 insertions(+), 4 deletions(-) diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt index 6786351..ee462c6 100644 --- a/Documentation/git-pack-objects.txt +++ b/Documentation/git-pack-objects.txt @@ -246,6 +246,10 @@ So does `git bundle` (see linkgit:git-bundle[1]) when it creates a bundle. Ignore missing objects without error. This may be used with or without and of the above filtering. +--exclude-promisor-objects:: + Silently omit referenced but missing objects from the packfile. + This is used with partial clone. + SEE ALSO linkgit:git-rev-list[1] diff --git a/builtin/gc.c b/builtin/gc.c index 3c5eae0..a17806a 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -20,6 +20,7 @@ #include "argv-array.h" #include "commit.h" #include "packfile.h" +#include "partial-clone-utils.h" #define FAILED_RUN "failed to run %s" @@ -458,6 +459,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix) argv_array_push(, prune_expire); if (quiet) argv_array_push(, "--no-progress"); + if (is_partial_clone_registered()) + argv_array_push(, + "--exclude-promisor-objects"); if (run_command_v_opt(prune.argv, RUN_GIT_CMD)) return error(FAILED_RUN, prune.argv[0]); } diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index e16722f..957e459 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -83,6 +83,7 @@ static unsigned long window_memory_limit = 0; static struct list_objects_filter_options filter_options; static int arg_ignore_missing; +static int arg_exclude_promisor_objects; /* * stats @@ -2561,6 +2562,11 @@ static void show_object(struct object *obj, const char *name, void *data) if (arg_ignore_missing && !has_object_file(>oid)) return; + if (arg_exclude_promisor_objects && + !has_object_file(>oid) && + is_promisor_object(>oid)) + return; + add_preferred_base_object(name); add_object_entry(obj->oid.hash, obj->type, name, 0); obj->flags |= OBJECT_ADDED; @@ -2972,6 +2978,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) OPT_PARSE_LIST_OBJECTS_FILTER(_options), OPT_BOOL(0, "filter-ignore-missing", _ignore_missing, N_("ignore and omit missing objects from packfile")), + OPT_BOOL(0, "exclude-promisor-objects", _exclude_promisor_objects, +N_("do not pack objects in promisor packfiles")), OPT_END(), }; @@ -3017,6 +3025,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) argv_array_push(, "--unpacked"); } + if (arg_exclude_promisor_objects) { + use_internal_rev_list = 1; + fetch_if_missing = 0; + argv_array_push(, "--exclude-promisor-objects"); + } + if (!reuse_object) reuse_delta = 0; if (pack_compression_level == -1) diff --git a/builtin/prune.c b/builtin/prune.c index cddabf2..be34645 100644 --- a/builtin/prune.c +++ b/builtin/prune.c @@ -101,12 +101,15 @@ int cmd_prune(int argc, const char **argv, const char *prefix) { struct rev_info revs; struct progress *progress = NULL; + int exclude_promisor_objects = 0; const struct option options[] = { OPT__DRY_RUN(_only, N_("do not remove, show only")), OPT__VERBOSE(, N_("report pruned objects")), OPT_BOOL(0, "progress", _progress, N_("show progress")), OPT_EXPIRY_DATE(0, "expire", , N_("expire objects older than ")), + OPT_BOOL(0, "exclude-promisor-objects", _promisor_objects, +N_("limit traversal to objects outside promisor packfiles")), OPT_END() }; char *s; @@ -139,6 +142,10 @@ int cmd_prune(int argc, const char **argv, const char
[PATCH 5/9] fsck: support promisor objects as CLI argument
From: Jonathan TanTeach fsck to not treat missing promisor objects provided on the CLI as an error when extensions.partialcloneremote is set. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- builtin/fsck.c | 2 ++ t/t0410-partial-clone.sh | 13 + 2 files changed, 15 insertions(+) diff --git a/builtin/fsck.c b/builtin/fsck.c index 4c2a56d..578a7c8 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -750,6 +750,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) struct object *obj = lookup_object(oid.hash); if (!obj || !(obj->flags & HAS_OBJ)) { + if (is_promisor_object()) + continue; error("%s: object missing", oid_to_hex()); errors_found |= ERROR_OBJECT; continue; diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index b1d404e..002e071 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -125,4 +125,17 @@ test_expect_success 'missing object, but promised, passes fsck' ' git -C repo fsck ' +test_expect_success 'missing CLI object, but promised, passes fsck' ' + rm -rf repo && + test_create_repo repo && + test_commit -C repo my_commit && + + A=$(git -C repo commit-tree -m a HEAD^{tree}) && + promise_and_delete "$A" && + + git -C repo config core.repositoryformatversion 1 && + git -C repo config extensions.partialcloneremote "arbitrary string" && + git -C repo fsck "$A" +' + test_done -- 2.9.3
[PATCH 7/9] introduce fetch-object: fetch one promisor object
From: Jonathan TanIntroduce fetch-object, providing the ability to fetch one object from a promisor remote. This uses fetch-pack. To do this, the transport mechanism has been updated with 2 flags, "from-promisor" to indicate that the resulting pack comes from a promisor remote (and thus should be annotated as such by index-pack), and "no-haves" to suppress the sending of "have" lines. This will be tested in a subsequent commit. NEEDSWORK: update this when we have more information about protocol v2, which should allow a way to suppress the ref advertisement and officially allow any object type to be "want"-ed. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- Documentation/gitremote-helpers.txt | 2 ++ Makefile| 1 + builtin/fetch-pack.c| 8 builtin/index-pack.c| 16 +--- fetch-object.c | 23 +++ fetch-object.h | 6 ++ fetch-pack.c| 8 ++-- fetch-pack.h| 2 ++ remote-curl.c | 17 - transport.c | 8 transport.h | 8 11 files changed, 93 insertions(+), 6 deletions(-) create mode 100644 fetch-object.c create mode 100644 fetch-object.h diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index 4a584f3..6da3f41 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -466,6 +466,8 @@ set by Git if the remote helper has the 'option' capability. Transmit as a push option. As the push option must not contain LF or NUL characters, the string is not encoded. +TODO document 'option from-promisor' and 'option no-haves' ? + SEE ALSO linkgit:git-remote[1] diff --git a/Makefile b/Makefile index 12d141a..7a0679a 100644 --- a/Makefile +++ b/Makefile @@ -792,6 +792,7 @@ LIB_OBJS += ewah/ewah_bitmap.o LIB_OBJS += ewah/ewah_io.o LIB_OBJS += ewah/ewah_rlw.o LIB_OBJS += exec_cmd.o +LIB_OBJS += fetch-object.o LIB_OBJS += fetch-pack.o LIB_OBJS += fsck.o LIB_OBJS += gettext.o diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 366b9d1..9f303cf 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -143,6 +143,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) args.update_shallow = 1; continue; } + if (!strcmp("--from-promisor", arg)) { + args.from_promisor = 1; + continue; + } + if (!strcmp("--no-haves", arg)) { + args.no_haves = 1; + continue; + } usage(fetch_pack_usage); } if (deepen_not.nr) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 4f305a7..24c2f05 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1429,14 +1429,16 @@ static void write_special_file(const char *suffix, const char *msg, if (close(fd) != 0) die_errno(_("cannot close written %s file '%s'"), suffix, filename); - *report = suffix; + if (report) + *report = suffix; } strbuf_release(_buf); } static void final(const char *final_pack_name, const char *curr_pack_name, const char *final_index_name, const char *curr_index_name, - const char *keep_msg, unsigned char *sha1) + const char *keep_msg, const char *promisor_msg, + unsigned char *sha1) { const char *report = "pack"; struct strbuf pack_name = STRBUF_INIT; @@ -1455,6 +1457,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name, if (keep_msg) write_special_file("keep", keep_msg, final_pack_name, sha1, ); + if (promisor_msg) + write_special_file("promisor", promisor_msg, final_pack_name, + sha1, NULL); if (final_pack_name != curr_pack_name) { if (!final_pack_name) @@ -1644,6 +1649,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) const char *curr_index; const char *index_name = NULL, *pack_name = NULL; const char *keep_msg = NULL; + const char *promisor_msg = NULL; struct strbuf index_name_buf = STRBUF_INIT; struct pack_idx_entry **idx_objects; struct pack_idx_option opts; @@ -1693,6 +1699,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) keep_msg = ""; }
[PATCH 4/9] fsck: support referenced promisor objects
From: Jonathan TanTeach fsck to not treat missing promisor objects indirectly pointed to by refs as an error when extensions.partialcloneremote is set. Signed-off-by: Jonathan Tan Signed-off-by: Jeff Hostetler --- builtin/fsck.c | 11 +++ t/t0410-partial-clone.sh | 23 +++ 2 files changed, 34 insertions(+) diff --git a/builtin/fsck.c b/builtin/fsck.c index ee937bb..4c2a56d 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -149,6 +149,15 @@ static int mark_object(struct object *obj, int type, void *data, struct fsck_opt if (obj->flags & REACHABLE) return 0; obj->flags |= REACHABLE; + + if (is_promisor_object(>oid)) + /* +* Further recursion does not need to be performed on this +* object since it is a promisor object (so it does not need to +* be added to "pending"). +*/ + return 0; + if (!(obj->flags & HAS_OBJ)) { if (parent && !has_object_file(>oid)) { printf("broken link from %7s %s\n", @@ -208,6 +217,8 @@ static void check_reachable_object(struct object *obj) * do a full fsck */ if (!(obj->flags & HAS_OBJ)) { + if (is_promisor_object(>oid)) + return; if (has_sha1_pack(obj->oid.hash)) return; /* it is in pack - forget about it */ printf("missing %s %s\n", printable_type(obj), diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 5a03ead..b1d404e 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -102,4 +102,27 @@ test_expect_success 'missing ref object, but promised, passes fsck' ' git -C repo fsck ' +test_expect_success 'missing object, but promised, passes fsck' ' + rm -rf repo && + test_create_repo repo && + test_commit -C repo 1 && + test_commit -C repo 2 && + test_commit -C repo 3 && + git -C repo tag -a annotated_tag -m "annotated tag" && + + C=$(git -C repo rev-parse 1) && + T=$(git -C repo rev-parse 2^{tree}) && + B=$(git hash-object repo/3.t) && + AT=$(git -C repo rev-parse annotated_tag) && + + promise_and_delete "$C" && + promise_and_delete "$T" && + promise_and_delete "$B" && + promise_and_delete "$AT" && + + git -C repo config core.repositoryformatversion 1 && + git -C repo config extensions.partialcloneremote "arbitrary string" && + git -C repo fsck +' + test_done -- 2.9.3
[PATCH 0/9] WIP Partial clone part 2: fsck and promisors
From: Jeff HostetlerThis is part 2 of a proposed 3 part sequence for partial clone. Part 2 assumes part 1 [1] is in place. Part 2 is concerned with fsck, gc, initial support for dynamic object fetching, and tracking promisor objects. Jonathan Tan originally developed this code. I have moved it on top of [1]. [1] https://public-inbox.org/git/20171102124445.fbffd43521cd35f6a71e1...@google.com/T/ [2] https://public-inbox.org/git/cover.1506714999.git.jonathanta...@google.com/ Jeff Hostetler (1): extension.partialclone: introduce partial clone extension Jonathan Tan (8): fsck: introduce partialclone extension fsck: support refs pointing to promisor objects fsck: support referenced promisor objects fsck: support promisor objects as CLI argument index-pack: refactor writing of .keep files introduce fetch-object: fetch one promisor object sha1_file: support lazily fetching missing objects gc: do not repack promisor packfiles Documentation/git-pack-objects.txt | 4 + Documentation/gitremote-helpers.txt| 2 + Documentation/technical/repository-version.txt | 22 ++ Makefile | 2 + builtin/cat-file.c | 3 + builtin/fetch-pack.c | 10 + builtin/fsck.c | 26 +- builtin/gc.c | 4 + builtin/index-pack.c | 113 builtin/pack-objects.c | 14 + builtin/prune.c| 7 + builtin/repack.c | 12 +- builtin/rev-list.c | 35 ++- cache.h| 15 +- config.h | 3 + environment.c | 2 + fetch-object.c | 26 ++ fetch-object.h | 6 + fetch-pack.c | 8 +- fetch-pack.h | 2 + list-objects.c | 8 +- object.c | 2 +- packfile.c | 78 +- packfile.h | 13 + partial-clone-utils.c | 78 ++ partial-clone-utils.h | 34 +++ remote-curl.c | 17 +- revision.c | 32 ++- revision.h | 5 +- setup.c| 12 + sha1_file.c| 39 ++- t/t0410-partial-clone.sh | 343 + transport.c| 8 + transport.h| 8 + 34 files changed, 917 insertions(+), 76 deletions(-) create mode 100644 fetch-object.c create mode 100644 fetch-object.h create mode 100644 partial-clone-utils.c create mode 100644 partial-clone-utils.h create mode 100755 t/t0410-partial-clone.sh -- 2.9.3
Re: Git libsecret No Unlock Dialog Issue
I've tested the code change locally and seems like it fixes the issue. Yaroslav On Thu, Nov 2, 2017 at 2:55 PM, Dennis Kaarsemakerwrote: > On Thu, 2017-11-02 at 11:35 -0700, Stefan Beller wrote: >> On Thu, Nov 2, 2017 at 9:00 AM, Yaroslav Sapozhnyk >> wrote: >> > When using Git on Fedora with locked password store >> > credential-libsecret asks for username/password instead of displaying >> > the unlock dialog. >> >> Git as packaged by Fedora or upstream Git (which version)? > > Looking at the code: current upstream git. Looking at the documentation > for libsecret, this should fix it. I've not been able to test it > though. > > diff --git a/contrib/credential/libsecret/git-credential-libsecret.c > b/contrib/credential/libsecret/git-credential-libsecret.c > index 4c56979d8a..b4750c9ee8 100644 > --- a/contrib/credential/libsecret/git-credential-libsecret.c > +++ b/contrib/credential/libsecret/git-credential-libsecret.c > @@ -104,7 +104,7 @@ static int keyring_get(struct credential *c) > items = secret_service_search_sync(service, >SECRET_SCHEMA_COMPAT_NETWORK, >attributes, > - SECRET_SEARCH_LOAD_SECRETS, > + SECRET_SEARCH_LOAD_SECRETS | > SECRET_SEARCH_UNLOCK, >NULL, >); > g_hash_table_unref(attributes); -- Regards, Yaroslav Sapozhnyk
Re: [PATCH v2 0/6] Partial clone part 1: object filtering
On Thu, 2 Nov 2017 17:50:07 + Jeff Hostetlerwrote: > From: Jeff Hostetler > > Here is V2 of the list-object filtering. It replaces [1] > and reflect a refactoring and simplification of the original. Thanks, overall this looks quite good. I reviewed patches 2-6 (skipping 1 since it's already in next), made my comments on 4, and don't have any for the rest (besides what's below). > I've added "--filter-ignore-missing" parameter to rev-list and > pack-objects to ignore missing objects rather than error out. > This allows this patch series to better stand on its own eliminates > the need in part 1 for "patch 9" from V1. > > This is a brute force ignore all missing objects. Later, in part > 2 or part 3 when --exclude-promisor-objects is introduced, we will > be able to ignore EXPECTED missing objects. (This is regarding patches 5 and 6.) Is the intention to support both flags? (That is, --ignore-missing to ignore without checking whether the object being missing is not unexpected, and --exclude-promisor-objects to check and ignore.)
[PATCHv3 4/7] builtin/describe.c: rename `oid` to avoid variable shadowing
The function `describe` has already a variable named `oid` declared at the beginning of the function for an object id. Do not shadow that variable with a pointer to an object id. Signed-off-by: Stefan Beller--- builtin/describe.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/describe.c b/builtin/describe.c index 29075dbd0f..fd61f463cf 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -381,9 +381,9 @@ static void describe(const char *arg, int last_one) } if (!match_cnt) { - struct object_id *oid = >object.oid; + struct object_id *cmit_oid = >object.oid; if (always) { - printf("%s", find_unique_abbrev(oid->hash, abbrev)); + printf("%s", find_unique_abbrev(cmit_oid->hash, abbrev)); if (suffix) printf("%s", suffix); printf("\n"); @@ -392,11 +392,11 @@ static void describe(const char *arg, int last_one) if (unannotated_cnt) die(_("No annotated tags can describe '%s'.\n" "However, there were unannotated tags: try --tags."), - oid_to_hex(oid)); + oid_to_hex(cmit_oid)); else die(_("No tags can describe '%s'.\n" "Try --always, or create some tags."), - oid_to_hex(oid)); + oid_to_hex(cmit_oid)); } QSORT(all_matches, match_cnt, compare_pt); -- 2.15.0.7.g980e40477f
[PATCHv3 1/7] t6120: fix typo in test name
Signed-off-by: Stefan Beller--- t/t6120-describe.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index 1c0e8659d9..c8b7ed82d9 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -304,7 +304,7 @@ test_expect_success 'describe chokes on severely broken submodules' ' mv .git/modules/sub1/ .git/modules/sub_moved && test_must_fail git describe --dirty ' -test_expect_success 'describe ignoring a borken submodule' ' +test_expect_success 'describe ignoring a broken submodule' ' git describe --broken >out && test_when_finished "mv .git/modules/sub_moved .git/modules/sub1" && grep broken out -- 2.15.0.7.g980e40477f
[PATCHv3 6/7] builtin/describe.c: factor out describe_commit
In the next patch we'll learn how to describe more than just commits, so factor out describing commits into its own function. That will make the next patches easy as we still need to describe a commit as part of describing blobs. While factoring out the functionality to describe_commit, make sure that any output to stdout is put into a strbuf, which we can print afterwards, using puts which also adds the line ending. Signed-off-by: Stefan Beller--- builtin/describe.c | 63 -- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/builtin/describe.c b/builtin/describe.c index 3136efde31..9e9a5ed5d4 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -256,7 +256,7 @@ static unsigned long finish_depth_computation( return seen_commits; } -static void display_name(struct commit_name *n) +static void append_name(struct commit_name *n, struct strbuf *dst) { if (n->prio == 2 && !n->tag) { n->tag = lookup_tag(>oid); @@ -272,19 +272,18 @@ static void display_name(struct commit_name *n) } if (n->tag) - printf("%s", n->tag->tag); + strbuf_addstr(dst, n->tag->tag); else - printf("%s", n->path); + strbuf_addstr(dst, n->path); } -static void show_suffix(int depth, const struct object_id *oid) +static void append_suffix(int depth, const struct object_id *oid, struct strbuf *dst) { - printf("-%d-g%s", depth, find_unique_abbrev(oid->hash, abbrev)); + strbuf_addf(dst, "-%d-g%s", depth, find_unique_abbrev(oid->hash, abbrev)); } -static void describe(const char *arg, int last_one) +static void describe_commit(struct object_id *oid, struct strbuf *dst) { - struct object_id oid; struct commit *cmit, *gave_up_on = NULL; struct commit_list *list; struct commit_name *n; @@ -293,26 +292,18 @@ static void describe(const char *arg, int last_one) unsigned long seen_commits = 0; unsigned int unannotated_cnt = 0; - if (debug) - fprintf(stderr, _("describe %s\n"), arg); - - if (get_oid(arg, )) - die(_("Not a valid object name %s"), arg); - cmit = lookup_commit_reference(); - if (!cmit) - die(_("%s is not a valid '%s' object"), arg, commit_type); + cmit = lookup_commit_reference(oid); n = find_commit_name(>object.oid); if (n && (tags || all || n->prio == 2)) { /* * Exact match to an existing ref. */ - display_name(n); + append_name(n, dst); if (longformat) - show_suffix(0, n->tag ? >tag->tagged->oid : ); + append_suffix(0, n->tag ? >tag->tagged->oid : oid, dst); if (suffix) - printf("%s", suffix); - printf("\n"); + strbuf_addstr(dst, suffix); return; } @@ -386,10 +377,9 @@ static void describe(const char *arg, int last_one) if (!match_cnt) { struct object_id *cmit_oid = >object.oid; if (always) { - printf("%s", find_unique_abbrev(cmit_oid->hash, abbrev)); + strbuf_addstr(dst, find_unique_abbrev(cmit_oid->hash, abbrev)); if (suffix) - printf("%s", suffix); - printf("\n"); + strbuf_addstr(dst, suffix); return; } if (unannotated_cnt) @@ -437,15 +427,36 @@ static void describe(const char *arg, int last_one) } } - display_name(all_matches[0].name); + append_name(all_matches[0].name, dst); if (abbrev) - show_suffix(all_matches[0].depth, >object.oid); + append_suffix(all_matches[0].depth, >object.oid, dst); if (suffix) - printf("%s", suffix); - printf("\n"); + strbuf_addstr(dst, suffix); +} + +static void describe(const char *arg, int last_one) +{ + struct object_id oid; + struct commit *cmit; + struct strbuf sb = STRBUF_INIT; + + if (debug) + fprintf(stderr, _("describe %s\n"), arg); + + if (get_oid(arg, )) + die(_("Not a valid object name %s"), arg); + cmit = lookup_commit_reference(); + if (!cmit) + die(_("%s is not a valid '%s' object"), arg, commit_type); + + describe_commit(, ); + + puts(sb.buf); if (!last_one) clear_commit_marks(cmit, -1); + + strbuf_release(); } int cmd_describe(int argc, const char **argv, const char *prefix) -- 2.15.0.7.g980e40477f
[PATCHv3 3/7] revision.h: introduce blob/tree walking in order of the commits
The functionality to list tree objects in the order they were seen while traversing the commits will be used in the next commit, where we teach `git describe` to describe not only commits, but trees and blobs, too. Signed-off-by: Stefan Beller--- Documentation/rev-list-options.txt | 5 list-objects.c | 2 ++ revision.c | 2 ++ revision.h | 3 ++- t/t6100-rev-list-in-order.sh | 47 ++ 5 files changed, 58 insertions(+), 1 deletion(-) create mode 100755 t/t6100-rev-list-in-order.sh diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 13501e1556..9066e0c777 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -686,6 +686,11 @@ ifdef::git-rev-list[] all object IDs which I need to download if I have the commit object _bar_ but not _foo_''. +--in-commit-order:: + Print tree and blob ids in order of the commits. The tree + and blob ids are printed after they are first referenced + by a commit. + --objects-edge:: Similar to `--objects`, but also print the IDs of excluded commits prefixed with a ``-'' character. This is used by diff --git a/list-objects.c b/list-objects.c index 7c2ce9c4bd..07a92f35fe 100644 --- a/list-objects.c +++ b/list-objects.c @@ -239,6 +239,8 @@ void traverse_commit_list(struct rev_info *revs, if (commit->tree) add_pending_tree(revs, commit->tree); show_commit(commit, data); + if (revs->tree_blobs_in_commit_order) + traverse_trees_and_blobs(revs, , show_object, data); } traverse_trees_and_blobs(revs, , show_object, data); diff --git a/revision.c b/revision.c index d167223e69..9329d4ebbf 100644 --- a/revision.c +++ b/revision.c @@ -1845,6 +1845,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg revs->dense = 0; } else if (!strcmp(arg, "--show-all")) { revs->show_all = 1; + } else if (!strcmp(arg, "--in-commit-order")) { + revs->tree_blobs_in_commit_order = 1; } else if (!strcmp(arg, "--remove-empty")) { revs->remove_empty_trees = 1; } else if (!strcmp(arg, "--merges")) { diff --git a/revision.h b/revision.h index 54761200ad..86985d68aa 100644 --- a/revision.h +++ b/revision.h @@ -121,7 +121,8 @@ struct rev_info { bisect:1, ancestry_path:1, first_parent_only:1, - line_level_traverse:1; + line_level_traverse:1, + tree_blobs_in_commit_order:1; /* Diff flags */ unsigned intdiff:1, diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh new file mode 100755 index 00..d4d539b0da --- /dev/null +++ b/t/t6100-rev-list-in-order.sh @@ -0,0 +1,47 @@ +#!/bin/sh + +test_description='rev-list testing in-commit-order' + +. ./test-lib.sh + +test_expect_success 'rev-list --in-commit-order' ' + for x in one two three four + do + echo $x >$x && + git add $x && + git commit -m "add file $x" || + return 1 + done && + for x in four three + do + git rm $x && + git commit -m "remove $x" || + return 1 + done && + git rev-list --in-commit-order --objects HEAD >actual.raw && + cut -c 1-40 >actual expect.raw <<-\EOF && + HEAD^{commit} + HEAD^{tree} + HEAD^{tree}:one + HEAD^{tree}:two + HEAD~1^{commit} + HEAD~1^{tree} + HEAD~1^{tree}:three + HEAD~2^{commit} + HEAD~2^{tree} + HEAD~2^{tree}:four + HEAD~3^{commit} + # HEAD~3^{tree} skipped, same as HEAD~1^{tree} + HEAD~4^{commit} + # HEAD~4^{tree} skipped, same as HEAD^{tree} + HEAD~5^{commit} + HEAD~5^{tree} + EOF + grep -v "#" >expect
[PATCHv3 5/7] builtin/describe.c: print debug statements earlier
For debuggers aid we'd want to print debug statements early, so introduce a new line in the debug output that describes the whole function, and then change the next debug output to describe why we need to search. Conveniently drop the arg from the second line; which will be useful in a follow up commit, that refactors the\ describe function. Signed-off-by: Stefan Beller--- builtin/describe.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/describe.c b/builtin/describe.c index fd61f463cf..3136efde31 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -293,6 +293,9 @@ static void describe(const char *arg, int last_one) unsigned long seen_commits = 0; unsigned int unannotated_cnt = 0; + if (debug) + fprintf(stderr, _("describe %s\n"), arg); + if (get_oid(arg, )) die(_("Not a valid object name %s"), arg); cmit = lookup_commit_reference(); @@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one) if (!max_candidates) die(_("no tag exactly matches '%s'"), oid_to_hex(>object.oid)); if (debug) - fprintf(stderr, _("searching to describe %s\n"), arg); + fprintf(stderr, _("No exact match on refs or tags, searching to describe\n")); if (!have_util) { struct hashmap_iter iter; -- 2.15.0.7.g980e40477f
[PATCHv3 7/7] builtin/describe.c: describe a blob
Sometimes users are given a hash of an object and they want to identify it further (ex.: Use verify-pack to find the largest blobs, but what are these? or [1]) "This is an interesting endeavor, because describing things is hard." -- me, upon writing this patch. When describing commits, we try to anchor them to tags or refs, as these are conceptually on a higher level than the commit. And if there is no ref or tag that matches exactly, we're out of luck. So we employ a heuristic to make up a name for the commit. These names are ambiguous, there might be different tags or refs to anchor to, and there might be different path in the DAG to travel to arrive at the commit precisely. When describing a blob, we want to describe the blob from a higher layer as well, which is a tuple of (commit, deep/path) as the tree objects involved are rather uninteresting. The same blob can be referenced by multiple commits, so how we decide which commit to use? This patch implements a rather naive approach on this: As there are no back pointers from blobs to commits in which the blob occurs, we'll start walking from any tips available, listing the blobs in-order of the commit and once we found the blob, we'll take the first commit that listed the blob. For source code this is likely not the first commit that introduced the blob, but rather the latest commit that contained the blob. For example: git describe v0.99:Makefile v0.99-5-gab6625e06a:Makefile tells us the latest commit that contained the Makefile as it was in tag v0.99 is commit v0.99-5-gab6625e06a (and at the same path), as the next commit on top v0.99-6-gb1de9de2b9 ([PATCH] Bootstrap "make dist", 2005-07-11) touches the Makefile. Let's see how this description turns out, if it is useful in day-to-day use as I have the intuition that we'd rather want to see the *first* commit that this blob was introduced to the repository (which can be achieved easily by giving the `--reverse` flag in the describe_blob rev walk). [1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob Signed-off-by: Stefan Beller--- Documentation/git-describe.txt | 13 - builtin/describe.c | 65 ++ t/t6120-describe.sh| 15 ++ 3 files changed, 87 insertions(+), 6 deletions(-) diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt index c924c945ba..79ec0be62a 100644 --- a/Documentation/git-describe.txt +++ b/Documentation/git-describe.txt @@ -3,7 +3,7 @@ git-describe(1) NAME -git-describe - Describe a commit using the most recent tag reachable from it +git-describe - Describe a commit or blob using the graph relations SYNOPSIS @@ -11,6 +11,7 @@ SYNOPSIS [verse] 'git describe' [--all] [--tags] [--contains] [--abbrev=] [...] 'git describe' [--all] [--tags] [--contains] [--abbrev=] --dirty[=] +'git describe' [] DESCRIPTION --- @@ -24,6 +25,16 @@ By default (without --all or --tags) `git describe` only shows annotated tags. For more information about creating annotated tags see the -a and -s options to linkgit:git-tag[1]. +If the given object refers to a blob, it will be described +as `:`, such that the blob can be found +at `` in the ``. Note, that the commit is likely +not the commit that introduced the blob, but the one that was found +first; to find the commit that introduced the blob, you need to find +the commit that last touched the path, e.g. +`git log -- `. +As blobs do not point at the commits they are contained in, +describing blobs is slow as we have to walk the whole graph. + OPTIONS --- ...:: diff --git a/builtin/describe.c b/builtin/describe.c index 9e9a5ed5d4..cf08bef344 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -3,6 +3,7 @@ #include "lockfile.h" #include "commit.h" #include "tag.h" +#include "blob.h" #include "refs.h" #include "builtin.h" #include "exec_cmd.h" @@ -11,8 +12,9 @@ #include "hashmap.h" #include "argv-array.h" #include "run-command.h" +#include "revision.h" +#include "list-objects.h" -#define SEEN (1u << 0) #define MAX_TAGS (FLAG_BITS - 1) static const char * const describe_usage[] = { @@ -434,6 +436,56 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst) strbuf_addstr(dst, suffix); } +struct process_commit_data { + struct object_id current_commit; + struct object_id looking_for; + struct strbuf *dst; + struct rev_info *revs; +}; + +static void process_commit(struct commit *commit, void *data) +{ + struct process_commit_data *pcd = data; + pcd->current_commit = commit->object.oid; +} + +static void process_object(struct object *obj, const char *path, void *data) +{ + struct process_commit_data *pcd = data; + + if (!oidcmp(>looking_for, >oid) && !pcd->dst->len) { + reset_revision_walk(); +
[PATCHv3 0/7] git describe blob
Thanks for the discussion on v2[1]. Interdiff is below, just fixing minor things. We'll keep the original algorithm for now, deferring an improvement on the algorithm front towards a future developer. Thanks, Stefan [1] https://public-inbox.org/git/20171031211852.13001-1-sbel...@google.com/ Stefan Beller (7): t6120: fix typo in test name list-objects.c: factor out traverse_trees_and_blobs revision.h: introduce blob/tree walking in order of the commits builtin/describe.c: rename `oid` to avoid variable shadowing builtin/describe.c: print debug statements earlier builtin/describe.c: factor out describe_commit builtin/describe.c: describe a blob Documentation/git-describe.txt | 13 +++- Documentation/rev-list-options.txt | 5 ++ builtin/describe.c | 125 - list-objects.c | 52 +-- revision.c | 2 + revision.h | 3 +- t/t6100-rev-list-in-order.sh | 47 ++ t/t6120-describe.sh| 17 - 8 files changed, 214 insertions(+), 50 deletions(-) create mode 100755 t/t6100-rev-list-in-order.sh diff --git c/Documentation/git-describe.txt w/Documentation/git-describe.txt index 077c3c2193..79ec0be62a 100644 --- c/Documentation/git-describe.txt +++ w/Documentation/git-describe.txt @@ -11,7 +11,7 @@ SYNOPSIS [verse] 'git describe' [--all] [--tags] [--contains] [--abbrev=] [...] 'git describe' [--all] [--tags] [--contains] [--abbrev=] --dirty[=] -'git describe' [] +'git describe' [] DESCRIPTION --- diff --git c/builtin/describe.c w/builtin/describe.c index 382cbae908..cf08bef344 100644 --- c/builtin/describe.c +++ w/builtin/describe.c @@ -440,6 +440,7 @@ struct process_commit_data { struct object_id current_commit; struct object_id looking_for; struct strbuf *dst; + struct rev_info *revs; }; static void process_commit(struct commit *commit, void *data) @@ -456,6 +457,7 @@ static void process_object(struct object *obj, const char *path, void *data) reset_revision_walk(); describe_commit(>current_commit, pcd->dst); strbuf_addf(pcd->dst, ":%s", path); + pcd->revs->max_count = 0; } } @@ -463,7 +465,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst) { struct rev_info revs; struct argv_array args = ARGV_ARRAY_INIT; - struct process_commit_data pcd = { null_oid, oid, dst}; + struct process_commit_data pcd = { null_oid, oid, dst, }; argv_array_pushl(, "internal: The first arg is not parsed", "--all", "--reflog", /* as many starting points as possible */ @@ -497,14 +499,12 @@ static void describe(const char *arg, int last_one) die(_("Not a valid object name %s"), arg); cmit = lookup_commit_reference_gently(, 1); - if (cmit) { + if (cmit) describe_commit(, ); - } else { - if (lookup_blob()) - describe_blob(oid, ); - else - die(_("%s is neither a commit nor blob"), arg); - } + else if (lookup_blob()) + describe_blob(oid, ); + else + die(_("%s is neither a commit nor blob"), arg); puts(sb.buf); diff --git c/list-objects.c w/list-objects.c index 03438e5a8b..07a92f35fe 100644 --- c/list-objects.c +++ w/list-objects.c @@ -184,13 +184,13 @@ static void add_pending_tree(struct rev_info *revs, struct tree *tree) } static void traverse_trees_and_blobs(struct rev_info *revs, -struct strbuf *base_path, +struct strbuf *base, show_object_fn show_object, void *data) { int i; - assert(base_path->len == 0); + assert(base->len == 0); for (i = 0; i < revs->pending.nr; i++) { struct object_array_entry *pending = revs->pending.objects + i; @@ -208,12 +208,12 @@ static void traverse_trees_and_blobs(struct rev_info *revs, path = ""; if (obj->type == OBJ_TREE) { process_tree(revs, (struct tree *)obj, show_object, -base_path, path, data); +base, path, data); continue; } if (obj->type == OBJ_BLOB) { process_blob(revs, (struct blob *)obj, show_object, -base_path, path, data); +base, path, data); continue; } die("unknown pending object %s (%s)", diff --git c/t/t6100-rev-list-in-order.sh w/t/t6100-rev-list-in-order.sh index
[PATCHv3 2/7] list-objects.c: factor out traverse_trees_and_blobs
With traverse_trees_and_blobs factored out of the main traverse function, the next patch can introduce an in-order revision walking with ease. In the next patch we'll call `traverse_trees_and_blobs` from within the loop walking the commits, such that we'll have one invocation of that function per commit. That is why we do not want to have memory allocations in that function, such as we'd have if we were to use a strbuf locally. Pass a strbuf from traverse_commit_list into the blob and tree traversing function as a scratch pad that only needs to be allocated once. Signed-off-by: Stefan Beller--- list-objects.c | 50 +++--- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/list-objects.c b/list-objects.c index b3931fa434..7c2ce9c4bd 100644 --- a/list-objects.c +++ b/list-objects.c @@ -183,25 +183,15 @@ static void add_pending_tree(struct rev_info *revs, struct tree *tree) add_pending_object(revs, >object, ""); } -void traverse_commit_list(struct rev_info *revs, - show_commit_fn show_commit, - show_object_fn show_object, - void *data) +static void traverse_trees_and_blobs(struct rev_info *revs, +struct strbuf *base, +show_object_fn show_object, +void *data) { int i; - struct commit *commit; - struct strbuf base; - strbuf_init(, PATH_MAX); - while ((commit = get_revision(revs)) != NULL) { - /* -* an uninteresting boundary commit may not have its tree -* parsed yet, but we are not going to show them anyway -*/ - if (commit->tree) - add_pending_tree(revs, commit->tree); - show_commit(commit, data); - } + assert(base->len == 0); + for (i = 0; i < revs->pending.nr; i++) { struct object_array_entry *pending = revs->pending.objects + i; struct object *obj = pending->item; @@ -218,17 +208,39 @@ void traverse_commit_list(struct rev_info *revs, path = ""; if (obj->type == OBJ_TREE) { process_tree(revs, (struct tree *)obj, show_object, -, path, data); +base, path, data); continue; } if (obj->type == OBJ_BLOB) { process_blob(revs, (struct blob *)obj, show_object, -, path, data); +base, path, data); continue; } die("unknown pending object %s (%s)", oid_to_hex(>oid), name); } object_array_clear(>pending); - strbuf_release(); +} + +void traverse_commit_list(struct rev_info *revs, + show_commit_fn show_commit, + show_object_fn show_object, + void *data) +{ + struct commit *commit; + struct strbuf csp; /* callee's scratch pad */ + strbuf_init(, PATH_MAX); + + while ((commit = get_revision(revs)) != NULL) { + /* +* an uninteresting boundary commit may not have its tree +* parsed yet, but we are not going to show them anyway +*/ + if (commit->tree) + add_pending_tree(revs, commit->tree); + show_commit(commit, data); + } + traverse_trees_and_blobs(revs, , show_object, data); + + strbuf_release(); } -- 2.15.0.7.g980e40477f
Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list
On Thu, 2 Nov 2017 17:50:11 + Jeff Hostetlerwrote: > +int parse_list_objects_filter(struct list_objects_filter_options > *filter_options, > + const char *arg) Returning void is fine, I think. It seems that all your code paths either return 0 or die. > +{ > + struct object_context oc; > + struct object_id sparse_oid; > + const char *v0; > + const char *v1; > + > + if (filter_options->choice) > + die(_("multiple object filter types cannot be combined")); > + > + /* > + * TODO consider rejecting 'arg' if it contains any > + * TODO injection characters (since we might send this > + * TODO to a sub-command or to the server and we don't > + * TODO want to deal with legacy quoting/escaping for > + * TODO a new feature). > + */ > + > + filter_options->raw_value = strdup(arg); > + > + if (skip_prefix(arg, "blob:", ) || skip_prefix(arg, "blobs:", )) { > + if (!strcmp(v0, "none")) { > + filter_options->choice = LOFC_BLOB_NONE; > + return 0; > + } > + > + if (skip_prefix(v0, "limit=", ) && > + git_parse_ulong(v1, _options->blob_limit_value)) { > + filter_options->choice = LOFC_BLOB_LIMIT; > + return 0; > + } > + } > + else if (skip_prefix(arg, "sparse:", )) { Style: join the 2 lines above. > + if (skip_prefix(v0, "oid=", )) { > + filter_options->choice = LOFC_SPARSE_OID; > + if (!get_oid_with_context(v1, GET_OID_BLOB, > + _oid, )) { > + /* > + * We successfully converted the > + * into an actual OID. Rewrite the raw_value > + * in canonoical form with just the OID. > + * (If we send this request to the server, we > + * want an absolute expression rather than a > + * local-ref-relative expression.) > + */ I think this would lead to confusing behavior - for example, a fetch with "--filter=oid=mybranch:sparseconfig" would have different results depending on whether "mybranch" refers to a valid object locally. The way I see it, this should either (i) only accept full 40-character OIDs or (ii) retain the raw string to be interpreted only when the filtering is done. (i) is simpler and safer, but is not so useful. In both cases, if the user really wants client-side interpretation, they can still use "$(git rev-parse foo)" to make it explicit. > + free((char *)filter_options->raw_value); > + filter_options->raw_value = > + xstrfmt("sparse:oid=%s", > + oid_to_hex(_oid)); > + filter_options->sparse_oid_value = > + oiddup(_oid); > + } else { > + /* > + * We could not turn the into an > + * OID. Leave the raw_value as is in case > + * the server can parse it. (It may refer to > + * a branch, commit, or blob we don't have.) > + */ > + } > + return 0; > + } > + > + if (skip_prefix(v0, "path=", )) { > + filter_options->choice = LOFC_SPARSE_PATH; > + filter_options->sparse_path_value = strdup(v1); > + return 0; > + } > + } > + > + die(_("invalid filter expression '%s'"), arg); > + return 0; > +} [snip] > +void arg_format_list_objects_filter( > + struct argv_array *argv_array, > + const struct list_objects_filter_options *filter_options) Is this function used anywhere (in this patch or subsequent patches)? > diff --git a/list-objects-filter.c b/list-objects-filter.c > +/* See object.h and revision.h */ > +#define FILTER_REVISIT (1<<25) Looking later in the code, this flag indicates that a tree has been SHOWN, so it might be better to just call this FILTER_SHOWN. Also document this flag in object.h in the table above FLAG_BITS. > +static enum list_objects_filter_result filter_blobs_limit( > + enum list_objects_filter_type filter_type, > + struct object *obj, > + const char *pathname, > + const char *filename, > + void *filter_data_) > +{ > + struct filter_blobs_limit_data *filter_data = filter_data_; > + unsigned long object_length; > + enum object_type t; > + int is_special_filename; > + > + switch (filter_type) { > + default: > +
Re: Git libsecret No Unlock Dialog Issue
On Thu, 2017-11-02 at 11:35 -0700, Stefan Beller wrote: > On Thu, Nov 2, 2017 at 9:00 AM, Yaroslav Sapozhnyk >wrote: > > When using Git on Fedora with locked password store > > credential-libsecret asks for username/password instead of displaying > > the unlock dialog. > > Git as packaged by Fedora or upstream Git (which version)? Looking at the code: current upstream git. Looking at the documentation for libsecret, this should fix it. I've not been able to test it though. diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c index 4c56979d8a..b4750c9ee8 100644 --- a/contrib/credential/libsecret/git-credential-libsecret.c +++ b/contrib/credential/libsecret/git-credential-libsecret.c @@ -104,7 +104,7 @@ static int keyring_get(struct credential *c) items = secret_service_search_sync(service, SECRET_SCHEMA_COMPAT_NETWORK, attributes, - SECRET_SEARCH_LOAD_SECRETS, + SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK, NULL, ); g_hash_table_unref(attributes);
Re: Git libsecret No Unlock Dialog Issue
Sorry, should have mentioned that. It's packaged by Fedora - 2.13.6. Yaroslav On Thu, Nov 2, 2017 at 2:35 PM, Stefan Bellerwrote: > On Thu, Nov 2, 2017 at 9:00 AM, Yaroslav Sapozhnyk > wrote: >> When using Git on Fedora with locked password store >> credential-libsecret asks for username/password instead of displaying >> the unlock dialog. > > Git as packaged by Fedora or upstream Git (which version)? > >> If the store is unlocked credential helper gets the credentials from >> the store though. >> >> -- >> Regards, >> Yaroslav Sapozhnyk -- Regards, Yaroslav Sapozhnyk
Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation
On Thu, Nov 2, 2017 at 1:39 AM, Kaartic Sivaraamwrote: > On Thursday 02 November 2017 12:24 PM, Kaartic Sivaraam wrote: >> >> Signed-off-by: Kaartic Sivaraam >> Signed-off-by: Kaartic Sivaraam > > > I just now saw this small glitch as a consequence of recently > changing my email address. I would prefer to keep the second one > but as the other patches have the first one it's better to keep > the first one for now. If you prefer one of them, you may have incentive to add an entry into .mailmap file, otherwise I'd kindly ask you to. :) (c.f. `git log --no-merges -- .mailmap`) > But wait, it seems that this commit also has a different author > identity (the email adress part). If this is a big enough issue, > I'll fix that and send a v4 (possibly with any other suggested > changes) else I'll leave it as it is. > > BTW, I added the Ccs to this mail which I forgot to do when > sending the patches, hope it's not an issue.
Re: Git libsecret No Unlock Dialog Issue
On Thu, Nov 2, 2017 at 9:00 AM, Yaroslav Sapozhnykwrote: > When using Git on Fedora with locked password store > credential-libsecret asks for username/password instead of displaying > the unlock dialog. Git as packaged by Fedora or upstream Git (which version)? > If the store is unlocked credential helper gets the credentials from > the store though. > > -- > Regards, > Yaroslav Sapozhnyk
Re: Regression[2.14.3->2.15]: Interactive rebase fails if submodule is modified
On Thu, Nov 2, 2017 at 1:30 AM, Orgad Shanehwrote: > I can't reproduce this with a minimal example, but it happens in my project. > > What I tried to do for reproducing is: > rm -rf super sub > mkdir sub; cd sub; git init > git commit --allow-empty -m 'Initial commit' > mkdir ../super; cd ../super > git init > git submodule add ../sub > touch foo; git add foo sub > git commit -m 'Initial commit' > touch a; git add a; git commit -m 'a' > touch b; git add b; git commit -m 'b' > cd sub; git commit --allow-empty -m 'New commit'; cd .. > git rebase -i HEAD^^ > > Then drop a. > > In my project I get: > error: cannot rebase: You have unstaged changes. > > This works fine with 2.14.3. git log --oneline v2.14.3..v2.15.0 -- submodule.c doesn't give any promising hints (i.e. I don't think one of a submodule related series introduced this either by chance or on purpose) "rebase -i" was rewritten into C in 570676e011, though that series was extensively tested by DScho, so I wouldn't want to point fingers here quickly. Would you be willing to bisect this behavior?
Re: [PATCHv2 6/7] builtin/describe.c: describe a blob
On Thu, Nov 2, 2017 at 12:23 AM, Andreas Schwabwrote: > On Nov 01 2017, Johannes Schindelin wrote: > >> Sure, but it is still a tricky thing. Imagine >> >> - A1 - B1 - A2 - B2 - B3 >> >> where all the B* commits have the blob. Do you really want to report B1 >> rather than B2 as the commit introducing the blob? (I would prefer B2...) > > What if B3 renames or copies the blob? > > Andreas. With the current proposed patch you'd find B3, and then use the diff machinery to digg deeper from there (renames/copies ought to be easy to detect already?) So with a copy B3 might be a better start than B1, as starting from B1 you would not find B3 easily. For a rename, I would think a reverse log/blame on B1:path may help. With that said, I think I'll just reroll the series with the current logic fixing the other minor issues that were brought up as B3 seems to be the most versatile (though not optimal) answer for many use cases. Thanks for that thought, Stefan
[PATCHv2] config: document blame configuration
The options are currently only referenced by the git-blame man page, also explain them in git-config, which is the canonical page to contain all config options. Signed-off-by: Stefan Beller--- * correct option to blame.showRoot * camelCased other options * use linkgit:git-[1] instead of `git-cmd` as that is correct, but maybe overused. * --date is `backticked` now. Documentation/config.txt | 17 + 1 file changed, 17 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 1ac0ae6adb..ba0156b1e8 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -949,6 +949,23 @@ apply.whitespace:: Tells 'git apply' how to handle whitespaces, in the same way as the `--whitespace` option. See linkgit:git-apply[1]. +blame.showRoot:: + Do not treat root commits as boundaries in linkgit:git-blame[1]. + This option defaults to false. + +blame.blankBoundary:: + Show blank SHA-1 for boundary commits in linkgit:git-blame[1]. + This option defaults to false. + +blame.showEmail:: + Show the author email instead of author name in linkgit:git-blame[1]. + This option defaults to false. + +blame.date:: + Specifies the format used to output dates in linkgit:git-blame[1]. + If unset the iso format is used. For supported values, + see the discussion of the `--date` option at linkgit:git-log[1]. + branch.autoSetupMerge:: Tells 'git branch' and 'git checkout' to set up new branches so that linkgit:git-pull[1] will appropriately merge from the -- 2.15.0.7.g980e40477f
Re: [BUG] git clean -d is too greedy
I was a bit trigger happy posting this, after digging a bit more this is a way more serious than I originally thought. 1) .gitignore exists in nested repo (either tracked or untracked) 2) .gitignore is excluded This can be any file, including those commonly excluded such as *~. Demonstrating this in a way that looks a lot more likely use case which will potentially wipe out lots of uncommitted work: # git init -q foo && cd foo # git init -q bar && cd bar # touch bar bar~ # git add bar && git commit -qm asd && cd .. # git clean -e \*~ -dn Would remove bar/bar If there were more tracked files in nested repo they'd be removed as well as long as there is at least one excluded file in nested repo. -- Hermanni
Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]
On Thu, Nov 2, 2017 at 1:01 AM, Jeff Kingwrote: > On Thu, Nov 02, 2017 at 04:48:45PM +0900, Junio C Hamano wrote: > >> Jeff King writes: >> >> > If the clutter is too much, it could also go into a git-notes ref >> > (that's not already implemented, but it seems like it would be pretty >> > easy to teach "git am" to do that). >> >> FWIW, that is what I use a notes ref "amlog" for. > > Right, I completely forgot that you were already doing that. So jumping > to the thread for a commit is basically: > > $browser $(git notes --ref=amlog show $commit | > perl -pe 's{Message-Id: > <(.*)>}{https://public-inbox.org/git/$1/t}') > > (Or whichever archive you prefer to use). Thanks for the reminder. > Thanks from my side as well. I did not pay attention when this was introduced and see the notes ref for the first time today. I'll incorporate that into my digging repertoire. Thanks, Stefan
[PATCH v2 4/6] list-objects: filter objects in traverse_commit_list
From: Jeff HostetlerCreate traverse_commit_list_filtered() and add filtering interface to allow certain objects to be omitted from the traversal. Update traverse_commit_list() to be a wrapper for the above with a null filter to minimize the number of callers that needed to be changed. Object filtering will be used in a future commit by rev-list and pack-objects for partial clone and fetch to omit unwanted objects from the result. traverse_bitmap_commit_list() does not work with filtering. If a packfile bitmap is present, it will not be used. Signed-off-by: Jeff Hostetler --- Makefile | 2 + list-objects-filter-options.c | 119 list-objects-filter-options.h | 55 ++ list-objects-filter.c | 408 ++ list-objects-filter.h | 84 + list-objects.c| 95 -- list-objects.h| 2 +- 7 files changed, 748 insertions(+), 17 deletions(-) create mode 100644 list-objects-filter-options.c create mode 100644 list-objects-filter-options.h create mode 100644 list-objects-filter.c create mode 100644 list-objects-filter.h diff --git a/Makefile b/Makefile index cd75985..ca378a4 100644 --- a/Makefile +++ b/Makefile @@ -807,6 +807,8 @@ LIB_OBJS += levenshtein.o LIB_OBJS += line-log.o LIB_OBJS += line-range.o LIB_OBJS += list-objects.o +LIB_OBJS += list-objects-filter.o +LIB_OBJS += list-objects-filter-options.o LIB_OBJS += ll-merge.o LIB_OBJS += lockfile.o LIB_OBJS += log-tree.o diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c new file mode 100644 index 000..31255e7 --- /dev/null +++ b/list-objects-filter-options.c @@ -0,0 +1,119 @@ +#include "cache.h" +#include "commit.h" +#include "config.h" +#include "revision.h" +#include "argv-array.h" +#include "list-objects.h" +#include "list-objects-filter.h" +#include "list-objects-filter-options.h" + +/* + * Parse value of the argument to the "filter" keword. + * On the command line this looks like: + * --filter= + * and in the pack protocol as: + * "filter" SP + * + * ::= blob:none + * blob:limit=[kmg] + * sparse:oid= + * sparse:path= + */ +int parse_list_objects_filter(struct list_objects_filter_options *filter_options, + const char *arg) +{ + struct object_context oc; + struct object_id sparse_oid; + const char *v0; + const char *v1; + + if (filter_options->choice) + die(_("multiple object filter types cannot be combined")); + + /* +* TODO consider rejecting 'arg' if it contains any +* TODO injection characters (since we might send this +* TODO to a sub-command or to the server and we don't +* TODO want to deal with legacy quoting/escaping for +* TODO a new feature). +*/ + + filter_options->raw_value = strdup(arg); + + if (skip_prefix(arg, "blob:", ) || skip_prefix(arg, "blobs:", )) { + if (!strcmp(v0, "none")) { + filter_options->choice = LOFC_BLOB_NONE; + return 0; + } + + if (skip_prefix(v0, "limit=", ) && + git_parse_ulong(v1, _options->blob_limit_value)) { + filter_options->choice = LOFC_BLOB_LIMIT; + return 0; + } + } + else if (skip_prefix(arg, "sparse:", )) { + if (skip_prefix(v0, "oid=", )) { + filter_options->choice = LOFC_SPARSE_OID; + if (!get_oid_with_context(v1, GET_OID_BLOB, + _oid, )) { + /* +* We successfully converted the +* into an actual OID. Rewrite the raw_value +* in canonoical form with just the OID. +* (If we send this request to the server, we +* want an absolute expression rather than a +* local-ref-relative expression.) +*/ + free((char *)filter_options->raw_value); + filter_options->raw_value = + xstrfmt("sparse:oid=%s", + oid_to_hex(_oid)); + filter_options->sparse_oid_value = + oiddup(_oid); + } else { + /* +* We could not turn the into an +* OID. Leave the raw_value as is in case +* the server can parse it. (It may refer to +
[PATCH v2 5/6] rev-list: add list-objects filtering support
From: Jeff HostetlerTeach rev-list to use the filtering provided by the traverse_commit_list_filtered() interface to omit unwanted objects from the result. This feature is intended to help with partial clone. Object filtering is only allowed when one of the "--objects*" options are used. When the "--filter-print-omitted" option is used, the omitted objects are printed at the end. These are marked with a "~". This option can be combined with "--quiet" to get a list of just the omitted objects. Normally, rev-list will stop with an error when there are missing objects. When the "--filter-print-missing" option is used, rev-list will print a list of any missing objects that should have been included in the output (rather than stopping). These are marked with a "?". When the "--filter-ignore-missing" option is used, rev-list will silently ignore any missing objects and continue without error. Add t6112 test. Signed-off-by: Jeff Hostetler --- Documentation/git-rev-list.txt | 6 +- Documentation/rev-list-options.txt | 34 ++ builtin/rev-list.c | 75 +++- t/t6112-rev-list-filters-objects.sh | 225 4 files changed, 337 insertions(+), 3 deletions(-) create mode 100755 t/t6112-rev-list-filters-objects.sh diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt index ef22f17..b8a3a5b 100644 --- a/Documentation/git-rev-list.txt +++ b/Documentation/git-rev-list.txt @@ -47,7 +47,11 @@ SYNOPSIS [ --fixed-strings | -F ] [ --date=] [ [ --objects | --objects-edge | --objects-edge-aggressive ] - [ --unpacked ] ] + [ --unpacked ] + [ --filter= ] ] +[ --filter-print-missing ] +[ --filter-print-omitted ] +[ --filter-ignore-missing ] [ --pretty | --header ] [ --bisect ] [ --bisect-vars ] diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 13501e1..9233134 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -706,6 +706,40 @@ ifdef::git-rev-list[] --unpacked:: Only useful with `--objects`; print the object IDs that are not in packs. + +--filter=:: + Only useful with one of the `--objects*`; omits objects (usually + blobs) from the list of printed objects. The '' + may be one of the following: ++ +The form '--filter=blob:none' omits all blobs. ++ +The form '--filter=blob:limit=[kmg]' omits blobs larger than n bytes +or units. The value may be zero. Special files matching '.git*' are +alwayse included, regardless of size. ++ +The form '--filter=sparse:oid=' uses a sparse-checkout +specification contained in the object (or the object that the expression +evaluates to) to omit blobs not required by the corresponding sparse +checkout. ++ +The form '--filter=sparse:path=' similarly uses a sparse-checkout +specification contained in . + +--filter-print-missing:: + Prints a list of the missing objects for the requested traversal. + Object IDs are prefixed with a ``?'' character. The object type + is printed after the ID. This may be used with or without any of + the above filtering options. + +--filter-ignore-missing:: + Ignores missing objects encountered during the requested traversal. + This may be used with or without any of the above filtering options. + +--filter-print-omitted:: + Only useful with one of the above `--filter*`; prints a list + of the omitted objects. Object IDs are prefixed with a ``~'' + character. endif::git-rev-list[] --no-walk[=(sorted|unsorted)]:: diff --git a/builtin/rev-list.c b/builtin/rev-list.c index c1c74d4..cc9fa40 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -4,6 +4,8 @@ #include "diff.h" #include "revision.h" #include "list-objects.h" +#include "list-objects-filter.h" +#include "list-objects-filter-options.h" #include "pack.h" #include "pack-bitmap.h" #include "builtin.h" @@ -12,6 +14,7 @@ #include "bisect.h" #include "progress.h" #include "reflog-walk.h" +#include "oidset.h" static const char rev_list_usage[] = "git rev-list [OPTION] ... [ -- paths... ]\n" @@ -54,6 +57,15 @@ static const char rev_list_usage[] = static struct progress *progress; static unsigned progress_counter; +static struct list_objects_filter_options filter_options; +static struct oidset missing_objects; +static struct oidset omitted_objects; +static int arg_print_missing; +static int arg_print_omitted; +static int arg_ignore_missing; + +#define DEFAULT_OIDSET_SIZE (16*1024) + static void finish_commit(struct commit *commit, void *data); static void show_commit(struct commit *commit, void *data) @@ -181,8 +193,16 @@ static void finish_commit(struct commit *commit, void *data) static void finish_object(struct
[PATCH v2 3/6] oidset: add iterator methods to oidset
From: Jeff HostetlerAdd the usual iterator methods to oidset. Add oidset_remove(). Signed-off-by: Jeff Hostetler --- oidset.c | 10 ++ oidset.h | 36 2 files changed, 46 insertions(+) diff --git a/oidset.c b/oidset.c index f1f874a..454c54f 100644 --- a/oidset.c +++ b/oidset.c @@ -24,6 +24,16 @@ int oidset_insert(struct oidset *set, const struct object_id *oid) return 0; } +int oidset_remove(struct oidset *set, const struct object_id *oid) +{ + struct oidmap_entry *entry; + + entry = oidmap_remove(>map, oid); + free(entry); + + return (entry != NULL); +} + void oidset_clear(struct oidset *set) { oidmap_free(>map, 1); diff --git a/oidset.h b/oidset.h index f4c9e0f..783abce 100644 --- a/oidset.h +++ b/oidset.h @@ -24,6 +24,12 @@ struct oidset { #define OIDSET_INIT { OIDMAP_INIT } + +static inline void oidset_init(struct oidset *set, size_t initial_size) +{ + return oidmap_init(>map, initial_size); +} + /** * Returns true iff `set` contains `oid`. */ @@ -39,9 +45,39 @@ int oidset_contains(const struct oidset *set, const struct object_id *oid); int oidset_insert(struct oidset *set, const struct object_id *oid); /** + * Remove the oid from the set. + * + * Returns 1 if the oid was present in the set, 0 otherwise. + */ +int oidset_remove(struct oidset *set, const struct object_id *oid); + +/** * Remove all entries from the oidset, freeing any resources associated with * it. */ void oidset_clear(struct oidset *set); +struct oidset_iter { + struct oidmap_iter m_iter; +}; + +static inline void oidset_iter_init(struct oidset *set, + struct oidset_iter *iter) +{ + oidmap_iter_init(>map, >m_iter); +} + +static inline struct object_id *oidset_iter_next(struct oidset_iter *iter) +{ + struct oidmap_entry *e = oidmap_iter_next(>m_iter); + return e ? >oid : NULL; +} + +static inline struct object_id *oidset_iter_first(struct oidset *set, + struct oidset_iter *iter) +{ + oidset_iter_init(set, iter); + return oidset_iter_next(iter); +} + #endif /* OIDSET_H */ -- 2.9.3
[PATCH v2 6/6] pack-objects: add list-objects filtering
From: Jeff HostetlerTeach pack-objects to use the filtering provided by the traverse_commit_list_filtered() interface to omit unwanted objects from the resulting packfile. This feature is intended for partial clone/fetch. Filtering requires the use of the "--stdout" option. Add t5317 test. Signed-off-by: Jeff Hostetler --- Documentation/git-pack-objects.txt | 12 +- builtin/pack-objects.c | 28 ++- t/t5317-pack-objects-filter-objects.sh | 369 + 3 files changed, 407 insertions(+), 2 deletions(-) create mode 100755 t/t5317-pack-objects-filter-objects.sh diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt index 473a161..6786351 100644 --- a/Documentation/git-pack-objects.txt +++ b/Documentation/git-pack-objects.txt @@ -12,7 +12,8 @@ SYNOPSIS 'git pack-objects' [-q | --progress | --all-progress] [--all-progress-implied] [--no-reuse-delta] [--delta-base-offset] [--non-empty] [--local] [--incremental] [--window=] [--depth=] - [--revs [--unpacked | --all]] [--stdout | base-name] + [--revs [--unpacked | --all]] + [--stdout [--filter=] | base-name] [--shallow] [--keep-true-parents] < object-list @@ -236,6 +237,15 @@ So does `git bundle` (see linkgit:git-bundle[1]) when it creates a bundle. With this option, parents that are hidden by grafts are packed nevertheless. +--filter=:: + Requires `--stdout`. Omits certain objects (usually blobs) from + the resulting packfile. See linkgit:git-rev-list[1] for valid + `` forms. + +--filter-ignore-missing: + Ignore missing objects without error. This may be used with + or without and of the above filtering. + SEE ALSO linkgit:git-rev-list[1] diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 6e77dfd..e16722f 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -15,6 +15,8 @@ #include "diff.h" #include "revision.h" #include "list-objects.h" +#include "list-objects-filter.h" +#include "list-objects-filter-options.h" #include "pack-objects.h" #include "progress.h" #include "refs.h" @@ -79,6 +81,9 @@ static unsigned long cache_max_small_delta_size = 1000; static unsigned long window_memory_limit = 0; +static struct list_objects_filter_options filter_options; +static int arg_ignore_missing; + /* * stats */ @@ -2547,6 +2552,15 @@ static void show_commit(struct commit *commit, void *data) static void show_object(struct object *obj, const char *name, void *data) { + /* +* Quietly ignore missing objects when they are expected. This +* avoids staging them and getting an odd error later. If we are +* not expecting them, stage it and let the normal error handling +* deal with it. +*/ + if (arg_ignore_missing && !has_object_file(>oid)) + return; + add_preferred_base_object(name); add_object_entry(obj->oid.hash, obj->type, name, 0); obj->flags |= OBJECT_ADDED; @@ -2816,7 +2830,10 @@ static void get_object_list(int ac, const char **av) if (prepare_revision_walk()) die("revision walk setup failed"); mark_edges_uninteresting(, show_edge); - traverse_commit_list(, show_commit, show_object, NULL); + + traverse_commit_list_filtered(_options, , + show_commit, show_object, NULL, + NULL); if (unpack_unreachable_expiration) { revs.ignore_missing_links = 1; @@ -2952,6 +2969,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) N_("use a bitmap index if available to speed up counting objects")), OPT_BOOL(0, "write-bitmap-index", _bitmap_index, N_("write a bitmap index together with the pack index")), + OPT_PARSE_LIST_OBJECTS_FILTER(_options), + OPT_BOOL(0, "filter-ignore-missing", _ignore_missing, +N_("ignore and omit missing objects from packfile")), OPT_END(), }; @@ -3028,6 +3048,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) if (!rev_list_all || !rev_list_reflog || !rev_list_index) unpack_unreachable_expiration = 0; + if (filter_options.choice) { + if (!pack_to_stdout) + die("cannot use filtering with an indexable pack."); + use_bitmap_index = 0; + } + /* * "soft" reasons not to use bitmaps - for on-disk repack by default we want * diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh new file mode 100755 index 000..4249557 --- /dev/null +++ b/t/t5317-pack-objects-filter-objects.sh @@ -0,0 +1,369 @@ +#!/bin/sh +
[PATCH v2 1/6] dir: allow exclusions from blob in addition to file
From: Jeff HostetlerRefactor add_excludes() to separate the reading of the exclude file into a buffer and the parsing of the buffer into exclude_list items. Add add_excludes_from_blob_to_list() to allow an exclude file be specified with an OID without assuming a local worktree or index exists. Refactor read_skip_worktree_file_from_index() and add do_read_blob() to eliminate duplication of preliminary processing of blob contents. Signed-off-by: Jeff Hostetler --- dir.c | 132 ++ dir.h | 3 ++ 2 files changed, 104 insertions(+), 31 deletions(-) diff --git a/dir.c b/dir.c index 1d17b80..1962374 100644 --- a/dir.c +++ b/dir.c @@ -220,6 +220,57 @@ int within_depth(const char *name, int namelen, return 1; } +/* + * Read the contents of the blob with the given OID into a buffer. + * Append a trailing LF to the end if the last line doesn't have one. + * + * Returns: + *-1 when the OID is invalid or unknown or does not refer to a blob. + * 0 when the blob is empty. + * 1 along with { data, size } of the (possibly augmented) buffer + * when successful. + * + * Optionally updates the given sha1_stat with the given OID (when valid). + */ +static int do_read_blob(const struct object_id *oid, + struct sha1_stat *sha1_stat, + size_t *size_out, + char **data_out) +{ + enum object_type type; + unsigned long sz; + char *data; + + *size_out = 0; + *data_out = NULL; + + data = read_sha1_file(oid->hash, , ); + if (!data || type != OBJ_BLOB) { + free(data); + return -1; + } + + if (sha1_stat) { + memset(_stat->stat, 0, sizeof(sha1_stat->stat)); + hashcpy(sha1_stat->sha1, oid->hash); + } + + if (sz == 0) { + free(data); + return 0; + } + + if (data[sz - 1] != '\n') { + data = xrealloc(data, st_add(sz, 1)); + data[sz++] = '\n'; + } + + *size_out = xsize_t(sz); + *data_out = data; + + return 1; +} + #define DO_MATCH_EXCLUDE (1<<0) #define DO_MATCH_DIRECTORY (1<<1) #define DO_MATCH_SUBMODULE (1<<2) @@ -600,32 +651,22 @@ void add_exclude(const char *string, const char *base, x->el = el; } -static void *read_skip_worktree_file_from_index(const struct index_state *istate, - const char *path, size_t *size, - struct sha1_stat *sha1_stat) +static int read_skip_worktree_file_from_index(const struct index_state *istate, + const char *path, + size_t *size_out, + char **data_out, + struct sha1_stat *sha1_stat) { int pos, len; - unsigned long sz; - enum object_type type; - void *data; len = strlen(path); pos = index_name_pos(istate, path, len); if (pos < 0) - return NULL; + return -1; if (!ce_skip_worktree(istate->cache[pos])) - return NULL; - data = read_sha1_file(istate->cache[pos]->oid.hash, , ); - if (!data || type != OBJ_BLOB) { - free(data); - return NULL; - } - *size = xsize_t(sz); - if (sha1_stat) { - memset(_stat->stat, 0, sizeof(sha1_stat->stat)); - hashcpy(sha1_stat->sha1, istate->cache[pos]->oid.hash); - } - return data; + return -1; + + return do_read_blob(>cache[pos]->oid, sha1_stat, size_out, data_out); } /* @@ -739,6 +780,10 @@ static void invalidate_directory(struct untracked_cache *uc, dir->dirs[i]->recurse = 0; } +static int add_excludes_from_buffer(char *buf, size_t size, + const char *base, int baselen, + struct exclude_list *el); + /* * Given a file with name "fname", read it (either from disk, or from * an index if 'istate' is non-null), parse it and store the @@ -754,9 +799,10 @@ static int add_excludes(const char *fname, const char *base, int baselen, struct sha1_stat *sha1_stat) { struct stat st; - int fd, i, lineno = 1; + int r; + int fd; size_t size = 0; - char *buf, *entry; + char *buf; fd = open(fname, O_RDONLY); if (fd < 0 || fstat(fd, ) < 0) { @@ -764,17 +810,13 @@ static int add_excludes(const char *fname, const char *base, int baselen, warn_on_fopen_errors(fname); else close(fd); - if (!istate || - (buf =
[PATCH v2 0/6] Partial clone part 1: object filtering
From: Jeff HostetlerHere is V2 of the list-object filtering. It replaces [1] and reflect a refactoring and simplification of the original. After much discussion on the "list-object-filter-map" I've replaced it with a regular oidset -- the only need for the map was to store the first observed pathname for each blob, but that itself was of questionable value. I've extended oidmap and oidset to have iterators. These 2 commits could be pulled out and applied on their own, but for now I need them here. There were also several comments on the layout of the filtering API and the layout of the filter source code. I've restructured the filtering routines to put them in the same source file, and made them all static. These are now hidden behind a "factory-like" function with a vtable. This greatly simplifies the code in traverse_commit_list_filtered(). I've added "--filter-ignore-missing" parameter to rev-list and pack-objects to ignore missing objects rather than error out. This allows this patch series to better stand on its own eliminates the need in part 1 for "patch 9" from V1. This is a brute force ignore all missing objects. Later, in part 2 or part 3 when --exclude-promisor-objects is introduced, we will be able to ignore EXPECTED missing objects. Finally, patch 1 in this series is the same [2] which is currently cooking in next. [1] https://public-inbox.org/git/20171024185332.57261-1-...@jeffhostetler.com/ [2] * jh/dir-add-exclude-from-blob (2017-10-27) 1 commit - dir: allow exclusions from blob in addition to file Jeff Hostetler (6): dir: allow exclusions from blob in addition to file oidmap: add oidmap iterator methods oidset: add iterator methods to oidset list-objects: filter objects in traverse_commit_list rev-list: add list-objects filtering support pack-objects: add list-objects filtering Documentation/git-pack-objects.txt | 12 +- Documentation/git-rev-list.txt | 6 +- Documentation/rev-list-options.txt | 34 +++ Makefile | 2 + builtin/pack-objects.c | 28 ++- builtin/rev-list.c | 75 +- dir.c | 132 --- dir.h | 3 + list-objects-filter-options.c | 119 ++ list-objects-filter-options.h | 55 + list-objects-filter.c | 408 + list-objects-filter.h | 84 +++ list-objects.c | 95 ++-- list-objects.h | 2 +- oidmap.h | 22 ++ oidset.c | 10 + oidset.h | 36 +++ t/t5317-pack-objects-filter-objects.sh | 369 + t/t6112-rev-list-filters-objects.sh| 225 ++ 19 files changed, 1664 insertions(+), 53 deletions(-) create mode 100644 list-objects-filter-options.c create mode 100644 list-objects-filter-options.h create mode 100644 list-objects-filter.c create mode 100644 list-objects-filter.h create mode 100755 t/t5317-pack-objects-filter-objects.sh create mode 100755 t/t6112-rev-list-filters-objects.sh -- 2.9.3
[PATCH v2 2/6] oidmap: add oidmap iterator methods
From: Jeff HostetlerAdd the usual map iterator functions to oidmap. Signed-off-by: Jeff Hostetler --- oidmap.h | 22 ++ 1 file changed, 22 insertions(+) diff --git a/oidmap.h b/oidmap.h index 18f54cd..d3cd2bb 100644 --- a/oidmap.h +++ b/oidmap.h @@ -65,4 +65,26 @@ extern void *oidmap_put(struct oidmap *map, void *entry); */ extern void *oidmap_remove(struct oidmap *map, const struct object_id *key); + +struct oidmap_iter { + struct hashmap_iter h_iter; +}; + +static inline void oidmap_iter_init(struct oidmap *map, struct oidmap_iter *iter) +{ + hashmap_iter_init(>map, >h_iter); +} + +static inline void *oidmap_iter_next(struct oidmap_iter *iter) +{ + return hashmap_iter_next(>h_iter); +} + +static inline void *oidmap_iter_first(struct oidmap *map, + struct oidmap_iter *iter) +{ + oidmap_iter_init(map, iter); + return oidmap_iter_next(iter); +} + #endif -- 2.9.3
Re: [PATCH] imap-send: handle NULL return of next_arg()
Am 02.11.2017 um 03:18 schrieb Junio C Hamano: > René Scharfewrites: > >> @@ -807,6 +816,8 @@ static int get_cmd_result(struct imap_store *ctx, struct >> imap_cmd *tcmd) >> if (cmdp->cb.cont || cmdp->cb.data) >> imap->literal_pending = 0; >> arg = next_arg(); >> +if (!arg) >> +arg = ""; > > This is being cute and needs reading of the post-context a bit. > > arg = next_arg(); > +if (!arg) > +arg = ""; > if (!strcmp("OK", arg)) > resp = DRV_OK; > else { > if (!strcmp("NO", arg)) > resp = RESP_NO; > else /*if (!strcmp("BAD", arg))*/ > resp = RESP_BAD; > fprintf(stderr, "IMAP command '%s' returned response (%s) - > %s\n", > !starts_with(cmdp->cmd, "LOGIN") ? > cmdp->cmd : "LOGIN ", > arg, cmd ? cmd : ""); > } > if ((resp2 = parse_response_code(ctx, >cb, cmd)) > resp) > resp = resp2; > > > Because the existing code does not explicitly check for "BAD", we > get RESP_BAD, which is what we want in the end, and the error > message we give has "returned response (%s)" which is filled with > this empty string. > > I notice that when this change matters, i.e. we hit a premature end, > next_arg() that yielded NULL would have cleared cmd. That is OK for > the fprintf() we see above, but wouldn't it hit parse_response_code() > that is shared between good and bad cases? The function starts like > so: > > static int parse_response_code(struct imap_store *ctx, struct > imap_cmd_cb *cb, > char *s) > { > struct imap *imap = ctx->imap; > char *arg, *p; > > if (*s != '[') > return RESP_OK; /* no response code */ > > so we will get an immediate NULL dereference, it appears. Good catch. The NULL check in the fprintf() call (two lines above) makes it obvious already that cmd can be NULL. So parse_response_code() needs to learn to handle NULL passed as its third parameter. And since "no response code" yields "RESP_OK" I guess that this should be an appropriate reaction to s == NULL as well. RFC 3501 seems to agree (response codes are optional): 7.1.Server Responses - Status Responses Status responses are OK, NO, BAD, PREAUTH and BYE. OK, NO, and BAD can be tagged or untagged. PREAUTH and BYE are always untagged. Status responses MAY include an OPTIONAL "response code". A response code consists of data inside square brackets in the form of an atom, possibly followed by a space and arguments. The response code contains additional information or status codes for client software beyond the OK/NO/BAD condition, and are defined when there is a specific action that a client can take based upon the additional information. The follow-up patch below makes sense in this light, but I wonder why it hasn't been necessary so far. Do most IMAP servers actually send response codes? Or at least some other string after a status response? Are no users of git imap-send left? I have no way to test any of that. :-( René -- >8 -- Subject: [PATCH 2/1] imap-send: handle missing response codes gracefully Response codes are optional. Exit parse_response_code() early if it's passed a NULL string, indicating that we reached the end of the reply. This avoids dereferencing said NULL pointer. Noticed-by: Junio C Hamano Signed-off-by: Rene Scharfe --- imap-send.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imap-send.c b/imap-send.c index 0031566309..12cc4ea4c8 100644 --- a/imap-send.c +++ b/imap-send.c @@ -684,7 +684,7 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb, struct imap *imap = ctx->imap; char *arg, *p; - if (*s != '[') + if (!s || *s != '[') return RESP_OK; /* no response code */ s++; if (!(p = strchr(s, ']'))) { -- 2.15.0
[BUG] git clean -d is too greedy
Hi all, Since commit 6b1db4310 it is possible to make git clean -d to remove nested git repositories if 1) .gitignore exists in nested repo (either tracked or untracked) 2) .gitignore is excluded Regarding to 2) it doesn't matter if .gitignore is excluded from (another) .gitignore or from command-line (but I assume they populate same ignore list so that's just stating the obvious). To demonstrate this I can run the following commands: # git init -q foo && cd foo # git init -q bar && cd bar # touch .gitignore bar # git add bar && git commit -qm asd && cd .. # git clean -e .gitignore -dn Would remove bar/bar Pre 6b1db4310, and if .gitignore isn't exclued, nested repo is correctly skipped: # git clean -dn Would skip repository bar/ It probably isn't very common use case to exclude .gitignore but nevertheless this has been broken for a while, and to make things worse it can wipe out lots of uncommitted changes. -- Hermanni
Re: [PATCH 3/3] mingw: document the experimental standard handle redirection
Hi Junio, On Thu, 2 Nov 2017, Junio C Hamano wrote: > Johannes Schindelinwrites: > > >> > I feel this is the wrong way round. `>/dev/null` may sound very intuitive > >> > to you, but this feature is Windows only. Guess three times how intuitive > >> > it sounds to Windows developers to write `>/dev/null` if you want to > >> > suppress output... > >> > >> It would be just as intuitive to write '2>&1' for dup-redirection, > > > > No. You misunderstand. I was mainly concerned with the `/dev/null`. Every > > Windows developer knows what `>file.txt` means, and many know what > > `2>error.txt` means. But `/dev/null` is not Windows, period. > > Actually I did know that much. > > If I was correct in assuming that "2>&1" is just as foreign as > ">/dev/null", then we should be shunning "2>&1" just like we shun > ">/dev/null". That was all I meant to say. Did you know that `2>&1` works in Powershell? > Are you saying "2>&1" is just as likely to be known as ">file.txt" > and my assumption of foreignness of "2>&1" was incorrect? > > Side note: would ">NUL" look more familiar, I wonder, and > can stand for ">/dev/null" for the target audience? > > > ... It is so not > > Windows that Git itself translates it to `NUL` (which you Linux die-hards > > won't have a clue about, I would wager a bet). > > Ah, you lost your bet. When can I collect ;-)? As soon as we meet in person again. Ciao, Dscho
Git libsecret No Unlock Dialog Issue
When using Git on Fedora with locked password store credential-libsecret asks for username/password instead of displaying the unlock dialog. If the store is unlocked credential helper gets the credentials from the store though. -- Regards, Yaroslav Sapozhnyk
COMPENSATION SCAM VICTIM'S $5,000 EVERYDAY,
Attn: Beneficiary, I write to inform you that we already issued those documents to accompany your $5,000 payment each day. But the only problem we are having right here is your personal signatures which the Federal Administer of Fund Benin Republic requested that you must sign those documents before we can transfer funds to you. However, I told the officer in charge that it will not be necessary for you to come down here due to your occupation or some other thing that may not allow you to come down here to sign those documents yourself. The Minster Administrator of Fund said that you should get an attorney to sign on your behalf if you are unable to come down here in person. I think this way is the best and the only way forward for you to receive the funds. I have negotiated with an attorney who would sign on your behalf. His name is Sam Floyd. According to him you are to pay for the accredited attorney. He is charging $110.00 to be paid before he gets those documents signed in your favor. I told him to consider signing those documents on your behalf today with a promise that you would pay him back from the $5,000 payment you suppose to receive tomorrow morning if he gets those documents signed today. And his respond is that you have to pay the accredited attorney fee of $110.00 before signing those documents. Well, I asked him for the very last time if he could allow you pay half of his fee today with a promise that you would pay him the Remain balance tomorrow from your $5,000 payment at the western union office once you pick up the $5,000 payment. Well, he said that you should pay the half of the fee $55.00 through western union today. He want me to ask you if you would be so kind to pay him back the remain $55.00 balance at the western union office once you picked up the $5,000 pay out each day. I am hereby this writing to ask you to let me know if you would pay his balance immediately you pick up the $5,000 payment. He wants you to pay half of $55.00 today if you are so kind to pay the balance at the western union office once you picked up the $5,000 payment tomorrow. But if you are not kind enough to pay his balance after picking the first transfer just do not bother yourself not to reply because I won't see another attorney here to do this. Here is information for you to pay $55.00 half of his fee through western union today. Send the half of the fee $55.00 today via Money Gram or Western Union Here is our agent information here in USA you will send the fee to him below: RECEIVER NAME:. . BURL WILLIAMS CITY:. . . . MARICOPA STATE:..ARIZONA COUNTRY:. . USA TEST QUESTION:. GOD TEST ANSWER:. . IS GOOD AMOUNT:. . $55.00 MTCN. . . . . The attorney will get those documents signed as soon as he confirms the half of his fee from you. I will advice you to pay him the half of $55.00 and let him sign the documents and you will then pay the other balance of $55.00 form your $5,000 payment. I await your compliance. Sincerely, Mr. David Hassn, Western Union Money Transfer Department TELEPHONE NUMBER:+1 (207)536-9017.
Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming
On Thu, Nov 2, 2017 at 2:54 AM, Kaartic Sivaraamwrote: > From: Kaartic Sivaraam > > When trying to rename an inexistent branch to with a name of a branch > that already exists the rename failed specifying the new branch name > exists rather than specifying that the branch trying to be renamed > doesn't exist. > > $ git branch -m tset master > fatal: A branch named 'master' already exists. > > It's conventional to report that 'tset' doesn't exist rather than > reporting that 'master' exists, the same way the 'mv' command does. > > (hypothetical) > $ git branch -m tset master > fatal: branch 'tset' doesn't exist. > > That has the problem that the error about an existing branch is shown > only after the user corrects the error about inexistent branch. > > $ git branch -m test master > fatal: A branch named 'master' already exists. > > This isn't useful either because the user would have corrected this error in > a single go if he had been told this alongside the first error. So, give > more useful error messages by giving errors about old branch name and new > branch name at the same time. This is possible as the branch name validation > functions now return the reason they were about to die, when requested. > > $ git branch -m tset master > fatal: branch 'tset' doesn't exist, and branch 'master' already exists Nicely explained; easily understood. > Note: Thanks to the strbuf API that made it possible to easily construct > the composite error message strings! This may be a problem. See below... > Signed-off-by: Kaartic Sivaraam > --- > diff --git a/builtin/branch.c b/builtin/branch.c > +static void get_error_msg(struct strbuf* error_msg, const char* oldname, > unsigned old_branch_exists, > + const char* newname, enum branch_validation_result > res) > +{ > + const char* connector_string = _(", and "); > + > + if (!old_branch_exists) { > + strbuf_addf(error_msg, _("branch '%s' doesn't exist"), > oldname); > + } > + > + switch (res) { > + case BRANCH_EXISTS_NO_FORCE: > + strbuf_addf(error_msg, "%s", (!old_branch_exists) ? > connector_string : ""); > + strbuf_addf(error_msg,_("branch '%s' already > exists"), newname); > + break; > + case CANNOT_FORCE_UPDATE_CURRENT_BRANCH: > + strbuf_addf(error_msg, "%s", (!old_branch_exists) ? > connector_string : ""); > + strbuf_addstr(error_msg, _("cannot force update the > current branch")); > + break; > + case INVALID_BRANCH_NAME: > + strbuf_addf(error_msg, "%s", (!old_branch_exists) ? > connector_string : ""); > + strbuf_addf(error_msg, _("branch name '%s' is > invalid"), newname); > + break; > + /* not necessary to handle success cases */ > + case BRANCH_EXISTS: > + case BRANCH_DOESNT_EXIST: > + break; > + } > +} Translators can correct me, but this smells like "sentence lego"[1], which we'd like to avoid. Translators lack full context when presented with bits and pieces of a sentence like this, thus the translation may be of poor quality; it may even be entirely incorrect since they don't necessarily know how you will be composing the various pieces. You _might_ be able to able to resolve this by dropping "and" from: "foo is moo, and bar is boo" to turn the error messages into independent clauses: "foo is moo; bar is boo" but I'm no translator, and even that may fail the lego sniff test. A sure-fire way to avoid lego construction would be simply to emit each messages on its own line: fatal: branch 'tset' doesn't exist fatal: branch 'master' already exists (though, you might consider that too ugly). [1]: http://www.gnu.org/software/gettext/manual/gettext.html#Preparing-Strings
Re: [PATCH 1/3] bisect: fix memory leak and document `find_bisection()`
On 2 November 2017 at 05:54, Junio C Hamanowrote: > Martin Ågren writes: > >> `find_bisection()` rebuilds the commit list it is given by reversing it >> and skipping uninteresting commits. The uninteresting list entries are >> leaked. Free them to fix the leak. >> >> While we're here and understand what's going on, document the function. >> In particular, make sure to document that the original list should not >> be examined by the caller. > > Good. Thanks. > > I notice that this has only two callers and both of them do > > revs.commits = find_bisection(revs.commits, ...); > > I wonder if updating its calling convention to > > (void) find_bisection(, ...); > > makes sense. This is obviously outside the scope of this patch. I think it would. I considered it briefly but punted, though I don't really understand why now. In hindsight, it would have saved me some work, since I wouldn't have had to carefully document that the original list mustn't be examined. I'll let this simmer for a few days in case other comments turn up, then do a v2 with a preparatory patch that switches the calling convention as you suggested. Thanks.
Re: [PATCH] reduce_heads: fix memory leaks
On 2 November 2017 at 04:11, Junio C Hamanowrote: > Martin Ågren writes: > >> diff --git a/builtin/merge-base.c b/builtin/merge-base.c >> index 6dbd167d3..b1b7590c4 100644 >> --- a/builtin/merge-base.c >> +++ b/builtin/merge-base.c >> @@ -59,6 +59,8 @@ static int handle_independent(int count, const char **args) >> commit_list_insert(get_commit_reference(args[i]), ); >> >> result = reduce_heads(revs); >> + free_commit_list(revs); >> + >> if (!result) >> return 1; > > The post-context of this hunk continues like so: > > while (result) { > printf("%s\n", oid_to_hex(>item->object.oid)); > result = result->next; > } > return 0; > } > > and we end up leaking "result". This function is directly called from > cmd_merge_base() and its value is returned to main(), so leaking it > is not that a grave offence, but that excuse applies equally well to > revs. Good catch. I even have a patch to address the leak of `result`, except I seem to have sorted it into another pile. For this series I just grepped for "reduce_heads" and didn't stop to think about using UNLEAK, nor about the leaking of `result`. > I can see you are shooting for minimum change in this patch, but if > we were writing this code in a codebase where reduce_heads_replace() > is already available, I would imagine that we wouldn't use two separate > variables, perhaps? The way my other patch addresses the leaking of `result` is that it rewrites the loop to avoid losing the original value of `result`, so that it can be UNLEAK-ed at the very end. (That makes it obvious where the leak happens, compared to adding an UNLEAK a few lines up.) If I do `reduce_heads_replace()`, I'll need to touch the loop anyway, and then I could probably just as well UNLEAK a little while at it. I'll get to this within the next couple of days, then I'll see what it looks like. Thanks for your feedback.