* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Jakub Narebski @ 2012-02-11 13:45 UTC (permalink / raw)
To: Johan Herland; +Cc: Philip Oakley, git
In-Reply-To: <CALKQrgdWOgG3y2HzM694zDykGJWa4QDetsEVXf0AGpf=FNFaVg@mail.gmail.com>
On Sat, 11 Feb 2012, Johan Herland wrote:
> On Fri, Feb 10, 2012 at 20:38, Jakub Narebski <jnareb@gmail.com> wrote:
> > On Tue, 7 Feb 2012, Johan Herland wrote:
[...]
> > > I am unsure whether the 'secret'-ness of a commit should follow across
> > > the push, but if you do (assuming we store the 'secret' flag using
> > > git-notes) this is simply a matter of synchronizing the
> > > refs/notes/secret to the same remote.
> >
> > I think it should, so that 'secret' commit would not escape by accident
> > via a group secret repository.
> >
> > What makes it hard (I think) is that we would prefer to transfer
> > 'secret'-ness only for pushed commits. That might be problem for notes
> > based implementation of 'secret' annotation and 'secret'-ness transfer...
> > though I guess knowing that there exist 'secret' commit with given SHA1
> > which we do not have and should not have is not much breach of
> > confidentiality. Still...
>
> If you don't want to transfer all of refs/notes/secret, you would
> probably have to extend the git protocol with a per-commit 'secret'
> flag (which would then be applied to the receiving repo's
> refs/notes/secret).
Or create per-remote custom notes ref, for example for 'foo' remote it
would be 'refs/remotes/foo/notes/secret'... assuming that canonalization
of remote-tracking refs goes in (refs/remotes/foo/{heads,tags,notes,replace})
This would be updated with 'secret'-ness state of comits being pushed
before actual push, and secretness notes ref would be pushed together
with actual push.
> Still, this is all specific to the 'secret' feature, which IMHO is
> much less important then the 'public' feature. Implementing the
> barebones 'public' feature (i.e. refuse rewrite of commits reachable
> from upstream) is much less work, and would be enough for 90% of git
> users, I believe.
Hmmm... I thought that is 'public' feature that is more work, especially
in full incarnation. But perhaps bare bones (no 'pre-push' or 'pre-rewrite'
hooks, no traits info in "git log" output, per remote tracking of 'public'
status only, no support for bundles or send-email, etc.) could be as easy
or easier.
As to what is more important for git users... perhaps short survey would
help here? I wonder if asking Mercurial users about their use of "phases"
on their mailing would help us...
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Nguyen Thai Ngoc Duy @ 2012-02-11 14:06 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, Tom Grennan, git, krh, jasampler
In-Reply-To: <m3sjihu18t.fsf@localhost.localdomain>
On Sat, Feb 11, 2012 at 5:13 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> In a future versions, we may want to have "branch/tag --list" also ask for
>> FNM_PATHNAME (this *is* a backward incompatible change, so it needs to be
>> performed across major version boundary, with backward compatibility
>> configurations, deprecation warnings and whole nine yards). Under the new
>> match function, today's "branch --list 'maint*'" needs to be spelled as
>> "branch --list 'maint*/*'" or something.
One of the reasons I proposed pathspec is to avoid another set of
matching syntax in addition to gitattr, gitignore and pathspec,
Pathspec magic makes it extensible, FNM_PATHNAME be set that way.
> Or "branch --list 'maint**'
But this comes up a few times already on gitignore discussions. We
have fnmatch.c in compat for Windows. Someone just needs to step up
and implement "**". It would also a nice thing to add to pathspec
syntax, if we can forget the backward compatibility nightmare.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/8] gitweb: Extract print_sidebyside_diff_lines()
From: Jakub Narebski @ 2012-02-11 15:20 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-2-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> Currently, print_sidebyside_diff_chunk() does two things: it
> accumulates diff lines and prints them. Accumulation may be used to
> perform additional operations on diff lines, so it makes sense to split
> these two things. Thus, the code that prints diff lines in a side-by-side
> manner is moved out of print_sidebyside_diff_chunk() to a separate
> subroutine.
>
> The outcome of this patch is that print_sidebyside_diff_chunk() is now
> much shorter and easier to read.
>
> This change is meant to be a simple code movement. No behavior change is
> intended.
>
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
As it is pure refactoring, and improves readibility of code quite a
bit, I am all for it, but...
You replace the following set of conditions
> - ## print from accumulator when type of class of lines change
> - # empty contents block on start rem/add block, or end of chunk
> - if (@ctx && (!$class || $class eq 'rem' || $class eq 'add')) {
and
> - # empty add/rem block on start context block, or end of chunk
> - if ((@rem || @add) && (!$class || $class eq 'ctx')) {
> - if (!@add) {
[...]
> - } elsif (!@rem) {
[...]
> - } else {
with these (I have reindented the code to match)
> + ## print from accumulator when have some add/rem lines or end
> + # of chunk (flush context lines)
> + if (((@rem || @add) && $class eq 'ctx') || !$class) {
> + if (@$ctx) {
[...]
> + }
> + if (!@$add) {
[...]
> + } elsif (!@$rem) {
[...]
> + } else {
It is not obvious that the final result is the same.
BTW doesn't new code print context when printing added and removed
lines, and not as soon as class changes? This doesn't change the
output, but it slightly changes behavior.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 2/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs
From: Jakub Narebski @ 2012-02-11 15:53 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-3-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> This renames print_sidebyside_diff_chunk() to print_diff_chunk() and
> makes use of it for both side-by-side and inline diffs. Now diff lines
> are always accumulated before they are printed. This opens the
> possibility to preprocess diff output before it's printed.
>
> The new function print_inline_diff_lines() could reorder diff lines.
I think you meant here "if left as is", isn't it?
> It first prints all context lines, then all removed lines and
> finally all added lines. If the diff output consisted of mixed added
> and removed lines, gitweb would reorder these lines. This is
> especially true for combined diff output, for example:
>
> - removed line for first parent
> + added line for first parent
> -removed line for second parent
> ++added line for both parents
>
> would be rendered as:
>
> - removed line for first parent
> -removed line for second parent
> + added line for first parent
> ++added line for both parents
This is not "is especially true"; it can happen (though: can it
really?) in combined diff output; in ordinary diff you would always
have context or end/beginning of chunk between added and removed
lines.
>
> To prevent gitweb from reordering lines, print_diff_chunk() calls
> print_diff_lines() as soon as it detects that both added and removed
> lines are present and there was a class change.
You really should mention, in my opinion, that this change is
preparation for diff refinement highlighting (higlighting the
differing segments).
Otherwise I don't see the reason for it: ordinary diff didn't need the
fancy stuff with accumulating then printing, and could be send
straightly to output. This adds complexity for non-sidebyside diff,
unnecessary if not for diff refinement highlighting.
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> ---
> gitweb/gitweb.perl | 53 +++++++++++++++++++++++++++++++++++++--------------
> 1 files changed, 38 insertions(+), 15 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1247607..ed63261 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4908,9 +4908,31 @@ sub print_sidebyside_diff_lines {
> }
> }
>
> -sub print_sidebyside_diff_chunk {
> - my @chunk = @_;
> +# Print context lines and then rem/add lines in inline manner.
> +sub print_inline_diff_lines {
> + my ($ctx, $rem, $add) = @_;
> +
> + print foreach (@$ctx);
> + print foreach (@$rem);
> + print foreach (@$add);
> +}
> +
> +# Print context lines and then rem/add lines.
> +sub print_diff_lines {
> + my ($ctx, $rem, $add, $diff_style, $is_combined) = @_;
> +
> + if ($diff_style eq 'sidebyside' && !$is_combined) {
> + print_sidebyside_diff_lines($ctx, $rem, $add);
> + } else {
> + # default 'inline' style and unknown styles
> + print_inline_diff_lines($ctx, $rem, $add);
> + }
> +}
That looks nice.
BTW. I didn't examine the final code, but what happens for binary
diffs that git supports? Is it handled outside print_diff_chunk()?
> +
> +sub print_diff_chunk {
> + my ($diff_style, $is_combined, @chunk) = @_;
> my (@ctx, @rem, @add);
> + my $prev_class = '';
Is $prev_class here class of previous chunk fragment or _previous class_
(i.e. always different from $class), or is it class of previous line?
It is not obvious here, from variable name. Better name or at least
a comment would be appreciated.
>
> return unless @chunk;
>
> @@ -4935,9 +4957,13 @@ sub print_sidebyside_diff_chunk {
> }
>
> ## print from accumulator when have some add/rem lines or end
> - # of chunk (flush context lines)
> - if (((@rem || @add) && $class eq 'ctx') || !$class) {
> - print_sidebyside_diff_lines(\@ctx, \@rem, \@add);
> + # of chunk (flush context lines), or when have add and rem
> + # lines and new block is reached (otherwise add/rem lines could
> + # be reordered)
> + if (((@rem || @add) && $class eq 'ctx') || !$class ||
> + (@rem && @add && $class ne $prev_class)) {
We usually use tabs to align, but spaces to indent in gitweb, so it
would be
> + if (((@rem || @add) && $class eq 'ctx') || !$class ||
> + (@rem && @add && $class ne $prev_class)) {
Anyway, this conditional gets ridicously complicated. I wonder if we
could simplify it.
If we allow printing context together widh added/removed lines, and
not as soon as possible, perhaps good conditional would be: class
changed, and accumulator for current class is full:
> + if ($class ne $prev_class && @{ $acc{$class} }) {
Or something like that, where
my %acc = (ctx => \@ctx, add => \@add, rem => \@rem);
If we want to print context as soon as possible, like it was before
1st patch in this series, we could uuse the following conditional
> + if ($class ne $prev_class &&
> + ($prev_class eq 'ctx' || @{ $acc{$class} })) {
Perhaps with "scalar @{ $acc{$class} }" if it would make things more
clear.
Though probably to avoid "Use of uninitialized value in string ne"
we would probably have to use empty string instead of undefined
value for "no class" case, i.e. beginning or end of chunk.
> + print_diff_lines(\@ctx, \@rem, \@add, $diff_style,
> + $is_combined);
> @ctx = @rem = @add = ();
> }
>
> @@ -4954,6 +4980,8 @@ sub print_sidebyside_diff_chunk {
> if ($class eq 'ctx') {
> push @ctx, $line;
> }
> +
> + $prev_class = $class;
> }
> }
[...]
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 3/8] gitweb: Move HTML-formatting diff line back to process_diff_line()
From: Jakub Narebski @ 2012-02-11 16:02 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-4-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> Commit 6ba1eb51b (gitweb: Add a feature to show side-by-side diff,
> 2011-10-31) for no special reason moved wrapping diff line in <div> out
> of format_diff_line(). Bring back old behavior.
>
> This simplifies code in git_patchset_body() and keeps formatting of a
> diff line in one place.
>
> The more long-term purpose of this patch is to move formatting diff
> lines down to print_diff_chunk(), to allow processing lines without
> HTML-formatting.
>
> This is just a refactoring patch. It's not meant to change gitweb
> output.
Gah, I don't remember why we ended with change in behavior when adding
side-by-side diff support to gitweb.
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Acked-by: Jakub Narębski <jnareb@gmail.com>
> ---
> gitweb/gitweb.perl | 15 ++++++++-------
> 1 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ed63261..d2f75c4 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2332,14 +2332,18 @@ sub process_diff_line {
>
> if ($from && $to && $line =~ m/^\@{2} /) {
> $line = format_unidiff_chunk_header($line, $from, $to);
> - return $diff_class, $line;
>
> } elsif ($from && $to && $line =~ m/^\@{3}/) {
> $line = format_cc_diff_chunk_header($line, $from, $to);
> - return $diff_class, $line;
> -
> + } else {
> + $line = esc_html($line, -nbsp=>1);
> }
> - return $diff_class, esc_html($line, -nbsp=>1);
> +
> + my $diff_classes = "diff";
> + $diff_classes .= " $diff_class" if ($diff_class);
> + $line = "<div class=\"$diff_classes\">$line</div>\n";
> +
> + return $diff_class, $line;
> }
>
> # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
> @@ -5104,9 +5108,6 @@ sub git_patchset_body {
> next PATCH if ($patch_line =~ m/^diff /);
>
> my ($class, $line) = process_diff_line($patch_line, \%from, \%to);
> - my $diff_classes = "diff";
> - $diff_classes .= " $class" if ($class);
> - $line = "<div class=\"$diff_classes\">$line</div>\n";
>
> if ($class eq 'chunk_header') {
> print_diff_chunk($diff_style, $is_combined, @chunk);
> --
> 1.7.3.4
>
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 4/8] gitweb: Push formatting diff lines to print_diff_chunk()
From: Jakub Narebski @ 2012-02-11 16:29 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-5-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> Now git_patchset_body() only calls diff_line_class() (removed from
> process_diff_line()). The latter function is renamed to
> format_diff_line() and is called from print_diff_chunk().
>
> This is a pure code movement, needed for processing raw diff lines in
> the accumulator in print_diff_chunk(). No behavior change is intended by
> this change.
Well, this is not "pure code movement" per se; it is meant to be
refactoring that doesn't change gitweb output nor behavior.
If I understand correctly the change is from
read
format
accumulate
print
to
read
accumulate
format
print
Isn't it?
As a note I would add also that process_diff_line got renamed to
format_diff_line, and its output changed to returning only
HTML-formatted line, which bringg it in line with other format_*
subroutines.
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
I think it is a good change even without subsequent patches.
Acked-by: Jakub Narębski <jnareb@gmail.com>
> ---
> gitweb/gitweb.perl | 25 ++++++++++++-------------
> 1 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d2f75c4..cae9dfa 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2320,12 +2320,9 @@ sub format_cc_diff_chunk_header {
> }
>
> # process patch (diff) line (not to be used for diff headers),
> -# returning class and HTML-formatted (but not wrapped) line
> -sub process_diff_line {
> - my $line = shift;
> - my ($from, $to) = @_;
> -
> - my $diff_class = diff_line_class($line, $from, $to);
> +# returning HTML-formatted (but not wrapped) line
> +sub format_diff_line {
> + my ($line, $diff_class, $from, $to) = @_;
>
> chomp $line;
> $line = untabify($line);
> @@ -2343,7 +2340,7 @@ sub process_diff_line {
> $diff_classes .= " $diff_class" if ($diff_class);
> $line = "<div class=\"$diff_classes\">$line</div>\n";
>
> - return $diff_class, $line;
> + return $line;
> }
>
> # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
> @@ -4934,7 +4931,7 @@ sub print_diff_lines {
> }
>
> sub print_diff_chunk {
> - my ($diff_style, $is_combined, @chunk) = @_;
> + my ($diff_style, $is_combined, $from, $to, @chunk) = @_;
> my (@ctx, @rem, @add);
> my $prev_class = '';
>
> @@ -4954,6 +4951,8 @@ sub print_diff_chunk {
> foreach my $line_info (@chunk) {
> my ($class, $line) = @$line_info;
>
> + $line = format_diff_line($line, $class, $from, $to);
> +
> # print chunk headers
> if ($class && $class eq 'chunk_header') {
> print $line;
> @@ -5107,19 +5106,19 @@ sub git_patchset_body {
>
> next PATCH if ($patch_line =~ m/^diff /);
>
> - my ($class, $line) = process_diff_line($patch_line, \%from, \%to);
> + my $class = diff_line_class($patch_line, \%from, \%to);
>
> if ($class eq 'chunk_header') {
> - print_diff_chunk($diff_style, $is_combined, @chunk);
> - @chunk = ( [ $class, $line ] );
> + print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
> + @chunk = ( [ $class, $patch_line ] );
> } else {
> - push @chunk, [ $class, $line ];
> + push @chunk, [ $class, $patch_line ];
> }
> }
>
> } continue {
> if (@chunk) {
> - print_diff_chunk($diff_style, $is_combined, @chunk);
> + print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
> @chunk = ();
> }
> print "</div>\n"; # class="patch"
> --
> 1.7.3.4
>
Nice!
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 5/8] gitweb: Format diff lines just before printing
From: Jakub Narebski @ 2012-02-11 17:14 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-6-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> Now we're ready to insert highlightning to diff output.
>
> The call to untabify() remains in the main loop in print_diff_chunk().
> The motivation is that it must be called before any call to esc_html()
> (because that converts spaces to ) and to call it only once.
>
> This is a refactoring patch. It's not meant to change gitweb output.
I'm not sure about this patch.
First, in my opinion it doesn't make much sense standalone, and not
squashed with subsequent patch.
Second, it makes format_diff_line an odd duck among all other format_*
subroutines in that it post-processes HTML output, rather than
generating HTML from data.
Why "diff refinement highlighting" cannot be part of format_diff_line()?
If it does need additional data, just pass it as additional parameters
to this subroutine.
Another solution could be to borrow idea from stalled and inactive
committags feature, namely that parts that are HTML are to be passed
as scalar reference (\$str), while plain text (unescaped yet) is to be
passed as string ($str).
> -# process patch (diff) line (not to be used for diff headers),
> -# returning HTML-formatted (but not wrapped) line
> +# 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 {
I just don't like this error-prone "the line must be esc_html()'ed".
> +# HTML-format diff context, removed and added lines.
> +sub format_ctx_rem_add_lines {
> + my ($ctx, $rem, $add) = @_;
> + my (@new_ctx, @new_rem, @new_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);
> +}
> +
> # Print context lines and then 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);
> +
Nice trick.
--
Jakub Narębski
^ permalink raw reply
* git-subtree Ready #2
From: David A. Greene @ 2012-02-11 17:35 UTC (permalink / raw)
To: git
[This bounced for some reason.]
Ok, I have http access now:
git clone http://sources.obbligato.org/git/git.git
git pull origin subtree
I might need to fiddle with permissions, let me know.
-Dave
^ permalink raw reply
* Re: [PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs
From: Junio C Hamano @ 2012-02-11 17:59 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jeff King
In-Reply-To: <CACsJy8Bh=FZ6kNN5hERK5_H7XnZ83BZ_EfsZ5XmJbrnn+CfgcQ@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 2012/2/11 Junio C Hamano <gitster@pobox.com>:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> ...
>> Would it make more sense to note the
>> directory for which rmdir() fails in try_remove_empty_parents(), and
>> revisit only these directories, at least?
>
> That would leave empty directories not sharing the ref's path until
> the failed rmdir() unexamined, I think.
True. Thanks.
>>> + subpath = xmalloc(pathlen + 257);
>>
>> What is this 257 about?
>
> This function is a ripoff from get_ref_dir(). I think 257 is 255 below
> plus '/' and NIL.
I do not think there is any justification to copy-and-paste from code that
predates the strbuf infrastructure these days.
^ permalink raw reply
* Re: git-subtree Ready #2
From: Junio C Hamano @ 2012-02-11 18:03 UTC (permalink / raw)
To: David A. Greene; +Cc: git
In-Reply-To: <877gztmfwy.fsf@smith.obbligato.org>
greened@obbligato.org (David A. Greene) writes:
> I might need to fiddle with permissions, let me know.
Everybody's client can talk smart http these days. Please don't
host/publish the code behind a dumb HTTP server.
^ permalink raw reply
* Re: Undo last commit?
From: Neal Kreitzinger @ 2012-02-11 18:19 UTC (permalink / raw)
To: git
Cc: Massimo Manca, Jakub Narebski, Jonathan Nieder, Mike, Ben Walton,
Ramkumar Ramachandra, Nguyễn Thái Ngọc Duy
In-Reply-To: <4E09DDAE.30801@ira.uka.de>
On 6/28/2011 8:57 AM, Holger Hellmuth wrote:
>
> It would be a nice companion to the not-yet-realized "git unadd" ;-)
>
or perhaps "git unstage"...
v/r,
neal
^ permalink raw reply
* "git pull" doesn't know "--edit"
From: Linus Torvalds @ 2012-02-11 18:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
Ok, so now "git merge" defaults to editing when interactive - lovely. But
when testing that, I noticed that while you can say
git merge --[no-]edit ..branch..
that does not work with "git pull". You get a message like
error: unknown option `no-edit'
usage: git fetch [<options>] [<repository> [<refspec>...]]
or: git fetch [<options>] <group>
or: git fetch --multiple [<options>] [(<repository> | <group>)...]
or: git fetch --all [<options>]
-v, --verbose be more verbose
-q, --quiet be more quiet
--all fetch from all remotes
...
which is because that stupid shell script doesn't know about the new
flags, and just passes it to "git fetch" instead.
Now, I really wanted to just make "git pull" a built-in instead of that
nasty shell script, but I'm lazy. So here's the trivial updates to
git-pull.sh to at least teach it about -e/--edit/--no-edit.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
This hasn't seen a lot of testing, but it looks pretty obvious
git-pull.sh | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/git-pull.sh b/git-pull.sh
index d8b64d7a67a1..434c139f077e 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -40,7 +40,7 @@ test -f "$GIT_DIR/MERGE_HEAD" && die_merge
strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
log_arg= verbosity= progress= recurse_submodules=
-merge_args=
+merge_args= edit=
curr_branch=$(git symbolic-ref -q HEAD)
curr_branch_short="${curr_branch#refs/heads/}"
rebase=$(git config --bool branch.$curr_branch_short.rebase)
@@ -70,6 +70,10 @@ do
no_commit=--no-commit ;;
--c|--co|--com|--comm|--commi|--commit)
no_commit=--commit ;;
+ -e|--edit)
+ edit=--edit ;;
+ --no-edit)
+ edit=--no-edit ;;
--sq|--squ|--squa|--squas|--squash)
squash=--squash ;;
--no-sq|--no-squ|--no-squa|--no-squas|--no-squash)
@@ -278,7 +282,7 @@ true)
eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
;;
*)
- eval="git-merge $diffstat $no_commit $squash $no_ff $ff_only"
+ eval="git-merge $diffstat $no_commit $edit $squash $no_ff $ff_only"
eval="$eval $log_arg $strategy_args $merge_args $verbosity $progress"
eval="$eval \"\$merge_name\" HEAD $merge_head"
;;
^ permalink raw reply related
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Junio C Hamano @ 2012-02-11 18:31 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Tom Grennan, pclouds, git, krh, jasampler
In-Reply-To: <m3sjihu18t.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> In a future versions, we may want to have "branch/tag --list" also ask for
>> FNM_PATHNAME (this *is* a backward incompatible change, so it needs to be
>> performed across major version boundary, with backward compatibility
>> configurations, deprecation warnings and whole nine yards). Under the new
>> match function, today's "branch --list 'maint*'" needs to be spelled as
>> "branch --list 'maint*/*'" or something.
>
> Or "branch --list 'maint**'
That is one of the things covered by "or something", and I deliberately
left it like so because all the good things that can happen "In future
version" are irrelevant at the present _unless_ the required first step
you omitted from your quote is not done properly, and I didn't want to
invite people to derail the discussion into such a tangent that does not
yet have any value.
Please don't.
^ permalink raw reply
* Re: [PATCH 0/8] gitweb: Highlight interesting parts of diff
From: Jakub Narebski @ 2012-02-11 18:32 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-1-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> gitweb: Use print_diff_chunk() for both side-by-side and inline diffs
> gitweb: Move HTML-formatting diff line back to process_diff_line()
> gitweb: Push formatting diff lines to print_diff_chunk()
> gitweb: Format diff lines just before printing
Those patshes are expected to not change gitweb output. Did you check
that gitweb after those changes handles incomplete line marker
correctly? print_sidebyside_diff_chunk() ealt with 'incomplete' class
in slightly hacky way, assuming side-by-side output.
Namely, does gitweb after those patches print incomplete line marker
correctly, i.e. only once for "inline" output while it prints it for
both sides in "side-by-side" output, and using 'incomplete' class?
You have to treat correctly all the following situations:
* removing end of line at end of file (eol-at-eof):
diff --git a/a b/b
index 257cc56..1910281 100644
--- a/a
+++ b/b
@@ -1 +1 @@
-foo
+foo
\ No newline at end of file
* adding eol-at-eof:
diff --git b/b a/a
index 1910281..257cc56 100644
--- b/b
+++ a/a
@@ -1 +1 @@
-foo
\ No newline at end of file
+foo
* removing from end of file, preserving lack of eol-at-eof:
diff --git b/c a/a
index a5d4a2f..257cc56 100644
--- b/c
+++ a/a
@@ -1,2 +1 @@
foo
-foo
\ No newline at end of file
* change at the end of file, preserving lack of eol-at-eof:
diff --git a/b b/d
index 1910281..ba0e162 100644
--- a/b
+++ b/d
@@ -1 +1 @@
-foo
\ No newline at end of file
+bar
\ No newline at end of file
* change near the end of file, with incomplete line marker in context
diff --git b/c a/b
index a5d4a2f..1910281 100644
--- b/c
+++ a/b
@@ -1,2 +1 @@
-foo
foo
\ No newline at end of file
--
Jakub Narębski
^ permalink raw reply
* [PATCH] column: Fix an incorrect parse of the 'nodense' option token
From: Ramsay Jones @ 2012-02-11 18:41 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, GIT Mailing-list
The parse_option() function always saw the 'nodense' token as
'dense', since it stripped the 'no' off the input argument string
earlier when checking for the presence of the 'color' token.
In order to fix the parse, we use local variables (within the loop)
initialised to the function input arguments instead, which allows
each token check to be independent.
Also, add some 'nodense' tests to t/t9002-column.sh.
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi Nguyen,
While writing the earlier "column: Fix some compiler and sparse warnings"
patch a few days ago, I had a feeling that a bug lurked in the parse_option()
code, but I just could not put my finger on the problem; so I just sent
what I had.
This morning I suddenly realised what was bothering me ... As a first
step, I duplicated the 'dense' tests in t9002-column.sh and changed the
'dense' token to 'nodense' and the tests still passed!
Note that the code supports the 'nocolor' token, but it is not documented
in config.txt.
If I understand correctly (and it's quite possible that I don't!), the
'nodense' token has the same meaning as the absence of the 'dense' token
(and I assume a similar comment applies to the '[no]color' tokens). If that
is the case, then maybe a better solution would be to simply remove the
'nodense' and 'nocolor' tokens. I will leave it to you to decide which
solution is more appropriate.
ATB,
Ramsay Jones
column.c | 13 +++++++------
t/t9002-column.sh | 26 ++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/column.c b/column.c
index 98328cf..6df7640 100644
--- a/column.c
+++ b/column.c
@@ -313,19 +313,20 @@ static int parse_option(const char *arg, int len,
int i;
for (i = 0; i < ARRAY_SIZE(opts); i++) {
- int set = 1, name_len;
+ int set = 1, arg_len = len, name_len;
+ const char *arg_str = arg;
if (opts[i].type == OPTION) {
- if (len > 2 && !strncmp(arg, "no", 2)) {
- arg += 2;
- len -= 2;
+ if (arg_len > 2 && !strncmp(arg_str, "no", 2)) {
+ arg_str += 2;
+ arg_len -= 2;
set = 0;
}
}
name_len = strlen(opts[i].name);
- if (len != name_len ||
- strncmp(arg, opts[i].name, name_len))
+ if (arg_len != name_len ||
+ strncmp(arg_str, opts[i].name, name_len))
continue;
switch (opts[i].type) {
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 23d340e..fe7a30e 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -71,6 +71,19 @@ EOF
test_cmp expected actual
'
+test_expect_success '20 columns, nodense' '
+ cat >expected <<\EOF &&
+one seven
+two eight
+three nine
+four ten
+five eleven
+six
+EOF
+ git column --mode=column,nodense < lista > actual &&
+ test_cmp expected actual
+'
+
test_expect_success '20 columns, dense' '
cat >expected <<\EOF &&
one five nine
@@ -121,6 +134,19 @@ EOF
test_cmp expected actual
'
+test_expect_success '20 columns, row first, nodense' '
+ cat >expected <<\EOF &&
+one two
+three four
+five six
+seven eight
+nine ten
+eleven
+EOF
+ git column --mode=row,nodense <lista >actual &&
+ test_cmp expected actual
+'
+
test_expect_success '20 columns, row first, dense' '
cat >expected <<\EOF &&
one two three
--
1.7.9
^ permalink raw reply related
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Tom Grennan @ 2012-02-11 19:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: pclouds, git, jasampler
In-Reply-To: <7vaa4qnk4u.fsf@alter.siamese.dyndns.org>
On Fri, Feb 10, 2012 at 07:06:57PM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>>>If we pursue this, it may be best to first add match_patterns() to ./refs.[ch]
>>>then incrementally modify these builtin commands to use it.
>>
>> The following series implements !<pattern> with: git-tag, git-branch, and
>> git-for-each-ref.
>>
>> This still requires Documentation and unit test updates but I think these are
>> close to functionally complete.
>>
>>>>About the '!' for exclusion, maybe it's better to move from fnmatch()
>>>>as matching machinery to pathspec. Then when git learns negative
>>>>pathspec [1], we have this feature for free.
>>>>
>>>>[1] http://thread.gmane.org/gmane.comp.version-control.git/189645/focus=190072
>>
>> After looking at this some more, I don't understand the value of replacing
>> libc:fnmatch(). Or are you just referring to '--exclude' instead of
>> [!]<pattern> argument parsing?
>
>I have not formed a firm opinion on Nguyen's idea to reuse pathspec
>matching infrastructure for this purpose, so I wouldn't comment on that
>part. It certainly looks attractive, as it allows users to learn one and
>only one extended matching syntax, but at the same time, it has a risk to
>mislead people to think that the namespace for refs is similar to that of
>the filesystem paths, which I see as a mild downside.
>
>In any case, I do not like the structure of this series. If it followed
>our usual pattern, it would consist of patches in this order:
>
> - Patch 1 would extract match_pattern() from builtin/tag.c and introduce
> the new helper function refname_match_patterns() to refs.c. It updates
> the call sites of match_pattern() in builtin/tag.c, match_patterns() in
> builtin/branch.c, and the implementation of grab_single_ref() in
> builtin/for-each-ref.c with a call to the new helper function.
>
> This step can and probably should be done as three sub-steps. 1a would
> move builtin/tag.c::match_pattern() to refs.::refname_match_patterns(),
> 1b would use the new helper in builtin/branch.c and 1c would do the
> same for builtin/for-each-ref.c.
>
> It is important that this patch does so without introducing any new
> functionality to the new function over the old one. When done this way,
> there is no risk of introducing new bugs at 1a because it is purely a
> code movement and renaming; 1b could introduce a bug that changes
> semantics for bulitin/branch.c if its match_patterns() does things
> differently from match_pattern() lifted from builtin/tag.c, and if it
> is found out to be buggy, we can discard 1b without discarding 1a. Same
> for 1c, which I highly suspect will introduce regression without
> looking at the code (for-each-ref is prefix-match only), that can
> safely be discarded.
>
> This is to make it easier to ensure that the update does not introduce
> new bugs.
>
> - Patch 2 would then add the new functionality to the new helper. It
> would also adjust the documentation of the three end user facing
> commands to describe the fallout coming from this change, and adds new
> tests to make sure future changes will not break this new
> functionality.
>
>That is, first refactor and clean-up without adding anything new, and then
>build new stuff on solidified ground.
Nuts! I have the cart before the horse. I'll try to rearrange the series as
suggested by tomorrow. Thanks.
>Do we allow a refname whose pathname component begins with '!', by the
>way? If we do, how does a user look for a tag whose name is "!xyzzy"?
>"Naming your tag !xyzzy used to be allowed but it is now forbidden after
>this patch" is not an acceptable answer---it is called a regression. If
>the negation operator were "^" or something that we explicitly forbid from
>a refname, we wouldn't have such a problem.
Cool, as I recall, v7 or earlier /bin/sh also used "^" to preface
exclusion patterns. Another option is the bash extglob syntax of
!(pattern) although I'd prefer "^" b/c one wouldn't have to quote it
with bash:
$ git branch --list ^pu
--
TomG
^ permalink raw reply
* Re: [PATCHv2 1/4] refs: add common refname_match_patterns()
From: Tom Grennan @ 2012-02-11 19:17 UTC (permalink / raw)
To: Michael Haggerty; +Cc: pclouds, git, gitster, jasampler
In-Reply-To: <4F3614F6.2000106@alum.mit.edu>
On Sat, Feb 11, 2012 at 08:12:54AM +0100, Michael Haggerty wrote:
>On 02/11/2012 03:16 AM, Tom Grennan wrote:
>> diff --git a/refs.h b/refs.h
>> index 00ba1e2..13015ba 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -152,4 +152,12 @@ int update_ref(const char *action, const char *refname,
>> const unsigned char *sha1, const unsigned char *oldval,
>> int flags, enum action_on_err onerr);
>>
>> +/**
>> + * Returns:
>> + * 1 with NULL patterns
>> + * 0 if refname fnmatch()es any ! prefaced pattern
>> + * 1 if refname fnmatch()es any pattern
>> + */
>> +extern int refname_match_patterns(const char **patterns, const char *refname);
>> +
>> #endif /* REFS_H */
>
>This comment is unclear and incomplete.
>
>1. What does "NULL patterns" mean? Your code fails if patterns==NULL,
>so I guess you mean "1 if there are no patterns in the list".
>
>2. Since the three conditions are not mutually exclusive, you should say
>how they are connected. I believe that you want something like "A
>otherwise B otherwise C".
>
>3. You haven't specified what happens if refname matches neither a
>!-prefixed pattern nor a non-!-prefixed pattern. Does this behavior
>depend on which types of patterns were present in the list?
>
>I see that you have described the behavior more completely in the commit
>message for patch 2/4, but the commit message is not enough: this
>behavior should be described precisely in both code comments (when the
>function is defined) and in the user documentation (when the
>functionality is added to a command).
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
*/
Thanks,
TomG
^ permalink raw reply
* Re: git-subtree Ready #2
From: David A. Greene @ 2012-02-11 19:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8vk9mem4.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> greened@obbligato.org (David A. Greene) writes:
>
>> I might need to fiddle with permissions, let me know.
>
> Everybody's client can talk smart http these days. Please don't
> host/publish the code behind a dumb HTTP server.
I'll have to learn how to set up a smart http.
In the meantime, this should also work:
git clone git://sources.obbligato.org/git/git.git
Or via gitweb:
http://sources.obbligato.org
-Dave
^ permalink raw reply
* Re: [PATCHv2 1/4] refs: add common refname_match_patterns()
From: Tom Grennan @ 2012-02-11 19:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: pclouds, git, jasampler
In-Reply-To: <7vpqdln68v.fsf@alter.siamese.dyndns.org>
On Sat, Feb 11, 2012 at 12:06:56AM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> +int refname_match_patterns(const char **patterns, const char *refname)
>> +{
>> + int given_match_pattern = 0, had_match = 0;
>> +
>> + for (; *patterns; patterns++)
>> + if (**patterns != '!') {
>> + given_match_pattern = 1;
>> + if (!fnmatch(*patterns, refname, 0))
>> + had_match = 1;
>> + } else if (!fnmatch(*patterns+1, refname, 0))
>> + return 0;
>> + return given_match_pattern ? had_match : 1;
>> +}
>
>This, while its semantics seem sane, is highly inefficient when you have
>many patterns, and you will be calling this to filter dozens of refs. And
>it can trivially improved by first pre-parsing the pattern[] array.
>
> * If you know the patterns do not have any negative entry, you can return
> true upon seeing the first match. Because you do not pre-parse the
> pattern[] array, this loop does not know if there is any negative one,
> and has to scan it always all the way.
>
> * If you arrange the pattern[] array so that it has negative ones early,
> again, you can return false upon seeing the first hit with a negative
> one. If your input has negative ones at the end, the loop ends up
> scanning all the way, noting the positive matches, only to discard upon
> seeing the negative match at the end.
>
>That is why I said Nguyen's idea of reusing pathspec matching logic
>somewhat attractive, even though I think it has downsides (the exact
>matching logic for pathspec is more similar to that of for-each-ref
>and very different from branch/tag).
Yes, I should have stated that this emphasized containment over
efficiency. If instead we stipulate that the caller must list exclusion
patterns before others, this could simply be:
int match_pattern(const char **patterns, const char *refname)
{
if (*patterns)
return 1;
for (; *patterns && **patterns == '!'; patterns++)
if (!fnmatch(*patterns+1, refname, 0))
return 0;
for (; *patterns; patterns++)
if (!fnmatch(*patterns, refname, 0))
return 1;
return 0;
}
Of course I'd add a with_exclusions_first() before the
respective ref iterator.
--
TomG
^ permalink raw reply
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Tom Grennan @ 2012-02-11 19:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: pclouds, git, jasampler
In-Reply-To: <7vy5s9n70x.fsf@alter.siamese.dyndns.org>
On Fri, Feb 10, 2012 at 11:50:06PM -0800, Junio C Hamano wrote:
>Junio C Hamano <gitster@pobox.com> writes:
>
>> ... Same
>> for 1c, which I highly suspect will introduce regression without
>> looking at the code (for-each-ref is prefix-match only), ...
OK I'll study this further and run through t6300-for-each-ref.sh
I see it has a bunch of errors.
I think there are similar issues using match_pattern() with
show-branch and ls-remote.
Thanks,
>This part needs correction. for-each-ref matches the command line
>arguments differently from branch --list and tag --list in two important
>ways.
>
> (1) It allows (not "only" which was a mistake in my earlier message)
> prefix matching, e.g. "for-each-ref refs/heads/", in addition to
> fnmatch(); and
>
> (2) The fnmatch() call is made with FNM_PATHMAME, which "branch --list"
> and "tag --list" does not use.
>
>Strictly speaking, therefore, if you make all three commands to use the
>same matching logic, there is no way to avoid regression. If you choose
>to use fnmatch() without FNM_PATHNAME, then for-each-ref suddenly starts
>matching wildcards across name hierarchy boundary '/' for a pattern that
>does not match today, e.g. "git for-each-ref 'refs/heads/*'" was a good
>way to grab only the integration branches while excluding individual topic
>branches such as refs/heads/tg/tag-points-at, but this technique can no
>longer be used for such a purpose, which is an unpleasant regression.
>
>I personally think that it was an annoying UI mistake that we let branch
>and tag call fnmatch without FNM_PATHNAME, but we cannot fix it lightly,
>either. People who use hierchical branch names (e.g. maint-1.0/$topic,
>maint-2.0/$topic, and feature-2.0/$topic) may already be used to list all
>the topics on the maintenance tracks with "branch --list 'maint*'", and we
>need to keep "branch --list" and "tag --list" working as they expect.
>
>One possible way forward (now I am talking about a longer term solution)
>would be to introduce
>
> refname_match_pattern(const char *refname,
> const char **pattern,
> unsigned flags);
>
>where flags can tell the implementation if FNM_PATHNAME should be used,
>and if prefix matching should be attempted, so that the three commands
>share the single same matching function while still retaining their
>current behaviour in the initial round. Inside the implementation, we
>would use good old fnmatch(), with or without FNM_PATHNAME, depending on
>the flags the caller passes.
>
>In a future versions, we may want to have "branch/tag --list" also ask for
>FNM_PATHNAME (this *is* a backward incompatible change, so it needs to be
>performed across major version boundary, with backward compatibility
>configurations, deprecation warnings and whole nine yards). Under the new
>match function, today's "branch --list 'maint*'" needs to be spelled as
>"branch --list 'maint*/*'" or something. The prefix matching is probably
>safer to enable by default without causing big regression hassle if we
>limit the prefix match to only patterns that end with an explicit slash,
>as users already *know* today's "branch --list tg/" would not match
>anything (because the pattern does not even match a brahch 'tg', so it is
>unlikely they are using it and expecting only 'tg' to match), which means
>that is an unlikely input we can safely give new meaning to match anything
>under tg/ hierarchy.
>
^ permalink raw reply
* Re: "git pull" doesn't know "--edit"
From: Linus Torvalds @ 2012-02-11 20:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <alpine.LFD.2.02.1202111016340.28503@i5.linux-foundation.org>
On Sat, Feb 11, 2012 at 10:21 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Ok, so now "git merge" defaults to editing when interactive - lovely. But
> when testing that,
Ok, I found another thing that seems to be a buglet, or at least an
undocumented surprise.
In the docs, the "GIT_MERGE_AUTOEDIT=no" thing is mentioned as the way
to get the legacy behavior, which (at least to me) implies that
setting it to "yes" gets the modern behavior.
But try this:
.. create test branch that can be merged ..
export GIT_MERGE_AUTOEDIT=yes
git merge test < /dev/null
and notice how the "GIT_MERGE_AUTOEDIT=yes" will actually *override*
the automatic merge thing, and will try to start an editor even for
non-interactive sessions.
Maybe this is intentional, and not a bug? But it does seem a bit odd -
the name is "AUTOEDIT", not "FORCEDEDIT". And at least my default
editor gets confused by the redirected input, although obviously if
you have a graphical editor in its own window this may well be what
you want.
Anyway, maybe the "return v" in default_edit_option() should be
if (!v)
return 0;
instead - so that if AUTOEDIT it set to true, it does what the "auto"
in the name implies.
Of course, the current behavior *can* actually be useful, exactly as
that way to force the editor to come up. So maybe it's just that my
expectations that are wrong, and the behavior that "yes" causes a
forced editor should just be documented instead.
Or maybe the thing could extend the notion of the current boolean to
be a tri-state instead: in addition to the traditional true/yes/on and
false/no/off have a "force" mode that is that "always force it on
regardless".
And maybe this is just a "nobody cares" situation - "Don't do that then".
Linus
^ permalink raw reply
* Re: Undo last commit?
From: Jakub Narebski @ 2012-02-11 22:07 UTC (permalink / raw)
To: Mike
Cc: Neal Kreitzinger, Holger Hellmuth, Massimo Manca, Jonathan Nieder,
Ben Walton, Ramkumar Ramachandra,
Nguyễn Thái Ngọc Duy, git
In-Reply-To: <CAHK-92oMc62O0S8Bxt6+uxobE+kg5wOeRDoOsHWvvenXaXmZGQ@mail.gmail.com>
Please do not top-post, and do not remove git mailing list from Cc.
Sorry for double posting; forgot to re-add git@vger.kernel.org
On Sat, 11 Feb 2012, Mike ??? wrote:
> 2012/2/11 Neal Kreitzinger <nkreitzinger@gmail.com>
> > On 6/28/2011 8:57 AM, Holger Hellmuth wrote:
> > >
> > > It would be a nice companion to the not-yet-realized "git unadd" ;-)
> >
> > or perhaps "git unstage"...
>
> If lot's of people have the same problem then it IS a design flaw. If
> something is designed well and genuinely intuitive then they just work.
> Think iPhone, iPod, some DVD players and other well designed user
> interfaces. The same goes for command line tools... the options should have
> names that don't have any ambiguity.
>
Many of problems with git user interface have their source from the fact
that git, including its interface, was evolved rather than created using
big-design-upfront workflow. And it _had_ to be evolved, as there was not
much of prior art (well, not good prior art) in the area of DVCS.
Besides, as they say:
The only "intuitive" interface is the nipple. After that it's all learned.
-- Bruce Ediger
> Techie guys almost always blame the users, this is a very bad attitude. For
> example I've met so many techies that THINK they can design websites...
> err... they can't. They CAN program sure, but they CAN'T design the user
> experience properly as that is not their expertise. Just as we wouldn't
> expect a graphic designer or user interface specialist to do the coding.
>
There is also problem in that you need to know git well to _code_ interface;
and when you know git well you don't notice no longer the problems that you
had as a newbie.
On the other hand new git users sometimes have problems distinguishing
between accidental complexity of bad UI design, and essential complexity
of a powerfull and flexible tool.
[...]
So, Mike, will you bitch or will you try to help?
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: Undo last commit?
From: Jonathan Nieder @ 2012-02-11 22:29 UTC (permalink / raw)
To: Jakub Narebski
Cc: Mike, Neal Kreitzinger, Holger Hellmuth, Massimo Manca,
Ben Walton, Ramkumar Ramachandra,
Nguyễn Thái Ngọc Duy, git
In-Reply-To: <201202112307.10528.jnareb@gmail.com>
Hi,
Jakub Narebski wrote:
> So, Mike, will you bitch or will you try to help?
I sent a message (not to the git list by accident, sorry) that made
the same mistake, but I think I was misdiagnosing.
I suspect Mike wanted to invite us to take a closer look at the story
in [1] and learn what can be learned from it. (For example, may be an
error message or some documentation needs to be improved, or maybe new
enhancements like "git checkout -B master HEAD^" would have helped.)
Unfortunately the message [1] is not very focused, making it hard to
learn from. So my advice to Mike would be to try again, and to
clearly explain what problem he was trying to solve and when git
failed him (either by a command producing a different effect than
expected or the appropriate command to carry out some action being
hard to find).
Jonathan
[1] http://thread.gmane.org/gmane.comp.version-control.git/175968
^ permalink raw reply
* Re: [PATCH 0/8] gitweb: Highlight interesting parts of diff
From: Michał Kiedrowicz @ 2012-02-11 22:56 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m31uq1te51.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
>
> > gitweb: Use print_diff_chunk() for both side-by-side and inline diffs
> > gitweb: Move HTML-formatting diff line back to process_diff_line()
> > gitweb: Push formatting diff lines to print_diff_chunk()
> > gitweb: Format diff lines just before printing
>
> Those patshes are expected to not change gitweb output.
Yes.
> Did you check
> that gitweb after those changes handles incomplete line marker
> correctly? print_sidebyside_diff_chunk() ealt with 'incomplete' class
> in slightly hacky way, assuming side-by-side output.
>
Yeah, I saw the 'for' loop at the beginning of
print_sidebyside_diff_chunk() but I didn't look at it very closely.
I'll make sure 'incomplete' lines are properly printed.
^ permalink raw reply
* Re: [PATCH 1/8] gitweb: Extract print_sidebyside_diff_lines()
From: Michał Kiedrowicz @ 2012-02-11 23:03 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3obt5tn0g.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > Currently, print_sidebyside_diff_chunk() does two things: it
> > accumulates diff lines and prints them. Accumulation may be used to
> > perform additional operations on diff lines, so it makes sense to split
> > these two things. Thus, the code that prints diff lines in a side-by-side
> > manner is moved out of print_sidebyside_diff_chunk() to a separate
> > subroutine.
> >
> > The outcome of this patch is that print_sidebyside_diff_chunk() is now
> > much shorter and easier to read.
> >
> > This change is meant to be a simple code movement. No behavior change is
> > intended.
> >
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
>
> As it is pure refactoring, and improves readibility of code quite a
> bit, I am all for it, but...
>
> You replace the following set of conditions
>
> > - ## print from accumulator when type of class of lines change
> > - # empty contents block on start rem/add block, or end of chunk
> > - if (@ctx && (!$class || $class eq 'rem' || $class eq 'add')) {
>
> and
>
> > - # empty add/rem block on start context block, or end of chunk
> > - if ((@rem || @add) && (!$class || $class eq 'ctx')) {
> > - if (!@add) {
> [...]
> > - } elsif (!@rem) {
> [...]
> > - } else {
>
>
> with these (I have reindented the code to match)
>
> > + ## print from accumulator when have some add/rem lines or end
> > + # of chunk (flush context lines)
> > + if (((@rem || @add) && $class eq 'ctx') || !$class) {
>
> > + if (@$ctx) {
> [...]
> > + }
> > + if (!@$add) {
> [...]
> > + } elsif (!@$rem) {
> [...]
> > + } else {
>
> It is not obvious that the final result is the same.
You are right. I should have described it in the commit message.
>
> BTW doesn't new code print context when printing added and removed
> lines, and not as soon as class changes? This doesn't change the
> output, but it slightly changes behavior.
>
This is also true. I'll describe it in the commit message. I could also
create two functions: one that prints @ctx and second one that prints
@rem and @add but then I would have to create two other functions for
inline diff.
^ 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