Git development
 help / color / mirror / Atom feed
* Re: git-format-patch little gripe
From: Luben Tuikov @ 2006-11-03  9:59 UTC (permalink / raw)
  To: Martin Langhoff; +Cc: git
In-Reply-To: <46a038f90611030122reecee87ufac5bbaa910ee933@mail.gmail.com>

--- Martin Langhoff <martin.langhoff@gmail.com> wrote:
> On 11/3/06, Luben Tuikov <ltuikov@yahoo.com> wrote:
> > Yep, after more than a year, I simply cannot get used to it...
> > http://marc.theaimsgroup.com/?l=git&m=113259043217761&w=2
> > And as I've seen, other people brought that up too.
> 
> Hi Luben,
> 
> reading the thread, it sounds like you have a couple of shells scripts
> or aliases that do what you want already ;-)

Yeah, would you know it those scripts got lost somewhere.  I now
know better -- branch off of git "next" and store in USB key. :-)
So I generally work off of "next" merged to my own branch which includes
extras I've collected along the way.

But what wouldn't I give to have
  git-format-_patch_ -o /tmp/ <commit>
generate a _single_ patch just as its name implies...

   Luben


^ permalink raw reply

* Re: [PATCH] Allow hand-editing of patches before sending
From: Karl Hasselström @ 2006-11-03 10:01 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: git
In-Reply-To: <20061103095859.GC16721@diana.vm.bytemark.co.uk>

On 2006-11-03 10:58:59 +0100, Karl Hasselström wrote:

> I believe all the mails I send with mutt are QP-encoded,

I just checked, and that one certainly was.

-- 
Karl Hasselström, kha@treskal.com

^ permalink raw reply

* Re: [PATCH] Allow hand-editing of patches before sending
From: Petr Baudis @ 2006-11-03 10:21 UTC (permalink / raw)
  To: Karl Hasselström; +Cc: Catalin Marinas, git
In-Reply-To: <20061103100142.GD16721@diana.vm.bytemark.co.uk>

Dear diary, on Fri, Nov 03, 2006 at 11:01:42AM CET, I got a letter
where Karl Hasselström <kha@treskal.com> said that...
> On 2006-11-03 10:58:59 +0100, Karl Hasselström wrote:
> 
> > I believe all the mails I send with mutt are QP-encoded,
> 
> I just checked, and that one certainly was.

Are you sure? As far as I can see, it's 8bit.

-- 
				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

^ permalink raw reply

* [PATCH 5/10] gitweb: New improved patchset view
From: Jakub Narebski @ 2006-11-03 10:26 UTC (permalink / raw)
  To: git
In-Reply-To: <200610311522.32228.jnareb@gmail.com>

Replace "gitweb diff header" with its full sha1 of blobs and replace
it by "git diff" header and extended diff header. Change also somewhat
highlighting of diffs.

