git apply --index --reject does not add new files

[Karoly Negyesi]

Hi,

I believe this to be a bug. git apply --index does add new files but
git apply --index --reject does not even those that apply without
errors.

Regards

Karoly Negyesi
--
To unsubscribe from this list: send the line "unsubscribe 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] show-branch: use strbuf instead of static buffer

[Jeff King]

On Fri, Apr 05, 2013 at 04:49:15PM -0700, Jonathan Nieder wrote:

> > Though this is a stack overflow, I don't know that it's exploitable for
> > anything interesting; an attacker does not get to write arbitrary data,
> > but rather only a sequence of "^%d" and "~%d" relative history markers.
> > Perhaps in theory one could devise a history such that the sequence
> > markers spelled out some malicious code, but it would be quite a
> > challenge
> 
> Overwrite the return address and return-to-libc?

Still hard, since you need to construct a usable address (and arguments)
out of sequences of "^[0-9]+" and "~[0-9]+". But I'd love to see a
working exploit if somebody thinks they can do it. :)

> Very clean and obviously correct.  Thanks.
> 
> Reviewed-by: Jonathan Nieder 

Thanks.

> A test would be nice, though.

What should it be testing? That a giant chain of second-parent merges
that exceeds 1000 bytes doesn't segfault? Tests like that are not all
that interesting, because they do not catch real-world regressions. We
have closed this barn door; it is not impossible that it will be
re-opened, but it is not likely. A test that checks only for a very
specific type of failure is only ever going to see that failure.

If you want to design a suite of tests that check that show-branch gives
correct output for particular brands of large repo, that would be
generic and potentially useful. But I don't think it's actually worth
spending a lot of time on (reviewing the code for more static buffers
and sprintfs would probably be a much more fruitful use of time).

-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: Fwd: Delivery Status Notification (Failure)

[Jeff King]

On Sat, Apr 06, 2013 at 12:15:33AM -0400, Drew Gross wrote:

> I'm am trying to patch git to refuse to commit if there are both staged and
> unstaged changes, and I pass the -a flag. I expected this simple addition
> to parse_and_validate_options() in commit.c to accomplish most of what I
> want:
> 
> if (all && s->workdir_dirty)
> die(_("Cannot commit with -a if there are staged and unstaged changes"));
> 
> But when compiled, this will commit anyway even with staged and unstaged
> files. I think I misunderstanding the workdir_dirty flag. Can someone help
> me?

I am not sure if that is a good safety measure in general. But
ignoring that question for a moment, there are a few issues with your
proposed implementation:

  1. workdir_dirty only talks about unstaged changes; it sounds like you
 want to check for both staged and unstaged.

  2. no flags in in the wt_status struct are set until wt_status_collect
 is called; you need to put your check later. But of course by the
 time we call it, we have already updated the index, so you would
 not know anymore which changes were already in the index and which
 were added by "-a".

  3. we do not always run wt_collect_status (e.g., if you are not going
 to run an editor, we do not bother going to the effort to create
 the commit template).

So you'd probably need something more like this:

diff --git a/builtin/commit.c b/builtin/commit.c
index 4620437..ebb5480 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1061,6 +1061,10 @@ static int parse_and_validate_options(int argc, const 
char *argv[],
if (status_format != STATUS_FORMAT_NONE)
dry_run = 1;
 
+   wt_status_collect(s);
+   if (all && s->change.nr && s->workdir_dirty)
+   die("Cannot use '-a' with staged and unstaged changes");
+
return argc;
 }
 

Note that this may run the diff-index and diff-files procedures twice
(once here, and once later if we actually call run_status). If were
doing this for real (and I do not think it is something we want to take
upstream anyway), you would want to make sure that information was
cached.

-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

Fwd: Delivery Status Notification (Failure)

[Drew Gross]

Hello,

I'm am trying to patch git to refuse to commit if there are both staged and
unstaged changes, and I pass the -a flag. I expected this simple addition
to parse_and_validate_options() in commit.c to accomplish most of what I
want:

if (all && s->workdir_dirty)
die(_("Cannot commit with -a if there are staged and unstaged changes"));

But when compiled, this will commit anyway even with staged and unstaged
files. I think I misunderstanding the workdir_dirty flag. Can someone help
me?

Drew Gross
--
To unsubscribe from this list: send the line "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 7/7] remote-bzr: add utf-8 support for pushing

[Felipe Contreras]

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-bzr |  6 ++
 contrib/remote-helpers/test-bzr.sh| 31 +++
 2 files changed, 37 insertions(+)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index 0bd0759..fad4a48 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -512,6 +512,11 @@ class CustomTree():
 def get_symlink_target(self, file_id):
 return self.updates[file_id]['data']
 
+def c_style_unescape(string):
+if string[0] == string[-1] == '"':
+return string.decode('string-escape')[1:-1]
+return string
+
 def parse_commit(parser):
 global marks, blob_marks, bmarks, parsed_refs
 global mode
@@ -551,6 +556,7 @@ def parse_commit(parser):
 f = { 'deleted' : True }
 else:
 die('Unknown file command: %s' % line)
+path = c_style_unescape(path).decode('utf-8')
 files[path] = f
 
 repo = parser.repo
diff --git a/contrib/remote-helpers/test-bzr.sh 
b/contrib/remote-helpers/test-bzr.sh
index e468079..f4c7768 100755
--- a/contrib/remote-helpers/test-bzr.sh
+++ b/contrib/remote-helpers/test-bzr.sh
@@ -190,4 +190,35 @@ test_expect_success 'fetch utf-8 filenames' '
   test_cmp expected actual
 '
 
+test_expect_success 'push utf-8 filenames' '
+  mkdir -p tmp && cd tmp &&
+  test_when_finished "cd .. && rm -rf tmp && LC_ALL=C" &&
+
+  export LC_ALL=en_US.UTF-8
+
+  (
+  bzr init bzrrepo &&
+  cd bzrrepo &&
+
+  echo one >> content &&
+  bzr add content &&
+  bzr commit -m one
+  ) &&
+
+  (
+  git clone "bzr::$PWD/bzrrepo" gitrepo &&
+  cd gitrepo &&
+
+  echo test >> "áéíóú" &&
+  git add "áéíóú" &&
+  git commit -m utf-8 &&
+
+  git push
+  ) &&
+
+  (cd bzrrepo && bzr ls > ../actual) &&
+  echo -e "content\náéíóú" > expected &&
+  test_cmp expected actual
+'
+
 test_done
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/7] remote-bzr: add utf-8 support for fetching

[Felipe Contreras]

From: Timotheus Pokorra 

[fc: added tests]

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-bzr |  4 ++--
 contrib/remote-helpers/test-bzr.sh| 25 +
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index 0bcf8c5..0bd0759 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -223,7 +223,7 @@ def export_files(tree, files):
 # is the blog already exported?
 if h in filenodes:
 mark = filenodes[h]
-final.append((mode, mark, path))
+final.append((mode, mark, path.encode('utf-8')))
 continue
 
 d = tree.get_file_text(fid)
@@ -240,7 +240,7 @@ def export_files(tree, files):
 print "data %d" % len(d)
 print d
 
-final.append((mode, mark, path))
+final.append((mode, mark, path.encode('utf-8')))
 
 return final
 
diff --git a/contrib/remote-helpers/test-bzr.sh 
b/contrib/remote-helpers/test-bzr.sh
index 68105fc..e468079 100755
--- a/contrib/remote-helpers/test-bzr.sh
+++ b/contrib/remote-helpers/test-bzr.sh
@@ -165,4 +165,29 @@ test_expect_success 'different authors' '
   test_cmp expected actual
 '
 
+test_expect_success 'fetch utf-8 filenames' '
+  mkdir -p tmp && cd tmp &&
+  test_when_finished "cd .. && rm -rf tmp && LC_ALL=C" &&
+
+  export LC_ALL=en_US.UTF-8
+
+  (
+  bzr init bzrrepo &&
+  cd bzrrepo &&
+
+  echo test >> "áéíóú" &&
+  bzr add "áéíóú" &&
+  bzr commit -m utf-8
+  ) &&
+
+  (
+  git clone "bzr::$PWD/bzrrepo" gitrepo &&
+  cd gitrepo &&
+  git ls-files > ../actual
+  ) &&
+
+  echo "\"\\303\\241\\303\\251\\303\\255\\303\\263\\303\\272\"" > expected &&
+  test_cmp expected actual
+'
+
 test_done
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/7] remote-bzr: avoid unreferred tags

[Felipe Contreras]

They have no content, there's nothing we can do with them.

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-bzr | 4 
 1 file changed, 4 insertions(+)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index 9466cb9..0bcf8c5 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -654,7 +654,11 @@ def do_capabilities(parser):
 def do_list(parser):
 global tags
 print "? refs/heads/%s" % 'master'
+
+history = parser.repo.revision_history()
 for tag, revid in parser.repo.tags.get_tag_dict().items():
+if revid not in history:
+continue
 print "? refs/tags/%s" % tag
 tags[tag] = revid
 print "@refs/heads/%s HEAD" % 'master'
-- 
1.8.2

--
To unsubscribe from this list: send the line "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 4/7] remote-bzr: only update workingtree on local repos

[Felipe Contreras]

Apparently, that's the only way it's possible.

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-bzr | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index a99a924..9466cb9 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -630,10 +630,9 @@ def do_export(parser):
 peer.import_last_revision_info_and_tags(repo, revno, revid)
 else:
 peer.import_last_revision_info(repo.repository, revno, 
revid)
-wt = peer.bzrdir.open_workingtree()
 else:
 wt = repo.bzrdir.open_workingtree()
-wt.update()
+wt.update()
 print "ok %s" % ref
 print
 
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/7] remote-bzr: set author if available

[Felipe Contreras]

From: David Engster 

[fc: added tests]

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-bzr |  7 ++-
 contrib/remote-helpers/test-bzr.sh| 15 +++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index f818e93..a99a924 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -266,7 +266,12 @@ def export_branch(branch, name):
 tz = rev.timezone
 committer = rev.committer.encode('utf-8')
 committer = "%s %u %s" % (fixup_user(committer), time, gittz(tz))
-author = committer
+authors = rev.get_apparent_authors()
+if authors:
+author = authors[0].encode('utf-8')
+author = "%s %u %s" % (fixup_user(author), time, gittz(tz))
+else:
+author = committer
 msg = rev.message.encode('utf-8')
 
 msg += '\n'
diff --git a/contrib/remote-helpers/test-bzr.sh 
b/contrib/remote-helpers/test-bzr.sh
index d26e5c7..68105fc 100755
--- a/contrib/remote-helpers/test-bzr.sh
+++ b/contrib/remote-helpers/test-bzr.sh
@@ -150,4 +150,19 @@ test_expect_success 'moving directory' '
   test_cmp expected actual
 '
 
+test_expect_success 'different authors' '
+  (cd bzrrepo &&
+  echo john >> content &&
+  bzr commit -m john \
+--author "Jane Rey " \
+--author "John Doe ") &&
+
+  (cd gitrepo &&
+  git pull &&
+  git show --format="%an <%ae>, %cn <%ce>" --quiet > ../actual) &&
+
+  echo "Jane Rey , A U Thor " > expected 
&&
+  test_cmp expected actual
+'
+
 test_done
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/7] remote-bzr: remove files before modifications

[Felipe Contreras]

From: Christophe Simonis 

Allow re-add of a deleted file in the same commit.

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-bzr | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index a7d041b..f818e93 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -303,10 +303,10 @@ def export_branch(branch, name):
 else:
 print "merge :%s" % m
 
+for f in removed:
+print "D %s" % (f,)
 for f in modified_final:
 print "M %s :%u %s" % f
-for f in removed:
-print "D %s" % (f)
 print
 
 count += 1
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] remote-bzr: fix directory renaming

[Felipe Contreras]

From: Christophe Simonis 

Git does not handle directories, renaming a directory is renaming every
files in this directory.

[fc: added tests]

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-bzr |  8 +++-
 contrib/remote-helpers/test-bzr.sh| 24 
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index c5822e4..a7d041b 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -191,7 +191,13 @@ def get_filechanges(cur, prev):
 modified[path] = fid
 for oldpath, newpath, fid, kind, mod, _ in changes.renamed:
 removed[oldpath] = None
-modified[newpath] = fid
+if kind == 'directory':
+lst = cur.list_files(from_dir=newpath, recursive=True)
+for path, file_class, kind, fid, entry in lst:
+if kind != 'directory':
+modified[newpath + '/' + path] = fid
+else:
+modified[newpath] = fid
 
 return modified, removed
 
diff --git a/contrib/remote-helpers/test-bzr.sh 
b/contrib/remote-helpers/test-bzr.sh
index 8450432..d26e5c7 100755
--- a/contrib/remote-helpers/test-bzr.sh
+++ b/contrib/remote-helpers/test-bzr.sh
@@ -126,4 +126,28 @@ test_expect_success 'special modes' '
   test_cmp expected actual
 '
 
+cat > expected < movedir/one &&
+  echo two > movedir/two &&
+  bzr add movedir &&
+  bzr commit -m movedir &&
+  bzr mv movedir movedir-new &&
+  bzr commit -m movedir-new) &&
+
+  (cd gitrepo &&
+  git pull &&
+  git ls-tree HEAD > ../actual) &&
+
+  test_cmp expected actual
+'
+
 test_done
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/7] remote-bzr: generic updates

[Felipe Contreras]

Hi,

Here are a couple fixes for remote-bzr, some of these can be really annoying to
certain users. I believe all of them should be safe.

Christophe Simonis (2):
  remote-bzr: fix directory renaming
  remote-bzr: remove files before modifications

David Engster (1):
  remote-bzr: set author if available

Felipe Contreras (3):
  remote-bzr: only update workingtree on local repos
  remote-bzr: avoid unreferred tags
  remote-bzr: add utf-8 support for pushing

Timotheus Pokorra (1):
  remote-bzr: add utf-8 support for fetching

 contrib/remote-helpers/git-remote-bzr | 36 ++---
 contrib/remote-helpers/test-bzr.sh| 95 +++
 2 files changed, 123 insertions(+), 8 deletions(-)

-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe 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 00/13] remote-hg: general updates

[Felipe Contreras]

On Fri, Apr 5, 2013 at 7:28 PM, Junio C Hamano  wrote:

> A tool that is in contrib/ follows the contrib/README rule.
>
> I do not maintain it. Maintenance is up to the person who asked to
> include it there.  I do ask the people who propose to add something
> in contrib/ to promise that they arrange it to be maintained.

That's true, but I meant that you are the gatekeeper. Ultimately you
decide which patches go in. If Max, or anybody else, wants a patch
into contrib, you can get it in, even if I disagree with it. You can
also decide that somebody from gitifyhg might be a more suitable
person as a maintainer of remote-hg.

Either way, I think if things go well, remote-hg will prove it's worth
and move out of contrib and into git's core.

-- 
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 00/13] remote-hg: general updates

[Junio C Hamano]

Max Horn  writes:

> OK, I'll try to keep a professional tone from now on :-).
>
> Please consider that the willingness of people to collaborate with
> you in any way is directly related to how you treat them. That
> includes bug reports. The way you acted towards Jed, who was very
> calmly and matter-of-factly explaining things, was IMHO completely
> inappropriate and unacceptable. Indeed, I should augment my list
> of reasons why people might not want to contribute to remote-hg by
> one major bullet point: You. And please, don't feel to compelled
> to tell us that Junio is really the maintainer of remote-hg and
> not you: Whether this is true or not doesn't matter for this
> point.

Only on this point, as the top-level maintainer.  I do not have any
opinion on technical merits between the two Hg gateways myself.

A tool that is in contrib/ follows the contrib/README rule.

I do not maintain it. Maintenance is up to the person who asked to
include it there.  I do ask the people who propose to add something
in contrib/ to promise that they arrange it to be maintained.

I do not even guarantee that they are the best in the breed in their
respective category. When something is added to contrib/, others can
raise objections by proposing alternatives, by arguing that tools of
the nature are better kept out of my tree, etc.  When remote-hg was
added, I didn't see specific objections against it.

There is one generic objection to adding anything new in contrib/ I
have myself, though.

In early days of Git, almost all users, who might be interested in
improving their Git experience by helping to polish third-party
tools, had clones of my tree and did not hesitate to come to this
list. Back then, having a copy of an emerging third-party tool in my
tree in contrib/ was a good way to give more exposure to it, and to
give those interested in it a place to meet and join forces to
improve it. Because Git population was small, almost everybody was
here, and it was an efficient distribution mechanism.

Git is now reasonably well known and has big enough user base, and
many users, even those who are inclined to help improving their Git
experience by contributing to third-party tools, do not necessarily
have a clone of my tree.  A third-party tool around Git, if it is
any good, is likely to have much much better chance to thrive as a
free-standing project with its own community, compared to those
early days.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] http-backend: respect GIT_NAMESPACE with dumb clients

[John Koleszar]

On Thu, Apr 4, 2013 at 10:43 PM, Jeff King  wrote:
>
> On Thu, Apr 04, 2013 at 10:34:49PM -0700, Junio C Hamano wrote:
>
> > > +static void get_head(char *arg)
> > > +{
> > > +   struct strbuf buf = STRBUF_INIT;
> > > +   head_ref_namespaced(show_text_ref, &buf);
> > > +   send_strbuf("text/plain", &buf);
> > > +   strbuf_release(&buf);
> > > +}
> >
> > You identified the right place to patch, but I think we need a bit
> > more than this.
> >
> > The show_text_ref() function gives "SHA-1  refname". It is
> > likely that the dumb client will ignore the trailing part of that
> > output, but let's avoid a hack that we would not want see other
> > implementations imitate.
>
> Oh, right. I was thinking too much about normal clients which see HEAD
> in the ref advertisement; of course the dumb client is expecting to see
> the actual HEAD file.
>
> > One advantage dumb clients has over smart ones is that they can read
> > HEAD that is a textual symref from a dumb server and learn which
> > branch is the default one (remote.c::guess_remote_head()) without
> > guessing.  I think this function should:
> >
> >  - Turn "HEAD" into a namespaced equivalent;
> >
> >  - Run resolve_ref() on the result of the above;
> >
> >  - Is it a symbolic ref?
> >
> >. If it is, then format "ref: \n" into a strbuf and send
> >  it (make sure  is without the namespace prefix);
> >
> >. Otherwise, HEAD is detached. Prepare "%s\n" % sha1_to_hex(sha1),
> >  and send it.
>
> Yes, that sounds right; it is basically just reconstructing a HEAD
> file. What do the HEADs inside namespaces look like? Do they refer to
> full global refs, or do they refer to refs within the namespace?
>
> If the latter, we could just send the HEAD file directly. But I suspect
> it is the former, so that they can function when non-namespaced commands
> are used.
>

Here's a quick cut at this. Seems to work ok in local testing, I
haven't updated the test suite yet. If the namespaced HEAD is a
symbolic ref, its target must have the namespace prefix applied, or
the resolved ref will be from outside the namespace (eg
refs/heads/master vs refs/namespace/ns/refs/heads/master). This seems
to be handled at write time, not sure if we need to do more
verification here or not.

diff --git a/http-backend.c b/http-backend.c
index d32128f..da4482c 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -404,13 +404,40 @@ static void get_info_refs(char *arg)

} else {
select_getanyfile();
-   head_ref_namespaced(show_text_ref, &buf);
for_each_namespaced_ref(show_text_ref, &buf);
send_strbuf("text/plain", &buf);
}
strbuf_release(&buf);
 }

