* [PATCH v2 0/7] Cure some format-patch wrapping and encoding issues
From: Jan H. Schönherr @ 2012-10-18 14:43 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Jan H. Schönherr
Hi all.
[This is the second version of this series. If you still remember
the first version, you might want to jump directly to the summary
of changes below.]
The main point of this series is to teach git to encode my name
correctly, see patches 5+6, so that the decoded version is actually
my name, so that send-email does not insist on adding a wrong
superfluous From: line to the mail body.
The other patches more mostly by-products that fix other issues
I came across.
Patch 1 fixes an old off-by-one error, so that wrapped text may
now use all available columns.
Patches 2 and 3 make the wrapping of header lines more correct,
i. e., neither too early nor too late.
Patch 4 does some refactoring, which is too unrelated to be included
in one of the later patches.
Patch 5 improves RFC 2047 encoding; patch 6 removes an old non-RFC
conform workaround.
Patch 7 is more an RFC, which seems to be a good idea from my point
of view. Indeed, I thought the current implementation is erroneous,
until Junio C Hamano pointed out, that this might be desired behavior.
Thus, make up your mind about this one.
The series is currently based on the maint branch, but it applies
to master as well. It does also apply to next, but then my
implementation of isprint() has to be dropped from patch 5.
Changes in v2:
- patch 1 is new and is a result of the v1 discussion
- patch 5+6 split the old patch 4 into two patches
- use of constants for maximum line lengths
- even better adherence to RFC 2047 than v1
- updated commit messages/comments
Regards
Jan
Jan H. Schönherr (7):
utf8: fix off-by-one wrapping of text
format-patch: do not wrap non-rfc2047 headers too early
format-patch: do not wrap rfc2047 encoded headers too late
format-patch: introduce helper function last_line_length()
format-patch: make rfc2047 encoding more strict
format-patch: fix rfc2047 address encoding with respect to rfc822
specials
format-patch tests: check quoting/encoding in To: and Cc: headers
git-compat-util.h | 2 +
pretty.c | 149 +++++++++++++++++++++++--------
t/t4014-format-patch.sh | 231 ++++++++++++++++++++++++++++++------------------
t/t4202-log.sh | 4 +-
utf8.c | 2 +-
5 Dateien geändert, 262 Zeilen hinzugefügt(+), 126 Zeilen entfernt(-)
--
1.7.12
^ permalink raw reply
* Re: [PATCH] Add new git-remote-hd helper
From: Felipe Contreras @ 2012-10-18 14:26 UTC (permalink / raw)
To: Michael J Gruber
Cc: git, Junio C Hamano, Sverre Rabbelier, Johannes Schindelin,
Ilari Liusvaara, Daniel Barkalow
In-Reply-To: <50800197.90105@drmicha.warpmail.net>
On Thu, Oct 18, 2012 at 3:18 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Felipe Contreras venit, vidit, dixit 17.10.2012 14:58:
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>
>> I've looked at many hg<->git tools and none satisfy me. Too complicated, or too
>> slow, or to difficult to setup, etc.
>
> It's in an unsatisfying state, I agree. We have a great remote helper
> infrastructure, but neither git-svn nor git-cvsimport/export use it, and
> remote-hg is not in git.git.
>
> git-svn used to be our killer feature! (It's becoming hard to maintain,
> though.)
>
> git-hg (in the shape of a remote helper) could be our next killer
> feature, finally leading to our world domination ;)
>
>> The only one I've liked so far is hg-fast-export[1], which is indeed fast,
>> relatively simple, and relatively easy to use. But it's not properly maintained
>> any more.
>>
>> So, I decided to write my own from scratch, using hg-fast-export as
>> inspiration, and voila.
>>
>> This one doesn't have any dependencies, just put it into your $PATH, and you
>> can clone and fetch hg repositories. More importantly to me; the code is
>> simple, and easy to maintain.
>
> Well, it still has hg as a dependency. All our remote
> integration/helpers suffer from that. At least, every hg install comes
> with the modules, whereas svn is a beast (apr and such) that often comes
> without the language bindings.
Yeah, but there's no way around that, even if we write some code to
access the repository, it's quite likely that it will change. Unlike
git, mercurial developers expect people to access the repository
through mercurial API, not directly, and that's probably what we
should do if we want to avoid problems.
>> contrib/remote-hd/git-remote-hg | 231 ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 231 insertions(+)
>> create mode 100755 contrib/remote-hd/git-remote-hg
>
> That diffstat looks great (sans tests, of course; it's contrib), but
> there's no push functionality, and that is usually the most difficult
> part all helpers have to implement: Not only the push interaction, but
> also making sure that commits don't get duped in a roundtrip (git fetch
> from vcs, push to vcs, git fetch from vcs).
Yeah, it will probably require much more code, but I think by far the
most important feature is fetching.
> Just cloning and fetching can be done most easily with a shared worktree
> and scripting around "hg up" and "git commit -A" in some flavour.
Yea, but that will be dead slow.
>> diff --git a/contrib/remote-hd/git-remote-hg b/contrib/remote-hd/git-remote-hg
>> new file mode 100755
>> index 0000000..9157b30
>> --- /dev/null
>> +++ b/contrib/remote-hd/git-remote-hg
>> @@ -0,0 +1,231 @@
>> +#!/usr/bin/python2
>> +
>> +# Inspired by Rocco Rutte's hg-fast-export
>> +
>> +# Just copy to your ~/bin, or anywhere in your $PATH.
>> +# Then you can clone with:
>> +# hg::file:///path/to/mercurial/repo/
>> +
>> +from mercurial import hg, ui
>> +
>> +import re
>> +import sys
>> +import os
>> +import json
>> +
>> +def die(msg, *args):
>> + print >> sys.stderr, 'ERROR:', msg % args
>> + sys.exit(1)
>
> While we don't have to code for py3, avoiding '>>' will help us later.
> (It got removed in py3.) sys.sdterr.write() should be most portable.
All right.
>> +def gitmode(flags):
>> + return 'l' in flags and '120000' or 'x' in flags and '100755' or '100644'
>> +
>> +def export_file(fc):
>> + if fc.path() == '.hgtags':
>
> Is this always relative? Just wondering, dunno.
AFAIK, why wouldn't it?
>> +def get_repo(path, alias):
>> + myui = ui.ui()
>> + myui.setconfig('ui', 'interactive', 'off')
>> + repo = hg.repository(myui, path)
>> + return repo
>
> Is there a reason to spell this out, e.g.: Why not
>
> return hg.repository(myui, path)
No reason, but I already have patches on top of this.
>> +def hg_branch(b):
>> + if b == 'master':
>> + return 'default'
>> + return b
>> +
>> +def git_branch(b):
>> + if b == 'default':
>> + return 'master'
>> + return b
>> +
>> +def export_tag(repo, tag):
>> + global prefix
>> + print "reset %s/tags/%s" % (prefix, tag)
>> + print "from :%s" % (repo[tag].rev() + 1)
>> + print
>> +
>> +def export_branch(repo, branch):
>> + global prefix, marks, cache, branches
>> +
>> + heads = branches[hg_branch(branch)]
>> +
>> + # verify there's only one head
>> + if (len(heads) > 1):
>> + die("Branch '%s' has more than one head" % hg_branch(branch))
>
> We have to deal with this at some point... Do you support "hg
> bookmarks"? They'd be an option, or we implement better detached head
> handling in git...
I already updated this, I converted it to a warning and just picked a
random head.
Adding support for bookmarks should be easy, it's just a matter of
deciding how to differentiate branches from bookmarks. Perhaps
'refs/heads/bookmarks/foo'.
>> + revs = [rev for rev in revs if not cache.get(rev, False)]
>> +
>> + for rev in revs:
>
> Those lines set up the list just so that you iterate over it later.
> Please don't do this (unless you know that revs is very small).
>
> for rev in revs:
> if cache.get(rev, False):
> continue
>
> is more performant. You can reduce this further to
>
> count=0
> for rev in repo.revs('%u:%u' % (tip, head)):
> if cache.get(rev, False):
> continue
>
> which is even more performant generally, and especially if repo.revs()
> returns an iterator rather than a list.
>
> [Yes, you could use lambda+filter, but let's not get religious. The
> above is simple and pythonic.]
And we wouldn't be able to calculate the progress: len(revs).
>> +
>> + c = repo[rev]
>> + (manifest, user, (time, tz), files, desc, extra) = repo.changelog.read(c.node())
>
> Same here, you introduce c just for the next line (unless I'm mistaken).
The second line is already too big.
>> + print "from :%s" % (parents[0] + 1)
>
> What is this +1 offset that is appearing here and there?
Marks start from 1, no? If not, this can be removed.
>> +def do_list(repo, args):
>> + global branches
>> +
>> + head = repo.dirstate.branch()
>> + for branch in repo.branchmap():
>> + heads = repo.branchheads(branch)
>> + if len(heads):
>> + branches[branch] = heads
>
> I'm a bit confused here. repo.brancheads() is a list, no? Is this the
> single head case only? I'd expect [0] of that, but you seem to be
> getting branch names (strings).
No, heads is a list of heads, as nodes.
> Also, if len(heads) == 0 then branches[branch] is undefined or stale. No?
Yes, but we don't care about those branches; they are closed.
> Overall, this looks like plain scripting in python rather than anything
> oo'ish, but that's okay and probably dictated by the remote helper
> interface and/or the existing exporter.
If there was an advantage of creating new classes, I would be all for
it, but I don't see any.
> I'm all for an improvement in that area, but still feel we'd need a
> combined effort rather than yet another start.
Me too, but as far a sverre's remote-hg code goes, there isn't even a
branch to work from.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* [PATCH v3] status: refactor output format to represent "default" and add --long
From: Nguyễn Thái Ngọc Duy @ 2012-10-18 14:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20121018020308.GA24484@sigill.intra.peff.net>
From: Jeff King <peff@peff.net>
When deciding which output format to use, we default an internal enum
to STATUS_FORMAT_LONG and modify it if "--porcelain" or "--short" is
given. If this enum is set to LONG, then we know the user has not
specified any format, and we can kick in default behaviors. This works
because there is no "--long" which they could use to explicitly
specify LONG.
Let's expand the enum to have an explicit STATUS_FORMAT_NONE, in
preparation for adding "--long", which can be used to override --short
or --porcelain. Then we can distinguish between LONG and NONE when
setting other defaults. There are two such cases:
1. The user has asked for NUL termination. With NONE, we
currently default to turning on the porcelain mode.
With an explicit --long, we would in theory use NUL
termination with the long mode, but it does not support
it. So we can just complain and die.
2. When an output format is given to "git commit", we
default to "--dry-run". This behavior would now kick in
when "--long" is given, too.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
On Thu, Oct 18, 2012 at 9:03 AM, Jeff King <peff@peff.net> wrote:
> I think that is fine to split it like this, but you would want to update
> the commit message above. Probably just remove those two cases and say
> something like:
>
> Note that you cannot actually trigger STATUS_FORMAT_LONG, as we do
> not yet have "--long"; that will come in a follow-on patch.
>
> And then move the reasoning for how "--long" works with each case into
> the commit message of the next patch.
Nope, it's hard to split the explanation in two (at least to me),
which may mean that the split does not make sense.
Documentation/git-commit.txt | 4 ++++
Documentation/git-status.txt | 3 +++
builtin/commit.c | 29 +++++++++++++++++++++++------
3 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 9594ac8..3acf2e7 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -109,6 +109,10 @@ OPTIONS
format. See linkgit:git-status[1] for details. Implies
`--dry-run`.
+--long::
+ When doing a dry-run, give the output in a the long-format.
+ Implies `--dry-run`.
+
-z::
--null::
When showing `short` or `porcelain` status output, terminate
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 67e5f53..9f1ef9a 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -38,6 +38,9 @@ OPTIONS
across git versions and regardless of user configuration. See
below for details.
+--long::
+ Give the output in the long-format. This is the default.
+
-u[<mode>]::
--untracked-files[=<mode>]::
Show untracked files.
diff --git a/builtin/commit.c b/builtin/commit.c
index a17a5df..1dd2ec5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -112,10 +112,11 @@ static const char *only_include_assumed;
static struct strbuf message = STRBUF_INIT;
static enum {
+ STATUS_FORMAT_NONE = 0,
STATUS_FORMAT_LONG,
STATUS_FORMAT_SHORT,
STATUS_FORMAT_PORCELAIN
-} status_format = STATUS_FORMAT_LONG;
+} status_format;
static int opt_parse_m(const struct option *opt, const char *arg, int unset)
{
@@ -454,6 +455,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
case STATUS_FORMAT_PORCELAIN:
wt_porcelain_print(s);
break;
+ case STATUS_FORMAT_NONE:
case STATUS_FORMAT_LONG:
wt_status_print(s);
break;
@@ -1058,9 +1060,13 @@ static int parse_and_validate_options(int argc, const char *argv[],
if (all && argc > 0)
die(_("Paths with -a does not make sense."));
- if (s->null_termination && status_format == STATUS_FORMAT_LONG)
- status_format = STATUS_FORMAT_PORCELAIN;
- if (status_format != STATUS_FORMAT_LONG)
+ if (s->null_termination) {
+ if (status_format == STATUS_FORMAT_NONE)
+ status_format = STATUS_FORMAT_PORCELAIN;
+ else if (status_format == STATUS_FORMAT_LONG)
+ die(_("--long and -z are incompatible"));
+ }
+ if (status_format != STATUS_FORMAT_NONE)
dry_run = 1;
return argc;
@@ -1159,6 +1165,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
OPT_SET_INT(0, "porcelain", &status_format,
N_("machine-readable output"),
STATUS_FORMAT_PORCELAIN),
+ OPT_SET_INT(0, "long", &status_format,
+ N_("show status in long format (default)"),
+ STATUS_FORMAT_LONG),
OPT_BOOLEAN('z', "null", &s.null_termination,
N_("terminate entries with NUL")),
{ OPTION_STRING, 'u', "untracked-files", &untracked_files_arg,
@@ -1186,8 +1195,12 @@ int cmd_status(int argc, const char **argv, const char *prefix)
builtin_status_usage, 0);
finalize_colopts(&s.colopts, -1);
- if (s.null_termination && status_format == STATUS_FORMAT_LONG)
- status_format = STATUS_FORMAT_PORCELAIN;
+ if (s.null_termination) {
+ if (status_format == STATUS_FORMAT_NONE)
+ status_format = STATUS_FORMAT_PORCELAIN;
+ else if (status_format == STATUS_FORMAT_LONG)
+ die(_("--long and -z are incompatible"));
+ }
handle_untracked_files_arg(&s);
if (show_ignored_in_status)
@@ -1216,6 +1229,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
case STATUS_FORMAT_PORCELAIN:
wt_porcelain_print(&s);
break;
+ case STATUS_FORMAT_NONE:
case STATUS_FORMAT_LONG:
s.verbose = verbose;
s.ignore_submodule_arg = ignore_submodule_arg;
@@ -1386,6 +1400,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
OPT_BOOLEAN(0, "branch", &s.show_branch, N_("show branch information")),
OPT_SET_INT(0, "porcelain", &status_format,
N_("machine-readable output"), STATUS_FORMAT_PORCELAIN),
+ OPT_SET_INT(0, "long", &status_format,
+ N_("show status in long format (default)"),
+ STATUS_FORMAT_LONG),
OPT_BOOLEAN('z', "null", &s.null_termination,
N_("terminate entries with NUL")),
OPT_BOOLEAN(0, "amend", &amend, N_("amend previous commit")),
--
1.8.0.rc2.22.gad9383a
^ permalink raw reply related
* Re: [PATCH] Add new git-remote-hd helper
From: Michael J Gruber @ 2012-10-18 13:18 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Sverre Rabbelier, Johannes Schindelin,
Ilari Liusvaara, Daniel Barkalow
In-Reply-To: <1350478721-3685-1-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras venit, vidit, dixit 17.10.2012 14:58:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>
> I've looked at many hg<->git tools and none satisfy me. Too complicated, or too
> slow, or to difficult to setup, etc.
It's in an unsatisfying state, I agree. We have a great remote helper
infrastructure, but neither git-svn nor git-cvsimport/export use it, and
remote-hg is not in git.git.
git-svn used to be our killer feature! (It's becoming hard to maintain,
though.)
git-hg (in the shape of a remote helper) could be our next killer
feature, finally leading to our world domination ;)
> The only one I've liked so far is hg-fast-export[1], which is indeed fast,
> relatively simple, and relatively easy to use. But it's not properly maintained
> any more.
>
> So, I decided to write my own from scratch, using hg-fast-export as
> inspiration, and voila.
>
> This one doesn't have any dependencies, just put it into your $PATH, and you
> can clone and fetch hg repositories. More importantly to me; the code is
> simple, and easy to maintain.
Well, it still has hg as a dependency. All our remote
integration/helpers suffer from that. At least, every hg install comes
with the modules, whereas svn is a beast (apr and such) that often comes
without the language bindings.
> [1] http://repo.or.cz/w/fast-export.git
>
> contrib/remote-hd/git-remote-hg | 231 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 231 insertions(+)
> create mode 100755 contrib/remote-hd/git-remote-hg
That diffstat looks great (sans tests, of course; it's contrib), but
there's no push functionality, and that is usually the most difficult
part all helpers have to implement: Not only the push interaction, but
also making sure that commits don't get duped in a roundtrip (git fetch
from vcs, push to vcs, git fetch from vcs).
Just cloning and fetching can be done most easily with a shared worktree
and scripting around "hg up" and "git commit -A" in some flavour.
> diff --git a/contrib/remote-hd/git-remote-hg b/contrib/remote-hd/git-remote-hg
> new file mode 100755
> index 0000000..9157b30
> --- /dev/null
> +++ b/contrib/remote-hd/git-remote-hg
> @@ -0,0 +1,231 @@
> +#!/usr/bin/python2
> +
> +# Inspired by Rocco Rutte's hg-fast-export
> +
> +# Just copy to your ~/bin, or anywhere in your $PATH.
> +# Then you can clone with:
> +# hg::file:///path/to/mercurial/repo/
> +
> +from mercurial import hg, ui
> +
> +import re
> +import sys
> +import os
> +import json
> +
> +def die(msg, *args):
> + print >> sys.stderr, 'ERROR:', msg % args
> + sys.exit(1)
While we don't have to code for py3, avoiding '>>' will help us later.
(It got removed in py3.) sys.sdterr.write() should be most portable.
> +def gitmode(flags):
> + return 'l' in flags and '120000' or 'x' in flags and '100755' or '100644'
> +
> +def export_file(fc):
> + if fc.path() == '.hgtags':
Is this always relative? Just wondering, dunno.
> + return
> + d = fc.data()
> + print "M %s inline %s" % (gitmode(fc.flags()), fc.path())
> + print "data %d" % len(d)
> + print d
> +
> +def get_filechanges(repo, ctx, parents):
> + l = [repo.status(p, ctx)[:3] for p in parents]
> + changed, added, removed = [sum(e, []) for e in zip(*l)]
> + return added + changed, removed
> +
> +author_re = re.compile('^((.+?) )?(<.+?>)$')
> +
> +def fixup_user(user):
> + user = user.replace('"', '')
> + m = author_re.match(user)
> + if m:
> + name = m.group(1)
> + mail = m.group(3)
> + else:
> + name = user
> + mail = None
> +
> + if not name:
> + name = 'Unknown'
> + if not mail:
> + mail = '<unknown>'
> +
> + return '%s %s' % (name, mail)
> +
> +def get_repo(path, alias):
> + myui = ui.ui()
> + myui.setconfig('ui', 'interactive', 'off')
> + repo = hg.repository(myui, path)
> + return repo
Is there a reason to spell this out, e.g.: Why not
return hg.repository(myui, path)
> +
> +def hg_branch(b):
> + if b == 'master':
> + return 'default'
> + return b
> +
> +def git_branch(b):
> + if b == 'default':
> + return 'master'
> + return b
> +
> +def export_tag(repo, tag):
> + global prefix
> + print "reset %s/tags/%s" % (prefix, tag)
> + print "from :%s" % (repo[tag].rev() + 1)
> + print
> +
> +def export_branch(repo, branch):
> + global prefix, marks, cache, branches
> +
> + heads = branches[hg_branch(branch)]
> +
> + # verify there's only one head
> + if (len(heads) > 1):
> + die("Branch '%s' has more than one head" % hg_branch(branch))
We have to deal with this at some point... Do you support "hg
bookmarks"? They'd be an option, or we implement better detached head
handling in git...
> +
> + head = repo[heads[0]]
> + tip = marks.get(branch, 0)
> + # mercurial takes too much time checking this
> + if tip == head.rev():
> + # nothing to do
> + return
> + revs = repo.revs('%u:%u' % (tip, head))
> + count = 0
> +
> + revs = [rev for rev in revs if not cache.get(rev, False)]
> +
> + for rev in revs:
Those lines set up the list just so that you iterate over it later.
Please don't do this (unless you know that revs is very small).
for rev in revs:
if cache.get(rev, False):
continue
is more performant. You can reduce this further to
count=0
for rev in repo.revs('%u:%u' % (tip, head)):
if cache.get(rev, False):
continue
which is even more performant generally, and especially if repo.revs()
returns an iterator rather than a list.
[Yes, you could use lambda+filter, but let's not get religious. The
above is simple and pythonic.]
> +
> + c = repo[rev]
> + (manifest, user, (time, tz), files, desc, extra) = repo.changelog.read(c.node())
Same here, you introduce c just for the next line (unless I'm mistaken).
> + rev_branch = git_branch(extra['branch'])
> +
> + tz = '%+03d%02d' % (-tz / 3600, -tz % 3600 / 60)
> +
> + print "commit %s/branches/%s" % (prefix, rev_branch)
> + print "mark :%d" % (rev + 1)
> + print "committer %s %d %s" % (fixup_user(user), time, tz)
> + print "data %d" % (len(desc) + 1)
> + print desc
> + print
> +
> + parents = [p for p in repo.changelog.parentrevs(rev) if p >= 0]
> +
> + if len(parents) == 0:
> + modified = c.manifest().keys()
> + removed = []
> + else:
> + added = []
> + changed = []
> + print "from :%s" % (parents[0] + 1)
What is this +1 offset that is appearing here and there?
> + if len(parents) > 1:
> + print "merge :%s" % (parents[1] + 1)
> + modified, removed = get_filechanges(repo, c, parents)
> +
> + for f in removed:
> + print "D %s" % (f)
> + for f in modified:
> + export_file(c.filectx(f))
> + print
> +
> + count += 1
> + if (count % 100 == 0):
> + print "progress revision %d '%s' (%d/%d)" % (rev, branch, count, len(revs))
> + print "#############################################################"
> +
> + cache[rev] = True
> +
> + # store the latest revision
> + marks[branch] = rev
> +
> +def do_capabilities(repo, args):
> + global prefix, dirname
> +
> + print "import"
> + print "refspec refs/heads/*:%s/branches/*" % prefix
> + print "refspec refs/tags/*:%s/tags/*" % prefix
> +
> + path = os.path.join(dirname, 'marks-git')
> +
> + print "*export-marks %s" % path
> + if os.path.exists(path):
> + print "*import-marks %s" % path
> +
> + print
> +
> +def do_list(repo, args):
> + global branches
> +
> + head = repo.dirstate.branch()
> + for branch in repo.branchmap():
> + heads = repo.branchheads(branch)
> + if len(heads):
> + branches[branch] = heads
I'm a bit confused here. repo.brancheads() is a list, no? Is this the
single head case only? I'd expect [0] of that, but you seem to be
getting branch names (strings).
Also, if len(heads) == 0 then branches[branch] is undefined or stale. No?
> +
> + for branch in branches:
> + print "? refs/heads/%s" % git_branch(branch)
> + for tag, node in repo.tagslist():
> + if tag == 'tip':
> + continue
> + print "? refs/tags/%s" % tag
> + print "@refs/heads/%s HEAD" % git_branch(head)
> + print
> +
> +def do_import(repo, args):
> + ref = args[1]
> +
> + if (ref == 'HEAD'):
> + return
> +
> + if ref.startswith('refs/heads/'):
> + branch = ref[len('refs/heads/'):]
> + export_branch(repo, branch)
> + elif ref.startswith('refs/tags/'):
> + tag = ref[len('refs/tags/'):]
> + export_tag(repo, tag)
> +
> +def main(args):
> + global prefix, dirname, marks, cache, branches
> +
> + alias = args[1]
> + url = args[2]
> +
> + gitdir = os.environ['GIT_DIR']
> + dirname = os.path.join(gitdir, 'hg')
> + cache = {}
> + branches = {}
> +
> + repo = get_repo(url, alias)
> + prefix = 'refs/hg/%s' % alias
> +
> + if not os.path.exists(dirname):
> + os.makedirs(dirname)
> +
> + marks_path = os.path.join(dirname, 'marks-hg')
> + try:
> + fp = open(marks_path, 'r')
> + marks = json.load(fp)
> + fp.close()
> + except IOError:
> + marks = {}
> +
> + line = True
> + while (line):
> + line = sys.stdin.readline().strip()
> + if line == '':
> + break
> + args = line.split()
> + cmd = args[0]
> + if cmd == 'capabilities':
> + do_capabilities(repo, args)
> + elif cmd == 'list':
> + do_list(repo, args)
> + elif cmd == 'import':
> + do_import(repo, args)
> + sys.stdout.flush()
> +
> + fp = open(marks_path, 'w')
> + json.dump(marks, fp)
> + fp.close()
> +
> +sys.exit(main(sys.argv))
>
Overall, this looks like plain scripting in python rather than anything
oo'ish, but that's okay and probably dictated by the remote helper
interface and/or the existing exporter.
I'm all for an improvement in that area, but still feel we'd need a
combined effort rather than yet another start.
Michael
^ permalink raw reply
* Re: [PATCH 0/6] Bring "format-patch --notes" closer to a real feature
From: Michael J Gruber @ 2012-10-18 12:24 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git
In-Reply-To: <CACsJy8Dxx6JtRXP_HWNBrobNems+4UsaOGSRef08cf8--b5dOw@mail.gmail.com>
Nguyen Thai Ngoc Duy venit, vidit, dixit 18.10.2012 13:06:
> On Thu, Oct 18, 2012 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> This replaces the earlier "wip" with a real thing.
>>
>> We never advertised the "--notes" option to format-patch (or
>> anything related to the pretty format options for that matter)
>> because the behaviour of these options was whatever they happened to
>> do, not what they were designed to do.
>
> Stupid question: does "git am" recreate notes from "format-patch
> --notes" output? If it does not, should it? I think it could be a nice
> way of copying notes from one machine to another, but not enabled by
> default (iow "am --notes").
>
Not yet, but you can already start basing your am patches on top of
Junio's series ;)
am should allow a notes ref (--notes=hereyougo) like other similar
instances.
Michael
^ permalink raw reply
* Re: [PATCH] notes: mention --notes in more places
From: Michael J Gruber @ 2012-10-18 12:11 UTC (permalink / raw)
To: Jeff King; +Cc: Eric Blake, git, Junio C Hamano
In-Reply-To: <20121017190507.GA17648@sigill.intra.peff.net>
Jeff King venit, vidit, dixit 17.10.2012 21:05:
> On Wed, Oct 17, 2012 at 07:30:56AM -0600, Eric Blake wrote:
>
>>> We've talked about it several times, but it's never happened (probably
>>> because most people don't actually use notes).
>>
>> And people (like me) don't use notes because they aren't documented.
>> Catch-22, so we have to start somewhere.
>
> Oh, I definitely agree your patch is the right direction. I was just
> explaining why it hasn't happened, even though people think it's a good
> idea.
>
>> I'll submit a v2 with the non-controversial edits, and spend some time
>> trying to figure out how to isolate the portion of pretty-options.txt
>> that is relevant to format-patch. If it's easy enough, I can also
>> consider using --- instead of Notes: as the separator when using
>> format-patch.
>
> Hmm. After digging in the archive, it seems we (including both you and
> me!) have discussed this several times, and there are even some patches
> floating around. Maybe one of them would be a good starting point for
> your submission (I did not read carefully over all of the arguments for
> each):
>
> Patch from Thomas, Feb 2010:
>
> http://thread.gmane.org/gmane.comp.version-control.git/139919/focus=140818
>
> Discussion between us, Dec 2010:
>
> http://thread.gmane.org/gmane.comp.version-control.git/163141
>
> Patch from Michael, Apr 2011:
>
> http://thread.gmane.org/gmane.comp.version-control.git/172079
That one used to work for about one more year or so (it went through a
few rebases) but stopped working during some rework involving the
signature (signed-off-by), i.e. it puts the notes before the signed-off
now. I didn't update it because nobody seemed interested anyway (and
because branch-notes got implemented in a different, non-note way, so I
dumped that part of my workflow also).
Michael
^ permalink raw reply
* [PATCH 5/5] branch: show targets of deleted symrefs, not sha1s
From: René Scharfe @ 2012-10-18 12:08 UTC (permalink / raw)
To: Git mailing list; +Cc: Junio C Hamano, Johan Herland, Miklos Vajna
In-Reply-To: <507FEF0B.1060309@lsrfire.ath.cx>
git branch reports the abbreviated hash of the head commit of
a deleted branch to make it easier for a user to undo the
operation. For symref branches this doesn't help. Print the
symref target instead for them.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
builtin/branch.c | 19 +++++++++++--------
t/t3200-branch.sh | 5 ++---
2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index d87035a..1ec9c02 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -251,15 +251,18 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
: _("Error deleting branch '%s'"),
bname.buf);
ret = 1;
- } else {
- if (!quiet)
- printf(remote_branch
- ? _("Deleted remote branch %s (was %s).\n")
- : _("Deleted branch %s (was %s).\n"),
- bname.buf,
- find_unique_abbrev(sha1, DEFAULT_ABBREV));
- delete_branch_config(bname.buf);
+ continue;
+ }
+ if (!quiet) {
+ printf(remote_branch
+ ? _("Deleted remote branch %s (was %s).\n")
+ : _("Deleted branch %s (was %s).\n"),
+ bname.buf,
+ (flags & REF_ISSYMREF)
+ ? target
+ : find_unique_abbrev(sha1, DEFAULT_ABBREV));
}
+ delete_branch_config(bname.buf);
}
free(name);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 1323f6f..80e6be3 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -265,8 +265,7 @@ test_expect_success 'config information was renamed, too' \
test_expect_success 'deleting a symref' '
git branch target &&
git symbolic-ref refs/heads/symref refs/heads/target &&
- sha1=$(git rev-parse symref | cut -c 1-7) &&
- echo "Deleted branch symref (was $sha1)." >expect &&
+ echo "Deleted branch symref (was refs/heads/target)." >expect &&
git branch -d symref >actual &&
test_path_is_file .git/refs/heads/target &&
test_path_is_missing .git/refs/heads/symref &&
@@ -276,7 +275,7 @@ test_expect_success 'deleting a symref' '
test_expect_success 'deleting a dangling symref' '
git symbolic-ref refs/heads/dangling-symref nowhere &&
test_path_is_file .git/refs/heads/dangling-symref &&
- echo "Deleted branch dangling-symref (was 0000000)." >expect &&
+ echo "Deleted branch dangling-symref (was nowhere)." >expect &&
git branch -d dangling-symref >actual &&
test_path_is_missing .git/refs/heads/dangling-symref &&
test_i18ncmp expect actual
--
1.7.12
^ permalink raw reply related
* [PATCH 4/5] branch: skip commit checks when deleting symref branches
From: René Scharfe @ 2012-10-18 12:07 UTC (permalink / raw)
To: Git mailing list; +Cc: Junio C Hamano, Johan Herland, Miklos Vajna
In-Reply-To: <507FEF0B.1060309@lsrfire.ath.cx>
Before a branch is deleted, we check that it points to a valid
commit. With -d we also check that the commit is a merged; this
check is not done with -D.
The reason for that is that commits pointed to by branches should
never go missing; if they do then something broke and it's better
to stop instead of adding to the mess. And a non-merged commit
may contain changes that are worth preserving, so we require the
stronger option -D instead of -d to get rid of them.
If a branch consists of a symref, these concerns don't apply.
Deleting such a branch can't make a commit become unreferenced,
so we don't need to check if it is merged, or even if it is
actually a valid commit. Skip them in that case. This allows
us to delete dangling symref branches.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
builtin/branch.c | 10 ++++++++--
t/t3200-branch.sh | 9 +++++++++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 5e1e5b4..d87035a 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -214,6 +214,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
die(_("Couldn't look up commit object for HEAD"));
}
for (i = 0; i < argc; i++, strbuf_release(&bname)) {
+ const char *target;
+ int flags = 0;
+
strbuf_branchname(&bname, argv[i]);
if (kinds == REF_LOCAL_BRANCH && !strcmp(head, bname.buf)) {
error(_("Cannot delete the branch '%s' "
@@ -225,7 +228,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
free(name);
name = mkpathdup(fmt, bname.buf);
- if (read_ref(name, sha1)) {
+ target = resolve_ref_unsafe(name, sha1, 0, &flags);
+ if (!target ||
+ (!(flags & REF_ISSYMREF) && is_null_sha1(sha1))) {
error(remote_branch
? _("remote branch '%s' not found.")
: _("branch '%s' not found."), bname.buf);
@@ -233,7 +238,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
continue;
}
- if (check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
+ if (!(flags & REF_ISSYMREF) &&
+ check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
force)) {
ret = 1;
continue;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ec5f70e..1323f6f 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -273,6 +273,15 @@ test_expect_success 'deleting a symref' '
test_i18ncmp expect actual
'
+test_expect_success 'deleting a dangling symref' '
+ git symbolic-ref refs/heads/dangling-symref nowhere &&
+ test_path_is_file .git/refs/heads/dangling-symref &&
+ echo "Deleted branch dangling-symref (was 0000000)." >expect &&
+ git branch -d dangling-symref >actual &&
+ test_path_is_missing .git/refs/heads/dangling-symref &&
+ test_i18ncmp expect actual
+'
+
test_expect_success 'renaming a symref is not allowed' \
'
git symbolic-ref refs/heads/master2 refs/heads/master &&
--
1.7.12
^ permalink raw reply related
* [PATCH 3/5] branch: delete symref branch, not its target
From: René Scharfe @ 2012-10-18 12:05 UTC (permalink / raw)
Cc: Junio C Hamano, Johan Herland, Git mailing list, Miklos Vajna
In-Reply-To: <507FEF0B.1060309@lsrfire.ath.cx>
If a branch that is to be deleted happens to be a symref to another
branch, the current code removes the targeted branch instead of the
one it was called for.
Change this surprising behaviour and delete the symref branch
instead.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
builtin/branch.c | 2 +-
t/t3200-branch.sh | 11 +++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 97c7361..5e1e5b4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -239,7 +239,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
continue;
}
- if (delete_ref(name, sha1, 0)) {
+ if (delete_ref(name, sha1, REF_NODEREF)) {
error(remote_branch
? _("Error deleting remote branch '%s'")
: _("Error deleting branch '%s'"),
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 79c8d01..ec5f70e 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -262,6 +262,17 @@ test_expect_success 'config information was renamed, too' \
"test $(git config branch.s.dummy) = Hello &&
test_must_fail git config branch.s/s/dummy"
+test_expect_success 'deleting a symref' '
+ git branch target &&
+ git symbolic-ref refs/heads/symref refs/heads/target &&
+ sha1=$(git rev-parse symref | cut -c 1-7) &&
+ echo "Deleted branch symref (was $sha1)." >expect &&
+ git branch -d symref >actual &&
+ test_path_is_file .git/refs/heads/target &&
+ test_path_is_missing .git/refs/heads/symref &&
+ test_i18ncmp expect actual
+'
+
test_expect_success 'renaming a symref is not allowed' \
'
git symbolic-ref refs/heads/master2 refs/heads/master &&
--
1.7.12
^ permalink raw reply related
* [PATCH 2/5] branch: factor out delete_branch_config()
From: René Scharfe @ 2012-10-18 12:04 UTC (permalink / raw)
To: Git mailing list; +Cc: Junio C Hamano, Johan Herland, Miklos Vajna
In-Reply-To: <507FEF0B.1060309@lsrfire.ath.cx>
Provide a small helper function for deleting branch config sections.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
builtin/branch.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 852019e..97c7361 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -172,6 +172,15 @@ static int check_branch_commit(const char *branchname, const char *refname,
return 0;
}
+static void delete_branch_config(const char *branchname)
+{
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addf(&buf, "branch.%s", branchname);
+ if (git_config_rename_section(buf.buf, NULL) < 0)
+ warning(_("Update of config-file failed"));
+ strbuf_release(&buf);
+}
+
static int delete_branches(int argc, const char **argv, int force, int kinds,
int quiet)
{
@@ -237,17 +246,13 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
bname.buf);
ret = 1;
} else {
- struct strbuf buf = STRBUF_INIT;
if (!quiet)
printf(remote_branch
? _("Deleted remote branch %s (was %s).\n")
: _("Deleted branch %s (was %s).\n"),
bname.buf,
find_unique_abbrev(sha1, DEFAULT_ABBREV));
- strbuf_addf(&buf, "branch.%s", bname.buf);
- if (git_config_rename_section(buf.buf, NULL) < 0)
- warning(_("Update of config-file failed"));
- strbuf_release(&buf);
+ delete_branch_config(bname.buf);
}
}
--
1.7.12
^ permalink raw reply related
* [PATCH 1/5] branch: factor out check_branch_commit()
From: René Scharfe @ 2012-10-18 12:02 UTC (permalink / raw)
To: Git mailing list; +Cc: Junio C Hamano, Johan Herland, Miklos Vajna
In-Reply-To: <507FEF0B.1060309@lsrfire.ath.cx>
Move the code to perform checks on the tip commit of a branch
to its own function.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
builtin/branch.c | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index ffd2684..852019e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -154,10 +154,28 @@ static int branch_merged(int kind, const char *name,
return merged;
}
+static int check_branch_commit(const char *branchname, const char *refname,
+ unsigned char *sha1, struct commit *head_rev,
+ int kinds, int force)
+{
+ struct commit *rev = lookup_commit_reference(sha1);
+ if (!rev) {
+ error(_("Couldn't look up commit object for '%s'"), refname);
+ return -1;
+ }
+ if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
+ error(_("The branch '%s' is not fully merged.\n"
+ "If you are sure you want to delete it, "
+ "run 'git branch -D %s'."), branchname, branchname);
+ return -1;
+ }
+ return 0;
+}
+
static int delete_branches(int argc, const char **argv, int force, int kinds,
int quiet)
{
- struct commit *rev, *head_rev = NULL;
+ struct commit *head_rev = NULL;
unsigned char sha1[20];
char *name = NULL;
const char *fmt;
@@ -206,17 +224,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
continue;
}
- rev = lookup_commit_reference(sha1);
- if (!rev) {
- error(_("Couldn't look up commit object for '%s'"), name);
- ret = 1;
- continue;
- }
-
- if (!force && !branch_merged(kinds, bname.buf, rev, head_rev)) {
- error(_("The branch '%s' is not fully merged.\n"
- "If you are sure you want to delete it, "
- "run 'git branch -D %s'."), bname.buf, bname.buf);
+ if (check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
+ force)) {
ret = 1;
continue;
}
--
1.7.12
^ permalink raw reply related
* Re: BUG when trying to delete symbolic refs
From: René Scharfe @ 2012-10-18 11:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johan Herland, Git mailing list, Miklos Vajna
In-Reply-To: <7vr4oytn4q.fsf@alter.siamese.dyndns.org>
Am 16.10.2012 18:09, schrieb Junio C Hamano:
> Having said all that, I think your patch is going in the right
> direction. If somebody had a symbolic ref in refs/heads/, the
> removal should remove it, not the pointee, which may not even
> exist. Does "branch -d sym" work correctly with your patch when
> refs/heads/sym is pointing at something that does not exist?
No, it doesn't, neither with nor without the patch. But we can make it
work, and also address a UI issue. This series starts with two patches
that only move code around, then follows the patch you commented on, a
patch addressing dangling symrefs and finally a change to the way
deleted symrefs are reported by git branch.
branch: factor out check_branch_commit()
branch: factor out delete_branch_config()
branch: delete symref branch, not its target
branch: skip commit checks when deleting symref branches
branch: show targets of deleted symrefs, not sha1s
builtin/branch.c | 75 ++++++++++++++++++++++++++++++++++++-------------------
t/t3200-branch.sh | 19 ++++++++++++++
2 files changed, 68 insertions(+), 26 deletions(-)
--
1.7.12
^ permalink raw reply
* Re: [PATCH 0/6] Bring "format-patch --notes" closer to a real feature
From: Nguyen Thai Ngoc Duy @ 2012-10-18 11:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1350539128-21577-1-git-send-email-gitster@pobox.com>
On Thu, Oct 18, 2012 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This replaces the earlier "wip" with a real thing.
>
> We never advertised the "--notes" option to format-patch (or
> anything related to the pretty format options for that matter)
> because the behaviour of these options was whatever they happened to
> do, not what they were designed to do.
Stupid question: does "git am" recreate notes from "format-patch
--notes" output? If it does not, should it? I think it could be a nice
way of copying notes from one machine to another, but not enabled by
default (iow "am --notes").
--
Duy
^ permalink raw reply
* [PATCHv2 2/2] filter-branch: use git-sh-setup's ident parsing functions
From: Jeff King @ 2012-10-18 10:33 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Ilya Basin, git
In-Reply-To: <20121018102627.GA17543@sigill.intra.peff.net>
This saves us some code, but it also reduces the number of
processes we start for each filtered commit. Since we can
parse both author and committer in the same sed invocation,
we save one process. And since the new interface avoids tr,
we save 4 processes.
It also avoids using "tr", which has had some odd
portability problems reported with from Solaris's xpg6
version.
We also tweak one of the tests in t7003 to double-check that
we are properly exporting the variables (because test-lib.sh
exports GIT_AUTHOR_NAME, it will be automatically exported
in subprograms. We override this to make sure that
filter-branch handles it properly itself).
Signed-off-by: Jeff King <peff@peff.net>
---
This fixes the missing exports from v1. There's no changes needed to
patch 1.
git-filter-branch.sh | 46 +++++++++++++---------------------------------
t/t7003-filter-branch.sh | 5 +++--
2 files changed, 16 insertions(+), 35 deletions(-)
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 178e453..5314249 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -64,37 +64,19 @@ set_ident () {
eval "$functions"
-# When piped a commit, output a script to set the ident of either
-# "author" or "committer
+finish_ident() {
+ # Ensure non-empty id name.
+ echo "case \"\$GIT_$1_NAME\" in \"\") GIT_$1_NAME=\"\${GIT_$1_EMAIL%%@*}\" && export GIT_$1_NAME;; esac"
+ # And make sure everything is exported.
+ echo "export GIT_$1_NAME"
+ echo "export GIT_$1_EMAIL"
+ echo "export GIT_$1_DATE"
+}
set_ident () {
- lid="$(echo "$1" | tr "[A-Z]" "[a-z]")"
- uid="$(echo "$1" | tr "[a-z]" "[A-Z]")"
- pick_id_script='
- /^'$lid' /{
- s/'\''/'\''\\'\'\''/g
- h
- s/^'$lid' \([^<]*\) <[^>]*> .*$/\1/
- s/'\''/'\''\'\'\''/g
- s/.*/GIT_'$uid'_NAME='\''&'\''; export GIT_'$uid'_NAME/p
-
- g
- s/^'$lid' [^<]* <\([^>]*\)> .*$/\1/
- s/'\''/'\''\'\'\''/g
- s/.*/GIT_'$uid'_EMAIL='\''&'\''; export GIT_'$uid'_EMAIL/p
-
- g
- s/^'$lid' [^<]* <[^>]*> \(.*\)$/@\1/
- s/'\''/'\''\'\'\''/g
- s/.*/GIT_'$uid'_DATE='\''&'\''; export GIT_'$uid'_DATE/p
-
- q
- }
- '
-
- LANG=C LC_ALL=C sed -ne "$pick_id_script"
- # Ensure non-empty id name.
- echo "case \"\$GIT_${uid}_NAME\" in \"\") GIT_${uid}_NAME=\"\${GIT_${uid}_EMAIL%%@*}\" && export GIT_${uid}_NAME;; esac"
+ parse_ident_from_commit author AUTHOR committer COMMITTER
+ finish_ident AUTHOR
+ finish_ident COMMITTER
}
USAGE="[--env-filter <command>] [--tree-filter <command>]
@@ -320,10 +302,8 @@ while read commit parents; do
git cat-file commit "$commit" >../commit ||
die "Cannot read commit $commit"
- eval "$(set_ident AUTHOR <../commit)" ||
- die "setting author failed for commit $commit"
- eval "$(set_ident COMMITTER <../commit)" ||
- die "setting committer failed for commit $commit"
+ eval "$(set_ident <../commit)" ||
+ die "setting author/committer failed for commit $commit"
eval "$filter_env" < /dev/null ||
die "env filter failed: $filter_env"
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 4d13e10..1e7a209 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -167,10 +167,11 @@ test_expect_success 'author information is preserved' '
test_tick &&
GIT_AUTHOR_NAME="B V Uips" git commit -m bvuips &&
git branch preserved-author &&
- git filter-branch -f --msg-filter "cat; \
+ (sane_unset GIT_AUTHOR_NAME &&
+ git filter-branch -f --msg-filter "cat; \
test \$GIT_COMMIT != $(git rev-parse master) || \
echo Hallo" \
- preserved-author &&
+ preserved-author) &&
test 1 = $(git rev-list --author="B V Uips" preserved-author | wc -l)
'
--
1.8.0.rc3.3.gba630e1
^ permalink raw reply related
* Re: [PATCH 2/2] filter-branch: use git-sh-setup's ident parsing functions
From: Jeff King @ 2012-10-18 10:26 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Ilya Basin, git
In-Reply-To: <20121018102217.GA17332@sigill.intra.peff.net>
On Thu, Oct 18, 2012 at 06:22:17AM -0400, Jeff King wrote:
> Hrm. We _do_ test this in t7003. Weirder, if I instrument filter-branch
> like this:
> [...]
> and run t7003, it shows that the variable is properly exported to the
> sub-process! But I can't seem to figure out why. Confused...
Oh, I see. The variables are already exported by test-lib.sh. You can
see the breakage with:
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 4d13e10..1e7a209 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -167,10 +167,11 @@ test_expect_success 'author information is preserved' '
test_tick &&
GIT_AUTHOR_NAME="B V Uips" git commit -m bvuips &&
git branch preserved-author &&
- git filter-branch -f --msg-filter "cat; \
+ (sane_unset GIT_AUTHOR_NAME &&
+ git filter-branch -f --msg-filter "cat; \
test \$GIT_COMMIT != $(git rev-parse master) || \
echo Hallo" \
- preserved-author &&
+ preserved-author) &&
test 1 = $(git rev-list --author="B V Uips" preserved-author | wc -l)
'
-Peff
^ permalink raw reply related
* Re: [PATCH 2/2] filter-branch: use git-sh-setup's ident parsing functions
From: Jeff King @ 2012-10-18 10:22 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Ilya Basin, git
In-Reply-To: <20121018075429.GD9999@sigill.intra.peff.net>
On Thu, Oct 18, 2012 at 03:54:29AM -0400, Jeff King wrote:
> On Thu, Oct 18, 2012 at 09:49:04AM +0200, Johannes Sixt wrote:
>
> > > - s/.*/GIT_'$uid'_NAME='\''&'\''; export GIT_'$uid'_NAME/p
> >
> > Didn't you lose the export GIT_$uid_{NAME,EMAIL,DATE} parts somewhere on
> > the way?
>
> Yikes, you're right. I didn't even notice, as the test suite still
> passes. I can see how the env filter would still be able to see the
> variables, but the commit-tree call wouldn't. I guess it happens to work
> because we do not test alternate idents in our filter branch tests (IOW,
> we are silently rewriting each commit during the filter-branch, but it
> happens to have the same identities).
Hrm. We _do_ test this in t7003. Weirder, if I instrument filter-branch
like this:
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 69406ae..1b504ce 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -298,8 +298,13 @@ while read commit parents; do
git cat-file commit "$commit" >../commit ||
die "Cannot read commit $commit"
+ echo >&2 pre: $GIT_AUTHOR_NAME
+ sh -c 'echo >&2 pre, subshell: $GIT_AUTHOR_NAME'
+
eval "$(set_ident <../commit)" ||
die "setting author/committer failed for commit $commit"
+ echo >&2 post: $GIT_AUTHOR_NAME
+ sh -c 'echo >&2 post, subshell: $GIT_AUTHOR_NAME'
eval "$filter_env" < /dev/null ||
die "env filter failed: $filter_env"
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 4d13e10..ce57fc5 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -174,6 +174,8 @@ test_expect_success 'author information is preserved' '
test 1 = $(git rev-list --author="B V Uips" preserved-author | wc -l)
'
+test_done
+
test_expect_success "remove a certain author's commits" '
echo i > i &&
test_tick &&
and run t7003, it shows that the variable is properly exported to the
sub-process! But I can't seem to figure out why. Confused...
-Peff
^ permalink raw reply related
* Re: [PATCH 0/6] Bring "format-patch --notes" closer to a real feature
From: Jeff King @ 2012-10-18 10:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1350539128-21577-1-git-send-email-gitster@pobox.com>
On Wed, Oct 17, 2012 at 10:45:22PM -0700, Junio C Hamano wrote:
> We never advertised the "--notes" option to format-patch (or
> anything related to the pretty format options for that matter)
> because the behaviour of these options was whatever they happened to
> do, not what they were designed to do.
>
> It had a few obvious glitches:
>
> * The notes section was appended immediately after the log message,
> and then the three-dash line was added. Such a supplimental
> material should come after the three-dash line.
>
> * The logic to append a new sign-off with "format-patch --signoff"
> worked on the message after the note was added, which made the
> detection of existing sign-off lines incorrect.
>
> This updates the handling of "--notes" option to correct these, in
> an attempt to bring it closer to a real feature.
I just read through the whole series. Aside from the tangent about
strbuf_detach, I didn't see anything wrong. Thanks for moving this
forward.
-Peff
^ permalink raw reply
* [PATCH] strbuf: always return a non-NULL value from strbuf_detach
From: Jeff King @ 2012-10-18 10:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20121018074921.GC9999@sigill.intra.peff.net>
On Thu, Oct 18, 2012 at 03:49:21AM -0400, Jeff King wrote:
> This last line seems like it is caused by a bug in the strbuf API.
> Detaching an empty string will sometimes get you NULL and sometimes not.
> For example, this:
>
> struct strbuf foo = STRBUF_INIT;
> strbuf_detach(&foo, NULL);
>
> will return NULL. But this:
>
> struct strbuf foo = STRBUF_INIT;
> strbuf_addstr(&foo, "bar");
> strbuf_reset(&foo);
> strbuf_detach(&foo, NULL);
>
> will get you a zero-length string. Which just seems insane to me. The
> whole point of strbuf_detach is that you do not have to care about the
> internal representation. It should probably always return a newly
> allocated zero-length string.
> [...]
> It's possible that switching it would create bugs elsewhere (there are
> over 100 uses of strbuf_detach, so maybe somebody really does want this
> NULL behavior), but I tend to think it is just as likely to be fixing
> undiscovered bugs.
So I just read through all 108 grep hits for strbuf_detach. In almost
every case, the NULL return is not triggerable, because we do _some_
strbuf operation. And even if it is empty, like strbuf_read from an
empty source, or strbuf_addstr on an empty string, we still end up
calling `strbuf_grow(sb, 0)`, which will allocate.
There are a few cases where there are code paths where we really might
not add anything to the strbuf, and changing strbuf_detach would have an
impact:
In log.c:cmd_format_patch, creating the rev.extra_headers string from
the individual headers currently ends up NULL when you have no such
headers. But it ends up not mattering if we have NULL or an empty
string, since all code paths just end up appending it to our headers
anyway, and the empty string is a noop.
In commit.c:read_commit_extra_header_lines, a commit without a value
will get a NULL value instead of the empty string. But we end up not
dereferencing the NULL, because it just gets fed to add_extra_header
later, which will only look at the value if its length parameter is
non-zero. So it is built to expect the current behavior, but would be
fine with a switch.
In http-push.c:xml_entities, we will return NULL if fed an empty
string. You can dereference NULL by doing "git http-push ''", although
on glibc systems it will not segfault, because we feed the NULL to
printf, which converts it to "(null)".
In http-backend.c:get_parameters, we call url_decode_parameter_* to
look at the contents of $QUERY_STRING. These functions can return
NULL via strbuf_detach if fed an empty string. We are ready to handle
a NULL value like "empty=&other=value". But not an empty name, like
"one=1&&two=2" (note the bogus double-& which yields an empty
parameter). You can get a segfault by sending that to a smart-http
server.
Probably more detail than you wanted, but I'm pretty confident now that
we should switch it, and that it won't cause any regressions. Patch is
below.
-- >8 --
Subject: strbuf: always return a non-NULL value from strbuf_detach
The current behavior is to return NULL when strbuf did not
actually allocate a string. This can be quite surprising to
callers, though, who may feed the strbuf from arbitrary data
and expect to always get a valid value.
In most cases, it does not make a difference because calling
any strbuf function will cause an allocation (even if the
function ends up not inserting any data). But if the code is
structured like:
struct strbuf buf = STRBUF_INIT;
if (some_condition)
strbuf_addstr(&buf, some_string);
return strbuf_detach(&buf, NULL);
then you may or may not return NULL, depending on the
condition. This can cause us to segfault in http-push
(when fed an empty URL) and in http-backend (when an empty
parameter like "foo=bar&&" is in the $QUERY_STRING).
This patch forces strbuf_detach to allocate an empty
NUL-terminated string when it is called on a strbuf that has
not been allocated.
I investigated all call-sites of strbuf_detach. The majority
are either not affected by the change (because they call a
strbuf_* function unconditionally), or can handle the empty
string just as easily as NULL.
Signed-off-by: Jeff King <peff@peff.net>
---
strbuf.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/strbuf.c b/strbuf.c
index 0510f76..4b9e30c 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -44,7 +44,9 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
char *strbuf_detach(struct strbuf *sb, size_t *sz)
{
- char *res = sb->alloc ? sb->buf : NULL;
+ char *res;
+ strbuf_grow(sb, 0);
+ res = sb->buf;
if (sz)
*sz = sb->len;
strbuf_init(sb, 0);
--
1.8.0.rc3.3.gba630e1
^ permalink raw reply related
* Re: [PATCH] Add new git-remote-hd helper
From: Matthieu Moy @ 2012-10-18 9:42 UTC (permalink / raw)
To: Felipe Contreras
Cc: Junio C Hamano, Johannes Schindelin, Sverre Rabbelier, git,
Ilari Liusvaara, Daniel Barkalow
In-Reply-To: <CAMP44s0yqBFWFyYx6VdNhq43U1u2S-OPrbKAyMWBGS1n=ffQMg@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
> Basically what I already described:
> 1) You need a non-vanilla version of git
> 2) It's not easy to set up
> 3) It's not even clear which branch you should be using (in case you
> are not using msysgit)
I do not read that as "it does not work", but instead as "no one took
the time to push this code into git.git (although someone did in
msysgit)".
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] Add new git-remote-hd helper
From: Felipe Contreras @ 2012-10-18 9:38 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Sverre Rabbelier, git, Ilari Liusvaara,
Daniel Barkalow
In-Reply-To: <7vbog0m8r0.fsf@alter.siamese.dyndns.org>
On Thu, Oct 18, 2012 at 11:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> As to the functionality you seek: git-remote-hg found in
>>> git://github.com/msysgit/git works. It has the following advantages over
>>> every other solution, including the one proposed in this thread:
>>>
>>> - it works
>>>
>>> - no really, it works
>>
>> Not for me.
>
> Felipe, an argument along this line would not help very much.
>
> Please elaborate a bit to describe what does not work and where you
> are having problems more concretely. Even for people who may want
> to see if they agree with your "It does not work", "Not for me"
> alone is too little for them to try out. Others who may want to and
> are capable of helping you won't know where to start.
Basically what I already described:
1) You need a non-vanilla version of git
2) It's not easy to set up
3) It's not even clear which branch you should be using (in case you
are not using msysgit)
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] Add new git-remote-hd helper
From: Junio C Hamano @ 2012-10-18 9:26 UTC (permalink / raw)
To: Felipe Contreras
Cc: Johannes Schindelin, Sverre Rabbelier, git, Ilari Liusvaara,
Daniel Barkalow
In-Reply-To: <CAMP44s2wOX0-UrTM5t5ewZ4yhOJJU=+SfB9PoTQKdsP4Pzsnpg@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
>> As to the functionality you seek: git-remote-hg found in
>> git://github.com/msysgit/git works. It has the following advantages over
>> every other solution, including the one proposed in this thread:
>>
>> - it works
>>
>> - no really, it works
>
> Not for me.
Felipe, an argument along this line would not help very much.
Please elaborate a bit to describe what does not work and where you
are having problems more concretely. Even for people who may want
to see if they agree with your "It does not work", "Not for me"
alone is too little for them to try out. Others who may want to and
are capable of helping you won't know where to start.
Thanks.
^ permalink raw reply
* Re: [PATCH] Add new git-remote-hd helper
From: Felipe Contreras @ 2012-10-18 9:22 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Sverre Rabbelier, Jeff King, git, Junio C Hamano, Ilari Liusvaara,
Daniel Barkalow
In-Reply-To: <alpine.DEB.1.00.1210181112050.3049@bonsai2>
On Thu, Oct 18, 2012 at 11:13 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Thu, 18 Oct 2012, Felipe Contreras wrote:
>
>> On Thu, Oct 18, 2012 at 8:12 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
>> > On Wed, Oct 17, 2012 at 10:18 PM, Felipe Contreras
>> > <felipe.contreras@gmail.com> wrote:
>> >> Right now I've just added an error when using remote repositories.
>> >> But it seems there's no way around it; if we want to have support for
>> >> remote repos, we need to make a local clone.
>> >
>> > My git-remote-hg does the local clone into .git/ using a hash of the
>> > url (although you could just as well use urlencode, basically any way
>> > to safely use a url as a directory name). Have a look if you want.
>>
>> Can you point to the version you are talking about? I've been checking
>> the remote-hg branch of fingolfin.
>>
>> https://github.com/fingolfin/git/tree/remote-hg/
>
> The code is in https://github.com/msysgit/git/ (Sverre does not have
> enough time to work on remote-hg, and was okay with me hosting it in Git
> for Windows, for all the reasons I mentioned earlier in this thread).
Which branch? I don't see any remote-hg branch.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] Add new git-remote-hd helper
From: Johannes Schindelin @ 2012-10-18 9:13 UTC (permalink / raw)
To: Felipe Contreras
Cc: Sverre Rabbelier, Jeff King, git, Junio C Hamano, Ilari Liusvaara,
Daniel Barkalow
In-Reply-To: <CAMP44s3+FP2N4iEmxzcu_ayUAF+XWmKdBFDCvFvj6D5RQmN+5Q@mail.gmail.com>
Hi,
On Thu, 18 Oct 2012, Felipe Contreras wrote:
> On Thu, Oct 18, 2012 at 8:12 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> > On Wed, Oct 17, 2012 at 10:18 PM, Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> >> Right now I've just added an error when using remote repositories.
> >> But it seems there's no way around it; if we want to have support for
> >> remote repos, we need to make a local clone.
> >
> > My git-remote-hg does the local clone into .git/ using a hash of the
> > url (although you could just as well use urlencode, basically any way
> > to safely use a url as a directory name). Have a look if you want.
>
> Can you point to the version you are talking about? I've been checking
> the remote-hg branch of fingolfin.
>
> https://github.com/fingolfin/git/tree/remote-hg/
The code is in https://github.com/msysgit/git/ (Sverre does not have
enough time to work on remote-hg, and was okay with me hosting it in Git
for Windows, for all the reasons I mentioned earlier in this thread).
Hth,
Johannes
^ permalink raw reply
* Re: [PATCH 3/6] pretty: prepare notes message at a centralized place
From: Jeff King @ 2012-10-18 9:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vfw5cm96a.fsf@alter.siamese.dyndns.org>
On Thu, Oct 18, 2012 at 02:17:01AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > It's possible that switching it would create bugs elsewhere (there are
> > over 100 uses of strbuf_detach, so maybe somebody really does want this
> > NULL behavior), but I tend to think it is just as likely to be fixing
> > undiscovered bugs.
>
> Yeah, I tend to agree.
>
> This "format-patch --notes" is obviously a post 1.8.0 topic, and so
> is the strbuf_detach() clean-up. Let me bookmark this thread in
> case it hasn't been resolved when I came back from my vacation, so
> that I won't forget ;-).
Actually, I have found a few segfaults, one of them remotely triggerable
in http-backend. I think it can probably wait until post-1.8.0 as it
does not have any security implications, though.
Details in a moment.
-Peff
^ permalink raw reply
* Re: [PATCH] Add new git-remote-hd helper
From: Johannes Schindelin @ 2012-10-18 9:10 UTC (permalink / raw)
To: Felipe Contreras
Cc: Sverre Rabbelier, git, Junio C Hamano, Ilari Liusvaara,
Daniel Barkalow
In-Reply-To: <CAMP44s2wOX0-UrTM5t5ewZ4yhOJJU=+SfB9PoTQKdsP4Pzsnpg@mail.gmail.com>
Hi Felipe,
On Thu, 18 Oct 2012, Felipe Contreras wrote:
> On Thu, Oct 18, 2012 at 10:47 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> So your idea of collaboration is accept that your code is awesome, and
> my code sucks, and that I should fix your code, and throw my code to the
> trash, while you do absolutely nothing but complain about the whole
> situation.
I said no such thing. I like to keep things professional and keep emotions
out.
Hth,
Johannes
^ 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