Re: [PATCH] send-email: support NNTP

[Łukasz Stelmach]

It was 2013-04-24 śro 18:17, when Junio C Hamano wrote:
 l.stelm...@samsung.com (Łukasz Stelmach) writes:

 It was 2013-04-23 wto 17:02, when Junio C Hamano wrote:
 Łukasz Stelmach l.stelm...@samsung.com writes:

 Enable sending patches to NNTP servers (Usenet, Gmane).
 ---

 The patch implements support for sending messages to groups on NNTP
 serviers.

 Cute.

 A Perl guru might want to encapsulate the differences between $smtp
 and $nntp codepaths into two Perl modules, but it looks like a good
 starting point.

 You mean *one* perl module like Git::EmailTransport which hides the
 differences.

 What I meant was one class to handle SMTP and another for NNTP.

 You look at the --protocol option, choose one of these classes, and
 initialize an instance of the chosen class.

 You can ask the chosen class to instantiate an instance without
 if/else cascade like this:

 +
 +# Transport specific setup
 +my ($email_authuser, $email_authpass);
 +if ($email_protocol eq 'nntp') {
 +$email_authuser = $nntp_authuser;
 +$email_authuser = $nntp_authuser;
 +@initial_to = @initial_cc = @bcclist = ();
 +$to_cmd = $cc_cmd = undef;
 +$no_cc = $no_bcc = 1;
 +} else {
 +$email_authuser = $smtp_authuser;
 +$email_authpass = $smtp_authpass;
 +$newsgroups_cmd = undef;
 +}
 +

[...]

OK, I see. Good point. Where would you recommend me to put these modules
and how to name them? I mean I don't want to make to much mess here (;

-- 
Łukasz Stelmach
Software wizzard
Samsung Poland RD Center

--
To unsubscribe from this list: send the line unsubscribe 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] send-email: support NNTP

[Łukasz Stelmach]

It was 2013-04-25 czw 00:41, when Junio C Hamano wrote:
 Thomas Rast tr...@inf.ethz.ch writes:

 Łukasz Stelmach l.stelm...@samsung.com writes:

 Enable sending patches to NNTP servers (Usenet, Gmane).

 I'm surprised Junio didn't mention this: your patch lacks the
 Signed-off-by.

 Heh, that was because I took the patch as an early preview and not a
 final submission.  It came without documentation updates, and also
 it seemed to break t9001 for whatever reason.

I suppose that means I should write tests too. OK.

-- 
Łukasz Stelmach
Software wizzard
Samsung Poland RD Center

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

Re: [BUG] Highly inconsistent diff UI

[Thomas Rast]

Ramkumar Ramachandra artag...@gmail.com writes:

 Thomas Rast wrote:
 I personally think we have enough magic revision syntax to last at least
 another decade.  If you propose to add some, please make a patch that we
 can cook in next for a few release cycles and then conduct a straw poll
 if people actually use it.

 Isn't it obviously incredibly useful?  I'm working on a topic branch I
 need to send out to git.git, and I want see how my WIP looks: should I
 have to rebase on master just to see this?

 Why such a huge resistance against such a small feature?  Can you
 think of ways in which it is myopic (and therefore a pain to keep
 supporting, if we find it undesirable)?

What's the problem with cooking it for a while?  You can start using it
immediately.  I'm just somewhat annoyed that the syntax is rapidly
converging to Perl-style line noise.

I already hate half of the existing syntax, and I cannot remember using
^! (except while investigating what 'git diff C^!' does and why not),
^@, @{-N} (only the related 'git checkout -'), @{date} and @{relative},
^{}, :/foo, and ^{/foo}, *at all*.

In fact I had to look up the second half of that list on the manpage.

That's not to say that they are not useful for *someone*.  But it does
motivate my suggestion that unless we have tried it and *found* that
someone for a new syntax, let's not make it any more magic.

 On a related note- In my opinion, :/ is broken, because it blocks
 composition completely.  I would've really liked {:/quuxery}~3.

I guess this constitutes an argument in my favor (i.e. that the syntax
is too convoluted to understand and know):

^{/foo} is the same as :/foo, except it properly groups.

-- 
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: [BUG] Highly inconsistent diff UI

[Ramkumar Ramachandra]

Okay, so the point I was trying to make is:

The range version of $(git merge-base A B) B is B ^$(git merge-base A
B), and not B --not $(git merge-base --all A B) [which is equivalent
to B ^A or A..B].

Junio C Hamano wrote:
 I dunno.  The description you gave was insufficient for me to answer
 that question.

Sorry about that.  I'll rewrite it as a set of commands:

$ cd /tmp
$ git clone gh:artagnon/clayoven
$ cd clayoven
$ git checkout -b pu master~10
$ echo foom LICENSE
$ git commit -a -m Change in pu
$ git checkout -b rebase.autostash master~5
$ echo loom  clayoven.gemspec
$ git commit -a -m Change in rebase.autostash
$ git checkout master
$ git merge pu
$ git checkout rebase.autostash
$ git merge pu
$ echo quux Gemfile
$ git commit -a -m  Change 2 in rebase.autostash
$ git log rebase.autostash ^master
$ git log rebase.autostash --not $(git merge-base --all rebase.autostash master)
$ git log rebase.autostash ^$(git merge-base rebase.autostash master)
$ git diff rebase.autostash ^$(git merge-base rebase.autostash master)

Note that git diff can only take two endpoints so git merge-base --all
is meaningless in this case.

To summarize: B ^(git merge-base A B) [the current ... syntax of git
diff] has no exact equivalent in the log world.  Therefore, no matter
which existing rev spec we overload, we can never get the current ...
for diff in a consistent manner.

We certainly know that 'git diff A~B' is going to be useful, and have
an exact equivalent 'git log A~B' (but I'm not sure how useful it will
be, because I don't work with merges much).  The point is that it can
be overloaded in a consistent manner for other commands like 'rebase
-i' too.

I recently caved to overloading .. [1] because there's so much
resistance to inventing a new rev spec.  I'm not exactly elated with
that.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/222343
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] Highly inconsistent diff UI

[Ramkumar Ramachandra]

Thomas Rast wrote:
 I already hate half of the existing syntax, and I cannot remember using
 ^! (except while investigating what 'git diff C^!' does and why not),
 ^@, @{-N} (only the related 'git checkout -'), @{date} and @{relative},
 ^{}, :/foo, and ^{/foo}, *at all*.

I'm actually totally in love with the existing syntax (with the
possible exception of :/).  I think it's extremely terse and
expressive.  People who don't care about such flexibility and power
can stick to using ~ and ^.  Enabling other users with additional
syntax doesn't harm them in any way.

 ^{/foo} is the same as :/foo, except it properly groups.

No, not at all.  First of all, ^{/foo} is invalid: you need to specify
a ref to dig through, like HEAD^{/foo}.  OTOH, :/foo returns a match
from any ref.
--
To unsubscribe from this list: send the line unsubscribe 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] inotify to minimize stat() calls

[Thomas Rast]

Robert Zeh robert.allan@gmail.com writes:

 Here is a patch that creates a daemon that tracks file
 state with inotify, writes it out to a file upon request,
 and changes most of the calls to stat to use said cache.

 It has bugs, but I figured it would be smarter to see
 if the approach was acceptable at all before spending the
 time to root the bugs out.

Thanks for tackling this; it's probably about time we got a inotify
support :-(

 I've implemented the communication with a file, and not a socket,
 because I think implementing a socket is going to create
 security issues on multiuser systems.  For example, would a
 socket allow stat information to cross user boundaries?

This ties in with an issue discussed in an earlier thread:

  http://thread.gmane.org/gmane.comp.version-control.git/217817/focus=218307

The conclusion there was that the default limits are set such that it is
not feasible to run one daemon per repository (that would quickly hit
the limits when e.g. iterating all repos in a typical android tree using
repo).

So whatever you use for communication needs to work as a global daemon.

I'd just trust the SSH folks to know about security; on my system
ssh-agent creates

  /tmp/ssh-RANDOMSTRING/agent.PID

where the directory has mode 0700, and the file is a unit socket with
mode 0600.  That should make doubly sure that no other user can open the
socket.

  filechange-cache.c   | 203
 +++

Is your MUA wrapping the patch?

 +static void watch_directory(int inotify_fd)
 +{
 + char buf[PATH_MAX];
 +
 + if (!getcwd(buf, sizeof(buf)))
 + die_errno(Unable to get current directory);
 +
 + int i = 0;
 + struct dir_struct dir;
 + const char *pathspec[1] = { buf, NULL };
 +
 + memset(dir, 0, sizeof(dir));
 + setup_standard_excludes(dir);
 +
 + fill_directory(dir, pathspec);
 + for(i = 0; i  dir.nr; i++) {
 + struct dir_entry *ent = dir.entries[i];
 + watch_file(inotify_fd, ent-name);
 + free(ent);
 + }

I don't get this bit.  The lstat() are run over all files listed in the
index.  So shouldn't your daemon watch exactly those (or rather, all
dirnames of such files)?

The actual directory contents are only needed to find untracked files,
and there would be a lot of complication surrounding that, so I suggest
saving that for later (and for now measuring the speedup with 'git
status -uno'!).

For example, you'd have to actually watch and re-read all .gitignore
files, and the .git/info/exclude, and the core.excludesfile, to see if
your notion of an ignored file became stale.

Also, you seem to call watch_directory() only on the current(?) dir, but
you need to recursively set up watches for all directories in the
repository.

 + while (1) {
 + int i = 0;
 + length = read(inotify_fd, buffer, sizeof(buffer));
 + for(i = 0; i  length; ) {
 + struct inotify_event *event =
 + (struct inotify_event*)(buffer+i);
 + /* printf(event: %d %x %d %s\n, event-wd, 
 event-mask,
 +event-len, event-name); */
 + if (request_watch_descriptor == event-wd) {
 + write_stat_cache();
 + } else if (root_directory_watch_descriptor
 +== event-wd) {
 + printf(root directory died!\n);
 + exit(0);
 + } else if (event-mask  IN_Q_OVERFLOW) {
 + restart();

Good.

 + } else if (event-mask  IN_MODIFY) {
 + if (event-len)
 + update_stat_cache(event-name);
 + }

So whenever a file changes, you stat() it.  That's good for simplicity
now, but I suspect it will provide some optimization opportunities
later.


On some design aspects, I'd want:

* a toggle to run the test suite with the daemons, or without

* if you go with a user-wide daemon, a way to ensure that the test-suite
  daemon is not the same as my real daemon, and make sure it is killed
  after the test runs finish

* a test that triggers IN_Q_OVERFLOW, e.g. by sending SIGSTOP and doing
  a large repository operation

* a test that renames directories

The last one is just based on my personal experience with messing with
inotify; renaming directories is the hard case for that API.  We may
already cover this in the test suite, or we may not; but it must be
tested.

Other than that last point, focus your tests not on small tests but on
the test suite.  It would seem rather unlikely to me that you could
manage to pass the entire test suite with this daemon active but broken.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to 

Re: Itches with the current rev spec

[Matthieu Moy]

Ramkumar Ramachandra artag...@gmail.com writes:

 Hi,

 So, I have three serious itches that would be nice to address:

 1. git reset --hard HEAD~1/ git show HEAD~1 is a very common idiom
 that's unnecessarily cumbersome to type out.  We can make the rev
 part of rev~n optional without being ambiguous: you might argue
 that ~n normally refers to a /home/n, but who uses numbers in
 place of usernames?  Even if they do, how can that path possibly be
 inside our repository?

It's a bit more complex than that: the ~username is expanded by the
shell, before Git has any opportunity to guess anything.

~1 would be unusable for zsh users and tcsh users at least by default:

zsh% echo ~1
zsh: not enough directory stack entries.

tcsh% echo ~1
Unknown user: 1.

(An obvious workaround is to shell-quote it, but as the goal is to have
something easy to type, \~1 or '~1' do not give so much benefit over
HEAD~1)

That said, it seems to work fine for bash (even if the number is a PID,
it's not expanded), so it may be a good idea to add it as a shortcut,
with a warning in the doc about shell expansion.

-- 
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: Itches with the current rev spec

[Felipe Contreras]

On Thu, Apr 25, 2013 at 3:22 AM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
 Ramkumar Ramachandra artag...@gmail.com writes:

 Hi,

 So, I have three serious itches that would be nice to address:

 1. git reset --hard HEAD~1/ git show HEAD~1 is a very common idiom
 that's unnecessarily cumbersome to type out.  We can make the rev
 part of rev~n optional without being ambiguous: you might argue
 that ~n normally refers to a /home/n, but who uses numbers in
 place of usernames?  Even if they do, how can that path possibly be
 inside our repository?

 It's a bit more complex than that: the ~username is expanded by the
 shell, before Git has any opportunity to guess anything.

 ~1 would be unusable for zsh users and tcsh users at least by default:

 zsh% echo ~1
 zsh: not enough directory stack entries.

 tcsh% echo ~1
 Unknown user: 1.

 (An obvious workaround is to shell-quote it, but as the goal is to have
 something easy to type, \~1 or '~1' do not give so much benefit over
 HEAD~1)

 That said, it seems to work fine for bash (even if the number is a PID,
 it's not expanded), so it may be a good idea to add it as a shortcut,
 with a warning in the doc about shell expansion.

Yeah, probably fine, but I would also like H~1.

-- 
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: Itches with the current rev spec

[Andreas Schwab]

Ramkumar Ramachandra artag...@gmail.com writes:

 you might argue that ~n normally refers to a /home/n, but who uses
 numbers in place of usernames?

~n expands to the nth element of the dir stack.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
And now for something completely different.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Itches with the current rev spec

[Ramkumar Ramachandra]

Andreas Schwab wrote:
 Ramkumar Ramachandra artag...@gmail.com writes:

 you might argue that ~n normally refers to a /home/n, but who uses
 numbers in place of usernames?

 ~n expands to the nth element of the dir stack.

Oh, ouch.  And this is bash.

Have to think of something else.
--
To unsubscribe from this list: send the line 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] bash-prompt.sh: Show where rebase is at when interrupted by a merge conflict

[Zoltan Klinger]

When a rebase is interrupted by a merge conflict it could be useful to
know how far a rebase has progressed and how many commits in total this
rebase will apply. Teach the __git_ps1() command to display the number
of commits so far applied and the total number of commits to be applied.

Below is a sample output of the improved __git_ps1() command:
  ((3ec0a6a...)|REBASE 2/5)

In the example above the rebase has stopped at the second commit due to
a merge conflict and there are a total number of five commits to be
applied by this rebase.

This information can be already obtained from the following files which are
being generated during the rebase:
GIT_DIR/.git/rebase-merge/msgnum (git-rebase--merge.sh)
GIT_DIR/.git/rebase-merge/end(git-rebase--merge.sh)
GIT_DIR/.git/rebase-apply/next   (git-am.sh)
GIT_DIR/.git/rebase-apply/last   (git-am.sh)

1) Modify git-rebase--interactive.sh to also create
  GIT_DIR/.git/rebase-merge/msgnum
  GIT_DIR/.git/rebase-merge/end
   files for the number of commits so far applied and the total number of
   commits to be applied.
2) Modify git-prompt.sh to read and display info from the above files
3) Update test t9903-bash-prompt.sh to reflect changes introduced by
   this patch.

Signed-off-by: Zoltan Klinger zoltan.klin...@gmail.com
---

Changes since previous patch:

* Fix typo in commit message
* Make prompt easier to read by replacing '|' character with space
* Modify test cases to have more than one commit to apply by the rebase

 contrib/completion/git-prompt.sh |   21 -
 git-rebase--interactive.sh   |5 +
 t/t9903-bash-prompt.sh   |   14 ++
 3 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 054c52e..eaf5c36 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -263,14 +263,21 @@ __git_ps1 ()
else
local r=
local b=
-   if [ -f $g/rebase-merge/interactive ]; then
-   r=|REBASE-i
-   b=$(cat $g/rebase-merge/head-name)
-   elif [ -d $g/rebase-merge ]; then
-   r=|REBASE-m
+   local step=
+   local total=
+   if [ -d $g/rebase-merge ]; then
b=$(cat $g/rebase-merge/head-name)
+   step=$(cat $g/rebase-merge/msgnum)
+   total=$(cat $g/rebase-merge/end)
+   if [ -f $g/rebase-merge/interactive ]; then
+   r=|REBASE-i
+   else
+   r=|REBASE-m
+   fi
else
if [ -d $g/rebase-apply ]; then
+   step=$(cat $g/rebase-apply/next)
+   total=$(cat $g/rebase-apply/last)
if [ -f $g/rebase-apply/rebasing ]; then
r=|REBASE
elif [ -f $g/rebase-apply/applying ]; then
@@ -308,6 +315,10 @@ __git_ps1 ()
}
fi
 
+   if [ -n $step ]  [ -n $total ]; then
+   r=$r $step/$total
+   fi
+
local w=
local i=
local s=
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 048a140..f76ff8f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -57,6 +57,9 @@ rewritten=$state_dir/rewritten
 
 dropped=$state_dir/dropped
 
+end=$state_dir/end
+msgnum=$state_dir/msgnum
+
 # A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
 # GIT_AUTHOR_DATE that will be used for the commit that is currently
 # being rebased.
@@ -109,7 +112,9 @@ mark_action_done () {
sed -e 1d  $todo  $todo.new
mv -f $todo.new $todo
new_count=$(git stripspace --strip-comments $done | wc -l)
+   echo $new_count  $msgnum
total=$(($new_count + $(git stripspace --strip-comments $todo | wc 
-l)))
+   echo $total  $end
if test $last_count != $new_count
then
last_count=$new_count
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index e147a8d..083b319 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -28,6 +28,10 @@ test_expect_success 'setup for prompt tests' '
git checkout -b b2 master 
echo 0  file 
git commit -m second b2 file 
+   echo 00  file 
+   git commit -m another b2 file 
+   echo 000  file 
+   git commit -m yet another b2 file 
git checkout master
 '
 
@@ -243,10 +247,12 @@ test_expect_success 'prompt - inside bare repository' '
 '
 
 test_expect_success 'prompt - interactive rebase' '
-   printf  (b1|REBASE-i)  expected
+   printf  (b1|REBASE-i 2/3)  expected

Re: [BUG] Highly inconsistent diff UI

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
 $ git diff rebase.autostash ^$(git merge-base rebase.autostash master)

So, Thomas pointed out on IRC that there is one edge-case: a
criss-cross merge.  So, we can define A~B as B ^$(git-merge-base
--closest-to=B A B).  If both A and B are equidistant from the
criss-cross merge [will that ever happen in practice?], we have no
choice but to error out.
--
To unsubscribe from this list: send the line 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] t5801: properly test the test shell

[Michael J Gruber]

fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
test which was meant to skip the test unless the test shell is bash.
Unfortunately, it tests for the availability of bash only. But users can
opt to use a different shell (using SHELL_PATH) for the tests even though
bash is available.

At least for dash,
21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)
is the commit which actually introduces a test (pushing without refspec)
which fails to fail even though it is supposed to. It uses the
construct:

VAR=value function arguments

Make t5801 actually test whether the test shell is bash.

An even better alternative would be to make the test POSIX compliant, of
course.

Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net
---
 t/t5801-remote-helpers.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index ed962c4..c979863 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -8,7 +8,7 @@ test_description='Test remote-helper import and export commands'
 . ./test-lib.sh
 . $TEST_DIRECTORY/lib-gpg.sh
 
-if ! type ${BASH-bash} /dev/null 21; then
+if test $(basename ${SHELL_PATH}) != bash; then
skip_all='skipping remote-testgit tests, bash not available'
test_done
 fi
-- 
1.8.2.1.882.gefdb4b7

--
To unsubscribe from this list: send the line 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] Fix segfault for insane ident line

[Jiang Xin]

Today when I push a long run repository, git complains:

$ git push -u github master:doc-tech
Counting objects: 5575, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2560/2560), done.
remote: error: object b798e3ffca56af58c2a7728d75027212a558b6d3:invalid 
author/committer line - missing space before email
remote: fatal: Error in object
error: pack-objects died of signal 13
error: failed to push some refs to ...

When I check the commit, git will segfault:

$ git show b798e3ffca56af58c2a7728d75027212a558b6d3
Segmentation fault: 11