+static int show_head_ref(const char *name, const unsigned char *sha1,
+   int flag, void *cb_data)
+{
+   struct strbuf *buf = cb_data;
+
+   if (flag & REF_ISSYMREF) {
+   unsigned char sha1[20];
+   const char *target = resolve_ref_unsafe(name, sha1, 1, NULL);
+   const char *target_nons = strip_namespace(target);
+
+   strbuf_addf(buf, "ref: %s\n", target_nons);
+   } else {
+   strbuf_addf(buf, "%s\n", sha1_to_hex(sha1));
+   }
+
+   return 0;
+}
+
+static void get_head(char *arg)
+{
+   struct strbuf buf = STRBUF_INIT;
+
+   select_getanyfile();
+   head_ref_namespaced(show_head_ref, &buf);
+   send_strbuf("text/plain", &buf);
+   strbuf_release(&buf);
+}
+
 static void get_info_packs(char *arg)
 {
size_t objdirlen = strlen(get_object_directory());
--
To unsubscribe from this list: send the line "unsubscribe 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 00/13] remote-hg: general updates

[Felipe Contreras]

On Fri, Apr 5, 2013 at 4:30 PM, Max Horn  wrote:

> On 04.04.2013, at 08:42, Felipe Contreras wrote:

> Please consider that the willingness of people to collaborate with you in any 
> way is directly related to how you treat them. That includes bug reports. The 
> way you acted towards Jed, who was very calmly and matter-of-factly 
> explaining things, was IMHO completely inappropriate and unacceptable

He did not provide any bug report at all, also, he was not willing to
collaborate, as he clearly stated; he won't work git-hg bridges no
more.

> Indeed, I should augment my list of reasons why people might not want to 
> contribute to remote-hg by one major bullet point: You. And please, don't 
> feel to compelled to tell us that Junio is really the maintainer of remote-hg 
> and not you: Whether this is true or not doesn't matter for this point.

Oh but it does. The only reason my remote-hg managed to land in the
mainline is because I showed it was superior to the other
remote-hg[1], and that there really was no point in trying to salvage
the other code base. Maybe this turns out to be a mistake, and the
other remote-hg manages to improve and surpass my remote-hg, but it
seems rather unlikely at this point.

Similarly, you could *show* that gitifyhg is superior than remote-hg,
why it makes sense to use that instead of trying to improve this code
base, but you don't, because you can't, because it doesn't make sense.

Ultimately this is not about people, this is about the code. A
sensible person that is not emotionally attached to any code, would
simply look at the code, and if what you claim is true (that remote-hg
has many issues), this sensible person would start cherry picking
patches from gitifyhg, send them to the git mailing list explaining
why they make sense. Eventually, remote-hg in git.git would become
essentially gitifyhg (albeit better because it would have received
more review from other git developers). Codewise this would make
sense.

And this sensible person would replace me as the go-to person for
remote-hg. And he would probably have a friendly and wonderful
personality, and you can accept him as your messiah and what not. But
this wouldn't change the fact that codewise this is the best way to
proceed.

But the fact of the matter is that either a) remote-hg is perfectly
fine, or b) this sensible person doesn't exist.

>> , I find it
>> annoying that you claim to know what is important for users, as if
>> somehow knowing that gitifyhg doesn't work with the user's version of
>> mercurial (e.g. 1.8) is better than remote-hg's situation; where it
>> *actually works*, but it's not mentioned. Yeah, mentioning that it
>> doesn't work is better than working, right.
>
> I'll leave it to everybody to read what you wrote there, and contrast it with 
> the following, and draw their own conclusions...

Keep in mind that it's not me the one that claims remote-hg is
superior because it supports 1.8, as you seem try to portray. Rather,
it's _you_ the one that claims superiority because gitifyhg *mentions*
support until 1.9 (which remote-hg also supports).

It's not important who supports the most ancient version of mercurial
that nobody uses anyway, what is important is that *mentioning* or not
mentioning which ancient version of mercurial is supported doesn't
make one project superior to the other.

But perhaps this point is too subtle for you to understand, so there:
https://github.com/felipec/git/wiki/Git-remote-hg

Now it's mentioned that remote-hg too, supports up to version 1.9. Can
we agree now, that nobody has any advantage, either on version
support, or the mentioning of version support?

> Indeed, util.url was only added in 1.9.3. OK, let's work around that. Then 
> local clones work. But of course in reality, most users will want to interact 
> with a remote repository. But that's still broken:

Fixed:
https://github.com/felipec/git/commit/df0ed732b56c6c7910cc76f3e930229816e27117

And it was _you_ the one that sent the commit that broke it to the git
mailing list to be pushed.

> Right, clone() changed. And some more stuff. See 
> .

Indeed, that might be useful, but that doesn't mean remote-hg doesn't
work with 1.8; it works perfectly fine for local repositories, and all
the tests pass.

> There also was a good reason why I stopped at that point, but I don't 
> remember the details right now. And quite frankly, I have zero incentive to 
> even try to remember. But anyway, I don't think it's that useful to add 
> support for 1.8, too, since one can't get back much further anyway. And 
> upgrading to a newer Mercurial is (a) quite easy (even if you don't have 
> root, just install it into $HOME), and (b) using a newer Mercurial version is 
> a good idea for other reasons, too.

If supporting 1.8 is not useful because most people have newer
versions, then mentioning support for 1.9 doesn't give any advantage.

Re: [PATCH] show-branch: use strbuf instead of static buffer

[Jonathan Nieder]

Jeff King wrote:

> When we generate relative names (e.g., "master~20^2"), we
> format the name into a static buffer, then xstrdup the
> result to attach it to the commit. Since the first thing we
> add into the static buffer is the already-computed name of
> the child commit, the names may get longer and longer as
> the traversal gets deeper, and we may eventually overflow
> the fixed-size buffer.

Good catch.

[...]
> Though this is a stack overflow, I don't know that it's exploitable for
> anything interesting; an attacker does not get to write arbitrary data,
> but rather only a sequence of "^%d" and "~%d" relative history markers.
> Perhaps in theory one could devise a history such that the sequence
> markers spelled out some malicious code, but it would be quite a
> challenge

Overwrite the return address and return-to-libc?

[...]
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c

Very clean and obviously correct.  Thanks.

Reviewed-by: Jonathan Nieder 

A test would be nice, though.

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] glossary: extend "detached HEAD" description

[Eric Sunshine]

On Fri, Apr 5, 2013 at 11:19 AM, Junio C Hamano  wrote:
> Add a blanket description to the glossary to cover them instead.
> The general principle is that operations to update the branch work
> and affect on the HEAD, while operations to update the information

s/work and affect on the/work on and affect the/

> about a branch do not.
>
> Signed-off-by: Junio C Hamano 
--
To unsubscribe from this list: send the line "unsubscribe 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/2] perl: redirect stderr to /dev/null instead of closing

[Petr Baudis]

On Fri, Apr 05, 2013 at 11:57:19AM -0700, Junio C Hamano wrote:
> Petr Baudis  writes:
> >> > -if (defined $opts{STDERR}) {
> >> > -close STDERR;
> >> > -}
> >> >  if ($opts{STDERR}) {
> >> >  open (STDERR, '>&', $opts{STDERR})
> >
> >   I'm sorry, I don't follow. Doesn't this just break the STDERR option
> > altogether as we will try to dup2() over an already open file
> > descriptor? We do need to close STDERR if we are going to reopen it,
> > I think.
> 
> When $opts{STDERR} is 2, what the three lines the proposed patch
> removes did is actively wrong, because you dup2 the fd you just
> closed.

Indeed, though $opts{STDERR} == 2 is something weird to do, it is a case
to consider.

> When $opts{STDERR} is 1, it seems to do the right thing with or
> without the "close STDERR" in front.  Isn't this because the usual
> "open($fd, <<>>) closes $fd as necessary" applies to this
> case as well?

I never actually tried that and was always happy to go with perldoc
maxim

To (re)open "STDOUT" or "STDERR" as an in-memory file, close it first:
   close STDOUT;
   open(STDOUT, ">", \$variable)
   or die "Can't open STDOUT: $!";

but my assumption that this generalizes to other kinds of open was
apparently invalid; an example further down the page proves me wrong
completely, moreover.

  The thing is, I was confused about dup2() all along as my old UNIX
masters taught me that I must close() the original descriptor first
and since that's what's commonly done anyway, I never thought to
double-check. Now I did and I learned something new, thanks!

I guess Acked-by: Petr Baudis  then. :-)

-- 
Petr "Pasky" Baudis
For every complex problem there is an answer that is clear,
simple, and wrong.  -- H. L. Mencken
--
To unsubscribe from this list: send the line "unsubscribe 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 00/13] remote-hg: general updates

[Max Horn]

While I am not really interested in exchanging any further emails or any other 
form of communication with Felipe, as I find his vitriolic style of 
communication unbearable, I feel compelled to reply to a few points. I'll 
probably regret this... anyway, I promise this will be my last mail in this 
thread. Even though I fully expect to receive a barrage of hostile and 
aggressive replies by Felipe. So be it, /dev/null has plenty space left.


OK, I'll try to keep a professional tone from now on :-).


On 04.04.2013, at 08:42, Felipe Contreras wrote:

[...]

 * The gitifyhg test suite is run after each push on Travis CI against 
 several git / mercurial combinations [4].
 In particular, unlike all other remote-hg implementations I know, we 
 explicitly promise (and test) compatibility with a specific range of 
 Mercurial versions (not just the one the dev happens to have installed 
 right now). This has been a frequent issue for me with the msysgit 
 remote-hg
>>> 
>>> I've personally checked against multiple versions of Mercurial. It's
>>> possible that some error might slip by, but it would get quickly
>>> noticed.
>> 
>> Really? This sounds close to some people who say things like "I don't need a 
>> test suite, I personally run some tests every now and then on my machine."
> 
> Do you see any compatibility issues reported in the git mailing list,
> or my github[1]? No? KTHXBYE. There _were_ compatibility issues, and
> those got reported, and fixed, not any more.

Please consider that the willingness of people to collaborate with you in any 
way is directly related to how you treat them. That includes bug reports. The 
way you acted towards Jed, who was very calmly and matter-of-factly explaining 
things, was IMHO completely inappropriate and unacceptable. Indeed, I should 
augment my list of reasons why people might not want to contribute to remote-hg 
by one major bullet point: You. And please, don't feel to compelled to tell us 
that Junio is really the maintainer of remote-hg and not you: Whether this is 
true or not doesn't matter for this point.


> remote-hg certainly works on versions older than 1.9, again

That's plain wrong. Indeed, remote-hg is using hg interfaces that were only 
introduced in 1.9. As such, I would be quite surprised if remote-hg worked with 
older hg versions, but I don't see why I should bother to test... Hmm, wait, I 
see a reason:

> , I find it
> annoying that you claim to know what is important for users, as if
> somehow knowing that gitifyhg doesn't work with the user's version of
> mercurial (e.g. 1.8) is better than remote-hg's situation; where it
> *actually works*, but it's not mentioned. Yeah, mentioning that it
> doesn't work is better than working, right.

I'll leave it to everybody to read what you wrote there, and contrast it with 
the following, and draw their own conclusions...

The reason why I did not write what exactly is wrong with remote-hg in hg 1.8 
and older is that it is so obvious that I didn't think anybody would need 
handholding to verify it or find out the details :-). But since you "asked" for 
it:


$ hg --version
Mercurial Distributed SCM (version 1.8.4)
(see http://mercurial.selenic.com for more information)

Copyright (C) 2005-2011 Matt Mackall and others
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ git clone hg::foobar/ foobar.git
Cloning into 'foobar.git'...
Traceback (most recent call last):
  File "/Users/mhorn/bin/git-remote-hg", line 846, in 
sys.exit(main(sys.argv))
  File "/Users/mhorn/bin/git-remote-hg", line 819, in main
fix_path(alias, peer or repo, url)
  File "/Users/mhorn/bin/git-remote-hg", line 765, in fix_path
repo_url = util.url(repo.url())
AttributeError: 'module' object has no attribute 'url'

Indeed, util.url was only added in 1.9.3. OK, let's work around that. Then 
local clones work. But of course in reality, most users will want to interact 
with a remote repository. But that's still broken:

$ git clone hg::ssh://h...@bitbucket.org/fingolfin/test-gitifyhg
Cloning into 'test-gitifyhg'...
Traceback (most recent call last):
  File "/Users/mhorn/bin/git-remote-hg", line 1138, in 
sys.exit(main(sys.argv))
  File "/Users/mhorn/bin/git-remote-hg", line 1107, in main
repo = get_repo(url, alias)
  File "/Users/mhorn/bin/git-remote-hg", line 284, in get_repo
peer, dstpeer = hg.clone(myui, {}, url, local_path, update=False, pull=True)
TypeError: clone() got multiple values for keyword argument 'pull'


Right, clone() changed. And some more stuff. See 
.
 

There also was a good reason why I stopped at that point, but I don't remember 
the details right now. And quite frankly, I have zero incentive to even try to 
remember. But anyway, I don't think it's that useful to add support for

Re: [RFC/PATH 3/4] remote-hg: add version checks to the marks

[Felipe Contreras]

On Fri, Apr 5, 2013 at 6:46 AM, Jed Brown  wrote:
> Felipe Contreras  writes:
>
>> @@ -76,12 +78,19 @@ class Marks:
>>
>>  def __init__(self, path):
>>  self.path = path
>> +self.clear()
>> +self.load()
>> +
>> +if self.version < VERSION:
>> +self.clear()
>
> It's friendlier to just upgrade the marks in-place. This takes less than
> one second to run on repositories where full re-import would take half
> an hour:

Yeah, but that's riskier, and only works for this particular version.
Besides, it only happens one time per repository at best. Also, it
would mess up the current organization of the code. I'm not sure it's
worth worrying about it at this point, and we could always add a patch
on top.

-- 
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

What's cooking in git.git (Apr 2013, #02; Fri, 5)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.

A handful of topics that have been stalled for quite a while have
been discarded; for those that are not superseded by something else,
interested parties can still resubmit a reroll, but without any
advances, we do not get any benefit from carrying them in my tree.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* bk/document-commit-tree-S (2013-03-25) 1 commit
  (merged to 'next' on 2013-03-26 at 8ee205f)
 + commit-tree: document -S option consistently


* jc/apply-ws-fix-tab-in-indent (2013-03-29) 2 commits
  (merged to 'next' on 2013-03-29 at 26eb6e9)
 + test: resurrect q_to_tab
  (merged to 'next' on 2013-03-26 at 46c6bda)
 + apply --whitespace=fix: avoid running over the postimage buffer

 "git apply --whitespace=fix" was not prepared to see a line getting
 longer after fixing whitespaces (e.g. tab-in-indent aka Python).


* jc/directory-attrs-regression-fix (2013-03-28) 6 commits
  (merged to 'next' on 2013-03-29 at a3dce2b)
 + t: check that a pattern without trailing slash matches a directory
 + dir.c::match_pathname(): pay attention to the length of string parameters
 + dir.c::match_pathname(): adjust patternlen when shifting pattern
 + dir.c::match_basename(): pay attention to the length of string parameters
 + attr.c::path_matches(): special case paths that end with a slash
 + attr.c::path_matches(): the basename is part of the pathname

 Fix 1.8.1.x regression that stopped matching "dir" (without
 trailing slash) to a directory "dir".


* jc/merge-tag-object (2013-04-01) 3 commits
  (merged to 'next' on 2013-04-03 at 94b5c7d)
 + t6200: test message for merging of an annotated tag
 + t6200: use test_config/test_unconfig
  (merged to 'next' on 2013-03-29 at aeec39c)
 + merge: a random object may not necssarily be a commit

 "git merge $(git rev-parse v1.8.2)" behaved quite differently from
 "git merge v1.8.2" as if v1.8.2 were written as v1.8.2^0 and did
 not pay much attention to the annotated tag payload.

 This makes the code notice the type of the tag object, in addition
 to the dwim_ref() based classification the current code uses
 (i.e. the name appears in refs/tags/) to decide when to special
 case merging of tags.


* jc/sha1-name-object-peeler (2013-03-31) 2 commits
  (merged to 'next' on 2013-04-01 at cdb4a18)
 + peel_onion(): teach $foo^{object} peeler
 + peel_onion: disambiguate to favor tree-ish when we know we want a tree-ish
 (this branch is used by mh/rev-parse-verify-doc.)

 There was no good way to ask "I have a random string that came from
 outside world. I want to turn it into a 40-hex object name while
 making sure such an object exists".  A new peeling suffix ^{object}
 can be used for that purpose, together with "rev-parse --verify".


* jc/t5516-pushInsteadOf-vs-pushURL (2013-03-28) 1 commit
  (merged to 'next' on 2013-04-01 at bed2879)
 + t5516: test interaction between pushURL and pushInsteadOf correctly

 Update a test to match the documented interaction between pushURL
 and pushInsteadOf.


* jk/check-corrupt-objects-carefully (2013-03-29) 10 commits
  (merged to 'next' on 2013-03-29 at b6a04a7)
 + clone: leave repo in place after checkout errors
 + clone: run check_everything_connected
 + clone: die on errors from unpack_trees
 + add tests for cloning corrupted repositories
 + streaming_write_entry: propagate streaming errors
 + add test for streaming corrupt blobs
 + avoid infinite loop in read_istream_loose
 + read_istream_filtered: propagate read error from upstream
 + check_sha1_signature: check return value from read_istream
 + stream_blob_to_fd: detect errors reading from stream

 Have the streaming interface and other codepaths more carefully
 examine for corrupt objects.


* jk/config-with-empty-section (2013-03-29) 1 commit
  (merged to 'next' on 2013-04-01 at 7972aa9)
 + t1300: document some aesthetic failures of the config editor

 Document that "git config --unset" does not remove an empty section
 head after removing the last variable in a section, and adding a
 new variable does not try to reuse a leftover empty section head.


* jk/difftool-no-overwrite-on-copyback (2013-03-29) 5 commits
  (merged to 'next' on 2013-03-29 at 9f42d34)
 + t7800: run --dir-diff tests with and without symlinks
 + t7800: fix tests when difftool uses --no-symlinks
 + t7800: don't hide grep output
 + difftool: don't overwrite modified files
 + t7800: move '--symlinks' specific test to the end

 Try to be careful when difftool backend allows the user to write
 into the temporary files being shown *and* the user makes changes
 to the working tree at the same time. One of the changes has to be
 lost in such a case, but at least tell the us

[PATCH 9/9] http: drop http_error function

[Jeff King]

This function is a single-liner and is only called from one
place. Just inline it, which makes the code more obvious.

Signed-off-by: Jeff King 
---
This is a cleanup made possible by the previous patch.

 http-push.c | 2 +-
 http.c  | 5 -
 http.h  | 5 -
 3 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/http-push.c b/http-push.c
index 439a555..395a8cf 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1551,7 +1551,7 @@ static int remote_exists(const char *path)
ret = 0;
break;
case HTTP_ERROR:
-   http_error(url);
+   error("unable to access '%s': %s", url, curl_errorstr);
default:
ret = -1;
}
diff --git a/http.c b/http.c
index 64068a2..58c063c 100644
--- a/http.c
+++ b/http.c
@@ -941,11 +941,6 @@ cleanup:
return ret;
 }
 
