* [PATCH 1/2] diff.c: second war on whitespace.
From: Junio C Hamano @ 2006-09-23 7:47 UTC (permalink / raw)
To: git
This adds DIFF_WHITESPACE color class (default = reverse red) to
colored diff output to let you catch common whitespace errors.
- trailing whitespaces at the end of line
- a space followed by a tab in the indent
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
diff.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++---------------
diff.h | 1
2 files changed, 132 insertions(+), 41 deletions(-)
diff --git a/diff.c b/diff.c
index 443e248..2464238 100644
--- a/diff.c
+++ b/diff.c
@@ -20,12 +20,13 @@ static int diff_use_color_default;
static char diff_colors[][COLOR_MAXLEN] = {
"\033[m", /* reset */
- "", /* normal */
- "\033[1m", /* bold */
- "\033[36m", /* cyan */
- "\033[31m", /* red */
- "\033[32m", /* green */
- "\033[33m" /* yellow */
+ "", /* PLAIN (normal) */
+ "\033[1m", /* METAINFO (bold) */
+ "\033[36m", /* FRAGINFO (cyan) */
+ "\033[31m", /* OLD (red) */
+ "\033[32m", /* NEW (green) */
+ "\033[33m", /* COMMIT (yellow) */
+ "\033[41m", /* WHITESPACE (red background) */
};
static int parse_diff_color_slot(const char *var, int ofs)
@@ -42,6 +43,8 @@ static int parse_diff_color_slot(const c
return DIFF_FILE_NEW;
if (!strcasecmp(var+ofs, "commit"))
return DIFF_COMMIT;
+ if (!strcasecmp(var+ofs, "whitespace"))
+ return DIFF_WHITESPACE;
die("bad config variable '%s'", var);
}
@@ -383,9 +386,89 @@ const char *diff_get_color(int diff_use_
return "";
}
+static void emit_line(const char *set, const char *reset, const char *line, int len)
+{
+ if (len > 0 && line[len-1] == '\n')
+ len--;
+ fputs(set, stdout);
+ fwrite(line, len, 1, stdout);
+ puts(reset);
+}
+
+static void emit_add_line(const char *reset, struct emit_callback *ecbdata, const char *line, int len)
+{
+ int col0 = ecbdata->nparents;
+ int last_tab_in_indent = -1;
+ int last_space_in_indent = -1;
+ int i;
+ int tail = len;
+ int need_highlight_leading_space = 0;
+ const char *ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
+ const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_NEW);
+
+ if (!*ws) {
+ emit_line(set, reset, line, len);
+ return;
+ }
+
+ /* The line is a newly added line. Does it have funny leading
+ * whitespaces? In indent, SP should never precede a TAB.
+ */
+ for (i = col0; i < len; i++) {
+ if (line[i] == '\t') {
+ last_tab_in_indent = i;
+ if (0 <= last_space_in_indent)
+ need_highlight_leading_space = 1;
+ }
+ else if (line[i] == ' ')
+ last_space_in_indent = i;
+ else
+ break;
+ }
+ fputs(set, stdout);
+ fwrite(line, col0, 1, stdout);
+ fputs(reset, stdout);
+ if (((i == len) || line[i] == '\n') && i != col0) {
+ /* The whole line was indent */
+ emit_line(ws, reset, line + col0, len - col0);
+ return;
+ }
+ i = col0;
+ if (need_highlight_leading_space) {
+ while (i < last_tab_in_indent) {
+ if (line[i] == ' ') {
+ fputs(ws, stdout);
+ putchar(' ');
+ fputs(reset, stdout);
+ }
+ else
+ putchar(line[i]);
+ i++;
+ }
+ }
+ tail = len - 1;
+ if (line[tail] == '\n' && i < tail)
+ tail--;
+ while (i < tail) {
+ if (!isspace(line[tail]))
+ break;
+ tail--;
+ }
+ if ((i < tail && line[tail + 1] != '\n')) {
+ /* This has whitespace between tail+1..len */
+ fputs(set, stdout);
+ fwrite(line + i, tail - i + 1, 1, stdout);
+ fputs(reset, stdout);
+ emit_line(ws, reset, line + tail + 1, len - tail - 1);
+ }
+ else
+ emit_line(set, reset, line + i, len - i);
+}
+
static void fn_out_consume(void *priv, char *line, unsigned long len)
{
int i;
+ int color;
struct emit_callback *ecbdata = priv;
const char *set = diff_get_color(ecbdata->color_diff, DIFF_METAINFO);
const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
@@ -403,45 +486,52 @@ static void fn_out_consume(void *priv, c
;
if (2 <= i && i < len && line[i] == ' ') {
ecbdata->nparents = i - 1;
- set = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
+ emit_line(diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO),
+ reset, line, len);
+ return;
}
- else if (len < ecbdata->nparents)
+
+ if (len < ecbdata->nparents) {
set = reset;
- else {
- int nparents = ecbdata->nparents;
- int color = DIFF_PLAIN;
- if (ecbdata->diff_words && nparents != 1)
- /* fall back to normal diff */
- free_diff_words_data(ecbdata);
- if (ecbdata->diff_words) {
- if (line[0] == '-') {
- diff_words_append(line, len,
- &ecbdata->diff_words->minus);
- return;
- } else if (line[0] == '+') {
- diff_words_append(line, len,
- &ecbdata->diff_words->plus);
- return;
- }
- if (ecbdata->diff_words->minus.text.size ||
- ecbdata->diff_words->plus.text.size)
- diff_words_show(ecbdata->diff_words);
- line++;
- len--;
- } else
- for (i = 0; i < nparents && len; i++) {
- if (line[i] == '-')
- color = DIFF_FILE_OLD;
- else if (line[i] == '+')
- color = DIFF_FILE_NEW;
- }
- set = diff_get_color(ecbdata->color_diff, color);
+ emit_line(reset, reset, line, len);
+ return;
}
- if (len > 0 && line[len-1] == '\n')
+
+ color = DIFF_PLAIN;
+ if (ecbdata->diff_words && ecbdata->nparents != 1)
+ /* fall back to normal diff */
+ free_diff_words_data(ecbdata);
+ if (ecbdata->diff_words) {
+ if (line[0] == '-') {
+ diff_words_append(line, len,
+ &ecbdata->diff_words->minus);
+ return;
+ } else if (line[0] == '+') {
+ diff_words_append(line, len,
+ &ecbdata->diff_words->plus);
+ return;
+ }
+ if (ecbdata->diff_words->minus.text.size ||
+ ecbdata->diff_words->plus.text.size)
+ diff_words_show(ecbdata->diff_words);
+ line++;
len--;
- fputs (set, stdout);
- fwrite (line, len, 1, stdout);
- puts (reset);
+ emit_line(set, reset, line, len);
+ return;
+ }
+ for (i = 0; i < ecbdata->nparents && len; i++) {
+ if (line[i] == '-')
+ color = DIFF_FILE_OLD;
+ else if (line[i] == '+')
+ color = DIFF_FILE_NEW;
+ }
+
+ if (color != DIFF_FILE_NEW) {
+ emit_line(diff_get_color(ecbdata->color_diff, color),
+ reset, line, len);
+ return;
+ }
+ emit_add_line(reset, ecbdata, line, len);
}
static char *pprint_rename(const char *a, const char *b)
diff --git a/diff.h b/diff.h
index b60a02e..3435fe7 100644
--- a/diff.h
+++ b/diff.h
@@ -86,6 +86,7 @@ enum color_diff {
DIFF_FILE_OLD = 4,
DIFF_FILE_NEW = 5,
DIFF_COMMIT = 6,
+ DIFF_WHITESPACE = 7,
};
const char *diff_get_color(int diff_use_color, enum color_diff ix);
--
1.4.2.1.gf2bba
^ permalink raw reply related
* [PATCH 2/2] git-apply: second war on whitespace.
From: Junio C Hamano @ 2006-09-23 7:47 UTC (permalink / raw)
To: git
This makes --whitespace={warn,error,strip} option to also notice
the leading whitespace errors in addition to the trailing
whitespace errors. Spaces that are followed by a tab in indent
are detected as errors, and --whitespace=strip option fixes them.
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
builtin-apply.c | 122 ++++++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 97 insertions(+), 25 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 25e90d8..de5f855 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -854,6 +854,49 @@ static int find_header(char *line, unsig
return -1;
}
+static void check_whitespace(const char *line, int len)
+{
+ const char *err = "Adds trailing whitespace";
+ int seen_space = 0;
+ int i;
+
+ /*
+ * We know len is at least two, since we have a '+' and we
+ * checked that the last character was a '\n' before calling
+ * this function. That is, an addition of an empty line would
+ * check the '+' here. Sneaky...
+ */
+ if (isspace(line[len-2]))
+ goto error;
+
+ /*
+ * Make sure that there is no space followed by a tab in
+ * indentation.
+ */
+ err = "Space in indent is followed by a tab";
+ for (i = 1; i < len; i++) {
+ if (line[i] == '\t') {
+ if (seen_space)
+ goto error;
+ }
+ else if (line[i] == ' ')
+ seen_space = 1;
+ else
+ break;
+ }
+ return;
+
+ error:
+ whitespace_error++;
+ if (squelch_whitespace_errors &&
+ squelch_whitespace_errors < whitespace_error)
+ ;
+ else
+ fprintf(stderr, "%s.\n%s:%d:%.*s\n",
+ err, patch_input_file, linenr, len-2, line+1);
+}
+
+
/*
* Parse a unified diff. Note that this really needs to parse each
* fragment separately, since the only way to know the difference
@@ -904,25 +947,8 @@ static int parse_fragment(char *line, un
trailing = 0;
break;
case '+':
- /*
- * We know len is at least two, since we have a '+' and
- * we checked that the last character was a '\n' above.
- * That is, an addition of an empty line would check
- * the '+' here. Sneaky...
- */
- if ((new_whitespace != nowarn_whitespace) &&
- isspace(line[len-2])) {
- whitespace_error++;
- if (squelch_whitespace_errors &&
- squelch_whitespace_errors <
- whitespace_error)
- ;
- else {
- fprintf(stderr, "Adds trailing whitespace.\n%s:%d:%.*s\n",
- patch_input_file,
- linenr, len-2, line+1);
- }
- }
+ if (new_whitespace != nowarn_whitespace)
+ check_whitespace(line, len);
added++;
newlines--;
trailing = 0;
@@ -1494,22 +1520,68 @@ static int apply_line(char *output, cons
{
/* plen is number of bytes to be copied from patch,
* starting at patch+1 (patch[0] is '+'). Typically
- * patch[plen] is '\n'.
+ * patch[plen] is '\n', unless this is the incomplete
+ * last line.
*/
+ int i;
int add_nl_to_tail = 0;
- if ((new_whitespace == strip_whitespace) &&
- 1 < plen && isspace(patch[plen-1])) {
+ int fixed = 0;
+ int last_tab_in_indent = -1;
+ int last_space_in_indent = -1;
+ int need_fix_leading_space = 0;
+ char *buf;
+
+ if ((new_whitespace != strip_whitespace) || !whitespace_error) {
+ memcpy(output, patch + 1, plen);
+ return plen;
+ }
+
+ if (1 < plen && isspace(patch[plen-1])) {
if (patch[plen] == '\n')
add_nl_to_tail = 1;
plen--;
while (0 < plen && isspace(patch[plen]))
plen--;
- applied_after_stripping++;
+ fixed = 1;
}
- memcpy(output, patch + 1, plen);
+
+ for (i = 1; i < plen; i++) {
+ char ch = patch[i];
+ if (ch == '\t') {
+ last_tab_in_indent = i;
+ if (0 <= last_space_in_indent)
+ need_fix_leading_space = 1;
+ }
+ else if (ch == ' ')
+ last_space_in_indent = i;
+ else
+ break;
+ }
+
+ buf = output;
+ if (need_fix_leading_space) {
+ /* between patch[1..last_tab_in_indent] strip the
+ * funny spaces, updating them to tab as needed.
+ */
+ for (i = 1; i < last_tab_in_indent; i++, plen--) {
+ char ch = patch[i];
+ if (ch != ' ')
+ *output++ = ch;
+ else if ((i % 8) == 0)
+ *output++ = '\t';
+ }
+ fixed = 1;
+ i = last_tab_in_indent;
+ }
+ else
+ i = 1;
+
+ memcpy(output, patch + i, plen);
if (add_nl_to_tail)
output[plen++] = '\n';
- return plen;
+ if (fixed)
+ applied_after_stripping++;
+ return output + plen - buf;
}
static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag, int inaccurate_eof)
--
1.4.2.1.gf2bba
^ permalink raw reply related
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file.
From: Junio C Hamano @ 2006-09-23 8:04 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <200609230645.37773.chriscool@tuxfamily.org>
Christian Couder <chriscool@tuxfamily.org> writes:
> If we move ".git/refs/heads/frotz/nitfol"
> to ".git/deleted-refs/heads/frotz/nitfol" when we remove this ref, we only
> need to try to rmdir all subdirectories under ".git/refs/heads/frotz/" and
> then ".git/refs/heads/frotz/" to see if we can
> create ".git/refs/heads/frotz", and if we can, we won't
> resurect "frotz/nitfol" because ".git/deleted-refs/heads/frotz/nitfol"
> still exists.
I am not sure if that would be workable. I suspect that you
would need to do quite an involved sequence in "git branch" to
make this sequence to work with .git/deleted-refs/ scheme:
git branch frotz/nitfol
git pack-refs --prune
git branch -d frotz/nitfol
git branch frotz
git pack-refs
git branch -d frotz
After deleting frotz/nitfol you would create frotz/nitfol in
deleted hierarchy. Then when you delete frotz you would need to
create frotz in deleted hierarchy, but you cannot, without
losing frotz/nitfol.
^ permalink raw reply
* Re: [RFC/PATCH] gitweb: Add committags support
From: Jakub Narebski @ 2006-09-23 8:34 UTC (permalink / raw)
To: Petr Baudis, git
In-Reply-To: <20060923032948.GE8259@pasky.or.cz>
Petr "Pasky" Baudis wrote:
> Dear diary, on Thu, Sep 21, 2006 at 11:56:31PM CEST, I got a letter
> where Jakub Narebski <jnareb@gmail.com> said that...
> > + # The committag subroutine is called with match for pattern,
> > + # and options if they are defined. Match is replaced by return
> > + # value of committag-sub.
>
> You should note that the pattern matches in an already HTML-escaped
> text. Which is actually quite ugly especially wrt. the , perhaps
> it would be worth considering converting the spaces after this? (The
> Marc links would need fixing tho'.)
Perhaps we better use "white-space: pre;" for commit (and tag) messages
instead of 'manually' doing this via converting ' ' to ' '.
> ..snip..
> > + 'message_id' => {
> > + 'pattern' => qr/(message|msg)[- ]?id <([^&]*)&rt;/i,
> > + 'enabled' => 1,
> > + 'options' => [
> > + 'http://news.gmane.org/find-root.php?message_id=',
> > + \"e_msgid_gmane ],
> > + 'sub' => \&tag_msgid},
>
> Mention quote_msgid_marc() (and the URL) in a comment so that it's not
> completely unreferenced?
Well, that is RFC. I'm not sure if the final patch would have all those
committags, all those committags but disabled with exception of commitsha/sha,
or just commitsha tag in source and the rest in commit message as examples.
For MARC:
'options' => [
'http://marc.theaimsgroup.com/?i=',
\"e_msgid_marc ],
> > +);
> ..snip..
> > +sub quote_msgid_gmane {
> > + my $msgid = shift || return;
> > +
> > + return '<'.(quotemeta $msgid).'>';
> > +}
>
> <> should be HTML-escaped (unless CGI::a() does that).
CGI::a() probably does that. But true, "<" and ">" should be "params quoted":
return '%3c' . (quotemeta $msgid) . '%3e';
(Quotemeta is because Message-Id contains '@').
> > @@ -577,18 +693,43 @@ ## which don't beling to other sections
> ..snip..
> > - if ($line =~ m/([0-9a-fA-F]{40})/) {
> > - my $hash_text = $1;
> > - if (git_get_type($hash_text) eq "commit") {
> > - my $link =
> > - $cgi->a({-href => href(action=>"commit", hash=>$hash_text),
> > - -class => "text"}, $hash_text);
> > - $line =~ s/$hash_text/$link/;
> > +
> > + foreach my $ct (@tags) {
> > + next unless exists $committags{$ct};
>
> At this point, I'd complain (even die) rather than silently pass,
> that's definitely a bug.
I've programmed defensively. Perhaps too defensively.
> > + my $wrap = $a_attr && %$a_attr && $committags{$ct}{'islink'};
>
> $a_attr can't be but a hashref, that test is redundant.
$a_attr can be undefined or be a hashref. It could theoretically
be empty hashref, but that is a mistake (<a>...</a>).
> > @@ -626,12 +767,13 @@ sub format_subject_html {
> > $extra = '' unless defined($extra);
> >
> > if (length($short) < length($long)) {
> > - return $cgi->a({-href => $href, -class => "list subject",
> > - -title => $long},
> > - esc_html($short) . $extra);
> > + my $a_attr = {-href => $href, -class => "list subject", -title => $long};
> > + return $cgi->a($a_attr,
> > + format_log_line_html($short, $a_attr, @subjecttags) . $extra);
> > } else {
> > - return $cgi->a({-href => $href, -class => "list subject"},
> > - esc_html($long) . $extra);
> > + my $a_attr = {-href => $href, -class => "list subject"};
> > + return $cgi->a($a_attr,
> > + format_log_line_html($long, $a_attr, @subjecttags) . $extra);
> > }
> > }
> >
>
> Subjects are often clickable and we don't want links in those.
The extra code with $a_attr is to have links within links. It works
quite well, I'd say. The subject link is broken, and the committag
link is inserted in the break (gitweb-xmms2 committag code did the same,
but did not preserved all the subject link attributes, like title or class,
only the target of the link).
The result is somethink like:
<a href="..." class="subject" ...>Fix </a><a href="...=137">BUG(137)</a><a href="..." class="subject" ...>: ...</a>
> > @@ -693,9 +835,9 @@ sub git_get_type {
> >
> > open my $fd, "-|", git_cmd(), "cat-file", '-t', $hash or return;
> > my $type = <$fd>;
> > - close $fd or return;
> > + close $fd or return "unknown";
> > chomp $type;
> > - return $type;
> > + return ($type || "unknown");
> > }
> >
> > sub git_get_project_config {
>
> > P.S. I've corrected git_get_type for comparison
> > git_get_type($hash) eq "commit"
> > to work without Perl syntax errors.
>
> Hey it's just a warning. ;-) And if you are calling git_get_type() on a
> non-existing object, don't you have a problem you should better know
> about? Couldn't this break git_get_refs_list()?
There are many cherry-picked comments with commitsha of object which
existed on some topic branch, was pruned, and does not exist anymore.
Perhaps we should add "2> /dev/null", too...
I've not tested how this change works with git_get_refs_list(), and other
subroutines that use git_get_type().
> > @@ -1585,7 +1727,7 @@ sub git_print_log ($;%) {
> > $empty = 0;
> > }
> >
> > - print format_log_line_html($line) . "<br/>\n";
> > + print format_log_line_html($line, @committags) . "<br/>\n";
> > }
> >
> > if ($opts{'-final_empty_line'}) {
>
> What about the tags? Or perhaps even blobs, for that matter?
What about? In commit messages you usually reference other commits
(as: this correct some commit, this finishes what was started in commit,
this reverts commit (!), cherry-picked commit).
Thanks for all the comments.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH 2/6] many cleanups to sha1_file.c
From: Junio C Hamano @ 2006-09-23 8:44 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0609210004550.2627@xanadu.home>
Nicolas Pitre <nico@cam.org> writes:
> Those cleanups are mainly to set the table for the support of deltas
> with base objects referenced by offsets instead of sha1. This means
> that many pack lookup functions are converted to take a pack/offset
> tuple instead of a sha1.
There still remains some "struct pack_entry" and I wonder if it
would make sense to get rid of them if only for consistency's
sake.
> diff --git a/sha1_file.c b/sha1_file.c
> index b89edb9..6fae766 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -926,8 +925,8 @@ static int packed_delta_info(unsigned ch
>
> memset(&stream, 0, sizeof(stream));
>
> - data = stream.next_in = base_sha1 + 20;
> - stream.avail_in = left - 20;
> + data = stream.next_in = (unsigned char *) p->pack_base + offset;;
> + stream.avail_in = p->pack_size - offset;
> stream.next_out = delta_head;
> stream.avail_out = sizeof(delta_head);
>
";;"?
> @@ -989,75 +988,60 @@ int check_reuse_pack_delta(struct packed
> ...
> +static int packed_object_info(struct packed_git *p, unsigned long offset,
> char *type, unsigned long *sizep)
> {
> + unsigned long size;
> enum object_type kind;
>
> - if (use_packed_git(p))
> - die("cannot map packed file");
> + offset = unpack_object_header(p, offset, &kind, &size);
>
Do all callers of packed_object_info() call use_packed_git to
make sure p is mapped? Ah sha1_object_info() is the only one
except packed_delta_info() that calls itself and it protects it
with use_packed_git(), so you are safe.
We would need to revamp use/unuse code when we implement the
partial mapping anyway, but we still need to get this right for
now.
^ permalink raw reply
* Re: Git user survey and `git pull`
From: Jakub Narebski @ 2006-09-23 8:54 UTC (permalink / raw)
To: git
In-Reply-To: <20060921162401.GD3934@spearce.org>
Shawn Pearce wrote:
> git-pull git-pull --merge
And we can even have "git-pull --merge=<branch to merge to>"
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH 4/6] gitweb: Link to associated tree from a particular log item in full log view
From: Jakub Narebski @ 2006-09-23 8:59 UTC (permalink / raw)
To: git
In-Reply-To: <7vodt7xvgu.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
> Petr Baudis <pasky@suse.cz> writes:
>
>> Oops. It's trivial typo:
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index d2366c7..c5f3810 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -2880,7 +2880,7 @@ sub git_log {
>> " | " .
>> $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") .
>> " | " .
>> - $cgi->a({-href => href(action=>"tree", hash=>$commit), hash_base=>$commit}, "tree") .
>> + $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree") .
>> "<br/>\n" .
>> "</div>\n" .
>> "<i>" . esc_html($co{'author_name'}) . " [$ad{'rfc2822'}]</i><br/>\n" .
>>
>>> You do not have the tree object name available in git_log to
>>> generate an URL with both h and hb, and getting to it is an
>>> extra work.
>>
>> Well using commit hash as a tree id works just fine...
>
> I agree that change is much safer. Will apply with an
> appropriate log message and attribution. No need to resend.
I think that hash_base without hash should be enough. But I'm not
sure if it is. Well, commit hash is tree-ish...
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] Fix snapshot link in tree view
From: Jakub Narebski @ 2006-09-23 9:01 UTC (permalink / raw)
To: git
In-Reply-To: <20060922232120.596.63045.stgit@rover>
By the way, using "[PATCH] gitweb: Fix snapshot link in tree view" would
make this patch easier to read as gitweb patch.
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH 1/2] diff.c: second war on whitespace.
From: Jakub Narebski @ 2006-09-23 9:15 UTC (permalink / raw)
To: git
In-Reply-To: <7vbqp7vxb8.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
> gmane.comp.version-control.git
> This adds DIFF_WHITESPACE color class (default = reverse red) to
> colored diff output to let you catch common whitespace errors.
>
> - trailing whitespaces at the end of line
> - a space followed by a tab in the indent
In gitweb/gitweb.perl, around %feature hash definition, we have:
<TAB> # <SP> <TAB> 'sub' ...
and I think that is exception to the "no space followed by tab" rule.
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH 1/2] diff.c: second war on whitespace.
From: Junio C Hamano @ 2006-09-23 9:37 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <ef2u05$f29$1@sea.gmane.org>
Jakub Narebski <jnareb@gmail.com> writes:
> Junio C Hamano wrote:
>
>> gmane.comp.version-control.git
>> This adds DIFF_WHITESPACE color class (default = reverse red) to
>> colored diff output to let you catch common whitespace errors.
>>
>> - trailing whitespaces at the end of line
>> - a space followed by a tab in the indent
>
> In gitweb/gitweb.perl, around %feature hash definition, we have:
>
> <TAB> # <SP> <TAB> 'sub' ...
>
> and I think that is exception to the "no space followed by tab" rule.
Read the description again. No space IN INDENT that is followed
by tab.
^ permalink raw reply
* [PATCH 0/2 (take 3)] Fetch: branch properties.
From: Santi Béjar @ 2006-09-23 10:04 UTC (permalink / raw)
To: git
Hi *,
This patch series implements the following branch properties for fetch:
- default repository to fetch
- default remotes branches to merge from the default repository
Documentation/config.txt | 7 +++++++
git-fetch.sh | 9 ++++-----
git-parse-remote.sh | 34 ++++++++++++++++++++++++++--------
3 files changed, 37 insertions(+), 13 deletions(-)
Santi
^ permalink raw reply
* [PATCH 1/2] Fetch: default remote repository from branch properties
From: Santi Béjar @ 2006-09-23 10:05 UTC (permalink / raw)
To: git
In-Reply-To: <87venex5je.fsf@gmail.com>
If in branch "foo" and this in config:
[branch "foo"]
remote=bar
"git fetch" = "git fetch bar"
"git pull" = "git pull bar"
Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
Documentation/config.txt | 3 +++
git-fetch.sh | 9 ++++-----
git-parse-remote.sh | 6 ++++++
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index bb2fbc3..04c5094 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -119,6 +119,9 @@ apply.whitespace::
Tells `git-apply` how to handle whitespaces, in the same way
as the '--whitespace' option. See gitlink:git-apply[1].
+branch.<name>.remote::
+ When in branch <name>, it tells `git fetch` which remote to fetch.
+
pager.color::
A boolean to enable/disable colored output when the pager is in
use (default is true).
diff --git a/git-fetch.sh b/git-fetch.sh
index 09a5d6c..50ad101 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -68,11 +68,10 @@ done
case "$#" in
0)
- test -f "$GIT_DIR/branches/origin" ||
- test -f "$GIT_DIR/remotes/origin" ||
- git-repo-config --get remote.origin.url >/dev/null ||
- die "Where do you want to fetch from today?"
- set origin ;;
+ origin=$(get_default_remote)
+ test -n "$(get_remote_url ${origin})" ||
+ die "Where do you want to fetch from today?"
+ set x $origin ; shift ;;
esac
remote_nick="$1"
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 187f088..6999816 100755
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -68,6 +68,12 @@ get_remote_url () {
esac
}
+get_default_remote () {
+ curr_branch=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
+ origin=$(git-repo-config --get "branch.$curr_branch.remote")
+ echo ${origin:-origin}
+}
+
get_remote_default_refs_for_push () {
data_source=$(get_data_source "$1")
case "$data_source" in
--
1.4.2.1.g94da7
^ permalink raw reply related
* [PATCH 2/2] Fetch: get the remote branches to merge from the branch properties
From: Santi Béjar @ 2006-09-23 10:08 UTC (permalink / raw)
To: git
In-Reply-To: <87venex5je.fsf@gmail.com>
If in branch "foo" and this in config:
[branch "foo"]
merge=bar
"git fetch": fetch from the default repository and program the "bar"
branch to be merged with pull.
Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
Documentation/config.txt | 4 ++++
git-parse-remote.sh | 30 ++++++++++++++++++++++--------
2 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 04c5094..98c1f3e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -122,6 +122,10 @@ apply.whitespace::
branch.<name>.remote::
When in branch <name>, it tells `git fetch` which remote to fetch.
+branch.<name>.merge::
+ When in branch <name>, it tells `git fetch` the default remote branch
+ to be merged.
+
pager.color::
A boolean to enable/disable colored output when the pager is in
use (default is true).
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 6999816..d3a1b9a 100755
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -92,9 +92,22 @@ get_remote_default_refs_for_push () {
# Subroutine to canonicalize remote:local notation.
canon_refs_list_for_fetch () {
- # Leave only the first one alone; add prefix . to the rest
+ # If called from get_remote_default_refs_for_fetch
+ # leave the branches in branch.${curr_branch}.merge alone,
+ # or the first one otherwise; add prefix . to the rest
# to prevent the secondary branches to be merged by default.
- dot_prefix=
+ if test "$1" = "-d"
+ then
+ shift ; remote="$1" ; shift
+ if test "$remote" = "$(get_default_remote)"
+ then
+ curr_branch=$(git-symbolic-ref HEAD | \
+ sed -e 's|^refs/heads/||')
+ merge_branches=$(git-repo-config \
+ --get-all "branch.${curr_branch}.merge")
+ fi
+ fi
+ [ -z "$merge_branches" ] && merge_branches=$1
for ref
do
force=
@@ -107,6 +120,11 @@ canon_refs_list_for_fetch () {
expr "z$ref" : 'z.*:' >/dev/null || ref="${ref}:"
remote=$(expr "z$ref" : 'z\([^:]*\):')
local=$(expr "z$ref" : 'z[^:]*:\(.*\)')
+ dot_prefix=.
+ for merge_branch in $merge_branches
+ do
+ [ "$remote" = "$merge_branch" ] && dot_prefix= && break
+ done
case "$remote" in
'') remote=HEAD ;;
refs/heads/* | refs/tags/* | refs/remotes/*) ;;
@@ -126,7 +144,6 @@ canon_refs_list_for_fetch () {
die "* refusing to create funny ref '$local_ref_name' locally"
fi
echo "${dot_prefix}${force}${remote}:${local}"
- dot_prefix=.
done
}
@@ -137,7 +154,7 @@ get_remote_default_refs_for_fetch () {
'' | config-partial | branches-partial)
echo "HEAD:" ;;
config)
- canon_refs_list_for_fetch \
+ canon_refs_list_for_fetch -d "$1" \
$(git-repo-config --get-all "remote.$1.fetch") ;;
branches)
remote_branch=$(sed -ne '/#/s/.*#//p' "$GIT_DIR/branches/$1")
@@ -145,10 +162,7 @@ get_remote_default_refs_for_fetch () {
echo "refs/heads/${remote_branch}:refs/heads/$1"
;;
remotes)
- # This prefixes the second and later default refspecs
- # with a '.', to signal git-fetch to mark them
- # not-for-merge.
- canon_refs_list_for_fetch $(sed -ne '/^Pull: */{
+ canon_refs_list_for_fetch -d "$1" $(sed -ne '/^Pull: */{
s///p
}' "$GIT_DIR/remotes/$1")
;;
--
1.4.2.1.g94da7
^ permalink raw reply related
* Re: [PATCH 1/2] Fetch: default remote repository from branch properties
From: Junio C Hamano @ 2006-09-23 10:40 UTC (permalink / raw)
To: Santi Béjar; +Cc: git
In-Reply-To: <87r6y2x5hk.fsf@gmail.com>
You would need a new test in the testsuite for this.
-- >8 --
[PATCH] Add t5510 to test per branch configuration affecting git-fetch.
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
t/t5510-fetch.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 44 insertions(+), 0 deletions(-)
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
new file mode 100755
index 0000000..e71581a
--- /dev/null
+++ b/t/t5510-fetch.sh
@@ -0,0 +1,44 @@
+#!/bin/sh
+# Copyright (c) 2006, Junio C Hamano.
+
+test_description='Per branch config variables affects "git fetch".
+
+'
+
+. ./test-lib.sh
+
+D=`pwd`
+
+test_expect_success setup '
+ echo >file original &&
+ git add file &&
+ git commit -a -m original'
+
+test_expect_success "clone and setup child repos" '
+ git clone . one &&
+ cd one &&
+ echo >file updated by one &&
+ git commit -a -m "updated by one" &&
+ cd .. &&
+ git clone . two &&
+ cd two &&
+ git repo-config branch.master.remote one &&
+ {
+ echo "URL: ../one/.git/"
+ echo "Pull: refs/heads/master:refs/heads/one"
+ } >.git/remotes/one
+'
+
+test_expect_success "fetch test" '
+ cd "$D" &&
+ echo >file updated by origin &&
+ git commit -a -m "updated by origin" &&
+ cd two &&
+ git fetch &&
+ test -f .git/refs/heads/one &&
+ mine=`git rev-parse refs/heads/one` &&
+ his=`cd ../one && git rev-parse refs/heads/master` &&
+ test "z$mine" = "z$his"
+'
+
+test_done
--
1.4.2.1.ga89e
^ permalink raw reply related
* Re: [PATCH 2/2] Fetch: get the remote branches to merge from the branch properties
From: Junio C Hamano @ 2006-09-23 10:43 UTC (permalink / raw)
To: Santi Béjar; +Cc: git
In-Reply-To: <877izux5d2.fsf@gmail.com>
Santi Béjar <sbejar@gmail.com> writes:
> If in branch "foo" and this in config:
>
> [branch "foo"]
> merge=bar
>
> "git fetch": fetch from the default repository and program the "bar"
> branch to be merged with pull.
>
> Signed-off-by: Santi Béjar <sbejar@gmail.com>
This still breaks t5700 test the same way as the earlier one.
^ permalink raw reply
* Re: [PATCH] Remove branch by putting a null sha1 into the ref file.
From: Christian Couder @ 2006-09-23 11:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vu02zuhya.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > If we move ".git/refs/heads/frotz/nitfol"
> > to ".git/deleted-refs/heads/frotz/nitfol" when we remove this ref, we
> > only need to try to rmdir all subdirectories under
> > ".git/refs/heads/frotz/" and then ".git/refs/heads/frotz/" to see if we
> > can
> > create ".git/refs/heads/frotz", and if we can, we won't
> > resurect "frotz/nitfol" because ".git/deleted-refs/heads/frotz/nitfol"
> > still exists.
>
> I am not sure if that would be workable. I suspect that you
> would need to do quite an involved sequence in "git branch" to
> make this sequence to work with .git/deleted-refs/ scheme:
>
> git branch frotz/nitfol
> git pack-refs --prune
> git branch -d frotz/nitfol
> git branch frotz
> git pack-refs
> git branch -d frotz
>
> After deleting frotz/nitfol you would create frotz/nitfol in
> deleted hierarchy. Then when you delete frotz you would need to
> create frotz in deleted hierarchy, but you cannot, without
> losing frotz/nitfol.
You are right, so what about moving ".git/refs/heads/frotz"
to ".git/deleted-refs/heads/frotz.ref"
or ".git/deleted-refs/heads/frotz~ref" (because "~" is forbidden in ref
names).
Christian.
^ permalink raw reply
* Re: Git user survey and `git pull`
From: Alan Chandler @ 2006-09-23 11:51 UTC (permalink / raw)
To: git
In-Reply-To: <ef1j6j$8sn$1@sea.gmane.org>
On Friday 22 September 2006 22:05, Matthias Urlichs wrote:
> For what it's worth, I'm in favor of renaming the things.
Me too. I am continually coming back to things after a couple of months and
having to relearn everything from scratch. I never over the hump of even
becoming an intermediate user of anything.
--
Alan Chandler
http://www.chandlerfamily.org.uk
^ permalink raw reply
* Re: [RFC/PATCH] gitweb: Add committags support
From: Petr Baudis @ 2006-09-23 12:11 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <200609231034.49545.jnareb@gmail.com>
Dear diary, on Sat, Sep 23, 2006 at 10:34:49AM CEST, I got a letter
where Jakub Narebski <jnareb@gmail.com> said that...
> Perhaps we better use "white-space: pre;" for commit (and tag) messages
> instead of 'manually' doing this via converting ' ' to ' '.
+1
> > ..snip..
> > > + 'message_id' => {
> > > + 'pattern' => qr/(message|msg)[- ]?id <([^&]*)&rt;/i,
> > > + 'enabled' => 1,
> > > + 'options' => [
> > > + 'http://news.gmane.org/find-root.php?message_id=',
> > > + \"e_msgid_gmane ],
> > > + 'sub' => \&tag_msgid},
> >
> > Mention quote_msgid_marc() (and the URL) in a comment so that it's not
> > completely unreferenced?
>
> Well, that is RFC. I'm not sure if the final patch would have all those
> committags, all those committags but disabled with exception of commitsha/sha,
> or just commitsha tag in source and the rest in commit message as examples.
I'd disable mantis and bugzilla by default (but leave them in the code
commented out) since those are totally project-specific, but a lot of
OSS projects use gmane and URL and commitsha are universally beneficial.
Also, there is a fundamental limitation for the multi-word patterns that
they won't work if the line wraps at that point in the log message. This
will likely be a problem especially for the msgids, because those are
very long and are very likely to cause a linewrap immediately before.
By the way, I don't think taking just 40-digits sha1s is very useful,
since that's insanely long and besides the linewrap issue, a lot of
people just shorten that to some 8 to 12 digits now - I'd use {8-40}
instead (the enforced minimum is 4 in the Git autocompletion code but we
shouldn't encourage people to write so ambiguous sha1s to persistent
records).
> > > +);
> > ..snip..
> > > +sub quote_msgid_gmane {
> > > + my $msgid = shift || return;
> > > +
> > > + return '<'.(quotemeta $msgid).'>';
> > > +}
> >
> > <> should be HTML-escaped (unless CGI::a() does that).
>
> CGI::a() probably does that. But true, "<" and ">" should be "params quoted":
>
> return '%3c' . (quotemeta $msgid) . '%3e';
Ah, sillly me, I didn't notice that it goes into the URL.
> (Quotemeta is because Message-Id contains '@').
Hmm, and at which point would that be eaten?
> > > @@ -577,18 +693,43 @@ ## which don't beling to other sections
> > > + my $wrap = $a_attr && %$a_attr && $committags{$ct}{'islink'};
> >
> > $a_attr can't be but a hashref, that test is redundant.
>
> $a_attr can be undefined or be a hashref. It could theoretically
> be empty hashref, but that is a mistake (<a>...</a>).
Yes so no point in testing the emptiness. But this is just nitpicking.
> > > @@ -626,12 +767,13 @@ sub format_subject_html {
> > > $extra = '' unless defined($extra);
> > >
> > > if (length($short) < length($long)) {
> > > - return $cgi->a({-href => $href, -class => "list subject",
> > > - -title => $long},
> > > - esc_html($short) . $extra);
> > > + my $a_attr = {-href => $href, -class => "list subject", -title => $long};
> > > + return $cgi->a($a_attr,
> > > + format_log_line_html($short, $a_attr, @subjecttags) . $extra);
> > > } else {
> > > - return $cgi->a({-href => $href, -class => "list subject"},
> > > - esc_html($long) . $extra);
> > > + my $a_attr = {-href => $href, -class => "list subject"};
> > > + return $cgi->a($a_attr,
> > > + format_log_line_html($long, $a_attr, @subjecttags) . $extra);
> > > }
> > > }
> > >
> >
> > Subjects are often clickable and we don't want links in those.
>
> The extra code with $a_attr is to have links within links. It works
> quite well, I'd say. The subject link is broken, and the committag
> link is inserted in the break (gitweb-xmms2 committag code did the same,
> but did not preserved all the subject link attributes, like title or class,
> only the target of the link).
>
> The result is somethink like:
>
> <a href="..." class="subject" ...>Fix </a><a href="...=137">BUG(137)</a><a href="..." class="subject" ...>: ...</a>
I don't think this is good idea though - if I'm clicking at links, I
don't want to have to carefully watch where that bit of the link leads.
IMHO this would be just annoying.
> > > @@ -693,9 +835,9 @@ sub git_get_type {
> > >
> > > open my $fd, "-|", git_cmd(), "cat-file", '-t', $hash or return;
> > > my $type = <$fd>;
> > > - close $fd or return;
> > > + close $fd or return "unknown";
> > > chomp $type;
> > > - return $type;
> > > + return ($type || "unknown");
> > > }
> > >
> > > sub git_get_project_config {
> >
> > > P.S. I've corrected git_get_type for comparison
> > > git_get_type($hash) eq "commit"
> > > to work without Perl syntax errors.
> >
> > Hey it's just a warning. ;-) And if you are calling git_get_type() on a
> > non-existing object, don't you have a problem you should better know
> > about? Couldn't this break git_get_refs_list()?
>
> There are many cherry-picked comments with commitsha of object which
> existed on some topic branch, was pruned, and does not exist anymore.
Then I think it's better to explicitly state that "in this case, this
object may not exist" by checking the return value for undef first.
> Perhaps we should add "2> /dev/null", too...
Yes that would be sensible.
> > > @@ -1585,7 +1727,7 @@ sub git_print_log ($;%) {
> > > $empty = 0;
> > > }
> > >
> > > - print format_log_line_html($line) . "<br/>\n";
> > > + print format_log_line_html($line, @committags) . "<br/>\n";
> > > }
> > >
> > > if ($opts{'-final_empty_line'}) {
> >
> > What about the tags? Or perhaps even blobs, for that matter?
>
> What about? In commit messages you usually reference other commits
> (as: this correct some commit, this finishes what was started in commit,
> this reverts commit (!), cherry-picked commit).
I meant that we should consider substituting the committags in those as
well.
--
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)
^ permalink raw reply
* Re: [PATCH] Fix snapshot link in tree view
From: Petr Baudis @ 2006-09-23 12:16 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <ef2t4n$bcp$3@sea.gmane.org>
Dear diary, on Sat, Sep 23, 2006 at 11:01:15AM CEST, I got a letter
where Jakub Narebski <jnareb@gmail.com> said that...
> By the way, using "[PATCH] gitweb: Fix snapshot link in tree view" would
> make this patch easier to read as gitweb patch.
Sorry, I try to mark those patches as gitweb but sometimes I forget. :-)
--
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)
^ permalink raw reply
* Re: [PATCH] Make cvsexportcommit work with filenames containing spaces.
From: Robin Rosenberg @ 2006-09-23 12:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vvenfw727.fsf@assigned-by-dhcp.cox.net>
lördag 23 september 2006 06:17 skrev Junio C Hamano:
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> > Binary files are except to this so far.
>
> Funny. This works on my home machine (CVS 1.12.13) while it
> does not pass test #2 "with spaces" on another machine that has
> CVS 1.11.22.
That must be something else. I've tried with various cvs versions here
(including plain 1.11.22) on Mandriva and SLES and it works on all with the
lastest git on master (and 1.4.2.1).
Could you give me some more info, like the output from -v on the failing
machine and your perl and patch version numbers and locale? I'm just guessing
what could matter here.
Why patch? Well this patch works around (i.e. not perfect) a mismatch between
what patch eats and git submits. They are not totally, compatible, and I'm
not sure who to blame yet. git emits diff's without timestamps, and what
matters to patch, without a TAB before the file timestamp. When patch sees a
header like "+++ filename with spaces.txt" it patches "filename". When it
sees "+++ filename with spaces.txt<TAB>" if patches "filename with
spaces.txt". The real fix would ofcourse be in git diff or patch sometime in
the future.
> > +. ./test-lib.sh
> > +
> > +export CVSROOT=$(dirname $(pwd))/cvsroot
> > +export CVSWORK=$(dirname $(pwd))/cvswork
acknowledged
> You are creating t/{cvsroot,cvswork} directories. Do not
> contaminate outside the test/trash directory your tests is
> started in.
>
> > +rm -rf $CVSROOT $CVSWORK
>
> People's $(pwd) can contain shell $IFS characters, especially on
> Cygwin. Be careful and quote them when in doubt.
Considering this is a patch that should improve on whitespace handlung... ok.
I'll resubmit later.
-- robin
^ permalink raw reply
* Re: [RFC][PATCH] gitweb: Make the Git logo link target to point to the homepage
From: Petr Baudis @ 2006-09-23 12:57 UTC (permalink / raw)
To: junkio; +Cc: git
In-Reply-To: <20060919212725.GA13132@pasky.or.cz>
Dear diary, on Tue, Sep 19, 2006 at 11:27:25PM CEST, I got a letter
where Petr Baudis <pasky@suse.cz> said that...
> It provides more useful information for causual Git users than the Git docs
> (especially about where to get Git and such).
>
> Signed-off-by: Petr Baudis <pasky@suse.cz>
Ping? This is the only gitweb patch still in my stg stack. I guess
noone really cares strongly either way since there were no comments.
--
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)
^ permalink raw reply
* Re: git pull for update of netdev fails.
From: Catalin Marinas @ 2006-09-23 13:10 UTC (permalink / raw)
To: Petr Baudis; +Cc: Johannes Schindelin, Linus Torvalds, git
In-Reply-To: <20060923034407.GF8259@pasky.or.cz>
Petr,
(I'm slow at replying and even slower at implementing anything over
the next 2-3 weeks - paternity leave :-))
On 23/09/06, Petr Baudis <pasky@suse.cz> wrote:
> Dear diary, on Wed, Sep 20, 2006 at 11:14:25PM CEST, I got a letter
> where Johannes Schindelin <Johannes.Schindelin@gmx.de> said that...
> > Another, even more serious problems with rebasing: You can introduce a bug
> > by rebasing. Meaning: git-rebase can succeed, even compilation is fine,
> > but the sum of your patches, and the patches you are rebasing on, is
> > buggy. And there is _no_ way to bisect this, since the "good" version can
> > be gone for good.
>
> Yes, I agree that this really is a problem, but that's a fundamental
> limitation. At least for StGIT-maintained floating branches the latest
> bleeding edge StGIT could fix that. (Except that the problem outlined by
> Linus is present here as well, first prune will wipe your older patch
> versions and your patch log will be useless - Catalin? Can we store the
> older patch versions references in something like
> .git/refs/patches-old/?) And except that it does that only for you -
> there should be a way to conveniently mirror (clone+pull) the patch
> stack setup.
I wasn't following this thread (well, any thread in the last days) but
the current patch history implementation in StGIT is prune-safe as it
generates additional commits to keep the history. If you undo an
operation (push, refresh), the undo will be recorded in the patch
history (that's really immutable). However, deleting a patch would
delete the corresponding history log as well.
--
Catalin
^ permalink raw reply
* Re: git pull for update of netdev fails.
From: Catalin Marinas @ 2006-09-23 13:15 UTC (permalink / raw)
To: Petr Baudis; +Cc: Shawn Pearce, Johannes Schindelin, Linus Torvalds, git
In-Reply-To: <20060923040942.GG8259@pasky.or.cz>
On 23/09/06, Petr Baudis <pasky@suse.cz> wrote:
> Dear diary, on Sat, Sep 23, 2006 at 06:00:35AM CEST, I got a letter
> where Shawn Pearce <spearce@spearce.org> said that...
> > I can see the same concept of ref history being useful even for
> > core git-rebase and doing it this way would also give it to StGIT
> > without Catalin needing to change code.
>
> In my StGIT tree, I don't want to have arbitrary N-days cutoff point,
> I want all the patches history preserved at least as long as I carry
> the patch, because it's just as valuable as the "regular" project
> history to me.
That's how it currently works (the top of the log is in
refs/patches/<branch>/<patch>.log). I gave up the idea of using
reflogs for patch history.
--
Catalin
^ permalink raw reply
* Re: [RFC/PATCH] gitweb: Add committags support
From: Jakub Narebski @ 2006-09-23 13:33 UTC (permalink / raw)
To: Petr Baudis, git
In-Reply-To: <20060923121134.GM13132@pasky.or.cz>
Petr Baudis wrote:
> Dear diary, on Sat, Sep 23, 2006 at 10:34:49AM CEST, I got a letter
> where Jakub Narebski <jnareb@gmail.com> said that...
> Also, there is a fundamental limitation for the multi-word patterns that
> they won't work if the line wraps at that point in the log message. This
> will likely be a problem especially for the msgids, because those are
> very long and are very likely to cause a linewrap immediately before.
We do not wrap log messages in gitweb. So the problem is only when
commit message is wrongly wrapped itself (pre imples nowrap).
>> > > +);
>> > ..snip..
>> > > +sub quote_msgid_gmane {
>> > > + my $msgid = shift || return;
>> > > +
>> > > + return '<'.(quotemeta $msgid).'>';
>> > > +}
>> >
>> > <> should be HTML-escaped (unless CGI::a() does that).
>>
>> CGI::a() probably does that. But true, "<" and ">" should be "params quoted":
>>
>> return '%3c' . (quotemeta $msgid) . '%3e';
>
> Ah, silly me, I didn't notice that it goes into the URL.
Should it be "params quoted" or not? What are valid characters
in message-id? (And in which RFC it is defined?)
>> (Quotemeta is because Message-Id contains '@').
>
> Hmm, and at which point would that be eaten?
In the substitution phase... but perhaps I was to defensive.
s/$from/$to/g where $to can have '@'.
>> > > @@ -626,12 +767,13 @@ sub format_subject_html {
>> > > $extra = '' unless defined($extra);
>> > >
>> > > if (length($short) < length($long)) {
>> > > - return $cgi->a({-href => $href, -class => "list subject",
>> > > - -title => $long},
>> > > - esc_html($short) . $extra);
>> > > + my $a_attr = {-href => $href, -class => "list subject", -title => $long};
>> > > + return $cgi->a($a_attr,
>> > > + format_log_line_html($short, $a_attr, @subjecttags) . $extra);
>> > > } else {
>> > > - return $cgi->a({-href => $href, -class => "list subject"},
>> > > - esc_html($long) . $extra);
>> > > + my $a_attr = {-href => $href, -class => "list subject"};
>> > > + return $cgi->a($a_attr,
>> > > + format_log_line_html($long, $a_attr, @subjecttags) . $extra);
>> > > }
>> > > }
>> > >
>> >
>> > Subjects are often clickable and we don't want links in those.
>>
>> The extra code with $a_attr is to have links within links. It works
>> quite well, I'd say. The subject link is broken, and the committag
>> link is inserted in the break (gitweb-xmms2 committag code did the same,
>> but did not preserved all the subject link attributes, like title or class,
>> only the target of the link).
>>
>> The result is somethink like:
>>
>> <a href="..." class="subject" ...>Fix </a><a href="...=137">BUG(137)</a><a href="..." class="subject" ...>: ...</a>
>
> I don't think this is good idea though - if I'm clicking at links, I
> don't want to have to carefully watch where that bit of the link leads.
> IMHO this would be just annoying.
The committag links within subject link are clearly visually distinguished:
first they have default link color (blue for not visited, dark red for
visited links), second they are not bold width (as opposed to gitweb-xmms2,
where bold font was due to <b>...</b> element and not CSS styling
of a.subject).
I have just noticed that somehow git_log ("log" view) doesn't use
format_subject_line (perhaps because it is not shortened) and that
committags are not used there. And that is not only place where
subjecttags (committags in clickable subject line) are not used.
By the way, you can easily disable some committags in subject, either
removing 'insubject' field, or setting it to false.
>> > > @@ -1585,7 +1727,7 @@ sub git_print_log ($;%) {
>> > > $empty = 0;
>> > > }
>> > >
>> > > - print format_log_line_html($line) . "<br/>\n";
>> > > + print format_log_line_html($line, @committags) . "<br/>\n";
>> > > }
>> > >
>> > > if ($opts{'-final_empty_line'}) {
>> >
>> > What about the tags? Or perhaps even blobs, for that matter?
>>
>> What about? In commit messages you usually reference other commits
>> (as: this correct some commit, this finishes what was started in commit,
>> this reverts commit (!), cherry-picked commit).
>
> I meant that we should consider substituting the committags in those as
> well.
Ahh... For tags I guess it is a good idea (especially that I think that
fixes for bugtracker tracked bugs and feature request should be marked by tags,
e.g. b/<bugid>, to be used to link to commit/change/patch from bugtracker.
By the way, should we use some color for PGP signature block in signed tags?
--
Jakub Narebski
ShadeHawk on #git
Torun, Poland
^ permalink raw reply
* Re: [RFC/PATCH] gitweb: Add committags support
From: Petr Baudis @ 2006-09-23 14:05 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <200609231533.02455.jnareb@gmail.com>
Dear diary, on Sat, Sep 23, 2006 at 03:33:01PM CEST, I got a letter
where Jakub Narebski <jnareb@gmail.com> said that...
> Petr Baudis wrote:
>
> > Dear diary, on Sat, Sep 23, 2006 at 10:34:49AM CEST, I got a letter
> > where Jakub Narebski <jnareb@gmail.com> said that...
>
> > Also, there is a fundamental limitation for the multi-word patterns that
> > they won't work if the line wraps at that point in the log message. This
> > will likely be a problem especially for the msgids, because those are
> > very long and are very likely to cause a linewrap immediately before.
>
> We do not wrap log messages in gitweb. So the problem is only when
> commit message is wrongly wrapped itself (pre imples nowrap).
The commit message is not "wrongly" wrapped but just wrapped to fit into
72 or whatever columns. It would be silly to mandate users to use msg-id: <200609231533.02455.jnareb@gmail.com>
with the message id stretching far away just for the sake of some
gitweb limitations when having the message wrapped such as msg-id:
<200609231533.02455.jnareb@gmail.com> looks much more reasonable.
> >> > > +);
> >> > ..snip..
> >> > > +sub quote_msgid_gmane {
> >> > > + my $msgid = shift || return;
> >> > > +
> >> > > + return '<'.(quotemeta $msgid).'>';
> >> > > +}
> >> >
> >> > <> should be HTML-escaped (unless CGI::a() does that).
> >>
> >> CGI::a() probably does that. But true, "<" and ">" should be "params quoted":
> >>
> >> return '%3c' . (quotemeta $msgid) . '%3e';
> >
> > Ah, silly me, I didn't notice that it goes into the URL.
>
> Should it be "params quoted" or not? What are valid characters
> in message-id? (And in which RFC it is defined?)
RFC2822. Unfortunately there can be all kind of crap inside if you put
it inside quotes, so yes, it should.
What a pity that (apparently) noone supports RFC2392.
> >> (Quotemeta is because Message-Id contains '@').
> >
> > Hmm, and at which point would that be eaten?
>
> In the substitution phase... but perhaps I was to defensive.
> s/$from/$to/g where $to can have '@'.
$ perl -le 'my $x="a\@bc"; my $y="@"; $x =~ s/$y/$y.$y/g; print $x;'
a@.@bc
You probably should regexp-quote $from if you don't yet, though,
although a misbehaviour from that side is not very probable.
> >> > Subjects are often clickable and we don't want links in those.
> >>
> >> The extra code with $a_attr is to have links within links. It works
> >> quite well, I'd say. The subject link is broken, and the committag
> >> link is inserted in the break (gitweb-xmms2 committag code did the same,
> >> but did not preserved all the subject link attributes, like title or class,
> >> only the target of the link).
> >>
> >> The result is somethink like:
> >>
> >> <a href="..." class="subject" ...>Fix </a><a href="...=137">BUG(137)</a><a href="..." class="subject" ...>: ...</a>
> >
> > I don't think this is good idea though - if I'm clicking at links, I
> > don't want to have to carefully watch where that bit of the link leads.
> > IMHO this would be just annoying.
>
> The committag links within subject link are clearly visually distinguished:
> first they have default link color (blue for not visited, dark red for
> visited links), second they are not bold width (as opposed to gitweb-xmms2,
> where bold font was due to <b>...</b> element and not CSS styling
> of a.subject).
Ok, in that case it's better though I'm still feeling somewhat
uncomfortable about it. What if the _whole_ subject is just "Bug 1324"?
I can click the 'commit' link instead but it throws exception in my
brain I need to handle and I need to move my mouse around.
> > I meant that we should consider substituting the committags in those as
> > well.
>
> Ahh... For tags I guess it is a good idea (especially that I think that
> fixes for bugtracker tracked bugs and feature request should be marked by tags,
> e.g. b/<bugid>, to be used to link to commit/change/patch from bugtracker.
For blobs, the point is mainly comments, those can contain all kinds of
stuff.
> By the way, should we use some color for PGP signature block in signed tags?
Sounds like a good idea.
--
Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)
^ 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