But git v1.8.1 works without difficulty, only with a suspect datetime.

$ gitv1.8.1 log -1 b798e3ffca56af58c2a7728d75027212a558b6d3
commit b798e3ffca56af58c2a7728d75027212a558b6d3
Author: leiweiweile...@bj.ossxp.com
Date:   Thu Jan 1 00:00:00 1970 +

add blog/typo.mm.

Detail of this commit. (Note that username and email is insane.)

$ gitv1.8.1 cat-file commit b798e3ffca56af58c2a7728d75027212a558b6d3
tree a1fcf9257bfbcd75f8c9aa931d1e89dbc60ae308
parent 566f0a6489316db9c9dd12bfda51ffc75a24a9b0
author leiweiweile...@bj.ossxp.com leiweiweile...@bj.ossxp.com 
1261964093 +0800
committer leiweiweile...@bj.ossxp.com leiweiweile...@bj.ossxp.com 
1261964093 +0800

add blog/typo.mm.

Because git v1.8.1 is good and v1.8.2 is bad, so I run a `git bisect` on it,
then I find this issue was was introduced in v1.8.1-rc1-7-g3c020bd.

commit 3c020bd528d5dc320b82bd787670edfe6695f097
Author: Antoine Pelisse apeli...@gmail.com
Date:   Sat Jan 5 22:26:38 2013 +0100

Use split_ident_line to parse author and committer

Currently blame.c::get_acline(), pretty.c::pp_user_info() and
shortlog.c::insert_one_record() are parsing author name, email, time
and tz themselves.

Use ident.c::split_ident_line() for better code reuse.

Signed-off-by: Antoine Pelisse apeli...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com

Backtrace for this segfault:

(gdb) bt
#0  0x7fff8d8959ae in strtoul_l ()
#1  0x00010012da4a in pp_user_info ()
#2  0x00010012f811 in pp_header ()
#3  0x00010012f546 in pretty_print_commit ()
#4  0x00010010dd1f in show_log ()
#5  0x00010010e601 in log_tree_commit ()
#6  0x0001000565f1 in cmd_log_walk ()
#7  0x000100056c5c in cmd_show ()
#8  0x00012710 in run_builtin ()
#9  0x00011717 in handle_internal_command ()
#10 0x00011f39 in run_argv ()
#11 0x0001155d in main ()

This patch can fix it.

Jiang Xin (1):
  Fix segfault for insane ident line

 builtin/blame.c | 14 +++---
 pretty.c|  9 +++--
 2 files changed, 18 insertions(+), 5 deletions(-)

-- 
1.8.2.1.348.gb94490b

--
To unsubscribe from this list: send the line 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] Fix segfault for insane ident line

[Jiang Xin]

An insane ident line (generated by old version of git or by hands such
as git-hash-object) may cause segfault for `git log` and others commands.

This is because the `split_ident_line` function save the result in
ident_split, and the returned ident_split.date_begin, ident_split.date_end,
ident_split.tz_begin, and ident_split.tz_end maybe NULL pointers for an
insame ident line.

This issue was was introduced in v1.8.1-rc1-7-g3c020bd (Use split_ident_line
to parse author and committer).

Signed-off-by: Jiang Xin worldhello@gmail.com
---
 builtin/blame.c | 14 +++---
 pretty.c|  9 +++--
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 86100..4b94e 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1375,10 +1375,18 @@ static void get_ac_line(const char *inbuf, const char 
*what,
maillen = ident.mail_end - ident.mail_begin;
mailbuf = ident.mail_begin;
 
-   *time = strtoul(ident.date_begin, NULL, 10);
+   if (ident.date_begin != NULL) {
+   *time = strtoul(ident.date_begin, NULL, 10);
+   } else {
+   *time = 0;
+   }
 
-   len = ident.tz_end - ident.tz_begin;
-   strbuf_add(tz, ident.tz_begin, len);
+   if (ident.tz_begin != NULL  ident.tz_end !=NULL) {
+   len = ident.tz_end - ident.tz_begin;
+   strbuf_add(tz, ident.tz_begin, len);
+   } else {
+   strbuf_addstr(tz, (unknown));
+   }
 
/*
 * Now, convert both name and e-mail using mailmap
diff --git a/pretty.c b/pretty.c
index d3a82..2402f 100644
--- a/pretty.c
+++ b/pretty.c
@@ -438,8 +438,13 @@ void pp_user_info(const struct pretty_print_context *pp,
strbuf_add(name, namebuf, namelen);
 
namelen = name.len + mail.len + 3; /* ' ' + '' + '' */
-   time = strtoul(ident.date_begin, date, 10);
-   tz = strtol(date, NULL, 10);
+   if (ident.date_begin != NULL) {
+   time = strtoul(ident.date_begin, date, 10);
+   tz = strtol(date, NULL, 10);
+   } else {
+   time = 0;
+   tz = 0;
+   }
 
if (pp-fmt == CMIT_FMT_EMAIL) {
strbuf_addstr(sb, From: );
-- 
1.8.2.1.348.gb94490b

--
To unsubscribe from this list: send the line unsubscribe 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] Fix segfault for insane ident line

[Jiang Xin]

Oops, Ignore this patch please, I found they were already fixed by the
following commits
of next branch. Maybe next time I should patch something in next branch.

commit 9dbe7c3d7f4424cf0c27c2d4efabf72e58fa76b9
Author: René Scharfe rene.scha...@lsrfire.ath.cx
Date:   Wed Apr 17 20:33:35 2013 +0200

pretty: handle broken commit headers gracefully

Centralize the parsing of the date and time zone strings in the new
helper function show_ident_date() and make sure it checks the pointers
provided by split_ident_line() for NULL before use.

Reported-by: Ivan Lyapunov dron...@gmail.com
Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx
Signed-off-by: Junio C Hamano gits...@pobox.com


commit de5abe9fe91a496d019d62abefe23df9d72fad30
Author: René Scharfe rene.scha...@lsrfire.ath.cx
Date:   Wed Apr 17 20:33:54 2013 +0200

blame: handle broken commit headers gracefully

split_ident_line() can leave us with the pointers date_begin, date_end,
tz_begin and tz_end all set to NULL.  Check them before use and supply
the same fallback values as in the case of a negative return code from
split_ident_line().

The (unknown) is not actually shown in the output, though, because it
will be converted to a number (zero) eventually.

Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx


--
蒋鑫

北京群英汇信息技术有限公司
邮件: worldhello@gmail.com
网址: http://www.ossxp.com/
博客: http://www.worldhello.net/
微博: http://weibo.com/gotgit/
电话: 18601196889
--
To unsubscribe from this list: send the line unsubscribe 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] t5801: properly test the test shell

[Johannes Sixt]

Am 4/25/2013 12:09, schrieb Michael J Gruber:
 fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
 test which was meant to skip the test unless the test shell is bash.
 Unfortunately, it tests for the availability of bash only. But users can
 opt to use a different shell (using SHELL_PATH) for the tests even though
 bash is available.

After my patch this morning (avoid process substitution), there is not
much bashism left in git-remote-testgit:

diff --git a/git-remote-testgit b/git-remote-testgit
index e99d5fa..178d030 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/bin/sh
 # Copyright (c) 2012 Felipe Contreras
 
 alias=$1
@@ -23,7 +23,6 @@ then
testgitmarks=$dir/testgit.marks
test -e $gitmarks || $gitmarks
test -e $testgitmarks || $testgitmarks
-   testgitmarks_args=( --{import,export}-marks=$testgitmarks )
 fi
 
 while read line
@@ -70,7 +69,10 @@ do
fi
 
echo feature done
-   git fast-export ${testgitmarks_args[@]} $refs |
+   git fast-export \
+   ${testgitmarks:+--import-marks=$testgitmarks} \
+   ${testgitmarks:+--export-marks=$testgitmarks} \
+   $refs |
sed -e s#refs/heads/#${prefix}/heads/#g
echo done
;;
@@ -89,7 +91,10 @@ do
 
before=$(git for-each-ref --format='%(refname) %(objectname)')
 
-   git fast-import ${testgitmarks_args[@]} --quiet
+   git fast-import \
+   ${testgitmarks:+--import-marks=$testgitmarks} \
+   ${testgitmarks:+--export-marks=$testgitmarks} \
+   --quiet
 
# figure out which refs were updated
git for-each-ref --format='%(refname) %(objectname)' |

What's left is to take care of the shbang line, remove the bash
check from t5801, make a proper commit from this patch. But I leave
that to the interested reader. :-)

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

Re: Itches with the current rev spec

[Ramkumar Ramachandra]

Felipe Contreras wrote:
 Yeah, probably fine, but I would also like H~1.

You can get that now using `git symbolic ref H HEAD`.  I'm wondering
if we can do better than hard-interpreting HEAD as H at the rev-parse
level.
--
To unsubscribe from this list: send the line unsubscribe 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] t5801: properly test the test shell

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 25.04.2013 12:59:
 Am 4/25/2013 12:09, schrieb Michael J Gruber:
 fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
 test which was meant to skip the test unless the test shell is bash.
 Unfortunately, it tests for the availability of bash only. But users can
 opt to use a different shell (using SHELL_PATH) for the tests even though
 bash is available.
 
 After my patch this morning (avoid process substitution), there is not
 much bashism left in git-remote-testgit:

Is that a patch you submitted?

 diff --git a/git-remote-testgit b/git-remote-testgit
 index e99d5fa..178d030 100755
 --- a/git-remote-testgit
 +++ b/git-remote-testgit
 @@ -1,4 +1,4 @@
 -#!/usr/bin/env bash
 +#!/bin/sh
  # Copyright (c) 2012 Felipe Contreras
  
  alias=$1
 @@ -23,7 +23,6 @@ then
   testgitmarks=$dir/testgit.marks
   test -e $gitmarks || $gitmarks
   test -e $testgitmarks || $testgitmarks
 - testgitmarks_args=( --{import,export}-marks=$testgitmarks )
  fi
  
  while read line
 @@ -70,7 +69,10 @@ do
   fi
  
   echo feature done
 - git fast-export ${testgitmarks_args[@]} $refs |
 + git fast-export \
 + ${testgitmarks:+--import-marks=$testgitmarks} \
 + ${testgitmarks:+--export-marks=$testgitmarks} \
 + $refs |
   sed -e s#refs/heads/#${prefix}/heads/#g
   echo done
   ;;
 @@ -89,7 +91,10 @@ do
  
   before=$(git for-each-ref --format='%(refname) %(objectname)')
  
 - git fast-import ${testgitmarks_args[@]} --quiet
 + git fast-import \
 + ${testgitmarks:+--import-marks=$testgitmarks} \
 + ${testgitmarks:+--export-marks=$testgitmarks} \
 + --quiet
  
   # figure out which refs were updated
   git for-each-ref --format='%(refname) %(objectname)' |
 
 What's left is to take care of the shbang line, remove the bash
 check from t5801, make a proper commit from this patch. But I leave
 that to the interested reader. :-)
 

No, the problem (that I'm adressing) is not git-remote-testgit which
uses bash unconditionally, independent of SHELL_PATH.

The problem is bashism(s) in t5801 itself. That is completely orthogonal
to your (non-) patch.

Michael
--
To unsubscribe from this list: send the line 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/9] remote-helpers: fixes and cleanups

[Felipe Contreras]

Hi,

Here's a bunch of cleanups mostly to synchronize remote-bzr and remote-hg.

One of these might conflict with a series already in pu, if so, the code here
should be the prefered one.

Felipe Contreras (9):
  remote-bzr: trivial cleanups
  remote-hg: remove extra check
  remote-bzr: fix bad state issue
  remote-bzr: add support to push URLs
  remote-hg: use hashlib instead of hg sha1 util
  remote-bzr: store converted URL
  remote-hg: use python urlparse
  remote-bzr: tell bazaar to be quiet
  remote-bzr: strip extra newline

 contrib/remote-helpers/git-remote-bzr | 47 ++-
 contrib/remote-helpers/git-remote-hg  | 17 ++---
 2 files changed, 48 insertions(+), 16 deletions(-)

-- 
1.8.2.1

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

[PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/remote-helpers/git-remote-bzr | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index aa7bc97..82bf7c7 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -94,7 +94,7 @@ class Marks:
 return self.last_mark
 
 def is_marked(self, rev):
-return self.marks.has_key(rev)
+return rev in self.marks
 
 def new_mark(self, rev, mark):
 self.marks[rev] = mark
@@ -224,7 +224,7 @@ def export_files(tree, files):
 else:
 mode = '100644'
 
-# is the blog already exported?
+# is the blob already exported?
 if h in filenodes:
 mark = filenodes[h]
 final.append((mode, mark, path))
@@ -521,7 +521,7 @@ def c_style_unescape(string):
 return string
 
 def parse_commit(parser):
-global marks, blob_marks, bmarks, parsed_refs
+global marks, blob_marks, parsed_refs
 global mode
 
 parents = []
@@ -555,7 +555,7 @@ def parse_commit(parser):
 mark = int(mark_ref[1:])
 f = { 'mode' : m, 'data' : blob_marks[mark] }
 elif parser.check('D'):
-t, path = line.split(' ')
+t, path = line.split(' ', 1)
 f = { 'deleted' : True }
 else:
 die('Unknown file command: %s' % line)
@@ -643,6 +643,7 @@ def do_export(parser):
 wt = repo.bzrdir.open_workingtree()
 wt.update()
 print ok %s % ref
+
 print
 
 def do_capabilities(parser):
-- 
1.8.2.1

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

[PATCH 2/9] remote-hg: remove extra check

[Felipe Contreras]

Not needed since we use xrange ourselves.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/remote-helpers/git-remote-hg | 4 
 1 file changed, 4 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-hg 
b/contrib/remote-helpers/git-remote-hg
index 5481331..0b7c81f 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -332,10 +332,6 @@ def export_ref(repo, name, kind, head):
 ename = '%s/%s' % (kind, name)
 tip = marks.get_tip(ename)
 
-# mercurial takes too much time checking this
-if tip and tip == head.rev():
-# nothing to do
-return
 revs = xrange(tip, head.rev() + 1)
 count = 0
 
-- 
1.8.2.1

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

[PATCH 3/9] remote-bzr: fix bad state issue

[Felipe Contreras]

Carried from remote-hg.

The problem reportedly happened after doing a push that fails, the abort
causes the state of remote-hg to go bad, this happens because
remote-hg's marks are not stored, but 'git fast-export' marks are.

Ensure that the marks are _always_ stored.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/remote-helpers/git-remote-bzr | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index 82bf7c7..84734c7 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -32,6 +32,7 @@ import os
 import json
 import re
 import StringIO
+import atexit
 
 NAME_RE = re.compile('^([^]+)')
 AUTHOR_RE = re.compile('^([^]+?)? ?([^]*)$')
@@ -728,6 +729,7 @@ def main(args):
 blob_marks = {}
 parsed_refs = {}
 files_cache = {}
+marks = None
 
 gitdir = os.environ['GIT_DIR']
 dirname = os.path.join(gitdir, 'bzr', alias)
@@ -754,6 +756,10 @@ def main(args):
 die('unhandled command: %s' % line)
 sys.stdout.flush()
 
+def bye():
+if not marks:
+return
 marks.store()
 
+atexit.register(bye)
 sys.exit(main(sys.argv))
-- 
1.8.2.1

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

[PATCH 4/9] remote-bzr: add support to push URLs

[Felipe Contreras]

Just like in remote-hg.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/remote-helpers/git-remote-bzr | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index 84734c7..d6319d6 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -32,7 +32,7 @@ import os
 import json
 import re
 import StringIO
-import atexit
+import atexit, shutil, hashlib
 
 NAME_RE = re.compile('^([^]+)')
 AUTHOR_RE = re.compile('^([^]+?)? ?([^]*)$')
@@ -719,11 +719,11 @@ def main(args):
 global blob_marks
 global parsed_refs
 global files_cache
+global is_tmp
 
 alias = args[1]
 url = args[2]
 
-prefix = 'refs/bzr/%s' % alias
 tags = {}
 filenodes = {}
 blob_marks = {}
@@ -731,6 +731,13 @@ def main(args):
 files_cache = {}
 marks = None
 
+if alias[5:] == url:
+is_tmp = True
+alias = hashlib.sha1(alias).hexdigest()
+else:
+is_tmp = False
+
+prefix = 'refs/bzr/%s' % alias
 gitdir = os.environ['GIT_DIR']
 dirname = os.path.join(gitdir, 'bzr', alias)
 
@@ -759,7 +766,10 @@ def main(args):
 def bye():
 if not marks:
 return
-marks.store()
+if not is_tmp:
+marks.store()
+else:
+shutil.rmtree(dirname)
 
 atexit.register(bye)
 sys.exit(main(sys.argv))
-- 
1.8.2.1

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

[PATCH 5/9] remote-hg: use hashlib instead of hg sha1 util

[Felipe Contreras]

To be in sync with remote-bzr.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/remote-helpers/git-remote-hg | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/remote-helpers/git-remote-hg 
b/contrib/remote-helpers/git-remote-hg
index 0b7c81f..99abda4 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -22,6 +22,7 @@ import shutil
 import subprocess
 import urllib
 import atexit
+import hashlib
 
 #
 # If you want to switch to hg-git compatibility mode:
@@ -830,7 +831,7 @@ def main(args):
 
 if alias[4:] == url:
 is_tmp = True
-alias = util.sha1(alias).hexdigest()
+alias = hashlib.sha1(alias).hexdigest()
 else:
 is_tmp = False
 
-- 
1.8.2.1

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

[PATCH 6/9] remote-bzr: store converted URL

[Felipe Contreras]

Mercurial might convert the URL to something more appropriate, like an
absolute path. Lets store that instead of the original URL, which won't
work from a different working directory if it's relative.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/remote-helpers/git-remote-bzr | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index d6319d6..3d3b1c1 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -32,7 +32,7 @@ import os
 import json
 import re
 import StringIO
-import atexit, shutil, hashlib
+import atexit, shutil, hashlib, urlparse, subprocess
 
 NAME_RE = re.compile('^([^]+)')
 AUTHOR_RE = re.compile('^([^]+?)? ?([^]*)$')
@@ -713,6 +713,14 @@ def get_repo(url, alias):
 
 return branch
 
+def fix_path(alias, orig_url):
+url = urlparse.urlparse(orig_url, 'file')
+if url.scheme != 'file' or os.path.isabs(url.path):
+return
+abs_url = urlparse.urljoin(%s/ % os.getcwd(), orig_url)
+cmd = ['git', 'config', 'remote.%s.url' % alias, bzr::%s % abs_url]
+subprocess.call(cmd)
+
 def main(args):
 global marks, prefix, dirname
 global tags, filenodes
@@ -741,6 +749,9 @@ def main(args):
 gitdir = os.environ['GIT_DIR']
 dirname = os.path.join(gitdir, 'bzr', alias)
 
+if not is_tmp:
+fix_path(alias, url)
+
 if not os.path.exists(dirname):
 os.makedirs(dirname)
 
-- 
1.8.2.1

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

[PATCH 7/9] remote-hg: use python urlparse

[Felipe Contreras]

It's simpler, and we don't need to depend on certain Mercurial versions.

Also, now we don't update the URL if 'file://' is not present.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/remote-helpers/git-remote-hg | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-hg 
b/contrib/remote-helpers/git-remote-hg
index 99abda4..67c3074 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -12,7 +12,7 @@
 # For remote repositories a local clone is stored in
 # $GIT_DIR/hg/origin/clone/.hg/.
 
-from mercurial import hg, ui, bookmarks, context, util, encoding, node, error
+from mercurial import hg, ui, bookmarks, context, encoding, node, error
 
 import re
 import sys
@@ -22,7 +22,7 @@ import shutil
 import subprocess
 import urllib
 import atexit
-import hashlib
+import urlparse, hashlib
 
 #
 # If you want to switch to hg-git compatibility mode:
@@ -788,11 +788,11 @@ def do_export(parser):
 print
 
 def fix_path(alias, repo, orig_url):
-repo_url = util.url(repo.url())
-url = util.url(orig_url)
-if str(url) == str(repo_url):
+url = urlparse.urlparse(orig_url, 'file')
+if url.scheme != 'file' or os.path.isabs(url.path):
 return
-cmd = ['git', 'config', 'remote.%s.url' % alias, hg::%s % repo_url]
+abs_url = urlparse.urljoin(%s/ % os.getcwd(), orig_url)
+cmd = ['git', 'config', 'remote.%s.url' % alias, hg::%s % abs_url]
 subprocess.call(cmd)
 
 def main(args):
-- 
1.8.2.1

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

[PATCH 8/9] remote-bzr: tell bazaar to be quiet

[Felipe Contreras]

Otherwise we get notification, progress bars, and what not.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/remote-helpers/git-remote-bzr | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index 3d3b1c1..19668a9 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -26,6 +26,7 @@ bzrlib.plugin.load_plugins()
 import bzrlib.generate_ids
 import bzrlib.transport
 import bzrlib.errors
+import bzrlib.ui
 
 import sys
 import os
@@ -755,6 +756,8 @@ def main(args):
 if not os.path.exists(dirname):
 os.makedirs(dirname)
 
+bzrlib.ui.ui_factory.be_quiet(True)
+
 repo = get_repo(url, alias)
 
 marks_path = os.path.join(dirname, 'marks-int')
-- 
1.8.2.1

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

[PATCH 9/9] remote-bzr: strip extra newline

[Felipe Contreras]

It's added by fast-export, the user didn't type it.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 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 19668a9..8c316fe 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -549,6 +549,10 @@ def parse_commit(parser):
 parents.append(parser.get_mark())
 parser.next()
 
+# fast-export adds an extra newline
+if data[-1] == '\n':
+data = data[:-1]
+
 files = {}
 
 for line in parser:
-- 
1.8.2.1

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

[PATCH] remote-bzr: use proper push method

[Felipe Contreras]

Not just randomly synchronize the revisions with no checks at all. This
is the way bazaar's UI does it.

Also, add a non-ff check.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---

This patch should probably go to maint, as the results of pushing the way we
currently push are not really understood. Perhaps it's similar to a 'git push
 --force', or perhaps it can potentially screw the repository.

It's better to be safe and just do what bazaar does.

 contrib/remote-helpers/git-remote-bzr | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr 
b/contrib/remote-helpers/git-remote-bzr
index 8c316fe..cb8d081 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -639,12 +639,12 @@ def do_export(parser):
 for ref, revid in parsed_refs.iteritems():
 if ref == 'refs/heads/master':
 repo.generate_revision_history(revid, marks.get_tip('master'))
-revno, revid = repo.last_revision_info()
 if peer:
-if hasattr(peer, import_last_revision_info_and_tags):
-peer.import_last_revision_info_and_tags(repo, revno, revid)
-else:
-peer.import_last_revision_info(repo.repository, revno, 
revid)
+try:
+repo.push(peer, stop_revision=revid)
+except bzrlib.errors.DivergedBranches:
+print error %s non-fast forward % ref
+continue
 else:
 wt = repo.bzrdir.open_workingtree()
 wt.update()
-- 
1.8.2.1

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

Re: [PATCH] t5801: properly test the test shell

[Johannes Sixt]

Am 4/25/2013 13:21, schrieb Michael J Gruber:
 Johannes Sixt venit, vidit, dixit 25.04.2013 12:59:
 Am 4/25/2013 12:09, schrieb Michael J Gruber:
 fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
 test which was meant to skip the test unless the test shell is bash.
 Unfortunately, it tests for the availability of bash only. But users can
 opt to use a different shell (using SHELL_PATH) for the tests even though
 bash is available.

 After my patch this morning (avoid process substitution), there is not
 much bashism left in git-remote-testgit:
 
 Is that a patch you submitted?

It is not the patch I submitted this morning, but a patch on top that
removes the remaining bashisms from git-remote-testgit.

 No, the problem (that I'm adressing) is not git-remote-testgit which
 uses bash unconditionally, independent of SHELL_PATH.
 
 The problem is bashism(s) in t5801 itself. That is completely orthogonal
 to your (non-) patch.

OK. But wouldn't it be nicer to remove that bashism as well and have
portable t5801 and git-remote-testgit? :-)

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

[PATCH v2 1/1] templates: pre-push hook: check for missing GPG signatures

[Sebastian Götte]

Also added a missing colon that caused my bash 4.2.45(2)-release to
complain about an empty if.

Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
---
 templates/hooks--pre-push.sample | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
old mode 100644
new mode 100755
index 15ab6d8..a16283c
--- a/templates/hooks--pre-push.sample
+++ b/templates/hooks--pre-push.sample
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-# An example hook script to verify what is about to be pushed.  Called by git
+# An example hook script to verify what is about to be pushed. Called by git
 # push after it has checked the remote status, but before anything has been
 # pushed.  If this script exits with a non-zero status nothing will be pushed.
 #
@@ -14,22 +14,22 @@
 # Information about the commits which are being pushed is supplied as lines to
 # the standard input in the form:
 #
-#   local ref local sha1 remote ref remote sha1
+#  local ref local sha1 remote ref remote sha1
 #
-# This sample shows how to prevent push of commits where the log message starts
-# with WIP (work in progress).
+# This sample shows how to prevent pushing commits without good GPG signatures
+# or where the log message starts with WIP (work in progress).
 
 remote=$1
 url=$2
 
 z40=
+exitcode=0
 
-IFS=' '
 while read local_ref local_sha remote_ref remote_sha
 do
if [ $local_sha = $z40 ]
then
-   # Handle delete
+   : # Handle delete
else
if [ $remote_sha = $z40 ]
then
@@ -45,9 +45,16 @@ do
if [ -n $commit ]
then
echo Found WIP commit in $local_ref, not pushing
-   exit 1
+   exitcode=1
fi
+
+   # Check for missing good GPG signatures
+   for commit in `git log --format=%G? %h $range | grep -v 
'^G' | cut -d\  -f2`
+   do
+   echo Commit $commit does not have a good GPG signature
+   exitcode=1
+   done
fi
 done
 
-exit 0
+exit $exitcode
-- 
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 v2 0/1] templates: pre-push hook: check for missing GPG signatures

