Re: Git crash in Ubuntu 12.04
On Wed, Apr 17, 2013 at 4:28 PM, Sivaram Kannan wrote: > Got an proper dump from git this time. See whether it helps. Probably not because there are no debugging symbols. Not sure how ubuntu packages these symbols.. > I have setup another machine with Ubuntu 12.04 and updated only git and > ported the repo there, after 200 clones no crash so far. Any chance you could publish the repository that causes the crash? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] l10n: de.po: translate 39 new messages
Ralf Thielow writes: >> #: sequencer.c:536 >> #, c-format >> msgid "could not revert %s... %s" >> msgstr "Konnte %s nicht zurücksetzen... %s" >> >> #: sequencer.c:1016 >> msgid "Can't revert as initial commit" >> msgstr "Kann nicht zu initialer Version zurücksetzen." >> >> which I don't really like either now that you mention it -- I would >> re-translate it as 'reset'. But either way they should be consistent. >> > > I'm not sure I understand. We currently translate "reset" as "neu > setzen/umsetzen", > which is fine if it means a branch or HEAD ('git reset'), but for commits? > What about "zurücknehmen"? Sorry for the confusion -- what I meant is: given only "zurücksetzen" and no context, I would probably infer that the original message related to 'reset'. Which it doesn't, so that would be confusing. "Zurücknehmen" works for me, or in the same vein you could try "widerrufen". You can add Acked-by: Thomas Rast when you reroll. Thanks for your work! -- Thomas Rast trast@{inf,student}.ethz.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] l10n: de.po: translate 39 new messages
Ralf Thielow writes: > Hi, > > Thanks for review! > > On Mon, Apr 15, 2013 at 9:26 PM, Christian Stimming wrote: > >> Thanks for the regular update! This is great work. >> >> One issue with the plural form messages, though: >> >> Am Montag, 15. April 2013, 18:27:40 schrieb Ralf Thielow: >> > #: bundle.c:186 >> > -#, fuzzy, c-format >> > +#, c-format >> > msgid "The bundle contains this ref:" >> > msgid_plural "The bundle contains these %d refs:" >> > -msgstr[0] "Das Paket enthält %d Referenz" >> > -msgstr[1] "Das Paket enthält %d Referenzen" >> > +msgstr[0] "Das Paket enthält diese Referenz:" >> > +msgstr[1] "Das Paket enthält diese %d Referenzen:" >> >> The msgstr[0] must still contain a %d conversion specifier (which will be >> filled with the number 1) even though the translated sentence wouldn't need >> the 1 anymore. The previous msgstr[0] was correct; the English singular >> msgid >> is not. >> > That made me wonder, too. I've played around a bit with this, and it seems > to be OK as long as one of those strings contain at least one format > specifier. C printf() only knows about the number and types of arguments from the format string, so *ignoring* arguments is not a problem for correctness. -- Thomas Rast trast@{inf,student}.ethz.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/33] t3210: test for spurious error messages for dangling packed refs
On 04/17/2013 01:22 AM, Junio C Hamano wrote: > Michael Haggerty writes: > >> would be that it expires *everything*. But in fact it seems to only >> expire things that are at least one second old, which doesn't seem at >> all useful in the real world. "--expire=all" is accepted without >> complaint but doesn't do what one would hope. > > Perhaps that is worth fixing, independent of this topic. > > Approxidate gives the current time for anything it does not > understand, and that is how --expire=all is interpreted as "anything > older than now". For that matter, even a string "now" has long been > interpreted as the current time with the same "I do not understand > it, so I'll give you the current timestamp" logic, until we added an > official support for "now" at 93cfa7c7a85e (approxidate_careful() > reports errorneous date string, 2010-01-26) for entirely different > reasons. > > A completely untested patch for your enjoyment... I enjoy it :-) But it would be better to put the the function in the date module ("approxidate_expiry_careful()"?) and also use it from other places where an expiry date is being parsed, like prune --expire= reflog expire --expire=/--expire-unreachable= gc --prune= (maybe there are others). And the special values "all" etc. would need to be documented. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
On Wed, Apr 17, 2013 at 09:22:01AM +0400, Ivan Lyapunov wrote: > I checked René Scharfe's patch and it works - no more git log crash. > Also I checked a broken commits by git show and the most of them > created by me. I can confirm I never used anyting else except console > git commit or netbeans gui to commit, which is just git gui wrapper > tool. Doesn't NetBeans use JGit[1]? That makes it a bit more than a wrapper for the Git command line tools. [1] http://eclipse.org/jgit/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
So, I read through git-stash.sh a little more, and found the following: 1. Any stash that can be shown can be applied, but not necessarily popped or dropped (as the documentation indicates). The reason for this is simple: a pop/drop attempts to clear the entry in the stash reflog as well, but all stashes need to have a corresponding reflog entry (for instance, those created with 'stash create'). 2. IS_STASH_LIKE is a misnomer: all it checks is that the given is a merge commit. As a result, you can 'stash show' and 'stash apply' any merge commit. Should we attempt to tighten this somehow, or are we okay with the stash being just another merge commit? Check for a special commit message perhaps? Brandon Casey wrote: > You can create a stash without modifying the refs/stash reflog using > 'sha1=`git stash create`' and then later apply it using 'git stash > apply --index $sha1'. You'll have to reset the work directory > yourself though since 'git stash create' does not do so. The stash > created this way is just a dangling commit so it will have a lifetime > according to the gc.pruneexpire (default 2 weeks currently). Thanks, but I was worried more about reachability of the commit: if I create a ref to it in refs/stashes/* like I suggested, it wouldn't expire until that ref was gone. Then again, I suppose a ref is unnecessary for a temporary stash. Yeah, I can store the SHA-1 hex of the dangling commit in my caller's $state_dir, and apply it from there later. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "What's cooking" between #05 and #06
On Tue, Apr 16, 2013 at 04:52:14PM -0700, Junio C Hamano wrote: > > * jk/remote-helper-with-signed-tags (2013-04-15) 3 commits > > - transport-helper: add 'signed-tags' capability > > - transport-helper: pass --signed-tags=warn-strip to fast-export > > - fast-export: add --signed-tags=warn-strip mode > > There were some comments on the noisiness of the warning output, but > it appears that everybody involved in the area is basically happy > with the direction this series goes in, so I'll expect a reroll and > then merge it to 'next'. What do you expect to change in the reroll? The only comments I've seen have been about the warning output it seems to me that we've agreed to leave that as it is. Have I missed something? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/33] repack_without_ref(): silence errors for dangling packed refs
On 04/15/2013 07:39 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> Stop emitting an error message for dangling packed references found >> when deleting another packed reference. See the previous commit for a >> longer explanation of the issue. >> >> Change repack_without_ref_fn() to silently ignore dangling packed >> references. >> >> Signed-off-by: Michael Haggerty > > Somehow this feels as if it is sweeping the problem under the rug. > > If you ignore a ref for which a loose ref exists when you update a > packed refs file, whether the stale "packed" one points at an object > that is still there or an object that has been garbage collected, > you would not even have to check if the "ref" resolves to object or > anything like that, no? > > Am I missing something? > > This one feels iffy in the otherwise pleasant-to-read series. The usual situation when this code would be triggered would be that the packed reference is overridden by a loose ref and points at an object that has been garbage collected. In that case it is definitely incorrect to emit an error message. But the fact that we don't explicitly verify that there is an overriding loose reference means that it is possible that the failure to resolve the packed ref comes from some kind of repository corruption, and you are correct that such a problem would be swept under the rug by my change. I've been trying to minimize the extra work that repack_without_ref() needs to do to write peeled references, to avoid stretching out the delay that can now occur when deleting a reference. Thus I was trying to save a check of loose references during this operation. But I guess I agree that a little bit more caution would be prudent. I can think of a few ways to avoid sweeping possible indications of repo corruption under the rug, in order of increasing run-time: 1. If a packed ref's SHA-1 cannot be resolved, write the packed ref to the new packed-refs file anyway with SHA-1 but without a peeled value. This would avoid having to check the loose references and avoid erasing possible evidence of corruption, but would delay an actual check for corruption until a later time. It would be a quick fix, effectively kicking the can down the road instead of sweeping it under the rug. Minor pitfall: a reference that is listed without a peeled value in a fully-peeled pack-refs file tells future readers that the corresponding SHA-1 *cannot* be peeled. IF the named object would somehow reappear in the repository (e.g., via a fetch) and IF the object is peelable and IF there is in fact no loose ref overriding the packed ref, then the final result would be that one form of corruption (reference points to non-existent object) would be converted to another form (reference falsely believed to be non-peelable). I think this is an acceptable risk because (a) it would only happen in an unlikely series of events in a repo that was already corrupt, and (b) because falsely believing a reference to be non-peelable wouldn't have terrible consequences. 2. Whenever a packed reference cannot be resolved to an object, verify that there is indeed a loose reference overriding it; if not, emit an error and in either case omit the packed ref from the output. 3. Check for an overriding loose reference *before* trying to peel a packed reference, and omit any overridden loose references from the output packed-refs file. This would be close to running "pack-refs --no-prune" without the "is_tag_ref" test and with reuse of available peeled values. This approach would tidy up the packed-refs file a bit more than (2) because it would cause the deletion of more overridden packed refs, but only as part of first peeling them, which should only happen once in a repo, and only if the first peeling occurs within repack_without_ref() as opposed to an explicit pack_refs(). So it's a negligible improvement over (2). 4. Further along the "correctness" spectrum, one could check for overriding loose references *every* time the packed-refs file is rewritten by repack_without_ref(), even for references whose peeled values are already known. But this would add overhead to every deletion of a packed reference, which is probably not justified. I'm worried that implementing 2-4 would introduce new race conditions of the type that Peff discovered recently, unless we fix the locking policy first (which is also on my TODO list). So my suggestion is to implement 1 now and implement 2 sometime in the future. Opinions? Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Mon, Apr 15, 2013 at 01:28:53PM -0700, Junio C Hamano wrote: > Here are the topics that have been cooking. Commits prefixed with > '-' are only in 'pu' (proposed updates) while commits prefixed with > '+' are in 'next'. > [...] > -- > [New Topics] > [...] > * lf/read-blob-data-from-index (2013-04-15) 3 commits > (merged to 'next' on 2013-04-15 at 09f92c6) > + convert.c: Remove duplicate code > + Add size parameter to read_blob_data_from_index_path() > + Add public function read_blob_data_from_index_path() > > Reduce duplicated code between convert.c and attr.c. > > Will merge to 'master'. Not sure if you care but the commit messages of these are all wrong now that you squashed your API fix into the first commit. They all refer to read_blob_data_from_index_path() instead of read_blob_data_from_index() and most of the details mentioned in the first commit of this series no longer apply... Just saying :) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
netbeans use some integrated git wrapper and I don't know about JGit source base or not. In Eclipse we use Egit. Also all broken commits limited to november 2012, but we still continue to use the same development environments without any problems Ivan 2013/4/17 John Keeping : > On Wed, Apr 17, 2013 at 09:22:01AM +0400, Ivan Lyapunov wrote: >> I checked René Scharfe's patch and it works - no more git log crash. >> Also I checked a broken commits by git show and the most of them >> created by me. I can confirm I never used anyting else except console >> git commit or netbeans gui to commit, which is just git gui wrapper >> tool. > > Doesn't NetBeans use JGit[1]? That makes it a bit more than a wrapper > for the Git command line tools. > > [1] http://eclipse.org/jgit/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
On Wed, 17 Apr 2013 13:14:48 +0400 Ivan Lyapunov wrote: > netbeans use some integrated git wrapper and I don't know about JGit > source base or not. In Eclipse we use Egit. Also all broken commits > limited to november 2012, but we still continue to use the same > development environments without any problems Does "the same" also mean "of the same version"? I mean that if, say, you managed to update netbeans after November, 2012 that would explain a lot as the update might just silently pull a fix for the Git-interfacing code. The same might apply to Git itself as well. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 11/13] pretty: support padding placeholders, %< %> and %>
On Wed, Apr 17, 2013 at 6:41 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> +delete_trailing_dollar() { >> + sed 's/\$$//' >> +} > > This is what we have qz_to_tab_space for, isn't it? Thanks! I looked for something like this, but my eyes focused on similar sed line and forgot the "tr" -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Junio C Hamano writes: > * jc/add-2.0-delete-default (2013-03-08) 3 commits > - git add ... defaults to "-A" > (merged to 'next' on 2013-04-05 at 199442e) > + git add: start preparing for "git add ..." to default to "-A" > + builtin/add.c: simplify boolean variables > > In Git 2.0, "git add pathspec" will mean "git add -A pathspec". If > you did this in a working tree that tracks dir/lost and dir/another: > > $ rm dir/lost > $ edit dir/another > $ git add dir > > The last step will not only notices and records updated > dir/another, but also notices and records the removal of dir/lost > in the index. > > Start training the users for this change to say --no-all when they > want to ignore the removal to smooth the transition hump. > > Will merge to 'master' the early bits and cook the rest in 'next' until Git > 2.0. The warning triggers in some cases where it shouldn't, relating to submodules: $ git submodule add gito...@git.csa.inf.ethz.ch:domjudge.git domjudge Adding existing repo at 'domjudge' to the index warning: In Git 2.0, 'git add ...' will also update the index for paths removed from the working tree that match the given pathspec. If you want to 'add' only changed or newly created paths, say 'git add --no-all ...' instead. It also seems to hint that the problem is with giving a 'pathspec', but in fact in the case of a "proper" pathspec (that isn't an existing path) it does *not* trigger, even though it probably should: $ git ls-files foo $ rm foo $ git add 'f*' $ git status # On branch master # Changes not staged for commit: # (use "git add/rm ..." to update what will be committed) # (use "git checkout -- ..." to discard changes in working directory) # # deleted:foo # no changes added to commit (use "git add" and/or "git commit -a") $ git add -A 'f*' $ git status # On branch master # Changes to be committed: # (use "git reset HEAD ..." to unstage) # # deleted:foo # # Untracked files not listed (use -u option to show untracked files) That's of course assuming that you want to unconditionally make -A the default. -- Thomas Rast trast@{inf,student}.ethz.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 10/13] pretty: add %C(auto) for auto-coloring
On Wed, Apr 17, 2013 at 7:33 AM, Junio C Hamano wrote: >> @@ -1005,7 +1006,15 @@ static size_t format_commit_one(struct strbuf *sb, /* >> in UTF-8 */ >> /* these are independent of the commit */ >> switch (placeholder[0]) { >> case 'C': >> - return parse_color(sb, placeholder, c); >> + if (!prefixcmp(placeholder + 1, "(auto)")) { >> + c->auto_color_next = 1; >> + return 7; >> + } else { >> + int ret = parse_color(sb, placeholder, c); >> + if (ret) >> + c->auto_color_next = 0; >> + return ret; >> + } > > This is to handle a corrupt input, e.g. "%C(auto)%Cbleu%H" where > (perhaps deprecated) "%Cblue" is misspelled, and parse_color() > returns 0 without consuming any byte. > > Does it make sense not to turn auto off in such a case? We don't have any mechanism to report invalid %C. Instead we let them through as literals. If they are literals, they should not have any side effects. So I think it makes sense not to turn off things. > Otherwise the above would become > > if (!prefixcmp(placeholder + 1, "(auto)")) { > c->auto_color_next = 1; > return 7; /* consumed 7 bytes, "C(auto)" */ > } > c->auto_color_next = 0; > return parse_color(sb, placeholder, c); > > which may be simpler. When we see %C, previous %C(auto) is > cancelled. If we do this, maybe we could show invalid %C with blinking. Quite catchy and might make the user wonder why. Of course it won't work without coloring. But who would add %C in that case. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] clone: introduce clone.submoduleGitDir to relocate $GITDIR
On Sun, Apr 14, 2013 at 5:23 AM, Ramkumar Ramachandra wrote: > This configuration variable comes into effect when 'git clone' is > invoked inside an existing git repository's worktree. When set, > instead of cloning the given repository as-is, it relocates the gitdir > of the repository to the path specified by this variable. This > setting is especially useful when working with submodules. What if I clone a repo then realize it was a mistake and remove it? With current clone, a "rm -rf" would do. With this, I'll need to figure out which subdir in the top .git contains the repo I want to remove. I'm not sure how "git submodule" handles this case though (i.e. total submodule ignorant speaking..) -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
missed a list sorry for dup I ment the same because git changes the version too. Also netbeans/eclipse upgrade are not synced, because of different products. So the same ment only names, not versions. But in deep search another repos I found the broken commits in Eclipse repo dated by 13 march 2013. Can them produced because of previous broken commits? And probably you can be right about java git wrappers can produce this issues, I'll try to make a broken repo from scratch later. Ivan 2013/4/17 Ivan Lyapunov : > I ment the same because git changes the version too. Also > netbeans/eclipse upgrade are not synced, because of different > products. So the same ment only names, not versions. But in deep > search another repos I found the broken commits in Eclipse repo dated > by 13 march 2013. Can them produced because of previous broken > commits? And probably you can be right about java git wrappers can > produce this issues, I'll try to make a broken repo from scratch > later. > Ivan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/7] Rework git core for native submodules
On Mon, Apr 8, 2013 at 7:23 AM, Jonathan Nieder wrote: > Ramkumar Ramachandra wrote: > >> It's about the core object code of git parsing links, as >> opposed to a fringe submodule.c/ submodule.sh parsing .gitmodules. > > What's stopping the core object code of git parsing .gitmodules? What > is the core object code? How does this compare to other metadata > files like .gitattributes and .gitignore? Somewhat related to the topic. Why can't .gitattributes be used for storing what's currently in .gitmodules? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] clone: introduce clone.submoduleGitDir to relocate $GITDIR
Duy Nguyen wrote: > What if I clone a repo then realize it was a mistake and remove it? > With current clone, a "rm -rf" would do. With this, I'll need to > figure out which subdir in the top .git contains the repo I want to > remove. I'm not sure how "git submodule" handles this case though > (i.e. total submodule ignorant speaking..) Currently, submodules relocate the GITDIR of submodules to .git/modules. So, my proposed patch doesn't make the situation any worse. In fact, it improves the situation because you're guaranteed that all your GITDIRs will be in ~/bare (or whatever your core.submoduleGitDir is), as opposed to a complex path in .git/modules of your containing superproject. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] clone: introduce clone.submoduleGitDir to relocate $GITDIR
On Wed, Apr 17, 2013 at 8:53 PM, Ramkumar Ramachandra wrote: > Duy Nguyen wrote: >> What if I clone a repo then realize it was a mistake and remove it? >> With current clone, a "rm -rf" would do. With this, I'll need to >> figure out which subdir in the top .git contains the repo I want to >> remove. I'm not sure how "git submodule" handles this case though >> (i.e. total submodule ignorant speaking..) > > Currently, submodules relocate the GITDIR of submodules to > .git/modules. So, my proposed patch doesn't make the situation any > worse. In fact, it improves the situation because you're guaranteed > that all your GITDIRs will be in ~/bare (or whatever your > core.submoduleGitDir is), as opposed to a complex path in .git/modules > of your containing superproject. No, submodule code does not change "git clone". If I'm not mistaken, submodule will not kick in until you type "git submodule something". If I turn clone.submoduleGitDir on, how can I undo my mistake in a user friendly way? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/7] Rework git core for native submodules
Duy Nguyen wrote: > Somewhat related to the topic. Why can't .gitattributes be used for > storing what's currently in .gitmodules? It can. It's just a small syntax change from "key = value" attributes inside a toplevel [submodule ] section separated by newlines, to a path marked with multiple "key=value" attributes separated by whitespace. However, we don't want to make this change because these submodule attributes are somewhat "different" from .gitattributes attributes. Roughly speaking, the current .gitmodules design treats submodule directories as "directories with special attributes", with two differences: these directories have a special mode in the index, and a commit object is created in the database to represent the "partial state" of this submodule. If you think about it, the information stored in the commit object is no less/ no more important than the path-attribute mapping in .gitmodules. I was arguing for using a special OBJ_LINK to represent the full state of the submodule, and doing away with the attributes altogether, but not everyone agrees. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] clone: introduce clone.submoduleGitDir to relocate $GITDIR
Duy Nguyen wrote: > No, submodule code does not change "git clone". If I'm not mistaken, > submodule will not kick in until you type "git submodule something". > If I turn clone.submoduleGitDir on, how can I undo my mistake in a > user friendly way? So, if you currently want to add a submodule, you have to 'git submodule add', which runs clone internally apart from other things. How do you undo this mistake? What I'm essentially proposing is to give the job of cloning back to clone, and the job of adding back to add, instead of creating an unnatural abstraction over them using 'git submodule add'. The point being: why would you ever _want_ to clone inside a worktree unless you intend to add a submodule? In other words, you intent for running a 'git clone' inside a worktree is exactly the same as your intent for running a 'git submodule add' inside a worktree. Ofcourse, if you have a fringe case where that was _not_ your intent, we'll provide a command-line switch to turn off the relocation for that clone. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/7] Rework git core for native submodules
On Wed, Apr 17, 2013 at 9:06 PM, Ramkumar Ramachandra wrote: > Duy Nguyen wrote: >> Somewhat related to the topic. Why can't .gitattributes be used for >> storing what's currently in .gitmodules? > > It can. It's just a small syntax change from "key = value" attributes > inside a toplevel [submodule ] section separated by newlines, to > a path marked with multiple "key=value" attributes separated by > whitespace. However, we don't want to make this change because these > submodule attributes are somewhat "different" from .gitattributes > attributes. > > Roughly speaking, the current .gitmodules design treats submodule > directories as "directories with special attributes", with two > differences: these directories have a special mode in the index, and a > commit object is created in the database to represent the "partial > state" of this submodule. That was my thinking. .gitmodules would break if a user moves the submodule manually (or even if .gitattributes is used) > If you think about it, the information > stored in the commit object is no less/ no more important than the > path-attribute mapping in .gitmodules. I was arguing for using a > special OBJ_LINK to represent the full state of the submodule, and > doing away with the attributes altogether, but not everyone agrees. Include me to those everyone. url feels like a local thing that should not stay in object database (another way of looking at it is like an email address: the primary one fixed in stone in commits with .mailmap for future substitution). Other attributes like .update, .fetchRecursiveSubmodules... definitely should not be stored in object database. I think if they are stored in the submodule's config file, then the manual move problem above will go away. And if you're dead set on storing some submodule state in object database, why not reuse tag object with some nea header lines? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] clone: introduce clone.submoduleGitDir to relocate $GITDIR
On Wed, Apr 17, 2013 at 9:13 PM, Ramkumar Ramachandra wrote: > Duy Nguyen wrote: >> No, submodule code does not change "git clone". If I'm not mistaken, >> submodule will not kick in until you type "git submodule something". >> If I turn clone.submoduleGitDir on, how can I undo my mistake in a >> user friendly way? > > So, if you currently want to add a submodule, you have to 'git > submodule add', which runs clone internally apart from other things. > How do you undo this mistake? Well, it has "submodule" in the command line. My first reaction would be looking for "git submodule rm" or something. > What I'm essentially proposing is to give the job of cloning back to > clone, and the job of adding back to add, instead of creating an > unnatural abstraction over them using 'git submodule add'. The point > being: why would you ever _want_ to clone inside a worktree unless you > intend to add a submodule? In other words, you intent for running a > 'git clone' inside a worktree is exactly the same as your intent for > running a 'git submodule add' inside a worktree. Ofcourse, if you > have a fringe case where that was _not_ your intent, we'll provide a > command-line switch to turn off the relocation for that clone. No, the point is people make mistakes. What do we do in that case? Or will you introduce yet another "gc" command for clean up ~/bare? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/7] Rework git core for native submodules
Duy Nguyen wrote: > Include me to those everyone. url feels like a local thing that should > not stay in object database (another way of looking at it is like an > email address: the primary one fixed in stone in commits with .mailmap > for future substitution). We've been over this several times in earlier emails. That's like saying that a blob should not be stored in the object database, because it is not "fixed in stone" (my OBJ_LINK is just a special kind of blob, as I've repeated many times already). I don't rely on what I "feel", which is why I started out by posting an implementation: the implementation seems to indicate that getting an OBJ_LINK will simplify a lot of things. And that is my primary criterion for deciding: if the implementation is simple and elegant, it must clearly be doing something right. Again, I'm not saying that my approach is Correct and Final. What I'm saying is: "Here's what I've done. Something interesting is going on. It's probably worth a look?" > Other attributes like .update, > .fetchRecursiveSubmodules... definitely should not be stored in object > database. "Coffee and other beverages definitely should be served cold." All very nice to say, but I don't see any rationale. > I think if they are stored in the submodule's config file, > then the manual move problem above will go away. What? The submodule's .git/config? Why should a submodule repository know that it is being used as a submodule? What inherent properties of a git repository change if it is being used as a submodule? > And if you're dead set on storing some submodule state in object > database, I'm not. I'm just saying that it seems to be an interesting alternative approach. Considering that nobody else brought up a real alternative approach, and chose to just keep defending .gitmodules to the death, it's the only other approach we have. > why not reuse tag object with some nea header lines? Or a unified blob, which is currently what we have. The point is to have structured parseable information that the object-parsing code of git code and easily slurp and give to the rest of git-core. Please clear your reading backlog to avoid bringing up the same points over and over again. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/7] Rework git core for native submodules
On Wed, Apr 17, 2013 at 9:56 PM, Ramkumar Ramachandra wrote: >> why not reuse tag object with some nea header lines? > > Or a unified blob, which is currently what we have. The point is to > have structured parseable information that the object-parsing code of > git code and easily slurp and give to the rest of git-core. I think you misunderstood. I meant instead of introducing new object type OBJ_LINK, you can reuse tag object and add new header lines for your purposes. > Please clear your reading backlog to avoid bringing up the same points > over and over again. Yep. I'll shut up until it's cleared. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/7] Rework git core for native submodules
Duy Nguyen wrote: > On Wed, Apr 17, 2013 at 9:56 PM, Ramkumar Ramachandra > wrote: >>> why not reuse tag object with some nea header lines? >> >> Or a unified blob, which is currently what we have. The point is to >> have structured parseable information that the object-parsing code of >> git code and easily slurp and give to the rest of git-core. > > I think you misunderstood. I meant instead of introducing new object > type OBJ_LINK, you can reuse tag object and add new header lines for > your purposes. Oh, I interpreted your typo "nea" as "neat", when you meant "new". Yeah, it's worth exploring: I don't know what backward compatibility benefits it will yield yet. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] contrib/test-hg*.sh: Do not use PYTHON_PATH
The test cases in contrib/remote-helpers use mercurial and python. Before the tests are run, we check if python can import "mercurial" and "hggit". To run this check, python pointed out by PYTHON_PATH is used. This may not work when different python binaries exist, and PYTHON_PATH is not set: Makefile sets it to the default /usr/bin/python The PATH may point out e.g. /sw/bin/python. When /sw/bin/python has the mercurial module installed, but /usr/bin/python has not, the test will not be run. Git respects PYTHON_PATH, hg does not. Use python instead of $PYTHON_PATH to check for installed modules. While at it, split exportX=Y into 2 lines Signed-off-by: Torsten Bögershausen --- contrib/remote-helpers/test-hg-bidi.sh | 14 +- contrib/remote-helpers/test-hg-hg-git.sh | 12 +++- contrib/remote-helpers/test-hg.sh| 2 +- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/contrib/remote-helpers/test-hg-bidi.sh b/contrib/remote-helpers/test-hg-bidi.sh index f368953..9f4a430 100755 --- a/contrib/remote-helpers/test-hg-bidi.sh +++ b/contrib/remote-helpers/test-hg-bidi.sh @@ -15,7 +15,7 @@ if ! test_have_prereq PYTHON; then test_done fi -if ! "$PYTHON_PATH" -c 'import mercurial'; then +if ! python -c 'import mercurial'; then skip_all='skipping remote-hg tests; mercurial not available' test_done fi @@ -68,10 +68,13 @@ setup () { ) >> "$HOME"/.hgrc && git config --global remote-hg.hg-git-compat true - export HGEDITOR=/usr/bin/true + HGEDITOR=/usr/bin/true + export HGEDITOR - export GIT_AUTHOR_DATE="2007-01-01 00:00:00 +0230" - export GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE" + GIT_AUTHOR_DATE="2007-01-01 00:00:00 +0230" + GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE" + + export GIT_AUTHOR_DATE GIT_COMMITTER_DATE } setup @@ -88,7 +91,8 @@ test_expect_success 'encoding' ' git add alpha && git commit -m "add älphà" && - export GIT_AUTHOR_NAME="tést èncödîng" && + GIT_AUTHOR_NAME="tést èncödîng" && + export GIT_AUTHOR_NAME && echo beta > beta && git add beta && git commit -m "add beta" && diff --git a/contrib/remote-helpers/test-hg-hg-git.sh b/contrib/remote-helpers/test-hg-hg-git.sh index 253e65a..5414f81 100755 --- a/contrib/remote-helpers/test-hg-hg-git.sh +++ b/contrib/remote-helpers/test-hg-hg-git.sh @@ -15,12 +15,12 @@ if ! test_have_prereq PYTHON; then test_done fi -if ! "$PYTHON_PATH" -c 'import mercurial'; then +if ! python -c 'import mercurial'; then skip_all='skipping remote-hg tests; mercurial not available' test_done fi -if ! "$PYTHON_PATH" -c 'import hggit'; then +if ! python -c 'import hggit'; then skip_all='skipping remote-hg tests; hg-git not available' test_done fi @@ -103,10 +103,12 @@ setup () { git config --global receive.denycurrentbranch warn git config --global remote-hg.hg-git-compat true - export HGEDITOR=/usr/bin/true + HGEDITOR=/usr/bin/true + export HGEDITOR - export GIT_AUTHOR_DATE="2007-01-01 00:00:00 +0230" - export GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE" + GIT_AUTHOR_DATE="2007-01-01 00:00:00 +0230" + GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE" + export GIT_AUTHOR_DATE GIT_COMMITTER_DATE } setup diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh index d5b8730..8614fa1 100755 --- a/contrib/remote-helpers/test-hg.sh +++ b/contrib/remote-helpers/test-hg.sh @@ -15,7 +15,7 @@ if ! test_have_prereq PYTHON; then test_done fi -if ! "$PYTHON_PATH" -c 'import mercurial'; then +if ! python -c 'import mercurial'; then skip_all='skipping remote-hg tests; mercurial not available' test_done fi -- 1.8.2.282.g4bc7171 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] clone: introduce clone.submoduleGitDir to relocate $GITDIR
Duy Nguyen wrote: > Well, it has "submodule" in the command line. My first reaction would > be looking for "git submodule rm" or something. No, 'git submodule rm' cannot remove the corresponding GITDIR. What if there are other branches that refer to the submodule? What if you want to remove it from this branch and add it to another branch? > No, the point is people make mistakes. What do we do in that case? Or > will you introduce yet another "gc" command for clean up ~/bare? So, people don't make mistakes when they use 'git submodule add', but do make mistakes when using 'git clone'? How has the problem _changed_ with my patch? It's reasonable to point it out as an existing problem, and ask for it to be fixed independent of this discussion, but that is not what you are doing. git cannot read your mind to determine if you made a mistake, if that's what you're asking. No, a gc equivalent won't work either (and there's nothing in the current submodule world), because it is impossible to determine if a workdir is attached to that GITDIR somewhere on your filesystem. You'll have to do _something_ to say that you don't want that GITDIR anymore. It's reasonable to request tooling to help with this task, but your request is entirely different. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Lukas Fleischer writes: > Not sure if you care but the commit messages of these are all wrong now > that you squashed your API fix into the first commit. They all refer to > read_blob_data_from_index_path()... Ouch; thanks for noticing. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Thomas Rast writes: > The warning triggers in some cases where it shouldn't, relating to > submodules: > > $ git submodule add gito...@git.csa.inf.ethz.ch:domjudge.git domjudge > Adding existing repo at 'domjudge' to the index > warning: In Git 2.0, 'git add ...' will also update > the index for paths removed from the working tree that match > the given pathspec. If you want to 'add' only changed > or newly created paths, say 'git add --no-all ...' instead. Good one. So "add" used internally there needs to say --no-add? > It also seems to hint that the problem is with giving a 'pathspec', but > in fact in the case of a "proper" pathspec (that isn't an existing path) > it does *not* trigger, even though it probably should: We have seen users who explicitly say: git add dir after removing dir/del and adding dir/ins got surprised that we do not notice removal of dir/del without "add -A". And it is fairly straight-forward to check and warn for such a case. > That's of course assuming that you want to unconditionally make -A the > default. I thought what the warning text says is what we decided to do eventually. Not "unconditionally", but "with ", but not "only with pathspec that exactly matches an existing path". It appears that we would better discuss and decide such details further, so let's revert the "warn early" bits from master and kick the topic back. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 10/13] pretty: add %C(auto) for auto-coloring
Duy Nguyen writes: >> This is to handle a corrupt input, e.g. "%C(auto)%Cbleu%H" where >> (perhaps deprecated) "%Cblue" is misspelled, and parse_color() >> returns 0 without consuming any byte. >> >> Does it make sense not to turn auto off in such a case? > > We don't have any mechanism to report invalid %C. Instead we let them > through as literals. If they are literals, they should not have any > side effects. So I think it makes sense not to turn off things. Oh, you are right. Thanks for a dose of sanity. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "What's cooking" between #05 and #06
John Keeping writes: > On Tue, Apr 16, 2013 at 04:52:14PM -0700, Junio C Hamano wrote: >> > * jk/remote-helper-with-signed-tags (2013-04-15) 3 commits >> > - transport-helper: add 'signed-tags' capability >> > - transport-helper: pass --signed-tags=warn-strip to fast-export >> > - fast-export: add --signed-tags=warn-strip mode >> >> There were some comments on the noisiness of the warning output, but >> it appears that everybody involved in the area is basically happy >> with the direction this series goes in, so I'll expect a reroll and >> then merge it to 'next'. > > What do you expect to change in the reroll? The only comments I've seen > have been about the warning output it seems to me that we've agreed to > leave that as it is. Have I missed something? You missed the sender timestamp of the message you are responding to, and that of the discussion we later agreed there is nothing to change ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] l10n: de.po: translate 39 new messages
Am Mittwoch, 17. April 2013, 10:09:29 schrieb Thomas Rast: > >> > msgid "The bundle contains this ref:" > >> > msgid_plural "The bundle contains these %d refs:" > >> > > >> > -msgstr[0] "Das Paket enthält %d Referenz" > >> > -msgstr[1] "Das Paket enthält %d Referenzen" > >> > +msgstr[0] "Das Paket enthält diese Referenz:" > >> > +msgstr[1] "Das Paket enthält diese %d Referenzen:" > >> > >> The msgstr[0] must still contain a %d conversion specifier (which will > >> be filled with the number 1) even though the translated sentence > >> wouldn't need the 1 anymore. The previous msgstr[0] was correct; the > >> English singular msgid > >> is not. > > > > That made me wonder, too. I've played around a bit with this, and it > > seems to be OK as long as one of those strings contain at least one > > format specifier. > > C printf() only knows about the number and types of arguments from the > format string, so *ignoring* arguments is not a problem for correctness. Indeed both of you are correct and I learned something new. http://stackoverflow.com/questions/3578970/passing-too-many-arguments-to- printf where the second answer quotes Online C Draft Standard (n1256), section 7.19.6.1, paragraph 2: "If the format is exhausted while arguments remain, the excess arguments are evaluated (as always) but are otherwise ignored." Hence, it is indeed safe to skip unneeded conversion specifiers both in general ngettext messages and also in their respective translation. This is an explanation which has yet to be added to ngettext's documentation. Best Regards, Christian -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] clone: introduce clone.submoduleGitDir to relocate $GITDIR
Ramkumar Ramachandra wrote: > 2. This ugliness complicates implementation of add/ rm/ mv, because > each of them will have to know about this contrived path solution. Why is that? Can't they look at the gitfile or call some helper (that happens to be part of the same binary)? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/test-hg*.sh: Do not use PYTHON_PATH
Hi, Torsten Bögershausen wrote: > Git respects PYTHON_PATH, hg does not. Probably more relevant is that contrib/remote-helpers/git-remote-hg has a shebang line of "#!/usr/bin/env python" and hence does not respect PYTHON_PATH. > Use python instead of $PYTHON_PATH to check for installed modules. Makes sense to me. > While at it, split exportX=Y into 2 lines Would you mind splitting this change (which also looks good) into a separate patch? It makes life debugging, cherry-picking, reading, reverting when needed, and so on easier. Hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Junio C Hamano writes: > Thomas Rast writes: > >> The warning triggers in some cases where it shouldn't, relating to >> submodules: >> >> $ git submodule add gito...@git.csa.inf.ethz.ch:domjudge.git domjudge >> Adding existing repo at 'domjudge' to the index >> warning: In Git 2.0, 'git add ...' will also update >> the index for paths removed from the working tree that match >> the given pathspec. If you want to 'add' only changed >> or newly created paths, say 'git add --no-all ...' instead. > > Good one. So "add" used internally there needs to say --no-add? I think the logic in git-add needs to learn about submodules. The same warning later trigger when you later say 'git add submoduledir', even though that obviously doesn't walk inside the submodule. >> It also seems to hint that the problem is with giving a 'pathspec', but >> in fact in the case of a "proper" pathspec (that isn't an existing path) >> it does *not* trigger, even though it probably should: > > We have seen users who explicitly say: > > git add dir > > after removing dir/del and adding dir/ins got surprised that we do > not notice removal of dir/del without "add -A". And it is fairly > straight-forward to check and warn for such a case. I can see that problem, but along the same lines, why shouldn't I have an expectation that when I say 'git add "*.py"' it removes stuff that I have removed? That's what I tried to show with the f?o example. -- Thomas Rast trast@{inf,student}.ethz.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/7] Rework git core for native submodules
Duy Nguyen writes: > Somewhat related to the topic. Why can't .gitattributes be used for > storing what's currently in .gitmodules? You _could_ use gitattributes to encode, but it goes against what a gitattributes file does or is for. It is a mechanism to associate groups of paths (that may not even exist) to a set of attributes. You could list a single pattern that happens to match a single path and at the implementation level you may be able to make it work, but at the design/philosophical level, it is wrong. We need info on each submodule and we need to key it with the name of the submodule, not with its path. At any given time, a single submodule lives at (at most) one path, so you could still use path as a key in the .gitattributes, but when you need to move the submodule path, you would need to update the entry for the submodule in .gitattributes file by finding a pattern that match the old path and making it a pattern that match the new path. We have a much more suitable file format that we use to associate various values to keys: the config format. Also having a file that is only about submodules and nothing else means we could write a content-aware smart ll-merge driver that can take advantage of the knowledge that it is written in the config format and it talks about submodules. The answer to "why can't" question is "no". No, there is no reason why you can't use it. We don't do it, because it just does not make sense. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/test-hg*.sh: Do not use PYTHON_PATH
Torsten Bögershausen writes: > The test cases in contrib/remote-helpers use mercurial and python. > Before the tests are run, we check if python can import > "mercurial" and "hggit". > To run this check, python pointed out by PYTHON_PATH is used. > This may not work when different python binaries exist, > and PYTHON_PATH is not set: > Makefile sets it to the default /usr/bin/python > The PATH may point out e.g. /sw/bin/python. > When /sw/bin/python has the mercurial module installed, > but /usr/bin/python has not, the test will not be run. > Git respects PYTHON_PATH, hg does not. The above problem analysis looks sensible. > Use python instead of $PYTHON_PATH to check for installed modules. But I am not sure if I agree with the solution. Going back to the analysis, I find this: > This may not work when different python binaries exist, > and PYTHON_PATH is not set: Isn't the real problem that PYTHON_PATH is not set to point at the instance of a python with mercurial module for it? Why not make sure it is set and can be seen by tests correctly? I do not offhand know where in the contrib/remote-helpers/ part the user is expected to tweak PYTHON_PATH variable, and if the variable is correctly arranged to be exported to the environment when running the tests, but your problem analysis tells me that that is the part you would want to fix. If a default-fallback value for PYTHON_PATH is the easiest solution to the problem, you could even solve that in the "export it while running the tests" logic. Perhaps adding PYTHON_PATH ?= python export PYTHON_PATH to contrib/remote-helpers/Makefile and changing nothing else would be a starting point for a more reasonable fix to the issue? > While at it, split exportX=Y into 2 lines That is an important portability fix, but as you said "while at it", it is orthogonal. > > Signed-off-by: Torsten Bögershausen > --- > contrib/remote-helpers/test-hg-bidi.sh | 14 +- > contrib/remote-helpers/test-hg-hg-git.sh | 12 +++- > contrib/remote-helpers/test-hg.sh| 2 +- > 3 files changed, 17 insertions(+), 11 deletions(-) > > diff --git a/contrib/remote-helpers/test-hg-bidi.sh > b/contrib/remote-helpers/test-hg-bidi.sh > index f368953..9f4a430 100755 > --- a/contrib/remote-helpers/test-hg-bidi.sh > +++ b/contrib/remote-helpers/test-hg-bidi.sh > @@ -15,7 +15,7 @@ if ! test_have_prereq PYTHON; then > test_done > fi > > -if ! "$PYTHON_PATH" -c 'import mercurial'; then > +if ! python -c 'import mercurial'; then > skip_all='skipping remote-hg tests; mercurial not available' > test_done > fi > @@ -68,10 +68,13 @@ setup () { > ) >> "$HOME"/.hgrc && > git config --global remote-hg.hg-git-compat true > > - export HGEDITOR=/usr/bin/true > + HGEDITOR=/usr/bin/true > + export HGEDITOR > > - export GIT_AUTHOR_DATE="2007-01-01 00:00:00 +0230" > - export GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE" > + GIT_AUTHOR_DATE="2007-01-01 00:00:00 +0230" > + GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE" > + > + export GIT_AUTHOR_DATE GIT_COMMITTER_DATE > } > > setup > @@ -88,7 +91,8 @@ test_expect_success 'encoding' ' > git add alpha && > git commit -m "add älphà" && > > - export GIT_AUTHOR_NAME="tést èncödîng" && > + GIT_AUTHOR_NAME="tést èncödîng" && > + export GIT_AUTHOR_NAME && > echo beta > beta && > git add beta && > git commit -m "add beta" && > diff --git a/contrib/remote-helpers/test-hg-hg-git.sh > b/contrib/remote-helpers/test-hg-hg-git.sh > index 253e65a..5414f81 100755 > --- a/contrib/remote-helpers/test-hg-hg-git.sh > +++ b/contrib/remote-helpers/test-hg-hg-git.sh > @@ -15,12 +15,12 @@ if ! test_have_prereq PYTHON; then > test_done > fi > > -if ! "$PYTHON_PATH" -c 'import mercurial'; then > +if ! python -c 'import mercurial'; then > skip_all='skipping remote-hg tests; mercurial not available' > test_done > fi > > -if ! "$PYTHON_PATH" -c 'import hggit'; then > +if ! python -c 'import hggit'; then > skip_all='skipping remote-hg tests; hg-git not available' > test_done > fi > @@ -103,10 +103,12 @@ setup () { > git config --global receive.denycurrentbranch warn > git config --global remote-hg.hg-git-compat true > > - export HGEDITOR=/usr/bin/true > + HGEDITOR=/usr/bin/true > + export HGEDITOR > > - export GIT_AUTHOR_DATE="2007-01-01 00:00:00 +0230" > - export GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE" > + GIT_AUTHOR_DATE="2007-01-01 00:00:00 +0230" > + GIT_COMMITTER_DATE="$GIT_AUTHOR_DATE" > + export GIT_AUTHOR_DATE GIT_COMMITTER_DATE > } > > setup > diff --git a/contrib/remote-helpers/test-hg.sh > b/contrib/remote-helpers/test-hg.sh > index d5b8730..8614fa1 100755 > --- a/contrib/remote-helpers/test-hg.sh > +++ b/contrib/remote-helpers/test-hg.sh > @@ -15,7 +15,7 @@ if ! test_have_prereq PYTHON; then > test_done > fi > > -if ! "$PYTHON_PATH" -c 'impor
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Thomas Rast writes: > I can see that problem, but along the same lines, why shouldn't I have > an expectation that when I say 'git add "*.py"' it removes stuff that I > have removed? You _should_ have that expectation. If it does not remove with the code that has been prepared for 2.0 (that is a bit beyond 'next'), then it is a big problem, but I think it does remove the removed python source without "-A", as long as you give a pathspec "*.py" (with quotes around it) that match it. I think it is just the warning code avoiding extra complexity and overhead, if you are talking about not getting warning in the pre-2.0 step that is in 'next'. Patches are very much welcomed, especially the ones that come before I get around to it ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] clone: introduce clone.submoduleGitDir to relocate $GITDIR
Duy Nguyen writes: > No, the point is people make mistakes. What do we do in that case? Or > will you introduce yet another "gc" command for clean up ~/bare? I do not know if it will be a "gc", but we would need a way for the user to say "I no longer need the repository for this submodule kept locally here (I may have to re-clone when I check out a version that needs the submodule)" to free up the .git/modules/ directories in the superproject. We might want to allow "submodule deinit" to also ask for it, but "deinit" will not be the only occasion the user might want it. It is already a problem that needs to be addressed in the current setup. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
Ramkumar Ramachandra writes: > 1. Any stash that can be shown can be applied, but not necessarily > popped or dropped (as the documentation indicates). The reason for > this is simple: a pop/drop attempts to clear the entry in the stash > reflog as well, but all stashes need to have a corresponding reflog > entry (for instance, those created with 'stash create'). Correct. To show or apply, you only need a product of "stash create" that may not be linked anywhere in the refs or reflogs. But you can only pop or drop something on the stash reflog. > 2. IS_STASH_LIKE is a misnomer: all it checks is that the given > is a merge commit. The purpose of the logic is to reject a mistake to name stuff that is clearly not a product of "stash create". The name is just being humble by not claiming "I know this is definitely a product of stash-create" but merely hinting "This smells like such an object"; I am not sure if that is a "misnomer". You are free to try to think of a way to tighten the implemention to reject a random two-or-three parent merge commit that is not a product of "stash create". People already have looked at this code since it was written, and didn't find a reasonable way to tighten it without triggering false negatives, so I wouldn't be surprised if anybody tried it again today and failed. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
Jeff King writes: > What about sane_ident_split in builtin/commit.c? It explicitly rejects a > NULL date. The logic in determine_author_info is a little hard to follow > (it assembles the ident line with fmt_ident and then reparses it), but I > believe it should be catching a bogus line from "commit -c", or from > GIT_AUTHOR_DATE in the environment. Yeah, you are of course right. I noticed that "fmt_ident then parse" sequence a bit funny, too. It seems to manually parse to prepare what it feeds fmt_ident. > As a side note, when determine_author_info sees a bogus ident, it > appears to just silently ignore it, which is probably a bad thing. True, too. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
Am 17.04.2013 08:39, schrieb Jeff King: On Tue, Apr 16, 2013 at 10:26:40PM -0700, Junio C Hamano wrote: Junio C Hamano writes: René Scharfe writes: How about making split_ident_line() a bit friendlier be letting it provide the epoch as default time stamp instead of NULL? Two knee-jerk concerns I have without going back to the callers: * Would that "0" ever be given to the approxidate parser, which rejects ancient dates in numbers-since-epoch format without @ prefix? * Does any existing caller use the NULL as a sign to see the input was without date and act on that information? I looked at all the callers (there aren't that many), and none of them did "Do this on a person-only ident, and do that on an ident with timestamp". So for the callers that ignore timestamp, your patch will be a no-op, and for others that assume there is a timestamp, it will turn a crash/segv into output with funny timestamp. What about sane_ident_split in builtin/commit.c? It explicitly rejects a NULL date. The logic in determine_author_info is a little hard to follow (it assembles the ident line with fmt_ident and then reparses it), but I believe it should be catching a bogus line from "commit -c", or from GIT_AUTHOR_DATE in the environment. Right, so let's keep the NULLs and fix the individual cases. A quick "git grep -W -e date_begin -e date_end -e tz_begin -e tz_end" reveals that there are only the ones we talked about: blame, pretty, commit and -- of course -- ident. And only the first two need fixing. As a side note, when determine_author_info sees a bogus ident, it appears to just silently ignore it, which is probably a bad thing. Shouldn't we by complaining? Or am I mis-reading the code? The code looks complicated, but I just tried it: fmt_ident() dies if you give it an invalid date. René -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
On Wed, Apr 17, 2013 at 07:59:28PM +0200, René Scharfe wrote: > >What about sane_ident_split in builtin/commit.c? It explicitly rejects a > >NULL date. The logic in determine_author_info is a little hard to follow > >(it assembles the ident line with fmt_ident and then reparses it), but I > >believe it should be catching a bogus line from "commit -c", or from > >GIT_AUTHOR_DATE in the environment. > > Right, so let's keep the NULLs and fix the individual cases. A quick > "git grep -W -e date_begin -e date_end -e tz_begin -e tz_end" reveals > that there are only the ones we talked about: blame, pretty, commit > and -- of course -- ident. And only the first two need fixing. Yes, that matches the list I came up with in February. I think we also need to do something about "git cat-file -p", which does not use the split_ident_line parser (but has its own problems with the home-grown parser). > >As a side note, when determine_author_info sees a bogus ident, it > >appears to just silently ignore it, which is probably a bad thing. > >Shouldn't we by complaining? Or am I mis-reading the code? > > The code looks complicated, but I just tried it: fmt_ident() dies if > you give it an invalid date. It does seem like determine_author_info can be greatly simplified, but I haven't looked all that closely. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Junio C Hamano writes: > Thomas Rast writes: > >> I can see that problem, but along the same lines, why shouldn't I have >> an expectation that when I say 'git add "*.py"' it removes stuff that I >> have removed? > > You _should_ have that expectation. > > If it does not remove with the code that has been prepared for 2.0 > (that is a bit beyond 'next'), then it is a big problem, but I think > it does remove the removed python source without "-A", as long as > you give a pathspec "*.py" (with quotes around it) that match it. > > I think it is just the warning code avoiding extra complexity and > overhead, if you are talking about not getting warning in the > pre-2.0 step that is in 'next'. Patches are very much welcomed, > especially the ones that come before I get around to it ;-) I took a brief look at the code, and as you said "add" needs to know about submodules, and the best fix looks to me to take the same approach Jonathan came up with to de-noise the "add -u/-A" topic. That is, to scan the working tree to actually see if we would record removals to the index in 2.0, but not remove them in this current version, and give the warning when the differences in the behaviours matter. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] pretty: handle broken commit headers gracefully
Centralize the parsing of the date and time zone strings in the new helper function show_ident_date() and make sure it checks the pointers provided by split_ident_line() for NULL before use. Reported-by: Ivan Lyapunov Signed-off-by: Rene Scharfe --- The first test case passes on v1.8.1, i.e. it also showed the epoch. The second one passes on v1.8.1 and on master because there already is a NULL check in format_person_part(). pretty.c | 45 - t/t4211-log-corrupt.sh | 42 ++ 2 files changed, 66 insertions(+), 21 deletions(-) create mode 100755 t/t4211-log-corrupt.sh diff --git a/pretty.c b/pretty.c index d3a82d2..c116248 100644 --- a/pretty.c +++ b/pretty.c @@ -393,6 +393,19 @@ static void add_rfc2047(struct strbuf *sb, const char *line, size_t len, strbuf_addstr(sb, "?="); } +static const char *show_ident_date(const struct ident_split *ident, + enum date_mode mode) +{ + unsigned long date = 0; + int tz = 0; + + if (ident->date_begin && ident->date_end) + date = strtoul(ident->date_begin, NULL, 10); + if (ident->tz_begin && ident->tz_end) + tz = strtol(ident->tz_begin, NULL, 10); + return show_date(date, tz, mode); +} + void pp_user_info(const struct pretty_print_context *pp, const char *what, struct strbuf *sb, const char *line, const char *encoding) @@ -401,12 +414,10 @@ void pp_user_info(const struct pretty_print_context *pp, struct strbuf mail; struct ident_split ident; int linelen; - char *line_end, *date; + char *line_end; const char *mailbuf, *namebuf; size_t namelen, maillen; int max_length = 78; /* per rfc2822 */ - unsigned long time; - int tz; if (pp->fmt == CMIT_FMT_ONELINE) return; @@ -438,8 +449,6 @@ void pp_user_info(const struct pretty_print_context *pp, strbuf_add(&name, namebuf, namelen); namelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */ - time = strtoul(ident.date_begin, &date, 10); - tz = strtol(date, NULL, 10); if (pp->fmt == CMIT_FMT_EMAIL) { strbuf_addstr(sb, "From: "); @@ -472,13 +481,16 @@ void pp_user_info(const struct pretty_print_context *pp, switch (pp->fmt) { case CMIT_FMT_MEDIUM: - strbuf_addf(sb, "Date: %s\n", show_date(time, tz, pp->date_mode)); + strbuf_addf(sb, "Date: %s\n", + show_ident_date(&ident, pp->date_mode)); break; case CMIT_FMT_EMAIL: - strbuf_addf(sb, "Date: %s\n", show_date(time, tz, DATE_RFC2822)); + strbuf_addf(sb, "Date: %s\n", + show_ident_date(&ident, DATE_RFC2822)); break; case CMIT_FMT_FULLER: - strbuf_addf(sb, "%sDate: %s\n", what, show_date(time, tz, pp->date_mode)); + strbuf_addf(sb, "%sDate: %s\n", what, + show_ident_date(&ident, pp->date_mode)); break; default: /* notin' */ @@ -688,8 +700,6 @@ static size_t format_person_part(struct strbuf *sb, char part, { /* currently all placeholders have same length */ const int placeholder_len = 2; - int tz; - unsigned long date = 0; struct ident_split s; const char *name, *mail; size_t maillen, namelen; @@ -716,30 +726,23 @@ static size_t format_person_part(struct strbuf *sb, char part, if (!s.date_begin) goto skip; - date = strtoul(s.date_begin, NULL, 10); - if (part == 't') { /* date, UNIX timestamp */ strbuf_add(sb, s.date_begin, s.date_end - s.date_begin); return placeholder_len; } - /* parse tz */ - tz = strtoul(s.tz_begin + 1, NULL, 10); - if (*s.tz_begin == '-') - tz = -tz; - switch (part) { case 'd': /* date */ - strbuf_addstr(sb, show_date(date, tz, dmode)); + strbuf_addstr(sb, show_ident_date(&s, dmode)); return placeholder_len; case 'D': /* date, RFC2822 style */ - strbuf_addstr(sb, show_date(date, tz, DATE_RFC2822)); + strbuf_addstr(sb, show_ident_date(&s, DATE_RFC2822)); return placeholder_len; case 'r': /* date, relative */ - strbuf_addstr(sb, show_date(date, tz, DATE_RELATIVE)); + strbuf_addstr(sb, show_ident_date(&s, DATE_RELATIVE)); return placeholder_len; case 'i': /* date, ISO 8601 */ - strbuf_addstr(sb, show_date(date, tz, DATE_ISO8601)); + strbuf_addstr(sb, show_ident_date(&s, DATE_ISO8601)); return placeholder_len;
[PATCH] blame: handle broken commit headers gracefully
split_ident_line() can leave us with the pointers date_begin, date_end, tz_begin and tz_end all set to NULL. Check them before use and supply the same fallback values as in the case of a negative return code from split_ident_line(). The "(unknown)" is not actually shown in the output, though, because it will be converted to a number (zero) eventually. Signed-off-by: Rene Scharfe --- Minimal patch, test case missing. It's a bit sad that the old commit parser of blame handled Ivan's specific corruption (extra "-<>" after email) gracefully because it used the spaces as cutting points instead of "<" and ">". builtin/blame.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index 86100e9..7770781 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -1375,10 +1375,15 @@ static void get_ac_line(const char *inbuf, const char *what, maillen = ident.mail_end - ident.mail_begin; mailbuf = ident.mail_begin; - *time = strtoul(ident.date_begin, NULL, 10); + if (ident.date_begin && ident.date_end) + *time = strtoul(ident.date_begin, NULL, 10); + else + *time = 0; - len = ident.tz_end - ident.tz_begin; - strbuf_add(tz, ident.tz_begin, len); + if (ident.tz_begin && ident.tz_end) + strbuf_add(tz, ident.tz_begin, ident.tz_end - ident.tz_begin); + else + strbuf_addstr(tz, "(unknown)"); /* * Now, convert both name and e-mail using mailmap -- 1.8.2.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/test-hg*.sh: Do not use PYTHON_PATH
On Wed, Apr 17, 2013 at 9:10 AM, Torsten Bögershausen wrote: > The test cases in contrib/remote-helpers use mercurial and python. > Before the tests are run, we check if python can import > "mercurial" and "hggit". > To run this check, python pointed out by PYTHON_PATH is used. > This may not work when different python binaries exist, > and PYTHON_PATH is not set: > Makefile sets it to the default /usr/bin/python > The PATH may point out e.g. /sw/bin/python. > When /sw/bin/python has the mercurial module installed, > but /usr/bin/python has not, the test will not be run. > > Git respects PYTHON_PATH, hg does not. > Use python instead of $PYTHON_PATH to check for installed modules. And this would fail if the distribution doesn't have a 'python' binary, and instead has python2, python3, etc. > While at it, split exportX=Y into 2 lines Do it in a separate patch. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Tue, Apr 16, 2013 at 5:45 PM, Phil Hord wrote: > On Tue, Apr 16, 2013 at 3:04 PM, Felipe Contreras > wrote: >> On Tue, Apr 16, 2013 at 4:59 AM, Thomas Rast wrote: >>> A cursory look^W^Wreview of the messages in fc/remote-hg: >> >> [skipping irrelevant comments] >> >> I'm sorry, did you actually hit an issue that required to look at the >> commit message to understand where the issue came from? No? Then I >> won't bother with hypotheticals. >> >> If you want to waste your time, by all means, rewrite all my commit >> messages with essays that nobody will ever read. I'm not going to do >> that for some hypothetical case that will never happen. I'm not going >> to waste my time. > > This is not a hypothetical. Almost every time I bisect a regression > in git.git, I find the commit message tells me exactly why the commit > did what it did and what the expected result was. I find this to be > amazingly useful. Do I need to show you real instances of that > happening? No. I promise it did, though. Yes please. Show me one of the instances where you hit a bisect with any of the remote-hg commits mentioned above by Thomas Rast. > Of course, 99% of the commit messages may never be useful to me or > anyone else. But we do not eschew them altogether. The 1% I have to > rely on are nearly always helpful and clear, and that is the part I > care about. And how do you know this will be part of the 1%? You don't. How many times have you tracked regressions in transport helper's import/export functionality? How many times in remote-hg? How many times has *anybody* done so? > If you will not waste your time to write a decent commit message, why > do you waste our time asking us to review and accept ill-defined > patches? Because it *fixes a problem*. And a commit essay doesn't fix any, because nobody will ever go back in history and wonder, hey, what is up with this commit. If somebody does, then I will accept that commit essays are always a must. But it won't happen. > Here, of course, I use the royal "us" as I do not review > your patches. I do not know why that is; I suppose you patch things > outside of my interests, but it may also be that your patches are > simply incomprehensible by design. Yeah, but that's the thing, if you don't understand the code the patches are changing, then how can you know the commit message is sufficient to figure things out when a regression is found? You don't. You can't. Let's face the truth, you are advocating for stopping progress on the name that something might happen sometime in the feature, although most likely won't. When in reality, it just won't. And you are not saying "it would be nice to have full commit essay", you are saying: "without a commit essay this patch should NOT be merged", even more "without a commit essay this patch should NOT be considered a cooking patch". I think the commit message is fine, you don't. So YOU go ahead and write the proper one. If you don't, all you are doing is being an impediment to progress. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
Am 17.04.2013 20:02, schrieb Jeff King: I think we also need to do something about "git cat-file -p", which does not use the split_ident_line parser (but has its own problems with the home-grown parser). Ah, while it prints commit object contents verbatim, it formats the date of tags. And it does it without help from tag.c (or ident.c), which in turn does its own parsing as well. So it looks like we have two more candidates for conversion to split_ident_line() here. René -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/test-hg*.sh: Do not use PYTHON_PATH
On Wed, Apr 17, 2013 at 1:36 PM, Felipe Contreras wrote: > On Wed, Apr 17, 2013 at 9:10 AM, Torsten Bögershausen wrote: >> The test cases in contrib/remote-helpers use mercurial and python. >> Before the tests are run, we check if python can import >> "mercurial" and "hggit". >> To run this check, python pointed out by PYTHON_PATH is used. >> This may not work when different python binaries exist, >> and PYTHON_PATH is not set: >> Makefile sets it to the default /usr/bin/python >> The PATH may point out e.g. /sw/bin/python. >> When /sw/bin/python has the mercurial module installed, >> but /usr/bin/python has not, the test will not be run. >> >> Git respects PYTHON_PATH, hg does not. >> Use python instead of $PYTHON_PATH to check for installed modules. > > And this would fail if the distribution doesn't have a 'python' > binary, and instead has python2, python3, etc. Also, it would fail if mercurial is installed for python2, and python is really python3. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/14] Improve git-status --ignored
Am 15.04.2013 22:25, schrieb Junio C Hamano: > Karsten Blees writes: > >> Am 15.04.2013 21:33, schrieb Junio C Hamano: >>> Junio C Hamano writes: >>> Karsten Blees writes: > This patch series addresses several bugs and performance issues in > .gitignore processing. A 8-patch series ending at 5d765dc7888b (dir.c: git-status: avoid is_excluded checks for tracked files, 2013-03-18) has been cooking in 'next'; in general we won't revert and requeue a new round for a topic that has already merged to 'next'. >> >> I'm sorry to have caused such trouble. I thought you were expecting a reroll >> from this: > > Heh, that was an ancient history. My git time is scarce, so progress is slow...guess I need some performance improvements, too :-) > > It is not such a big deal to revert stuff from 'next'. I've tried > to queue this reroll, but tentatively ejected the result from 'pu' > for today's integration run, as as/check-ignore topic has an > unpleasant conflict with this. > I'll send fixups for #11 and #14, or you can pick the entire series rebased to pu from: https://github.com/kblees/git/tree/kb/improve-git-status-ignored-v2-pu git pull git://github.com/kblees/git.git kb/improve-git-status-ignored-v2-pu HTH, Karsten -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2-pu 11/14] dir.c: replace is_path_excluded with now equivalent is_excluded API
Signed-off-by: Karsten Blees --- builtin/add.c | 5 +--- builtin/check-ignore.c | 16 -- builtin/ls-files.c | 15 +++--- dir.c | 79 -- dir.h | 16 ++ unpack-trees.c | 10 +-- unpack-trees.h | 1 - 7 files changed, 21 insertions(+), 121 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index ddd5e38..050330e 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -419,9 +419,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) if (pathspec) { int i; - struct path_exclude_check check; - path_exclude_check_init(&check, &dir); if (!seen) seen = find_pathspecs_matching_against_index(pathspec); for (i = 0; pathspec[i]; i++) { @@ -429,7 +427,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) && !file_exists(pathspec[i])) { if (ignore_missing) { int dtype = DT_UNKNOWN; - if (is_path_excluded(&check, pathspec[i], -1, &dtype)) + if (is_excluded(&dir, pathspec[i], &dtype)) dir_add_ignored(&dir, pathspec[i], strlen(pathspec[i])); } else die(_("pathspec '%s' did not match any files"), @@ -437,7 +435,6 @@ int cmd_add(int argc, const char **argv, const char *prefix) } } free(seen); - path_exclude_check_clear(&check); } plug_bulk_checkin(); diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c index c00a7d6..a4357fb 100644 --- a/builtin/check-ignore.c +++ b/builtin/check-ignore.c @@ -63,7 +63,7 @@ static void output_exclude(const char *path, struct exclude *exclude) } } -static int check_ignore(struct path_exclude_check *check, +static int check_ignore(struct dir_struct *dir, const char *prefix, const char **pathspec) { const char *path, *full_path; @@ -91,8 +91,7 @@ static int check_ignore(struct path_exclude_check *check, die_if_path_beyond_symlink(full_path, prefix); exclude = NULL; if (!seen[i]) { - exclude = last_exclude_matching_path(check, full_path, --1, &dtype); + exclude = last_exclude_matching(dir, full_path, &dtype); } if (!quiet && (exclude || show_non_matching)) output_exclude(path, exclude); @@ -104,7 +103,7 @@ static int check_ignore(struct path_exclude_check *check, return num_ignored; } -static int check_ignore_stdin_paths(struct path_exclude_check *check, const char *prefix) +static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix) { struct strbuf buf, nbuf; char *pathspec[2] = { NULL, NULL }; @@ -121,7 +120,7 @@ static int check_ignore_stdin_paths(struct path_exclude_check *check, const char strbuf_swap(&buf, &nbuf); } pathspec[0] = buf.buf; - num_ignored += check_ignore(check, prefix, (const char **)pathspec); + num_ignored += check_ignore(dir, prefix, (const char **)pathspec); maybe_flush_or_die(stdout, "check-ignore to stdout"); } strbuf_release(&buf); @@ -133,7 +132,6 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix) { int num_ignored; struct dir_struct dir; - struct path_exclude_check check; git_config(git_default_config, NULL); @@ -166,16 +164,14 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix) dir.flags |= DIR_COLLECT_IGNORED; setup_standard_excludes(&dir); - path_exclude_check_init(&check, &dir); if (stdin_paths) { - num_ignored = check_ignore_stdin_paths(&check, prefix); + num_ignored = check_ignore_stdin_paths(&dir, prefix); } else { - num_ignored = check_ignore(&check, prefix, argv); + num_ignored = check_ignore(&dir, prefix, argv); maybe_flush_or_die(stdout, "ignore to stdout"); } clear_directory(&dir); - path_exclude_check_clear(&check); return !num_ignored; } diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 175e6e3..2202072 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -201,19 +201,15 @@ static void show_ru_info(void) } } -static int ce_excluded(struct path_exclude_check *check, struct cache_entry *ce) +static int ce_excluded(struct dir_struct *dir
[PATCH v2-pu 14/14] dir.c: git-status --ignored: don't scan the work tree twice
'git-status --ignored' still scans the work tree twice to collect untracked and ignored files, respectively. fill_directory / read_directory already supports collecting untracked and ignored files in a single directory scan. However, the DIR_COLLECT_IGNORED flag to enable this has some git-add specific side-effects (e.g. it doesn't recurse into ignored directories, so listing ignored files with --untracked=all doesn't work). The DIR_SHOW_IGNORED flag doesn't list untracked files and returns ignored files in dir_struct.entries[] (instead of dir_struct.ignored[] as DIR_COLLECT_IGNORED). DIR_SHOW_IGNORED is used all throughout git. We don't want to break the existing API, so lets introduce a new flag DIR_SHOW_IGNORED_TOO that lists untracked as well as ignored files similar to DIR_COLLECT_FILES, but will recurse into sub-directories based on the other flags as DIR_SHOW_IGNORED does. In dir.c::read_directory_recursive, add ignored files to either dir_struct.entries[] or dir_struct.ignored[] based on the flags. Also move the DIR_COLLECT_IGNORED case here so that filling result lists is in a common place. In wt-status.c::wt_status_collect_untracked, use the new flag and read results from dir_struct.ignored[]. Remove the extra fill_directory call. builtin/check-ignore.c doesn't call fill_directory, setting the git-add specific DIR_COLLECT_IGNORED flag has no effect here. Remove for clarity. Update API documentation to reflect the changes. Performance: with this patch, 'git-status --ignored' is typically as fast as 'git-status'. Signed-off-by: Karsten Blees --- Documentation/technical/api-directory-listing.txt | 25 --- builtin/check-ignore.c| 1 - dir.c | 10 + dir.h | 3 ++- wt-status.c | 24 ++ 5 files changed, 41 insertions(+), 22 deletions(-) diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt index 1f349b2..7f8e78d 100644 --- a/Documentation/technical/api-directory-listing.txt +++ b/Documentation/technical/api-directory-listing.txt @@ -22,12 +22,23 @@ The notable options are: `flags`:: - A bit-field of options: + A bit-field of options (the `*IGNORED*` flags are mutually exclusive): `DIR_SHOW_IGNORED`::: - The traversal is for finding just ignored files, not unignored - files. + Return just ignored files in `entries[]`, not untracked files. + +`DIR_SHOW_IGNORED_TOO`::: + + Similar to `DIR_SHOW_IGNORED`, but return ignored files in `ignored[]` + in addition to untracked files in `entries[]`. + +`DIR_COLLECT_IGNORED`::: + + Special mode for git-add. Return ignored files in `ignored[]` and + untracked files in `entries[]`. Only returns ignored files that match + pathspec exactly (no wildcards). Does not recurse into ignored + directories. `DIR_SHOW_OTHER_DIRECTORIES`::: @@ -57,6 +68,14 @@ The result of the enumeration is left in these fields: Internal use; keeps track of allocation of `entries[]` array. +`ignored[]`:: + + An array of `struct dir_entry`, used for ignored paths with the + `DIR_SHOW_IGNORED_TOO` and `DIR_COLLECT_IGNORED` flags. + +`ignored_nr`:: + + The number of members in `ignored[]` array. Calling sequence diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c index a4357fb..4a8fc70 100644 --- a/builtin/check-ignore.c +++ b/builtin/check-ignore.c @@ -161,7 +161,6 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix) die(_("index file corrupt")); memset(&dir, 0, sizeof(dir)); - dir.flags |= DIR_COLLECT_IGNORED; setup_standard_excludes(&dir); if (stdin_paths) { diff --git a/dir.c b/dir.c index 2088891..ee4d04d 100644 --- a/dir.c +++ b/dir.c @@ -1183,15 +1183,12 @@ static enum path_treatment treat_one_path(struct dir_struct *dir, return path_none; exclude = is_excluded(dir, path->buf, &dtype); - if (exclude && (dir->flags & DIR_COLLECT_IGNORED) - && exclude_matches_pathspec(path->buf, path->len, simplify)) - dir_add_ignored(dir, path->buf, path->len); /* * Excluded? If we don't explicitly want to show * ignored files, ignore it */ - if (exclude && !(dir->flags & DIR_SHOW_IGNORED)) + if (exclude && !(dir->flags & (DIR_SHOW_IGNORED|DIR_SHOW_IGNORED_TOO))) return path_excluded; switch (dtype) { @@ -1280,6 +1277,11 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir, case path_excluded: if (dir->flags & DIR_SHOW_IGNORED) dir_add_name(dir, path.buf, path.len); + else if ((dir
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Wed, Apr 17, 2013 at 11:14:42AM -0700, Junio C Hamano wrote: > > I think it is just the warning code avoiding extra complexity and > > overhead, if you are talking about not getting warning in the > > pre-2.0 step that is in 'next'. Patches are very much welcomed, > > especially the ones that come before I get around to it ;-) > > I took a brief look at the code, and as you said "add" needs to know > about submodules, and the best fix looks to me to take the same > approach Jonathan came up with to de-noise the "add -u/-A" topic. > > That is, to scan the working tree to actually see if we would record > removals to the index in 2.0, but not remove them in this current > version, and give the warning when the differences in the behaviours > matter. Yeah, I had the same thought, as this warning has been bugging me for the last day or two. The worst part about it is that I finally trained myself to type "git add ." to silence the _other_ warning, and now it triggers this one. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] submodule deinit: clarify work tree removal message
Am 17.04.2013 07:16, schrieb Junio C Hamano: > Phil Hord writes: > >> On Mon, Apr 1, 2013 at 3:02 PM, Jens Lehmann wrote: >>> Okay, so here is the patch for that. If someone could point out >>> a portable and efficient way to check if a directory is already >>> empty I would be happy to use that to silence the "Cleaned >>> directory" message currently printed also when deinit is run on >>> an already empty directory. >> >>isemptydir() { >> test -d "$(find $1 -maxdepth 0 -empty)" >>} > > Hrm, -maxdepth and -empty are not even in POSIX. Folks on GNU > platforms and BSDs (I checked NetBSD 6 and OpenBSD 5.2) should be > fine, but it makes other platforms unhappy. Ok, that is not acceptable. > What is this check used for? To avoid running "rmdir" on non-empty > ones? Saying "cleaning foo/" (or "cleaned foo/") when foo/ is > already empty is not a crime; not omitting an empty one may actually > be a better behaviour from the point of view of repeatability and > uniformity. It's no big issue, but 'init' issues the "Submodule ... registered for path ..." message only once and is quiet on subsequent calls, 'deinit' does the same with "Submodule ... unregistered for path ...", only the "Cleared directory ..." message appears each time 'deinit' is called, which makes it stand out. I do not believe this little inconsistency is important enough to write a helper in C (to have a portable way to see if the directory has already been cleared), but this simple additional "if" looked easy enough. That should have made me suspicious ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] cat-file: print tags raw for "cat-file -p"
On Wed, Apr 17, 2013 at 09:06:21PM +0200, René Scharfe wrote: > Am 17.04.2013 20:02, schrieb Jeff King: > >I think we also need to do something about "git cat-file -p", which does > >not use the split_ident_line parser (but has its own problems with the > >home-grown parser). > > Ah, while it prints commit object contents verbatim, it formats the date > of tags. And it does it without help from tag.c (or ident.c), which in > turn does its own parsing as well. So it looks like we have two more > candidates for conversion to split_ident_line() here. I think we should apply the patch below to just drop the date formatting from cat-file, along with your two patches. This is the 4/4 from the series I posted in February: http://thread.gmane.org/gmane.comp.version-control.git/216870/focus=217081 but there I claimed that "git tag -v" might be affected. Upon looking closer, it is not; we accidentally dropped the pretty-printing of the date from it many years ago (and nobody seemed to care). The other patches from that series aren't necessary. The 1/4 is replaced by your patches (which do roughly the same thing, but add nice tests and seem to refactor a bit more). The 2/4 and 3/4 patches were about adding new fsck checks for tags, but I think there is some refactoring necessary there. They can wait for now. -- >8 -- Subject: [PATCH] cat-file: print tags raw for "cat-file -p" When "cat-file -p" prints commits, it shows them in their raw format, since git's format is already human-readable. For tags, however, we print the whole thing raw except for one thing: we convert the timestamp on the tagger line into a human-readable date. This dates all the way back to a0f15fa (Pretty-print tagger dates, 2006-03-01). At that time there was no other way to pretty-print a tag. These days, however, neither of those matters much. The normal way to pretty-print a tag is with "git show", which is much more flexible than "cat-file -p". Commit a0f15fa also built "verify-tag --verbose" (and subsequently "tag -v") around the "cat-file -p" output. However, that behavior was lost in commit 62e09ce (Make git tag a builtin, 2007-07-20), and we went back to printing the raw tag contents. Nobody seems to have noticed the bug since then (and it is arguably a saner behavior anyway, as it shows the actual bytes for which we verified the signature). Let's drop the tagger-date formatting for "cat-file -p". It makes us more consistent with cat-file's commit pretty-printer, and as a bonus, we can drop the hand-rolled tag parsing code in cat-file (which happened to behave inconsistently with the tag pretty-printing code elsewhere). This is a change of output format, so it's possible that some callers could considered this a regression. However, the original behavior was arguably a bug (due to the inconsistency with commits), likely nobody was relying on it (even we do not use it ourselves these days), and anyone relying on the "-p" pretty-printer should be able to expect a change in the output format (i.e., while "cat-file" is plumbing, the output format of "-p" was never guaranteed to be stable). Signed-off-by: Jeff King --- builtin/cat-file.c | 71 - t/t1006-cat-file.sh | 5 +--- 2 files changed, 1 insertion(+), 75 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 40f87b4..045cee7 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -16,73 +16,6 @@ #define BATCH 1 #define BATCH_CHECK 2 -static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long size) -{ - /* the parser in tag.c is useless here. */ - const char *endp = buf + size; - const char *cp = buf; - - while (cp < endp) { - char c = *cp++; - if (c != '\n') - continue; - if (7 <= endp - cp && !memcmp("tagger ", cp, 7)) { - const char *tagger = cp; - - /* Found the tagger line. Copy out the contents -* of the buffer so far. -*/ - write_or_die(1, buf, cp - buf); - - /* -* Do something intelligent, like pretty-printing -* the date. -*/ - while (cp < endp) { - if (*cp++ == '\n') { - /* tagger to cp is a line -* that has ident and time. -*/ - const char *sp = tagger; - char *ep; - unsigned long date; - long tz; - while (sp < cp && *sp != '>') - sp++; -
Re: [PATCH] blame: handle broken commit headers gracefully
On Wed, Apr 17, 2013 at 08:33:54PM +0200, René Scharfe wrote: > Minimal patch, test case missing. It's a bit sad that the old commit > parser of blame handled Ivan's specific corruption (extra "-<>" after > email) gracefully because it used the spaces as cutting points instead > of "<" and ">". That may mean there is room for improvement in split_ident_line to be more resilient in removing cruft. With something like: Name -<> 123456789 - it would obviously be nice to find the date timestamp there, but I wonder what the "email" field should return? The full broken string, or just "email@host"? One way is convenient for overlooking problems in broken commits, but I would worry about code paths that are using split_ident_line to verify the quality of the string (like determine_author_info). It's possible we would need a strict and a forgiving mode. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git rev-list --pretty=format:"" issue
git 1.8.2.1 on OSX(Mountain Lion) installed with Homebrew I am seeing an issue when trying to format the output from rev-list command. git log --pretty=format:"%H - %an, %ar : %s" When I attempt the above string, instead of printing to the shell, LESS is opened and the output is displayed there. Got the command from here: http://git-scm.com/book/en/Git-Basics-Viewing-the-Commit-History git log --pretty=format:"%h - %an, %ar : %s" The string above works fine when I change the %h to %H the issue shoes up. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] blame: handle broken commit headers gracefully
Am 17.04.2013 23:07, schrieb Jeff King: On Wed, Apr 17, 2013 at 08:33:54PM +0200, René Scharfe wrote: Minimal patch, test case missing. It's a bit sad that the old commit parser of blame handled Ivan's specific corruption (extra "-<>" after email) gracefully because it used the spaces as cutting points instead of "<" and ">". That may mean there is room for improvement in split_ident_line to be more resilient in removing cruft. With something like: Name -<> 123456789 - it would obviously be nice to find the date timestamp there, but I wonder what the "email" field should return? The full broken string, or just "email@host"? One way is convenient for overlooking problems in broken commits, but I would worry about code paths that are using split_ident_line to verify the quality of the string (like determine_author_info). It's possible we would need a strict and a forgiving mode. You can have both; the necessary data is in the struct ident_split: Just check that *mail_end == '>' and mail_end + 1 == date_begin etc. René -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: "What's cooking" between #05 and #06
Am 17.04.2013 01:52, schrieb Junio C Hamano: >> * jk/submodule-subdirectory-ok (2013-04-10) 2 commits >> - submodule: drop the top-level requirement >> - rev-parse: add --prefix option >> >> Allow various subcommands of "git submodule" to be run not from the >> top of the working tree of the superproject. >> >> Waiting for comments. > > Any submodule users wants to weigh in? The code looked fine, but I > do not heavily use it (and the repository with a submodule I have, I > do not have a "subdirectory" ;-, so I am a bad guinea pig). I like it, as it gets rid of the top-level requirement. But from my testing it looks like we're not quite there yet. 'summary' and 'status' behave as if they were run in the toplevel directory, while a "git status" shows all filenames relative to the current directory. Me thinks 'summary' and 'status' (and all other submodule commands) should behave like status and print relative paths too. I'm not really sure yet how $sm_path should behave for 'foreach', but I suspect having it relative to the current directory would be the way to go (which it currently isn't). When "submodule add" is run with a relative path it is relative to the top-level directory, which I find confusing (and won't play well with shell completion). 'deinit .' doesn't deinit submodules above the current directory (but prints the path relative to top-level) while 'init' will initialize all submodules known to the superproject. So this is a good start, but it looks like there is some work left to do before this can hit master. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-svn: added an --include-path flag
The SVN::Fetcher module is now able to filter for inclusion as well as exclusion (as used by --ignore-path). Also added tests, documentation changes and git completion script. If you have an SVN repository with many top level directories and you only want a git-svn clone of some of them then using --ignore-path is difficult as it requires a very long regexp. In this case it's much easier to filter for inclusion. Signed-off-by: Paul Walmsley --- Documentation/git-svn.txt | 12 +++ contrib/completion/git-completion.bash |2 +- git-svn.perl |4 + perl/Git/SVN/Fetcher.pm| 13 ++- t/t9147-git-svn-include-paths.sh | 150 5 files changed, 179 insertions(+), 2 deletions(-) create mode 100755 t/t9147-git-svn-include-paths.sh --1.7.10.4 Content-Type: text/x-patch; name="0001-git-svn-added-an-include-path-flag.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="0001-git-svn-added-an-include-path-flag.patch" diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt index 69decb1..df1f40c 100644 --- a/Documentation/git-svn.txt +++ b/Documentation/git-svn.txt @@ -85,6 +85,10 @@ COMMANDS When passed to 'init' or 'clone' this regular expression will be preserved as a config key. See 'fetch' for a description of '--ignore-paths'. +--include-paths=;; + When passed to 'init' or 'clone' this regular expression will + be preserved as a config key. See 'fetch' for a description + of '--include-paths'. --no-minimize-url;; When tracking multiple directories (using --stdlayout, --branches, or --tags options), git svn will attempt to connect @@ -146,6 +150,14 @@ Skip "branches" and "tags" of first level directories;; -- +--include-paths=;; + This allows one to specify a Perl regular expression that will + cause the inclusion of only matching paths from checkout from SVN. + The '--include-paths' option should match for every 'fetch' + (including automatic fetches due to 'clone', 'dcommit', + 'rebase', etc) on a given repository. '--ignore-paths' takes + precedence over '--include-paths'. + --log-window-size=;; Fetch log entries per request when scanning Subversion history. The default is 100. For very large Subversion repositories, larger diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 39c7ff8..8b75d01 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -,7 +,7 @@ _git_svn () --no-metadata --use-svm-props --use-svnsync-props --log-window-size= --no-checkout --quiet --repack-flags --use-log-author --localtime - --ignore-paths= $remote_opts + --ignore-paths= --include-paths= $remote_opts " local init_opts=" --template= --shared= --trunk= --tags= diff --git a/git-svn.perl b/git-svn.perl index bd5266c..6d713e1 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -127,6 +127,7 @@ my %remote_opts = ( 'username=s' => \$Git::SVN::Prompt::_username, 'config-dir=s' => \$Git::SVN::Ra::config_dir, 'no-auth-cache' => \$Git::SVN::Prompt::_no_auth_cache, 'ignore-paths=s' => \$Git::SVN::Fetcher::_ignore_regex, +'include-paths=s' => \$Git::SVN::Fetcher::_include_regex, 'ignore-refs=s' => \$Git::SVN::Ra::_ignore_refs_regex ); my %fc_opts = ( 'follow-parent|follow!' => \$Git::SVN::_follow_parent, 'authors-file|A=s' => \$_authors, @@ -477,6 +478,9 @@ sub do_git_init_db { my $ignore_paths_regex = \$Git::SVN::Fetcher::_ignore_regex; command_noisy('config', "$pfx.ignore-paths", $$ignore_paths_regex) if defined $$ignore_paths_regex; + my $include_paths_regex = \$Git::SVN::Fetcher::_include_regex; + command_noisy('config', "$pfx.include-paths", $$include_paths_regex) + if defined $$include_paths_regex; my $ignore_refs_regex = \$Git::SVN::Ra::_ignore_refs_regex; command_noisy('config', "$pfx.ignore-refs", $$ignore_refs_regex) if defined $$ignore_refs_regex; diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm index 046a7a2..27824e7 100644 --- a/perl/Git/SVN/Fetcher.pm +++ b/perl/Git/SVN/Fetcher.pm @@ -1,5 +1,5 @@ package Git::SVN::Fetcher; -use vars qw/@ISA $_ignore_regex $_preserve_empty_dirs $_placeholder_filename +use vars qw/@ISA $_ignore_regex $_include_regex $_preserve_empty_dirs $_placeholder_filename @deleted_gpath %added_placeholder $repo_id/; use strict; use warnings; @@ -33,6 +33,10 @@ su
Re: [PATCH] blame: handle broken commit headers gracefully
Jeff King writes: > On Wed, Apr 17, 2013 at 08:33:54PM +0200, René Scharfe wrote: > >> Minimal patch, test case missing. It's a bit sad that the old commit >> parser of blame handled Ivan's specific corruption (extra "-<>" after >> email) gracefully because it used the spaces as cutting points instead >> of "<" and ">". > > That may mean there is room for improvement in split_ident_line to > be more resilient in removing cruft. With something like: > > Name -<> 123456789 - > > it would obviously be nice to find the date timestamp there, but I > wonder what the "email" field should return? The full broken string, or > just "email@host"? Or you can imagine nastier input strings, like Name <>- 123456789 - Name - 123456789 - Name 56789 - I am afraid that at some point "we should salvage as much as we can", which is a worthy goal, becomes a losing proposition. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/14] Improve git-status --ignored
Karsten Blees writes: > I'll send fixups for #11 and #14, or you can pick the entire series rebased > to pu from: > https://github.com/kblees/git/tree/kb/improve-git-status-ignored-v2-pu > git pull git://github.com/kblees/git.git kb/improve-git-status-ignored-v2-pu Will take a look; very much appreciated. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git rev-list --pretty=format:"" issue
Forrest Galloway writes: > git 1.8.2.1 on OSX(Mountain Lion) installed with Homebrew > > I am seeing an issue when trying to format the output from rev-list command. > git log --pretty=format:"%H - %an, %ar : %s" When I attempt the above > string, instead of printing to the shell, LESS is opened and the > output is displayed there. > > > Got the command from here: > http://git-scm.com/book/en/Git-Basics-Viewing-the-Commit-History > > git log --pretty=format:"%h - %an, %ar : %s" The string above works > fine when I change the %h to %H the issue shoes up. Actually, less is running in both cases. We give --quit-if-one-screen (-F) and --chop-long-lines (-S) by default when we run "less", unless you have your own LESS environment variable to override our choice, if your history is shorter than one screenful *and* if your output lines are narrower than your terminal, it exits after showing the output. By passing %H instead of %h, you make the output wider, and when viewing output with --chop-long-lines, less refuses to implicitly exit with --quit-if-one-screen, because you may want to look at the RHS end of the output with right/left arrow keys, and it cannot do so if it exits after showing the last line. If you do not want pager, run it with no-pager, like this: git --no-pager log ...your other parameters... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitremote-helpers(1): clarify refspec behaviour
On Sat, Apr 6, 2013 at 12:13 PM, John Keeping wrote: > --- a/Documentation/gitremote-helpers.txt > +++ b/Documentation/gitremote-helpers.txt > @@ -174,8 +174,8 @@ ref. > This capability can be advertised multiple times. The first > applicable refspec takes precedence. The left-hand of refspecs > advertised with this capability must cover all refs reported by > -the list command. If no 'refspec' capability is advertised, > -there is an implied `refspec *:*`. > +the list command. If a helper does not need a specific 'refspec' > +capability then it should advertise `refspec *:*`. But if it advertises a straight 'refspec *:*', nothing would work. If anything, it should be 'refs/heads/*:refs/remotes/$alias/*', but then tags would fail. Why not just tell the remote helpers to do the right thing and avoid this comment altogether? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/33] t3210: test for spurious error messages for dangling packed refs
Junio C Hamano writes: >> Do we want to use approxidate_careful here to catch other junk? > > We can catch a misspelt argument or configuration value that way. > That would be a good idea. -- >8 -- Subject: date.c: add parse_expiry_date() "git reflog --expire=all" tries to expire reflog entries up to the current second, because the approxidate() parser gives the current timestamp for anything it does not understand (and it does not know what time "all" means). When the user tells us to expire "all" (or set the expiration time to "now"), the user wants to remove all the reflog entries (no reflog entry should record future time). Just set it to ULONG_MAX and to let everything that is older that timestamp expire. While at it, allow "now" to be treated the same way for callers that parse expiry date timestamp with this function. Also use an error reporting version of approxidate() to report misspelled date. When the user says e.g. "--expire=mnoday" to delete entries two days or older on Wednesday, we wouldn't want the "unknown, default to now" logic to kick in. Signed-off-by: Junio C Hamano --- * adoption to more users and tests are left as an exercise to the reader. builtin/reflog.c | 14 +++--- cache.h | 1 + date.c | 22 ++ 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/builtin/reflog.c b/builtin/reflog.c index b3c9e27..44700f9 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -496,11 +496,9 @@ static int parse_expire_cfg_value(const char *var, const char *value, unsigned l { if (!value) return config_error_nonbool(var); - if (!strcmp(value, "never") || !strcmp(value, "false")) { - *expire = 0; - return 0; - } - *expire = approxidate(value); + if (parse_expiry_date(value, expire)) + return error(_("%s' for '%s' is not a valid timestamp"), +value, var); return 0; } @@ -613,11 +611,13 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix) if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n")) cb.dry_run = 1; else if (!prefixcmp(arg, "--expire=")) { - cb.expire_total = approxidate(arg + 9); + if (parse_expiry_date(arg + 9, &cb.expire_total)) + die(_("'%s' is not a valid timestamp"), arg); explicit_expiry |= EXPIRE_TOTAL; } else if (!prefixcmp(arg, "--expire-unreachable=")) { - cb.expire_unreachable = approxidate(arg + 21); + if (parse_expiry_date(arg + 21, &cb.expire_unreachable)) + die(_("'%s' is not a valid timestamp"), arg); explicit_expiry |= EXPIRE_UNREACH; } else if (!strcmp(arg, "--stale-fix")) diff --git a/cache.h b/cache.h index 3622e18..f43f6d9 100644 --- a/cache.h +++ b/cache.h @@ -878,6 +878,7 @@ void show_date_relative(unsigned long time, int tz, const struct timeval *now, struct strbuf *timebuf); int parse_date(const char *date, char *buf, int bufsize); int parse_date_basic(const char *date, unsigned long *timestamp, int *offset); +int parse_expiry_date(const char *date, unsigned long *timestamp); void datestamp(char *buf, int bufsize); #define approxidate(s) approxidate_careful((s), NULL) unsigned long approxidate_careful(const char *, int *); diff --git a/date.c b/date.c index 57331ed..876d679 100644 --- a/date.c +++ b/date.c @@ -705,6 +705,28 @@ int parse_date_basic(const char *date, unsigned long *timestamp, int *offset) return 0; /* success */ } +int parse_expiry_date(const char *date, unsigned long *timestamp) +{ + int errors = 0; + + if (!strcmp(date, "never") || !strcmp(date, "false")) + *timestamp = 0; + else if (!strcmp(date, "all") || !strcmp(date, "now")) + /* +* We take over "now" here, which usually translates +* to the current timestamp. This is because the user +* really means to expire everything she has done in +* the past, and by definition reflogs are the record +* of the past, and there is nothing from the future +* to be kept. +*/ + *timestamp = ULONG_MAX; + else + *timestamp = approxidate_careful(date, &errors); + + return errors; +} + int parse_date(const char *date, char *result, int maxlen) { unsigned long timestamp; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] clone: introduce clone.submoduleGitDir to relocate $GITDIR
On Thu, Apr 18, 2013 at 1:02 AM, Ramkumar Ramachandra wrote: >> No, the point is people make mistakes. What do we do in that case? Or >> will you introduce yet another "gc" command for clean up ~/bare? > > So, people don't make mistakes when they use 'git submodule add', but > do make mistakes when using 'git clone'? How has the problem > _changed_ with my patch? It's reasonable to point it out as an > existing problem, and ask for it to be fixed independent of this > discussion, but that is not what you are doing. It's the magic in git-clone that changes its behavior that I want to address. I know you agree to go with a command line option. But I think in the end there will be a switch hidden somewhere in config to make things smooth, unless you make this mode the default (*). With normal mode, "rm -rf repo" is enough, with the new submodule mode, it leaves some garbage behind that the user may not be aware about. Maybe this is something that should be addressed anyway even for .gitmodules mode like Junio said. But I wonder if there are any other traps that come with the config switch. (*) I don't think you can make the new mode the default though. There are repos in repos in the field that are not managed by "git submodule". Switching the default will disrupt those setups. Some deprecation cycles might help, I don't know. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC] l10n: de.po: translate 39 new messages
Christian Stimming writes: > Thanks for the regular update! This is great work. > > One issue with the plural form messages, though: > > Am Montag, 15. April 2013, 18:27:40 schrieb Ralf Thielow: >> #: bundle.c:186 >> -#, fuzzy, c-format >> +#, c-format >> msgid "The bundle contains this ref:" >> msgid_plural "The bundle contains these %d refs:" >> -msgstr[0] "Das Paket enthält %d Referenz" >> -msgstr[1] "Das Paket enthält %d Referenzen" >> +msgstr[0] "Das Paket enthält diese Referenz:" >> +msgstr[1] "Das Paket enthält diese %d Referenzen:" > > The msgstr[0] must still contain a %d conversion specifier (which will be > filled with the number 1) even though the translated sentence wouldn't need > the 1 anymore. The previous msgstr[0] was correct; the English singular msgid > is not. > > Technical background: The ngettext function chooses only the string itself, > which will then be fed to printf() as a second step. If the printf sees more > variadic arguments than conversion specifiers in the string, it will be > unhappy. At least that's what I remembered about the ngettext things... > > http://www.gnu.org/software/libc/manual/html_node/Advanced-gettext- > functions.html This vaguely sounds familiar: http://thread.gmane.org/gmane.comp.version-control.git/218173 http://www.gnu.org/software/gettext/manual/gettext.html#Plural-forms In the English singular case, the number – always 1 – can be replaced with "one": printf (ngettext ("One file removed", "%d files removed", n), n); This works because the ‘printf’ function discards excess arguments that are not consumed by the format string. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] help.c: add a compatibility comment to cmd_version()
From: "Junio C Hamano" Sent: Tuesday, April 16, 2013 11:35 PM "Philip Oakley" writes: int cmd_version(int argc, const char **argv, const char *prefix) { + /* + * The format of this string should be kept stable for compatibility + * with external projects that rely on the output of "git version". This 'tantalizes without telling', the same complaint that is given often for over-succinct commit messages. How about * E.g. git gui uses the extended regular expression "^git version [1-9]+(\.[0-9]+)+.*" * to check for an acceptable version string. The ERE is from git-gui.sh:L958. Shouldn't the expected format of our known external project also be indicated? ... printf("git version %s\n", git_version_string); It is fairly clear from the commented code that the only guarantee they will be getting is that it begins with a string "git version ". I read the code the opposite way. It says "This is the code to be changed" if you (anyone doing tweaks) want a special version string. git_version_string[] has anything of the builder's choice. I am not sure if there anything more to "indicate". Really, if you run $ git version and you get "Git Source Code Management Version 3.56" from its output, it is likely that the version is very different from what you know, and you should not assume any your assumption would hold. Again I am reading this from the opposite side. There would be no assumption of difference if it _passed_ the test scripts. Unfortunately it wouldn't be friendly to other tools (like git gui). Hence my suggestion of the basic test that a "passing" git would produce a consistent version string. It still allows the supplier's suffixes to be added, but not the prefixes. The test suite tests that git is 'good enough for most usages and picks up regressions. No? Obvious inconsistent special versions would fail in many other places. Or mention "such as git gui"? I do not see what it would buy us. It is not like it is OK as long as we upadte Git gui at the same time. Philip -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/7] Rework git core for native submodules
From: "Ramkumar Ramachandra" Sent: Wednesday, April 17, 2013 12:56 PM We've been over this several times in earlier emails. [...] Again, I'm not saying that my approach is Correct and Final. What I'm saying is: "Here's what I've done. Something interesting is going on. It's probably worth a look?" [...] The point is to have structured parseable information that the object-parsing code of git code and easily slurp and give to the rest of git-core. Please clear your reading backlog to avoid bringing up the same points over and over again. -- Ram, The email thread is pretty long with a lot of too and fro, that would be difficult to catch up on (too much $dayjob+$family vs $sparetime). Would it be possible to summarise the key points and proposals of where the subject is now? The submodules does need 'fixing', as does agreeing the problem and abuse cases. Philip -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Felipe Contreras writes: > And how do you know this will be part of the 1%? You don't. How many > times have you tracked regressions in transport helper's import/export > functionality? How many times in remote-hg? How many times has > *anybody* done so? The last point makes it all the more important to have a good history [*1*]. An area that no developer rarely touches with a little user base can stay dormant for a long time, and when people do need to hunt for an ancient bug or to enhance the existing feature to support a new use case without breaking the old use case, the original author may not be around, lost interest, or no longer uses his own creation. The code left behind tells us what the author thought was the best way to solve his problem, but it does not clearly define what the problem he tried to solve was, within what constraint he had to find a solution for it, and why he thought that the solution was the best (or sometimes "only") one. Log and in-code comments are to explain such things that are beyond how the code works and what it does. [Footnote] *1* In this message, I am not judging if the depth of your writing for the particular change is deep enough. It depends on how well the reader knows the area, and there is no single right answer to that question. Incidentally that is why we tend to err on the more descriptive side. The next person your commit will help may not know the area as well as you do and has to figure things out on his own. You are helping him by being descriptive. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/6] transport-helper: some clarifications and a fix
Hi, It seems the workings of transport-helper are anything but clear, so let's try to clarify them a bit, and after that, hopefully it would become clearer why the last patch is actually a good fix. Felipe Contreras (6): transport-helper: clarify *:* refspec transport-helper: update refspec documentation transport-helper: clarify pushing without refspecs transport-helper: warn when refspec is not used transport-helper: trivial code shuffle transport-helper: update remote helper namespace Documentation/gitremote-helpers.txt | 12 ++-- t/t5801-remote-helpers.sh | 39 ++--- transport-helper.c | 37 +++ 3 files changed, 50 insertions(+), 38 deletions(-) -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/6] transport-helper: clarify *:* refspec
The *:* refspec doesn't work, and never has, clarify the code and documentation to reflect that. This in effect reverts commit 9e7673e (gitremote-helpers(1): clarify refspec behaviour). Signed-off-by: Felipe Contreras --- Documentation/gitremote-helpers.txt | 4 ++-- t/t5801-remote-helpers.sh | 15 --- transport-helper.c | 2 +- 3 files changed, 3 insertions(+), 18 deletions(-) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index f506031..0c91aba 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -174,8 +174,8 @@ ref. This capability can be advertised multiple times. The first applicable refspec takes precedence. The left-hand of refspecs advertised with this capability must cover all refs reported by -the list command. If a helper does not need a specific 'refspec' -capability then it should advertise `refspec *:*`. +the list command. If no 'refspec' capability is advertised, +there is an implied `refspec *:*`. 'bidi-import':: This modifies the 'import' capability. diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index f387027..cd1873c 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -120,21 +120,6 @@ test_expect_failure 'pushing without refspecs' ' compare_refs local2 HEAD server HEAD ' -test_expect_success 'pulling with straight refspec' ' - (cd local2 && - GIT_REMOTE_TESTGIT_REFSPEC="*:*" git pull) && - compare_refs local2 HEAD server HEAD -' - -test_expect_failure 'pushing with straight refspec' ' - test_when_finished "(cd local2 && git reset --hard origin)" && - (cd local2 && - echo content >>file && - git commit -a -m eleven && - GIT_REMOTE_TESTGIT_REFSPEC="*:*" git push) && - compare_refs local2 HEAD server HEAD -' - test_expect_success 'pulling without marks' ' (cd local2 && GIT_REMOTE_TESTGIT_NO_MARKS=1 git pull) && diff --git a/transport-helper.c b/transport-helper.c index dcd8d97..cea787c 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -469,7 +469,7 @@ static int fetch_with_import(struct transport *transport, * were fetching. * * (If no "refspec" capability was specified, for historical -* reasons we default to *:*.) +* reasons we default to the equivalent of *:*.) * * Store the result in to_fetch[i].old_sha1. Callers such * as "git fetch" can use the value to write feedback to the -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/6] transport-helper: update refspec documentation
The refspec capability is not only used by 'import', also by 'export', and it's recommend in both. Signed-off-by: Felipe Contreras --- Documentation/gitremote-helpers.txt | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index 0c91aba..ba7240c 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -159,11 +159,11 @@ Miscellaneous capabilities carried out. 'refspec' :: - This modifies the 'import' capability, allowing the produced - fast-import stream to modify refs in a private namespace - instead of writing to refs/heads or refs/remotes directly. - It is recommended that all importers providing the 'import' - capability use this. + For remote helpers that implement 'import' or 'export', this capability + allows the refs to be constrained to a private namespace, instead of + writing to refs/heads or refs/remotes directly. + It is recommended that all importers providing the 'import' or 'export' + capabilities use this. + A helper advertising the capability `refspec refs/heads/*:refs/svn/origin/branches/*` -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/6] transport-helper: clarify pushing without refspecs
This has never worked, since it's inception the code simply skips all the refs, essentially telling fast-export to do nothing. Let's at least tell the user what's going on. Signed-off-by: Felipe Contreras --- Documentation/gitremote-helpers.txt | 4 ++-- t/t5801-remote-helpers.sh | 6 +++--- transport-helper.c | 5 +++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index ba7240c..4d26e37 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -162,8 +162,8 @@ Miscellaneous capabilities For remote helpers that implement 'import' or 'export', this capability allows the refs to be constrained to a private namespace, instead of writing to refs/heads or refs/remotes directly. - It is recommended that all importers providing the 'import' or 'export' - capabilities use this. + It is recommended that all importers providing the 'import' + capability use this. It's mandatory for 'export'. + A helper advertising the capability `refspec refs/heads/*:refs/svn/origin/branches/*` diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index cd1873c..3eeb309 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -111,13 +111,13 @@ test_expect_success 'pulling without refspecs' ' compare_refs local2 HEAD server HEAD ' -test_expect_failure 'pushing without refspecs' ' +test_expect_success 'pushing without refspecs' ' test_when_finished "(cd local2 && git reset --hard origin)" && (cd local2 && echo content >>file && git commit -a -m ten && - GIT_REMOTE_TESTGIT_REFSPEC="" git push) && - compare_refs local2 HEAD server HEAD + GIT_REMOTE_TESTGIT_REFSPEC="" test_must_fail git push 2> ../error) && + grep "remote-helper doesn.t support push; refspec needed" error ' test_expect_success 'pulling without marks' ' diff --git a/transport-helper.c b/transport-helper.c index cea787c..4d98567 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -785,6 +785,9 @@ static int push_refs_with_export(struct transport *transport, struct string_list revlist_args = STRING_LIST_INIT_NODUP; struct strbuf buf = STRBUF_INIT; + if (!data->refspecs) + die("remote-helper doesn't support push; refspec needed"); + helper = get_helper(transport); write_constant(helper->in, "export\n"); @@ -795,8 +798,6 @@ static int push_refs_with_export(struct transport *transport, char *private; unsigned char sha1[20]; - if (!data->refspecs) - continue; private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name); if (private && !get_sha1(private, sha1)) { strbuf_addf(&buf, "^%s", private); -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/6] transport-helper: warn when refspec is not used
For the modes that need it. In the future we should probably error out, instead of providing half-assed support. The reason we want to do this is because if it's not present, the remote helper might be updating refs/heads/*, or refs/remotes/origin/*, directly, and in the process fetch will get confused trying to update refs that are already updated, or older than what they should be. We shouldn't be messing with the rest of git. Signed-off-by: Felipe Contreras --- t/t5801-remote-helpers.sh | 6 -- transport-helper.c| 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index 3eeb309..1bb7529 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -100,14 +100,16 @@ test_expect_failure 'push new branch with old:new refspec' ' test_expect_success 'cloning without refspec' ' GIT_REMOTE_TESTGIT_REFSPEC="" \ - git clone "testgit::${PWD}/server" local2 && + git clone "testgit::${PWD}/server" local2 2> error && + grep "This remote helper should implement refspec capability" error && compare_refs local2 HEAD server HEAD ' test_expect_success 'pulling without refspecs' ' (cd local2 && git reset --hard && - GIT_REMOTE_TESTGIT_REFSPEC="" git pull) && + GIT_REMOTE_TESTGIT_REFSPEC="" git pull 2> ../error) && + grep "This remote helper should implement refspec capability" error && compare_refs local2 HEAD server HEAD ' diff --git a/transport-helper.c b/transport-helper.c index 4d98567..573eaf7 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -215,6 +215,8 @@ static struct child_process *get_helper(struct transport *transport) free((char *)refspecs[i]); } free(refspecs); + } else if (data->import || data->bidi_import || data->export) { + warning("This remote helper should implement refspec capability."); } strbuf_release(&buf); if (debug) -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/6] transport-helper: trivial code shuffle
Just shuffle the die() part to make it more explicit, and cleanup the code-style. Signed-off-by: Felipe Contreras --- transport-helper.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 573eaf7..9d31f2d 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -800,6 +800,9 @@ static int push_refs_with_export(struct transport *transport, char *private; unsigned char sha1[20]; + if (ref->deletion) + die("remote-helpers do not support ref deletion"); + private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name); if (private && !get_sha1(private, sha1)) { strbuf_addf(&buf, "^%s", private); @@ -807,13 +810,8 @@ static int push_refs_with_export(struct transport *transport, } free(private); - if (ref->deletion) { - die("remote-helpers do not support ref deletion"); - } - if (ref->peer_ref) string_list_append(&revlist_args, ref->peer_ref->name); - } if (get_exporter(transport, &exporter, &revlist_args)) -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/6] transport-helper: update remote helper namespace
When pushing, the remote namespace is updated correctly (e.g. refs/origin/master), but not the remote helper's (e.g. refs/testgit/origin/master), which currently is only updated while fetching. Since the remote namespace is used to tell fast-export which commits to avoid (because they were already imported/exported), it makes sense to have them in sync so they don't get generated twice. If the remote helper was implemented properly, they would be ignored, if not, they probably would end up repeated (probably). Signed-off-by: Felipe Contreras --- t/t5801-remote-helpers.sh | 12 transport-helper.c| 20 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index 1bb7529..097691c 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -153,4 +153,16 @@ test_expect_success 'push ref with existing object' ' compare_refs local dup server dup ' +test_expect_success 'push update refs' ' + (cd local && + git checkout -b update master && + echo update >>file && + git commit -a -m update && + git push origin update + git rev-parse --verify testgit/origin/heads/update >expect && + git rev-parse --verify remotes/origin/update >actual + test_cmp expect actual + ) +' + test_done diff --git a/transport-helper.c b/transport-helper.c index 9d31f2d..414d6c8 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -11,6 +11,7 @@ #include "thread-utils.h" #include "sigchain.h" #include "argv-array.h" +#include "refs.h" static int debug; @@ -620,7 +621,7 @@ static int fetch(struct transport *transport, return -1; } -static void push_update_ref_status(struct strbuf *buf, +static int push_update_ref_status(struct strbuf *buf, struct ref **ref, struct ref *remote_refs) { @@ -686,7 +687,7 @@ static void push_update_ref_status(struct strbuf *buf, *ref = find_ref_by_name(remote_refs, refname); if (!*ref) { warning("helper reported unexpected status of %s", refname); - return; + return 1; } if ((*ref)->status != REF_STATUS_NONE) { @@ -695,11 +696,12 @@ static void push_update_ref_status(struct strbuf *buf, * status reported by the remote helper if the latter is 'no match'. */ if (status == REF_STATUS_NONE) - return; + return 1; } (*ref)->status = status; (*ref)->remote_status = msg; + return 0; } static void push_update_refs_status(struct helper_data *data, @@ -708,11 +710,21 @@ static void push_update_refs_status(struct helper_data *data, struct strbuf buf = STRBUF_INIT; struct ref *ref = remote_refs; for (;;) { + char *private; + recvline(data, &buf); if (!buf.len) break; - push_update_ref_status(&buf, &ref, remote_refs); + if (push_update_ref_status(&buf, &ref, remote_refs)) + continue; + + /* propagate back the update to the remote namespace */ + private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name); + if (!private) + continue; + update_ref("update by helper", private, ref->new_sha1, NULL, 0, 0); + free(private); } strbuf_release(&buf); } -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] help.c: add a compatibility comment to cmd_version()
"Philip Oakley" writes: > How about >* E.g. git gui uses the extended regular expression "^git version > [1-9]+(\.[0-9]+)+.*" >* to check for an acceptable version string. > > The ERE is from git-gui.sh:L958. That is exactly the kind of guarantee we do _not_ want to give. > ... Hence my suggestion of the basic test that a "passing" git > would produce a consistent version string. I have been assuming that you are trying to avoid an exchange like this one we had in the past: http://thread.gmane.org/gmane.comp.version-control.git/216923/focus=217007 I also have been assuming that you are pushing to limit the possible versioning scheme, but I do not know what that extra limitation would achieve in the real world. By sticking to the pattern "git gui" happens to use, "git gui" may be able to guess that the next version of Git says it is verison "1.8.3". That is the _only_ thing your "test" buys. But having parsed the "1.8.3" substring out of it, "git gui" does not know anything about what 1.8.3 gives it. As far as it is concerned, it is a version whose "git version" output it has never seen (unless it has been kept up to date with the development of Git). By matching against "git version [1-9]+(\.[0-9]+)+", it is accepting that future versions may break assumptions it makes for some known versions (which is warranted) and all future versions (which is unwarranted) of Git. Maybe the version 2.0 of Git adds all changes in the directory "d", including removals, when you say "git add d", but it may have assumed that with Git version 1.5.0 or newer, saying "git add d" would result in added and modified inside that directory getting updated in the index, but paths removed from the working tree will stay in the index. The only thing the scripts that read from the output of "git version" can reliably tell is if it is interacting with a version of Git it knows about. If it made any unwarranted assumption on the versions it hasn't seen, it has to be fixed in "git gui" _anyway_. Of course, we would not change the output of "git version" willy-nilly without good reason, but that is a different topic. So I do not know what you want to achieve in the real world with that extra limitation on the "git version" output format. Maybe you are proposing something else. I dunno. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] transport-helper: clarify pushing without refspecs
On Wed, Apr 17, 2013 at 5:05 PM, Felipe Contreras wrote: > This has never worked, since it's inception the code simply skips all > the refs, essentially telling fast-export to do nothing. Makes sense. -- Cheers, Sverre Rabbelier -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitweb/INSTALL: GITWEB_CONFIG_SYSTEM is for backward compatibility
On Tue, Apr 16, 2013 at 6:26 PM, Jonathan Nieder wrote: > Drew Northup wrote: > >> This is unobtrusive yet to the point. > > I agree with the spirit. > > [...] >> --- a/Documentation/gitweb.conf.txt >> +++ b/Documentation/gitweb.conf.txt >> @@ -55,7 +55,8 @@ following order: >> then fallback system-wide configuration file (defaults to >> '/etc/gitweb.conf'). >> >> Values obtained in later configuration files override values obtained >> earlier >> -in the above sequence. >> +in the above sequence. This is different from many system-wide software >> +installations and will stay this way for historical reasons. > > That makes it sound like the "per instance overrides common overrides > built-in" cascading is what is unusual and what we need to apologize > for. I don't think were apologizing for anything. It is helpful to say "we do some things differently here and don't plan on changing for a very important reason. > How about something like the following? (It uses a BUGS section to > make the warning easy to notice for people tracking down confusing > behavior by searching for "gitweb.conf".) > > diff --git i/Documentation/gitweb.conf.txt w/Documentation/gitweb.conf.txt > index eb63631..ea0526e 100644 > --- i/Documentation/gitweb.conf.txt > +++ w/Documentation/gitweb.conf.txt > @@ -857,6 +857,13 @@ adding the following lines to your gitweb configuration > file: > $known_snapshot_formats{'zip'}{'disabled'} = 1; > $known_snapshot_formats{'tgz'}{'compressor'} = ['gzip','-6']; > > +BUGS > + > +Debugging would be easier if the fallback configuration file > +(`/etc/gitweb.conf`) and environment variable to override its location > +('GITWEB_CONFIG_SYSTEM') had names reflecting their "fallback" role. > +The current names are kept to avoid breaking working setups. > + > ENVIRONMENT > --- > The location of per-instance and system-wide configuration files can be I don't disagree with this, as some would consider the naming to be a bug, but after having been given a good schooling on the git list a while back as to why it is the way it is I'm hesitant to label "has history" as a bug. -- -Drew Northup -- "As opposed to vegetable or mineral error?" -John Pescatore, SANS NewsBites Vol. 12 Num. 59 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Jeff King writes: > Yeah, I had the same thought, as this warning has been bugging me for > the last day or two. The worst part about it is that I finally trained > myself to type "git add ." to silence the _other_ warning, and now it > triggers this one. :) So here is the "reworked" one on top of what is in 'next'. It introduces a bit of conflict with the "add -u/-A" topic, so I am not ready to push out the integration result yet. -- >8 -- Subject: [PATCH] git add: rework the logic to warn "git add ..." default change The earlier logic to warn against "git add subdir" that is run without "-A" or "--no-all" was only to check any given exactly spells a directory name that (still) exists on the filesystem. This had number of problems: * "git add '*dir'" (note that the wildcard is hidden from the shell) would not trigger the warning. * "git add '*.py'" would behave differently between the current version of Git and Git 2.0 for the same reason as "subdir", but would not trigger the warning. * "git add dir" for a submodule "dir" would just update the index entry for the submodule "dir" without ever recursing into it, and use of "-A" or "--no-all" would matter. But the logic only checks the directory-ness of "dir" and gives an unnecessary warning. Rework the logic to detect the case where the behaviour will be different in Git 2.0, and issue a warning only when it matters. Even with the code before this warning, "git add subdir" will have to traverse the directory in order to find _new_ files the index does not know about _anyway_, so we can do this check without adding an extra pass to find if matches any removed file. This essentially updates the "add_files_to_cache()" public API to "update_files_in_cache()" API that is internal to "git add", because with the "--all" option, the function is no longer about "adding" paths to the cache, but is also used to remove them. There are other callers of the former from "checkout" (used when "checkout -m" prepares the temporary tree that represents the local modifications to be merged) and "commit" ("commit --include" that picks up local changes in addition to what is in the index). Since ADD_CACHE_IGNORE_ERRORS (aka "--no-all") is not used by either of them, once dust settles after Git 2.0 and the warning becomes unnecessary, we may want to unify these two functions again. Signed-off-by: Junio C Hamano --- builtin/add.c | 64 +++ 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index f8f6c9e..4242bce 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -26,6 +26,9 @@ static int take_worktree_changes; struct update_callback_data { int flags; int add_errors; + + /* only needed for 2.0 transition preparation */ + int warn_add_would_remove; }; static int fix_unmerged_status(struct diff_filepair *p, @@ -49,6 +52,17 @@ static int fix_unmerged_status(struct diff_filepair *p, return DIFF_STATUS_MODIFIED; } +static void warn_add_would_remove(const char *path) +{ + warning(_("In Git 2.0, 'git add ...' will also update the\n" + "index for paths removed from the working tree that match\n" + "the given pathspec. If you want to 'add' only changed\n" + "or newly created paths, say 'git add --no-all ...'" + " instead.\n\n" + "'%s' would be removed from the index without --no-all."), + path); +} + static void update_callback(struct diff_queue_struct *q, struct diff_options *opt, void *cbdata) { @@ -70,6 +84,10 @@ static void update_callback(struct diff_queue_struct *q, } break; case DIFF_STATUS_DELETED: + if (data->warn_add_would_remove) { + warn_add_would_remove(path); + data->warn_add_would_remove = 0; + } if (data->flags & ADD_CACHE_IGNORE_REMOVAL) break; if (!(data->flags & ADD_CACHE_PRETEND)) @@ -81,20 +99,27 @@ static void update_callback(struct diff_queue_struct *q, } } -int add_files_to_cache(const char *prefix, const char **pathspec, int flags) +static void update_files_in_cache(const char *prefix, const char **pathspec, + struct update_callback_data *data) { - struct update_callback_data data; struct rev_info rev; init_revisions(&rev, prefix); setup_revisions(0, NULL, &rev, NULL); init_pathspec(&rev.prune_data, pathspec); rev.diffopt.output_format = DIFF_FORMAT_CALLBACK; rev.diffopt.format_callback = update_callback; - data.flags = flags; - data.add_errors = 0; - rev.diffopt.format_callback_data = &da
Re: [PATCH] gitweb/INSTALL: GITWEB_CONFIG_SYSTEM is for backward compatibility
Drew Northup writes: >> That makes it sound like the "per instance overrides common overrides >> built-in" cascading is what is unusual and what we need to apologize >> for. > > I don't think were apologizing for anything. It is helpful to say "we > do some things differently here and don't plan on changing for a very > important reason. My biggest resistance against this whole thing is that word "differently". It really depends on who reads this document. For some, it may be different. For others, it may not be unexpected at all. "We do things differently" may help the former but I do not want the latter to try to find "difference" that does not exist elsewhere and get confused. >> >> +BUGS >> + >> +Debugging would be easier if the fallback configuration file >> +(`/etc/gitweb.conf`) and environment variable to override its location >> +('GITWEB_CONFIG_SYSTEM') had names reflecting their "fallback" role. >> +The current names are kept to avoid breaking working setups. >> + >> ENVIRONMENT >> --- >> The location of per-instance and system-wide configuration files can be > > I don't disagree with this, as some would consider the naming to be a > bug, but after having been given a good schooling on the git list a > while back as to why it is the way it is I'm hesitant to label "has > history" as a bug. BUGS section being traditionally where you would throw this kind of thing, either a bug, non-bug, or wai-but-some-may-not-agree, I think this is a good description. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git add ... defaults to "-A"
Make "git add ..." notice paths that have been removed from the working tree, i.e. a synonym to "git add -A ...". Given that "git add " is to update the index with the state of the named part of the working tree as a whole, it makes it more intuitive, and also makes it possible to simplify the advice we give while marking the paths the user finished resolving conflicts with. We used to say "to record removal as a resolution, remove the path from the working tree and say 'git rm'; for all other cases, edit the path in the working tree and say 'git add'", but we can now say "update the path in the working tree and say 'git add'" instead. As promised, this merges the temporary update_files_in_cache() helper function back to add_files_to_cache() function. Signed-off-by: Junio C Hamano --- * So this comes on top of the previous update that will sit at jc/add-2.0-delete-default~1 and replaces the one previouly at its tip. Documentation/git-add.txt | 18 +-- builtin/add.c | 57 --- 2 files changed, 20 insertions(+), 55 deletions(-) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index 5c501a2..5bd0791 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -53,8 +53,14 @@ OPTIONS Files to add content from. Fileglobs (e.g. `*.c`) can be given to add all matching files. Also a leading directory name (e.g. `dir` to add `dir/file1` - and `dir/file2`) can be given to add all files in the - directory, recursively. + and `dir/file2`) can be given to update the index to + match the current state of the directory as a whole (e.g. + specifying `dir` will record not just a file `dir/file1` + modified in the working tree, a file `dir/file2` added to + the working tree, but also a file `dir/file3` removed from + the working tree. Note that older versions of Git used + to ignore removed files; use `--no-all` option if you want + to add modified or new files but ignore removed ones. -n:: --dry-run:: @@ -127,11 +133,9 @@ of Git, hence the form without should not be used. files that have been removed from the working tree. This option is a no-op when no is used. + -This option is primarily to help the current users of Git, whose -"git add ..." ignores removed files. In future versions -of Git, "git add ..." will be a synonym to "git add -A -..." and "git add --no-all ..." will behave like -today's "git add ...", ignoring removed files. +This option is primarily to help users who are used to older +versions of Git, whose "git add ..." was a synonym +to "git add --no-all ...", i.e. ignored removed files. -N:: --intent-to-add:: diff --git a/builtin/add.c b/builtin/add.c index 4242bce..21c685f 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -26,9 +26,6 @@ static int take_worktree_changes; struct update_callback_data { int flags; int add_errors; - - /* only needed for 2.0 transition preparation */ - int warn_add_would_remove; }; static int fix_unmerged_status(struct diff_filepair *p, @@ -52,17 +49,6 @@ static int fix_unmerged_status(struct diff_filepair *p, return DIFF_STATUS_MODIFIED; } -static void warn_add_would_remove(const char *path) -{ - warning(_("In Git 2.0, 'git add ...' will also update the\n" - "index for paths removed from the working tree that match\n" - "the given pathspec. If you want to 'add' only changed\n" - "or newly created paths, say 'git add --no-all ...'" - " instead.\n\n" - "'%s' would be removed from the index without --no-all."), - path); -} - static void update_callback(struct diff_queue_struct *q, struct diff_options *opt, void *cbdata) { @@ -84,10 +70,6 @@ static void update_callback(struct diff_queue_struct *q, } break; case DIFF_STATUS_DELETED: - if (data->warn_add_would_remove) { - warn_add_would_remove(path); - data->warn_add_would_remove = 0; - } if (data->flags & ADD_CACHE_IGNORE_REMOVAL) break; if (!(data->flags & ADD_CACHE_PRETEND)) @@ -99,27 +81,20 @@ static void update_callback(struct diff_queue_struct *q, } } -static void update_files_in_cache(const char *prefix, const char **pathspec, - struct update_callback_data *data) +int add_files_to_cache(const char *prefix, const char **pathspec, int flags) { + struct update_callback_data data; struct rev_info rev; init_revisions(&rev, prefix); setup_revisions(0, NULL, &rev, NULL); init_pathspec(&rev.prune_data, pa
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Wed, Apr 17, 2013 at 6:56 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> And how do you know this will be part of the 1%? You don't. How many >> times have you tracked regressions in transport helper's import/export >> functionality? How many times in remote-hg? How many times has >> *anybody* done so? > > The last point makes it all the more important to have a good > history [*1*]. An area that no developer rarely touches with a little > user base can stay dormant for a long time, and when people do need > to hunt for an ancient bug or to enhance the existing feature to > support a new use case without breaking the old use case, the > original author may not be around, lost interest, or no longer uses > his own creation. You are going in circles, I said such situation was *HYPOTHETICAL*, Phil Hord said it wasn't, and now you are bringing back more hypothetical examples, which I would gladly address, as soon as you accept they are HYPOTHETICAL. Now, how about you answer the questions about the *REAL* situations Phil Hord mentioned? * How many times have you tracked regressions in transport helper's import/export functionality? Hint: zero. * How many times in remote-hg? Hint: zero. * How many times has *anybody* done so? Hint: other than me, quite possibly zero. And then, before we consider this *hypothetical* situation, it might be worth noticing what commit this hypothetical person would hit if you do *not* apply this patch, and what the commit message says: --- remote-helpers: add support for an export command Signed-off-by: Junio C Hamano --- Yeah, well, glad you didn't apply my patch, wouldn't want to mess up the code that was clearly explained by that commit message. And before you rationalize the above commit, because maybe the functionality was described in the documentation, it wasn't: transport-helper.c | 132 ++--- 1 file changed, 120 insertions(+), 12 deletions(-) If you do apply my patch, it turns out even the shortest version of my commit message already gives more information to this *hypothetical* developer person. > [Footnote] > > *1* In this message, I am not judging if the depth of your writing > for the particular change is deep enough. It depends on how well > the reader knows the area, and there is no single right answer > to that question. > > Incidentally that is why we tend to err on the more descriptive > side. The next person your commit will help may not know the > area as well as you do and has to figure things out on his > own. You are helping him by being descriptive. I partially agree with this, but I think documenting the nuts and bolts of transport-helper would be better in done in code, documentation, tests, and mailing list analysis. And in all those respects, I believe I've done a more than adequate job. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] transport-helper: update remote helper namespace
On Wed, Apr 17, 2013 at 7:05 PM, Felipe Contreras wrote: > --- a/t/t5801-remote-helpers.sh > +++ b/t/t5801-remote-helpers.sh > @@ -153,4 +153,16 @@ test_expect_success 'push ref with existing object' ' > compare_refs local dup server dup > ' > > +test_expect_success 'push update refs' ' > + (cd local && > + git checkout -b update master && > + echo update >>file && > + git commit -a -m update && > + git push origin update > + git rev-parse --verify testgit/origin/heads/update >expect && > + git rev-parse --verify remotes/origin/update >actual > + test_cmp expect actual Slightly confusing and a bit buggy: - git rev-parse --verify testgit/origin/heads/update >expect && - git rev-parse --verify remotes/origin/update >actual + git rev-parse --verify remotes/origin/update >expect && + git rev-parse --verify testgit/origin/heads/update >actual && > static void push_update_refs_status(struct helper_data *data, > @@ -708,11 +710,21 @@ static void push_update_refs_status(struct helper_data > *data, > struct strbuf buf = STRBUF_INIT; > struct ref *ref = remote_refs; > for (;;) { > + char *private; > + > recvline(data, &buf); > if (!buf.len) > break; > > - push_update_ref_status(&buf, &ref, remote_refs); > + if (push_update_ref_status(&buf, &ref, remote_refs)) > + continue; Actually, since this function is also used by push_with_push: if (!data->refspecs) continue; I had it in my previous series but removed it. I'll reroll. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/6] transport-helper: some clarifications and a fix
Hi, Same as before, but with a small bug fix, and minor test improvements. It seems the workings of transport-helper are anything but clear, so let's try to clarify them a bit, and after that, hopefully it would become clearer why the last patch is actually a good fix. Felipe Contreras (6): transport-helper: clarify *:* refspec transport-helper: update refspec documentation transport-helper: clarify pushing without refspecs transport-helper: warn when refspec is not used transport-helper: trivial code shuffle transport-helper: update remote helper namespace Documentation/gitremote-helpers.txt | 12 +-- t/t5801-remote-helpers.sh | 39 ++-- transport-helper.c | 40 ++--- 3 files changed, 53 insertions(+), 38 deletions(-) -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/6] transport-helper: clarify *:* refspec
The *:* refspec doesn't work, and never has, clarify the code and documentation to reflect that. This in effect reverts commit 9e7673e (gitremote-helpers(1): clarify refspec behaviour). Signed-off-by: Felipe Contreras --- Documentation/gitremote-helpers.txt | 4 ++-- t/t5801-remote-helpers.sh | 15 --- transport-helper.c | 2 +- 3 files changed, 3 insertions(+), 18 deletions(-) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index f506031..0c91aba 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -174,8 +174,8 @@ ref. This capability can be advertised multiple times. The first applicable refspec takes precedence. The left-hand of refspecs advertised with this capability must cover all refs reported by -the list command. If a helper does not need a specific 'refspec' -capability then it should advertise `refspec *:*`. +the list command. If no 'refspec' capability is advertised, +there is an implied `refspec *:*`. 'bidi-import':: This modifies the 'import' capability. diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index f387027..cd1873c 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -120,21 +120,6 @@ test_expect_failure 'pushing without refspecs' ' compare_refs local2 HEAD server HEAD ' -test_expect_success 'pulling with straight refspec' ' - (cd local2 && - GIT_REMOTE_TESTGIT_REFSPEC="*:*" git pull) && - compare_refs local2 HEAD server HEAD -' - -test_expect_failure 'pushing with straight refspec' ' - test_when_finished "(cd local2 && git reset --hard origin)" && - (cd local2 && - echo content >>file && - git commit -a -m eleven && - GIT_REMOTE_TESTGIT_REFSPEC="*:*" git push) && - compare_refs local2 HEAD server HEAD -' - test_expect_success 'pulling without marks' ' (cd local2 && GIT_REMOTE_TESTGIT_NO_MARKS=1 git pull) && diff --git a/transport-helper.c b/transport-helper.c index dcd8d97..cea787c 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -469,7 +469,7 @@ static int fetch_with_import(struct transport *transport, * were fetching. * * (If no "refspec" capability was specified, for historical -* reasons we default to *:*.) +* reasons we default to the equivalent of *:*.) * * Store the result in to_fetch[i].old_sha1. Callers such * as "git fetch" can use the value to write feedback to the -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/6] transport-helper: update refspec documentation
The refspec capability is not only used by 'import', also by 'export', and it's recommend in both. Signed-off-by: Felipe Contreras --- Documentation/gitremote-helpers.txt | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index 0c91aba..ba7240c 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -159,11 +159,11 @@ Miscellaneous capabilities carried out. 'refspec' :: - This modifies the 'import' capability, allowing the produced - fast-import stream to modify refs in a private namespace - instead of writing to refs/heads or refs/remotes directly. - It is recommended that all importers providing the 'import' - capability use this. + For remote helpers that implement 'import' or 'export', this capability + allows the refs to be constrained to a private namespace, instead of + writing to refs/heads or refs/remotes directly. + It is recommended that all importers providing the 'import' or 'export' + capabilities use this. + A helper advertising the capability `refspec refs/heads/*:refs/svn/origin/branches/*` -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/6] transport-helper: clarify pushing without refspecs
This has never worked, since it's inception the code simply skips all the refs, essentially telling fast-export to do nothing. Let's at least tell the user what's going on. Signed-off-by: Felipe Contreras --- Documentation/gitremote-helpers.txt | 4 ++-- t/t5801-remote-helpers.sh | 6 +++--- transport-helper.c | 5 +++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt index ba7240c..4d26e37 100644 --- a/Documentation/gitremote-helpers.txt +++ b/Documentation/gitremote-helpers.txt @@ -162,8 +162,8 @@ Miscellaneous capabilities For remote helpers that implement 'import' or 'export', this capability allows the refs to be constrained to a private namespace, instead of writing to refs/heads or refs/remotes directly. - It is recommended that all importers providing the 'import' or 'export' - capabilities use this. + It is recommended that all importers providing the 'import' + capability use this. It's mandatory for 'export'. + A helper advertising the capability `refspec refs/heads/*:refs/svn/origin/branches/*` diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index cd1873c..3eeb309 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -111,13 +111,13 @@ test_expect_success 'pulling without refspecs' ' compare_refs local2 HEAD server HEAD ' -test_expect_failure 'pushing without refspecs' ' +test_expect_success 'pushing without refspecs' ' test_when_finished "(cd local2 && git reset --hard origin)" && (cd local2 && echo content >>file && git commit -a -m ten && - GIT_REMOTE_TESTGIT_REFSPEC="" git push) && - compare_refs local2 HEAD server HEAD + GIT_REMOTE_TESTGIT_REFSPEC="" test_must_fail git push 2> ../error) && + grep "remote-helper doesn.t support push; refspec needed" error ' test_expect_success 'pulling without marks' ' diff --git a/transport-helper.c b/transport-helper.c index cea787c..4d98567 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -785,6 +785,9 @@ static int push_refs_with_export(struct transport *transport, struct string_list revlist_args = STRING_LIST_INIT_NODUP; struct strbuf buf = STRBUF_INIT; + if (!data->refspecs) + die("remote-helper doesn't support push; refspec needed"); + helper = get_helper(transport); write_constant(helper->in, "export\n"); @@ -795,8 +798,6 @@ static int push_refs_with_export(struct transport *transport, char *private; unsigned char sha1[20]; - if (!data->refspecs) - continue; private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name); if (private && !get_sha1(private, sha1)) { strbuf_addf(&buf, "^%s", private); -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/6] transport-helper: warn when refspec is not used
For the modes that need it. In the future we should probably error out, instead of providing half-assed support. The reason we want to do this is because if it's not present, the remote helper might be updating refs/heads/*, or refs/remotes/origin/*, directly, and in the process fetch will get confused trying to update refs that are already updated, or older than what they should be. We shouldn't be messing with the rest of git. Signed-off-by: Felipe Contreras --- t/t5801-remote-helpers.sh | 6 -- transport-helper.c| 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index 3eeb309..1bb7529 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -100,14 +100,16 @@ test_expect_failure 'push new branch with old:new refspec' ' test_expect_success 'cloning without refspec' ' GIT_REMOTE_TESTGIT_REFSPEC="" \ - git clone "testgit::${PWD}/server" local2 && + git clone "testgit::${PWD}/server" local2 2> error && + grep "This remote helper should implement refspec capability" error && compare_refs local2 HEAD server HEAD ' test_expect_success 'pulling without refspecs' ' (cd local2 && git reset --hard && - GIT_REMOTE_TESTGIT_REFSPEC="" git pull) && + GIT_REMOTE_TESTGIT_REFSPEC="" git pull 2> ../error) && + grep "This remote helper should implement refspec capability" error && compare_refs local2 HEAD server HEAD ' diff --git a/transport-helper.c b/transport-helper.c index 4d98567..573eaf7 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -215,6 +215,8 @@ static struct child_process *get_helper(struct transport *transport) free((char *)refspecs[i]); } free(refspecs); + } else if (data->import || data->bidi_import || data->export) { + warning("This remote helper should implement refspec capability."); } strbuf_release(&buf); if (debug) -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/6] transport-helper: trivial code shuffle
Just shuffle the die() part to make it more explicit, and cleanup the code-style. Signed-off-by: Felipe Contreras --- transport-helper.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 573eaf7..9d31f2d 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -800,6 +800,9 @@ static int push_refs_with_export(struct transport *transport, char *private; unsigned char sha1[20]; + if (ref->deletion) + die("remote-helpers do not support ref deletion"); + private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name); if (private && !get_sha1(private, sha1)) { strbuf_addf(&buf, "^%s", private); @@ -807,13 +810,8 @@ static int push_refs_with_export(struct transport *transport, } free(private); - if (ref->deletion) { - die("remote-helpers do not support ref deletion"); - } - if (ref->peer_ref) string_list_append(&revlist_args, ref->peer_ref->name); - } if (get_exporter(transport, &exporter, &revlist_args)) -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 6/6] transport-helper: update remote helper namespace
When pushing, the remote namespace is updated correctly (e.g. refs/origin/master), but not the remote helper's (e.g. refs/testgit/origin/master), which currently is only updated while fetching. Since the remote namespace is used to tell fast-export which commits to avoid (because they were already imported/exported), it makes sense to have them in sync so they don't get generated twice. If the remote helper was implemented properly, they would be ignored, if not, they probably would end up repeated (probably). Signed-off-by: Felipe Contreras --- t/t5801-remote-helpers.sh | 12 transport-helper.c| 23 +++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index 1bb7529..5466969 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -153,4 +153,16 @@ test_expect_success 'push ref with existing object' ' compare_refs local dup server dup ' +test_expect_success 'push update refs' ' + (cd local && + git checkout -b update master && + echo update >>file && + git commit -a -m update && + git push origin update + git rev-parse --verify remotes/origin/update >expect && + git rev-parse --verify testgit/origin/heads/update >actual && + test_cmp expect actual + ) +' + test_done diff --git a/transport-helper.c b/transport-helper.c index 9d31f2d..da16393 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -11,6 +11,7 @@ #include "thread-utils.h" #include "sigchain.h" #include "argv-array.h" +#include "refs.h" static int debug; @@ -620,7 +621,7 @@ static int fetch(struct transport *transport, return -1; } -static void push_update_ref_status(struct strbuf *buf, +static int push_update_ref_status(struct strbuf *buf, struct ref **ref, struct ref *remote_refs) { @@ -686,7 +687,7 @@ static void push_update_ref_status(struct strbuf *buf, *ref = find_ref_by_name(remote_refs, refname); if (!*ref) { warning("helper reported unexpected status of %s", refname); - return; + return 1; } if ((*ref)->status != REF_STATUS_NONE) { @@ -695,11 +696,12 @@ static void push_update_ref_status(struct strbuf *buf, * status reported by the remote helper if the latter is 'no match'. */ if (status == REF_STATUS_NONE) - return; + return 1; } (*ref)->status = status; (*ref)->remote_status = msg; + return 0; } static void push_update_refs_status(struct helper_data *data, @@ -708,11 +710,24 @@ static void push_update_refs_status(struct helper_data *data, struct strbuf buf = STRBUF_INIT; struct ref *ref = remote_refs; for (;;) { + char *private; + recvline(data, &buf); if (!buf.len) break; - push_update_ref_status(&buf, &ref, remote_refs); + if (push_update_ref_status(&buf, &ref, remote_refs)) + continue; + + if (!data->refspecs) + continue; + + /* propagate back the update to the remote namespace */ + private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name); + if (!private) + continue; + update_ref("update by helper", private, ref->new_sha1, NULL, 0, 0); + free(private); } strbuf_release(&buf); } -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
put THEIR commits AFTER my commits with a single rebase command
I asked this on stackoverflow, but no reply. http://stackoverflow.com/questions/15971244/git-put-their-commits-after-my-commits-with-a-single-rebase-command Suppose master and origin/master diverged. I'm on master and I want to put the commits from origin/master after my commits and then push --force. A---B---C origin/master / D---E---F---G *master desired result: A---B---C origin/master / D---E---F---G---A'---B'---C' *master Variant 1: git branch -f tmp git reset --hard origin/master git rebase tmp This variant is bad, because 'git reset --hard' checks out some files and 'git rebase' rewrites them again before applying commits. It's a redundant job. Variant 2: git branch -f tmp origin/master git rebase --onto master master tmp git branch -f master git checkout master Too many commands. I want to do this with just one command. And I want to stay be on branch master in case of rebase conflicts. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] remote-hg: fix commit messages
git fast-import expects an extra newline after the commit message data, but we are adding it only on hg-git compat mode, which is why the bidirectionality tests pass. We should add it unconditionally. Signed-off-by: Felipe Contreras --- contrib/remote-helpers/git-remote-hg | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg index a5f0013..5481331 100755 --- a/contrib/remote-helpers/git-remote-hg +++ b/contrib/remote-helpers/git-remote-hg @@ -362,6 +362,8 @@ def export_ref(repo, name, kind, head): else: modified, removed = get_filechanges(repo, c, parents[0]) +desc += '\n' + if mode == 'hg': extra_msg = '' @@ -385,7 +387,6 @@ def export_ref(repo, name, kind, head): else: extra_msg += "extra : %s : %s\n" % (key, urllib.quote(value)) -desc += '\n' if extra_msg: desc += '\n--HG--\n' + extra_msg -- 1.8.2.1.679.g509521a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] remote-hg: fix commit messages
On Thu, Apr 18, 2013 at 1:06 AM, Felipe Contreras wrote: > git fast-import expects an extra newline after the commit message data, > but we are adding it only on hg-git compat mode, which is why the > bidirectionality tests pass. > > We should add it unconditionally. This doesn't depend on any other patches, and I think it might be worth to put it in 'maint', it's not like a *huge* deal, but it would be nice if a mercurial repo cloned with v1.8.2.2 and one cloned with v1.8.3 end up with the same commit ids. It would have been nicer if v1.8.1 had this already, but hey, it's new, and it's in contrib. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: put THEIR commits AFTER my commits with a single rebase command
Am 4/18/2013 7:18, schrieb Ilya Basin: > desired result: > > A---B---C origin/master > / > D---E---F---G---A'---B'---C' *master > > > > Variant 1: > > git branch -f tmp > git reset --hard origin/master > git rebase tmp Variant 1a: git reset --hard origin/master git rebase @{1} > > This variant is bad, because 'git reset --hard' checks out some files > and 'git rebase' rewrites them again before applying commits. It's a redundant job. > > Variant 2: > > git branch -f tmp origin/master > git rebase --onto master master tmp > git branch -f master > git checkout master > > Too many commands. I want to do this with just one command. And I want > to stay be on branch master in case of rebase conflicts. Perhaps this one: git merge origin/master git rebase ORIG_HEAD -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: put THEIR commits AFTER my commits with a single rebase command
On Thu, Apr 18, 2013 at 7:18 AM, Ilya Basin wrote: > I asked this on stackoverflow, but no reply. > http://stackoverflow.com/questions/15971244/git-put-their-commits-after-my-commits-with-a-single-rebase-command > > Suppose master and origin/master diverged. > I'm on master and I want to put the commits from origin/master after my > commits and then push --force. > > A---B---C origin/master > / > D---E---F---G *master > > desired result: > > A---B---C origin/master > / > D---E---F---G---A'---B'---C' *master Note that if other people are working on top of origin/master, then what you are proposing is quite rude to them, since they must now manually rebase their own work on top of your rebased history. Rewriting public history is generally considered evil. > Variant 1: > > git branch -f tmp > git reset --hard origin/master > git rebase tmp > > This variant is bad, because 'git reset --hard' checks out some files and > 'git rebase' rewrites them again before applying commits. It's a redundant > job. > Variant 2: > > git branch -f tmp origin/master > git rebase --onto master master tmp > git branch -f master > git checkout master > > Too many commands. I want to do this with just one command. And I want > to stay be on branch master in case of rebase conflicts. git cherry-pick master..origin/master ...Johan -- Johan Herland, www.herland.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
t6200: avoid path mangling issue on Windows
From: Johannes Sixt MSYS bash interprets the slash in the argument core.commentchar="/" as root directory and mangles it into a Windows style path. Use a different core.commentchar to dodge the issue. Signed-off-by: Johannes Sixt --- t/t6200-fmt-merge-msg.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh index e7e945d..54b5744 100755 --- a/t/t6200-fmt-merge-msg.sh +++ b/t/t6200-fmt-merge-msg.sh @@ -179,8 +179,8 @@ test_expect_success '--log=5 with custom comment character' ' cat >expected <<-EOF && Merge branch ${apos}left${apos} - / By Another Author (3) and A U Thor (2) - / Via Another Committer + x By Another Author (3) and A U Thor (2) + x Via Another Committer * left: Left #5 Left #4 @@ -189,7 +189,7 @@ test_expect_success '--log=5 with custom comment character' ' Common #1 EOF - git -c core.commentchar="/" fmt-merge-msg --log=5 <.git/FETCH_HEAD >actual && + git -c core.commentchar="x" fmt-merge-msg --log=5 <.git/FETCH_HEAD >actual && test_cmp expected actual ' -- 1.8.2.1.1678.gf713add -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html