* Re: [RFC/PATCH v6 2/2] gitweb: Smarter snapshot names
From: Mark Rada @ 2009-10-30 20:07 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Shawn O. Pearce
In-Reply-To: <1256854062-25496-3-git-send-email-jnareb@gmail.com>
Hmm, I guess I was taking too long? Fair enough.
I've had more to do this academic term than I thought.
Thank you for getting this done.
--
Mark Rada (ferrous26)
marada@uwaterloo.ca
On 09-10-29 6:07 PM, Jakub Narebski wrote:
> From: Mark Rada <marada@uwaterloo.ca>
>
> Teach gitweb how to produce nicer snapshot names by only using the
> short hash id. If clients make requests using a tree-ish that is not
> a partial or full SHA-1 hash, then the short hash will also be appended
> to whatever they asked for. If clients request snapshot of a tag
> (which means that $hash ('h') parameter has 'refs/tags/' prefix),
> use only tag name.
>
> This also includes tests cases for t9502-gitweb-standalone-parse-output.
>
> Signed-off-by: Mark Rada <marada@uwaterloo.ca>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> This is currently an RFC because commit message does not describe
> changes completely. Among others it doesn't describe current rules.
> Also tests could be better described.
>
> Changes since v5 (by Mark Rada):
> - put common parts of git_get_full_hash and git_get_short_hash
> into git_get_hash, do not do double verification
> - separate finding snapshot name and prefix in snapshot_name()
> subroutine, more smarts for snapshot name
> - improved tests (use 'tar' snapshot format, testing new smarts,
> testing prefix)
>
> gitweb/gitweb.perl | 76 +++++++++++++++----
> t/t9502-gitweb-standalone-parse-output.sh | 112 +++++++++++++++++++++++++++++
> 2 files changed, 172 insertions(+), 16 deletions(-)
> create mode 100755 t/t9502-gitweb-standalone-parse-output.sh
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 8d4a2ae..d8dfd95 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1983,16 +1983,27 @@ sub quote_command {
>
> # get HEAD ref of given project as hash
> sub git_get_head_hash {
> - my $project = shift;
> + return git_get_full_hash(shift, 'HEAD');
> +}
> +
> +sub git_get_full_hash {
> + return git_get_hash(@_);
> +}
> +
> +sub git_get_short_hash {
> + return git_get_hash(@_, '--short=7');
> +}
> +
> +sub git_get_hash {
> + my ($project, $hash, @options) = @_;
> my $o_git_dir = $git_dir;
> my $retval = undef;
> $git_dir = "$projectroot/$project";
> - if (open my $fd, "-|", git_cmd(), "rev-parse", "--verify", "HEAD") {
> - my $head = <$fd>;
> + if (open my $fd, '-|', git_cmd(), 'rev-parse',
> + '--verify', '-q', @options, $hash) {
> + $retval = <$fd>;
> + chomp $retval if defined $retval;
> close $fd;
> - if (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {
> - $retval = $1;
> - }
> }
> if (defined $o_git_dir) {
> $git_dir = $o_git_dir;
> @@ -5179,6 +5190,43 @@ sub git_tree {
> git_footer_html();
> }
>
> +sub snapshot_name {
> + my ($project, $hash) = @_;
> +
> + # path/to/project.git -> project
> + # path/to/project/.git -> project
> + my $name = to_utf8($project);
> + $name =~ s,([^/])/*\.git$,$1,;
> + $name = basename($name);
> + # sanitize name
> + $name =~ s/[[:cntrl:]]/?/g;
> +
> + my $ver = $hash;
> + if ($hash =~ /^[0-9a-fA-F]+$/) {
> + # shorten SHA-1 hash
> + my $full_hash = git_get_full_hash($project, $hash);
> + if ($full_hash =~ /^$hash/ && length($hash) > 7) {
> + $ver = git_get_short_hash($project, $hash);
> + }
> + } elsif ($hash =~ m!^refs/tags/(.*)$!) {
> + # tags don't need shortened SHA-1 hash
> + $ver = $1;
> + } else {
> + # branches and other need shortened SHA-1 hash
> + if ($hash =~ m!^refs/(?:heads|remotes)/(.*)$!) {
> + $ver = $1;
> + }
> + $ver .= '-' . git_get_short_hash($project, $hash);
> + }
> + # in case of hierarchical branch names
> + $ver =~ s!/!.!g;
> +
> + # name = project-version_string
> + $name = "$name-$ver";
> +
> + return wantarray ? ($name, $name) : $name;
> +}
> +
> sub git_snapshot {
> my $format = $input_params{'snapshot_format'};
> if (!@snapshot_fmts) {
> @@ -5203,24 +5251,20 @@ sub git_snapshot {
> die_error(400, 'Object is not a tree-ish');
> }
>
> - my $name = $project;
> - $name =~ s,([^/])/*\.git$,$1,;
> - $name = basename($name);
> - my $filename = to_utf8($name);
> - $name =~ s/\047/\047\\\047\047/g;
> - my $cmd;
> - $filename .= "-$hash$known_snapshot_formats{$format}{'suffix'}";
> - $cmd = quote_command(
> + my ($name, $prefix) = snapshot_name($project, $hash);
> + my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
> + my $cmd = quote_command(
> git_cmd(), 'archive',
> "--format=$known_snapshot_formats{$format}{'format'}",
> - "--prefix=$name/", $hash);
> + "--prefix=$prefix/", $hash);
> if (exists $known_snapshot_formats{$format}{'compressor'}) {
> $cmd .= ' | ' . quote_command(@{$known_snapshot_formats{$format}{'compressor'}});
> }
>
> + $filename =~ s/(["\\])/\\$1/g;
> print $cgi->header(
> -type => $known_snapshot_formats{$format}{'type'},
> - -content_disposition => 'inline; filename="' . "$filename" . '"',
> + -content_disposition => 'inline; filename="' . $filename . '"',
> -status => '200 OK');
>
> open my $fd, "-|", $cmd
> diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh
> new file mode 100755
> index 0000000..fbf4e26
> --- /dev/null
> +++ b/t/t9502-gitweb-standalone-parse-output.sh
> @@ -0,0 +1,112 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2009 Mark Rada
> +#
> +
> +test_description='gitweb as standalone script (parsing script output).
> +
> +This test runs gitweb (git web interface) as a CGI script from the
> +commandline, and checks that it produces the correct output, either
> +in the HTTP header or the actual script output.'
> +
> +
> +. ./gitweb-lib.sh
> +
> +# ----------------------------------------------------------------------
> +# snapshot file name
> +
> +cat >>gitweb_config.perl <<\EOF
> +
> +$known_snapshot_formats{'tar'} = {
> + 'display' => 'tar',
> + 'type' => 'application/x-tar',
> + 'suffix' => '.tar',
> + 'format' => 'tar',
> +};
> +
> +$feature{'snapshot'}{'default'} = ['tar'];
> +EOF
> +
> +test_expect_success setup '
> + test_commit first foo &&
> + git branch xx/test &&
> + FULL_ID=$(git rev-parse --verify HEAD) &&
> + SHORT_ID=$(git rev-parse --verify --short=7 HEAD)
> +'
> +test_debug '
> + echo "FULL_ID = $FULL_ID"
> + echo "SHORT_ID = $SHORT_ID"
> +'
> +
> +test_expect_success 'snapshots: give full hash' '
> + gitweb_run "p=.git;a=snapshot;h=$FULL_ID;sf=tar" &&
> + grep ".git-$SHORT_ID.tar" gitweb.headers
> +'
> +test_debug 'cat gitweb.headers'
> +test_debug 'cat gitweb.log'
> +
> +test_expect_success 'snapshots: give short hash' '
> + gitweb_run "p=.git;a=snapshot;h=$SHORT_ID;sf=tar" &&
> + grep ".git-$SHORT_ID.tar" gitweb.headers
> +'
> +test_debug 'cat gitweb.headers'
> +
> +test_expect_success 'snapshots: give almost full hash' '
> + ID=$(git rev-parse --short=30 HEAD) &&
> + gitweb_run "p=.git;a=snapshot;h=$ID;sf=tar" &&
> + grep ".git-$SHORT_ID.tar" gitweb.headers
> +'
> +test_debug 'cat gitweb.headers'
> +
> +test_expect_success 'snapshots: give HEAD' '
> + gitweb_run "p=.git;a=snapshot;h=HEAD;sf=tar" &&
> + grep ".git-HEAD-$SHORT_ID.tar" gitweb.headers
> +'
> +test_debug 'cat gitweb.headers'
> +
> +test_expect_success 'snapshots: give short branch name' '
> + gitweb_run "p=.git;a=snapshot;h=master;sf=tar" &&
> + ID=$(git rev-parse --verify --short=7 master) &&
> + grep ".git-master-$ID.tar" gitweb.headers
> +'
> +test_debug 'cat gitweb.headers'
> +
> +test_expect_success 'snapshots: give short tag name' '
> + gitweb_run "p=.git;a=snapshot;h=first;sf=tar" &&
> + ID=$(git rev-parse --verify --short=7 first) &&
> + grep ".git-first-$ID.tar" gitweb.headers
> +'
> +test_debug 'cat gitweb.headers'
> +
> +test_expect_success 'snapshots: give full branch name' '
> + gitweb_run "p=.git;a=snapshot;h=refs/heads/master;sf=tar" &&
> + ID=$(git rev-parse --verify --short=7 master) &&
> + grep ".git-master-$ID.tar" gitweb.headers
> +'
> +test_debug 'cat gitweb.headers'
> +
> +test_expect_success 'snapshots: give full tag name' '
> + gitweb_run "p=.git;a=snapshot;h=refs/tags/first;sf=tar" &&
> + grep ".git-first.tar" gitweb.headers
> +'
> +test_debug 'cat gitweb.headers'
> +
> +test_expect_success 'snapshots: give hierarchical branch name' '
> + gitweb_run "p=.git;a=snapshot;h=xx/test;sf=tar" &&
> + grep -v "filename=.*/" gitweb.headers
> +'
> +test_debug 'cat gitweb.headers'
> +
> +cat >expected <<EOF
> +.git-HEAD-$SHORT_ID/
> +.git-HEAD-$SHORT_ID/foo
> +EOF
> +test_expect_success 'snapshots: check prefix' '
> + gitweb_run "p=.git;a=snapshot;h=HEAD;sf=tar" &&
> + grep ".git-HEAD-$SHORT_ID.tar" gitweb.headers &&
> + "$TAR" tf gitweb.body >actual &&
> + test_cmp expected actual
> +'
> +test_debug 'cat gitweb.headers'
> +
> +test_done
^ permalink raw reply
* Re: Bug#553296: gitignore broken completely
From: Klaus Ethgen @ 2009-10-30 20:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, 553296, git
In-Reply-To: <7vvdhwk6dq.fsf@alter.siamese.dyndns.org>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Hi Junio,
Am Fr den 30. Okt 2009 um 20:51 schrieb Junio C Hamano:
> I am not sure; my head spins when I see "tracked but ignored" (you have
> a new one in the test) which is quite a bogus concept.
>
> Does it mean "tracked but would be ignored if it weren't---perhaps it was
> added by mistake?"?
Right. Exact. Sorry if my arguing was a bit misunderstandable. (My
spellchecker do not know "misunderstandable", so I hope it is ok; I am
from Germany, we like long words. ;-)
Regards
Klaus
- --
Klaus Ethgen http://www.ethgen.de/
pub 2048R/D1A4EDE5 2000-02-26 Klaus Ethgen <Klaus@Ethgen.de>
Fingerprint: D7 67 71 C4 99 A6 D4 FE EA 40 30 57 3C 88 26 2B
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
iQEVAwUBSutG9J+OKpjRpO3lAQqBCQf+MWXaVdCeD7bT/dTCEQ8WfVKwISR6qNCP
wttV5oHnMp/f1mDWqhwYc4OUeOcLtkcV6pNf4oCMLY5v6zuvnLDgGB/8XW3l6JgZ
Y7m0UoSRu0s5Hux+VX/+CLwJ1cylstkwx3yOxcZ5VSXAgDlRIrt7LpwCsh/7lMSO
n5KUHw3/RodTwomxXUSRTQqU+SQMOUJrUvYR1EE9zR0YQkDKYqhulhUH+uirLaBQ
0rfJB7sTRqFfv2kHMLzO8t+jfco39cOZWTxlE1QBN1+k7aTZIJD2d79atfY7NLo0
2kCfcJXRurQ2rSpjUAbDVBo8JvkclnEnVLZP+s5kuwzej0hy4D2ocg==
=sN6Y
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: Bug#553296: gitignore broken completely
From: Jeff King @ 2009-10-30 20:05 UTC (permalink / raw)
To: Klaus Ethgen; +Cc: 553296, Junio C Hamano, git
In-Reply-To: <20091030200148.GD10671@ikki.ethgen.de>
On Fri, Oct 30, 2009 at 09:01:48PM +0100, Klaus Ethgen wrote:
> > +test_expect_success 'ls-files -i lists only tracked-but-ignored files' '
> > + echo content >other-file &&
> > + git add other-file &&
> > + echo file >expect &&
> > + git ls-files -i --exclude-standard >output &&
> > + test_cmp expect output
> > +'
> > +
> > test_done
>
> Do that fit? shouldn't it be "test_cmp other-file output"? "git ls-files
> -i --exclude-standard" should show the files in the index that are also
> in the exclude list. And you only add other-file to the index. And
> shouldn't there be also a "echo other-file > .gitignore"?
No, because it is building on the previous tests. The point of the test
is:
- we already have 'file' tracked and in gitignore (from previous
tests in the series)
- we add other-file to have some other file which is tracked but not
in gitignore
- we check the output of "ls-files -i" to make sure that "file" is
there, but "other-file" is not
-Peff
^ permalink raw reply
* Re: Bug#553296: gitignore broken completely
From: Klaus Ethgen @ 2009-10-30 20:01 UTC (permalink / raw)
To: Jeff King; +Cc: 553296, Junio C Hamano, git
In-Reply-To: <20091030190552.GA3528@coredump.intra.peff.net>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Hi Jeff,
Am Fr den 30. Okt 2009 um 20:05 schrieb Jeff King:
> On Fri, Oct 30, 2009 at 02:41:55PM -0400, Jeff King wrote:
>
> > > 6. Revert the patch and rework it so that it will only have effect if
> > > there is no -i option on the command line. (That is similiar to a
> > > mix of 3 and 4.)
> >
> > Yeah, that would actually be the least invasive change, and would keep
> > "-i" just as it is. If we do anything except a simple, I think your (6)
> > makes the most sense.
Ok.
> > Let me see if I can make a patch.
Fine, thanks.
> - Show ignored files in the output.
> - Note that this also reverses any exclude list present.
> + Show only ignored files in the output. When showing files in the
> + index, print only those matched by an exclude pattern. When
> + showing "other" files, show only those matched by an exclude
> + pattern.
Hmmm... I do not see that much difference. But that might be justified
by my bad English.
> --- a/t/t3003-ls-files-exclude.sh
> +++ b/t/t3003-ls-files-exclude.sh
> @@ -29,4 +29,12 @@ test_expect_success 'add file to gitignore' '
> '
> check_all_output
>
> +test_expect_success 'ls-files -i lists only tracked-but-ignored files' '
> + echo content >other-file &&
> + git add other-file &&
> + echo file >expect &&
> + git ls-files -i --exclude-standard >output &&
> + test_cmp expect output
> +'
> +
> test_done
Do that fit? shouldn't it be "test_cmp other-file output"? "git ls-files
- -i --exclude-standard" should show the files in the index that are also
in the exclude list. And you only add other-file to the index. And
shouldn't there be also a "echo other-file > .gitignore"?
Regards
Klaus
- --
Klaus Ethgen http://www.ethgen.de/
pub 2048R/D1A4EDE5 2000-02-26 Klaus Ethgen <Klaus@Ethgen.de>
Fingerprint: D7 67 71 C4 99 A6 D4 FE EA 40 30 57 3C 88 26 2B
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
iQEVAwUBSutGLJ+OKpjRpO3lAQqmZwgAi7DqPorAdp0dZtgMpgzCsrWTJx5xb/dv
voQ4K5pYmxG1PgLb8zY7ywIvcG36uakCGvwgFQxsLzSgg0hRO1UsQ8XFVqSPp62X
evsZ/On5LYHY3xz+Fl0cPM5oomtqY+ZmHAd5syj4oh6hSHM8J93RuQzGEfMshtrQ
NUfAbAVmjuA5d4Cl/SLNUvoLE6M6O3YGkCGKXA9aPQcker6W+nODExJgTqyh4RHK
ATfbPC6+VJGUPfjVGmrqHVW8LOb/wP7grEBMaHvGpP/ysh4FHjy6HtXLhyPSRES/
KpsMG5g0dF0dbS0qBgrIck+6cifiHNlAbxc5LF7tydFBMnRAmjDyeA==
=7JTn
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: Bug#553296: gitignore broken completely
From: Jeff King @ 2009-10-30 19:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Klaus Ethgen, 553296, git
In-Reply-To: <7vvdhwk6dq.fsf@alter.siamese.dyndns.org>
On Fri, Oct 30, 2009 at 12:51:13PM -0700, Junio C Hamano wrote:
> > Here it is. I think this is the right thing to do. Junio?
>
> I am not sure; my head spins when I see "tracked but ignored" (you have
> a new one in the test) which is quite a bogus concept.
>
> Does it mean "tracked but would be ignored if it weren't---perhaps it was
> added by mistake?"?
Yes, that is exactly what it means.
-Peff
^ permalink raw reply
* Re: Bug#553296: gitignore broken completely
From: Junio C Hamano @ 2009-10-30 19:51 UTC (permalink / raw)
To: Jeff King; +Cc: Klaus Ethgen, 553296, Junio C Hamano, git
In-Reply-To: <20091030190552.GA3528@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Oct 30, 2009 at 02:41:55PM -0400, Jeff King wrote:
>
>> > 6. Revert the patch and rework it so that it will only have effect if
>> > there is no -i option on the command line. (That is similiar to a
>> > mix of 3 and 4.)
>>
>> Yeah, that would actually be the least invasive change, and would keep
>> "-i" just as it is. If we do anything except a simple, I think your (6)
>> makes the most sense.
>>
>> Let me see if I can make a patch.
>
> Here it is. I think this is the right thing to do. Junio?
I am not sure; my head spins when I see "tracked but ignored" (you have
a new one in the test) which is quite a bogus concept.
Does it mean "tracked but would be ignored if it weren't---perhaps it was
added by mistake?"?
^ permalink raw reply
* [PATCH] Changed timestamp behavior of options -c/-C/--amend
From: Erick Mattos @ 2009-10-30 19:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Erick Mattos
The code herein changes commit timestamp recording from a source in a
more intuitive way.
Description:
When we use one of the options above we are normally trying to do mainly
two things: one is using the source as a template and second is to
recreate a commit with corrections.
In the first case timestamp should by default be taken by the time we
are doing the commit, not by the source. On the second case the actual
behavior is acceptable.
Anyway this update creates new options for choosing the source timestamp
or a new one. And set as default for -c option (editing one) to take a
new timestamp and for -C option the source timestamp. That is because
we are normally using the source as template when we we are editing and
as a correction when we are just copying it.
Those options are also useful for --amend option which is by default
behaving the same.
Signed-off-by: Erick Mattos <erick.mattos@gmail.com>
---
Documentation/git-commit.txt | 10 ++++++++--
builtin-commit.c | 15 ++++++++++++---
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0578a40..27c61d2 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -10,6 +10,7 @@ SYNOPSIS
[verse]
'git commit' [-a | --interactive] [-s] [-v] [-u<mode>] [--amend] [--dry-run]
[(-c | -C) <commit>] [-F <file> | -m <msg>]
+ [(--old-timestamp | --new-timestamp)]
[--allow-empty] [--no-verify] [-e] [--author=<author>]
[--cleanup=<mode>] [--] [[-i | -o ]<file>...]
@@ -61,13 +62,16 @@ OPTIONS
-C <commit>::
--reuse-message=<commit>::
Take an existing commit object, and reuse the log message
- and the authorship information (including the timestamp)
- when creating the commit.
+ and the authorship information when creating the commit.
+ By default, timestamp is taken from specified commit unless
+ option --new-timestamp is included.
-c <commit>::
--reedit-message=<commit>::
Like '-C', but with '-c' the editor is invoked, so that
the user can further edit the commit message.
+ By default, timestamp is recalculated and not taken from
+ specified commit unless option --old-timestamp is included.
-F <file>::
--file=<file>::
@@ -134,6 +138,8 @@ OPTIONS
current tip -- if it was a merge, it will have the parents of
the current tip as parents -- so the current top commit is
discarded.
+ By default, timestamp is taken from latest commit unless option
+ --new-timestamp is included.
+
--
It is a rough equivalent for:
diff --git a/builtin-commit.c b/builtin-commit.c
index c395cbf..a924584 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -52,6 +52,7 @@ static char *edit_message, *use_message;
static char *author_name, *author_email, *author_date;
static int all, edit_flag, also, interactive, only, amend, signoff;
static int quiet, verbose, no_verify, allow_empty, dry_run;
+static int old_timestamp, new_timestamp;
static char *untracked_files_arg;
/*
* The default commit message cleanup mode will remove the lines
@@ -91,8 +92,10 @@ static struct option builtin_commit_options[] = {
OPT_FILENAME('F', "file", &logfile, "read log from file"),
OPT_STRING(0, "author", &force_author, "AUTHOR", "override author for commit"),
OPT_CALLBACK('m', "message", &message, "MESSAGE", "specify commit message", opt_parse_m),
- OPT_STRING('c', "reedit-message", &edit_message, "COMMIT", "reuse and edit message from specified commit "),
+ OPT_STRING('c', "reedit-message", &edit_message, "COMMIT", "reuse and edit message from specified commit"),
OPT_STRING('C', "reuse-message", &use_message, "COMMIT", "reuse message from specified commit"),
+ OPT_BOOLEAN(0, "old-timestamp", &old_timestamp, "reuse previous commit's timestamp"),
+ OPT_BOOLEAN(0, "new-timestamp", &new_timestamp, "regenerate timestamp, ignoring previous one"),
OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
OPT_FILENAME('t', "template", &template_file, "use specified template file"),
OPT_BOOLEAN('e', "edit", &edit_flag, "force edit of commit"),
@@ -396,7 +399,8 @@ static void determine_author_info(void)
name = xstrndup(a + 8, lb - (a + 8));
email = xstrndup(lb + 2, rb - (lb + 2));
- date = xstrndup(rb + 2, eol - (rb + 2));
+ if (!new_timestamp)
+ date = xstrndup(rb + 2, eol - (rb + 2));
}
if (force_author) {
@@ -763,11 +767,16 @@ static int parse_and_validate_options(int argc, const char *argv[],
die("You have nothing to amend.");
if (amend && in_merge)
die("You are in the middle of a merge -- cannot amend.");
+ if (old_timestamp && new_timestamp)
+ die("You can not use --old-timesamp and --new-timestamp together.");
if (use_message)
f++;
- if (edit_message)
+ if (edit_message) {
+ if (!old_timestamp)
+ new_timestamp = 1;
f++;
+ }
if (logfile)
f++;
if (f > 1)
--
1.6.5.2.102.g3ad0
^ permalink raw reply related
* Re: Bug#553296: gitignore broken completely
From: Jeff King @ 2009-10-30 19:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Klaus Ethgen, 553296, git
In-Reply-To: <7vaaz8lleg.fsf@alter.siamese.dyndns.org>
On Fri, Oct 30, 2009 at 12:41:27PM -0700, Junio C Hamano wrote:
> I've never understood the use of "ls-files -i" without -o, so in that
> sense, I have done 2. myself already long time ago.
>
> In other words, I do not really care that much, and the choice would be
> between "0. do not do anything---the patch in question was a bugfix for
> longstanding insanity" and your "4. -i without -o didn't make much sense
> but now it does and here is the new meaning".
OK, I think the patch I sent elsewhere in the thread should be applied,
then, as it should make you, me, and Klaus happy.
-Peff
^ permalink raw reply
* Re: Bug#553296: gitignore broken completely
From: Junio C Hamano @ 2009-10-30 19:41 UTC (permalink / raw)
To: Jeff King; +Cc: Klaus Ethgen, 553296, Junio C Hamano, git
In-Reply-To: <20091030173838.GB18583@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I am not sure simply reverting is the best choice; the patch does do
> something useful. And while it strictly breaks backwards compatibility
> on the output without "-i", the old behavior was considered a bug. But
> the "-i" behavior is useless now, so we need to figure out how to
> proceed. We can:
>
> 1. Revert and accept that the behavior is historical. Callers need to
> work around it by dropping --exclude* when invoking ls-files.
>
> 2. Declare "-i" useless, deprecate and/or remove. Obviously this is
> also breaking existing behavior, but again, I don't think that
> using "-i" actually accomplishes anything.
>
> 3. Revert for now, and then reinstate the patch during a major version
> bump when we are declaring some compatibility breakages.
>
> 4. Re-work "-i" to show tracked but ignored files, but still show all
> files when "-i" is not given at all.
>
> I think (4) preserves the benefit of the patch in question, but still
> allows your usage ("git ls-files -i --exclude-standard"). I do question
> whether that usage is worth supporting. Certainly I wouldn't implement
> it if I were writing git-ls-files from scratch today, but we do in
> general try to maintain backwards compatibility, especially for plumbing
> like ls-files.
>
> Junio, what do you want to do?
I've never understood the use of "ls-files -i" without -o, so in that
sense, I have done 2. myself already long time ago.
In other words, I do not really care that much, and the choice would be
between "0. do not do anything---the patch in question was a bugfix for
longstanding insanity" and your "4. -i without -o didn't make much sense
but now it does and here is the new meaning".
^ permalink raw reply
* Re: [RFC PATCH v4 03/26] pkt-line: Make packet_read_line easier to debug
From: Junio C Hamano @ 2009-10-30 19:35 UTC (permalink / raw)
To: Jeff King; +Cc: Shawn O. Pearce, Junio C Hamano, git
In-Reply-To: <20091030175741.GC18583@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Oct 29, 2009 at 02:58:29PM -0700, Shawn O. Pearce wrote:
>
>> Junio C Hamano <gitster@pobox.com> wrote:
>> > > The NUL assignment isn't about safe_read(), its about making the
>> > > block of 4 bytes read a proper C-style string so we can safely pass
>> > > it down into the snprintf that is within die().
>> >
>> > I knew and understood all of what you just said. static linelen[] is not
>> > about stack allocation. It's about letting the compiler initialize it to
>> > five NULs so that you do not have to assign NUL to its fifth place before
>> > you die. This removes one added line from your patch.
>>
>> Thanks, I get it now. Patch amended.
>
> I am just a bystander, so maybe my opinion is not worth anything, but
> personally I think you are obfuscating the code to save a single line.
Well, the comment was not a serious "you should do it this way" to begin
with. An extra assignment in the error path is not a big deal.
^ permalink raw reply
* Re: [RFC PATCH v4 03/26] pkt-line: Make packet_read_line easier to debug
From: David Brown @ 2009-10-30 19:31 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Jeff King, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <20091030191239.GF10505@spearce.org>
On Fri, Oct 30, 2009 at 12:12:40PM -0700, Shawn O. Pearce wrote:
> > If you really just want to initialize to zero, using
> >
> > char linelen[5] = { 0 };
>
> Bleh, I find that has hard to grok as what we have now. Perhaps my
> understanding of the relevant standards is incomplete, but I'd read
> that as linelen[0] = 0, but the other 4 positions are undefined
> and may be not be initialized.
C isn't that into undefined things. In this particular case, all
remaining values will be initialized to zero.
> I actually considered this one, but again I wasn't clear what would
> happen in the standard C library when we fed a string that wasn't
> actually NUL terminated. Is the library permitted to call strlen()
> before formatting? If so strlen() could SIGSEGV if we are unlucky
> and no NUL is present between our string and the end of the assigned
> memory region.
The linux manpage says "If a precision is given, no null byte
need be present". This text is copied verbatim out of the Posix
specification, so a C library that failed to handle this would be
non-compliant.
I think the %.4s is clearest, and is, in fact, somewhat
idiomatic.
David
^ permalink raw reply
* Re: [RFC PATCH v4 03/26] pkt-line: Make packet_read_line easier to debug
From: Jeff King @ 2009-10-30 19:30 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20091030191239.GF10505@spearce.org>
On Fri, Oct 30, 2009 at 12:12:40PM -0700, Shawn O. Pearce wrote:
> > I am just a bystander, so maybe my opinion is not worth anything, but
> > personally I think you are obfuscating the code to save a single line.
>
> Yup, me too. But I'm also willing to do what I need to get my
> patches included in git.git. Smart HTTP is something a lot of people
> have been waiting for. If the maintainer wants the bikeshed to be
> a particular shade of red, I'll paint it that way.
Yes, I too want it merged ASAP, so please Junio if you disagree with my
comments, ignore them. But I did want to point out that the proposed
"improvement" was making things worse, IMHO. So I feel like I was at
least bikeshedding his bikeshed, instead of starting my own. ;)
> > If you really just want to initialize to zero, using
> >
> > char linelen[5] = { 0 };
>
> Bleh, I find that has hard to grok as what we have now. Perhaps my
> understanding of the relevant standards is incomplete, but I'd read
> that as linelen[0] = 0, but the other 4 positions are undefined
> and may be not be initialized.
Your understanding is incomplete. :) C99, 6.7.8, paragraph 19:
... all subobjects that are not initialized explicitly shall be
initialized implicitly the same as objects that have static storage
duration.
I don't recall offhand whether that was the case in C89 or not.
But that being said, it was an attempt to make the meaning clear. If
it's not to you (who I consider at least reasonably competent ;) ), then
it failed.
> > die("protocol error: bad line length character: %.4s", linelen);
>
> I actually considered this one, but again I wasn't clear what would
> happen in the standard C library when we fed a string that wasn't
> actually NUL terminated. Is the library permitted to call strlen()
> before formatting? If so strlen() could SIGSEGV if we are unlucky
> and no NUL is present between our string and the end of the assigned
> memory region.
No, using a precision specifier for a string with no NUL is explicitly
allowed by the standard (and we use it elsewhere in git, IIRC).
> To me, my original version was the most clear, to me and anyone
> else who could ever possibly come by to read it. The "one extra
> line of code" is also only in an error condition which never occurs
> (but did once due to a bug in the HTTP code, which is why I added
> this patch to my series, to help debug it). Its not like this is
> a performance sensitive section of git that Linus is going to come
> back and overhaul.
Heh. You never know what performance item Linus will complain about. ;)
But yes, I am fine with your initial one, too. The drawback to it (and
my "%.4s") is not that one line of code is so unbearable, but that it is
one extra thing anyone using it as a string must do, and if they forget,
we get a horrible segfault.
Anyway, my complaint has been lodged. Junio is aware of the alternatives
and can pick whichever he wants.
-Peff
^ permalink raw reply
* Re: [PATCH] More precise description of 'git describe --abbrev'
From: Gisle Aas @ 2009-10-30 19:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7viqdwlnam.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 685 bytes --]
On Fri, Oct 30, 2009 at 20:00, Junio C Hamano <gitster@pobox.com> wrote:
> Adding an explanation like this so that nobody will be tempted to "fix"
> the example would be the best, I think.
Updated patch attached.
>
> [torvalds@g5 git]$ git describe --all --abbrev=4 v1.0.5^2
> tags/v1.0.0-21-g975b
>
> Note that the suffix you get if you type this command today may be
> longer than what Linus saw above when he ran this command, as your
> git repository may have new commits whose object names begin with
> 975b that did not exist back then, and "-g975b" suffix alone is not
> sufficient to disambiguate these commits.
>
>
[-- Attachment #2: 0001-More-precise-description-of-git-describe-abbrev.patch --]
[-- Type: application/octet-stream, Size: 2430 bytes --]
From 5f6b405fa24861ae89207d334ea2e11f383a7e4e Mon Sep 17 00:00:00 2001
From: Gisle Aas <gisle@aas.no>
Date: Thu, 29 Oct 2009 22:29:35 +0100
Subject: [PATCH] More precise description of 'git describe --abbrev'
Also adds a note about why the output in the examples might give
different output today.
Signed-off-by: Gisle Aas <gisle@aas.no>
---
Documentation/git-describe.txt | 17 +++++++++++++----
1 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index b231dbb..f8d91b1 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -44,7 +44,9 @@ OPTIONS
--abbrev=<n>::
Instead of using the default 7 hexadecimal digits as the
- abbreviated object name, use <n> digits.
+ abbreviated object name, use <n> digits or as many digits
+ are needed to form a unique object name. An <n> of 0
+ will suppress long format, only showing the closest tag.
--candidates=<n>::
Instead of considering only the 10 most recent tags as
@@ -68,8 +70,8 @@ OPTIONS
This is useful when you want to see parts of the commit object name
in "describe" output, even when the commit in question happens to be
a tagged version. Instead of just emitting the tag name, it will
- describe such a commit as v1.2-0-deadbeef (0th commit since tag v1.2
- that points at object deadbeef....).
+ describe such a commit as v1.2-0-gdeadbee (0th commit since tag v1.2
+ that points at object deadbee....).
--match <pattern>::
Only consider tags matching the given pattern (can be used to avoid
@@ -108,7 +110,7 @@ the output shows the reference path as well:
[torvalds@g5 git]$ git describe --all --abbrev=4 v1.0.5^2
tags/v1.0.0-21-g975b
- [torvalds@g5 git]$ git describe --all HEAD^
+ [torvalds@g5 git]$ git describe --all --abbrev=4 HEAD^
heads/lt/describe-7-g975b
With --abbrev set to 0, the command can be used to find the
@@ -117,6 +119,13 @@ closest tagname without any suffix:
[torvalds@g5 git]$ git describe --abbrev=0 v1.0.5^2
tags/v1.0.0
+Note that the suffix you get if you type these commands today may be
+longer than what Linus saw above when he ran this command, as your
+git repository may have new commits whose object names begin with
+975b that did not exist back then, and "-g975b" suffix alone is not
+sufficient to disambiguate these commits.
+
+
SEARCH STRATEGY
---------------
--
1.6.2.95.g934f7
^ permalink raw reply related
* Re: [RFC PATCH v4 03/26] pkt-line: Make packet_read_line easier to debug
From: Shawn O. Pearce @ 2009-10-30 19:12 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20091030175741.GC18583@coredump.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> On Thu, Oct 29, 2009 at 02:58:29PM -0700, Shawn O. Pearce wrote:
> > Junio C Hamano <gitster@pobox.com> wrote:
> > > I knew and understood all of what you just said. static linelen[] is not
> > > about stack allocation. It's about letting the compiler initialize it to
> > > five NULs so that you do not have to assign NUL to its fifth place before
> > > you die. This removes one added line from your patch.
>
> I am just a bystander, so maybe my opinion is not worth anything, but
> personally I think you are obfuscating the code to save a single line.
Yup, me too. But I'm also willing to do what I need to get my
patches included in git.git. Smart HTTP is something a lot of people
have been waiting for. If the maintainer wants the bikeshed to be
a particular shade of red, I'll paint it that way.
> When I see a static variable, I assume it is because the value should be
> saved from invocation to invocation, and now I will spend time wondering
> why that would be the case here.
Me too. I also can't grok the code I just wrote, for the same
reason, it just reads wrong. But who am I to argue with the guy
who is most likely going to be the one who needs to deal with it
in the future?
> If you really just want to initialize to zero, using
>
> char linelen[5] = { 0 };
Bleh, I find that has hard to grok as what we have now. Perhaps my
understanding of the relevant standards is incomplete, but I'd read
that as linelen[0] = 0, but the other 4 positions are undefined
and may be not be initialized.
> I think it would be even more clear to leave it as
>
> die("protocol error: bad line length character: %.4s", linelen);
I actually considered this one, but again I wasn't clear what would
happen in the standard C library when we fed a string that wasn't
actually NUL terminated. Is the library permitted to call strlen()
before formatting? If so strlen() could SIGSEGV if we are unlucky
and no NUL is present between our string and the end of the assigned
memory region.
To me, my original version was the most clear, to me and anyone
else who could ever possibly come by to read it. The "one extra
line of code" is also only in an error condition which never occurs
(but did once due to a bug in the HTTP code, which is why I added
this patch to my series, to help debug it). Its not like this is
a performance sensitive section of git that Linus is going to come
back and overhaul.
--
Shawn.
^ permalink raw reply
* Re: [RFC PATCH v4 11/26] Move WebDAV HTTP push under remote-curl
From: Clemens Buchacher @ 2009-10-30 19:06 UTC (permalink / raw)
To: Tay Ray Chuan; +Cc: git, Shawn O. Pearce, Daniel Barkalow, Mike Hommey
In-Reply-To: <be6fef0d0910300910me43c77fue6dcb6034dd0ea5b@mail.gmail.com>
On Sat, Oct 31, 2009 at 12:10:29AM +0800, Tay Ray Chuan wrote:
> Clemens, the following addresses your non-desire to remove the
> "unpacked refs" test in your earlier email
> <20091025161630.GB8532@localhost>.
> Now that the first push in "push to remote repository with packed
> refs" succeeds, the "unpacked refs" test is redundant.
How can the changed result of one test suddenly make another test redundant?
The two are testing different things.
> Since http-push
> in that first test already updated /refs/heads/master and /info/refs,
> 'git update-ref' incorrectly reverts the earlier push, and 'git
> update-server-info' is redundant.
No, 'git update-ref' correctly reverts the earlier push, so we can push again
and 'git update-server-info' is therefore necessary for the test to work
independently of its predecessors result.
Clemens
^ permalink raw reply
* Re: Bug#553296: gitignore broken completely
From: Jeff King @ 2009-10-30 19:05 UTC (permalink / raw)
To: Klaus Ethgen; +Cc: 553296, Junio C Hamano, git
In-Reply-To: <20091030184155.GC19901@coredump.intra.peff.net>
On Fri, Oct 30, 2009 at 02:41:55PM -0400, Jeff King wrote:
> > 6. Revert the patch and rework it so that it will only have effect if
> > there is no -i option on the command line. (That is similiar to a
> > mix of 3 and 4.)
>
> Yeah, that would actually be the least invasive change, and would keep
> "-i" just as it is. If we do anything except a simple, I think your (6)
> makes the most sense.
>
> Let me see if I can make a patch.
Here it is. I think this is the right thing to do. Junio?
-- >8 --
Subject: [PATCH] ls-files: unbreak "ls-files -i"
Commit b5227d8 changed the behavior of "ls-files" with
respect to includes, but accidentally broke the "-i" option
The original behavior was:
1. if no "-i" is given, cull all results according to --exclude*
2. if "-i" is given, show the inverse of (1)
The broken behavior was:
1. if no "-i" is given:
a. for "-o", cull results according to --exclude*
b. for index files, always show all
2. if "-i" is given:
a. for "-o", shows the inverse of (1a)
b. for index files, always show all
The fixed behavior keeps the new (1b) behavior introduced
by b5227d8, but fixes the (2b) behavior to show only ignored
files, not all files.
This patch also tweaks the documentation. The original text
was somewhat obscure in the first place, but it is also now
inaccurate (the relationship between (1b) and (2b) is not
quite a "reverse").
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-ls-files.txt | 6 ++++--
builtin-ls-files.c | 8 ++++++++
t/t3003-ls-files-exclude.sh | 8 ++++++++
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 021066e..625723e 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -48,8 +48,10 @@ OPTIONS
-i::
--ignored::
- Show ignored files in the output.
- Note that this also reverses any exclude list present.
+ Show only ignored files in the output. When showing files in the
+ index, print only those matched by an exclude pattern. When
+ showing "other" files, show only those matched by an exclude
+ pattern.
-s::
--stage::
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index c5c0407..c9a03e5 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -170,6 +170,10 @@ static void show_files(struct dir_struct *dir, const char *prefix)
if (show_cached | show_stage) {
for (i = 0; i < active_nr; i++) {
struct cache_entry *ce = active_cache[i];
+ int dtype = ce_to_dtype(ce);
+ if (dir->flags & DIR_SHOW_IGNORED &&
+ !excluded(dir, ce->name, &dtype))
+ continue;
if (show_unmerged && !ce_stage(ce))
continue;
if (ce->ce_flags & CE_UPDATE)
@@ -182,6 +186,10 @@ static void show_files(struct dir_struct *dir, const char *prefix)
struct cache_entry *ce = active_cache[i];
struct stat st;
int err;
+ int dtype = ce_to_dtype(ce);
+ if (dir->flags & DIR_SHOW_IGNORED &&
+ !excluded(dir, ce->name, &dtype))
+ continue;
if (ce->ce_flags & CE_UPDATE)
continue;
err = lstat(ce->name, &st);
diff --git a/t/t3003-ls-files-exclude.sh b/t/t3003-ls-files-exclude.sh
index fc1e379..d5ec333 100755
--- a/t/t3003-ls-files-exclude.sh
+++ b/t/t3003-ls-files-exclude.sh
@@ -29,4 +29,12 @@ test_expect_success 'add file to gitignore' '
'
check_all_output
+test_expect_success 'ls-files -i lists only tracked-but-ignored files' '
+ echo content >other-file &&
+ git add other-file &&
+ echo file >expect &&
+ git ls-files -i --exclude-standard >output &&
+ test_cmp expect output
+'
+
test_done
--
1.6.5.2.224.g22719.dirty
^ permalink raw reply related
* Re: [PATCH] More precise description of 'git describe --abbrev'
From: Junio C Hamano @ 2009-10-30 19:00 UTC (permalink / raw)
To: Gisle Aas; +Cc: git
In-Reply-To: <b48ea8a00910300101i38f2e4a8q2c2e014d2df1a150@mail.gmail.com>
Gisle Aas <gisle@aas.no> writes:
> On Thu, Oct 29, 2009 at 23:47, Junio C Hamano <gitster@pobox.com> wrote:
>> Gisle Aas <gisle@aas.no> writes:
>>
>>> Also make the examples show what 'git describe' actually outputs
>>> currently. I guess the default --abbrev value has been changed from 4
>>> to 7 at some point.
>>
>> Some are good changes, but I do not think the example with --abbrev=4 is.
>>
>> $ git describe 975bf9cf5ad5d440f98f464ae8124609a4835ce1
>> v1.3.2-216-g975bf9c
>> $ git describe 975b31dc6e12fba8f7b067ddbe32230995e05400
>> v1.0.0-21-g975b31d
>>
>> Next time somebody adds a new object whose name happens to begin with
>> 975b3 you would need to update the example output.
>
> Yeah, I know, but I don't think that's a big deal. So do you want an
> updated patch for that? We could either simply remove this example or
> make it use --abbrev=10 or something like that.
Not touching the example would be the simplest.
Adding an explanation like this so that nobody will be tempted to "fix"
the example would be the best, I think.
[torvalds@g5 git]$ git describe --all --abbrev=4 v1.0.5^2
tags/v1.0.0-21-g975b
Note that the suffix you get if you type this command today may be
longer than what Linus saw above when he ran this command, as your
git repository may have new commits whose object names begin with
975b that did not exist back then, and "-g975b" suffix alone is not
sufficient to disambiguate these commits.
^ permalink raw reply
* Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option
From: Junio C Hamano @ 2009-10-30 18:54 UTC (permalink / raw)
To: Charles Bailey
Cc: Scott Chacon, Junio C Hamano, Jay Soffian, git list,
David Aguilar
In-Reply-To: <20091030174421.GA21486@hashpling.org>
Charles Bailey <charles@hashpling.org> writes:
> On Wed, Oct 28, 2009 at 02:39:32PM -0700, Scott Chacon wrote:
>> p4merge is now a built-in diff/merge tool.
>> This adds p4merge to git-completion and updates
>> the documentation to mention p4merge.
>>
>> Signed-Off-By: Scott Chacon <schacon@gmail.com>
>> ---
>
> Acked-by: Charles Bailey <charles@hashpling.org>
>
> I'm aware that we haven't reached full agreement on the best way to
> make p4merge + git as Mac OS X friendly as possible, but Jay said that
> this patch is 'good enough' and I agree. If we go with this for now,
> we're not closing the door to further improvements.
Will queue; the peculiarity of MacOS X may be annoying, but the annoyance
is not limited to the topic of adding p4merge to this codepath.
^ permalink raw reply
* Bug#553296: gitignore broken completely
From: Klaus Ethgen @ 2009-10-30 18:23 UTC (permalink / raw)
To: Jeff King; +Cc: 553296, Junio C Hamano, git
In-Reply-To: <20091030173838.GB18583@coredump.intra.peff.net>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Am Fr den 30. Okt 2009 um 18:38 schrieb Jeff King:
> It was not exactly on purpose. The point of that change was that "git
> ls-files" should also show things mentioned in the .gitignore, because
> .gitignore has nothing to do whatsoever with tracked files.
(Mostly) true. It has when sending such patches that adds a ignored
file, to some other.
> But I simply forgot about "git ls-files -i", so changing it was an
> unintended side effect (and sadly, we don't seem to have any regression
> tests for it).
Could happen. No Problem with that as such.
> > That makes the whole sense of -i ad absurdum (I do not know how to tell
> > "ad absurdum" in english). With that patch there is no way anymore to
> > see if some ignored files are accidentally committed.
>
> Yes, with the current code "-i" serves no purpose unless you are using
> "-o".
But this is an other use case.
> > I have to use git often as frontend for that broken design (svn). So to
> > hold the ignores up2date I use "git svn show-ignore > .gitignore" But
> > many, many repositories have broken ignores so I have to check it with
> > "git ls-files -i --exclude-standard" to see if there is accidentally
> > ignored files. Well, that patch makes that impossible at all!
>
> Just to be clear: nothing is actually broken about ignores that cover
> tracked files. Ignores are (and have been since long before this patch)
> purely about untracked files. So there is no problem at all with:
See my comment above.
> The _only_ thing that respected such ignores was "git ls-files", and the
> point of the patch in question was to fix that.
Well ls-files is used to see such broken files. (Another example is if
you accidentally add a file which you (later) decide to be ignored. You
will have no change to find that files at all anymore.) With the patch
that use case of ls-files has been gone without a replacement.
> > So I think, this patch is a bug at all!
> >
> > I add Jeff (and Junio as he did the commit) to the Cc. @Jeff and or
> > Junio: could you please revoke that patch?
>
> I am not sure simply reverting is the best choice; the patch does do
> something useful. And while it strictly breaks backwards compatibility
> on the output without "-i", the old behavior was considered a bug. But
> the "-i" behavior is useless now, so we need to figure out how to
> proceed. We can:
>
> 1. Revert and accept that the behavior is historical. Callers need to
> work around it by dropping --exclude* when invoking ls-files.
>
> 2. Declare "-i" useless, deprecate and/or remove. Obviously this is
> also breaking existing behavior, but again, I don't think that
> using "-i" actually accomplishes anything.
>
> 3. Revert for now, and then reinstate the patch during a major version
> bump when we are declaring some compatibility breakages.
>
> 4. Re-work "-i" to show tracked but ignored files, but still show all
> files when "-i" is not given at all.
I have two more options:
5. Revert the patch and rework it to have a option to ignore or
respect the excluded files (Such as --use-excludes for example) or
respect them anyway if a --exclude* option is given on command
line.
6. Revert the patch and rework it so that it will only have effect if
there is no -i option on the command line. (That is similiar to a
mix of 3 and 4.)
I have nothing against the patch as such. But in the current form it
breaks at least one frequent used use case of ls-files.
> I think (4) preserves the benefit of the patch in question, but still
> allows your usage ("git ls-files -i --exclude-standard"). I do question
> whether that usage is worth supporting. Certainly I wouldn't implement
> it if I were writing git-ls-files from scratch today,
Well I had to search explicit for this use case as I had several
problems with ignored files in combination with svn. So I would. (if I
know the code good enough. And this problem to list such files did made
enough pain to me that I would go into the code to get it implemented.)
Regards
Klaus
- --
Klaus Ethgen http://www.ethgen.de/
pub 2048R/D1A4EDE5 2000-02-26 Klaus Ethgen <Klaus@Ethgen.de>
Fingerprint: D7 67 71 C4 99 A6 D4 FE EA 40 30 57 3C 88 26 2B
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
iQEVAwUBSusvI5+OKpjRpO3lAQqMhQgAnM7w+VqvUB+zFCJj8sCqO6QgcI+oup1z
dMwsv+5QU+S5UH7xdm/L6AhFJEsWsbpHzytVg7Rv3wCp8OzFiXmnjfUw+3JEvuLJ
+ggWHvFeKkTReDaRY00dafAQCP/8h0Yar6hVmXfdqGeiOnK0LeN5OXx0T3K51U/2
r8YOeNPZOzbaITcRaeIi5ghAEpyAgdqEw++f8h1xCRGo6DyncUIoexmFSG0pZS8q
tsyksW7q02LscTEp6PinQa7jhUN0xWJFTvpuCBWlfNgNTTffWt1xDjXTebwRKsYR
cT1ygEiI2+aZfrE47Fn91G9I+JjF5KYo7jt5UFNFlck9YsEKGPf22g==
=2ZcU
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: Bug#553296: gitignore broken completely
From: Jeff King @ 2009-10-30 18:41 UTC (permalink / raw)
To: Klaus Ethgen; +Cc: 553296, Junio C Hamano, git
In-Reply-To: <20091030182331.GC10671@ikki.ethgen.de>
On Fri, Oct 30, 2009 at 07:23:31PM +0100, Klaus Ethgen wrote:
> Well ls-files is used to see such broken files. (Another example is if
> you accidentally add a file which you (later) decide to be ignored. You
> will have no change to find that files at all anymore.) With the patch
> that use case of ls-files has been gone without a replacement.
I think your reasoning is a little bit circular. They are not actually
broken with respect to git. But they might represent an error on the
part of the programmer, and one which they would want to investigate.
You could also catch it by looking at diffs ("why is this file in my
diff, it is supposed to be ignored"), but I am not opposed to accessing
the information via git-ls-files (and I think you are right that the
query cannot be easily done any other way).
> I have two more options:
>
> 5. Revert the patch and rework it to have a option to ignore or
> respect the excluded files (Such as --use-excludes for example) or
> respect them anyway if a --exclude* option is given on command
> line.
I think that is basically equivalent to (1). There is already a way for
callers to avoid the bug, which is not to provide --exclude* at all. So
you are already setting that one bit of information in whether or not
you provide any excludes.
> 6. Revert the patch and rework it so that it will only have effect if
> there is no -i option on the command line. (That is similiar to a
> mix of 3 and 4.)
Yeah, that would actually be the least invasive change, and would keep
"-i" just as it is. If we do anything except a simple, I think your (6)
makes the most sense.
Let me see if I can make a patch.
-Peff
^ permalink raw reply
* Re: [PATCH] diff --color-words -U0: fix the location of hunk headers
From: Junio C Hamano @ 2009-10-30 18:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: markus.heidelberg, git
In-Reply-To: <alpine.DEB.1.00.0910301831190.5383@felix-maschine>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> The reason I did not do that was to avoid a full subroutine call, as I
> expected this code path to be very expensive.
This is only done for the "word diff" mode, and my gut feeling is that it
is not such a big issue. But you can always inline it if it is
performance critical.
The current structure shows how this code evolved. fn_out_consume() used
to be "this is called every time a line worth of diff becomes ready from
the lower-level diff routine, and here is what we do to output line
oriented diff using that line."
When we introduced the "word diff" mode, we could have done three things.
* change fn_out_consume() to "this is called every time a line worth of
diff becomes ready from the lower-level diff routine. This function
knows two sets of helpers (one for line-oriented diff, another for word
diff), and each set has various functions to be called at certain
places (e.g. hunk header, context, ...). The function's role is to
inspect the incoming line, and dispatch appropriate helpers to produce
either line- or word- oriented diff output."
* introduce fn_out_consume_word_diff() that is "this is called every time
a line worth of diff becomes ready from the lower-level diff routine,
and here is what we do to prepare word oriented diff using that line."
without touching fn_out_consume() at all.
* Do neither of the above, and keep fn_out_consume() to "this is called
every time a line worth of diff becomes ready from the lower-level diff
routine, and here is what we do to output line oriented diff using that
line." but sprinkle a handful of 'are we in word-diff mode? if so do
this totally different thing' at random places.
My clean-up "something like this" patch was to at least abstract the
details of "this totally different thing" out from the main codepath, in
order to improve readability.
We can of course refactor this by introducing fn_out_consume_word_diff(),
i.e. the second route above. Then we do not have to check "are we in word
diff mode" for every line of diff and that would give you even better
performance.
^ permalink raw reply
* Re: [PATCH 0/3] increase user-friendliness
From: Jeff King @ 2009-10-30 18:29 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.1.00.0910291159110.3687@felix-maschine>
On Thu, Oct 29, 2009 at 11:59:35AM +0100, Johannes Schindelin wrote:
> On Wed, 28 Oct 2009, Jeff King wrote:
>
> > Git has a reputation as being unfriendly to users. Let's fix that by
> > adding some features implemented by more user-friendly programs.
>
> So the bar for pirate patches has been raised to patch series now?
Well, I wasn't intending to do a series, but strangely when you ask in a
room full of git developers, "what ridiculous joke patch would you like
me to code this afternoon?", you get way more answers than you wanted. I
had to start rejecting suggestions. ;)
-Peff
PS Thanks to all who responded with ridiculous suggestions in your
reviews. I'm not going to bother keeping the thread alive by responding
to each, though. :)
^ permalink raw reply
* Re: [PATCH] gitignore: root most patterns at the top-level directory
From: Jeff King @ 2009-10-30 18:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmy3cys0f.fsf@alter.siamese.dyndns.org>
On Tue, Oct 27, 2009 at 11:03:28PM -0700, Junio C Hamano wrote:
> How does .cvsignore and .svnignore work? Don't they have the same issue,
> and perhaps worse as I do not recall seeing a way to anchor a pattern to a
> particular directory like we do in their .SCMignore files? And judging
> from the fact that they can get away with the lack of that "feature", this
> perhaps is not an issue in real life?
Happily, I did not remember how .cvsignore worked and had to go read the
documentation. :) The answer is that no, it does not have the recursive
feature in the root .cvsignore list at all. But it does apply the
repo-wide CVSROOT/cvsignore, the user's ~/.cvsignore, and the
environment's $CVSIGNORE to all directories. So it is safe from this
problem (though now that I think on it, I think I was once bitten by
something similar in the CVSROOT/cvsignore).
SVN implements this with "properties" on the directories. They are not
recursive at all. However, it also implements "global-ignores" which
applies everywhere.
So no, they don't have the same issue, because they explicitly split the
"everywhere" and "this directory" cases into two locations. We wouldn't
want to use .git/info/excludes for this, because we do want to support
the project shipping some global excludes itself.
> I am actually a bit reluctant to queue this, even though I most likely
> will, and then hope that we will think of a better solution later, at
> which time this file again needs to change.
I am mixed on it, as well. I did see it bite someone, but I think it's
very rare, and everyone who reads or touches the file will have to deal
with the ugliness every time. If you want to drop the patch, I will not
complain.
> For example, it crossed my mind that perhaps we can change the ignore
> rules so that a non-globbing pattern is automatically anchored at the
> current directly but globbing ones are recursive as before.
That makes some sense to me (and in fact when making the patch, it was
the rule of thumb I used). Though I think you might want to make it
"starts with glob" as the trigger for the rule. We have "git-core-*/?*",
which I would expect to still be anchored at the root.
> If we do so, there is no need to change the current .gitignore entires.
> You need to spell a concrete filename as a glob pattern that matches only
> one path if you want the recursive behaviour. E.g. if you have a Makefile
> per subdirectory, each of which generates and includes Makefile.depend
> file, you would write "Makefile.depen[d]" in the toplevel .gitignore file.
While clever, that use of '[d]' seems unneccessarily obscure to me. Why
not just give a wildcard for "any subdirectory of me" and do:
Makefile.depend
**/Makefile.depend
Since "**" is in common use in other systems, it's pretty clear (to me,
anyway, but then I am the one suggesting the syntax ;) ) what that
means.
-Peff
^ permalink raw reply
* Re: finding the merge of two or more commits
From: Jeff King @ 2009-10-30 18:04 UTC (permalink / raw)
To: Avery Pennarun; +Cc: E R, git
In-Reply-To: <32541b130910291434q1b068918x38c5aec543cb1c2a@mail.gmail.com>
On Thu, Oct 29, 2009 at 05:34:45PM -0400, Avery Pennarun wrote:
> On Thu, Oct 29, 2009 at 5:12 PM, E R <pc88mxer@gmail.com> wrote:
> > That is, I don't want to merge c1 and c2 myself, but I want to know if
> > someone else has merged c1 and c2, performed any conflict resolution
> > and committed the result.
>
> Try this:
>
> (git branch -a --contains c1; git branch -a --contains c2) | sort | uniq -d
>
> It's not exactly pretty, but it works.
That will tell you whether any branch contains both of them, which is
not exactly what the original poster asked for (though it may have been
what he meant :) ). To see if there is a specific merge of those two
commits, you can do:
git log --all --format='%H %P' | grep " $c1" | grep " $c2" | cut -d' ' -f1
-Peff
^ permalink raw reply
* Re: [RFC PATCH v4 03/26] pkt-line: Make packet_read_line easier to debug
From: Jeff King @ 2009-10-30 17:57 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20091029215829.GD10505@spearce.org>
On Thu, Oct 29, 2009 at 02:58:29PM -0700, Shawn O. Pearce wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > > The NUL assignment isn't about safe_read(), its about making the
> > > block of 4 bytes read a proper C-style string so we can safely pass
> > > it down into the snprintf that is within die().
> >
> > I knew and understood all of what you just said. static linelen[] is not
> > about stack allocation. It's about letting the compiler initialize it to
> > five NULs so that you do not have to assign NUL to its fifth place before
> > you die. This removes one added line from your patch.
>
> Thanks, I get it now. Patch amended.
I am just a bystander, so maybe my opinion is not worth anything, but
personally I think you are obfuscating the code to save a single line.
When I see a static variable, I assume it is because the value should be
saved from invocation to invocation, and now I will spend time wondering
why that would be the case here.
If you really just want to initialize to zero, using
char linelen[5] = { 0 };
should be sufficient (I think all of the compilers we care about support
such incomplete array assignments, right?).
I think it would be even more clear to leave it as
char linelen[4];
which declares how the data is _actually_ expected to be used, and then
do:
die("protocol error: bad line length character: %.4s", linelen);
-Peff
PS All of the proposals also suffer from the fact that die will truncate
broken output at a NUL sent by the remote. Probably OK, since we are
expecting ascii hex or some variant, and if we don't correctly print
what the remote said in a totally broken NUL-filled case, it is not that
big a deal.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox