Re: [git-users] Git clone fails with "bad pack header", how to get remote log
Hi Konstantin, thanks for the reply. The versions of git are: - on remote: 1.5.6.5 - on windows build machine: 1.7.11.msysgit.1 - on mac build machine: 1.7.3.4 I will try to install latest git version on my remote server and get back to you. thanks again Kevin On 10/29/12 6:18 PM, Konstantin Khomoutov wrote: On Mon, 29 Oct 2012 09:52:54 -0700 (PDT) Kevin Molcard wrote: I have a problem with my build system. I have a remote server with a relatively large repository (around 12 GB, each branch having a size of 3 GB). I have also 2 build servers (Mac, Windows) that are cloning the repo from the remote. Sometimes (very often when several git clone are sent at the same time), I have the following error: remote: internal server error fatal: protocol error: bad pack header I know that it happens when the remote is compressing objects (thanks to `--progress -v` flags) because the last line of the log before the erro is: remote: Compressing objects: 93% (17959/19284) [K * So I have 2 questions, does anybody what is the problem and what should I do? * Is there a way to get a more precise log from the remote to debug this problem? This reminds me of a bug fixed in 1.7.12.1 [1]: * When "git push" triggered the automatic gc on the receiving end, a message from "git prune" that said it was removing cruft leaked to the standard output, breaking the communication protocol. In any case, bugs should be reported to the main Git list (which is git at vger.kernel.org), not here. I'm Cc'ing the main Git list so you'll get any responses from there, if any. Kevin, please answer to this message (keeping all the Ccs -- use "Reply to group" or "Reply to all" in your MUA) and describe exactly what Git versions on which platforms your have. 1. https://raw.github.com/git/git/master/Documentation/RelNotes/1.7.12.1.txt -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib: avoid full path to store test results
Elia Pinto wrote:
> The shell word splitting done in base is a bashism, iow not portable.
No, ${varname##glob} is in POSIX and we already use it here and there.
See Documentation/CodingGuidelines:
- We use ${parameter#word} and its [#%] siblings, and their
doubled "longest matching" form.
Thanks for looking the patch over.
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Links broken in ref docs.
Any follow-up on this? On Tue, Oct 23, 2012 at 7:11 AM, Scott Chacon wrote: > > So, this is due to the major AWS outage today. git-scm.com is hosted > on Heroku and thus on AWS. Heroku is continuing to bring up their > database systems in the wake of the massive AWS outage. Once that is > back online, git-scm.com will also be back online. > > As for the git-fetch issue, we'll look into it once Heroku is back online. > > Scott -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Links broken in ref docs.
Not seen any recently. I'm guessing the dev is in the path of hurricane Sandy? (Not sarcasm, btw.) On Tue, Oct 30, 2012 at 1:04 AM, Kevin wrote: > Any follow-up on this? > > On Tue, Oct 23, 2012 at 7:11 AM, Scott Chacon wrote: >> >> So, this is due to the major AWS outage today. git-scm.com is hosted >> on Heroku and thus on AWS. Heroku is continuing to bring up their >> database systems in the wake of the massive AWS outage. Once that is >> back online, git-scm.com will also be back online. >> >> As for the git-fetch issue, we'll look into it once Heroku is back online. >> >> Scott -- To unsubscribe from this list: send the line "unsubscribe 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] fix 'make test' for HP NonStop
> From: Jeff King [mailto:p...@peff.net] > Sent: Monday, October 29, 2012 8:07 AM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: [PATCH v2] fix 'make test' for HP NonStop > > On Thu, Oct 25, 2012 at 12:57:10PM +0200, Joachim Schmitz wrote: > > > diff --git a/Makefile b/Makefile > > index f69979e..35380dd 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -1381,6 +1381,15 @@ ifeq ($(uname_S),NONSTOP_KERNEL) > > MKDIR_WO_TRAILING_SLASH = YesPlease > > # RFE 10-120912-4693 submitted to HP NonStop development. > > NO_SETITIMER = UnfortunatelyYes > > + > > + # for 'make test' > > + # some test don't work with /bin/diff, some fail with /bin/tar > > + # some need bash, and some need /usr/local/bin in PATH first > > + SHELL_PATH=/usr/local/bin/bash > > + SANE_TOOL_PATH=/usr/local/bin > > I think we can drop these comments, as the reasoning really should just > go in the commit message. OK by me. > > + # as of H06.25/J06.14, we might better use this > > + #SHELL_PATH=/usr/coreutils/bin/bash > > + #SANE_TOOL_PATH=/usr/coreutils/bin:/usr/local/bin > > Is there any reason not to put both into the default SANE_TOOL_PATH? If > /usr/coreutils/bin does not exist on older versions, it will be a > harmless no-op. I guess we arestuck with picking one $SHELL_PATH, > though. And because of that have to modify something anyway... But I don't really mind about an extended SANE_TOOL_PATH > -Peff Bye, Jojo -- 8< -- This fixes the vast majority of test failures on HP NonStop. Some test don't work with /bin/diff, some fail with /bin/tar, so let's put /usr/local/bin in PATH first. Some tests fail with /bin/sh (link to /bin/ksh) so use bash instead Signed-off-by: Joachim Schmitz --- Makefile | 9 + 1 file changed, 9 insertions(+) diff --git a/Makefile b/Makefile index f69979e..35380dd 100644 --- a/Makefile +++ b/Makefile @@ -1381,6 +1381,10 @@ ifeq ($(uname_S),NONSTOP_KERNEL) MKDIR_WO_TRAILING_SLASH = YesPlease # RFE 10-120912-4693 submitted to HP NonStop development. NO_SETITIMER = UnfortunatelyYes + SANE_TOOL_PATH=/usr/coreutils/bin:/usr/local/bin + SHELL_PATH=/usr/local/bin/bash + # as of H06.25/J06.14, we might better use this + #SHELL_PATH=/usr/coreutils/bin/bash endif ifneq (,$(findstring MINGW,$(uname_S))) pathsep = ; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: relative objects/info/alternates doesn't work on remote SMB repo
On Thu, Aug 30, 2012 at 3:34 PM, Orgad and Raizel Shaneh wrote: > > On Thu, Aug 30, 2012 at 4:22 PM, Nguyen Thai Ngoc Duy > wrote: > > On Thu, Aug 30, 2012 at 8:12 PM, Orgad and Raizel Shaneh > > wrote: > >>> Could be path normalization. What does "git rev-parse --git-dir" say? > >>> Try to run it at top working directory and a subdirectory as well. > >>> > >>> If you set GIT_OBJECT_DIRECTORY environment variable to > >>> //server/share/foo/repo/.git/objects, does it work? > >> > >> git rev-parse --git-dir in a subdirectory has //server > > > > Hmm where is your git repository? That does not look like a git > > repository's path. > > > > Let me try to explain again. > I have /d/share/bare, which is a bare repository, and /d/share/repo > which is a clone with a relative path to bare/.git/objects in its > .git/objects/info/alternates > > D:\share is configured as a SMB shared folder. It is accessed using > //server/share. > I do not clone from this directory, but work directly in it using 'cd > //server/share', then performing git operations. > > >> setting GIT_OBJECT_DIRECTORY prints "fatal: bad object HEAD" on git > >> status. > > > > I guessed you put your repo in .../repo/.git, but I was probably > > wrong. Try setting again, pointing GIT_OBJECT_DIRECTORY to the > > "objects" directory inside your repository. I just want to make see if > > it's because git miscalculates this path. If setting the env variable > > works, then it probably does. > > -- > > Duy > > Same result. fatal: bad object HEAD. Tried even using a full (local) > path to the objects dir. > > - Orgad Any news? This still doesn't work with 1.8.0. - Orgad -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Password parsing fix on windows
patch.diff Description: Binary data
[PATCH] update-index/diff-index: use core.preloadindex to improve performance
'update-index --refresh' and 'diff-index' (without --cached) don't honor
the core.preloadindex setting yet. Porcelain commands using these (such as
git [svn] rebase) suffer from this, especially on Windows.
Use read_cache_preload to improve performance.
Additionally, in builtin/diff.c, don't preload index status if we don't
access the working copy (--cached).
Results with msysgit on WebKit repo (2GB in 200k files):
| update-index | diff-index | rebase
+--++-
msysgit-v1.8.0 | 9.157s |10.536s | 42.791s
+ preloadindex | 9.157s |10.536s | 28.725s
+ this patch| 2.329s | 2.752s | 15.152s
+ fscache [1] | 0.731s | 1.171s | 8.877s
[1] https://github.com/kblees/git/tree/kb/fscache-v3
Thanks-to: Albert Krawczyk
Signed-off-by: Karsten Blees
---
Can also be pulled from:
https://github.com/kblees/git/tree/kb/update-diff-index-preload-upstream
I thought I might send this upstream directly, as its not msysgit related.
More performance figures (for msysgit) can be found in this discussion:
https://github.com/pro-logic/git/commit/32c03dd8
Ciao,
Karsten
builtin/diff-index.c | 8 ++--
builtin/diff.c | 12
builtin/update-index.c | 1 +
3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 2eb32bd..1c737f7 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -41,9 +41,13 @@ int cmd_diff_index(int argc, const char **argv, const
char *prefix)
if (rev.pending.nr != 1 ||
rev.max_count != -1 || rev.min_age != -1 || rev.max_age != -1)
usage(diff_cache_usage);
- if (!cached)
+ if (!cached) {
setup_work_tree();
- if (read_cache() < 0) {
+ if (read_cache_preload(rev.diffopt.pathspec.raw) < 0) {
+ perror("read_cache_preload");
+ return -1;
+ }
+ } else if (read_cache() < 0) {
perror("read_cache");
return -1;
}
diff --git a/builtin/diff.c b/builtin/diff.c
index 9650be2..198b921 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -130,8 +130,6 @@ static int builtin_diff_index(struct rev_info *revs,
usage(builtin_diff_usage);
argv++; argc--;
}
- if (!cached)
- setup_work_tree();
/*
* Make sure there is one revision (i.e. pending object),
* and there is no revision filtering parameters.
@@ -140,8 +138,14 @@ static int builtin_diff_index(struct rev_info *revs,
revs->max_count != -1 || revs->min_age != -1 ||
revs->max_age != -1)
usage(builtin_diff_usage);
- if (read_cache_preload(revs->diffopt.pathspec.raw) < 0) {
- perror("read_cache_preload");
+ if (!cached) {
+ setup_work_tree();
+ if (read_cache_preload(revs->diffopt.pathspec.raw) < 0) {
+ perror("read_cache_preload");
+ return -1;
+ }
+ } else if (read_cache() < 0) {
+ perror("read_cache");
return -1;
}
return run_diff_index(revs, cached);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 74986bf..ada1dff 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -593,6 +593,7 @@ struct refresh_params {
static int refresh(struct refresh_params *o, unsigned int flag)
{
setup_work_tree();
+ read_cache_preload(NULL);
*o->has_errors |= refresh_cache(o->flags | flag);
return 0;
}
--
1.8.0.msysgit.0.3.g7d9d98c
--
To unsubscribe from this list: send the line "unsubscribe 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] update-index/diff-index: use core.preloadindex to improve performance
On Tue, Oct 30, 2012 at 10:50 AM, wrote: > 'update-index --refresh' and 'diff-index' (without --cached) don't honor > the core.preloadindex setting yet. Porcelain commands using these (such as > git [svn] rebase) suffer from this, especially on Windows. > > Use read_cache_preload to improve performance. > > Additionally, in builtin/diff.c, don't preload index status if we don't > access the working copy (--cached). > > Results with msysgit on WebKit repo (2GB in 200k files): > > | update-index | diff-index | rebase > +--++- > msysgit-v1.8.0 | 9.157s |10.536s | 42.791s > + preloadindex | 9.157s |10.536s | 28.725s > + this patch| 2.329s | 2.752s | 15.152s > + fscache [1] | 0.731s | 1.171s | 8.877s > Wow, awesome results :) This also makes me want to play around with the fscache stuff a bit; about an order of magnitude improvement is quite noticeable :) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 00/14] New remote-hg helper
Hi. I routinely work with projects in both hg and git, so I'm really interested in this. Thanks for working on it! I grabbed the latest version from https://github.com/felipec/git/blob/fc-remote-hg/contrib/remote-hg/git-remote-hg and have been trying it out. For the most part, it seems to work very nicely for the hg repos I have access to and can test against. I've spotted a couple of issues along the way that I thought would be worth reporting. The first is really a symptom of a general difference between hg and git: an hg repository can have multiple heads, whereas a git repo has exactly one head. To demonstrate: $ hg init hgtest && cd hgtest $ echo zero >foo && hg add foo && hg commit -m zero $ echo one >foo && hg commit -m one $ hg checkout -r 0 1 files updated, 0 files merged, 0 files removed, 0 files unresolved $ echo two >foo && hg commit -m two created new head $ hg log --graph @ changeset: 2:ca09651009cb | tag: tip | parent: 0:9f552c53d116 | user:Chris Webb | date:Tue Oct 30 09:33:38 2012 + | summary: two | | o changeset: 1:58fad8998339 |/ user:Chris Webb |date:Tue Oct 30 09:33:25 2012 + |summary: one | o changeset: 0:9f552c53d116 user:Chris Webb date:Tue Oct 30 09:33:08 2012 + summary: zero $ cd .. Now if I try to convert this: $ git clone hg::$PWD/hgtest gittest Cloning into 'gittest'... WARNING: Branch 'default' has more than one head, consider merging Traceback (most recent call last): File "/home/chris/bin/git-remote-hg", line 773, in sys.exit(main(sys.argv)) File "/home/chris/bin/git-remote-hg", line 759, in main do_list(parser) File "/home/chris/bin/git-remote-hg", line 463, in do_list list_branch_head(repo, cur) File "/home/chris/bin/git-remote-hg", line 425, in list_branch_head tip = get_branch_tip(repo, cur) File "/home/chris/bin/git-remote-hg", line 418, in get_branch_tip return repo.branchtip(branch) AttributeError: 'mqrepo' object has no attribute 'branchtip' Strip the second head and it's fine: $ hg -R hgtest strip 2 1 files updated, 0 files merged, 0 files removed, 0 files unresolved saved backup bundle to /tmp/hgtest/hgtest/.hg/strip-backup/ca09651009cb-backup.hg $ git clone hg::$PWD/hgtest gittest Cloning into 'gittest'... $ Not sure what the most friendly thing to do here is. Perhaps refuse to clone/pull from a repo with multiple heads unless you name the specific head you want? The second thing I spotted is the behaviour of bookmarks on push: $ hg init hgtest && cd hgtest $ echo zero >foo && hg add foo && hg commit -m zero $ hg bookmark development $ cd .. $ git clone hg::$PWD/hgtest gittest && cd gittest Cloning into 'gittest'... $ git checkout development Branch development set up to track remote branch development from origin. Switched to a new branch 'development' $ echo one >foo && git add foo && git commit -m one [development 9f67dc4] one 1 file changed, 1 insertion(+), 1 deletion(-) $ git status # On branch development # Your branch is ahead of 'origin/development' by 1 commit. # nothing to commit $ git push warning: helper reported unexpected status of refs/hg/origin/bookmarks/development To hg::/tmp/hgtest/hgtest * [new branch] branches/default -> branches/default * [new branch] development -> development $ hg log -R ../hgtest changeset: 1:1c0714d93864 tag: tip user:Chris Webb date:Tue Oct 30 09:51:51 2012 + summary: one changeset: 0:f56c463398ea bookmark:development user:Chris Webb date:Tue Oct 30 09:50:53 2012 + summary: zero i.e. the development bookmark hasn't been updated by the push. This might be connected to the warning message warning: helper reported unexpected status of refs/hg/origin/bookmarks/development I'm testing with hg 2.2.2 and current git master, so I expect this could be a python api change in the more recent versions of hg if you don't see the same behaviour. Best wishes, Chris. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 00/14] New remote-hg helper
Chris Webb writes: > The first is really a symptom of a general difference between hg and git: an > hg > repository can have multiple heads, whereas a git repo has exactly one head. By this I mean an hg repository without bookmarks or branches can still have multiple heads, whereas a git branch points at exactly one place. Sorry, very vague description there! Cheers, Chris. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git-p4 clone @all error
Hi, (I am a French student, sorry for my English.) So, i want import my perforce projet on my server git. perforce my project tree : depot dev_data mainline release_1.0 release_1.0.0 my command is : git-p4 clone -v --detect-branches //depot@all /home/user/projets/deport The problem : Importing revision 7727 (100%)Traceback (most recent call last): File "/usr/bin/git-p4", line 3183, in main() File "/usr/bin/git-p4", line 3177, in main if not cmd.run(args): File "/usr/bin/git-p4", line 3048, in run if not P4Sync.run(self, depotPaths): File "/usr/bin/git-p4", line 2911, in run self.importChanges(changes) File "/usr/bin/git-p4", line 2618, in importChanges self.initialParent) File "/usr/bin/git-p4", line 2198, in commit epoch = details["time"] KeyError: 'time' if i make a p4 sync //depot/...#head on my perforce server i've this error : Librarian checkout depot/mainline/xxx/api429decryption.txt failed. open for read: depot/mainline/xx/api429decryption.txt,v: Le fichier spcifi est introuvable. My p4 clone can't checking out files after importing revision.. I hope I was clear ... Thanks for your help. -- View this message in context: http://git.661346.n2.nabble.com/git-p4-clone-all-error-tp7570219.html Sent from the git mailing list archive at Nabble.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: Links broken in ref docs.
Am 30.10.2012 09:07, schrieb Mike Norman: Not seen any recently. I'm guessing the dev is in the path of hurricane Sandy? (Not sarcasm, btw.) Do you still see failures? I checked out the website just now and it seemed to work flawlessly (at least the links I tried, could not find any Sharing or Updating section). New design since I last visited, more end-user polish. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Why does git-commit --template want the template to be modified ?
Hi, I'm using git-commit with the --template option. The template I'm given is self sufficient for my purpose but as stated in the documentation, git-commit wants the template to be edited otherwise it aborts the operation. Is it possible to change this ? Thanks -- Francis -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Why does git-commit --template want the template to be modified ?
On Tue, 30 Oct 2012 11:53:08 +0100, Francis Moreau wrote: > Hi, > > I'm using git-commit with the --template option. The template I'm > given is self sufficient for my purpose but as stated in the > documentation, git-commit wants the template to be edited otherwise it > aborts the operation. > > Is it possible to change this ? It seems you want -F instead of --template. -- To unsubscribe from this list: send the line "unsubscribe 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] fix 'make test' for HP NonStop
On Tue, Oct 30, 2012 at 10:21:40AM +0100, Joachim Schmitz wrote: > This fixes the vast majority of test failures on HP NonStop. > Some test don't work with /bin/diff, some fail with /bin/tar, > so let's put /usr/local/bin in PATH first. > Some tests fail with /bin/sh (link to /bin/ksh) so use bash instead > > Signed-off-by: Joachim Schmitz > --- > > Makefile | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/Makefile b/Makefile > index f69979e..35380dd 100644 > --- a/Makefile > +++ b/Makefile > @@ -1381,6 +1381,10 @@ ifeq ($(uname_S),NONSTOP_KERNEL) > MKDIR_WO_TRAILING_SLASH = YesPlease > # RFE 10-120912-4693 submitted to HP NonStop development. > NO_SETITIMER = UnfortunatelyYes > + SANE_TOOL_PATH=/usr/coreutils/bin:/usr/local/bin > + SHELL_PATH=/usr/local/bin/bash > + # as of H06.25/J06.14, we might better use this > + #SHELL_PATH=/usr/coreutils/bin/bash > endif > ifneq (,$(findstring MINGW,$(uname_S))) > pathsep = ; Your patch was whitespace damaged, but I was able to fix it up. Thanks. -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: Why does git-commit --template want the template to be modified ?
Am 10/30/2012 11:53, schrieb Francis Moreau: > I'm using git-commit with the --template option. The template I'm > given is self sufficient for my purpose but as stated in the > documentation, git-commit wants the template to be edited otherwise it > aborts the operation. > > Is it possible to change this ? Perhaps you wanted to use --file instead of --template? -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: crash on git diff-tree -Ganything for new files with textconv filter
On Mon, Oct 29, 2012 at 06:47:05PM -0400, Jeff King wrote: > On Mon, Oct 29, 2012 at 06:35:21PM -0400, Jeff King wrote: > > > The patch below fixes it, but it's terribly inefficient (it just detects > > the situation and reallocates). It would be much better to disable the > > reuse_worktree_file mmap when we populate the filespec, but it is too > > late to pass an option; we may have already populated from an earlier > > diffcore stage. > > > > I guess if we teach the whole diff code that "-G" (and --pickaxe-regex) > > is brittle, we can disable the optimization from the beginning based on > > the diff options. I'll take a look. > > Hmm. That is problematic for two reasons. > > 1. The whole diff call chain will have to be modified to pass the > options around, so they can make it down to the > diff_populate_filespec level. Alternatively, we could do some kind > of global hack, which is ugly but would work OK in practice. > > 2. Reusing a working tree file is only half of the reason a filespec > might be mmap'd. It might also be because we are literally diffing > the working tree. "-G" was meant to be used to limit log traversal, > but it also works to reduce the diff output for something like "git > diff HEAD^". > > I really wish there were an alternate regexec interface we could use > that took a pointer/size pair. Bleh. Thinking on it more, my patch, hacky thought it seems, may not be the worst solution. Here are the options that I see: 1. Use a regex library that does not require NUL termination. If we are bound by the regular regexec interface, this is not feasible. But the GNU implementation works on arbitrary-length buffers (you just have to use a slightly different interface), and we already carry it in compat. It would mean platforms which provide a working but non-GNU regexec would have to start defining NO_REGEX. 2. Figure out a way to get one extra zero byte via mmap. If the requested size does not fall on a page boundary, you get extra zero-ed bytes. Unfortunately, requesting an extra byte does not do what we want; you get SIGBUS accessing it. 3. Copy mmap'd data at point-of-use into a NUL-terminated buffer. That way we only incur the cost when we need it. 4. Avoid mmap-ing in the first place when we are using -G or --pickaxe-regex (e.g., by doing a big read()). At first glance, this sounds more efficient than loading the data one way and then making another copy. But mmap+memcpy, aside from the momentary doubled memory requirement, is probably just as fast or faster than calling read() repeatedly. I am really tempted by (1). Given that (2) does not work, unless somebody comes up with something clever there, that would make (3) the next best choice. -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: crash on git diff-tree -Ganything for new files with textconv filter
(1) sounds attractive for more than one reason. In addition to avoidance of this issue, it would bring bug-to-bug compatibility across platforms. (4), if we can run grep on streaming data (tweak interface we have for checking out a large blob to the working tree), would let us work on dataset larger than fit in core. Even though it would be much more work, it might turn out to be a better option in the longer run. Jeff King wrote: >On Mon, Oct 29, 2012 at 06:47:05PM -0400, Jeff King wrote: > >> On Mon, Oct 29, 2012 at 06:35:21PM -0400, Jeff King wrote: >> >> > The patch below fixes it, but it's terribly inefficient (it just >detects >> > the situation and reallocates). It would be much better to disable >the >> > reuse_worktree_file mmap when we populate the filespec, but it is >too >> > late to pass an option; we may have already populated from an >earlier >> > diffcore stage. >> > >> > I guess if we teach the whole diff code that "-G" (and >--pickaxe-regex) >> > is brittle, we can disable the optimization from the beginning >based on >> > the diff options. I'll take a look. >> >> Hmm. That is problematic for two reasons. >> >> 1. The whole diff call chain will have to be modified to pass the >> options around, so they can make it down to the >> diff_populate_filespec level. Alternatively, we could do some >kind >> of global hack, which is ugly but would work OK in practice. >> >> 2. Reusing a working tree file is only half of the reason a >filespec >> might be mmap'd. It might also be because we are literally >diffing >> the working tree. "-G" was meant to be used to limit log >traversal, >> but it also works to reduce the diff output for something like >"git >> diff HEAD^". >> >> I really wish there were an alternate regexec interface we could use >> that took a pointer/size pair. Bleh. > >Thinking on it more, my patch, hacky thought it seems, may not be the >worst solution. Here are the options that I see: > > 1. Use a regex library that does not require NUL termination. If we > are bound by the regular regexec interface, this is not feasible. > But the GNU implementation works on arbitrary-length buffers (you > just have to use a slightly different interface), and we already >carry it in compat. It would mean platforms which provide a working > but non-GNU regexec would have to start defining NO_REGEX. > > 2. Figure out a way to get one extra zero byte via mmap. If the > requested size does not fall on a page boundary, you get extra > zero-ed bytes. Unfortunately, requesting an extra byte does not > do what we want; you get SIGBUS accessing it. > > 3. Copy mmap'd data at point-of-use into a NUL-terminated buffer. That > way we only incur the cost when we need it. > > 4. Avoid mmap-ing in the first place when we are using -G or > --pickaxe-regex (e.g., by doing a big read()). At first glance, > this sounds more efficient than loading the data one way and then > making another copy. But mmap+memcpy, aside from the momentary >doubled memory requirement, is probably just as fast or faster than > calling read() repeatedly. > >I am really tempted by (1). > >Given that (2) does not work, unless somebody comes up with something >clever there, that would make (3) the next best choice. > >-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: crash on git diff-tree -Ganything for new files with textconv filter
On Tue, Oct 30, 2012 at 09:46:01PM +0900, Junio C Hamano wrote: > (1) sounds attractive for more than one reason. In addition to > avoidance of this issue, it would bring bug-to-bug compatibility > across platforms. Yeah. I mentioned breaking the build for people who would now need to turn on NO_REGEX. But the only reason to do that is to let people on glibc systems use the system version of the tools. A much saner approach would be to just always build with our compat regex, and turn NO_REGEX into a no-op. We already do the same thing for kwset. > (4), if we can run grep on streaming data (tweak interface we have for > checking out a large blob to the working tree), would let us work on > dataset larger than fit in core. Even though it would be much more > work, it might turn out to be a better option in the longer run. Agreed, that would be nice. It's potentially a lot of work, but we could probably get by with a special streaming version of diff_populate_filespec. The tricky thing is that we have to run the regex matcher progressively as we stream data in (since your match might fall in the middle of a read boundary). Which is certainly going to require switching off of regular regexec. I don't think glibc regex will handle it either, though. It looks like pcre can report a partial match at the end of the string, and you can either continue with the next chunk (if using pcre_dfa) or append and re-start the pattern match (for regular pcre_exec). Which means we'd probably have to make streaming matches an optional feature, and still do (1) first to fix the correctness issue. -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: What's cooking in git.git (Oct 2012, #01; Tue, 2)
Sorry for reacting so late, I didn't read the list carefully in the last weeks and my gmail filter somehow didn't trigger on that. On Tuesday 02 October 2012 16:20:22 Junio C Hamano wrote: > * fa/remote-svn (2012-09-19) 16 commits > - Add a test script for remote-svn > - remote-svn: add marks-file regeneration > - Add a svnrdump-simulator replaying a dump file for testing > - remote-svn: add incremental import > - remote-svn: Activate import/export-marks for fast-import > - Create a note for every imported commit containing svn metadata > - vcs-svn: add fast_export_note to create notes > - Allow reading svn dumps from files via file:// urls > - remote-svn, vcs-svn: Enable fetching to private refs > - When debug==1, start fast-import with "--stats" instead of "--quiet" > - Add documentation for the 'bidi-import' capability of remote-helpers > - Connect fast-import to the remote-helper via pipe, adding 'bidi-import' > capability - Add argv_array_detach and argv_array_free_detached > - Add svndump_init_fd to allow reading dumps from arbitrary FDs > - Add git-remote-testsvn to Makefile > - Implement a remote helper for svn in C > (this branch is used by fa/vcs-svn.) > > A GSoC project. > Waiting for comments from mentors and stakeholders. >From my point of view, this is rather complete. It got eight review cycles on the list. Note that the remote helper can only fetch, pushing is not possible at all. > > > * fa/vcs-svn (2012-09-19) 4 commits > - vcs-svn: remove repo_tree > - vcs-svn/svndump: rewrite handle_node(), begin|end_revision() > - vcs-svn/svndump: restructure node_ctx, rev_ctx handling > - svndump: move struct definitions to .h > (this branch uses fa/remote-svn.) > > A GSoC project. > Waiting for comments from mentors and stakeholders. This is the result of what I did when I wanted to start implementing branch detection. I found that the existing code is not suitable and restructured it. The main goal is to seperate svn revision parsing from git commit creation. Because for creating commits, you need to know on which branch to create the commit. While for finding out which branch is the right one, you need to read the complete svn revision first to see what dirs are changed and how. It is rather invasive and it doesn't make sense without using it later on. So I'm not surprised that you may not like it. Anyways it passes all existing tests (that doesn't mean it's good of course ;)) Florian -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Why does git-commit --template want the template to be modified ?
Hi, On Tue, Oct 30, 2012 at 12:09 PM, Tomas Carnecky wrote: > On Tue, 30 Oct 2012 11:53:08 +0100, Francis Moreau > wrote: >> Hi, >> >> I'm using git-commit with the --template option. The template I'm >> given is self sufficient for my purpose but as stated in the >> documentation, git-commit wants the template to be edited otherwise it >> aborts the operation. >> >> Is it possible to change this ? > > It seems you want -F instead of --template. Yes, but I want git to still parse the log and sanitize it (remove trailing whitespaces, comments ...) I actually found that "-F --edit" is what I needed. Thanks both for your answer. -- Francis -- To unsubscribe from this list: send the line "unsubscribe 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-users] Git clone fails with "bad pack header", how to get remote log
I tried to install git 1.8 on the remote server and get exactly the same problem :(. Kevin On 10/29/12 6:18 PM, Konstantin Khomoutov wrote: On Mon, 29 Oct 2012 09:52:54 -0700 (PDT) Kevin Molcard wrote: I have a problem with my build system. I have a remote server with a relatively large repository (around 12 GB, each branch having a size of 3 GB). I have also 2 build servers (Mac, Windows) that are cloning the repo from the remote. Sometimes (very often when several git clone are sent at the same time), I have the following error: remote: internal server error fatal: protocol error: bad pack header I know that it happens when the remote is compressing objects (thanks to `--progress -v` flags) because the last line of the log before the erro is: remote: Compressing objects: 93% (17959/19284) [K * So I have 2 questions, does anybody what is the problem and what should I do? * Is there a way to get a more precise log from the remote to debug this problem? This reminds me of a bug fixed in 1.7.12.1 [1]: * When "git push" triggered the automatic gc on the receiving end, a message from "git prune" that said it was removing cruft leaked to the standard output, breaking the communication protocol. In any case, bugs should be reported to the main Git list (which is git at vger.kernel.org), not here. I'm Cc'ing the main Git list so you'll get any responses from there, if any. Kevin, please answer to this message (keeping all the Ccs -- use "Reply to group" or "Reply to all" in your MUA) and describe exactly what Git versions on which platforms your have. 1. https://raw.github.com/git/git/master/Documentation/RelNotes/1.7.12.1.txt -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 00/14] New remote-hg helper
On Tue, Oct 30, 2012 at 11:25 AM, Chris Webb wrote: > Hi. I routinely work with projects in both hg and git, so I'm really > interested in this. Thanks for working on it! I grabbed the latest version > from > > > https://github.com/felipec/git/blob/fc-remote-hg/contrib/remote-hg/git-remote-hg > > and have been trying it out. For the most part, it seems to work very nicely > for the hg repos I have access to and can test against. I've spotted a couple > of issues along the way that I thought would be worth reporting. Great! > The first is really a symptom of a general difference between hg and git: an > hg > repository can have multiple heads, whereas a git repo has exactly one head. > To > demonstrate: > Now if I try to convert this: > > $ git clone hg::$PWD/hgtest gittest > Cloning into 'gittest'... > WARNING: Branch 'default' has more than one head, consider merging > Traceback (most recent call last): > File "/home/chris/bin/git-remote-hg", line 773, in > sys.exit(main(sys.argv)) > File "/home/chris/bin/git-remote-hg", line 759, in main > do_list(parser) > File "/home/chris/bin/git-remote-hg", line 463, in do_list > list_branch_head(repo, cur) > File "/home/chris/bin/git-remote-hg", line 425, in list_branch_head > tip = get_branch_tip(repo, cur) > File "/home/chris/bin/git-remote-hg", line 418, in get_branch_tip > return repo.branchtip(branch) > AttributeError: 'mqrepo' object has no attribute 'branchtip' Yes, it seems this is an API issue; repo.branchtip doesn't exist in python 2.2. I've added a check for that, and it should work fine now. We'll be picking a random head (the first one), but the user has been warned anyway. > The second thing I spotted is the behaviour of bookmarks on push: > i.e. the development bookmark hasn't been updated by the push. This might be > connected to the warning message This is not an API issue, this is a bug; bookmarks are not updated (only the first creation works). I've fixed this as well, and added a test with the example you put above: http://github.com/felipec/git/commit/d006d54fc84707dffa24d9fad053e574918d Both issues should be fixed now :) Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe 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 v6 0/3] completion: refactor and zsh wrapper
Hi, On Mon, Oct 22, 2012 at 3:45 AM, Felipe Contreras wrote: > Here's a bit of reorganition. I'm introducing a new __gitcompadd helper that > is > useful to wrapp all changes to COMPREPLY, but first, lets get rid of > unnecessary assignments as SZEDER suggested. > > The zsh wrapper is now very very simple. Junio, Jeff, just to let you know, this is an updated version of the new zsh wrapper patch series with the feedback from SZEDER. I see the old version is in pu, and hasn't been updated. Cheers. > Since v5: > > * Get rid of unnecessary COMPREPLY assignments > > Felipe Contreras (3): > completion: get rid of empty COMPREPLY assignments > completion: add new __gitcompadd helper > completion: add new zsh completion -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe 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 push tags
On Mon, Oct 29, 2012 at 4:35 PM, Jeff King wrote:
> On Mon, Oct 29, 2012 at 06:23:30PM +0100, Kacper Kornet wrote:
>
>> > That patch just blocks non-forced updates to refs/tags/. I think a saner
>> > start would be to disallow updating non-commit objects without a force.
>> > We already do so for blobs and trees because they are not (and cannot
>> > be) fast forwards. The fact that annotated tags are checked for
>> > fast-forward seems to me to be a case of "it happens to work that way"
>> > and not anything planned. Since such a push drops the reference to the
>> > old version of the tag, it should probably require a force.
>>
>> I'm not sure. Looking at 37fde87 ("Fix send-pack for non-commitish
>> tags.") I have an impression that Junio allowed for fast-forward pushes
>> of annotated tags on purpose.
>
> Hmm. You're right, though I'm not sure I agree with the reasoning of
> that commit. I'd certainly like to get Junio's input on the subject.
>
>> > Then on top of that we can talk about what lightweight tags should do.
>> > I'm not sure. Following the regular fast-forward rules makes some sense
>> > to me, because you are never losing objects. But there may be
>> > complications with updating tags in general because of fetch's rules,
>> > and we would be better off preventing people from accidentally doing so.
>> > I think a careful review of fetch's tag rules would be in order before
>> > making any decision there.
>>
>> The problem with the current behaviour is, that one can never be 100% sure
>> that his push will not overwrite someone else tag.
>
> Yes, although you do know that you are not throwing away history if you
> do (because it must be a fast forward). Whereas if you have to use "-f"
> to update a tag, then you have turned off all safety checks. So it is an
> improvement for one case (creating a tag), but a regression for another
> (updating an existing tag). I agree that the latter is probably less
> common, but how much? If virtually nobody is doing it because git-fetch
> makes the fetching side too difficult, then the regression is probably
> not a big deal.
>
> -Peff
This is probably a bit premature given there are still open questions,
but I was curious and decided to take a stab at this.
The change is to only allow fast-forward when both the old and new are
commits and the reference is not a lightweight tag. All other
reference updates require --force. I think this resolves the reported
issue and takes into account feedback on this thread. This change
only broke one test and it was an expected failure given the change in
behavior (i.e., I needed to add a "-f" to update a tag in the remote.)
I wasn't sure how to handle provided feedback to the user when there
are multiple refs not pushed for different reasons. But I think this
adds the plumbing for handling it correctly, whateverever that this.
It needs some work, but thought I'd throw it out for feedback to see
if it's at least in the right direction.
Chris
--- 8< ---
diff --git a/builtin/push.c b/builtin/push.c
index db9ba30..fabcea0 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -220,6 +220,10 @@ static const char message_advice_checkout_pull_push[] =
"(e.g. 'git pull') before pushing again.\n"
"See the 'Note about fast-forwards' in 'git push --help' for
details.");
+static const char message_advice_ref_already_exists[] =
+ N_("Updates were rejected because a matching reference already exists
in\n"
+ "the remote. Use git push -f if you really want to make this
update.");
+
static void advise_pull_before_push(void)
{
if (!advice_push_non_ff_current || !advice_push_nonfastforward)
@@ -241,6 +245,11 @@ static void advise_checkout_pull_push(void)
advise(_(message_advice_checkout_pull_push));
}
+static void advise_ref_already_exists(void)
+{
+ advise(_(message_advice_ref_already_exists));
+}
+
static int push_with_options(struct transport *transport, int flags)
{
int err;
@@ -277,6 +286,9 @@ static int push_with_options(struct transport
*transport, int flags)
else
advise_checkout_pull_push();
break;
+ case ALREADY_EXISTS:
+ advise_ref_already_exists();
+ break;
}
return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 7d05064..f159ec3 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -202,6 +202,11 @@ static void print_helper_status(struct ref *ref)
msg = "non-fast forward";
break;
+ case REF_STATUS_REJECT_ALREADY_EXISTS:
+ res = "error";
+ msg = "already exists";
+ break;
+
case REF_STATUS_REJECT_NODELETE:
case REF_STATUS_REMOTE_REJECT:
res = "error";
@@ -288,6 +293,7 @@ int send_pack(struct send_pack_args *args,
/* Ch
Re: [PATCH v4 00/13] New remote-hg helper
On Mon, Oct 29, 2012 at 11:06 PM, Jeff King wrote: > On Mon, Oct 29, 2012 at 11:02:31PM +0100, Felipe Contreras wrote: > >> > If remote-hg is going to live in contrib, it probably makes sense to >> > have its tests live there, too, like subtree. >> >> Probably, I'll check that option. >> >> But eventually I think it should be installed by default, unless >> somebody can come up for a reason not to. For now contrib might be OK. > > I would one day like to have it as part of the main distribution, too, > but it would be nice to prove its worth in the field for a while first. > I especially would like to find out how it compares in practice with the > work that is in msysgit. Yeah, I would like to compare it with that work, if only the patches were readily available somewhere. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/13] New remote-hg helper
Hi all, On Mon, 29 Oct 2012, Jeff King wrote: > On Mon, Oct 29, 2012 at 10:47:04PM +0100, Felipe Contreras wrote: > > > >> Yeah, the test script is not ready for merging, it needs to check > > >> for python, hg, and hg-git. > > >> > > >> Do you have hg-git installed? > > > > > > No. But it's important that it fail gracefully; I can't even take it > > > in pu if I can't run the test suite in a sane way. > > > > The contrib part is fine for 'pu'. The tests aren't even meant to > > exercise stuff in 'contrib', right? There might be some exceptions, > > but either way, there's plenty of stuff in 'contrib' without any > > tests. The tests I'm providing are simply a little sugar. > > Yeah, contrib is a bit of a wildcard. Most things do not have tests. Given that the tests of remote-hg as in git://github.com/msysgit/git's 'devel' branch run just fine without additional dependencies (which probably triggered the not-quite-constructive and unnecessarily-flaming "bloated" comment of Felipe), and given that the code in said branch is well-tested and exercised by daily use, and given the fact that my major concern was not understood (and probably not addressed), and also given the fact that Sverre indicated that he could finalize the work as a 20% project, I decided that other projects I have to do unfortunately have a too-high priority to take care of testing and measuring the performance of the patch series that is discussed in this thread. Sorry, Johannes P.S.: I would still recommend to have a detailed look at the 'devel' branch, in particular the commits starting with "fast-export: do not refer to non-existing marks" and ending with "t5801: skip without hg". My understanding is that it was completely ignored after a brief and maybe too-cursory look. In the least, it has a couple of lessons we learnt the hard way, and if git.git is dead set on duplicating the work, making these mistakes again could be avoided by learning from our lessons. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 00/14] New remote-hg helper
Hi Chris, On Tue, 30 Oct 2012, Chris Webb wrote: > I routinely work with projects in both hg and git, so I'm really > interested in this. Thanks for working on it! I grabbed the latest > version from > > > https://github.com/felipec/git/blob/fc-remote-hg/contrib/remote-hg/git-remote-hg > > and have been trying it out. You might also want to try out the 'devel' branch of https://github.com/msysgit/git. It is in production use here. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 00/14] New remote-hg helper
Felipe Contreras writes: > Yes, it seems this is an API issue; repo.branchtip doesn't exist in > python 2.2. Hi. Presumably this is a problem with old mercurial not a problem with old python as mentioned in the commit? > Both issues should be fixed now :) They are indeed, and it now works nicely on all the repos I've tested it with, including http://selenic.com/hg: very impressive! I wonder whether it's worth ignoring heads with bookmarks pointing to them when it comes to considering heads of branches, or at least allowing the hg branch tracking to be easily disabled? A common idiom when working with hg bookmarks is to completely ignore the (not very useful) hg branches (i.e. all commits are on the default hg branch) and have a bookmark for each line of development used exactly as a git branch would be. On such a repository, at the moment you will always get a warning about multiple heads on branches/default, even though you actually don't care about branches/default (and would prefer it not to exist) and just want the branches coming from the bookmarks. I've also seen repositories with no hg branches, but with a single unbookmarked tip and bookmarks on some other heads to mark non-mainline development. It would be nice for branches/default to track the unbookmarked tip in this case, without warning about the other, bookmarked heads. Finally, on a simple repo with no branches and where there's no clash with a bookmark called master, I'd love to be able to a get a more idiomatic origin/master rather than origin/branches/default. Just some idle thoughts... Best wishes, Chris. -- To unsubscribe from this list: send the line "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] Enable parallelism in git submodule update.
The --jobs parameter may be used to set the degree of per-submodule
parallel execution.
Signed-off-by: Stefan Zager
---
Documentation/git-submodule.txt |8 ++-
git-submodule.sh| 40 ++-
2 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index b4683bb..cb23ba7 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -14,7 +14,8 @@ SYNOPSIS
'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
'git submodule' [--quiet] init [--] [...]
'git submodule' [--quiet] update [--init] [-N|--no-fetch] [--rebase]
- [--reference ] [--merge] [--recursive] [--]
[...]
+ [--reference ] [--merge] [--recursive]
+ [-j|--jobs [jobs]] [--] [...]
'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
[commit] [--] [...]
'git submodule' [--quiet] foreach [--recursive]
@@ -146,6 +147,11 @@ If the submodule is not yet initialized, and you just want
to use the
setting as stored in .gitmodules, you can automatically initialize the
submodule with the `--init` option.
+
+By default, each submodule is treated serially. You may specify a degree of
+parallel execution with the --jobs flag. If a parameter is provided, it is
+the maximum number of jobs to run in parallel; without a parameter, all jobs
are
+run in parallel.
++
If `--recursive` is specified, this command will recurse into the
registered submodules, and update any nested submodules within.
+
diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b110..60a5f96 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -8,7 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
USAGE="[--quiet] add [-b branch] [-f|--force] [--reference ] [--]
[]
or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]
or: $dashless [--quiet] init [--] [...]
- or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force]
[--rebase] [--reference ] [--merge] [--recursive] [--] [...]
+ or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force]
[--rebase] [--reference ] [--merge] [--recursive] [-j|--jobs
[jobs]] [--] [...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ]
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive]
or: $dashless [--quiet] sync [--] [...]"
@@ -500,6 +500,7 @@ cmd_update()
{
# parse $args after "submodule ... update".
orig_flags=
+ jobs="1"
while test $# -ne 0
do
case "$1" in
@@ -518,6 +519,20 @@ cmd_update()
-r|--rebase)
update="rebase"
;;
+ -j|--jobs)
+ case "$2" in
+ ''|-*)
+ jobs="0"
+ ;;
+ *)
+ jobs="$2"
+ shift
+ ;;
+ esac
+ # Don't preserve this arg.
+ shift
+ continue
+ ;;
--reference)
case "$2" in '') usage ;; esac
reference="--reference=$2"
@@ -551,11 +566,34 @@ cmd_update()
shift
done
+ # Correctly handle the case where '-q' came before 'update' on the
command line.
+ if test -n "$GIT_QUIET"
+ then
+ orig_flags="$orig_flags -q"
+ fi
+
if test -n "$init"
then
cmd_init "--" "$@" || return
fi
+ if test "$jobs" != 1
+ then
+ if ( echo test | xargs -P "$jobs" true 2>/dev/null )
+ then
+ if ( echo test | xargs --max-lines=1 true 2>/dev/null
); then
+ max_lines="--max-lines=1"
+ else
+ max_lines="-L 1"
+ fi
+ module_list "$@" | awk '{print $4}' |
+ xargs $max_lines -P "$jobs" git submodule update
$orig_flags
+ return
+ else
+ echo "Warn: parallel execution is not supported on this
platform."
+ fi
+ fi
+
cloned_modules=
module_list "$@" | {
err=
--
1.7.7.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Enable parallelism in git submodule update.
The --jobs parameter may be used to set the degree of per-submodule
parallel execution.
Signed-off-by: Stefan Zager
---
Documentation/git-submodule.txt |8 ++-
git-submodule.sh| 40 ++-
2 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index b4683bb..cb23ba7 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -14,7 +14,8 @@ SYNOPSIS
'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
'git submodule' [--quiet] init [--] [...]
'git submodule' [--quiet] update [--init] [-N|--no-fetch] [--rebase]
- [--reference ] [--merge] [--recursive] [--]
[...]
+ [--reference ] [--merge] [--recursive]
+ [-j|--jobs [jobs]] [--] [...]
'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
[commit] [--] [...]
'git submodule' [--quiet] foreach [--recursive]
@@ -146,6 +147,11 @@ If the submodule is not yet initialized, and you just want
to use the
setting as stored in .gitmodules, you can automatically initialize the
submodule with the `--init` option.
+
+By default, each submodule is treated serially. You may specify a degree of
+parallel execution with the --jobs flag. If a parameter is provided, it is
+the maximum number of jobs to run in parallel; without a parameter, all jobs
are
+run in parallel.
++
If `--recursive` is specified, this command will recurse into the
registered submodules, and update any nested submodules within.
+
diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b110..60a5f96 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -8,7 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
USAGE="[--quiet] add [-b branch] [-f|--force] [--reference ] [--]
[]
or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]
or: $dashless [--quiet] init [--] [...]
- or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force]
[--rebase] [--reference ] [--merge] [--recursive] [--] [...]
+ or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force]
[--rebase] [--reference ] [--merge] [--recursive] [-j|--jobs
[jobs]] [--] [...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ]
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive]
or: $dashless [--quiet] sync [--] [...]"
@@ -500,6 +500,7 @@ cmd_update()
{
# parse $args after "submodule ... update".
orig_flags=
+ jobs="1"
while test $# -ne 0
do
case "$1" in
@@ -518,6 +519,20 @@ cmd_update()
-r|--rebase)
update="rebase"
;;
+ -j|--jobs)
+ case "$2" in
+ ''|-*)
+ jobs="0"
+ ;;
+ *)
+ jobs="$2"
+ shift
+ ;;
+ esac
+ # Don't preserve this arg.
+ shift
+ continue
+ ;;
--reference)
case "$2" in '') usage ;; esac
reference="--reference=$2"
@@ -551,11 +566,34 @@ cmd_update()
shift
done
+ # Correctly handle the case where '-q' came before 'update' on the
command line.
+ if test -n "$GIT_QUIET"
+ then
+ orig_flags="$orig_flags -q"
+ fi
+
if test -n "$init"
then
cmd_init "--" "$@" || return
fi
+ if test "$jobs" != 1
+ then
+ if ( echo test | xargs -P "$jobs" true 2>/dev/null )
+ then
+ if ( echo test | xargs --max-lines=1 true 2>/dev/null
); then
+ max_lines="--max-lines=1"
+ else
+ max_lines="-L 1"
+ fi
+ module_list "$@" | awk '{print $4}' |
+ xargs $max_lines -P "$jobs" git submodule update
$orig_flags
+ return
+ else
+ echo "Warn: parallel execution is not supported on this
platform."
+ fi
+ fi
+
cloned_modules=
module_list "$@" | {
err=
--
1.7.7.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/13] New remote-hg helper
On Tue, Oct 30, 2012 at 6:20 PM, Johannes Schindelin wrote: > P.S.: I would still recommend to have a detailed look at the 'devel' > branch, in particular the commits starting with "fast-export: do not refer > to non-existing marks" and ending with "t5801: skip without hg". My > understanding is that it was completely ignored after a brief and maybe > too-cursory look. In the least, it has a couple of lessons we learnt the > hard way, and if git.git is dead set on duplicating the work, making these > mistakes again could be avoided by learning from our lessons. % g l --grep="t5801: skip without hg" devel 1e000d4 t5801: skip without hg bee410c t5801: skip without hg 5cdc7d0 t5801: skip without hg 05b703f t5801: skip without hg 6bb8d90 t5801: skip without hg c70b4d0 t5801: skip without hg 2f46371 t5801: skip without hg 39bc40f t5801: skip without hg d0a618b t5801: skip without hg % g l --grep="fast-export: do not refer" devel d3ac32c fast-export: do not refer to non-existing marks bdbb22f fast-export: do not refer to non-existing marks 5d99930 fast-export: do not refer to non-existing marks 381f276 fast-export: do not refer to non-existing marks b4686c7 fast-export: do not refer to non-existing marks e3dfe01 fast-export: do not refer to non-existing marks c00fe59 fast-export: do not refer to non-existing marks ce357ce fast-export: do not refer to non-existing marks 5c1c7a4 fast-export: do not refer to non-existing marks 9c827d1 fast-export: do not refer to non-existing marks I'll assume you are referring the latest ones: % g log --oneline --reverse d3ac32c^..1e000d4 * d3ac32c fast-export: do not refer to non-existing marks Not needed at all. * b013fe0 setup_revisions: remember whether a ref was positive or not * fb89a2c fast-export: do not export negative refs * 7655869 setup_revisions: remember whether a ref was positive or not I've fixed this problem already. The solution proposed in these patches is to convoluted: 1) Requires multiple unrelated changes 2) Proposes change in committish semantics It's hard to test, because the test to check for this is not in this patch series, and it's testing for something completely unrelated: --- cat > expected << EOF reset refs/heads/master from $(git rev-parse master) EOF test_expect_success 'refs are updated even if no commits need to be exported' ' git fast-export master..master > actual && test_cmp expected actual ' --- This is most certainly not what we want. Notice that in my patch (a single patch) I added the tests at the same time so it's clear what it's fixing, and I also added a test to the relevant remote-helper behavior we want: https://github.com/felipec/git/commit/76e75315bd1bd8d9d8365bb09261a745a10ceae0 * 512cb13 t5800: test pushing a new branch with old content If this is what the patches above were trying to fix, then yes, my patch fixes that. Also, it's tainted by changes from another patch. * a85de2c t5800: point out that deleting branches does not work Correct, but hardly _necessary_. * 2412a45 transport-helper: add trailing -- No description what's the problem, or what it's trying to fix, or tests, so it's not possible to know if this is _needed_ or not. But probably correct. * 026d07c remote-helper: check helper status after import/export Again, no explanation, but the issue was already addressed: http://article.gmane.org/gmane.comp.version-control.git/208202 The problem is minuscule, not _needed_. * 5165e26 remote-testgit: factor out RemoteHelper class * 049f093 git-remote-testgit: make local a function * f835bb2 git_remote_helpers: add fastimport library * 088ad33 git-remote-hg: add hgimport, an hg-fast-import equivalent * 7de6ca0 git-remote-hg: add GitHg, a helper class for converting hg commits to git * e3cc5ed git-remote-hg: add hgexport, an hg-fast-export equivalent * 0edc8e9 git-remote-hg: add GitExporter/GitImporter/NonLocalGit * 5c73277 remote-hg: adjust to hg 1.9 * 1b47007 git-remote-hg: add the helper * 4dcc671 git-remote-hg: add tests * 48b2769 remote-hg: Postel's law dictates we should handle Author * 2587cc6 remote-hg: another case of Postel's law * 9f934c9 remote-hg: handle another funny author line from http://scelenic.com/hg * a799904 remote-hg: do not interfer with hg's revs() method All these are specific to this remote-hg version. * ac77256 Always auto-gc after calling a fast-import transport This might be a good idea, but not _needed_. * 1e000d4 t5801: skip without hg Specific to this remote-hg. So, yeah, nothing really needed there. Some patches might be nice, but that's it. Now, if this is really the latest and greatest remote-hg patch series, I can try to port them to git's master and see how it fares. But you mentioned something about cooperation, and I've yet to see how is it that you are planning to cooperate. If you say you don't have time to spend on this, I don't see why I should worry about testing this series of patches. Also, you seem to be clearly against my im
Re: [PATCH] Enable parallelism in git submodule update.
This is a refresh of a conversation from a couple of months ago.
I didn't try to implement all the desired features (e.g., smart logic
for passing a -j parameter to recursive submodule invocations), but I
did address the one issue that Junio insisted on: the code makes a
best effort to detect whether xargs supports parallel execution on the
host platform, and if it doesn't, then it prints a warning and falls
back to serial execution.
Stefan
On Tue, Oct 30, 2012 at 11:03 AM, wrote:
> The --jobs parameter may be used to set the degree of per-submodule
> parallel execution.
>
> Signed-off-by: Stefan Zager
> ---
> Documentation/git-submodule.txt |8 ++-
> git-submodule.sh| 40
> ++-
> 2 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index b4683bb..cb23ba7 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -14,7 +14,8 @@ SYNOPSIS
> 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
> 'git submodule' [--quiet] init [--] [...]
> 'git submodule' [--quiet] update [--init] [-N|--no-fetch] [--rebase]
> - [--reference ] [--merge] [--recursive] [--]
> [...]
> + [--reference ] [--merge] [--recursive]
> + [-j|--jobs [jobs]] [--] [...]
> 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit)
> ]
> [commit] [--] [...]
> 'git submodule' [--quiet] foreach [--recursive]
> @@ -146,6 +147,11 @@ If the submodule is not yet initialized, and you just
> want to use the
> setting as stored in .gitmodules, you can automatically initialize the
> submodule with the `--init` option.
> +
> +By default, each submodule is treated serially. You may specify a degree of
> +parallel execution with the --jobs flag. If a parameter is provided, it is
> +the maximum number of jobs to run in parallel; without a parameter, all jobs
> are
> +run in parallel.
> ++
> If `--recursive` is specified, this command will recurse into the
> registered submodules, and update any nested submodules within.
> +
> diff --git a/git-submodule.sh b/git-submodule.sh
> index ab6b110..60a5f96 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -8,7 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
> USAGE="[--quiet] add [-b branch] [-f|--force] [--reference ]
> [--] []
> or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]
> or: $dashless [--quiet] init [--] [...]
> - or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force]
> [--rebase] [--reference ] [--merge] [--recursive] [--] [...]
> + or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force]
> [--rebase] [--reference ] [--merge] [--recursive] [-j|--jobs
> [jobs]] [--] [...]
> or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ]
> [commit] [--] [...]
> or: $dashless [--quiet] foreach [--recursive]
> or: $dashless [--quiet] sync [--] [...]"
> @@ -500,6 +500,7 @@ cmd_update()
> {
> # parse $args after "submodule ... update".
> orig_flags=
> + jobs="1"
> while test $# -ne 0
> do
> case "$1" in
> @@ -518,6 +519,20 @@ cmd_update()
> -r|--rebase)
> update="rebase"
> ;;
> + -j|--jobs)
> + case "$2" in
> + ''|-*)
> + jobs="0"
> + ;;
> + *)
> + jobs="$2"
> + shift
> + ;;
> + esac
> + # Don't preserve this arg.
> + shift
> + continue
> + ;;
> --reference)
> case "$2" in '') usage ;; esac
> reference="--reference=$2"
> @@ -551,11 +566,34 @@ cmd_update()
> shift
> done
>
> + # Correctly handle the case where '-q' came before 'update' on the
> command line.
> + if test -n "$GIT_QUIET"
> + then
> + orig_flags="$orig_flags -q"
> + fi
> +
> if test -n "$init"
> then
> cmd_init "--" "$@" || return
> fi
>
> + if test "$jobs" != 1
> + then
> + if ( echo test | xargs -P "$jobs" true 2>/dev/null )
> + then
> + if ( echo test | xargs --max-lines=1 true 2>/dev/null
> ); then
> + max_lines="--max-lines=1"
> + else
> + max_lines="-L 1"
> + fi
> + module_list "$@" | awk '{print $4}' |
> + xargs $max_lines -P "$jobs" git submodu
Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
On Tue, Oct 30, 2012 at 10:11 AM, Felipe Contreras wrote: > When an object has already been exported (and thus is in the marks) it > is flagged as SHOWN, so it will not be exported again, even if this time > it's exported through a different ref. > > We don't need the object to be exported again, but we want the ref > updated, which doesn't happen. > > Since we can't know if a ref was exported or not, let's just assume that > if the commit was marked (flags & SHOWN), the user still wants the ref > updated. > > So: > > % git branch test master > % git fast-export $mark_flags master > % git fast-export $mark_flags test > > Would export 'test' properly. > > Additionally, this fixes issues with remote helpers; now they can push > refs wich objects have already been exported. Won't this also export child (or maybe parent) branches that weren't mentioned? For example: $ git branch one $ echo foo > content $ git commit -m two $ git fast-export one $ git fast-export two I suspect that one of those will export both one and two. If not, this seems like a great solution to the fast-export problem. -- Cheers, Sverre Rabbelier -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 00/14] New remote-hg helper
Chris Webb writes: > A common idiom when working with hg bookmarks is to completely ignore the > (not very useful) hg branches (i.e. all commits are on the default hg > branch) and have a bookmark for each line of development used exactly as a > git branch would be. > > On such a repository, at the moment you will always get a warning about > multiple heads on branches/default, even though you actually don't care > about branches/default (and would prefer it not to exist) and just want the > branches coming from the bookmarks. Something which you can do with hg clone is hg clone http://my.repo/foo#master to clone just the history behind the master bookmark from foo. This works nicely with git-remote-hg too: git clone hg::http://my.repo/foo#master gives you just origin/master and origin/branches/default, not origin/otherbookmark. This is a case where it would be particularly nice to be able to kill origin/branches/default and just keep the identical origin/master. Cheers, Chris. -- To unsubscribe from this list: send the line "unsubscribe 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 6/8] longest_ancestor_length(): require prefix list entries to be normalized
Michael Haggerty wrote:
> Move the responsibility for normalizing prefixes from
> longest_ancestor_length() to its callers. Use slightly different
> normalizations at the two callers:
>
> In setup_git_directory_gently_1(), use the old normalization, which
> ignores paths that are not usable. In the next commit we will change
> this caller to also resolve symlinks in the paths from
> GIT_CEILING_DIRECTORIES as part of the normalization.
>
> In "test-path-utils longest_ancestor_length", use the old
> normalization, but die() if any paths are unusable. Also change t0060
> to only pass normalized paths to the test program (no empty entries or
> non-absolute paths, strip trailing slashes from the paths, and remove
> tests that thereby become redundant).
>
> The point of this change is to reduce the scope of the ancestor_length
> tests in t0060 from testing normalization+longest_prefix to testing
> only mostly longest_prefix. This is necessary because when
> setup_git_directory_gently_1() starts resolving symlinks as part of
> its normalization, it will not be reasonable to do the same in the
> test suite, because that would make the test results depend on the
> contents of the root directory of the filesystem on which the test is
> run. HOWEVER: under Windows, bash mangles arguments that look like
> absolute POSIX paths into DOS paths.
Just to be clear, this is true for the MinGW port to Windows, but *not*
the cygwin port.
:-P
> So we have to retain the level
> of normalization done by normalize_path_copy() to convert the
> bash-mangled DOS paths (which contain backslashes) into paths that use
> forward slashes.
>
> Signed-off-by: Michael Haggerty
> ---
> path.c| 26 +++---
> setup.c | 23 +++
> t/t0060-path-utils.sh | 41 +
> test-path-utils.c | 45 -
> 4 files changed, 91 insertions(+), 44 deletions(-)
>
[snip]
> diff --git a/test-path-utils.c b/test-path-utils.c
> index acb0560..0092cbf 100644
> --- a/test-path-utils.c
> +++ b/test-path-utils.c
> @@ -1,6 +1,33 @@
> #include "cache.h"
> #include "string-list.h"
>
> +/*
> + * A "string_list_each_func_t" function that normalizes an entry from
> + * GIT_CEILING_DIRECTORIES. If the path is unusable for some reason,
> + * die with an explanation.
> + */
> +static int normalize_ceiling_entry(struct string_list_item *item, void
> *unused)
> +{
> + const char *ceil = item->string;
> + int len = strlen(ceil);
> + char buf[PATH_MAX+1];
> +
> + if (len == 0)
> + die("Empty path is not supported");
> + if (len > PATH_MAX)
> + die("Path \"%s\" is too long", ceil);
> + if (!is_absolute_path(ceil))
> + die("Path \"%s\" is not absolute", ceil);
> + if (normalize_path_copy(buf, ceil) < 0)
> + die("Path \"%s\" could not be normalized", ceil);
> + len = strlen(buf);
> + if (len > 1 && buf[len-1] == '/')
> + die("Normalized path \"%s\" ended with slash", buf);
> + free(item->string);
> + item->string = xstrdup(buf);
> + return 1;
> +}
> +
> int main(int argc, char **argv)
> {
> if (argc == 3 && !strcmp(argv[1], "normalize_path_copy")) {
> @@ -33,10 +60,26 @@ int main(int argc, char **argv)
> if (argc == 4 && !strcmp(argv[1], "longest_ancestor_length")) {
> int len;
> struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
> + char *path = xstrdup(argv[2]);
>
> + /*
> + * We have to normalize the arguments because under
> + * Windows, bash mangles arguments that look like
ditto
> + * absolute POSIX paths or colon-separate lists of
> + * absolute POSIX paths into DOS paths (e.g.,
> + * "/foo:/foo/bar" might be converted to
> + * "D:\Src\msysgit\foo;D:\Src\msysgit\foo\bar"),
> + * whereas longest_ancestor_length() requires paths
> + * that use forward slashes.
> + */
> + if (normalize_path_copy(path, path))
> + die("Path \"%s\" could not be normalized", argv[2]);
> string_list_split(&ceiling_dirs, argv[3], PATH_SEP, -1);
> - len = longest_ancestor_length(argv[2], &ceiling_dirs);
> + filter_string_list(&ceiling_dirs, 0,
> +normalize_ceiling_entry, NULL);
> + len = longest_ancestor_length(path, &ceiling_dirs);
> string_list_clear(&ceiling_dirs, 0);
> + free(path);
> printf("%d\n", len);
> return 0;
> }
HTH
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 v5 00/14] New remote-hg helper
On Tue, Oct 30, 2012 at 7:00 PM, Chris Webb wrote: > Felipe Contreras writes: > >> Yes, it seems this is an API issue; repo.branchtip doesn't exist in >> python 2.2. > > Hi. Presumably this is a problem with old mercurial not a problem with old > python as mentioned in the commit? > >> Both issues should be fixed now :) > > They are indeed, and it now works nicely on all the repos I've tested it > with, including http://selenic.com/hg: very impressive! > > I wonder whether it's worth ignoring heads with bookmarks pointing to them > when it comes to considering heads of branches, or at least allowing the > hg branch tracking to be easily disabled? > > A common idiom when working with hg bookmarks is to completely ignore the > (not very useful) hg branches (i.e. all commits are on the default hg > branch) and have a bookmark for each line of development used exactly as a > git branch would be. > > On such a repository, at the moment you will always get a warning about > multiple heads on branches/default, even though you actually don't care > about branches/default (and would prefer it not to exist) and just want the > branches coming from the bookmarks. > > I've also seen repositories with no hg branches, but with a single > unbookmarked tip and bookmarks on some other heads to mark non-mainline > development. It would be nice for branches/default to track the unbookmarked > tip in this case, without warning about the other, bookmarked heads. > > Finally, on a simple repo with no branches and where there's no clash with a > bookmark called master, I'd love to be able to a get a more idiomatic > origin/master rather than origin/branches/default. Sounds like you might want to try this: % git config --global remote-hg.hg-git-compat true This would match the behavior of hg-git, which seems to be what you are looking for: branches will be ignored. But additionally there will be other obtrusive changes: 1) Extra information will be stored in the commit message: branch, renames, extra info. Personally I don't like this, which is why it's only enabled with the hg-git compat mode. Eventually there will be support for this in the normal mode through git notes, and eventually (hopefully), hg-git will use notes as well. 2) Authors will be fixed trying to preserve as much information as possible The problem with this mode is what happens when there are no bookmarks, and what I decided to do is create a fake bookmark to the current branch (e.g. default), so you would get 'origin/default', but you will be warned if there are multiple heads. We could try to create a 'master' ref to the 'tip', but that might not be what the user wants: (s)he might switch to a certain branch, and clone the repository in git; currently the result working directory would be the same as in hg, but if we point to 'tip', then it would not. Maybe we should have a branch pointing to 'tip' either way. The reason I decided on having 'branches/default' on the normal mode is that users unfamiliar with mercurial might expect to see all the branches somewhere. But perhaps that should happen only for other branches, while 'default' gets a special treatment and gets promoted to 'origin/master' but only if there's no 'master' bookmark, as you say. Now the tricky part would be handling the case where there was no 'master' bookmark, and suddenly one gets created, but it might be overkill to think about that right now. Either way, try the hg-git-compat mode, should be close :) Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe 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] parse_dirstat_params(): use string_list to split comma-separated string
Michael Haggerty alum.mit.edu> writes:
...
> -static int parse_dirstat_params(struct diff_options *options, const char ...
> +static int parse_dirstat_params(struct diff_options *options, const char ...
> struct strbuf *errmsg)
> {
> - const char *p = params;
> - int p_len, ret = 0;
> + char *params_copy = xstrdup(params_string);
> + struct string_list params = STRING_LIST_INIT_NODUP;
> + int ret = 0;
> + int i;
>
> - while (*p) {
> - p_len = strchrnul(p, ',') - p;
> - if (!memcmp(p, "changes", p_len)) {
> + if (*params_copy)
params_copy is set to the value returned by xstrdup, which cannot be NULL.
This check can be removed and if params_string can be NULL, it should be
checked before being passed to xstrdup.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
On Tue, Oct 30, 2012 at 7:12 PM, Sverre Rabbelier wrote:
> On Tue, Oct 30, 2012 at 10:11 AM, Felipe Contreras
> wrote:
>> When an object has already been exported (and thus is in the marks) it
>> is flagged as SHOWN, so it will not be exported again, even if this time
>> it's exported through a different ref.
>>
>> We don't need the object to be exported again, but we want the ref
>> updated, which doesn't happen.
>>
>> Since we can't know if a ref was exported or not, let's just assume that
>> if the commit was marked (flags & SHOWN), the user still wants the ref
>> updated.
>>
>> So:
>>
>> % git branch test master
>> % git fast-export $mark_flags master
>> % git fast-export $mark_flags test
>>
>> Would export 'test' properly.
>>
>> Additionally, this fixes issues with remote helpers; now they can push
>> refs wich objects have already been exported.
>
> Won't this also export child (or maybe parent) branches that weren't
> mentioned? For example:
>
> $ git branch one
> $ echo foo > content
> $ git commit -m two
> $ git fast-export one
> $ git fast-export two
>
> I suspect that one of those will export both one and two. If not, this
> seems like a great solution to the fast-export problem.
Why would it? We are not changing the way objects are exported, the
only difference is what happens at the end
(handle_tags_and_duplicates()).
And if you are talking about the ref for the reset at the end, it has
to be both in the list of refs selected by the user (initially in
&revs.pending), either marked or the object already referenced by
another ref in the list selected by the user (e.g. fast-export one
two, where one^{commit} == two^{commit}, and not marked as
UNINTERESTING (e.g. ^two).
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] fast-export: trivial cleanup
Felipe Contreras wrote:
> Setting commit to commit is a no-op.
Wrong description. This should say:
The code uses the idiom of assigning commit to itself to quench a
"may be used uninitialized" warning. Luckily at least modern
versions of gcc do not produce that warning here, so we can drop
the self-assignment.
This makes the code clearer to human beings, makes static
analyzers that do not know that idiom happier, and means that
if the code some day evolves to use this variable uninitialized
then we will catch it.
> Signed-off-by: Felipe Contreras
With that change, for what it's worth,
Reviewed-by: Jonathan Nieder
Patch left unsnipped since it doesn't seem to have hit the list.
> ---
> builtin/fast-export.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 12220ad..065f324 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -483,7 +483,7 @@ static void get_tags_and_duplicates(struct object_array
> *pending,
> for (i = 0; i < pending->nr; i++) {
> struct object_array_entry *e = pending->objects + i;
> unsigned char sha1[20];
> - struct commit *commit = commit;
> + struct commit *commit;
> char *full_name;
>
> if (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] fast-export: fix comparisson in tests
(actually cc-ing the git list this time. Sorry for the noise, all.) Felipe Contreras wrote: > [Subject: [PATCH v2 2/4] fast-export: fix comparisson in tests] > > First the expected, then the actual, otherwise the diff would be the > opposite of what we want. Spelling: s/comparisson/comparison/. Semantics: this isn't actually fixing anything --- it's a cosmetic thing. It would be clearer to say: fast-export test: swap arguments to test_cmp This way if diff output is produced, it describes how the actual output differs from what was expected rather than the other way around. > Signed-off-by: Felipe Contreras For what it's worth, with amended message, Reviewed-by: Jonathan Nieder Patch left unsnipped because it hadn't hit the list. > --- > t/t9350-fast-export.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh > index 3e821f9..49bdb44 100755 > --- a/t/t9350-fast-export.sh > +++ b/t/t9350-fast-export.sh > @@ -303,7 +303,7 @@ test_expect_success 'dropping tag of filtered out object' > ' > ( > cd limit-by-paths && > git fast-export --tag-of-filtered-object=drop mytag -- there > output && > - test_cmp output expected > + test_cmp expected output > ) > ' > > @@ -320,7 +320,7 @@ test_expect_success 'rewriting tag of filtered out > object' ' > ( > cd limit-by-paths && > git fast-export --tag-of-filtered-object=rewrite mytag -- there > > output && > - test_cmp output expected > + test_cmp expected output > ) > ' > > @@ -351,7 +351,7 @@ test_expect_failure 'no exact-ref revisions included' ' > ( > cd limit-by-paths && > git fast-export master~2..master~1 > output && > - test_cmp output expected > + test_cmp expected output > ) > ' > > -- > 1.8.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote:
> They have been marked as UNINTERESTING for a reason, lets respect that.
This patch looks unsafe, and in the examples listed in the patch
description the changed behavior does not look like an improvement.
Worse, the description lists a few examples but gives no convincing
explanation to reassure about the lack of bad behavior for examples
not listed.
Perhaps this patch has a prerequisite and has come out of order.
Hope that helps,
Jonathan
Patch left unsnipped so we can get a copy in the list archive.
> Currently the first ref is handled properly, but not the rest, so:
>
> % git fast-export master ^master
>
> Would currently throw a reset for master (2nd ref), which is not what we
> want.
>
> % git fast-export master ^foo ^bar ^roo
> % git fast-export master salsa..tacos
>
> Even if all these refs point to the same object; foo, bar, roo, salsa,
> and tacos would all get a reset.
>
> This is most certainly not what we want. After this patch, nothing gets
> exported, because nothing was selected (everything is UNINTERESTING).
>
> Signed-off-by: Felipe Contreras
> ---
> builtin/fast-export.c | 7 ---
> t/t9350-fast-export.sh | 6 ++
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 065f324..7fb6fe1 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -523,10 +523,11 @@ static void get_tags_and_duplicates(struct object_array
> *pending,
> typename(e->item->type));
> continue;
> }
> - if (commit->util)
> + if (commit->util) {
> /* more than one name for the same object */
> - string_list_append(extra_refs, full_name)->util =
> commit;
> - else
> + if (!(commit->object.flags & UNINTERESTING))
> + string_list_append(extra_refs, full_name)->util
> = commit;
> + } else
> commit->util = full_name;
> }
> }
> diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
> index 49bdb44..6ea8f6f 100755
> --- a/t/t9350-fast-export.sh
> +++ b/t/t9350-fast-export.sh
> @@ -440,4 +440,10 @@ test_expect_success 'fast-export quotes pathnames' '
> )
> '
>
> +test_expect_success 'proper extra refs handling' '
> + git fast-export master ^master master..master > actual &&
> + echo -n > expected &&
> + test_cmp expected actual
> +'
> +
> test_done
> --
> 1.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/4] fast-export: general fixes
Hi, Note: sorry for the noise, the first try (v2) was silently eaten by the mailing list handler. First patches are general cleanups and fixes, the last patch fixes a real issue that affects remote helpers. Changes since v2: * Actually send it to the ml Changes since v1: * Improved commit messages * Use /dev/null in tests * Add test for remote helpers Felipe Contreras (4): fast-export: trivial cleanup fast-export: fix comparisson in tests fast-export: don't handle uninteresting refs fast-export: make sure refs are updated properly builtin/fast-export.c | 16 +++- t/t5800-remote-helpers.sh | 11 +++ t/t9350-fast-export.sh| 26 +++--- 3 files changed, 45 insertions(+), 8 deletions(-) -- 1.8.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/4] fast-export: trivial cleanup
Setting commit to commit is a no-op. It might have been there to avoid a
compiler warning, but if so, it was the compiler to blame.
Signed-off-by: Felipe Contreras
---
builtin/fast-export.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 12220ad..065f324 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -483,7 +483,7 @@ static void get_tags_and_duplicates(struct object_array
*pending,
for (i = 0; i < pending->nr; i++) {
struct object_array_entry *e = pending->objects + i;
unsigned char sha1[20];
- struct commit *commit = commit;
+ struct commit *commit;
char *full_name;
if (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)
--
1.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/4] fast-export: fix comparisson in tests
First the expected, then the actual, otherwise the diff would be the opposite of what we want. Signed-off-by: Felipe Contreras --- t/t9350-fast-export.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 3e821f9..49bdb44 100755 --- a/t/t9350-fast-export.sh +++ b/t/t9350-fast-export.sh @@ -303,7 +303,7 @@ test_expect_success 'dropping tag of filtered out object' ' ( cd limit-by-paths && git fast-export --tag-of-filtered-object=drop mytag -- there > output && - test_cmp output expected + test_cmp expected output ) ' @@ -320,7 +320,7 @@ test_expect_success 'rewriting tag of filtered out object' ' ( cd limit-by-paths && git fast-export --tag-of-filtered-object=rewrite mytag -- there > output && - test_cmp output expected + test_cmp expected output ) ' @@ -351,7 +351,7 @@ test_expect_failure 'no exact-ref revisions included' ' ( cd limit-by-paths && git fast-export master~2..master~1 > output && - test_cmp output expected + test_cmp expected output ) ' -- 1.8.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/4] fast-export: don't handle uninteresting refs
They have been marked as UNINTERESTING for a reason, lets respect that.
Currently the first ref is handled properly, but not the rest, so:
% git fast-export master ^master
Would currently throw a reset for master (2nd ref), which is not what we
want.
% git fast-export master ^foo ^bar ^roo
% git fast-export master salsa..tacos
Even if all these refs point to the same object; foo, bar, roo, salsa,
and tacos would all get a reset.
This is most certainly not what we want. After this patch, nothing gets
exported, because nothing was selected (everything is UNINTERESTING).
Signed-off-by: Felipe Contreras
---
builtin/fast-export.c | 7 ---
t/t9350-fast-export.sh | 6 ++
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 065f324..7fb6fe1 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -523,10 +523,11 @@ static void get_tags_and_duplicates(struct object_array
*pending,
typename(e->item->type));
continue;
}
- if (commit->util)
+ if (commit->util) {
/* more than one name for the same object */
- string_list_append(extra_refs, full_name)->util =
commit;
- else
+ if (!(commit->object.flags & UNINTERESTING))
+ string_list_append(extra_refs, full_name)->util
= commit;
+ } else
commit->util = full_name;
}
}
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 49bdb44..6ea8f6f 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -440,4 +440,10 @@ test_expect_success 'fast-export quotes pathnames' '
)
'
+test_expect_success 'proper extra refs handling' '
+ git fast-export master ^master master..master > actual &&
+ echo -n > expected &&
+ test_cmp expected actual
+'
+
test_done
--
1.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/4] fast-export: make sure refs are updated properly
When an object has already been exported (and thus is in the marks) it
is flagged as SHOWN, so it will not be exported again, even if this time
it's exported through a different ref.
We don't need the object to be exported again, but we want the ref
updated, which doesn't happen.
Since we can't know if a ref was exported or not, let's just assume that
if the commit was marked (flags & SHOWN), the user still wants the ref
updated.
So:
% git branch test master
% git fast-export $mark_flags master
% git fast-export $mark_flags test
Would export 'test' properly.
Additionally, this fixes issues with remote helpers; now they can push
refs wich objects have already been exported.
Signed-off-by: Felipe Contreras
---
builtin/fast-export.c | 11 ---
t/t5800-remote-helpers.sh | 11 +++
t/t9350-fast-export.sh| 14 ++
3 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 7fb6fe1..663a93d 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array
*pending,
typename(e->item->type));
continue;
}
- if (commit->util) {
- /* more than one name for the same object */
+
+ /*
+* This ref will not be updated through a commit, lets make
+* sure it gets properly upddated eventually.
+*/
+ if (commit->util || commit->object.flags & SHOWN) {
if (!(commit->object.flags & UNINTERESTING))
string_list_append(extra_refs, full_name)->util
= commit;
- } else
+ }
+ if (!commit->util)
commit->util = full_name;
}
}
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index e7dc668..69a145a 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new refspec'
'
compare_refs clone HEAD server refs/heads/new-refspec
'
+test_expect_success 'push ref with existing object' '
+ (cd localclone &&
+ git branch point-to-master master &&
+ git push origin point-to-master
+ ) &&
+
+ (cd server &&
+ git show-ref refs/heads/point-to-master
+ )
+'
+
test_done
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 6ea8f6f..a4178e3 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -446,4 +446,18 @@ test_expect_success 'proper extra refs handling' '
test_cmp expected actual
'
+cat > expected << EOF
+reset refs/heads/master
+from :13
+
+EOF
+
+test_expect_success 'refs are updated even if no commits need to be exported' '
+ git fast-export --import-marks=tmp-marks \
+ --export-marks=tmp-marks master > /dev/null &&
+ git fast-export --import-marks=tmp-marks \
+ --export-marks=tmp-marks master > actual &&
+ test_cmp expected actual
+'
+
test_done
--
1.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git push tags
On Tue, Oct 30, 2012 at 1:34 PM, Angelo Borsotti wrote: > Hi Cris, > > I think a key in the config file of the remote repo is better than an > option on git-push for what concerns security: it allows the owner of > the remote repo to enforce the policy not to overwrite tags, which > would not be possible if any user that has push access can --force > tags. > > -Angelo Hi Angelo, Security is orthogonal to what this patch is attempting to resolve. As Kacper pointed out, you can never be sure you're not going to clobber an existing tag in the remote repo. This patch attempts to give git-push better (i.e., less surprising) semantics for tags. In other words, it's should will prevent mistakes, not provide any sort of security. So I don't think a config option is better or worse, it's just trying to solve a different problem. Thanks, Chris -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
(again to the mailing list) On Tue, Oct 30, 2012 at 7:59 PM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >> They have been marked as UNINTERESTING for a reason, lets respect that. That doesn't say anything. > and in the examples listed in the patch > description the changed behavior does not look like an improvement. I disagree. % git log master ^master What do you expect? Nothing. % git fast-export master ^master What do you expect? Nothing. > Worse, the description lists a few examples but gives no convincing > explanation to reassure about the lack of bad behavior for examples > not listed. What examples not listed? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/13] New remote-hg helper
Hi Felipe, On Tue, 30 Oct 2012, Felipe Contreras wrote: > But you mentioned something about cooperation, and I've yet to see how > is it that you are planning to cooperate. If you say you don't have time > to spend on this, I don't see why I should worry about testing this > series of patches. It has been mentioned before that the communication style including all these snarky and nasty comments is not helpful. It is hardly the first time that your mails have been insulting, as can be researched easily from in the public mailing list archives. In light of the indignation when advised to keep the tone down a little, it is probable that the mails were never put through the "would I be insulted or hurt if I was the recipient of this?" test, as in "do you want me to throw away my work?" when you literally asked us to throw away our work. So unlike others, I do not ask you to change your tone, nor your willingness to work with others. Instead, I prefer to do other things instead. Hth, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Hi Felipe, On Tue, 30 Oct 2012, Felipe Contreras wrote: > On Tue, Oct 30, 2012 at 8:01 PM, Jonathan Nieder wrote: > > Felipe Contreras wrote: > >> On Tue, Oct 30, 2012 at 7:47 PM, Jonathan Nieder > >> wrote: > > > >>> and in the examples listed in the patch > >>> description the changed behavior does not look like an improvement. > >> > >> I disagree. > >> > >> % git log master ^master > >> > >> What do you expect? Nothing. > > > > Yep. > > > >> % git fast-export master ^master > >> > >> What do you expect? Nothing. > > > > Nope. > > That's _your_ opinion. I would like to see what others think. If you wanted to prove that you can work with others without offending them, I think that failed. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/13] New remote-hg helper
Hi, On Tue, Oct 30, 2012 at 8:33 PM, Johannes Schindelin wrote: > On Tue, 30 Oct 2012, Felipe Contreras wrote: > >> But you mentioned something about cooperation, and I've yet to see how >> is it that you are planning to cooperate. If you say you don't have time >> to spend on this, I don't see why I should worry about testing this >> series of patches. > > It has been mentioned before that the communication style including all > these snarky and nasty comments is not helpful. Snarky and nasty comments? How about this? --- > As to the functionality you seek: git-remote-hg found in > git://github.com/msysgit/git works. It has the following advantages > over every other solution, including the one proposed in this thread: > > - it works > > - no really, it works > > - it supports pushes, too > > - it matured over a long time > > - there are tests > > - whenever we fixed bugs, we also added tests for the bug fixes > > - it is rock solid > > - it is in constant use > > Without push support, remote-hg is useless to me. Without regression > tests proving that it is rock solid, I will not use remote-hg. And I > will not indulge in efforts to duplicate work. --- How many times does somebody has to say "it works" before it becomes a snarky comment? Or this? --- > FTR, the reason that it's crashing is because you're lying. You're > saying you already have master (by means of ^master), but you don't. --- Or this? --- > It seems unlikely to me that this never worked, surely no reviewer > would accept a patch that doesn't actually implement the feature? > What's the history here? --- So what did I say? > But you mentioned something about cooperation, That's a fact. Johannes: --- > > It would be better to work together, but to me the code-styles are way > > too different, the difference between night and day. > > Aha. Well, okay, it was an offer to collaborate. --- > and I've yet to see how is it that you are planning to cooperate. This is also a fact. You haven't provided a branch, you haven't reviewed my implementation, you haven't tried it. You mentioned something about testing the performance, but then retracted from it. So, if you were planning to collaborate, now it would be a good time to mention how. > If you say you don't have time to spend on this, I don't see why I > should worry about testing this series of patches. I'm just clarifying how I'm planning to spend my time, specifically if you are not going to collaborate. What is snarky and nasty about any of these comments? I'm simply asking you if you are going to collaborate and how, because I don't see it, and what I'm going to do. You think that's snarkier than the comments above? Well, I disagree. But I don't blame you when you are snarky, nor do I think I should. > It is hardly the first time that your mails have been insulting, as > can be researched easily from in the public mailing list archives. Those who want to be insulted would get insulted. I asked a simple question "are you going to collaborate?", if you find that offensive, that's your right. > In light of the indignation when advised to keep the tone down a little, > it is probable that the mails were never put through the "would I be > insulted or hurt if I was the recipient of this?" test, as in "do you want > me to throw away my work?" when you literally asked us to throw away our > work. How did I ask you to throw away your work? I have asked multiple times now for you to provide a branch so that we can take a look and try it. I don't know of a better way to throw away code than to refuse to provide it, which is what you have been doing. So, if anybody can be blamed of trying to throw away code, it shouldn't be me. I know you will find the previous statement offensive, but it happens to be true. It is sad that you will concentrate on the statement, rather than the fact, and instead of providing the branch (which will help to avoid throwing away the code), and thus nullify the statement, you choose to be offended and complain about how offended you are. Well, I'm offended at how much you refuse to collaborate, and at how much disdain you throw at my code, but I'm not going to complain about me being offended; people get offended about all sorts of things. Why is why there's no law against offending people. Instead, I choose to do something positive about it and improve my code with your criticism (e.g. lack of tests), even if that criticism is rude and unwarranted. But that seems to mean nothing to you. > So unlike others, I do not ask you to change your tone, nor your > willingness to work with others. Instead, I prefer to do other things > instead. I guess that answers the question; you are not going to collaborate. Got it. I will not ask you again for a branch with the remote-hg code. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: Can't understand the behaviour of git-diff --submodule
Am 28.10.2012 01:02, schrieb Jens Lehmann: > Am 26.10.2012 22:43, schrieb Francis Moreau: >> On Fri, Oct 26, 2012 at 10:05 PM, Jens Lehmann wrote: >> [...] >>> >>> That is weird, "git diff --submodule" should show that too. Is there >>> anything unusual about your setup? (The only explanation I can come >>> up with after checking the code is that your submodule has neither a >>> .git directory nor a gitfile or the objects directory in there doesn't >>> contain these commits) >> >> Oh now you're asking, I think the submodule has been added by using >> the --reference option of git-submodule-add. >> >> $ cd configs >> $ cat .git >> gitdir: ../.git/modules/configs > > Thanks, I suspect the --reference option makes the difference here, > I'll check that as soon as I find some time. Since 1.7.11 and 1.7.10.3 git does handle submodules with alternates (which is what --reference uses) correctly. What version are you seeing this problem with? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras wrote: > Why would it? We are not changing the way objects are exported, the > only difference is what happens at the end > (handle_tags_and_duplicates()). Because the marking is per-commit, not per-ref, right? Perhaps you could add a simple test case to make sure it works as expected? Something along the lines of the scenario I described in my previous email? -- Cheers, Sverre Rabbelier -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: change symlink
shawn wilson writes: > i'm curious why this is being reported as deleted in status and diff > and not modified? this was tested on a build of the master branch of > the current git repo (1.8.0). > > mkdir t cd t; git --init > > touch test > git add test > git commit test -m "test" > > ln -s test t2 > git add t2 > git commit t2 -m "symlink" > > rm t2 > mkdir -p t2/one > ln -s test t2/one/test git add t2/one/test > this then shows up as: > > % git status > # On branch master > # Changes not staged for commit: > # (use "git add/rm ..." to update what will be committed) > # (use "git checkout -- ..." to discard changes in working directory) > # > # deleted:t2 > # > no changes added to commit (use "git add" and/or "git commit -a") I'd expected t2/one/test be reported as untracked. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: change symlink
On Tue, Oct 30, 2012 at 9:19 PM, Andreas Schwab wrote: > shawn wilson writes: >> % git status >> # On branch master >> # Changes not staged for commit: >> # (use "git add/rm ..." to update what will be committed) >> # (use "git checkout -- ..." to discard changes in working directory) >> # >> # deleted:t2 >> # >> no changes added to commit (use "git add" and/or "git commit -a") > > I'd expected t2/one/test be reported as untracked. > that's fine - i can do 'git list-files --others' but should t2 be reported as 'deleted'? -- To unsubscribe from this list: send the line "unsubscribe 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/5] diff: introduce diff.submoduleFormat configuration variable
Am 29.10.2012 11:30, schrieb Ramkumar Ramachandra:
> Jens Lehmann wrote:
>> Am 02.10.2012 21:44, schrieb Jens Lehmann:
>>> Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra:
Introduce a diff.submoduleFormat configuration variable corresponding
to the '--submodule' command-line option of 'git diff'.
>>>
>>> Nice. Maybe a better name would be "diff.submodule", as this sets the
>>> default for the "--submodule" option of diff?
>>>
>>> And I think you should also test in t4041 that "--submodule=short"
>>> overrides the config setting.
>>
>> We also need tests which show that setting that config to "log" does
>> not break one of the many users of "git diff" ("stash", "rebase" and
>> "format-patch" come to mind, most probably I missed some others). I
>> suspect we'll have to add "--submodule=short" options to some call
>> sites to keep them working with submodule changes.
>
> Um, why would "stash", "rebase" or "format-patch" be affected by this
> setting? They don't operate on submodules at all. To be sure, I ran
> all the tests with the following diff and nothing broke.
They do operate on the submodule commits too (while they don't touch
submodule work trees) and IIRC rebase applies diffs, so that could
break when the output of diff changes due to the new config option.
But it looks like your test did prove that nothing goes wrong there,
I assume they they use plumbing diff commands which aren't affected
by the new option.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] completion: simplify __gitcomp test helper
On Mon, Oct 22, 2012 at 03:39:01AM +0200, Felipe Contreras wrote:
> By using print_comp as suggested by SZEDER Gábor.
>
> Signed-off-by: Felipe Contreras
> ---
> t/t9902-completion.sh | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 1c6952a..2e7fc06 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -74,15 +74,12 @@ newline=$'\n'
This $newline variable was only used to set IFS to a newline inside SQ
blocks. AFAICS after this change there are no such places left,
because print_comp() takes care of IFS, so $newline is not necessary
anymore.
> test_gitcomp ()
> {
> + local -a COMPREPLY &&
> sed -e 's/Z$//' > expected &&
> - (
> - local -a COMPREPLY &&
> - cur="$1" &&
> - shift &&
> - __gitcomp "$@" &&
> - IFS="$newline" &&
> - echo "${COMPREPLY[*]}" > out
> - ) &&
> + cur="$1" &&
> + shift &&
> + __gitcomp "$@" &&
> + print_comp &&
> test_cmp expected out
> }
>
> --
> 1.8.0
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Teach rm to remove submodules when given with a trailing '/'
Am 29.10.2012 08:11, schrieb Johannes Sixt:
> Am 10/29/2012 0:28, schrieb Jens Lehmann:
>> +/* Remove trailing '/' from directories to find submodules in the index
>> */
>> +for (i = 0; i < argc; i++) {
>> +size_t pathlen = strlen(argv[i]);
>> +if (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1]
>> == '/'))
>> +argv[i] = xmemdupz(argv[i], pathlen - 1);
>> +}
>> +
>> pathspec = get_pathspec(prefix, argv);
>> refresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);
>
> That's wrong: Either move the check below get_pathspec() (which normalizes
> backslashes to forward-slashes on Windows) or use is_dir_sep().
Thanks for bringing this up.
> But isn't it somewhat dangerous to check pathspec for existance in the
> worktree without interpreting them? Think of magic pathspec syntax (that
> we do not have yet, but which may materialize sometime in the future).
I have to admit I'm not aware of magic pathspec syntax. Do you happen to
have any pointers where I could look at code doing similar things right?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: change symlink
shawn wilson writes: > but should t2 be reported as 'deleted'? Sure, that's what you did. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
Felipe Contreras wrote: > % git fast-export $marks_args one > % git fast-export $marks_args one two > > Then yeah, 'one' will be updated once again in the second command, That's probably worth a mention in the commit message and tests (test_expect_failure), to save future readers from some confusion. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
On Tue, Oct 30, 2012 at 8:01 PM, Jonathan Nieder wrote: > Felipe Contreras wrote: >> On Tue, Oct 30, 2012 at 7:47 PM, Jonathan Nieder wrote: > >>> and in the examples listed in the patch >>> description the changed behavior does not look like an improvement. >> >> I disagree. >> >> % git log master ^master >> >> What do you expect? Nothing. > > Yep. > >> % git fast-export master ^master >> >> What do you expect? Nothing. So you think what we have now is the correct behavior: % git fast-export master ^master reset refs/heads/master from :0 That of course would crash fast-import. But hey, it's your opinion. Would be interesting to see if other people think the above is correct. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe 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] completion: simplify __gitcomp test helper
On Tue, Oct 30, 2012 at 10:27 PM, SZEDER Gábor wrote: > On Mon, Oct 22, 2012 at 03:39:01AM +0200, Felipe Contreras wrote: >> By using print_comp as suggested by SZEDER Gábor. >> >> Signed-off-by: Felipe Contreras >> --- >> t/t9902-completion.sh | 13 + >> 1 file changed, 5 insertions(+), 8 deletions(-) >> >> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh >> index 1c6952a..2e7fc06 100755 >> --- a/t/t9902-completion.sh >> +++ b/t/t9902-completion.sh >> @@ -74,15 +74,12 @@ newline=$'\n' > > This $newline variable was only used to set IFS to a newline inside SQ > blocks. AFAICS after this change there are no such places left, > because print_comp() takes care of IFS, so $newline is not necessary > anymore. Right, I thought I did that =/ -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: change symlink
On Tue, Oct 30, 2012 at 9:35 PM, Andreas Schwab wrote: > shawn wilson writes: > >> but should t2 be reported as 'deleted'? > > Sure, that's what you did. > if i do the same to a file (same repo): touch test2 git add test2 git commit test2 -m "test2" rm test ln -s test2 test git status # On branch master # Changes not staged for commit: # (use "git add/rm ..." to update what will be committed) # (use "git checkout -- ..." to discard changes in working directory) # # deleted:t2 # typechange: test # no changes added to commit (use "git add" and/or "git commit -a") why is this different? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: > So you think what we have now is the correct behavior: > > % git fast-export master ^master > reset refs/heads/master > from :0 No, I don't think that, either. Hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: change symlink
shawn wilson writes: > why is this different? You didn't tell git about t2/one/test. You need to add it first to make it known. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
On Tue, Oct 30, 2012 at 2:35 PM, Felipe Contreras wrote: > On Tue, Oct 30, 2012 at 10:17 PM, Sverre Rabbelier > wrote: >> On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras >> wrote: >>> Why would it? We are not changing the way objects are exported, the >>> only difference is what happens at the end >>> (handle_tags_and_duplicates()). >> >> Because the marking is per-commit, not per-ref, right? > > Oh, you meant using marks? No, I meant the 'SHOWN' flag, doesn't it get added per commit, not per ref? That is, commit->object.flags & SHOWN refers to the object underlying the ref. So I suspect this scenario doesn't pass the tests: git init && echo first > content && git add content && git commit -m "first" && git branch first && echo two > content && git commit -m "second" && git branch second && git fast-export first > actual && test_cmp actual expected_first && git fast-export second > actual && test_cmp actual expected_second With expected_first being something like: And expected_second being something like -- Cheers, Sverre Rabbelier -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
On Tue, Oct 30, 2012 at 10:45 PM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >> So you think what we have now is the correct behavior: >> >> % git fast-export master ^master >> reset refs/heads/master >> from :0 > > No, I don't think that, either. Well, that's what we have now, and you want to preserve this "feature" (aka bug), right? And I still haven't why this is "unsafe", and what are those "examples not listed". -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: > Well, that's what we have now, and you want to preserve this "feature" > (aka bug), right? Nope. I just don't want regressions, and found a patch description that did nothing to explain to the reader how it avoids regressions more than a little disturbing. I also think the proposed behavior is wrong. I don't think there's much benefit to be gained from continuing to discuss this. Consider it a single data point: I would be deeply worried if this patch were applied without at least a clearer description of the change in behavior. Maybe I'm the only one! Hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: change symlink
shawn wilson writes: > and once it's added, status says: > # renamed:t2 -> t2/one/test > > that's not exactly true, but... What's wrong with it? Both files have the same contents, which is the link target for symlinks. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib: avoid full path to store test results
Thanks. I know that posix support these usages, but exists some
traditional shell that not support it. These are described in the
autoconf manual, last time i have checked. As the construct ; export
var = x should be portable, but it is not. If this is important these
days i don't know.
Best
2012/10/30, Jonathan Nieder :
> Elia Pinto wrote:
>
>> The shell word splitting done in base is a bashism, iow not portable.
>
> No, ${varname##glob} is in POSIX and we already use it here and there.
> See Documentation/CodingGuidelines:
>
>- We use ${parameter#word} and its [#%] siblings, and their
> doubled "longest matching" form.
>
> Thanks for looking the patch over.
> Jonathan
>
--
Inviato dal mio dispositivo mobile
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
On Tue, Oct 30, 2012 at 10:59 PM, Sverre Rabbelier wrote: > On Tue, Oct 30, 2012 at 2:35 PM, Felipe Contreras > wrote: >> On Tue, Oct 30, 2012 at 10:17 PM, Sverre Rabbelier >> wrote: >>> On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras >>> wrote: Why would it? We are not changing the way objects are exported, the only difference is what happens at the end (handle_tags_and_duplicates()). >>> >>> Because the marking is per-commit, not per-ref, right? >> >> Oh, you meant using marks? > > No, I meant the 'SHOWN' flag, doesn't it get added per commit, not per > ref? That is, commit->object.flags & SHOWN refers to the object > underlying the ref. So I suspect this scenario doesn't pass the tests: Without marks you cannot have the SHOWN mark at that point; we haven't traversed the commits. > git init && > echo first > content && > git add content && > git commit -m "first" && > git branch first && > echo two > content && > git commit -m "second" && > git branch second && > git fast-export first > actual && > test_cmp actual expected_first && > git fast-export second > actual && > test_cmp actual expected_second > > With expected_first being something like: > > Why would a 'reset' command be expected if the 'first' branch is already pointing to the 'first' commit? > And expected_second being something like > > Ditto, plus, why would 'git fast-export second' do anything regarding 'first'? It wasn't specified in the committish; it's not relevant. Before an after my patch the output is the same: % git fast-export first: reset refs/heads/first commit refs/heads/first % git fast-export second: reset refs/heads/second commit refs/heads/second commit refs/heads/second Which is expected and correct; the branch already points to the right commit, no need for an extra reset. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: change symlink
On Tue, Oct 30, 2012 at 10:09 PM, Andreas Schwab wrote: > shawn wilson writes: > >> and once it's added, status says: >> # renamed:t2 -> t2/one/test >> >> that's not exactly true, but... > > What's wrong with it? Both files have the same contents, which is the > link target for symlinks. > ok, you're right about that (another test where i changed where the symlink pointed): # deleted:test # new file: test/one/t3 however (what got me started wondering about this and a point i forgot about) - t2/one/test doesn't show up under 'untracked files' in in status that scenario. shouldn't it? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >> Well, that's what we have now, and you want to preserve this "feature" >> (aka bug), right? > > Nope. I just don't want regressions, and found a patch description > that did nothing to explain to the reader how it avoids regressions > more than a little disturbing. I see, so you don't have any specific case where this could cause regressions, you are just saying it _might_ (like all patches). > I also think the proposed behavior is wrong. That's a different matter, lets see what others think. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
On Tue, Oct 30, 2012 at 3:18 PM, Felipe Contreras wrote: > Which is expected and correct; the branch already points to the right > commit, no need for an extra reset. I think you're correct. Thanks for confirming. -- Cheers, Sverre Rabbelier -- To unsubscribe from this list: send the line "unsubscribe 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] completion: refactor __gitcomp related tests
On Mon, Oct 22, 2012 at 03:39:00AM +0200, Felipe Contreras wrote:
> Lots of duplicated code!
>
> No functional changes.
I'm not sure.
I'm all for removing duplicated application code, but I'm usually more
conservative when it comes to test code. The more logic, the more
possibility for bugs in tests. So tests should be dead simple, even
if that means some duplicated test code or the lack of convenience
functions.
While this might be considered just a matter of personal preference, ...
> Signed-off-by: Felipe Contreras
> ---
> t/t9902-completion.sh | 72
> ---
> 1 file changed, 23 insertions(+), 49 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index cbd0fb6..1c6952a 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -72,87 +72,61 @@ test_completion_long ()
>
> newline=$'\n'
>
> -test_expect_success '__gitcomp - trailing space - options' '
> - sed -e "s/Z$//" >expected <<-\EOF &&
> - --reuse-message=Z
> - --reedit-message=Z
> - --reset-author Z
> - EOF
> +test_gitcomp ()
> +{
> + sed -e 's/Z$//' > expected &&
> (
> local -a COMPREPLY &&
> - cur="--re" &&
> - __gitcomp "--dry-run --reuse-message= --reedit-message=
> - --reset-author" &&
> + cur="$1" &&
> + shift &&
> + __gitcomp "$@" &&
... I was really puzzled by how __gitcomp() gets its arguments here,
and had to think for a while to figure out why it's not broken.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
On Tue, Oct 30, 2012 at 11:35 PM, Sverre Rabbelier wrote: > On Tue, Oct 30, 2012 at 3:18 PM, Felipe Contreras > wrote: >> Which is expected and correct; the branch already points to the right >> commit, no need for an extra reset. > > I think you're correct. Thanks for confirming. Thanks for reviewing. If you are still not convinced, I could pull the patches from msysgit and simplify them, I'm sure the end result would be pretty similar, if not exactly the same as this patch (plus other orthogonal changes). I saw some patches that were not part of the patch series you sent before, so maybe that's why you expected certain behavior that wasn't actually there in that particular patch series. But hopefully that's not needed. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe 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 v6 2/3] completion: add new __gitcompadd helper
On Mon, Oct 22, 2012 at 03:45:41AM +0200, Felipe Contreras wrote: > The idea is to never touch the COMPREPLY variable directly. > > This allows other completion systems override __gitcompadd, and do > something different instead. > > Also, this allows the simplification of the completion tests (separate > patch). This doesn't apply anymore, does it? The mentioned simplification is done in the other series. -- To unsubscribe from this list: send the line "unsubscribe 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] completion: refactor __gitcomp related tests
On Tue, Oct 30, 2012 at 11:45 PM, SZEDER Gábor wrote:
> On Mon, Oct 22, 2012 at 03:39:00AM +0200, Felipe Contreras wrote:
>> Lots of duplicated code!
>>
>> No functional changes.
>
> I'm not sure.
> I'm all for removing duplicated application code, but I'm usually more
> conservative when it comes to test code. The more logic, the more
> possibility for bugs in tests. So tests should be dead simple, even
> if that means some duplicated test code or the lack of convenience
> functions.
> While this might be considered just a matter of personal preference, ...
Fair enough, but my preference is different: test code should be the
same as normal code; easy to maintain.
>> +test_gitcomp ()
>> +{
>> + sed -e 's/Z$//' > expected &&
>> (
>> local -a COMPREPLY &&
>> - cur="--re" &&
>> - __gitcomp "--dry-run --reuse-message= --reedit-message=
>> - --reset-author" &&
>> + cur="$1" &&
>> + shift &&
>> + __gitcomp "$@" &&
>
> ... I was really puzzled by how __gitcomp() gets its arguments here,
> and had to think for a while to figure out why it's not broken.
You mean because "$@" is treated in a special way? Without that
behavior I'm not sure many things would be possible :)
Anyway, I don't think we should make our life more difficult while
maintaining test code... that's my opinion.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe 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 v6 2/3] completion: add new __gitcompadd helper
On Tue, Oct 30, 2012 at 11:58 PM, SZEDER Gábor wrote: > On Mon, Oct 22, 2012 at 03:45:41AM +0200, Felipe Contreras wrote: >> The idea is to never touch the COMPREPLY variable directly. >> >> This allows other completion systems override __gitcompadd, and do >> something different instead. >> >> Also, this allows the simplification of the completion tests (separate >> patch). > > This doesn't apply anymore, does it? The mentioned simplification is > done in the other series. Yeah, but you mentioned you didn't like all the COMPREPLY=() changes and it might be time to get rid of them. So this series supersedes that one. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] completion: add new __gitcompadd helper
On Mon, Oct 22, 2012 at 02:41:21AM +0200, Felipe Contreras wrote:
> On Wed, Oct 17, 2012 at 7:28 PM, SZEDER Gábor wrote:
> > On Sun, Oct 14, 2012 at 05:52:49PM +0200, Felipe Contreras wrote:
>
> >> diff --git a/contrib/completion/git-completion.bash
> >> b/contrib/completion/git-completion.bash
> >> index d743e56..01325de 100644
> >> --- a/contrib/completion/git-completion.bash
> >> +++ b/contrib/completion/git-completion.bash
> >> @@ -225,6 +225,11 @@ _get_comp_words_by_ref ()
> >> fi
> >> fi
> >>
> >> +__gitcompadd ()
> >> +{
> >> + COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
> >> +}
> >> +
> >> # Generates completion reply with compgen, appending a space to possible
> >> # completion words, if necessary.
> >> # It accepts 1 to 4 arguments:
> >> @@ -238,13 +243,11 @@ __gitcomp ()
> >>
> >> case "$cur_" in
> >> --*=)
> >> - COMPREPLY=()
> >> + __gitcompadd
> >> ;;
> >> *)
> >> local IFS=$'\n'
> >> - COMPREPLY=($(compgen -P "${2-}" \
> >> - -W "$(__gitcomp_1 "${1-}" "${4-}")" \
> >> - -- "$cur_"))
> >> + __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}"
> >> "$cur_" ""
> >> ;;
> >> esac
> >> }
> >> @@ -261,7 +264,7 @@ __gitcomp ()
> >> __gitcomp_nl ()
> >> {
> >> local IFS=$'\n'
> >> - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> >> + __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"
> >> }
> >
> > I feel hesitant about this change. One of the ways I'm exploring to
> > fix the issues with shell metacharacters and expansion in compgen is
> > to actually replace compgen. We already iterate over all possible
> > completion words in __gitcomp_1(), so it doesn't make much of a
> > difference to do the filtering for the current word while we are at
> > it. However, the way __gitcompadd() encapsulates COMPREPLY=($(compgen
> > ...)), and tha basic idea of never touching COMPREPLY directly make
> > this basically impossible.
>
> How is it impossible? You can still replace compgen, all you have to
> do is modify __gitcompadd and replace that code with whatever custom
> code you want. You can change the arguments and everything. The only
> limitation is that it should be the only place where COMPREPLY is
> modified, and all is good. Well, it doesn't have to be only _one_
> place, but the less functions that do this, the better.
That's exactly the problem: there isn't, there can't be one single
"whatever custom code I want".
The compgen() in __gitcomp() will be replaced by an enhanced version
of the loop in __gitcomp_1(), while in __gitcomp_nl() it will be
replaced by a little awk scriptlet. And then there is the oddball
$(git ls-tree |sed magic) in __git_complete_revlist_file(), where
possible completion words are filenames possibly containing newlines,
therefore requiring yet another approach.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Links broken in ref docs.
I just checked and the issue seems to be fixed! Clicked around on a bunch of previously broken links and they work! On Tue, Oct 30, 2012 at 3:38 AM, Holger Hellmuth (IKS) wrote: > Am 30.10.2012 09:07, schrieb Mike Norman: > >> Not seen any recently. I'm guessing the dev is in the path of >> hurricane Sandy? (Not sarcasm, btw.) > > > Do you still see failures? I checked out the website just now and it seemed > to work flawlessly (at least the links I tried, could not find any Sharing > or Updating section). New design since I last visited, more end-user polish. > > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: > On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder wrote: >> Nope. I just don't want regressions, and found a patch description >> that did nothing to explain to the reader how it avoids regressions >> more than a little disturbing. > > I see, so you don't have any specific case where this could cause > regressions, you are just saying it _might_ (like all patches). Yes, exactly. The commit log needs a description of the current behavior, the intent behind the current code, the change the patch makes, and the motivation behind that change, like all patches. Despite the nice examples, it doesn't currently have that. The patch description just raises more questions for the reader. From the description, one might imagine that this patch causes git fast-export master not to emit anything when another branch that has already been exported is ahead of "master". If I understand correctly (though I haven't tested), this patch does cause git fast-export ^next master not to emit anything when next is ahead of "master". That doesn't seem like progress. I haven't reviewed the later patches in the series; maybe they fix these things. But in the long term it is much easier to understand and maintain a patch series that does not introduce regressions in the first place, and the context one might use to convincingly explain that a patch is not introducing a regression turns out to be essential for many other purposes as well. Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
(cc-ing the git list) Felipe Contreras wrote: > When an object has already been exported (and thus is in the marks) it > is flagged as SHOWN, so it will not be exported again, even if this time > it's exported through a different ref. > > We don't need the object to be exported again, but we want the ref > updated Yes, makes perfect sense. For what it's worth, Acked-by: Jonathan Nieder [...] > --- a/t/t5800-remote-helpers.sh > +++ b/t/t5800-remote-helpers.sh > @@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new > refspec' ' > compare_refs clone HEAD server refs/heads/new-refspec > ' > > +test_expect_success 'push ref with existing object' ' > + (cd localclone && > + git branch point-to-master master && > + git push origin point-to-master > + ) && > + > + (cd server && > + git show-ref refs/heads/point-to-master > + ) Style: if you indent like this, the test becomes clearer: ( cd localclone && git branch point-to-master master && git push origin point-to-master ) && ( cd server && git rev-parse --verify refs/heads/point-to-master ) [...] > +test_expect_success 'refs are updated even if no commits need to be > exported' ' > + git fast-export --import-marks=tmp-marks \ > + --export-marks=tmp-marks master > /dev/null && The redirect just makes the test log with "-v" less informative, so I'd drop it. > + git fast-export --import-marks=tmp-marks \ > + --export-marks=tmp-marks master > actual && > + test_cmp expected actual Redirections in git shell scripts are generally spelled as "do_something >actual", without a space between the operator and filename. Hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
Felipe Contreras wrote:
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array
> *pending,
> typename(e->item->type));
> continue;
> }
> - if (commit->util) {
> - /* more than one name for the same object */
> +
> + /*
> + * This ref will not be updated through a commit, lets make
> + * sure it gets properly upddated eventually.
> + */
> + if (commit->util || commit->object.flags & SHOWN) {
> if (!(commit->object.flags & UNINTERESTING))
> string_list_append(extra_refs, full_name)->util
> = commit;
> - } else
> + }
> + if (!commit->util)
> commit->util = full_name;
Here's an explanation of why the above makes sense to me.
get_tags_and_duplicates() gets called after the marks import and
before the revision walk. It walks through the revs from the
commandline and for each one:
- peels it to a refname, and then to a commit
- stores the refname so fast-export knows what arg to pass to
the "commit" command during the revision walk
- if it already had a refname stored, instead adds the
(refname, commit) pair to the extra_refs list, so fast-export
knows to add a "reset" command later.
If the commit already has the SHOWN flag set because it was pointed to
by a mark, it is not going to come up in the revision walk, so it will
not be mentioned in the output stream unless it is added to
extra_refs. That's what this patch does.
Incidentally, the change from "else" to "if (!commit->util)" is
unnecessary because if a commit is already SHOWN then it will not be
encountered in the revision walk so commit->util does not need to be
set.
If the commit does not have the SHOWN or UNINTERESTING flag set but it
is going to get the UNINTERESTING flag set during the walk because of
a negative commit listed on the command line, this patch won't help.
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Hi again, Felipe Contreras wrote: > They have been marked as UNINTERESTING for a reason, lets respect that. So, the above description conveyed zero information, as you mentioned. A clearer explanation would be the following: fast-export: don't emit "reset" command for negative refs When "git fast-export" encounters two refs on the commandline referring to the same commit, it exports the first during the usual commit walk and the second using a "reset" command in a final pass over extra_refs: $ git fast-export master next reset refs/heads/master commit refs/heads/master mark :1 author Jonathan Nieder 1351644412 -0700 committer Jonathan Nieder 1351644412 -0700 data 17 My first commit! reset refs/heads/next from :1 Unfortunately the code to do this doesn't distinguish between positive and negative refs, producing confusing results: $ git fast-export ^master next reset refs/heads/next from :0 $ git fast-export master ^next reset refs/heads/next from :0 Use revs->cmdline instead of revs->pending to iterate over the rev-list arguments, checking the UNINTERESTING flag bit to distinguish between positive (master, --all, etc) and negative (next.., --not --all, etc) revs and avoid enqueueing negative revs in extra_revs. This does not affect revs that were excluded from the revision walk because pointed to by a mark, since those use the SHOWN bit on the commit object itself and not UNINTERESTING on the rev_cmdline_entry. A patch meeting the above description would make perfect sense to me. Except for the somewhat strange testcase, the patch I am replying to would also be fine in the short term, as long as it had an analagous description (i.e., with an appropriate replacement for the second-to-last paragraph). Thanks for your patience, and hoping that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
On Wed, Oct 31, 2012 at 12:55 AM, Jonathan Nieder wrote:
> Felipe Contreras wrote:
>> On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder wrote:
>
>>> Nope. I just don't want regressions, and found a patch description
>>> that did nothing to explain to the reader how it avoids regressions
>>> more than a little disturbing.
>>
>> I see, so you don't have any specific case where this could cause
>> regressions, you are just saying it _might_ (like all patches).
>
> Yes, exactly. The commit log needs a description of the current
> behavior, the intent behind the current code, the change the patch
> makes, and the motivation behind that change, like all patches.
> Despite the nice examples, it doesn't currently have that.
>
> The patch description just raises more questions for the reader. From
> the description, one might imagine that this patch causes
>
> git fast-export master
>
> not to emit anything when another branch that has already been
> exported is ahead of "master".
This is already the case.
I don't see what part of my patch description would give you the idea
that this would change in any way how the objects are flagged, or how
get_revision() decides how to traverse them.
I clearly stated that this doesn't affect *the first* ref, which is
handled properly already; this patch affects *the rest* of the refs,
of which you have none in that command above.
> If I understand correctly (though
> I haven't tested), this patch does cause
>
> git fast-export ^next master
>
> not to emit anything when next is ahead of "master". That doesn't
> seem like progress.
Again, this is already the case RIGHT NOW.
And nothing in my description should give you an idea that anything
would change for this case because the 2nd ref (*the first* doesn't
get affected), is not marked as UNINTERESTING.
Not only you are not reading what is in the description, but I don't
think you understand what the code actually does, and how it behaves.
Let me give you some examples:
% git fast-export ^next next
reset refs/heads/next
from :0
% git fast-export ^next next^{commit}
# nothing
% git fast-export ^next next~0
# nothing
% git fast-export ^next next~1
# nothing
% git fast-export ^next next~2
# nothing
...
# you get the idea
The *only time* when this patch would have any effect is when you
specify more than *one ref*, and they both point to *exactly the same
object*.
Additionally, and this is something I just found out; when the are
pure refs (e.g. 'next'), and not refs to objects (e.g.
'next^{commit}').
In any other case; *there would be no change*.
After my patch:
% git fast-export ^next next
# nothing
% git fast-export ^next next^{commit}
# nothing
% git fast-export ^next next~0
# nothing
% git fast-export ^next next~1
# nothing
% git fast-export ^next next~2
# nothing
...
# you get the idea
> But in the long term it is much easier to understand
> and maintain a patch series that does not introduce regressions in the
> first place
It does not introduce regressions.
I don't think it's my job to explain to you how 'git fast-export'
works. Above you made too many assumptions of what get broken, when in
fact that's the current behavior already... maybe, just maybe, you are
also making wrong assumptions about this patch as well.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: > I don't think it's my job to explain to you how 'git fast-export' > works. Actually, if you are submitting a patch for inclusion, it is your job to explain to future readers what the patch does. Yes, the reader might not be deeply familiar with the part of fast-export you are modifying. It might have even been modified since then, by the time the reader is looking at the change! Sad but true. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Hi,
On Wed, Oct 31, 2012 at 1:57 AM, Jonathan Nieder wrote:
> Felipe Contreras wrote:
>
>> They have been marked as UNINTERESTING for a reason, lets respect that.
>
> So, the above description conveyed zero information, as you mentioned.
I meant, this, of course:
>> They have been marked as UNINTERESTING for a reason, lets respect that.
>
> This patch looks unsafe,
Which you know, because you received that message without the mistake.
> A clearer explanation would be the following:
>
> fast-export: don't emit "reset" command for negative refs
What is a negative ref?
> When "git fast-export" encounters two refs on the commandline
commandline?
Only two refs? How about four?
> referring to the same commit, it exports the first during the usual
> commit walk and the second using a "reset" command in a final pass
> over extra_refs:
That is not exactly true: (next^{commit}).
> $ git fast-export master next
> reset refs/heads/master
> commit refs/heads/master
> mark :1
> author Jonathan Nieder 1351644412 -0700
> committer Jonathan Nieder 1351644412
> -0700
> data 17
> My first commit!
>
> reset refs/heads/next
> from :1
I don't think this example is good. Where does it say that 'next'
points to master? Using 'points-to-master' or a 'git branch stable
master' and using 'master stable'.
Even simpler would be to use 'git fast-export master master'; it would
show the same behavior.
> Unfortunately the code to do this doesn't distinguish between positive
> and negative refs, producing confusing results:
>
> $ git fast-export ^master next
> reset refs/heads/next
> from :0
>
> $ git fast-export master ^next
> reset refs/heads/next
> from :0
>
> Use revs->cmdline instead of revs->pending to iterate over the
> rev-list
> arguments, checking the UNINTERESTING flag bit to distinguish between
> positive (master, --all, etc) and negative (next.., --not --all, etc)
> revs and avoid enqueueing negative revs in extra_revs.
Use what? You mean, "To solve the problem, lets use".
But this is not correct, cmdline is not being used. Have you even
looked at the patch?
> This does not affect revs that were excluded from the revision walk
> because pointed to by a mark, since those use the SHOWN bit on the
> commit object itself and not UNINTERESTING on the rev_cmdline_entry.
revs? You mean commits?
"excluded because point to by a mark"? Doesn't sound like proper
grammar. Maybe "excluded because they were pointed to by a mark".
And I don't see why this paragraph is needed at all. Why would the
reader think marks have anything to do with this? There's no mention
of marks before.
This might help you, or other people involved in the problem, but not
anybody else. Anything related to marks is completely orthogonal to
this patch, and there's no point in mentioning that.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib: avoid full path to store test results
Felipe Contreras wrote: > On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder wrote: > > Felipe Contreras wrote: >>> No reason to use the full path in case this is used externally. >>> >>> Signed-off-by: Felipe Contreras >> >> "No reason not to" is not a reason to do anything. What symptoms does >> this prevent? Could you describe the benefit of this patch in a >> paragraph starting "Now you can ..."? > > ./test-lib.sh: line 394: > /home/felipec/dev/git/t/test-results//home/felipec/dev/git/contrib/remote-hg/test-21865.counts: > No such file or directory Ok, so a description for this patch is test: use test's basename to name results file Running a test using its full path produces an error: $ ~/dev/git/contrib/remote-hg/test-21865.sh [...] ./test-lib.sh: line 394: /home/felipec/dev/t/\ test-results//home/felipec/dev/git/contrib/remote-hg/\ test-21865.counts: No such file or directory In --tee and --valgrind modes we already use the basename to name the .out and .exit files; this patch teaches the test-lib to name the .counts file the same way. That is still not enough to tell if it is a good change, though. Should the test results for contrib/remote-hg/test-* be included with the results for t/t*.sh when I run "make aggregate-results"? Before 60d02ccc, git-svn had its own testsuite under contrib/, with glue in contrib/git-svn/t/lib-git-svn.sh to use test-lib --- maybe that code could provide some inspiration for questions like these. Hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
On Tue, Oct 30, 2012 at 5:37 PM, Jonathan Nieder wrote:
> Felipe Contreras wrote:
>
>> --- a/builtin/fast-export.c
>> +++ b/builtin/fast-export.c
>> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct
>> object_array *pending,
>> typename(e->item->type));
>> continue;
>> }
>> - if (commit->util) {
>> - /* more than one name for the same object */
>> +
>> + /*
>> + * This ref will not be updated through a commit, lets make
>> + * sure it gets properly upddated eventually.
>> + */
>> + if (commit->util || commit->object.flags & SHOWN) {
>> if (!(commit->object.flags & UNINTERESTING))
>> string_list_append(extra_refs,
>> full_name)->util = commit;
>> - } else
>> + }
>> + if (!commit->util)
>> commit->util = full_name;
>
> Here's an explanation of why the above makes sense to me.
>
> get_tags_and_duplicates() gets called after the marks import and
> before the revision walk. It walks through the revs from the
> commandline and for each one:
>
> - peels it to a refname, and then to a commit
> - stores the refname so fast-export knows what arg to pass to
>the "commit" command during the revision walk
> - if it already had a refname stored, instead adds the
>(refname, commit) pair to the extra_refs list, so fast-export
>knows to add a "reset" command later.
>
> If the commit already has the SHOWN flag set because it was pointed to
> by a mark, it is not going to come up in the revision walk, so it will
> not be mentioned in the output stream unless it is added to
> extra_refs. That's what this patch does.
>
> Incidentally, the change from "else" to "if (!commit->util)" is
> unnecessary because if a commit is already SHOWN then it will not be
> encountered in the revision walk so commit->util does not need to be
> set.
>
> If the commit does not have the SHOWN or UNINTERESTING flag set but it
> is going to get the UNINTERESTING flag set during the walk because of
> a negative commit listed on the command line, this patch won't help.
Thanks for the thorough explanation. Perhaps some of that could make
it's way into the commit message?
--
Cheers,
Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: > This might help you, or other people involved in the problem, but not > anybody else. Ok, I give up. Bye. Sometimes the author of some code and the right person to interact with the development community by submitting and maintaining it are not the same person. Hopefully others more patient than we two can pick up where we left off. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
On Wed, Oct 31, 2012 at 2:08 AM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >> I don't think it's my job to explain to you how 'git fast-export' >> works. > > Actually, if you are submitting a patch for inclusion, it is your job > to explain to future readers what the patch does. That's already explained. > Yes, the reader > might not be deeply familiar with the part of fast-export you are > modifying. This has nothing to do with what you said. I'm literally explaining to you how 'git fast-export' works in situations that are completely orthogonal to this patch, because you are using wrong examples as grounds to prevent this patch from being accepted. It's not my job to explain to you that 'git fast-export' doesn't work this way, you have a command line to type those commands and see for yourself if they do what you think they do with a vanilla version of git. That's exactly what I did, to make sure I'm not using assumptions as basis for arguing, it took me a few minutes. That being said, if your problem is that it's not clear to people not deeply familiar with that part of fast-export, this extra paragraph in addition to the current commit message should do the trick: --- The reason this happens is that before traversing the commits, fast-export checks if any of the refs point to the same object, and any duplicated ref gets added to a list in order to issue 'reset' commands after the traversing. Unfortunately, it's not even checking if the commit is flagged as UNINTERESTING. The fix of course, is to do precisely that. --- And to get that all had to do is ask: "Can you please add an explanation of what this part of the code does? For the ones of us not familiar with it". Not; "This patch looks unsafe", "This patch makes Sally mad", "This patch causes regressions", and so on. But hey, at least we are not arguing about what is wrong with this patch (or so I hope). Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: >It's not my job to > explain to you that 'git fast-export' doesn't work this way, you have > a command line to type those commands and see for yourself if they do > what you think they do with a vanilla version of git. That's exactly > what I did, to make sure I'm not using assumptions as basis for > arguing, it took me a few minutes. Well no, when I run "git blame" 10 years down the line and do not understand what your code is doing, it is not at all reasonable to expect me to checkout the parent commit, get it to compile with a modern toolchain, and type those commands for myself. Instead, the commit message should be self-contained and explain what the patch does. That has multiple parts: - first, what the current behavior is - second, what the intent behind the current behavior is. This is crucial information because presumably we want the change not to break that. - third, what change the patch makes - fourth, what the consequences of that are, in terms of new use cases that become possible and old use cases that become less convenient - fifth, optionally, how the need for this change was discovered (real-life usage, code inspection, or something else) - sixth, optionally, implementation considerations and alternate approaches that were discarded If you run "git log", you'll see many good and bad examples to think over and compare to this goal. It's hard work to describe one's work well in terms that other people can understand, but I think it can be satisfying, and in any event, it's just as necessary as including comments near confusing code. Sincerely, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib: avoid full path to store test results
On Wed, Oct 31, 2012 at 2:27 AM, Jonathan Nieder wrote: > Felipe Contreras wrote: >> On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder wrote: >> > Felipe Contreras wrote: > No reason to use the full path in case this is used externally. Signed-off-by: Felipe Contreras >>> >>> "No reason not to" is not a reason to do anything. What symptoms does >>> this prevent? Could you describe the benefit of this patch in a >>> paragraph starting "Now you can ..."? >> >> ./test-lib.sh: line 394: >> /home/felipec/dev/git/t/test-results//home/felipec/dev/git/contrib/remote-hg/test-21865.counts: >> No such file or directory > > Ok, so a description for this patch is > > test: use test's basename to name results file Is this solving an actual problem or is it just something nice to do? Like in all good novels, one has to read more find out... > Running a test using its full path produces an error: I'm not sure what that even means. Do you mean this produces an error? % make -C t $PWD/t9902-completion.sh Well, sure it does, but this patch doesn't fix that. If you want a precise explanation of what kind of usages are enabled by this patch, that would require some work, and no I haven't done it, and no, I'm not sure. > $ ~/dev/git/contrib/remote-hg/test-21865.sh > [...] > ./test-lib.sh: line 394: /home/felipec/dev/t/\ > test-results//home/felipec/dev/git/contrib/remote-hg/\ > test-21865.counts: No such file or directory Except that I didn't do this. So the fact that this happens is an assumption, and I'm not willing to make that. Most likely if somebody does that they are doing something wrong; they didn't define the TESTDIR variable (or something like that). It's all fun and games to write explanations for things, but it's not that easy when you want those explanations to be actually true, and corrent--you have to spend time to make sure of that. > In --tee and --valgrind modes we already use the basename > to name the .out and .exit files; this patch teaches the test-lib > to name the .counts file the same way. I don't see the point of listing each and every place where this already happens. As a matter of fact, the base-name is used in other places as well, and just saying "This is already done in other places", is more than enough. But who says they are not the ones doing it wrong? Maybe this part of the code is right, and it's the others that need fixing. I don't see how saying "Others are doing it" makes the patch better or worse in any way. There might also be different reasons for why they do it that doesn't apply here. > That is still not enough to tell if it is a good change, though. > Should the test results for contrib/remote-hg/test-* be included with > the results for t/t*.sh when I run "make aggregate-results"? > > Before 60d02ccc, git-svn had its own testsuite under contrib/, with > glue in contrib/git-svn/t/lib-git-svn.sh to use test-lib --- maybe > that code could provide some inspiration for questions like these. Or maybe they are the ones that should look for inspiration in contrib/remote-hg. The patch is obviously correct; it's generally good not to name files with slashes in them, and $0 is not guaranteed not to have slashes. Even if you run all the tests inside the 't' directory, this script is not only used by git, and others might want sub-directories, and not thousands of tests on the same directory like git. Either way, if obvious fixes that are one-liners require an essay for you, I give up. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly
On Wed, Oct 31, 2012 at 1:11 AM, Jonathan Nieder wrote: > (cc-ing the git list) > Felipe Contreras wrote: > >> When an object has already been exported (and thus is in the marks) it >> is flagged as SHOWN, so it will not be exported again, even if this time >> it's exported through a different ref. >> >> We don't need the object to be exported again, but we want the ref >> updated > > Yes, makes perfect sense. > > For what it's worth, > Acked-by: Jonathan Nieder Yay! > [...] >> --- a/t/t5800-remote-helpers.sh >> +++ b/t/t5800-remote-helpers.sh >> @@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new >> refspec' ' >> compare_refs clone HEAD server refs/heads/new-refspec >> ' >> >> +test_expect_success 'push ref with existing object' ' >> + (cd localclone && >> + git branch point-to-master master && >> + git push origin point-to-master >> + ) && >> + >> + (cd server && >> + git show-ref refs/heads/point-to-master >> + ) > > Style: if you indent like this, the test becomes clearer: And then it would become inconsistent with the rest of the file. >> + git fast-export --import-marks=tmp-marks \ >> + --export-marks=tmp-marks master > actual && >> + test_cmp expected actual > > Redirections in git shell scripts are generally spelled as > "do_something >actual", without a space between the operator and > filename. I generally am OK with adapting to whatever code-style is used (sometimes under protest), but this is a place where I draw the line. Sorry, '>actual' is more annoying to me than a knife screeching glass. Fortunately, '> actual' is used in many other places in 't/', so I'm going to use the other people jumping over the bridge argument. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly
On Wed, Oct 31, 2012 at 1:37 AM, Jonathan Nieder wrote:
> Felipe Contreras wrote:
>
>> --- a/builtin/fast-export.c
>> +++ b/builtin/fast-export.c
>> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct
>> object_array *pending,
>> typename(e->item->type));
>> continue;
>> }
>> - if (commit->util) {
>> - /* more than one name for the same object */
>> +
>> + /*
>> + * This ref will not be updated through a commit, lets make
>> + * sure it gets properly upddated eventually.
>> + */
>> + if (commit->util || commit->object.flags & SHOWN) {
>> if (!(commit->object.flags & UNINTERESTING))
>> string_list_append(extra_refs,
>> full_name)->util = commit;
>> - } else
>> + }
>> + if (!commit->util)
>> commit->util = full_name;
>
> Here's an explanation of why the above makes sense to me.
>
> get_tags_and_duplicates() gets called after the marks import and
> before the revision walk. It walks through the revs from the
> commandline and for each one:
>
> - peels it to a refname, and then to a commit
> - stores the refname so fast-export knows what arg to pass to
>the "commit" command during the revision walk
> - if it already had a refname stored, instead adds the
>(refname, commit) pair to the extra_refs list, so fast-export
>knows to add a "reset" command later.
>
> If the commit already has the SHOWN flag set because it was pointed to
> by a mark, it is not going to come up in the revision walk, so it will
> not be mentioned in the output stream unless it is added to
> extra_refs. That's what this patch does.
That is correct.
> Incidentally, the change from "else" to "if (!commit->util)" is
> unnecessary because if a commit is already SHOWN then it will not be
> encountered in the revision walk so commit->util does not need to be
> set.
Maybe, but that's yet another change, and with more changes come more
possibilities of regressions. I haven't verified this is the case.
If this makes sense, I would do it in another, separate patch.
> If the commit does not have the SHOWN or UNINTERESTING flag set but it
> is going to get the UNINTERESTING flag set during the walk because of
> a negative commit listed on the command line, this patch won't help.
I don't know what that means in practice.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-lib: avoid full path to store test results
Felipe Contreras wrote: > It's all fun and games to write explanations for things, but it's not > that easy when you want those explanations to be actually true, and > corrent--you have to spend time to make sure of that. That's why it's useful for the patch submitter to write them, asking for help when necessary. As a bonus, it helps reviewers understand the effect of the patch. Bugs averted! [...] > Either way, if obvious fixes that are one-liners require an essay for > you, I give up. I guess it is fair to call a reasonable subject line plus a couple of sentences an essay. Yes, obvious fixes especially require that, since the bug caused by an obvious fix is one of the worst kinds. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
On Wed, Oct 31, 2012 at 2:51 AM, Jonathan Nieder wrote: > Felipe Contreras wrote: > >>It's not my job to >> explain to you that 'git fast-export' doesn't work this way, you have >> a command line to type those commands and see for yourself if they do >> what you think they do with a vanilla version of git. That's exactly >> what I did, to make sure I'm not using assumptions as basis for >> arguing, it took me a few minutes. > > Well no, when I run "git blame" 10 years down the line and do not > understand what your code is doing, it is not at all reasonable to > expect me to checkout the parent commit, get it to compile with a > modern toolchain, and type those commands for myself. > > Instead, the commit message should be self-contained and explain what > the patch does. > > That has multiple parts: > > - first, what the current behavior is > > - second, what the intent behind the current behavior is. This is >crucial information because presumably we want the change not to >break that. > > - third, what change the patch makes > > - fourth, what the consequences of that are, in terms of new use >cases that become possible and old use cases that become less >convenient > > - fifth, optionally, how the need for this change was discovered >(real-life usage, code inspection, or something else) > > - sixth, optionally, implementation considerations and alternate >approaches that were discarded I don't see any "Explain in detail what different commands do, even if they are irrelevant to the patch in question because someone might think they would get broken by this patch when in fact they wouldn't", that might belong in the discussion, but not in the commit message, and certainly not in the form of any entitlement. Again, it's _your_ responsibility to make sure the commands you say might get broken do actually work with your current git, it's not mine to run them for you, even though that's exactly what I did, because I'm interested in getting things correctly on record. And FTR, since you removed it, here is what I proposed to add to the commit message: --- The reason this happens is that before traversing the commits, fast-export checks if any of the refs point to the same object, and any duplicated ref gets added to a list in order to issue 'reset' commands after the traversing. Unfortunately, it's not even checking if the commit is flagged as UNINTERESTING. The fix of course, is to do precisely that. --- With that, all the points above are tackled, except fourth, because there aren't any. Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
