* [RFC/PATCH] git-new-workdir: add option --rsync
From: Michael Schubert @ 2012-02-05 16:27 UTC (permalink / raw)
To: git
Currently, git-new-workdir doesn't allow you to rather duplicate your
old workdir instead of creating a new one based on a branch. This can be
annoying when you are used to carry untracked helper scripts in your
workdir (e.g. test.sh).
Add a new option -s | --rsync to allow users to "sync" the new workdir.
Signed-off-by: Michael Schubert <mschub@elegosoft.com>
---
Maybe we even want to stop after rsync'ing without doing the checkout -f.
Opinions?
contrib/workdir/git-new-workdir | 31 +++++++++++++++++++++++++++++--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/contrib/workdir/git-new-workdir b/contrib/workdir/git-new-workdir
index 75e8b25..bac75f9 100755
--- a/contrib/workdir/git-new-workdir
+++ b/contrib/workdir/git-new-workdir
@@ -10,9 +10,30 @@ die () {
exit 128
}
-if test $# -lt 2 || test $# -gt 3
+if test $# -lt 2
then
- usage "$0 <repository> <new_workdir> [<branch>]"
+ usage "$0 [-s | --rsync] <repository> <new_workdir> [<branch>]"
+fi
+
+rsync=
+
+while test $# != 0
+do
+ case "$1" in
+ -s|--rsync)
+ rsync=t ;;
+ *)
+ break ;;
+ esac
+ shift
+done
+
+if test -n "$rsync"
+then
+ if ! $(hash rsync 2>/dev/null)
+ then
+ die "cannot find rsync"
+ fi
fi
orig_git=$1
@@ -77,6 +98,12 @@ done
cd "$new_workdir"
# copy the HEAD from the original repository as a default branch
cp "$git_dir/HEAD" .git/HEAD
+
+if test -n "$rsync"
+then
+ rsync --archive --exclude '.*' "../$orig_git/" .
+fi
+
# checkout the branch (either the same as HEAD from the original repository, or
# the one that was asked for)
git checkout -f $branch
--
1.7.9.230.gbd302
^ permalink raw reply related
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Johan Herland @ 2012-02-05 17:29 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Philip Oakley, git
In-Reply-To: <201202051715.38896.jnareb@gmail.com>
2012/2/5 Jakub Narebski <jnareb@gmail.com>:
>> Being able to mark temporary, out of sequence or other hacks as Secret could
>> be useful, as would recording where Public commits had been sent.
>
> Marking as 'secret' must I think be explicit, but I think 'public' phase
> should be inferred from remote-tracking branches. The idea of phases is
> to allow UI to ask about status of commits: can we amend / rebase it or
> not, can we push it or not.
I agree that the 'public' state should (by default) be automatically
inferred from remote-tracking branches. As it stands, we can do this
with current git, by writing a pre-rebase hook that checks if any of
the commits to-be-rebased are reachable from any remote-tracking
branch.
Unfortunately, the pre-rebase hook only affects 'git rebase', and in
order to do the same check on 'git commit --amend' you'd have to write
a similar pre-commit hook (don't know how easy it is to find the
amended commit from within the hook). Maybe we should add a
pre-rewrite hook that trigger in the same situations as the
post-rewrite hook.
This should take care of the simplest 'public' use case in a
push-based workflow. If you publish commits by other means
(send-email, bundles, pulling directly from your repo), you need some
other way to mark the 'public' commits. One solution would be using
'git notes' to annotate the 'public' commits on a 'refs/notes/public'
notes ref. Your pre-rebase/pre-rewrite hook should then check if any
of the commits to-be-rewritten are reachable from any commit annotated
as 'public'.
Also, if you want to record where 'public' commits have been sent
(other than what can be inferred from the remote-tracking branches),
you could write this into the refs/notes/public annotation.
As for 'secret' commits, you could annotate these on a
refs/notes/secret notes ref, and then teach 'git push' (or whatever
other method for publishing commits you use) to refuse to publish
commits annotated on this notes ref. Possibly we would want to add a
"pre-push" or "pre-publish" hook.
Have fun! :)
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH] send-email: add extra safetly in address sanitazion
From: Thomas Rast @ 2012-02-05 19:39 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Brandon Casey, Uwe Kleine-König, Brian Gernhardt,
Robin H. Johnson, Ævar Arnfjörð Bjarmason
In-Reply-To: <1328373162-25188-1-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> Currently bad addresses like 'Foo Bar <foo@bar.com>>' will just be sent
> verbatim -- that's not good; we should either error out, or sanitize
> them.
>
> The following patch adds extra sanitazion so the following
> transformations are performed:
>
> 'Foo Bar <foo@bar.com>' -> 'Foo Bar <foo@bar.com>'
> '"Foo Bar" <foo@bar.com>' -> '"Foo Bar" <foo@bar.com>'
> 'foo@bar.com' -> 'foo@bar.com'
> '<foo@bar.com>' -> 'foo@bar.com'
> 'Foo Bar' -> 'Foo Bar'
Am I the only one who stared at this for ten seconds, only to then
realize that there is no sanitizing whatsoever going on here?
> 'Foo Bar <foo@bar.com>>' -> 'Foo Bar <foo@bar.com>'
> '"Foo Bar" <foo@bar.com>>' -> '"Foo Bar" <foo@bar.com>'
> '<foo@bar.com>>' -> 'foo@bar.com'
All of these are the same underlying issue. Does your patch fix any
other malformed addresses, or just this particular type?
> Basically, we try to check that the address is in the form of
> "Name <email>", and if not, assume it's "email". According to commit
> 155197e[1], the "prhase" should not be empty, so if it is, remove the
^^^^^^
"phrase"
> git-send-email.perl | 14 ++++++++++----
> 1 files changed, 10 insertions(+), 4 deletions(-)
Tests?
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 1/3] blame: fix email output with mailmap
From: Thomas Rast @ 2012-02-05 19:57 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git
In-Reply-To: <1328385024-6955-2-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>, "Brian Gianforcaro" <b.gianfo@gmail.com>, "Marius Storm-Olsen" <marius@trolltech.com>, "Junio C Hamano" <junkio@cox.net>
I hope you noticed that Cc lists like the above really prove my point
that you cannot automate common sense. Your cccmd script apparently
uses the line range in
> diff --git a/builtin/blame.c b/builtin/blame.c
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1403,10 +1403,13 @@
to determine who to Cc. But in doing so, it dug up an old address for
Junio. It also added Brian whose only fault in all of this was fixing
style of the 'if' you are patching.
And on top of that it had absolutely no way of knowing that Cc'ing Peff
would have been a good idea, seeing as you two were involved in an
earlier discussion about this precise bug.
(Granted, omitting *Peff* doesn't make that much of a difference, since
for all I know he reads every email that crosses this list. But my
point still stands.)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [bug] blame duplicates trailing ">" in mailmapped emails
From: Junio C Hamano @ 2012-02-05 20:16 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, Jonathan Nieder, git, SZEDER Gábor
In-Reply-To: <20120204182611.GA31091@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sat, Feb 04, 2012 at 05:46:05PM +0200, Felipe Contreras wrote:
>
>> In any case, the one to blame for the header corruption is git:
>> [...]
>> f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)
>>
>> Notice the mail is wrong.
> ...
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 5a67c20..9b886fa 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1406,7 +1406,8 @@ static void get_ac_line(const char *inbuf, const char *what,
> /* Add a trailing '>' to email, since map_user returns plain emails
> Note: It already has '<', since we replace from mail+1 */
> mailpos = memchr(mail, '\0', mail_len);
> - if (mailpos && mailpos-mail < mail_len - 1) {
> + if (mailpos && mailpos-mail < mail_len - 1 &&
> + mailpos > mail && *(mailpos-1) != '>') {
> *mailpos = '>';
> *(mailpos+1) = '\0';
> }
>
> but it feels like the fix should go into map_user.
Thanks.
The map_user() API takes an email address that is terminated by either NUL
or '>' to allow the caller to learn the corresponding up-to-date email
address that is NUL terminated, while indicating with its return value
that if the caller even needs to replace what it already has. But the
function does not properly terminate email when it only touched the name
part. And I think that is the real bug.
So I agree that the real fix should go to map_user() so that when it says
"I've updated something, so pick up the updated result from the i/o
arguments you gave me, i.e. email and name", it makes sure what it claims
to be an e-mail address does not have the extra '>' in it.
Working around the current behaviour by forcing all callers that pass '>'
terminated e-mail address to have the code like the above quoted patch
does not feel right.
^ permalink raw reply
* Re: Specifying revisions in the future
From: Matthieu Moy @ 2012-02-05 20:18 UTC (permalink / raw)
To: jpaugh; +Cc: git
In-Reply-To: <jgjkk0$qrg$1@dough.gmane.org>
jpaugh@gmx.us writes:
> Hello.
>
> Is it possible to specify revisions in the future?
You mean, the opposite of <commit>^ or <commit>~n?
AFAIK, there isn't, and there's a good reason for that: <commit>^ is
well-defined, it's the first parent of <commit>, and it won't change
unless one rewrites this commit.
"the successor of <commit>", OTOH, is not well defined, since there can
be several successors, and one can't order them reliably (you can't
really know the set of successors, because they can exist in different
repositories).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 2/3] t: mailmap: add 'git blame -e' tests
From: Junio C Hamano @ 2012-02-05 20:38 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Jonathan Nieder, git, Marius Storm-Olsen
In-Reply-To: <CAMP44s2djXurzMSXLOAkx84Sm8P2YV67M1yS2AuidkfGbTdmEQ@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
>>> Look at the title:
>>> add 'git blame -e' tests
>>>
>>> s/blame/blame -e/
>>
>> And? After copy/pasting this particular test with that substitution,
>> what do we get a test for?
>
> For 'git blame -e'.
>
>> What class of problem is it supposed to catch?
>
> Problems related to 'git blame -e'?
You very well know that we know you better than that, so it is no use to
pretend to be dumb. It does not do anything other than wasting bandwidth
and irritating readers.
You know that you are not addressing "git blame with the -e option shows
wrong line numbers on its output". The symptom you are checking with is
"e-mail address output from 'blame -e' used to add an extra '>' at the end
when only name is mapped, and I fixed it with the previous patch."
Why is it so hard to either
(1) give the more descriptive answer upfront when somebody who did not
read the first patch of the 3-patch series pointed it out the comment
is not descriptive enough the first time; or
(2) give the more descriptive answer and then say "we could do that, but
when somebody views this change in "git log" as a part of 3-patch
series, it should be clear enough---so let's aim for brevity instead
of adding that two-line description" to defend the line in the patch?
> Or just apply it. Don't let the perfect be the enemy of the good.
Perfect is the enemy of the good, but it is not an excuse to be sloppy.
I tend to think that a single line "# blame -e" is sufficient if this were
a part of just a single patch that has the fix and the test to guard the
fix against future breakage (i.e. "not sloppy"). I would even say that not
even "# blame" is necessary in such a case.
But if this is a standalone patch to add this test, it does not describe
what it wants to test very well.
In any case, I have doubts that the fix should go to blame and not to
map_user(), so I'll see what happens in the further discussions.
Thanks.
^ permalink raw reply
* Re: [PATCH] Change include order in two compat/ files to avoid compiler warning
From: Junio C Hamano @ 2012-02-05 20:41 UTC (permalink / raw)
To: Ben Walton; +Cc: git
In-Reply-To: <1328404107-15757-1-git-send-email-bwalton@artsci.utoronto.ca>
Ben Walton <bwalton@artsci.utoronto.ca> writes:
> The inet_ntop and inet_pton compatibility wrapper source files
> included system headers before git-compat-utils.h.
Thanks, that is definitely a breakage.
> diff --git a/compat/inet_ntop.c b/compat/inet_ntop.c
> index 60b5a1d..f1bf81c 100644
> --- a/compat/inet_ntop.c
> +++ b/compat/inet_ntop.c
> @@ -15,11 +15,9 @@
> * SOFTWARE.
> */
>
> +#include "../git-compat-util.h"
> #include <errno.h>
> #include <sys/types.h>
> -
> -#include "../git-compat-util.h"
> -
> #include <stdio.h>
> #include <string.h>
I actually have to wonder if any of these four inclusion of the system headers
are warranted. Wouldn't they be included as part of git-compat-util.h anyway?
^ permalink raw reply
* Re: [PATCH v4 4/4] completion: simplify __gitcomp*
From: Junio C Hamano @ 2012-02-05 20:45 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Felipe Contreras, git, Jonathan Nieder, Thomas Rast,
Shawn O. Pearce
In-Reply-To: <20120204135404.GF16099@goldbirke>
SZEDER Gábor <szeder@ira.uka.de> writes:
> And it does make a difference, it breaks the completion of a single
> word in multiple steps, e.g. git log --pretty=<TAB> master..<TAB>. In
> such cases we pass "${cur##--pretty=}" and "${cur_#*..}" as third
> argument to __gitcomp() and __gitcomp_nl(), which can be empty strings
> when the user hits TAB right after the '=' and '..'.
After saying "this rewrite is wrong", I was actually wondering if I should
have said "this rewrite is not faithful to the original". Based on your
analysis, the difference does break the callers, so the rewrite is indeed
wrong.
Thanks for following up.
^ permalink raw reply
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Jakub Narebski @ 2012-02-05 20:46 UTC (permalink / raw)
To: Johan Herland; +Cc: Philip Oakley, git
In-Reply-To: <CALKQrgcPW5VnVtGYDo6i00bvmWr6PvnWfEdWW+9ttG4hVQm58A@mail.gmail.com>
On Sun, 5 Feb 2012, Johan Herland wrote:
> 2012/2/5 Jakub Narebski <jnareb@gmail.com>:
> > > Being able to mark temporary, out of sequence or other hacks as Secret could
> > > be useful, as would recording where Public commits had been sent.
> >
> > Marking as 'secret' must I think be explicit, but I think 'public' phase
> > should be inferred from remote-tracking branches. The idea of phases is
> > to allow UI to ask about status of commits: can we amend / rebase it or
> > not, can we push it or not.
>
> I agree that the 'public' state should (by default) be automatically
> inferred from remote-tracking branches. As it stands, we can do this
> with current git, by writing a pre-rebase hook that checks if any of
> the commits to-be-rebased are reachable from any remote-tracking
> branch.
It is nice that we can achieve a large part of this feature with existing
infrastructure. It would be nice if we ship such pre-rebase hook with
git, so people can just enable it if they want to use this functionality,
like the default pre-commit hook that checks for whitespace errors.
> Unfortunately, the pre-rebase hook only affects 'git rebase', and in
> order to do the same check on 'git commit --amend' you'd have to write
> a similar pre-commit hook (don't know how easy it is to find the
> amended commit from within the hook). Maybe we should add a
> pre-rewrite hook that trigger in the same situations as the
> post-rewrite hook.
pre-rewrite hook would be a really nice to have, especially that it would
(I hope) cover third party tools like various GUIs for git; and also
git-filter-branch.
Note however that the safety net, i.e. refusing or warning against attempted
rewrite of published history is only part of issue. Another important part
is querying and showing "phase" of a commit. What I'd like to see is
ability to show among others in "git log" and "git show" output if commit
was already published or not (and if it is marked 'secret').
> This should take care of the simplest 'public' use case in a
> push-based workflow. If you publish commits by other means
> (send-email, bundles, pulling directly from your repo), you need some
> other way to mark the 'public' commits. One solution would be using
> 'git notes' to annotate the 'public' commits on a 'refs/notes/public'
> notes ref. Your pre-rebase/pre-rewrite hook should then check if any
> of the commits to-be-rewritten are reachable from any commit annotated
> as 'public'.
Another solution would be to create "fake" remote-tracking branches
by git-bundle and git-send-email.
> Also, if you want to record where 'public' commits have been sent
> (other than what can be inferred from the remote-tracking branches),
> you could write this into the refs/notes/public annotation.
I wonder if this too can be done by hook...
> As for 'secret' commits, you could annotate these on a
> refs/notes/secret notes ref, and then teach 'git push' (or whatever
> other method for publishing commits you use) to refuse to publish
> commits annotated on this notes ref. Possibly we would want to add a
> "pre-push" or "pre-publish" hook.
Well, addition of pre-push / pre-publish was resisted on the grounds
that all it does is something that can be as easy done by hand before
push. Perhaps this new use case would help bring it forward, don't
you think?
Thanks for all the comments.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH v3 1/4] completion: be nicer with zsh
From: Junio C Hamano @ 2012-02-05 20:49 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Jonathan Nieder, git, SZEDER Gábor
In-Reply-To: <CAMP44s2QdJ4+qgg4fF5-DOWHx3Btd0pTivTT9s_E=qqxg16YLQ@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
>> Do not blame pobox.com; they have nothing to do with the corruption of
>> your headers.
>
> No, but they have everything to do with *silently* dropping it. Why
> couldn't they _at least_ return an error saying that the headers are
> wrong? Note that other servers didn't even complain, they processed
> the mail happily.
Again, please do not blame pobox.com; they fall into your "other servers"
category. You are probably talking about vger.kernel.org that is extra
picky and wants to avoid wasting their outgoing bandwidth because they get
so much spams.
> % git blame -e -L 947,+7 contrib/completion/git-completion.bash v1.7.9
> ...
> f2bb9f88 (<spearce@spearce.org>> 2006-11-27 03:41:01 -0500 952)
I am glad to see that something useful came out from your digging, and a
fix is being worked on it, while I was away from my machines. Thanks for
getting the ball rolling.
^ permalink raw reply
* Re: [PATCH] send-email: add extra safetly in address sanitazion
From: Felipe Contreras @ 2012-02-05 20:51 UTC (permalink / raw)
To: Thomas Rast
Cc: git, Brandon Casey, Uwe Kleine-König, Brian Gernhardt,
Robin H. Johnson, Ævar Arnfjörð
In-Reply-To: <87sjipxe5u.fsf@thomas.inf.ethz.ch>
2012/2/5 Thomas Rast <trast@inf.ethz.ch>:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Currently bad addresses like 'Foo Bar <foo@bar.com>>' will just be sent
>> verbatim -- that's not good; we should either error out, or sanitize
>> them.
>>
>> The following patch adds extra sanitazion so the following
>> transformations are performed:
>>
>> 'Foo Bar <foo@bar.com>' -> 'Foo Bar <foo@bar.com>'
>> '"Foo Bar" <foo@bar.com>' -> '"Foo Bar" <foo@bar.com>'
>> 'foo@bar.com' -> 'foo@bar.com'
>> '<foo@bar.com>' -> 'foo@bar.com'
>> 'Foo Bar' -> 'Foo Bar'
>
> Am I the only one who stared at this for ten seconds, only to then
> realize that there is no sanitizing whatsoever going on here?
There is: '<foo@bar.com>' -> 'foo@bar.com'
>> 'Foo Bar <foo@bar.com>>' -> 'Foo Bar <foo@bar.com>'
>> '"Foo Bar" <foo@bar.com>>' -> '"Foo Bar" <foo@bar.com>'
>> '<foo@bar.com>>' -> 'foo@bar.com'
>
> All of these are the same underlying issue. Does your patch fix any
> other malformed addresses, or just this particular type?
See above.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 1/3] blame: fix email output with mailmap
From: Felipe Contreras @ 2012-02-05 20:58 UTC (permalink / raw)
To: Thomas Rast; +Cc: git
In-Reply-To: <87liohvysi.fsf@thomas.inf.ethz.ch>
On Sun, Feb 5, 2012 at 9:57 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>, "Brian Gianforcaro" <b.gianfo@gmail.com>, "Marius Storm-Olsen" <marius@trolltech.com>, "Junio C Hamano" <junkio@cox.net>
>
> I hope you noticed that Cc lists like the above really prove my point
> that you cannot automate common sense. Your cccmd script apparently
> uses the line range in
The fact that my script is less than perfect, doesn't mean you
*cannot* automate. Again, look at Linux's scripts/get_maintainer.pl.
It works * perfectly* fine.
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -1403,10 +1403,13 @@
>
> to determine who to Cc. But in doing so, it dug up an old address for
> Junio. It also added Brian whose only fault in all of this was fixing
> style of the 'if' you are patching.
Yes, it can be improved. But it's still better than nothing.
> And on top of that it had absolutely no way of knowing that Cc'ing Peff
> would have been a good idea, seeing as you two were involved in an
> earlier discussion about this precise bug.
>
> (Granted, omitting *Peff* doesn't make that much of a difference, since
> for all I know he reads every email that crosses this list. But my
> point still stands.)
That's not the fault of the script; that's what --cc is for.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 2/3] t: mailmap: add 'git blame -e' tests
From: Felipe Contreras @ 2012-02-05 21:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Marius Storm-Olsen
In-Reply-To: <7vvcnlggno.fsf@alter.siamese.dyndns.org>
On Sun, Feb 5, 2012 at 10:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>>> Look at the title:
>>>> add 'git blame -e' tests
>>>>
>>>> s/blame/blame -e/
>>>
>>> And? After copy/pasting this particular test with that substitution,
>>> what do we get a test for?
>>
>> For 'git blame -e'.
>>
>>> What class of problem is it supposed to catch?
>>
>> Problems related to 'git blame -e'?
>
> You very well know that we know you better than that, so it is no use to
> pretend to be dumb. It does not do anything other than wasting bandwidth
> and irritating readers.
That description is *perfectly* fine. It's succinct, there's nothing
wrong with that.
There's *nothing* else to say. There's no tests for 'git blame -e',
the patch adds tests for 'git blame -e', that's all, thus the title
"add 'git blame -e' tests.
The patch is good by itself, it doesn't _need_ any other context. It
would detect regressions on 'git blame -e', obviously, which is good.
> You know that you are not addressing "git blame with the -e option shows
> wrong line numbers on its output". The symptom you are checking with is
> "e-mail address output from 'blame -e' used to add an extra '>' at the end
> when only name is mapped, and I fixed it with the previous patch."
No, that's not true. This patch doesn't do that.
> Why is it so hard to either
>
> (1) give the more descriptive answer upfront when somebody who did not
> read the first patch of the 3-patch series pointed it out the comment
> is not descriptive enough the first time; or
Because it's not needed. Adding tests for 'git blame -e' is good in itself.
Some tests = better than no tests.
> (2) give the more descriptive answer and then say "we could do that, but
> when somebody views this change in "git log" as a part of 3-patch
> series, it should be clear enough---so let's aim for brevity instead
> of adding that two-line description" to defend the line in the patch?
What is unclear of "add 'git blame -e' tests"? It adds tests, that's good.
>> Or just apply it. Don't let the perfect be the enemy of the good.
>
> Perfect is the enemy of the good, but it is not an excuse to be sloppy.
>
> I tend to think that a single line "# blame -e" is sufficient if this were
> a part of just a single patch that has the fix and the test to guard the
> fix against future breakage (i.e. "not sloppy"). I would even say that not
> even "# blame" is necessary in such a case.
>
> But if this is a standalone patch to add this test, it does not describe
> what it wants to test very well.
It wants to test the output of 'git blame -e', as it can be obviates
from the title. Is there something wrong with that?
> In any case, I have doubts that the fix should go to blame and not to
> map_user(), so I'll see what happens in the further discussions.
This patch is orthogonal to that; there are no tests for 'git blame -e'.
--
Felipe Contreras
^ permalink raw reply
* Re: [bug] blame duplicates trailing ">" in mailmapped emails
From: Felipe Contreras @ 2012-02-05 21:11 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Jonathan Nieder, git, SZEDER Gábor
In-Reply-To: <20120204232015.GB1170@sigill.intra.peff.net>
On Sun, Feb 5, 2012 at 1:20 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 04, 2012 at 09:30:42PM +0200, Felipe Contreras wrote:
>
>> > but it feels like the fix should go into map_user. I tried a few things,
>> > like "git log -1 --format=%aE", and couldn't find other code paths with
>> > this problem. So presumably they are all feeding email addresses without
>> > the closing ">" (so one option is to just say "map_user needs to get
>> > NUL-terminated strings).
>>
>> Perhaps, but I though the idea was to make it efficient. I think the
>> above fix should be ok.
>
> Because of the calling convention of map_user, the buffer with the input
> must also be writable (since it holds the result). So there should be no
> loss of efficiency to convert the ">" into a "\0" (and in fact, the
> simplest fix is probably to just have map_user "tie off" any ">" it
> detects).
Yes, but then the caller (git blame) would need to _always_ do that
conversion before (">" -> "\0"), and after ("\0" -> ">"), as opposed
to now, that it does the conversion only when map_user succeeds (or
checks if it has to do it).
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] send-email: add extra safetly in address sanitazion
From: Junio C Hamano @ 2012-02-05 21:12 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git
In-Reply-To: <CAMP44s0xfbxLs_r81ppO9hYf3ML_gaYCaW3TKpLM=BjfaM8vHg@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> On Sat, Feb 4, 2012 at 5:10 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Otherwise, 'git send-email' would be happy to do:
>>
>> % git send-email --to '<foo@bar.com>>'
>>
>> And use '<foo@bar.com>>' in the headers.
>
> Er, actually that's not correct: '<foo@bar.com>>' will remain the
> same, but 'Foo <foo@bar.com>>' will be sanitized.
I suspect that this "Er" is merely a sympotom of a larger issue in the
approach taken by this patch. The code takes a potentially malformed
input, and applies a rewrite logic without telling the user what it is
doing. If the rewrite logic is perfect, that may be OK, but if not, the
logic to rewrite may or may not trigger, or when it triggers it may or may
not produce a correct result, and it all depends on the nature of breakage
in the input.
Wouldn't a better approach to detect problem on the input side and reject
a wrong one by erroring out, so that the user has a chance to fix?
^ permalink raw reply
* Re: [PATCH v4 4/4] completion: simplify __gitcomp*
From: Felipe Contreras @ 2012-02-05 21:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: SZEDER Gábor, git, Jonathan Nieder, Thomas Rast,
Shawn O. Pearce
In-Reply-To: <7vmx8xggan.fsf@alter.siamese.dyndns.org>
2012/2/5 Junio C Hamano <gitster@pobox.com>:
> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> And it does make a difference, it breaks the completion of a single
>> word in multiple steps, e.g. git log --pretty=<TAB> master..<TAB>. In
>> such cases we pass "${cur##--pretty=}" and "${cur_#*..}" as third
>> argument to __gitcomp() and __gitcomp_nl(), which can be empty strings
>> when the user hits TAB right after the '=' and '..'.
>
> After saying "this rewrite is wrong", I was actually wondering if I should
> have said "this rewrite is not faithful to the original". Based on your
> analysis, the difference does break the callers, so the rewrite is indeed
> wrong.
That's why we need tests for the completion stuff as well. I was
thinking on doing that, but if I have to write a peer-reviewed essay
with an introduction for the people that are not familiar with the
code in each of the patches, I'd rather not.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] send-email: add extra safetly in address sanitazion
From: Felipe Contreras @ 2012-02-05 21:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4nv5gf2n.fsf@alter.siamese.dyndns.org>
On Sun, Feb 5, 2012 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sat, Feb 4, 2012 at 5:10 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> Otherwise, 'git send-email' would be happy to do:
>>>
>>> % git send-email --to '<foo@bar.com>>'
>>>
>>> And use '<foo@bar.com>>' in the headers.
>>
>> Er, actually that's not correct: '<foo@bar.com>>' will remain the
>> same, but 'Foo <foo@bar.com>>' will be sanitized.
>
> I suspect that this "Er" is merely a sympotom of a larger issue in the
> approach taken by this patch. The code takes a potentially malformed
> input, and applies a rewrite logic without telling the user what it is
> doing.
That's what the function is doing already: sanitizing the address.
> Wouldn't a better approach to detect problem on the input side and reject
> a wrong one by erroring out, so that the user has a chance to fix?
Perhaps, but the code is not prepared for that. Anyway, feel free to
drop it, I am not interested in pursing this.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 0/3] On compresing large index
From: Thomas Rast @ 2012-02-05 21:22 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Joshua Redstone
In-Reply-To: <1328430605-4566-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> $ time ~/w/git/git ls-files | head >/dev/null
> real 0m4.635s
> user 0m4.258s
> sys 0m0.329s
>
> $ time ~/w/git/git update-index level-0-0000/foo
> real 0m4.593s
> user 0m4.264s
> sys 0m0.323s
[...]
> We need to figure out what git uses 4s user time for.
When I worked on the cache-tree stuff, my observation (based on
profiling, so I had actual data :-) was that computing SHA1s absolutely
dominates everything in such operations. It does that when writing the
index to write the trailing checksum, and also when loading it to verify
that the index is valid.
ls-files shouldn't be so slow though. A quick run with callgrind in a
linux-2.6.git tells me it spends about 45% of its time on SHA1s and a
whopping 25% in quote_c_style(). I wonder what's so hard about
quoting...
> This series may be useful on OSes that do not cache heavily. Though
> I'm not sure if there is any out there nowadays.
I think you could make a case that they should not be called "OS" ;-)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: git-svn: t9155 fails against subversion 1.7.0
From: Robin H. Johnson @ 2012-02-05 21:25 UTC (permalink / raw)
To: Git Mailing List
In-Reply-To: <op.v4pu1zcq0aolir@keputer>
On Thu, Nov 10, 2011 at 07:02:13AM +0100, Frans Klaver wrote:
> I missed $gmane/184644 in my search for this issue.
Did you make any progress in fixing this?
> On Tue, 08 Nov 2011 23:09:30 +0100, Frans Klaver <fransklaver@gmail.com>
> wrote:
>
> > For kicks I decided to run the tests and noticed that on master
> > t9155-git-svn-fetch-deleted-tag fails against svn 1.7.0. We hit an
> > assertion in subversion's dirent_uri.c, stating that we don't provide a
> > canonical url. I haven't tested against other subversion versions. I
> > dare assume that this issue doesn't arise on earlier versions. It
> > probably won't affect a lot of users right now, but it will in the
> > future.
--
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
^ permalink raw reply
* Re: Specifying revisions in the future
From: Andreas Schwab @ 2012-02-05 21:37 UTC (permalink / raw)
To: Matthieu Moy; +Cc: jpaugh, git
In-Reply-To: <vpqipjlf309.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> "the successor of <commit>", OTOH, is not well defined, since there can
> be several successors, and one can't order them reliably (you can't
> really know the set of successors, because they can exist in different
> repositories).
Yet it would be nice to have a concise notation for "the nth successor
of <commit> towards <commit>" (using --first-parent ordering when
ambiguous).
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: [bug] blame duplicates trailing ">" in mailmapped emails
From: Junio C Hamano @ 2012-02-05 21:38 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, Jonathan Nieder, git, SZEDER Gábor
In-Reply-To: <7v39aphw85.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
> ...
>> but it feels like the fix should go into map_user.
>
> Thanks.
>
> The map_user() API takes an email address that is terminated by either NUL
> or '>' to allow the caller to learn the corresponding up-to-date email
> address that is NUL terminated, while indicating with its return value
> that if the caller even needs to replace what it already has. But the
> function does not properly terminate email when it only touched the name
> part. And I think that is the real bug.
And the gist of the patch to fix the bug would look like this two liner.
In the real fix, "p" should be renamed to "end_of_email" or something
descriptive like that.
I only made sure that this fixes the original case of the email address of
Shawn reported by Felipe, but other than that like everything else I send
here with "... would look like this", not tested beyond "it compiles".
But conceptually it looks correct (famous last words ;-).
Felipe, does it pass your test cases?
mailmap.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/mailmap.c b/mailmap.c
index 8c3196c..ce805fa 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -236,6 +236,8 @@ int map_user(struct string_list *map,
}
if (maxlen_email && mi->email)
strlcpy(email, mi->email, maxlen_email);
+ else
+ *p = '\0';
if (maxlen_name && mi->name)
strlcpy(name, mi->name, maxlen_name);
debug_mm("map_user: to '%s' <%s>\n", name, mi->email ? mi->email : "");
^ permalink raw reply related
* Re: [PATCH] send-email: add extra safetly in address sanitazion
From: Thomas Rast @ 2012-02-05 21:51 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Brandon Casey, Uwe Kleine-König, Brian Gernhardt,
Robin H. Johnson, Ævar Arnfjörð
In-Reply-To: <CAMP44s1wqmT4mavsXrEhB-OquOtQrYnnoSoX9G7X4wzFoMD29A@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> 2012/2/5 Thomas Rast <trast@inf.ethz.ch>:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>> 'Foo Bar <foo@bar.com>' -> 'Foo Bar <foo@bar.com>'
>>> '"Foo Bar" <foo@bar.com>' -> '"Foo Bar" <foo@bar.com>'
>>> 'foo@bar.com' -> 'foo@bar.com'
>>> '<foo@bar.com>' -> 'foo@bar.com'
>>> 'Foo Bar' -> 'Foo Bar'
>>
>> Am I the only one who stared at this for ten seconds, only to then
>> realize that there is no sanitizing whatsoever going on here?
>
> There is: '<foo@bar.com>' -> 'foo@bar.com'
Indeed.
I still feel cheated as a reader though, you showed me four examples of
no change but let me figure that on my own.
>>> 'Foo Bar <foo@bar.com>>' -> 'Foo Bar <foo@bar.com>'
>>> '"Foo Bar" <foo@bar.com>>' -> '"Foo Bar" <foo@bar.com>'
>>> '<foo@bar.com>>' -> 'foo@bar.com'
>>
>> All of these are the same underlying issue. Does your patch fix any
>> other malformed addresses, or just this particular type?
>
> See above.
Ok, I see I am falling into the same communication trap as Jonathan, so
let's be more explicit.
Your commit message first tells me you are going to sanitize something,
but starts out with examples of leaving the string unchanged. Then it
continues with only the '>>' examples.
Today, and being someone who on average reads about half the mail that
comes through here, I know that this relates to the blame -e '>>' bug.
So today, I am wondering from the commit message why you narrowly focus
on that bug. But you don't! It's just that the commit message
insinuates it.
In a year, your reader (and bear in mind that this may very well be
yourself, at least if your memory is as good as mine) will wonder what
was so damn special about that '>>' string that it needs a specific fix
to send-email.
I see that you wrote in another thread:
> I have to write a peer-reviewed essay with an introduction for the
> people that are not familiar with the code in each of the patches
I'm not sure you meant it that literally, but the whole *point* is that
the message is for people who are not familiar with the code. After
all, if I knew that your code did the right thing in the right way, I
would not be bothering with reading the message. Today, I would just
send an Acked-by instead. In a year, I'd scroll down for another
potential culprit for the bug I'm hunting.
What's especially striking me about your proposed messages of late: they
leave me with more open questions than I started with. I tried to show
this above. I'm not sure whether other contributors are better at
answering questions, or just better at not touching any topics that
might raise them.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH] send-email: add extra safetly in address sanitazion
From: Junio C Hamano @ 2012-02-05 21:52 UTC (permalink / raw)
To: Thomas Rast
Cc: Felipe Contreras, git, Brandon Casey, Uwe Kleine-König,
Brian Gernhardt, Robin H. Johnson,
Ævar Arnfjörð Bjarmason
In-Reply-To: <87sjipxe5u.fsf@thomas.inf.ethz.ch>
Thomas Rast <trast@inf.ethz.ch> writes:
> Am I the only one who stared at this for ten seconds, only to then
> realize that there is no sanitizing whatsoever going on here?
>
>> 'Foo Bar <foo@bar.com>>' -> 'Foo Bar <foo@bar.com>'
>> '"Foo Bar" <foo@bar.com>>' -> '"Foo Bar" <foo@bar.com>'
>> '<foo@bar.com>>' -> 'foo@bar.com'
>
> All of these are the same underlying issue. Does your patch fix any
> other malformed addresses, or just this particular type?
Just this particular type, as long as the code handles it correctly, would
be better than nothing.
On the recieving end in mailinfo, I think we also support
gitster@pobox.com (Junio C Hamano)
although it does not seem to be used by many people these days (the only
one I can remember seeing on this list was merlyn), so we may want to be a
bit more consistent between sending and receiving end, though.
^ permalink raw reply
* Re: Specifying revisions in the future
From: Jakub Narebski @ 2012-02-05 21:57 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Matthieu Moy, jpaugh, git
In-Reply-To: <m21uq9x8q2.fsf@igel.home>
Andreas Schwab <schwab@linux-m68k.org> writes:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
> > "the successor of <commit>", OTOH, is not well defined, since there can
> > be several successors, and one can't order them reliably (you can't
> > really know the set of successors, because they can exist in different
> > repositories).
>
> Yet it would be nice to have a concise notation for "the nth successor
> of <commit> towards <commit>" (using --first-parent ordering when
> ambiguous).
First, "the nth successor"... from which refs? Commit objects have
pointers in one direction only, from commit to its ancestors (earlier
commits).
Second, `--first-parent' won't help here. Take for example the
following situation:
---X<---*<---.<---A
\
\--.<---B
X+3 is A or B? Note that pointers point _to_ '*' commit, so there is
not first or second here - no natural ordering like in the case of
commit parents.
--
Jakub Narebski
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox