Re: [PATCH] The images from picon and gravatar are always used over http://, and browsers give mixed contents warning when gitweb is served over https://.
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Odd. https://www.gravatar.com/; also seems to work. I've put in a technical support query to find out what the Gravatar admins prefer. Thanks; will hold onto Andrej's patch until we hear what the story is. Good news: a kind person from Automattic answered that www.gravatar.com should work fine over SSL, both now and in the future, and promised to add updating documentation to their todo list. Thanks for your help and patience. 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 3/3] gpg: Allow translation of more error messages
Stephen Boyd wrote: Mark these strings for translation so that error messages are printed in the user's language of choice. Yeah. Other error messages in this file are already translated, so it's oddly inconsistent. Assuming this passes tests with GETTEXT_POISON=YesPlease, 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/7] Undocument deprecated alias 'push.default=tracking'
Hi, Junio C Hamano wrote: Wow, that's a blast from the past. I tend to agree that deprecating and removing are quite different, but a simple revert of the change would not be good, either. We still would want to _discourage_ its use. Hm, I was about to try adding a line in that vein, like * `tracking` - deprecated synonym for `upstream`. Imagine my surprise when I saw that that is what you just said would be no good: [...] `git pull`. -* `tracking` - deprecated synonym for `upstream`. * `current` - push the current branch to a branch of the same name. I really do think that including `tracking` in the same list would be valuable. When I look over a friend's .gitconfig file to help track down a problem she is running into, it is helpful if I can find the meaning of each item in a straightforward way. Is the problem that deprecated is not precise enough? For example, would it make sense to say deprecated synonym for `upstream`. Will be dropped in git 2.1 or something like that? 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 2/7] Undocument deprecated alias 'push.default=tracking'
Jonathan Nieder wrote: Is the problem that deprecated is not precise enough? For example, would it make sense to say deprecated synonym for `upstream`. Will be dropped in git 2.1 or something like that? Also, if we plan to remove support soon, we should start warning when this setting is encountered so people know to update their configuration. 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: Why git-whatchanged shows a commit touching every file, but git-log doesn't?
Hi Constantine, Constantine A. Murenin wrote: DragonFly BSD uses git as its SCM, with one single repository and branch for both the kernel and the whole userland. On 2011-11-26 (1322296064), someone did a commit that somehow touched every single file in the repository, even though most of the files were not modified one bit. gitk --simplify-by-decoration might provide some insight. In the dragonfly history, it seems that imports of a packages typically proceed in two steps: 1. First, the upstream code is imported as a new initial commit with no history: cd ~/src git init gcc-4.7.2-import cd gcc-4.7.2-import tar -xf /path/to/gcc-4.7.2 mkdir contrib mv gcc-4.7.2 contrib/gcc-4.7 git add . git commit -m 'Import gcc-4.7.2 to new vendor branch' 2. Next, that code is incorporated into dragonfly. cd ~/src/dragonfly git fetch ../gcc-4.7.2-import master:refs/heads/vendor/GCC47 git merge vendor/GCC47 rm -fr ../gcc-4.7.2-import Unfortunately in the commit you mentioned, someone made a mistake. Instead of importing a single new upstream package, the author imported the entire dragonfly tree as a new vendor branch. Oops. The effects might be counterintuitive: * tools like git blame and path-limited git log get a choice: when looking at the merge that pulled in a copy of dragonfly into the existing dragonfly codebase, either parent is an equally sensible from blame's point of view as an explanation of the origin of this code. I think both prefer the first parent here, making them happen to produce the right result. * tools like git show that describe what change a commit made get a choice: when looking at a parentless commit, the diff that brings a project into existence may or may not be interesting, depending on the situation. See http://thread.gmane.org/gmane.comp.version-control.git/182571/focus=182577 for more about that. But at its heart, this is just an instance of lie when creating your history and history-mining tools will lie back to you. :) Hoping that clarifies a little, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Undocument deprecated alias 'push.default=tracking'
Junio C Hamano wrote: That is why I tend to prefer how check-ref-format documentation describes --print: --normalize:: Normalize 'refname' by removing any leading slash (`/`) characters and collapsing runs of adjacent slashes between name components into a single slash. Iff the normalized refname is valid then print it to standard output and exit with a status of 0. (`--print` is a deprecated way to spell `--normalize`.) That works because, as you mention, the usual way to look up an option in manpages is to search for --print, including the two minus signs. Unfortunately an analagous approach in gitconfig(5) would be seriously broken, because searching for tracking (no minus signs) is going to hit many false positives. I do not think such a change would be an improvement. Meanwhile I believe the prominent words deprecated synonym already make it completely obvious that when I write a new config file, I should use the modern option, unless I am trying to write a config file that also works with older versions of git. In the latter case (which unfortunately is not too uncommon), hiding the option is not going to make my life easier. What would allow me to make an informed choice is mentioning what version of git *introduced* the new name of the option: - `tracking` - deprecated old name for `upstream`, used by git versions before 1.7.4.2. Don't use this. Also I do not think anyone claimed we are removing tracking from the documentation in order to stop people from using it. The rationale when the patch was proposed is that it makes the documentation easier to read. I agree with that rationale, with the caveat Avar mentioned. There is a simple fix: just simplify the behavior being explained as well, by biting the bullet and dropping the tracking synonym after a suitable period in which it produces a warning. In the meantime, the documentation is valuable, and pretending that tracking does not exist for everyone who does not confusedly reread the docs a few times is just a way to lie to ourselves and make users' lives more difficult. Is that really the intent? Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Undocument deprecated alias 'push.default=tracking'
Junio C Hamano wrote: For those who want to _learn_ what possibilities are available to them (i.e. they are not going from `tracking` to what it means, but going in the opposite direction), it should be unmistakingly clear that `tracking` is not a part of the choices they should make. Until pre-1.7.4 versions of git fall out of use, I don't agree that the above is true. :( I do not think the following list created by a simple revert makes it clear. * `nothing` - do not push anything. * `matching` - push all branches having the same name in both ends. * `upstream` - push the current branch to ... * `simple` - like `upstream`, but refuses to ... * `tracking` - deprecated synonym for `upstream`. * `current` - push the current branch to a branch of the same name. How about the following? * `nothing` - ... * `matching` - ... * `upstream` - ... * `simple` - ... * `current` - ... For compatibility with ancient config files, the following synonym is also supported. Don't use it. * `tracking` - old name for `upstream` -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Undocument deprecated alias 'push.default=tracking'
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: That works because, as you mention, the usual way to look up an option in manpages is to search for --print, including the two minus signs. Unfortunately an analagous approach in gitconfig(5) would be seriously broken, because searching for tracking (no minus signs) is going to hit many false positives. I do not think such a change would be an improvement. I thought your example was that you saw pull.default = tracking and wondering what it is. Why do you need global search for tracking, not just near pull.default is described, in the first place? Because the UI for local searches in web browsers and man pagers is seriously lacking. Or, because people have bad habits and do not take apppropriate advantage of search in small subsections of a document. All I know is that I have seen myself and others doing searches analagous to --print and not seen searches analagous to tracking. Am I really the only one that doesn't see the --print change as hiding an option and sees burying tracking in the text as qualitatively different? Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Undocument deprecated alias 'push.default=tracking'
Junio C Hamano wrote: How about doing it this way? [...] --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1795,7 +1795,8 @@ push.default:: + This is currently the default, but Git 2.0 will change the default to `simple`. -* `upstream` - push the current branch to its upstream branch. +* `upstream` - push the current branch to its upstream branch + (`tracking` is a deprecated synonym for this). I have already explained that I believe this is a bad idea and why and proposed an alternative. I take it that either we are miscommunicating or we fundamentally disagree about the role of documentation. :( -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] Undocument deprecated alias 'push.default=tracking'
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Junio C Hamano wrote: For those who want to _learn_ what possibilities are available to them (i.e. they are not going from `tracking` to what it means, but going in the opposite direction), it should be unmistakingly clear that `tracking` is not a part of the choices they should make. Until pre-1.7.4 versions of git fall out of use, I don't agree that the above is true. :( The documentation ships with the version that the above is true. We are not making an update to documentation that comes with ancient versions. Part of the context that I should have mentioned but didn't is that it is common to put $HOME on a shared filesystem. [...] How about the following? * `nothing` - ... * `matching` - ... * `upstream` - ... * `simple` - ... * `current` - ... For compatibility with ancient config files, the following synonym is also supported. Don't use it. * `tracking` - old name for `upstream` Didn't I say I am fine to mention it as a side note in the original message you started responding to? Yes, I understood what you were proposing and I directly disagreed with it and explained why. The above is something like a compromise --- more precisely, it is an attempt to do something better than a straight revert and to understand whether it would address your objection. Clearly it doesn't. I don't understand why. Perhaps the Don't use it is over the top and that is your complaint? It's true that if I were writing it without your objection in mind, I wouldn't have included that sentence. It was written on the assumption that you want to discourage people from using the tracking synonym --- I am not personally convinced that that is worth discouraging at all, but it's fine with me if the consensus is to do so. 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] Rename {git- = git}remote-helpers.txt
Jeff King wrote: Maybe it is just me, but the fact that accessing the manpage is now: man gitremote-helpers feels weird to me. I know it technically follows our syntactic rules, but having the lack of dash be significant between git and remote, but then having a dash later makes it hard on the eyes. Yes. I have thought for years that it should be git-remote-helpers, that git help should be tweaked to look for that, and that the existing gitrepository-layout and friends should be replaced with redirects. I didn't say anything (except a random comment once on #git) because I can't promise to have time soon to work on it. Might try anyway. 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] Rename {git- = git}remote-helpers.txt
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Yes. I have thought for years that it should be git-remote-helpers, that git help should be tweaked to look for that, and that the existing gitrepository-layout and friends should be replaced with redirects. Because of the git help look up rules, we cannot have two pages that only differ at the dash (or absense of it) immediately after 'git'; e.g. one about the concept of 'frotz' in the context of Git, i.e. man gitfrotz, and the other about the subcommand to perform 'frotz', i.e. man git-frotz. The way to refer to these two pages are both git help frotz. Exactly. Hence the disambiguating dash-versus-nondash convention buys us nothing. -- To unsubscribe from this list: send the line unsubscribe 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] Documentation/Makefile: clean up MAN*_TXT lists
Jeff King wrote: We keep a list of the various files that end up as man1, man5, etc. Let's break these single-line lists into sorted multi-line lists, which makes diffs that touch them much easier to read. Independentally of the rest of the series, I think this is a good cleanup. --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -1,13 +1,28 @@ MAN7_TXT += gitcredentials.txt -MAN1_TXT= \ - $(filter-out $(addsuffix .txt, $(ARTICLES) $(SP_ARTICLES)), \ - $(wildcard git-*.txt)) \ - gitk.txt gitweb.txt git.txt +MAN1_TXT += git.txt +MAN1_TXT += gitk.txt +MAN1_TXT += gitweb.txt + If the user happens to have MAN[157]_TXT set in the environment, this would be affected by that. How about: # Guard against environment variables MAN1_TXT = MAN5_TXT = MAN7_TXT = MAN1_TXT += ... ... ? With 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/2] docs: convert concept manpages to git-*
Jeff King wrote: Let's just call everything git-*, which is simpler. This patch renames the documentation files, updates the Makefile to find them, and updates internal linkgit references to the pages. It updates builtin/help.c so that users of git help foo will not even notice the difference. Users of external html links, or users who have trained their fingers to type man gitfoo will notice the missing pages. This patch does not install a this page has moved placeholder, but that can easily be done on top. Thanks for writing this. I think this one should wait until someone (perhaps me) takes care of the redirects. Ideally in addition to simple this place has moved HTML placeholders and manpages using the .so macro, a makefile target to generate redirect directives for your apache configuration might make sense. In the meantime, having man gitrepository-layout is not the worst wart in the world. Cheers, 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] fixup! fixup! fixup! Change 'git' to 'Git' whenever the whole system is referred to #1
Hi, Thomas Ackermann wrote: Found by Junio: Change git-dir to $GIT_DIR and git-file to gitfile. Signed-off-by: Thomas Ackermann th.ac...@arcor.de --- Documentation/git-rev-parse.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt index c743469..14386ed 100644 --- a/Documentation/git-rev-parse.txt +++ b/Documentation/git-rev-parse.txt @@ -187,9 +187,9 @@ print a message to stderr and exit with nonzero status. Flags and parameters to be parsed. --resolve-git-dir path:: - Check if path is a valid git-dir or a git-file pointing to a valid - git-dir. If path is a valid git-dir the resolved path to git-dir will - be printed. + Check if path is a valid `$GIT_DIR` or a gitfile pointing to a valid + `$GIT_DIR`. If path is a valid `$GIT_DIR` the resolved path to `$GIT_DIR` + will be printed. Hm, I don't find the old or the new version very easy to understand. Perhaps the idea is something like this? Check if path is a valid git repository (.git or project.git directory) or gitdir: file. If path is a gitdir: file then the resolved path to the corresponding real git repository will be printed. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] Add a description for 'gitfile' to glossary
Junio C Hamano wrote: How about saying something like this here in the glossary: A plain file `.git` at the root of a working tree that points at the directory that is the real repository. And then as a separate patch, in gitrepository-layout.txt (eek---see the other thread), we can do something like this: Documentation/gitrepository-layout.txt | 24 ++-- 1 file changed, 18 insertions(+), 6 deletions(-) Looks correct and very readable. Thanks. Jonathan (patch left unsnipped for reference) diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt index 9f62886..473c6a0 100644 --- a/Documentation/gitrepository-layout.txt +++ b/Documentation/gitrepository-layout.txt @@ -12,12 +12,24 @@ $GIT_DIR/* DESCRIPTION --- -You may find these things in your git repository (`.git` -directory for a repository associated with your working tree, or -`project.git` directory for a public 'bare' repository. It is -also possible to have a working tree where `.git` is a plain -ASCII file containing `gitdir: path`, i.e. the path to the -real git repository). +A Git repository comes in two different flavours: + + * a `.git` directory at the root of the working tree; + + * a `project.git` directory that is a 'bare' repository + (i.e. without its own working tree), that is typically used for + exchanging histories with others by pushing into it and fetching + from it. + +*Note*: Also you can have a plain text file `.git` at the root of +your working tree, containing `gitdir: path` to point at the real +directory that has the repository. This mechanism is often used for +a working tree of a submodule checkout, to allow you in the +containing superproject to `git checkout` a branch that does not +have the submodule. The `checkout` has to remove the entire +submodule working tree, without losing the submodule repository. + +These things may exist in a Git repository. objects:: Object store associated with this repository. Usually -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] Use consistent links for User Manual and Everyday Git; Fix a quoting error
Thomas Ackermann wrote: --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -23,7 +23,7 @@ and full access to internals. See linkgit:gittutorial[7] to get started, then see link:everyday.html[Everyday Git] for a useful minimum set of -commands. The link:user-manual.html[Git User's Manual] has a more +commands. The link:user-manual.html[The Git User's Manual] has a more in-depth introduction. In the rendered version, this looks like: The The Git User's Manual[1] has a more in-depth introduction. Presumably the first The should be dropped from either the link or the surrounding text. [...] --- a/Documentation/gitcore-tutorial.txt +++ b/Documentation/gitcore-tutorial.txt @@ -17,7 +17,7 @@ work with a Git repository. If you just need to use Git as a revision control system you may prefer to start with A Tutorial Introduction to Git (linkgit:gittutorial[7]) or -link:user-manual.html[the Git User Manual]. +link:user-manual.html[The Git User's Manual]. This comes out as ... you may prefer to start with A Tutorial Instruction to Git (gittutorial(7)) or The Git User's Manual[1]. The capital 'T' in The looks a bit strange, but a lowercase 't' in the corresponding footnote would also look strange. We can't have everything, I guess. A possible fix would be to drop the The from the link. The way you have it here also seems fine. [...] --- a/Documentation/gittutorial-2.txt +++ b/Documentation/gittutorial-2.txt @@ -406,7 +406,7 @@ pages for any of the git commands; one good place to start would be with the commands mentioned in link:everyday.html[Everyday Git]. You should be able to find any unknown jargon in linkgit:gitglossary[7]. -The link:user-manual.html[Git User's Manual] provides a more +The link:user-manual.html[The Git User's Manual] provides a more comprehensive introduction to Git. Doubled 'The'. [...] --- a/Documentation/gittutorial.txt +++ b/Documentation/gittutorial.txt @@ -656,7 +656,7 @@ digressions that may be interesting at this point are: * linkgit:gitworkflows[7]: Gives an overview of recommended workflows. - * link:everyday.html[Everyday Git with 20 Commands Or So] + * link:everyday.html[Everyday Git] Isn't the old title more informative? 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: Re: Re: Re: Segmentation fault with latest git (070c57df)
Jongman Heo wrote: But it doesn't stimulate any prerequisites in make, which is weird. What's in builtin/.depend/fetch.o.d? [...] please see below~. $ cat builtin/.depend/fetch.o.d fetch.o: builtin/fetch.c cache.h git-compat-util.h compat/bswap.h \ That's the problem. See the following thread: http://thread.gmane.org/gmane.comp.version-control.git/185625/focus=185680 Currently when COMPUTE_HEADER_DEPENDENCIES=auto git tests for dependency generation support by checking the output and exit status from the following command: $(CC) $(ALL_CFLAGS) -c -MF /dev/null -MMD -MP \ -x c /dev/null -o /dev/null 21 Perhaps this can be improved? Even something as simple as a ccache version test could presumably help a lot. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH resend] Makefile: explicitly set target name for autogenerated dependencies
Date: Fri, 18 Nov 2011 17:23:24 -0600 gcc -MF depfile -MMD -MP -c -o path/to/file.o produces a makefile snippet named depfile describing what files are needed to build the target given by -o. When ccache versions before v3.0pre0~187 (Fix handling of the -MD and -MDD options, 2009-11-01) run, they execute gcc -MF depfile -MMD -MP -E instead to get the final content for hashing. Notice that the -c -o combination is replaced by -E. The result is a target name without a leading path. Thus when building git with such versions of ccache with COMPUTE_HEADER_DEPENDENCIES enabled, the generated makefile snippets define dependencies for the wrong target: $ make builtin/add.o GIT_VERSION = 1.7.8.rc3 * new build flags or prefix CC builtin/add.o $ head -1 builtin/.depend/add.o.d add.o: builtin/add.c cache.h git-compat-util.h compat/bswap.h strbuf.h \ After a change in a header file, object files in a subdirectory are not automatically rebuilt by make: $ touch cache.h $ make builtin/add.o $ Luckily we can prevent trouble by explicitly supplying the name of the target to ccache and gcc, using the -MQ option. Do so. Reported-and-tested-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com Reported-by: : 허종만 jongman@samsung.com Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 731b6a8..5a2e02d 100644 --- a/Makefile +++ b/Makefile @@ -973,7 +973,8 @@ endif ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto) dep_check = $(shell $(CC) $(ALL_CFLAGS) \ - -c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 21; \ + -c -MF /dev/null -MQ /dev/null -MMD -MP \ + -x c /dev/null -o /dev/null 21; \ echo $$?) ifeq ($(dep_check),0) override COMPUTE_HEADER_DEPENDENCIES = yes @@ -1843,7 +1844,7 @@ $(dep_dirs): missing_dep_dirs := $(filter-out $(wildcard $(dep_dirs)),$(dep_dirs)) dep_file = $(dir $@).depend/$(notdir $@).d -dep_args = -MF $(dep_file) -MMD -MP +dep_args = -MF $(dep_file) -MQ $@ -MMD -MP ifdef CHECK_HEADER_DEPENDENCIES $(error cannot compute header dependencies outside a normal build. \ Please unset CHECK_HEADER_DEPENDENCIES and try again) -- 1.8.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gitk-git/.gitignore: add rule for gitk-wish
Hi Ram, Ramkumar Ramachandra wrote: 8f26aa4 (Makefile: remove tracking of TCLTK_PATH, 2012-12-18) removed /gitk-git/gitk-wish from the toplevel .gitignore, with the intent of moving it to gitk-git/.gitignore in a later patch. This was never realized. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- Minor patch, so I didn't bother sending it through Paul. All gitk patches go through Paul's repo. I keep forgetting the address, so I look it up each time. $ git log -1 --oneline gitk-git/ 9a6c84e Merge git://ozlabs.org/~paulus/gitk Looks like this was fixed in the week since last pull. http://thread.gmane.org/gmane.comp.version-control.git/214312 Paul, would it be safe for Junio to pull again? 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 1/3] fix clang -Wtautological-compare with unsigned enum
John Keeping wrote:
From: Antoine Pelisse apeli...@gmail.com
Create a GREP_HEADER_FIELD_MIN so we can check that the field value is
sane and silent the clang warning.
Thanks. Looks good to me.
[...]
--- a/grep.c
+++ b/grep.c
@@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct
grep_opt *opt)
for (p = opt-header_list; p; p = p-next) {
if (p-token != GREP_PATTERN_HEAD)
die(bug: a non-header pattern in grep header list.);
- if (p-field 0 || GREP_HEADER_FIELD_MAX = p-field)
+ if (p-field GREP_HEADER_FIELD_MIN ||
+ GREP_HEADER_FIELD_MAX = p-field)
die(bug: unknown header field %d, p-field);
I also think it would be fine to drop this test or replace it with an
assert((unsigned) p-field ARRAY_SIZE(header_field));
because we know the test never trips.
--
To unsubscribe from this list: send the line unsubscribe 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] gitk: Display the date of a tag in a human friendly way.
Hi Anand, Anand Kumria wrote: I've not been able to find the canonical location of your gitk repository. Here's how I find it: $ git clone git://repo.or.cz/git.git [...] $ cd git $ git log -1 --oneline -- gitk-git ec3ae6ec Merge git://ozlabs.org/~paulus/gitk $ cd .. $ git clone git://ozlabs.org/~paulus/gitk.git Patches, including documentation patches, go to git@vger.kernel.org, cc-ing Paul Mackerras. 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: Segmentation fault with latest git (070c57df)
Jongman Heo wrote: Unfortunately, the patch didn't help to me. Thanks for testing. Did you apply the patch to the older version of git that generates builtin/.depend/fetch.o.d or the newer version that consumes it? 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: Segmentation fault with latest git (070c57df)
Junio C Hamano wrote: The only case that worries me is when make or cc gets interrupted. As long as make removes the ultimate target *.o in such a case, it is fine to leave a half-written .depend/foo.o.d (or getting it removed) behind. gcc removes the target .o in its signal handler in such a case. In cases where it doesn't get a chance to (e.g., sudden power failure), there is a partially written .o file already in place, the linker produces errors, and the operator is convinced to run make clean, all without .depend's help. -- To unsubscribe from this list: send the line unsubscribe 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: file named - on git commit
Junio C Hamano wrote: (some may be too minor to be worth backproting, for example). Yes, this is the part I was asking for help with. Backporting is easy but convincing the release team and upgrade-averse sysadmins to like the result generally isn't. Occasional nominations of the form this change is important in my workflow could help. Continuing to stick to fixes to very severe bugs that stand out plus a random assortment of problems people have reported can also work fine, though. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] status: show the branch name if possible in in-progress info
Nguyễn Thái Ngọc Duy wrote: Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com Missing description. Stealing from the link you sent: The typical use-case is starting a rebase, do something else, come back the day after, run git status or make a new commit and wonder what in the world's going on. Which branch is being rebased is probably the most useful tidbit to help, but the target may help too. Ideally, I would have loved to see rebasing master on origin/master, but the target ref name is not stored during rebase, so this patch writes rebasing master on a78c8c98b as a half-measure to remind future users of that potential improvement. Signed-off-by: ... -- To unsubscribe from this list: send the line unsubscribe 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 0/8] Hiding refs
Hi Michael, Michael Haggerty wrote: I would again like to express my discomfort about this feature, which is already listed as will merge to next. Frankly, I have the feeling that this feature is being steamrolled in before a community consensus has been reached and indeed before many valid points raised by other members of the community have even been addressed. For example: In $dayjob I work with Gerrit, so I think I can start to answer some of these questions. * I didn't see a response to Peff's convincing arguments that this should be a client-side feature rather than a server-side feature [1]. The client can't control the size of the ref advertisement. That is the main motivation if I understood correctly. * I didn't see an answer to Duy's question [2] about what is different between the proposed feature and gitnamespaces. Namespaces are more complicated and don't sit well in existing setups involving git repositories whose refs are not namespaced. * I didn't see a response to my worries that this feature could be abused [3]. Can you elaborate? Do you mean that through social engineering an attacker would convince the server admin to store secrets using a hidden ref and enable the upload-archive service? That does sound like a reasonable concern. Perhaps the documentation should be updated along these lines transfer.hiderefs:: String(s) `upload-pack` and `receive-pack` use to decide which refs to omit from their initial advertisement. Use more than one transfer.hiderefs configuration variables to specify multiple prefix strings. A ref that are under the hierarchies listed on the value of this variable is excluded, and is hidden from `git ls-remote`, `git fetch`, `git push :`, etc. An attempt to update or delete a hidden ref by `git push` is rejected, and an attempt to fetch a hidden ref by `git fetch` will fail. + This setting does not currently affect the `upload-archive` service. until someone interested implements the same for upload-archive. I also think that the feature is poorly designed. For example: That's another reasonable concern. It's very hard to get a design correct right away, which is presumably part of the motivation of getting this into the hands of interested users who can give feedback on it. What would potentially be worth blocking even that is concerns about the wire protocol, since it is hard to take back mistakes there. * Why should a repository have exactly one setting for what refs should be hidden? Wouldn't it make more sense to allow multiple views to be defined?: How do I request a different view of the repository at /path/to/repo.git over the network? How can we make the common case of only one view easy to achieve? Isn't the multiple-views case exactly what gitnamespaces is for? [...] * Is it enough to support only reference exclusion (as opposed to exclusion and inclusion rules)? The motivating example is turning off advertisement of the refs/changes hierarchy. If and when more complicated cases come up, that would presumably be the time to support more complicated configuration. [...] * Why should this feature only be available remotely? It is about transport. Ref namespaces have their own set of use cases and are a distinct feature. 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 v3 0/8] Hiding refs
Junio C Hamano wrote: Duy Nguyen pclo...@gmail.com writes: On Tue, Feb 5, 2013 at 5:29 PM, Michael Haggerty mhag...@alum.mit.edu wrote: Hiderefs creates a dark corner of a remote git repo [...] Or you can think hiderefs is the first step to addressing the initial ref advertisment problem. The series says hidden refs are to be fetched out of band, but that's not the only way. Let me help unconfuse this thread. I think the series as 8-patch series was poorly presented, and separating it into two will help understanding what they are about. The first three: upload-pack: share more code upload-pack: simplify request validation upload/receive-pack: allow hiding ref hierarchies is _the_ topic of the series. As far as I am concerned (I am not speaking for Gerrit users, but am speaking as the Git maintainer), the topic is solely about uncluttering. There may be refs that the server end may need to keep for its operation, but that remote users have _no_ business knowing about. An obvious question when looking at that alone is, is there ever actually need for such private refs? If the refs are not meant to be shared with users *at all*, why are they even refs? An answer is because refs force gc to keep the corresponding objects. For example, the sysadmin may want to keep refs/archived/ refs for dead branches that should not be advertised or accessible to the user any more. Seems sane, though not especially exciting. What is more exciting to me is that it is a first step toward addressing the complicated problem of offering access to more refs than can be efficiently presented in the current ref advertisement. I think that's a harder problem but something like this would be needed in order to support existing clients without performance degredation. And in the meantime, it helps with the refs/archived case. Thanks for explaining. 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 0/8] Hiding refs
Michael Haggerty wrote: Scenario 1: Some providers junk up their users' repositories with content that is not created by the repository's owner and that the owner doesn't want to appear to vouch for (e.g., GitHub pull requests). These references might sometimes be useful to fetch, singly or in bulk. Scenario 2: Some systems junk up their users' repositories with additional references that are not interesting to most pullers (e.g., Gerrit activity markers) though they don't add questionable content. Actually Gerrit's refs/changes refs are pretty similar to Github's refs/pull. Both are requests for code review. [...] But now every time I do a gitk --all or git log --decorate, the output is cluttered with all of his references (most of which are just old versions of references from the upstream repository that we both use). I would like to be able to hide his references most of the time but turn them back on when I need them. Scenario 5: Our upstream repository has gazillions of release tags under refs/tags/releases/..., sometimes including customer-specific releases. In my daily life these are just clutter. For both of these use cases, putting the refs somewhere other than refs/heads, refs/tags, and refs/remotes should be enough to avoid clutter. I agree that a --decorate-glob along the lines of git rev-parse's --glob would be nice. [...] * Some small improvements (e.g. allowing *multiple* views to be defined) would provide much more benefit for about the same effort, and would be a better base for building other features in the future (e.g., local views). Would advertising GIT_CONFIG_PARAMETERS and giving examples for server admins to set it in inetd et al to provide different kinds of access to a same repository through different URLs work? Thanks for listening. Michael [1] Theoretically one could support multiple views of a single repository by using something like GIT_CONFIG=view_1_config git upload-pack ... or git -c transfer.hiderefs=... git upload-pack ..., but this would be awkward. Ah, I missed this comment before. What's awkward about that? I think it's a clean way to make many aspects of how a repository is presented (including hook actions) configurable. Thanks for your help clarifying this feature. Hopefully some of the discussion will filter into the documentation. 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: Proposal: branch.name.remotepush
Hi Ram, Ramkumar Ramachandra wrote: And yes, a regular `git push origin refs/for/master` is just retarded. The usual incantation is git push gerrit HEAD:refs/for/master. Is the code review creation push that uses a different branchname from the branch the integrator pulls what seems backward, or is it the need to specify a refname at all on the command line? I agree that a [branch master] pushremote configuration would be handy. pushremote instead of remotepush to be less surprising to people who have already seen pushurl. Good luck, 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: Proposal: branch.name.remotepush
Junio C Hamano wrote: I'd actually see this as Gerrit being weird. If it wants to quarantine a commit destined to the master branch, couldn't it just let people push to master and then internally update for/master instead? It is because pushing doesn't update refs/heads/master. Instead, it starts a code review. Suppose Gerrit allows starting a new code review by pushing to refs/heads/master. It sounds okay if I squint --- it's just a very slow asynchronous ref update, right? Let's see: $ git clone gerrit server test Cloning into 'test'... $ echo hi greeting $ git add greeting $ git commit -q -m 'hello' $ git push origin master [...] remote: New Changes: remote: gerrit server/r/1234 remote: To url ea4cb77b..9117390e master - master $ : walk away, forget what I was doing $ git fetch origin From url + 9117390...ea4cb77 master - origin/master (forced update) Wait, why did the remote rewind? Regards, 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: Proposal: branch.name.remotepush
Michael J Gruber wrote: Junio C Hamano venit, vidit, dixit 08.02.2013 09:16: Jonathan Nieder jrnie...@gmail.com writes: Wait, why did the remote rewind? Oh, I am very well aware of that glitch. git push has this hack to pretend as if the pusher immediately turned around and fetched from the remote. It shouldn't have been made to do so unconditionally; instead it should have been designed to give the pushee a way to optionally tell you I acccept this push, but you may not see it to be updated to that exact value you pushed when you fetched from me right now. Yes, I agree with this. The git push hack does seem to be useful in practice for helping people just starting to use git. If they have a separate gitk --all window open, they can refresh it and see the remote-tracking branch corresponding to the branch that has been pushed advancing. It matches a model in which remote-tracking refs represent git's idea of where these branches are in the remote repository. And in that model, a remote being able to respond to a push with ref update queued, but please keep in mind that it may take me a while to chew through that queue should be perfectly reasonable. [...] And this seems to be more natural, too. It can keep the internals (the auxiliary ref on the server side) hidden from the user. Just to clarify: this is not an internal ref being exposed. No auxiliary refs/for/master ref actually exists. The ref Gerrit users push to is a UI fiction. That's important because otherwise two developers could not propose changes for the same branch at the same time. 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 checkout --orpan` leaves a dirty worktree
Ramkumar Ramachandra wrote: Why should I have to `git rm -rf .` after a `git checkout --orphan`? What sort of misfeature/ incomplete feature is this? One designed for the going open source use case, where you have existing code that you want to put into a new branch without history. When there is no existing code, it seems simpler to do cd .. git init code-that-has-nothing-to-do-previous-cwd cd code-that-* ... hack hack hack ... git commit git remote add origin url git push -u origin master BTW, I suspect a clearer way to say what you meant is Sounds like a misfeature which is gentler and more focused than an implied What kind of idiot designed this? Even if you are thinking the latter. :) 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: Permission denied on home dir results in fatal error as of 1.8.1.1
Junio C Hamano wrote: Nick Muerdter st...@nickm.org writes: As of git 1.8.1.1 and above (tested up to 1.8.1.3), if the home directory can't be accessed, it results in a fatal error. In git 1.8.1 and below this same setup just resulted in warnings. Was this an intentional change? I think this was done to not just help diagnosing misconfiguration, but to prevent an unintended misconfiguration from causing problems (e.g. the user thinks user.name is set up correctly, but forbids Git from reading it from the configuration files, and ends up creating commits under wrong names). Yes, that's right. Sometimes ignoring settings has bad consequences, so git errors out to let the user intervene and decide whether the inaccessible settings are important. 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] git-bisect.txt: clarify that reset finishes bisect
Hi, Michael J Gruber wrote: reset can be easily misunderstood as resetting a bisect session to its start without finishing it. Clarify that it actually finishes the bisect session. FWIW, Reviewed-by: Jonathan Nieder jrnie...@gmail.com Addressing Andreas's original concern about the discoverability of 'git bisect reset' would presumably require doing two more things: 1. adding an example of the normal bisection workflow to the EXAMPLES section 2. training users to look to the EXAMPLES section That is, something like the below. But I'm not happy with it, because it just runs over the same material as the current Description section. Maybe the current tutorial material could be moved to examples and replaced with something terser that fleshes out the descriptions in git bisect -h output. What do you think? diff --git i/Documentation/git-bisect.txt w/Documentation/git-bisect.txt index e4f46bc1..b89abd78 100644 --- i/Documentation/git-bisect.txt +++ w/Documentation/git-bisect.txt @@ -356,6 +356,54 @@ $ git bisect run sh -c make || exit 125; ~/check_test_case.sh This shows that you can do without a run script if you write the test on a single line. +* Bisect to find which patch caused a boot failure: ++ +Install a recent kernel: ++ + +$ cd ~/src/linux +$ git checkout origin/master +$ make deb-pkg # or binrpm-pkg, or tar-pkg +$ dpkg -i ../name of package # as root (or rpm -i, or tar -C / -xf) +$ reboot # as root + ++ +Hopefully it fails to boot, so tell git so and begin bisection: ++ + +$ cd ~/src/linux +$ git bisect start HEAD v3.2 # assuming 3.2 works fine +- ++ +A candidate revision to test is automatically checked out. +Test it: ++ +- +$ make deb-pkg # or binrpm-pkg, or tar-pkg +$ dpkg -i ../name of package # as root (or rpm -i, or tar -C / -xf) +$ reboot # as root +- ++ +Record the result: ++ +- +$ cd ~/src/linux +$ git bisect good # if it booted correctly +$ git bisect bad # if it failed to boot +$ git bisect skip # if some other bug made it hard to test +- ++ +Repeat until bored or git prints the first bad commit. When +done: ++ +- +$ git bisect log log # let others pick up where you left off +$ git bisect reset HEAD # exit the bisecting state +- ++ +At any step, you can run `git bisect visualize` to watch the +regression range narrowing. + * Locate a good region of the object graph in a damaged repository + -- To unsubscribe from this list: send the line unsubscribe 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: Allow extracting revisions into directories
Hi Robert, Robert Clausecker wrote: There are two things git archive is missing that are needed in my use case: First, git archive in combination with tar won't remove unneeded files. You have to run rm -rf before manually which brings me to the next point; git archive can't really make incremental updates. My advice is to keep a separate index file for your exported files. Like this: GIT_DIR=$(readlink -f $(git rev-parse --git-dir)) GIT_INDEX_FILE=$GIT_DIR/index-for-deployment export GIT_DIR GIT_INDEX_FILE cd $dest git read-tree -m -u tree 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: Feature request: Allow extracting revisions into directories
Robert Clausecker wrote: That is actually a pretty interesting approach. I can use a different index file for different deployments. How does this cooperate with bare repositories? Aren't they supposed to have no index file at all? It should work fine in a bare repo. If you can think of a good place to sneak hints about this into the documentation (maybe as an example in git-archive(1) or a new gitenvironment(7) page), that would be very welcome. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git prompt
Ethan Reesor wrote: I have a git user set up on my server. It's prompt is set to git-prompt and it's git-shell-commands is empty. [...] How do I make the git user work like github where, upon attempting to get a prompt, the connection is closed? I assume you mean that the user's login shell is git-shell. You can disable interactive logins by removing the ~/git-shell-commands/ directory. Unfortunately that doesn't let you customize the message. Perhaps it would make sense to teach shell.c to look for a [shell] greeting = 'Hi %(username)! You've successfully authenticated, but I do not provide interactive shell access.' setting in git's config file. 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 v2 03/15] user-manual: Use 'remote add' to setup push URLs
W. Trevor King wrote: On Sun, Feb 10, 2013 at 01:33:31PM -0800, Junio C Hamano wrote: The resulting text may read like so: … I'm fine with this too, but if this is the suggested route, why bother with `git config` at all? Is it just for ease of scripting? Yes. It can also be helpful when giving help over IRC, since you can get reproducible results without assuming the user has a proper text editor set up, but that is just a special case of scripting. ;-) For everyday interactive configuration editing, config files have some good advantages: - The settings are easy to read, well organized, and all in one place - The file can include comments. 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
[RFC/PATCH] shell: allow 'help' command to disable interactive shell
If I disable git-shell's interactive mode by removing the
~/git-shell-commands directory, then attempts to use 'ssh' with the
git account interactively produce an error message intended for the
administrator:
$ ssh git@myserver
fatal: Interactive git shell is not enabled.
hint: ~/git-shell-commands should exist and have read and execute
access.
$
It is better to give the user a friendly hint that she is on the
right track, like GitHub does:
Hi username! You've successfully authenticated, but
GitHub does not provide shell access.
An appropriate greeting might even include more complex information,
like a list of repositories the user has access to. A
git-shell-commands directory with only a help script can get us most
of the way there, but it unfortunately it produces a git prompt
where the user can do nothing but ask for more help or exit. So allow
the help script to abort the shell by exiting with nonzero status.
Downside: this will prevent interactive git-shell logins in existing
setups where the help script exits with nonzero status by mistake.
Hopefully those are rare enough to not cause much trouble in practice.
Reported-by: Ethan Reesor firelizz...@gmail.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Sitaram Chamarty wrote:
Indeed! In gitolite, I borrowed that idea added to it by making it
print a list of repos you have access to, along with what permissions
(R or RW) you have :-)
I'm not suggesting git should do that, but instead of a fixed string,
a default command to be executed would be better.
Good call.
[...]
This of course now means that the ~/git-shell-commands should not be
empty, since that is where this default command also will be present.
How about this?
A patch on top could change the default git-shell-commands is not
present message if that seems worthwhile.
Documentation/git-shell.txt | 26 ++
shell.c | 10 --
2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 9b925060..758083ff 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -29,6 +29,32 @@ read and execute permissions to the directory in order to
execute the
programs in it. The programs are executed with a cwd of $HOME, and
argument is parsed as a command-line string.
+When run interactively (with no arguments), 'git-shell' will
+automatically run `~/git-shell-commands/help` on startup, provided it
+exists. If the 'help' command fails then the interactive shell is
+aborted.
+
+EXAMPLE
+---
+
+To disable interactive logins, displaying a greeting instead:
++
+
+$ chsh -s /usr/bin/git-shell
+$ mkdir $HOME/git-shell-commands
+$ cat $HOME/git-shell-commands/help \EOF
+#!/bin/sh
+printf '%s\n' Hi $USER! You've successfully authenticated, but I do not
+printf '%s\n' provide interactive shell access.
+exit 128
+EOF
+$ chmod +x $HOME/git-shell-commands/help
+
+
+SEE ALSO
+
+contrib/git-shell-commands/README
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/shell.c b/shell.c
index 84b237fe..3abc2b84 100644
--- a/shell.c
+++ b/shell.c
@@ -63,10 +63,16 @@ static void cd_to_homedir(void)
static void run_shell(void)
{
- int done = 0;
+ int done = 0, status;
static const char *help_argv[] = { HELP_COMMAND, NULL };
/* Print help if enabled */
- run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+ status = run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+ if (!status)
+ ; /* success */
+ else if (status == -1 errno == ENOENT)
+ ; /* help disabled */
+ else
+ exit(status);
do {
struct strbuf line = STRBUF_INIT;
--
1.8.1.3
--
To unsubscribe from this list: send the line unsubscribe 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] shell: allow 'help' command to disable interactive shell
Jeff King wrote: On Sun, Feb 10, 2013 at 05:20:16PM -0800, Jonathan Nieder wrote: +When run interactively (with no arguments), 'git-shell' will +automatically run `~/git-shell-commands/help` on startup, provided it +exists. If the 'help' command fails then the interactive shell is +aborted. Doesn't that mean that people who currently do allow interactive access and have a ~/git-shell-commands/help (that returns zero) will get spammed by its as a motd each time they connect? Only interactive connections. That's the existing behavior. [...] What about ssh example.com foo? Do we want to allow a custom message there, too (it might be different there; e.g., an allowed list of commands might make more sense)? I wouldn't mind, but it's definitely not my itch. 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: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
Junio C Hamano wrote:
Jonathan Nieder jrnie...@gmail.com writes:
How about this?
A patch on top could change the default git-shell-commands is not
present message if that seems worthwhile.
Hmph.
I wonder if rewording the message when git-shell-commmands directory
is not there may be a better first step (which actually could be the
last step)?
Maybe, but it's not a step that I'm interested in. I don't think it
changes the desirability of the patch I sent. They are independent.
[...]
--- a/shell.c
+++ b/shell.c
@@ -162,9 +162,11 @@ int main(int argc, char **argv)
/* Allow the user to run an interactive shell */
cd_to_homedir();
if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
- die(Interactive git shell is not enabled.\n
+ die(The user has been recognized as '%s' but
+ interactive git shell is not enabled.\n
hint: ~/ COMMAND_DIR should exist
- and have read and execute access.);
+ and have read and execute access.,
+ get_user_name());
Personally I don't think the hint should be here at all (it should be
obvious that git-shell(1) is the place to read about the login
behavior of an account with shell set to git-shell), but I don't mind
as long as it's possible to override the message.
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] shell: allow 'help' command to disable interactive shell
Jeff King wrote: On Sun, Feb 10, 2013 at 08:14:04PM -0800, Jonathan Nieder wrote: Only interactive connections. That's the existing behavior. Ah, sorry. I misread the patch. I see now that we already run help, and this is just making the exit value significant. In that case, yeah, I think it's fine. No problem --- the description was unclear. Would retitling the patch to shell: pay attention to exit status from 'help' command 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: [RFC/PATCH] shell: allow 'help' command to disable interactive shell
Junio C Hamano wrote: Are you shooting for customizability? Yes, and the ability to generate the message dynamically. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2 v2] shell: allow 'help' command to disable interactive shell
Jeff King wrote: I think what threw me off was reading the documentation part of the patch, which adds a note that we run help on startup, and then elaborates on the exit value. I didn't realize that the first half was documenting what already happened. Tweaking the third paragraph of the commit message to: Very nice. How about this version? Jonathan Nieder (2): shell doc: emphasize purpose and security guarantees shell: pay attention to exit status from 'help' command Documentation/git-shell.txt | 86 + shell.c | 10 -- 2 files changed, 79 insertions(+), 17 deletions(-) -- To unsubscribe from this list: send the line 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/2] shell doc: emphasize purpose and security model
The original git-shell(1) manpage emphasized that the shell supports only git transport commands, and as the shell gained features that emphasis and focus in the manual has been lost. Bring it back by splitting the manpage into a few short sections and fleshing out each: - SYNOPSIS, describing how the shell gets used in practice - DESCRIPTION, which gives an overview of the purpose and guarantees provided by this restricted shell - COMMANDS, listing supported commands and restrictions on the arguments they accept - INTERACTIVE USE, describing the interactive mode Also add a see also section with some relevant related reading. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- New text. Split off from patch 2 --- this is just documenting existing behavior. Documentation/git-shell.txt | 66 ++--- 1 file changed, 51 insertions(+), 15 deletions(-) diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt index 9b925060..4fe93203 100644 --- a/Documentation/git-shell.txt +++ b/Documentation/git-shell.txt @@ -9,25 +9,61 @@ git-shell - Restricted login shell for Git-only SSH access SYNOPSIS [verse] -'git shell' [-c command argument] +'chsh' -s $(which git-shell) git +'git clone' `git@localhost:/path/to/repo.git` +'ssh' `git@localhost` DESCRIPTION --- -A login shell for SSH accounts to provide restricted Git access. When -'-c' is given, the program executes command non-interactively; -command can be one of 'git receive-pack', 'git upload-pack', 'git -upload-archive', 'cvs server', or a command in COMMAND_DIR. The shell -is started in interactive mode when no arguments are given; in this -case, COMMAND_DIR must exist, and any of the executables in it can be -invoked. - -'cvs server' is a special command which executes git-cvsserver. - -COMMAND_DIR is the path $HOME/git-shell-commands. The user must have -read and execute permissions to the directory in order to execute the -programs in it. The programs are executed with a cwd of $HOME, and -argument is parsed as a command-line string. +This is a login shell for SSH accounts to provide restricted Git access. +It permits execution only of server-side Git commands implementing the +pull/push functionality, plus custom commands present in a subdirectory +named `git-shell-commands` in the user's home directory. + +COMMANDS + + +'git shell' accepts the following commands after the '-c' option: + +'git receive-pack argument':: +'git upload-pack argument':: +'git upload-archive argument':: + Call the corresponding server-side command to support + the client's 'git push', 'git fetch', or 'git archive --remote' + request. +'cvs server':: + Imitate a CVS server. See linkgit:git-cvsserver[1]. + +If a `~/git-shell-commands` directory is present, 'git shell' will +also handle other, custom commands by running +`git-shell-commands/command arguments` from the user's home +directory. + +INTERACTIVE USE +--- + +By default, the commands above can be executed only with the '-c' +option; the shell is not interactive. + +If a `~/git-shell-commands` directory is present, 'git shell' +can also be run interactively (with no arguments). If a `help` +command is present in the `git-shell-commands` directory, it is +run to provide the user with an overview of allowed actions. Then a +`git ` prompt is presented at which one can enter any of the +commands from the `git-shell-commands` directory, or `exit` to close +the connection. + +Generally this mode is used as an administrative interface to allow +users to list repositories they have access to, create, delete, or +rename repositories, or change repository descriptions and +permissions. + +SEE ALSO + +ssh(1), +linkgit:git-daemon[1], +contrib/git-shell-commands/README GIT --- -- 1.8.1.3 -- To unsubscribe from this list: send the line 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/2] shell: pay attention to exit status from 'help' command
If I disable git-shell's interactive mode by removing the
~/git-shell-commands directory, then attempts to use 'ssh' with the
git account interactively produce an error message intended for the
administrator:
$ ssh git@myserver
fatal: Interactive git shell is not enabled.
hint: ~/git-shell-commands should exist and have read and execute
access.
$
That is helpful for the new admin who is wondering What? Why isn't
the git-shell I just set up working?, but once the site setup is
finished, it is better to give the user a friendly hint that she is on
the right track, like GitHub does:
Hi username! You've successfully authenticated, but
GitHub does not provide shell access.
An appropriate greeting might even include more complex information,
like a list of repositories the user has access to. If the
git-shell-commands directory exists and contains a help script, we
already run it when the shell is run without any commands, giving the
server a chance to provide a custom message. Unfortunately, the
presence of the git-shell-commands directory means we also enter an
interactive mode, prompting and accepting commands (of which there may
be none) from the user, which many servers would not want. To solve
this, we abort the interactive shell on a non-zero exit code from the
help script. This lets the server say whatever it likes, and then
hang up.
Downside: this will prevent interactive git-shell logins in existing
setups where the help script exits with nonzero status by mistake.
Hopefully those are rare enough to not cause much trouble in practice.
Reported-by: Ethan Reesor firelizz...@gmail.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Improved-by: Jeff King p...@peff.net
---
Documentation/git-shell.txt | 20
shell.c | 10 --
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 4fe93203..60051e63 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -59,6 +59,26 @@ users to list repositories they have access to, create,
delete, or
rename repositories, or change repository descriptions and
permissions.
+If the `help` command exists and exits with nonzero status, the
+interactive shell is aborted.
+
+EXAMPLE
+---
+
+To disable interactive logins, displaying a greeting instead:
++
+
+$ chsh -s /usr/bin/git-shell
+$ mkdir $HOME/git-shell-commands
+$ cat $HOME/git-shell-commands/help \EOF
+#!/bin/sh
+printf '%s\n' Hi $USER! You've successfully authenticated, but I do not
+printf '%s\n' provide interactive shell access.
+exit 128
+EOF
+$ chmod +x $HOME/git-shell-commands/help
+
+
SEE ALSO
ssh(1),
diff --git a/shell.c b/shell.c
index 84b237fe..3abc2b84 100644
--- a/shell.c
+++ b/shell.c
@@ -63,10 +63,16 @@ static void cd_to_homedir(void)
static void run_shell(void)
{
- int done = 0;
+ int done = 0, status;
static const char *help_argv[] = { HELP_COMMAND, NULL };
/* Print help if enabled */
- run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+ status = run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+ if (!status)
+ ; /* success */
+ else if (status == -1 errno == ENOENT)
+ ; /* help disabled */
+ else
+ exit(status);
do {
struct strbuf line = STRBUF_INIT;
--
1.8.1.3
--
To unsubscribe from this list: send the line unsubscribe 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] shell: allow 'help' command to disable interactive shell
Ethan Reesor wrote: That way, there's a default setting, there can be a system-wide message, there can be a user specific message, and those messages can be set via `git-commit`. That won't let me imitate gitolite's behavior without a lot of config file churn: $ ssh git@localhost Hello, jrn. This is git@elie running git-shell 1.8.1.3. R Wpath/to/one/repo R path/to/another/repo $ -- To unsubscribe from this list: send the line unsubscribe 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] shell: allow 'help' command to disable interactive shell
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: Junio C Hamano wrote: Are you shooting for customizability? Yes, and the ability to generate the message dynamically. Hmph, if that is the case, wouldn't it be a better direction to give a better help for majority of the case where git-shell is used as the login shell to allow push and fetch but not for interactive access at all? The first step in that direction may be to give a better canned message, followed by a mechanism (perhaps a hook) that lets a message customized for the site's needs, no? The trouble is that I can't imagine a canned message that everyone will like. (For example, I quite dislike the current one.) That's exactly the situation in which some configurability is helpful. Some configurability is nice for other situations, anyway. For example, sites serving a multilingual audience may want the message to vary based on the user's language (or even source IP). The message can include a list of available repositories or extra information that changes over time. And so on. 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] shell: allow 'help' command to disable interactive shell
[administrivia: please don't top-post] Ethan Reesor wrote: Why not have both? That way there is a way to get a customizable response that avoids Junio's complaints and there is a way to do what you are trying to achieve. What was Junio's complaint? 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] shell: allow 'help' command to disable interactive shell
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: The trouble is that I can't imagine a canned message that everyone will like. (For example, I quite dislike the current one.) That's exactly the situation in which some configurability is helpful. I am not saying we should have a perfect canned message everybody likes and not have any configurability. I however think we can aim to come up with a message that covers 80% of site administrators who do not care too much and just want git-shell to allow the standard services without giving any custom command. Isn't the current message meant to be that? Just removing the hint: line would be enough to leave me happy with it. And for the remaining 20% of those who do not like the canned message but still do not need any custom command, I think it is way suboptimal to force them to create git-shell-commands directory for 47 users his host gives git-shell access to, and copy the help script to all of them, only to get a customized message. Isn't that a criticism of the git-shell-commands facility in general? If it is common to have a lot of users with distinct home directories but all with git-shell as their login shell, then the git-shell-commands should not go in their home directory to begin with, no? I think sharing a home directory is fine and the normal thing to do with such a restricted account, fwiw, so I am not the one to guess what people who do something different would find most useful. Maybe I am not the right person to have proposed this patch in the first place --- I saw something that looked wrong and proposed what I thought was a reasonable fix, but I am not actively depending on git-shell myself, so... *shrug* Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] shell: pay attention to exit status from 'help' command
Junio C Hamano wrote: Jonathan Nieder jrnie...@gmail.com writes: +To disable interactive logins, displaying a greeting instead: ++ + +$ chsh -s /usr/bin/git-shell +$ mkdir $HOME/git-shell-commands +$ cat $HOME/git-shell-commands/help \EOF +#!/bin/sh +printf '%s\n' Hi $USER! You've successfully authenticated, but I do not Where in the sshd to git-shell exec chain is $USER variable set for the user? Just being curious if this is the simplest but one of the more robust ways to get the user's name. That's a good question. environment= in an authorized_keys file is obsolete, so USER generally represents the actual logged in user. That means the main way to base behavior on private key (letting one system user represent multiple people) is a gitolite-style command= wrapper that checks SSH_ORIGINAL_COMMAND. In that setup, there is no reason to forward simple no-args are you there? requests to the git-shell, so we can forget about it here. So by the time we get to git-shell, most likely either A) this is a generic system user, with a username like git, and the above example would insult the client with Hi git! or Hi project-x-git! or B) each person has a separate account on the system, perhaps to help the admin to set filesystem permissions based on users and groups, and the above would address the user by her normal name. 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] shell: allow 'help' command to disable interactive shell
Junio C Hamano wrote: The purpose of the directory is to keep custom commands that are allowed. If the site administrator does not want any command, it would be more natural to expect that the way to disable them would be _not_ to have that directory which is a collection of allowed commands. Adding that directory and add a help that exits with non-zero feels quite a roundabout and counter-intuitive way, no? I think it comes down to the reason the site admin doesn't want to allow interactive logins. That reason seems to be mostly that presenting a git prompt at which you can only ask for help or exit is a bit confusing and pointless. I have sympathy for that, which is why I looked for a way for the admin to ask to avoid the prompt altogether in that case. I do not think the reason is because I don't want a git-shell-commands directory. I think it's good to have basically one kind of setup instead of significantly different ones with and without that special directory --- and it means that starting from a setup like this, one can easily drop in additional commands like set-head or create-repo without changing anything basic. It's making the admin's later life easier. Maybe a better test than help exits with special exit code is there are no other custom commands than help. Would that be more sensible? From a make it possible to emulate gitolite point of view, that doesn't permit disabling the interactive mode when there are other commands available, so my hunch is that it wouldn't. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] git-bisect.txt: clarify that reset quits bisect
Michael J Gruber wrote: --- a/Documentation/git-bisect.txt +++ b/Documentation/git-bisect.txt [...] @@ -284,6 +284,7 @@ EXAMPLES $ git bisect start HEAD v1.2 -- # HEAD is bad, v1.2 is good $ git bisect run make# make builds the app +$ git bisect reset # quit the bisect session I had forgotten that git bisect run ends in a bisecting state. Good call. Reviewed-by: Jonathan Nieder jrnie...@gmail.com 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 05/12] sequencer.c: recognize (cherry picked from ... as part of s-o-b footer
Brandon Casey wrote:
On 2/12/2013 11:13 AM, Junio C Hamano wrote:
Brandon Casey draf...@gmail.com writes:
+static int is_cherry_picked_from_line(const char *buf, int len)
+{
+ /*
+* We only care that it looks roughly like (cherry picked from ...)
+*/
+ return len strlen(cherry_picked_prefix) + 1
+ !prefixcmp(buf, cherry_picked_prefix) buf[len - 1] == ')';
+}
Does the first is it longer than the prefix? check matter? If it
is not, prefixcmp() would not match anyway, no?
Probably not in practice, but technically we should only be accessing
len characters in buf even though buf may be longer than len.
Yep. Technically the buf[len - 1] == ')' check is enough to avoid
false positives, but if it and the 'len' check were dropped then this
would be checking that buf is a (cherry-picked from line instead of
checking that its first 'len' bytes are one.
So it's just paranoid futureproofing. In the long term, it would be
nice to drop the number of bytes to ignore at the end argument to
append_signoff to avoid having to think about this kind of thing.
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/FYI v4 13/12] fixup! t/t3511: add some tests of 'cherry-pick -s' functionality
Brandon Casey wrote: I'm not sure we should apply this though. I'm leaning towards saying that the 'cherry-pick -s' behavior with respect to a commit with an empty message body should be undefined. If we want it to be undefined then we probably shouldn't introduce a test which would have the effect of defining it. Maybe it would make sense to just check that cherry-pick doesn't segfault in this case? That is, compute the output but don't compare it to expected output, as in: test_expect_success 'adding signoff to empty message does something sane' ' git reset --hard HEAD^ git cherry-pick --allow-empty-message -s empty-branch git show --pretty=format:%B -s empty-branch actual # sign-off is included *somewhere* grep ^Signed-off-by:.*\$ actual ' Alternatively, if there are only a few sane behaviors, a test can check for all of them and pass as long as git follows one. I haven't thought carefully enough about this example to suggest doing that. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/12] unify appending of sob
Brandon Casey wrote: Round 4. Yay. I think this is cooked now and a good foundation for later changes on top. For what it's worth, with or without the two tweaks Junio suggested (simplifying (cherry picked from detection, deferring introduction of no_dup_sob variable until it is used), 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/3] Documentation/Makefile: fix spaces around assignments
Hi, John Keeping wrote: [Subject: [PATCH 1/3] Documentation/Makefile: fix spaces around assignments] It's not so much fix spaces as use consistent spacing, no? Aside from that nit, looks like a sensible no-op to me, so Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] Documentation/Makefile: move infodir to be with other '*dir's
John Keeping wrote: Signed-off-by: John Keeping j...@keeping.me.uk [...] --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -81,6 +81,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT)) prefix ?= $(HOME) bindir ?= $(prefix)/bin htmldir ?= $(prefix)/share/doc/git-doc +infodir ?= $(prefix)/share/info pdfdir ?= $(prefix)/share/doc/git-doc mandir ?= $(prefix)/share/man man1dir = $(mandir)/man1 @@ -98,7 +99,6 @@ RM ?= rm -f MAN_REPO = ../../git-manpages HTML_REPO = ../../git-htmldocs -infodir ?= $(prefix)/share/info MAKEINFO = makeinfo Is this another stylefix or is there a functional reason for this change? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Fix installation paths with make install-doc
John Keeping wrote:
Documentation/Makefile: fix inherited {html,info,man}dir
This doesn't seem to have hit the list.
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 0/3] Fix installation paths with make install-doc
John Keeping wrote:
On Tue, Feb 12, 2013 at 02:25:08PM -0800, Jonathan Nieder wrote:
John Keeping wrote:
Documentation/Makefile: fix inherited {html,info,man}dir
This doesn't seem to have hit the list.
Hmm... it made it to gmane:
http://article.gmane.org/gmane.comp.version-control.git/216188
So it did. Sorry for the noise --- I'll grab it from there.
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] Makefile: do not export mandir/htmldir/infodir
Junio C Hamano wrote:
These are defined in the main Makefile to be funny values that are
optionally relative to an unspecified location that is determined at
runtime.
[...]
A longer term fix is to introduce runtime_{man,html,info}dir variables
to hold these funny values, and make {man,html,info}dir variables
to have real paths whose default values begin with $(prefix), but
as a first step, stop exporting them from the top-level Makefile.
Makes sense.
Reported-by: John Keeping j...@keeping.me.uk
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
jn/shell-disable-interactive (Re: What's cooking in git.git (Feb 2013, #05; Tue, 12))
Junio C Hamano wrote: * jn/shell-disable-interactive (2013-02-11) 2 commits - shell: pay attention to exit status from 'help' command - shell doc: emphasize purpose and security model Will merge to 'next'. Please hold off on merging the second patch. I'd like to reroll renaming the command to 'no-interactive-login' or some such, which would be less disruptive to existing setups and should be easier to explain. 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] Makefile: don't run rm without any files
Junio C Hamano wrote: I amended the log message like so: commit bd9df384b16077337fffe9836c9255976b0e7b91 Author: Matt Kraai matt.kr...@amo.abbott.com Date: Wed Feb 13 07:57:48 2013 -0800 Makefile: don't run rm without any files When COMPUTE_HEADER_DEPENDENCIES is set to auto and the compiler does not support it, $(dep_dirs) becomes empty. make clean runs rm -rf $(dep_dirs), which fails in such a case. To pedantic, that only fails on some platforms. The autoconf manual explains: It is not portable to invoke rm without options or operands. On the other hand, Posix now requires rm -f to silently succeed when there are no operands (useful for constructs like rm -rf $filelist without first checking if ‘$filelist’ was empty). But this was not always portable; at least NetBSD rm built before 2008 would fail with a diagnostic. Anyway, looks like a good fix. 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 2/3] contrib/subtree/t: Added tests for .gitsubtree support
Hi Paul,
Paul Campbell wrote:
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -465,4 +465,34 @@ test_expect_success 'verify one file change per commit' '
[...]
+test_expect_success 'change in subtree is pushed okay' '
+cd copy0 create new_file git commit -mAdded new_file
+cd .. git subtree push --prefix=copy0 21 | \
If it possible to restrict the chdirs to subshells, that can make the
test more resiliant to early failures without breaking later tests.
That is:
(
cd copy0
create new_file
test_tick
git commit -m add new_file
)
git subtree push --prefix=copy0 output 21
grep ... output
+grep
^\s\{3\}[0-9a-f]\{7\}\.\.[0-9a-f]\{7\}\s\s[0-9a-f]\{40\}\s-\ssub1$
This might not be portable if I understand
Documentation/CodingGuidelines correctly.
[...]
+(grep ^copy3 . sub2$ .gitsubtree die || true)
! grep ^copy3 . sub2\$ .gitsubtree
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/3] pkt-line: teach packet_get_line a no-op mode
Jeff King wrote: --- a/pkt-line.c +++ b/pkt-line.c @@ -234,9 +234,10 @@ int packet_get_line(struct strbuf *out, *src_len -= 4; len -= 4; - strbuf_add(out, *src_buf, len); + if (out) + strbuf_add(out, *src_buf, len); + packet_trace(*src_buf, len, 0); *src_buf += len; *src_len -= len; - packet_trace(out-buf, out-len, 0); return len; For what it's worth, Reviewed-by: Jonathan Nieder jrnie...@gmail.com The above code has a structure of prepare to return(buf, len); trace(buf, len); discard used part of buf; return; which is nice and readable. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines
Jeff King wrote: --- a/remote-curl.c +++ b/remote-curl.c [...] @@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char *service) [...] - strbuf_reset(buffer); - while (packet_get_line(buffer, last-buf, last-len) 0) - strbuf_reset(buffer); + if (read_packets_until_flush(last-buf, last-len) 0) Style nit: this made me wonder What would it mean if read_packets_until_flush() 0? Since the convention for this function is 0 for success, I would personally find if (read_packets_until_flush(...)) handle error; easier to read. + die(smart-http metadata lines are invalid at %s, + refs_url); Especially given that other clients would be likely to run into trouble in the same situation, as long as this cooks in next for a suitable amount of time to catch bad servers, it looks like a good idea. Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] remote-curl: sanity check ref advertisement from server
Jeff King wrote: If the smart HTTP response from the server is truncated for any reason, we will get an incomplete ref advertisement. If we then feed this incomplete list to fetch-pack, one of a few things may happen: 1. If the truncation is in a packet header, fetch-pack will notice the bogus line and complain. 2. If the truncation is inside a packet, fetch-pack will keep waiting for us to send the rest of the packet, which we never will. Mostly harmless since the operator could hit ^C, but still unpleasant. [...] This fortunately doesn't happen in the normal fetching workflow, because git-fetch first uses the list command, which feeds the refs to get_remote_heads, which does notice the error. However, you can trigger it by sending a direct fetch to the remote-curl helper. Ah. Would a test for this make sense? [...] --- a/remote-curl.c +++ b/remote-curl.c [...] @@ -174,6 +183,9 @@ static struct discovery* discover_refs(const char *service) die(smart-http metadata lines are invalid at %s, refs_url); + if (verify_ref_advertisement(last-buf, last-len) 0) + die(ref advertisement is invalid at %s, refs_url); Won't this error out with protocol error: bad line length character: ERR instead of the current more helpful behavior for ERR lines? Same stylistic comment about what would it mean for the return value to be positive? as in patch 2/3. Aside from those two details, the idea looks sane, though. Good catch, and thanks for a pleasant read. Good night, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] t7800: modernize tests
David Aguilar wrote:
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -10,29 +10,11 @@ Testing basic diff tool invocation
[...]
-restore_test_defaults()
-{
- # Restores the test defaults used by several tests
- remove_config_vars
- unset GIT_DIFF_TOOL
- unset GIT_DIFFTOOL_PROMPT
- unset GIT_DIFFTOOL_NO_PROMPT
- git config diff.tool test-tool
- git config difftool.test-tool.cmd 'cat $LOCAL'
- git config difftool.bogus-tool.cmd false
Yay. :)
[...]
# Ensures that git-difftool ignores bogus --tool values
test_expect_success PERL 'difftool ignores bad --tool values' '
diff=$(git difftool --no-prompt --tool=bad-tool branch)
test $? = 1
- test $diff =
+ test -z $diff
'
Not about this patch: if I add more commands before that diff,
their exit status would be ignored. Could this be made more resilient
using test_expect_code? Something like
test_expect_code 1 git diff --no-prompt --tool=bad-tool branch actual
expect
test_cmp expect actual
[...]
# Specify the diff tool using $GIT_DIFF_TOOL
test_expect_success PERL 'GIT_DIFF_TOOL variable' '
- test_might_fail git config --unset diff.tool
+ difftool_test_setup
+ git config --unset diff.tool
+
GIT_DIFF_TOOL=test-tool
export GIT_DIFF_TOOL
diff=$(git difftool --no-prompt branch)
test $diff = branch
-
- restore_test_defaults
+ sane_unset GIT_DIFF_TOOL
If this test fails, GIT_DIFF_TOOL would remain set which could take
down later tests, too. Could it be set in a subprocess (e.g., a
subshell) to avoid that?
difftool_test_setup
git config --unset diff.tool
echo branch expect
GIT_DIFF_TOOL=test-tool git difftool --no-prompt branch actual
test_cmp expect actual
[...]
test_expect_success PERL 'GIT_DIFF_TOOL overrides' '
- git config diff.tool bogus-tool
- git config merge.tool bogus-tool
-
+ difftool_test_setup
+ test_config diff.tool bogus-tool
+ test_config merge.tool bogus-tool
GIT_DIFF_TOOL=test-tool
export GIT_DIFF_TOOL
diff=$(git difftool --no-prompt branch)
Likewise.
[...]
GIT_DIFF_TOOL=bogus-tool
export GIT_DIFF_TOOL
diff=$(git difftool --no-prompt --tool=test-tool branch)
Likewise.
[...]
test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' '
+ difftool_test_setup
GIT_DIFFTOOL_NO_PROMPT=true
export GIT_DIFFTOOL_NO_PROMPT
diff=$(git difftool branch)
Likewise.
[...]
test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' '
- git config difftool.prompt false
+ difftool_test_setup
+ test_config difftool.prompt false
GIT_DIFFTOOL_PROMPT=true
export GIT_DIFFTOOL_PROMPT
prompt=$(echo | git difftool branch | tail -1)
Likewise. This one loses the exit status from 'git difftool',
which could be avoided by writing to temporary files:
echo input
GIT_DIFFTOOL_PROMPT=true git difftool branch input output
prompt=$(tail -1 output)
[...]
test_expect_success PERL 'difftool last flag wins' '
+ difftool_test_setup
diff=$(git difftool --prompt --no-prompt branch)
test $diff = branch
- restore_test_defaults
-
prompt=$(echo | git difftool --no-prompt --prompt branch | tail -1)
[...]
Likewise.
Thanks for cleaning up, and sorry I don't have anything more
substantial to offer.
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] t7800: defaults is no longer a builtin tool name
David Aguilar wrote: t7800 tests that configured commands can override builtins, but this test was not adjusted when the defaults file was removed because the test continued to pass. Adjust the test to use the everlasting vimdiff Heh. :) tool name instead of defaults so that it correctly tests against a tool that is known by mergetool--lib. Makes sense. Thanks for a pleasant read. Good night, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] contrib/subtree/t: Added tests for .gitsubtree support
Paul Campbell wrote: Is there was a better way to verify that the push operation succeeds then grepping for a SHA1? IIRC then when a push fails, it will exit with nonzero status (so the usual -chaining would propagate the error). Alternatively, one can fetch, ls-remote, or enter the target repo and use history inspection tools to check that the result is as expected. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] contrib/subtree/t: Added tests for .gitsubtree support
Paul Campbell wrote: Here's the updated version of the tests: Just a few more nits: --- a/contrib/subtree/t/t7900-subtree.sh +++ b/contrib/subtree/t/t7900-subtree.sh @@ -465,4 +465,37 @@ test_expect_success 'verify one file change per commit' ' [...] +test_expect_success 'change in subtree is pushed okay' ' + (cd copy0 create new_file git commit -mAdded new_file) Style: this would be easier to read with each command on a separate line, like so: ( cd copy0 create new_file test_tick git commit -m Add new_file ) [...] +test_expect_success 'pull into subtree okay' ' + git subtree add --prefix=copy1 sub1 + git subtree add --prefix=copy2 sub1 + (cd copy1 create new_file_in_copy1 git commit -mAdded new_file_in_copy1) Likewise (and as a nice side-benefit, it would avoid a long line that mailers like to wrap). + git subtree push --prefix=copy1 + git subtree pull --prefix=copy2 | grep ^ create mode 100644 copy2/new_file_in_copy1$ Likewise. More importantly, this forgets the exit status from git subtree pull, so if it were to segfault after writing appropriate output, the test wouldn't notice. How about: git subtree pull --prefix=copy2 output grep ^ create mode 100644 copy2/new_file_in_copy1\$ output Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] remote-curl: verify smart-http metadata lines
Jeff King wrote: On Sun, Feb 17, 2013 at 02:49:39AM -0800, Jonathan Nieder wrote: Jeff King wrote: --- a/remote-curl.c [...] + if (read_packets_until_flush(last-buf, last-len) 0) Style nit: this made me wonder What would it mean if read_packets_until_flush() 0? [...] My intent was that it followed the error convention of negative is error, 0 is success, and positive is not used, but reserved for future use. From a maintainability perspective, that kind of contract would be dangerous, since some *other* caller could arrive and use the function without a 0 without knowing it is doing anything wrong. When new return values appear, the function should be renamed to help the patch author and reviewers remember to check all callers. That is, from the point of view of maintainability, there is no distinction between if (read_packets_until_... 0) and if (read_packets_until_...) and either form is fine. My comment was just to say the 0 forced me to pause a moment and check out the implementation. This is basically a stylistic thing and if you prefer to keep the 0, that's fine with me. If an implementation is producing bogus packet lines and expecting us not to complain, it really needs to be fixed. Agreed completely. Thanks again for the 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 3/3] remote-curl: sanity check ref advertisement from server
Jeff King wrote:
On Sun, Feb 17, 2013 at 03:05:34AM -0800, Jonathan Nieder wrote:
Jeff King wrote:
+ if (verify_ref_advertisement(last-buf, last-len) 0)
+ die(ref advertisement is invalid at %s, refs_url);
Won't this error out with
protocol error: bad line length character: ERR
instead of the current more helpful behavior for ERR lines?
I don't think so. Don't ERR lines appear inside their own packets?
Yes, I misread get_remote_heads for some reason. Thanks for checking.
[...]
The one thing we do also check, though, is that we end with a flush
packet. So depending on what servers produce, it may mean we trigger
this complaint instead of passing the ERR along to fetch-pack.
Rather than doing this fake syntactic verification, I wonder if we
should simply call get_remote_heads, which does a more thorough check
I'm not sure whether servers are expected to send a flush after an
ERR packet. The only codepath I know of in git itself that sends
such packets is git-daemon, which does not flush after the error (but
is not used in the stateless-rpc case). http-backend uses HTTP error
codes for its errors.
If I am reading get_remote_heads correctly, calling it with the
following tweak should work ok. The extra thread is just to feed a
string into a fd-based interface and could be avoided for list, too,
if it costs too much.
diff --git i/connect.c w/connect.c
index 49e56ba3..55ee7de7 100644
--- i/connect.c
+++ w/connect.c
@@ -68,7 +68,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
{
int got_at_least_one_head = 0;
- *list = NULL;
+ if (list)
+ *list = NULL;
for (;;) {
struct ref *ref;
unsigned char old_sha1[20];
@@ -92,6 +93,9 @@ struct ref **get_remote_heads(int in, struct ref **list,
die(protocol error: expected sha/ref, got '%s',
buffer);
name = buffer + 41;
+ if (!list)
+ continue;
+
name_len = strlen(name);
if (len != name_len + 41) {
free(server_capabilities);
--
To unsubscribe from this list: send the line unsubscribe 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/9] user-manual: Use 'remote add' to setup push URLs
Junio C Hamano wrote: W. Trevor King wk...@tremily.us writes: There is no need to use here documents to setup this configuration. It is easier, less confusing, and more robust to use `git remote add` directly. [...] This looks like a good 'maint' material that can be applied straight away there in preparation for 1.8.1.4 to me; reviewers watching from the sideline, please stop me if you see issues. Agreed --- this looks good. [...] As the additional remote.public-repo.fetch line hints, this does more than lets you do the same push with just [lazily]; it also starts pretending to have run a fetch from there immediately after you pushed and update the remote tracking branches. I couldn't decide if it is a good idea to point it out in this point of the flow as well, or it is too much detail that is not exactly relevant while teaching git push. I tend to think it would be the latter. I think it's possible to improve the text here to hint that there's more to learn (maybe a forward-reference to a section about the remotes/* hierarchy) without getting lost in the details. But that problem was already there, and I don't think it should block this improvement. Thanks. Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 03/10] pkt-line: clean up gentle reading function
Jeff King wrote: Originally we had a single function for reading packetized data: packet_read_line. Commit 46284dd grew a more gentle form that would return an error instead of dying upon reading a truncated input stream. However: In other words: Based on the names of two functions packet_read and packet_read_line, it is not obvious which to use and what the ramifications of that choice are. Rename packet_read to packet_read_line_gently and add a comment explaining that the latter is a gentler form that returns an error instead of dying upon reading a truncated input stream. While at it: * Rename the internal argument triggering the gentle mode to gentle instead of return_line_fail. * Drop the redundant return_line_fail in checks like if (return_line_fail ret 0). safe_read() never returns an error when !gentle. No functional change intended. FWIW, the patch itself is Reviewed-by: Jonathan Nieder jrnie...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/
Jeff King wrote: Originally packets were used just for the line-oriented ref advertisement and negotiation. These days, we also stuff packfiles and sidebands into them, and they do not necessarily represent a line. Drop the _line suffix, as it is not informative and makes the function names quite long (especially as we add _gently and other variants). Signed-off-by: Jeff King p...@peff.net --- Again, this is a taste issue. Can be optional. In combination with patch 3, this changes the meaning of packet_read() without changing its signature, which could make other patches cherry-picked on top change behavior in unpredictable ways. :( So I'd be all for this if the signature changes (for example to put the fd at the end or something), but not so if not. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation
Jeff King wrote:
The packet_read function reads from a descriptor.
Ah, so this introduces a new analagous helper that reads from
a strbuf, to avoid the copy-from-async-procedure hack?
[...]
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -103,12 +103,26 @@ static int safe_read(int fd, void *buffer, unsigned
size, int gently)
strbuf_add(buf, buffer, n);
}
-static int safe_read(int fd, void *buffer, unsigned size, int gently)
+static int get_packet_data(int fd, char **src_buf, size_t *src_size,
+void *dst, unsigned size, int gently)
{
- ssize_t ret = read_in_full(fd, buffer, size);
- if (ret 0)
- die_errno(read error);
- else if (ret size) {
+ ssize_t ret;
+
+ /* Read up to size bytes from our source, whatever it is. */
+ if (src_buf) {
+ ret = size *src_size ? size : *src_size;
+ memcpy(dst, *src_buf, ret);
+ *src_buf += size;
+ *src_size -= size;
+ }
+ else {
Style: git cuddles its elses.
assert(src_buf ? fd 0 : fd = 0);
if (src_buf) {
...
} else {
...
}
+ ret = read_in_full(fd, dst, size);
+ if (ret 0)
+ die_errno(read error);
This is noisy about upstream pipe gone missing, which makes sense
since this is transport-related. Maybe that deserves a comment.
[...]
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -138,28 +138,29 @@ static struct discovery* discover_refs(const char
*service)
if (maybe_smart
(5 = last-len last-buf[4] == '#')
!strbuf_cmp(exp, type)) {
+ char line[1000];
+ int len;
+
/*
* smart HTTP response; validate that the service
* pkt-line matches our request.
*/
- if (packet_get_line(buffer, last-buf, last-len) = 0)
- die(%s has invalid packet header, refs_url);
- if (buffer.len buffer.buf[buffer.len - 1] == '\n')
- strbuf_setlen(buffer, buffer.len - 1);
+ len = packet_read_from_buf(line, sizeof(line), last-buf,
last-len);
+ if (len line[len - 1] == '\n')
+ len--;
Was anything guaranteeing that buffer.len 1000 before this change?
The rest looks good from a quick glance.
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 08/10] remote-curl: pass buffer straight to get_remote_heads
Jeff King wrote: I don't know that this code was hurting anything, but it has always struck me as ugly and a possible source of error. And now it's gone. Heh. Belongs in the commit message, presumably. I don't think the async procedure was very harmful, but it's nice to avoid the cost of a new thread and some copying. remote-curl.c | 26 ++ 1 file changed, 2 insertions(+), 24 deletions(-) Nice. The patch looks sane. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 10/10] remote-curl: always parse incoming refs
Jeff King wrote: remote-curl.c | 23 +-- 1 file changed, 13 insertions(+), 10 deletions(-) I like. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation
Jeff King wrote: But is it noisy about a missing pipe? We do not get EPIPE for reading. Ah, that makes more sense. [...] + len = packet_read_from_buf(line, sizeof(line), last-buf, last-len); + if (len line[len - 1] == '\n') + len--; Was anything guaranteeing that buffer.len 1000 before this change? No. That's discussed in point (3) of the implications in the commit message. Thanks. Sorry I missed it the first time. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 05/10] pkt-line: rename s/packet_read_line/packet_read/
Jeff King wrote: On Mon, Feb 18, 2013 at 02:19:15AM -0800, Jonathan Nieder wrote: In combination with patch 3, this changes the meaning of packet_read() without changing its signature, which could make other patches cherry-picked on top change behavior in unpredictable ways. :( So I'd be all for this if the signature changes (for example to put the fd at the end or something), but not so if not. True. Though packet_read has only existed since last June, only had one callsite (which would now conflict, since I'm touching it in this series), and has no new calls in origin..origin/pu. So it's relatively low risk for such a problem. I don't know how careful we want to be. I was unclear. What I am worried about is that someone using a version of git without this patch will try some yet-to-be-written patch using packet_read from the mailing list and not notice that they are using the wrong function. For example, if someone is using 1.7.12.y or 1.8.1.y and wants to try a patch from after the above, they would get subtly different and wrong results. The rule change the name or signature when breaking the ABI of a global function is easy to remember and follow. I think we want not to have to be careful at all, and such rules can help with that. :) 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] shell-prompt: clean up nested if-then
Hi Martin,
Martin Erik Werner wrote:
Minor clean up of if-then nesting in checks for environment variables
and config options. No functional changes.
Yeah, the nesting was getting a little deep. Thanks for the cleanup.
May we have your sign-off?
Once this is signed off,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Patch left unsnipped for reference.
---
contrib/completion/git-prompt.sh | 27 +--
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/contrib/completion/git-prompt.sh
b/contrib/completion/git-prompt.sh
index 9b2eec2..e29694d 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -320,26 +320,25 @@ __git_ps1 ()
b=GIT_DIR!
fi
elif [ true = $(git rev-parse --is-inside-work-tree
2/dev/null) ]; then
- if [ -n ${GIT_PS1_SHOWDIRTYSTATE-} ]; then
- if [ $(git config --bool bash.showDirtyState)
!= false ]; then
- git diff --no-ext-diff --quiet
--exit-code || w=*
- if git rev-parse --quiet --verify HEAD
/dev/null; then
- git diff-index --cached --quiet
HEAD -- || i=+
- else
- i=#
- fi
+ if test -n ${GIT_PS1_SHOWDIRTYSTATE-}
+test $(git config --bool bash.showDirtyState) !=
false
+ then
+ git diff --no-ext-diff --quiet --exit-code ||
w=*
+ if git rev-parse --quiet --verify HEAD
/dev/null; then
+ git diff-index --cached --quiet HEAD --
|| i=+
+ else
+ i=#
fi
fi
if [ -n ${GIT_PS1_SHOWSTASHSTATE-} ]; then
git rev-parse --verify refs/stash /dev/null
21 s=$
fi
- if [ -n ${GIT_PS1_SHOWUNTRACKEDFILES-} ]; then
- if [ $(git config --bool
bash.showUntrackedFiles) != false ]; then
- if [ -n $(git ls-files --others
--exclude-standard) ]; then
- u=%
- fi
- fi
+ if test -n ${GIT_PS1_SHOWUNTRACKEDFILES-}
+test $(git config --bool bash.showUntrackedFiles)
!= false
+test -n $(git ls-files --others --exclude-standard)
+ then
+ u=%
fi
if [ -n ${GIT_PS1_SHOWUPSTREAM-} ]; then
--
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Google Summer of Code 2013 (GSoC13)
Hi, Jeff King wrote: I will do it again, if people feel strongly about Git being a part of it. However, I have gotten a little soured on the GSoC experience. Not because of anything Google has done; it's a good idea, and I think they do a fine of administering the program. But I have noticed that the work that comes out of GSoC the last few years has quite often not been merged, or not made a big impact in the codebase, and nor have the participants necessarily stuck around. I think that if we can commit enough time to mentor well it's worthwhile. Even such a negative result is useful, since it can teach us how good or poor we are at bringing new contributors in and what parts of that process need more work. That said, I won't have time to mentor a project on my own. It takes a lot of time (or luck, to get the student that doesn't need mentoring). I'd be happy to help on a project with 1 or 2 co-mentors. Some potential projects (unfiltered --- please take them with a grain of salt): - cross-compilable git - incorporation of the cgit web interface, or formalizing a subset of libgit.a to export as a stable library to it - merging the gitweb-caching fork - moving forward on a project that was the subject of a previous gsoc project: line-level logging, rebase --interactive on top of sequencer, usable svn remote helper - collapsable --first-parent history in gitk http://bugs.debian.org/61 - drag-and-drop cherry-pick in gitk - a sub-library of code shared with libgit2 (might be hard because our notions of strings are different :(). - assimilating the distro builds: make deb-pkg, make rpm-pkg, etc along the same lines as the linux kernel's script/package/, to help people get recent git installed when they want it - please cherry-pick this before testing that notes for less scary bisecting - collaborative notes editing: fix the default notes refspec, make sure the notes pull workflow works well and is documented well, offer an easy way to hide private notes after the fact without disrupting public history 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: Google Summer of Code 2013 (GSoC13)
Ramkumar Ramachandra wrote: Take what I'm about to say with a pinch of salt, because I've never mentored. Mentors often don't provide much technical assistance: students should just post to the list with queries, or ask on #git-devel. Mentors serve a different purpose; their primary responsibility, in my opinion, is to teach the student a sustainable productive workflow. I basically agree. One of the most important jobs of mentors is to make sure there are people available to provide prompt technical assistance, hopefully before the project begins. [...] - using gdb efficiently to quickly understand parts? Oh, dear. I hope not. ;-) 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
Potential GSoC13 projects (Re: Google Summer of Code 2013 (GSoC13))
Ramkumar Ramachandra wrote: Jonathan Nieder wrote: - cross-compilable git Why, exactly? Git for embedded devices? My personal motivation would be building Git for Windows while spending as little time on Windows as possible. People deploying git to 32-bit x86, 64-bit x86, and ARM (think ARM laptops) might also find it handy. - incorporation of the cgit web interface, or formalizing a subset of libgit.a to export as a stable library to it I didn't understand this: you want cgit in-tree? Yes, or a stable API that cgit out-of-tree can use. - moving forward on a project that was the subject of a previous gsoc project: line-level logging, rebase --interactive on top of sequencer, usable svn remote helper I can't see a roadmap for gradually phasing out `rebase -i` as more and more of its functionality is built into the sequencer. It's a break-the-world thing. rebase -i --experimental. [...] For usable svn remote helper, the major TODO is a git - svn bridge. There are other major TODOs, too. [...] - drag-and-drop cherry-pick in gitk You expect someone to write Tcl/Tk today? Sure, why not? Tcl is not actually too unpleasant of a language. Maybe it has a prerequisite, though: - modular gitk (splitting gitk into digestible pieces) [...] - assimilating the distro builds: [...] Overkill. My itch is that it would let me send packaging patches to the list and get the usual high-quality feedback. Oh well. ;-) [...] - collaborative notes editing: fix the default notes refspec, make sure the notes pull workflow works well and is documented well, offer an easy way to hide private notes after the fact without disrupting public history I personally don't care for notes much, because I can't see practical usecases. Are you sure that's not because of the poor current state of collaborative notes editing? Some example use cases: - marking regressions discovered later, to warn people bisecting or cherry-picking - matching up to corresponding commits in another repository - link to corresponding mailing list discussion, blog post, or related patches - a wiki-like document storing review comments - marking which CVE this fixes, once the CVE number has been allocated - a tour of the project for new contributors, using explanatory notes that end with a mention the next commit to look at I'm not married to the current implementation, but I think the basic idea of git notes is a promising feature that could use some polish. 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: Google Summer of Code 2013 (GSoC13)
Jeff King wrote: On Mon, Feb 18, 2013 at 11:34:24AM -0800, Jonathan Nieder wrote: Some potential projects (unfiltered --- please take them with a grain of salt): [...] - collaborative notes editing: fix the default notes refspec, make sure the notes pull workflow works well and is documented well, offer an easy way to hide private notes after the fact without disrupting public history I know you said a grain of salt, so please don't feel like I'm beating up on your idea. I'm picking this one because I think it has some characteristics of projects that have not gone well in the past, so it's a good illustrative example. IMHO, this is the type of project that is likely to fail, because most of the work is not technical at all, but political. Changing the default refspecs is a few lines of code. But the hard part is figuring out where they should go, the implications of doing so, and how people are going to react. I think I agree, if by likely to fail you mean easy to underestimate the difficulty of. I actually think it would be a pretty good summer student project, for a few related reasons: * Years of evidence show it is a hard problem. It would be a good notch in the belt of whoever takes the project on. * It does not require a deep understanding of git internals. A good familiarity with the git user interface, on the other hand, would be essential, but I hope that is becoming more common among students these days. * It requires good taste and design sense, which are something it would be nice to cultivate and encourage. * The change is necessary and the satisfaction of helping a student through the process might be enough to finally get it done. * If an amazing candidate finishes the make collaboration possible task early, there's plenty of valuable, interesting, and technically complicated follow-on work regarding the related share some notes while hiding others to fill the rest of the summer. The code change for the most basic subset of make collaboration possible would presumably be a changed refspec, some documentation, and some tests. On top of that there is presumably some automagic incorporation of upstream notes to be cooked into git pull. Some better conflict-resolution magic. Example scripts to generate notes. Support for the format-patch / am workflow. gitweb support for showing notes. It's a good example of when it's useful to not be afraid of failing to please everybody and just get something done. I also can't think of any examples of such technically straightforward student projects being tried before. 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] shell-prompt: clean up nested if-then
Martin Erik Werner wrote: Minor clean up of if-then nesting in checks for environment variables and config options. No functional changes. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- contrib/completion/git-prompt.sh | 27 +-- 1 file changed, 13 insertions(+), 14 deletions(-) Reviewed-by: Jonathan Nieder jrnie...@gmail.com Thanks for the quick turnaround. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] t/t7502: compare entire commit message with what was expected
Brandon Casey wrote:
So, let's use the --no-status option to 'git commit' which will cause
git to refrain from appending the lines of instructional text to the
commit message. This will allow the entire resulting commit message to
be compared against the expected value.
The downside (not a new problem, but a downside nonetheless) is that
it means the test doesn't demonstrate what --cleanup=verbatim --status
will do.
How about something like this?
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/t/t7502-commit.sh w/t/t7502-commit.sh
index cbd7a459..64162fce 100755
--- i/t/t7502-commit.sh
+++ w/t/t7502-commit.sh
@@ -180,15 +180,37 @@ test_expect_success 'verbose respects diff config' '
test_expect_success 'cleanup commit messages (verbatim option,-t)' '
echo negative
- { echo;echo # text;echo; } expect
- git commit --cleanup=verbatim -t expect -a
- git cat-file -p HEAD |sed -e 1,/^\$/d |head -n 3 actual
+ {
+ echo
+ echo # text
+ echo
+ } template
+ {
+ cat template
+ cat -\EOF
+
+ # Please enter the commit message for your changes. Lines
starting
+ # with '\''#'\'' will be kept; you may remove them yourself if
you want to.
+ # An empty message aborts the commit.
+ #
+ # Author:A U Thor aut...@example.com
+ #
+ EOF
+ git commit -a --dry-run
+ } expect
+ git commit --cleanup=verbatim -t template -a
+ git cat-file -p HEAD |sed -e 1,/^\$/d actual
test_cmp expect actual
'
test_expect_success 'cleanup commit messages (verbatim option,-F)' '
+ {
+ echo
+ echo # text
+ echo
+ } expect
echo negative
git commit --cleanup=verbatim -F expect -a
git cat-file -p HEAD |sed -e 1,/^\$/dactual
--
To unsubscribe from this list: send the line unsubscribe 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] t/t7502: compare entire commit message with what was expected
Jonathan Nieder wrote: +++ w/t/t7502-commit.sh [...] + # Please enter the commit message for your changes. Lines starting + # with '\''#'\'' will be kept; you may remove them yourself if you want to. + # An empty message aborts the commit. + # + # Author:A U Thor aut...@example.com + # + EOF + git commit -a --dry-run + } expect + git commit --cleanup=verbatim -t template -a - git cat-file -p HEAD |sed -e 1,/^\$/d |head -n 3 actual + git cat-file -p HEAD |sed -e 1,/^\$/d actual test_cmp expect actual Quick correction: this would use test_i18ncmp instead of test_cmp if it ends up being a good idea. -- To unsubscribe from this list: send the line unsubscribe 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] t7502: demonstrate breakage with a commit message with trailing newlines
Brandon Casey wrote:
This test attempts to verify that a commit message supplied to 'git
commit' via the -m switch was used in full as the commit message for a
commit when --cleanup=verbatim was used.
[...]
The test was able to complete successfully since internally, git appends
two newlines to each string supplied via the -m switch.
[...]
Mark this test as failing, since it is not handled correctly by git.
As described above, git appends two extra newlines to every string
supplied via -m.
Good catch. This is an old one, triggered by a combination of
v1.5.4-rc0~78^2~23 builtin-commit: resurrect behavior for multiple -m
options, 2007-11-11
and
v1.5.4-rc2~3^2 Allow selection of different cleanup modes for commit
messages, 2007-12-22
The patch makes sense and makes the test easier to read, so
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
(Patch left unsnipped for reference.)
Signed-off-by: Brandon Casey draf...@gmail.com
---
t/t7502-commit.sh | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 9040f8a..39e55f8 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -177,10 +177,18 @@ test_expect_success 'verbose respects diff config' '
git config --unset color.diff
'
+mesg_with_comment_and_newlines='
+# text
+
+'
+
+test_expect_success 'prepare file with comment line and trailing newlines' '
+ printf %s $mesg_with_comment_and_newlines expect
+'
+
test_expect_success 'cleanup commit messages (verbatim option,-t)' '
echo negative
- { echo;echo # text;echo; } expect
git commit --cleanup=verbatim --no-status -t expect -a
git cat-file -p HEAD |sed -e 1,/^\$/d actual
test_cmp expect actual
@@ -196,10 +204,10 @@ test_expect_success 'cleanup commit messages (verbatim
option,-F)' '
'
-test_expect_success 'cleanup commit messages (verbatim option,-m)' '
+test_expect_failure 'cleanup commit messages (verbatim option,-m)' '
echo negative
- git commit --cleanup=verbatim -m $(cat expect) -a
+ git commit --cleanup=verbatim -m $mesg_with_comment_and_newlines -a
git cat-file -p HEAD |sed -e 1,/^\$/dactual
test_cmp expect actual
--
--
To unsubscribe from this list: send the line unsubscribe 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] git-commit: only append a newline to -m mesg if necessary
Brandon Casey wrote:
Currently, git will append two newlines to every message supplied via
the -m switch. The purpose of this is to allow -m to be supplied
multiple times and have each supplied string become a paragraph in the
resulting commit message.
Normally, this does not cause a problem since any trailing newlines will
be removed by the cleanup operation. If cleanup=verbatim for example,
then the trailing newlines will not be removed and will survive into the
resulting commit message.
Instead, let's ensure that the string supplied to -m is newline terminated,
but only append a second newline when appending additional messages.
[...]
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -124,8 +124,10 @@ static int opt_parse_m(const struct option *opt, const
char *arg, int unset)
if (unset)
strbuf_setlen(buf, 0);
else {
+ if (buf-len)
+ strbuf_addch(buf, '\n');
strbuf_addstr(buf, arg);
- strbuf_addstr(buf, \n\n);
+ strbuf_complete_line(buf);
As long as 'message' always consists of complete lines, this will
append 'arg' as a new paragraph, as desired. And no other code path
touches 'message', so it always consists of complete lines.
Thanks for a clear patch and explanation.
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
(rest of patch kept unsnipped for reference)
}
return 0;
}
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 39e55f8..292bc08 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -204,7 +204,7 @@ test_expect_success 'cleanup commit messages (verbatim
option,-F)' '
'
-test_expect_failure 'cleanup commit messages (verbatim option,-m)' '
+test_expect_success 'cleanup commit messages (verbatim option,-m)' '
echo negative
git commit --cleanup=verbatim -m $mesg_with_comment_and_newlines -a
--
--
To unsubscribe from this list: send the line unsubscribe 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] Documentation/git-commit.txt: correct a few minor grammatical mistakes
Brandon Casey wrote: --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -174,10 +174,10 @@ OPTIONS --cleanup=mode:: This option sets how the commit message is cleaned up. The 'mode' can be one of 'verbatim', 'whitespace', 'strip', - and 'default'. The 'default' mode will strip leading and + or 'default'. The 'default' mode will strip leading and trailing empty lines and #commentary from the commit message - only if the message is to be edited. Otherwise only whitespace - removed. The 'verbatim' mode does not change message at all, + only if the message is to be edited. Otherwise only whitespace is + removed. The 'verbatim' mode does not change the message at all, 'whitespace' removes just leading/trailing whitespace lines and 'strip' removes both whitespace and commentary. The default can be changed by the 'commit.cleanup' configuration variable Yeah, the current text is a bit choppy. How about this? Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- i/Documentation/git-commit.txt +++ w/Documentation/git-commit.txt @@ -172,16 +172,25 @@ OPTIONS linkgit:git-commit-tree[1]. --cleanup=mode:: - This option sets how the commit message is cleaned up. - The 'mode' can be one of 'verbatim', 'whitespace', 'strip', - and 'default'. The 'default' mode will strip leading and - trailing empty lines and #commentary from the commit message - only if the message is to be edited. Otherwise only whitespace - removed. The 'verbatim' mode does not change message at all, - 'whitespace' removes just leading/trailing whitespace lines - and 'strip' removes both whitespace and commentary. The default - can be changed by the 'commit.cleanup' configuration variable - (see linkgit:git-config[1]). + This option determines how the supplied commit message should be + cleaned up before committing. The 'mode' can be `verbatim`, + `whitespace`, `strip`, or `default`. ++ +-- +default:: + Strip leading and trailing empty lines and #commentary from + the commit message only if the message is to be edited. + Otherwise only remove whitespace. +verbatim:: + Do not change the message at all. +whitespace:: + Remove only leading and trailing whitespace lines. +strip:: + Remove both whitespace and commentary. +-- ++ +The default can be changed using the 'commit.cleanup' configuration +variable (see linkgit:git-config[1]). -e:: --edit:: -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Google Summer of Code 2013 (GSoC13)
Ramkumar Ramachandra wrote: The short undiplomatic version of that is that our mentors suck (I'm not pointing fingers, but that's what I infer from failing projects). Hold on a second. I'm not remembering such a grim outcome with 100% failure from prior summers of code as you're describing. Before I start beating myself up, I guess I'd like a little more information --- is there some specific project or statistic that you're thinking of that brings you to that conclusion? [...] I propose that we have one thread for every proposal where we can all discuss the implementation outline- this will serve as authoritative source of information for students, and for picking mentors (the people who contribute most to the discussion). Students should be matched with mentors on an individual basis. How is that different from what happened in previous summers where students made proposals, received feedback, and were accepted and matched to mentors or rejected based on how the discussion went? 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] Documentation/git-commit.txt: correct a few minor grammatical mistakes
Brandon Casey wrote: Hmm, I think the original text was more confusing than I realized. I think we should reorder the cleanup modes, placing default last, and then describe default in terms of either strip or whitespace depending on whether an editor will be spawned. Sounds good to me. :) -- To unsubscribe from this list: send the line 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] fixup! Documentation/git-commit.txt: rework the --cleanup section
Hi, Brandon Casey wrote: Signed-off-by: Brandon Casey draf...@gmail.com This renders as --cleanup=mode This option determines how the supplied commit message should be cleaned up before committing. The mode can be strip, whitespace, verbatim, or default. strip Strip leading and trailing empty lines, trailing whitespace, and #commentary and collapse consecutive empty lines. whitespace Same as strip except #commentary is not removed. verbatim Do not change the message at all. default strip if the message is to be edited. Otherwise whitespace. The default can be changed by the 'commit.cleanup' config variable (see linkgit:git-config[1]). Problems: * There's a weird extra blank line after default * Wrong indentation for the final paragraph. * The linkgit isn't resolved for some reason. The following fixes it for me. Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Documentation/git-commit.txt | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index 992c219..24a99cc 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -185,11 +185,12 @@ whitespace:: verbatim:: Do not change the message at all. default:: - `strip` if the message is to be edited. Otherwise `whitespace`. + Same as `strip` if the message is to be edited. + Otherwise `whitespace`. -- + - The default can be changed by the 'commit.cleanup' configuration - variable (see linkgit:git-config[1]). +The default can be changed by the 'commit.cleanup' configuration +variable (see linkgit:git-config[1]). -e:: --edit:: -- 1.8.1.3 -- To unsubscribe from this list: send the line unsubscribe 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 08/19] write_or_die: raise SIGPIPE when we get EPIPE
Jeff King wrote:
The write_or_die function will always die on an error,
including EPIPE. However, it currently treats EPIPE
specially by suppressing any error message, and by exiting
with exit code 0.
Suppressing the error message makes some sense; a pipe death
may just be a sign that the other side is not interested in
what we have to say. However, exiting with a successful
error code is not a good idea, as write_or_die is frequently
used in cases where we want to be careful about having
written all of the output, and we may need to signal to our
caller that we have done so (e.g., you would not want a push
whose other end has hung up to report success).
This distinction doesn't typically matter in git, because we
do not ignore SIGPIPE in the first place. Which means that
we will not get EPIPE, but instead will just die when we get
a SIGPIPE. But it's possible for a default handler to be set
by a parent process,
Not so much default as insane inherited, as in the example
of old versions of Python's subprocess.Popen.
I suspect this used exit(0) instead of raise(SIGPIPE) in the first
place to work around a bash bug (too much verbosity about SIGPIPE).
If any programs still have that kind of bug, I'd rather put pressure
on them to fix it by *not* working around it. So the basic idea here
looks good to me.
[...]
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,5 +1,15 @@
#include cache.h
+static void check_pipe(int err)
+{
+ if (err == EPIPE) {
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
How about
die(BUG: another thread changed SIGPIPE handling behind my
back!);
to make it easier to find and fix such problems?
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 08/19] write_or_die: raise SIGPIPE when we get EPIPE
Jeff King wrote:
On Wed, Feb 20, 2013 at 01:51:11PM -0800, Jonathan Nieder wrote:
+ if (err == EPIPE) {
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
How about
die(BUG: another thread changed SIGPIPE handling behind my
back!);
to make it easier to find and fix such problems?
You mean for the should never happen bit, not the first part, right? I
actually wonder if we should simply exit(141) in the first place. That
is shell exit-code for SIGPIPE death already (so it's what our
run_command would show us, and what anybody running us through shell
would see).
Yes, for the should never happen part. Raising a signal is nice
because it means the wait()-ing process can see what happened by
checking WIFSIGNALED(status).
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 08/19] write_or_die: raise SIGPIPE when we get EPIPE
Jeff King wrote: On Wed, Feb 20, 2013 at 02:01:14PM -0800, Jonathan Nieder wrote: How about die(BUG: another thread changed SIGPIPE handling behind my back!); to make it easier to find and fix such problems? You mean for the should never happen bit, not the first part, right? I actually wonder if we should simply exit(141) in the first place. That is shell exit-code for SIGPIPE death already (so it's what our run_command would show us, and what anybody running us through shell would see). Yes, for the should never happen part. [...] I don't mind adding a BUG: message like you described, but we should still try to exit(141) as the backup, since that is the shell-equivalent code to the SIGPIPE signal death. If you want. :) I think caring about graceful degradation of behavior in the case of an assertion failure is overengineering, but it's mostly harmless. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH] hash-object doc: git hash-object -w can write invalid objects
When using hash-object -w to create non-blob objects, it is generally a good policy to run git fsck afterward to make sure the resulting object is valid. Add a warning to the manpage. While it at, gently nudge the user of hash-object -w toward higher-level interfaces for creating or modifying trees, commits, and tags. Reported-by: Mantas Mikulėnas graw...@gmail.com Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Hi Mantas, Mantas Mikulėnas wrote: When messing around with various repositories, I noticed that git 1.8 (currently using 1.8.2.rc0.22.gb3600c3) has problems parsing tag objects that have invalid timestamps. [...] Git doesn't handle the resulting tag objects nicely at all. For example, running `git cat-file -p` on the new object outputs a really odd timestamp Thu Jun Thu Jan 1 00:16:09 1970 +0016 (I'm guessing it parses the year as Unix time), The usual rule is that with invalid objects (e.g. as detected by git fsck), any non-crash result is acceptable. Garbage in, garbage out. and `git show` outright crashes (backtrace included below.) Probably worth fixing. I notice that git-hash-object(1) doesn't contain any reference to git-fsck(1). How about something like this, to start? Perhaps by default hash-object should automatically fsck the objects it is asked to create. Thanks, Jonathan Documentation/git-hash-object.txt | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Documentation/git-hash-object.txt b/Documentation/git-hash-object.txt index 02c1f12..8ed8c6e 100644 --- a/Documentation/git-hash-object.txt +++ b/Documentation/git-hash-object.txt @@ -30,6 +30,8 @@ OPTIONS -w:: Actually write the object into the object database. + This does not check that the resulting object is valid; + for that, see linkgit:git-fsck[1]. --stdin:: Read the object from standard input instead of from a file. @@ -53,6 +55,14 @@ OPTIONS conversion. If the file is read from standard input then this is always implied, unless the --path option is given. +SEE ALSO + +linkgit:git-mktree[1], +linkgit:git-commit-tree[1], +linkgit:git-tag[1], +linkgit:git-filter-branch[1], +sha1sum(1) + GIT --- Part of the linkgit:git[1] suite -- 1.8.1.4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