[Sebastian Götte]

On 04/24/2013 09:54 PM, Junio C Hamano wrote:
 None of the above is part of a proper commit log message, is it?
Fixed (I hope)

 Signed-off-by: Sebastian Götte ja...@physik-pool.tu-berlin.de
 diff --git a/templates/hooks--pre-push.sample 
 b/templates/hooks--pre-push.sample
 old mode 100644
 new mode 100755
 Why?
According to man githooks(5):
It is also a requirement for a given hook to be executable. However - in a
freshly initialized repository - the .sample files are executable by default.
This file is the only one in templates/ that was not executable, so I assume
this was a mistake.

 index 15ab6d8..08a72df
 --- a/templates/hooks--pre-push.sample
 +++ b/templates/hooks--pre-push.sample
 -# This sample shows how to prevent push of commits where the log message 
 starts
 -# with WIP (work in progress).
 +# This sample shows how to prevent pushing commits without good GPG 
 signatures
 What justifies to remove existing demonstration?  It is far easier
 for the end users to remove parts that do not apply to their needs,
 than coming up with a solution to add themselves without help from
 an example.
re-added it.

 +ec=0
 I think it is more customary to call this kind of variable ret or
 retval, not an abbreviation for european commission ;-).
renamed it to exitcode.

 -IFS=' '
 Why?
Otherwise in the for-loop below the output of the pipe chain is not correctly
split by newlines. Also AFAIK, this is not needed: I think the default
'spacetabnewline' is just fine here.
 
 +commits=`git log --format=%G? %h $range | grep -v '^G' | 
 cut -d\  -f2`
 Useless use of cut.  You could do
I just tried this, but I really want the script to output a list of *all*
offending commits (instead of exiting on the first problem). For this I need
the exitcode variable, but since at least bash executes the while loop in a
subshell due to the preceding pipe, I have some issues getting that out of the
subshell. This is what the code looked like without grep/cut:
# Check for missing good GPG signatures
git log --format=%G? %h $range |
(
exitcode=0
while read sign commit
do
test $sign = G  continue
echo Commit $commit does not have a good GPG 
 signature
exitcode=1
done
exit $exitcode
)
let exitcode=exitcode\|$?
This is less readable and only spawns one process less.

Sebastian Götte (1):
  templates: pre-push hook: check for missing GPG signatures

 templates/hooks--pre-push.sample | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)
 mode change 100644 = 100755 templates/hooks--pre-push.sample

-- 
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 5/7] rebase -i: return control to the caller, for housekeeping

[Ramkumar Ramachandra]

Martin von Zweigbergk wrote:
 Normally one would break if unsuccessful. What would fail if this was
 replaced by do_next || break and the above ..  return 1 was ..
  return. I assume that was your first attempt, but why did it not
 work?

Thanks.  This was a major thinko on my part, but I don't remember
exactly why I got confused.  I'll explain this patch properly in the
next iteration.
--
To unsubscribe from this list: send the line unsubscribe 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] remote-bzr: use proper push method

[Thomas Rast]

Felipe Contreras felipe.contre...@gmail.com writes:

 Not just randomly synchronize the revisions with no checks at all. This
 is the way bazaar's UI does it.

 Also, add a non-ff check.

 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 ---

 This patch should probably go to maint, as the results of pushing the way we
 currently push are not really understood. Perhaps it's similar to a 'git push
  --force', or perhaps it can potentially screw the repository.

 It's better to be safe and just do what bazaar does.

Other than this patch should probably go to maint, this should be in
the commit message.

-- 
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] git-remote-testgit: avoid process substitution

[Junio C Hamano]

Johannes Sixt j.s...@viscovery.net writes:

 From: Johannes Sixt j...@kdbg.org

 Bash on Windows does not implement process substitution.

 Signed-off-by: Johannes Sixt j...@kdbg.org
 ---
 ...
 Here is a fix. It assumes that the list of refs after the import is
 a superset of the refs before the import. (Can refs be deleted
 via fast-import?)

  git-remote-testgit | 12 +++-
  1 file changed, 7 insertions(+), 5 deletions(-)

 diff --git a/git-remote-testgit b/git-remote-testgit
 index 23c9d40..e99d5fa 100755
 --- a/git-remote-testgit
 +++ b/git-remote-testgit
 @@ -91,13 +91,15 @@ do
  
   git fast-import ${testgitmarks_args[@]} --quiet
  
 - after=$(git for-each-ref --format='%(refname) %(objectname)')
 -
   # figure out which refs were updated
 - join -e 0 -o '0 1.2 2.2' -a 2 (echo $before) (echo 
 $after) |
 - while read ref a b
 + git for-each-ref --format='%(refname) %(objectname)' |
 + while read ref a
   do
 - test $a == $b  continue
 + case $before in
 + *$ref $a*)
 + continue

This just like the original 'join' depends on the two output from
for-each-ref to be sorted the same way, which is true and fine.  But
I wonder one thing.  When $before has this in it:

refs/heads/refs/heads/master 664059...126eaa7

and your read ref a got this in the input:

refs/heads/master 664059...126eaa7

would the pattern matching by case work corretly?

Doing something like this might be needed.

case $LF$before$LF in
*$LF$ref $a$LF*)
continue ;; # matches
esac

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

Re: [PATCH 1/3] remote: add a test for extra arguments, according to docs

[Junio C Hamano]

Thomas Rast tr...@inf.ethz.ch writes:

 Junio C Hamano gits...@pobox.com writes:

 Thomas Rast tr...@inf.ethz.ch writes:

 +test_extra_arg () {
 +   expect=success
 +   if test z$1 = z-f; then
 +   expect=failure
 +   shift
 +   fi
 +   test_expect_$expect extra args: $* 
 +   test_must_fail git remote $* bogus_extra_arg 2actual 
 +   grep '^usage:' actual
 +   
 +}
 +
 +test_extra_arg -f add nick url
 +test_extra_arg rename origin newname

 Perhaps just a taste in readability thing, but I would prefer to see
 them more like

  test_extra_arg_expect failure add nick url
  test_extra_arg_expect success rename origin newname

 than misunderstanding-inviting -f that often stands for --force.

 Hmm.  I had that at first, but then the final cleanup would have had to
 touch all tests to remove the optional argument, making it noisy.

You do not need a final cleanup, as I _never_ meant failure/success
in the above illustration to be _optional_.  Being explicit reduces
mental burden when you later have to read such a custom scaffolding
each test invents in an ad-hoc manner to suit its needs.
--
To unsubscribe from this list: send the line unsubscribe 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] t5801: properly test the test shell

[Junio C Hamano]

Michael J Gruber g...@drmicha.warpmail.net writes:

 fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
 test which was meant to skip the test unless the test shell is bash.
 Unfortunately, it tests for the availability of bash only.

True.

 But users can
 opt to use a different shell (using SHELL_PATH) for the tests even though
 bash is available.

 At least for dash,
 21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)
 is the commit which actually introduces a test (pushing without refspec)
 which fails to fail even though it is supposed to. It uses the
 construct:

 VAR=value function arguments

The right fix for that is to fix that line, so that the test itself
can run under any sane POSIX shell, isn't it?  The test in turn may
need to run git-remote-testgit, which, without J6t's updates, only
is usable under bash, but to make sure the test will choke on
absence of bash, the existing check should be sufficient, no?

 Make t5801 actually test whether the test shell is bash.

 An even better alternative would be to make the test POSIX compliant, of
 course.

 Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net
 ---
  t/t5801-remote-helpers.sh | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
 index ed962c4..c979863 100755
 --- a/t/t5801-remote-helpers.sh
 +++ b/t/t5801-remote-helpers.sh
 @@ -8,7 +8,7 @@ test_description='Test remote-helper import and export 
 commands'
  . ./test-lib.sh
  . $TEST_DIRECTORY/lib-gpg.sh
  
 -if ! type ${BASH-bash} /dev/null 21; then
 +if test $(basename ${SHELL_PATH}) != bash; then
   skip_all='skipping remote-testgit tests, bash not available'
   test_done
  fi
--
To unsubscribe from this list: send the line unsubscribe 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] t5801: properly test the test shell

[Junio C Hamano]

Johannes Sixt j.s...@viscovery.net writes:

 Am 4/25/2013 12:09, schrieb Michael J Gruber:
 fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
 test which was meant to skip the test unless the test shell is bash.
 Unfortunately, it tests for the availability of bash only. But users can
 opt to use a different shell (using SHELL_PATH) for the tests even though
 bash is available.

 After my patch this morning (avoid process substitution), there is not
 much bashism left in git-remote-testgit:

Not much, or no more?  I think the latter is a very worthy goal.

Even if it is only a test helper, we would prefer to have a portable
one.

 diff --git a/git-remote-testgit b/git-remote-testgit
 index e99d5fa..178d030 100755
 --- a/git-remote-testgit
 +++ b/git-remote-testgit
 @@ -1,4 +1,4 @@
 -#!/usr/bin/env bash
 +#!/bin/sh
  # Copyright (c) 2012 Felipe Contreras
  
  alias=$1
 @@ -23,7 +23,6 @@ then
   testgitmarks=$dir/testgit.marks
   test -e $gitmarks || $gitmarks
   test -e $testgitmarks || $testgitmarks
 - testgitmarks_args=( --{import,export}-marks=$testgitmarks )
  fi
  
  while read line
 @@ -70,7 +69,10 @@ do
   fi
  
   echo feature done
 - git fast-export ${testgitmarks_args[@]} $refs |
 + git fast-export \
 + ${testgitmarks:+--import-marks=$testgitmarks} \
 + ${testgitmarks:+--export-marks=$testgitmarks} \
 + $refs |
   sed -e s#refs/heads/#${prefix}/heads/#g
   echo done
   ;;
 @@ -89,7 +91,10 @@ do
  
   before=$(git for-each-ref --format='%(refname) %(objectname)')
  
 - git fast-import ${testgitmarks_args[@]} --quiet
 + git fast-import \
 + ${testgitmarks:+--import-marks=$testgitmarks} \
 + ${testgitmarks:+--export-marks=$testgitmarks} \
 + --quiet
  
   # figure out which refs were updated
   git for-each-ref --format='%(refname) %(objectname)' |

 What's left is to take care of the shbang line, remove the bash
 check from t5801, make a proper commit from this patch. But I leave
 that to the interested reader. :-)

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

Re: [RFC/PATCH] Make --full-history consider more merges

[Kevin Bracey]

On 25/04/2013 04:59, Junio C Hamano wrote:

Junio C Hamano gits...@pobox.com writes:


So, given all that, revised patch below:

I tried to squeeze the minimum test I sent $gmane/220919 to the test
suite.  I think the do not use --parents option for this test
switch needs to be cleaned up a bit more, but it fails without your
patch and does pass with your patch.

I somehow was hoping that your fix to TREESAME semantics would also
correct the known breakage documented in that test, but it seems
that I was too greedy ;-)
Thanks for the test addition. Maybe we will be able to satisfy your 
greed in this series. There could be more worth doing here, and I think 
getting TREESAME precise is key.


I think I do want to take the step of storing treesame per parent. And 
once we do that, as well as avoiding the expensive re-diff, we have much 
richer information readily available as a simplification input (and output).


I'm working on a patch that does this - filling in an initial treesame[] 
array as a decoration in try_to_simplify_commit() is easy, and 
maintaining the array through later parent rewrites isn't as onerous as 
I feared - there are only a few places that rewrite parents after the 
initial scan. With a couple of helper functions to do things like 
delete nth, I think it'll be quite tidy.


I believe that simplify_merges itself needs at least one addition, and 
could use the treesame[] array to do it: if after doing reduce_heads, a 
commit is now different to all remaining parents, but there was a 
TREESAME parent eliminated, that parent should be reinstated. That would 
clearly highlight missed merges, showing both that older TREESAME 
parent and the newer !TREESAME parent that would have been taken in a 
normal merge.


And maybe there's more simplify_merges could do, if it had this full 
TREESAME information available.


(But even after you do all this stuff to get the right commits out, we 
then hit a niggle of mine that gitk forces --cc diffs - even if it shows 
shows the offending merge commit, you can't get it to do a diff...)


Kevin

--
To unsubscribe from this list: send the line unsubscribe 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] t5801: properly test the test shell

[Junio C Hamano]

Torsten Bögershausen tbo...@web.de writes:

 Could we use the same logic as in t9903:
 ...
 . ./lib-bash.sh

Please no.  The test itself is not about something that checks how
we behave under bash (which is what 9903 wants to see).  The use of
construct that is portable in t5801 is a pure and simple bug, and it
should not be hard to fix it.

If there still is such an unportable construct left, that is.

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

Re: [BUG] Highly inconsistent diff UI

[Junio C Hamano]

Thomas Rast tr...@inf.ethz.ch writes:

 What's the problem with cooking it for a while?  You can start using it
 immediately.  I'm just somewhat annoyed that the syntax is rapidly
 converging to Perl-style line noise.

Not quite, not quite, and a mild agreement.

Adding an obscure feature that may not be useful at all behind a
command line option will fall into the you can afford to cook for a
while, you can use it immediately yourself, nobody will get hurt
category.  Adding a configuration to turn such a feature by default
on already is a more severe problem because we need to adjust and
protect scripted Porcelains from getting hurt by an unexpected new
behaviour user may trigger by setting such a configuration before it
fully cooks, which is annoying maintenance burden for an obscure
feature with an unknown value.

Piling cruft on syntax is in a totally different league.  If not
carefully thought out, adding a random new syntax on a whim can
paint us into a corner we cannot later get out of, like the :/ we
recently discussed (which does have an escape hatch planned, but
imagine a world without one).

 I already hate half of the existing syntax, and I cannot remember using
 ^! (except while investigating what 'git diff C^!' does and why not),
 ^@, @{-N} (only the related 'git checkout -'), @{date} and @{relative},
 ^{}, :/foo, and ^{/foo}, *at all*.

 In fact I had to look up the second half of that list on the manpage.

I would have actually expected that people are more familiar with
the second half, i.e. @{2.weeks.ago}, @{4} and :/string, than ^!/^@
which I agree were more-or-less let's add a random cruft on a whim
without thinking things through mistakes.
--
To unsubscribe from this list: send the line unsubscribe 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 0/1] templates: pre-push hook: check for missing GPG signatures

[Junio C Hamano]