Added `file_type_long' subroutine to convert file mode in octal to
file type description (only for file modes which used by git).

Changes:
* "gitweb diff header" which looked for example like below:
    file:_<sha1 before>_ -> file:_<sha1 after>_
  where 'file' is file type and '<sha1>' is full sha1 of blob is
  changed to
    diff --git _a/<file before>_ _b/<file after>_
  In both cases links are visible and use default link style. If file
  is added, a/<file> is not hyperlinked, if file is deleted, b/<file>
  is not hyperlinked.
* there is added "extended diff header", with <path> and <hash>
  hyperlinked (and <hash> shortened to 7 characters), and <mode>
  explained: '<mode>' is extended to '<mode> (<file type description>)',
  where added text is slightly lighter to easy distinguish that it
  was added (and it is difference from git-diff output).
* from-file/to-file two-line header lines have slightly darker color
  than removed/added lines.
* chunk header has now delicate line above for easier finding chunk
  boundary, and top margin of 2px, both barely visible.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
I'm very sorry, previous version was linewrapped.

As usual, you can see new gitweb in action (after all the series,
including this patch) at my site
  http://roke . dyndns . info/cgi-bin/gitweb/gitweb.cgi
(anti-anti-spam protected against "DOT info" being vger banned word).

You can doenload and view snapshots at:
  http://repo.or.cz/w/git/jnareb-git.git/gitweb/test:gitweb/snapshots/
(before means current 'master' gitweb, middle means after this patch,
after means after all patches in the series).

 gitweb/gitweb.css  |   66 +++++++++++++++----
 gitweb/gitweb.perl |  187 ++++++++++++++++++++++++++++++++++------------------
 2 files changed, 176 insertions(+), 77 deletions(-)

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index 0eda982..87dbc52 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css
@@ -291,40 +291,82 @@ td.mode {
 	font-family: monospace;
 }
 
-div.diff a.list {
+/* styling of diffs (patchsets): commitdiff and blobdiff views */
+div.diff.header,
+div.diff.extended_header {
+	white-space: normal;
+}
+
+div.diff.header {
+	font-weight: bold;
+
+	background-color: #edece6;
+
+	margin-top: 4px;
+	padding: 4px 0px 2px 0px;
+	border: solid #d9d8d1;
+	border-width: 1px 0px 1px 0px;
+}
+
+div.diff.header a.path {
+	text-decoration: underline;
+}
+
+div.diff.extended_header,
+div.diff.extended_header a.path,
+div.diff.extended_header a.hash {
+	color: #777777;
+}
+
+div.diff.extended_header .info {
+	color: #b0b0b0;
+}
+
+div.diff.extended_header {
+	background-color: #f6f5ee;
+	padding: 2px 0px 2px 0px;
+}
+
+div.diff a.path,
+div.diff a.hash {
 	text-decoration: none;
 }
 
-div.diff a.list:hover {
+div.diff a.path:hover,
+div.diff a.hash:hover {
 	text-decoration: underline;
 }
 
-div.diff.to_file a.list,
-div.diff.to_file,
+div.diff.to_file a.path,
+div.diff.to_file {
+	color: #007000;
+}
+
 div.diff.add {
 	color: #008800;
 }
 
-div.diff.from_file a.list,
-div.diff.from_file,
+div.diff.from_file a.path,
+div.diff.from_file {
+	color: #aa0000;
+}
+
 div.diff.rem {
 	color: #cc0000;
 }
 
 div.diff.chunk_header {
 	color: #990099;
+
+	border: dotted #ffe0ff;
+	border-width: 1px 0px 0px 0px;
+	margin-top: 2px;
 }
 
 div.diff.incomplete {
 	color: #cccccc;
 }
 
-div.diff_info {
-	font-family: monospace;
-	color: #000099;
-	background-color: #edece6;
-	font-style: italic;
-}
 
 div.index_include {
 	border: solid #d9d8d1;
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4554067..c22fcc5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -732,6 +732,32 @@ sub file_type {
 	}
 }
 
+# convert file mode in octal to file type description string
+sub file_type_long {
+	my $mode = shift;
+
+	if ($mode !~ m/^[0-7]+$/) {
+		return $mode;
+	} else {
+		$mode = oct $mode;
+	}
+
+	if (S_ISDIR($mode & S_IFMT)) {
+		return "directory";
+	} elsif (S_ISLNK($mode)) {
+		return "symlink";
+	} elsif (S_ISREG($mode)) {
+		if ($mode & S_IXUSR) {
+			return "executable";
+		} else {
+			return "file";
+		};
+	} else {
+		return "unknown";
+	}
+}
+
+
 ## ----------------------------------------------------------------------
 ## functions returning short HTML fragments, or transforming HTML fragments
 ## which don't beling to other sections
@@ -2055,7 +2081,9 @@ sub git_patchset_body {
 	my $patch_idx = 0;
 	my $in_header = 0;
 	my $patch_found = 0;
+	my $skip_patch = 0;
 	my $diffinfo;
+	my (%from, %to);
 
 	print "<div class=\"patchset\">\n";
 
@@ -2065,6 +2093,8 @@ sub git_patchset_body {
 
 		if ($patch_line =~ m/^diff /) { # "git diff" header
 			# beginning of patch (in patchset)
+			$skip_patch = 0;
+
 			if ($patch_found) {
 				# close previous patch
 				print "</div>\n"; # class="patch"
@@ -2074,96 +2104,123 @@ sub git_patchset_body {
 			}
 			print "<div class=\"patch\" id=\"patch". ($patch_idx+1) ."\">\n";
 
+			# read and prepare patch information
 			if (ref($difftree->[$patch_idx]) eq "HASH") {
+				# pre-parsed (or generated by hand)
 				$diffinfo = $difftree->[$patch_idx];
 			} else {
 				$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);
 			}
+			$from{'file'} = $diffinfo->{'from_file'} || $diffinfo->{'file'};
+			$to{'file'}   = $diffinfo->{'to_file'}   || $diffinfo->{'file'};
+			if ($diffinfo->{'status'} ne "A") { # not new (added) file
+				$from{'href'} = href(action=>"blob", hash_base=>$hash_parent,
+				                     hash=>$diffinfo->{'from_id'},
+				                     file_name=>$from{'file'});
+			}
+			if ($diffinfo->{'status'} ne "D") { # not deleted file
+				$to{'href'} = href(action=>"blob", hash_base=>$hash,
+				                   hash=>$diffinfo->{'to_id'},
+				                   file_name=>$to{'file'});
+			}
 			$patch_idx++;
 
-			# for now, no extended header, hence we skip empty patches
-			# companion to	next LINE if $in_header;
+			# for now we skip empty patches
 			if ($diffinfo->{'from_id'} eq $diffinfo->{'to_id'}) { # no change
 				$in_header = 1;
+				$skip_patch = 0;
 				next LINE;
 			}
 
-			if ($diffinfo->{'status'} eq "A") { # added
-				print "<div class=\"diff_info\">" . file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'to_id'}) . " (new)" .
-				      "</div>\n"; # class="diff_info"
-
-			} elsif ($diffinfo->{'status'} eq "D") { # deleted
-				print "<div class=\"diff_info\">" . file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'from_id'}) . " (deleted)" .
-				      "</div>\n"; # class="diff_info"
-
-			} elsif ($diffinfo->{'status'} eq "R" || # renamed
-			         $diffinfo->{'status'} eq "C" || # copied
-			         $diffinfo->{'status'} eq "2") { # with two filenames (from git_blobdiff)
-				print "<div class=\"diff_info\">" .
-				      file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'from_file'})},
-				              $diffinfo->{'from_id'}) .
-				      " -> " .
-				      file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'to_file'})},
-				              $diffinfo->{'to_id'});
-				print "</div>\n"; # class="diff_info"
-
-			} else { # modified, mode changed, ...
-				print "<div class=\"diff_info\">" .
-				      file_type($diffinfo->{'from_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-				                             hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'from_id'}) .
-				      " -> " .
-				      file_type($diffinfo->{'to_mode'}) . ":" .
-				      $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-				                             hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})},
-				              $diffinfo->{'to_id'});
-				print "</div>\n"; # class="diff_info"
+			# print "git diff" header
+			$patch_line =~ s!^(diff (.*?) )a/.*$!$1!;
+			if ($from{'href'}) {
+				$patch_line .= $cgi->a({-href => $from{'href'}, -class => "path"},
+				                       'a/' . esc_path($from{'file'}));
+			} else { # file was added
+				$patch_line .= 'a/' . esc_path($from{'file'});
+			}
+			$patch_line .= ' ';
+			if ($to{'href'}) {
+				$patch_line .= $cgi->a({-href => $to{'href'}, -class => "path"},
+				                       'b/' . esc_path($to{'file'}));
+			} else { # file was deleted
+				$patch_line .= 'b/' . esc_path($to{'file'});
 			}
 
-			#print "<div class=\"diff extended_header\">\n";
+			print "<div class=\"diff header\">$patch_line</div>\n";
+			print "<div class=\"diff extended_header\">\n";
 			$in_header = 1;
 			next LINE;
+		} else {
+			# actual skipping of empty patches
+			next LINE if $skip_patch;
 		} # start of patch in patchset
 
+		if ($in_header) {
+			if ($patch_line !~ m/^---/) {
+				# match <path>
+				if ($patch_line =~ s!^((copy|rename) from ).*$!$1! && $from{'href'}) {
+					$patch_line .= $cgi->a({-href=>$from{'href'}, -class=>"path"},
+					                        esc_path($from{'file'}));
+				}
+				if ($patch_line =~ s!^((copy|rename) to ).*$!$1! && $to{'href'}) {
+					$patch_line = $cgi->a({-href=>$to{'href'}, -class=>"path"},
+					                      esc_path($to{'file'}));
+				}
+				# match <mode>
+				if ($patch_line =~ m/\s(\d{6})$/) {
+					$patch_line .= '<span class="info"> (' .
+					               file_type_long($1) .
+					               ')</span>';
+				}
+				# match <hash>
+				if ($patch_line =~ m/^index/) {
+					my ($from_link, $to_link);
+					if ($from{'href'}) {
+						$from_link = $cgi->a({-href=>$from{'href'}, -class=>"hash"},
+						                     substr($diffinfo->{'from_id'},0,7));
+					} else {
+						$from_link = '0' x 7;
+					}
+					if ($to{'href'}) {
+						$to_link = $cgi->a({-href=>$to{'href'}, -class=>"hash"},
+						                   substr($diffinfo->{'to_id'},0,7));
+					} else {
+						$to_link = '0' x 7;
+					}
+					my ($from_id, $to_id) = ($diffinfo->{'from_id'}, $diffinfo->{'to_id'});
+					$patch_line =~ s!$from_id\.\.$to_id!$from_link..$to_link!;
+				}
+				print $patch_line . "<br/>\n";
 
-		if ($in_header && $patch_line =~ m/^---/) {
-			#print "</div>\n"; # class="diff extended_header"
-			$in_header = 0;
+			} else {
+				#$in_header && $patch_line =~ m/^---/;
+				print "</div>\n"; # class="diff extended_header"
+				$in_header = 0;
+
+				if ($from{'href'}) {
+					$patch_line = '--- a/' .
+					              $cgi->a({-href=>$from{'href'}, -class=>"path"},
+					                      esc_path($from{'file'}));
+				}
+				print "<div class=\"diff from_file\">$patch_line</div>\n";
 
-			my $file = $diffinfo->{'from_file'};
-			$file  ||= $diffinfo->{'file'};
-			$file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent,
-			                               hash=>$diffinfo->{'from_id'}, file_name=>$file),
-			                -class => "list"}, esc_path($file));
-			$patch_line =~ s|a/.*$|a/$file|g;
-			print "<div class=\"diff from_file\">$patch_line</div>\n";
+				$patch_line = <$fd>;
+				chomp $patch_line;
 
-			$patch_line = <$fd>;
-			chomp $patch_line;
+				#$patch_line =~ m/^+++/;
+				if ($to{'href'}) {
+					$patch_line = '+++ b/' .
+					              $cgi->a({-href=>$to{'href'}, -class=>"path"},
+					                      esc_html($to{'file'}));
+				}
+				print "<div class=\"diff to_file\">$patch_line</div>\n";
 
-			#$patch_line =~ m/^+++/;
-			$file    = $diffinfo->{'to_file'};
-			$file  ||= $diffinfo->{'file'};
-			$file = $cgi->a({-href => href(action=>"blob", hash_base=>$hash,
-			                               hash=>$diffinfo->{'to_id'}, file_name=>$file),
-			                -class => "list"}, esc_path($file));
-			$patch_line =~ s|b/.*|b/$file|g;
-			print "<div class=\"diff to_file\">$patch_line</div>\n";
+			}
 
 			next LINE;
 		}
-		next LINE if $in_header;
 
 		print format_diff_line($patch_line);
 	}
-- 
1.4.3.3

^ permalink raw reply related

* Re: git bug? + question
From: Santi Béjar @ 2006-11-03 10:27 UTC (permalink / raw)
  To: Martin Waitz; +Cc: Junio C Hamano, Karl Hasselström, Miles Bader, git
In-Reply-To: <20061103095905.GD7545@admingilde.org>

On 11/3/06, Martin Waitz <tali@admingilde.org> wrote:
> I think the most intuitive thing for pull would be to fetch into
> remotes/<remotename>/* and then to merge
> remotes/<remotename>/<currentbranch>.

Yes as the default branch to merge instead the first line, the problem
is that it changes the current behaviour. But I think the most
intuitive thing would be to record the branch it is based off at the
branch creation time. Something similar to my patch:

Oct 17 [PATCHv2] git-branch: Set branch properties
Message-ID: <87y7rf80es.fsf@gmail.com>

Note that it is for the "old" git-branch.sh.


^ permalink raw reply

* Re: [PATCH] Allow hand-editing of patches before sending
From: Robin Rosenberg @ 2006-11-03 10:31 UTC (permalink / raw)
  To: Petr Baudis; +Cc: Karl Hasselström, Catalin Marinas, git
In-Reply-To: <20061103102119.GO20017@pasky.or.cz>

fredag 03 november 2006 11:21 skrev Petr Baudis:
> Dear diary, on Fri, Nov 03, 2006 at 11:01:42AM CET, I got a letter
> where Karl Hasselström <kha@treskal.com> said that...
>
> > On 2006-11-03 10:58:59 +0100, Karl Hasselström wrote:
> > > I believe all the mails I send with mutt are QP-encoded,
> >
> > I just checked, and that one certainly was.
>
> Are you sure? As far as I can see, it's 8bit.

The patch mail was QP-encoded, not the other messasges in this thread.


^ permalink raw reply

* Re: [PATCH] Allow hand-editing of patches before sending
From: Andreas Ericsson @ 2006-11-03 10:36 UTC (permalink / raw)
  To: Petr Baudis; +Cc: Karl Hasselström, Catalin Marinas, git
In-Reply-To: <20061103102119.GO20017@pasky.or.cz>

Petr Baudis wrote:
> Dear diary, on Fri, Nov 03, 2006 at 11:01:42AM CET, I got a letter
> where Karl Hasselström <kha@treskal.com> said that...
>> On 2006-11-03 10:58:59 +0100, Karl Hasselström wrote:
>>
>>> I believe all the mails I send with mutt are QP-encoded,
>> I just checked, and that one certainly was.
> 
> Are you sure? As far as I can see, it's 8bit.
> 

8bit here too.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se

^ permalink raw reply

* Re: [PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same name as path
From: Jakub Narebski @ 2006-11-03 10:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vlkmseutr.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Junio C Hamano <junkio@cox.net> writes:
> 
>> I'd see if I can add some constructive comments on patches 5-10
>> tonight, but I'm in the middle of other things so don't hold
>> your breath ;-).
> 
> 7 and 9 look obviously good, so I've applied them without
> others.
> 
>         gitweb: Output also empty patches in "commitdiff" view

This patch in my opinion has no sense without having extended diff
header in commitdiff view, i.e. without "New improved patchset view"
(I have send non-line wrapped version).

>         gitweb: Better support for non-CSS aware web browsers

Thats independent from other changes, true.

> 5 is terminally linewrapped and rather big to comment on without
> comparing pre- and post- patch outputs, so I'll refrain from
> commenting on it. 

You can check out new gitweb at work at my site (when it is up)
  http://roke . dyndns . info/cgi-bin/gitweb/gitweb.cgi

>                    8 is "oops, I made a mistake when I did 5", 
> which discourages me even more from looking at 5 X-<.

Well, you wouldn't notice error corrected by 8 unless you have
files with funny filenames.
-- 
Jakub Narebski

^ permalink raw reply

* Re: [PATCH 2/4] Rename remote_only to display_mode
From: Andreas Ericsson @ 2006-11-03 10:51 UTC (permalink / raw)
  To: Andy Parkins; +Cc: git
In-Reply-To: <200611030841.05888.andyparkins@gmail.com>

Andy Parkins wrote:
> Digressing a little: what is the polite form of patches for git?  My strategy 
> with this set was to make each patch as small as possible to reach my end 
> point.  If those patches were okayed on the list, I could then do a "make 
> more beautiful" patch, which is really nothing to do with the original 
> changes to functionality but would make the code prettier.

I believe the order of preferrence goes: tested, concise, short.

Linus has a nasty habit of ending his mails with "totally untested 
ofcourse", which is not a good strategy to adopt if you want your 
patches included.

>  Really I'm asking 
> what level of intrusiveness of patch is not considered rude?  In making my 
> patches, should I ride rough-shod over current implementation and just do it 
> how I'd do it or should I try to fit in (as I did in this case)?
> 

If you *need* to change something, change it. If you *want* to change 
something just because it's not written the way you would write it, back 
away. If you think some interface you're using needs clearing up 
(codewise or with extra comments), send a separate patch for that so the 
actual feature/bugfix you're sending in doesn't drown in cosmetic 
changes to the interfaces the patch uses/touches.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se

^ permalink raw reply

* [PATCH] Add support to git-branch to show local and remote branches
From: Andy Parkins @ 2006-11-03 10:52 UTC (permalink / raw)
  To: git
In-Reply-To: <7v64dxl0bf.fsf@assigned-by-dhcp.cox.net>

Instead of storing a list of refnames in append_ref, a list of structures is
created.  Each of these stores the refname and a symbolic constant representing
its type.

The creation of the list is filtered based on a command line switch; no switch
means "local branches only", "-r" means "remote branches only" (as they always
did); but now "-a" means "local branches or remote branches".

As a side effect, the list is now not global, but allocated in print_ref_list()
where it used.

Also a memory leak is plugged, the memory allocated during the list creation
was never freed.  This is now done in the new function, tidy_ref_list()

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 builtin-branch.c |   95 +++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 77 insertions(+), 18 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 368b68e..6dd33ee 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -79,46 +79,100 @@ static void delete_branches(int argc, co
 	}
 }
 
-static int ref_index, ref_alloc;
-static char **ref_list;
+#define REF_UNKNOWN_TYPE    0x00
+#define REF_LOCAL_BRANCH    0x01
+#define REF_REMOTE_BRANCH   0x02
+#define REF_TAG             0x04
+
+struct ref_item {
+	char *name;
+	unsigned int type;
+};
+
+struct ref_list {
+	int index, alloc;
+	struct ref_item *list;
+	int type_wanted;
+};
 
 static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 		void *cb_data)
 {
-	if (ref_index >= ref_alloc) {
-		ref_alloc = alloc_nr(ref_alloc);
-		ref_list = xrealloc(ref_list, ref_alloc * sizeof(char *));
+	struct ref_list *ref_list = (struct ref_list*)(cb_data);
+	struct ref_item *newitem;
+	int type = REF_UNKNOWN_TYPE;
+
+	/* Detect type */
+	if (!strncmp(refname, "refs/heads/", 11)) {
+		type = REF_LOCAL_BRANCH;
+		refname += 11;
+	} else if (!strncmp(refname, "refs/remotes/", 13)) {
+		type = REF_REMOTE_BRANCH;
+		refname += 13;
+	} else if (!strncmp(refname, "refs/tags/", 10)) {
+		type = REF_TAG;
+		refname += 10;
+	}
+
+	/* Don't add type the caller doesn't want */
+	if ((type & ref_list->type_wanted) == 0) {
+		return 0;
+	}
+
+	/* Resize buffer */
+	if (ref_list->index >= ref_list->alloc) {
+		ref_list->alloc = alloc_nr(ref_list->alloc);
+		ref_list->list = xrealloc(ref_list->list,
+				ref_list->alloc * sizeof(struct ref_item));
 	}
 
-	ref_list[ref_index++] = xstrdup(refname);
+	/* Record the new item */
+	newitem = &(ref_list->list[ref_list->index++]);
+	newitem->name = xstrdup(refname);
+	newitem->type = type;
 
 	return 0;
 }
 
+static int tidy_ref_list( struct ref_list *ref_list )
+{
+	int i;
+	for (i = 0; i < ref_list->index; i++) {
+		free( ref_list->list[i].name );
+	}
+	free( ref_list->list );
+}
+
 static int ref_cmp(const void *r1, const void *r2)
 {
+	struct ref_item *c1 = (struct ref_item*)(r1),
+					*c2 = (struct ref_item*)(r2);
+	if( c1->type != c2->type )
+		return c1->type - c2->type;
 	return strcmp(*(char **)r1, *(char **)r2);
 }
 
-static void print_ref_list(int remote_only)
+static void print_ref_list( int type_wanted )
 {
 	int i;
 	char c;
+	struct ref_list ref_list;
 
-	if (remote_only)
-		for_each_remote_ref(append_ref, NULL);
-	else
-		for_each_branch_ref(append_ref, NULL);
+	memset( &ref_list, 0, sizeof( ref_list ) );
+	ref_list.type_wanted = type_wanted;
+	for_each_ref(append_ref, &ref_list);
 
-	qsort(ref_list, ref_index, sizeof(char *), ref_cmp);
+	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
 
-	for (i = 0; i < ref_index; i++) {
+	for (i = 0; i < ref_list.index; i++) {
 		c = ' ';
-		if (!strcmp(ref_list[i], head))
+		if (!strcmp(ref_list.list[i].name, head))
 			c = '*';
 
-		printf("%c %s\n", c, ref_list[i]);
+		printf("%c %s\n", c, ref_list.list[i].name);
 	}
+
+	tidy_ref_list( &ref_list );
 }
 
 static void create_branch(const char *name, const char *start,
@@ -160,9 +214,10 @@ static void create_branch(const char *na
 
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
-	int delete = 0, force_delete = 0, force_create = 0, remote_only = 0;
+	int delete = 0, force_delete = 0, force_create = 0;
 	int reflog = 0;
 	int i;
+	int type_wanted = REF_LOCAL_BRANCH;
 
 	git_config(git_default_config);
 
@@ -189,7 +244,11 @@ int cmd_branch(int argc, const char **ar
 			continue;
 		}
 		if (!strcmp(arg, "-r")) {
-			remote_only = 1;
+			type_wanted = REF_REMOTE_BRANCH;
+			continue;
+		}
+		if (!strcmp(arg, "-a")) {
+			type_wanted = REF_LOCAL_BRANCH | REF_REMOTE_BRANCH;
 			continue;
 		}
 		if (!strcmp(arg, "-l")) {
@@ -209,7 +268,7 @@ int cmd_branch(int argc, const char **ar
 	if (delete)
 		delete_branches(argc - i, argv + i, force_delete);
 	else if (i == argc)
-		print_ref_list(remote_only);
+		print_ref_list(type_wanted);
 	else if (i == argc - 1)
 		create_branch(argv[i], head, force_create, reflog);
 	else if (i == argc - 2)
-- 
1.4.3.2

^ permalink raw reply related

* Re: [PATCH] Allow hand-editing of patches before sending
From: Karl Hasselström @ 2006-11-03 10:53 UTC (permalink / raw)
  To: Andreas Ericsson; +Cc: Petr Baudis, Catalin Marinas, git
In-Reply-To: <454B1BC3.1070203@op5.se>

On 2006-11-03 11:36:51 +0100, Andreas Ericsson wrote:

> Petr Baudis wrote:
>
> > Dear diary, on Fri, Nov 03, 2006 at 11:01:42AM CET, I got a letter
> > where Karl Hasselström <kha@treskal.com> said that...
> >
> > > On 2006-11-03 10:58:59 +0100, Karl Hasselström wrote:
> > >
> > > > I believe all the mails I send with mutt are QP-encoded,
> > >
> > > I just checked, and that one certainly was.
> >
> > Are you sure? As far as I can see, it's 8bit.
>
> 8bit here too.

Spooky. When I get copies of my own mails via the git list, they are
QP-encoded; but when I bcc myself directly, I get them in 8bit. But
you all say you get them still 8bit-encoded. I'm not sure I even
_want_ to know what is happening there.

One thing is certain, though: When I sent 8bit-encoded patches from
StGIT to this list, the vger mail server added headers saying that it
didn't like 8bit, and had re-encoded the mail (to what, I don't recall
just now). It warned that in doing so, it had to make assumptions
about the charset used. The assumption it had made was that the text
was latin1, which is not so good when it is in fact utf8.

This all just strengthens my belief that StGIT should go to great
legths to avoid stepping on mail servers' toes.

-- 
Karl Hasselström, kha@treskal.com

^ permalink raw reply

* Re: [PATCH 3/4] Default to displaying /all/ non-tag refs, not just locals
From: Andreas Ericsson @ 2006-11-03 10:55 UTC (permalink / raw)
  To: Andy Parkins; +Cc: git
In-Reply-To: <200611030847.22252.andyparkins@gmail.com>

Andy Parkins wrote:
> On Friday 2006 November 03 02:40, Junio C Hamano wrote:
> 
>> That is a change in behaviour and given that we introduced
>> remotes for the explicit purpose of not to clutter the local
>> branch namespace, I doubt defaulting to _show_ remotes is a good
> 
> Really?  I had imagined it was to prevent accidental checking out of an 
> upstream-tracking branch.  Also; I don't think "not cluttering the namespace" 
> is the same as "not showing multiple namespaces".  The local namespace 
> remains as uncluttered as it ever was.  This is a question of what to 
> display.
> 
> Assuming my "mixed mode" display thing were in place, doesn't that make the 
> two choices of UI
> 
> 1)
>  git-branch            : show local branches
>  git-branch --all      : show local and remote branches
>  git-branch -r         : show remote branches
> 2)
>  git-branch            : show local and remote branches
>  git-branch --local    : show local branches
>  git-branch -r         : show remote branches
> 
> In case 2) the switch is simply selecting a filter, and so fits in with 
> the "-r" better.
> 

I think it'd make more sense if git-branch could instead take a --filter 
parameter that does a simple strncmp(filter, branch, strlen(filter)) to 
see if it should show a branch or not. That way, "--filter=remotes" 
would work splendidly. "local" as keyword to "--filter" could possibly 
be a special case and need documentation.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se

^ permalink raw reply

* Re: [PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of pathnames
From: Jakub Narebski @ 2006-11-03 10:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vlkmtgd3o.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index ec46b80..a15e916 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -563,12 +563,42 @@ sub esc_html {
> >  	return $str;
> >  }
> >  
> > +# quote unsafe characters and escape filename to HTML
> > +sub esc_path {
> > +	my $str = shift;
> > +	$str = esc_html($str);
> > +	$str =~ s/[[:cntrl:]\a\b\e\f\n\r\t\011]/?/g; # like --hide-control-chars in ls
> > +	return $str;
> > +}
> > +
> 
> When you say "[:cntrl:]" do you need to say anything more?

Ooops. Yes, the \a\b\e\f\n\r\t\011 part is redundant.

> >  # git may return quoted and escaped filenames
> >  sub unquote {
> >  	my $str = shift;
> > +
> > +	sub unq {
> > +		my $seq = shift;
> > +		my %es = (
> > +			't' => "\t", # tab            (HT, TAB)
> > +			'n' => "\n", # newline        (NL)
> > +			'r' => "\r", # return         (CR)
> > +			'f' => "\f", # form feed      (FF)
> > +			'b' => "\b", # backspace      (BS)
> > +			'a' => "\a", # alarm (bell)   (BEL)
> > +			#'e' => "\e", # escape        (ESC)
> > +			'v' => "\011", # vertical tab (VT)
> > +		);
> > +
> > +		# octal char sequence
> > +		return chr(oct($seq))  if ($seq =~ m/^[0-7]{1,3}$/);
> > +		# C escape sequence (this includes '\n' (LF) and '\t' (TAB))
> > +		return $es{$seq}       if ($seq =~ m/^[abefnrtv]$/);
> 
> Problems in this part of the code X-<.
> 
>  * Was there a reason not to unwrap '\e' to "\e"?

It was not mentioned in description of git pathname quoting in the
message
  http://marc.theaimsgroup.com/?l=git&m=112927316408690&w=2;

>  * The vertical tab is \013 (decimal 11), not \011 (which is TAB).

Oops. ASCII 11 is decimal 11.

>  * The name and the abbreviated name of the character "\n" are
>    "line feed" and "LF"; I personally do not think these
>    character name comments are needed in this part of the code,
>    but I do not object if you want to have them there, as long
>    as you spell them correctly. cf. ISO/IEC 6429:1992 or
>    http://www.unicode.org/charts/PDF/U0000.pdf for example.

Perhaps it should be "LF ('\n') and TAB ('\t')".

>  * The hash %es and the pattern /[abef...]/ must be kept in
>    sync; it is a maintenance nightmare,
> 
>  * Worse yet, they do not agree even in this initial version,
>    which proves the previous point.
> 
> Perhaps this is better written as:
> 
> 	if (exists $es{$seq}) {
>         	return $es{$seq};
> 	}

Fact.

Or
	return $es{$seq} if exists $es{$seq};

-- 
Jakub Narebski

^ permalink raw reply

* Re: [PATCH 6/n] gitweb: Remove redundant "blob" links from git_difftree_body
From: Jakub Narebski @ 2006-11-03 11:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v3b91jalp.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> ... First, it did that incompletely: it did not add "blob"
>> link for added files, and added block used mixture of tabs and spaces
>> for align. Second, in "difftree" view the "blob" link is not the most
>> interesting, *contrary* to "blob"/"tree" link in "tree" view, so it
>> should be enough to have hidden link in the form of file name entry.
> 
> I think these "blob" links are good thing to have, and if you
> think the earlier work was incomplete and know some cases are
> not covered I think it would be better to help completing it
> rather than reverting.
> 
> I do not understand why you feel "blob" is not the most
> interesting.  Often, when it is not obvious if a patch is
> correct only with the context, it is useful to view the whole
> postimage after applying the patch, and the "blob" link helps
> that.

O.K. I'll choose the "add blob links where there are none in difftree view"
approach in cleaned up and resend series.

I'll wait a while (a day or two) for further comments before redoing
the series.

P.S. Remove empty patches might produce incorrect HTML (one of <div>
is not closed). I'll correct it on resend.
-- 
Jakub Narebski

^ permalink raw reply

* Re: git-format-patch little gripe
From: Junio C Hamano @ 2006-11-03 11:17 UTC (permalink / raw)
  To: Luben Tuikov; +Cc: git
In-Reply-To: <511753.217.qm@web31807.mail.mud.yahoo.com>

Luben Tuikov <ltuikov@yahoo.com> writes:

> But what wouldn't I give to have
>   git-format-_patch_ -o /tmp/ <commit>
> generate a _single_ patch just as its name implies...

It would have been nicer if you made that argument as a reply to
one of these messages:

http://thread.gmane.org/gmane.comp.version-control.git/4266/focus=4279
http://thread.gmane.org/gmane.comp.version-control.git/4843/focus=4843
http://thread.gmane.org/gmane.comp.version-control.git/5440/focus=5446

It is not an ideology; it is called not breaking existing UI,
which is rather unfortunate, because its UI is not the greatest
in the world.

Back when it was done, it was not clear that we would have a
rich set of revision set operations as "--since=2.weeks",
"from..to", or "this...that" as we have today.  Even though we
have them now, these set operations do not mesh very well with
the patch-id based filtering format-patch needs to do.

In order to exclude patches that you have as commits that have
not been _merged_ into your upstream, but the change they
contain have already been _applied_, you would need "the other
set" which is roughly "rev-list yours..upstream".  Some of your
patches (i.e. "rev-list upstream..yours") may have already
applied to the upstream but obviously as different commits, and
you would fiter them by comparing the patch-ids of them and
those from "the other set".  

Unfortunately, other than the recent addition "this...that",
none of the revision set operation would give us the "other set"
that is efficient to use ("all the commits that is older than 2
weeks" is an obvious "other set" for "--since=2.weeks", but that
set goes all the way down to the initial commit, which is
obviously not what we want).

One thing we talked about but nobody stepped up to code [*1*] is
to give "git-format-patch --stdin" that reads list of commits,
and runs "git-diff-tree --pretty --stat --summary -p $commit".
With that, we could do something like:

	git rev-list linus..orinoco | git format-patch --stdin

Your "git-format-patch --single $commit" is a shorthand for a
degenerated special case of that pattern.

You cannot do patch-id based filtering with this form, but I see
that "single" is often wanted on the list and #git, and people
who want it do not care about patch-id based filtering at all.
And I do not think it is that "they do not realize how much they
would be missing without patch-id filtering", in this case.  So
the above command line would probably be Ok.

With --left-right (in "pu"), you could even do something a bit
fancier like this:

	git rev-list --left-right linus...8139cp linus...airo |
	git format-patch --stdin

The --left-right output option, when used in conjunction with
the symmetric difference set operator, prefixes each commit with
'<' (left) or '>' (right) to indicate which ancestry it belongs
to (in the above example, the commits only in the branch that
tracks Linus but not in branches 8139cp or airo are prefixed
with '<', and commits on 8139cp or airo branches that have not
been merged in linus are prefixed with '>').  --stdin could use
that and take '<' as "the other set", i.e. the ones to base
filtering of '>' commits on, and output the commits that came
with '>' prefix (but subtracting the ones that have equivalent
patches in the '<' set).


[Footnote]

*1* format-patch is primarily the tool for a patch submitter,
and I did its original version back when I was one.  For a long
time (an equivalent to "eternity" in git timescale back then)
Linus did not show _any_ interest in it (you can compare the
dates on the messages I quoted above with the commit date of
0acfc972), and I suspect one of the reasons is because he was
the toplevel maintainer and did not have a need for a tool like
that.  Now I am the toplevel maintainer here and I haven't felt
the need to update it myself for quite some time ("it works for
me"), which is a bit sad.


^ permalink raw reply

* Re: [PATCH] gitweb: Remove extra "/" in path names for git_get_project_list
From: Jakub Narebski @ 2006-11-03 11:18 UTC (permalink / raw)
  To: git
In-Reply-To: <7vslh1jcji.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:

> "Aneesh Kumar K.V" <aneesh.kumar@gmail.com> writes:
> 
>> Without this change we get a wrong $pfxlen value and the check_export_ok()
>> checks with with a wrong directory name. Without this patch the below
>> $projects_list fails with gitweb
>>
>> $projects_list = "/tmp/a/b/";
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@gmail.com>
> 
> Hmph.  Doesn't this break $projects_list = "/", I wonder?
> 
>> +            # remove the trailing "/"
>> +            $dir =~ s!/+$!!;

So perhaps

        $dir =~ s!(?<=[^/])/+$!!;

(zero-width positive look-behind assertion).
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git


^ permalink raw reply

* Re: [PATCH] Allow hand-editing of patches before sending
From: Junio C Hamano @ 2006-11-03 11:21 UTC (permalink / raw)
  To: Karl Hasselström; +Cc: Petr Baudis, Catalin Marinas, git
In-Reply-To: <20061103105349.GA18651@diana.vm.bytemark.co.uk>

Karl Hasselström <kha@treskal.com> writes:

> One thing is certain, though: When I sent 8bit-encoded patches from
> StGIT to this list, the vger mail server added headers saying that it
> didn't like 8bit, and had re-encoded the mail (to what, I don't recall
> just now). It warned that in doing so, it had to make assumptions
> about the charset used. The assumption it had made was that the text
> was latin1, which is not so good when it is in fact utf8.
>
> This all just strengthens my belief that StGIT should go to great
> legths to avoid stepping on mail servers' toes.

I wonder if this can be solved by simply reusing the machinery
format-patch already has.  If calling it as a standalone script
does more unnecessary things than what StGIT wants, we should
certainly be able to separate the only necessary part out to
suit StGIT's needs.


^ permalink raw reply

* Re: [PATCH] Allow hand-editing of patches before sending
From: Karl Hasselström @ 2006-11-03 11:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Petr Baudis, Catalin Marinas, git
In-Reply-To: <7vslh0bwsm.fsf@assigned-by-dhcp.cox.net>

On 2006-11-03 03:21:29 -0800, Junio C Hamano wrote:

> Karl Hasselström <kha@treskal.com> writes:
>
> > This all just strengthens my belief that StGIT should go to great
> > legths to avoid stepping on mail servers' toes.
>
> I wonder if this can be solved by simply reusing the machinery
> format-patch already has. If calling it as a standalone script does
> more unnecessary things than what StGIT wants, we should certainly
> be able to separate the only necessary part out to suit StGIT's
> needs.

Yes, I agree. Mail code tends to not be so beautiful that you want to
maintain more copies of it than absolutely necessary.

Of course, the same reasoning applies when importing mails into stgit.

-- 
Karl Hasselström, kha@treskal.com

^ permalink raw reply

* Re: [PATCH 2/4] Rename remote_only to display_mode
From: Junio C Hamano @ 2006-11-03 11:51 UTC (permalink / raw)
  To: Andy Parkins; +Cc: git, Andreas Ericsson
In-Reply-To: <454B1F3B.1020603@op5.se>

Andreas Ericsson <ae@op5.se> writes:

> Andy Parkins wrote:
>> Digressing a little: what is the polite form of patches for git?  My
>> strategy with this set was to make each patch as small as possible
>> to reach my end point.  If those patches were okayed on the list, I
>> could then do a "make more beautiful" patch, which is really nothing
>> to do with the original changes to functionality but would make the
>> code prettier.
>
> I believe the order of preferrence goes: tested, concise, short.
>
> Linus has a nasty habit of ending his mails with "totally untested
> ofcourse", which is not a good strategy to adopt if you want your
> patches included.

I've picked it up as well.  Consider it a privilege for being
the toplevel maintainer ;-).

Seriously, it is perfectly Ok to send "for discussion" feelers
that are untested or messy, but marking them clearly as "for
discussion only -- will clean-up after discussion" would be very
much appreciated.

The organization of our four series was almost perfect, except
you went a bit too far with [2/4].  I said "taken alone this is
regression in readability", with the full knowledge that the
real reason of the change was that it would not make any sense
to call the variable "remote_only" in [3/4].  IOW, I would have
rolled 2 and 3 into a single change.

> If you *need* to change something, change it. If you *want* to change
> something just because it's not written the way you would write it,
> back away. If you think some interface you're using needs clearing up
> (codewise or with extra comments), send a separate patch for that so
> the actual feature/bugfix you're sending in doesn't drown in cosmetic
> changes to the interfaces the patch uses/touches.

This is a very good advice.  I fully agree with Andreas.

When I was an active contributor, somebody (I do not remember
who) asked me privately: "you seem to be getting along pretty
well with Linus; I have these changes I want to send in, but can
you suggest a good strategy to get patches accepted?"  I recall
saying something along this line:

 - Make sure the patches apply cleanly to upstream (rebase if
   necessary).

 - When making a series, make clean-ups and obviously correct
   ones early, then follow them with bigger and potentially more
   controversial ones.  Doing it the other way around takes the
   "obviously correct" ones hostage to the latter; IOW, you
   would be effectively saying "if you swallow this big change,
   you would get these clean-ups and obviously correct
   bugfixes", which is not nice to the maintainer.

 - When some of your changes are accepted but others were not,
   do not give up, if you believe in the cause, try to come up
   with convincing examples to explain why your change helps
   certain workflows.

 - When doing so, make sure the patches apply cleanly to the
   updated upstream that now has some of your changes.  It might
   have been applied with fix-ups, and other people may have
   made clean-ups in neighbouring area.  You may need to re-roll
   the patch.

 - Linus is not doing this full-time and is a busy person.
   Although "not giving up" is important, do not push him too
   hard.  Try to guess how busy he is and what area his
   attention currently is.  The latter is important for two
   reasons.  (1) If you have to touch the same part of code but
   for a different reason, that would add more work on him.  It
   is better wait until the upstream code settles in the part of
   the code and base your patch on that.  (2) If your change
   needs a deep thinking to swallow, it is likely to be dropped
   when Linus's attention is in completely different area.


^ permalink raw reply

* Re: [PATCH 7/n] gitweb: Output also empty patches in "commitdiff" view
From: Jakub Narebski @ 2006-11-03 11:56 UTC (permalink / raw)
  To: git
In-Reply-To: <200610311736.27910.jnareb@gmail.com>

Jakub Narebski wrote:
> Remove skipping over empty patches (i.e. patches which consist solely
> of extended headers) in git_patchset_body, and add links to those
> header-only patches in git_difftree_body (but not generate blobdiff
> links when there were no change in file contents).

Attention: I think this patch causes that in some cases gitweb generates
incorrect HTML (one of <div> elements is not closed).
-- 
Jakub Narebski

^ permalink raw reply

* Re: [PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of pathnames
From: Junio C Hamano @ 2006-11-03 11:58 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <200611031159.02065.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> Perhaps it should be "LF ('\n') and TAB ('\t')".

Official terminology seems to call \t "HT", but my feeling is
that we would not need that comment there.

> Or
> 	return $es{$seq} if exists $es{$seq};

Although gitweb is full of that syntax, I personally do not like
it very much.  It is really hard to read when you are trying to
skim through the code quickly.  You would have to say "why
return it?  ah -- only when it exists, then it makes sense",
which is a hiccup that disrupts the thought process.

^ permalink raw reply

* Re: [PATCH 2/4] Rename remote_only to display_mode
From: Andy Parkins @ 2006-11-03 12:00 UTC (permalink / raw)
  To: git
In-Reply-To: <454B1F3B.1020603@op5.se>

On Friday 2006 November 03 10:51, Andreas Ericsson wrote:

> If you *need* to change something, change it. If you *want* to change
> something just because it's not written the way you would write it, back
> away. If you think some interface you're using needs clearing up
> (codewise or with extra comments), send a separate patch for that so the
> actual feature/bugfix you're sending in doesn't drown in cosmetic
> changes to the interfaces the patch uses/touches.

Thank you for the excellent advice.  What then would you suggest in the case 
in point?  I made as minimal a change as I could make; but that left the code 
a little bit bitty - I had press-ganged a variable into taking on another 
function and was using numeric literals that should really have been given 
meaning with #define?

My question is perhaps different from simply git-etiquette; it's should I 
prefer my patches to be minimal or neat?  If there is a more appropriate way 
of doing something should I do it or should I favour minimalism?

I've actually rewritten it now as per Junio's request, and while I'm happier 
with the code, it was much bigger change, that didn't really lend itself to 
being broken into smaller patches as did my first attempt.

I guess in the end it's a judgement call and the best thing to do is post it 
and see who shoots it down :-)


Andy

-- 
Dr Andy Parkins, M Eng (hons), MIEE

^ permalink raw reply

* [PATCH] Colourise git-branch output
From: Andy Parkins @ 2006-11-03 12:06 UTC (permalink / raw)
  To: git
In-Reply-To: <200611031052.16095.andyparkins@gmail.com>

I wanted to have a visual indication of which branches are local and which are
remote in git-branch -a output; however Junio was concerned that someone might
be using the output in a script.  This patch addresses the problem by colouring
the git-branch output - which in "auto" mode won't be activated.

I've based it off the colouring code for builtin-diff.c; which means there is a
branch.color configuration variable that needs setting to something before the
color will appear.

This patch chooses green for local, red for remote and bold green for current.

As yet, there is no support for changing the colors using the config file; but
it wouldn't be hard to add.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 builtin-branch.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 6dd33ee..de7f81e 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -5,6 +5,7 @@
  * Based on git-branch.sh by Junio C Hamano.
  */
 
+#include "color.h"
 #include "cache.h"
 #include "refs.h"
 #include "commit.h"
@@ -17,6 +18,38 @@ static const char builtin_branch_usage[]
 static const char *head;
 static unsigned char head_sha1[20];
 
+static int branch_use_color;
+static char branch_colors[][COLOR_MAXLEN] = {
+	"\033[m",	/* reset */
+	"",		/* PLAIN (normal) */
+	"\033[31m",	/* REMOTE (red) */
+	"\033[32m",	/* LOCAL (green) */
+	"\033[1;32m",	/* CURRENT (boldgreen) */
+};
+enum color_branch {
+	COLOR_BRANCH_RESET = 0,
+	COLOR_BRANCH_PLAIN = 1,
+	COLOR_BRANCH_REMOTE = 2,
+	COLOR_BRANCH_LOCAL = 3,
+	COLOR_BRANCH_CURRENT = 4,
+};
+
+int git_branch_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "branch.color")) {
+		branch_use_color = git_config_colorbool(var, value);
+		return 0;
+	}
+	return git_default_config(var, value);
+}
+
+const char *branch_get_color(enum color_branch ix)
+{
+	if (branch_use_color)
+		return branch_colors[ix];
+	return "";
+}
+
 static int in_merge_bases(const unsigned char *sha1,
 			  struct commit *rev1,
 			  struct commit *rev2)
@@ -157,6 +190,7 @@ static void print_ref_list( int type_wan
 	int i;
 	char c;
 	struct ref_list ref_list;
+	int color;
 
 	memset( &ref_list, 0, sizeof( ref_list ) );
 	ref_list.type_wanted = type_wanted;
@@ -165,11 +199,28 @@ static void print_ref_list( int type_wan
 	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
 
 	for (i = 0; i < ref_list.index; i++) {
+		switch( ref_list.list[i].type ) {
+			case REF_LOCAL_BRANCH:
+				color = COLOR_BRANCH_LOCAL;
+				break;
+			case REF_REMOTE_BRANCH:
+				color = COLOR_BRANCH_REMOTE;
+				break;
+			default:
+				color = COLOR_BRANCH_PLAIN;
+				break;
+		}
+
 		c = ' ';
-		if (!strcmp(ref_list.list[i].name, head))
+		if (!strcmp(ref_list.list[i].name, head)) {
 			c = '*';
+			color = COLOR_BRANCH_CURRENT;
+		}
 
-		printf("%c %s\n", c, ref_list.list[i].name);
+		printf("%c %s%s%s\n", c,
+				branch_get_color(color),
+				ref_list.list[i].name,
+				branch_get_color(COLOR_BRANCH_RESET));
 	}
 
 	tidy_ref_list( &ref_list );
@@ -219,7 +270,7 @@ int cmd_branch(int argc, const char **ar
 	int i;
 	int type_wanted = REF_LOCAL_BRANCH;
 
-	git_config(git_default_config);
+	git_config(git_branch_config);
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
-- 
1.4.3.2

^ permalink raw reply related

* Re: [PATCH] Allow hand-editing of patches before sending
From: Andy Whitcroft @ 2006-11-03 12:07 UTC (permalink / raw)
  To: Karl Hasselström; +Cc: Catalin Marinas, git
In-Reply-To: <20061103100142.GD16721@diana.vm.bytemark.co.uk>

Karl Hasselström wrote:
> On 2006-11-03 10:58:59 +0100, Karl Hasselström wrote:
> 
>> I believe all the mails I send with mutt are QP-encoded,
> 
> I just checked, and that one certainly was.

Mime-Version: 1.0
Content-Type:	text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: QUOTED-PRINTABLE

It reached me as quoted printable, with =20 on your signature intro.

-apw

^ permalink raw reply

* Re: [PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of pathnames
From: Jakub Narebski @ 2006-11-03 12:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vodro91ye.fsf@assigned-by-dhcp.cox.net>

Dnia piątek 3. listopada 2006 12:58, Junio C Hamano napisał:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Perhaps it should be "LF ('\n') and TAB ('\t')".
> 
> Official terminology seems to call \t "HT", but my feeling is
> that we would not need that comment there.

O.K. I'll remove that comment.

>> Or
>> 	return $es{$seq} if exists $es{$seq};
> 
> Although gitweb is full of that syntax, I personally do not like
> it very much.  It is really hard to read when you are trying to
> skim through the code quickly.  You would have to say "why
> return it?  ah -- only when it exists, then it makes sense",
> which is a hiccup that disrupts the thought process.

O.K., I'll change style from

	return chr(oct($seq))  if ($seq =~ m/^[0-7]{1,3}$/);
	return $es{$seq}       if exists $es{$seq};
	return $seq;

to the more readable form

	if ($seq =~ m/^[0-7]{1,3}$/) {
		return chr(oct($seq));
	} elsif (exists $es{$seq}) {
		return $es{$seq};
	}
	return $seq;

I think it is better to leave final "return $seq;" outside else block.
-- 
Jakub Narebski

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox