Git development
 help / color / mirror / Atom feed
* Re: [RFC PATCH 0/3] git-p4: move to toplevel
From: Junio C Hamano @ 2012-02-13 21:20 UTC (permalink / raw)
  To: Clemens Buchacher; +Cc: Pete Wyckoff, git, Luke Diamand, Vitor Antunes
In-Reply-To: <20120213203709.GA31671@ecki>

Clemens Buchacher <drizzd@aon.at> writes:

>> Erm,... do you really need the alias if you add git-p4 in a directory on
>> your $PATH?
>
> With recent git versions, this has stopped working.

Erm, I am confused.

	$ git --exec-path
	/home/junio/g/Debian-6.X-x86_64/git-jch/libexec/git-core
	$ type git-hello
	bash: type: git-hello: not found
	$ cat >~/bin/common/git-hello <<EOF
	#!/bin/sh
	echo hello world
	EOF
	$ chmod +x ~/bin/common/git-hello
	$ type git-hello
	git-hello is /home/junio/bin/common/git-hello
	$ git hello
	hello world

What am I missing???

^ permalink raw reply

* Re: [PATCH 5/5] push/fetch/clone --no-progress suppresses progress output
From: Junio C Hamano @ 2012-02-13 21:13 UTC (permalink / raw)
  To: Clemens Buchacher; +Cc: git
In-Reply-To: <1329164235-29955-6-git-send-email-drizzd@aon.at>

Clemens Buchacher <drizzd@aon.at> writes:

> diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
> index 9ee52cf..3683df1 100755
> --- a/t/t5523-push-upstream.sh
> +++ b/t/t5523-push-upstream.sh
> @@ -101,10 +101,11 @@ test_expect_success TTY 'push -q suppresses progress' '
>  	! grep "Writing objects" err
>  '
>  
> -test_expect_failure TTY 'push --no-progress suppresses progress' '
> +test_expect_success TTY 'push --no-progress suppresses progress' '
>  	ensure_fresh_upstream &&
>  
>  	test_terminal git push -u --no-progress upstream master >out 2>err &&
> +	! grep "Unpacking objects" err &&
>  	! grep "Writing objects" err
>  '

Very nice to see an old expect-failure turned into expect-success.

> diff --git a/transport.c b/transport.c
> index cac0c06..6074ee9 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -994,10 +994,14 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
>  	 * when a rule is satisfied):
>  	 *
>  	 *   1. Report progress, if force_progress is 1 (ie. --progress).
> +	 *   2. Don't report progress, if force_progress is 0 (ie. --no-progress).
>  	 *   2. Don't report progress, if verbosity < 0 (ie. -q/--quiet ).
>  	 *   3. Report progress if isatty(2) is 1.
>  	 **/

I'll turn this into an unnumbered bulletted list, while I tease it apart
to remove textual dependency on the 'verify-refs' extension, which this
fix shouldn't depend on and taken hostage to.

Thanks.

^ permalink raw reply

* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Michał Kiedrowicz @ 2012-02-13 21:10 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <201202132058.18001.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> wrote:

> Michal Kiedrowicz wrote:
> > Jakub Narebski <jnareb@gmail.com> wrote:
> > > Jakub Narebski <jnareb@gmail.com> writes:
> [...]
> > > > 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 :)
>  
> Yes, you are right.  Let's do it when we actually need it.
> 

Good :)

^ permalink raw reply

* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Michał Kiedrowicz @ 2012-02-13 21:09 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <201202131944.50886.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> wrote:

> On Mon, 13 Feb 2012, Michal Kiedrowicz wrote:
> > Jakub Narebski <jnareb@gmail.com> wrote:
> > > Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> > 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
> 
> Thanks.  That is what I meant by "good examples".  Perhaps they should
> be put in the commit message?

OK

> 
> BTW GitHub is closed source, but we can check what algorithm does Trac
> use for diff refinement highlighting (highlighting changed portions of
> diff).
> 

I think it's
http://trac.edgewall.org/browser/trunk/trac/versioncontrol/diff.py
(see markup intraline_changes()).


> > > 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.
> 
> I wonder if we can use --diff-words for diff refinement highlighting,
> i.e. LCS on words.

I think we can try it, but I worry about performance of running `git
diff` on every diff chunk.


> 
> Anyway Jeff's approach is a bit limited, in that it would work only for
> change that does not involve adding newlines, for example splitting
> overly long line when changing something.
> 
> See for example line 1786 (in pre-image) in http://trac.edgewall.org/changeset/10973
>  

Yes, I'm aware of that. I was thinking about improving it later ("Let's
start with a simple refinment highlightning and maybe later add more
sophisticated algorithms").

> > > By completly different you mean that they do not have common prefix or
> > > common suffix (at least one of them), isn't it?
> 
> BTW. is it "at least one of prefix or suffix are non-empty" or "both prefix
> and suffix are non-empty"?
> 

At least one. See:

	-a = 42;
	+b = 42;

Here prefix is empty but suffix is not.

> > I would also consider ignoring prefixes/suffixes with punctuation, like:
> > 
> > 	- * I like you.
> > 	+ * Alice had a little lamb.
> 
> But this patch doesn't implement this feature yet, isn't it?

No, but is a matter of adding

	-$prefix_is_space = 0 if ($r[$prefix] !~ /\s/);
	+$prefix_is_space = 0 if ($r[$prefix] !~ /\s|[[:punct:]]/);

(and the same for suffix)

> Well, here is another idea: do not highlight if sum of prefix and suffix
> lengths are less than some threshold, e.g. 2 characters not including 
> whitespace, or some percentage with respect to total line length.
> 

That might be a good idea.

> > > 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.
> 
> Please do.
> 
> BTW. don't you assume here that both common prefix and common suffix
> are non-empty?