Sebastian Götte ja...@physik.tu-berlin.de writes:

 On 04/24/2013 09:54 PM, Junio C Hamano wrote:
 None of the above is part of a proper commit log message, is it?
 Fixed (I hope)

Don't hope, instead do.  ;-)

The questions I asked were not requests to explain them to _me_ in a
response like this.  They were the examples of what the proposed
commit log message should have explained what the patch attempts to
do.

 -IFS=' '
 Why?
 Otherwise in the for-loop below the output of the pipe chain is not correctly
 split by newlines. Also AFAIK, this is not needed: I think the default
 'spacetabnewline' is just fine here.

It is not enough to make sure that IFS has SP so that existing code
works correctly; we also need to see if the existing code needs to
avoid cutting the tokens at HT or LF.  I think in this case using
the default IFS is safe, as input to pre-push are SP separated refs
and object names, none of which can have SP, HT or LF in it.

# Check for missing good GPG signatures
git log --format=%G? %h $range |
(
exitcode=0
while read sign commit
do
test $sign = G  continue
echo Commit $commit does not have a good GPG 
 signature
exitcode=1
done
exit $exitcode
)
let exitcode=exitcode\|$?

Don't use bash-ism let.

The above loop is a perfectly fine and readable way to write the
logic, by the way

Except that we tend to prefer $ret over $exitcode, but I've already
said that.

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: [RFC/PATCH] Make --full-history consider more merges

[Junio C Hamano]

Kevin Bracey ke...@bracey.fi writes:

 On 25/04/2013 04:59, Junio C Hamano wrote:
 Junio C Hamano gits...@pobox.com writes:

 So, given all that, revised patch below:
 I tried to squeeze the minimum test I sent $gmane/220919 to the test
 suite.  I think the do not use --parents option for this test
 switch needs to be cleaned up a bit more, but it fails without your
 patch and does pass with your patch.

 I somehow was hoping that your fix to TREESAME semantics would also
 correct the known breakage documented in that test, but it seems
 that I was too greedy ;-)
 Thanks for the test addition. Maybe we will be able to satisfy your
 greed in this series. There could be more worth doing here, and I
 think getting TREESAME precise is key.

It is perfectly fine to do things one step at a time.  Let's get
the --full-history change into a good shape first and worry about
the more complex case after we are done.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] send-email: support NNTP

[Junio C Hamano]

l.stelm...@samsung.com (Łukasz Stelmach) writes:

 OK, I see. Good point. Where would you recommend me to put these modules
 and how to name them? I mean I don't want to make to much mess here (;

I would not recommend you to do any of the above now.  As I said, the
open-coded if/elsif cascade version you posted is fine as a starting
point.  I'd prefer to see it made work correctly as advertised first
with documentation updates and tests.

A Perl guru might want to come and refactor the result once the code
solidifies, but that should be left as a separate series.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] bash-prompt.sh: Show where rebase is at when interrupted by a merge conflict

[Junio C Hamano]

Zoltan Klinger zoltan.klin...@gmail.com writes:

 When a rebase is interrupted by a merge conflict it could be useful to

The command also stops when told to stop with amend, etc. no?  I
would phrase this way When a rebase stops (e.g. interrupted ...),
it could be useful

 This information can be already obtained from the following files which are
 being generated during the rebase:
 GIT_DIR/.git/rebase-merge/msgnum (git-rebase--merge.sh)
 GIT_DIR/.git/rebase-merge/end(git-rebase--merge.sh)
 GIT_DIR/.git/rebase-apply/next   (git-am.sh)
 GIT_DIR/.git/rebase-apply/last   (git-am.sh)

Here, I would add:

But git rebase -i did not leave enough clues.

 1) Modify git-rebase--interactive.sh to also create
   GIT_DIR/.git/rebase-merge/msgnum
   GIT_DIR/.git/rebase-merge/end
files for the number of commits so far applied and the total number of
commits to be applied.
 2) Modify git-prompt.sh to read and display info from the above files

Missing full-stop at the end.

 3) Update test t9903-bash-prompt.sh to reflect changes introduced by
this patch.

 Signed-off-by: Zoltan Klinger zoltan.klin...@gmail.com
 ---

Thanks.

 diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
 index 048a140..f76ff8f 100644
 --- a/git-rebase--interactive.sh
 +++ b/git-rebase--interactive.sh
 @@ -57,6 +57,9 @@ rewritten=$state_dir/rewritten
  
  dropped=$state_dir/dropped
  
 +end=$state_dir/end
 +msgnum=$state_dir/msgnum
 +
  # A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
  # GIT_AUTHOR_DATE that will be used for the commit that is currently
  # being rebased.
 @@ -109,7 +112,9 @@ mark_action_done () {
   sed -e 1d  $todo  $todo.new
   mv -f $todo.new $todo
   new_count=$(git stripspace --strip-comments $done | wc -l)
 + echo $new_count  $msgnum
   total=$(($new_count + $(git stripspace --strip-comments $todo | wc 
 -l)))
 + echo $total  $end

Write them like this:

echo foo $variable

Having no SP between redirection operator and redirection target is
merely a style thing [*1*].  Having an unnecessary dq around a
variable name that is used as redirection target is to work around a
bug in certain versions of bash that issues useless warnings.

I'll locally squeeze the above suggestions in and queue the result.
Please check what will appear on 'pu' by the end of day.

Thanks.


[Footnote]

*1* But a style request is not optional ;-)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] Highly inconsistent diff UI

[Ramkumar Ramachandra]

Junio C Hamano wrote:
 than ^!/^@
 which I agree were more-or-less let's add a random cruft on a whim
 without thinking things through mistakes.

I thought ^@ was invented for scripting, but can't imagine a usecase for ^!.
--
To unsubscribe from this list: send the line unsubscribe 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] t5801: properly test the test shell

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 Michael J Gruber g...@drmicha.warpmail.net writes:

 fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) introduced a
 test which was meant to skip the test unless the test shell is bash.
 Unfortunately, it tests for the availability of bash only.

 True.

 But users can
 opt to use a different shell (using SHELL_PATH) for the tests even though
 bash is available.

 At least for dash,
 21610d8 (transport-helper: clarify pushing without refspecs, 2013-04-17)
 is the commit which actually introduces a test (pushing without refspec)
 which fails to fail even though it is supposed to. It uses the
 construct:

 VAR=value function arguments

 The right fix for that is to fix that line, so that the test itself
 can run under any sane POSIX shell, isn't it?  The test in turn may
 need to run git-remote-testgit, which, without J6t's updates, only
 is usable under bash, but to make sure the test will choke on
 absence of bash, the existing check should be sufficient, no?

Curiously enough, there were a few instances of the correct set and
export environment explicitly during the life of subshell construct
already in the script.  I found only this one as problematic.

Does it fix your issue without your change?

It is a separate issue to port git-remote-testgit to POSIX (J6t
already has a two part draft), move it to git-remote-testgit.sh, and
get its shebang line preprocessed like all other shell scripts.  I
think it is worth doing.

Takers?

 t/t5801-remote-helpers.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 4dcf744..c956abd 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -118,7 +118,9 @@ test_expect_success 'pushing without refspecs' '
(cd local2 
echo content file 
git commit -a -m ten 
-   GIT_REMOTE_TESTGIT_REFSPEC= test_must_fail git push 2../error) 
+   GIT_REMOTE_TESTGIT_REFSPEC= 
+   export GIT_REMOTE_TESTGIT_REFSPEC 
+   test_must_fail git push 2../error) 
grep remote-helper doesn.t support push; refspec needed error
 '
 
--
To unsubscribe from this list: send the line unsubscribe 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] send-email: support NNTP

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 l.stelm...@samsung.com (Łukasz Stelmach) writes:

 OK, I see. Good point. Where would you recommend me to put these modules
 and how to name them? I mean I don't want to make to much mess here (;

 I would not recommend you to do any of the above now.  As I said, the
 open-coded if/elsif cascade version you posted is fine as a starting
 point.  I'd prefer to see it made work correctly as advertised first
 with documentation updates and tests.

 A Perl guru might want to come and refactor the result once the code
 solidifies, but that should be left as a separate series.

An alternative structure of the patch, if you are feeling brave,
could go like this:

 (1) With a thought process similar to what I illustrated in my
 message you responded with OK, I see. Good point., identify
 all the codepaths that will need to become methods that are
 specific to the transport (SMTP or NNTP).

 (2) Without adding any NNTP goodies, refactor the current code to
 use a single class for driving conversation with a SMTP server.
 You will implement all the methods you identified in the
 previous step and the result will become [PATCH 1/2]
 send-email: refactor to use a transport-specific class.

 (3) Implement a new class to drive conversation with an NNTP
 server, that has the same set of methods as the SMTP one you
 wrote in the previous step.  The result will become [PATCH
 2/2] send-email: introduce NNTP transport.  This would need
 updates to the documentation.

I do not necessarily suggest you to go this route, by the way. It is
up to you and depends on how proficient you are (and how comfortable
you feel) dealing with modular Perl programs.
--
To unsubscribe from this list: send the line unsubscribe 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] Make --full-history consider more merges

[Kevin Bracey]

On 25/04/2013 19:51, Junio C Hamano wrote:

Kevin Bracey ke...@bracey.fi writes:


Thanks for the test addition. Maybe we will be able to satisfy your
greed in this series. There could be more worth doing here, and I
think getting TREESAME precise is key.

It is perfectly fine to do things one step at a time.  Let's get
the --full-history change into a good shape first and worry about
the more complex case after we are done.


So do you see the rerun of try_to_simplify_commit() as acceptable? I'm 
really not happy with it - it was fine for an initial proof-of-concept, 
but it's an obvious waste of CPU cycles to recompute something we 
already figured out, and I'm uncomfortable with the fact that the 
function potentially does more than just compute TREESAME; by inspection 
it seems safe given the known context inside simplify_merges(), but it 
feels like something waiting to trip us up. The latter could be dealt 
with by breaking try_to_simplify_commit() up, but that feels like a 
diversion. I'd rather just get on and make this first patch store and 
use the per-parent-treesame flags if feasible.


Kevin

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

Re: [PATCH] git-remote-testgit: avoid process substitution

[Felipe Contreras]

On Thu, Apr 25, 2013 at 9:57 AM, Junio C Hamano gits...@pobox.com wrote:
 Johannes Sixt j.s...@viscovery.net writes:

 From: Johannes Sixt j...@kdbg.org

 Bash on Windows does not implement process substitution.

 Signed-off-by: Johannes Sixt j...@kdbg.org
 ---
 ...
 Here is a fix. It assumes that the list of refs after the import is
 a superset of the refs before the import. (Can refs be deleted
 via fast-import?)

  git-remote-testgit | 12 +++-
  1 file changed, 7 insertions(+), 5 deletions(-)

 diff --git a/git-remote-testgit b/git-remote-testgit
 index 23c9d40..e99d5fa 100755
 --- a/git-remote-testgit
 +++ b/git-remote-testgit
 @@ -91,13 +91,15 @@ do

   git fast-import ${testgitmarks_args[@]} --quiet

 - after=$(git for-each-ref --format='%(refname) %(objectname)')
 -
   # figure out which refs were updated
 - join -e 0 -o '0 1.2 2.2' -a 2 (echo $before) (echo 
 $after) |
 - while read ref a b
 + git for-each-ref --format='%(refname) %(objectname)' |
 + while read ref a
   do
 - test $a == $b  continue
 + case $before in
 + *$ref $a*)
 + continue

 This just like the original 'join' depends on the two output from
 for-each-ref to be sorted the same way, which is true and fine.  But
 I wonder one thing.  When $before has this in it:

I wonder if we should bother with this at all. The purpose of the code
was mainly to show to users that they should report the success only
if the refs have been updated, but the code is becoming more
obfuscated, a comment should do the trick. And then, we can just
report success for all the refs (and explain in the comment why).

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

[PATCH] Hold an 'unsigned long' chunk of the sha1 in obj_hash

[Thomas Rast]

The existing obj_hash is really straightforward: it holds a struct
object * and spills into the subsequent slots (linear probing), which
is good enough because it doesn't need to support deletion.

However, this arrangement has pretty bad cache locality in the case of
collisions.  Because the sha1 is contained in the object itself, it
resides in a different memory region from the hash table.  So whenever
we have to process a hash collision, we need to access (and
potentially fetch from slower caches or memory) an object that we are
not going to use again.

And probing lookups happen a lot: some simple instrumentation shows
that 'git rev-list --all --objects' on my git.git,

* 19.4M objects are accessed in lookup_object and grow_object_hash
  combined, while

* the linear probing loops in lookup_object and insert_obj_hash run a
  total of 9.4M times.

So we take a slightly different approach, and trade some memory for
better cache locality.  Namely, we change the hash table slots to
contain

  struct object *obj;
  unsigned long sha1prefix;

We use this new 'sha1prefix' field to store the first part of the
object's sha1, from which its hash table slot is computed.  This
allows us to do two things with data that resides inside the hash
table:

* In lookup_object(), we can do a pre-filtering of the probed slots;
  the probability that we need to actually peek inside any colliding
  object(s) is very small.

* In grow_object_hash(), we actually do not need to look inside the
  objects at all.  This should give a substantial speedup during
  hashtable resizing.

The choice of 'long' makes it the same size as a pointer (to which any
smaller data type would be padded anyway) on x86 and x86_64 Linuxen,
and probably many others.  So the hash table will be twice as big as
before.

I get a decent speedup, for example using git.git as a test
repository:

Test   before  after
-
0001.1: rev-list --all 0.42(0.40+0.01) 0.41(0.39+0.01)   -1.5%**
0001.2: rev-list --all --objects   2.40(2.37+0.03) 2.28(2.25+0.03)   
-5.0%***
-

And even more in linux.git:

-
0001.1: rev-list --all 3.31(3.19+0.12) 3.21(3.09+0.11)   -2.9%**
0001.2: rev-list --all --objects   27.99(27.70+0.26)   25.99(25.71+0.25) 
-7.1%***
-

Signed-off-by: Thomas Rast tr...@inf.ethz.ch
---

I expected the big win to be in grow_object_hash(), but perf says that
'git rev-list --all --objects' spends a whopping 33.6% of its time in
lookup_object(), and this change gets that down to 30.0%.

 object.c | 58 --
 1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/object.c b/object.c
index 20703f5..6b84c87 100644
--- a/object.c
+++ b/object.c
@@ -5,7 +5,12 @@
 #include commit.h
 #include tag.h
 
-static struct object **obj_hash;
+struct obj_hash_ent {
+   struct object *obj;
+   unsigned long sha1prefix;
+};
+
+static struct obj_hash_ent *obj_hash;
 static int nr_objs, obj_hash_size;
 
 unsigned int get_max_object_index(void)
@@ -15,7 +20,7 @@ unsigned int get_max_object_index(void)
 
 struct object *get_indexed_object(unsigned int idx)
 {
-   return obj_hash[idx];
+   return obj_hash[idx].obj;
 }
 
 static const char *object_type_strings[] = {
@@ -43,43 +48,52 @@ int type_from_string(const char *str)
die(invalid object type \%s\, str);
 }
 
-static unsigned int hash_obj(struct object *obj, unsigned int n)
+static unsigned long hash_sha1(const unsigned char *sha1)
 {
-   unsigned int hash;
-   memcpy(hash, obj-sha1, sizeof(unsigned int));
-   return hash % n;
+   unsigned long sha1prefix;
+   memcpy(sha1prefix, sha1, sizeof(unsigned long));
+   return sha1prefix;
 }
 
-static void insert_obj_hash(struct object *obj, struct object **hash, unsigned 
int size)
+static unsigned long hash_obj(struct object *obj)
 {
-   unsigned int j = hash_obj(obj, size);
+   return hash_sha1(obj-sha1);
+}
 
-   while (hash[j]) {
+static void insert_obj_hash_1(struct object *obj, struct obj_hash_ent *hash, 
unsigned int size,
+ unsigned long sha1prefix)
+{
+   unsigned int j = (unsigned int) sha1prefix % size;
+
+   while (hash[j].obj) {
j++;
if (j = size)
j = 0;
}
-   hash[j] = obj;
+   hash[j].obj = obj;
+   hash[j].sha1prefix = sha1prefix;
 }
 
-static unsigned int hashtable_index(const unsigned char *sha1)
+static void insert_obj_hash(struct object *obj, struct obj_hash_ent *table, 
unsigned int size)
 {
-   unsigned int i;
-   memcpy(i, sha1, 

[PATCH] prune: introduce OPT_EXPIRY_DATE() and use it

[Junio C Hamano]

Earlier we added support for --expire=all (or --expire=now) that
considers all crufts, regardless of their age, as eligible for
garbage collection by turning command argument parsers that use
approxidate() to use parse_expiry_date(), but git prune used a
built-in parse-options facility OPT_DATE() and did not benefit from
the new function.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 Documentation/technical/api-parse-options.txt | 4 
 builtin/prune.c   | 4 ++--
 parse-options-cb.c| 6 ++
 parse-options.h   | 4 
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-parse-options.txt 
b/Documentation/technical/api-parse-options.txt
index facc8c8..a8bae69 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -176,6 +176,10 @@ There are some macros to easily define options:
Introduce an option with date argument, see `approxidate()`.
The timestamp is put into `int_var`.
 
+`OPT_EXPIRY_DATE(short, long, int_var, description)`::
+   Introduce an option with expiry date argument, see 
`parse_expiry_date()`.
+   The timestamp is put into `int_var`.
+
 `OPT_CALLBACK(short, long, var, arg_str, description, func_ptr)`::
Introduce an option with argument.
The argument will be fed into the function given by `func_ptr`
diff --git a/builtin/prune.c b/builtin/prune.c
index 85843d4..b90e5cc 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -132,8 +132,8 @@ int cmd_prune(int argc, const char **argv, const char 
*prefix)
OPT__DRY_RUN(show_only, N_(do not remove, show only)),
OPT__VERBOSE(verbose, N_(report pruned objects)),
OPT_BOOL(0, progress, show_progress, N_(show progress)),
-   OPT_DATE(0, expire, expire,
-N_(expire objects older than time)),
+   OPT_EXPIRY_DATE(0, expire, expire,
+   N_(expire objects older than time)),
OPT_END()
};
char *s;
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 0de5fb1..be8c413 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -33,6 +33,12 @@ int parse_opt_approxidate_cb(const struct option *opt, const 
char *arg,
return 0;
 }
 
+int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,
+int unset)
+{
+   return parse_expiry_date(arg, (unsigned long *)opt-value);
+}
+
 int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
int unset)
 {
diff --git a/parse-options.h b/parse-options.h
index 71a39c6..8541811 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -140,6 +140,9 @@ struct option {
 #define OPT_DATE(s, l, v, h) \
{ OPTION_CALLBACK, (s), (l), (v), N_(time),(h), 0,\
  parse_opt_approxidate_cb }
+#define OPT_EXPIRY_DATE(s, l, v, h) \
+   { OPTION_CALLBACK, (s), (l), (v), N_(expiry date),(h), 0, \
+ parse_opt_expiry_date_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) \
{ OPTION_CALLBACK, (s), (l), (v), (a), (h), 0, (f) }
 #define OPT_NUMBER_CALLBACK(v, h, f) \
@@ -215,6 +218,7 @@ extern int parse_options_concat(struct option *dst, size_t, 
struct option *src);
 /*- some often used options -*/
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
 extern int parse_opt_approxidate_cb(const struct option *, const char *, int);
+extern int parse_opt_expiry_date_cb(const struct option *, const char *, int);
 extern int parse_opt_color_flag_cb(const struct option *, const char *, int);
 extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
 extern int parse_opt_with_commit(const struct option *, const char *, int);
--
To unsubscribe from this list: send the line unsubscribe 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] Make --full-history consider more merges

[Junio C Hamano]

Kevin Bracey ke...@bracey.fi writes:

 On 25/04/2013 19:51, Junio C Hamano wrote:
 Kevin Bracey ke...@bracey.fi writes:

 Thanks for the test addition. Maybe we will be able to satisfy your
 greed in this series. There could be more worth doing here, and I
 think getting TREESAME precise is key.
 It is perfectly fine to do things one step at a time.  Let's get
 the --full-history change into a good shape first and worry about
 the more complex case after we are done.

 So do you see the rerun of try_to_simplify_commit() as acceptable? I'm
 really not happy with it - it was fine for an initial
 proof-of-concept, but it's an obvious waste of CPU cycles to recompute
 something we already figured out, and I'm uncomfortable with the fact
 that the function potentially does more than just compute TREESAME; by
 inspection it seems safe given the known context inside
 simplify_merges(), but it feels like something waiting to trip us
 up.

True.

 The latter could be dealt with by breaking
 try_to_simplify_commit() up, but that feels like a diversion. I'd
 rather just get on and make this first patch store and use the
 per-parent-treesame flags if feasible.

OK.  We have survived with this corner case glitch for more than 6
years, so a fix is not ultra-urgent.  Let's try to see if we can get
it right.

How many decorations are we talking about here?  One for each merge
commit in the entire history?  Do we have a cue that can tell us
when we are really done with a commit that lets us discard the
associated data as we go on digging, keeping the size of our working
set somewhat bounded, perhaps proportional to the number of commits
in our rev_info-commits queue?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

Felipe Contreras wrote:
 diff --git a/contrib/remote-helpers/git-remote-bzr 
 b/contrib/remote-helpers/git-remote-bzr
 index aa7bc97..82bf7c7 100755
 --- a/contrib/remote-helpers/git-remote-bzr
 +++ b/contrib/remote-helpers/git-remote-bzr
 @@ -94,7 +94,7 @@ class Marks:
  return self.last_mark

  def is_marked(self, rev):
 -return self.marks.has_key(rev)
 +return rev in self.marks

Why?  Is the new form faster than the older one?

 @@ -224,7 +224,7 @@ def export_files(tree, files):
  else:
  mode = '100644'

 -# is the blog already exported?
 +# is the blob already exported?

What is this?  Whitespace?

 @@ -521,7 +521,7 @@ def c_style_unescape(string):
  return string

  def parse_commit(parser):
 -global marks, blob_marks, bmarks, parsed_refs
 +global marks, blob_marks, parsed_refs

How is this trivial?  You just removed one argument.

 @@ -555,7 +555,7 @@ def parse_commit(parser):
  mark = int(mark_ref[1:])
  f = { 'mode' : m, 'data' : blob_marks[mark] }
  elif parser.check('D'):
 -t, path = line.split(' ')
 +t, path = line.split(' ', 1)

How on earth is this trivial?  It changes the entire meaning!

 @@ -643,6 +643,7 @@ def do_export(parser):
  wt = repo.bzrdir.open_workingtree()
  wt.update()
  print ok %s % ref
 +

Whitespace?

I'm outraged by this.  What kind of changes are you pushing to
remote-hg?  A trivial cleanups bundling miscellaneous changes, with
no commit message?  Why don't you just squash everything into one
miscellaneous changes patch?
--
To unsubscribe from this list: send the line unsubscribe 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/9] remote-hg: remove extra check

