Re: [PATCH] test-lib.sh - cygwin does not have usable FIFOs
Mark Levedahl wrote: However, I don't understand why git would need to consume its own output - If named pipes are really needed to use git-svn because git-svn depends upon git feeding the same git process, then that package should not be available on cygwin or any other platform that does not support fifos. This isn't about git-svn but about the svn remote helper. The same considerations would apply to any other foreign repository importer that uses git fast-import --cat-blob-fd. So I would like to make sure it is at least possible to support such a thing in the Cygwin and mingw ports. If not, then I don't think the test suite should require fifos or any other construct with the same git process feeding itself either, it just blurs the line about what is actually being tested. I'm not sure I follow. Are you saying Windows users would never want to access Subversion repositories? 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.sh - cygwin does not have usable FIFOs
Torsten Bögershausen wrote: Disabling PIPE under cygwin seems to be the right thing to do, or do I miss something ? If fifos don't work on Cygwin, disabling that test prerequisite is defintely the right thing to do. I was taking the opportunity to find out whether and how git could be tweaked to avoid needing fifos when setting pipes up in scripts. 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] templates: Use heredoc in pre-commit hook
Hi, Richard Hartmann wrote: Spawning a new subprocess for every line printed is inefficient. Use heredoc, instead. I think this makes sense as a code clarity, simplicity, and internationalizability improvement, but don't like the precedent of eliminating 'echo' for the sake of fork removal (unless we have measurements showing it's worthwhile, which would be included here). Maybe a simpler commit message could sidestep the issue? Use a heredoc instead of an echo for each line. Based on 98770971aef8d1cbc78876d9023d10aa25df0526 in original patch series from 2013-06-10. Please don't include this. The audience for the commit message doesn't have that commit to compare to. If you want to preserve the original date, the way to do that is a Date: field at the top of the message body. Date: Fri, 28 Jun 2013 21:16:19 +0530 Spawning a new subprocess for ... [...] --- a/templates/hooks--pre-commit.sample +++ b/templates/hooks--pre-commit.sample @@ -31,18 +31,19 @@ if [ $allownonascii != true ] test $(git diff --cached --name-only --diff-filter=A -z $against | LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0 then - echo Error: Attempt to add a non-ascii file name. - echo - echo This can cause problems if you want to work - echo with people on other platforms. - echo - echo - echo If you know what you are doing you can disable this - echo check using: - echo - echo git config hooks.allownonascii true - echo + cat -EOF +Error: Attempt to add a non-ascii file name. Using cat \EOF would make reading easier since the reader then doesn't have to worry about whether the text being cat'ed is indented or uses variable substitutions. - echo To be portable it is advisable to rename the file ... +To be portable it is advisable to rename the file. Yes, nice. With the above nits addressed, this change looks to be going in the right direction. Thanks. 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/6] templates: Reformat pre-commit hook's message
Richard Hartmann wrote: Now that we're using heredoc, the message can span the full 80 chars. The output is going to a console and not an email, so makes sense. :) Verbatim copy of 634709b489bb3db79f59127fd6bf79c5fd9b5ddf in original patch series from 2013-06-10. As in patch 1, please drop this. I'll stop mentioning that for the later patches, but the same comment applies there. 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 4/6] Documentation: Update manpage for pre-commit hook
Richard Hartmann wrote: --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -80,7 +80,8 @@ causes the 'git commit' to abort. The default 'pre-commit' hook, when enabled, catches introduction of lines with trailing whitespaces and aborts the commit when -such a line is found. +such a line is found. It will also prevent addition of non-ASCII +file names. The tenses are inconsistent here (catches versus will also). It also seems odd to call the sample hooks default hooks, but that's a wider problem and should probably be fixed by one commit all at once (maybe imitating the wording of the prepare-commit-message description). Previously enabling them was a matter of a chmod +x and the wording made more sense. How about: The default 'pre-commit' hook, when enabled, prevents introduction of lines with trailing whitespace and prevents introduction of files with non-ASCII filenames unless the hooks.allowNonAscii configuration variable is true. -- To unsubscribe from this list: send the line 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] templates: Fix ASCII art in pre-rebase hook
Richard Hartmann wrote: The example assumes 8-char wide tabs and breaks for people with 4-char wide tabs. Convert all of those tabs to whitespace, instead. Makes sense --- we cannot assume much about the end-user's editor setup used to look at sample hooks. 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 6/6] template: Fix comment indentation in pre-rebase hook
Richard Hartmann wrote: The other hooks use two whitespace for indentation instead of tabs to signify code in the example/echo output. Follow the same layout in templates/hooks--pre-rebase.sample I don't understand the point of this one. Is it just consistency for the sake of consistency? Aren't other parts of git inconsistent in this area? -- To unsubscribe from this list: send the line 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] templates: Use heredoc in pre-commit hook
Richard Hartmann wrote: fwiw, I replaced my one single echo with heredoc as you suggested I do that. I don't mind undoing that, or I can drop it from this series altogether. Guidance would be appreciated. :) Thanks for your work, and no problem. Both Junio's and my responses were about the (confusing and false) commit message. Code is not the only thing that matters when submitting a patch --- commit messages become part of the product, too, and are especially important as documentation that guides future contributors. So my advice is to fix the commit message, prepare improvements to later patches in the series with help from reviewers where needed, and then resubmit. My review also included some advice about the code. Naturally I would be happy if that was of use, too. ;-) 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 7/7] push: document --lockref
Johannes Sixt wrote: Sorry, IMO, this goes into a totally wrong direction, in particular, I think that this is going to close to door to make --lockref the default some day in a way that helps everyone. Would a '*' that acts like --lockref on a per ref basis address your concerns? I realize that that design would hurt a project of making '+' use lockref automatically some day. I think that's ok, and that '+' meaning push whatever I have, regardless of what's on the other end, and I mean it would be better semantics in the long term (which doesn't match the current behavior either :/). 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 7/7] push: document --lockref
Jonathan Nieder wrote: Johannes Sixt wrote: Sorry, IMO, this goes into a totally wrong direction, in particular, I think that this is going to close to door to make --lockref the default some day in a way that helps everyone. Would a '*' that acts like --lockref on a per ref basis address your concerns? (Aside: '*' is not a great character for that. * is already taken in refspec syntax. There's no clash but the two uses would be confusing. *: *:* Some other single-character prefix could work, such as '.' or '~'.) -- To unsubscribe from this list: send the line 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/4] daemon.c:handle: Remove unneeded check for null pointer.
Hi,
Stefan Beller wrote:
addr doesn't need to be checked at that line as it it already accessed
7 lines before in the if (addr-sa_family).
Good catch. This asymmetry has been present since the lines were first
introduced (all guarded by if (addr)) in v1.4.1-rc1~3^2~4 (Log peer
address when git-daemon called from inetd, 2006-06-20).
--- a/daemon.c
+++ b/daemon.c
@@ -754,19 +754,19 @@ static void handle(int incoming, struct sockaddr *addr,
socklen_t addrlen)
}
if (addr-sa_family == AF_INET) {
struct sockaddr_in *sin_addr = (void *) addr;
inet_ntop(addr-sa_family, sin_addr-sin_addr, addrbuf + 12,
sizeof(addrbuf) - 12);
snprintf(portbuf, sizeof(portbuf), REMOTE_PORT=%d,
ntohs(sin_addr-sin_port));
#ifndef NO_IPV6
- } else if (addr addr-sa_family == AF_INET6) {
+ } else if (addr-sa_family == AF_INET6) {
At this point 'addr' is ss.sa from service_loop, so it really cannot
be NULL.
So fwiw, I like this patch.
--
To unsubscribe from this list: send the line 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] commit: Fix a memory leak in determine_author_info
Stefan Beller wrote: Signed-off-by: Stefan Beller stefanbel...@googlemail.com Thanks. That was quick. :) 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/4] diff-no-index: Remove unused variable.
Stefan Beller wrote:
[Subject: diff-no-index: Remove unused variable.]
[...]
--- a/diff-no-index.c
+++ b/diff-no-index.c
[...]
- else if (!strcmp(argv[i], -q)) {
+ else if (!strcmp(argv[i], -q))
- options |= DIFF_SILENT_ON_REMOVED;
i++;
- }
This feature was obviously never tested with --no-index, so I agree it
makes sense to remove it. Probably the commit message and a comment
should say so, though. E.g.:
diff --no-index: remove nonfunctional -q handling
Before v1.5.6-rc1~41^2~2, the option parsing for diff --no-index
and git diff-files shared code. In git diff-files, -q means
to be silent about removed files. In git diff --no-index, in
various versions it has been an error, an infinite loop, or a no-op.
Simplify the code to clarify that it is now a no-op, continuing to
accept and ignore the -q option in git diff --no-index to avoid
breaking scripts.
I wouldn't mind removing support for -q altogether, by the way (as a
separate change).
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/4] diff.c: Do not initialize a variable, which gets reassigned anyway.
Stefan Beller wrote:
Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
diff.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
[...]
--- a/diff.c
+++ b/diff.c
@@ -1677,21 +1677,19 @@ static void show_stats(struct diffstat_t *data,
struct diff_options *options)
}
/*
* scale the add/delete
*/
add = added;
del = deleted;
if (graph_width = max_change) {
- int total = add + del;
-
- total = scale_linear(add + del, graph_width,
max_change);
+ int total = scale_linear(add + del, graph_width,
max_change);
Yeah, we should have caught this in review.
Thanks for reporting.
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] gitweb: Ensure OPML text fits inside its box.
Tony Finch wrote: The rss_logo CSS style has a fixed width which is too narrow for the string OPML. Replace the fixed width with horizontal padding so the text fits with nice margins. Sounds sensible. Can we have your sign-off? (Likewise for the next 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 7/7] push: document --lockref
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Then I will use (5c) git push --force which means not to use this new lockref trick that looks at my remote-tracking branch and instead to just force the ref update. I am not sure I follow. Do other contributors update this remote repository? They are only using fast-forward updates, so their updates may not lose anything we pushed, but with --force, aren't you losing their work on top of yours? Yep, I meant that when you really *do* want to force a push regardless of what's on the remote end, the current --force behavior is more useful than --lockref. The example I used to introduce (5c) is too vague to be useful. A more compelling example (to me, at least) is the one from later in that message involving a relay, which does not involve other contributors at all. That is, suppose I maintain a mirror of the branches from git://repo.or.cz/git.git by pushing regularly to a hosting service where I do not have shell access. Since I can't fetch from the target repository or push from the source, I instead fetch and then push from a relay, like this:I might push like this: git fetch upstream git push --force origin refs/remotes/upstream/*:refs/heads/* Or, in the same spirit, with a detached HEAD: git fetch upstream refs/heads/*:refs/heads/* git push --force origin : The --force is to account for pu and next rewinding. In this scenario, assuming I have exclusive access to the repository and the push updates the remote-tracking branches, --lockref and --force work equally well. The commands might run once every 6 hours using a cronjob. Now suppose my relay has some downtime. That's fine --- I can still maintain the mirror by running the same commands on another machine. But when the old relay comes back up, push --lockref will fail and pu and next in my mirror are not updated any more. That is why I said that --force is more appropriate than --lockref for this application. 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 1/2] git-multimail: an improved replacement for post-receive-email
Michael Haggerty wrote: Add git-multimail, a tool for generating notification emails for pushes to a Git repository. It is largely plug-in compatible with post-receive-email, and is proposed to eventually replace that script. The advantages of git-multimail relative to post-receive-email are described in README.migrate-from-post-receive-email. Yay! contrib/hooks/multimail/README | 486 contrib/hooks/multimail/README.Git | 15 + .../README.migrate-from-post-receive-email | 146 ++ contrib/hooks/multimail/git_multimail.py | 2394 contrib/hooks/multimail/migrate-mailhook-config| 270 +++ contrib/hooks/multimail/post-receive | 90 + 6 files changed, 3401 insertions(+) create mode 100644 contrib/hooks/multimail/README create mode 100644 contrib/hooks/multimail/README.Git create mode 100644 contrib/hooks/multimail/README.migrate-from-post-receive-email create mode 100755 contrib/hooks/multimail/git_multimail.py create mode 100755 contrib/hooks/multimail/migrate-mailhook-config create mode 100755 contrib/hooks/multimail/post-receive I wouldn't be surprised if it is possible to improve this in some way or other, but where as usual Reviewed-by means If I were maintainer I would commit and push it out and as a non maintainer I vouch for its suitability, Reviewed-by: Jonathan Nieder jrnie...@gmail.com The patch integrates it into the git tree sanely and further refinements can safely come on top. Thanks for your hard work. -- To unsubscribe from this list: send the line 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 2/2] post-receive-email: deprecate script in favor of git-multimail
Michael Haggerty wrote: Add a notice to the top of post-receive-email explaining that the script is no longer under active development and pointing the user to git-multimail. I think the spirit of this patch is sane. Some thoughts on wording: [...] --- a/contrib/hooks/post-receive-email +++ b/contrib/hooks/post-receive-email @@ -2,10 +2,19 @@ # # Copyright (c) 2007 Andy Parkins # -# An example hook script to mail out commit update information. This hook -# sends emails listing new revisions to the repository introduced by the -# change being reported. The rule is that (for branch updates) each commit -# will appear on one email and one email only. +# An example hook script to mail out commit update information. +# +# ***NOTICE***: This script is no longer under active development. It +# has been superseded by git-multimail, which is more capable and +# configurable and is largely backwards-compatible with this script; +# please see contrib/hooks/multimail/. For instructions on how to +# migrate from post-receive-email to git-multimail, please see +# README.migrate-from-post-receive-email in that directory. I think I'd say something like (1) # An example hook script to mail out commit update information. # # This script is kept for compatibility, but it is no longer actively # maintained. Consider switching to git-multimail, which is more # configurable and largely compatible with this script. See # contrib/hooks/multimail/README.migrate-from-post-receive. # # This hook sends emails listing new revisions ... or, if I wanted to emphasize the warning, (2) # An example hook ... # # Warning: this script is kept for compatibility, but it is no longer # actively maintained. Consider switching to ... or, if I wanted to avoid seeming to promise that the script will be around in the future, (3) # An example hook ... # # Warning: this script is no longer actively maintained. Consider # switching to ... I prefer (2), which makes it clear to the reader that it is dangerous to keep using the script (since no one is actively chasing down bugs) while also making it clear why a potentially buggy script with a good natural successor is still in contrib for now. 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] .mailmap: Combine more (email, name) to individual persons
Stefan Beller wrote: Signed-off-by: Stefan Beller stefanbel...@googlemail.com Markup and methodology look correct. Fwiw, Reviewed-by: Jonathan Nieder jrnie...@gmail.com Is this meant to be squashed with 94b410bb (.mailmap: Map email addresses to names, 2013-07-12)? 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: repo consistency under crashes and power failures?
Greg Troxel wrote: Alternatively, is there somewhere a first-principles analysis vs POSIX specs (such as fsyncing object files before updating refs to point to them, which I realize has performance negatives)? You might be interested in the 'core.fsyncobjectfiles' setting. git-config(1) has details. 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 v3 1/6] t4000-diff-format.sh: modernize style
Matthieu Moy wrote: Signed-off-by: Matthieu Moy matthieu@imag.fr --- t/t4000-diff-format.sh | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) This test script can use more cleanup, but as preparation for later patches in this series the above is enough. :) If I forget to do more cleanup as a followup, feel free to kick me. 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 v3 3/6] diff: allow --patch cie to override -s/--no-patch
Matthieu Moy wrote: All options that trigger a patch output now override --no-patch. The case of --binary is particular as the name may suggest that it turns Usage nit: this should say is unusual or In the case of --binary in particular, the name may suggest a normal patch into a binary patch, but it actually already enables patch output when normally disabled (e.g. git log --binary displays a patch), hence it makes sense that git show --no-patch --binary display the binary patch. [...] --- a/t/t4000-diff-format.sh +++ b/t/t4000-diff-format.sh @@ -71,4 +71,9 @@ test_expect_success 'git diff-files --no-patch as synonym for -s' ' test_must_be_empty err ' +test_expect_success 'git diff-files --no-patch --patch shows the patch' ' + git diff-files --no-patch --patch diff-np-output 2err + compare_diff_patch expected actual Shouldn't that be compare_diff_patch expected diff-np-output? A couple of other test ideas: - git diff-files --patch --no-patch - git diff-files -s --patch-with-stat Aside from that, 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: [RFC/PATCH] Add the NO_SENTINEL build variable
Ramsay Jones wrote: One of the three gcc compilers that I use does not understand the sentinel function attribute. (so, it spews 108 warning messages) Do you know what version of gcc introduced the sentinel attribute? Would it make sense for the ifdef in git-compat-util.h to be keyed on __GNUC__ and __GNUC_MINOR__ instead of a new makefile flag? 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 6/6] Documentation/git-log.txt: capitalize section names
Matthieu Moy wrote: This is the convention in other files and even at the beginning of git-log.txt The docs aren't so consistent on this, but I agree that it makes sense to at least be consistent within the generated git-log.html. :) Generally the series looks very good. Thanks for taking this on. -- To unsubscribe from this list: send the line 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 3/6] diff: allow --patch cie to override -s/--no-patch
Matthieu Moy wrote: Jonathan Nieder jrnie...@gmail.com writes: Matthieu Moy wrote: All options that trigger a patch output now override --no-patch. The case of --binary is particular as the name may suggest that it turns Usage nit: this should say is unusual I don't get it. The point is not that --binary is unusual, but that it is a particular case that deserves extra attention. Ah, so you mean: The case of --binary deserves extra attention because is particular would be an unusual expression, meaning something like is made of particles. It's a weird case in English usage where a word commonly appears attached to a noun (This particular case) but cannot be used as the RHS of is (This case is particular). [...] A couple of other test ideas: - git diff-files --patch --no-patch - git diff-files -s --patch-with-stat I'd rather avoid having a too long list here, or we'll end-up testing all combinations of options ... Sure. The point of --patch --no-patch is to test that ordering is respected. The point of --no-patch --patch-with-stat is so we remember that there are options other than --patch that should override --no-patch, for example if this code is ever converted to parse_options some day. I'll send a reroll tomorrow. Thanks for the quick and thoughtful work. -- To unsubscribe from this list: send the line 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 0/6] Make git show -s easier to discover for users
Matthieu Moy wrote: Matthieu Moy (6): t4000-diff-format.sh: modernize style diff: allow --no-patch as synonym for -s diff: allow --patch cie to override -s/--no-patch Documentation/git-show.txt: include common diff options, like git-log.txt Documentation: move description of -s, --no-patch to diff-options.txt Documentation/git-log.txt: capitalize section names Documentation/diff-options.txt | 5 Documentation/git-log.txt | 8 +++ Documentation/git-show.txt | 9 +++ Documentation/rev-list-options.txt | 3 --- diff.c | 30 ++-- t/t4000-diff-format.sh | 48 +++--- 6 files changed, 75 insertions(+), 28 deletions(-) Thanks! I think this series is ready now. 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 1/2] apply, entry: speak of submodules instead of subprojects
Thomas Rast wrote: There are only four (with some generous rounding) instances in the current source code where we speak of subproject instead of submodule. They are as follows: [...] Let's at least change the error messages to consistently call them submodule. Signed-off-by: Thomas Rast tr...@inf.ethz.ch --- This and the next one are message changes for things I found during my review. Thanks. It's nice when translation results in the messages in English being improved, too. 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 2/2] show-branch: fix description of --date-order
Thomas Rast wrote: The existing description reads as if it somehow applies a filter. Change it to explain that it is merely about the ordering. [...] OPT_SET_INT(0, date-order, sort_order, - N_(show commits where no parent comes before its + N_(sort commits such that no parent comes before its children), REV_SORT_BY_COMMIT_DATE), I fear this wording tweak doesn't go far enough. The above description seems to describe --topo-order just as well as --date-order. How about something like N_(topologically sort, maintaining date order where possible), ? I haven't checked the code to see if that's accurate, though. Is the idea that: - by default, commits are listed in commit date order (newest first) - with --topo-order, they are topologically sorted in such a way as to ensure that in cases like ---1---2---4---7 \ \ 3---5---6---8 (from git-log(1)), parallel tracks are not interleaved - with --date-order, they are topologically sorted but less aggressively, in particular matching commit date order in the usual case that that is already topologically sorted. That would make --topo-order stronger than show commits in topological order --- it should say something like sort trying to avoid breaking up lines of development. -- To unsubscribe from this list: send the line 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] howto: Use all-space indentation in ASCII art
Dirk Wallenstein wrote: Those text files are installed as documentation (at least on my distribution). That's probably a distribution bug (or a git makefile bug, depending on how you look at it). It would be better to ship the HTML documentation, converted to text, instead of keeping the version with markup including occasional random \ signs, linkgit:, ``, etc. What distribution do you use? (As maintainer of packaging for a Linux distro, I know at least one that is guilty of this.) 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: t3032 incompatible with Cygwin/Windows
Mark Levedahl wrote: Subtests 6,7, and 9 of t3032 fail on Cygwin, and I presume will fail on msysgit for similar reasons. Looking at test 6, the expected result is a line ending with \r\n in text.txt. This line is extracted with grep (grep 'justice and holiness' text.txt actual), with unavoidable result that on Cygwin the line ending is \n. Would using perl instead of grep fix this? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t3032 - make compatible with systems using \r\n as a line ending
Mark Levedahl wrote: Subtests 6, 7, and 9 rely test that merge-recursive correctly ignores whitespace when so directed. Change the particular whitespace sequences to be ones that are not known line endings so the whitespace is not changed when being extracted by line oriented grep. merge-recursive needs to be able to deal with \r at EOL, too, so if at all possible I would prefer to see the test fixed to pass on Cygwin some other way. 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 v4 2/2] git-svn: allow git-svn fetching to work using serf
(cc-ing Eric Wong, who maintains git-svn and knows both it and the libsvn perl bindings much better than I do) Kyle J. McKay wrote: 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: Thanks. Sorry to nitpick, but the problem is not depth-first versus breadth-first versus random. Blaming the traversal order makes this completely confusing. The actual problem is that the driver asks us to keep multiple files open at a time. The approach taken in this patch would be racy if the driver calls us multiple times concurrently (since temp_acquire can fail). I believe it doesn't but haven't checked. The approach is generally good. I wanted to propose some clearer documentation for temp_is_locked() but didn't end up finding a moment for it, so... meh. I'll be happy to help get the details right if someone else finds time for that (hint, hint). 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 v3 1/2] Git.pm: add new temp_is_locked function
Kyle J. McKay wrote: That change was made as a result of this feedback: On Jul 6, 2013, at 17:11, Jonathan Nieder wrote: 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 [snip] 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? So I think the answer is it does not make sense for _temp_cache to call this helper. Thanks for looking into it. Sorry for the confusion. The point of my question was an example of a way to make sure the internal API stays easy to understand. But it seems to have backfired, and this is a small enough isolated change that I think it's okay to say let's clean it up later. Will release a v4 in just a moment with that single change reverted. 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] Add the GIT_SENTINEL macro
Ramsay Jones wrote: This was built on the next branch All the uses of the sentinel attribute seem to come from eccb6149 (use sentinel function attribute for variadic lists, 2013-07-09), so this should be okay to go on top of the jk/gcc-function-attributes branch. --- a/git-compat-util.h +++ b/git-compat-util.h @@ -303,6 +303,13 @@ extern char *gitbasename(char *); #endif #endif +/* The sentinel attribute is valid from gcc version 4.0 */ +#if defined(__GNUC__) (__GNUC__ = 4) +#define GIT_SENTINEL(n) __attribute__((sentinel(n))) +#else +#define GIT_SENTINEL(n) +#endif I'd mildly prefer #if ... #define GIT_SENTINEL __attribute__((sentinel)) #else ... (without the numeric parameter). I don't know any function in git (or any other project for that matter) that takes extra parameters after the NULL sentinel. But I don't care much, so Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks for a pleasant patch. 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] diff: deprecate -q option to diff-files
Junio C Hamano wrote: We should remove the support for -q in Git 2.0. No. I hope you are teasing. I don't mind seeing support for -q dropped, but I really don't think it's worth delaying git 2.0 for that. Would s/in Git 2.0/in some future release/ be ok? The patch text itself 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] Add the GIT_SENTINEL macro
Junio C Hamano wrote: It seems to apply well on the tip of jk/gcc-function-attributes. - This macro is not about git at all, so I'll edit the patch to call it GCC_ATTR_SENTINEL before applying. Would naming it something like LAST_ARG_MUST_BE_NULL instead make sense? That way, if some other compiler gains a different syntax for the same annotation, it would be possible to do #if defined(__GNUC__) __GNUC__ = 4 # define LAST_ARG_MUST_BE_NULL __attribute__((sentinel)) #elif defined(_MSC_VER) _MSC_VER 27 # define LAST_ARG_MUST_BE_NULL __declspec(lastargnull) #else # define LAST_ARG_MUST_BE_NULL #endif -- To unsubscribe from this list: send the line 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, git reset: fix typo.
Stefan Beller wrote: --- a/Documentation/git-reset.txt +++ b/Documentation/git-reset.txt @@ -9,7 +9,7 @@ SYNOPSIS [verse] 'git reset' [-q] [tree-ish] [--] paths... -'git reset' (--patch | -p) [tree-sh] [--] [paths...] +'git reset' (--patch | -p) [tree-ish] [--] [paths...] 'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [commit] Sure. Though these days it might make more sense to say 'git reset' [--soft | --mixed | --hard | --merge | --keep] commit [--] 'git reset' [tree] [--] path... 'git reset' (--patch | -p) [tree] [--] [path...] since commands accept a commit or tag where a tree is expected pretty much universally. 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 6/6] diff: deprecate -q option to diff-files
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: I don't mind seeing support for -q dropped, but I really don't think it's worth delaying git 2.0 for that. Would s/in Git 2.0/in some future release/ be ok? I do not think keeping the support for -q in is any huge burden. We do not have to remove it, forever, for that matter. I agree with the above, which is why I don't want a promise to remove the -q option to cause Git 2.0 to be delayed. It would be better to schedule it for Git 3.0, or for another unspecified future git release. I thought the 2.0 boundary was a time for changes that everyone already knew we should make, where we had been waiting for a good moment to change behavior while giving people adequate warning to avoid disrupting them too much. We have a good collection of those for 2.0, and the next batch can wait until 3.0. Sorry for the lack of clarity, 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] Cygwin has trustable filemode
Mark Levedahl wrote: After merging the following into current pu, all tests that run by default pass on Cygwin 1.7, i.e. prove -j 8 t[0-9]*.sh reports All tests successful. I've *never* had this happen on Cygwin before. Nice. Thanks for your hard work. -- To unsubscribe from this list: send the line 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: getting git from kernel.org is failing
Jeff King wrote: then smart HTTP works fine. I wonder if there is a problem in the cgit setup on kernel.org (or if it was even intended that you could fetch from the cgit URL at all; the cgit page shows the clone URLs in /pub/scm/git). I didn't think cgit URLs were meant to be clonable, but since ls-remote works on them, it seems I thought wrong. :) Odd. 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: [PATCHv3] git-tag man: when to use lightweight or annotated tags
Jeff King wrote: --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -42,6 +42,17 @@ is used to specify custom GnuPG binary. GnuPG key for signing. The configuration variable `gpg.program` is used to specify custom GnuPG binary. +Tag objects (created with `-a`, `s`, or `-u`) are called annotated +tags; they contain a creation date, the tagger name and e-mail, a +tagging message, and an optional GnuPG signature. Whereas a +lightweight tag is simply a name for an object (usually a commit +object). + +Annotated tags are meant for release while lightweight tags are meant +for private or temporary object labels. For this reason, some git +commands for naming objects (like `git describe`) will ignore +lightweight tags by default. Very readable, and I think this is a good start. Reviewed-by: Jonathan Nieder jrnie...@gmail.com Eventually the description section should probably be tweaked to start by explaining what the command is actually for. ;-) -- To unsubscribe from this list: send the line 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] builtins: search builtin commands via binary search.
Hi,
Stefan Beller wrote:
--- a/git.c
+++ b/git.c
@@ -309,9 +309,18 @@ static int run_builtin(struct cmd_struct *p, int argc,
const char **argv)
return 0;
}
+static int compare_internal_command(const void *a, const void *b) {
+ /* The first parameter is of type char* describing the name,
+the second is a struct cmd_struct */
Style:
/*
* Multi-line comments in git look like this, with an initial
* /* line, a leading * on each line with text, and a line
* with '*' '/' at the end.
*/
[...]
@@ -447,12 +456,12 @@ static void handle_internal_command(int argc, const
char **argv)
argv[0] = cmd = help;
}
- for (i = 0; i ARRAY_SIZE(commands); i++) {
- struct cmd_struct *p = commands+i;
- if (strcmp(p-cmd, cmd))
- continue;
+ struct cmd_struct *p = (struct cmd_struct *)bsearch(cmd, commands,
+ ARRAY_SIZE(commands), sizeof(struct cmd_struct),
+ compare_internal_command);
No need to cast --- this is C.
Fun. Does this result in a measurable speedup, or is it just for more
pleasant reading?
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] commit: correct advice about aborting a cherry-pick
Jeff King wrote: Your patch is just swapping out git reset for cherry-pick --abort, so I think that is a good improvement in the meantime. Um, wasn't the idea of the original message that you can run git reset and then git cherry-pick --continue? Confused, 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] commit: correct advice about aborting a cherry-pick
Jeff King wrote: --- a/builtin/commit.c +++ b/builtin/commit.c @@ -63,8 +63,14 @@ N_(The previous cherry-pick is now empty, possibly due to conflict resolution.\ If you wish to commit it anyway, use:\n \n git commit --allow-empty\n +\n); +static const char empty_cherry_pick_advice_skip_single[] = +N_(Otherwise, please use 'git reset'\n); +static const char empty_cherry_pick_advice_skip_multi[] = +N_(If you wish to skip this commit, use:\n \n -Otherwise, please use 'git reset'\n); +git reset git cherry-pick --continue\n +\n); Hmm, wouldn't it be more consistent to either say If you wish to commit it anyway, use git commit --allow-empty git cherry-pick --continue If you wish to skip this commit, use git reset git cherry-pick --continue Or If you wish to commit it anyway, use git commit --allow-empty If you wish to skip this commit, use git reset Then git cherry-pick --continue will resume cherry-picking the remaining commits. ? -- To unsubscribe from this list: send the line 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] imap-send: use Apple's Security framework for base64 encoding
Hi, David Aguilar wrote: --- a/imap-send.c +++ b/imap-send.c @@ -22,14 +22,11 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ -#include cache.h -#include exec_cmd.h -#include run-command.h -#include prompt.h #ifdef NO_OPENSSL typedef void *SSL; #else #ifdef APPLE_COMMON_CRYPTO +/* git-compat-util.h overwrites ctype.h; this must be included first */ #include CommonCrypto/CommonHMAC.h Thanks for your work on this. Currently each translation unit of git includes git-compat-util.h or a header like cache.h that includes git-compat-util.h before doing anything else, since otherwise feature test macros are not set before the first system header is included. The above (CommonCrypto needing to be included before some of the definitions from git-compat-util.h) suggests to me that CommonCrypto should just be included directly from git-compat-util.h in some appropriate place. That way any other header that needs CommonCrypto routines only has to include git-compat-util.h first as usual and doesn't have to worry about the order of other #includes. Could that work? 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 v2] Provide some linguistic guidance for the documentation.
Junio C Hamano wrote: Is that accurate? My impression has been: The documentation liberally mixes US and UK English (en_US/UK) norms for spelling and grammar, which is somewhat unfortunate. In an ideal world, it would have been better if it consistently used only one and not the other, and we would have picked en_US. I'm not convinced that would be better, even in an ideal world. It's certainly useful to have a consistent spelling of each term to make searching with grep easier. But searches with grep do not work well with line breaks anyway, and search engines for larger collections of documents seem to know about the usual spelling variants (along with knowing about stemming, etc). Unless we are planning to provide a separate en_GB translation, it seems unfortunate to consistently have everything spelled in the natural way for one group of people and in an unnatural way for another, just in the name of having a convention. I am not sure it makes sense for the documentation to say A huge disruptive patch of such-and-such specific kind of no immediate benefit is unwelcome. Isn't there some more general principle that implies that? Or the CodingGuidelines could simply say The documentation uses a mixture of U.S. and British English. My two cents, 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
[regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces
Hi Joey, Joey Hess wrote[1]: Commit c334b87b30c1464a1ab563fe1fb8de5eaf0e5bac caused a reversion in git-cat-file --batch. With an older version: joey@gnu:~/tmp/rrrgit cat-file --batch :file name e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 blob 0 With the new version: joey@wren:~/tmp/rgit cat-file --batch :file name :file missing This has broken git-annex's support for operating on files/directories containing whitespace. I cannot see a way to query such a filename using the new interface. Oh dear. Luckily you caught this before the final 1.8.4 release. I wonder if we should just revert c334b87b (cat-file: split --batch input lines on whitespace, 2013-07-11) for now. Thanks, Jonathan [1] http://bugs.debian.org/718517 -- To unsubscribe from this list: send the line 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: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces
Junio C Hamano wrote: Here is what is on top of the revert that has been pushed out on 'pu'. For what it's worth, Reviewed-by: Jonathan Nieder jrnie...@gmail.com [...] To remain backwards compatible, we cannot split on whitespace by default, hence we will ship 1.8.4 with the commit reverted. [...] It might be more robust to have something like -z to separate the input elements. But this patch is still a reasonable step before having that. It makes the easy cases easy; people who do not care about %(rest) do not have to consider it, and the %(rest) code handles the spaces and newlines of rev-list --objects correctly. Another idea for the future might be to start rejecting refnames starting with a double-quote '', which would make it safe to treat a leading quote-mark as the start of a C-style quoted string. But currently that would technically be a breaking change, making -z more useful in the meantime. I think several commands already don't deal well with filenames with newlines. I hope POSIX forbids them (with some suitable migration plan) soonish and even wouldn't mind if git were taught to refuse to track them. 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
[PATCH] log doc: the argument to --encoding is not optional
$ git log --encoding fatal: Option '--encoding' requires a value $ git rev-list --encoding fatal: Option '--encoding' requires a value The argument to --encoding has always been mandatory. Unfortunately manpages like git-rev-list(1), git-log(1), and git-show(1) have described the option's syntax as --encoding[=encoding] since it was first documented. Clarify by removing the extra brackets. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Documentation/git-rev-list.txt | 2 +- Documentation/pretty-options.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt index 65ac27e0..045b37b8 100644 --- a/Documentation/git-rev-list.txt +++ b/Documentation/git-rev-list.txt @@ -40,7 +40,7 @@ SYNOPSIS [ \--right-only ] [ \--cherry-mark ] [ \--cherry-pick ] -[ \--encoding[=encoding] ] +[ \--encoding=encoding ] [ \--(author|committer|grep)=pattern ] [ \--regexp-ignore-case | -i ] [ \--extended-regexp | -E ] diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index 5e499421..eea0e306 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -28,7 +28,7 @@ people using 80-column terminals. This is a shorthand for --pretty=oneline --abbrev-commit used together. ---encoding[=encoding]:: +--encoding=encoding:: The commit objects record the encoding used for the log message in their encoding header; this option can be used to tell the command to re-code the commit log message in the encoding -- 1.8.4.rc1 -- To unsubscribe from this list: send the line 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/RFC] log doc: explain --encoding=none and default output encoding
Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- I'm not thrilled with the wording. This can probably be explained more simply. Ideas? Documentation/pretty-options.txt | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index 5e499421..e31fd494 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -32,8 +32,14 @@ people using 80-column terminals. The commit objects record the encoding used for the log message in their encoding header; this option can be used to tell the command to re-code the commit log message in the encoding - preferred by the user. For non plumbing commands this - defaults to UTF-8. + preferred by the user. --encoding=none means to use the + raw log message without paying attention to its encoding header. ++ +For non plumbing commands, the output encoding defaults to the commit +encoding (as set using the `i18n.commitEncoding` variable, or UTF-8 +by default). This default can be overridden using the +`i18n.logOutputEncoding` configuration item. See linkgit:git-config[1] +for details. --notes[=ref]:: Show the notes (see linkgit:git-notes[1]) that annotate the -- 1.8.4.rc1 -- To unsubscribe from this list: send the line 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/RFC 0/3] post-receive-email: explicitly set Content-Type header
Hi all, This is a revival of [1], which declares encoding in emails to make it more likely that they can be read. I like to think it avoids the mistakes of previous attempts, but I'll let you judge. :) Sorry for the long delay. Thoughts of all kinds welcome, as always. Thanks, Jonathan Gerrit Pape (1): hooks/post-receive-email: set declared encoding to utf-8 Jonathan Nieder (2): hooks/post-receive-email: use plumbing instead of 'git log' and 'git show' hooks/post-receive-email: force log messages in UTF-8 contrib/hooks/post-receive-email | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) [1] http://thread.gmane.org/gmane.comp.version-control.git/181737/focus=183070 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] hooks/post-receive-email: use plumbing instead of git log/show
This way the hook doesn't have to keep being tweaked as porcelain learns new features like color and pagination. While at it, replace the git rev-list | git shortlog idiom with plain git shortlog for simplicity. Except for depending less on the value of settings like '[log] abbrevCommit', no change in output intended. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- contrib/hooks/post-receive-email | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email index 15311502..72084511 100755 --- a/contrib/hooks/post-receive-email +++ b/contrib/hooks/post-receive-email @@ -471,7 +471,7 @@ generate_delete_branch_email() echowas $oldrev echo echo $LOGBEGIN - git show -s --pretty=oneline $oldrev + git diff-tree -s --always --pretty=oneline $oldrev echo $LOGEND } @@ -547,11 +547,11 @@ generate_atag_email() # performed on them if [ -n $prevtag ]; then # Show changes since the previous release - git rev-list --pretty=short $prevtag..$newrev | git shortlog + git shortlog $prevtag..$newrev else # No previous tag, show all the changes since time # began - git rev-list --pretty=short $newrev | git shortlog + git shortlog $newrev fi ;; *) @@ -571,7 +571,7 @@ generate_delete_atag_email() echowas $oldrev echo echo $LOGBEGIN - git show -s --pretty=oneline $oldrev + git diff-tree -s --always --pretty=oneline $oldrev echo $LOGEND } @@ -617,7 +617,7 @@ generate_general_email() echo if [ $newrev_type = commit ]; then echo $LOGBEGIN - git show --no-color --root -s --pretty=medium $newrev + git diff-tree -s --always --pretty=medium $newrev echo $LOGEND else # What can we do here? The tag marks an object that is not @@ -636,7 +636,7 @@ generate_delete_general_email() echowas $oldrev echo echo $LOGBEGIN - git show -s --pretty=oneline $oldrev + git diff-tree -s --always --pretty=oneline $oldrev echo $LOGEND } -- 1.8.4.rc1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] hooks/post-receive-email: set declared encoding to utf-8
From: Gerrit Pape p...@smarden.org
Date: Thu, 11 Dec 2008 20:27:21 +
Some email clients (e.g., claws-mail) display the message body
incorrectly when the charset is not defined explicitly in a
Content-Type header. git log generates logs in UTF-8 encoding by
default, so add a Content-Type header declaring that encoding to
the emails the post-receive-email example hook sends.
[jn: also setting the Content-Transfer-Encoding so MTAs know what
kind of mangling might be needed when sending to a non 8-bit clean
SMTP host]
Requested-by: Alexander Gerasiov g...@debian.org
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
That's the end of the series. Thanks for reading.
contrib/hooks/post-receive-email | 3 +++
1 file changed, 3 insertions(+)
diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index ba93a0d8..8ee410f8 100755
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -242,6 +242,9 @@ generate_email_header()
cat -EOF
To: $recipients
Subject: ${emailprefix}$projectdesc $refname_type $short_refname
${change_type}d. $describe
+ MIME-Version: 1.0
+ Content-Type: text/plain; charset=utf-8
+ Content-Transfer-Encoding: 8bit
X-Git-Refname: $refname
X-Git-Reftype: $refname_type
X-Git-Oldrev: $oldrev
--
1.8.4.rc1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] hooks/post-receive-email: force log messages in UTF-8
Git commands write commit messages in UTF-8 by default, but that default can be overridden by the [i18n] commitEncoding and logOutputEncoding settings. With such a setting, the emails written by the post-receive-email hook use a mixture of encodings: 1. Log messages use the configured log output encoding, which is meant to be whatever encoding works best with local terminals (and does not have much to do with what encoding should be used for email) 2. Filenames are left as is: on Linux, usually UTF-8, and in the Mingw port (which uses Unicode filesystem APIs), always UTF-8 3. The This is an automated email preface uses a project description from .git/description, which is typically in UTF-8 to support gitweb. So (1) is configurable, and (2) and (3) are unconfigurable and typically UTF-8. Override the log output encoding to always use UTF-8 when writing the email to get the best chance of a comprehensible single-encoding email. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- contrib/hooks/post-receive-email | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email index 72084511..ba93a0d8 100755 --- a/contrib/hooks/post-receive-email +++ b/contrib/hooks/post-receive-email @@ -471,7 +471,7 @@ generate_delete_branch_email() echowas $oldrev echo echo $LOGBEGIN - git diff-tree -s --always --pretty=oneline $oldrev + git diff-tree -s --always --encoding=UTF-8 --pretty=oneline $oldrev echo $LOGEND } @@ -571,7 +571,7 @@ generate_delete_atag_email() echowas $oldrev echo echo $LOGBEGIN - git diff-tree -s --always --pretty=oneline $oldrev + git diff-tree -s --always --encoding=UTF-8 --pretty=oneline $oldrev echo $LOGEND } @@ -617,7 +617,7 @@ generate_general_email() echo if [ $newrev_type = commit ]; then echo $LOGBEGIN - git diff-tree -s --always --pretty=medium $newrev + git diff-tree -s --always --encoding=UTF-8 --pretty=medium $newrev echo $LOGEND else # What can we do here? The tag marks an object that is not @@ -636,7 +636,7 @@ generate_delete_general_email() echowas $oldrev echo echo $LOGBEGIN - git diff-tree -s --always --pretty=oneline $oldrev + git diff-tree -s --always --encoding=UTF-8 --pretty=oneline $oldrev echo $LOGEND } -- 1.8.4.rc1 -- To unsubscribe from this list: send the line 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_mkstemps: improve test suite test
Hi, Dale R. Worley wrote: Commit 52749 fixes a bug regarding testing the return of an open() $ git show 52749 fatal: ambiguous argument '52749': unknown revision or path not in the working tree. Could you mention its subject line or date so it's easier to find? call for success/failure. Improve the testsuite test for that fix by removing the helper program 'test-close-fd-0' and replacing it with the shell redirection '-'. (The redirection is Posix, so it should be portable.) Signed-off-by: Dale Worley wor...@ariadne.com [...] Someone has gone ahead and made the code change, so all that remains is to update the testsuite test by replacing the helper program 'test-close-fd-0' with the Posix shell redirection '-'. The above paragraph should be part of the commit message, since otherwise the patch is hard to understand. The patch text looks good. 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: Bug: Pulling remotes into an empty repo deletes the index
Daniel Convissor wrote:
All is not lost. Your local files should be stored in the repository's
reflog. Examine the output of git reflog. You can then reset your
working directory to obtain those files by doing something _like_
git reset --hard HEAD@{1}.
Adam hadn't made a commit, so that wouldn't work in this case.
git fsck --lost-found should be helpful, but yeah, this is a bug.
See for example [1] for the start of a patch to play with (needs
tests).
Thanks and hope that helps,
Jonathan
[1] http://thread.gmane.org/gmane.comp.version-control.git/196502/focus=196503
--
To unsubscribe from this list: send the line 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: change remote to track new branch
Daniel Convissor wrote: On Sat, Aug 03, 2013 at 09:14:59AM +0200, Andreas Schwab wrote: Use git remote set-branches to change the tracked branches of a remote. Use git branch --set-upstream-to to change the upstream of a branch (or create a new branch from the new upstream). Thanks. Those commands were introduced in 1.8. Is there a way to do it in 1.7, please? git remote set-branches was introduced by v1.7.8-rc2~4^2~1. Are you stuck on 1.7.2.5, perhaps? In older (and current) versions of git, you can control the list of branches tracked by a remote by modifying its fetch refspec in .git/config: [remote origin] url = ... fetch = +refs/heads/master:refs/remotes/origin/master fetch = +refs/heads/next:refs/remotes/origin/next git branch --set-upstream-to is from v1.8.0-rc0~50^2~4. In older versions of git, git branch --set-upstream-to=origin/master master was spelled as git branch --set-upstream master origin/master or the branch's upstream can be set directly in .git/config by modifying the remote and merge values in the [branch foo] paragraph. 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] submodule: fix confusing variable name
brian m. carlson wrote: cmd_summary reads the output of git diff, but reads in the submodule path into a variable called name. Since this variable does not contain the name of the submodule, but the path, rename it to be clearer what data it actually holds. Nice. 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 2/2] submodule: don't print status output with ignore=all
brian m. carlson wrote:
git status prints information for submodules, but it should ignore the status
of
those which have submodule.name.ignore set to all. Fix it so that it does
properly ignore those which have that setting either in .git/config or in
.gitmodules.
Signed-off-by: brian m. carlson sand...@crustytoothpaste.net
---
git-submodule.sh | 2 ++
t/t7508-status.sh | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
Thanks. Cc-ing Jens, who wrote that test and knows this code much
better than I do. :)
[...]
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1034,6 +1034,8 @@ cmd_summary() {
sane_egrep '^:([0-7]* )?16' |
while read mod_src mod_dst sha1_src sha1_dst status path
do
+ name=$(module_name $path)
+ test $(get_submodule_config $name ignore none) = all
continue
# Always show modules deleted or type-changed
(blob-module)
test $status = D -o $status = T echo $path
continue
I'm not sure what the exact semantics should be here, though that's
mostly because of my unfamiliarity with submodules in general.
If I have '[submodule favorite] ignore = all' and I then replace
that submodule with a blob, should git submodule status not mention
that path?
If I just renamed a submodule, will 'module_name $path' do the right
thing with the old path?
(rest of the patch kept unsnipped for reference)
# Also show added or modified modules which are checked
out
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index ac3d0fe..fb89fb9 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1316,7 +1316,7 @@ test_expect_success --ignore-submodules=all suppresses
submodule summary '
test_i18ncmp expect output
'
-test_expect_failure '.gitmodules ignore=all suppresses submodule summary' '
+test_expect_success '.gitmodules ignore=all suppresses submodule summary' '
git config --add -f .gitmodules submodule.subname.ignore all
git config --add -f .gitmodules submodule.subname.path sm
git status output
@@ -1324,7 +1324,7 @@ test_expect_failure '.gitmodules ignore=all suppresses
submodule summary' '
git config -f .gitmodules --remove-section submodule.subname
'
-test_expect_failure '.git/config ignore=all suppresses submodule summary' '
+test_expect_success '.git/config ignore=all suppresses submodule summary' '
git config --add -f .gitmodules submodule.subname.ignore none
git config --add -f .gitmodules submodule.subname.path sm
git config --add submodule.subname.ignore all
--
1.8.4.rc1
--
To unsubscribe from this list: send the line 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] hooks/post-receive-email: force log messages in UTF-8
Alexey Shumkin wrote: On Fri, Aug 02, 2013 at 04:23:38PM -0700, Jonathan Nieder wrote: 1. Log messages use the configured log output encoding, which is meant to be whatever encoding works best with local terminals (and does not have much to do with what encoding should be used for email) 2. Filenames are left as is: on Linux, usually UTF-8, and in the Mingw port (which uses Unicode filesystem APIs), always UTF-8 I cannot say exactly if it makes sense for THIS patch, but I'd like to remind about Cygwin port, which definitely does not use UTF-8 encoding (in my case it is Windows-1251) for filenames. 3. The This is an automated email preface uses a project description from .git/description, which is typically in UTF-8 to support gitweb. Thanks for clarifying. So in the context you describe, (1) is configurable, (2) is Windows-1251, (3) is unconfigurably UTF-8, and there is no way with current git facilities to force the email to use a single encoding unless (3) happens to contain no special characters. What is the value of the [i18n] commitEncoding setting in your project? What encoding do the raw commit messages (shown with git log --format=raw) use for their text, and what do they declare with an in-commit 'encoding' header, if any? Does everyone on this project use Cygwin? That should be fine, but I'd expect there to be problems as soon as someone wants to try the Mingw port (Git for Windows). I wonder if there should be an [i18n] repositoryPathEncoding configuration item to support this kind of repository. Then git could be aware of the intended encoding of paths, could recode them for display to a terminal, and at least on Linux and Mingw could recode them for use in filenames on disk. repositoryPathEncoding = none would mean the current behavior of treating paths as raw sequences of bytes. What do you think? 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 should not use a default user.email config value
Hi Thorsten, Thorsten Glaser wrote[1]: git config user.email SHOULD NOT default to $(id -un)@$(hostname -f) because just too many cow-orkers seem to be unable to follow basic instructions Heh. Can you say a little more about your setup? In a university environment with sysadmin-managed email and /etc/mailname set up correctly it is handy that people can start working without doing anything special to configure git's [user] email setting. On the other hand it is obnoxious to receive patches with wrong authorship information. So I'm wondering if there's some detail that distinguishes between these cases. Incidentally, it's been a long time since I looked at the Please configure your email address; I've made something up, but you'll want to check it message: Your name and email address were configured automatically based on your username and hostname. Please check that they are accurate. You can suppress this message by setting them explicitly: git config --global user.name Your Name git config --global user.email y...@example.com After doing this, you may fix the identity used for this commit with: git commit --amend --reset-author I wonder if it's too gentle and long to get the point across. Would something the following (including the guesses in the message for easier copy-pasting) help? No name and email address configured, so I had to guess. You can suppress this message by setting your identity explicitly: git config --global user.name Thorsten Glaser git config --global user.email t...@mirbsd.de After doing so, you may fix the identity used for this commit with git commit --amend --reset-author. It may also make sense to distinguish between cases where a mailname is set and not set. Git already notices the cases where the guessed email address ends with .(none) and errors out, and it could make sense to be more aggressive. Hope that helps, Jonathan [1] http://bugs.debian.org/719226 -- To unsubscribe from this list: send the line 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] .mailmap: mark bouncing email addresses
Hi, From a quick search for homepages: Stefan Beller wrote: [...] +# The 2 following authors are probably the same person, +# but both emails bounce. +Amos Waterland a...@rossby.metr.ou.edu +Amos Waterland a...@us.ibm.com From the history of http://people.seas.harvard.edu/~apw/sreplay/sreplay.git this looks like the same person as a...@debian.org. [...] +# The 2 following authors are probably the same person, +# but both emails bounce. +Daniel Trstenjak daniel.trsten...@online.de +Daniel Trstenjak trs...@science-computing.de daniel.trsten...@gmail.com [...] +# The 2 following authors are probably the same person, +# but both emails bounce. +Jason McMullan jason.mcmul...@timesys.com +Jason McMullan mcmul...@netapp.com A search at http://search.gmane.org/ by name sorted by Newest first finds a candidate email address. +# The 2 following authors are probably the same person, +# but both emails bounce. +Jens Axboe ax...@suse.de +Jens Axboe jens.ax...@oracle.com ax...@kernel.dk +# The 2 following authors are probably the same person, +# but both emails bounce. Nanako Shiraishi nana...@bluebottle.com Nanako Shiraishi nana...@lavabit.com I don't know how to contact Nanako these days. :( Maybe mailmap should learn a way to say there's no current public email address for this person. +# The 2 following authors are probably the same person, +# but both emails bounce. +Rutger Nijlunsing rut...@nospam.com +Rutger Nijlunsing g...@tux.tmfweb.nl Probably rutger.nijluns...@gmail.com. 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: git should not use a default user.email config value
Jeff King wrote: Yeah, there are basically three levels of ident: 1. The user told us explicitly (e.g., $EMAIL, user.email). Trust it. 2. We guessed and it looks reasonable (e.g., hostname is FQDN). Warn but use it. 3. It looks obviously bogus (e.g., we do not have a domain name). Reject it. We can move some cases from (2) down to (3), like when we use gethostname rather than /etc/mailname. But we risk breaking people's existing setups. I don't think we know how many people rely on the implicit hostname selection and would be affected. I don't know if there is a good way to find out short of changing it and seeing who screams. Yes. The result from a reverse DNS lookup is almost never the right mailname. * Small installations tend to use a smarthost. * Large installations tend to use more than one machine, and only one machine's name gets the MX record. So except for cases where someone doesn't actually care about the recorded author and just has a script making commits (such users already suffer from the .(none) heuristic), I don't think this would hurt anyone. We can put a deprecation warning in the release notes, but people tend to ignore those. Not so much a deprecation warning as an Here is one of the more noticeable changes in this release announcement. I'm pretty sure a deprecation warning would not help here. Either people are affected and we say WARNING: You were doing something perfectly reasonable, but now we discourage it, or, more likely, people are not affected. Announcing a change too loudly to users not affected by it has a very bad side effect of training them not to pay much attention to release notes. [...] Another option could to add an option to control the strictness. I suspect a new config item for this is a bad idea, given how simple it is to choose a good default for everyone. 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: git should not use a default user.email config value
Jeff King wrote: Sorry to be unclear. I meant that treating /etc/mailname and gethostname differently might be justified on Debian under the logic if you have /etc/mailname, that is a trustworthy address, and if you do not, then we cannot guess at a trustworthy address (because putting it in /etc/mailname is the accepted way to do so on Debian). But such logic would not extend to other operating systems, where /etc/mailname does not have such a status. I thought that on other operating systems people typically don't have an /etc/mailname. How does trusting the file when present hurt? I am guessing, too, about what people even put in /etc/mailname. If they relay mail from the machine to a smarthost, do they put the individual hostname into /etc/mailname? Or do they put in the domain name that represents a real deliverable address? If the former, then it is no better than gethostname anyway. Debian policy explains: If your package needs to know what hostname to use on (for example) outgoing news and mail messages which are generated locally, you should use the file /etc/mailname. It will contain the portion after the username and @ (at) sign for email addresses of users on the machine (followed by a newline). Such a package should check for the existence of this file when it is being configured. If it exists, it should be used without comment, although an MTA's configuration script may wish to prompt the user even if it finds that this file exists. If the file does not exist, the package should prompt the user for the value (preferably using debconf) and store it in /etc/mailname as well as using it in the package's configuration. The prompt should make it clear that the name will not just be used by that package. So on a properly configured Debian system, /etc/mailname contains something appropriate to put after the @ sign in an email address and the sysadmin expects it to be used for that. As far as I can tell, to the extent that other distros support /etc/mailname, it is only as a side effect of handling that Debian requirement. I don't think e.g. Fedora or Solaris systems typically will have a /etc/mailname file. I *am* a bit worried about what people might put in /etc/mailname on Debian systems when there is no appropriate host to put there (as on Thorsten's machine). 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: Support logic or shell execution to control values in .gitconfig
Matthieu Moy wrote: That would mean executing SOMETEXTTOEXECUTE each time the config file is read. This raises two issues: * A security issue, as SOMETEXTTOEXECUTE could also be something dangerous. It would not be much worse than the current situation (if your config file is not trusted, then an attacker could put malicious code in core.editor for example), but still increase the security risk (as any command reading the config may trigger execution). I don't think the security issue is too bad. As you say, the combination of control over core.pager and pager.config is already pretty dangerous. * A performance issue with the current git implementation, as the config file may be read many time for a single git execution. This issue is harder. Is this a feature others could get behind? I think it's unlikely that this ever be implemented. What I suggest instead is to edit/track/share template configuration files like ~/.gitconfig.in email = me@HOSTNAME@ and then script something like sed -e s/@HOSTNAME@/$(hostname)/ ~/.gitconfig.in ~/.gitconfig. Yeah, substitution scripts like this are probably the simplest way to go. Maybe some day it will make sense for commands to check the timestamp or checksum of foo.in to automatically regenerate foo on the fly when foo.in and other inputs change, but that sounds like more complication than it's worth for git to take on. Other alternatives might be to do that on the filesystem level (a FUSE filesystem or Hurd-style translator generating determining the read(2) result for foo on the fly), the editor level (vi knowing to regenerate foo when you save foo.in), or the session management level (bash via ~/.profile or ~/.bashrc or pam-login regenerating foo at the start of each interactive session). I wish there were an standard way to deal with such tasks instead of running an update script manually, but now I'm getting off topic. 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] submodule: don't print status output with ignore=all
brian m. carlson wrote:
module_name uses whatever's in .gitmodules. I'm not sure what you mean
by renamed a submodule, since git mv foo bar fails with:
vauxhall ok % git mv .vim/bundle/ctrlp .vim/bundle/ctrlq
fatal: source directory is empty, source=.vim/bundle/ctrlp,
destination=.vim/bundle/ctrlq
Can you provide me a set of steps to reproduce that operation so I can
test it effectively?
Ah, I forgot Jens's submodule-mv patch series has not hit master yet.
You can get it by running
git merge origin/pu^{/jl/submodule-mv}^2
before building git.
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: ephemeral-branches instead of detached-head?
David Jeske wrote: Ephemeral branch names would be local-only and would never be pushed. That's how normal branch names behave (local branch names and remote branch names live in different namespaces). How would the proposed detached HEAD replacement compare to them? Would the temporary branch created by checking out a random commit be automatically deleted when checking out another branch, or would it be garbage-collected at some point later? 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: [PATCH] xread(): Fix read error when filtering = 2GB on Mac OS X
Hi,
Steffen Prohaska wrote:
--- a/wrapper.c
+++ b/wrapper.c
@@ -139,6 +139,14 @@ ssize_t xread(int fd, void *buf, size_t len)
{
ssize_t nr;
while (1) {
+#ifdef __APPLE__
+ const size_t twoGB = (1l 31);
+ /* len = 2GB immediately fails on Mac OS X with EINVAL when
+ * reading from pipe. */
+ if (len = twoGB) {
+ len = twoGB - 1;
+ }
+#endif
nr = read(fd, buf, len);
See 6c642a87 (compat: large write(2) fails on Mac OS X/XNU,
2013-05-10) for a cleaner way to do this.
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] tag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times
Stefano Lattarini wrote: Why not encourage the use of a standardized '--action' option instead? Because it's an unpleasant UI. :) This can work with lesser compatibility headaches for both the commands taking mode options and the commands taking mode words: git submodule init becomes git submodule --action=init git tag --delete TAG becomes git tag --action=delete TAGNAME That looks like a bad change in both cases --- it involves more typing without much upside to go along with it. But git submodule init gains synonym git submodule --init git tag --delete TAG stays as git tag --delete TAG looks fine to me. In the long run, we could require that for new commands the 'action' option must come immediately after the git command name if that makes things easier to learn. Thanks for some food for thought. My two cents, 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] xread(): Fix read error when filtering = 2GB on Mac OS X
Kyle J. McKay wrote:
According to POSIX [1] for read:
If the value of nbyte is greater than {SSIZE_MAX}, the result is
implementation-defined.
Sure.
[...]
Since OS X still supports running 32-bit executables, and SSIZE_MAX is 2GB -
1 when running 32-bit it would seem the same limit has been imposed on
64-bit executables. In any case, we should avoid implementation-defined
behavior
Wait --- that's a big leap.
In a 64-bit executable, SSIZE_MAX is 2^63 - 1, so the behavior is not
implementation-defined. I'm not sure if Steffen's copy of git is
32-bit or 64-bit --- my guess would be 64-bit. So at first glance
this does look like an XNU-specific bug, not a standards thing.
What about the related case where someone does try to git add
a file with a clean filter producing more than SSIZE_MAX and less
than SIZE_MAX bytes?
strbuf_grow() does not directly protect against a strbuf growing to
SSIZE_MAX bytes, but in practice on most machines realloc() does. So
in practice we could never read more than SSIZE_MAX characters in the
strbuf_read() codepath, but it might be worth a check for paranoia
anyway.
While we're here, it's easy to wonder: why is git reading into such a
large buffer anyway? Normally git uses the streaming codepath for
files larger than big_file_threshold (typically 512 MiB).
Unfortunately there are cases where it doesn't. For example:
- convert_to_git() has not been taught to stream, so paths
with a clean filter or requiring crlf conversion are read or
mapped into memory.
- deflate_to_pack() relies on seeking backward to retry when
a pack would grow too large, so git hash-object --stdin
cannot use that codepath.
- a clean filter can make a file bigger.
Perhaps git needs to learn to write to a temporary file
when asked to keep track of a blob that is larger than fits
reasonably in memory. Or maybe not.
So there is room for related work but the codepaths that read()
indefinitely large files do seem to be needed, at least in the short
term. Working around this Mac OS X-specific limitation at the read()
level like you've done still sounds like the right thing to do.
Thanks, both, for your work tracking this down. Hopefully the next
version of the patch will be in good shape and then it can be applied
quickly.
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: Should git apply --check imply verbose?
Hi Paul, Paul Gortmaker wrote: OK, so given your feedback, how do you feel about a patch to the documentation that indicates to use -v in combination with the --check to get equivalent patch --dry-run behaviour? Sounds like a good idea to me. I assume you mean a note in the OPTIONS or EXAMPLES section of Documentation/git-apply.txt? 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: [RFC PATCHv4] repack: rewrite the shell script in C.
Stefan Beller wrote: On 08/20/2013 03:31 PM, Johannes Sixt wrote: +packdir = mkpathdup(%s/pack, get_object_directory()); +packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, getpid()); Should this not be packdir = xstrdup(git_path(pack)); packtmp = xstrdup(git_path(pack/.tmp-%d-pack, getpid())); Just a question for documentational purpose. ;) Am I right suggesting the following: `mkpathdup`:: Use parameters to build the path on the filesystem, i.e. create required folders and then return a duplicate of that path. The caller is responsible to free the memory Right. mkpathdup is basically just mkpath composed with xstrdup, except that it avoids stomping on mkpath's buffers. The corresponding almost-shortcut for xstrdup(git_path(s)) is git_pathdup(s). But that's a minor detail. Maybe a new Documentation/technical/api-paths.txt is in order. 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: Dokumenting api-paths.txt
Stefan Beller wrote: On 08/20/2013 03:31 PM, Johannes Sixt wrote: Stefan Beller wrote: +packdir = mkpathdup(%s/pack, get_object_directory()); +packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, getpid()); Should this not be packdir = xstrdup(git_path(pack)); packtmp = xstrdup(git_path(pack/.tmp-%d-pack, getpid())); [...] So if I have packdir = xstrdup(git_path(pack)); ... path = git_path(%s/%s, packdir, filename) This produces something as: .git/.git/objects/pack/.tmp-13199-pack-c59c5758ef159b272f6ab10cb9fadee443966e71.idx definitely having one .git too much. The version with get_object_directory() was right. The object directory is not even necessarily under .git/, since it can be overridden using the GIT_OBJECT_DIRECTORY envvar. Also interesting to add would be that git_path operates in the .git/objects directory? git_path is for resolving paths within GIT_DIR, such as git_path(config) and git_path(COMMIT_EDITMSG). 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: [RFC PATCHv4] repack: rewrite the shell script in C.
Stefan Beller wrote: I think I got all the suggestions except the use of git_path/mkpathdup. I replaced mkpathdup by mkpath where possible, but it's still not perfect. No, mkpathdup is generally better unless you know what you're doing. I'll wait for the dokumentation patch of Jonathan, I never promised to write one. :) I would have preferred to have a rough draft with the results of your investigations so far to start from. Oh well. I'll look into it tonight. 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] teach git-config to output large integers
Jeff King wrote:
I kind of hate the name --ulong. I wanted to call it --size or
something and abstract away the actual platform representation, and just
make it big enough for file sizes.
Yes, something like --size would be more pleasant.
It could still use unsigned long internally. My only worry about
--size is that it does not make it clear we are talking about file
sizes and not in-memory sizes (size_t), and I'm not too worried about
that.
[...]
--- a/builtin/config.c
+++ b/builtin/config.c
[...]
@@ -268,6 +272,10 @@ static char *normalize_value(const char *key, const char
*value)
int v = git_config_int(key, value);
sprintf(normalized, %d, v);
}
+ else if (types == TYPE_ULONG)
+ sprintf(normalized, %lu,
+ git_config_ulong(key, value));
+
else if (types == TYPE_BOOL)
Style: uncuddled else, stray blank line. (The former was already
there, but it still stands out.) I think
if (types == TYPE_INT) {
...
} else if (types == TYPE_ULONG) {
...
} else if (types == TYPE_BOOL) {
...
} else if (types == TYPE_BOOL_OR_INT) {
...
} else {
...
}
would be easiest to read.
Thanks for taking this on.
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 1/2] config: properly range-check integer values
Jeff King wrote:
I added a test. It would not fail on existing 32-bit systems, but would
on existing LP64 systems. It will pass with the new code on both.
However, it will fail on ILP64 systems (because their int is large, and
can represent 3GB). I'm not sure if any such systems are in wide use
(SPARC64?), but we would want a prereq in that case, I guess. I'm
inclined to wait to see if it actually fails for anybody.
Yuck.
What will go wrong if git config --int starts returning numbers too
large to fit in an 'int'? That can already happen if git and a
command that uses it are built for different ABIs (e.g., ILP64 git,
32-bit custom tool that calls git).
It's possible that what the test should be checking for is either
returns a sane answer or fails (which would pass regardless of ABI).
Something like:
test_expect_success 'large integers do not confuse config --int' '
git config giga.crash 3g
test_might_fail git config --int giga.crash actual
echo 3221225472 expect
{
test_cmp expect actual ||
test_must_fail git config --int giga.crash
}
'
Sensible?
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] teach git-config to output large integers
Jeff King wrote: I almost sent it as --size with unsigned long internally. But try writing the documentation for it. You want to say something like it's big enough to handle file sizes. Except that on 32-bit, it's _not_. It's only 4G. You really want something that uses off_t internally, so 32-bit systems with largefile support do the sane thing. But now you have no way of emulating the way that git parses stuff internally. Let's take a step back for a moment. What problem is this patch solving? From the motivating example, I thought it was When reading or writing an integer config item, git sometimes encounters integer overflow and doesn't know how to deal with it. Worse, this means that some meaningful values are unrepresentable in config files. Fix it in two steps: 1. Catch overflow, and error out instead of pretending to be able to handle it. 2. Provide at least an option to use a wider integer type and handle larger meaningful values. This involves a new option --size instead of making --int use intmax_t for the following compatibility reason: ... For example, the compatibility reason could be that some scripts calling git config were not able to handle large integers and that we do not want to expose them to unexpectedly large values. But that reason doesn't sound realistic to me. So what is the actual reason not to always use a wider range? That is what I was trying to get at in discussing the test. It is not We would like --int to reject values higher than this, but some platforms do not allow us to, but Either rejecting this value, or even better, computing the right size and printing it, is an acceptable behavior, and this test checks for those. 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 2/2] teach git-config to output large integers
Jeff King wrote: On Tue, Aug 20, 2013 at 09:38:41PM -0700, Jonathan Nieder wrote: That is what I was trying to get at in discussing the test. It is not We would like --int to reject values higher than this, but some platforms do not allow us to, but Either rejecting this value, or even better, computing the right size and printing it, is an acceptable behavior, and this test checks for those. You are conflating the two patches, I think. The test we were discussing is for the _first_ patch, which fixes a bug in the range check. It is not meant to test git-config in particular, but to test that values higher than INT_MAX and lower than LONG_MAX are properly range-checked. Forget the second patch for a moment. I believe the first one is a bug fix that we would want even if we do not take the second patch at all. Sure. I'm not conflating the patches. What I mean is that tests are supposed to test desirable behavior, whatever that is --- they are not about preventing all behavior changes but only about preventing regressions. So talking about tests is a (perhaps overly roundabout) way to figure out the desirable behavior. In particular, at first glance I would think computing 3 * 2^20 instead of erroring out would be a *good* behavior, not a regression. If that's right, it doesn't make sense to me to go to careful lengths either to test that git continues to error out on most platforms, or to introduce new options to ensure git config --int continues to error out. That is what I am trying to understand. Everything about the first patch except for the test makes sense to me, but the test doesn't. As you noted, we know the test won't pass on some platforms. Why is it something we should *want* to pass? 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] repack: rewrite the shell script in C.
Hi,
Stefan Beller wrote:
[PATCH] repack: rewrite the shell script in C.
Thanks for your work so far. This review will have mostly cosmetic
notes. Hopefully others can try it out to see if the actual behavior
is good.
As a first nit: in git, as usual in emails, the style in subject lines
is not to end with a period. The above subject line is otherwise good
(a nice summary that quickly explains the effect, which is handy in
e.g. abbreviated changelogs from release announcements).
This is the beginning of the rewrite of the repacking.
This is a place to explain
- the motivation / intended positive effect of the patch
- any noticeable behavior changes
- complications and other hints for people looking back and trying
to understand this code
Based on the discussion before, I think the motivation is to get
closer to a goal of being able to have a core subset of git
functionality built in to git. That would mean
* people on Windows could get a copy of at least the core parts
of Git without having to install a Unix-style shell
* people deploying to servers don't have to rewrite the #! line
or worry about the PATH and quality of installed POSIX
utilities, if they are only using the built-in part written
in C
This patch is meant to be mostly a literal translation of the
git-repack script; the intent is that later patches would start using
more library facilities, but this patch is meant to be as close to a
no-op as possible so it doesn't do that kind of thing.
All tests are constantly positive now.
This kind of changes-since-the-previous-iteration information that
doesn't need to be recorded in the commit log for posterity goes
after the --- marker.
Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
Makefile| 2 +-
[...]
--- /dev/null
+++ b/builtin/repack.c
@@ -0,0 +1,361 @@
[...]
+static int delta_base_offset = 0;
The = 0 is automatic for statics without an initializer. The
prevailing style in git is to leave it out.
Behavior change: in the script, wasn't the default true?
[...]
+static void remove_temporary_files() {
Style: argument list should have void. (In C89 and C99, an empty
argument list means having unspecified arguments instead of having
no arguments as in C++.)
+ DIR *dir;
+ struct dirent *e;
+ char *prefix, *path;
+
+ prefix = mkpathdup(.tmp-%d-pack, getpid());
pid_t is not guaranteed to be an int, so this needs a cast.
+ path = mkpathdup(%s/pack, get_object_directory());
The names prefix and path are quite generic. What does this
function do? A comment could help, e.g.:
/*
* Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.
*/
+
+ dir = opendir(path);
+ while ((e = readdir(dir)) != NULL) {
What happens if the directory does not exist?
+ if (!prefixcmp(e-d_name, prefix)) {
The git-repack script removes $PACKTMP-*, but this code matches $PACKTMP*
instead. Intentional?
+ struct strbuf fname = STRBUF_INIT;
+ strbuf_addf(fname, %s/%s, path, e-d_name);
+ unlink(strbuf_detach(fname, NULL));
This leaks fname (see Documentation/technical/api-strbuf.txt for an
explanation of strbuf_detach).
+ }
+ }
+ free(prefix);
+ free(path);
+ closedir(dir);
I wonder if it would make sense for buffers to share space here.
E.g. something like
{
/*
* Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.
*/
struct strbuf buf = STRBUF_INIT;
size_t dirlen, prefixlen;
DIR *dir;
struct dirent *e;
/* .git/objects/pack */
strbuf_addstr(buf, get_object_directory());
strbuf_addstr(buf, /pack);
dir = opendir(buf.buf);
if (!dir)
... handle error ...
/* .git/objects/pack/.tmp-$$-pack-* */
dirlen = buf.len + 1;
strbuf_addf(buf, /.tmp-%d-pack-, getpid());
prefixlen = buf.len - dirlen;
while ((e = readdir(dir))) {
if (strncmp(e-d_name, buf.buf + dirlen, prefixlen))
continue;
strbuf_setlen(buf, dirlen);
strbuf_addstr(buf, e-d_name);
unlink(buf.buf);
}
if (closedir(dir))
... handle error ...
strbuf_release(buf);
}
I dunno.
[...]
+/*
+ * Fills the filename list with all the files found in the pack directory
+ * ending with .pack, without that extension.
+ */
Ideally a comment opening a function will save lazy readers the
trouble of reading the body of the function, by explaining what the
function is for and giving them some reliable summary of what its
effect will be.
The above comment doesn't do either: it doesn't make it clear why
the function exists, and it doesn't make the semantics precise:
should fname_list be empty before
Re: [RFC PATCHv6 1/2] repack: rewrite the shell script in C
Junio C Hamano wrote: Stefan Beller stefanbel...@googlemail.com writes: The motivation of this patch is to get closer to a goal of being able to have a core subset of git functionality built in to git. That would mean * people on Windows could get a copy of at least the core parts of Git without having to install a Unix-style shell * people deploying to servers don't have to rewrite the #! line or worry about the PATH and quality of installed POSIX utilities, if they are only using the built-in part written in C I am not sure what is meant by the latter. Rewriting #! is part of any scripted Porcelain done by the top-level Makefile, and I do not think we have seen any problem reports on it. As to quality of ... utilities, I think the real issue some people in the thread had was not about deploying to servers but about installing in a minimalistic chrooted environment where standard tools may be lacking. Thanks for a sanity check. Yeah, the second item should be about minimal chroots, not my sloppy guess about some hypothetical bad operating system with untrustworthy tools. 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/3] Fixes for tests run without perl
Hi, Kacper Kornet wrote: This is a set of fixes for problems found while running test suite without perl installed. I don't think git ever supported that. The PERL prerequisite was to check for systems that did not have (a suitable) perl at runtime, but perl is still pretty heavily used in tests. I assume you do have perl installed and that these fixes are from testing the NO_PERL=YesPlease case, which is valuable. Thanks for working on this. Now on to the patches. 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/3] Make test using invalid commit with -C more strict
Kacper Kornet wrote: In the test 'using invalid commit with -C' git-commit would have failed even if the -C option had been given the correct commit, as there was nothing to commit. Good catch. [...] --- a/t/t7501-commit.sh +++ b/t/t7501-commit.sh @@ -53,7 +53,10 @@ test_expect_success PERL 'can use paths with --interactive' ' ' test_expect_success 'using invalid commit with -C' ' - test_must_fail git commit -C bogus + echo bong file + git add file + test_must_fail git commit -C bogus + git reset I guess to be pedantic this should say echo bong file git add file test_when_finished git reset --hard test_must_fail git commit -C bogus to avoid interfering with later tests even when this one fails and the prevents the 'git reset' from being executed. 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: [PATCH 2/3] t/t3701-add-interactive.sh: Add PERL prerequisite
Kacper Kornet wrote: --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -330,7 +330,7 @@ test_expect_success PERL 'split hunk add -p (edit)' ' ! grep ^+15 actual ' -test_expect_success 'patch mode ignores unmerged entries' ' +test_expect_success PERL 'patch mode ignores unmerged entries' ' Mph. This is a symptom of f0459319 (change from skip_all=* to prereq skip, 2010-08-13), which hurt maintainability without much upside to balance it. I wonder if it would be easier to do something like the following instead. -- 8 -- Subject: add -i test: use skip_all instead of repeated PERL prerequisite It is too easy to forget to add the PERL prerequisite for new add -i tests, especially given that many people do not test with NO_PERL so the missing prereq is not always noticed quickly. The test had used the skip_all mechanism since 1b19ccd2 (2009-04-03) but switched to explicit PERL prereqs in f0459319 (2010-10-13) in hope of helping people see how many tests were skipped, perhaps to motivate them to tweak their platform or tests to improve test coverage. That didn't pan out much in practice, so let's move back to the simpler skip_all method. Reported-by: Kacper Kornet drae...@pld-linux.org Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- t/t3701-add-interactive.sh | 76 +- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 9fab25cc..9dc91d09 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -4,18 +4,24 @@ test_description='add -i basic tests' . ./test-lib.sh . $TEST_DIRECTORY/lib-prereq-FILEMODE.sh -test_expect_success PERL 'setup (initial)' ' +if ! test_have_prereq PERL +then + skip_all='skipping add -i tests, perl not available' + test_done +fi + +test_expect_success 'setup (initial)' ' echo content file git add file echo more file echo lines file ' -test_expect_success PERL 'status works (initial)' ' +test_expect_success 'status works (initial)' ' git add -i /dev/null output grep +1/-0 *+2/-0 file output ' -test_expect_success PERL 'setup expected' ' +test_expect_success 'setup expected' ' cat expected EOF new file mode 100644 index 000..d95f3ad @@ -26,19 +32,19 @@ index 000..d95f3ad EOF ' -test_expect_success PERL 'diff works (initial)' ' +test_expect_success 'diff works (initial)' ' (echo d; echo 1) | git add -i output sed -ne /new file/,/content/p output diff test_cmp expected diff ' -test_expect_success PERL 'revert works (initial)' ' +test_expect_success 'revert works (initial)' ' git add file (echo r; echo 1) | git add -i git ls-files output ! grep . output ' -test_expect_success PERL 'setup (commit)' ' +test_expect_success 'setup (commit)' ' echo baseline file git add file git commit -m commit @@ -47,12 +53,12 @@ test_expect_success PERL 'setup (commit)' ' echo more file echo lines file ' -test_expect_success PERL 'status works (commit)' ' +test_expect_success 'status works (commit)' ' git add -i /dev/null output grep +1/-0 *+2/-0 file output ' -test_expect_success PERL 'setup expected' ' +test_expect_success 'setup expected' ' cat expected EOF index 180b47c..b6f2c08 100644 --- a/file @@ -63,12 +69,12 @@ index 180b47c..b6f2c08 100644 EOF ' -test_expect_success PERL 'diff works (commit)' ' +test_expect_success 'diff works (commit)' ' (echo d; echo 1) | git add -i output sed -ne /^index/,/content/p output diff test_cmp expected diff ' -test_expect_success PERL 'revert works (commit)' ' +test_expect_success 'revert works (commit)' ' git add file (echo r; echo 1) | git add -i git add -i /dev/null output @@ -76,24 +82,24 @@ test_expect_success PERL 'revert works (commit)' ' ' -test_expect_success PERL 'setup expected' ' +test_expect_success 'setup expected' ' cat expected EOF EOF ' -test_expect_success PERL 'setup fake editor' ' +test_expect_success 'setup fake editor' ' fake_editor.sh chmod a+x fake_editor.sh test_set_editor $(pwd)/fake_editor.sh ' -test_expect_success PERL 'dummy edit works' ' +test_expect_success 'dummy edit works' ' (echo e; echo a) | git add -p git diff diff test_cmp expected diff ' -test_expect_success PERL 'setup patch' ' +test_expect_success 'setup patch' ' cat patch EOF @@ -1,1 +1,4 @@ this @@ -103,7 +109,7 @@ cat patch EOF EOF ' -test_expect_success PERL 'setup fake editor' ' +test_expect_success 'setup fake editor' ' echo #!$SHELL_PATH fake_editor.sh cat fake_editor.sh \EOF mv -f $1 oldpatch @@ -113,26 +119,26 @@ EOF test_set_editor $(pwd)/fake_editor.sh ' -test_expect_success PERL 'bad edit rejected
Re: [PATCH 3/3] t/t7106-reset-unborn-branch.sh: Add PERL prerequisite
Kacper Kornet wrote: Signed-off-by: Kacper Kornet drae...@pld-linux.org Thanks. Reviewed-by: Jonathan Nieder jrnie...@gmail.com Here's a style cleanup on top. -- 8 -- Subject: reset test: modernize style Avoid command substitution and pipes to ensure that the exit status from each git command is tested (and in particular that any segfaults are caught). Maintain the test setup (no commits, one file named a, another named b) even after the last test, to make it easier to rearrange tests or add new tests after the last in the future. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- t/t7106-reset-unborn-branch.sh | 31 --- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh index 499cd88c..af00ab4d 100755 --- a/t/t7106-reset-unborn-branch.sh +++ b/t/t7106-reset-unborn-branch.sh @@ -11,7 +11,10 @@ test_expect_success 'setup' ' test_expect_success 'reset' ' git add a b git reset - test $(git ls-files) = + + expect + git ls-files actual + test_cmp expect actual ' test_expect_success 'reset HEAD' ' @@ -24,28 +27,42 @@ test_expect_success 'reset $file' ' rm .git/index git add a b git reset a - test $(git ls-files) = b + + echo b expect + git ls-files actual + test_cmp expect actual ' test_expect_success PERL 'reset -p' ' rm .git/index git add a - echo y | git reset -p - test $(git ls-files) = + echo y yes + git reset -p yes + + expect + git ls-files actual + test_cmp expect actual ' test_expect_success 'reset --soft is a no-op' ' rm .git/index git add a - git reset --soft - test $(git ls-files) = a + git reset --soft + + echo a expect + git ls-files actual + test_cmp expect actual ' test_expect_success 'reset --hard' ' rm .git/index git add a + test_when_finished echo a a git reset --hard - test $(git ls-files) = + + expect + git ls-files actual + test_cmp expect actual test_path_is_missing a ' -- 1.8.4.rc4 -- To unsubscribe from this list: send the line 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 01/13] Call it Git User Manual and remove reference to very old Git version
Thomas Ackermann wrote: --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -1,6 +1,5 @@ -Git User's Manual (for version 1.5.3 or newer) +Git User Manual User versus User's: why? But I don't care much about this. Dropping the reference to 1.5.3 is very welcome, since it should help people to update the manual without fearing that new sections will fail for people with ancient copies of git. 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 02/13] Use current detached HEAD message
Thomas Ackermann wrote:
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -312,10 +312,17 @@ referenced by a tag:
$ git checkout v2.6.17
- Note: moving to v2.6.17 which isn't a local branch
- If you want to create a new branch from this checkout, you may do so
- (now or later) by using -b with the checkout command again. Example:
- git checkout -b new_branch_name
+ Note: checking out 'v2.6.17'.
+
+ You are in 'detached HEAD' state. You can look around, make experimental
+ changes and commit them, and you can discard any commits you make in
this
+ state without impacting any branches by performing another checkout.
+
+ If you want to create a new branch to retain commits you create, you may
+ do so (now or later) by using -b with the checkout command again.
Example:
+
+ git checkout -b new_branch_name
+
HEAD is now at 427abfa... Linux v2.6.17
I wonder if this longer wall of text (added in 13be3e31, 2010-01-29)
is too aggressive.
It is the only piece of advice that I explicitly disable in
~/.gitconfig, so I haven't looked at it again for a while. Since
then, the usual stream of questions about how to recover from people
who accidentally detached HEAD has still been showing up in #git, so I
don't think the message succeeded in its purpose.
That might be partly because it is too long to digest at a glance.
When I see this message, what I actually take in is
$ git checkout v1.7.3
Hmm, capital ---Note: checking out 'v1.7.3'.
heading before
lowercaseYou are in 'detached HEAD' state. You ...
sentence.
... checkout.
If you want ...
do so (now or later) by using -b Example:
git ...
HEAD is ...
Phew, I can $
type commands
again.
Whereas I think the message is just meant to convey the following:
$ git checkout v2.6.17
note: checking out a tag for inspection and discardable experiments on top
To create a new branch to save your changes:
git checkout -b my-branch-based-on-v2.6.17
HEAD is now at 427abfa... Linux v2.6.17
$
@@ -326,7 +333,7 @@ and git branch shows that you are no longer on a branch:
$ cat .git/HEAD
427abfa28afedffadfca9dd8b067eb6d36bac53f
$ git branch
-* (no branch)
+* (detached from v2.6.17)
grep no branch Documentation/user-manual.txt
finds two other instances of that message, which this branch doesn't
touch. One is about a bisection, where (no branch) is pretty close
to the actual message ('(no branch, bisect started on master)').
The other is about submodules. Here's a patch for potential squashing
in that corrects it.
Thanks,
Jonathan
diff --git i/Documentation/user-manual.txt w/Documentation/user-manual.txt
index 3e226190..b76219ee 100644
--- i/Documentation/user-manual.txt
+++ w/Documentation/user-manual.txt
@@ -3647,7 +3647,7 @@ working on a branch.
-
$ git branch
-* (no branch)
+* (detached from d266b98)
master
-
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/13] Use current output for git repack
Thomas Ackermann wrote: --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -3203,14 +3203,11 @@ To put the loose objects into a pack, just run git repack: $ git repack [...] -Total 6020, written 6020 (delta 4070), reused 0 (delta 0) -Pack pack-3e54ad29d5b2e05838c75df582c65257b8d08e1c created. +Total 6020 (delta 4070), reused 0 (delta 0) Sure. I wonder if there should be some text to replace the output that mentions the pack being created, though. E.g.: You can then run Total 6020 (delta 4070), reused 0 (delta 0) This creates a single pack file in .git/objects/pack/ containing all currently unpacked objects. You can then run -- To unsubscribe from this list: send the line 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 04/13] Use git merge instead of git pull .
Thomas Ackermann wrote: --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -1784,17 +1784,6 @@ repository that you pulled from. fast-forwards,fast-forward; instead, your branch will just be updated to point to the latest commit from the upstream branch.) -The `git pull` command can also be given `.` as the remote repository, -in which case it just merges in a branch from the current repository; so -the commands - -- -$ git pull . branch -$ git merge branch -- - -are roughly equivalent. The former is actually very commonly used. - I wonder if it would make sense to say they simply *are* equivalent. I.e., what differences are there between those two commands, and could git pull be tweaked to eliminate them? I agree that the historical The former is actually very commonly used ought to go. It wouldn't too relevant for someone learning to use git even if it were still true. ;-) [...] @@ -2259,7 +2248,7 @@ When you are happy with the state of this change, you can pull it into the test branch in preparation to make it public: - -$ git checkout test git pull . speed-up-spinlocks +$ git checkout test git merge speed-up-spinlocks - Yes. 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: [RFC/PATCH] Fix path prefixing in grep_object
Junio C Hamano wrote: Phil Hord phil.h...@gmail.com writes: On Sat, Aug 24, 2013 at 9:35 PM, Phil Hord ho...@cisco.com wrote: When the pathspec given to grep includes a tree name, the full name of matched files is assembled using colon as a separator. If the pathspec includes a tree name, it should use a slash instead. [...] If the tree name includes an object name, as in HEAD:some/path, it should use a slash instead. What problem are you trying to solve? It should use HEAD:some/path, not HEAD/some/path. I think Phil meant that when git grep is asked to search within HEAD:some/path, filenames tacked on at the end should be appended with a '/' separator instead of the usual ':' (e.g., HEAD:some/path/inner/path.c, not HEAD:some/path:inner/path.c). Otherwise I cannot copy and paste git grep output and get something suitable for passing to git show. I don't think we have a standard name for the tree:path syntax. I've always just called it tree:path syntax. :) 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 05/13] Fix some typos
Thomas Ackermann wrote: --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -219,7 +219,7 @@ of development leading to that point. The best way to see how this works is using the linkgit:gitk[1] command; running gitk now on a Git repository and looking for merge -commits will help understand how the Git organizes history. +commits will help understand how Git organizes history. Heh. Sure. [...] @@ -793,7 +793,7 @@ e05db0fd4f31dde7005f075a84f96b360d05984b - Or you could recall that the `...` operator selects all commits -contained reachable from either one reference or the other but not +reachable from either one reference or the other but not both; so Yes. Here one of the references is the nickname of a remote and not a branch, so reachable from reads better than contained in would. @@ -820,7 +820,7 @@ You could just visually inspect the commits since e05db0fd: $ gitk e05db0fd.. - -Or you can use linkgit:git-name-rev[1], which will give the commit a +or you can use linkgit:git-name-rev[1], which will give the commit a I think this reads better with a capital 'O'. (The pedant in me likes it, too, since a colon ends a sentence.) The lowercase 'but' later in this section should perhaps also be capitalized, since it also starts an independent thought. But that may sometimes help you guess which tags come after the given commit. The sentence So, you can run something like ... then search for a line that looks like ... is a sequence of incomplete thoughts. It could be paraphrased a little to scan better: So, if you run something like git show-branch e05db0fd v1.5.0-rc0 v1.5.0-rc1 v1.5.0-rc2 $ git show-branch e05db0fd v1.5.0-rc0 v1.5.0-rc1 v1.5.0-rc2 ! [e05db... then a line like + ++ [e05db0fd] Fix warnings in ... shows that e05db0fd is reachable from itself, from v1.5.0-rc1, and from v1.5.0-rc2, and not from v1.5.0-rc0. [...] @@ -3525,7 +3525,7 @@ with Git 1.5.2 can look up the submodule commits in the repository and manually check them out; earlier versions won't recognize the submodules at all. -To see how submodule support works, create (for example) four example +To see how submodule support works, create four example I'd keep the joke. [...] @@ -3897,7 +3897,7 @@ fact that such a commit brings together (merges) two or more previous states represented by other commits. In other words, while a tree represents a particular directory state -of a working directory, a commit represents that state in time, +of a working directory, a commit represents that state in time, and explains how we got there. It's not really about time but about (hypothetical, possibly branched) history, but I think your change makes it about as clear as it can be. 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 06/13] Simplify How to make a commit
Thomas Ackermann wrote: --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -1080,19 +1080,14 @@ produce no output at that point. Modifying the index is easy: -To update the index with the new contents of a modified file, use +To add the contents of a new file to the index or update the index +with the new contents of a modified file, use That's a mouthful. I'd say To update the index with the contents of a new or modified file, use [...] -To add the contents of a new file to the index, use - -- -$ git add path/to/file -- - \o/ -To remove a file from the index and from the working tree, +To remove a file from the index and from the working tree, use - $ git rm path/to/file In git 2.0, (plain rm followed by) git add should work for this, too. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/13] Improve description in How to merge
Thomas Ackermann wrote: Describe the conflict resolution in terms of the commands the user is supposed to use. [...] --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -1251,10 +1251,8 @@ Automatic merge failed; fix conflicts and then commit the result. - Conflict markers are left in the problematic files, and after -you resolve the conflicts manually, you can update the index -with the contents and run Git commit, as you normally would when -creating a new file. Hm. It's been too long since I was a novice, since I find the above clear already. To make the text clearer, I think it would be best to *add* an example instead of replacing it by one. 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 09/13] Improve section Merge multiple trees
Thomas Ackermann wrote: --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -3992,16 +3992,16 @@ Merging multiple trees Git helps you do a three-way merge, which you can expand to n-way by repeating the merge procedure arbitrary times until you finally -commit the state. +commit the state. The above sentence is unclear to me both before and after this change. Git helps me do a three-way merge, but I'm on my own if I want to expand to n-way? Those times I repeat it are arbitrary times, not arbitrarily many times? Using git merge I make commits, but I do not finally commit to the result until the (n-1)st? And what is this state I am committing? Maybe the intent is Git can help you perform a three-way merge, which can in turn be used for a many-way merge by repeating the merge procedure several times. The usual situation is that you only do one three-way merge (reconciling two lines of history) and commit the result, but if you like to, you can merge several branches in one go. To perform a three-way merge, you start with the two commits you want to merge, find their closest common parent (a third commit), and compare the trees corresponding to these three commits. To get the base for the merge, look up the common parent of two commits: $ git merge-base commit1 commit2 This prints the name of a commit they are both based on. ... [...] -To get the base for the merge, you first look up the common parent +To get the base for the merge, you first look up the common parent Merge base hasn't been defined, so this is using quotes to point out that it is defining a new, unfamiliar term. [...] -now look up the tree objects of those commits, which you can easily -do with (for example) +now look up the tree objects of those commits, which you can easily +do with Yes. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/13] Remove obscure reference from Examples
Thomas Ackermann wrote: --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -2131,8 +2131,6 @@ He uses two public branches: - A test tree into which patches are initially placed so that they can get some exposure when integrated with other ongoing development. - This tree is available to Andrew for pulling into -mm whenever he - wants. This drops useful information (namely, that Tony was publishing history for two people to consume). Perhaps it should spell out the bleeding-edge -mm tree? 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: [RFC/PATCH] Fix path prefixing in grep_object
Jeff King wrote:
So we are necessarily reconstructing based on what we know of the
syntax. And I think that your rule is OK, because we know that refnames
cannot contain a colon.
What happens with expressions like HEAD^{/test:}?
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/3] t3404: preserve test_tick state across short SHA-1 collision test
Hi, Eric Sunshine wrote: The short SHA-1 collision test requires carefully crafted commits in order to ensure a collision at rebase time. Yeah, this breaks the usual rule that tests should be independent of hashing function. But it's the best we can do, I think. [...] --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -994,17 +994,23 @@ test_expect_success 'short SHA-1 setup' ' test_when_finished git checkout master git checkout --orphan collide git rm -rf . + ( unset test_tick test_commit collide1 collide test_commit --notick collide2 collide test_commit --notick collide3 115158b5 collide collide3 collide3 + ) Would be clearer if the code in a subshell were indented: ( unset test_tick test_commit ... ) [...] test_expect_success 'short SHA-1 collide' ' test_when_finished reset_rebase git checkout master git checkout collide + ( + unset test_tick + test_tick FAKE_COMMIT_MESSAGE=collide2 815200e \ FAKE_LINES=reword 1 2 git rebase -i HEAD~2 + ) Likewise. 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] Documentation/fast-import: clarify summary for `feature` command
Matthieu Moy wrote: In most cases, feature foo does not just require that the feature exists, but also changes the behavior to enable it. 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 2/2] dir: test_one_path: fix inconsistent behavior due to missing '/'
Eric Sunshine wrote: Although undocumented, directory_exists_in_index_icase(dirname,len) unconditionally assumes the presence of a '/' at dirname[len] (despite being past the end-of-string). Callers are expected to respect [...] Fix this problem. So, does this fix the problem by changing directory_exists_in_index_icase() to be more liberal in what it accepts, or callers to be more conservative in what they pass in? Please forgive my laziness. I ask in order to save future readers the time of digging into the code. 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] write_index: optionally allow broken null sha1s
Hi,
Jeff King wrote:
---
I was tempted to not involve filter-branch in this commit at all, and
instead require the user to manually invoke
GIT_ALLOW_NULL_SHA1=1 git filter-branch ...
to perform such a filter. That would be slightly safer, but requires
some specialized knowledge from the user (and advice on using
filter-branch to remove such entries already exists on places like
stackoverflow, and this patch makes it Just Work on recent versions of
git).
The above few paragraphs explained the most mysterious part of the
patch to me. I think they would be fine to include in the commit
message.
[...]
--- a/read-cache.c
+++ b/read-cache.c
@@ -1817,8 +1817,17 @@ 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 (is_null_sha1(ce-sha1)) {
+ static const char msg[] = cache entry has null sha1:
%s;
+ static int allow = -1;
+
+ if (allow 0)
+ allow = git_env_bool(GIT_ALLOW_NULL_SHA1, 0);
+ if (allow)
+ warning(msg, ce-name);
+ else
+ return error(msg, ce-name);
+ }
Makes sense.
[...]
--- /dev/null
+++ b/t/t7009-filter-branch-null-sha1.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+
+test_description='filter-branch removal of trees with null sha1'
+. ./test-lib.sh
+
+test_expect_success 'create base commits' '
+ test_commit one
+ test_commit two
+ test_commit three
+'
+
+test_expect_success 'create a commit with a bogus null sha1 in the tree' '
+ test_tick
+ tree=$(
+ {
+ git ls-tree HEAD
+ printf 16 commit $_z40\\tbroken
+ } | git mktree
+ )
To avoid pipes involving git commands, since they can losing the exit
status (and hence information about whether git crashed):
{
git ls-tree HEAD
echo 16 commit $_z40 broken
} listing
echo add broken entry msg
tree=$(git mktree listing)
test_tick
commit=$(git commit-tree $tree -p HEAD msg)
git update-ref HEAD $commit
The rest 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 v2 2/3] t3404: rebase -i: demonstrate short SHA-1 collision
Eric Sunshine wrote: --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1037,4 +1037,28 @@ test_expect_success 'rebase -i with --strategy and -X' ' test $(cat file1) = Z ' +test_expect_success 'short SHA-1 setup' ' + test_when_finished git checkout master + git checkout --orphan collide + git rm -rf . + ( + unset test_tick + test_commit collide1 collide + test_commit --notick collide2 collide + test_commit --notick collide3 collide + ) Style: would be clearer indented: ( unset test_tick test_commit ... ) +test_expect_failure 'short SHA-1 collide' ' + test_when_finished reset_rebase git checkout master + git checkout collide + ( + unset test_tick Likewise. 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
