Re: [PATCH 2/2] Move sequencer to builtin
Duy Nguyen wrote: libgit.a is just a way of grouping a bunch of objects together, not a real library and not meant to be. If you aim something more organized, please show at least a roadmap what to move where. Exactly. There are some rough plans I would like to help with in the direction of a more organized source tree (so ls output can be less intimidating --- see Nico Pitre's mail on this a while ago for more hints), but randomly moving files one at a time to builtin/ destroys consistency and just makes things *worse*. So if you'd like to work on this, you'll need to start with a description of the endpoint, to help people work with you to ensure it is something consistent and usable. Actually, Felipe, I doubt that would work well. This project requires understanding how a variety of people use the git source code, which requires listening carefully to them and not alienating them so you can find out what they need. Someone good at moderating a discussion could do that on-list, but based on my experience of how threads with you go, a better strategy might be to cultivate a wiki page somewhere with a plan and give it some time (a month, maybe) to collect input. NAK to changing the meaning of builtin/ to built-in commands, plus sequencer, which seriously hurts consistency. Sincerely, 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: [PATCH] tests: fix autostash
Ramkumar Ramachandra wrote: How else am I supposed to write them? If there is a stale state from the previous test, there isn't too much I can do. Or should I be cleaning up state at the beginning of each test, instead of at the end? That's one strategy. test_when_finished to restore the set-up state is another. Making tests skippable unless labelled otherwise is currently an aspirational goal rather than a practical one. Hopefully some day we'll get there and the test harness can start checking it. :) It makes reorganizing test scripts, for example by reordering tests, much easier. Thanks, 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: [PATCH 2/2] Move sequencer to builtin
Felipe Contreras wrote: This has nothing to do with better strategy, it has everything to do with gut feelings and tradition. Not reasons. I try to help you, and you insult me. I don't think this is worth it. If I were managing this list, I would ban mails from you, since this discussion style does more harm than good. If I were maintaining git, I'd still accept your contributions, waiting until times when I had more patience to read them and sending them to the list when appropriate to get more feedback. Of course I am neither managing the list nor maintaining git, but I thought I should put that out there... Annoyed, 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: [PATCH 2/2] Move sequencer to builtin
Felipe Contreras wrote: Just the past three months I've probably done more work than anybody else[1], and you would ban me because you don't like my words? Definitely, yes. Even if you look at the impact on code alone and don't care about the people, destroying a collegial work environment is harmful enough to the code to outweigh the (admittedly often useful) patches. But I am not the mailing list owner, so what I would do is not too important. 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: [PATCH 2/2] Move sequencer to builtin
Felipe Contreras wrote: A collegial work environment is overrated, and proof of that the Linux kernel, where honest and straight talk is the bread and butter of the mailing list. An aside, since it doesn't bear too much on the topic at hand: For what it's worth, in my experience the people working on the kernel are quite sensible and friendly on-list. Probably you are referring to some high-profile cases of flames, which perhaps I have just been lucky to avoid. I do not think the way the list works normally is a counterexample to common decency being useful. So no, I don't find But they are mean, and look how well they are doing! to be a compelling argument here. 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: [PATCH 2/2] Move sequencer to builtin
Jeff King wrote: My advice would be to ignore him when the discussion proceeds in an unproductive direction. There is something appealing about that option. The problem is that it doesn't work, at least for someone that relies on the list as a way of understanding patches that have been applied (which often don't have self-contained descriptions, sadly) and the context of other patches. Of course that's not the intent: the intent of ignoring someone is to hope they'll go away. :) In the context of other unhealthy behaviors (like alcoholism) there is a concept of enabling behavior. One of an addict's friends might confront her and try to help her understand that things have gone too far. Another friend says, What a mess. Let's go to a bar and talk and they are drinking again. The usual approach for avoiding this is an intervention, where a large group of people that care about a person together agree to confront the addict and make sure she actually understands and work together to find a real way out. Of course the git development community is not organized enough for an intervention, but as context I thought I'd mention that that's what works. Ramkumar Ramachandra wrote: I'll be frank: I'm a pragmatic person, and I want to see work. Despite all this mess, who has shown me the most number of patches with some direction? Felipe. Who gets the most number of patches into git.git, by far? Felipe. And who is wasting time theorizing about what's wrong with Felipe in various ways? Everyone else. In that case, I can see a simple solution. Felipe, who provides the most patches in git.git, by far (I don't know what that means, but I'll take it as an assumption), can put up a fork of git that you run. He can solicit whatever level of review he is comfortable with before pushing out changes, and then the result is available, without the pesky middle-man of those theorizers that were trying to develop git a different way and then got annoyed. No harm done, right? It doesn't have to involve the list, because what's relevant in this worldview is code, not the people. So why aren't I privately ignoring his messages and letting the list become what it may? It would seem that I'm making the problem much worse, by starting discussions that focus of how to stop pushing other contributors away instead of (what's important) code! 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: [PATCH] completion: remove ancient ensure colon in COMP_WORDBREAKS workaround
SZEDER Gábor wrote: Fedore 9 shipped the gvfs package with a buggy Bash completion script: it removed the ':' character from COMP_WORDBREAKS, thereby breaking certain features of git's completion script. We worked this around in db8a9ff0 (bash completion: Resolve git show ref:pathtab losing ref: portion, 2008-07-15). The bug was fixed in Fedora 10 and Fedora 9 reached its EOL on 2009-07-10, almost four years ago. It's about time to remove our workaround. Nice! I had wondered what that workaround was about but never ended up digging into it. Searching for COMP_WORDBREAKS in /etc/bash_completion.d/* finds similar breakage (removal of '=' and '@') in the npm completion script, but nothing involving colon. So this looks like a good change. Reviewed-by: Jonathan Nieder jrnie...@gmail.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: [PATCH] Documentation/CommunityGuidelines
Junio C Hamano wrote: 0. You do not take offense, no matter what. If someone attacks you irrationally, you do not respond. This is a public mailing list, and we are all rational people: the attacker has already humiliated herself in public, and everyone can see that. [...] I suspect it mostly has to do with the desire to make sure that bystanders do not get an impression that the one who speaks last gives the conclusion to the discussion, so stating The attacker being the one who speaks last in the discussion does not mean the conclusion is his. explicitly might be one way to make it more practically useful by alleviating the urge to respond, instead of saying no matter what. I dunno. Actually my motivation is worse than that in at least one of the cases I am assuming Ram is referring to. I don't think most bystanders would misunderstand if I let a certain person alone instead of responding and saying You are being unproductive. Please stop. But that certain person seems to misunderstand, whether I say that or not. So when I lose patience I say so, knowing that it will spark a discussion with others, knowing that that discussion needs to happen and that if the problem is not addressed I will continue to lose motivation for regular work on-list. Is that an instance of taking offense and letting emotion overtake reason? Is that against the rules? 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: git log: Add a switch to limit the number of displayed lines from the commit messages
Hi Paul, Paul Menzel wrote: there are people out there disliking elaborate commit messages, as going over `git log` is tedious as you have to scroll a lot. As I do not like the suggestion to make commit messages shorter by omitting certain details, a way to limit the number displayed lines of the commit messages would be nice to have. Have you tried gitk or tig? They split the screen so you can browse through commits, one per line, in one half and read the corresponding commit messages and patches in the other. 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: git log: Add a switch to limit the number of displayed lines from the commit messages
Junio C Hamano wrote: Or inside less that is spawned by git log -p, I often say this: /^commit .*|^diff --git .*ENTER and navigate with 'n' and 'p'. Hm, that implies an interesting trick. If I run LESS='FRSX +/^commit |^diff --git ' git log -p then 'n' and shift+'n' can be used for navigating without having to spell out the /pattern to start by hand. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Documentation/Makefile: move infodir to be with other '*dir's
Jun 17, 2013 at 01:14:51PM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: Signed-off-by: John Keeping j...@keeping.me.uk --- Thanks; will directly apply 1/2 on maint. I am not absolutely sure about this one, where variables related to an optional info support used to be in one place but with the patch only infodir is separated away. Maybe it is not a big deal, though. In practice, I think keeping the variables that specify the filesystem hierarchy together is more useful (or in other words, this looks like a good patch). Thanks, 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: [PATCH] Remove pdf target from Makefiles
Hi Thomas, Thomas Ackermann wrote: This target was only used to create user-manual.pdf with dblatex using a separate style definition than was used for user-manual.html. These two style definitions had to be maintained separately and so made improvements to user-manual.html unnecessarily hard. I don't understand. Do you mean that you want to change the rules that generate user-manual.xml? Would generating different XML files for the PDF and for other purposes (with different names) work as a way to achieve that without losing the printable manual? Thanks, 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: Re: [PATCH] Remove pdf target from Makefiles
Thomas Ackermann wrote: Would generating different XML files for the PDF and for other purposes (with different names) work as a way to achieve that without losing the printable manual? This would be even worse when we have to create different xml depending on the wanted output. The problem here is in the xml-pdf/html step: I wanted to change the formatting of links in the pdf from Section number to section_name to make it the same as in the html. Thereby I noticed that the definition for this is in files from /etc/asciidoc/dblatex. So to change it we have to introduce our own files in ./Documentation and work on them to bring the formatting of user-manual.html and user-manual.pdf closer together. Now I'm even more confused. ;-) If I understood the original commit message correctly, you were saying the XML file was not suitable for html generation and you wanted to tweak it, and were dropping the PDF target to avoid breaking it. Now if I understand correctly you are saying the XML file actually *is* suitable for html generation, and that the html generation rules just need tweaking. In that case, why remove the PDF target? Puzzled, 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: [PATCH] fix builtin-* references to be builtin/*
Hi, Phil Hord wrote: Documentation and some comments still refer to files in builtin/ as 'builtin-*.[cho]'. Update these to show the correct location. Yeah, good call. [...] --- a/builtin/help.c +++ b/builtin/help.c @@ -1,5 +1,5 @@ /* - * builtin-help.c + * builtin/help.c * It would probably be better to remove the above two lines which are redundant next to the filename. That can wait for a later patch if you like, though. With or without that change, Reviewed-by: Jonathan Nieder jrnie...@gmail.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: Re: Re: [PATCH] Remove pdf target from Makefiles
Thomas Ackermann wrote: When I want to tweak the html generation rules I also have to tweak the pdf generation rules because html and pdf should be as similiar to each other as possible. Ah, *that's* what I missed. Thanks for explaining. I think it's fine for the html and pdf to look different. After all, a better way to get an exact rendering of the HTML as a PDF would be to convert from HTML to PDF (or to print from a browser). I always assumed the point of the PDF was to look better on paper than the HTML, so as long as you're not changing the source document in some way that breaks the PDF, I don't see any harm in it. Thanks again and 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: [PATCHv2] fix builtin-* references to be builtin/*
Phil Hord wrote: Reviewed-by: Jonathan Nieder jrnie...@gmail.com Yep, still looks good from a quick glance. Thanks for the quick turnaround. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] glossary: define committish (a.k.a. commit-ish)
Hi, Richard Hansen wrote: --- a/Documentation/glossary-content.txt +++ b/Documentation/glossary-content.txt @@ -82,6 +82,17 @@ to point at the new commit. to the top def_directory,directory of the stored revision. +[[def_committish]]committish (also commit-ish):: + A def_ref,ref pointing to an def_object,object that + can be recursively dereferenced to a + def_commit_object,commit object. Usually I would expect that the string 4d1c565 is not a ref, but the glossary contains a different definition (A 40-byte hex representation of a SHA-1 or ...). I guess we need a shorter name for extended SHA-1 syntax (as described in gitrevisions(7)) that is a little less confusing. Perhaps we can sidestep the issue by saying A parameter pointing to an def_object,object that can be recursively dereferenced to ... since the most common use of commitish is in describing a command's syntax. I'm tempted to go even further and just call that a commit parameter, explaining the more pedantic synonym here --- something like [[def_commitish]]commitish (also commit-ish):: A commandline parameter to a command that requires a def_commit,commit. + The following are all commitishes: an expression (see linkgit:gitrevisions[7]) directly representing a commit object, an expression naming a tag that points to a commit object, a tag that points to a tag that points to a commit, etc. Thanks, 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: detached HEAD before root commit - possible?
Hi, SZEDER Gábor wrote: $ git init Initialized empty Git repository in /tmp/test/.git/ $ git checkout --detach fatal: You are on a branch yet to be born Are there some plumbing commands and options that would still allow this, or can I rely on that that it's impossible? gitrepository-layout(5) tells me HEAD can be in one of a few states: a) Missing. In this case the repository is considered to be corrupt and attempts to access the repository fail until the user runs git init to recover. b) A symbolic link, which is one way to represent a symref (pointing to an existing branch or an unborn branch). c) A standard symref, in the format ref: refs/some/branch. Behaves like (b). d) A direct SHA-1 reference (40-character commit object name) pointing to a commit without associating a branch (detached HEAD). e) Anything else means the repository is corrupt, as in case (a). In other words, HEAD always either points to an unborn or existing branch or an existing commit. It's not clear to me what it would mean to detach from an unborn branch. Improvements to the documentation to make that clearer would of course be welcome. ;-) Thanks, 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: [PATCH] Change remote tracking to remote-tracking
Michael Schubert wrote: --- a/Documentation/git-p4.txt +++ b/Documentation/git-p4.txt @@ -180,7 +180,7 @@ subsequent 'sync' operations. Import changes into given branch. If the branch starts with 'refs/', it will be used as is. Otherwise if it does not start with 'p4/', that prefix is added. The branch is assumed to - name a remote tracking, but this can be modified using + name a remote-tracking, but this can be modified using '--import-local', or by giving a full ref name. The default branch is 'master'. This is confusing both before and after the patch. What is a remote tracking? Perhaps: --branch ref:: Import changes into ref instead of refs/remotes/p4/master. If ref starts with refs/, it is used as is. Otherwise, if it does not start with p4/, that prefix is added. + By default a ref not starting with refs/ is treated as the name of a remote-tracking branch (under refs/remotes/). This behavior can be modified using the --import-local option. + The default ref is master. The rest of the patch looks good. Thanks, 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: [PATCH] gitweb: allow extra breadcrumbs to prefix the trail
(cc-ing Jakub, gitweb wrangler)
Tony Finch wrote:
There are often parent pages logically above the gitweb projects
list, e.g. home pages of the organization and department that host
the gitweb server. This change allows you to include links to those
pages in gitweb's breadcrumb trail.
Neat.
Signed-off-by: Tony Finch d...@dotat.at
---
Documentation/gitweb.conf.txt | 8
gitweb/gitweb.perl| 6 ++
2 files changed, 14 insertions(+)
diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index ea0526e..4579578 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -339,6 +339,14 @@ $home_link_str::
as this link leads to the list of projects. Other popular choice it to
set it to the name of site.
+@extra_breadcrumbs::
+ Additional links to be added to the start of the breadcrumb trail,
+ that are logically above the gitweb projects list. For example,
+ links to the organization and department which host the gitweb
+ server. Each element of the list is a reference to an array,
+ in which element 0 is the link text and element 1 is the
+ target URL.
Is arbitrary HTML permitted in the link text?
I think it makes sense to permit it for consistency with $home_link_str,
but it might be worth mentioning in the manpage so the administrator
knows not to set it to something user-controlled --- e.g.:
The link text can contain arbitrary HTML --- to escape link
text generated programatically, use esc_html($text).
For what it's worth, with or without such a change,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
(Patch left unsnipped for reference.)
+
$logo_url::
$logo_label::
URI and label (title) for the Git logo link (or your site logo,
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8d69ada..436f17a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -85,6 +85,9 @@ our $project_maxdepth = ++GITWEB_PROJECT_MAXDEPTH++;
# string of the home link on top of all pages
our $home_link_str = ++GITWEB_HOME_LINK_STR++;
+# extra breadcrumbs preceding the home link
+our @extra_breadcrumbs = ();
+
# name of your site or organization to appear in page titles
# replace this with something more descriptive for clearer bookmarks
our $site_name = ++GITWEB_SITENAME++
@@ -3982,6 +3985,9 @@ sub print_nav_breadcrumbs_path {
sub print_nav_breadcrumbs {
my %opts = @_;
+ for my $crumb (@extra_breadcrumbs) {
+ print $cgi-a({-href = esc_url($crumb-[1])}, $crumb-[0]) .
/ ;
+ }
print $cgi-a({-href = esc_url($home_link)}, $home_link_str) . / ;
if (defined $project) {
my @dirname = split '/', $project;
--
--
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: unexpected file deletion after using git rebase --abort
Paul A. Kennedy wrote: If we don't expect this, should we update the documentation for the --abort heading in the git rebase man page to indicate that newly staged content will be lost after a git rebase --abort? How about something along these lines? diff --git i/Documentation/git-rebase.txt w/Documentation/git-rebase.txt index 6b2e1c8..dcae40d 100644 --- i/Documentation/git-rebase.txt +++ w/Documentation/git-rebase.txt @@ -240,6 +240,9 @@ leave out at most one of A and B, in which case it defaults to HEAD. started, then HEAD will be reset to branch. Otherwise HEAD will be reset to where it was when the rebase operation was started. ++ +This discards any changes to files tracked in the working tree or branch. +You may want to stash your changes first (see linkgit:git-stash[1]). --keep-empty:: Keep the commits that do not change anything from its -- 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: Feature request: prevent push -f from pushing all branches at once
Hi Dany, Dany wrote: I had a pretty sucky thing happen to me today: while remote tracking a non-master branch, I force pushed. This had the intended effect of force pushing the branch I was working on, but also the unintended function of force pushing all branches I wasn't on. Yeah, I agree that this is lousy. When you run git push or git push -f without further arguments, current versions of git print a long message: | $ git push | warning: push.default is unset; its implicit value is changing in | Git 2.0 from 'matching' to 'simple'. To squelch this message | and maintain the current behavior after the default changes, use: | | git config --global push.default matching | | To squelch this message and adopt the new behavior now, use: | | git config --global push.default simple | | See 'git help config' and search for 'push.default' for further information. | (the 'simple' mode was introduced in Git 1.7.11. Use the similar mode | 'current' instead of 'simple' if you sometimes use older versions of Git) This is intended as a warning that git is not necessarily going to push the branches that you intended, with instructions for teaching git what you actually meant. For historical reasons (to support habits and scripts) the current default is the push matching branches behavior you ran into, but in the future the default will be a more conservative push the current branch, and only if it is configured to pull from the matching branch, allowing this long message to go away. What version of git are you using? Any ideas about how that message could be improved? (Perhaps it is too long to work as an effective warning.) Thanks and 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: [PATCH 1/2] Git.pm: add new temp_is_locked function
Hi, Kyle McKay wrote: The temp_is_locked function can be used to determine whether or not a given name previously passed to temp_acquire is currently locked. [...] +=item temp_is_locked ( NAME ) + +Returns true if the file mapped to CNAME is currently locked. + +If true is returned, an attempt to Ctemp_acquire() the same CNAME will +throw an error. Looks like this was line-wrapped somewhere in transit. More importantly, it's not obvious from the above description what this function will do. What is a typical value of NAME? Is it a filename, an arbitrary string, a reference, or something else? Is this a way of checking if a file is flocked? What is a typical way to use this function? Looking more closely, it looks like this is factoring out the idiom for checking if a name is already in use from the _temp_cache function. Would it make sense for _temp_cache to call this helper? When is a caller expected to call this function? What guarantees can the caller rely on? After a call to temp_acquire(NAME), will temp_is_locked(NAME) always return true until temp_release(NAME) is called? Does this only work within the context of a single process or can locks persist beyond a process lifetime? Do locks ever need to be broken? I didn't spend a lot of time trying to find the answers to these questions because I want to make sure that people using Git.pm in the future similarly do not have to spend a lot of time. So hopefully a documentation change could fix this. Thanks and 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: [PATCH 2/2] git-svn: allow git-svn fetching to work using serf
(cc-ing Eric Wong, who wrote this code)
Hi,
Kyle McKay wrote:
Temp file with moniker 'svn_delta' already in use at Git.pm line 1250
Temp file with moniker 'git_blob' already in use at Git.pm line 1250
David Rothenberger daver...@acm.org has determined the cause to
be that ra_serf does not drive the delta editor in a depth-first
manner [...]. Instead, the calls come in this order:
[...]
--- a/perl/Git/SVN/Fetcher.pm
+++ b/perl/Git/SVN/Fetcher.pm
@@ -315,11 +315,13 @@ sub change_file_prop {
sub apply_textdelta {
my ($self, $fb, $exp) = @_;
return undef if $self-is_path_ignored($fb-{path});
- my $fh = $::_repository-temp_acquire('svn_delta');
+ my $suffix = 0;
+ ++$suffix while
$::_repository-temp_is_locked(svn_delta_${$}_$suffix);
+ my $fh = $::_repository-temp_acquire(svn_delta_${$}_$suffix);
# $fh gets auto-closed() by SVN::TxDelta::apply(),
# (but $base does not,) so dup() it for reading in close_file
open my $dup, '', $fh or croak $!;
- my $base = $::_repository-temp_acquire('git_blob');
+ my $base = $::_repository-temp_acquire(git_blob_${$}_$suffix);
Thanks for your work tracking this down.
I'm a bit confused. Are you saying that apply_textdelta gets called
multiple times in a row without an intervening close_file?
Puzzled,
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: [PATCH 0/2] allow git-svn fetching to work using serf
David Rothenberger wrote: On 7/5/2013 8:41 PM, Kyle McKay wrote: Daniel Shahaf has suggested also setting servers:global:http-bulk-updates=on. I have a patch that does this, but since turning on bulk updates has a possible performance penalty, I prefer your approach. I assume that's because http-bulk-updates defeats caching. If so, makes sense. Please forgive my ignorance: is there a bug filed about ra_serf's misbehavior here? Is it eventually going to be fixed and this is just a workaround, or is the growth in temp file use something we'd live with permanently? Thanks, 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 (Jul 2013, #02; Fri, 5)
Hi, Junio C Hamano wrote: We are in the middle of 5th week now in the 11-week releace cycle for 1.8.4 (http://tinyurl.com/gitCal), and quite a few topics have graduated to 'master'. I'd expect the rest of the week to be slow. I'd like this or the next release to be 2.0, so the common user trouble with git push pushing more branches than they intended can be over. Am I crazy? If not, what can I do to help make it happen? Thanks, 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: [PATCH] documentation: add git transport security notice
Hi, Fraser Tweedale wrote: --- a/Documentation/urls.txt +++ b/Documentation/urls.txt @@ -11,6 +11,9 @@ and ftps can be used for fetching and rsync can be used for fetching and pushing, but these are inefficient and deprecated; do not use them). +The git transport does not do any authentication and should be used +with caution on unsecured networks. How about the something like the following? I'm starting to think it would make more sense to add a SECURITY section to git-clone(1), though. -- 8 -- Subject: doc: clarify git:// transport security notice The recently added warning about the git protocol's lack of authentication does not make it clear that the protocol lacks both client and server authentication. The lack of non IP-based client auth is obvious (when does user enter her credentials?), while the lack of authentication of the server is less so, so emphasize it. Put the warning in context by making an analogy to HTTP's security properties as compared to HTTPS. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Thanks, Jonathan diff --git a/Documentation/urls.txt b/Documentation/urls.txt index 9ccb246..bd0058f 100644 --- a/Documentation/urls.txt +++ b/Documentation/urls.txt @@ -11,8 +11,8 @@ and ftps can be used for fetching and rsync can be used for fetching and pushing, but these are inefficient and deprecated; do not use them). -The native transport (i.e. git:// URL) does no authentication and -should be used with caution on unsecured networks. +Like HTTP, the native protocol (used for git:// URLs) does no server +authentication and should be used with caution on unsecured networks. The following syntaxes may be used with them: -- 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] test-lib.sh - cygwin does not have usable FIFOs
Mark Levedahl wrote: Do not use FIFOs on cygwin, they do not work. Cygwin includes coreutils, so has mkfifo, and that command does something. However, the resultant named pipe is known (on the Cygwin mailing list at least) to not work correctly. Hm. How would you recommend going about writing a script that takes output from a command, transforms it, and then feeds it back into that command's input? Are sockets a more reliable way to do this kind of IPC on Cygwin? See reinit_git and try_dump from t9010-svn-fe.sh for context. Thanks, 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: [PATCH v3] gitweb: allow extra breadcrumbs to prefix the trail
Tony Finch wrote: Reviewed-by: Jonathan Nieder jrnie...@gmail.com Yep, fwiw this version looks perfect to me. :) Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] allow git-svn fetching to work using serf
Kyle McKay wrote: On Jul 6, 2013, at 17:28, Jonathan Nieder wrote: David Rothenberger wrote: On 7/5/2013 8:41 PM, Kyle McKay wrote: Daniel Shahaf has suggested also setting servers:global:http-bulk-updates=on. I have a patch that does this, but since turning on bulk updates has a possible performance penalty, I prefer your approach. I assume that's because http-bulk-updates defeats caching. If so, makes sense. Please forgive my ignorance: is there a bug filed about ra_serf's misbehavior here? Is it eventually going to be fixed and this is just a workaround, or is the growth in temp file use something we'd live with permanently? [...] Begin forwarded message: [...] [2] http://subversion.tigris.org/issues/show_bug.cgi?id=2932 Ah, thanks for the context. It's still not clear to me how we know that ra_serf driving the editor in a non depth-first manner is the problem here. Has that explanation been confirmed somehow? For example, does the workaround mentioned by danielsh work? Does using ra_neon instead of ra_serf avoid trouble as well? Is there a simple explanation of why violating the depth-first constraint would lead to multiple blob (i.e., file, not directory) deltas being opened in a row without an intervening close? 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: Possible error on the git-svn man page
Hi, Szuba, Marek (IKP) wrote: On the git-svn(1) man page, the third example in the Basic Examples [...] $ git checkout -b master FETCH_HEAD fatal: Cannot update paths and switch to branch 'master' at the same time. Did you intend to checkout 'FETCH_HEAD' which can not be resolved as commit? What branch are you on when you run git fetch? What does git log FETCH_HEAD say? Is this repository public, which would let people on this list try it out? Thanks, 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: [PATCH 2/2] git-svn: allow git-svn fetching to work using serf
Kyle McKay wrote: Unless bulk updates are disabled when using the serf access method (the only one available with svn 1.8) for https?: urls, apply_textdelta does indeed get called multiple times in a row without an intervening temp_release. You mean Unless bulk updates are enabled and without an intervening close_file, right? Unlike the non-depth-first thing, that sounds basically broken --- what would be stopping subversion from calling the editor's close method when done with each file? I can't see much reason unless it is calling apply_textdelta multiple times in parallel --- is it doing that, and if so is git-svn able to cope with that? This sounds like something that should be fixed in ra_serf. But if the number of overlapping open text nodes is bounded by a low number, the workaround of using multiple temp files sounds ok as a way of dealing with unfixed versions of Subversion. 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: [PATCH 0/2] allow git-svn fetching to work using serf
(cc-ing users@ as requested by danielsh) David Rothenberger wrote: On 7/6/2013 5:28 PM, Jonathan Nieder wrote: Is there a simple explanation of why violating the depth-first constraint would lead to multiple blob (i.e., file, not directory) deltas being opened in a row without an intervening close? I believe serf is doing the following for a number of files in parallel: 1. open_file 2. apply_textdelta 3. change_file_prop, change_file_prop, ... 4. close_file Ah, that makes more sense. It is not about traversal order but about processing multiple non-directory files in parallel, and step (3) potentially involving a large number of property changes means that it can make sense not to take a lock. Perhaps the reference documentation could warn about this? On the git-svn side, it looks like we have enough information to make a more complete commit message or in-code comment so the reason for multiple git_blob tempfiles is not forgotten. Thanks for your patient explanations. 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: [PATCH] builtin/push.c: use OPT_BOOL, not OPT_BOOLEAN
Junio C Hamano wrote: The command line parser of git push for --tags, --delete, and --thin options still used outdated OPT_BOOLEAN. Because these options do not give escalating levels when given multiple times, they should use OPT_BOOL. Thanks. Looks obviously correct, so Reviewed-by: Jonathan Nieder jrnie...@gmail.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: [PATCH] test-lib: avoid full path to store test results
Hi, Felipe Contreras wrote: No reason to use the full path in case this is used externally. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com No reason not to is not a reason to do anything. What symptoms does this prevent? Could you describe the benefit of this patch in a paragraph starting Now you can ...? Thanks, 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: [PATCH] test-lib: avoid full path to store test results
Elia Pinto wrote:
The shell word splitting done in base is a bashism, iow not portable.
No, ${varname##glob} is in POSIX and we already use it here and there.
See Documentation/CodingGuidelines:
- We use ${parameter#word} and its [#%] siblings, and their
doubled longest matching form.
Thanks for looking the patch over.
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: [PATCH v2 1/4] fast-export: trivial cleanup
Felipe Contreras wrote:
Setting commit to commit is a no-op.
Wrong description. This should say:
The code uses the idiom of assigning commit to itself to quench a
may be used uninitialized warning. Luckily at least modern
versions of gcc do not produce that warning here, so we can drop
the self-assignment.
This makes the code clearer to human beings, makes static
analyzers that do not know that idiom happier, and means that
if the code some day evolves to use this variable uninitialized
then we will catch it.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
With that change, for what it's worth,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Patch left unsnipped since it doesn't seem to have hit the list.
---
builtin/fast-export.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 12220ad..065f324 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -483,7 +483,7 @@ static void get_tags_and_duplicates(struct object_array
*pending,
for (i = 0; i pending-nr; i++) {
struct object_array_entry *e = pending-objects + i;
unsigned char sha1[20];
- struct commit *commit = commit;
+ struct commit *commit;
char *full_name;
if (dwim_ref(e-name, strlen(e-name), sha1, full_name) != 1)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] fast-export: fix comparisson in tests
(actually cc-ing the git list this time. Sorry for the noise, all.) Felipe Contreras wrote: [Subject: [PATCH v2 2/4] fast-export: fix comparisson in tests] First the expected, then the actual, otherwise the diff would be the opposite of what we want. Spelling: s/comparisson/comparison/. Semantics: this isn't actually fixing anything --- it's a cosmetic thing. It would be clearer to say: fast-export test: swap arguments to test_cmp This way if diff output is produced, it describes how the actual output differs from what was expected rather than the other way around. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com For what it's worth, with amended message, Reviewed-by: Jonathan Nieder jrnie...@gmail.com Patch left unsnipped because it hadn't hit the list. --- t/t9350-fast-export.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 3e821f9..49bdb44 100755 --- a/t/t9350-fast-export.sh +++ b/t/t9350-fast-export.sh @@ -303,7 +303,7 @@ test_expect_success 'dropping tag of filtered out object' ' ( cd limit-by-paths git fast-export --tag-of-filtered-object=drop mytag -- there output - test_cmp output expected + test_cmp expected output ) ' @@ -320,7 +320,7 @@ test_expect_success 'rewriting tag of filtered out object' ' ( cd limit-by-paths git fast-export --tag-of-filtered-object=rewrite mytag -- there output - test_cmp output expected + test_cmp expected output ) ' @@ -351,7 +351,7 @@ test_expect_failure 'no exact-ref revisions included' ' ( cd limit-by-paths git fast-export master~2..master~1 output - test_cmp output expected + test_cmp expected output ) ' -- 1.8.0 -- 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 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote:
They have been marked as UNINTERESTING for a reason, lets respect that.
This patch looks unsafe, and in the examples listed in the patch
description the changed behavior does not look like an improvement.
Worse, the description lists a few examples but gives no convincing
explanation to reassure about the lack of bad behavior for examples
not listed.
Perhaps this patch has a prerequisite and has come out of order.
Hope that helps,
Jonathan
Patch left unsnipped so we can get a copy in the list archive.
Currently the first ref is handled properly, but not the rest, so:
% git fast-export master ^master
Would currently throw a reset for master (2nd ref), which is not what we
want.
% git fast-export master ^foo ^bar ^roo
% git fast-export master salsa..tacos
Even if all these refs point to the same object; foo, bar, roo, salsa,
and tacos would all get a reset.
This is most certainly not what we want. After this patch, nothing gets
exported, because nothing was selected (everything is UNINTERESTING).
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
builtin/fast-export.c | 7 ---
t/t9350-fast-export.sh | 6 ++
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 065f324..7fb6fe1 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -523,10 +523,11 @@ static void get_tags_and_duplicates(struct object_array
*pending,
typename(e-item-type));
continue;
}
- if (commit-util)
+ if (commit-util) {
/* more than one name for the same object */
- string_list_append(extra_refs, full_name)-util =
commit;
- else
+ if (!(commit-object.flags UNINTERESTING))
+ string_list_append(extra_refs, full_name)-util
= commit;
+ } else
commit-util = full_name;
}
}
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 49bdb44..6ea8f6f 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -440,4 +440,10 @@ test_expect_success 'fast-export quotes pathnames' '
)
'
+test_expect_success 'proper extra refs handling' '
+ git fast-export master ^master master..master actual
+ echo -n expected
+ test_cmp expected actual
+'
+
test_done
--
1.8.0
--
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 4/4] fast-export: make sure refs are updated properly
Felipe Contreras wrote: % git fast-export $marks_args one % git fast-export $marks_args one two Then yeah, 'one' will be updated once again in the second command, That's probably worth a mention in the commit message and tests (test_expect_failure), to save future readers from some confusion. Thanks, 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: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: So you think what we have now is the correct behavior: % git fast-export master ^master reset refs/heads/master from :0 No, I don't think that, either. 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: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: Well, that's what we have now, and you want to preserve this feature (aka bug), right? Nope. I just don't want regressions, and found a patch description that did nothing to explain to the reader how it avoids regressions more than a little disturbing. I also think the proposed behavior is wrong. I don't think there's much benefit to be gained from continuing to discuss this. Consider it a single data point: I would be deeply worried if this patch were applied without at least a clearer description of the change in behavior. Maybe I'm the only one! 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: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder jrnie...@gmail.com wrote: Nope. I just don't want regressions, and found a patch description that did nothing to explain to the reader how it avoids regressions more than a little disturbing. I see, so you don't have any specific case where this could cause regressions, you are just saying it _might_ (like all patches). Yes, exactly. The commit log needs a description of the current behavior, the intent behind the current code, the change the patch makes, and the motivation behind that change, like all patches. Despite the nice examples, it doesn't currently have that. The patch description just raises more questions for the reader. From the description, one might imagine that this patch causes git fast-export mark args master not to emit anything when another branch that has already been exported is ahead of master. If I understand correctly (though I haven't tested), this patch does cause git fast-export ^next master not to emit anything when next is ahead of master. That doesn't seem like progress. I haven't reviewed the later patches in the series; maybe they fix these things. But in the long term it is much easier to understand and maintain a patch series that does not introduce regressions in the first place, and the context one might use to convincingly explain that a patch is not introducing a regression turns out to be essential for many other purposes as well. 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: [PATCH v3 4/4] fast-export: make sure refs are updated properly
Felipe Contreras wrote:
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array
*pending,
typename(e-item-type));
continue;
}
- if (commit-util) {
- /* more than one name for the same object */
+
+ /*
+ * This ref will not be updated through a commit, lets make
+ * sure it gets properly upddated eventually.
+ */
+ if (commit-util || commit-object.flags SHOWN) {
if (!(commit-object.flags UNINTERESTING))
string_list_append(extra_refs, full_name)-util
= commit;
- } else
+ }
+ if (!commit-util)
commit-util = full_name;
Here's an explanation of why the above makes sense to me.
get_tags_and_duplicates() gets called after the marks import and
before the revision walk. It walks through the revs from the
commandline and for each one:
- peels it to a refname, and then to a commit
- stores the refname so fast-export knows what arg to pass to
the commit command during the revision walk
- if it already had a refname stored, instead adds the
(refname, commit) pair to the extra_refs list, so fast-export
knows to add a reset command later.
If the commit already has the SHOWN flag set because it was pointed to
by a mark, it is not going to come up in the revision walk, so it will
not be mentioned in the output stream unless it is added to
extra_refs. That's what this patch does.
Incidentally, the change from else to if (!commit-util) is
unnecessary because if a commit is already SHOWN then it will not be
encountered in the revision walk so commit-util does not need to be
set.
If the commit does not have the SHOWN or UNINTERESTING flag set but it
is going to get the UNINTERESTING flag set during the walk because of
a negative commit listed on the command line, this patch won't help.
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: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Hi again, Felipe Contreras wrote: They have been marked as UNINTERESTING for a reason, lets respect that. So, the above description conveyed zero information, as you mentioned. A clearer explanation would be the following: fast-export: don't emit reset command for negative refs When git fast-export encounters two refs on the commandline referring to the same commit, it exports the first during the usual commit walk and the second using a reset command in a final pass over extra_refs: $ git fast-export master next reset refs/heads/master commit refs/heads/master mark :1 author Jonathan Nieder jrnie...@gmail.com 1351644412 -0700 committer Jonathan Nieder jrnie...@gmail.com 1351644412 -0700 data 17 My first commit! reset refs/heads/next from :1 Unfortunately the code to do this doesn't distinguish between positive and negative refs, producing confusing results: $ git fast-export ^master next reset refs/heads/next from :0 $ git fast-export master ^next reset refs/heads/next from :0 Use revs-cmdline instead of revs-pending to iterate over the rev-list arguments, checking the UNINTERESTING flag bit to distinguish between positive (master, --all, etc) and negative (next.., --not --all, etc) revs and avoid enqueueing negative revs in extra_revs. This does not affect revs that were excluded from the revision walk because pointed to by a mark, since those use the SHOWN bit on the commit object itself and not UNINTERESTING on the rev_cmdline_entry. A patch meeting the above description would make perfect sense to me. Except for the somewhat strange testcase, the patch I am replying to would also be fine in the short term, as long as it had an analagous description (i.e., with an appropriate replacement for the second-to-last paragraph). Thanks for your patience, and hoping 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: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: I don't think it's my job to explain to you how 'git fast-export' works. Actually, if you are submitting a patch for inclusion, it is your job to explain to future readers what the patch does. Yes, the reader might not be deeply familiar with the part of fast-export you are modifying. It might have even been modified since then, by the time the reader is looking at the change! Sad but true. Thanks, 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: [PATCH] test-lib: avoid full path to store test results
Felipe Contreras wrote: On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder jrnie...@gmail.com wrote: Felipe Contreras wrote: No reason to use the full path in case this is used externally. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com No reason not to is not a reason to do anything. What symptoms does this prevent? Could you describe the benefit of this patch in a paragraph starting Now you can ...? ./test-lib.sh: line 394: /home/felipec/dev/git/t/test-results//home/felipec/dev/git/contrib/remote-hg/test-21865.counts: No such file or directory Ok, so a description for this patch is test: use test's basename to name results file Running a test using its full path produces an error: $ ~/dev/git/contrib/remote-hg/test-21865.sh [...] ./test-lib.sh: line 394: /home/felipec/dev/t/\ test-results//home/felipec/dev/git/contrib/remote-hg/\ test-21865.counts: No such file or directory In --tee and --valgrind modes we already use the basename to name the .out and .exit files; this patch teaches the test-lib to name the .counts file the same way. That is still not enough to tell if it is a good change, though. Should the test results for contrib/remote-hg/test-* be included with the results for t/t*.sh when I run make aggregate-results? Before 60d02ccc, git-svn had its own testsuite under contrib/, with glue in contrib/git-svn/t/lib-git-svn.sh to use test-lib --- maybe that code could provide some inspiration for questions like these. 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: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: It's not my job to explain to you that 'git fast-export' doesn't work this way, you have a command line to type those commands and see for yourself if they do what you think they do with a vanilla version of git. That's exactly what I did, to make sure I'm not using assumptions as basis for arguing, it took me a few minutes. Well no, when I run git blame 10 years down the line and do not understand what your code is doing, it is not at all reasonable to expect me to checkout the parent commit, get it to compile with a modern toolchain, and type those commands for myself. Instead, the commit message should be self-contained and explain what the patch does. That has multiple parts: - first, what the current behavior is - second, what the intent behind the current behavior is. This is crucial information because presumably we want the change not to break that. - third, what change the patch makes - fourth, what the consequences of that are, in terms of new use cases that become possible and old use cases that become less convenient - fifth, optionally, how the need for this change was discovered (real-life usage, code inspection, or something else) - sixth, optionally, implementation considerations and alternate approaches that were discarded If you run git log, you'll see many good and bad examples to think over and compare to this goal. It's hard work to describe one's work well in terms that other people can understand, but I think it can be satisfying, and in any event, it's just as necessary as including comments near confusing code. Sincerely, 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: [PATCH] test-lib: avoid full path to store test results
Felipe Contreras wrote: It's all fun and games to write explanations for things, but it's not that easy when you want those explanations to be actually true, and corrent--you have to spend time to make sure of that. That's why it's useful for the patch submitter to write them, asking for help when necessary. As a bonus, it helps reviewers understand the effect of the patch. Bugs averted! [...] Either way, if obvious fixes that are one-liners require an essay for you, I give up. I guess it is fair to call a reasonable subject line plus a couple of sentences an essay. Yes, obvious fixes especially require that, since the bug caused by an obvious fix is one of the worst kinds. -- 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 4/4] fast-export: make sure refs are updated properly
Sverre Rabbelier wrote: Thanks for the thorough explanation. Perhaps some of that could make it's way into the commit message? It's fine with me if it doesn't, since the original commit message covers the basics (current behavior and intent of the change) in its first two paragraphs and anyone wanting more detail can use GIT_NOTES_REF=refs/remotes/charon/notes/full \ git show --show-notes commit to find more details. Thanks, 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: [PATCH v4 00/13] New remote-hg helper
Felipe Contreras wrote: On Wed, Oct 31, 2012 at 7:20 PM, Johannes Schindelin johannes.schinde...@gmx.de wrote: I just tested this with junio/next and it seems this issue is still unfixed: instead of reset refs/heads/blub from e7510461b7db54b181d07acced0ed3b1ada072c8 I get reset refs/heads/blub from :0 when running git fast-export ^master blub. That is not a problem. It has been discussed extensively, and the consensus seems to be that such command should throw nothing: http://article.gmane.org/gmane.comp.version-control.git/208729 Um. Are you claiming I have said that git fast-export ^master blub should silently emit nothing? Or has this been discussed extensively with someone else? 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: Lack of netiquette, was Re: [PATCH v4 00/13] New remote-hg helper
Junio C Hamano wrote: We've been hoping we can do without a rigid code of conduct written down to maintain cordial community focused on technical merits, and instead relied on people's common sense, but sense may not be so common, unfortunately, so we may have to have one. I think that except for occasional lapses this list stays pretty close to what, for example, the Fedora[1] and Ubuntu[2] codes of conduct require, and what Emily Postnews's guide[3] explains not to do. What's the next step? [1] http://www.ubuntu.com/project/about-ubuntu/conduct [2] http://www.ubuntu.com/project/about-ubuntu/conduct [3] http://www.templetons.com/brad/emily.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Wrap commit messages on `git commit -m`
Kevin wrote: As I see it, the problem is not the possibility to add new lines, but colleagues being too lazy to add them. I suspect the underlying problem is that we make it too hard to tell git which text editor to run. Ram, what platform do your colleagues use? Thanks, 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: [PATCH v3 4/4] fast-export: make sure refs are updated properly
Jeff King wrote: If so, then this series isn't regressing behavior; the only downside is that it's an incomplete fix. In theory this could get in the way of the full fix later on, but given the commit messages and the archive of this discussion, it would be simple enough to revert it later in favor of a more full fix. Is that accurate? Sorry if I am belaboring the discussion. I just want to make sure I understand the situation before deciding what to do with the topic. It sounds like the consensus at this point is not perfect, but good enough to make forward progress. Patch 1, 2, and 4 are good modulo their descriptions. They should work fine without patch 3. Patch 3 is a regression in comprehensibility. I think we can do better. Maybe all it would take is a less confusing description, and tweaks to the code (to loop over revs-cmdline instead of revs-pending) could come on top. -- 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] Documentation/log: fix description of format.pretty
Hi Ram, Ramkumar Ramachandra wrote: 59893a88 (Documentation/log: add a CONFIGURATION section, 2010-05-08) mentioned that `format.pretty` is the default for the `--format` option. Such an option never existed, False. Have you tried it? Thanks, 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: [PATCH] Documentation/log: fix description of format.pretty
Ramkumar Ramachandra wrote: Oops, I read about `--pretty` in pretty-formats.txt and didn't realize that `--format` existed. However, your patch is still wrong because there seems to be a subtle (and confusing) difference between `--pretty` and `--format`. In the latter, you can't omit the format, and expect it to be picked up from format.pretty: $ git log --format fatal: unrecognized argument: --format You can do $ git log and format.pretty will still take effect. In other words, setting format.pretty to foo is somewhat like making $ git log do $ git log --format=foo which is what the text is supposed to explain. It is based on the following text from Documentation/config.txt: format.pretty:: The default pretty format for log/show/whatchanged command, See linkgit:git-log[1], linkgit:git-show[1], linkgit:git-whatchanged[1]. I do imagine it can be made clearer. s/--format/--pretty/ does not go far enough --- it only replaces one confusing explanation with another. Hoping that clarifies, 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: [PATCH v5 11/15] remote-testgit: make clear the 'done' feature
Max Horn wrote: Aha, now I understand what this patch is about. So I would suggest this alternate commit message: remote-testgit: make it explicit clear that we use the 'done' feature Previously we relied on passing '--use-done-feature ' to git fast-export, which is easy to miss when looking at this script. I'm not immediately sure I agree this is even a problem. Is the point that other fast-import frontends do not have a --use-done-feature switch, so a typical remote helper has to do that work itself, and the sample testgit remote helper would be a more helpful example by doing that work itself? The idea behind --use-done-feature is that if fast-export exits early for some reason and its output is going to a pipe then at least the stream will be malformed, making it easier to catch errors. So there is something to be weighed here: is it more important to illustrate how to make your fast-export tool's output prefix-free, or is it more important to illustrate how to work around a fast-export tool that doesn't support that feature? The answer is not immediately obvious to me. A good description could provide context to make it obvious. Hoping that clarifies, 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: [PATCH 1/6] ident: make user_ident_explicitly_given private
Jeff King wrote:
There are no users of this global variable, as queriers
go through the user_ident_sufficiently_given accessor.
Let's make it private, which will enable further
refactoring.
[...]
--- a/cache.h
+++ b/cache.h
@@ -1149,10 +1149,6 @@ struct config_include_data {
#define CONFIG_INCLUDE_INIT { 0 }
extern int git_config_include(const char *name, const char *value, void
*data);
-#define IDENT_NAME_GIVEN 01
-#define IDENT_MAIL_GIVEN 02
-#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-extern int user_ident_explicitly_given;
extern int user_ident_sufficiently_given(void);
In v1.5.6-rc0~56^2 (2008-05-04) user_ident_explicitly_given was
introduced as a global for communication between config, ident, and
builtin-commit. In v1.7.0-rc0~72^2 (2010-01-07) readers switched to
using the common wrapper user_ident_sufficiently_given(). After
v1.7.11-rc1~15^2~18 (2012-05-21) the var is only written in ident.c,
and the variable can finally be made static.
This patch finally does that, which is a nice way to make cache.h
easier to read and change less often.
For what it's worth,
Reviewed-by: Jonathan Nieder jrnie...@gmail.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: [PATCH 3/6] var: accept multiple variables on the command line
Jeff King wrote:
This patch lets callers specify multiple variables, and
prints one per line.
Yay!
[...]
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -9,11 +9,16 @@ git-var - Show a git logical variable
SYNOPSIS
[verse]
-'git var' ( -l | variable )
+'git var' ( -l | variable... )
DESCRIPTION
---
-Prints a git logical variable.
+Prints one or more git logical variables, separated by newlines.
+
+Note that some variables may contain newlines themselves
Maybe a -z option to NUL-terminate values would be useful some day.
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value,
void *cb)
int cmd_var(int argc, const char **argv, const char *prefix)
{
- const char *val = NULL;
- if (argc != 2)
+ if (argc 2)
usage(var_usage);
if (strcmp(argv[1], -l) == 0) {
What should happen if I pass -l followed by other arguments?
[...]
--- /dev/null
+++ b/t/t0007-git-var.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='basic sanity checks for git var'
+. ./test-lib.sh
+
+test_expect_success 'get GIT_AUTHOR_IDENT' '
+ test_tick
+ echo A U Thor aut...@example.com 1112911993 -0700 expect
Do we need to hardcode the timestamp? Something like
test_cmp_filtered () {
expect=$1 actual=$2
sed -e 's/[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]/TIMESTAMP \
$actual $actual.filtered
test_cmp $expect $actual.filtered
}
...
echo A U Thor aut...@example.com $timestamp expect
git var GIT_AUTHOR_IDENT actual
test_cmp_filtered expect actual
should make reordering tests a lot easier, though it has the downside
of not being able to catch a weird bug that would make the timestamp
out of sync with reality.
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: [PATCH 4/6] var: provide explicit/implicit ident information
Jeff King wrote: Internally, we keep track of whether the author or committer ident information was provided by the user, or whether it was implicitly determined by the system. However, there is currently no way for external programs or scripts to get this information What are the intended semantics? If my machine has /etc/mailname filled out, is that an implicit identity? How about if I set the EMAIL envvar but not GIT_COMMITTER_EMAIL? If external scripts are going to start using this mechanism, they will need answers to these questions to support users that run into configuration problems. A few words on this in the documentation could probably help. On most machines I have the EMAIL envvar set explicitly, but in the recent past I relied on /etc/mailname on some others, so I'm also genuinely curious about the use case here (and too lazy to dig it up). Thanks, 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: [PATCH 5/6] Git.pm: teach ident to query explicitness
Jeff King wrote:
git var recently learned to report on whether an ident we
fetch from it was configured explicitly or implicitly. Let's
make that information available to callers of the ident
function.
Sounds sensible. Quick nits:
[...]
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -737,7 +737,7 @@ sub remote_refs {
}
-=item ident ( TYPE | IDENTSTR )
+=item ident ( TYPE | IDENTSTR [, options] )
=item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
@@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array
with the fields parsed.
Alternatively, it can take a prepared ident string (e.g. from the commit
object) and just parse it.
+If the Cexplicit option is set to 1, the returned array will contain an
+additional boolean specifying whether the ident was configure explicitly by
the
+user.
s/configure/configured/
I'd suggest adding See GIT_COMMITTER_EXPLICIT in git-var(1) for
details to make the semantics crystal clear. What do you think?
Thanks,
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: [PATCH 6/6] send-email: do not prompt for explicit repo ident
Jeff King wrote: If git-send-email is configured with sendemail.from, we will not prompt the user for the From address of the emails. If it is not configured, we prompt the user, but provide the repo author or committer as a default. Even though we probably have a sensible value for the default, the prompt is a safety check in case git generated an incorrect implicit ident string. I haven't read the code carefully, but this behavior sounds sensible, so for what it's worth, Acked-by: Jonathan Nieder jrnie...@gmail.com [...] The test scripts need to be adjusted to not expect a prompt for the sender, since they always have the author explicitly defined in the environment. Unfortunately, we cannot reliably test that prompting still happens in the implicit case, as send-email will produce inconsistent results depending on the machine config (if we cannot find a FQDN, git var will barf, causing us to exit early; At first this sounded like a bug to me --- how could the user keep working without the sysadmin intervening? But then I remembered that the user can set her name and email in .gitconfig and probably would want to in such a setup anyway. When someone writes such a test, I think it could check that git either prompts or writes a message advising to configure the user email, no? Waiting until later for that seems fine to me, though. Thanks, 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: [PATCHv2 1/8] test-lib: allow negation of prerequisites
Jeff King wrote: +test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' ' I have a visceral nervousness when reading this code, from too much unpleasant experience of bash's csh-style !history expansion. Luckily bash does not treat ! specially in the '-o sh' mode used by tests. Does this feature work when running a test explicitly using bash name of test? That's something I do from time to time to figure out whether a weird behavior is shell-specific. If it works everywhere, this patch would help me conquer my fear of exclamation points in git's tests, which would be a comfort to me and a very good thing. -- 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: [PATCHv2 2/8] t7502: factor out autoident prerequisite
Jeff King wrote: --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -738,6 +738,12 @@ test_lazy_prereq UTF8_NFD_TO_NFC ' esac ' +test_lazy_prereq AUTOIDENT ' + sane_unset GIT_AUTHOR_NAME + sane_unset GIT_AUTHOR_EMAIL + git var GIT_AUTHOR_IDENT +' Lazy prereq scripts run in a subshell, so this should be safe. Ack. Thanks, 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: [PATCHv2 3/8] ident: make user_ident_explicitly_given static
Jeff King wrote: In v1.5.6-rc0~56^2 (2008-05-04) user_ident_explicitly_given was introduced as a global for communication between config, ident, and builtin-commit. In v1.7.0-rc0~72^2 (2010-01-07) readers switched to using the common wrapper user_ident_sufficiently_given(). After v1.7.11-rc1~15^2~18 (2012-05-21), the var is only written in ident.c. Now we can make it static, which will enable further refactoring without worrying about upsetting other code. Signed-off-by: Jeff King p...@peff.net For what it's worth I liked the old commit message more. But I don't mind either. 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: [PATCHv2 4/8] ident: keep separate explicit flags for author and committer
Jeff King wrote: 1. GIT_COMMITTER_* is set explicitly, but we fallback for GIT_AUTHOR. We claim the ident is explicit, even though the author is not. 2. GIT_AUTHOR_* is set and we ask for author ident, but not committer ident. We will claim the ident is implicit, even though it is explicit. This patch uses two variables instead of one, updates both when we set the fallback values, and updates them individually when we read from the environment. Nice problem description. The fixed behavior makes sense to me, for what it's worth. Not about this patch, but: in case (1), shouldn't the author fall back to $GIT_COMMITER_NAME? -- 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: [PATCHv2 5/8] var: accept multiple variables on the command line
Jeff King wrote: This patch lets callers specify multiple variables, and prints one per line. [...] Signed-off-by: Jeff King p...@peff.net Very pleasantly done --- thanks. For what it's worth, assuming this is tested, I can't see any reason not to apply it. Reviewed-by: Jonathan Nieder jrnie...@gmail.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: [PATCHv2 7/8] Git.pm: teach ident to query explicitness
Jeff King wrote: Signed-off-by: Jeff King p...@peff.net For what it's worth, Acked-by: Jonathan Nieder jrnie...@gmail.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: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
Jeff King wrote: --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -191,15 +191,47 @@ test_expect_success $PREREQ 'Show all headers' ' test_expect_success $PREREQ 'Prompting works' ' clean_fake_sendmail - (echo Example f...@example.com - echo t...@example.com + (echo t...@example.com echo ) | GIT_SEND_EMAIL_NOTTY=1 git send-email \ --smtp-server=$(pwd)/fake.sendmail \ $patches \ 2errors + grep ^From: A U Thor aut...@example.com\$ msgtxt1 + grep ^To: t...@example.com\$ msgtxt1 +' The indentation seems strange here --- are the new grep lines continuations of the git send-email line? It's probably easier to change the structure completely: clean_fake_sendmail echo t...@examples.com prompt.input echo prompt.input GIT_SEND_EMAIL_NOTTY=1 \ git send-email --smtp-server=... $patches prompt.input grep ^From: A U Thor authorid...@example.com\$ msgtxt1 grep ^To: t...@example.com\$ msgtxt1 +test_expect_success $PREREQ,AUTOIDENT 'implicit ident prompts for sender' ' + clean_fake_sendmail + (echo Example f...@example.com + echo t...@example.com + echo + ) | + (sane_unset GIT_AUTHOR_NAME + sane_unset GIT_AUTHOR_EMAIL + sane_unset GIT_COMMITTER_NAME + sane_unset GIT_COMMITTER_EMAIL + GIT_SEND_EMAIL_NOTTY=1 git send-email \ + --smtp-server=$(pwd)/fake.sendmail \ + $patches \ + 2errors grep ^From: Example f...@example.com\$ msgtxt1 grep ^To: t...@example.com\$ msgtxt1 + ) +' Likewise: clean_fake_sendmail echo Example f...@example.com prompt.in echo t...@example.com prompt.in echo prompt.in ( sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL GIT_SEND_EMAIL_NOTTY=1 \ git send-email --smtp-server=... $patches prompt.in ) grep ^From: Example f...@example.com\$ msgtxt1 grep ^To: t...@example.com\$ msgtxt1 +test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' ' + clean_fake_sendmail + (sane_unset GIT_AUTHOR_NAME + sane_unset GIT_AUTHOR_EMAIL + sane_unset GIT_COMMITTER_NAME + sane_unset GIT_COMMITTER_EMAIL + GIT_SEND_EMAIL_NOTTY=1 export GIT_SEND_EMAIL_NOTTY + test_must_fail git send-email \ + --smtp-server=$(pwd)/fake.sendmail \ + $patches /dev/null 2errors.out + test_i18ngrep tell me who you are errors.out + ) ' Likewise: clean_fake_sendmail ( sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL GIT_SEND_EMAIL_NOTTY=1 \ git send-email --smtp-server=... $patches /dev/null 2err ) test_i18ngrep [Tt]ell me who you are err For what it's worth, with or without such changes, Acked-by: Jonathan Nieder jrnie...@gmail.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: [PATCHv2 1/8] test-lib: allow negation of prerequisites
Jeff King wrote: Yes. You can test it yourself with bash t-basic.sh. The reason is that the ! is part of history expansion, which is only enabled by default for interactive shells. Nice to hear. Thanks much for looking into it. On Wed, Nov 14, 2012 at 11:46:58PM -0800, Jonathan Nieder wrote: If it works everywhere, this patch would help me conquer my fear of exclamation points in git's tests, which would be a comfort to me and a very good thing. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] completion: make the 'basic' test more tester-friendly
SZEDER Gábor wrote: The 'basic' test uses 'grep -q' to filter the resulting possible completion words while looking for the presence or absence of certain git commands, and relies on grep's exit status to indicate a failure. [...] To make testers' life easier provide some output about the failed condition: store the results of the filtering in a file and compare its contents to the expected results by the good old test_cmp helper. Looks good. I wonder if this points to the need for a test_grep helper more generally. [...] run_completion git f - ! grep -q -v ^f out + expected + sed -n /^[^f]/p out out2 + test_cmp expected out2 Functional change: before, this would fail if out contained a blank line, while afterward it does not. I doubt that matters, though. Thanks and 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: [PATCH 2/7] completion: fix args of run_completion() test helper
SZEDER Gábor wrote: Fix this by passing the command line to run_completion() as separate words. Good catch. The change makes sense, the code looks saner after the fix, and since this is test code any breakage should be caught quickly, so Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] completion: add tests for the __gitcomp_nl() completion helper function
SZEDER Gábor wrote: --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -155,6 +155,90 @@ test_expect_success '__gitcomp - suffix' ' test_cmp expected out ' +test_expect_success '__gitcomp_nl - trailing space' ' + sed -e s/Z$// expected -\EOF '$' is usually a shell metacharacter, so it would be more comfortable to read a version that escapes it: sed -e s/Z\$// expected -\EOF Since '$/' is not a valid parameter expansion, if I understand correctly then POSIX leaves the meaning of the quoted string s/Z$// undefined (XCU §2.2.3). Luckily every shell I've tried, including bash, keeps the $ unmolested. Other parts of the file already use the same style, so I wouldn't suggest changing it in this patch. Thanks for some nice tests. Reviewed-by: Jonathan Nieder jrnie...@gmail.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: [PATCH 4/7] completion: add tests for invalid variable name among completion words
SZEDER Gábor wrote:
The breakage can
be simply bogus possible completion words, but it can also be a
failure:
$ git branch '${foo.bar}'
$ git checkout TAB
bash: ${foo.bar}: bad substitution
Or arbitrary code execution:
$ git branch '$(kilroy-was-here)'
$ git checkout TAB
$ ls -l kilroy-was-here
-rw-rw-r-- 1 jrn jrn 0 nov 17 15:40 kilroy-was-here
The final version of this patch should go to maint. Thanks for
catching it.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 15/15] fast-export: don't handle uninteresting refs
Junio C Hamano wrote: Felipe Contreras felipe.contre...@gmail.com writes: Of course, transport-helper shouldn't even be specifying the negative (^) refs, but that's another story. Hrm, I am not sure I understand what you mean by this. How should it be telling the fast-export up to what commit the receiving end should already have the history for (hence they do not need to be sent)? Or are you advocating to re-send the entire history down to the root commit every time? I think Felipe has mentioned before that he considers it the remote helper's responsibility to keep track of what commits have already been imported, for example using a marks file. Never mind that others have said that that's not the current interface (I don't yet see why it would be a good interface after a transition, but maybe it would be). Still, hopefully that clarifies the intended meaning. 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: [PATCH 2/3] do not write null sha1s to on-disk index
Hi Peff, Jeff King wrote: --- a/read-cache.c +++ b/read-cache.c @@ -1800,6 +1800,8 @@ int write_index(struct index_state *istate, int newfd) continue; if (!ce_uptodate(ce) is_racy_timestamp(istate, ce)) ce_smudge_racily_clean_entry(ce); + if (is_null_sha1(ce-sha1)) + return error(cache entry has null sha1: %s, ce-name); if (ce_write_entry(c, newfd, ce, previous_name) 0) return -1; Quick heads up: this is tripping for me in the git am --abort codepath: $ cd ~/src/linux $ git checkout v3.2.35 $ git am -3sc /tmp/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch Applying: ALSA: usb-audio: Fix missing autopm for MIDI input Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... error: Your local changes to the following files would be overwritten by merge: sound/usb/midi.c Please, commit your changes or stash them before you can merge. Aborting Failed to merge in the changes. Patch failed at 0001 ALSA: usb-audio: Fix missing autopm for MIDI input When you have resolved this problem run git am --resolved. If you would prefer to skip this patch, instead run git am --skip. To restore the original branch and stop patching run git am --abort. $ git am --abort error: cache entry has null sha1: sound/usb/midi.c fatal: unable to write new index file Unstaged changes after reset: M sound/usb/midi.c $ Reproducible using v1.8.1-rc3 and master. Bisects to 4337b585 (do not write null sha1s to on-disk index, 2012-07-28). For comparison, older gits produced $ git am --abort Unstaged changes after reset: M sound/usb/midi.c which is also not great but at least didn't involve any obviously invalid behavior. Known problem? Thanks, 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: [PATCH 2/3] do not write null sha1s to on-disk index
Jeff King wrote: I can't reproduce here. I can checkout v3.2.35, and I guess that the patch you are applying comes from f5f1654, but I don't know your local modification to sound/usb/midi.c. No local modification. The unstaged change after git am --abort to recover from a conflicted git am is a longstanding bug (at least a couple of years old). The patch creating conflicts is queue-3.2/alsa-usb-audio-fix-missing-autopm-for-midi-input.patch from git://git.kernel.org/pub/scm/linux/kernel/git/bwh/linux-3.2.y-queue.git [...] $ git am --abort Unstaged changes after reset: M sound/usb/midi.c What does your index look like afterwards? Does it have a null sha1 in it (check ls-files -s)? $ git diff-index --abbrev HEAD :100644 100644 eeefbce3873c... ... Msound/usb/midi.c $ git ls-files --abbrev -s sound/usb/midi.c 100644 eeefbce3873c 0 sound/usb/midi.c -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] do not write null sha1s to on-disk index
Jeff King wrote: Can you give more details? $ GIT_TRACE=1 git am --abort trace: exec: 'git-am' '--abort' trace: run_command: 'git-am' '--abort' trace: built-in: git 'rev-parse' '--parseopt' '--' '--abort' trace: built-in: git 'rev-parse' '--git-dir' trace: built-in: git 'rev-parse' '--show-prefix' trace: built-in: git 'rev-parse' '--show-toplevel' trace: built-in: git 'var' 'GIT_COMMITTER_IDENT' trace: built-in: git 'rev-parse' '--verify' '-q' 'HEAD' trace: built-in: git 'config' '--bool' '--get' 'am.keepcr' trace: built-in: git 'rerere' 'clear' trace: built-in: git 'rev-parse' '--verify' '-q' 'HEAD' trace: built-in: git 'read-tree' '--reset' '-u' 'HEAD' 'ORIG_HEAD' error: cache entry has null sha1: sound/usb/midi.c fatal: unable to write new index file trace: built-in: git 'reset' 'ORIG_HEAD' Unstaged changes after reset: M sound/usb/midi.c -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] do not write null sha1s to on-disk index
Jeff King wrote: Hrm. But your output does not say there is a conflict. It says you have a local modification and it does not try the merge: That's probably operator error on my part when gathering output to paste into the email. In other words, nothing to see there. :) Sorry for the confusion. [...] If I try to apply it, I get a real conflict: [...] Although running git am --abort after that does seem to produce the cache entry has null sha1 error. Yep, that is what my report should have said. 'night, 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: Makefile dependency from 'configure' to 'GIT-VERSION-FILE'
Hi Martin,
Martin von Zweigbergk wrote:
I use autoconf with git.git. I have noticed lately, especially when
doing things like git rebase -i --exec make, that ./configure is run
every time. If I understand correctly, this is because of 8242ff4
(build: reconfigure automatically if configure.ac changes,
2012-07-19).
How about this patch (untested)?
-- 8 --
Subject: build: do not automatically reconfigure unless configure.ac changed
Starting with v1.7.12-rc0~4^2 (build: reconfigure automatically if
configure.ac changes, 2012-07-19), config.status --recheck is
automatically run every time the configure script changes. In
particular, that means the configuration procedure repeats whenever
the version number changes (since the configure script changes to
support ./configure --version and ./configure --help), making
bisecting painfully slow.
The intent was to make the reconfiguration process only trigger for
changes to configure.ac's logic. Tweak the Makefile rule to match
that intent by depending on configure.ac instead of configure.
Reported-by: Martin von Zweigbergk martinv...@gmail.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
[...]
--- a/Makefile
+++ b/Makefile
@@ -2267,12 +2267,9 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % :
unimplemented.sh
mv $@+ $@
endif # NO_PYTHON
-configure: configure.ac GIT-VERSION-FILE
+configure: configure.ac
[...]
--- a/configure.ac
+++ b/configure.ac
@@ -142,7 +142,10 @@ fi
## Configure body starts here.
AC_PREREQ(2.59)
-AC_INIT([git], [@@GIT_VERSION@@], [git@vger.kernel.org])
+AC_INIT([git],
+ m4_esyscmd([ ./GIT-VERSION-GEN
+{ sed -ne 's/GIT_VERSION = //p' GIT-VERSION-FILE | xargs
echo -n; } ]),
+ [git@vger.kernel.org])
I don't think that would warrant dropping the GIT-VERSION-FILE
dependency, since the resulting configure script still hard-codes the
version number.
Sane?
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 736ecd45..2a22041f 100644
--- a/Makefile
+++ b/Makefile
@@ -2275,7 +2275,7 @@ configure: configure.ac GIT-VERSION-FILE
$(RM) $+
ifdef AUTOCONFIGURED
-config.status: configure
+config.status: configure.ac
$(QUIET_GEN)if test -f config.status; then \
./config.status --recheck; \
else \
--
1.8.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] Replace git-cvsimport with a rewrite that fixes major bugs.
Hi, Eric S. Raymond wrote: Junio C Hamano gits...@pobox.com: So..., is this a flag-day patch? After this is merged, users who have been interoperating with CVS repositories with the older cvsps have to install the updated cvsps before using a new version of Git that ships with it? Yes, they must install an updated cvsps. But this is hardly a loss, as the old version was perilously broken. Speaking with my Debian packager hat on: the updated cvsps is not available in Debian. git cvsimport is, and it has users that report bugs from time to time. With this change, I would either have to take on responsibility for maintenance of the cvsps package (not going to happen) or drop git cvsimport. That's a serious regression. The moment someone takes care of packaging the updated cvsps, I'll stop minding, though. ;-) I wouldn't be surprised if the situation on other OSes is similar. This is too early to require such a dependency. 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
[PATCH v2] build: do not automatically reconfigure unless configure.ac changed
Starting with v1.7.12-rc0~4^2 (build: reconfigure automatically if configure.ac changes, 2012-07-19), configure is automatically run every time the configure script changes. In particular, that means configure is automatically rerun whenever the version number changes (which changes the configure script to support ./configure --helpe), which makes bisecting painfully slow. The intent was to make the reconfiguration process only trigger for changes to configure.ac's logic. Tweak the Makefile rule to match that intent by depending on configure.ac instead of configure. Reported-by: Martin von Zweigbergk martinv...@gmail.com Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Martin von Zweigbergk wrote: The next line just outside the context here does depend on 'configure', which is why I thought this would not be right. Yes, the 'configure' script that is run needs to reflect the changes to configure.ac. Hopefully this version will work better. Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 736ecd45..be3bbcd4 100644 --- a/Makefile +++ b/Makefile @@ -2275,10 +2275,11 @@ configure: configure.ac GIT-VERSION-FILE $(RM) $+ ifdef AUTOCONFIGURED -config.status: configure +config.status: configure.ac $(QUIET_GEN)if test -f config.status; then \ ./config.status --recheck; \ else \ + $(MAKE) configure \ ./configure; \ fi reconfigure config.mak.autogen: config.status -- 1.8.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] build: do not automatically reconfigure unless configure.ac changed
Jonathan Nieder wrote: Starting with v1.7.12-rc0~4^2 (build: reconfigure automatically if configure.ac changes, 2012-07-19), configure is automatically run every time the configure script changes. In particular, that means configure is automatically rerun whenever the version number changes (which changes the configure script to support ./configure --helpe) Gah, I sent the log commit message --- the patch description from v1 is the right one. Sorry for the trouble. Here is the fixed description again. Subject: build: do not automatically reconfigure unless configure.ac changed Starting with v1.7.12-rc0~4^2 (build: reconfigure automatically if configure.ac changes, 2012-07-19), config.status --recheck is automatically run every time the configure script changes. In particular, that means the configuration procedure repeats whenever the version number changes (since the configure script changes to support ./configure --version and ./configure --help), making bisecting painfully slow. The intent was to make the reconfiguration process only trigger for changes to configure.ac's logic. Tweak the Makefile rule to match that intent by depending on configure.ac instead of configure. Reported-by: Martin von Zweigbergk martinv...@gmail.com Signed-off-by: Jonathan Nieder jrnie...@gmail.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: [PATCH v2] build: do not automatically reconfigure unless configure.ac changed
Jeff King wrote: It seems I am late to the party. But FWIW, this looks the most sane to me of the patches posted in this thread. Thanks. config.status runs ./configure itself, though, so the rule should actually be config.status: configure.ac $(QUIET_GEN)$(MAKE) configure \ if test -f config.status; then \ ./config.status --recheck; \ else \ ./configure; fi Rather than screw it up yet again, I'm going to sleep. :) If someone else corrects the patch before tomorrow, I won't mind. -- 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] Replace git-cvsimport with a rewrite that fixes major bugs.
Eric S. Raymond wrote: Jonathan Nieder jrnie...@gmail.com: The former is already loudly advertised in the package description and manpage, at least lets you get work done, and works fine for simple repositories with linear history. Two of the three claims in this paragraph are false. Give me a break. Are you telling me that when multiple users read a manpage that states | WARNING: for certain situations the import leads to incorrect | results. Please see the section ISSUES for further reference. [...] | Problems related to timestamps: [...] | Problems related to branches: [...] | Problems related to tags: [...] | consider using these alternative tools which proved to be more | stable in practice: and a package description that states | The git cvsimport tool can incrementally import from a repository | that is being actively developed and only requires remote access | over CVS protocol. Unfortunately, in many situations the import | leads to incorrect results. For reliable, one-shot imports, cvs2git | from the cvs2svn package or parsecvs may be a better fit. and decide to use the tool anyway, this is not evidence that the tool is invaluable to them, despite its shortcomings? Perhaps the users reporting bugs didn't read the manpage and package description (despite quoting the same passages and explaining why they used the command nonetheless) or I should ignore the judgement calls they make. Consider the following workflow: 1. Update imported project periodically using git-cvsimport 2. Hack, do code archaeology using git log -S and git bisect, etc. 3. Fall back to a web browser and cvsweb to confirm conclusions. You are telling me that it is not a regression to change the workflow to the following: 1. Try to use git-cvsimport. 2. Wonder where that command went. Meanwhile Junio has already suggested a way out. Just rename the command. 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: [PATCH] Add getenv.so for catching invalid getenv() use via LD_PRELOAD
Nguyễn Thái Ngọc Duy wrote: Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- Perhaps this will help the getenv bug hunting Even if no one decides to do the getenv hunting (I haven't decided yet whether it's worth the trouble, though patches like the setup_path() one that make string lifetimes clearer are valuable anyway), this looks useful as a debugging tool when people on strange platforms report problems. And unlike the trick I sent a while ago, it doesn't involve recompiling git. So for what it's worth, Acked-by: Jonathan Nieder jrnie...@gmail.com Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: cvsps, parsecvs, svn2git and the CVS exporter mess
Eric S. Raymond wrote: Michael Haggerty wants me to trust that cvs2git's analysis stage has been fixed, but I must say that is a more difficult leap of faith when two of the most visible things about it are still (a) a conspicuous instance of interface misdesign, and (b) documentation that is careless and incomplete. For what it's worth, I use cvs2git quite often. I've found it to work well and its code to be clear and its developers responsive. But I don't mind if we disagree, and multiple implementations to explore the design space of importers doesn't seem like a terrible outcome. Thanks for your work, 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: [PATCH] Alphabetize the fast-import options, following a suggestion on the list.
Eric S. Raymond wrote: --- Missing sign-off. Depending on when you prefer to add the sign-off, something like echo '[alias] c = commit --signoff' ~/.gitconfig or echo '[alias] f = format-patch --signoff' ~/.gitconfig might be useful for the future, assuming you already look over what you are sending out in a mail client to avoid mistakes. Documentation/git-fast-import.txt | 94 +++ 1 file changed, 45 insertions(+), 49 deletions(-) My knee-jerk response was If the options are currently organized logically, wouldn't it be more appropriate to add a sub-heading for each group of options and alphabetize only within the subgroups? But in fact the current options list doesn't seem to be well organized at all. What do you think would be a logical way to group these? Features of input syntax --date-format --done Verbosity --quiet --stats Marks handling (checkpoint/restore) --import-marks --import-marks-if-exists --export-marks --relative-marks Semantics of execution --dry-run --force --cat-blob-fd --export-pack-edges Tuning --active-branches --max-pack-size --big-file-threshold --depth -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-fast-import(1): remove duplicate --done option
John Keeping wrote: The --done option to git-fast-import is documented twice in its manual page. Combine the best bits of each description, keeping the location of the instance that was added first. Signed-off-by: John Keeping j...@keeping.me.uk Good catch, thanks. Reviewed-by: Jonathan Nieder jrnie...@gmail.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: [PATCH] Documentation: fix man page dependency on asciidoc.conf
John Keeping wrote:
When building manual pages, the source text is transformed to XML with
AsciiDoc before the man pages are generated from the XML with xmlto.
Fix the dependency in the Makefile so that the XML files are rebuilt
when asciidoc.conf changes and not just the manual pages from unchanged
XML.
Good catch, thanks.
Would something like the following make sense, to make it more obvious
how the dependency needs to be adjusted if we change the $(ASCIIDOC)
command line for some reason?
diff --git i/Documentation/Makefile w/Documentation/Makefile
index e53d333e..971977b8 100644
--- i/Documentation/Makefile
+++ w/Documentation/Makefile
@@ -178,8 +178,6 @@ all: html man
html: $(DOC_HTML)
-$(DOC_HTML) $(DOC_MAN1) $(DOC_MAN5) $(DOC_MAN7): asciidoc.conf
-
man: man1 man5 man7
man1: $(DOC_MAN1)
man5: $(DOC_MAN5)
@@ -257,7 +255,7 @@ clean:
$(RM) $(cmds_txt) *.made
$(RM) manpage-base-url.xsl
-$(MAN_HTML): %.html : %.txt
+$(MAN_HTML): %.html : %.txt asciidoc.conf
$(QUIET_ASCIIDOC)$(RM) $@+ $@ \
$(ASCIIDOC) -b xhtml11 -d manpage -f asciidoc.conf \
$(ASCIIDOC_EXTRA) -agit_version=$(GIT_VERSION) -o $@+ $ \
@@ -270,7 +268,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
$(QUIET_XMLTO)$(RM) $@ \
$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $
-%.xml : %.txt
+%.xml : %.txt asciidoc.conf
$(QUIET_ASCIIDOC)$(RM) $@+ $@ \
$(ASCIIDOC) -b docbook -d manpage -f asciidoc.conf \
$(ASCIIDOC_EXTRA) -agit_version=$(GIT_VERSION) -o $@+ $ \
@@ -286,7 +284,7 @@ technical/api-index.txt: technical/api-index-skel.txt \
$(QUIET_GEN)cd technical '$(SHELL_PATH_SQ)' ./api-index.sh
technical/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../
-$(patsubst %,%.html,$(API_DOCS) technical/api-index $(TECH_DOCS)): %.html :
%.txt
+$(patsubst %,%.html,$(API_DOCS) technical/api-index $(TECH_DOCS)): %.html :
%.txt asciidoc.conf
$(QUIET_ASCIIDOC)$(ASCIIDOC) -b xhtml11 -f asciidoc.conf \
$(ASCIIDOC_EXTRA) -agit_version=$(GIT_VERSION) $*.txt
diff --git i/t/test-terminal.perl w/t/test-terminal.perl
index 10172aee..1fb373f2 100755
--- i/t/test-terminal.perl
+++ w/t/test-terminal.perl
@@ -31,7 +31,7 @@ sub finish_child {
} elsif ($? 127) {
my $code = $? 127;
warn died of signal $code;
- return $code - 128;
+ return $code + 128;
} else {
return $? 8;
}
--
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] archive-tar: split long paths more carefully
René Scharfe wrote:
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -153,6 +153,8 @@ static unsigned int ustar_header_chksum(const struct
ustar_header *header)
static size_t get_path_prefix(const char *path, size_t pathlen, size_t
maxlen)
{
size_t i = pathlen;
+ if (i 1 path[i - 1] == '/')
+ i--;
Beautiful.
--
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] fix compilation with NO_PTHREADS
Jeff King wrote: I happened to notice this while looking at the sigpipe topic. I guess not many people are building with NO_PTHREADS these days. Or that those people don't build next. :) Thanks for catching it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] run-command: encode signal death as a positive integer
Jeff King wrote: I'd expecting cooking this patch for a while would flush out any I missed. Heh, probably not. ;-) But I tried to examine all the callsites (and only found the two messages I mentioned), and among the reviewers, I'm guessing we hit them all. Ciao, 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: [Feature request] make git buildable from a separate directory
Hi Manlio, Manlio Perillo wrote: Many C projects I have seen (based on autoconf, but not always - like Nginx) allow the project to be build in a separate directory, in order to avoid to pollute the working directory with compiled files. Unfortunately this seems to not be possible with Git. I believe the Cygwin build[1] does this, so that could be a starting point if you want to work on it. They use the VPATH variable[2]. Hope that helps, Jonathan [1] http://cygwin.com/ml/cygwin/2012-02/msg00391.html [2] https://www.gnu.org/software/make/manual/html_node/General-Search.html#General-Search -- 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] clone: support atomic operation with --separate-git-dir
Duy Nguyen wrote: And because junk_work_tree is not set up for --bare, I guess we still need to fix --bare --separate-git-dir case, or forbid it because i'm not sure if that case makes sense at all. Forbidding it makes sense to me. If some day we want a git command to create a .git file, I don't think it should be spelled as git clone --bare --separate-git-dir repo. In the meantime, printf '%s\n' 'gitdir: /path/to/repo' repo.git works fine. -- 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: Version 1.8.1 does not compile on Cygwin 1.7.14
Hi, Torsten Bögershausen wrote: Stephen Linda Smith wrote: git co 9fca6c make all ... The make errored out as before [...] git co 9fca6c^ make all ... and this compiles to completion CYGWIN_NT-5.1 WINXPMACHINE 1.7.14(0.260/5/3) 2012-04-24 17:22 i686 Cygwin [...] You can either upgrade to cygwin 1.17 or higher. Or, if that is really not possible (because you are sitting on a production machine, where no changes are allowed), You can enable this in Makefile: CYGWIN_V15_WIN32API = YesPlease What I don't understand is why commit 9fca6c would have had any effect at all. Since 1.7.14 doesn't match /^1\.[1-6]\./, wouldn't the setting to avoid including sys/stat.h and sys/errno.h be unset both before and after that commit? Stephen, what is the content of your config.mak? Curious, 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: Version 1.8.1 does not compile on Cygwin 1.7.14
Torsten Bögershausen wrote: The short version: Cygwin versions 1.7.1 up to 1.7.16 use the same header files as cygwin 1.5 [...] I don't know if we want to improve the Makefile to enable CYGWIN_V15_WIN32API = YesPlease for cygwin versions 1.7.1 .. 1.7.16 (which are outdated) Confusing. Sounds like the condition in 380a4d92 (Update cygwin.c for new mingw-64 win32 api headers, 2012-11-11) was too strict and the Makefile should say something like # Cygwin versions up to 1.7.16 used the same headers # as Cygwin 1.5. ifeq ($(shell expr $(uname_R) : '1\.7\.[0-9]$$'),5) CYGWIN_V15_WIN32API = YesPlease endif ifeq ($(shell expr $(uname_R) : '1\.7\.1[0-6]$$'),6) CYGWIN_V15_WIN32API = YesPlease endif ifeq ($(shell expr $(uname_R) : '1\.[1-6]\.'),4) CYGWIN_V15_WIN32API = YesPlease ... endif Is that right? -- 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] clone: forbid --bare --separate-git-dir dir
Nguyễn Thái Ngọc Duy wrote: --separate-git-dir was added to clone with the repository away from standard position worktree/.git. It does not make sense to use it without creating working directory. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com The patch correctly implements the above. The description leaves out detail. I'd say something like The --separate-git-dir option was introduced to make it simple to put the git directory somewhere outside the worktree, for example when cloning a repository for use as a submodule. It was not intended for use when creating a bare repository. In that case there is no worktree and it is more natural to directly clone the repository and create a .git file as separate steps: git clone --bare /path/to/repo.git bar.git printf 'gitdir: bar.git\n' foo.git Unfortunately we forgot to forbid the --bare --separate-git-dir combination. In practice, we know no one could be using --bare with --separate-git-dir because it is broken in the following way: explanation here. So it is safe to make good on our mistake and forbid the combination, making the command easier to explain. I don't know what would go in the explanation here blank above, though. Is it possible that some people are relying on this option combination? -- 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] docs: manpage XML depends on asciidoc.conf
When building manual pages, the source text is transformed to XML with AsciiDoc before the man pages are generated from the XML with xmlto. Fix the dependencies in the Makefile so that the XML files are rebuilt when asciidoc.conf changes and not just the manual pages from unchanged XML, and move the dependencies from a recipeless rule to the rules with commands that use asciidoc.conf to make the dependencies easier to understand and maintain. Reported-by: John Keeping j...@keeping.me.uk Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Junio C Hamano wrote: Care to do a real patch? Here you go. Documentation/Makefile | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Documentation/Makefile b/Documentation/Makefile index e53d333e..971977b8 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -178,8 +178,6 @@ all: html man html: $(DOC_HTML) -$(DOC_HTML) $(DOC_MAN1) $(DOC_MAN5) $(DOC_MAN7): asciidoc.conf - man: man1 man5 man7 man1: $(DOC_MAN1) man5: $(DOC_MAN5) @@ -257,7 +255,7 @@ clean: $(RM) $(cmds_txt) *.made $(RM) manpage-base-url.xsl -$(MAN_HTML): %.html : %.txt +$(MAN_HTML): %.html : %.txt asciidoc.conf $(QUIET_ASCIIDOC)$(RM) $@+ $@ \ $(ASCIIDOC) -b xhtml11 -d manpage -f asciidoc.conf \ $(ASCIIDOC_EXTRA) -agit_version=$(GIT_VERSION) -o $@+ $ \ @@ -270,7 +268,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in $(QUIET_XMLTO)$(RM) $@ \ $(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $ -%.xml : %.txt +%.xml : %.txt asciidoc.conf $(QUIET_ASCIIDOC)$(RM) $@+ $@ \ $(ASCIIDOC) -b docbook -d manpage -f asciidoc.conf \ $(ASCIIDOC_EXTRA) -agit_version=$(GIT_VERSION) -o $@+ $ \ @@ -286,7 +284,7 @@ technical/api-index.txt: technical/api-index-skel.txt \ $(QUIET_GEN)cd technical '$(SHELL_PATH_SQ)' ./api-index.sh technical/%.html: ASCIIDOC_EXTRA += -a git-relative-html-prefix=../ -$(patsubst %,%.html,$(API_DOCS) technical/api-index $(TECH_DOCS)): %.html : %.txt +$(patsubst %,%.html,$(API_DOCS) technical/api-index $(TECH_DOCS)): %.html : %.txt asciidoc.conf $(QUIET_ASCIIDOC)$(ASCIIDOC) -b xhtml11 -f asciidoc.conf \ $(ASCIIDOC_EXTRA) -agit_version=$(GIT_VERSION) $*.txt -- 1.8.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: Version 1.8.1 does not compile on Cygwin 1.7.14
Mark Levedahl wrote: However, the newer win32api is provided only for the current cygwin release series, which can be reliably identified by having dll version 1.7.x, while the older frozen releases (dll versions 1.6.x from redhat, 1.5.x open source) still have the older api as no updates are being made for the legacy version(s). Ah. That makes sense, thanks. (For the future, if we wanted to diagnose an out-of-date win32api and print a helpful message, I guess cygcheck would be the command to use.) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
