* Re: [PATCH v3 2/3] valgrind: ignore ldso and more libz errors
From: Jeff King @ 2009-01-26 0:14 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.1.00.0901260101030.14855@racer>
On Mon, Jan 26, 2009 at 01:02:24AM +0100, Johannes Schindelin wrote:
> > Your 0/3 cover letter lists this me as the author of this patch, but
> > there is no "From:" line at the top of this email. I don't particularly
> > care one way or the other for this patch, but I wanted to point it out
> > as a potential issue with your patch-sending workflow.
>
> Yep, sorry. I would not touch send-email with lead-protected gloves, so
> what I do is to edit all patches I send. And in this case, I missed the
> fact that there was another "From:". I am sorry.
Heh. I certainly can't blame you for that; I don't use send-email
myself.
It might be convenient for format-patch to have a mode where it uses the
committer as the rfc822 "From:" and then adds a "From:" for the author
in the body if it is not the same as the committer.
It certainly shouldn't be the default, since that would confuse things
like rebase. But it makes sense if you are just going to throw away the
From header anyway when you import into your MUA.
-Peff
^ permalink raw reply
* Re: bug: transform a binary file into a symlink in one commit => invalid binary patch
From: Jeff King @ 2009-01-26 0:35 UTC (permalink / raw)
To: Pixel; +Cc: git
In-Reply-To: <lyhc3q9pl1.fsf@leia.mandriva.com>
On Fri, Jan 23, 2009 at 01:25:30PM +0100, Pixel wrote:
> i hit a bug (git 1.6.1): when you transform a binary file into a
> symlink in one commit, the binary patch can't be used in "git apply".
> Is it a known issue?
Not that I know of.
Below is a patch against the test suite that fairly neatly displays the
problem. I didn't get a chance to look into actually fixing it, though
(I'm not even sure the problem is in apply, and not in the generated
patch).
---
diff --git a/t/t4130-apply-symlink-binary.sh b/t/t4130-apply-symlink-binary.sh
new file mode 100755
index 0000000..0ee2ba1
--- /dev/null
+++ b/t/t4130-apply-symlink-binary.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='apply handles binary to symlink conversion'
+. ./test-lib.sh
+
+test_expect_success 'create commit with binary' '
+ echo content >file && git add file &&
+ printf "\0" > binary && git add binary &&
+ git commit -m one
+'
+
+test_expect_success 'convert binary to symlink' '
+ rm binary &&
+ ln -s file binary &&
+ git add binary &&
+ git commit -m two
+'
+
+test_expect_success 'create patch' '
+ git diff-tree --binary HEAD^ HEAD >patch
+'
+
+test_expect_success 'apply patch' '
+ git reset --hard HEAD^ &&
+ git apply patch &&
+ test -h binary &&
+ test_cmp binary file
+'
+
+test_done
^ permalink raw reply related
* Re: [PATCH 0/2] Add submodule-support to git archive
From: Junio C Hamano @ 2009-01-26 0:41 UTC (permalink / raw)
To: Lars Hjemli; +Cc: Nanako Shiraishi, Johannes Schindelin, git
In-Reply-To: <8c5c35580901251512q5058dde3rdfae81979c46c36a@mail.gmail.com>
Lars Hjemli <hjemli@gmail.com> writes:
> If you want me to build on top of the series in next anyways, would it
> be acceptable if the first patch on top of ee306d2d59 reverts the
> previous attempt? I think the rest of the series will be easier to
> review that way.
Ok, then I'll simply revert and then queue the new ones on top of it.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] Valgrind support: check for more than just programming errors
From: Johannes Schindelin @ 2009-01-26 0:43 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20090125234249.GE19099@coredump.intra.peff.net>
Hi,
On Sun, 25 Jan 2009, Jeff King wrote:
> On Mon, Jan 26, 2009 at 12:20:21AM +0100, Johannes Schindelin wrote:
>
> > While it is not strictly necessary to look through the whole PATH to
> > find git binaries to override, it is in line with running an expensive
> > test (which valgrind is) to make extra sure that no binary is tested
> > that actually comes from the git.git checkout.
>
> Should this be "...no binary is tested that _doesn't_ actually come from
> the git.git checkout"?
Yep, that was half the change to "that only binaries are tested...".
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH 1/2] rebase -i --root: simplify code
From: Johannes Schindelin @ 2009-01-26 0:44 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, gitster
In-Reply-To: <200901260049.25563.trast@student.ethz.ch>
Hi,
On Mon, 26 Jan 2009, Thomas Rast wrote:
> Johannes Schindelin wrote:
> >
> > When we rebase with --root, what we are actually picking are the commits
> > in $ONTO..$HEAD. So $ONTO is really our $UPSTREAM. Spell it out.
> [...]
> > + UPSTREAM=$ONTO
>
> While I think the simplification is reasonable, it breaks this check:
>
> get_saved_options () {
> # ...
> test ! -s "$DOTEST"/upstream && REBASE_ROOT=t
> }
>
> So you'll have to change that too.
Will fix tomorrow.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH 2/2] rebase -i --root: fix check for number of arguments
From: Johannes Schindelin @ 2009-01-26 0:49 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, gitster
In-Reply-To: <200901260108.07402.trast@student.ethz.ch>
Hi,
On Mon, 26 Jan 2009, Thomas Rast wrote:
> Johannes Schindelin wrote:
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > git-rebase--interactive.sh | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index 6e2bf25..5df35b2 100755
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -571,7 +571,8 @@ first and then run 'git rebase --continue' again."
> > ;;
> > --)
> > shift
> > - test ! -z "$REBASE_ROOT" -o $# -eq 1 -o $# -eq 2 || usage
> > + test -z "$REBASE_ROOT" -a $# -ge 1 -a $# -le 2 ||
> > + test ! -z "$REBASE_ROOT" -a $# -le 1 || usage
> > test -d "$DOTEST" &&
> > die "Interactive rebase already started"
>
> Acked-by: Thomas Rast <trast@student.ethz.ch>
>
> I'll postpone 1/2 till I've had enough sleep to check whether
> --continue ever needed to know about --root, and either remove or fix
> the remembering. (Sorry for the noise.)
It does need to remember, as pick_one wants to look at the parent to be
able to fast-forward.
But the sane fix is to add a file "$DOTEST"/root in case we're running
under --root.
Ciao,
Dscho
^ permalink raw reply
* [PATCH] git-svn: fix memory leak when checking for empty symlinks
From: Eric Wong @ 2009-01-26 1:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
By enforcing SVN::Pool usage when calling get_file once again.
This regression was introduced with the reintroduction of
SVN::Ra::get_file() usage in
dbc6c74d0858d77e61e092a48d467e725211f8e9
Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
git-svn.perl | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index d4cb538..5d39b39 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4021,7 +4021,8 @@ my ($ra_invalid, $can_do_switch, %ignored_err, $RA);
BEGIN {
# enforce temporary pool usage for some simple functions
no strict 'refs';
- for my $f (qw/rev_proplist get_latest_revnum get_uuid get_repos_root/) {
+ for my $f (qw/rev_proplist get_latest_revnum get_uuid get_repos_root
+ get_file/) {
my $SUPER = "SUPER::$f";
*$f = sub {
my $self = shift;
--
Eric Wong
^ permalink raw reply related
* Re: [PATCH] git-svn: add --ignore-paths option for fetching
From: Eric Wong @ 2009-01-26 1:18 UTC (permalink / raw)
To: public_vi; +Cc: Thomas Rast, git
In-Reply-To: <497CEC56.3020900@tut.by>
public_vi <public_vi@tut.by> wrote:
> Eric Wong wrote:
>> Thomas Rast <trast@student.ethz.ch> wrote:
>>
>>
>> Also, indentation is always done with tabs in git-svn (and the vast
>> majority of git as well).
>>
>>
> Whitespace is invisible for me now, I only checked according to
> Documentation/SubmittingPatches about extra lines adds or removes caused
> just by whitespace difference.
>>
>>
>> Vitaly: thank you for the patch. Can you also provide a testcase to
>> ensure this functionality doesn't break during refactorings? Thanks.
>>
>>
> Already done it with testcase. There are two new packs of patches sent
> to git@vger.kernel.org, the first is outdated too, the second is current.
> (I don't yet completely know how things should be done).
Thanks Vitaly, acked and pushed out with minor fixes to
git://git.bogomips.org/git-svn.git
git-svn: Add test for --ignore-paths parameter
[ew: replaced 'echo -e' with printf so it works on POSIX shells]
[ew: added Vitaly to copyright even though it's based on my test]
git-svn: documented --ignore-paths
[ew: trailing whitespace removed]
git-svn: add --ignore-paths option for fetching
[ew: replaced 4-space indent with tabs]
[ew: prefixed $ignore_regex with an underscore to be consistent
with other globals in git-svn]
[ew: rearranged functions to minimize diff and removed prototype
usage to be consistent with the rest of git-svn (and other
Perl code in git (and they're ugly to me)]
--
Eric Wong
^ permalink raw reply
* [ANNOUNCE] GIT 1.6.1.1
From: Junio C Hamano @ 2009-01-26 1:19 UTC (permalink / raw)
To: git; +Cc: linux-kernel
The latest maintenance release GIT 1.6.1.1 is available at the
usual places:
http://www.kernel.org/pub/software/scm/git/
git-1.6.1.1.tar.{gz,bz2} (source tarball)
git-htmldocs-1.6.1.1.tar.{gz,bz2} (preformatted docs)
git-manpages-1.6.1.1.tar.{gz,bz2} (preformatted docs)
The RPM binary packages for a few architectures are also provided
as courtesy.
RPMS/$arch/git-*-1.6.1.1-1.fc9.$arch.rpm (RPM)
----------------------------------------------------------------
GIT v1.6.1.1 Release Notes
==========================
Fixes since v1.6.1
------------------
* "git add frotz/nitfol" when "frotz" is a submodule should have errored
out, but it didn't.
* "git apply" took file modes from the patch text and updated the mode
bits of the target tree even when the patch was not about mode changes.
* "git bisect view" on Cygwin did not launch gitk
* "git checkout $tree" did not trigger an error.
* "git commit" tried to remove COMMIT_EDITMSG from the work tree by mistake.
* "git describe --all" complained when a commit is described with a tag,
which was nonsense.
* "git diff --no-index --" did not trigger no-index (aka "use git-diff as
a replacement of diff on untracked files") behaviour.
* "git format-patch -1 HEAD" on a root commit failed to produce patch
text.
* "git fsck branch" did not work as advertised; instead it behaved the same
way as "git fsck".
* "git log --pretty=format:%s" did not handle a multi-line subject the
same way as built-in log listers (i.e. shortlog, --pretty=oneline, etc.)
* "git daemon", and "git merge-file" are more careful when freopen fails
and barf, instead of going on and writing to unopened filehandle.
* "git http-push" did not like some RFC 4918 compliant DAV server
responses.
* "git merge -s recursive" mistakenly overwritten an untracked file in the
work tree upon delete/modify conflict.
* "git merge -s recursive" didn't leave the index unmerged for entries with
rename/delete conflictd.
* "git merge -s recursive" clobbered untracked files in the work tree.
* "git mv -k" with more than one errorneous paths misbehaved.
* "git read-tree -m -u" hence branch switching incorrectly lost a
subdirectory in rare cases.
* "git rebase -i" issued an unnecessary error message upon a user error of
marking the first commit to be "squash"ed.
* "git shortlog" did not format a commit message with multi-line
subject correctly.
Comes with many documentation updates.
^ permalink raw reply
* [PATCH] gitweb: ensure the default stylesheet is accessible
From: Giuseppe Bilotta @ 2009-01-26 1:28 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Junio C Hamano, Giuseppe Bilotta
On some installations the CSS fails to be linked correctly when
path_info is enabled, since the link refers to "gitweb.css", whereas it
should be "${my_uri}/gitweb.css". Fix by setting the appropriate default
in the Makefile.
Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
Makefile | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Makefile b/Makefile
index 4ef8234..4f60de9 100644
--- a/Makefile
+++ b/Makefile
@@ -215,7 +215,7 @@ GITWEB_STRICT_EXPORT =
GITWEB_BASE_URL =
GITWEB_LIST =
GITWEB_HOMETEXT = indextext.html
-GITWEB_CSS = gitweb.css
+GITWEB_CSS = $${my_uri}/gitweb.css
GITWEB_LOGO = git-logo.png
GITWEB_FAVICON = git-favicon.png
GITWEB_SITE_HEADER =
--
1.5.6.5
^ permalink raw reply related
* Re: [PATCH] gitweb: ensure the default stylesheet is accessible
From: Jakub Narebski @ 2009-01-26 1:48 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Junio C Hamano
In-Reply-To: <1232933322-9186-1-git-send-email-giuseppe.bilotta@gmail.com>
On Mon, 26 Jan 2009, Giuseppe Bilotta wrote:
> On some installations the CSS fails to be linked correctly when
> path_info is enabled, since the link refers to "gitweb.css", whereas it
> should be "${my_uri}/gitweb.css". Fix by setting the appropriate default
> in the Makefile.
Why "on some installations"? What does "some" mean? I don't think it
is something indeterministic: please spell when one can have problems
with linking CSS file.
Wouldn't it be simpler to deal with problem of base URL when using
path_info gitweb URLs to add BASE element to HTML head if we use
path_info? Something like:
if ($ENV{'PATH_INFO'}) { # $path_info is unfortunately stripped
print qq(<base href="$my_uri">\n);
}
somewhere in git_header_html() subroutine?
See also comment below.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
> Makefile | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 4ef8234..4f60de9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -215,7 +215,7 @@ GITWEB_STRICT_EXPORT =
> GITWEB_BASE_URL =
> GITWEB_LIST =
> GITWEB_HOMETEXT = indextext.html
> -GITWEB_CSS = gitweb.css
> +GITWEB_CSS = $${my_uri}/gitweb.css
> GITWEB_LOGO = git-logo.png
> GITWEB_FAVICON = git-favicon.png
It is not the same case for git-logo.png and git-favicon.png as for
gitweb.css? If it is not, please explain why in commit message.
If it is, then your patch is only partial solution to path_info
problem.
> GITWEB_SITE_HEADER =
> --
> 1.5.6.5
>
>
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] http-push: refactor request url creation
From: Ray Chuan @ 2009-01-26 1:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin
In-Reply-To: <7vpribdszr.fsf@gitster.siamese.dyndns.org>
Hi,
On Mon, Jan 26, 2009 at 4:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> @@ -304,17 +312,7 @@ static void start_fetch_loose(struct
>> transfer_request *request)
>>
>> git_SHA1_Init(&request->c);
>>
>> - url = xmalloc(strlen(remote->url) + 50);
>> ...
>> - strcpy(request->url, url);
>> + request->url = get_remote_object_url(remote->url, hex, 0);
>> ...
>> - curl_easy_setopt(slot->curl, CURLOPT_URL, url);
>> + curl_easy_setopt(slot->curl, CURLOPT_URL, request->url);
>
> The original code gave a separate "url" to setop() but this gives the same
> string. Does curl_easy_setop() copies the given string away? IOW is this
> change safe?
>
curl strdup's it, so this is safe.
The stack is like this:
curl_easy_setopt
calls Curl_setopt (at 391)
calls setstropt (at 1566)
calls strdup (at 276).
http://cool.haxx.se/cvs.cgi/curl/lib/easy.c?annotate=1.132 (for
curl_easy_setopt)
http://cool.haxx.se/cvs.cgi/curl/lib/url.c?annotate=1.782 (for
Curl_setopt, setstropt)
--
Cheers,
Ray Chuan
^ permalink raw reply
* Re: [PATCH] gitweb: last-modified time should be commiter, not author
From: Jakub Narebski @ 2009-01-26 1:54 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git
In-Reply-To: <1232923370-4427-1-git-send-email-giuseppe.bilotta@gmail.com>
On Sun, 25 Jan 2009, Giuseppe Bilotta wrote:
> Subject: [PATCH] gitweb: last-modified time should be commiter, not author
Should be really either "[PATCH 1/2]" or "[PATCH 5/4]" or "[PATCH 5/6]"
just in case for the next patch, because next patch _depends_ on this
one, and just in case of threading problem it should be marked as it;
it also makes easier to apply patches from emails saved as individual
files each.
>
> The last-modified time header added by RSS to increase cache hits from
> readers should be set to the date the repository was last modified. The
> author time in this respect is not a good guess because the last commit
> might come from a oldish patch.
>
> Use the committer time for the last-modified header to ensure a more
> correct guess of the last time the repository was modified.
Good catch, good thinking IMHO. Committer date has much better chance
to be monotonic than author date, and is more close related to
_publishing_ date (author date is more of _creation_ date).
Lack signoff; if Junio forges it (or you reply that it should be
signed off), you can add from me
Acked-by: Jakub Narebski <jnareb@gmail.com>
P.S. I wonder what other web interfaces do, for example cgit. I guess
that web interfaces for other SCMs like SVN::Web, ViewVC etc. do not
have this problem because they have only one, single date.
> ---
> gitweb/gitweb.perl | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 756868a..8c49c75 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -6015,7 +6015,7 @@ sub git_feed {
> }
> if (defined($commitlist[0])) {
> %latest_commit = %{$commitlist[0]};
> - %latest_date = parse_date($latest_commit{'author_epoch'});
> + %latest_date = parse_date($latest_commit{'committer_epoch'});
> print $cgi->header(
> -type => $content_type,
> -charset => 'utf-8',
> --
> 1.5.6.5
>
>
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: diff settings
From: Ted Pavlic @ 2009-01-26 2:06 UTC (permalink / raw)
To: Keith Cascio; +Cc: Teemu Likonen, git, Junio C Hamano, Johannes Schindelin
In-Reply-To: <alpine.GSO.2.00.0901251307030.12651@kiwi.cs.ucla.edu>
>> Mercurial has a "defaults.*" that does exactly what you want. That is, you can
>> [defaults]
>> diff = -w
>> and "hg diff" will automatically do "hg diff -w". Such a feature might be a
>> nice addition to git.
> Thank you, that is very interesting. I just submitted a patch this morning that
> does exactly what you describe, but I called it "primer" instead of "defaults"
> because it seemed more explicit. Check it out.
I saw that. However, in Mercurial's case, the [defaults] section applies
to all commands (including new ones introduced by extensions). That is,
it looks like your patch adds a diff.primer, but Mercurial would add a
primer.* that allows you to have a primer.diff, primer.commit, etc.
For example, wouldn't it be nice to have something like...
[defaults]
diff = -w
commit = -a
etc.? Right now the only way you can do that is with aliases (e.g., I
have an alias from "ci" to "commit -a").
This task is pretty easy in Mercurial because Mercurial porcelains are
implemented as modules that are all executed through the central "hg"
command. This isn't the case with git.
(on a somewhat unrelated note, Mercurial does have a [diff] section that
allows for, say, forcing the use of git diff and other things)
--Ted
--
Ted Pavlic <ted@tedpavlic.com>
Please visit my ALS association page:
http://web.alsa.org/goto/tedpavlic
My family appreciates your support in the fight to defeat ALS.
^ permalink raw reply
* Re: [PATCH] gitweb: check if-modified-since for feeds
From: Jakub Narebski @ 2009-01-26 2:18 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git
In-Reply-To: <1232923370-4427-2-git-send-email-giuseppe.bilotta@gmail.com>
Should be "[PATCH 2/2]" or similar, just in case.
On Sun, 25 Jun 2009, Giuseppe Bilotta wrote:
> Offering Last-modified header
And skipping generating the body if client uses 'HEAD' request to
get only Last-Modified header.
> for feeds is only half the work: we should
> also check that same date against If-modified-since, and bail out early
> with 304 Not Modified.
Lacks signoff.
> ---
> gitweb/gitweb.perl | 20 +++++++++++++++++++-
> 1 files changed, 19 insertions(+), 1 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 8c49c75..0a5d229 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -6015,7 +6015,25 @@ sub git_feed {
> }
> if (defined($commitlist[0])) {
> %latest_commit = %{$commitlist[0]};
> - %latest_date = parse_date($latest_commit{'committer_epoch'});
> + my $latest_epoch = $latest_commit{'committer_epoch'};
> + %latest_date = parse_date($latest_epoch);
> + my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
> + if (defined $if_modified) {
> + my $since;
> + if (eval { require HTTP::Date; 1; }) {
> + $since = HTTP::Date::str2time($if_modified);
> + } elsif (eval { require Time::ParseDate; 1; }) {
> + $since = Time::ParseDate::parsedate($if_modified, GMT => 1);
> + }
I'd really like to fallback on hand-parsing, as we have to parse date
in well defined HTTP-date format (RFC-1123, update to RFC-822), which
I think is what we send in Last-Modified header (or is it RFC-2822?).
But that might be too much work. I like the checking for modules,
and the fallback cascade, but could you explain why in this order?
> + if (defined $since && $latest_epoch <= $since) {
> + print $cgi->header(
> + -type => $content_type,
> + -charset => 'utf-8',
> + -last_modified => $latest_date{'rfc2822'},
> + -status => 304);
I think we spell HTTP status messages in full (even if it is hidden
in die_error subroutine), i.e.
+ -status => '304 Not Modified');
> + return;
> + }
> + }
> print $cgi->header(
> -type => $content_type,
> -charset => 'utf-8',
> --
> 1.5.6.5
>
>
P.S. It would be nice to have this mechanism (responding to
cache-control headers such as If-Modified-Since) for all of gitweb,
but I guess it is most critical for feeds, which are _polled_.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH v1 1/3] Introduce config variable "diff.primer"
From: Junio C Hamano @ 2009-01-26 2:30 UTC (permalink / raw)
To: Keith Cascio; +Cc: Jeff King, Johannes Schindelin, git
In-Reply-To: <7v1vurf7lq.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Scriptability by definition means you do not know how scripts written by
> people around plumbing use the output; I do not think you can sensibly say
> "this should not be turned on in a machine friendly output, but this is
> safe to use".
>
> I would not be opposed to an enhancement to the plumbing that the scripts
> can use to say "I am willing to take any option (or perhaps "these
> options") given to me via diff.primer". Some scripts may want to be just
> a pass-thru of whatever the underlying git-diff-* command outputs, and it
> may be a handy way to magically upgrade them to allow their invocation of
> lowlevel plumbing to be affected by what the end-user configured. But
> that magic upgrade has to be an opt/in process.
I suspect it is pretty much orthogonal to the "use user's default without
being told from the command line", but it might be a worthy goal to
introduce a mechanism for the scripts to accept "safe" default options
from the end user while rejecting undesirable ones that would interfere
with the way it uses plumbing.
For example, gitk drives "git rev-list" and many options you give from the
command line (e.g. "gitk --all --simplify-merges -- drivers/") are passed
to the underlying plumbing.
This is a double edged sword. When we add new features to git-rev-list,
(e.g. --simplify-merges or --simplify-by-decoration are fairly recent
inventions that did not exist when gitk was written originally), some of
them can be safely passed and automagically translates to a new feature in
gitk. However, use of some options (e.g. --reverse) breaks the assumption
the tool makes on the output from the underlying plumbing and should not
be accepted from the end-user.
It would be a good addition to our toolset if scripts like gitk can
declare which options and features are safe to accept from the end user to
pass down to the plumbing tools. "git rev-parse", which lets the script
sift between options that are meant to affect ancestry traversal and the
ones that are for other (primarily diff family) commands, does not do
anything fancy like that, but it would be a logical place to do this sort
of thing.
And it is not limited to "scripts" use. A recent topic on rejecting
colouring options from being given to format-patch would also be helped
with such a mechanism if it is available to builtins.
Just an idle thought.
^ permalink raw reply
* Re: [PATCH v1 1/3] Introduce config variable "diff.primer"
From: Keith Cascio @ 2009-01-26 2:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git
In-Reply-To: <7v1vuqdcjp.fsf@gitster.siamese.dyndns.org>
On Sun, 25 Jan 2009, Junio C Hamano wrote:
> I suspect it is pretty much orthogonal to the "use user's default without
> being told from the command line", but it might be a worthy goal to introduce
> a mechanism for the scripts to accept "safe" default options from the end user
> while rejecting undesirable ones that would interfere with the way it uses
> plumbing.
>
> For example, gitk drives "git rev-list" and many options you give from the
> command line (e.g. "gitk --all --simplify-merges -- drivers/") are passed to
> the underlying plumbing.
>
> This is a double edged sword. When we add new features to git-rev-list, (e.g.
> --simplify-merges or --simplify-by-decoration are fairly recent inventions
> that did not exist when gitk was written originally), some of them can be
> safely passed and automagically translates to a new feature in gitk.
> However, use of some options (e.g. --reverse) breaks the assumption the tool
> makes on the output from the underlying plumbing and should not be accepted
> from the end-user.
>
> It would be a good addition to our toolset if scripts like gitk can declare
> which options and features are safe to accept from the end user to pass down
> to the plumbing tools. "git rev-parse", which lets the script sift between
> options that are meant to affect ancestry traversal and the ones that are for
> other (primarily diff family) commands, does not do anything fancy like that,
> but it would be a logical place to do this sort of thing.
>
> And it is not limited to "scripts" use. A recent topic on rejecting colouring
> options from being given to format-patch would also be helped with such a
> mechanism if it is available to builtins.
>
> Just an idle thought.
Yes yes yes yes!!!!! I've been working on a response to your previous message,
in which I address exactly this possibility. Coming soon.
^ permalink raw reply
* Re: [PATCH v1 1/3] Introduce config variable "diff.primer"
From: Keith Cascio @ 2009-01-26 2:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1vurf7lq.fsf@gitster.siamese.dyndns.org>
On Sun, 25 Jan 2009, Junio C Hamano wrote:
> Your Subject is good; in a shortlog output that will be taken 3 months
> down the road, it will still tell us what this patch was about, among 100
> other patches that are about different topics.
Thanks, I value the encouragement.
> The proposed commit log message describes what the patch does, but it does
> not explain what problem it solves, nor why the approach the patch takes
> to solve that problem is good. Lines that are too short and too dense
> without paragraph breaks do not help readability either.
Noted, future versions will be better.
> You can work around the backward incompatibility you are introducing for known
> users that you broke with your patch, by including updates to them, and that
> is what your patches to git-gui and gitk are, but that is a sure sign that the
> approach is flawed.
>
> The point of lowlevel plumbing (e.g. diff-{files,index,tree}) is to give
> people's scripts an interface that they can rely on. It is not about giving a
> magic interface that all the users are somehow magically upgraded without
> change, when the underlying git is upgraded.
I believe, as I know you do as well, concept is the right way to approach code.
I believe in the power of concept, and I agree that porcelain/plumbing is a good
and powerful concept, and should not be violated.
> If a script X does not use "ignore whitespace" without an explicit request
> from the end user when it runs diff-tree internally, installing a new
> version of diff-tree should *NOT* magically make script X to run it with
> "ignore whitespace", because you do not know what the script X uses
> diff-tree output for and how, even when the end user sets diff.primer to
> get "ignore whitespace" applied to his command line invocation of "git
> diff" Porcelain. Imagine a case where the operation of that script X
> relies on seeing at least two context lines around the hunk in order to
> correctly parse textual diff output from "diff-index -p", and the user
> sets "-U1" in diff.primer --- you would break the script and it is not
> fair to blame the script for not explicitly passing -U3 and relying on the
> default.
>
> Scriptability by definition means you do not know how scripts written by
> people around plumbing use the output; I do not think you can sensibly say
> "this should not be turned on in a machine friendly output, but this is
> safe to use".
>
> I would not be opposed to an enhancement to the plumbing that the scripts
> can use to say "I am willing to take any option (or perhaps "these
> options") given to me via diff.primer". Some scripts may want to be just
> a pass-thru of whatever the underlying git-diff-* command outputs, and it
> may be a handy way to magically upgrade them to allow their invocation of
> lowlevel plumbing to be affected by what the end-user configured. But
> that magic upgrade has to be an opt/in process.
I agree opt-in is always better with new grammar/semantics. However, the
constraint I was trying to live inside is: if I call "git diff" on the command
line with no options at all, then primer active. Yet perhaps that's not
possible, and the only way to do primer is to require opt-in spelled "--primer".
Then I can tell bash to alias 'gitdiff' as 'git diff --primer' and use that on
the command line. And I could patch git-gui and gitk to call 'git diff --primer
--no-color'. Note that would still require the same magnitude of change to the
git-gui and gitk Tcl code as occurs with my v1 patch.
It would be really cool if there was a way
to make this work without "--primer" !!!
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Worth considering: I believe Git users who've written their own scripts are not
the type to use diff.primer blindly, if at all. If they did use it, they'd be
very thoughtful about its consequences for their existing scripts. Based on
that belief, and the fact that I already protected every script in Git core, and
that I could work with the authors of contrib to do the same there, I have a
very high degree of confidence that no existing script would be broken the day
you release diff.primer, if ever :(
Because an enhancement *could* break existing scripts is not sufficient grounds
to declare it worthless. My gut tells me there is a sane way to do this, but I
need more familiarity with the "literature" to discover it.
I see evidence there is already anxiety surrounding the necessity of
"machine-friendliness", Cf.
http://article.gmane.org/gmane.comp.version-control.git/107006
I think my DIFF_MACHINE_FRIENDLY() cpp macro with --machine-friendly command
line option does a nice job of making that explicit, so we could all just relax!
When a monster is lurking under the bridge threatening the kingdom, put it to
death once and for all! Rather than relying on an elaborate system of
conventions prescribing how to tip-toe acrosss the bridge just the right way
while pretending the monster isn't there.
> There are funny indentation to align the same variable names on two
> adjacent lines and such; please don't.
You're right. I'm sorry. I'll fix all style to adhere to local convention.
-- Keith
^ permalink raw reply
* Re: [PATCH v1 1/3] Introduce config variable "diff.primer"
From: Jeff King @ 2009-01-26 3:12 UTC (permalink / raw)
To: Keith Cascio; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.GSO.2.00.0901251345240.12651@kiwi.cs.ucla.edu>
On Sun, Jan 25, 2009 at 06:40:02PM -0800, Keith Cascio wrote:
> I agree opt-in is always better with new grammar/semantics. However,
> the constraint I was trying to live inside is: if I call "git diff" on
> the command line with no options at all, then primer active. Yet
> perhaps that's not possible, and the only way to do primer is to
> require opt-in spelled "--primer". Then I can tell bash to alias
> 'gitdiff' as 'git diff --primer' and use that on the command line.
What's the point of aliasing something that isn't "git diff" to "git
diff --primer"? At that point, couldn't you just do away with --primer
entirely and alias "gitdiff" to "git diff --whatever --your --primer
--options --are"?
Anyway, I think that isn't necessary. We _do_ have a mechanism to handle
this already: some commands are plumbing, and must have stable
interfaces, and some commands are porcelain, and can do your magic
automatically. For example, gitk doesn't actually call "git diff"; it
calls "git diff-tree", "git diff-index", etc.
So if you just want this from the command line, then I think it is safe
to have "git diff" always respect "diff.primer", and scripts shouldn't
be impacted.
But this can break down in two ways:
1. Sometimes we blur the line of plumbing and porcelain, where
functionality is available only through plumbing. For example,
gitweb until recently called "git diff" because there is no other
way to diff two arbitrary blobs. But the solution there is, I
think, to make that functionality available through plumbing. Not
to disallow enhancements to porcelain.
2. When you want a script to take advantage of porcelain-like options,
the situation is much more difficult (and this is what Junio was
talking about in his last mail).
What I think is sane is:
a. You grow new feature X.
b. Porcelain takes advantage of any config that asks us to use X.
c. Plumbing does _not_ respect such config, but will respect
command line options.
d. Scripts control which command line options they use; when the
script writer decides feature X will not interfere (either
because it is harmless to the script's use, or because the
script is enhanced to handle the new behavior), then it can
pass an "--allow-X" command line option.
And of course that has two disadvantages (and I'm running out of
numbering schemes):
I. You have to wait for the script to be updated before you can
start using X, even if _you_ know that it's harmless.
II. Point (d) is not always true. Junio mentioned the fact that
gitk passes command line parameters blindly to rev-list, which
is potentially unsafe. Up until now, our attitude has been "if
it hurts, don't do it". In other words, if you call "gitk
--reverse" and it looks ugly, then it is your fault. :)
-Peff
^ permalink raw reply
* Re: [PATCH v1 1/3] Introduce config variable "diff.primer"
From: Jeff King @ 2009-01-26 3:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Keith Cascio, Johannes Schindelin, git
In-Reply-To: <7v1vuqdcjp.fsf@gitster.siamese.dyndns.org>
On Sun, Jan 25, 2009 at 06:30:18PM -0800, Junio C Hamano wrote:
> It would be a good addition to our toolset if scripts like gitk can
> declare which options and features are safe to accept from the end user to
> pass down to the plumbing tools. "git rev-parse", which lets the script
> sift between options that are meant to affect ancestry traversal and the
> ones that are for other (primarily diff family) commands, does not do
> anything fancy like that, but it would be a logical place to do this sort
> of thing.
I'm not sure there is a good way of doing this at a less fine-grained
level than "each option". That is, how can git-core, without knowing how
the script will use the output, classify options in groups according to
how the script will react to them?
It seems like "--since" is innocent enough for gitk. It just limits the
commits shown. So maybe it goes into the "ancestry traversal" list. But
is that whole list safe? "--reverse" isn't, but I would have put it in
the same list.
So I think what you will end up with is a list in gitk of "these
particular options are known good for passing through". And that doesn't
really need tool support from git-core. It's up to each script how much
it wants to protect the user.
But if you are proposing that some config options can be "enabled" by
scripts selectively, then I think that does need tool support. Keith's
"primer" example will be parsed by git, not by whatever script is
calling it. So we would need to feed it some list of "these are the OK
options".
-Peff
^ permalink raw reply
* Re: [PATCH v1 1/3] Introduce config variable "diff.primer"
From: Junio C Hamano @ 2009-01-26 3:36 UTC (permalink / raw)
To: Keith Cascio; +Cc: git
In-Reply-To: <alpine.GSO.2.00.0901251345240.12651@kiwi.cs.ucla.edu>
Keith Cascio <keith@CS.UCLA.EDU> writes:
> I agree opt-in is always better with new grammar/semantics. However, the
> constraint I was trying to live inside is: if I call "git diff" on the command
> line with no options at all
"git diff" is a Porcelain.
> Worth considering: I believe Git users who've written their own scripts are not
> the type to use diff.primer blindly, if at all.
Perhaps this nonsense comes from your misunderstanding on the line between
the plumbing and the Porcelain.
Users of git, including me, would love to be able to use default options
in our $HOME/.gitconfig file when using "git diff" interactively, but will
refuse to see our scripts that we wrote using "git diff-files" and "git
diff-index" broken, because the reason we explicitly used these plumbing
commands is to avoid getting broken with a change from the underlying
version of git. That's the whole point of output stability for the
plumbing.
If you want to be able to use -w or -b (or --color) in git-gui, you must
first vet the script to see if it can sanely operate on the output from
git-diff-index with such options, and after it is determined that it is
safe, it should give its users a way to pass that to the underlying
plumbing, or picked up such options from the configuration (perhaps using
the same diff.primer configuration).
This has to be a conscious opt-in process per script.
^ permalink raw reply
* Re: [PATCH v1 1/3] Introduce config variable "diff.primer"
From: Junio C Hamano @ 2009-01-26 3:38 UTC (permalink / raw)
To: Jeff King; +Cc: Keith Cascio, Johannes Schindelin, git
In-Reply-To: <20090126031820.GC14277@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So I think what you will end up with is a list in gitk of "these
> particular options are known good for passing through". And that doesn't
> really need tool support from git-core. It's up to each script how much
> it wants to protect the user.
I tend to agree. Also at the same time this does not have to contradict
with what Keith wants to do. gitk just needs to learn to peek into
diff.primer, and use the safe ones while discarding others. A tool
support is already there in the form of git-config to do this, though.
^ permalink raw reply
* Re: [PATCH v1 1/3] Introduce config variable "diff.primer"
From: Junio C Hamano @ 2009-01-26 3:42 UTC (permalink / raw)
To: Jeff King; +Cc: Keith Cascio, git
In-Reply-To: <20090126031206.GB14277@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
We seem to think the same way these days, so I do not have very much to
add on top of what you already said. I'll fix one typo, though.
> But this can break down in two ways:
>
> 1. Sometimes we blur the line of plumbing and porcelain, where
> functionality is available only through plumbing. For example,
s/through plumbing/through Porcelain/.
^ permalink raw reply
* Re: [PATCH v1 1/3] Introduce config variable "diff.primer"
From: Jeff King @ 2009-01-26 3:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Keith Cascio, git
In-Reply-To: <7v4ozmbunl.fsf@gitster.siamese.dyndns.org>
On Sun, Jan 25, 2009 at 07:42:06PM -0800, Junio C Hamano wrote:
> We seem to think the same way these days, so I do not have very much to
> add on top of what you already said. I'll fix one typo, though.
Heh. Am I corrupting you, or you me?
> > But this can break down in two ways:
> >
> > 1. Sometimes we blur the line of plumbing and porcelain, where
> > functionality is available only through plumbing. For example,
>
> s/through plumbing/through Porcelain/.
Oops, yes, thank you. That was what I meant.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] rebase -i --root: simplify code
From: Junio C Hamano @ 2009-01-26 5:54 UTC (permalink / raw)
To: Thomas Rast; +Cc: Johannes Schindelin, git
In-Reply-To: <200901260053.06315.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> Thomas Rast wrote:
>> test ! -s "$DOTEST"/upstream && REBASE_ROOT=t
>
> Actually, I think that test never worked (and it's clearly my fault).
>
> The corresponding 'echo $UPSTREAM > "$DOTEST"/upstream' just expanded
> to 'echo > ...', resulting in a file containing a single newline, but
> never a zero-length file. Duh.
Since you never use the value stored in "$DOTEST/upstream" for anything
else anyway, how about doing something like this instead? It would make
the meaning of the file used as a state variable much clearer.
It may break hooks and outside scripts that look at $DOTEST/upstream. I
didn't check.
The hunk in the middle is to protect you against an environment variable
UPSTREAM the user may have before starting "rebase -i". There could be
other state variables you added in recent commit d911d14 (rebase -i: learn
to rebase root commit, 2009-01-02) that needs similar protection. Please
check.
git-rebase--interactive.sh | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git c/git-rebase--interactive.sh w/git-rebase--interactive.sh
index 21ac20c..17cf0e5 100755
--- c/git-rebase--interactive.sh
+++ w/git-rebase--interactive.sh
@@ -456,7 +456,7 @@ get_saved_options () {
test -d "$REWRITTEN" && PRESERVE_MERGES=t
test -f "$DOTEST"/strategy && STRATEGY="$(cat "$DOTEST"/strategy)"
test -f "$DOTEST"/verbose && VERBOSE=t
- test ! -s "$DOTEST"/upstream && REBASE_ROOT=t
+ test -f "$DOTEST"/rebase-root && REBASE_ROOT=t
}
while test $# != 0
@@ -585,6 +585,7 @@ first and then run 'git rebase --continue' again."
test -z "$ONTO" && ONTO=$UPSTREAM
shift
else
+ UPSTREAM=
UPSTREAM_ARG=--root
test -z "$ONTO" &&
die "You must specify --onto when using --root"
@@ -611,7 +612,12 @@ first and then run 'git rebase --continue' again."
echo "detached HEAD" > "$DOTEST"/head-name
echo $HEAD > "$DOTEST"/head
- echo $UPSTREAM > "$DOTEST"/upstream
+ case "$REBASE_ROOT" in
+ '')
+ rm -f "$DOTEST"/rebase-root ;;
+ *)
+ : >"$DOTEST"/rebase-root ;;
+ esac
echo $ONTO > "$DOTEST"/onto
test -z "$STRATEGY" || echo "$STRATEGY" > "$DOTEST"/strategy
test t = "$VERBOSE" && : > "$DOTEST"/verbose
^ permalink raw reply related
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