* Re: [FYI] very large text files and their problems.
From: Ian Kumlien @ 2012-02-22 18:39 UTC (permalink / raw)
To: git; +Cc: pclouds
[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]
Seems like i ruined my dovecot config in a recent upgrade - which also
affected my mail... =/
Anyway, it's all fixed now.
from: Nguyen Thai Ngoc Duy <pclouds () gmail ! com>
> On Wed, Feb 22, 2012 at 10:49 PM, Ian Kumlien <pomac@vapor.com> wrote:
> > Hi,
> >
> > We just saw a interesting issue, git compressed a ~3.4 gb project to
> ~57 mb.
>
> How big are those files? How many of them? How often do they change?
This is the initial check in, one of the files is a 3.3 gb text file.
> > But when we tried to clone it on a big machine we got:
> >
> > fatal: Out of memory, malloc failed (tried to allocate
> > 18446744072724798634 bytes)
> >
> > This is already fixed in the 1.7.10 mainline - but it also seems
> like
>
> Does 1.7.9 have this problem?
I've tested with 1.7.9.1, haven't downgraded to test with 1.7.9...
> > git needs to have atleast the same ammount of memory as the largest
> > file free... Couldn't this be worked around?
> >
> > On a (32 bit) machine with 4GB memory - results in:
> > fatal: Out of memory, malloc failed (tried to allocate 3310214313
> bytes)
> >
> > (and i see how this could be a problem, but couldn't it be
> mitigated? or
> > is it bydesign and intended behaviour?)
>
> I think that it's delta resolving that hogs all your memory. If your
> files are smaller than 512M, try lower core.bigFileThreshold. The
> topic jc/split-blob, which stores a big file are several smaller
> pieces, might solve your problem. Unfortunately the topic is not
> complete yet.
the problem here is that there is one file that is exactly: 3310214313
bytes, so it should all be one "blob".
split-blob would be really interesting for several reasons though =)
> --
> Duy
> --
--
Ian Kumlien -- http://demius.net || http://pomac.netswarm.net
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCHv4] git-p4: add initial support for RCS keywords
From: Junio C Hamano @ 2012-02-22 19:29 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Eric Scouten, Pete Wyckoff
In-Reply-To: <20120222125327.GA2292@padd.com>
Pete Wyckoff <pw@padd.com> writes:
>> Improved-by: Pete Wyckoff <pw@padd.com>
>> Signed-off-by: Luke Diamand <luke@diamand.org>
>
> Looks brilliant. Ack. Thanks for suffering through N rounds of
> review. :)
Well, I hate to say that I need to ask another round, to redo this patch
on top of ld/git-p4-expanded-keywords topic that has already been in
'next'; a patch that replaces what is in 'next' will lose fix-ups for
issues I pointed out in the first round that you forgot to follow and were
fixed up locally by me when I queued the existing one.
When working on an improvement to what you have sent out, please make it a
habit of comparing your result with what are already queued, even when the
earlier patches are still in 'pu'. They often are polished with trivial
improvements (both to the patch and the log message) based on review
comments from people when they are queued, which you do not want to lose.
Thanks.
^ permalink raw reply
* [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Thomas Rast @ 2012-02-22 19:34 UTC (permalink / raw)
To: git; +Cc: Jannis Pohlmann
In-Reply-To: <a795f6dca5e7c3fc5f9212becda4a46116c502b7.1329939233.git.trast@student.ethz.ch>
The first part of the bundle header contains the boundary commits, and
could be approximated by
# v2 git bundle
$(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-)
git-bundle actually spawns exactly this rev-list invocation, and does
the grepping internally.
There was a subtle bug in the latter step: it used fgets() with a
1024-byte buffer. If the user has sufficiently long subjects (e.g.,
by not adhering to the git oneline-subject convention in the first
place), the 'oneline' format can easily overflow the buffer. fgets()
then returns the rest of the line in the next call(s). If one of
these remaining parts started with '-', git-bundle would mistakenly
insert it into the bundle thinking it was a boundary commit.
Fix it by using strbuf_getwholeline() instead, which handles arbitrary
line lengths correctly.
Note that on the receiving side in parse_bundle_header() we were
already using strbuf_getwholeline_fd(), so that part is safe.
Reported-by: Jannis Pohlmann <jannis.pohlmann@codethink.co.uk>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
bundle.c | 14 +++++++-------
t/t5704-bundle.sh | 17 +++++++++++++++++
2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/bundle.c b/bundle.c
index 313de42..0dbd174 100644
--- a/bundle.c
+++ b/bundle.c
@@ -234,7 +234,7 @@ int create_bundle(struct bundle_header *header, const char *path,
const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
const char **argv_pack = xmalloc(6 * sizeof(const char *));
int i, ref_count = 0;
- char buffer[1024];
+ struct strbuf buf = STRBUF_INIT;
struct rev_info revs;
struct child_process rls;
FILE *rls_fout;
@@ -266,16 +266,16 @@ int create_bundle(struct bundle_header *header, const char *path,
if (start_command(&rls))
return -1;
rls_fout = xfdopen(rls.out, "r");
- while (fgets(buffer, sizeof(buffer), rls_fout)) {
+ while (strbuf_getwholeline(&buf, rls_fout, '\n') != EOF) {
unsigned char sha1[20];
- if (buffer[0] == '-') {
- write_or_die(bundle_fd, buffer, strlen(buffer));
- if (!get_sha1_hex(buffer + 1, sha1)) {
+ if (buf.len > 0 && buf.buf[0] == '-') {
+ write_or_die(bundle_fd, buf.buf, buf.len);
+ if (!get_sha1_hex(buf.buf + 1, sha1)) {
struct object *object = parse_object(sha1);
object->flags |= UNINTERESTING;
- add_pending_object(&revs, object, buffer);
+ add_pending_object(&revs, object, buf.buf);
}
- } else if (!get_sha1_hex(buffer, sha1)) {
+ } else if (!get_sha1_hex(buf.buf, sha1)) {
struct object *object = parse_object(sha1);
object->flags |= SHOWN;
}
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 4ae127d..7c2f307 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -59,4 +59,21 @@ test_expect_success 'empty bundle file is rejected' '
'
+# If "ridiculous" is at least 1004 chars, this traps a bug in old
+# versions where the resulting 1025-char line (with --pretty=oneline)
+# was longer than a 1024-char buffer
+test_expect_success 'ridiculously long subject in boundary' '
+
+ : > file4 &&
+ test_tick &&
+ git add file4 &&
+ printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - &&
+ test_commit fifth &&
+ git bundle create long-subject-bundle.bdl HEAD^..HEAD &&
+ git fetch long-subject-bundle.bdl &&
+ sed -n "/^-/{p;q}" long-subject-bundle.bdl > boundary &&
+ grep "^-$_x40 " boundary
+
+'
+
test_done
--
1.7.9.1.430.g4998543
^ permalink raw reply related
* [PATCH 1/2] bundle: put strbuf_readline_fd in strbuf.c with adjustments
From: Thomas Rast @ 2012-02-22 19:34 UTC (permalink / raw)
To: git; +Cc: Jannis Pohlmann
In-Reply-To: <4F451259.7010304@codethink.co.uk>
The comment even said that it should eventually go there. While at
it, match the calling convention and name of the function to the
strbuf_get*line family. So it now is strbuf_getwholeline_fd.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
I was left looking for strbuf_readline_fd() for a while, then tried to
look for the readline() from the libc that it would presumably match.
Hence this cleanup.
bundle.c | 21 ++-------------------
strbuf.c | 16 ++++++++++++++++
strbuf.h | 1 +
3 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/bundle.c b/bundle.c
index b8acf3c..313de42 100644
--- a/bundle.c
+++ b/bundle.c
@@ -23,23 +23,6 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,
list->nr++;
}
-/* Eventually this should go to strbuf.[ch] */
-static int strbuf_readline_fd(struct strbuf *sb, int fd)
-{
- strbuf_reset(sb);
-
- while (1) {
- char ch;
- ssize_t len = xread(fd, &ch, 1);
- if (len <= 0)
- return len;
- strbuf_addch(sb, ch);
- if (ch == '\n')
- break;
- }
- return 0;
-}
-
static int parse_bundle_header(int fd, struct bundle_header *header,
const char *report_path)
{
@@ -47,7 +30,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
int status = 0;
/* The bundle header begins with the signature */
- if (strbuf_readline_fd(&buf, fd) ||
+ if (strbuf_getwholeline_fd(&buf, fd, '\n') ||
strcmp(buf.buf, bundle_signature)) {
if (report_path)
error("'%s' does not look like a v2 bundle file",
@@ -57,7 +40,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
}
/* The bundle header ends with an empty line */
- while (!strbuf_readline_fd(&buf, fd) &&
+ while (!strbuf_getwholeline_fd(&buf, fd, '\n') &&
buf.len && buf.buf[0] != '\n') {
unsigned char sha1[20];
int is_prereq = 0;
diff --git a/strbuf.c b/strbuf.c
index ff0b96b..5135d59 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -383,6 +383,22 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
return 0;
}
+int strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term)
+{
+ strbuf_reset(sb);
+
+ while (1) {
+ char ch;
+ ssize_t len = xread(fd, &ch, 1);
+ if (len <= 0)
+ return EOF;
+ strbuf_addch(sb, ch);
+ if (ch == term)
+ break;
+ }
+ return 0;
+}
+
int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
{
int fd, len;
diff --git a/strbuf.h b/strbuf.h
index fbf059f..3effaa8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -116,6 +116,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
extern int strbuf_getline(struct strbuf *, FILE *, int);
+extern int strbuf_getwholeline_fd(struct strbuf *, int, int);
extern void stripspace(struct strbuf *buf, int skip_comments);
extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
--
1.7.9.1.430.g4998543
^ permalink raw reply related
* Re: [PATCH 0/8 v6] diff --stat: use the full terminal width
From: Junio C Hamano @ 2012-02-22 19:41 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, Michael J Gruber, pclouds, j.sixt
In-Reply-To: <4F44D084.7030308@in.waw.pl>
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> by "scales monotonically with the change count" I meant with two
> different commits.
Because of the scaling, the same 300-line change to the same file will be
shown with different number of +- depending on the amount of change to
other files in the same commit. It is not something we should be worried
about to begin with, unless you are going to read all commits in advance
and find necessary length of the name part, and also maxchange across
these commits to align things across commits, which of course would not
mesh well with our desire to stream output whenever possible.
> [7.3/8] limit graph part to 40 columns
Read the problem you are trying to solve again (from your 3/8):
>> If commits changing a lot of lines are displayed in a wide terminal
>> window (200 or more columns), and the +- graph would use the full
>> width, the output would look bad. Messages wrapped to about 80 columns
>> would be interspersed with very long +- lines.
If your name part needs only 10-20 columns and you are used to seeing
graph bars that are 60-70 columns long on your standard 80-column
terminal, you may see it ugly if the same short names are followed by
graph bars that are 180-190 columns (i.e. "very long +-"), when the
messages still fit 80 columns on the standard terminal, which is a width
that can comfortably be scanned with horizontal movement of human eyes. I
would understand that concern, although I do not personally think it is a
big deal.
Imagine if your name part were 125 columns wide (200 * 5/8), and also
imagine you drew the same graph bars as before, i.e. using 60-70 columns.
That line will fill almost the full terminal width, and will extend beyond
the right edge of the message block, but you are not showing "very long +-
lines" anymore. The length of the line comes mostly from overly long
names, carrying more information than before (because your patch lets us
show larger parts of such a long name on wider terminal), and without
losing much information from the graph part.
Now imagine if your patch only needed 10-20 columns for the names, and you
are showing the same change on an 80-column terminal with your 40-column
cap. You will introduce a new problem with "very short +-" lines instead,
as the "diff --stat" block will be much shorter than the surrounding text.
You fixed a bug in the original partitioning with 1/8, which was caused by
a code that capped the length of the graph part too early before taking
the width necessary to show the name into account; the bug resulted in a
graph that did not fill the allotted space fully to the right, even though
the user wanted to give full width to the graph.
The ugliness avoidance against "very long +-" becomes an issue only on
wide terminals; doing the same 40-column cap on non-wide terminals is to
re-introduce the same "cap the graph width too early, ending up with a
graph that is narrower than desired" bug you fixed. Why do you keep
insisting on that broken approach?
It might be just the matter of raising the artificial cap to much higher
than 40-column (say, 80-column). A possible alternative may be to declare
that the perceived ugliness is the user's problem of having overly wide
terminal in the first place and do without any such cap.
Either is fine, but regressing output on 80-column terminal when showing a
patch with short filenames and large changes is unacceptable, not because
I personally use 80-col terminal myself (I don't---mine is a bit wider but
not 200), but because it changes behaviour from the old code without any
good justification to do so.
^ permalink raw reply
* Re: contrib/git-move-refs.py not exists in cvs2svn-2.3.0
From: Michael Haggerty @ 2012-02-22 20:05 UTC (permalink / raw)
To: supadhyay; +Cc: git
In-Reply-To: <1329909939769-7308023.post@n2.nabble.com>
On 02/22/2012 12:25 PM, supadhyay wrote:
> I have run the cvs2git tool to migrate my CVS repository to git. I am able
> to finish successfully. Now as the last recommended steps from
> http://cvs2svn.tigris.org/cvs2git.html , I found to run
> "contrib/git-move-refs.py" but in my cvs2svn-2.3.0 I can not see any file
> name like "git-move-refs.ph", I have one file "git-move-tags.py". Is this
> is the same file ?
This program was added in the trunk version of cvs2svn; it is not in
release 2.3.0. I recommend using the trunk version for your conversion
anyway, as it includes a number of improvements (some git-relevant) and
it would be released already except for a few loose ends that I haven't
gotten back to.
cvs2svn/cvs2git/cvs2etc is a project separate from git and it has its
own mailing list to which your conversion questions should probably be
directed:
users@cvs2svn.tigris.org
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH] contrib: added git-diffall
From: Tim Henigan @ 2012-02-22 20:14 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
In-Reply-To: <CAJDDKr72rPqp1z-12Ht3Q2UaeUVFutKwoOgkD1G+SbhsBs+p1A@mail.gmail.com>
On Wed, Feb 22, 2012 at 4:15 AM, David Aguilar <davvid@gmail.com> wrote:
> On Tue, Feb 21, 2012 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Tim Henigan <tim.henigan@gmail.com> writes:
>>
>> David, any idea on this?
>
> I don't see any bash-isms there myself, either. We should keep this
> stuff without bash-isms.
> I haven't had time to read these patches in depth yet but will try to
> read the re-roll.
>
> Can we ask the github user to elaborate on what exactly was erroring out?
> Does dash not handle || inside $()? We can only make wild guesses
> without their help.
>
> The only hint from the pull request is "silent exit with no results".
> Do we do that?
> There are a few code paths where we do "exit 1" but that's only under
> error conditions.
>
> We haven't had any reports about git-mergetool/difftool, which use
> these functions...
> are we certain the problem was not some other bash-isms in the script?
I have not heard back from the user, but I tested on Ubuntu earlier
today. I found that when using an older version of the script
(fbedb7a in the GitHub repo), I could repeat the error. It is the
'git-diffall' script that fails silently, I believe due to a bash-ism
that was fixed in a subsequent commit.
In my latest local version of the script, I have switched back to
#!/bin/sh. It runs successfully, so I will squash the change into v2
of the patch.
For what it is worth, since that bug was originally reported, I have
been running "checkbashisms" [1] on the git-diffall script. That
utility reports that the script is clean.
-Tim
[1]: http://sourceforge.net/projects/checkbaskisms/
^ permalink raw reply
* Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()
From: Felipe Contreras @ 2012-02-22 20:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vvcn5qecl.fsf@alter.siamese.dyndns.org>
On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> There's a lot of code that can be consolidated there, and will be useful
>> for next patches.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>> remote.c | 59 ++++++++++++++++++++++++++++++-----------------------------
>> 1 files changed, 30 insertions(+), 29 deletions(-)
>>
>> diff --git a/remote.c b/remote.c
>> index 55d68d1..019aafc 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1110,10 +1110,11 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
>> return errs;
>> }
>>
>> -static const struct refspec *check_pattern_match(const struct refspec *rs,
>> - int rs_nr,
>> - const struct ref *src)
>> +static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
>> + int send_mirror, const struct refspec **ret_pat)
>> {
>
> For a change that not just adds parameters but removes an existing one,
> this is way under-described with neither in-code comment nor log message.
But it doesn't. src is renamed to ref.
>> + const struct refspec *pat;
>> + char *name;
>> int i;
>> int matching_refs = -1;
>> for (i = 0; i < rs_nr; i++) {
>> @@ -1123,14 +1124,31 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,
>> continue;
>> }
>>
>> - if (rs[i].pattern && match_name_with_pattern(rs[i].src, src->name,
>> - NULL, NULL))
>> - return rs + i;
>> + if (rs[i].pattern) {
>> + const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
>> + if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
>> + matching_refs = i;
>> + break;
>
> We used to discard what match_name_with_pattern() finds out by matching a
> wildcard refspec against the ref by passing two NULLs. This updates the
> code to capture what destination ref ref->name is mapped to, by using the
> same logic as the original and only caller, i.e. 'foo' without destination
> maps to the same 'foo' destination, 'foo:bar' maps to the named 'bar'.
>
> This function is not used by fetching side of the codepath, so we do not
> have to worry about its need to use different dst_side selection logic
> (i.e. 'foo' without destination maps to "do not store anywhere other than
> FETCH_HEAD"). Good.
I actually didn't parse a lot of that.
>> + }
>> + }
>> }
>> -...
>> + if (matching_refs == -1)
>> return NULL;
>> +
>> + pat = rs + matching_refs;
>> + if (pat->matching) {
>> + /*
>> + * "matching refs"; traditionally we pushed everything
>> + * including refs outside refs/heads/ hierarchy, but
>> + * that does not make much sense these days.
>> + */
>> + if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
>> + return NULL;
>> + name = xstrdup(ref->name);
>> + }
>
> So you are moving some code from what the sole caller of this function
> does after calling us, and that is where the new parameters come from.
> And by doing so, you do not have to run the same match_name_with_pattern()
> again. OK.
Indeed.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Junio C Hamano @ 2012-02-22 20:22 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Jannis Pohlmann
In-Reply-To: <fa1553d59714fd89fdab1bf54af19ac631a30a8c.1329939233.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> The first part of the bundle header contains the boundary commits, and
> could be approximated by
>
> # v2 git bundle
> $(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-)
>
> git-bundle actually spawns exactly this rev-list invocation, and does
> the grepping internally.
>
> There was a subtle bug in the latter step: it used fgets() with a
> 1024-byte buffer. If the user has sufficiently long subjects (e.g.,
> by not adhering to the git oneline-subject convention in the first
> place), the 'oneline' format can easily overflow the buffer. fgets()
> then returns the rest of the line in the next call(s). If one of
> these remaining parts started with '-', git-bundle would mistakenly
> insert it into the bundle thinking it was a boundary commit.
>
> Fix it by using strbuf_getwholeline() instead, which handles arbitrary
> line lengths correctly.
>
> Note that on the receiving side in parse_bundle_header() we were
> already using strbuf_getwholeline_fd(), so that part is safe.
Thanks for diagnosing this, but I wonder if it even needs --pretty=oneline
to begin with, except for debugging purposes.
Do we ever use the subject string read from the rev-list output in any
way?
In other words, I am wondering if the right patch to minimally fix the
issue starting from older releases is something along this line instead:
diff --git a/bundle.c b/bundle.c
index b8acf3c..339dbb0 100644
--- a/bundle.c
+++ b/bundle.c
@@ -248,7 +248,7 @@ int create_bundle(struct bundle_header *header, const char *path,
static struct lock_file lock;
int bundle_fd = -1;
int bundle_to_stdout;
- const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
+ const char **argv_boundary = xmalloc((argc + 3) * sizeof(const char *));
const char **argv_pack = xmalloc(6 * sizeof(const char *));
int i, ref_count = 0;
char buffer[1024];
@@ -271,11 +271,10 @@ int create_bundle(struct bundle_header *header, const char *path,
init_revisions(&revs, NULL);
/* write prerequisites */
- memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));
+ memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));
argv_boundary[0] = "rev-list";
argv_boundary[1] = "--boundary";
- argv_boundary[2] = "--pretty=oneline";
- argv_boundary[argc + 2] = NULL;
+ argv_boundary[argc + 1] = NULL;
memset(&rls, 0, sizeof(rls));
rls.argv = argv_boundary;
rls.out = -1;
^ permalink raw reply related
* Re: Problems with unrecognized headers in git bundles
From: Øyvind A. Holm @ 2012-02-22 20:25 UTC (permalink / raw)
To: Jannis Pohlmann; +Cc: git
In-Reply-To: <4F451259.7010304@codethink.co.uk>
On 22 February 2012 17:05, Jannis Pohlmann wrote:
> Hi,
>
> creating bundles from some repositories seems to lead to bundles with
> incorrectly formatted headers, at least with git >= 1.7.2. When
> cloning from such bundles, git prints the following error/warning:
>
> $ git clone perl-clone.bundle perl-clone
> Cloning into 'perl-clone'...
> warning: unrecognized header: --work around mangled archname on...
>
> This can be reproduced easily with git from any version >= 1.7.2 or
> from master, using the following steps:
>
> git clone git://perl5.git.perl.org/perl.git perl
> GIT_DIR=perl/.git git bundle create perl-clone.bundle --all
> git clone perl-clone.bundle perl-clone
>
> The content of the bundle is:
>
> # v2 git bundle
> -- work around mangled archname on win32 while finding...
> 39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/heads/blead
> 39ec54a59ce332fc44e553f4e5eeceef88e8369e refs/remotes/origin/HEAD
Have researched this a bit, and I've found that all git versions back to
when git-bundle was introduced (around v1.5.4) produces the same invalid
line. The culprit is commit 3e8148feadabd0d0b1869fcc4d218a6475a5b0bc in
perl.git, branch 'maint-5.005'. The log message of that commit contains
email headers, maybe that's the reason git bundle gets confused?
Øyvind
^ permalink raw reply
* Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Thomas Rast @ 2012-02-22 20:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Rast, git, Jannis Pohlmann
In-Reply-To: <7vlinuaaab.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Thomas Rast <trast@student.ethz.ch> writes:
>
>> # v2 git bundle
>> $(git rev-list --pretty=oneline --boundary <ARGS> | grep ^-)
>>
>> git-bundle actually spawns exactly this rev-list invocation, and does
>> the grepping internally.
>>
>> There was a subtle bug in the latter step: it used fgets() with a
>> 1024-byte buffer. If the user has sufficiently long subjects (e.g.,
>> by not adhering to the git oneline-subject convention in the first
>> place), the 'oneline' format can easily overflow the buffer.
>
> Thanks for diagnosing this, but I wonder if it even needs --pretty=oneline
> to begin with, except for debugging purposes.
>
> Do we ever use the subject string read from the rev-list output in any
> way?
>
> In other words, I am wondering if the right patch to minimally fix the
> issue starting from older releases is something along this line instead:
Not sure. The only use I could think of would be to google for the
subjects, in the hope of finding some repository that has the commit you
are looking for. Other than that...
In any case the --pretty=oneline is very deliberate, as we can see from
the commit below. It just doesn't give a reason :-)
commit 239296770dae75e21c179733785731ec6ffae1f5
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date: Fri Feb 23 03:17:51 2007 +0100
git-bundle: record commit summary in the prerequisite data
diff --git a/builtin-bundle.c b/builtin-bundle.c
index 191ec55..d74afaa 100644
--- a/builtin-bundle.c
+++ b/builtin-bundle.c
@@ -267,7 +267,7 @@ static int create_bundle(struct bundle_header *header, const char *path,
int argc, const char **argv)
{
int bundle_fd = -1;
- const char **argv_boundary = xmalloc((argc + 3) * sizeof(const char *));
+ const char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));
const char **argv_pack = xmalloc(4 * sizeof(const char *));
int pid, in, out, i, status;
char buffer[1024];
@@ -282,10 +282,11 @@ static int create_bundle(struct bundle_header *header, const char *path,
write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
/* write prerequisites */
- memcpy(argv_boundary + 2, argv + 1, argc * sizeof(const char *));
+ memcpy(argv_boundary + 3, argv + 1, argc * sizeof(const char *));
argv_boundary[0] = "rev-list";
argv_boundary[1] = "--boundary";
- argv_boundary[argc + 1] = NULL;
+ argv_boundary[2] = "--pretty=oneline";
+ argv_boundary[argc + 2] = NULL;
out = -1;
pid = fork_with_pipe(argv_boundary, NULL, &out);
if (pid < 0)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply related
* Re: Ambiguous reference weirdness
From: Jeff King @ 2012-02-22 20:38 UTC (permalink / raw)
To: Phil Hord; +Cc: git, Ramkumar Ramachandra, Junio C Hamano, Jonathan Nieder
In-Reply-To: <CABURp0r+3gOQ3rq_ubv=uEoU=XmtYs=etfT4W2Lb9M0LBrikWg@mail.gmail.com>
On Wed, Feb 22, 2012 at 09:48:23AM -0500, Phil Hord wrote:
> > What is 1147? Is it supposed to be a partial sha1, or is it a ref you
> > have?
>
> 1147 was a typo. It was a Gerritt changeset ID I forgot to expand.
> When I type "git cherry-pick 1147<TAB>", autocompletion expands this
> for me to "git cherry-pick origin/changes/47/1147/1". Except in this
> case I misfired the TAB and got the weird "BUG:" report. But I didn't
> see the same problem with other invalid refs, so I went searching for
> the variants and to see what caused it.
Ah. It's a little annoying that Gerrit names refs that look kind of
like partial sha1s. But I guess most of the time it's not that big a
deal (it is only because you were using the partial Gerrit ref with tab
completion). And I don't think we're about to change how Gerrit names
things. :)
> > Have you looked at the object that it resolves to? I suspect it is the
> > partial sha1 of a non-commit object. E.g.:
>
> All of these examples were run in current git.git, so you can try them
> yourself if needed. But I did figure out that 1147 resolves to a
> blob, and that's apparently the difference between these three:
Yeah. I figured that after reading more, but didn't go back and revise
the first part of my message. I was able to easily get the same results
as you (actually, in my git.git, 1147 is ambiguous because I happen to
have some extra refs, but it was easy to replicate with a fresh clone).
> $ git cherry-pick 1147
> fatal: BUG: expected exactly one commit from walk
>
> $ git cherry-pick 1146
> error: short SHA1 1146 is ambiguous.
> error: short SHA1 1146 is ambiguous.
> fatal: ambiguous argument '1146': unknown revision or path not in the
> working tree.
> Use '--' to separate paths from revisions
>
> $ git cherry-pick 114333
> fatal: ambiguous argument '114333': unknown revision or path not in
> the working tree.
> Use '--' to separate paths from revisions
>
> I consider the first two responses to be UI bugs. The second one is
> minor (the twice-reported error message), and the first one is pretty
> rude. I would expect all three to report the same conclusion,
> "fatal: ambiguous argument 'XXXXX': unknown revision or path not in
> the working tree." But the first one doesn't.
Right. Because cherry-pick's logic is:
if the arguments do not resolve to any objects at all
complain of unknown revision
if the resolved object is not a single commit
die of BUG
And I think you just want to collapse those two conditions into a single
conditional, which seems reasonable (the error message does say "unknown
revision", not "unknown object". It's a little more complicated than
that, because you are crossing a boundary between reusable library code
and cherry-pick specific code.
As for the doubled "ambiguous" warning, I'm not sure what the cause is
for that. I suspect it is because cherry-pick tries to parse one way (as
a revision walk specifier, like "a..b"), and then parses again (as a
single commit) when that fails, due to historical reasons. Obviously
it would be nice to see that improved, but I suspect it will be annoying
to do so; the error message is emitted by a low-level, and cherry-pick
has no idea that it has happened (it only sees "this didn't result in
parsing anything").
> Thanks. I'm not familiar with the {tree} syntax -- in fact I'd like
> to find a dictionary for all the reference spelling variants -- but
> this is elucidating.
See "git help rev-parse", section "Specifying Revisions".
> > In the cherry-pick case, the code is checking the right thing, but the
> > message is horrible. It is not a bug, but merely unexpected input, and
> > it should provide a usage message.
>
> Bug is too strong a word in one sense, but from the user perspective I
> consider this "horrible message" a bug.
Sorry, I meant git is wrong to use the word "BUG". For us, die("BUG:
...") is the equivalent of an assert. It means something totally
unexpected happened that should never happen, and therefore there is a
bug in git. But this is not a bug in git (well, it is, but not that
kind). It is a totally reasonable and expected malformed input that git
should diagnose and report correctly.
So it is a bug that git says "BUG". It should say "you gave me objects,
but they are not revisions" or something similar.
> > I think checkout has the same "is this a path or a revision" ambiguity
> > to resolve. But rather than be explicit that you might have meant "114"
> > as a tree, the error message assumes you meant a path. That might be
> > worth improving, similar to the above example.
> >
> > Again, you can disambiguate with:
> >
> > $ git checkout -- 1147
> > error: pathspec '1147' did not match any file(s) known to git.
> >
> > $ git checkout 1147 --
> > fatal: reference is not a tree: 1147
> >
> >> $ git checkout 1147
> >> fatal: reference is not a tree: 1147
>
> Yes, I understand this. This was a typo and it was ambiguous. But
> shouldn't we tell the user the same thing when encountering the same
> failure?
No, because there are really three cases here:
1. The user told us 1147 is a path.
2. The user told us 1147 is a tree.
3. The user did not tell us which, and we must guess.
We guess (1) if the name does not resolve at all, and (2) otherwise. And
the error message only indicates the particular guess we made.
So while I don't think they should all have the same error message,
there are two improvements I can see:
a. If we are guessing, and 1147 resolves but is _not_ a tree, we
could guess path instead.
b. When our guess results in an error, it would be better to be
explicit about the fact that we guessed (i.e., say "this is neither
a tree nor a path that git knows about", similar to git-log).
> > I think the outcomes are all working as intended, but the error messages
> > could stand to be improved.
>
> Yes, I agree. I only meant to complain about the error messages, not
> the results. Thanks for the discussion. I'll try to look for where
> these come from and see if they can be improved within reason.
Great. I look forward to it.
-Peff
^ permalink raw reply
* Re: Problems with unrecognized headers in git bundles
From: Øyvind A. Holm @ 2012-02-22 20:40 UTC (permalink / raw)
To: Jannis Pohlmann; +Cc: git
In-Reply-To: <CAA787rm4c1zYgQJ3kP5=ujpEK1Dda9+h_P3BBmg2yX2eZca=TA@mail.gmail.com>
On 22 February 2012 21:25, Øyvind A. Holm <sunny@sunbase.org> wrote:
> On 22 February 2012 17:05, Jannis Pohlmann wrote:
> > creating bundles from some repositories seems to lead to bundles
> > with incorrectly formatted headers, at least with git >= 1.7.2. When
> > cloning from such bundles, git prints the following error/warning:
> >
> > $ git clone perl-clone.bundle perl-clone
> > Cloning into 'perl-clone'...
> > warning: unrecognized header: --work around mangled archname on...
>
> Have researched this a bit, and I've found that all git versions back
> to when git-bundle was introduced (around v1.5.4) produces the same
> invalid line. The culprit is commit
> 3e8148feadabd0d0b1869fcc4d218a6475a5b0bc in perl.git, branch
> 'maint-5.005'. The log message of that commit contains email headers,
> maybe that's the reason git bundle gets confused?
...or maybe because the log message doesn't contain any empty lines, so
they're joined together into an insanely long line. I've seen this
behaviour before, in git-am or git-apply, I think. Anyway, when the
bundle doesn't contain this commit, the line is not present.
Øyvind
^ permalink raw reply
* Re: [PATCH] remote-curl: Fix push status report when all branches fail
From: Jeff King @ 2012-02-22 20:40 UTC (permalink / raw)
To: Shawn Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <CAJo=hJsFDrt4rsxVAnx86bxZDY3yfWc1=GDd8opUU+9z7esLnw@mail.gmail.com>
On Wed, Feb 22, 2012 at 07:22:10AM -0800, Shawn O. Pearce wrote:
> > + /*
> > + * Ignore write errors; there's nothing we can do,
> > + * since we're about to close the pipe anyway. And the
> > + * most likely error is EPIPE due to the helper dying
> > + * to report an error itself.
> > + */
> > + sigchain_push(SIGPIPE, SIG_IGN);
> > + xwrite(data->helper->in, "\n", 1);
> > + sigchain_pop(SIGPIPE);
> [...]
>
> This sounds right to me. Its unfortunate that we missed the error
> status output when we built the remote helper protocol, but your patch
> above might be the best we can do now.
>
> Eh, well, actually we could have the helper advertise a new capability
> that can be enabled to return exit status. That is a much bigger
> change, and even if we do it for remote-curl (since that is in tree
> and easy to update) we still need your patch for the same race
> condition for out of tree helpers (which Google actually has so I care
> about out of tree helpers too).
I don't think it's worth a new capability. This is one of those "it
would be nice if it were designed that way from day one" cases, but it
wasn't. And while this is a minor hack, I don't think it has any
functional downsides. So adding a new capability on top of the hack just
makes things more complex.
I'll re-send the patch with a stand-alone commit message.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 3/3] push: add 'prune' option
From: Felipe Contreras @ 2012-02-22 20:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vobsxqebz.fsf@alter.siamese.dyndns.org>
On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> This will allow us to remove refs from the remote that have been removed
>> locally.
>
> Can you enhance this a bit more to summarize the gist of what the semantic
> of this new feature is, perhaps like this:
>
> After pushing refs, "git push --prune" will remove refs from the
> remote that existed before the push and would have been pushed
> from us if we had some local refs that would have matched the
> refspecs used. For example,
>
> $ git push --prune remote refs/heads/*:refs/remotes/repo1/*
>
> will push all local branches in our repository to refs with
> corresponding names under refs/remotes/repo1/ at the remote, and
> removes remote's refs in refs/remotes/repo1/ that no longer have
> corresponding local branches in our repository. The refs at the
> remote outside refs/remotes/repo1/ are not affected.
>
> In order to alley the worries raised in the previous discussion, something
> to the effect of the last sentence above is crucial to have, I would think.
OK.
>> --- a/builtin/push.c
>> +++ b/builtin/push.c
>> @@ -261,6 +261,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>> OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
>> TRANSPORT_PUSH_SET_UPSTREAM),
>> OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
>> + OPT_BIT('p', "prune", &flags, "prune locally removed refs",
>> + TRANSPORT_PUSH_PRUNE),
>
> Please refrain from squatting on a short-and-sweet one letter option
> before this new feature proves to be popular and useful in a few cycles,
> especially when there already is a long option that begins with 'p'.
OK.
>> OPT_END()
>> };
>>
>> diff --git a/remote.c b/remote.c
>> index 019aafc..0900bb5 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1111,7 +1111,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
>> }
>>
>> static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
>> - int send_mirror, const struct refspec **ret_pat)
>> + int send_mirror, int dir, const struct refspec **ret_pat)
>
> Can we name this a bit better? I first thought "Huh? directory?", and had
> to scratch my head, wondering if it is an offset into refs/heads/* string
> or something....
OK.
>> {
>> const struct refspec *pat;
>> char *name;
>> @@ -1126,7 +1126,12 @@ static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref
>>
>> if (rs[i].pattern) {
>> const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
>> - if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
>> + int match;
>> + if (dir == 0)
>> + match = match_name_with_pattern(rs[i].src, ref->name, dst_side, &name);
>> + else
>> + match = match_name_with_pattern(dst_side, ref->name, rs[i].src, &name);
>
> ....until the code told us that it is some sort of direction of the
> matching. A symbolic constant or two would be even better.
>
> Originally this funcion was fed a list of refs in the source (i.e. on our
> end, as this is only used in 'push') and matched them against the source
> side of the refspec, rs[i].src, to see under what name the destination
> side will store it (i.e. give dst_side as value to find out the result in
> &name). This patch adds a new caller, who feeds a list of refs in the
> destination (i.e. on the remote end) to find out how they map to the names
> on our end (i.e. source). So "direction" is not necessarily incorrect; it
> is the direction this function maps the names (either src-to-dst for the
> original caller, or dst-to-src for the new caller).
>
> Perhaps "enum map_direction { SRC_TO_DST, DST_TO_SRC }" or something?
I think only FROM_SRC, FROM_DST is more than enough to figure it out.
>> + if (match) {
>> matching_refs = i;
>> break;
>> }
>
> So what is the updated semantics of this function? Is it still
> appropriate to name it "check_pattern_match()"?
>
> It seems that by now this does a lot more than just "check if a pattern
> matches". Since your patch 2/3, it is a function that finds out the
> refname in the remote that the given one refspec would try to update, and
> with this patch, it can also map in the reverse direction, given the list
> of remote refs, finding out which local ref a refspec would use to update
> them.
>
> At the same time, to reduce risk of future breakage, we probably should
> rename this function to make it clear that this function is to be only
> used by the push side.
>
> Perhaps rename this to "map_push_refs()" or something in the patch 2/3?
I think get_ref_match() would be more appropriate because we are
acting on a specific (singular) ref, and the primary thing we care
about is getting the peer name, based on the refspec match, which we
might want as a return value.
>> @@ -1173,6 +1178,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
>> struct refspec *rs;
>> int send_all = flags & MATCH_REFS_ALL;
>> int send_mirror = flags & MATCH_REFS_MIRROR;
>> + int send_prune = flags & MATCH_REFS_PRUNE;
>> int errs;
>> static const char *default_refspec[] = { ":", NULL };
>> struct ref *ref, **dst_tail = tail_ref(dst);
>> @@ -1193,7 +1199,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
>> if (ref->peer_ref)
>> continue;
>>
>> - dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);
>> + dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 0, &pat);
>> if (!dst_name)
>> continue;
>>
>> @@ -1220,6 +1226,23 @@ int match_push_refs(struct ref *src, struct ref **dst,
>> free_name:
>> free(dst_name);
>> }
>> + if (send_prune) {
>> + /* check for missing refs on the remote */
>> + for (ref = *dst; ref; ref = ref->next) {
>> + char *src_name;
>> +
>> + if (ref->peer_ref)
>> + /* We're already sending something to this ref. */
>> + continue;
>> +
>> + src_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 1, NULL);
>> + if (src_name) {
>> + if (!find_ref_by_name(src, src_name))
>> + ref->peer_ref = try_explicit_object_name("");
>
> Yuck. You do not want it to "try" as its name says. You just want to
> trigger its "delete" codepath.
>
> Please extract the body of "if (!*name) { ... }" block out of that
> function into a separate helper function, i.e.
>
> static struct ref *deleted_ref(void)
> {
> struct ref *ref = alloc_ref("(delete)");
> hashclr(ref->new_sha1);
> return ref;
> }
>
> then update try_explicit_...() to call it, and call the same helper here.
>
> This is not for runtime efficiency; feeding a constant to a function that
> says try_foo() or check_bar() that makes decision on the parameter only to
> trigger a partial codepath hurts readability.
All right.
>> + free(src_name);
>> + }
>> + }
>> + }
>> if (errs)
>> return -1;
>> return 0;
>> diff --git a/remote.h b/remote.h
>> index b395598..341142c 100644
>> --- a/remote.h
>> +++ b/remote.h
>> @@ -145,7 +145,8 @@ int branch_merge_matches(struct branch *, int n, const char *);
>> enum match_refs_flags {
>> MATCH_REFS_NONE = 0,
>> MATCH_REFS_ALL = (1 << 0),
>> - MATCH_REFS_MIRROR = (1 << 1)
>> + MATCH_REFS_MIRROR = (1 << 1),
>> + MATCH_REFS_PRUNE = (1 << 2),
>> };
>>
>> /* Reporting of tracking info */
>> diff --git a/transport.c b/transport.c
>> index cac0c06..c20267c 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -1028,6 +1028,8 @@ int transport_push(struct transport *transport,
>> match_flags |= MATCH_REFS_ALL;
>> if (flags & TRANSPORT_PUSH_MIRROR)
>> match_flags |= MATCH_REFS_MIRROR;
>> + if (flags & TRANSPORT_PUSH_PRUNE)
>> + match_flags |= MATCH_REFS_PRUNE;
>
> Does it make sense to specify --prune when --mirror is in effect? If so,
> how would it behave differently from a vanilla --mirror? If not, should
> it be detected as an error?
Probably doesn't make sense, should be an error.
> I couldn't infer from the context shown in the patch, but how in general
> does this new feature interact with the codepath for --mirror?
>
>> if (match_push_refs(local_refs, &remote_refs,
>> refspec_nr, refspec, match_flags)) {
>> diff --git a/transport.h b/transport.h
>> index 059b330..5d30328 100644
>> --- a/transport.h
>> +++ b/transport.h
>> @@ -101,6 +101,7 @@ struct transport {
>> #define TRANSPORT_PUSH_MIRROR 8
>> #define TRANSPORT_PUSH_PORCELAIN 16
>> #define TRANSPORT_PUSH_SET_UPSTREAM 32
>> +#define TRANSPORT_PUSH_PRUNE 64
>> #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
>
> Hrm...?
Probably some rebase mistake =/
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Junio C Hamano @ 2012-02-22 20:50 UTC (permalink / raw)
To: Thomas Rast; +Cc: Thomas Rast, git, Jannis Pohlmann
In-Reply-To: <87hayivcmm.fsf@thomas.inf.ethz.ch>
Thomas Rast <trast@inf.ethz.ch> writes:
> In any case the --pretty=oneline is very deliberate, as we can see from
> the commit below. It just doesn't give a reason :-)
Thanks for digging and I hope everybody on this list who would ever send a
patch learns the importance of describing _why_ in the log from this
lesson.
I'll amend your 2/2 with a note that keeping the subject material is not
justified and we may want to remove (or at least reduce) it in the future.
^ permalink raw reply
* Re: [PATCH 1/2] bundle: put strbuf_readline_fd in strbuf.c with adjustments
From: Jeff King @ 2012-02-22 20:51 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Jannis Pohlmann
In-Reply-To: <a795f6dca5e7c3fc5f9212becda4a46116c502b7.1329939233.git.trast@student.ethz.ch>
On Wed, Feb 22, 2012 at 08:34:22PM +0100, Thomas Rast wrote:
> The comment even said that it should eventually go there. While at
> it, match the calling convention and name of the function to the
> strbuf_get*line family. So it now is strbuf_getwholeline_fd.
> [...]
> bundle.c | 21 ++-------------------
> strbuf.c | 16 ++++++++++++++++
> strbuf.h | 1 +
Nit: no update to Documentation/technical/api-strbuf.txt.
You might want to also mention that this will read() one byte at a time,
and is therefore only a good idea if you really care about the position
of the fd. Otherwise, fdopen() + strbuf_getwholeline() is much more
efficient.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Jeff King @ 2012-02-22 20:55 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Jannis Pohlmann
In-Reply-To: <fa1553d59714fd89fdab1bf54af19ac631a30a8c.1329939233.git.trast@student.ethz.ch>
On Wed, Feb 22, 2012 at 08:34:23PM +0100, Thomas Rast wrote:
> +# If "ridiculous" is at least 1004 chars, this traps a bug in old
> +# versions where the resulting 1025-char line (with --pretty=oneline)
> +# was longer than a 1024-char buffer
> +test_expect_success 'ridiculously long subject in boundary' '
> +
> + : > file4 &&
> + test_tick &&
> + git add file4 &&
> + printf "abcdefghijkl %s\n" $(seq 1 100) | git commit -F - &&
Seq is not portable. I usually use either
perl -le "print for (1..100)"
or just do:
z16=zzzzzzzzzzzzzzzz
z256=$z16$z16$z16$z16$z16$z16$z16$z16
z1024=$z256$z256$z256$z256$z256$z256$z256$z256
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 3/3] push: add 'prune' option
From: Junio C Hamano @ 2012-02-22 20:56 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <CAMP44s3+XCM1E_AtW1yGifmGoGSkFSpSTaFbbMffz+hmUzWahw@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
>> Perhaps "enum map_direction { SRC_TO_DST, DST_TO_SRC }" or something?
>
> I think only FROM_SRC, FROM_DST is more than enough to figure it out.
Yeah, as you can tell from my "or something", as long as the name makes
the direction clear, I don't care too much about the exact spelling.
>> Perhaps rename this to "map_push_refs()" or something in the patch 2/3?
>
> I think get_ref_match() would be more appropriate because we are
> acting on a specific (singular) ref, and the primary thing we care
> about is getting the peer name, based on the refspec match, which we
> might want as a return value.
Again, as long as the name makes it clear this is meant only for "push"
and never be used for "fetch", I am fine with it. One way to make it sure
is to have a substring "_push" somewhere in that name.
> Probably some rebase mistake =/
Thanks; I was wondering if there is something subtle unexplained going on.
^ permalink raw reply
* Re: [PATCH 2/2] bundle: use a strbuf to scan the log for boundary commits
From: Jeff King @ 2012-02-22 21:00 UTC (permalink / raw)
To: Thomas Rast; +Cc: Junio C Hamano, Thomas Rast, git, Jannis Pohlmann
In-Reply-To: <87hayivcmm.fsf@thomas.inf.ethz.ch>
On Wed, Feb 22, 2012 at 09:25:53PM +0100, Thomas Rast wrote:
> > Thanks for diagnosing this, but I wonder if it even needs --pretty=oneline
> > to begin with, except for debugging purposes.
> >
> > Do we ever use the subject string read from the rev-list output in any
> > way?
> >
> > In other words, I am wondering if the right patch to minimally fix the
> > issue starting from older releases is something along this line instead:
>
> Not sure. The only use I could think of would be to google for the
> subjects, in the hope of finding some repository that has the commit you
> are looking for. Other than that...
Or because the bundle creator (which may even be you on a different day)
did not correctly guess at the negative refs when specifying a cutoff.
Assuming you no longer have access to the original repo (not
unreasonable, since you are using a bundle), the messages could help you
understand where the error occurred.
Of course, once you figure out "aha! I expected the bundle to include
foo, but it is actually a cutoff point. I should have said XYZ^ as the
cutoff", I'm not sure what you do then. It's not like there is an easy
way to salvage the data that is in the bundle.
So I think it is a debugging aid, but one that ultimately doesn't help
you that much.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()
From: Junio C Hamano @ 2012-02-22 21:02 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Jeff King
In-Reply-To: <CAMP44s2HUG_ocHBaVpcsZHWMf2Tww+=bVun5H9+S5EGkoiJHRQ@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
>> For a change that not just adds parameters but removes an existing one,
>> this is way under-described with neither in-code comment nor log message.
>
> But it doesn't. src is renamed to ref.
That is exactly why I mentioned that this is a "chance" in the part of my
message you did not quote ;-).
It was a chance for you to learn how the thought process of others may
work or not work well, depending on how well the log message at the top
prepares their mind before they start to read the patch, by looking at how
one reader (me) tried to figure your patch out.
Anything that I said in the message you are responding to that tempts you
to say "No you are reading wrong" is an indication that the patch did not
do a good job to help the reader understand what you wanted to do.
^ permalink raw reply
* Re: [PATCH] contrib: added git-diffall
From: Junio C Hamano @ 2012-02-22 21:06 UTC (permalink / raw)
To: Tim Henigan; +Cc: David Aguilar, git
In-Reply-To: <CAFouetgyYRKT7h1h4hv40Kxp=ibq1Hw-1mzFe6DsyAUknXKyYQ@mail.gmail.com>
Tim Henigan <tim.henigan@gmail.com> writes:
> I have not heard back from the user, but I tested on Ubuntu earlier
> today. I found that when using an older version of the script
> (fbedb7a in the GitHub repo), I could repeat the error. It is the
> 'git-diffall' script that fails silently, I believe due to a bash-ism
> that was fixed in a subsequent commit.
Thanks.
That would explain it, especially given that you've been doing:
> For what it is worth, since that bug was originally reported, I have
> been running "checkbashisms" [1] on the git-diffall script. That
> utility reports that the script is clean.
The checkbashisms scritp is interesting ;-)
I do not get much of the comment removal and string munging that happen
before it really start to check the contents for bash-isms, but most of
the check logic contained within the %foo_bashisms seem to be looking for
the right kind of mistakes people who are too used to bash make very
often.
^ permalink raw reply
* Re: [PATCH 2/4] completion: normalize increment/decrement style
From: Junio C Hamano @ 2012-02-22 22:05 UTC (permalink / raw)
To: Philip Jägenstedt
Cc: git, SZEDER Gábor, Felipe Contreras, Teemu Likonen
In-Reply-To: <1329901093-24106-3-git-send-email-philip@foolip.org>
Philip Jägenstedt <philip@foolip.org> writes:
> The style used for incrementing and decrementing variables was fairly
> inconsistenty and was normalized to use x++, or ((x++)) in contexts
> where the former would otherwise be interpreted as a command. This is a
> bash-ism, but for obvious reasons this script is already bash-specific.
>
> Signed-off-by: Philip Jägenstedt <philip@foolip.org>
I am not sure if the kind of changes seen in the following hunks are
necessary but I do not care too deeply about it. Assuming that the
change does not make the emulation by zsh unhappy, will apply.
Please stop me if somebody has issues with this rewrite.
Thanks.
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c63a408..1903bc9 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -178,10 +178,8 @@ __git_ps1_show_upstream ()
> for commit in $commits
> do
> case "$commit" in
> - "<"*) let ++behind
> - ;;
> - *) let ++ahead
> - ;;
> + "<"*) ((behind++)) ;;
> + *) ((ahead++)) ;;
> esac
> done
> count="$behind $ahead"
> @@ -739,7 +737,7 @@ __git_complete_remote_or_refspec ()
> local cur_="$cur" cmd="${words[1]}"
> local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
> if [ "$cmd" = "remote" ]; then
> - c=$((++c))
> + ((c++))
> fi
> while [ $c -lt $cword ]; do
> i="${words[c]}"
^ permalink raw reply
* Re: [PATCHv2 2/8] gitweb: Option for filling only specified info in fill_project_list_info
From: Jakub Narebski @ 2012-02-22 22:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vfwe6ng8u.fsf@alter.siamese.dyndns.org>
On Mon, 20 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > Introduce project_info_needs_filling($pr, $key[, \%fill_only]), which
>
> That's "project_info_needs_filling($pr, @key[, \%fill_only])", isn't it?
Yes it is. I'm sorry, I have missed that this needs fixing from the
first version of patch series. Since then I have noticed that in few
cases we fill two fields at once, so we have to ask about two fields
at once as well.
> > +# entry for given @keys needs filling if at least one of interesting keys
> > +# in list is not present in %$project_info; key is interesting if $fill_only
> > +# is not passed, or is empty (all keys are interesting in both of those cases),
> > +# or if key is in $fill_only hash
> > +#
> > +# USAGE:
> > +# * project_info_needs_filling($project_info, 'key', ...)
> > +# * project_info_needs_filling($project_info, 'key', ..., \%fill_only)
> > +# where %fill_only = map { $_ => 1 } @fill_only;
>
> The reason this spells bad to me is that it gives the readers (and anybody
> who may want to touch gitweb code) that the caller has _two_ ways to say
> the same thing.
>
> If a caller is interested in finding out $key in $project_info, it can
> either (1) pass $key as one of the earlier parameters to the function and
> not pass any \%fill_only, or (2) pass $key and pass \%fill_only but after
> making sure \%fill_only also has $key defined.
>
> And if the caller passes \%fill_only, it has to remember that it needs to
> add $key to it; otherwise the earlier request "I want $key" is ignored by
> the function.
Yes, you are right. All of this is caused by fill_project_list_info()
trying to do *two* things, instead of doing one thing and doing it well,
as is Unix philosophy. It tried to check both that key is needed and
that key is wanted.
> Why is it a good thing to have such a convoluted API?
Thanks for a sanity check.
> Is it that "fill_project_list_info" is called by multiple callers, and
> they do not necessarily need to see all the fields that can be filled by
> the function (namely, age, age_string, descr, descr_long, owner, ctags and
> category)? If that is the case, I must say that the approach to put the
> set filtering logic inside project_info_needs_filling function smells like
> a bad API design.
You are right.
> In other words, wouldn't a callchain that uses a more natural set of API
> functions be like this?
>
> # the top-level caller that is interested only in these two fields
> fill_project_list_info($projlist, 'age', 'owner');
>
> # in fill_project_list_info()
> my $projlist = shift;
> my %wanted = map { $_ => 1 } @_;
>
> foreach my $pr (@$projlist) {
> if (project_info_needs_filling($pr,
> filter_set(\%wanted, 'age', 'age_string'))) {
That or
if (project_info_needs_filling($pr, 'age', 'age_string') &&
project_info_is_wanted(\%wanted, 'age', 'age_string')) {
and in your case there is no duplication of field names.
filter_set() is simply intersection of two sets, but it treats empty
%wanted as a full set.
> ... get @activity ...
> ($pr->{'age'}, $pr->{'age_string'}) = @activity;
> }
> ...
> }
>
> That way, "needs_filling" has to do one and only one thing well: it checks
> if any of the fields it was asked is missing and answers. And a generic
> set filtering helper that takes a filtering hashref and an array can be
> used later outside of the "needs_filling" function.
Right.
> > sub project_info_needs_filling {
> > + my $fill_only = ref($_[-1]) ? pop : undef;
> > my ($project_info, @keys) = @_;
> >
> > # return List::MoreUtils::any { !exists $project_info->{$_} } @keys;
> > foreach my $key (@keys) {
> > - if (!exists $project_info->{$key}) {
> > + if ((!$fill_only || !%$fill_only || $fill_only->{$key}) &&
> > + !exists $project_info->{$key}) {
> > return 1;
> > }
> > }
> > return;
> > }
> >
> > -
>
> Shouldn't the previous patch updated not to add this blank line in the
> first place?
O.K.
> > # fills project list info (age, description, owner, category, forks)
>
> Is this enumeration up-to-date? If we cannot keep it up-to-date, it may
> make sense to show only a few as examples and add ellipses.
O.K.
> > # for each project in the list, removing invalid projects from
> > -# returned list
> > +# returned list, or fill only specified info (removing invalid projects
> > +# only when filling 'age').
>
> It is unclear what the added clause wants to say; especially, the link
> between the mention of 'age' and 'only when' is unclear. Is asking of
> 'age' what makes a request a notable exception to remove invalid projects?
> Or is asking to partially fill fields what triggers the removal of invalid
> projects, and you mentioned 'age' as an example?
>
> If it is the former (I am guessing it is), it would be much clearer to
> say:
>
> Invalid projects are removed from the returned list if and only if you
> ask 'age' to be filled, because of such and such reasons.
O.K.
Sidenote: the reason is that to check that project is valid you really
have to run git command that requires repository in it[1]. Only when
filling 'age' and 'age_string' using git_get_last_activity() we always
run git command.
[1] Gitweb actually uses --git-dir=<PATH> parameter to git wrapper.
--
Jakub Narebski
Poland
^ permalink raw reply
* Bug report: "git-merge --ff" should fail if branches have diverged
From: Sean Gies @ 2012-02-22 21:51 UTC (permalink / raw)
To: git@vger.kernel.org
Git developers, I have a small bug report:
When I specify the "--ff" option to git-merge, I expect it to perform a fast-forward merge or none at all. If the branches have diverged and a fast-forward cannot be done, I expect the command to fail. With git 1.7.6, if "git-merge -ff" cannot fast-forward, it falls back to creating the merge commit I did not want.
Thank you,
-Sean
^ 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