* Re: pread() over NFS (again) [1.5.5.4]
From: Christian Holtje @ 2008-06-26 21:36 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20080626210556.GZ11793@spearce.org>
On Jun 26, 2008, at 5:05 PM, Shawn O. Pearce wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
>> "Shawn O. Pearce" <spearce@spearce.org> writes:
>>> Christian Holtje <docwhat@gmail.com> wrote:
>>>> I have read all the threads on git having trouble with pread()
>>>> and I
>>>> didn't see anything to help.
>>> ...
>>>> Receiving objects: 100% (253/253), 5.27 MiB | 9136 KiB/s, done.
>>>> fatal: cannot pread pack file: No such file or directory
>>>> fatal: index-pack failed
>>>>
>>>> The end of the strace looks like so:
>>>> pread(3, "", 205, 1373) = 0
>>>> write(2, "fatal: cannot pread pack file: N"..., 57) = 57
>>>
>>> Hmmph. So pread for a length of 205 can return 0 on NFS? Is this
>>> a transient error? If so, perhaps a patch like this might help:
> ...
>>> The file shouldn't be short unless someone truncated it, or there
>>> is a bug in index-pack. Neither is very likely, but I don't think
>>> we would want to retry pread'ing the same block forever.
>>
>> I don't think we would want to retry even once. Return value of 0
>> from
>> pread is defined to be an EOF, isn't it?
>
> Indeed, it is defined to be EOF, but EOF here makes no sense.
>
> We have a file position we saw once before as the start of a delta.
> We wrote it down to disk. We want to go back and open it up, as
> we have the base decompressed and in memory and need to compute
> the SHA-1 of the object that resides at this offset.
>
> And *wham* we get an EOF. Where there should be data. Where we
> know there is data.
>
> I'm open to the idea that index-pack has a bug, but I doubt it.
> We shovel hundreds of megabytes through that on a daily basis
> across all of the git users, and nobody ever sees it crash out
> with an EOF in the middle of an object it knows to be present.
> Except poor Christian on NFS.
>
> Actually, I think the last time someone reported something like this
> in Git it turned out to be an NFS kernel bug. I didn't quote it
> in my reply to him, but I think he did say this was a linux client,
> linux server.
I did see the threads that talked about NFS, but I couldn't find any
matching messages in the linux kernel mailing list. If someone can
give me a pointer to where this picked up on other mailing lists (say,
via a URL) then I'll take that back to IT as a reason to upgrade.
Would it be a bug in the client or server? I'd assume client...
The other NFS/pread() email thread was also a client of linux 2.6.9....
Ciao!
^ permalink raw reply
* Re: pread() over NFS (again) [1.5.5.4]
From: Junio C Hamano @ 2008-06-26 22:04 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Christian Holtje, git
In-Reply-To: <20080626210556.GZ11793@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> We have a file position we saw once before as the start of a delta.
> We wrote it down to disk. We want to go back and open it up, as
> we have the base decompressed and in memory and need to compute
> the SHA-1 of the object that resides at this offset.
>
> And *wham* we get an EOF. Where there should be data. Where we
> know there is data.
We have written that earlier in the same process? Are we playing games
with mixed mmap() and pread()? Is fsync() or msync() or unmap/remap
needed?
> Actually, I think the last time someone reported something like this
> in Git it turned out to be an NFS kernel bug. I didn't quote it
> in my reply to him, but I think he did say this was a linux client,
> linux server.
This is getting into the area Linus would immediately know the answer to,
but he is away for this week.
^ permalink raw reply
* Re: pread() over NFS (again) [1.5.5.4]
From: Shawn O. Pearce @ 2008-06-26 22:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Christian Holtje, git
In-Reply-To: <7vod5nq2dy.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
> > We have a file position we saw once before as the start of a delta.
> > We wrote it down to disk. We want to go back and open it up, as
> > we have the base decompressed and in memory and need to compute
> > the SHA-1 of the object that resides at this offset.
> >
> > And *wham* we get an EOF. Where there should be data. Where we
> > know there is data.
>
> We have written that earlier in the same process? Are we playing games
> with mixed mmap() and pread()? Is fsync() or msync() or unmap/remap
> needed?
Yes, we wrote the very same file earlier in the process.
index-pack _used_ to use a mixed write/mmap game. Today it uses
write, followed by later pread. No mmap. We had major performance
issues on Mac OS X with mmap, not to mention the coherency issues
that can arise from using it. So index-pack is mmap free[*1*].
> > Actually, I think the last time someone reported something like this
> > in Git it turned out to be an NFS kernel bug. I didn't quote it
> > in my reply to him, but I think he did say this was a linux client,
> > linux server.
>
> This is getting into the area Linus would immediately know the answer to,
> but he is away for this week.
Yea, I was expecting a reply from him, but that explains it.
*1* There is usage of mmap in index-pack, but only to access
_existing_ objects and packs from the object store.
--
Shawn.
^ permalink raw reply
* Re: Searching all git objects
From: Sam G. @ 2008-06-26 22:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tarmigan, Paul Mackerras, Shawn O. Pearce, git
In-Reply-To: <7v7icdvwml.fsf@gitster.siamese.dyndns.org>
git-lost-found actually did it... she had git-stashed it a number of
times throughout development, so through some grepping of the lost-
found objects, we were able to find a very close version. What a
relief! Thanks to everyone for your help!
-Sam
On Jun 25, 2008, at 6:00 PM, Junio C Hamano wrote:
> "Sam G." <ceptorial@comcast.net> writes:
>
>> And, just in case, is there any way to
>> search raw objects for text, either commit text, file name or
>> content?
>> Thanks very much!
>
> Or run "git lost-found" and grep for blobs in .git/lost-found/other/*
^ permalink raw reply
* Re: [PATCH v2] pre-commit hook should ignore carriage returns at EOL
From: Junio C Hamano @ 2008-06-26 22:33 UTC (permalink / raw)
To: Christian Holtje; +Cc: Alex Riesen, git
In-Reply-To: <635350D7-2CC0-4FA6-BB74-CE9ED930ECE8@gmail.com>
Christian Holtje <docwhat@gmail.com> writes:
>> I suggested using "diff --check" (and possibly teaching "diff --check"
>> other things the scripted example checks, such as conflict markers),
>> which would know to honor the line endings specified per path via
>> gitattributes(5), instead of building on top of the big Perl script,
>> and I
>> had an impression that you agreed to the approach.
>
> I'm completely confused how gitattributes and core.autocrlf interact,
> etc.
Here is a series I just cooked up so that we can remove the whole Perl
script and replace it by adding --check to "diff-index" used there.
The first three are code clean-ups and the last two implements necessary
new features to "diff --check". The whole series somewhat depend on the
fix to 'maint' not to lose the exit status I sent earlier.
[PATCH 1/5] diff --check: explain why we do not care whether old side is binary
[PATCH 2/5] check_and_emit_line(): rename and refactor
[PATCH 3/5] checkdiff: pass diff_options to the callback
[PATCH 4/5] Teach "diff --check" about a new blank lines at end
[PATCH 5/5] diff --check: detect leftover conflict markers
^ permalink raw reply
* [PATCH 1/5] diff --check: explain why we do not care whether old side is binary
From: Junio C Hamano @ 2008-06-26 22:34 UTC (permalink / raw)
To: Christian Holtje; +Cc: Alex Riesen, git
In-Reply-To: <7vk5gbq10p.fsf@gitster.siamese.dyndns.org>
All other codepaths refrain from running textual diff when either the old
or the new side is binary, but this function only checks the new side. I
was almost going to change it to check both, but that would be a bad
change. Explain why to prevent future mistakes.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/diff.c b/diff.c
index 8939423..c00d633 100644
--- a/diff.c
+++ b/diff.c
@@ -1544,8 +1544,9 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
static void builtin_checkdiff(const char *name_a, const char *name_b,
const char *attr_path,
- struct diff_filespec *one,
- struct diff_filespec *two, struct diff_options *o)
+ struct diff_filespec *one,
+ struct diff_filespec *two,
+ struct diff_options *o)
{
mmfile_t mf1, mf2;
struct checkdiff_t data;
@@ -1564,6 +1565,12 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
die("unable to read files to diff");
+ /*
+ * All the other codepaths check both sides, but not checking
+ * the "old" side here is deliberate. We are checking the newly
+ * introduced changes, and as long as the "new" side is text, we
+ * can and should check what it introduces.
+ */
if (diff_filespec_is_binary(two))
goto free_and_return;
else {
--
1.5.6.1.78.gde8d9
^ permalink raw reply related
* [PATCH 2/5] check_and_emit_line(): rename and refactor
From: Junio C Hamano @ 2008-06-26 22:35 UTC (permalink / raw)
To: Christian Holtje; +Cc: Alex Riesen, git
In-Reply-To: <7vk5gbq10p.fsf@gitster.siamese.dyndns.org>
The function name was too bland and not explicit enough as to what it is
checking. Split it into two, and call the one that checks if there is a
whitespace breakage "ws_check()", and call the other one that checks and
emits the line after color coding "ws_check_emit()".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-apply.c | 5 ++---
cache.h | 5 ++---
diff.c | 13 ++++++-------
ws.c | 18 +++++++++++++++---
4 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index c497889..92f0047 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -979,8 +979,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
static void check_whitespace(const char *line, int len, unsigned ws_rule)
{
char *err;
- unsigned result = check_and_emit_line(line + 1, len - 1, ws_rule,
- NULL, NULL, NULL, NULL);
+ unsigned result = ws_check(line + 1, len - 1, ws_rule);
if (!result)
return;
@@ -991,7 +990,7 @@ static void check_whitespace(const char *line, int len, unsigned ws_rule)
else {
err = whitespace_error_string(result);
fprintf(stderr, "%s:%d: %s.\n%.*s\n",
- patch_input_file, linenr, err, len - 2, line + 1);
+ patch_input_file, linenr, err, len - 2, line + 1);
free(err);
}
}
diff --git a/cache.h b/cache.h
index 64ef86e..3dfa53c 100644
--- a/cache.h
+++ b/cache.h
@@ -819,9 +819,8 @@ void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, i
extern unsigned whitespace_rule_cfg;
extern unsigned whitespace_rule(const char *);
extern unsigned parse_whitespace_rule(const char *);
-extern unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
- FILE *stream, const char *set,
- const char *reset, const char *ws);
+extern unsigned ws_check(const char *line, int len, unsigned ws_rule);
+extern void ws_check_emit(const char *line, int len, unsigned ws_rule, FILE *stream, const char *set, const char *reset, const char *ws);
extern char *whitespace_error_string(unsigned ws);
extern int ws_fix_copy(char *, const char *, int, unsigned, int *);
diff --git a/diff.c b/diff.c
index c00d633..52a34ee 100644
--- a/diff.c
+++ b/diff.c
@@ -535,9 +535,9 @@ static void emit_add_line(const char *reset, struct emit_callback *ecbdata, cons
else {
/* Emit just the prefix, then the rest. */
emit_line(ecbdata->file, set, reset, line, ecbdata->nparents);
- (void)check_and_emit_line(line + ecbdata->nparents,
- len - ecbdata->nparents, ecbdata->ws_rule,
- ecbdata->file, set, reset, ws);
+ ws_check_emit(line + ecbdata->nparents,
+ len - ecbdata->nparents, ecbdata->ws_rule,
+ ecbdata->file, set, reset, ws);
}
}
@@ -1153,8 +1153,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
if (line[0] == '+') {
unsigned bad;
data->lineno++;
- bad = check_and_emit_line(line + 1, len - 1,
- data->ws_rule, NULL, NULL, NULL, NULL);
+ bad = ws_check(line + 1, len - 1, data->ws_rule);
if (!bad)
return;
data->status |= bad;
@@ -1162,8 +1161,8 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
fprintf(data->file, "%s:%d: %s.\n", data->filename, data->lineno, err);
free(err);
emit_line(data->file, set, reset, line, 1);
- (void)check_and_emit_line(line + 1, len - 1, data->ws_rule,
- data->file, set, reset, ws);
+ ws_check_emit(line + 1, len - 1, data->ws_rule,
+ data->file, set, reset, ws);
} else if (line[0] == ' ')
data->lineno++;
else if (line[0] == '@') {
diff --git a/ws.c b/ws.c
index ba7e834..24d3e3d 100644
--- a/ws.c
+++ b/ws.c
@@ -117,9 +117,9 @@ char *whitespace_error_string(unsigned ws)
}
/* If stream is non-NULL, emits the line after checking. */
-unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
- FILE *stream, const char *set,
- const char *reset, const char *ws)
+static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
+ FILE *stream, const char *set,
+ const char *reset, const char *ws)
{
unsigned result = 0;
int written = 0;
@@ -213,6 +213,18 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
return result;
}
+void ws_check_emit(const char *line, int len, unsigned ws_rule,
+ FILE *stream, const char *set,
+ const char *reset, const char *ws)
+{
+ (void)ws_check_emit_1(line, len, ws_rule, stream, set, reset, ws);
+}
+
+unsigned ws_check(const char *line, int len, unsigned ws_rule)
+{
+ return ws_check_emit_1(line, len, ws_rule, NULL, NULL, NULL, NULL);
+}
+
/* Copy the line to the buffer while fixing whitespaces */
int ws_fix_copy(char *dst, const char *src, int len, unsigned ws_rule, int *error_count)
{
--
1.5.6.1.78.gde8d9
^ permalink raw reply related
* [PATCH 3/5] checkdiff: pass diff_options to the callback
From: Junio C Hamano @ 2008-06-26 22:36 UTC (permalink / raw)
To: Christian Holtje; +Cc: Alex Riesen, git
In-Reply-To: <7vk5gbq10p.fsf@gitster.siamese.dyndns.org>
We could later use more information from the diff_options.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff.c | 21 +++++++++++----------
1 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/diff.c b/diff.c
index 52a34ee..6bcbe20 100644
--- a/diff.c
+++ b/diff.c
@@ -1136,18 +1136,19 @@ static void free_diffstat_info(struct diffstat_t *diffstat)
struct checkdiff_t {
struct xdiff_emit_state xm;
const char *filename;
- int lineno, color_diff;
+ int lineno;
+ struct diff_options *o;
unsigned ws_rule;
unsigned status;
- FILE *file;
};
static void checkdiff_consume(void *priv, char *line, unsigned long len)
{
struct checkdiff_t *data = priv;
- const char *ws = diff_get_color(data->color_diff, DIFF_WHITESPACE);
- const char *reset = diff_get_color(data->color_diff, DIFF_RESET);
- const char *set = diff_get_color(data->color_diff, DIFF_FILE_NEW);
+ int color_diff = DIFF_OPT_TST(data->o, COLOR_DIFF);
+ const char *ws = diff_get_color(color_diff, DIFF_WHITESPACE);
+ const char *reset = diff_get_color(color_diff, DIFF_RESET);
+ const char *set = diff_get_color(color_diff, DIFF_FILE_NEW);
char *err;
if (line[0] == '+') {
@@ -1158,11 +1159,12 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
return;
data->status |= bad;
err = whitespace_error_string(bad);
- fprintf(data->file, "%s:%d: %s.\n", data->filename, data->lineno, err);
+ fprintf(data->o->file, "%s:%d: %s.\n",
+ data->filename, data->lineno, err);
free(err);
- emit_line(data->file, set, reset, line, 1);
+ emit_line(data->o->file, set, reset, line, 1);
ws_check_emit(line + 1, len - 1, data->ws_rule,
- data->file, set, reset, ws);
+ data->o->file, set, reset, ws);
} else if (line[0] == ' ')
data->lineno++;
else if (line[0] == '@') {
@@ -1557,9 +1559,8 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
data.xm.consume = checkdiff_consume;
data.filename = name_b ? name_b : name_a;
data.lineno = 0;
- data.color_diff = DIFF_OPT_TST(o, COLOR_DIFF);
+ data.o = o;
data.ws_rule = whitespace_rule(attr_path);
- data.file = o->file;
if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
die("unable to read files to diff");
--
1.5.6.1.78.gde8d9
^ permalink raw reply related
* [PATCH 4/5] Teach "diff --check" about a new blank lines at end
From: Junio C Hamano @ 2008-06-26 22:36 UTC (permalink / raw)
To: Christian Holtje; +Cc: Alex Riesen, git
In-Reply-To: <7vk5gbq10p.fsf@gitster.siamese.dyndns.org>
When a patch adds new blank lines at the end, "git apply --whitespace"
warns. This teaches "diff --check" to do the same.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
cache.h | 1 +
diff.c | 17 +++++++++++++++--
t/t4015-diff-whitespace.sh | 6 ++++++
ws.c | 15 +++++++++++++++
4 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 3dfa53c..188428d 100644
--- a/cache.h
+++ b/cache.h
@@ -823,6 +823,7 @@ extern unsigned ws_check(const char *line, int len, unsigned ws_rule);
extern void ws_check_emit(const char *line, int len, unsigned ws_rule, FILE *stream, const char *set, const char *reset, const char *ws);
extern char *whitespace_error_string(unsigned ws);
extern int ws_fix_copy(char *, const char *, int, unsigned, int *);
+extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
/* ls-files */
int pathspec_match(const char **spec, char *matched, const char *filename, int skiplen);
diff --git a/diff.c b/diff.c
index 6bcbe20..f31c721 100644
--- a/diff.c
+++ b/diff.c
@@ -1140,6 +1140,7 @@ struct checkdiff_t {
struct diff_options *o;
unsigned ws_rule;
unsigned status;
+ int trailing_blanks_start;
};
static void checkdiff_consume(void *priv, char *line, unsigned long len)
@@ -1154,6 +1155,10 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
if (line[0] == '+') {
unsigned bad;
data->lineno++;
+ if (!ws_blank_line(line + 1, len - 1, data->ws_rule))
+ data->trailing_blanks_start = 0;
+ else if (!data->trailing_blanks_start)
+ data->trailing_blanks_start = data->lineno;
bad = ws_check(line + 1, len - 1, data->ws_rule);
if (!bad)
return;
@@ -1165,14 +1170,16 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
emit_line(data->o->file, set, reset, line, 1);
ws_check_emit(line + 1, len - 1, data->ws_rule,
data->o->file, set, reset, ws);
- } else if (line[0] == ' ')
+ } else if (line[0] == ' ') {
data->lineno++;
- else if (line[0] == '@') {
+ data->trailing_blanks_start = 0;
+ } else if (line[0] == '@') {
char *plus = strchr(line, '+');
if (plus)
data->lineno = strtol(plus, NULL, 10) - 1;
else
die("invalid diff");
+ data->trailing_blanks_start = 0;
}
}
@@ -1584,6 +1591,12 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
ecb.outf = xdiff_outf;
ecb.priv = &data;
xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+
+ if (data.trailing_blanks_start) {
+ fprintf(o->file, "%s:%d: ends with blank lines.\n",
+ data.filename, data.trailing_blanks_start);
+ data.status = 1; /* report errors */
+ }
}
free_and_return:
diff_free_filespec_data(one);
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index b7cc6b2..0922c70 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -335,4 +335,10 @@ test_expect_success 'line numbers in --check output are correct' '
'
+test_expect_success 'checkdiff detects trailing blank lines' '
+ echo "foo();" >x &&
+ echo "" >>x &&
+ git diff --check | grep "ends with blank"
+'
+
test_done
diff --git a/ws.c b/ws.c
index 24d3e3d..7a7ff13 100644
--- a/ws.c
+++ b/ws.c
@@ -225,6 +225,21 @@ unsigned ws_check(const char *line, int len, unsigned ws_rule)
return ws_check_emit_1(line, len, ws_rule, NULL, NULL, NULL, NULL);
}
+int ws_blank_line(const char *line, int len, unsigned ws_rule)
+{
+ /*
+ * We _might_ want to treat CR differently from other
+ * whitespace characters when ws_rule has WS_CR_AT_EOL, but
+ * for now we just use this stupid definition.
+ */
+ while (len-- > 0) {
+ if (!isspace(*line))
+ return 0;
+ line++;
+ }
+ return 1;
+}
+
/* Copy the line to the buffer while fixing whitespaces */
int ws_fix_copy(char *dst, const char *src, int len, unsigned ws_rule, int *error_count)
{
--
1.5.6.1.78.gde8d9
^ permalink raw reply related
* [PATCH 5/5] diff --check: detect leftover conflict markers
From: Junio C Hamano @ 2008-06-26 22:37 UTC (permalink / raw)
To: Christian Holtje; +Cc: Alex Riesen, git
In-Reply-To: <7vk5gbq10p.fsf@gitster.siamese.dyndns.org>
This teaches "diff --check" to detect and complain if new lines
have lines that look like leftover conflict markers.
We should be able to remove the old Perl script used in the sample
pre-commit hook and modernize the script with this facility.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff.c | 35 +++++++++++++++++++++++++++++++++++
t/t4017-diff-retval.sh | 14 ++++++++++++++
2 files changed, 49 insertions(+), 0 deletions(-)
diff --git a/diff.c b/diff.c
index f31c721..d515b06 100644
--- a/diff.c
+++ b/diff.c
@@ -1143,6 +1143,35 @@ struct checkdiff_t {
int trailing_blanks_start;
};
+static int is_conflict_marker(const char *line, unsigned long len)
+{
+ char firstchar;
+ int cnt;
+
+ if (len < 8)
+ return 0;
+ firstchar = line[0];
+ switch (firstchar) {
+ case '=': case '>': case '<':
+ break;
+ default:
+ return 0;
+ }
+ for (cnt = 1; cnt < 7; cnt++)
+ if (line[cnt] != firstchar)
+ return 0;
+ /* line[0] thru line[6] are same as firstchar */
+ if (firstchar == '=') {
+ /* divider between ours and theirs? */
+ if (len != 8 || line[7] != '\n')
+ return 0;
+ } else if (len < 8 || !isspace(line[7])) {
+ /* not divider before ours nor after theirs */
+ return 0;
+ }
+ return 1;
+}
+
static void checkdiff_consume(void *priv, char *line, unsigned long len)
{
struct checkdiff_t *data = priv;
@@ -1159,6 +1188,12 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
data->trailing_blanks_start = 0;
else if (!data->trailing_blanks_start)
data->trailing_blanks_start = data->lineno;
+ if (is_conflict_marker(line + 1, len - 1)) {
+ data->status |= 1;
+ fprintf(data->o->file,
+ "%s:%d: leftover conflict marker\n",
+ data->filename, data->lineno);
+ }
bad = ws_check(line + 1, len - 1, data->ws_rule);
if (!bad)
return;
diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
index 0d0fb87..d748d45 100755
--- a/t/t4017-diff-retval.sh
+++ b/t/t4017-diff-retval.sh
@@ -113,4 +113,18 @@ test_expect_success 'check should test not just the last line' '
'
+test_expect_success 'check detects leftover conflict markers' '
+ git reset --hard &&
+ git checkout HEAD^ &&
+ echo binary >>b &&
+ git commit -m "side" b &&
+ test_must_fail git merge master &&
+ git add b && (
+ git --no-pager diff --cached --check >test.out
+ test $? = 2
+ ) &&
+ test "$(grep "conflict marker" test.out | wc -l)" = 3 &&
+ git reset --hard
+'
+
test_done
--
1.5.6.1.78.gde8d9
^ permalink raw reply related
* Re: [PATCH v2] pre-commit hook should ignore carriage returns at EOL
From: Junio C Hamano @ 2008-06-26 23:01 UTC (permalink / raw)
To: Christian Holtje; +Cc: Alex Riesen, git
In-Reply-To: <7vk5gbq10p.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Christian Holtje <docwhat@gmail.com> writes:
>
>>> I suggested using "diff --check" (and possibly teaching "diff --check"
>>> other things the scripted example checks, such as conflict markers),
>>> which would know to honor the line endings specified per path via
>>> gitattributes(5), instead of building on top of the big Perl script,
>>> and I
>>> had an impression that you agreed to the approach.
>>
>> I'm completely confused how gitattributes and core.autocrlf interact,
>> etc.
>
> Here is a series I just cooked up so that we can remove the whole Perl
> script and replace it by adding --check to "diff-index" used there.
>
> The first three are code clean-ups and the last two implements necessary
> new features to "diff --check". The whole series somewhat depend on the
> fix to 'maint' not to lose the exit status I sent earlier.
>
> [PATCH 1/5] diff --check: explain why we do not care whether old side is binary
> [PATCH 2/5] check_and_emit_line(): rename and refactor
> [PATCH 3/5] checkdiff: pass diff_options to the callback
> [PATCH 4/5] Teach "diff --check" about a new blank lines at end
> [PATCH 5/5] diff --check: detect leftover conflict markers
With these enhancements in place, I think the pre-commit hook to find
problematic change would become essentially a one-liner, something like:
git diff-index --check -M --cached
and the checking will obey what you configured with core.whitespace, which
globally defines what kind of whitespace breakages are "problematic",
and/or whitespace attribute which determines the same per path.
If you have for example Python source files that you would want all the
default whitespace checks (that is, trailing whitespaces are not allowed,
initial indentation part should not have SP followed by HT), you would
have
*.py whitespace=trail,space-before-tab
in your .gitattributes, and the above command would catch such a
breakage. If you further want to catch indentation with more than 8
SPs that can be replaced with HTs in your C sources, you would say:
*.[ch] whitespace=indent-with-no-tab,trail,space-before-tab
You could choose to have CRLF line endings in the repository [*1*], and
for such projects, diff output would have tons of lines that end with
CRs. To consider these CRs part of the line terminator, add cr-at-eol
to the value of whitespace attribute, like so:
*.py whitespace=trail,space,cr-at-eol
*.[ch] whitespace=indent,trail,space,cr-at-eol
[Footnote]
*1* I do not do Windows, but my understanding is that this practice is not
recommended because it would hurt cross-platform use of the project. You
would instead keep your repository copy with LF line endings, and make
your checkouts have CRLF line endings by core.autocrlf configuration.
^ permalink raw reply
* Re: [PATCH v2] pre-commit hook should ignore carriage returns at EOL
From: Junio C Hamano @ 2008-06-26 23:06 UTC (permalink / raw)
To: Christian Holtje; +Cc: Alex Riesen, git
In-Reply-To: <7vprq3ol63.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> With these enhancements in place, I think the pre-commit hook to find
> problematic change would become essentially a one-liner, something like:
>
> git diff-index --check -M --cached
More specifically, it would be like this.
templates/hooks--pre-commit.sample | 89 ++++++++----------------------------
1 files changed, 19 insertions(+), 70 deletions(-)
diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample
dissimilarity index 79%
index 71c10f2..c302ed1 100755
--- a/templates/hooks--pre-commit.sample
+++ b/templates/hooks--pre-commit.sample
@@ -1,70 +1,19 @@
-#!/bin/sh
-#
-# An example hook script to verify what is about to be committed.
-# Called by git-commit with no arguments. The hook should
-# exit with non-zero status after issuing an appropriate message if
-# it wants to stop the commit.
-#
-# To enable this hook, rename this file to "pre-commit".
-
-# This is slightly modified from Andrew Morton's Perfect Patch.
-# Lines you introduce should not have trailing whitespace.
-# Also check for an indentation that has SP before a TAB.
-
-if git-rev-parse --verify HEAD 2>/dev/null
-then
- git-diff-index -p -M --cached HEAD --
-else
- # NEEDSWORK: we should produce a diff with an empty tree here
- # if we want to do the same verification for the initial import.
- :
-fi |
-perl -e '
- my $found_bad = 0;
- my $filename;
- my $reported_filename = "";
- my $lineno;
- sub bad_line {
- my ($why, $line) = @_;
- if (!$found_bad) {
- print STDERR "*\n";
- print STDERR "* You have some suspicious patch lines:\n";
- print STDERR "*\n";
- $found_bad = 1;
- }
- if ($reported_filename ne $filename) {
- print STDERR "* In $filename\n";
- $reported_filename = $filename;
- }
- print STDERR "* $why (line $lineno)\n";
- print STDERR "$filename:$lineno:$line\n";
- }
- while (<>) {
- if (m|^diff --git a/(.*) b/\1$|) {
- $filename = $1;
- next;
- }
- if (/^@@ -\S+ \+(\d+)/) {
- $lineno = $1 - 1;
- next;
- }
- if (/^ /) {
- $lineno++;
- next;
- }
- if (s/^\+//) {
- $lineno++;
- chomp;
- if (/\s$/) {
- bad_line("trailing whitespace", $_);
- }
- if (/^\s* \t/) {
- bad_line("indent SP followed by a TAB", $_);
- }
- if (/^([<>])\1{6} |^={7}$/) {
- bad_line("unresolved merge conflict", $_);
- }
- }
- }
- exit($found_bad);
-'
+#!/bin/sh
+#
+# An example hook script to verify what is about to be committed.
+# Called by git-commit with no arguments. The hook should
+# exit with non-zero status after issuing an appropriate message if
+# it wants to stop the commit.
+#
+# To enable this hook, rename this file to "pre-commit".
+
+if git-rev-parse --verify HEAD 2>/dev/null
+then
+ against=
+else
+ # Initial commit: diff against an empty tree object
+ against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+fi
+
+exec git diff-index --check --cached $against
+
^ permalink raw reply related
* Re: pread() over NFS (again) [1.5.5.4]
From: logank @ 2008-06-26 23:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, Christian Holtje, git
In-Reply-To: <7vskuzq5ix.fsf@gitster.siamese.dyndns.org>
On Jun 26, 2008, at 1:56 PM, Junio C Hamano wrote:
>> "The file shouldn't be short unless someone truncated it, or there
>> is a bug in index-pack. Neither is very likely, but I don't think
>> we would want to retry pread'ing the same block forever.
>
> I don't think we would want to retry even once. Return value of 0
> from
> pread is defined to be an EOF, isn't it?
No, it seems to be a simple error-out in this case. We have 2.4.20
systems with nfs-utils 0.3.3 and used to frequently get the same error
while pushing. I made a similar change back in February and haven't
had a problem since:
diff --git a/index-pack.c b/index-pack.c
index 5ac91ba..85c8bdb 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -313,7 +313,14 @@ static void *get_data_from_pack(struct
object_entry *obj)
src = xmalloc(len);
data = src;
do {
+ // It appears that if multiple threads read across NFS, the
+ // second read will fail. I know this is awful, but we wait for
+ // a little bit and try again.
ssize_t n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
+ if (n <= 0) {
+ sleep(1);
+ n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
+ }
if (n <= 0)
die("cannot pread pack file: %s", strerror(errno));
rdy += n;
I use a sleep request since it seems less likely that the other thread
will have an outstanding request after a second of waiting.
--
Logan Kennelly
,,,
(. .)
--ooO-(_)-Ooo--
^ permalink raw reply related
* Re: [PATCH] daemon: accept "git program" as well
From: Olivier Marin @ 2008-06-26 23:38 UTC (permalink / raw)
To: Tommi Virtanen
Cc: Git Mailing List, Junio C Hamano,
しらいしななこ,
Miklos Vajna, pclouds, Johannes Schindelin, Pieter de Bie,
Shawn O. Pearce
In-Reply-To: <20080626082013.GT22344@eagain.net>
Hi,
Tommi Virtanen a écrit :
> On Wed, Jun 25, 2008 at 04:26:46PM -0700, Junio C Hamano wrote:
>>
>> In gitosis/serve.py, there are COMMANDS_READONLY and COMMANDS_WRITE array
>> that holds 'git-upload-pack' and 'git-receive-pack' commands, and they are
>> compared with user commands after doing:
>
> Yeah, that's pretty much a trivial change, doing it now to future-proof
> gitosis.
>
This just happened to me with a dashless client, so I tried your patch but it
does not work. The problem comes from git-shell that do not support dashless
argument, yet (IOW: git shell -c 'git upload-pack ...' give an error).
The following patch on top of yours fix the problem. The s/git-shell/git shell/
part is not really necessary, but why not?
diff --git a/gitosis/serve.py b/gitosis/serve.py
index 9a91fcb..5aac355 100644
--- a/gitosis/serve.py
+++ b/gitosis/serve.py
@@ -21,12 +21,10 @@ ALLOW_RE = re.compile("^'/*(?P<path>[a-zA-Z0-9][a-zA-Z0-9@._-]*(/[a-zA-Z0-9][a-z
COMMANDS_READONLY = [
'git-upload-pack',
- 'git upload-pack',
]
COMMANDS_WRITE = [
'git-receive-pack',
- 'git receive-pack',
]
class ServingError(Exception):
@@ -75,7 +73,7 @@ def serve(
# all known "git foo" commands take one argument; improve
# if/when needed
raise UnknownCommandError()
- verb = '%s %s' % (verb, subverb)
+ verb = '%s-%s' % (verb, subverb)
if (verb not in COMMANDS_WRITE
and verb not in COMMANDS_READONLY):
@@ -201,6 +199,6 @@ class Main(app.App):
sys.exit(1)
main_log.debug('Serving %s', newcmd)
- os.execvp('git-shell', ['git-shell', '-c', newcmd])
- main_log.error('Cannot execute git-shell.')
+ os.execvp('git', ['git', 'shell', '-c', newcmd])
+ main_log.error('Cannot execute git.')
sys.exit(1)
Olivier.
^ permalink raw reply related
* Re: pread() over NFS (again) [1.5.5.4]
From: Junio C Hamano @ 2008-06-26 23:38 UTC (permalink / raw)
To: logank; +Cc: J. Bruce Fields, Shawn O. Pearce, Christian Holtje, git
In-Reply-To: <65688C06-BB6A-4E95-A4B9-A1A7C206BE2E@sent.com>
logank@sent.com writes:
> On Jun 26, 2008, at 1:56 PM, Junio C Hamano wrote:
>
>>> "The file shouldn't be short unless someone truncated it, or there
>>> is a bug in index-pack. Neither is very likely, but I don't think
>>> we would want to retry pread'ing the same block forever.
>>
>> I don't think we would want to retry even once. Return value of 0
>> from
>> pread is defined to be an EOF, isn't it?
>
> No, it seems to be a simple error-out in this case. We have 2.4.20
> systems with nfs-utils 0.3.3 and used to frequently get the same error
> while pushing. I made a similar change back in February and haven't
> had a problem since:
>
> diff --git a/index-pack.c b/index-pack.c
> index 5ac91ba..85c8bdb 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -313,7 +313,14 @@ static void *get_data_from_pack(struct
> object_entry *obj)
> src = xmalloc(len);
> data = src;
> do {
> + // It appears that if multiple threads read across NFS, the
> + // second read will fail. I know this is awful, but we wait for
> + // a little bit and try again.
> ssize_t n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> + if (n <= 0) {
> + sleep(1);
> + n = pread(pack_fd, data + rdy, len - rdy, from + rdy);
> + }
> if (n <= 0)
> die("cannot pread pack file: %s", strerror(errno));
> rdy += n;
>
> I use a sleep request since it seems less likely that the other thread
> will have an outstanding request after a second of waiting.
Gaah. Don't we have NFS experts in house? Bruce, perhaps?
^ permalink raw reply
* Re: [PATCH] daemon: accept "git program" as well
From: Junio C Hamano @ 2008-06-26 23:42 UTC (permalink / raw)
To: Olivier Marin
Cc: Tommi Virtanen, Git Mailing List,
しらいしななこ,
Miklos Vajna, pclouds, Johannes Schindelin, Pieter de Bie,
Shawn O. Pearce
In-Reply-To: <4864286C.80609@free.fr>
Olivier Marin <dkr+ml.git@free.fr> writes:
> Hi,
>
> Tommi Virtanen a écrit :
>> On Wed, Jun 25, 2008 at 04:26:46PM -0700, Junio C Hamano wrote:
>>>
>>> In gitosis/serve.py, there are COMMANDS_READONLY and COMMANDS_WRITE array
>>> that holds 'git-upload-pack' and 'git-receive-pack' commands, and they are
>>> compared with user commands after doing:
>>
>> Yeah, that's pretty much a trivial change, doing it now to future-proof
>> gitosis.
>>
>
> This just happened to me with a dashless client, so I tried your patch but it
> does not work. The problem comes from git-shell that do not support dashless
> argument, yet (IOW: git shell -c 'git upload-pack ...' give an error).
>
> The following patch on top of yours fix the problem. The s/git-shell/git shell/
> part is not really necessary, but why not?
>
> diff --git a/gitosis/serve.py b/gitosis/serve.py
> index 9a91fcb..5aac355 100644
> --- a/gitosis/serve.py
> +++ b/gitosis/serve.py
> @@ -21,12 +21,10 @@ ALLOW_RE = re.compile("^'/*(?P<path>[a-zA-Z0-9][a-zA-Z0-9@._-]*(/[a-zA-Z0-9][a-z
>
> COMMANDS_READONLY = [
> 'git-upload-pack',
> - 'git upload-pack',
> ]
>
> COMMANDS_WRITE = [
> 'git-receive-pack',
> - 'git receive-pack',
> ]
>
> class ServingError(Exception):
> @@ -75,7 +73,7 @@ def serve(
> # all known "git foo" commands take one argument; improve
> # if/when needed
> raise UnknownCommandError()
> - verb = '%s %s' % (verb, subverb)
> + verb = '%s-%s' % (verb, subverb)
>
> if (verb not in COMMANDS_WRITE
> and verb not in COMMANDS_READONLY):
> @@ -201,6 +199,6 @@ class Main(app.App):
> sys.exit(1)
>
> main_log.debug('Serving %s', newcmd)
> - os.execvp('git-shell', ['git-shell', '-c', newcmd])
> - main_log.error('Cannot execute git-shell.')
> + os.execvp('git', ['git', 'shell', '-c', newcmd])
> + main_log.error('Cannot execute git.')
> sys.exit(1)
Hmm, Tommi, if you are doing command sanitizing yourself, is there a
reason to still invoke the command via git-shell?
^ permalink raw reply
* is rebase the same as merging every commit?
From: David Jeske @ 2008-06-26 23:04 UTC (permalink / raw)
To: git
Rebasing is described in the docs I've read as turning this: (sorry for the
dots)
..........A---B---C topic
........./
....D---E---F---G master
Into this:
...................A'--B'--C' topic
................../
.....D---E---F---G master
If I understand it right (and that's a BIG if), it's the same as doing a merge
of C into G where every individual commit in the C-line is individually
committed into the new C' line.
...........-------------A---B---C
........../ / / /
........./ /---A'--B'--C' topic
......../ /
....D---E---F---G - master
(1) Is the above model a valid explanation?
(2) From the documentation diagrams, it looks like the rebased A' has only (G)
as a parent, not (A,G). If this is the case, why? (i.e. not connecting those
nodes throws away useful information)
(3) If it only has (G) as a parent, does the rebase explicitly remove the
source A,B,C nodes from the repository? (the diagrams make it look like it
does) ..or do they just get cleaned up during GC?
^ permalink raw reply
* is rebase the same as merging every commit?
From: David Jeske @ 2008-06-26 23:04 UTC (permalink / raw)
To: git
Rebasing is described in the docs I've read as turning this: (sorry for the
dots)
..........A---B---C topic
........./
....D---E---F---G master
Into this:
...................A'--B'--C' topic
................../
.....D---E---F---G master
If I understand it right (and that's a BIG if), it's the same as doing a merge
of C into G where every individual commit in the C-line is individually
committed into the new C' line.
...........-------------A---B---C
........../ / / /
........./ /---A'--B'--C' topic
......../ /
....D---E---F---G - master
(1) Is the above model a valid explanation?
(2) From the documentation diagrams, it looks like the rebased A' has only (G)
as a parent, not (A,G). If this is the case, why? (i.e. not connecting those
nodes throws away useful information)
(3) If it only has (G) as a parent, does the rebase explicitly remove the
source A,B,C nodes from the repository? (the diagrams make it look like it
does) ..or do they just get cleaned up during GC?
^ permalink raw reply
* [PATCH] Documentation: remove {show,whatchanged}.difftree config options
From: Olivier Marin @ 2008-06-27 0:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
From: Olivier Marin <dkr@freesurf.fr>
This removes, from the documentation and the bash completion script, the
two config options that were introduced by the git-whatchanged.sh script
and lost in the C rewrite. Today, we can use aliases as an alternative.
Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---
Documentation/config.txt | 8 --------
contrib/completion/git-completion.bash | 2 --
2 files changed, 0 insertions(+), 10 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 90c8a45..6966384 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1007,10 +1007,6 @@ repack.usedeltabaseoffset::
Allow linkgit:git-repack[1] to create packs that uses
delta-base offset. Defaults to false.
-show.difftree::
- The default linkgit:git-diff-tree[1] arguments to be used
- for linkgit:git-show[1].
-
showbranch.default::
The default set of branches for linkgit:git-show-branch[1].
See linkgit:git-show-branch[1].
@@ -1075,10 +1071,6 @@ user.signingkey::
unchanged to gpg's --local-user parameter, so you may specify a key
using any method that gpg supports.
-whatchanged.difftree::
- The default linkgit:git-diff-tree[1] arguments to be used
- for linkgit:git-whatchanged[1].
-
imap::
The configuration variables in the 'imap' section are described
in linkgit:git-imap-send[1].
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ebf7cde..3f46149 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1041,7 +1041,6 @@ _git_config ()
pull.octopus
pull.twohead
repack.useDeltaBaseOffset
- show.difftree
showbranch.default
tar.umask
transfer.unpackLimit
@@ -1050,7 +1049,6 @@ _git_config ()
user.name
user.email
user.signingkey
- whatchanged.difftree
branch. remote.
"
}
--
1.5.6.1.141.ge3ab.dirty
^ permalink raw reply related
* [PATCH] show_stats(): fix stats width calculation
From: Olivier Marin @ 2008-06-27 0:18 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
From: Olivier Marin <dkr@freesurf.fr>
Before this patch, name_width becomes negative or null for width values
less than 15 and name_width values greater than 25 (default: 50). This
leads to output random data.
This patch checks for minimal width and name_width values.
Signed-off-by: Olivier Marin <dkr@freesurf.fr>
---
diff.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index 44e8790..a85c8ca 100644
--- a/diff.c
+++ b/diff.c
@@ -830,12 +830,12 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)
/* Sanity: give at least 5 columns to the graph,
* but leave at least 10 columns for the name.
*/
- if (width < name_width + 15) {
- if (name_width <= 25)
- width = name_width + 15;
- else
- name_width = width - 15;
- }
+ if (width < 25)
+ width = 25;
+ if (name_width < 10)
+ name_width = 10;
+ else if (width < name_width + 15)
+ name_width = width - 15;
/* Find the longest filename and max number of changes */
reset = diff_get_color_opt(options, DIFF_RESET);
--
1.5.6.1.141.ge3ab.dirty
^ permalink raw reply related
* git-gui fails to start
From: Michael P. Soulier @ 2008-06-27 0:23 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]
Hi,
I'm on a CentOS Linux box, tracking 4.6 with Git 1.5.5.4 built from source.
When I run gitk on a repository it works fine, but nothing happens when I run
git-gui. No error messages, nothing.
If I set GITGUI_VERBOSE, I get this
auto_load tk_optionMenu
source /usr/share/tk8.4/optMenu.tcl
auto_load tk::MenuDup
source /usr/share/tk8.4/tearoff.tcl
auto_load ::status_bar::new
source /home/msoulier/git/share/git-gui/lib/status_bar.tcl
auto_load class
source /home/msoulier/git/share/git-gui/lib/class.tcl
auto_load load_all_remotes
source /home/msoulier/git/share/git-gui/lib/remote.tcl
auto_load spellcheck::init
source /home/msoulier/git/share/git-gui/lib/spellcheck.tcl
and then it just sits there.
I have Tcl/Tk 8.4.9.
How would I troubleshoot this?
Thanks,
Mike
--
Michael P. Soulier <msoulier@digitaltorque.ca>
"Any intelligent fool can make things bigger and more complex... It
takes a touch of genius - and a lot of courage to move in the opposite
direction." --Albert Einstein
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: is rebase the same as merging every commit?
From: Junio C Hamano @ 2008-06-27 0:51 UTC (permalink / raw)
To: David Jeske; +Cc: git
In-Reply-To: <1006.35704952783$1214525911@news.gmane.org>
"David Jeske" <jeske@willowmail.com> writes:
> Rebasing is described in the docs I've read as turning this: (sorry for the
> dots)
>
> ..........A---B---C topic
> ........./
> ....D---E---F---G master
>
> Into this:
>
> ...................A'--B'--C' topic
> ................../
> .....D---E---F---G master
>
> If I understand it right (and that's a BIG if), it's the same as doing a merge
> of C into G where every individual commit in the C-line is individually
> committed into the new C' line.
>
> ...........-------------A---B---C
> ........../ / / /
> ........./ /---A'--B'--C' topic
> ......../ /
> ....D---E---F---G - master
>
>
> (1) Is the above model a valid explanation?
I would presume that the resulting trees A' in the second picture and in
the first picture would be the same, so are B' and C'. But that is only
true when commits between A and C do not have any duplicate with the
development that happened between E and G.
Thinking about it like that is an interesting mental exercise, but it is
not very useful otherwise.
> (2) From the documentation diagrams, it looks like the rebased A' has
> only (G) as a parent, not (A,G). If this is the case, why? (i.e. not
> connecting those nodes throws away useful information)
You would rebase ONLY WHEN the project as the whole (either "other people
in the project", or "yourself down the road one year from now") is
interested mostly in the progress of 'master' D-E-F-G, and nobody cares
whether you developed your A (or B or C) on top of E or G. So the answer
is definite "no" --- the line you drew between A and A' is a useless
information. Nobody cares you did it first on top of E but then you have
redone the patches based on G (because things changed between E and G).
If there were no "rebase", your changes will be integrated into 'master'
branch like this:
A---B---C
/ \
D---E---F---G---M
Rebasing is a way to _help you_ pretend that you did _not_ start working
on an ancient code base that was at E. You redo your series on top of the
latest and greatest G, the commit that everybody else agrees is the
current state of affairs when he sees your changes for the first time, to
produce a history like this:
D---E---F---G---A'--B'--C'
Doing so tends to make the history easier to understand, and more
importantly, it reduces mistakes during the integration _and_ distributes
the burden of integration from central point.
If E..G and A..C happen to have conflicting changes, rebasing puts the
burden to rewrite the changes A..C into A'..C', based on the modified base
code G, on _you_ (the person who is rebasing). Some people do not like
this, as they feel that is an added, unwanted burden. On the other hand,
if your upstream maintainer is integrating like the above picture to
create a merge 'M', it is more likely that he would make mistakes during
the conflict resolution, than you make incorrect adjustment during your
rebasing to recreate the series A'..C'. You read what G gives you as the
foundation to build your changes on, determine what got changed since E,
on which you originally based your changes, and adjust your changes to
better integrate on top of G. After all, A..C is _your code_ and you
understand what it assumes better than anybody else.
If the fact that parallel developments have happened is important, instead
of the second picture like you drew, you will just do the real merge
naturally to create a merge "M" like the picture I drew above.
Your "A' is merge between E and A, B' is merge between A' and B" is not
something anybody is interested in if you are going to rebase. It is not
interesting because it is not how things happened in the real life at all,
and it is not interesting because it is not simplifying the history for
later analysis nor reducing mistakes during the conflict resolution.
^ permalink raw reply
* Re: git svn clone a non-standard repository
From: John Locke @ 2008-06-27 1:06 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git
In-Reply-To: <4863E261.8040508@freelock.com>
Okay. I think I've closed the loop, and have this updating successfully,
if not necessarily easily.
John Locke wrote:
> Sorry to respond to my own post, but this section looks wrong:
>
> John Locke wrote:
>>
>>
>> Now the tricky part: I checked out a new "work" branch, and deleted
>> everything in the working copy. Then, 4 steps out of the howto,
>> adjusted to pull the particular branch from the current repository
>> (instead of an outside one):
>>
>> git remote add -t dojo -f dojo ./
>> git merge -s ours --no-commit dojo-trunk
>> git read-tree --prefix=dojo/ -u dojo-trunk
>> git commit -m "merge dojo into subdirectory"
>
> ... since I added the remotes as svn-remote sections directly in
> git-config, I skipped that first line.
>
>
To update, we need a local branch for each submodule:
git checkout -b dojo dojo-trunk <- create local branch and switch to it
(only create it the first time through)
git svn rebase dojo-trunk <- do this to update each local branch from
the remote svn repository version--must have corresponding branch
checked out.
... now that we've merged the remote changes to a local branch, we can
update our work branch from each module's branch:
git checkout work
git pull -s subtree ./ dojo
git pull -s subtree ./ dijit
....
--
John Locke
"Open Source Solutions for Small Business Problems"
published by Charles River Media, June 2004
http://www.freelock.com
^ permalink raw reply
* Re: [PATCH 12/13] Build in merge
From: Miklos Vajna @ 2008-06-27 1:06 UTC (permalink / raw)
To: Olivier Marin; +Cc: Junio C Hamano, git
In-Reply-To: <486270D5.5050204@free.fr>
[-- Attachment #1: Type: text/plain, Size: 2670 bytes --]
On Wed, Jun 25, 2008 at 06:22:45PM +0200, Olivier Marin <dkr+ml.git@free.fr> wrote:
> > + if (new_head && show_diffstat) {
> > + diff_setup(&opts);
> > + opts.output_format |=
> > + DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> > + opts.detect_rename = DIFF_DETECT_RENAME;
>
> You probably want to add DIFF_OPT_SET(&opts, COLOR_DIFF) to have a nice colored diff
> like the original script.
That would always turn colors on.
This should respect diff.color:
diff --git a/builtin-merge.c b/builtin-merge.c
index 66189d2..c2a32ee 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -331,6 +331,8 @@ static void finish(const unsigned char *new_head, const char *msg)
opts.output_format |=
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
opts.detect_rename = DIFF_DETECT_RENAME;
+ if (diff_use_color_default > 0)
+ DIFF_OPT_SET(&opts, COLOR_DIFF);
diff_tree_sha1(head, new_head, "", &opts);
diffcore_std(&opts);
diff_flush(&opts);
@@ -693,6 +695,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
}
git_config(git_merge_config, NULL);
+ git_config(git_diff_ui_config, NULL);
argc = parse_options(argc, argv, builtin_merge_options,
builtin_merge_usage, 0);
> > +int cmd_merge(int argc, const char **argv, const char *prefix)
> > +{
>
> [...]
>
> > + } else if (allow_fast_forward && !remoteheads->next &&
> > + !hashcmp(common->item->object.sha1, head)) {
> > + /* Again the most common case of merging one remote. */
> > + struct strbuf msg;
> > + struct object *o;
> > +
> > + printf("Updating %s..%s\n",
> > + find_unique_abbrev(head, DEFAULT_ABBREV),
> > + find_unique_abbrev(remoteheads->item->object.sha1,
> > + DEFAULT_ABBREV));
>
> Here, the second call to find_unique_abbrev() will overwrite the previous returned
> value because this function return a pointer to a static buffer.
This should fix it:
diff --git a/builtin-merge.c b/builtin-merge.c
index cc04d01..77de9e8 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -836,9 +836,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
/* Again the most common case of merging one remote. */
struct strbuf msg;
struct object *o;
+ char hex[41];
+
+ memcpy(hex, find_unique_abbrev(head, DEFAULT_ABBREV), 41);
printf("Updating %s..%s\n",
- find_unique_abbrev(head, DEFAULT_ABBREV),
+ hex,
find_unique_abbrev(remoteheads->item->object.sha1,
DEFAULT_ABBREV));
refresh_cache(REFRESH_QUIET);
I pushed both to my working branch and I'll send proper patches to this
list among with other small changes soon.
Thanks!
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply related
* Re: is rebase the same as merging every commit?
From: Junio C Hamano @ 2008-06-27 1:08 UTC (permalink / raw)
To: David Jeske; +Cc: git
In-Reply-To: <7vzlp7n1j4.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> "David Jeske" <jeske@willowmail.com> writes:
> ...
>> (2) From the documentation diagrams, it looks like the rebased A' has
>> only (G) as a parent, not (A,G). If this is the case, why? (i.e. not
>> connecting those nodes throws away useful information)
>
> You would rebase ONLY WHEN the project as the whole (either "other people
> in the project", or "yourself down the road one year from now") is
> interested mostly in the progress of 'master' D-E-F-G, and nobody cares
> whether you developed your A (or B or C) on top of E or G. So the answer
> is definite "no" --- the line you drew between A and A' is a useless
> information. Nobody cares you did it first on top of E but then you have
> redone the patches based on G (because things changed between E and G).
The last sentence came out in somewhat inappropriate way.
In the situation "rebase" (which is a way to help you pretend you
did not start building on a stale codebase) is appropriate, nobody
wants to know you did it first on E
is what I meant. More importantly, _you_ do not want anybody to know.
That is the whole reason you would rebase.
With that clarification in mind, the explanation would flow more smoothly
to this part...
> If the fact that parallel developments have happened is important, instead
> of the second picture like you drew, you will just do the real merge
> naturally to create a merge "M" like the picture I drew above.
So you have a choice between merging and rebasing. And your extra parents
goes against the reason you chose rebasing in the first place. That is
why we do not record the original parents anywhere.
^ 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