-void http_error(const char *url)
-{
-   error("unable to access '%s': %s", url, curl_errorstr);
-}
-
 int http_fetch_ref(const char *base, struct ref *ref)
 {
char *url;
diff --git a/http.h b/http.h
index fa65128..4dc5353 100644
--- a/http.h
+++ b/http.h
@@ -135,11 +135,6 @@ int http_get_strbuf(const char *url, struct strbuf 
*content_type, struct strbuf
  */
 int http_get_strbuf(const char *url, struct strbuf *content_type, struct 
strbuf *result, int options);
 
-/*
- * Prints an error message using error() containing url and curl_errorstr.
- */
-void http_error(const char *url);
-
 extern int http_fetch_ref(const char *base, struct ref *ref);
 
 /* Helpers for fetching packs */
-- 
1.8.2.rc0.33.gd915649
--
To unsubscribe from this list: send the line "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 8/9] remote-curl: die directly with http error messages

[Jeff King]

When we encounter an unknown http error (e.g., a 403), we
hand the error code to http_error, which then prints it with
error(). After that we die with the redundant message "HTTP
request failed".

Instead, let's just drop http_error entirely, which does
nothing but pass arguments to error(), and instead die
directly with a useful message.

So before:

  $ git clone https://example.com/repo.git
  Cloning into 'repo'...
  error: unable to access 'https://example.com/repo.git': The requested URL 
returned error: 403 Forbidden
  fatal: HTTP request failed

and after:

  $ git clone https://example.com/repo.git
  Cloning into 'repo'...
  fatal: unable to access 'https://example.com/repo.git': The requested URL 
returned error: 403 Forbidden

Signed-off-by: Jeff King 
---
 remote-curl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 9abe4b7..60eda63 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -216,8 +216,7 @@ static struct discovery* discover_refs(const char *service, 
int for_push)
die("Authentication failed for '%s'", url);
default:
show_http_message(&type, &buffer);
-   http_error(url);
-   die("HTTP request failed");
+   die("unable to access '%s': %s", url, curl_errorstr);
}
 
last= xcalloc(1, sizeof(*last_discovery));
-- 
1.8.2.rc0.33.gd915649

--
To unsubscribe from this list: send the line "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 7/9] http: re-word http error message

[Jeff King]

When we report an http error code, we say something like:

  error: The requested URL reported failure: 403 Forbidden while accessing 
http://example.com/repo.git

Everything between "error:" and "while" is written by curl,
and the resulting sentence is hard to read (especially
because there is no punctuation between curl's sentence and
the remainder of ours). Instead, let's re-order this to give
better flow:

  error: unable to access 'http://example.com/repo.git: The requested URL 
reported failure: 403 Forbidden

This is still annoyingly long, but at least reads more
clearly left to right.

Signed-off-by: Jeff King 
---
As I mentioned in the cover letter, I'm still not that excited about the
"after" result here. Suggestions welcome.

 http.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 5e6f67d..64068a2 100644
--- a/http.c
+++ b/http.c
@@ -943,7 +943,7 @@ void http_error(const char *url)
 
 void http_error(const char *url)
 {
-   error("%s while accessing %s", curl_errorstr, url);
+   error("unable to access '%s': %s", url, curl_errorstr);
 }
 
 int http_fetch_ref(const char *base, struct ref *ref)
-- 
1.8.2.rc0.33.gd915649

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/9] http: simplify http_error helper function

[Jeff King]

This helper function should really be a one-liner that
prints an error message, but it has ended up unnecessarily
complicated:

  1. We call error() directly when we fail to start the curl
 request, so we must later avoid printing a duplicate
 error in http_error().

 It would be much simpler in this case to just stuff the
 error message into our usual curl_errorstr buffer
 rather than printing it ourselves. This means that
 http_error does not even have to care about curl's exit
 value (the interesting part is in the errorstr buffer
 already).

  2. We return the "ret" value passed in to us, but none of
 the callers actually cares about our return value. We
 can just drop this entirely.

Signed-off-by: Jeff King 
---
 http-push.c   |  2 +-
 http.c| 11 ---
 http.h|  5 ++---
 remote-curl.c |  2 +-
 4 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/http-push.c b/http-push.c
index bd66f6a..439a555 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1551,7 +1551,7 @@ static int remote_exists(const char *path)
ret = 0;
break;
case HTTP_ERROR:
-   http_error(url, HTTP_ERROR);
+   http_error(url);
default:
ret = -1;
}
diff --git a/http.c b/http.c
index 45cc7c7..5e6f67d 100644
--- a/http.c
+++ b/http.c
@@ -857,7 +857,8 @@ static int http_request(const char *url, struct strbuf 
*type,
run_active_slot(slot);
ret = handle_curl_result(&results);
} else {
-   error("Unable to start HTTP request for %s", url);
+   snprintf(curl_errorstr, sizeof(curl_errorstr),
+"failed to start HTTP request");
ret = HTTP_START_FAILED;
}
 
@@ -940,13 +941,9 @@ int http_error(const char *url, int ret)
return ret;
 }
 
-int http_error(const char *url, int ret)
+void http_error(const char *url)
 {
-   /* http_request has already handled HTTP_START_FAILED. */
-   if (ret != HTTP_START_FAILED)
-   error("%s while accessing %s", curl_errorstr, url);
-
-   return ret;
+   error("%s while accessing %s", curl_errorstr, url);
 }
 
 int http_fetch_ref(const char *base, struct ref *ref)
diff --git a/http.h b/http.h
index 0fe54f4..fa65128 100644
--- a/http.h
+++ b/http.h
@@ -136,10 +136,9 @@ int http_get_strbuf(const char *url, struct strbuf 
*content_type, struct strbuf
 int http_get_strbuf(const char *url, struct strbuf *content_type, struct 
strbuf *result, int options);
 
 /*
- * Prints an error message using error() containing url and curl_errorstr,
- * and returns ret.
+ * Prints an error message using error() containing url and curl_errorstr.
  */
-int http_error(const char *url, int ret);
+void http_error(const char *url);
 
 extern int http_fetch_ref(const char *base, struct ref *ref);
 
diff --git a/remote-curl.c b/remote-curl.c
index 6c6714b..9abe4b7 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -216,7 +216,7 @@ static struct discovery* discover_refs(const char *service, 
int for_push)
die("Authentication failed for '%s'", url);
default:
show_http_message(&type, &buffer);
-   http_error(url, http_ret);
+   http_error(url);
die("HTTP request failed");
}
 
-- 
1.8.2.rc0.33.gd915649

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/9] remote-curl: consistently report repo url for http errors

[Jeff King]

When we report http errors in fetching the initial ref
advertisement, we show the full URL we attempted to use,
including "info/refs?service=git-upload-pack". While this
may be useful for debugging a broken server, it is
unnecessarily verbose and confusing for most cases, in which
the client user is not even the same person as the owner of
the repository.

Let's just show the repository URL; debugging can happen
with GIT_CURL_VERBOSE, which shows way more useful
information, anyway.

At the same time, let's also make sure to mention the
repository URL when we report failed authentication
(previously we said only "Authentication failed"). Knowing
the URL can help the user realize why authentication failed
(e.g., they meant to push to remote A, not remote B).

Signed-off-by: Jeff King 
---
This is the same rationale as the latter half of the last patch; if we
take them all, I'd be happy to see them squashed together.

 remote-curl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 5d9f961..6c6714b 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -213,10 +213,10 @@ static struct discovery* discover_refs(const char 
*service, int for_push)
die("repository '%s' not found", url);
case HTTP_NOAUTH:
show_http_message(&type, &buffer);
-   die("Authentication failed");
+   die("Authentication failed for '%s'", url);
default:
show_http_message(&type, &buffer);
-   http_error(refs_url, http_ret);
+   http_error(url, http_ret);
die("HTTP request failed");
}
 
-- 
1.8.2.rc0.33.gd915649

--
To unsubscribe from this list: send the line "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 4/9] remote-curl: always show friendlier 404 message

[Jeff King]

When we get an http 404 trying to get the initial list of
refs from the server, we try to be helpful and remind the
user that update-server-info may need to be run. This looks
like:

  $ git clone https://github.com/non/existent
  Cloning into 'existent'...
  fatal: https://github.com/non/existent/info/refs?service=git-upload-pack not 
found: did you run git update-server-info on the server?

Suggesting update-server-info may be a good suggestion for
users who are in control of the server repo and who are
planning to set up dumb http. But for users of smart http,
and especially users who are not in control of the server
repo, the advice is useless and confusing.

Since most people are expected to use smart http these days,
it does not make sense to keep the update-server-info hint.

We not only drop the mention of update-server-info, but also
show only the main repo URL, not the full "info/refs" and
service parameter. These elements may be useful for
debugging a broken server configuration, but in the majority
of cases, users are not fetching from their own
repositories, but rather from other people's repositories;
they have neither the power nor interest to fix a broken
configuration, and the extra components just make the
message more confusing. Users who do want to debug can and
should use GIT_CURL_VERBOSE to get more complete information
on the actual URLs visited.

Signed-off-by: Jeff King 
---
This is obviously a more stringent version of the last patch, and could
just replace it. I think the last one is a no-brainer, because it lets
well-configured sites replace these messages. This one is less obvious,
because we may be hitting some random poorly configured server that
somebody is trying to set up.

Still, if you think about the number of people fetching (against any
server) versus the number of people configuring new servers, I think the
advice to run update-server-info has a vanishingly small chance of being
useful (and IMHO is misleading if you do not know how dumb-http works,
as it makes you think the server is misconfigured, when it is much more
likely to be a user error).

 remote-curl.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 083efdf..5d9f961 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -209,10 +209,8 @@ static struct discovery* discover_refs(const char 
*service, int for_push)
case HTTP_OK:
break;
case HTTP_MISSING_TARGET:
-   if (!show_http_message(&type, &buffer))
-   die("repository '%s' not found", url);
-   die("%s not found: did you run git update-server-info on the"
-   " server?", refs_url);
+   show_http_message(&type, &buffer);
+   die("repository '%s' not found", url);
case HTTP_NOAUTH:
show_http_message(&type, &buffer);
die("Authentication failed");
-- 
1.8.2.rc0.33.gd915649

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/9] remote-curl: let servers override http 404 advice

[Jeff King]

When we get an http 404 trying to get the initial list of
refs from the server, we try to be helpful and remind the
user that update-server-info may need to be run. This looks
like:

  $ git clone https://github.com/non/existent
  Cloning into 'existent'...
  fatal: https://github.com/non/existent/info/refs?service=git-upload-pack not 
found: did you run git update-server-info on the server?

Suggesting update-server-info may be a good suggestion for
users who are in control of the server repo and who are
planning to set up dumb http. But for users of smart http,
and especially users who are not in control of the server
repo, the advice is useless and confusing.

The previous patch taught remote-curl to show custom advice
from the server when it is available. When we have shown
messages from the server, we can also drop our custom
advice; what the server has to say is likely to be more
accurate and helpful.

We not only drop the mention of update-server-info, but also
show only the main repo URL, not the full "info/refs" and
service parameter. These elements may be useful for
debugging a broken server configuration, but again, anything
the server has provided is likely to be more useful (and one
can still use GIT_CURL_VERBOSE to get much more complete
debugging information).

Signed-off-by: Jeff King 
---
 remote-curl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 4fea94f..083efdf 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -209,7 +209,8 @@ static struct discovery* discover_refs(const char *service, 
int for_push)
case HTTP_OK:
break;
case HTTP_MISSING_TARGET:
-   show_http_message(&type, &buffer);
+   if (!show_http_message(&type, &buffer))
+   die("repository '%s' not found", url);
die("%s not found: did you run git update-server-info on the"
" server?", refs_url);
case HTTP_NOAUTH:
-- 
1.8.2.rc0.33.gd915649

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/9] remote-curl: show server content on http errors

[Jeff King]

If an http request to a remote git server fails, we show
only the http response code, or sometimes a custom message
for particular codes. This gives the server no opportunity
to offer a more detailed explanation of the reason for the
failure, or to give extra advice.

This patch teaches remote-curl to record and display the
body content of a failed http response. We only display such
responses when the content-type is advertised as text/plain,
as it is the most likely to look presentable on the user's
terminal (and it is hoped to be a good indication that the
message is intended for git clients, and not for a web
browser).

Each line of the new output is prepended with "remote:".
Example output may look like this (assuming the server is
configured to display such a helpful message):

  $ GIT_SMART_HTTP=0 git clone https://example.com/some/repo.git
  Cloning into 'repo'...
  remote: Sorry, fetching via dumb http is forbidden.
  remote: Please upgrade your git client to v1.6.6 or greater
  remote: and make sure that smart-http is enabled.
  error: The requested URL returned error: 403 while accessing 
http://localhost:5001/some/repo.git/info/refs
  fatal: HTTP request failed

For the sake of simplicity, we only record and display these
errors during the initial fetch of the ref list, as that is
the initial contact with the server and where the most
common, interesting errors happen (and there is already
precedent, as that is the only place we currently massage
http error codes into more helpful messages).

Signed-off-by: Jeff King 
---
I took Jonathan's suggestion of just respecting text/plain to signify a
message that git can show on the terminal. It is the most sensible
content-type to use, I think, but I'm a little worried that
random servers may produce totally uninteresting text/plain (e.g., many
servers ship with 404 pages that just say "404" again with more useless
text). But such pages tend to be text/html, so we may be able to get
away with assuming text/plain will be useful.

 remote-curl.c | 33 -
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 93a09a6..4fea94f 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -151,6 +151,33 @@ static void free_discovery(struct discovery *d)
}
 }
 
+static int show_http_message(struct strbuf *type, struct strbuf *msg)
+{
+   const char *p, *eol;
+
+   /*
+* We only show text/plain parts, as other types are likely
+* to be ugly to look at on the user's terminal.
+*
+* TODO should handle "; charset=XXX", and re-encode into
+* logoutputencoding
+*/
+   if (strcasecmp(type->buf, "text/plain"))
+   return -1;
+
+   strbuf_trim(msg);
+   if (!msg->len)
+   return -1;
+
+   p = msg->buf;
+   do {
+   eol = strchrnul(p, '\n');
+   fprintf(stderr, "remote: %.*s\n", (int)(eol - p), p);
+   p = eol + 1;
+   } while(*eol);
+   return 0;
+}
+
 static struct discovery* discover_refs(const char *service, int for_push)
 {
struct strbuf exp = STRBUF_INIT;
@@ -176,16 +203,20 @@ static struct discovery* discover_refs(const char 
*service, int for_push)
}
refs_url = strbuf_detach(&buffer, NULL);
 
-   http_ret = http_get_strbuf(refs_url, &type, &buffer, HTTP_NO_CACHE);
+   http_ret = http_get_strbuf(refs_url, &type, &buffer,
+  HTTP_NO_CACHE | HTTP_KEEP_ERROR);
switch (http_ret) {
case HTTP_OK:
break;
case HTTP_MISSING_TARGET:
+   show_http_message(&type, &buffer);
die("%s not found: did you run git update-server-info on the"
" server?", refs_url);
case HTTP_NOAUTH:
+   show_http_message(&type, &buffer);
die("Authentication failed");
default:
+   show_http_message(&type, &buffer);
http_error(refs_url, http_ret);
die("HTTP request failed");
}
-- 
1.8.2.rc0.33.gd915649

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/9] http: add HTTP_KEEP_ERROR option

[Jeff King]

We currently set curl's FAILONERROR option, which means that
any http failures are reported as curl errors, and the
http body content from the server is thrown away.

This patch introduces a new option to http_get_strbuf which
specifies that the body content from a failed http response
should be placed in the destination strbuf, where it can be
accessed by the caller.

Signed-off-by: Jeff King 
---
 http.c | 37 +
 http.h |  1 +
 2 files changed, 38 insertions(+)

