Re: What's cooking in git.git (Mar 2013, #07; Tue, 26)
Junio C Hamano gits...@pobox.com writes:
* tr/line-log (2013-03-23) 6 commits
- Speed up log -L... -M
- log -L: :pattern:file syntax to find by funcname
- Implement line-history search (git log -L)
- Export rewrite_parents() for 'log -L'
- fixup
- Refactor parse_loc
Rerolled; collides with nd/magic-pathspecs.
Honestly I am not sure what to make of this one. I'd say we should
merge this down as-is to 'master', expecting that in some future we
would fix log --follow to keep the refspecs per history traversal
path, so that this can be more naturally reimplemented. Objections?
I was really hoping for something like that to happen :-) but I need to
look into at least one segfault bug in the option parser, noticed by
Antoine Pelisse. Expect a (final?) reroll soon.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Avoid loading commits twice in log with diffs
If you run a log with diffs (such as -p, --raw, --stat etc.) the
current code ends up loading many objects twice. For example, for
'log -3000 -p' my instrumentation said the objects loaded more than
once are distributed as follows:
2008 blob
2103 commit
2678 tree
Fixing blobs and trees will be harder, because those are really used
within the diff engine and need some form of caching.
However, fixing the commits is easy at least at the band-aid level.
They are triggered by log_tree_diff() invoking diff_tree_sha1() on
commits, which duly loads the specified object to dereference it to a
tree. Since log_tree_diff() knows that it works with commits and they
must have trees, we can simply pass through the trees.
We add some parse_commit() calls. The ones for the parents are
required; we do not know at this stage if they have been looked at.
The one for the commit itself is pure paranoia, but has about the same
cost as an assertion on commit-object.parsed.
This has a quite dramatic effect on log --raw, though only a
negligible impact on log -p:
Test this tree HEAD
4000.2: log --raw -3000 0.50(0.43+0.06) 0.54(0.46+0.06) +7.0%***
4000.3: log -p -3000 2.34(2.20+0.13) 2.37(2.22+0.13) +1.2%
Significance hints: '.' 0.1 '*' 0.05 '**' 0.01 '***' 0.001
Signed-off-by: Thomas Rast tr...@student.ethz.ch
Signed-off-by: Thomas Rast tr...@inf.ethz.ch
---
Adjusted for the concern that the commit might not be parsed yet. I
think it's now more paranoid than the original code, since we cannot
look at commit-parents without parsing. But it's really an almost
free check.
log-tree.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index 5dc45c4..8a34332 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -792,11 +792,14 @@ static int log_tree_diff(struct rev_info *opt, struct
commit *commit, struct log
{
int showed_log;
struct commit_list *parents;
- unsigned const char *sha1 = commit-object.sha1;
+ unsigned const char *sha1;
if (!opt-diff !DIFF_OPT_TST(opt-diffopt, EXIT_WITH_STATUS))
return 0;
+ parse_commit(commit);
+ sha1 = commit-tree-object.sha1;
+
/* Root commit? */
parents = commit-parents;
if (!parents) {
@@ -819,7 +822,9 @@ static int log_tree_diff(struct rev_info *opt, struct
commit *commit, struct log
* parent, showing summary diff of the others
* we merged _in_.
*/
- diff_tree_sha1(parents-item-object.sha1, sha1, ,
opt-diffopt);
+ parse_commit(parents-item);
+ diff_tree_sha1(parents-item-tree-object.sha1,
+ sha1, , opt-diffopt);
log_tree_diff_flush(opt);
return !opt-loginfo;
}
@@ -832,7 +837,9 @@ static int log_tree_diff(struct rev_info *opt, struct
commit *commit, struct log
for (;;) {
struct commit *parent = parents-item;
- diff_tree_sha1(parent-object.sha1, sha1, , opt-diffopt);
+ parse_commit(parent);
+ diff_tree_sha1(parent-tree-object.sha1,
+ sha1, , opt-diffopt);
log_tree_diff_flush(opt);
showed_log |= !opt-loginfo;
--
1.8.2.351.g867a5da
--
To unsubscribe from this list: send the line unsubscribe 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 help config: s/insn/instruction/
On 03/28/2013 06:59 AM, Junio C Hamano wrote: Matthias Krüger matthias.krue...@famsik.de writes: insn appears to be an in-code abbreviation and should not appear in manual/help pages. --- Thanks; sign-off? Oops, sorry. Signed-off-by: Matthias Krüger matthias.krue...@famsik.de (Is this sufficient or do I have to re-send the patch with the sign-off line?) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Composing git repositories
Jens Lehmann wrote: Unless you acknowledge that submodules are a different repo, you'll always run into problems. I believe future enhancements will make this less tedious, but in the end they will stay separate repos (which is the whole point, you'd want to use a different approach - e.g. subtree - if you want to put stuff from different upstreams into a single repo without keeping the distinction where all that came from). I acknowledge that it's a different repository. It's just that I think that our current design has too many seams: why do you think it's impossible to make it seamless? git-subtree is not an answer to anything. Dumping all the history into one repository has its limited usecases, but it is no solution. What else than a commit object should that be??? Submodules are there to have a different upstream for a part of your work tree, and that means a commit in that repo is the only sane thing to record in the superproject. A lot of thought has been put into this, and it is definitely a good choice [1]. Linus argues that it shouldn't be a tree object, and I agree with that. I don't see an argument that says that the commit object is a perfect fit (probably because it's not). How? The submodules suck, we should try a completely different approach thingy comes up from time to time, but so far nobody could provide a viable alternative to what we currently do. My argument is not submodules suck; we should throw them out of the window, and start from scratch at all. I'm merely questioning the fundamental assumptions that submodules make, instead of proposing that we work around everything in shell. We don't have to be married to the existing implementation of submodules and try to fix all the problems in shell. And apart from that, let's not forget we identified some valuable improvements to submodules in this thread: [...] All of those are topics I like to see materialize, and you are welcome to tackle them. Allow me a few days to think about changing the fundamental building blocks to make our shell hackery easier. 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] merge-tree: fix same file added in subdir
On Wed, Mar 27, 2013 at 10:57:39PM +, John Keeping wrote: On Wed, Mar 27, 2013 at 03:42:40PM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: When the same file is added with identical content at the top level, git-merge-tree prints added in both with the details. But if the file is added in an existing subdirectory, threeway_callback() bails out early because the two trees have been modified identically. In order to detect this, we need to fall through and recurse into the subtree in this case. Signed-off-by: John Keeping j...@keeping.me.uk The rationale the above description gives is internally consistent, but it is rather sad to see this optimization go. The primary motivation behind this program, which does not use the usual unpack-trees machinery, is to allow us to cull the identical result at a shallow level of the traversal when the both sides changed (not added) a file deep in a subdirectory hierarchy. The patch makes me wonder if we should go the other way around, resolving the both added identically case at the top cleanly without complaint. I don't use merge-tree so I have no opinion on this, just wanted to fix an inconsistency :-) Having re-read the manpage, I think you're right that we should just resolve the both added identically case cleanly, but I wonder whether some of the other cases should also be resolved cleanly. git-merge-tree(1) says: the output from the command omits entries that match the branch1 tree. so you could argue that added in branch1, changed in branch1, unmodified in branch2 and removed in branch1, unchanged in branch2 should also print no output. But as I said above I don't use git-merge-tree so perhaps people who do would like to explain what they expect in these cases. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Composing git repositories
Jonathan Nieder wrote: Do you mean that you wish you could ignore subrepository boundaries and use commands like git clone --recurse-submodules http://git.zx2c4.com/cgit cd cgit vi git/cache.h ... edit edit edit ... git add --recurse-submodules git/cache.h git commit --recurse-submodules git push --recurse-submodules , possibly with configuration to allow the --recurse-submodules to be implied, and have everything work out well? Do you realize how difficult this is to implement? We'll need to patch all the git commands to essentially do what we'd get for free if the submodule were a tree object instead of a commit object (although I'm not saying that's the Right thing to do). Some caveats: - If we maintain one global index, and try to emulate git-subtree using submodules, we've lost. It's going to take freakin' ages to stat billions of files across hundreds of nested sumodules. One major advantage of having repository boundaries is separate object stores, indexes, worktrees: little ones that git is designed to work with. - Auto-splitting commits that touch multiple submodules/ superproject at once. Although git-subtree does this, I think it's horribly ugly. - Auto-propagating commits upwards to the superproject is another big challenge. I think the current design of anchoring to a specific commit SHA-1 has its usecases, but is unwieldy when things become big. We have to fix that first. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Collective wisdom about repos on NFS accessed by concurrent clients (== corruption!?)
Hi, I'm hoping to hear some wisdom on the subject so I can decide if I'm chasing a pipe dream or if it should be expected to work and I just need to work out the kinks. Finding things like this makes it sound possible: http://permalink.gmane.org/gmane.comp.version-control.git/122670 but then again, in threads like this: http://kerneltrap.org/mailarchive/git/2010/11/14/44799 opinions are that it's just not reliable to trust. My experience so far is that I eventually get repo corruption when I stress it with concurrent read/write access from multiple hosts (beyond any sort of likely levels, but still). Maybe I'm doing something wrong, missing a configuration setting somewhere, put an unfair stress on the system, there's a bona fide bug - or, given the inherent difficulty in achieving perfect coherency between machines on what's visible on the mount, it's just impossible (?) to truly get it working under all situations. My eventual usecase is to set up a bunch of (gitolite) hosts that all are effectively identical and all seeing the same storage and clients can then be directed to any of them to get served. For the purpose of this query I assume it can be boiled down to be the same as n users working on their own workstations and accessing a central repo on an NFS share they all have mounted, using regular file paths. Correct? I have a number of load-generating test scripts (that from humble beginnings have grown to beasts), but basically, they fork a number of code pieces that proceed to hammer the repo with concurrent reading and writing. If necessary, the scripts can be started on different hosts, too. It's all about the central repo so clients will retry in various modes whenever they get an error, up to re-cloning and starting over. All in all, they can yield quite a load. On a local filesystem everything seems to be holding up fine which is expected. In the scenario with concurrency on top of shared NFS storage however, the scripts eventually fails with various problems (when the timing finally finds a hole, I guess) - possible to clone but checkouts fails, errors about refs pointing wrong and errors where the original repo is actually corrupted and can't be cloned from. Depending on test strategy, I've also had everything going fine (ops looks fine in my logs), but afterwards the repo has lost a branch or two. I've experimented with various NFS settings (e.g. turning off the attribute cache), but haven't reached a conclusion. I do suspect that it just is a fact of life with a remote filesystem to have coherency problems with high concurrency like this but I'd be happily proven wrong - I'm not an expert in either NFS or git. So, any opionions either way would be valuable, e.g. 'it should work' or 'it can never work 100%' is fine, as well as any suggestions. Obviously, the concurrency needed to make it probable to hit this seems so unlikely that maybe I just shouldn't worry... I'd be happy to share scripts and whatever if someone would like to try it out themselves. Thanks for your time, ken1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Composing git repositories
Junio C Hamano wrote: As I said in another thread, your top-level may be only a part in somebody else's project, and what you consider just a part of your project may be the whole project to somebody else. If you pick one location to store both for the above clone, e.g. cgit/.git (it could be cgit/.ram-git or any other name), embedding it in a yet larger project (perhaps having both cgit and gitolite to give a one-stop solution for hosting services) later would face the same issue as Ram seemed to be complaining. It needs to address what happens when that cgit/.git (or whatever name) gets in the way in the scope of the larger project. That is why I said Ram's rant, using subjective words like elegant, without sound technical justification, did not make much sense to me. I was having a lot of difficulty writing down my thoughts. Thank you for providing an illustrative example. It is terribly hard to do with our current implementation: we'd have to rewrite the gitdir: lines in all the .git files in the submodule trees and rebuild all the .git/modules paths. I'm thinking that we need to separate the object stores from the worktrees for good. For a project with no submodules, the object store can be present in .git/ of the toplevel directory, like it is now. The moment submodules are added, all the object stores should be relocated to a place outside the worktree. So my ~/src might look like: dotfiles.git/, auto-complete.git/, magit.git/, git-commit-mode.git/, yasnippet.git/ and dotfiles/. dotfiles/ contains lots of worktrees stitched together nicely, pointing to these object stores in ~/src. This would certainly get rid of the asymmetry for good. Now, we can focus our attention on composing git worktrees. What is a worktree? A tree object pointed to by the commit object referred to by HEAD. What we need to do is embed one tree inside another using a mediating object to establish repository boundaries, while not introducing an ugly seam. If you think about it, the mediator we've picked conveys little/ no information to the parent; it says: there's a commit with this SHA-1 present in this submodule, but I can't tell you the commit message, tree object, branch, remote, or anything else (obviously because the commit isn't present in the parent's object store). So, the mediator might as well have been a SHA-1 string. And we have an ugly .gitmodules conveying the remote and the branch. Why can't we stuff more information into the mediating object and get rid of .gitmodules altogether? Okay, here's a first draft of the new design. The new mediator object should look like: name = git ref = v1.7.8 The name is looked up in refs/modules/branch, which in turn looks like: [submodule git] origin = gh:artagnon/git path = git [submodule magit] origin = gh:magit/magit path = git/extensions/magit The ref could be 'master', 'HEAD~1', or even a commit SHA-1 (to do the current anchored-submodules). Finally, there's a .git file in the worktree, which contains a gitdir: line pointing to the object store, as before. This solves the two problems that I brought up earlier: - Floating submodules (which are _necessary_ if you don't want to propagate commits upwards to the root). - Initializing a nested submodule without having to initialize all the submodules in the path leading up to it. However, I suspect that we can put more information the mediator object to make life easier for the parent repository and make seams disappear. I'm currently thinking about what information git core needs to behave smoothly with submodules. -- To unsubscribe from this list: send the line 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/2] t5520 (pull): use test_config where appropriate
Configuration from test_config does not last beyond the end of the current test assertion, making each test easier to think about in isolation. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- Removed first hunk, as per Junio's comment. t/t5520-pull.sh | 18 +++--- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index e5adee8..8ea 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -96,8 +96,7 @@ test_expect_success '--rebase' ' ' test_expect_success 'pull.rebase' ' git reset --hard before-rebase - git config --bool pull.rebase true - test_when_finished git config --unset pull.rebase + test_config pull.rebase true git pull . copy test $(git rev-parse HEAD^) = $(git rev-parse copy) test new = $(git show HEAD:file2) @@ -105,8 +104,7 @@ test_expect_success 'pull.rebase' ' test_expect_success 'branch.to-rebase.rebase' ' git reset --hard before-rebase - git config --bool branch.to-rebase.rebase true - test_when_finished git config --unset branch.to-rebase.rebase + test_config branch.to-rebase.rebase true git pull . copy test $(git rev-parse HEAD^) = $(git rev-parse copy) test new = $(git show HEAD:file2) @@ -114,10 +112,8 @@ test_expect_success 'branch.to-rebase.rebase' ' test_expect_success 'branch.to-rebase.rebase should override pull.rebase' ' git reset --hard before-rebase - git config --bool pull.rebase true - test_when_finished git config --unset pull.rebase - git config --bool branch.to-rebase.rebase false - test_when_finished git config --unset branch.to-rebase.rebase + test_config pull.rebase true + test_config branch.to-rebase.rebase false git pull . copy test $(git rev-parse HEAD^) != $(git rev-parse copy) test new = $(git show HEAD:file2) @@ -171,9 +167,9 @@ test_expect_success 'pull --rebase dies early with dirty working directory' ' git update-ref refs/remotes/me/copy copy^ COPY=$(git rev-parse --verify me/copy) git rebase --onto $COPY copy - git config branch.to-rebase.remote me - git config branch.to-rebase.merge refs/heads/copy - git config branch.to-rebase.rebase true + test_config branch.to-rebase.remote me + test_config branch.to-rebase.merge refs/heads/copy + test_config branch.to-rebase.rebase true echo dirty file git add file test_must_fail git pull -- 1.8.2.141.g07926c6 -- To unsubscribe from this list: send the line 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] git-send-email.perl: implement suggestions made by perlcritic
Running perlcritic with gentle severity reports six problems. The
following lists the line numbers on which the problems occur, along
with a description of the problem. This patch fixes them all, after
carefully considering the consequences.
516: Contrary to common belief, subroutine prototypes do not enable
compile-time checks for proper arguments. They serve the singular
purpose of defining functions that behave like built-in functions, but
check_file_rev_conflict was never intended to behave like one. We
have verified that the callers of the subroutines are alright with the
change.
714, 836, 855, 1487: Returning `undef' upon failure from a subroutine
is pretty common. But if the subroutine is called in list context, an
explicit `return undef;' statement will return a one-element list
containing `(undef)'. Now if that list is subsequently put in a
boolean context to test for failure, then it evaluates to true. But
you probably wanted it to be false. The solution is to just use a
bare `return' statement whenever you want to return failure. In list
context, Perl will then give you an empty list (which is false), and
`undef' in scalar context (which is also false).
1441: The three-argument form of `open' (introduced in Perl 5.6)
prevents subtle bugs that occur when the filename starts with funny
characters like '' or ''. It's also more explicitly clear to define
the input mode of the file. This policy will not complain if the file
explicitly states that it is compatible with a version of Perl prior
to 5.6 via an include statement (see 'use 5.008' near the top of the
file).
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
Junio: In future, please tell me explicitly that you're expecting a
re-roll with an updated commit message. It wasn't obvious to me at
all.
git-send-email.perl | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index c3501d9..633f447 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -513,7 +513,7 @@ if (@alias_files and $aliasfiletype and defined
$parse_alias{$aliasfiletype}) {
($sender) = expand_aliases($sender) if defined $sender;
# returns 1 if the conflict must be solved using it as a format-patch argument
-sub check_file_rev_conflict($) {
+sub check_file_rev_conflict {
return unless $repo;
my $f = shift;
try {
@@ -711,7 +711,7 @@ sub ask {
}
}
}
- return undef;
+ return;
}
my %broken_encoding;
@@ -833,7 +833,7 @@ sub extract_valid_address {
# less robust/correct than the monster regexp in Email::Valid,
# but still does a 99% job, and one less dependency
return $1 if $address =~ /($local_part_regexp\@$domain_regexp)/;
- return undef;
+ return;
}
sub extract_valid_address_or_die {
@@ -852,7 +852,7 @@ sub validate_address {
valid_re = qr/^(?:quit|q|drop|d|edit|e)/i,
default = 'q');
if (/^d/i) {
- return undef;
+ return;
} elsif (/^q/i) {
cleanup_compose_files();
exit(0);
@@ -1453,7 +1453,7 @@ sub recipients_cmd {
my $sanitized_sender = sanitize_address($sender);
my @addresses = ();
- open my $fh, $cmd \Q$file\E |
+ open my $fh, q{-|}, $cmd \Q$file\E
or die ($prefix) Could not execute '$cmd';
while (my $address = $fh) {
$address =~ s/^\s*//g;
@@ -1499,7 +1499,7 @@ sub validate_patch {
return $.: patch contains a line longer than 998
characters;
}
}
- return undef;
+ return;
}
sub file_has_nonascii {
--
1.8.2.141.g07926c6
--
To unsubscribe from this list: send the line unsubscribe 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] use refnames instead of left/right in dirdiffs
Hi John.
On Wed, 2013-03-27 at 23:07 +, John Keeping wrote:
That's not going to work well on Windows, is it?
Uhm Winwhat? No seriously... doesn't dir-diff fail ther anway? The mkdir
right now also uses mkpath with /... and I could read in it's
documentation that it would automatically translate this, does it?
Anything with two dots
in is already forbidden so we don't need to worry about that
Sure about this? I mean they're forbidden as refnames, but is this
really checked within git-difftool before?
; I'm not
sure we need to remove forward slashes at all
Mhh could be... seems that the cleanup happens via completely removing
the $tmpdir path.
And for the actual diff it shouldn't matter when there's a partentdir
more.
until we consider the
commit containing syntax ':/fix nasty bug' or 'master^{/fix bug}'.
Phew... don't ask me... I'm not that into the git source code... from
the filename side, these shouldn't bother, only / an NUL is forbidden
(in POSIX,... again I haven't checked win/mac which might not adhere to
the standards).
I'm more concerned with specifiers containing '^', '@', '{', ':' - see
'SPECIFYING REVISIONS' in git-rev-parse(1) for the full details of
what's acceptable.
Mhh there are likely more problems... I just noted that $ldir/$rdir are
used in a call to system() so... that needs to be shell escaped to
prevent any bad stuff
And if perl (me not a perl guy) interprets any such characters specially
in strings, it must be handled either.
At some point I think it may be better to fall back
to the SHA1 of the relevant commit.
Think that would be quite a drawback...
Cheers,
Chris.
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH 3/6] t5516 (fetch-push): introduce mk_test_with_name()
Jeff King wrote: Subject: [PATCH] t5516: drop implicit arguments from helper functions Thanks a lot for this! I just had to s/ $repo_name/ $repo_name/ to fix the quoting. Will post a re-roll soon. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bug in git rev-parse --verify
On Junio's master, git rev-parse --verify accepts *any* 40-digit hexadecimal number. For example, pass it 40 1 characters, and it accepts the argument: $ git rev-parse --verify $ echo $? 0 Obviously, my repo doesn't have an object with this hash :-) so I think this argument should be rejected. If you add or remove a digit (to make the length different than 40), it is correctly rejected: $ git rev-parse --verify 111 fatal: Needed a single revision $ echo $? 128 I believe that git rev-parse --verify is meant to verify that the argument is an actual object, and that it should reject fictional SHA1s. (If not then the documentation should be clarified.) The same problem also exists in 1.8.2 but I haven't checked how much older it is. The behavior presumably comes from the following clause in get_sha1_basic(): if (len == 40 !get_sha1_hex(str, sha1)) return 0; I won't have time to pursue this. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/6] Support triangular workflows
Hi, The changes in this round are: 1. Peff submitted a patch to squash into [3/6]. Since his patch essentially reverts mine, I've blamed him for the change. 2. Peff suggested a code movement in [5/6] to make things flow more naturally. 3. Jonathan suggested a better test description in [2/6]. 4. Junio suggested a minor documentation update in [6/6]. My build of git has had this feature for two weeks now (since the first iteration), and I'm very happy with it. Jeff King (1): t5516 (fetch-push): drop implicit arguments from helper functions Ramkumar Ramachandra (5): remote.c: simplify a bit of code using git_config_string() t5516 (fetch-push): update test description remote.c: introduce a way to have different remotes for fetch/push remote.c: introduce remote.pushdefault remote.c: introduce branch.name.pushremote Documentation/config.txt | 24 +++- builtin/push.c | 2 +- remote.c | 41 -- remote.h | 1 + t/t5516-fetch-push.sh| 316 +++ 5 files changed, 238 insertions(+), 146 deletions(-) -- 1.8.2.141.g3797f84 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/6] remote.c: simplify a bit of code using git_config_string()
A small segment where handle_config() parses the branch.remote
configuration variable can be simplified using git_config_string().
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
remote.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/remote.c b/remote.c
index 174e48e..02e6c4c 100644
--- a/remote.c
+++ b/remote.c
@@ -357,9 +357,8 @@ static int handle_config(const char *key, const char
*value, void *cb)
return 0;
branch = make_branch(name, subkey - name);
if (!strcmp(subkey, .remote)) {
- if (!value)
- return config_error_nonbool(key);
- branch-remote_name = xstrdup(value);
+ if (git_config_string(branch-remote_name, key, value))
+ return -1;
if (branch == current_branch) {
default_remote_name = branch-remote_name;
explicit_default_remote_name = 1;
--
1.8.2.141.g3797f84
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/6] t5516 (fetch-push): update test description
The file was originally created in bcdb34f (Test wildcard push/fetch, 2007-06-08), and only contained tests that exercised wildcard functionality at the time. In subsequent commits, many other tests unrelated to wildcards were added but the test description was never updated. Fix this. Helped-by: Jonathan Nieder jrnie...@gmail.com Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- t/t5516-fetch-push.sh | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 6fd125a..38f8fc0 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1,6 +1,17 @@ #!/bin/sh -test_description='fetching and pushing, with or without wildcard' +test_description='Basic fetch/push functionality. + +This test checks the following functionality: + +* command-line syntax +* refspecs +* fast-forward detection, and overriding it +* configuration +* hooks +* --porcelain output format +* hiderefs +' . ./test-lib.sh -- 1.8.2.141.g3797f84 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/6] t5516 (fetch-push): drop implicit arguments from helper functions
From: Jeff King p...@peff.net
Many of the tests in t5516 look like:
mk_empty
git push testrepo ...
check_push_result $commit heads/master
It's reasonably easy to see what is being tested, with the
exception that testrepo is a magic global name (it is
implicitly used in the helpers, but we have to name it
explicitly when calling git directly). Let's make it
explicit when call the helpers, too. This is slightly more
typing, but makes the test snippets read more naturally.
It also makes it easy for future tests to use an alternate
or multiple repositories, without a proliferation of helper
functions.
[rr: fixed sloppy quoting]
Signed-off-by: Jeff King p...@peff.net
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
t/t5516-fetch-push.sh | 276 ++
1 file changed, 142 insertions(+), 134 deletions(-)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 38f8fc0..94e0189 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -18,10 +18,11 @@ This test checks the following functionality:
D=`pwd`
mk_empty () {
- rm -fr testrepo
- mkdir testrepo
+ repo_name=$1
+ rm -fr $repo_name
+ mkdir $repo_name
(
- cd testrepo
+ cd $repo_name
git init
git config receive.denyCurrentBranch warn
mv .git/hooks .git/hooks-disabled
@@ -29,16 +30,19 @@ mk_empty () {
}
mk_test () {
- mk_empty
+ repo_name=$1
+ shift
+
+ mk_empty $repo_name
(
for ref in $@
do
- git push testrepo $the_first_commit:refs/$ref || {
+ git push $repo_name $the_first_commit:refs/$ref || {
echo Oops, push refs/$ref failure
exit 1
}
done
- cd testrepo
+ cd $repo_name
for ref in $@
do
r=$(git show-ref -s --verify refs/$ref)
@@ -52,9 +56,10 @@ mk_test () {
}
mk_test_with_hooks() {
+ repo_name=$1
mk_test $@
(
- cd testrepo
+ cd $repo_name
mkdir .git/hooks
cd .git/hooks
@@ -86,13 +91,16 @@ mk_test_with_hooks() {
}
mk_child() {
- rm -rf $1
- git clone testrepo $1
+ rm -rf $2
+ git clone $1 $2
}
check_push_result () {
+ repo_name=$1
+ shift
+
(
- cd testrepo
+ cd $repo_name
it=$1
shift
for ref in $@
@@ -124,7 +132,7 @@ test_expect_success setup '
'
test_expect_success 'fetch without wildcard' '
- mk_empty
+ mk_empty testrepo
(
cd testrepo
git fetch .. refs/heads/master:refs/remotes/origin/master
@@ -137,7 +145,7 @@ test_expect_success 'fetch without wildcard' '
'
test_expect_success 'fetch with wildcard' '
- mk_empty
+ mk_empty testrepo
(
cd testrepo
git config remote.up.url ..
@@ -152,7 +160,7 @@ test_expect_success 'fetch with wildcard' '
'
test_expect_success 'fetch with insteadOf' '
- mk_empty
+ mk_empty testrepo
(
TRASH=$(pwd)/
cd testrepo
@@ -169,7 +177,7 @@ test_expect_success 'fetch with insteadOf' '
'
test_expect_success 'fetch with pushInsteadOf (should not rewrite)' '
- mk_empty
+ mk_empty testrepo
(
TRASH=$(pwd)/
cd testrepo
@@ -186,7 +194,7 @@ test_expect_success 'fetch with pushInsteadOf (should not
rewrite)' '
'
test_expect_success 'push without wildcard' '
- mk_empty
+ mk_empty testrepo
git push testrepo refs/heads/master:refs/remotes/origin/master
(
@@ -199,7 +207,7 @@ test_expect_success 'push without wildcard' '
'
test_expect_success 'push with wildcard' '
- mk_empty
+ mk_empty testrepo
git push testrepo refs/heads/*:refs/remotes/origin/*
(
@@ -212,7 +220,7 @@ test_expect_success 'push with wildcard' '
'
test_expect_success 'push with insteadOf' '
- mk_empty
+ mk_empty testrepo
TRASH=$(pwd)/
git config url.$TRASH.insteadOf trash/
git push trash/testrepo refs/heads/master:refs/remotes/origin/master
@@ -226,7 +234,7 @@ test_expect_success 'push with insteadOf' '
'
test_expect_success 'push with pushInsteadOf' '
- mk_empty
+ mk_empty testrepo
TRASH=$(pwd)/
git config url.$TRASH.pushInsteadOf trash/
git push trash/testrepo refs/heads/master:refs/remotes/origin/master
@@ -240,7 +248,7 @@ test_expect_success 'push with pushInsteadOf' '
'
test_expect_success 'push with pushInsteadOf and explicit pushurl
(pushInsteadOf
[PATCH 4/6] remote.c: introduce a way to have different remotes for fetch/push
Currently, do_push() in push.c calls remote_get(), which gets the
configured remote for fetching and pushing. Replace this call with a
call to pushremote_get() instead, a new function that will return the
remote configured specifically for pushing. This function tries to
work with the string pushremote_name, before falling back to the
codepath of remote_get(). This patch has no visible impact, but
serves to enable future patches to introduce configuration variables
to set pushremote_name. For example, you can now do the following in
handle_config():
if (!strcmp(key, remote.pushdefault))
git_config_string(pushremote_name, key, value);
Then, pushes will automatically go to the remote specified by
remote.pushdefault.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
builtin/push.c | 2 +-
remote.c | 25 +
remote.h | 1 +
3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 42b129d..d447a80 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -322,7 +322,7 @@ static int push_with_options(struct transport *transport,
int flags)
static int do_push(const char *repo, int flags)
{
int i, errs;
- struct remote *remote = remote_get(repo);
+ struct remote *remote = pushremote_get(repo);
const char **url;
int url_nr;
diff --git a/remote.c b/remote.c
index 02e6c4c..b733aec 100644
--- a/remote.c
+++ b/remote.c
@@ -49,6 +49,7 @@ static int branches_nr;
static struct branch *current_branch;
static const char *default_remote_name;
+static const char *pushremote_name;
static int explicit_default_remote_name;
static struct rewrites rewrites;
@@ -670,17 +671,21 @@ static int valid_remote_nick(const char *name)
return !strchr(name, '/'); /* no slash */
}
-struct remote *remote_get(const char *name)
+static struct remote *remote_get_1(const char *name, const char
*pushremote_name)
{
struct remote *ret;
int name_given = 0;
- read_config();
if (name)
name_given = 1;
else {
- name = default_remote_name;
- name_given = explicit_default_remote_name;
+ if (pushremote_name) {
+ name = pushremote_name;
+ name_given = 1;
+ } else {
+ name = default_remote_name;
+ name_given = explicit_default_remote_name;
+ }
}
ret = make_remote(name, 0);
@@ -699,6 +704,18 @@ struct remote *remote_get(const char *name)
return ret;
}
+struct remote *remote_get(const char *name)
+{
+ read_config();
+ return remote_get_1(name, NULL);
+}
+
+struct remote *pushremote_get(const char *name)
+{
+ read_config();
+ return remote_get_1(name, pushremote_name);
+}
+
int remote_is_configured(const char *name)
{
int i;
diff --git a/remote.h b/remote.h
index f7b08f1..5fe5f53 100644
--- a/remote.h
+++ b/remote.h
@@ -51,6 +51,7 @@ struct remote {
};
struct remote *remote_get(const char *name);
+struct remote *pushremote_get(const char *name);
int remote_is_configured(const char *name);
typedef int each_remote_fn(struct remote *remote, void *priv);
--
1.8.2.141.g3797f84
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/6] remote.c: introduce remote.pushdefault
This new configuration variable defines the default remote to push to,
and overrides `branch.name.remote` for all branches. It is useful
in the typical triangular-workflow setup, where the remote you're
fetching from is different from the remote you're pushing to.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
Documentation/config.txt | 13 ++---
remote.c | 7 +++
t/t5516-fetch-push.sh| 12
3 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index c1f435f..dd78171 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -727,9 +727,12 @@ branch.autosetuprebase::
This option defaults to never.
branch.name.remote::
- When in branch name, it tells 'git fetch' and 'git push' which
- remote to fetch from/push to. It defaults to `origin` if no remote is
- configured. `origin` is also used if you are not on any branch.
+ When on branch name, it tells 'git fetch' and 'git push'
+ which remote to fetch from/push to. The remote to push to
+ may be overridden with `remote.pushdefault` (for all branches).
+ If no remote is configured, or if you are not on any branch,
+ it defaults to `origin` for fetching and `remote.pushdefault`
+ for pushing.
branch.name.merge::
Defines, together with branch.name.remote, the upstream branch
@@ -1898,6 +1901,10 @@ receive.updateserverinfo::
If set to true, git-receive-pack will run git-update-server-info
after receiving data from git-push and updating refs.
+remote.pushdefault::
+ The remote to push to by default. Overrides
+ `branch.name.remote` for all branches.
+
remote.name.url::
The URL of a remote repository. See linkgit:git-fetch[1] or
linkgit:git-push[1].
diff --git a/remote.c b/remote.c
index b733aec..49c4b8b 100644
--- a/remote.c
+++ b/remote.c
@@ -389,9 +389,16 @@ static int handle_config(const char *key, const char
*value, void *cb)
add_instead_of(rewrite, xstrdup(value));
}
}
+
if (prefixcmp(key, remote.))
return 0;
name = key + 7;
+
+ /* Handle remote.* variables */
+ if (!strcmp(name, pushdefault))
+ return git_config_string(pushremote_name, key, value);
+
+ /* Handle remote.name.* variables */
if (*name == '/') {
warning(Config remote shorthand cannot begin with '/': %s,
name);
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 94e0189..ac1ec9d 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -520,6 +520,18 @@ test_expect_success 'push with config remote.*.push =
HEAD' '
git config --remove-section remote.there
git config --remove-section branch.master
+test_expect_success 'push with remote.pushdefault' '
+ mk_test up_repo heads/frotz
+ mk_test down_repo heads/master
+ test_config remote.up.url up_repo
+ test_config remote.down.url down_repo
+ test_config branch.master.remote up
+ test_config remote.pushdefault down
+ git push
+ test_must_fail check_push_result up_repo $the_commit heads/master
+ check_push_result down_repo $the_commit heads/master
+'
+
test_expect_success 'push with config remote.*.pushurl' '
mk_test testrepo heads/master
--
1.8.2.141.g3797f84
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/6] remote.c: introduce branch.name.pushremote
This new configuration variable overrides `remote.pushdefault` and
`branch.name.remote` for pushes. When you pull from one
place (e.g. your upstream) and push to another place (e.g. your own
publishing repository), you would want to set `remote.pushdefault` to
specify the remote to push to for all branches, and use this option to
override it for a specific branch.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
Documentation/config.txt | 19 +++
remote.c | 4
t/t5516-fetch-push.sh| 15 +++
3 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index dd78171..ec579c5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -730,9 +730,19 @@ branch.name.remote::
When on branch name, it tells 'git fetch' and 'git push'
which remote to fetch from/push to. The remote to push to
may be overridden with `remote.pushdefault` (for all branches).
- If no remote is configured, or if you are not on any branch,
- it defaults to `origin` for fetching and `remote.pushdefault`
- for pushing.
+ The remote to push to, for the current branch, may be further
+ overridden by `branch.name.pushremote`. If no remote is
+ configured, or if you are not on any branch, it defaults to
+ `origin` for fetching and `remote.pushdefault` for pushing.
+
+branch.name.pushremote::
+ When on branch name, it overrides `branch.name.remote` for
+ pushing. It also overrides `remote.pushdefault` for pushing
+ from branch name. When you pull from one place (e.g. your
+ upstream) and push to another place (e.g. your own publishing
+ repository), you would want to set `remote.pushdefault` to
+ specify the remote to push to for all branches, and use this
+ option to override it for a specific branch.
branch.name.merge::
Defines, together with branch.name.remote, the upstream branch
@@ -1903,7 +1913,8 @@ receive.updateserverinfo::
remote.pushdefault::
The remote to push to by default. Overrides
- `branch.name.remote` for all branches.
+ `branch.name.remote` for all branches, and is overridden by
+ `branch.name.pushremote` for specific branches.
remote.name.url::
The URL of a remote repository. See linkgit:git-fetch[1] or
diff --git a/remote.c b/remote.c
index 49c4b8b..e89b1b7 100644
--- a/remote.c
+++ b/remote.c
@@ -364,6 +364,10 @@ static int handle_config(const char *key, const char
*value, void *cb)
default_remote_name = branch-remote_name;
explicit_default_remote_name = 1;
}
+ } else if (!strcmp(subkey, .pushremote)) {
+ if (branch == current_branch)
+ if (git_config_string(pushremote_name, key,
value))
+ return -1;
} else if (!strcmp(subkey, .merge)) {
if (!value)
return config_error_nonbool(key);
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index ac1ec9d..13028a4 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -542,6 +542,21 @@ test_expect_success 'push with config remote.*.pushurl' '
check_push_result testrepo $the_commit heads/master
'
+test_expect_success 'push with config branch.*.pushremote' '
+ mk_test up_repo heads/frotz
+ mk_test side_repo heads/quux
+ mk_test down_repo heads/master
+ test_config remote.up.url up_repo
+ test_config remote.pushdefault side_repo
+ test_config remote.down.url down_repo
+ test_config branch.master.remote up
+ test_config branch.master.pushremote down
+ git push
+ test_must_fail check_push_result up_repo $the_commit heads/master
+ test_must_fail check_push_result side_repo $the_commit heads/master
+ check_push_result down_repo $the_commit heads/master
+'
+
# clean up the cruft left with the previous one
git config --remove-section remote.there
--
1.8.2.141.g3797f84
--
To unsubscribe from this list: send the line unsubscribe 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 subtree oddity
I built v1.8.2 last evening and found that the subtree command isn't supported. What version of git are you using? And where did you get it? SPS Sent from my iPhone On Mar 27, 2013, at 8:12 PM, Thomas Taranowski t...@baringforge.com wrote: I'd like to have the following configuration: /myproject.git |__/upstream_dependency -- Points to a remote library git repo |__/project_source -- local project source I issue the following commands to pull in the upstream dependency as a subtree of the myproject.git repo: git remote add upstream git://gnuradio.org/gnuradio git fetch upstream git checkout master git subtree add -P upstream upstream/master Now, my expectation is that I would have the following: /myproject.git |__/upstream_dependency -- Points to a remote library git repo | all the upstream files are present here, as expected |__/project_source this is still intact, as expected |__ all the upstream files are present here. wtf? My question is, why does subtree add pull in all the subtree files into the root of the repo, and not just into the specified subtree prefix? # # Here's an excerpt of what I see: # $:~/scratch/myproject.git$ ls AUTHORS gr-comedi gr-utils cmake gr-digital gr-video-sdl CMakeLists.txt gr-fcd gr-vocoder config.h.in gr-fft gr-wavelet COPYING gr-filter gr-wxgui docsgr-howto-write-a-block README dtools gr-noaa README.building-boost gnuradio-core gr-pagerREADME.hacking gr-analog gr-qtguiREADME-win32-mingw-short.txt gr-atsc gr-shd upstream the subtree directory gr-audiogr-trellis volk gr-blocks gruel grc gr-uh # # Also, those same files are in the upstream subtree directory as well (as expected) # $:~/scratch/myproject.git$ ls upstream AUTHORS grc gruel cmake gr-comedi gr-uhd CMakeLists.txt gr-digital gr-utils config.h.in gr-fcd gr-video-sdl COPYING gr-fft gr-vocoder docsgr-filter gr-wavelet dtools gr-howto-write-a-block gr-wxgui gnuradio-core gr-noaa README gr-analog gr-pagerREADME.building-boost gr-atsc gr-qtguiREADME.hacking gr-audiogr-shd README-win32-mingw-short.txt gr-blocks gr-trellis volk -- To unsubscribe from this list: send the line 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: propagating repo corruption across clone
On Tue, Mar 26, 2013 at 11:47 PM, Junio C Hamano gits...@pobox.com wrote: The difference between --mirror and no --mirror is a red herring. You may want to ask Jeff Mitchell to remove the mention of it; it only adds to the confusion without helping users. If you made byte-for-byte copy of corrupt repository, it wouldn't make any difference if the first checkout notices it. Hi, Several days ago I had actually already updated the post to indicate that my testing methodology was incorrect as a result of mixing up --no-hardlinks and --no-local, and pointed to this thread. I will say that we did see corrupted repos on the downstream mirrors. I don't have an explanation for it, but have not been able to reproduce it either. My only potential guess (untested) is that perhaps when the corruption was detected the clone aborted but left the objects already transferred locally. Again, untested -- I mention it only because it's my only potential explanation :-) To be paranoid, you may want to set transfer.fsckObjects to true, perhaps in your ~/.gitconfig. Interesting; I'd known about receive.fsckObjects but not transfer/fetch. Thanks for the pointer. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
Ramkumar Ramachandra artag...@gmail.com writes: The changes in this round are: 1. Peff submitted a patch to squash into [3/6]. Since his patch essentially reverts mine, I've blamed him for the change. 2. Peff suggested a code movement in [5/6] to make things flow more naturally. 3. Jonathan suggested a better test description in [2/6]. 4. Junio suggested a minor documentation update in [6/6]. My build of git has had this feature for two weeks now (since the first iteration), and I'm very happy with it. Will take a look; thanks for a reroll. We may need a bit of adjustment to the longer term plan for the push.default topic, as I expect we will have this feature a lot sooner (e.g. perhaps in 1.8.4) than we will switch the push default to simple. The description of simple and upstream still is written around the you push to and pull from the same place, and may need to be updated to explain what happens if you are employing a triangular workflow? Or it could turn out that triangular people may be better served by a push.default different from simple or upstream. Or it may turn out that we do not need any change to what is queued on the push-2.0-default-to-simple topic (I haven't thought things through). It is not urgent, but please start thinking about how you can help, now you introduced the triangular stuff. Thanks. Jeff King (1): t5516 (fetch-push): drop implicit arguments from helper functions Ramkumar Ramachandra (5): remote.c: simplify a bit of code using git_config_string() t5516 (fetch-push): update test description remote.c: introduce a way to have different remotes for fetch/push remote.c: introduce remote.pushdefault remote.c: introduce branch.name.pushremote Documentation/config.txt | 24 +++- builtin/push.c | 2 +- remote.c | 41 -- remote.h | 1 + t/t5516-fetch-push.sh| 316 +++ 5 files changed, 238 insertions(+), 146 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
Re: [PATCH v2 2/2] t5520 (pull): use test_config where appropriate
Junio C Hamano gits...@pobox.com writes: Ramkumar Ramachandra artag...@gmail.com writes: Configuration from test_config does not last beyond the end of the current test assertion, making each test easier to think about in isolation. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- Removed first hunk, as per Junio's comment. Thanks, but doesn't yd/use-test-config-unconfig topic already address this? The last hunk from your version was missing from 9d6aa64dc32b (t5520: use test_config to set/unset git config variables, 2013-03-24); I'll pick that part and apply as a follow up to the series. No need to resend; 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: Git and GSoC 2013
Jeff King wrote: There was a big thread about a month ago on whether Git should do Google Summer of Code this year[1]. Take only one or two students and get the entire community involved in learning from the GSoC experience, so we can do a bigger one next year. -- To unsubscribe from this list: send the line unsubscribe 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: Move hard-coded colors to .gitk
Gauthier Östervall wrote: Screenshot of my current coloring setup using this patch, based on zenburn: http://s11.postimg.org/hozbtsfj7/gitk_zenburn.png And the .gitk used to that end: https://gist.github.com/fleutot/5253281 This is a really cool color theme. Would we consider shipping some themes with gitk, in contrib/ perhaps? -- To unsubscribe from this list: send the line unsubscribe 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 in git rev-parse --verify
On 03/28/2013 02:48 PM, Junio C Hamano wrote:
I think it has always been about is this well formed and we can turn it
into a raw 20-byte object name? and never aboutdoes it exist?
That's surprising. The man page says
--verify
The parameter given must be usable as a single, valid object name.
Otherwise barf and abort.
Valid, to me, implies that the parameter should be the name of an
actual object, and this also seems a more useful concept to me and more
consistent with the command's behavior when passed other arguments.
Is there a simple way to verify an object name more strictly and convert
it to an SHA1? I can only think of solutions that require two commands,
like
git cat-file -e $ARG git rev-parse --verify $ARG
I suppose in most contexts where one wants to know whether an object
name is valid, one should also verify that the object has the type that
you expect:
test X$(git cat-file -t $ARG) = Xcommit
git rev-parse --verify $ARG
or (allowing tag dereferencing)
git cat-file -e $ARG^{commit}
git rev-parse --verify $ARG^{commit}
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
On Thu, Mar 28, 2013 at 06:56:36PM +0530, Ramkumar Ramachandra wrote:
Jeff King (1):
t5516 (fetch-push): drop implicit arguments from helper functions
Ramkumar Ramachandra (5):
remote.c: simplify a bit of code using git_config_string()
t5516 (fetch-push): update test description
remote.c: introduce a way to have different remotes for fetch/push
remote.c: introduce remote.pushdefault
remote.c: introduce branch.name.pushremote
Thanks, this iteration looks pretty good. I have one minor nit, which is
that the tests in patches 5 and 6 do things like:
+ git push
+ test_must_fail check_push_result up_repo $the_commit heads/master
+ check_push_result down_repo $the_commit heads/master
Using test_must_fail here caught my eye, because usually it is meant to
check failure of a single git command. When it (or !, for that matter)
is used with a compound function, you end up losing robustness in corner
cases.
For example, imagine your function is:
check_foo() {
cd $1
git foo
}
and you expect in some cases that git foo will succeed and in some
cases it will fail. In the affirmative case (running check_foo), this
is robust; if any of the steps fails, the test fails.
But if you run the negative case (test_must_fail check_foo), you will
also fail if any of the preparatory steps fail. I.e., you wanted to say:
cd $1 test_must_fail git foo
but you actually said (applying De Morgan's laws):
test_must_fail cd $1 || test_must_fail git foo
Now we probably don't expect the cd to fail here, but of course the
other steps can be more complicated, too. In your case, I think the
effect you are looking for is that heads/master != $the_commit. But
note that we would also fail if git fsck fails in the pushed
repository, which is not what we want.
Sometimes it's annoyingly verbose to break down a compound function. But
I think in this case, you can make your tests more robust by just
checking the affirmative that the ref is still where we expect it to be,
like:
check_push_result up_repo $the_first_commit heads/master
Sorry if that was a bit long-winded. I think that practically speaking,
it is not a likely source of problems in this case. But it's an
anti-pattern in our tests that I think is worth mentioning.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Josh Triplett j...@joshtriplett.org writes: On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote: ... The test that checked that pushInsteadOf + pushurl shouldn't work as I expect was actually broken; I have removed it, updated the documentation, and sent a new patch to the list. There's an argument for either behavior as valid. My original patch specifically documented and tested for the opposite behavior, namely that pushurl overrides any pushInsteadOf, because I intended pushInsteadOf as a fallback if you don't have an explicit pushurl set. For only this bit. I think the test in question is this one from t5516: test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty TRASH=$(pwd)/ git config url.trash2/.pushInsteadOf trash/ git config remote.r.url trash/wrong git config remote.r.pushurl $TRASH/testrepo git push r refs/heads/master:refs/remotes/origin/master ( cd testrepo r=$(git show-ref -s --verify refs/remotes/origin/master) test z$r = z$the_commit test 1 = $(git for-each-ref refs/remotes/origin | wc -l) ) ' It defines a remote r, with URL trash/wrong (used for fetch) and pushURL $(pwd)/testrepo (used for push). There is a pushInsteadOf rule to rewrite anything that goes to trash/* to be pushed to trash2/* instead but that shouldn't be used to rewrite an explicit pushURL. And then the test pushes to r and checks if testrepo gets updated; in other words, it wants to make sure remote.r.pushURL defines the final destination, without pushInsteadOf getting in the way. But $(pwd)/testrepo does not match trash/* in the first place, so there is no chance for a pushInsteadOf to interfere; it looks to me that it is not testing what it wants to test. Perhaps we should do something like this? We tell it to push to testrepo/ with pushURL, and set up a pushInsteadOf to rewrite testrepo/ to trash2/, but because for this push it comes from an explicit pushURL, it still goes to testrepo/. As we do not have trash2/ repository, the test not just tests the push goes to testrepo/, but it also tests that it does not attempt to push to trash2/, checking both sides of the coin. t/t5516-fetch-push.sh | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index d3dc5df..b5ea32c 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -230,10 +230,9 @@ test_expect_success 'push with pushInsteadOf' ' test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty - TRASH=$(pwd)/ - git config url.trash2/.pushInsteadOf trash/ + git config url.trash2/.pushInsteadOf testrepo/ git config remote.r.url trash/wrong - git config remote.r.pushurl $TRASH/testrepo + git config remote.r.pushurl testrepo/ git push r refs/heads/master:refs/remotes/origin/master ( cd testrepo -- To unsubscribe from this list: send the line unsubscribe 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 help config: s/insn/instruction/
Matthias Krüger matthias.krue...@famsik.de writes: On 03/28/2013 06:59 AM, Junio C Hamano wrote: Matthias Krüger matthias.krue...@famsik.de writes: insn appears to be an in-code abbreviation and should not appear in manual/help pages. --- Thanks; sign-off? Oops, sorry. Signed-off-by: Matthias Krüger matthias.krue...@famsik.de (Is this sufficient or do I have to re-send the patch with the sign-off line?) I can squash it in, so there is no need to resend. The more important thing is I won't have to for your future patches. 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] push: Alias pushurl from push rewrites
On Thu, Mar 28, 2013 at 08:37:58AM -0700, Junio C Hamano wrote: Josh Triplett j...@joshtriplett.org writes: On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote: ... The test that checked that pushInsteadOf + pushurl shouldn't work as I expect was actually broken; I have removed it, updated the documentation, and sent a new patch to the list. There's an argument for either behavior as valid. My original patch specifically documented and tested for the opposite behavior, namely that pushurl overrides any pushInsteadOf, because I intended pushInsteadOf as a fallback if you don't have an explicit pushurl set. For only this bit. I think the test in question is this one from t5516: test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty TRASH=$(pwd)/ git config url.trash2/.pushInsteadOf trash/ git config remote.r.url trash/wrong git config remote.r.pushurl $TRASH/testrepo git push r refs/heads/master:refs/remotes/origin/master ( cd testrepo r=$(git show-ref -s --verify refs/remotes/origin/master) test z$r = z$the_commit test 1 = $(git for-each-ref refs/remotes/origin | wc -l) ) ' It defines a remote r, with URL trash/wrong (used for fetch) and pushURL $(pwd)/testrepo (used for push). There is a pushInsteadOf rule to rewrite anything that goes to trash/* to be pushed to trash2/* instead but that shouldn't be used to rewrite an explicit pushURL. And then the test pushes to r and checks if testrepo gets updated; in other words, it wants to make sure remote.r.pushURL defines the final destination, without pushInsteadOf getting in the way. But $(pwd)/testrepo does not match trash/* in the first place, so there is no chance for a pushInsteadOf to interfere; it looks to me that it is not testing what it wants to test. That test does actually test something important: it tests that the result of applying pushInsteadOf to url does *not* override pushurl. Not the same thing as testing that pushInsteadOf does or does not apply to pushurl. The relevant sentence of the git-config manpage (in the documentation for pushInsteadOf) says: If a remote has an explicit pushurl, git will ignore this setting for that remote. That really meant what I just said above: git will prefer an explicit pushurl over the pushInsteadOf rewrite of url. It says nothing about applying pushInsteadOf to rewrite pushurl. Perhaps we should do something like this? We tell it to push to testrepo/ with pushURL, and set up a pushInsteadOf to rewrite testrepo/ to trash2/, but because for this push it comes from an explicit pushURL, it still goes to testrepo/. As we do not have trash2/ repository, the test not just tests the push goes to testrepo/, but it also tests that it does not attempt to push to trash2/, checking both sides of the coin. Sensible test, assuming you want to enforce that behavior. I don't strongly care either way about that one, since it only applies if your pushInsteadOf rewrites could apply to your pushurl, and I only ever use pushInsteadOf to rewrite unpushable repos to pushable ones. However... t/t5516-fetch-push.sh | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index d3dc5df..b5ea32c 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -230,10 +230,9 @@ test_expect_success 'push with pushInsteadOf' ' test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty - TRASH=$(pwd)/ - git config url.trash2/.pushInsteadOf trash/ + git config url.trash2/.pushInsteadOf testrepo/ git config remote.r.url trash/wrong - git config remote.r.pushurl $TRASH/testrepo + git config remote.r.pushurl testrepo/ git push r refs/heads/master:refs/remotes/origin/master ( cd testrepo ...the test you describe should appear in *addition* to this test, not replacing it, because as described above this test does test one critical bit of behavior, namely prefering pushurl over the pushInsteadOf rewrite of url. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe 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] push: Alias pushurl from push rewrites
Josh Triplett j...@joshtriplett.org writes: OK, I take it back. I *can* imagine configurations that this change would break, since it does change intentional and documented behavior, but I don't have any such configuration. The only such configuration I can imagine involves directly counting on the non-rewriting of pushUrl, by using pushInsteadOf to rewrite urls and then sometimes using pushUrl to override that and point back at the un-rewritten URL. And while supported, that does seem *odd*. Objection withdrawn; if nobody can come up with a sensible configuration that relies on the documented behavior, I don't particularly care if it changes. I actually do. Given the popularity of the system, people involved in this thread cannot imagine a case that existing people may get hurt is very different from this is not a regression. After merging this change when people start complaining, you and Rob can hide and ignore them, but we collectively as the Git project have to have a way to help them when it happens. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: which files will have conflicts between two branches?
On Wednesday, March 27, 2013 at 17:48 EDT, J.V. jvsr...@gmail.com wrote: I have two local branches (tracked to remote) that are in sync (did a git pull on both branches from their corresponding remote). Is this the best way to merge? I would be merging local/branch1 = local/branch2 (test this branch) and then push local/branch2 = origin/branch1 (and would expect no merge conflicts if anyone has not checked in anything. Except for maybe unusual corner cases I don't see how the merge order (branch1 into branch2 or vice versa) makes any difference. If nothing has happened with origin/branch1 since you merged from it to your local branches the push will succeed. If there have been upstream commits you'll have to update your local branch first (which might result in conflicts) before you'll be able to push. Also with two local branches, Is there a way to get a list of files (one line per file) of files that would have merge conflicts that would need to be resolved? You'd have to perform an actual merge for that, perhaps with --no-commit to avoid creating the actual commit object. Inspect the git status output to find the files with conflicts. In a script, use git ls-files -u instead. -- Magnus Bäck ba...@google.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] http-backend: respect GIT_NAMESPACE with dumb clients
On Wed, Mar 27, 2013 at 11:02:28PM -0700, Junio C Hamano wrote:
John Koleszar jkoles...@google.com writes:
Filter the list of refs returned via the dumb HTTP protocol according
to the active namespace, consistent with other clients of the
upload-pack service.
Signed-off-by: John Koleszar jkoles...@google.com
---
Looks sane from a cursory read---thanks.
Josh, any comments?
Looks reasonable; glad to see tests explicitly covering it, as well.
Ideally I'd like to see an additional test against that same namespaced
repository, fetching from a different URL that doesn't set
GIT_NAMESPACE, and verifying that info/refs contains the full set of
namespace-qualified refs from all namespaces. That would make sure that
particular behavior doesn't regress in the future, since one of the use
cases for namespaced repositories involves fetching the whole combined
repo, for purposes such as backups or replication.
http-backend.c | 8 +---
t/lib-httpd/apache.conf | 5 +
t/t5561-http-backend.sh | 4
t/t556x_common | 16
4 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index f50e77f..b9896b0 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -361,17 +361,19 @@ static void run_service(const char **argv)
static int show_text_ref(const char *name, const unsigned char *sha1,
int flag, void *cb_data)
{
+ const char *name_nons = strip_namespace(name);
struct strbuf *buf = cb_data;
struct object *o = parse_object(sha1);
if (!o)
return 0;
- strbuf_addf(buf, %s\t%s\n, sha1_to_hex(sha1), name);
+ strbuf_addf(buf, %s\t%s\n, sha1_to_hex(sha1), name_nons);
if (o-type == OBJ_TAG) {
o = deref_tag(o, name, 0);
if (!o)
return 0;
- strbuf_addf(buf, %s\t%s^{}\n, sha1_to_hex(o-sha1), name);
+ strbuf_addf(buf, %s\t%s^{}\n, sha1_to_hex(o-sha1),
+ name_nons);
}
return 0;
}
@@ -402,7 +404,7 @@ static void get_info_refs(char *arg)
} else {
select_getanyfile();
- for_each_ref(show_text_ref, buf);
+ for_each_namespaced_ref(show_text_ref, buf);
send_strbuf(text/plain, buf);
}
strbuf_release(buf);
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 938b4cf..ad85537 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -61,6 +61,11 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_COMMITTER_NAME Custom User
SetEnv GIT_COMMITTER_EMAIL cus...@example.com
/LocationMatch
+LocationMatch /smart_namespace/
+ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+ SetEnv GIT_HTTP_EXPORT_ALL
+ SetEnv GIT_NAMESPACE ns
+/LocationMatch
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
Directory ${GIT_EXEC_PATH}
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index b5d7fbc..5a19d61 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -134,6 +134,10 @@ POST /smart/repo.git/git-receive-pack HTTP/1.1 200 -
###
GET /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -
POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
+
+### namespace test
+###
+GET /smart_namespace/repo.git/info/refs HTTP/1.1 200
EOF
test_expect_success 'server request log matches test results' '
sed -e
diff --git a/t/t556x_common b/t/t556x_common
index 82926cf..cb9eb9d 100755
--- a/t/t556x_common
+++ b/t/t556x_common
@@ -120,3 +120,19 @@ test_expect_success 'http.receivepack false' '
GET info/refs?service=git-receive-pack 403 Forbidden
POST git-receive-pack 403 Forbidden
'
+test_expect_success 'backend respects namespaces' '
+ log_div namespace test
+ config http.uploadpack true
+ config http.getanyfile true
+
+ GIT_NAMESPACE=ns export GIT_NAMESPACE
+ git push public master:master
+ (cd $HTTPD_DOCUMENT_ROOT_PATH/repo.git
+ git for-each-ref | grep /$GIT_NAMESPACE/ /dev/null
+ )
+
+ git ls-remote public exp
+ curl $HTTPD_URL/smart_namespace/repo.git/info/refs act
+ test_cmp exp act
+ (grep /ns/ exp false || true)
+'
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git subtree oddity
I am starting to regret that I caved in and started carrying a copy of it in contrib/. It probably is a good idea to drop it from my tree and let it mature and eventually flourish outside. that's a shame... it solves a real problem, is simple to use, and really powerfull. but unfortunately, I have sent a patch that solves a serious bug... which had already been reported and patched but had received no answer, and nobody replied to it. Is there anything that can be done to get this rolling, or a way to have the use-case it covers better handle by git-submodule ? currently the problem of a git repo in a git repo is very complicated to deal with in a clean way... Regards Jérémy -- To unsubscribe from this list: send the line unsubscribe 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 in git rev-parse --verify
On Thu, Mar 28, 2013 at 04:52:15PM +0100, Michael Haggerty wrote: On 03/28/2013 04:38 PM, Jeff King wrote: On Thu, Mar 28, 2013 at 04:34:19PM +0100, Michael Haggerty wrote: Is there a simple way to verify an object name more strictly and convert it to an SHA1? I can only think of solutions that require two commands, like git cat-file -e $ARG git rev-parse --verify $ARG Is the rev-parse line doing anything there? If $ARG does not resolve to a sha1, then wouldn't cat-file have failed? It's outputting the SHA1, which cat-file seems incapable of providing in a useful way. Ah, I see; I was looking too much at your example, and not thinking about how you would want to use it in a script. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Thu, Mar 28, 2013 at 09:10:59AM -0700, Junio C Hamano wrote: Josh Triplett j...@joshtriplett.org writes: OK, I take it back. I *can* imagine configurations that this change would break, since it does change intentional and documented behavior, but I don't have any such configuration. The only such configuration I can imagine involves directly counting on the non-rewriting of pushUrl, by using pushInsteadOf to rewrite urls and then sometimes using pushUrl to override that and point back at the un-rewritten URL. And while supported, that does seem *odd*. Objection withdrawn; if nobody can come up with a sensible configuration that relies on the documented behavior, I don't particularly care if it changes. I actually do. Given the popularity of the system, people involved in this thread cannot imagine a case that existing people may get hurt is very different from this is not a regression. After merging this change when people start complaining, you and Rob can hide and ignore them, but we collectively as the Git project have to have a way to help them when it happens. I entirely agree that it represents a regression from documented behavior; I just mean that it no longer matches a specific use case I had in mind with the original change. I agree that we should hesitate to change that documented behavior. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe 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 and GSoC 2013
Christian Couder christian.cou...@gmail.com writes: On Wed, Mar 27, 2013 at 7:52 PM, Jonathan Nieder jrnie...@gmail.com wrote: Jeff King wrote: There was a big thread about a month ago on whether Git should do Google Summer of Code this year[1]. I think we should do it. It looks strange to me to say that students are great and at the same time that we should not do it. Let's give them and us one more chance do to well. This is the only way we can improve. Do you mean we should be doing the same thing over and over again and expecting different results? Einstein may not like it, and I certainly don't. What I gathered from the discussion so far is that everybody agrees that our mentoring has been suboptimal in various ways (not enough encouragement to engage with the community early, working in the cave for too long, biting too much to chew etc.). What makes you think we would do better this year? We have a track record of being not great at mentoring, and we haven't made an effort to improve it. is a perfectly valid and humble reason to excuse ourselves from this year's GSoC. Students are great is immaterial. In fact, if they are great, I think it is better to give them a chance to excel by working with organizations that can mentor them better, instead of squandering their time and GSoC's money for another failure, until _we_ are ready to take great students. It is preferrable if the decision were accompanied with a concrete plan for us to prepare our mentoring capability better (if we want to participate in future GSoC, that is), but I think it is a separate issue, and I suspect that it is too late for this year's. -- To unsubscribe from this list: send the line unsubscribe 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] http-backend: respect GIT_NAMESPACE with dumb clients
John Koleszar jkoles...@google.com writes: Facepalm. The intent here is to invert the grep, to make sure that the /ns/ does not appear in the output. No idea why I wrote it that way. Will fix. OK, ! grep /ns/ exp would do. 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
[PATCH v10 0/5] git log -L
From: Thomas Rast tr...@inf.ethz.ch This adds a bunch of fixes and failing tests for invalid -L arguments; as Antoine discovered, some variations would segfault v9. I also changed the beginning of parse_range_funcname (in patch 4/5), which now also lets you backslash-escape a : in a funcname regex. The old version was based on the assumption that there could only be a ':' in the string if we were coming from scan_range_arg, which made it a bit hard to read. Bo Yang (2): Refactor parse_loc Export rewrite_parents() for 'log -L' Thomas Rast (3): Implement line-history search (git log -L) log -L: :pattern:file syntax to find by funcname Speed up log -L... -M Documentation/blame-options.txt | 21 +- Documentation/git-blame.txt |6 +- Documentation/git-log.txt | 23 + Documentation/line-range-format.txt | 25 + Makefile|4 + builtin/blame.c | 99 +-- builtin/log.c | 31 + line-log.c | 1228 +++ line-log.h | 49 ++ line-range.c| 243 +++ line-range.h| 36 + log-tree.c |4 + revision.c | 22 +- revision.h | 16 +- t/perf/p4211-line-log.sh| 34 + t/t4211-line-log.sh | 53 ++ t/t4211/expect.beginning-of-file| 43 ++ t/t4211/expect.end-of-file | 62 ++ t/t4211/expect.move-support-f | 40 ++ t/t4211/expect.simple-f | 59 ++ t/t4211/expect.simple-f-to-main | 100 +++ t/t4211/expect.simple-main | 68 ++ t/t4211/expect.simple-main-to-end | 70 ++ t/t4211/expect.two-ranges | 102 +++ t/t4211/expect.vanishes-early | 39 ++ t/t4211/history.export | 330 ++ t/t8003-blame-corner-cases.sh |6 + 27 files changed, 2690 insertions(+), 123 deletions(-) create mode 100644 Documentation/line-range-format.txt create mode 100644 line-log.c create mode 100644 line-log.h create mode 100644 line-range.c create mode 100644 line-range.h create mode 100755 t/perf/p4211-line-log.sh create mode 100755 t/t4211-line-log.sh create mode 100644 t/t4211/expect.beginning-of-file create mode 100644 t/t4211/expect.end-of-file create mode 100644 t/t4211/expect.move-support-f create mode 100644 t/t4211/expect.simple-f create mode 100644 t/t4211/expect.simple-f-to-main create mode 100644 t/t4211/expect.simple-main create mode 100644 t/t4211/expect.simple-main-to-end create mode 100644 t/t4211/expect.two-ranges create mode 100644 t/t4211/expect.vanishes-early create mode 100644 t/t4211/history.export -- 1.8.2.446.g2b4de83 -- To unsubscribe from this list: send the line 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 v10 1/5] Refactor parse_loc
From: Bo Yang struggleyb@gmail.com
We want to use the same style of -L n,m argument for 'git log -L' as
for git-blame. Refactor the argument parsing of the range arguments
from builtin/blame.c to the (new) file that will hold the 'git log -L'
logic.
To accommodate different data structures in blame and log -L, the file
contents are abstracted away; parse_range_arg takes a callback that it
uses to get the contents of a line of the (notional) file.
The new test is for a case that made me pause during debugging: the
'blame -L with invalid end' test was the only one that noticed an
outright failure to parse the end *at all*. So make a more explicit
test for that.
Signed-off-by: Bo Yang struggleyb@gmail.com
Signed-off-by: Thomas Rast tr...@student.ethz.ch
---
Documentation/blame-options.txt | 19 +--
Documentation/line-range-format.txt | 18 +++
Makefile| 2 +
builtin/blame.c | 99 +++--
line-range.c| 92 ++
line-range.h| 24 +
t/t8003-blame-corner-cases.sh | 6 +++
7 files changed, 151 insertions(+), 109 deletions(-)
create mode 100644 Documentation/line-range-format.txt
create mode 100644 line-range.c
create mode 100644 line-range.h
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index b0d31df..6998d9f 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -13,24 +13,7 @@
Annotate only the given line range. start and end can take
one of these forms:
- - number
-+
-If start or end is a number, it specifies an
-absolute line number (lines count from 1).
-+
-
-- /regex/
-+
-This form will use the first line matching the given
-POSIX regex. If end is a regex, it will search
-starting at the line given by start.
-+
-
-- +offset or -offset
-+
-This is only valid for end and will specify a number
-of lines before or after the line given by start.
-+
+include::line-range-format.txt[]
-l::
Show long rev (Default: off).
diff --git a/Documentation/line-range-format.txt
b/Documentation/line-range-format.txt
new file mode 100644
index 000..265bc23
--- /dev/null
+++ b/Documentation/line-range-format.txt
@@ -0,0 +1,18 @@
+- number
++
+If start or end is a number, it specifies an
+absolute line number (lines count from 1).
++
+
+- /regex/
++
+This form will use the first line matching the given
+POSIX regex. If end is a regex, it will search
+starting at the line given by start.
++
+
+- +offset or -offset
++
+This is only valid for end and will specify a number
+of lines before or after the line given by start.
++
diff --git a/Makefile b/Makefile
index 598d631..e83f64b 100644
--- a/Makefile
+++ b/Makefile
@@ -667,6 +667,7 @@ LIB_H += help.h
LIB_H += http.h
LIB_H += kwset.h
LIB_H += levenshtein.h
+LIB_H += line-range.h
LIB_H += list-objects.h
LIB_H += ll-merge.h
LIB_H += log-tree.h
@@ -795,6 +796,7 @@ LIB_OBJS += hex.o
LIB_OBJS += ident.o
LIB_OBJS += kwset.o
LIB_OBJS += levenshtein.o
+LIB_OBJS += line-range.o
LIB_OBJS += list-objects.o
LIB_OBJS += ll-merge.o
LIB_OBJS += lockfile.o
diff --git a/builtin/blame.c b/builtin/blame.c
index 86100e9..20eb439 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -21,6 +21,7 @@
#include parse-options.h
#include utf8.h
#include userdiff.h
+#include line-range.h
static char blame_usage[] = N_(git blame [options] [rev-opts] [rev] [--]
file);
@@ -566,11 +567,16 @@ static void dup_entry(struct blame_entry *dst, struct
blame_entry *src)
dst-score = 0;
}
-static const char *nth_line(struct scoreboard *sb, int lno)
+static const char *nth_line(struct scoreboard *sb, long lno)
{
return sb-final_buf + sb-lineno[lno];
}
+static const char *nth_line_cb(void *data, long lno)
+{
+ return nth_line((struct scoreboard *)data, lno);
+}
+
/*
* It is known that lines between tlno to same came from parent, and e
* has an overlap with that range. it also is known that parent's
@@ -1927,83 +1933,6 @@ static const char *add_prefix(const char *prefix, const
char *path)
}
/*
- * Parsing of (comma separated) one item in the -L option
- */
-static const char *parse_loc(const char *spec,
-struct scoreboard *sb, long lno,
-long begin, long *ret)
-{
- char *term;
- const char *line;
- long num;
- int reg_error;
- regex_t regexp;
- regmatch_t match[1];
-
- /* Allow -L something,+20 to mean starting at something
-* for 20 lines, or -L something,-5 for 5 lines ending at
-* something.
-*/
- if (1 begin (spec[0] == '+' || spec[0] == '-')) {
- num = strtol(spec + 1, term, 10);
- if (term != spec + 1) {
- if (spec[0] == '-')
- num = 0 - num;
-
[PATCH v10 2/5] Export rewrite_parents() for 'log -L'
From: Bo Yang struggleyb@gmail.com
The function rewrite_one is used to rewrite a single
parent of the current commit, and is used by rewrite_parents
to rewrite all the parents.
Decouple the dependence between them by making rewrite_one
a callback function that is passed to rewrite_parents. Then
export rewrite_parents for reuse by the line history browser.
We will use this function in line-log.c.
Signed-off-by: Bo Yang struggleyb@gmail.com
Signed-off-by: Thomas Rast tr...@student.ethz.ch
---
revision.c | 13 -
revision.h | 10 ++
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/revision.c b/revision.c
index ef60205..46319d5 100644
--- a/revision.c
+++ b/revision.c
@@ -2173,12 +2173,6 @@ int prepare_revision_walk(struct rev_info *revs)
return 0;
}
-enum rewrite_result {
- rewrite_one_ok,
- rewrite_one_noparents,
- rewrite_one_error
-};
-
static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit
**pp)
{
struct commit_list *cache = NULL;
@@ -2200,12 +2194,13 @@ static enum rewrite_result rewrite_one(struct rev_info
*revs, struct commit **pp
}
}
-static int rewrite_parents(struct rev_info *revs, struct commit *commit)
+int rewrite_parents(struct rev_info *revs, struct commit *commit,
+ rewrite_parent_fn_t rewrite_parent)
{
struct commit_list **pp = commit-parents;
while (*pp) {
struct commit_list *parent = *pp;
- switch (rewrite_one(revs, parent-item)) {
+ switch (rewrite_parent(revs, parent-item)) {
case rewrite_one_ok:
break;
case rewrite_one_noparents:
@@ -2371,7 +2366,7 @@ enum commit_action simplify_commit(struct rev_info *revs,
struct commit *commit)
if (action == commit_show
!revs-show_all
revs-prune revs-dense want_ancestry(revs)) {
- if (rewrite_parents(revs, commit) 0)
+ if (rewrite_parents(revs, commit, rewrite_one) 0)
return commit_error;
}
return action;
diff --git a/revision.h b/revision.h
index 5da09ee..640110d 100644
--- a/revision.h
+++ b/revision.h
@@ -241,4 +241,14 @@ enum commit_action {
extern enum commit_action get_commit_action(struct rev_info *revs, struct
commit *commit);
extern enum commit_action simplify_commit(struct rev_info *revs, struct commit
*commit);
+enum rewrite_result {
+ rewrite_one_ok,
+ rewrite_one_noparents,
+ rewrite_one_error
+};
+
+typedef enum rewrite_result (*rewrite_parent_fn_t)(struct rev_info *revs,
struct commit **pp);
+
+extern int rewrite_parents(struct rev_info *revs, struct commit *commit,
+ rewrite_parent_fn_t rewrite_parent);
#endif
--
1.8.2.446.g2b4de83
--
To unsubscribe from this list: send the line 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 v10 4/5] log -L: :pattern:file syntax to find by funcname
This new syntax finds a funcname matching /pattern/, and then takes from there
up to (but not including) the next funcname. So you can say
git log -L:main:main.c
and it will dig up the main() function and show its line-log, provided
there are no other funcnames matching 'main'.
Signed-off-by: Thomas Rast tr...@student.ethz.ch
---
Documentation/blame-options.txt | 2 +-
Documentation/git-blame.txt | 6 +-
Documentation/git-log.txt | 11 +--
Documentation/line-range-format.txt | 7 ++
builtin/blame.c | 2 +-
line-log.c | 5 +-
line-range.c| 136 +++-
line-range.h| 3 +-
t/t4211-line-log.sh | 4 ++
t/t4211/expect.simple-f-to-main | 100 ++
t/t4211/expect.simple-main-to-end | 70 +++
11 files changed, 332 insertions(+), 14 deletions(-)
create mode 100644 t/t4211/expect.simple-f-to-main
create mode 100644 t/t4211/expect.simple-main-to-end
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 6998d9f..e9f984b 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -9,7 +9,7 @@
--show-stats::
Include additional statistics at the end of blame output.
--L start,end::
+-L start,end, -L :regex::
Annotate only the given line range. start and end can take
one of these forms:
diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index 9a05c2b..6cea7f1 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -8,9 +8,9 @@ git-blame - Show what revision and author last modified each
line of a file
SYNOPSIS
[verse]
-'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w]
[--incremental] [-L n,m]
- [-S revs-file] [-M] [-C] [-C] [-C] [--since=date] [--abbrev=n]
- [rev | --contents file | --reverse rev] [--] file
+'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w]
[--incremental]
+ [-L n,m | -L :fn] [-S revs-file] [-M] [-C] [-C] [-C]
[--since=date]
+ [--abbrev=n] [rev | --contents file | --reverse rev] [--]
file
DESCRIPTION
---
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 8727c60..4850226 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -69,12 +69,13 @@ produced by --stat etc.
Note that only message is considered, if also a diff is shown
its size is not included.
--L start,end:file::
+-L start,end:file, -L :regex:file::
+
Trace the evolution of the line range given by start,end
- within the file. You may not give any pathspec limiters.
- This is currently limited to a walk starting from a single
- revision, i.e., you may only give zero or one positive
- revision arguments.
+ (or the funcname regex regex) within the file. You may
+ not give any pathspec limiters. This is currently limited to
+ a walk starting from a single revision, i.e., you may only
+ give zero or one positive revision arguments.
start and end can take one of these forms:
diff --git a/Documentation/line-range-format.txt
b/Documentation/line-range-format.txt
index 265bc23..3e7ce72 100644
--- a/Documentation/line-range-format.txt
+++ b/Documentation/line-range-format.txt
@@ -16,3 +16,10 @@ starting at the line given by start.
This is only valid for end and will specify a number
of lines before or after the line given by start.
+
+
+- :regex
++
+If the option's argument is of the form :regex, it denotes the range
+from the first funcname line that matches regex, up to the next
+funcname line.
++
diff --git a/builtin/blame.c b/builtin/blame.c
index 20eb439..1c09d55 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1940,7 +1940,7 @@ static void prepare_blame_range(struct scoreboard *sb,
long lno,
long *bottom, long *top)
{
- if (parse_range_arg(bottomtop, nth_line_cb, sb, lno, bottom, top))
+ if (parse_range_arg(bottomtop, nth_line_cb, sb, lno, bottom, top,
sb-path))
usage(blame_usage);
}
diff --git a/line-log.c b/line-log.c
index 6244231..68972e2 100644
--- a/line-log.c
+++ b/line-log.c
@@ -12,6 +12,7 @@
#include strbuf.h
#include log-tree.h
#include graph.h
+#include userdiff.h
#include line-log.h
static void range_set_grow(struct range_set *rs, size_t extra)
@@ -438,7 +439,6 @@ static void range_set_map_across_diff(struct range_set *out,
*touched_out = touched;
}
-
static struct commit *check_single_commit(struct rev_info *revs)
{
struct object *commit = NULL;
@@ -559,7 +559,8 @@ static const char *nth_line(void *data, long line)
cb_data.line_ends = ends;
if (parse_range_arg(range_part, nth_line,
[PATCH v10 5/5] Speed up log -L... -M
So far log -L only used the implicit diff filtering by pathspec. If
the user specifies -M, we cannot do that, and so we simply handed the
whole diff queue (which is approximately 'git show --raw') to
diffcore_std().
Unfortunately this is very slow. We can optimize a lot if we throw
out files that we know cannot possibly be interesting, in the same
spirit that the pathspec filtering reduces the number of files.
However, in this case, we have to be more careful. Because we want to
look out for renames, we need to keep all filepairs where something
was deleted.
This is a bit hacky and should really be replaced by equivalent
support in --follow, and just using that. However, in the meantime it
speeds up 'log -M -L' by an order of magnitude.
Signed-off-by: Thomas Rast tr...@student.ethz.ch
---
line-log.c | 56
1 file changed, 52 insertions(+), 4 deletions(-)
diff --git a/line-log.c b/line-log.c
index 68972e2..30edef4 100644
--- a/line-log.c
+++ b/line-log.c
@@ -750,7 +750,50 @@ static void move_diff_queue(struct diff_queue_struct *dst,
DIFF_QUEUE_CLEAR(src);
}
-static void queue_diffs(struct diff_options *opt,
+static void filter_diffs_for_paths(struct line_log_data *range, int
keep_deletions)
+{
+ int i;
+ struct diff_queue_struct outq;
+ DIFF_QUEUE_CLEAR(outq);
+
+ for (i = 0; i diff_queued_diff.nr; i++) {
+ struct diff_filepair *p = diff_queued_diff.queue[i];
+ struct line_log_data *rg = NULL;
+
+ if (!DIFF_FILE_VALID(p-two)) {
+ if (keep_deletions)
+ diff_q(outq, p);
+ else
+ diff_free_filepair(p);
+ continue;
+ }
+ for (rg = range; rg; rg = rg-next) {
+ if (!strcmp(rg-spec-path, p-two-path))
+ break;
+ }
+ if (rg)
+ diff_q(outq, p);
+ else
+ diff_free_filepair(p);
+ }
+ free(diff_queued_diff.queue);
+ diff_queued_diff = outq;
+}
+
+static inline int diff_might_be_rename(void)
+{
+ int i;
+ for (i = 0; i diff_queued_diff.nr; i++)
+ if (!DIFF_FILE_VALID(diff_queued_diff.queue[i]-one)) {
+ /* fprintf(stderr, diff_might_be_rename found creation
of: %s\n, */
+ /* diff_queued_diff.queue[i]-two-path); */
+ return 1;
+ }
+ return 0;
+}
+
+static void queue_diffs(struct line_log_data *range,
+ struct diff_options *opt,
struct diff_queue_struct *queue,
struct commit *commit, struct commit *parent)
{
@@ -766,7 +809,12 @@ static void queue_diffs(struct diff_options *opt,
DIFF_QUEUE_CLEAR(diff_queued_diff);
diff_tree(desc1, desc2, , opt);
- diffcore_std(opt);
+ if (opt-detect_rename) {
+ filter_diffs_for_paths(range, 1);
+ if (diff_might_be_rename())
+ diffcore_std(opt);
+ filter_diffs_for_paths(range, 0);
+ }
move_diff_queue(queue, diff_queued_diff);
if (tree1)
@@ -1050,7 +1098,7 @@ static int process_ranges_ordinary_commit(struct rev_info
*rev, struct commit *c
if (commit-parents)
parent = commit-parents-item;
- queue_diffs(rev-diffopt, queue, commit, parent);
+ queue_diffs(range, rev-diffopt, queue, commit, parent);
changed = process_all_files(parent_range, rev, queue, range);
if (parent)
add_line_range(rev, parent, parent_range);
@@ -1075,7 +1123,7 @@ static int process_ranges_merge_commit(struct rev_info
*rev, struct commit *comm
for (i = 0; i nparents; i++) {
parents[i] = p-item;
p = p-next;
- queue_diffs(rev-diffopt, diffqueues[i], commit, parents[i]);
+ queue_diffs(range, rev-diffopt, diffqueues[i], commit,
parents[i]);
}
for (i = 0; i nparents; i++) {
--
1.8.2.446.g2b4de83
--
To unsubscribe from this list: send the line unsubscribe 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 in git rev-parse --verify
Jeff King p...@peff.net writes:
On Thu, Mar 28, 2013 at 04:34:19PM +0100, Michael Haggerty wrote:
Is there a simple way to verify an object name more strictly and convert
it to an SHA1? I can only think of solutions that require two commands,
like
git cat-file -e $ARG git rev-parse --verify $ARG
Is the rev-parse line doing anything there? If $ARG does not resolve to
a sha1, then wouldn't cat-file have failed?
-Peff
You could force rev-parse to resolve the input to an existing
object, with something like this:
git rev-parse --verify $ARG^{}
It will unwrap a tag, so the output may end up pointing at a object
that is different from $ARG in such a case.
But what is the purpose of turning a random string to a random
40-hex in the first place?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] Support triangular workflows
Jeff King p...@peff.net writes: Sometimes it's annoyingly verbose to break down a compound function. But I think in this case, you can make your tests more robust by just checking the affirmative that the ref is still where we expect it to be, like: check_push_result up_repo $the_first_commit heads/master Sorry if that was a bit long-winded. I think that practically speaking, it is not a likely source of problems in this case. But it's an anti-pattern in our tests that I think is worth mentioning. Thanks. That is one of the reasons why we do not want to see too many custom test helper functions. -- To unsubscribe from this list: send the line unsubscribe 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 and GSoC 2013
On Thu, Mar 28, 2013 at 09:45:02AM -0700, Junio C Hamano wrote: It is preferrable if the decision were accompanied with a concrete plan for us to prepare our mentoring capability better (if we want to participate in future GSoC, that is), but I think it is a separate issue, and I suspect that it is too late for this year's. I agree with this. I do think we should participate again, and I think we should try to address the issues I brought up (and you mentioned in your email) about project size and community involvement. My email was meant to be a maybe it's not too late if people really want to work on the project ideas page. But I haven't seen anything happening there, and we really are running right up to the deadline[1]. I think at this point it makes sense to wait a year and try to approach it sooner next year. -Peff [1] I know my email didn't give much time for action; the deadline snuck up on me, too. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Composing git repositories
Ramkumar Ramachandra wrote: Do you realize how difficult this is to implement? We'll need to patch all the git commands to essentially do what we'd get for free if the submodule were a tree object instead of a commit object (although I'm not saying that's the Right thing to do). What are you talking about? Yes, of course I realize that recursing over subprojects that are managed as separate git repositories requires writing new code. That's why people have started to write such code. They seem to think it's worth it. Meanwhile others with different designs in mind have written other tools. Use cases even overlap a little, so they can compete. That is exactly as it should be. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: More detailed error message for 403 forbidden.
On Wed, Mar 27, 2013 at 12:29:57PM +0900, Yi, EungJun wrote: Currently, if user tried to access a git repository via HTTP and it fails because the user's permission is not enough to access the repository, git client tells that http request failed and the error was 403 forbidden. The situations in which you'll get a 403 depend on how the server is configured. For instance, on github.com, if you successfully authenticate but are not authorized to access a repository, you get a 404 (we do this to avoid leaking information about which private repositories exist). But we do provide a 403 if you try to access the repository with a non-smart-http client. So the 403 forbidden there is not about your account, but about the method; if git is going to give a more verbose message, it needs to be careful not to mislead the user. It would be much better if git client shows response body which might include an explanation of the failure. For example, [...] $ git clone http://localhost/foo/bar error: The requested URL returned error: 403 while accessing http://localhost/foo/bar remote: User 'me' does not have enough permission to access the repository. fatal: HTTP request failed I agree that is the best way forward, as that means the server is telling us what is going on, and we are not guessing about the meaning of the 403. One problem is that the content body sent along with the error is not necessarily appropriate for showing to the user (e.g., if it is HTML, it is probably not a good idea to show it on the terminal). So I think we would want to only show it when the server has indicated via the content-type that the message is meant to be shown to the user. I'm thinking the server would generate something like: HTTP/1.1 403 Forbidden Content-type: application/x-git-error-message User 'me' does not have enough permission to access the repository. which would produce the example you showed above. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: More detailed error message for 403 forbidden.
Jeff King wrote: One problem is that the content body sent along with the error is not necessarily appropriate for showing to the user (e.g., if it is HTML, it is probably not a good idea to show it on the terminal). So I think we would want to only show it when the server has indicated via the content-type that the message is meant to be shown to the user. I'm thinking the server would generate something like: HTTP/1.1 403 Forbidden Content-type: application/x-git-error-message User 'me' does not have enough permission to access the repository. which would produce the example you showed above. Would it make sense to use text/plain this way? 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: More detailed error message for 403 forbidden.
On Thu, Mar 28, 2013 at 11:41:20AM -0700, Jonathan Nieder wrote: Jeff King wrote: One problem is that the content body sent along with the error is not necessarily appropriate for showing to the user (e.g., if it is HTML, it is probably not a good idea to show it on the terminal). So I think we would want to only show it when the server has indicated via the content-type that the message is meant to be shown to the user. I'm thinking the server would generate something like: HTTP/1.1 403 Forbidden Content-type: application/x-git-error-message User 'me' does not have enough permission to access the repository. which would produce the example you showed above. Would it make sense to use text/plain this way? Maybe. But I would worry somewhat about sites which provide a useless and verbose text/plain message. Ideally an x-git-error-message would be no more than few lines, suitable for the error message of a terminal program. I would not want a site-branded Your page cannot be found. Here's a complete navigation bar page to be spewed to the terminal. Those tend to be text/html, though, so we may be safe. It's just that we're gambling on what random servers do, and if we show useless spew even some of the time, that would be a regression. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Josh Triplett j...@joshtriplett.org writes: (on url.$base.pushInsteadOf) If a remote has an explicit pushurl, git will ignore this setting for that remote. That really meant what I just said above: git will prefer an explicit pushurl over the pushInsteadOf rewrite of url. Very correct. It says nothing about applying pushInsteadOf to rewrite pushurl. Incorrect, I think. If you have pushURL, pushInsteadOf is *ignored*. Of course, if you have both URL and pushURL, the ignored pushInsteadOf will not apply to _anything_. It will not apply to URL, and it will certainly not apply to pushURL. You are correct to point out that with the test we would want to make sure that for a remote with pushURL and URL, a push goes - to pushURL; - not to URL; - not to insteadOf(URL); - not to pushInsteadOf(URL); - not to insteadOf(pushURL); and - not to pushInsteadOf(pushURL). I do not think it is worth checking all of them, but I agree we should make sure it does not go to pushInsteadOf(URL) which you originally meant to check, and we should also make sure it does not go to pushInsteadOf(pushURL). test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty -TRASH=$(pwd)/ -git config url.trash2/.pushInsteadOf trash/ +git config url.trash2/.pushInsteadOf testrepo/ Adding git config url.trash3/.pusnInsteadOf trash/wrong here should be sufficient for that, no? If we mistakenly used URL (i.e. trash/wrong) the push would fail. If we mistakenly used pushInsteadOf(URL), that is rewritten to trash3/ and again the push would fail. pushInsteadOf(pushURL) would go to trash2/ and that would also fail. We aren't checking insteadOf(URL) and insteadOf(pushURL) but everything else is checked, I think, so we can do without replacing anything. We can just extend it, no? git config remote.r.url trash/wrong -git config remote.r.pushurl $TRASH/testrepo +git config remote.r.pushurl testrepo/ git push r refs/heads/master:refs/remotes/origin/master ( cd testrepo ...the test you describe should appear in *addition* to this test, not replacing it, because as described above this test does test one critical bit of behavior, namely prefering pushurl over the pushInsteadOf rewrite of url. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe 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] fast-import: Fix an gcc -Wuninitialized warning
Jeff King wrote:
On Tue, Mar 26, 2013 at 07:09:44PM +, Ramsay Jones wrote:
Commit cbfd5e1c (drop some obsolete x = x compiler warning hacks,
21-03-2013) removed a gcc hack that suppressed an might be used
uninitialized warning issued by older versions of gcc.
However, commit 3aa99df8 ('fast-import: clarify inline logic in
file_change_m', 21-03-2013) addresses an (almost) identical issue
(with very similar code), but includes additional code in it's
resolution. The solution used by this commit, unlike that used by
commit cbfd5e1c, also suppresses the -Wuninitialized warning on
older versions of gcc.
In order to suppress the warning (against the 'oe' symbol) in the
note_change_n() function, we adopt the same solution used by commit
3aa99df8.
Yeah, they are essentially the same piece of code, so I don't mind this
change. It is odd to me that gcc gets it right in one case but not the
other, but I think we are deep into the vagaries of the compiler's code
flow analysis here, and we cannot make too many assumptions.
Were you actually triggering this warning, and if so, on what version of
gcc?
yes, with:
gcc v3.4.4 (cygwin)
gcc v4.1.2 (Linux)
msvc v15.00.30729.01 (VC/C++ 2008 express edition)
no, with:
gcc v4.4.0 (msysgit)
clang 3.2 (Linux)
tcc v0.9.26 (Linux)
[lcc can't compile git; I forget why exactly.]
Or did the asymmetry just offend your sensibilities?
My sensibilities were, indeed, very offended! ;-)
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line unsubscribe 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] cat-file: Fix an gcc -Wuninitialized warning
Jeff King wrote: On Tue, Mar 26, 2013 at 07:20:11PM +, Ramsay Jones wrote: After commit cbfd5e1c (drop some obsolete x = x compiler warning hacks, 21-03-2013) removed a gcc specific hack, older versions of gcc now issue an 'contents' might be used uninitialized warning. In order to suppress the warning, we simply initialize the variable to NULL in it's declaration. I'm OK with this, if it's the direction we want to go. But I thought the discussion kind of ended as we do not care about these warnings on ancient versions of gcc; those people should use -Wno-error=uninitialized. Hmm, I don't recall any agreement or conclusions being reached. I guess I missed that! What version of gcc are you using? If it is the most recent thing reasonably available on msysgit, then I am more sympathetic. But if it's just an antique version of gcc, I am less so. (see previous email for compiler versions). I suppose it depends on what you consider antique. [I recently downloaded the first C compiler from github. Yes, that is an antique compiler! ;-)] I would call some of the compilers I use a bit mature. :-P Hmm, so are you saying that this patch is not acceptable because I used a compiler that is no longer supported? ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe 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] fast-import: Fix an gcc -Wuninitialized warning
On Thu, Mar 28, 2013 at 06:20:29PM +, Ramsay Jones wrote:
Jeff King wrote:
On Tue, Mar 26, 2013 at 07:09:44PM +, Ramsay Jones wrote:
Commit cbfd5e1c (drop some obsolete x = x compiler warning hacks,
21-03-2013) removed a gcc hack that suppressed an might be used
uninitialized warning issued by older versions of gcc.
However, commit 3aa99df8 ('fast-import: clarify inline logic in
file_change_m', 21-03-2013) addresses an (almost) identical issue
(with very similar code), but includes additional code in it's
resolution. The solution used by this commit, unlike that used by
commit cbfd5e1c, also suppresses the -Wuninitialized warning on
older versions of gcc.
In order to suppress the warning (against the 'oe' symbol) in the
note_change_n() function, we adopt the same solution used by commit
3aa99df8.
Yeah, they are essentially the same piece of code, so I don't mind this
change. It is odd to me that gcc gets it right in one case but not the
other, but I think we are deep into the vagaries of the compiler's code
flow analysis here, and we cannot make too many assumptions.
Were you actually triggering this warning, and if so, on what version of
gcc?
yes, with:
gcc v3.4.4 (cygwin)
gcc v4.1.2 (Linux)
msvc v15.00.30729.01 (VC/C++ 2008 express edition)
no, with:
gcc v4.4.0 (msysgit)
clang 3.2 (Linux)
tcc v0.9.26 (Linux)
[lcc can't compile git; I forget why exactly.]
Thanks. I do not mind this fix, as it matches the similar code, and it
is fairly straightforward. But I am also happy to let people running gcc
v3.4.4 and other ancient compilers deal with not turning on -Werror. ;)
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-send-email.perl: implement suggestions made by perlcritic
Ramkumar Ramachandra wrote: Junio: In future, please tell me explicitly that you're expecting a re-roll with an updated commit message. It wasn't obvious to me at all. When there are questions in response to a patch, there are two possibilities: * temporary brainfart --- sorry for the bother * the clarity of the patch or commit message has room for improvement This wasn't a case of the former, so a seasoned contributor could safely assume that it was definitely a case of the latter. The explanation in Linux kernel's Documentation/SubmittingPatches item 10 (Don't get discouraged. Re-submit) has some fitting advice. 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] cat-file: Fix an gcc -Wuninitialized warning
On Thu, Mar 28, 2013 at 06:48:43PM +, Ramsay Jones wrote: I'm OK with this, if it's the direction we want to go. But I thought the discussion kind of ended as we do not care about these warnings on ancient versions of gcc; those people should use -Wno-error=uninitialized. Hmm, I don't recall any agreement or conclusions being reached. I guess I missed that! I think Jonathan said that and nobody disagreed, and I took it as a conclusion. Hmm, so are you saying that this patch is not acceptable because I used a compiler that is no longer supported? No, I just think we should come to a decision on how unreadable to make the code in order to suppress incorrect warnings on old compilers. I can see the point in either of the following arguments: 1. These compilers are old, and we do not need to cater to them in the code because people can just _not_ set -Werror=uninitialized (or its equivalent). It is still worth catering to bugs in modern compilers that most devs use, because being able to set -Werror is helpful. 2. The code is not made significantly less readable, especially if you put in a comment, so why not help these compilers. When we can make the code more readable _and_ help the compiler, I think it is a no-brainer. I am on the fence otherwise and don't care that much. I just think we should apply the rule consistently. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Thu, Mar 28, 2013 at 11:50:08AM -0700, Junio C Hamano wrote: Josh Triplett j...@joshtriplett.org writes: (on url.$base.pushInsteadOf) If a remote has an explicit pushurl, git will ignore this setting for that remote. That really meant what I just said above: git will prefer an explicit pushurl over the pushInsteadOf rewrite of url. Very correct. It says nothing about applying pushInsteadOf to rewrite pushurl. Incorrect, I think. If you have pushURL, pushInsteadOf is *ignored*. Of course, if you have both URL and pushURL, the ignored pushInsteadOf will not apply to _anything_. It will not apply to URL, and it will certainly not apply to pushURL. Debatable whether the documentation sentence above really says that; it certainly doesn't say it very clearly if so. But that does match the actual behavior, making the proposed change a regression from the actual behavior, whether the documentation clearly guarantees that behavior or not. You are correct to point out that with the test we would want to make sure that for a remote with pushURL and URL, a push goes - to pushURL; - not to URL; - not to insteadOf(URL); - not to pushInsteadOf(URL); - not to insteadOf(pushURL); and - not to pushInsteadOf(pushURL). I do not think it is worth checking all of them, but I agree we should make sure it does not go to pushInsteadOf(URL) which you originally meant to check, and we should also make sure it does not go to pushInsteadOf(pushURL). Agreed. Related to this, as a path forward, I do think it makes sense to have a setting usable as an insteadOf that only applies to pushurl, even though pushInsteadOf won't end up serving that purpose. That way, pushInsteadOf covers the map read-only repo url to pushable repo url case, and insteadOfPushOnly covers the construct a magic prefix that maps to different urls when used in url or pushurl case. test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty - TRASH=$(pwd)/ - git config url.trash2/.pushInsteadOf trash/ + git config url.trash2/.pushInsteadOf testrepo/ Adding git config url.trash3/.pusnInsteadOf trash/wrong here should be sufficient for that, no? If we mistakenly used URL (i.e. trash/wrong) the push would fail. If we mistakenly used pushInsteadOf(URL), that is rewritten to trash3/ and again the push would fail. pushInsteadOf(pushURL) would go to trash2/ and that would also fail. We aren't checking insteadOf(URL) and insteadOf(pushURL) but everything else is checked, I think, so we can do without replacing anything. We can just extend it, no? That sounds sensible. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: More detailed error message for 403 forbidden.
Jeff King p...@peff.net writes: One problem is that the content body sent along with the error is not necessarily appropriate for showing to the user (e.g., if it is HTML, it is probably not a good idea to show it on the terminal). So I think we would want to only show it when the server has indicated via the content-type that the message is meant to be shown to the user. I'm thinking the server would generate something like: HTTP/1.1 403 Forbidden Content-type: application/x-git-error-message User 'me' does not have enough permission to access the repository. which would produce the example you showed above. Actually, isn't the human-readable part of the server response meant for this kind of thing? I.e. HTTP/1.1 403 User 'me' not allowed to accept the 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] push: Alias pushurl from push rewrites
Josh Triplett wrote: Related to this, as a path forward, I do think it makes sense to have a setting usable as an insteadOf that only applies to pushurl, even though pushInsteadOf won't end up serving that purpose. That way, pushInsteadOf covers the map read-only repo url to pushable repo url case, and insteadOfPushOnly covers the construct a magic prefix that maps to different urls when used in url or pushurl case. I hope not. That would be making configuration even more complicated. I hope that we can fix the documentation, tests, and change description in the commit message enough to make Rob's patch a no-brainer. If that's not possible, I think the current state is livable, just confusing. I was happy to see Rob's patch because it brings git's behavior closer to following the principle of least surprise. I am not actually that excited by the use case, except the avoiding surprise part of it. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git subtree oddity
I agree that subtree solves some specific use cases I would like to support. In particular, I was hoping to use the subtree command in lieu of using the subtree merge strategy to manage and overlay changes to upstream projects, as well as other local components. At any rate, it looks like the problem I'm having is not entirely related to the subtree command, but happens when I checkout a remote into a branch ( which subtree is presumably doing in the background). It's the same setup as before. Here is the sequence of commands I'm running. git init git remote add upstream git://gnuradio.org/gnuradio fetch upstream git checkout -b upstream_tracking upstream/master Now, at this point, I expect the upstream branch to contain the contents of the gnuradio project. I also expect that my local mater branch has only the contents of my local sources, and NOT the contents of the gnuradio. However, if I 'git checkout master', I see the contents of the gnuradio project. Why, when I checkout a branch tracking upstream/master, do the changes also appear on my master branch, and not just in the remote tracking branch? As a reference, this is close to what I'm trying to accomplish. His screenshot titled 'Directory Listing in Master' shows what I expect. http://typecastexception.com/post/2013/03/16/Managing-Nested-Libraries-Using-the-GIT-Subtree-Merge-Workflow.aspx Thanks -Tom Taranowski On Thu, Mar 28, 2013 at 9:34 AM, Jeremy Rosen jeremy.ro...@openwide.fr wrote: I am starting to regret that I caved in and started carrying a copy of it in contrib/. It probably is a good idea to drop it from my tree and let it mature and eventually flourish outside. that's a shame... it solves a real problem, is simple to use, and really powerfull. but unfortunately, I have sent a patch that solves a serious bug... which had already been reported and patched but had received no answer, and nobody replied to it. Is there anything that can be done to get this rolling, or a way to have the use-case it covers better handle by git-submodule ? currently the problem of a git repo in a git repo is very complicated to deal with in a clean way... Regards Jérémy -- To unsubscribe from this list: send the line unsubscribe 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] cat-file: Fix an gcc -Wuninitialized warning
Jeff King wrote: When we can make the code more readable _and_ help the compiler, I think it is a no-brainer. Yep. :) -- To unsubscribe from this list: send the line unsubscribe 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 subtree oddity
Oh, this is odd. I can get the behavior I want by adding the '-f' flag to the remote add. So: git remote add -f upstream git://gnuradio.org/gnuradio According to the remote add help, the -f is only doing a fetch, which I was doing as a manual step after the remote add. Another interesting artifact is that I know see the warning: no common commits log, which I wasn't seeing in my prior process. So, my problem is 'fixed' now, but it seems like this is a bug, particularly since most of the subtree merge tuturoials I've seen online do the manual fetch step. Is there any additional information that would be useful for folks to see? -Tom On Thu, Mar 28, 2013 at 12:29 PM, Thomas Taranowski t...@baringforge.com wrote: I agree that subtree solves some specific use cases I would like to support. In particular, I was hoping to use the subtree command in lieu of using the subtree merge strategy to manage and overlay changes to upstream projects, as well as other local components. At any rate, it looks like the problem I'm having is not entirely related to the subtree command, but happens when I checkout a remote into a branch ( which subtree is presumably doing in the background). It's the same setup as before. Here is the sequence of commands I'm running. git init git remote add upstream git://gnuradio.org/gnuradio fetch upstream git checkout -b upstream_tracking upstream/master Now, at this point, I expect the upstream branch to contain the contents of the gnuradio project. I also expect that my local mater branch has only the contents of my local sources, and NOT the contents of the gnuradio. However, if I 'git checkout master', I see the contents of the gnuradio project. Why, when I checkout a branch tracking upstream/master, do the changes also appear on my master branch, and not just in the remote tracking branch? As a reference, this is close to what I'm trying to accomplish. His screenshot titled 'Directory Listing in Master' shows what I expect. http://typecastexception.com/post/2013/03/16/Managing-Nested-Libraries-Using-the-GIT-Subtree-Merge-Workflow.aspx Thanks -Tom Taranowski On Thu, Mar 28, 2013 at 9:34 AM, Jeremy Rosen jeremy.ro...@openwide.fr wrote: I am starting to regret that I caved in and started carrying a copy of it in contrib/. It probably is a good idea to drop it from my tree and let it mature and eventually flourish outside. that's a shame... it solves a real problem, is simple to use, and really powerfull. but unfortunately, I have sent a patch that solves a serious bug... which had already been reported and patched but had received no answer, and nobody replied to it. Is there anything that can be done to get this rolling, or a way to have the use-case it covers better handle by git-submodule ? currently the problem of a git repo in a git repo is very complicated to deal with in a clean way... Regards Jérémy -- To unsubscribe from this list: send the line unsubscribe 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] attr.c::path_matches(): special case paths that end with a slash
On Tue, Mar 26, 2013 at 03:05:58PM -0400, Jeff King wrote: On Tue, Mar 26, 2013 at 11:39:30AM -0700, Junio C Hamano wrote: A similar adjustment for match_pathname() might be needed, but I didn't look into it. [...] We do seem to use strncmp_icase through the rest of the function, though, which should be OK. The one exception is that we call fnmatch at the end. Should the allocation hack from the previous patch make its way into an fnmatch_icase_bytes() function, so we can use it here, too? And then when we have a more efficient solution, we can just plug it in there. Hmm, yeah, there is more going on here than just that. If I add the tests below, the first one (a wildcard) passes, because you fixed the fnmatch code path. But the deep/ ones do not, as they should be going through match_pathname. I expected the deep/with/wildcard one to fail (because of the fnmatch problem I mentioned above), but not the deep/and/slashless one, which should be using strncmp. I'll see if I can track down the cause. -Peff --- diff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh index 3be809c..234a615 100755 --- a/t/t5002-archive-attr-pattern.sh +++ b/t/t5002-archive-attr-pattern.sh @@ -32,6 +32,21 @@ test_expect_success 'setup' ' git add ignored-without-slash/foo echo ignored-without-slash export-ignore .git/info/attributes + mkdir -p wildcard-without-slash + echo ignored without slash wildcard-without-slash/foo + git add wildcard-without-slash/foo + echo wild*-without-slash export-ignore .git/info/attributes + + mkdir -p deep/and/slashless + echo ignored without slash deep/and/slashless/foo + git add deep/and/slashless/foo + echo deep/and/slashless export-ignore .git/info/attributes + + mkdir -p deep/with/wildcard + echo ignored without slash deep/with/wildcard/foo + git add deep/with/wildcard/foo + echo deep/*t*/wildcard export-ignore .git/info/attributes + mkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir echo ignored by ignored dir one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir git add one-level-lower @@ -55,6 +70,12 @@ test_expect_missing archive/ignored-without-slash/foo test_expect_missingarchive/ignored-ony-if-dir/ignored-by-ignored-dir test_expect_missing archive/ignored-without-slash/ test_expect_missing archive/ignored-without-slash/foo +test_expect_missing archive/wildcard-without-slash/ +test_expect_missing archive/wildcard-without-slash/foo +test_expect_missing archive/deep/and/slashless/ +test_expect_missing archive/deep/and/slashless/foo +test_expect_missing archive/deep/with/wildcard/ +test_expect_missing archive/deep/with/wildcard/foo test_expect_exists archive/one-level-lower/ test_expect_missing archive/one-level-lower/two-levels-lower/ignored-only-if-dir/ test_expect_missing archive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Composing git repositories
Am 28.03.2013 11:01, schrieb Ramkumar Ramachandra: Jonathan Nieder wrote: Do you mean that you wish you could ignore subrepository boundaries and use commands like git clone --recurse-submodules http://git.zx2c4.com/cgit cd cgit vi git/cache.h ... edit edit edit ... git add --recurse-submodules git/cache.h git commit --recurse-submodules git push --recurse-submodules , possibly with configuration to allow the --recurse-submodules to be implied, and have everything work out well? Do you realize how difficult this is to implement? We'll need to patch all the git commands to essentially do what we'd get for free if the submodule were a tree object instead of a commit object (although I'm not saying that's the Right thing to do). Some caveats: - If we maintain one global index, and try to emulate git-subtree using submodules, we've lost. It's going to take freakin' ages to stat billions of files across hundreds of nested sumodules. One major advantage of having repository boundaries is separate object stores, indexes, worktrees: little ones that git is designed to work with. Are you aware that current Git code already stats all files across all submodules recursive by default? So (again) no problem here, we do that already (unless configured otherwise). - Auto-splitting commits that touch multiple submodules/ superproject at once. Although git-subtree does this, I think it's horribly ugly. You don't like it, but what technical argument is hidden here I'm missing? - Auto-propagating commits upwards to the superproject is another big challenge. I think the current design of anchoring to a specific commit SHA-1 has its usecases, but is unwieldy when things become big. We have to fix that first. What??? Again there is nothing to fix here, anchoring to a specific commit SHA-1 is *the* most prominent use case (think reproducibility of the whole work tree), floating submodules are the oddball here. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: More detailed error message for 403 forbidden.
On Thu, Mar 28, 2013 at 12:11:55PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: One problem is that the content body sent along with the error is not necessarily appropriate for showing to the user (e.g., if it is HTML, it is probably not a good idea to show it on the terminal). So I think we would want to only show it when the server has indicated via the content-type that the message is meant to be shown to the user. I'm thinking the server would generate something like: HTTP/1.1 403 Forbidden Content-type: application/x-git-error-message User 'me' does not have enough permission to access the repository. which would produce the example you showed above. Actually, isn't the human-readable part of the server response meant for this kind of thing? I.e. HTTP/1.1 403 User 'me' not allowed to accept the repository. In theory, yes. But I don't think that most servers make it very easy to use custom reason phrases (that is the rfc 2616 term for them). At least I could not easily figure out how to make Apache do so. You can do so from CGIs, but I think you'd want to customize some of this at the HTTP server level (e.g., overriding 404s with a custom message). There's much better support at that level for custom error documents (e.g., Apache's ErrorDocument). I do not configure http servers very often, though, so I could be wrong about what is normal practice, and what is easy to do. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Composing git repositories
Am 28.03.2013 12:48, schrieb Ramkumar Ramachandra: Okay, here's a first draft of the new design. The new mediator object should look like: name = git ref = v1.7.8 The name is looked up in refs/modules/branch, which in turn looks like: [submodule git] origin = gh:artagnon/git path = git [submodule magit] origin = gh:magit/magit path = git/extensions/magit What happens when you rename magit to foo in that branch and want to check out an older commit of the same branch? That is one of the reasons why that belongs in to a checked in .gitmodules and not someplace untracked. This solves the two problems that I brought up earlier: - Floating submodules (which are _necessary_ if you don't want to propagate commits upwards to the root). If you don't want that either don't use submodules or set the ignore config so you won't be bothered with any changes to the submodules. Floating up to the submodule's tip can be easily achieved with a script (possibly checked in in the superproject). You loose the reproducibility by doing that, but that's what you asked for. No problem here. - Initializing a nested submodule without having to initialize all the submodules in the path leading up to it. You cannot access a nested sub-submodule without its parent telling you what submodules it has. Otherwise the first level submodule would not be self-contained, so you'll need to check it out too to access the sub-submodules. Nothing to fix here either. However, I suspect that we can put more information the mediator object to make life easier for the parent repository and make seams disappear. I'm currently thinking about what information git core needs to behave smoothly with submodules. To me your proposal is trying to fix non-issues and breaking stuff that works, so I see no improvement. -- To unsubscribe from this list: send the line unsubscribe 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] git-send-email.perl: implement suggestions made by perlcritic
Ramkumar Ramachandra artag...@gmail.com writes:
Running perlcritic with gentle severity reports six problems. The
following lists the line numbers on which the problems occur, along
with a description of the problem. This patch fixes them all,
Thanks.
after
carefully considering the consequences.
H.
516: Contrary to common belief, subroutine prototypes do not enable
compile-time checks for proper arguments. They serve the singular
purpose of defining functions that behave like built-in functions, but
check_file_rev_conflict was never intended to behave like one. We
have verified that the callers of the subroutines are alright with the
change.
This, together with the carefully considering, does not build any
confidence on the part of the reader. Subroutine prototypes are not
for compile-time checks (correct), and they were introduced to the
language only for the purpose of letting you write subroutine that
emulate built-ins like pop @ary (again correct),
The most important fact that is not described in the above is that
by using prototype, the subroutine enforces a non-default context at
the callsite when the arguments are evaluated. That is why you can
write an emulated pop; otherwise the first @ary will simply be
flattened and you won't grab an element from the array.
Saying check_file_rev_conflict is not emulating any Perl built-in
function is irrelevant as a justification for the change. A
subroutine that does not emulate a built-in can still be relying on
the non-default context in which its arguments are evaluated.
sub foo ($) { my ($arg) = @_; print $arg\n; }
sub bar { my ($arg) = @_; print $arg\n; }
my @baz = (100, 101, 102);
foo @baz; # says 3
bar @baz; # says 100
In general, writing subroutines without prototypes is much
preferred. I do not dispute that and would not argue against
perlcritic. But if you blindly _fix_ the above foo subroutine by
dropping its prototype, it changes behaviour if the callers passed
an array variable. You need to check the callers and they are not
doing something to cause the prototyped and unprototyped versions
to behave differently.
And that is what needs to be explained in the log message; not these
handwavy carefully considering consequences (what consequences did
you consider?), was never intended to behave like one (how does
that matter?) and the callers of the subroutines are alright (why
do you think so?).
Without that, the reviewer needs to go and check the callers. Your
log message is _not_ helping them.
Same for the remainder.
In general, you do not have to copy and paste the output from
perlcritic. Treat it more as a learning tool, and use the knowledge
you learned from its output to explain why your changes are
improvements. Not just because perlcritic said so.
For ask subroutine, all its callers assign the returned value to a
single scaler variable, so the difference between return undef and
just return does not matter. If somebody starts doing
@foo = ask(...);
if (@foo) { ... we got an answer ... } else { ... we did not ... }
then return undef; form will break. So it is less error prone if
we dropped the explicit undef. The same goes for extract_valid_address,
whose current callers all assign the returned value to a single scalar,
or apply ! operator inside while conditional.
The change to validate_address is correct, but it is correct only
because its only caller, validate_address_list, filters out undef
returned from map() that calls this subroutine. By dropping the
explicit undef from there, it seems to me that validate_address no
longer returns undef so validate_address_list loses any need to
filter its return value. Seeing a patch that does not change that
caller while changing the callee makes reviewers wonder what is
going on. Perhaps even with this patch, there still is a need to
filter in validate_address_list, and if so, that needs to be
explained.
If I were doing this change, I would rather leave this subroutine
as-is. Nothing is broken and we are risking new breakages by
changing it.
1441: The three-argument form of `open' (introduced in Perl 5.6)
prevents subtle bugs that occur when the filename starts with funny
characters like '' or ''.
Correct, and this patch is about using the three-or-more-arg form to
take advantage of its safety. So why are we still using the shell
invocation form inherited back from the days we used two-arg form?
Again, seeing a patch that only turns open FILEHANDLE,EXPR into
open FILEHANDLE,MODE,EXPR when EXPR is not a simple command name
and not into open FILEHANDLE,MODE,EXPR,LIST form makes reviewers
wonder why the patch stops in the middle.
Junio: In future, please tell me explicitly that you're expecting a
re-roll with an updated commit message. It wasn't obvious to me at
all.
It wasn't obvious to me, either ;-).
I said the patch was not explained well, but it may not be the only
problem with it. Other
Re: Git and GSoC 2013
On Thu, Mar 28, 2013 at 5:45 PM, Junio C Hamano gits...@pobox.com wrote: Christian Couder christian.cou...@gmail.com writes: On Wed, Mar 27, 2013 at 7:52 PM, Jonathan Nieder jrnie...@gmail.com wrote: Jeff King wrote: There was a big thread about a month ago on whether Git should do Google Summer of Code this year[1]. I think we should do it. It looks strange to me to say that students are great and at the same time that we should not do it. Let's give them and us one more chance do to well. This is the only way we can improve. Do you mean we should be doing the same thing over and over again and expecting different results? Einstein may not like it, and I certainly don't. No, I don't mean we should be doing the same. I agree that smaller projects are helpful and insisting on submitting right away on the mailing list is helpful. But if we don't even try we have no chance to see if it works. We just lose time. What I gathered from the discussion so far is that everybody agrees that our mentoring has been suboptimal in various ways (not enough encouragement to engage with the community early, working in the cave for too long, biting too much to chew etc.). What makes you think we would do better this year? The fact that we will be more conscious that we need smaller projects and that we need to push even more for students to send their patch soon on the mailing list. If it doesn't work at all we will be set and we will know that there is not much we can do to make it work. If we don't even try we will not know soon, so not be able to improve or decide to stop. It's like software or science. If you don't test soon your hypothesis you don't progress fast. Or do you think we just stand no chance to progress? By the way we say that students should post soon to the mailing list to get a feedback soon, but it looks like we don't want to try our hypothesis around mentoring as soon as we can. Doesn't it sound strange to you? Aren't we saying do as I say not as I do? We have a track record of being not great at mentoring, and we haven't made an effort to improve it. is a perfectly valid and humble reason to excuse ourselves from this year's GSoC. It is also a perfectly valid justification to decide to make an effort to improve our mentoring and to try again. Students are great is immaterial. We are not great at mentoring is as much immaterial. In fact, if they are great, I think it is better to give them a chance to excel by working with organizations that can mentor them better, instead of squandering their time and GSoC's money for another failure, until _we_ are ready to take great students. How do we know we are ready if we don't try? By waiting we just lose the experience we already have, because some mentors might not be around next year, or they will not remember well about the process. And some organizations that will perhaps be accepted, if we decide not to do it, might have no mentoring experience at all. How do you know they will mentor students better than what we have been doing? Best regards, Christian. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Composing git repositories
Am 28.03.2013 10:16, schrieb Ramkumar Ramachandra: Jens Lehmann wrote: Unless you acknowledge that submodules are a different repo, you'll always run into problems. I believe future enhancements will make this less tedious, but in the end they will stay separate repos (which is the whole point, you'd want to use a different approach - e.g. subtree - if you want to put stuff from different upstreams into a single repo without keeping the distinction where all that came from). I acknowledge that it's a different repository. It's just that I think that our current design has too many seams: why do you think it's impossible to make it seamless? git-subtree is not an answer to anything. Dumping all the history into one repository has its limited usecases, but it is no solution. Guess what: submodules are the solution for a certain set of use cases, and tools like subtree are a solution for another set of use cases. There is no silver bullet. What else than a commit object should that be??? Submodules are there to have a different upstream for a part of your work tree, and that means a commit in that repo is the only sane thing to record in the superproject. A lot of thought has been put into this, and it is definitely a good choice [1]. Linus argues that it shouldn't be a tree object, and I agree with that. I don't see an argument that says that the commit object is a perfect fit (probably because it's not). There was discussion about what to record in the index/commit of the superproject in early submodule days (some time before I became involved in Git, seems I currently cannot find a link to that). A commit is the thing to record here because it *is* the perfect fit, as some years of submodule experience show. How? The submodules suck, we should try a completely different approach thingy comes up from time to time, but so far nobody could provide a viable alternative to what we currently do. My argument is not submodules suck; we should throw them out of the window, and start from scratch at all. I'm merely questioning the fundamental assumptions that submodules make, instead of proposing that we work around everything in shell. We don't have to be married to the existing implementation of submodules and try to fix all the problems in shell. You cannot simply change the fundamental assumptions of submodules and expect them to be the same thing afterwards. And it doesn't matter at all if we fix all the problems in shell or in C-code, we'll fix the remaining problems that are fixable in whatever part of Git it makes sense. And I don't have the impression you have an idea about what submodules are good at, where they can be improved and what problems they'll probably never solve. And apart from that, let's not forget we identified some valuable improvements to submodules in this thread: [...] All of those are topics I like to see materialize, and you are welcome to tackle them. Allow me a few days to think about changing the fundamental building blocks to make our shell hackery easier. Please go ahead, but if your goal is to make our shell hackery easier I'm not interested. I want to improve the user experience of submodules and don't care much in what language we achieve that. And I can't see anything fundamental being wrong with submodules but strongly believe they are a perfect match for some very important use cases (some of which I see happening at my $dayjob for some years now), so I still don't see what you are trying to fix here. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2013, #07; Tue, 26)
John Keeping j...@keeping.me.uk writes: On Wed, Mar 27, 2013 at 03:15:44PM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: On Wed, Mar 27, 2013 at 02:47:25PM -0700, Junio C Hamano wrote: * jk/difftool-dir-diff-edit-fix (2013-03-14) 3 commits (merged to 'next' on 2013-03-19 at e68014a) + difftool --dir-diff: symlink all files matching the working tree + difftool: avoid double slashes in symlink targets + git-difftool(1): fix formatting of --symlink description I lost track of various discussions on difftool and its symlink so that the user can edit working tree files in the tool. Would it be easiest if I send a new series incorporating jk/difftool-dirr-diff-edit-fix and the proposed change to not overwrite modified working tree files, built on top of t7800-modernize? I am somewhat reluctant to rewind a topic that has been cooking in 'next' for over a week (the above says 19th). Rebuilding the style-fixes on top of the above is fine---that topic is much younger. Sadly that's easier said than done, since it just introduces further conflicts as jk/difftool-dir-diff-edit-fix doesn't include da/difftool-fixes (now in master). OK, let's make it simpler then by merging jk/difftool-dir-diff-edit-fix to 'master'. The test tweaks and other work can then built on top. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/6] attribute regression fix for maint-1.8.1 and upward
On Tue, Mar 26, 2013 at 11:39:27AM -0700, Junio C Hamano wrote: So here is an attempt to fix the unintended regression, on top of 9db9eecfe5c2 (attr: avoid calling find_basename() twice per path, 2013-01-16). It consists of four patches. Here's my update to the series. I think this should fix all of the issues. And it should be very easy to drop in Duy's nwildmatch later on; it can just replace the fnmatch_icase_mem function added in patch 2 below. The main fix in this iteration is that match_pathname receives the same treatment as match_basename, which is done in patches 3 and 4 (the issues were subtly different enough that I didn't want to squash it all together; plus, gotta keep that commit count up). [1/6]: attr.c::path_matches(): the basename is part of the pathname [2/6]: dir.c::match_basename(): pay attention to the length of string parameters [3/6]: dir.c::match_pathname(): adjust patternlen when shifting pattern [4/6]: dir.c::match_pathname(): pay attention to the length of string parameters [5/6]: attr.c::path_matches(): special case paths that end with a slash [6/6]: t: check that a pattern without trailing slash matches a directory -Peff PS I followed your subject-naming convention since I was adding into your series, but it seems quite long to me. I would have just said: match_basename: pay attention -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/6] attr.c::path_matches(): the basename is part of the pathname
From: Junio C Hamano gits...@pobox.com
The function takes two strings (pathname and basename) as if they
are independent strings, but in reality, the latter is always
pointing into a substring in the former.
Clarify this relationship by expressing the latter as an offset into
the former.
Signed-off-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Jeff King p...@peff.net
---
This is identical to the original 1/4.
attr.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/attr.c b/attr.c
index ab2aab2..4cfe0ee 100644
--- a/attr.c
+++ b/attr.c
@@ -655,7 +655,7 @@ static int path_matches(const char *pathname, int pathlen,
}
static int path_matches(const char *pathname, int pathlen,
- const char *basename,
+ int basename_offset,
const struct pattern *pat,
const char *base, int baselen)
{
@@ -667,8 +667,8 @@ static int path_matches(const char *pathname, int pathlen,
return 0;
if (pat-flags EXC_FLAG_NODIR) {
- return match_basename(basename,
- pathlen - (basename - pathname),
+ return match_basename(pathname + basename_offset,
+ pathlen - basename_offset,
pattern, prefix,
pat-patternlen, pat-flags);
}
@@ -701,7 +701,7 @@ static int fill_one(const char *what, struct match_attr *a,
int rem)
return rem;
}
-static int fill(const char *path, int pathlen, const char *basename,
+static int fill(const char *path, int pathlen, int basename_offset,
struct attr_stack *stk, int rem)
{
int i;
@@ -711,7 +711,7 @@ static int fill(const char *path, int pathlen, const char
*basename,
struct match_attr *a = stk-attrs[i];
if (a-is_macro)
continue;
- if (path_matches(path, pathlen, basename,
+ if (path_matches(path, pathlen, basename_offset,
a-u.pat, base, stk-originlen))
rem = fill_one(fill, a, rem);
}
@@ -750,7 +750,8 @@ static void collect_all_attrs(const char *path)
{
struct attr_stack *stk;
int i, pathlen, rem, dirlen;
- const char *basename, *cp, *last_slash = NULL;
+ const char *cp, *last_slash = NULL;
+ int basename_offset;
for (cp = path; *cp; cp++) {
if (*cp == '/' cp[1])
@@ -758,10 +759,10 @@ static void collect_all_attrs(const char *path)
}
pathlen = cp - path;
if (last_slash) {
- basename = last_slash + 1;
+ basename_offset = last_slash + 1 - path;
dirlen = last_slash - path;
} else {
- basename = path;
+ basename_offset = 0;
dirlen = 0;
}
@@ -771,7 +772,7 @@ static void collect_all_attrs(const char *path)
rem = attr_nr;
for (stk = attr_stack; 0 rem stk; stk = stk-prev)
- rem = fill(path, pathlen, basename, stk, rem);
+ rem = fill(path, pathlen, basename_offset, stk, rem);
}
int git_check_attr(const char *path, int num, struct git_attr_check *check)
--
1.8.2.13.g0f18d3c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters
From: Junio C Hamano gits...@pobox.com
The function takes two counted strings (basename, basenamelen and
pattern, patternlen) as parameters, together with prefix (the
length of the prefix in pattern that is to be matched literally
without globbing against the basename) and EXC_* flags that tells it
how to match the pattern against the basename.
However, it did not pay attention to the length of these counted
strings. Update them to do the following:
* When the entire pattern is to be matched literally, the pattern
matches the basename only when the lengths of them are the same,
and they match up to that length.
* When the pattern is * followed by a string to be matched
literally, make sure that the basenamelen is equal or longer than
the literal part of the pattern, and the tail of the basename
string matches that literal part.
* Otherwise, use the new fnmatch_icase_mem helper to make
sure we only lookmake sure we use only look at the
counted part of the strings. Because these counted strings are
full strings most of the time, we check for termination
to avoid unnecessary allocation.
Signed-off-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Jeff King p...@peff.net
---
Compared to v1:
- This factors the fnmatch bits into a helper function so we can reuse it
later. As a result, the variable names are changed a bit.
- The original did:
if (use_pat)
strbuf_release(pat);
but AFAICT that was a useless conditional; use_pat always points to
either the incoming buffer or the strbuf. But strbuf_release will
handle both cases for us.
dir.c | 40
1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/dir.c b/dir.c
index 5a83aa7..fac82c1 100644
--- a/dir.c
+++ b/dir.c
@@ -34,6 +34,33 @@ int fnmatch_icase(const char *pattern, const char *string,
int flags)
return fnmatch(pattern, string, flags | (ignore_case ? FNM_CASEFOLD :
0));
}
+static int fnmatch_icase_mem(const char *pattern, int patternlen,
+const char *string, int stringlen,
+int flags)
+{
+ int match_status;
+ struct strbuf pat_buf = STRBUF_INIT;
+ struct strbuf str_buf = STRBUF_INIT;
+ const char *use_pat = pattern;
+ const char *use_str = string;
+
+ if (pattern[patternlen]) {
+ strbuf_add(pat_buf, pattern, patternlen);
+ use_pat = pat_buf.buf;
+ }
+ if (string[stringlen]) {
+ strbuf_add(str_buf, string, stringlen);
+ use_str = str_buf.buf;
+ }
+
+ match_status = fnmatch_icase(use_pat, use_str, 0);
+
+ strbuf_release(pat_buf);
+ strbuf_release(str_buf);
+
+ return match_status;
+}
+
static size_t common_prefix_len(const char **pathspec)
{
const char *n, *first;
@@ -537,15 +564,20 @@ int match_basename(const char *basename, int basenamelen,
int flags)
{
if (prefix == patternlen) {
- if (!strcmp_icase(pattern, basename))
+ if (patternlen == basenamelen
+ !strncmp_icase(pattern, basename, basenamelen))
return 1;
} else if (flags EXC_FLAG_ENDSWITH) {
+ /* *literal matching against fooliteral */
if (patternlen - 1 = basenamelen
- !strcmp_icase(pattern + 1,
- basename + basenamelen - patternlen + 1))
+ !strncmp_icase(pattern + 1,
+ basename + basenamelen - (patternlen - 1),
+ patternlen - 1))
return 1;
} else {
- if (fnmatch_icase(pattern, basename, 0) == 0)
+ if (fnmatch_icase_mem(pattern, patternlen,
+ basename, basenamelen,
+ 0) == 0)
return 1;
}
return 0;
--
1.8.2.13.g0f18d3c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/6] dir.c::match_pathname(): adjust patternlen when shifting pattern
If we receive a pattern that starts with /, we shift it
forward to avoid looking at the / part. Since the prefix
and patternlen parameters are counts of what is in the
pattern, we must decrement them as we increment the pointer.
We remembered to handle prefix, but not patternlen. This
didn't cause any bugs, though, because the patternlen
parameter is not actually used. Since it will be used in
future patches, let's correct this oversight.
Signed-off-by: Jeff King p...@peff.net
---
New in this iteration.
dir.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/dir.c b/dir.c
index fac82c1..cc4ce8b 100644
--- a/dir.c
+++ b/dir.c
@@ -597,6 +597,7 @@ int match_pathname(const char *pathname, int pathlen,
*/
if (*pattern == '/') {
pattern++;
+ patternlen--;
prefix--;
}
--
1.8.2.13.g0f18d3c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/6] attr.c::path_matches(): special case paths that end with a slash
From: Junio C Hamano gits...@pobox.com
The function is given a string that ends with a slash to signal that
the path is a directory to make sure that a pattern that ends with a
slash (i.e. MUSTBEDIR) can tell directories and non-directories
apart. However, the pattern itself (pat-pattern and
pat-patternlen) that came from such a MUSTBEDIR pattern is
represented as a string that ends with a slash, but patternlen does
not count that trailing slash. A MUSTBEDIR pattern element/ is
represented as a counted string element/, 7 and this must match
match pathname element/.
Because match_basename() and match_pathname() want to see pathname
element to match against the pattern element/, 7, reduce the
length of the path to exclude the trailing slash when calling
these functions.
Signed-off-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Jeff King p...@peff.net
---
Tweaked since v1 to also drop the trailing slash when we pass the path
to match_pathname.
attr.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/attr.c b/attr.c
index 4cfe0ee..4d620bc 100644
--- a/attr.c
+++ b/attr.c
@@ -661,18 +661,18 @@ static int path_matches(const char *pathname, int pathlen,
{
const char *pattern = pat-pattern;
int prefix = pat-nowildcardlen;
+ int isdir = (pathlen pathname[pathlen - 1] == '/');
- if ((pat-flags EXC_FLAG_MUSTBEDIR)
- ((!pathlen) || (pathname[pathlen-1] != '/')))
+ if ((pat-flags EXC_FLAG_MUSTBEDIR) !isdir)
return 0;
if (pat-flags EXC_FLAG_NODIR) {
return match_basename(pathname + basename_offset,
- pathlen - basename_offset,
+ pathlen - basename_offset - isdir,
pattern, prefix,
pat-patternlen, pat-flags);
}
- return match_pathname(pathname, pathlen,
+ return match_pathname(pathname, pathlen - isdir,
base, baselen,
pattern, prefix, pat-patternlen, pat-flags);
}
--
1.8.2.13.g0f18d3c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/6] t: check that a pattern without trailing slash matches a directory
Prior to v1.8.1.1, with: git init echo content foo mkdir subdir echo content subdir/bar echo subdir export-ignore .gitattributes git add . git commit -m one git archive HEAD | tar tf - the resulting archive would contain only foo and .gitattributes, not subdir. This was broken with a recent change that intended to allow subdir/ export-ignore to also exclude the directory, but instead ended up _requiring_ the trailing slash by mistake. A pattern subdir should match any path subdir, whether it is a directory or a non-diretory. A pattern subdir/ insists that a path subdir must be a directory for it to match. This patch adds test not just for this simple case, but also for deeper cross-directory cases, as well as cases with wildcards. Signed-off-by: Jeff King p...@peff.net --- Added new tests since v1 that handle the match_pathname code path. t/t5002-archive-attr-pattern.sh | 27 +++ 1 file changed, 27 insertions(+) diff --git a/t/t5002-archive-attr-pattern.sh b/t/t5002-archive-attr-pattern.sh index 0c847fb..6667d15 100755 --- a/t/t5002-archive-attr-pattern.sh +++ b/t/t5002-archive-attr-pattern.sh @@ -27,6 +27,25 @@ test_expect_success 'setup' ' echo ignored-only-if-dir/ export-ignore .git/info/attributes git add ignored-only-if-dir + mkdir -p ignored-without-slash + echo ignored without slash ignored-without-slash/foo + git add ignored-without-slash/foo + echo ignored-without-slash export-ignore .git/info/attributes + + mkdir -p wildcard-without-slash + echo ignored without slash wildcard-without-slash/foo + git add wildcard-without-slash/foo + echo wild*-without-slash export-ignore .git/info/attributes + + mkdir -p deep/and/slashless + echo ignored without slash deep/and/slashless/foo + git add deep/and/slashless/foo + echo deep/and/slashless export-ignore .git/info/attributes + + mkdir -p deep/with/wildcard + echo ignored without slash deep/with/wildcard/foo + git add deep/with/wildcard/foo + echo deep/*t*/wildcard export-ignore .git/info/attributes mkdir -p one-level-lower/two-levels-lower/ignored-only-if-dir echo ignored by ignored dir one-level-lower/two-levels-lower/ignored-only-if-dir/ignored-by-ignored-dir @@ -49,6 +68,14 @@ test_expect_missing archive/ignored-ony-if-dir/ignored-by-ignored-dir test_expect_exists archive/not-ignored-dir/ test_expect_missingarchive/ignored-only-if-dir/ test_expect_missingarchive/ignored-ony-if-dir/ignored-by-ignored-dir +test_expect_missingarchive/ignored-without-slash/ +test_expect_missingarchive/ignored-without-slash/foo +test_expect_missingarchive/wildcard-without-slash/ +test_expect_missingarchive/wildcard-without-slash/foo +test_expect_missingarchive/deep/and/slashless/ +test_expect_missingarchive/deep/and/slashless/foo +test_expect_missingarchive/deep/with/wildcard/ +test_expect_missingarchive/deep/with/wildcard/foo test_expect_exists archive/one-level-lower/ test_expect_missing archive/one-level-lower/two-levels-lower/ignored-only-if-dir/ test_expect_missing archive/one-level-lower/two-levels-lower/ignored-ony-if-dir/ignored-by-ignored-dir -- 1.8.2.13.g0f18d3c -- To unsubscribe from this list: send the line unsubscribe 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] t: check that a pattern without trailing slash matches a directory
On Thu, Mar 28, 2013 at 5:50 PM, Jeff King p...@peff.net wrote: A pattern subdir should match any path subdir, whether it is a directory or a non-diretory. A pattern subdir/ insists that a s/diretory/directory/ [1] path subdir must be a directory for it to match. [1]: http://article.gmane.org/gmane.comp.version-control.git/219185/ -- To unsubscribe from this list: send the line unsubscribe 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] t: check that a pattern without trailing slash matches a directory
On Thu, Mar 28, 2013 at 06:21:08PM -0400, Eric Sunshine wrote: On Thu, Mar 28, 2013 at 5:50 PM, Jeff King p...@peff.net wrote: A pattern subdir should match any path subdir, whether it is a directory or a non-diretory. A pattern subdir/ insists that a s/diretory/directory/ [1] I think I've taken proofreading to a new level by missing the error I already corrected somebody else for. Thanks for noticing. :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters
Jeff King p...@peff.net writes: This function takes two counted strings: a pattern, patternlen pair and a pathname, pathlen pair. But we end up feeding the result to fnmatch, which expects NUL-terminated strings. We can fix this by calling the fnmatch_icase_mem function, which handles re-allocating into a NUL-terminated string if necessary. While we're at it, we can avoid even calling fnmatch in some cases. In addition to patternlen, we get prefix, the size of the pattern that contains no wildcard characters. We do a straight match of the prefix part first, and then use fnmatch to cover the rest. But if there are no wildcards in the pattern at all, we do not even need to call fnmatch; we would simply be comparing two empty strings. It is not a new issue, but it would be nicer to have a comment to warn people that this part needs to be adjusted when they want to add support for using FNM_PERIOD in our codebase. Other than that, looks sane to me. Thanks. Signed-off-by: Jeff King p...@peff.net --- New in this iteration. dir.c | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/dir.c b/dir.c index cc4ce8b..3ad44c3 100644 --- a/dir.c +++ b/dir.c @@ -624,11 +624,22 @@ int match_pathname(const char *pathname, int pathlen, if (strncmp_icase(pattern, name, prefix)) return 0; pattern += prefix; + patternlen -= prefix; name+= prefix; namelen -= prefix; + + /* + * If the whole pattern did not have a wildcard, + * then our prefix match is all we need; we + * do not need to call fnmatch at all. + */ + if (!patternlen !namelen) + return 1; } - return fnmatch_icase(pattern, name, FNM_PATHNAME) == 0; + return fnmatch_icase_mem(pattern, patternlen, + name, namelen, + FNM_PATHNAME) == 0; } /* Scan the list and let the last match determine the fate. -- To unsubscribe from this list: send the line unsubscribe 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] checkout: avoid unnecessary match_pathspec calls
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: diff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh index 56090d2..5e01d58 100755 --- a/t/t2022-checkout-paths.sh +++ b/t/t2022-checkout-paths.sh @@ -39,4 +39,25 @@ test_expect_success 'checking out paths out of a tree does not clobber unrelated test_cmp expect.next2 dir/next2 ' +test_expect_success 'do not touch unmerged entries matching $path but not in $tree' ' + git checkout next + git reset --hard + + cat dir/common expect.common + EMPTY_SHA1=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 EMPTY_SHA1=$(git hash-object -w --stdin /dev/null) + git rm dir/next0 + cat expect.next0EOF +100644 $EMPTY_SHA1 1 dir/next0 +100644 $EMPTY_SHA1 2 dir/next0 +EOF + git update-index --index-info expect.next0 cat expect.next0 -EOF 100644 $EMPTY_SHA1 1 dir/next0 100644 $EMPTY_SHA1 2 dir/next0 EOF git update-index --index-info expect.next0 + + git checkout master dir + + test_cmp expect.common dir/common + test_path_is_file dir/master + git diff --exit-code master dir/master + git ls-files -s dir/next0 actual.next0 +' ... and actual.next0 is checked against what? Ending this test with git ls-files -s dir/next0 actual.next0 test_cmp expect.next0 actual.next0 would be sufficient, methinks. Will replace v2 with the above fixups. Thanks. + test_done -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/5] commit.c/GPG signature verification: Also look at the first GPG status line
Sebastian Götte ja...@physik.tu-berlin.de writes:
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
commit.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/commit.c b/commit.c
index 1aeb17a..533727c 100644
--- a/commit.c
+++ b/commit.c
@@ -1027,8 +1027,8 @@ static struct {
char result;
const char *check;
} signature_check[] = {
- { 'G', \n[GNUPG:] GOODSIG },
- { 'B', \n[GNUPG:] BADSIG },
+ { 'G', [GNUPG:] GOODSIG },
+ { 'B', [GNUPG:] BADSIG },
};
static void parse_signature_lines(struct signature *sig)
@@ -1041,6 +1041,9 @@ static void parse_signature_lines(struct signature *sig)
const char *next;
if (!found)
continue;
+ if (found != buf)
+ if (found[-1] != '\n')
+ continue;
It would be much easier to read if it were unless we are not
looking at the very first byte, the previous byte must be LF, i.e.
if (found != buf found[-1] != '\n')
Is that continue correct? Don't you want to retry from the end of
the line that contains the mistaken hit?
The \n at the beginning anchors the expected string for quicker
multi-line scan done with strstr(). If you really want to lose that
LF and still write this function correctly and clearly, I think you
would need to iterate over the buffer line by line.
What you want to do may be more like this, I think.
commit.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/commit.c b/commit.c
index d093c9c..d6e0b00 100644
--- a/commit.c
+++ b/commit.c
@@ -1045,8 +1045,8 @@ static struct {
char result;
const char *check;
} signature_check[] = {
- { 'G', [GNUPG:] GOODSIG },
- { 'B', [GNUPG:] BADSIG },
+ { 'G', \n[GNUPG:] GOODSIG },
+ { 'B', \n[GNUPG:] BADSIG },
};
static void parse_signature_lines(struct signature *sig)
@@ -1055,15 +1055,18 @@ static void parse_signature_lines(struct signature *sig)
int i;
for (i = 0; i ARRAY_SIZE(signature_check); i++) {
- const char *found = strstr(buf, signature_check[i].check);
- const char *next;
- if (!found)
- continue;
- if (found != buf)
- if (found[-1] != '\n')
+ const char *found, *next;
+
+ if (!prefixcmp(buf, signature_check[i].check + 1)) {
+ /* At the very beginning of the buffer */
+ found = buf + strlen(signature_check[i].check + 1);
+ } else {
+ found = strstr(buf, signature_check[i].check);
+ if (!found)
continue;
+ found += strlen(signature_check[i].check);
+ }
sig-check_result = signature_check[i].result;
- found += strlen(signature_check[i].check);
sig-key = xmemdupz(found, 16);
found += 17;
next = strchrnul(found, '\n');
--
To unsubscribe from this list: send the line unsubscribe 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 3/5] merge/pull: verify GPG signatures of commits being merged
Sebastian Götte ja...@physik.tu-berlin.de writes:
When --verify-signatures is specified on the command-line of git-merge
or git-pull, check whether the commits being merged have good gpg
signatures and abort the merge in case they do not. This allows e.g.
auto-deployment from untrusted repo hosts.
Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
...
+ if (verify_signatures) {
+ /* Verify the commit signatures */
+ for (p = remoteheads; p; p = p-next) {
+ struct commit *commit = p-item;
+ struct signature signature;
+ signature.check_result = 0;
Even though you may happen to know all other fields are output only,
it is a good practice to clear it with
memset(signature, 0, sizeof(signature);
By the way, I think this variable and type should be called more
like signature_check or something with check in its name, not
signature. After all it is _not_ a signature itself.
+ check_commit_signature(commit, signature);
+
+ char hex[41];
builtin/merge.c: In function 'cmd_merge':
builtin/merge.c:1247: error: ISO C90 forbids mixed declarations and code
+
+test_expect_success GPG 'merge unsigned commit with verification' '
+ test_must_fail git merge --ff-only --verify-signatures side-unsigned 2
mergeerror
No SP between redirection and the target filename.
+ grep does not have a GPG signature mergeerror
Do we plan to make this message localized? If so I think you would
need to do this with test_i18ngrep.
+'
+
+test_expect_success GPG 'merge commit with bad signature with verification' '
+ test_must_fail git merge --ff-only --verify-signatures $(cat
forged.commit) 2 mergeerror
+ grep has a bad GPG signature mergeerror
+'
+
+test_expect_success GPG 'merge signed commit with verification' '
+ git merge -v --ff-only --verify-signatures side-signed mergeoutput
+ grep has a good GPG signature mergeoutput
+'
Hmph.
So the caller needs to check both the standard output and the
standard error to get the whole picture? Is there a reason why we
are not sending everything to the standard output consistently?
+test_expect_success GPG 'merge commit with bad signature without
verification' '
+ git merge $(cat forged.commit)
+'
Good to see that negative case where the new feature should _not_
trigger is properly tested.
+
+test_done
--
To unsubscribe from this list: send the line unsubscribe 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] dir.c::match_basename(): pay attention to the length of string parameters
On Thu, Mar 28, 2013 at 05:47:28PM -0400, Jeff King wrote: From: Junio C Hamano gits...@pobox.com The function takes two counted strings (basename, basenamelen and pattern, patternlen) as parameters, together with prefix (the length of the prefix in pattern that is to be matched literally without globbing against the basename) and EXC_* flags that tells it how to match the pattern against the basename. However, it did not pay attention to the length of these counted strings. Update them to do the following: * When the entire pattern is to be matched literally, the pattern matches the basename only when the lengths of them are the same, and they match up to that length. Hrm. Though the tip of this series passes all tests, this one actually breaks bisectability. What happens is that the existing code passes: path=foo/ pathlen=4 pattern=foo/ patternlen=3 match_basename is happy to compare foo/ to foo/ and realize they're equal. With this change, we compare foo to foo/ and do not match. It isn't until the later patch where you start passing pathlen=3 that it works again. I wonder if it is worth reordering the series to put the path_matches fix first. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters
On Thu, Mar 28, 2013 at 06:40:27PM -0400, Jeff King wrote: On Thu, Mar 28, 2013 at 05:47:28PM -0400, Jeff King wrote: From: Junio C Hamano gits...@pobox.com The function takes two counted strings (basename, basenamelen and pattern, patternlen) as parameters, together with prefix (the length of the prefix in pattern that is to be matched literally without globbing against the basename) and EXC_* flags that tells it how to match the pattern against the basename. However, it did not pay attention to the length of these counted strings. Update them to do the following: * When the entire pattern is to be matched literally, the pattern matches the basename only when the lengths of them are the same, and they match up to that length. Hrm. Though the tip of this series passes all tests, this one actually breaks bisectability. What happens is that the existing code passes: Ugh. That is a problem, but this series does _not_ pass all tests. I think I failed to run the complete test suite on the correct tip. My match_pathspec fix breaks at least t1011. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters
Jeff King p...@peff.net writes: On Thu, Mar 28, 2013 at 06:40:27PM -0400, Jeff King wrote: On Thu, Mar 28, 2013 at 05:47:28PM -0400, Jeff King wrote: From: Junio C Hamano gits...@pobox.com The function takes two counted strings (basename, basenamelen and pattern, patternlen) as parameters, together with prefix (the length of the prefix in pattern that is to be matched literally without globbing against the basename) and EXC_* flags that tells it how to match the pattern against the basename. However, it did not pay attention to the length of these counted strings. Update them to do the following: * When the entire pattern is to be matched literally, the pattern matches the basename only when the lengths of them are the same, and they match up to that length. Hrm. Though the tip of this series passes all tests, this one actually breaks bisectability. What happens is that the existing code passes: Ugh. That is a problem, but this series does _not_ pass all tests. I think I failed to run the complete test suite on the correct tip. My match_pathspec fix breaks at least t1011. Yeah, the tip of 'jch' (slightly ahead of 'next' that I use myself) has 0003, 1011 and 3001 broken X-. -- To unsubscribe from this list: send the line unsubscribe 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] dir.c::match_basename(): pay attention to the length of string parameters
On Fri, Mar 29, 2013 at 5:49 AM, Jeff King p...@peff.net wrote: My match_pathspec fix breaks at least t1011. Haven't looked closely at the series, but I suspect you need this http://article.gmane.org/gmane.comp.version-control.git/219008 -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters
On Fri, Mar 29, 2013 at 4:47 AM, Jeff King p...@peff.net wrote:
+static int fnmatch_icase_mem(const char *pattern, int patternlen,
+const char *string, int stringlen,
+int flags)
+{
+ int match_status;
+ struct strbuf pat_buf = STRBUF_INIT;
+ struct strbuf str_buf = STRBUF_INIT;
+ const char *use_pat = pattern;
+ const char *use_str = string;
+
+ if (pattern[patternlen]) {
+ strbuf_add(pat_buf, pattern, patternlen);
+ use_pat = pat_buf.buf;
+ }
+ if (string[stringlen]) {
+ strbuf_add(str_buf, string, stringlen);
+ use_str = str_buf.buf;
+ }
+
+ match_status = fnmatch_icase(use_pat, use_str, 0);
You should pass flags in here instead of 0.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] dir.c::match_basename(): pay attention to the length of string parameters
On Fri, Mar 29, 2013 at 08:25:00AM +0700, Nguyen Thai Ngoc Duy wrote:
On Fri, Mar 29, 2013 at 4:47 AM, Jeff King p...@peff.net wrote:
+static int fnmatch_icase_mem(const char *pattern, int patternlen,
+const char *string, int stringlen,
+int flags)
+{
+ int match_status;
+ struct strbuf pat_buf = STRBUF_INIT;
+ struct strbuf str_buf = STRBUF_INIT;
+ const char *use_pat = pattern;
+ const char *use_str = string;
+
+ if (pattern[patternlen]) {
+ strbuf_add(pat_buf, pattern, patternlen);
+ use_pat = pat_buf.buf;
+ }
+ if (string[stringlen]) {
+ strbuf_add(str_buf, string, stringlen);
+ use_str = str_buf.buf;
+ }
+
+ match_status = fnmatch_icase(use_pat, use_str, 0);
You should pass flags in here instead of 0.
Eek, yeah, that's obviously wrong. Thanks for catching it. Fixing that
clears up all of the test failures outside of t5002.
And if you move patch 5 (special case paths that end with a slash)
into position 2, it cleans up the mid-series failures of t5002, making
the series clean for later bisecting.
Thanks for looking it over.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Thu, 28 Mar 2013 12:25:07 -0700 Jonathan Nieder jrnie...@gmail.com wrote: Josh Triplett wrote: Related to this, as a path forward, I do think it makes sense to have a setting usable as an insteadOf that only applies to pushurl, even though pushInsteadOf won't end up serving that purpose. That way, pushInsteadOf covers the map read-only repo url to pushable repo url case, and insteadOfPushOnly covers the construct a magic prefix that maps to different urls when used in url or pushurl case. I hope not. That would be making configuration even more complicated. I hope that we can fix the documentation, tests, and change description in the commit message enough to make Rob's patch a no-brainer. If that's not possible, I think the current state is livable, just confusing. I was happy to see Rob's patch because it brings git's behavior closer to following the principle of least surprise. I am not actually that excited by the use case, except the avoiding surprise part of it. Hope that helps, Jonathan Thanks for all of the input, everyone. I personally agree with Jonathon's notion of principle of least surprise, as I was quite surprised when my configuration with pushInsteadOf + pushurl did not work. However, I also understand Junio's arguments about going back on documented behavior, as well as whether or not it's a good idea to have this triangular remote set up. Honestly, if my workflow here is stupid and not Git-like and someone has a better suggestion, I would happy to let my patch go. Using two remotes is an option, but to me, using this triangular setup is just easier. The only way I see this breaking an existing configuration is if a user has something like url.u...@server.com.pushInsteadOf = myserver:, and pushurl = myserver:repo/. If this behavior weren't documented, I would say that such a configuration works because it relies on a bug, and should use ssh://myserver:repo/ instead. I personally feel that the fact that insteadOf + url works and pushInsteadOf + pushurl doesn't is inconsistent and confusing. However, I am one user of many, and this is my first exposure to Git from a project contributor point of view. Therefore, I submit to the wisdom of more seasoned Git developers such as yourselves. =) -Rob -- To unsubscribe from this list: send the line unsubscribe 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 and GSoC 2013
Christian Couder christian.cou...@gmail.com writes: On Thu, Mar 28, 2013 at 5:45 PM, Junio C Hamano gits...@pobox.com wrote: What I gathered from the discussion so far is that everybody agrees that our mentoring has been suboptimal in various ways (not enough encouragement to engage with the community early, working in the cave for too long, biting too much to chew etc.). What makes you think we would do better this year? The fact that we will be more conscious that we need smaller projects and that we need to push even more for students to send their patch soon on the mailing list. If it doesn't work at all we will be set and we will know that there is not much we can do to make it work. That sounds like doing the same thing over and over again to me. I just looked at the ideas page Thomas sent the link to upthread this morning, but I didn't see any evidence that it has been been curated with we need smaller projects in mind. We will be more conscious? I cannot take that promise at face value after seeing that the page stayed the same since Thomas resurrected it from last year's ideas page ever since it was created. If we don't even try we will not know soon, so not be able to improve or decide to stop. It's like software or science. If you don't test soon your hypothesis you don't progress fast. The impression I am getting is that the concensus is that we do not even have hypothesis worth testing with a grant money from GSoC and students' time at this point, if I may borrow your science analogy. -- To unsubscribe from this list: send the line unsubscribe 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] push: Alias pushurl from push rewrites
Rob Hoelz r...@hoelz.ro writes: Honestly, if my workflow here is stupid and not Git-like and someone has a better suggestion, I would happy to let my patch go. Using two remotes is an option, but to me, using this triangular setup is just easier. I think you are conflating two unrelated things. Pulling from one repository to integrate others' work with yours (either by merging others' work to yours, or rebasing your work on others'), and pushing the result of your work to another repository to publish, i.e. triangular workflow, is no less Git-like than pulling from and pushing to the same repository. Both are valid workflows, and Git supports them. What is not correct in your set-up is that a single remote with URL and pushURL (or rewritten URL derived from them via pushInsteadOf and insteadOf) that point at two different repositories is *not* the way to express that triangular configuration. You name two remotes, pull from one and push to the other. If you look at Ram's triangular workflows series, cf. http://thread.gmane.org/gmane.comp.version-control.git/219387 you can see that a progress is being made to make the two remotes configuration easier to use. The discussion on the earliest iteration of the patch series, cf. http://thread.gmane.org/gmane.comp.version-control.git/215702 shows that even I initially made the same pointing two different repositories with URL and pushURL should be sufficient mistake, which was corrected by others. The primary issue is remote tracking branches are designed to keep track of the state of the branches at the named remote---for this reason alone, you must not name a logically different repository with URL and pushURL for a single remote. So that is one thing. tl;dr: Triangular workflow is valid. A single remote with URL and pushURL to point at the two remote repositories is not a valid way to express that workflow. The other thing is if it is worth risking to break the backward compatibility and hurting existing users in order to remove the strange To an explicit pushURL, insteadOf rewriting will not apply exception. The reason I didn't bring up the possible breakage of documented behaviour in the earlier review of this series is exactly because that special case was unintuitive, so you do not have to argue it is strange, unintuitive, and would not be a way we would have designed the system if we knew better. I already agree to that point, and I think others do, too. There is a gap between We would design it differently if we were building it now with what we know and We should change it and make it ideal and the gap is called existing users. These two are unrelated and independent. I suspect that Ram's triangular series, when it matures, will help your pull from there, push to another workflow in a different way. You will just define two remotes for these two places, and you may no longer need pushInsteadOf is not ignored when you have pushURL to solve your immediate issue. But removing the pushInsteadOf is ignored when explicit pushURL exists may still be a worthwhile thing to do, even if you do not need it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git subtree oddity
Jeremy Rosen jeremy.ro...@openwide.fr writes: I am starting to regret that I caved in and started carrying a copy of it in contrib/. It probably is a good idea to drop it from my tree and let it mature and eventually flourish outside. that's a shame... it solves a real problem, is simple to use, and really powerfull. I've never said the program does not solve a real problem, it is hard to use, nor it is useless. It just is not well maintained. I knew (and I said), from the very beginning when people started making noises about adding it to my tree, that I will not be a good maintainer for it. I am not its user, I do not know what its users expect out of the program, and I cannot read from its history what the developers were thinking when they designed its features and implemented its internals. I started carrying it in contrib/ only to give it wider exposure, but under the condition that somebody else would be the real maintainer for it. I'd say we should wait for at least a few days to see what David says. Perhaps he is too busy with other things. Perhaps he needs co-maintainers who are also interested in the program to help him. Leaving it in my tree without real maintenance is not an ideal state. I do not know why you think it is a shame. I honestly think it will do better outside my tree. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
