Re: [PATCH 2/2] submodule: munge paths to submodule git directories
At 15:33 -0700 08 Aug 2018, Brandon Williams wrote: Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url encoding it) before using it to build a path to the submodule's gitdir. Seems like this will be a problem if it results in names that exceed NAME_MAX? On common systems that's 255, so it's probably not going to be common; but it certainly could for some repositories.
[PATCH v3] sequencer: use configured comment character
At 10:15 -0700 12 Jul 2018, Junio C Hamano wrote: >Aaron Schrab writes: >> Note that the comment_line_char has already been resolved by this point, >> even if the user has configured the comment character to be selected >> automatically. > >Isn't this a slight lie? Looking into that a bit further, it does seem like my explanation above was incorrect. Here's another attempt to explain why setting core.commentChar=auto isn't a problem for this change. 8< - Use the configured comment character when generating comments about branches in a todo list. Failure to honor this configuration causes a failure to parse the resulting todo list. Setting core.commentChar to "auto" will not be honored here, and the previously configured or default value will be used instead. But, since the todo list will consist of only generated content, there should not be any non-comment lines beginning with that character. Signed-off-by: Aaron Schrab --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 4034c0461b..caf91af29d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp, entry = oidmap_get(&state.commit2label, &commit->object.oid); if (entry) - fprintf(out, "\n# Branch %s\n", entry->string); + fprintf(out, "\n%c Branch %s\n", comment_line_char, entry->string); else fprintf(out, "\n"); -- 2.18.0.419.gfe4b301394
Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar
Sorry for taking so long to get back to this. Hopefully the following will be acceptable to everyone. 8< - Subject: [PATCH v2] sequencer: use configured comment character Use the configured comment character when generating comments about branches in a todo list. Failure to honor this configuration causes a failure to parse the resulting todo list. Note that the comment_line_char has already been resolved by this point, even if the user has configured the comment character to be selected automatically. Signed-off-by: Aaron Schrab --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 4034c0461b..caf91af29d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp, entry = oidmap_get(&state.commit2label, &commit->object.oid); if (entry) - fprintf(out, "\n# Branch %s\n", entry->string); + fprintf(out, "\n%c Branch %s\n", comment_line_char, entry->string); else fprintf(out, "\n"); -- 2.18.0.419.gfe4b301394
[PATCH] sequencer: use configured comment character
Use configured comment character when generating comments about branches in an instruction sheet. Failure to honor this configuration causes a failure to parse the resulting instruction sheet. Signed-off-by: Aaron Schrab --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 4034c0461b..caf91af29d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp, entry = oidmap_get(&state.commit2label, &commit->object.oid); if (entry) - fprintf(out, "\n# Branch %s\n", entry->string); + fprintf(out, "\n%c Branch %s\n", comment_line_char, entry->string); else fprintf(out, "\n"); -- 2.18.0.419.gfe4b301394
Re: [PATCH v3] Documentation: declare "core.ignorecase" as internal variable
At 12:11 -0700 27 Jun 2018, Junio C Hamano wrote: Hmph. Do other people have difficulty applying this patch to their trees? It is just several lines long so I could retype it myself, but I guess "Content-Type: text/plain; charset=utf-8; format=flowed" has destroyed formatting of the patch rather badly. Yes, format=flowed requires lines that start with a space (along with '>' or 'From ') to be space-stuffed, adding a leading space. This will affect context lines in patches. I was able to apply it cleanly (I think) by sending the message to: sed '/@@/,$s/^ / /' | git am That's replacing two leading spaces with one.
Re: [PATCH/RFC] completion: complete all possible -no-
At 17:24 +0200 08 May 2018, Duy Nguyen wrote: It took me so long to reply partly because I remember seeing some guy doing clever trick with tab completion that also shows a short help text in addition to the complete words. I could not find that again and from my reading (also internet searching) it's probably not possible to do this without trickery. Was that perhaps using zsh rather than bash? Below is some of the display from its git completion (this is likely affected somewhat by my configuration). The group descriptions (lines that begin with "Completing") appear in a different color, and are not available for selection. 1113$ git c Completing alias ci -- alias for 'commit -v' cia -- alias for 'commit -v -a' co -- alias for 'checkout' conf -- alias for 'config' Completing main porcelain command checkout -- checkout branch or paths to working tree cherry-pick -- apply changes introduced by some existing commits citool -- graphical alternative to git commit clean-- remove untracked files from working tree clone-- clone repository into new directory commit -- record changes to repository Completing ancillary manipulator command config -- get and set repository or global options Completing ancillary interrogator command cherry -- find commits not merged upstream count-objects-- count unpacked objects and display their disk consumption Completing plumbing manipulator command checkout-index -- copy files from index to working directory commit-tree -- create new commit object Completing plumbing interrogator command cat-file -- provide content or type information for repository objects 1114$ git commit - Completing option --all -a -- stage all modified and deleted paths --allow-empty -- allow recording an empty commit --allow-empty-message -- allow recording a commit with an empty message --amend -- amend the tip of the current branch --author-- override the author name used in the commit
Re: Is there a way to have local changes in a branch 'bake' while working in different branches?
At 20:14 + 15 Dec 2016, Larry Minton wrote: Let's say I have a code change that I want to 'bake' for a while locally, just to make sure some edge case doesn't pop up while I am working on other things. Is there any practical way of doing that? I could constantly merge that 'bake me' branch into other branches as I work on them and then remove those changes from the branches before sending them out for code review, but sooner or later pretty much guaranteed to screw that up That sounds like the best way to me. How do you envision screwing it up? If you anticipate messing up while removing the changes, that's only likely if there are conflicts and any other strategy is likely to be worse there. If you suspect you'll forget to remove them before sending for code review there may be ways to help with that. Easiest you can add a large notice in the commit message(s) and/or comments; this may not prevent going for review but reviewers should catch it pretty quickly. To help prevent it even getting that far you may be able to add a pre-push hook to prevent such commits from being pushed to somewhere other than a private fork or a branch with a name that clearly indicates that it contains experimental code.
Re: git describe number of commits different from git log count
At 12:17 +0100 05 Feb 2016, Jan Hudec wrote: I have a repository with following situation: $ git describe next v4.1-2196-g5a414d7 $ git describe next --match=v4.2 v4.2-4757-g5a414d7 Since the tag with fewest commits since is selected, it appears logical. However, v4.2 is descendant of v4.1, so it does not make sense for it to have more commits since. And rev-list or log confirm that: $ git rev-list v4.1..next | wc -l 2196 $ git rev-list v4.2..next | wc -l 1152 The number of commits since v4.1 matches what the describe counted, but the number of commits since v4.2 does not. I'm encountering what seems to be a similar issue. I'm working with the `build` branch of https://github.com/aschrab/mutt currently at 65b7094. Most of the commits in that repo come from a mercurial repository, but I don't think that is really effecting things (other than being related to the need to use the --tags option). $ git describe --tags mutt-1-7-1-rel-137-g65b7094 $ git describe --tags --match=mutt-1-7-2-rel mutt-1-7-2-rel-6246-g65b7094 $ git rev-list --count mutt-1-7-2-rel..HEAD 126 $ git rev-list --count mutt-1-7-1-rel..HEAD 137 $ git --version git version 2.10.2 The number of additional commits shown when I force the tag is completely insane! That's nearly every commit that is part of that branch: 1036$ git rev-list --count HEAD 6250 Both of the tags above should be reachable along the first-parent path. If I add the `--first-parent` option to the describe command it picks the expected tag and the number additional commits seems sane: $ git describe --tags --first-parent mutt-1-7-2-rel-14-g65b7094
A couple errors dealing with uninitialized submodules
I was working with a fresh clone of a repository where I'd forgotten that one of the directories was setup as a submodule, so I hadn't initialized it. I tried to add a symlink to a location outside the repository and this failed with an assertion (exact text in comment below). When looking into that I realized that the directory was meant to be a submodule and decided to try to change that. So I tried to remove the submodule, and got an error (misleadingly) saying that couldn't be done because it uses a .git directory. I first noticed this with git 2.9.3 from Debian unstable, but I also see it building from v2.10.1-502-g6598894 fetched from master recently. The following script replicates both of these issues. These could both be classified as "don't do that", although at lease the assertion is quite ugly. --- >8 #!/bin/sh -e directory=$(mktemp -d) echo "Using directory '$directory'" cd $directory git init --quiet orig ( cd orig # Using a random, small repository for the submodule. git submodule --quiet add https://github.com/diepm/vim-rest-console.git sub git commit -m init >/dev/null ) git clone --quiet orig dup cd dup ( cd sub ln -s /tmp/dont_care # Next command aborts with # git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.` git add . || : expected to fail ) rm -f .git/index.lock # Next command fails with error wrongly saying that the submodule uses a .git # directory. I believe that the real problem is that the uninitialized # submodule has content. git rm sub
Re: [PATCH 18/18] alternates: use fspathcmp to detect duplicates
At 16:36 -0400 03 Oct 2016, Jeff King wrote: On a case-insensitive filesystem, we should realize that "a/objects" and "A/objects" are the same path. The current repository being on a case-insensitive filesystem doesn't guarantee that the alternates are as well. On the other hand, I suspect that people who use a case-insensitive filesystem would be less likely to use names which differ only by case.
Re: [Opinion gathering] Git remote whitelist/blacklist
At 14:55 +0200 24 May 2016, Matthieu Moy wrote: So, when trying a forbidden push, Git would deny it and the only way to force the push would be to remove the blacklist from the config, right? Probably the sanest way to go. I thought about adding a "git push --force-even-if-in-blacklist" or so, but I don't think the feature deserves one specific option (hence add some noise in `git push -h`). It might make sense to bypass the blacklist checking if the existing --no-verify is used. In the past I've used a pre-push hook to implement a similar method of preventing accidental pushes, and found that to be a good way to skip the checking when I wanted to override the check for a specific push. The builtin blacklist checking could be seen as another type of verification. The downside to that would be that if the blacklist was used along with a pre-push hook for different types of checks users would likely only be able to see the error message from one of them; but that could also apply to a pre-push hook that implements different types of checks and short circuits at the first failure. -- To unsubscribe from this list: send the line "unsubscribe 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 1/2] send-email: new option to quote an email and reply to
At 14:49 +0200 24 May 2016, Matthieu Moy wrote: Samuel GROOT writes: What kind of help text would you want to see? Maybe something like this: GIT: Quoted message body below. GIT: Feel free to trim down the quoted text GIT: to only relevant portions. As "GIT:" portions are ignored when parsed by `git send-email`. That's an option, but in the context of email, I think these instructions are not necessary. In an ideal world that would be true. But in the real world I think evidence of many messages to this mailing list containing full quotes suggests it might be helpful. I'd actually argue that the message be more forceful, making it a suggestion/request to trim rather than simply telling the user that it's allowed. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Redirect "git" subcommand to itself?
At 10:38 +0200 29 May 2015, Christian Neukirchen wrote: Junio C Hamano writes: * You can help yourself with something like this, I suppose: [alias] git = "!sh -c 'exec git \"$@\"' -" but I personally feel that it is too ugly to live as part of our official suggestion, so please do not send a patch to add it as a built-in alias ;-). So I thought I was clever, but this didn't work: I've done the same as the original poster a few times myself, so awhile ago I added the following to my .gitconfig file: [alias] git = "!f() { git $@; }; f" And this has worked for 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
Merge without marking conflicts in working tree
Is there a way to do a merge but only record conflicts in the index, not update the working versions of files with conflict markers? Like many people, I use git to manage configuration files for my shell, editor, git itself, and a number of other things. The vast majority of times that I update things no conflicts are occur and everything just works, so I'd like to avoid extra work in this case. But occasionally a conflict will occur, and if it's in a file that will be read while trying to resolve the conflict this can make things more difficult. I'd like to find a way to have the conflict recorded in just the index without touching the working tree. I could then use my usual tools to resolve the conflict without the errors caused by the conflict markers. I generally use vim+fugitive to resolve conflicts anyway, and typically the first step I take is to replace the working-tree version with the merge-base version, completely ignoring any conflict markers. If there isn't currently a way to do this, I was thinking of implementing something like an "ours" value for merge.conflictstyle configuration. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool: honor --trust-exit-code for builtin tools
At 10:11 -0800 16 Nov 2014, Junio C Hamano wrote: It does not have any significance that a random shell implementation is not POSIX compliant. That would merely mean that such a shell cannot be used to run POSIX shell scripts like our Porcelain. Right, and I suspect that it's very rare for zsh to be used as /bin/sh. I've heard of people doing it just to see what would fail, but not of anybody doing that for regular use. I would suspect that zsh has more "posixly correct" mode, with which it _can_ run POSIX shell scripts, and I would imagine that this "$status is an alias $?" business is disabled in that mode? Yes, if zsh is invoked as either "sh" or "ksh" it attempts to emulate the usual semantics of the named shell. One of the differences is that $status isn't special in the emulation modes. -- To unsubscribe from this list: send the line "unsubscribe 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] CEST is +0200 during April
Mattias, the convention on this list is to send directly to participants in the thread as well as the list. So I've added Andreas as a recipient. At 05:52 +0200 17 Aug 2013, Mattias Andrée wrote: - For example CET (which is 2 hours ahead UTC) is `+0200`. + For example CEST (which is 2 hours ahead UTC) is `+0200`. While changing that line it would be good to also fix the grammar: + For example CEST (which is 2 hours ahead of UTC) is `+0200`. ^^ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git should not use a default user.email config value
At 11:59 +0200 10 Aug 2013, Michael Haggerty wrote: I intentionally don't set user.email in my ~/.gitconfig because I use different identities (on the same machine) depending on what project I am committing to (open-source vs. work). After I clone a repo, I *rely* on Git reminding me to set user.email on my first commit, because I invariably forget to set it myself. And for me, *any* universal, heuristically-determined email address would be wrong for me for at least some repos. I was in a similar situation for awhile. Except in my case I had $EMAIL set for other reasons, so I didn't get the reminder even if git wasn't configured. The solution I came up with was to use a template directory to have the following script installed as a pre-commit hook in all new repos: #!/bin/sh git config user.email > /dev/null && exit echo 'Set email address with `git config user.email` first' >&2 exit 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 v3] config: add support for http..* settings
At 06:07 -0700 12 Jul 2013, "Kyle J. McKay" wrote: I don't think it's necessary to split the URL apart though. Whatever URL the user gave to git on the command line (at some point even if it's now stored as a remote setting in config) complete with URL- encoding, user names, port names, etc. is the same url, possibly shortened, that needs to be used for the http..option setting. This seems to be assuming that the configuration is done after the URL is entered and that URLs are always entered manually. I don't think either of those assumptions is valid. A user may want to specify http settings for all repositories on a specified host and so add settings for that host to ~/.gitconfig expecting those settings to be used later. A URL in a slightly different format may later be copy+pasted without the user realizing that it won't use that config due to one of the versions being in a non-canonical form. I think that's simple and very easy to explain and avoids user confusion and surprise while still allowing a default to be set for a site but easily overridden for a portion of that site without needing to worry about the order config files are processed or the order of the [http ""] sections within them. I agree that the method is easy to explain, but I think a user may very well be surprised and confused in a scenario like I described above. And having the order not matter (in some cases) for these configuration items deviates from how other configuration values are handled. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] push: document --lockref
At 12:53 -0700 09 Jul 2013, Junio C Hamano wrote: +This is meant to make `--force` safer to use. Imagine that you have +to rebase what you have already published. You will have to +`--force` the push to replace the history you originally published +with the rebased history. If somebody else built on top of your +original history while you are rebasing, the tip of the branch at +the remote may advance with her commit, and blindly pushing with +`--force` will lose her work. By using this option to specify that +you expect the history you are updating is what you rebased and want +to replace, you can make sure other people's work will not be losed +by a forced push. in such a case. s/losed/lost/ How does this behave if --force is not used? I think it would be best if it was a no-op in that case to make it easy to add a config option to turn this on by default. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/5] check-ignore: allow incremental streaming of queries via --stdin
At 13:05 +0100 11 Apr 2013, Adam Spiers wrote: The above use case suggests that empty STDIN is actually a reasonable scenario (e.g. when the caller doesn't know in advance whether any queries need to be fed to the background process until after it's already started), so we make the minor behavioural change that "no pathspec given." is no longer emitted in when STDIN is empty. The last "in" there looks to be misplaced. Was that originally something like "in the case"? If so the removed words should be restored or the lingering one removed as well. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/2] clone: Allow repo using gitfile as a reference
Try reading gitfile files when processing --reference options to clone. This will allow, among other things, using a submodule checked out with a recent version of git as a reference repository without requiring the user to have internal knowledge of submodule layout. Signed-off-by: Aaron Schrab diff --git a/builtin/clone.c b/builtin/clone.c index 0a1e0bf..58fee98 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -232,11 +232,21 @@ static void strip_trailing_slashes(char *dir) static int add_one_reference(struct string_list_item *item, void *cb_data) { char *ref_git; + const char *repo; struct strbuf alternate = STRBUF_INIT; - /* Beware: real_path() and mkpath() return static buffer */ + /* Beware: read_gitfile(), real_path() and mkpath() return static buffer */ ref_git = xstrdup(real_path(item->string)); - if (is_directory(mkpath("%s/.git/objects", ref_git))) { + + repo = read_gitfile(ref_git); + if (!repo) + repo = read_gitfile(mkpath("%s/.git", ref_git)); + if (repo) { + free(ref_git); + ref_git = xstrdup(repo); + } + + if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) { char *ref_git_git = mkpathdup("%s/.git", ref_git); free(ref_git); ref_git = ref_git_git; diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index 2a7b78b..719d778 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -185,4 +185,17 @@ test_expect_success 'fetch with incomplete alternates' ' ! grep " want $tag_object" "$U.K" ' +test_expect_success 'clone using repo with gitfile as a reference' ' + git clone --separate-git-dir=L A M && + git clone --reference=M A N && + echo "$base_dir/L/objects" >expected && + test_cmp expected "$base_dir/N/.git/objects/info/alternates" +' + +test_expect_success 'clone using repo pointed at by gitfile as reference' ' + git clone --reference=M/.git A O && + echo "$base_dir/L/objects" >expected && + test_cmp expected "$base_dir/O/.git/objects/info/alternates" +' + test_done -- 1.8.2.677.g9202ef0 -- To unsubscribe from this list: send the line "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 v3 1/2] clone: Fix error message for reference repository
Do not report that an argument to clone's --reference option is not a local directory. Nothing checks for the existence or type of the path as supplied by the user; checks are only done for particular contents of the supposed directory, so we have no way to know the status of the supplied path. Telling the user that a directory doesn't exist when that isn't actually known may lead him or her on the wrong path to finding the problem. Instead just state that the entered path is not a local repository which is really all that is known about it. It could be more helpful to state the actual paths which were checked, but I believe that giving a good description of that would be too verbose for a simple error message and would be too dependent on implementation details. Signed-off-by: Aaron Schrab Reviewed-by: Jonathan Nieder diff --git a/builtin/clone.c b/builtin/clone.c index f9c380e..0a1e0bf 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -241,7 +241,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data) free(ref_git); ref_git = ref_git_git; } else if (!is_directory(mkpath("%s/objects", ref_git))) - die(_("reference repository '%s' is not a local directory."), + die(_("reference repository '%s' is not a local repository."), item->string); strbuf_addf(&alternate, "%s/objects", ref_git); -- 1.8.2.677.g9202ef0 -- To unsubscribe from this list: send the line "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 v3 0/2] Using gitfile repository with clone --reference
Here's the third version of my series for dealing with gitfiles in clone --reference. The first patch is unchanged from the previous version except for the addition of a Reviewed-by line. The second patch has been modified so that it now supports having a .git file supplied as the argument to the option directly rather than only dealing with that if the containing directory was supplied. This makes the first patch from the series more important, since it would make even less sense to complain that the path isn't a directory when a non-directory is acceptable. I've also fixed the minor style issue in the test script from the previous versions. -- To unsubscribe from this list: send the line "unsubscribe 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] clone: Allow repo using gitfile as a reference
At 09:47 -0700 09 Apr 2013, Junio C Hamano wrote: Aaron Schrab writes: But if others disagree, I could be convinced to add support for that. If M/.git weren't a gitfile that points elsewhere, that request ought to work, no? A gitfile is the moral equilvalent of a symbolic link, meant to help people on platforms and filesystems that lack symbolic links, so in that sense, not supporting the case goes against the whole reason why we have added support for gitfile in the first place, I think. OK, I'm convinced. I'll modify it to support that as well. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] clone: Allow repo using gitfile as a reference
At 17:24 -0700 08 Apr 2013, Jonathan Nieder wrote: +test_expect_success 'clone using repo with gitfile as a reference' ' + git clone --separate-git-dir=L A M && + git clone --reference=M A N && What should happen if I pass --reference=M/.git? That isn't supported and I wouldn't expect it to work. The --reference option is documented as taking the location of a repository as the argument and I wouldn't consider a .git file to be a repository. I also can't think of a reason that it would be very useful since it should be simple to just refer to the directory containing the .git file. But if others disagree, I could be convinced to add support for that. I also wouldn't consider it breakage if that use would start working, so I don't see a point in adding a test to check that that usage fails. + echo "$base_dir/L/objects" > expected && The usual style in tests is to include no space after >redirection operators. Fixed for the next version, pending further comments. -- To unsubscribe from this list: send the line "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] clone: Allow repo using gitfile as a reference
Try reading gitfile files when processing --reference options to clone. This will allow, among other things, using a submodule checked out with a recent version of git as a reference repository without requiring the user to have internal knowledge of submodule layout. Signed-off-by: Aaron Schrab --- builtin/clone.c| 12 ++-- t/t5700-clone-reference.sh |7 +++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 0a1e0bf..0dc0791 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -232,11 +232,19 @@ static void strip_trailing_slashes(char *dir) static int add_one_reference(struct string_list_item *item, void *cb_data) { char *ref_git; + const char *repo; struct strbuf alternate = STRBUF_INIT; - /* Beware: real_path() and mkpath() return static buffer */ + /* Beware: read_gitfile(), real_path() and mkpath() return static buffer */ ref_git = xstrdup(real_path(item->string)); - if (is_directory(mkpath("%s/.git/objects", ref_git))) { + + repo = read_gitfile(mkpath("%s/.git", ref_git)); + if (repo) { + free(ref_git); + ref_git = xstrdup(repo); + } + + if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) { char *ref_git_git = mkpathdup("%s/.git", ref_git); free(ref_git); ref_git = ref_git_git; diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index 2a7b78b..7a9044c 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -185,4 +185,11 @@ test_expect_success 'fetch with incomplete alternates' ' ! grep " want $tag_object" "$U.K" ' +test_expect_success 'clone using repo with gitfile as a reference' ' + git clone --separate-git-dir=L A M && + git clone --reference=M A N && + echo "$base_dir/L/objects" > expected && + test_cmp expected "$base_dir/N/.git/objects/info/alternates" +' + test_done -- 1.7.10.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
[PATCH 1/2] clone: Fix error message for reference repository
Do not report that an argument to clone's --reference option is not a local directory. Nothing checks for the existence or type of the path as supplied by the user; checks are only done for particular contents of the supposed directory, so we have no way to know the status of the supplied path. Telling the user that a directory doesn't exist when that isn't actually known may lead him or her on the wrong path to finding the problem. Instead just state that the entered path is not a local repository which is really all that is known about it. It could be more helpful to state the actual paths which were checked, but I believe that giving a good description of that would be too verbose for a simple error message and would be too dependent on implementation details. Signed-off-by: Aaron Schrab --- builtin/clone.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/clone.c b/builtin/clone.c index f9c380e..0a1e0bf 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -241,7 +241,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data) free(ref_git); ref_git = ref_git_git; } else if (!is_directory(mkpath("%s/objects", ref_git))) - die(_("reference repository '%s' is not a local directory."), + die(_("reference repository '%s' is not a local repository."), item->string); strbuf_addf(&alternate, "%s/objects", ref_git); -- 1.7.10.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
[PATCH v2 0/2] Using gitfile repository with clone --reference
Here's the promised second version of this series. The diff in the first patch is unchanged, but I have made significant changes to the commit message to hopefully to a better job of describing why I think the old error message is bad. For the second patch I've eliminated the need to do a cast. Although I'm sending these as a series, the changes are independent both textually and semantically. -- To unsubscribe from this list: send the line "unsubscribe 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] clone: Allow repo using gitfile as a reference
At 11:00 -0700 08 Apr 2013, Junio C Hamano wrote: Aaron Schrab writes: Good catch. I'll fix that in the next version. Thanks. The patch otherwise looks good to me. Great, I'll plan to send version 2 of this series later today. -- To unsubscribe from this list: send the line "unsubscribe 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] clone: Fix error message for reference repository
At 10:57 -0700 08 Apr 2013, Junio C Hamano wrote: In general I am in favor of resolving a gitfile given to --reference when clone interprets it, and have it use the location of the real underlying object store when it grabs objects not in there from the origin and store the location of the real underlying object store in the objects/info/alternates of the newly created repository. But that is not limited to the gitfile used at the root level of a submodule checkout. Yes, I agree that it's not limited to submodules. The commit message for the second part of this series only mentioned submodules because I suspect that is by far the most common use of gitfiles. The commit message for the first didn't even mention submodules at all, they were only brought up because I was asked about what lead to me having an issue. Blindly using .git at the root level of submodule checkout as a reference is what I was recommending against as a general precaution. I agree with that. But I still don't think it's relevant to this patch series. You may be dealing with an old-style submodule checkout. No, the submodule in question was done with the new style. If it were an old-style checkout my attempt to clone using that as a reference would have worked without issue (at least at clone 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: [PATCH 1/2] clone: Fix error message for reference repository
At 08:30 -0700 08 Apr 2013, Junio C Hamano wrote: You switch to a version of the superproject with a plain file at dirA/ or there is nothing at dirA. The checkout will fail and you need to manually rectify the situation [*1*], but after that is done, you do not have any repository at /path/to/super/dirA/.git anymore. That was the reason why I recommended against the practice. So you're essentially saying you don't want to support using a new-world submodule as a reference because using an old-world submodule as such is likely to be problematic? Even though the type of submodule that is actually likely to cause problems would currently be accepted as a reference repository? That seems somewhat perverse to me. Also, nothing in this series is strictly about submodules; that just happens to be what I was working with when I noticed the issue. It would apply to any repository created with --separate-git-dir, although submodules are likely to be the most common occurrence by far. So you are right that we do not remove in the new world order, but then --reference can be given to point at the real location ;-) Yes, that's definitely a possibility. But I think that the location of the work tree for a repository is much more likely to come to a user's mind than the location of a non-bare repository. Especially when dealing with submodules where the repository location was decided for the user, and is somewhat of an implementation detail that the user shouldn't need to care about. -- To unsubscribe from this list: send the line "unsubscribe 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] clone: Fix error message for reference repository
At 06:58 -0700 08 Apr 2013, Junio C Hamano wrote: I do agree that it would be nice to dereference .git gitfile when we deal with --reference argument, but you do not want to use in-tree repository of a submodule working tree. What happens when you have to check out a version of the containing superproject that did not have the submodule you are borrowing from? The directory will disappear, leaving the borrowing repository still pointing at it with its .git/objects/info/alternates file, no? No, submodule directories don't get removed when you checkout a version which didn't contain that submodule. I believe that there are plans to change that for submodules which store the repository data under the containing project's .git directory; but removing the submodule working tree would not affect a repository using that submodule as a reference, since the reading of the .git file is only done during the initial clone. I don't think that the risk of such a repository being deleted or moved is substantially higher than for any other 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: [PATCH 1/2] clone: Fix error message for reference repository
At 20:06 -0400 07 Apr 2013, I wrote: At 16:48 -0700 07 Apr 2013, Jonathan Nieder wrote: Would it make sense for the message to say something like the following? fatal: alternate object store '/path/to/repo.git/objects' is not a local directory That would also avoid lying to the user. But if combined with the second patch in this series it could cause confusion for a different reason. Once .git files are honored, the path reported there may have no relation to the path supplied by the user. Thinking on this further, even without the companion patch there's another issue. The problem isn't just that /path/supplied/by/user/objects isn't a directory. It's that neither that nor /path/supplied/by/user/.git/objects is a directory. And in many cases it's the latter that the user would be expecting to have been used. Reporting on just the last name checked isn't really a good description of what's going on. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] clone: Allow repo using gitfile as a reference
At 16:51 -0700 07 Apr 2013, Jonathan Nieder wrote: - char *ref_git; + char *ref_git, *repo; [...] + repo = (char *)read_gitfile(mkpath("%s/.git", ref_git)); Why not make repo a "const char *" and avoid the cast? The above would seem to make it too tempting to treat the return value from read_gitfile() as a mutable buffer instead of a real_path string that should be copied asap. Good catch. I'll fix that in the next version. 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 1/2] clone: Fix error message for reference repository
At 16:48 -0700 07 Apr 2013, Jonathan Nieder wrote: Hi Aaron, Thanks for the feedback. Aaron Schrab wrote: Do not report an argument to clone's --reference option is not a local directory. Nothing checks for the actual directory so we have no way to know if whether or not exists. Telling the user that a directory doesn't exist when that isn't actually known may lead him or her on the wrong path to finding the problem. I don't understand the above explanation. Could you give an example? I originally noticed this while trying to use a submodule as a reference repository. Since that submodule was first checked out using a recent version of git it used a .git file rather than having a .git directory. This caused the checks to fail, and the misleading error message had me checking for a typo in the path which I'd supplied. I'll attempt to clarify that message in the next version. --- a/builtin/clone.c +++ b/builtin/clone.c @@ -241,7 +241,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data) free(ref_git); ref_git = ref_git_git; } else if (!is_directory(mkpath("%s/objects", ref_git))) - die(_("reference repository '%s' is not a local directory."), + die(_("reference repository '%s' is not a local repository."), "is_directory" calls stat and checks if its target is a directory. Is the problem that "/path/to/repo.git" might be a directory but "/path/to/repo.git/objects" may not? In my case the issue was that /path/to/repo is a directory, but /path/to/repo/.git/objects (which is checked shortly before the above context) didn't exist since /path/to/repo/.git is a file. Would it make sense for the message to say something like the following? fatal: alternate object store '/path/to/repo.git/objects' is not a local directory That would also avoid lying to the user. But if combined with the second patch in this series it could cause confusion for a different reason. Once .git files are honored, the path reported there may have no relation to the path supplied by the user. -- To unsubscribe from this list: send the line "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] clone: Fix error message for reference repository
Do not report an argument to clone's --reference option is not a local directory. Nothing checks for the actual directory so we have no way to know if whether or not exists. Telling the user that a directory doesn't exist when that isn't actually known may lead him or her on the wrong path to finding the problem. Signed-off-by: Aaron Schrab --- builtin/clone.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/clone.c b/builtin/clone.c index f9c380e..0a1e0bf 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -241,7 +241,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data) free(ref_git); ref_git = ref_git_git; } else if (!is_directory(mkpath("%s/objects", ref_git))) - die(_("reference repository '%s' is not a local directory."), + die(_("reference repository '%s' is not a local repository."), item->string); strbuf_addf(&alternate, "%s/objects", ref_git); -- 1.8.2.677.g7422c62 -- To unsubscribe from this list: send the line "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] clone: Allow repo using gitfile as a reference
Try reading gitfile files when processing --reference options to clone. This will allow, among other things, using a submodule checked out with a recent version of git as a reference repository without requiring the user to have internal knowledge of submodule layout. Signed-off-by: Aaron Schrab --- builtin/clone.c| 13 ++--- t/t5700-clone-reference.sh | 7 +++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 0a1e0bf..376ded8 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -231,12 +231,19 @@ static void strip_trailing_slashes(char *dir) static int add_one_reference(struct string_list_item *item, void *cb_data) { - char *ref_git; + char *ref_git, *repo; struct strbuf alternate = STRBUF_INIT; - /* Beware: real_path() and mkpath() return static buffer */ + /* Beware: read_gitfile(), real_path() and mkpath() return static buffer */ ref_git = xstrdup(real_path(item->string)); - if (is_directory(mkpath("%s/.git/objects", ref_git))) { + + repo = (char *)read_gitfile(mkpath("%s/.git", ref_git)); + if (repo) { + free(ref_git); + ref_git = xstrdup(repo); + } + + if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) { char *ref_git_git = mkpathdup("%s/.git", ref_git); free(ref_git); ref_git = ref_git_git; diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index 2a7b78b..7a9044c 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -185,4 +185,11 @@ test_expect_success 'fetch with incomplete alternates' ' ! grep " want $tag_object" "$U.K" ' +test_expect_success 'clone using repo with gitfile as a reference' ' + git clone --separate-git-dir=L A M && + git clone --reference=M A N && + echo "$base_dir/L/objects" > expected && + test_cmp expected "$base_dir/N/.git/objects/info/alternates" +' + test_done -- 1.8.2.677.g7422c62 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] Add sample pre-push hook script
Create a sample of a script for a pre-push hook. The main purpose is to illustrate how a script may parse the information which is supplied to such a hook. The script may also be useful to some people as-is for avoiding to push commits which are marked as a work in progress. Signed-off-by: Aaron Schrab --- templates/hooks--pre-push.sample | 53 1 file changed, 53 insertions(+) create mode 100644 templates/hooks--pre-push.sample diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample new file mode 100644 index 000..15ab6d8 --- /dev/null +++ b/templates/hooks--pre-push.sample @@ -0,0 +1,53 @@ +#!/bin/sh + +# An example hook script to verify what is about to be pushed. Called by "git +# push" after it has checked the remote status, but before anything has been +# pushed. If this script exits with a non-zero status nothing will be pushed. +# +# This hook is called with the following parameters: +# +# $1 -- Name of the remote to which the push is being done +# $2 -- URL to which the push is being done +# +# If pushing without using a named remote those arguments will be equal. +# +# Information about the commits which are being pushed is supplied as lines to +# the standard input in the form: +# +# +# +# This sample shows how to prevent push of commits where the log message starts +# with "WIP" (work in progress). + +remote="$1" +url="$2" + +z40= + +IFS=' ' +while read local_ref local_sha remote_ref remote_sha +do + if [ "$local_sha" = $z40 ] + then + # Handle delete + else + if [ "$remote_sha" = $z40 ] + then + # New branch, examine all commits + range="$local_sha" + else + # Update to existing branch, examine new commits + range="$remote_sha..$local_sha" + fi + + # Check for WIP commit + commit=`git rev-list -n 1 --grep '^WIP' "$range"` + if [ -n "$commit" ] + then + echo "Found WIP commit in $local_ref, not pushing" + exit 1 + fi + fi +done + +exit 0 -- 1.8.1.340.g425b78d -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] push: Add support for pre-push hooks
Add support for a pre-push hook which can be used to determine if the set of refs to be pushed is suitable for the target repository. The hook is run with two arguments specifying the name and location of the destination repository. Information about what is to be pushed is provided by sending lines of the following form to the hook's standard input: SP SP SP LF If the hook exits with a non-zero status, the push will be aborted. This will allow the script to determine if the push is acceptable based on the target repository and branch(es), the commits which are to be pushed, and even the source branches in some cases. Signed-off-by: Aaron Schrab --- Documentation/githooks.txt | 29 ++ builtin/push.c | 1 + t/t5571-pre-push-hook.sh | 129 + transport.c| 60 + transport.h| 1 + 5 files changed, 220 insertions(+) create mode 100755 t/t5571-pre-push-hook.sh diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index b9003fe..d839233 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -176,6 +176,35 @@ save and restore any form of metadata associated with the working tree (eg: permissions/ownership, ACLS, etc). See contrib/hooks/setgitperms.perl for an example of how to do this. +pre-push + + +This hook is called by 'git push' and can be used to prevent a push from taking +place. The hook is called with two parameters which provide the name and +location of the destination remote, if a named remote is not being used both +values will be the same. + +Information about what is to be pushed is provided on the hook's standard +input with lines of the form: + + SP SP SP LF + +For instance, if the command +git push origin master:foreign+ were run the +hook would receive a line like the following: + + refs/heads/master 67890 refs/heads/foreign 12345 + +although the full, 40-character SHA1s would be supplied. If the foreign ref +does not yet exist the `` will be 40 `0`. If a ref is to be +deleted, the `` will be supplied as `(delete)` and the `` will be 40 `0`. If the local commit was specified by something other +than a name which could be expanded (such as `HEAD~`, or a SHA1) it will be +supplied as it was originally given. + +If this hook exits with a non-zero status, 'git push' will abort without +pushing anything. Information about why the push is rejected may be sent +to the user by writing to standard error. + [[pre-receive]] pre-receive ~~~ diff --git a/builtin/push.c b/builtin/push.c index 8491e43..b158028 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -407,6 +407,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "progress", &progress, N_("force progress reporting")), OPT_BIT(0, "prune", &flags, N_("prune locally removed refs"), TRANSPORT_PUSH_PRUNE), + OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK), OPT_END() }; diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh new file mode 100755 index 000..d68fed7 --- /dev/null +++ b/t/t5571-pre-push-hook.sh @@ -0,0 +1,129 @@ +#!/bin/sh + +test_description='check pre-push hooks' +. ./test-lib.sh + +# Setup hook that always succeeds +HOOKDIR="$(git rev-parse --git-dir)/hooks" +HOOK="$HOOKDIR/pre-push" +mkdir -p "$HOOKDIR" +write_script "$HOOK" <actual +echo "$2" >>actual +cat >>actual +EOF + +cat >expected <expected <expected <expected <expected <expected + nr=1000 + while test $nr -lt 2000 + do + nr=$(( $nr + 1 )) + git branch b/$nr $COMMIT3 + echo "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr $_z40" >>expected + done + + test_expect_success 'push many refs' ' + git push parent1 "refs/heads/b/*:refs/heads/b/*" && + diff expected actual + ' +fi + +test_done diff --git a/transport.c b/transport.c index 2673d27..0750a5f 100644 --- a/transport.c +++ b/transport.c @@ -1034,6 +1034,62 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing) die("Aborting."); } +static int run_pre_push_hook(struct transport *transport, +struct ref *remote_refs) +{ + int ret = 0, x; + struct ref *r; + struct child_process proc; + struct strbuf buf; + const char *argv[4]; + + if (!(argv[0] = find_hook("pre-push"))) + return 0; + + argv[1] = transport->remote->name; + argv[2] = transport->url;
[PATCH v2 1/3] hooks: Add function to check if a hook exists
Create find_hook() function to determine if a given hook exists and is executable. If it is, the path to the script will be returned, otherwise NULL is returned. This encapsulates the tests that are used to check for the existence of a hook in one place, making it easier to modify those checks if that is found to be necessary. This also makes it simple for places that can use a hook to check if a hook exists before doing, possibly lengthy, setup work which would be pointless if no such hook is present. The returned value is left as a static value from get_pathname() rather than a duplicate because it is anticipated that the return value will either be used as a boolean, immediately added to an argv_array list which would result in it being duplicated at that point, or used to actually run the command without much intervening work. Callers which need to hold onto the returned value for a longer time are expected to duplicate the return value themselves. Signed-off-by: Aaron Schrab --- builtin/commit.c | 6 ++ builtin/receive-pack.c | 25 +++-- run-command.c | 15 +-- run-command.h | 1 + 4 files changed, 27 insertions(+), 20 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index d6dd3df..65d08d2 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1327,8 +1327,6 @@ static int git_commit_config(const char *k, const char *v, void *cb) return git_status_config(k, v, s); } -static const char post_rewrite_hook[] = "hooks/post-rewrite"; - static int run_rewrite_hook(const unsigned char *oldsha1, const unsigned char *newsha1) { @@ -1339,10 +1337,10 @@ static int run_rewrite_hook(const unsigned char *oldsha1, int code; size_t n; - if (access(git_path(post_rewrite_hook), X_OK) < 0) + argv[0] = find_hook("post-rewrite"); + if (!argv[0]) return 0; - argv[0] = git_path(post_rewrite_hook); argv[1] = "amend"; argv[2] = NULL; diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index ff781fe..e8878de 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -182,9 +182,6 @@ struct command { char ref_name[FLEX_ARRAY]; /* more */ }; -static const char pre_receive_hook[] = "hooks/pre-receive"; -static const char post_receive_hook[] = "hooks/post-receive"; - static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2))); static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2))); @@ -242,10 +239,10 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, void *feed_sta const char *argv[2]; int code; - if (access(hook_name, X_OK) < 0) + argv[0] = find_hook(hook_name); + if (!argv[0]) return 0; - argv[0] = hook_name; argv[1] = NULL; memset(&proc, 0, sizeof(proc)); @@ -331,15 +328,14 @@ static int run_receive_hook(struct command *commands, const char *hook_name, static int run_update_hook(struct command *cmd) { - static const char update_hook[] = "hooks/update"; const char *argv[5]; struct child_process proc; int code; - if (access(update_hook, X_OK) < 0) + argv[0] = find_hook("update"); + if (!argv[0]) return 0; - argv[0] = update_hook; argv[1] = cmd->ref_name; argv[2] = sha1_to_hex(cmd->old_sha1); argv[3] = sha1_to_hex(cmd->new_sha1); @@ -532,24 +528,25 @@ static const char *update(struct command *cmd) } } -static char update_post_hook[] = "hooks/post-update"; - static void run_update_post_hook(struct command *commands) { struct command *cmd; int argc; const char **argv; struct child_process proc; + char *hook; + hook = find_hook("post-update"); for (argc = 0, cmd = commands; cmd; cmd = cmd->next) { if (cmd->error_string || cmd->did_not_exist) continue; argc++; } - if (!argc || access(update_post_hook, X_OK) < 0) + if (!argc || !hook) return; + argv = xmalloc(sizeof(*argv) * (2 + argc)); - argv[0] = update_post_hook; + argv[0] = hook; for (argc = 1, cmd = commands; cmd; cmd = cmd->next) { char *p; @@ -704,7 +701,7 @@ static void execute_commands(struct command *commands, const char *unpacker_erro 0, &cmd)) set_connectivity_errors(commands); - if (run_receive_hook(commands, pre_receive_hook, 0)) { + if (run_receive_hook(commands, "pre-receive", 0)) { for (cmd = commands; cmd; cmd = cmd->next) { if (!
[PATCH v2 0/3] pre-push hook support
Main changes since the initial version: * The first patch converts the existing hook callers to use the new find_hook() function. * Information about what is to be pushed is now sent over a pipe rather than passed as command-line parameters. Aaron Schrab (3): hooks: Add function to check if a hook exists push: Add support for pre-push hooks Add sample pre-push hook script Documentation/githooks.txt | 29 + builtin/commit.c | 6 +- builtin/push.c | 1 + builtin/receive-pack.c | 25 run-command.c| 15 - run-command.h| 1 + t/t5571-pre-push-hook.sh | 129 +++ templates/hooks--pre-push.sample | 53 transport.c | 60 ++ transport.h | 1 + 10 files changed, 300 insertions(+), 20 deletions(-) create mode 100755 t/t5571-pre-push-hook.sh create mode 100644 templates/hooks--pre-push.sample -- 1.8.1.340.g425b78d -- To unsubscribe from this list: send the line "unsubscribe 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] hooks: Add function to check if a hook exists
At 18:08 -0800 28 Dec 2012, Junio C Hamano wrote: Aaron Schrab writes: Create find_hook() function to determine if a given hook exists and is executable. If it is the path to the script will be returned, otherwise NULL is returned. Sounds like a sensible thing to do. To make sure the API is also sensible, all the existing hooks should be updated to use this API, no? I'd been trying to keep the changes limited. I'll see about modifying the existing places that run hooks in v2 of the series. This is in support for an upcoming run_hook_argv() function which will expect the full path to the hook script as the first element in the argv_array. There is currently a public function called run_hook() that squats on the good name with a kludgy API that is too specific to using separate index file. Back when it was a private helper in the implementation of "git commit", it was perfectly fine, but it was exported without giving much thought on the API. If you are introducing a new run_hook_* function, give it a generic enough API that lets all the existing hook callers to use it. I would imagine that the API requirement may be modelled after run_command() API so that we can pass argv[] and tweak the hook's environ[], as well as feeding its stdin and possibly reading from its stdout. That would be very useful. I think the attraction of the run_hook() API is its simplicity. It's currently a fairly thin wrapper around the run_command() API. I suspect that if the run_hook() API were made generic enough to support all of the existing hook callers it would greatly complicate the existing calls to run_hook() while not providing much benefit to hook callers which can't currently use it beyond what run_command() offers. Since I'm going to be changing the interface for this hook in v2 of the series so that it will be more complicated than can be readily addressed with the run_hook() API (and will have use a fixed number of arguments anyway) I'll be dropping the run_hook_argv() function. -- To unsubscribe from this list: send the line "unsubscribe 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/4] pre-push hook support
At 18:01 -0800 28 Dec 2012, Junio C Hamano wrote: One lesson we learned long time ago while doing hooks is to avoid unbound number of command line arguments and instead feed them from the standard input. I think this should do the same. Good point. I had been trying to keep the interface for this hook as close as possible to the ones for other client-side hooks on the theory that less development effort may go into those than for server-side hooks. But thinking on that more I certainly see that this could easily run into limits on argument length on some systems; especially when it's likely that each of those arguments is likely to be over 100 bytes long. I'll work on an updated version which sends the variable length information over a pipe, using the command-line arguments only to pass the remote name and URL. How does the hook communicate its decision to the calling Git? Will it be "all-or-none", or "I'll allow these but not those"? Currently it just uses the exit code to communicate that back, so it's all-or-none. I think I'll keep that in the updated version as well. A future enhancement could modify the protocol to support reading from the hook's stdout the names of remote refs which are to be rejected, I think that just having the option for all-or-nothing is a good starting point. -- To unsubscribe from this list: send the line "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] Use longer alias names in subdirectory tests
When testing aliases in t/t1020-subdirectory.sh use longer names so that they're less likely to conflict with a git-* command somewhere in the $PATH. I have a git-ss command in my path which prevents the 'ss' alias from being used. This command will always fail for git.git, causing the test to fail. Even if the command succeeded, that would be a false success for the test since the alias wasn't actually used. A longer, more descriptive name will make it much less likely that somebody has a command in their $PATH which will shadow the alias created for the test. While here, use a longer name for the 'test' alias as well since that is also short and meaningful enough to make it not unlikely that somebody would have a command in their $PATH which will shadow that as well. Signed-off-by: Aaron Schrab --- t/t1020-subdirectory.sh | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh index e23ac0e..1e2945e 100755 --- a/t/t1020-subdirectory.sh +++ b/t/t1020-subdirectory.sh @@ -111,19 +111,19 @@ test_expect_success 'read-tree' ' test_expect_success 'alias expansion' ' ( - git config alias.ss status && + git config alias.test-status-alias status && cd dir && git status && - git ss + git test-status-alias ) ' test_expect_success NOT_MINGW '!alias expansion' ' pwd >expect && ( - git config alias.test !pwd && + git config alias.test-alias-directory !pwd && cd dir && - git test >../actual + git test-alias-directory >../actual ) && test_cmp expect actual ' @@ -131,9 +131,9 @@ test_expect_success NOT_MINGW '!alias expansion' ' test_expect_success 'GIT_PREFIX for !alias' ' printf "dir/" >expect && ( - git config alias.test "!sh -c \"printf \$GIT_PREFIX\"" && + git config alias.test-alias-directory "!sh -c \"printf \$GIT_PREFIX\"" && cd dir && - git test >../actual + git test-alias-directory >../actual ) && test_cmp expect actual ' -- 1.8.1.rc3.16.g47d6ba6 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] push: Add support for pre-push hooks
Add support for a pre-push hook which can be used to determine if the set of refs to be pushed is suitable for the target repository. The hook should be supplied with: 1. name of the remote being used, or the URL if not using a named remote 2. the URL to which we're pushing 3. descriptions of what references are to be pushed Each reference to be pushed should be described in a separate parameter to the hook script in the form: ::: This will allow the script to determine if the push is acceptable based on the target repository and branch(es), the commits which are to be pushed, and even the source branches in some cases. Signed-off-by: Aaron Schrab --- Documentation/githooks.txt | 28 + builtin/push.c |1 + t/t5571-pre-push-hook.sh | 145 transport.c| 25 transport.h|1 + 5 files changed, 200 insertions(+) create mode 100755 t/t5571-pre-push-hook.sh diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index b9003fe..e9539bb 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -176,6 +176,34 @@ save and restore any form of metadata associated with the working tree (eg: permissions/ownership, ACLS, etc). See contrib/hooks/setgitperms.perl for an example of how to do this. +pre-push + + +This hook is called by 'git push' and can be used to prevent a push from +taking place. The hook is called with a variable number of parameters. + +The first parameters provide the name and location of the destination +remote, if a named remote is not being used both values will be the same. + +Remaining parameters provide information about the commits which are to be +pushed and the ref names being used. These arguments take the form: + + ::: + +For instance, if the command +git push origin master:foreign+ were run the +hook would be called with a third arugment similar to: + + refs/heads/master:67890:refs/heads/foreign:12345 + +although the full, 40-character SHA1s would be supplied. If the foreign ref +does not yet exist the `` will be 40 `0`. If a ref is to be +deleted, the `` will be supplied as `(delete)` and the `` will be 40 `0`. If the local commit was specified by something other +than a name which could be expanded (such as `HEAD~`, or a SHA1) it will be +supplied as it was originally given. + +If this hook exits with a non-zero status, 'git push' will abort. + [[pre-receive]] pre-receive ~~~ diff --git a/builtin/push.c b/builtin/push.c index db9ba30..c33fb9b 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -399,6 +399,7 @@ int cmd_push(int argc, const char **argv, const char *prefix) OPT_BOOL(0, "progress", &progress, N_("force progress reporting")), OPT_BIT(0, "prune", &flags, N_("prune locally removed refs"), TRANSPORT_PUSH_PRUNE), + OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK), OPT_END() }; diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh new file mode 100755 index 000..5444c9b --- /dev/null +++ b/t/t5571-pre-push-hook.sh @@ -0,0 +1,145 @@ +#!/bin/sh + +test_description='check pre-push hooks' +. ./test-lib.sh + +# Setup hook that always succeeds +HOOKDIR="$(git rev-parse --git-dir)/hooks" +HOOK="$HOOKDIR/pre-push" +mkdir -p "$HOOKDIR" +cat >"$HOOK" <"$HOOK" <"$HOOK" <<'EOF' +#!/bin/sh -ex +test "$#" = 3 +test "$1" = parent1 +test "$2" = repo1 +test "$3" = "refs/heads/master:$COMMIT2:refs/heads/foreign:$COMMIT1" +EOF + +test_expect_success 'push with hook' ' + git push parent1 master:foreign +' + +test_expect_success 'add a branch' ' + git checkout -b other && + test_commit three +' + +COMMIT3="$(git rev-parse HEAD)" +export COMMIT3 + +cat >"$HOOK" <<'EOF' +#!/bin/sh -ex +test "$#" = 4 +test "$1" = parent1 +test "$2" = repo1 +test "$3" = "refs/heads/other:$COMMIT3:refs/heads/foreign:$COMMIT2" +test "$4" = "refs/heads/master:$COMMIT2:refs/heads/new:$_z40" +EOF + +test_expect_success 'push multiple refs' ' + git push parent1 other:foreign master:new +' + +test_expect_success 'add a branch with an upstream' ' + git checkout -t -b tracking parent1/foreign && + test_commit four +' +COMMIT4="$(git rev-parse HEAD)" +export COMMIT4 + +cat >"$HOOK" <<'EOF' +#!/bin/sh -ex +test "$#" = 3 +test "$1" = parent1 +test "$2&
[PATCH 4/4] Add sample pre-push hook script
Create a sample of a script for a pre-push hook. The main purpose is to illustrate how a script may parse the parameters which are supplied to such a hook. The script may also be useful to some people as-is for avoiding to push commits which are marked as a work in progress. Signed-off-by: Aaron Schrab --- templates/hooks--pre-push.sample | 63 ++ 1 file changed, 63 insertions(+) create mode 100644 templates/hooks--pre-push.sample diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample new file mode 100644 index 000..1d3b4a3 --- /dev/null +++ b/templates/hooks--pre-push.sample @@ -0,0 +1,63 @@ +#!/bin/sh + +# An example hook script to verify what is about to be pushed. +# Called by "git push" after it has checked the remote status, but before +# anything has been pushed. If this script exits with a non-zero status +# nothing will be pushed. +# +# This hook is called with the following parameters: +# +# $1 -- Name of the remote to which the push is being done +# $2 -- URL to which the push is being done +# +# If pushing without using a named remote those arguments will be equal. +# +# Further arguments provide information about the commits which are being +# pushed in the form: +# +# ::: +# +# This sample shows how to prevent push of commits where the log +# message starts with "WIP" (work in progress). + +remote="$1" +url="$2" +shift 2 + +z40= + +old_ifs="$IFS" +for to_push in "$@" +do + # Split the value into its parts + IFS=: + set -- $to_push + IFS="$old_ifs" + + local_ref="$1" + local_sha="$2" + remote_ref="$3" + remote_sha="$4" + + if [ "$local_sha" = $z40 ] + then + range='' + # Handle deletes + else + if [ "$remote_sha" = $z40 ] + then + range="$local_sha" + else + range="$remote_sha..$local_sha" + fi + + commit=`git rev-list -n 1 --grep '^WIP' "$range"` + if [ -n "$commit" ] + then + echo "Found WIP commit in $local_ref, not pushing" + exit 1 + fi + fi +done + +exit 0 -- 1.7.10.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
[PATCH 2/4] hooks: support variable number of parameters
Define the run_hook_argv() function to allow hooks to be created where the number of parameters to be passed is variable. The existing run_hook() function uses stdarg to allow it to receive a variable number of arguments, but the number of arguments that a given caller is passing is fixed at compile time. This function will allow the caller of a hook to determine the number of arguments to pass when preparing to call the hook. The first use of this function will be for a pre-push hook which will add an argument for every reference which is to be pushed. Signed-off-by: Aaron Schrab --- run-command.c | 20 +--- run-command.h |2 ++ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/run-command.c b/run-command.c index 49c8fa0..e07202b 100644 --- a/run-command.c +++ b/run-command.c @@ -2,7 +2,6 @@ #include "run-command.h" #include "exec_cmd.h" #include "sigchain.h" -#include "argv-array.h" #ifndef SHELL_PATH # define SHELL_PATH "/bin/sh" @@ -746,10 +745,8 @@ char *find_hook(const char *name) int run_hook(const char *index_file, const char *name, ...) { - struct child_process hook; struct argv_array argv = ARGV_ARRAY_INIT; - const char *p, *env[2]; - char index[PATH_MAX]; + const char *p; va_list args; int ret; @@ -764,6 +761,17 @@ int run_hook(const char *index_file, const char *name, ...) argv_array_push(&argv, p); va_end(args); + ret = run_hook_argv(index_file, argv); + argv_array_clear(&argv); + return ret; +} + +int run_hook_argv(const char *index_file, struct argv_array argv) +{ + struct child_process hook; + char index[PATH_MAX]; + const char *env[2]; + memset(&hook, 0, sizeof(hook)); hook.argv = argv.argv; hook.no_stdin = 1; @@ -775,7 +783,5 @@ int run_hook(const char *index_file, const char *name, ...) hook.env = env; } - ret = run_command(&hook); - argv_array_clear(&argv); - return ret; + return run_command(&hook); } diff --git a/run-command.h b/run-command.h index 221ce33..12faa5b 100644 --- a/run-command.h +++ b/run-command.h @@ -1,6 +1,7 @@ #ifndef RUN_COMMAND_H #define RUN_COMMAND_H +#include "argv-array.h" #ifndef NO_PTHREADS #include #endif @@ -47,6 +48,7 @@ int run_command(struct child_process *); extern char *find_hook(const char *name); extern int run_hook(const char *index_file, const char *name, ...); +extern int run_hook_argv(const char *index_file, struct argv_array); #define RUN_COMMAND_NO_STDIN 1 #define RUN_GIT_CMD 2 /*If this is to be git sub-command */ -- 1.7.10.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
[PATCH 0/4] pre-push hook support
There have been at least a couple of submissions to add support for a pre-push hook, which were rejected at least partially because they didn't provide enough information to a hook script for it to determine what was to be pushed any better than a separate wrapper around the 'git push' command would be able to do. In this series I attempt to address that problem. The first two patches in this series do a little bit of refactoring in order to make it easier to call hooks with a variable number of arguments. The third patch actually adds support for calling a pre-push hook. If it exists, it will be called with the name and URL of the destination remote (if a named remote isn't being used, the URL will be supplied for both) followed by another argument for each ref being pushed; these arguments take the form: ::: This should provide enough information for a script to easily determine the set of commits that is being pushed, and thus make a decision if that should be allowed. The final patch adds a sample pre-push hook script which will deny attempts to push commits that are marked as a work in progress. Aaron Schrab (4): hooks: Add function to check if a hook exists hooks: support variable number of parameters push: Add support for pre-push hooks Add sample pre-push hook script Documentation/githooks.txt | 28 builtin/push.c |1 + run-command.c| 35 ++--- run-command.h|3 + t/t5571-pre-push-hook.sh | 145 ++ templates/hooks--pre-push.sample | 63 + transport.c | 25 +++ transport.h |1 + 8 files changed, 292 insertions(+), 9 deletions(-) create mode 100755 t/t5571-pre-push-hook.sh create mode 100644 templates/hooks--pre-push.sample -- 1.7.10.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
[PATCH 1/4] hooks: Add function to check if a hook exists
Create find_hook() function to determine if a given hook exists and is executable. If it is the path to the script will be returned, otherwise NULL is returned. This is in support for an upcoming run_hook_argv() function which will expect the full path to the hook script as the first element in the argv_array. This also makes it simple for places that can use a hook to check if a hook exists before doing, possibly lengthy, setup work which would be pointless if no such hook is present. The returned value is left as a static value from get_pathname() rather than a duplicate because it is anticipated that the return value will either be used as a boolean, or immediately added to an argv_array list which would result in it being duplicated at that point. Signed-off-by: Aaron Schrab --- run-command.c | 15 +-- run-command.h |1 + 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/run-command.c b/run-command.c index 3b982e4..49c8fa0 100644 --- a/run-command.c +++ b/run-command.c @@ -735,6 +735,15 @@ int finish_async(struct async *async) #endif } +char *find_hook(const char *name) +{ + char *path = git_path("hooks/%s", name); + if (access(path, X_OK) < 0) + path = NULL; + + return path; +} + int run_hook(const char *index_file, const char *name, ...) { struct child_process hook; @@ -744,11 +753,13 @@ int run_hook(const char *index_file, const char *name, ...) va_list args; int ret; - if (access(git_path("hooks/%s", name), X_OK) < 0) + p = find_hook(name); + if (!p) return 0; + argv_array_push(&argv, p); + va_start(args, name); - argv_array_push(&argv, git_path("hooks/%s", name)); while ((p = va_arg(args, const char *))) argv_array_push(&argv, p); va_end(args); diff --git a/run-command.h b/run-command.h index 850c638..221ce33 100644 --- a/run-command.h +++ b/run-command.h @@ -45,6 +45,7 @@ int start_command(struct child_process *); int finish_command(struct child_process *); int run_command(struct child_process *); +extern char *find_hook(const char *name); extern int run_hook(const char *index_file, const char *name, ...); #define RUN_COMMAND_NO_STDIN 1 -- 1.7.10.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
Re: RFC: "git config -l" should not expose sensitive information
At 10:04 -0500 20 Dec 2012, Jeff King wrote: The problem seems to be that people are giving bad advice to tell people to post "git config -l" output without looking at. Maybe we could help them with a "git config --share-config" option that dumps all config, but sanitizes the output. It would need to have a list of sensitive keys (which does not exist yet), and would need to not just mark up things like smtppass, but would also need to pull credential information out of remote.*.url strings. And maybe more (I haven't thought too long on it). If such an option is added, it is likely to cause more people to think that there is no need to examine the output before sharing it. But, I don't think that the sanitizing could ever be sufficient to guarantee that. Tools outside of the core git tree may add support for new config keys which are meant to contain sensitive information, and there would be no way for `git config` to know about those. Even for known sensitive keys, the person entering it might have made a typo in the name (e.g. smptpass) preventing it from being recognized as sensitive by the software, but easily recognizable as such by a human. There's also the problem of varying opinions on what is considered as sensitive. You mention credential information in URLs, but some people may consider the entire URL as something which they would not want to expose. I think that attempting to do this would only result in a false sense of security. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Possible vulnerability to SHA-1 collisions
At 18:07 -0500 27 Nov 2012, Jeff King wrote: PS I also think the OP's "sockpuppet creates innocuous bugfix" above is easier said than done. We do not have SHA-1 collisions yet, but if the md5 attacks are any indication, the innocuous file will not be completely clean; it will need to have some embedded binary goo that is mutated randomly during the collision process (which is why the md5 attacks were demonstrated with postscript files which _rendered_ to look good, but contained a chunk of random bytes in a spot ignored by the postscript interpreter). I don't think that really saves us though. Many formats have parts of the file which will be ignored, such as comments in source code. With the suggested type of attack, there isn't a requirement about which version of the file is modified. So the attacker should be able to generate a version of a file with an innocuous change, get the SHA-1 for that, then add garbage comments to their malicious version of the file to try to get the same SHA-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: [RFC/PATCH] Option to revert order of parents in merge commit
At 09:58 -0800 26 Nov 2012, Junio C Hamano wrote: Kacper Kornet writes: The change of order of parents happens at the very last moment, so "ours" in merge options is local version and "theirs" upstream. That may be something that wants to go to the proposed commit log message. I am neutral on the "feature" (i.e. not against it but not extremely enthusiastic about it either). That should also be included in the (currently nonexistent) documentation of the proposed option. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Strange behaviour of git diff branch1 ... branch2
I came across this odd question on stackoverflow: http://stackoverflow.com/q/13092854/1507392 If git diff is run with "..." as a separate argument between two commit-ish arguments causes it to produce strange output. The differences seem to be the same as if "..." was left out, but change lines begin with 4 + or - characters rather than just 1. Can anybody explain what is happening here? I don't have any reason to want to use that form myself, but I'm very curious about why it produces this odd output. 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