[Ramkumar Ramachandra]

Felipe Contreras wrote:
 diff --git a/contrib/remote-helpers/git-remote-hg 
 b/contrib/remote-helpers/git-remote-hg
 index 5481331..0b7c81f 100755
 --- a/contrib/remote-helpers/git-remote-hg
 +++ b/contrib/remote-helpers/git-remote-hg
 @@ -332,10 +332,6 @@ def export_ref(repo, name, kind, head):
  ename = '%s/%s' % (kind, name)
  tip = marks.get_tip(ename)

 -# mercurial takes too much time checking this
 -if tip and tip == head.rev():
 -# nothing to do
 -return
  revs = xrange(tip, head.rev() + 1)

I'm surprised these four lines were even there in a previous revision.
 Again, you changed the meaning: if xrange() returns an empty range,
you must return, by extension.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-remote-testgit: avoid process substitution

[Junio C Hamano]

Felipe Contreras felipe.contre...@gmail.com writes:

 ...
 + git for-each-ref --format='%(refname) %(objectname)' |
 + while read ref a
   do
 - test $a == $b  continue
 + case $before in
 + *$ref $a*)
 + continue

 I wonder if we should bother with this at all. The purpose of the code
 was mainly to show to users that they should report the success only
 if the refs have been updated, but the code is becoming more
 obfuscated, a comment should do the trick. And then, we can just
 report success for all the refs (and explain in the comment why).

Are you proposing to say ok $ref to everything we see in the
resulting repository, even the ones the caller of remote-testgit did
not ask us to do anything with?

Wouldn't the caller be surprised if we did so?

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

Re: [PATCH 5/9] remote-hg: use hashlib instead of hg sha1 util

[Ramkumar Ramachandra]

Felipe Contreras wrote:
 To be in sync with remote-bzr.

Huh?  Why do you have to be in sync with remote-bzr?  Are you sharing
code between remote-hg and remote-bzr?

 @@ -830,7 +831,7 @@ def main(args):

  if alias[4:] == url:
  is_tmp = True
 -alias = util.sha1(alias).hexdigest()
 +alias = hashlib.sha1(alias).hexdigest()

Did you eve bother justifying this change with a line in the commit
message?  How is the new form different from the old form?
--
To unsubscribe from this list: send the line 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] check-non-portable-shell: echo -e is not portable

[Torsten Bögershausen]

Depending on the implementation, the echo command may
support options like -n -e -E or no options at all.

The only portable usage of echo is without any options,
which means that no parameter may start with a hyphen.

check-non-portable-shell.pl checks if echo -n is used.
Improve it to reject any parameter starting with a hyphen
and recommend the usage of printf instead

Signed-off-by: Torsten Bögershausen tbo...@web.de
---
 t/check-non-portable-shell.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 8b5a71d..ff87611 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -17,7 +17,7 @@ sub err {
 while () {
chomp;
/^\s*sed\s+-i/ and err 'sed -i is not portable';
-   /^\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
+   /^\s*echo\s+-(\S)\s+/ and err echo -$1 is not portable (please use 
printf);
/^\s*declare\s+/ and err 'arrays/declare not portable';
/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
/test\s+[^=]*==/ and err 'test a == b is not portable (please use =)';
-- 
1.8.2.1.614.g66d7af5


--
To unsubscribe from this list: send the line 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] check-non-portable-shell: Check for non portable grep usage

[Torsten Bögershausen]

The make test-lint could check for some common non portable
grep usage:

The + as a qualifier is not a basic regular expression.
Some grep understand \+, but in general egrep should be used.

The \t can not be used to find an ASCII TAB, neither
can \s be used to find a whitespace

Signed-off-by: Torsten Bögershausen tbo...@web.de
---
 t/check-non-portable-shell.pl | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index ff87611..58f8e2c 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -16,6 +16,8 @@ sub err {
 
 while () {
chomp;
+   / e?grep\s+\[^]*\\([st])/ and err grep \\$1 is not portable);
+   / [^e]grep.*\\\+/ and err 'grep with \+ is not portable (please use 
egrep)';
/^\s*sed\s+-i/ and err 'sed -i is not portable';
/^\s*echo\s+-(\S)\s+/ and err echo -$1 is not portable (please use 
printf);
/^\s*declare\s+/ and err 'arrays/declare not portable';
-- 
1.8.2.1.614.g66d7af5


--
To unsubscribe from this list: send the line 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] test-lint-shell-syntax: Allow bash syntax in bash tests

[Torsten Bögershausen]

t/check-non-portable-sh.pl checks for bash only syntax like test $a == $b
This is wrong when bash is used.
Switch to bash mode and skip this test

Signed-off-by: Torsten Bögershausen tbo...@web.de
---
 t/check-non-portable-shell.pl | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 58f8e2c..1ca8c8b 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -7,23 +7,26 @@ use strict;
 use warnings;
 
 my $exit_code=0;
+my $bashmode=0;
 
 sub err {
my $msg = shift;
-   print $ARGV:$.: error: $msg: $_\n;
+   print $ARGV:$.: error: (bashmode=$bashmode) $msg: $_\n;
$exit_code = 1;
 }
 
 while () {
chomp;
+   /^\. \.\/lib\-bash.sh/ and $bashmode=1;
/ e?grep\s+\[^]*\\([st])/ and err grep \\$1 is not portable);
/ [^e]grep.*\\\+/ and err 'grep with \+ is not portable (please use 
egrep)';
/^\s*sed\s+-i/ and err 'sed -i is not portable';
/^\s*echo\s+-(\S)\s+/ and err echo -$1 is not portable (please use 
printf);
/^\s*declare\s+/ and err 'arrays/declare not portable';
/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
-   /test\s+[^=]*==/ and err 'test a == b is not portable (please use =)';
+   $bashmode==0 and /test\s+[^=]*==/ and err 'test a == b is not 
portable (please use =)';
# this resets our $. for each file
close ARGV if eof;
+   $bashmode=0 if eof;
 }
 exit $exit_code;
-- 
1.8.2.1.614.g66d7af5


--
To unsubscribe from this list: send the line 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] check-non-portable-shell: export X=Y usage

[Torsten Bögershausen]

The make test-lint could check for lines like
export X=Y

This is bash syntax and should be written in 2 lines:
X=Y
export X

Signed-off-by: Torsten Bögershausen tbo...@web.de
---
 t/check-non-portable-shell.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 1ca8c8b..6a11b02 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -11,7 +11,7 @@ my $bashmode=0;
 
 sub err {
my $msg = shift;
-   print $ARGV:$.: error: (bashmode=$bashmode) $msg: $_\n;
+   print $ARGV:$.: error: $msg: $_\n;
$exit_code = 1;
 }
 
@@ -25,6 +25,7 @@ while () {
/^\s*declare\s+/ and err 'arrays/declare not portable';
/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
$bashmode==0 and /test\s+[^=]*==/ and err 'test a == b is not 
portable (please use =)';
+   $bashmode==0 and /^\s*export\s+\S+=\S+/ and err 'export X=Y is not 
portable (please split into 2 lines)';
# this resets our $. for each file
close ARGV if eof;
$bashmode=0 if eof;
-- 
1.8.2.1.614.g66d7af5


--
To unsubscribe from this list: send the line 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] t9501: Do not use export X=Y

[Torsten Bögershausen]

Spilt lines like export X=Y into 2 lines:
X=Y
export X

Signed-off-by: Torsten Bögershausen tbo...@web.de
---
 t/t9501-gitweb-standalone-http-status.sh | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/t/t9501-gitweb-standalone-http-status.sh 
b/t/t9501-gitweb-standalone-http-status.sh
index ef86948..d3a5bac 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -130,7 +130,8 @@ test_expect_success DATE_PARSER 'modification: feed 
last-modified' '
 test_debug 'cat gitweb.headers'
 
 test_expect_success DATE_PARSER 'modification: feed if-modified-since 
(modified)' '
-   export HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
+   HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
+   export HTTP_IF_MODIFIED_SINCE 
test_when_finished unset HTTP_IF_MODIFIED_SINCE 
gitweb_run p=.git;a=atom;h=master 
grep Status: 200 OK gitweb.headers
@@ -138,7 +139,8 @@ test_expect_success DATE_PARSER 'modification: feed 
if-modified-since (modified)
 test_debug 'cat gitweb.headers'
 
 test_expect_success DATE_PARSER 'modification: feed if-modified-since 
(unmodified)' '
-   export HTTP_IF_MODIFIED_SINCE=Thu, 7 Apr 2005 22:14:13 + 
+   HTTP_IF_MODIFIED_SINCE=Thu, 7 Apr 2005 22:14:13 + 
+   export HTTP_IF_MODIFIED_SINCE 
test_when_finished unset HTTP_IF_MODIFIED_SINCE 
gitweb_run p=.git;a=atom;h=master 
grep Status: 304 Not Modified gitweb.headers
@@ -153,7 +155,8 @@ test_expect_success DATE_PARSER 'modification: snapshot 
last-modified' '
 test_debug 'cat gitweb.headers'
 
 test_expect_success DATE_PARSER 'modification: snapshot if-modified-since 
(modified)' '
-   export HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
+   HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
+   export HTTP_IF_MODIFIED_SINCE 
test_when_finished unset HTTP_IF_MODIFIED_SINCE 
gitweb_run p=.git;a=snapshot;h=master;sf=tgz 
grep Status: 200 OK gitweb.headers
@@ -161,7 +164,8 @@ test_expect_success DATE_PARSER 'modification: snapshot 
if-modified-since (modif
 test_debug 'cat gitweb.headers'
 
 test_expect_success DATE_PARSER 'modification: snapshot if-modified-since 
(unmodified)' '
-   export HTTP_IF_MODIFIED_SINCE=Thu, 7 Apr 2005 22:14:13 + 
+   HTTP_IF_MODIFIED_SINCE=Thu, 7 Apr 2005 22:14:13 + 
+   export HTTP_IF_MODIFIED_SINCE 
test_when_finished unset HTTP_IF_MODIFIED_SINCE 
gitweb_run p=.git;a=snapshot;h=master;sf=tgz 
grep Status: 304 Not Modified gitweb.headers
@@ -170,7 +174,8 @@ test_debug 'cat gitweb.headers'
 
 test_expect_success DATE_PARSER 'modification: tree snapshot' '
ID=`git rev-parse --verify HEAD^{tree}` 
-   export HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
+   HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
+   export HTTP_IF_MODIFIED_SINCE 
test_when_finished unset HTTP_IF_MODIFIED_SINCE 
gitweb_run p=.git;a=snapshot;h=$ID;sf=tgz 
grep Status: 200 OK gitweb.headers 
-- 
1.8.2.1.614.g66d7af5


--
To unsubscribe from this list: send the line 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] t9501: Use PERL_PATH instead of perl

[Torsten Bögershausen]

The prerequisite checker for DATE_PARSER should use
$PERL_PATH instead of perl

Signed-off-by: Torsten Bögershausen tbo...@web.de
---
 t/t9501-gitweb-standalone-http-status.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9501-gitweb-standalone-http-status.sh 
b/t/t9501-gitweb-standalone-http-status.sh
index d3a5bac..98c870e 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -16,8 +16,8 @@ code and message.'
 # Gitweb only provides the functionality tested by the 'modification times'
 # tests if it can access a date parser from one of these modules:
 #
-perl -MHTTP::Date -e 0 /dev/null 21  test_set_prereq DATE_PARSER
-perl -MTime::ParseDate -e 0 /dev/null 21  test_set_prereq DATE_PARSER
+$PERL_PATH -MHTTP::Date -e 0 /dev/null 21  test_set_prereq DATE_PARSER
+$PERL_PATH -MTime::ParseDate -e 0 /dev/null 21  test_set_prereq 
DATE_PARSER
 
 # --
 # snapshot settings
-- 
1.8.2.1.614.g66d7af5

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

Re: Itches with the current rev spec

[Phil Hord]

On Thu, Apr 25, 2013 at 1:07 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 2. git rebase -i master fails unless I've rebased my branch on top of
 master.  I always wished I could do the equivalent of 'git rebase -i
 master..', but I can't.

In what way does it fail?   It seems to work ok for me.  Do you mean
that it chooses extra commits you did not want?

Maybe you expect rebase--interactive will only result in changes to
commits you touch in the todo list and it will not actually rebase
anything.   Is that the goal?

I have been thinking of adding a targeted rebase -i extension.  I
often use rebase -i to change one commit in recent history or to
squash some fixup into place.  The trip through $EDITOR to do this
seems disruptive to my thinking.   So I would like to be able to do
this:

   git rebase --edit $REF

which should act the same as

  GIT_EDITOR='sed -i s/^pick $REF/edit $REF/' \
  git rebase -i $REF^

Except that $REF could be any ref and not just the exact
SHA1-abbreviation given in todo.

The change I imagine allows --fixup, --reword,  --squash, etc.  It
might even allow multiple instances of each.

I haven't thought through how to handle the case where there are
merges in the way, but I do already suppose that the command will
simply fail if a ref is not an ancestor of HEAD.

Maybe this is too simple for your workflow, though. As I said, I did
not understand your itch.

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

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Thu, Apr 25, 2013 at 1:19 PM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 Felipe Contreras wrote:
 diff --git a/contrib/remote-helpers/git-remote-bzr 
 b/contrib/remote-helpers/git-remote-bzr
 index aa7bc97..82bf7c7 100755
 --- a/contrib/remote-helpers/git-remote-bzr
 +++ b/contrib/remote-helpers/git-remote-bzr
 @@ -94,7 +94,7 @@ class Marks:
  return self.last_mark

  def is_marked(self, rev):
 -return self.marks.has_key(rev)
 +return rev in self.marks

 Why?  Is the new form faster than the older one?

has_key is deprecated.

 @@ -224,7 +224,7 @@ def export_files(tree, files):
  else:
  mode = '100644'

 -# is the blog already exported?
 +# is the blob already exported?

 What is this?  Whitespace?

s/blog/blob/

 @@ -521,7 +521,7 @@ def c_style_unescape(string):
  return string

  def parse_commit(parser):
 -global marks, blob_marks, bmarks, parsed_refs
 +global marks, blob_marks, parsed_refs

 How is this trivial?  You just removed one argument.

It's not an argument.

 @@ -555,7 +555,7 @@ def parse_commit(parser):
  mark = int(mark_ref[1:])
  f = { 'mode' : m, 'data' : blob_marks[mark] }
  elif parser.check('D'):
 -t, path = line.split(' ')
 +t, path = line.split(' ', 1)

 How on earth is this trivial?  It changes the entire meaning!

And nobody has noticed any problem.

 @@ -643,6 +643,7 @@ def do_export(parser):
  wt = repo.bzrdir.open_workingtree()
  wt.update()
  print ok %s % ref
 +

 Whitespace?

Aka. trivial.

 I'm outraged by this.  What kind of changes are you pushing to
 remote-hg?  A trivial cleanups bundling miscellaneous changes, with
 no commit message?

There are no miscellaneous changes other than the *possible* fix for
deleted files. Which we don't know if it would actually fix anything,
but as far as we know if it's a bug, nobody has seen it, and if it
isn't, it's very unlikely that anybody is relying on the current
behavior.

Plus the change seems to be obviously correct, as it comes from
remote-hg, where somebody appeared to have found a bug.

That being said, I do remember writing an explanation for this in the
commit message:

--
commit ca8c02dc7ea6395b1c864296f2500b718892fab8
Reflog: HEAD@{143} (Felipe Contreras felipe.contre...@gmail.com)
Reflog message: rebase -i (fixup): remote-bzr: trivial cleanups
Author: Felipe Contreras felipe.contre...@gmail.com
Date:   Tue Apr 23 18:29:49 2013 -0500

remote-bzr: trivial cleanups

Mostly from remote-hg. It's possible that there's a fix to delete files
with spaces.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---

Yeap, there it is. It was just squashed by mistake.

But I do not care that much really. The patch is good either way, if
you don't like it, you go ahead and fix it, because I won't. I have
174 remote-helper related patches in my queue, and nobody benefits
from rambling about a one liner that is obviously correct, not you,
not me, not the users, not the developers.

Junio of course might disagree and drop this patch, but then he would
need to deal with the fallout of possible conflicts. Or he can do the
sensible thing and pick the commit message above. I have real issues
to deal with, and I think the less-than-perfect commit messages in a
*contrib* script that is extremely recent is a small price to pay for
having nice and workable bzr and mercurial remote-helpers as soon as
possible; our users would thank us, and in fact, they already are.

In my hurry to reorganize all the commits of my fourteen remote-helper
branches, I squashed the commit message of a trivial fix into trivial
cleanups. Big whooping deal.

 Why don't you just squash everything into one
 miscellaneous changes patch?

Hyperbole much?

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/9] remote-hg: remove extra check

[Felipe Contreras]

On Thu, Apr 25, 2013 at 1:23 PM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 Felipe Contreras wrote:
 diff --git a/contrib/remote-helpers/git-remote-hg 
 b/contrib/remote-helpers/git-remote-hg
 index 5481331..0b7c81f 100755
 --- a/contrib/remote-helpers/git-remote-hg
 +++ b/contrib/remote-helpers/git-remote-hg
 @@ -332,10 +332,6 @@ def export_ref(repo, name, kind, head):
  ename = '%s/%s' % (kind, name)
  tip = marks.get_tip(ename)

 -# mercurial takes too much time checking this
 -if tip and tip == head.rev():
 -# nothing to do
 -return
  revs = xrange(tip, head.rev() + 1)

 I'm surprised these four lines were even there in a previous revision.
  Again, you changed the meaning: if xrange() returns an empty range,
 you must return, by extension.

