* Re: Git documentation at kernel.org
From: Junio C Hamano @ 2012-02-13 3:23 UTC (permalink / raw)
To: Jeff King
Cc: Scott Chacon, Matthieu Moy, Clemens Buchacher, ftpadmin,
Petr Onderka, git
In-Reply-To: <20120213003024.GA25794@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> That sounds great to me. I'd like to be link-compatible with the old
> kernel.org docs section (even if through redirects) so that old links
> work (assuming kernel.org gives us a wholesale redirect). Which means
> importing all of the docs for released versions. I don't know if the old
> kernel.org doc tree was saved anywhere, but if I understand correctly,
> they are identical to what's in the "git-htmldocs" repository (which I
> _thought_ Junio wasn't going to keep updating, but it seems pretty up to
> date).
I have been updating htmldocs/manpages repositories on "unless I forget"
basis every time the public 'master' gets updated, so they are updated as
frequently as they used to back when they were autogenerated, "unless I
forget".
But the contents of htmldocs does _not_ match what used to be at k.org in
that its git.html does not have links to documentation pages for older
releases, iow, formatted without "stalenotes" defined.
This is because formatting with "stalenotes" needs to make an assumption
on the filesystem layout that I cannot enforce to the users of htmldocs
repository. They will get one tarball for one version, and it is up to
them where they extract these tarballs. They need to extract the tarball
of an older release vX.Y.Z in vX.Y.Z subdirectory next to the git.html of
the latest living document to match the layout, but otherwise the links
created by "stalenotes" will become dangling.
^ permalink raw reply
* Re: [PATCH] strbuf: move strbuf_readline_fd() from bundle.c to strbuf.{c,h}
From: 徐迪 @ 2012-02-13 3:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git 邮件列表
In-Reply-To: <7v39agkmjf.fsf@alter.siamese.dyndns.org>
>> Shouldn't I supposed to receive any information about this patch?
>
> Not necessarily X-<.
>
> Things can get lost in the noise, and sending a reminder message like you
> just did, after waiting for a few to several days, is considered a good
> practice.
>
Ok, excuse me, but this is my first time to make a patch. I'm
desperately want to know its fate :)
>> Has it accepted or not?
>
> Anything and everything being considered for inclusion is reachable from
> the tip of the 'next' branch; other patches that might turn out to be of
> value, or ones that at least deserve more sets of eyeballs, are only in
> the 'pu' branch.
>
Does this mean that we can check 'next' branch to see if any of our
patches being considered for inclusion?
^ permalink raw reply
* Re: Bulgarian translation of git
From: Junio C Hamano @ 2012-02-13 3:56 UTC (permalink / raw)
To: Jiang Xin
Cc: Git List, Ævar Arnfjörð Bjarmason,
Alexander Shopov (Александър Шопов)
In-Reply-To: <CANYiYbGr3zN-kJwq_MCnttNZP6Cc0aj-fsZjd4V=4z+BA5TvUw@mail.gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Junio has a suggestion on how to contrib l10n for git, and you can see the
> discussion here: http://article.gmane.org/gmane.comp.version-control.git/189584.
>
> Seems that the suggested git-po repo has not been setup yet, so let me
> have a try.
>
> 1. Repositiry git-po is hosted on GitHub: https://github.com/gotgit/git-po/
>
> 2. I made a commit on the maint branch with a initial version of 'po/git.pot'
> https://github.com/gotgit/git-po/commit/4247a7a9d39e2a74ce1d58e5eb1f5e5d87977989
Somebody needs to eyeball this commit before anything else happens on top
of it, so that if there is a glaring mistake it can be caught before it
spreads to affect work by translators for various languages.
> 3. As l10n coordinator for Chinese, I translate against the pot file, and
> the translated po file 'po/zh_CN.po' is commited in my own branch:
> maint-zh-cn.
> You can find the commit in:
> https://github.com/gotgit/git-po/commits/maint-zh-cn
>
> 4. I will merge the commit in branch maint-zh-cn back or other l10n
> coordinators' contributes
> to maint branch.
>
> I have a question, which version of po should be maintained? master
> branch or maint branch.
I would say for this round the git-po repository and its pot file should
pick up whatever new translatable strings are added to 'master'.
After this is merged in 1.7.10, we may want to maintain separate tracks,
but at this moment there is no point maintaining something mergeable to
1.7.9.x maintenance track.
^ permalink raw reply
* Re: [PATCH 1/7] t5700: document a failure of alternates to affect fetch
From: Junio C Hamano @ 2012-02-13 3:35 UTC (permalink / raw)
To: mhagger; +Cc: git, Jeff King, Jakub Narebski, Heiko Voigt, Johan Herland
In-Reply-To: <1328941261-29746-2-git-send-email-mhagger@alum.mit.edu>
mhagger@alum.mit.edu writes:
> +test_expect_success 'prepare branched repository' '
> + git clone A J &&
> + (
> + cd J &&
> + git checkout -b other master^ &&
> + echo other > otherfile &&
s/ > / >/; but that is nothing I cannot fix locally.
> +test_expect_failure 'fetch with incomplete alternates' '
I am assuming that this "incomplete" means "this alternate helps reducing
the number of objects we need to fetch from the remote, but it does not
have everything objects we need, and we still need to fetch some from the
remote". Am I correct?
I do not think you meant the alternate repository is in some way corrupt,
but I am just making sure, because I found the phrasing a bit odd.
^ permalink raw reply
* Re: [PATCH 2/7] clone.c: move more code into the "if (refs)" conditional
From: Junio C Hamano @ 2012-02-13 3:35 UTC (permalink / raw)
To: mhagger; +Cc: git, Jeff King, Jakub Narebski, Heiko Voigt, Johan Herland
In-Reply-To: <1328941261-29746-3-git-send-email-mhagger@alum.mit.edu>
mhagger@alum.mit.edu writes:
> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> The bahavior of a bunch of code before the "if (refs)" statement also
> depends on whether refs is set, so make the logic clearer by shifting
> this code into the if statement.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> builtin/clone.c | 39 ++++++++++++++++++++-------------------
> 1 files changed, 20 insertions(+), 19 deletions(-)
Nice; the result is much easier to follow.
^ permalink raw reply
* Re: [PATCH 3/7] fetch-pack.c: rename some parameters from "path" to "refname"
From: Junio C Hamano @ 2012-02-13 3:47 UTC (permalink / raw)
To: mhagger; +Cc: git, Jeff King, Jakub Narebski, Heiko Voigt, Johan Herland
In-Reply-To: <1328941261-29746-4-git-send-email-mhagger@alum.mit.edu>
mhagger@alum.mit.edu writes:
> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> The parameters denote reference names, which are no longer 1:1 with
> filesystem paths.
These three functions are only used as callback from for_each_ref() so
they always get the full refname and nothing else (like a partial refname
like tags/v1.7.9), so calling them refname makes perfect sense.
Even though I generally try to stay away from this kind of naming churn
patches, but hopefully there is nothing in flight to cause horrible
conflict with it.
Thanks.
^ permalink raw reply
* Re: [PATCH 6/7] clone: do not add alternate references to extra_refs
From: Junio C Hamano @ 2012-02-13 4:00 UTC (permalink / raw)
To: mhagger; +Cc: git, Jeff King, Jakub Narebski, Heiko Voigt, Johan Herland
In-Reply-To: <1328941261-29746-7-git-send-email-mhagger@alum.mit.edu>
mhagger@alum.mit.edu writes:
> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> Alternate references are directly (and now, correctly) handled by
> fetch-pack, so there is no need to inform fetch-pack about them via
> the extra_refs back channel.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> builtin/clone.c | 12 ------------
> 1 files changed, 0 insertions(+), 12 deletions(-)
Very nice (the real niceness primarily comes from the previous step ;-).
Thanks.
^ permalink raw reply
* Re: [PATCH 1/7] t5700: document a failure of alternates to affect fetch
From: Michael Haggerty @ 2012-02-13 4:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Jakub Narebski, Heiko Voigt, Johan Herland
In-Reply-To: <7vsjifids5.fsf@alter.siamese.dyndns.org>
On 02/13/2012 04:35 AM, Junio C Hamano wrote:
> mhagger@alum.mit.edu writes:
>
>> +test_expect_success 'prepare branched repository' '
>> + git clone A J &&
>> + (
>> + cd J &&
>> + git checkout -b other master^ &&
>> + echo other > otherfile &&
>
> s/ > / >/; but that is nothing I cannot fix locally.
>
>> +test_expect_failure 'fetch with incomplete alternates' '
>
> I am assuming that this "incomplete" means "this alternate helps reducing
> the number of objects we need to fetch from the remote, but it does not
> have everything objects we need, and we still need to fetch some from the
> remote". Am I correct?
Correct. Feel free to improve the description if you can think of a
better way to describe it.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCHv2 1/4] refs: add common refname_match_patterns()
From: Michael Haggerty @ 2012-02-13 5:00 UTC (permalink / raw)
To: Tom Grennan; +Cc: pclouds, git, gitster, jasampler
In-Reply-To: <20120211191748.GC4903@tgrennan-laptop>
On 02/11/2012 08:17 PM, Tom Grennan wrote:
> Yes, I didn't explicitly state that the precedence is the order written
> and in correctly described the first case. How about?
>
> /**
> * Returns in highest to lowest precedence:
> * 1 with an empty patterns list
> * 0 if refname fnmatch()es any ^ prefaced pattern
> * 1 if refname fnmatch()es any other pattern
> * 0 otherwise
> */
Much better; thanks.
Please note that this choice of semantics limits its power. For
example, if the rule were instead (like with gitattributes(5)) "if more
than one pattern matches a refname, a later pattern overrides an earlier
pattern", then one could do things like
refs/remotes/*/* !refs/remotes/gitster/* refs/remotes/gitster/master
to include specific references within a hierarchy that is otherwise
excluded.
However, since rev-list apparently uses a rule more like the one that
you are proposing, it might be better to be consistent than to choose a
different convention.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Michael Haggerty @ 2012-02-13 5:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, pclouds, git, krh, jasampler
In-Reply-To: <7vlio9n5ym.fsf@alter.siamese.dyndns.org>
On 02/11/2012 09:13 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> The proposal, amended to use "^" instead of "!", is that
>>
>> git for-each-ref A B ^C
>>
>> should mean "the reference names A and B but not C". Therefore, the command
>>
>> git rev-list $(git for-each-ref A B ^C)
>>
>> , which consistency suggests should do the same thing as the first
>> command,...
>
> That is an utter rubbish that does not even deserve a response.
>
> Your argument is like saying
>
> git for-each-ref A
>
> and
>
> git for-each-ref $(git rev-parse A)
>
> should somehow magically produce the same (or related) result. The
> for-each-ref command operates on refname patterns, while rev-list and
> rev-parse takes object names.
Of *course* they operate on different namespaces. But part of the way
that revisions are selected using rev-list is by *selecting or excluding
refnames* from which it should crawl. How long do you think it will be
before somebody asks for rev-list to be able to crawl from a set of
branches *except for those that match some pattern*? (Hint: I more or
less asked for this feature in the "rubbish" email that you quoted.) At
that point we will have to say one of the following:
"Your suggestion is utter rubbish; just type 'git rev-list $(git
for-each-ref A B ^C)'."
This is an acceptable (though somewhat verbose) answer for the Unix
command line; for msgit users it is useless, and it probably cannot be
used in non-command-line scenarios like the gitk "edit view" dialog.
"Sure, but we have to invent yet another syntax because the one used by
for-each-ref conflicts with the existing syntax of rev-list." This
would be the standard git project practice of letting the UI accrete
into an ever more incomprehensible mess.
That is why I suggest that we choose a new syntax *now* for for-each-ref
(and "branch --list", "tag --list", etc) so that the same syntax can be
used later in rev-list, rev-parse, etc.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [RFC PATCH 0/3] git-p4: move to toplevel
From: Junio C Hamano @ 2012-02-13 6:17 UTC (permalink / raw)
To: Luke Diamand; +Cc: Pete Wyckoff, git, Vitor Antunes
In-Reply-To: <4F380ADB.4090304@diamand.org>
Luke Diamand <luke@diamand.org> writes:
> On 12/02/12 18:13, Pete Wyckoff wrote:
>> The git-p4 code is in a single python script down in
>> contrib/fast-import now. I'd like to move it up to the top-level
>> source directory of git to make it easier to build and
>> distribute. Git-p4 already takes advantage of the git
>> infrastructure for documentation and testing, as well as the
>> community support (Junio, many reviewers).
>
> About time this was done. There's still a few oddities around but far
> fewer than there used to be. I don't know if Junio has some rules on
> what a command needs before it graduates from contrib though.
I try not to play a dictator around here. The primary thing I hesitated so
far about git-p4 is that it is useless if you live in the open source only
world, iow without proprietary software.
^ permalink raw reply
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Junio C Hamano @ 2012-02-13 6:37 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Tom Grennan, pclouds, git, krh, jasampler
In-Reply-To: <4F389FB1.2070706@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Of *course* they operate on different namespaces. But part of the way
> that revisions are selected using rev-list is by *selecting or excluding
> refnames* from which it should crawl.
I am appalled if that is truly the understanding of yours, after having
taken more than a few patches from you to fairly core parts of Git.
"rev-list A ^B" does not say "include A and exclude B from which rev-list
should crawl" AT ALL. We _actively_ crawl from both A and B. It is that
what are reachable from B is painted in a color different from the color
in which we paint what are reachable from A.
A better pair you could have mentioned would be for-each-ref vs rev-parse
(not rev-list). What Tom wanted with "do not show the refs that match the
pattern" he originally wanted to give to "tag --list" would be
for-each-ref A ^B
that is "show ref that matches A but do not show if it also matches B",
while what you want to say is "I want to paint A in positive color and
paint B in negative color, and I want to get a canonical notation to do
so", it is spelled with rev-parse, not for-each-ref, like this:
rev-parse A ^B
In other words,
git rev-list $(git rev-parse A ^B)
would be the equivalent to "git rev-list A ^B".
Maybe you are troubled that there are multiple concepts of negation, which
ultimately comes from the undeniable fact that for-each-ref and rev-parse
operate on entities in different concept domain (refnames and objects)?
And if we decide to use "^", then these two different concepts of negation
are both expressed with the same operator "prefix ^", leading to
confusion?
I am kind of sympathetic to that argument, and it might be a better idea
to avoid using "^" as the negation for matching operator, in order to make
it more apparent to the users that for-each-ref and rev-parse operate on
different concepts. But at the same time, if you know these are distinct
concepts, using the same "^" operator as a consistent way to express the
"negation" that is applicable in each concept domain does make it easier
for users as they gain experience. I tend to avoid making things too hard
for experienced users for the sake of flattening the very early learning
curve when possible, because nobody will stay novice forever.
^ permalink raw reply
* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Michal Kiedrowicz @ 2012-02-13 6:41 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3y5s9rl3g.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > Reading diff output is sometimes very hard, even if it's colored,
> > especially if lines differ only in few characters. This is often
> > true when a commit fixes a typo or renames some variables or
> > functions.
> >
> This is certainly nice feature to have. I think most tools that
> implement side-by-side diff implement this too.
>
> > This commit teaches gitweb to highlight characters that are
> > different between old and new line. This should work in the
> > similar manner as in Trac or GitHub.
> >
> Doe you have URLs with good examples of such diff refinement
> highlighting (GNU Emacs ediff/emerge terminology) / highlighting
> differing segments (diff-highlight terminology)?
I haven't found *examples* on GitHub and Trac sites, but what about
these ones:
https://github.com/gitster/git/commit/8cad4744ee37ebec1d9491a1381ec1771a1ba795
http://trac.edgewall.org/changeset/10973
>
> > The code that compares lines is based on
> > contrib/diff-highlight/diff-highlight, except that it works with
> > multiline changes too. It also won't highlight lines that are
> > completely different because that would only make the output
> > unreadable.
> >
> So what does it look like? From the contrib/diff-highlight/README I
> guess that it finds common prefix and common suffix, and highlights
> the rest, which includes change.
Yes.
> It doesn't implement LCS / diff
> algorithm like e.g. tkdiff does for its diff refinement highlighting,
> isn't it?
>
I doesn't. I share the Jeff's opinion that:
a) Jeff's approach is "good enough"
b) LCS on bytes could be very confusing if it marked few sets of
characters.
> By completly different you mean that they do not have common prefix or
> common suffix (at least one of them), isn't it?
Yes, but I also don't highlight lines which prefix/suffix consists only
of whitespace (This is quite common). I would also consider ignoring
prefixes/suffixes with punctuation, like:
- * I like you.
+ * Alice had a little lamb.
>
> > Combined diffs are not supported but a following commit will change
> > it.
> >
> O.K.
>
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> > ---
> > gitweb/gitweb.perl | 82
> > ++++++++++++++++++++++++++++++++++++++++++++++++--- 1 files
> > changed, 77 insertions(+), 5 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index db61553..1a5b454 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -2322,7 +2322,7 @@ sub format_cc_diff_chunk_header {
> > # wrap patch (diff) line into a <div> (not to be used for diff
> > headers), # the line must be esc_html()'ed
> > sub format_diff_line {
> > - my ($line, $diff_class, $from, $to) = @_;
> > + my ($line, $diff_class) = @_;
>
> Why that change?
I think it fallout from previous patch where format_diff_line() stopped
using $from and $to. Will fix that.
>
> >
> > my $diff_classes = "diff";
> > $diff_classes .= " $diff_class" if ($diff_class);
> > @@ -4923,14 +4923,85 @@ sub print_inline_diff_lines {
> > print foreach (@$add);
> > }
> >
> > +# Highlight characters from $prefix to $suffix and escape HTML.
> > +# $str is a reference to the array of characters.
> > +sub esc_html_mark_range {
> > + my ($str, $prefix, $suffix) = @_;
> > +
> > + # Don't generate empty <span> element.
> > + if ($prefix == $suffix + 1) {
> > + return esc_html(join('', @$str), -nbsp=>1);
> > + }
> > +
> > + my $before = join('', @{$str}[0..($prefix - 1)]);
> > + my $marked = join('', @{$str}[$prefix..$suffix]);
> > + my $after = join('', @{$str}[($suffix + 1)..$#{$str}]);
>
> Eeeeeek! First you split into letters, in caller at that, then join?
> Why not pass striung ($str suggests string not array of characters),
> and use substr instead?
>
> [Please disregard this and the next paragraph at first reading]
I will rename $str to something more self describing.
>
> > +
> > + return esc_html($before, -nbsp=>1) .
> > + $cgi->span({-class=>'marked'}, esc_html($marked,
> > -nbsp=>1)) .
> > + esc_html($after,-nbsp=>1);
> > +}
>
> Anyway I have send to git mailing list a patch series, which in one of
> patches adds esc_html_match_hl($str, $regexp) to highlight matches in
> a string. Your esc_html_mark_range(), after a generalization, could
> be used as underlying "engine".
>
> Something like this, perhaps (untested):
>
> # Highlight selected fragments of string, using given CSS class,
> # and escape HTML. It is assumed that fragments do not overlap.
> # Regions are passed as list of pairs (array references).
> sub esc_html_hl {
> my ($str, $css_class, @sel) = @_;
> return esc_html($str) unless @sel;
>
> my $out = '';
> my $pos = 0;
>
> for my $s (@sel) {
> $out .= esc_html(substr($str, $pos, $s->[0] - $pos))
> if ($s->[0] - $pos > 0);
> $out .= $cgi->span({-class => $css_class},
> esc_html(substr($str, $s->[0],
> $s->[1] - $s->[0])));
>
> $pos = $m->[1];
> }
> $out .= esc_html(substr($str, $pos))
> if ($pos < length($str));
>
> return $out;
> }
>
> > +
> > +# Format removed and added line, mark changed part and HTML-format
> > them.
>
> You should probably ad here that this code is taken from
> diff-highlight in contrib area, isn't it?
True.
>
> > +sub format_rem_add_line {
> > + my ($rem, $add) = @_;
> > + my @r = split(//, $rem);
> > + my @a = split(//, $add);
> > + my ($esc_rem, $esc_add);
> > + my ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);
>
> It is not as much $prefix, as $prefix_len, isn't it?
Yes.
> Shouldn't
> $prefix / $prefix_len start from 0, not from 1?
It starts from 1 because it skips first +/-. It should become obvious
after reading the comment from last patch :).
+ # In combined diff we must ignore two +/- characters.
+ $prefix = 2 if ($is_combined);
+
>
> > + my ($prefix_is_space, $suffix_is_space) = (1, 1);
> > +
> > + while ($prefix < @r && $prefix < @a) {
> > + last if ($r[$prefix] ne $a[$prefix]);
> > +
> > + $prefix_is_space = 0 if ($r[$prefix] !~ /\s/);
> > + $prefix++;
> > + }
>
> Ah, I see that it is easier to find common prefix by treating string
> as array of characters.
>
> Though I wonder if it wouldn't be easier to use trick of XOR-ing two
> strings (see "Bitwise String Operators" in perlop(1)):
>
> my $xor = "$rem" ^ "$add";
>
> and finding starting sequence of "\0", which denote common prefix.
>
>
> Though this and the following is a nice implementation of
> algorithm... as it would be implemented in C. Nevermind, it might be
> good enough...
The splitting and comparing by characters is taken from diff-highlight.
I don't think it's worth changing here.
>
> > +
> > + while ($suffix_rem >= $prefix && $suffix_add >= $prefix) {
> > + last if ($r[$suffix_rem] ne $a[$suffix_add]);
> > +
> > + $suffix_is_space = 0 if ($r[$suffix_rem] !~ /\s/);
> > + $suffix_rem--;
> > + $suffix_add--;
> > + }
>
> BTW., perhaps using single negative $suffix_len instead of separate
> $suffix_rem_pos and $suffix_add_pos would make code simpler and easier
> to understand?
I'll try that.
>
> > +
> > + # Mark lines that are different from each other, but have
> > some common
> > + # part that isn't whitespace. If lines are completely
> > different, don't
> > + # mark them because that would make output unreadable,
> > especially if
> > + # diff consists of multiple lines.
> > + if (($prefix == 1 && $suffix_rem == $#r && $suffix_add ==
> > $#a)
> > + || ($prefix_is_space && $suffix_is_space)) {
>
> Micronit about style: in gitweb we put boolean operator at the end of
> continued line, not at beginning of next one.
>
> So this would be:
>
> + if (($prefix == 1 && $suffix_rem == $#r && $suffix_add ==
> $#a) ||
> + ($prefix_is_space && $suffix_is_space)) {
>
OK
> > + $esc_rem = esc_html($rem);
> > + $esc_add = esc_html($add);
> > + } else {
> > + $esc_rem = esc_html_mark_range(\@r, $prefix,
> > $suffix_rem);
> > + $esc_add = esc_html_mark_range(\@a, $prefix,
> > $suffix_add);
> > + }
> > +
> > + return format_diff_line($esc_rem, 'rem'),
> > + format_diff_line($esc_add, 'add');
>
> Please use spaces to align.
OK
>
> > +}
> > +
> > # HTML-format diff context, removed and added lines.
> > sub format_ctx_rem_add_lines {
> > - my ($ctx, $rem, $add) = @_;
> > + my ($ctx, $rem, $add, $is_combined) = @_;
> > my (@new_ctx, @new_rem, @new_add);
> > + my $num_add_lines = @$add;
>
> Why is this temporary variable needed? If you are not sure that ==
> operator enforces scalar context on both arguments you can always
> write
>
> scalar @$add == scalar @$rem
>
You read my mind.
> > +
> > + if (!$is_combined && $num_add_lines > 0 && $num_add_lines
> > == @$rem) {
> > + for (my $i = 0; $i < $num_add_lines; $i++) {
> > + my ($line_rem, $line_add) =
> > format_rem_add_line(
> > + $rem->[$i], $add->[$i]);
> > + push @new_rem, $line_rem;
> > + push @new_add, $line_add;
>
> The original contrib/diff-highlight works only for single changed line
> (single removed and single added). You make this code work also for
> the case where number of aded lines is equal to the number of removed
> lines, and assume that subsequent changed lines in preimage
> correcponds to subsequent changed lines in postimage, which is not
> always true:
>
> -foo
> -bar
> +baz
> +fooo
>
> This is not described in commit message, I think.
True. Note that in this particular case nothing should be highlighted
because corresponding lines don't have common prefix/suffix.
>
> > + }
> > + } else {
> > + @new_rem = map { format_diff_line(esc_html($_,
> > -nbsp=>1), 'rem') } @$rem;
> > + @new_add = map { format_diff_line(esc_html($_,
> > -nbsp=>1), 'add') } @$add;
> > + }
> >
> > @new_ctx = map { format_diff_line(esc_html($_, -nbsp=>1),
> > 'ctx') } @$ctx;
> > - @new_rem = map { format_diff_line(esc_html($_, -nbsp=>1),
> > 'rem') } @$rem;
> > - @new_add = map { format_diff_line(esc_html($_, -nbsp=>1),
> > 'add') } @$add;
> > return (\@new_ctx, \@new_rem, \@new_add);
> > }
> > @@ -4939,7 +5010,8 @@ sub format_ctx_rem_add_lines {
> > sub print_diff_lines {
> > my ($ctx, $rem, $add, $diff_style, $is_combined) = @_;
> >
> > - ($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem,
> > $add);
> > + ($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem,
> > $add,
> > + $is_combined);
>
> O.K., now the code depends on $is_combined
>
^ permalink raw reply
* Re: [PATCH 7/8] gitweb: Use different colors to present marked changes
From: Michal Kiedrowicz @ 2012-02-13 6:46 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3pqdksyg1.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > This makes use of the highlight diff feature.
> >
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> > ---
> > I decided to split mechanism (generate HTML page with <span>
> > elements that mark interesting fragments of diff output) from
> > politics (use these particular colors for this <span> elements),
> > but otherwise this commit may be squashed with the previous one.
> > These colors work for me but if someone comes out with better ones,
> > I'd be happy.
>
> I think it would be better squashed with previous patch, otherwise it
> is a bit not visible change...
OK, but please note that since previous patch HTML contains <span>
elements around differing segments of diff so the change exists. It
just isn't reflected by CSS.
>
> > gitweb/static/gitweb.css | 8 ++++++++
> > 1 files changed, 8 insertions(+), 0 deletions(-)
> >
> > diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> > index c7827e8..4f87d16 100644
> > --- a/gitweb/static/gitweb.css
> > +++ b/gitweb/static/gitweb.css
> > @@ -438,6 +438,10 @@ div.diff.add {
> > color: #008800;
> > }
> >
> > +div.diff.add span.marked {
> > + background-color: #77ff77;
> > +}
> > +
> > div.diff.from_file a.path,
> > div.diff.from_file {
> > color: #aa0000;
> > @@ -447,6 +451,10 @@ div.diff.rem {
> > color: #cc0000;
> > }
> >
> > +div.diff.rem span.marked {
> > + background-color: #ff7777;
> > +}
> > +
> > div.diff.chunk_header a,
> > div.diff.chunk_header {
> > color: #990099;
> > --
>
> I'd have to see those colors in use.
Then why don't you try it?
> BTW what colors other
> highlighting diff GUIs use?
>
AFAIK they just use darker red/green background than for the rest of
line.
^ permalink raw reply
* Re: [PATCH 8/8] gitweb: Highlight combined diffs
From: Michal Kiedrowicz @ 2012-02-13 6:48 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3ty2wsyt6.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > The highlightning of combined diffs is currently disabled. This is
> > because output from a combined diff is much harder to highlight
> > because it's not obvious which removed and added lines should be
> > compared.
> >
> > Moreover, code that compares added and removed lines doesn't care
> > about combined diffs. It only skips first +/- character, treating
> > second +/- as a line content.
> >
> > Let's start with a simple case: only highlight changes that come
> > from one parent, i.e. when every removed line has a corresponding
> > added line for the same parent. This way the highlightning cannot
> > get wrong. For example, following diffs would be highlighted:
> >
> > - removed line for first parent
> > + added line for first parent
> > context line
> > -removed line for second parent
> > +added line for second parent
> >
> > or
> >
> > - removed line for first parent
> > -removed line for second parent
> > + added line for first parent
> > +added line for second parent
> >
> > but following output will not:
> >
> > - removed line for first parent
> > -removed line for second parent
> > +added line for second parent
> > ++added line for both parents
> >
> That is a very reasonable approach.
Thanks.
>
> > Further changes may introduce more intelligent approach that better
> > handles combined diffs.
> >
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> > ---
> > gitweb/gitweb.perl | 40 +++++++++++++++++++++++++++++++++++++---
> > 1 files changed, 37 insertions(+), 3 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 1a5b454..2b6cb9e 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -4944,13 +4944,16 @@ sub esc_html_mark_range {
> >
> > # Format removed and added line, mark changed part and HTML-format
> > them. sub format_rem_add_line {
> > - my ($rem, $add) = @_;
> > + my ($rem, $add, $is_combined) = @_;
> > my @r = split(//, $rem);
> > my @a = split(//, $add);
> > my ($esc_rem, $esc_add);
> > my ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);
> > my ($prefix_is_space, $suffix_is_space) = (1, 1);
> >
> > + # In combined diff we must ignore two +/- characters.
> > + $prefix = 2 if ($is_combined);
> > +
>
> Errr... actually number of prefix is equalt to number of parents, so
> it might be in case of octopus merge more than 2.
OK
>
> > while ($prefix < @r && $prefix < @a) {
> > last if ($r[$prefix] ne $a[$prefix]);
> >
> > @@ -4988,11 +4991,42 @@ sub format_ctx_rem_add_lines {
> > my ($ctx, $rem, $add, $is_combined) = @_;
> > my (@new_ctx, @new_rem, @new_add);
> > my $num_add_lines = @$add;
> > + my $can_highlight;
> > +
> > + # Highlight if every removed line has a corresponding
> > added line.
> > + if ($num_add_lines > 0 && $num_add_lines == @$rem) {
> > + $can_highlight = 1;
> > +
> > + # Highlight lines in combined diff only if the
> > chunk contains
> > + # diff between the same version, e.g.
> > + #
> > + # - a
> > + # - b
> > + # + c
> > + # + d
> > + #
> > + # Otherwise the highlightling would be confusing.
> > + if ($is_combined) {
> > + for (my $i = 0; $i < $num_add_lines; $i++)
> > {
> > + my $prefix_rem =
> > substr($rem->[$i], 0, 2);
> > + my $prefix_add =
> > substr($add->[$i], 0, 2); +
> > + $prefix_rem =~ s/-/+/g;
> > +
> > + if ($prefix_rem ne $prefix_add) {
> > + $can_highlight = 0;
> > + last;
>
> Nb. this assumes that we cannot refine and highlight something like
> this:
>
> # - a
> # - b
> # + c
> # ++ d
>
> But perhaps this is better left for future improvemnt.
I can add a patch for that at the end of this series.
>
> > + }
> > + }
> > + }
> > + } else {
> > + $can_highlight = 0;
> > + }
>
> This 'else' would be not necessary if $can_highlight was initialized
> to 0.
>
OK.
> >
> > - if (!$is_combined && $num_add_lines > 0 && $num_add_lines
> > == @$rem) {
> > + if ($can_highlight) {
> > for (my $i = 0; $i < $num_add_lines; $i++) {
> > my ($line_rem, $line_add) =
> > format_rem_add_line(
> > - $rem->[$i], $add->[$i]);
> > + $rem->[$i], $add->[$i],
> > $is_combined); push @new_rem, $line_rem;
> > push @new_add, $line_add;
>
> O.K.
>
Thanks for your thorough review.
^ permalink raw reply
* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Michal Kiedrowicz @ 2012-02-13 6:54 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3lio8s57v.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> > Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > > +# Highlight characters from $prefix to $suffix and escape HTML.
> > > +# $str is a reference to the array of characters.
> > > +sub esc_html_mark_range {
> > > + my ($str, $prefix, $suffix) = @_;
> > > +
> > > + # Don't generate empty <span> element.
> > > + if ($prefix == $suffix + 1) {
> > > + return esc_html(join('', @$str), -nbsp=>1);
> > > + }
> > > +
> > > + my $before = join('', @{$str}[0..($prefix - 1)]);
> > > + my $marked = join('', @{$str}[$prefix..$suffix]);
> > > + my $after = join('', @{$str}[($suffix + 1)..$#{$str}]);
> >
> > Eeeeeek! First you split into letters, in caller at that, then
> > join? Why not pass striung ($str suggests string not array of
> > characters), and use substr instead?
> >
> > [Please disregard this and the next paragraph at first reading]
> >
> > > +
> > > + return esc_html($before, -nbsp=>1) .
> > > + $cgi->span({-class=>'marked'}, esc_html($marked,
> > > -nbsp=>1)) .
> > > + esc_html($after,-nbsp=>1);
> > > +}
> >
> > Anyway I have send to git mailing list a patch series, which in one
> > of patches adds esc_html_match_hl($str, $regexp) to highlight
> > matches in a string.
Yeah, I saw that but after seeing that they accept different arguments
I decided to leave them alone.
> Your esc_html_mark_range(), after a
> > generalization, could be used as underlying "engine".
> >
> > Something like this, perhaps (untested):
I think I'll leave it to you after merging both these series to
master :)
> >
> > # Highlight selected fragments of string, using given CSS class,
> > # and escape HTML. It is assumed that fragments do not overlap.
> > # Regions are passed as list of pairs (array references).
> > sub esc_html_hl {
> > my ($str, $css_class, @sel) = @_;
> > return esc_html($str) unless @sel;
> >
> > my $out = '';
> > my $pos = 0;
> >
> > for my $s (@sel) {
> > $out .= esc_html(substr($str, $pos, $s->[0] - $pos))
> > if ($s->[0] - $pos > 0);
> > $out .= $cgi->span({-class => $css_class},
> > esc_html(substr($str, $s->[0],
> > $s->[1] - $s->[0])));
> >
> > $pos = $m->[1];
> > }
> > $out .= esc_html(substr($str, $pos))
> > if ($pos < length($str));
> >
> > return $out;
> > }
>
> Actually we can accomodate both operating on string and operating on
> array of characters in a single subroutine. Though it can be left for
> later commit, anyway...
>
> # Highlight selected fragments of string, using given CSS class,
> # and escape HTML. It is assumed that fragments do not overlap.
> # Regions are passed as list of pairs (array references).
> sub esc_html_hl {
> my ($sth, $css_class, @sel) = @_;
>
> if (!@sel) {
> if (ref($sth) eq "ARRAY") {
> return esc_html(join('', @$sth), -nbsp=>1);
> } else {
> return esc_html($sth, -nbsp=>1);
> }
>
> if (ref($sth) eq "ARRAY") {
> return esc_html_hl_gen($sth,
> sub {
> my ($arr, $from, $to) = @_;
> return join('', @{$arr}[$from..$to]);
> },
> scalar @{$arr}, $css_class, @sel);
> } else {
> return esc_html_hl_gen($sth,
> sub {
> my ($str, $from, $to) = @_;
> if ($to < 0) { $to += lenght($str); };
> return substr($str, $from, $to -
> $from); },
> length($sth), $css_class, @sel);
> }
> }
>
> # Highlight selected fragments of string or array of characters
> # with given length, using provided $extr subroutine to extract
> # fragment (substring)
> sub esc_html_hl_gen {
> my ($sth, $extr, $len, $css_class, @sel) = @_;
>
> my $out = '';
> my $pos = 0;
>
> for my $s (@sel) {
> $out .= esc_html($extr->($str, $pos, $s->[0]))
> if ($s->[0] - $pos > 0);
> $out .= $cgi->span({-class => $css_class},
> esc_html($extr->($str, $s->[0],
> $s->[1])));
> $pos = $s->[1];
> }
> $out .= esc_html($extr->($str, $pos, $len))
> if ($pos < $len);
>
> return $out;
> }
>
> Or maybe I have read "Higher-Order Perl" one time too many ;-))))
>
^ permalink raw reply
* Re: Bulgarian translation of git
From: Jiang Xin @ 2012-02-13 7:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Ævar Arnfjörð,
Alexander Shopov (Александър Шопов)
In-Reply-To: <7vy5s7idxb.fsf@alter.siamese.dyndns.org>
2012/2/13 Junio C Hamano <gitster@pobox.com>:
> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> Junio has a suggestion on how to contrib l10n for git, and you can see the
>> discussion here: http://article.gmane.org/gmane.comp.version-control.git/189584.
>>
>> Seems that the suggested git-po repo has not been setup yet, so let me
>> have a try.
>>
>> 1. Repositiry git-po is hosted on GitHub: https://github.com/gotgit/git-po/
>>
>> 2. I made a commit on the maint branch with a initial version of 'po/git.pot'
>> https://github.com/gotgit/git-po/commit/4247a7a9d39e2a74ce1d58e5eb1f5e5d87977989
This commit is based on current maint branch. Since the translated po files will
first in 1.7.10, so the initial pot file should be generated from master branch.
The pot file from the master branch has 10 new messages than from the maint
branch.
For clarity, I removed the maint and other branches except the master branch,
and made a new commit.
> Somebody needs to eyeball this commit before anything else happens on top
> of it, so that if there is a glaring mistake it can be caught before it
> spreads to affect work by translators for various languages.
So please check this commit with the updated pot file:
https://github.com/gotgit/git-po/commit/816049b7ec0e5f452c77a4c4e71e2cb40a0ccb45
Please note In this commit I removed one line from the 'po/.gitignore' file.
As a translater for Chinese, I also made a commit with the translated po file
'po/zh_CN.po' in branch master-zh-cn (merged back to master branch).
>>
>> I have a question, which version of po should be maintained? master
>> branch or maint branch.
>
> I would say for this round the git-po repository and its pot file should
> pick up whatever new translatable strings are added to 'master'.
>
> After this is merged in 1.7.10, we may want to maintain separate tracks,
> but at this moment there is no point maintaining something mergeable to
> 1.7.9.x maintenance track.
>
--
Jiang Xin
^ permalink raw reply
* Re: Bulgarian translation of git
From: Junio C Hamano @ 2012-02-13 8:07 UTC (permalink / raw)
To: Jiang Xin
Cc: Git List, Christian Stimming, Pat Thoyts, Michele Ballabio,
Alex Riesen, Miklos Vajna, Laszlo Papp, Peter Krefting,
Christian Couder, Emmanuel Trillaud, Nanako Shiraishi, Mizar,
Peter Karlsson, Mikael Magnusson, Ævar Arnfjörð,
Alexander Shopov (Александър Шопов)
In-Reply-To: <CANYiYbFYmGNE09fAeHL_uk+0s+yBapTs4BeGch7iCtWR_v9LXQ@mail.gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
>> Somebody needs to eyeball this commit before anything else happens on top
>> of it, so that if there is a glaring mistake it can be caught before it
>> spreads to affect work by translators for various languages.
>
> So please check this commit with the updated pot file:
> https://github.com/gotgit/git-po/commit/816049b7ec0e5f452c77a4c4e71e2cb40a0ccb45
> Please note In this commit I removed one line from the 'po/.gitignore' file.
Thanks.
As far as I can tell, this adds git.pot while removes the entry to remove
it from po/.gitignore (because now it is a tracked file), but there is no
description on how this file was generated in the log message.
Please give a short description to the effect that it was mechanically
created by running xgettext on the sources as of b6b3b6a (Update draft
release notes to 1.7.10, 2012-02-10), or something like that.
Other than that, I do not immediately see anything objectionable in this
commit; I would still appreciate an extra set of eyeballs or two from
people who have worked on gitk and/or git-gui i18n/l10n infrastructure in
the past, just in case I forgot to check some obvious/trivial gotchas.
I am also Cc'ing people who participated in git-gui and gitk l10n with
more than a couple of commits to their po/ part, found by:
$ cd git-gui/po &&
git rev-list --parents HEAD . |
while read commit parent
do
git log --pretty=short $parent..$commit^2 -- :/po
done | git shortlog -n -e
(same for gitk-git)
> As a translater for Chinese, I also made a commit with the translated po file
> 'po/zh_CN.po' in branch master-zh-cn (merged back to master branch).
Thanks.
^ permalink raw reply
* Re: Bulgarian translation of git
From: Jiang Xin @ 2012-02-13 8:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Christian Stimming, Pat Thoyts, Michele Ballabio,
Alex Riesen, Miklos Vajna, Laszlo Papp, Peter Krefting,
Christian Couder, Emmanuel Trillaud, Nanako Shiraishi, Mizar,
Mikael Magnusson, Ævar Arnfjörð,
Alexander Shopov (Александър Шопов)
In-Reply-To: <7v8vk7gnqf.fsf@alter.siamese.dyndns.org>
2012/2/13 Junio C Hamano <gitster@pobox.com>:
> Jiang Xin <worldhello.net@gmail.com> writes:
>
>>> Somebody needs to eyeball this commit before anything else happens on top
>>> of it, so that if there is a glaring mistake it can be caught before it
>>> spreads to affect work by translators for various languages.
>>
>> So please check this commit with the updated pot file:
>> ...
Please check this commit with the updated pot file:
https://github.com/gotgit/git-po/commit/master%5E
( The commit with the amended commit message can be access using
the above URL, no longer how this commit is ammended latter.)
> As far as I can tell, this adds git.pot while removes the entry to remove
> it from po/.gitignore (because now it is a tracked file), but there is no
> description on how this file was generated in the log message.
>
> Please give a short description to the effect that it was mechanically
> created by running xgettext on the sources as of b6b3b6a (Update draft
> release notes to 1.7.10, 2012-02-10), or something like that.
I add a prefix 'l10n:' to the new amended commit log, is it OK?
$ git show --stat master^
commit b55102542911326e8191500ff6f53a2e1d52e504
Author: Jiang Xin <worldhello.net@gmail.com>
Date: Mon Feb 13 14:37:59 2012 +0800
l10n: initial git.pot for 1.7.10 upcoming release
The file 'po/git.pot' is generated using the command 'make pot'
against git v1.7.9-209-gb6b3b (Update draft release notes to 1.7.10).
Also removed the entry po/git.pot from po/.gitignore, since it is a
tracked file.
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
po/.gitignore | 1 -
po/git.pot | 3431 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 3431 insertions(+), 1 deletions(-)
--
Jiang Xin
^ permalink raw reply
* [PATCH v5 0/3] push: submodule support
From: Heiko Voigt @ 2012-02-13 9:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Fredrik Gustafsson, Jens Lehmann
I have rewritten and cleaned up the recursive submodule on-demand push
patch from Fredrik. Since this involved changing large parts of it I
have taken over the ownership. To pay credit that this is built upon
Fredriks work I have left the signed-off and mentored-by footers the
same. I hope that this is the proper way to handle such cases. If
someone comes up with a better idea I am happy to change things.
Cheers Heiko
The first iteration of this patch series can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/176328/focus=176327
The second iteration of this patch series can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/177992
The third iteration of this patch series can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/179037/focus=179048
The fourth iteration of this patch series can be found here:
http://thread.gmane.org/gmane.comp.version-control.git/179731
Heiko Voigt (3):
Teach revision walking machinery to walk multiple times sequencially
Refactor submodule push check to use string list instead of integer
push: teach --recurse-submodules the on-demand option
.gitignore | 1 +
Documentation/git-push.txt | 14 +++-
Documentation/technical/api-revision-walking.txt | 5 +
Makefile | 1 +
builtin/push.c | 7 ++
object.c | 11 +++
object.h | 2 +
revision.c | 5 +
revision.h | 1 +
submodule.c | 70 ++++++++++++++---
submodule.h | 4 +-
t/t0062-revision-walking.sh | 33 ++++++++
t/t5531-deep-submodule-push.sh | 94 ++++++++++++++++++++++
test-revision-walking.c | 66 +++++++++++++++
transport.c | 41 +++++++++-
transport.h | 1 +
16 files changed, 338 insertions(+), 18 deletions(-)
create mode 100755 t/t0062-revision-walking.sh
create mode 100644 test-revision-walking.c
--
1.7.9.114.gead08
^ permalink raw reply
* [PATCH v5 1/3] Teach revision walking machinery to walk multiple times sequencially
From: Heiko Voigt @ 2012-02-13 9:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Fredrik Gustafsson, Jens Lehmann
In-Reply-To: <20120213092541.GA15585@t1405.greatnet.de>
Previously it was not possible to iterate revisions twice using the
revision walking api. We add a reset_revision_walk() which clears the
used flags. This allows us to do multiple sequencial revision walks.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
.gitignore | 1 +
Documentation/technical/api-revision-walking.txt | 5 ++
Makefile | 1 +
object.c | 11 ++++
object.h | 2 +
revision.c | 5 ++
revision.h | 1 +
submodule.c | 2 +
t/t0062-revision-walking.sh | 33 +++++++++++
test-revision-walking.c | 66 ++++++++++++++++++++++
10 files changed, 127 insertions(+), 0 deletions(-)
create mode 100755 t/t0062-revision-walking.sh
create mode 100644 test-revision-walking.c
diff --git a/.gitignore b/.gitignore
index 3b7680e..2f09842 100644
--- a/.gitignore
+++ b/.gitignore
@@ -184,6 +184,7 @@
/test-obj-pool
/test-parse-options
/test-path-utils
+/test-revision-walking
/test-run-command
/test-sha1
/test-sigchain
diff --git a/Documentation/technical/api-revision-walking.txt b/Documentation/technical/api-revision-walking.txt
index 996da05..b7d0d9a 100644
--- a/Documentation/technical/api-revision-walking.txt
+++ b/Documentation/technical/api-revision-walking.txt
@@ -56,6 +56,11 @@ function.
returning a `struct commit *` each time you call it. The end of the
revision list is indicated by returning a NULL pointer.
+`reset_revision_walk`::
+
+ Reset the flags used by the revision walking api. You can use
+ this to do multiple sequencial revision walks.
+
Data structures
---------------
diff --git a/Makefile b/Makefile
index c457c34..bff686f 100644
--- a/Makefile
+++ b/Makefile
@@ -472,6 +472,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
TEST_PROGRAMS_NEED_X += test-obj-pool
TEST_PROGRAMS_NEED_X += test-parse-options
TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-revision-walking
TEST_PROGRAMS_NEED_X += test-run-command
TEST_PROGRAMS_NEED_X += test-sha1
TEST_PROGRAMS_NEED_X += test-sigchain
diff --git a/object.c b/object.c
index 6b06297..6291ce9 100644
--- a/object.c
+++ b/object.c
@@ -275,3 +275,14 @@ void object_array_remove_duplicates(struct object_array *array)
array->nr = dst;
}
}
+
+void clear_object_flags(unsigned flags)
+{
+ int i;
+ struct object *obj;
+
+ for (i=0; i < obj_hash_size; i++) {
+ if ((obj = obj_hash[i]) && obj->flags & flags)
+ obj->flags &= ~flags;
+ }
+}
diff --git a/object.h b/object.h
index b6618d9..6a97b6b 100644
--- a/object.h
+++ b/object.h
@@ -76,4 +76,6 @@ void add_object_array(struct object *obj, const char *name, struct object_array
void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);
void object_array_remove_duplicates(struct object_array *);
+void clear_object_flags(unsigned flags);
+
#endif /* OBJECT_H */
diff --git a/revision.c b/revision.c
index c97d834..77ce6bd 100644
--- a/revision.c
+++ b/revision.c
@@ -2061,6 +2061,11 @@ static void set_children(struct rev_info *revs)
}
}
+void reset_revision_walk()
+{
+ clear_object_flags(SEEN | ADDED | SHOWN);
+}
+
int prepare_revision_walk(struct rev_info *revs)
{
int nr = revs->pending.nr;
diff --git a/revision.h b/revision.h
index b8e9223..3535733 100644
--- a/revision.h
+++ b/revision.h
@@ -192,6 +192,7 @@ extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ct
const char * const usagestr[]);
extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename);
+extern void reset_revision_walk();
extern int prepare_revision_walk(struct rev_info *revs);
extern struct commit *get_revision(struct rev_info *revs);
extern char *get_revision_mark(const struct rev_info *revs, const struct commit *commit);
diff --git a/submodule.c b/submodule.c
index 9a28060..645ff5d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -404,6 +404,7 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
while ((commit = get_revision(&rev)) && !needs_pushing)
commit_need_pushing(commit, &needs_pushing);
+ reset_revision_walk();
free(sha1_copy);
strbuf_release(&remotes_arg);
@@ -741,6 +742,7 @@ static int find_first_merges(struct object_array *result, const char *path,
if (in_merge_bases(b, &commit, 1))
add_object_array(o, NULL, &merges);
}
+ reset_revision_walk();
/* Now we've got all merges that contain a and b. Prune all
* merges that contain another found merge and save them in
diff --git a/t/t0062-revision-walking.sh b/t/t0062-revision-walking.sh
new file mode 100755
index 0000000..3d98eb8
--- /dev/null
+++ b/t/t0062-revision-walking.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 Heiko Voigt
+#
+
+test_description='Test revision walking api'
+
+. ./test-lib.sh
+
+cat >run_twice_expected <<-EOF
+1st
+ > add b
+ > add a
+2nd
+ > add b
+ > add a
+EOF
+
+test_expect_success 'setup' '
+ echo a > a &&
+ git add a &&
+ git commit -m "add a" &&
+ echo b > b &&
+ git add b &&
+ git commit -m "add b"
+'
+
+test_expect_success 'revision walking can be done twice' '
+ test-revision-walking run-twice > run_twice_actual
+ test_cmp run_twice_expected run_twice_actual
+'
+
+test_done
diff --git a/test-revision-walking.c b/test-revision-walking.c
new file mode 100644
index 0000000..27ad597
--- /dev/null
+++ b/test-revision-walking.c
@@ -0,0 +1,66 @@
+/*
+ * test-revision-walking.c: test revision walking API.
+ *
+ * (C) 2012 Heiko Voigt <hvoigt@hvoigt.net>
+ *
+ * This code is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "cache.h"
+#include "commit.h"
+#include "diff.h"
+#include "revision.h"
+
+static void print_commit(struct commit *commit)
+{
+ struct strbuf sb = STRBUF_INIT;
+ struct pretty_print_context ctx = {0};
+ ctx.date_mode = DATE_NORMAL;
+ format_commit_message(commit, " %m %s", &sb, &ctx);
+ printf("%s\n", sb.buf);
+ strbuf_release(&sb);
+}
+
+static int run_revision_walk()
+{
+ struct rev_info rev;
+ struct commit *commit;
+ const char *argv[] = {NULL, "--all", NULL};
+ int argc = ARRAY_SIZE(argv) - 1;
+ int got_revision = 0;
+
+ init_revisions(&rev, NULL);
+ setup_revisions(argc, argv, &rev, NULL);
+ if (prepare_revision_walk(&rev))
+ die("revision walk setup failed");
+
+ while ((commit = get_revision(&rev)) != NULL) {
+ print_commit(commit);
+ got_revision = 1;
+ }
+
+ reset_revision_walk();
+ return got_revision;
+}
+
+int main(int argc, char **argv)
+{
+ if (argc < 2)
+ return 1;
+
+ if (!strcmp(argv[1], "run-twice")) {
+ printf("1st\n");
+ if (!run_revision_walk())
+ return 1;
+ printf("2nd\n");
+ if (!run_revision_walk())
+ return 1;
+
+ return 0;
+ }
+
+ fprintf(stderr, "check usage\n");
+ return 1;
+}
--
1.7.9.114.gead08
^ permalink raw reply related
* [PATCH v5 2/3] Refactor submodule push check to use string list instead of integer
From: Heiko Voigt @ 2012-02-13 9:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Fredrik Gustafsson, Jens Lehmann
In-Reply-To: <20120213092541.GA15585@t1405.greatnet.de>
This allows us to tell the user which submodules have not been pushed.
Additionally this is helpful when we want to automatically try to push
submodules that have not been pushed.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
submodule.c | 20 ++++++++++----------
submodule.h | 3 ++-
transport.c | 24 ++++++++++++++++++++++--
3 files changed, 34 insertions(+), 13 deletions(-)
diff --git a/submodule.c b/submodule.c
index 645ff5d..3c714c2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -357,21 +357,20 @@ static void collect_submodules_from_diff(struct diff_queue_struct *q,
void *data)
{
int i;
- int *needs_pushing = data;
+ struct string_list *needs_pushing = data;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
if (!S_ISGITLINK(p->two->mode))
continue;
if (submodule_needs_pushing(p->two->path, p->two->sha1)) {
- *needs_pushing = 1;
- break;
+ if (!string_list_has_string(needs_pushing, p->two->path))
+ string_list_insert(needs_pushing, p->two->path);
}
}
}
-
-static void commit_need_pushing(struct commit *commit, int *needs_pushing)
+static void commit_need_pushing(struct commit *commit, struct string_list *needs_pushing)
{
struct rev_info rev;
@@ -382,14 +381,15 @@ static void commit_need_pushing(struct commit *commit, int *needs_pushing)
diff_tree_combined_merge(commit, 1, &rev);
}
-int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)
+int check_submodule_needs_pushing(unsigned char new_sha1[20],
+ const char *remotes_name, struct string_list *needs_pushing)
{
struct rev_info rev;
struct commit *commit;
const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
int argc = ARRAY_SIZE(argv) - 1;
char *sha1_copy;
- int needs_pushing = 0;
+
struct strbuf remotes_arg = STRBUF_INIT;
strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
@@ -401,14 +401,14 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remote
if (prepare_revision_walk(&rev))
die("revision walk setup failed");
- while ((commit = get_revision(&rev)) && !needs_pushing)
- commit_need_pushing(commit, &needs_pushing);
+ while ((commit = get_revision(&rev)) != NULL)
+ commit_need_pushing(commit, needs_pushing);
reset_revision_walk();
free(sha1_copy);
strbuf_release(&remotes_arg);
- return needs_pushing;
+ return needs_pushing->nr;
}
static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
diff --git a/submodule.h b/submodule.h
index 80e04f3..ddd1941 100644
--- a/submodule.h
+++ b/submodule.h
@@ -29,6 +29,7 @@ int fetch_populated_submodules(int num_options, const char **options,
unsigned is_submodule_modified(const char *path, int ignore_untracked);
int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
const unsigned char a[20], const unsigned char b[20], int search);
-int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name);
+int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name,
+ struct string_list *needs_pushing);
#endif
diff --git a/transport.c b/transport.c
index cac0c06..d13bd4a 100644
--- a/transport.c
+++ b/transport.c
@@ -11,6 +11,7 @@
#include "branch.h"
#include "url.h"
#include "submodule.h"
+#include "string-list.h"
/* rsync support */
@@ -1000,6 +1001,20 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
transport->progress = force_progress || (verbosity >= 0 && isatty(2));
}
+static void die_with_unpushed_submodules(struct string_list *needs_pushing)
+{
+ int i;
+
+ fprintf(stderr, "The following submodule paths contain changes that can\n"
+ "not be found on any remote:\n");
+ for (i = 0; i < needs_pushing->nr; i++)
+ printf(" %s\n", needs_pushing->items[i].string);
+
+ string_list_clear(needs_pushing, 0);
+
+ die("Aborting.");
+}
+
int transport_push(struct transport *transport,
int refspec_nr, const char **refspec, int flags,
int *nonfastforward)
@@ -1040,10 +1055,15 @@ int transport_push(struct transport *transport,
if ((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) && !is_bare_repository()) {
struct ref *ref = remote_refs;
+ struct string_list needs_pushing;
+
+ memset(&needs_pushing, 0, sizeof(struct string_list));
+ needs_pushing.strdup_strings = 1;
for (; ref; ref = ref->next)
if (!is_null_sha1(ref->new_sha1) &&
- check_submodule_needs_pushing(ref->new_sha1,transport->remote->name))
- die("There are unpushed submodules, aborting.");
+ check_submodule_needs_pushing(ref->new_sha1,
+ transport->remote->name, &needs_pushing))
+ die_with_unpushed_submodules(&needs_pushing);
}
push_ret = transport->push_refs(transport, remote_refs, flags);
--
1.7.9.114.gead08
^ permalink raw reply related
* [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option
From: Heiko Voigt @ 2012-02-13 9:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Fredrik Gustafsson, Jens Lehmann
In-Reply-To: <20120213092541.GA15585@t1405.greatnet.de>
When using this option git will search for all submodules that
have changed in the revisions to be send. It will then try to
push the currently checked out branch of each submodule.
This helps when a user has finished working on a change which
involves submodules and just wants to push everything in one go.
Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>
Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>
---
Documentation/git-push.txt | 14 ++++--
builtin/push.c | 7 +++
submodule.c | 48 ++++++++++++++++++++
submodule.h | 1 +
t/t5531-deep-submodule-push.sh | 94 ++++++++++++++++++++++++++++++++++++++++
transport.c | 17 +++++++-
transport.h | 1 +
7 files changed, 177 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index aede488..649ee3a 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -162,10 +162,16 @@ useful if you write an alias or script around 'git push'.
is specified. This flag forces progress status even if the
standard error stream is not directed to a terminal.
---recurse-submodules=check::
- Check whether all submodule commits used by the revisions to be
- pushed are available on a remote tracking branch. Otherwise the
- push will be aborted and the command will exit with non-zero status.
+--recurse-submodules=<check|on-demand>::
+ Make sure all submodule commits used by the revisions to be
+ pushed are available on a remote tracking branch. If check is
+ used it will be checked that all submodule commits that changed
+ in the revisions to be pushed are available on a remote.
+ Otherwise the push will be aborted and exit with non-zero
+ status. If on-demand is used all submodules that changed in the
+ revisions to be pushed will be pushed. If on-demand was not able
+ to push all necessary revisions it will also be aborted and exit
+ with non-zero status.
include::urls-remotes.txt[]
diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..f2ef8dd 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -224,9 +224,16 @@ static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
{
int *flags = opt->value;
+
+ if (*flags & (TRANSPORT_RECURSE_SUBMODULES_CHECK |
+ TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND))
+ die("%s can only be used once.", opt->long_name);
+
if (arg) {
if (!strcmp(arg, "check"))
*flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
+ else if (!strcmp(arg, "on-demand"))
+ *flags |= TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND;
else
die("bad %s argument: %s", opt->long_name, arg);
} else
diff --git a/submodule.c b/submodule.c
index 3c714c2..ff0cfd8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -411,6 +411,54 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20],
return needs_pushing->nr;
}
+static int push_submodule(const char *path)
+{
+ if (add_submodule_odb(path))
+ return 1;
+
+ if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
+ struct child_process cp;
+ const char *argv[] = {"push", NULL};
+
+ memset(&cp, 0, sizeof(cp));
+ cp.argv = argv;
+ cp.env = local_repo_env;
+ cp.git_cmd = 1;
+ cp.no_stdin = 1;
+ cp.dir = path;
+ if (run_command(&cp))
+ return 0;
+ close(cp.out);
+ }
+
+ return 1;
+}
+
+int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)
+{
+ int i, ret = 1;
+ struct string_list needs_pushing;
+
+ memset(&needs_pushing, 0, sizeof(struct string_list));
+ needs_pushing.strdup_strings = 1;
+
+ if (!check_submodule_needs_pushing(new_sha1, remotes_name, &needs_pushing))
+ return 1;
+
+ for (i = 0; i < needs_pushing.nr; i++) {
+ const char *path = needs_pushing.items[i].string;
+ fprintf(stderr, "Pushing submodule '%s'\n", path);
+ if (!push_submodule(path)) {
+ fprintf(stderr, "Unable to push submodule '%s'\n", path);
+ ret = 0;
+ }
+ }
+
+ string_list_clear(&needs_pushing, 0);
+
+ return ret;
+}
+
static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
{
int is_present = 0;
diff --git a/submodule.h b/submodule.h
index ddd1941..af74941 100644
--- a/submodule.h
+++ b/submodule.h
@@ -31,5 +31,6 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c
const unsigned char a[20], const unsigned char b[20], int search);
int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name,
struct string_list *needs_pushing);
+int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
#endif
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 30bec4b..1947c28 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -119,4 +119,98 @@ test_expect_success 'push succeeds if submodule has no remote and is on the firs
)
'
+test_expect_success 'push unpushed submodules when not needed' '
+ (
+ cd work &&
+ (
+ cd gar/bage &&
+ git checkout master &&
+ >junk5 &&
+ git add junk5 &&
+ git commit -m "Fifth junk" &&
+ git push &&
+ git rev-parse origin/master >../../../expected
+ ) &&
+ git checkout master &&
+ git add gar/bage &&
+ git commit -m "Fifth commit for gar/bage" &&
+ git push --recurse-submodules=on-demand ../pub.git master
+ ) &&
+ (
+ cd submodule.git &&
+ git rev-parse master >../actual
+ ) &&
+ test_cmp expected actual
+'
+
+test_expect_success 'push unpushed submodules when not needed 2' '
+ (
+ cd submodule.git &&
+ git rev-parse master >../expected
+ ) &&
+ (
+ cd work &&
+ (
+ cd gar/bage &&
+ >junk6 &&
+ git add junk6 &&
+ git commit -m "Sixth junk"
+ ) &&
+ >junk2 &&
+ git add junk2 &&
+ git commit -m "Second junk for work" &&
+ git push --recurse-submodules=on-demand ../pub.git master
+ ) &&
+ (
+ cd submodule.git &&
+ git rev-parse master >../actual
+ ) &&
+ test_cmp expected actual
+'
+
+test_expect_success 'push unpushed submodules recursively' '
+ (
+ cd work &&
+ (
+ cd gar/bage &&
+ git checkout master &&
+ > junk7 &&
+ git add junk7 &&
+ git commit -m "Seventh junk" &&
+ git rev-parse master >../../../expected
+ ) &&
+ git checkout master &&
+ git add gar/bage &&
+ git commit -m "Seventh commit for gar/bage" &&
+ git push --recurse-submodules=on-demand ../pub.git master
+ ) &&
+ (
+ cd submodule.git &&
+ git rev-parse master >../actual
+ ) &&
+ test_cmp expected actual
+'
+
+test_expect_success 'push unpushable submodule recursively fails' '
+ (
+ cd work &&
+ (
+ cd gar/bage &&
+ git rev-parse origin/master >../../../expected &&
+ git checkout master~0 &&
+ > junk8 &&
+ git add junk8 &&
+ git commit -m "Eighth junk"
+ ) &&
+ git add gar/bage &&
+ git commit -m "Eighth commit for gar/bage" &&
+ test_must_fail git push --recurse-submodules=on-demand ../pub.git master
+ ) &&
+ (
+ cd submodule.git &&
+ git rev-parse master >../actual
+ ) &&
+ test_cmp expected actual
+'
+
test_done
diff --git a/transport.c b/transport.c
index d13bd4a..8c0fec0 100644
--- a/transport.c
+++ b/transport.c
@@ -1009,6 +1009,11 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
"not be found on any remote:\n");
for (i = 0; i < needs_pushing->nr; i++)
printf(" %s\n", needs_pushing->items[i].string);
+ fprintf(stderr, "\nPlease try\n\n"
+ " git push --recurse-submodules=on-demand\n\n"
+ "or cd to the path and use\n\n"
+ " git push\n\n"
+ "to push them to a remote.\n\n");
string_list_clear(needs_pushing, 0);
@@ -1053,7 +1058,17 @@ int transport_push(struct transport *transport,
flags & TRANSPORT_PUSH_MIRROR,
flags & TRANSPORT_PUSH_FORCE);
- if ((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) && !is_bare_repository()) {
+ if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {
+ struct ref *ref = remote_refs;
+ for (; ref; ref = ref->next)
+ if (!is_null_sha1(ref->new_sha1) &&
+ !push_unpushed_submodules(ref->new_sha1,
+ transport->remote->name))
+ die ("Failed to push all needed submodules!");
+ }
+
+ if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
+ TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {
struct ref *ref = remote_refs;
struct string_list needs_pushing;
diff --git a/transport.h b/transport.h
index 059b330..9d19c78 100644
--- a/transport.h
+++ b/transport.h
@@ -102,6 +102,7 @@ struct transport {
#define TRANSPORT_PUSH_PORCELAIN 16
#define TRANSPORT_PUSH_SET_UPSTREAM 32
#define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
+#define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 128
#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
--
1.7.9.114.gead08
^ permalink raw reply related
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Michael Haggerty @ 2012-02-13 9:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, pclouds, git, krh, jasampler
In-Reply-To: <7vsjifgrwl.fsf@alter.siamese.dyndns.org>
On 02/13/2012 07:37 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> Of *course* they operate on different namespaces. But part of the way
>> that revisions are selected using rev-list is by *selecting or excluding
>> refnames* from which it should crawl.
>
> I am appalled if that is truly the understanding of yours, after having
> taken more than a few patches from you to fairly core parts of Git.
>
> "rev-list A ^B" does not say "include A and exclude B from which rev-list
> should crawl" AT ALL. We _actively_ crawl from both A and B. It is that
> what are reachable from B is painted in a color different from the color
> in which we paint what are reachable from A.
Please read my emails more carefully before insulting me.
It is perfectly clear to me that there are two types of exclusion that
we are talking about. And *both* of them are (or should be) relevant to
rev-parse.
Take the following repository with three branches:
o---o---o---o A
\ \
\ o---o C
\
o---o B
If I do "git rev-list A B ^C" then I get the commits marked "*" in the
following diagram
o---o---o---* A
\ \
\ o---o C
\
*---* B
By excluding C I have necessarily excluded a part of the history of A and B.
If we assume that the proposed feature is implemented and I do "git
rev-list $(git for-each-ref --format='%(refname)' A B ^C)", then I get
something different:
*---*---*---* A
\ \
\ o---o C
\
*---* B
I argue that this is a useful selection. For example, maybe I want to
remove the clutter of branch C from my view, but I still want to see the
*whole* history of branches A and B. The middle selection doesn't do it.
Obviously this is not really necessary if there are only three branches,
but if there are dozens, and if A, B, and C are patterns rather than
literal branch names, then it can be very convenient.
For example, suppose I want to see the status of all of my submissions
in your repository in the context of your main branches plus my local
branches. It would be great to be able to type
gitk --with-branch='refs/heads/*' \
--with-branch='remotes/gitster/*' \
--without-branch='remotes/gitster/*/**' \
--with-branch='remotes/gitster/mh/*'
I don't know of a way to do that now.
> A better pair you could have mentioned would be for-each-ref vs rev-parse
> (not rev-list). What Tom wanted with "do not show the refs that match the
> pattern" he originally wanted to give to "tag --list" would be
>
> for-each-ref A ^B
>
> that is "show ref that matches A but do not show if it also matches B",
> while what you want to say is "I want to paint A in positive color and
> paint B in negative color, and I want to get a canonical notation to do
> so", it is spelled with rev-parse, not for-each-ref, like this:
>
> rev-parse A ^B
That's not what I want; see above.
> In other words,
>
> git rev-list $(git rev-parse A ^B)
>
> would be the equivalent to "git rev-list A ^B".
>
> Maybe you are troubled that there are multiple concepts of negation, which
> ultimately comes from the undeniable fact that for-each-ref and rev-parse
> operate on entities in different concept domain (refnames and objects)?
> And if we decide to use "^", then these two different concepts of negation
> are both expressed with the same operator "prefix ^", leading to
> confusion?
Not only that, but also that both concepts of negation are interesting
and useful within "git rev-list", and therefore we should make them
*combinable*.
To be very explicit, I advocate:
1. Implement an explicit syntax for "do not include references matching
this pattern in a list of references". Implement this syntax in
for-each-ref; something like
--with-ref=PATTERN / --without-ref=PATTERN
--with-branch=PATTERN / --without-branch=PATTERN
--with-tag=PATTERN / --without-tag=PATTERN
--with-remote=PATTERN / --without-remote=PATTERN
The point of having multiple with/without pairs would be that the first
would match full refnames explicitly (i.e., the pattern would usually
start with "refs/"), whereas the other pairs would implicitly prepend
"refs/heads/", "refs/tags/", or "refs/remotes/", respectively, to the
pattern for convenience. There should also be an "--all" option that is
equivalent to "--with-ref=**".
The output from for-each-ref would essentially be a *list of positive
references* matching the criteria. In other words,
"--without-branch=foo" would cause "refs/heads/foo" to be *excluded*
from the output altogether, *not* included as "^refs/heads/foo".
The order of the options should be significant, with the last matching
pattern winning.
2. The pattern matching of refnames should be like fnmatch, with the
addition of "**" as a wildcard meaning "any characters, including '/'".
3. Other reference-listing commands should take the same options as
appropriate; for example, "git branch --list" would take
--with(out)?-branch and --with(out)?-remote (and maybe
--with(out)?-ref); "git tag --list" would take --with(out)?-tag (and
maybe --with(out)?-ref), etc.
4. The *exact same options* should be added to rev-list, and would
effectively be expanded into a list of positive references; e.g.,
git rev-list --with-branch=A --with-branch=B --without-branch=C
would be equivalent to
git rev-list $(git for-each-ref --format='%(refname)'
--with-branch=A --with-branch=B --without-branch=C)
If A, B, and C happen to be branch names rather than patterns, the above
would be equivalent to
git rev-list refs/heads/A refs/heads/B
Note that this *differs* (in a useful way!) from
git rev-list refs/heads/A refs/heads/B --not refs/heads/C
or
git rev-list refs/heads/A refs/heads/B ^refs/heads/C
which are useful in other scenarios and whose meanings we would of
course retain.
If "--not" is used in git-rev-list, it would demarcate groups of options
that are passed separately to for-each-ref; for example,
git rev-list --all --with-branch=A --without-branch=B \
--not --with-branch=C --without-branch=D
would be equivalent to
git rev-list $(git for-each-ref --format='%(refname)' --all
--with-branch=A --without-branch=B)\
--not $(git for-each-ref --format='%(refname)'
--with-branch=C --without-branch=D)
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Junio C Hamano @ 2012-02-13 10:23 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Tom Grennan, pclouds, git, krh, jasampler
In-Reply-To: <4F38D9D4.5000203@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 02/13/2012 07:37 AM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> Of *course* they operate on different namespaces. But part of the way
>>> that revisions are selected using rev-list is by *selecting or excluding
>>> refnames* from which it should crawl.
>>
>> I am appalled if that is truly the understanding of yours, after having
>> taken more than a few patches from you to fairly core parts of Git.
>>
>> "rev-list A ^B" does not say "include A and exclude B from which rev-list
>> should crawl" AT ALL. We _actively_ crawl from both A and B. It is that
>> what are reachable from B is painted in a color different from the color
>> in which we paint what are reachable from A.
>
> Please read my emails more carefully before insulting me.
> ...
> o---o---o---* A
> \ \
> \ o---o C
> \
> *---* B
>
> ... vs ...
>
> *---*---*---* A
> \ \
> \ o---o C
> \
> *---* B
>
> I argue that this is a useful selection.
Then why were you so against the addition of "negation" to for-each-ref?
If you want "I want histories reaching A and B", just say "rev-list A B",
without adding useless "er, I do not want histories reaching C in the
output, but I do not want commits reachable from C to be excluded from the
output either" by mentioning C. Learn to shut your mouth and not talk
about irrelevant "C" in such a case, and you will do just fine.
Especially, re-read your first message where you said that between
git rev-list A B ^C
and
git rev-list $(git for-each-ref A B ^C)
"consistency suggests should do the same". Should the consistency also
suggest that
git rev-list $(git rev-parse A B ^C)
do the same? That is a total bullshit that can only come from somebody
who does not understand the distinction between pattern matching in
refnames vs set operation over commit DAG.
Having said all that, if your argument against using "^" as negation for
for-each-ref *were* with something like this from the beginning:
git rev-list --all --exclude-refs=refs/tags/v\*
it would have been very different. I would wholeheartedly buy the
consistency argument that says
git for-each-ref --exclude-refs=refs/tags/v\*
ought to give all refs (only because for-each-ref "all" is implied) except
for the tagged tips, and
git log --all --exclude-refs=refs/tags/v\*
should be the notation to produce consistently the same result as
git log $(git for-each-ref --format='%(objectname)' --exclude-refs=refs/tags/v\*)
but if we used "^" as negated match in for-each-ref argument, we would
close the door to give such consistency to log family of commands later.
But that wasn't what you said. Why should I get accused of not guessing
what you meant to say but you clearly didn't, and in addition blamed for
insulting merely for pointing out the idiocy in what you said?
No. *YOU* go back and re-read your message more carefully.
^ 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