diff --git a/http.c b/http.c
index 8803c70..45cc7c7 100644
--- a/http.c
+++ b/http.c
@@ -761,6 +761,25 @@ int handle_curl_result(struct slot_results *results)
 
 int handle_curl_result(struct slot_results *results)
 {
+   /*
+* If we see a failing http code with CURLE_OK, we have turned off
+* FAILONERROR (to keep the server's custom error response), and should
+* translate the code into failure here.
+*/
+   if (results->curl_result == CURLE_OK &&
+   results->http_code >= 400) {
+   results->curl_result = CURLE_HTTP_RETURNED_ERROR;
+   /*
+* Normally curl will already have put the "reason phrase"
+* from the server into curl_errorstr; unfortunately without
+* FAILONERROR it is lost, so we can give only the numeric
+* status code.
+*/
+   snprintf(curl_errorstr, sizeof(curl_errorstr),
+"The requested URL returned error: %ld",
+results->http_code);
+   }
+
if (results->curl_result == CURLE_OK) {
credential_approve(&http_auth);
return HTTP_OK;
@@ -825,6 +844,8 @@ static int http_request(const char *url, struct strbuf 
*type,
strbuf_addstr(&buf, "Pragma:");
if (options & HTTP_NO_CACHE)
strbuf_addstr(&buf, " no-cache");
+   if (options & HTTP_KEEP_ERROR)
+   curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 
headers = curl_slist_append(headers, buf.buf);
 
@@ -862,6 +883,22 @@ static int http_request_reauth(const char *url,
int ret = http_request(url, type, result, target, options);
if (ret != HTTP_REAUTH)
return ret;
+
+   /*
+* If we are using KEEP_ERROR, the previous request may have
+* put cruft into our output stream; we should clear it out before
+* making our next request. We only know how to do this for
+* the strbuf case, but that is enough to satisfy current callers.
+*/
+   if (options & HTTP_KEEP_ERROR) {
+   switch (target) {
+   case HTTP_REQUEST_STRBUF:
+   strbuf_reset(result);
+   break;
+   default:
+   die("BUG: HTTP_KEEP_ERROR is only supported with 
strbufs");
+   }
+   }
return http_request(url, type, result, target, options);
 }
 
diff --git a/http.h b/http.h
index 25d1931..0fe54f4 100644
--- a/http.h
+++ b/http.h
@@ -118,6 +118,7 @@ extern char *get_remote_object_url(const char *url, const 
char *hex,
 
 /* Options for http_request_*() */
 #define HTTP_NO_CACHE  1
+#define HTTP_KEEP_ERROR2
 
 /* Return values for http_request_*() */
 #define HTTP_OK0
-- 
1.8.2.rc0.33.gd915649

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATCH 0/9] friendlier http error messages

[Jeff King]

The error messages that git generates for routine http problems can
sometimes be a bit verbose or confusing. They also provide no
opportunity for the server to communicate any free-form text, even
though the server knows much better than the git client the reason for
the error or what the next step to suggest to the user might be.

This series provides a channel for those messages, and does some general
cleanup and reformatting of the error messages that git itself produces.

Here are some before-and-after examples with this series (apologies for
the long lines, but they are part of the ugliness I want to address, so
I am leaving them in).

The "remote:" bits are simulated in the output below, as I haven't yet
updated GitHub's server side to produce more useful messages. So you can
repeat the tests, but note that the text you get from the server will
not be the same (e.g., our 404 currently just says "Repository not
found" which is not all that helpful).

  [before]
  $ git ls-remote https://github.com/non/existent
  fatal: https://github.com/non/existent/info/refs?service=git-upload-pack not 
found: did you run git update-server-info on the server?

  [after]
  $ git ls-remote https://github.com/non/existent
  remote: The remote repository was not found, or you do not have
  remote: permission to access it.
  fatal: repository 'https://github.com/non/existent/' not found

  [before]
  $ GIT_SMART_HTTP=0 git ls-remote https://github.com/git/git.git
  error: The requested URL returned error: 403 Forbidden while accessing 
https://github.com/git/git.git/info/refs
  fatal: HTTP request failed

  [after]
  $ GIT_SMART_HTTP=0 git ls-remote https://github.com/git/git.git
  remote: Sorry, fetching via dumb http is forbidden.
  remote: Please upgrade your git client to v1.6.6 or greater
  remote: and make sure that smart-http is enabled.
  fatal: unable to access 'https://github.com/git/git.git/': The requested URL 
returned error: 403

I still really hate the length of the generic http message (which you
can see in the final 403 example). The text "The requested URL returned
error:" comes from curl, though there is actually an opportunity to
munge it, as you will see in the patches. However, I was unable to come
up with a shorter text that sounded any better.

Another option would be to just split it across lines with some
indentation, like:

  fatal: The requested URL returned error: 403
while accessing https://github.com/git/git.git

I'm open to suggestions.

There are a lot of little patches, as I tried to explain the rationale
for each individual change (and it makes it easy to take or reject
individual patches). If we do take them all, it may make sense to just
squash patches 3-5.

  [1/9]: http: add HTTP_KEEP_ERROR option
  [2/9]: remote-curl: show server content on http errors
  [3/9]: remote-curl: let servers override http 404 advice
  [4/9]: remote-curl: always show friendlier 404 message
  [5/9]: remote-curl: consistently report repo url for http errors
  [6/9]: http: simplify http_error helper function
  [7/9]: http: re-word http error message
  [8/9]: remote-curl: die directly with http error messages
  [9/9]: http: drop http_error function

-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

[PATCH] show-branch: use strbuf instead of static buffer

[Jeff King]

When we generate relative names (e.g., "master~20^2"), we
format the name into a static buffer, then xstrdup the
result to attach it to the commit. Since the first thing we
add into the static buffer is the already-computed name of
the child commit, the names may get longer and longer as
the traversal gets deeper, and we may eventually overflow
the fixed-size buffer.

Fix this by converting the fixed-size buffer into a dynamic
strbuf.  The performance implications should be minimal, as
we end up allocating a heap copy of the name anyway (and now
we can just detach the heap copy from the strbuf).

Reported-by: Eric Roman 
Signed-off-by: Jeff King 
---
This is a fix for a bug report that came to me off-list.  A real-world
example can be seen by running "git show-branch --all" on a fresh clone
of:

  https://chromium.googlesource.com/chromium/src.git

(but that repo is 1.7G, so I don't recommend cloning it unless you're
really interested). Its master branch consists of a strange sequence of
merges that results in naming commits like master^2^2^2^2... and so on
(it's unclear to me why, but it looks like maybe syncing up separate svn
and git repositories?).  Which is odd, but looking at graph, I think the
names show-branch is generating are correct; they're just really long.
And of course odd history is no excuse to overflow a buffer.

Though this is a stack overflow, I don't know that it's exploitable for
anything interesting; an attacker does not get to write arbitrary data,
but rather only a sequence of "^%d" and "~%d" relative history markers.
Perhaps in theory one could devise a history such that the sequence
markers spelled out some malicious code, but it would be quite a
challenge (and given that you have only ascii [^~0-9] to work with,
probably impossible).

I prepared this on "master", but it should be suitable for "maint"; the
code dates all the way back to git v0.99.

 builtin/show-branch.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d208fd6..90fc6b1 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -162,29 +162,28 @@ static void name_commits(struct commit_list *list,
nth = 0;
while (parents) {
struct commit *p = parents->item;
-   char newname[1000], *en;
+   struct strbuf newname = STRBUF_INIT;
parents = parents->next;
nth++;
if (p->util)
continue;
-   en = newname;
switch (n->generation) {
case 0:
-   en += sprintf(en, "%s", n->head_name);
+   strbuf_addstr(&newname, n->head_name);
break;
case 1:
-   en += sprintf(en, "%s^", n->head_name);
+   strbuf_addf(&newname, "%s^", 
n->head_name);
break;
default:
-   en += sprintf(en, "%s~%d",
-   n->head_name, n->generation);
+   strbuf_addf(&newname, "%s~%d",
+   n->head_name, 
n->generation);
break;
}
if (nth == 1)
-   en += sprintf(en, "^");
+   strbuf_addch(&newname, '^');
else
-   en += sprintf(en, "^%d", nth);
-   name_commit(p, xstrdup(newname), 0);
+   strbuf_addf(&newname, "^%d", nth);
+   name_commit(p, strbuf_detach(&newname, NULL), 
0);
i++;
name_first_parent_chain(p);
}
-- 
1.8.2.rc0.33.gd915649
--
To unsubscribe from this list: send the line "unsubscribe 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] count-objects: output "KiB" instead of "kilobytes"

[Antoine Pelisse]

Should we use that opportunity to implement an option like -h (for
humanize) similar to what ls(1), df(1), du(1) does ? Of course "-h" is
already used for help, so we could use -H or any other sensible
choice.
It can become tough to read the size when it gets big enough.

On Thu, Apr 4, 2013 at 6:27 PM, Junio C Hamano  wrote:
> Mihai Capotă  writes:
>
>> The git manual contains an explicit warning about the output of a
>> porcelain command changing: "The interface to Porcelain commands on
>> the other hand are subject to change in order to improve the end user
>> experience."
>
> Yeah, I know that, as I wrote it ;-)
>
> Aside from count-object being not exactly a Porcelain, the statement
> does not give us a blank check to make random changes as we see fit.
> There needs to be a clear improvement.
>
> I am just having a hard time weighing the benefit of using more
> accurate kibibytes over kilobytes and the possible downside of
> breaking other peoples' tools.
>
> Perhaps it would be alright if the change was accompanied by a
> warning in the Release Notes to say something like:
>
> If you have scripts that decide when to run "git repack" by
> parsing the output from "git count-objects", this release
> may break them.  Sorry about that.  One of the scripts
> shipped by git-core itself also had to be adjusted.  The
> command reports the total diskspace used to store loose
> objects in kibibytes, but it was labelled as "kilobytes".
> The number now is shown with "KiB", e.g. "6750 objects,
> 50928 KiB".
>
> You may want to consider updating such scripts to always
> call "git gc --auto" to let it decide when to repack for
> you.
>
> Also, I suspect that for the purpose of this exact output field,
> nobody cares the difference between kibibytes and kilobytes.
> Depending on the system, we add up either st.st_blocks or st.st_size
> and the result is not that exact as "how much diskspace is
> consumed".
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFD] annnotating a pair of commit objects?

[Antoine Pelisse]

On Thu, Jan 3, 2013 at 10:59 AM, Michael Haggerty  wrote:
> On 01/03/2013 08:03 AM, Junio C Hamano wrote:
>> I'd like a datastore that maps a pair of commit object names to
>> another object name, such that:
>>
>>  * When looking at two commits A and B, efficiently query all data
>>associated with a pair of commits  where X is contained in
>>the range A..B and not in B..A, and Y is contained in the range
>>B..A and not in A..B.
>>
>>  * When  is registered in the datastore, and X is rewritten to
>>X' and/or Y is rewritten to Y', the mapping is updated so that it
>>can be queried with  as a new key, similar to the way a
>>notes tree that maps object X can be updated to map object X'
>>when such a rewrite happens.
>>
>> The intended use case is to "go beyond rerere".  Given a history of
>> this shape:
>>
>> o---o---o---I  mainline
>>/
>>   O---o---X---o---Atopic A
>>\
>> o---Y---o---o---B  topic B

I would indeed also be interested in such a feature for my day-to-day
work where we use a workflow similar to git.git's.

> When doing this merge, I think your goal is equivalent to discovering
> that M includes part of the merge of J and B, and adding M as an
> (implicit or explicit) third parent to the merge:
>
>  o---o---o---I---J---K  mainline
> /   /.  /
>O---o---X---o---A.  /topic A
> \   \  .  /
>  \   M.  /
>   \ /   /
>o---Y---o---o---Btopic B
>
> How could M be stored?  Assuming that these type of premerge merges are
> sparse, then Jeff's analysis seems good.  Concretely, one could simply
> store pointers to M from both X and Y; e.g.,
>
> * Add a note to X with the information "when merging this commit with Y,
> use premerge M"
>
> * Add a note to Y with the information "when merging this commit with X,
> use premerge M"
>
> Then, when merging, create the set J..B, scan all of the commits on B..J
> for these "premerge" notes (O(|B..J|)), and for each one, look in the
> set J..B to see if it is present.  The effort should scale like
>
> O( |J..B| + |B..J| * lg(|J..B|) )
>
> where, of course J and B could be exchanged for either aesthetic or
> performance reasons.  (One would also need a mechanism for preventing M
> from being garbage-collected.)

I like the idea of using notes and a kind of "pre-merge". The proposal
seems decent to me.

Michael, have you started implementing such a thing ? If you did, I
would love to help as much as I can.
If you didn't, I would like to start implementing this feature (I
think I now have some time to do so).
Maybe that would require some kind of mentoring though. It could be a
nice opportunity for the community to improve that too as a fake
"gsoc" (no google, no summer, no student)
--
To unsubscribe from this list: send the line "unsubscribe 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/2] perl: redirect stderr to /dev/null instead of closing

[Junio C Hamano]

Petr Baudis  writes:

>> >} elsif ($pid == 0) {
>> > -  if (defined $opts{STDERR}) {
>> > -  close STDERR;
>> > -  }
>> >if ($opts{STDERR}) {
>> >open (STDERR, '>&', $opts{STDERR})
>> >or die "dup failed: $!";
>> 
>> Indeed.  Thanks for pointing that out.
>
>   I'm sorry, I don't follow. Doesn't this just break the STDERR option
> altogether as we will try to dup2() over an already open file
> descriptor? We do need to close STDERR if we are going to reopen it,
> I think.

When $opts{STDERR} is 2, what the three lines the proposed patch
removes did is actively wrong, because you dup2 the fd you just
closed.

When $opts{STDERR} is 1, it seems to do the right thing with or
without the "close STDERR" in front.  Isn't this because the usual
"open($fd, <<>>) closes $fd as necessary" applies to this
case as well?

So, I am not sure what you are viewing as a problem.  Puzzled...

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] log -L overlapping ranges

[Junio C Hamano]

Thomas Rast  writes:

> I noticed that it doesn't like getting multiple overlapping ranges
> from the user.  This fixes it.

Both look sensible.  Will queue on top of tr/line-log.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] diffcore-pickaxe: respect --no-textconv

[Junio C Hamano]

Thanks; will replace the one in 'pu' with this.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 0/7] Rework git core for native submodules

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Once you start recording the latter also at path "A", it becomes
> unclear what "git diff A" should show.
>
> That is what I said in the message, to which you invented "diff-link"
> as a solution to the "unclear"-ness.

I just thought it would be a stopgap until we get diff to support
links natively.  Obviously, when we get native diff support, 'log -p'
will be able to show differences as well.  As it turns out from my
little experiment with cat-link, it's really easy to get native diff
support, and I'm targeting that directly instead of a scripted
solution.

As for the unclearness issue, it's a little more complicated than
that: a non-floating submodule could've previously been a floating
one, or vice-versa.  As of this moment, I'm only planning to show
differences between link buffers.  In the case when two consecutive
commits change a link checkout_rev (where floating is set to 0), we
can come up with something like the current diff.submodule = log.  I
see no cause to worry about the interface of that now.
--
To unsubscribe from this list: send the line "unsubscribe 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 09/12] pretty: add %C(auto) for auto-coloring on the next placeholder

[Junio C Hamano]

Duy Nguyen  writes:

> The above should have written "will turn on auto coloring on the
> following _textual_ placeholder". I didn't intend %C(auto) to be
> followed by %C(color) as it's already covered by %C(auto,red). But of
> course we could make it work too.

You are right that there is no need to say "%C(auto)%C(red)", it is
"%C(auto,red)", but that misses the point.

If %C(auto) applies to some % but not to some others,
the user needs to learn which % will eat the "auto" (so
it no longer applies to the next one) and which one will not even
look at "auto" (so the next % is affected by the auto,
i.e. making the effect of auto skip a %).  If the rule
were "%C(auto) applies to -next- placeholder", then the user does
not have to worry about which ones are what you call textual and
which ones are not (and there is no textual placeholder defined in
the glossary).

That would make it harder to learn. It would be much easier to
explain if you said "%C(auto) affects the next %-placeholder and
then resets".

I wonder if "Everything after %C(auto) will not be coloured if the
output is not going to the terminal.", i.e. not resetting once
colouring decision is made, makes more sense, though...




--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 0/7] Rework git core for native submodules

[Junio C Hamano]

Ramkumar Ramachandra  writes:

> Junio C Hamano wrote:
>> "git log -p .gitmodules" would be a way to review what changed in
>> the information about submodules.  Don't you need "git log-link" for
>> exactly the same reason why you need "git diff-link" in the first
>> place?
>>
>> So you may not have suggested it, but I suspect that was only
>> because you haven't had enough time to think things through.
>
> What is this git log -p .gitmodules doing?  It's walking down the
> commit history, and picking out the commits in which that blob
> changed.  Then it's diffing the blobs in those commits with each
> other.  Why is git log -p  any different?  We already know how
> to diff blobs, and we just need a way to diff links.

You already forget what you invented "git diff-link" as a "solution"
for, perhaps?

By recording the submodules themselve and information _about_ the
submodules separately (the latter is in .gitmodules), "git diff A"
can show the difference in submodule A, while "git diff .gitmodules"
can show a change, which is a possibly in-working-tree-only proposed
change, in information about submoudules.

Once you start recording the latter also at path "A", it becomes
unclear what "git diff A" should show.

That is what I said in the message, to which you invented "diff-link"
as a solution to the "unclear"-ness.

Am I misremembering the flow of discussion in this thread?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Beyond inotify recursive watches

[Ramkumar Ramachandra]

Jan Kara wrote:
>   Hum, I have somewhat hard time to understand what do you mean by
> 'magically optimized syscalls'. What should happen in VFS to speedup your
> load?

In retrospect, I think this is a terrible hack to begin with.  Tuning
the filesystem specifically for git repositories is inelegant on so
many levels, I can't recall why I ever thought it would be a good
idea.  Like all software, Git has scaling issues with ultra-large
repositories.  Too many stat() calls is just one of the problems:
there will be too many objects to do any operation at reasonable
speed, and the overall UX would just suck.  Instead of growing to a
huge monolithic beast that spawns off worker threads for everything
and ultimately dying off, I've decided that git should take a
different direction: it should work with well with many small
easily-composable repositories.  I've started work on this already,
and it looks very promising.

Let the filesystem people do what they do best: optimizing for all
applications uniformly.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible bug: Git submodules can get into broken state

[Junio C Hamano]

Chris Wilson  writes:

> They fixed it by checking in a rectified .gitmodules file.
>
> In the mean time, I ran git submodule init, and now I've ended up in a
> situation where git submodule update doesn't work, and there's no
> submodule command to fix it, so I have to remove the broken submodules
> from .git/config.
>
>> and the configuration mechanism is the way to use custom URL in
>> place of it.
>
> I don't want to use a custom URL, I want to use the URL which is now
> in .gitmodules.

Then don't call it a "custom URL" ;-).

Isn't your problem that the old, broken one is now in your config,
and you want to update that with a corrected one?  How you learned
which one is correct does not really matter---you may learned it out
of band by looking at upstream's gitweb, or by running "git fetch"
in the superproject again and looking at the updated .gitmodules
file.  The configuration mechanism has a way to update that entry.

> Could submodule init at least change the URLs of submodules which are
> not checked out? (e.g. because they couldn't be)?

Perhaps "submodule deinit" might be what you are looking for, but I
dunno.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] diffcore-pickaxe: unify setup and teardown code between log -S/-G

[Junio C Hamano]

Jeff King  writes:

> I didn't actually test that patch beyond compilation (but it's
> _obviously_ correct, right?), and I'm about to go to bed. Do you want to
> take care of adapting your commit message to it?

The code looks obviously correct, yes.

We might have to revisit the "Is unmerged?" thing, Cf. d7c9bf22351e
(diffcore-rename: don't consider unmerged path as source,
2011-03-23), but that is an unlikely corner case, especially for
these options (-S/-G).

-- >8 --
From: Jeff King 
Date: Fri, 5 Apr 2013 01:28:10 -0400
Subject: [PATCH] diffcore-pickaxe: unify code for log -S/-G

The logic flow of has_changes() used for "log -S" and diff_grep()
used for "log -G" are essentially the same.  See if we have both
sides that could be different in any interesting way, slurp the
contents in core, possibly after applying textconv, inspect the
contents, clean-up and report the result.  The only difference
between the two is how "inspect" step works.

Unify this codeflow in a helper, pickaxe_match(), which takes a
callback function that implements the specific "inspect" step.

After removing the common scaffolding code from the existing
has_changes() and diff_grep(), they each becomes such a callback
function suitable for passing to pickaxe_match().

Signed-off-by: Jeff King 
Signed-off-by: Junio C Hamano 
---
 diffcore-pickaxe.c | 118 ++---
 1 file changed, 49 insertions(+), 69 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index cadb071..63722f8 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -8,7 +8,12 @@
 #include "xdiff-interface.h"
 #include "kwset.h"
 
-typedef int (*pickaxe_fn)(struct diff_filepair *p, struct diff_options *o, 
regex_t *regexp, kwset_t kws);
+typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
+ struct diff_options *o,
+ regex_t *regexp, kwset_t kws);
+
+static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
+regex_t *regexp, kwset_t kws, pickaxe_fn fn);
 
 static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
regex_t *regexp, kwset_t kws, pickaxe_fn fn)
@@ -22,7 +27,7 @@ static void pickaxe(struct diff_queue_struct *q, struct 
diff_options *o,
/* Showing the whole changeset if needle exists */
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
-   if (fn(p, o, regexp, kws))
+   if (pickaxe_match(p, o, regexp, kws, fn))
return; /* do not munge the queue */
}
 
@@ -37,7 +42,7 @@ static void pickaxe(struct diff_queue_struct *q, struct 
diff_options *o,
/* Showing only the filepairs that has the needle */
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
-   if (fn(p, o, regexp, kws))
+   if (pickaxe_match(p, o, regexp, kws, fn))
diff_q(&outq, p);
else
diff_free_filepair(p);
@@ -74,64 +79,33 @@ static void diffgrep_consume(void *priv, char *line, 
unsigned long len)
line[len] = hold;
 }
 
-static int diff_grep(struct diff_filepair *p, struct diff_options *o,
+static int diff_grep(mmfile_t *one, mmfile_t *two,
+struct diff_options *o,
 regex_t *regexp, kwset_t kws)
 {
regmatch_t regmatch;
-   struct userdiff_driver *textconv_one = NULL;
-   struct userdiff_driver *textconv_two = NULL;
-   mmfile_t mf1, mf2;
-   int hit;
+   struct diffgrep_cb ecbdata;
+   xpparam_t xpp;
+   xdemitconf_t xecfg;
 
-   if (!o->pickaxe[0])
-   return 0;
+   if (!one)
+   return !regexec(regexp, two->ptr, 1, ®match, 0);
+   if (!two)
+   return !regexec(regexp, one->ptr, 1, ®match, 0);
 
-   if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
-   textconv_one = get_textconv(p->one);
-   textconv_two = get_textconv(p->two);
-   }
-
-   if (textconv_one == textconv_two && diff_unmodified_pair(p))
-   return 0;
-
-   mf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);
-   mf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);
-
-   if (!DIFF_FILE_VALID(p->one)) {
-   if (!DIFF_FILE_VALID(p->two))
-   hit = 0; /* ignore unmerged */
-   else
-   /* created "two" -- does it have what we are looking 
for? */
-   hit = !regexec(regexp, mf2.ptr, 1, ®match, 0);
-   } else if (!DIFF_FILE_VALID(p->two)) {
-   /* removed "one" -- did it have what we are looking for? */
-   hit = !regexec(regexp, mf1.ptr, 1, ®match, 0);
-   } else {
-   /*
-  

Re: [RFC/PATCH 0/7] Rework git core for native submodules

[Ramkumar Ramachandra]

Linus Torvalds wrote:
> So you absolutely need a dirty worktree. You need it for testing, and
> you need it for merging. Having a model where you don't have a
> in-progress entity that works as a temporary is absolutely and
> entirely wrong.

I agree entirely.  My comment was just a "by the way", and specific to
how people work with .gitmodules: I didn't imply any strong notions of
Right or Wrong with respect to dirty worktrees in general.  So, yes:
links stage and unstage, just like blobs do.

Oh, and I'm currently writing infrastructure to work with links like
blobs.  Here's a WIP: git cat-link  is exactly the same as cat
, to the end user.

-- 8< --
>From d8a1de6f9075771dde6f1fde9ffa193dce386a17 Mon Sep 17 00:00:00 2001
From: Ramkumar Ramachandra 
Date: Fri, 5 Apr 2013 19:42:56 +0530
Subject: [PATCH] builtin/cat-link: implement new builtin

This is a simple program that calls unpack_trees() with a custom
callback that just prints the contents of whatever objects were
matched using revs.prune_data.  Blobs can be cat'ed directly from the
filesystem, so this program is primarily useful for links; git
cat-link  shows it up like a blob.

We will use this program to build edit-link.

Signed-off-by: Ramkumar Ramachandra 
---
 Makefile   |  3 +-
 builtin.h  |  1 +
 builtin/cat-link.c | 83 ++
 diff-lib.c | 10 +++
 diff.h |  6 
 git.c  |  1 +
 6 files changed, 98 insertions(+), 6 deletions(-)
 create mode 100644 builtin/cat-link.c

diff --git a/Makefile b/Makefile
index cd4b6f9..28194d7 100644
--- a/Makefile
+++ b/Makefile
@@ -349,7 +349,7 @@ GIT-VERSION-FILE: FORCE
 
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
-CFLAGS = -g -O2 -Wall
+CFLAGS = -g -O0 -Wall
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
@@ -893,6 +893,7 @@ BUILTIN_OBJS += builtin/blame.o
 BUILTIN_OBJS += builtin/branch.o
 BUILTIN_OBJS += builtin/bundle.o
 BUILTIN_OBJS += builtin/cat-file.o
+BUILTIN_OBJS += builtin/cat-link.o
 BUILTIN_OBJS += builtin/check-attr.o
 BUILTIN_OBJS += builtin/check-ignore.o
 BUILTIN_OBJS += builtin/check-ref-format.o
diff --git a/builtin.h b/builtin.h
index faef559..be0160d 100644
--- a/builtin.h
+++ b/builtin.h
@@ -49,6 +49,7 @@ extern int cmd_blame(int argc, const char **argv, const char 
*prefix);
 extern int cmd_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_bundle(int argc, const char **argv, const char *prefix);
 extern int cmd_cat_file(int argc, const char **argv, const char *prefix);
+extern int cmd_cat_link(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout_index(int argc, const char **argv, const char *prefix);
 extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
diff --git a/builtin/cat-link.c b/builtin/cat-link.c
new file mode 100644
index 000..14dd92b
--- /dev/null
+++ b/builtin/cat-link.c
@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2013 Ramkumar Ramachandra
+ */
+#include "cache.h"
+#include "tree.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "commit.h"
+#include "diff.h"
+#include "revision.h"
+
+static int cat_file(struct cache_entry **src, struct unpack_trees_options *o) {
+   int cached, match_missing = 1;
+   unsigned dirty_submodule = 0;
+   unsigned int mode;
+   const unsigned char *sha1;
+   struct cache_entry *idx = src[0];
+   struct cache_entry *tree = src[1];
+   struct rev_info *revs = o->unpack_data;
+   enum object_type type;
+   unsigned long size;
+   char *buf;
+
+   cached = o->index_only;
+   if (ce_path_match(idx ? idx : tree, &revs->prune_data)) {
+   if (get_stat_data(idx, &sha1, &mode, cached, match_missing,
+   &dirty_submodule, NULL) < 0)
+   die("Something went wrong!");
+   buf = read_sha1_file(sha1, &type, &size);
+   printf("%s", buf);
+   }
+   return 0;
+}
+
+int cmd_cat_link(int argc, const char **argv, const char *prefix)
+{
+   struct unpack_trees_options opts;
+   int cached = 1;
+   struct rev_info revs;
+   struct tree *tree;
+   struct tree_desc tree_desc;
+   struct object_array_entry *ent;
+
+   if (argc < 2)
+   die("Usage: git cat-link ");
+
+   init_revisions(&revs, prefix);
+   setup_revisions(argc, argv, &revs, NULL); /* For revs.prune_data */
+   add_head_to_pending(&revs);
+
+   /* Hack to diff against index; we create a dummy tree for the
+  index information */
+   if (!revs.pending.nr) {
+   struct tree *tree;
+   tree = lookup_tree(EMPTY_TREE_SHA1_BIN);
+   add_pending_object(&revs, &tree->object, "HEAD");
+   }
+
+   if (read_cache() < 0)
+   die("read_cache(

Re: [PATCH v2] submodule: print graph output next to submodule log

[Johannes Schindelin]

Hi John,

On Fri, 5 Apr 2013, John Keeping wrote:

> When running "git log -p --submodule=log", the submodule log is not
> indented by the graph output, although all other lines are.  Fix this by
> prepending the current line prefix to each line of the submodule log.

Nice! I agree that submodules are an ugly child of Git; not even Git
itself uses them ;-)

Thanks for that fix!

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] submodule: print graph output next to submodule log

[John Keeping]

When running "git log -p --submodule=log", the submodule log is not
indented by the graph output, although all other lines are.  Fix this by
prepending the current line prefix to each line of the submodule log.

Signed-off-by: John Keeping 
---
On Fri, Apr 05, 2013 at 05:06:30PM +0100, John Keeping wrote:
> From: John Keeping 

That shouldn't have escaped here :-(  The email in the S-O-B is the
right one.

No other change from v1.

 diff.c  |  1 +
 submodule.c | 13 +
 submodule.h |  1 +
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index db952a5..28a742c 100644
--- a/diff.c
+++ b/diff.c
@@ -2255,6 +2255,7 @@ static void builtin_diff(const char *name_a,
const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
show_submodule_summary(o->file, one ? one->path : two->path,
+   line_prefix,
one->sha1, two->sha1, two->dirty_submodule,
meta, del, add, reset);
return;
diff --git a/submodule.c b/submodule.c
index 975bc87..e728025 100644
--- a/submodule.c
+++ b/submodule.c
@@ -216,6 +216,7 @@ static int prepare_submodule_summary(struct rev_info *rev, 
const char *path,
 }
 
 static void print_submodule_summary(struct rev_info *rev, FILE *f,
+   const char *line_prefix,
const char *del, const char *add, const char *reset)
 {
static const char format[] = "  %m %s";
@@ -226,6 +227,7 @@ static void print_submodule_summary(struct rev_info *rev, 
FILE *f,
struct pretty_print_context ctx = {0};
ctx.date_mode = rev->date_mode;
strbuf_setlen(&sb, 0);
+   strbuf_addstr(&sb, line_prefix);
if (commit->object.flags & SYMMETRIC_LEFT) {
if (del)
strbuf_addstr(&sb, del);
@@ -256,6 +258,7 @@ int parse_fetch_recurse_submodules_arg(const char *opt, 
const char *arg)
 }
 
 void show_submodule_summary(FILE *f, const char *path,
+   const char *line_prefix,
unsigned char one[20], unsigned char two[20],
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset)
@@ -280,16 +283,18 @@ void show_submodule_summary(FILE *f, const char *path,
message = "(revision walker failed)";
 
if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
-   fprintf(f, "Submodule %s contains untracked content\n", path);
+   fprintf(f, "%sSubmodule %s contains untracked content\n",
+   line_prefix, path);
if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-   fprintf(f, "Submodule %s contains modified content\n", path);
+   fprintf(f, "%sSubmodule %s contains modified content\n",
+   line_prefix, path);
 
if (!hashcmp(one, two)) {
strbuf_release(&sb);
return;
}
 
-   strbuf_addf(&sb, "%sSubmodule %s %s..", meta, path,
+   strbuf_addf(&sb, "%s%sSubmodule %s %s..", line_prefix, meta, path,
find_unique_abbrev(one, DEFAULT_ABBREV));
if (!fast_backward && !fast_forward)
strbuf_addch(&sb, '.');
@@ -301,7 +306,7 @@ void show_submodule_summary(FILE *f, const char *path,
fwrite(sb.buf, sb.len, 1, f);
 
if (!message) /* only NULL if we succeeded in setting up the walk */
-   print_submodule_summary(&rev, f, del, add, reset);
+   print_submodule_summary(&rev, f, line_prefix, del, add, reset);
if (left)
clear_commit_marks(left, ~0);
if (right)
diff --git a/submodule.h b/submodule.h
index 3dc1b3f..c7ffc7c 100644
--- a/submodule.h
+++ b/submodule.h
@@ -19,6 +19,7 @@ int parse_submodule_config_option(const char *var, const char 
*value);
 void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
 void show_submodule_summary(FILE *f, const char *path,
+   const char *line_prefix,
unsigned char one[20], unsigned char two[20],
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset);
-- 
1.8.2.452.gb520e27
--
To unsubscribe from this list: send the line "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] submodule: print graph output next to submodule log

[John Keeping]

From: John Keeping 

When running "git log -p --submodule=log", the submodule log is not
indented by the graph output, although all other lines are.  Fix this by
prepending the current line prefix to each line of the submodule log.

Signed-off-by: John Keeping 
---
 diff.c  |  1 +
 submodule.c | 13 +
 submodule.h |  1 +
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index db952a5..28a742c 100644
--- a/diff.c
+++ b/diff.c
@@ -2255,6 +2255,7 @@ static void builtin_diff(const char *name_a,
const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
show_submodule_summary(o->file, one ? one->path : two->path,
+   line_prefix,
one->sha1, two->sha1, two->dirty_submodule,
meta, del, add, reset);
return;
diff --git a/submodule.c b/submodule.c
index 975bc87..e728025 100644
--- a/submodule.c
+++ b/submodule.c
@@ -216,6 +216,7 @@ static int prepare_submodule_summary(struct rev_info *rev, 
const char *path,
 }
 
 static void print_submodule_summary(struct rev_info *rev, FILE *f,
+   const char *line_prefix,
const char *del, const char *add, const char *reset)
 {
static const char format[] = "  %m %s";
@@ -226,6 +227,7 @@ static void print_submodule_summary(struct rev_info *rev, 
FILE *f,
struct pretty_print_context ctx = {0};
ctx.date_mode = rev->date_mode;
strbuf_setlen(&sb, 0);
+   strbuf_addstr(&sb, line_prefix);
if (commit->object.flags & SYMMETRIC_LEFT) {
if (del)
strbuf_addstr(&sb, del);
@@ -256,6 +258,7 @@ int parse_fetch_recurse_submodules_arg(const char *opt, 
const char *arg)
 }
 
 void show_submodule_summary(FILE *f, const char *path,
+   const char *line_prefix,
unsigned char one[20], unsigned char two[20],
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset)
@@ -280,16 +283,18 @@ void show_submodule_summary(FILE *f, const char *path,
message = "(revision walker failed)";
 
if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
-   fprintf(f, "Submodule %s contains untracked content\n", path);
+   fprintf(f, "%sSubmodule %s contains untracked content\n",
+   line_prefix, path);
if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-   fprintf(f, "Submodule %s contains modified content\n", path);
+   fprintf(f, "%sSubmodule %s contains modified content\n",
+   line_prefix, path);
 
if (!hashcmp(one, two)) {
strbuf_release(&sb);
return;
}
 
-   strbuf_addf(&sb, "%sSubmodule %s %s..", meta, path,
+   strbuf_addf(&sb, "%s%sSubmodule %s %s..", line_prefix, meta, path,
find_unique_abbrev(one, DEFAULT_ABBREV));
if (!fast_backward && !fast_forward)
strbuf_addch(&sb, '.');
@@ -301,7 +306,7 @@ void show_submodule_summary(FILE *f, const char *path,
fwrite(sb.buf, sb.len, 1, f);
 
if (!message) /* only NULL if we succeeded in setting up the walk */
-   print_submodule_summary(&rev, f, del, add, reset);
+   print_submodule_summary(&rev, f, line_prefix, del, add, reset);
if (left)
clear_commit_marks(left, ~0);
if (right)
diff --git a/submodule.h b/submodule.h
index 3dc1b3f..c7ffc7c 100644
--- a/submodule.h
+++ b/submodule.h
@@ -19,6 +19,7 @@ int parse_submodule_config_option(const char *var, const char 
*value);
 void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
 void show_submodule_summary(FILE *f, const char *path,
+   const char *line_prefix,
unsigned char one[20], unsigned char two[20],
unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset);
-- 
1.8.2.452.gb520e27

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 0/7] Rework git core for native submodules

[Linus Torvalds]

On Thu, Apr 4, 2013 at 1:04 PM, Ramkumar Ramachandra  wrote:
> Linus Torvalds wrote:
>> Or you could also just edit and carry a dirty .gitmodules around for
>> your personal use-case.
>
> I'm sorry, but a dirty worktree is unnecessarily painful to work with.

Bzzt. Wrong.

A dirty worktree is not only easy to work with (I do it all the time,
having random test-patches in my tree that I never even intend to
commit), it's a *requirement*.

One thing that git does really really well is merging. And one of the
reasons why git does merging well (apart from the obvious meta-issue:
it's what I care about) is that it not only has the stable information
in the object database, it also has the staging information in the
index, *and* it has dirty data in the working tree.

You absolutely need all three. Having an "edit" command to edit stable
data (or staging data) is broken. Trust me, I've been there, done
that, got the T-shirt and know it is wrong. The whole "stable objects"
+ "index" + "dirty worktree" is FUNDAMENTALLY the right way to work,
and it *has* to work that way for merges to work well.

The only things that we don't have "dirty data" for in the worktree is
creating commits and tags, but those aren't relevant for the merging
process anyway, in the sense that you never change them for merging,
you create them *after* merging (and this is fundamental, and not just
a git implementation issue).

So you absolutely need a dirty worktree. You need it for testing, and
you need it for merging. Having a model where you don't have a
in-progress entity that works as a temporary is absolutely and
entirely wrong.

   Linus
--
To unsubscribe from this list: send the line "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] glossary: extend "detached HEAD" description

[Junio C Hamano]

When we introduced the concept of "detached HEAD", we made sure that
commands that operate on the history of the current branch "just
work" in that state.  They update the HEAD to point at the new
history without affecting any branch when the HEAD is detached, just
like they update the tip of the "current branch" to point at the new
history when HEAD points at a specific branch.

As this is done as the natural extension for these commands, we did
not, we still do not, and we do not want to repeat "A detached HEAD
is updated without affecting any branch" when describing what each
and every one of these commands that operate "on the current branch"
do.  

Add a blanket description to the glossary to cover them instead.
The general principle is that operations to update the branch work
and affect on the HEAD, while operations to update the information
about a branch do not.

Signed-off-by: Junio C Hamano 
---

 * Noticed that we describe "commit --amend" as updating "the
   current branch" while reviewing the recent update by CMN, and it
   is shared by all the manual pages of commands that work on "the
   current branch".  "git grep -i detached Documentation/" shows
   very small number of hits, indicating that we do not repeat "if
   detached, only the HEAD is updated".

   I am not absolutely sure if glossary is the best place, but
   something like this may be necessary to help people who are new
   to this state.

 Documentation/glossary-content.txt | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/glossary-content.txt 
b/Documentation/glossary-content.txt
index f928b57..69c90d1 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -100,9 +100,22 @@ to point at the new commit.
 
 [[def_detached_HEAD]]detached HEAD::
Normally the <> stores the name of a
-   <>.  However, git also allows you to 
<>
-   an arbitrary <> that isn't necessarily the tip of any
-   particular branch.  In this case HEAD is said to be "detached".
+   <>, and commands that operate on the
+   history HEAD represents operate on the history leading to the
+   tip of the branch the HEAD points at.  However, Git also
+   allows you to <> an arbitrary
+   <> that isn't necessarily the tip of any
+   particular branch.  The HEAD in such a state is called
+   "detached".
++
+Note that commands that operate on the history of the current branch
+(e.g. `git commit` to build a new history on top of it) still work
+while the HEAD is detached. They update the HEAD to point at the tip
+of the updated history without affecting any branch.  Commands that
+update or inquire information _about_ the current branch (e.g. `git
+branch --set-upstream-to` that sets what remote tracking branch the
+current branch integrates with) obviously do not work, as there is no
+(real) current branch to ask about in this state.
 
 [[def_dircache]]dircache::
You are *way* behind. See <>.
--
To unsubscribe from this list: send the line "unsubscribe 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/2] perl: redirect stderr to /dev/null instead of closing

[Petr Baudis]

  Hi!

On Thu, Apr 04, 2013 at 10:41:41PM +0200, Thomas Rast wrote:
> As pointed out by Eric Wong (thanks), the initial close needs to go:
> die() would again write nowhere if we close STDERR beforehand.
> 
> > Perhaps we should also do the following:
> >
> > --- a/perl/Git.pm
> > +++ b/perl/Git.pm
> > @@ -1489,9 +1489,6 @@ sub _command_common_pipe {
> > if (not defined $pid) {
> > throw Error::Simple("open failed: $!");
> > } elsif ($pid == 0) {
> > -   if (defined $opts{STDERR}) {
> > -   close STDERR;
> > -   }
> > if ($opts{STDERR}) {
> > open (STDERR, '>&', $opts{STDERR})
> > or die "dup failed: $!";
> 
> Indeed.  Thanks for pointing that out.

  I'm sorry, I don't follow. Doesn't this just break the STDERR option
altogether as we will try to dup2() over an already open file
descriptor? We do need to close STDERR if we are going to reopen it,
I think.

  Kind regards,

Petr "Pasky" Baudis
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Collective wisdom about repos on NFS accessed by concurrent clients (== corruption!?)

[Kenneth Ölwing]

On 2013-04-05 15:42, Thomas Rast wrote:
Can you run the same tests under strace or similar, and gather the 
relevant outputs? Otherwise it's probably very hard to say what is 
going wrong. In particular we've had some reports on lustre that 
boiled down to "impossible" returns from libc functions, not git 
issues. It's hard to say without some evidence. 

Thomas, thanks for your reply.

I'm assuming I should strace the git commands as they're issued? I'm 
already collecting regular stdout/err output in a log as I go. Is there 
any debugging things I can turn on to make the calls issue internal 
tracing of some sort?


The main issue I see is that I suspect it will generate so much data 
that it'll overflow my disk ;-). Consider that my hammer consists of a 
Perl script that forks a number of tasks (e.g. 15) that each loops doing 
clone/commit/push/pull, with retrying on a few levels as errors occur 
(usually expected ones due to the concurrency, i.e. someone else pushed 
so a pull is necessary first, but occasionally the central repo is 
broken enough that it can't be cloned from, or at least not checked out 
master from...sometimes with printed errors that still give me a zero 
exit code...). That is then also run on several machines to the same 
repo to hopefully cause a breakage by sheer pounding...it's going to 
generate huge collections of strace output I expect...


I have some variations of this (e.g. all tasks are working on different 
branches, improving concurrency in some respects, but effects there have 
been that at the end I was missing a branch or so...). The likelihood of 
problems seems to increase when I actually use ssh in my ultimate setup, 
so a loadbalancer roundrobins each call to any of several hosts. In that 
case I must admit I don't know how to get in on the action since I guess 
I would need to strace the git-upload/receive-pack processes on the 
server side...?


Lastly, I don't know how much this will impact timings etc, or load. To 
get a broken result I have sometimes needed to run for many hours, 
others fairly quickly.


Well...I will try, it'll probably be a blast :-)

BTW, this is mostly done on Centos 6.3 and 6.4, locally built git-1.8.2.

ken1




--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] log -L: fix overlapping input ranges

[Thomas Rast]

The existing code was too defensive, and would trigger the assert in
range_set_append() if the user gave overlapping ranges.

The intent was always to define overlapping ranges as just the union
of all of them, as evidenced by the call to sort_and_merge_range_set().
(Which was already used, unlike what the comment said.)

Fix by splitting out the meat of range_set_append() to a new _unsafe()
function that lacks the paranoia.  sort_and_merge_range_set will fix
up the ranges, so we don't need the checks there.

Signed-off-by: Thomas Rast 
---
 line-log.c  |  17 ++--
 t/t4211-line-log.sh |   6 ++
 t/t4211/expect.multiple | 104 
 t/t4211/expect.multiple-overlapping | 187 
 t/t4211/expect.multiple-superset|  59 
 5 files changed, 366 insertions(+), 7 deletions(-)
 create mode 100644 t/t4211/expect.multiple
 create mode 100644 t/t4211/expect.multiple-overlapping
 create mode 100644 t/t4211/expect.multiple-superset

diff --git a/line-log.c b/line-log.c
index 5a12ceb..85c7c24 100644
--- a/line-log.c
+++ b/line-log.c
@@ -56,16 +56,21 @@ static void range_set_move(struct range_set *dst, struct 
range_set *src)
 }
 
 /* tack on a _new_ range _at the end_ */
-static void range_set_append(struct range_set *rs, long a, long b)
+static void range_set_append_unsafe(struct range_set *rs, long a, long b)
 {
assert(a <= b);
-   assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a);
range_set_grow(rs, 1);
rs->ranges[rs->nr].start = a;
rs->ranges[rs->nr].end = b;
rs->nr++;
 }
 
+static void range_set_append(struct range_set *rs, long a, long b)
+{
+   assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a);
+   range_set_append_unsafe(rs, a, b);
+}
+
 static int range_cmp(const void *_r, const void *_s)
 {
const struct range *r = _r;
@@ -99,10 +104,8 @@ static void range_set_check_invariants(struct range_set *rs)
 }
 
 /*
- * Helper: In-place pass of sorting and merging the ranges in the
- * range set, to re-establish the invariants after another operation
- *
- * NEEDSWORK currently not needed
+ * In-place pass of sorting and merging the ranges in the range set,
+ * to establish the invariants when we get the ranges from the user
  */
 static void sort_and_merge_range_set(struct range_set *rs)
 {
@@ -280,7 +283,7 @@ static void line_log_data_insert(struct line_log_data 
**list,
struct line_log_data *p = search_line_log_data(*list, spec->path, &ip);
 
if (p) {
-   range_set_append(&p->ranges, begin, end);
+   range_set_append_unsafe(&p->ranges, begin, end);
sort_and_merge_range_set(&p->ranges);
free_filespec(spec);
return;
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 426a828..2341351 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -39,6 +39,12 @@ canned_test "-L 24,+1:a.c simple" vanishes-early
 
 canned_test "-L '/long f/,/^}/:b.c' move-support" move-support-f
 
+canned_test "-L 4,12:a.c -L :main:a.c simple" multiple
+canned_test "-L 4,18:a.c -L :main:a.c simple" multiple-overlapping
+canned_test "-L :main:a.c -L 4,18:a.c simple" multiple-overlapping
+canned_test "-L 4:a.c -L 8,12:a.c simple" multiple-superset
+canned_test "-L 8,12:a.c -L 4:a.c simple" multiple-superset
+
 test_bad_opts "-L" "switch.*requires a value"
 test_bad_opts "-L b.c" "argument.*not of the form"
 test_bad_opts "-L 1:" "argument.*not of the form"
diff --git a/t/t4211/expect.multiple b/t/t4211/expect.multiple
new file mode 100644
index 000..76ad5b5
--- /dev/null
+++ b/t/t4211/expect.multiple
@@ -0,0 +1,104 @@
+commit 4659538844daa2849b1a9e7d6fadb96fcd26fc83
+Author: Thomas Rast 
+Date:   Thu Feb 28 10:48:43 2013 +0100
+
+change back to complete line
+
+diff --git a/a.c b/a.c
+--- a/a.c
 b/a.c
+@@ -18,5 +18,7 @@
+ int main ()
+ {
+   printf("%ld\n", f(15));
+   return 0;
+-}
+\ No newline at end of file
++}
++
++/* incomplete lines are bad! */
+
+commit 100b61a6f2f720f812620a9d10afb3a960ccb73c
+Author: Thomas Rast 
+Date:   Thu Feb 28 10:48:10 2013 +0100
+
+change to an incomplete line at end
+
+diff --git a/a.c b/a.c
+--- a/a.c
 b/a.c
+@@ -18,5 +18,5 @@
+ int main ()
+ {
+   printf("%ld\n", f(15));
+   return 0;
+-}
++}
+\ No newline at end of file
+
+commit a6eb82647d5d67f893da442f8f9375fd89a3b1e2
+Author: Thomas Rast 
+Date:   Thu Feb 28 10:45:16 2013 +0100
+
+touch both functions
+
+diff --git a/a.c b/a.c
+--- a/a.c
 b/a.c
+@@ -3,9 +3,9 @@
+-int f(int x)
++long f(long x)
+ {
+   int s = 0;
+   while (x) {
+   x >>= 1;
+   s++;
+   }
+   return s;
+ }
+@@ -17,5 +17,5 @@
+ int main ()
+ {
+-  printf("%d\n", f(15));
++  printf("%ld\n", f(15));
+   return 0;
+ }
+
+commit f04fb20f2c77850996cba739709acc6faecc58f7
+Author: Thomas Rast 
+Date:   Thu Feb 28 10:44:55 2013 +

[PATCH 1/2] log -L: check range set invariants when we look it up

[Thomas Rast]

lookup_line_range() is a good place to check that the range sets
satisfy the invariants: they have been computed and set in earlier
iterations, and we now start working with them.

Signed-off-by: Thomas Rast 
---
 line-log.c | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/line-log.c b/line-log.c
index 30edef4..5a12ceb 100644
--- a/line-log.c
+++ b/line-log.c
@@ -80,6 +80,25 @@ static int range_cmp(const void *_r, const void *_s)
 }
 
 /*
+ * Check that the ranges are non-empty, sorted and non-overlapping
+ */
+static void range_set_check_invariants(struct range_set *rs)
+{
+   int i;
+
+   if (!rs)
+   return;
+
+   if (rs->nr)
+   assert(rs->ranges[0].start < rs->ranges[0].end);
+
+   for (i = 1; i < rs->nr; i++) {
+   assert(rs->ranges[i-1].end < rs->ranges[i].start);
+   assert(rs->ranges[i].start < rs->ranges[i].end);
+   }
+}
+
+/*
  * Helper: In-place pass of sorting and merging the ranges in the
  * range set, to re-establish the invariants after another operation
  *
@@ -103,6 +122,8 @@ static void sort_and_merge_range_set(struct range_set *rs)
}
assert(o <= rs->nr);
rs->nr = o;
+
+   range_set_check_invariants(rs);
 }
 
 /*
@@ -687,8 +708,13 @@ static struct line_log_data *lookup_line_range(struct 
rev_info *revs,
   struct commit *commit)
 {
struct line_log_data *ret = NULL;
+   struct line_log_data *d;
 
ret = lookup_decoration(&revs->line_log_data, &commit->object);
+
+   for (d = ret; d; d = d->next)
+   range_set_check_invariants(&d->ranges);
+
return ret;
 }
 
-- 
1.8.2.662.g6b31d33

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] log -L overlapping ranges

[Thomas Rast]

I noticed that it doesn't like getting multiple overlapping ranges
from the user.  This fixes it.


Thomas Rast (2):
  log -L: check range set invariants when we look it up
  log -L: fix overlapping input ranges

 line-log.c  |  43 +++--
 t/t4211-line-log.sh |   6 ++
 t/t4211/expect.multiple | 104 
 t/t4211/expect.multiple-overlapping | 187 
 t/t4211/expect.multiple-superset|  59 
 5 files changed, 392 insertions(+), 7 deletions(-)
 create mode 100644 t/t4211/expect.multiple
 create mode 100644 t/t4211/expect.multiple-overlapping
 create mode 100644 t/t4211/expect.multiple-superset

-- 
1.8.2.662.g6b31d33

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Collective wisdom about repos on NFS accessed by concurrent clients (== corruption!?)

[Thomas Rast]

Kenneth Ölwing  writes:

> Basically, I'm at a place where I'm considering giving up getting this
> to work reliably. In general, my setup work really fine, except for
> the itty-bitty detail that when I put pressure on things I tend to get
> into various kinds of trouble with the central repo being corrupted.
>
> Can anyone authoritatively state anything either way?

My non-authoritative impression was that it's supposed to work
concurrently.  Obviously something breaks:

>> My experience so far is that I eventually get repo corruption when I
>> stress it with concurrent read/write access from multiple hosts
>> (beyond any sort of likely levels, but still). Maybe I'm doing
>> something wrong, missing a configuration setting somewhere, put an
>> unfair stress on the system, there's a bona fide bug - or, given the
>> inherent difficulty in achieving perfect coherency between machines
>> on what's visible on the mount, it's just impossible (?) to truly
>> get it working under all situations.

Can you run the same tests under strace or similar, and gather the
relevant outputs?  Otherwise it's probably very hard to say what is
going wrong.

In particular we've had some reports on lustre that boiled down to
"impossible" returns from libc functions, not git issues.  It's hard to
say without some evidence.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] diffcore-pickaxe: remove unnecessary call to get_textconv()

[Simon Ruderich]

On Thu, Apr 04, 2013 at 01:48:52PM -0700, Junio C Hamano wrote:
> If I am reading the code correctly, it is has_changes(), which is
> used for "log -S" (not "log -G" that uses diff_grep()), that does
> the unnecessary get_textconv() unconditionally.  The way diff_grep()
> divides the work to make fill_one() responsible for filling the
> textconv as necessary is internally consistent, and there is no
> unnecessary call.

Yes, of course. I meant has_changes() which has the unnecessary
call.

> Perhaps...
>
>   The fill_one() function is responsible for finding and
>   filling the textconv filter as necessary, and is called by
>   diff_grep() function that implements "git log -G".
>
>   The has_changes() function calls get_textconv() for two
>   sides being compared, before it checks to see if it was
>   asked to perform the pickaxe limiting with the -S option.
>   Move the code around to avoid this wastage.  After that,
>   fill_one() is called to use the textconv.
>
>   By adding get_textconv() to diff_grep() and relieving
>   fill_one() of responsibility to find the textconv filter, we
>   can avoid calling get_textconv() twice.

Sounds good to me.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line "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/3] diffcore-pickaxe: respect --no-textconv

[Simon Ruderich]

git log -S doesn't respect --no-textconv:

$ echo '*.txt diff=wrong' > .gitattributes
$ git -c diff.wrong.textconv='xxx' log --no-textconv -Sfoo
error: cannot run xxx: No such file or directory
fatal: unable to read files to diff

Reported-by: Matthieu Moy 
Signed-off-by: Simon Ruderich 
---
On Fri, Apr 05, 2013 at 09:40:21AM +0200, Matthieu Moy wrote:
> While we're there, we could test -G --no-textconv too.

Hello Matthieu,

Good idea, I've added it.

Regards
Simon

 diffcore-pickaxe.c | 12 
 t/t4209-log-pickaxe.sh | 28 
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 3124f49..26ddf00 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -86,8 +86,10 @@ static int diff_grep(struct diff_filepair *p, struct 
diff_options *o,
if (diff_unmodified_pair(p))
return 0;
 
-   textconv_one = get_textconv(p->one);
-   textconv_two = get_textconv(p->two);
+   if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
+   textconv_one = get_textconv(p->one);
+   textconv_two = get_textconv(p->two);
+   }
 
mf1.size = fill_textconv(textconv_one, p->one, &mf1.ptr);
mf2.size = fill_textconv(textconv_two, p->two, &mf2.ptr);
@@ -201,8 +203,10 @@ static int has_changes(struct diff_filepair *p, struct 
diff_options *o,
if (!o->pickaxe[0])
return 0;
 
-   textconv_one = get_textconv(p->one);
-   textconv_two = get_textconv(p->two);
+   if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
+   textconv_one = get_textconv(p->one);
+   textconv_two = get_textconv(p->two);
+   }
 
/*
 * If we have an unmodified pair, we know that the count will be the
diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
index eed7273..38fb80f 100755
--- a/t/t4209-log-pickaxe.sh
+++ b/t/t4209-log-pickaxe.sh
@@ -80,6 +80,20 @@ test_expect_success 'log -G -i (match)' '
test_cmp expect actual
 '
 
+test_expect_success 'log -G --textconv (missing textconv tool)' '
+   echo "* diff=test" >.gitattributes &&
+   test_must_fail git -c diff.test.textconv=missing log -Gfoo &&
+   rm .gitattributes
+'
+
+test_expect_success 'log -G --no-textconv (missing textconv tool)' '
+   echo "* diff=test" >.gitattributes &&
+   git -c diff.test.textconv=missing log -Gfoo --no-textconv >actual &&
+   >expect &&
+   test_cmp expect actual &&
+   rm .gitattributes
+'
+
 test_expect_success 'log -S (nomatch)' '
git log -Spicked --format=%H >actual &&
>expect &&
@@ -116,4 +130,18 @@ test_expect_success 'log -S -i (nomatch)' '
test_cmp expect actual
 '
 
+test_expect_success 'log -S --textconv (missing textconv tool)' '
+   echo "* diff=test" >.gitattributes &&
+   test_must_fail git -c diff.test.textconv=missing log -Sfoo &&
+   rm .gitattributes
+'
+
+test_expect_success 'log -S --no-textconv (missing textconv tool)' '
+   echo "* diff=test" >.gitattributes &&
+   git -c diff.test.textconv=missing log -Sfoo --no-textconv >actual &&
+   >expect &&
+   test_cmp expect actual &&
+   rm .gitattributes
+'
+
 test_done
-- 
1.8.2

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATH 3/4] remote-hg: add version checks to the marks

[Jed Brown]

Felipe Contreras  writes:

> @@ -76,12 +78,19 @@ class Marks:
>  
>  def __init__(self, path):
>  self.path = path
> +self.clear()
> +self.load()
> +
> +if self.version < VERSION:
> +self.clear()

It's friendlier to just upgrade the marks in-place. This takes less than
one second to run on repositories where full re-import would take half
an hour:

def upgrade_marks(self, hgrepo):
if self.marks_version == 1: # Convert from integer reversions to hgsha1
warn("Upgrading marks-hg from hg sequence number to SHA1")
self.marks_to_revisions = dict(
(mark, hghex(hgrepo.changelog.node(int(rev
for mark, rev in self.marks_to_revisions.iteritems())
self.revisions_to_marks = dict(
(hghex(hgrepo.changelog.node(int(rev))), mark)
for rev, mark in self.revisions_to_marks.iteritems())
self.marks_version = 2
warn("Upgrade complete")

https://github.com/buchuki/gitifyhg/commit/23a6709efd14f3e058e3a846624b7677d1e8b497#L0R195
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: trouble on windows network share

[Pyeron, Jason J CTR (US)]

[strace attachment has been removed, email being resent]

It looks like there is a race condition going on, especially since the location 
and message changes.

Could it be the file creation, file read, apply file security is happening when 
it should be file create, apply security, file read?

Looking at trace line: 1088001 (full trace attached)

1181 1087049 [main] git 9320 symlink_info::check: 0x0 = NtCreateFile 
(\??\UNC\server\share\dir1\dir2\test\.git\objects\68)
   84 1087133 [main] git 9320 symlink_info::check: not a symlink
  733 1087866 [main] git 9320 symlink_info::check: 0 = 
symlink.check(\\server\share\dir1\dir2\test\.git\objects\68, 0x288280) (0x4022)
   62 1087928 [main] git 9320 path_conv::check: 
this->path(\\server\share\dir1\dir2\test\.git\objects\68\38761d549cf76033d2e9faf5954e62839eb25d),
 has_acls(1)
   31 1087959 [main] git 9320 build_fh_pc: fh 0x61274CA0, dev 0xC3
   21 1087980 [main] git 9320 __set_errno: int fhandler_base::fhaccess(int, 
bool):367 setting errno 2
   21 1088001 [main] git 9320 fhandler_base::fhaccess: returning -1
   19 1088020 [main] git 9320 access: returning -1
  164 1088184 [main] git 9320 fhandler_pty_slave::write: pty0, write(50A474, 7)
   18 1088202 [main] git 9320 fhandler_pty_slave::write: (657): pty 
output_mutex (0xC0): waiting -1 ms
   20 1088222 [main] git 9320 fhandler_pty_slave::write: (657): pty 
output_mutex: acquired
error: 
   21 1088243 [main] git 9320 fhandler_pty_slave::write: (672): pty 
output_mutex(0xC0) released
   18 1088261 [main] git 9320 write: 7 = write(2, 0x50A474, 7)
   36 1088297 [main] git 9320 fhandler_pty_slave::write: pty0, write(288680, 
102)
   18 1088315 [main] git 9320 fhandler_pty_slave::write: (657): pty 
output_mutex (0xC0): waiting -1 ms
   18 1088333 [main] git 9320 fhandler_pty_slave::write: (657): pty 
output_mutex: acquired
Trying to write ref refs/heads/master with nonexistent object 
6838761d549cf76033d2e9faf5954e62839eb25d



> -Original Message-
> From: Jeff King
> Sent: Thursday, April 04, 2013 11:51 AM
> 
> On Thu, Apr 04, 2013 at 03:01:36PM +, Pyeron, Jason J CTR (US)
> wrote:
> 
> > I am having trouble when the .git folder is on a network share, given
> the below where should I start on my debugging?
> > [...]
> > jason.pyeron@localhost //server/share/dir/subdir/test
> > $ git add test.txt
> >
> > jason.pyeron@localhost //server/share/dir/subdir/test
> > $ git commit -m test
> > error: unable to find 8b7323820a21ebd1360e27262b3c61283c266c23
> > fatal: 8b7323820a21ebd1360e27262b3c61283c266c23 is not a valid object
> 
> Hmm. That message probably comes from:
> 
>   static int sha1_loose_object_info(const unsigned char *sha1, unsigned
> long *sizep)
>   {
>   [...]
>   map = map_sha1_file(sha1, &mapsize);
>   if (!map)
>   return error("unable to find %s", sha1_to_hex(sha1));
> 
> So we have found the object and know that it is loose, but then mmap-
> ing
> it fails. My guess is that your system does not support mmap across
> network shares (whether this is an OS issue or a cygwin limitation, I
> don't know). You could confirm it by running your "git commit" under
> strace, which I expect would show mmap returning -ENODEV or similar.
> 
> You can work around it by compiling git with NO_MMAP=1.
> 
> You might also try msysgit rather than cygwin, which seems to have its
> own win32 mmap compatibility layer.
> 
> Finally, I suspect we could include our emulate-mmap-with-pread
> compatibility wrapper all the time, and drop back to it automatically
> at
> run-time when we see ENODEV or a similar error return from mmap.
> 
> -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


smime.p7s
Description: S/MIME cryptographic signature

Re: Collective wisdom about repos on NFS accessed by concurrent clients (== corruption!?)

[Kenneth Ölwing]

Hi

Basically, I'm at a place where I'm considering giving up getting this 
to work reliably. In general, my setup work really fine, except for the 
itty-bitty detail that when I put pressure on things I tend to get into 
various kinds of trouble with the central repo being corrupted.


Can anyone authoritatively state anything either way?

TIA,

ken1

On 2013-03-28 11:22, Kenneth Ölwing wrote:

Hi,

I'm hoping to hear some wisdom on the subject so I can decide if I'm 
chasing a pipe dream or if it should be expected to work and I just 
need to work out the kinks.


Finding things like this makes it sound possible:
  http://permalink.gmane.org/gmane.comp.version-control.git/122670
but then again, in threads like this:
  http://kerneltrap.org/mailarchive/git/2010/11/14/44799
opinions are that it's just not reliable to trust.

My experience so far is that I eventually get repo corruption when I 
stress it with concurrent read/write access from multiple hosts 
(beyond any sort of likely levels, but still). Maybe I'm doing 
something wrong, missing a configuration setting somewhere, put an 
unfair stress on the system, there's a bona fide bug - or, given the 
inherent difficulty in achieving perfect coherency between machines on 
what's visible on the mount, it's just impossible (?) to truly get it 
working under all situations.


My eventual usecase is to set up a bunch of (gitolite) hosts that all 
are effectively identical and all seeing the same storage and clients 
can then be directed to any of them to get served. For the purpose of 
this query I assume it can be boiled down to be the same as n users 
working on their own workstations and accessing a central repo on an 
NFS share they all have mounted, using regular file paths. Correct?


I have a number of load-generating test scripts (that from humble 
beginnings have grown to beasts), but basically, they fork a number of 
code pieces that proceed to hammer the repo with concurrent reading 
and writing. If necessary, the scripts can be started on different 
hosts, too. It's all about the central repo so clients will retry in 
various modes whenever they get an error, up to re-cloning and 
starting over. All in all, they can yield quite a load.


On a local filesystem everything seems to be holding up fine which is 
expected. In the scenario with concurrency on top of shared NFS 
storage however, the scripts eventually fails with various problems 
(when the timing finally finds a hole, I guess) - possible to clone 
but checkouts fails, errors about refs pointing wrong and errors where 
the original repo is actually corrupted and can't be cloned from. 
Depending on test strategy, I've also had everything going fine (ops 
looks fine in my logs), but afterwards the repo has lost a branch or two.


I've experimented with various NFS settings (e.g. turning off the 
attribute cache), but haven't reached a conclusion. I do suspect that 
it just is a fact of life with a remote filesystem to have coherency 
problems with high concurrency like this but I'd be happily proven 
wrong - I'm not an expert in either NFS or git.


So, any opionions either way would be valuable, e.g. 'it should work' 
or 'it can never work 100%' is fine, as well as any suggestions. 
Obviously, the concurrency needed to make it probable to hit this 
seems so unlikely that maybe I just shouldn't worry...


I'd be happy to share scripts and whatever if someone would like to 
try it out themselves.


Thanks for your time,

ken1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] diff: allow unstuck arguments with --diff-algorithm

[John Keeping]

The argument to --diff-algorithm is mandatory, so there is no reason to
require the argument to be stuck to the option with '='.  Change this
for consistency with other Git commands.

Note that this does not change the handling of diff-algorithm in
merge-recursive.c since the primary interface to that is via the -X
option to 'git merge' where the unstuck form does not make sense.

Signed-off-by: John Keeping 
---
On Fri, Apr 05, 2013 at 01:43:16PM +0200, Thomas Rast wrote:
> John Keeping  writes:
> 
> > The argument to --diff-algorithm is mandatory, so there is no reason to
> > require the argument to be stuck to the option with '='.  Change this
> > for consistency with other Git commands.
> >
> > Note that this doesi not change the handling of diff-algorithm in
>  ^
> strayi ;-)

Thanks.

 diff.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index db952a5..e0152f8 100644
--- a/diff.c
+++ b/diff.c
@@ -3596,8 +3596,8 @@ int diff_opt_parse(struct diff_options *options, const 
char **av, int ac)
options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
else if (!strcmp(arg, "--histogram"))
options->xdl_opts = DIFF_WITH_ALG(options, HISTOGRAM_DIFF);
-   else if (!prefixcmp(arg, "--diff-algorithm=")) {
-   long value = parse_algorithm_value(arg+17);
+   else if ((argcount = parse_long_opt("diff-algorithm", av, &optarg))) {
+   long value = parse_algorithm_value(optarg);
if (value < 0)
return error("option diff-algorithm accepts \"myers\", "
 "\"minimal\", \"patience\" and 
\"histogram\"");
@@ -3605,6 +3605,7 @@ int diff_opt_parse(struct diff_options *options, const 
char **av, int ac)
DIFF_XDL_CLR(options, NEED_MINIMAL);
options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
options->xdl_opts |= value;
+   return argcount;
}
 
/* flags options */
-- 
1.8.2.540.gf023cfe

--
To unsubscribe from this list: send the line "unsubscribe 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 11/13] remote-hg: force remote push

[Jed Brown]

Joachim Schmitz  writes:

> Jed Brown wrote:
>>
>> Really?  If there is no Hg Team, why bother with an Hg upstream?
>
> Huh? the counterpart of "every user" wpuld be "some users" and not "no user" 
> or "no HG team", isn't it? 

I'm not sure what you're getting at here, but the whole premise of a
two-way git-remote-X is that the users of git-remote-X have less
influence in the project than the users of X have in the project.
Usually this means that the project workflow is whatever the X users
find comfortable rather than whatever git-remote-X users prefer.

If you are the sole publisher to a remote repository, sending pull
requests to upstream, and if they are comfortable with pulling bookmarks
(much more likely if they use a pull-request model rather than a shared
repo), then force-pushing by default is more reasonable.  An imperfect
analogy is Git's push.default=simple, which is more friendly to
beginners and to those sharing a remote.
--
To unsubscribe from this list: send the line "unsubscribe 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 00/13] remote-hg: general updates

[Felipe Contreras]

On Thu, Apr 4, 2013 at 10:14 AM, Felipe Contreras
 wrote:
> On Tue, Apr 2, 2013 at 4:23 PM, Max Horn  wrote:

>> * internally, the marks are using the hg sha1s instead of the hg rev ids. 
>> The latter are not necessarily invariant, and using the sha1s makes it much 
>> easier to recover from semi-broken states.
>
> I will investigate the pros and cons of this, but either way it's not
> something people are going to immediately need (I doubt the
> semi-broken states will happen again).

And another one bites the dust:
https://github.com/felipec/git/commits/fc/remote/hg-noteids

-- 
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] diff: allow unstuck arguments with --diff-algorithm

[Thomas Rast]

John Keeping  writes:

> The argument to --diff-algorithm is mandatory, so there is no reason to
> require the argument to be stuck to the option with '='.  Change this
> for consistency with other Git commands.
>
> Note that this doesi not change the handling of diff-algorithm in
 ^
strayi ;-)

> merge-recursive.c since the primary interface to that is via the -X
> option to 'git merge' where the unstuck form does not make sense.
>
> Signed-off-by: John Keeping 
> ---
>  diff.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index db952a5..e0152f8 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3596,8 +3596,8 @@ int diff_opt_parse(struct diff_options *options, const 
> char **av, int ac)
>   options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
>   else if (!strcmp(arg, "--histogram"))
>   options->xdl_opts = DIFF_WITH_ALG(options, HISTOGRAM_DIFF);
> - else if (!prefixcmp(arg, "--diff-algorithm=")) {
> - long value = parse_algorithm_value(arg+17);
> + else if ((argcount = parse_long_opt("diff-algorithm", av, &optarg))) {
> + long value = parse_algorithm_value(optarg);
>   if (value < 0)
>   return error("option diff-algorithm accepts \"myers\", "
>"\"minimal\", \"patience\" and 
> \"histogram\"");
> @@ -3605,6 +3605,7 @@ int diff_opt_parse(struct diff_options *options, const 
> char **av, int ac)
>   DIFF_XDL_CLR(options, NEED_MINIMAL);
>   options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
>   options->xdl_opts |= value;
> + return argcount;
>   }
>  
>   /* flags options */

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATH 4/4] remote-hg: switch from revisions to SHA-1 noteids

[Felipe Contreras]

Otherwise we won't know if revisions are replaced.

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-hg | 40 +++-
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-hg 
b/contrib/remote-helpers/git-remote-hg
index 9e124e1..162dabc 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -43,7 +43,7 @@ AUTHOR_RE = re.compile('^([^<>]+?)? ?<([^<>]*)>$')
 AUTHOR_HG_RE = re.compile('^(.*?) ?<(.*?)(?:>(.+)?)?$')
 RAW_AUTHOR_RE = re.compile('^(\w+) (?:(.+)? )?<(.*)> (\d+) ([+-]\d+)')
 
-VERSION = 1
+VERSION = 2
 
 def die(msg, *args):
 sys.stderr.write('ERROR: %s\n' % (msg % args))
@@ -104,7 +104,7 @@ class Marks:
 self.version = tmp.get('version', 1)
 
 for rev, mark in self.marks.iteritems():
-self.rev_marks[mark] = int(rev)
+self.rev_marks[mark] = rev
 
 def dict(self):
 return { 'tips': self.tips, 'marks': self.marks, 'last-mark' : 
self.last_mark, 'version' : self.version }
@@ -116,23 +116,23 @@ class Marks:
 return str(self.dict())
 
 def from_rev(self, rev):
-return self.marks[str(rev)]
+return self.marks[rev]
 
 def to_rev(self, mark):
 return self.rev_marks[mark]
 
 def get_mark(self, rev):
 self.last_mark += 1
-self.marks[str(rev)] = self.last_mark
+self.marks[rev] = self.last_mark
 return self.last_mark
 
 def new_mark(self, rev, mark):
-self.marks[str(rev)] = mark
+self.marks[rev] = mark
 self.rev_marks[mark] = rev
 self.last_mark = mark
 
 def is_marked(self, rev):
-return self.marks.has_key(str(rev))
+return self.marks.has_key(rev)
 
 def get_tip(self, branch):
 return self.tips.get(branch, 0)
@@ -305,7 +305,7 @@ def get_repo(url, alias):
 
 def rev_to_mark(rev):
 global marks
-return marks.from_rev(rev)
+return marks.from_rev(rev.hex())
 
 def mark_to_rev(mark):
 global marks
@@ -316,6 +316,10 @@ def export_ref(repo, name, kind, head):
 
 ename = '%s/%s' % (kind, name)
 tip = marks.get_tip(ename)
+if tip in repo:
+tip = repo[tip].rev()
+else:
+tip = 0
 
 # mercurial takes too much time checking this
 if tip and tip == head.rev():
@@ -382,16 +386,16 @@ def export_ref(repo, name, kind, head):
 print 'reset %s/%s' % (prefix, ename)
 
 print "commit %s/%s" % (prefix, ename)
-print "mark :%d" % (marks.get_mark(rev))
+print "mark :%d" % (marks.get_mark(c.hex()))
 print "author %s" % (author)
 print "committer %s" % (committer)
 print "data %d" % (len(desc))
 print desc
 
 if len(parents) > 0:
-print "from :%s" % (rev_to_mark(parents[0].rev()))
+print "from :%s" % (rev_to_mark(parents[0]))
 if len(parents) > 1:
-print "merge :%s" % (rev_to_mark(parents[1].rev()))
+print "merge :%s" % (rev_to_mark(parents[1]))
 
 for f in modified:
 export_file(c.filectx(f))
@@ -406,10 +410,10 @@ def export_ref(repo, name, kind, head):
 
 # make sure the ref is updated
 print "reset %s/%s" % (prefix, ename)
-print "from :%u" % rev_to_mark(head.rev())
+print "from :%u" % rev_to_mark(head)
 print
 
-marks.set_tip(ename, head.rev())
+marks.set_tip(ename, head.hex())
 
 def export_tag(repo, tag):
 export_ref(repo, tag, 'tags', repo[tag])
@@ -626,12 +630,12 @@ def parse_commit(parser):
 extra['committer'] = "%s %u %u" % committer
 
 if from_mark:
-p1 = repo.changelog.node(mark_to_rev(from_mark))
+p1 = mark_to_rev(from_mark)
 else:
 p1 = '\0' * 20
 
 if merge_mark:
-p2 = repo.changelog.node(mark_to_rev(merge_mark))
+p2 = mark_to_rev(merge_mark)
 else:
 p2 = '\0' * 20
 
@@ -672,10 +676,8 @@ def parse_commit(parser):
 
 encoding.encoding = tmp
 
-rev = repo[node].rev()
-
 parsed_refs[ref] = node
-marks.new_mark(rev, commit_mark)
+marks.new_mark(node, commit_mark)
 
 def parse_reset(parser):
 global parsed_refs
@@ -691,8 +693,8 @@ def parse_reset(parser):
 from_mark = parser.get_mark()
 parser.next()
 
-node = parser.repo.changelog.node(mark_to_rev(from_mark))
-parsed_refs[ref] = hghex(node)
+rev = mark_to_rev(from_mark)
+parsed_refs[ref] = rev
 
 def parse_tag(parser):
 name = parser[1]
-- 
1.8.2

--
To unsubscribe from this list: send the line "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] git-merge(1): document diff-algorithm option to merge-recursive

[John Keeping]

Commit 07924d4 (diff: Introduce --diff-algorithm command line option
2013-01-16) added diff-algorithm as a parameter to the recursive merge
strategy but did not document it.  Do so.

Signed-off-by: John Keeping 
---
 Documentation/merge-strategies.txt | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/merge-strategies.txt 
b/Documentation/merge-strategies.txt
index 66db802..49a9a7d 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -48,6 +48,12 @@ patience;;
this when the branches to be merged have diverged wildly.
See also linkgit:git-diff[1] `--patience`.
 
+diff-algorithm=[patience|minimal|histogram|myers];;
+   Tells 'merge-recursive' to use a different diff algorithm, which
+   can help avoid mismerges that occur due to unimportant matching
+   lines (such as braces from distinct functions).  See also
+   linkgit:git-diff[1] `--diff-algorithm`.
+
 ignore-space-change;;
 ignore-all-space;;
 ignore-space-at-eol;;
-- 
1.8.2.540.gf023cfe

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATH 3/4] remote-hg: add version checks to the marks

[Felipe Contreras]

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-hg | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-hg 
b/contrib/remote-helpers/git-remote-hg
index 02fda2d..9e124e1 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -43,6 +43,8 @@ AUTHOR_RE = re.compile('^([^<>]+?)? ?<([^<>]*)>$')
 AUTHOR_HG_RE = re.compile('^(.*?) ?<(.*?)(?:>(.+)?)?$')
 RAW_AUTHOR_RE = re.compile('^(\w+) (?:(.+)? )?<(.*)> (\d+) ([+-]\d+)')
 
+VERSION = 1
+
 def die(msg, *args):
 sys.stderr.write('ERROR: %s\n' % (msg % args))
 sys.exit(1)
@@ -76,12 +78,19 @@ class Marks:
 
 def __init__(self, path):
 self.path = path
+self.clear()
+self.load()
+
+if self.version < VERSION:
+self.clear()
+self.version = VERSION
+
+def clear(self):
 self.tips = {}
 self.marks = {}
 self.rev_marks = {}
 self.last_mark = 0
-
-self.load()
+self.version = 0
 
 def load(self):
 if not os.path.exists(self.path):
@@ -92,12 +101,13 @@ class Marks:
 self.tips = tmp['tips']
 self.marks = tmp['marks']
 self.last_mark = tmp['last-mark']
+self.version = tmp.get('version', 1)
 
 for rev, mark in self.marks.iteritems():
 self.rev_marks[mark] = int(rev)
 
 def dict(self):
-return { 'tips': self.tips, 'marks': self.marks, 'last-mark' : 
self.last_mark }
+return { 'tips': self.tips, 'marks': self.marks, 'last-mark' : 
self.last_mark, 'version' : self.version }
 
 def store(self):
 json.dump(self.dict(), open(self.path, 'w'))
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATH 1/4] remote-hg: shuffle some code

[Felipe Contreras]

In preparation to shift to SHA-1's.

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-hg | 27 +++
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-hg 
b/contrib/remote-helpers/git-remote-hg
index d82eb2d..a8591a2 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -63,6 +63,9 @@ def hgmode(mode):
 def hghex(node):
 return hg.node.hex(node)
 
+def hgbin(node):
+return hg.node.bin(node)
+
 def get_config(config):
 cmd = ['git', 'config', '--get', config]
 process = subprocess.Popen(cmd, stdout=subprocess.PIPE)
@@ -207,7 +210,7 @@ def get_filechanges(repo, ctx, parent):
 removed = set()
 
 cur = ctx.manifest()
-prev = repo[parent].manifest().copy()
+prev = parent.manifest().copy()
 
 for fn in cur:
 if fn in prev:
@@ -326,7 +329,7 @@ def export_ref(repo, name, kind, head):
 else:
 committer = author
 
-parents = [p for p in repo.changelog.parentrevs(rev) if p >= 0]
+parents = [repo[p] for p in repo.changelog.parentrevs(rev) if p >= 0]
 
 if len(parents) == 0:
 modified = c.manifest().keys()
@@ -372,9 +375,9 @@ def export_ref(repo, name, kind, head):
 print desc
 
 if len(parents) > 0:
-print "from :%s" % (rev_to_mark(parents[0]))
+print "from :%s" % (rev_to_mark(parents[0].rev()))
 if len(parents) > 1:
-print "merge :%s" % (rev_to_mark(parents[1]))
+print "merge :%s" % (rev_to_mark(parents[1].rev()))
 
 for f in modified:
 export_file(c.filectx(f))
@@ -389,10 +392,10 @@ def export_ref(repo, name, kind, head):
 
 # make sure the ref is updated
 print "reset %s/%s" % (prefix, ename)
-print "from :%u" % rev_to_mark(rev)
+print "from :%u" % rev_to_mark(head.rev())
 print
 
-marks.set_tip(ename, rev)
+marks.set_tip(ename, head.rev())
 
 def export_tag(repo, tag):
 export_ref(repo, tag, 'tags', repo[tag])
@@ -651,7 +654,7 @@ def parse_commit(parser):
 tmp = encoding.encoding
 encoding.encoding = 'utf-8'
 
-node = repo.commitctx(ctx)
+node = hghex(repo.commitctx(ctx))
 
 encoding.encoding = tmp
 
@@ -675,7 +678,7 @@ def parse_reset(parser):
 parser.next()
 
 node = parser.repo.changelog.node(mark_to_rev(from_mark))
-parsed_refs[ref] = node
+parsed_refs[ref] = hghex(node)
 
 def parse_tag(parser):
 name = parser[1]
@@ -720,10 +723,10 @@ def do_export(parser):
 elif ref.startswith('refs/tags/'):
 tag = ref[len('refs/tags/'):]
 if mode == 'git':
-msg = 'Added tag %s for changeset %s' % (tag, hghex(node[:6]));
-parser.repo.tag([tag], node, msg, False, None, {})
+msg = 'Added tag %s for changeset %s' % (tag, node[:6]);
+parser.repo.tag([tag], hgbin(node), msg, False, None, {})
 else:
-parser.repo.tag([tag], node, None, True, None, {})
+parser.repo.tag([tag], hgbin(node), None, True, None, {})
 print "ok %s" % ref
 else:
 # transport-helper/fast-export bugs
@@ -735,7 +738,7 @@ def do_export(parser):
 # handle bookmarks
 for bmark, node in p_bmarks:
 ref = 'refs/heads' + bmark
-new = hghex(node)
+new = node
 
 if bmark in bmarks:
 old = bmarks[bmark].hex()
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATH 2/4] remote-hg: improve node traversing

[Felipe Contreras]

We won't be able to count the unmarked commits, but we are not going to
be able to do that anyway when we switch to SHA-1 ids.

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-hg | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-hg 
b/contrib/remote-helpers/git-remote-hg
index a8591a2..02fda2d 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -314,12 +314,16 @@ def export_ref(repo, name, kind, head):
 revs = xrange(tip, head.rev() + 1)
 count = 0
 
-revs = [rev for rev in revs if not marks.is_marked(rev)]
-
 for rev in revs:
 
 c = repo[rev]
-(manifest, user, (time, tz), files, desc, extra) = 
repo.changelog.read(c.node())
+node = c.node()
+
+if marks.is_marked(c.hex()):
+count += 1
+continue
+
+(manifest, user, (time, tz), files, desc, extra) = 
repo.changelog.read(node)
 rev_branch = extra['branch']
 
 author = "%s %d %s" % (fixup_user(user), time, gittz(tz))
-- 
1.8.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATH 0/4] remoe-hg: switch to SHA-1 ids

[Felipe Contreras]

Hi,

As some people suggested this is necessary for some use-cases, because revision
id numbers can change and point to different revisions.

Seems to work fine, but I wouldn't merge it just yet.

Felipe Contreras (4):
  remote-hg: shuffle some code
  remote-hg: improve node traversing
  remote-hg: add version checks to the marks
  remote-hg: switch from revisions to SHA-1 noteids

 contrib/remote-helpers/git-remote-hg | 77 ++--
 1 file changed, 48 insertions(+), 29 deletions(-)

-- 
1.8.2

--
To unsubscribe from this list: send the line "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] diff: allow unstuck arguments with --diff-algorithm

[John Keeping]

The argument to --diff-algorithm is mandatory, so there is no reason to
require the argument to be stuck to the option with '='.  Change this
for consistency with other Git commands.

Note that this doesi not change the handling of diff-algorithm in
merge-recursive.c since the primary interface to that is via the -X
option to 'git merge' where the unstuck form does not make sense.

Signed-off-by: John Keeping 
---
 diff.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index db952a5..e0152f8 100644
--- a/diff.c
+++ b/diff.c
@@ -3596,8 +3596,8 @@ int diff_opt_parse(struct diff_options *options, const 
char **av, int ac)
options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
else if (!strcmp(arg, "--histogram"))
options->xdl_opts = DIFF_WITH_ALG(options, HISTOGRAM_DIFF);
-   else if (!prefixcmp(arg, "--diff-algorithm=")) {
-   long value = parse_algorithm_value(arg+17);
+   else if ((argcount = parse_long_opt("diff-algorithm", av, &optarg))) {
+   long value = parse_algorithm_value(optarg);
if (value < 0)
return error("option diff-algorithm accepts \"myers\", "
 "\"minimal\", \"patience\" and 
\"histogram\"");
@@ -3605,6 +3605,7 @@ int diff_opt_parse(struct diff_options *options, const 
char **av, int ac)
DIFF_XDL_CLR(options, NEED_MINIMAL);
options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
options->xdl_opts |= value;
+   return argcount;
}
 
/* flags options */
-- 
1.8.2.540.gf023cfe

--
To unsubscribe from this list: send the line "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 diff --quiet on dirty tree sometimes erroneously exits with status 0

[Mike Crowe]

I'm seeing a strange problem where "git diff --quiet" sometimes returns an
exit code of zero even though the tree is dirty and other invocations of
"git diff --quiet" in the same directory return an exit code of 1. I'm
using git v1.8.2 from Debian unstable but I've also seen the problem when
running v1.8.2 built from source and using older versions from Debian. The
repository in question is a submodule.

Unfortunately this bad behaviour only happens deep inside a variable
expansion under OpenEmbedded's bitbake so investigation is not
straightforward. It's possible that when the bad behaviour occurs other
"git diff" and similar informational commands are being run on the same
repository in parallel. I have been unable to provoke the problem by more
conventional means. :( I can reproduce it on a separate repository on the
same machine though.

Using "git diff --exit-code --no-ext-diff" instead seems to avoid the
problem but unfortunately so does adding a sleep before calling "git diff
--quiet" so that result isn't necessarily significant.

I have been able to strace the problematic case. Diffing the straces and
ignoring unrelated differences such as addresses yields  (+ =
diff returns 0, - = diff returns 1):

 [...lots of omitted stuff...]
 open("/home/mac/src/oe/sources/linux/.gitmodules", O_RDONLY) = -1 ENOENT
 (No such file or directory)
 access("/etc/gitconfig", R_OK)  = -1 ENOENT (No such file or
 directory)
 access("/home/mac/.config/git/config", R_OK) = -1 ENOENT (No such file or
 directory)
 access("/home/mac/.gitconfig", R_OK)= 0
 open("/home/mac/.gitconfig", O_RDONLY)  = 20
 fstat(20, {st_mode=S_IFREG|0644, st_size=424, ...}) = 0
0x7f22bfa18000
 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7f79414f1000
 read(20, "[color]\n\tui = true\n[core]\n#\tpage"..., 4096) = 424
 read(20, "", 4096)  = 0
 close(20)   = 0
 munmap(0x7f79414f1000, 4096)= 0
 access("/home/mac/src/oe/.git/modules/sources/linux/config", R_OK) = 0
 open("/home/mac/src/oe/.git/modules/sources/linux/config", O_RDONLY) = 20
 fstat(20, {st_mode=S_IFREG|0644, st_size=1467, ...}) = 0
 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7f79414f1000
 read(20, "[core]\n\trepositoryformatversion "..., 4096) = 1467
 read(20, "", 4096)  = 0
 close(20)   = 0
 munmap(0x7f79414f1000, 4096)= 0
 chdir("/home/mac/src/oe/sources/linux") = 0
 lstat(".gitignore", {st_mode=S_IFREG|0644, st_size=955, ...}) = 0

At this point the diff that erroneously returned zero went off and did:

+lstat(".gitignore", {st_mode=S_IFREG|0644, st_size=955, ...}) = 0
+open("/home/mac/src/oe/.git/modules/sources/linux/objects/pack",
O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 20
+getdents(20, /* 4 entries */, 32768)= 192
+access("/home/mac/src/oe/.git/modules/sources/linux/objects/pack/pack-2711d98bc06e8caa9c8d8e10ee40e645e6d05f18.keep",
F_OK) = -1 ENOENT (No such file or directory)
+stat("/home/mac/src/oe/.git/modules/sources/linux/objects/pack/pack-2711d98bc06e8caa9c8d8e10ee40e645e6d05f18.pack",
{st_mode=S_IFREG|0444, st_size=148768254, ...}) = 0
+getdents(20, /* 0 entries */, 32768)= 0
+close(20)   = 0

..and spent a while re-reading config files before continuing to recurse
over all the files but whilst the diff that provides the correct result
merely lstats each file this one reads them too:

 lstat("COPYING", {st_mode=S_IFREG|0644, st_size=18693, ...}) = 0
+open("COPYING", O_RDONLY)   = 21
+read(21, "\n   NOTE! This copyright does *n"..., 18693) = 18693
+close(21)   = 0

When it comes across the repository file that has been changed the
behaviour of the invocation that provides the correct result is rather
different:

 lstat("drivers/net/bcmgenet/bcmgenet.c", {st_mode=S_IFREG|0644,
 st_size=114290, ...}) = 0
-lstat("drivers/net/bcmgenet/bcmgenet.c", {st_mode=S_IFREG|0644,
st_size=114290, ...}) = 0
-open("/home/mac/src/oe/.git/modules/sources/linux/objects/pack",
O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 20
-getdents(20, /* 4 entries */, 32768)= 192
-access("/home/mac/src/oe/.git/modules/sources/linux/objects/pack/pack-2711d98bc06e8caa9c8d8e10ee40e645e6d05f18.keep",
F_OK) = -1 ENOENT (No such file or directory)
-stat("/home/mac/src/oe/.git/modules/sources/linux/objects/pack/pack-2711d98bc06e8caa9c8d8e10ee40e645e6d05f18.pack",
{st_mode=S_IFREG|0444, st_size=148768254, ...}) = 0
-getdents(20, /* 0 entries */, 32768)= 0
-close(20)   = 0
-open("/home/mac/src/oe/.git/modules/sources/linux/objects/info/alternates",
O_RDONLY|O_NOATIME) = -1 ENOENT (No such file or directory)
-open("/home/mac/src/oe/.git/modules/sources/linux/objects/pack/pack-2711d98bc06e8caa9c8d8e10ee40e645e6d05f18.idx",
O_RDONLY|O_NOATIME) = 20
-fstat(20, {st_mode=S_IFREG|0444, st_size=1945504, ...}) = 0
-mmap

[PATCH] count-objects doc: document use of kibibytes

[Mihai Capotă]

Document the use of kibibytes, not kilobytes, in the output of count-objects
and the reason for not correcting the output.

Also, make cvsimport comment and variable name reflect unit actually used.

Signed-off-by: Mihai Capotă 
---
 Documentation/git-count-objects.txt |7 +++
 git-cvsimport.perl  |6 +++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-count-objects.txt 
b/Documentation/git-count-objects.txt
index 23c80ce..d562dad 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -26,6 +26,13 @@ OPTIONS
and number of objects that can be removed by running
`git prune-packed`.
 
+
+BUGS
+
+Consumed space is actually expressed in kibibytes, not kilobytes as stated in
+the output. The output is kept as it is for compatibility reasons.
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 73d367c..6803f04 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -1126,12 +1126,12 @@ unless ($opt_P) {
 }
 
 # The heuristic of repacking every 1024 commits can leave a
-# lot of unpacked data.  If there is more than 1MB worth of
+# lot of unpacked data.  If there is more than 1MiB worth of
 # not-packed objects, repack once more.
 my $line = `git count-objects`;
 if ($line =~ /^(\d+) objects, (\d+) kilobytes$/) {
-  my ($n_objects, $kb) = ($1, $2);
-  1024 < $kb
+  my ($n_objects, $kib) = ($1, $2);
+  1024 < $kib
 and system(qw(git repack -a -d));
 }
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe 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] count-objects: output "KiB" instead of "kilobytes"

[Mihai Capotă]

On Thu, Apr 4, 2013 at 6:27 PM, Junio C Hamano  wrote:
> Mihai Capotă  writes:
>
>> The git manual contains an explicit warning about the output of a
>> porcelain command changing: "The interface to Porcelain commands on
>> the other hand are subject to change in order to improve the end user
>> experience."
>
> Yeah, I know that, as I wrote it ;-)
>
> Aside from count-object being not exactly a Porcelain, the statement
> does not give us a blank check to make random changes as we see fit.
> There needs to be a clear improvement.
>
> I am just having a hard time weighing the benefit of using more
> accurate kibibytes over kilobytes and the possible downside of
> breaking other peoples' tools.
>
> Perhaps it would be alright if the change was accompanied by a
> warning in the Release Notes to say something like:
>
> If you have scripts that decide when to run "git repack" by
> parsing the output from "git count-objects", this release
> may break them.  Sorry about that.  One of the scripts
> shipped by git-core itself also had to be adjusted.  The
> command reports the total diskspace used to store loose
> objects in kibibytes, but it was labelled as "kilobytes".
> The number now is shown with "KiB", e.g. "6750 objects,
> 50928 KiB".
>
> You may want to consider updating such scripts to always
> call "git gc --auto" to let it decide when to repack for
> you.
>
> Also, I suspect that for the purpose of this exact output field,
> nobody cares the difference between kibibytes and kilobytes.
> Depending on the system, we add up either st.st_blocks or st.st_size
> and the result is not that exact as "how much diskspace is
> consumed".

I agree completely. I think the release notes warning is a good plan.
Just in case you decide against it, I'm also sending a completely
different patch to document the issue.

Mihai
--
To unsubscribe from this list: send the line "unsubscribe 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 13/13] remote-hg: push to the appropriate branch

[Felipe Contreras]

On Thu, Apr 4, 2013 at 10:50 AM, Junio C Hamano  wrote:
> Felipe Contreras  writes:
>
>> From: Dusty Phillips 
>>
>> Signed-off-by: Felipe Contreras 
>> ---
>>  contrib/remote-helpers/git-remote-hg | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/contrib/remote-helpers/git-remote-hg 
>> b/contrib/remote-helpers/git-remote-hg
>> index 56b3641..d82eb2d 100755
>> --- a/contrib/remote-helpers/git-remote-hg
>> +++ b/contrib/remote-helpers/git-remote-hg
>> @@ -625,6 +625,10 @@ def parse_commit(parser):
>>  if merge_mark:
>>  get_merge_files(repo, p1, p2, files)
>>
>> +# Check if the ref is supposed to be a named branch
>> +if ref.startswith('refs/heads/branches/'):
>> +extra['branch'] = ref.rpartition('/')[2]
>> +
>
> Is this meant to cut everything after "refs/heads/branches/", or cut
> at the last slash?  I know rpartition does the latter, but I was
> wondering if we see "refs/heads/branches/foo/bar" as its input here.

Good catch, it should be the former.

-- 
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] Documentation/git-commit: reword the --amend explanation

[Carlos Martín Nieto]

On Thu, 2013-04-04 at 10:04 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto  writes:
> 
> > On Wed, 2013-04-03 at 23:25 +0100, Philip Oakley wrote:
> >
> >>  + Replace the tip of the current branch with a fresh commit.
> >> [or updated commit, or new commit, or ...]
> >
> > Ack, we should lead with the goal, I'd go for the
> >
> > "Replace the tip of the current branch with a new commit"
> >
> > wording.
> 
> We would want to be careful to make sure that the reader understands
> that the "new commit" is created by running this command (i.e. it is
> not like "git branch -f $current_branch $new_commit"), but other
> than that, sounds sensible.
> 
> Perhaps like this?
> 
>  Documentation/git-commit.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index bc919ac..61266d8 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -190,8 +190,8 @@ OPTIONS
>   without changing its commit message.
>  
>  --amend::
> - Create a new commit and replace the tip of the current
> - branch. The recorded tree is prepared as usual (including
> + Replace the tip of the current branch by creating a new
> + commit. The recorded tree is prepared as usual (including
>   the effect of the `-i` and `-o` options and explicit
>   pathspec), and the message from the original commit is used
>   as the starting point, instead of an empty message, when no
> 

Looks good, yeah. This should stop anybody thinking that they can
replace the tip with an arbitrary commit.

   cmn


--
To unsubscribe from this list: send the line "unsubscribe 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 00/13] remote-hg: general updates

[Felipe Contreras]

Antoine Pelisse wrote:
> >> * internally, the marks are using the hg sha1s instead of the hg rev ids. 
> >> The latter are not necessarily invariant, and using the sha1s makes it 
> >> much easier to recover from semi-broken states.
> >
> > I doubt this makes any difference (except for more wasted space).
> 
> I think this is definitely wrong. If you happen to strip a changeset
> from the mercurial repository, and redo a completely different commit
> on top of it, the new commit will never be seen on git end (because it
> will have the same rev id and will thus be identified as identical
> from git point of view).

That is true. I've written the code to use SHA-1 nodeids inststead andd this
particular scenario works correctly, I just need to figure a way to discard the
old ones.

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/7] sha1_file, link: write link objects to the database

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> This needs to be replaced by a .git/config parser.  However, I can't
> use the parser from config.c as-it-is, because it expects a section
> like [core] to be present.  So, we have to refactor it to optionally
> parse section-less configs.

Er, sorry about the thinko: I meant that edit-link should use the
.git/config parser.  This one is just fine.
--
To unsubscribe from this list: send the line "unsubscribe 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 02/12] pretty: share code between format_decoration and show_decorations

[Jakub Narębski]

Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy   writes:

>> diff --git a/log-tree.h b/log-tree.h
>> index 9140f48..e6a2da5 100644
>> --- a/log-tree.h
>> +++ b/log-tree.h
>> @@ -13,6 +13,9 @@ int log_tree_diff_flush(struct rev_info *);
>>  int log_tree_commit(struct rev_info *, struct commit *);
>>  int log_tree_opt_parse(struct rev_info *, const char **, int);
>>  void show_log(struct rev_info *opt);
>> +void format_decoration(struct strbuf *sb,
>> +   const struct commit *commit,
>> +   int use_color);
> 
> I think you can fit these on a single line, especially if you drop
> the unused variable names (they help when there are more than one
> parameter of the same type to document the order of the arguments,
> but that does not apply here).  That would help people who run
> "grep" on the header files without using CTAGS/ETAGS.

Well, I think "int use_color" should be left with variable name,
don't you think?

-- 
Jakub Narębski

--
To unsubscribe from this list: send the line "unsubscribe 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/3] diffcore-pickaxe: respect --no-textconv

[Matthieu Moy]

Simon Ruderich  writes:

> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -116,4 +116,18 @@ test_expect_success 'log -S -i (nomatch)' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'log -S --textconv (missing textconv tool)' '
> + echo "* diff=test" >.gitattributes &&
> + test_must_fail git -c diff.test.textconv=missing log -Sfoo &&
> + rm .gitattributes
> +'
> +
> +test_expect_success 'log -S --no-textconv (missing textconv tool)' '
> + echo "* diff=test" >.gitattributes &&
> + git -c diff.test.textconv=missing log -Sfoo --no-textconv >actual &&
> + >expect &&
> + test_cmp expect actual &&
> + rm .gitattributes
> +'

While we're there, we could test -G --no-textconv too.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Composing git repositories

[Jens Lehmann]

Am 05.04.2013 07:27, schrieb Duy Nguyen:
> On Fri, Apr 5, 2013 at 3:53 PM, Junio C Hamano  wrote:
>>> A brief summary or outcome from these links in the comment would
>>> be nice.
>>
>> A summary of what to consider in Documentation/technical/ somewhere
>> may be a very welcome addition.  Thanks for volunteering ;-).
> 
> No thanks :-) I did not really follow this thread to make such
> contribution. Still have some work to do with other topics.

I'll do that (unless someone else steps up). I had these links in
my todo list with the intent of adding them to my github page, but
I agree they make more sense in Documentation/technical. Will see
when I find a time slot to read through the whole threads to give
them meaningful captions.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/7] sha1_file, link: write link objects to the database

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> diff --git a/link.c b/link.c
> index bb20a51..349646d 100644
> --- a/link.c
> +++ b/link.c
> @@ -20,8 +20,30 @@ struct link *lookup_link(const unsigned char *sha1)
>
>  int parse_link_buffer(struct link *item, void *buffer, unsigned long size)
>  {
> +   char *bufptr = buffer;
> +   char *tail = buffer + size;
> +   char *eol;
> +
> if (item->object.parsed)
> return 0;
> item->object.parsed = 1;
> +   while (bufptr < tail) {
> +   eol = strchr(bufptr, '\n');
> +   *eol = '\0';
> +   if (!prefixcmp(bufptr, "upstream_url = "))
> +   item->upstream_url = xstrdup(bufptr + 15);
> +   else if (!prefixcmp(bufptr, "checkout_rev = "))
> +   item->checkout_rev = xstrdup(bufptr + 15);
> +   else if (!prefixcmp(bufptr, "ref_name = "))
> +   item->ref_name = xstrdup(bufptr + 11);
> +   else if (!prefixcmp(bufptr, "floating = "))
> +   item->floating = atoi(bufptr + 11);
> +   else if (!prefixcmp(bufptr, "statthrough = "))
> +   item->statthrough = atoi(bufptr + 14);
> +   else
> +   return error("Parse error in link buffer");
> +
> +   bufptr = eol + 1;
> +   }
> return 0;
>  }

This needs to be replaced by a .git/config parser.  However, I can't
use the parser from config.c as-it-is, because it expects a section
like [core] to be present.  So, we have to refactor it to optionally
parse section-less configs.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/7] clone: introduce clone.submodulegitdir

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> diff --git a/builtin/clone.c b/builtin/clone.c
> index e0aaf13..1b798e6 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -658,11 +659,22 @@ static void write_refspec_config(const char* 
> src_ref_prefix,
> strbuf_release(&value);
>  }
>
> +static int git_clone_config(const char *var, const char *value, void *cb)
> +{
> +   if (!strcmp(var, "clone.submodulegitdir")) {
> +   git_config_string(&submodule_gitdir, var, value);
> +   return 0;
> +   }
> +   return git_default_config(var, value, cb);
> +}

submodule_gitdir can be a human-path, and we will need real_path() to
turn it into a concrete path.  Why doesn't real_path() expand ~ yet?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/7] sha1_file: write ref_name to link object

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Great.  Now, we just have to write refs/modules//* at
> commit-time.

Actually, we have to update things in refs/modules/ everytime we
update things in refs/heads/.  In the case of a 'git branch -M' for
example, refs/heads/ is rewritten to refs/heads/:
similarly, refs/modules// needs to be moved to
refs/modules//.

There is one special case worth mentioning.  Let's say I'm committing
changes to link objects to a detached HEAD, b8bb3f.  Then, I write
refs/modules/b8bb3f/ at commit-time.  A subsequent 'checkout -b' that
updates refs/heads/ will just have to move
refs/modules/b8bb3f/ to refs/modules//.

The caveat is that I might commit to the detached HEAD and leave it
hanging around until the next gc.  So, gc will need to remove
refs/modules/b8bb3f/ too, but that's not a pressing issue at the
moment.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html