I'm not going to go back in history, but we were not always using
xrange, but the mercurial API helper, which was dead slow, and in the
end did basically an xrange().

-- 
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] git-remote-testgit: avoid process substitution

[Felipe Contreras]

On Thu, Apr 25, 2013 at 1:25 PM, Junio C Hamano gits...@pobox.com wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 ...
 + git for-each-ref --format='%(refname) %(objectname)' |
 + while read ref a
   do
 - test $a == $b  continue
 + case $before in
 + *$ref $a*)
 + continue

 I wonder if we should bother with this at all. The purpose of the code
 was mainly to show to users that they should report the success only
 if the refs have been updated, but the code is becoming more
 obfuscated, a comment should do the trick. And then, we can just
 report success for all the refs (and explain in the comment why).

 Are you proposing to say ok $ref to everything we see in the
 resulting repository, even the ones the caller of remote-testgit did
 not ask us to do anything with?

 Wouldn't the caller be surprised if we did so?

Why would it? The only effective difference is what you'll see
reported in the UI, but there's no user here.

-- 
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 1/9] remote-bzr: trivial cleanups

[Stefano Lattarini]

On 04/25/2013 08:19 PM, Ramkumar Ramachandra wrote:
 Felipe Contreras wrote:
 diff --git a/contrib/remote-helpers/git-remote-bzr 
 b/contrib/remote-helpers/git-remote-bzr
 index aa7bc97..82bf7c7 100755
 --- a/contrib/remote-helpers/git-remote-bzr
 +++ b/contrib/remote-helpers/git-remote-bzr
 @@ -94,7 +94,7 @@ class Marks:
  return self.last_mark

  def is_marked(self, rev):
 -return self.marks.has_key(rev)
 +return rev in self.marks
 
 Why?  Is the new form faster than the older one?

I think the has_key() method is deprecated in modern python,
and the 'key in dict' usage is more idiomatic.

 @@ -224,7 +224,7 @@ def export_files(tree, files):
  else:
  mode = '100644'

 -# is the blog already exported?
 +# is the blob already exported?
 
 What is this?  Whitespace?

Typofix: s/blog/blob/

 @@ -521,7 +521,7 @@ def c_style_unescape(string):
  return string

  def parse_commit(parser):
 -global marks, blob_marks, bmarks, parsed_refs
 +global marks, blob_marks, parsed_refs
 
 How is this trivial?  You just removed one argument.

Maybe bmarks was no longer used there as a global variable
(left-over from previous patches?), so there is no longer any
need to declare it global.

 @@ -555,7 +555,7 @@ def parse_commit(parser):
  mark = int(mark_ref[1:])
  f = { 'mode' : m, 'data' : blob_marks[mark] }
  elif parser.check('D'):
 -t, path = line.split(' ')
 +t, path = line.split(' ', 1)
 
 How on earth is this trivial?  It changes the entire meaning!

Indeed, that should best go in a separate path with a proper
explanation in the commit message.

 @@ -643,6 +643,7 @@ def do_export(parser):
  wt = repo.bzrdir.open_workingtree()
  wt.update()
  print ok %s % ref
 +
 
 Whitespace?

Isn't that obvious?

 I'm outraged by this.  What kind of changes are you pushing to
 remote-hg?  A trivial cleanups bundling miscellaneous changes, with
 no commit message?  Why don't you just squash everything into one
 miscellaneous changes patch?

I have no opinion on this, so I won't comment.

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

Re: [PATCH 5/9] remote-hg: use hashlib instead of hg sha1 util

[Felipe Contreras]

On Thu, Apr 25, 2013 at 1:25 PM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 Felipe Contreras wrote:
 To be in sync with remote-bzr.

 Huh?  Why do you have to be in sync with remote-bzr?  Are you sharing
 code between remote-hg and remote-bzr?

We don't have to.

 @@ -830,7 +831,7 @@ def main(args):

  if alias[4:] == url:
  is_tmp = True
 -alias = util.sha1(alias).hexdigest()
 +alias = hashlib.sha1(alias).hexdigest()

 Did you eve bother justifying this change with a line in the commit
 message?  How is the new form different from the old form?

Why would it be any difference? It's a hex version of the SHA-1
digest. It would be the same in every language and every tool.

And a bit of context: historically the reason I started remote-bzr was
to show that we didn't need the *huge* infrastructure that is sitting
git_remote_helpers, which is nothing compared to what was prepared to
be merged for msysgit's remote-hg. I wrote it as a proof-of-concept to
show we didn't need a framework, and if we do, it would only be clear
after having _two_ remote helpers, which we now do. It might make
sense to refactor the common parts into a framework later on, so
having them in sync as much as it's reasonably possible makes sense.

But if even if it wasn't, there's nothing wrong with this patch. Also,
who knows, maybe old versions of mercurial don't have util.sha1(), or
maybe newer ones will move it, who knows.

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 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Thu, Apr 25, 2013 at 2:29 PM, Stefano Lattarini
stefano.lattar...@gmail.com wrote:
 On 04/25/2013 08:19 PM, Ramkumar Ramachandra wrote:

 @@ -521,7 +521,7 @@ def c_style_unescape(string):
  return string

  def parse_commit(parser):
 -global marks, blob_marks, bmarks, parsed_refs
 +global marks, blob_marks, parsed_refs

 How is this trivial?  You just removed one argument.

 Maybe bmarks was no longer used there as a global variable
 (left-over from previous patches?), so there is no longer any
 need to declare it global.

Even more, it never was used, it was a mistake carried when copying
this method from remote-hg; we don't have bookmarks in bazaar. And
bmarks wasn't even used in this method in remote-hg either =/

But it would be obvious that it was not used once one ran the tests
and they passed, which they do.

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] inotify to minimize stat() calls

[Robert Zeh]