Yes. The caller makes sure they are correct (at least 

> > > > +sub format_rem_add_line {
> > > > +	my ($rem, $add) = @_;
> > > > +	my @r = split(//, $rem);
> > > > +	my @a = split(//, $add);
> 
> BTW the name of variable can be just @add and @rem.
>

I know they are different scopes but I don't like it. It makes the code
more confusing IMO. But I won't insist.

> > > 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);
> 
> Anyway comment about that fact would be nice.

Will do.

>  
> > > > +	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.
> 
> You are right.
> 
> I'll try to come with hacky algorithm using string bitwise xor and regexp,
> and benchmark it comparing to your C-like solution, but it can be left for
> later (simple is better than clever, usually).

If you have time :).

> > > >  # 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.
> 
> BTW. the same comment applies to patch adding support for highlighting
> changed part in combined diff.
>

OK
  

^ permalink raw reply

* GitWeb and atom feed links
From: Heiko W. Rupp @ 2012-02-13 20:43 UTC (permalink / raw)
  To: git; +Cc: jnareb

Hi,

( got this email from Junio )

when you e.g. look at http://git.kernel.org/?p=git/git.git;a=summary and then the lower right, there
are two buttons for feeds. If you click on e.g. atom, you end up with an url of http://git.kernel.org/?p=git/git.git;a=atom
where the output is not a feed in atom format, but plain html with tables etc.

If you change the url to http://git.kernel.org/?p=git/git.git&a=atom

the output is a correct atom feed (same for rss).

I've traced it down in my copy of gitweb to 

sub git_footer_html {
...
                     $href_params{'action'} = lc($format);
                      print $cgi->a({-href => href(%href_params),
                                    -title => "$href_params{'-title'} $format feed",
                                    -class => $feed_class}, $format)."\n";

and here to the usage of href()

Locally I've modified that to 

                     $href_params{'action'} = lc($format);
                      my $hr = "?p=$project&a="; 
                      $hr .= lc($format);
                      print $cgi->a({-href => $hr,
                                    -title => "$href_params{'-title'} $format feed",
                                    -class => $feed_class}, $format)."\n";

and it works.

Attached is a patch against current master that reflects above change

   Heiko

-- 
Heiko Rupp   hwr@pilhuhn.de
Blog: http://javablogs.com/ViewBlog.action?id=14468

^ permalink raw reply

* Re: [BUG] git-merge-octopus creates an empty merge commit with one parent
From: Michał Kiedrowicz @ 2012-02-13 20:53 UTC (permalink / raw)
  To: git
In-Reply-To: <7vpqdieclu.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster <at> pobox.com> writes:

> Well, what was missing from my "don't do it then" quote was "Doctor it
> HURTS when I do this."; there is no question that it is not correct if it
> hurts .

All right :)

^ permalink raw reply

* Re: [PATCH 1/5] git rev-list: fix invalid typecast
From: Junio C Hamano @ 2012-02-13 20:48 UTC (permalink / raw)
  To: Clemens Buchacher; +Cc: git
In-Reply-To: <1329164235-29955-2-git-send-email-drizzd@aon.at>

Clemens Buchacher <drizzd@aon.at> writes:

> git rev-list passes rev_list_info, not rev_list objects. Without this
> fix, rev-list enables or disables the --verify-objects option depending
> on a read from an undefined memory location.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---

Wow, I simply couldn't believe it, but you are right.

Thanks.

^ permalink raw reply

* Re: [RFC PATCH 0/3] git-p4: move to toplevel
From: Clemens Buchacher @ 2012-02-13 20:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Pete Wyckoff, git, Luke Diamand, Vitor Antunes
In-Reply-To: <7vehtyec64.fsf@alter.siamese.dyndns.org>

On Mon, Feb 13, 2012 at 12:00:35PM -0800, Junio C Hamano wrote:
> Pete Wyckoff <pw@padd.com> writes:
> 
> > Users install git-p4 currently by copying the git-p4 script from
> > contrib/fast-import into a local or personal bin directory, and
> > setting up an alias for "git p4" to invoke it.
> 
> Erm,... do you really need the alias if you add git-p4 in a directory on
> your $PATH?

With recent git versions, this has stopped working. I was told it is a
feature that git only looks in exec-path now. Maybe I should not have
believed that?

Clemens

^ permalink raw reply

* What's cooking in git.git (Feb 2012, #05; Mon, 13)
From: Junio C Hamano @ 2012-02-13 20:42 UTC (permalink / raw)
  To: git

Here are the topics that have been cooking.  Commits prefixed with '-' are
only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'.

This round mosty consists of topics to fix new features introduced in
1.7.9, in preparation for 1.7.9.1 maintenance release.  There are still a
few more to come.

You can find the changes described here in the integration branches of the
repositories listed at

    http://git-blame.blogspot.com/p/git-public-repositories.html

--------------------------------------------------
[New Topics]

* bl/gitweb-project-filter (2012-02-12) 1 commit
  (merged to 'next' on 2012-02-13 at 35366b8)
 + gitweb: Harden and improve $project_filter page title

An update to the new feature to "gitweb" to show list of projects for
intermediate levels in directory hierarchies, already slated for 1.7.10.

* dp/i18n-libcharset (2012-02-13) 1 commit
  (merged to 'next' on 2012-02-13 at 528de77)
 + Makefile: introduce CHARSET_LIB to link with -lcharset

Some systems need to explicitly link -lcharset to get locale_charset().

* lt/pull-no-edit (2012-02-12) 1 commit
  (merged to 'next' on 2012-02-13 at 352f0cb)
 + "git pull" doesn't know "--edit"

For 1.7.10 where "git merge" by default spawns the editor when it can
automatically commit to record the resulting merge.

* mh/war-on-extra-refs (2012-02-12) 7 commits
  (merged to 'next' on 2012-02-13 at adb7353)
 + refs: remove the extra_refs API
 + clone: do not add alternate references to extra_refs
 + everything_local(): mark alternate refs as complete
 + fetch-pack.c: inline insert_alternate_refs()
 + fetch-pack.c: rename some parameters from "path" to "refname"
 + clone.c: move more code into the "if (refs)" conditional
 + t5700: document a failure of alternates to affect fetch

Internal API clean-up that is very cleanly done.

--------------------------------------------------
[Graduated to "master"]

* bw/inet-pton-ntop-compat (2012-02-05) 1 commit
  (merged to 'next' on 2012-02-06 at 61303e6)
 + Drop system includes from inet_pton/inet_ntop compatibility wrappers

The inclusion order of header files bites Solaris again and this fixes it.

* fc/zsh-completion (2012-02-06) 3 commits
  (merged to 'next' on 2012-02-06 at c94dd12)
 + completion: simplify __gitcomp and __gitcomp_nl implementations
 + completion: use ls -1 instead of rolling a loop to do that ourselves
 + completion: work around zsh option propagation bug

Fix git subcommand completion for zsh (in contrib/completion).

* jc/checkout-out-of-unborn (2012-02-06) 1 commit
  (merged to 'next' on 2012-02-07 at 60eb328)
 + git checkout -b: allow switching out of an unborn branch

"checkout -b" did not allow switching out of an unborn branch.

* jc/maint-commit-ignore-i-t-a (2012-02-07) 1 commit
  (merged to 'next' on 2012-02-10 at e0040cf)
 + commit: ignore intent-to-add entries instead of refusing

Replaces the nd/commit-ignore-i-t-a series that was made unnecessary
complicated by bad suggestions I made earlier.

* jc/maint-mailmap-output (2012-02-06) 1 commit
  (merged to 'next' on 2012-02-06 at 0a21425)
 + mailmap: always return a plain mail address from map_user()

map_user() was not rewriting its output correctly, which resulted in the
user visible symptom that "git blame -e" sometimes showed excess '>' at
the end of email addresses.

* jk/maint-tag-show-fixes (2012-02-08) 3 commits
  (merged to 'next' on 2012-02-08 at 18459c4)
 + tag: do not show non-tag contents with "-n"
 + tag: die when listing missing or corrupt objects
 + tag: fix output of "tag -n" when errors occur

Bugfixes to "git tag -n" that lacked much error checking.

* jk/prompt-fallback-to-tty (2012-02-03) 2 commits
  (merged to 'next' on 2012-02-06 at c0c995a)
 + prompt: fall back to terminal if askpass fails
 + prompt: clean up strbuf usage

The code to ask for password did not fall back to the terminal input when
GIT_ASKPASS is set but does not work (e.g. lack of X with GUI askpass
helper).

* jn/gitweb-search-utf-8 (2012-02-03) 1 commit
  (merged to 'next' on 2012-02-05 at 055e446)
 + gitweb: Allow UTF-8 encoded CGI query parameters and path_info

Search box in "gitweb" did not accept non-ASCII characters correctly.

* jn/merge-no-edit-fix (2012-02-09) 1 commit
  (merged to 'next' on 2012-02-10 at 014eec9)
 + merge: do not launch an editor on "--no-edit $tag"

In 1.7.9, "merge --no-edit $tag" incorrectly ignored --no-edit.

* mm/empty-loose-error-message (2012-02-06) 1 commit
  (merged to 'next' on 2012-02-07 at f119cac)
 + fsck: give accurate error message on empty loose object files

Updates the error message emitted when we see an empty loose object.

* mp/make-cleanse-x-for-exe (2012-02-09) 1 commit
  (merged to 'next' on 2012-02-09 at 35cc89d)
 + Explicitly set X to avoid potential build breakage

The makefile allowed environment variable X seep into it result in
command names suffixed with unnecessary strings.

* nd/cache-tree-api-refactor (2012-02-07) 1 commit
  (merged to 'next' on 2012-02-08 at a9abbca)
 + cache-tree: update API to take abitrary flags

Code cleanup.

* nd/diffstat-gramnum (2012-02-03) 1 commit
  (merged to 'next' on 2012-02-05 at 7335ecc)
 + Use correct grammar in diffstat summary line

The commands in the "git diff" family and "git apply --stat" that count
the number of files changed and the number of lines inserted/deleted have
been updated to match the output from "diffstat".  This also opens the
door to i18n this line.

* nd/find-pack-entry-recent-cache-invalidation (2012-02-01) 2 commits
  (merged to 'next' on 2012-02-01 at e26aed0)
 + find_pack_entry(): do not keep packed_git pointer locally
 + sha1_file.c: move the core logic of find_pack_entry() into fill_pack_entry()

* nk/ctype-for-perf (2012-02-10) 2 commits
  (merged to 'next' on 2012-02-10 at b41c6bb)
 + ctype: implement islower/isupper macro
 + ctype.c only wants git-compat-util.h

* tt/profile-build-fix (2012-02-09) 2 commits
  (merged to 'next' on 2012-02-09 at 1c183af)
 + Makefile: fix syntax for older make
  (merged to 'next' on 2012-02-07 at c8c5f3f)
 + Fix build problems related to profile-directed optimization

--------------------------------------------------
[Stalled]

* jc/advise-push-default (2011-12-18) 1 commit
 - push: hint to use push.default=upstream when appropriate

Peff had a good suggestion outlining an updated code structure so that
somebody new can try to dip his or her toes in the development. Any
takers?

* ss/git-svn-prompt-sans-terminal (2012-01-04) 3 commits
 - fixup! 15eaaf4
 - git-svn, perl/Git.pm: extend Git::prompt helper for querying users
 - perl/Git.pm: "prompt" helper to honor GIT_ASKPASS and SSH_ASKPASS

The bottom one has been replaced with a rewrite based on comments from
Ævar. The second one needs more work, both in perl/Git.pm and prompt.c, to
give precedence to tty over SSH_ASKPASS when terminal is available.

* jc/split-blob (2012-01-24) 6 commits
 - chunked-object: streaming checkout
 - chunked-object: fallback checkout codepaths
 - bulk-checkin: support chunked-object encoding
 - bulk-checkin: allow the same data to be multiply hashed
 - new representation types in the packstream
 - varint-in-pack: refactor varint encoding/decoding

Not ready.

I finished the streaming checkout codepath, but as explained in 127b177
(bulk-checkin: support chunked-object encoding, 2011-11-30), these are
still early steps of a long and painful journey. At least pack-objects and
fsck need to learn the new encoding for the series to be usable locally,
and then index-pack/unpack-objects needs to learn it to be used remotely.

Given that I heard a lot of noise that people want large files, and that I
was asked by somebody at GitTogether'11 privately for an advice on how to
pay developers (not me) to help adding necessary support, I am somewhat
dissapointed that the original patch series that was sent almost two
months ago still remains here without much comments and updates from the
developer community. I even made the interface to the logic that decides
where to split chunks easily replaceable, and I deliberately made the
logic in the original patch extremely stupid to entice others, especially
the "bup" fanboys, to come up with a better logic, thinking that giving
people an easy target to shoot for, they may be encouraged to help
out. The plan is not working :-(.

* nd/columns (2012-02-08) 15 commits
 - column: Fix some compiler and sparse warnings
 - column: add a corner-case test to t3200
 - columns: minimum coding style fixes
 - tag: add --column
 - column: support piping stdout to external git-column process
 - status: add --column
 - branch: add --column
 - help: reuse print_columns() for help -a
 - column: add column.ui for default column output settings
 - column: support columns with different widths
 - column: add columnar layout
 - Stop starting pager recursively
 - Add git-column and column mode parsing
 - column: add API to print items in columns
 - Save terminal width before setting up pager

The "show list of ..." mode of a handful of commands learn to produce
column-oriented output.

Expecting a reroll.

--------------------------------------------------
[Cooking]

* jn/ancient-meld-support (2012-02-10) 1 commit
  (merged to 'next' on 2012-02-13 at 28aca31)
 + mergetools/meld: Use --help output to detect --output support

More reliably tell if the given version of "meld" supports --output
option.

* jk/config-include (2012-02-06) 2 commits
  (merged to 'next' on 2012-02-13 at 307ddf6)
 + config: add include directive
 + docs: add a basic description of the config API

An assignment to the include.path pseudo-variable causes the named file
to be included in-place when Git looks up configuration variables.

* jk/userdiff-config-simplify (2012-02-07) 1 commit
  (merged to 'next' on 2012-02-10 at e9854c1)
 + drop odd return value semantics from userdiff_config

Code cleanup.

* tg/tag-points-at (2012-02-13) 2 commits
  (merged to 'next' on 2012-02-13 at a8f4046)
 + builtin/tag.c: Fix a sparse warning
  (merged to 'next' on 2012-02-10 at 4bff88f)
 + tag: add --points-at list option

* jl/maint-submodule-relative (2012-02-09) 2 commits
 - submodules: always use a relative path from gitdir to work tree
 - submodules: always use a relative path to gitdir

The second one looked iffy.

* ld/git-p4-expanded-keywords (2012-02-09) 2 commits
 - git-p4: initial demonstration of possible RCS keyword fixup
 - git-p4: add test case for RCS keywords

Waiting for reviews and user reports.

* jk/grep-binary-attribute (2012-02-02) 9 commits
  (merged to 'next' on 2012-02-05 at 9dffa7e)
 + grep: pre-load userdiff drivers when threaded
 + grep: load file data after checking binary-ness
 + grep: respect diff attributes for binary-ness
 + grep: cache userdiff_driver in grep_source
 + grep: drop grep_buffer's "name" parameter
 + convert git-grep to use grep_source interface
 + grep: refactor the concept of "grep source" into an object
 + grep: move sha1-reading mutex into low-level code
 + grep: make locking flag global

Fixes a longstanding bug that there was no way to tell "git grep" that a
path may look like text but it is not, which "git diff" can do using the
attributes system. Now "git grep" honors the same "binary" (or "-diff")
attribute.

Will merge to 'master' after 1.7.9.1 settles.

* jk/git-dir-lookup (2012-02-02) 1 commit
  (merged to 'next' on 2012-02-05 at 1856d74)
 + standardize and improve lookup rules for external local repos

When you have both .../foo and .../foo.git, "git clone .../foo" did not
favor the former but the latter.

Will merge to 'master' after 1.7.9.1 settles.

* nd/pack-objects-parseopt (2012-02-01) 3 commits
  (merged to 'next' on 2012-02-05 at d0dc25d)
 + pack-objects: convert to use parse_options()
 + pack-objects: remove bogus comment
 + pack-objects: do not accept "--index-version=version,"

"pack-objects" learned use parse-options, losing custom command line
parsing code.

Will merge to 'master' after 1.7.9.1 settles.

^ permalink raw reply

* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Junio C Hamano @ 2012-02-13 20:29 UTC (permalink / raw)
  To: Michael Haggerty; +Cc: Tom Grennan, Albert Yale, pclouds, git, krh, jasampler
In-Reply-To: <4F391F5C.1000400@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> 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.
>
> That *has* been exactly my argument from the beginning [1].

Well, if the only thing you say is "rev-list A B ^C" and you are expecting
that your reader to substitute A with any dashed option like --all, --not
or --stdin, I would have to say you are expecting too much.  I wouldn't
even think of substituting A with $(git for-each-ref refs/heads/) in such
an example.

> I cautiously hope that we are now talking about the same thing, even if it
> is not yet clear whether we agree on a conclusion.

I think we are on the same page now.  Given that we seem to have settled
for the recent "find in the paths that pathspec matches, but exclude
matches from paths that match these patterns" topic by Albert Yale to
tentatively use separate --exclude [*1*] command line option instead of
mixing the negatives to the usual list of positives at the UI level, it
appears to me that the most sensible way forward would be to expose the
negative match to the UI level is to introduce a similar --exclude-refs on
the command line.

That would eventually allow us to say something like [*2*]:

	git log --all \
            --exclude-refs=refs/heads/experimental/* \
            --exclude-paths=compat/ \
	    --since=30.days \
            -- '*.c'

to ask for recent changes to all C sources outside the compat/ area for
everything except for the experimental topics.


[Footnote]

*1* It might be better to spell this as --exclude-paths, though, if we are
going to call this other exclude-refs-by-pattern --exclude-refs.  The
longer names would not bother us with the help of parse-options, and the
commands that need to support both, namely the commands in the log family,
need to allow users to be explicit which exclusion they want anyway, and
having --exclude vs --exclude-refs as options that work on different
dimensions look asymmetric.

*2* "something like" includes using a convenience short-hand on this
command line such as "--branches --exclude-branches=experimental/*"
instead of "--all --exclude-refs=...", but I consider that is icing on the
cake after the first step to define the overall structure settles, so I
would prefer not to go into the tangential details.

^ permalink raw reply

* [PATCH 2/5] do not override receive-pack errors
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>

Receive runs rev-list --verify-objects in order to detect missing
objects. However, such errors are ignored and overridden later.
Instead, consequently ignore all update commands for which an error has
already been detected.

Some tests in t5504 are obsoleted by this change, because invalid
objects are detected even if fsck is not enabled. Instead, they now test
for different error messages depending on whether or not fsck is turned
on. A better fix would be to force a corruption that will be detected by
fsck but not by rev-list.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 builtin/receive-pack.c          |   24 +++++++++++++++++-------
 t/t5504-fetch-receive-strict.sh |   22 ++++++++++++++++++----
 2 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index fa7448b..0afb8b2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -642,8 +642,10 @@ static void check_aliased_updates(struct command *commands)
 	}
 	sort_string_list(&ref_list);
 
-	for (cmd = commands; cmd; cmd = cmd->next)
-		check_aliased_update(cmd, &ref_list);
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		if (!cmd->error_string)
+			check_aliased_update(cmd, &ref_list);
+	}
 
 	string_list_clear(&ref_list, 0);
 }
@@ -707,8 +709,10 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 		set_connectivity_errors(commands);
 
 	if (run_receive_hook(commands, pre_receive_hook, 0)) {
-		for (cmd = commands; cmd; cmd = cmd->next)
-			cmd->error_string = "pre-receive hook declined";
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (!cmd->error_string)
+				cmd->error_string = "pre-receive hook declined";
+		}
 		return;
 	}
 
@@ -717,9 +721,15 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 	free(head_name_to_free);
 	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
 
-	for (cmd = commands; cmd; cmd = cmd->next)
-		if (!cmd->skip_update)
-			cmd->error_string = update(cmd);
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		if (cmd->error_string)
+			continue;
+
+		if (cmd->skip_update)
+			continue;
+
+		cmd->error_string = update(cmd);
+	}
 }
 
 static struct command *read_head_info(void)
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 8341fc4..35ec294 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -58,6 +58,11 @@ test_expect_success 'fetch with transfer.fsckobjects' '
 	)
 '
 
+cat >exp <<EOF
+To dst
+!	refs/heads/master:refs/heads/test	[remote rejected] (missing necessary objects)
+EOF
+
 test_expect_success 'push without strict' '
 	rm -rf dst &&
 	git init dst &&
@@ -66,7 +71,8 @@ test_expect_success 'push without strict' '
 		git config fetch.fsckobjects false &&
 		git config transfer.fsckobjects false
 	) &&
-	git push dst master:refs/heads/test
+	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+	test_cmp exp act
 '
 
 test_expect_success 'push with !receive.fsckobjects' '
@@ -77,9 +83,15 @@ test_expect_success 'push with !receive.fsckobjects' '
 		git config receive.fsckobjects false &&
 		git config transfer.fsckobjects true
 	) &&
-	git push dst master:refs/heads/test
+	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+	test_cmp exp act
 '
 
+cat >exp <<EOF
+To dst
+!	refs/heads/master:refs/heads/test	[remote rejected] (n/a (unpacker error))
+EOF
+
 test_expect_success 'push with receive.fsckobjects' '
 	rm -rf dst &&
 	git init dst &&
@@ -88,7 +100,8 @@ test_expect_success 'push with receive.fsckobjects' '
 		git config receive.fsckobjects true &&
 		git config transfer.fsckobjects false
 	) &&
-	test_must_fail git push dst master:refs/heads/test
+	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+	test_cmp exp act
 '
 
 test_expect_success 'push with transfer.fsckobjects' '
@@ -98,7 +111,8 @@ test_expect_success 'push with transfer.fsckobjects' '
 		cd dst &&
 		git config transfer.fsckobjects true
 	) &&
-	test_must_fail git push dst master:refs/heads/test
+	test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+	test_cmp exp act
 '
 
 test_done
-- 
1.7.9

^ permalink raw reply related

* [PATCH 5/5] push/fetch/clone --no-progress suppresses progress output
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>

By default, progress output is disabled if stderr is not a terminal.
The --progress option can be used to force progress output anyways.
Conversely, --no-progress does not force progress output. In particular,
if stderr is a terminal, progress output is enabled.

This is unintuitive. Change --no-progress to force output off.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 builtin/clone.c          |    6 +++---
 builtin/fetch-pack.c     |    2 +-
 builtin/fetch.c          |    4 ++--
 builtin/push.c           |    4 ++--
 builtin/send-pack.c      |   12 +++++++-----
 t/t5523-push-upstream.sh |    3 ++-
 transport.c              |    6 +++++-
 7 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index c62d4b5..09dbb09 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -45,7 +45,7 @@ static char *option_branch = NULL;
 static const char *real_git_dir;
 static char *option_upload_pack = "git-upload-pack";
 static int option_verbosity;
-static int option_progress;
+static int option_progress = -1;
 static struct string_list option_config;
 static struct string_list option_reference;
 
@@ -60,8 +60,8 @@ static int opt_parse_reference(const struct option *opt, const char *arg, int un
 
 static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
-	OPT_BOOLEAN(0, "progress", &option_progress,
-			"force progress reporting"),
+	OPT_BOOL(0, "progress", &option_progress,
+		 "force progress reporting"),
 	OPT_BOOLEAN('n', "no-checkout", &option_no_checkout,
 		    "don't create a checkout"),
 	OPT_BOOLEAN(0, "bare", &option_bare, "create a bare repository"),
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 6207ecd..a4d3e90 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -736,7 +736,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
 	}
 	else {
 		*av++ = "unpack-objects";
-		if (args.quiet)
+		if (args.quiet || args.no_progress)
 			*av++ = "-q";
 	}
 	if (*hdr_arg)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ab18633..65f5f9b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -30,7 +30,7 @@ enum {
 };
 
 static int all, append, dry_run, force, keep, multiple, prune, update_head_ok, verbosity;
-static int progress, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT;
 static const char *depth;
 static const char *upload_pack;
@@ -78,7 +78,7 @@ static struct option builtin_fetch_options[] = {
 	OPT_BOOLEAN('k', "keep", &keep, "keep downloaded pack"),
 	OPT_BOOLEAN('u', "update-head-ok", &update_head_ok,
 		    "allow updating of HEAD ref"),
-	OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+	OPT_BOOL(0, "progress", &progress, "force progress reporting"),
 	OPT_STRING(0, "depth", &depth, "depth",
 		   "deepen history of shallow clone"),
 	{ OPTION_STRING, 0, "submodule-prefix", &submodule_prefix, "dir",
diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..6c373cf 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -19,7 +19,7 @@ static int thin;
 static int deleterefs;
 static const char *receivepack;
 static int verbosity;
-static int progress;
+static int progress = -1;
 
 static const char **refspec;
 static int refspec_nr;
@@ -260,7 +260,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
 		OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
 			TRANSPORT_PUSH_SET_UPSTREAM),
-		OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+		OPT_BOOL(0, "progress", &progress, "force progress reporting"),
 		OPT_END()
 	};
 
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 7d514ca..8fc9789 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -58,7 +58,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 		argv[i++] = "--thin";
 	if (args->use_ofs_delta)
 		argv[i++] = "--delta-base-offset";
-	if (args->quiet)
+	if (args->quiet || !args->progress)
 		argv[i++] = "-q";
 	if (args->progress)
 		argv[i++] = "--progress";
@@ -250,6 +250,7 @@ int send_pack(struct send_pack_args *args,
 	int allow_deleting_refs = 0;
 	int status_report = 0;
 	int use_sideband = 0;
+	int quiet_supported = 0;
 	unsigned cmds_sent = 0;
 	int ret;
 	struct async demux;
@@ -263,8 +264,8 @@ int send_pack(struct send_pack_args *args,
 		args->use_ofs_delta = 1;
 	if (server_supports("side-band-64k"))
 		use_sideband = 1;
-	if (!server_supports("quiet"))
-		args->quiet = 0;
+	if (server_supports("quiet"))
+		quiet_supported = 1;
 	if (server_supports("verify-refs"))
 		args->verify_refs = 1;
 
@@ -304,13 +305,14 @@ int send_pack(struct send_pack_args *args,
 		} else {
 			char *old_hex = sha1_to_hex(ref->old_sha1);
 			char *new_hex = sha1_to_hex(ref->new_sha1);
+			int quiet = quiet_supported && (args->quiet || !args->progress);
 
-			if (!cmds_sent && (status_report || use_sideband || args->quiet || args->verify_refs)) {
+			if (!cmds_sent && (status_report || use_sideband || quiet || args->verify_refs)) {
 				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s%s",
 					old_hex, new_hex, ref->name, 0,
 					status_report ? " report-status" : "",
 					use_sideband ? " side-band-64k" : "",
-					args->quiet ? " quiet" : "",
+					quiet ? " quiet" : "",
 					args->verify_refs ? " verify-refs" : "");
 			}
 			else
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 9ee52cf..3683df1 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -101,10 +101,11 @@ test_expect_success TTY 'push -q suppresses progress' '
 	! grep "Writing objects" err
 '
 
-test_expect_failure TTY 'push --no-progress suppresses progress' '
+test_expect_success TTY 'push --no-progress suppresses progress' '
 	ensure_fresh_upstream &&
 
 	test_terminal git push -u --no-progress upstream master >out 2>err &&
+	! grep "Unpacking objects" err &&
 	! grep "Writing objects" err
 '
 
diff --git a/transport.c b/transport.c
index cac0c06..6074ee9 100644
--- a/transport.c
+++ b/transport.c
@@ -994,10 +994,14 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
 	 * when a rule is satisfied):
 	 *
 	 *   1. Report progress, if force_progress is 1 (ie. --progress).
+	 *   2. Don't report progress, if force_progress is 0 (ie. --no-progress).
 	 *   2. Don't report progress, if verbosity < 0 (ie. -q/--quiet ).
 	 *   3. Report progress if isatty(2) is 1.
 	 **/
-	transport->progress = force_progress || (verbosity >= 0 && isatty(2));
+	if (force_progress >= 0)
+		transport->progress = !!force_progress;
+	else
+		transport->progress = verbosity >= 0 && isatty(2);
 }
 
 int transport_push(struct transport *transport,
-- 
1.7.9

^ permalink raw reply related

* [PATCH 1/5] git rev-list: fix invalid typecast
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>

git rev-list passes rev_list_info, not rev_list objects. Without this
fix, rev-list enables or disables the --verify-objects option depending
on a read from an undefined memory location.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 builtin/rev-list.c |    4 ++--
 t/t1450-fsck.sh    |   26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ab3be7c..264e3ae 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -180,10 +180,10 @@ static void show_object(struct object *obj,
 			const struct name_path *path, const char *component,
 			void *cb_data)
 {
-	struct rev_info *info = cb_data;
+	struct rev_list_info *info = cb_data;
 
 	finish_object(obj, path, component, cb_data);
-	if (info->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
+	if (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
 		parse_object(obj->sha1);
 	show_object_with_name(stdout, obj, path, component);
 }
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 523ce9c..5b8ebd8 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -191,4 +191,30 @@ test_expect_success 'cleaned up' '
 	test_cmp empty actual
 '
 
+test_expect_success 'rev-list --verify-objects' '
+	git rev-list --verify-objects --all >/dev/null 2>out &&
+	test_cmp empty out
+'
+
+test_expect_success 'rev-list --verify-objects with bad sha1' '
+	sha=$(echo blob | git hash-object -w --stdin) &&
+	old=$(echo $sha | sed "s+^..+&/+") &&
+	new=$(dirname $old)/ffffffffffffffffffffffffffffffffffffff &&
+	sha="$(dirname $new)$(basename $new)" &&
+	mv .git/objects/$old .git/objects/$new &&
+	test_when_finished "remove_object $sha" &&
+	git update-index --add --cacheinfo 100644 $sha foo &&
+	test_when_finished "git read-tree -u --reset HEAD" &&
+	tree=$(git write-tree) &&
+	test_when_finished "remove_object $tree" &&
+	cmt=$(echo bogus | git commit-tree $tree) &&
+	test_when_finished "remove_object $cmt" &&
+	git update-ref refs/heads/bogus $cmt &&
+	test_when_finished "git update-ref -d refs/heads/bogus" &&
+
+	test_might_fail git rev-list --verify-objects refs/heads/bogus >/dev/null 2>out &&
+	cat out &&
+	grep -q "error: sha1 mismatch 63ffffffffffffffffffffffffffffffffffffff" out
+'
+
 test_done
-- 
1.7.9

^ permalink raw reply related

* [PATCH 0/5] verify refs early
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhazobto3.fsf@alter.siamese.dyndns.org>

Hi Junio,

This series centers around "[PATCH 3/5] git push: verify refs early",
which implements your suggestions in reply to my "[PATCH] optionally
deny all pushes". I am not pursuing the deny config option further,
because as you noted, it is of course made redundant by a simple
pre-receive hook.

[PATCH 1/5] git rev-list: fix invalid typecast

I consider this one a fairly important bugfix. Although I was unable to
determine under which circumstances the bug takes effect, rev-list
probably takes a performance hit when it does.

[PATCH 2/5] do not override receive-pack errors
[PATCH 3/5] git push: verify refs early

Note that [PATCH 3/5] makes an extension to the transfer protocol.

[PATCH 4/5] t5541: use configured port number

A mostly unrelated bugfix.

[PATCH 5/5] push/fetch/clone --no-progress suppresses progress

A totally unrelated change. However, this conflicts textually
(lexically?) with [PATCH 3/5], and therefore I am including it here.

Clemens

^ permalink raw reply

* [PATCH 4/5] t5541: use configured port number
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>


Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 t/t5541-http-push.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index d66ed24..cc6f081 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -106,7 +106,7 @@ cat >exp <<EOF
 remote: error: hook declined to update refs/heads/dev2
 To http://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git
  ! [remote rejected] dev2 -> dev2 (hook declined)
-error: failed to push some refs to 'http://127.0.0.1:5541/smart/test_repo.git'
+error: failed to push some refs to 'http://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git'
 EOF
 
 test_expect_success 'rejected update prints status' '
-- 
1.7.9

^ permalink raw reply related

* [PATCH 3/5] git push: verify refs early
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>

Receive-pack waits for the transfer of all new object data to complete
before taking a closer look at the received refs. It may then decide to
refuse updates of refs without even looking at the data.

Instead, do as much ref validation as possible before new object data is
available, in order to avoid a potentially expensive transfer.
Receive-pack will then respond with "verify-refs ok" if at least one ref
update is valid. Otherwise, it will respond with "verify-refs no valid
refs".

On the client side, send-pack will wait for "verify-refs ok" or skip the
object transfer entirely if it reads a packet line that starts with
"verify-refs " but does not match "verify-refs ok". Any other response
is considered an error.

Early ref validation is disabled for the smart HTTP protocol, since it
tries to fit the entire push (ref info and object data) into a single
POST request.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

I suppose with some effort, this could be done for smart HTTP as well.
But I am not sure if we actually want the overhead of the additional
ping-pong for HTTP.

 builtin/receive-pack.c |   83 ++++++++++++++++++++++++++++++++++++++----------
 builtin/send-pack.c    |   43 +++++++++++++++++--------
 send-pack.h            |    3 +-
 3 files changed, 97 insertions(+), 32 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0afb8b2..0129d9c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -34,6 +34,8 @@ static int unpack_limit = 100;
 static int report_status;
 static int use_sideband;
 static int quiet;
+static int verify_refs;
+static int stateless_rpc;
 static int prefer_ofs_delta = 1;
 static int auto_update_server_info;
 static int auto_gc = 1;
@@ -123,7 +125,7 @@ static void show_ref(const char *path, const unsigned char *sha1)
 	else
 		packet_write(1, "%s %s%c%s%s\n",
 			     sha1_to_hex(sha1), path, 0,
-			     " report-status delete-refs side-band-64k quiet",
+			     " report-status delete-refs side-band-64k quiet verify-refs",
 			     prefer_ofs_delta ? " ofs-delta" : "");
 	sent_capabilities = 1;
 }
@@ -410,14 +412,13 @@ static void refuse_unconfigured_deny_delete_current(void)
 		rp_error("%s", refuse_unconfigured_deny_delete_current_msg[i]);
 }
 
-static const char *update(struct command *cmd)
+static const char *verify_ref(struct command *cmd)
 {
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
 	const char *namespaced_name;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
-	struct ref_lock *lock;
 
 	/* only refs/... are allowed */
 	if (prefixcmp(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -444,12 +445,6 @@ static const char *update(struct command *cmd)
 		}
 	}
 
-	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
-		error("unpack should have generated %s, "
-		      "but I can't find it!", sha1_to_hex(new_sha1));
-		return "bad pack";
-	}
-
 	if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
 		if (deny_deletes && !prefixcmp(name, "refs/heads/")) {
 			rp_error("denying ref deletion for %s", name);
@@ -473,6 +468,27 @@ static const char *update(struct command *cmd)
 		}
 	}
 
+	return NULL;
+}
+
+static const char *update(struct command *cmd)
+{
+	const char *name = cmd->ref_name;
+	struct strbuf namespaced_name_buf = STRBUF_INIT;
+	const char *namespaced_name;
+	unsigned char *old_sha1 = cmd->old_sha1;
+	unsigned char *new_sha1 = cmd->new_sha1;
+	struct ref_lock *lock;
+
+	strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
+	namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
+
+	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
+		error("unpack should have generated %s, "
+		      "but I can't find it!", sha1_to_hex(new_sha1));
+		return "bad pack";
+	}
+
 	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
 	    !is_null_sha1(old_sha1) &&
 	    !prefixcmp(name, "refs/heads/")) {
@@ -692,10 +708,41 @@ static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])
 	return -1; /* end of list */
 }
 
+static int verify_ref_commands(struct command *commands)
+{
+	unsigned char sha1[20];
+	int commands_ok;
+	struct command *cmd;
+
+	free(head_name_to_free);
+	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
+
+	commands_ok = 0;
+	for (cmd = commands; cmd; cmd = cmd->next) {
+		cmd->error_string = verify_ref(cmd);
+		if (!cmd->error_string)
+			commands_ok++;
+	}
+
+	if (verify_refs && !stateless_rpc) {
+		struct strbuf buf = STRBUF_INIT;
+
+		packet_buf_write(&buf, "verify-refs %s\n",
+			 commands_ok > 0 ? "ok" : "no valid refs");
+
+		if (use_sideband)
+			send_sideband(1, 1, buf.buf, buf.len, use_sideband);
+		else
+			safe_write(1, buf.buf, buf.len);
+		strbuf_release(&buf);
+	}
+
+	return commands_ok;
+}
+
 static void execute_commands(struct command *commands, const char *unpacker_error)
 {
 	struct command *cmd;
-	unsigned char sha1[20];
 
 	if (unpacker_error) {
 		for (cmd = commands; cmd; cmd = cmd->next)
@@ -718,9 +765,6 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 
 	check_aliased_updates(commands);
 
-	free(head_name_to_free);
-	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
-
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (cmd->error_string)
 			continue;
@@ -766,6 +810,8 @@ static struct command *read_head_info(void)
 				use_sideband = LARGE_PACKET_MAX;
 			if (parse_feature_request(feature_list, "quiet"))
 				quiet = 1;
+			if (parse_feature_request(feature_list, "verify-refs"))
+				verify_refs = 1;
 		}
 		cmd = xcalloc(1, sizeof(struct command) + len - 80);
 		hashcpy(cmd->old_sha1, old_sha1);
@@ -905,7 +951,6 @@ static int delete_only(struct command *commands)
 int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 {
 	int advertise_refs = 0;
-	int stateless_rpc = 0;
 	int i;
 	char *dir = NULL;
 	struct command *commands;
@@ -962,11 +1007,15 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		return 0;
 
 	if ((commands = read_head_info()) != NULL) {
+		int commands_ok;
 		const char *unpack_status = NULL;
 
-		if (!delete_only(commands))
-			unpack_status = unpack();
-		execute_commands(commands, unpack_status);
+		commands_ok = verify_ref_commands(commands);
+		if (!verify_refs || commands_ok > 0) {
+			if (!delete_only(commands))
+				unpack_status = unpack();
+			execute_commands(commands, unpack_status);
+		}
 		if (pack_lockfile)
 			unlink_or_warn(pack_lockfile);
 		if (report_status)
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 71f258e..7d514ca 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -265,6 +265,8 @@ int send_pack(struct send_pack_args *args,
 		use_sideband = 1;
 	if (!server_supports("quiet"))
 		args->quiet = 0;
+	if (server_supports("verify-refs"))
+		args->verify_refs = 1;
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -303,12 +305,13 @@ int send_pack(struct send_pack_args *args,
 			char *old_hex = sha1_to_hex(ref->old_sha1);
 			char *new_hex = sha1_to_hex(ref->new_sha1);
 
-			if (!cmds_sent && (status_report || use_sideband || args->quiet)) {
-				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
+			if (!cmds_sent && (status_report || use_sideband || args->quiet || args->verify_refs)) {
+				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s%s",
 					old_hex, new_hex, ref->name, 0,
 					status_report ? " report-status" : "",
 					use_sideband ? " side-band-64k" : "",
-					args->quiet ? " quiet" : "");
+					args->quiet ? " quiet" : "",
+					args->verify_refs ? " verify-refs" : "");
 			}
 			else
 				packet_buf_write(&req_buf, "%s %s %s",
@@ -341,17 +344,29 @@ int send_pack(struct send_pack_args *args,
 		in = demux.out;
 	}
 
-	if (new_refs && cmds_sent) {
-		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
-			for (ref = remote_refs; ref; ref = ref->next)
-				ref->status = REF_STATUS_NONE;
-			if (args->stateless_rpc)
-				close(out);
-			if (git_connection_is_socket(conn))
-				shutdown(fd[0], SHUT_WR);
-			if (use_sideband)
-				finish_async(&demux);
-			return -1;
+	if (cmds_sent) {
+		int verify_refs_status = 0;
+
+		if (args->verify_refs && !args->stateless_rpc) {
+			char line[1000];
+			int len = packet_read_line(in, line, sizeof(line));
+			if (len < 15 || memcmp(line, "verify-refs ", 12))
+				return error("did not receive remote status");
+			verify_refs_status = memcmp(line, "verify-refs ok\n", 15);
+		}
+
+		if (!verify_refs_status && new_refs) {
+			if (pack_objects(out, remote_refs, extra_have, args) < 0) {
+				for (ref = remote_refs; ref; ref = ref->next)
+					ref->status = REF_STATUS_NONE;
+				if (args->stateless_rpc)
+					close(out);
+				if (git_connection_is_socket(conn))
+					shutdown(fd[0], SHUT_WR);
+				if (use_sideband)
+					finish_async(&demux);
+				return -1;
+			}
 		}
 	}
 	if (args->stateless_rpc && cmds_sent)
diff --git a/send-pack.h b/send-pack.h
index 05d7ab1..87edaa5 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -11,7 +11,8 @@ struct send_pack_args {
 		use_thin_pack:1,
 		use_ofs_delta:1,
 		dry_run:1,
-		stateless_rpc:1;
+		stateless_rpc:1,
+		verify_refs:1;
 };
 
 int send_pack(struct send_pack_args *args,
-- 
1.7.9

^ permalink raw reply related

* Re: [RFC PATCH 0/3] git-p4: move to toplevel
From: Junio C Hamano @ 2012-02-13 20:00 UTC (permalink / raw)
  To: Pete Wyckoff; +Cc: git, Luke Diamand, Vitor Antunes
In-Reply-To: <1329070423-23761-1-git-send-email-pw@padd.com>

Pete Wyckoff <pw@padd.com> writes:

> Users install git-p4 currently by copying the git-p4 script from
> contrib/fast-import into a local or personal bin directory, and
> setting up an alias for "git p4" to invoke it.

Erm,... do you really need the alias if you add git-p4 in a directory on
your $PATH?

^ permalink raw reply

* Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree
From: Junio C Hamano @ 2012-02-13 19:59 UTC (permalink / raw)
  To: Jens Lehmann; +Cc: Git Mailing List, Antony Male, Phil Hord
In-Reply-To: <4F338156.9090507@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

> The first version was whitespace damaged, please use this instead.

Thanks.  This one looks somewhat iffy (1/2 looked perfectly fine, though).

> +	a=$(cd "$gitdir" && pwd)
> +	b=$(cd "$path" && pwd)
> +	while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
> +	do
> +		a=${a#*/} b=${b#*/};
> +	done
> +	rel=$(echo $a | sed -e 's|[^/]*|..|g')
> +	(clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
>  }


The style ([ "$b" ] vs "test -n "$b") aside, it strikes me odd that you
only check $b; it is unclear what guarantees that "$a" is always longer.
Maybe there is a reason but then a one-line comment here would not hurt?

^ permalink raw reply

* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Jakub Narebski @ 2012-02-13 19:58 UTC (permalink / raw)
  To: Michal Kiedrowicz; +Cc: git
In-Reply-To: <20120213075451.1bc20885@mkiedrowicz.ivo.pl>

Michal Kiedrowicz wrote:
> Jakub Narebski <jnareb@gmail.com> wrote:
> > Jakub Narebski <jnareb@gmail.com> writes:
[...]
> > > 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 :)
 
Yes, you are right.  Let's do it when we actually need it.

[...]
> > >    # 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 {
[...]
> > 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...

-- 
Jakub Narebski
Poland

^ permalink raw reply

* Re: [BUG] git-merge-octopus creates an empty merge commit with one parent
From: Junio C Hamano @ 2012-02-13 19:51 UTC (permalink / raw)
  To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <20120213202039.5346fa89@gmail.com>

Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:

> ... Except that I still think this behavior is not correct.

Well, what was missing from my "don't do it then" quote was "Doctor it
HURTS when I do this."; there is no question that it is not correct if it
hurts ;-).

Patches welcome; I suspect it shouldn't be too intrusive or difficult.

^ permalink raw reply

* Re: [BUG] git-merge-octopus creates an empty merge commit with one parent
From: Michał Kiedrowicz @ 2012-02-13 19:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vipjafu27.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> wrote:

> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> 
> > This happens when git merge is run to merge multiple commits that are
> > descendants of current HEAD (or are HEAD).
> 
> I am reasonably sure you meant ancestors here.

I meant: ... to merge commits whose parent, grand-parent or
grand-grand-...-parent is HEAD. Commits to which HEAD may be
fast-forwarded.

> 
> > to origin/master but accidentaly we called (while being on master):
> >
> > 	$ git merge master origin/master
> 
> I am very tempted to throw this into "don't do it then" category.

I'm all for it. This is not a thing I would want to be doing. At
no time I would call this "a serious bug that needs fixing right
now". Except that I still think this behavior is not correct. git-merge
should not create commits that even aren't merges (they have single
parrent) and have exactly the same tree as its parent, and give it a
message "merge". Isn't this against the normal behavior of Git:
forwarding if possible and refusing to create commits that don't change
any file?

^ permalink raw reply

* Re: [PATCH] builtin/tag.c: Fix a sparse warning
From: Tom Grennan @ 2012-02-13 19:15 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <4F395569.8030106@ramsay1.demon.co.uk>

On Mon, Feb 13, 2012 at 06:24:41PM +0000, Ramsay Jones wrote:
>
>In particular, sparse complains as follows:
>
>        SP builtin/tag.c
>    builtin/tag.c:411:5: warning: symbol 'parse_opt_points_at' was \
>        not declared. Should it be static?
>
>In order to suppress the warning, since the parse_opt_points_at()
>function does not need to be an external symbol, we simply add the
>static modifier to the function definition.
>
>Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>

Acked-by: Tom Grennan <tmgrennan@gmail.com>

Thanks

>---
>
> builtin/tag.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
>diff --git a/builtin/tag.c b/builtin/tag.c
>index 8cfaaf8..fe7e5e5 100644
>--- a/builtin/tag.c
>+++ b/builtin/tag.c
>@@ -408,7 +408,7 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
> 	return check_refname_format(sb->buf, 0);
> }
> 
>-int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
>+static int parse_opt_points_at(const struct option *opt __attribute__((unused)),
> 			const char *arg, int unset)
> {
> 	unsigned char sha1[20];
>-- 
>1.7.9
>
>

^ permalink raw reply

* Re: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines
From: Jakub Narebski @ 2012-02-13 19:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: rajesh boyapati, git
In-Reply-To: <7vmx8mfu8k.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Anyway, here is the patch that should fix those "CGI: fatal: Not a valid
> > object name HEAD" errors for you.
> 
> I have to wonder if it is simpler and less error prone to check HEAD
> before doing anything else immediately after which repository is being
> consulted, and give the same "no history at all yet in this project" page
> for most if not all operations, instead of patching things up at this deep
> in the callchain.

Actually the problem is that there is history (there are other branches),
but HEAD points to unborn (unused) 'master' branch.

But you are right that we should fix underlying issue with invalid
assumption which gitweb uses, that HEAD points to a valid commit if
repository is non-empty.

Though I think leaving safety of 2nd patch could be a good idea anyway.
And 1st patch fixes a real issue, and does not only provide band-aid.


P.S. I will resend those three patches as patch series for easier review,
so they are not tangled in this deep thread.  They should be in 'gitweb/web'
branch in my public forks of git repository on repo.or.cz and github.

-- 
Jakub Narebski
Poland

^ permalink raw reply

* Re: Setting up a Git server (+ gitweb) with .htaccess files HOWTO
From: Junio C Hamano @ 2012-02-13 19:10 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git
In-Reply-To: <vpqbop266ak.fsf@bauges.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> I've set up a Git server on a machine on which I have a webspace,
> permission to run CGI scripts, but no shell or root access. Good news:
> it worked :-).
>
> I've documented the process here in case anyone's interested:
>
>   http://www-verimag.imag.fr/~moy/?Host-a-Git-repository-over-HTTP-S

After seeing the section that runs "git init" in a throw-away CGI script,
I started wondering what the point of this site in forbidding a shell
access in the first place.

After all, if you changed that example with a script that feeds its input
to a shell, you would have enough "shell access" to do anything you need.

So...

Well, my reaction at this point was "why bother?", but then I realized
that that reaction is meant for the administrator of that hosting site,
not a user of that site, i.e. you.

Thanks for an amusing read, anyway.

^ permalink raw reply

* Re: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines
From: Jakub Narebski @ 2012-02-13 19:04 UTC (permalink / raw)
  To: rajesh boyapati; +Cc: git
In-Reply-To: <CA+EqV8xin_ubOoGouhHz2qnzoHrpMMQsjUTXnrtmsxRTLPZtZQ@mail.gmail.com>

On Mon, 13 Feb 2012, rajesh boyapati wrote:
> 2012/2/13 Jakub Narebski <jnareb@gmail.com>
>> On Mon, 13 Feb 2012, rajesh boyapati wrote:

>>> I am getting this error with this patch
>>>>>>>>>>>>>>>>>>>>>
>>> [2012-02-13 11:20:19,268] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: usage: git rev-list
>>> [OPTION] <commit-id>... [ -- paths... ]
>>> [2012-02-13 11:20:19,268] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI:   limiting output:
>>> [2012-02-13 11:20:19,268] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI:     --max-count=nr
>> [...]
>>> [2012-02-13 11:20:27,913] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad flag '--2'
>>> used after filename
>>> [2012-02-13 11:20:32,579] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad flag '--2'
>>> used after filename
>>> <<<<<<<<<<<<<<<<<<<
>>
>> Strange, I cannot reproduce this with non-Gerrit gitweb.  It looks
>> like it somehow lost in between "... -- 2>/dev/null" at the end of
>> git-rev-list command, and fails at "--2" which is bad flag.
>>
> This is the patch I applied
>>>>>>>>>>>>>
> sub parse_commit {
>     my ($commit_id) = @_;
>     my %co;
> 
>         return unless defined $commit_id;
> 
>     local $/ = "\0";
> 
>     *open my $fd, "-|", quote_command(
>              git_cmd(), "rev-list",*
>         "--parents",
>         "--header",
>         "--max-count=1",
>         $commit_id,
>        * "--") . '2>/dev/null',*
                   ^^^^^^^^^^^^^

It should be ' 2>/dev/null', with space before redirection, and not
'2>/dev/null'.  This space is here necessary.

> With this Patch, Gerrit's gitweb is not showing anything.
> I mean, I can access gitweb from gerrit, but if I click on Tabs(like log,
> commit, etc...which worked with previous patches), I cannot see any thing.
> 
> Even with previous patches also there is no improvement in Gerrit's gitweb,
> only some of the errors are gone in error_log.
> The improvement I am talking about is "If I click other tabs(log, shortlog,
> commit, tree,etc) after clicking "summary", Gerrit's gitweb is not showing
> anything".

Many views in gitweb do default to HEAD.  If HEAD does not point to a valid
commit, they would fail, in better or worse way.

Except for the first one those patches are more of band-aid and workaround
than fixing underlying issue that gitweb assumes that HEAD is valid in 
non-empty repository.  But fixing this will require more work.

-- 
Jakub Narebski
Poland

^ permalink raw reply


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