On Thu, Apr 25, 2013 at 3:18 AM, Thomas Rast tr...@inf.ethz.ch wrote:

 Robert Zeh robert.allan@gmail.com writes:

  Here is a patch that creates a daemon that tracks file
  state with inotify, writes it out to a file upon request,
  and changes most of the calls to stat to use said cache.
 
  It has bugs, but I figured it would be smarter to see
  if the approach was acceptable at all before spending the
  time to root the bugs out.

 Thanks for tackling this; it's probably about time we got a inotify
 support :-(

  I've implemented the communication with a file, and not a socket,
  because I think implementing a socket is going to create
  security issues on multiuser systems.  For example, would a
  socket allow stat information to cross user boundaries?

 This ties in with an issue discussed in an earlier thread:

   http://thread.gmane.org/gmane.comp.version-control.git/217817/focus=218307

 The conclusion there was that the default limits are set such that it is
 not feasible to run one daemon per repository (that would quickly hit
 the limits when e.g. iterating all repos in a typical android tree using
 repo).

 So whatever you use for communication needs to work as a global daemon.

 I'd just trust the SSH folks to know about security; on my system
 ssh-agent creates

   /tmp/ssh-RANDOMSTRING/agent.PID

 where the directory has mode 0700, and the file is a unit socket with
 mode 0600.  That should make doubly sure that no other user can open the
 socket.

   filechange-cache.c   | 203
  +++

 Is your MUA wrapping the patch?

Almost certainly.  I'll double check before I send off the next patch.

  +static void watch_directory(int inotify_fd)
  +{
  + char buf[PATH_MAX];
  +
  + if (!getcwd(buf, sizeof(buf)))
  + die_errno(Unable to get current directory);
  +
  + int i = 0;
  + struct dir_struct dir;
  + const char *pathspec[1] = { buf, NULL };
  +
  + memset(dir, 0, sizeof(dir));
  + setup_standard_excludes(dir);
  +
  + fill_directory(dir, pathspec);
  + for(i = 0; i  dir.nr; i++) {
  + struct dir_entry *ent = dir.entries[i];
  + watch_file(inotify_fd, ent-name);
  + free(ent);
  + }

 I don't get this bit.  The lstat() are run over all files listed in the
 index.  So shouldn't your daemon watch exactly those (or rather, all
 dirnames of such files)?
I believe that fill_directory is handling watching only files in the index.
I had some problems a while back when I was only watching the
directory with some of the inotify structures coming back empty, which
is why I started watching each individual file.

 The actual directory contents are only needed to find untracked files,
 and there would be a lot of complication surrounding that, so I suggest
 saving that for later (and for now measuring the speedup with 'git
 status -uno'!).
The speed up test is a good idea.

 For example, you'd have to actually watch and re-read all .gitignore
 files, and the .git/info/exclude, and the core.excludesfile, to see if
 your notion of an ignored file became stale.
The thought in the back of my head was to simple have the daemon
restart if one of those files changed, under the assumption that a
restart wasn't that expensive, and that it would be complicated to check.


 Also, you seem to call watch_directory() only on the current(?) dir, but
 you need to recursively set up watches for all directories in the
 repository.

I'm calling fill_directory to get the list of files to watch; it appears to
be handling the recursion for me.  It also appears to be handling filtering
out all of the untracked files, etc.

  + while (1) {
  + int i = 0;
  + length = read(inotify_fd, buffer, sizeof(buffer));
  + for(i = 0; i  length; ) {
  + struct inotify_event *event =
  + (struct inotify_event*)(buffer+i);
  + /* printf(event: %d %x %d %s\n, event-wd, 
  event-mask,
  +event-len, event-name); */
  + if (request_watch_descriptor == event-wd) {
  + write_stat_cache();
  + } else if (root_directory_watch_descriptor
  +== event-wd) {
  + printf(root directory died!\n);
  + exit(0);
  + } else if (event-mask  IN_Q_OVERFLOW) {
  + restart();

 Good.

  + } else if (event-mask  IN_MODIFY) {
  + if (event-len)
  + update_stat_cache(event-name);
  + }

 So whenever a file changes, you stat() it.  That's good for simplicity
 now, but I suspect it will provide some optimization opportunities
 later.
I figured it would be a good idea to get things working, and then 

[PATCH 1/3] pretty: simplify input line length calculation in pp_user_info()

[René Scharfe]

Instead of searching for LF and NUL with two strchr() calls use a single
strchrnul() call.  We don't need to check if the returned pointer is NULL
because either we'll find the NUL at the end of line, or the caller
forgot to NUL-terminate the string and we'll overrun the buffer in any
case.  Also we don't need to pass LF or NUL to split_ident_line() as it
ignores it anyway.

Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx
---
 pretty.c | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/pretty.c b/pretty.c
index ba31481..e51c993 100644
--- a/pretty.c
+++ b/pretty.c
@@ -413,7 +413,6 @@ void pp_user_info(const struct pretty_print_context *pp,
struct strbuf name;
struct strbuf mail;
struct ident_split ident;
-   int linelen;
char *line_end;
const char *mailbuf, *namebuf;
size_t namelen, maillen;
@@ -422,18 +421,10 @@ void pp_user_info(const struct pretty_print_context *pp,
if (pp-fmt == CMIT_FMT_ONELINE)
return;
 
-   line_end = strchr(line, '\n');
-   if (!line_end) {
-   line_end = strchr(line, '\0');
-   if (!line_end)
-   return;
-   }
-
-   linelen = ++line_end - line;
-   if (split_ident_line(ident, line, linelen))
+   line_end = strchrnul(line, '\n');
+   if (split_ident_line(ident, line, line_end - line))
return;
 
-
mailbuf = ident.mail_begin;
maillen = ident.mail_end - ident.mail_begin;
namebuf = ident.name_begin;
-- 
1.8.2.1

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

[PATCH 2/3] pretty: simplify output line length calculation in pp_user_info()

[René Scharfe]

Keep namelen unchanged and don't use it to hold a value that we're not
interested in anyway -- we can use maillen and the constant part
directly instead.  This simplifies the code slightly and prepares for
the next patch that makes use of the original value of namelen.

Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx
---
 pretty.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/pretty.c b/pretty.c
index e51c993..6861997 100644
--- a/pretty.c
+++ b/pretty.c
@@ -439,8 +439,6 @@ void pp_user_info(const struct pretty_print_context *pp,
strbuf_add(mail, mailbuf, maillen);
strbuf_add(name, namebuf, namelen);
 
-   namelen = name.len + mail.len + 3; /* ' ' + '' + '' */
-
if (pp-fmt == CMIT_FMT_EMAIL) {
strbuf_addstr(sb, From: );
if (needs_rfc2047_encoding(name.buf, name.len, 
RFC2047_ADDRESS)) {
@@ -457,9 +455,10 @@ void pp_user_info(const struct pretty_print_context *pp,
strbuf_add_wrapped_bytes(sb, name.buf, name.len,
 -6, 1, max_length);
}
-   if (namelen - name.len + last_line_length(sb)  max_length)
-   strbuf_addch(sb, '\n');
 
+   if (max_length 
+   last_line_length(sb) + strlen( ) + maillen + strlen())
+   strbuf_addch(sb, '\n');
strbuf_addf(sb,  %s\n, mail.buf);
} else {
strbuf_addf(sb, %s: %.*s%s %s\n, what,
-- 
1.8.2.1

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

[PATCH 3/3] pretty: remove intermediate strbufs from pp_user_info()

[René Scharfe]

Use namebuf/namelen and mailbuf/maillen directly instead of copying
their contents into strbufs first.

Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx
---
 pretty.c | 29 +
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/pretty.c b/pretty.c
index 6861997..9e43154 100644
--- a/pretty.c
+++ b/pretty.c
@@ -410,8 +410,6 @@ void pp_user_info(const struct pretty_print_context *pp,
  const char *what, struct strbuf *sb,
  const char *line, const char *encoding)
 {
-   struct strbuf name;
-   struct strbuf mail;
struct ident_split ident;
char *line_end;
const char *mailbuf, *namebuf;
@@ -433,42 +431,33 @@ void pp_user_info(const struct pretty_print_context *pp,
if (pp-mailmap)
map_user(pp-mailmap, mailbuf, maillen, namebuf, namelen);
 
-   strbuf_init(mail, 0);
-   strbuf_init(name, 0);
-
-   strbuf_add(mail, mailbuf, maillen);
-   strbuf_add(name, namebuf, namelen);
-
if (pp-fmt == CMIT_FMT_EMAIL) {
strbuf_addstr(sb, From: );
-   if (needs_rfc2047_encoding(name.buf, name.len, 
RFC2047_ADDRESS)) {
-   add_rfc2047(sb, name.buf, name.len,
+   if (needs_rfc2047_encoding(namebuf, namelen, RFC2047_ADDRESS)) {
+   add_rfc2047(sb, namebuf, namelen,
encoding, RFC2047_ADDRESS);
max_length = 76; /* per rfc2047 */
-   } else if (needs_rfc822_quoting(name.buf, name.len)) {
+   } else if (needs_rfc822_quoting(namebuf, namelen)) {
struct strbuf quoted = STRBUF_INIT;
-   add_rfc822_quoted(quoted, name.buf, name.len);
+   add_rfc822_quoted(quoted, namebuf, namelen);
strbuf_add_wrapped_bytes(sb, quoted.buf, quoted.len,
-6, 1, max_length);
strbuf_release(quoted);
} else {
-   strbuf_add_wrapped_bytes(sb, name.buf, name.len,
+   strbuf_add_wrapped_bytes(sb, namebuf, namelen,
 -6, 1, max_length);
}
 
if (max_length 
last_line_length(sb) + strlen( ) + maillen + strlen())
strbuf_addch(sb, '\n');
-   strbuf_addf(sb,  %s\n, mail.buf);
+   strbuf_addf(sb,  %.*s\n, (int)maillen, mailbuf);
} else {
-   strbuf_addf(sb, %s: %.*s%s %s\n, what,
- (pp-fmt == CMIT_FMT_FULLER) ? 4 : 0,
- , name.buf, mail.buf);
+   strbuf_addf(sb, %s: %.*s%.*s %.*s\n, what,
+   (pp-fmt == CMIT_FMT_FULLER) ? 4 : 0, ,
+   (int)namelen, namebuf, (int)maillen, mailbuf);
}
 
-   strbuf_release(mail);
-   strbuf_release(name);
-
switch (pp-fmt) {
case CMIT_FMT_MEDIUM:
strbuf_addf(sb, Date:   %s\n,
-- 
1.8.2.1

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

Re: [PATCH] inotify to minimize stat() calls

[Robert Zeh]

On Wed, Apr 24, 2013 at 4:32 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Thu, Apr 25, 2013 at 3:20 AM, Robert Zeh robert.allan@gmail.com 
 wrote:
 Here is a patch that creates a daemon that tracks file
 state with inotify, writes it out to a file upon request,
 and changes most of the calls to stat to use said cache.

 It has bugs, but I figured it would be smarter to see
 if the approach was acceptable at all before spending the
 time to root the bugs out.

 Any preliminary performance numbers? How does it do compared to
 no-inotify version? When only a few files are changed? When half the
 repo is changed?

No numbers yet; I'm still working on correctness.  What I posted does
not pass all of the tests.

I like your ideas for performance tests.  My testing setup is
a VirtualBox instance on MacOS, so I'm not convinced that my numbers
will be meaningful.  The one thing I can report is that running the daemon
doesn't affect compilation performance.

The real win for this type of cache is Windows, where the file system
is slow.

 I've implemented the communication with a file, and not a socket, because I
 think implementing a socket is going to create
 security issues on multiuser systems.  For example, would a
 socket allow stat information to cross user boundaries?

 I think UNIX socket on Linux at least respects file permissions. But
 unix(7) follows with This behavior differs from many BSD-derived
 systems which ignore permissions for Unix sockets. Sighh

  abspath.c|   9 ++-
  bisect.c |   3 +-
  check-racy.c |   2 +-
  combine-diff.c   |   3 +-
  command-list.txt |   1 +
  config.c |   3 +-
  copy.c   |   3 +-
  diff-lib.c   |   3 +-
  diff-no-index.c  |   3 +-
  diff.c   |   9 ++-
  diffcore-order.c |   3 +-
  dir.c|   4 +-
  filechange-cache.c   | 203
 +++
  filechange-cache.h   |  20 +
  filechange-daemon.c  | 164 +
  filechange-printer.c |  13 
  git.c|  27 +++
  ll-merge.c   |   3 +-
  merge-recursive.c|   5 +-
  name-hash.c  |   3 +-
  name-hash.h  |   1 +
  notes-merge.c|   3 +-
  path.c   |   5 +-
  read-cache.c |  11 +--
  rerere.c |   7 +-
  setup.c  |   5 +-
  test-chmtime.c   |   2 +-
  test-wildmatch.c |   2 +-
  unpack-trees.c   |   6 +-
  29 files changed, 486 insertions(+), 40 deletions(-)
  create mode 100644 filechange-cache.c
  create mode 100644 filechange-cache.h
  create mode 100644 filechange-daemon.c
  create mode 100644 filechange-printer.c
  create mode 100644 name-hash.h

 Can you just replace lstat/stat with cached_lstat/stat inside
 git-compat-util.h and not touch all files at once? I think you may
 need to deal with paths outside working directory. But because you're
 using lookup table, that should be no problem.

That's a good idea; but there are a few places where you want to call
the uncached stat because calling the cache leads to recursion or
you bump into things that haven't been setup yet.  Any ideas how to
handle that?
--
To unsubscribe from this list: send the line unsubscribe 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] inotify to minimize stat() calls

[Thomas Rast]

Robert Zeh robert.allan@gmail.com writes:

 On Thu, Apr 25, 2013 at 3:18 AM, Thomas Rast tr...@inf.ethz.ch wrote:

 I don't get this bit.  The lstat() are run over all files listed in the
 index.  So shouldn't your daemon watch exactly those (or rather, all
 dirnames of such files)?
 I believe that fill_directory is handling watching only files in the index.
 I had some problems a while back when I was only watching the
 directory with some of the inotify structures coming back empty, which
 is why I started watching each individual file.

This probably doesn't scale well enough.  For example on my system the
maximum number of watches I can set[1] is 64k.  linux.git contains 38k
files and the total number of files in all repos of an android clone I
have lying around is almost 300k.

Can you clarify what went wrong if you only watch directories?  After
all the events should be the same, except that you need to reassemble
the actual filename from the 'name' field in inotify_event and the
directory name associated with the watch descriptor.

I'll keep the rest of your mail for another reply ;-)

[1]  /proc/sys/fs/inotify/max_user_watches

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

[PATCH v4 00/11] Add new git-related helper to contrib

[Felipe Contreras]

Hi,

Previously known as git-cc-cmd, I've renamed it git-related (tentatively). I
removed support for aliases, as I don't think it's very useful, and I've added
support for mailmap, which I think covers similar use-cases.

This script allows you to get a list of relevant persons to Cc when sending a
patch series.

  % git cc-cmd v1.8.1.6^^1..v1.8.1.6^^2
  Junio C Hamano gits...@pobox.com (signer: 84%, author: 15%)
  Nguyễn Thái Ngọc Duy pclo...@gmail.com (author: 38%, signer: 7%)
  Michael Haggerty mhag...@alum.mit.edu (author: 15%)
  Jean-Noel Avila jn.av...@free.fr (signer: 7%)
  Jean-Noël AVILA avila...@gmail.com (author: 7%)
  Henrik Grubbström gru...@grubba.org (author: 7%)
  Clemens Buchacher dri...@aon.at (author: 7%)
  Joshua Jensen jjen...@workspacewhiz.com (author: 7%)
  Johannes Sixt j...@kdbg.org (signer: 7%)

It finds people that might be interesting in a patch, by going back through the
history for each single hunk modified, and finding people that reviewed,
acknowledge, signed, or authored the code the patch is modifying.

It does this by running 'git blame' incrementally on each hunk, and then
parsing the commit message. After gathering all the relevant people, it groups
them to show what exactly was their role when the participated in the
development of the relevant commit, and on how many relevant commits they
participated. They are only displayed if they pass a minimum threshold of
participation.

The code finds the changes in each commit in the list, runs 'git blame'
to see which other commits are relevant to those lines, and then adds
the author and signer to the list.

Finally, it calculates what percentage of the total relevant commits
each person was involved in, and if it passes the threshold, it goes in.

You can also choose to show the commits themselves:

  % git cc-cmd --commits v1.8.1.6^^1..v1.8.1.6^^2
  9db9eec attr: avoid calling find_basename() twice per path
  94bc671 Add directory pattern matching to attributes
  82dce99 attr: more matching optimizations from .gitignore
  593cb88 exclude: split basename matching code into a separate function
  b559263 exclude: split pathname matching code into a separate function
  4742d13 attr: avoid searching for basename on every match
  f950eb9 rename pathspec_prefix() to common_prefix() and move to dir.[ch]
  4a085b1 consolidate pathspec_prefix and common_prefix
  d932f4e Rename git_checkattr() to git_check_attr()
  2d72174 Extract a function collect_all_attrs()
  8cf2a84 Add string comparison functions that respect the ignore_case variable.
  407a963 Merge branch 'rr/remote-helper-doc'
  ec775c4 attr: Expand macros immediately when encountered.

But wait, there's more: you can also specify a list of patch files, which means
this can be used for 'git send-emails' --cc-cmd option.


Felipe Contreras (11):
  Add new git-related  helper to contrib
  contrib: related: add option parsing
  contrib: related: add support for multiple patches
  contrib: related: add option to show commits
  contrib: related: add option to parse from committish
  contrib: related: parse committish like format-patch
  contrib: related: fix parsing of rev-list args
  contrib: related: support multiple roles
  contrib: related: sort by participation
  contrib: related: group persons with same email
  contrib: related: add support for mailmap

 contrib/related/git-related | 272 
 1 file changed, 272 insertions(+)
 create mode 100755 contrib/related/git-related

-- 
1.8.2.1

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

[PATCH v4 01/11] Add new git-related helper to contrib

[Felipe Contreras]

This script find people that might be interested in a patch, by going
back through the history for each single hunk modified, and finding
people that reviewed, acknowledge, signed, or authored the code the
patch is modifying.

It does this by running 'git blame' incrementally on each hunk, and then
parsing the commit message. After gathering all the relevant people, it
groups them to show what exactly was their role when the participated in
the development of the relevant commit, and on how many relevant commits
they participated. They are only displayed if they pass a minimum
threshold of participation.

For example:

  % git related 0001-remote-hg-trivial-cleanups.patch
  Felipe Contreras felipe.contre...@gmail.com (author: 100%)
  Jeff King p...@peff.net (signer: 83%)
  Max Horn m...@quendi.de (signer: 16%)
  Junio C Hamano gits...@pobox.com (signer: 16%)

Thus it can be used for 'git send-email' as a cc-cmd.

There might be some other related functions to this script, not just to
be used as a cc-cmd.

Comments-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/related/git-related | 131 
 1 file changed, 131 insertions(+)
 create mode 100755 contrib/related/git-related

diff --git a/contrib/related/git-related b/contrib/related/git-related
new file mode 100755
index 000..2d47efa
--- /dev/null
+++ b/contrib/related/git-related
@@ -0,0 +1,131 @@
+#!/usr/bin/env ruby
+
+$since = '3-years-ago'
+$min_percent = 5
+
+class Commit
+
+  attr_reader :id, :roles
+
+  def initialize(id)
+@id = id
+@roles = []
+  end
+
+  def parse(data)
+author = msg = nil
+roles = {}
+data.each_line do |line|
+  if not msg
+case line
+when /^author ([^]+) (\S+) (.+)$/
+  author = $1, $2
+  roles[author] = :author
+when /^$/
+  msg = true
+end
+  else
+if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^]+) (\S+?)$/
+  person = $2, $3
+  roles[person] = :signer if person != author
+end
+  end
+end
+@roles = roles.map do |person, role|
+  [person, role]
+end
+  end
+
+end
+
+class Commits
+
+  attr_reader :items
+
+  def initialize()
+@items = {}
+  end
+
+  def size
+@items.size
+  end
+
+  def import
+return if @items.empty?
+File.popen(%w[git cat-file --batch], 'r+') do |p|
+  p.write(@items.keys.join(\n))
+  p.close_write
+  p.each do |l|
+if l =~ /^(\h{40}) commit (\d+)/
+  id, len = $1, $2
+  data = p.read($2.to_i)
+  @items[id].parse(data)
+end
+  end
+end
+  end
+
+  def get_blame(source, start, offset, from)
+return unless source and offset
+File.popen(['git', 'blame', '--incremental', '-C',
+   '-L', '%u,+%u' % [start, offset],
+   '--since', $since, from + '^',
+   '--', source]) do |p|
+  p.each do |line|
+if line =~ /^(\h{40})/
+  id = $1
+  @items[id] = Commit.new(id)
+end
+  end
+end
+  end
+
+  def from_patch(file)
+source = nil
+from = nil
+File.open(file) do |f|
+  f.each do |line|
+case line
+when /^From (\h+) (.+)$/
+  from = $1
+when /^---\s+(\S+)/
+  source = $1 != '/dev/null' ? $1[2..-1] : nil
+when /^@@\s-(\d+),(\d+)/
+  get_blame(source, $1, $2, from)
+end
+  end
+end
+  end
+
+end
+
+exit 1 if ARGV.size != 1
+
+commits = Commits.new
+commits.from_patch(ARGV[0])
+commits.import
+
+# hash of hashes
+persons = Hash.new { |hash, key| hash[key] = {} }
+
+commits.items.values.each do |commit|
+  commit.roles.each do |person, role|
+persons[person][role] ||= 0
+persons[person][role] += 1
+  end
+end
+
+persons.each do |person, roles|
+  roles = roles.map do |role, count|
+percent = count.to_f * 100 / commits.size
+next if percent  $min_percent
+'%s: %u%%' % [role, percent]
+  end.compact
+  next if roles.empty?
+
+  name, email = person
+  # must quote chars?
+  name = '%s' % name if name =~ /[^\w \-]/i
+  person = name ? '%s %s' % [name, email] : email
+  puts '%s (%s)' % [person, roles.join(', ')]
+end
-- 
1.8.2.1

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

[PATCH v4 02/11] contrib: related: add option parsing

[Felipe Contreras]

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/related/git-related | 17 +
 1 file changed, 17 insertions(+)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 2d47efa..702836a 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -1,8 +1,25 @@
 #!/usr/bin/env ruby
 
+require 'optparse'
+
 $since = '3-years-ago'
 $min_percent = 5
 
+begin
+  OptionParser.new do |opts|
+opts.program_name = 'git cc-cmd'
+opts.banner = 'usage: git cc-cmd [options] file'
+
+opts.on('-p', '--min-percent N', Integer, 'Minium percentage of role 
participation') do |v|
+  $min_percent = v
+end
+opts.on('-d', '--since DATE', 'How far back to search for relevant 
commits') do |v|
+  $since = v
+end
+  end.parse!
+rescue OptionParser::InvalidOption
+end
+
 class Commit
 
   attr_reader :id, :roles
-- 
1.8.2.1

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

[PATCH v4 03/11] contrib: related: add support for multiple patches

[Felipe Contreras]

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/related/git-related | 34 ++
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 702836a..90ec3aa 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -8,7 +8,7 @@ $min_percent = 5
 begin
   OptionParser.new do |opts|
 opts.program_name = 'git cc-cmd'
-opts.banner = 'usage: git cc-cmd [options] file'
+opts.banner = 'usage: git cc-cmd [options] files'
 
 opts.on('-p', '--min-percent N', Integer, 'Minium percentage of role 
participation') do |v|
   $min_percent = v
@@ -61,6 +61,7 @@ class Commits
 
   def initialize()
 @items = {}
+@main_commits = {}
   end
 
   def size
@@ -91,24 +92,27 @@ class Commits
   p.each do |line|
 if line =~ /^(\h{40})/
   id = $1
-  @items[id] = Commit.new(id)
+  @items[id] = Commit.new(id) if not @main_commits.include?(id)
 end
   end
 end
   end
 
-  def from_patch(file)
+  def from_patches(files)
 source = nil
-from = nil
-File.open(file) do |f|
-  f.each do |line|
-case line
-when /^From (\h+) (.+)$/
-  from = $1
-when /^---\s+(\S+)/
-  source = $1 != '/dev/null' ? $1[2..-1] : nil
-when /^@@\s-(\d+),(\d+)/
-  get_blame(source, $1, $2, from)
+files.each do |file|
+  from = nil
+  File.open(file) do |f|
+f.each do |line|
+  case line
+  when /^From (\h+) (.+)$/
+from = $1
+@main_commits[from] = true
+  when /^---\s+(\S+)/
+source = $1 != '/dev/null' ? $1[2..-1] : nil
+  when /^@@\s-(\d+),(\d+)/
+get_blame(source, $1, $2, from)
+  end
 end
   end
 end
@@ -116,10 +120,8 @@ class Commits
 
 end
 
-exit 1 if ARGV.size != 1
-
 commits = Commits.new
-commits.from_patch(ARGV[0])
+commits.from_patches(ARGV)
 commits.import
 
 # hash of hashes
-- 
1.8.2.1

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

[PATCH v4 04/11] contrib: related: add option to show commits

[Felipe Contreras]

Instead of showing the authors and signers, show the commits themselves.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/related/git-related | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 90ec3aa..6eed4bc 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -4,6 +4,7 @@ require 'optparse'
 
 $since = '3-years-ago'
 $min_percent = 5
+$show_commits = false
 
 begin
   OptionParser.new do |opts|
@@ -16,6 +17,9 @@ begin
 opts.on('-d', '--since DATE', 'How far back to search for relevant 
commits') do |v|
   $since = v
 end
+opts.on('-c', '--commits[=FORMAT]', [:raw, :full], 'List commits instead 
of persons') do |v|
+  $show_commits = v || true
+end
   end.parse!
 rescue OptionParser::InvalidOption
 end
@@ -124,6 +128,20 @@ commits = Commits.new
 commits.from_patches(ARGV)
 commits.import
 
+if $show_commits
+  cmd = nil
+  case $show_commits
+  when :raw
+puts commits.items.keys
+  when :full
+cmd = %w[git log --patch --no-walk]
+  else
+cmd = %w[git log --oneline --no-walk]
+  end
+  system(*cmd + commits.items.keys) if cmd
+  exit 0
+end
+
 # hash of hashes
 persons = Hash.new { |hash, key| hash[key] = {} }
 
-- 
1.8.2.1

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

[PATCH v4 05/11] contrib: related: add option to parse from committish

[Felipe Contreras]

For example master..feature-a.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/related/git-related | 36 ++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 6eed4bc..0015b3c 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -5,11 +5,13 @@ require 'optparse'
 $since = '3-years-ago'
 $min_percent = 5
 $show_commits = false
+$files = []
+$rev_args = []
 
 begin
   OptionParser.new do |opts|
 opts.program_name = 'git cc-cmd'
-opts.banner = 'usage: git cc-cmd [options] files'
+opts.banner = 'usage: git cc-cmd [options] files | rev-list options'
 
 opts.on('-p', '--min-percent N', Integer, 'Minium percentage of role 
participation') do |v|
   $min_percent = v
@@ -122,10 +124,40 @@ class Commits
 end
   end
 
+  def from_rev_args(args)
+return if args.empty?
+source = nil
+File.popen(%w[git rev-list --reverse] + args) do |p|
+  p.each do |e|
+id = e.chomp
+@main_commits[id] = true
+File.popen(%w[git show -C --oneline] + [id]) do |p|
+  p.each do |e|
+case e
+when /^---\s+(\S+)/
+  source = $1 != '/dev/null' ? $1[2..-1] : nil
+when /^@@\s-(\d+),(\d+)/
+  get_blame(source, $1, $2, id)
+end
+  end
+end
+  end
+end
+  end
+
+end
+
+ARGV.each do |e|
+  if File.exists?(e)
+$files  e
+  else
+$rev_args  e
+  end
 end
 
 commits = Commits.new
-commits.from_patches(ARGV)
+commits.from_patches($files)
+commits.from_rev_args($rev_args)
 commits.import
 
 if $show_commits
-- 
1.8.2.1

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

[PATCH v4 06/11] contrib: related: parse committish like format-patch

[Felipe Contreras]

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/related/git-related | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 0015b3c..2f38ee1 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -126,6 +126,20 @@ class Commits
 
   def from_rev_args(args)
 return if args.empty?
+
+revs = []
+
+File.popen(%w[git rev-parse --revs-only --default=HEAD --symbolic] + 
args).each do |rev|
+  revs  rev.chomp
+end
+
+case revs.size
+when 1
+  committish = [ '%s..HEAD' % revs[0] ]
+else
+  committish = revs
+end
+
 source = nil
 File.popen(%w[git rev-list --reverse] + args) do |p|
   p.each do |e|
-- 
1.8.2.1

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

[PATCH v4 07/11] contrib: related: fix parsing of rev-list args

[Felipe Contreras]

For example '-1'.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/related/git-related | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 2f38ee1..e8603be 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -23,7 +23,8 @@ begin
   $show_commits = v || true
 end
   end.parse!
-rescue OptionParser::InvalidOption
+rescue OptionParser::InvalidOption = e
+  $rev_args += e.args
 end
 
 class Commit
@@ -135,9 +136,11 @@ class Commits
 
 case revs.size
 when 1
-  committish = [ '%s..HEAD' % revs[0] ]
+  r = revs[0]
+  r = '^' + r if r[0] != '-'
+  args = [ r, 'HEAD' ]
 else
-  committish = revs
+  args = revs
 end
 
 source = nil
-- 
1.8.2.1

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

[PATCH v4 08/11] contrib: related: support multiple roles

[Felipe Contreras]

Currently only the roles of 'author' and 'signer' and handler, but now
there's also 'reviewer' and 'acker'.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/related/git-related | 29 -
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index e8603be..cf6818e 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -27,6 +27,12 @@ rescue OptionParser::InvalidOption = e
   $rev_args += e.args
 end
 
+KNOWN_ROLES = {
+  'Signed-off' = :signer,
+  'Reviewed' = :reviewer,
+  'Acked' = :acker,
+}
+
 class Commit
 
   attr_reader :id, :roles
@@ -38,25 +44,28 @@ class Commit
 
   def parse(data)
 author = msg = nil
-roles = {}
+# hash of arrays
+roles = Hash.new { |hash, key| hash[key] = [] }
 data.each_line do |line|
   if not msg
 case line
 when /^author ([^]+) (\S+) (.+)$/
   author = $1, $2
-  roles[author] = :author
+  roles[author]  :author
 when /^$/
   msg = true
 end
   else
-if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^]+) (\S+?)$/
+role_regex = KNOWN_ROLES.keys.join('|')
+if line =~ /^(#{role_regex})-by: ([^]+) (\S+?)$/
   person = $2, $3
-  roles[person] = :signer if person != author
+  role = KNOWN_ROLES[$1]
+  roles[person]  role if person != author
 end
   end
 end
-@roles = roles.map do |person, role|
-  [person, role]
+@roles = roles.map do |person, roles|
+  [person, roles]
 end
   end
 
@@ -195,9 +204,11 @@ end
 persons = Hash.new { |hash, key| hash[key] = {} }
 
 commits.items.values.each do |commit|
-  commit.roles.each do |person, role|
-persons[person][role] ||= 0
-persons[person][role] += 1
+  commit.roles.each do |person, roles|
+roles.each do |role|
+  persons[person][role] ||= 0
+  persons[person][role] += 1
+end
   end
 end
 
-- 
1.8.2.1

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

[PATCH v4 09/11] contrib: related: sort by participation

[Felipe Contreras]

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/related/git-related | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index cf6818e..4e9b916 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -212,7 +212,12 @@ commits.items.values.each do |commit|
   end
 end
 
-persons.each do |person, roles|
+# sort by number of participations
+count_sort = lambda do |a, b|
+  b[1].values.reduce(:+) = a[1].values.reduce(:+)
+end
+
+persons.sort(count_sort).each do |person, roles|
   roles = roles.map do |role, count|
 percent = count.to_f * 100 / commits.size
 next if percent  $min_percent
-- 
1.8.2.1

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

[PATCH v4 10/11] contrib: related: group persons with same email

[Felipe Contreras]

We still need the name of the person, so it might make sense to have a
Person object to simplify the code. Later.

Suggested-by: Duy Nguyen pclo...@gmail.com
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/related/git-related | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 4e9b916..f85e924 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -202,12 +202,15 @@ end
 
 # hash of hashes
 persons = Hash.new { |hash, key| hash[key] = {} }
+names = {}
 
 commits.items.values.each do |commit|
   commit.roles.each do |person, roles|
+name, email = person
+names[email] ||= name
 roles.each do |role|
-  persons[person][role] ||= 0
-  persons[person][role] += 1
+  persons[email][role] ||= 0
+  persons[email][role] += 1
 end
   end
 end
@@ -217,7 +220,7 @@ count_sort = lambda do |a, b|
   b[1].values.reduce(:+) = a[1].values.reduce(:+)
 end
 
-persons.sort(count_sort).each do |person, roles|
+persons.sort(count_sort).each do |email, roles|
   roles = roles.map do |role, count|
 percent = count.to_f * 100 / commits.size
 next if percent  $min_percent
@@ -225,7 +228,7 @@ persons.sort(count_sort).each do |person, roles|
   end.compact
   next if roles.empty?
 
-  name, email = person
+  name = names[email]
   # must quote chars?
   name = '%s' % name if name =~ /[^\w \-]/i
   person = name ? '%s %s' % [name, email] : email
-- 
1.8.2.1

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

[PATCH v4 11/11] contrib: related: add support for mailmap

[Felipe Contreras]

This seems to be the way git tools do it.

Suggested-by: Duy Nguyen pclo...@gmail.com
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/related/git-related | 36 
 1 file changed, 36 insertions(+)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index f85e924..be263e2 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -7,6 +7,8 @@ $min_percent = 5
 $show_commits = false
 $files = []
 $rev_args = []
+$mailmaps = {}
+$mailmaps_complex = {}
 
 begin
   OptionParser.new do |opts|
@@ -27,6 +29,39 @@ rescue OptionParser::InvalidOption = e
   $rev_args += e.args
 end
 
+def get_mailmap(filename)
+  return unless File.exists?(filename)
+  File.open(filename) do |f|
+f.each do |line|
+  case line.gsub(/\s*#.*$/, '')
+  when /^([^]+)\s+(\S+)$/
+$mailmaps[$2] = [ $1, nil ]
+  when /^(\S+)\s+(\S+)$/
+$mailmaps[$2] = [ nil, $1 ]
+  when /^([^]+)\s+(\S+)\s+(\S+)$/
+$mailmaps[$3] = [ $1, $2 ]
+  when /^([^]+)\s+(\S+)\s+([^]+)\s+(\S+)$/
+$mailmaps_complex[[$3, $4]] = [ $1, $2 ]
+  end
+end
+  end
+end
+
+def mailmap_fix(person)
+  new = nil
+  name, email = person
+  new = $mailmaps_complex[person] if not new and 
$mailmaps_complex.include?(person)
+  new = $mailmaps[email] if not new and $mailmaps.include?(email)
+  return if not new
+  person[0] = new[0] if new[0]
+  person[1] = new[1] if new[1]
+end
+
+get_aliases if $get_aliases
+get_mailmap('.mailmap')
+mailmap_file = %x[git config mailmap.file].chomp
+get_mailmap(mailmap_file)
+
 KNOWN_ROLES = {
   'Signed-off' = :signer,
   'Reviewed' = :reviewer,
@@ -65,6 +100,7 @@ class Commit
   end
 end
 @roles = roles.map do |person, roles|
+  mailmap_fix(person)
   [person, roles]
 end
   end
-- 
1.8.2.1

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

Re: [PATCH] git-remote-testgit: avoid process substitution

[Junio C Hamano]

Felipe Contreras felipe.contre...@gmail.com writes:

 On Thu, Apr 25, 2013 at 1:25 PM, Junio C Hamano gits...@pobox.com wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 ...
 + git for-each-ref --format='%(refname) %(objectname)' |
 + while read ref a
   do
 - test $a == $b  continue
 + case $before in
 + *$ref $a*)
 + continue

 I wonder if we should bother with this at all. The purpose of the code
 was mainly to show to users that they should report the success only
 if the refs have been updated, but the code is becoming more
 obfuscated, a comment should do the trick. And then, we can just
 report success for all the refs (and explain in the comment why).

 Are you proposing to say ok $ref to everything we see in the
 resulting repository, even the ones the caller of remote-testgit did
 not ask us to do anything with?

 Wouldn't the caller be surprised if we did so?

 Why would it?  The only effective difference is what you'll see
 reported in the UI, but there's no user here.

You are correct that it affects only the UI, but isn't that because
the current implementation of push_update_refs_status() blindly
accepts whatever 'ok' response says without checking the ref
mentioned against what it tried to push out?  I was wondering if
that codepath should stay that way forever, or if we may want add
sanity checks later.  If the latter, I suspect this would manifest
as an ancient bug to say 'ok' for everything we have instead of what
we actually pushed out (of course, the explanation in the comment
would help).

So I am OK with that simpler approach.  Care to roll the conclusion
of your idea into a patch?

By the way, I noticed that Documentation/gitremote-helpers.txt does
not even mention these 'ok' responses for 'export' command, but they
should be the same as responses to 'push'. Perhaps we can share some
text between the two?
--
To unsubscribe from this list: send the line unsubscribe 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] Hold an 'unsigned long' chunk of the sha1 in obj_hash

[Junio C Hamano]

Thomas Rast tr...@inf.ethz.ch writes:

 So we take a slightly different approach, and trade some memory for
 better cache locality.

Interesting.  It feels somewhat bait-and-switch to reveal that the
above some turns out to be double later, but the resulting code
does not look too bad, and the numbers do not look insignificant.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Thomas Rast]

Felipe Contreras felipe.contre...@gmail.com writes:

 But I do not care that much really. The patch is good either way, if
 you don't like it, you go ahead and fix it, because I won't. I have
 174 remote-helper related patches in my queue, and nobody benefits
 from rambling about a one liner that is obviously correct, not you,
 not me, not the users, not the developers.

You don't stick to the rules of this project, which have been pointed
out already:

  The body should provide a meaningful commit message, which:

. explains the problem the change tries to solve, iow, what is wrong
  with the current code without the change.

. justifies the way the change solves the problem, iow, why the
  result with the change is better.

. alternate solutions considered but discarded, if any.

Your project is moving too fast to put up with the established
procedures in this community.

In fact you are pretty much holding us hostage with a take it or keep
it broken while causing more work attitude:

 Junio of course might disagree and drop this patch, but then he would
 need to deal with the fallout of possible conflicts.

You did not respond well to reviews and criticism.  Even the
constructive fine-let's-do-the-work-for-him kind that Peff offered.

And on top of that, remote helpers are written against an interface that
was designed to allow working with external programs.

So why is this in git.git?

Why should we take any more contrib additions from you?

-- 
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] git-remote-testgit: avoid process substitution

[Felipe Contreras]

On Thu, Apr 25, 2013 at 3:06 PM, Junio C Hamano gits...@pobox.com wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 On Thu, Apr 25, 2013 at 1:25 PM, Junio C Hamano gits...@pobox.com wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 ...
 + git for-each-ref --format='%(refname) %(objectname)' |
 + while read ref a
   do
 - test $a == $b  continue
 + case $before in
 + *$ref $a*)
 + continue

 I wonder if we should bother with this at all. The purpose of the code
 was mainly to show to users that they should report the success only
 if the refs have been updated, but the code is becoming more
 obfuscated, a comment should do the trick. And then, we can just
 report success for all the refs (and explain in the comment why).

 Are you proposing to say ok $ref to everything we see in the
 resulting repository, even the ones the caller of remote-testgit did
 not ask us to do anything with?

 Wouldn't the caller be surprised if we did so?

 Why would it?  The only effective difference is what you'll see
 reported in the UI, but there's no user here.

 You are correct that it affects only the UI, but isn't that because
 the current implementation of push_update_refs_status() blindly
 accepts whatever 'ok' response says without checking the ref
 mentioned against what it tried to push out?  I was wondering if
 that codepath should stay that way forever, or if we may want add
 sanity checks later.  If the latter, I suspect this would manifest
 as an ancient bug to say 'ok' for everything we have instead of what
 we actually pushed out (of course, the explanation in the comment
 would help).

Actually, I think the code already checks for that.

 So I am OK with that simpler approach.  Care to roll the conclusion
 of your idea into a patch?

Will do.

 By the way, I noticed that Documentation/gitremote-helpers.txt does
 not even mention these 'ok' responses for 'export' command, but they
 should be the same as responses to 'push'. Perhaps we can share some
 text between the two?

Indeed, it's the same code for both.

-- 
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 1/9] remote-bzr: trivial cleanups

[Junio C Hamano]

Felipe Contreras felipe.contre...@gmail.com writes:

 But I do not care that much really. The patch is good either way, if
 you don't like it, you go ahead and fix it, because I won't. I have
 174 remote-helper related patches in my queue, and nobody benefits
 from rambling about a one liner that is obviously correct, not you,
 not me, not the users, not the developers.

Three random points.

 * For this particular patch [1/9], especially because this would
   land close to the corresponding remote-hg fixes (e.g. has_key is
   deprecated), I think it is sufficient to say port fixes from
   corresponding remote-hg patches (you said it in 0/9 and didn't
   say it in 1/9, though) without going into individual details.
   Anybody who wonders what these changes were about will have a
   clue to check contemporary patches to remote-hg that way.

 * You may want to hold onto those 174 patches and polish their
   explanation up to save the list audiences' time by avoiding this
   kind of useless why no explanation exchanges.

 * If you do not want to keep a readable history, it would mean that
   nobody but you will fix problems discovered in the future in
   remote-hg, and there is no point carrying it in my tree for other
   Git developers to look at it.  The users are better off getting
   them from your tree and that will make it clear for them whom to
   ask help/fix for when they hit a snag.

 Junio of course might disagree and drop this patch, but then he would
 need to deal with the fallout of possible conflicts.

A much more sensible thing in such a case for me to do actually is
to drop the whole thing. I do not want to do that unless necessary.

 ... I think the less-than-perfect commit messages in a
 *contrib* script that is extremely recent is a small price to pay for
 having nice and workable bzr and mercurial remote-helpers as soon as
 possible

I do not share this view at all. The users survived without it long
enough; they can wait for a well maintained version.  On the other
hand, shipping something that will not be maintainable is not the
way to help end users. It is being irresponsive to them.

Helping other developers understand your code is a way to ensure
that your code that would help users will be kept maintained.  I do
not agree with Ram at all when he says that developers are more
important than users, and I agree with you that the project exists
for users, and not for developers.  But you need to help your fellow
developers anyway by spending effort to keep your history readable,
in order to help them help the users.

Do not take the users matter mantra to the extreme. You need other
developers to put users first.

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

Re: [PATCH] check-non-portable-shell: Check for non portable grep usage

[Junio C Hamano]

Torsten Bögershausen tbo...@web.de writes:

 The make test-lint could check for some common non portable
 grep usage:

 The + as a qualifier is not a basic regular expression.
 Some grep understand \+, but in general egrep should be used.

 The \t can not be used to find an ASCII TAB, neither
 can \s be used to find a whitespace

 Signed-off-by: Torsten Bögershausen tbo...@web.de
 ---
  t/check-non-portable-shell.pl | 2 ++
  1 file changed, 2 insertions(+)

 diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
 index ff87611..58f8e2c 100755
 --- a/t/check-non-portable-shell.pl
 +++ b/t/check-non-portable-shell.pl
 @@ -16,6 +16,8 @@ sub err {
  
  while () {
   chomp;
 + / e?grep\s+\[^]*\\([st])/ and err grep \\$1 is not portable);

grep -e foo\tbar is exempt?  Indenting with a single tab,
immediately followed by grep or egrep without SP in front, is
exempt?

 + / [^e]grep.*\\\+/ and err 'grep with \+ is not portable (please use 
 egrep)';

Not even checking if the it is inside the pattern of grep?

As I said number of times, I do not think it is a workable approach
to textually match patterns in a script that does not understand
even the basic shell syntax.

   /^\s*sed\s+-i/ and err 'sed -i is not portable';
   /^\s*echo\s+-(\S)\s+/ and err echo -$1 is not portable (please use 
 printf);
   /^\s*declare\s+/ and err 'arrays/declare not portable';
--
To unsubscribe from this list: send the line unsubscribe 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] t9501: Do not use export X=Y

[Junio C Hamano]

Torsten Bögershausen tbo...@web.de writes:

 Spilt lines like export X=Y into 2 lines:
 X=Y
 export X

That can be read from the patch text.

If you are going to spend three lines, please describe why it has to
be split; that would help educate developers new to the codebase.

Thanks.

 Signed-off-by: Torsten Bögershausen tbo...@web.de
 ---
  t/t9501-gitweb-standalone-http-status.sh | 15 ++-
  1 file changed, 10 insertions(+), 5 deletions(-)

 diff --git a/t/t9501-gitweb-standalone-http-status.sh 
 b/t/t9501-gitweb-standalone-http-status.sh
 index ef86948..d3a5bac 100755
 --- a/t/t9501-gitweb-standalone-http-status.sh
 +++ b/t/t9501-gitweb-standalone-http-status.sh
 @@ -130,7 +130,8 @@ test_expect_success DATE_PARSER 'modification: feed 
 last-modified' '
  test_debug 'cat gitweb.headers'
  
  test_expect_success DATE_PARSER 'modification: feed if-modified-since 
 (modified)' '
 - export HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
 + HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
 + export HTTP_IF_MODIFIED_SINCE 
   test_when_finished unset HTTP_IF_MODIFIED_SINCE 
   gitweb_run p=.git;a=atom;h=master 
   grep Status: 200 OK gitweb.headers
 @@ -138,7 +139,8 @@ test_expect_success DATE_PARSER 'modification: feed 
 if-modified-since (modified)
  test_debug 'cat gitweb.headers'
  
  test_expect_success DATE_PARSER 'modification: feed if-modified-since 
 (unmodified)' '
 - export HTTP_IF_MODIFIED_SINCE=Thu, 7 Apr 2005 22:14:13 + 
 + HTTP_IF_MODIFIED_SINCE=Thu, 7 Apr 2005 22:14:13 + 
 + export HTTP_IF_MODIFIED_SINCE 
   test_when_finished unset HTTP_IF_MODIFIED_SINCE 
   gitweb_run p=.git;a=atom;h=master 
   grep Status: 304 Not Modified gitweb.headers
 @@ -153,7 +155,8 @@ test_expect_success DATE_PARSER 'modification: snapshot 
 last-modified' '
  test_debug 'cat gitweb.headers'
  
  test_expect_success DATE_PARSER 'modification: snapshot if-modified-since 
 (modified)' '
 - export HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
 + HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
 + export HTTP_IF_MODIFIED_SINCE 
   test_when_finished unset HTTP_IF_MODIFIED_SINCE 
   gitweb_run p=.git;a=snapshot;h=master;sf=tgz 
   grep Status: 200 OK gitweb.headers
 @@ -161,7 +164,8 @@ test_expect_success DATE_PARSER 'modification: snapshot 
 if-modified-since (modif
  test_debug 'cat gitweb.headers'
  
  test_expect_success DATE_PARSER 'modification: snapshot if-modified-since 
 (unmodified)' '
 - export HTTP_IF_MODIFIED_SINCE=Thu, 7 Apr 2005 22:14:13 + 
 + HTTP_IF_MODIFIED_SINCE=Thu, 7 Apr 2005 22:14:13 + 
 + export HTTP_IF_MODIFIED_SINCE 
   test_when_finished unset HTTP_IF_MODIFIED_SINCE 
   gitweb_run p=.git;a=snapshot;h=master;sf=tgz 
   grep Status: 304 Not Modified gitweb.headers
 @@ -170,7 +174,8 @@ test_debug 'cat gitweb.headers'
  
  test_expect_success DATE_PARSER 'modification: tree snapshot' '
   ID=`git rev-parse --verify HEAD^{tree}` 
 - export HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
 + HTTP_IF_MODIFIED_SINCE=Wed, 6 Apr 2005 22:14:13 + 
 + export HTTP_IF_MODIFIED_SINCE 
   test_when_finished unset HTTP_IF_MODIFIED_SINCE 
   gitweb_run p=.git;a=snapshot;h=$ID;sf=tgz 
   grep Status: 200 OK gitweb.headers 
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Thu, Apr 25, 2013 at 3:30 PM, Thomas Rast tr...@inf.ethz.ch wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 But I do not care that much really. The patch is good either way, if
 you don't like it, you go ahead and fix it, because I won't. I have
 174 remote-helper related patches in my queue, and nobody benefits
 from rambling about a one liner that is obviously correct, not you,
 not me, not the users, not the developers.

 You don't stick to the rules of this project, which have been pointed
 out already:

The rules of the contrib area are different from the ones of the rest
of the project.

 Your project is moving too fast to put up with the established
 procedures in this community.

That's one of the reasons it's in the contrib area.

 In fact you are pretty much holding us hostage with a take it or keep
 it broken while causing more work attitude:

I'm the maintainer of this code, so it's my call. If Junio has a
problem with that, I would gladly take my code somewhere else. I doubt
that's in the best interest of anyone.

But if the problem is this particular patch (reaally?), Junio could
just drop this particular patch. Are you seriously suggesting that the
whole contrib/remote-helpers should be dropped because this patch
introduces a one-liner fix without mentioning it in the commit
message? Really? I haven't seen anybody complain about *any* of the
other patches where I held the project hostage and refused to fix
the commit message or change the patch.

Other than this instance, show me where exactly did I do that.

 Junio of course might disagree and drop this patch, but then he would
 need to deal with the fallout of possible conflicts.

 You did not respond well to reviews and criticism.  Even the
 constructive fine-let's-do-the-work-for-him kind that Peff offered.

Define respond well. If your idea to respond well is to say Yes
sir! to every criticism, then no, I didn't. OTOH, if it's to reply
and address the issues with objective reasoning and an open mind, I
did.

I don't understand this notion that every review criticism is valid
and correct. They are not, and it's OK to point that out.. really. If
they turn to be valid and correct, the reviewer can surely
counter-argue and substantiate his/her claims.

And I don't recall Peff ever doing this constructive
fine-let's-do-the-work-for-him on any contrib/remote-helpers stuff.

 So why is this in git.git?

 Why should we take any more contrib additions from you?

Because it's good for the users.

If you are seriously suggesting to drop contrib/remote-helpers, I
suggest that 1) don't do it in the review thread of a trivial patch 2)
start a new thread where you point multiple instances where the
maintainer of the code (me) failed to respond correctly to criticism
(of remote-helpers's code), 3) show how this affects negatively the
project, and 4) ask for new maintainers if the job of the current one
is not deemed up-to-par, and only if no maintainer steps up, drop the
code.

Cheers.

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

Re: [PATCH] Hold an 'unsigned long' chunk of the sha1 in obj_hash

[Thomas Rast]

Junio C Hamano gits...@pobox.com writes:

 Thomas Rast tr...@inf.ethz.ch writes:

 So we take a slightly different approach, and trade some memory for
 better cache locality.

 Interesting.  It feels somewhat bait-and-switch to reveal that the
 above some turns out to be double later, but the resulting code
 does not look too bad, and the numbers do not look insignificant.

Oh, that wasn't the intent.  I was too lazy to gather some memory
numbers, so here's an estimate on the local effect and some measurements
on the global one.

struct object is at least 24 bytes (flags etc. and sha1).  We grow the
hash by 2x whenever it reaches 50% load, so it is always at least 25%
loaded.

A 25% loaded hash-table used to consist of 75% pointers (8 bytes) and
25% pointers-to-struct-object (32 bytes), for 14 bytes per average slot.
Now it's 22 bytes (one more unsigned long) per slot, i.e., a 60%
increase for the data managed by the hash table.

But that's using the crudest estimates I could think of.  If we assume
that an average blob and tree is at least as big as the smallest
possible commit, we'd guess that objects are at least ~240 bytes (this
is still somewhat of an estimate and assumes that you don't go and
handcraft commits with single-digit timestamps).  So the numbers above
go up by 25% * 240 per average slot, and work out to an about 11%
overall increase.

Here are some real numbers from /usr/bin/time git rev-list --all --objects:

before:

  2.30user 0.02system 0:02.33elapsed 99%CPU (0avgtext+0avgdata 
247760maxresident)k
  0inputs+0outputs (0major+17844minor)pagefaults 0swaps

after:

  2.18user 0.02system 0:02.21elapsed 99%CPU (0avgtext+0avgdata 
261936maxresident)k   
  0inputs+0outputs (0major+18202minor)pagefaults 0swaps

So that would be about 14MB or 5.7% of extra memory.

-- 
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] remote-bzr: use proper push method

[Junio C Hamano]

Thomas Rast tr...@inf.ethz.ch writes:

 Felipe Contreras felipe.contre...@gmail.com writes:

 Not just randomly synchronize the revisions with no checks at all. This
 is the way bazaar's UI does it.

 Also, add a non-ff check.

 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 ---

 This patch should probably go to maint, as the results of pushing the way we
 currently push are not really understood. Perhaps it's similar to a 'git push
  --force', or perhaps it can potentially screw the repository.

 It's better to be safe and just do what bazaar does.

 Other than this patch should probably go to maint, this should be in
 the commit message.

Hmph, should it?

I do not quite understand what ... are not really understood.
Perhaps... wants to say.  Understood by whom?  By the author of the
patch?  By the author of the original code?

The log would end up saying Doing the same as bazaar should be the
right thing to do(TM), but don't ask me why.  I do not know what I
am doing, or why checking is better than not checking, but it seems
to work.  That _could_ be the truth, but it won't help people who
are reading the code later, will it?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Hold an 'unsigned long' chunk of the sha1 in obj_hash

[Duy Nguyen]

On Fri, Apr 26, 2013 at 1:04 AM, Thomas Rast tr...@inf.ethz.ch wrote:
 So we take a slightly different approach, and trade some memory for
 better cache locality.  Namely, we change the hash table slots to
 contain

   struct object *obj;
   unsigned long sha1prefix;

 We use this new 'sha1prefix' field to store the first part of the
 object's sha1, from which its hash table slot is computed.

Clever. I went a similar road before. But I put the whole 20-byte
sha-1 in obj_hash, which makes obj_hash even bigger and less
cache-friendly.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html